fix: close P1 v8 source and spend stop paths - #300
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (9)
🚧 Files skipped from review as they are similar to previous changes (9)
📝 WalkthroughWalkthroughThe executable v2 benchmark now binds studies to an approved source commit, validates retained Git references, supports offline rehearsal, advances the manifest schema to v6, classifies budget failures as terminal, and persists canonical invalid-decision evidence after lifecycle refusals. ChangesBenchmark v2 hardening
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Operator
participant context_guard_bench
participant GitHub
participant StudyManifest
Operator->>context_guard_bench: prepare with approved commit and retained ref
context_guard_bench->>GitHub: resolve retained ref with git ls-remote
GitHub-->>context_guard_bench: commit for retained ref
context_guard_bench->>StudyManifest: validate candidate and source binding
StudyManifest-->>context_guard_bench: validated v6 executable manifest
context_guard_bench-->>Operator: prepared study or refusal
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.
🧹 Nitpick comments (4)
context-guard-kit/benchmark_runner.py (2)
14066-14083: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNarrow the swallowed exception surface or record the suppression.
_benchmark_study_v2_persist_invalid_after_refusaldiscardsOSError,SystemExit,TypeError, andValueErrorwithout any trace. The original refusal message stays authoritative, which is correct. However, the operator receives no signal that canonical P1-X persistence was attempted and failed. In this workflow the missing decision file is itself evidence.Print a short, non-sensitive note to
stderrbefore returning.♻️ Proposed refactor
- except (OSError, SystemExit, TypeError, ValueError): + except (OSError, SystemExit, TypeError, ValueError) as exc: # The original refusal remains authoritative when damaged or incomplete # evidence cannot safely support a canonical P1-X decision. + print( + f"v2 invalid-decision persistence skipped: {exc}", file=sys.stderr, + ) return🤖 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 `@context-guard-kit/benchmark_runner.py` around lines 14066 - 14083, Update _benchmark_study_v2_persist_invalid_after_refusal to emit a short, non-sensitive diagnostic to stderr inside the existing exception handler before returning, while preserving the original refusal authority and current exception handling behavior.
12162-12164: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider resolving
gitinstead of hardcoding/usr/bin/git.The verifier fails closed when
/usr/bin/gitis absent. Several environments install Git at/opt/homebrew/bin/git,/usr/local/bin/git, or a Nix store path. Livepreparethen refuses with "v2 retained ref verifier is unavailable" even though Git is present and usable.A bounded lookup over a fixed allowlist of absolute paths keeps the no-
PATH-trust property and removes the environment dependency.♻️ Proposed refactor
- git = Path("/usr/bin/git") - if not git.is_file() or not os.access(git, os.X_OK): - raise ValueError("v2 retained ref verifier is unavailable") + git = next( + ( + candidate + for candidate in ( + Path("/usr/bin/git"), + Path("/usr/local/bin/git"), + Path("/opt/homebrew/bin/git"), + ) + if candidate.is_file() and os.access(candidate, os.X_OK) + ), + None, + ) + if git is None: + raise ValueError("v2 retained ref verifier is unavailable")🤖 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 `@context-guard-kit/benchmark_runner.py` around lines 12162 - 12164, Update the Git validation in the v2 retained ref verifier to resolve an executable from a bounded allowlist of approved absolute paths, including common Homebrew, system, and Nix locations, instead of checking only /usr/bin/git. Reuse the resolved executable for subsequent verifier operations while preserving the existing fail-closed “v2 retained ref verifier is unavailable” behavior when no allowed path is usable.research/p1-live-authorization-packet.md (1)
227-232: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the
--study-v2-offline-rehearsalboundary in both contracts. Both documents now require an approved source commit and a retained ref verified by a bounded credential-freegit ls-remote. Neither document mentions the new--study-v2-offline-rehearsalflag, which skips that verification and records anoffline-rehearsal-unverified-v1binding. An operator reading either document cannot tell that a rehearsal-prepared root is not eligible for live work.
research/p1-live-authorization-packet.md#L227-L232: state in step 2 that livepreparemust never pass--study-v2-offline-rehearsal, and that a root prepared with that flag is rehearsal-only.research/token-savings-roadmap.md#L387-L392: add the same restriction to ordered item 2, next to the--study-v2-source-commitand--study-v2-retained-refrequirements.🤖 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 `@research/p1-live-authorization-packet.md` around lines 227 - 232, Update step 2 in research/p1-live-authorization-packet.md (lines 227-232) to state that live prepare must never use --study-v2-offline-rehearsal and that roots prepared with it are rehearsal-only; add the same restriction to ordered item 2 in research/token-savings-roadmap.md (lines 387-392), alongside the source-commit and retained-ref requirements.tests/test_benchmark_study_v2.py (1)
477-518: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAssert the Git argv and environment in the retained-ref test.
The test mocks
run_bounded_commandand checks only the return value and the mismatch refusal. It does not check the command that the verifier builds. The credential-free properties are the security-relevant part of_benchmark_study_v2_verify_retained_ref: the fixed repository URL,--exit-code,--refs, the emptycredential.helperandcore.askPasssettings, andGIT_TERMINAL_PROMPT=0. A regression that drops one of them keeps this test green.Capture the mock call and assert those values.
💚 Proposed test addition
with mock.patch.object( self.runner, "run_bounded_command", return_value=completed, - ): + ) as bounded: self.assertEqual( helper(retained_ref, expected_commit), { "commit_sha": expected_commit, "ref": retained_ref, "repository": "ictechgy/context-guard", "verification": "git-ls-remote-v1", }, ) + argv, keyword = bounded.call_args + command = argv[0] + self.assertEqual(command[-3:], [ + "--refs", + "https://github.com/ictechgy/context-guard.git", + retained_ref, + ]) + self.assertIn("credential.helper=", command) + self.assertIn("core.askPass=", command) + self.assertIn("--exit-code", command) + self.assertEqual(keyword["env"]["GIT_TERMINAL_PROMPT"], "0") + self.assertEqual(keyword["env"]["GIT_CONFIG_NOSYSTEM"], "1")🤖 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 477 - 518, Update test_v2_retained_ref_requires_exact_remote_resolution to capture the run_bounded_command mock call and assert the verifier invokes the fixed repository URL with --exit-code and --refs, includes empty credential.helper and core.askPass settings, and sets GIT_TERMINAL_PROMPT=0 in the environment. Apply these assertions to the retained-ref verification call while preserving the existing success and mismatch checks.
🤖 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 `@context-guard-kit/benchmark_runner.py`:
- Around line 14066-14083: Update
_benchmark_study_v2_persist_invalid_after_refusal to emit a short, non-sensitive
diagnostic to stderr inside the existing exception handler before returning,
while preserving the original refusal authority and current exception handling
behavior.
- Around line 12162-12164: Update the Git validation in the v2 retained ref
verifier to resolve an executable from a bounded allowlist of approved absolute
paths, including common Homebrew, system, and Nix locations, instead of checking
only /usr/bin/git. Reuse the resolved executable for subsequent verifier
operations while preserving the existing fail-closed “v2 retained ref verifier
is unavailable” behavior when no allowed path is usable.
In `@research/p1-live-authorization-packet.md`:
- Around line 227-232: Update step 2 in research/p1-live-authorization-packet.md
(lines 227-232) to state that live prepare must never use
--study-v2-offline-rehearsal and that roots prepared with it are rehearsal-only;
add the same restriction to ordered item 2 in research/token-savings-roadmap.md
(lines 387-392), alongside the source-commit and retained-ref requirements.
In `@tests/test_benchmark_study_v2.py`:
- Around line 477-518: Update
test_v2_retained_ref_requires_exact_remote_resolution to capture the
run_bounded_command mock call and assert the verifier invokes the fixed
repository URL with --exit-code and --refs, includes empty credential.helper and
core.askPass settings, and sets GIT_TERMINAL_PROMPT=0 in the environment. Apply
these assertions to the retained-ref verification call while preserving the
existing success and mismatch checks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 1e2b25ed-4618-4c82-a1c1-57f738633de5
📒 Files selected for processing (10)
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.mdscripts/rehearse_measurement_study.pytests/test_benchmark_study_v2.pytests/test_contextguard_stage2_feasibility.pytests/test_contextguard_stage2_protected_surfaces.py
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.
Summary
error_max_budget_usdas a hard spend stop and persist verifiable P1-X decisions automaticallyVerification
python3 -m unittest -v tests.test_benchmark_study_v2(51 passed)Summary by CodeRabbit
New Features
Bug Fixes
Documentation