scripts/brev-launchable-ci-cpu.sh (2)

83-99: ⚡ Quick win

Consider filtering out annotated tag dereference entries.

Annotated tags appear twice in git ls-remote output: once as refs/tags/v1.0.0 and once as refs/tags/v1.0.0^{} (the dereferenced commit). The --sort=-v:refname may not handle this predictably, and ^{} entries could end up first.

♻️ Proposed fix to filter out ^{} entries

       tag=$(git ls-remote --tags --sort=-v:refname \
         "https://github.com/NVIDIA/NemoClaw.git" 'refs/tags/v*' 2>/dev/null \
-        | head -1 | sed 's|.*refs/tags/||')
+        | grep -v '\^{}' | head -1 | sed 's|.*refs/tags/||')

🤖 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 `@scripts/brev-launchable-ci-cpu.sh` around lines 83 - 99, The git tag
resolution in resolve_stable_ref can pick up annotated-tag dereference lines
(refs/tags/vX...^{}) which may sort first; update the git ls-remote pipeline in
resolve_stable_ref so it filters out any lines containing ^{} before selecting
the top tag (e.g., pipe through a grep -v or similar), then proceed to set
NEMOCLAW_REF and log as before using the tag variable.

---

264-266: 💤 Low value

git pull --ff-only will fail when NEMOCLAW_REF is a tag.

After resolving to a tag (e.g., v0.0.36), git checkout "$NEMOCLAW_REF" puts the repo in detached HEAD state, and git pull --ff-only origin "$NEMOCLAW_REF" will fail because you cannot pull into a detached HEAD.

The || true at line 266 silently swallows this failure, which is acceptable for idempotency, but the error message may confuse users checking logs.

♻️ Optional: Skip pull when on a tag

   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 (tags are immutable, pull would fail)
+  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 current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@scripts/brev-launchable-ci-cpu.sh` around lines 264 - 266, The git pull
--ff-only step can fail when NEMOCLAW_REF is a tag because git checkout
"$NEMOCLAW_REF" puts the repo in detached HEAD and pull into detached HEAD
errors (currently swallowed by || true). Modify the script around the git
checkout/pull sequence to detect detached HEAD (or detect that NEMOCLAW_REF
resolves to a tag) and skip the git -C "$NEMOCLAW_CLONE_DIR" pull --ff-only
origin "$NEMOCLAW_REF" step in that case; alternatively, create a local branch
from the ref before pulling (e.g., git -C "$NEMOCLAW_CLONE_DIR" checkout -B
temp-sync "$NEMOCLAW_REF") so pull can succeed. Ensure the logic references
NEMOCLAW_REF, git -C "$NEMOCLAW_CLONE_DIR" checkout, and git -C
"$NEMOCLAW_CLONE_DIR" pull --ff-only.

test/onboard.test.ts (1)

3422-3422: ⚡ Quick win

Cover the 401-is-healthy case in these probe stubs.

These updates lock the tests to /health, but every stub still returns only "200". The regression this PR fixes is specifically that device-auth health probes can return 401 and should still be treated as alive, so these flows would still pass if the code regressed back to 200-only handling. Please run at least one representative path with "401" as the stubbed status, or parameterize these cases over ["200", "401"].

Also applies to: 3559-3559, 3655-3655, 4018-4018, 5411-5411, 5806-5806, 5938-5938, 6555-6555

🤖 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/onboard.test.ts` at line 3422, The probe stub that matches
_n(command).includes("sandbox exec") &&
_n(command).includes("http://localhost:18789/health") currently always returns
"200" which misses the regression where a healthy device-auth probe may return
"401"; update the test stub(s) around the `_n(command)` check (and the other
identical occurrences) to either parameterize the returned status over
["200","401"] or add at least one representative test case/run that returns
"401" so the code path treating 401 as healthy is exercised; ensure you modify
the same conditional that matches "sandbox exec" and the /health URL so those
tests cover both statuses.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

src/lib/onboard.ts (1)

9506-9541: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Block final success when deployment verification fails.

Because Line 9483 already completes the session and the legacy-credential cleanup runs before this block, an unhealthy verificationResult still leaves onboarding finalized and then prints the normal dashboard output. That makes the new verifier warning-only. Move completeSession(), legacy cleanup, and printDashboard() behind a verificationResult.healthy check, and exit non-zero when verification fails.

Suggested flow

-    onboardSession.completeSession(toSessionUpdates({ sandboxName, provider, model }));
-    completed = true;
-    const allStagedMigrated =
-      stagedLegacyKeys.length > 0 && stagedLegacyKeys.every((k) => migratedLegacyKeys.has(k));
-    if (allStagedMigrated) {
-      removeLegacyCredentialsFile();
-    } else if (stagedLegacyKeys.length > 0) {
-      ...
-    }
     const verifyDeploymentModule: typeof import("./verify-deployment") = require("./verify-deployment");
     ...
     const verificationResult = verifyDeploymentModule.verifyDeployment(
       sandboxName,
       verifyChain,
       { ... },
     );

     const diagLines = verifyDeploymentModule.formatVerificationDiagnostics(verificationResult);
     for (const line of diagLines) {
       console.log(line);
     }
+
+    if (!verificationResult.healthy) {
+      process.exit(1);
+    }
+
+    onboardSession.completeSession(toSessionUpdates({ sandboxName, provider, model }));
+    completed = true;
+    const allStagedMigrated =
+      stagedLegacyKeys.length > 0 && stagedLegacyKeys.every((k) => migratedLegacyKeys.has(k));
+    if (allStagedMigrated) {
+      removeLegacyCredentialsFile();
+    } else if (stagedLegacyKeys.length > 0) {
+      ...
+    }

     printDashboard(sandboxName, model, provider, nimContainer, agent);

🤖 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/onboard.ts` around lines 9506 - 9541, The onboarding flow currently
finalizes the session and prints the dashboard even when verification fails;
update the logic around verifyDeploymentModule.verifyDeployment so that after
obtaining verificationResult you check verificationResult.healthy and only then
call completeSession(), run the legacy-credential cleanup, and call
printDashboard(sandboxName, model, provider, nimContainer, agent); if
verificationResult.healthy is false, log/print the diagnostics (using
verifyDeploymentModule.formatVerificationDiagnostics) and exit with a non-zero
status instead of completing the session. Ensure you reference the existing
verificationResult, verifyDeploymentModule, completeSession(), and
printDashboard(...) symbols and do not duplicate verification logic—just gate
the finalization/cleanup/dashboard steps on verificationResult.healthy and exit
non-zero on failure.

test/onboard.test.ts (1)

3418-3418: ⚡ Quick win

Add a 401-ready health probe case.

These updated mocks lock in the /health path, but they still only return 200. That means this file never exercises the device-auth case that caused #2342, so a regression back to treating 401 from /health as offline would still pass here. Please make at least one create/reuse readiness path return 401 and assert it is still treated as alive.

Also applies to: 3555-3555, 3651-3651, 4014-4014, 5407-5407, 5802-5802, 5934-5934, 6551-6551

🤖 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/onboard.test.ts` at line 3418, The test's mock of the sandbox health
probe currently always returns "200" (the branch in onboard.test.ts that checks
for _n(command).includes("sandbox exec") &&
_n(command).includes("http://localhost:18789/health")), so it never exercises
the device-auth 401 case; update the mock so at least one create/reuse readiness
path returns "401" (e.g., make one health URL case return "401" and leave others
"200") and add an assertion after the create/reuse flow that the instance is
still considered alive (assert the same success/ready state the tests expect) to
ensure 401 is treated as alive—locate and change the branch that builds the
sandbox exec mock and add the corresponding assertion in the create/reuse test
block.

src/lib/onboard.ts (1)

9486-9544: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Only finalize onboarding after verificationResult.healthy is true.

This still completes the session, performs legacy-credential cleanup, and prints the normal post-onboard output before the verification result is enforced. A failed verification therefore leaves onboarding marked complete and non-resumable, which makes the new verifier warning-only instead of authoritative.

🤖 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/onboard.ts` around lines 9486 - 9544, The code currently marks
onboarding complete and performs credential cleanup/printing before enforcing
verification; change the control flow so you call
verifyDeploymentModule.verifyDeployment and evaluate verificationResult.healthy
first, and only when healthy === true call
onboardSession.completeSession(toSessionUpdates(...)), perform the legacy-file
removal logic (removeLegacyCredentialsFile / console.error fallback) and call
printDashboard; if verificationResult.healthy is false, do not complete the
session or remove the legacy file — instead surface the verification diagnostics
and an actionable warning so the session remains resumable. Use the existing
symbols verifyDeploymentModule.verifyDeployment, verificationResult.healthy,
onboardSession.completeSession,
stagedLegacyKeys/migratedLegacyKeys/removeLegacyCredentialsFile, and
printDashboard to locate and reorder the logic.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

src/lib/agent-runtime.ts (1)

169-169: ⚡ Quick win

Consider deduplicating the HTTP probe guard string.

The _GW_CODE + case 200|401 snippet is repeated in two places; extracting one helper reduces drift risk for future health-probe changes.

♻️ Suggested refactor

+function buildAliveProbeGuard(url: string): string {
+  return `_GW_CODE=$(curl -so /dev/null -w '%{http_code}' --max-time 3 ${shellQuote(url)} 2>/dev/null || echo 000); case "$_GW_CODE" in 200|401) echo ALREADY_RUNNING; exit 0 ;; esac;`;
+}
+
 export function buildOpenClawRecoveryScript(port: number): string {
+  const probeUrl = `http://127.0.0.1:${port}/health`;
   const staleGatewayPattern = "[o]penclaw([ -]gateway| gateway run|$)";
   return [
@@
-    `_GW_CODE=$(curl -so /dev/null -w '%{http_code}' --max-time 3 http://127.0.0.1:${port}/health 2>/dev/null || echo 000); case "$_GW_CODE" in 200|401) echo ALREADY_RUNNING; exit 0 ;; esac;`,
+    buildAliveProbeGuard(probeUrl),
@@
 export function buildRecoveryScript(agent: AgentDefinition | null, port: number): string | null {
@@
-    `_GW_CODE=$(curl -so /dev/null -w '%{http_code}' --max-time 3 ${shellQuote(probeUrl)} 2>/dev/null || echo 000); case "$_GW_CODE" in 200|401) echo ALREADY_RUNNING; exit 0 ;; esac;`,
+    buildAliveProbeGuard(probeUrl),

Also applies to: 234-234

🤖 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/agent-runtime.ts` at line 169, The HTTP probe guard string starting
with `_GW_CODE=$(curl -so /dev/null -w '%{http_code}' --max-time 3
http://127.0.0.1:${port}/health 2>/dev/null || echo 000); case "$_GW_CODE" in
200|401) echo ALREADY_RUNNING; exit 0 ;; esac;` is duplicated; extract it into a
single helper (e.g., a constant HEALTH_PROBE_CMD or function
buildHealthProbe(port)) and replace both inline occurrences with that helper,
updating any callers that interpolate ${port} to use the helper API; ensure the
helper preserves the exact behavior and string formatting so both places (the
`_GW_CODE` probe usages) reuse the single source of truth.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes