This repository has been archived on 2026-08-04. You can view files and clone it, but cannot push or open issues or pull requests.
orchestrator/agent-team/agent_team/nodes/verifier.py
Adam Moussa 00c51192c8 fix(agent-team): remediate C1 security-review BLOCK (2 HIGH + MED/LOW)
High-recall /sh-security-review fan-out + proof-or-kill verifier found two
confirmed HIGH; both now closed (verified empirically against the working tree):

- LOGIC-RACE-01 (HIGH, CWE-835): the build-loop budget was structurally dead
  (verifier read a shared wiring-time VerifierConfig.build_loops, always 0, so
  the max_build_loops park never fired -> a perpetually-failing task looped
  BUILD->DISPATCH->VERIFY forever, force-pushing + firing a CI run each round).
  Threaded build_loops through durable PipelineState/TaskRecord; verifier reads
  state.get('build_loops',0), writes the incremented count back on each FAIL, and
  PARKS at max_build_loops. Parks after exactly N failures, never unbounded.
- SEC-01 (HIGH, CWE-532) + SEC-02 (MED, CWE-214): p3_rollback.sh echoed the live
  App JWT to stdout in default dry-run and passed it as a gh argv literal. Added
  redact_secrets (Bearer/Authorization/ghX_/PEM masking) through run_or_plan; the
  App uninstall now uses curl -H @<0600 tempfile> (JWT never on argv), shredded
  after. Empirical: app/incident/all dry-runs leak 0 JWT occurrences.
- SEC-03 (MED, CWE-798): assert_no_write_token now applies the PEM regex + the
  configured App-ID to env/config VALUES (not just files) — an App private key
  under a benign env name is caught.
- SEC-04 (LOW) + P3-IAC-08 (LOW): tightened the box GITHUB_TOKEN fallback /
  value-scan; staged-only WARN on the live workflow revert.

Suite: 1382 passed, ruff clean. Branch only; not merged/deployed.
NOTE: re-verifier flagged SEC-01 as open by grepping COMMITTED blobs (the fix was
uncommitted working-tree state); independently confirmed closed empirically.
2026-06-23 19:52:04 -04:00

303 lines
13 KiB
Python

"""Verifier node — the VERIFY-stage LangGraph node (design §3.3, §3.3.2 P3).
The verifier reads the authenticated CI results for a task's candidate diff and
either advances it to a draft PR or loops it back to the builders. The single
load-bearing rule from §3.3.2 boundary #4 is enforced structurally here:
**The verifier *agent* cannot declare success.** Pass/fail is owned by the
pure-code gate (:mod:`agent_team.ci_gate`) over the authenticated,
patch-independent CI conclusion. The LLM verifier only ever reads *failures*
to propose the next fix.
So this node calls :func:`agent_team.ci_gate.evaluate_ci_gate` for the decision
and consults an (optional, injectable) LLM **only** on a FAIL/BLOCK to author a
fix hint for the builders. The LLM is never asked whether the task passed.
State contract (mirrors :class:`agent_team.task_model.PipelineState`):
* reads ``candidate_diff``, ``diff_hash`` (ledger hash), ``ci_results``,
``build_loops`` (the durable per-task build<->verify count);
* writes ``status``, ``current_phase``, ``review_verdicts`` (appends the gate
verdict), ``ci_results`` (annotated with the gate decision for provenance),
and — on a recoverable gate FAIL — the incremented ``build_loops`` so the
budget advances across the BUILD->DISPATCH->VERIFY loop (LOGIC-RACE-01).
Transitions (the §3.3 "Stability + autonomy bounds" — the verifier must pass or
the task loops/holds, never ships):
* gate PASS -> :class:`~agent_team.task_model.Phase.DONE`,
:class:`~agent_team.task_model.TaskStatus.DONE` (verifier produced a draft PR).
* gate FAIL -> :class:`~agent_team.task_model.Phase.BUILD`,
:class:`~agent_team.task_model.TaskStatus.ACTIVE` (loop back to builders),
unless the per-task build-loop budget is spent, in which case it PARKs.
* gate BLOCK -> :class:`~agent_team.task_model.Phase.PARKED`,
:class:`~agent_team.task_model.TaskStatus.PARKED` — an ALARM-worthy trust
violation (denylist hit, hash mismatch, ambiguous conclusion) is never auto-
retried; it parks for human + GPT cross-review.
This module imports the committed foundation contracts verbatim and redefines
none of them. The LLM seam is injectable (mirroring
:func:`agent_team.billing.set_invoker`) so the node is pure-function testable
with no SDK or network.
"""
from __future__ import annotations
from dataclasses import dataclass
from datetime import datetime, timezone
from typing import Any, Callable, Mapping
from agent_team.ci_gate import GateDecision, GateResult, evaluate_ci_gate
from agent_team.task_model import Phase, PipelineState, TaskStatus
__all__ = [
"DEFAULT_MAX_BUILD_LOOPS",
"VerifierConfig",
"set_fix_advisor",
"verifier_node",
]
# Default cap on build<->verify loops before a still-failing task parks rather
# than spinning (§3.3 bound #6 "N failed build loops"). Configurable per task
# via :class:`VerifierConfig`.
DEFAULT_MAX_BUILD_LOOPS: int = 3
@dataclass
class VerifierConfig:
"""Per-invocation knobs for the verifier node (§3.3, §3.3.2).
``expected_run_id`` is a STATIC fallback only. The gate binds to the run id
THIS task dispatched, read from ``state["run_id"]`` at node-run time (the
dispatcher persists it there); the config value is consulted only when state
carries no ``run_id`` (e.g. a unit harness that drives the node directly). A
per-task ``state["run_id"]`` therefore always wins over this constant, and a
``None`` effective run id is a BLOCK (never a vacuous pass).
``allowed_scope`` is the task's declared-scope path prefixes for the
denylist boundary. ``max_build_loops`` caps build<->verify retries before
the task parks.
``build_loops`` is an INITIAL FALLBACK ONLY. The loop count that actually
bounds the build<->verify cycle is DURABLE per-task state read from
``state["build_loops"]`` at node-run time, because a single ``VerifierConfig``
is shared across every task at wiring time and never advances (LOGIC-RACE-01:
reading the count from this shared config meant the park guard never fired and
a perpetually-FAILing task looped BUILD->DISPATCH->VERIFY forever). The
verifier writes the incremented count back into the returned partial state so
the checkpointer carries it to the NEXT VERIFY. This field is consulted only
when state carries no ``build_loops`` (e.g. a unit harness driving the node
directly).
"""
expected_run_id: str | None = None
allowed_scope: list[str] | None = None
max_build_loops: int = DEFAULT_MAX_BUILD_LOOPS
build_loops: int = 0
# Fix-advisor seam: signature (gate_result, state) -> str. Consulted ONLY on a
# gate FAIL/BLOCK to author a fix hint for the builders from the *failure*
# details. Never consulted on PASS — the gate, not the LLM, owns success. The
# default returns no hint so an un-wired environment degrades to "loop back with
# no extra guidance" rather than crashing.
FixAdvisor = Callable[[GateResult, Mapping[str, Any]], str]
def _null_advisor(gate_result: GateResult, state: Mapping[str, Any]) -> str:
return ""
_fix_advisor: FixAdvisor = _null_advisor
def set_fix_advisor(advisor: FixAdvisor) -> None:
"""Bind the LLM fix-advisor consulted on a gate FAIL/BLOCK.
Leaves call this once at startup with an implementation that reads the gate
failure reasons + the CI logs and authors a next-fix hint for the builders.
Keeping it injectable keeps this node dependency-free and unit-testable, and
structurally enforces that the advisor is only ever invoked on failure (this
module never calls it on a PASS).
"""
global _fix_advisor
_fix_advisor = advisor
def _utc_now_iso() -> str:
return datetime.now(timezone.utc).isoformat()
def _verdict(
gate_result: GateResult,
*,
next_phase: Phase,
build_loops: int,
fix_hint: str = "",
) -> dict[str, Any]:
"""Build the review-verdict record appended to ``review_verdicts``."""
return {
"stage": "verify",
"decision": gate_result.decision.value,
"reasons": list(gate_result.reasons),
"run_id": gate_result.run_id,
"diff_hash": gate_result.diff_hash,
"ci_conclusion": gate_result.ci_conclusion,
"next_phase": next_phase.value,
"build_loops": build_loops,
"fix_hint": fix_hint,
"at": _utc_now_iso(),
}
def verifier_node(
state: PipelineState,
config: VerifierConfig,
) -> PipelineState:
"""Run the VERIFY stage: gate the CI result, decide the next phase.
Pure function of ``state`` + ``config`` (the gate does no I/O; the optional
fix-advisor is the only outbound call, and only on failure). Returns a
**partial** :class:`PipelineState` update (``total=False``) carrying the new
``status``/``current_phase``, an appended ``review_verdicts`` entry, and a
gate-annotated ``ci_results`` for provenance — the LangGraph reducer merges
it into the durable thread state.
Decision flow (§3.3.2 boundary #4 + §3.3 autonomy bounds):
1. Resolve the PER-TASK expected run id from ``state["run_id"]`` (the id the
dispatch node captured for THIS task), falling back to
``config.expected_run_id`` only when state carries none. Call
:func:`agent_team.ci_gate.evaluate_ci_gate` with the candidate diff, the
ledger hash (``diff_hash``), the authenticated ``ci_results``, that
per-task expected run id, and the task's ``allowed_scope``. A ``None``
effective run id BLOCKs (anti-substitution: the verdict has nothing to
bind to), never a vacuous pass.
2. PASS -> advance to DONE (draft PR). The advisor is NOT consulted.
3. FAIL -> consult the fix-advisor for a hint, then loop back to BUILD —
unless ``build_loops`` has reached ``max_build_loops``, in which case
PARK (a task that keeps failing must hold, never ship: §3.3 bound #3/#6).
4. BLOCK -> PARK + (advisor hint) for human + GPT cross-review. A trust
violation is never auto-retried.
"""
candidate_diff = state.get("candidate_diff")
ledger_hash = state.get("diff_hash")
ci_results = state.get("ci_results")
# Per-task binding: the gate must compare CI's run_id against the id THIS
# task dispatched (persisted by the dispatch node as ``state["run_id"]``),
# not a static wiring-time constant. State wins; the config value is only a
# fallback for harnesses that drive the node without a per-task run_id. A
# blank/None effective run id is left as None so the gate BLOCKs.
state_run_id = state.get("run_id")
expected_run_id = (
state_run_id
if isinstance(state_run_id, str) and state_run_id
else config.expected_run_id
)
# Per-task build-loop budget: the count that bounds the build<->verify cycle
# is DURABLE per-task state (``state["build_loops"]``), NOT the shared
# wiring-time config. Reading it from state is the LOGIC-RACE-01 fix: the
# shared ``VerifierConfig.build_loops`` never advanced, so the park guard
# never fired and a perpetually-FAILing task looped forever. ``config`` is
# only a fallback for a harness that drives the node without per-task state.
state_build_loops = state.get("build_loops")
current_build_loops = (
state_build_loops if isinstance(state_build_loops, int) else config.build_loops
)
if not isinstance(candidate_diff, str):
# No diff to verify is itself a refuse-to-proceed: park for a human
# rather than declaring anything. (A builder must have produced a diff
# before VERIFY runs.)
gate_result = GateResult(
decision=GateDecision.BLOCK,
reasons=["no candidate_diff present in state to verify"],
run_id=expected_run_id,
diff_hash=ledger_hash,
ci_conclusion=None,
)
else:
gate_result = evaluate_ci_gate(
candidate_diff=candidate_diff,
ledger_hash=ledger_hash,
ci_result=ci_results,
expected_run_id=expected_run_id,
allowed_scope=config.allowed_scope,
)
annotated_ci = dict(ci_results) if isinstance(ci_results, Mapping) else {}
annotated_ci["gate_decision"] = gate_result.decision.value
annotated_ci["gate_reasons"] = list(gate_result.reasons)
if gate_result.decision is GateDecision.PASS:
verdict = _verdict(
gate_result, next_phase=Phase.DONE, build_loops=current_build_loops
)
return {
"status": TaskStatus.DONE.value,
"current_phase": Phase.DONE.value,
"review_verdicts": [verdict],
"ci_results": annotated_ci,
"updated_at": verdict["at"],
}
# Every non-PASS path consults the fix-advisor on the *failure* details.
# This is the only place the LLM is touched, and it never decides success.
fix_hint = _fix_advisor(gate_result, state)
if gate_result.decision is GateDecision.FAIL:
# Count this failed loop against the DURABLE per-task budget read above.
next_loops = current_build_loops + 1
if next_loops >= config.max_build_loops:
# Exhausted the build-loop budget: hold rather than spin (§3.3 #6).
verdict = _verdict(
gate_result,
next_phase=Phase.PARKED,
build_loops=next_loops,
fix_hint=fix_hint,
)
verdict["reasons"].append(
f"max build loops ({config.max_build_loops}) reached; parking"
)
return {
"status": TaskStatus.PARKED.value,
"current_phase": Phase.PARKED.value,
"review_verdicts": [verdict],
"ci_results": annotated_ci,
"build_loops": next_loops,
"updated_at": verdict["at"],
}
verdict = _verdict(
gate_result,
next_phase=Phase.BUILD,
build_loops=next_loops,
fix_hint=fix_hint,
)
# Persist the incremented count into DURABLE state so the NEXT VERIFY
# (after BUILD->DISPATCH) sees it and the budget actually advances. The
# checkpointer carries it because it is a PipelineState key.
return {
"status": TaskStatus.ACTIVE.value,
"current_phase": Phase.BUILD.value,
"review_verdicts": [verdict],
"ci_results": annotated_ci,
"build_loops": next_loops,
"updated_at": verdict["at"],
}
# GateDecision.BLOCK — trust violation. Park for human + GPT cross-review;
# never auto-retried, never shipped.
verdict = _verdict(
gate_result,
next_phase=Phase.PARKED,
build_loops=current_build_loops,
fix_hint=fix_hint,
)
return {
"status": TaskStatus.PARKED.value,
"current_phase": Phase.PARKED.value,
"review_verdicts": [verdict],
"ci_results": annotated_ci,
"updated_at": verdict["at"],
}