From f7a0da4051210b37203e885f94b0dd2b5a9e6b40 Mon Sep 17 00:00:00 2001 From: Timothy Date: Sat, 5 Sep 2026 12:07:17 +0200 Subject: [PATCH] fix(916): the cross-family review step follows the lenses instead of running beside them, and fix commits are a sha range MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit One .NET slot's review round ran two worktree-isolated reviewers at once — the correctness lens and, on a rubric change, the Codex fallback — and took swap from 6.8 GB to 10.8 GB in three minutes on the 16 GB host; three slots reached load 82. review() now awaits the lenses, then the Codex runner, then its fallback. The finisher's fix attribution is the sha range the fixer's report head advances (head_sha is required on every report), replacing a line-set difference over free text that listed all eleven #563 commits as fixes. The harness gains a case that records how many lenses were still in flight when the cross-family agents started (must be zero); moving the fallback back into the parallel batch reddens it in both scripts. The mechanics page and the standing prompt state the measured cap: one .NET-building slot at a time. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_015QqCpYFsKgnAnx6jVwrKiV --- .claude/workflows/ersatztv-issue-build.js | 33 ++++++++-------- .claude/workflows/ersatztv-resume-branch.js | 31 ++++++++------- docs/handoffs/orchestration.md | 13 +++++-- docs/handoffs/orchestrator-prompt.md | 4 +- .../orchestration-workflow-loop.test.mjs | 38 +++++++++++++++---- 5 files changed, 77 insertions(+), 42 deletions(-) diff --git a/.claude/workflows/ersatztv-issue-build.js b/.claude/workflows/ersatztv-issue-build.js index d11b49b42..7f4c6b379 100644 --- a/.claude/workflows/ersatztv-issue-build.js +++ b/.claude/workflows/ersatztv-issue-build.js @@ -60,7 +60,7 @@ const GATE = gateFor(args.port, WT) const REPORT_SCHEMA = { type: 'object', - required: ['done', 'summary', 'verified', 'left', 'commits'], + required: ['done', 'summary', 'verified', 'left', 'commits', 'head_sha'], properties: { done: { type: 'boolean' }, summary: { type: 'string', description: 'what was built, file by file' }, @@ -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' }, + head_sha: { type: 'string', description: 'git rev-parse HEAD after your last commit — 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)' }, }, } @@ -148,25 +148,28 @@ 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) { - const runs = LENSES.map(l => () => agent(`${reviewCommon(Number(args.port) + 1)} + // The two lenses run together; the cross-family step (the Codex runner, then its worktree-isolated + // fallback) runs AFTER they return. Two worktree-isolated reviewers building .NET at once from one + // slot saturated swap on the 16 GB host (#916), and the fallback is one of them. + const lenses = (await parallel(LENSES.map(l => () => agent(`${reviewCommon(Number(args.port) + 1)} Review round ${round} of the branch for ${REF}. Lens: ${l.prompt} Be adversarial; report only what you verified, with evidence. blocking = done condition or a repo rule violated, or a test that passes for the wrong reason; should-fix = real defect; nit = style. Verdict send-back if any blocking.`, - { label: `review:${l.key}:r${round}`, phase: 'Review', model: l.model, effort: 'high', isolation: l.isolation, schema: FINDINGS_SCHEMA })) - if (rubric) runs.push(() => codexReview(round)) - return parallel(runs) + { label: `review:${l.key}:r${round}`, phase: 'Review', model: l.model, effort: 'high', isolation: l.isolation, schema: FINDINGS_SCHEMA })))).filter(Boolean) + if (!rubric) return lenses + const xf = await codexReview(round) + return xf ? lenses.concat([xf]) : lenses } let round = 1 const actionable = rs => rs.flatMap(r => r.findings.filter(f => f.severity === 'blocking' || f.severity === 'should-fix')) const countBy = (rs, sev) => rs.flatMap(r => r.findings).filter(f => f.severity === sev).length -const newLines = (before, after) => { const seen = new Set(String(before || '').split('\n')); return String(after || '').split('\n').filter(l => l && !seen.has(l)) } -let knownCommits = impl.commits +let knownHead = impl.head_sha let reviews = (await review(round)).filter(Boolean) if (!reviews.length) return { issues, error: `review round ${round} produced no reviews (every lens failed); not pushed`, history: [] } let blocking = reviews.flatMap(r => r.findings.filter(f => f.severity === 'blocking')) let sendBack = actionable(reviews) -const history = [{ round, reviews, fix: null, fix_commits: [] }] +const history = [{ round, reviews, fix: null, fix_range: null }] while (sendBack.length && round < 3) { log(`${REF} round ${round}: ${blocking.length} blocking, ${sendBack.length - blocking.length} should-fix — sending back`) const fix = await agent(`${COMMON} @@ -180,19 +183,19 @@ Report.`, { label: `fix:r${round}`, phase: 'Fix', model: implModel, effort: implEffort, schema: REPORT_SCHEMA }) if (!fix || !fix.done) return { issues, error: `fixer for round ${round} ${fix ? 'stopped' : 'returned nothing'}; not pushed`, fix, history } history[history.length - 1].fix = fix - history[history.length - 1].fix_commits = newLines(knownCommits, fix.commits) - knownCommits = fix.commits || knownCommits + history[history.length - 1].fix_range = fix.head_sha && fix.head_sha !== knownHead ? `${knownHead}..${fix.head_sha}` : null + knownHead = fix.head_sha || knownHead round++ reviews = (await review(round)).filter(Boolean) if (!reviews.length) return { issues, error: `review round ${round} produced no reviews (every lens failed); not pushed`, history } blocking = reviews.flatMap(r => r.findings.filter(f => f.severity === 'blocking')) sendBack = actionable(reviews) - history.push({ round, reviews, fix: null, fix_commits: [] }) + history.push({ round, reviews, fix: null, fix_range: null }) } 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 } -const FIX_COMMITS = history.flatMap(h => h.fix_commits) -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_commits.length ? `; answered by fix commit(s): ${h.fix_commits.join(' | ')}` : '; answered without a new commit (findings refuted with evidence in the fixer report)') : '; clean — loop ended')).join('\n') +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') phase('Land') const FINISH = `FINISH, in this order. Record the patch-id first: git diff $(git merge-base origin/main HEAD)..HEAD | git patch-id --stable. Then git fetch origin; if origin/main moved, rebase onto it (never merge main in; regenerate, never hand-resolve, generated artifacts — the decisions catalog by its generator), re-run the LOCAL GATE, and recompute the patch-id: report patch_changed=true if it differs. ${GATE} @@ -200,7 +203,7 @@ Then ONE push: git push -u origin ${BRANCH}. Open the PR with the Gitea API (POS <> -${FIX_COMMITS.length ? `followed by what each of these fix commits changed, read from git show and not from memory: ${FIX_COMMITS.join(' | ')}` : (history.some(h => h.fix) ? 'and a sentence saying every finding was answered without a new commit, as the history block records' : 'and a sentence saying no fix commit exists because round one was clean')}, then the cross-family review status verbatim — "${xfamily}" — and every deliberately-left item with an issue number (file follow-up issues where needed). End the body with: +${FIX_RANGES.length ? `followed by what each fix commit changed, read from git show and not from memory, for exactly the commits git log --oneline lists in these ranges: ${FIX_RANGES.join(' | ')}` : (history.some(h => h.fix) ? 'and a sentence saying every finding was answered without a new commit, as the history block records' : 'and a sentence saying no fix commit exists because round one was clean')}, then the cross-family review status verbatim — "${xfamily}" — and every deliberately-left item with an issue number (file follow-up issues where needed). End the body with: 🤖 Generated with [Claude Code](https://claude.com/claude-code) ${SESSION_URL} diff --git a/.claude/workflows/ersatztv-resume-branch.js b/.claude/workflows/ersatztv-resume-branch.js index 946418533..4f205e55a 100644 --- a/.claude/workflows/ersatztv-resume-branch.js +++ b/.claude/workflows/ersatztv-resume-branch.js @@ -46,7 +46,7 @@ const GATE = gateFor(args.port, WT) const REBASE = `Rebase onto origin/main FIRST: git fetch origin; git rebase origin/main; resolve conflicts faithfully, keeping both sides' intent; regenerate generated artifacts rather than hand-resolving them. A commit titled "WIP: orchestrator checkpoint" holds uncommitted work from the paused session and must be folded into the commit it belongs to, never left in history — if it sits directly on that commit: git reset --soft HEAD~1 && git commit --amend --no-edit; otherwise: git commit --fixup= is already its shape, so GIT_SEQUENCE_EDITOR=true git rebase --autosquash ~1 folds it non-interactively.` const REPORT_SCHEMA = { - type: 'object', required: ['done', 'summary', 'verified', 'left', 'commits'], + type: 'object', required: ['done', 'summary', 'verified', 'left', 'commits', 'head_sha'], properties: { done: { type: 'boolean' }, summary: { type: 'string' }, verified: { type: 'string', description: 'exact gate commands run and their real output summary' }, @@ -116,25 +116,28 @@ 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) { - const runs = LENSES.map(l => () => agent(`${reviewCommon(Number(args.port) + 1)} + // The two lenses run together; the cross-family step (the Codex runner, then its worktree-isolated + // fallback) runs AFTER they return. Two worktree-isolated reviewers building .NET at once from one + // slot saturated swap on the 16 GB host (#916), and the fallback is one of them. + const lenses = (await parallel(LENSES.map(l => () => agent(`${reviewCommon(Number(args.port) + 1)} Review round ${round} of the branch for ${REF}. Lens: ${l.prompt} Be adversarial; report only what you verified, with evidence. blocking = done condition or a repo rule violated, or a test that passes for the wrong reason; should-fix = real defect; nit = style. Verdict send-back if any blocking.`, - { label: `review:${l.key}:r${round}`, phase: 'Review', model: l.model, effort: 'high', isolation: l.isolation, schema: FINDINGS_SCHEMA })) - if (rubric) runs.push(() => codexReview(round)) - return parallel(runs) + { label: `review:${l.key}:r${round}`, phase: 'Review', model: l.model, effort: 'high', isolation: l.isolation, schema: FINDINGS_SCHEMA })))).filter(Boolean) + if (!rubric) return lenses + const xf = await codexReview(round) + return xf ? lenses.concat([xf]) : lenses } let round = 1 const actionable = rs => rs.flatMap(r => r.findings.filter(f => f.severity === 'blocking' || f.severity === 'should-fix')) const countBy = (rs, sev) => rs.flatMap(r => r.findings).filter(f => f.severity === sev).length -const newLines = (before, after) => { const seen = new Set(String(before || '').split('\n')); return String(after || '').split('\n').filter(l => l && !seen.has(l)) } -let knownCommits = work.commits +let knownHead = work.head_sha let reviews = (await review(round)).filter(Boolean) if (!reviews.length) return { issues, error: `review round ${round} produced no reviews (every lens failed); not pushed`, history: [] } let blocking = reviews.flatMap(r => r.findings.filter(f => f.severity === 'blocking')) let sendBack = actionable(reviews) -const history = [{ round, reviews, fix: null, fix_commits: [] }] +const history = [{ round, reviews, fix: null, fix_range: null }] while (sendBack.length && round < 3) { log(`${REF} round ${round}: ${blocking.length} blocking, ${sendBack.length - blocking.length} should-fix — sending back`) const fix = await agent(`${COMMON} @@ -145,19 +148,19 @@ Re-run the LOCAL GATE and STOP without pushing. ${GATE}`, { label: `fix:r${round}`, phase: 'Fix', model: 'opus', effort: 'high', schema: REPORT_SCHEMA }) if (!fix || !fix.done) return { issues, error: `fixer for round ${round} ${fix ? 'stopped' : 'returned nothing'}; not pushed`, fix, history } history[history.length - 1].fix = fix - history[history.length - 1].fix_commits = newLines(knownCommits, fix.commits) - knownCommits = fix.commits || knownCommits + history[history.length - 1].fix_range = fix.head_sha && fix.head_sha !== knownHead ? `${knownHead}..${fix.head_sha}` : null + knownHead = fix.head_sha || knownHead round++ reviews = (await review(round)).filter(Boolean) if (!reviews.length) return { issues, error: `review round ${round} produced no reviews (every lens failed); not pushed`, history } blocking = reviews.flatMap(r => r.findings.filter(f => f.severity === 'blocking')) sendBack = actionable(reviews) - history.push({ round, reviews, fix: null, fix_commits: [] }) + history.push({ round, reviews, fix: null, fix_range: null }) } 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 } -const FIX_COMMITS = history.flatMap(h => h.fix_commits) -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_commits.length ? `; answered by fix commit(s): ${h.fix_commits.join(' | ')}` : '; answered without a new commit (findings refuted with evidence in the fixer report)') : '; clean — loop ended')).join('\n') +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') phase('Land') const FINISH = `FINISH: record the patch-id (git diff $(git merge-base origin/main HEAD)..HEAD | git patch-id --stable); git fetch origin; if origin/main moved again, rebase onto it, re-run the LOCAL GATE, and recompute the patch-id — report patch_changed=true if it differs. ${GATE} @@ -165,7 +168,7 @@ Then git push --force-with-lease origin ${BRANCH}. ${args.pr ? `Update PR #${arg <> -${FIX_COMMITS.length ? `followed by what each of these fix commits changed, read from git show and not from memory: ${FIX_COMMITS.join(' | ')}` : (history.some(h => h.fix) ? 'and a sentence saying every finding was answered without a new commit, as the history block records' : 'and a sentence saying no fix commit exists because round one was clean')}, then the cross-family review status verbatim — "${xfamily}" — every deliberately-left item with an issue number, and end with: +${FIX_RANGES.length ? `followed by what each fix commit changed, read from git show and not from memory, for exactly the commits git log --oneline lists in these ranges: ${FIX_RANGES.join(' | ')}` : (history.some(h => h.fix) ? 'and a sentence saying every finding was answered without a new commit, as the history block records' : 'and a sentence saying no fix commit exists because round one was clean')}, then the cross-family review status verbatim — "${xfamily}" — every deliberately-left item with an issue number, and end with: 🤖 Generated with [Claude Code](https://claude.com/claude-code) ${SESSION_URL} diff --git a/docs/handoffs/orchestration.md b/docs/handoffs/orchestration.md index b985a82e2..4282084c8 100644 --- a/docs/handoffs/orchestration.md +++ b/docs/handoffs/orchestration.md @@ -51,10 +51,15 @@ script and returns a path for later runs. - `web/node_modules` is per worktree: clone it from the shared checkout with `cp -Rc` when the lockfiles match, otherwise `npm ci`. The shared copy is kept current by `scripts/refresh-shared-checkout.sh` at session end. -- **Three issues in flight**, not five: every implementer and every worktree-isolated reviewer runs - a `dotnet` build and a web build, and the cap is 3–4 concurrent builds gated on free RAM - (`process.build-concurrency-limits`). The Agent hook enforces the RAM gate for agents launched by - the Agent tool, not for a workflow's agents, so the orchestrator holds the count itself. +- **One .NET-building slot at a time on this host; docs and Python slots may run beside it.** A + slot's correctness reviewer builds .NET in its own worktree, and on a rubric-class change the + cross-family fallback is a second such reviewer, so the review round of ONE .NET slot is two + build-and-test pipelines on top of the implementer's lingering MSBuild node servers. Measured + 2026-09-05 on the 16 GB Mac: three slots in review reached load 82 and swap exhaustion; one slot's + round alone took swap from 6.8 GB to 10.8 GB in three minutes. The scripts therefore run the + cross-family step after the lenses return, never beside them (`process.build-concurrency-limits` + is the standing rule; the Agent hook's RAM gate does not see a workflow's agents, so the + orchestrator holds the count itself). - **Live-E2E runs per worktree on the slot's own port.** `scripts/e2e-local.sh` refuses concurrent runs *within one repo root* because each run re-copies that root's `wwwroot`; across worktrees there is nothing shared but the ports, so every slot gets a distinct `port` argument, one run at a diff --git a/docs/handoffs/orchestrator-prompt.md b/docs/handoffs/orchestrator-prompt.md index 0af2d2d13..66475a6ce 100644 --- a/docs/handoffs/orchestrator-prompt.md +++ b/docs/handoffs/orchestrator-prompt.md @@ -7,10 +7,10 @@ Way of working: - Size every subagent to its task; this is a rule, not a preference, because value per token is what the session is judged on. Picking and refuting: sonnet, medium. Recon on a large issue, implementation, fixing and correctness review: Opus at high (xhigh for a lock, threading or migration fix). Fable is for orchestrating and for the frontier escalations the kickoff lists, never for implementing. Small, well-specified fixes and mechanical finishing (rebase, push, PR body, label clearing): sonnet, medium. Conformance review: sonnet, high. Cross-family review through `codex exec` for the rubric's risk classes (locks, auth, API write paths, migrations, more than ~150 changed C# lines); when Codex cannot run, the workflow substitutes a cold same-family review-only agent and states the substitution in the PR body and its return, and your verdict note repeats it — a substitution is never silent. State the model and effort in every launch and revisit them when an incident degrades a model. - You never pick, claim, code or push yourself. A picker (sonnet, medium; `.claude/workflows/ersatztv-pick-next.js` with the taken list as args) runs `scripts/select-queue.sh`, applies the kickoff's claim and bundle rules to live Gitea state, and two refuters try to overturn it; you accept, or take a refuter's better pick when its rule is right. Then one issue-build workflow per pick, from `.claude/workflows/ersatztv-issue-build.js`, passing the session's commit trailer as `trailer`. - Before picking anything, finish what is already open: every open non-Renovate PR and every `in-progress` issue with a branch is a paused branch, resumed through `.claude/workflows/ersatztv-resume-branch.js` from a JSON brief. -- Keep three issues in flight, each in its own worktree under `~/orca/workspaces/ersatztv/wt-` on branch `-`, cut from a fresh `origin/main` by absolute path, never under `/tmp`, and each with its own E2E port passed as `port`. Refill a slot as soon as one merges. Pass every running issue's files as the avoid list so two agents never edit one file; a pick that needs a file another slot holds waits for that slot. +- Keep at most one .NET-building issue in flight (a docs or Python issue may run beside it), each in its own worktree under `~/orca/workspaces/ersatztv/wt-` on branch `-`, cut from a fresh `origin/main` by absolute path, never under `/tmp`, and each with its own E2E port passed as `port`. Refill a slot as soon as one merges. Pass every running issue's files as the avoid list so two agents never edit one file; a pick that needs a file another slot holds waits for that slot. - Each implementer claims its own issue after the four-way check (open PRs, remote branches, comments predating the label, a fresh fetch), with the `in-progress` label and a claiming comment; rebases onto a fresh `origin/main`, runs the local gate and sits inside the two-lens review loop BEFORE its single push; its finisher opens a PR whose body says `fixes #N` for every issue in the bundle and states the cross-family review status, and posts the `## Closing record` on each issue with the evidence per `## Done-when` box, ticking nothing — a box ticked by the agent it certifies is self-consent. - You referee: read the verdicts and the evidence, send a PR back for anything that lets a route or test pass having done nothing, tick each Done-when box whose evidence holds, re-read the head sha, then post the verdict with `scripts/post-review-verdict.sh MERGEABLE ` and tick the review box. Merge only through the Gitea merge tool with the full head sha, so the consent hook derives consent from the ticked boxes, the sha-bound verdict and green CI. A rebase voids the verdict: the finisher rebases once before the push and reports whether the patch changed; a changed patch gets one more review round before you post. After the merge clear the `in-progress` label and remove the worktree. -- Builds are capped at three to four concurrent and gated on free RAM; live-E2E runs one at a time per worktree on the slot's own port, and a busy port is reported, never taken over. Kill only PIDs you started, gently. A `dotnet test` or E2E run silent for ten minutes is a hang. +- One .NET slot means two build pipelines in its review round (the correctness lens and, on a rubric change, the fallback reviewer, run one after the other); measure `memory_pressure -Q` and swap before every launch; live-E2E runs one at a time per worktree on the slot's own port, and a busy port is reported, never taken over. Kill only PIDs you started, gently. A `dotnet test` or E2E run silent for ten minutes is a hang. - Reviewers never delete anything outside a scratch directory they created under `/private/tmp` and never build a path with `..` segments; the auto-mode classifier is the last line, not the first. - Under an API incident (watch `https://status.claude.com/api/v2/status.json`), move mechanical finishers and probe-driven reviews to whichever model is healthy and park large implementation until it clears; back off before retrying. - Report to the user only what changes what they would do next: merged and closed issues, a send-back and why, a hang, an incident. Before any stop: the H12 audit (`process.issue-qualification-audit`), `scripts/refresh-shared-checkout.sh`, and a handoff memory naming every open branch, its worktree, its PR and its next step, so a resumed session finishes those before picking anything new. diff --git a/web/scripts/orchestration-workflow-loop.test.mjs b/web/scripts/orchestration-workflow-loop.test.mjs index 35bd5936d..606875375 100644 --- a/web/scripts/orchestration-workflow-loop.test.mjs +++ b/web/scripts/orchestration-workflow-loop.test.mjs @@ -38,16 +38,30 @@ 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, fixerCommits = null }) { +function run(script, args, { reviewRounds, landOverrides = {}, postRebase = null, fixerNull = false, fixerDone = true, fixerNoCommit = false }) { const calls = []; let round = 0; let fixCount = 0; + let lensesInFlight = 0; + const xfamilyStartedWithLensesInFlight = []; 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 }; + if (label.startsWith('impl:') || label.startsWith('fix:#')) return { done: true, summary: '', verified: '', left: '', commits: IMPL_COMMITS, head_sha: 'aaaaaaa' }; + if (label.startsWith('review:codex:')) { + xfamilyStartedWithLensesInFlight.push(lensesInFlight); + return { verdict: 'merge', findings: [], ran: false }; + } + if (label.startsWith('review:fallback:')) { + xfamilyStartedWithLensesInFlight.push(lensesInFlight); + const spec = reviewRounds[round - 1] || { findings: [] }; + return { verdict: 'merge', findings: spec.findings }; + } if (label.startsWith('review:')) { const spec = reviewRounds[round - 1] || { findings: [] }; + lensesInFlight += 1; + await new Promise((resolve) => setTimeout(resolve, 5)); + lensesInFlight -= 1; if (spec.lensesNull) return null; return { verdict: 'merge', findings: spec.findings }; } @@ -55,7 +69,7 @@ function run(script, args, { reviewRounds, landOverrides = {}, postRebase = null fixCount += 1; if (fixerNull) return null; if (!fixerDone) return { done: false, summary: 'stopped', verified: '', left: 'blocked', commits: IMPL_COMMITS }; - return { done: true, summary: '', verified: '', left: '', commits: fixerCommits !== null ? fixerCommits : `f${fixCount}xxxxx fix round ${fixCount}\n${IMPL_COMMITS}` }; + return { done: true, summary: '', verified: '', left: '', commits: fixerNoCommit ? IMPL_COMMITS : `f${fixCount}xxxxx fix round ${fixCount}\n${IMPL_COMMITS}`, head_sha: fixerNoCommit ? 'aaaaaaa' : `f${fixCount}xxxxx` }; } if (label.startsWith('land:')) return { done: true, summary: '', verified: '', left: '', commits: '', pr_url: 'http://pr/1', head_sha: 'deadbeef', patch_changed: false, ...landOverrides }; throw new Error(`unexpected agent label ${label}`); @@ -67,7 +81,7 @@ function run(script, args, { reviewRounds, landOverrides = {}, postRebase = null } return Promise.all(thunks.map((t) => t().catch(() => null))); }; - return script(args, agent, parallel, () => {}, () => {}).then((result) => ({ result, calls, fixCount })); + return script(args, agent, parallel, () => {}, () => {}).then((result) => ({ result, calls, fixCount, xfamilyStartedWithLensesInFlight })); } const cases = [ @@ -94,10 +108,10 @@ describe.each(cases)('%s review loop', (name, args) => { expect(result.error).toBeUndefined(); expect(fixCount).toBe(1); const hist = finisherPrompt(calls); - expect(hist).toContain('round 1: 2 lens(es); 0 blocking, 2 should-fix, 0 nit; answered by fix commit(s): f1xxxxx fix round 1'); + expect(hist).toContain('round 1: 2 lens(es); 0 blocking, 2 should-fix, 0 nit; answered by the fix commit(s) in git log --oneline aaaaaaa..f1xxxxx'); expect(hist).toContain('round 2: 2 lens(es); 0 blocking, 0 should-fix, 0 nit; clean — loop ended'); expect(hist).not.toContain(IMPL_COMMITS); - expect(hist).toContain('what each of these fix commits changed'); + expect(hist).toContain('for exactly the commits git log --oneline lists in these ranges: aaaaaaa..f1xxxxx'); }); it('a blocking finding that survives two fix rounds stops before the push', async () => { @@ -147,7 +161,7 @@ describe.each(cases)('%s review loop', (name, args) => { }); it('findings answered without a new commit are described as such, never as a clean round one', async () => { - const { calls } = await run(script, args, { reviewRounds: [{ findings: [finding('should-fix')] }, { findings: [] }], fixerCommits: IMPL_COMMITS }); + const { calls } = await run(script, args, { reviewRounds: [{ findings: [finding('should-fix')] }, { findings: [] }], fixerNoCommit: true }); const p = finisherPrompt(calls); expect(p).toContain('answered without a new commit'); expect(p).toContain('every finding was answered without a new commit'); @@ -159,6 +173,16 @@ 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 cross-family step starts only after both lenses have returned', async () => { + const rubricArgs = { ...args, risk: 'rubric' }; + const { result, calls, xfamilyStartedWithLensesInFlight } = 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(xfamilyStartedWithLensesInFlight).toEqual([0, 0]); + expect(finisherPrompt(calls)).toContain('substituted a cold same-family review-only agent'); + }); + it('a should-fix on the pushed head after a patch-changing rebase is an error', async () => { const { result } = await run(script, args, { reviewRounds: [{ findings: [] }],