fix(cli): honor port 0 in checkPortAvailable - #1304
Conversation
📝 WalkthroughWalkthroughThe Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~2 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@cv I fixed the PR issue and this should be good now. Updated the PR body sign-off format, and checks are now passing on my side (npx prek run --all-files, npm test, make docs). Please take another look when you have a moment. |
|
@cv I see that you enabled auto merge I am still getting this workflow requires approval from a maintainer. Please take another look when you have a moment. |
…ions API Two upstream bug fixes backported from NVIDIA/NemoClaw: - preflight.js: `port || 18789` → `port ?? 18789` so port=0 (OS-assigned) is not incorrectly replaced by the default (upstream NVIDIA#1304) - onboard.js: Force openai-completions for Ollama regardless of probe result. Ollama 0.19.0 /v1/responses endpoint passes probe but produces malformed tool calls in the TUI. Matches existing vLLM/NIM override pattern. Update test assertion that was asserting the buggy value (upstream NVIDIA#1315)
<!-- markdownlint-disable MD041 --> ## Summary This PR fixes a bug in CLI preflight port handling by preserving `port = 0` instead of incorrectly defaulting it to `18789`. In Node.js networking, `0` is a valid value that asks the OS to assign a temporary available port. The fix changes null/default logic to keep valid explicit inputs while still applying the default when no port is provided. ## Changes - Updated `checkPortAvailable` in `src/lib/preflight.ts`: - Before: `const p = port || 18789;` - After: `const p = port ?? 18789;` - This preserves: - `undefined`/`null` -> default `18789` - `0` -> stays `0` (valid OS-assigned temporary available port request) - any explicit non-null value -> unchanged - No CLI command surface changes. - No docs changes required. ## Type of Change - [x] Code change for a new feature, bug fix, or refactor. - [ ] Code change with doc updates. - [ ] Doc only. Prose changes without code sample modifications. - [ ] Doc only. Includes code sample changes. ## Testing - [x] `npx prek run --all-files` passes (or equivalently `make check`). - [x] `npm test` passes. - [x] `make docs` builds without warnings. (for doc-only changes) ## Checklist ### General - [x] I have read and followed the [contributing guide](https://github.com/NVIDIA/NemoClaw/blob/main/CONTRIBUTING.md). - [ ] I have read and followed the [style guide](https://github.com/NVIDIA/NemoClaw/blob/main/docs/CONTRIBUTING.md). (for doc-only changes) ### Code Changes - [x] Formatters applied — `npx prek run --all-files` auto-fixes formatting (or `make format` for targeted runs). - [x] Tests added or updated for new or changed behavior. - [x] No secrets, API keys, or credentials committed. - [x] Doc pages updated for any user-facing behavior changes (new commands, changed defaults, new features, bug fixes that contradict existing docs). ### Doc Changes - [ ] Follows the [style guide](https://github.com/NVIDIA/NemoClaw/blob/main/docs/CONTRIBUTING.md). Try running the `update-docs` agent skill to draft changes while complying with the style guide. For example, prompt your agent with "`/update-docs` catch up the docs for the new changes I made in this PR." - [ ] New pages include SPDX license header and frontmatter, if creating a new page. - [ ] Cross-references and links verified. --- Signed-off-by: Revant Patel <revant.h.patel@gmail.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Corrected port configuration handling to properly respect explicitly set values instead of defaulting to the standard port. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Carlos Villela <cvillela@nvidia.com>
<!-- markdownlint-disable MD041 --> ## Summary This PR fixes a bug in CLI preflight port handling by preserving `port = 0` instead of incorrectly defaulting it to `18789`. In Node.js networking, `0` is a valid value that asks the OS to assign a temporary available port. The fix changes null/default logic to keep valid explicit inputs while still applying the default when no port is provided. ## Changes - Updated `checkPortAvailable` in `src/lib/preflight.ts`: - Before: `const p = port || 18789;` - After: `const p = port ?? 18789;` - This preserves: - `undefined`/`null` -> default `18789` - `0` -> stays `0` (valid OS-assigned temporary available port request) - any explicit non-null value -> unchanged - No CLI command surface changes. - No docs changes required. ## Type of Change - [x] Code change for a new feature, bug fix, or refactor. - [ ] Code change with doc updates. - [ ] Doc only. Prose changes without code sample modifications. - [ ] Doc only. Includes code sample changes. ## Testing - [x] `npx prek run --all-files` passes (or equivalently `make check`). - [x] `npm test` passes. - [x] `make docs` builds without warnings. (for doc-only changes) ## Checklist ### General - [x] I have read and followed the [contributing guide](https://github.com/NVIDIA/NemoClaw/blob/main/CONTRIBUTING.md). - [ ] I have read and followed the [style guide](https://github.com/NVIDIA/NemoClaw/blob/main/docs/CONTRIBUTING.md). (for doc-only changes) ### Code Changes - [x] Formatters applied — `npx prek run --all-files` auto-fixes formatting (or `make format` for targeted runs). - [x] Tests added or updated for new or changed behavior. - [x] No secrets, API keys, or credentials committed. - [x] Doc pages updated for any user-facing behavior changes (new commands, changed defaults, new features, bug fixes that contradict existing docs). ### Doc Changes - [ ] Follows the [style guide](https://github.com/NVIDIA/NemoClaw/blob/main/docs/CONTRIBUTING.md). Try running the `update-docs` agent skill to draft changes while complying with the style guide. For example, prompt your agent with "`/update-docs` catch up the docs for the new changes I made in this PR." - [ ] New pages include SPDX license header and frontmatter, if creating a new page. - [ ] Cross-references and links verified. --- Signed-off-by: Revant Patel <revant.h.patel@gmail.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Corrected port configuration handling to properly respect explicitly set values instead of defaulting to the standard port. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Carlos Villela <cvillela@nvidia.com>
Summary
This PR fixes a bug in CLI preflight port handling by preserving
port = 0instead of incorrectly defaulting it to18789.In Node.js networking,
0is a valid value that asks the OS to assign a temporary available port.The fix changes null/default logic to keep valid explicit inputs while still applying the default when no port is provided.
Changes
checkPortAvailableinsrc/lib/preflight.ts:const p = port || 18789;const p = port ?? 18789;undefined/null-> default187890-> stays0(valid OS-assigned temporary available port request)Type of Change
Testing
npx prek run --all-filespasses (or equivalentlymake check).npm testpasses.make docsbuilds without warnings. (for doc-only changes)Checklist
General
Code Changes
npx prek run --all-filesauto-fixes formatting (ormake formatfor targeted runs).Doc Changes
update-docsagent skill to draft changes while complying with the style guide. For example, prompt your agent with "/update-docscatch up the docs for the new changes I made in this PR."Signed-off-by: Revant Patel revant.h.patel@gmail.com
Summary by CodeRabbit