From 595c819de1fe5e3611e07d60fe355c800bf16351 Mon Sep 17 00:00:00 2001 From: Timothy Date: Sat, 5 Sep 2026 13:14:09 +0200 Subject: [PATCH] fix(916): a failed substitute is an error, not a landed claim; head_sha described in both schemas; the issue's box says what the code does The runner-beside-the-lenses design is now what Done-when box 1 asks for (body amended). A rubric round whose runner and worktree fallback both fail returns an error before the push instead of landing a PR whose body claims a substitute reviewed it. The harness records lens count at the runner's start too (expects 0, so a re-serialised runner reddens), resets its counter per round, and has a case for the double failure. Decisions-Edit: yes Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_015QqCpYFsKgnAnx6jVwrKiV --- .claude/workflows/ersatztv-issue-build.js | 7 +++++-- .claude/workflows/ersatztv-resume-branch.js | 5 ++++- .../records/process/orchestrated-session.md | 5 +++-- .../orchestration-workflow-loop.test.mjs | 20 +++++++++++++++---- 4 files changed, 28 insertions(+), 9 deletions(-) diff --git a/.claude/workflows/ersatztv-issue-build.js b/.claude/workflows/ersatztv-issue-build.js index b5fa31668..c6e4d1945 100644 --- a/.claude/workflows/ersatztv-issue-build.js +++ b/.claude/workflows/ersatztv-issue-build.js @@ -68,7 +68,7 @@ const REPORT_SCHEMA = { left: { type: 'string', description: 'what is not done and why; what the next agent must know' }, commits: { type: 'string', description: 'git log --oneline origin/main..HEAD' }, pr_url: { type: 'string' }, - head_sha: { type: 'string', description: 'git rev-parse HEAD after your last commit — the finisher derives fix commits from these' }, + head_sha: { type: 'string', description: 'git rev-parse HEAD of YOUR WORKTREE after your last commit (not a PR head) — the finisher derives fix commits from these' }, patch_changed: { type: 'boolean', description: 'finisher only: true if the pre-push rebase changed the patch-id (a conflict resolved or an artifact regenerated)' }, }, } @@ -118,7 +118,7 @@ ${CLAIM} ${recon ? `Recon (verify what you rely on):\nPLAN: ${recon.plan}\nFACTS: ${recon.facts}\nRISKS: ${recon.risks}\nTEST PLAN: ${recon.test_plan}\n` : ''} You are the implementer. Close ${REF} completely: pin the behaviour with tests named for the branch they protect, update the docs the change obligates, commit. Then git fetch origin and rebase onto origin/main if it moved (never merge main in; regenerate generated artifacts), run the LOCAL GATE and STOP — do not push; reviewers read your worktree first, and a finisher pushes once after the review loop is clean. ${GATE} -Report done=true with the gate output when the worktree is ready for review, with pr_url empty and head_sha = git rev-parse HEAD.`, +Report done=true with the gate output when the worktree is ready for review, with pr_url empty and head_sha = git rev-parse HEAD of the worktree after your last commit.`, { label: `impl:${REF}`, model: implModel, effort: implEffort, schema: REPORT_SCHEMA }) if (!impl) return { issues, error: 'implementer returned nothing' } if (!impl.done) return { issues, error: 'implementer stopped', impl } @@ -133,6 +133,7 @@ const LENSES = [ { key: 'correctness', model: 'opus', isolation: 'worktree', prompt: 'correctness against the done condition: run the gate and, for a write path or screen, the live-E2E route yourself, and read the output; try to break the change with the edge cases the issue and the docs name; check the pinning test actually reddens when the fix alone is reverted (mutate the clause, not the file).' }, { key: 'conformance', model: 'sonnet', prompt: 'repo conformance: docs-update obligations met in this diff (endpoint → api-conventions + regenerated v1.json/endpoint-index; screen/route → blazor-route-parity + domain-model; convention → decision record + regenerated catalog; new doc → README index); no narrative in docs; every new script or hook has its inventory row; CPM respected; both-provider migration if the model changed; tests are NUnit/vitest in the existing projects; no BOM in touched .cs; no edit to a file another slot owns (listed above); commit trailers present; branch rebased on current origin/main; nothing pushed yet.' }, ] +let xfamilyFailedRound = null let xfamily = rubric ? 'codex' : 'not required (routine risk class under process.independent-review-rubric)' async function codexRunner(round) { const r = await agent(`${reviewCommon(Number(args.port) + 3)} @@ -163,6 +164,7 @@ Be adversarial; report only what you verified, with evidence. blocking = done co if (r && r.ran === true) return lenses.concat([r]) let fb = null try { fb = await codexFallback(round, r) } catch (e) { log(`${REF}: fallback reviewer threw: ${e && e.message}`) } + if (!fb) { xfamily += ` — the substitute ALSO failed in round ${round}; no cross-family-equivalent review ran`; xfamilyFailedRound = round } return fb ? lenses.concat([fb]) : lenses } @@ -199,6 +201,7 @@ Report, with head_sha = git rev-parse HEAD of the worktree after your last commi } if (blocking.length) return { issues, error: 'blocking findings after two fix rounds; not pushed', blocking_remaining: blocking, history } if (sendBack.length) return { issues, error: 'should-fix findings still open after two fix rounds; not pushed — the orchestrator decides', should_fix_remaining: sendBack, history } +if (xfamilyFailedRound) return { issues, error: `the cross-family runner and its substitute both failed in round ${xfamilyFailedRound}; not pushed`, cross_family: xfamily, history } const FIX_RANGES = history.map(h => h.fix_range).filter(Boolean) const REVIEW_HISTORY = history.map(h => `round ${h.round}: ${h.reviews.length} lens(es); ${countBy(h.reviews, 'blocking')} blocking, ${countBy(h.reviews, 'should-fix')} should-fix, ${countBy(h.reviews, 'nit')} nit` + (h.fix ? (h.fix_range ? `; answered by the fix commit(s) in git log --oneline ${h.fix_range}` : '; answered without a new commit (findings refuted with evidence in the fixer report)') : '; clean — loop ended')).join('\n') diff --git a/.claude/workflows/ersatztv-resume-branch.js b/.claude/workflows/ersatztv-resume-branch.js index 47b75d672..d2121e59f 100644 --- a/.claude/workflows/ersatztv-resume-branch.js +++ b/.claude/workflows/ersatztv-resume-branch.js @@ -50,7 +50,7 @@ const REPORT_SCHEMA = { properties: { done: { type: 'boolean' }, summary: { type: 'string' }, verified: { type: 'string', description: 'exact gate commands run and their real output summary' }, - left: { type: 'string' }, commits: { type: 'string', description: 'git log --oneline origin/main..HEAD' }, pr_url: { type: 'string' }, head_sha: { type: 'string' }, + left: { type: 'string' }, commits: { type: 'string', description: 'git log --oneline origin/main..HEAD' }, pr_url: { type: 'string' }, head_sha: { type: 'string', description: 'git rev-parse HEAD of YOUR WORKTREE after your last commit (not a PR head) — the finisher derives fix commits from these' }, patch_changed: { type: 'boolean', description: 'finisher only: true if a second rebase before the push changed the patch-id' }, }, } @@ -101,6 +101,7 @@ const LENSES = [ { key: 'correctness', model: 'opus', isolation: 'worktree', prompt: 'correctness against the done condition: run the gate and, for a write path or screen, the live-E2E route yourself, and read the output; try to break the change with the edge cases the issue and the docs name; check the pinning test reddens when the fix alone is reverted.' }, { key: 'conformance', model: 'sonnet', prompt: 'repo conformance: docs-update obligations met; no narrative in docs; inventory rows for new scripts/hooks; CPM respected; both-provider migration if the model changed; no BOM in touched .cs; no WIP commit left in history; branch rebased on current origin/main; commit trailers present; PR body will carry fixes #N for each issue.' }, ] +let xfamilyFailedRound = null let xfamily = rubric ? 'codex' : 'not required (routine risk class under process.independent-review-rubric)' async function codexRunner(round) { const r = await agent(`${reviewCommon(Number(args.port) + 3)} @@ -131,6 +132,7 @@ Be adversarial; report only what you verified, with evidence. blocking = done co if (r && r.ran === true) return lenses.concat([r]) let fb = null try { fb = await codexFallback(round, r) } catch (e) { log(`${REF}: fallback reviewer threw: ${e && e.message}`) } + if (!fb) { xfamily += ` — the substitute ALSO failed in round ${round}; no cross-family-equivalent review ran`; xfamilyFailedRound = round } return fb ? lenses.concat([fb]) : lenses } @@ -164,6 +166,7 @@ Re-run the LOCAL GATE and STOP without pushing; report head_sha = git rev-parse } if (blocking.length) return { issues, error: 'blocking findings after two fix rounds; not pushed', blocking_remaining: blocking, history } if (sendBack.length) return { issues, error: 'should-fix findings still open after two fix rounds; not pushed — the orchestrator decides', should_fix_remaining: sendBack, history } +if (xfamilyFailedRound) return { issues, error: `the cross-family runner and its substitute both failed in round ${xfamilyFailedRound}; not pushed`, cross_family: xfamily, history } const FIX_RANGES = history.map(h => h.fix_range).filter(Boolean) const REVIEW_HISTORY = history.map(h => `round ${h.round}: ${h.reviews.length} lens(es); ${countBy(h.reviews, 'blocking')} blocking, ${countBy(h.reviews, 'should-fix')} should-fix, ${countBy(h.reviews, 'nit')} nit` + (h.fix ? (h.fix_range ? `; answered by the fix commit(s) in git log --oneline ${h.fix_range}` : '; answered without a new commit (findings refuted with evidence in the fixer report)') : '; clean — loop ended')).join('\n') diff --git a/docs/decisions/records/process/orchestrated-session.md b/docs/decisions/records/process/orchestrated-session.md index 1fe9a9528..1d2a9af6d 100644 --- a/docs/decisions/records/process/orchestrated-session.md +++ b/docs/decisions/records/process/orchestrated-session.md @@ -10,8 +10,9 @@ signals: 'orchestrator · referee · slots · parallel issues · worktree per is mechanics: '`docs/handoffs/orchestration.md` owns roles, isolation and the landing order; the Workflow scripts under `.claude/workflows/` encode it. Box-ticking: the implementer writes the evidence per box into the `## Closing record`; the orchestrator ticks after reading it and the review evidence, then `scripts/post-review-verdict.sh`.' --- -The kickoff (`docs/handoffs/chicorytv-issue-queue.md`) binds a session that closes one issue. Running several at once does not relax any of it; what it adds is a referee, and two places where the -single-session rules needed a stated scope rather than a contradiction. +The kickoff (`docs/handoffs/chicorytv-issue-queue.md`) binds a session that closes one issue. +Running several at once does not relax any of it; what it adds is a referee, and two places where +the single-session rules needed a stated scope rather than a contradiction. **Force-with-lease.** `process.pr-routine-sequence` forbids amending or force-pushing a pushed branch; `release.format-as-you-touch-rebase` (H11) refuses to push a branch behind `origin/main` and forbids diff --git a/web/scripts/orchestration-workflow-loop.test.mjs b/web/scripts/orchestration-workflow-loop.test.mjs index dc14d5d28..49a715f43 100644 --- a/web/scripts/orchestration-workflow-loop.test.mjs +++ b/web/scripts/orchestration-workflow-loop.test.mjs @@ -38,21 +38,24 @@ const finding = (severity) => ({ severity, file: 'f', summary: `${severity} find // Every stub lens returns the same findings, so a finding counts once per lens (two lenses → 2). // reviewRounds: array indexed by round-1 of {findings, lensesNull?}; landOverrides: extra fields on the finisher report; // postRebase: findings returned by the review after a patch-changing rebase, or 'lensesNull'; fixerNull: the fixer agent dies. -function run(script, args, { reviewRounds, landOverrides = {}, postRebase = null, fixerNull = false, fixerDone = true, fixerNoCommit = false }) { +function run(script, args, { reviewRounds, landOverrides = {}, postRebase = null, fixerNull = false, fixerDone = true, fixerNoCommit = false, fallbackNull = false }) { const calls = []; let round = 0; let fixCount = 0; let lensesReturned = 0; const lensesReturnedWhenFallbackStarted = []; + const lensesReturnedWhenRunnerStarted = []; const agent = async (prompt, opts) => { const label = (opts && opts.label) || ''; calls.push({ label, prompt, opts }); if (label.startsWith('impl:') || label.startsWith('fix:#')) return { done: true, summary: '', verified: '', left: '', commits: IMPL_COMMITS, head_sha: 'aaaaaaa' }; if (label.startsWith('review:codex:')) { + lensesReturnedWhenRunnerStarted.push(lensesReturned); return { verdict: 'merge', findings: [], ran: false }; } if (label.startsWith('review:fallback:')) { lensesReturnedWhenFallbackStarted.push(lensesReturned); + if (fallbackNull) return null; const spec = reviewRounds[round - 1] || { findings: [] }; return { verdict: 'merge', findings: spec.findings }; } @@ -74,12 +77,13 @@ function run(script, args, { reviewRounds, landOverrides = {}, postRebase = null }; const parallel = async (thunks) => { round += 1; + lensesReturned = 0; if (postRebase !== null && calls.some((c) => c.label.startsWith('land:'))) { return Promise.all(thunks.map(() => Promise.resolve(postRebase === 'lensesNull' ? null : { verdict: 'merge', findings: postRebase }))); } return Promise.all(thunks.map((t) => t().catch(() => null))); }; - return script(args, agent, parallel, () => {}, () => {}).then((result) => ({ result, calls, fixCount, lensesReturnedWhenFallbackStarted })); + return script(args, agent, parallel, () => {}, () => {}).then((result) => ({ result, calls, fixCount, lensesReturnedWhenFallbackStarted, lensesReturnedWhenRunnerStarted })); } const cases = [ @@ -171,13 +175,21 @@ describe.each(cases)('%s review loop', (name, args) => { expect(result.error).toMatch(/^the post-rebase review round produced no reviews/); }); - it('on a rubric-class change the worktree-isolated fallback reviewer starts only after both lenses have returned', async () => { + it('a rubric round whose runner and substitute both fail is an error, never a landed PR', async () => { const rubricArgs = { ...args, risk: 'rubric' }; - const { result, calls, lensesReturnedWhenFallbackStarted } = await run(script, rubricArgs, { reviewRounds: [{ findings: [] }] }); + const { result, calls } = await run(script, rubricArgs, { reviewRounds: [{ findings: [] }], fallbackNull: true }); + expect(result.error).toMatch(/^the cross-family runner and its substitute both failed in round 1/); + expect(calls.some((c) => c.label.startsWith('land:'))).toBe(false); + }); + + it('on a rubric-class change the fallback reviewer starts only after both lenses have returned, and the runner beside them', async () => { + const rubricArgs = { ...args, risk: 'rubric' }; + const { result, calls, lensesReturnedWhenFallbackStarted, lensesReturnedWhenRunnerStarted } = await run(script, rubricArgs, { reviewRounds: [{ findings: [] }] }); expect(result.error).toBeUndefined(); expect(calls.filter((c) => c.label.startsWith('review:codex:')).length).toBe(1); expect(calls.filter((c) => c.label.startsWith('review:fallback:')).length).toBe(1); expect(lensesReturnedWhenFallbackStarted).toEqual([2]); + expect(lensesReturnedWhenRunnerStarted).toEqual([0]); expect(finisherPrompt(calls)).toContain('substituted a cold same-family review-only agent'); });