fix(mcp): preserve external policy authority - #10119
Conversation
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
wscurran
added
area: policy
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
|
The MCP discovery failures came from remove using an empty network-policy map as an authority probe. The policy parser correctly rejects that map before inspection. Verified commit 7d8081e uses a valid capability-free per-server entry for managed removal. Recorded external authority requires the exact rendered MCP policy immediately; legacy external authority retries only after preflight persists external authority, and still verifies the exact policy before mutation. Verification:
CI is rerunning with the corrected authority qualification. |
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
…to codex/9833-mcp-authority
…rity Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
…rity Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
cv
left a comment
cv
left a comment
There was a problem hiding this comment.
The commit under review has an unresolved MCP add recovery defect.
src/lib/actions/sandbox/mcp-bridge-add-restart.ts:569-572 removes addState, writes the committed bridge entry, and then revalidates policy authority. If that final check refuses, the catch path removes the adapter, policy, attachment, and possibly provider. However, it does not restore addState in the durable bridge entry. A retry then reaches src/lib/actions/sandbox/mcp-bridge-add-restart.ts:205-209 and rejects the entry as an existing completed server, although rollback removed its runtime resources.
Move the final authority check before the durable commit, or preserve a retryable transaction state when a post-commit refusal starts rollback. Add a focused test that changes authority after writeBridgeEntry commits the entry and proves that retry can converge.
I also confirmed that issue #9833 establishes the product scope. The latest PR commit and every PR commit are GitHub Verified. The DCO workflow passed; the PR description has a Signed-off-by: declaration. CodeRabbit skipped this stacked PR, and no review threads exist. I inspected the PR Advisor results and current check evidence. I did not use failed or skipped checks as the reason for this review.
senthilr-nv
left a comment
senthilr-nv
left a comment
There was a problem hiding this comment.
Product-scope verdict: BLOCKED
The accepted #9833 decision now requires a durable creation receipt that matches the gateway, immutable sandbox identity, and verified effective policy identity before NemoClaw may treat policy as managed. The commit under review is based on 1fe92d4, which does not contain merged #10049 (3cb2ff4), and this stack still persists authority inferred from live policy-source inspection. Please restack and reconcile the MCP lifecycle work with that receipt contract before review. Otherwise unknown ownership can be recorded as NemoClaw-managed and admit policy mutation.
Review verdict: CHANGES_REQUESTED
- In src/lib/actions/sandbox/mcp-bridge-add-restart.ts:569-572, the final authority check runs after writeBridgeEntry removes addState and publishes a completed bridge. If that check refuses, the catch path removes runtime resources but preserves the completed manifest. A retry then exits at lines 205-209 because the server already exists, so the promised retry intent is lost. Move the final check before the completed write or retain an explicit retryable state, and add a focused refusal test at this post-write boundary.
- The commit under review also has an unresolved source-test failure: rebuild-flow-lifecycle.test.ts expects the managed authority registry update once but observes zero calls. This needs a stable passing rerun after the restack or a focused correction.
Security review: Secrets PASS; Input validation PASS; Authentication/authorization FAIL (unproved policy ownership); Dependencies PASS; Logging/error handling PASS; Cryptography/data protection PASS; Configuration/headers FAIL (authority attribution contradicts the accepted durable-receipt contract); Security testing FAIL (missing completed-manifest refusal coverage and current lifecycle failure); System security PASS.
Cross-issue sweep: #9833 is SAME_ISSUE_DIFF; no separate related open issue was found. Documentation review: no public docs changed, and the explanatory comments are clear, but documentation quality cannot override the product and correctness blockers. DCO and GitHub commit verification pass for every commit inspected. Approval remains independent of CI; auto-merge must remain off.
<!-- markdownlint-disable MD041 --> ## Outcome Hermes GPU creation retains NemoClaw policy authority when OpenShell applies the reviewed GPU baseline enrichment or changes the policy during the same verified create transaction. Completed sandboxes, external policy mutations, and arbitrary filesystem additions remain unable to refresh a receipt. ## Reason The fallback and compatibility-only Hermes GPU jobs reached the exact sandbox, then rejected policy ownership. The compatibility route did not recognize the reviewed GPU enrichment. A later product-owned container transition could also advance the policy identity after the first receipt was recorded. The broader seven-job report contains three distinct root causes. This PR claims only the product-owned GPU receipt-transition cause. PR #10398 owns a different fallback cleanup failure. PRs #10116 and #10119 own external-policy preservation. ### Related issues Part of #9833. Relates to #10155. ## Changes - Recognize the reviewed native and compatibility GPU enrichment shapes while preserving route-specific `/proc` requirements. - Refresh a managed receipt only through the in-progress verified create transaction after exact identity and stable policy verification pass again. - Update the durable pending checkpoint with compare-and-set against the prior checkpoint before later effects continue. - Reject receipt refresh for completed sandbox mutations, routes without GPU injection, and arbitrary added filesystem paths. - Preserve the Hermes portable native-policy verifier on the renamed shared enrichment predicate. ## Verification - `npx vitest run --project cli src/lib/onboard/sandbox-create/policy-creation-receipt.test.ts` — 29 tests passed. - Focused CLI suites for receipt verification, orchestration, GPU identity gates, GPU create, policy mutation authority, and registry transitions — 9 files and 288 tests passed. - `tsc -p tsconfig.cli.json` — passed. - `tsx scripts/checks/run.mts` — repository checks passed. - File-local formatting, Oxlint, whitespace, and ignored-file hooks — passed. - `git diff --check` — passed. - The diff contains no secrets, API keys, or credentials. ## Review notes E2E root cause: `sandbox create / GPU policy receipt verification / reviewed compatibility enrichment and verified create transition rejected` Source run: https://github.com/NVIDIA/NemoClaw/actions/runs/33111985611 (run 33111985611, attempt 1) Failed jobs: - Hermes GPU fallback: https://github.com/NVIDIA/NemoClaw/actions/runs/33111985611/job/98657665722 - Hermes GPU compatibility-only: https://github.com/NVIDIA/NemoClaw/actions/runs/33111985611/job/98657665787 Signatures: - The live base policy does not match the policy supplied by the create transaction. - The creation receipt no longer matches the live sandbox policy before the runtime patch. Scope: one root cause. Security boundary: refresh requires the active verified create boundary, exact sandbox lifecycle identity, stable sandbox-scoped policy, an allowlisted GPU enrichment, and checkpoint compare-and-set. External mutations and completed-sandbox policy drift remain denied. Independent sensitive-path review remains required. --- Signed-off-by: Aaron Erickson <aerickson@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Bug Fixes** - Improved GPU policy validation across native and compatibility routes, including supported proxy-only access. - Strengthened post-creation checks for sandbox identity, reservations, checkpoints, and policy consistency. - Prevented unexpected policy registration changes during active sandbox creation. - Ensured mismatched managed policy registrations are rejected with clearer error handling. - **Tests** - Expanded coverage for GPU enrichment, proxy-only access, and policy registration revalidation. - Added coverage confirming mismatched managed registrations are rejected rather than refreshed. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
|
Closing this PR because the accepted policy decision in #10514 supersedes #9833. This PR preserves policy-authority receipts across MCP lifecycle operations, but #10514 prohibits durable policy receipts and authority records. Replacement work is in #10515, which delivers the accepted cutover and states that no intermediate authority-stack PR is intended to merge. This closure records supersession. It does not assess the contributor or the implementation quality. |
4 participants
Summary
MCP bridge add, remove, restart, rebuild, and destroy operations now retain exact policy-authority receipts. Authority drift stops further policy-sensitive mutation and suppresses success publication.
Related Issue
Part of #9833
Changes
Type of Change
Quality Gates
Verification
Signed-off-by:line and every commit appears asVerifiedin GitHubpre-commit,commit-msg, andpre-pushhooks passed, ornpm run validate:prpassed after refreshingorigin/mainwhen hooks were skipped or unavailablenpm testfor broad runtime/test-harness changes;npm run checkfor repo-wide validation/coverage changes — command/result:npm run docsbuilds without warnings (doc changes only)Signed-off-by: Apurv Kumaria akumaria@nvidia.com