feat(cli): merge-request command group -- Layer 1 over MergeRequestService (DMD-1900) - #736

Copilot AI left a comment

Copilot AI left a comment

...ilt, L1 as-implemented) [DMD-1899, DMD-1900]
The five documents, at the state that holds after PR #703 (Layer 2, merged
to main as 5281eef) and the Layer 1 implementation branch (PR #736):
- merge-requests-notes.md verified backend facts, all layers
- merge-requests-layer3.md the HTTP client, as shipped in #556
- merge-requests-layer2.md the service RFC + "Additions made for Layer 1"
 (get_merge_request_row, resolution_candidate,
 merge() cleanup_warnings -> warnings)
- merge-requests-layer1.md the command RFC after walking #703's review
 findings into it (seven decisions), plus the
 pointer to the follow-ups below
- merge-requests-layer2-followups.md non-blocking leftovers of #703 for L1
 (F1 done on L1; F2-F8 open)
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

...-1900]
Decided while walking PR #703's review findings into the Layer 1 RFC
(docs/merge-requests-layer1.md, "Layer 2 changes shipping with this PR").
Each exists so Layer 1 does not re-derive something the service knows.
- get_merge_request_row(alias, id): the row tier (_enrich_row) by id --
 one GET, no conflicts(), no verify_token(). list/find already return
 rows but only by branch; the sole by-id method was the detail, three
 round trips and a dependency on the conflicts endpoint that a write
 (request-review on an armed MR, the merge confirmation prompt) has no
 business inheriting. L3's merge_requests.get() was always this GET.
- get_config_diff -> resolution_candidate: the ours envelope through
 _DIFF_CONTENT_KEYS, description as an explicit null, changeDescription
 excluded; null when ours is absent/isDeleted. Composed in L2 so the
 `diff --output` prefill and the five-key replace guard in
 resolve_conflict share one constant -- a candidate built in L1 that
 dropped a null description would be a file kbagent writes and then
 refuses. Pinned by a round-trip test: candidate -> resolve_conflict
 unmodified.
- merge(): cleanup_warnings -> warnings. One soft-failure key for the
 group (resolve_conflict already used `warnings`); a renderer reading
 `warnings` must not silently drop the post-merge ones a user must act
 on. Specificity stays in the text.
Tests: 5 new (row tier cost pinned via assert_not_called on conflicts +
verify_token; candidate shape; null cases; round trip; warnings key).
L2 RFC updated in place.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

...MD-1900]
The `kbagent merge-request` group (hidden alias `mr`) over
MergeRequestService: wiring, the helpers every command shares, and
list / detail / conflicts / diff with their renderers. Writes follow.
Skeleton -- the Layer 1 decisions from docs/merge-requests-layer1.md:
- Target resolution (_resolve_target): --merge-request-id/--id optional;
 omitted -> resolve_branch() (--branch, else active branch) ->
 find_merge_request_for_branch(). Both flags at once is exit 2, not
 silent precedence. The resolution is reported on stderr in human mode
 and stamped into every --json result (merge_request_id, branch_from_id,
 resolved_from_branch) so a machine caller can assert on what was
 operated upon.
- One error handler (_handle_error), no per-command except: keeps
 FeatureNotEnabledError's FEATURE_NOT_ENABLED code, which now surfaces
 from the resolver behind every omitted id -- reads included.
- The destructive-under-json rule and the armed-auto-merge escalation
 helpers (used by the writes next): policy check first, then the
 explicit-target rule, which for state-derived escalations can only fire
 after resolution.
- warnings[] rendered identically everywhere; hint-next Rich-only.
Permissions: 11 registry entries (merge = destructive) + the serve-only
by-branch; FLAG_ESCALATIONS gains five state/flag-derived destructive
entries (arming auto-merge on create/update; request-review / approve /
resolve on an armed MR) with the Connection citations that justify them.
Renderers (_merge_request_render.py): every wire string escaped;
derived_state never raw state; list preserves server order and shows
optional columns only when populated; empty list tells feature-off apart
via feature_enabled; detail says the change log is empty by design in
development; diff checks the *_deleted flags BEFORE the table and
recommends the --take, since a null side yields zero rows; --output
writes the service's resolution_candidate verbatim and refuses when there
is nothing to prefill. Table value columns fold rather than crop so
--format full is actually full.
Also hoists parse_json_arg into _helpers (transformation.py had the
private copy; resolve is the third consumer).
35 CLI tests via CliRunner.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

..., resolve [DMD-1900]
The seven write commands, each routed through the shared skeleton
(_resolve_target, _handle_error, _stamp_target, warnings, hint-next).
Where a human says so, and where a policy does (docs/merge-requests-layer1.md):
- merge: statically destructive. Under --json the explicit-target rule
 fires BEFORE any lookup (no --merge-request-id/--branch -> exit 2); in
 human mode the active-branch fallback stays and the prompt names the MR,
 its title and the branch that will be deleted. --yes skips the prompt.
- create/update --auto-merge-strategy immediately|scheduled: arming is a
 delayed production merge, so it escalates to destructive
 (FLAG_ESCALATIONS), needs an explicit target under --json (--branch for
 create), prompts in human mode worded as arming, and warns afterwards.
 `none` is the disarm and escalates nothing. The strategy/--auto-merge-at
 pairing is validated at exit 2.
- request-review / approve / resolve on an ALREADY-armed MR escalate via
 the state-derived operation strings; the row comes free on the implicit
 path and via get_merge_request_row (one GET, never the detail) on the
 explicit path. Under --json with an implicit target this exits 2 only
 AFTER resolution -- deliberate, the information does not exist earlier;
 the error names the MR and the flag to pass. request-changes moves the
 MR away from approved and never escalates.
- update with no field flags is exit 2 (PUT {} is a server no-op).
 --reviewer-id is normalised to None when absent -- [] would clear the set.
- resolve: exactly one of --take/--resolved (exit 2 otherwise); --resolved
 parsed via the hoisted parse_json_arg and must be an object; a
 --change-description on a delete is the service's warning, not a Layer 1
 refusal (the implicit-delete collapse is only known after the diff).
Escalations are tested against the real engine (--deny-destructive ->
exit 6), not a mocked check. 38 more CLI tests (73 total).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

...ng [DMD-1900]
829 code lines after the writes landed -- exactly what the RFC predicted
for eleven commands at ~75 each. CONTRIBUTING lets a file sit over the
soft ceiling until the next PR adds to it, but a brand-new module born
over it is debt on day one, so split now:
- merge_request.py -- app, callback, the four reads; mounts the writes
- _merge_request_common -- what both need: option declarations, the ONE
 error handler, target resolution, the
 destructive-under-json rule, escalation, output
- _merge_request_writes -- the seven writes on their own Typer, mounted
 flat via register(app) so permission keys stay
 merge-request.* and --help lists one group
 (precedent: _storage_describe.register)
- _merge_request_render -- unchanged
A third module instead of reads importing writes (or vice versa): both
import common, only merge_request imports writes -- no cycle. Behaviour
unchanged; 73 CLI tests green; every module well under 800.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

...n-enforced [DMD-1900]
server/routers/merge_requests.py: twelve routes under /merge-requests --
one per CLI command plus GET /{project}/by-branch/{branch_id}, the
branch->MR resolver the CLI hides behind an omitted --merge-request-id
(no active-branch idiom over HTTP; registered as the serve-only
merge-request.by-branch). Declared before /{project}/{merge_request_id}
so FastAPI never tries to read 'by-branch' as an id. Skipped on purpose:
`diff --output PATH` -- GET .../diff returns resolution_candidate and the
caller writes its own file.
Every route declares Depends(require_permission(...)). Until now only
/auth/* did; here it is not optional -- the CLI classifies merge as
destructive and escalates arming auto-merge and the transitions on an
armed MR (FLAG_ESCALATIONS), and without the same checks over HTTP that
analysis would be decorative for serve callers. The static class is a
route dependency; the flag/state-derived escalations run in the route
body: arming in the create/update body -> check_or_raise the flag string;
request-review/approve/resolve -> one row GET (get_merge_request_row,
never the three-call detail) and check_or_raise when armed.
Caller errors (unknown state/take, both-or-neither take/resolved, empty
update body, broken auto-merge pairing) raise INVALID_ARGUMENT -> 400,
the REST twin of the CLI's exit 2. POST .../merge documents that it is
synchronous for up to 600 s.
Wiring: ServiceRegistry.merge_request, include_router, an OPENAPI_TAGS
entry (endpoints-gen would otherwise emit an untagged section), and
docs/web-server-endpoints.md regenerated (endpoints-check green).
14 router tests: kwarg parity per route (the drift this file exists to
catch) and the permission story over HTTP -- merge 403 under
deny_destructive, arming 403 while `none` passes, armed request-review
403 via the row tier, reads pass.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

...ranch merge; E2E [DMD-1900]
The silent-drift surfaces, all of them (nothing but check_command_sync
gates any of this):
- CLAUDE.md "All CLI Commands": the eleven signatures plus the block that
 matters most -- what may happen without a human saying so (merge is
 destructive; arming auto-merge is a delayed production merge; the
 --json explicit-target rule; the 0-approval facts; no `close`; the
 conflict loop; the error shapes; the feature-blind allowed_actions).
- commands/context.py AGENT_CONTEXT: a Merge Requests section after
 Branches, same content compressed for the agent.
- commands-reference.md: the group's cheat sheet.
- gotchas.md: one `(since vNEXT)` entry covering every non-obvious
 behaviour the RFC listed for it.
- keboola-expert.md: a tool-selection-matrix row with the anti-patterns
 (--auto-merge-strategy treated as metadata; --json merge with no
 target; a partial --resolved body; reading allowed_actions as
 feature-aware; approve on a 0-approval project).
- SKILL.md: triggers (merge request, mr, merge branch, auto-merge, review
 request), the description, the workflow link; decision table via
 `make skill-gen`.
- New merge-request-workflow.md: the short path, the --json path, the
 auto-merge table, the conflict loop, output semantics, the error table.
- branch-workflow.md points at the new group.
`branch merge` is deprecated with a CONDITIONAL pointer: it only builds a
UI URL (and unconditionally resets the active branch), but it works on
projects WITHOUT the feature, so it is not a 1:1 replacement. Behaviour
unchanged; `deprecation` key in --json, a warning in human mode.
E2E (convention #16): TestE2EMergeRequestLifecycle -- branch -> config on
the branch -> create -> list/detail/conflicts (id and --branch) -> approve
asserts the 422 -> bare --json merge exits 2 -> merge -> config in
production -> explicit teardown. GATED ON THE FEATURE: `list` on a
feature-less project answers feature_enabled: false and the suite skips
with the one-time enable command in the reason -- explicit, never silent.
The E2E project does not carry the feature today and this environment has
no E2E credentials; recorded in the ship ledger, not hidden.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

...r 1024 chars [DMD-1900]
check_version_gates: a vNEXT inside a heading would rewrite the anchor
slug on release; the three new sections carry the tag on their first body
line instead. test_skill_frontmatter: the description hit 1130/1024 chars;
kept the 'merge request' trigger, dropped the redundant ones, and
compressed three neutral list phrases (and -> /). No trigger word lost.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

Three reviews (Opus, Sonnet, /code-review) over the implementation; every
finding either fixed here with a pin, or recorded below as deferred.
High:
- _handle_error dropped exc.details, so the merge 409's conflict list and
 the truncation marker never reached --json, and the RFC's human render
 of MR_MERGE_CONFLICT (entries + "list truncated -- run conflicts") was
 never implemented. Both fixed; details flow through, the list renders
 escaped, the truncation line names no number.
- Unescaped wire/user strings in the ad-hoc console.print sites outside
 the renderer module (conflicts hint, diff hint, resolve success line,
 merge message): a `[/x]` in a config id raised MarkupError AFTER the
 rebase had landed server-side. Escaped everywhere.
Medium:
- branch_from_id was null on every explicit --merge-request-id path,
 beside a payload saying branches.branchFromId: 123. _stamp_target now
 derives it from the result (row branches, diff branch_id); conflicts
 fetches the row tier (one GET) since its result carries no branch.
- The auto-merge vocabulary was copied into the CLI and the router -- the
 exact drift the RFC forbids, and a SAFETY divergence (one surface would
 stop escalating an arming value the other still knows). Now
 AUTO_MERGE_STRATEGIES / AUTO_MERGE_DISARMED / validate_auto_merge_flags /
 arms_auto_merge live in the service module; both surfaces import them.
- next_step_hints silently dropped unknown action names; once DMD-1988
 serialises a camelCase vocabulary every hint-next line would vanish.
 Falls back to the raw name.
- Over `serve`, MR_MERGE_CONFLICT / MR_NOT_READY_TO_MERGE answered 502 with
 no details (retry-inviting, list dropped). app.py maps them to 409 and
 _format_error carries non-empty details.
- The service's two caller-mistake refusals (resolving/diffing a finished
 MR, a config outside the conflict set) were VALIDATION_ERROR -> 502 over
 serve; now INVALID_ARGUMENT -> 400. CLI exit code unchanged.
- resolve_conflict coerced a caller body's isDisabled with bool(), so a
 hand-edited "false" DISABLED the config on replace and returned 200.
 Non-bool is refused (the guard's refuse-don't-default policy).
Low:
- The armed-auto-merge warning is human-only (formatter.warning), no
 longer injected into the payload -- Layer 1 does not manufacture data.
- A hole in the ours envelope no longer becomes an explicit-null candidate
 that resolve then blames the caller for; get_config_diff returns
 resolution_candidate: null + a warning, and --output words the three
 null shapes apart (deleted / absent / envelope hole).
- parse_json_arg turns OSError (a directory, permissions) into the
 ValueError the callers expect; docstring stops claiming config.py's
 copy is gone. --output on an unwritable path is a readable exit 2.
- merge skips the row GET when no prompt will show (--yes / --json).
- --reason cap enforced on the REST route too. CLAUDE.md --state line
 stops hand-listing a subset of the vocabulary.
- FEATURE_NOT_ENABLED pinned on every command, as the RFC promised; the
 misnamed CLI "round-trip" test renamed (the real round trip is pinned at
 the service layer).
Deferred to PR #703 (Layer 2 design/refactor findings from /code-review,
which reviewed the L2 branch; too large for the tail of this run):
_classify_three_way missing `both` rows for nested-vs-parent edits; the
`or code is None` 409 fallback; SOX-project reads reporting
feature_enabled: false; the tuple return in http_base._bound_error_params;
the post-merge cleanup being a third copy of BranchService's; the
try/finally client idiom vs the context manager.
Tests: 6643 passed. The 9 failures in test_release_kbagent_ai_kit_sync
are environmental (git commit signing via 1Password unavailable to the
test process), unrelated to this diff.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

...D-1900]
Three second-order effects of rebasing onto 7cd1855:
- _branch_from_id_of: the L2 round split the message for an absent vs a
 non-numeric branchFromId, but the Phase-5 INVALID_ARGUMENT re-code had
 made both faults carry the caller's code. They blame different parties:
 absent = the caller is resolving a finished MR (INVALID_ARGUMENT, 400
 over serve); non-numeric = the server's payload (VALIDATION_ERROR).
- diff renderer: the L2 round makes _classify_three_way return zero rows
 for an empty-envelope side too, not only a null/deleted one. With no
 deletion flag set the renderer would have claimed "this conflict has
 cleared" while the service's warning beside it said "envelope hole".
 When there are no rows and the result carries warnings, say that no
 classification could be produced and let the warning explain.
- RFC: the five-key rule is the CALLER-body rule; a --take side composes
 an absent description as null (wire-identical -- the rebase omits the
 key), and a hole or non-boolean isDisabled there is a backend contract
 violation (VALIDATION_ERROR), never a caller error. The table said
 "refused when absent" for both paths.
Pinned: the two error codes; the no-rows-with-warning render.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

...F2-F5, F7) [DMD-1900]
docs/merge-requests-layer2-followups.md collects the non-blocking leftovers
of PR #703 that go through this PR. Per item:
- F2: _classify_three_way's docstring now says what is true -- the
 classifier is deliberately STRICTER than resolve_conflict (an empty
 envelope yields no rows here, a VALIDATION_ERROR there; collapsing it to
 the delete resolution would destroy a configuration). And the
 empty-envelope half finally has a test. Beyond the docstring: an empty
 envelope on EITHER side is now reported in `warnings` (_diff_warnings
 replaces the ours-only _candidate_warnings), so the diff renderer's
 "no rows + warnings" branch fires instead of claiming the conflict
 cleared -- which it would have done for a theirs-side hole.
- F3: merge() records the branch-id degradation structurally --
 `cleanup_skipped: true` + `branch_from_id_raw` -- and the message says
 "Source branch id could not be read; see warnings." instead of nothing.
 The CLI's merge renderer keys on the flag (a "Local cleanup skipped"
 line naming the raw value) and its hint-next points at branch reset +
 sync branch-unlink. A legitimate published-MR null carries no flag.
- F4: find_default_branch_id logs the skipped non-numeric entry instead of
 folding it into None silently (the callers then say "no default branch"
 for a project that DID report one). The `sync init` exits-0-with-empty-
 branches decision is a UX call left for Martin -- not changed.
- F5: the detail tier is feature-aware for free (has_feature after the
 verify_token it already pays): `feature_enabled` on the detail payload,
 a "Feature: not enabled" line in the panel, and hint-next refusing to
 recommend a write that cannot succeed. `list` stays feature-blind on
 non-empty results, as Layer 2 decided; docs say which is which.
- F7: config_service.py's last `if folder_branch_id:` truthiness test ->
 `is not None`; the positive assertion for "Active branch reset to main."
 on a successful reset; the 110-char docstring line rewrapped. The
 isDisabled-before-missing ordering is left as noted (house pattern).
Not in this PR: F6 (cleanup_branch_id_from_mapping project scope -- both
call sites, standalone PR) and F8 (test_changelog_render under
FORCE_COLOR -- main, unrelated).
9 new tests; make check exit 0 (6672).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

...f 0.16 ISC004) [DMD-1900]
main's ruff upgrade (9d823d5, >=0.16 default rule set) fires ISC004 on two
tuple items in the detail renderer. No behaviour change.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

...[DMD-1900]
main's #731 (command telemetry) requires every serve route in the
route->CLI-command map, enforced by
test_serve_telemetry::test_command_map_matches_every_route_exactly. The
twelve /merge-requests routes mirror their commands; by-branch is
serve-only (empty string, logged under its route label).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

...1900]
Eight inline findings, all confirmed against the code and fixed with pins:
- E2E setup skipped on ANY `merge-request list` failure, turning a crash or
 an auth regression into a green run. It now asserts success and skips
 only on feature_enabled: false -- the one gate the class documents.
- The E2E covered 6 of 11 commands. The scenario now manufactures a REAL
 conflict (config in production, branch inherits it, both sides change
 it) and walks every command: create, update, list, detail, conflicts
 (via --branch), diff (+ --output candidate), resolve --take ours,
 request-review (-> approved), request-changes (-> development), approve,
 the bare --json merge exit 2, merge, and the production content check.
- `approve`'s refusal was asserted as "any error but FEATURE_NOT_ENABLED";
 it now asserts API_ERROR with the 422 in the message.
- warnings[] text (backend / exception prose) reached Rich unescaped -- an
 unbalanced tag would raise MarkupError after the irreversible operation
 succeeded. Escaped.
- `diff --output` wrote with the platform encoding (a name outside a
 Windows code page would fail the promised round trip); utf-8 now. And the
 path was interpolated into Rich markup unescaped.
- A HOLED (partial) envelope still classified: content() omitted the
 missing key and the intersection reported it as that side's removal;
 holes on theirs were not warned about. A side missing any required
 content key is now unclassifiable on either side, and _diff_warnings
 names the holes for both (one shared _envelope_holes criterion feeds the
 classifier, the candidate and the warnings).
The eighth finding (a stale "code-less conflict" rationale in the L2 RFC)
is fixed on ms/merge-requests-rfcs (76a2adb); this branch's first commit
is rebuilt from it.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

@zajca zajca left a comment