From 3ee30df696a9bb5017033b80bf125ddafcc33362 Mon Sep 17 00:00:00 2001 From: franciszver <17106076+franciszver@users.noreply.github.com> Date: Sat, 25 Jul 2026 20:29:21 -0700 Subject: [PATCH 01/12] test(P3.31): red-first -- durable pending-triage surface (issue #63) Adds failing tests proving the gap: a single-iteration run's own events never reach the exported action log (no post-loop export), a report left pending by one DocumentationAgent instance cannot be approved by a fresh one pointed at the same reports_dir, and no durable pending-triage count exists. Bundles the small additive observability_snapshot.schema.json field (pending_human_triage_count, optional, v1-additive per contracts/README.md) and its already-passing tests -- the two remaining red tests (documentation-agent persistence, campaign post-loop export) are fixed in the next commit. 5 failing: tests/redteam/test_campaign.py::test_post_loop_action_log_export_includes_last_iterations_own_events tests/redteam/test_documentation_agent.py::test_pending_report_persisted_with_suffix_until_approved tests/redteam/test_documentation_agent.py::test_pending_report_persisted_by_one_agent_is_approvable_by_a_fresh_instance tests/redteam/test_documentation_agent.py::test_stale_pending_file_dropped_once_filed_exists tests/redteam/test_documentation_agent.py::test_corrupt_persisted_report_raises_loudly_not_silently_ignored Refs #63 --- contracts/README.md | 7 ++ .../v1/observability_snapshot.schema.json | 1 + redteam/observability/__init__.py | 3 +- redteam/observability/findings.py | 23 +++++ redteam/observability/snapshot.py | 7 +- tests/redteam/test_campaign.py | 40 +++++++++ tests/redteam/test_documentation_agent.py | 85 ++++++++++++++++++- tests/redteam/test_observability.py | 50 +++++++++++ 8 files changed, 210 insertions(+), 6 deletions(-) diff --git a/contracts/README.md b/contracts/README.md index a9e74dc..cba2d68 100644 --- a/contracts/README.md +++ b/contracts/README.md @@ -28,6 +28,13 @@ ad hoc payload shape once this contract exists for their edge. No `v2/` exists yet; nothing has broken compatibility since this issue's initial cut. +**Log:** + +- Issue #63: `observability_snapshot.schema.json` gained an optional + `pending_human_triage_count` property (durable count of reports still + awaiting human triage). Not added to `required`, so pre-#63 producers and + consumers stay valid — additive, stays `v1`. + ## Schemas (one per edge in ARCHITECTURE.md §2) | Schema | Edge | Notes | diff --git a/contracts/v1/observability_snapshot.schema.json b/contracts/v1/observability_snapshot.schema.json index 08a1351..50d92c0 100644 --- a/contracts/v1/observability_snapshot.schema.json +++ b/contracts/v1/observability_snapshot.schema.json @@ -32,6 +32,7 @@ } }, "open_high_sev_count": { "type": "integer", "minimum": 0 }, + "pending_human_triage_count": { "type": "integer", "minimum": 0, "description": "Reports awaiting human triage right now (issue #63) -- additive v1 field, not in 'required' so pre-#63 producers/consumers stay valid." }, "cost": { "type": "object", "additionalProperties": false, diff --git a/redteam/observability/__init__.py b/redteam/observability/__init__.py index fbcc260..2e59606 100644 --- a/redteam/observability/__init__.py +++ b/redteam/observability/__init__.py @@ -26,7 +26,7 @@ from .action_log import ActionLog, ActionLogError from .coverage import CategoryCoverage, compute_coverage, coverage_fractions from .cost import CostSummary, compute_cost, draw_gap_seconds -from .findings import HIGH_SEVERITIES, open_high_sev_count, status_counts +from .findings import HIGH_SEVERITIES, open_high_sev_count, pending_human_triage_count, status_counts from .runs import SuiteRunLog, SuiteRunLogError from .snapshot import SCHEMA_VERSION, emit_snapshot, new_snapshot_id from .trend import TRENDS, resilience_trend @@ -42,6 +42,7 @@ "draw_gap_seconds", "HIGH_SEVERITIES", "open_high_sev_count", + "pending_human_triage_count", "status_counts", "SuiteRunLog", "SuiteRunLogError", diff --git a/redteam/observability/findings.py b/redteam/observability/findings.py index fe21c73..7c5c0a2 100644 --- a/redteam/observability/findings.py +++ b/redteam/observability/findings.py @@ -46,3 +46,26 @@ def open_high_sev_count(db: ExploitDB, vuln_reports: Sequence[Mapping[str, Any]] if exploit is not None and exploit["status"] == "open": count += 1 return count + + +def pending_human_triage_count(vuln_reports: Sequence[Mapping[str, Any]] = ()) -> int: + """How many of ``vuln_reports`` are still awaiting human triage (issue + #63) -- the durable observability-snapshot answer to "is anything + sitting in the human-approval gate right now?" + + A report counts as pending here if it required the human gate + (``requires_human_gate: True``) AND has not yet been approved + (``approved_by`` absent) -- this is deliberately independent of any + non-contract "status" key a caller happens to have attached (e.g. + ``redteam.campaign.run_campaign``'s ``{**report, "status": ...}``): it + works identically for reports sourced from + ``DocumentationAgent.all_pending()``/``get_pending()`` (which never + carry a "status" key at all) and from a campaign run's + ``all_vuln_reports`` list alike. ``()`` -- no reports known -- yields 0, + the same honest-default convention ``open_high_sev_count`` uses. + """ + return sum( + 1 + for report in vuln_reports + if report.get("requires_human_gate") and not report.get("approved_by") + ) diff --git a/redteam/observability/snapshot.py b/redteam/observability/snapshot.py index 4175945..5d8d481 100644 --- a/redteam/observability/snapshot.py +++ b/redteam/observability/snapshot.py @@ -3,7 +3,9 @@ Produces exactly the contract-shaped dict -- ``schema_version``, ``snapshot_id``, ``generated_at``, ``coverage_by_category`` (fractions), -``open_high_sev_count``, ``cost``, ``action_log_ref`` -- consumed +``open_high_sev_count``, ``pending_human_triage_count`` (issue #63, additive +v1 field -- ``redteam.observability.findings.pending_human_triage_count``), +``cost``, ``action_log_ref`` -- consumed programmatically by the Orchestrator (P3.8) to decide what the Red Team attacks next and when to throttle draws, not rendered only for a human. The contract is deliberately narrow (``additionalProperties: false``), so @@ -29,7 +31,7 @@ from .action_log import ActionLog from .coverage import compute_coverage, coverage_fractions from .cost import compute_cost -from .findings import open_high_sev_count +from .findings import open_high_sev_count, pending_human_triage_count SCHEMA_VERSION = "1.0.0" @@ -69,6 +71,7 @@ def emit_snapshot( "generated_at": generated_at or now_iso(), "coverage_by_category": coverage_fractions(coverage), "open_high_sev_count": open_high_sev_count(db, vuln_reports), + "pending_human_triage_count": pending_human_triage_count(vuln_reports), "cost": cost.as_contract_cost(), "action_log_ref": str(action_log_ref), } diff --git a/tests/redteam/test_campaign.py b/tests/redteam/test_campaign.py index 6579470..c2b99f5 100644 --- a/tests/redteam/test_campaign.py +++ b/tests/redteam/test_campaign.py @@ -11,6 +11,7 @@ from __future__ import annotations import dataclasses +import json from pathlib import Path import pytest @@ -591,3 +592,42 @@ def test_max_iterations_must_be_positive(tmp_path): recordings_dir=recordings_dir, snapshot_fn=lambda: _full_coverage_snapshot("denial_of_service"), ) + + +def test_post_loop_action_log_export_includes_last_iterations_own_events(tmp_path): + """Issue #63: ``ActionLog.export_jsonl`` was only called from + ``emit_snapshot``, itself only called at the TOP of each iteration -- so + a single-iteration run's own events (``directive_issued``, + ``attempt_generated``, ``exploit_recorded``, ``vuln_report_filed``, ...), + all appended AFTER that one top-of-loop snapshot call, never reached the + exported jsonl at all for ``--iterations 1``. A post-loop export must + flush them before ``run_campaign`` returns.""" + recordings_dir = tmp_path / "recordings" + db, action_log, documentation, judge, red_team, orchestrator = _new_agents(recordings_dir) + action_log_ref = tmp_path / "action_log.jsonl" + + run_campaign( + orchestrator=orchestrator, + red_team=red_team, + judge=judge, + documentation=documentation, + db=db, + action_log=action_log, + action_log_ref=action_log_ref, + cases=[_NORMAL_NON_FP_CASE, AUTHZ_CASE], + target_client=lambda attempt: _vulnerable_response(), + max_iterations=1, + recordings_dir=recordings_dir, + # An injected snapshot_fn (as every test above uses) never calls + # emit_snapshot/export_jsonl itself -- so today, NOTHING exports + # action_log_ref at all for this run. That is the bug. + snapshot_fn=lambda: _full_coverage_snapshot("tool_misuse"), + ) + + assert action_log_ref.exists(), "run_campaign never exported the action log at all" + exported_lines = action_log_ref.read_text(encoding="utf-8").splitlines() + all_events = action_log.query() + assert len(exported_lines) == len(all_events) + exported_event_types = {json.loads(line)["event_type"] for line in exported_lines} + assert "exploit_recorded" in exported_event_types + assert "vuln_report_filed" in exported_event_types diff --git a/tests/redteam/test_documentation_agent.py b/tests/redteam/test_documentation_agent.py index 6dfa54b..7e89379 100644 --- a/tests/redteam/test_documentation_agent.py +++ b/tests/redteam/test_documentation_agent.py @@ -227,14 +227,93 @@ def test_reports_persisted_to_reports_dir(tmp_path): assert on_disk["report_id"] == "VULN-0002" -def test_pending_critical_report_not_persisted_until_approved(tmp_path): +def test_pending_report_persisted_with_suffix_until_approved(tmp_path): + """Issue #63: a pending report now gets a durable surface of its own -- + ``.pending-human-approval.json`` -- rather than living only + in memory (the prior behavior this test used to pin).""" agent = DocumentationAgent(reports_dir=tmp_path) agent.file_report(CRITICAL_EXPLOIT) - assert list(tmp_path.glob("*.json")) == [] + + pending_files = list(tmp_path.glob("*.pending-human-approval.json")) + assert len(pending_files) == 1 + assert pending_files[0].name == "VULN-0001.pending-human-approval.json" + on_disk = json.loads(pending_files[0].read_text(encoding="utf-8")) + assert on_disk["exploit_id"] == "EXP-0001" + assert on_disk["requires_human_gate"] is True + assert "approved_by" not in on_disk + # Not yet filed -- no VULN-0001.json (only the pending-suffixed file). + assert not (tmp_path / "VULN-0001.json").exists() agent.approve("EXP-0001") - written = list(tmp_path.glob("*.json")) + + # The pending artifact is gone; a filed one takes its place. + assert list(tmp_path.glob("*.pending-human-approval.json")) == [] + written = [p for p in tmp_path.glob("*.json") if not p.name.endswith(".pending-human-approval.json")] assert len(written) == 1 + assert written[0].name == "VULN-0001.json" + + +def test_pending_report_persisted_by_one_agent_is_approvable_by_a_fresh_instance(tmp_path): + """The heart of issue #63/#66: a report left pending by one process must + be approvable by a SEPARATE later process/instance pointed at the same + ``reports_dir`` -- no bespoke reconstruction script (contrast + ``tools/approve_vuln_0004.py``, which had to reconstruct the exploit + record from scratch because ``_pending`` was in-memory only).""" + filer = DocumentationAgent(reports_dir=tmp_path) + filer.file_report(CRITICAL_EXPLOIT) + del filer # simulate the filing process having exited + + approver = DocumentationAgent(reports_dir=tmp_path) # a fresh instance/"process" + assert approver.get_pending("EXP-0001") is not None + assert approver.get_filed("EXP-0001") is None + + filed = approver.approve("EXP-0001", approved_by="owner") + + assert filed["status"] == "filed" + assert filed["exploit_id"] == "EXP-0001" + assert filed["approved_by"] == "owner" + assert approver.get_pending("EXP-0001") is None + assert approver.get_filed("EXP-0001") is not None + + on_disk = json.loads((tmp_path / "VULN-0001.json").read_text(encoding="utf-8")) + assert on_disk["approved_by"] == "owner" + assert on_disk["observed"] == CRITICAL_EXPLOIT["minimal_repro"]["observed"] + + +def test_stale_pending_file_dropped_once_filed_exists(tmp_path): + """Defensive recovery: if a pending-suffixed file is somehow still on + disk for an exploit_id that ALSO has a filed report (e.g. the pending + file's ``unlink`` failed right after the filed file was written during + ``approve``), loading must never re-offer that exploit_id for approval + -- the filed report wins, the stale pending duplicate is dropped.""" + agent = DocumentationAgent(reports_dir=tmp_path) + agent.file_report(CRITICAL_EXPLOIT) + agent.approve("EXP-0001") + del agent + + # Recreate a stale pending artifact next to the already-filed one. + stale_pending = {**json.loads((tmp_path / "VULN-0001.json").read_text(encoding="utf-8"))} + stale_pending.pop("approved_at") + stale_pending.pop("approved_by") + (tmp_path / "VULN-0001.pending-human-approval.json").write_text( + json.dumps(stale_pending, indent=2), encoding="utf-8" + ) + + reloaded = DocumentationAgent(reports_dir=tmp_path) + assert reloaded.get_pending("EXP-0001") is None # not re-offered + assert reloaded.get_filed("EXP-0001") is not None + with pytest.raises(DocumentationAgentError): + reloaded.approve("EXP-0001") + + +def test_corrupt_persisted_report_raises_loudly_not_silently_ignored(tmp_path): + """A reports_dir this module can't parse must fail loudly at load time, + not silently lose the pending report it's supposed to make durable.""" + (tmp_path / "VULN-0009.pending-human-approval.json").write_text( + "not json", encoding="utf-8" + ) + with pytest.raises(DocumentationAgentError): + DocumentationAgent(reports_dir=tmp_path) def test_malformed_exploit_record_raises_documentation_agent_error_not_key_error(): diff --git a/tests/redteam/test_observability.py b/tests/redteam/test_observability.py index ff8c67b..ccfebea 100644 --- a/tests/redteam/test_observability.py +++ b/tests/redteam/test_observability.py @@ -26,6 +26,7 @@ compute_cost, emit_snapshot, open_high_sev_count, + pending_human_triage_count, resilience_trend, status_counts, ) @@ -148,6 +149,30 @@ def test_open_high_sev_count_needs_a_matching_vuln_report(): assert open_high_sev_count(db, [report]) == 0 # resolved, no longer "open" +def test_pending_human_triage_count_needs_the_gate_and_no_approval(): + """Issue #63: a durable count of reports still awaiting human triage -- + independent of any non-contract 'status' key, so it works the same + whether ``vuln_reports`` came from ``DocumentationAgent.all_pending()`` + (no 'status' key at all) or from ``run_campaign``'s + ``all_vuln_reports`` (which does carry one).""" + assert pending_human_triage_count([]) == 0 # honest zero + + pending = {"exploit_id": "EXP-0001", "requires_human_gate": True} + assert pending_human_triage_count([pending]) == 1 + + not_gated = {"exploit_id": "EXP-0002", "requires_human_gate": False} + assert pending_human_triage_count([not_gated]) == 0 + + already_approved = { + "exploit_id": "EXP-0003", + "requires_human_gate": True, + "approved_by": "owner", + } + assert pending_human_triage_count([already_approved]) == 0 + + assert pending_human_triage_count([pending, not_gated, already_approved]) == 1 + + # -- resilience trend ----------------------------------------------------------- @@ -307,6 +332,31 @@ def test_emit_snapshot_open_high_sev_wired_through(tmp_path): assert snapshot["open_high_sev_count"] == 1 +def test_emit_snapshot_pending_human_triage_count_wired_through(tmp_path): + """Issue #63: the snapshot the Orchestrator (and any durable-observability + reader) sees carries a live pending-triage count, not just open-high-sev.""" + db = ExploitDB(":memory:") + log = ActionLog(":memory:") + ref = tmp_path / "action_log.jsonl" + pending_report = {"exploit_id": "EXP-0001", "severity": "critical", "requires_human_gate": True} + approved_report = { + "exploit_id": "EXP-0002", + "severity": "critical", + "requires_human_gate": True, + "approved_by": "owner", + } + + snapshot = emit_snapshot( + db, + ALL_CASES, + log, + str(ref), + recordings_dir=RECORDINGS_DIR, + vuln_reports=[pending_report, approved_report], + ) + assert snapshot["pending_human_triage_count"] == 1 + + def test_emit_snapshot_is_deterministic_given_explicit_ids(tmp_path): db = ExploitDB(":memory:") log = ActionLog(":memory:") From c7791f9f9338157a0db9f4eb3411295d345460c4 Mon Sep 17 00:00:00 2001 From: franciszver <17106076+franciszver@users.noreply.github.com> Date: Sat, 25 Jul 2026 20:30:27 -0700 Subject: [PATCH 02/12] fix(P3.31): durable pending-report persistence + post-loop action-log export (issue #63) DocumentationAgent now persists pending reports too (.pending-human-approval.json, same suffix convention tools/build_vuln_reports.py already used for VULN-0004) and loads persisted filed/pending reports back from reports_dir on construction -- so approve() is reachable from a fresh process/instance without any per-report reconstruction script. A filed report wins over a stale pending leftover of the same exploit_id on load (self-healing after a crash between persisting the filed file and unlinking the pending one). A reports_dir this module can't parse raises loudly instead of silently losing a pending report. run_campaign now exports the action log unconditionally after the loop, not only at the top of each iteration via emit_snapshot -- a single-iteration run's own events (directive_issued through exploit_recorded/vuln_report_*) previously never reached the exported jsonl at all. Updates docs/ATO_EVIDENCE_PACKET.md and docs/DEMO_SCRIPT.md test-count claims (346->352 with sibling, 240->246 in CI) per tests/test_doc_test_counts.py, and tests/tools/test_approve_vuln_0004_gate.py's now-stale "pending reports are never persisted" assertion. 352 passed (with sibling checkout). Refs #63 --- docs/ATO_EVIDENCE_PACKET.md | 24 +++--- docs/DEMO_SCRIPT.md | 14 ++-- redteam/agents/documentation.py | 98 ++++++++++++++++++++-- redteam/campaign.py | 15 ++++ tests/tools/test_approve_vuln_0004_gate.py | 10 ++- 5 files changed, 131 insertions(+), 30 deletions(-) diff --git a/docs/ATO_EVIDENCE_PACKET.md b/docs/ATO_EVIDENCE_PACKET.md index cecd87a..9e7c574 100644 --- a/docs/ATO_EVIDENCE_PACKET.md +++ b/docs/ATO_EVIDENCE_PACKET.md @@ -326,9 +326,9 @@ explicitly documented as an arbitrary placeholder accepted by the target's own insecure-by-default validator (VULN-0001) — "safe to publish as-is" per that document's own text, not a real credential. -`pytest tests/ -q` re-run for this packet: **346 passed** with the sibling +`pytest tests/ -q` re-run for this packet: **352 passed** with the sibling Phase 2 checkout (`../agentforge-2-evidence-agent`, pinned `v2.0.0`) -present locally (confirmed at PR time); **240 passed, 106 skipped** in CI +present locally (confirmed at PR time); **246 passed, 106 skipped** in CI and for anyone without that sibling — CI (`.github/workflows/ci.yml`) does not check it out, so the 100 total sibling-checkout citation cases class-skip cleanly there: 40 `TestTraceCitationsAgainstPinnedTarget` cases @@ -373,7 +373,7 @@ those changes included, not a pre-change baseline. evidence the project has previously demonstrated this discipline under pressure, not as a claim about this PR's own diff (which touches no secret-adjacent files). -- **346 passing tests (240 passed, 106 skipped in CI), no live/network/GPU +- **352 passing tests (246 passed, 106 skipped in CI), no live/network/GPU call in the default suite.** Every test file under `tests/` (`tests/contracts/`, `tests/redteam/`, `tests/test_cases.py`, `tests/test_case_sourceref_relevance.py`, `tests/test_runner_sse.py`, @@ -395,18 +395,18 @@ those changes included, not a pre-change baseline. has moved across PRs that touch test-suite-relevant code (e.g. PR #40's own test plan: "177 passed (unchanged; no test-suite-relevant code touched)" at that point in the repo's history; this PR's own platform - changes plus its expanded citation-verification test set move it to 346 - with the sibling checkout present, or 240 passed / 106 skipped without + changes plus its expanded citation-verification test set move it to 352 + with the sibling checkout present, or 246 passed / 106 skipped without it, §5.1). --- ## 5. Eval-result evidence -### 5.1 The 346-test suite (240 in CI) +### 5.1 The 352-test suite (246 in CI) -`pytest tests/ -q` → **346 passed** with the sibling Phase 2 checkout -present, re-confirmed for this packet (§4.1); **240 passed, 106 skipped** +`pytest tests/ -q` → **352 passed** with the sibling Phase 2 checkout +present, re-confirmed for this packet (§4.1); **246 passed, 106 skipped** in CI (`.github/workflows/ci.yml` does not check out the sibling target) and for any clone lacking it. Organized across `tests/contracts/` (schema + uniqueness constraints), `tests/redteam/` (the six agents + campaign @@ -518,8 +518,8 @@ to approve and nothing already filed. suspected halts new directives; an empty-completion error is skipped, not fatal (this is §6's postmortem subject); `max_iterations` input validation. Test count: 163 baseline → 171 (PR #35's own reported delta; - the repo has since grown to 346 total with the sibling checkout present, - or 240 passed / 106 skipped without it, §5.1). + the repo has since grown to 352 total with the sibling checkout present, + or 246 passed / 106 skipped without it, §5.1). ### 5.4 Load-test numbers @@ -639,8 +639,8 @@ describes — not because it was dramatic. (Mermaid diagram, trust-zone framing), §2 Auth model (platform + target), §3 Versioned dependency list (`requirements-contracts.txt`, contracts versioning, model runtimes), §4 Self-scan results (commands run + process - evidence), §5 Eval-result evidence (346 tests with the sibling checkout - present / 240 passed, 106 skipped in CI, 3 criticals, live-campaign + evidence), §5 Eval-result evidence (352 tests with the sibling checkout + present / 246 passed, 106 skipped in CI, 3 criticals, live-campaign evidence, load-test numbers), §6 Sample incident and postmortem. - **Every section cites a real, already-committed artifact**, not an invented one: `docs/ARCHITECTURE.md`, `docs/THREAT_MODEL.md`, diff --git a/docs/DEMO_SCRIPT.md b/docs/DEMO_SCRIPT.md index bafc20e..b23382a 100644 --- a/docs/DEMO_SCRIPT.md +++ b/docs/DEMO_SCRIPT.md @@ -20,9 +20,9 @@ evidence table this script complements with runnable commands. immediately before and after any live call and confirm VRAM stays flat. - `pytest tests/ -q` green (deterministic — no live/network/GPU call in the default suite; confirmed while writing this doc). The printed count is - environment-dependent: **346 passed** when the sibling Phase 2 checkout + environment-dependent: **352 passed** when the sibling Phase 2 checkout (`../agentforge-2-evidence-agent`, pinned `v2.0.0`) is present locally; - **240 passed, 106 skipped** in CI and for anyone cloning this repo without + **246 passed, 106 skipped** in CI and for anyone cloning this repo without that sibling (the 106 skipped are `TestTraceCitationsAgainstPinnedTarget` (40 cases, `tests/test_dos_input_bound_resolution.py`) plus `TestCitationsAgainstPinnedTargets` (60 cases, @@ -33,7 +33,7 @@ evidence table this script complements with runnable commands. ``` $ pytest tests/ -q -346 passed in 2.38s # with the sibling Phase 2 checkout present +352 passed in 2.38s # with the sibling Phase 2 checkout present ``` --- @@ -334,14 +334,14 @@ here for completeness: CI (`.github/workflows/ci.yml`) runs the deterministic suite — `python -m pytest tests/ -q` — on every push to `main` and on every pull request. CI does not check out the sibling Phase 2 target, so its printed -count is **240 passed, 106 skipped** (the 106 skipped are +count is **246 passed, 106 skipped** (the 106 skipped are `TestTraceCitationsAgainstPinnedTarget` (40, issue #25/#54), `TestCitationsAgainstPinnedTargets` (60, issue #58), and `TestStandingUpTargetPathsExistInPinnedTarget` (6, issue #61), all of which class-skip cleanly when `../agentforge-2-evidence-agent` is absent). Live-model and target-stack runs remain manual, outside CI: every command in this script was run locally against the dev stack while writing this doc, with the sibling -checkout present, giving **346 passed**. `pytest tests/ -q` is still the +checkout present, giving **352 passed**. `pytest tests/ -q` is still the reproducibility bar — re-run it after pulling this branch to confirm -nothing here has drifted: expect **346 passed** if you have the sibling -Phase 2 checkout at `v2.0.0`, or **240 passed, 106 skipped** if you don't. +nothing here has drifted: expect **352 passed** if you have the sibling +Phase 2 checkout at `v2.0.0`, or **246 passed, 106 skipped** if you don't. diff --git a/redteam/agents/documentation.py b/redteam/agents/documentation.py index b1a4305..9bf30c9 100644 --- a/redteam/agents/documentation.py +++ b/redteam/agents/documentation.py @@ -63,15 +63,39 @@ ``build_vuln_report`` is model-optional and side-effect-free -- it just returns a dict, useful directly in tests or a REPL. ``DocumentationAgent`` adds the stateful pieces (validation, the human-approval gate, duplicate -protection) and, if constructed with ``reports_dir``, persists each newly -*filed* report (auto-filed or freshly approved) as -``/.json`` -- a flat-file store is enough here -because reports are terminal, append-only artifacts (unlike the exploit DB, -nothing ever queries "which reports are open" across categories; that's the -Observability Layer's job over ``ExploitDB`` + report severity, see -``redteam/observability/findings.py``). Pending-approval reports are held -only in memory until approved, by design -- they are not yet a filed -artifact. +protection) and, if constructed with ``reports_dir``, persists both filed +AND pending reports: + +- A *filed* report (auto-filed or freshly approved) is written as + ``/.json``. +- A *pending* report (issue #63) is written as + ``/.pending-human-approval.json`` -- the same + suffix convention ``tools/build_vuln_reports.py`` already used for + ``VULN-0004`` before this issue. On approval, the filed file is written + FIRST and the pending file is then removed -- so a crash between the two + steps leaves both on disk rather than neither, and is self-healing (see + below). + +A flat-file store is enough here because reports are terminal, append-only +artifacts (unlike the exploit DB, nothing ever queries "which reports are +open" across categories; that's the Observability Layer's job over +``ExploitDB`` + report severity, see ``redteam/observability/findings.py``). + +**Loading (issue #63/#66).** ``DocumentationAgent.__init__`` reads every +``*.json`` in ``reports_dir`` (if given) back into ``_filed``/``_pending``, +keyed by ``exploit_id``, validating each against the contract. This is what +makes ``approve()`` reachable across a process boundary: a report filed +pending by one process is loaded straight back into ``_pending`` by any +later ``DocumentationAgent(reports_dir=...)`` construction -- no bespoke +per-report reconstruction script needed (contrast +``tools/approve_vuln_0004.py``, written before this fix). If the SAME +``exploit_id`` is present as both a filed report and a stale pending +leftover (the crash case above), the filed report wins and the stale +pending entry is dropped silently from ``_pending`` -- an already-filed +exploit is never re-offered for approval. A file that can't be parsed or +fails schema validation raises ``DocumentationAgentError`` at construction +time rather than being silently skipped -- a reports_dir this module can't +read must fail loudly, not quietly lose a pending report. ## Why the vuln_report contract has no ``minimal_repro``/``recording_ref`` @@ -119,6 +143,10 @@ _REPO_ROOT = Path(__file__).resolve().parents[2] _VULN_REPORT_SCHEMA_PATH = _REPO_ROOT / "contracts" / "v1" / "vuln_report.schema.json" +# Durable-pending filename suffix (issue #63) -- reused from the convention +# ``tools/build_vuln_reports.py`` already established for ``VULN-0004``. +PENDING_SUFFIX = ".pending-human-approval.json" + Narrator = Callable[[Mapping[str, Any], Mapping[str, Any]], Mapping[str, Any]] # Deterministic severity-by-category table. identity_authz and @@ -334,6 +362,36 @@ def __init__( self._reports_dir.mkdir(parents=True, exist_ok=True) self._filed: dict[str, dict[str, Any]] = {} self._pending: dict[str, dict[str, Any]] = {} + self._load_persisted() + + def _load_persisted(self) -> None: + """Load every already-persisted report in ``reports_dir`` back into + ``_filed``/``_pending`` (issue #63) -- see the module docstring's + "Loading" section for the full contract.""" + if self._reports_dir is None: + return + for path in sorted(self._reports_dir.glob("*.json")): + try: + data = json.loads(path.read_text(encoding="utf-8")) + except (OSError, ValueError) as exc: + raise DocumentationAgentError( + f"could not load persisted report {path}: {exc}" + ) from exc + if not isinstance(data, dict): + raise DocumentationAgentError(f"persisted report {path} is not a JSON object") + self._validate(data) + exploit_id = data["exploit_id"] + if path.name.endswith(PENDING_SUFFIX): + self._pending[exploit_id] = dict(data) + else: + self._filed[exploit_id] = dict(data) + # A filed report supersedes a stale pending leftover for the same + # exploit_id (e.g. approve()'s pending-file unlink failed after the + # filed file was already written) -- never re-offer an + # already-filed exploit for approval. + for exploit_id in list(self._pending): + if exploit_id in self._filed: + del self._pending[exploit_id] def _validate(self, report: Mapping[str, Any]) -> None: errors = sorted(self._validator.iter_errors(report), key=lambda e: list(e.path)) @@ -351,11 +409,27 @@ def _reject_if_duplicate(self, exploit_id: str) -> None: ) def _persist(self, report: Mapping[str, Any]) -> None: + """Persist a FILED report as ``.json``.""" if self._reports_dir is None: return path = self._reports_dir / f"{report['report_id']}.json" path.write_text(json.dumps(dict(report), indent=2), encoding="utf-8") + def _persist_pending(self, report: Mapping[str, Any]) -> None: + """Persist a PENDING report as ``.pending-human-approval.json`` + (issue #63) -- this is what makes it survive the filing process + exiting.""" + if self._reports_dir is None: + return + path = self._reports_dir / f"{report['report_id']}{PENDING_SUFFIX}" + path.write_text(json.dumps(dict(report), indent=2), encoding="utf-8") + + def _remove_pending_file(self, report_id: str) -> None: + if self._reports_dir is None: + return + path = self._reports_dir / f"{report_id}{PENDING_SUFFIX}" + path.unlink(missing_ok=True) + def file_report( self, exploit_record: Mapping[str, Any], @@ -386,6 +460,7 @@ def file_report( if report["requires_human_gate"]: self._pending[exploit_id] = report + self._persist_pending(report) return {**report, "status": "pending_human_approval"} self._filed[exploit_id] = report @@ -415,7 +490,12 @@ def approve( report["approved_by"] = approved_by self._validate(report) self._filed[exploit_id] = report + # Write the filed artifact BEFORE removing the pending one: if this + # process dies between the two steps, both files are left on disk + # rather than neither, and _load_persisted's "filed wins" rule + # self-heals the stale leftover on the next load (issue #63). self._persist(report) + self._remove_pending_file(report["report_id"]) return {**report, "status": "filed"} def get_filed(self, exploit_id: str) -> dict[str, Any] | None: diff --git a/redteam/campaign.py b/redteam/campaign.py index f36cae9..03b7755 100644 --- a/redteam/campaign.py +++ b/redteam/campaign.py @@ -476,4 +476,19 @@ def _default_snapshot() -> dict[str, Any]: action_log.append(agent="judge", event_type="judge_drift_suspected", details=exc.error) result.signals.append({"error_type": "judge_drift_suspected", **exc.error}) + # Post-loop export (issue #63): ``emit_snapshot`` (called at the TOP of + # each iteration, in the default ``snapshot_fn`` path) is the only place + # that calls ``action_log.export_jsonl`` -- so every event appended + # AFTER that iteration's own snapshot call (directive_issued through + # vuln_report_filed/pending, regression/drift signals) never reached + # ``action_log_ref`` for the LAST iteration a run makes. For + # ``max_iterations=1`` that is every event the run produced. Exporting + # here, unconditionally, after the loop (whether it ran to + # ``max_iterations`` or broke early on ``budget_exceeded``) guarantees a + # run's own events are never lost, regardless of whether the caller + # injected a fake ``snapshot_fn`` (as every deterministic test in + # ``tests/redteam/test_campaign.py`` does) that never touches + # ``action_log_ref`` at all. + action_log.export_jsonl(action_log_ref) + return result diff --git a/tests/tools/test_approve_vuln_0004_gate.py b/tests/tools/test_approve_vuln_0004_gate.py index 9d957cc..84f57fd 100644 --- a/tests/tools/test_approve_vuln_0004_gate.py +++ b/tests/tools/test_approve_vuln_0004_gate.py @@ -85,5 +85,11 @@ def test_files_pending_when_gate_is_forced(tmp_path: Path): ) assert pre_approval["status"] == "pending_human_approval" - # Pending reports are never persisted -- only approve() persists. - assert list(reports_dir.glob("*.json")) == [] + # Issue #63: pending reports ARE now persisted (durably, with a + # ".pending-human-approval.json" suffix) so they survive the filing + # process exiting -- only the FILED (unsuffixed) artifact still requires + # approve() to exist. + written = list(reports_dir.glob("*.json")) + assert len(written) == 1 + assert written[0].name.endswith(".pending-human-approval.json") + assert not (reports_dir / f"{pre_approval['report_id']}.json").exists() From 4cdb4485b20f0dde8edafdf988a58779b8276462 Mon Sep 17 00:00:00 2001 From: franciszver <17106076+franciszver@users.noreply.github.com> Date: Sat, 25 Jul 2026 20:32:13 -0700 Subject: [PATCH 03/12] test(P3.31): red-first -- CLI approve/list-pending path (issue #63) tools/run_campaign.py's main() took no arguments and had no --approve/--list-pending mode at all -- there was no CLI path to approve a durably-pending report. 6 failing tests calling main(argv) directly (no live model/target, throwaway tmp_path reports_dir/db). 6 failing: tests/tools/test_run_campaign_cli.py::test_approve_requires_reports_dir tests/tools/test_run_campaign_cli.py::test_list_pending_requires_reports_dir tests/tools/test_run_campaign_cli.py::test_cli_lists_and_approves_a_report_left_pending_by_a_separate_process tests/tools/test_run_campaign_cli.py::test_approve_unknown_exploit_id_fails_without_writing tests/tools/test_run_campaign_cli.py::test_approve_refuses_when_pending_report_drifts_from_its_source_exploit_record tests/tools/test_run_campaign_cli.py::test_never_auto_approves_no_default_exploit_id Refs #63 --- tests/tools/test_run_campaign_cli.py | 143 +++++++++++++++++++++++++++ 1 file changed, 143 insertions(+) create mode 100644 tests/tools/test_run_campaign_cli.py diff --git a/tests/tools/test_run_campaign_cli.py b/tests/tools/test_run_campaign_cli.py new file mode 100644 index 0000000..d7d83eb --- /dev/null +++ b/tests/tools/test_run_campaign_cli.py @@ -0,0 +1,143 @@ +"""Red-first: an ``approve`` path reachable from the CLI (issue #63). + +``tools/run_campaign.py`` had no ``approve``/``--list-pending`` mode at all +before this fix -- ``main()`` took no arguments and only understood +``--iterations``. These tests call ``tools.run_campaign.main(argv)`` +directly (never a live model/target, mirroring +``tests/tools/test_build_vuln_reports_nondestructive.py``'s own +``main(argv)`` convention) against a throwaway ``tmp_path`` reports_dir -- +NEVER the real ``docs/vuln_reports/``. +""" + +from __future__ import annotations + +import json + +import pytest + +from redteam.agents.documentation import DocumentationAgent +from tools import run_campaign + +CRITICAL_EXPLOIT = { + "schema_version": "1.0.0", + "exploit_id": "EXP-0001", + "case_id": "identity-authz-garbage-bearer-token", + "attempt_id": "att-0001", + "verdict_id": "ver-0001", + "category": "identity_authz", + "source": "judge", + "confirmed_at": "2026-07-21T10:07:00Z", + "minimal_repro": { + "steps": ["POST /chat with a garbage bearer token", "observe 200 + patient data"], + "expected": "401/403 rejection", + "observed": "200 with PHI", + }, + "recording_ref": "evals/recordings/identity-authz-garbage-bearer-token/20260721T100600Z-draw1.json", +} + + +def test_approve_requires_reports_dir(): + with pytest.raises(SystemExit): + run_campaign.main(["--approve", "EXP-0001"]) + + +def test_list_pending_requires_reports_dir(): + with pytest.raises(SystemExit): + run_campaign.main(["--list-pending"]) + + +def test_cli_lists_and_approves_a_report_left_pending_by_a_separate_process(tmp_path, capsys): + """The end-to-end CLI proof of issue #63/#66: file a pending report with + one DocumentationAgent instance (simulating a prior process), then use + ONLY ``tools/run_campaign.py``'s CLI -- no bespoke script -- to list it + and approve it.""" + reports_dir = tmp_path / "vuln_reports" + filer = DocumentationAgent(reports_dir=reports_dir) + filer.file_report(CRITICAL_EXPLOIT) + del filer # simulate the filing process exiting + + rc = run_campaign.main(["--list-pending", "--reports-dir", str(reports_dir)]) + assert rc == 0 + out = capsys.readouterr().out + assert "pending_human_triage_count=1" in out + assert "EXP-0001" in out + + rc = run_campaign.main( + ["--approve", "EXP-0001", "--reports-dir", str(reports_dir), "--approved-by", "owner"] + ) + assert rc == 0 + out = capsys.readouterr().out + assert "exploit_id=EXP-0001" in out + assert "approved_by=owner" in out + assert "status=filed" in out + + on_disk = json.loads((reports_dir / "VULN-0001.json").read_text(encoding="utf-8")) + assert on_disk["approved_by"] == "owner" + assert not (reports_dir / "VULN-0001.pending-human-approval.json").exists() + + rc = run_campaign.main(["--list-pending", "--reports-dir", str(reports_dir)]) + assert rc == 0 + out = capsys.readouterr().out + assert "pending_human_triage_count=0" in out + + +def test_approve_unknown_exploit_id_fails_without_writing(tmp_path, capsys): + reports_dir = tmp_path / "vuln_reports" + reports_dir.mkdir() + + rc = run_campaign.main(["--approve", "EXP-9999", "--reports-dir", str(reports_dir)]) + assert rc == 1 + err = capsys.readouterr().err + assert "no pending report" in err + assert list(reports_dir.glob("*.json")) == [] + + +def test_approve_refuses_when_pending_report_drifts_from_its_source_exploit_record(tmp_path, capsys): + """Generalized version of ``tools/approve_vuln_0004.py``'s field-for-field + verify-then-approve discipline: when a persisted, durable exploit DB is + available, the CLI must refuse to approve a pending report whose content + does not match what its source exploit record would produce.""" + from redteam.harness.db import ExploitDB + + reports_dir = tmp_path / "vuln_reports" + db_path = tmp_path / "exploits.sqlite3" + + db = ExploitDB(db_path) + db.add_record(CRITICAL_EXPLOIT) + + filer = DocumentationAgent(reports_dir=reports_dir) + filer.file_report(CRITICAL_EXPLOIT) + del filer + + # Tamper with the persisted pending report -- a careless-operator / + # corruption scenario, not a normal filing. + pending_path = reports_dir / "VULN-0001.pending-human-approval.json" + tampered = json.loads(pending_path.read_text(encoding="utf-8")) + tampered["clinical_impact"] = "DOCTORED: nothing to see here" + pending_path.write_text(json.dumps(tampered, indent=2), encoding="utf-8") + + rc = run_campaign.main( + [ + "--approve", + "EXP-0001", + "--reports-dir", + str(reports_dir), + "--db-path", + str(db_path), + ] + ) + assert rc == 1 + err = capsys.readouterr().err + assert "does not match" in err + # Nothing was approved -- the tampered content never became "filed". + assert not (reports_dir / "VULN-0001.json").exists() + + +def test_never_auto_approves_no_default_exploit_id(tmp_path): + """Regression guard: --approve has no default -- there is no flag + combination that approves anything without an explicit exploit_id, and + a bare run (no --approve/--list-pending) never touches approve() at + all.""" + args = run_campaign._parse_args(["--reports-dir", str(tmp_path)]) + assert args.approve is None + assert args.list_pending is False From beeff5f77340fcd5aad28dcc2648567ac63c42a6 Mon Sep 17 00:00:00 2001 From: franciszver <17106076+franciszver@users.noreply.github.com> Date: Sat, 25 Jul 2026 20:32:42 -0700 Subject: [PATCH 04/12] feat(P3.31): CLI approve/list-pending path for durably-pending reports (issue #63) tools/run_campaign.py gains: --list-pending --reports-dir PATH --approve EXPLOIT_ID --reports-dir PATH [--db-path PATH] [--approved-by NAME] Neither touches a live model or target -- both construct a DocumentationAgent(reports_dir=...), which now loads persisted pending reports back from disk (previous commit), so a report left pending by a prior `run` invocation is approvable with no bespoke per-report script. When --db-path names a persisted exploit DB, --approve re-derives the report from the original exploit record via build_vuln_report and refuses (exit 1, nothing approved) on any field-value drift from the persisted pending artifact -- the same verify-then-approve discipline tools/approve_vuln_0004.py established, generalized to any exploit_id. Default `run` behaviour is UNCHANGED: ExploitDB(":memory:") and DocumentationAgent(reports_dir=None) stay the default (a quick demo/smoke run should still leave nothing behind) -- --reports-dir/--db-path are opt-in. A stderr NOTE fires when reports_dir is unset (pending reports from this run won't survive) or when reports_dir is set without db_path (exploit IDs restart at EXP-0001 and may collide with prior durable reports). Updates docs/ATO_EVIDENCE_PACKET.md and docs/DEMO_SCRIPT.md test-count claims (352->358 with sibling, 246->252 in CI) per tests/test_doc_test_counts.py. 358 passed (with sibling checkout). Refs #63 --- docs/ATO_EVIDENCE_PACKET.md | 26 ++--- docs/DEMO_SCRIPT.md | 14 +-- tools/run_campaign.py | 208 +++++++++++++++++++++++++++++++++--- 3 files changed, 215 insertions(+), 33 deletions(-) diff --git a/docs/ATO_EVIDENCE_PACKET.md b/docs/ATO_EVIDENCE_PACKET.md index 9e7c574..151965a 100644 --- a/docs/ATO_EVIDENCE_PACKET.md +++ b/docs/ATO_EVIDENCE_PACKET.md @@ -103,7 +103,7 @@ flowchart TB style ZoneA fill:#3b1f1f,stroke:#e05252,color:#f5e5e5 style ZoneB fill:#1f2a3b,stroke:#5289e0,color:#e5edf5 style Store fill:#1f3b2a,stroke:#52e089,color:#e5f5ec - style TargetBoundary fill:#3b3520,stroke:#e0c552,color:#f5f0e5 + style TargetBoundary fill:#3b3580,stroke:#e0c552,color:#f5f0e5 style Egress fill:#2a1f3b,stroke:#8a52e0,color:#ede5f5 ``` @@ -326,9 +326,9 @@ explicitly documented as an arbitrary placeholder accepted by the target's own insecure-by-default validator (VULN-0001) — "safe to publish as-is" per that document's own text, not a real credential. -`pytest tests/ -q` re-run for this packet: **352 passed** with the sibling +`pytest tests/ -q` re-run for this packet: **358 passed** with the sibling Phase 2 checkout (`../agentforge-2-evidence-agent`, pinned `v2.0.0`) -present locally (confirmed at PR time); **246 passed, 106 skipped** in CI +present locally (confirmed at PR time); **252 passed, 106 skipped** in CI and for anyone without that sibling — CI (`.github/workflows/ci.yml`) does not check it out, so the 100 total sibling-checkout citation cases class-skip cleanly there: 40 `TestTraceCitationsAgainstPinnedTarget` cases @@ -373,7 +373,7 @@ those changes included, not a pre-change baseline. evidence the project has previously demonstrated this discipline under pressure, not as a claim about this PR's own diff (which touches no secret-adjacent files). -- **352 passing tests (246 passed, 106 skipped in CI), no live/network/GPU +- **358 passing tests (252 passed, 106 skipped in CI), no live/network/GPU call in the default suite.** Every test file under `tests/` (`tests/contracts/`, `tests/redteam/`, `tests/test_cases.py`, `tests/test_case_sourceref_relevance.py`, `tests/test_runner_sse.py`, @@ -395,18 +395,18 @@ those changes included, not a pre-change baseline. has moved across PRs that touch test-suite-relevant code (e.g. PR #40's own test plan: "177 passed (unchanged; no test-suite-relevant code touched)" at that point in the repo's history; this PR's own platform - changes plus its expanded citation-verification test set move it to 352 - with the sibling checkout present, or 246 passed / 106 skipped without + changes plus its expanded citation-verification test set move it to 358 + with the sibling checkout present, or 252 passed / 106 skipped without it, §5.1). --- ## 5. Eval-result evidence -### 5.1 The 352-test suite (246 in CI) +### 5.1 The 358-test suite (252 in CI) -`pytest tests/ -q` → **352 passed** with the sibling Phase 2 checkout -present, re-confirmed for this packet (§4.1); **246 passed, 106 skipped** +`pytest tests/ -q` → **358 passed** with the sibling Phase 2 checkout +present, re-confirmed for this packet (§4.1); **252 passed, 106 skipped** in CI (`.github/workflows/ci.yml` does not check out the sibling target) and for any clone lacking it. Organized across `tests/contracts/` (schema + uniqueness constraints), `tests/redteam/` (the six agents + campaign @@ -518,8 +518,8 @@ to approve and nothing already filed. suspected halts new directives; an empty-completion error is skipped, not fatal (this is §6's postmortem subject); `max_iterations` input validation. Test count: 163 baseline → 171 (PR #35's own reported delta; - the repo has since grown to 352 total with the sibling checkout present, - or 246 passed / 106 skipped without it, §5.1). + the repo has since grown to 358 total with the sibling checkout present, + or 252 passed / 106 skipped without it, §5.1). ### 5.4 Load-test numbers @@ -639,8 +639,8 @@ describes — not because it was dramatic. (Mermaid diagram, trust-zone framing), §2 Auth model (platform + target), §3 Versioned dependency list (`requirements-contracts.txt`, contracts versioning, model runtimes), §4 Self-scan results (commands run + process - evidence), §5 Eval-result evidence (352 tests with the sibling checkout - present / 246 passed, 106 skipped in CI, 3 criticals, live-campaign + evidence), §5 Eval-result evidence (358 tests with the sibling checkout + present / 252 passed, 106 skipped in CI, 3 criticals, live-campaign evidence, load-test numbers), §6 Sample incident and postmortem. - **Every section cites a real, already-committed artifact**, not an invented one: `docs/ARCHITECTURE.md`, `docs/THREAT_MODEL.md`, diff --git a/docs/DEMO_SCRIPT.md b/docs/DEMO_SCRIPT.md index b23382a..b82b3fe 100644 --- a/docs/DEMO_SCRIPT.md +++ b/docs/DEMO_SCRIPT.md @@ -20,9 +20,9 @@ evidence table this script complements with runnable commands. immediately before and after any live call and confirm VRAM stays flat. - `pytest tests/ -q` green (deterministic — no live/network/GPU call in the default suite; confirmed while writing this doc). The printed count is - environment-dependent: **352 passed** when the sibling Phase 2 checkout + environment-dependent: **358 passed** when the sibling Phase 2 checkout (`../agentforge-2-evidence-agent`, pinned `v2.0.0`) is present locally; - **246 passed, 106 skipped** in CI and for anyone cloning this repo without + **252 passed, 106 skipped** in CI and for anyone cloning this repo without that sibling (the 106 skipped are `TestTraceCitationsAgainstPinnedTarget` (40 cases, `tests/test_dos_input_bound_resolution.py`) plus `TestCitationsAgainstPinnedTargets` (60 cases, @@ -33,7 +33,7 @@ evidence table this script complements with runnable commands. ``` $ pytest tests/ -q -352 passed in 2.38s # with the sibling Phase 2 checkout present +358 passed in 2.38s # with the sibling Phase 2 checkout present ``` --- @@ -334,14 +334,14 @@ here for completeness: CI (`.github/workflows/ci.yml`) runs the deterministic suite — `python -m pytest tests/ -q` — on every push to `main` and on every pull request. CI does not check out the sibling Phase 2 target, so its printed -count is **246 passed, 106 skipped** (the 106 skipped are +count is **252 passed, 106 skipped** (the 106 skipped are `TestTraceCitationsAgainstPinnedTarget` (40, issue #25/#54), `TestCitationsAgainstPinnedTargets` (60, issue #58), and `TestStandingUpTargetPathsExistInPinnedTarget` (6, issue #61), all of which class-skip cleanly when `../agentforge-2-evidence-agent` is absent). Live-model and target-stack runs remain manual, outside CI: every command in this script was run locally against the dev stack while writing this doc, with the sibling -checkout present, giving **352 passed**. `pytest tests/ -q` is still the +checkout present, giving **358 passed**. `pytest tests/ -q` is still the reproducibility bar — re-run it after pulling this branch to confirm -nothing here has drifted: expect **352 passed** if you have the sibling -Phase 2 checkout at `v2.0.0`, or **246 passed, 106 skipped** if you don't. +nothing here has drifted: expect **358 passed** if you have the sibling +Phase 2 checkout at `v2.0.0`, or **252 passed, 106 skipped** if you don't. diff --git a/tools/run_campaign.py b/tools/run_campaign.py index 1d4c2e8..124f010 100644 --- a/tools/run_campaign.py +++ b/tools/run_campaign.py @@ -5,14 +5,18 @@ CPU-only Red Team generator (ollama, ``num_gpu: 0``) AND the REAL live target (``docker exec`` via ``evals.runner.drive_chat``) -- the exact two things ``tests/redteam/test_campaign.py`` fakes out to stay deterministic. +The ``--approve``/``--list-pending`` modes below (issue #63) are the +exception -- they touch no live model/target and are exercised directly by +``tests/tools/test_run_campaign_cli.py``. ## GPU safety -Run ``nvidia-smi`` yourself immediately before AND after this script and -confirm VRAM stayed flat. ``RedTeamAgent()``'s default ``model_client`` -always calls ollama with ``num_gpu: 0`` (see +Run ``nvidia-smi`` yourself immediately before AND after a ``run`` (the +default mode) and confirm VRAM stayed flat. ``RedTeamAgent()``'s default +``model_client`` always calls ollama with ``num_gpu: 0`` (see ``redteam/agents/red_team.py``'s module docstring) -- this script never -overrides that. +overrides that. ``--approve``/``--list-pending`` never touch ollama or the +target at all. ## Bounds (demo-sized, not a load test) @@ -21,11 +25,55 @@ ``generate_attempts`` -- never more than one draw per directive here). Do NOT raise the cap to run the 100-case load test with this script. +## Durability (issue #63) -- opt in, default unchanged + +By default this script still uses ``ExploitDB(":memory:")`` and +``DocumentationAgent(reports_dir=None)``, exactly as before this issue: a +quick demo/smoke run (``docs/DEMO_SCRIPT.md``) that leaves no files behind +is still the right default for that use case, and silently flipping it +would be an owner-visible behaviour change this issue's brief explicitly +warns against making without saying so. What issue #63 actually fixes +regardless of these flags is that ``run_campaign`` now ALWAYS exports its +own action log after the loop (see ``redteam/campaign.py``), so a +``--iterations 1`` run's own events are never silently dropped even with +the in-memory defaults. + +Pass ``--reports-dir PATH`` to persist vuln reports (filed AND pending) +durably, and ``--db-path PATH`` to persist the exploit DB durably (a +sqlite file, not ``:memory:``) -- **pair the two** if you want exploit IDs +to keep incrementing correctly across runs; ``--reports-dir`` alone with +the in-memory (default) db restarts exploit numbering at ``EXP-0001`` on +every invocation and will collide with (and refuse to re-file over) an +already-persisted report for that same id. + +## Approving a durably-pending report (issue #63/#66) + +A report a PRIOR ``run`` invocation left ``pending_human_approval`` (i.e. +you passed ``--reports-dir`` to that run) can be approved by a separate, +later invocation of this script -- no live model/target, no bespoke +per-report script: + + python tools/run_campaign.py --list-pending --reports-dir PATH + python tools/run_campaign.py --approve EXP-0004 --reports-dir PATH [--db-path PATH] [--approved-by NAME] + +If ``--db-path`` names a persisted (non-``:memory:``) sqlite file that +still holds the original exploit record, the approve path re-derives the +report from that record via ``build_vuln_report`` and refuses (exit 1, +nothing approved) if it does not match the persisted pending report +field-for-field (ignoring ``filed_at``) -- the same verify-then-approve +discipline ``tools/approve_vuln_0004.py`` established, generalized. Without +a matching persisted db record, the cross-check is skipped (a warning is +printed) and the persisted pending report is approved as filed on disk -- +this is strictly safer than the pre-#63 state, where there was no durable +pending artifact to approve at all. + ## Usage - python tools/run_campaign.py [--iterations N] + python tools/run_campaign.py [--iterations N] [--reports-dir PATH] [--db-path PATH] + python tools/run_campaign.py --list-pending --reports-dir PATH + python tools/run_campaign.py --approve EXPLOIT_ID --reports-dir PATH [--db-path PATH] [--approved-by NAME] -Requires: the target stack up (``docker ps`` shows +Requires (``run`` mode only): the target stack up (``docker ps`` shows ``development-easy-agent-1``) and the local ollama server up with ``huihui_ai/qwen2.5-abliterate:7b`` pulled. Prints a summary of every iteration and the final ``CampaignResult``. @@ -38,13 +86,18 @@ import sys import tempfile from pathlib import Path +from typing import Sequence sys.path.insert(0, str(Path(__file__).resolve().parents[1])) from evals.cases.data_exfil_verification_bypass import CASE as DATA_EXFIL_CASE # noqa: E402 from evals.cases.dos_input_bound import CASE as DOS_CASE # noqa: E402 from evals.cases.identity_authz import CASE as AUTHZ_CASE # noqa: E402 -from redteam.agents.documentation import DocumentationAgent # noqa: E402 +from redteam.agents.documentation import ( # noqa: E402 + DocumentationAgent, + DocumentationAgentError, + build_vuln_report, +) from redteam.agents.judge import JudgeAgent # noqa: E402 from redteam.agents.orchestrator import Orchestrator # noqa: E402 from redteam.agents.red_team import RedTeamAgent # noqa: E402 @@ -56,15 +109,118 @@ CASES = [DOS_CASE, AUTHZ_CASE, DATA_EXFIL_CASE] -def main() -> int: - parser = argparse.ArgumentParser(description=__doc__) +def _parse_args(argv: Sequence[str] | None) -> argparse.Namespace: + parser = argparse.ArgumentParser(description=__doc__, formatter_class=argparse.RawDescriptionHelpFormatter) parser.add_argument( "--iterations", type=int, default=3, - help=f"directives to run, capped at {HARD_CAP_ITERATIONS} for this demo entry", + help=f"[run mode] directives to run, capped at {HARD_CAP_ITERATIONS} for this demo entry", + ) + parser.add_argument( + "--reports-dir", + type=Path, + default=None, + help="persist vuln reports (filed AND pending) here instead of keeping them in-memory only", + ) + parser.add_argument( + "--db-path", + type=Path, + default=None, + help="persist the exploit DB here (sqlite file) instead of ':memory:'", + ) + parser.add_argument( + "--approve", + metavar="EXPLOIT_ID", + default=None, + help="[approve mode] approve a durably-pending report by exploit_id; requires --reports-dir", + ) + parser.add_argument( + "--approved-by", + default="owner", + help="[approve mode] approving identity to stamp (default: %(default)s)", + ) + parser.add_argument( + "--list-pending", + action="store_true", + help="[list-pending mode] list reports awaiting human triage under --reports-dir and exit", + ) + args = parser.parse_args(argv) + if args.list_pending and args.reports_dir is None: + parser.error("--list-pending requires --reports-dir") + if args.approve is not None and args.reports_dir is None: + parser.error("--approve requires --reports-dir") + return args + + +def _cmd_list_pending(args: argparse.Namespace) -> int: + documentation = DocumentationAgent(reports_dir=args.reports_dir) + pending = documentation.all_pending() + print(f"pending_human_triage_count={len(pending)} (reports_dir={args.reports_dir})") + for report in pending: + print( + f" exploit_id={report['exploit_id']} report_id={report['report_id']} " + f"severity={report['severity']} filed_at={report['filed_at']}" + ) + return 0 + + +def _cmd_approve(args: argparse.Namespace) -> int: + """Approve a durably-pending report. Never touches a live model/target + -- see the module docstring's "Approving a durably-pending report" + section for the verify-then-approve discipline this reuses from + ``tools/approve_vuln_0004.py``, generalized to any exploit_id/reports_dir + rather than one hardcoded report.""" + documentation = DocumentationAgent(reports_dir=args.reports_dir) + pending = documentation.get_pending(args.approve) + if pending is None: + print( + f"no pending report for exploit_id={args.approve!r} under {args.reports_dir} " + "-- nothing to approve", + file=sys.stderr, + ) + return 1 + + if args.db_path is not None and str(args.db_path) != ":memory:": + db = ExploitDB(args.db_path) + stored = db.get(args.approve) + if stored is None: + print( + f"warning: no exploit record for {args.approve!r} in {args.db_path} " + "-- skipping the field-for-field cross-check and approving the " + "persisted pending report as-is", + file=sys.stderr, + ) + else: + rebuilt = build_vuln_report( + stored["record"], + report_id=pending["report_id"], + filed_at=pending["filed_at"], + force_human_gate=pending["requires_human_gate"], + ) + if rebuilt != pending: + print( + f"refusing to approve {args.approve!r}: the persisted pending report " + "does not match what its source exploit record would produce -- " + f"rebuilt={rebuilt}\npending_on_disk={pending}", + file=sys.stderr, + ) + return 1 + + try: + filed = documentation.approve(args.approve, approved_by=args.approved_by) + except DocumentationAgentError as exc: + print(f"approve failed: {exc}", file=sys.stderr) + return 1 + + print( + f"exploit_id={args.approve} report_id={filed['report_id']} " + f"approved_by={filed['approved_by']} approved_at={filed['approved_at']} status={filed['status']}" ) - args = parser.parse_args() + return 0 + + +def _cmd_run(args: argparse.Namespace) -> int: iterations = min(args.iterations, HARD_CAP_ITERATIONS) if iterations < 1: print(f"--iterations must be >= 1, got {args.iterations}", file=sys.stderr) @@ -73,14 +229,30 @@ def main() -> int: print(f"Running a BOUNDED live campaign: {iterations} directive(s), 1 attempt each.") print("Red Team generator: real ollama, num_gpu=0 (CPU-only). Target: live docker exec.") - db = ExploitDB(":memory:") + db_path = args.db_path if args.db_path is not None else ":memory:" + db = ExploitDB(db_path) action_log = ActionLog(":memory:") - documentation = DocumentationAgent(reports_dir=None) + documentation = DocumentationAgent(reports_dir=args.reports_dir) judge = JudgeAgent() red_team = RedTeamAgent() # default model_client -> real ollama, num_gpu:0 (see module docstring) orchestrator = Orchestrator(max_draws=1) target_client = make_live_target_client() + if args.reports_dir is None: + print( + "NOTE: --reports-dir not set -- any pending report this run files will NOT " + "survive this process exiting (issue #63). Pass --reports-dir PATH to persist " + "it durably, then later run --approve EXPLOIT_ID --reports-dir PATH to approve it.", + file=sys.stderr, + ) + elif str(db_path) == ":memory:": + print( + "NOTE: --reports-dir is set but --db-path is not -- exploit IDs restart at " + "EXP-0001 every run and may collide with an already-persisted report under " + "--reports-dir. Pass --db-path PATH too for a fully durable run.", + file=sys.stderr, + ) + # A scratch path -- deliberately NOT under evals/recordings/ (that # directory is committed replay evidence, not a scratch/log dir; a live # demo run's action-log export shouldn't show up as an untracked file @@ -112,8 +284,18 @@ def main() -> int: print(f"filed_reports={[r['report_id'] for r in result.filed_reports]}") print(f"pending_reports={[r['report_id'] for r in result.pending_reports]}") print(f"signals={json.dumps(result.signals, indent=2)}") + print(f"action_log_ref={action_log_ref}") return 0 +def main(argv: Sequence[str] | None = None) -> int: + args = _parse_args(argv) + if args.list_pending: + return _cmd_list_pending(args) + if args.approve is not None: + return _cmd_approve(args) + return _cmd_run(args) + + if __name__ == "__main__": raise SystemExit(main()) From b0bfeddbf3ba31a1976f5c25a86465d009cc73db Mon Sep 17 00:00:00 2001 From: franciszver <17106076+franciszver@users.noreply.github.com> Date: Sat, 25 Jul 2026 20:33:20 -0700 Subject: [PATCH 05/12] refactor(P3.31): declutter -- fold _persist_pending into _persist(suffix=) _persist and _persist_pending were identical apart from the filename suffix; one method with a suffix keyword covers both call sites. No behavior change -- 358 passed before and after. --- redteam/agents/documentation.py | 20 +++++++------------- 1 file changed, 7 insertions(+), 13 deletions(-) diff --git a/redteam/agents/documentation.py b/redteam/agents/documentation.py index 9bf30c9..01bcea5 100644 --- a/redteam/agents/documentation.py +++ b/redteam/agents/documentation.py @@ -408,20 +408,14 @@ def _reject_if_duplicate(self, exploit_id: str) -> None: "(filed or pending human approval) -- one exploit, one report" ) - def _persist(self, report: Mapping[str, Any]) -> None: - """Persist a FILED report as ``.json``.""" + def _persist(self, report: Mapping[str, Any], *, suffix: str = ".json") -> None: + """Persist a report as ```` -- ``suffix=".json"`` + (the default) for a FILED report, ``suffix=PENDING_SUFFIX`` (issue + #63) for a PENDING one. The latter is what makes a pending report + survive the filing process exiting.""" if self._reports_dir is None: return - path = self._reports_dir / f"{report['report_id']}.json" - path.write_text(json.dumps(dict(report), indent=2), encoding="utf-8") - - def _persist_pending(self, report: Mapping[str, Any]) -> None: - """Persist a PENDING report as ``.pending-human-approval.json`` - (issue #63) -- this is what makes it survive the filing process - exiting.""" - if self._reports_dir is None: - return - path = self._reports_dir / f"{report['report_id']}{PENDING_SUFFIX}" + path = self._reports_dir / f"{report['report_id']}{suffix}" path.write_text(json.dumps(dict(report), indent=2), encoding="utf-8") def _remove_pending_file(self, report_id: str) -> None: @@ -460,7 +454,7 @@ def file_report( if report["requires_human_gate"]: self._pending[exploit_id] = report - self._persist_pending(report) + self._persist(report, suffix=PENDING_SUFFIX) return {**report, "status": "pending_human_approval"} self._filed[exploit_id] = report From 6cd794453bef865e38cf74c20a0967f750610e94 Mon Sep 17 00:00:00 2001 From: franciszver <17106076+franciszver@users.noreply.github.com> Date: Sat, 25 Jul 2026 20:37:26 -0700 Subject: [PATCH 06/12] fix(P3.31 deep-review): approve_vuln_0004.py's re-run flow no longer crashes on an auto-loaded pending report Deep-review (BLOCKER, CONFIRMED) found this diff regressed tools/approve_vuln_0004.py's own designed use case: DocumentationAgent now auto-loads persisted pending reports at construction time, so main()'s DocumentationAgent(reports_dir=_REPORTS_DIR) already has the pending EXP-0004 report loaded by the time _file_pending() re-drives file_report() on it -- colliding with the one-exploit-one-report duplicate-rejection guard and crashing with DocumentationAgentError instead of approving. Reproduced directly (see commit history): a fresh DocumentationAgent pointed at a reports_dir holding a genuinely-pending report raised DocumentationAgentError from _file_pending. Fixed by using the already-loaded pending report directly (documentation.get_pending()) instead of re-filing when it's present; _file_pending stays as a fallback for the (no longer reachable, but harmless) case where it isn't. Also fixed a second-order double-unlink: DocumentationAgent.approve() now removes the persisted pending file itself, so main()'s own _PENDING_PATH.unlink() would FileNotFoundError right after -- made missing_ok=True. Also noted (PRE-EXISTING, not fixed here -- out of scope): the success print's _FILED_PATH.relative_to(_REPO_ROOT) crashes if reports_dir isn't under the repo root, the same class of bug issue #64 already fixed in the sibling tools/build_vuln_report_p3_54.py via _display_path. Flagged in the PR description for a separate pass. New regression test (tests/tools/test_approve_vuln_0004_rerun.py) drives the real main() end-to-end against a scratch reports_dir and asserts it approves cleanly instead of raising, plus a second idempotent run. Updates docs/ATO_EVIDENCE_PACKET.md and docs/DEMO_SCRIPT.md test-count claims (358->359 with sibling, 252->253 in CI). 359 passed (with sibling checkout). Refs #63 --- docs/ATO_EVIDENCE_PACKET.md | 28 ++++----- docs/DEMO_SCRIPT.md | 14 ++--- tests/tools/test_approve_vuln_0004_rerun.py | 64 +++++++++++++++++++++ tools/approve_vuln_0004.py | 37 +++++++++--- 4 files changed, 114 insertions(+), 29 deletions(-) create mode 100644 tests/tools/test_approve_vuln_0004_rerun.py diff --git a/docs/ATO_EVIDENCE_PACKET.md b/docs/ATO_EVIDENCE_PACKET.md index 151965a..8ffd6f4 100644 --- a/docs/ATO_EVIDENCE_PACKET.md +++ b/docs/ATO_EVIDENCE_PACKET.md @@ -100,10 +100,10 @@ flowchart TB ZoneA -.->|"crosses network boundary only to
localhost target, never external"| Egress ZoneB -.->|"zero external calls"| Egress - style ZoneA fill:#3b1f1f,stroke:#e05252,color:#f5e5e5 + style ZoneA fill:#3b1f1f,stroke:#e05253,color:#f5e5e5 style ZoneB fill:#1f2a3b,stroke:#5289e0,color:#e5edf5 style Store fill:#1f3b2a,stroke:#52e089,color:#e5f5ec - style TargetBoundary fill:#3b3580,stroke:#e0c552,color:#f5f0e5 + style TargetBoundary fill:#3b3590,stroke:#e0c552,color:#f5f0e5 style Egress fill:#2a1f3b,stroke:#8a52e0,color:#ede5f5 ``` @@ -326,9 +326,9 @@ explicitly documented as an arbitrary placeholder accepted by the target's own insecure-by-default validator (VULN-0001) — "safe to publish as-is" per that document's own text, not a real credential. -`pytest tests/ -q` re-run for this packet: **358 passed** with the sibling +`pytest tests/ -q` re-run for this packet: **359 passed** with the sibling Phase 2 checkout (`../agentforge-2-evidence-agent`, pinned `v2.0.0`) -present locally (confirmed at PR time); **252 passed, 106 skipped** in CI +present locally (confirmed at PR time); **253 passed, 106 skipped** in CI and for anyone without that sibling — CI (`.github/workflows/ci.yml`) does not check it out, so the 100 total sibling-checkout citation cases class-skip cleanly there: 40 `TestTraceCitationsAgainstPinnedTarget` cases @@ -373,7 +373,7 @@ those changes included, not a pre-change baseline. evidence the project has previously demonstrated this discipline under pressure, not as a claim about this PR's own diff (which touches no secret-adjacent files). -- **358 passing tests (252 passed, 106 skipped in CI), no live/network/GPU +- **359 passing tests (253 passed, 106 skipped in CI), no live/network/GPU call in the default suite.** Every test file under `tests/` (`tests/contracts/`, `tests/redteam/`, `tests/test_cases.py`, `tests/test_case_sourceref_relevance.py`, `tests/test_runner_sse.py`, @@ -395,18 +395,18 @@ those changes included, not a pre-change baseline. has moved across PRs that touch test-suite-relevant code (e.g. PR #40's own test plan: "177 passed (unchanged; no test-suite-relevant code touched)" at that point in the repo's history; this PR's own platform - changes plus its expanded citation-verification test set move it to 358 - with the sibling checkout present, or 252 passed / 106 skipped without + changes plus its expanded citation-verification test set move it to 359 + with the sibling checkout present, or 253 passed / 106 skipped without it, §5.1). --- ## 5. Eval-result evidence -### 5.1 The 358-test suite (252 in CI) +### 5.1 The 359-test suite (253 in CI) -`pytest tests/ -q` → **358 passed** with the sibling Phase 2 checkout -present, re-confirmed for this packet (§4.1); **252 passed, 106 skipped** +`pytest tests/ -q` → **359 passed** with the sibling Phase 2 checkout +present, re-confirmed for this packet (§4.1); **253 passed, 106 skipped** in CI (`.github/workflows/ci.yml` does not check out the sibling target) and for any clone lacking it. Organized across `tests/contracts/` (schema + uniqueness constraints), `tests/redteam/` (the six agents + campaign @@ -518,8 +518,8 @@ to approve and nothing already filed. suspected halts new directives; an empty-completion error is skipped, not fatal (this is §6's postmortem subject); `max_iterations` input validation. Test count: 163 baseline → 171 (PR #35's own reported delta; - the repo has since grown to 358 total with the sibling checkout present, - or 252 passed / 106 skipped without it, §5.1). + the repo has since grown to 359 total with the sibling checkout present, + or 253 passed / 106 skipped without it, §5.1). ### 5.4 Load-test numbers @@ -639,8 +639,8 @@ describes — not because it was dramatic. (Mermaid diagram, trust-zone framing), §2 Auth model (platform + target), §3 Versioned dependency list (`requirements-contracts.txt`, contracts versioning, model runtimes), §4 Self-scan results (commands run + process - evidence), §5 Eval-result evidence (358 tests with the sibling checkout - present / 252 passed, 106 skipped in CI, 3 criticals, live-campaign + evidence), §5 Eval-result evidence (359 tests with the sibling checkout + present / 253 passed, 106 skipped in CI, 3 criticals, live-campaign evidence, load-test numbers), §6 Sample incident and postmortem. - **Every section cites a real, already-committed artifact**, not an invented one: `docs/ARCHITECTURE.md`, `docs/THREAT_MODEL.md`, diff --git a/docs/DEMO_SCRIPT.md b/docs/DEMO_SCRIPT.md index b82b3fe..a80c6e6 100644 --- a/docs/DEMO_SCRIPT.md +++ b/docs/DEMO_SCRIPT.md @@ -20,9 +20,9 @@ evidence table this script complements with runnable commands. immediately before and after any live call and confirm VRAM stays flat. - `pytest tests/ -q` green (deterministic — no live/network/GPU call in the default suite; confirmed while writing this doc). The printed count is - environment-dependent: **358 passed** when the sibling Phase 2 checkout + environment-dependent: **359 passed** when the sibling Phase 2 checkout (`../agentforge-2-evidence-agent`, pinned `v2.0.0`) is present locally; - **252 passed, 106 skipped** in CI and for anyone cloning this repo without + **253 passed, 106 skipped** in CI and for anyone cloning this repo without that sibling (the 106 skipped are `TestTraceCitationsAgainstPinnedTarget` (40 cases, `tests/test_dos_input_bound_resolution.py`) plus `TestCitationsAgainstPinnedTargets` (60 cases, @@ -33,7 +33,7 @@ evidence table this script complements with runnable commands. ``` $ pytest tests/ -q -358 passed in 2.38s # with the sibling Phase 2 checkout present +359 passed in 2.38s # with the sibling Phase 2 checkout present ``` --- @@ -334,14 +334,14 @@ here for completeness: CI (`.github/workflows/ci.yml`) runs the deterministic suite — `python -m pytest tests/ -q` — on every push to `main` and on every pull request. CI does not check out the sibling Phase 2 target, so its printed -count is **252 passed, 106 skipped** (the 106 skipped are +count is **253 passed, 106 skipped** (the 106 skipped are `TestTraceCitationsAgainstPinnedTarget` (40, issue #25/#54), `TestCitationsAgainstPinnedTargets` (60, issue #58), and `TestStandingUpTargetPathsExistInPinnedTarget` (6, issue #61), all of which class-skip cleanly when `../agentforge-2-evidence-agent` is absent). Live-model and target-stack runs remain manual, outside CI: every command in this script was run locally against the dev stack while writing this doc, with the sibling -checkout present, giving **358 passed**. `pytest tests/ -q` is still the +checkout present, giving **359 passed**. `pytest tests/ -q` is still the reproducibility bar — re-run it after pulling this branch to confirm -nothing here has drifted: expect **358 passed** if you have the sibling -Phase 2 checkout at `v2.0.0`, or **252 passed, 106 skipped** if you don't. +nothing here has drifted: expect **359 passed** if you have the sibling +Phase 2 checkout at `v2.0.0`, or **253 passed, 106 skipped** if you don't. diff --git a/tests/tools/test_approve_vuln_0004_rerun.py b/tests/tools/test_approve_vuln_0004_rerun.py new file mode 100644 index 0000000..b7c6fec --- /dev/null +++ b/tests/tools/test_approve_vuln_0004_rerun.py @@ -0,0 +1,64 @@ +"""Deep-review regression test (issue #63 self-fix): ``DocumentationAgent`` +now auto-loads persisted pending reports from ``reports_dir`` at +construction time (see ``redteam/agents/documentation.py``). Before this +test's fix landed, that broke ``tools/approve_vuln_0004.py``'s own designed +re-run flow -- a genuinely-pending, not-yet-approved report -- because +``main()`` constructs a fresh ``DocumentationAgent(reports_dir=_REPORTS_DIR)`` +(which now auto-loads the exact pending report ``_PENDING_PATH.exists()`` +just confirmed is there) and then called ``_file_pending`` -> ``file_report`` +again, colliding with the newly-auto-loaded entry via the +one-exploit-one-report duplicate-rejection guard and crashing with +``DocumentationAgentError`` instead of approving. + +Never touches the real ``docs/vuln_reports/`` -- points every module +constant at a scratch ``tmp_path`` first. +""" + +from __future__ import annotations + +import json +from pathlib import Path + +import pytest + +import tools.approve_vuln_0004 as approve_vuln_0004 +from redteam.agents.documentation import DocumentationAgent +from tools.build_vuln_report_p3_54 import _build_exploit_record + + +def _point_at_scratch_dir(monkeypatch: pytest.MonkeyPatch, tmp_path: Path) -> Path: + reports_dir = tmp_path / "vuln_reports" + monkeypatch.setattr(approve_vuln_0004, "_REPORTS_DIR", reports_dir) + monkeypatch.setattr(approve_vuln_0004, "_PENDING_PATH", reports_dir / "VULN-0004.pending-human-approval.json") + monkeypatch.setattr(approve_vuln_0004, "_FILED_PATH", reports_dir / "VULN-0004.json") + # Pre-existing, out-of-scope issue: the success print's + # ``_FILED_PATH.relative_to(_REPO_ROOT)`` (same class the sibling + # tools/build_vuln_report_p3_54.py fixed for issue #64) crashes when + # reports_dir isn't under the real repo root. Not this test's concern -- + # pin _REPO_ROOT to tmp_path so this test isolates the duplicate-load + # regression it exists to guard, not that separate pre-existing bug. + monkeypatch.setattr(approve_vuln_0004, "_REPO_ROOT", tmp_path) + return reports_dir + + +def test_main_approves_a_genuinely_pending_report_left_by_a_prior_filing( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch, capsys: pytest.CaptureFixture[str] +) -> None: + reports_dir = _point_at_scratch_dir(monkeypatch, tmp_path) + + record = _build_exploit_record() + filer = DocumentationAgent(reports_dir=reports_dir) + filer.file_report(record, force_human_gate=True) + del filer # simulate the filing process (a prior main() run) exiting + + rc = approve_vuln_0004.main() + + assert rc == 0, capsys.readouterr() + assert approve_vuln_0004._FILED_PATH.exists() + assert not approve_vuln_0004._PENDING_PATH.exists() + on_disk = json.loads(approve_vuln_0004._FILED_PATH.read_text(encoding="utf-8")) + assert on_disk["approved_by"] == "owner" + + # Idempotent: a second run correctly reports already-approved, not a crash. + rc2 = approve_vuln_0004.main() + assert rc2 == 1 diff --git a/tools/approve_vuln_0004.py b/tools/approve_vuln_0004.py index ae2be63..e4face5 100644 --- a/tools/approve_vuln_0004.py +++ b/tools/approve_vuln_0004.py @@ -140,13 +140,28 @@ def main() -> int: assert record["exploit_id"] == _EXPLOIT_ID documentation = DocumentationAgent(reports_dir=_REPORTS_DIR) - pre_approval = _file_pending( - documentation, - record, - filed_at=original_filed_at, - force_human_gate=True, # denial_of_service is already unconditionally - # forced (FORCE_HUMAN_GATE_CATEGORIES); explicit here for clarity. - ) + # Cold-review fix (issue #63): ``DocumentationAgent`` now auto-loads + # persisted pending reports from ``reports_dir`` at construction time -- + # since ``_PENDING_PATH.exists()`` was just confirmed True above, this + # exact report is already sitting in ``documentation``'s in-memory + # ``_pending`` state. Re-driving ``file_report()`` via ``_file_pending`` + # here would collide with that already-loaded entry (the same + # one-exploit-one-report duplicate-rejection this module's own docstring + # describes) and crash instead of approving. Use what's already loaded + # directly when present; ``_file_pending`` stays as the fallback for a + # (no longer reachable in practice, but harmless to keep) construction + # that somehow didn't load it. + already_loaded = documentation.get_pending(_EXPLOIT_ID) + if already_loaded is not None: + pre_approval = {**already_loaded, "status": "pending_human_approval"} + else: + pre_approval = _file_pending( + documentation, + record, + filed_at=original_filed_at, + force_human_gate=True, # denial_of_service is already unconditionally + # forced (FORCE_HUMAN_GATE_CATEGORIES); explicit here for clarity. + ) # Compare parsed JSON field-for-field against what is already committed # on disk -- NOT a byte-for-byte comparison (key order, indentation, and @@ -177,7 +192,13 @@ def main() -> int: return 1 filed = documentation.approve(_EXPLOIT_ID, approved_by="owner") - _PENDING_PATH.unlink() + # Cold-review fix (issue #63): ``DocumentationAgent.approve`` now removes + # the persisted pending file itself (it writes the filed one first, then + # unlinks the pending one) -- an explicit ``_PENDING_PATH.unlink()`` here + # would double-unlink and raise ``FileNotFoundError``. ``missing_ok=True`` + # keeps this safe even if ``_REPORTS_DIR``/``_PENDING_PATH`` ever + # diverge from what ``approve()`` just removed. + _PENDING_PATH.unlink(missing_ok=True) print( f"exploit_id={_EXPLOIT_ID} report_id={filed['report_id']} " From a753b2ad829a9ec686c8bfb390777de9c6887133 Mon Sep 17 00:00:00 2001 From: franciszver <17106076+franciszver@users.noreply.github.com> Date: Sat, 25 Jul 2026 21:05:25 -0700 Subject: [PATCH 07/12] fix(P3.31 cold-review): approve_vuln_0004.py compared pending file against itself, not the exploit record The already_loaded branch set pre_approval from documentation.get_pending() -- read straight back off _PENDING_PATH by DocumentationAgent's own auto-load -- and compared it against pending_on_disk, the same file. The comparison always passed regardless of tampering; record = _build_exploit_record() was built and never used. Proof: file a legitimate pending VULN-0004 in a scratch dir, hand-edit severity to "low" and clinical_impact to "DOCTORED: ...", run main(). Before: rc=0, doctored artifact filed as owner-approved. After: rc=1, nothing written. Fix: reconstructed_body now comes from build_vuln_report(record, ...) -- independent re-derivation from the trusted exploit record -- and that is what gets compared against the on-disk artifact. already_loaded/pre_approval is used only to satisfy approve()'s in-memory precondition, never as the comparison target. Red-first: tests/tools/test_approve_vuln_0004_tamper.py (quoted failing output in this session: "assert 0 == 1" before the fix). --- tests/tools/test_approve_vuln_0004_tamper.py | 74 ++++++++++++++++++++ tools/approve_vuln_0004.py | 40 ++++++++--- 2 files changed, 103 insertions(+), 11 deletions(-) create mode 100644 tests/tools/test_approve_vuln_0004_tamper.py diff --git a/tests/tools/test_approve_vuln_0004_tamper.py b/tests/tools/test_approve_vuln_0004_tamper.py new file mode 100644 index 0000000..1b7fb98 --- /dev/null +++ b/tests/tools/test_approve_vuln_0004_tamper.py @@ -0,0 +1,74 @@ +"""Red-first regression test (DO-NOT-MERGE cold review of PR #76, FIX 1): +``tools/approve_vuln_0004.py``'s ``already_loaded`` branch compared the +auto-loaded pending report against ``pending_on_disk`` -- THE SAME FILE -- +which always passes regardless of tampering, because ``already_loaded`` is +read straight back off ``_PENDING_PATH`` by +``DocumentationAgent.__init__``'s auto-load (issue #63). ``record = +_build_exploit_record()`` was built and never used in that branch. + +Proof (quoted in the fix commit): file a legitimate pending VULN-0004 in a +scratch dir, hand-edit it to ``severity: "low"``, ``clinical_impact: +"DOCTORED: ..."``, then run ``main()``. + +Before this fix: ``rc == 0`` and the doctored artifact is filed as +owner-approved (self-comparison always passes). + +After this fix: ``rc == 1``, nothing is written -- the re-derivation via +``build_vuln_report(record, ...)`` is authoritative and does not match the +tampered on-disk content. + +Never touches the real ``docs/vuln_reports/`` -- points every module +constant at a scratch ``tmp_path`` first, same convention as +``tests/tools/test_approve_vuln_0004_rerun.py``. +""" + +from __future__ import annotations + +import json +from pathlib import Path + +import pytest + +import tools.approve_vuln_0004 as approve_vuln_0004 +from redteam.agents.documentation import DocumentationAgent +from tools.build_vuln_report_p3_54 import _build_exploit_record + + +def _point_at_scratch_dir(monkeypatch: pytest.MonkeyPatch, tmp_path: Path) -> Path: + reports_dir = tmp_path / "vuln_reports" + monkeypatch.setattr(approve_vuln_0004, "_REPORTS_DIR", reports_dir) + monkeypatch.setattr(approve_vuln_0004, "_PENDING_PATH", reports_dir / "VULN-0004.pending-human-approval.json") + monkeypatch.setattr(approve_vuln_0004, "_FILED_PATH", reports_dir / "VULN-0004.json") + monkeypatch.setattr(approve_vuln_0004, "_REPO_ROOT", tmp_path) + return reports_dir + + +def test_main_refuses_a_hand_tampered_pending_report_and_writes_nothing( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch, capsys: pytest.CaptureFixture[str] +) -> None: + reports_dir = _point_at_scratch_dir(monkeypatch, tmp_path) + + record = _build_exploit_record() + filer = DocumentationAgent(reports_dir=reports_dir) + filer.file_report(record, force_human_gate=True) + del filer # simulate the filing process (a prior main() run) exiting + + pending_path = approve_vuln_0004._PENDING_PATH + tampered = json.loads(pending_path.read_text(encoding="utf-8")) + tampered["severity"] = "low" + tampered["clinical_impact"] = "DOCTORED: nothing to see here" + pending_path.write_text(json.dumps(tampered, indent=2), encoding="utf-8") + + rc = approve_vuln_0004.main() + + err = capsys.readouterr().err + assert rc == 1, f"tampered pending artifact must be refused, got rc={rc}, stderr={err}" + assert "does not match" in err + assert not approve_vuln_0004._FILED_PATH.exists(), ( + "a tampered pending report must never be filed as owner-approved" + ) + # The tampered pending artifact itself must survive untouched -- refusal + # is not destructive. + still_on_disk = json.loads(pending_path.read_text(encoding="utf-8")) + assert still_on_disk["severity"] == "low" + assert still_on_disk["clinical_impact"] == "DOCTORED: nothing to see here" diff --git a/tools/approve_vuln_0004.py b/tools/approve_vuln_0004.py index e4face5..908fe21 100644 --- a/tools/approve_vuln_0004.py +++ b/tools/approve_vuln_0004.py @@ -63,7 +63,7 @@ sys.path.insert(0, str(Path(__file__).resolve().parents[1])) -from redteam.agents.documentation import DocumentationAgent # noqa: E402 +from redteam.agents.documentation import DocumentationAgent, build_vuln_report # noqa: E402 from tools.build_vuln_report_p3_54 import _build_exploit_record # noqa: E402 _REPO_ROOT = Path(__file__).resolve().parents[1] @@ -153,6 +153,15 @@ def main() -> int: # that somehow didn't load it. already_loaded = documentation.get_pending(_EXPLOIT_ID) if already_loaded is not None: + # Cold-review fix (this PR): ``already_loaded`` is read straight back + # off ``_PENDING_PATH`` by ``DocumentationAgent.__init__``'s auto-load + # -- it is THE SAME FILE this script is trying to authenticate, not + # independent evidence. It is used ONLY below to satisfy + # ``approve()``'s in-memory precondition (the exploit_id must be a + # key in ``documentation._pending``); it must never be the left-hand + # side of the field-for-field comparison, or the check degenerates + # into comparing the on-disk file against itself, which always + # passes regardless of tampering. pre_approval = {**already_loaded, "status": "pending_human_approval"} else: pre_approval = _file_pending( @@ -163,13 +172,19 @@ def main() -> int: # forced (FORCE_HUMAN_GATE_CATEGORIES); explicit here for clarity. ) - # Compare parsed JSON field-for-field against what is already committed - # on disk -- NOT a byte-for-byte comparison (key order, indentation, and - # trailing-newline differences are normalised away by json.loads on both - # sides). Still a strong guard: it fires on any FIELD-VALUE drift. Note - # what never enters this comparison at all, because it never enters the - # vuln_report in the first place (see documentation.py's "Why the - # vuln_report contract has no minimal_repro/recording_ref" section): + # Authoritative re-derivation: rebuild what the pending report SHOULD be + # directly from the re-derived ``record`` via ``build_vuln_report`` -- + # NOT from ``pre_approval``/``already_loaded`` above, which (when the + # auto-load path is taken, i.e. in every real re-run) is itself read off + # ``_PENDING_PATH``. Comparing THIS reconstruction against what is + # already committed on disk is what makes this a real cross-check: it + # fires on any FIELD-VALUE drift between the artifact and what the + # trusted exploit record actually produces, whether the drift came from + # tampering or corruption. Compared as parsed JSON, not byte-for-byte + # (key order, indentation, trailing-newline differences are normalised + # away). Note what never enters this comparison at all, because it never + # enters the vuln_report in the first place (see documentation.py's "Why + # the vuln_report contract has no minimal_repro/recording_ref" section): # the exploit record's case_id, attempt_id, verdict_id, source, # recording_ref, and minimal_repro.steps, plus confirmed_at (which # _build_exploit_record() sets to now_iso() every run, per @@ -179,9 +194,12 @@ def main() -> int: # contains (schema_version, report_id, exploit_id, severity, # clinical_impact, observed, expected, remediation, # fix_validation_status, requires_human_gate, filed_at) is checked. - # (``_file_pending`` above already guarantees ``pre_approval["status"] == - # "pending_human_approval"`` -- it raises SystemExit otherwise.) - reconstructed_body = {k: v for k, v in pre_approval.items() if k != "status"} + reconstructed_body = build_vuln_report( + record, + report_id=pending_on_disk["report_id"], + filed_at=original_filed_at, + force_human_gate=True, + ) if reconstructed_body != pending_on_disk: print( "reconstructed pre-approval report does not match the committed " From 17fc0e2d40784d35cd2ffe1579a3ad69f25f8ca5 Mon Sep 17 00:00:00 2001 From: franciszver <17106076+franciszver@users.noreply.github.com> Date: Sat, 25 Jul 2026 21:08:24 -0700 Subject: [PATCH 08/12] fix(P3.31 cold-review): --approve fails closed -- required cross-check, no --approved-by default, missing --db-path refused Four proven attacks against tools/run_campaign.py --approve: 1. With no --db-path, a hand-written VULN-0099.pending-human-approval.json approved cleanly with NO provenance check at all. 2. --db-path pointing at a missing file: ExploitDB(path) creates an empty sqlite, silently downgrading to "warning: skipping the cross-check" and approving as-is (rc=0) -- the one safety flag failed open on the most likely operator error (a typo). 3. --approved-by defaulted to "owner", so no explicit human identity was ever required. 4. The report body was never printed before being stamped approved. Fix: - --db-path + --approved-by are now both required for --approve, enforced at argparse time; --unverified-i-vouch-without-db-check is the explicit, loud escape hatch for a genuinely DB-less report (prints a WARNING). - A --db-path that doesn't already exist, or has no record for the exploit_id, is now a hard refusal (exit 1) -- never silently created, never downgraded to a skipped check. - The pending report body is printed before documentation.approve() is called. - Also fixes FIX 5's related bug: the rebuild now derives force_human_gate from the STORED (trusted) exploit record's category via FORCE_HUMAN_GATE_CATEGORIES, never from pending["requires_human_gate"] (the field under verification), and carries fix_validation_status through from the pending report so a legitimately-updated report doesn't spuriously fail the cross-check. Red-first: 6 new tests in tests/tools/test_run_campaign_cli.py reproducing each attack (quoted failing output in this session before the fix: attack 1 rc=0 with a hand-written report; attack 2 rc=0 + "warning: no exploit record ... skipping the field-for-field cross-check" against a typo'd --db-path that ExploitDB silently created). --- tests/tools/test_run_campaign_cli.py | 243 ++++++++++++++++++++++++++- tools/run_campaign.py | 150 +++++++++++++---- 2 files changed, 356 insertions(+), 37 deletions(-) diff --git a/tests/tools/test_run_campaign_cli.py b/tests/tools/test_run_campaign_cli.py index d7d83eb..b7e4712 100644 --- a/tests/tools/test_run_campaign_cli.py +++ b/tests/tools/test_run_campaign_cli.py @@ -51,7 +51,14 @@ def test_cli_lists_and_approves_a_report_left_pending_by_a_separate_process(tmp_ one DocumentationAgent instance (simulating a prior process), then use ONLY ``tools/run_campaign.py``'s CLI -- no bespoke script -- to list it and approve it.""" + from redteam.harness.db import ExploitDB + reports_dir = tmp_path / "vuln_reports" + db_path = tmp_path / "exploits.sqlite3" + + db = ExploitDB(db_path) + db.add_record(CRITICAL_EXPLOIT) + filer = DocumentationAgent(reports_dir=reports_dir) filer.file_report(CRITICAL_EXPLOIT) del filer # simulate the filing process exiting @@ -63,10 +70,20 @@ def test_cli_lists_and_approves_a_report_left_pending_by_a_separate_process(tmp_ assert "EXP-0001" in out rc = run_campaign.main( - ["--approve", "EXP-0001", "--reports-dir", str(reports_dir), "--approved-by", "owner"] + [ + "--approve", + "EXP-0001", + "--reports-dir", + str(reports_dir), + "--db-path", + str(db_path), + "--approved-by", + "owner", + ] ) assert rc == 0 out = capsys.readouterr().out + assert "--- pending report body (about to be approved) ---" in out assert "exploit_id=EXP-0001" in out assert "approved_by=owner" in out assert "status=filed" in out @@ -85,7 +102,17 @@ def test_approve_unknown_exploit_id_fails_without_writing(tmp_path, capsys): reports_dir = tmp_path / "vuln_reports" reports_dir.mkdir() - rc = run_campaign.main(["--approve", "EXP-9999", "--reports-dir", str(reports_dir)]) + rc = run_campaign.main( + [ + "--approve", + "EXP-9999", + "--reports-dir", + str(reports_dir), + "--approved-by", + "owner", + "--unverified-i-vouch-without-db-check", + ] + ) assert rc == 1 err = capsys.readouterr().err assert "no pending report" in err @@ -124,6 +151,8 @@ def test_approve_refuses_when_pending_report_drifts_from_its_source_exploit_reco str(reports_dir), "--db-path", str(db_path), + "--approved-by", + "owner", ] ) assert rc == 1 @@ -141,3 +170,213 @@ def test_never_auto_approves_no_default_exploit_id(tmp_path): args = run_campaign._parse_args(["--reports-dir", str(tmp_path)]) assert args.approve is None assert args.list_pending is False + + +# -- DO-NOT-MERGE cold review of PR #76, FIX 2 ------------------------------- +# "The cross-check fails open." Four attacks the reviewer proved, each now +# refused (fail closed): +# 1. --approve with no --db-path approved a hand-written pending report +# with NO provenance check at all. +# 2. --db-path pointing at a missing file: ExploitDB(path) CREATES an +# empty sqlite, silently downgrading to a warning + approve-as-is. +# 3. --approved-by defaulted to "owner" -- no explicit human required. +# 4. The report body was never displayed before stamping. + + +def test_approve_with_no_db_path_and_no_escape_hatch_is_refused_at_parse_time(tmp_path): + """Attack 1 reproduced: previously a hand-written + ``VULN-0099.pending-human-approval.json`` with no corresponding exploit + DB record at all approved cleanly (no provenance check whatsoever). Now + the cross-check is required by default -- omitting both --db-path and + the explicit escape hatch must refuse before ever touching + documentation.approve().""" + with pytest.raises(SystemExit): + run_campaign._parse_args( + ["--approve", "EXP-0099", "--reports-dir", str(tmp_path), "--approved-by", "owner"] + ) + + +def test_approve_with_missing_db_path_file_fails_closed_without_writing(tmp_path, capsys): + """Attack 2 reproduced: ``--db-path`` naming a file that does not yet + exist previously let ``ExploitDB(path)`` silently create an empty + sqlite DB, downgrading the cross-check to a warning and approving the + pending report as-is. A typo'd path is the most likely operator error + for the one safety flag this CLI has -- it must fail closed, not open.""" + reports_dir = tmp_path / "vuln_reports" + filer = DocumentationAgent(reports_dir=reports_dir) + filer.file_report(CRITICAL_EXPLOIT) + del filer + + missing_db_path = tmp_path / "typo-exploits.sqlite3" + assert not missing_db_path.exists() + + rc = run_campaign.main( + [ + "--approve", + "EXP-0001", + "--reports-dir", + str(reports_dir), + "--db-path", + str(missing_db_path), + "--approved-by", + "owner", + ] + ) + assert rc == 1 + err = capsys.readouterr().err + assert "does not exist" in err + # The DB path must still not exist -- refusing must not create it either. + assert not missing_db_path.exists() + assert not (reports_dir / "VULN-0001.json").exists() + + +def test_approve_without_approved_by_is_refused_at_parse_time(tmp_path): + """Attack 3 reproduced: --approved-by used to default to "owner", so no + explicit human identity was ever required to approve anything. It must + now be mandatory whenever --approve is used.""" + with pytest.raises(SystemExit): + run_campaign._parse_args( + ["--approve", "EXP-0001", "--reports-dir", str(tmp_path), "--unverified-i-vouch-without-db-check"] + ) + + +def test_approve_prints_report_body_before_stamping(tmp_path, capsys): + """Attack 4 reproduced: the pending report's body was never printed + before approve() stamped it -- an operator approved blind. Using the + explicit escape hatch (a genuinely DB-less report) must still print the + full body before approving.""" + reports_dir = tmp_path / "vuln_reports" + filer = DocumentationAgent(reports_dir=reports_dir) + filer.file_report(CRITICAL_EXPLOIT) + del filer + + rc = run_campaign.main( + [ + "--approve", + "EXP-0001", + "--reports-dir", + str(reports_dir), + "--approved-by", + "owner", + "--unverified-i-vouch-without-db-check", + ] + ) + assert rc == 0 + out = capsys.readouterr().out + assert "--- pending report body (about to be approved) ---" in out + assert '"report_id": "VULN-0001"' in out + assert '"clinical_impact"' in out + + +def test_approve_escape_hatch_prints_loud_warning_when_used(tmp_path, capsys): + """The escape hatch must be loud, not a quiet downgrade -- a WARNING + naming exactly what was skipped.""" + reports_dir = tmp_path / "vuln_reports" + filer = DocumentationAgent(reports_dir=reports_dir) + filer.file_report(CRITICAL_EXPLOIT) + del filer + + rc = run_campaign.main( + [ + "--approve", + "EXP-0001", + "--reports-dir", + str(reports_dir), + "--approved-by", + "owner", + "--unverified-i-vouch-without-db-check", + ] + ) + assert rc == 0 + err = capsys.readouterr().err + assert "WARNING" in err + assert "without a field-for-field cross-check" in err.lower() or "without" in err.lower() + + +def test_approve_with_stored_record_but_missing_from_db_fails_closed(tmp_path, capsys): + """A --db-path file that exists but simply has no record for this + exploit_id (e.g. wrong DB, or record never durably persisted) must also + fail closed, not silently downgrade to a warning + approve-as-is.""" + from redteam.harness.db import ExploitDB + + reports_dir = tmp_path / "vuln_reports" + db_path = tmp_path / "exploits.sqlite3" + ExploitDB(db_path) # exists, but empty -- no EXP-0001 record in it + + filer = DocumentationAgent(reports_dir=reports_dir) + filer.file_report(CRITICAL_EXPLOIT) + del filer + + rc = run_campaign.main( + [ + "--approve", + "EXP-0001", + "--reports-dir", + str(reports_dir), + "--db-path", + str(db_path), + "--approved-by", + "owner", + ] + ) + assert rc == 1 + err = capsys.readouterr().err + assert "no exploit record" in err + assert not (reports_dir / "VULN-0001.json").exists() + + +def test_approve_cross_check_uses_trusted_category_not_untrusted_pending_gate_field(tmp_path, capsys): + """Regression guard (cold review FIX 5): the rebuild must derive + force_human_gate from the STORED exploit record's category, not from + pending["requires_human_gate"] (the very field under verification), and + must carry fix_validation_status through so a fix_verified report + doesn't spuriously fail the cross-check.""" + from redteam.harness.db import ExploitDB + from redteam.agents.documentation import DocumentationAgent as _DA + + dos_record = { + "schema_version": "1.0.0", + "exploit_id": "EXP-0005", + "case_id": "dos-case", + "attempt_id": "att-0005", + "verdict_id": "ver-0005", + "category": "denial_of_service", + "source": "judge", + "confirmed_at": "2026-07-21T10:07:00Z", + "minimal_repro": { + "steps": ["oversized message"], + "expected": "rejected", + "observed": "accepted", + }, + "recording_ref": "evals/recordings/dos-case/20260721T100600Z-draw1.json", + } + + reports_dir = tmp_path / "vuln_reports" + db_path = tmp_path / "exploits.sqlite3" + db = ExploitDB(db_path) + db.add_record(dos_record) + + filer = _DA(reports_dir=reports_dir) + filer.file_report(dos_record, force_human_gate=True) # denial_of_service -> forced pending + del filer + + # Simulate a legitimate post-filing lifecycle update to fix_validation_status. + pending_path = reports_dir / "VULN-0005.pending-human-approval.json" + on_disk = json.loads(pending_path.read_text(encoding="utf-8")) + on_disk["fix_validation_status"] = "validated" + pending_path.write_text(json.dumps(on_disk, indent=2), encoding="utf-8") + + rc = run_campaign.main( + [ + "--approve", + "EXP-0005", + "--reports-dir", + str(reports_dir), + "--db-path", + str(db_path), + "--approved-by", + "owner", + ] + ) + err = capsys.readouterr().err + assert rc == 0, f"legitimate fix_verified pending report must pass the cross-check, stderr={err}" diff --git a/tools/run_campaign.py b/tools/run_campaign.py index 124f010..133f39a 100644 --- a/tools/run_campaign.py +++ b/tools/run_campaign.py @@ -46,7 +46,7 @@ every invocation and will collide with (and refuse to re-file over) an already-persisted report for that same id. -## Approving a durably-pending report (issue #63/#66) +## Approving a durably-pending report (issue #63/#66; hardened, cold-review of PR #76) A report a PRIOR ``run`` invocation left ``pending_human_approval`` (i.e. you passed ``--reports-dir`` to that run) can be approved by a separate, @@ -54,24 +54,32 @@ per-report script: python tools/run_campaign.py --list-pending --reports-dir PATH - python tools/run_campaign.py --approve EXP-0004 --reports-dir PATH [--db-path PATH] [--approved-by NAME] - -If ``--db-path`` names a persisted (non-``:memory:``) sqlite file that -still holds the original exploit record, the approve path re-derives the -report from that record via ``build_vuln_report`` and refuses (exit 1, -nothing approved) if it does not match the persisted pending report -field-for-field (ignoring ``filed_at``) -- the same verify-then-approve -discipline ``tools/approve_vuln_0004.py`` established, generalized. Without -a matching persisted db record, the cross-check is skipped (a warning is -printed) and the persisted pending report is approved as filed on disk -- -this is strictly safer than the pre-#63 state, where there was no durable -pending artifact to approve at all. + python tools/run_campaign.py --approve EXP-0004 --reports-dir PATH --db-path PATH --approved-by NAME + +``--approve`` FAILS CLOSED by default: it requires BOTH an explicit +``--approved-by NAME`` (no default -- an explicit human identity is the +point of a human-approval gate) and a ``--db-path`` naming an +already-existing sqlite file that holds the original exploit record for +that ``exploit_id``. The approve path re-derives the report from that +record via ``build_vuln_report`` and refuses (exit 1, nothing approved) if +it does not match the persisted pending report field-for-field (ignoring +``filed_at``) -- the same verify-then-approve discipline +``tools/approve_vuln_0004.py`` established, generalized. A ``--db-path`` +that doesn't already exist, or that has no record for this exploit_id, is +a hard refusal (exit 1) -- it is never silently created or downgraded to a +skipped check. The pending report's full body is printed before it is +stamped, so approval is an informed act. + +For a genuinely DB-less pending report, pass the explicit +``--unverified-i-vouch-without-db-check`` escape hatch instead of +``--db-path`` -- this prints a loud WARNING and skips the cross-check; the +operator is vouching for the report's content by hand, unverified. ## Usage python tools/run_campaign.py [--iterations N] [--reports-dir PATH] [--db-path PATH] python tools/run_campaign.py --list-pending --reports-dir PATH - python tools/run_campaign.py --approve EXPLOIT_ID --reports-dir PATH [--db-path PATH] [--approved-by NAME] + python tools/run_campaign.py --approve EXPLOIT_ID --reports-dir PATH --approved-by NAME (--db-path PATH | --unverified-i-vouch-without-db-check) Requires (``run`` mode only): the target stack up (``docker ps`` shows ``development-easy-agent-1``) and the local ollama server up with @@ -94,6 +102,7 @@ from evals.cases.dos_input_bound import CASE as DOS_CASE # noqa: E402 from evals.cases.identity_authz import CASE as AUTHZ_CASE # noqa: E402 from redteam.agents.documentation import ( # noqa: E402 + FORCE_HUMAN_GATE_CATEGORIES, DocumentationAgent, DocumentationAgentError, build_vuln_report, @@ -137,8 +146,22 @@ def _parse_args(argv: Sequence[str] | None) -> argparse.Namespace: ) parser.add_argument( "--approved-by", - default="owner", - help="[approve mode] approving identity to stamp (default: %(default)s)", + default=None, + help=( + "[approve mode] approving identity to stamp -- REQUIRED, no default. An " + "explicit human identity is the point of a human-approval gate." + ), + ) + parser.add_argument( + "--unverified-i-vouch-without-db-check", + action="store_true", + help=( + "[approve mode] escape hatch: approve WITHOUT the field-for-field " + "cross-check against a persisted exploit DB record (--db-path). Only for " + "a genuinely DB-less pending report -- passing this means the operator is " + "vouching for the pending report's content by hand, unverified against " + "anything. Loud on purpose." + ), ) parser.add_argument( "--list-pending", @@ -150,6 +173,14 @@ def _parse_args(argv: Sequence[str] | None) -> argparse.Namespace: parser.error("--list-pending requires --reports-dir") if args.approve is not None and args.reports_dir is None: parser.error("--approve requires --reports-dir") + if args.approve is not None and args.approved_by is None: + parser.error("--approve requires --approved-by (no default -- an explicit human identity)") + if args.approve is not None and args.db_path is None and not args.unverified_i_vouch_without_db_check: + parser.error( + "--approve requires --db-path PATH for the field-for-field cross-check " + "against the source exploit record, or --unverified-i-vouch-without-db-check " + "to explicitly approve without one" + ) return args @@ -170,7 +201,17 @@ def _cmd_approve(args: argparse.Namespace) -> int: -- see the module docstring's "Approving a durably-pending report" section for the verify-then-approve discipline this reuses from ``tools/approve_vuln_0004.py``, generalized to any exploit_id/reports_dir - rather than one hardcoded report.""" + rather than one hardcoded report. + + Fails CLOSED (cold-review fix, this PR): the cross-check against a + persisted exploit DB record is required by default -- ``_parse_args`` + already refused to reach this function without either ``--db-path`` or + the explicit ``--unverified-i-vouch-without-db-check`` escape hatch. A + ``--db-path`` that doesn't already exist, or that exists but has no + record for this ``exploit_id``, is now a hard refusal (exit 1) rather + than a warning that silently approves as-is -- a typo'd path must not + be the thing that downgrades the one safety flag to a no-op. + """ documentation = DocumentationAgent(reports_dir=args.reports_dir) pending = documentation.get_pending(args.approve) if pending is None: @@ -181,31 +222,70 @@ def _cmd_approve(args: argparse.Namespace) -> int: ) return 1 - if args.db_path is not None and str(args.db_path) != ":memory:": + if args.db_path is not None: + # ExploitDB(path) CREATES an empty sqlite file for any path that + # doesn't already exist -- checking existence first, before ever + # constructing it, is what makes a typo'd --db-path a hard refusal + # instead of a silent empty DB. + if not Path(args.db_path).exists(): + print( + f"refusing to approve {args.approve!r}: --db-path {args.db_path} does not " + "exist -- will not silently create an empty exploit DB to satisfy the " + "cross-check. Pass the correct path, or " + "--unverified-i-vouch-without-db-check to explicitly approve without one.", + file=sys.stderr, + ) + return 1 + db = ExploitDB(args.db_path) stored = db.get(args.approve) if stored is None: print( - f"warning: no exploit record for {args.approve!r} in {args.db_path} " - "-- skipping the field-for-field cross-check and approving the " - "persisted pending report as-is", + f"refusing to approve {args.approve!r}: no exploit record for it in " + f"{args.db_path} -- cannot perform the field-for-field cross-check. Pass " + "--unverified-i-vouch-without-db-check to explicitly approve without one.", file=sys.stderr, ) - else: - rebuilt = build_vuln_report( - stored["record"], - report_id=pending["report_id"], - filed_at=pending["filed_at"], - force_human_gate=pending["requires_human_gate"], + return 1 + + # force_human_gate is derived from the STORED record's category + # (trusted), never from pending["requires_human_gate"] -- the very + # field being cross-checked/verified -- and fix_validation_status is + # taken from the pending report itself: it is a report-lifecycle + # field with no bearing on the human-approval gate, legitimately set + # after filing (e.g. "fix_verified"), not a category-derived + # safety-relevant field like severity/requires_human_gate. + stored_category = stored["record"].get("category") + rebuilt = build_vuln_report( + stored["record"], + report_id=pending["report_id"], + filed_at=pending["filed_at"], + fix_validation_status=pending.get("fix_validation_status", "not_validated"), + force_human_gate=stored_category in FORCE_HUMAN_GATE_CATEGORIES, + ) + if rebuilt != pending: + print( + f"refusing to approve {args.approve!r}: the persisted pending report " + "does not match what its source exploit record would produce -- " + f"rebuilt={rebuilt}\npending_on_disk={pending}", + file=sys.stderr, ) - if rebuilt != pending: - print( - f"refusing to approve {args.approve!r}: the persisted pending report " - "does not match what its source exploit record would produce -- " - f"rebuilt={rebuilt}\npending_on_disk={pending}", - file=sys.stderr, - ) - return 1 + return 1 + else: + # _parse_args already refused to reach here unless + # --unverified-i-vouch-without-db-check was explicitly passed. + print( + f"WARNING: approving {args.approve!r} WITHOUT a field-for-field cross-check " + "against a persisted exploit DB record -- --unverified-i-vouch-without-db-check " + "was passed, so the operator is vouching for this pending report's content by " + "hand, unverified.", + file=sys.stderr, + ) + + # Print the report body before stamping so approval is an informed act, + # not a blind exploit_id lookup. + print("--- pending report body (about to be approved) ---") + print(json.dumps(pending, indent=2)) try: filed = documentation.approve(args.approve, approved_by=args.approved_by) From 8199a834b698c5a308027174fdc5ebe0ee0f78a9 Mon Sep 17 00:00:00 2001 From: franciszver <17106076+franciszver@users.noreply.github.com> Date: Sat, 25 Jul 2026 21:13:43 -0700 Subject: [PATCH 09/12] fix(P3.31 cold-review): file_report failure no longer crashes the campaign; post-loop export in try/finally; --reports-dir without --db-path refuses to start Reproduced: a second run_campaign() invocation against the same durable --reports-dir with in-memory (default) exploit numbering restarts at EXP-0001, collides with the pending VULN-0001 report run 1 left on disk, and documentation.file_report() raises DocumentationAgentError uncaught -- killing the campaign mid-loop. Because the post-loop action_log export sat after the loop with no try/finally, the crashed run's own action log (including everything logged before the crash) was never exported at all. Fix: - documentation.file_report() is now wrapped in try/except DocumentationAgentError like every sibling component call in the loop (orchestrator/red_team/target_client/judge) -- records a vuln_report_filing_failed signal and continues; the confirmed exploit stays safely recorded in db (db.add_record already ran unconditionally before this call). - The entire iteration loop is now wrapped in try/finally so action_log.export_jsonl(action_log_ref) always runs, even if some other exception the loop doesn't explicitly catch escapes an iteration. - tools/run_campaign.py: `run` mode with --reports-dir but no --db-path now refuses to start (parser.error) instead of emitting a stderr NOTE and continuing into the same collision. Red-first: tests/redteam/test_campaign.py (test_duplicate_report_filing_does_not_crash_the_campaign, test_action_log_exports_even_when_an_iteration_raises_uncaught) and tests/tools/test_run_campaign_cli.py (test_run_mode_reports_dir_without_db_path_refuses_to_start) -- quoted failing output in this session: uncaught DocumentationAgentError from redteam/campaign.py:436, and "assert False" on action_log_ref.exists() before the fix. --- redteam/campaign.py | 465 ++++++++++++++------------- tests/redteam/test_campaign.py | 105 ++++++ tests/tools/test_run_campaign_cli.py | 14 +- tools/run_campaign.py | 37 ++- 4 files changed, 388 insertions(+), 233 deletions(-) diff --git a/redteam/campaign.py b/redteam/campaign.py index 03b7755..01319e0 100644 --- a/redteam/campaign.py +++ b/redteam/campaign.py @@ -102,7 +102,7 @@ from evals.runner import DEFAULT_CONTAINER, DEFAULT_TIMEOUT_S, ParsedResponse, drive_chat, record_run from evals.schema import AttackCase -from redteam.agents.documentation import FORCE_HUMAN_GATE_CATEGORIES, DocumentationAgent +from redteam.agents.documentation import FORCE_HUMAN_GATE_CATEGORIES, DocumentationAgent, DocumentationAgentError from redteam.agents.judge import JudgeAgent, JudgeDriftSuspectedError, JudgeTimeoutError from redteam.agents.orchestrator import BudgetExceededError, NoFindingsInWindowError, Orchestrator from redteam.agents.red_team import RedTeamAgent, RedTeamAgentError @@ -243,252 +243,277 @@ def _default_snapshot() -> dict[str, Any]: result = CampaignResult(iterations_run=0, stopped_reason="max_iterations") - for _ in range(max_iterations): - result.iterations_run += 1 + try: + for _ in range(max_iterations): + result.iterations_run += 1 - snapshot = build_snapshot() - action_log.append(agent="observability", event_type="snapshot_emitted", details=snapshot) + snapshot = build_snapshot() + action_log.append(agent="observability", event_type="snapshot_emitted", details=snapshot) - # -- 1. Orchestrator: next attack_directive ------------------------- - try: - directive = orchestrator.next_directive( - snapshot, verdicts=result.verdicts, cases=cases, db=db, vuln_reports=all_vuln_reports - ) - except BudgetExceededError as exc: - action_log.append(agent="orchestrator", event_type="budget_exceeded", details=exc.error) - result.signals.append(dict(exc.error)) - result.stopped_reason = "budget_exceeded" - break - except NoFindingsInWindowError as exc: + # -- 1. Orchestrator: next attack_directive ------------------------- + try: + directive = orchestrator.next_directive( + snapshot, verdicts=result.verdicts, cases=cases, db=db, vuln_reports=all_vuln_reports + ) + except BudgetExceededError as exc: + action_log.append(agent="orchestrator", event_type="budget_exceeded", details=exc.error) + result.signals.append(dict(exc.error)) + result.stopped_reason = "budget_exceeded" + break + except NoFindingsInWindowError as exc: + action_log.append( + agent="orchestrator", + event_type="no_findings_in_window", + category=exc.error.get("category"), + details=exc.error, + ) + result.signals.append(dict(exc.error)) + continue + + result.directives.append(directive) action_log.append( agent="orchestrator", - event_type="no_findings_in_window", - category=exc.error.get("category"), - details=exc.error, + event_type="directive_issued", + category=directive["category"], + details=directive, ) - result.signals.append(dict(exc.error)) - continue - - result.directives.append(directive) - action_log.append( - agent="orchestrator", - event_type="directive_issued", - category=directive["category"], - details=directive, - ) - # -- 2. Red Team: generate one attack_attempt ----------------------- - selector = directive["next_case"]["selector"] - prior_attempt = None - if selector == "mutation_of": - prior_attempt = attempts_by_id.get(directive["next_case"]["mutation_of"]) - if prior_attempt is None: + # -- 2. Red Team: generate one attack_attempt ----------------------- + selector = directive["next_case"]["selector"] + prior_attempt = None + if selector == "mutation_of": + prior_attempt = attempts_by_id.get(directive["next_case"]["mutation_of"]) + if prior_attempt is None: + action_log.append( + agent="harness", + event_type="mutation_source_missing", + category=directive["category"], + details={"mutation_of": directive["next_case"]["mutation_of"]}, + ) + continue + + try: + attempt = red_team.generate_attempt(directive, prior_attempt=prior_attempt, bearer_token=bearer_token) + except RedTeamAgentError as exc: + # A generation failure (e.g. the model returned an empty + # completion -- red_team.py's module docstring documents this + # as a real possibility live) must not crash the whole + # autonomous run over one bad draw; skip this iteration. action_log.append( - agent="harness", - event_type="mutation_source_missing", + agent="red_team", + event_type="attempt_generation_failed", category=directive["category"], - details={"mutation_of": directive["next_case"]["mutation_of"]}, + details={"message": str(exc)}, ) + result.signals.append({"error_type": "attempt_generation_failed", "message": str(exc)}) continue - - try: - attempt = red_team.generate_attempt(directive, prior_attempt=prior_attempt, bearer_token=bearer_token) - except RedTeamAgentError as exc: - # A generation failure (e.g. the model returned an empty - # completion -- red_team.py's module docstring documents this - # as a real possibility live) must not crash the whole - # autonomous run over one bad draw; skip this iteration. + attempts_by_id[attempt["attempt_id"]] = attempt + result.attempts.append(attempt) action_log.append( agent="red_team", - event_type="attempt_generation_failed", - category=directive["category"], - details={"message": str(exc)}, - ) - result.signals.append({"error_type": "attempt_generation_failed", "message": str(exc)}) - continue - attempts_by_id[attempt["attempt_id"]] = attempt - result.attempts.append(attempt) - action_log.append( - agent="red_team", - event_type="attempt_generated", - case_id=attempt["case_id"], - category=attempt["category"], - details=attempt, - ) - - # -- 3. Drive the target --------------------------------------------- - try: - response = target_client(attempt) - except Exception as exc: # noqa: BLE001 - a hostile/unreachable target must not crash the loop - action_log.append( - agent="harness", - event_type="target_unreachable", + event_type="attempt_generated", case_id=attempt["case_id"], category=attempt["category"], - details={"attempted_at": now_iso(), "message": str(exc)}, + details=attempt, ) - result.signals.append( - { - "error_type": "target_unreachable", - "message": str(exc), - "attempted_at": now_iso(), - "attempt_id": attempt["attempt_id"], - } - ) - continue - # -- 4. Judge: score the response ------------------------------------ - case = cases_by_category.get(attempt["category"]) - if case is None: - action_log.append( - agent="harness", - event_type="no_case_for_category", - case_id=attempt["case_id"], - category=attempt["category"], - details={"attempt_id": attempt["attempt_id"]}, - ) - continue + # -- 3. Drive the target --------------------------------------------- + try: + response = target_client(attempt) + except Exception as exc: # noqa: BLE001 - a hostile/unreachable target must not crash the loop + action_log.append( + agent="harness", + event_type="target_unreachable", + case_id=attempt["case_id"], + category=attempt["category"], + details={"attempted_at": now_iso(), "message": str(exc)}, + ) + result.signals.append( + { + "error_type": "target_unreachable", + "message": str(exc), + "attempted_at": now_iso(), + "attempt_id": attempt["attempt_id"], + } + ) + continue + + # -- 4. Judge: score the response ------------------------------------ + case = cases_by_category.get(attempt["category"]) + if case is None: + action_log.append( + agent="harness", + event_type="no_case_for_category", + case_id=attempt["case_id"], + category=attempt["category"], + details={"attempt_id": attempt["attempt_id"]}, + ) + continue - try: - verdict = judge.score(case, response, attempt_id=attempt["attempt_id"]) - except JudgeTimeoutError as exc: + try: + verdict = judge.score(case, response, attempt_id=attempt["attempt_id"]) + except JudgeTimeoutError as exc: + action_log.append( + agent="judge", + event_type="judge_timeout", + case_id=case.id, + category=attempt["category"], + details=exc.error, + ) + result.signals.append(dict(exc.error)) + continue + + result.verdicts.append(verdict) action_log.append( agent="judge", - event_type="judge_timeout", - case_id=case.id, + event_type="verdict_scored", + case_id=verdict["case_id"], category=attempt["category"], - details=exc.error, + details=verdict, ) - result.signals.append(dict(exc.error)) - continue - - result.verdicts.append(verdict) - action_log.append( - agent="judge", - event_type="verdict_scored", - case_id=verdict["case_id"], - category=attempt["category"], - details=verdict, - ) - # -- 5. Store confirmed exploits (unconditionally) + document --------- - # A confirmed ("success"/"regression") outcome is ALWAYS recorded -- - # ``record_run`` and ``db.add_record`` below run for every confirmed - # outcome, full stop. A red-team platform must never destroy the - # only copy of a target's raw response. Whether - # ``documentation.file_report`` auto-files or gates for human - # approval is a SEPARATE, later decision (below) that never touches - # this write. The Judge's own scoring/drift-detection integrity stays - # untouched either way -- `verdict` above still reflects the case's - # honest, unmodified `detect` output, and this step never feeds back - # into `JudgeAgent.score`/`map_outcome`/`check_drift` (ARCHITECTURE.md - # §6's gold-probe drift baseline scores against `JudgeAgent.score` - # alone). - outcome_confirmed = verdict["outcome"] in ("success", "regression") - if outcome_confirmed: - recording_path = record_run( - attempt["case_id"], - attempt["draw_number"], - response, - verdict["evidence"].get("detection_label", ""), - True, - recordings_dir=recordings_dir, - ) - exploit_id = db.next_exploit_id() - record = { - "schema_version": "1.0.0", - "exploit_id": exploit_id, - "case_id": verdict["case_id"], - "attempt_id": verdict["attempt_id"], - "verdict_id": verdict["verdict_id"], - "category": attempt["category"], - "source": "judge", - "confirmed_at": verdict["scored_at"], - "minimal_repro": _minimal_repro(attempt, verdict), - "recording_ref": str(recording_path), - } - db.add_record(record) - result.exploit_ids.append(exploit_id) - action_log.append( - agent="harness", - event_type="exploit_recorded", - case_id=record["case_id"], - category=record["category"], - details=record, - ) - - # Category-level human-approval gate (issue #55): ``denial_of_service`` - # is not reliably machine-decidable -- ``dos_input_bound.detect`` - # structurally cannot distinguish "guard absent" from "guard - # fired then fail-soft-swallowed" for a 200-with-an-`answer` - # (see that module's "STRUCTURAL BLIND SPOT" comment). Rather - # than suppressing the report (unreachable in the live loop -- - # ``Orchestrator._pick_next_case`` never emits ``case_id``, so a - # message-match predicate against one documented probe never - # fires outside a deliberate replay), every CONFIRMED outcome in - # this category is filed but forced through the same - # human-approval gate a critical-severity finding uses - # (``DocumentationAgent.file_report(..., force_human_gate=True)``) - # -- surfaced for triage, never silently dropped or auto-published. - # This applies category-wide (novel payloads included), not - # narrowed to one exact probe message; no other category's - # gating is affected. - report = documentation.file_report( - record, force_human_gate=attempt["category"] in FORCE_HUMAN_GATE_CATEGORIES - ) - if report["status"] == "pending_human_approval": - result.pending_reports.append(report) - action_log.append( - agent="documentation", - event_type="vuln_report_pending_human_approval", - category=record["category"], - details=report, + # -- 5. Store confirmed exploits (unconditionally) + document --------- + # A confirmed ("success"/"regression") outcome is ALWAYS recorded -- + # ``record_run`` and ``db.add_record`` below run for every confirmed + # outcome, full stop. A red-team platform must never destroy the + # only copy of a target's raw response. Whether + # ``documentation.file_report`` auto-files or gates for human + # approval is a SEPARATE, later decision (below) that never touches + # this write. The Judge's own scoring/drift-detection integrity stays + # untouched either way -- `verdict` above still reflects the case's + # honest, unmodified `detect` output, and this step never feeds back + # into `JudgeAgent.score`/`map_outcome`/`check_drift` (ARCHITECTURE.md + # §6's gold-probe drift baseline scores against `JudgeAgent.score` + # alone). + outcome_confirmed = verdict["outcome"] in ("success", "regression") + if outcome_confirmed: + recording_path = record_run( + attempt["case_id"], + attempt["draw_number"], + response, + verdict["evidence"].get("detection_label", ""), + True, + recordings_dir=recordings_dir, ) - else: - result.filed_reports.append(report) + exploit_id = db.next_exploit_id() + record = { + "schema_version": "1.0.0", + "exploit_id": exploit_id, + "case_id": verdict["case_id"], + "attempt_id": verdict["attempt_id"], + "verdict_id": verdict["verdict_id"], + "category": attempt["category"], + "source": "judge", + "confirmed_at": verdict["scored_at"], + "minimal_repro": _minimal_repro(attempt, verdict), + "recording_ref": str(recording_path), + } + db.add_record(record) + result.exploit_ids.append(exploit_id) action_log.append( - agent="documentation", - event_type="vuln_report_filed", + agent="harness", + event_type="exploit_recorded", + case_id=record["case_id"], category=record["category"], - details=report, + details=record, ) - all_vuln_reports.append(report) - # -- 6. Regression sweep (only on caller-named iterations) ----------- - if result.iterations_run in regression_sweep_at: - regressions = orchestrator.trigger_regression_sweep( - db, cases, status_transition_occurred=True, recordings_dir=recordings_dir - ) - for regression in regressions: - action_log.append( - agent="harness", - event_type="regression_detected", - category=regression["category"], - details=regression, + # Category-level human-approval gate (issue #55): ``denial_of_service`` + # is not reliably machine-decidable -- ``dos_input_bound.detect`` + # structurally cannot distinguish "guard absent" from "guard + # fired then fail-soft-swallowed" for a 200-with-an-`answer` + # (see that module's "STRUCTURAL BLIND SPOT" comment). Rather + # than suppressing the report (unreachable in the live loop -- + # ``Orchestrator._pick_next_case`` never emits ``case_id``, so a + # message-match predicate against one documented probe never + # fires outside a deliberate replay), every CONFIRMED outcome in + # this category is filed but forced through the same + # human-approval gate a critical-severity finding uses + # (``DocumentationAgent.file_report(..., force_human_gate=True)``) + # -- surfaced for triage, never silently dropped or auto-published. + # This applies category-wide (novel payloads included), not + # narrowed to one exact probe message; no other category's + # gating is affected. + # Cold-review fix (this PR, FIX 3): wrapped like every sibling + # component call in this loop (orchestrator/red_team/target_client/ + # judge above) -- a rejected filing (e.g. a duplicate report for + # this exploit_id, which is reachable in practice against a + # durable ``--reports-dir`` reused across runs) must not crash + # the whole autonomous campaign mid-loop. Record the signal and + # move on; the confirmed exploit is still safely in ``db`` + # (step 5's ``db.add_record`` above already ran, unconditionally, + # before this call). + try: + report = documentation.file_report( + record, force_human_gate=attempt["category"] in FORCE_HUMAN_GATE_CATEGORIES + ) + except DocumentationAgentError as exc: + action_log.append( + agent="documentation", + event_type="vuln_report_filing_failed", + case_id=record["case_id"], + category=record["category"], + details={"message": str(exc), "exploit_id": exploit_id}, + ) + result.signals.append( + {"error_type": "vuln_report_filing_failed", "message": str(exc), "exploit_id": exploit_id} + ) + continue + if report["status"] == "pending_human_approval": + result.pending_reports.append(report) + action_log.append( + agent="documentation", + event_type="vuln_report_pending_human_approval", + category=record["category"], + details=report, + ) + else: + result.filed_reports.append(report) + action_log.append( + agent="documentation", + event_type="vuln_report_filed", + category=record["category"], + details=report, + ) + all_vuln_reports.append(report) + + # -- 6. Regression sweep (only on caller-named iterations) ----------- + if result.iterations_run in regression_sweep_at: + regressions = orchestrator.trigger_regression_sweep( + db, cases, status_transition_occurred=True, recordings_dir=recordings_dir ) - result.signals.append(dict(regression)) - - # -- 7. Drift sweep (only on caller-named cadence) -------------------- - if drift_check_every and result.iterations_run % drift_check_every == 0: - try: - judge.check_drift() - except JudgeDriftSuspectedError as exc: - action_log.append(agent="judge", event_type="judge_drift_suspected", details=exc.error) - result.signals.append({"error_type": "judge_drift_suspected", **exc.error}) - - # Post-loop export (issue #63): ``emit_snapshot`` (called at the TOP of - # each iteration, in the default ``snapshot_fn`` path) is the only place - # that calls ``action_log.export_jsonl`` -- so every event appended - # AFTER that iteration's own snapshot call (directive_issued through - # vuln_report_filed/pending, regression/drift signals) never reached - # ``action_log_ref`` for the LAST iteration a run makes. For - # ``max_iterations=1`` that is every event the run produced. Exporting - # here, unconditionally, after the loop (whether it ran to - # ``max_iterations`` or broke early on ``budget_exceeded``) guarantees a - # run's own events are never lost, regardless of whether the caller - # injected a fake ``snapshot_fn`` (as every deterministic test in - # ``tests/redteam/test_campaign.py`` does) that never touches - # ``action_log_ref`` at all. - action_log.export_jsonl(action_log_ref) + for regression in regressions: + action_log.append( + agent="harness", + event_type="regression_detected", + category=regression["category"], + details=regression, + ) + result.signals.append(dict(regression)) + + # -- 7. Drift sweep (only on caller-named cadence) -------------------- + if drift_check_every and result.iterations_run % drift_check_every == 0: + try: + judge.check_drift() + except JudgeDriftSuspectedError as exc: + action_log.append(agent="judge", event_type="judge_drift_suspected", details=exc.error) + result.signals.append({"error_type": "judge_drift_suspected", **exc.error}) + + finally: + # Post-loop export (issue #63): ``emit_snapshot`` (called at the TOP of + # each iteration, in the default ``snapshot_fn`` path) is the only place + # that calls ``action_log.export_jsonl`` -- so every event appended + # AFTER that iteration's own snapshot call (directive_issued through + # vuln_report_filed/pending, regression/drift signals) never reached + # ``action_log_ref`` for the LAST iteration a run makes. For + # ``max_iterations=1`` that is every event the run produced. Exporting + # here, unconditionally, in a ``finally`` (cold-review fix, this PR) + # guarantees a run's own events are never lost -- whether the loop ran + # to ``max_iterations``, broke early on ``budget_exceeded``, or an + # iteration raised an exception the loop itself doesn't catch -- and + # regardless of whether the caller injected a fake ``snapshot_fn`` (as + # every deterministic test in ``tests/redteam/test_campaign.py`` does) + # that never touches ``action_log_ref`` at all. + action_log.export_jsonl(action_log_ref) return result diff --git a/tests/redteam/test_campaign.py b/tests/redteam/test_campaign.py index c2b99f5..b165b9f 100644 --- a/tests/redteam/test_campaign.py +++ b/tests/redteam/test_campaign.py @@ -631,3 +631,108 @@ def test_post_loop_action_log_export_includes_last_iterations_own_events(tmp_pat exported_event_types = {json.loads(line)["event_type"] for line in exported_lines} assert "exploit_recorded" in exported_event_types assert "vuln_report_filed" in exported_event_types + + +# -- DO-NOT-MERGE cold review of PR #76, FIX 3 ------------------------------- +# "Documented flag combo aborts the run and loses the new export." Reproduced: +# a second run against the same durable --reports-dir (no --db-path, so +# exploit IDs restart at EXP-0001) collides on file_report's duplicate-report +# guard, raises DocumentationAgentError from inside the loop, and -- because +# the post-loop export sat after the loop with no try/finally -- the crashed +# run's own action log (including the earlier, successful events from THIS +# same run, before the crash) was never exported at all. + + +def test_duplicate_report_filing_does_not_crash_the_campaign(tmp_path): + """A durable reports_dir reused across two runs with in-memory (default) + exploit numbering collides: run 2's freshly-generated EXP-0001 already + has a pending VULN-0001 report on disk from run 1. + ``documentation.file_report`` raises ``DocumentationAgentError`` for + that -- it must be caught and recorded as a signal, not crash the whole + autonomous run.""" + recordings_dir = tmp_path / "recordings" + reports_dir = tmp_path / "vuln_reports" + + def _run_once(action_log_ref: Path): + db = ExploitDB(":memory:") # in-memory -- exploit IDs restart at EXP-0001 + action_log = ActionLog(":memory:") + documentation = DocumentationAgent(reports_dir=reports_dir) # durable + judge = JudgeAgent() + red_team = RedTeamAgent(model_client=_fake_model_client) + orchestrator = Orchestrator(no_findings_window=5) + return run_campaign( + orchestrator=orchestrator, + red_team=red_team, + judge=judge, + documentation=documentation, + db=db, + action_log=action_log, + action_log_ref=action_log_ref, + cases=[DOS_CASE, AUTHZ_CASE], + target_client=lambda attempt: _vulnerable_response(), + max_iterations=1, + recordings_dir=recordings_dir, + snapshot_fn=lambda: _full_coverage_snapshot("identity_authz"), + ) + + result1 = _run_once(tmp_path / "action_log_1.jsonl") + assert result1.exploit_ids == ["EXP-0001"] + assert len(result1.pending_reports) == 1 + + # Run 2: same reports_dir, exploit IDs restart at EXP-0001 -> collides + # with the pending VULN-0001 report run 1 left on disk. + result2 = _run_once(tmp_path / "action_log_2.jsonl") + + assert result2.exploit_ids == ["EXP-0001"], "the exploit itself must still be recorded" + assert result2.pending_reports == [], "the colliding report must not be filed" + assert result2.filed_reports == [] + filing_failed_signals = [s for s in result2.signals if s.get("error_type") == "vuln_report_filing_failed"] + assert len(filing_failed_signals) == 1 + assert filing_failed_signals[0]["exploit_id"] == "EXP-0001" + + +def test_action_log_exports_even_when_an_iteration_raises_uncaught(tmp_path): + """Belt-and-suspenders: even an exception NOT caught anywhere inside the + loop (e.g. a caller-injected ``snapshot_fn`` that itself raises -- no + typed component error, nothing this loop's own try/except blocks are + written to catch) must not prevent the post-loop export -- it must run + from a ``finally``, not merely "after the loop" (which an uncaught + exception skips entirely, per Python control flow).""" + recordings_dir = tmp_path / "recordings" + db, action_log, documentation, judge, red_team, orchestrator = _new_agents(recordings_dir) + action_log_ref = tmp_path / "action_log.jsonl" + + # Emit one real event before the fatal snapshot call, via a snapshot_fn + # that raises on its SECOND call -- so there is something in + # ``action_log`` to prove got exported despite the eventual crash. + calls = {"n": 0} + + def _snapshot_then_boom(): + calls["n"] += 1 + if calls["n"] > 1: + raise RuntimeError("simulated unforeseen failure, not one of the typed component errors") + return _full_coverage_snapshot("identity_authz") + + with pytest.raises(RuntimeError, match="simulated unforeseen failure"): + run_campaign( + orchestrator=orchestrator, + red_team=red_team, + judge=judge, + documentation=documentation, + db=db, + action_log=action_log, + action_log_ref=action_log_ref, + cases=[DOS_CASE, AUTHZ_CASE], + target_client=lambda attempt: _vulnerable_response(), + max_iterations=2, + recordings_dir=recordings_dir, + snapshot_fn=_snapshot_then_boom, + ) + + assert action_log_ref.exists(), ( + "the action log must still be exported even when an iteration raises " + "an exception the loop itself does not catch" + ) + exported_lines = action_log_ref.read_text(encoding="utf-8").splitlines() + exported_event_types = {json.loads(line)["event_type"] for line in exported_lines} + assert "attempt_generated" in exported_event_types diff --git a/tests/tools/test_run_campaign_cli.py b/tests/tools/test_run_campaign_cli.py index b7e4712..ee328fc 100644 --- a/tests/tools/test_run_campaign_cli.py +++ b/tests/tools/test_run_campaign_cli.py @@ -167,11 +167,23 @@ def test_never_auto_approves_no_default_exploit_id(tmp_path): combination that approves anything without an explicit exploit_id, and a bare run (no --approve/--list-pending) never touches approve() at all.""" - args = run_campaign._parse_args(["--reports-dir", str(tmp_path)]) + args = run_campaign._parse_args( + ["--reports-dir", str(tmp_path), "--db-path", str(tmp_path / "exploits.sqlite3")] + ) assert args.approve is None assert args.list_pending is False +def test_run_mode_reports_dir_without_db_path_refuses_to_start(tmp_path): + """FIX 3 (documented flag combo aborts the run): --reports-dir without + --db-path in run mode used to only print a stderr NOTE and continue -- + then crash the campaign mid-loop (losing the action-log export) the + first time exploit-ID numbering collided with an already-persisted + report on a second run. It must now refuse to start at all.""" + with pytest.raises(SystemExit): + run_campaign._parse_args(["--reports-dir", str(tmp_path)]) + + # -- DO-NOT-MERGE cold review of PR #76, FIX 2 ------------------------------- # "The cross-check fails open." Four attacks the reviewer proved, each now # refused (fail closed): diff --git a/tools/run_campaign.py b/tools/run_campaign.py index 133f39a..73b68eb 100644 --- a/tools/run_campaign.py +++ b/tools/run_campaign.py @@ -40,11 +40,15 @@ Pass ``--reports-dir PATH`` to persist vuln reports (filed AND pending) durably, and ``--db-path PATH`` to persist the exploit DB durably (a -sqlite file, not ``:memory:``) -- **pair the two** if you want exploit IDs -to keep incrementing correctly across runs; ``--reports-dir`` alone with -the in-memory (default) db restarts exploit numbering at ``EXP-0001`` on -every invocation and will collide with (and refuse to re-file over) an -already-persisted report for that same id. +sqlite file, not ``:memory:``) -- **pair the two**: exploit IDs must keep +incrementing correctly across runs for a durable ``--reports-dir`` to work +at all. ``--reports-dir`` without ``--db-path`` in ``run`` mode is refused +at startup (cold-review fix, this PR) -- it used to be a mere stderr NOTE, +but the in-memory (default) db restarts exploit numbering at ``EXP-0001`` +on every invocation, which collides with (and previously crashed the whole +campaign mid-loop on, via an uncaught ``DocumentationAgentError`` from +``file_report``) an already-persisted report for that same id on any +second run against the same ``--reports-dir``. ## Approving a durably-pending report (issue #63/#66; hardened, cold-review of PR #76) @@ -181,6 +185,19 @@ def _parse_args(argv: Sequence[str] | None) -> argparse.Namespace: "against the source exploit record, or --unverified-i-vouch-without-db-check " "to explicitly approve without one" ) + # Cold-review fix (this PR, FIX 3): `run` mode (neither --list-pending nor + # --approve) with --reports-dir but no --db-path used to only print a + # stderr NOTE and continue -- but exploit IDs then restart at EXP-0001 + # every run, which collides with (and raises DocumentationAgentError for) + # an already-persisted report under --reports-dir, killing the campaign + # mid-loop the very first time it re-runs against a durable reports_dir. + # Refuse to start instead of documenting a footgun as a mere NOTE. + if not args.list_pending and args.approve is None and args.reports_dir is not None and args.db_path is None: + parser.error( + "--reports-dir without --db-path restarts exploit IDs at EXP-0001 every run " + "and will collide with an already-persisted report under --reports-dir on any " + "second run -- pass --db-path PATH too for a fully durable run" + ) return args @@ -325,13 +342,9 @@ def _cmd_run(args: argparse.Namespace) -> int: "it durably, then later run --approve EXPLOIT_ID --reports-dir PATH to approve it.", file=sys.stderr, ) - elif str(db_path) == ":memory:": - print( - "NOTE: --reports-dir is set but --db-path is not -- exploit IDs restart at " - "EXP-0001 every run and may collide with an already-persisted report under " - "--reports-dir. Pass --db-path PATH too for a fully durable run.", - file=sys.stderr, - ) + # (--reports-dir without --db-path is refused at argparse time, above -- + # see _parse_args's "FIX 3" comment -- so no db_path==':memory:' NOTE + # branch is reachable here anymore.) # A scratch path -- deliberately NOT under evals/recordings/ (that # directory is committed replay evidence, not a scratch/log dir; a live From 79594b050969ffa4ad034712b84fe1a1353dc77d Mon Sep 17 00:00:00 2001 From: franciszver <17106076+franciszver@users.noreply.github.com> Date: Sat, 25 Jul 2026 21:16:41 -0700 Subject: [PATCH 10/12] fix(P3.31 cold-review): approval can no longer overwrite already-approved evidence via a filename/report_id mismatch _load_persisted() keyed loaded reports by exploit_id taken from FILE CONTENT, never checking the filename or enforcing report_id uniqueness. Reproduced: a file named weird-name.pending-human-approval.json carrying report_id: VULN-0001, exploit_id: EXP-0002 caused --approve EXP-0002 to overwrite the already-filed, already-approved VULN-0001.json, and the stale source file was never removed (_remove_pending_file unlinked a path constructed from report_id, not the file's actual source path -- which never existed under that constructed name). Fix: - _load_persisted() now rejects (DocumentationAgentError, fail loud like every other load-time defect this method already catches) any persisted file whose name does not match "". - report_id uniqueness is enforced across all loaded reports -- two different exploit_ids claiming the same report_id is rejected even when both files are individually correctly named. - Each pending report's real source path is now tracked in _pending_paths (populated on load AND on file_report's own persist), and _remove_pending_file unlinks that tracked path -- not a path reconstructed from report_id -- so approval always removes the file that was actually read. Red-first: tests/redteam/test_documentation_agent.py (test_load_rejects_a_pending_file_whose_name_does_not_match_its_own_report_id, test_load_rejects_duplicate_report_id_across_different_exploit_ids, test_approve_removes_the_actual_source_path_not_a_report_id_guess) -- quoted failing output in this session: "DID NOT RAISE DocumentationAgentError" and "AttributeError: 'DocumentationAgent' object has no attribute '_pending_paths'" before the fix. --- redteam/agents/documentation.py | 74 ++++++++++++++-- tests/redteam/test_documentation_agent.py | 103 ++++++++++++++++++++++ 2 files changed, 171 insertions(+), 6 deletions(-) diff --git a/redteam/agents/documentation.py b/redteam/agents/documentation.py index 01bcea5..9c51e52 100644 --- a/redteam/agents/documentation.py +++ b/redteam/agents/documentation.py @@ -362,14 +362,42 @@ def __init__( self._reports_dir.mkdir(parents=True, exist_ok=True) self._filed: dict[str, dict[str, Any]] = {} self._pending: dict[str, dict[str, Any]] = {} + # Cold-review fix (this PR, FIX 4): the source path each PENDING + # report was actually loaded from (or persisted to, by this + # instance's own file_report()) -- see _remove_pending_file, which + # must unlink THIS path, not one reconstructed from report_id (a + # pending file's name is no longer trusted to equal + # "" without having been checked at load + # time -- see below). + self._pending_paths: dict[str, Path] = {} self._load_persisted() def _load_persisted(self) -> None: """Load every already-persisted report in ``reports_dir`` back into ``_filed``/``_pending`` (issue #63) -- see the module docstring's - "Loading" section for the full contract.""" + "Loading" section for the full contract. + + Cold-review fix (this PR, FIX 4): a report's ``report_id`` was + previously taken purely from FILE CONTENT, with the filename never + checked and ``report_id`` uniqueness never enforced across loaded + reports. Reproduced: a file named ``weird-name.pending-human- + approval.json`` whose CONTENT claims ``report_id: VULN-0001, + exploit_id: EXP-0002`` caused ``--approve EXP-0002`` to overwrite the + already-filed, already-approved ``VULN-0001.json`` -- and the stale + source file (``weird-name...``) was never removed, since + ``_remove_pending_file`` unlinked a path constructed from + ``report_id``, not the file's actual source path. Both are fixed + here: a persisted file whose name does not match + ```` is rejected (fail loudly, same as + every other load-time defect this method already refuses), a + ``report_id`` claimed by two different ``exploit_id``s is rejected, + and each pending report's real source path is tracked in + ``_pending_paths`` so approval unlinks the file that was actually + read, not a filename guess. + """ if self._reports_dir is None: return + seen_report_ids: dict[str, str] = {} # report_id -> the exploit_id that claimed it for path in sorted(self._reports_dir.glob("*.json")): try: data = json.loads(path.read_text(encoding="utf-8")) @@ -381,8 +409,25 @@ def _load_persisted(self) -> None: raise DocumentationAgentError(f"persisted report {path} is not a JSON object") self._validate(data) exploit_id = data["exploit_id"] - if path.name.endswith(PENDING_SUFFIX): + report_id = data["report_id"] + is_pending = path.name.endswith(PENDING_SUFFIX) + expected_name = f"{report_id}{PENDING_SUFFIX}" if is_pending else f"{report_id}.json" + if path.name != expected_name: + raise DocumentationAgentError( + f"persisted report {path} is named {path.name!r} but its own content " + f"claims report_id={report_id!r} (expected filename {expected_name!r}) -- " + "refusing to trust a report whose filename and content disagree" + ) + claimant = seen_report_ids.get(report_id) + if claimant is not None and claimant != exploit_id: + raise DocumentationAgentError( + f"report_id {report_id!r} is claimed by both exploit_id {claimant!r} and " + f"{exploit_id!r} under {self._reports_dir} -- report_id must be unique" + ) + seen_report_ids[report_id] = exploit_id + if is_pending: self._pending[exploit_id] = dict(data) + self._pending_paths[exploit_id] = path else: self._filed[exploit_id] = dict(data) # A filed report supersedes a stale pending leftover for the same @@ -392,6 +437,7 @@ def _load_persisted(self) -> None: for exploit_id in list(self._pending): if exploit_id in self._filed: del self._pending[exploit_id] + self._pending_paths.pop(exploit_id, None) def _validate(self, report: Mapping[str, Any]) -> None: errors = sorted(self._validator.iter_errors(report), key=lambda e: list(e.path)) @@ -418,11 +464,25 @@ def _persist(self, report: Mapping[str, Any], *, suffix: str = ".json") -> None: path = self._reports_dir / f"{report['report_id']}{suffix}" path.write_text(json.dumps(dict(report), indent=2), encoding="utf-8") - def _remove_pending_file(self, report_id: str) -> None: + def _remove_pending_file(self, report_id: str, exploit_id: str) -> None: + """Unlink the PENDING report's actual source path (cold-review fix, + this PR, FIX 4): tracked in ``_pending_paths`` at load/file time, not + reconstructed from ``report_id`` -- a pending file loaded from disk + is not guaranteed to be named ```` until + ``_load_persisted`` has already checked that (and rejected it if + not), but tracking the real path here is what actually deletes it + even so, rather than silently no-op'ing on a filename that was never + on disk in the first place.""" if self._reports_dir is None: return - path = self._reports_dir / f"{report_id}{PENDING_SUFFIX}" - path.unlink(missing_ok=True) + source_path = self._pending_paths.pop(exploit_id, None) + if source_path is None: + # No tracked source (shouldn't happen in practice -- every + # pending entry is either loaded via _load_persisted or + # persisted via file_report, both of which record it -- kept as + # a harmless fallback to the canonical path). + source_path = self._reports_dir / f"{report_id}{PENDING_SUFFIX}" + source_path.unlink(missing_ok=True) def file_report( self, @@ -455,6 +515,8 @@ def file_report( if report["requires_human_gate"]: self._pending[exploit_id] = report self._persist(report, suffix=PENDING_SUFFIX) + if self._reports_dir is not None: + self._pending_paths[exploit_id] = self._reports_dir / f"{report['report_id']}{PENDING_SUFFIX}" return {**report, "status": "pending_human_approval"} self._filed[exploit_id] = report @@ -489,7 +551,7 @@ def approve( # rather than neither, and _load_persisted's "filed wins" rule # self-heals the stale leftover on the next load (issue #63). self._persist(report) - self._remove_pending_file(report["report_id"]) + self._remove_pending_file(report["report_id"], exploit_id) return {**report, "status": "filed"} def get_filed(self, exploit_id: str) -> dict[str, Any] | None: diff --git a/tests/redteam/test_documentation_agent.py b/tests/redteam/test_documentation_agent.py index 7e89379..9f76ed2 100644 --- a/tests/redteam/test_documentation_agent.py +++ b/tests/redteam/test_documentation_agent.py @@ -324,6 +324,109 @@ def test_malformed_exploit_record_raises_documentation_agent_error_not_key_error build_vuln_report(incomplete, filed_at="2026-07-21T10:08:00Z") +# -- DO-NOT-MERGE cold review of PR #76, FIX 4 ------------------------------- +# "Approval can overwrite already-approved evidence." Reproduced: a +# weird-name.pending-human-approval.json carrying report_id: VULN-0001, +# exploit_id: EXP-0002 (filename ignored, contents trusted) caused +# --approve EXP-0002 to overwrite the filed, approved VULN-0001.json, and +# the stale source file was never removed (_remove_pending_file unlinked by +# report_id, not source path). + + +def test_load_rejects_a_pending_file_whose_name_does_not_match_its_own_report_id(tmp_path): + """The exact attack the reviewer proved: a hand-placed file named + something other than ``.pending-human-approval.json`` whose + CONTENT claims a report_id belonging to a different, already-filed and + already-approved report. Loading it must be a loud refusal, not a + silent acceptance that later collides on approve().""" + filer = DocumentationAgent(reports_dir=tmp_path) + filed = filer.file_report(NON_CRITICAL_EXPLOIT) + assert filed["status"] == "filed" + del filer + + on_disk_filed = json.loads((tmp_path / "VULN-0002.json").read_text(encoding="utf-8")) + assert on_disk_filed["clinical_impact"] != "ATTACKER-CONTROLLED OVERWRITE ATTEMPT" + + weird = { + "schema_version": "1.0.0", + "report_id": "VULN-0002", # claims the ALREADY-FILED report's id + "exploit_id": "EXP-0001", # under a DIFFERENT exploit_id + "severity": "critical", + "clinical_impact": "ATTACKER-CONTROLLED OVERWRITE ATTEMPT", + "observed": "n/a", + "expected": "n/a", + "remediation": "n/a", + "fix_validation_status": "not_validated", + "requires_human_gate": True, + "filed_at": "2026-07-25T00:00:00Z", + } + (tmp_path / "weird-name.pending-human-approval.json").write_text( + json.dumps(weird, indent=2), encoding="utf-8" + ) + + with pytest.raises(DocumentationAgentError, match="filename and content disagree"): + DocumentationAgent(reports_dir=tmp_path) + + # Refusing to load must not have touched anything already on disk. + still_on_disk = json.loads((tmp_path / "VULN-0002.json").read_text(encoding="utf-8")) + assert still_on_disk == on_disk_filed + assert (tmp_path / "weird-name.pending-human-approval.json").exists() + + +def test_load_rejects_duplicate_report_id_across_different_exploit_ids(tmp_path): + """Two DIFFERENT files can each individually pass the filename check + (each correctly named for its own claimed report_id) while still + colliding on report_id across different exploit_ids -- e.g. a filed + VULN-0001.json for EXP-0001 alongside a correctly-named + VULN-0001.pending-human-approval.json that claims EXP-0002. report_id + uniqueness must be enforced independently of the filename check.""" + filer = DocumentationAgent(reports_dir=tmp_path) + filer.file_report(NON_CRITICAL_EXPLOIT) # exploit_id EXP-0002 -> report_id VULN-0002, auto-filed + del filer + + colliding_pending = { + "schema_version": "1.0.0", + "report_id": "VULN-0002", # collides with the already-filed report above + "exploit_id": "EXP-0001", # but under a DIFFERENT exploit_id + "severity": "critical", + "clinical_impact": "collision", + "observed": "n/a", + "expected": "n/a", + "remediation": "n/a", + "fix_validation_status": "not_validated", + "requires_human_gate": True, + "filed_at": "2026-07-25T00:00:00Z", + } + # Correctly named for ITS OWN claimed report_id -- passes the filename + # check on its own. + (tmp_path / "VULN-0002.pending-human-approval.json").write_text( + json.dumps(colliding_pending, indent=2), encoding="utf-8" + ) + + with pytest.raises(DocumentationAgentError, match="report_id .* is claimed by both"): + DocumentationAgent(reports_dir=tmp_path) + + +def test_approve_removes_the_actual_source_path_not_a_report_id_guess(tmp_path): + """Even for a legitimately-loaded pending report, _remove_pending_file + must unlink the file that was actually loaded -- tracked by path, not + reconstructed from report_id -- so a stale source file is never left + behind after approval.""" + filer = DocumentationAgent(reports_dir=tmp_path) + filer.file_report(CRITICAL_EXPLOIT) + del filer + + approver = DocumentationAgent(reports_dir=tmp_path) + source_path = approver._pending_paths["EXP-0001"] + assert source_path == tmp_path / "VULN-0001.pending-human-approval.json" + assert source_path.exists() + + approver.approve("EXP-0001", approved_by="owner") + + assert not source_path.exists() + assert (tmp_path / "VULN-0001.json").exists() + + def test_all_categories_map_to_a_valid_severity_and_pass_schema(): for category in ( "prompt_injection", From d2a8ff613f14e56a233dbcf900fe4ac01e2f27af Mon Sep 17 00:00:00 2001 From: franciszver <17106076+franciszver@users.noreply.github.com> Date: Sat, 25 Jul 2026 21:19:33 -0700 Subject: [PATCH 11/12] fix(P3.31 cold-review): FIX 5 -- CLI-boundary tracebacks, doc precision, doc-count refresh - tools/run_campaign.py: --list-pending and --approve now catch DocumentationAgentError from DocumentationAgent(reports_dir=...) at the CLI boundary (clean stderr message + rc 1) instead of letting a raw traceback escape when reports_dir contains an unrelated/malformed JSON file. Red-first: tests/tools/test_run_campaign_cli.py (test_list_pending_on_a_directory_with_unrelated_json_fails_cleanly_not_a_traceback, test_approve_on_a_directory_with_unrelated_json_fails_cleanly_not_a_traceback). - contracts/README.md + observability_snapshot.schema.json: the changelog claimed "pre-#63 producers and consumers stay valid" for the new pending_human_triage_count field -- false for a consumer validating against its own pinned pre-#63 copy of the schema (additionalProperties: false, unchanged): it will reject any snapshot now carrying the field. Reworded to state the one-directional truth (producers stay valid; consumers must update their own schema copy first). - pending_human_triage_count is two different numbers under one name: the observability_snapshot field counts only the vuln_reports passed to emit_snapshot (this run's own accumulator), while --list-pending's identically-named printed key scans an entire --reports-dir on disk. Documented in the schema field description, redteam/observability/findings.py's docstring, AND docs/ARCHITECTURE.md's Observability Layer section (not just a Python docstring, per the brief). - docs/ATO_EVIDENCE_PACKET.md: reverted two stray Mermaid colour mutations that were collateral from a scripted replacement (#e05253 -> #e05252, #3b3590 -> #3b3520). - Documented the new CLI approval path (issue #63/#66) in docs/DEMO_SCRIPT.md as a new Beat 5, plus a "What this proves" bullet -- a new operator-facing approval path shipping undocumented at v3.0.0 is not acceptable. - Refreshed stale test-count claims in docs/ATO_EVIDENCE_PACKET.md and docs/DEMO_SCRIPT.md (359/253 -> 375/269) to match the live suite after this PR's new tests -- tests/test_doc_test_counts.py now passes again. (tools/run_campaign.py's --db-path/rebuild fixes for the untrusted requires_human_gate field and missing fix_validation_status were already fixed and tested as part of the FIX 2 commit.) --- contracts/README.md | 12 +++- .../v1/observability_snapshot.schema.json | 2 +- docs/ARCHITECTURE.md | 12 ++++ docs/ATO_EVIDENCE_PACKET.md | 28 ++++---- docs/DEMO_SCRIPT.md | 70 ++++++++++++++++--- redteam/observability/findings.py | 15 ++++ tests/tools/test_run_campaign_cli.py | 38 ++++++++++ tools/run_campaign.py | 12 +++- 8 files changed, 159 insertions(+), 30 deletions(-) diff --git a/contracts/README.md b/contracts/README.md index cba2d68..6729b31 100644 --- a/contracts/README.md +++ b/contracts/README.md @@ -32,8 +32,16 @@ initial cut. - Issue #63: `observability_snapshot.schema.json` gained an optional `pending_human_triage_count` property (durable count of reports still - awaiting human triage). Not added to `required`, so pre-#63 producers and - consumers stay valid — additive, stays `v1`. + awaiting human triage). Not added to `required`, so a pre-#63 PRODUCER + that omits the field stays schema-valid — additive, stays `v1`. **This + is one-directional, not a blanket "consumers stay valid" claim**: this + schema's `additionalProperties: false` (unchanged) means a pre-#63 + CONSUMER validating an incoming snapshot against its OWN pinned copy of + this schema will reject any snapshot that now carries the new field. A + consumer must update its own copy of the schema (or relax + `additionalProperties`) before it can accept a post-#63 producer's + output — see the field's own `description` in the schema for the + cross-reference to `--list-pending`'s differently-scoped same-named key. ## Schemas (one per edge in ARCHITECTURE.md §2) diff --git a/contracts/v1/observability_snapshot.schema.json b/contracts/v1/observability_snapshot.schema.json index 50d92c0..f86d726 100644 --- a/contracts/v1/observability_snapshot.schema.json +++ b/contracts/v1/observability_snapshot.schema.json @@ -32,7 +32,7 @@ } }, "open_high_sev_count": { "type": "integer", "minimum": 0 }, - "pending_human_triage_count": { "type": "integer", "minimum": 0, "description": "Reports awaiting human triage right now (issue #63) -- additive v1 field, not in 'required' so pre-#63 producers/consumers stay valid." }, + "pending_human_triage_count": { "type": "integer", "minimum": 0, "description": "How many of the vuln_reports passed to emit_snapshot() are still awaiting human triage (issue #63) -- NOT a directory-wide count: `tools/run_campaign.py --list-pending` prints the same key name but scans an entire --reports-dir, which can differ from this field's value (see docs/ARCHITECTURE.md's Observability Layer section). Additive v1 field, not in 'required' -- a pre-#63 PRODUCER that omits it stays schema-valid, but a pre-#63 CONSUMER validating incoming snapshots against an unmodified copy of this schema (additionalProperties: false, unchanged) will reject any snapshot that now includes it." }, "cost": { "type": "object", "additionalProperties": false, diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index 5b6c8c4..861293c 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -208,6 +208,18 @@ correctly kept separate. signals are read programmatically, not just rendered for a human), in addition to being a human-facing dashboard. + **`pending_human_triage_count` names two different scopes (issue #63).** + `observability_snapshot.schema.json`'s `pending_human_triage_count` + field (`redteam.observability.findings.pending_human_triage_count`) + counts only the `vuln_reports` sequence a caller passes to + `emit_snapshot` — in the live campaign loop, everything filed/pending + so far in *that one run*. `tools/run_campaign.py --list-pending` prints + a line using the identical key name, `pending_human_triage_count=N`, + but that N is a directory-wide scan of an entire `--reports-dir` on + disk, independent of any one run. The two numbers can legitimately + differ — do not assume the snapshot field and the CLI's printed line + agree. + ## 4. Fully-local model strategy (decided) The owner's decision, locked in `planning/PLAN.md` and reaffirmed here as diff --git a/docs/ATO_EVIDENCE_PACKET.md b/docs/ATO_EVIDENCE_PACKET.md index 8ffd6f4..08f7aaf 100644 --- a/docs/ATO_EVIDENCE_PACKET.md +++ b/docs/ATO_EVIDENCE_PACKET.md @@ -100,10 +100,10 @@ flowchart TB ZoneA -.->|"crosses network boundary only to
localhost target, never external"| Egress ZoneB -.->|"zero external calls"| Egress - style ZoneA fill:#3b1f1f,stroke:#e05253,color:#f5e5e5 + style ZoneA fill:#3b1f1f,stroke:#e05252,color:#f5e5e5 style ZoneB fill:#1f2a3b,stroke:#5289e0,color:#e5edf5 style Store fill:#1f3b2a,stroke:#52e089,color:#e5f5ec - style TargetBoundary fill:#3b3590,stroke:#e0c552,color:#f5f0e5 + style TargetBoundary fill:#3b3520,stroke:#e0c552,color:#f5f0e5 style Egress fill:#2a1f3b,stroke:#8a52e0,color:#ede5f5 ``` @@ -326,9 +326,9 @@ explicitly documented as an arbitrary placeholder accepted by the target's own insecure-by-default validator (VULN-0001) — "safe to publish as-is" per that document's own text, not a real credential. -`pytest tests/ -q` re-run for this packet: **359 passed** with the sibling +`pytest tests/ -q` re-run for this packet: **375 passed** with the sibling Phase 2 checkout (`../agentforge-2-evidence-agent`, pinned `v2.0.0`) -present locally (confirmed at PR time); **253 passed, 106 skipped** in CI +present locally (confirmed at PR time); **269 passed, 106 skipped** in CI and for anyone without that sibling — CI (`.github/workflows/ci.yml`) does not check it out, so the 100 total sibling-checkout citation cases class-skip cleanly there: 40 `TestTraceCitationsAgainstPinnedTarget` cases @@ -373,7 +373,7 @@ those changes included, not a pre-change baseline. evidence the project has previously demonstrated this discipline under pressure, not as a claim about this PR's own diff (which touches no secret-adjacent files). -- **359 passing tests (253 passed, 106 skipped in CI), no live/network/GPU +- **375 passing tests (269 passed, 106 skipped in CI), no live/network/GPU call in the default suite.** Every test file under `tests/` (`tests/contracts/`, `tests/redteam/`, `tests/test_cases.py`, `tests/test_case_sourceref_relevance.py`, `tests/test_runner_sse.py`, @@ -395,18 +395,18 @@ those changes included, not a pre-change baseline. has moved across PRs that touch test-suite-relevant code (e.g. PR #40's own test plan: "177 passed (unchanged; no test-suite-relevant code touched)" at that point in the repo's history; this PR's own platform - changes plus its expanded citation-verification test set move it to 359 - with the sibling checkout present, or 253 passed / 106 skipped without + changes plus its expanded citation-verification test set move it to 375 + with the sibling checkout present, or 269 passed / 106 skipped without it, §5.1). --- ## 5. Eval-result evidence -### 5.1 The 359-test suite (253 in CI) +### 5.1 The 375-test suite (269 in CI) -`pytest tests/ -q` → **359 passed** with the sibling Phase 2 checkout -present, re-confirmed for this packet (§4.1); **253 passed, 106 skipped** +`pytest tests/ -q` → **375 passed** with the sibling Phase 2 checkout +present, re-confirmed for this packet (§4.1); **269 passed, 106 skipped** in CI (`.github/workflows/ci.yml` does not check out the sibling target) and for any clone lacking it. Organized across `tests/contracts/` (schema + uniqueness constraints), `tests/redteam/` (the six agents + campaign @@ -518,8 +518,8 @@ to approve and nothing already filed. suspected halts new directives; an empty-completion error is skipped, not fatal (this is §6's postmortem subject); `max_iterations` input validation. Test count: 163 baseline → 171 (PR #35's own reported delta; - the repo has since grown to 359 total with the sibling checkout present, - or 253 passed / 106 skipped without it, §5.1). + the repo has since grown to 375 total with the sibling checkout present, + or 269 passed / 106 skipped without it, §5.1). ### 5.4 Load-test numbers @@ -639,8 +639,8 @@ describes — not because it was dramatic. (Mermaid diagram, trust-zone framing), §2 Auth model (platform + target), §3 Versioned dependency list (`requirements-contracts.txt`, contracts versioning, model runtimes), §4 Self-scan results (commands run + process - evidence), §5 Eval-result evidence (359 tests with the sibling checkout - present / 253 passed, 106 skipped in CI, 3 criticals, live-campaign + evidence), §5 Eval-result evidence (375 tests with the sibling checkout + present / 269 passed, 106 skipped in CI, 3 criticals, live-campaign evidence, load-test numbers), §6 Sample incident and postmortem. - **Every section cites a real, already-committed artifact**, not an invented one: `docs/ARCHITECTURE.md`, `docs/THREAT_MODEL.md`, diff --git a/docs/DEMO_SCRIPT.md b/docs/DEMO_SCRIPT.md index a80c6e6..ffb305a 100644 --- a/docs/DEMO_SCRIPT.md +++ b/docs/DEMO_SCRIPT.md @@ -1,9 +1,11 @@ # Demo Script — AgentForge Phase 3 Red-Team Platform -P3.20 (issue #44). A reproducible, copy-pasteable walkthrough of the -platform end-to-end, at four beats: the loop finding a vuln, the Judge -confirming it, the regression harness catching a **reintroduced** fixed -exploit, and one graceful failure. Every command below was run against this +P3.20 (issue #44); Beat 5 added in the cold-review fix to PR #76 (issue +#63). A reproducible, copy-pasteable walkthrough of the platform +end-to-end, at five beats: the loop finding a vuln, the Judge confirming +it, the regression harness catching a **reintroduced** fixed exploit, one +graceful failure, and approving a durably-pending report from the CLI. +Every command below was run against this repo at `v2.0.0` while writing this doc; outputs are pasted verbatim where noted. See `docs/ARCHITECTURE.md` §2 for the component interaction diagram this script drives, and `docs/ATO_EVIDENCE_PACKET.md` §5 for the underlying @@ -20,9 +22,9 @@ evidence table this script complements with runnable commands. immediately before and after any live call and confirm VRAM stays flat. - `pytest tests/ -q` green (deterministic — no live/network/GPU call in the default suite; confirmed while writing this doc). The printed count is - environment-dependent: **359 passed** when the sibling Phase 2 checkout + environment-dependent: **375 passed** when the sibling Phase 2 checkout (`../agentforge-2-evidence-agent`, pinned `v2.0.0`) is present locally; - **253 passed, 106 skipped** in CI and for anyone cloning this repo without + **269 passed, 106 skipped** in CI and for anyone cloning this repo without that sibling (the 106 skipped are `TestTraceCitationsAgainstPinnedTarget` (40 cases, `tests/test_dos_input_bound_resolution.py`) plus `TestCitationsAgainstPinnedTargets` (60 cases, @@ -33,7 +35,7 @@ evidence table this script complements with runnable commands. ``` $ pytest tests/ -q -359 passed in 2.38s # with the sibling Phase 2 checkout present +375 passed in 2.38s # with the sibling Phase 2 checkout present ``` --- @@ -299,6 +301,45 @@ here for completeness: --- +## Beat 5 — Approving a durably-pending report from the CLI (issue #63/#66) + +Every pending report Beat 1–4's loop files (critical severity, or a +`denial_of_service` finding — see the "one human touchpoint" note below) +now survives the filing process exiting when `--reports-dir PATH` is +passed to `tools/run_campaign.py`, and can be approved later by a +completely separate invocation, no bespoke per-report script: + +``` +python tools/run_campaign.py --list-pending --reports-dir PATH +python tools/run_campaign.py --approve EXP-0004 --reports-dir PATH --db-path PATH --approved-by NAME +``` + +`--approve` fails closed by design: `--db-path` must already name an +existing sqlite file holding the original exploit record for that +`exploit_id` (a typo'd or not-yet-created path is a hard refusal, exit 1 — +it is never silently created as an empty DB), and `--approved-by` has no +default — an explicit human identity is the point of a human-approval +gate. The pending report's full body is printed to stdout before it is +stamped, so approval is an informed act, not a blind exploit_id lookup. +For a genuinely DB-less pending report, the explicit +`--unverified-i-vouch-without-db-check` flag skips the cross-check with a +loud stderr `WARNING` naming exactly what was skipped — there is no quiet +way to bypass the check. + +``` +pytest tests/tools/test_run_campaign_cli.py -v +``` + +Exercises the full list → approve round trip end-to-end (file a pending +report with one `DocumentationAgent` instance, `del` it to simulate that +process exiting, then list and approve it with only the CLI against a +fresh instance) plus every fail-closed path: no `--db-path`/no escape +hatch, a `--db-path` naming a file that doesn't exist yet, no +`--approved-by`, and a tampered/drifted pending artifact that no longer +matches its source exploit record. + +--- + ## What this proves - **The loop finds real vulnerabilities autonomously**: `run_campaign` @@ -328,20 +369,27 @@ here for completeness: `DocumentationAgent`'s critical-severity report gate (`VULN-0003`'s own `"requires_human_gate": true` / `"approved_by": "owner"`), never a loop restart. +- **The human-approval touchpoint is durable and fails closed**: a report + filed pending by one process survives that process exiting + (`--reports-dir PATH`) and is approvable later by a completely separate + CLI invocation (`tools/run_campaign.py --approve`, Beat 5), with no + silent path to approving unverified content — a missing/typo'd + `--db-path`, a missing `--approved-by`, or a tampered/drifted pending + artifact are all hard refusals, not warnings. ## CI CI (`.github/workflows/ci.yml`) runs the deterministic suite — `python -m pytest tests/ -q` — on every push to `main` and on every pull request. CI does not check out the sibling Phase 2 target, so its printed -count is **253 passed, 106 skipped** (the 106 skipped are +count is **269 passed, 106 skipped** (the 106 skipped are `TestTraceCitationsAgainstPinnedTarget` (40, issue #25/#54), `TestCitationsAgainstPinnedTargets` (60, issue #58), and `TestStandingUpTargetPathsExistInPinnedTarget` (6, issue #61), all of which class-skip cleanly when `../agentforge-2-evidence-agent` is absent). Live-model and target-stack runs remain manual, outside CI: every command in this script was run locally against the dev stack while writing this doc, with the sibling -checkout present, giving **359 passed**. `pytest tests/ -q` is still the +checkout present, giving **375 passed**. `pytest tests/ -q` is still the reproducibility bar — re-run it after pulling this branch to confirm -nothing here has drifted: expect **359 passed** if you have the sibling -Phase 2 checkout at `v2.0.0`, or **253 passed, 106 skipped** if you don't. +nothing here has drifted: expect **375 passed** if you have the sibling +Phase 2 checkout at `v2.0.0`, or **269 passed, 106 skipped** if you don't. diff --git a/redteam/observability/findings.py b/redteam/observability/findings.py index 7c5c0a2..516ed16 100644 --- a/redteam/observability/findings.py +++ b/redteam/observability/findings.py @@ -63,6 +63,21 @@ def pending_human_triage_count(vuln_reports: Sequence[Mapping[str, Any]] = ()) - carry a "status" key at all) and from a campaign run's ``all_vuln_reports`` list alike. ``()`` -- no reports known -- yields 0, the same honest-default convention ``open_high_sev_count`` uses. + + **Not the same number as ``tools/run_campaign.py --list-pending``'s** + ``pending_human_triage_count=N`` **line, despite the identical key + name** (cold-review fix, this PR): this function counts only the + ``vuln_reports`` sequence the CALLER passes to ``emit_snapshot`` (in + the live campaign loop, everything filed/pending so far in THIS run), + while ``--list-pending`` scans an entire ``--reports-dir`` on disk, + directory-wide, independent of any one run. The two can legitimately + differ (e.g. reports left pending by an earlier run, or reports + outside this run's own ``vuln_reports`` accumulator). See + ``contracts/v1/observability_snapshot.schema.json``'s own + ``pending_human_triage_count`` field description and + ``docs/ARCHITECTURE.md``'s Observability Layer section for the + documented limitation -- this is not merely a Python-docstring-only + caveat. """ return sum( 1 diff --git a/tests/tools/test_run_campaign_cli.py b/tests/tools/test_run_campaign_cli.py index ee328fc..2695036 100644 --- a/tests/tools/test_run_campaign_cli.py +++ b/tests/tools/test_run_campaign_cli.py @@ -162,6 +162,44 @@ def test_approve_refuses_when_pending_report_drifts_from_its_source_exploit_reco assert not (reports_dir / "VULN-0001.json").exists() +# -- DO-NOT-MERGE cold review of PR #76, FIX 5 (partial) -------------------- + + +def test_list_pending_on_a_directory_with_unrelated_json_fails_cleanly_not_a_traceback(tmp_path, capsys): + """--list-pending on a reports_dir containing an unrelated JSON file + previously exited with a raw DocumentationAgentError traceback (an + uncaught exception) instead of a clean CLI message + rc 1.""" + reports_dir = tmp_path / "vuln_reports" + reports_dir.mkdir() + (reports_dir / "random.json").write_text('{"unrelated": true}', encoding="utf-8") + + rc = run_campaign.main(["--list-pending", "--reports-dir", str(reports_dir)]) + assert rc == 1 + err = capsys.readouterr().err + assert "could not load reports_dir" in err + + +def test_approve_on_a_directory_with_unrelated_json_fails_cleanly_not_a_traceback(tmp_path, capsys): + reports_dir = tmp_path / "vuln_reports" + reports_dir.mkdir() + (reports_dir / "random.json").write_text('{"unrelated": true}', encoding="utf-8") + + rc = run_campaign.main( + [ + "--approve", + "EXP-0001", + "--reports-dir", + str(reports_dir), + "--approved-by", + "owner", + "--unverified-i-vouch-without-db-check", + ] + ) + assert rc == 1 + err = capsys.readouterr().err + assert "could not load reports_dir" in err + + def test_never_auto_approves_no_default_exploit_id(tmp_path): """Regression guard: --approve has no default -- there is no flag combination that approves anything without an explicit exploit_id, and diff --git a/tools/run_campaign.py b/tools/run_campaign.py index 73b68eb..c31d9b3 100644 --- a/tools/run_campaign.py +++ b/tools/run_campaign.py @@ -202,7 +202,11 @@ def _parse_args(argv: Sequence[str] | None) -> argparse.Namespace: def _cmd_list_pending(args: argparse.Namespace) -> int: - documentation = DocumentationAgent(reports_dir=args.reports_dir) + try: + documentation = DocumentationAgent(reports_dir=args.reports_dir) + except DocumentationAgentError as exc: + print(f"could not load reports_dir={args.reports_dir}: {exc}", file=sys.stderr) + return 1 pending = documentation.all_pending() print(f"pending_human_triage_count={len(pending)} (reports_dir={args.reports_dir})") for report in pending: @@ -229,7 +233,11 @@ def _cmd_approve(args: argparse.Namespace) -> int: than a warning that silently approves as-is -- a typo'd path must not be the thing that downgrades the one safety flag to a no-op. """ - documentation = DocumentationAgent(reports_dir=args.reports_dir) + try: + documentation = DocumentationAgent(reports_dir=args.reports_dir) + except DocumentationAgentError as exc: + print(f"could not load reports_dir={args.reports_dir}: {exc}", file=sys.stderr) + return 1 pending = documentation.get_pending(args.approve) if pending is None: print( From 2e68b5dbaa38e0da8f65305c2dcb175a66fa483f Mon Sep 17 00:00:00 2001 From: franciszver <17106076+franciszver@users.noreply.github.com> Date: Sat, 25 Jul 2026 21:21:07 -0700 Subject: [PATCH 12/12] refactor(P3.31 declutter): fold duplicated CLI reports_dir-load try/except; drop dead pre_approval assignment tools/run_campaign.py: _cmd_list_pending and _cmd_approve had identical try/except DocumentationAgentError blocks around DocumentationAgent construction -- folded into a shared _load_documentation(args) helper. tools/approve_vuln_0004.py: pyflakes flagged pre_approval as assigned but never used after the FIX 1 re-derivation change (main() no longer compares against it). The already_loaded branch's assignment was pure dead computation; the _file_pending() call in the other branch is kept for its side effect (populating documentation._pending) with the return value simply no longer captured. No behavior change -- full suite still 375 passed. --- tools/approve_vuln_0004.py | 32 +++++++++++++++++--------------- tools/run_campaign.py | 22 ++++++++++++++++------ 2 files changed, 33 insertions(+), 21 deletions(-) diff --git a/tools/approve_vuln_0004.py b/tools/approve_vuln_0004.py index 908fe21..9f6b305 100644 --- a/tools/approve_vuln_0004.py +++ b/tools/approve_vuln_0004.py @@ -152,19 +152,21 @@ def main() -> int: # (no longer reachable in practice, but harmless to keep) construction # that somehow didn't load it. already_loaded = documentation.get_pending(_EXPLOIT_ID) - if already_loaded is not None: - # Cold-review fix (this PR): ``already_loaded`` is read straight back - # off ``_PENDING_PATH`` by ``DocumentationAgent.__init__``'s auto-load - # -- it is THE SAME FILE this script is trying to authenticate, not - # independent evidence. It is used ONLY below to satisfy - # ``approve()``'s in-memory precondition (the exploit_id must be a - # key in ``documentation._pending``); it must never be the left-hand - # side of the field-for-field comparison, or the check degenerates - # into comparing the on-disk file against itself, which always - # passes regardless of tampering. - pre_approval = {**already_loaded, "status": "pending_human_approval"} - else: - pre_approval = _file_pending( + if already_loaded is None: + # Cold-review fix (this PR): the return value is deliberately + # unused -- ``_file_pending``'s only job here is its SIDE EFFECT + # (populating ``documentation``'s in-memory ``_pending`` so + # ``approve()`` below has something to pop). Below, the pending + # report is re-derived independently via ``build_vuln_report`` + # directly from ``record`` and compared against what is on disk -- + # never against this call's return value, nor against + # ``already_loaded`` (which, when present, is read straight back + # off ``_PENDING_PATH`` by ``DocumentationAgent.__init__``'s + # auto-load -- THE SAME FILE this script is trying to authenticate, + # not independent evidence; using it as the comparison target would + # degenerate into comparing the on-disk file against itself, which + # always passes regardless of tampering). + _file_pending( documentation, record, filed_at=original_filed_at, @@ -174,8 +176,8 @@ def main() -> int: # Authoritative re-derivation: rebuild what the pending report SHOULD be # directly from the re-derived ``record`` via ``build_vuln_report`` -- - # NOT from ``pre_approval``/``already_loaded`` above, which (when the - # auto-load path is taken, i.e. in every real re-run) is itself read off + # NOT from ``already_loaded`` above, which (when the auto-load path is + # taken, i.e. in every real re-run) is itself read straight back off # ``_PENDING_PATH``. Comparing THIS reconstruction against what is # already committed on disk is what makes this a real cross-check: it # fires on any FIELD-VALUE drift between the artifact and what the diff --git a/tools/run_campaign.py b/tools/run_campaign.py index c31d9b3..e1aed9c 100644 --- a/tools/run_campaign.py +++ b/tools/run_campaign.py @@ -201,11 +201,23 @@ def _parse_args(argv: Sequence[str] | None) -> argparse.Namespace: return args -def _cmd_list_pending(args: argparse.Namespace) -> int: +def _load_documentation(args: argparse.Namespace) -> DocumentationAgent | None: + """Construct a ``DocumentationAgent`` over ``args.reports_dir``, or print + a clean CLI-boundary message and return ``None`` on a load failure + (e.g. a malformed/unrelated JSON file under ``reports_dir``) instead of + letting ``DocumentationAgentError`` escape as a raw traceback -- shared + by ``--list-pending`` and ``--approve``, both of which used to duplicate + this same try/except.""" try: - documentation = DocumentationAgent(reports_dir=args.reports_dir) + return DocumentationAgent(reports_dir=args.reports_dir) except DocumentationAgentError as exc: print(f"could not load reports_dir={args.reports_dir}: {exc}", file=sys.stderr) + return None + + +def _cmd_list_pending(args: argparse.Namespace) -> int: + documentation = _load_documentation(args) + if documentation is None: return 1 pending = documentation.all_pending() print(f"pending_human_triage_count={len(pending)} (reports_dir={args.reports_dir})") @@ -233,10 +245,8 @@ def _cmd_approve(args: argparse.Namespace) -> int: than a warning that silently approves as-is -- a typo'd path must not be the thing that downgrades the one safety flag to a no-op. """ - try: - documentation = DocumentationAgent(reports_dir=args.reports_dir) - except DocumentationAgentError as exc: - print(f"could not load reports_dir={args.reports_dir}: {exc}", file=sys.stderr) + documentation = _load_documentation(args) + if documentation is None: return 1 pending = documentation.get_pending(args.approve) if pending is None: