From b1d5fbefcba02fdc6c19fef85cec1c4e82fc8dea Mon Sep 17 00:00:00 2001 From: Timothy Date: Sat, 5 Sep 2026 13:46:39 +0200 Subject: [PATCH] fix(916): cross-family state is per round, and the orchestrator reads cross_family before a verdict A substitute that failed in round one said nothing about the tree that lands after round two, yet the flag was sticky and doomed the run; the xfamily string was never reset either, so clearing the stickiness alone would have let a stale "substitute ALSO failed" sentence into the PR body. Both reset at the top of review(). The harness runner is round-aware (ran per round, its own counter reset) and a two-round case pins the fix; restoring the sticky flag reddens it in both scripts. Step 4 of the mechanics page tells the referee to read cross_family, not only error, before posting on a rubric-class PR. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_015QqCpYFsKgnAnx6jVwrKiV --- .claude/workflows/ersatztv-issue-build.js | 4 ++++ .claude/workflows/ersatztv-resume-branch.js | 4 ++++ docs/handoffs/orchestration.md | 8 +++++--- .../orchestration-workflow-loop.test.mjs | 18 ++++++++++++++++-- 4 files changed, 29 insertions(+), 5 deletions(-) diff --git a/.claude/workflows/ersatztv-issue-build.js b/.claude/workflows/ersatztv-issue-build.js index c6e4d1945..f6fea6750 100644 --- a/.claude/workflows/ersatztv-issue-build.js +++ b/.claude/workflows/ersatztv-issue-build.js @@ -151,6 +151,10 @@ You are a COLD, review-only substitute for a cross-family reviewer that could no { label: `review:fallback:r${round}`, phase: 'Review', model: 'opus', effort: 'high', isolation: 'worktree', schema: FINDINGS_SCHEMA }) } async function review(round) { + // Per round, like blocking/sendBack: a substitute that failed in round 1 says nothing about the tree + // that lands after round 2, and a stale xfamily string must never reach the PR body. + xfamilyFailedRound = null + xfamily = rubric ? 'codex' : 'not required (routine risk class under process.independent-review-rubric)' // The Codex runner builds nothing, so it may run beside the lenses; the FALLBACK is a second // worktree-isolated .NET reviewer and starts only after both lenses have returned. const runnerPromise = rubric ? codexRunner(round).catch(() => null) : Promise.resolve(null) diff --git a/.claude/workflows/ersatztv-resume-branch.js b/.claude/workflows/ersatztv-resume-branch.js index d2121e59f..05f71a451 100644 --- a/.claude/workflows/ersatztv-resume-branch.js +++ b/.claude/workflows/ersatztv-resume-branch.js @@ -119,6 +119,10 @@ You are a COLD, review-only substitute for a cross-family reviewer that could no { label: `review:fallback:r${round}`, phase: 'Review', model: 'opus', effort: 'high', isolation: 'worktree', schema: FINDINGS_SCHEMA }) } async function review(round) { + // Per round, like blocking/sendBack: a substitute that failed in round 1 says nothing about the tree + // that lands after round 2, and a stale xfamily string must never reach the PR body. + xfamilyFailedRound = null + xfamily = rubric ? 'codex' : 'not required (routine risk class under process.independent-review-rubric)' // The Codex runner builds nothing, so it may run beside the lenses; the FALLBACK is a second // worktree-isolated .NET reviewer and starts only after both lenses have returned. const runnerPromise = rubric ? codexRunner(round).catch(() => null) : Promise.resolve(null) diff --git a/docs/handoffs/orchestration.md b/docs/handoffs/orchestration.md index 07b1cb29c..c59e904fe 100644 --- a/docs/handoffs/orchestration.md +++ b/docs/handoffs/orchestration.md @@ -107,9 +107,11 @@ the review loop *inside* the worktree, before the single push: 4. **Orchestrator**: read the review evidence, not the summaries. Send the PR back for anything that lets a route or test pass having done nothing. Tick each box whose evidence holds, then re-read the head sha immediately before posting `scripts/post-review-verdict.sh MERGEABLE - ` naming the reviewers, the rounds and the workflow's returned `cross_family` status (a - Codex failure in a post-rebase round is in that return, not in the PR body); tick the review - box. Wait for CI — a `cancelled` job + ` naming the reviewers, the rounds and the workflow's returned `cross_family` status. Read + that field, not only `error`: a cross-family failure in the post-rebase round leaves the branch + pushed and the run successful, and only `cross_family` says the substitute also failed; a + rubric-class PR in that state gets no verdict until a cross-family or substitute review of the + pushed head has run. Tick the review box. Wait for CI — a `cancelled` job reads as `failure` at the combined status endpoint, so resolve it via the run's jobs (`ci.cancelled-is-not-a-verdict`). 5. **Merge through the Gitea merge tool with the full head sha**; the consent hook derives consent diff --git a/web/scripts/orchestration-workflow-loop.test.mjs b/web/scripts/orchestration-workflow-loop.test.mjs index 49a715f43..db9aa63cd 100644 --- a/web/scripts/orchestration-workflow-loop.test.mjs +++ b/web/scripts/orchestration-workflow-loop.test.mjs @@ -50,12 +50,14 @@ function run(script, args, { reviewRounds, landOverrides = {}, postRebase = null 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:')) { + lensesReturned = 0; lensesReturnedWhenRunnerStarted.push(lensesReturned); - return { verdict: 'merge', findings: [], ran: false }; + const spec = reviewRounds[round] || {}; + return { verdict: 'merge', findings: [], ran: spec.runnerRan === true }; } if (label.startsWith('review:fallback:')) { lensesReturnedWhenFallbackStarted.push(lensesReturned); - if (fallbackNull) return null; + if (fallbackNull === true || (Array.isArray(fallbackNull) && fallbackNull.includes(round))) return null; const spec = reviewRounds[round - 1] || { findings: [] }; return { verdict: 'merge', findings: spec.findings }; } @@ -175,6 +177,18 @@ describe.each(cases)('%s review loop', (name, args) => { expect(result.error).toMatch(/^the post-rebase review round produced no reviews/); }); + it('a substitute failure in round one does not doom a run whose landing round had a real cross-family review', async () => { + const rubricArgs = { ...args, risk: 'rubric' }; + const { result, calls } = await run(script, rubricArgs, { + reviewRounds: [{ findings: [finding('should-fix')] }, { findings: [], runnerRan: true }], + fallbackNull: [1], + }); + expect(result.error).toBeUndefined(); + expect(result.cross_family).toBe('codex'); + expect(finisherPrompt(calls)).not.toContain('substitute ALSO failed'); + expect(calls.filter((c) => c.label.startsWith('review:codex:')).length).toBe(2); + }); + 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 } = await run(script, rubricArgs, { reviewRounds: [{ findings: [] }], fallbackNull: true });