diff --git a/BACKLOG.md b/BACKLOG.md index 6c97eaf..61d29fd 100644 --- a/BACKLOG.md +++ b/BACKLOG.md @@ -206,15 +206,15 @@ d=pathlib.Path(tempfile` ### B. Model-supplied values trusted as code-set facts -- [ ] **HIGH** `pipeline.py:437` — Triage decisions are keyed on the model-supplied `TriageDecision.finding_id`, so a verified finding the triage agent omits or renames silently gets no band-aid, no ledger entry and no log line +- [x] **HIGH** `pipeline.py:437` — Triage decisions are keyed on the model-supplied `TriageDecision.finding_id`, so a verified finding the triage agent omits or renames silently gets no band-aid, no ledger entry and no log line - *Fails when:* `TriageBatch`/`TriageDecision` is the triage agent's instructor response_model, so `finding_id` is a free string the model writes. pipeline.py:437 does `f = by_id.get(d.finding_id); if not f: continue` — an id that matches nothing is dropped with no log — and there is no reciprocal check that every verified finding got a decision. ledger.init_from_scan (ledger.py:72) then builds the ENTIRE ledger - *Repro:* `/Users/d.henley/demos/virtual-patch-copilot/.venv/bin/python /private/tmp/claude-502/-Users-d-henley-demos-virtual-patch-copilot/fa4d9adf-b050-4d9d-911d-2130d7c6285b/scratchpad/rep` - *Why the suite misses it:* tests/test_pipeline_replay.py's FakeHarness always returns decisions whose finding_id matches the finding exactly, and test_ledger.py seeds decisions from the same ids it seeds findings from. No test feeds the pipeline a decision list that is shorter than the verified list or carries an id that is n -- [ ] **HIGH** `pipeline.py:416` — `GeneratedArtifact.finding_id` and `.control` come from the generate agent and are written verbatim into policies.json — the policy->finding index every apply/refine/simulate/emit/backfill path resolves through +- [x] **HIGH** `pipeline.py:416` — `GeneratedArtifact.finding_id` and `.control` come from the generate agent and are written verbatim into policies.json — the policy->finding index every apply/refine/simulate/emit/backfill path resolves through - *Fails when:* generate.run is called with an explicit finding and an explicit control ('CONTROL TO GENERATE: service_policy'), but the returned artifacts are appended unchanged (pipeline.py:416-429) and _write_out:570 writes `{finding_id: a.finding_id, control: a.control.value, ...}` as policies.json. Consequences, all reproduced: (1) ledger.find_finding_for_policy returns the model's id, so applying that polic - *Repro:* `/Users/d.henley/demos/virtual-patch-copilot/.venv/bin/python /private/tmp/claude-502/-Users-d-henley-demos-virtual-patch-copilot/fa4d9adf-b050-4d9d-911d-2130d7c6285b/scratchpad/rep` - *Why the suite misses it:* Every fixture that exercises generate (tests/test_pipeline_replay.py:27-31 and the mcp/ci fixtures) returns a GeneratedArtifact whose finding_id and control already equal the ones the pipeline asked for, so the fields are never observed being trusted. tests/test_correlate.py tests coverage_key in is -- [ ] **MEDIUM** `schemas.py:157` — `RemediationPlan.kind` / `package` / `fixed_version` are on the remediate agent's response_model, so a repo-scan cure can be relabelled a dependency upgrade and pr.py reports a model-invented package version as the fix +- [x] **MEDIUM** `schemas.py:157` — `RemediationPlan.kind` / `package` / `fixed_version` are on the remediate agent's response_model, so a repo-scan cure can be relabelled a dependency upgrade and pr.py reports a model-invented package version as the fix - *Fails when:* pipeline.py:492 appends `remediate.run(...)` results unchanged. A model that decides the cure is an upgrade sets kind='dependency_upgrade' and fills package/ecosystem/fixed_version from memory — no OSV lookup happens anywhere on a repo scan. Then: summary['code_fix_prs'] and metrics.synthesize.code_fix_prs drop to 0 while a complete patched file sits in remediations.json, the report hero renders ' - *Repro:* `/Users/d.henley/demos/virtual-patch-copilot/.venv/bin/python /private/tmp/claude-502/-Users-d-henley-demos-virtual-patch-copilot/fa4d9adf-b050-4d9d-911d-2130d7c6285b/scratchpad/rep` - *Why the suite misses it:* Every dependency_upgrade in the suite is built by inputs/cve.py or inputs/deps.py, where code fills the fields from OSV; every repo-path remediation fixture leaves kind at its 'code_fix' default. The tests therefore only ever see the two consistent combinations and never the model-labelled one, so t diff --git a/src/vpcopilot/pipeline.py b/src/vpcopilot/pipeline.py index 72ee65c..ef41384 100644 --- a/src/vpcopilot/pipeline.py +++ b/src/vpcopilot/pipeline.py @@ -366,6 +366,44 @@ def _triage(ch): for ds in ex.map(_triage, chunks): decisions.extend(ds) # extend, never reassign: the forced ones are already in + # AGENTS REASON, CODE ACTS — reconcile the decision set against the verified set, in code. + # + # `TriageDecision.finding_id` is filled by the TRIAGE AGENT, and every downstream stage keys + # off it. Nothing checked that the ids the model returned were the ids it was given, so: + # * a finding the agent OMITTED was dropped silently — no band-aid, no ledger entry, no + # log line. Measured: three verified findings in, one band-aid out, nothing said. + # * a finding the agent RENAMED produced a PHANTOM ledger row keyed on an id no finding + # has, carrying `title: null, severity: null, file: null`. + # Both are the J5 defect's shape: a model-supplied value trusted as a code-set fact. A + # verified vulnerability silently losing its mitigation is the worst outcome this pipeline + # has, so it is reconciled here rather than hoped for in a prompt. + seen: set[str] = set() + kept: list = [] + for d in decisions: + if d.finding_id not in by_id: + log(f" ⚠ triage returned a decision for '{d.finding_id}', which is not one of the " + f"{len(by_id)} verified findings — discarding it (the agent renamed or invented " + f"an id; a phantom ledger entry is worse than a missing one)") + continue + if d.finding_id in seen: + log(f" ⚠ triage returned two decisions for '{d.finding_id}' — keeping the first") + continue + seen.add(d.finding_id) + kept.append(d) + for f in verified: + if f.id in seen: + continue + # Fail CLOSED and LOUD, exactly as verify does when it cannot parse a verdict. A + # code-cure-only decision is the safe default: it never claims a band-aid exists. + log(f" ⚠ triage returned NO decision for verified finding '{f.id}' ({f.title[:50]}) — " + f"routing it to code-cure-only rather than dropping it silently") + from .schemas import TriageDecision + kept.append(TriageDecision( + finding_id=f.id, bandaids=[], no_bandaid=True, + residual_risk="triage returned no decision for this finding — no band-aid was " + "chosen, so this vulnerability is UNMITIGATED at the edge")) + decisions = kept + # A2: derive validation probes BEFORE generate, so each band-aid is built against the # finding's CONCRETE exploit (exact method + full path) and spares its legit request. bandaided = [by_id[d.finding_id] for d in decisions if not d.no_bandaid and d.finding_id in by_id] @@ -421,6 +459,16 @@ def _try_bandaids(d, f, options, *, only_one: bool) -> int: f"{e} — code fix only") continue made += len(arts) + # AGENTS REASON, CODE ACTS. `GeneratedArtifact.finding_id` and `.control` are on the + # generate agent's response_model, so they are whatever the MODEL echoed back — but + # this call site already KNOWS both: we asked for `b.control` on finding `f`. They + # were written verbatim into policies.json, which is the policy->finding index every + # apply / refine / simulate / emit / backfill path keys off, so a model that echoed + # a different id filed the band-aid against the wrong vulnerability — or against one + # that does not exist. Overwritten unconditionally, like `Finding.file` and the J5 + # classification fields, because a fact code holds must never be taken from a model. + for a in arts: + a.finding_id, a.control = d.finding_id, b.control for a in arts: # A3/A9: lint the consumed-spec controls now; refiner corrects at apply iss = lint_generated_spec(a.control.value, a.spec, exploit) if iss: @@ -486,7 +534,24 @@ def _try_bandaids(d, f, options, *, only_one: bool) -> int: pass elif draft_code_fixes: def _remediate(f): - return remediate.run(h, f, file_raw.get(f.file, "")) + r = remediate.run(h, f, file_raw.get(f.file, "")) + # AGENTS REASON, CODE ACTS. `kind` and the package/version fields are on the + # remediate agent's response_model, and `RemediationPlan`'s own docstring says they + # are "filled from OSV by code, never by a model" — but nothing enforced it. A + # repo-scan cure relabelled `dependency_upgrade` makes `pr.py` skip writing the + # patch and report a model-INVENTED "upgrade to " instead, which + # is a version number an operator acts on directly. + # + # This branch handles findings from SOURCE, where the answer is known: there is a + # file to patch, so it is a code_fix. The advisory path (`forced_remediations`) sets + # these from OSV in code and never reaches here. + r.finding_id = f.id + if r.kind != "code_fix": + log(f" ⚠ remediate labelled the cure for {f.id} '{r.kind}', but this finding " + f"came from source, not an advisory — recording it as a code fix") + r.kind = "code_fix" + r.package = r.ecosystem = r.vulnerable_range = r.fixed_version = "" + return r with ThreadPoolExecutor(max_workers=concurrency) as ex: remediations += list(ex.map(_remediate, to_remediate)) diff --git a/tests/test_model_supplied_keys.py b/tests/test_model_supplied_keys.py new file mode 100644 index 0000000..cc3aeab --- /dev/null +++ b/tests/test_model_supplied_keys.py @@ -0,0 +1,140 @@ +"""Model-supplied values that code then trusted as facts. + +From the 2026-08-04 deep-dive review, and the same shape as the J5 defect: a field lives on a +pydantic model that is ALSO an agent's `response_model`, so the model can fill it — and downstream +code treats it as something it established itself. + +J5 looked like a one-off. It was not: three more instances, all on the identifier that joins one +pipeline stage to the next. A verified vulnerability silently losing its mitigation is the worst +outcome this pipeline has, so these are reconciled in code rather than hoped for in a prompt. +""" +from __future__ import annotations + +import json +import re +import sys +from pathlib import Path + +import pytest + +sys.path.insert(0, str(Path(__file__).resolve().parent)) +from conftest import FakeHarness # noqa: E402 + +from vpcopilot import pipeline # noqa: E402 +from vpcopilot.schemas import ( # noqa: E402 + BandaidOption, Control, Coverage, Finding, FindingList, TriageBatch, TriageDecision, Verdict, +) + + +def _finding(fid, cls="sqli"): + return Finding(id=fid, title=f"t{fid}", vuln_class=cls, severity="high", + description="d", exploit_sketch="e", file="app.py") + + +def _decision(fid): + return TriageDecision( + finding_id=fid, no_bandaid=False, residual_risk="r", + bandaids=[BandaidOption(control=Control("service_policy"), coverage=Coverage("full"), + recommended=True, rationale="x")]) + + +@pytest.fixture +def run(monkeypatch, tmp_path): + """One source file, so `discover` is called exactly once and finding ids are not + collision-suffixed — a three-file fixture makes the fake return the same ids repeatedly and + the suffixing masks what is under test.""" + def go(triage_decisions, findings=("f1", "f2", "f3")): + # Distinct vuln_class per finding: A6's dedup collapses same-file/same-class/same-line + # findings into one, which would silently reduce the fixture to a single finding and make + # every assertion below vacuous. + classes = ["sqli", "xss", "ssrf", "mass_assignment"] + fl = FindingList(findings=[_finding(f, classes[i % len(classes)]) + for i, f in enumerate(findings)]) + fake = FakeHarness({ + "discover": fl, + "verify": lambda s, u: Verdict( + finding_id=re.search(r"\b(f\d+)\b", u).group(1), + is_real=True, confidence=0.9, rationale="r"), + "triage": lambda s, u: TriageBatch(decisions=triage_decisions), + }) + monkeypatch.setattr(pipeline, "Harness", lambda *a, **k: fake) + repo = tmp_path / "repo" + repo.mkdir(exist_ok=True) + (repo / "app.py").write_text("q = 'SELECT ' + u\n") + out = tmp_path / "out" + logs: list[str] = [] + summary = pipeline.run_pipeline(str(repo), out_dir=str(out), log=logs.append, + draft_code_fixes=False) + ledger = json.loads((out / "ledger.json").read_text()) + return summary, ledger, logs + return go + + +def test_a_verified_finding_the_triage_agent_omits_is_never_dropped_silently(run): + """The worst one. `for d in decisions: f = by_id.get(d.finding_id)` iterated the AGENT's list, + so a finding the agent simply did not mention got no band-aid, no ledger entry and no log + line. Measured before the fix: three verified findings in, one ledger entry out, nothing said. + + It is now routed to code-cure-only — fail CLOSED, exactly as verify does with an unparseable + verdict — because a decision that claims no band-aid exists is safe, and silence is not.""" + summary, ledger, logs = run([_decision("f1")]) + assert summary["verified"] == 3 + assert sorted(ledger) == ["f1", "f2", "f3"], \ + f"a verified finding vanished between triage and the ledger: {sorted(ledger)}" + for fid in ("f2", "f3"): + assert any(f"NO decision for verified finding '{fid}'" in x for x in logs), \ + f"{fid} was dropped without a word" + assert ledger["f2"].get("mitigation") is None, "an omitted finding must not claim a band-aid" + + +def test_an_id_the_triage_agent_invents_does_not_create_a_phantom_ledger_entry(run): + """The inverse. A renamed id produced a ledger row keyed on an id no finding has, carrying + `title: null, severity: null, file: null` — a row about nothing, in the audit trail.""" + _, ledger, logs = run([_decision("f1"), _decision("sqli-001")]) + assert "sqli-001" not in ledger, "a model-invented id reached the ledger" + assert any("not one of the 3 verified findings" in x for x in logs), \ + "the invented id was discarded silently — it must be reported" + assert all(ledger[k].get("title") for k in ledger), "a ledger entry with no title is a phantom" + + +def test_a_duplicate_decision_does_not_double_count(run): + """Two decisions for the same finding used to mean two passes over it.""" + _, ledger, logs = run([_decision("f1"), _decision("f1")]) + assert sorted(ledger) == ["f1", "f2", "f3"] + assert any("two decisions for 'f1'" in x for x in logs) + + +def test_the_normal_case_produces_no_warnings(run): + """The fix must not cry wolf: when the agent returns exactly what it was given, the + reconciliation is silent. Otherwise every real run would train the operator to ignore it.""" + _, ledger, logs = run([_decision(f) for f in ("f1", "f2", "f3")]) + assert sorted(ledger) == ["f1", "f2", "f3"] + noisy = [x for x in logs if "triage returned" in x] + assert not noisy, f"the reconciliation warned on a clean run: {noisy}" + + +def test_the_generated_policy_index_is_written_by_code_not_by_the_model(): + """`GeneratedArtifact.finding_id` and `.control` are on the generate agent's response_model, + but the call site already KNOWS both — it asked for that control on that finding. They are the + policy->finding index every apply / refine / simulate / emit / backfill path keys off, so a + model that echoed a different id filed the band-aid against the wrong vulnerability. + + Asserted on the source, because the overwrite is the guarantee.""" + src = (Path(__file__).resolve().parents[1] / "src/vpcopilot/pipeline.py").read_text() + assert re.search(r"a\.finding_id,\s*a\.control\s*=\s*d\.finding_id,\s*b\.control", src), \ + "the generated artifact's identifiers are still whatever the model returned" + + +def test_a_source_finding_cannot_be_relabelled_a_dependency_upgrade(): + """`RemediationPlan.kind` and the package/version fields are on the remediate agent's + response_model. The class docstring says they are "filled from OSV by code, never by a model" — + and nothing enforced it. A repo-scan cure relabelled `dependency_upgrade` makes `pr.py` skip + writing the patch and report a model-INVENTED "upgrade to ", which is a version + number an operator acts on directly.""" + src = (Path(__file__).resolve().parents[1] / "src/vpcopilot/pipeline.py").read_text() + body = src[src.index("def _remediate(f):"):] + body = body[:body.index("return r") + 8] + assert 'r.kind = "code_fix"' in body, "the model still decides what kind of cure this is" + assert "r.package = r.ecosystem = r.vulnerable_range = r.fixed_version" in body, \ + "a model-invented package/version can still reach the operator" + assert "r.finding_id = f.id" in body, "the cure is still keyed on the model's echo of the id"