Repository navigation
Add a sanitized tool-loop replay corpus - #2182
Aaronontheweb wants to merge 4 commits into
Conversation
|
|
||
|
|
||
| def _token(value: Any, location: str) -> str: | ||
| if not isinstance(value, str) or not _TOKEN.fullmatch(value) or _PRIVATE_VALUE.search(value): |
There was a problem hiding this comment.
[P1] The privacy check uses a denylist, not a closed symbolic vocabulary. Any identity or host token that lacks the listed substrings passes _token, and extract_sanitized_case accepts it. The static test covers only five marker strings. This does not prove the stated no-identity and no-host boundary. Please use a closed token vocabulary or an independently reviewed redaction manifest, then add tests for private values that lack denylist markers.
There was a problem hiding this comment.
[Addressed] The validator now accepts only four-digit symbolic namespaces for case, group, tool, argument, and result tokens. The updated tests reject non-numeric names. My independent privacy checks rejected each such mutation at revision 67c7981.
| "candidate_dispatch_count": candidate_dispatch_count, | ||
| "completed_event_count": completed_event_count, | ||
| "first_intervention_request_index": case["first_intervention_request_index"], | ||
| "passed": decisions == expected, |
There was a problem hiding this comment.
[P1] The replay compares its computed decisions with expected_decision values stored in the same fixture. The source fingerprint protects only the C# file; it does not independently verify this Python algorithm. A bug in action hashing, outcome hashing, ordering, boundary handling, or intervention state can pass after the expected values change with the bug. The 12-of-12 result is therefore self-fulfilling. Please add an independent oracle, generated vectors from the detector, or a known-bad mutation that this suite rejects.
There was a problem hiding this comment.
[Addressed] The replay now accepts an injected detector and includes two known-bad mutations. My independent run showed both mutations fail the corpus while the baseline passes. This supplies a bounded sensitivity check, although it remains distinct from a full independent oracle.
| "expected": expected, | ||
| "candidate_dispatch_count": candidate_dispatch_count, | ||
| "completed_event_count": completed_event_count, | ||
| "first_intervention_request_index": case["first_intervention_request_index"], |
There was a problem hiding this comment.
[P1] The historical metadata is not part of the replay. The validator only checks ranges, and replay_case returns historical_request_count, historical_exact_suffix_count, and first_intervention_request_index without deriving them from events. The incident has four replayed completions but reports 45 requests and intervention index 19; the test repeats those constants. An in-range metadata mutation still passes all 12 cases. Please replay the complete symbolic request sequence or validate an explicit mapping from the summarized prefix to the intervention index, with an inconsistency test.
There was a problem hiding this comment.
[Addressed] The fixture no longer stores a manually supplied intervention index. The validator derives the replay-prefix length from the aggregate counts, and the tests reject inconsistent request and suffix counts. My independent count checks rejected both mutations.
| self.assertEqual("0.27.0-beta.3", decoded["baseline"]["release"]) | ||
|
|
||
| def test_locked_source_fingerprint_matches_detector_source(self) -> None: | ||
| verify_baseline_source(FIXTURE.parents[3]) |
There was a problem hiding this comment.
[P1] This test hashes the current detector source against the beta.3 fingerprint. Any legitimate detector edit on a later stacked branch makes this test fail, even when the locked replay remains valid. I reproduced this with a one-line source change. The PR states that detector changes will land on later stacked branches, so this test blocks that workflow. Please keep the beta.3 source as a separate baseline input, or scope this check to the exact baseline revision and add a distinct compatibility check for later branches.
There was a problem hiding this comment.
[Addressed for the default stacked workflow] The normal replay and test suite no longer hash the current detector source. I simulated a detector edit, and the default tests and replay passed. The optional source audit still fails on that edit, as intended for a strict beta.3 provenance audit; do not use that optional audit as a later-branch compatibility gate.
| "properties": { | ||
| "tool": { | ||
| "type": "string", | ||
| "pattern": "^tool_[a-z0-9_]{1,26}$" |
There was a problem hiding this comment.
[P2] The checked-in JSON schema is not loaded by the replay or by CI. It also permits arbitrary symbolic names that the Python validator can reject through its private-marker denylist. The schema and validator can therefore accept different corpora, while schema drift can pass CI unnoticed. Please validate the corpus against this schema in CI and align the schema with the enforced privacy contract.
There was a problem hiding this comment.
[Addressed] The checked-in JSON schema was removed. The fixture README now names the Python validator as the contract, so the previous schema-validator drift cannot occur.
Aaronontheweb
left a comment
There was a problem hiding this comment.
Verdict: needs changes before this corpus can support its proof claims
The change adds no daemon or detector runtime code. The offline replay passed 12 of 12 tests at a59f5d5d. The source fingerprint check also passed.
I found five issues that affect the stated evidence boundary:
- The privacy guard accepts arbitrary tokens outside a small denylist. It does not prove that names or host data stay out.
- The replay compares its output with expected decisions stored in the same fixture. This makes the main result self-fulfilling.
- The historical request counts and intervention index are metadata only. The replay does not derive or validate them.
- The source fingerprint test fails after any detector edit on a later stacked branch. This blocks the stated follow-on workflow.
- The JSON schema is not executed in CI and does not match the Python validator's privacy rules.
The inline comments contain reproducible checks and acceptance conditions:
- privacy boundary
- independent oracle
- historical metadata
- stacked branch use
- schema and validator contract
The current CI command does execute the new test file. It does not execute the JSON schema or the optional source audit as separate checks.
Aaronontheweb
left a comment
There was a problem hiding this comment.
Follow-up review: original findings addressed at 67c798121f8114515317b3abf45e59af8826afa3
I reviewed every prior inline thread and the complete change again. I found no remaining issue within the reviewed scope.
Disposition:
- Privacy boundary: addressed. The validator now accepts only closed four-digit symbolic namespaces.
- Replay sensitivity: addressed for the bounded claim. The injected detector and two known-bad mutations fail as expected.
- Historical counts: addressed. The validator derives the replay-prefix length and rejects inconsistent aggregate counts.
- Stacked branches: addressed for normal replay and CI. A simulated detector edit leaves both passing. The optional source audit rejects the edit by design because it checks beta.3 provenance.
- Schema drift: addressed. The separate schema was removed, and the Python validator is now the stated contract.
Independent evidence at this revision:
- The replay test suite passed 16 of 16 tests.
- The replay passed 12 of 12 cases.
- Both known-bad mutations failed the corpus, as required.
- Privacy and aggregate-count mutations were rejected.
- No detector or other runtime source file changed in this PR.
Follow-up replies preserve the evidence for each original thread:
This is a comment-only review. It does not replace CI or human acceptance.
Summary
Local evidence
Privacy boundary
Limits
Verification update
External synthetic evaluation
PR1 changes no runtime behavior. This result motivates later work. It does not prove safety.