Skip to content

Commit 7bf427e

Browse files
jyaunchescjagwani
andauthored
fix(sandbox): harden PR 6065 advisor follow-ups (#6068)
## Summary Follow-up to merged PR #6065 to address the unaddressed PR Review Advisor warnings: - Adds safe gateway-log append helper for PID 1/root recovery warnings; refuses symlink/non-regular/replaced log targets instead of appending through them. - Adds guard-chain recovery tests proving regular log append still works and unsafe targets are refused. - Adds focused reconciliation coverage proving `NEMOCLAW_MODEL_OVERRIDE` wins over conflicting gateway state, plus no-override reconciliation still runs. ## Test plan - [x] `npx vitest run --project integration test/nemoclaw-start-reconcile.test.ts test/nemoclaw-start-guard-recovery.test.ts` Refs #6065 <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Hardened gateway log writing to refuse unsafe targets (non-regular, swapped, invalid), while sanitizing and newline-terminating log lines in UTF-8. * Recovery now records restoration warnings in the gateway log when safe, without blocking startup if logging fails. * Reconcile behavior improved: explicit model overrides are preserved; otherwise configuration is reconciled from the live gateway model. * **Tests** * Expanded gateway recovery coverage with regular/symlink/directory/missing log scenarios, validating behavior via gateway-log contents. * Added reconcile tests for overridden vs non-overridden behavior. * Updated gateway health watchdog tests to direct critical logging to controlled per-test log files. <!-- end of auto-generated comment: release notes by coderabbit.ai --> ## Advisor disposition / E2E evidence PR Review Advisor `PRA-1` / `PRA-2` runtime-validation warnings are resolved by the combination of in-PR helper hardening coverage plus live E2E evidence: - Source boundary: `append_openclaw_gateway_log_line` is the permanent PID 1 append boundary and hardcodes canonical `/tmp/gateway.log`; it intentionally refuses symlink, non-regular, replaced, or missing log targets rather than honoring inherited path seams. - Source-fix constraint: startup remains responsible for pre-creating canonical `/tmp/gateway.log`; recovery/watchdog append paths must not create a missing log because creation at recovery time would re-open the unsafe target/ownership ambiguity this PR removes. - Regression/unit coverage: `test/nemoclaw-start-guard-recovery.test.ts` covers regular append, symlink refusal, non-regular refusal, missing-log no-create, replaced-target refusal, inherited env ignored, and newline sanitization. `test/nemoclaw-start-gateway-health.test.ts` covers watchdog CRITICAL breadcrumb append through the same safe helper. - Runtime validation: current-head E2E revalidation passed on `9cda35bdaa6d426d160d262ac35c99c7449c1560`: run https://github.com/NVIDIA/NemoClaw/actions/runs/28874860688 passed required jobs `gateway-guard-recovery` and `issue-2478-crash-loop-recovery`, and run https://github.com/NVIDIA/NemoClaw/actions/runs/28874896411 passed required target `ubuntu-repo-cloud-openclaw`. The earlier same-PR artifact for `issue-2478-crash-loop-recovery` showed the real sandbox respawn path emitted `[gateway-recovery] WARNING ... (#2478/#2701)` and mirrored the same line as `[gateway-log:] [gateway-recovery] WARNING ...`, proving the real PID 1 startup/recovery lifecycle pre-created and preserved the canonical gateway log before append. - Removal condition: keep the helper-level contract tests until gateway recovery/watchdog logging is no longer routed through `/tmp/gateway.log` or the startup ownership contract is replaced by a different diagnostics channel with equivalent live E2E coverage. --------- Signed-off-by: cjagwani <cjagwani@nvidia.com> Co-authored-by: cjagwani <cjagwani@nvidia.com>
1 parent 778811d commit 7bf427e

4 files changed

Lines changed: 340 additions & 14 deletions

File tree

scripts/nemoclaw-start.sh

Lines changed: 52 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -4448,9 +4448,7 @@ start_gateway_serving_watchdog() {
44484448
fi
44494449
msg="[gateway-watchdog] CRITICAL: gateway pid $pid is alive but dropped its HTTP listener on port ${_DASHBOARD_PORT} ($refused_streak consecutive refused probes); killing it so the respawn loop can relaunch (#4710)"
44504450
echo "$msg" >&2
4451-
# _NEMOCLAW_GATEWAY_LOG is a test seam; production always appends to
4452-
# /tmp/gateway.log alongside the gateway's own output.
4453-
echo "$msg" >>"${_NEMOCLAW_GATEWAY_LOG:-/tmp/gateway.log}" 2>/dev/null || true
4451+
append_openclaw_gateway_log_line "$msg" || true
44544452
record_gateway_watchdog_kill "$tracked_identity"
44554453
kill -TERM "$pid" 2>/dev/null || true
44564454
for _ in 1 2 3 4 5 6 7 8 9 10; do
@@ -4796,11 +4794,61 @@ openclaw_runtime_guard_chain_complete() {
47964794
done
47974795
}
47984796

4797+
append_openclaw_gateway_log_line() {
4798+
local log_file="/tmp/gateway.log"
4799+
local line="$1"
4800+
python3 -I - "$log_file" "$line" <<'PYAPPEND'
4801+
import os
4802+
import stat
4803+
import sys
4804+
4805+
# Source boundary: production startup owns /tmp/gateway.log creation through
4806+
# _nemoclaw_safe_create_tmp_file before any PID 1 recovery path runs. This
4807+
# permanent defensive append policy never creates the log, never honors an
4808+
# inherited alternate-path environment variable, and refuses link/swap targets
4809+
# before writing recovery breadcrumbs.
4810+
path = sys.argv[1]
4811+
line = sys.argv[2].replace("\r", " ").replace("\n", " ")
4812+
flags = (
4813+
os.O_WRONLY
4814+
| os.O_APPEND
4815+
| getattr(os, "O_CLOEXEC", 0)
4816+
| getattr(os, "O_NOFOLLOW", 0)
4817+
| getattr(os, "O_NONBLOCK", 0)
4818+
)
4819+
try:
4820+
before = os.lstat(path)
4821+
if not stat.S_ISREG(before.st_mode):
4822+
print(f"[SECURITY] refusing unsafe gateway log path: {path}", file=sys.stderr)
4823+
raise SystemExit(1)
4824+
fd = os.open(path, flags)
4825+
except FileNotFoundError:
4826+
# Production pre-creates /tmp/gateway.log with _nemoclaw_safe_create_tmp_file.
4827+
# Do not create it here from a PID 1/root recovery path.
4828+
raise SystemExit(0)
4829+
except OSError as exc:
4830+
print(f"[SECURITY] refusing unsafe gateway log path: {path}: {exc}", file=sys.stderr)
4831+
raise SystemExit(1)
4832+
try:
4833+
current = os.fstat(fd)
4834+
if not stat.S_ISREG(current.st_mode) or (current.st_dev, current.st_ino) != (before.st_dev, before.st_ino):
4835+
print(f"[SECURITY] refusing replaced gateway log path: {path}", file=sys.stderr)
4836+
raise SystemExit(1)
4837+
os.write(fd, (line + "\n").encode("utf-8"))
4838+
finally:
4839+
os.close(fd)
4840+
PYAPPEND
4841+
}
4842+
47994843
restore_openclaw_runtime_guard_chain() {
48004844
if ! openclaw_runtime_guard_chain_complete; then
48014845
local _guard_warn="[gateway-recovery] WARNING: /tmp guard chain missing or unsafe - restoring library guards from packaged preloads (#2478/#2701)"
48024846
echo "$_guard_warn" >&2
4803-
echo "$_guard_warn" >>"${_NEMOCLAW_GATEWAY_LOG:-/tmp/gateway.log}" 2>/dev/null || true
4847+
local _append_rc=0
4848+
append_openclaw_gateway_log_line "$_guard_warn" || _append_rc=$?
4849+
if [ "$_append_rc" -ne 0 ] && [ "$_append_rc" -ne 1 ]; then
4850+
return "$_append_rc"
4851+
fi
48044852
fi
48054853

48064854
# Preserve startup ordering: immutable core preloads first, then the

test/nemoclaw-start-gateway-health.test.ts

Lines changed: 15 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -47,6 +47,13 @@ function extractShellFunction(src: string, name: string): string {
4747
return `${name}() {${body.slice(0, closing?.index ?? 0)}\n}`;
4848
}
4949

50+
function extractGatewayLogAppendFunction(src: string, gatewayLog: string): string {
51+
const functionSource = extractShellFunction(src, "append_openclaw_gateway_log_line");
52+
const marker = ' local log_file="/tmp/gateway.log"';
53+
expect(functionSource).toContain(marker);
54+
return functionSource.replace(marker, ` local log_file=${JSON.stringify(gatewayLog)}`);
55+
}
56+
5057
function safeTmpHelpers(src: string): string {
5158
const start = src.indexOf("_nemoclaw_safe_replace_tmp_file() {");
5259
const end = src.indexOf("_START_LOG=", Math.max(start, 0));
@@ -78,11 +85,12 @@ const writeProcStatFunction = [
7885
"}",
7986
].join("\n");
8087

81-
function watchdogFunctions(): string {
88+
function watchdogFunctions(gatewayLog: string): string {
8289
const src = fs.readFileSync(START_SCRIPT, "utf-8");
8390
return [
8491
safeTmpHelpers(src),
8592
pidIdentityFunctions(src),
93+
extractGatewayLogAppendFunction(src, gatewayLog),
8694
extractShellFunction(src, "record_gateway_pid"),
8795
extractShellFunction(src, "gateway_pid_is_openclaw_gateway"),
8896
extractShellFunction(src, "gateway_watchdog_positive_int_ok"),
@@ -124,6 +132,7 @@ function runWatchdog(opts: {
124132
const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), "nemoclaw-watchdog-"));
125133
const planFile = path.join(tmpDir, "curl-plan.txt");
126134
const wedgeLogFile = path.join(tmpDir, "gateway.log");
135+
fs.writeFileSync(wedgeLogFile, "", { mode: 0o644 });
127136
const pidFile = path.join(tmpDir, "gateway.pid");
128137
const procRoot = path.join(tmpDir, "proc");
129138
fs.writeFileSync(planFile, `${opts.curlPlan.join("\n")}\n`);
@@ -135,7 +144,7 @@ function runWatchdog(opts: {
135144
`GATEWAY_PID_FILE=${JSON.stringify(pidFile)}`,
136145
"_DASHBOARD_PORT=18789",
137146
`_NEMOCLAW_PROC_ROOT=${JSON.stringify(procRoot)}`,
138-
`_NEMOCLAW_GATEWAY_LOG=${JSON.stringify(wedgeLogFile)}`,
147+
`_NEMOCLAW_GATEWAY_LOG=${JSON.stringify(path.join(tmpDir, "ignored-inherited.log"))}`,
139148
writeProcStatFunction,
140149
// Throttle rather than no-op so the spinning loop stays cheap but the
141150
// test still completes in well under a second per cycle.
@@ -158,9 +167,7 @@ function runWatchdog(opts: {
158167
`mkdir -p ${JSON.stringify(procRoot)}/$FAKE_GATEWAY_PID`,
159168
`printf '%s' ${JSON.stringify(opts.cmdline ?? "openclaw-gateway")} >${JSON.stringify(procRoot)}/$FAKE_GATEWAY_PID/cmdline`,
160169
`write_proc_stat "$FAKE_GATEWAY_PID" "$$" "$FAKE_GATEWAY_START" >${JSON.stringify(procRoot)}/$FAKE_GATEWAY_PID/stat`,
161-
watchdogFunctions(),
162-
// The watchdog itself is outside the fake proc fixture. Its launch
163-
// identity is irrelevant to these gateway-target tests.
170+
watchdogFunctions(wedgeLogFile),
164171
'capture_openclaw_pid_start_identity() { printf -v "$2" "%s" "watchdog-test"; }',
165172
'record_gateway_pid "$FAKE_GATEWAY_PID" "$FAKE_GATEWAY_START"',
166173
"start_gateway_serving_watchdog",
@@ -339,6 +346,8 @@ describe("gateway serving watchdog (#4710)", () => {
339346
try {
340347
const planFile = path.join(tmpDir, "curl-plan.txt");
341348
const probeLog = path.join(tmpDir, "probes.log");
349+
const wedgeLogFile = path.join(tmpDir, "gateway.log");
350+
fs.writeFileSync(wedgeLogFile, "", { mode: 0o644 });
342351
const pidFile = path.join(tmpDir, "gateway.pid");
343352
const procRoot = path.join(tmpDir, "proc");
344353
// First probe arms on gateway A; everything after refuses.
@@ -380,7 +389,7 @@ describe("gateway serving watchdog (#4710)", () => {
380389
`printf 'openclaw-gateway' >${JSON.stringify(procRoot)}/$GATEWAY_B/cmdline`,
381390
`write_proc_stat "$GATEWAY_A" "$$" "$GATEWAY_A_START" >${JSON.stringify(procRoot)}/$GATEWAY_A/stat`,
382391
`write_proc_stat "$GATEWAY_B" "$$" "$GATEWAY_B_START" >${JSON.stringify(procRoot)}/$GATEWAY_B/stat`,
383-
watchdogFunctions(),
392+
watchdogFunctions(wedgeLogFile),
384393
'capture_openclaw_pid_start_identity() { printf -v "$2" "%s" "watchdog-test"; }',
385394
'record_gateway_pid "$GATEWAY_A" "$GATEWAY_A_START"',
386395
"start_gateway_serving_watchdog",

0 commit comments

Comments
 (0)