test/e2e/test-issue-2478-crash-loop-recovery.sh (1)

316-326: File restore may fail silently due to permission issues.

The restore logic writes to /tmp/nemoclaw-proxy-env.sh.restore as the sandbox user, but the original file was root-owned with mode 444. The mv on line 324 will fail if the sandbox user lacks write permission to replace the original.

The comment acknowledges this ("best-effort"), but consider adding explicit feedback:

 sandbox_exec sh -c "cat > /tmp/nemoclaw-proxy-env.sh.restore <<'REPL'
 $SNAPSHOT
 REPL
 chmod 444 /tmp/nemoclaw-proxy-env.sh.restore 2>/dev/null || true
 mv /tmp/nemoclaw-proxy-env.sh.restore /tmp/nemoclaw-proxy-env.sh 2>/dev/null || true
 " >/dev/null
-info "proxy-env.sh restored (best-effort; soak phase will tolerate degraded state)"
+# Verify restore succeeded
+if sandbox_exec sh -c "[ -r /tmp/nemoclaw-proxy-env.sh ] && grep -q 'NODE_OPTIONS' /tmp/nemoclaw-proxy-env.sh" >/dev/null 2>&1; then
+  info "proxy-env.sh restored successfully"
+else
+  info "proxy-env.sh restore failed (best-effort); soak phase will tolerate degraded state"
+fi

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@test/e2e/test-issue-2478-crash-loop-recovery.sh` around lines 316 - 326, The
restore can fail silently because sandbox_exec redirects output and ignores
errors from chmod/mv; update the block that calls sandbox_exec so failures are
captured and reported: stop swallowing stdout/stderr (remove the final
>/dev/null) or capture the sandbox_exec exit code and log a clear error via
info/error if it is non-zero, and inside the sandbox_exec command ensure the
mv/chmod failures are propagated (e.g. check their exits and write an
explanatory message to stderr) and, as a fallback, attempt a privileged
move/chown when mv fails; reference the sandbox_exec invocation and the
/tmp/nemoclaw-proxy-env.sh.restore -> /tmp/nemoclaw-proxy-env.sh mv/chmod steps
so the script logs explicit failure details instead of silently continuing.

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

test/e2e/test-issue-2478-crash-loop-recovery.sh (1)

347-363: Verify the env-file restore before starting the soak.

This block suppresses restore failures, then immediately enters Phase 5 without re-checking the guard chain. If proxy-env.sh did not come back cleanly, the soak is no longer exercising the healthy recovery path this test is supposed to validate.

✅ Suggested hardening

-sandbox_exec sh -c "cat > /tmp/nemoclaw-proxy-env.sh.restore <<'REPL'
+if ! sandbox_exec sh -c "cat > /tmp/nemoclaw-proxy-env.sh.restore <<'REPL'
 $SNAPSHOT
 REPL
-chmod 444 /tmp/nemoclaw-proxy-env.sh.restore 2>/dev/null || true
-mv /tmp/nemoclaw-proxy-env.sh.restore /tmp/nemoclaw-proxy-env.sh 2>/dev/null || true
-" >/dev/null
-info "proxy-env.sh restored (best-effort; soak phase will tolerate degraded state)"
+chmod 444 /tmp/nemoclaw-proxy-env.sh.restore &&
+mv /tmp/nemoclaw-proxy-env.sh.restore /tmp/nemoclaw-proxy-env.sh
+" >/dev/null; then
+  fail "Failed to restore /tmp/nemoclaw-proxy-env.sh before soak"
+  exit 1
+fi
+info "proxy-env.sh restored"
 
 ...
 SOAK_START_PID="$(wait_for_gateway_up 30)"
 if [ -z "$SOAK_START_PID" ]; then
   fail "Gateway not up entering soak phase"
   exit 1
 fi
+SOAK_NODE_OPTIONS="$(gateway_node_options "$SOAK_START_PID")"
+if ! echo "$SOAK_NODE_OPTIONS" | grep -q 'nemoclaw-sandbox-safety-net'; then
+  fail "Gateway entered soak without safety-net preload after restore"
+  exit 1
+fi
 pass "Gateway healthy entering soak (pid=$SOAK_START_PID)"

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@test/e2e/test-issue-2478-crash-loop-recovery.sh` around lines 347 - 363, The
restore of the proxy env file is being done best-effort and failures are
suppressed; change it to verify the restore succeeded before starting the soak:
after calling sandbox_exec to write the snapshot, check the command exit status
and then validate that /tmp/nemoclaw-proxy-env.sh exists and is readable (and
optionally that its content matches $SNAPSHOT or a checksum) before proceeding
to call wait_for_gateway_up; if the verification fails, call fail (or exit
non‑zero) instead of continuing into the soak phase so that the test exercises
the healthy-recovery path; reference sandbox_exec, $SNAPSHOT,
/tmp/nemoclaw-proxy-env.sh, wait_for_gateway_up and fail when updating the
logic.

Fix all unresolved CodeRabbit comments on this PR:

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

.github/workflows/nightly-e2e.yaml (1)

678-701: ⚠️ Potential issue | 🟡 Minor

Add issue-2478-crash-loop-recovery-e2e to notify-on-failure.

This job is still missing from the notifier’s needs list, so a failure here will not be included in the auto-issue path or waited on before the notifier runs.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In @.github/workflows/nightly-e2e.yaml around lines 678 - 701, The
notify-on-failure job's needs list is missing the
issue-2478-crash-loop-recovery-e2e job; update the notify-on-failure job
(notify-on-failure) to include "issue-2478-crash-loop-recovery-e2e" in its needs
array so the notifier waits on and includes failures from that job in the
auto-issue path.

src/nemoclaw.ts (1)

289-314: Consider collapsing this fallback onto the shared recovery-script builder.

This block now mirrors src/lib/agent-runtime.ts almost verbatim. Keeping the warning strings, ordering, and NODE_OPTIONS checks in two places makes the next preload-chain change easy to miss in one path.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/nemoclaw.ts` around lines 289 - 314, This duplicates the recovery-launch
shell block found in src/lib/agent-runtime.ts (see buildRecoveryScript); instead
of maintaining two nearly identical arrays, refactor src/nemoclaw.ts to
import/use the shared builder (buildRecoveryScript) or a common helper that
returns the recovery script/command list, and replace the inline commands (the
array containing checks for /tmp/nemoclaw-proxy-env.sh, ~/.bashrc, NODE_OPTIONS
guard check, warnings, OPENCLAW lookup and nohup launch) with a call to that
shared builder so the warning strings, ordering, and NODE_OPTIONS logic are
maintained in one place (also update any callers that expect the current array
shape to use the unified output, and ensure compatibility with
executeSandboxCommand if used).

.github/workflows/nightly-e2e.yaml (1)

197-228: ⚠️ Potential issue | 🟠 Major

Add the new #2478 job to notify-on-failure.needs.

Line 201 introduces issue-2478-crash-loop-recovery-e2e, but notify-on-failure (Line 777-Line 797) still excludes it. Failures from this job can be missed by notifier sequencing/reporting.

🔧 Proposed patch

   notify-on-failure:
     runs-on: ubuntu-latest
     needs:
       [
         cloud-e2e,
         messaging-providers-e2e,
         token-rotation-e2e,
         sandbox-survival-e2e,
+        issue-2478-crash-loop-recovery-e2e,
         hermes-e2e,
         skip-permissions-e2e,
         sandbox-operations-e2e,
         inference-routing-e2e,
         network-policy-e2e,
         deployment-services-e2e,
         diagnostics-e2e,
         snapshot-commands-e2e,
         shields-config-e2e,
         rebuild-openclaw-e2e,
         upgrade-stale-sandbox-e2e,
         rebuild-hermes-e2e,
         overlayfs-autofix-e2e,
         gpu-e2e,
         gpu-double-onboard-e2e,
       ]

As per coding guidelines: "Keeping notify-on-failure updated: include the new job in the needs list so failures trigger the failure issue/reporting."

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In @.github/workflows/nightly-e2e.yaml around lines 197 - 228, The new workflow
job issue-2478-crash-loop-recovery-e2e is not included in the needs list for the
notify-on-failure job; update the notify-on-failure job's needs array to include
"issue-2478-crash-loop-recovery-e2e" so failures from that job trigger the
notifier sequencing/reporting (locate notify-on-failure and add the exact job
name to its needs list).

Fix all unresolved CodeRabbit comments on this PR:

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

src/nemoclaw.ts (1)

292-319: Recommend centralizing this fallback script to avoid drift with agent-runtime.ts.

This block now mirrors src/lib/agent-runtime.ts:52-123 very closely. Consolidating into one shared builder would reduce future regression risk when guard-chain behavior changes again.

♻️ Refactor direction

-  const script =
-    agentScript ||
-    [
-      // inline OpenClaw recovery script...
-    ].join(" ");
+  const script =
+    agentScript ||
+    agentRuntime.buildOpenclawRecoveryScript({
+      dashboardPort: DASHBOARD_PORT,
+      gatewayLogPath: "/tmp/gateway.log",
+    });

// src/lib/agent-runtime.ts
export function buildOpenclawRecoveryScript(opts: {
  dashboardPort: number;
  gatewayLogPath: string;
}): string {
  // single source of truth for OpenClaw fallback recovery chain
}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/nemoclaw.ts` around lines 292 - 319, The recovery-script block in
src/nemoclaw.ts duplicates logic from src/lib/agent-runtime.ts; extract it into
a single exported builder function e.g.
buildOpenclawRecoveryScript({dashboardPort, gatewayLogPath}) in
src/lib/agent-runtime.ts that returns the full joined script string (preserving
all checks, touch/chmod, warnings, nohup/GPID logic), then import and call that
function from src/nemoclaw.ts (pass DASHBOARD_PORT and "/tmp/gateway.log" or
configurable gatewayLogPath) replacing the inline array.join(" ") block; ensure
the new function is exported and tests/uses still behave the same (keep variable
names like OPENCLAW, GPID, and the exact warning texts).