diff --git a/.claude-plugin/marketplace.json b/.claude-plugin/marketplace.json index 5d7823c70..914eabdda 100644 --- a/.claude-plugin/marketplace.json +++ b/.claude-plugin/marketplace.json @@ -10,7 +10,7 @@ "plugins": [ { "name": "kbagent", - "version": "0.94.0", + "version": "0.95.0", "source": "./plugins/kbagent", "description": "DEPRECATED — install from keboola/ai-kit: /plugin marketplace add keboola/ai-kit && /plugin install kbagent@keboola-claude-kit — AI-friendly interface to Keboola Connection projects — explore configs, jobs, lineage, sync configs as files, manage dev branches, and debug SQL in workspaces", "category": "development" diff --git a/plugins/kbagent/.claude-plugin/plugin.json b/plugins/kbagent/.claude-plugin/plugin.json index 21a0994e0..822197443 100644 --- a/plugins/kbagent/.claude-plugin/plugin.json +++ b/plugins/kbagent/.claude-plugin/plugin.json @@ -1,6 +1,6 @@ { "name": "kbagent", - "version": "0.94.0", + "version": "0.95.0", "description": "AI-friendly interface to Keboola Connection projects — explore configs, jobs, lineage, sync configs as files, manage dev branches, and debug SQL in workspaces", "author": { "name": "Keboola", diff --git a/plugins/kbagent/skills/kbagent/references/gotchas.md b/plugins/kbagent/skills/kbagent/references/gotchas.md index eddaf3d93..70a686cdf 100644 --- a/plugins/kbagent/skills/kbagent/references/gotchas.md +++ b/plugins/kbagent/skills/kbagent/references/gotchas.md @@ -122,14 +122,29 @@ Versioning convention: counterpart** -- it did not exist when this rule was written and does not fall under it. - **The device-login panel's one-click link is conditional -- relay only what - is actually printed (since 0.92.0).** Alongside the verification URL and - code, the panel also prints a pre-filled `verification_uri_complete` link - ("Or open this link (code pre-filled):") whenever the stack's device - response includes one; a stack that returns an empty - `verificationUriComplete` prints neither that line nor the label. Don't - promise the user a one-click link unconditionally when relaying the panel - -- check what actually came back and relay only those lines. The URL and - code lines are always present regardless. + is actually printed (since 0.92.0).** The panel prints a pre-filled + `verification_uri_complete` link only when the stack's device response + includes one; a stack that returns an empty `verificationUriComplete` + prints neither the link nor its label. Don't promise the user a one-click + link unconditionally when relaying the panel -- check what actually came + back and relay only those lines. The verification URL and the code are + always present regardless. **The panel's order changed in v0.95.0**: the + pre-filled link is now the headline ("Open this link to finish signing in + (code already filled in):") and the type-it-yourself URL is the fallback + under it ("Or enter the code by hand at this URL:"). It used to be the + other way round, with the link last under "Or open this link (code + pre-filled):" -- so a relay that keys on that old label, or that assumes + the first URL in the panel is the manual one, now picks the wrong line. +- **`press c to copy` is absent exactly when an agent is driving (since + v0.95.0).** In a real terminal the device-login panel is followed by + `Press c to copy the link`, and the keypress is read inside the wait + between device-token polls. The option disables itself -- printing + nothing, output byte-for-byte as before -- in `--json` mode, when stdin or + stdout is not a TTY, or when no native clipboard command is installed + (`pbcopy` on macOS, `clip` on Windows, `wl-copy` / `xclip` / `xsel` / + `clip.exe` on Linux and WSL). A background shell is not a TTY, so the + login you drive will not offer it: relay the link itself, never "press c", + and do not wait for a copy that cannot happen. - **PKCE is the default; the device flow is a fallback, not a mode switch.** The CLI tries the browser (PKCE authorization-code) flow first and falls back to the RFC 8628 device flow ONLY on a *pre-exchange* failure: no diff --git a/pyproject.toml b/pyproject.toml index d7e4522b1..9c501c9a1 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -1,6 +1,6 @@ [project] name = "keboola-cli" -version = "0.94.0" +version = "0.95.0" description = "AI-friendly CLI for managing Keboola projects" readme = "README.md" requires-python = ">=3.12" diff --git a/scratchpad/reply-3984272224.md b/scratchpad/reply-3984272224.md new file mode 100644 index 000000000..8d3188935 --- /dev/null +++ b/scratchpad/reply-3984272224.md @@ -0,0 +1 @@ +Confirmed and fixed in e6954b4. `_envelope_holes` now applies the guard's own criterion -- a required key that is absent **or** `None` is a hole, and so is a blank `name` -- so for an ours envelope carrying `"name": null` the candidate is `null`, the reason lands in `warnings[]` ("carries no name -- ... no resolution candidate could be prefilled; author the resolution manually"), `diff --output` refuses instead of writing the file, and `classifiable()` excludes the side rather than classifying against `None`. Pinned in `test_merge_request_service.py` for both `None` and `" "`; the existing end-to-end round-trip pin (`diff --output` -> `resolve --resolved @file` unmodified) still passes. The RFC now says the constant alone did not prevent the drift and records the shared criterion. diff --git a/scratchpad/reply-3984272227.md b/scratchpad/reply-3984272227.md new file mode 100644 index 000000000..cf726c083 --- /dev/null +++ b/scratchpad/reply-3984272227.md @@ -0,0 +1,3 @@ +Confirmed -- and my earlier "both surfaces map `INVALID_ARGUMENT`" was wrong for the CLI side; thanks for checking it. Took the narrow fix in e6954b4: `create` and `update` pre-check `--external-id` against `MERGE_REQUEST_EXTERNAL_ID_MAX_LENGTH` and exit 2 before any call, the same way `request-changes` does for `--reason`. The service keeps the cap as the single rule (serve still answers 400 from it). Pinned for all three flag/command pairs, asserting no service method was called. + +I did not remap `INVALID_ARGUMENT` -> 2 in `map_error_to_exit_code`: that is a repo-wide behaviour change (~20 service raise sites across job/data-app/workspace/storage/repo-validate) with no test in the repo pinning either exit code for them, so it would be a silent behaviour change and belongs in its own PR if we want it. Consequence for this PR: `_require_in_conflict_set` and the absent-`branchFromId` case stay at exit 1 on the CLI (400 over serve), consistent with how every other group treats a service-raised `INVALID_ARGUMENT` today. The RFC records the choice and the alternative. diff --git a/scratchpad/zajca-round2-replies.md b/scratchpad/zajca-round2-replies.md new file mode 100644 index 000000000..e91a168ab --- /dev/null +++ b/scratchpad/zajca-round2-replies.md @@ -0,0 +1,11 @@ +# Zajca round 2 -- reply drafts (NOT posted; Martin approves) + +## Thread 3984272224 (`_envelope_holes` vs the resolve guard) + +Confirmed and fixed in e6954b4. `_envelope_holes` now applies the guard's own criterion -- a required key that is absent **or** `None` is a hole, and so is a blank `name` -- so for an ours envelope carrying `"name": null` the candidate is `null`, the reason lands in `warnings[]` ("carries no name -- ... no resolution candidate could be prefilled; author the resolution manually"), `diff --output` refuses instead of writing the file, and `classifiable()` excludes the side rather than classifying against `None`. Pinned in `test_merge_request_service.py` for both `None` and `" "`; the existing end-to-end round-trip pin (`diff --output` -> `resolve --resolved @file` unmodified) still passes. The RFC now says the constant alone did not prevent the drift and records the shared criterion. + +## Thread 3984272227 (`--external-id` exit 1 vs `--reason` exit 2) + +Confirmed -- and my earlier "both surfaces map `INVALID_ARGUMENT`" was wrong for the CLI side; thanks for checking it. Took the narrow fix in e6954b4: `create` and `update` pre-check `--external-id` against `MERGE_REQUEST_EXTERNAL_ID_MAX_LENGTH` and exit 2 before any call, the same way `request-changes` does for `--reason`. The service keeps the cap as the single rule (serve still answers 400 from it). Pinned for all three flag/command pairs, asserting no service method was called. + +I did not remap `INVALID_ARGUMENT` -> 2 in `map_error_to_exit_code`: that is a repo-wide behaviour change (~20 service raise sites across job/data-app/workspace/storage/repo-validate) with no test in the repo pinning either exit code for them, so it would be a silent behaviour change and belongs in its own PR if we want it. Consequence for this PR: `_require_in_conflict_set` and the absent-`branchFromId` case stay at exit 1 on the CLI (400 over serve), consistent with how every other group treats a service-raised `INVALID_ARGUMENT` today. The RFC records the choice and the alternative. diff --git a/src/keboola_agent_cli/changelog.py b/src/keboola_agent_cli/changelog.py index 7c471a60d..c24018650 100644 --- a/src/keboola_agent_cli/changelog.py +++ b/src/keboola_agent_cli/changelog.py @@ -24,6 +24,29 @@ # Ordered newest-first. Each value is a list of brief one-line descriptions. CHANGELOG: dict[str, list[str]] = { + "0.95.0": [ + "New (#772): `auth login`'s device-code panel now offers `press c to copy` for the " + "verification link, and leads with the pre-filled one-click link. The panel used to print " + "the manual URL + code first and the pre-filled `verification_uri_complete` link last, " + "under `Or open this link (code pre-filled):`; that link is now the headline and the " + "type-the-code URL is the fallback below it. The copy hint reads the single keypress " + "inside the wait the command already does between device-token polls -- no background " + "thread, no redraw -- and copies through a native clipboard command (`pbcopy`, `clip`, " + "`wl-copy`, `xclip`, `xsel`, or `clip.exe` on WSL). It disables itself and prints nothing " + "in `--json` mode, off a TTY, or when no clipboard command is installed, so a scripted or " + "agent-driven login is byte-for-byte unchanged.", + "Note (#768): building kbagent from source now requires uv 0.12.15 or newer " + "(`uv self update`) -- CI gates on `uv audit` instead of pip-audit (CLI-16). This is a " + "contributor-facing requirement: it comes from `[tool.uv] required-version` in this " + "repository's `pyproject.toml`, so installing or updating a published kbagent is " + "unaffected. The dependency audit also moved into its own non-blocking CI job, so an " + "advisory in a transitive dependency reports without failing an unrelated PR.", + "Note (#773): housekeeping with no user-facing change. #773 makes the `_url_copy` tests " + "type-clean (`ty` rejected reading a Rich console's `StringIO` back through " + "`Console.file`) and independent of the machine they run on -- two of them asserted that " + "no clipboard backend is detected, which is true only in a bare CI container and false on " + "macOS, Windows, WSL, or any Linux desktop.", + ], "0.94.0": [ "New (#759): a browser-login session (`auth login`) now works with nearly every command, " "not just the Storage and Manage paths (CLI-13). It reaches the Scheduler (`flow schedule` / " diff --git a/tasks/dmd-1899-findings-from-layer1.md b/tasks/dmd-1899-findings-from-layer1.md new file mode 100644 index 000000000..bd7443ae4 --- /dev/null +++ b/tasks/dmd-1899-findings-from-layer1.md @@ -0,0 +1,243 @@ +# Layer 2 findings surfaced while writing the Layer 1 RFC (DMD-1900 → DMD-1899) + +Context: `docs/merge-requests-layer1.md` (PR #708) is the Layer 1 command RFC, written against +`MergeRequestService` as it stands on `ms/dmd-1899/cli-layer-2` (PR #703). Designing the +commands surfaced seven things that belong to Layer 2 or to the docs Layer 2 owns. Two are +defects, one is a safety fact nobody has recorded anywhere, four are documentation gaps. + +Everything below was verified against the code / the local Connection checkout +(`/home/martin/keboola/connection`). Nothing here is a style preference. + +--- + +## 1. DEFECT — `get_config_diff` takes `branch_id` from the caller; `resolve_conflict` proves it must not + +`services/merge_request_service.py:742-744`: + +```python +def get_config_diff(self, alias: str, component_id: str, config_id: str, branch_id: int) +``` + +`resolve_conflict` (`:834-853`) deliberately does the opposite — its docstring says so: + +> The branch is NOT a parameter: it is derived from the merge request itself +> (`branches.branchFromId`), so the conflict-set guard and the branch being written to can +> never disagree -- a caller-supplied branch id could point the rebase at an unrelated dev +> branch that the guard never checked (rebase REPLACES; that would be silent data loss). + +That reasoning came out of the PR #703 self-review. `get_config_diff` has the identical shape +and never got the same treatment: it reads whatever branch the caller names, with no relation +to any merge request. + +**Why it matters for Layer 1.** The RFC's `diff` command is documented as "branch derived from +the MR". With the service as-is, Layer 1 cannot do that: when the user passes `--mr-id` +explicitly, the command layer holds only an integer and would have to call +`get_merge_request()` just to read `branches.branchFromId` — which additionally fires a +conflicts GET and a `verify_token` GET it has no use for (`:383-397`). Three round trips for +one integer, with the branch→MR relation re-derived in the command layer, i.e. business logic +in Layer 1. + +**Suggested fix (either works):** +- change the signature to `get_config_diff(alias, merge_request_id, component_id, config_id)` + and derive the branch via the existing `_branch_from_id_of` — full symmetry with + `resolve_conflict`; or +- keep the signature and add a small public accessor (`branch_from_id(alias, merge_request_id)`) + wrapping `_branch_from_id_of`, so Layer 1 gets the integer in one cheap call. + +The first is better: it makes the wrong call unrepresentable, which is the argument +`resolve_conflict`'s docstring already makes. + +## 2. DEFECT — `allowed_actions` is produced by `get_merge_request` only + +`derive_allowed_actions` is applied in `get_merge_request` (`:408`) and nowhere else. +`_enrich_row` (`:236-239`) adds **only** `derived_state`, so every other return — +`create_merge_request:463-468`, `update_merge_request:499`, `request_review:516`, +`approve:527`, `request_changes:546`, and each `list_merge_requests` row (`:314`) — carries no +action data. `merge` (`:649-664`) carries neither. + +**Why it matters.** A `--json` consumer of `create` (an agent deciding what to do next) gets +`derived_state` and nothing else. The natural next-step question — *can I merge now, or does +this need review?* — is answerable from the same state the row already carries, so the data is +free; it is simply not applied. + +**Suggested fix:** apply `derive_allowed_actions` inside `_enrich_row` so every command's JSON +carries it. One line, additive, no behaviour change for existing consumers. If list rows should +stay lean, at minimum apply it to the transition returns. + +## 3. SAFETY FACT, recorded nowhere — `autoMergeStrategy: immediately` makes the backend merge on its own + +Verified in Connection: + +``` +AutoMerge/AutoMergeCandidateRepository.php:44-47 + WHERE mr.state = :approved + AND (mr.autoMergeStrategy = :immediately + OR (mr.autoMergeStrategy = :scheduled AND mr.autoMergeAt <= :now)) + +AutoMerge/AutoMergeTickHandler.php:86 + $this->mergeProcessor->process($legacyRow, new SystemToken($candidate->projectId)); +``` + +It is the **same `MergeProcessor`** the `PUT …/{id}/merge` endpoint uses, driven by a background +tick under a system token. + +**It is polling, not an event on `approve`.** `AutoMergeScheduleProvider.php:24-26` is an +`#[AsSchedule('auto_merge')]` dispatching `AutoMergeTickMessage` every `AUTO_MERGE_INTERVAL` +(`config/services.php:265`, an env var — per-deployment, unknowable from source). Nothing else +dispatches that message anywhere in `src/`. `MergeRequestService` touches auto-merge only in +`applyAutoMerge` (`:226-236`, called from create `:63` and update `:107`), which merely +persists the strategy on the entity; the approve path has no auto-merge hook at all. So the +chain is: the MR reaches `approved` → the **next tick** merges it, after re-checking state, +strategy and schedule (`AutoMergeTickHandler.php:63-83`). + +Three consequences: + +- **Order does not matter.** The strategy can be set *before* the MR is approved — it sits on + the entity and fires the moment the state arrives. That is what makes the sequence below + work: neither write command is the thing that merges. +- On a non-SOX project with the default 0 required approvals there is no "last approve" at + all — `request_review` lands the MR straight in `approved` via skip_review, so that is the + trigger. `create(auto_merge_strategy="immediately")` + `request_review()` is enough; + `update(auto_merge_strategy="immediately")` on an already-approved MR is enough on its own. +- **The tick retries indefinitely.** A conflict or a "not ready" does not cancel the auto-merge + — `AutoMergeTickHandler.php:88-99` logs and retries every subsequent tick until it succeeds + (notifying once per blocked episode). Unlike a CLI `merge`, which is one shot and returns the + error, an auto-merge cannot be left to fail; it stops only when the strategy is set to `none`. + +For kbagent this also means `update --auto-merge-strategy immediately` returns 200 and the +merge happens later, invisibly: no job handle to await, nothing to report. + +Nothing in the CLI records this. `docs/merge-requests-notes.md:183` says only +"`AutoMergeStrategy` is exactly `immediately` | `scheduled` | `none`"; `create_merge_request` +(`:409-437`) and `update_merge_request` (`:461-484`) pass the field through without comment. + +**Why it matters.** It means two *write*-class operations can cause a production merge and a +source-branch deletion without anything ever calling `merge()`. Layer 1 handles the permission +consequence on its side (escalating the flag to `destructive`), but the underlying fact belongs +in the notes doc and in both service docstrings — a reader of `create_merge_request` currently +has no way to know the parameter is anything but metadata. + +**Suggested fix:** record it in `docs/merge-requests-notes.md` with the two citations above, and +add one sentence to both docstrings. + +## 4. DOCS — the wire field list in the notes doc is incomplete + +`docs/merge-requests-notes.md:111-114` records the `approvals` and `reviewers` sub-shapes but +never the full response. The authoritative serializer is +`Storage/MergeRequests/MergeRequestResponseProvider.php:86-117` (`getCreateMRResponseArray`), +and it emits: + +``` +id, creator{id,name}, title, description, state, +branches{branchFromId, branchIntoId}, +merge{mergedAt, mergerId, mergerName}, <- NESTED, not a flat mergerName +createdAt, <- the MR's own creation time +externalId, autoMergeStrategy, autoMergeAt, +approvals[]{approverId, approverName, createdAt}, +reviewers[]{id, name, status} +``` + +`getListMRResponseWithoutChangeLogArray` (`:132-139`) maps the same builder per row, so list and +detail have a byte-identical item shape; detail adds `changeLog` (`:122-127`) and, with +`?include=activityLog`, `activityLog`. + +Three of these are absent from every kbagent doc: the top-level `createdAt`, the `merge{}` +envelope, and `autoMergeStrategy` / `autoMergeAt` as **response** fields (they are documented +only as request parameters). The Layer 1 RFC got the field list wrong as a direct result — it +asserted there were no timestamps at all and that `mergerName` was top-level. + +**Suggested fix:** put the full serializer shape in the notes doc's wire-truth table with the +`MergeRequestResponseProvider.php:86-117` citation. + +## 5. DOCS — the scoped-token access claim needs a citation and a reconciliation + +`MergeRequestVoter.php:49-56` returns false when `$authorizedTokenRow->getAdmin() === null`, +and `MergeRequestService::requireMergeRequest` runs it for every `/merge-request/{id}` route — +including the read-only detail and conflicts actions. So a scoped Storage token (no admin +identity) gets 403 there, while `GET /v2/storage/merge-request` has no voter and works. + +This is *not* contradicted by `docs/merge-requests-notes.md:137-138` ("Reads (list, detail, +conflicts) are `#[AsReadOnlyAction]` with no role whitelist") — that is about **role** +whitelisting, a different axis from **admin identity**. But the two read together as a +contradiction, and the voter is cited nowhere in a doc set whose whole convention is file:line +evidence. + +Related: `docs/merge-requests-layer3.md:22,28` lists the notable failures of +`GET …/{id}` and `GET …/{id}/conflicts` as **404 only**. Every write row lists 403; the read +rows should too. + +Note also that `derive_viewer`'s docstring (`:210-234`) reasons about a scoped token reaching it +with no admin identity — via `get_merge_request` that path is unreachable, because the call +fails at the voter first. The None-flags branch is still correct as defence in depth; the +docstring just overstates how it is reached. + +**Suggested fix:** add the voter to the notes doc with its citation and one sentence separating +the two axes; add 403 to the two read rows in the Layer 3 doc; soften the `derive_viewer` +docstring. + +## 6. MINOR — `list` on a project without the feature is indistinguishable from an empty project + +`list_merge_requests` (`:295-333`) runs no pre-flight (correctly — the read endpoints are +ungated), so on a project lacking `branches-merge-requests` it returns HTTP 200 with +`count: 0`. Identical to a project that simply has no merge requests. Every subsequent write +then fails with `FEATURE_NOT_ENABLED`. + +**Suggested fix (optional, Layer 2 is the cheap place):** when the result is empty, include a +`feature_enabled: bool` from the already-warm `has_feature` cache, so the renderer can say +"merge requests are not enabled on this project" instead of "No merge requests". Purely +additive. + +## 7. MINOR — two vocabularies are private / inline, so Layer 1 must duplicate them + +- `_STATE_FILTER_VOCABULARY` (`:78`) is private, but the `--state` help text needs to enumerate + the accepted values. +- The take modes are inline literals (`:877`, `("ours", "theirs", "delete")`). + +The house precedent is public names imported by the command layer: +`from ..services.notification_service import KNOWN_EVENTS, SCOPE_PROJECT_WIDE, VALID_CHANNELS` +(`commands/notification.py:33`). + +**Suggested fix:** rename to `STATE_FILTER_VOCABULARY` and add a `TAKE_MODES` tuple, both +public. Pure rename, no behaviour change — and it lets Layer 1 pre-validate to exit 2 +(`INVALID_ARGUMENT`, matching every other bad-enum flag in the repo) instead of letting a typo +reach the service and exit 5. + +--- + +## Not Layer 2's problem — listed so it is not re-reported + +- `rebase_config_delete` (`client/configs.py:766-772`) has no `change_description`, so + `resolve --take delete --change-description "..."` would silently drop the text. Layer 1 + rejects the combination at exit 2; Layer 3 is fine as built (the wire body is + `{"version": N, "diff": {}}`). +- Permission classification, `FLAG_ESCALATIONS`, `--json` confirmation semantics and Rich + markup escaping are all Layer 1 concerns and are handled in the Layer 1 RFC. + +--- + +# Resolution (2026-08-28, ověřeno proti Connection, zapracováno) + +Všech 7 nálezů OPRÁVNĚNÝCH (ověřeno: AutoMergeCandidateRepository::findCandidates +WHERE state=approved AND (immediately OR scheduled<=now); AutoMergeTickHandler.php:86 +mergeProcessor->process(..., new SystemToken(...)); MergeRequestResponseProvider:86-117 +merge{} vnořené + createdAt top-level; MergeRequestVoter:49-56). Jediná korekce +odůvodnění: u #6 features cache NENÍ warm (list verify_token nevolá) — has_feature +stojí 1 GET, utrácí se jen při prázdném výsledku. + +1. FIX — get_config_diff(alias, merge_request_id, component_id, config_id): branch + derivována přes _branch_from_id_of, echo branch_id ve výsledku; closed MR → čitelná + VALIDATION_ERROR. Test navíc pinuje wiring (diff volán s branchFromId) — původní testy + po změně signatury FALEŠNĚ prošly (MagicMock spolkl posunuté argy), přepsáno. +2. FIX — _enrich_row přidává i allowed_actions (každý return: list/find/create/update/ + transitions; merge přidává k post-merge stavu). +3. DOCS — nová sekce "Auto-merge" v notes (s citacemi) + věty v docstringech create/update. + L1 eskalace flagu na destructive zůstává na DMD-1900. +4. DOCS — plný tvar MergeRequestResponse do wire-truth tabulky (merge{} vnořené, + createdAt, autoMerge* jako response pole; list==detail item shape). +5. DOCS — voter odstavec v notes (osa admin-identity vs. role), 403 na obou read + řádcích v layer3, derive_viewer docstring změkčen (defense in depth). +6. FIX — list: feature_enabled bool při prázdném výsledku (has_feature jen tehdy; + non-empty neplatí GET navíc — pinnuto testem). +7. FIX — STATE_FILTER_VOCABULARY public, TAKE_MODES public tuple (resolve je používá). + +94 testů zelených; commit na větvi PR #703. diff --git a/tasks/dmd-1899-l2-plan.md b/tasks/dmd-1899-l2-plan.md new file mode 100644 index 000000000..3e4f17212 --- /dev/null +++ b/tasks/dmd-1899-l2-plan.md @@ -0,0 +1,181 @@ +# DMD-1899 — Layer 2 (merge_request_service): plán a fronta rozhodnutí + +Brainstorm plán; pracovní poznámky s fakty jsou v `docs/merge-requests-layer2.md` +(+ `-notes.md` backend fakta, `-layer3.md` as-built klient, `-layer1.md` budoucí UX). +Tenhle soubor drží **stav brainstormu**: co je rozhodnuté, co je ve frontě. + +## Kostra: `services/merge_request_service.py` + +DI: `ConfigStore` + `client_factory` (jako každá service). Metody 1:1 k budoucím L1 +příkazům (DMD-1900): + +| Metoda | Nad čím z L3 | Co přidává L2 | +|---|---|---| +| `list_merge_requests(state=None)` | `merge_requests.list()` | client-side `state` filtr, derived state na řádku | +| `find_merge_request_for_branch(branch_id)` | `merge_requests.list()` | filtr `branches.branchFromId` (branch má nejvýš 1 MR); podklad pro volitelný `--mr-id` v L1 | +| `get_merge_request(id)` | `.get()` (+ `conflicts()`) | derived state + merge readiness, defenzivní čtení `changeLog` / budoucích server polí | +| `create_merge_request(...)` | `.create()` | feature pre-flight, resoluce source branch (bod 2) | +| `update_merge_request(...)` | `.update()` | pre-flight, empty-string-clears sémantika | +| `request_review(id)` | `.request_review()` | pre-flight, mapování 422 | +| `approve(id)` | `.approve()` | pre-flight, mapování 422 | +| `request_changes(id, reason=None)` | `.request_changes()` | pre-flight, mapování 422 (reason cap 1000 hlídá server) | +| `merge(id)` | `.merge()` (awaituje job) | pre-flight, 409 → nové ErrorCode (bod 3), post-merge cleanup (bod 5) | +| `list_conflicts(id)` | `.conflicts()` | obohacení o názvy konfigurací? | +| `get_config_diff(...)` | `configs.get_config_diff` | flatten `base/ours/theirs` (každá strana nullable) | +| `resolve_conflict(...)` | `rebase_config[_delete]` | `version` z `theirs.version`, kontrola conflict setu, případně take ours/theirs (bod 4) | + +Průřezově: feature pre-flight `has_feature(FEATURE_BRANCHES_MERGE_REQUESTS)` jen před +writes; dataclass návratové typy; nové `ErrorCode` členy + `docs/error-codes.md`. + +ROZHODNUTO (2026-08-26) — tvar service: domácí vzor beze změn. Jedna třída +`MergeRequestService`, plná `verb_noun` jména metod (call site má obecnou proměnnou +`service`, noun jinde není), čistá logika (derivace, flatten, kompozice payloadů) jako +module-level funkce, jeden soubor dokud loc-check nedonutí štípnout (čára: lifecycle vs +conflict resolution). Zapsáno v RFC: docs/merge-requests-layer2.md → „Service shape". + +## Audit pokrytí workflow (2026-08-26) + +Kostra ověřena proti stavovému automatu: každý volatelný endpoint má metodu, žádná +metoda nemíří na neexistující endpoint (skip_review/finish_review/rollback_merge/publish +jsou interní — správně chybí). Nálezy: + +- **Close/cancel MR — vyřešeno, žádná L2 práce.** Cancel endpoint neexistuje; UI Cancel + = creator zavolá request-changes na vlastní MR (kbc-ui helpers.ts, „reuses + request-changes under the hood"). Naše `request_changes(id)` to mechanicky umí. + Pojmenování (`merge-request close` alias?) je L1 otázka → DMD-1900. +- **Resoluce MR podle branche — rozhodnuto: jako všude jinde.** L1 používá standardní + `resolve_branch()` (`commands/_helpers.py:269`; --branch → active_branch_id). + Nový krok branch→MR patří do L2: +1 metoda `find_merge_request_for_branch(branch_id)` + (list + filtr `branches.branchFromId`; jednoznačné — branch má nejvýš 1 MR kdy). + L1: `--mr-id` explicitně → jinak resolve_branch + find. Ostatní metody dál berou mr_id. +- `get_merge_request` musí propustit `include_activity_log` (L3 to umí) — doplnit param. +- **Known limitation (scope out):** „co MR mergne" ve stavu `development` neumíme — + changeLog se plní až při request-review; UI si to počítá client-side porovnáním + branch vs. produkce (SendForReviewButton.tsx), žádný endpoint. Po submitu pokrývá + changeLog. + +## Fronta rozhodovacích bodů + +### 1. Status derivation — ROZHODNUTO (2026-08-26, plný inventář) +Finální inventář 4 derivátů (viz RFC sekce „Derived status", kompletní tabulky tam): +`derived_state` (7 hodnot, UI decision table) · `merge_blockers` seznam + `mergeable` +(conflicts/approvals/state; jen detail) · `allowed_actions` (mechanicky ze stavového +automatu, bez rolí/features — ty drží pre-flight) · `viewer` ({is_creator, has_approved} +z verify_token identity). Vzor GitHub API/MCP (mergeable_state, reviewDecision, viewer*). +Vše aditivní pole v --json, syrový `state` vždy vedle. Nejistota „bude to stačit?" krytá +polyfillem: změna = jedna čistá funkce + nové klíče, žádný breaking change. +Evidence pro backend: UI list badge vs. detail panel si odporují (Merged/Published, +Closed/Canceled, chybějící in_merge badge, panel nederivuje rejected/closed). + +Původní směr (2026-08-25/26): +Rozhodnuto (2026-08-25/26): +- Derivace stavů dlouhodobě patří na **backend** (3 klienti: UI/CLI/MCP = 1 logika). + Backend teď nemá kapacitu → CLI jede **polyfill pattern**: jedna čistá funkce + `derive_state(mr)`, server-first (`mr.get("derivedState")`), lokální fallback + = port decision table z UI, komentář s odkazem na Connection issue, smazat až + backend serializuje. Precedens: defenzivní čtení required-approvals-count (DMD-1969). +- Kanonická decision table = UI (`kbc-ui .../merge-requests/components/MergeRequestRow.tsx`, + `helpers.ts`): state + dvě derivace z `reviewers[]` — + **Rejected** (development + non-creator reviewer rejected), + **Closed** (development + creator self-rejection = UI „cancel" trik). + `in_merge` v UI badge chybí (Partial record). Konflikty do badge nevstupují. +- Dvě osy: `derived_state` (UI-kompatibilní enum) × `merge_readiness` + (mergeable / blocked_conflicts — jen v detailu, konflikty se na list nevolají). + Readiness NENÍ guard — autorita merge zůstává backend 409. +Zbývá: +- [x] Connection issue: DMD-1988 (serialize derived status; kontrakt = tabulky z RFC, + evidence UI nekonzistence; provázáno s DMD-1899/1969/1987) +- [x] enum hodnoty + jména polí v kbagent --json výstupu (v RFC) +- [x] sekce do docs/merge-requests-layer2.md (přepsána 2026-08-26) + +### 2. Source branch pro `create` — ROZHODNUTO (2026-08-26) +Stejně jako všude jinde: standardní `resolve_branch()` idiom. MR se zakládá na vybrané +větvi — explicitní `--branch` → jinak `active_branch_id`; když není ani jedno, chyba +(„pass --branch or run branch use"). Žádný další fallback. Výstup create jasně vypíše, +ze které větve MR založil. +### 3. Error mapping — ROZHODNUTO (2026-08-26) +Dva nové ErrorCode členy, mapované **v service** (jen ta ví, že 409 přišla z merge +endpointu; konfliktní tvar nemá string code, v generickém `http_base` nerozpoznatelný): +- `MR_NOT_READY_TO_MERGE` — 409 se string codem `storage.mergeRequests.notReadyToMerge` + (3 příčiny: merge lock / špatný stav / jiný MR běží; rozlišitelné jen textem → + jeden kód). Přechodné stavy → `retryable=True`. +- `MR_MERGE_CONFLICT` — 409 bez string codu na merge endpointu (tělo nese konfliktní + konfigurace). `retryable=False` + hint „run merge-request conflicts" v detailech. +Oba do `docs/error-codes.md` (`scripts/check_error_codes.py` vynucuje v CI). +Přesná jména kódů se můžou doladit při implementaci podle konvence enumu. +Backend protikus: DMD-1984 (audit string codů na všech MR endpointech; až konfliktní +409 dostane vlastní code, heuristika „409 bez codu" se v service zjednoduší). + +### 4. Conflict-resolution API — ROZHODNUTO (2026-08-26) +`resolve_conflict` se čtyřmi režimy, **všechny přes rebase** (jednotný mechanismus, +žádná nová L3 metoda): +- `take=theirs` — theirs strana diffu (produkce) → rebase s `theirs.version`, +- `take=ours` — ours strana (dev) → rebase s `theirs.version`, +- `delete` — `rebase_config_delete` (tombstone `{}`), +- vlastní tělo (JSON/@file) — pass-through s kontrolou conflict setu. +Edge case: ours strana s `isDeleted` → take=ours přechází v delete resoluci. +Vědomá odchylka od UI: tlačítko „Keep production version" volá reset-to-default +(config vypadne z MR); naše take=theirs rebasem nechá config v changeLogu a merge +zapíše obsahový no-op. Které chování je správně, ověřuje DMD-1987 — podle výsledku +se případně přidá L3 `reset_config_to_default`. +Hromadné `--all` ve v1 vynecháno (triviální smyčka v L1, přidat jde kdykoli). + +Prezentace 3-way diffu (rozhodnuto 2026-08-26): žádná tři okna — **klasifikace per +cesta**. L2 čistá funkce spočítá dva párové diffy (`base→ours`, `base→theirs`) a každé +dotčené cestě dá `changed_by: ours | theirs | both`; skutečný konflikt = jen `both`. +Tooling ukrást: `json_utils.compute_diff` má tu rekurzivní procházku — refactor na +strukturovanou variantu (per-path entries jako data), stávající string výstup zůstane +formatterem nad ní (užívá ho config_service). Human výstup (L1, → DMD-1900): tabulka +ve třech sekcích (Both changed / Only you / Only production), dlouhé hodnoty jen +„differs" + `--format full`. `--json`: entries + celé tři strany. Ruční merge bez +markerů: `diff --output resolved.json` (předvyplněno ours) → edit → `resolve --file`. + +### 5. Post-merge cleanup — ROZHODNUTO (2026-08-26) +Zrcadlo `delete_branch` (`branch_service.py:255-307`), po úspěšném merge: +- reset `active_branch_id` **jen podmíněně** (== mergovaná source branch; `was_active` + logika z delete_branch; NEkopírovat bezpodmínečný reset z `get_merge_url:349`), +- `branches.branchFromId` číst z MR payloadu PŘED merge (po publish je nullable), +- sync mapping přes `cleanup_branch_id_from_mapping` — helper polyká chyby čtení, + ale `save_branch_mapping` umí vyhodit IO chybu → merge() obalí celý cleanup + try/exceptem; chyba úklidu = warning ve výsledku, exit code zůstává úspěch, +- selhání merge = žádný cleanup (branch žije), +- workspaces na branchi neřešíme (server je maže s branchí; sirotci → workspace gc), +- výstup: „source branch is being deleted" (nikdy „is deleted"); strukturovaný + výsledek jako delete_branch (`was_active`, `mapping_cleanup`, `message`). + +### 6. Drobnosti — ROZHODNUTO (2026-08-26) +- Konstanta: rename `FEATURE_BRANCHES_MERGE_REQUESTS` → `BRANCHES_MERGE_REQUESTS_FEATURE` + (konvence souboru: STORAGE_BRANCHES_FEATURE, GLOBAL_SEARCH_FEATURE, PAYG_FEATURE; + dotčena 2 místa: constants.py:439 + docstring merge_requests.py:132). Rozhodl Claude + na Martinovo pověření. +- `mcp_parity.py`: BEZPŘEDMĚTNÉ — mapa, check skript i weekly canary smazány v 0.85.0 + (#609, 2026-08-19, po sepsání notes). Žádné parity záznamy se nikam nepřidávají; + historická mapa je docs/mcp-migration.md. Sekci v RFC označit jako zastaralou. +- SOX-fence komentář: implementační povinnost u pre-flightu (předpoklad „SOX projekt + nikdy nemá zároveň branches-merge-requests" + přísnost vůči čistému + protected-default-branch do chybové hlášky). + +## Implementace — HOTOVO (2026-08-26) + +Draft PR: https://github.com/keboola/cli/pull/703 (6 commitů po celcích: +json_utils DiffEntry → error plumbing (kódy + api_error_code v details + +rename konstanty) → derivace + admin_id na verify → reads → create/update/ +transitions → merge (409 map + cleanup) → conflicts/diff/resolve). +69 unit testů v tests/test_merge_request_service.py; make check zelený až na: +- changelog-check: base předchází releases 0.87–0.91 → vyřeší rebase na main, +- test_changelog_render: 2 faily reprodukovatelné i na čistém base (env). +Bez verze bumpu — changelog entry patří bump PR ve stacku s DMD-1900. + +## Proces +Bod po bodu: analýza + doporučení → Martin rozhodne → zápis sem + do RFC dokumentu +(`docs/merge-requests-layer2.md`, TEMP RFC commit, amend + force-push). Neřešit víc +bodů najednou. + +## MERGED (2026-09-03) + +PR #703 squash-merged do main jako `5281eef` (Martin). Remote větev smazána. Linear DMD-1899 → Done. +Cesta: 4 review kola (self, Opus wire-truth, Zajca ×3 vč. approvalu), 18 commitů, +6544 testů zelených. Non-blocking leftovers + odklady → `docs/merge-requests-layer2-followups.md` +na `ms/merge-requests-rfcs` (10f0ba7), převzít v DMD-1900. +Pozn. pro L1 rebase: vypustit 7b2bba9 (duplikát 7cd1855), sjednotit soft-failure klíč na `warnings`. +Žádný release — L2 leží na mainu dark; ven jde s L1 + release PR. diff --git a/tasks/pr-703-opus-wire-review.md b/tasks/pr-703-opus-wire-review.md new file mode 100644 index 000000000..7b1c865b7 --- /dev/null +++ b/tasks/pr-703-opus-wire-review.md @@ -0,0 +1,54 @@ +# Opus wire-truth review — PR #703 vs. reálné Connection API (2026-08-27) + +Zadání: ověřit každý wire předpoklad L2 proti zdrojákům Connection (poučení z nálezu #2 +prvního review — fixtures nejsou důkaz). 10 položek, verdikty: 7× CONFIRMED, 3× MISMATCH. + +## Souhrn verdiktů + +| # | Položka | Verdikt | +|---|---|---| +| 1 | MergeRequestResponse tvar; list == detail; activityLog aditivní | CONFIRMED (creator.id/reviewers[].id int, approverId string, branchFromId null přes FK ON DELETE SET NULL; detail/conflicts chtějí ADMIN token — voter; list ne) | +| 2 | Conflicts entry (bare array; componentId/configurationId string) | CONFIRMED (isDeleted = DEV strana) | +| 3 | Merge job results = MR dict s changeLog, state published | CONFIRMED | +| 4 | Merge 409 dva tvary | **MISMATCH** — konfliktní 409 MÁ code `storage.mergeRequests.validation` + konfliktní configy v `params.errors`; CLI else-branch mislabeloval cizí 409 a data zahazoval | +| 5 | Diff strany, rows round-trip, rebase kontrakt, nested JSON | CONFIRMED (3 ostré hrany: name:null projde presence checkem → 400; rebase chce config v dev branchi; forward-only version guard) | +| 6 | tokens/verify admin blok (id int; chybí pro non-admin) | CONFIRMED | +| 7 | Stavový automat pro allowed_actions | **MISMATCH** — `approve` z `approved` = 422 (transition jen in_review→in_review self-loop... resp. from: in_review); in_merge lock jen na SOX cestě; s 0 required approvals je approve 422 VŠUDE a in_review nedosažitelný | +| 8 | Create: 201 = MR response; non-default target 404; 1 MR/branch ever | CONFIRMED (+nepopsaná 422: creator nesmí být reviewer) | +| 9 | List bez server-side filtru | CONFIRMED (createdAt DESC, bez stránkování) | +| 10 | reviewers[].status + creator „cancel" trik | **MISMATCH** — creator nikdy nemůže být explicitní reviewer; explicitní revieweři stíní cizí decisions; decisions se zahazují bez `review_requested` kotvy, kterou skip_review NIKDY nezapíše ⇒ v default non-SOX projektu (0 approvals) jsou `rejected`/`closed`(self) NEDERIVOVATELNÉ z reviewers[] — status je vždy null. UI má TUTÉŽ vadu. Pravda žije v activity logu (changes_requested event, bez kotvy a stínění). | + +## Klíčové citace (Connection) + +- 409 serializace: `legacy-app/src/Common/Exception/JsonExceptionConverter/ExceptionConverter.php:99-125` + (error = message, code = stringCode, params = context → konfliktní configy v params.errors); + `MergeValidationException::getStringCode` → `storage.mergeRequests.validation` (`:174-177`). +- approve transition: `MergeRequestLifecycleStateMachine.php:47-50` (from: in_review only); + `AddApprovalGuard.php:32-56` (ne creator, ne duplicitně, canReceiveApproval); + `RequiredApprovalsCountProvider.php:136,166-169` (default 0 → canReceiveApproval vždy false). +- reviewers stínění: `MergeRequestResponseProvider.php:27-63`; kotva review_requested: + `MergeRequestActivityLogRepository::findLastReviewRoundDecisions:66-91`; skip_review bez + activity logu: `MergeRequestService.php:130-138`, `MergeRequestActivityEventType.php:25-28`. +- branchFromId null mechanismus: FK `ON DELETE SET NULL` (`init2025.sql:1596`) → nuluje ho až + async smazání branche, ne stav → `_branch_from_id_of` guard je racy (čerstvě published MR + může branchFromId ještě mít). +- Voter: `MergeRequestVoter.php:54-56` — detail/conflicts 403 pro non-admin token; list bez voteru. +- update privilege: non-SOX blokuje jen published/canceled (in_merge updatable); rebase in_merge + lock jen v `canAccessInProtectedBranch` (SOX) — `StorageRouteGuard.php:99-125,297,400-421`. + +## Zapracování (commit navazující na tenhle soubor) + +- #4: `_CONFLICT_CODE = storage.mergeRequests.validation`; explicitní match obou tvarů, None + fallback pro starší stacky, cizí 409 propadá nemapované; http_base nově nese `params` → + `details["api_error_params"]`; MR_MERGE_CONFLICT details nesou konfliktní configy rovnou. + Opraveny lživé komentáře (service, client, errors.py) + docs (error-codes, notes, RFC). +- #7: `approve` vyhozen z `approved` tuple; `update` přidán do `in_merge` (server ho tam bere); + docstring: AddApprovalGuard gating + „s 0 approvals je approve 422 všude, in_review + nedosažitelný". RFC decision table opravena. +- #10: fallback derivace ponechána, ale poctivě označena jako best-effort (docstring + + RFC): rejected/closed(self) fungují jen při required-approvals ≥ 1 + bez stínění. + Backend fix (DMD-1988) musí derivovat z activity logu / interního stavu — do issue + přidán komentář; activity-log derivaci v CLI neděláme (draží detail, na list nejde vůbec). +- #5 hrany: falsy `name` na take straně = contract violation; v resolved body = caller error. +- #1 extras: race komentář u `_branch_from_id_of`; viewer docstring (detail stejně chce + admin token — voter). diff --git a/tasks/pr-703-review.md b/tasks/pr-703-review.md new file mode 100644 index 000000000..4a097598a --- /dev/null +++ b/tasks/pr-703-review.md @@ -0,0 +1,183 @@ +# Review — PR #703 (merge-request Layer 2, DMD-1899) + +Self-review requested 2026-08-27. Branch `ms/dmd-1899/cli-layer-2`, 8 commits, 16 files, ++2545/−19. Verified locally: `ruff check` + `ruff format --check` clean, `make loc-check` +clean (service is under the 1000-line services budget), `scripts/check_error_codes.py` OK, +`make check-sentinel-guards` OK, **full suite green: 5768 passed / 61 skipped**. + +Overall: this is well above the bar. Layer separation is exact (no HTTP in L2, no logic in +L3), the derivation polyfill is pure + module-level as the RFC decided, every non-obvious +decision has a comment pointing at the backend evidence or the Linear issue, and the +`compute_diff` refactor is a genuine behaviour-preserving extraction. The findings below are +mostly about the conflict-resolution half, which is the part with real blast radius. + +--- + +## 1. `resolve_conflict` — the conflict-set guard does not cover the branch it writes to + +`merge_request_service.py:714`, guard at `:822` + +`_require_in_conflict_set` validates `component_id`/`config_id` against **merge request N's** +conflict set, but `get_config_diff` and the rebase then target the independently supplied +`branch_id`. Nothing checks that `branch_id == mr.branches.branchFromId`. + +Failure scenario: an agent (or the future L1 with a stale `active_branch_id`) passes +`--mr-id 7` and a `--branch` that belongs to a *different* dev branch. The guard says "yes, +that config is conflicting" — for MR 7 — and the rebase then **replaces** that config's +content in the unrelated branch (rebase replaces, per `rebase_config`'s own docstring). Silent +data loss, with the PR's advertised service-level guard reporting success. + +Recommended fix: drop `branch_id` from the signature and derive it from the MR +(`client.merge_requests.get(id)["branches"]["branchFromId"]`) — same "make the illegal state +unrepresentable in the signature" reasoning `get_config_diff`'s L3 docstring uses for the +default branch. It costs one GET, and `_require_in_conflict_set` already spends one. If you +want to keep the parameter, at minimum assert it against `branchFromId` and refuse on +mismatch. Consider the same for the public `get_config_diff` for symmetry (there it is only a +misleading read, not a write). + +## 2. The `missing`-keys check blames the caller for a server-shaped problem + +`:790` + +The message — *"A resolved body must spell out the full replaced content (rebase REPLACES): +missing rows."* — is correct for `resolved={...}` and wrong for `take=ours|theirs`, where the +caller supplied no body at all and the keys are missing from the **diff side**. + +Underneath that is a load-bearing unverified assumption: that `ConfigurationDiffResponse` +serializes `name`, `rows` and `configuration` on every side. `merge-requests-notes.md:201` +records the diff shape only as "`base`/`ours`/`theirs`, each nullable" — the per-side key set +is nowhere in the notes. If the live response omits `rows` (plausible — the diff is about +content), **both take modes are dead on arrival** and the unit tests cannot catch it, because +the `_side()` fixture is what defines the shape. + +Two things: split the error so a take-mode failure reads "the diff's `theirs` side carries no +`rows` — cannot compose a replace body" (and points at `resolve --file`), and verify the +per-side key set against connection, recording it in the notes table like every other wire +fact there. + +## 3. `take=theirs` has no deleted-side handling, `take=ours` does + +`:777` handles `ours is None or ours.get("isDeleted")` → delete resolution. There is no +mirror for `theirs`. Per `DefaultConflictValidator` +(`merge-requests-notes.md:52-62`) a conflict is *not* raised only when **both** sides are +deleted — so "production deleted it, dev changed it" is a live conflict shape, and the +conflicts entry itself carries `isDeleted`. In that case `take=theirs` either composes a +replace body out of a tombstone or trips finding #2's message. Symmetric handling is a +two-line change. + +## 4. `isDeleted` is excluded from the per-path classification — the code contradicts itself + +`_DIFF_CONTENT_KEYS` at `:620` omits `isDeleted`, while `resolve_conflict:777` reads +`ours["isDeleted"]` as decisive. Consequence: for a config deleted on one side, `changes` shows +only content movement and the deletion — the single most consequential difference — produces +no entry at all. The L1 rendering ("Both changed / Only you changed / Only production +changed") would then be actively misleading. + +Either add `isDeleted` to the tuple, or surface explicit `ours_deleted` / `theirs_deleted` +booleans on the `get_config_diff` result. I lean toward the booleans: a `changed_by` row for a +boolean flag reads oddly next to `configuration.limit`. + +## 5. `changed_by: "both"` also fires when both sides made the *identical* change + +`:690`. Two entries exist (each side differs from base) even when `ours.new == theirs.new` — +which in three-way terms is agreement, not a conflict. The RFC puts `both` in the "actual +conflict hotspots" section of the L1 table, so agreed changes get rendered as hotspots. +Cheapest fix: compare the two entries' `new` (and presence) and emit `both_same`, or keep +`both` and add an `agreed: bool`. + +## 6. Wire-id comparisons bypass the module's own `_same_id` + +`:537` — `project.active_branch_id == branch_from_id`, where `branch_from_id` comes straight +off the wire — and `:284` — `branchFromId == branch_id`. The module defines `_same_id` at `:65` +*precisely because* MR payload ids mix int and str (`approverId` is a string while `creator.id` +is a number). If `branches.branchFromId` ever serializes as a string, `was_active` silently +becomes `False` and a stale `active_branch_id` survives the merge, pointing at a branch the +server is deleting — the exact state the cleanup exists to prevent, and it would fail silently +(no warning path). `int()` coercion or `_same_id` at both sites. + +## 7. Feature pre-flight has no machine-readable code + +`_require_merge_requests_feature` raises a bare `ConfigError`, so a caller cannot tell +"feature not enabled" from "unknown project alias" — both arrive as the same shape with no +code. In-repo precedent goes the other way for exactly this case: `search_service.py:188` +emits `error_code: "FEATURE_NOT_ENABLED"`, and `PAYG_NOT_AVAILABLE` was added in #594 with the +rationale "a missing project feature is a configuration problem" so it *inherits the right +category instead of taking the default". Given the MCP/agent audience the docs keep invoking, +this one deserves a code. + +## 8. Smaller things + +- **`from ..config_store import ConfigError`** (`:26`) — the only service in the repo that does + not import it from `..errors`; 20+ others do. +- **`_default_branch_id`** (`:476`) is the fifth copy of the `isDefault`-scan over + `list_dev_branches()` (`config_service` ×2, `sync_service`, `workspace_service`, `lib.py`). + Good moment to hoist it onto `BaseService`. +- **`verify_token()` is always paid** (`:325`) even when the server already serialized + `viewer`, and even for terminal MRs where the flags are near-useless. Guard it with + `if not isinstance(mr.get("viewer"), dict)` so the polyfill's *cost* disappears with the + polyfill when DMD-1988 lands, not later. +- **`list_merge_requests` accepts any `state` string**; a typo returns `count: 0` with no hint. + The vocabulary is closed and known here — a validation error naming the accepted values beats + a silent empty list, and L2 is where the guard belongs. +- **Doc drift introduced by this PR**: `docs/merge-requests-layer2.md:96` still writes + `has_feature(FEATURE_BRANCHES_MERGE_REQUESTS)` and cites `client/tokens.py:200` (now `:233`); + `layer3.md:10, :121, :145` keep the old name, and `:145` lists the rename as an open nit that + this PR just closed. + +## 9. Tests + +69 service tests, tight and well-named; the "server field wins" test per derivation is exactly +the right shape for a polyfill that gets deleted later. + +- **Mid-file imports with 12 × `# noqa: E402`** (`:184-195`) — no precedent anywhere in + `tests/`; the three `src/` files doing this are commands with a specific reason. Hoisting + them to the top costs nothing. +- **`MagicMock()` without `spec`** for the client: this PR's whole value sits on the L3 seam + shipped in #556, and a renamed/removed L3 method would keep every test green. + `MagicMock(spec=KeboolaClient)` is cheap drift insurance at exactly the seam that matters. +- Gaps worth one test each: `_default_branch_id` with no default branch; `onto_version is None` + (null `theirs`); a take mode against a diff side missing `rows` (finding #2); and the + branch/MR mismatch of finding #1 once it is guarded. + +## 10. On the PR body's CI caveats + +- `make changelog-check` failure **confirmed** — the branch is **68 commits / 6 releases** + behind `origin/main` (0.87.0–0.91.0 missing). A rebase fixes it. Heads-up on churn in the + files this PR touches: `http_base.py` (+23/−4, `retry_safe` override), `json_utils.py` (+25, + `find_matches_in_json` appended right where `compute_diff` moved), `client/tokens.py` + (+73/−4, per-token last-used), `errors.py` (+8), `constants.py` (+69). Textual conflicts + only, nothing structural. +- The two `tests/test_changelog_render.py` failures **did not reproduce** — full suite green + here. Either narrow the claim to the environment that produces them, or drop it. + +--- + +# Resolution (2026-08-27, commit 51eaa2c) + +Vše zapracováno, force-pushed, PR body aktualizován. Po nálezech: + +1. **PŘIJATO** — `branch_id` odstraněn ze signatury `resolve_conflict`, odvozuje se z + `mr.branches.branchFromId` (`_branch_from_id_of`); published/canceled MR → čitelná chyba. + `get_config_diff` parametr nechán (nemá mr_id, jen čtení). +2. **PŘIJATO + POVÝŠENO** — wire tvar ověřen v connection: strany NEJSOU ploché, obsah je + vnořený pod `diff` ({version, isDeleted, diff:{name,description,changeDescription, + isDisabled,configuration,rows}}). Původní kód i fixtures byly špatně → přepsáno obojí, + fakt zapsán do notes wire-truth tabulky. Chybová hláška rozdělená (take = server + contract violation + odkaz na resolved body; resolved = caller error). +3. **PŘIJATO** — take=theirs smazané strany → delete resoluce, symetricky s ours. +4. **PŘIJATO** — `ours_deleted`/`theirs_deleted` booleany (None = strana neexistuje); + `changeDescription` vyloučen z klasifikace (commit message, ne obsah). +5. **PŘIJATO (varianta b)** — `agreed: bool` na `both` řádcích; changed_by zůstává 3hodnotový. +6. **PŘIJATO** — `_same_id` ve find; int koerce `branchFromId` v merge() (+ regresní testy). +7. **PŘIJATO (tvar dle precedentu)** — `ErrorCode.FEATURE_NOT_ENABLED` + `FeatureNotEnabledError(ConfigError)` + (vzor SessionAuthUnsupportedError); hodnota shodná se stringem v search_service. + Follow-up (nezapracováno): migrovat search_service na enum member. +8. **PŘIJATO vše** — import z ..errors; `find_default_branch_id` v services.base + migrace + config/sync/workspace (lib.py záměrně ne — SDK neimportuje services); verify_token guard + (přeskočen při serializovaném viewer); --state validace proti uzavřenému slovníku; + doc drift (layer2:96, layer3:10/121/145, tokens.py:302). +9. **PŘIJATO** — importy nahoru bez noqa; MagicMock(spec=KeboolaClient) + spec=MergeRequests; + gap testy: no-default-branch, null theirs, take side bez klíčů, closed MR, string wire id ×2, + state typo, viewer guard. Celkem 82 testů. +10. Rebase udělal Martin; changelog-check zelený. Render faily: claim v PR zúžen na + „v některých prostředích" (u mě reprodukovatelné na čistém mainu, u reviewera ne). diff --git a/tasks/pr-703-zajca-review-2.md b/tasks/pr-703-zajca-review-2.md new file mode 100644 index 000000000..48498c6e1 --- /dev/null +++ b/tasks/pr-703-zajca-review-2.md @@ -0,0 +1,60 @@ +# Zajcovo druhé review PR #703 (2026-09-02) — triage a zapracování + +Verdikt reviewera: NEEDS-WORK (1 blocking). Moje validace: **blocking + 3 non-blocking ++ 5 nitů OPRÁVNĚNÝCH a opravených; 1 non-blocking ODMÍTNUT s odůvodněním; 1 nit +odložen jako follow-up.** Commit `01fb0fa`. + +## BLOCKING #1 — isDisabled/description bez guardu → OPRÁVNĚNÝ, opraveno +Přesný zásah: guard kryl 3 z 5 replace klíčů a `is_disabled=bool(body.get(..., False))` +tiše re-enabloval vypnutý config (L3 docstring před tím explicitně varuje). Fix: +všech 5 klíčů povinných — name/rows/configuration/isDisabled odmítají absent-or-null, +`description` odmítá jen ABSENCI (explicitní null je rozhodnutí, pole je nullable — +jemnější než reviewerův návrh, který by nullové isDisabled pustil do bool()=False). +`is_disabled` teď forwarduje verbatim. Testy: missing isDisabled / missing description / +explicit-null description legal / take strana forwarduje isDisabled=True. + +## #2 — allowed_actions ukazuje na write, který selže → ODMÍTNUTO (s dokumentací) +Popsaný scénář je v aktuálním kódu NEMOŽNÝ: `feature_enabled: false` se emituje jen +při prázdném listu — a prázdný list žádné řádky s allowed_actions nenese. Reálná +zbytková varianta je projekt, kterému feature ODEBRALI po vzniku MRs (neprázdný list, +feature off) — tam by fix stál verify_token GET na KAŽDÝ neprázdný list kvůli vzácné +konfiguraci, a pre-flight každý pokus o write stejně zodpoví přesnou chybou +(FEATURE_NOT_ENABLED se SOX rozlišením). „Features cache already warm" v nálezu +neplatí (na neprázdné cestě se verify_token nevolá). Správný domov fixu je server-side +`allowedActions` (DMD-1988), který features zohlední. Rozhodnutí zapsáno do docstringu +`_enrich_row` (state-only = feature-blind, deliberate). + +## #3 — api_error_params bez changelog položky → OPRÁVNĚNÝ, částečně opraveno +- Unmasked pass-through je teď VYSLOVENÉ rozhodnutí v komentáři u raise-site + (server-authored error context, stejná důvěra jako message text). +- PR body: bump-PR poznámka rozšířena — changelog entry musí pokrýt + details.api_error_params(+_truncated) jako cross-cutting změnu výstupního kontraktu. +- Samotný changelog nejde napsat teď (feature PR nesmí, konvence). + +## #4 — null strana × neprázdná base = rozporné signály → OPRÁVNĚNÝ, opraveno +`_classify_three_way` při null ours/theirs vrací [] — side-level fakt nesou +ours_deleted/theirs_deleted (None = strana neexistuje), per-path řádky proti prázdné +náhražce se nefabrikují. Docstring + test aktualizovány. + +## #5 — change_description u take=delete tiše zahozen → OPRÁVNĚNÝ, opraveno warningem +Plumbing přes L3 NENÍ možný: changeDescription žije UVNITŘ diff envelope a delete +tombstone je přesně {"version": N, "diff": {}} — wire ho nemá kudy nést. Výsledek +nese warnings[] („change_description ignored: ... server records its default +message"). L1 navíc plánuje kombinaci odmítat na exit 2 (findings doc). + +## Nity +- History-narrating komentáře (Opus/Zajca/datumy, „Renamed from", „previously + copy-pasted") → vyčištěno; věcná fakta (verified against StateMachine) zůstala. +- search_service raw "FEATURE_NOT_ENABLED" → ErrorCode member. ✔ +- merge(): dva nezávislé try bloky pro cleanupy (+ test: failed config write + nepřeskočí mapping unlink). ✔ +- int(raw_branch_from) ValueError → `_coerce_branch_id` helper (non-numeric → + None → cleanup skip; v `_branch_from_id_of` → čitelná VALIDATION_ERROR). + test ✔ +- find_default_branch_id: isDefault entry bez id → přeskočí (None), žádný KeyError. ✔ +- cleanup_branch_id_from_mapping bez project scope → NEOPRAVENO zde: zděděno + z BranchService.delete_branch; oprava patří oběma call sites najednou + (follow-up kandidát), divergence v tomto PR by byla horší než status quo. + +## Stav +107 testů v test_merge_request_service.py, ruff/ty čisté, pushnuto `01fb0fa`, +odpověď postnutá na PR. diff --git a/tasks/ship-DMD-1900.md b/tasks/ship-DMD-1900.md new file mode 100644 index 000000000..bed836185 --- /dev/null +++ b/tasks/ship-DMD-1900.md @@ -0,0 +1,52 @@ +# ship-task: DMD-1900 — kbagent merge-request command group (Layer 1) + +Plan: docs/merge-requests-layer1.md @ 6fa0e55. Branch ms/dmd-1900/cli-layer-1 (in place, no worktree). +PR: #709 (draft; retitle from docs(rfc) to feat when implementation lands). Base: ms/dmd-1899/cli-layer-2 +until #703 merges, then retarget to main. + +- [x] Phase 0 — context +- [x] Phase 1 — plan (GATE) — approved 2026-09-03 +- [x] Phase 2 — branch: ms/dmd-1900/cli-layer-1, in place +- [x] Phase 3 — implement + - [x] 3.1 L2: resolution_candidate, warnings rename, get_merge_request_row (+tests, +layer2 doc) + - [x] 3.2 skeleton: _handle_error, target resolution, FLAG_ESCALATIONS + registry, cli.py wiring + - [x] 3.3 reads: list, detail, conflicts, diff + _merge_request_render.py + - [x] 3.4 writes: create, update, request-review, approve, request-changes, merge, resolve + - [x] 3.5 serve router + dependencies + OPENAPI_TAGS + test_server_router_calls + - [x] 3.6 branch merge deprecation; convention #17 surfaces; gotchas (vNEXT); endpoints-gen, skill-gen +- [x] Phase 4 — make check green +- [x] Phase 5 — self-review loop: Opus+Sonnet+code-review, all in-scope fixed @ f32d013 +- [x] Phase 6 — E2E: written (TestE2EMergeRequestLifecycle), skip-gated on feature + credentials; deferred (feature absent on kbagent-e2e, no local E2E creds) +- [x] Phase 7 — #735 (merged into L2 07:36 then undone by the L2 re-purification); replacement draft PR #736 vs ms/dmd-1899/cli-layer-2 @ d5291a5 +- [~] Phase 8 — #736 ready for review 2026-09-04 (Zajca, padak requested; CI green); monitor armed; Copilot threads → me, human threads → Martin + +## Hard stops +draft only · never merge/approve · --force-with-lease only (rejected → stop) · breaking existing tests → escalate · human review thread → ask + +## Log +- 3.1 done: d165c3d (112 service tests green) +- 3.2+3.3 done: d165c3d (35 CLI tests) +- 3.2+3.3 done: 0610a07 (35 CLI tests) +- 3.4 done: 4f98986 (73 CLI tests) +- split at soft ceiling: ea27d52 (common/reads/writes/render) +- 3.5 done: 7497fa1 (12 routes, 14 router tests, endpoints-check green) +- 3.6 done: f9eb01d (all #17 surfaces, skill-check + command-sync green; E2E written, credential- and feature-gated skip) +- Phase 4: make check exit 0 @ 9509c69 (6626 passed; ty warning on scripts/hatch_build.py is pre-existing) +- Phase 5: three reviews (Opus 14, Sonnet 4, /code-review 10 -- NOTE: /code-review took the branch arg as TARGET and reviewed the L2 branch; findings still real). All in-scope findings fixed + pinned; L2 design/refactor items deferred to #703 (listed in the pending commit message). +- BLOCKED: `git commit` fails -- commit signing via 1Password unavailable to this process ("failed to fill whole buffer"). Review-round changes are STAGED, not committed. Same cause makes tests/test_release_kbagent_ai_kit_sync.py fail (9) -- environmental, unrelated. Needs Martin: unlock 1Password (or decide on an unsigned commit). +- Phase 7: PR #735 opened (draft). #709 title/body accidentally edited then restored. +- rebased onto L2 7cd1855 (Zajca 3rd review); conflicts in service+tests resolved (L2 logic kept, warnings rename kept, duplicated test classes removed); make check exit 0, 6661 passed; force-with-lease pushed d5291a5 +- PR #736 opened (draft) after rebase; #735 is a dead MERGED record +- post-rebase alignment with L2 3rd review: 17cadc1 (error-code split, empty-envelope render, RFC wording); pushed +- rebased onto main (L2 squash 5281eef) via --onto; make check exit 0 (6663); force-with-lease pushed 9734b81; #736 retargeted to main, merge-order warning replaced +- RFC docs consolidated: ms/merge-requests-rfcs @ 2f62123 = truth (my L1 deltas merged in, followups F1 marked done); L1 rebuilt by Martin so docs live only in the first commit 445cd2f (identical to 2f62123); make check exit 0 on 2e2c808; NOT yet pushed +- followups F2-F5,F7 implemented (34f3240 -> replayed); RFC docs updated on ms/merge-requests-rfcs @ 647ddaf and L1 first commit rebuilt from it (d58fd98); make check exit 0 (6672); pushed +- rebased by Martin onto main 6e3d131 (#731): fixed ruff 0.16 ISC004 (render), md code-block formatting (RFCs @ d7547f2, first commit rebuilt), SERVE_COMMAND_MAP entries; make check exit 0 (6694); pushed +- Phase 8: ready for review @ df6e018, 13 commits +- Copilot Balanced round: 8 findings, all confirmed + fixed @ 2042c3b (E2E covers all 11 commands with a real conflict; L2 RFC rationale fixed on RFCs @ 76a2adb, first commit rebuilt); threads replied; pushed +- Zajca review (CHANGES_REQUESTED) + addendum: must-fix 1-5 implemented LOCALLY (not pushed, threads not answered -- human review hard stop); design push-backs pending Martin's decision +- 2026-09-11: static destructive model + auto-merge command (Martin's decision 2026-09-10); Zajca must-fixes 1-5 + non-blocking design push-backs resolved by the model change; RFCs @ d895f9e, L1 first commit rebuilt; make check green except changelog-check (pre-existing on main: 0.93.0/0.93.1 entries owed by a release PR); pushed 2e8dc0e (15 commits); monitor re-armed. PENDING: PR body refresh; replies to Zajca (human -> Martin decides). +- 2026-09-10: Zajca reply posted (issuecomment-5626484983), review re-requested from zajca; requested set now zajca,padak,soustruh +- 2026-09-11: Zajca round 2 (CHANGES_REQUESTED @ 2e8dc0e, 2 threads): 3984272224 `_envelope_holes` presence-only vs resolve guard -> mirrors guard (absent|None|blank name), pinned; 3984272227 `--external-id` cap exit 1 vs `--reason` exit 2 -> `_check_external_id` pre-check on create/update (narrow fix; global INVALID_ARGUMENT->2 remap NOT taken: ~20 raise sites in other groups), pinned x3. Commit 502cad5 -> replayed as e6954b4; RFCs @ fccb6b3 (both rules recorded), L1 first commit rebuilt e36ff48; make check green except changelog-check (pre-existing); pushed. PENDING Martin: approve replies (scratchpad/zajca-round2-replies.md), re-request review. Still owed: D (feature project-add on kbagent-e2e). +- 2026-09-14: Zajca round-2 replies posted (4002532057, 4002532194) under Martin's account; review re-requested from zajca (requested set: padak, soustruh, zajca). Waiting on round 3 / approval. Still owed: D (feature project-add on kbagent-e2e). +- 2026-09-15: rebased onto main fd280aa (releases 0.93.0/0.93.1/0.93.2 + CLI-12 telemetry + CLI-5 sync clone + CODEOWNERS). NO conflicts; L1's own files byte-identical (verified e6954b4..291026d touches only main's files); RFC docs still only in first commit 17494e2, identical to ms/merge-requests-rfcs @ fccb6b3. `make check` exit 0 -- changelog-check now PASSES (the pre-existing 0.93.0/0.93.1 debt was paid by main's release PRs). Force-pushed 291026d; Zajca's APPROVED survived the force-push. diff --git a/uv.lock b/uv.lock index 96ebb351f..9495c022f 100644 --- a/uv.lock +++ b/uv.lock @@ -465,7 +465,7 @@ wheels = [ [[package]] name = "keboola-cli" -version = "0.94.0" +version = "0.95.0" source = { editable = "." } dependencies = [ { name = "croniter" },