diff --git a/src/forge_loop/briefs/critic.md.tmpl b/src/forge_loop/briefs/critic.md.tmpl index 2a5b766..d73a1c0 100644 --- a/src/forge_loop/briefs/critic.md.tmpl +++ b/src/forge_loop/briefs/critic.md.tmpl @@ -1,158 +1,224 @@ 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. + + 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 + — 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|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", + "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 +226,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/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/critic.py b/src/forge_loop/critic.py index 7ba9fc0..af8b951 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,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,6 +1163,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 +1198,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/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/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: 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/src/forge_loop/runner/dispatch.py b/src/forge_loop/runner/dispatch.py index cc33e60..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: @@ -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, 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 7c3f25f..dfbd791 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"}) @@ -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 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_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" 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: 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" + ) 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" 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" 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"