fix(whatsapp): revert post-pair gateway restart#6571
Closed
sandl99 wants to merge 5 commits into
Closed
Conversation
Reverts PR #6496 / merge commit 1ff8855. Reason 1: Manual validation shows PR #6496 does not fix the issue; it still reproduces. Reason 2: It regresses DGX Spark and DGX Station. Observed error: NemoClaw rejected a conflicting gateway trust anchor. The ambient gateway token could not be cleared. Signed-off-by: San Dang <sdang@nvidia.com>
Contributor
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage remains at 96%, unchanged from the branch. TypeScript / code-coverage/cliThe overall coverage in the branch remains at 77%, unchanged from the branch. Show a code coverage summary of the most impacted files.
Updated |
Contributor
|
🌿 Preview your docs: https://nvidia-preview-pr-6571.docs.buildwithfern.com/nemoclaw |
Contributor
📝 WalkthroughWalkthroughThis PR removes the WhatsApp post-pair restart path, drops the trusted gateway URL anchor guard, updates related shell tests and harness timeouts, and shortens the WhatsApp sandbox documentation. ChangesWhatsApp login and runtime cleanup
Estimated code review effort: 3 (Moderate) | ~25 minutes Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Contributor
PR Review Advisor (Nemotron Ultra) — InformationalMerge posture: Informational / low confidence Action checklist
Findings index
Review findings by urgency: 0 required fixes, 1 item to resolve/justify, 0 in-scope improvements
|
Contributor
E2E Advisor RecommendationRequired E2E: Dispatch hint: Full advisor summaryE2E Recommendation AdvisorBase: Required E2E
Optional E2E
New E2E recommendations
Dispatch hint
|
Contributor
E2E Target RecommendationRequired E2E targets: Dispatch required E2E targets:
Full E2E target advisor summaryE2E Target AdvisorBase: Required E2E targets
Optional E2E targets
Relevant changed files
|
Contributor
PR Review Advisor — Changes requestedMerge posture: Do not merge yet Action checklist
Findings index
Review findings by urgency: 0 required fixes, 2 items to resolve/justify, 0 in-scope improvements
|
Signed-off-by: San Dang <sdang@nvidia.com>
Signed-off-by: San Dang <sdang@nvidia.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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 `@src/lib/actions/sandbox/mcp-bridge-status-resolution.test.ts`:
- Around line 16-17: Keep the harness timeout ordering fixed by ensuring
HARNESS_PROCESS_TIMEOUT_MS remains below the Vitest timeout used in
HARNESS_TEST_OPTIONS. In mcp-bridge-status-resolution.test.ts, adjust the setup
around execTimeout() and testTimeoutOptions() so both derive from one shared
budget or add an explicit guard that prevents NEMOCLAW_EXEC_TIMEOUT from
exceeding NEMOCLAW_TEST_TIMEOUT. Use the HARNESS_PROCESS_TIMEOUT_MS and
HARNESS_TEST_OPTIONS constants as the anchor points for the fix.
🪄 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: 7f321b35-b02b-47a3-9f08-6d1a0148dd40
📒 Files selected for processing (1)
src/lib/actions/sandbox/mcp-bridge-status-resolution.test.ts
Signed-off-by: San Dang <sdang@nvidia.com>
Signed-off-by: San Dang <sdang@nvidia.com>
Contributor
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/nemoclaw-start-gateway-ws-host.test.ts`:
- Around line 327-385: The test in the WhatsApp gateway login case is not
actually verifying token withholding because OPENCLAW_GATEWAY_TOKEN is never
seeded, so the unset value can pass trivially. Update the test setup in the
`it("withholds the gateway token from caller-selected WhatsApp login gateways
(`#6291`)")` block to export a known token before invoking `openclaw channels
login --channel whatsapp`, then assert that the spawned `openclaw` process in
the fake bin sees `TOKEN=unset` and does not receive the seeded token. Keep the
existing checks around `writeRuntimeShellEnv`, `spawnSync`, and the call log so
the test still validates the gateway URL and insecure flag behavior.
🪄 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: 8b200b08-4176-4ca7-94ef-c011827c5f91
📒 Files selected for processing (3)
scripts/nemoclaw-start.shsrc/lib/actions/sandbox/mcp-bridge-status-resolution.test.tstest/nemoclaw-start-gateway-ws-host.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/lib/actions/sandbox/mcp-bridge-status-resolution.test.ts
- scripts/nemoclaw-start.sh
Comment on lines
+327
to
+385
| it("withholds the gateway token from caller-selected WhatsApp login gateways (#6291)", () => { | ||
| const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), "nemoclaw-whatsapp-custom-gw-")); | ||
| try { | ||
| const runtimeEnv = writeRuntimeShellEnv(tmpDir); | ||
| const fakeBin = path.join(tmpDir, "bin"); | ||
| const callLog = path.join(tmpDir, "openclaw-call.log"); | ||
| fs.mkdirSync(fakeBin); | ||
| fs.writeFileSync( | ||
| path.join(fakeBin, "openclaw"), | ||
| `#!/usr/bin/env bash | ||
| { | ||
| printf 'ARGS=%s\n' "$*" | ||
| printf 'URL=%s\n' "\${OPENCLAW_GATEWAY_URL-unset}" | ||
| printf 'INSECURE=%s\n' "\${OPENCLAW_ALLOW_INSECURE_PRIVATE_WS-unset}" | ||
| printf 'TOKEN=%s\n' "\${OPENCLAW_GATEWAY_TOKEN-unset}" | ||
| } > ${JSON.stringify(callLog)} | ||
| exit 23 | ||
| `, | ||
| { mode: 0o755 }, | ||
| ); | ||
|
|
||
| const result = spawnSync( | ||
| "bash", | ||
| [ | ||
| "--noprofile", | ||
| "--norc", | ||
| "-c", | ||
| [ | ||
| "set -euo pipefail", | ||
| `. ${JSON.stringify(runtimeEnv)}`, | ||
| "OPENCLAW_GATEWAY_URL=wss://gateway.example.test:443", | ||
| "OPENCLAW_ALLOW_INSECURE_PRIVATE_WS=explicit-marker", | ||
| "openclaw channels login --channel whatsapp", | ||
| ].join("\n"), | ||
| ], | ||
| { | ||
| encoding: "utf-8", | ||
| timeout: 5000, | ||
| env: { | ||
| ...process.env, | ||
| PATH: `${fakeBin}:${process.env.PATH ?? ""}`, | ||
| NODE_OPTIONS: "", | ||
| }, | ||
| }, | ||
| ); | ||
|
|
||
| expect(result.status, result.stderr || result.stdout).toBe(23); | ||
| expect(result.stderr).toContain("Pairing exited with code 23 before it completed"); | ||
| const call = fs.readFileSync(callLog, "utf-8"); | ||
| expect(call).toContain("ARGS=channels login --channel whatsapp"); | ||
| expect(call).toContain("URL=wss://gateway.example.test:443"); | ||
| expect(call).toContain("INSECURE=explicit-marker"); | ||
| expect(call).toContain("TOKEN=unset"); | ||
| expect(call).not.toContain("test-gateway-token"); | ||
| } finally { | ||
| fs.rmSync(tmpDir, { recursive: true, force: true }); | ||
| } | ||
| }); | ||
|
|
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## test/nemoclaw-start-gateway-ws-host.test.ts (relevant range)\n'
sed -n '327,385p' test/nemoclaw-start-gateway-ws-host.test.ts
printf '\n## search for OPENCLAW_GATEWAY_TOKEN usage in tests and scripts\n'
rg -n "OPENCLAW_GATEWAY_TOKEN|withholds the gateway token|clears the gateway token" test scripts -S
printf '\n## scripts/nemoclaw-start.sh around gateway unset logic\n'
rg -n "unset OPENCLAW_GATEWAY_TOKEN|_nemoclaw_whatsapp_gateway_url_from_override|OPENCLAW_GATEWAY_URL" scripts/nemoclaw-start.sh -n -S
sed -n '3430,3505p' scripts/nemoclaw-start.shRepository: NVIDIA/NemoClaw
Length of output: 2014
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
p = Path('test/nemoclaw-start-gateway-ws-host.test.ts')
text = p.read_text()
print('OPENCLAW_GATEWAY_TOKEN set in this test:', 'OPENCLAW_GATEWAY_TOKEN=' in text[text.find('withholds the gateway token'):text.find('finally')])
PYRepository: NVIDIA/NemoClaw
Length of output: 2014
Seed a known gateway token before checking it gets withheld
This test never sets OPENCLAW_GATEWAY_TOKEN, so TOKEN=unset and not.toContain("test-gateway-token") can pass without exercising the clearing path. Export a token first, then assert it is removed when the caller selects a gateway URL.
Suggested setup
"set -euo pipefail",
`. ${JSON.stringify(runtimeEnv)}`,
+ "OPENCLAW_GATEWAY_TOKEN=test-gateway-token",
"OPENCLAW_GATEWAY_URL=wss://gateway.example.test:443",
"OPENCLAW_ALLOW_INSECURE_PRIVATE_WS=explicit-marker",
"openclaw channels login --channel whatsapp",📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| it("withholds the gateway token from caller-selected WhatsApp login gateways (#6291)", () => { | |
| const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), "nemoclaw-whatsapp-custom-gw-")); | |
| try { | |
| const runtimeEnv = writeRuntimeShellEnv(tmpDir); | |
| const fakeBin = path.join(tmpDir, "bin"); | |
| const callLog = path.join(tmpDir, "openclaw-call.log"); | |
| fs.mkdirSync(fakeBin); | |
| fs.writeFileSync( | |
| path.join(fakeBin, "openclaw"), | |
| `#!/usr/bin/env bash | |
| { | |
| printf 'ARGS=%s\n' "$*" | |
| printf 'URL=%s\n' "\${OPENCLAW_GATEWAY_URL-unset}" | |
| printf 'INSECURE=%s\n' "\${OPENCLAW_ALLOW_INSECURE_PRIVATE_WS-unset}" | |
| printf 'TOKEN=%s\n' "\${OPENCLAW_GATEWAY_TOKEN-unset}" | |
| } > ${JSON.stringify(callLog)} | |
| exit 23 | |
| `, | |
| { mode: 0o755 }, | |
| ); | |
| const result = spawnSync( | |
| "bash", | |
| [ | |
| "--noprofile", | |
| "--norc", | |
| "-c", | |
| [ | |
| "set -euo pipefail", | |
| `. ${JSON.stringify(runtimeEnv)}`, | |
| "OPENCLAW_GATEWAY_URL=wss://gateway.example.test:443", | |
| "OPENCLAW_ALLOW_INSECURE_PRIVATE_WS=explicit-marker", | |
| "openclaw channels login --channel whatsapp", | |
| ].join("\n"), | |
| ], | |
| { | |
| encoding: "utf-8", | |
| timeout: 5000, | |
| env: { | |
| ...process.env, | |
| PATH: `${fakeBin}:${process.env.PATH ?? ""}`, | |
| NODE_OPTIONS: "", | |
| }, | |
| }, | |
| ); | |
| expect(result.status, result.stderr || result.stdout).toBe(23); | |
| expect(result.stderr).toContain("Pairing exited with code 23 before it completed"); | |
| const call = fs.readFileSync(callLog, "utf-8"); | |
| expect(call).toContain("ARGS=channels login --channel whatsapp"); | |
| expect(call).toContain("URL=wss://gateway.example.test:443"); | |
| expect(call).toContain("INSECURE=explicit-marker"); | |
| expect(call).toContain("TOKEN=unset"); | |
| expect(call).not.toContain("test-gateway-token"); | |
| } finally { | |
| fs.rmSync(tmpDir, { recursive: true, force: true }); | |
| } | |
| }); | |
| "set -euo pipefail", | |
| `. ${JSON.stringify(runtimeEnv)}`, | |
| "OPENCLAW_GATEWAY_TOKEN=test-gateway-token", | |
| "OPENCLAW_GATEWAY_URL=wss://gateway.example.test:443", | |
| "OPENCLAW_ALLOW_INSECURE_PRIVATE_WS=explicit-marker", | |
| "openclaw channels login --channel whatsapp", |
🧰 Tools
🪛 ast-grep (0.44.1)
[warning] 333-345: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.writeFileSync(
path.join(fakeBin, "openclaw"),
#!/usr/bin/env bash { printf 'ARGS=%s\n' "$*" printf 'URL=%s\n' "\${OPENCLAW_GATEWAY_URL-unset}" printf 'INSECURE=%s\n' "\${OPENCLAW_ALLOW_INSECURE_PRIVATE_WS-unset}" printf 'TOKEN=%s\n' "\${OPENCLAW_GATEWAY_TOKEN-unset}" } > ${JSON.stringify(callLog)} exit 23 ,
{ mode: 0o755 },
)
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename-typescript)
[warning] 374-374: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.readFileSync(callLog, "utf-8")
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename-typescript)
🤖 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/nemoclaw-start-gateway-ws-host.test.ts` around lines 327 - 385, The test
in the WhatsApp gateway login case is not actually verifying token withholding
because OPENCLAW_GATEWAY_TOKEN is never seeded, so the unset value can pass
trivially. Update the test setup in the `it("withholds the gateway token from
caller-selected WhatsApp login gateways (`#6291`)")` block to export a known token
before invoking `openclaw channels login --channel whatsapp`, then assert that
the spawned `openclaw` process in the fake bin sees `TOKEN=unset` and does not
receive the seeded token. Keep the existing checks around
`writeRuntimeShellEnv`, `spawnSync`, and the call log so the test still
validates the gateway URL and insecure flag behavior.
Source: Path instructions
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR reverts PR #6496 because manual validation shows the original WhatsApp issue still persists after the change. It also removes the DGX Spark and DGX Station regression that reports:
NemoClaw rejected a conflicting gateway trust anchor, and the ambient gateway token could not be cleared.Related Issue
Reopens #6413
Changes
_TOOL_REDIRECTSstub for the extracted runtime-env harness.Type of Change
Quality Gates
test/nemoclaw-start-gateway-ws-host.test.ts.Verification
Verifiedin GitHubpre-commit,commit-msg, andpre-pushhooks passed, ornpm run check:diffpassed when hooks were skipped or unavailablenpx vitest run --project integration test/nemoclaw-start-gateway-ws-host.test.tspassed, 9 tests.npm testfor broad runtime/test-harness changes;npm run checkfor repo-wide validation/coverage changes — command/result:npm run docsbuilds without warnings (doc changes only) —npm run docspassed with 0 errors and 2 existing warnings.Additional validation
npx prek run shfmt --files scripts/nemoclaw-start.shpassed.npx prek run shellcheck --files scripts/nemoclaw-start.shpassed.Signed-off-by: San Dang sdang@nvidia.com
Summary by CodeRabbit
Documentation
1008close-code recovery steps.Refactor
Tests