Skip to content

fix(cli): reject invalid node run port#84307

Merged
giodl73-repo merged 1 commit into
mainfrom
fix-node-run-invalid-port-83923
May 21, 2026
Merged

fix(cli): reject invalid node run port#84307
giodl73-repo merged 1 commit into
mainfrom
fix-node-run-invalid-port-83923

Conversation

@giodl73-repo

@giodl73-repo giodl73-repo commented May 19, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Reject explicit invalid openclaw node run --port values instead of falling back to the configured or default port.
  • Add focused node CLI registration coverage for invalid, valid, and omitted --port values.
  • Add the user-visible fix to the changelog.

Fixes #83923.

Verification

  • git diff --check HEAD~1..HEAD -- CHANGELOG.md src/cli/node-cli/register.ts src/cli/node-cli/register.test.ts
  • Crabbox AWS cbx_b19076fdaf79, run run_26642c3eee98: regression failed before the source patch, then node scripts/run-vitest.mjs src/cli/node-cli/register.test.ts -- --reporter=verbose passed after the patch (4/4).
  • WSL Ubuntu 24.04 checkout at PR head c6a8ad803a48e4f19577b3c9ed9942fedfd707f5: timeout 10s node scripts/run-node.mjs node run --port abc exited before starting the node host.

Behavior addressed: openclaw node run --port abc now reports Invalid --port and exits before starting the node host.
Real environment tested: AWS Crabbox Linux lease cbx_b19076fdaf79, run run_26642c3eee98; WSL Ubuntu 24.04 checkout at PR head c6a8ad803a48e4f19577b3c9ed9942fedfd707f5.
Exact steps or command run after this patch: node scripts/run-vitest.mjs src/cli/node-cli/register.test.ts -- --reporter=verbose; OPENCLAW_HOME="$(mktemp -d)" timeout 10s node scripts/run-node.mjs node run --port abc.
Evidence after fix: focused Vitest passed with 4 passed; scoped git diff --check passed; the foreground CLI proof returned exit_status=1, stderr Invalid --port. Use a port number from 1 to 65535, for example 18789., and pgrep -af 'openclaw node run|node-host|src/node-host|run-node.mjs node run' printed no matching process afterward.
Observed result after fix: invalid explicit --port fails fast with the user-facing validation error and does not leave a node host running; valid explicit and omitted ports still dispatch with the expected gateway port in focused registration coverage.
What was not tested: no live gateway connection was attempted, because the invalid explicit port path should exit before connecting.

@openclaw-barnacle openclaw-barnacle Bot added cli CLI command changes size: S maintainer Maintainer-authored PR labels May 19, 2026
@clawsweeper

clawsweeper Bot commented May 19, 2026

Copy link
Copy Markdown
Contributor

Codex review: needs maintainer review before merge.

Workflow note: Future ClawSweeper reviews update this same comment in place.

How this review workflow works
  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

Summary
The branch changes openclaw node run --port handling to reject explicit invalid ports, adds registration tests for invalid, valid, and omitted port values, and adds a changelog entry.

Reproducibility: yes. Source inspection on current main shows invalid explicit opts.port becomes null and is replaced by the configured/default port before runNodeHost; the PR body supplies matching after-fix live CLI output, though I did not run it locally because this review is read-only.

PR rating
Overall: 🦞 diamond lobster
Proof: 🦞 diamond lobster
Patch quality: 🦞 diamond lobster
Summary: Focused patch with real CLI proof, targeted coverage, and no blocking findings; only maintainer-owned compatibility acceptance remains.

What the crustacean ranks mean
  • 🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
  • 🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
  • 🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
  • 🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
  • 🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
  • 🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
  • 🌊 off-meta tidepool: rating does not apply to this item.

Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

Real behavior proof
Sufficient (live_output): The PR body includes after-fix real CLI proof from a WSL checkout showing the invalid-port command exits with the expected error and leaves no node-host process running.

Risk before merge

  • Merging intentionally changes existing scripts with an invalid explicit node run --port value from fallback startup to a visible error and exit; maintainers need to accept that compatibility impact.

Maintainer options:

  1. Accept fail-fast invalid ports (recommended)
    Merge after maintainers accept that explicit invalid node run --port values now stop startup instead of silently using config or default fallback.
  2. Ask for compatibility-preserving behavior
    If silent fallback must remain for existing scripts, ask for a different design such as warning-only behavior or an explicit strict mode before merging.

Next step before merge
No repair PR is needed; maintainers need to accept the compatibility-sensitive fail-fast behavior and handle the protected-label merge path.

Security
Cleared: The diff is limited to CLI validation, colocated tests, and changelog text with no dependency, workflow, secret, package-resolution, or supply-chain surface changes.

Review details

Best possible solution:

Land the focused fail-fast validation after maintainer acceptance of the compatibility impact, then let the merge close #83923.

Do we have a high-confidence way to reproduce the issue?

Yes. Source inspection on current main shows invalid explicit opts.port becomes null and is replaced by the configured/default port before runNodeHost; the PR body supplies matching after-fix live CLI output, though I did not run it locally because this review is read-only.

Is this the best way to solve the issue?

Yes. The patch is the narrow maintainable fix: reject only explicit invalid --port values, preserve omitted-port fallback, reuse the existing invalid-port message, and add focused regression coverage.

Label justifications:

  • P2: This is a normal CLI bugfix with limited blast radius around openclaw node run --port handling.
  • merge-risk: 🚨 compatibility: The PR deliberately changes invalid explicit --port inputs from fallback startup to fail-fast exit, which can break scripts relying on the old fallback.
  • rating: 🦞 diamond lobster: Current PR rating is 🦞 diamond lobster because proof is 🦞 diamond lobster, patch quality is 🦞 diamond lobster, and Focused patch with real CLI proof, targeted coverage, and no blocking findings; only maintainer-owned compatibility acceptance remains.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (live_output): The PR body includes after-fix real CLI proof from a WSL checkout showing the invalid-port command exits with the expected error and leaves no node-host process running.
  • proof: sufficient: Contributor real behavior proof is sufficient. The PR body includes after-fix real CLI proof from a WSL checkout showing the invalid-port command exits with the expected error and leaves no node-host process running.

What I checked:

  • Current-main bug path: Current main still uses parsePortWithFallback, so an invalid explicit opts.port that parses to null is replaced by the configured/default port before runNodeHost is called. (src/cli/node-cli/register.ts:18, 7f4462e5c06d)
  • Adjacent CLI contract: node install already treats an explicitly invalid port as a CLI error via formatInvalidPortOption("--port"), which supports the PR's consistency argument. (src/cli/node-cli/daemon.ts:82, 7f4462e5c06d)
  • Shared parser contract: parsePort returns null for nullish, non-integer, non-finite, non-numeric, zero/negative, and values above 65535, so the PR can distinguish omitted --port from invalid explicit values. (src/cli/shared/parse-port.ts:9, 7f4462e5c06d)
  • PR implementation: The PR replaces the silent fallback helper with an omitted-only fallback, emits Invalid --port, exits, and returns before runNodeHost when the explicit port is invalid. (src/cli/node-cli/register.ts:58, c6a8ad803a48)
  • Regression coverage and formatting proof: The PR adds focused tests for invalid, valid, and omitted node run --port paths; the reviewed diff also passes git diff --check for the touched files. (src/cli/node-cli/register.test.ts:69, c6a8ad803a48)
  • Discussion and proof context: The PR body and follow-up comment provide after-fix WSL CLI proof: node run --port abc exits with Invalid --port, exit status 1, and no matching node-host process afterward; labels include proof: sufficient, maintainer, and compatibility merge risk. (c6a8ad803a48)

Likely related people:

  • Shakker: Current checkout blame and git log --follow route the node CLI registration file and shared port parser surface through commit b248b48, so this is the strongest local history signal for the affected path. (role: recent area contributor; confidence: medium; commits: b248b4816b0d; files: src/cli/node-cli/register.ts, src/cli/shared/parse-port.ts)

Codex review notes: model gpt-5.5, reasoning high; reviewed against 7f4462e5c06d.

@clawsweeper clawsweeper Bot added rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. P2 Normal backlog priority with limited blast radius. merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. labels May 19, 2026
@Takhoffman

Copy link
Copy Markdown
Contributor

@clawsweeper hatch

@clawsweeper

clawsweeper Bot commented May 19, 2026

Copy link
Copy Markdown
Contributor

🦞👀
ClawSweeper PR egg hatch requested.

I queued a comment sync for this PR. If the egg is hatchable, ClawSweeper will generate the image once and update the existing review comment.
Action: PR egg hatch queued (workflow sweep.yml, event repository_dispatch).
The ASCII egg stays as the fallback.

@clawsweeper

clawsweeper Bot commented May 19, 2026

Copy link
Copy Markdown
Contributor

ClawSweeper could not hatch this PR egg yet.

Reason: there is no current durable ClawSweeper review record for this PR, so there is no PR egg state record to update.
Ask for @clawsweeper re-review first, then retry @clawsweeper hatch after the ClawSweeper review comment appears.

@clawsweeper clawsweeper Bot added rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. and removed rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. labels May 19, 2026
@giodl73-repo giodl73-repo force-pushed the fix-node-run-invalid-port-83923 branch from c9cbddd to b354f03 Compare May 20, 2026 21:07
@clawsweeper

clawsweeper Bot commented May 20, 2026

Copy link
Copy Markdown
Contributor

ClawSweeper PR egg

✨ Hatched: 🥚 common Pearl Signal Puff

Hatch command

Comment @clawsweeper hatch when this PR is hatchable.

Hatchability rules:

  • Merged PRs are hatchable.
  • Open PRs are hatchable when they are status: 👀 ready for maintainer look, status: 🚀 automerge armed, or labeled clawsweeper:automerge.
  • Closed unmerged PRs are hatchable only when one of those hatchable labels is still present in the durable record.

Rarity: 🥚 common.
Trait: sparkles near resolved comments.
Image traits: location diff observatory; accessory green check lantern; palette violet, aqua, and starlight; mood mischievous; pose leaning over a miniature review desk; shell frosted glass shell; lighting warm desk-lamp glow; background soft code-shaped tiles.
Share on X: post this hatch
Copy: My PR egg hatched a 🥚 common Pearl Signal Puff in ClawSweeper.

What is this egg doing here?
  • Eggs appear after the PR passes real-behavior proof. It is here for vibes, not verdicts: it does not change labels, ratings, merge decisions, or automation.
  • The shell reacts to review momentum: open follow-up work warms it up, re-review makes it wobble, and a clean final review lets it hatch.
  • Hatchability usually comes from sufficient real-behavior proof, no blocking P0/P1/P2 findings, no security attention needed, and clean correctness. A merged PR is already final, so merge makes the egg hatchable independently.
  • The hatch is seeded from this repository and PR number, so the same PR keeps the same creature; the reviewed head SHA can only change safe visual details.
  • Rarity is just collectible sparkle: 🥚 common, 🌱 uncommon, 💎 rare, ✨ glimmer, and 🌈 legendary.

@giodl73-repo giodl73-repo force-pushed the fix-node-run-invalid-port-83923 branch from b354f03 to b98b16b Compare May 21, 2026 05:13
@giodl73-repo giodl73-repo force-pushed the fix-node-run-invalid-port-83923 branch from b98b16b to c6a8ad8 Compare May 21, 2026 05:15
@giodl73-repo

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented May 21, 2026

Copy link
Copy Markdown
Contributor

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

Re-review progress:

@giodl73-repo

Copy link
Copy Markdown
Contributor Author

Added the requested real CLI behavior proof to the PR body: the WSL checkout at c6a8ad803a48e4f19577b3c9ed9942fedfd707f5 now shows node scripts/run-node.mjs node run --port abc exits with Invalid --port and leaves no matching node-host process.

@clawsweeper re-review

@clawsweeper clawsweeper Bot added proof: sufficient ClawSweeper judged the real behavior proof convincing. rating: 🦞 diamond lobster Very strong PR readiness with only minor maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. and removed rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. labels May 21, 2026
@giodl73-repo giodl73-repo merged commit 8961eae into main May 21, 2026
131 of 135 checks passed
@giodl73-repo giodl73-repo deleted the fix-node-run-invalid-port-83923 branch May 21, 2026 20:47
SebTardif pushed a commit to SebTardif/openclaw that referenced this pull request May 24, 2026
Co-authored-by: Gio Della-Libera <giodl@microsoft.com>
SebTardif pushed a commit to SebTardif/openclaw that referenced this pull request May 24, 2026
Co-authored-by: Gio Della-Libera <giodl@microsoft.com>
SebTardif pushed a commit to SebTardif/openclaw that referenced this pull request May 24, 2026
Co-authored-by: Gio Della-Libera <giodl@microsoft.com>
github-actions Bot pushed a commit to Desicool/openclaw that referenced this pull request May 24, 2026
Co-authored-by: Gio Della-Libera <giodl@microsoft.com>
galiniliev pushed a commit to galiniliev/openclaw that referenced this pull request May 25, 2026
Co-authored-by: Gio Della-Libera <giodl@microsoft.com>
SebTardif pushed a commit to SebTardif/openclaw that referenced this pull request May 26, 2026
Co-authored-by: Gio Della-Libera <giodl@microsoft.com>
SebTardif pushed a commit to SebTardif/openclaw that referenced this pull request May 26, 2026
Co-authored-by: Gio Della-Libera <giodl@microsoft.com>
SebTardif pushed a commit to SebTardif/openclaw that referenced this pull request May 26, 2026
Co-authored-by: Gio Della-Libera <giodl@microsoft.com>
jameslcowan pushed a commit to jameslcowan/openclaw that referenced this pull request Jun 2, 2026
Co-authored-by: Gio Della-Libera <giodl@microsoft.com>
SYU8384 pushed a commit to SYU8384/openclaw that referenced this pull request Jun 3, 2026
Co-authored-by: Gio Della-Libera <giodl@microsoft.com>
sablehead pushed a commit to sablehead/openclaw that referenced this pull request Jun 10, 2026
Co-authored-by: Gio Della-Libera <giodl@microsoft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cli CLI command changes maintainer Maintainer-authored PR merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. P2 Normal backlog priority with limited blast radius. proof: sufficient ClawSweeper judged the real behavior proof convincing. rating: 🦞 diamond lobster Very strong PR readiness with only minor maintainer review expected. size: S status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Silent port fallback on invalid --port in node run command

2 participants