From 692affca974634b4698ab12ae5152a50b4bc1022 Mon Sep 17 00:00:00 2001 From: Timothy Date: Sat, 5 Sep 2026 02:07:50 +0200 Subject: [PATCH] fix(911): a round with no reviews is an error, the history binds each fix to the round it answered, and a harness executes the loop MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A round in which every lens returned null read as a clean pass and would now have been quoted verbatim into the PR body; it is an error before the push, in the review loop and in the post-rebase round, which also gains the same blocking-or-should-fix filter; a fixer that dies or stops (no done) is an error too, the same test the implementer already gets. The history entry now carries the fix that answered that round and only the commits that fix added (a line-set difference against the previous branch log — a fixer that reformats or rebases mid-loop defeats it, which is why the finisher is told to read git show, not the list). An empty fix-commit set is described as "answered without a new commit" when a fix round ran, and as "round one was clean" only when none did. web/scripts/orchestration-workflow-loop.test.mjs compiles the committed script bodies with stubbed agent/parallel and pins eleven paths per script (22 tests). Measured: reverting the loop condition to blocking-only reddens six cases per script (every case that needs a should-fix round to reach the fixer); deleting any of the three zero-lens guards, the fixer guard or its done half, or the empty-fix sentence branch reddens its own case, in both scripts. web/vite.config.ts is untouched: it is pinned whole by test_image_build_delegates_the_spa_suite.py, comments included. Decisions-Edit: yes Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_015QqCpYFsKgnAnx6jVwrKiV --- .claude/workflows/ersatztv-issue-build.js | 27 ++- .claude/workflows/ersatztv-resume-branch.js | 27 ++- docs/decisions/records/ci/ui-e2e-harness.md | 2 +- docs/handoffs/orchestration.md | 14 +- .../orchestration-workflow-loop.test.mjs | 170 ++++++++++++++++++ 5 files changed, 222 insertions(+), 18 deletions(-) create mode 100644 web/scripts/orchestration-workflow-loop.test.mjs diff --git a/.claude/workflows/ersatztv-issue-build.js b/.claude/workflows/ersatztv-issue-build.js index a01eafb6f..d11b49b42 100644 --- a/.claude/workflows/ersatztv-issue-build.js +++ b/.claude/workflows/ersatztv-issue-build.js @@ -159,10 +159,14 @@ Be adversarial; report only what you verified, with evidence. blocking = done co 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 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 }] +const history = [{ round, reviews, fix: null, fix_commits: [] }] 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} @@ -174,19 +178,29 @@ ${JSON.stringify(reviews.flatMap(r => r.findings.filter(f => f.severity !== 'nit Then re-run the LOCAL GATE and STOP without pushing; the reviewers read the worktree again. ${GATE} 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 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, fix, reviews }) + history.push({ round, reviews, fix: null, fix_commits: [] }) } 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 REVIEW_HISTORY = history.map(h => `round ${h.round}: ${h.reviews.length} lens(es); ${h.reviews.flatMap(r => r.findings).filter(f => f.severity === 'blocking').length} blocking, ${h.reviews.flatMap(r => r.findings).filter(f => f.severity === 'should-fix').length} should-fix, ${h.reviews.flatMap(r => r.findings).filter(f => f.severity === 'nit').length} nit${h.fix ? `; answered by the fix commit(s): ${(h.fix.commits || '').split('\n')[0]}` : ''}`).join('\n') +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') 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} -Then ONE push: git push -u origin ${BRANCH}. Open the PR with the Gitea API (POST ${API}/pulls; head=${BRANCH}, base=main, title, body). The body must contain "fixes #N" for every issue in the bundle so the merge closes them, the root cause for a bug fix, the measured numbers, the review history VERBATIM as recorded by the workflow — "${REVIEW_HISTORY}" — followed by what each fix commit changed (from git log, not from memory), 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: +Then ONE push: git push -u origin ${BRANCH}. Open the PR with the Gitea API (POST ${API}/pulls; head=${BRANCH}, base=main, title, body). The body must contain "fixes #N" for every issue in the bundle so the merge closes them, the root cause for a bug fix, the measured numbers, the review history VERBATIM as recorded by the workflow, one line per round, between the markers <>: +<> +${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: 🤖 Generated with [Claude Code](https://claude.com/claude-code) ${SESSION_URL} @@ -205,7 +219,8 @@ if (land.patch_changed) { log(`${REF}: patch changed on rebase — one more review round on the pushed head before any verdict`) round++ post_rebase_reviews = (await review(round)).filter(Boolean) - const late = post_rebase_reviews.flatMap(r => r.findings.filter(f => f.severity === 'blocking')) - if (late.length) return { issues, error: 'blocking findings on the pushed head after the pre-push rebase; no verdict may be posted', pr_url: land.pr_url, head_sha: land.head_sha, blocking_remaining: late, cross_family: xfamily, history, post_rebase_reviews } + if (!post_rebase_reviews.length) return { issues, error: 'the post-rebase review round produced no reviews (every lens failed); pushed, no verdict may be posted', pr_url: land.pr_url, head_sha: land.head_sha, cross_family: xfamily, history } + const late = actionable(post_rebase_reviews) + if (late.length) return { issues, error: 'blocking or should-fix findings on the pushed head after the pre-push rebase; no verdict may be posted', pr_url: land.pr_url, head_sha: land.head_sha, findings_remaining: late, cross_family: xfamily, history, post_rebase_reviews } } return { issues, pr_url: land.pr_url, head_sha: land.head_sha, patch_changed: !!land.patch_changed, cross_family: xfamily, impl, land, history, post_rebase_reviews } diff --git a/.claude/workflows/ersatztv-resume-branch.js b/.claude/workflows/ersatztv-resume-branch.js index 291558355..bc32ba874 100644 --- a/.claude/workflows/ersatztv-resume-branch.js +++ b/.claude/workflows/ersatztv-resume-branch.js @@ -127,10 +127,14 @@ Be adversarial; report only what you verified, with evidence. blocking = done co 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 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 }] +const history = [{ round, reviews, fix: null, fix_commits: [] }] 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} @@ -139,19 +143,29 @@ You are the fixer. Reviewers found these problems in the unpushed, rebased branc ${JSON.stringify(reviews.flatMap(r => r.findings.filter(f => f.severity !== 'nit')), null, 1)} 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 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, fix, reviews }) + history.push({ round, reviews, fix: null, fix_commits: [] }) } 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 REVIEW_HISTORY = history.map(h => `round ${h.round}: ${h.reviews.length} lens(es); ${h.reviews.flatMap(r => r.findings).filter(f => f.severity === 'blocking').length} blocking, ${h.reviews.flatMap(r => r.findings).filter(f => f.severity === 'should-fix').length} should-fix, ${h.reviews.flatMap(r => r.findings).filter(f => f.severity === 'nit').length} nit${h.fix ? `; answered by the fix commit(s): ${(h.fix.commits || '').split('\n')[0]}` : ''}`).join('\n') +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') 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} -Then git push --force-with-lease origin ${BRANCH}. ${args.pr ? `Update PR #${args.pr}'s body (PATCH ${API}/pulls/${args.pr}) so it describes the branch as it now is` : `Open a PR (POST ${API}/pulls; head=${BRANCH}, base=main)`}: the body must contain "fixes #N" for every issue in the bundle, the root cause for a bug fix, the measured numbers, the review history VERBATIM as recorded by the workflow — "${REVIEW_HISTORY}" — followed by what each fix commit changed (from git log, not from memory), the cross-family review status verbatim — "${xfamily}" — every deliberately-left item with an issue number, and end with: +Then git push --force-with-lease origin ${BRANCH}. ${args.pr ? `Update PR #${args.pr}'s body (PATCH ${API}/pulls/${args.pr}) so it describes the branch as it now is` : `Open a PR (POST ${API}/pulls; head=${BRANCH}, base=main)`}: the body must contain "fixes #N" for every issue in the bundle, the root cause for a bug fix, the measured numbers, the review history VERBATIM as recorded by the workflow, one line per round, between the markers <>: +<> +${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: 🤖 Generated with [Claude Code](https://claude.com/claude-code) ${SESSION_URL} @@ -168,7 +182,8 @@ if (land.patch_changed) { log(`${REF}: patch changed on rebase — one more review round on the pushed head before any verdict`) round++ post_rebase_reviews = (await review(round)).filter(Boolean) - const late = post_rebase_reviews.flatMap(r => r.findings.filter(f => f.severity === 'blocking')) - if (late.length) return { issues, error: 'blocking findings on the pushed head after the pre-push rebase; no verdict may be posted', pr_url: land.pr_url || args.pr, head_sha: land.head_sha, blocking_remaining: late, cross_family: xfamily, history, post_rebase_reviews } + if (!post_rebase_reviews.length) return { issues, error: 'the post-rebase review round produced no reviews (every lens failed); pushed, no verdict may be posted', pr_url: land.pr_url || args.pr, head_sha: land.head_sha, cross_family: xfamily, history } + const late = actionable(post_rebase_reviews) + if (late.length) return { issues, error: 'blocking or should-fix findings on the pushed head after the pre-push rebase; no verdict may be posted', pr_url: land.pr_url || args.pr, head_sha: land.head_sha, findings_remaining: late, cross_family: xfamily, history, post_rebase_reviews } } return { issues, pr_url: land.pr_url || args.pr, head_sha: land.head_sha, patch_changed: !!land.patch_changed, cross_family: xfamily, work, land, history, post_rebase_reviews } diff --git a/docs/decisions/records/ci/ui-e2e-harness.md b/docs/decisions/records/ci/ui-e2e-harness.md index 72e8fd608..f61d6b768 100644 --- a/docs/decisions/records/ci/ui-e2e-harness.md +++ b/docs/decisions/records/ci/ui-e2e-harness.md @@ -41,7 +41,7 @@ context, which gives the login specs a signed-out browser without a logout dance CI — a retry lets a flaky flow merge looking green. Coupling worth knowing: vitest's default `include` glob would run `web/e2e/*.spec.ts` under jsdom, so `vite.config.ts` excludes `e2e/**` by spreading `configDefaults.exclude` — not by narrowing `include` to `src/**`, which would silently stop collecting -the real vitest test under `web/scripts/`. +the real vitest tests under `web/scripts/`. **Lifecycle correctness in `scripts/e2e-ui.sh`** — all four found by adversarial review, **none by a passing run**; that is the transferable lesson (green runs never exercise the failure/interrupt paths). diff --git a/docs/handoffs/orchestration.md b/docs/handoffs/orchestration.md index c51ab670e..361f69fb3 100644 --- a/docs/handoffs/orchestration.md +++ b/docs/handoffs/orchestration.md @@ -81,21 +81,25 @@ the review loop *inside* the worktree, before the single push: 2. **Reviewers** read the worktree diff (`git diff origin/main...HEAD`) and run the gate in their own worktrees. A round carrying any `blocking` or `should-fix` finding goes back to the fixer regardless of the lens's own verdict word (a `should-fix` is a real defect by definition); nits - alone end the loop. After two fix rounds an open finding stops the workflow before the push and - the orchestrator decides. The loop ends on a clean round, never on "round one's findings are - fixed". + alone end the loop. A round in which every lens failed is an error, never a clean round. After + two fix rounds an open finding stops the workflow before the push and the orchestrator decides. + The same filter applies to the post-rebase round on the pushed head. The loop ends on a clean + round, never on "round one's findings are fixed". 3. **Finisher**: fetch; if `origin/main` moved, rebase, re-run the gate, and compare the patch-id before and after — a changed patch (a conflict resolved, an artifact regenerated) sends the branch through one more review round before the verdict, because the reviewed tree must be the pushed tree. Then **one push**, `git push -u origin `; open the PR with `fixes #N` for - every issue in the bundle, the cross-family review status, and the session trailers; arm the CI + every issue in the bundle, the review history the workflow recorded (one line per round, quoted + verbatim — the finisher never describes a fix it did not see), the cross-family review status, + and the session trailers; arm the CI monitor on the head sha; post the `## Closing record` on each issue with the evidence for every `## Done-when` box, ticking none. 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 + 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 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 new file mode 100644 index 000000000..35bd5936d --- /dev/null +++ b/web/scripts/orchestration-workflow-loop.test.mjs @@ -0,0 +1,170 @@ +// Pins the review-loop contract of the orchestration Workflow scripts (ersatztv#907, #911): +// a round carrying any blocking OR should-fix finding goes back to the fixer whatever the lens's +// verdict word; nits alone end the loop; a round in which every lens failed is an error, never a +// clean pass; the recorded history binds each fix to the round it answered and names only the +// commits that fix added; and the post-rebase round applies the same filter. +// +// The scripts run inside the Workflow tool, which supplies agent/parallel/phase/log and top-level +// await/return. This harness compiles each script body as an AsyncFunction with stubbed versions of +// those, so it executes the COMMITTED control flow rather than a re-description of it. +import { readFileSync } from 'node:fs'; +import { fileURLToPath } from 'node:url'; +import { dirname, resolve } from 'node:path'; +import { describe, expect, it } from 'vitest'; + +const here = dirname(fileURLToPath(import.meta.url)); +const scriptsDir = resolve(here, '../../.claude/workflows'); +const AsyncFunction = Object.getPrototypeOf(async function () {}).constructor; + +function compile(name) { + const src = readFileSync(resolve(scriptsDir, name), 'utf8'); + const body = src.replace(/^export const meta = \{[\s\S]*?\n\}\n/, ''); + return new AsyncFunction('args', 'agent', 'parallel', 'phase', 'log', body); +} + +const TRAILER = 'Co-Authored-By: t \nClaude-Session: https://example/session'; +const buildArgs = { + issues: [1], slug: 's', title: 't', body_summary: 'b', done_condition: 'd', files_likely: [], + area: 'docs', size: 'small', risk: 'routine', needs_e2e: false, port: 8440, avoid: [], trailer: TRAILER, +}; +const resumeArgs = { + issues: [1], branch: 'b', wt: '/tmp/wt', pr: 42, mode: 'fix', title: 't', risk: 'routine', + needs_e2e: false, port: 8440, trailer: TRAILER, brief: '/tmp/brief.json', +}; + +const IMPL_COMMITS = 'aaaaaaa feat(1): the implementer commit'; +const finding = (severity) => ({ severity, file: 'f', summary: `${severity} finding`, evidence: 'e' }); + +// 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 }) { + const calls = []; + let round = 0; + let fixCount = 0; + 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('review:')) { + const spec = reviewRounds[round - 1] || { findings: [] }; + if (spec.lensesNull) return null; + return { verdict: 'merge', findings: spec.findings }; + } + if (label.startsWith('fix:r')) { + 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}` }; + } + 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}`); + }; + const parallel = async (thunks) => { + round += 1; + 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 })); +} + +const cases = [ + ['ersatztv-issue-build.js', buildArgs], + ['ersatztv-resume-branch.js', resumeArgs], +]; + +describe.each(cases)('%s review loop', (name, args) => { + const script = compile(name); + const finisherPrompt = (calls) => calls.find((c) => c.label.startsWith('land:')).prompt; + + it('a clean first round lands with zero fix rounds and says so', async () => { + const { result, calls, fixCount } = await run(script, args, { reviewRounds: [{ findings: [finding('nit')] }] }); + expect(result.error).toBeUndefined(); + expect(fixCount).toBe(0); + expect(finisherPrompt(calls)).toContain('round 1: 2 lens(es); 0 blocking, 0 should-fix, 2 nit; clean — loop ended'); + expect(finisherPrompt(calls)).toContain('no fix commit exists'); + }); + + it('a should-fix finding sends the round back even though the lens said merge', async () => { + const { result, calls, fixCount } = await run(script, args, { + reviewRounds: [{ findings: [finding('should-fix')] }, { findings: [] }], + }); + 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 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'); + }); + + it('a blocking finding that survives two fix rounds stops before the push', async () => { + const { result, calls, fixCount } = await run(script, args, { + reviewRounds: [{ findings: [finding('blocking')] }, { findings: [finding('blocking')] }, { findings: [finding('blocking')] }], + }); + expect(fixCount).toBe(2); + expect(result.error).toMatch(/^blocking findings after two fix rounds/); + expect(calls.some((c) => c.label.startsWith('land:'))).toBe(false); + }); + + it('a should-fix finding that survives two fix rounds stops before the push', async () => { + const { result, calls, fixCount } = await run(script, args, { + reviewRounds: [{ findings: [finding('should-fix')] }, { findings: [finding('should-fix')] }, { findings: [finding('should-fix')] }], + }); + expect(fixCount).toBe(2); + expect(result.error).toMatch(/^should-fix findings still open after two fix rounds/); + expect(calls.some((c) => c.label.startsWith('land:'))).toBe(false); + }); + + it('a round in which every lens failed is an error, not a clean round', async () => { + const { result, calls, fixCount } = await run(script, args, { reviewRounds: [{ lensesNull: true }] }); + expect(fixCount).toBe(0); + expect(result.error).toMatch(/^review round 1 produced no reviews/); + expect(calls.some((c) => c.label.startsWith('land:'))).toBe(false); + }); + + it('a round in which every lens failed AFTER a fix round is an error, not a clean round', async () => { + const { result, calls, fixCount } = await run(script, args, { + reviewRounds: [{ findings: [finding('should-fix')] }, { lensesNull: true }], + }); + expect(fixCount).toBe(1); + expect(result.error).toMatch(/^review round 2 produced no reviews/); + expect(calls.some((c) => c.label.startsWith('land:'))).toBe(false); + }); + + it('a fixer that dies is an error, not a clean round', async () => { + const { result, calls } = await run(script, args, { reviewRounds: [{ findings: [finding('should-fix')] }], fixerNull: true }); + expect(result.error).toMatch(/^fixer for round 1 returned nothing/); + expect(calls.some((c) => c.label.startsWith('land:'))).toBe(false); + }); + + it('a fixer that stops (done=false) is an error, not a clean round', async () => { + const { result, calls } = await run(script, args, { reviewRounds: [{ findings: [finding('should-fix')] }], fixerDone: false }); + expect(result.error).toMatch(/^fixer for round 1 stopped/); + expect(calls.some((c) => c.label.startsWith('land:'))).toBe(false); + }); + + 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 p = finisherPrompt(calls); + expect(p).toContain('answered without a new commit'); + expect(p).toContain('every finding was answered without a new commit'); + expect(p).not.toContain('round one was clean'); + }); + + it('a post-rebase round in which every lens failed is an error, never a verdict', async () => { + const { result } = await run(script, args, { reviewRounds: [{ findings: [] }], landOverrides: { patch_changed: true }, postRebase: 'lensesNull' }); + expect(result.error).toMatch(/^the post-rebase review round produced no reviews/); + }); + + 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: [] }], + landOverrides: { patch_changed: true }, + postRebase: [finding('should-fix')], + }); + expect(result.error).toMatch(/^blocking or should-fix findings on the pushed head/); + }); +});