fix(911): a should-fix finding sends the round back, a failed-lens round is an error, and a harness executes the loop #912

Merged

timothy merged 3 commits from 911-should-fix-reaches-fixer into main 2026-09-05 04:43:45 +02:00

Conversation 1 Commits 3 Files Changed 5

+238 -20

timothy commented 2026-09-05 03:23:04 +02:00

Owner

Copy Link

Copy Source

fixes #911

Root cause

The fixer brief in both orchestration workflows said "fix every blocking and should-fix one", but the loop condition sent a round back only on blocking. A round whose lens wrote verdict: merge beside real should-fix findings exited the loop unanswered, and the finisher was then told to describe "how the findings were answered" with no fix commit to read — on #554 / PR #910 it wrote that they were "already folded into the diff". The same shape sat one layer deeper: a round in which every lens failed emptied to zero findings and read as clean.

Fix

• Any blocking or should-fix finding sends the round back; nits alone end the loop; the two-round cap stays; an open should-fix after two rounds returns an error before the push, like a blocking one. The post-rebase round on the pushed head applies the same filter.
• A round with no reviews (every lens null) is an error before the push, at every placement; a fixer that dies or reports no done is an error too.
• The history binds each fix to the round it answered and records 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, so the finisher is told to read git show). The finisher quotes that history verbatim between markers and describes only the listed fix commits; an empty set is "answered without a new commit" when a fix round ran, "round one was clean" only when none did.
• In a resume, the reviewer brief states that the existing PR's stale head and body are the expected state until the finisher pushes — three consecutive #554 resume rounds had graded exactly that as blocking.
• docs/handoffs/orchestration.md → Landing a branch, steps 2 and 3, say which severities send a round back and that the PR body carries the recorded history.

Measured

web/scripts/orchestration-workflow-loop.test.mjs compiles both committed script bodies as AsyncFunctions with stubbed agent/parallel/phase/log and pins eleven paths per script (22 tests). Reviewer-run mutation sweep on the final head, 11 mutants × 2 scripts, zero survivors: reverting the loop condition reddens 6 per script; each zero-lens guard, each half of the fixer guard, the binding, the set difference and the empty-fix branch redden their own case in both scripts.

Gate	Result
PYTHONPATH=. python3 -m pytest scripts/tests -q	1569 passed, 3 skipped (reviewer re-ran on the final head: same)
npx vitest run scripts/orchestration-workflow-loop.test.mjs	22 passed
npm run lint / npm run typecheck	clean
scripts/check-doc-narrative.py --diff origin/main	0 advisory warnings
scripts/decisions_validate.py	OK (ui-e2e-harness.md prose edit carries Decisions-Edit: yes)

Review

Four cold review rounds (Opus, high, worktree-isolated, same-family — docs, JS prompts and a vitest harness are outside the cross-family rubric). Round one: 1 blocking (a zero-lens round published as clean) and 5 should-fix; round two: 3 should-fix (an empty-fix sentence that could assert a clean round, three unpinned guards, a wrong count in the commit message); round three: 1 blocking introduced by a cosmetic edit to a whole-file-pinned web/vite.config.ts (reverted rather than re-pinned) and two nits; round four: no findings, VERDICT: merge. Every finding was verified by the reviewer by execution.

Cross-family review: not required (routine risk class under process.independent-review-rubric).

Deferred

web/vite.config.ts's comment still names one web/scripts test where two now live; the file is pinned whole (test_image_build_delegates_the_spa_suite.py) and the decision record carries the accurate plural. Left deliberately.

🤖 Generated with Claude Code

https://claude.ai/code/session_015QqCpYFsKgnAnx6jVwrKiV

fixes #911 ## Root cause The fixer brief in both orchestration workflows said "fix every blocking and should-fix one", but the loop condition sent a round back only on `blocking`. A round whose lens wrote `verdict: merge` beside real `should-fix` findings exited the loop unanswered, and the finisher was then told to describe "how the findings were answered" with no fix commit to read — on #554 / PR #910 it wrote that they were "already folded into the diff". The same shape sat one layer deeper: a round in which every lens failed emptied to zero findings and read as clean. ## Fix - Any `blocking` or `should-fix` finding sends the round back; nits alone end the loop; the two-round cap stays; an open should-fix after two rounds returns an error before the push, like a blocking one. The post-rebase round on the pushed head applies the same filter. - A round with no reviews (every lens null) is an error before the push, at every placement; a fixer that dies or reports no `done` is an error too. - The history binds each fix to the round it answered and records 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, so the finisher is told to read `git show`). The finisher quotes that history verbatim between markers and describes only the listed fix commits; an empty set is "answered without a new commit" when a fix round ran, "round one was clean" only when none did. - In a resume, the reviewer brief states that the existing PR's stale head and body are the expected state until the finisher pushes — three consecutive #554 resume rounds had graded exactly that as blocking. - `docs/handoffs/orchestration.md` → Landing a branch, steps 2 and 3, say which severities send a round back and that the PR body carries the recorded history. ## Measured `web/scripts/orchestration-workflow-loop.test.mjs` compiles both committed script bodies as AsyncFunctions with stubbed `agent`/`parallel`/`phase`/`log` and pins eleven paths per script (22 tests). Reviewer-run mutation sweep on the final head, 11 mutants × 2 scripts, zero survivors: reverting the loop condition reddens 6 per script; each zero-lens guard, each half of the fixer guard, the binding, the set difference and the empty-fix branch redden their own case in both scripts. | Gate | Result | |---|---| | `PYTHONPATH=. python3 -m pytest scripts/tests -q` | 1569 passed, 3 skipped (reviewer re-ran on the final head: same) | | `npx vitest run scripts/orchestration-workflow-loop.test.mjs` | 22 passed | | `npm run lint` / `npm run typecheck` | clean | | `scripts/check-doc-narrative.py --diff origin/main` | 0 advisory warnings | | `scripts/decisions_validate.py` | OK (`ui-e2e-harness.md` prose edit carries `Decisions-Edit: yes`) | ## Review Four cold review rounds (Opus, high, worktree-isolated, same-family — docs, JS prompts and a vitest harness are outside the cross-family rubric). Round one: 1 blocking (a zero-lens round published as clean) and 5 should-fix; round two: 3 should-fix (an empty-fix sentence that could assert a clean round, three unpinned guards, a wrong count in the commit message); round three: 1 blocking introduced by a cosmetic edit to a whole-file-pinned `web/vite.config.ts` (reverted rather than re-pinned) and two nits; round four: no findings, `VERDICT: merge`. Every finding was verified by the reviewer by execution. Cross-family review: not required (routine risk class under process.independent-review-rubric). ## Deferred `web/vite.config.ts`'s comment still names one `web/scripts` test where two now live; the file is pinned whole (`test_image_build_delegates_the_spa_suite.py`) and the decision record carries the accurate plural. Left deliberately. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_015QqCpYFsKgnAnx6jVwrKiV

timothy added 3 commits 2026-09-05 03:23:04 +02:00

fix(911): a should-fix finding sends the round back, and the finisher reports the recorded review history ... ade14a7521

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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015QqCpYFsKgnAnx6jVwrKiV

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 ... 692affca97

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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015QqCpYFsKgnAnx6jVwrKiV

fix(911): in a resume, the existing PR's stale head is the expected state until the push, not a finding ... 14e075174b

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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015QqCpYFsKgnAnx6jVwrKiV

timothy referenced this pull request 2026-09-05 03:23:42 +02:00

Orchestration workflows: should-fix findings never reach the fixer, and the finisher then attests they were folded in #911

timothy commented 2026-09-05 03:23:44 +02:00

Author

Owner

Copy Link

Copy Source

Review-verdict: MERGEABLE @ 14e0751

Four cold review rounds (Opus high, worktree-isolated, same-family: docs + JS prompts + a vitest harness, outside the cross-family rubric); round four clean after an 11-mutant x 2-script sweep with zero survivors and the full Python suite green on the final head. Cross-family review: not required (routine risk class). Orchestrator session.

Review-verdict: MERGEABLE @ 14e0751 Four cold review rounds (Opus high, worktree-isolated, same-family: docs + JS prompts + a vitest harness, outside the cross-family rubric); round four clean after an 11-mutant x 2-script sweep with zero survivors and the full Python suite green on the final head. Cross-family review: not required (routine risk class). Orchestrator session.

timothy merged commit 4a56d54283 into main 2026-09-05 04:43:45 +02:00

timothy deleted branch 911-should-fix-reaches-fixer 2026-09-05 04:43:49 +02:00

Sign in to join this conversation.

Reviewers

No Reviewers

Labels

Clear labels

ad-hoc

One-off / ad-hoc work not tracked by a dedicated issue

api

REST API / HTTP endpoints

bug

Something isn't working

ci-cd

Build, test, deploy pipeline

content

Channel content / schedules / playlists

dependencies

Dependency updates (Renovate)

enhancement

New feature or improvement

frontend

ChicoryTV React SPA frontend

in-progress

Claimed by an active session — do not pick up

jellyfin

Jellyfin tuner / IPTV integration

parked

Excluded from automatic queue pickup; work only when explicitly selected

priority: high

priority: low

priority: medium

review

Adversarial review finding

security

Security / vulnerability fix

No labels

Milestone

No items

No Milestone

Projects

Clear projects

No projects

Assignees

Clear assignees

renovate timothy (Timothy)

No Assignees

1 Participants

Notifications

Subscribe

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: timothy/ersatztv#912