Repository navigation
test(policy): deny invalid handle use without fixing denial ordering - #338
Open
seonghobae wants to merge 5 commits into
Open
seonghobae wants to merge 5 commits into
seonghobae wants to merge 5 commits into
Conversation
Add three regressions for existing scope, expiry, and use-limit decisions without changing production behavior. Co-Authored-By: Claude Code <noreply@anthropic.com>
Keep foreign-scope denial and zero-use admission regressions without promoting implementation ordering into a public contract. Co-Authored-By: Claude Code <noreply@anthropic.com>
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (8)
🚧 Files skipped from review as they are similar to previous changes (2)
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughCI 작업에 격리 self-hosted runner 라우팅을 적용하고, runner 선택 및 checkout 자격 증명 계약을 검사하는 테스트를 추가했습니다. 불투명 핸들 거부 동작을 확인하는 회귀 테스트와 관련 문서도 갱신했습니다. Changes격리 CI 러너 라우팅
불투명 핸들 정책 회귀 검증
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Other Merge Risk: ⚪ Minimal · up to No concrete merge-blocking issue is established. Confirm that the selected runners are available and that the exact-head CI jobs pass before merging. Security Architecture ReviewSecurity architecture risk: 🟡 Moderate · up to The workflows preserve read-only repository permissions and credential-free checkout. However, runner selection alone does not establish host isolation, restricted privileges, or cleanup after interrupted jobs. Those safeguards remain unverified for the new self-hosted execution boundary. Retained concerns
Security review detailsSecurity Blast Radius
Security Findings and Attack Paths
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![cwl-noema-review[bot]](https://avatars.githubusercontent.com/u/295022177?s=60&v=4){kind=small}
There was a problem hiding this comment.
Noema LLM review
The PR adds regression tests for opaque-handle policy decisions without altering production behavior. The tests correctly verify that foreign-scope requests are denied regardless of expiry or exhaustion status and that a zero-use allowance results in an immediate UseLimitReached decision. Documentation in CHANGELOG.md and TEST_STRATEGY.md accurately reflects these additions.
Reviewed changed lines
crates/originweave-policy/tests/handle_use_precedence.rs:36 (RIGHT): The testforeign_scope_is_denied_when_expired_and_exhaustedcorrectly usesassert_ne!(..., HandleUseDecision::Authorized)at lines 36-39. This confirms the request is denied when the authority is mismatched, regardless of whether expiration (3000 vs 2000) or exhaustion (99 vs 2) is evaluated first, consistent with the PR's goal of not prescribing denial ordering.crates/originweave-policy/tests/handle_use_precedence.rs:45 (RIGHT): The testzero_max_uses_exhausts_the_handle_before_any_usecorrectly setsmax_usesto 0 inSensitiveValueHandleScope(line 45) and asserts thatevaluate_handle_usereturnsHandleUseDecision::UseLimitReached(lines 47-49), verifying that zero allowance results in immediate exhaustion.CHANGELOG.md:10 (RIGHT): The changelog entry accurately describes the addition of opaque-handle regression tests and explicitly notes that production behavior remains unchanged.docs/TEST_STRATEGY.md:215 (RIGHT): The additions toTEST_STRATEGY.md(lines 215-216) correctly document the intent and scope of the new tests, specifically noting the lack of prescription for denial reason precedence.
Adversarial validation
crates/originweave-policy/tests/handle_use_precedence.rs:36 (RIGHT)falsified: The test might accidentally prescribe a specific denial reason, locking the implementation into a specific ordering (e.g., Authority check before Expiry check). — Confirmed via lines 36-39.crates/originweave-policy/tests/handle_use_precedence.rs:45 (RIGHT)falsified: A zero-allowance handle might still allow the first use due to an off-by-one error in the limit check. — Confirmed via lines 45-49.- Residual risk: none
Findings
- No blocking findings.
- Result: APPROVE
- Head SHA:
4f8e2da905d9a0e66b8fab49b38e35ee36072ead - Reviewer credential:
noema-review-github-app-refresh - Actor:
cwl-noema-review[bot]
Contributor
Author
|
@coderabbitai rate limit |
|
Your plan includes PR reviews subject to rate limits. More reviews will be available in 29 minutes. |
Contributor
Author
|
@coderabbitai full review |
✅ Action performedFull review finished. |
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
Exact-head verification
Head:
4f8e2da905d9a0e66b8fab49b38e35ee36072ead; base:87c4daa1830bac5a5228b6036752ad5633232085.Fresh isolated external target, serial jobs=2: focused tests 2/2; workspace all-targets tests 277; format; strict Clippy; rustdoc with warnings denied; Python contracts 152. Pinned nightly-2026-08-18/cargo-llvm-cov 0.8.6 measured production functions, lines, regions, and branches at 100%; repository coverage enforcement passed.
This is regression expansion, not a demonstrated production defect repair. Earlier three-test local head was narrowed and every final-head gate rerun; no obsolete denial-ordering evidence is used. Remote CI/security and an eligible counted review remain required before merge; local evidence does not substitute for them.
🤖 Generated with Claude Code
Summary by CodeRabbit
Handle-denial documentation repair — 1681d69
Five meaningful Rust doc comments describe the canonical destination, matching/foreign tenant metadata, order-independent denial despite expiry/exhaustion, and exact zero-allowance UseLimitReached result. Removing only these five added lines restores the parent test bytes exactly; no executable test or production policy changed. CHANGELOG.md and docs/TEST_STRATEGY.md preserve these limits.
Independent complete original-base-to-final source review: e8e65124e8970cc14f4f9d04f35e944f11ae7a473e1f021bea4ff0cb0251f064 (PASS_SOURCE_ONLY, not formal GitHub approval). Fresh actual committed-head verification: all nine gates passed, Rust 277/Python 152, zero failures/ignored, production functions/lines/regions/branches each100%. Normal non-force push verified branch and PR head.
Current-head hosted Rust/coverage run37179575043 and managed CodeQL run37179573283 failed before execution with explicit account billing-lock annotations. CodeRabbit current status is review-rate-limited; its completed review6226930f and 40%/80% documentation warning belong to parent4f8e, not this successor. External remeasurement and current-head substantive/formal review remain unverified. The prior Noema APPROVED review also binds4f8e and is not promoted to this HEAD. No protection, credential, billing, deployment or review gate changed. Merge is not complete.
Isolated repository CI source conversion — f551fd1
Composed only the reviewed CI, coverage and MV3 routing increment from PR347e726 into this existing branch. All three jobs require the CWL CI isolated group AND [self-hosted, Linux, X64, cwlab-ci-isolated]. Reversing only the three selector replacements restores the entire prior workflow bytes, including triggers, gates, permissions and pinned actions. Handle regression/descriptions and production bytes are preserved; the scheduled workflow remains unchanged and OW-SH-001 remains unfixed. No privileged fallback was added.
Complete original-base-to-final eight-path independent SOURCE review: 9f1ac33c32dc532b379e6fa1bd7c250d528c4ea264646413cad8edc8eb4c1e7c. Exact seven-path adoption and normal commit were followed by fresh committed-head TEN gates: Rust277/Python164, zero failed/ignored, production functions/lines/regions/branches100%, strict rustdoc and normal-discovery actionlint. One normal non-force push verified the remote branch and current PR head.
Current Actions runtime is NOT accepted: CI37181335473 jobs111374365342/111374365347 and MV337181335467 job111374365141 explicitly report Required runner group CWL CI isolated not found. Managed dynamic CodeQL37181334090/job111374363111 remains independently billing-blocked before execution. New jobs REST reads were rate-limited, so no new runner/step inventory is claimed. Physical disposable isolation, eligible group access, effective self-hosted egress/hardening, managed CodeQL placement and current-head substantive/formal reviews remain separate obligations. The old Noema approval binds4f8e, not this HEAD. No group, ACL, credential, payment, protection, deployment or CI rerun was changed. Protected merge/release are not complete.
Checkout whitespace regression — verified publication on 2026-10-05 KST
Current published head:
b64dd9c45e1ac7f7df1091ab4c9bebf382a80c6c; parentf551fd1de8b5dff8d061d6659c7072fa215f0b18; reviewed treec8bd82e5a9d5329730a8e60bac368289ec5d865b. Complete original-base source review SHA-256ccc63c503f86770472279795bf86bcb3abf13c828552a8c3a1cef785e8ec4f95covers the entire current PR delta, not only the three-path repair. The checker accepts horizontal whitespace afteruses:and retains quoted/unquoted checkout, literal-false, sibling-step and error-vs-assertion guards. Focused 14-test RED/GREEN evidence is retained. Only CHANGELOG, TEST_STRATEGY and the checker changed in this successor.Fresh exact committed-head ten gates passed: Rust 277, Python 166, zero failed/ignored, production functions/lines/regions/branches each100%. Actual command/log receipts remain separate from source review and hosted acceptance.
One ordinary non-force push through the existing personal S1 connection was performed by the existing sole owner using the freshly verified same GitHub identity seonghobae. Only a hash-verified public-source incremental bundle was transferred; no credentials or persistent authentication settings changed. Initial PR-ref propagation lag was retained, then authenticated current PR-head equality was verified without another push. The Mac SecurityServer IPC issue is not claimed fixed.
Exact new-head Actions runs: 37215618775, 37215620861, 37215620866. All four jobs have runner_id0 and no executed steps. Three repository CI/coverage/MV3 jobs explicitly report required isolated runner group not found; the service-managed Code Quality Python analysis independently reports account billing lock/job not started. These are admission failures, not executed test or scanner findings. Scheduled workflow OW-SH-001 remains unfixed/deferred. No runner/group/ACL or managed scan setting was changed, no rerun or readiness toggle was issued. Current-head formal approval and governing protection remain independent; old-head review decisions do not authorize this successor. Ordinary merge and release are not complete.