[mchwang]

Closes #184

Backs design tasks T6 and T12 (docs/designs/codeboost-plan-indexed-review.md, Implementation Tasks) with tests and ticks both. Recording real gh output for T12 found two product bugs, which are fixed here.

Bugs fixed (github/merge.ts #rules)

On this repository's own main, codeboost blocked every merge with "Required branch checks could not be read.", for these two reasons:

1. Missing integration_id. GitHub leaves integration_id out of a ruleset's required check when any app may report it ({"context":"test"}). codeboost required the key, so it set rulesKnown: false. A missing key now reads as null (no app pin), as GitHub documents. A malformed value still fails closed, and classic app_id is still required.

2. Ruleset-only branches. GET branches/<b> reports protected: true when only a ruleset applies. GET branches/<b>/protection then answers an admin token with 404 {"message":"Branch not protected","status":"404"}, and the branch read shows protection.enabled: false. codeboost threw "Branch protection is present but unreadable". Now, only gh's own 404 with that body, together with protection.enabled: false, counts as no classic protection. Everything else on a protected branch still fails closed:

   • the 404 {"message":"Not Found"} that a token without admin rights gets (checked on cli/cli trunk, gh 2.102.0);
   • a body that is not JSON, or another status;
   • an error that is not gh's own exit;
   • protection.enabled true or missing.

   What a non-admin token gets on a ruleset-only branch was not checked. If it is "Not Found", that branch still blocks as unknown rules.

Fix commits and regression tests (#184). History was kept rather than rewritten, so the fix is spread over three commits:

• ba80fb0: the first fix of both causes, committed together with the T6 tests.
• a19dcf4: requires status: "404" in the body.
• 031c64b: also requires the branch read's protection.enabled: false.

Regression tests that use GitHub's real recorded answers and fail without the fix (checked by reverting github/merge.ts to main):

• test/merge-recorded.test.ts "reads the ruleset's required checks as pending, then as passing at the same head";
• the three job tests in that file;
• test/merge.test.ts "for GitHub's "Branch not protected" answer with classic protection off, …".

Fakes that send what real gh/GitHub does not (audit; not changed here)

test/merge.test.ts:

• app: { databaseId: 10 } on check runs (lines ~918, ~1215, ~1428). gh's statusCheckRollup never carries app; this is App-pinned required checks always read as missing #176.
• integration_id: null on ruleset checks (lines ~922, ~1672, and requiredTestState's default). GitHub omits the key for an any-app check; it was never seen sending null. These paths now read both forms the same way.
• Branch answers { protected: true } with no protection object (6 tests). GitHub always sent protection in the recordings. The code reads it only after a "Branch not protected" 404, which these tests never reach.
• { type: 'merge_queue' } with no parameters (line ~922). GitHub's merge-queue rule carries parameters; the code does not read them.

test/merge-job.test.ts:

• The fakes inject RemoteMergeState, not gh output. They pair atomicBaseGuard: true with zero required checks and mergeQueue: false, a state the real gateway never produces: with nothing required, only a merge queue gives the guard. "merges at once when the required set was read and is empty" uses that state.

T6

Verify: "a rebase onto a breaking main blocks the merge; a head-only collaborator push recomputes review and reruns checks; matching SHAs still refresh both rule sources".

• Breaking main. test/pre-merge.test.ts "fails the job when the moved base itself breaks the command check on the rewritten head". The check fails only on a head that holds the moved base's new file, so it would pass on the reviewed head.

• Collaborator push. The design (merge-handoff.md, Outcomes: review-required) reruns checks on the next job after the person reviews the new head, not automatically on the push. "reruns command checks on a collaborator head after review when the base did not move" walks that path:

  1. A first job passes the check on the rewritten head, and GitHub refuses the merge.
  2. A collaborator pushes; the base does not move.
  3. The next job recomputes the review.
  4. The person assigns and approves the new lines.
  5. The job after that runs the check again on the new head and merges (exit 0) or fails (exit 1).

• Matching SHAs. test/merge.test.ts:

  • "reads both rule sources again on a fresh inspection when base and head are unchanged";
  • "refuses a guarded merge when the %s adds a failing required check after a cached ready status", for a ruleset and for classic protection.

  In the recorded job test, every inspection reads both rule sources.

T12

Verify: "recorded-output tests for zero checks, pending then passing, and timeout". Title: "Read required checks from branch rules; zero checks passes; ignore reviews".

• Recorder. scripts/record-merge-gh.ts runs one inspection through the production gateway and the real gh. It saves each call's argv, exit status, stdout and stderr, as runWithInput reports them. A branch read keeps only name, protected and protection, which drops commit authors' emails. A fresh recording of PR 104 reproduces its fixture exactly.
• Recordings. test/fixtures/gh/ holds three, all from this repository with gh 2.102.0:
  • zero-checks: PR 104, into a branch that no rule covers.
  • checks-pending: this PR at ba80fb0, while test and real-docker ran.
  • checks-passing: the same head, after both checks passed.
• Replay tests. test/merge-recorded.test.ts replays the recordings through the real parser and through the merge job. Each test asserts that every gh call had a recorded answer, apart from the job's gh pr merge, which is faked. The job waits while the checks are pending and merges once they pass. At the time limit, with the checks still pending, it ends blocked.
• One changed field. For the job tests, the recorded ruleset's strict policy is turned on, and the test asserts that this is the only change. The repository's real ruleset is not strict, so a job against it stops at once for the missing atomic base guard (T7). That stop is also tested with the unchanged recordings.
• Zero checks passing a job. This needs a merge queue: with nothing required, a strict policy gives no guard. That case stays covered by injected states (test/merge-job.test.ts, "ends queued when the merge queue accepted the head").
• Ignore reviews. "ignores required reviews in both rule sources and reads only their required checks".
• App-pinned required checks always read as missing #176 stays open under build step 4. "reads a requirement pinned to an app as missing against gh's app-less check runs" pins its fail-closed reading. No fixture copies its fake app.databaseId.

Follow-ups (not in this PR)

• Non-admin tokens. Classic required checks could be read without admin rights. The branches/<b> answer already carries protection.required_status_checks, which read-only tokens can see. This would remove the admin-only /protection read.
• Load flake. test/review.test.ts "keeps review load available when continuation reconciliation refuses" timed out once at 15 s under full-suite load. It passes alone. This PR does not touch that file.

Validation

Each gate is reported separately. Validated head: eeafaf7.

• Focused, at eeafaf7: test/merge-recorded.test.ts and test/merge.test.ts, 197 passed. In test/pre-merge.test.ts, the new tests (3 cases) passed.
• Affected suites, at eeafaf7 (8 merge and pre-merge files, run locally at load average ~88): 503 passed, 1 failed. The failure was the existing "stops an active command check when the operation-wide deadline expires": its deadline ran out before the check attempt started (attempt undefined). It passes alone, and CI test passed it. This is recorded as a load failure.
• Type check: npx tsc --noEmit is clean.
• Local full suite: one early run at an intermediate head did not cover the final code. Its only non-recording failure was the load timeout named under Follow-ups. CI test is the full-suite gate.
• CI on eeafaf7: test passed (the full Vitest suite), and real-docker passed. Mergeable: MERGEABLE, CLEAN.
• Mutations: each fix and each new test was checked by reverting or breaking the code it covers. The test failed every time.

Review record

• Self-review. Mutations were run before every push.
• Independent review. Each round used a fresh general-purpose agent on the full diff against 42abf4a. Rounds 1–17 found issues, all fixed:
  • Round 11 made the 404 check also require protection.enabled: false, and added a real breaking-base test.
  • The other rounds tightened tests: the rerun after an earlier pass, the review-rules test, the non-admin 404, status === "404", and asserting that every call was recorded. They also corrected claims and the notes on what fakes leave out.
  • Nothing was declined.
• Round 18 (head eeafaf7, base 42abf4a): no findings. That clean pass authorizes the Copilot request.
• Copilot (head eeafaf7): "Approval recommended … No open issues", with no inline comments and 0 review threads.

Review-lesson audit

Finding type	Classification
Tests that would pass with the feature broken (rerun after an earlier pass, non-strict guard order)	Covered by "A test MUST fail when the feature it tests is broken" (CLAUDE.md)
Evidence and comment claims broader than the tests (breaking main, non-admin token, #176 blocking)	Covered by "Treat every behavioural claim … as something to verify" (CLAUDE.md, Review readiness)
gh fakes missing a version or a left-out note, and hand-written shapes that differ from real gh	Covered by "A fake of an external tool … must give, for every call the code reads, each part of the answer" (CLAUDE.md, Review readiness)
Personal emails in recorded fixtures	One-off: the recorder now drops them, and ci.md already says to read each recording before committing it

No AGENTS.md or CLAUDE.md change is needed.

🤖 Generated with Claude Code

[Copilot]

🟢 Approval recommended

The narrowly scoped parser fixes preserve fail-closed behavior, have focused regression coverage, and have no unresolved review findings.

No open issues 🎉

What changed in this PR

Adds regression evidence for T6 and T12 and fixes ruleset-only branch handling in the guarded merge gateway.

Changes:

• Accepts omitted ruleset app IDs and confirmed absence of classic protection while preserving fail-closed behavior.
• Adds recorded GitHub responses and tests for refreshed rules, changed heads, check progression, and timeout.
• Updates implementation documentation and task completion evidence; #176 remains open.

File	Description
test/​pre-merge.test.ts	Tests breaking-base and collaborator-head check reruns.
test/​merge.test.ts	Tests rule refresh, review exclusion, and protection parsing.
test/​merge-recorded.test.ts	Replays recorded responses through inspection and merge jobs.
test/​fixtures/​gh/​zero-checks.json	Records a branch without required checks.
test/​fixtures/​gh/​recorded.ts	Loads and replays recorded CLI responses.
test/​fixtures/​gh/​checks-pending.json	Records pending required checks.
test/​fixtures/​gh/​checks-passing.json	Records passing required checks.
scripts/​record-merge-gh.ts	Captures inspection responses with branch-data filtering.
github/​merge.ts	Fixes ruleset app IDs and classic-protection absence detection.
docs/​implementation/​guarded-merge.md	Documents parser behavior and permission limitations.
docs/​implementation/​ci.md	Documents fixture recording and replay.
docs/​designs/​codeboost-plan-indexed-review.md	Marks T6 and T12 verified with evidence.
docs/​architecture.md	Updates merge verification status.

_{Review effort: Balanced}