fix: resolve silent output and remove nc dependency#3669
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds Node type refs and guarded filesystem traversal, guards skills frontmatter tests when generated skills are absent, replaces an external nc port probe with an inline Node net probe invoked via process.execPath, makes the Ollama proxy startup timeout configurable, updates tests to match the new probe, and adds hadolint download retries. ChangesError Resilience Improvements
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/skills-frontmatter.test.ts (1)
48-53: ⚡ Quick winRemove the duplicated
skillsRootexistence guard.This block is a duplicate of Lines 41-46 and is redundant/unreachable in practice.
Proposed cleanup
- if (!fs.existsSync(skillsRoot)) { - it("no generated skills present (CI) — skipping", () => { - expect(true).toBe(true); - }); - return; - }🤖 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 `@test/skills-frontmatter.test.ts` around lines 48 - 53, The duplicate existence guard for skillsRoot (the block starting with if (!fs.existsSync(skillsRoot)) { it("no generated skills present (CI) — skipping", ...); return; }) should be removed because it repeats the earlier guard; delete this second block so only the initial check for skillsRoot remains in the test file (keep the first occurrence that guards the test suite and remove the redundant copy to avoid unreachable/duplicated logic).
🤖 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.
Nitpick comments:
In `@test/skills-frontmatter.test.ts`:
- Around line 48-53: The duplicate existence guard for skillsRoot (the block
starting with if (!fs.existsSync(skillsRoot)) { it("no generated skills present
(CI) — skipping", ...); return; }) should be removed because it repeats the
earlier guard; delete this second block so only the initial check for skillsRoot
remains in the test file (keep the first occurrence that guards the test suite
and remove the redundant copy to avoid unreachable/duplicated logic).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 829d456c-baa2-4c7c-8cf7-8519bd4d2aa9
📒 Files selected for processing (2)
scripts/find-source-shape-tests.tstest/skills-frontmatter.test.ts
Iamkewl
force-pushed
the
fix/issue-3665-pr
branch
from
d8085d2 to
0a48cc5
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/lib/core/wait.ts (1)
52-55: 💤 Low valueConsider aligning socket timeout with the polling interval.
The socket timeout is hardcoded to 1000ms, but the probe is invoked every 200ms (line 64). Each probe attempt can block for up to 1 second, which reduces the effective polling frequency from the expected 5 Hz down to ~1 Hz. This means fewer probe attempts occur within the overall timeout window.
For tighter polling behavior, consider reducing the socket timeout to 200-500ms to match the polling interval.
🤖 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/core/wait.ts` around lines 52 - 55, The socket probe currently uses a hardcoded 1000ms timeout (socket.setTimeout(1000)), which blocks each probe and undermines the 200ms polling interval; change socket.setTimeout to match the polling interval (e.g., 200–500ms) or use the existing poll interval constant/variable instead of 1000 so socket.setTimeout(...) aligns with the probe frequency; update the code around socket.setTimeout and its related handlers (socket.once('connect'), socket.once('timeout'), socket.once('error')) to use that interval.
🤖 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.
Nitpick comments:
In `@src/lib/core/wait.ts`:
- Around line 52-55: The socket probe currently uses a hardcoded 1000ms timeout
(socket.setTimeout(1000)), which blocks each probe and undermines the 200ms
polling interval; change socket.setTimeout to match the polling interval (e.g.,
200–500ms) or use the existing poll interval constant/variable instead of 1000
so socket.setTimeout(...) aligns with the probe frequency; update the code
around socket.setTimeout and its related handlers (socket.once('connect'),
socket.once('timeout'), socket.once('error')) to use that interval.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: ab07f6d6-012c-433e-91fc-6cd9def6555e
📒 Files selected for processing (1)
src/lib/core/wait.ts
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 @.github/actions/basic-checks/action.yaml:
- Around line 22-27: The checksum verification can silently succeed when both
EXPECTED and ACTUAL are empty; update the hadolint download loop to set and
check an explicit verified flag (e.g., verified=1) only when both EXPECTED and
ACTUAL are populated and match, and after the loop replace the current guard
that compares EXPECTED and ACTUAL with a conditional that fails if verified is
not set or if EXPECTED is empty (use [ -n "$EXPECTED" ] and the verified flag)
so the script errors out on missing checksum or failed verification; adjust the
loop body that currently computes EXPECTED and ACTUAL to assign those variables
conditionally and set verified when the hash check passes.
In `@src/lib/inference/ollama/proxy.ts`:
- Around line 213-214: The parsed proxyStartupTimeout can be NaN if
process.env.NEMOCLAW_OLLAMA_PROXY_STARTUP_TIMEOUT is non-numeric, causing
waitForPort(OLLAMA_PROXY_PORT, proxyStartupTimeout) to behave incorrectly;
update the code around Number.parseInt(...) to validate the parsed value (use
Number.parseInt(..., 10) result checked with Number.isFinite/Number.isNaN or
isNaN) and fall back to the default 10 (or clamp to a positive integer) when the
env value is invalid or <= 0 before calling waitForPort; ensure you keep the
existing symbols proxyStartupTimeout, OLLAMA_PROXY_PORT and waitForPort when
making the change.
🪄 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: c1dd8a3d-a52d-40bb-aa93-393505d1086b
📒 Files selected for processing (6)
.github/actions/basic-checks/action.yamlci/env-var-doc-allowlist.jsonscripts/find-source-shape-tests.tssrc/lib/core/wait.tssrc/lib/inference/ollama/proxy.tstest/onboard-selection.test.ts
✅ Files skipped from review due to trivial changes (1)
- ci/env-var-doc-allowlist.json
🚧 Files skipped from review as they are similar to previous changes (2)
- src/lib/core/wait.ts
- scripts/find-source-shape-tests.ts
wscurran
added
fix
NemoClaw CLI
|
✨ Thanks for submitting this detailed PR to fix the silent output issue and remove the dependency on the Related open issues: |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Iamkewl
force-pushed
the
fix/issue-3665-pr
branch
from
0ac306d to
37f8eaa
Compare
2 participants
Summary
Replaces the platform-dependent
ncreadiness probe with an inline Node TCP probe, aligns timeouts, hardens tests to avoid CI flakes, and adds CI reliability fixes.Related Issue
Fixes #3665
Changes
ncwith Node-based TCP probe in src/lib/core/wait.ts: runs viaspawnSync(process.execPath, ["-e", probeScript, port]), alignssocket.setTimeoutwithpollIntervalMs, makesdone()idempotent (removes listeners before close), preferssocket.end()and enforces a child timeout.NEMOCLAW_OLLAMA_PROXY_STARTUP_TIMEOUT(default10) and used it in src/lib/inference/ollama/proxy.ts; added to ci/env-var-doc-allowlist.json.process.execPathprobe invocations; added loopback integration tests forwaitForPortin test/wait.test.ts; changed test/skills-frontmatter.test.ts to fail unlessNEMOCLAW_TEST_ALLOW_MISSING_GENERATED_SKILLS=1.scripts/find-source-shape-tests.tsto ignore ENOENT/EACCES while walking directories.Type of Change
Verification
npx prek run --all-filespassesnpm testpassesmake docsbuilds without warnings (doc changes only)Signed-off-by: Your Name your-email@example.com
Summary by CodeRabbit
New Features
Bug Fixes
Tests