Fix doctor and backup checks for local gateway runtimes - #4583
Fix doctor and backup checks for local gateway runtimes#4583Christoffer91 wants to merge 2 commits 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 (1)
💤 Files with no reviewable changes (1)
📝 WalkthroughWalkthroughAdds a host-level gateway detection fallback when ChangesSandbox Operation Resilience
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly Related PRs
Suggested Labels
Suggested Reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
✨ Thanks for submitting this detailed PR about fixing doctor and backup checks for local gateway runtimes. This proposes a way to improve the NemoClaw CLI's compatibility with local OpenShell gateway processes and enhance the backup validation for large sandbox state archives. |
1 similar comment
|
✨ Thanks for submitting this detailed PR about fixing doctor and backup checks for local gateway runtimes. This proposes a way to improve the NemoClaw CLI's compatibility with local OpenShell gateway processes and enhance the backup validation for large sandbox state archives. |
wscurran
added
area: cli
20b906e to
8567021
Compare
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR improves sandbox tar validation robustness for large archives and enhances the sandbox “doctor” gateway diagnostics when Docker inspection fails.
Changes:
- Increase
spawnSyncoutput buffer fortarlisting commands to handle very large verbose output. - Add a regression test ensuring
rejectHardLinkscan process large archives without hitting Node’s default spawn buffer limit. - In
doctor, detect a locally runningopenshell-gateway(process + listening port) when Docker container inspection fails.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| test/security-sandbox-tar-traversal.test.ts | Adds a regression test for large-archive verbose listing behavior. |
| src/lib/state/sandbox.ts | Raises spawnSync maxBuffer for tar listing output in validation and hard-link rejection. |
| src/lib/actions/sandbox/doctor.ts | Adds fallback checks for a local gateway process/port when docker inspect fails. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| encoding: "utf-8", | ||
| stdio: ["pipe", "pipe", "pipe"], | ||
| timeout: 60000, | ||
| maxBuffer: 256 * 1024 * 1024, |
| encoding: "utf-8", | ||
| stdio: ["pipe", "pipe", "pipe"], | ||
| timeout: 60000, | ||
| maxBuffer: 256 * 1024 * 1024, |
| it("accepts a large archive whose verbose listing exceeds Node's default spawn buffer", async () => { | ||
| const { rejectHardLinks } = await loadSandboxState(); | ||
| const entries = Array.from({ length: 20_000 }, (_, index) => ({ | ||
| path: `workspace/file-${index.toString().padStart(5, "0")}.txt`, | ||
| content: "x", | ||
| })); | ||
|
|
||
| const violations = rejectHardLinks(buildTar(entries)); | ||
|
|
||
| expect(violations).toEqual([]); | ||
| }); |
| const processCheck = captureHostCommand("pgrep", ["-af", "openshell-gateway"], 5000); | ||
| const portCheck = captureHostCommand("ss", ["-ltn", `( sport = :${GATEWAY_PORT} )`], 5000); | ||
| const processRunning = processCheck.status === 0 && processCheck.stdout.trim().length > 0; | ||
| const portListening = portCheck.status === 0 && portCheck.stdout.includes(`:${GATEWAY_PORT}`); | ||
| if (processRunning && portListening) { | ||
| checks.push({ | ||
| group: "Gateway", | ||
| label: "Local gateway process", | ||
| status: "ok", | ||
| detail: `openshell-gateway is running and listening on port ${GATEWAY_PORT}`, | ||
| }); | ||
| return checks; | ||
| } |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/doctor.ts`:
- Line 204: The fallback in dockerInspectGateway is using
captureHostCommand("pgrep", ["-af", "openshell-gateway"], 5000) which can
produce false positives; replace the literal "openshell-gateway" argv with the
anchored HOST_GATEWAY_PGREP_PATTERN imported from
src/lib/onboard/host-gateway-process.ts and call captureHostCommand("pgrep",
["-f", HOST_GATEWAY_PGREP_PATTERN], 5000) so pgrep runs with the proper anchored
pattern; keep the existing timeout and ensure any variable name references
(e.g., processCheck) are preserved.
🪄 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: a3b47909-ef2f-4473-8a16-d45ac84dba47
📒 Files selected for processing (3)
src/lib/actions/sandbox/doctor.tssrc/lib/state/sandbox.tstest/security-sandbox-tar-traversal.test.ts
| 5000, | ||
| ); | ||
| if (inspect.status !== 0) { | ||
| const processCheck = captureHostCommand("pgrep", ["-af", "openshell-gateway"], 5000); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Simulate a false positive scenario where a user is editing the gateway binary
# but the gateway itself is not running.
# Check if the bare substring pattern would match unrelated processes
echo "Testing bare substring pattern (current code):"
ps aux | grep -i openshell-gateway | grep -v grep || echo "No matches"
echo ""
echo "The anchored pattern from HOST_GATEWAY_PGREP_PATTERN should be used instead:"
cat src/lib/onboard/host-gateway-process.ts | grep -A2 "HOST_GATEWAY_PGREP_PATTERN"Repository: NVIDIA/NemoClaw
Length of output: 618
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Inspect doctor.ts around the reported lines
echo "=== doctor.ts around lines 180-240 ==="
nl -ba src/lib/actions/sandbox/doctor.ts | sed -n '180,240p'
# 2) Inspect host-gateway-process.ts for the anchored pattern definition
echo
echo "=== host-gateway-process.ts HOST_GATEWAY_PGREP_PATTERN definition ==="
rg -n "HOST_GATEWAY_PGREP_PATTERN" src/lib/onboard/host-gateway-process.ts
nl -ba src/lib/onboard/host-gateway-process.ts | sed -n '1,120p'
# 3) Check what ss usage looks like elsewhere in the repo (to judge syntax expectations)
echo
echo "=== repo-wide 'ss -ltn' occurrences (trimmed) ==="
rg -n "ss -ltn" -S src | head -n 50Repository: NVIDIA/NemoClaw
Length of output: 159
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Show doctor.ts around the target area (no nl available)
echo "=== src/lib/actions/sandbox/doctor.ts lines 180-240 (approx) ==="
sed -n '180,240p' src/lib/actions/sandbox/doctor.ts
# 2) Locate and show HOST_GATEWAY_PGREP_PATTERN definition
echo
echo "=== src/lib/onboard/host-gateway-process.ts: HOST_GATEWAY_PGREP_PATTERN ==="
rg -n "HOST_GATEWAY_PGREP_PATTERN" src/lib/onboard/host-gateway-process.ts
sed -n '1,120p' src/lib/onboard/host-gateway-process.ts
# 3) Find how doctor.ts is currently searching for the gateway process
echo
echo "=== doctor.ts: pgrep occurrences ==="
rg -n "pgrep" src/lib/actions/sandbox/doctor.ts
# 4) Find how 'ss -ltn' is used in the repo (syntax comparison)
echo
echo "=== repo: ss -ltn occurrences (first 80 matches total) ==="
rg -n "ss -ltn" src | head -n 80Repository: NVIDIA/NemoClaw
Length of output: 7162
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== src/lib/actions/sandbox/doctor.ts lines 140-230 ==="
sed -n '140,230p' src/lib/actions/sandbox/doctor.ts
echo
echo "=== Search for other ss invocations / sport filters ==="
rg -n "ss\\b|sport\\s*=\\s*:\\$\\{?GATEWAY_PORT\\}?|sport\\s*=\\s*:" src | head -n 80Repository: NVIDIA/NemoClaw
Length of output: 10194
🌐 Web query:
ss -ltn filter syntax "( sport = :PORT )" "sport = :PORT" examples
💡 Result:
The ss command uses a filter expression syntax that supports comparing source (sport) and destination (dport) ports against specific values [1][2]. Because these characters (like parentheses, greater-than/less-than signs, or pipes) are often interpreted by the shell, they must typically be quoted or escaped when used in a command line [3][4]. Filter Syntax The general syntax for port filtering is: {dport|sport} [OP] [FAMILY:]:PORT - [OP]: The operator, such as "=", "<", "<=", "!=", ">=", or ">" [1][2]. If no operator is provided, "=" is assumed [1][2]. - [FAMILY:]: Optional, typically inet or inet6. -:PORT: The port number or name (e.g.,:80 or:http) [1][2]. Key Usage Notes - Shell Escaping: When using operators like ">", "<", or groupings like "", you must wrap the expression in quotes or escape the characters to prevent the shell from interpreting them [3][4]. For example, use '( sport =:80)' or ( sport =:80 ). - Implicit AND: If multiple predicates are provided without a boolean operator, an implicit AND is assumed [1][2]. - Boolean Operators: You can use "or" (|/||), "and" (&/&&), and "not" (!) to combine filters [1][2]. Examples - Filter by a specific port: ss sport =:80 ss 'sport =:80' - Filter with operators (requires quoting/escaping): ss 'sport >:1024' ss 'dport <=:5000' - Complex filters (grouping and boolean logic): ss -ltn '( sport =:80 or sport =:443)' ss -ltn 'state established ( dport =:ssh or sport =:ssh)' ss -ltn 'sport >=:1024 and sport <=:5000' The syntax "( sport =:PORT)" serves to group subexpressions, which is essential when combining multiple conditions with "or" to ensure correct precedence [1][2].
Citations:
- 1: https://manpages.debian.org/trixie/iproute2/ss.8
- 2: https://man7.org/linux/man-pages/man8/ss.8.html
- 3: https://www.cyberciti.biz/tips/linux-investigate-sockets-network-connections.html
- 4: https://www.cyberciti.biz/files/ss.html
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find captureHostCommand implementation
rg -n "function captureHostCommand|captureHostCommand\\(" src/lib/actions/sandbox/doctor.ts
# Print the relevant section (top of file)
sed -n '1,140p' src/lib/actions/sandbox/doctor.ts
# Print captureHostCommand section if it's elsewhere in same file
# (fallback: print around first occurrence line)
line=$(rg -n "function captureHostCommand" src/lib/actions/sandbox/doctor.ts | cut -d: -f1 | head -n1 || true)
if [ -n "${line:-}" ]; then
start=$((line-20))
end=$((line+80))
sed -n "${start},${end}p" src/lib/actions/sandbox/doctor.ts
fiRepository: NVIDIA/NemoClaw
Length of output: 7856
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show imports at top of doctor.ts to see if it can/does use HOST_GATEWAY_PGREP_PATTERN
sed -n '1,80p' src/lib/actions/sandbox/doctor.ts
# Show any references to host-gateway-process.ts / HOST_GATEWAY_PGREP_PATTERN in doctor.ts
rg -n "host-gateway-process|HOST_GATEWAY_PGREP_PATTERN" src/lib/actions/sandbox/doctor.ts || trueRepository: NVIDIA/NemoClaw
Length of output: 2950
🌐 Web query:
ss filter whitespace "sport = :80" "sport =:80" syntax
💡 Result:
In the ss command filter syntax, both "sport =:80" and "sport =:80" are generally parsed correctly because the filter expression parser is designed to be flexible regarding whitespace around operators [1][2][3]. The underlying grammar for ss filters, implemented in the iproute2 source code (specifically in ssfilter.y), processes tokens like SPORT, operators (e.g., '='), and host conditions (e.g., ':80') as distinct elements regardless of whether they are separated by spaces [3][4]. Key points regarding the syntax: - Operator flexibility: The '=' operator (and its aliases like '==', 'eq') can be used with or without surrounding whitespace [5][3]. - Port notation: The colon prefix (e.g., ':80') is the standard way to specify a port in ss filter expressions [1][6][7]. - Expression grouping: It is best practice to enclose the entire filter expression in single quotes (e.g., 'sport =:80') to prevent the shell from interpreting characters like '>', '<', or '|' as shell operators [1][8][7]. While "sport =:80" works, including a space (i.e., "sport =:80") is the recommended convention for readability and consistency with common documentation examples [1][9][7].
Citations:
- 1: https://www.man7.org/linux/man-pages/man8/ss.8.html
- 2: https://man.archlinux.org/man/ss.8
- 3: https://github.com/shemminger/iproute2/blob/main/misc/ssfilter.y
- 4: https://android.googlesource.com/platform/external/iproute2/+/cb09af2a5cf9a7b763dd35e1af4c4cde036399a5/misc/ssfilter.y
- 5: https://man7.org/linux/man-pages/man8/ss.8.html
- 6: https://android.googlesource.com/platform/external/iproute2/+/0146a363ec4ee6a98910edbb97f0032e87375989/doc/ss.sgml
- 7: https://www.mankier.com/8/ss
- 8: https://manpages.org/ss/8
- 9: https://github.com/Jimmy-Ki/linux-wiki/blob/main/docs/commands/system-info/ss.md
Use HOST_GATEWAY_PGREP_PATTERN in dockerInspectGateway’s fallback to avoid false positives.
src/lib/actions/sandbox/doctor.ts currently checks pgrep -af openshell-gateway, which can match any unrelated process whose command line merely contains the substring. Use the anchored HOST_GATEWAY_PGREP_PATTERN from src/lib/onboard/host-gateway-process.ts (via pgrep -f ...) instead. The ss -ltn ( sport = :${GATEWAY_PORT} ) filter is passed as an argv element (no shell parsing) and is consistent with valid ss filter grouping.
🤖 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/doctor.ts` at line 204, The fallback in
dockerInspectGateway is using captureHostCommand("pgrep", ["-af",
"openshell-gateway"], 5000) which can produce false positives; replace the
literal "openshell-gateway" argv with the anchored HOST_GATEWAY_PGREP_PATTERN
imported from src/lib/onboard/host-gateway-process.ts and call
captureHostCommand("pgrep", ["-f", HOST_GATEWAY_PGREP_PATTERN], 5000) so pgrep
runs with the proper anchored pattern; keep the existing timeout and ensure any
variable name references (e.g., processCheck) are preserved.
## Summary Salvages the intended fixes from #4583 on a clean branch. `nemoclaw doctor` now accepts a verified local `openshell-gateway` process when legacy container inspection fails, and tar listing validation has enough buffer headroom for large sandbox state backups. ## Changes - Reuse the anchored host gateway pgrep pattern in `doctor` and report a local gateway as healthy only when both the process and gateway port are present. - Increase the tar listing `spawnSync` buffer used by path validation and hard-link rejection. - Add regression coverage for the local gateway doctor fallback and for large hard-link validation archives. ## Type of Change - [x] 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) ## Verification - [x] `npx prek run --all-files` passes - [x] `npm test` passes - [x] Tests added or updated for new or changed behavior - [x] No secrets, API keys, or credentials committed - [ ] Docs updated for user-facing behavior changes - [ ] `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: Carlos Villela <cvillela@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Diagnostics: improved gateway checks — when container inspection fails, doctor probes the local gateway process/port, adjusts results based on available evidence and probe-tool availability, and more reliably captures host command outcomes. * Reliability: safer handling of very large tar archives by constraining command output buffering during archive inspection. * **Tests** * Added integration and regression tests covering gateway fallback behaviors and large-tar scenarios. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Summary
nemoclaw doctorto accept a running localopenshell-gatewayprocess when the legacyopenshell-cluster-nemoclawDocker container is not presenttar -tvfhard-link validation buffer for large sandbox state backupsWhy
On a Linux Docker-driver install using a local OpenShell gateway process,
nemoclaw xterm doctorreported a gateway Docker-container failure even though OpenShell status, sandbox exec, inference, and local services were healthy. The check was hardcoded aroundopenshell-cluster-nemoclaw.Large OpenClaw sandbox state also caused
nemoclaw backup-allto fail during hard-link validation becausetar -tvf -produced more stdout than Node’s defaultspawnSyncbuffer. The tar archive itself was valid and backup succeeded after increasing the buffer.Validation
npm run build:clinpm test -- test/security-sandbox-tar-traversal.test.tsnemoclaw xterm doctor --jsonreturnedstatus=ok failed=0 warnings=0after applying the equivalent patchSummary by CodeRabbit
Bug Fixes
Tests