ci: add CI-Ready CPU Brev launchable for E2E tests (#1327) - #1362
Merged
Conversation
) Add scripts/brev-launchable-ci-cpu.sh — a startup script for a Brev launchable that pre-bakes a VM with Docker, Node.js 22, OpenShell CLI, NemoClaw npm deps, and pre-pulled Docker images. Compared to the existing NemoClaw launchable (launch-nemoclaw.sh from OpenShell-Community), this script: - Pre-pulls sandbox-base, openshell/cluster, and node:22-slim images (saves 3-5 min per CI run) - Pre-installs npm deps and builds the TS plugin - Skips code-server, VS Code themes, and other interactive tooling - Uses a sentinel file for reliable readiness detection - Includes retry logic for apt and network operations - Waits for apt locks (Brev VMs run unattended-upgrades at boot) Part of epic NVIDIA#1326.
Update brev-e2e.test.js with dual-path bootstrap: - Launchable path (USE_LAUNCHABLE=1): uses pre-baked CI environment, waits for sentinel file, rsyncs branch code, runs nemoclaw onboard - Bare instance path (USE_LAUNCHABLE=0): existing brev-setup.sh flow Launchable readiness detection uses a sentinel file check (/var/run/nemoclaw-launchable-ready) instead of grepping log files, which was the root cause of the 40-minute timeouts that led to the launchable being removed previously. Update e2e-brev.yaml workflow with: - use_launchable input (default: true) - launchable_id input (for CI-Ready CPU launchable ID) - Temporarily disable repo check for fork testing The LAUNCHABLE_ID defaults to empty, which disables the launchable path until the Brev org billing is resolved and the launchable is created. Set USE_LAUNCHABLE=0 explicitly to use bare instances. Part of epic NVIDIA#1326.
Launchable 'NemoClaw CI CPU' created on Brev under Nemoclaw CI/CD org. Config: NEBIUS 4 vCPU / 16 GiB RAM / 256 GiB, VM Mode, setup script curls brev-launchable-ci-cpu.sh from the repo. This enables USE_LAUNCHABLE=1 (default) in the e2e-brev workflow. Part of NVIDIA#1327.
jyaunches
added
enhancement
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds optional Launchable-based provisioning and branch auto-resolution to E2E CI: new workflow inputs, resolved-branch checkout, changed keep_alive default, a Launchable bootstrap script, and test changes to support Launchable vs. bare VM flows and related robustness fixes. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Developer
participant GitHubActions as GitHub Actions
participant Workflow as e2e-brev.yaml
participant Runner as Actions Runner
participant Launchable as Launchable (OpenShell)
participant VM as Remote VM (SSH)
Developer->>GitHubActions: trigger workflow (dispatch / workflow_call)
GitHubActions->>Workflow: provide inputs (pr_number, use_launchable, setup_script_url)
Workflow->>Workflow: resolve RESOLVED_BRANCH (gh pr view)
Workflow->>Runner: checkout RESOLVED_BRANCH / inputs.branch
Workflow->>Runner: set env USE_LAUNCHABLE, LAUNCHABLE_SETUP_SCRIPT
alt USE_LAUNCHABLE == true
Runner->>Launchable: brev create --startup-script `@script`
Launchable->>VM: provision VM and run startup script
Runner->>VM: poll sentinel /var/run/nemoclaw-launchable-ready
Runner->>VM: rsync repo, npm install, build, start onboard
else
Runner->>Launchable: brev create --detached (bare flow)
Runner->>VM: run scripts/brev-setup.sh, wait for completion
end
Runner->>Runner: run E2E tests against provisioned sandbox
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
) The Brev CLI doesn't have an --env flag for launchable IDs. Brev 'launchables' are web-UI deployment templates — the CLI equivalent is `brev start <repo> --setup-script <url> --cpu <spec>`. Changes: - Replace --env with --setup-script pointing to brev-launchable-ci-cpu.sh - Add --cpu 4x16 flag (matches existing launchable compute config) - Rename LAUNCHABLE_ID env var to LAUNCHABLE_SETUP_SCRIPT - Add BREV_CPU env var (default: 4x16) - Update workflow inputs accordingly
…VIDIA#1327) Brev CLI v0.6.322 removed --cpu from `brev start` and has no way to specify CPU instances via that command. The correct v0.6.322 pattern is: brev search cpu | brev create <name> --startup-script @file --detached This downloads the setup script URL to a temp file first (brev create accepts @filepath but not URLs), then pipes CPU search results into create with the startup script attached. Both launchable and bare-instance paths now use the same instance selection: `brev search cpu --min-vcpu 4 --min-ram 16 --sort price`.
…name error The Brev API can succeed in creating an instance but return an EOF before the CLI confirms it. The CLI then falls back to the next instance type, which fails with 'duplicate workspace'. Pre-deleting any instance with the same name prevents this. Part of NVIDIA#1327.
…ists The Brev API sometimes creates the instance server-side but returns 'unexpected EOF' to the CLI. The CLI's fallback then tries the next instance type, which fails with 'duplicate workspace'. Fix: wrap brev create in try/catch, and on failure check brev ls to see if the instance was actually created. If it exists, proceed normally. Applied to both launchable and bare-instance paths. Part of NVIDIA#1327.
…DIA#1327) Three fixes for the launchable path on Brev VMs: 1. sudo npm link: Node.js is installed system-wide via nodesource so npm link needs root to symlink into /usr/lib/node_modules/. 2. sg docker wrapping: SSH sessions don't pick up the docker group added by the setup script (needs re-login). Wrap Docker-dependent commands with sg docker -c. 3. chmod 666 /var/run/docker.sock: Belt-and-suspenders for Docker access on short-lived CI VMs. 4. npm install instead of npm ci: more forgiving when branch package.json/lock may have drifted from the launchable's clone. Part of NVIDIA#1327.
The `sg docker -c` wrapper reparents child processes to init, causing `streamSandboxCreate` in onboard.js to lose track of the openshell subprocess. When onboard tries to SIGTERM the child after the sandbox is Ready, it kills the defunct bash wrapper instead of openshell, leaving the create process running forever. The setup script already `chmod 666 /var/run/docker.sock`, so Docker is accessible without sg docker. Remove all sg docker wrappers. Part of NVIDIA#1327.
jyaunches
force-pushed
the
feat/brev-ci-launchables
branch
from
April 2, 2026 22:02
91a0ac4 to
72a7a86
Compare
Move PR number to the primary input — entering a PR number automatically resolves the branch via `gh pr view`. The branch input is kept as a fallback for testing branches without a PR. This makes the GitHub Actions UI simpler: just enter the PR number instead of remembering the branch name.
The nohup'd onboard process was keeping SSH file descriptors open, causing ETIMEDOUT after 30s. Fix with: - </dev/null to detach stdin - disown to remove from shell job table - try/catch for ETIMEDOUT with fallback log file check - sleep 2 before confirming launch Part of NVIDIA#1327.
The heredoc delimiter 'REGISTRY' contains single quotes that conflict with the ssh() function's single-quote wrapping, causing SSH exit 255. Replace with printf + shellEscape which handles quoting correctly. Also split pkill and registry write into separate ssh() calls for clearer error handling. Part of NVIDIA#1327.
jyaunches
marked this pull request as ready for review
April 2, 2026 23:09
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
scripts/brev-launchable-ci-cpu.sh (2)
217-233:sg dockeris unnecessary afterchmod 666 /var/run/docker.sock.Lines 139 explicitly sets world-readable/writable permissions on the Docker socket (
chmod 666). Thesg docker -cwrapper on lines 220, 229, and 231 is redundant since any user can now access the socket without being in the docker group.This simplifies the code and removes a potential failure point if the
sgcommand isn't available or the docker group doesn't exist yet.Proposed fix
for image in "${DOCKER_IMAGES[@]}"; do info " Pulling $image..." - sg docker -c "docker pull $image" 2>&1 | tail -1 \ + docker pull "$image" 2>&1 | tail -1 \ || warn " Failed to pull $image (will be pulled at test time)" done # The openshell/cluster image tag should match the CLI version. # Try the pinned version first, fall back to latest. CLUSTER_TAG="${OPENSHELL_VERSION#v}" # v0.0.20 → 0.0.20 CLUSTER_IMAGE="ghcr.io/nvidia/openshell/cluster:${CLUSTER_TAG}" info " Pulling $CLUSTER_IMAGE..." - if ! sg docker -c "docker pull $CLUSTER_IMAGE" 2>&1 | tail -1; then + if ! docker pull "$CLUSTER_IMAGE" 2>&1 | tail -1; then warn " Could not pull $CLUSTER_IMAGE — trying :latest" - sg docker -c "docker pull ghcr.io/nvidia/openshell/cluster:latest" 2>&1 | tail -1 \ + docker pull ghcr.io/nvidia/openshell/cluster:latest 2>&1 | tail -1 \ || warn " Failed to pull openshell/cluster (will be pulled at test time)" fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/brev-launchable-ci-cpu.sh` around lines 217 - 233, The sg docker wrapper is redundant after chmod 666 /var/run/docker.sock; remove the sg docker -c wrapper around docker pull invocations so they call docker directly: update the loop that iterates DOCKER_IMAGES (currently using sg docker -c "docker pull $image") and the CLUSTER_IMAGE pull logic (currently using sg docker -c "docker pull $CLUSTER_IMAGE" and the fallback sg docker -c "docker pull ghcr.io/nvidia/openshell/cluster:latest") to run docker pull without sg, keeping the same logging/warning behavior and using the existing variables CLUSTER_TAG and CLUSTER_IMAGE.
186-195:git pull --ff-onlywill fail ifNEMOCLAW_REFis a tag or commit SHA.When
NEMOCLAW_REFis a tag (e.g.,v1.0.0) or a commit SHA,git pull --ff-only origin <ref>will fail because you can't pull a tag/commit as a branch. The|| truemasks the error, but the checkout on line 189 already positions HEAD correctly, making the pull redundant for non-branch refs.Consider making the pull conditional on branch refs:
Proposed fix
git -C "$NEMOCLAW_CLONE_DIR" fetch origin "$NEMOCLAW_REF" git -C "$NEMOCLAW_CLONE_DIR" checkout "$NEMOCLAW_REF" - git -C "$NEMOCLAW_CLONE_DIR" pull --ff-only origin "$NEMOCLAW_REF" || true + # Only pull if on a branch (not a detached HEAD from tag/commit) + if git -C "$NEMOCLAW_CLONE_DIR" symbolic-ref -q HEAD >/dev/null 2>&1; then + git -C "$NEMOCLAW_CLONE_DIR" pull --ff-only origin "$NEMOCLAW_REF" || true + fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/brev-launchable-ci-cpu.sh` around lines 186 - 195, The git pull --ff-only line is unsafe for tags/SHAs; change the block that handles an existing clone so after git -C "$NEMOCLAW_CLONE_DIR" checkout "$NEMOCLAW_REF" you detect whether NEMOCLAW_REF is a branch (e.g., use git -C "$NEMOCLAW_CLONE_DIR" show-ref --verify --quiet "refs/heads/$NEMOCLAW_REF" or query origin with git ls-remote --heads origin "$NEMOCLAW_REF") and only run git -C "$NEMOCLAW_CLONE_DIR" pull --ff-only origin "$NEMOCLAW_REF" when that check succeeds; remove the || true masking and keep checkout behavior unchanged so tags/SHAs aren’t pulled.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/e2e-brev.yaml:
- Line 109: Uncomment and restore the repository guard so the workflow only runs
for the upstream repo by re-enabling the if condition `if: github.repository ==
'NVIDIA/NemoClaw'` in the e2e-brev.yaml workflow, and add a brief TODO comment
(or create a tracking issue) near that line to remind maintainers to verify it
before merging to main; ensure the condition is active rather than commented out
to prevent workflows from running on forks without required secrets.
In `@scripts/brev-launchable-ci-cpu.sh`:
- Around line 241-242: The append to LAUNCH_LOG may fail if the path is
root-protected (sudo was used for touch but not for the echo append). Update the
script to perform the log append with elevated privileges (e.g., use sudo with a
shell redirection or sudo tee -a) when writing to $LAUNCH_LOG after touching
$SENTINEL, or alternatively validate/document that LAUNCH_LOG is user-writable
and fail early; ensure changes reference the SENTINEL and LAUNCH_LOG operations
so the touch and subsequent log append run under consistent permission handling.
In `@test/e2e/brev-e2e.test.js`:
- Around line 457-475: The manually constructed registry JSON written to
~/.nemoclaw/sandboxes.json is missing fields that registerSandbox
(bin/lib/registry.js: registerSandbox) expects/defaults (createdAt, model,
nimContainer, provider), which can cause callers like getSandbox to see
undefineds; update the registryJson object built in the test (variable
registryJson used in the ssh printf) to include those fields for the "e2e-test"
sandbox (set createdAt to an ISO timestamp, model to the default model string,
nimContainer to an object or null matching registerSandbox shape, and provider
to the expected default), so the test writes a fully formed sandbox entry
instead of only name/gpuEnabled/policies.
---
Nitpick comments:
In `@scripts/brev-launchable-ci-cpu.sh`:
- Around line 217-233: The sg docker wrapper is redundant after chmod 666
/var/run/docker.sock; remove the sg docker -c wrapper around docker pull
invocations so they call docker directly: update the loop that iterates
DOCKER_IMAGES (currently using sg docker -c "docker pull $image") and the
CLUSTER_IMAGE pull logic (currently using sg docker -c "docker pull
$CLUSTER_IMAGE" and the fallback sg docker -c "docker pull
ghcr.io/nvidia/openshell/cluster:latest") to run docker pull without sg, keeping
the same logging/warning behavior and using the existing variables CLUSTER_TAG
and CLUSTER_IMAGE.
- Around line 186-195: The git pull --ff-only line is unsafe for tags/SHAs;
change the block that handles an existing clone so after git -C
"$NEMOCLAW_CLONE_DIR" checkout "$NEMOCLAW_REF" you detect whether NEMOCLAW_REF
is a branch (e.g., use git -C "$NEMOCLAW_CLONE_DIR" show-ref --verify --quiet
"refs/heads/$NEMOCLAW_REF" or query origin with git ls-remote --heads origin
"$NEMOCLAW_REF") and only run git -C "$NEMOCLAW_CLONE_DIR" pull --ff-only origin
"$NEMOCLAW_REF" when that check succeeds; remove the || true masking and keep
checkout behavior unchanged so tags/SHAs aren’t pulled.
🪄 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: Pro
Run ID: c478d13e-d9a6-492c-bdcf-e19199367451
📒 Files selected for processing (3)
.github/workflows/e2e-brev.yamlscripts/brev-launchable-ci-cpu.shtest/e2e/brev-e2e.test.js
pkill -f 'nemoclaw onboard' matches the SSH process running the command (its cmdline contains the pattern), causing SSH exit 255. This is expected — wrap in try/catch and proceed to write the registry file in a separate SSH call. Part of NVIDIA#1327.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/e2e/brev-e2e.test.js (1)
196-575: 🛠️ Refactor suggestion | 🟠 MajorSplit this
beforeAllinto helpers before it trips the complexity budget.This hook now owns auth, cleanup, two provisioning paths, two polling loops, the onboard workaround, and registry validation. Please extract at least the launchable path, bare path, onboard wait, and registry-write steps so failures are easier to localize. As per coding guidelines,
**/*.{js,ts,tsx}: Enforce cyclomatic complexity limit of 20 (ratcheting down to 15) via ESLint.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/brev-e2e.test.js` around lines 196 - 575, The beforeAll hook is too large and should be split into clear helper functions: extract the entire Launchable flow into a helper (e.g., createInstanceWithLaunchable or launchableProvisioning) that contains the setup-script download, brev create, rsync, npm install/build/link, background onboard start and the waitForLaunchableReady usage; extract the bare VM flow into a separate helper (e.g., createBareInstance or bareProvisioning) that runs brev create, rsync, and sshEnv('bash scripts/brev-setup.sh'); extract the onboard polling logic (the while loop that checks openshell sandbox list, reads /tmp/nemoclaw-onboard.log, and inspects ~/.nemoclaw/onboard-session.json) into waitForOnboardReady/onboardPoll; and extract the registry write and verification (printf to ~/.nemoclaw/sandboxes.json and JSON.parse/expect checks) into writeAndVerifyRegistry; then have beforeAll call these helpers in sequence (authenticate/cleanup -> either createInstanceWithLaunchable or createBareInstance -> waitForSsh() -> waitForOnboardReady() -> writeAndVerifyRegistry()), keeping existing helpers like waitForSsh, waitForLaunchableReady, ssh, sshEnv, and shellEscape unchanged so errors are localized and cyclomatic complexity is reduced.
♻️ Duplicate comments (1)
test/e2e/brev-e2e.test.js (1)
466-480:⚠️ Potential issue | 🟡 MinorWrite the full sandbox shape here, not just the stable fields.
This manual registry entry still omits fields the normal registry path populates, including
createdAt,model,nimContainer, andprovider. Anything that reads~/.nemoclaw/sandboxes.jsondirectly will see a partial sandbox record.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/brev-e2e.test.js` around lines 466 - 480, The manual registry JSON created in the test (registryJson) only contains stable fields; update the object passed to JSON.stringify to include the full sandbox shape used by the real registry (add createdAt timestamp, model, nimContainer, provider and any other fields the normal registry populates) so the test writes a complete sandbox record to ~/.nemoclaw/sandboxes.json; locate the registryJson construction in the test (variable registryJson) and extend the "e2e-test" sandbox object to include those missing properties with realistic test values.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/e2e/brev-e2e.test.js`:
- Around line 44-46: DEFAULT_SETUP_SCRIPT_URL currently defaults to the remote
main-branch URL which makes PR runs non-hermetic; change the default to the
repo-local script (scripts/brev-launchable-ci-cpu.sh) and keep
process.env.LAUNCHABLE_SETUP_SCRIPT as an override so CI uses the checked-out
branch by default; update the other duplicate occurrence referenced around the
234-239 area as well to use the same repo-local default (refer to the
DEFAULT_SETUP_SCRIPT_URL constant and its duplicate) so both places behave
identically.
- Around line 296-324: The npm install ssh invocation currently pipes to `tail
-5` which masks npm failures; update the ssh command used for dependency install
(the ssh([...].join(" && ") call that runs `cd ${remoteDir} && npm install
--ignore-scripts 2>&1 | tail -5`) to enable pipefail (e.g., prefix with `set -o
pipefail`) so npm exit codes propagate, and make the build step robust by
reinstalling the nemoclaw package before building (modify the ssh call that runs
`cd ${remoteDir}/nemoclaw && npm run build` to run `npm install
--ignore-scripts` in that folder first, then run the build, ensuring you still
source nvm as in the existing commands).
---
Outside diff comments:
In `@test/e2e/brev-e2e.test.js`:
- Around line 196-575: The beforeAll hook is too large and should be split into
clear helper functions: extract the entire Launchable flow into a helper (e.g.,
createInstanceWithLaunchable or launchableProvisioning) that contains the
setup-script download, brev create, rsync, npm install/build/link, background
onboard start and the waitForLaunchableReady usage; extract the bare VM flow
into a separate helper (e.g., createBareInstance or bareProvisioning) that runs
brev create, rsync, and sshEnv('bash scripts/brev-setup.sh'); extract the
onboard polling logic (the while loop that checks openshell sandbox list, reads
/tmp/nemoclaw-onboard.log, and inspects ~/.nemoclaw/onboard-session.json) into
waitForOnboardReady/onboardPoll; and extract the registry write and verification
(printf to ~/.nemoclaw/sandboxes.json and JSON.parse/expect checks) into
writeAndVerifyRegistry; then have beforeAll call these helpers in sequence
(authenticate/cleanup -> either createInstanceWithLaunchable or
createBareInstance -> waitForSsh() -> waitForOnboardReady() ->
writeAndVerifyRegistry()), keeping existing helpers like waitForSsh,
waitForLaunchableReady, ssh, sshEnv, and shellEscape unchanged so errors are
localized and cyclomatic complexity is reduced.
---
Duplicate comments:
In `@test/e2e/brev-e2e.test.js`:
- Around line 466-480: The manual registry JSON created in the test
(registryJson) only contains stable fields; update the object passed to
JSON.stringify to include the full sandbox shape used by the real registry (add
createdAt timestamp, model, nimContainer, provider and any other fields the
normal registry populates) so the test writes a complete sandbox record to
~/.nemoclaw/sandboxes.json; locate the registryJson construction in the test
(variable registryJson) and extend the "e2e-test" sandbox object to include
those missing properties with realistic test values.
🪄 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: Pro
Run ID: 8bedc98d-fb49-4856-8ac7-d49fa22f2e03
📒 Files selected for processing (1)
test/e2e/brev-e2e.test.js
Re-enable `if: github.repository == 'NVIDIA/NemoClaw'` to prevent unintended workflow runs on forks without the required secrets. Addresses PR review feedback on NVIDIA#1362.
- Use sudo tee for LAUNCH_LOG write consistency (setup script) - Add missing registry fields (createdAt, model, nimContainer, provider) - Default to repo-local setup script instead of hardcoded main URL (hermetic — always matches the checked-out branch) - Add set -o pipefail to npm install to propagate failures through pipe - Reinstall nemoclaw plugin deps before build (npm install && npm run build)
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/e2e/brev-e2e.test.js (1)
197-593: 🛠️ Refactor suggestion | 🟠 MajorSplit
beforeAllinto provisioning helpers.This hook now contains two full provisioning flows, readiness polling, onboard recovery, registry patching, and shared assertions. It’s well past the repo’s complexity budget and hard to change safely; extracting the launchable path, bare path, repo sync, and sandbox-wait logic into separate helpers will make both branches much easier to maintain.
As per coding guidelines,
**/*.{js,ts,tsx}: Enforce cyclomatic complexity limit of 20 (ratcheting down to 15) via ESLint.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/brev-e2e.test.js` around lines 197 - 593, The beforeAll hook is too large and should be split into focused provisioning helpers: extract the launchable path into a function (e.g., provisionLaunchableInstance) that encapsulates setup-script resolution, brev create with retry logic, SSH wait, rsync, npm install/build/link, background onboard launch and registry write (reference ssh, sshEnv, execSync usage and waitForLaunchableReady), extract the bare-instance path into provisionBareInstance that runs brev create, waitForSsh, rsync, and runs brev-setup.sh (reference waitForSsh and sshEnv), and extract the sandbox polling/assertion logic into waitForSandboxReady (which contains the openshell sandbox list loop, onboard log checks, and final registry verification). Replace the large beforeAll body with calls to these helpers and keep shared steps (mkdirSync onboarding file, brev login, pre-delete) in beforeAll; ensure instanceCreated, remoteDir, and timing logs are returned or set by helpers so the final registry assertions still run.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/e2e/brev-e2e.test.js`:
- Around line 229-245: The test currently logs the raw setup-script override via
DEFAULT_SETUP_SCRIPT_PATH which may leak presigned or credential-bearing URLs;
update the logic around DEFAULT_SETUP_SCRIPT_PATH and the setupScriptPath
handling (the branch that sets setupScriptPath when
DEFAULT_SETUP_SCRIPT_PATH.startsWith("http")) to avoid printing the full
URL—either log a generic message like "remote setup script override in use" or
redact sensitive parts (strip query string and userinfo by parsing the URL)
before including it in the console.log, so change the console.log call that
prints DEFAULT_SETUP_SCRIPT_PATH to use a redacted value or non-sensitive
indicator instead.
- Around line 209-218: The pre-cleanup currently swallows all errors from
brev("delete", INSTANCE_NAME); change it to only ignore a explicit "not found"
outcome and otherwise surface/log or retry: detect the specific error/message
that indicates the instance does not exist (from brev("delete", ...)) and let
other errors propagate or be retried; alternatively after a non-throwing delete
attempt call brev("ls", INSTANCE_NAME) (or use the brev list API) to verify the
instance is actually gone and if still present fail the test or retry deletion;
update the code paths around brev("delete", INSTANCE_NAME), the surrounding
try/catch, and the fallback logic that reads brev ls/ brev create to use this
verification so stale instances cannot be reused.
- Around line 307-337: The rsync + npm install flow is not hermetic because
execSync rsync excludes node_modules so the remote keeps pre-cached modules
which npm install mutates; modify the deployment sequence around the
execSync(rsync ...) and the subsequent ssh(...) calls to ensure a clean install:
either remove the --exclude node_modules from the rsync command so the
checked-out repo's node_modules is used, or (preferred) keep the exclusion but
add a cleanup step on the remote (e.g., ssh run to rm -rf
${remoteDir}/node_modules && rm -f ${remoteDir}/package-lock.json) and replace
npm install with npm ci (or run npm ci after deleting node_modules) in the
ssh(...) invocations that currently call npm install (the npm install in the
first ssh block and the npm install in the TypeScript plugin build command) so
the test run uses a fresh, lockfile-resolved install.
---
Outside diff comments:
In `@test/e2e/brev-e2e.test.js`:
- Around line 197-593: The beforeAll hook is too large and should be split into
focused provisioning helpers: extract the launchable path into a function (e.g.,
provisionLaunchableInstance) that encapsulates setup-script resolution, brev
create with retry logic, SSH wait, rsync, npm install/build/link, background
onboard launch and registry write (reference ssh, sshEnv, execSync usage and
waitForLaunchableReady), extract the bare-instance path into
provisionBareInstance that runs brev create, waitForSsh, rsync, and runs
brev-setup.sh (reference waitForSsh and sshEnv), and extract the sandbox
polling/assertion logic into waitForSandboxReady (which contains the openshell
sandbox list loop, onboard log checks, and final registry verification). Replace
the large beforeAll body with calls to these helpers and keep shared steps
(mkdirSync onboarding file, brev login, pre-delete) in beforeAll; ensure
instanceCreated, remoteDir, and timing logs are returned or set by helpers so
the final registry assertions still run.
🪄 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: Pro
Run ID: 0a53409e-b95b-4262-8216-4f156b5621a8
📒 Files selected for processing (2)
scripts/brev-launchable-ci-cpu.shtest/e2e/brev-e2e.test.js
🚧 Files skipped from review as they are similar to previous changes (1)
- scripts/brev-launchable-ci-cpu.sh
Comment on lines
+209
to
218
| // Pre-cleanup: delete any leftover instance with the same name. | ||
| // This can happen when a previous run's create succeeded on the backend | ||
| // but the CLI got a network error (unexpected EOF) before confirming, | ||
| // then the retry/fallback fails with "duplicate workspace". | ||
| try { | ||
| brev("refresh"); | ||
| brev("delete", INSTANCE_NAME); | ||
| console.log(`[${elapsed()}] Deleted leftover instance "${INSTANCE_NAME}"`); | ||
| } catch { | ||
| /* ignore */ | ||
| // Expected — no leftover instance exists | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's get an overview of the test file and check if it exists
ls -la test/e2e/brev-e2e.test.js 2>&1 || echo "File not found"Repository: NVIDIA/NemoClaw
Length of output: 134
🏁 Script executed:
# Read the code at the specified line ranges to verify the issue
# Lines 209-218 (pre-cleanup)
echo "=== Lines 209-218 (pre-cleanup) ===" && sed -n '209,218p' test/e2e/brev-e2e.test.js && \
echo "" && \
echo "=== Lines 264-284 (first related section) ===" && sed -n '264,284p' test/e2e/brev-e2e.test.js && \
echo "" && \
echo "=== Lines 509-535 (second related section) ===" && sed -n '509,535p' test/e2e/brev-e2e.test.jsRepository: NVIDIA/NemoClaw
Length of output: 2514
🏁 Script executed:
# Get more context around these lines to understand the full flow
# Let's look at lines 200-230 for context around the pre-cleanup
echo "=== Full context: lines 200-230 ===" && sed -n '200,230p' test/e2e/brev-e2e.test.jsRepository: NVIDIA/NemoClaw
Length of output: 1503
Pre-cleanup silently ignores delete errors, risking stale instance reuse.
The catch-all at line 216 swallows all delete errors—not just "not found" cases. If that delete fails for any reason (network, permission, state lock, etc.), the old instance persists. Later, when brev create fails at lines 264–284 or 509–535, the fallback brev ls check finds the leftover instance by name and treats it as the newly created one. This causes the test to reuse dirty state instead of provisioning fresh.
Either catch only the specific "not found" error, or add a verification step that confirms the instance is actually gone before relying on the brev ls fallback.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/e2e/brev-e2e.test.js` around lines 209 - 218, The pre-cleanup currently
swallows all errors from brev("delete", INSTANCE_NAME); change it to only ignore
a explicit "not found" outcome and otherwise surface/log or retry: detect the
specific error/message that indicates the instance does not exist (from
brev("delete", ...)) and let other errors propagate or be retried; alternatively
after a non-throwing delete attempt call brev("ls", INSTANCE_NAME) (or use the
brev list API) to verify the instance is actually gone and if still present fail
the test or retry deletion; update the code paths around brev("delete",
INSTANCE_NAME), the surrounding try/catch, and the fallback logic that reads
brev ls/ brev create to use this verification so stale instances cannot be
reused.
Comment on lines
+229
to
+245
| console.log( | ||
| `[${elapsed()}] Creating instance via launchable (brev search cpu | brev create + startup-script)...`, | ||
| ); | ||
| console.log(`[${elapsed()}] setup-script: ${DEFAULT_SETUP_SCRIPT_PATH}`); | ||
| console.log(`[${elapsed()}] cpu: min ${BREV_MIN_VCPU} vCPU, ${BREV_MIN_RAM} GB RAM`); | ||
|
|
||
| // Resolve the setup script to a local file path. | ||
| // Default: repo-local scripts/brev-launchable-ci-cpu.sh (hermetic). | ||
| // Override: set LAUNCHABLE_SETUP_SCRIPT to a URL and it gets downloaded. | ||
| let setupScriptPath; | ||
| if (DEFAULT_SETUP_SCRIPT_PATH.startsWith("http")) { | ||
| setupScriptPath = "/tmp/brev-ci-setup.sh"; | ||
| execSync(`curl -fsSL -o ${setupScriptPath} "${DEFAULT_SETUP_SCRIPT_PATH}"`, { | ||
| encoding: "utf-8", | ||
| timeout: 30_000, | ||
| }); | ||
| console.log(`[${elapsed()}] Setup script downloaded to ${setupScriptPath}`); |
Contributor
There was a problem hiding this comment.
Don’t log the raw setup-script override.
Line 232 prints LAUNCHABLE_SETUP_SCRIPT verbatim. If that override is a presigned or credential-bearing URL, the secret lands in the CI log. Log only that a remote override is in use, or redact query/userinfo before printing.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/e2e/brev-e2e.test.js` around lines 229 - 245, The test currently logs
the raw setup-script override via DEFAULT_SETUP_SCRIPT_PATH which may leak
presigned or credential-bearing URLs; update the logic around
DEFAULT_SETUP_SCRIPT_PATH and the setupScriptPath handling (the branch that sets
setupScriptPath when DEFAULT_SETUP_SCRIPT_PATH.startsWith("http")) to avoid
printing the full URL—either log a generic message like "remote setup script
override in use" or redact sensitive parts (strip query string and userinfo by
parsing the URL) before including it in the console.log, so change the
console.log call that prints DEFAULT_SETUP_SCRIPT_PATH to use a redacted value
or non-sensitive indicator instead.
Comment on lines
+307
to
+337
| execSync( | ||
| `rsync -az --delete --exclude node_modules --exclude .git --exclude dist --exclude .venv "${REPO_DIR}/" "${INSTANCE_NAME}:${remoteDir}/"`, | ||
| { encoding: "utf-8", timeout: 120_000 }, | ||
| ); | ||
| console.log(`[${elapsed()}] Code synced`); | ||
|
|
||
| // Re-install deps for our branch (most already cached by launchable). | ||
| // Use `npm install` instead of `npm ci` because the rsync'd branch code | ||
| // may have a package.json/package-lock.json that are slightly out of sync | ||
| // (e.g. new transitive deps). npm install is more forgiving and still | ||
| // benefits from the launchable's pre-cached node_modules. | ||
| console.log(`[${elapsed()}] Running npm install to sync dependencies...`); | ||
| ssh( | ||
| [ | ||
| `set -o pipefail`, | ||
| `source ~/.nvm/nvm.sh 2>/dev/null || true`, | ||
| `cd ${remoteDir}`, | ||
| `npm install --ignore-scripts 2>&1 | tail -5`, | ||
| ].join(" && "), | ||
| { timeout: 300_000, stream: true }, | ||
| ); | ||
| console.log(`[${elapsed()}] Dependencies synced`); | ||
|
|
||
| // Rebuild TS plugin for our branch (reinstall plugin deps in case they changed) | ||
| console.log(`[${elapsed()}] Building TypeScript plugin...`); | ||
| ssh( | ||
| `source ~/.nvm/nvm.sh 2>/dev/null || true && cd ${remoteDir}/nemoclaw && npm install && npm run build`, | ||
| { | ||
| timeout: 120_000, | ||
| stream: true, | ||
| }, |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n test/e2e/brev-e2e.test.js | sed -n '300,345p'Repository: NVIDIA/NemoClaw
Length of output: 2431
🏁 Script executed:
head -20 test/e2e/brev-e2e.test.js | grep -E "^(import|export|require)"Repository: NVIDIA/NemoClaw
Length of output: 41
🏁 Script executed:
rg "node_modules" test/e2e/brev-e2e.test.js -B 3 -A 3Repository: NVIDIA/NemoClaw
Length of output: 1233
🏁 Script executed:
head -50 test/e2e/brev-e2e.test.jsRepository: NVIDIA/NemoClaw
Length of output: 2520
🏁 Script executed:
rg "(^import |^const.*require\(|^const.*from)" test/e2e/brev-e2e.test.js | head -20Repository: NVIDIA/NemoClaw
Length of output: 304
The launchable path is not hermetic due to persisted pre-cached node_modules.
Line 308 uses rsync --exclude node_modules, which preserves the launchable's pre-cached node_modules directory on the remote. Lines 324 and 333 then run npm install, which mutates that cached tree in place rather than installing clean. This allows dependency state from the image to bleed into the PR run—removed packages or lockfile drift can still be present. Use npm ci (clean install) or delete/prune the preserved node_modules before testing to ensure the PR run validates the checked-out revision.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/e2e/brev-e2e.test.js` around lines 307 - 337, The rsync + npm install
flow is not hermetic because execSync rsync excludes node_modules so the remote
keeps pre-cached modules which npm install mutates; modify the deployment
sequence around the execSync(rsync ...) and the subsequent ssh(...) calls to
ensure a clean install: either remove the --exclude node_modules from the rsync
command so the checked-out repo's node_modules is used, or (preferred) keep the
exclusion but add a cleanup step on the remote (e.g., ssh run to rm -rf
${remoteDir}/node_modules && rm -f ${remoteDir}/package-lock.json) and replace
npm install with npm ci (or run npm ci after deleting node_modules) in the
ssh(...) invocations that currently call npm install (the npm install in the
first ssh block and the npm install in the TypeScript plugin build command) so
the test run uses a fresh, lockfile-resolved install.
sudo npm link creates root-owned files in dist/. When test-full-e2e.sh later runs install.sh which rebuilds via npm run build:cli, the TS compiler can't write to the root-owned dist/ directory. Fix: chown the repo dir back to the user after sudo npm link. Part of NVIDIA#1327.
PR number resolves the branch automatically. The setup script defaults to the repo-local file. Remove these from workflow_dispatch to keep the form clean. They remain available in workflow_call for programmatic use. Also default keep_alive to false (was true — left over from debugging).
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
test/e2e/brev-e2e.test.js (2)
214-219:⚠️ Potential issue | 🟡 MinorPre-cleanup catches all delete errors, risking stale instance reuse.
The catch-all at line 217 swallows all delete errors—not just "not found" cases. If delete fails for network, permission, or state lock reasons, the old instance persists and may be reused by the
brev lsfallback later (lines 274-284, 528-538). Consider catching only the specific "not found" error, or adding a verification step after delete.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/brev-e2e.test.js` around lines 214 - 219, The try/catch around brev("delete", INSTANCE_NAME) is swallowing all errors; change it to only ignore a "not found" error (or equivalent sentinel) and rethrow or surface other errors, or add a verification step after delete (e.g., call brev("ls") or a getInstance check to confirm INSTANCE_NAME no longer exists) and fail the test if deletion did not succeed; update the catch to check the error message/code before suppressing and ensure any network/permission/state errors are not silently ignored.
230-250:⚠️ Potential issue | 🟡 MinorLogging potentially sensitive setup script URL.
Line 233 prints
DEFAULT_SETUP_SCRIPT_PATHverbatim. If the override is a presigned or credential-bearing URL, the secret lands in CI logs. Log only that a remote override is in use, or redact query parameters before printing.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/brev-e2e.test.js` around lines 230 - 250, The test prints DEFAULT_SETUP_SCRIPT_PATH verbatim which may leak presigned/credential-bearing URLs; update the branch that handles DEFAULT_SETUP_SCRIPT_PATH.startsWith("http") to avoid logging the full URL (reference DEFAULT_SETUP_SCRIPT_PATH and setupScriptPath in the launch logic). Instead log a safe message such as "remote setup script override detected" and/or a redacted form (e.g., strip query parameters or show only the hostname/path without the query string) before downloading, and keep the curl/execSync behavior unchanged; ensure any console.log that previously included DEFAULT_SETUP_SCRIPT_PATH is replaced with the redacted/safe message.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/e2e/brev-e2e.test.js`:
- Around line 443-448: The error string is showing “[object Object]” because
parsed.failure is a SessionFailure object; update the throw to extract a human
message (e.g. use parsed.failure?.message) and fall back to a safe stringified
form if message is missing (e.g. JSON.stringify(parsed.failure) or "unknown") so
the thrown Error contains a readable failure message; adjust the code around
parsed.status/parsed.failure (the throw in the test using failLog and ssh) to
use parsed.failure?.message ?? JSON.stringify(parsed.failure) ?? "unknown".
---
Duplicate comments:
In `@test/e2e/brev-e2e.test.js`:
- Around line 214-219: The try/catch around brev("delete", INSTANCE_NAME) is
swallowing all errors; change it to only ignore a "not found" error (or
equivalent sentinel) and rethrow or surface other errors, or add a verification
step after delete (e.g., call brev("ls") or a getInstance check to confirm
INSTANCE_NAME no longer exists) and fail the test if deletion did not succeed;
update the catch to check the error message/code before suppressing and ensure
any network/permission/state errors are not silently ignored.
- Around line 230-250: The test prints DEFAULT_SETUP_SCRIPT_PATH verbatim which
may leak presigned/credential-bearing URLs; update the branch that handles
DEFAULT_SETUP_SCRIPT_PATH.startsWith("http") to avoid logging the full URL
(reference DEFAULT_SETUP_SCRIPT_PATH and setupScriptPath in the launch logic).
Instead log a safe message such as "remote setup script override detected"
and/or a redacted form (e.g., strip query parameters or show only the
hostname/path without the query string) before downloading, and keep the
curl/execSync behavior unchanged; ensure any console.log that previously
included DEFAULT_SETUP_SCRIPT_PATH is replaced with the redacted/safe message.
🪄 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: Pro
Run ID: 74f7d6a5-c993-440f-bbf1-8dfcc5fbe375
📒 Files selected for processing (2)
.github/workflows/e2e-brev.yamltest/e2e/brev-e2e.test.js
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/e2e-brev.yaml
Comment on lines
+443
to
+448
| if (parsed.status === "failed") { | ||
| const failLog = ssh("cat /tmp/nemoclaw-onboard.log 2>/dev/null || echo 'no log'", { | ||
| timeout: 10_000, | ||
| }); | ||
| throw new Error(`Onboard failed: ${parsed.failure || "unknown"}\n${failLog}`); | ||
| } |
Contributor
There was a problem hiding this comment.
parsed.failure is an object, not a string — error message will show [object Object].
Per src/lib/onboard-session.ts, Session.failure is a SessionFailure object with step, message, and recordedAt properties. Using parsed.failure || "unknown" will produce [object Object] in the error message.
Proposed fix to extract the failure message
- if (parsed.status === "failed") {
- const failLog = ssh("cat /tmp/nemoclaw-onboard.log 2>/dev/null || echo 'no log'", {
- timeout: 10_000,
- });
- throw new Error(`Onboard failed: ${parsed.failure || "unknown"}\n${failLog}`);
- }
+ if (parsed.status === "failed") {
+ const failLog = ssh("cat /tmp/nemoclaw-onboard.log 2>/dev/null || echo 'no log'", {
+ timeout: 10_000,
+ });
+ const failureMsg = parsed.failure?.message || JSON.stringify(parsed.failure) || "unknown";
+ throw new Error(`Onboard failed: ${failureMsg}\n${failLog}`);
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/e2e/brev-e2e.test.js` around lines 443 - 448, The error string is
showing “[object Object]” because parsed.failure is a SessionFailure object;
update the throw to extract a human message (e.g. use parsed.failure?.message)
and fall back to a safe stringified form if message is missing (e.g.
JSON.stringify(parsed.failure) or "unknown") so the thrown Error contains a
readable failure message; adjust the code around parsed.status/parsed.failure
(the throw in the test using failLog and ssh) to use parsed.failure?.message ??
JSON.stringify(parsed.failure) ?? "unknown".
3 tasks
ericksoa
approved these changes
Apr 3, 2026
ericksoa
left a comment
ericksoa
left a comment
Contributor
There was a problem hiding this comment.
Good improvement — the launchable path should meaningfully reduce E2E setup time and flakiness. The startup script is well-structured with retry logic, apt lock handling, and sentinel-based readiness. Structural follow-ups tracked in #1390.
cjagwani
pushed a commit
to fdzdev/NemoClaw
that referenced
this pull request
Apr 3, 2026
…DIA#1362) ## Summary Adds a pre-baked Brev launchable ("NemoClaw CI CPU") that eliminates 5-10 minutes of per-run setup time in E2E CI by pre-installing Docker, Node.js, OpenShell CLI, npm deps, and pre-pulling Docker images. ## Problem The current E2E Brev workflow bootstraps a bare VM from scratch every run — installing Docker, Node.js, OpenShell CLI, cloning repos, pulling multi-GB Docker images, and building the sandbox. This 10-15 minute setup window is where most CI failures occur (apt mirror timeouts, Docker pull rate limits, npm registry hiccups). We previously had a launchable (`launch-nemoclaw.sh` from OpenShell-Community) but removed it when readiness detection was unreliable (40-min timeouts grepping log files). The launchable itself was saving time — the detection mechanism was the problem. ## Solution ### New startup script: `scripts/brev-launchable-ci-cpu.sh` Purpose-built for CI (no code-server, no VS Code theming). Pre-installs: - Docker + systemd service - Node.js 22 (nodesource) - OpenShell CLI (pinned v0.0.20) - NemoClaw repo with `npm install` + TS plugin build - Docker images pre-pulled: `sandbox-base:latest`, `openshell/cluster`, `node:22-slim` - Retry logic for apt/network operations - Waits for apt locks (Brev VMs run unattended-upgrades at boot) ### Reliable readiness detection Uses a sentinel file (`/var/run/nemoclaw-launchable-ready`) instead of grepping log files. The test harness polls `ssh test -f <sentinel>` which is atomic and reliable. ### Dual-path test harness: `test/e2e/brev-e2e.test.js` - **Launchable path** (`USE_LAUNCHABLE=1`, default): `brev start` with launchable → wait for sentinel → rsync branch code → `npm ci` → `nemoclaw onboard` → test - **Bare instance path** (`USE_LAUNCHABLE=0`): existing `brev search cpu | brev create` + `brev-setup.sh` flow preserved as fallback ### Workflow: `.github/workflows/e2e-brev.yaml` - New inputs: `use_launchable` (default: true), `launchable_id` - Passes `USE_LAUNCHABLE` and `LAUNCHABLE_ID` env vars to test runner ## Launchable - **Name**: NemoClaw CI CPU - **ID**: `env-3BoRsC1YMHNLmu82xvIike1Nh6E` - **Compute**: NEBIUS 4 vCPU / 16 GiB RAM / 256 GiB — $0.12/hr - **Org**: Nemoclaw CI/CD - **Deploy URL**: https://brev.nvidia.com/launchable/deploy?launchableID=env-3BoRsC1YMHNLmu82xvIike1Nh6E ## Expected CI Time Savings | Step | Before (bare VM) | After (launchable) | |------|------------------|-------------------| | Docker install | ~30s | 0 (pre-installed) | | Node.js install | ~30s | 0 (pre-installed) | | OpenShell CLI | ~15s | 0 (pre-installed) | | Docker image pulls | ~3-5 min | 0 (pre-pulled) | | npm install | ~60s | ~30s (mostly cached) | | Plugin build | ~30s | ~15s (incremental) | | **Total setup** | **~10-12 min** | **~3-4 min** | ## Related - Epic: NVIDIA#1326 - Issue: NVIDIA#1327 - Brev bug filed: brevdev/brev-cli#346 ("Only my organization" visibility errors) ## Testing - [ ] Trigger `e2e-brev` workflow on fork with `use_launchable=true` - [ ] Verify launchable bootstrap completes (sentinel file detected) - [ ] Verify test suite passes - [ ] Compare timing vs bare-instance path <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Tests** * Added optional "launchable" provisioning mode with configurable setup script URL, pre-provision cleanup, polling-based readiness/onboarding handling, repo sync/build on target, and simplified stable sandbox assertions. * **Chores** * CI inputs expanded to toggle launchable mode and provide a setup script; PR-triggered runs now auto-resolve the source branch and use it for checkout and reporting. * CI defaults adjusted (keep-alive behavior changed). * **Chores** * Added a new CPU-focused CI bootstrapping script to improve environment setup and image pre-pulling. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Aaron Erickson 🦞 <aerickson@nvidia.com>
This was referenced Apr 3, 2026
lakamsani
pushed a commit
to lakamsani/NemoClaw
that referenced
this pull request
Apr 4, 2026
…DIA#1362) ## Summary Adds a pre-baked Brev launchable ("NemoClaw CI CPU") that eliminates 5-10 minutes of per-run setup time in E2E CI by pre-installing Docker, Node.js, OpenShell CLI, npm deps, and pre-pulling Docker images. ## Problem The current E2E Brev workflow bootstraps a bare VM from scratch every run — installing Docker, Node.js, OpenShell CLI, cloning repos, pulling multi-GB Docker images, and building the sandbox. This 10-15 minute setup window is where most CI failures occur (apt mirror timeouts, Docker pull rate limits, npm registry hiccups). We previously had a launchable (`launch-nemoclaw.sh` from OpenShell-Community) but removed it when readiness detection was unreliable (40-min timeouts grepping log files). The launchable itself was saving time — the detection mechanism was the problem. ## Solution ### New startup script: `scripts/brev-launchable-ci-cpu.sh` Purpose-built for CI (no code-server, no VS Code theming). Pre-installs: - Docker + systemd service - Node.js 22 (nodesource) - OpenShell CLI (pinned v0.0.20) - NemoClaw repo with `npm install` + TS plugin build - Docker images pre-pulled: `sandbox-base:latest`, `openshell/cluster`, `node:22-slim` - Retry logic for apt/network operations - Waits for apt locks (Brev VMs run unattended-upgrades at boot) ### Reliable readiness detection Uses a sentinel file (`/var/run/nemoclaw-launchable-ready`) instead of grepping log files. The test harness polls `ssh test -f <sentinel>` which is atomic and reliable. ### Dual-path test harness: `test/e2e/brev-e2e.test.js` - **Launchable path** (`USE_LAUNCHABLE=1`, default): `brev start` with launchable → wait for sentinel → rsync branch code → `npm ci` → `nemoclaw onboard` → test - **Bare instance path** (`USE_LAUNCHABLE=0`): existing `brev search cpu | brev create` + `brev-setup.sh` flow preserved as fallback ### Workflow: `.github/workflows/e2e-brev.yaml` - New inputs: `use_launchable` (default: true), `launchable_id` - Passes `USE_LAUNCHABLE` and `LAUNCHABLE_ID` env vars to test runner ## Launchable - **Name**: NemoClaw CI CPU - **ID**: `env-3BoRsC1YMHNLmu82xvIike1Nh6E` - **Compute**: NEBIUS 4 vCPU / 16 GiB RAM / 256 GiB — $0.12/hr - **Org**: Nemoclaw CI/CD - **Deploy URL**: https://brev.nvidia.com/launchable/deploy?launchableID=env-3BoRsC1YMHNLmu82xvIike1Nh6E ## Expected CI Time Savings | Step | Before (bare VM) | After (launchable) | |------|------------------|-------------------| | Docker install | ~30s | 0 (pre-installed) | | Node.js install | ~30s | 0 (pre-installed) | | OpenShell CLI | ~15s | 0 (pre-installed) | | Docker image pulls | ~3-5 min | 0 (pre-pulled) | | npm install | ~60s | ~30s (mostly cached) | | Plugin build | ~30s | ~15s (incremental) | | **Total setup** | **~10-12 min** | **~3-4 min** | ## Related - Epic: NVIDIA#1326 - Issue: NVIDIA#1327 - Brev bug filed: brevdev/brev-cli#346 ("Only my organization" visibility errors) ## Testing - [ ] Trigger `e2e-brev` workflow on fork with `use_launchable=true` - [ ] Verify launchable bootstrap completes (sentinel file detected) - [ ] Verify test suite passes - [ ] Compare timing vs bare-instance path <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Tests** * Added optional "launchable" provisioning mode with configurable setup script URL, pre-provision cleanup, polling-based readiness/onboarding handling, repo sync/build on target, and simplified stable sandbox assertions. * **Chores** * CI inputs expanded to toggle launchable mode and provide a setup script; PR-triggered runs now auto-resolve the source branch and use it for checkout and reporting. * CI defaults adjusted (keep-alive behavior changed). * **Chores** * Added a new CPU-focused CI bootstrapping script to improve environment setup and image pre-pulling. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Aaron Erickson 🦞 <aerickson@nvidia.com>
ericksoa
pushed a commit
that referenced
this pull request
Apr 4, 2026
…ty (#1470) ## Summary - unify installer and onboarding host detection around shared TypeScript preflight logic - move `deploy` behavior into TypeScript, thin the Brev compatibility wrapper, and harden Brev readiness handling - demote or remove legacy platform-specific setup paths (`setup-spark`, `brev-setup.sh`) in favor of the canonical installer + onboard flow - update docs, CLI help, and Brev E2E coverage to match the new behavior ## What Changed - added shared host assessment and remediation planning in `src/lib/preflight.ts` - wired installer and onboard flows to the same host preflight decisions - changed Podman handling from hard block to unsupported-runtime warning - migrated deploy logic into `src/lib/deploy.ts` - updated `nemoclaw deploy` to use the authenticated Brev CLI, current Brev create flags, explicit GCP provider default, stricter readiness checks, and standard installer/onboard flow - removed `scripts/setup-spark.sh` and reduced `scripts/brev-setup.sh` to a deprecated compatibility wrapper - updated README/docs/help text and hardened the Brev E2E cleanup path ## Validation - `npm run build:cli` - targeted Vitest coverage for `src/lib/preflight.test.ts`, `src/lib/deploy.test.ts`, `test/install-preflight.test.js`, `test/cli.test.js`, `test/runner.test.js` - live Brev validation with `TEST_SUITE=deploy-cli` on `cpu-e2.4vcpu-16gb` - confirmed successful end-to-end remote deploy after waiting for Brev `status=RUNNING`, `build_status=COMPLETED`, `shell_status=READY` ## Related Issues - Fixes #1377 - Addresses #1330 - Addresses #1390 - Related to #1404 ## Credit / Prior Work This branch builds on ideas and prior work from: - #1368 by @zyang-dev for simplifying Spark setup and removing the old cgroup workaround - #1395 and #1468 by @kjw3 for the thin installer/bootstrap direction and installer path reliability - #1450 by @cjagwani for switching Brev flows toward GCP for reliability - #1383 by @13ernkastel for the current Brev create flag compatibility work - #1364 by @WuKongAI-CMU for deploy sync-path fixes - #1362 and #1266 by @jyaunches for the Brev E2E/launchable infrastructure direction - issue ideas from #1377 and #1404 by @zNeill, #1330 by @Marcelo5444, and #1390 by @ericksoa <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Improved host diagnostics with actionable remediation guidance surfaced during installer/onboard preflight. * **Improvements** * macOS (Intel) now recommends Docker Desktop; DGX Spark guidance now uses the standard installer + `nemoclaw onboard`. * Preflight output shows detected runtime and WSL notes; installer prints remediation actions and will skip onboarding on blocking issues. * **Deprecations** * `nemoclaw deploy`, `nemoclaw setup-spark`, and the legacy bootstrap wrapper are now deprecated compatibility paths. * **Documentation** * Quickstart, troubleshooting, and command reference updated to reflect installer+onboard flow and deprecation guidance. * **Tests** * Added/updated tests covering preflight, deploy compatibility, CLI aliases, and deploy e2e scenarios. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
This was referenced Apr 13, 2026
gemini2026
pushed a commit
to gemini2026/NemoClaw
that referenced
this pull request
Apr 14, 2026
…DIA#1362) ## Summary Adds a pre-baked Brev launchable ("NemoClaw CI CPU") that eliminates 5-10 minutes of per-run setup time in E2E CI by pre-installing Docker, Node.js, OpenShell CLI, npm deps, and pre-pulling Docker images. ## Problem The current E2E Brev workflow bootstraps a bare VM from scratch every run — installing Docker, Node.js, OpenShell CLI, cloning repos, pulling multi-GB Docker images, and building the sandbox. This 10-15 minute setup window is where most CI failures occur (apt mirror timeouts, Docker pull rate limits, npm registry hiccups). We previously had a launchable (`launch-nemoclaw.sh` from OpenShell-Community) but removed it when readiness detection was unreliable (40-min timeouts grepping log files). The launchable itself was saving time — the detection mechanism was the problem. ## Solution ### New startup script: `scripts/brev-launchable-ci-cpu.sh` Purpose-built for CI (no code-server, no VS Code theming). Pre-installs: - Docker + systemd service - Node.js 22 (nodesource) - OpenShell CLI (pinned v0.0.20) - NemoClaw repo with `npm install` + TS plugin build - Docker images pre-pulled: `sandbox-base:latest`, `openshell/cluster`, `node:22-slim` - Retry logic for apt/network operations - Waits for apt locks (Brev VMs run unattended-upgrades at boot) ### Reliable readiness detection Uses a sentinel file (`/var/run/nemoclaw-launchable-ready`) instead of grepping log files. The test harness polls `ssh test -f <sentinel>` which is atomic and reliable. ### Dual-path test harness: `test/e2e/brev-e2e.test.js` - **Launchable path** (`USE_LAUNCHABLE=1`, default): `brev start` with launchable → wait for sentinel → rsync branch code → `npm ci` → `nemoclaw onboard` → test - **Bare instance path** (`USE_LAUNCHABLE=0`): existing `brev search cpu | brev create` + `brev-setup.sh` flow preserved as fallback ### Workflow: `.github/workflows/e2e-brev.yaml` - New inputs: `use_launchable` (default: true), `launchable_id` - Passes `USE_LAUNCHABLE` and `LAUNCHABLE_ID` env vars to test runner ## Launchable - **Name**: NemoClaw CI CPU - **ID**: `env-3BoRsC1YMHNLmu82xvIike1Nh6E` - **Compute**: NEBIUS 4 vCPU / 16 GiB RAM / 256 GiB — $0.12/hr - **Org**: Nemoclaw CI/CD - **Deploy URL**: https://brev.nvidia.com/launchable/deploy?launchableID=env-3BoRsC1YMHNLmu82xvIike1Nh6E ## Expected CI Time Savings | Step | Before (bare VM) | After (launchable) | |------|------------------|-------------------| | Docker install | ~30s | 0 (pre-installed) | | Node.js install | ~30s | 0 (pre-installed) | | OpenShell CLI | ~15s | 0 (pre-installed) | | Docker image pulls | ~3-5 min | 0 (pre-pulled) | | npm install | ~60s | ~30s (mostly cached) | | Plugin build | ~30s | ~15s (incremental) | | **Total setup** | **~10-12 min** | **~3-4 min** | ## Related - Epic: NVIDIA#1326 - Issue: NVIDIA#1327 - Brev bug filed: brevdev/brev-cli#346 ("Only my organization" visibility errors) ## Testing - [ ] Trigger `e2e-brev` workflow on fork with `use_launchable=true` - [ ] Verify launchable bootstrap completes (sentinel file detected) - [ ] Verify test suite passes - [ ] Compare timing vs bare-instance path <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Tests** * Added optional "launchable" provisioning mode with configurable setup script URL, pre-provision cleanup, polling-based readiness/onboarding handling, repo sync/build on target, and simplified stable sandbox assertions. * **Chores** * CI inputs expanded to toggle launchable mode and provide a setup script; PR-triggered runs now auto-resolve the source branch and use it for checkout and reporting. * CI defaults adjusted (keep-alive behavior changed). * **Chores** * Added a new CPU-focused CI bootstrapping script to improve environment setup and image pre-pulling. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Aaron Erickson 🦞 <aerickson@nvidia.com>
gemini2026
pushed a commit
to gemini2026/NemoClaw
that referenced
this pull request
Apr 14, 2026
…ty (NVIDIA#1470) ## Summary - unify installer and onboarding host detection around shared TypeScript preflight logic - move `deploy` behavior into TypeScript, thin the Brev compatibility wrapper, and harden Brev readiness handling - demote or remove legacy platform-specific setup paths (`setup-spark`, `brev-setup.sh`) in favor of the canonical installer + onboard flow - update docs, CLI help, and Brev E2E coverage to match the new behavior ## What Changed - added shared host assessment and remediation planning in `src/lib/preflight.ts` - wired installer and onboard flows to the same host preflight decisions - changed Podman handling from hard block to unsupported-runtime warning - migrated deploy logic into `src/lib/deploy.ts` - updated `nemoclaw deploy` to use the authenticated Brev CLI, current Brev create flags, explicit GCP provider default, stricter readiness checks, and standard installer/onboard flow - removed `scripts/setup-spark.sh` and reduced `scripts/brev-setup.sh` to a deprecated compatibility wrapper - updated README/docs/help text and hardened the Brev E2E cleanup path ## Validation - `npm run build:cli` - targeted Vitest coverage for `src/lib/preflight.test.ts`, `src/lib/deploy.test.ts`, `test/install-preflight.test.js`, `test/cli.test.js`, `test/runner.test.js` - live Brev validation with `TEST_SUITE=deploy-cli` on `cpu-e2.4vcpu-16gb` - confirmed successful end-to-end remote deploy after waiting for Brev `status=RUNNING`, `build_status=COMPLETED`, `shell_status=READY` ## Related Issues - Fixes NVIDIA#1377 - Addresses NVIDIA#1330 - Addresses NVIDIA#1390 - Related to NVIDIA#1404 ## Credit / Prior Work This branch builds on ideas and prior work from: - NVIDIA#1368 by @zyang-dev for simplifying Spark setup and removing the old cgroup workaround - NVIDIA#1395 and NVIDIA#1468 by @kjw3 for the thin installer/bootstrap direction and installer path reliability - NVIDIA#1450 by @cjagwani for switching Brev flows toward GCP for reliability - NVIDIA#1383 by @13ernkastel for the current Brev create flag compatibility work - NVIDIA#1364 by @WuKongAI-CMU for deploy sync-path fixes - NVIDIA#1362 and NVIDIA#1266 by @jyaunches for the Brev E2E/launchable infrastructure direction - issue ideas from NVIDIA#1377 and NVIDIA#1404 by @zNeill, NVIDIA#1330 by @Marcelo5444, and NVIDIA#1390 by @ericksoa <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Improved host diagnostics with actionable remediation guidance surfaced during installer/onboard preflight. * **Improvements** * macOS (Intel) now recommends Docker Desktop; DGX Spark guidance now uses the standard installer + `nemoclaw onboard`. * Preflight output shows detected runtime and WSL notes; installer prints remediation actions and will skip onboarding on blocking issues. * **Deprecations** * `nemoclaw deploy`, `nemoclaw setup-spark`, and the legacy bootstrap wrapper are now deprecated compatibility paths. * **Documentation** * Quickstart, troubleshooting, and command reference updated to reflect installer+onboard flow and deprecation guidance. * **Tests** * Added/updated tests covering preflight, deploy compatibility, CLI aliases, and deploy e2e scenarios. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
wscurran
added
area: ci
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
3 participants
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
Adds a pre-baked Brev launchable ("NemoClaw CI CPU") that eliminates 5-10 minutes of per-run setup time in E2E CI by pre-installing Docker, Node.js, OpenShell CLI, npm deps, and pre-pulling Docker images.
Problem
The current E2E Brev workflow bootstraps a bare VM from scratch every run — installing Docker, Node.js, OpenShell CLI, cloning repos, pulling multi-GB Docker images, and building the sandbox. This 10-15 minute setup window is where most CI failures occur (apt mirror timeouts, Docker pull rate limits, npm registry hiccups).
We previously had a launchable (
launch-nemoclaw.shfrom OpenShell-Community) but removed it when readiness detection was unreliable (40-min timeouts grepping log files). The launchable itself was saving time — the detection mechanism was the problem.Solution
New startup script:
scripts/brev-launchable-ci-cpu.shPurpose-built for CI (no code-server, no VS Code theming). Pre-installs:
npm install+ TS plugin buildsandbox-base:latest,openshell/cluster,node:22-slimReliable readiness detection
Uses a sentinel file (
/var/run/nemoclaw-launchable-ready) instead of grepping log files. The test harness pollsssh test -f <sentinel>which is atomic and reliable.Dual-path test harness:
test/e2e/brev-e2e.test.jsUSE_LAUNCHABLE=1, default):brev startwith launchable → wait for sentinel → rsync branch code →npm ci→nemoclaw onboard→ testUSE_LAUNCHABLE=0): existingbrev search cpu | brev create+brev-setup.shflow preserved as fallbackWorkflow:
.github/workflows/e2e-brev.yamluse_launchable(default: true),launchable_idUSE_LAUNCHABLEandLAUNCHABLE_IDenv vars to test runnerLaunchable
env-3BoRsC1YMHNLmu82xvIike1Nh6EExpected CI Time Savings
Related
Testing
e2e-brevworkflow on fork withuse_launchable=trueSummary by CodeRabbit
Tests
Chores