fix(sandbox): surface policy-denial logs breadcrumb in connect shells (#5978) - #5992
fix(sandbox): surface policy-denial logs breadcrumb in connect shells (#5978)#5992yimoj wants to merge 1 commit into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (5)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds a CONNECT 403 policy-denial breadcrumb in connect flows, plus shell startup logic, regression tests, and troubleshooting docs for diagnosis and remediation. ChangesPolicy Denial Hint Breadcrumb
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Suggested reviewers
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
d1670d7 to
624f72a
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
test/repro-5978-policy-denial-hint.test.ts (1)
92-96: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPrefer an observable auto-source check over matching implementation text.
This asserts the stanza contains
_nemoclaw_maybe_policy_denial_hint, not that sourcing it actually emits the breadcrumb. It also becomes brittle on harmless refactors. Please drive the PTY path and assert the auto-call behavior directly. As per path instructions, “Prefer observable outcomes through the public boundary over source-text, private-shape, or mock-call assertions.”🤖 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/repro-5978-policy-denial-hint.test.ts` around lines 92 - 96, The current test in repro-5978-policy-denial-hint.test.ts only matches the source text for _nemoclaw_maybe_policy_denial_hint, which is brittle and does not prove the auto-invocation behavior. Update the test to exercise the PTY/sourcing path through the public boundary and assert the observable breadcrumb is emitted when the stanza is sourced, rather than checking for the private symbol name in the stanza string.Source: Path instructions
🤖 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 `@scripts/nemoclaw-start.sh`:
- Around line 3051-3058: Sanitize the OPENSHELL_SANDBOX value before
_nemoclaw_policy_denial_hint_label prints it, since the current fallback path
echoes it verbatim to the terminal and can be abused with control characters or
newlines. Update the logic in _nemoclaw_policy_denial_hint_label to strip unsafe
characters before emitting the sandbox name, while preserving the existing
placeholder behavior for empty/boolean values. Add a negative-path regression
test that feeds a crafted OPENSHELL_SANDBOX value with control characters and
verifies the output is sanitized and does not leak raw escape sequences.
In `@test/repro-5978-policy-denial-hint.test.ts`:
- Around line 57-85: The shell helpers run commands via runInPty and runPlain
but their callers only assert on captured output, so a failing spawned shell
could still look like a successful silent path. Update the tests that use
runInPty and runPlain to also assert the returned status is zero before treating
empty output as a pass, using the existing status field from these helpers to
validate the public outcome of the spawned bash/script execution.
- Around line 37-46: The helper extractHintStanza currently uses a conditional
guard that the growth-guardrail job rejects, so rewrite it without an if
statement while preserving the same validation and slice behavior. Update
extractHintStanza in the policy-denial-hint test to compute or assert the marker
positions using a guardrail-safe approach, keeping the same begin/end marker
checks and error path without introducing any conditional syntax.
---
Nitpick comments:
In `@test/repro-5978-policy-denial-hint.test.ts`:
- Around line 92-96: The current test in repro-5978-policy-denial-hint.test.ts
only matches the source text for _nemoclaw_maybe_policy_denial_hint, which is
brittle and does not prove the auto-invocation behavior. Update the test to
exercise the PTY/sourcing path through the public boundary and assert the
observable breadcrumb is emitted when the stanza is sourced, rather than
checking for the private symbol name in the stanza string.
🪄 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: 1d816909-1911-4ffa-8fe4-916ebdd21a61
📒 Files selected for processing (3)
docs/reference/troubleshooting.mdxscripts/nemoclaw-start.shtest/repro-5978-policy-denial-hint.test.ts
624f72a to
8f16de8
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/repro-5978-policy-denial-hint.test.ts (1)
96-100: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExercise shell behavior here instead of matching the stanza text.
This assertion is coupled to the private helper name and only proves the source text contains a call site. Source the stanza in the same PTY setup used below and assert the breadcrumb appears without an explicit
_nemoclaw_maybe_policy_denial_hintcall. As per path instructions, "Prefer observable outcomes through the public boundary over source-text, private-shape, or mock-call assertions."🤖 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/repro-5978-policy-denial-hint.test.ts` around lines 96 - 100, The current test is asserting on the stanza source text and a private helper name, which couples it to implementation details. Update the test in the auto-invokes gate case to use the same PTY/shell setup as the other tests, source the stanza through that public boundary, and assert the breadcrumb appears from observable shell behavior without matching `_nemoclaw_maybe_policy_denial_hint` or the raw stanza text.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@test/repro-5978-policy-denial-hint.test.ts`:
- Around line 96-100: The current test is asserting on the stanza source text
and a private helper name, which couples it to implementation details. Update
the test in the auto-invokes gate case to use the same PTY/shell setup as the
other tests, source the stanza through that public boundary, and assert the
breadcrumb appears from observable shell behavior without matching
`_nemoclaw_maybe_policy_denial_hint` or the raw stanza text.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: e89f90a1-f10d-4922-a185-532121c2be60
📒 Files selected for processing (3)
docs/reference/troubleshooting.mdxscripts/nemoclaw-start.shtest/repro-5978-policy-denial-hint.test.ts
✅ Files skipped from review due to trivial changes (1)
- docs/reference/troubleshooting.mdx
🚧 Files skipped from review as they are similar to previous changes (1)
- scripts/nemoclaw-start.sh
Reporter-workflow E2E evidence (real sandbox base image)This fix's surface is the in-sandbox connect shell, so there is no host The breadcrumb is emitted once at connect-shell init by the real image hooks (dedup holds even though both This is codified as an opt-in container E2E ( NEMOCLAW_RUN_POLICY_HINT_DOCKER_E2E=1 vitest run \
test/repro-5978-policy-denial-hint.test.ts --project integration
# → 11 passed (includes "real base image: connect shell shows the breadcrumb once and curl is denied with 403")A full |
978ff1c to
2f768ab
Compare
Real host-CLI E2E — reporter workflow against a worktree-built sandboxStood up an isolated sandbox built from this branch (so the image bakes in the changed This proves the full acceptance criteria end-to-end through the host CLI: the breadcrumb appears in the Note on the |
2f768ab to
fcc75cc
Compare
|
Follow-up (head |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/repro-5978-policy-denial-hint.test.ts (1)
121-162: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid testing the shell-private helpers directly.
These cases call
_nemoclaw_policy_denial_hint_label/_nemoclaw_policy_denial_hint_textinstead of exercising the emitted connect-shell behavior, so they lock the suite to internal helper names and can miss regressions in the real breadcrumb path. Prefer driving the same scenarios through_nemoclaw_maybe_policy_denial_hint(or source-time sourcing) and asserting the user-visible breadcrumb text/fallback name. As per path instructions, “Prefer observable outcomes through the public boundary over source-text, private-shape, or mock-call assertions.”🤖 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/repro-5978-policy-denial-hint.test.ts` around lines 121 - 162, The tests are targeting shell-private helpers directly, which ties them to internal names and bypasses the real breadcrumb path. Update the cases around _nemoclaw_policy_denial_hint_label and _nemoclaw_policy_denial_hint_text to exercise the emitted behavior through _nemoclaw_maybe_policy_denial_hint or equivalent source-time sourcing, and assert only the user-visible breadcrumb text, fallback name, and sanitization outcomes. Keep the same scenarios, but move the assertions to the public boundary so the suite covers the actual connect-shell flow.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@test/repro-5978-policy-denial-hint.test.ts`:
- Around line 121-162: The tests are targeting shell-private helpers directly,
which ties them to internal names and bypasses the real breadcrumb path. Update
the cases around _nemoclaw_policy_denial_hint_label and
_nemoclaw_policy_denial_hint_text to exercise the emitted behavior through
_nemoclaw_maybe_policy_denial_hint or equivalent source-time sourcing, and
assert only the user-visible breadcrumb text, fallback name, and sanitization
outcomes. Keep the same scenarios, but move the assertions to the public
boundary so the suite covers the actual connect-shell flow.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: baeb933a-7a9b-4d21-8bc6-fae59d1ace05
📒 Files selected for processing (5)
docs/reference/troubleshooting.mdxscripts/nemoclaw-start.shsrc/lib/actions/sandbox/connect-policy-hint.test.tssrc/lib/actions/sandbox/connect.tstest/repro-5978-policy-denial-hint.test.ts
✅ Files skipped from review due to trivial changes (2)
- src/lib/actions/sandbox/connect-policy-hint.test.ts
- docs/reference/troubleshooting.mdx
🚧 Files skipped from review as they are similar to previous changes (1)
- scripts/nemoclaw-start.sh
|
Addressed the CodeRabbit nitpick: the breadcrumb tests no longer call the shell-private |
fcc75cc to
c0577f7
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/repro-5978-policy-denial-hint.test.ts`:
- Around line 143-149: The control-character sanitization test only verifies
that ESC is removed, but it still misses the newline-injection path. Update the
existing `gate(...)` assertion in `repro-5978-policy-denial-hint.test.ts` to
also confirm the crafted `OPENSHELL_SANDBOX` value cannot emit a separate
`INJECTED` line through stdout, alongside the current `stdout` checks in the
`it("strips control characters...")` case. Keep the verification at the public
boundary by asserting the observable terminal output, not internal state.
🪄 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: a92c6b75-0adb-45c0-ae0a-db5018216604
📒 Files selected for processing (5)
docs/reference/troubleshooting.mdxscripts/nemoclaw-start.shsrc/lib/actions/sandbox/connect-policy-hint.test.tssrc/lib/actions/sandbox/connect.tstest/repro-5978-policy-denial-hint.test.ts
✅ Files skipped from review due to trivial changes (1)
- docs/reference/troubleshooting.mdx
🚧 Files skipped from review as they are similar to previous changes (3)
- src/lib/actions/sandbox/connect-policy-hint.test.ts
- src/lib/actions/sandbox/connect.ts
- scripts/nemoclaw-start.sh
c0577f7 to
16f8e75
Compare
|
@coderabbitai resume |
✅ Action performedReviews resumed. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/repro-5978-policy-denial-hint.test.ts`:
- Around line 188-203: The test in policy denial hint should model the real
login-shell behavior by re-sourcing the extracted stanza twice instead of
invoking _nemoclaw_maybe_policy_denial_hint twice directly. Update the snippet
setup in the repro test so the stanza is sourced on both passes with SHLVL=1,
then keep asserting the observable public output from stdout only once to verify
the once-per-session sentinel survives a second source.
🪄 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: a6a7397d-d0b2-495c-8eba-375ad0a33e02
📒 Files selected for processing (5)
docs/reference/troubleshooting.mdxscripts/nemoclaw-start.shsrc/lib/actions/sandbox/connect-policy-hint.test.tssrc/lib/actions/sandbox/connect.tstest/repro-5978-policy-denial-hint.test.ts
✅ Files skipped from review due to trivial changes (1)
- docs/reference/troubleshooting.mdx
🚧 Files skipped from review as they are similar to previous changes (3)
- src/lib/actions/sandbox/connect-policy-hint.test.ts
- src/lib/actions/sandbox/connect.ts
- scripts/nemoclaw-start.sh
16f8e75 to
dae14e6
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/repro-5978-policy-denial-hint.test.ts`:
- Around line 136-141: The fallback-path tests in the gate behavior only assert
on stdout, so they can pass even if the spawned shell exits with a failure
status. Update the affected cases in the gate/repro policy denial tests to also
capture the process status from gate() and assert it is 0, matching the
surrounding behavioral checks and the public-boundary assertion style.
🪄 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: 591712b7-cf48-485c-a1a0-92abda6c6b51
📒 Files selected for processing (5)
docs/reference/troubleshooting.mdxscripts/nemoclaw-start.shsrc/lib/actions/sandbox/connect-policy-hint.test.tssrc/lib/actions/sandbox/connect.tstest/repro-5978-policy-denial-hint.test.ts
✅ Files skipped from review due to trivial changes (1)
- docs/reference/troubleshooting.mdx
🚧 Files skipped from review as they are similar to previous changes (3)
- src/lib/actions/sandbox/connect-policy-hint.test.ts
- src/lib/actions/sandbox/connect.ts
- scripts/nemoclaw-start.sh
Sandbox outbound egress is denied-by-default and enforced by the OpenShell L7 proxy. From inside the sandbox, generic CLIs (curl, git, wget, python) see a policy denial only as the opaque "CONNECT tunnel failed, response 403", with no pointer to the detailed allow/deny reason, which lives in the NemoClaw logs. Surface a breadcrumb on two complementary surfaces: - In-sandbox: a `# nemoclaw-policy-denial-hint` stanza emitted into /tmp/nemoclaw-proxy-env.sh (sourced by every interactive/login sandbox shell), shown once per top-level interactive TTY session and suppressible with NEMOCLAW_NO_POLICY_HINT=1. It deliberately does not wrap curl/git/wget (wrapping pipes their stderr, dropping TTY progress/colour and missing python), so every tool's output and exit code are unchanged. The sandbox name comes from OPENSHELL_SANDBOX (sanitized of control characters), with a `<name>` fallback on older OpenShell that sets it to "1". - Host-side: the `nemoclaw <name> connect` one-shot hint now also prints the breadcrumb with the real sandbox name, so the reporter's flow gets a directly runnable `nemoclaw <name> logs --tail 50` even when the in-sandbox value is unavailable. Adds regression tests (the emitted stanza is executed, plus a real-image docker E2E behind NEMOCLAW_RUN_POLICY_HINT_DOCKER_E2E=1) and a troubleshooting doc entry. Fixes NVIDIA#5978 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Yimo Jiang <yimoj@nvidia.com>
dae14e6 to
4067864
Compare
|
Closed/recreated because this PR was accidentally opened from fork head yimoj:NemoClaw. Replacement upstream-head PR: #6018 (NVIDIA:fix/5978-policy-denial-logs-hint). |
Summary
Sandbox outbound egress is denied-by-default and enforced by the OpenShell L7 proxy. From inside the sandbox, generic CLIs (curl, git, wget, python) see a policy denial only as the opaque
CONNECT tunnel failed, response 403, with no pointer to the detailed allow/deny reason that lives in the NemoClaw logs. This adds a tool-agnostic breadcrumb that points the user tonemoclaw <name> logs --tail 50.Related Issue
Fixes #5978
Changes
scripts/nemoclaw-start.sh: add a# nemoclaw-policy-denial-hintstanza to the/tmp/nemoclaw-proxy-env.shblock already sourced by every interactive/login sandbox shell (via/etc/bash.bashrcand/etc/profile.d/nemoclaw-proxy.sh). It prints a one-line breadcrumb naming theCONNECT tunnel failed, response 403signature andnemoclaw <name> logs --tail 50.$-interactive,[ -t 2 ],SHLVL <= 1, a proxy being configured, and a once-per-shell guard so a login shell sourcing both/etc/profileand/etc/bash.bashrcdoes not double-print). Suppressible withNEMOCLAW_NO_POLICY_HINT=1.OPENSHELL_SANDBOX(OpenShell ≥0.0.44), falling back to<name>on older versions.curl/git/wget: wrapping them to scan stderr turns their stderr into a pipe, makes the tools drop progress meters and colour, and still missespython. The informational hint leaves every tool's stdout/stderr/TTY behaviour and exit code byte-for-byte unchanged, while covering generic CLIs — not just Node processes (the explicit risk called out in the issue).test/repro-5978-policy-denial-hint.test.ts: executes the actual emitted stanza (extracted from the single-quoted heredoc, byte-identical to runtime) and asserts the breadcrumb content, name derivation/fallback, the suppress/no-proxy/non-interactive/subshell gates, and single-print on double-source.docs/reference/troubleshooting.mdx: new entry "A request inside the sandbox fails withCONNECT tunnel failed, response 403".nemoclaw <name> logs --tail 50already surfaces the merged OpenShell denial line, so no logs-path change is required.Type of Change
Quality Gates
Verification
Real reporter-workflow E2E with the host CLI, against a sandbox built from this branch (the image bakes in the changed
nemoclaw-start.sh):The
connectshell printed the breadcrumb, a realcurlwas denied withCONNECT tunnel failed, response 403, andlogs --tail 50showed the denial reason (DENIEDonexample.com:443,not in policy). Full transcript in the PR comment.Verifiedin GitHubvitest run test/repro-5978-policy-denial-hint.test.ts(13 passing incl. the opt-in real-image container E2E; deterministic underenv -u SHLVL)shfmt -i 2 -ci -bnclean; Biome clean;tsc -p jsconfig.jsonclean; source-shape budget 0; codebase-growth-guardrails passnpm run docsbuilds without warningsSigned-off-by: Yimo Jiang yimoj@nvidia.com
Summary by CodeRabbit
CONNECT tunnel failed, response 403signature and anemoclaw <sandbox> logs --tail 50command to locate the denying rule.