fix(workflow): overlap verification and converge review repairs - #307
Conversation
External review — Codex — head 9b70ce6 — fullNo actionable regressions were identified in the workflow instructions or associated tests. All 57 tests across the three changed test files passed. Provenance: independently launched Review started approximately 19:16:50 UTC and was collected complete by 19:18 UTC on 2026-09-10, while hosted CI run 34519397579 was still in progress. No approval or merge action delegated to the reviewer. |
External review — Claude Code — head 9b70ce6 — fullIndependent review — PR #307, head
|
| Claim | Check | Result |
|---|---|---|
| Isolated worktree, owned by me | git worktree list |
/var/tmp/agentrig-shipping-review.EJfUAB/claude is a distinct entry (detached). A sibling .../codex exists at the same SHA — untouched. |
| At PR head | git rev-parse HEAD |
9b70ce60545ad07bf814af4021d8c9b96bafb48f = gh pr view 307 --json headRefOid. |
| Clean | git status --porcelain=v1 |
Empty. |
| Dependencies installed | ls node_modules |
Present; no pnpm install needed. |
| main is ancestor | git merge-base --is-ancestor main HEAD |
exit 0. main = e89ecc916ce88a5c11b84c2c8d63c484f8444c52; git log main..HEAD = one commit. |
Commands and exit codes
pnpm build exit 0
pnpm typecheck exit 0
pnpm test 253 files, 3856 passed, 4 skipped exit 0
The PR body's build/typecheck/suite numbers reproduce exactly.
Mutations (applied one at a time, restored from byte-exact backups, verified git diff --quiet after each; final HEAD unchanged and tree clean; no stray vitest/esbuild processes):
| # | Mutation | Result |
|---|---|---|
| M1 | SHIPPING §1: reviewers wait for hosted CI before any verdict | KILLED (1 failed / 9) |
| M2 | SHIPPING §2: HIGH findings deferrable with an issue | KILLED (1 failed / 9) |
| M3 | SHIPPING §3: ONE independent reviewer → fresh dual pair |
KILLED (1 failed / 49) |
| M4 | SHIPPING §3: broaden Mechanical to "anything the fixer judges low-risk, including small behavior fixes" | SURVIVED (63 passed) |
| M5 | SHIPPING §1: "both initial reviews must finish before landing" → one is enough | SURVIVED (63 passed) |
| M6 | land §1: "Verify both initial external reviews" → "at least one … where convenient" |
SURVIVED (63 passed) |
| M7 | topic §2 step 4: treat a moved head as reviewed from the recorded verdict |
SURVIVED (63 passed) |
The three mutants the PR claims are genuinely killed. M4–M7 are mine.
Hosted CI on 9b70ce6: scripted-structure SUCCESS; test (ubuntu-latest), test (macos-latest), windows-sandbox-none IN_PROGRESS. Reported as pending; code verdict returned without waiting.
These are instruction-contract checks over prose. Nothing here shows a live model obeys the policy, and I make no claim about the workflow speedup — the PR is appropriately explicit about that too.
Blocking
1. merge main in, never rebase was deleted with no replacement — MEDIUM
.agentrig/skills/topic/SKILL.md:220. main's §3 said "A merge conflict is the fixer's to resolve (merge main in, never rebase)". HEAD says only "CI failures and merge conflicts are repair work on the same branch, never a gate bypass." grep -rn rebase .agentrig docs/SHIPPING-WORKFLOW.md finds nothing but an unrelated line at land/SKILL.md:78.
Scenario: fixer resolves a round-1 conflict by rebasing. OLD is no longer an ancestor of NEW. topic §3 then runs git -C "$WT" branch "$BASE" "$OLD" and codex review --base $BASE, so the focused reviewer's diff is the re-applied fixer commits plus everything main gained — not the repair. The "affected checks and mutations" scoping in SHIPPING §3 becomes meaningless, and the repair goes effectively uncovered. This mattered less before, when repairs got a fresh pair; this PR makes the focused delta review the only independent review of a repair, so the dependency on a linear OLD→NEW is now load-bearing.
Fix: restore the clause in the fix bullet, or state it in SHIPPING §3 next to "Classify the complete delta since the last reviewed SHA".
2. The three-round counter cannot actually be recovered — MEDIUM
SHIPPING §3:88 says "Do not reset the counter after a push, conflict, restart, or reviewer change. Adopted PRs recover the ledger and review history", and topic/SKILL.md:41 tells the adopt path to "recover its review/disposition ledger and repair counter". But the ledger schema in SHIPPING §2:31 is "reviewer comment/finding ID, severity, concrete scenario, blocking or non-blocking, rationale, resolution/evidence, and affected SHA" — no round number. §4 asks for start/finish times "so … round count can be checked", which does not establish which push belonged to which round. ship/SKILL.md:58 likewise says "preserving the counter on resumption" while ship has no adoption path at all.
Scenario: conductor A burns all three rounds on PR #N and dies at budget. Conductor B invokes topic, adopts the PR, reads a ledger with findings and SHAs but no round count, starts at zero, and grants three more rounds. The cap is unbounded across sessions — the treadmill the cap exists to stop.
This one is blocking on the PR's own terms: the Decisions section states "Retain the three-round safety cap and counter on resumption", and unmet task acceptance blocks regardless of severity (SHIPPING §2).
Fix: add the round number to the §2 ledger schema, or require an explicit Repair round: N/3 line in the PR body that §1's adopt path and the lander read.
3. ship and standalone dogfood are told to run a focused delta review, but the mechanics pointer was deleted — MEDIUM
ship/SKILL.md:56 says "classify OLD..NEW under shipping policy §3: ONE independent focused reviewer"; dogfood/SKILL.md:159-161 says the same for the standalone author. SHIPPING §3 is policy — it states that one reviewer is used and what it verifies, and contains no worktree preparation, install deadline, TMPDIR, restore/join, provenance or model-assertion mechanics. Those exist only in topic/SKILL.md:222-243, which neither skill references. main's ship §3 did carry the pointer ("the same external pass over OLD..NEW as topic §3 describes"); this PR removed it. The surviving pointer, ship/SKILL.md:41, is explicitly scoped to the initial pair ("two external reviewers … in parallel"), and following it for a delta would be wrong — the delta pass must not merge main and uses one tree plus a unique base ref.
Scenario: a ship conductor or standalone dogfood author runs the focused review in the author's own tree, or reuses the initial pass's main-merged tree. Mutation probes and build outputs land in the author's working tree, and the delta diff silently includes main. The PR body lists "reviewer isolation, restoration/provenance" as retained; from two of the five entry points it is now unreachable.
Fix: point ship §3 and dogfood §9 at topic §3's "Cover the delta" bullet, or lift those mechanics into SHIPPING §3.
4. The gates this PR rewrote have no test coverage — MEDIUM (test-coverage)
M4, M5, M6 and M7 all survive the full instruction suite. In particular land §1's rewritten review-verification clause — the join point that makes "CI and review are independent tracks" safe — has zero assertions anywhere; shipping-convergence-guidance.test.ts touches land only to check the policy link line. The Mechanical definition is the single boundary deciding whether a delta gets any independent review, and only the adjacent sentence "A small executable fix is not mechanical" is pinned, so M4's self-contradictory broadening passes green. By this repo's own standard ("A surviving mutant on a security line is a finding even when every test passes"), these are findings.
Scenario: a later edit relaxes land §1 to accept one initial review, or widens Mechanical to cover small behavior fixes. pnpm test stays green and the change ships.
Fix: add assertions for Verify both initial external reviews, both initial reviews must finish before landing, Mechanical:** only spelling/formatting, and topic's do not call the new head reviewed.
Non-blocking defects
5. The Codex-sees-main's-changes carve-out was dropped — LOW. main's topic §3:256-257 said that when NEW contains a main merge, "findings on files the fixer's own commits did not touch are noted in the PR comment, not opened as findings". HEAD has only "The delta pass does not merge main" (topic/SKILL.md:229), which is about the worktree — the fixer may still have merged main into the branch. A focused Codex review then reports a finding in a file only main touched; SHIPPING §3's "Newly discovered real blockers still count" opens another round on already-landed code, burning the cap. Deferrable with an issue: it degrades convergence, not a safety gate.
6. The review-residual issue format was lost — LOW. main specified it twice (gh issue create, title [review residual] <one line>, label review-residual, body fields including reviewer identity and comment URL). HEAD has only SHIPPING §2:44-47. No title convention, no explicit --label, no gh issue create. land §0 checks only that each ## Residuals entry names an existing open issue, so unlabelled residuals land and the review-residual label query returns nothing — the deferred defects are never picked up.
7. The "six failures" fail-first claim does not reproduce — LOW. I restored main's five skill files, removed docs/SHIPPING-WORKFLOW.md, and ran the new test file: 9 failed / 9, exit 1 (then restored; HEAD unchanged, tree clean). The substance holds — the tests do fail first — but both the PR body and docs/STATUS.md state six. Correct the number or name the baseline it was measured against.
Advisory
- Author pushes during the initial pass now get one reviewer, not a pair. SHIPPING §1's freeze clause plus the rewrite at
topic/SKILL.md:181-185route any moved head into §3, so a large unrelated push gets a single focused review. This is exactly what the PR body describes, so it is in scope — but the scope's rationale is repair batching, and an arbitrary push is not a repair. Worth bounding the single-reviewer path to deltas attributable to assigned blockers, and requiring a fresh pair otherwise. - Adoption no longer gates on green CI.
topic/SKILL.md:41changed "verify CI is green on its current head" to "record current-head CI state … without waiting for pending CI". No gate is lost (landing still requires green exact-head CI), but the train now spends a full review pass before discovering the adopted head is red. - §3 is framed as "Repair blockers", yet §2 step 4 routes a clean pair with a moved head into it. Whether such a coverage-only review consumes a repair round is unstated.
Claims I verified, and one I could not
Verified: build/typecheck/suite exits and counts; the three claimed mutants; the shared-policy link in all five skills resolving to the same file; PR #306 = e89ecc9; that testing, authorization, initial-pair, exact-head CI and post-merge CI gates are textually retained. Not verified: any live-model or speedup claim — the PR correctly disclaims both.
Nothing was pushed, committed, merged or posted; no permission settings changed; no children or auxiliary models spawned; sibling trees untouched.
Focused review — Claude Code — head dacfab5 — delta 9b70ce6..dacfab5Focused delta review — PR #307 — head
|
| Claim | Check | Result |
|---|---|---|
| Isolated, mine | git worktree list |
/var/tmp/agentrig-shipping-review.EJfUAB/delta distinct, detached. Siblings .../claude and .../codex still at 9b70ce6 — untouched. |
| At NEW | git rev-parse HEAD |
dacfab5… = gh pr view 307 --json headRefOid, re-checked at exit: head never moved. |
| Clean | git status --porcelain=v1 |
Empty, before and after. |
| Deps installed | ls node_modules, node_modules/.bin/vitest |
Present; no install needed. |
| OLD is ancestor of NEW | git merge-base --is-ancestor 9b70ce6 dacfab5 |
exit 0 — applying the very rule this delta adds. origin/main = e89ecc9, also an ancestor. |
Commands and exit codes
vitest run <7 affected instruction test files> exit 0 145 passed
vitest run shipping-convergence-guidance.test.ts exit 0 13 passed
7 mutants, sequential, byte-exact restore exit 1 each (all KILLED)
fail-first A (policy present, 5 skills at main) exit 1 6 failed | 3 passed (9)
fail-first B (policy also removed) exit 1 9 failed (9)
fail-first C (NEW tests vs OLD sources) exit 1 3 failed | 10 passed (13)
Per the brief I ran affected tests and mutants, not the full trio. The author's build/typecheck/full-suite exit-0 claim (3,860 tests) is therefore unverified by me; the 3,856→3,860 delta is arithmetically consistent with the four test cases I did run. Hosted CI on dacfab5: scripted-structure SUCCESS, test (ubuntu-latest), test (macos-latest), windows-sandbox-none IN_PROGRESS — reported pending, returned without waiting.
Mutants (one at a time, restored, git diff --quiet verified after each; HEAD unchanged, tree and index clean, no stray runners):
| # | Mutation | Result |
|---|---|---|
| M4 | broaden Mechanical to "anything the fixer judges low-risk" | KILLED |
| M5 | one finished initial review is enough to land | KILLED |
| M6 | land: "at least one initial review where convenient" |
KILLED |
| M7 | topic: count the moved head as reviewed |
KILLED |
| N1 | drop the persisted Repair round: N/3 record |
KILLED |
| N2 | drop merge-main/no-rebase + ancestry check | KILLED |
| N3 | drop ship's Cover-the-delta pointer |
KILLED |
M4–M7 are the four that survived at OLD. N1–N3 are mine, confirming the newly added gates are load-bearing rather than decorative prose.
Blocker closure — 1–4 all closed
1 (merge-main/no-rebase + ancestry) — CLOSED. docs/SHIPPING-WORKFLOW.md:75-78 states it exactly where finding 1 asked, next to "Classify the complete delta". topic/SKILL.md:225 hooks it into the preparation step ahead of the branch "$BASE" "$OLD" commands, so the mechanical path in the scenario is covered. No contradicting rebase guidance exists (land:78 is about other PRs; land:83 already forbids force-push). N2 kills.
2 (counter recovery) — CLOSED via finding 2's second proposed fix. SHIPPING-WORKFLOW.md:36-42 requires the literal Repair round: N/3 line with per-round OLD/NEW, blocker IDs and outcome, increment-before-spawn, evidence-based reconstruction, halt-if-unestablished, and lander verification. The "ship has no adoption path" gap is closed by naming ship and standalone dogfood explicitly; topic:41 and ship:59 carry it. N1 kills. The PR body itself complies (round 1/3, OLD→NEW, B1–B4).
3 (ship/dogfood mechanics pointer) — CLOSED. Both now point at "topic §3's Cover the delta procedure"; I confirmed that bullet exists at topic/SKILL.md:220, so the pointer is not dangling. dogfood adds "the standalone author owns that procedure without becoming a topic train", which closes the standalone case correctly. N3 kills.
4 (test coverage) — CLOSED. All four assertions finding 4 asked for are present and each independently kills its mutant. Worth stating plainly so it isn't misread as an incomplete fail-first: the "pins the actual initial-pair…" test passes at OLD and NEW by design — it pins already-correct text against future erosion. That is why round 1's honest fail-first count is three, not four, and M4–M7 dying prove it is non-vacuous.
Dispositions 5–7 — adequate
7 — accepted, reproduced. 6 failed / 3 passed with the new policy present and the five skills at main; 9 failed with the policy also removed, matching reviewer B exactly. STATUS.md now names the measured baseline. No failed run is described as green.
6 — accepted, independently verified. I grepped .agentrig and docs for consumers of the review-residual label or any title convention: there are none, only the policy requirement itself and two historical STATUS mentions. The finding's stated consequence presumes automation that does not exist. The merge gate is intact and untouched: land §0 still requires every ## Residuals entry to name an open issue. One note — the rebuttal answers "title" where the finding emphasized "label"; I checked both, and the conclusion holds either way.
5 — accepted, with a caveat I'm recording rather than escalating. The protective substance survives in §3 ("Do not re-audit unchanged code", "checks direct regressions", "Prior independent reviews remain evidence for unchanged code"), and land §0's "evidence-backed rebuttals are not residuals" makes the no-issue disposition compliant. The author's point that a file-based blanket carve-out would also suppress genuine interaction findings is sound. Caveat: this delta newly prescribes "merge main into the branch", so main-merged content inside OLD..NEW becomes the normal case rather than an edge case, and §3's "Newly discovered real blockers still count" preserves the convergence ambiguity the deleted clause addressed. Non-blocking observation under §2 — it does not open a round. It is not covered by the ROADMAP advisory paragraph, which addresses the unassigned-push and coverage-only-counting advisories.
Non-blocking observations (§2 — no new repair round)
- The §3 main-merge ambiguity above.
- Assertions are substring checks over whitespace-normalized text, so a mutant that appends a loosening sentence while leaving the pinned sentence intact would still pass. Inherent to this file's pre-existing approach and exactly the form finding 4 requested — advisory, not a deferred defect.
- Cosmetic:
SHIPPING-WORKFLOW.md:73's colon now introduces a paragraph before its list;ship/SKILL.md:57overflows the file's line-width convention.
Verdict
No blocking findings. B1–B4 are closed on dacfab5; the dispositions for 5–7 are adequate and, where checkable, reproduce. No new blockers in OLD..NEW or its direct interactions.
This is a code verdict, not merge authorization: three of four required checks are still in progress on dacfab5, and landing remains gated on green exact-head CI plus the lander's join per §4. I make no claim about live model compliance or workflow speedup — the PR correctly disclaims both, and instruction-contract tests over prose cannot establish either.
Nothing was pushed, committed, merged or posted; no permission settings changed; no children or auxiliary models spawned; sibling trees untouched. One mid-review slip on my side: my first restore pass read from the index instead of HEAD, which left the tree dirty and invalidated my initial scenario C — I restored with git restore --source=HEAD --staged --worktree and re-ran it properly; the results above are from the corrected run.
Delivery completePR #307 squash-merged as
Authorization and finding dispositions remain in the PR body; no outstanding blocking findings. |
1 participant
Summary
Make shipping converge without weakening test, authorization or merge gates. All five workflow skills use one shared policy: builders/fixers return after locally verified push; hosted CI and the initial independent review pair run concurrently; blocker repairs are batched; material deltas get one independent focused review rather than another full pair. Mechanical changes carry explicit evidence. Non-blocking defects retain issue-backed dispositions; optional polish is recorded rather than implemented in another loop.
Decisions
Deviations
None. User-directed workflow correction; no roadmap row or acceptance criteria changed. Builder: operator outside AgentRig, no invented conductor session.
Verification
Review disposition
Repair round: 1/3 (recorded before external re-review; the operator performed the repair without a child).
Initial head:
9b70ce60545ad07bf814af4021d8c9b96bafb48f.Round 1 OLD → NEW:
9b70ce60545ad07bf814af4021d8c9b96bafb48f→dacfab5216f9f8c3d2b0f2164927962048cfb2f5; assigned blockers B1–B4. Full repaired build/typecheck/test exit 0: 3,860 passed, four existing skips, 253 files. Focused reviewer selected from the original reviewer pair because it raised those blockers. Started 19:29:11 UTC alongside CI run 34520635092; no second full pair launched.Repair round: N/3, assigned blocker IDs, OLD/NEW/outcome; increment before spawn; all entry points recover the record, lander verifies it. Missing history is not a fresh allowance.review-residualissue per real deferred defect and lists its required evidence fields. The term denotes the existing issue label. Omitting a literalgh issue createrecipe or title spelling does not remove that requirement; no repository automation consumes a particular residual title. A standardized command/template is optional polish, not a missing merge gate.Focused review on
dacfab5closes B1–B4 and accepts dispositions B5–B7. It independently passed 145 affected tests, killed seven mutants, and reproduced all three fail-first baselines. No blocking findings remain. No deferred verified defect remains requiring a new issue.Round 1 outcome: closed. One initial review pair, one batched repair, one focused follow-up; no repeated full pair. Initial and focused reviewers were in separate owned worktrees, restored clean at their recorded SHAs. Focused review completed by 19:37 UTC, before Windows CI completed, confirming actual overlap for this operator-run shipment (not yet a live AgentRig compliance claim).
Further advisory observations are retained here without another repair round: judge main-merge interactions carefully without pulling unrelated main-only code into the task; substring instruction tests cannot detect every contradictory appended sentence; minor wording/line-width polish is optional. Broader live-workflow validation remains in ROADMAP's final advisory section.
Merge authorization
Human response: "approved."
Context approved: "improve the shipping workflow next—run independent reviews alongside CI, reserve further reviews for material fixes, and defer non-blocking polish instead of repeatedly extending the task. Keep the testing and merge gates intact."
This authorization is bound to this shipping-convergence task and its resulting PR. The standing user delivery instruction is to merge when reviews and CI pass; no unrelated work is included.