fix: seal failed P1 canary evidence - #289
Merged
Merged
Conversation
📝 WalkthroughWalkthroughThe v2 benchmark detects failed terminal canaries, emits bounded invalid-decision evidence, identifies affected canaries, validates ledger integrity, and updates protected-surface hashes and readiness records. ChangesFailed canary evidence
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant CanaryProcess
participant Analyze
participant InvalidDecision
participant CanaryLedger
CanaryProcess->>Analyze: terminal canary state
Analyze->>InvalidDecision: evaluate canary evidence
InvalidDecision->>CanaryLedger: preserve ledger state
InvalidDecision-->>Analyze: P1-X stop reason and failed identities
Analyze-->>CanaryProcess: no replay or further provider calls
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tests/test_benchmark_study_v2.py (2)
611-613: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winTest permanent closure after a failed canary.
The test proves that
analyzeadds no provider calls. It does not prove that a laterresumeorruncannot launch another identity. Invokeresumeimmediately after the failed canary and assert refusal plus unchanged ledger and provider-log bytes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_benchmark_study_v2.py` around lines 611 - 613, Extend the failed-canary test around the existing analyze assertions to invoke resume immediately afterward and assert that it refuses to proceed. Verify that both the ledger bytes and provider_log bytes remain unchanged after the refused resume, preserving the existing no-provider-call checks.
567-610: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAssert the complete P1-X payload contract.
The test name promises a P1-X decision, but the assertions check only
stop_reason. Assertdecision == "P1-X",descriptive_only is True,claim_allowed is False,claim is None, the schema version, and the attempts-ledger hash.Proposed assertions
canary_path = output_root / "canary-events.jsonl" canary_raw = canary_path.read_bytes() + attempts_path = output_root / "attempts.jsonl" + attempts_raw = attempts_path.read_bytes() provider_log = output_root / "fake-cli-calls.jsonl" provider_calls_before = provider_log.read_bytes() completed = subprocess.run( ... self.assertEqual( decision_raw, self.runner._study_canonical_json_bytes(decision), ) + self.assertEqual( + decision["schema_version"], + "contextguard.bench.study-invalid-decision.v1", + ) + self.assertEqual(decision["decision"], "P1-X") + self.assertTrue(decision["descriptive_only"]) + self.assertFalse(decision["claim_allowed"]) + self.assertIsNone(decision["claim"]) ... + self.assertEqual( + decision["ledgers"]["attempts"]["sha256"], + hashlib.sha256(attempts_raw).hexdigest(), + )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_benchmark_study_v2.py` around lines 567 - 610, Extend the decision assertions in the analyze test after loading decision to validate the full P1-X payload: assert the decision value, descriptive_only, claim_allowed, and claim fields, along with the expected schema version and the attempts ledger SHA-256. Reuse the existing canonical decision and ledger data, keeping the current stop_reason and canary assertions unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@research/p1-live-authorization-packet.md`:
- Around line 27-30: Update the later P1-X execution rule to recognize both
stopped ambiguous roots and failed terminal canary evidence as valid P1-X
outcomes. Preserve the existing rule while adding the newly introduced
failed_canary_terminal_evidence case.
---
Nitpick comments:
In `@tests/test_benchmark_study_v2.py`:
- Around line 611-613: Extend the failed-canary test around the existing analyze
assertions to invoke resume immediately afterward and assert that it refuses to
proceed. Verify that both the ledger bytes and provider_log bytes remain
unchanged after the refused resume, preserving the existing no-provider-call
checks.
- Around line 567-610: Extend the decision assertions in the analyze test after
loading decision to validate the full P1-X payload: assert the decision value,
descriptive_only, claim_allowed, and claim fields, along with the expected
schema version and the attempts ledger SHA-256. Reuse the existing canonical
decision and ledger data, keeping the current stop_reason and canary assertions
unchanged.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 53fbb253-af01-49aa-b1a9-7be8c90efda3
📒 Files selected for processing (9)
context-guard-kit/benchmark_runner.pypackages/context-guard-receipt/scripts/verify_protected_surfaces.pypackages/context-guard-receipt/tests/contract/test_boundary.pyplugins/context-guard/bin/context-guard-benchresearch/p1-live-authorization-packet.mdresearch/token-savings-roadmap.mdtests/test_benchmark_study_v2.pytests/test_contextguard_stage2_feasibility.pytests/test_contextguard_stage2_protected_surfaces.py
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/test_benchmark_study_v2.py (1)
569-585: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAssert the initial failed-canary evidence.
The test only compares the canary ledger and provider log before and after
resume. It does not verify that the initial event is the reportedlegacy_trimterminal failure, that provider duration, token counts, and cost are zero, or that no second canary ran. Parse the terminal canary records and provider log after the firstcanaryaction. Assert those conditions explicitly.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_benchmark_study_v2.py` around lines 569 - 585, The test around the initial canary and subsequent resume must validate the failed-canary evidence before checking resume immutability. Parse the terminal canary records and provider log immediately after the first canary action, assert the reported terminal failure is legacy_trim with zero provider duration, token counts, and cost, and verify exactly one canary ran; retain the existing assertions that resume does not alter the canary ledger, attempts file, or provider log.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/test_benchmark_study_v2.py`:
- Around line 569-585: The test around the initial canary and subsequent resume
must validate the failed-canary evidence before checking resume immutability.
Parse the terminal canary records and provider log immediately after the first
canary action, assert the reported terminal failure is legacy_trim with zero
provider duration, token counts, and cost, and verify exactly one canary ran;
retain the existing assertions that resume does not alter the canary ledger,
attempts file, or provider log.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: c360ddb9-b801-4cdd-8bbf-3206d9a6bb9f
📒 Files selected for processing (2)
research/p1-live-authorization-packet.mdtests/test_benchmark_study_v2.py
🚧 Files skipped from review as they are similar to previous changes (1)
- research/p1-live-authorization-packet.md
This was referenced Aug 10, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Outcome
A terminal failed P1 canary now emits the promised ledger-bound P1-X decision instead of refusing analysis with exit 2. The failed root remains permanently closed and no provider identity is replayed.
Live checkpoint
Verification
npm next/latest are not authorized or invoked.
Summary by CodeRabbit
Bug Fixes
Documentation
Tests