fix(cli): add sudo to lsof and kill hints in onboarding port check#734
Closed
tienedev wants to merge 2 commits into
Closed
fix(cli): add sudo to lsof and kill hints in onboarding port check#734tienedev wants to merge 2 commits into
tienedev wants to merge 2 commits into
Conversation
…ixes #726) Non-root users cannot see root-owned listeners with lsof, so the onboarding error message now suggests `sudo lsof` and `sudo kill` instead of the unprivileged variants. The troubleshooting docs are updated to match. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
📝 WalkthroughWalkthroughThis pull request addresses a port conflict issue by adding Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~15 minutes 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 |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
docs/reference/troubleshooting.md (1)
98-103:⚠️ Potential issue | 🟡 MinorMissing
sudoon the force-terminate command.Line 102 mentions
kill -9 <PID>withoutsudo, but ifsudo kill <PID>is required (line 99), thensudo kill -9 <PID>should also be required for consistency when force-terminating root-owned processes.📝 Suggested fix
-If the process does not exit, use `kill -9 <PID>` to force-terminate it. +If the process does not exit, use `sudo kill -9 <PID>` to force-terminate it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/reference/troubleshooting.md` around lines 98 - 103, The troubleshooting doc shows "kill -9 <PID>" without sudo while earlier using "sudo kill <PID>"; update the force-terminate example so it consistently uses sudo (i.e., change "kill -9 <PID>" to "sudo kill -9 <PID>") near the "sudo lsof -i :18789" and "sudo kill <PID>" lines to ensure commands will work for root-owned processes..agents/skills/docs/nemoclaw-reference/references/troubleshooting.md (1)
72-77:⚠️ Potential issue | 🟡 MinorMissing
sudoon the force-terminate command (same as main docs).Line 76 mentions
kill -9 <PID>withoutsudo. For consistency with the updatedsudo kill <PID>on line 73, this should also usesudo kill -9 <PID>.📝 Suggested fix
-If the process does not exit, use `kill -9 <PID>` to force-terminate it. +If the process does not exit, use `sudo kill -9 <PID>` to force-terminate it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.agents/skills/docs/nemoclaw-reference/references/troubleshooting.md around lines 72 - 77, Update the force-terminate command to use sudo for consistency: change the plain "kill -9 <PID>" mentioned in the troubleshooting text to "sudo kill -9 <PID>" so it matches the earlier "sudo kill <PID>" usage; edit the string in the troubleshooting paragraph (the line containing the kill -9 instruction) accordingly.
🧹 Nitpick comments (2)
bin/lib/onboard.js (1)
323-346: Minor inconsistency inlsofflags between branches.Line 337 uses
sudo lsof -i :${port} -sTCP:LISTEN -P -n(with-P -nflags for numeric output), while line 343 usessudo lsof -i :${port} -sTCP:LISTEN(without-P -n). Consider using consistent flags in both branches for a uniform user experience.🔧 Suggested fix for consistency
} else { lines.push(` Could not identify the process using port ${port}.`); - lines.push(` Run: sudo lsof -i :${port} -sTCP:LISTEN`); + lines.push(` Run: sudo lsof -i :${port} -sTCP:LISTEN -P -n`); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/lib/onboard.js` around lines 323 - 346, The hint builder formatPortConflictHint has inconsistent lsof flags: it uses "sudo lsof -i :${port} -sTCP:LISTEN -P -n" when portCheck.process is known but omits "-P -n" in the fallback branch; update the fallback branch to include the same "-P -n" flags so both branches produce the same lsof command and consistent numeric output (modify the string in the else branch where it currently returns "sudo lsof -i :${port} -sTCP:LISTEN" to include "-P -n")..agents/skills/docs/nemoclaw-workspace/SKILL.md (1)
88-99: Resolve markdownlint MD014 warnings in command examples.
$prompts are used without command output in severalconsoleblocks, which matches the lint warnings and will keep docs lint noisy/failing depending on CI policy.🧹 One clean approach (switch to bash blocks for command-only snippets)
-```console -$ SANDBOX=my-assistant -$ BACKUP_DIR=~/.nemoclaw/backups/$(date +%Y%m%d-%H%M%S) -$ mkdir -p "$BACKUP_DIR" +```bash +SANDBOX=my-assistant +BACKUP_DIR=~/.nemoclaw/backups/$(date +%Y%m%d-%H%M%S) +mkdir -p "$BACKUP_DIR" ... -``` +```Also applies to: 105-115, 133-135, 139-141, 162-163
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.agents/skills/docs/nemoclaw-workspace/SKILL.md around lines 88 - 99, The console code blocks in SKILL.md use ```console with leading "$ " prompts (e.g., lines containing "$ SANDBOX=...", "$ BACKUP_DIR=...", "$ mkdir -p ..."), which triggers markdownlint MD014; change those ```console fences to ```bash and remove the leading "$ " prompt prefixes in each command example (including the other affected blocks around the examples referenced) so the blocks contain only raw commands like SANDBOX=..., BACKUP_DIR=..., mkdir -p ...; ensure all similar occurrences (the blocks around the other ranges noted) are updated consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.agents/skills/docs/nemoclaw-reference/references/commands.md:
- Around line 85-87: Replace the plain-text reference "Back Up and Restore (see
the `nemoclaw-workspace` skill)" with an explicit Markdown link that points to
the exact Back Up and Restore section or the nemoclaw-workspace skill doc;
update the sentence in commands.md (the warning block containing "Destroying a
sandbox..." and the phrase "Back Up and Restore") to use the Markdown link
syntax [Back Up and
Restore](<URL-or-relative-path-to-nemoclaw-workspace#back-up-and-restore>) so
users can click directly to the relevant section, keeping the
`nemoclaw-workspace` inline code mention if desired.
In @.agents/skills/docs/nemoclaw-workspace/SKILL.md:
- Line 3: Update the frontmatter "description" field in SKILL.md to fix grammar
and clarity: replace the current broken string with a concise sentence such as
"How to back up and restore OpenClaw workspace files before destructive
operations; explains what workspace files are, where they live, and how they
persist across sandbox restarts. Use for agents.md, backup/restore, identity.md,
memory.md, nemoclaw, nemoclaw backup, and nemoclaw restore." Ensure you modify
the description key in the file's frontmatter (the "description" field) to this
improved wording.
---
Outside diff comments:
In @.agents/skills/docs/nemoclaw-reference/references/troubleshooting.md:
- Around line 72-77: Update the force-terminate command to use sudo for
consistency: change the plain "kill -9 <PID>" mentioned in the troubleshooting
text to "sudo kill -9 <PID>" so it matches the earlier "sudo kill <PID>" usage;
edit the string in the troubleshooting paragraph (the line containing the kill
-9 instruction) accordingly.
In `@docs/reference/troubleshooting.md`:
- Around line 98-103: The troubleshooting doc shows "kill -9 <PID>" without sudo
while earlier using "sudo kill <PID>"; update the force-terminate example so it
consistently uses sudo (i.e., change "kill -9 <PID>" to "sudo kill -9 <PID>")
near the "sudo lsof -i :18789" and "sudo kill <PID>" lines to ensure commands
will work for root-owned processes.
---
Nitpick comments:
In @.agents/skills/docs/nemoclaw-workspace/SKILL.md:
- Around line 88-99: The console code blocks in SKILL.md use ```console with
leading "$ " prompts (e.g., lines containing "$ SANDBOX=...", "$
BACKUP_DIR=...", "$ mkdir -p ..."), which triggers markdownlint MD014; change
those ```console fences to ```bash and remove the leading "$ " prompt prefixes
in each command example (including the other affected blocks around the examples
referenced) so the blocks contain only raw commands like SANDBOX=...,
BACKUP_DIR=..., mkdir -p ...; ensure all similar occurrences (the blocks around
the other ranges noted) are updated consistently.
In `@bin/lib/onboard.js`:
- Around line 323-346: The hint builder formatPortConflictHint has inconsistent
lsof flags: it uses "sudo lsof -i :${port} -sTCP:LISTEN -P -n" when
portCheck.process is known but omits "-P -n" in the fallback branch; update the
fallback branch to include the same "-P -n" flags so both branches produce the
same lsof command and consistent numeric output (modify the string in the else
branch where it currently returns "sudo lsof -i :${port} -sTCP:LISTEN" to
include "-P -n").
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6a7f58d9-6a96-4dca-9b28-a270fccc0942
📒 Files selected for processing (7)
.agents/skills/docs/nemoclaw-reference/references/commands.md.agents/skills/docs/nemoclaw-reference/references/troubleshooting.md.agents/skills/docs/nemoclaw-workspace/SKILL.md.agents/skills/docs/nemoclaw-workspace/references/workspace-files.mdbin/lib/onboard.jsdocs/reference/troubleshooting.mdtest/onboard.test.js
Comment on lines
+85
to
+87
| > **Warning:** Destroying a sandbox permanently deletes all files inside it, including | ||
| > workspace files (see the `nemoclaw-workspace` skill) (SOUL.md, USER.md, IDENTITY.md, AGENTS.md, MEMORY.md, and daily memory notes). | ||
| > Back up your workspace first by following the instructions at Back Up and Restore (see the `nemoclaw-workspace` skill). |
Contributor
There was a problem hiding this comment.
Make “Back Up and Restore” an explicit Markdown link.
The warning is useful, but the current plain-text reference is harder to navigate than a direct link to the exact section/skill doc.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.agents/skills/docs/nemoclaw-reference/references/commands.md around lines
85 - 87, Replace the plain-text reference "Back Up and Restore (see the
`nemoclaw-workspace` skill)" with an explicit Markdown link that points to the
exact Back Up and Restore section or the nemoclaw-workspace skill doc; update
the sentence in commands.md (the warning block containing "Destroying a
sandbox..." and the phrase "Back Up and Restore") to use the Markdown link
syntax [Back Up and
Restore](<URL-or-relative-path-to-nemoclaw-workspace#back-up-and-restore>) so
users can click directly to the relevant section, keeping the
`nemoclaw-workspace` inline code mention if desired.
| @@ -0,0 +1,169 @@ | |||
| --- | |||
| name: nemoclaw-workspace | |||
| description: Hows to back up and restore OpenClaw workspace files before destructive operations. Also covers whats workspace files are, where they live, and how they persist across sandbox restarts. Use when agents.md, back restore workspace files, backup, identity.md, memory.md, nemoclaw, nemoclaw backup, nemoclaw restore. | |||
Contributor
There was a problem hiding this comment.
Fix frontmatter description grammar and phrasing.
The description string has multiple typos/awkward phrases (Hows, whats, back restore workspace files) that reduce discoverability and polish in generated docs.
✏️ Proposed wording
-description: Hows to back up and restore OpenClaw workspace files before destructive operations. Also covers whats workspace files are, where they live, and how they persist across sandbox restarts. Use when agents.md, back restore workspace files, backup, identity.md, memory.md, nemoclaw, nemoclaw backup, nemoclaw restore.
+description: How to back up and restore OpenClaw workspace files before destructive operations. Also covers what workspace files are, where they live, and how they persist across sandbox restarts. Use when querying AGENTS.md, backup/restore workspace files, IDENTITY.md, MEMORY.md, nemoclaw, nemoclaw backup, or nemoclaw restore.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.agents/skills/docs/nemoclaw-workspace/SKILL.md at line 3, Update the
frontmatter "description" field in SKILL.md to fix grammar and clarity: replace
the current broken string with a concise sentence such as "How to back up and
restore OpenClaw workspace files before destructive operations; explains what
workspace files are, where they live, and how they persist across sandbox
restarts. Use for agents.md, backup/restore, identity.md, memory.md, nemoclaw,
nemoclaw backup, and nemoclaw restore." Ensure you modify the description key in
the file's frontmatter (the "description" field) to this improved wording.
cjagwani
added a commit
to kagura-agent/NemoClaw
that referenced
this pull request
Jun 3, 2026
…IDIA#734/NVIDIA#735) CodeQL flagged the symlink-target paths in the two symlink-skip tests as insecure temp file creation (predictable `os.tmpdir() + pid` paths let a coresident attacker pre-create the target). Switch both to the existing `makeTempDir()` helper, which uses `mkdtempSync` for an unguessable directory name. Cleanup also simplifies to the afterEach hook since both temp dirs are now tracked in `tmpDirs`. Tests still pass (16/16). Signed-off-by: Charan Jagwani <cjagwani@nvidia.com>
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
2 participants
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
Non-root users cannot see root-owned listeners via
lsof, so the port-conflict hints shown during onboarding silently fail. This PR addssudoto thelsofandkillhints displayed to users, and updates the troubleshooting docs to match.The programmatic
lsofcall inpreflight.jsis intentionally left withoutsudo— adding it there would trigger an interactive password prompt during onboarding, which is a worse UX. The current approach (try without sudo, fall through gracefully, then suggest the user run it manually with sudo) is the right tradeoff.Related Issue
Fixes #726
Changes
formatPortConflictHint()helper inonboard.jsfor testabilitysudoprefix tolsofandkillcommands in the user-facing hints (both "unknown process" and "known process without PID" paths)docs/reference/troubleshooting.mdwithsudocommands.agents/skills/docs/viadocs-to-skills.pyType of Change
Testing
npm testpasses.test/onboard.test.jscovering allformatPortConflictHint()branches: unknown process, known with PID, known without PID.Checklist
General
Code Changes
Doc Changes
Summary by CodeRabbit
Documentation
Bug Fixes
Tests