Fix/critic block on spec

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,

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,32 @@ 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",
+        # ☠ 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",
+    )
+
+    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 +232,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,13 +1163,42 @@ 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,
         manifesto_violations=violations,
         raw=raw,
         minimal_path_to_green=minimal_path_to_green,
         follow_ups=follow_ups,
+        spec_defects=spec_defects,
     )
 
 

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,

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:

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")

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:
@@ -873,6 +884,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,

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(

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-<family>-<major>-<minor>`` 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"})
@@ -175,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

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,