Skip to content

fix(test): restore port-resolution assertions in nimStatusByName tests#3183

Merged
jyaunches merged 1 commit into
mainfrom
fix/issue-1280-nim-port-resolution-assertions
May 7, 2026
Merged

fix(test): restore port-resolution assertions in nimStatusByName tests#3183
jyaunches merged 1 commit into
mainfrom
fix/issue-1280-nim-port-resolution-assertions

Conversation

@laitingsheng

@laitingsheng laitingsheng commented May 7, 2026 •
edited by coderabbitai Bot
Loading

Copy link
Copy Markdown
Contributor

Summary

Restore the curl-URL assertions in two nimStatusByName tests so they actually verify the port-resolution logic instead of only the boolean health result.

Related Issue

Closes #1280

Changes

  • src/lib/nim.test.ts:
    • "uses published docker port when no port is provided" — assert the curl call used http://127.0.0.1:9000/v1/models.
    • "falls back to 8000 when docker port lookup fails" — assert the curl call used http://127.0.0.1:8000/v1/models.

Type of Change

  • 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

  • npx prek run --all-files passes
  • npm test passes
  • Tests added or updated for new or changed behavior
  • No secrets, API keys, or credentials committed
  • Docs updated for user-facing behavior changes
  • make docs builds without warnings (doc changes only)
  • Doc pages follow the style guide (doc changes only)
  • New doc pages include SPDX header and frontmatter (new pages only)

Signed-off-by: Tinson Lai tinsonl@nvidia.com

Summary by CodeRabbit

  • Tests
    • Enhanced test coverage for health-probe verification scenarios to ensure proper command execution validation.

@laitingsheng
fix(test): restore port-resolution assertions in nimStatusByName tests
c968e89 
Assert that the curl health-check URL actually used the resolved port
in the published-docker-port and 8000-fallback branches, so the tests
verify the port-resolution logic and not just the boolean health
result.

Closes #1280

Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
@coderabbitai

coderabbitai Bot commented May 7, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Review Change Stack
No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: b38e4e3e-65a5-4a2f-ba57-317cb2f19d3a

📥 Commits

Reviewing files that changed from the base of the PR and between 2c3a392 and c968e89.

📒 Files selected for processing (1)
  • src/lib/nim.test.ts
📝 Walkthrough

Walkthrough

This PR restores health-probe URL assertions in the nimStatusByName test suite. Two test scenarios now verify that curl commands target the correct port-resolved localhost URLs using substring matching via commands.some(...), confirming the port-resolution branching logic is exercised correctly.

Changes

Health Probe URL Assertions

Layer / File(s) Summary
Health Probe URL Assertions
src/lib/nim.test.ts		 "uses published docker port when no port is provided" test asserts that at least one recorded command contains http://127.0.0.1:9000/v1/models using commands.some(...). "falls back to 8000 when docker port lookup fails" test similarly asserts presence of http://127.0.0.1:8000/v1/models among recorded commands while preserving docker port invocation verification.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

Poem

A rabbit hops through test assertions bright,
Where curl commands now target ports just right—
Nine-thousand here, eight-thousand there,
Port resolution answers to our care! 🐰✓

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: restoring port-resolution assertions in specific tests.
Linked Issues check ✅ Passed The PR addresses the requirements from issue #1280 by restoring curl URL assertions for both port-resolution test scenarios.
Out of Scope Changes check ✅ Passed All changes are directly related to restoring assertions in the nimStatusByName tests as specified in issue #1280.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/issue-1280-nim-port-resolution-assertions

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 Microsoft Presidio Analyzer (2.2.362)
src/lib/nim.test.ts

Microsoft Presidio Analyzer failed to scan this file

Comment @coderabbitai help to get the list of available commands and usage tips.

@jyaunches jyaunches merged commit 54ea287 into main May 7, 2026
20 checks passed
@miyoungc miyoungc mentioned this pull request May 7, 2026
Merged
jyaunches pushed a commit that referenced this pull request May 8, 2026
@miyoungc @cursoragent
docs: prepare 0.0.37 release docs (#3215)
37050a6 
## Summary
- Bump the docs release metadata to `0.0.37`.
- Document release-prep updates for messaging policy presets, sandbox
runtime utilities, and the GPU CDI troubleshooting path.
- Refresh generated `nemoclaw-user-*` skills from the updated docs.

## Source summary
- #3159 -> `docs/reference/troubleshooting.md`: Documents the GPU CDI
preflight warning and remediation for `nvidia.com/gpu=all` gateway start
failures.
- #2415 -> `docs/reference/network-policies.md`,
`docs/manage-sandboxes/messaging-channels.md`,
`docs/network-policy/customize-network-policy.md`: Clarifies that
Telegram, Discord, and Slack egress comes from opt-in messaging presets,
not the baseline policy.
- #3091 -> `docs/deployment/sandbox-hardening.md`,
`docs/network-policy/customize-network-policy.md`: Documents the
retained sandbox utilities `vi`, `jq`, and `dos2unix` while keeping
host-side policy files as the durable source of truth.

## Test plan
- `python3 scripts/docs-to-skills.py docs/ .agents/skills/ --prefix
nemoclaw-user`
- `make docs`
- `npm run build:cli`
- `npm run typecheck:cli`
- Commit and pre-push hooks: markdownlint, docs-to-skills verification,
gitleaks, commitlint, CLI typecheck

## Skipped
- #3193 and #3191 matched `docs/.docs-skip` entries for experimental
shields/config paths.
- #3200 and #3183 were test-only fixes.
- #3189 and #3163 were internal documentation/refactor changes with no
public docs impact.

Made with [Cursor](https://cursor.com)

<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit

* **Documentation**
* Clarified which utilities remain in the sandbox runtime for
lightweight inspection and cleanup
* Noted that messaging endpoints (Discord, Slack, Telegram) are not in
the baseline policy and that channel presets are applied during
onboarding
  * Added GPU passthrough troubleshooting for gateway startup
* Updated release/version bump and release-prep workflow guidance,
including Discord preset description updates
<!-- end of auto-generated comment: release notes by coderabbit.ai -->

---------

Co-authored-by: Cursor <cursoragent@cursor.com>
@wscurran wscurran added the bug-fix PR fixes a bug or regression label Jun 8, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jyaunches jyaunches jyaunches approved these changes

Assignees

No one assigned

Labels

bug-fix PR fixes a bug or regression

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

fix(test): restore port-resolution assertions in nimStatusByName tests

3 participants

@laitingsheng @jyaunches @wscurran