docs(rfc): merge-request Layer 1 - command group RFC (DMD-1900) - #708
Merged
Conversation
Turns the working notes into an implementation-ready RFC for the `kbagent merge-request` group over MergeRequestService (DMD-1899, #703). Decided in this pass: - `--mr-id` is optional everywhere, resolved `--mr-id` -> `resolve_branch()` -> `find_merge_request_for_branch()`; `merge` is not exempt. - `merge` is classified `destructive`, so `--deny-destructive` lets an agent run the whole flow and hands only the last step to a human. - No `--wait`/`--timeout` on merge in v1 (L3 always awaits, 600 s) and no `resolve --all` (rebase replaces; conflicts are meant to be walked). - A full `server/routers/merge_requests.py` ships with the commands, plus a serve-only `by-branch` route; routers are not gated by CI, so a skip would reach users as an HTTP 404 with nothing red. Facts the analysis surfaced that shape the commands: - The MR serializer emits no timestamps, so no date column is possible and the renderer must preserve the server's `createdAt DESC` order. - `FeatureNotEnabledError` carries `FEATURE_NOT_ENABLED`; the common `except ConfigError` idiom would flatten it to `CONFIG_ERROR`. - An empty `--reviewer-id` list is sent as `reviewerIds: []` and clears the reviewer set -- it must be normalised to None. - `detail`/`conflicts` 403 on a scoped token while `list` works. - `approve` answers 422 in every state on a 0-approval project, and `request-review` lands directly in `approved` -- neither has a happy path to assert, and there is no `close` command for the same reason. E2E is deliberately left open: no project carries the feature, kbagent cannot provision one, and the happy path necessarily merges into production. The RFC records the proposed path and marks it unsettled. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
martinsifra
marked this pull request as ready for review
August 27, 2026 01:27
martinsifra
marked this pull request as draft
August 27, 2026 01:29
This was referenced Aug 27, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
RFC for the
kbagent merge-requestcommand group (Layer 1), overMergeRequestServicefrom#703 (Layer 2, DMD-1899).
Docs only — no code. Stacked on
ms/dmd-1899/cli-layer-2, so the diff is one file:docs/merge-requests-layer1.md. Retarget tomainonce #703 merges.What it decides
Layer 2 settles most of the surface — eleven service methods map to eleven commands with no
invention. This RFC decides what Layer 2 deliberately left to the caller:
merge-request+ hiddenmralias,_DEVpanel afterbranchcommands/merge_request.py+commands/_merge_request_render.py(split up front — 11 commands + 4 non-trivial renderers vs. the 800-line soft ceiling;output.pyis already at 1013/1000)--mr-idoptional everywhere:--mr-id→resolve_branch()→find_merge_request_for_branch().mergeis not exemptmergerisk--deny-destructivelets an agent run the whole flow and hands only the last step to a humanmergewait--wait/--timeoutin v1 (L3 always awaits, 600 s; a flag would mean re-reviewing L2+L3)resolve --allby-branchroutebranch mergeFacts the analysis surfaced
These shape the implementation and would otherwise be discovered at coding time:
Created/Updatedcolumn is possible; thelist arrives server-side
createdAt DESC, so the renderer must preserve that order — it isthe only chronological signal that survives.
FeatureNotEnabledErrorgets flattened by the house idiom. It is aConfigErrorsubclass carrying
FEATURE_NOT_ENABLED;except ConfigError → error_code=CONFIG_ERROR(and the shared
_handle_config_service_error) throws that away, leaving a--jsonconsumer unable to tell "merge requests are not enabled" from a bad alias.
--reviewer-idlist clears the reviewer set._optional_mr_fieldssendsreviewerIdswhenever it is notNone, so a Typer option yielding()silently wipesreviewers on any
updatethat did not mention them. Must be normalised toNone.detail/conflicts/diff/resolvebutlistworks —the list action has no voter.
approveanswers 422 in every state on a 0-approval project (its onlyfromplace,in_review, is unreachable becauserequest_reviewis auto-finished). Neither it norrequest-reviewhas a happy path to assert. Same root cause means there is noclosecommand: creator-request-changes is the UI's cancel but leaves
state=development, so aclosecommand would look like a no-op.Open
E2E is not settled by this RFC and is marked as such. No project carries
branches-merge-requests, kbagent cannot provision one (ManageClienthas no project create,and Connection's suite makes its own), and the happy path necessarily merges into production.
The RFC records the proposed path — enable the feature on the existing E2E project via
kbagent feature project-add, gate with aFEATURE_NOT_ENABLEDskip meanwhile — plus twoproperties worth knowing: the merge writes to production (explicit teardown required) and
mergetakes a project-wide lock, so concurrent runs collide.🤖 Generated with Claude Code