fix(checks): harden trusted policy scanners - #7159
Conversation
Co-authored-by: Carlos Villela <cvillela@nvidia.com> Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
apurvvkumaria
added
area: ci
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe policy-read checker now validates ChangesPolicy mutation-read discovery
CI ratchet execution
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage remains at 96%, unchanged from the TypeScript / code-coverage/cliThe overall coverage in the Show a code coverage summary of the most impacted files.
Updated |
PR Review Advisor — InformationalAdvisor assessment: Informational / high confidence Model lanes
Nemotron output stays in workflow artifacts and does not change the assessment above. Since last review: 0 prior items resolved · 0 still apply · 0 new items found E2E guidanceAdvisory only. E2E / PR Gate selects and runs jobs independently. Recommended E2E: None This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge. |
![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
🤖 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 `@test/policy-mutation-read-discovery.test.ts`:
- Around line 55-64: Add a focused false-positive test for countPolicyReadCalls
using the canonical /repo/src/lib/policy/commands.ts path, with a nested
function parameter or local named resolveOpenshellBinary shadowing the resolver
symbol; assert the result is 0. Keep the existing decoy test, and ensure the new
case exercises declaration-origin validation rather than being rejected by the
filename guard.
🪄 Autofix (Beta)
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 31419bc9-596d-4a36-ad26-0520eb9eb066
📒 Files selected for processing (4)
.github/actions/ci-static-checks/action.yamlscripts/checks/openshell-policy-mutation-read.mtstest/policy-mutation-read-discovery.test.tstest/pr-workflow-contract.test.ts
💤 Files with no reviewable changes (1)
- .github/actions/ci-static-checks/action.yaml
Co-authored-by: Carlos Villela <cvillela@nvidia.com> Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
|
Exact-head maintainer security audit completed at
No blocker found. Independent review requested from @laitingsheng; I am not self-approving an author-owned PR. |
Co-authored-by: Carlos Villela <cvillela@nvidia.com> Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
|
Follow-up exact-head security audit found one remaining declaration-origin gap in the policy-read scanner: a nested function declaration named resolveOpenshellBinary inside the canonical command file still satisfied the same-file/function predicate and could be counted as the canonical resolver.\n\nFixed in 3793c92 by requiring the resolver declaration to be top-level (SourceFile-owned), with a focused nested-function regression.\n\nValidation on the pushed revision:\n- targeted integration tests: 32/32 passed\n- direct OpenShell policy-mutation audit passed\n- Biome, changed-file prek hooks, test-title check, and git diff --check passed\n- pre-push CLI typecheck and version-sync hooks passed\n- commit is signed, DCO-compliant, GitHub Verified, and retains Carlos co-author credit\n\nCurrent CI/advisor/E2E checks are running. Existing labels were preserved unchanged. |
cv
left a comment
cv
left a comment
There was a problem hiding this comment.
Approved after current-base review: trusted scanner hardening is fail-closed, security review found no blocker, the canonical advisor reports no findings, risky paths have regression coverage, contributor compliance is complete, and all 51 current checks are green.
3 participants
Summary
This follow-up hardens the trusted CI ratchet and OpenShell policy-read scanner after #7047 merged before the final maintainer fix could land. It removes a duplicate root-dependency execution path and prevents locally shadowed
requireor lookalike resolver calls from satisfying the policy-read check.Related Issue
Follow-up to #7047.
Changes
npx tsxinvocation of the base-trusted createRequire ratchet.require, not a locally shadowed decoy.resolveOpenshellBinary()policy reads only from the canonical policy command module and declaration.Type of Change
Quality Gates
Verification
Signed-off-by:line and every commit appears asVerifiedin GitHubpre-commit,commit-msg, andpre-pushhooks passed, ornpm run check:diffpassed when hooks were skipped or unavailablenpx vitest run --project integration test/policy-mutation-read-discovery.test.ts test/pr-workflow-contract.test.ts(30 passed)npm testfor broad runtime/test-harness changes;npm run checkfor repo-wide validation/coverage changes — command/result:npm run checkspassed; diff-scoped prek and the normal pre-push TypeScript gates passednpm run docsbuilds without warnings (doc changes only)Signed-off-by: Apurv Kumaria akumaria@nvidia.com
Summary by CodeRabbit
requireusage and non-canonical OpenShell resolver calls.--baseand--full).npx tsx.