ORB-164: forbid the admin merge in the repo conventions (ORB-163 PR3a twin) - #445
Conversation
The prohibition already lives in AGENTS.md, which Codex reads. CLAUDE.md's Git workflow section is where a Claude session reads the absolute git rules, and it carried squash-merge-only without the admin-merge rule beside it. Names both raw API calls, not only the CLI flag: forbidding `gh pr merge --admin` alone leaves the REST and GraphQL merge paths open, which is the bypass shape ORB-163 exists to close. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Bojq5cLFZ87iBD8zhnoSWH
|
There was a problem hiding this comment.
Code Review: PR #445 (thomasluizon/orbit-api)
Scope: PR #445 — single-line edit to root CLAUDE.md's "Git workflow" section (+1/-1, one file, no source code touched).
Summary: The diff appends two governance sentences to CLAUDE.md that already exist verbatim/near-verbatim in AGENTS.md's "Guardrails you must not trip" list (added in the immediately preceding, already-merged PR #444): a ban on --no-verify/--no-gpg-sign, and a ban on admin-merging a PR in any of its three shapes (gh pr merge --admin, the raw PUT .../merge endpoint, the mergePullRequest GraphQL mutation). This closes a real inconsistency: the repo's own "opencode compatibility" rule (CLAUDE.md:52) states AGENTS.md defers to CLAUDE.md for conventions ("change behaviour here, never there"), but PR #444 introduced this behavior into AGENTS.md first, ahead of CLAUDE.md. This PR restores the correct direction of authority. No DTOs, endpoints, controllers, or tests are touched, so no rubric dimension beyond Correctness has a surface to check.
Findings
Critical: None. High: None. Medium: None.
Low / Info (not posted as blocking per the Signal gate, noted for completeness):
- The
PUT/GraphQL phrasing diverges slightly between the two files:CLAUDE.mdsays "no direct GraphQLmergePullRequestmutation,"AGENTS.md(AGENTS.md:34-35) says "no GraphQLmergePullRequestmutation" (no "direct"). Cosmetic only — same rule either way. - The PR body is explicitly marked "Work in progress... this body is replaced with the full report before review," and no such report has landed yet. Not a code defect, just a process note for the human.
Subagents
| Agent | Verdict |
|---|---|
| security-reviewer | N/A — gate ("any src/ code changed") did not fire; diff is CLAUDE.md only |
| contract-aligner | N/A — gate (DTO / Controller route / packages/shared type / endpoints.ts) did not fire |
Validation
Phase 7 skipped per orchestrator instructions (CI runs Build/Unit Tests separately).
Deferred — N/A dimensions & files not verdicted
- Dead/stale code, SOLID/clean-arch, No-workaround, Type safety, No console.log — N/A, no source code in diff.
- Comment policy (
ORBIT0001/no-comments.cjs) — N/A, governs code comments, notCLAUDE.mdprose itself. - DESIGN.md/AI-slop, Parity, i18n — N/A, no
apps/*files, frontend-only dimensions not applicable to an orbit-api-only diff. - Contract drift + backward-compat — N/A, no DTO/Controller/
packages/sharedhunk touched. - Backend hard rules — N/A, no
src/Orbit.*code changed. - FEATURES.md parity — N/A, no user-facing feature/screen/tool/gating change.
- Prior reviews — checked via
gh pr view --json reviews,comments: both empty; no earlier review to diff against, full single-hunk diff covered. - Security — checked directly (no need for the gated subagent): the change only restricts how a merge can happen (closes the admin-override bypass path), a net-positive governance control, not a risk.
- Cross-repo dimensions (contract-aligner field comparison,
packages/sharedbackward-compat) — not verifiable in CI (siblingorbit-ui-mobilerepo not checked out), but moot here since the diff never touches a contract surface.
Nothing changed-and-unexamined remains — one file, one hunk, fully verdicted.
What's good
Closes a genuine authority-direction bug in the repo's own docs (AGENTS.md had drifted ahead of CLAUDE.md); the added rule enumerates all three admin-merge bypass paths, not just the CLI flag; consistent with the existing terse style of the section.
Recommendation
Approve as-is. Optional non-blocking follow-up: make the PUT/GraphQL wording byte-identical between CLAUDE.md and AGENTS.md.



Harness-Lockstep-Peer: 667
ORB-164, the orbit-api half of ORB-163 PR3a. J3 place 3: the admin-merge prohibition in this repository's git conventions.
What changed
One line in
CLAUDE.md's## Git workflowsection, which is where a Claude session reads the absolute git rules. It already said squash-merge only and carried no admin-merge rule beside it, whileAGENTS.md(which Codex reads) has carried the full prohibition since PR1. The two entry docs now agree.The wording names both raw API calls alongside the CLI flag:
gh pr merge --adminPUT /repos/{owner}/{repo}/pulls/{number}/mergemergePullRequestmutationThat is deliberate and load-bearing. Forbidding only the flag leaves both API paths open, and whether a direct API call bypasses failing required checks for an admin actor while
enforce_adminsis false is still unverified, so the specification treats it as a live bypass.--no-verifyand--no-gpg-signwere also missing from this repository's line and are added in the same sentence.The prohibition is the PRIMARY control, not a reminder: the separate non-admin machine identity that would have closed the hole mechanically was declined on 2026-07-30, and
enforce_admins: trueis rejected because it cannot distinguish Thomas from an agent. The one legitimate exception is stated in the rule itself: if a merge genuinely needs an override, the agent STOPS and asks Thomas to merge it by hand.Decisions taken unattended
PreToolUseguard that refuses rawcodexandclaudeinvocations and the admin merge. It is NOT twinned here. The orchestrating session always opens in orbit-ui-mobile (D17); a Claude session inside an orbit-api worktree is exempt by that hook's own worktree rule anyway; and Claude Code hooks do not fire undercodex execat all, which this repository's ownAGENTS.mdalready states. Decisive: orbit-api has no hook harness, so an untested twin here would trade real coverage for the appearance of symmetry, and every new decision path must ship with its own harness case in the same commit.tools/context-budget.jsonand no such job in its workflows, so the byte cost that forced acontext:reseedlabel on the peer does not apply.AGENTS.mdwas left alone. It already carries the identical wording from PR1; re-stating it would be duplication, not coverage.Verification
CLAUDE.md, run locally before the commit and again by theDash Banjob. No em dash anywhere in the change, including this body and the commit message.reviewDecisionisAPPROVED.commit.oidequalsheadRefOid, checked with the same GraphQL read the peer pull request ships as A2. A PR-levelAPPROVEDsurvives every push and is not sufficient on its own; this one is anchored to the merged commit.Cross-repository
Paired with thomasluizon/orbit-ui-mobile#667 (ORB-163 PR3a: A1, A2, A3d, A4, A4b, J3 and ORB-165). Both bodies carry the reciprocal
Harness-Lockstep-Peer:directive on their first line, read back from the GitHub API after being set. This pull request merges FIRST, so the peer's lockstep reference resolves tomain.