From ade14a752189bb9a34bfecf7eb7476300b24e5c6 Mon Sep 17 00:00:00 2001 From: Timothy Date: Sat, 5 Sep 2026 01:41:00 +0200 Subject: [PATCH 1/3] fix(911): a should-fix finding sends the round back, and the finisher reports the recorded review history The fixer brief already said "fix every blocking and should-fix one"; the loop condition alone disagreed, so a merge-worded round with real defects skipped the fixer and the finisher attested to fixes it never saw (#554 / PR #910). Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_015QqCpYFsKgnAnx6jVwrKiV --- .claude/workflows/ersatztv-issue-build.js | 11 ++++++++--- .claude/workflows/ersatztv-resume-branch.js | 11 ++++++++--- docs/handoffs/orchestration.md | 7 +++++-- 3 files changed, 21 insertions(+), 8 deletions(-) diff --git a/.claude/workflows/ersatztv-issue-build.js b/.claude/workflows/ersatztv-issue-build.js index 4f3eb2108..a01eafb6f 100644 --- a/.claude/workflows/ersatztv-issue-build.js +++ b/.claude/workflows/ersatztv-issue-build.js @@ -158,11 +158,13 @@ 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')) let reviews = (await review(round)).filter(Boolean) let blocking = reviews.flatMap(r => r.findings.filter(f => f.severity === 'blocking')) +let sendBack = actionable(reviews) const history = [{ round, reviews }] -while (blocking.length && round < 3) { - log(`${REF} round ${round}: ${blocking.length} blocking, sending back`) +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} ${WORKTREE} @@ -175,13 +177,16 @@ Report.`, round++ reviews = (await review(round)).filter(Boolean) blocking = reviews.flatMap(r => r.findings.filter(f => f.severity === 'blocking')) + sendBack = actionable(reviews) history.push({ round, fix, reviews }) } 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') 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, what the reviewers found across ${round} round(s) and how it was answered, 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 — "${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: 🤖 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 e8d43ad5b..291558355 100644 --- a/.claude/workflows/ersatztv-resume-branch.js +++ b/.claude/workflows/ersatztv-resume-branch.js @@ -126,11 +126,13 @@ 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')) let reviews = (await review(round)).filter(Boolean) let blocking = reviews.flatMap(r => r.findings.filter(f => f.severity === 'blocking')) +let sendBack = actionable(reviews) const history = [{ round, reviews }] -while (blocking.length && round < 3) { - log(`${REF} round ${round}: ${blocking.length} blocking, sending back`) +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} You are the fixer. Reviewers found these problems in the unpushed, rebased branch; fix every blocking and should-fix one as new commits, or show with evidence why a finding is wrong: @@ -140,13 +142,16 @@ Re-run the LOCAL GATE and STOP without pushing. ${GATE}`, round++ reviews = (await review(round)).filter(Boolean) blocking = reviews.flatMap(r => r.findings.filter(f => f.severity === 'blocking')) + sendBack = actionable(reviews) history.push({ round, fix, reviews }) } 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') 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, what the reviewers found across ${round} round(s) and how it was answered, 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 — "${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: 🤖 Generated with [Claude Code](https://claude.com/claude-code) ${SESSION_URL} diff --git a/docs/handoffs/orchestration.md b/docs/handoffs/orchestration.md index 960893ac8..c51ab670e 100644 --- a/docs/handoffs/orchestration.md +++ b/docs/handoffs/orchestration.md @@ -79,8 +79,11 @@ the review loop *inside* the worktree, before the single push: touched `.cs` (`process.bom-format-detection-recipe`), and live-E2E on the slot's port for a write path or UI change (`testing.live-e2e-prepush-timing`). 2. **Reviewers** read the worktree diff (`git diff origin/main...HEAD`) and run the gate in their - own worktrees; the fixer answers `blocking` and `should-fix` findings; the loop ends on a clean - round, never on "round one's findings are fixed". + 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". 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 -- 2.47.3 From 692affca974634b4698ab12ae5152a50b4bc1022 Mon Sep 17 00:00:00 2001 From: Timothy Date: Sat, 5 Sep 2026 02:07:50 +0200 Subject: [PATCH 2/3] 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/); + }); +}); -- 2.47.3 From 14e075174bf8bcf54f250b10e931df73969c54b1 Mon Sep 17 00:00:00 2001 From: Timothy Date: Sat, 5 Sep 2026 03:05:42 +0200 Subject: [PATCH 3/3] fix(911): in a resume, the existing PR's stale head is the expected state until the push, not a finding Three consecutive #554 resume rounds graded "PR #910's head is still the first commit" as blocking; the push deliberately follows the loop. The reviewer brief now says so when args.pr is set. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_015QqCpYFsKgnAnx6jVwrKiV --- .claude/workflows/ersatztv-resume-branch.js | 2 +- docs/handoffs/orchestration.md | 5 +++-- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/.claude/workflows/ersatztv-resume-branch.js b/.claude/workflows/ersatztv-resume-branch.js index bc32ba874..946418533 100644 --- a/.claude/workflows/ersatztv-resume-branch.js +++ b/.claude/workflows/ersatztv-resume-branch.js @@ -95,7 +95,7 @@ const reviewCommon = (e2ePort) => `${COMMON} ${gateFor(e2ePort, 'your own isolated worktree (never ' + WT + ')')} -Diff: git -C ${WT} diff origin/main...HEAD (rebased, not yet pushed). Read-only except scratch you create under /private/tmp; do not commit or push. NEVER run rm -rf, git worktree remove, git branch -D or any delete outside a directory you created under /private/tmp this session, and never build a path with .. segments. If you must build or test, do it in your own isolated worktree, never in ${WT}: git fetch ${WT} ${BRANCH} && git checkout --detach FETCH_HEAD puts the branch there; gates sequentially; E2E there on port ${e2ePort} (the GATE above is written for your worktree and that port).` +Diff: git -C ${WT} diff origin/main...HEAD (rebased, not yet pushed). ${args.pr ? `PR #${args.pr} exists: its pushed head, its body and any earlier closing record are INTENTIONALLY behind this worktree until the finisher pushes after this review loop and resyncs them — a stale PR head or body is not a finding, and neither is "not pushed".` : 'No PR exists yet; the finisher opens it after this loop.'} Read-only except scratch you create under /private/tmp; do not commit or push. NEVER run rm -rf, git worktree remove, git branch -D or any delete outside a directory you created under /private/tmp this session, and never build a path with .. segments. If you must build or test, do it in your own isolated worktree, never in ${WT}: git fetch ${WT} ${BRANCH} && git checkout --detach FETCH_HEAD puts the branch there; gates sequentially; E2E there on port ${e2ePort} (the GATE above is written for your worktree and that port).` 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.' }, diff --git a/docs/handoffs/orchestration.md b/docs/handoffs/orchestration.md index 361f69fb3..b985a82e2 100644 --- a/docs/handoffs/orchestration.md +++ b/docs/handoffs/orchestration.md @@ -83,8 +83,9 @@ the review loop *inside* the worktree, before the single push: regardless of the lens's own verdict word (a `should-fix` is a real defect by definition); nits 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". + The same filter applies to the post-rebase round on the pushed head. In a resume, the existing + PR's stale head and body are the expected state until the finisher pushes, never a finding. 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 -- 2.47.3