Fix/critic block on spec - #467
Open
hadamrd wants to merge 8 commits into
Open
Conversation
…satisfiable ACs stop burning rounds THE BUG. The critic's rubric makes "missing acceptance criterion" a sev1 that ALWAYS blocks, and SEVERITY TRIAGE forbids demoting sev1/sev2. The only escape valve is sev3_demotion_round_threshold, which demotes COSMETICS — never what actually burns rounds. So when an acceptance criterion is UNSATISFIABLE, the critic is required to block and forbidden to clear it. The worker cannot edit the issue, so it answers the only way it can — by adding code — and the cycle repeats until a human intervenes. _round_guidance() compounds it: each round escalates SPECIFICITY (what -> why -> how -> patch sketch). That is right for a defect the worker can fix and useless for one it cannot. No round ever asks whether the criterion is satisfiable. THE PROOF THIS IS STRUCTURAL, NOT WORDING. On round 4 of a real PR the critic diagnosed the meta-cause correctly and wrote "escalate to a human to split the issue (land the resolver under a reduced AC, spin the walk into its own ticket)" — then still emitted request_changes and still blocked, because VALID_OVERALL had nowhere to put that conclusion. The diagnosis existed with no output channel and no state transition. MEASURED COST (retrobot, 2026-08-06): five repair passes on one PR at 17-44 min each, ~2h with zero merges. Three different issues hit the same shape: one bundled a code round with a live data grind; one graded a MECHANICAL analyser on a human judgement; one demanded blanking two wire-observed values to look scope-pure. THE FIX. - VALID_OVERALL gains `block_on_spec`. It is NOT a softer request_changes — it is a different ADDRESSEE. request_changes asks the WORKER to change the diff; block_on_spec asks a HUMAN to change the ISSUE and tells the worker to leave the diff alone. - New SpecDefect type (kind / criterion / why / fix / rounds_burned) with four valid kinds: unsatisfiable_in_one_pr, undecidable_by_deliverable, destroys_earned_work, environment. - _coerce_report parses spec_defects and enforces that the verdict and payload agree: a block_on_spec with no usable defect DEGRADES to request_changes rather than routing a human-addressed verdict at nobody, and stray defects on a normal verdict are dropped. - handle_critic_verdict gains the branch that actually saves the rounds: on block_on_spec it does NOT dispatch a revision, does not burn a critic iteration, parks the session and emits critic_verdict_blocked_on_spec naming the criterion and the fix. - briefs/critic.md.tmpl rewritten around it: a satisfiability pre-check that runs BEFORE the diff is read (can one PR satisfy it / can the deliverable decide it / does it destroy earned work); a carry-over test forcing every repeated blocking finding to be re-classified worker_clearable | spec_defective | environment; explicit round cost so blocking is weighed rather than free; "approve is a real outcome" replacing the old "emit request_changes with at least one sev3" rule that manufactured blocking pressure; and a hard rule never to require deleting a value earned from a live run. TESTS. tests/test_critic_block_on_spec.py. The load-bearing assertion is the NEGATIVE one — dispatch_revision is never called — so a third test drives the same harness with an ordinary request_changes and asserts a dispatch DOES happen (NEV-CTL-04: an instrument that cannot observe the event proves nothing). Mutation-checked: removing the runner branch fails exactly the two block_on_spec tests and leaves the control passing. Suite unchanged: 11 failed / 202 passed before, 11 failed / 205 passed after — the same pre-existing Windows failures (test_runner_boot_rotation collection error and test_runner_retry), plus the three new tests. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…he current generation _MODEL_PATTERN was ^claude-(opus|sonnet|haiku)-\d+-\d+(-...)?$ — BOTH numbers required. That cannot express claude-opus-5 / claude-sonnet-5, so a correct config died at startup with "unknown model alias — expected something like 'claude-opus-4-8'", which reads as a typo and sends the operator to edit the wrong file. Hit for real: after reinstalling from source the loop refused to start on critic.model=claude-sonnet-5. A validator that refuses the current generation of the thing it validates is worse than no validator — it blocks the right value with a confident error. Now ^claude-(opus|sonnet|haiku|fable)-\d+(-\d+)?(-[a-z0-9.-]+)?$: the minor version is optional and `fable` is a real family. Accepts claude-opus-5, claude-sonnet-5, claude-fable-5, claude-haiku-4-5-20251001 and the older claude-opus-4-8 shape; still rejects claude-5, opus-5, gpt-4, claude-turbo-5 and the empty string — the rejection half is asserted so the accept half is not vacuous (NEV-CTL-04). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… on Windows `signal.signal(signal.SIGUSR1, _pause_toggle)` runs unconditionally in _install_signal_handlers. On Windows `signal.SIGUSR1` does not exist, so boot raised AttributeError and `forge-loop run` could not start AT ALL. A pause CONVENIENCE was taking down the entire loop on an entire platform. Hit for real: after reinstalling from source the runner died on every start with "AttributeError: module 'signal' has no attribute 'SIGUSR1'". It also explains the pre-existing tests/test_runner_boot_rotation.py collection error on Windows. Guarded with hasattr. Nothing is lost there: pause/resume is driven by `cfg.pause_file`, which `_short_sleep` already polls every tick — the signal handler only toggles that touchfile, so the file itself remains the portable interface. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… round THE GAP. prep_worker_worktree already fetches a fresh base, so a NEW worker starts current. prep_repair_worktree did not: it fetched the PR branch and never merged the base in. So every repair round ran against whatever the base looked like when the branch was first cut. Measured on a live repo: a PR reached its FIFTH repair round still sitting on a base from hours earlier, while other PRs merged underneath it. The worker reasons about, and the critic reviews against, a repo that no longer exists. Git only warns about TEXTUAL conflicts — two workers independently "fixing" the same thing in incompatible ways is completely silent. THE FIX. _sync_base_into_worktree(): fetch origin/<base>, check rev-list HEAD..origin/<base>, and if the branch is behind, merge the base in. base_branch is threaded config -> dispatch -> run_repair_worker -> prep_repair_worktree. - MERGE, not rebase: the branch is already published as a PR, so rebasing needs a force-push and would invalidate the review history the critic's round counting reads from disk. - A CONFLICT ABORTS AND REPORTS (repair_base_sync_conflict) rather than leaving a half-merged tree. A stale tree is a bad starting point; a conflicted one is worse. The worktree is left exactly as it was. - Already-current is SILENT — that is the common case and does not deserve an event. TESTS (tests/test_repair_base_sync.py) run against real git repos, not mocks: a stale branch gains a commit that landed after it was cut while keeping its own work; a genuine same-line conflict emits the conflict event, leaves no MERGE_HEAD, and preserves the branch's content; and an already-current branch emits nothing — meaningful only because the first test proves the emitter can fire (NEV-CTL-04). Suite unchanged: 13 failed / 478 passed before, 13 failed / 481 passed after — the same pre-existing Windows failures plus the three new tests. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…whole loop THE STRUCTURAL PROBLEM. Repair ticks run their workers SYNCHRONOUSLY: dispatch collects fut.result() inside the ThreadPoolExecutor, so the tick sits inside the repair until the worker finishes — up to worker_timeout_s (3600s here). While that happens NO new issue is dispatched, however high its priority. Measured on a live repo: two PRs consumed an entire day at 17-44 minutes a round. The backlog sat untouched, and the product's north-star number went unmeasured the whole time, because the machine was busy re-reviewing the same two diffs. The earlier fixes each cover part of it and neither closes it: - block_on_spec parks a PR when the critic RECOGNISES the issue is at fault; - the base-sync stops repair rounds running against a stale base. Neither helps when the critic is simply wrong and confident — a sev1 it will re-emit forever. There was no bound at all. THE CAP. critic.max_repair_rounds (default 4, 0 disables). blocking_pr_repairs now reads count_prior_critic_rounds() and, past the cap, stops selecting the PR, emits repair_round_cap_reached with the counts, and labels the issue loop:needs-review. The PR is NOT closed and the branch is NOT touched — the work stays intact and a human resumes it. Lost throughput is recoverable; lost work is not. TESTS (tests/test_repair_round_cap.py): a PR past the cap is not selected and the parking is observable; a PR under the cap IS still selected (NEV-CTL-04 — without that, the first assertion would pass on a selector that returns nothing for any reason); cap=0 opts out. Suite unchanged: 19 failed / 632 passed before, 19 failed / 635 passed after. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ost rounds
Reading all six critic rounds on a real PR (a mutation-harness "instrument" PR)
shows block_on_spec caught only half the generator.
THE PATTERN. An instrument PR builds a measurement tool AND reports what it
measured, so its criteria split in two:
METHOD — what the DIFF controls: the harness runs, the operators match what
they claim, the report's headline numbers describe the same run as
its table. Real defects live here and MUST block. Two of the six
rounds were exactly this and were correct: a control-removal regex
that only matched a bare `return;`, and a report printing
"planted 24 killed 13 survived 11" while its own table disagreed.
OUTCOME — what the SUBJECT controls: how many survivors exist, how many rows
resolved, whether a giver offered at level 1.
The blocking criterion was "every survivor is strengthened or deleted". The PR
found 10 survivors, six of them in acquire.ts / farm.ts / npc.ts — modules it does
not touch — fixed 4 and marked 6 as follow-ups. Round 2 blocked: "a category the
issue does not allow". The author cannot bound that work when opening the PR
because the count is unknown until the instrument runs, so the criterion offers
only scope explosion or non-compliance. Neither failure was caused by the diff.
THE TELL, stated so it generalises: a criterion whose cost SCALES WITH WHAT THE
WORK DISCOVERS. Same shape as ">=8 of the 36 rows filled" and "the run reaches
level 5".
THE CORRECTION: demand disposition and honesty, never a disposition COUNT. Every
finding carries a verdict, nothing is silently dropped, and "deferred — lives in a
module this PR does not touch" is legitimate when it names the file and the reason.
Reporting 10 / fixing 4 / deferring 6 with reasons is COMPLETE; reporting 4 and
hiding 6 is a METHOD defect and still blocks.
Added as Q2b in the satisfiability pre-check (so it fires at round 1, not round 5)
and as a new SpecDefect kind `outcome_not_method`. Round 5 of that PR is the proof
this belongs at the front: the critic wrote "a human/PO must edit issue #168 to
resolve the self-contradiction" — it reached for escalation five rounds late and
had no verdict to put it in.
Explicitly NOT permission to slide: an unrun harness, a wrong operator, a
self-contradicting report or a dropped finding all still block.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…a ceiling
reserved_new_work_slots returned min(RESERVED_NEW_WORK_SLOTS, parallel - 1), i.e.
ONE issue whenever any repair was in flight. So raising `parallel` bought nothing:
the extra workers idled while the ready backlog waited.
Measured live: parallel=2 with one repair dispatched exactly one ready issue and
left the second slot empty for the whole tick — while two fully disjoint issues
were queued (a TypeScript SDK one and a Go manager one, no shared paths).
Repairs now keep exactly the slots they are USING (`repairs_pending`) and
everything left over goes to new work:
free = parallel - repairs_pending
return max(0, min(max(reserve, free), parallel - 1))
parallel=3/repairs=1 -> 2 (was 1); parallel=4/repairs=1 -> 3 (was 1);
parallel=4/repairs=2 -> 2 (was 1). Unchanged where it mattered before:
parallel=2/repairs=1 -> 1, parallel=1 -> 0 (repairs win the tick), no repairs -> 0,
no candidates -> 0, reserve<=0 -> 0. The #262 anti-starvation guarantee still holds
(its two tests pass untouched).
test_reserved_count_always_within_zero_to_parallel_minus_one asserted the OLD
constant. Its real invariant (0 <= reserved <= parallel-1) is kept; the second
assertion now states the free-slot contract and says why it changed, rather than
being weakened or deleted.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…he SDK's bundled claude run_maintenance shelled out to a bare "claude". The agent SDK ships its OWN claude binary and the workers use that one, so a machine can run workers perfectly while having no `claude` on PATH. This machine is exactly that. FileNotFoundError [WinError 2] escaped run_maintenance and killed the WHOLE runner every `maintenance_every_n_ticks` ticks (5 by default). Observed live: the loop died mid `maintenance_start` with the backlog full and workers healthy, repeatedly, all afternoon — each death looking like an unrelated "the loop stopped" until the traceback was read. Same class as the POSIX-only SIGUSR1 handler fixed earlier: an OPTIONAL feature ending the service. Two guards, because the resolver and the spawn can fail independently: - _claude_executable() prefers PATH, then falls back to the binary bundled with claude_agent_sdk (the one the workers already use). None => maintenance is SKIPPED with the reason in the outcome, not raised. - FileNotFoundError/OSError around the spawn itself is caught and degraded, for the case where the resolver succeeds and the exec still fails (deleted, not executable, bad permissions). Tests assert both degradations AND that the fallback resolver can actually find the bundled binary — without that third test the first two would pass against a resolver that always returns None (NEV-CTL-04). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.