Skip to content

fix(sandbox): respawn gateway after watchdog SIGTERM instead of exiting PID 1 - #6131

Merged
apurvvkumaria merged 4 commits into
mainfrom
fix/sandbox-gateway-respawn-clean-exit
Jul 1, 2026
Merged

fix(sandbox): respawn gateway after watchdog SIGTERM instead of exiting PID 1#6131
apurvvkumaria merged 4 commits into
mainfrom
fix/sandbox-gateway-respawn-clean-exit

Conversation

@laitingsheng

@laitingsheng laitingsheng commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

Summary

The in-sandbox OpenClaw gateway exits spontaneously on v0.0.71 and the sandbox is torn down, so the TUI and slash commands fail with "OpenClaw gateway is not running inside the sandbox (sandbox likely restarted)" and recovery also fails. This restores the respawn supervisor's intent so a watchdog-driven gateway kill relaunches the gateway instead of stopping PID 1.

Related Issue

Fixes #6107

Changes

  • The serving watchdog SIGTERMs a gateway that has dropped its HTTP listener so the respawn loop can relaunch it, but OpenClaw exits 0 on a graceful SIGTERM. Both PID-1 respawn loops treated a clean rc=0 exit as an operator-requested shutdown and called exit 0, tearing down the whole sandbox.
  • The watchdog now records an identity-scoped kill marker (<pid>:<start-identity>) immediately before its kill -TERM.
  • Both respawn loops (root and non-root) exit PID 1 on rc=0 only when the exit was not a watchdog kill; a watchdog-induced clean exit respawns, while a genuine operator clean exit still stops the sandbox.
  • The marker is consumed once and scoped to the exact pid and start identity, so a stale marker cannot force an unwanted respawn and self-clears on a non-matching read.
  • New unit coverage for the marker record/consume helpers (test/gateway-watchdog-kill-marker.test.ts).

Type of Change

  • Code change (feature, bug fix, or refactor)
  • Code change with doc updates
  • Doc only (prose changes, no code sample modifications)
  • Doc only (includes code sample changes)

Quality Gates

  • Tests added or updated for changed behavior
  • Existing tests cover changed behavior — justification:
  • Tests not applicable — justification:
  • Docs updated for user-facing behavior changes
  • Docs not applicable — justification:
  • Sensitive paths changed (security, policy, credentials, preflight, onboarding, inference, runner, sandbox, or messaging)
  • Sensitive-path review completed or maintainer-approved waiver recorded — reviewer/approval link/justification:
  • Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue:

Verification

  • PR description includes the DCO sign-off declaration and every commit appears as Verified in GitHub
  • Git hooks passed during commit and push, or npx prek run --from-ref main --to-ref HEAD passes
  • Targeted tests pass for changed behavior
  • Full npm test passes (broad runtime changes only)
  • Quality Gates section completed with required justifications or waivers
  • No secrets, API keys, or credentials committed
  • npm run docs builds without warnings (doc changes only)
  • Doc pages follow the style guide (doc changes only)
  • New doc pages include SPDX header and frontmatter (new pages only)

Signed-off-by: Tinson Lai tinsonl@nvidia.com

Summary by CodeRabbit

  • Bug Fixes
    • Improved gateway shutdown handling: intentional watchdog-triggered listener loss no longer stops the whole container when the event is recognized as safe.
    • Added a configurable watchdog kill-marker mechanism (via an environment setting) to ensure the gateway can restart without false container exits.
  • Tests
    • Added automated coverage for kill-marker matching/mismatching, missing/empty marker cases, and single-use consumption.
    • Validated respawn vs. teardown behavior for timing-sensitive gateway watchdog scenarios.

…ng PID 1

The serving watchdog SIGTERMs a gateway that dropped its HTTP listener so
the respawn loop can relaunch it, but OpenClaw exits 0 on a graceful
SIGTERM. Both respawn loops treat a clean rc=0 exit as an operator-requested
shutdown and exit PID 1, tearing down the whole sandbox instead of
respawning it.

Record an identity-scoped kill marker before the watchdog SIGTERM and
consume it in both respawn loops, so a watchdog-induced clean exit respawns
while a genuine operator clean exit still stops the sandbox. The marker is
scoped to the exact pid and start identity being killed and consumed once,
so a stale marker cannot force an unwanted respawn.

Co-Authored-By: Claude <noreply@anthropic.com>
Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
@coderabbitai

coderabbitai Bot commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

Adds a kill marker file in scripts/nemoclaw-start.sh so the gateway watchdog can record intentional kills, and updates both respawn loops to consume that marker before deciding whether to exit. Adds a test suite covering marker matching and respawn behavior.

Changes

Watchdog kill marker

Layer / File(s) Summary
Kill marker configuration and helpers
scripts/nemoclaw-start.sh
Adds GATEWAY_WATCHDOG_KILL_FILE config (default /tmp/nemoclaw-gateway-watchdog-kill) and record_gateway_watchdog_kill/consume_gateway_watchdog_kill helper functions.
Watchdog records kill identity
scripts/nemoclaw-start.sh
start_gateway_serving_watchdog records the tracked gateway identity into the marker file immediately before terminating the gateway.
Respawn loops consume marker to gate exit
scripts/nemoclaw-start.sh
Non-root and root gateway respawn loops now check consume_gateway_watchdog_kill on RC==0 exits, continuing the loop when the marker matches the exited gateway identity.
Marker behavior test suite
test/gateway-watchdog-kill-marker.test.ts
Adds a Vitest suite that extracts shell functions from the script and validates match, non-match, missing/empty identity, single-use consumption, marker clearing, and respawn behavior.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Suggested labels: area: sandbox, bug-fix

Suggested reviewers: ericksoa

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly describes the main fix: gateway respawn after watchdog SIGTERM instead of PID 1 exiting.
Linked Issues check ✅ Passed The changes address #6107 by preserving sandbox uptime and respawning the gateway when the watchdog stops it.
Out of Scope Changes check ✅ Passed The code and tests stay focused on watchdog-triggered gateway respawn and marker handling, with no obvious unrelated additions.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/sandbox-gateway-respawn-clean-exit

Comment @coderabbitai help to get the list of available commands.

@github-code-quality

github-code-quality Bot commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

Code Coverage Overview

Languages: TypeScript

TypeScript / code-coverage/plugin

The overall coverage in the fix/sandbox-gateway-... branch is 96%. Coverage data for the main branch is not yet available.

Show a code coverage summary of the most covered files.
File main fix/sandbox-gateway-... 233299e +/-
nemoclaw/src/se...cret-scanner.ts 100%
nemoclaw/src/commands/slash.ts 100%
nemoclaw/src/li...bprocess-env.ts 100%
nemoclaw/src/bl...eprint/state.ts 98%
nemoclaw/src/onboard/config.ts 98%
nemoclaw/src/bl...int/snapshot.ts 97%
nemoclaw/src/bl...print/runner.ts 95%
nemoclaw/src/co...ration-state.ts 94%
nemoclaw/src/bl...ate-networks.ts 94%
nemoclaw/src/index.ts 94%

TypeScript / code-coverage/cli

The overall coverage in the fix/sandbox-gateway-... branch is 68%. Coverage data for the main branch is not yet available.

Show a code coverage summary of the most covered files.
File main fix/sandbox-gateway-... 233299e +/-
src/lib/shields...nsition-lock.ts 86%
src/lib/actions...dbox/rebuild.ts 80%
src/lib/actions...all/run-plan.ts 80%
src/lib/state/o...oard-session.ts 80%
src/lib/state/sandbox.ts 72%
src/lib/onboard/preflight.ts 69%
src/lib/shields/index.ts 67%
src/lib/onboard...er-gpu-patch.ts 59%
src/lib/actions...licy-channel.ts 58%
src/lib/onboard.ts 20%

Updated July 01, 2026 17:55 UTC
Code Coverage is in Public Preview. Learn more and provide us with your feedback.

@github-actions

github-actions Bot commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

PR Review Advisor — Changes requested

Merge posture: Do not merge yet
Primary next action: Resolve or justify PRA-1: Source-of-truth review needed: Watchdog kill marker used to reinterpret clean gateway exits.
Open items: 0 required · 4 warnings · 0 suggestions · 8 test follow-ups
Since last review: 1 prior item resolved · 3 still apply · 0 new items found

Action checklist

  • PRA-1 Resolve or justify: Source-of-truth review needed: Watchdog kill marker used to reinterpret clean gateway exits
  • PRA-2 Resolve or justify: Harden the watchdog kill marker before trusting it to override clean gateway exits in scripts/nemoclaw-start.sh:257
  • PRA-3 Resolve or justify: Do not SIGTERM the gateway after an unrecorded watchdog marker failure in scripts/nemoclaw-start.sh:366
  • PRA-4 Resolve or justify: Complete the source-of-truth contract for watchdog-marked clean exits in scripts/nemoclaw-start.sh:364
  • PRA-T1 Add or justify test follow-up: Runtime validation
  • PRA-T2 Add or justify test follow-up: Runtime validation
  • PRA-T3 Add or justify test follow-up: Runtime validation
  • PRA-T4 Add or justify test follow-up: Runtime validation
  • PRA-T5 Add or justify test follow-up: Runtime validation
  • PRA-T6 Add or justify test follow-up: Acceptance clause
  • PRA-T7 Add or justify test follow-up: Acceptance clause
  • PRA-T8 Add or justify test follow-up: Acceptance clause

Findings index

ID Severity Category Location Required action
PRA-1 Resolve/justify architecture Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
PRA-2 Resolve/justify security scripts/nemoclaw-start.sh:257 Keep the exact identity match, and either move the marker into a private PID1-owned runtime/control directory or validate the marker before trusting it: require the expected trusted path, a regular non-symlink file, expected owner for root/non-root mode, and mode 0600. Apply the same validation to `_NEMOCLAW_GATEWAY_WATCHDOG_KILL_FILE` or constrain that override to test-only harnesses.
PRA-3 Resolve/justify correctness scripts/nemoclaw-start.sh:366 Make marker persistence part of the trusted kill precondition. For example, have `record_gateway_watchdog_kill` return success/failure and, if it cannot persist a trusted marker, skip the graceful SIGTERM and log a fail-closed warning or use a restart path that cannot be mistaken for an operator clean exit.
PRA-4 Resolve/justify architecture scripts/nemoclaw-start.sh:364 Document the contract near the marker helpers or watchdog loop: the invalid state, the trusted producer, why the source cannot be fixed in the current watchdog/reap path, and the removal condition. Add focused root-loop and runtime-level validation so both PID-1 loop shapes preserve the distinction between watchdog-induced clean exits and genuine operator clean exits.
Review findings by urgency: 0 required fixes, 4 items to resolve/justify, 0 in-scope improvements

⚠️ Resolve or justify before merge

Investigate these in the current review; either fix them, explain why they are not applicable, or document the accepted risk.

PRA-1 Resolve/justify — Source-of-truth review needed: Watchdog kill marker used to reinterpret clean gateway exits

  • Location: not file-specific
  • Category: architecture
  • Problem: The advisor marked localized patch analysis as needs_followup.
  • Impact: A localized workaround can preserve or hide an invalid state when the source boundary is unclear.
  • Recommended action: Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Missing regression test: Partial: the new tests cover marker helper semantics and the extracted non-root critical section. Missing: root-loop coverage and runtime validation of the actual watchdog-to-parent-loop handoff.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Evidence: `record_gateway_watchdog_kill "$tracked_identity"` is called immediately before `kill -TERM`, and both respawn loops use `consume_gateway_watchdog_kill` to override the `RC=0` clean-exit decision.

PRA-2 Resolve/justify — Harden the watchdog kill marker before trusting it to override clean gateway exits

  • Location: scripts/nemoclaw-start.sh:257
  • Category: security
  • Problem: The respawn decision now depends on `GATEWAY_WATCHDOG_KILL_FILE`, defaulting to `/tmp/nemoclaw-gateway-watchdog-kill`. `consume_gateway_watchdog_kill` checks `-f`, reads one line, removes the path, and compares the text to the expected `pid:start_identity`, but it does not validate that the marker came from the trusted watchdog/PID-1 path with an expected directory, regular non-symlink shape, owner, and mode.
  • Impact: A stale or attacker-planted matching marker in shared `/tmp` could cause a genuine clean gateway exit to be treated as watchdog-induced and force an unintended respawn. The exact PID/starttime match reduces exploitability, but this is sandbox lifecycle authority and could become a policy/availability bypass if the identity becomes guessable or a later refactor weakens the match.
  • Recommended action: Keep the exact identity match, and either move the marker into a private PID1-owned runtime/control directory or validate the marker before trusting it: require the expected trusted path, a regular non-symlink file, expected owner for root/non-root mode, and mode 0600. Apply the same validation to `_NEMOCLAW_GATEWAY_WATCHDOG_KILL_FILE` or constrain that override to test-only harnesses.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Read `scripts/nemoclaw-start.sh` around the `GATEWAY_WATCHDOG_KILL_FILE` assignment and `consume_gateway_watchdog_kill`; confirm whether path, file type, symlink status, owner, and permissions are checked before `RC=0` is converted from `exit 0` to respawn.
  • Missing regression test: Add a shell/Vitest negative test that pre-creates a matching `pid:start_identity` marker with an untrusted path shape, wrong mode, wrong owner where practical, or symlink indirection, then verifies a clean `RC=0` path does not respawn unless the marker was produced through the trusted watchdog path.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Read `scripts/nemoclaw-start.sh` around the `GATEWAY_WATCHDOG_KILL_FILE` assignment and `consume_gateway_watchdog_kill`; confirm whether path, file type, symlink status, owner, and permissions are checked before `RC=0` is converted from `exit 0` to respawn.
  • Evidence: `GATEWAY_WATCHDOG_KILL_FILE="${_NEMOCLAW_GATEWAY_WATCHDOG_KILL_FILE:-/tmp/nemoclaw-gateway-watchdog-kill}"`; `consume_gateway_watchdog_kill` performs `[ -f "$GATEWAY_WATCHDOG_KILL_FILE" ]`, `read`, `rm -f`, and string equality only.

PRA-3 Resolve/justify — Do not SIGTERM the gateway after an unrecorded watchdog marker failure

  • Location: scripts/nemoclaw-start.sh:366
  • Category: correctness
  • Problem: `record_gateway_watchdog_kill` hides write failures with `best-effort ... || true`, and the watchdog unconditionally sends `kill -TERM` immediately afterward. If the marker cannot be persisted, OpenClaw can still handle SIGTERM gracefully and exit with status 0, leaving the parent loop with no marker and the old `exit 0` teardown behavior.
  • Impact: The primary fix can fail in the reported failure mode when the marker write fails or the override path is unusable: watchdog SIGTERM, gateway returns 0, PID 1 exits, and the sandbox is torn down instead of respawning the gateway.
  • Recommended action: Make marker persistence part of the trusted kill precondition. For example, have `record_gateway_watchdog_kill` return success/failure and, if it cannot persist a trusted marker, skip the graceful SIGTERM and log a fail-closed warning or use a restart path that cannot be mistaken for an operator clean exit.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Inspect `record_gateway_watchdog_kill` and `start_gateway_serving_watchdog` around the `record_gateway_watchdog_kill "$tracked_identity"` and `kill -TERM "$pid"` sequence; confirm the watchdog only sends a clean-termination signal after the marker is durably recorded and trusted.
  • Missing regression test: Add a shell/Vitest test that forces marker recording to fail, then verifies the watchdog does not proceed with a graceful SIGTERM that can produce `RC=0` without a matching marker; the test should assert the fail-closed log or alternate restart path.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Inspect `record_gateway_watchdog_kill` and `start_gateway_serving_watchdog` around the `record_gateway_watchdog_kill "$tracked_identity"` and `kill -TERM "$pid"` sequence; confirm the watchdog only sends a clean-termination signal after the marker is durably recorded and trusted.
  • Evidence: `record_gateway_watchdog_kill` pipes into `_nemoclaw_safe_replace_tmp_file "$GATEWAY_WATCHDOG_KILL_FILE" 600 "" best-effort 2>/dev/null || true`, and `start_gateway_serving_watchdog` calls `kill -TERM "$pid"` unconditionally after recording.

PRA-4 Resolve/justify — Complete the source-of-truth contract for watchdog-marked clean exits

  • Location: scripts/nemoclaw-start.sh:364
  • Category: architecture
  • Problem: The PR adds a localized marker channel so the respawn loop can reinterpret `RC=0` exits as watchdog-induced rather than operator-requested. The invalid state is identifiable, and the new tests now cover helper behavior plus the non-root critical section, but the code still does not document why this marker is the durable source of truth, why the source cannot be fixed at the watchdog/reap contract, or when the workaround can be removed. The root loop remains untested by the added harness.
  • Impact: Without a clear source-boundary contract and root/runtime regression coverage, future lifecycle changes can silently reintroduce PID 1 teardown after watchdog SIGTERM or convert genuine operator clean exits into respawns, especially because `scripts/nemoclaw-start.sh` has many active overlapping sandbox lifecycle changes.
  • Recommended action: Document the contract near the marker helpers or watchdog loop: the invalid state, the trusted producer, why the source cannot be fixed in the current watchdog/reap path, and the removal condition. Add focused root-loop and runtime-level validation so both PID-1 loop shapes preserve the distinction between watchdog-induced clean exits and genuine operator clean exits.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Inspect `record_gateway_watchdog_kill`, `consume_gateway_watchdog_kill`, `start_gateway_serving_watchdog`, and the two `RC=0` branches; confirm the comments identify the source boundary/removal condition and that tests exercise both non-root `wait` and root `openclaw_reap_exited_gateway` paths.
  • Missing regression test: Add a root-loop shell harness where `openclaw_reap_exited_gateway` succeeds with `OPENCLAW_REAP_EXIT_STATUS=0`: with a matching original `pid:start_identity` marker the loop reaches the respawn path, and with no marker it exits cleanly. Also add or identify runtime validation where an actual watchdog-marked SIGTERM of a gateway that exits 0 causes a respawn without PID 1 exiting.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Inspect `record_gateway_watchdog_kill`, `consume_gateway_watchdog_kill`, `start_gateway_serving_watchdog`, and the two `RC=0` branches; confirm the comments identify the source boundary/removal condition and that tests exercise both non-root `wait` and root `openclaw_reap_exited_gateway` paths.
  • Evidence: Both respawn loops now use `! consume_gateway_watchdog_kill "${EXITED_GATEWAY_PID}:${EXITED_GATEWAY_START_IDENTITY}"` to decide whether `RC=0` exits PID 1. `test/gateway-watchdog-kill-marker.test.ts` extracts the first non-root critical section, but does not exercise the root `openclaw_reap_exited_gateway` branch or a real watchdog-to-parent-loop handoff.

💡 In-scope improvements

These are lower-risk, not throwaway. Prefer fixing them in this PR when they are local to changed code; defer only with rationale or a linked follow-up.

  • None.
Test follow-ups to resolve or justify

If these cover changed behavior, prefer adding them in this PR; otherwise state why existing coverage is enough or link the follow-up.

  • PRA-T1 Runtime validation — Root loop: when `openclaw_reap_exited_gateway` reports `OPENCLAW_REAP_EXIT_STATUS=0` and a trusted marker matches the snapshotted `pid:start_identity`, PID 1 takes the respawn path; with no marker it exits cleanly.. The PR touches sandbox PID-1 lifecycle behavior in `scripts/nemoclaw-start.sh`. The added Vitest shell harness is valuable and covers helper behavior plus the non-root critical section, but the root loop, marker trust negative cases, marker write failure, and a real watchdog SIGTERM-to-respawn handoff remain unproven statically.
  • PRA-T2 Runtime validation — Marker write failure: when recording the watchdog marker fails, the watchdog does not send a graceful SIGTERM that can be mistaken for an operator clean exit, and logs or follows a fail-closed restart path.. The PR touches sandbox PID-1 lifecycle behavior in `scripts/nemoclaw-start.sh`. The added Vitest shell harness is valuable and covers helper behavior plus the non-root critical section, but the root loop, marker trust negative cases, marker write failure, and a real watchdog SIGTERM-to-respawn handoff remain unproven statically.
  • PRA-T3 Runtime validation — Tampered marker: a preexisting matching marker with wrong mode, untrusted path shape, symlink indirection, or wrong owner is rejected and a genuine clean `RC=0` exit does not respawn.. The PR touches sandbox PID-1 lifecycle behavior in `scripts/nemoclaw-start.sh`. The added Vitest shell harness is valuable and covers helper behavior plus the non-root critical section, but the root loop, marker trust negative cases, marker write failure, and a real watchdog SIGTERM-to-respawn handoff remain unproven statically.
  • PRA-T4 Runtime validation — Runtime lifecycle: in a sandbox entrypoint scenario, the serving watchdog records a trusted marker, SIGTERMs a gateway that exits 0, and PID 1 respawns the gateway without exiting.. The PR touches sandbox PID-1 lifecycle behavior in `scripts/nemoclaw-start.sh`. The added Vitest shell harness is valuable and covers helper behavior plus the non-root critical section, but the root loop, marker trust negative cases, marker write failure, and a real watchdog SIGTERM-to-respawn handoff remain unproven statically.
  • PRA-T5 Runtime validation — Snapshot contract: after the gateway exit is observed and cleanup mutates `GATEWAY_PID_START_IDENTITY`, the loop still consumes against the snapshotted original identity in both root and non-root paths.. The PR touches sandbox PID-1 lifecycle behavior in `scripts/nemoclaw-start.sh`. The added Vitest shell harness is valuable and covers helper behavior plus the non-root critical section, but the root loop, marker trust negative cases, marker write failure, and a real watchdog SIGTERM-to-respawn handoff remain unproven statically.
  • PRA-T6 Acceptance clause — NemoClaw v0.0.71 regression: OpenClaw gateway exits spontaneously inside the sandbox. — add test evidence or identify existing coverage. The diff changes both PID-1 respawn loops so `RC=0` with a matching watchdog marker respawns instead of exiting PID 1; the added test covers this for the extracted non-root critical section, but no root/runtime sandbox validation is present in the diff.
  • PRA-T7 Acceptance clause — TUI and slash commands fail with "OpenClaw gateway is not running inside the sandbox (sandbox likely restarted)". — add test evidence or identify existing coverage. Preventing PID 1 teardown on watchdog-induced clean exits should preserve the in-sandbox gateway, but there is no direct TUI/slash-command or real gateway availability test in the changed files.
  • PRA-T8 Acceptance clause — Recovery also fails. — add test evidence or identify existing coverage. The PR targets the earlier PID-1 teardown condition rather than the recovery flow itself; no recovery-path code or test evidence changed.
Since last review details

Current findings, using the urgency labels above:

PRA-1 Resolve/justify — Source-of-truth review needed: Watchdog kill marker used to reinterpret clean gateway exits

  • Location: not file-specific
  • Category: architecture
  • Problem: The advisor marked localized patch analysis as needs_followup.
  • Impact: A localized workaround can preserve or hide an invalid state when the source boundary is unclear.
  • Recommended action: Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Missing regression test: Partial: the new tests cover marker helper semantics and the extracted non-root critical section. Missing: root-loop coverage and runtime validation of the actual watchdog-to-parent-loop handoff.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Evidence: `record_gateway_watchdog_kill "$tracked_identity"` is called immediately before `kill -TERM`, and both respawn loops use `consume_gateway_watchdog_kill` to override the `RC=0` clean-exit decision.

PRA-2 Resolve/justify — Harden the watchdog kill marker before trusting it to override clean gateway exits

  • Location: scripts/nemoclaw-start.sh:257
  • Category: security
  • Problem: The respawn decision now depends on `GATEWAY_WATCHDOG_KILL_FILE`, defaulting to `/tmp/nemoclaw-gateway-watchdog-kill`. `consume_gateway_watchdog_kill` checks `-f`, reads one line, removes the path, and compares the text to the expected `pid:start_identity`, but it does not validate that the marker came from the trusted watchdog/PID-1 path with an expected directory, regular non-symlink shape, owner, and mode.
  • Impact: A stale or attacker-planted matching marker in shared `/tmp` could cause a genuine clean gateway exit to be treated as watchdog-induced and force an unintended respawn. The exact PID/starttime match reduces exploitability, but this is sandbox lifecycle authority and could become a policy/availability bypass if the identity becomes guessable or a later refactor weakens the match.
  • Recommended action: Keep the exact identity match, and either move the marker into a private PID1-owned runtime/control directory or validate the marker before trusting it: require the expected trusted path, a regular non-symlink file, expected owner for root/non-root mode, and mode 0600. Apply the same validation to `_NEMOCLAW_GATEWAY_WATCHDOG_KILL_FILE` or constrain that override to test-only harnesses.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Read `scripts/nemoclaw-start.sh` around the `GATEWAY_WATCHDOG_KILL_FILE` assignment and `consume_gateway_watchdog_kill`; confirm whether path, file type, symlink status, owner, and permissions are checked before `RC=0` is converted from `exit 0` to respawn.
  • Missing regression test: Add a shell/Vitest negative test that pre-creates a matching `pid:start_identity` marker with an untrusted path shape, wrong mode, wrong owner where practical, or symlink indirection, then verifies a clean `RC=0` path does not respawn unless the marker was produced through the trusted watchdog path.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Read `scripts/nemoclaw-start.sh` around the `GATEWAY_WATCHDOG_KILL_FILE` assignment and `consume_gateway_watchdog_kill`; confirm whether path, file type, symlink status, owner, and permissions are checked before `RC=0` is converted from `exit 0` to respawn.
  • Evidence: `GATEWAY_WATCHDOG_KILL_FILE="${_NEMOCLAW_GATEWAY_WATCHDOG_KILL_FILE:-/tmp/nemoclaw-gateway-watchdog-kill}"`; `consume_gateway_watchdog_kill` performs `[ -f "$GATEWAY_WATCHDOG_KILL_FILE" ]`, `read`, `rm -f`, and string equality only.

PRA-3 Resolve/justify — Do not SIGTERM the gateway after an unrecorded watchdog marker failure

  • Location: scripts/nemoclaw-start.sh:366
  • Category: correctness
  • Problem: `record_gateway_watchdog_kill` hides write failures with `best-effort ... || true`, and the watchdog unconditionally sends `kill -TERM` immediately afterward. If the marker cannot be persisted, OpenClaw can still handle SIGTERM gracefully and exit with status 0, leaving the parent loop with no marker and the old `exit 0` teardown behavior.
  • Impact: The primary fix can fail in the reported failure mode when the marker write fails or the override path is unusable: watchdog SIGTERM, gateway returns 0, PID 1 exits, and the sandbox is torn down instead of respawning the gateway.
  • Recommended action: Make marker persistence part of the trusted kill precondition. For example, have `record_gateway_watchdog_kill` return success/failure and, if it cannot persist a trusted marker, skip the graceful SIGTERM and log a fail-closed warning or use a restart path that cannot be mistaken for an operator clean exit.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Inspect `record_gateway_watchdog_kill` and `start_gateway_serving_watchdog` around the `record_gateway_watchdog_kill "$tracked_identity"` and `kill -TERM "$pid"` sequence; confirm the watchdog only sends a clean-termination signal after the marker is durably recorded and trusted.
  • Missing regression test: Add a shell/Vitest test that forces marker recording to fail, then verifies the watchdog does not proceed with a graceful SIGTERM that can produce `RC=0` without a matching marker; the test should assert the fail-closed log or alternate restart path.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Inspect `record_gateway_watchdog_kill` and `start_gateway_serving_watchdog` around the `record_gateway_watchdog_kill "$tracked_identity"` and `kill -TERM "$pid"` sequence; confirm the watchdog only sends a clean-termination signal after the marker is durably recorded and trusted.
  • Evidence: `record_gateway_watchdog_kill` pipes into `_nemoclaw_safe_replace_tmp_file "$GATEWAY_WATCHDOG_KILL_FILE" 600 "" best-effort 2>/dev/null || true`, and `start_gateway_serving_watchdog` calls `kill -TERM "$pid"` unconditionally after recording.

PRA-4 Resolve/justify — Complete the source-of-truth contract for watchdog-marked clean exits

  • Location: scripts/nemoclaw-start.sh:364
  • Category: architecture
  • Problem: The PR adds a localized marker channel so the respawn loop can reinterpret `RC=0` exits as watchdog-induced rather than operator-requested. The invalid state is identifiable, and the new tests now cover helper behavior plus the non-root critical section, but the code still does not document why this marker is the durable source of truth, why the source cannot be fixed at the watchdog/reap contract, or when the workaround can be removed. The root loop remains untested by the added harness.
  • Impact: Without a clear source-boundary contract and root/runtime regression coverage, future lifecycle changes can silently reintroduce PID 1 teardown after watchdog SIGTERM or convert genuine operator clean exits into respawns, especially because `scripts/nemoclaw-start.sh` has many active overlapping sandbox lifecycle changes.
  • Recommended action: Document the contract near the marker helpers or watchdog loop: the invalid state, the trusted producer, why the source cannot be fixed in the current watchdog/reap path, and the removal condition. Add focused root-loop and runtime-level validation so both PID-1 loop shapes preserve the distinction between watchdog-induced clean exits and genuine operator clean exits.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Inspect `record_gateway_watchdog_kill`, `consume_gateway_watchdog_kill`, `start_gateway_serving_watchdog`, and the two `RC=0` branches; confirm the comments identify the source boundary/removal condition and that tests exercise both non-root `wait` and root `openclaw_reap_exited_gateway` paths.
  • Missing regression test: Add a root-loop shell harness where `openclaw_reap_exited_gateway` succeeds with `OPENCLAW_REAP_EXIT_STATUS=0`: with a matching original `pid:start_identity` marker the loop reaches the respawn path, and with no marker it exits cleanly. Also add or identify runtime validation where an actual watchdog-marked SIGTERM of a gateway that exits 0 causes a respawn without PID 1 exiting.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Inspect `record_gateway_watchdog_kill`, `consume_gateway_watchdog_kill`, `start_gateway_serving_watchdog`, and the two `RC=0` branches; confirm the comments identify the source boundary/removal condition and that tests exercise both non-root `wait` and root `openclaw_reap_exited_gateway` paths.
  • Evidence: Both respawn loops now use `! consume_gateway_watchdog_kill "${EXITED_GATEWAY_PID}:${EXITED_GATEWAY_START_IDENTITY}"` to decide whether `RC=0` exits PID 1. `test/gateway-watchdog-kill-marker.test.ts` extracts the first non-root critical section, but does not exercise the root `openclaw_reap_exited_gateway` branch or a real watchdog-to-parent-loop handoff.

Workflow run details

This is an automated, non-binding review; it still expects maintainers and agents to respond to each required or warning item. Treat suggestions as current-PR improvements when they touch changed code; defer only with maintainer rationale or a linked follow-up. A human maintainer must make the final merge decision.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (2)
test/gateway-watchdog-kill-marker.test.ts (2)

37-50: 🎯 Functional Correctness | 🔵 Trivial | 🏗️ Heavy lift

Marker tests don't exercise the actual respawn-loop integration.

runMarkerScenario extracts and runs record_gateway_watchdog_kill/consume_gateway_watchdog_kill/_nemoclaw_safe_replace_tmp_file in an isolated bash harness, decoupled from the real respawn loops in scripts/nemoclaw-start.sh (context snippet 3, lines 4809-4820) that actually call consume_gateway_watchdog_kill "${EXITED_GATEWAY_PID}:${GATEWAY_PID_START_IDENTITY}". This validates the marker helpers' internal contract well, but a wiring regression in the respawn loop itself (e.g., wrong identity string format, calling the function with swapped arguments, or forgetting to guard on RC -eq 0) wouldn't be caught by any test here. Per path instructions for test files, prefer observable outcomes through the public boundary over private-shape assertions — an end-to-end scenario driving the actual respawn loop (or at least the while :; do ... done block from context snippet 3) would give stronger behavioral confidence for the fix this PR is making.

As per path instructions, "Prefer observable outcomes through the public boundary over source-text, private-shape, or mock-call assertions."

🤖 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/gateway-watchdog-kill-marker.test.ts` around lines 37 - 50, The marker
test is only validating the helper functions in isolation, so it can miss wiring
bugs in the real respawn loop. Update the test around runMarkerScenario to
exercise the actual scripts/nemoclaw-start.sh respawn path (the while-loop that
calls consume_gateway_watchdog_kill with
EXITED_GATEWAY_PID:GATEWAY_PID_START_IDENTITY) and assert observable behavior
through the public boundary instead of extracting record_gateway_watchdog_kill,
consume_gateway_watchdog_kill, and _nemoclaw_safe_replace_tmp_file directly.
Keep the test focused on end-to-end outcomes that prove the marker is consumed
correctly by the respawn-loop integration.

Source: Path instructions


22-35: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Brace-matching extraction is fragile for future edits to the shell script.

extractShellFunction finds the closing brace by looking for a line that is exactly "}". If record_gateway_watchdog_kill, consume_gateway_watchdog_kill, or _nemoclaw_safe_replace_tmp_file ever gain an indented nested block (e.g. an if/case whose closing brace/fi isn't the very last unindented }), or another function definition sneaks in between, this silently extracts the wrong span (or throws a confusing "closing brace" error) without any indication that the shell function's actual structure changed. This is acceptable for the current flat function bodies, but it's a maintenance trap.

♻️ Suggested hardening
-  const endIndex = requireNonNegative(
-    lines.findIndex((line, index) => index > 0 && line === "}"),
-    `function ${name} missing closing brace in ${scriptPath}`,
-  );
+  const endIndex = requireNonNegative(
+    lines.findIndex((line, index) => index > 0 && /^}\s*$/.test(line)),
+    `function ${name} missing closing brace in ${scriptPath}`,
+  );
🤖 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/gateway-watchdog-kill-marker.test.ts` around lines 22 - 35, The
shell-function extraction in extractShellFunction is too brittle because it
assumes the closing brace is a line containing only “}”. Update the helper so it
locates the matching end of the function body more robustly for
record_gateway_watchdog_kill, consume_gateway_watchdog_kill, and
_nemoclaw_safe_replace_tmp_file, rather than relying on a single unindented
brace line. Use the existing function-name marker and add structure-aware
matching (or a safer parsing strategy) so nested blocks or intervening content
do not produce incorrect spans or misleading errors.
🤖 Prompt for all review comments with 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.

Inline comments:
In `@scripts/nemoclaw-start.sh`:
- Around line 4817-4818: The gateway stop handling in
mark_openclaw_gateway_stopped clears GATEWAY_PID_START_IDENTITY too early, so
consume_gateway_watchdog_kill receives an incomplete "${EXITED_GATEWAY_PID}:"
key and cannot match the watchdog record. Preserve the exited gateway start
identity in a temporary variable, or defer clearing GATEWAY_PID_START_IDENTITY
until after the consume_gateway_watchdog_kill call in the respawn loops, so the
full "${pid}:${start_identity}" value is used before deciding whether to exit 0.

---

Nitpick comments:
In `@test/gateway-watchdog-kill-marker.test.ts`:
- Around line 37-50: The marker test is only validating the helper functions in
isolation, so it can miss wiring bugs in the real respawn loop. Update the test
around runMarkerScenario to exercise the actual scripts/nemoclaw-start.sh
respawn path (the while-loop that calls consume_gateway_watchdog_kill with
EXITED_GATEWAY_PID:GATEWAY_PID_START_IDENTITY) and assert observable behavior
through the public boundary instead of extracting record_gateway_watchdog_kill,
consume_gateway_watchdog_kill, and _nemoclaw_safe_replace_tmp_file directly.
Keep the test focused on end-to-end outcomes that prove the marker is consumed
correctly by the respawn-loop integration.
- Around line 22-35: The shell-function extraction in extractShellFunction is
too brittle because it assumes the closing brace is a line containing only “}”.
Update the helper so it locates the matching end of the function body more
robustly for record_gateway_watchdog_kill, consume_gateway_watchdog_kill, and
_nemoclaw_safe_replace_tmp_file, rather than relying on a single unindented
brace line. Use the existing function-name marker and add structure-aware
matching (or a safer parsing strategy) so nested blocks or intervening content
do not produce incorrect spans or misleading errors.
🪄 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: Enterprise

Run ID: 8c2fb25d-a116-4cf7-b569-3af8ccc64271

📥 Commits

Reviewing files that changed from the base of the PR and between 96aacaa and 4f37e0b.

📒 Files selected for processing (2)
  • scripts/nemoclaw-start.sh
  • test/gateway-watchdog-kill-marker.test.ts

Comment thread scripts/nemoclaw-start.sh Outdated
@github-actions

github-actions Bot commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

PR Review Advisor (Nemotron Ultra) — Changes requested

Merge posture: Do not merge yet
Primary next action: Fix PRA-2: Source-of-truth review needed: record_gateway_watchdog_kill best-effort write; then add or justify PRA-T1.
Open items: 2 required · 4 warnings · 2 suggestions · 6 test follow-ups
Since last review: 1 prior item resolved · 3 still apply · 2 new items found

Action checklist

  • PRA-2 Fix: Source-of-truth review needed: record_gateway_watchdog_kill best-effort write in scripts/nemoclaw-start.sh:364
  • PRA-4 Fix: Best-effort marker write failure mode not explicitly documented in scripts/nemoclaw-start.sh:364
  • PRA-1 Resolve or justify: Source-of-truth review needed: scripts/nemoclaw-start.sh:364 record_gateway_watchdog_kill best-effort write
  • PRA-3 Resolve or justify: Integration test covers marker logic in isolation but not full watchdog→kill→marker→respawn E2E flow in test/gateway-watchdog-kill-marker.test.ts:1
  • PRA-5 Resolve or justify: Internal test seam env var could be redirected if exposed to untrusted input in scripts/nemoclaw-start.sh:257
  • PRA-6 Resolve or justify: High merge conflict risk with 9 open PRs modifying the same file in scripts/nemoclaw-start.sh:1
  • PRA-T1 Add or justify test follow-up: Runtime validation
  • PRA-T2 Add or justify test follow-up: Runtime validation
  • PRA-T3 Add or justify test follow-up: Runtime validation
  • PRA-T4 Add or justify test follow-up: Runtime validation
  • PRA-T5 Add or justify test follow-up: Integration test covers marker logic in isolation but not full watchdog→kill→marker→respawn E2E flow
  • PRA-T6 Add or justify test follow-up: scripts/nemoclaw-start.sh:364 record_gateway_watchdog_kill best-effort write
  • PRA-7 In-scope improvement: Test extracts shell functions by string matching — fragile to refactoring in test/gateway-watchdog-kill-marker.test.ts:1
  • PRA-8 In-scope improvement: Duplicated respawn critical section logic in non-root and root paths in scripts/nemoclaw-start.sh:4819

Findings index

ID Severity Category Location Required action
PRA-1 Resolve/justify architecture Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
PRA-2 Required architecture scripts/nemoclaw-start.sh:364 Add a comment at record_gateway_watchdog_kill (line 364) documenting: the invalid state (/tmp exhaustion), source boundary (watchdog marker write), source-fix constraint (must not block startup), regression test gap (no /tmp exhaustion test), and acceptance rationale. This makes the tradeoff explicit for future maintainers.
PRA-3 Resolve/justify tests test/gateway-watchdog-kill-marker.test.ts:1 Add an integration test following the runWatchdog pattern in test/nemoclaw-start-gateway-health.test.ts that spawns a real gateway, triggers the watchdog, and verifies the full marker handoff and respawn for both non-root and root paths, plus clean-exit verification.
PRA-4 Required correctness scripts/nemoclaw-start.sh:364 Add a comment at record_gateway_watchdog_kill (line 364) explaining: 'Best-effort: a write failure must never block startup. Marker loss falls back to non-zero exit code respawn behavior — safe for SIGTERM (143) but could exit PID 1 if gateway exits 0 on SIGTERM. This tradeoff is accepted because /tmp exhaustion is rare and the gateway does not handle SIGTERM with exit 0.'
PRA-5 Resolve/justify security scripts/nemoclaw-start.sh:257 Strengthen the comment at line 257-258 to explicitly state: 'Production deployments must not expose _NEMOCLAW_GATEWAY_WATCHDOG_KILL_FILE to untrusted input. This is an internal test seam only.'
PRA-6 Resolve/justify scope scripts/nemoclaw-start.sh:1 Rebase and test merge against main before final approval. Ensure no semantic conflicts with in-flight changes to gateway launch, watchdog, or respawn logic. Consider coordinating with authors of overlapping PRs.
PRA-7 Improvement correctness test/gateway-watchdog-kill-marker.test.ts:1 Add a comment in the test file noting the fragility and that updates may be needed if shell function formatting changes. Alternatively, investigate sourcing a minimal library file with just the marker functions.
PRA-8 Improvement correctness scripts/nemoclaw-start.sh:4819 Extract the shared respawn decision logic into a helper function (e.g., decide_gateway_respawn_or_exit) that takes EXITED_GATEWAY_PID, EXITED_GATEWAY_START_IDENTITY, and RC, and returns the action. This would eliminate duplication and ensure consistency.

🚨 Required before merge

Address these before merging unless a maintainer explicitly overrides the advisor with rationale.

PRA-2 Required — Source-of-truth review needed: record_gateway_watchdog_kill best-effort write

  • Location: scripts/nemoclaw-start.sh:364
  • Category: architecture
  • Problem: The watchdog records a kill marker via `_nemoclaw_safe_replace_tmp_file ... best-effort 2>/dev/null || true`. If /tmp is full or unwritable, the marker write silently fails. The respawn loop then relies on exit code fallback: SIGTERM (143) → respawn; exit 0 on SIGTERM → container exit. This localized workaround handles an invalid state (/tmp exhaustion) at the watchdog boundary rather than fixing the source (guaranteed /tmp writability). No regression test covers the /tmp exhaustion scenario. Removal condition is unclear — this is a deliberate tradeoff, not a temporary workaround.
  • Impact: In an extreme /tmp exhaustion scenario combined with a gateway that exits 0 on SIGTERM, the container would exit instead of respawning. Probability is low but the tradeoff is not documented for maintainers.
  • Required action: Add a comment at record_gateway_watchdog_kill (line 364) documenting: the invalid state (/tmp exhaustion), source boundary (watchdog marker write), source-fix constraint (must not block startup), regression test gap (no /tmp exhaustion test), and acceptance rationale. This makes the tradeoff explicit for future maintainers.
  • Expected follow-up: Fix before merge or get explicit maintainer override.
  • Verification: Read scripts/nemoclaw-start.sh:364-368 and respawn loops at lines 4819 and 5104. Confirm the fallback behavior: RC=0 && !consume_marker → exit 0; otherwise respawn.
  • Missing regression test: Integration test simulating /tmp exhaustion during watchdog kill (marker write fails) verifying respawn loop falls back to exit code check (respawns on 143, exits on 0).
  • Done when: The required change is committed and verification passes: Read scripts/nemoclaw-start.sh:364-368 and respawn loops at lines 4819 and 5104. Confirm the fallback behavior: RC=0 && !consume_marker → exit 0; otherwise respawn.
  • Evidence: record_gateway_watchdog_kill uses '_nemoclaw_safe_replace_tmp_file ... best-effort 2>/dev/null || true'; respawn loop checks RC=0 && !consume_marker

PRA-4 Required — Best-effort marker write failure mode not explicitly documented

  • Location: scripts/nemoclaw-start.sh:364
  • Category: correctness
  • Problem: record_gateway_watchdog_kill uses best-effort write (|| true). If /tmp is full/unwritable, the watchdog kills the gateway but fails to record the marker. The respawn loop sees RC=143 (SIGTERM) and respawns anyway — safe. Only if the gateway handles SIGTERM and exits 0 would the container incorrectly exit. This theoretical edge case is not documented in comments at the function definition (line 364) or at the call site (line 4139). The similar record_gateway_pid function has a 'Best-effort: a write failure must never block startup' comment but does not explain the respawn fallback tradeoff.
  • Impact: Future maintainers may not understand why best-effort is acceptable here, potentially leading to incorrect 'fixes' that make the write required and block startup, or missing the edge case where gateway exits 0 on SIGTERM.
  • Required action: Add a comment at record_gateway_watchdog_kill (line 364) explaining: 'Best-effort: a write failure must never block startup. Marker loss falls back to non-zero exit code respawn behavior — safe for SIGTERM (143) but could exit PID 1 if gateway exits 0 on SIGTERM. This tradeoff is accepted because /tmp exhaustion is rare and the gateway does not handle SIGTERM with exit 0.'
  • Expected follow-up: Fix before merge or get explicit maintainer override.
  • Verification: Read scripts/nemoclaw-start.sh:364-368. No explanatory comment exists at the function definition.
  • Missing regression test: None — this is a documented acceptance of a theoretical edge case. The existing tests cover the happy path; a /tmp exhaustion test would be the regression test but is not required if the tradeoff is explicitly documented.
  • Done when: The required change is committed and verification passes: Read scripts/nemoclaw-start.sh:364-368. No explanatory comment exists at the function definition.
  • Evidence: record_gateway_watchdog_kill uses '_nemoclaw_safe_replace_tmp_file ... best-effort 2>/dev/null || true'; respawn loop checks RC=0 && !consume_marker
Review findings by urgency: 2 required fixes, 4 items to resolve/justify, 2 in-scope improvements

⚠️ Resolve or justify before merge

Investigate these in the current review; either fix them, explain why they are not applicable, or document the accepted risk.

PRA-1 Resolve/justify — Source-of-truth review needed: scripts/nemoclaw-start.sh:364 record_gateway_watchdog_kill best-effort write

  • Location: not file-specific
  • Category: architecture
  • Problem: The advisor marked localized patch analysis as needs_followup.
  • Impact: A localized workaround can preserve or hide an invalid state when the source boundary is unclear.
  • Recommended action: Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Missing regression test: None currently — theoretical edge case. A /tmp exhaustion integration test would be the regression test
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Evidence: record_gateway_watchdog_kill uses '_nemoclaw_safe_replace_tmp_file ... best-effort 2>/dev/null || true'; respawn loop checks RC=0 && !consume_marker

PRA-3 Resolve/justify — Integration test covers marker logic in isolation but not full watchdog→kill→marker→respawn E2E flow

  • Location: test/gateway-watchdog-kill-marker.test.ts:1
  • Category: tests
  • Problem: The new test file (9 tests) extracts shell functions and mocks wait/respawn logic. It verifies marker record/consume matching, identity validation, once-only consumption, and marker clearing. It also exercises the real respawn critical section code with a stubbed wait. However, it does not test the complete end-to-end path with real processes: gateway serves → watchdog arms → listener drops → watchdog kills → marker recorded → gateway exits 0 → respawn loop consumes marker → gateway respawns. Both non-root and root respawn loop paths need coverage, plus clean-exit (no watchdog kill) verification.
  • Impact: Regression in marker handoff between watchdog and respawn loop (marker not written, wrong identity consumed, respawn loop logic error) would not be caught until runtime E2E failure.
  • Recommended action: Add an integration test following the runWatchdog pattern in test/nemoclaw-start-gateway-health.test.ts that spawns a real gateway, triggers the watchdog, and verifies the full marker handoff and respawn for both non-root and root paths, plus clean-exit verification.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Run the new test file: it passes 9 tests. Check test/nemoclaw-start-gateway-health.test.ts for the runWatchdog pattern that simulates the full sequence.
  • Missing regression test: Full E2E integration test for watchdog kill marker consumption in both non-root and root respawn loops, plus clean-exit verification without watchdog kill.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Run the new test file: it passes 9 tests. Check test/nemoclaw-start-gateway-health.test.ts for the runWatchdog pattern that simulates the full sequence.
  • Evidence: test/gateway-watchdog-kill-marker.test.ts covers marker functions only; test/nemoclaw-start-gateway-health.test.ts covers watchdog kill but not respawn loop marker consumption

PRA-5 Resolve/justify — Internal test seam env var could be redirected if exposed to untrusted input

  • Location: scripts/nemoclaw-start.sh:257
  • Category: security
  • Problem: GATEWAY_WATCHDOG_KILL_FILE uses _NEMOCLAW_GATEWAY_WATCHDOG_KILL_FILE env var (documented as internal test seam). In production this is not user-controlled, but if an attacker could influence this env var (e.g., via compromised sandbox create envArgs), they could redirect the marker to a location they control. The file is written with mode 600 and atomic rename, limiting impact. The marker contains only pid:starttime (non-sensitive). The comment at line 257-258 says 'Internal test seam shared by the PID writer and watchdog. This is deliberately not documented as a public env API; production always keeps the default path.' but does not explicitly warn against exposing to untrusted input.
  • Impact: Low — requires attacker control of sandbox creation environment variables. The marker contains only pid:starttime (non-sensitive). Atomic write with mode 600 limits exploitation.
  • Recommended action: Strengthen the comment at line 257-258 to explicitly state: 'Production deployments must not expose _NEMOCLAW_GATEWAY_WATCHDOG_KILL_FILE to untrusted input. This is an internal test seam only.'
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Search for _NEMOCLAW_GATEWAY_WATCHDOG_KILL_FILE in the codebase — it only appears at line 258 (variable definition), line 366 (record function), line 372 (consume function), line 4139 (watchdog call), and the test file.
  • Missing regression test: None — low risk, defense-in-depth documentation.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Search for _NEMOCLAW_GATEWAY_WATCHDOG_KILL_FILE in the codebase — it only appears at line 258 (variable definition), line 366 (record function), line 372 (consume function), line 4139 (watchdog call), and the test file.
  • Evidence: Variable definition: 'GATEWAY_WATCHDOG_KILL_FILE="${_NEMOCLAW_GATEWAY_WATCHDOG_KILL_FILE:-/tmp/nemoclaw-gateway-watchdog-kill}"' with comment 'Internal test seam shared by the PID writer and watchdog. This is deliberately not documented as a public env API; production always keeps the default path.'

PRA-6 Resolve/justify — High merge conflict risk with 9 open PRs modifying the same file

💡 In-scope improvements

These are lower-risk, not throwaway. Prefer fixing them in this PR when they are local to changed code; defer only with rationale or a linked follow-up.

PRA-7 Improvement — Test extracts shell functions by string matching — fragile to refactoring

  • Location: test/gateway-watchdog-kill-marker.test.ts:1
  • Category: correctness
  • Problem: The test uses extractShellFunction() which searches for 'functionName() {' and finds the closing '}' at the same indentation level. This is fragile to refactoring (e.g., adding nested functions, changing brace style, or moving functions). A more robust approach would be to source the script directly in a test harness, but the script has side effects (PID 1 logic, traps, etc.). The current approach is pragmatic for unit testing shell logic but should be noted as a maintenance risk.
  • Impact: Future refactoring of shell function formatting in nemoclaw-start.sh could break the test extraction logic, requiring test updates.
  • Suggested action: Add a comment in the test file noting the fragility and that updates may be needed if shell function formatting changes. Alternatively, investigate sourcing a minimal library file with just the marker functions.
  • Expected follow-up: Prefer a current-PR fix when local to changed code; defer only with rationale or linked follow-up.
  • Verification: Review extractShellFunction implementation in the test file. Note it assumes a specific brace format.
  • Missing regression test: None — test maintenance concern.
  • Done when: The local improvement is applied, or the PR notes why it should be deferred.
  • Evidence: extractShellFunction searches for `${name}() {` and finds closing `}` at same indentation level

PRA-8 Improvement — Duplicated respawn critical section logic in non-root and root paths

  • Location: scripts/nemoclaw-start.sh:4819
  • Category: correctness
  • Problem: The respawn critical section (wait, capture exit identity, check marker, decide respawn vs exit) is duplicated between non-root (line ~4819) and root (line ~5104) paths. The diff shows identical changes applied to both. This duplication increases maintenance burden and risk of divergence. The root path has additional gateway control signal handling, but the core marker check logic is the same.
  • Impact: Future changes to the respawn decision logic must be applied in two places, risking inconsistency.
  • Suggested action: Extract the shared respawn decision logic into a helper function (e.g., decide_gateway_respawn_or_exit) that takes EXITED_GATEWAY_PID, EXITED_GATEWAY_START_IDENTITY, and RC, and returns the action. This would eliminate duplication and ensure consistency.
  • Expected follow-up: Prefer a current-PR fix when local to changed code; defer only with rationale or linked follow-up.
  • Verification: Compare lines 4819-4835 (non-root) and 5104-5120 (root) — the marker check condition is identical.
  • Missing regression test: None — refactoring suggestion. Existing tests cover both paths via the extracted critical section test.
  • Done when: The local improvement is applied, or the PR notes why it should be deferred.
  • Evidence: Identical marker check condition at lines 4819 and 5104: 'if [ "$RC" -eq 0 ] && ! consume_gateway_watchdog_kill "${EXITED_GATEWAY_PID}:${EXITED_GATEWAY_START_IDENTITY}"; then'
Simplification opportunities: 1 possible cut, net -20 lines possible

These are safe simplification checks only. Do not remove validation, security controls, data-loss prevention, or required tests.

  • PRA-8 shrink (scripts/nemoclaw-start.sh:4819): Duplicated respawn critical section logic in non-root (L4819-4835) and root (L5104-5120) paths
    • Replacement: Shared helper function decide_gateway_respawn_or_exit(pid, start_identity, rc) -> action
    • Net: -20 lines
    • Safety boundary: Must preserve identity-scoped marker consumption and exact RC=0 clean-exit behavior for both paths
Test follow-ups to resolve or justify

If these cover changed behavior, prefer adding them in this PR; otherwise state why existing coverage is enough or link the follow-up.

  • PRA-T1 Runtime validation — test_gateway_watchdog_kill_marker_e2e_respawn_nonroot — full sequence with real gateway and watchdog. Runtime/sandbox/infrastructure paths need behavioral runtime validation: scripts/nemoclaw-start.sh. Unit tests cover marker logic but not full E2E flow with real processes or fault injection for /tmp exhaustion.
  • PRA-T2 Runtime validation — test_gateway_watchdog_kill_marker_e2e_respawn_root — root path equivalent. Runtime/sandbox/infrastructure paths need behavioral runtime validation: scripts/nemoclaw-start.sh. Unit tests cover marker logic but not full E2E flow with real processes or fault injection for /tmp exhaustion.
  • PRA-T3 Runtime validation — test_gateway_watchdog_kill_marker_tmp_exhaustion — fault injection for /tmp full during marker write. Runtime/sandbox/infrastructure paths need behavioral runtime validation: scripts/nemoclaw-start.sh. Unit tests cover marker logic but not full E2E flow with real processes or fault injection for /tmp exhaustion.
  • PRA-T4 Runtime validation — test_gateway_clean_exit_no_watchdog — verify clean exit still works without watchdog involvement. Runtime/sandbox/infrastructure paths need behavioral runtime validation: scripts/nemoclaw-start.sh. Unit tests cover marker logic but not full E2E flow with real processes or fault injection for /tmp exhaustion.
  • PRA-T5 Integration test covers marker logic in isolation but not full watchdog→kill→marker→respawn E2E flow — Add an integration test following the runWatchdog pattern in test/nemoclaw-start-gateway-health.test.ts that spawns a real gateway, triggers the watchdog, and verifies the full marker handoff and respawn for both non-root and root paths, plus clean-exit verification.
  • PRA-T6 scripts/nemoclaw-start.sh:364 record_gateway_watchdog_kill best-effort write — None currently — theoretical edge case. A /tmp exhaustion integration test would be the regression test. record_gateway_watchdog_kill uses '_nemoclaw_safe_replace_tmp_file ... best-effort 2>/dev/null || true'; respawn loop checks RC=0 && !consume_marker
Since last review details

Current findings, using the urgency labels above:

PRA-1 Resolve/justify — Source-of-truth review needed: scripts/nemoclaw-start.sh:364 record_gateway_watchdog_kill best-effort write

  • Location: not file-specific
  • Category: architecture
  • Problem: The advisor marked localized patch analysis as needs_followup.
  • Impact: A localized workaround can preserve or hide an invalid state when the source boundary is unclear.
  • Recommended action: Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Missing regression test: None currently — theoretical edge case. A /tmp exhaustion integration test would be the regression test
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Evidence: record_gateway_watchdog_kill uses '_nemoclaw_safe_replace_tmp_file ... best-effort 2>/dev/null || true'; respawn loop checks RC=0 && !consume_marker

PRA-2 Required — Source-of-truth review needed: record_gateway_watchdog_kill best-effort write

  • Location: scripts/nemoclaw-start.sh:364
  • Category: architecture
  • Problem: The watchdog records a kill marker via `_nemoclaw_safe_replace_tmp_file ... best-effort 2>/dev/null || true`. If /tmp is full or unwritable, the marker write silently fails. The respawn loop then relies on exit code fallback: SIGTERM (143) → respawn; exit 0 on SIGTERM → container exit. This localized workaround handles an invalid state (/tmp exhaustion) at the watchdog boundary rather than fixing the source (guaranteed /tmp writability). No regression test covers the /tmp exhaustion scenario. Removal condition is unclear — this is a deliberate tradeoff, not a temporary workaround.
  • Impact: In an extreme /tmp exhaustion scenario combined with a gateway that exits 0 on SIGTERM, the container would exit instead of respawning. Probability is low but the tradeoff is not documented for maintainers.
  • Required action: Add a comment at record_gateway_watchdog_kill (line 364) documenting: the invalid state (/tmp exhaustion), source boundary (watchdog marker write), source-fix constraint (must not block startup), regression test gap (no /tmp exhaustion test), and acceptance rationale. This makes the tradeoff explicit for future maintainers.
  • Expected follow-up: Fix before merge or get explicit maintainer override.
  • Verification: Read scripts/nemoclaw-start.sh:364-368 and respawn loops at lines 4819 and 5104. Confirm the fallback behavior: RC=0 && !consume_marker → exit 0; otherwise respawn.
  • Missing regression test: Integration test simulating /tmp exhaustion during watchdog kill (marker write fails) verifying respawn loop falls back to exit code check (respawns on 143, exits on 0).
  • Done when: The required change is committed and verification passes: Read scripts/nemoclaw-start.sh:364-368 and respawn loops at lines 4819 and 5104. Confirm the fallback behavior: RC=0 && !consume_marker → exit 0; otherwise respawn.
  • Evidence: record_gateway_watchdog_kill uses '_nemoclaw_safe_replace_tmp_file ... best-effort 2>/dev/null || true'; respawn loop checks RC=0 && !consume_marker

PRA-3 Resolve/justify — Integration test covers marker logic in isolation but not full watchdog→kill→marker→respawn E2E flow

  • Location: test/gateway-watchdog-kill-marker.test.ts:1
  • Category: tests
  • Problem: The new test file (9 tests) extracts shell functions and mocks wait/respawn logic. It verifies marker record/consume matching, identity validation, once-only consumption, and marker clearing. It also exercises the real respawn critical section code with a stubbed wait. However, it does not test the complete end-to-end path with real processes: gateway serves → watchdog arms → listener drops → watchdog kills → marker recorded → gateway exits 0 → respawn loop consumes marker → gateway respawns. Both non-root and root respawn loop paths need coverage, plus clean-exit (no watchdog kill) verification.
  • Impact: Regression in marker handoff between watchdog and respawn loop (marker not written, wrong identity consumed, respawn loop logic error) would not be caught until runtime E2E failure.
  • Recommended action: Add an integration test following the runWatchdog pattern in test/nemoclaw-start-gateway-health.test.ts that spawns a real gateway, triggers the watchdog, and verifies the full marker handoff and respawn for both non-root and root paths, plus clean-exit verification.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Run the new test file: it passes 9 tests. Check test/nemoclaw-start-gateway-health.test.ts for the runWatchdog pattern that simulates the full sequence.
  • Missing regression test: Full E2E integration test for watchdog kill marker consumption in both non-root and root respawn loops, plus clean-exit verification without watchdog kill.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Run the new test file: it passes 9 tests. Check test/nemoclaw-start-gateway-health.test.ts for the runWatchdog pattern that simulates the full sequence.
  • Evidence: test/gateway-watchdog-kill-marker.test.ts covers marker functions only; test/nemoclaw-start-gateway-health.test.ts covers watchdog kill but not respawn loop marker consumption

PRA-4 Required — Best-effort marker write failure mode not explicitly documented

  • Location: scripts/nemoclaw-start.sh:364
  • Category: correctness
  • Problem: record_gateway_watchdog_kill uses best-effort write (|| true). If /tmp is full/unwritable, the watchdog kills the gateway but fails to record the marker. The respawn loop sees RC=143 (SIGTERM) and respawns anyway — safe. Only if the gateway handles SIGTERM and exits 0 would the container incorrectly exit. This theoretical edge case is not documented in comments at the function definition (line 364) or at the call site (line 4139). The similar record_gateway_pid function has a 'Best-effort: a write failure must never block startup' comment but does not explain the respawn fallback tradeoff.
  • Impact: Future maintainers may not understand why best-effort is acceptable here, potentially leading to incorrect 'fixes' that make the write required and block startup, or missing the edge case where gateway exits 0 on SIGTERM.
  • Required action: Add a comment at record_gateway_watchdog_kill (line 364) explaining: 'Best-effort: a write failure must never block startup. Marker loss falls back to non-zero exit code respawn behavior — safe for SIGTERM (143) but could exit PID 1 if gateway exits 0 on SIGTERM. This tradeoff is accepted because /tmp exhaustion is rare and the gateway does not handle SIGTERM with exit 0.'
  • Expected follow-up: Fix before merge or get explicit maintainer override.
  • Verification: Read scripts/nemoclaw-start.sh:364-368. No explanatory comment exists at the function definition.
  • Missing regression test: None — this is a documented acceptance of a theoretical edge case. The existing tests cover the happy path; a /tmp exhaustion test would be the regression test but is not required if the tradeoff is explicitly documented.
  • Done when: The required change is committed and verification passes: Read scripts/nemoclaw-start.sh:364-368. No explanatory comment exists at the function definition.
  • Evidence: record_gateway_watchdog_kill uses '_nemoclaw_safe_replace_tmp_file ... best-effort 2>/dev/null || true'; respawn loop checks RC=0 && !consume_marker

PRA-5 Resolve/justify — Internal test seam env var could be redirected if exposed to untrusted input

  • Location: scripts/nemoclaw-start.sh:257
  • Category: security
  • Problem: GATEWAY_WATCHDOG_KILL_FILE uses _NEMOCLAW_GATEWAY_WATCHDOG_KILL_FILE env var (documented as internal test seam). In production this is not user-controlled, but if an attacker could influence this env var (e.g., via compromised sandbox create envArgs), they could redirect the marker to a location they control. The file is written with mode 600 and atomic rename, limiting impact. The marker contains only pid:starttime (non-sensitive). The comment at line 257-258 says 'Internal test seam shared by the PID writer and watchdog. This is deliberately not documented as a public env API; production always keeps the default path.' but does not explicitly warn against exposing to untrusted input.
  • Impact: Low — requires attacker control of sandbox creation environment variables. The marker contains only pid:starttime (non-sensitive). Atomic write with mode 600 limits exploitation.
  • Recommended action: Strengthen the comment at line 257-258 to explicitly state: 'Production deployments must not expose _NEMOCLAW_GATEWAY_WATCHDOG_KILL_FILE to untrusted input. This is an internal test seam only.'
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Search for _NEMOCLAW_GATEWAY_WATCHDOG_KILL_FILE in the codebase — it only appears at line 258 (variable definition), line 366 (record function), line 372 (consume function), line 4139 (watchdog call), and the test file.
  • Missing regression test: None — low risk, defense-in-depth documentation.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Search for _NEMOCLAW_GATEWAY_WATCHDOG_KILL_FILE in the codebase — it only appears at line 258 (variable definition), line 366 (record function), line 372 (consume function), line 4139 (watchdog call), and the test file.
  • Evidence: Variable definition: 'GATEWAY_WATCHDOG_KILL_FILE="${_NEMOCLAW_GATEWAY_WATCHDOG_KILL_FILE:-/tmp/nemoclaw-gateway-watchdog-kill}"' with comment 'Internal test seam shared by the PID writer and watchdog. This is deliberately not documented as a public env API; production always keeps the default path.'

PRA-6 Resolve/justify — High merge conflict risk with 9 open PRs modifying the same file

PRA-7 Improvement — Test extracts shell functions by string matching — fragile to refactoring

  • Location: test/gateway-watchdog-kill-marker.test.ts:1
  • Category: correctness
  • Problem: The test uses extractShellFunction() which searches for 'functionName() {' and finds the closing '}' at the same indentation level. This is fragile to refactoring (e.g., adding nested functions, changing brace style, or moving functions). A more robust approach would be to source the script directly in a test harness, but the script has side effects (PID 1 logic, traps, etc.). The current approach is pragmatic for unit testing shell logic but should be noted as a maintenance risk.
  • Impact: Future refactoring of shell function formatting in nemoclaw-start.sh could break the test extraction logic, requiring test updates.
  • Suggested action: Add a comment in the test file noting the fragility and that updates may be needed if shell function formatting changes. Alternatively, investigate sourcing a minimal library file with just the marker functions.
  • Expected follow-up: Prefer a current-PR fix when local to changed code; defer only with rationale or linked follow-up.
  • Verification: Review extractShellFunction implementation in the test file. Note it assumes a specific brace format.
  • Missing regression test: None — test maintenance concern.
  • Done when: The local improvement is applied, or the PR notes why it should be deferred.
  • Evidence: extractShellFunction searches for `${name}() {` and finds closing `}` at same indentation level

PRA-8 Improvement — Duplicated respawn critical section logic in non-root and root paths

  • Location: scripts/nemoclaw-start.sh:4819
  • Category: correctness
  • Problem: The respawn critical section (wait, capture exit identity, check marker, decide respawn vs exit) is duplicated between non-root (line ~4819) and root (line ~5104) paths. The diff shows identical changes applied to both. This duplication increases maintenance burden and risk of divergence. The root path has additional gateway control signal handling, but the core marker check logic is the same.
  • Impact: Future changes to the respawn decision logic must be applied in two places, risking inconsistency.
  • Suggested action: Extract the shared respawn decision logic into a helper function (e.g., decide_gateway_respawn_or_exit) that takes EXITED_GATEWAY_PID, EXITED_GATEWAY_START_IDENTITY, and RC, and returns the action. This would eliminate duplication and ensure consistency.
  • Expected follow-up: Prefer a current-PR fix when local to changed code; defer only with rationale or linked follow-up.
  • Verification: Compare lines 4819-4835 (non-root) and 5104-5120 (root) — the marker check condition is identical.
  • Missing regression test: None — refactoring suggestion. Existing tests cover both paths via the extracted critical section test.
  • Done when: The local improvement is applied, or the PR notes why it should be deferred.
  • Evidence: Identical marker check condition at lines 4819 and 5104: 'if [ "$RC" -eq 0 ] && ! consume_gateway_watchdog_kill "${EXITED_GATEWAY_PID}:${EXITED_GATEWAY_START_IDENTITY}"; then'

Workflow run details

This is an automated, non-binding review; it still expects maintainers and agents to respond to each required or warning item. Treat suggestions as current-PR improvements when they touch changed code; defer only with maintainer rationale or a linked follow-up. A human maintainer must make the final merge decision.

@github-actions

github-actions Bot commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

E2E Advisor Recommendation

Required E2E: issue-2478-crash-loop-recovery, gateway-guard-recovery
Optional E2E: sandbox-survival

Dispatch hint: issue-2478-crash-loop-recovery,gateway-guard-recovery

Workflow run

Full advisor summary

E2E Recommendation Advisor

Base: origin/main
Head: HEAD
Confidence: high

Required E2E

  • issue-2478-crash-loop-recovery (medium): Closest existing live coverage for the modified entrypoint gateway respawn contract. It onboards a real OpenClaw sandbox, kills/restarts the in-container gateway repeatedly, verifies guard-chain state, and soaks for crash-loop churn, exercising the production Docker/OpenShell/NemoClaw gateway lifecycle affected by this PR.
  • gateway-guard-recovery (medium): Validates the production connect --probe-only recovery route and authenticated PID 1 supervisor after gateway process disruption. The PR changes PID 1 gateway supervision logic in nemoclaw-start.sh, so this should run to catch regressions in live recovery behavior.

Optional E2E

  • sandbox-survival (medium): Useful adjacent confidence for install/onboard plus sandbox state, SSH/exec, inference.local, and gateway restart survival. It is broader and costlier than the targeted gateway recovery jobs, so optional unless maintainers want an end-to-end user-flow proof.

New E2E recommendations

  • gateway-watchdog-respawn (high): No existing live E2E appears to directly simulate the watchdog's refused-port threshold, have the watchdog terminate a still-live OpenClaw gateway with SIGTERM, and assert PID 1 respawns instead of exiting on a clean status. The new unit test covers shell marker logic, but the production failure mode depends on live container PID 1, curl refusal, watchdog timing, and OpenShell sandbox liveness.
    • Suggested test: Add a live E2E target that onboards an OpenClaw sandbox, forces the gateway HTTP listener to stop accepting connections while the gateway PID remains live, waits for the in-container gateway watchdog to record/terminate it, then asserts the sandbox container remains alive, the gateway PID changes, /health recovers, and inference.local still works.

Dispatch hint

  • Workflow: .github/workflows/e2e.yaml
  • jobs input: issue-2478-crash-loop-recovery,gateway-guard-recovery

@github-actions

github-actions Bot commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

E2E Target Recommendation

Required E2E targets: issue-2478-crash-loop-recovery, ubuntu-repo-cloud-openclaw
Optional E2E targets: None

Dispatch required E2E targets:

  • gh workflow run e2e.yaml --ref <pr-head-ref> --field jobs=issue-2478-crash-loop-recovery
  • gh workflow run e2e.yaml --ref <pr-head-ref> --field targets=ubuntu-repo-cloud-openclaw

Workflow run

Full E2E target advisor summary

E2E Target Advisor

Base: origin/main
Head: HEAD
Confidence: high

Required E2E targets

  • issue-2478-crash-loop-recovery: scripts/nemoclaw-start.sh changes the in-container OpenClaw gateway watchdog/respawn path. This live E2E job onboards a real OpenClaw sandbox, kills and recovers the gateway repeatedly, verifies guard-chain preservation, and soaks for crash-loop churn through the real Docker/OpenShell/NemoClaw boundaries.
    • Dispatch: gh workflow run e2e.yaml --ref <pr-head-ref> --field jobs=issue-2478-crash-loop-recovery
  • ubuntu-repo-cloud-openclaw: The entrypoint change can affect normal OpenClaw sandbox startup, gateway health, and steady-state runtime behavior for the primary Ubuntu Docker cloud OpenClaw path.
    • Dispatch: gh workflow run e2e.yaml --ref <pr-head-ref> --field targets=ubuntu-repo-cloud-openclaw

Optional E2E targets

  • None.

Relevant changed files

  • scripts/nemoclaw-start.sh

laitingsheng and others added 2 commits July 1, 2026 17:28
…l marker

mark_openclaw_gateway_stopped clears GATEWAY_PID_START_IDENTITY, and both
respawn loops ran it (directly, and via openclaw_reap_exited_gateway) before
consume_gateway_watchdog_kill. The consume argument therefore degraded to
"${pid}:" with an empty identity and never matched the watchdog's
"${pid}:${start_identity}" record, so a watchdog-induced clean exit still fell
through to exit 0 and tore down the sandbox.

Snapshot the start identity into EXITED_GATEWAY_START_IDENTITY alongside
EXITED_GATEWAY_PID before the stop clears it, and consume with the snapshot in
both loops. Add regression coverage for the snapshot-before-clear contract.

Co-Authored-By: Claude <noreply@anthropic.com>
Signed-off-by: Tinson Lai <tinsonl@nvidia.com>

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with 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.

Inline comments:
In `@test/gateway-watchdog-kill-marker.test.ts`:
- Around line 100-125: The timing tests are duplicating the respawn-loop logic
instead of exercising the real ordering in the production script. Update the
`runMarkerScenario` harness in `gateway-watchdog-kill-marker.test.ts` to invoke
the actual respawn-loop critical section from `scripts/nemoclaw-start.sh` (or
extract that section into a callable helper) so the snapshot-before-clear
behavior is tested directly. Ensure the scenarios use the real
`EXITED_GATEWAY_START_IDENTITY` snapshot and `consume_gateway_watchdog_kill`
path from the loop, rather than hand-writing a copied assignment order or
consuming with `GATEWAY_PID_START_IDENTITY`.
🪄 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: Enterprise

Run ID: 8a22a4b3-aea2-4902-ad78-4205e8db61e3

📥 Commits

Reviewing files that changed from the base of the PR and between 4f37e0b and 98a2b12.

📒 Files selected for processing (2)
  • scripts/nemoclaw-start.sh
  • test/gateway-watchdog-kill-marker.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
  • scripts/nemoclaw-start.sh

Comment thread test/gateway-watchdog-kill-marker.test.ts Outdated
…arker

The prior ordering tests hand-wrote the loop's assignment order inline, so
they would keep passing even if the production snapshot-before-clear ordering
regressed. Extract the non-root respawn loop's critical section straight from
scripts/nemoclaw-start.sh and run it with wait/mark_openclaw_gateway_stopped
stubbed to model a clean exit and the identity clear, so a moved snapshot or
wrong variable makes the tests fail. Cover respawn on a matching watchdog
marker and teardown on both a no-marker operator exit and a non-matching
marker.

Co-Authored-By: Claude <noreply@anthropic.com>
Signed-off-by: Tinson Lai <tinsonl@nvidia.com>

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
test/gateway-watchdog-kill-marker.test.ts (1)

137-149: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

No equivalent coverage for the root respawn loop's critical section.

The PR objectives state both root and non-root respawn loops were updated to distinguish watchdog-induced exits from operator shutdowns, but runRespawnCriticalSection/extractRespawnCriticalSection only target the non-root loop. If the root loop has an analogous critical section, consider adding a symmetric extraction + test to guard against the same regression there.

#!/bin/bash
# Check whether the root respawn loop has an analogous consume_gateway_watchdog_kill gate.
fd -a nemoclaw-start.sh scripts | xargs -I{} rg -n -B3 -A3 'consume_gateway_watchdog_kill' {}
🤖 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/gateway-watchdog-kill-marker.test.ts` around lines 137 - 149, The
current tests only exercise the non-root respawn loop, so add symmetric coverage
for the root respawn loop’s critical section as well. Update the test helpers
around extractRespawnCriticalSection and runRespawnCriticalSection to also
locate and execute the root loop’s analogous gate, then add assertions that a
watchdog-recorded identity respawns while a clean exit or mismatched marker
tears down. Use the root loop’s unique consume_gateway_watchdog_kill/respawn
loop symbols to keep the test resilient if the script layout changes.
🤖 Prompt for all review comments with 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.

Nitpick comments:
In `@test/gateway-watchdog-kill-marker.test.ts`:
- Around line 137-149: The current tests only exercise the non-root respawn
loop, so add symmetric coverage for the root respawn loop’s critical section as
well. Update the test helpers around extractRespawnCriticalSection and
runRespawnCriticalSection to also locate and execute the root loop’s analogous
gate, then add assertions that a watchdog-recorded identity respawns while a
clean exit or mismatched marker tears down. Use the root loop’s unique
consume_gateway_watchdog_kill/respawn loop symbols to keep the test resilient if
the script layout changes.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 628a4cf5-950b-4206-b8f4-5810fa093de4

📥 Commits

Reviewing files that changed from the base of the PR and between 98a2b12 and 233299e.

📒 Files selected for processing (1)
  • test/gateway-watchdog-kill-marker.test.ts

@laitingsheng laitingsheng added area: sandbox OpenShell sandbox lifecycle, runtime, config, or recovery bug-fix PR fixes a bug or regression labels Jul 1, 2026
@apurvvkumaria apurvvkumaria self-assigned this Jul 1, 2026
@apurvvkumaria
apurvvkumaria merged commit 9e061e0 into main Jul 1, 2026
45 checks passed
@apurvvkumaria
apurvvkumaria deleted the fix/sandbox-gateway-respawn-clean-exit branch July 1, 2026 21:06
ericksoa pushed a commit that referenced this pull request Jul 2, 2026
## Summary
- Add the `v0.0.72` release-note section with links to the deeper docs
pages for installer recovery, command diagnostics, inference, policy,
and sandbox repair changes.
- Document the custom preset `allowed_ips` guard for user-authored
policy files.

## Related Issue
None.

## Source summary
- #6132 -> `docs/about/release-notes.mdx`: Summarizes installer and
upgrade recovery before generic onboarding, with links to quickstart and
lifecycle docs.
- #6087 -> `docs/network-policy/customize-network-policy.mdx`: Documents
that user-authored custom presets reject `allowed_ips` for ordinary
endpoints; also summarized in release notes.
- #5975 -> `docs/about/release-notes.mdx`: Summarizes safer curl-based
inference probes that keep API keys out of process arguments.
- #6044 -> `docs/about/release-notes.mdx`: Summarizes compact `channels
status` configuration reporting.
- #6096 -> `docs/about/release-notes.mdx`: Summarizes OpenClaw EC2
metadata discovery disablement and links to security guidance.
- #5980 and #5991 -> `docs/about/release-notes.mdx`: Summarizes `exec`
multiline argument rejection and recovery guidance.
- #6023 -> `docs/about/release-notes.mdx`: Summarizes
registered-provider diagnostics for `inference set` failures.
- #6074 -> `docs/about/release-notes.mdx`: Summarizes the refreshed
NVIDIA Endpoints featured-model selection behavior.
- #5969 -> `docs/about/release-notes.mdx`: Summarizes `credentials add`
provider credential registration.
- #6060 -> `docs/about/release-notes.mdx`: Summarizes mutable OpenClaw
config permission restoration after `exec`.
- #6134 -> `docs/about/release-notes.mdx`: Summarizes restored Tavily
access for managed Python workflows.
- #6089 -> `docs/about/release-notes.mdx`: Summarizes Hermes runtime
version-scheme comparison during upgrade checks.
- #6131 -> `docs/about/release-notes.mdx`: Summarizes OpenClaw gateway
watchdog recovery behavior.
- #5976 and #5990 -> `docs/about/release-notes.mdx`: Summarizes prompt
stdin EOF cancellation behavior during onboarding.
- #5540 -> `docs/about/release-notes.mdx`: Summarizes clarified
host-level and per-sandbox status command scope.
- #5978 and #6018 -> `docs/about/release-notes.mdx`: Summarizes
policy-denial log breadcrumbs in connect shells.

## Testing
- `npm run docs:sync-agent-variants`
- `npm run docs`
- Commit hooks passed during `git commit`, including commitlint and
gitleaks.
- Pre-push hook passed during `git push`, including TypeScript CLI and
package/tag version sync.

## Checklist
- [x] Documentation updated.
- [x] `npm run docs` completed with 0 errors and 1 existing Fern
warning.
- [x] No source code or generated build artifacts committed.

Signed-off-by: Miyoung Choi <miyoungc@nvidia.com>

<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->

## Summary by CodeRabbit

* **Documentation**
* Added release notes for v0.0.72 covering improved installer recovery,
clearer CLI diagnostics, safer inference setup and provider switching,
better credential handling, stronger policy boundaries, and more robust
runtime repair behavior.
* Updated network policy guidance to clarify when `allowed_ips` can be
used, including a specific exception for the sandbox-to-host bridge
endpoint.

<!-- end of auto-generated comment: release notes by coderabbit.ai -->
Hadar301 pushed a commit to Hadar301/NemoClaw-OpenShift that referenced this pull request Jul 12, 2026
…ng PID 1 (NVIDIA#6131)

<!-- markdownlint-disable MD041 -->
## Summary

The in-sandbox OpenClaw gateway exits spontaneously on v0.0.71 and the
sandbox is torn down, so the TUI and slash commands fail with "OpenClaw
gateway is not running inside the sandbox (sandbox likely restarted)"
and recovery also fails. This restores the respawn supervisor's intent
so a watchdog-driven gateway kill relaunches the gateway instead of
stopping PID 1.

## Related Issue

Fixes NVIDIA#6107

## Changes

- The serving watchdog SIGTERMs a gateway that has dropped its HTTP
listener so the respawn loop can relaunch it, but OpenClaw exits 0 on a
graceful SIGTERM. Both PID-1 respawn loops treated a clean rc=0 exit as
an operator-requested shutdown and called `exit 0`, tearing down the
whole sandbox.
- The watchdog now records an identity-scoped kill marker
(`<pid>:<start-identity>`) immediately before its `kill -TERM`.
- Both respawn loops (root and non-root) exit PID 1 on rc=0 only when
the exit was not a watchdog kill; a watchdog-induced clean exit
respawns, while a genuine operator clean exit still stops the sandbox.
- The marker is consumed once and scoped to the exact pid and start
identity, so a stale marker cannot force an unwanted respawn and
self-clears on a non-matching read.
- New unit coverage for the marker record/consume helpers
(`test/gateway-watchdog-kill-marker.test.ts`).

## Type of Change

- [x] Code change (feature, bug fix, or refactor)
- [ ] Code change with doc updates
- [ ] Doc only (prose changes, no code sample modifications)
- [ ] Doc only (includes code sample changes)

## Quality Gates
<!-- Check all that apply. For any "covered by existing tests", "not
applicable", or waiver entry, add a brief justification on the same line
or in the Changes section. -->
- [x] Tests added or updated for changed behavior
- [ ] Existing tests cover changed behavior — justification:
- [ ] Tests not applicable — justification:
- [ ] Docs updated for user-facing behavior changes
- [x] Docs not applicable — justification:
- [x] Sensitive paths changed (security, policy, credentials, preflight,
onboarding, inference, runner, sandbox, or messaging)
- [ ] Sensitive-path review completed or maintainer-approved waiver
recorded — reviewer/approval link/justification:
- [ ] Non-success, skipped, or missing CI check accepted by maintainer —
check name, approval link, and follow-up issue:

## Verification
<!-- Check each item you ran and confirmed. Leave unchecked items you
skipped. Doc-only changes do not require npm test unless you ran it. -->
- [x] PR description includes the DCO sign-off declaration and every
commit appears as `Verified` in GitHub
- [x] Git hooks passed during commit and push, or `npx prek run
--from-ref main --to-ref HEAD` passes
- [x] Targeted tests pass for changed behavior
- [ ] Full `npm test` passes (broad runtime changes only)
- [x] Quality Gates section completed with required justifications or
waivers
- [x] No secrets, API keys, or credentials committed
- [ ] `npm run docs` builds without warnings (doc changes only)
- [ ] Doc pages follow the [style
guide](https://github.com/NVIDIA/NemoClaw/blob/main/docs/CONTRIBUTING.md)
(doc changes only)
- [ ] New doc pages include SPDX header and frontmatter (new pages only)

---
<!-- DCO sign-off is required in this PR description, and every commit
must appear as Verified in GitHub. Run: git config user.name && git
config user.email -->
Signed-off-by: Tinson Lai <tinsonl@nvidia.com>


<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit

* **Bug Fixes**
* Improved gateway shutdown handling: intentional watchdog-triggered
listener loss no longer stops the whole container when the event is
recognized as safe.
* Added a configurable watchdog kill-marker mechanism (via an
environment setting) to ensure the gateway can restart without false
container exits.
* **Tests**
* Added automated coverage for kill-marker matching/mismatching,
missing/empty marker cases, and single-use consumption.
* Validated respawn vs. teardown behavior for timing-sensitive gateway
watchdog scenarios.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->

---------

Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
Co-authored-by: Claude <noreply@anthropic.com>
Hadar301 pushed a commit to Hadar301/NemoClaw-OpenShift that referenced this pull request Jul 12, 2026
## Summary
- Add the `v0.0.72` release-note section with links to the deeper docs
pages for installer recovery, command diagnostics, inference, policy,
and sandbox repair changes.
- Document the custom preset `allowed_ips` guard for user-authored
policy files.

## Related Issue
None.

## Source summary
- NVIDIA#6132 -> `docs/about/release-notes.mdx`: Summarizes installer and
upgrade recovery before generic onboarding, with links to quickstart and
lifecycle docs.
- NVIDIA#6087 -> `docs/network-policy/customize-network-policy.mdx`: Documents
that user-authored custom presets reject `allowed_ips` for ordinary
endpoints; also summarized in release notes.
- NVIDIA#5975 -> `docs/about/release-notes.mdx`: Summarizes safer curl-based
inference probes that keep API keys out of process arguments.
- NVIDIA#6044 -> `docs/about/release-notes.mdx`: Summarizes compact `channels
status` configuration reporting.
- NVIDIA#6096 -> `docs/about/release-notes.mdx`: Summarizes OpenClaw EC2
metadata discovery disablement and links to security guidance.
- NVIDIA#5980 and NVIDIA#5991 -> `docs/about/release-notes.mdx`: Summarizes `exec`
multiline argument rejection and recovery guidance.
- NVIDIA#6023 -> `docs/about/release-notes.mdx`: Summarizes
registered-provider diagnostics for `inference set` failures.
- NVIDIA#6074 -> `docs/about/release-notes.mdx`: Summarizes the refreshed
NVIDIA Endpoints featured-model selection behavior.
- NVIDIA#5969 -> `docs/about/release-notes.mdx`: Summarizes `credentials add`
provider credential registration.
- NVIDIA#6060 -> `docs/about/release-notes.mdx`: Summarizes mutable OpenClaw
config permission restoration after `exec`.
- NVIDIA#6134 -> `docs/about/release-notes.mdx`: Summarizes restored Tavily
access for managed Python workflows.
- NVIDIA#6089 -> `docs/about/release-notes.mdx`: Summarizes Hermes runtime
version-scheme comparison during upgrade checks.
- NVIDIA#6131 -> `docs/about/release-notes.mdx`: Summarizes OpenClaw gateway
watchdog recovery behavior.
- NVIDIA#5976 and NVIDIA#5990 -> `docs/about/release-notes.mdx`: Summarizes prompt
stdin EOF cancellation behavior during onboarding.
- NVIDIA#5540 -> `docs/about/release-notes.mdx`: Summarizes clarified
host-level and per-sandbox status command scope.
- NVIDIA#5978 and NVIDIA#6018 -> `docs/about/release-notes.mdx`: Summarizes
policy-denial log breadcrumbs in connect shells.

## Testing
- `npm run docs:sync-agent-variants`
- `npm run docs`
- Commit hooks passed during `git commit`, including commitlint and
gitleaks.
- Pre-push hook passed during `git push`, including TypeScript CLI and
package/tag version sync.

## Checklist
- [x] Documentation updated.
- [x] `npm run docs` completed with 0 errors and 1 existing Fern
warning.
- [x] No source code or generated build artifacts committed.

Signed-off-by: Miyoung Choi <miyoungc@nvidia.com>

<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->

## Summary by CodeRabbit

* **Documentation**
* Added release notes for v0.0.72 covering improved installer recovery,
clearer CLI diagnostics, safer inference setup and provider switching,
better credential handling, stronger policy boundaries, and more robust
runtime repair behavior.
* Updated network policy guidance to clarify when `allowed_ips` can be
used, including a specific exception for the sandbox-to-host bridge
endpoint.

<!-- end of auto-generated comment: release notes by coderabbit.ai -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: sandbox OpenShell sandbox lifecycle, runtime, config, or recovery bug-fix PR fixes a bug or regression

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[All Platforms][Agent&Skills] OpenClaw gateway exits spontaneously — TUI fails with 'gateway is not running'

4 participants