Skip to content

P3.37: harden recording_ref derivation and widen the doc guard - #80

Merged
franciszver merged 5 commits into
mainfrom
fix/p3-37-harden-recording-ref
Jul 26, 2026
Merged

P3.37: harden recording_ref derivation and widen the doc guard#80
franciszver merged 5 commits into
mainfrom
fix/p3-37-harden-recording-ref

Conversation

@franciszver

Copy link
Copy Markdown
Owner

Closes #79

Small, surgical hardening round from the focused re-review of PR #78. Three one-line-scoped fixes.

FIX 1 — directory-shaped recording_ref now fails loudly

_recording_ref_for (redteam/agents/documentation.py) previously took parts[-2] unconditionally, so a directory-shaped ref (with or without a trailing slash) silently produced a schema-valid but WRONG pointer:

"evals/recordings/identity-authz-garbage-bearer-token/"  ->  'evals/recordings/recordings/'
"evals/recordings/identity-authz-garbage-bearer-token"   ->  'evals/recordings/recordings/'
"evals/recordings/../secrets/d.json"                     ->  'evals/recordings/secrets/'

Now rejects (raises DocumentationAgentError):

  • a ref ending in / (directory-shaped)
  • a ref whose last segment lacks a .json suffix
  • a ref containing a .. path segment

Does NOT re-anchor on a literal recordings segment or relativise against the repo root — that was PR #78's own fix, deliberately preserved (see the docstring, which explains why: tools/load_test_replay.py's scratch tempdir breaks that invariant). Regression-checked:

$ python tools/load_test_replay.py --iterations 5
exploit_ids = 5
signals     = 0

No regression — 0/3-filed-reports failure mode from re-anchoring does NOT reappear.

FIX 2 — doc guard now catches the 17-char phrasing it was written for

tests/test_ato_evidence_index.py's _NO_RECORDING_REF_CLAIM_RE allowed only 10 chars between "has no" and "recording_ref", missing the original docs/ARCHITECTURE.md:77 phrasing (17 chars: deliberately has no `minimal_repro`/`recording_ref` field). Widened to .{0,40} (matching the carries no branch).

A bare widen produced a false positive against redteam/agents/documentation.py's own section heading ("...has no ``minimal_repro.steps``, and how ## ``recording_ref`` works..."), so both branches now also require field within 10 chars after recording_ref — this matches every real "stale claim" phrasing (which all end in "...recording_ref` field") without matching the heading (which says "works", not "field").

Mutation-tested against all three original (pre-PR-#78) ARCHITECTURE.md phrasings:

  • :74-77 (prose summary): now caught (was missed before)
  • :182-187 (component list): legitimately out of scope — this location's original text only says "the schema has no minimal_repro field" and never claims recording_ref is absent, so it was never a stale claim
  • :318 (AI-use table): still caught (was already caught before, unaffected)

docs/ARCHITECTURE.md restored byte-identical after mutation testing (git status --porcelain clean throughout).

FIX 3 — portable regex anchor in the schema

contracts/v1/vuln_report.schema.json's recording_ref pattern used \Z, correct under Python's re (which jsonschema's Draft202012Validator uses) but not portable ECMA-262 (JSON Schema's mandated dialect) — \Z there matches a literal Z, and Go's RE2 rejects it outright. Switched to (?![\s\S]), a portable negative-lookahead with identical semantics in both dialects. Verified byte-for-byte equivalent to \Z under this repo's jsonschema (4.26.0) for every probed case (bare end, trailing \n, trailing space, no trailing slash) — a trailing newline is still rejected. Extended the in-schema comment to record the rationale for future editors.

Docs

docs/ATO_EVIDENCE_PACKET.md / docs/DEMO_SCRIPT.md test-count claims updated 389→392 (with sibling checkout) / 283→286 passed (CI shape), for the net +3 tests from FIX 1's red-first suite (tests/test_doc_test_counts.py enforces this). docs/RELEASE_NOTES_v3.0.0.md untouched (open PR #72).

Verification

  • Local: 392 passed (was 389 baseline; +3 from FIX 1's tests)
  • CI shape: 286 passed, 106 skipped (was 283/106 baseline)
  • docs/vuln_reports/*.json (4 files): byte-identical before/after — git diff empty, sha256 unchanged
  • Three inline gates run (declutter → sec-audit → deep-review): no BLOCKER/MAJOR findings; one MINOR noted (FIX 2's widened regex has no dedicated regression test, only manual mutation verification — matches pre-existing coverage model, not a regression)

Test plan

  • Red-first: 3 new tests for directory-with-slash, directory-without-slash, .. segment (quoted failing output in commit b8f8e3b)
  • python -m pytest -q → 392 passed
  • python tools/load_test_replay.py --iterations 5 → exploit_ids=5, signals=0 (no regression)
  • Mutation-tested all three original ARCHITECTURE.md phrasings; file restored clean
  • Verified (?![\s\S]) byte-equivalent to \Z under jsonschema 4.26.0

…ding_ref phrasing (#79)

Widened window from 10 to 40 chars (matching the sibling carries-no
branch) so the original ARCHITECTURE.md:77 phrasing (17 intervening
chars) is caught. Also required 'field' within 10 chars after
recording_ref on both branches -- an unqualified 0-40 widen produced a
false positive against documentation.py's own section heading ('has no
minimal_repro.steps, and how ## recording_ref works'), which isn't a
stale-claim at all. Mutation-verified against all three original
ARCHITECTURE.md phrasings (see PR description).
…A-262 lookahead (#79)

'\Z' is correct under Python re (which jsonschema's Draft202012Validator
uses) but is not portable JSON Schema: Draft 2020-12 mandates ECMA-262
regex semantics, under which '\Z' matches a literal 'Z' character, and
Go's RE2 rejects it outright. Switched to '(?![\s\S])', a portable
negative-lookahead anchor with identical semantics in both dialects --
verified byte-for-byte equivalent to '\Z' under this repo's jsonschema
for bare end, trailing newline, trailing space, and no-trailing-slash
cases. Only Python consumers exist today, but this keeps the contract
itself dialect-neutral.
…hardening round (#79)

389 -> 392 (with sibling checkout), 283 -> 286 passed (CI shape), per
tests/test_doc_test_counts.py. Net +3 from FIX 1's three red-first tests
(directory-with-slash, directory-without-slash, .. segment); FIX 2 and
FIX 3 added no new tests.
@franciszver
franciszver merged commit 73febf8 into main Jul 26, 2026
1 check passed
@franciszver
franciszver deleted the fix/p3-37-harden-recording-ref branch July 26, 2026 06:15
franciszver added a commit that referenced this pull request Jul 26, 2026
Red-first: extends tests/test_release_notes.py with 13 new failing
assertions covering the ten corrections from the final overclaim review,
then rewrites docs/RELEASE_NOTES_v3.0.0.md to satisfy them:

BLOCKERS:
1. Quotes planning/KICKOFF_PROMPT.md's HARD CONSTRAINT in full, including
   "A single-agent or linear pipeline FAILS the assignment" -- previously
   elided without an ellipsis. Stops calling "(separate process/context)"
   a parenthesised aside; it is the brief's own operational definition of
   architectural independence.
2. Issue #73 is CLOSED (a documentation-correction issue, not an
   implementation tracker) -- reworded to state the gap was raised and
   resolved at the documentation level, and OS-process isolation is not
   currently scheduled, rather than implying #73 tracks future work.
3. Scopes the upstream #130 argument to VULN-0002 (document_citations: []
   matches #130's Ask exactly); VULN-0003 is described as the
   alongside-a-DocumentCitation shape #130 pre-emptively called "harmless
   today", with the stronger argument for why that premise doesn't hold
   for this shape either (mirrors the correction already posted to
   upstream #170).
4. VULN-0001 is scoped to the shipped default
   (copilot_per_user_token_enabled=False) -- a real introspection
   validator replaces the permissive default when the flag is True.
   VULN-0004 is unconditional (no flag gates it).
5. Corrects "the only path any test exercises" -- test_judge_agent.py:236
   constructs JudgeAgent(scorer=corrupting_scorer) for a drift test.

NON-BLOCKING:
6. Qualifies "after these findings were filed" -- true for three of four;
   VULN-0004 was filed later the same day v2.1.0 was tagged.
7. Re-cites judge.py:44-53 (the scorer-seam docstring paragraph) instead
   of the mis-ranged :44-47.
8. Corrects "scans red_team.py the same way" -- the Judge-side AST scan
   resolves relative imports; the Red-Team-side scan checks absolute
   module paths only, so `from .judge import X` would pass undetected.
9. Adds "(temperature 0)" to the 3/3 draw-count claim, matching what the
   upstream filings state.
10. Fixes a quoted word substitution: config.py:251 says "mechanism", not
    "alternative".

Also, consequences of #77/#79 merging into main during the rebase:
- vuln reports are now 14-field artifacts (recording_ref added); updated
  the enumeration and field count everywhere they're cited.
- Each report now names its own evals/recordings/ directory via
  recording_ref; ATO §5.2 remains the human-readable index, not the sole
  resolution path.
- Restated the exploit_id limitation precisely: exploit_id alone still
  only resolves in-process, but the evidence trail no longer depends on
  that join now that recording_ref is on the artifact itself.
- Re-derived test counts post-rebase across RELEASE_NOTES/
  ATO_EVIDENCE_PACKET/DEMO_SCRIPT: 435 passed locally (sibling present),
  329 passed / 106 skipped in CI.

Rebase note: docs/ATO_EVIDENCE_PACKET.md and docs/DEMO_SCRIPT.md conflicts
were resolved by taking main's post-#78/#79/#80 versions (both merges'
own count bumps), then re-deriving counts fresh rather than carrying any
number from either side forward.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

P3.37: harden recording_ref derivation and widen the doc guard (latent, from PR #78 review)

1 participant