fix(ci): revert #2186 and use direct symlink for nemoclaw CLI on Brev#2195
Closed
fix(ci): revert #2186 and use direct symlink for nemoclaw CLI on Brev#2195
Conversation
PR #2186 tried to unblock non-full Brev E2E suites by moving the slow `sudo npm link` step from the test (120s cap) into the launchable setup (20min outer cap). That hit a worse pathology: on cold CPU Brev, `sudo npm link` + `sudo chown -R` on a ~50k-file node_modules tree doesn't complete within 20 minutes at all. Every run now hangs at "Linking nemoclaw CLI globally..." until the outer cap trips — 10× longer to fail and 10× more Brev credit per failed run. The previous commit reverts #2186. This commit replaces `sudo npm link` with what npm link actually produces: two symlinks. We can do them directly with `sudo ln -sf` and skip npm's global-prefix housekeeping entirely. O(1), no chown traversal, no hang. Changes: - scripts/brev-launchable-ci-cpu.sh: after plugin build, create /usr/local/bin/nemoclaw → $NEMOCLAW_CLONE_DIR/bin/nemoclaw.js with a direct `sudo ln -sf`. Drop the `sudo chown -R node_modules` that used to pair with npm link (no longer needed — only one file is owned by root now). - test/e2e/brev-e2e.test.ts: replace the in-test `sudo npm link` with the same direct-symlink approach. Launchable already pre-links on the same path; this is idempotent re-link so local dev runs that skip the launchable still work. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: Charan Jagwani <cjagwani@nvidia.com>
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughUpdated the NemoClaw CLI installation process in a shell script and end-to-end test from using Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Contributor
Author
|
Reopening with clearer branch name — see #NEW |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Reverts #2186 (which made Brev E2E failures 10× slower) and replaces `sudo npm link` with a direct `sudo ln -sf` symlink in both the launchable setup and the in-test bootstrap path. `npm link` is overkill for what we actually need and hangs indefinitely on cold CPU Brev instances. Direct symlink is O(1) and deterministic.
Related Issue
Regression from my own #2186. Surfaced while validating #2183 (ollama E2E).
Changes
This PR contains two commits:
Evidence
Type of Change
Verification
Retro note
Shipped #2186 without running the Brev suite end-to-end — exact failure mode I had flagged on #2123 earlier the same day. This PR is validated the right way before merge.
AI Disclosure
Summary by CodeRabbit
Chores