From 7f2048c96980d49bbbdea634c7e92fe112906c06 Mon Sep 17 00:00:00 2001 From: Hexalgo Date: Thu, 6 Aug 2026 13:55:52 +0200 Subject: [PATCH 1/8] =?UTF-8?q?critic:=20add=20block=5Fon=5Fspec=20?= =?UTF-8?q?=E2=80=94=20a=20verdict=20for=20"the=20ISSUE=20is=20wrong",=20s?= =?UTF-8?q?o=20unsatisfiable=20ACs=20stop=20burning=20rounds?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- src/forge_loop/briefs/critic.md.tmpl | 336 +++++++++++++++------------ src/forge_loop/critic.py | 64 ++++- src/forge_loop/runner/critic_flow.py | 38 +++ tests/test_critic_block_on_spec.py | 146 ++++++++++++ 4 files changed, 436 insertions(+), 148 deletions(-) create mode 100644 tests/test_critic_block_on_spec.py diff --git a/src/forge_loop/briefs/critic.md.tmpl b/src/forge_loop/briefs/critic.md.tmpl index 2a5b766..26a7fa2 100644 --- a/src/forge_loop/briefs/critic.md.tmpl +++ b/src/forge_loop/briefs/critic.md.tmpl @@ -1,158 +1,197 @@ You are the CRITIC agent in a Titan sprint loop. A worker just opened a PR. -Your job: review it AND TEACH THE WORKER HOW TO CONVERGE — emit a structured -JSON CriticReport whose blocking set is the MINIMAL, ORDERED path to merge. +Your job: decide whether this PR can merge, and if not, produce the SHORTEST +TRUE PATH to a merge — which is sometimes a change to the DIFF and sometimes a +change to the ISSUE. Emit a structured JSON CriticReport. PR URL: {pr_url} Linked issue: #{issue_number} Critic round for this PR: {round_number} prior review(s) so far. -ROUND-AWARE BEHAVIOUR (read this before you start — it changes HOW you write -findings; it NEVER changes WHAT counts as a defect): -{round_guidance} +WHAT A ROUND COSTS (you must weigh this; it is not free): +Each additional round costs roughly 20-45 minutes of one of only a handful of +worker slots, plus a full re-review. A round is justified when it will plausibly +END the cycle. A round that repeats a previous round's blocking finding without +new information has, empirically, never converged — it is the single largest +source of wasted capacity in this system. Blocking is a real cost, not a free +default. Approving a PR that meets its acceptance criteria is a SUCCESS, not a +lapse in vigilance. -YOUR JOB IS TO TEACH, NOT JUST FILTER. A worker reads your findings and tries -to converge in a bounded number of rounds. A finding that says only "this is -wrong" forces the worker to re-discover the fix by trial and error, which burns -rounds and introduces new defects. So: state the SMALLEST COMPLETE change that -would make the PR mergeable, and separate it cleanly from optional polish. Do -NOT lower the bar to make convergence easier — teach the path instead. +ROUND-AWARE BEHAVIOUR (changes HOW you write findings, never WHAT is a defect): +{round_guidance} MANIFESTOS (canonical project rules — violations are first-class findings): {manifestos} END MANIFESTOS -DO: -1. Read the issue via `gh issue view {issue_number} --comments` to learn the - acceptance criteria. Note any "Acceptance" or "Out of scope" sections. -2. Read the PR diff: `gh pr diff {pr_url}`. -3. Read the PR description: `gh pr view {pr_url} --json title,body,additions,deletions`. -4. Check worker commit commands / PR text for `git commit --no-verify`. - If `--no-verify` appears and the PR body lacks a non-empty - `## Pre-commit bypass justification` section, emit a sev1 finding - tagged `precommit_bypass`. Pre-commit gates are not optional. -5. Decide overall + per-finding. Use the rubric: - - sev1 = correctness/security bug, missing acceptance criterion, - or test that doesn't actually exercise the change. ALWAYS blocks merge. - - sev2 = meaningful concern (untested error path, weak assertion, - scope creep affecting reviewers). A real defect — ALWAYS worth fixing - before merge. - - sev3 = nit / cosmetic suggestion. Non-blocking, and DEMOTABLE to a - follow-up once the PR has stalled (see SEVERITY TRIAGE below). +=============================================================================== +STEP 0 — IS THE SPECIFICATION SATISFIABLE? DO THIS BEFORE READING THE DIFF. +=============================================================================== + +Read the issue FIRST: `gh issue view {issue_number} --comments`. Then, before +you look at a single line of the diff, test each acceptance criterion against +the three questions below. This ordering is deliberate: a defective criterion +makes the entire diff review moot, and diagnosing it AFTER writing ten findings +means the findings were wasted work. + + Q1. CAN ONE PR SATISFY IT? A criterion that requires a code change AND a + separate live/data-collection grind cannot be closed by any single PR. No + amount of worker effort fixes that — the issue must be split. + + Q2. CAN THE DELIVERABLE DECIDE IT? A criterion is undecidable when the thing + being built structurally cannot evaluate it — e.g. a MECHANICAL analyser + graded on whether a human's stated rationale is "adequate", or a static + check graded on runtime behaviour it cannot observe. The worker can build + the right thing and still never satisfy it. + + Q3. DOES IT DEMAND DESTROYING EARNED WORK? A criterion that requires deleting, + blanking or reverting a value that was obtained from an IRREVERSIBLE or + EXPENSIVE act — a live run, a measurement, a manual walk, a paid API call + — is harmful, not tidy. Satisfying it bills the cost again to re-derive + something already known. + +If ANY criterion fails ANY of these, STOP. Do not review the diff for defects. +Emit `overall = "block_on_spec"` with the offending criterion in `spec_defects`, +and leave `findings` EMPTY unless the diff has an independent defect that would +block even under a corrected spec. + +☠ `block_on_spec` IS NOT A SOFTENED `request_changes`. It is a different + ADDRESSEE. `request_changes` tells the WORKER to change the diff. + `block_on_spec` tells the HUMAN/PO to change the ISSUE, and explicitly tells + the worker NOT to touch the diff for that reason. Sending a spec defect to + the worker as `request_changes` is the failure this whole section exists to + prevent: the worker cannot fix an issue it is not allowed to edit, so it + "responds" by adding scaffolding, and the cycle repeats until someone + intervenes by hand. + +=============================================================================== +STEP 1 — THE DIFF +=============================================================================== + +1. Read the PR diff: `gh pr diff {pr_url}`. +2. Read the PR description: `gh pr view {pr_url} --json title,body,additions,deletions`. +3. Check for `git commit --no-verify`. If it appears and the PR body lacks a + non-empty `## Pre-commit bypass justification`, emit a sev1 tagged + `precommit_bypass`. Pre-commit gates are not optional. +4. Rate each finding: + - sev1 = correctness/security bug, a genuinely missing acceptance criterion + (one that PASSED Step 0), or a test that does not exercise the change. + - sev2 = meaningful concern: untested error path, weak assertion, scope creep. + - sev3 = nit. Non-blocking, and demotable once the PR has stalled. Categories: correctness | security | style | tests | docs | product | - performance | architecture. - -SEVERITY TRIAGE — NEVER LOWER THE BAR (Ch9 §9.5.2): - sev1 and sev2 are real defects and ALWAYS block, no matter how many rounds - this PR has taken. A gate that tires teaches the system that persistence - beats quality. The only legitimate easing is to stop grinding on COSMETICS: - once this PR has stalled (the round guidance above tells you when), put sev3 - nits in ``follow_ups`` instead of ``findings`` so they do not block. Demoting - a sev1/sev2 is forbidden — if you are tempted, it was mis-rated; re-rate it. - -FAILURE-MODE DIAGNOSIS — fix the cause, not the symptom (Ch9 §9.5.3): - Look at the diff shape and the round history before writing findings: - - LARGE PURE-ADDITION diff (e.g. +N/-0, hundreds of added lines, no edits to - existing code): this is the classic scope-inflation tell. The dead-code, - N+1, and over-abstraction findings you are about to write are SYMPTOMS of - over-building. Emit ONE sev2 architecture finding that names the meta-cause - ("you are adding, not editing — cut scope to the minimal wiring that - satisfies the acceptance criteria; delete the speculative surface") and let - it subsume the symptom findings rather than listing ten of them. - - SAME CLASS OF FINDING RECURRING across rounds (the worker keeps - re-introducing the same defect): the approach is wrong. Say so, and propose - the simpler approach or instruct the worker to SPLIT the ticket — do not - just re-flag the symptom a third time. - -ARCHITECTURE & PERFORMANCE REVIEW (anti-slop — catch code that compiles and -passes tests but rots the internals; this is the hardest slop to detect, so -look ON PURPOSE): - Beyond "does it work", judge whether the change is WELL-BUILT. The diff - passing its tests is NOT sufficient. Emit findings with - category="architecture" or category="performance" for: - - REUSE / DON'T-REINVENT (category="architecture"): - - Hand-rolls something the standard library or an EXISTING project - dependency already does well — e.g. a retry/backoff loop, an LRU, datetime - parsing, a JSON walk, a glob, an env loader. Name the lib/function to use. - - Duplicates an EXISTING internal helper instead of importing it. Before - blessing a new helper, check the repo for one that already exists - (`grep`/search for a similar name or shape); copy-paste of existing logic - is a finding. Prefer extending an existing module over forking a new one. - - Re-derives or re-fetches a value the codebase already computes or caches. - - PERFORMANCE / COMPLEXITY (category="performance"): - - O(n^2) or worse where O(n) is straightforward; a list scan where a - dict/set lookup is obvious. - - N+1 calls: a subprocess / `gh` API / DB query INSIDE a loop that could be - one batched call or hoisted out of the loop. - - Redundant I/O on a hot path: re-reading or re-parsing the same file, - config, or query per-tick / per-item instead of reading once. - - Unbounded growth with no rotation/cap; a frequent DB query with no index. - - ARCHITECTURE (category="architecture"): - - Over-abstraction / premature generalization: indirection, config knobs, or - a "framework" with a single caller (distinct from scaffold-theatre below: - this is needless complexity, not an orphan plug-in). - - Wrong layer: business logic in a CLI handler, I/O in a "pure" helper, - reaching across a module boundary the design forbids. - - Severity: default sev2 (a real internal-quality concern worth fixing before - merge). Use sev1 only when it causes a correctness or scaling failure in - normal operation; sev3 for marginal cases. "It works on the happy path" does - NOT excuse reinvention or an N+1 — name it with the concrete fix. - -NO-SCAFFOLD-THEATRE RULE (issue #39): - A PR that adds a configurable backend, an integration adapter, a - dashboard / metrics exporter, or any other "plug-in" surface WITHOUT - a downstream consumer wired up in the same repo on the same PR is - scaffold theatre. Emit a sev2 finding with category="product" naming - the orphan code; the operator must either wire it up or quarantine - it behind the [experimental] extra before merge. Examples: - * adding a redis/postgres queue backend with no operator using it - * adding a Slack adapter with no event in the runner that sends to it - * adding a Prometheus exporter no scrape job consumes - Default surface stays minimal; experiments live in extras. - -DO NOT: -- push code or edit files. -- comment on formatting (the formatter does that). -- post review comments yourself — the runner does that from your report. - -MANIFESTO COMPLIANCE CHECK: - For every manifesto above, scan the PR diff for rule violations and - emit one entry in ``manifesto_violations`` per violation. Each entry: - - rule_id: the rule's identifier as it appears in the manifesto - (e.g. ``EH-001``). - - manifesto: the manifesto filename (e.g. ``error-handling.md``). - - quote: the offending snippet, verbatim, from the PR diff. - - suggested_fix: a concrete, minimal change that would resolve it. - - severity: sev1 | sev2 | sev3 — use the severity declared in the - manifesto. Default to sev3 only if the manifesto is silent. - ANY sev1 manifesto violation forces ``overall = "request_changes"`` - regardless of other findings — manifesto compliance is a hard gate. - -FINAL OUTPUT (one JSON line, no prose after it, no markdown fence): -{{"overall": "approve|request_changes|block", + performance | architecture | spec. + +=============================================================================== +STEP 2 — THE CARRY-OVER TEST (this is what breaks stuck cycles) +=============================================================================== + +For EVERY blocking finding you are about to carry over from a previous round — +i.e. one you already raised and the worker did not clear — you MUST classify it +as exactly one of these, and say which in the message: + + (a) WORKER-CLEARABLE, NOT YET CLEARED. The worker can fix it inside this PR + and simply has not. Keep it blocking, and escalate specificity: what, why, + how, and a concrete minimal patch sketch naming file and function. + + (b) SPEC-DEFECTIVE. Re-run Step 0 on it. If it fails Q1/Q2/Q3, it is NOT a + worker failure and must MOVE OUT of `findings` into `spec_defects`, and + `overall` becomes `block_on_spec`. This is not demotion and it is not + lowering the bar — it is correcting the ADDRESSEE of a finding that was + mis-routed. A finding aimed at the wrong party is simply wrong. + + (c) ENVIRONMENTALLY BLOCKED. Neither the diff nor the issue is wrong; the + worker cannot execute (no credentials, no device, a dependency outage). + Put it in `spec_defects` with `kind: "environment"` and say what access is + missing. ☠ Verify this claim rather than accepting it — an + "environment-blocked" excuse that has since been fixed is how work stays + theoretical long after the blocker is gone. + +☠ A blocking finding repeated VERBATIM for a THIRD round without being + re-classified by this test is itself a defect in the review. If you find + yourself writing the same must-fix a third time, the answer is (b) or (c) — + not a better-worded (a). + +=============================================================================== +STEP 3 — QUALITY REVIEW (unchanged in substance; still mandatory) +=============================================================================== + +FAILURE-MODE DIAGNOSIS — fix the cause, not the symptom: + - LARGE PURE-ADDITION diff (+N/-0, hundreds of added lines, no edits to + existing code): the classic scope-inflation tell. Emit ONE sev2 + architecture finding naming the meta-cause ("you are adding, not editing — + cut to the minimal wiring that satisfies the AC") and let it subsume the + symptoms rather than listing ten of them. + - ☠ ADDING SCAFFOLDING IN RESPONSE TO A BLOCK is the signature of a + mis-routed finding. If the worker answered your last round with a test + harness, an atomic-write guarantee, or a methodology writeup while the + number the issue actually asked for did not move, that is evidence for + Step 2 (b) — the worker is trying to satisfy something unsatisfiable. + +REUSE / DON'T-REINVENT (architecture): hand-rolling what the stdlib or an +existing dependency does (retry/backoff, LRU, datetime parsing, glob, env +loader); duplicating an existing internal helper instead of importing it; +re-deriving a value the codebase already computes. Name the function to use. + +PERFORMANCE: O(n^2) where O(n) is straightforward; N+1 subprocess/API/DB calls +inside a loop; redundant I/O on a hot path; unbounded growth with no cap. + +ARCHITECTURE: over-abstraction with a single caller; wrong layer (business logic +in a CLI handler, I/O in a "pure" helper). + +NO-SCAFFOLD-THEATRE: a configurable backend, adapter, or exporter with NO +downstream consumer wired up in the same PR is scaffold theatre — sev2, +category="product", name the orphan code. + +Severity for this section: default sev2; sev1 only when it causes a correctness +or scaling failure in normal operation. + +MANIFESTO COMPLIANCE: for every manifesto above, scan the diff and emit one +`manifesto_violations` entry per violation (rule_id, manifesto, verbatim quote, +suggested_fix, severity). ANY sev1 manifesto violation forces +`overall = "request_changes"`. + +DO NOT: push code, edit files, comment on formatting, or post review comments +yourself — the runner posts from your report. + +=============================================================================== +SEVERITY TRIAGE — THE BAR DOES NOT MOVE; THE ADDRESSEE MIGHT +=============================================================================== +sev1/sev2 are real defects and always block. A gate that tires teaches the +system that persistence beats quality, so DEMOTING a sev1/sev2 to a nit is +forbidden. Once stalled, move sev3 nits to `follow_ups`. + +Re-ROUTING is different from demoting and is REQUIRED when Step 2 says so: a +spec defect moved to `spec_defects` still blocks the merge — it just blocks the +ISSUE instead of the DIFF, and asks a human rather than the worker. The bar is +identical; only the addressee changes. + +=============================================================================== +FINAL OUTPUT (one JSON line, no prose after it, no markdown fence) +=============================================================================== +{{"overall": "approve|request_changes|block|block_on_spec", "minimal_path_to_green": [ - "ordered, minimal must-fix steps — the EXACT set that, once done, makes this PR mergeable; one string per blocking finding, in the order the worker should tackle them. Empty list iff overall == approve." + "ordered, minimal must-fix steps — the EXACT set that makes this PR mergeable. One string per blocking finding. Empty iff overall == approve. When overall == block_on_spec, these are steps for the HUMAN (split the issue / rescope criterion N / drop criterion M), NOT for the worker." + ], + "spec_defects": [ + {{"kind": "unsatisfiable_in_one_pr|undecidable_by_deliverable|destroys_earned_work|environment", + "criterion": "the acceptance criterion, quoted verbatim from the issue", + "why": "which of Q1/Q2/Q3 it fails and why no diff can satisfy it", + "fix": "the concrete change to the ISSUE — split into two tickets, rescope to X, drop it, or grant access Y", + "rounds_burned": 0}} ], "findings": [ {{"severity": "sev1|sev2|sev3", - "category": "correctness|security|style|tests|docs|product|performance|architecture", + "category": "correctness|security|style|tests|docs|product|performance|architecture|spec", + "carry_over": "new|worker_clearable|spec_defective|environment", "file": "path/to/file" or null, "line": 42 or null, - "message": "what's wrong and what to do (on round >=2, escalate to why + how + a minimal patch sketch naming the file/function)"}} + "message": "what is wrong and what to do (round >=2: why + how + a minimal patch sketch naming file/function)"}} ], "follow_ups": [ - {{"severity": "sev3", - "category": "style|docs|...", - "file": "path/to/file" or null, - "line": null, - "message": "optional polish / demoted nit that does NOT block merge"}} + {{"severity": "sev3", "category": "style|docs|...", "file": null, "line": null, + "message": "polish that does NOT block merge"}} ], "manifesto_violations": [ - {{"rule_id": "EH-001", - "manifesto": "error-handling.md", + {{"rule_id": "EH-001", "manifesto": "error-handling.md", "quote": "except Exception: pass", "suggested_fix": "catch the specific exception and log it", "severity": "sev1|sev2|sev3"}} @@ -160,18 +199,21 @@ FINAL OUTPUT (one JSON line, no prose after it, no markdown fence): "issue": {issue_number}}} Hard rules: -- ``minimal_path_to_green`` is MANDATORY and must be the LAST thing you decide: - the ordered, minimal must-fix set the worker has to clear to merge — the - acceptance predicate, stated explicitly so the worker never has to guess it. - It must cover every BLOCKING (sev1/sev2) finding and nothing else (cosmetics - belong in ``follow_ups``, not here). When ``overall == "approve"`` it MUST be - an empty list. -- ``findings`` carries the blocking set; ``follow_ups`` carries non-blocking - polish (including sev3 nits you demoted because the PR has stalled). Never put - a sev1/sev2 in ``follow_ups``. -- "approve" with an empty findings list on a large diff is a red flag — - if you can't find anything, emit "request_changes" with at least one - sev3 noting what you reviewed. -- If genuinely uncertain about a finding, lean toward emitting it as sev3 - rather than swallowing it. -- The JSON object MUST be on the LAST line of your output and parse cleanly. +- `minimal_path_to_green` is MANDATORY and decided LAST: the ordered minimal set + that clears the merge. Empty iff `overall == "approve"`. Under + `block_on_spec` it is addressed to the human, not the worker. +- `spec_defects` is non-empty IFF `overall == "block_on_spec"`. Never put a spec + defect in `findings` — that is the mis-routing this template exists to stop. +- Every carried-over blocking finding MUST set `carry_over` to the Step 2 class. + `new` is only valid on a finding raised for the first time. +- Never put a sev1/sev2 in `follow_ups`. +- ☠ Never require deleting a value obtained from a live run, a measurement, or + any irreversible act in order to make a diff look scope-pure. If it is + genuinely out of scope, say it ships as-is and the issue wording should be + narrowed — that is a `spec_defects` entry, not a must-fix. +- APPROVE IS A REAL OUTCOME. If the diff satisfies the acceptance criteria and + the manifestos, approve it. Do not manufacture a finding to look diligent; do + not block on a preference. If your only remaining items are sev3, approve and + put them in `follow_ups`. +- If genuinely uncertain about a DEFECT, lean toward emitting it as sev3. +- The JSON object MUST be the LAST line of your output and parse cleanly. diff --git a/src/forge_loop/critic.py b/src/forge_loop/critic.py index 7ba9fc0..441ffa1 100644 --- a/src/forge_loop/critic.py +++ b/src/forge_loop/critic.py @@ -54,7 +54,13 @@ # Shared by parse-failure retries and transient-SDK-error retries. _CRITIC_ATTEMPTS = 2 -VALID_OVERALL = {"approve", "request_changes", "block"} +# ``block_on_spec`` 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/PO to change +# the ISSUE, and tells the worker to leave the diff alone. Without it the critic is required to +# emit sev1 for a "missing acceptance criterion" and forbidden to demote it, so an UNSATISFIABLE +# criterion blocks forever: the worker cannot edit the issue, so it answers with more code and +# the cycle repeats. Measured on a live repo: 5 repair passes on one PR, ~2h with zero merges. +VALID_OVERALL = {"approve", "request_changes", "block", "block_on_spec"} # The severity / category vocabulary lives in ``critic_format`` (the single # source of truth shared with the gh_issues thread classifier — #230). We alias # it here so existing call sites keep using ``VALID_SEVERITY`` / ``VALID_CATEGORY`` @@ -184,6 +190,27 @@ def deserialize_findings(rows: Any) -> list[Finding]: return out +@dataclass +class SpecDefect: + """A defect in the ISSUE that no diff can satisfy. Addressed to a human, not the worker.""" + + kind: str + criterion: str = "" + why: str = "" + fix: str = "" + rounds_burned: int = 0 + + VALID_KINDS = ( + "unsatisfiable_in_one_pr", + "undecidable_by_deliverable", + "destroys_earned_work", + "environment", + ) + + def is_valid(self) -> bool: + return self.kind in self.VALID_KINDS and bool(self.criterion.strip()) + + @dataclass class CriticReport: overall: str # approve | request_changes | block @@ -200,6 +227,12 @@ class CriticReport: minimal_path_to_green: list[str] = field(default_factory=list) follow_ups: list[Finding] = field(default_factory=list) round_number: int = 0 + # Non-empty IFF overall == "block_on_spec". Never carries diff defects. + spec_defects: list[SpecDefect] = field(default_factory=list) + + def blocks_on_spec(self) -> bool: + """True when the ISSUE must change, not the diff — do NOT dispatch a repair worker.""" + return self.overall == "block_on_spec" def severities(self) -> set[str]: return {f.severity for f in self.findings} @@ -1125,6 +1158,34 @@ def _coerce_report(obj: dict[str, Any], raw: str) -> CriticReport | None: if f.is_valid(): follow_ups.append(f) + # ☠ PARSE THE SPEC DEFECTS. Without this the new verdict arrives with an empty payload and the + # runner cannot tell a human WHICH criterion is broken — which is the entire point of it. + spec_defects: list[SpecDefect] = [] + for item in obj.get("spec_defects") or []: + if not isinstance(item, dict): + continue + rounds = item.get("rounds_burned", 0) + if isinstance(rounds, str) and rounds.isdigit(): + rounds = int(rounds) + elif not isinstance(rounds, int): + rounds = 0 + sd = SpecDefect( + kind=str(item.get("kind", "")), + criterion=str(item.get("criterion", "")), + why=str(item.get("why", "")), + fix=str(item.get("fix", "")), + rounds_burned=rounds, + ) + if sd.is_valid(): + spec_defects.append(sd) + + # The verdict and its payload must agree or the runner routes to the wrong actor. Degrade + # rather than raise: an unusable block_on_spec is just an ordinary request_changes. + if overall == "block_on_spec" and not spec_defects: + overall = "request_changes" + elif overall != "block_on_spec" and spec_defects: + spec_defects = [] + return CriticReport( overall=overall, findings=findings, @@ -1132,6 +1193,7 @@ def _coerce_report(obj: dict[str, Any], raw: str) -> CriticReport | None: raw=raw, minimal_path_to_green=minimal_path_to_green, follow_ups=follow_ups, + spec_defects=spec_defects, ) diff --git a/src/forge_loop/runner/critic_flow.py b/src/forge_loop/runner/critic_flow.py index 3266a38..2d1c405 100644 --- a/src/forge_loop/runner/critic_flow.py +++ b/src/forge_loop/runner/critic_flow.py @@ -182,6 +182,44 @@ def handle_critic_verdict( ) return "abandoned" + if overall == "block_on_spec": + # ☠ THE DEFECT IS IN THE ISSUE, NOT THE DIFF — so do NOT dispatch a revision. This is the + # branch that actually saves the wasted rounds: under the old three-verdict model an + # unsatisfiable acceptance criterion came back as request_changes, a worker was dispatched + # against something no diff can satisfy, it answered with more code, and the cycle repeated + # until a human noticed. Park the session for a human and say WHICH criterion is broken. + store.transition_to( + session_id, WorkerState.ABANDONED, reason="critic blocked on spec (issue defect)" + ) + defects = list(getattr(report, "spec_defects", []) or []) + if pr_url: + _label_pr_best_effort( + gh, + pr_url, + NEEDS_REVIEW_LABEL, + repo=repo, + emit=emit, + event="critic_block_label_failed", + issue=sess.issue, + ) + _emit_best_effort( + emit, + "critic_verdict_blocked_on_spec", + issue=sess.issue, + session_id=session_id, + pr=pr_url, + defects=[ + { + "kind": getattr(d, "kind", ""), + "criterion": str(getattr(d, "criterion", ""))[:300], + "fix": str(getattr(d, "fix", ""))[:300], + "rounds_burned": getattr(d, "rounds_burned", 0), + } + for d in defects + ], + ) + return "blocked_on_spec" + if overall == "request_changes": new_count = store.increment_iterations(session_id) store.transition_to(session_id, WorkerState.REVISING, reason="critic requested changes") diff --git a/tests/test_critic_block_on_spec.py b/tests/test_critic_block_on_spec.py new file mode 100644 index 0000000..71e8871 --- /dev/null +++ b/tests/test_critic_block_on_spec.py @@ -0,0 +1,146 @@ +"""block_on_spec: when the ISSUE is the defect, do NOT dispatch a repair worker. + +☠ THE BUG THIS LOCKS DOWN. The critic's rubric makes "missing acceptance criterion" a sev1 that +always blocks, and triage forbids demoting sev1/sev2. The only escape valve demotes COSMETICS. So an +UNSATISFIABLE criterion blocked forever: the worker cannot edit the issue, so it answered with more +code, and the cycle repeated. Measured on a live repo: FIVE repair passes on one PR and ~2h with zero +merges, while the critic itself had already written the correct diagnosis ("escalate to a human to +split the issue") into prose it had nowhere to put. + +The load-bearing assertion here is the NEGATIVE one: `dispatch_revision` is never called. Everything +else is bookkeeping. +""" + +from __future__ import annotations + +from typing import Any + +from forge_loop.critic import CriticReport, SpecDefect +from forge_loop.runner.critic_flow import handle_critic_verdict +from forge_loop.worker_sessions import WorkerSessionStore, WorkerState + + +class _StubGh: + def __call__(self, *a: Any, **k: Any) -> Any: + return None + + def __getattr__(self, _name: str) -> Any: + return lambda *a, **k: None + + +def _events() -> tuple[list[tuple[str, dict]], Any]: + seen: list[tuple[str, dict]] = [] + + def emit(kind: str, **kw: Any) -> None: + seen.append((kind, kw)) + + return seen, emit + + +def _seed_awaiting(store: WorkerSessionStore) -> str: + sess = store.create(issue=160, branch="loop/160", worktree_path="/tmp/wt-loop-160") + store.transition_to(sess.session_id, WorkerState.RUNNING) + store.transition_to(sess.session_id, WorkerState.AWAITING_CRITIC, reason="pr opened") + store.set_pr_url(sess.session_id, "https://github.com/o/r/pull/161") + return sess.session_id + + +def _spec_report() -> CriticReport: + return CriticReport( + overall="block_on_spec", + findings=[], + spec_defects=[ + SpecDefect( + kind="unsatisfiable_in_one_pr", + criterion=">=8 of the 36 PLACE rows carry a wire-observed mapId", + why="requires a code round AND a live data-collection grind; no single PR closes both", + fix="split into a mechanism ticket and a grind ticket", + rounds_burned=5, + ) + ], + ) + + +def test_block_on_spec_does_not_dispatch_a_revision() -> None: + """The whole point: no worker is sent at a criterion no diff can satisfy.""" + store = WorkerSessionStore(":memory:") + sid = _seed_awaiting(store) + events, emit = _events() + dispatched: list[dict] = [] + + result = handle_critic_verdict( + store=store, + session_id=sid, + report=_spec_report(), + pr_url="https://github.com/o/r/pull/161", + gh=_StubGh(), + emit=emit, + dispatch_revision=lambda **kw: dispatched.append(kw), + ) + + assert result == "blocked_on_spec" + assert dispatched == [], "a spec defect must NEVER dispatch a repair worker" + + sess = store.get(sid) + assert sess is not None + assert sess.state == WorkerState.ABANDONED + # Iterations must not be burned on a round the worker could never win. + assert sess.critic_iterations == 0 + + +def test_block_on_spec_reports_which_criterion_is_broken() -> None: + """A human is the addressee, so the event must name the criterion and the fix.""" + store = WorkerSessionStore(":memory:") + sid = _seed_awaiting(store) + events, emit = _events() + + handle_critic_verdict( + store=store, + session_id=sid, + report=_spec_report(), + pr_url="https://github.com/o/r/pull/161", + gh=_StubGh(), + emit=emit, + ) + + payloads = [p for k, p in events if k == "critic_verdict_blocked_on_spec"] + assert payloads, "the spec block must be observable as its own typed event" + defects = payloads[0]["defects"] + assert defects[0]["kind"] == "unsatisfiable_in_one_pr" + assert "wire-observed mapId" in defects[0]["criterion"] + assert defects[0]["rounds_burned"] == 5 + + +def test_request_changes_still_dispatches() -> None: + """NEV-CTL-04: prove the negative assertion above can fail — the same harness + with an ordinary verdict MUST dispatch, or the first test proves nothing.""" + from forge_loop.critic import Finding + + store = WorkerSessionStore(":memory:") + sid = _seed_awaiting(store) + _, emit = _events() + dispatched: list[dict] = [] + + result = handle_critic_verdict( + store=store, + session_id=sid, + report=CriticReport( + overall="request_changes", + findings=[ + Finding( + severity="sev1", + category="correctness", + file="a.py", + line=1, + message="off-by-one", + ) + ], + ), + pr_url="https://github.com/o/r/pull/161", + gh=_StubGh(), + emit=emit, + dispatch_revision=lambda **kw: dispatched.append(kw), + ) + + assert result == "revising" + assert len(dispatched) == 1, "the harness can observe a dispatch, so the negative test is real" From 4ac19f52c098b048e1b64700fd8e7201fbaafb5d Mon Sep 17 00:00:00 2001 From: Hexalgo Date: Thu, 6 Aug 2026 14:23:14 +0200 Subject: [PATCH 2/8] =?UTF-8?q?settings:=20accept=20the=20Claude=205=20fam?= =?UTF-8?q?ily=20=E2=80=94=20the=20model=20validator=20rejected=20the=20cu?= =?UTF-8?q?rrent=20generation?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit _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 --- src/forge_loop/settings.py | 9 +++++++- tests/test_model_alias_claude5.py | 38 +++++++++++++++++++++++++++++++ 2 files changed, 46 insertions(+), 1 deletion(-) create mode 100644 tests/test_model_alias_claude5.py diff --git a/src/forge_loop/settings.py b/src/forge_loop/settings.py index 7c3f25f..f92d287 100644 --- a/src/forge_loop/settings.py +++ b/src/forge_loop/settings.py @@ -35,7 +35,14 @@ from pydantic_settings import BaseSettings, SettingsConfigDict # Recognised Claude model aliases — same as the legacy loader. -_MODEL_PATTERN = re.compile(r"^claude-(opus|sonnet|haiku)-\d+-\d+(-[a-z0-9.-]+)?$") +# ☠ THE MINOR VERSION IS OPTIONAL, and the family list is not closed. The old pattern demanded +# ``claude---`` with BOTH numbers, so it could not express the Claude 5 +# family at all (claude-opus-5, claude-sonnet-5) and rejected a valid config at startup with +# "unknown model alias" — which reads as a typo rather than as a stale validator. A validator that +# refuses the current generation of the thing it validates is worse than no validator: it blocks +# the correct value and points the operator at the wrong file. +# Accepts: claude-opus-5, claude-sonnet-5, claude-opus-4-8, claude-haiku-4-5-20251001. +_MODEL_PATTERN = re.compile(r"^claude-(opus|sonnet|haiku|fable)-\d+(-\d+)?(-[a-z0-9.-]+)?$") _CODEX_MODEL_PATTERN = re.compile(r"^[A-Za-z0-9][A-Za-z0-9._:-]*$") _AGENT_PROVIDERS = frozenset({"claude", "codex"}) _THINKING_VALUES = frozenset({"off", "low", "medium", "high"}) diff --git a/tests/test_model_alias_claude5.py b/tests/test_model_alias_claude5.py new file mode 100644 index 0000000..84c0f20 --- /dev/null +++ b/tests/test_model_alias_claude5.py @@ -0,0 +1,38 @@ +"""The model validator must accept the CURRENT generation of model names. + +☠ It did not. `_MODEL_PATTERN` demanded ``claude---`` with BOTH numbers, so it +structurally could not express the Claude 5 family (claude-opus-5, claude-sonnet-5) and refused a +valid config at startup with "unknown model alias" — wording that reads as a typo and sends the +operator to edit the wrong file. A validator that rejects the current generation of the thing it +validates is worse than none: it blocks the correct value. +""" + +from __future__ import annotations + +import pytest + +from forge_loop.settings import _MODEL_PATTERN + + +@pytest.mark.parametrize( + "model", + [ + "claude-opus-5", + "claude-sonnet-5", + "claude-fable-5", + "claude-haiku-4-5-20251001", + "claude-opus-4-8", # the older two-number shape still works + "claude-sonnet-4-6", + ], +) +def test_accepts_real_model_ids(model: str) -> None: + assert _MODEL_PATTERN.match(model), f"{model} is a real model id and must validate" + + +@pytest.mark.parametrize( + "model", + ["claude-5", "opus-5", "gpt-4", "claude-turbo-5", "", "claude-opus-"], +) +def test_still_rejects_nonsense(model: str) -> None: + """NEV-CTL-04: prove the pattern can still FAIL, or the accept test proves nothing.""" + assert not _MODEL_PATTERN.match(model), f"{model} must not validate" From 8373370400c9d5b78f725783cd7111db4a54a8de Mon Sep 17 00:00:00 2001 From: Hexalgo Date: Thu, 6 Aug 2026 14:25:38 +0200 Subject: [PATCH 3/8] =?UTF-8?q?boot:=20guard=20SIGUSR1=20=E2=80=94=20it=20?= =?UTF-8?q?is=20POSIX-only=20and=20took=20the=20whole=20runner=20down=20on?= =?UTF-8?q?=20Windows?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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 --- src/forge_loop/runner/boot.py | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/forge_loop/runner/boot.py b/src/forge_loop/runner/boot.py index 10915d8..8cdd4de 100644 --- a/src/forge_loop/runner/boot.py +++ b/src/forge_loop/runner/boot.py @@ -237,7 +237,12 @@ def _pause_toggle(*_: Any) -> None: signal.signal(signal.SIGTERM, _stop) signal.signal(signal.SIGINT, _stop) - signal.signal(signal.SIGUSR1, _pause_toggle) + # ☠ SIGUSR1 IS POSIX-ONLY. On Windows `signal.SIGUSR1` does not exist, so this raised + # AttributeError during boot and the runner could not start AT ALL — a pause CONVENIENCE taking + # down the whole loop on an entire platform. Pause/resume still works there: `_short_sleep` + # already polls `cfg.pause_file`, which is the touchfile this handler merely toggles. + if hasattr(signal, "SIGUSR1"): + signal.signal(signal.SIGUSR1, _pause_toggle) def _short_sleep(seconds: int, cfg: Config, state: RunnerState | None = None) -> None: From 570030339e919573ac1e6c0aabecb07070468695 Mon Sep 17 00:00:00 2001 From: Hexalgo Date: Thu, 6 Aug 2026 14:40:37 +0200 Subject: [PATCH 4/8] worktree: the coordinator brings the base forward before every repair round MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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/, check rev-list HEAD..origin/, 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 --- src/forge_loop/runner/dispatch.py | 3 + src/forge_loop/worker.py | 4 ++ src/forge_loop/worker_worktree.py | 76 ++++++++++++++++++++++ tests/test_repair_base_sync.py | 103 ++++++++++++++++++++++++++++++ 4 files changed, 186 insertions(+) create mode 100644 tests/test_repair_base_sync.py diff --git a/src/forge_loop/runner/dispatch.py b/src/forge_loop/runner/dispatch.py index cc33e60..417767e 100644 --- a/src/forge_loop/runner/dispatch.py +++ b/src/forge_loop/runner/dispatch.py @@ -873,6 +873,9 @@ def _run_repair_workers( cfg.repo, cfg.logs_dir, cfg.worker_timeout_s, + # The repair worktree merges this forward before the round runs — a 5th-round + # worker must not still be sitting on the base its branch was cut from. + base_branch=cfg.base_branch, emit=bus_emit, lumen_top_k=cfg.lumen.top_k, lumen_test_pattern=cfg.lumen_test_pattern, diff --git a/src/forge_loop/worker.py b/src/forge_loop/worker.py index 3011619..0e1200c 100644 --- a/src/forge_loop/worker.py +++ b/src/forge_loop/worker.py @@ -581,6 +581,7 @@ def run_repair_worker( logs_dir: Path, timeout_s: int, *, + base_branch: str = "main", emit: Callable[[str, dict[str, Any]], None] | None = None, lumen_top_k: int = 3, lumen_test_pattern: str = "**/*Test.*", @@ -621,6 +622,9 @@ def run_repair_worker( repo, n, branch, + # Bring the PR branch forward onto the freshest base before the repair round runs, so a + # 5th-round worker is not still reasoning about the base its branch was cut from. + base_branch=base_branch, emit=emit, capability_policy=capability_policy, events_file=events_file, diff --git a/src/forge_loop/worker_worktree.py b/src/forge_loop/worker_worktree.py index 6fbff37..691b2f6 100644 --- a/src/forge_loop/worker_worktree.py +++ b/src/forge_loop/worker_worktree.py @@ -293,11 +293,73 @@ def prep_worktree( return wt, None + +def _sync_base_into_worktree( + repo: Path, + wt: Path, + base_branch: str, + branch: str, + issue: int, + emit: Callable[[str, dict[str, Any]], None] | None, +) -> None: + """Merge the freshest ``origin/`` into an existing PR worktree. + + Best-effort by design: a repair round on a stale base is worth running, a repair round on a + CONFLICTED tree is not. On conflict we abort and emit, leaving the worktree exactly as it was. + """ + + def _emit(kind: str, **kw: Any) -> None: + if emit is not None: + try: + emit(kind, {"issue": issue, "branch": branch, **kw}) + except Exception: # noqa: BLE001 — telemetry must never break dispatch + pass + + remote_ref = f"refs/remotes/origin/{base_branch}" + subprocess.run( + ["git", "fetch", "--prune", "origin", f"+refs/heads/{base_branch}:{remote_ref}"], + cwd=repo, + capture_output=True, + ) + + behind = subprocess.run( + ["git", "rev-list", "--count", f"HEAD..origin/{base_branch}"], + cwd=wt, + capture_output=True, + text=True, + ) + try: + n_behind = int((behind.stdout or "0").strip()) + except ValueError: + n_behind = 0 + if n_behind == 0: + return # already current — say nothing, this is the common case + + merged = subprocess.run( + ["git", "merge", "--no-edit", f"origin/{base_branch}"], + cwd=wt, + capture_output=True, + text=True, + ) + if merged.returncode == 0: + _emit("repair_base_synced", behind=n_behind, base=base_branch) + return + + subprocess.run(["git", "merge", "--abort"], cwd=wt, capture_output=True) + _emit( + "repair_base_sync_conflict", + behind=n_behind, + base=base_branch, + detail=(merged.stdout or merged.stderr or "")[-400:], + ) + + def prep_repair_worktree( repo: Path, issue: int, branch: str, *, + base_branch: str = "main", emit: Callable[[str, dict[str, Any]], None] | None = None, precommit_runner: PreCommitRunner | None = None, capability_policy: CapabilityPolicy | None = None, @@ -322,6 +384,20 @@ def prep_repair_worktree( ) if r.returncode != 0: return wt, r.stderr + + # ☠ BRING THE BASE FORWARD. Without this a repair round works on the base the branch was CUT + # from, however long ago and however much has merged since. PR #161 reached its fifth round still + # sitting on an 08:00 base. The worker then reasons about, and is reviewed against, a repo that no + # longer exists — and git only ever warns about TEXTUAL conflicts, never about two workers having + # independently "fixed" the same thing in incompatible ways. + # + # Merge, do not rebase: the branch is already published as a PR, so rebasing would need a + # force-push and would invalidate the review history the critic's round counting depends on. + # + # A CONFLICT IS REPORTED, NEVER SWALLOWED. We abort back to a clean tree and hand the worker a + # warning: a half-merged worktree is a far worse starting point than a stale one. + _sync_base_into_worktree(repo, wt, base_branch, branch, issue, emit) + if wt.exists(): plant_worker_settings(wt, capability_policy, events_file=events_file) _install_and_emit_worker_precommit_hook( diff --git a/tests/test_repair_base_sync.py b/tests/test_repair_base_sync.py new file mode 100644 index 0000000..e2c9522 --- /dev/null +++ b/tests/test_repair_base_sync.py @@ -0,0 +1,103 @@ +"""A repair round must run on the CURRENT base, not the base its branch was cut from. + +☠ THE BUG. prep_repair_worktree fetched the PR branch and never brought the base into it, so every +repair round worked against whatever main looked like when the branch was created. Measured: a PR +reached its FIFTH round still sitting on a base hours old, while other PRs had merged underneath it. +The worker reasons about — and the critic reviews against — a repo that no longer exists. Git only +ever warns about TEXTUAL conflicts; two workers independently "fixing" the same thing in incompatible +ways is silent. +""" + +from __future__ import annotations + +import subprocess +from pathlib import Path + +from forge_loop.worker_worktree import _sync_base_into_worktree + + +def _git(*args: str, cwd: Path) -> str: + r = subprocess.run(["git", *args], cwd=cwd, capture_output=True, text=True) + return r.stdout.strip() + + +def _seed_origin_and_clone(tmp_path: Path) -> tuple[Path, Path]: + origin = tmp_path / "origin" + origin.mkdir() + _git("init", "-q", "-b", "main", cwd=origin) + _git("config", "user.email", "t@t", cwd=origin) + _git("config", "user.name", "t", cwd=origin) + (origin / "base.txt").write_text("v1\n", encoding="utf-8") + _git("add", "-A", cwd=origin) + _git("commit", "-qm", "base v1", cwd=origin) + + clone = tmp_path / "clone" + subprocess.run(["git", "clone", "-q", str(origin), str(clone)], check=True) + _git("config", "user.email", "t@t", cwd=clone) + _git("config", "user.name", "t", cwd=clone) + return origin, clone + + +def test_stale_repair_branch_is_brought_forward(tmp_path: Path) -> None: + origin, clone = _seed_origin_and_clone(tmp_path) + + # A PR branch cut from base v1. + _git("checkout", "-qb", "loop/1", cwd=clone) + (clone / "feature.txt").write_text("work\n", encoding="utf-8") + _git("add", "-A", cwd=clone) + _git("commit", "-qm", "feature", cwd=clone) + + # main moves on in a file the branch does NOT touch. + _git("checkout", "-q", "main", cwd=origin) + (origin / "other.txt").write_text("landed later\n", encoding="utf-8") + _git("add", "-A", cwd=origin) + _git("commit", "-qm", "another PR merged", cwd=origin) + + _git("checkout", "-q", "loop/1", cwd=clone) + assert not (clone / "other.txt").exists(), "precondition: the branch is stale" + + events: list[tuple[str, dict]] = [] + _sync_base_into_worktree( + clone, clone, "main", "loop/1", 1, lambda k, p: events.append((k, p)) + ) + + assert (clone / "other.txt").exists(), "the later commit must be present after the sync" + assert (clone / "feature.txt").exists(), "the branch's own work must survive" + assert any(k == "repair_base_synced" for k, _ in events) + + +def test_conflict_aborts_and_reports_instead_of_leaving_a_half_merged_tree( + tmp_path: Path, +) -> None: + """A stale tree is a bad start; a CONFLICTED tree is a worse one.""" + origin, clone = _seed_origin_and_clone(tmp_path) + + _git("checkout", "-qb", "loop/2", cwd=clone) + (clone / "base.txt").write_text("branch edit\n", encoding="utf-8") + _git("add", "-A", cwd=clone) + _git("commit", "-qm", "branch edits base.txt", cwd=clone) + + # main edits the SAME line — a real conflict. + (origin / "base.txt").write_text("main edit\n", encoding="utf-8") + _git("add", "-A", cwd=origin) + _git("commit", "-qm", "main edits base.txt", cwd=origin) + + events: list[tuple[str, dict]] = [] + _sync_base_into_worktree( + clone, clone, "main", "loop/2", 2, lambda k, p: events.append((k, p)) + ) + + assert any(k == "repair_base_sync_conflict" for k, _ in events), "the conflict must be reported" + # The tree must be clean — no conflict markers, no MERGE_HEAD left behind. + assert not (clone / ".git" / "MERGE_HEAD").exists(), "the merge must have been aborted" + assert "branch edit" in (clone / "base.txt").read_text(encoding="utf-8") + + +def test_already_current_is_silent(tmp_path: Path) -> None: + """NEV-CTL-04: the emitter can fire (proved above), so silence here is meaningful.""" + origin, clone = _seed_origin_and_clone(tmp_path) + events: list[tuple[str, dict]] = [] + _sync_base_into_worktree( + clone, clone, "main", "main", 3, lambda k, p: events.append((k, p)) + ) + assert events == [], "a branch already on the base must not emit noise" From bc24b00dbdeed1dbca14c77842060dbb9e969b69 Mon Sep 17 00:00:00 2001 From: Hexalgo Date: Thu, 6 Aug 2026 17:21:08 +0200 Subject: [PATCH 5/8] =?UTF-8?q?repairs:=20cap=20repair=20rounds=20?= =?UTF-8?q?=E2=80=94=20one=20non-converging=20PR=20must=20not=20hold=20the?= =?UTF-8?q?=20whole=20loop?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- src/forge_loop/config.py | 8 ++++ src/forge_loop/runner/repairs.py | 34 ++++++++++++++++ src/forge_loop/settings.py | 1 + tests/test_repair_round_cap.py | 68 ++++++++++++++++++++++++++++++++ 4 files changed, 111 insertions(+) create mode 100644 tests/test_repair_round_cap.py diff --git a/src/forge_loop/config.py b/src/forge_loop/config.py index 54f7050..3f76089 100644 --- a/src/forge_loop/config.py +++ b/src/forge_loop/config.py @@ -91,6 +91,13 @@ class CriticConfig: # so rounds are not burned on nits. sev1/sev2 are NEVER demoted — this is # triage, not standard erosion. ``0`` disables demotion entirely. sev3_demotion_round_threshold: int = 3 + # ☠ HARD CONVERGENCE GUARANTEE. Repair ticks run their workers SYNCHRONOUSLY + # (dispatch.py collects fut.result() inside the executor), so ONE PR that never converges + # holds the whole loop for a worker_timeout_s at a time and no new issue is ever + # dispatched. Measured: two PRs consumed an entire day at 17-44 min per round while the + # backlog sat untouched. block_on_spec fixes the case where the critic RECOGNISES the + # issue is at fault; this cap covers the case where it does not. 0 disables. + max_repair_rounds: int = 4 @dataclass(frozen=True) @@ -328,6 +335,7 @@ def _from_settings(s: Settings) -> Config: thinking=s.critic.thinking, provider=s.critic.provider, sev3_demotion_round_threshold=s.critic.sev3_demotion_round_threshold, + max_repair_rounds=s.critic.max_repair_rounds, ), mutation_gate=MutationGateConfig( enabled=s.mutation_gate.enabled, diff --git a/src/forge_loop/runner/repairs.py b/src/forge_loop/runner/repairs.py index c3d23ee..c8e4c9d 100644 --- a/src/forge_loop/runner/repairs.py +++ b/src/forge_loop/runner/repairs.py @@ -6,14 +6,17 @@ from typing import Any from forge_loop.config import Config +from forge_loop.critic import count_prior_critic_rounds from forge_loop.gh_issues import ( CRITIC_BLOCK_LABELS, fetch_issue, + label, open_prs, pr_review_context, prs_by_label, prs_requiring_repair, ) +from forge_loop.runner.critic_flow import NEEDS_REVIEW_LABEL from forge_loop.state import append_event from forge_loop.worker import WorkerOutcome @@ -93,6 +96,37 @@ def _on_skip(pr: dict[str, Any]) -> None: reason="source_issue_not_found", ) continue + # ☠ ROUND CAP — the loop must CONVERGE, not grind. + # + # Repair ticks run their workers SYNCHRONOUSLY (dispatch collects fut.result() inside the + # executor), so a PR that never converges holds the WHOLE loop for a worker_timeout_s per + # round and no new issue is dispatched meanwhile. Measured: two PRs consumed an entire day + # at 17-44 min a round while the backlog sat untouched. + # + # `block_on_spec` covers the case where the critic RECOGNISES the issue is at fault. This cap + # covers the case where it does NOT — a wrong-but-confident sev1 repeated forever. After + # `max_repair_rounds` the PR stops being selected, is labelled for a human, and the loop moves + # on. The PR is NOT closed and the branch is NOT touched: the work stays intact and a human + # can resume it. Lost throughput is recoverable; lost work is not. + max_rounds = getattr(cfg.critic, "max_repair_rounds", 0) + if max_rounds: + rounds = count_prior_critic_rounds(issue_num, cfg.logs_dir) + if rounds >= max_rounds: + append_event( + cfg.events_file, + "repair_round_cap_reached", + pr=pr.get("url"), + issue=issue_num, + rounds=rounds, + cap=max_rounds, + reason="not converging — parked for a human so the loop can dispatch new work", + ) + try: + label(issue_num, [NEEDS_REVIEW_LABEL], repo=cfg.github_repo) + except Exception: # noqa: BLE001 — labelling must never break the tick + pass + continue + issue = fetch_issue_fn(issue_num, repo=cfg.github_repo) if not issue: append_event( diff --git a/src/forge_loop/settings.py b/src/forge_loop/settings.py index f92d287..dfbd791 100644 --- a/src/forge_loop/settings.py +++ b/src/forge_loop/settings.py @@ -182,6 +182,7 @@ class CriticSettings(BaseSettings): # Teaching-critic (Ch9): rounds after which sev3 nits are demoted to # non-blocking follow-ups. sev1/sev2 are never demoted. 0 disables. sev3_demotion_round_threshold: int = 3 + max_repair_rounds: int = 4 @field_validator("provider") @classmethod diff --git a/tests/test_repair_round_cap.py b/tests/test_repair_round_cap.py new file mode 100644 index 0000000..e26282c --- /dev/null +++ b/tests/test_repair_round_cap.py @@ -0,0 +1,68 @@ +"""A PR that never converges must stop consuming the loop. + +☠ WHY THIS EXISTS. Repair ticks run their workers SYNCHRONOUSLY — dispatch collects `fut.result()` +inside the ThreadPoolExecutor — so the tick sits inside a repair until the worker finishes, up to +worker_timeout_s. One PR stuck in review therefore holds the ENTIRE loop and no new issue is +dispatched. Measured on a live repo: two PRs consumed a whole day at 17-44 min a round while the +backlog sat untouched and the north-star number stayed unmeasured. + +`block_on_spec` handles the case where the critic RECOGNISES the issue is at fault. This cap handles +the case where it does not. +""" + +from __future__ import annotations + +from pathlib import Path +from typing import Any + +from forge_loop.runner.repairs import blocking_pr_repairs + + +class _Cfg: + def __init__(self, tmp: Path, cap: int) -> None: + self.events_file = tmp / "events.jsonl" + self.logs_dir = tmp / "logs" + self.logs_dir.mkdir(parents=True, exist_ok=True) + self.github_repo = "o/r" + self.parallel = 2 + self.critic = type("C", (), {"max_repair_rounds": cap})() + + +def _pr() -> dict[str, Any]: + return {"url": "https://github.com/o/r/pull/173", "number": 173, "body": "Fixes #168"} + + +def _seed_rounds(cfg: _Cfg, issue: int, n: int) -> None: + """count_prior_critic_rounds reads critic--*.log files off disk.""" + for i in range(n): + (cfg.logs_dir / f"critic-{issue}-{1000 + i}.log").write_text("x", encoding="utf-8") + + +def _select(cfg: _Cfg) -> list[Any]: + return blocking_pr_repairs( + cfg, # type: ignore[arg-type] + prs_requiring_repair_fn=lambda *a, **k: [_pr()], + fetch_issue_fn=lambda n, **k: {"number": n, "title": "t", "labels": []}, + pr_review_context_fn=lambda *a, **k: "ctx", + ) + + +def test_pr_over_the_cap_is_not_selected_for_repair(tmp_path: Path) -> None: + cfg = _Cfg(tmp_path, cap=4) + _seed_rounds(cfg, 168, 5) # already had five reviews + assert _select(cfg) == [], "a PR past the cap must stop eating repair ticks" + events = cfg.events_file.read_text(encoding="utf-8") + assert "repair_round_cap_reached" in events, "parking must be observable, never silent" + + +def test_pr_under_the_cap_is_still_repaired(tmp_path: Path) -> None: + """NEV-CTL-04: prove the selector can still RETURN work, or the test above is vacuous.""" + cfg = _Cfg(tmp_path, cap=4) + _seed_rounds(cfg, 168, 2) + assert len(_select(cfg)) == 1, "a converging PR must keep being repaired" + + +def test_cap_zero_disables_the_guard(tmp_path: Path) -> None: + cfg = _Cfg(tmp_path, cap=0) + _seed_rounds(cfg, 168, 99) + assert len(_select(cfg)) == 1, "cap=0 must opt out entirely" From d84d709bbeeb8eddfcca1b9493ccacb27fb619d7 Mon Sep 17 00:00:00 2001 From: Hexalgo Date: Thu, 6 Aug 2026 17:27:10 +0200 Subject: [PATCH 6/8] =?UTF-8?q?critic:=20teach=20METHOD=20vs=20OUTCOME=20?= =?UTF-8?q?=E2=80=94=20the=20criterion=20class=20that=20cost=20the=20most?= =?UTF-8?q?=20rounds?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- src/forge_loop/briefs/critic.md.tmpl | 29 +++++++++++++++++++++++++++- src/forge_loop/critic.py | 5 +++++ 2 files changed, 33 insertions(+), 1 deletion(-) diff --git a/src/forge_loop/briefs/critic.md.tmpl b/src/forge_loop/briefs/critic.md.tmpl index 26a7fa2..d73a1c0 100644 --- a/src/forge_loop/briefs/critic.md.tmpl +++ b/src/forge_loop/briefs/critic.md.tmpl @@ -43,6 +43,33 @@ means the findings were wasted work. check graded on runtime behaviour it cannot observe. The worker can build the right thing and still never satisfy it. + Q2b. IS IT A METHOD CRITERION OR AN OUTCOME CRITERION? ★ THE MOST COMMON DEFECT, AND THE + SUBTLEST. Split every criterion into what the DIFF controls and what the WORLD controls: + + METHOD — the work ran, it ran correctly, and it reported honestly. The harness executes; + the operators match what they claim; the report's headline numbers describe the + SAME run as its table; the walk used a sound oracle. ☠ BLOCK ON THESE. They are + entirely within the author's control and they are where real defects live. + + OUTCOME — what the work FOUND. How many survivors exist. How many places resolved. How much + XP a quest paid. Whether a giver offered at level 1. These are properties of the + SUBJECT, not of the diff. ☠ NEVER BLOCK ON THESE. + + THE TELL: a criterion whose cost scales with what the work discovers — "every survivor is + strengthened or deleted", ">=8 of the 36 rows filled", "the run reaches level 5". The author + cannot bound that work when they write the PR, because the number is not known until the + instrument runs. Such a criterion offers exactly two outs: explode the scope, or be + non-compliant. Both are failures the diff did not cause. + + THE CORRECTION — demand DISPOSITION AND HONESTY, never a disposition COUNT. Every finding must + carry a verdict and nothing may be silently dropped; "deferred, because it lives in a module + this PR does not touch" IS a legitimate verdict when it names the file and the reason. A PR that + reports 10 survivors, fixes 4 and defers 6 WITH REASONS is complete. A PR that reports 4 and + hides 6 is not — that is the defect worth blocking, and it is a method defect. + + ☠ Do not confuse this with letting work slide. Sloppiness is still sloppiness: an unrun + harness, a wrong operator, a self-contradicting report, a dropped finding — all still block. + Q3. DOES IT DEMAND DESTROYING EARNED WORK? A criterion that requires deleting, blanking or reverting a value that was obtained from an IRREVERSIBLE or EXPENSIVE act — a live run, a measurement, a manual walk, a paid API call @@ -172,7 +199,7 @@ FINAL OUTPUT (one JSON line, no prose after it, no markdown fence) "ordered, minimal must-fix steps — the EXACT set that makes this PR mergeable. One string per blocking finding. Empty iff overall == approve. When overall == block_on_spec, these are steps for the HUMAN (split the issue / rescope criterion N / drop criterion M), NOT for the worker." ], "spec_defects": [ - {{"kind": "unsatisfiable_in_one_pr|undecidable_by_deliverable|destroys_earned_work|environment", + {{"kind": "unsatisfiable_in_one_pr|undecidable_by_deliverable|outcome_not_method|destroys_earned_work|environment", "criterion": "the acceptance criterion, quoted verbatim from the issue", "why": "which of Q1/Q2/Q3 it fails and why no diff can satisfy it", "fix": "the concrete change to the ISSUE — split into two tickets, rescope to X, drop it, or grant access Y", diff --git a/src/forge_loop/critic.py b/src/forge_loop/critic.py index 441ffa1..af8b951 100644 --- a/src/forge_loop/critic.py +++ b/src/forge_loop/critic.py @@ -203,6 +203,11 @@ class SpecDefect: VALID_KINDS = ( "unsatisfiable_in_one_pr", "undecidable_by_deliverable", + # ☠ The commonest and subtlest: the criterion constrains what the work FOUND (an outcome the + # SUBJECT determines) instead of how it ran and reported (a method the DIFF determines). Its + # tell is that its cost scales with discovery — "every survivor fixed", ">=8 rows filled". + # It leaves the author only scope explosion or non-compliance, neither caused by the diff. + "outcome_not_method", "destroys_earned_work", "environment", ) From eeeedcc189c376120a075ca5af56f81b661f6098 Mon Sep 17 00:00:00 2001 From: Hexalgo Date: Thu, 6 Aug 2026 17:38:11 +0200 Subject: [PATCH 7/8] =?UTF-8?q?dispatch:=20new=20work=20takes=20every=20FR?= =?UTF-8?q?EE=20slot=20=E2=80=94=20`reserve`=20is=20a=20floor,=20not=20a?= =?UTF-8?q?=20ceiling?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- src/forge_loop/runner/dispatch.py | 13 ++++++++++++- tests/test_dispatch_slot_reservation.py | 9 +++++++-- 2 files changed, 19 insertions(+), 3 deletions(-) diff --git a/src/forge_loop/runner/dispatch.py b/src/forge_loop/runner/dispatch.py index 417767e..f9d6651 100644 --- a/src/forge_loop/runner/dispatch.py +++ b/src/forge_loop/runner/dispatch.py @@ -190,7 +190,18 @@ def reserved_new_work_slots( """ if repairs_pending <= 0 or ready_count <= 0 or reserve <= 0: return 0 - return max(0, min(reserve, parallel - 1)) + # ☠ USE EVERY GENUINELY FREE SLOT, not a constant 1. + # + # `reserve` is a FLOOR (never starve new work), never a ceiling. The old + # `min(reserve, parallel - 1)` capped new dispatch at ONE issue whenever any repair + # was in flight — so raising `parallel` bought nothing and the extra workers idled + # while the backlog waited. Repairs keep exactly the slots they are actually using + # (`repairs_pending`); everything left over goes to new work. + # + # Measured: with parallel=2 and one repair, one ready issue was dispatched and the + # second slot sat empty for the whole tick. + free = parallel - repairs_pending + return max(0, min(max(reserve, free), parallel - 1)) def _branch_for_issue(issue: dict[str, Any]) -> str: diff --git a/tests/test_dispatch_slot_reservation.py b/tests/test_dispatch_slot_reservation.py index 4a69b9e..86f0718 100644 --- a/tests/test_dispatch_slot_reservation.py +++ b/tests/test_dispatch_slot_reservation.py @@ -80,8 +80,13 @@ def test_reserved_count_always_within_zero_to_parallel_minus_one(parallel: int) slot, and the count is never negative.""" reserved = reserved_new_work_slots(parallel, repairs_pending=3, ready_count=3) assert 0 <= reserved <= max(0, parallel - 1) - # And specifically: with the default single-slot reserve it is min(1, p-1). - assert reserved == min(RESERVED_NEW_WORK_SLOTS, max(0, parallel - 1)) + # ☠ CONTRACT CHANGED DELIBERATELY. `reserve` is a FLOOR, not a ceiling: new work takes + # every slot the repairs in flight are not using. The old assertion pinned + # min(RESERVED_NEW_WORK_SLOTS, p-1), which capped new dispatch at ONE issue whenever any + # repair ran — so raising `parallel` bought nothing and the extra workers idled while the + # backlog waited. Repairs keep exactly `repairs_pending`; the remainder goes to new work. + free = parallel - 3 + assert reserved == max(0, min(max(RESERVED_NEW_WORK_SLOTS, free), max(0, parallel - 1))) def test_custom_reserve_is_clamped_to_parallel_minus_one() -> None: From 9ce40d286555d157691b12f7870202d8c3d2f14e Mon Sep 17 00:00:00 2001 From: Hexalgo Date: Thu, 6 Aug 2026 19:23:11 +0200 Subject: [PATCH 8/8] =?UTF-8?q?maintenance:=20never=20let=20a=20periodic?= =?UTF-8?q?=20nicety=20kill=20the=20runner=20=E2=80=94=20and=20find=20the?= =?UTF-8?q?=20SDK's=20bundled=20claude?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- src/forge_loop/maintenance.py | 51 +++++++++++++++++++- tests/test_maintenance_never_kills_runner.py | 51 ++++++++++++++++++++ 2 files changed, 101 insertions(+), 1 deletion(-) create mode 100644 tests/test_maintenance_never_kills_runner.py diff --git a/src/forge_loop/maintenance.py b/src/forge_loop/maintenance.py index 26fe028..7bb2662 100644 --- a/src/forge_loop/maintenance.py +++ b/src/forge_loop/maintenance.py @@ -15,6 +15,7 @@ import json import re +import shutil import subprocess import time from dataclasses import dataclass @@ -77,6 +78,32 @@ class MaintenanceOutcome: stdout_tail: str + + +def _claude_executable() -> str | None: + """Locate the `claude` CLI, or None. + + ☠ A BARE "claude" IS NOT ENOUGH. 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 — which is + exactly the state that crashed this runner every 5th tick. Prefer PATH, then the bundled binary + the SDK already uses. + """ + found = shutil.which("claude") + if found: + return found + try: + import claude_agent_sdk + + bundled = Path(claude_agent_sdk.__file__).parent / "_bundled" + for name in ("claude.exe", "claude"): + candidate = bundled / name + if candidate.exists(): + return str(candidate) + except Exception: # noqa: BLE001 — resolution must never raise + pass + return None + + def run_maintenance( repo: Path, logs_dir: Path, @@ -89,11 +116,24 @@ def run_maintenance( log_path = logs_dir / f"maintenance-{int(time.time())}.log" started = time.time() + # ☠ MAINTENANCE MUST NEVER TAKE THE RUNNER DOWN. It is a periodic nicety; the loop's job is to + # dispatch work. Before this guard a missing `claude` raised FileNotFoundError straight out of + # run_maintenance and killed the whole process every `maintenance_every_n_ticks` ticks — the same + # class of bug as the POSIX-only SIGUSR1 handler: an optional feature ending the service. + exe = _claude_executable() + if exe is None: + return MaintenanceOutcome( + duration_s=0.0, + acted_on=0, added_ready=[], closed_dupes=[], retitled=[], + raw={"error": "no claude CLI on PATH and none bundled with claude_agent_sdk"}, + stdout_tail="(maintenance skipped: claude CLI not found)", + ) + try: with open(log_path, "wb") as logf: subprocess.run( [ - "claude", "-p", brief, + exe, "-p", brief, "--max-turns", "30", "--allow-dangerously-skip-permissions", "--add-dir", str(repo), @@ -106,6 +146,15 @@ def run_maintenance( timeout=timeout_s, env=_subagent_env(), ) + except (FileNotFoundError, OSError) as exc: + # The resolver said it existed; the spawn still failed (deleted, not executable, bad perms). + # Degrade — never let a maintenance nicety end the runner. + return MaintenanceOutcome( + duration_s=time.time() - started, + acted_on=0, added_ready=[], closed_dupes=[], retitled=[], + raw={"error": f"maintenance spawn failed: {type(exc).__name__}: {exc}"}, + stdout_tail="(maintenance spawn failed)", + ) except subprocess.TimeoutExpired: return MaintenanceOutcome( duration_s=time.time() - started, diff --git a/tests/test_maintenance_never_kills_runner.py b/tests/test_maintenance_never_kills_runner.py new file mode 100644 index 0000000..d2d41fc --- /dev/null +++ b/tests/test_maintenance_never_kills_runner.py @@ -0,0 +1,51 @@ +"""Maintenance is a periodic nicety. It must NEVER end the runner. + +☠ run_maintenance shelled out to a bare "claude". The agent SDK ships its OWN claude binary and the +workers use that, so a machine can run workers perfectly while having no `claude` on PATH — exactly +this machine. FileNotFoundError escaped run_maintenance and killed the whole process every +`maintenance_every_n_ticks` ticks. Same class as the POSIX-only SIGUSR1 handler: an optional feature +ending the service. +""" + +from __future__ import annotations + +from pathlib import Path + +import pytest + +import forge_loop.maintenance as m + + +def test_missing_claude_returns_an_outcome_instead_of_raising( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + monkeypatch.setattr(m, "_claude_executable", lambda: None) + monkeypatch.setattr(m, "ensure_subagent_trusted", lambda *_a, **_k: None) + + out = m.run_maintenance(tmp_path, tmp_path / "logs") + + assert out.acted_on == 0 + assert "claude" in str(out.raw.get("error", "")).lower() + + +def test_spawn_failure_is_also_degraded(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> None: + """The resolver can succeed and the spawn still fail — deleted, not executable, bad perms.""" + monkeypatch.setattr(m, "_claude_executable", lambda: "/nonexistent/claude") + monkeypatch.setattr(m, "ensure_subagent_trusted", lambda *_a, **_k: None) + + def _boom(*_a, **_k): + raise FileNotFoundError(2, "The system cannot find the file specified") + + monkeypatch.setattr(m.subprocess, "run", _boom) + + out = m.run_maintenance(tmp_path, tmp_path / "logs") + assert "spawn failed" in str(out.raw.get("error", "")).lower() + + +def test_resolver_falls_back_to_the_sdk_bundled_binary(monkeypatch: pytest.MonkeyPatch) -> None: + """NEV-CTL-04: prove the resolver can actually FIND something, or the tests above are vacuous.""" + monkeypatch.setattr(m.shutil, "which", lambda _n: None) # force the fallback path + found = m._claude_executable() + assert found is not None and "claude" in found.lower(), ( + "the SDK bundles a claude binary; the fallback must find it" + )