Skip to content

fix(policy): reject messaging channel presets on terminal-runtime agents - #6197

Merged
cv merged 6 commits into
mainfrom
fix/dcode-policy-add-channel-gate
Jul 2, 2026
Merged

fix(policy): reject messaging channel presets on terminal-runtime agents#6197
cv merged 6 commits into
mainfrom
fix/dcode-policy-add-channel-gate

Conversation

@laitingsheng

@laitingsheng laitingsheng commented Jul 2, 2026

Copy link
Copy Markdown
Contributor

Summary

policy-add accepted the five messaging channel presets (telegram, discord, slack, wechat, whatsapp) on terminal-runtime DeepAgents sandboxes, which have no inbound messaging gateway. It now rejects an unsupported channel preset up front, mirroring the existing channels add agent gate.

Related Issue

Fixes #6185

Changes

  • addSandboxPolicy resolves the sandbox agent and rejects a messaging channel preset the agent's channel manifests do not support, before any endpoint disclosure or confirmation prompt, exiting non-zero.
  • Unsupported channel presets are dropped from the interactive picker and the valid-presets list.
  • Reuses the same message and supportedAgents source as channels add; non-messaging presets and messaging-capable agents (openclaw, hermes) are unaffected.
  • Docs: note the terminal-runtime rejection in the policy-add reference.
  • Tests: action-boundary unit coverage for all five channels, plus a spawned-process behaviour test asserting non-zero exit, no endpoint disclosure, and no prompt.

Type of Change

  • Code change (feature, bug fix, or refactor)
  • Code change with doc updates
  • Doc only (prose changes, no code sample modifications)
  • Doc only (includes code sample changes)

Quality Gates

  • Tests added or updated for changed behavior
  • Existing tests cover changed behavior — justification:
  • Tests not applicable — justification:
  • Docs updated for user-facing behavior changes
  • Docs not applicable — justification:
  • Sensitive paths changed (security, policy, credentials, preflight, onboarding, inference, runner, sandbox, or messaging)
  • Sensitive-path review completed or maintainer-approved waiver recorded — reviewer/approval link/justification:
  • Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue:

Verification

  • PR description includes the DCO sign-off declaration and every commit appears as Verified in GitHub
  • Git hooks passed during commit and push, or npx prek run --from-ref main --to-ref HEAD passes
  • Targeted tests pass for changed behavior
  • Full npm test passes (broad runtime changes only)
  • Quality Gates section completed with required justifications or waivers
  • No secrets, API keys, or credentials committed
  • npm run docs builds without warnings (doc changes only)
  • Doc pages follow the style guide (doc changes only)
  • New doc pages include SPDX header and frontmatter (new pages only)

Signed-off-by: Tinson Lai tinsonl@nvidia.com

Summary by CodeRabbit

  • Bug Fixes
    • Messaging-channel presets (Telegram/Discord/Slack/WeChat/WhatsApp) are now rejected when an agent doesn’t support the channel, with a clear error before any confirmation or endpoint disclosure.
    • --from-file/--from-dir custom presets containing messaging policy keys are refused early for unsupported terminal-runtime agents; supported agents still apply them.
  • Documentation
    • Updated policy-add reference docs to clarify messaging-preset applicability and the early-rejection behavior for terminal-runtime agents.
  • Tests
    • Added/extended tests to verify early rejection, no prompting, and no preset loading/applying.

@github-code-quality

github-code-quality Bot commented Jul 2, 2026

Copy link
Copy Markdown
Contributor

Code Coverage Overview

Languages: TypeScript

TypeScript / code-coverage/plugin

The overall coverage in the fix/dcode-policy-add... branch is 96%. Coverage data for the main branch is not yet available.

Show a code coverage summary of the most covered files.
File main fix/dcode-policy-add... 8c3a9f7 +/-
nemoclaw/src/se...cret-scanner.ts 100%
nemoclaw/src/commands/slash.ts 100%
nemoclaw/src/bl...eprint/state.ts 98%
nemoclaw/src/onboard/config.ts 98%
nemoclaw/src/bl...int/snapshot.ts 97%
nemoclaw/src/blueprint/ssrf.ts 97%
nemoclaw/src/bl...print/runner.ts 95%
nemoclaw/src/co...ration-state.ts 94%
nemoclaw/src/bl...ate-networks.ts 94%
nemoclaw/src/index.ts 94%

TypeScript / code-coverage/cli

The overall coverage in the fix/dcode-policy-add... branch is 69%. Coverage data for the main branch is not yet available.

Show a code coverage summary of the most covered files.
File main fix/dcode-policy-add... 8c3a9f7 +/-
src/lib/actions...dbox/rebuild.ts 82%
src/lib/actions...all/run-plan.ts 80%
src/lib/state/o...oard-session.ts 79%
src/lib/shields/index.ts 75%
src/lib/state/sandbox.ts 73%
src/lib/onboard/preflight.ts 69%
src/lib/actions...licy-channel.ts 59%
src/lib/onboard...er-gpu-patch.ts 59%
src/lib/policy/index.ts 56%
src/lib/onboard.ts 20%

Updated July 02, 2026 16:13 UTC
Code Coverage is in Public Preview. Learn more and provide us with your feedback.

@laitingsheng laitingsheng added NV QA Bugs found by the NVIDIA QA Team area: cli Command line interface, flags, terminal UX, or output bug-fix PR fixes a bug or regression integration: dcode LangChain Deep Code integration behavior labels Jul 2, 2026
@github-actions

github-actions Bot commented Jul 2, 2026

Copy link
Copy Markdown
Contributor

E2E Advisor Recommendation

Required E2E: ubuntu-repo-cloud-langchain-deepagents-code
Optional E2E: channels-add-remove, messaging-providers

Dispatch hint: targets=ubuntu-repo-cloud-langchain-deepagents-code

Workflow run

Full advisor summary

E2E Recommendation Advisor

Base: origin/main
Head: HEAD
Confidence: high

Required E2E

  • ubuntu-repo-cloud-langchain-deepagents-code (medium): Required because this PR changes network-policy mutation behavior for the DeepAgents terminal-runtime agent. The live DeepAgents cloud target validates the real onboarded terminal-agent sandbox, hosted inference, and DeepAgents policy/egress boundary so regressions in agent resolution or policy gating are caught beyond unit mocks.

Optional E2E

  • channels-add-remove (medium): Useful adjacent confidence because the changed code shares messaging channel compatibility helpers and agent resolution with channel lifecycle behavior; this validates a real OpenClaw Telegram channel add/remove path still applies messaging policy and rebuild state correctly.
  • messaging-providers (medium): Useful broader confidence for messaging policy/provider integration, token isolation, and Telegram/Discord/Slack/WhatsApp reachability after this PR changes messaging preset gating. Not required because the primary modified command path is policy-add, not provider setup.

New E2E recommendations

  • network-policy (high): Existing live E2E coverage validates DeepAgents policy egress and messaging provider/channel flows, but no existing named E2E appears to directly assert that nemoclaw <deepagents-sandbox> policy-add telegram|discord|slack|wechat|whatsapp and --from-file messaging-policy presets fail before endpoint disclosure/prompt/apply on a real onboarded terminal-agent sandbox.
    • Suggested test: Add a live DeepAgents policy-add channel rejection E2E that runs against ubuntu-repo-cloud-langchain-deepagents-code and verifies built-in and custom messaging presets are rejected before disclosure and without gateway policy mutation.

Dispatch hint

  • Workflow: .github/workflows/e2e.yaml
  • jobs input: targets=ubuntu-repo-cloud-langchain-deepagents-code

@github-actions

github-actions Bot commented Jul 2, 2026

Copy link
Copy Markdown
Contributor

E2E Target Recommendation

Required E2E targets: ubuntu-repo-cloud-langchain-deepagents-code
Optional E2E targets: ubuntu-repo-cloud-openclaw

Dispatch required E2E targets:

  • gh workflow run e2e.yaml --ref <pr-head-ref> --field targets=ubuntu-repo-cloud-langchain-deepagents-code

Workflow run

Full E2E target advisor summary

E2E Target Advisor

Base: origin/main
Head: HEAD
Confidence: high

Required E2E targets

  • ubuntu-repo-cloud-langchain-deepagents-code: The PR changes policy-add channel gating and sandbox agent resolution for the LangChain DeepAgents Code terminal-runtime agent. This live-supported target onboards the DeepAgents Code agent and includes the deepagents-code-policy/terminal-agent suites that exercise policy-add behavior against a real sandbox.
    • Dispatch: gh workflow run e2e.yaml --ref <pr-head-ref> --field targets=ubuntu-repo-cloud-langchain-deepagents-code

Optional E2E targets

  • ubuntu-repo-cloud-openclaw: Optional adjacent coverage for the messaging-capable OpenClaw path after refactoring policy-add agent resolution; the primary regression surface is DeepAgents Code rejection.
    • Dispatch: gh workflow run e2e.yaml --ref <pr-head-ref> --field targets=ubuntu-repo-cloud-openclaw

Relevant changed files

  • src/lib/actions/sandbox/policy-channel.ts
  • src/lib/sandbox/version.ts

@github-actions

github-actions Bot commented Jul 2, 2026

Copy link
Copy Markdown
Contributor

PR Review Advisor (Nemotron Ultra) — Changes requested

Merge posture: Do not merge yet
Primary next action: Fix PRA-3: Monolith growth: policy-channel.ts exceeds size threshold; then add or justify PRA-T1.
Open items: 1 required · 10 warnings · 2 suggestions · 8 test follow-ups
Since last review: 1 prior item resolved · 11 still apply · 2 new items found

Action checklist

  • PRA-3 Fix: Monolith growth: policy-channel.ts exceeds size threshold in src/lib/actions/sandbox/policy-channel.ts:1
  • PRA-1 Resolve or justify: Source-of-truth review needed: src/lib/actions/sandbox/policy-channel.ts:377
  • PRA-2 Resolve or justify: Source-of-truth review needed: src/lib/actions/sandbox/policy-channel.ts:379
  • PRA-4 Resolve or justify: Content-based gate bypass via unknown custom policy keys in src/lib/actions/sandbox/policy-channel.ts:385
  • PRA-5 Resolve or justify: Source-of-truth review: non-string content guard is localized workaround in src/lib/actions/sandbox/policy-channel.ts:377
  • PRA-6 Resolve or justify: Source-of-truth review: empty policy keys early return is localized workaround in src/lib/actions/sandbox/policy-channel.ts:379
  • PRA-7 Resolve or justify: formatAvailableChannelsForAgent returns confusing '(none)' for terminal agents in src/lib/actions/sandbox/policy-channel.ts:411
  • PRA-8 Resolve or justify: Integration test uses slow subprocess spawning duplicating unit test coverage in test/policy-add-deepagents-rejection.test.ts:1
  • PRA-9 Resolve or justify: Missing unit test for non-string content guard in unsupportedMessagingChannelForPresetContent in src/lib/actions/sandbox/policy-add-agent-gate.test.ts:1
  • PRA-10 Resolve or justify: Missing test for unknown policy key bypass scenario in src/lib/actions/sandbox/policy-add-agent-gate.test.ts:1
  • PRA-12 Resolve or justify: Missing security test coverage for content-based gate bypass and non-string content guard in src/lib/actions/sandbox/policy-channel.ts:385
  • PRA-T1 Add or justify test follow-up: Runtime validation
  • PRA-T2 Add or justify test follow-up: Runtime validation
  • PRA-T3 Add or justify test follow-up: Runtime validation
  • PRA-T4 Add or justify test follow-up: Integration test uses slow subprocess spawning duplicating unit test coverage
  • PRA-T5 Add or justify test follow-up: Missing unit test for non-string content guard in unsupportedMessagingChannelForPresetContent
  • PRA-T6 Add or justify test follow-up: Missing test for unknown policy key bypass scenario
  • PRA-T7 Add or justify test follow-up: src/lib/actions/sandbox/policy-channel.ts:377
  • PRA-T8 Add or justify test follow-up: src/lib/actions/sandbox/policy-channel.ts:379
  • PRA-11 In-scope improvement: Error message for custom presets could clarify content-based match in src/lib/actions/sandbox/policy-channel.ts:255
  • PRA-13 In-scope improvement: resolveAgentForSandbox deduplication completed — PRA-4 resolved in src/lib/actions/sandbox/policy-channel.ts:349

Findings index

ID Severity Category Location Required action
PRA-1 Resolve/justify architecture Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
PRA-2 Resolve/justify architecture Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
PRA-3 Required architecture src/lib/actions/sandbox/policy-channel.ts:1 Extract channel lifecycle (add/remove/start/stop) to src/lib/actions/sandbox/channels.ts, preset sync to policy-sync.ts, keep only policy-add/remove/list in policy-channel.ts. At minimum, offset the 54-line growth by extracting existing logic before merge.
PRA-4 Resolve/justify security src/lib/actions/sandbox/policy-channel.ts:385 Add secondary host-pattern check: scan preset endpoints against known messaging provider host patterns (api.telegram.org, discord.com, api.slack.com, api.weixin.qq.com, graph.facebook.com, graph.microsoft.com). Document as known limitation if not fixed in this PR.
PRA-5 Resolve/justify architecture src/lib/actions/sandbox/policy-channel.ts:377 Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition. The guard can be removed when loadPresetFromFile return type is enforced as string at TypeScript level.
PRA-6 Resolve/justify architecture src/lib/actions/sandbox/policy-channel.ts:379 Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition. The early return can be removed when preset validation rejects empty network_policies at load time.
PRA-7 Resolve/justify correctness src/lib/actions/sandbox/policy-channel.ts:411 Change return value to '(none — this agent has no messaging support)' or add clarifying note in the error message where this is displayed.
PRA-8 Resolve/justify tests test/policy-add-deepagents-rejection.test.ts:1 Consolidate behavioral coverage into the faster unit test file using mocks, or justify why subprocess spawning is needed for these specific cases.
PRA-9 Resolve/justify tests src/lib/actions/sandbox/policy-add-agent-gate.test.ts:1 Add unit test: mock loadPresetFromFile to return { presetName: 'test', content: 123 } or null; verify gate behavior (should reject or handle gracefully without crashing).
PRA-10 Resolve/justify tests src/lib/actions/sandbox/policy-add-agent-gate.test.ts:1 Add test: custom preset with policy key 'my_custom_key' and host 'api.telegram.org' on DeepAgents sandbox; assert current behavior (bypass) to lock in expected behavior, or assert rejection if host-pattern check is implemented.
PRA-11 Improvement correctness src/lib/actions/sandbox/policy-channel.ts:255 Clarify error message: 'Preset 'X' contains network policies matching the Y channel (policy key: K), which does not support agent Z' to make the content-based match explicit.
PRA-12 Resolve/justify security src/lib/actions/sandbox/policy-channel.ts:385 Add tests for PRA-8 and PRA-9 scenarios to lock in current behavior or verify fix.
PRA-13 Improvement architecture src/lib/actions/sandbox/policy-channel.ts:349 No action needed — verified fixed in this PR.

🚨 Required before merge

Address these before merging unless a maintainer explicitly overrides the advisor with rationale.

PRA-3 Required — Monolith growth: policy-channel.ts exceeds size threshold

  • Location: src/lib/actions/sandbox/policy-channel.ts:1
  • Category: architecture
  • Problem: policy-channel.ts grew from 1544 to 1598 lines (+54 this PR). File handles policy add/remove/list, channel add/remove/start/stop, conflict detection, preset sync, and custom preset agent-gate logic across 8+ exported functions, obscuring security boundaries.
  • Impact: Large mixed-concern file degrades maintainability and auditability; future changes riskier; security boundaries harder to verify.
  • Required action: Extract channel lifecycle (add/remove/start/stop) to src/lib/actions/sandbox/channels.ts, preset sync to policy-sync.ts, keep only policy-add/remove/list in policy-channel.ts. At minimum, offset the 54-line growth by extracting existing logic before merge.
  • Expected follow-up: Fix before merge or get explicit maintainer override.
  • Verification: wc -l src/lib/actions/sandbox/policy-channel.ts shows 1598 lines; grep -c '^export async function' shows 8+ exported functions spanning policy and channel concerns
  • Missing regression test: Architecture test enforcing max file size for policy-channel.ts (e.g., 1200 lines)
  • Done when: The required change is committed and verification passes: wc -l src/lib/actions/sandbox/policy-channel.ts shows 1598 lines; grep -c '^export async function' shows 8+ exported functions spanning policy and channel concerns.
  • Evidence: Diff shows +59 lines in policy-channel.ts; drift context reports delta 54 with severity blocker; previous review PRA-3 unresolved
Review findings by urgency: 1 required fix, 10 items to resolve/justify, 2 in-scope improvements

⚠️ Resolve or justify before merge

Investigate these in the current review; either fix them, explain why they are not applicable, or document the accepted risk.

PRA-1 Resolve/justify — Source-of-truth review needed: src/lib/actions/sandbox/policy-channel.ts:377

  • Location: not file-specific
  • Category: architecture
  • Problem: The advisor marked localized patch analysis as needs_followup.
  • Impact: A localized workaround can preserve or hide an invalid state when the source boundary is unclear.
  • Recommended action: Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Missing regression test: MISSING — add test mocking loadPresetFromFile to return non-string content
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Evidence: PRA-11 from previous review marked needs_followup; loadPresetFromFile validates YAML and returns string content

PRA-2 Resolve/justify — Source-of-truth review needed: src/lib/actions/sandbox/policy-channel.ts:379

  • Location: not file-specific
  • Category: architecture
  • Problem: The advisor marked localized patch analysis as needs_followup.
  • Impact: A localized workaround can preserve or hide an invalid state when the source boundary is unclear.
  • Recommended action: Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Missing regression test: MISSING — add test for empty preset rejection at load time
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Evidence: PRA-12 from previous review marked needs_followup; policyKeys.size === 0 returns null from gate

PRA-4 Resolve/justify — Content-based gate bypass via unknown custom policy keys

  • Location: src/lib/actions/sandbox/policy-channel.ts:385
  • Category: security
  • Problem: The gate in unsupportedMessagingChannelForPresetContent only matches against known policy keys from messaging manifests. A custom preset with an unknown policy key (e.g., 'my_custom_key') pointing to a known messaging host (e.g., 'api.telegram.org') would bypass the agent gate.
  • Impact: Defense-in-depth gap: attacker could craft custom preset with unknown key targeting messaging provider host to widen sandbox egress on terminal-runtime agents.
  • Recommended action: Add secondary host-pattern check: scan preset endpoints against known messaging provider host patterns (api.telegram.org, discord.com, api.slack.com, api.weixin.qq.com, graph.facebook.com, graph.microsoft.com). Document as known limitation if not fixed in this PR.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Create custom preset with policy key 'my_custom_key' and host 'api.telegram.org'; apply to DeepAgents sandbox; observe it bypasses gate
  • Missing regression test: Test: custom preset with unknown policy key pointing to known messaging host is rejected on terminal-runtime agent
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Create custom preset with policy key 'my_custom_key' and host 'api.telegram.org'; apply to DeepAgents sandbox; observe it bypasses gate.
  • Evidence: unsupportedMessagingChannelForPresetContent iterates listMessagingPolicyPresetMetadata() policyKeys; unknown keys not in any manifest pass through unchecked

PRA-5 Resolve/justify — Source-of-truth review: non-string content guard is localized workaround

  • Location: src/lib/actions/sandbox/policy-channel.ts:377
  • Category: architecture
  • Problem: loadPresetFromFile already validates YAML and returns { presetName: string, content: string }. The typeof content !== 'string' guard at line 377 handles an invalid state that should be impossible at the source boundary.
  • Impact: Localized workaround preserves/hides invalid state; if loadPresetFromFile ever returned non-string, gate would silently pass (return null) instead of failing fast.
  • Recommended action: Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition. The guard can be removed when loadPresetFromFile return type is enforced as string at TypeScript level.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Inspect loadPresetFromFile at policy/index.ts:1031 — returns { presetName: string, content: string } after YAML validation
  • Missing regression test: Test: non-string preset content does not bypass agent gate (mock loadPresetFromFile to return non-string content)
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Inspect loadPresetFromFile at policy/index.ts:1031 — returns { presetName: string, content: string } after YAML validation.
  • Evidence: PRA-11 from previous review marked needs_followup; loadPresetFromFile validates YAML and returns string content

PRA-6 Resolve/justify — Source-of-truth review: empty policy keys early return is localized workaround

  • Location: src/lib/actions/sandbox/policy-channel.ts:379
  • Category: architecture
  • Problem: A preset without network_policies keys returns null from the gate, but presets should require network_policies at load time.
  • Impact: Localized workaround preserves invalid state; empty network_policies preset silently passes through gate instead of being rejected at source.
  • Recommended action: Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition. The early return can be removed when preset validation rejects empty network_policies at load time.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Inspect loadPresetFromFile at policy/index.ts:1031 — validates network_policies exists but allows empty object
  • Missing regression test: Test: preset with empty network_policies rejected at load time, not silently passed through gate
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Inspect loadPresetFromFile at policy/index.ts:1031 — validates network_policies exists but allows empty object.
  • Evidence: PRA-12 from previous review marked needs_followup; policyKeys.size === 0 returns null from gate

PRA-7 Resolve/justify — formatAvailableChannelsForAgent returns confusing '(none)' for terminal agents

  • Location: src/lib/actions/sandbox/policy-channel.ts:411
  • Category: correctness
  • Problem: The error message shows 'Channels supported by agent 'langchain-deepagents-code': (none)' which doesn't clarify that the agent has no messaging support at all.
  • Impact: User confusion: '(none)' could mean no channels configured vs no messaging capability.
  • Recommended action: Change return value to '(none — this agent has no messaging support)' or add clarifying note in the error message where this is displayed.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Run policy-add with a messaging preset on DeepAgents sandbox; observe error message shows '(none)'
  • Missing regression test: Test: error message for unsupported channel on terminal agent clarifies no messaging support
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Run policy-add with a messaging preset on DeepAgents sandbox; observe error message shows '(none)'.
  • Evidence: formatAvailableChannelsForAgent returns join(', ') || '(none)'; used in error messages at lines 54, 103

PRA-8 Resolve/justify — Integration test uses slow subprocess spawning duplicating unit test coverage

  • Location: test/policy-add-deepagents-rejection.test.ts:1
  • Category: tests
  • Problem: The behavioral test spawns Node subprocesses (15s timeout each) to test the same gate logic already covered by fast unit tests in policy-add-agent-gate.test.ts.
  • Impact: Adds ~150s to test suite for marginal additional coverage; slows CI feedback loop.
  • Recommended action: Consolidate behavioral coverage into the faster unit test file using mocks, or justify why subprocess spawning is needed for these specific cases.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: npm test -- test/policy-add-deepagents-rejection.test.ts — observe ~150s runtime for 10 tests
  • Missing regression test: N/A — test exists but architecture is inefficient
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: npm test -- test/policy-add-deepagents-rejection.test.ts — observe ~150s runtime for 10 tests.
  • Evidence: 10 tests × ~15s = ~150s runtime for behavioral verification already covered by 6 fast unit tests (<1s total)

PRA-9 Resolve/justify — Missing unit test for non-string content guard in unsupportedMessagingChannelForPresetContent

  • Location: src/lib/actions/sandbox/policy-add-agent-gate.test.ts:1
  • Category: tests
  • Problem: The guard at line 377 (typeof content !== 'string') has no test coverage.
  • Impact: Branch not exercised; if loadPresetFromFile ever returned non-string, behavior unverified.
  • Recommended action: Add unit test: mock loadPresetFromFile to return { presetName: 'test', content: 123 } or null; verify gate behavior (should reject or handle gracefully without crashing).
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Search policy-add-agent-gate.test.ts for 'non-string' or 'typeof content' — no matches
  • Missing regression test: Test: non-string preset content does not bypass agent gate
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Search policy-add-agent-gate.test.ts for 'non-string' or 'typeof content' — no matches.
  • Evidence: Test file covers 6 scenarios but none mock non-string content from loadPresetFromFile

PRA-10 Resolve/justify — Missing test for unknown policy key bypass scenario

  • Location: src/lib/actions/sandbox/policy-add-agent-gate.test.ts:1
  • Category: tests
  • Problem: A custom preset with a policy key not in any manifest (e.g., 'my_custom_key') but pointing to a known messaging host would bypass the content-based gate.
  • Impact: Current bypass behavior untested; if host-pattern check (PRA-5) is implemented, regression undetected.
  • Recommended action: Add test: custom preset with policy key 'my_custom_key' and host 'api.telegram.org' on DeepAgents sandbox; assert current behavior (bypass) to lock in expected behavior, or assert rejection if host-pattern check is implemented.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Search policy-add-agent-gate.test.ts for 'unknown' or 'custom' policy key — no matches
  • Missing regression test: Test: custom preset with unknown policy key pointing to known messaging host is rejected on terminal-runtime agent
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Search policy-add-agent-gate.test.ts for 'unknown' or 'custom' policy key — no matches.
  • Evidence: unsupportedMessagingChannelForPresetContent only matches against known policyKeys from listMessagingPolicyPresetMetadata()

PRA-12 Resolve/justify — Missing security test coverage for content-based gate bypass and non-string content guard

  • Location: src/lib/actions/sandbox/policy-channel.ts:385
  • Category: security
  • Problem: Category 8 (Security Testing): Missing test coverage for content-based gate bypass via unknown custom policy keys and non-string content guard.
  • Impact: Security gaps in gate logic not verified by tests; regressions could be introduced without detection.
  • Recommended action: Add tests for PRA-8 and PRA-9 scenarios to lock in current behavior or verify fix.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Run test suite and verify no test covers unknown policy key bypass or non-string content guard
  • Missing regression test: Tests for unknown policy key bypass and non-string content guard
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Run test suite and verify no test covers unknown policy key bypass or non-string content guard.
  • Evidence: Security category 8 review found missing coverage for PRA-5 bypass and PRA-8 guard

💡 In-scope improvements

These are lower-risk, not throwaway. Prefer fixing them in this PR when they are local to changed code; defer only with rationale or a linked follow-up.

PRA-11 Improvement — Error message for custom presets could clarify content-based match

  • Location: src/lib/actions/sandbox/policy-channel.ts:255
  • Category: correctness
  • Problem: Current message: 'Preset 'X' targets the 'Y' channel' implies the preset declares a channel identity, but it's actually a content-based policy key match.
  • Impact: Minor UX confusion: user may think preset has channel metadata when it only matches policy keys.
  • Suggested action: Clarify error message: 'Preset 'X' contains network policies matching the Y channel (policy key: K), which does not support agent Z' to make the content-based match explicit.
  • Expected follow-up: Prefer a current-PR fix when local to changed code; defer only with rationale or linked follow-up.
  • Verification: Apply custom preset with telegram_bot key on DeepAgents sandbox; observe error says 'targets the telegram channel' rather than 'contains network policies matching the telegram channel'
  • Missing regression test: N/A — UX improvement
  • Done when: The local improvement is applied, or the PR notes why it should be deferred.
  • Evidence: Line 255 in applyExternalPreset constructs error from unsupportedMessagingChannelForPresetContent result

PRA-13 Improvement — resolveAgentForSandbox deduplication completed — PRA-4 resolved

  • Location: src/lib/actions/sandbox/policy-channel.ts:349
  • Category: architecture
  • Problem: Function now exported from version.ts and imported in policy-channel.ts — single source of truth achieved.
  • Impact: Positive: eliminates duplicate function definitions that could diverge.
  • Suggested action: No action needed — verified fixed in this PR.
  • Expected follow-up: Prefer a current-PR fix when local to changed code; defer only with rationale or linked follow-up.
  • Verification: grep -n 'resolveAgentForSandbox' src/lib/actions/sandbox/policy-channel.ts src/lib/sandbox/version.ts shows single definition in version.ts and import in policy-channel.ts
  • Missing regression test: Unit test verifying single source of truth for agent resolution
  • Done when: The local improvement is applied, or the PR notes why it should be deferred.
  • Evidence: PR commit 8c3a9f7 refactor(sandbox): deduplicate resolveAgentForSandbox into version.ts
Simplification opportunities: 1 possible cut, net -30 lines possible

These are safe simplification checks only. Do not remove validation, security controls, data-loss prevention, or required tests.

  • PRA-13 stdlib (src/lib/actions/sandbox/policy-channel.ts:349): Local resolveAgentForSandbox function definition (previously 30 lines in policy-channel.ts)
    • Replacement: Import from '../../sandbox/version'
    • Net: -30 lines
    • Safety boundary: Agent resolution logic unchanged; single source of truth now enforced
Test follow-ups to resolve or justify

If these cover changed behavior, prefer adding them in this PR; otherwise state why existing coverage is enough or link the follow-up.

  • PRA-T1 Runtime validation — Add unit test for non-string content guard in unsupportedMessagingChannelForPresetContent. Runtime/sandbox/infrastructure paths need behavioral runtime validation: docs/reference/commands-nemohermes.mdx, docs/reference/commands.mdx, src/lib/actions/sandbox/policy-channel.ts, src/lib/sandbox/version.ts. Unit tests cover gate logic but integration test uses inefficient subprocess spawning.
  • PRA-T2 Runtime validation — Add unit test for unknown policy key bypass scenario. Runtime/sandbox/infrastructure paths need behavioral runtime validation: docs/reference/commands-nemohermes.mdx, docs/reference/commands.mdx, src/lib/actions/sandbox/policy-channel.ts, src/lib/sandbox/version.ts. Unit tests cover gate logic but integration test uses inefficient subprocess spawning.
  • PRA-T3 Runtime validation — Consolidate behavioral coverage from test/policy-add-deepagents-rejection.test.ts into policy-add-agent-gate.test.ts using existing mocks. Runtime/sandbox/infrastructure paths need behavioral runtime validation: docs/reference/commands-nemohermes.mdx, docs/reference/commands.mdx, src/lib/actions/sandbox/policy-channel.ts, src/lib/sandbox/version.ts. Unit tests cover gate logic but integration test uses inefficient subprocess spawning.
  • PRA-T4 Integration test uses slow subprocess spawning duplicating unit test coverage — Consolidate behavioral coverage into the faster unit test file using mocks, or justify why subprocess spawning is needed for these specific cases.
  • PRA-T5 Missing unit test for non-string content guard in unsupportedMessagingChannelForPresetContent — Add unit test: mock loadPresetFromFile to return { presetName: 'test', content: 123 } or null; verify gate behavior (should reject or handle gracefully without crashing).
  • PRA-T6 Missing test for unknown policy key bypass scenario — Add test: custom preset with policy key 'my_custom_key' and host 'api.telegram.org' on DeepAgents sandbox; assert current behavior (bypass) to lock in expected behavior, or assert rejection if host-pattern check is implemented.
  • PRA-T7 src/lib/actions/sandbox/policy-channel.ts:377 — MISSING — add test mocking loadPresetFromFile to return non-string content. PRA-11 from previous review marked needs_followup; loadPresetFromFile validates YAML and returns string content
  • PRA-T8 src/lib/actions/sandbox/policy-channel.ts:379 — MISSING — add test for empty preset rejection at load time. PRA-12 from previous review marked needs_followup; policyKeys.size === 0 returns null from gate
Since last review details

Current findings, using the urgency labels above:

PRA-1 Resolve/justify — Source-of-truth review needed: src/lib/actions/sandbox/policy-channel.ts:377

  • Location: not file-specific
  • Category: architecture
  • Problem: The advisor marked localized patch analysis as needs_followup.
  • Impact: A localized workaround can preserve or hide an invalid state when the source boundary is unclear.
  • Recommended action: Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Missing regression test: MISSING — add test mocking loadPresetFromFile to return non-string content
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Evidence: PRA-11 from previous review marked needs_followup; loadPresetFromFile validates YAML and returns string content

PRA-2 Resolve/justify — Source-of-truth review needed: src/lib/actions/sandbox/policy-channel.ts:379

  • Location: not file-specific
  • Category: architecture
  • Problem: The advisor marked localized patch analysis as needs_followup.
  • Impact: A localized workaround can preserve or hide an invalid state when the source boundary is unclear.
  • Recommended action: Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Missing regression test: MISSING — add test for empty preset rejection at load time
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Evidence: PRA-12 from previous review marked needs_followup; policyKeys.size === 0 returns null from gate

PRA-3 Required — Monolith growth: policy-channel.ts exceeds size threshold

  • Location: src/lib/actions/sandbox/policy-channel.ts:1
  • Category: architecture
  • Problem: policy-channel.ts grew from 1544 to 1598 lines (+54 this PR). File handles policy add/remove/list, channel add/remove/start/stop, conflict detection, preset sync, and custom preset agent-gate logic across 8+ exported functions, obscuring security boundaries.
  • Impact: Large mixed-concern file degrades maintainability and auditability; future changes riskier; security boundaries harder to verify.
  • Required action: Extract channel lifecycle (add/remove/start/stop) to src/lib/actions/sandbox/channels.ts, preset sync to policy-sync.ts, keep only policy-add/remove/list in policy-channel.ts. At minimum, offset the 54-line growth by extracting existing logic before merge.
  • Expected follow-up: Fix before merge or get explicit maintainer override.
  • Verification: wc -l src/lib/actions/sandbox/policy-channel.ts shows 1598 lines; grep -c '^export async function' shows 8+ exported functions spanning policy and channel concerns
  • Missing regression test: Architecture test enforcing max file size for policy-channel.ts (e.g., 1200 lines)
  • Done when: The required change is committed and verification passes: wc -l src/lib/actions/sandbox/policy-channel.ts shows 1598 lines; grep -c '^export async function' shows 8+ exported functions spanning policy and channel concerns.
  • Evidence: Diff shows +59 lines in policy-channel.ts; drift context reports delta 54 with severity blocker; previous review PRA-3 unresolved

PRA-4 Resolve/justify — Content-based gate bypass via unknown custom policy keys

  • Location: src/lib/actions/sandbox/policy-channel.ts:385
  • Category: security
  • Problem: The gate in unsupportedMessagingChannelForPresetContent only matches against known policy keys from messaging manifests. A custom preset with an unknown policy key (e.g., 'my_custom_key') pointing to a known messaging host (e.g., 'api.telegram.org') would bypass the agent gate.
  • Impact: Defense-in-depth gap: attacker could craft custom preset with unknown key targeting messaging provider host to widen sandbox egress on terminal-runtime agents.
  • Recommended action: Add secondary host-pattern check: scan preset endpoints against known messaging provider host patterns (api.telegram.org, discord.com, api.slack.com, api.weixin.qq.com, graph.facebook.com, graph.microsoft.com). Document as known limitation if not fixed in this PR.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Create custom preset with policy key 'my_custom_key' and host 'api.telegram.org'; apply to DeepAgents sandbox; observe it bypasses gate
  • Missing regression test: Test: custom preset with unknown policy key pointing to known messaging host is rejected on terminal-runtime agent
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Create custom preset with policy key 'my_custom_key' and host 'api.telegram.org'; apply to DeepAgents sandbox; observe it bypasses gate.
  • Evidence: unsupportedMessagingChannelForPresetContent iterates listMessagingPolicyPresetMetadata() policyKeys; unknown keys not in any manifest pass through unchecked

PRA-5 Resolve/justify — Source-of-truth review: non-string content guard is localized workaround

  • Location: src/lib/actions/sandbox/policy-channel.ts:377
  • Category: architecture
  • Problem: loadPresetFromFile already validates YAML and returns { presetName: string, content: string }. The typeof content !== 'string' guard at line 377 handles an invalid state that should be impossible at the source boundary.
  • Impact: Localized workaround preserves/hides invalid state; if loadPresetFromFile ever returned non-string, gate would silently pass (return null) instead of failing fast.
  • Recommended action: Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition. The guard can be removed when loadPresetFromFile return type is enforced as string at TypeScript level.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Inspect loadPresetFromFile at policy/index.ts:1031 — returns { presetName: string, content: string } after YAML validation
  • Missing regression test: Test: non-string preset content does not bypass agent gate (mock loadPresetFromFile to return non-string content)
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Inspect loadPresetFromFile at policy/index.ts:1031 — returns { presetName: string, content: string } after YAML validation.
  • Evidence: PRA-11 from previous review marked needs_followup; loadPresetFromFile validates YAML and returns string content

PRA-6 Resolve/justify — Source-of-truth review: empty policy keys early return is localized workaround

  • Location: src/lib/actions/sandbox/policy-channel.ts:379
  • Category: architecture
  • Problem: A preset without network_policies keys returns null from the gate, but presets should require network_policies at load time.
  • Impact: Localized workaround preserves invalid state; empty network_policies preset silently passes through gate instead of being rejected at source.
  • Recommended action: Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition. The early return can be removed when preset validation rejects empty network_policies at load time.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Inspect loadPresetFromFile at policy/index.ts:1031 — validates network_policies exists but allows empty object
  • Missing regression test: Test: preset with empty network_policies rejected at load time, not silently passed through gate
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Inspect loadPresetFromFile at policy/index.ts:1031 — validates network_policies exists but allows empty object.
  • Evidence: PRA-12 from previous review marked needs_followup; policyKeys.size === 0 returns null from gate

PRA-7 Resolve/justify — formatAvailableChannelsForAgent returns confusing '(none)' for terminal agents

  • Location: src/lib/actions/sandbox/policy-channel.ts:411
  • Category: correctness
  • Problem: The error message shows 'Channels supported by agent 'langchain-deepagents-code': (none)' which doesn't clarify that the agent has no messaging support at all.
  • Impact: User confusion: '(none)' could mean no channels configured vs no messaging capability.
  • Recommended action: Change return value to '(none — this agent has no messaging support)' or add clarifying note in the error message where this is displayed.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Run policy-add with a messaging preset on DeepAgents sandbox; observe error message shows '(none)'
  • Missing regression test: Test: error message for unsupported channel on terminal agent clarifies no messaging support
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Run policy-add with a messaging preset on DeepAgents sandbox; observe error message shows '(none)'.
  • Evidence: formatAvailableChannelsForAgent returns join(', ') || '(none)'; used in error messages at lines 54, 103

PRA-8 Resolve/justify — Integration test uses slow subprocess spawning duplicating unit test coverage

  • Location: test/policy-add-deepagents-rejection.test.ts:1
  • Category: tests
  • Problem: The behavioral test spawns Node subprocesses (15s timeout each) to test the same gate logic already covered by fast unit tests in policy-add-agent-gate.test.ts.
  • Impact: Adds ~150s to test suite for marginal additional coverage; slows CI feedback loop.
  • Recommended action: Consolidate behavioral coverage into the faster unit test file using mocks, or justify why subprocess spawning is needed for these specific cases.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: npm test -- test/policy-add-deepagents-rejection.test.ts — observe ~150s runtime for 10 tests
  • Missing regression test: N/A — test exists but architecture is inefficient
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: npm test -- test/policy-add-deepagents-rejection.test.ts — observe ~150s runtime for 10 tests.
  • Evidence: 10 tests × ~15s = ~150s runtime for behavioral verification already covered by 6 fast unit tests (<1s total)

PRA-9 Resolve/justify — Missing unit test for non-string content guard in unsupportedMessagingChannelForPresetContent

  • Location: src/lib/actions/sandbox/policy-add-agent-gate.test.ts:1
  • Category: tests
  • Problem: The guard at line 377 (typeof content !== 'string') has no test coverage.
  • Impact: Branch not exercised; if loadPresetFromFile ever returned non-string, behavior unverified.
  • Recommended action: Add unit test: mock loadPresetFromFile to return { presetName: 'test', content: 123 } or null; verify gate behavior (should reject or handle gracefully without crashing).
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Search policy-add-agent-gate.test.ts for 'non-string' or 'typeof content' — no matches
  • Missing regression test: Test: non-string preset content does not bypass agent gate
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Search policy-add-agent-gate.test.ts for 'non-string' or 'typeof content' — no matches.
  • Evidence: Test file covers 6 scenarios but none mock non-string content from loadPresetFromFile

PRA-10 Resolve/justify — Missing test for unknown policy key bypass scenario

  • Location: src/lib/actions/sandbox/policy-add-agent-gate.test.ts:1
  • Category: tests
  • Problem: A custom preset with a policy key not in any manifest (e.g., 'my_custom_key') but pointing to a known messaging host would bypass the content-based gate.
  • Impact: Current bypass behavior untested; if host-pattern check (PRA-5) is implemented, regression undetected.
  • Recommended action: Add test: custom preset with policy key 'my_custom_key' and host 'api.telegram.org' on DeepAgents sandbox; assert current behavior (bypass) to lock in expected behavior, or assert rejection if host-pattern check is implemented.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Search policy-add-agent-gate.test.ts for 'unknown' or 'custom' policy key — no matches
  • Missing regression test: Test: custom preset with unknown policy key pointing to known messaging host is rejected on terminal-runtime agent
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Search policy-add-agent-gate.test.ts for 'unknown' or 'custom' policy key — no matches.
  • Evidence: unsupportedMessagingChannelForPresetContent only matches against known policyKeys from listMessagingPolicyPresetMetadata()

PRA-11 Improvement — Error message for custom presets could clarify content-based match

  • Location: src/lib/actions/sandbox/policy-channel.ts:255
  • Category: correctness
  • Problem: Current message: 'Preset 'X' targets the 'Y' channel' implies the preset declares a channel identity, but it's actually a content-based policy key match.
  • Impact: Minor UX confusion: user may think preset has channel metadata when it only matches policy keys.
  • Suggested action: Clarify error message: 'Preset 'X' contains network policies matching the Y channel (policy key: K), which does not support agent Z' to make the content-based match explicit.
  • Expected follow-up: Prefer a current-PR fix when local to changed code; defer only with rationale or linked follow-up.
  • Verification: Apply custom preset with telegram_bot key on DeepAgents sandbox; observe error says 'targets the telegram channel' rather than 'contains network policies matching the telegram channel'
  • Missing regression test: N/A — UX improvement
  • Done when: The local improvement is applied, or the PR notes why it should be deferred.
  • Evidence: Line 255 in applyExternalPreset constructs error from unsupportedMessagingChannelForPresetContent result

PRA-12 Resolve/justify — Missing security test coverage for content-based gate bypass and non-string content guard

  • Location: src/lib/actions/sandbox/policy-channel.ts:385
  • Category: security
  • Problem: Category 8 (Security Testing): Missing test coverage for content-based gate bypass via unknown custom policy keys and non-string content guard.
  • Impact: Security gaps in gate logic not verified by tests; regressions could be introduced without detection.
  • Recommended action: Add tests for PRA-8 and PRA-9 scenarios to lock in current behavior or verify fix.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Run test suite and verify no test covers unknown policy key bypass or non-string content guard
  • Missing regression test: Tests for unknown policy key bypass and non-string content guard
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Run test suite and verify no test covers unknown policy key bypass or non-string content guard.
  • Evidence: Security category 8 review found missing coverage for PRA-5 bypass and PRA-8 guard

PRA-13 Improvement — resolveAgentForSandbox deduplication completed — PRA-4 resolved

  • Location: src/lib/actions/sandbox/policy-channel.ts:349
  • Category: architecture
  • Problem: Function now exported from version.ts and imported in policy-channel.ts — single source of truth achieved.
  • Impact: Positive: eliminates duplicate function definitions that could diverge.
  • Suggested action: No action needed — verified fixed in this PR.
  • Expected follow-up: Prefer a current-PR fix when local to changed code; defer only with rationale or linked follow-up.
  • Verification: grep -n 'resolveAgentForSandbox' src/lib/actions/sandbox/policy-channel.ts src/lib/sandbox/version.ts shows single definition in version.ts and import in policy-channel.ts
  • Missing regression test: Unit test verifying single source of truth for agent resolution
  • Done when: The local improvement is applied, or the PR notes why it should be deferred.
  • Evidence: PR commit 8c3a9f7 refactor(sandbox): deduplicate resolveAgentForSandbox into version.ts

Workflow run details

This is an automated, non-binding review; it still expects maintainers and agents to respond to each required or warning item. Treat suggestions as current-PR improvements when they touch changed code; defer only with maintainer rationale or a linked follow-up. A human maintainer must make the final merge decision.

@github-actions

github-actions Bot commented Jul 2, 2026

Copy link
Copy Markdown
Contributor

PR Review Advisor — Changes requested

Merge posture: Do not merge yet
Primary next action: Resolve or justify PRA-1: Source-of-truth review needed: Built-in messaging policy preset gate in `addSandboxPolicy`.
Open items: 0 required · 4 warnings · 0 suggestions · 8 test follow-ups
Since last review: 0 prior items resolved · 4 still apply · 0 new items found

Action checklist

  • PRA-1 Resolve or justify: Source-of-truth review needed: Built-in messaging policy preset gate in `addSandboxPolicy`
  • PRA-2 Resolve or justify: Source-of-truth review needed: Custom preset content gate in `applyExternalPreset`
  • PRA-3 Resolve or justify: Drive built-in messaging preset gating from manifest preset metadata in src/lib/actions/sandbox/policy-channel.ts:146
  • PRA-4 Resolve or justify: Define the custom-preset boundary for messaging egress under non-manifest policy keys in src/lib/actions/sandbox/policy-channel.ts:380
  • PRA-T1 Add or justify test follow-up: Runtime validation
  • PRA-T2 Add or justify test follow-up: Runtime validation
  • PRA-T3 Add or justify test follow-up: Runtime validation
  • PRA-T4 Add or justify test follow-up: Runtime validation
  • PRA-T5 Add or justify test follow-up: Runtime validation
  • PRA-T6 Add or justify test follow-up: Acceptance clause
  • PRA-T7 Add or justify test follow-up: Built-in messaging policy preset gate in `addSandboxPolicy`
  • PRA-T8 Add or justify test follow-up: Custom preset content gate in `applyExternalPreset`

Findings index

ID Severity Category Location Required action
PRA-1 Resolve/justify architecture Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
PRA-2 Resolve/justify architecture Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
PRA-3 Resolve/justify security src/lib/actions/sandbox/policy-channel.ts:146 Build the built-in gate from `listMessagingPolicyPresetMetadata()` keyed by `presetName`, resolve the corresponding `channelId` to the manifest, and filter/reject by that manifest. Derive the built-in rejection test catalog from the same metadata where possible, or at minimum add the current `teams` preset to the explicit and interactive built-in tests and docs.
PRA-4 Resolve/justify security src/lib/actions/sandbox/policy-channel.ts:380 Either enforce the custom gate against the same source-of-truth endpoint/host set as the manifest-backed messaging presets, or explicitly document and test that custom presets are arbitrary user-reviewed egress and that only manifest-owned messaging policy keys receive the channel-agent gate.
Review findings by urgency: 0 required fixes, 4 items to resolve/justify, 0 in-scope improvements

⚠️ Resolve or justify before merge

Investigate these in the current review; either fix them, explain why they are not applicable, or document the accepted risk.

PRA-1 Resolve/justify — Source-of-truth review needed: Built-in messaging policy preset gate in `addSandboxPolicy`

  • Location: not file-specific
  • Category: architecture
  • Problem: The advisor marked localized patch analysis as needs_followup.
  • Impact: A localized workaround can preserve or hide an invalid state when the source boundary is unclear.
  • Recommended action: Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Missing regression test: Partial: tests cover the five issue-listed built-in names, but omit current built-in `teams` and do not prove a manifest preset whose `presetName` differs from `channelId` is gated.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Evidence: Production resolves built-in channel ownership with `resolveChannelManifest(preset.name)` / `resolveChannelManifest(normalized)`, while `listMessagingPolicyPresetMetadata()` exposes separate `presetName` and `channelId` fields.

PRA-2 Resolve/justify — Source-of-truth review needed: Custom preset content gate in `applyExternalPreset`

  • Location: not file-specific
  • Category: architecture
  • Problem: The advisor marked localized patch analysis as needs_followup.
  • Impact: A localized workaround can preserve or hide an invalid state when the source boundary is unclear.
  • Recommended action: Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Missing regression test: Partial: tests cover manifest-owned policy keys including `teams`, but do not cover a messaging service host under an arbitrary policy key.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Evidence: `unsupportedMessagingChannelForPresetContent()` compares parsed `network_policies` keys against manifest metadata and returns `null` for unknown keys.

PRA-3 Resolve/justify — Drive built-in messaging preset gating from manifest preset metadata

  • Location: src/lib/actions/sandbox/policy-channel.ts:146
  • Category: security
  • Problem: The built-in `policy-add` path still decides whether a preset is a messaging channel by calling `resolveChannelManifest(preset.name)` and `resolveChannelManifest(normalized)`. That assumes the policy preset name is the same as the channel id, even though `listMessagingPolicyPresetMetadata()` exposes `presetName` and `channelId` separately. The changed built-in tests and docs also enumerate only `telegram`, `discord`, `slack`, `wechat`, and `whatsapp`, while the current codebase has a manifest-backed `teams` channel and `teams.yaml` preset.
  • Impact: A current or future manifest-backed messaging preset can be missed by the DeepAgents gate and offered or applied as generic egress to a terminal-runtime agent with no inbound messaging gateway. That widens sandbox network policy for an unsupported agent and leaves the tests blind to manifest/catalog drift.
  • Recommended action: Build the built-in gate from `listMessagingPolicyPresetMetadata()` keyed by `presetName`, resolve the corresponding `channelId` to the manifest, and filter/reject by that manifest. Derive the built-in rejection test catalog from the same metadata where possible, or at minimum add the current `teams` preset to the explicit and interactive built-in tests and docs.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Read `src/lib/actions/sandbox/policy-channel.ts` around the `resolveChannelManifest(preset.name)` and `resolveChannelManifest(normalized)` calls, then compare with `src/lib/messaging/channels/metadata.ts:listMessagingPolicyPresetMetadata`, `src/lib/messaging/channels/teams/manifest.ts`, and `nemoclaw-blueprint/policies/presets/teams.yaml`.
  • Missing regression test: Add a test proving `addSandboxPolicy('da-test', { preset: 'teams', yes: true })` for `langchain-deepagents-code` exits with code 1 before `loadPreset`, endpoint disclosure, prompt, or `applyPreset`; also add a manifest-metadata test with a preset whose `presetName` differs from `channelId` to prove the gate follows `presetName -> channelId` rather than channel-id equality.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Read `src/lib/actions/sandbox/policy-channel.ts` around the `resolveChannelManifest(preset.name)` and `resolveChannelManifest(normalized)` calls, then compare with `src/lib/messaging/channels/metadata.ts:listMessagingPolicyPresetMetadata`, `src/lib/messaging/channels/teams/manifest.ts`, and `nemoclaw-blueprint/policies/presets/teams.yaml`.
  • Evidence: `listMessagingPolicyPresetMetadata()` returns separate `channelId` and `presetName` fields. Production uses `resolveChannelManifest(preset.name)` at `policy-channel.ts:146` and `resolveChannelManifest(normalized)` at `policy-channel.ts:155`. The changed built-in test arrays define `MESSAGING_CHANNELS = ["telegram", "discord", "slack", "wechat", "whatsapp"]` and mocked `PRESETS` without `teams`, while the repository contains `teamsManifest` with `policyPresets: [{ name: "teams", policyKeys: ["teams"] }]` and a built-in `teams.yaml` preset.

PRA-4 Resolve/justify — Define the custom-preset boundary for messaging egress under non-manifest policy keys

  • Location: src/lib/actions/sandbox/policy-channel.ts:380
  • Category: security
  • Problem: The new `--from-file` / `--from-dir` gate detects custom messaging intent by comparing parsed `network_policies` keys against manifest-declared policy keys and agent policy keys. A custom preset can still target the same messaging service hosts under an arbitrary policy key and be treated as generic custom egress. That may be the intended trust boundary for user-reviewed custom presets, but the changed docs/tests do not make the limitation explicit.
  • Impact: Operators and future maintainers may believe all messaging egress is blocked on terminal-runtime agents, while equivalent custom egress to messaging services can still be applied when the policy key is not manifest-owned. This creates a policy-bypass or false-assurance risk around sandbox egress controls.
  • Recommended action: Either enforce the custom gate against the same source-of-truth endpoint/host set as the manifest-backed messaging presets, or explicitly document and test that custom presets are arbitrary user-reviewed egress and that only manifest-owned messaging policy keys receive the channel-agent gate.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Inspect `unsupportedMessagingChannelForPresetContent()` and `policies.parsePresetPolicyKeys()`, then compare a custom preset using `network_policies: { teams: { endpoints: [{ host: graph.microsoft.com }] } }` with one using `network_policies: { my_teams_api: { endpoints: [{ host: graph.microsoft.com }] } }`.
  • Missing regression test: Add a `--from-file` behavior test with a messaging service host under a non-manifest policy key, asserting the chosen contract: either it is rejected before endpoint disclosure, prompt, and `applyPresetContent`, or it is allowed with docs stating that arbitrary custom egress remains outside the channel ownership gate.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Inspect `unsupportedMessagingChannelForPresetContent()` and `policies.parsePresetPolicyKeys()`, then compare a custom preset using `network_policies: { teams: { endpoints: [{ host: graph.microsoft.com }] } }` with one using `network_policies: { my_teams_api: { endpoints: [{ host: graph.microsoft.com }] } }`.
  • Evidence: `unsupportedMessagingChannelForPresetContent()` builds a set from `policies.parsePresetPolicyKeys(content)` and compares only those keys to `preset.policyKeys` and `preset.agentPolicyKeys`; it returns `null` for unknown keys. Changed tests cover manifest-owned keys including `teams`, but do not cover equivalent messaging hosts under arbitrary keys.

💡 In-scope improvements

These are lower-risk, not throwaway. Prefer fixing them in this PR when they are local to changed code; defer only with rationale or a linked follow-up.

  • None.
Test follow-ups to resolve or justify

If these cover changed behavior, prefer adding them in this PR; otherwise state why existing coverage is enough or link the follow-up.

  • PRA-T1 Runtime validation — DeepAgents `policy-add teams --yes` exits 1 before `loadPreset`, endpoint disclosure, prompt, or `applyPreset`.. The changed path controls sandbox network-policy widening and terminal-runtime rejection. Unit and spawned-process tests cover much of the behavior, but the source-of-truth gaps need targeted behavioral coverage; a real CLI/OpenShell smoke path would add confidence without relying on external E2E status.
  • PRA-T2 Runtime validation — Built-in gate maps a manifest `presetName` that differs from `channelId` through `listMessagingPolicyPresetMetadata()` and rejects it on DeepAgents.. The changed path controls sandbox network-policy widening and terminal-runtime rejection. Unit and spawned-process tests cover much of the behavior, but the source-of-truth gaps need targeted behavioral coverage; a real CLI/OpenShell smoke path would add confidence without relying on external E2E status.
  • PRA-T3 Runtime validation — Interactive picker omits every current `listMessagingPolicyPresetMetadata().presetName` unsupported by the sandbox agent, including `teams`.. The changed path controls sandbox network-policy widening and terminal-runtime rejection. Unit and spawned-process tests cover much of the behavior, but the source-of-truth gaps need targeted behavioral coverage; a real CLI/OpenShell smoke path would add confidence without relying on external E2E status.
  • PRA-T4 Runtime validation — Custom preset with a messaging-service host under a non-manifest policy key follows the documented contract: either reject before disclosure/prompt/apply or allow as arbitrary custom egress with docs stating that limitation.. The changed path controls sandbox network-policy widening and terminal-runtime rejection. Unit and spawned-process tests cover much of the behavior, but the source-of-truth gaps need targeted behavioral coverage; a real CLI/OpenShell smoke path would add confidence without relying on external E2E status.
  • PRA-T5 Runtime validation — `policy-list` or applied-preset state remains unchanged after a rejected DeepAgents messaging preset.. The changed path controls sandbox network-policy widening and terminal-runtime rejection. Unit and spawned-process tests cover much of the behavior, but the source-of-truth gaps need targeted behavioral coverage; a real CLI/OpenShell smoke path would add confidence without relying on external E2E status.
  • PRA-T6 Acceptance clause — Also observe `nemoclaw dcode-v072 channels list` before/after for consistency (it correctly reports "no supported channels", confirming DA has no messaging surface). — add test evidence or identify existing coverage. The production gate uses the same manifest availability helpers as channel listing (`availableManifestChannelsForAgent` / `formatAvailableChannelsForAgent`) and tests assert the error reports no supported channels for `langchain-deepagents-code`; no changed test directly invokes `channels list` before/after.
  • PRA-T7 Built-in messaging policy preset gate in `addSandboxPolicy` — Partial: tests cover the five issue-listed built-in names, but omit current built-in `teams` and do not prove a manifest preset whose `presetName` differs from `channelId` is gated.. Production resolves built-in channel ownership with `resolveChannelManifest(preset.name)` / `resolveChannelManifest(normalized)`, while `listMessagingPolicyPresetMetadata()` exposes separate `presetName` and `channelId` fields.
  • PRA-T8 Custom preset content gate in `applyExternalPreset` — Partial: tests cover manifest-owned policy keys including `teams`, but do not cover a messaging service host under an arbitrary policy key.. `unsupportedMessagingChannelForPresetContent()` compares parsed `network_policies` keys against manifest metadata and returns `null` for unknown keys.
Since last review details

Current findings, using the urgency labels above:

PRA-1 Resolve/justify — Source-of-truth review needed: Built-in messaging policy preset gate in `addSandboxPolicy`

  • Location: not file-specific
  • Category: architecture
  • Problem: The advisor marked localized patch analysis as needs_followup.
  • Impact: A localized workaround can preserve or hide an invalid state when the source boundary is unclear.
  • Recommended action: Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Missing regression test: Partial: tests cover the five issue-listed built-in names, but omit current built-in `teams` and do not prove a manifest preset whose `presetName` differs from `channelId` is gated.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Evidence: Production resolves built-in channel ownership with `resolveChannelManifest(preset.name)` / `resolveChannelManifest(normalized)`, while `listMessagingPolicyPresetMetadata()` exposes separate `presetName` and `channelId` fields.

PRA-2 Resolve/justify — Source-of-truth review needed: Custom preset content gate in `applyExternalPreset`

  • Location: not file-specific
  • Category: architecture
  • Problem: The advisor marked localized patch analysis as needs_followup.
  • Impact: A localized workaround can preserve or hide an invalid state when the source boundary is unclear.
  • Recommended action: Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Missing regression test: Partial: tests cover manifest-owned policy keys including `teams`, but do not cover a messaging service host under an arbitrary policy key.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Evidence: `unsupportedMessagingChannelForPresetContent()` compares parsed `network_policies` keys against manifest metadata and returns `null` for unknown keys.

PRA-3 Resolve/justify — Drive built-in messaging preset gating from manifest preset metadata

  • Location: src/lib/actions/sandbox/policy-channel.ts:146
  • Category: security
  • Problem: The built-in `policy-add` path still decides whether a preset is a messaging channel by calling `resolveChannelManifest(preset.name)` and `resolveChannelManifest(normalized)`. That assumes the policy preset name is the same as the channel id, even though `listMessagingPolicyPresetMetadata()` exposes `presetName` and `channelId` separately. The changed built-in tests and docs also enumerate only `telegram`, `discord`, `slack`, `wechat`, and `whatsapp`, while the current codebase has a manifest-backed `teams` channel and `teams.yaml` preset.
  • Impact: A current or future manifest-backed messaging preset can be missed by the DeepAgents gate and offered or applied as generic egress to a terminal-runtime agent with no inbound messaging gateway. That widens sandbox network policy for an unsupported agent and leaves the tests blind to manifest/catalog drift.
  • Recommended action: Build the built-in gate from `listMessagingPolicyPresetMetadata()` keyed by `presetName`, resolve the corresponding `channelId` to the manifest, and filter/reject by that manifest. Derive the built-in rejection test catalog from the same metadata where possible, or at minimum add the current `teams` preset to the explicit and interactive built-in tests and docs.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Read `src/lib/actions/sandbox/policy-channel.ts` around the `resolveChannelManifest(preset.name)` and `resolveChannelManifest(normalized)` calls, then compare with `src/lib/messaging/channels/metadata.ts:listMessagingPolicyPresetMetadata`, `src/lib/messaging/channels/teams/manifest.ts`, and `nemoclaw-blueprint/policies/presets/teams.yaml`.
  • Missing regression test: Add a test proving `addSandboxPolicy('da-test', { preset: 'teams', yes: true })` for `langchain-deepagents-code` exits with code 1 before `loadPreset`, endpoint disclosure, prompt, or `applyPreset`; also add a manifest-metadata test with a preset whose `presetName` differs from `channelId` to prove the gate follows `presetName -> channelId` rather than channel-id equality.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Read `src/lib/actions/sandbox/policy-channel.ts` around the `resolveChannelManifest(preset.name)` and `resolveChannelManifest(normalized)` calls, then compare with `src/lib/messaging/channels/metadata.ts:listMessagingPolicyPresetMetadata`, `src/lib/messaging/channels/teams/manifest.ts`, and `nemoclaw-blueprint/policies/presets/teams.yaml`.
  • Evidence: `listMessagingPolicyPresetMetadata()` returns separate `channelId` and `presetName` fields. Production uses `resolveChannelManifest(preset.name)` at `policy-channel.ts:146` and `resolveChannelManifest(normalized)` at `policy-channel.ts:155`. The changed built-in test arrays define `MESSAGING_CHANNELS = ["telegram", "discord", "slack", "wechat", "whatsapp"]` and mocked `PRESETS` without `teams`, while the repository contains `teamsManifest` with `policyPresets: [{ name: "teams", policyKeys: ["teams"] }]` and a built-in `teams.yaml` preset.

PRA-4 Resolve/justify — Define the custom-preset boundary for messaging egress under non-manifest policy keys

  • Location: src/lib/actions/sandbox/policy-channel.ts:380
  • Category: security
  • Problem: The new `--from-file` / `--from-dir` gate detects custom messaging intent by comparing parsed `network_policies` keys against manifest-declared policy keys and agent policy keys. A custom preset can still target the same messaging service hosts under an arbitrary policy key and be treated as generic custom egress. That may be the intended trust boundary for user-reviewed custom presets, but the changed docs/tests do not make the limitation explicit.
  • Impact: Operators and future maintainers may believe all messaging egress is blocked on terminal-runtime agents, while equivalent custom egress to messaging services can still be applied when the policy key is not manifest-owned. This creates a policy-bypass or false-assurance risk around sandbox egress controls.
  • Recommended action: Either enforce the custom gate against the same source-of-truth endpoint/host set as the manifest-backed messaging presets, or explicitly document and test that custom presets are arbitrary user-reviewed egress and that only manifest-owned messaging policy keys receive the channel-agent gate.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Inspect `unsupportedMessagingChannelForPresetContent()` and `policies.parsePresetPolicyKeys()`, then compare a custom preset using `network_policies: { teams: { endpoints: [{ host: graph.microsoft.com }] } }` with one using `network_policies: { my_teams_api: { endpoints: [{ host: graph.microsoft.com }] } }`.
  • Missing regression test: Add a `--from-file` behavior test with a messaging service host under a non-manifest policy key, asserting the chosen contract: either it is rejected before endpoint disclosure, prompt, and `applyPresetContent`, or it is allowed with docs stating that arbitrary custom egress remains outside the channel ownership gate.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Inspect `unsupportedMessagingChannelForPresetContent()` and `policies.parsePresetPolicyKeys()`, then compare a custom preset using `network_policies: { teams: { endpoints: [{ host: graph.microsoft.com }] } }` with one using `network_policies: { my_teams_api: { endpoints: [{ host: graph.microsoft.com }] } }`.
  • Evidence: `unsupportedMessagingChannelForPresetContent()` builds a set from `policies.parsePresetPolicyKeys(content)` and compares only those keys to `preset.policyKeys` and `preset.agentPolicyKeys`; it returns `null` for unknown keys. Changed tests cover manifest-owned keys including `teams`, but do not cover equivalent messaging hosts under arbitrary keys.

Workflow run details

This is an automated, non-binding review; it still expects maintainers and agents to respond to each required or warning item. Treat suggestions as current-PR improvements when they touch changed code; defer only with maintainer rationale or a linked follow-up. A human maintainer must make the final merge decision.

Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
@coderabbitai

coderabbitai Bot commented Jul 2, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • ✅ Review completed - (🔄 Check again to review again)

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 20c83cd2-58cc-4d9b-9553-22a8c86d2711

📥 Commits

Reviewing files that changed from the base of the PR and between 51dd30d and 8c3a9f7.

📒 Files selected for processing (2)
  • src/lib/actions/sandbox/policy-channel.ts
  • src/lib/sandbox/version.ts
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/lib/actions/sandbox/policy-channel.ts

📝 Walkthrough

Walkthrough

Adds agent compatibility rejection for messaging presets in policy-add, covers direct and subprocess paths, and updates the reference docs.

Changes

policy-add channel-agent gate

Layer / File(s) Summary
Channel-agent compatibility check in addSandboxPolicy
src/lib/actions/sandbox/policy-channel.ts, src/lib/sandbox/version.ts
addSandboxPolicy resolves the sandbox agent, filters built-in messaging presets by agent support, rejects incompatible explicit presets before confirmation, and rejects unsupported messaging content loaded from files before endpoint preview.
Unit coverage for preset gating
src/lib/actions/sandbox/policy-add-agent-gate.test.ts
The sandbox policy-add test suite stubs agent and policy services, checks rejection of unsupported messaging presets and custom presets, and verifies compatible presets still apply.
Subprocess rejection test for Deep Agents
test/policy-add-deepagents-rejection.test.ts
The subprocess-based test harness generates isolated Node scripts, captures exit/log/prompt/policy-call results, and asserts langchain-deepagents-code rejects messaging presets from both direct channel and --from-file paths.
Reference documentation updates
docs/reference/commands.mdx, docs/reference/commands-nemohermes.mdx
commands.mdx and commands-nemohermes.mdx now state that messaging presets only apply to supported agents and are rejected early for terminal-runtime agents.

Estimated code review effort: 4 (Complex) | ~45 minutes

Suggested reviewers: jyaunches, ericksoa

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the main behavior change: rejecting messaging channel presets on terminal-runtime agents.
Linked Issues check ✅ Passed The code and tests match the issue: all five messaging presets are rejected early on Deep Agents terminal-runtime sandboxes with a non-zero exit.
Out of Scope Changes check ✅ Passed The docs, tests, and sandbox-agent refactor all support the policy-add rejection fix and do not appear unrelated.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/dcode-policy-add-channel-gate

Comment @coderabbitai help to get the list of available commands.

@github-actions

github-actions Bot commented Jul 2, 2026

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 `@docs/reference/commands-nemohermes.mdx`:
- Around line 824-825: The rejection flow described here matches the behavior of
channels add, not channels list. Update the sentence in the documentation near
the policy-add/channel preset description to reference channels add as the
command that performs the supported-agent check and clear rejection before any
endpoint disclosure or prompt.
🪄 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: ca002f6c-a4bd-4bef-82b2-e79c543281e1

📥 Commits

Reviewing files that changed from the base of the PR and between 9fe4536 and 19d7ba2.

📒 Files selected for processing (5)
  • docs/reference/commands-nemohermes.mdx
  • docs/reference/commands.mdx
  • src/lib/actions/sandbox/policy-add-agent-gate.test.ts
  • src/lib/actions/sandbox/policy-channel.ts
  • test/policy-add-deepagents-rejection.test.ts

Comment thread docs/reference/commands-nemohermes.mdx Outdated

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (3)
src/lib/actions/sandbox/policy-channel.ts (1)

373-395: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoff

Misplaced layer: messaging-channel classification helper lives in the actions file.

unsupportedMessagingChannelForPresetContent is pure decision/classification logic (parses preset content, matches against messaging preset metadata, returns a manifest) with no host/process/network calls. Per the src/lib layering guidance, this should live in src/lib/messaging/** as a messaging-channel/policy classification helper, alongside listMessagingPolicyPresetMetadata and isMessagingChannelSupportedByAgent, rather than in src/lib/actions/sandbox/policy-channel.ts. Keeping it here duplicates the classification concern that's supposed to be centralized in the messaging module and blurs the actions/domain boundary.

As per path instructions, "actions orchestrate, domain modules make pure decisions... For this PR's area, place/extend logic in the existing transitional clusters: ... src/lib/messaging/** for messaging-channel/policy classification helpers."

🤖 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 `@src/lib/actions/sandbox/policy-channel.ts` around lines 373 - 395, The helper
unsupportedMessagingChannelForPresetContent is pure messaging-channel
classification logic, but it currently lives in the sandbox actions layer. Move
this decision code into src/lib/messaging/** alongside
listMessagingPolicyPresetMetadata and isMessagingChannelSupportedByAgent, then
update the actions file to call the centralized helper instead of owning the
parsing/matching logic. Keep the behavior the same: parse preset policy keys,
match against messaging preset metadata, resolve the manifest, and apply the
agent-support gate from the messaging module.

Source: Path instructions

src/lib/actions/sandbox/policy-add-agent-gate.test.ts (1)

32-39: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Duplicate MESSAGING_POLICY_KEYS fixture across two test files.

The same tuple array (telegram_bot/discord/slack/wechat_bridge/whatsapp/teams with hosts) is redefined identically in test/policy-add-deepagents-rejection.test.ts. Extracting it to a small shared test fixture would prevent the two suites from drifting apart if a policy key/host is added or renamed.

🤖 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 `@src/lib/actions/sandbox/policy-add-agent-gate.test.ts` around lines 32 - 39,
The MESSAGING_POLICY_KEYS tuple is duplicated in this test suite and the
deepagents rejection test, so the fixture can drift if policy names or hosts
change. Extract the shared array into a common test fixture/helper and import it
in both policy-add-agent-gate.test.ts and
policy-add-deepagents-rejection.test.ts, keeping the existing tuple contents
centralized under the MESSAGING_POLICY_KEYS symbol.
test/policy-add-deepagents-rejection.test.ts (1)

139-146: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Duplicate MESSAGING_POLICY_KEYS fixture (see companion file).

Identical to the array added in src/lib/actions/sandbox/policy-add-agent-gate.test.ts (Lines 32-39). Consider sharing this fixture between the two suites.

Also applies to: 184-207

🤖 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/policy-add-deepagents-rejection.test.ts` around lines 139 - 146, The
MESSAGING_POLICY_KEYS test fixture is duplicated between this suite and the
companion sandbox policy test, so it should be shared instead of maintained in
two places. Move the array to a common test helper or shared constant module and
update the affected tests to import and reuse it from both suites, keeping the
existing references in this file and in policy-add-agent-gate.test in sync.
🤖 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 `@src/lib/actions/sandbox/policy-add-agent-gate.test.ts`:
- Around line 32-39: The MESSAGING_POLICY_KEYS tuple is duplicated in this test
suite and the deepagents rejection test, so the fixture can drift if policy
names or hosts change. Extract the shared array into a common test
fixture/helper and import it in both policy-add-agent-gate.test.ts and
policy-add-deepagents-rejection.test.ts, keeping the existing tuple contents
centralized under the MESSAGING_POLICY_KEYS symbol.

In `@src/lib/actions/sandbox/policy-channel.ts`:
- Around line 373-395: The helper unsupportedMessagingChannelForPresetContent is
pure messaging-channel classification logic, but it currently lives in the
sandbox actions layer. Move this decision code into src/lib/messaging/**
alongside listMessagingPolicyPresetMetadata and
isMessagingChannelSupportedByAgent, then update the actions file to call the
centralized helper instead of owning the parsing/matching logic. Keep the
behavior the same: parse preset policy keys, match against messaging preset
metadata, resolve the manifest, and apply the agent-support gate from the
messaging module.

In `@test/policy-add-deepagents-rejection.test.ts`:
- Around line 139-146: The MESSAGING_POLICY_KEYS test fixture is duplicated
between this suite and the companion sandbox policy test, so it should be shared
instead of maintained in two places. Move the array to a common test helper or
shared constant module and update the affected tests to import and reuse it from
both suites, keeping the existing references in this file and in
policy-add-agent-gate.test in sync.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 2af0cdd6-4ea3-4430-9bf7-090231697cae

📥 Commits

Reviewing files that changed from the base of the PR and between bfa239e and f906d85.

📒 Files selected for processing (3)
  • src/lib/actions/sandbox/policy-add-agent-gate.test.ts
  • src/lib/actions/sandbox/policy-channel.ts
  • test/policy-add-deepagents-rejection.test.ts

Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
@wscurran
wscurran requested a review from cv July 2, 2026 16:40
@wscurran wscurran added v0.0.73 and removed v0.0.73 labels Jul 2, 2026
@cv
cv merged commit 3a51447 into main Jul 2, 2026
51 checks passed
@cv
cv deleted the fix/dcode-policy-add-channel-gate branch July 2, 2026 16:58
sandl99 added a commit that referenced this pull request Jul 2, 2026
…time agents (#6197)"

This reverts commit 3a51447.

Signed-off-by: San Dang <sdang@nvidia.com>
@cv cv mentioned this pull request Jul 2, 2026
21 tasks
cv added a commit that referenced this pull request Jul 2, 2026
<!-- markdownlint-disable MD041 -->
## Summary
This PR prepares the user-facing documentation for v0.0.73 before the
release plan is frozen.
It adds release notes for the merged runtime changes and closes
documentation gaps around DNS-backed HTTPS endpoint validation and
LangChain Deep Agents Code proxy recovery.

## Changes
- Add the `v0.0.73` release-note section with links to the detailed
command, inference, recovery, lifecycle, platform, and setup
documentation.
- Correct the custom endpoint guidance so DNS-backed HTTPS rejection and
the supported alternatives match the fail-closed runtime behavior.
- Document the managed `inference.local` proxy boundary and rebuild
requirement for existing LangChain Deep Agents Code sandboxes.
- Add troubleshooting guidance for the DNS-backed HTTPS validation
error.
- Validate with `npm run docs:sync-agent-variants` and `npm run docs`;
Fern completed with 0 errors and 2 existing warnings.
- Source summary:
- [#6139](#6139) ->
`docs/about/release-notes.mdx`, `docs/inference/inference-options.mdx`,
`docs/reference/commands.mdx`, `docs/reference/commands-nemohermes.mdx`,
and `docs/reference/troubleshooting.mdx`: Document fail-closed
DNS-backed HTTPS endpoint handling and recovery options.
- [#6142](#6142) ->
`docs/about/release-notes.mdx`: Summarize native OpenShell GPU injection
and compatibility-path diagnostics.
- [#6197](#6197) ->
`docs/about/release-notes.mdx`: Summarize agent-aware messaging preset
rejection.
- [#6199](#6199) ->
`docs/about/release-notes.mdx`: Summarize the unreachable-sandbox backup
opt-in, restore behavior, and data-loss boundary.
- [#6204](#6204) and
[#6206](#6206) ->
`docs/about/release-notes.mdx` and
`docs/get-started/quickstart-langchain-deepagents-code.mdx`: Document
the corrected managed proxy contract and required sandbox rebuild.
- [#6213](#6213) ->
`docs/about/release-notes.mdx`: Summarize the merged setup, recovery,
and host-state documentation.

## Type of Change

- [ ] Code change (feature, bug fix, or refactor)
- [ ] Code change with doc updates
- [x] Doc only (prose changes, no code sample modifications)
- [ ] Doc only (includes code sample changes)

## Quality Gates
<!-- Check all that apply. For any "covered by existing tests", "not
applicable", or waiver entry, add a brief justification on the same line
or in the Changes section. -->
- [ ] Tests added or updated for changed behavior
- [ ] Existing tests cover changed behavior — justification:
- [x] Tests not applicable — justification: documentation-only release
preparation; the Fern docs build validates the changed pages and routes.
- [x] Docs updated for user-facing behavior changes
- [ ] Docs not applicable — justification:
- [ ] Sensitive paths changed (security, policy, credentials, preflight,
onboarding, inference, runner, sandbox, or messaging)
- [ ] Sensitive-path review completed or maintainer-approved waiver
recorded — reviewer/approval link/justification:
- [ ] Non-success, skipped, or missing CI check accepted by maintainer —
check name, approval link, and follow-up issue:

## Verification
<!-- Check each item you ran and confirmed. Leave unchecked items you
skipped. Doc-only changes do not require npm test unless you ran it. -->
- [x] PR description includes the DCO sign-off declaration and every
commit appears as `Verified` in GitHub
- [x] Git hooks passed during commit and push, or `npx prek run
--from-ref main --to-ref HEAD` passes
- [ ] Targeted tests pass for changed behavior
- [ ] Full `npm test` passes (broad runtime changes only)
- [x] Quality Gates section completed with required justifications or
waivers
- [x] No secrets, API keys, or credentials committed
- [ ] `npm run docs` builds without warnings (doc changes only)
- [x] Doc pages follow the [style
guide](https://github.com/NVIDIA/NemoClaw/blob/main/docs/CONTRIBUTING.md)
(doc changes only)
- [ ] New doc pages include SPDX header and frontmatter (new pages only)

---
<!-- DCO sign-off is required in this PR description, and every commit
must appear as Verified in GitHub. Run: git config user.name && git
config user.email -->
Signed-off-by: Carlos Villela <cvillela@nvidia.com>


<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit

* **Documentation**
* Added a new **v0.0.73** release notes section with six highlights at
the top of the changelog.
* Expanded **Custom Endpoint URL Validation** guidance in inference
option docs, including explicit acceptance/rejection rules for HTTP vs
DNS-backed HTTPS and how validated IPs are stored.
* Updated command references (`nemohermes inference set`, `$$nemoclaw
inference set`) to match the new validation behavior.
* Added troubleshooting documentation for unsupported **DNS-backed HTTPS
endpoints**, plus clarified Deep Agents Code routing and post-upgrade
sandbox rebuild guidance.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->

---------

Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Hadar301 pushed a commit to Hadar301/NemoClaw-OpenShift that referenced this pull request Jul 12, 2026
…nts (NVIDIA#6197)

## Summary

`policy-add` accepted the five messaging channel presets (`telegram`,
`discord`, `slack`, `wechat`, `whatsapp`) on terminal-runtime DeepAgents
sandboxes, which have no inbound messaging gateway. It now rejects an
unsupported channel preset up front, mirroring the existing `channels
add` agent gate.

## Related Issue

Fixes NVIDIA#6185

## Changes

- `addSandboxPolicy` resolves the sandbox agent and rejects a messaging
channel preset the agent's channel manifests do not support, before any
endpoint disclosure or confirmation prompt, exiting non-zero.
- Unsupported channel presets are dropped from the interactive picker
and the valid-presets list.
- Reuses the same message and `supportedAgents` source as `channels
add`; non-messaging presets and messaging-capable agents (openclaw,
hermes) are unaffected.
- Docs: note the terminal-runtime rejection in the `policy-add`
reference.
- Tests: action-boundary unit coverage for all five channels, plus a
spawned-process behaviour test asserting non-zero exit, no endpoint
disclosure, and no prompt.

## Type of Change

- [ ] Code change (feature, bug fix, or refactor)
- [x] Code change with doc updates
- [ ] Doc only (prose changes, no code sample modifications)
- [ ] Doc only (includes code sample changes)

## Quality Gates
<!-- Check all that apply. For any "covered by existing tests", "not
applicable", or waiver entry, add a brief justification on the same line
or in the Changes section. -->
- [x] Tests added or updated for changed behavior
- [ ] Existing tests cover changed behavior — justification:
- [ ] Tests not applicable — justification:
- [x] Docs updated for user-facing behavior changes
- [ ] Docs not applicable — justification:
- [x] Sensitive paths changed (security, policy, credentials, preflight,
onboarding, inference, runner, sandbox, or messaging)
- [ ] Sensitive-path review completed or maintainer-approved waiver
recorded — reviewer/approval link/justification:
- [ ] Non-success, skipped, or missing CI check accepted by maintainer —
check name, approval link, and follow-up issue:

## Verification
<!-- Check each item you ran and confirmed. Leave unchecked items you
skipped. Doc-only changes do not require npm test unless you ran it. -->
- [x] PR description includes the DCO sign-off declaration and every
commit appears as `Verified` in GitHub
- [x] Git hooks passed during commit and push, or `npx prek run
--from-ref main --to-ref HEAD` passes
- [x] Targeted tests pass for changed behavior
- [ ] Full `npm test` passes (broad runtime changes only)
- [x] Quality Gates section completed with required justifications or
waivers
- [x] No secrets, API keys, or credentials committed
- [ ] `npm run docs` builds without warnings (doc changes only)
- [ ] Doc pages follow the [style
guide](https://github.com/NVIDIA/NemoClaw/blob/main/docs/CONTRIBUTING.md)
(doc changes only)
- [ ] New doc pages include SPDX header and frontmatter (new pages only)

---
Signed-off-by: Tinson Lai <tinsonl@nvidia.com>


<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit

* **Bug Fixes**
* Messaging-channel presets (Telegram/Discord/Slack/WeChat/WhatsApp) are
now rejected when an agent doesn’t support the channel, with a clear
error before any confirmation or endpoint disclosure.
* `--from-file/--from-dir` custom presets containing messaging policy
keys are refused early for unsupported terminal-runtime agents;
supported agents still apply them.
* **Documentation**
* Updated `policy-add` reference docs to clarify messaging-preset
applicability and the early-rejection behavior for terminal-runtime
agents.
* **Tests**
* Added/extended tests to verify early rejection, no prompting, and no
preset loading/applying.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->

---------

Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
Hadar301 pushed a commit to Hadar301/NemoClaw-OpenShift that referenced this pull request Jul 12, 2026
<!-- markdownlint-disable MD041 -->
## Summary
This PR prepares the user-facing documentation for v0.0.73 before the
release plan is frozen.
It adds release notes for the merged runtime changes and closes
documentation gaps around DNS-backed HTTPS endpoint validation and
LangChain Deep Agents Code proxy recovery.

## Changes
- Add the `v0.0.73` release-note section with links to the detailed
command, inference, recovery, lifecycle, platform, and setup
documentation.
- Correct the custom endpoint guidance so DNS-backed HTTPS rejection and
the supported alternatives match the fail-closed runtime behavior.
- Document the managed `inference.local` proxy boundary and rebuild
requirement for existing LangChain Deep Agents Code sandboxes.
- Add troubleshooting guidance for the DNS-backed HTTPS validation
error.
- Validate with `npm run docs:sync-agent-variants` and `npm run docs`;
Fern completed with 0 errors and 2 existing warnings.
- Source summary:
- [NVIDIA#6139](NVIDIA#6139) ->
`docs/about/release-notes.mdx`, `docs/inference/inference-options.mdx`,
`docs/reference/commands.mdx`, `docs/reference/commands-nemohermes.mdx`,
and `docs/reference/troubleshooting.mdx`: Document fail-closed
DNS-backed HTTPS endpoint handling and recovery options.
- [NVIDIA#6142](NVIDIA#6142) ->
`docs/about/release-notes.mdx`: Summarize native OpenShell GPU injection
and compatibility-path diagnostics.
- [NVIDIA#6197](NVIDIA#6197) ->
`docs/about/release-notes.mdx`: Summarize agent-aware messaging preset
rejection.
- [NVIDIA#6199](NVIDIA#6199) ->
`docs/about/release-notes.mdx`: Summarize the unreachable-sandbox backup
opt-in, restore behavior, and data-loss boundary.
- [NVIDIA#6204](NVIDIA#6204) and
[NVIDIA#6206](NVIDIA#6206) ->
`docs/about/release-notes.mdx` and
`docs/get-started/quickstart-langchain-deepagents-code.mdx`: Document
the corrected managed proxy contract and required sandbox rebuild.
- [NVIDIA#6213](NVIDIA#6213) ->
`docs/about/release-notes.mdx`: Summarize the merged setup, recovery,
and host-state documentation.

## Type of Change

- [ ] Code change (feature, bug fix, or refactor)
- [ ] Code change with doc updates
- [x] Doc only (prose changes, no code sample modifications)
- [ ] Doc only (includes code sample changes)

## Quality Gates
<!-- Check all that apply. For any "covered by existing tests", "not
applicable", or waiver entry, add a brief justification on the same line
or in the Changes section. -->
- [ ] Tests added or updated for changed behavior
- [ ] Existing tests cover changed behavior — justification:
- [x] Tests not applicable — justification: documentation-only release
preparation; the Fern docs build validates the changed pages and routes.
- [x] Docs updated for user-facing behavior changes
- [ ] Docs not applicable — justification:
- [ ] Sensitive paths changed (security, policy, credentials, preflight,
onboarding, inference, runner, sandbox, or messaging)
- [ ] Sensitive-path review completed or maintainer-approved waiver
recorded — reviewer/approval link/justification:
- [ ] Non-success, skipped, or missing CI check accepted by maintainer —
check name, approval link, and follow-up issue:

## Verification
<!-- Check each item you ran and confirmed. Leave unchecked items you
skipped. Doc-only changes do not require npm test unless you ran it. -->
- [x] PR description includes the DCO sign-off declaration and every
commit appears as `Verified` in GitHub
- [x] Git hooks passed during commit and push, or `npx prek run
--from-ref main --to-ref HEAD` passes
- [ ] Targeted tests pass for changed behavior
- [ ] Full `npm test` passes (broad runtime changes only)
- [x] Quality Gates section completed with required justifications or
waivers
- [x] No secrets, API keys, or credentials committed
- [ ] `npm run docs` builds without warnings (doc changes only)
- [x] Doc pages follow the [style
guide](https://github.com/NVIDIA/NemoClaw/blob/main/docs/CONTRIBUTING.md)
(doc changes only)
- [ ] New doc pages include SPDX header and frontmatter (new pages only)

---
<!-- DCO sign-off is required in this PR description, and every commit
must appear as Verified in GitHub. Run: git config user.name && git
config user.email -->
Signed-off-by: Carlos Villela <cvillela@nvidia.com>


<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit

* **Documentation**
* Added a new **v0.0.73** release notes section with six highlights at
the top of the changelog.
* Expanded **Custom Endpoint URL Validation** guidance in inference
option docs, including explicit acceptance/rejection rules for HTTP vs
DNS-backed HTTPS and how validated IPs are stored.
* Updated command references (`nemohermes inference set`, `$$nemoclaw
inference set`) to match the new validation behavior.
* Added troubleshooting documentation for unsupported **DNS-backed HTTPS
endpoints**, plus clarified Deep Agents Code routing and post-upgrade
sandbox rebuild guidance.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->

---------

Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: cli Command line interface, flags, terminal UX, or output bug-fix PR fixes a bug or regression integration: dcode LangChain Deep Code integration behavior NV QA Bugs found by the NVIDIA QA Team

Projects

None yet

3 participants