test(control): make owner helper integration context-aware - #86
Conversation
The real Windows owner-helper positive test assumed mkdtempSync(tmpdir()) yields an operator-owned directory (owner SID == whoami SID). That holds in a non-elevated context but not on GitHub's elevated Windows runner, where a freshly created directory is owned by Administrators (or SYSTEM) — a non-operator SID — so production correctly fails closed and the ok:true assertion breaks. The test now reads the temp directory's ACTUAL owner SID via the same real, build-provenanced helper binary, then asserts the corresponding deterministic gate behaviour: owner == operator must accept and echo that SID; a non-operator owner must fail closed with the exact reason (OWNER_IS_SYSTEM for SYSTEM, OWNER_MISMATCH otherwise). Meaningful in both elevated and non-elevated contexts, and never treats a foreign owner as success. Test-only. Production (ownerSid === operatorSid gate) unchanged; F1/F2/F3 and the six protected files byte-identical. Full suite 2010/2010; typecheck/lint/build/diff-check clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PPAMmQpdQbunE8kGbezgih
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe Windows owner verification test now reads the temporary directory’s actual owner SID with the build-provenanced helper. It accepts operator ownership and checks fail-closed error codes for system or other foreign ownership. ChangesOwner verification
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to The Windows owner-verification test now handles elevated and non-elevated runner ownership correctly while retaining rejection checks for SYSTEM and foreign owners. No current merge-blocking risk remains. Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 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 |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dc108807da
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@coderabbitai review |
✅ Action performedReview finished.
|
9548d12
into
repair/pr84-f1-control-anchor-owner
Nested validation repair for PR #85.
Finding:
Repair:
Exact identity:
b51cacafcd5f461fe0c79564b53883da44e51aaac68e7fa7bbaed2fe8204808b
Validation:
No F1 production change.
No F2/F3 repair.
No runtime/deployment mutation.
🤖 Generated with Claude Code
https://claude.ai/code/session_01PPAMmQpdQbunE8kGbezgih
Summary by CodeRabbit