fix(shields): keep registered network routes when shields down swaps policy - #7963
fix(shields): keep registered network routes when shields down swaps policy#7963harjothkhara wants to merge 7 commits into
Conversation
📝 WalkthroughWalkthroughThe runtime permissive policy now merges live network routes with the static baseline. Provider-composed entries are excluded, baseline definitions take precedence for duplicate names, and Shields-down tests verify preservation of live MCP routes. ChangesRuntime policy merge
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant shieldsDown
participant buildRuntimePermissivePolicy
participant LivePolicyFiles
participant PolicyApplication
shieldsDown->>buildRuntimePermissivePolicy: build permissive policy
buildRuntimePermissivePolicy->>LivePolicyFiles: read live filesystem and network policies
LivePolicyFiles-->>buildRuntimePermissivePolicy: return policy entries
buildRuntimePermissivePolicy-->>shieldsDown: return merged policy
shieldsDown->>PolicyApplication: apply merged policy
PolicyApplication-->>shieldsDown: preserve live MCP and baseline routes
Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
PR Review Advisor — Blocking findings reportedAdvisor assessment: Blockers require maintainer review Model lanes
1 additional E2E selection from the second opinionAdvisory only. The primary lane did not select these E2E jobs or targets.
Second-opinion terminology and E2E selections are advisory. They do not change the primary assessment or E2E / PR Gate. 3 semantic terminology decisionsTerminology decisions are advisory. They affect the assessment only when a separate finding identifies concrete semantic impact.
E2E guidanceAdvisory only. E2E / PR Gate selects and runs jobs independently. Recommended E2E: 2 optional E2E recommendations
Blockers
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/permissive-runtime.test.ts (1)
323-323: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd the local issue suffix to the test title.
Update the title to
carries across exactly what the canonical provider filter keeps (#7952). This preserves the behavior-oriented wording and follows the repository test-title guideline.🤖 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 `@test/permissive-runtime.test.ts` at line 323, Update the test title in the carries-across canonical provider filter test to append the local issue suffix “(`#7952`)”, preserving the existing behavior-oriented wording.Source: Coding guidelines
🤖 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 `@test/permissive-runtime.test.ts`:
- Line 323: Update the test title in the carries-across canonical provider
filter test to append the local issue suffix “(`#7952`)”, preserving the existing
behavior-oriented wording.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 6ee55f0c-1114-43c9-b909-e765025567e3
📒 Files selected for processing (1)
test/permissive-runtime.test.ts
30f6a24 to
1d72143
Compare
wscurran
added
area: policy
|
✨ Thanks for the PR. This fixes shields-down dropping registered MCP network routes by unioning live network_policies into the permissive baseline. Maintainers will review the policy union logic and test coverage for route preservation. Related open issues: |
…policy `shields down` replaces the whole live policy with the static permissive baseline. That baseline is a fixed allowlist, so a route registered at runtime — a generated MCP bridge entry — is dropped. The server stays in the registry with its provider attached and its adapter registered, but the gateway no longer serves it, so tool discovery and tool calls fail until `mcp restart` reapplies the generated policy. Union the live `network_policies` into the permissive document, the same way the helper already unions filesystem paths. The static base wins on a shared key, so every route the baseline declares is applied unchanged. Provider-composed `_provider_*` entries are never copied, because `policy set` must not receive them. Refs: NVIDIA#7952 Signed-off-by: harjoth <harjoth.khara@gmail.com>
… one The permissive merge inlines the `_provider_` prefix instead of importing `withoutProviderComposedPolicies`, which is only reachable through a built artifact. Nothing kept the two in sync, so a change to the canonical filter could let provider-composed entries through this write path unnoticed. Drive the merge and the canonical filter with the same routes and assert they keep the same set. The test fails if either side drifts. Refs: NVIDIA#7952 Signed-off-by: harjoth <harjoth.khara@gmail.com>
Base-wins on a shared network_policies key relies on the baseline being the more permissive of the two, and nothing enforces that. It holds because the dynamic writers namespace their keys. Say so, so a future dynamic preset does not reuse a baseline key and get silently ignored. Refs: NVIDIA#7952 Signed-off-by: harjoth <harjoth.khara@gmail.com>
5900b79 to
8c98627
Compare
cv
left a comment
cv
left a comment
There was a problem hiding this comment.
Reviewed commit bfe11fe53. The new down-transition test demonstrates the reported route loss, provider-composed entries remain filtered, and baseline entries retain precedence. No secret, injection, authorization, dependency, cryptography, logging, or privilege defect is apparent in that path.
Two supported-contract questions block approval. First, issue #7952 requires shields-up and automatic restoration to reconcile entries created or removed during the shields-down window, plus live Hermes MCP qualification. This PR explicitly leaves that stale-snapshot behavior and the required E2E unresolved. Either complete those criteria here or link an accepted, sequenced follow-up that owns them before treating this slice as independently mergeable. Second, the reproduced consumer is NemoClaw-managed MCP, but the implementation copies every non-provider live network route. That broadens the policy contract without an accepted requirement or registry ownership check. Narrow the merge to the current registered consumer, or provide the accepted product/security decision that authorizes all live routes.
The fallback also silently returns the static policy when parsing or temporary-file I/O fails, which recreates the route-loss defect. Record the intended fail-closed or degraded behavior in the accepted design and protect it with a behavior test. CI for the maintainer-updated commit is now running; the documentation-writer receipt must be refreshed after the new commit.
…merge Review asked for two contract changes. Narrow the merge to the registered consumer. The permissive swap now carries a live network route only when the registry records it for that sandbox, so an unregistered live route is left behind instead of being promoted into the applied policy. `registeredNetworkPolicyKeys` reads the registry through the existing policy seam, so no module gains a dependency. Stop losing owned routes silently. When the baseline cannot be read or parsed, or the merged policy cannot be written, shields down still applies the static policy — aborting would strand the sandbox in a locked posture — but it now says which routes were dropped and how to restore them. Refs: NVIDIA#7952 Signed-off-by: harjoth <harjoth.khara@gmail.com>
The degrade warning told the operator to run `mcp restart`, but the registry slot it reads holds two kinds of entry: generated MCP bridge policies and custom presets added with `policy add --from-file`. A dropped custom preset is not restored by `mcp restart`, so the instruction was wrong for half the cases it can fire on. Name the routes that were lost and give the command that owns each kind. Also say "applied policy" rather than naming the permissive document, matching the vocabulary the docs use. Refs: NVIDIA#7952 Signed-off-by: harjoth <harjoth.khara@gmail.com>
…-network-routes Signed-off-by: harjoth <harjoth.khara@gmail.com> # Conflicts: # src/lib/shields/flow.test.ts
|
Follow-up on exact head The first release-contract blocker remains. Issue #7952 still requires shields-up/automatic restoration to reconcile entries added or removed during the shields-down window and requires the Hermes Please complete those criteria here or link an accepted, sequenced follow-up/waiver, then resolve the conflict and refresh exact-head evidence. The existing requested-changes state remains accurate. |
|
All three addressed on Sequencing. #8237 owns the restore leg and the live Hermes qualification, and is linked here. Breadth. Narrowed in Fallback. Kept degraded rather than fail-closed — aborting when The first version of that warning said Docs receipt refreshed; main merged to clear #8130. Not covered here, tracked in #8237: the shields-up direction and the live E2E. |
5 participants
Summary
shields downreplaced the whole live OpenShell policy with the static permissive baseline. That baseline is a fixed network allowlist, so a route registered at runtime — a generated MCP bridge entry — was dropped from the effective gateway policy. The server stayed in the registry with its provider attached and its adapter registered, but the gateway no longer served it, so tool discovery and tool calls failed untilmcp restartreapplied the generated policy.buildRuntimePermissivePolicyalready unions the live sandbox's filesystem paths into that baseline for the same reason. This extends it tonetwork_policies, so live-only routes survive the swap.Related Issue
Refs #7952. This fixes the shields-down direction only — see Scope below.
Changes
src/lib/shields/permissive-runtime.ts— union the live policy'snetwork_policiesinto the permissive document. The static base wins on a shared key, so every route the baseline declares is applied byte-for-byte as before. The "nothing to merge" early return now also considers network routes._provider_*entries are never copied.policy get --basecan include them defensively andpolicy setmust not receive them; every other read-modify-write path filters them viawithoutProviderComposedPolicies. The prefix is inlined rather than imported because that module is only reachable through a built artifact and this helper is dependency-free.src/lib/shields/index.ts— comment only, at the call site.No new configuration, flag, or abstraction.
Scope
Deliberately not in this PR, so a reviewer is not surprised:
The restore direction is unchanged.
shields up, inline expiry recovery, the auto-restore handoff, rollback, and the detached timer still reapply the captured snapshot verbatim (src/lib/shields/index.ts:2229,:2341,:2400,:2508, andsrc/lib/shields/timer.ts:294). A route registered during the shields-down window is still discarded on restore. That is acceptance criterion 2, and it is a separate change on purpose: a correct restore cannot union the permissive document into the snapshot — that would carry permissive-only baseline entries into the locked posture. It needs registry-scoped reconciliation of additions and removals, which is a decision about what may enter lockdown. It also lands in the two files open PR refactor(architecture): collapse runtime cycle to policy seam #7749 is refactoring.Sequenced follow-up: Shields restoration reapplies a stale policy snapshot and drops routes registered during the window #8237 owns this half. It names the five restoration call sites, records why the naive union is unsafe (it would carry permissive-only baseline entries into the locked posture), lists the open design questions, and carries the live Hermes MCP qualification — including that the current lane runs
mcp restart Abefore proving A works, which masks the defect.Narrowed to the registered consumer. A live route is carried only when the registry records it for that sandbox; an unregistered live route is left behind.
registeredNetworkPolicyKeysreads the registry through the existing policy seam, so no module gained a dependency — the architecture budget reports the same 4503 edges before and after. Note the registry slot holds both generated MCP bridge policies and custom presets added withpolicy add --from-file; both are registered consumers and both are preserved.shields down --policy <path>is unchanged. An explicitly supplied policy file is still applied verbatim.The I/O fallback degrades, and now says so. If the baseline cannot be read or parsed, or the merged policy cannot be written, the static policy is applied and owned routes are lost — aborting shields down would strand the sandbox in a locked posture. That degrade is deliberate and documented on the helper, and it is no longer silent: the command names the routes that were dropped and the command that restores each kind. Four tests pin it, including that nothing is reported when no owned route was at stake.
Type of Change
Quality Gates
mcp restartworkaround for it, and no command, flag, default, or documented contract changes. The fix restores behavior the docs already assume.Documentation Writer Review
no-docs-needed8c80c7653, which adds operator-visible warning output. Checkeddocs/reference/troubleshoot-mcp-servers.mdx,docs/reference/troubleshooting.mdx(shields entries and the network-policy denial precedent),docs/manage-sandboxes/runtime-controls.mdx,docs/reference/commands.mdx(theshieldsandpolicy addentries),docs/deployment/set-up-mcp-bridge.mdx,docs/manage-sandboxes/manage-mcp-servers.mdx, anddocs/security/tcb-boundary.mdx(whose own rule asks reviewers to re-check when the shields coordinator changes). No page states or implies the old behavior.docs/.docs-skipexcludes bothopenclaw-sandbox-permissive.yamland the permissive-mode concept from generated content, so documenting the merge itself is a maintainer scope decision rather than a bug-fix change; the warning names its own remediation.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)Proof
The red comes from the real
shieldsDownpath, not the helper alone.src/lib/shields/flow.test.tsdrives the exportedshieldsDownwith only the process boundary stubbed, and reads the document handed toopenshell policy setat call time.Against upstream sources, with a live policy carrying
mcp_bridge_fakeand a baseline carryingnvidia:The applied document is the bare baseline. The route is gone.
Helper-level red, same commit, using the real generated policy from
buildMcpBridgePolicyYamlrather than hand-written YAML:After the fix:
A second commit adds a behavior parity test: the merge and the canonical
withoutProviderComposedPoliciesare driven with the same routes and must keep the same set, so the inlined_provider_prefix cannot drift from the canonical filter unnoticed. It fails if either side changes.Blast radius was taken by symbol, not by directory:
buildRuntimePermissivePolicyhas one production caller (src/lib/shields/index.ts:2665) and three test files (test/permissive-runtime.test.ts,src/lib/shields/flow.test.ts,src/lib/shields/legacy-hermes-compat.test.ts). Theintegrationproject is excluded fromnpm run test:changed, so those files were run explicitly.Limits
mcp-bridgeE2E (acceptance criterion 6) was not run. It needs Brev, which I do not have.test/e2e/live/mcp-bridge.test.tsstill callsmcp restart Abefore proving A works, which preserves the masking behavior the issue describes; proving A across B's lifecycle without that restart needs the live lane.test/e2e/support/lifecycle-user-service.test.tsfails on this machine, but it fails identically on pristineorigin/mainwith none of this diff applied. Not caused by this change.Signed-off-by: harjoth harjoth.khara@gmail.com
Summary by CodeRabbit
Bug Fixes
Documentation