From 461245a57cdb9c65cadd046e31ecfc2537611230 Mon Sep 17 00:00:00 2001 From: Alex Clarke Date: Fri, 18 Sep 2026 10:44:04 -0600 Subject: [PATCH] fix: force code review lanes to be additive only, not opt-in, and bump the verifier run cap --- assets/agents/finding-verifier/graph.yaml | 2 +- assets/agents/probe/config.yaml | 10 +- assets/agents/review-gauntlet/README.md | 10 +- assets/agents/review-gauntlet/graph.yaml | 17 ++- .../review-gauntlet/scripts/build_items.py | 130 ++++++++++++------ .../review-gauntlet/scripts/default_lanes.py | 11 +- .../agents/review-gauntlet/scripts/signals.py | 31 ++++- 7 files changed, 145 insertions(+), 66 deletions(-) diff --git a/assets/agents/finding-verifier/graph.yaml b/assets/agents/finding-verifier/graph.yaml index 4782e87..c8c75c6 100644 --- a/assets/agents/finding-verifier/graph.yaml +++ b/assets/agents/finding-verifier/graph.yaml @@ -174,7 +174,7 @@ nodes: - fs_read - fs_cat - fs_grep - max_iterations: 14 + max_iterations: 28 max_attempts: 2 state_updates: verdict: '{{output}}' diff --git a/assets/agents/probe/config.yaml b/assets/agents/probe/config.yaml index 9d7febd..aa64c79 100644 --- a/assets/agents/probe/config.yaml +++ b/assets/agents/probe/config.yaml @@ -120,7 +120,15 @@ instructions: | update — never delete or silence it). 4. **Clean, local, isolated.** Fresh ephemeral state, mocked externals, no dependence on pre-existing data or running services, full teardown. Bounded retries for startup only — - never to mask a flaky assertion. + never to mask a flaky assertion. Teardown MUST NOT depend on a shell trap firing (a + killed or timed-out lane leaks containers — it has happened): pick one unique run + prefix up front (e.g. `probe-$$`), give EVERY container you create `--name -…` + and `--label coyote-probe=` (compose: `-p `), prefer `docker run --rm`, + and make the LAST action before emitting your verdict an explicit sweep: + `docker ps -aq --filter label=coyote-probe= | xargs -r docker rm -f` (compose: + `docker compose -p down -v`). The label filter keeps the sweep scoped to YOUR + resources — rule 0 still forbids touching anyone else's. A leftover you cannot remove + is a finding to REPORT in the verdict, never to silence. 5. **Classify every failure** as BUG / EXPECTED-CHANGE / ENV per the skill table. The verdict depends on the classification being honest. 6. **Committed tests are the deliverable** alongside the verdict: write them where the repo's diff --git a/assets/agents/review-gauntlet/README.md b/assets/agents/review-gauntlet/README.md index f58f865..613abb1 100644 --- a/assets/agents/review-gauntlet/README.md +++ b/assets/agents/review-gauntlet/README.md @@ -14,7 +14,7 @@ flowchart TD parse -. "fallback" .-> pfault["parse_fault
(script: PIPELINE-FAULT marker)"] pfault --> gate signals --> select["select_lanes
(llm: ADDITIVE-only judgment)"] - select --> build["build_items
(script: rules floor ∪ additions;
forced lanes override exactly)"] + select --> build["build_items
(script: rules floor ∪ additions;
forced lanes add)"] select -. "fallback" .-> dlanes["default_lanes
(script: deterministic degraded
selection — wider, never narrower)"] dlanes --> build build --> mcr["map: code-review"] & madv["map: adversary"] & msec["map: security"] & mpb["map: probe"] @@ -155,7 +155,7 @@ Plan / acceptance criteria: Local-run recipe / usage suites: -Forced lanes: (optional — e.g. 'adversary, probe' to run exactly those)" \ +Forced lanes: (optional, ADDITIVE — e.g. 'adversary, probe' run on top of the computed selection; forcing never drops the always-on floor)" \ --variables {"verification_commands": "[\"cargo test --all\", \"cargo clippy -- -D warnings\"]"} ``` @@ -210,9 +210,9 @@ verbatim. (`'[]'` ⇒ "none declared"); a declaration that is not a JSON array of strings executes nothing and records a `PIPELINE-FAULT`, which the adversary lane reports as DIVERGES and the gate as BLOCKED. -- Signals failures degrade gracefully: if git can't compute the diff, lane - selection falls back to caller context + forced lanes, and the report notes - it. The auth-path regex deliberately over-fires (`auth(?!or)` matches +- Signals failures degrade WIDER: if git can't compute the diff, the surface + is unknown — the security lane is added (and probe, when a local-run recipe + exists) on top of caller context + forced lanes, and the report notes it. The auth-path regex deliberately over-fires (`auth(?!or)` matches auth/authn/authz/oauth but not "author") — the safe direction, since signals only ever ADD the security lane. - Requires the `code-reviewer`, `adversary`, `security-reviewer`, and `probe` diff --git a/assets/agents/review-gauntlet/graph.yaml b/assets/agents/review-gauntlet/graph.yaml index 3fd00a7..23ce12e 100644 --- a/assets/agents/review-gauntlet/graph.yaml +++ b/assets/agents/review-gauntlet/graph.yaml @@ -107,10 +107,14 @@ nodes: You extract the facts of a review request into structured fields. EXTRACTION ONLY — do not decide which review lanes should run. - - `diff_spec`: the git diff specification the caller named — a ref - range ("main...HEAD", "HEAD~3..HEAD"), a single ref, or the literal - "worktree" for uncommitted changes against HEAD. When the caller - did not name one, use "worktree". + - `diff_spec`: the git diff specification the caller named, as a BARE + revision token usable verbatim in `git diff ` — a ref range + ("main...HEAD", "HEAD~3..HEAD"), a single ref, or the literal + "worktree" for uncommitted changes against HEAD. NEVER prose, a + command, or a path: from "run get_diff --base origin/main" extract + "origin/main...HEAD"; from "diff origin/main...HEAD in ~/code/x" + extract "origin/main...HEAD". When the caller did not name one, use + "worktree". - `project_dir_in`: the repository directory the caller named as holding the change (absolute path preferred); empty string when the caller named none. @@ -127,7 +131,8 @@ nodes: pointers for black-box probing (how to boot the service, where the suites live). Empty string when the caller supplied none. - `forced_lanes`: lanes the caller explicitly demanded (verbatim - names). Empty array = automatic selection. + names). Empty array = automatic selection. Forcing is ADDITIVE + downstream: it widens the computed selection, never replaces it. - `extra_context`: any remaining caller context reviewers should see, condensed. prompt: | @@ -261,7 +266,7 @@ nodes: build_items: id: build_items type: script - description: Deterministic lane builder — hard rules are the floor, LLM flags may only add, forced lanes override exactly. Emits one 0-or-1-item list per lane; an empty list skips that lane's map. On a re-run pass (retry_lanes non-empty) it skips selection and emits items only for those lanes, each carrying its attempt number. + description: Deterministic lane builder — hard rules are the floor, LLM flags may only add, forced lanes are ADDITIVE (selection only ever widens; unavailable signals widen it too). Emits one 0-or-1-item list per lane; an empty list skips that lane's map. On a re-run pass (retry_lanes non-empty) it skips selection and emits items only for those lanes, each carrying its attempt number. script: scripts/build_items.py timeout: 30 next: [map_code_review, map_adversary, map_security, map_probe] diff --git a/assets/agents/review-gauntlet/scripts/build_items.py b/assets/agents/review-gauntlet/scripts/build_items.py index 0183c91..099cae5 100644 --- a/assets/agents/review-gauntlet/scripts/build_items.py +++ b/assets/agents/review-gauntlet/scripts/build_items.py @@ -1,14 +1,20 @@ #!/usr/bin/env python3 """Deterministic lane builder for the review gauntlet. -Selection = hard rules (the floor) ∪ LLM additions; forced lanes override -everything exactly. Emits one 0-or-1-item list per lane: an empty list makes -that lane's map node run zero branches (map-over-nothing), which IS the -skip mechanism — no dynamic routing anywhere. +Selection = hard rules (the floor) ∪ LLM additions ∪ caller-forced lanes. +Forcing is ADDITIVE: it can only widen the computed selection — honoring a +forced list as the exact set once dropped the always-on code-review lane in +a live run. Emits one 0-or-1-item list per lane: an empty list makes that +lane's map node run zero branches (map-over-nothing), which IS the skip +mechanism — no dynamic routing anywhere. The LLM's add_* flags can only ADD lanes. A flag that is false never removes a lane the hard rules selected. +Unavailable diff signals (malformed spec, git failure) mean the surface is +UNKNOWN — selection degrades WIDER (security on; probe when a recipe +exists), never narrower. + A re-run pass (retry_gate emitted `retry_lanes`) skips selection: only the named lanes get an item, stamped with their attempt number, and every other lane maps over nothing — retry_gate restores their earlier reports. @@ -101,56 +107,90 @@ def main(): (forced.append(canon) if canon else unknown.append(str(lane))) reasons = [] - if forced: - lanes = set(forced) - reasons.append(f"forced lanes honored exactly: {sorted(lanes)}") - else: - lanes = {"code-review"} + lanes = {"code-review"} + reasons.append( + "code-review: always on — the gauntlet is only spawned for non-trivial work" + ) + if text("plan_context"): + lanes.add("adversary") + reasons.append("adversary: a spec/plan/acceptance-criteria context is present") + if ( + state.get("touches_auth") + or state.get("touches_deps") + or state.get("touches_exec") + or state.get("security_posture") == "hardened" + ): + lanes.add("security") reasons.append( - "code-review: always on — the gauntlet is only spawned for non-trivial work" + "security: attack-surface signals (auth/deps/exec) or hardened posture" ) - if text("plan_context"): - lanes.add("adversary") - reasons.append("adversary: a spec/plan/acceptance-criteria context is present") - if ( - state.get("touches_auth") - or state.get("touches_deps") - or state.get("touches_exec") - or state.get("security_posture") == "hardened" - ): + if state.get("consumer_surface") and text("probe_context"): + lanes.add("probe") + reasons.append( + "probe: consumer-facing surface changed and a local-run recipe/suite pointer exists" + ) + + # Unavailable signals (malformed diff spec, git failure) = surface UNKNOWN. + # Degrade WIDER, never narrower — a run without signals once silently + # dropped the security and probe lanes. + if text("signals_summary") == "signals unavailable": + if "security" not in lanes: lanes.add("security") reasons.append( - "security: attack-surface signals (auth/deps/exec) or hardened posture" + "security: diff signals unavailable — surface unknown, degrading wider" ) - if state.get("consumer_surface") and text("probe_context"): + if "probe" not in lanes and text("probe_context"): lanes.add("probe") reasons.append( - "probe: consumer-facing surface changed and a local-run recipe/suite pointer exists" + "probe: diff signals unavailable and a local-run recipe/suite pointer " + "exists — degrading wider" ) - # LLM judgment is additive-only. - llm_why = text("lane_reasons")[:300] - for flag, lane in ( - ("add_code_review", "code-review"), - ("add_adversary", "adversary"), - ("add_security", "security"), - ("add_probe", "probe"), - ): - if state.get(flag) and lane not in lanes: - if lane == "probe" and not text("probe_context"): - reasons.append( - "probe: requested by lane judgment but skipped — no local-run " - "recipe/suite pointer (a probe without one is guaranteed INCONCLUSIVE)" - ) - continue - if lane == "adversary" and not text("plan_context"): - reasons.append( - "adversary: requested by lane judgment but skipped — no spec/plan " - "to check conformance against" - ) - continue - lanes.add(lane) - reasons.append(f"{lane}: added by lane-selection judgment — {llm_why}") + # LLM judgment is additive-only. + llm_why = text("lane_reasons")[:300] + for flag, lane in ( + ("add_code_review", "code-review"), + ("add_adversary", "adversary"), + ("add_security", "security"), + ("add_probe", "probe"), + ): + if state.get(flag) and lane not in lanes: + if lane == "probe" and not text("probe_context"): + reasons.append( + "probe: requested by lane judgment but skipped — no local-run " + "recipe/suite pointer (a probe without one is guaranteed INCONCLUSIVE)" + ) + continue + if lane == "adversary" and not text("plan_context"): + reasons.append( + "adversary: requested by lane judgment but skipped — no spec/plan " + "to check conformance against" + ) + continue + lanes.add(lane) + reasons.append(f"{lane}: added by lane-selection judgment — {llm_why}") + + # Caller-forced lanes are ADDITIVE: they widen the computed selection, + # never replace it. (Honoring them as the exact set dropped code-review — + # documented as always-on — and adversary in a live run.) Forcing also + # bypasses the context guards above: the caller demanded the lane, so it + # runs, with the expectation named in the summary. + if forced: + newly = sorted(set(forced) - lanes) + lanes |= set(forced) + reasons.append( + f"forced lanes (additive): {sorted(set(forced))}" + + (f" — newly added: {newly}" if newly else " — all already selected") + ) + if "probe" in forced and not text("probe_context"): + reasons.append( + "probe: forced without a local-run recipe/suite pointer — expect INCONCLUSIVE" + ) + if "adversary" in forced and not text("plan_context"): + reasons.append( + "adversary: forced without spec/plan context — it fails closed " + "(DIVERGES) without extractable criteria" + ) if unknown: reasons.append(f"ignored unknown forced lane name(s): {unknown}") diff --git a/assets/agents/review-gauntlet/scripts/default_lanes.py b/assets/agents/review-gauntlet/scripts/default_lanes.py index ffaeabf..8a8cf1a 100644 --- a/assets/agents/review-gauntlet/scripts/default_lanes.py +++ b/assets/agents/review-gauntlet/scripts/default_lanes.py @@ -8,8 +8,10 @@ AND a local-run recipe exists (build_items gates probe the same way — a probe without a recipe is INCONCLUSIVE by construction), plus security when the attack-surface signals (auth/deps/exec) or a hardened posture demand it, unioned with any lanes the caller forced (canonicalized -with the same aliases build_items uses) — which build_items honors exactly, -so selection degrades WIDER, never narrower. The degradation note goes into +with the same aliases build_items uses) — which build_items honors +ADDITIVELY, so selection degrades WIDER, never narrower. Unavailable diff +signals also widen: unknown surface adds security (and probe when a +local-run recipe exists). The degradation note goes into `lanes_degraded`; build_items folds it into lanes_summary so the final report names the degradation. @@ -72,6 +74,11 @@ def main(): or state.get("security_posture") == "hardened" ): lanes.add("security") + # Unavailable signals = surface UNKNOWN → widen (mirrors build_items). + if (state.get("signals_summary") or "") == "signals unavailable": + lanes.add("security") + if isinstance(probe_context, str) and probe_context.strip(): + lanes.add("probe") # Caller-forced lanes survive degradation — union, never overwrite. for lane in state.get("forced_lanes") or []: if canon := ALIASES.get(str(lane).strip().lower()): diff --git a/assets/agents/review-gauntlet/scripts/signals.py b/assets/agents/review-gauntlet/scripts/signals.py index 42f4a7b..01d3695 100644 --- a/assets/agents/review-gauntlet/scripts/signals.py +++ b/assets/agents/review-gauntlet/scripts/signals.py @@ -3,9 +3,10 @@ Pure git — no LLM judgment. Every signal is computed from the changed-file list and the added lines of the diff. Failures never sink the gauntlet: -on any error the script reports `signals_error` and leaves the defaults -(false/empty) in place, which biases lane selection toward the caller's -context and forced lanes. +on any error the script reports `signals_error` and marks the signals +unavailable — downstream (build_items / default_lanes) treats that as +UNKNOWN surface and degrades WIDER (security on; probe when a recipe +exists), on top of caller context and forced lanes. """ import json @@ -26,6 +27,19 @@ proj = os.path.expanduser( (state.get("project_dir_in") or "").strip() or state.get("project_dir") or "." ) spec = (state.get("diff_spec") or "worktree").strip() +spec_raw = spec +# The parse stage should emit a bare revision token, but callers write prose +# and extraction drifts ("run get_diff --base origin/main" once reached +# `git diff` verbatim, killing the signals — and with them the security and +# probe lanes). Deterministically recover a usable spec before giving up. +_TOKEN = r"[\w./~^@{}-]+" +if spec not in ("", "worktree") and not re.fullmatch( + rf"{_TOKEN}(\.\.\.?{_TOKEN})?", spec +): + if m := re.search(rf"({_TOKEN}\.\.\.?{_TOKEN})", spec): + spec = m.group(1) # a revision range buried in prose + elif m := re.search(rf"--base[= ]({_TOKEN})", spec): + spec = f"{m.group(1)}...HEAD" # the get_diff idiom def git(*args): @@ -112,13 +126,18 @@ try: ), "docs_only": bool(files) and all(re.search(r"\.(md|rst|txt|adoc)$", f, re.I) for f in files), - "signals_summary": f"{len(files)} file(s) changed (diff: {spec})", + "signals_summary": f"{len(files)} file(s) changed (diff: {spec})" + + (f" — spec normalized from {spec_raw!r}" if spec != spec_raw else ""), } ) except Exception as e: # noqa: BLE001 — degraded signals must not sink the gauntlet + what = f"diff spec {spec!r}" + ( + f", normalized from {spec_raw!r}" if spec != spec_raw else "" + ) out["signals_error"] = ( - f"NOTE: diff signals could not be computed ({e}); " - "lane selection falls back to caller context and forced lanes." + f"NOTE: diff signals could not be computed ({what}: {e}); surface UNKNOWN — " + "lane selection degrades WIDER (security lane added; probe eligible when a " + "local-run recipe exists), plus caller context and forced lanes." ) out["signals_summary"] = "signals unavailable"