fix(cli): remove dead --plan logic and unused snapshot modules, optimize test timeouts#1030
fix(cli): remove dead --plan logic and unused snapshot modules, optimize test timeouts#1030ksapru wants to merge 0 commit intoNVIDIA:mainfrom
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:
📝 WalkthroughWalkthroughRemoved support and runtime checks for the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
ksapru
force-pushed
the
refactor/remove-unused-exports
branch
2 times, most recently
from
d5a807f to
a8587b6
Compare
|
Hi! Thanks again for reviewing and approving the PR. It looks like there are 3 workflows awaiting approval — just wanted to check if anything is needed from my side to unblock them. Happy to make any additional changes if required! |
|
@ksapru looks like a legit e2e test breakage: https://github.com/NVIDIA/NemoClaw/actions/runs/23688838598/job/69015789660?pr=1030 |
|
@cv Thanks — good catch. This was a legitimate e2e breakage. We initially removed To fix this:
CI should now pass. I'll follow up with a separate PR to safely remove or refactor these modules with a full understanding of their runtime dependencies. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/cli.test.js (1)
740-747:⚠️ Potential issue | 🟡 MinorMissing explicit timeout in
runWithEnvcall.This test has a Vitest timeout of 25000ms but
runWithEnvuses the default 10000ms. Other similar tests (e.g., lines 289, 673) pass an explicit timeout torunWithEnv. The command could timeout at 10s while the test allows 25s, causing inconsistent failures.Proposed fix
const statusResult = runWithEnv("alpha status", { HOME: home, PATH: `${localBin}:${process.env.PATH || ""}`, - }); + }, 25000);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/cli.test.js` around lines 740 - 747, The runWithEnv invocation in the test uses the default 10s timeout while the test itself allows 25s; update the call to runWithEnv("alpha status", { HOME: home, PATH: `${localBin}:${process.env.PATH || ""}` }, 25000) so it explicitly passes the 25000ms timeout (matching the test's timeout) to prevent the command from timing out early; modify the call to runWithEnv in this test (and follow the same pattern used by other tests that already pass a timeout) to include the third timeout argument.
🧹 Nitpick comments (1)
test/cli.test.js (1)
754-763: Consider improvinggetRestoreFnBody()extraction and assertion clarity.Two observations:
Imprecise extraction:
src.slice(fnStart)returns everything from the function start to EOF, not just the function body. If another function later in the file happens to contain the checked patterns, the test could pass even ifrestoreSnapshotToHostloses its validation. This works today since the checks are indeed in the right function (per the context snippets), but it's fragile.Unhelpful assertion:
expect(fnStart !== -1).toBeTruthy()yields a generic failure message. Usingexpect(fnStart).toBeGreaterThanOrEqual(0)or throwing a descriptive error would improve debuggability.Optional improvement
function getRestoreFnBody() { const src = fs.readFileSync( path.join(import.meta.dirname, "..", "nemoclaw", "src", "commands", "migration-state.ts"), "utf-8", ); const fnStart = src.indexOf("function restoreSnapshotToHost"); - expect(fnStart !== -1).toBeTruthy(); - return src.slice(fnStart); + if (fnStart === -1) { + throw new Error("restoreSnapshotToHost function not found in migration-state.ts"); + } + // Extract until next top-level function or export (rough heuristic) + const afterFn = src.slice(fnStart); + const nextFnMatch = afterFn.slice(1).search(/\n(?:export\s+)?(?:async\s+)?function\s+\w+/); + return nextFnMatch === -1 ? afterFn : afterFn.slice(0, nextFnMatch + 1); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/cli.test.js` around lines 754 - 763, The getRestoreFnBody() helper currently returns src.slice(fnStart) which grabs from "function restoreSnapshotToHost" to EOF and can yield false positives; change it to extract only the restoreSnapshotToHost function body by locating the function's end (e.g., find the opening brace after "function restoreSnapshotToHost", then walk/balance braces to the matching closing brace) and slice that range, and replace the vague expect(fnStart !== -1).toBeTruthy() with a clearer assertion such as expect(fnStart).toBeGreaterThanOrEqual(0) or throw a descriptive error so missing function is reported clearly; update references to getRestoreFnBody and restoreSnapshotToHost accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@test/cli.test.js`:
- Around line 740-747: The runWithEnv invocation in the test uses the default
10s timeout while the test itself allows 25s; update the call to
runWithEnv("alpha status", { HOME: home, PATH: `${localBin}:${process.env.PATH
|| ""}` }, 25000) so it explicitly passes the 25000ms timeout (matching the
test's timeout) to prevent the command from timing out early; modify the call to
runWithEnv in this test (and follow the same pattern used by other tests that
already pass a timeout) to include the third timeout argument.
---
Nitpick comments:
In `@test/cli.test.js`:
- Around line 754-763: The getRestoreFnBody() helper currently returns
src.slice(fnStart) which grabs from "function restoreSnapshotToHost" to EOF and
can yield false positives; change it to extract only the restoreSnapshotToHost
function body by locating the function's end (e.g., find the opening brace after
"function restoreSnapshotToHost", then walk/balance braces to the matching
closing brace) and slice that range, and replace the vague expect(fnStart !==
-1).toBeTruthy() with a clearer assertion such as
expect(fnStart).toBeGreaterThanOrEqual(0) or throw a descriptive error so
missing function is reported clearly; update references to getRestoreFnBody and
restoreSnapshotToHost accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 59fec08e-df5b-436c-ba31-0c758eceecd2
📒 Files selected for processing (4)
nemoclaw/src/blueprint/runner.test.tsnemoclaw/src/blueprint/runner.tstest/cli.test.jstest/uninstall.test.js
💤 Files with no reviewable changes (1)
- nemoclaw/src/blueprint/runner.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- nemoclaw/src/blueprint/runner.ts
ksapru
changed the title
ksapru
force-pushed
the
refactor/remove-unused-exports
branch
from
02d3237 to
dcdd833
Compare
ksapru
force-pushed
the
refactor/remove-unused-exports
branch
from
dcdd833 to
c051cbb
Compare
wscurran
added
CI/CD
|
✨ Thanks for submitting this proposed fix with a detailed summary, it addresses issues with CLI test reliability and proposes changes to improve test consistency and reduce flakiness, which could improve the overall testing experience. |
|
Updating this PR after resolving a signing/rebase issue that affected the previous commits. The underlying changes remain the same, but I’ve rebased and pushed a clean, signed commit history. The PR is now focused on:
CI should be stable now — would appreciate a quick re-check. PR linked here: PR |
3 participants
Summary
Improves CLI test reliability and execution consistency by normalizing shell invocation and increasing timeouts for long-running commands.
Related Issue
Fixes #977 (part 1)
Changes
bash -lcwithbash -cin uninstall tests for more deterministic executionrunWithEnv(..., timeout)for long-running commandsVerification
npm testpasses locallynpx prek run --all-filespassesRationale
Some CLI tests were flaky due to shell initialization differences and insufficient timeouts.
These changes improve determinism and stability without altering runtime behavior.
Risk Assessment
Low risk:
Rollback:
Type of Change
Testing
npm testpassesnpx prek run --all-filespassesChecklist
General
Code Changes
Summary by CodeRabbit
Bug Fixes
Tests