fix(sandbox): stop destroying openclaw.json when .config-hash is absent on shields lock#7467
Conversation
…nt on shields lock On lock-from-mutable the guard regenerates the hash record from openclaw.json bytes regardless of the stored content, so a garbage .config-hash always locked cleanly, yet a missing one stat-failed the pair capture and drove the fail-closed handler into severing openclaw.json to a quarantine name, leaving the sandbox unrecoverable without rebuild. Synthesize a truly absent .config-hash under the frozen tree before pair capture (true-ENOENT only via lstat semantics; planted inodes at the name still fail closed), attempt a non-destructive config-only fresh publish before the last-resort sever, report quarantine names through the fail-closed detail channel, and record hashSynthesized on the ok result. The CLI sanitizes and caps guard issue text and propagates the new optional field. Lock-from-locked and unlock stay fail-stop; both-files-absent stays fail-closed. Closes #7382 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Dongni Yang <dongniy@nvidia.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe OpenClaw config guard now synthesizes missing ChangesOpenClaw config recovery
Estimated code review effort: 4 (Complex) | ~45 minutes Suggested labels: Sequence Diagram(s)sequenceDiagram
participant Operator
participant Shields
participant openclaw_config_guard as openclaw-config-guard.py
participant ConfigDirectory
Operator->>Shields: shields up or shields down
Shields->>openclaw_config_guard: lock transition
openclaw_config_guard->>ConfigDirectory: inspect openclaw.json and .config-hash
openclaw_config_guard->>ConfigDirectory: synthesize hash or republish config pair
ConfigDirectory-->>openclaw_config_guard: recovery result
openclaw_config_guard-->>Shields: result with optional hashSynthesized
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage in commit 530c4aa in the TypeScript / code-coverage/cliThe overall coverage in commit 530c4aa in the Show a code coverage summary of the most impacted files.
Updated |
|
🌿 Preview your docs: https://nvidia-preview-pr-7467.docs.buildwithfern.com/nemoclaw |
PR Review Advisor — InformationalAdvisor assessment: Informational / medium confidence Model lanes
Nemotron output stays in workflow artifacts and does not change the assessment above. E2E guidanceAdvisory only. E2E / PR Gate selects and runs jobs independently. Recommended E2E: 2 optional E2E recommendations
This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
scripts/openclaw-config-guard.py (1)
3267-3304: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate hash-record construction logic.
The sha256-digest-and-format-write sequence (
hashlib.sha256(...).hexdigest()→f"{digest} openclaw.json\n") is duplicated verbatim between_repair_absent_hash_for_lock(Lines 3294-3301) and the config-only-publish fallback in_force_fail_closed_lock(Lines 3334-3342). Since this is the canonical integrity-record format, a future change to one call site (e.g. a different hash format or encoding) could silently diverge from the other.♻️ Proposed shared helper
+def _write_hash_record(opened: OpenConfig, config_data: bytes, identity: Identity) -> None: + digest = hashlib.sha256(config_data).hexdigest() + _force_replace_bytes( + opened, + ".config-hash", + f"{digest} openclaw.json\n".encode("ascii"), + identity, + ) + + def _repair_absent_hash_for_lock(opened: OpenConfig, identity: Identity) -> None: ... config = _snapshot_file(opened, "openclaw.json") - digest = hashlib.sha256(config.data).hexdigest() - _force_replace_bytes( - opened, - ".config-hash", - f"{digest} openclaw.json\n".encode("ascii"), - identity, - ) + _write_hash_record(opened, config.data, identity) _hash_synthesized = Trueconfig = _snapshot_file(opened, "openclaw.json") - digest = hashlib.sha256(config.data).hexdigest() _force_replace_bytes(opened, "openclaw.json", config.data, identity) - _force_replace_bytes( - opened, - ".config-hash", - f"{digest} openclaw.json\n".encode("ascii"), - identity, - ) + _write_hash_record(opened, config.data, identity) _snapshot_pair(opened) published = TrueAlso applies to: 3332-3368
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/openclaw-config-guard.py` around lines 3267 - 3304, Extract the duplicated SHA-256 digest and canonical “openclaw.json” record formatting from _repair_absent_hash_for_lock and the config-only-publish fallback in _force_fail_closed_lock into one shared helper. Update both call sites to use that helper while preserving the existing ASCII-encoded record and write behavior.
🤖 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 `@scripts/openclaw-config-guard.py`:
- Around line 3267-3304: Extract the duplicated SHA-256 digest and canonical
“openclaw.json” record formatting from _repair_absent_hash_for_lock and the
config-only-publish fallback in _force_fail_closed_lock into one shared helper.
Update both call sites to use that helper while preserving the existing
ASCII-encoded record and write behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 539d93cc-86cf-4277-a772-da107e963d17
📒 Files selected for processing (7)
docs/inference/set-up-sub-agent.mdxdocs/manage-sandboxes/recover-rebuild-sandboxes.mdxdocs/reference/troubleshooting.mdxscripts/openclaw-config-guard.pysrc/lib/shields/openclaw-config-lock.test.tssrc/lib/shields/openclaw-config-lock.tstest/openclaw-config-guard-absent-hash.test.ts
… containment Extract the duplicated sha256-record construction into _write_hash_record so the canonical integrity-record format has a single definition, per review. Refs #7382 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Dongni Yang <dongniy@nvidia.com>
<!-- markdownlint-disable MD041 --> ## Summary Adds the canonical pre-tag `## v0.0.95` release entry to `docs/changelog/2026-07-24.mdx`, before the existing v0.0.94 entry. The entry summarizes approved user-visible changes merged since v0.0.94 and excludes internal-only prerequisites. ## Changes - Adds the v0.0.95 summary and detailed bullets for gateway lifecycle, recovery, state transfer, inference compatibility, sandbox security, Discord policy, and E2E evidence. - Links each user-facing theme to the most specific published documentation. - Records the release entry in the shared native changelog used by the OpenClaw, Hermes, and Deep Agents guides. Source summary: - [#7246](#7246), [#7228](#7228), [#7267](#7267), [#7489](#7489), [#7509](#7509), [#7351](#7351), and [#7290](#7290) -> `docs/changelog/2026-07-24.mdx`: Gateway authority, forward teardown and retry, managed recovery, Hermes restart recovery, scoped uninstall, and orphan-aware backup behavior. - [#7344](#7344) and [#7416](#7416) -> `docs/changelog/2026-07-24.mdx`: Atomic SQLite restore and host download verification. - [#7476](#7476), [#7347](#7347), [#7281](#7281), [#7485](#7485), [#7491](#7491), and [#7422](#7422) -> `docs/changelog/2026-07-24.mdx`: Windows Ollama reuse, CDI fallback, bounded OpenRouter connection setup, Nemotron-3 request compatibility, and managed Deep Agents retry and provider-error behavior. - [#6884](#6884), [#7481](#7481), [#6878](#6878), [#7467](#7467), [#7502](#7502), [#7503](#7503), [#7504](#7504), and [#7486](#7486) -> `docs/changelog/2026-07-24.mdx`: Trusted base-image overrides, local rebuild images, runtime validation, config preservation, reviewed package updates, and fewer final-image payload layers. - [#7303](#7303) -> `docs/changelog/2026-07-24.mdx`: Scoped Discord application-command management. - [#7488](#7488), [#7465](#7465), [#7497](#7497), [#7464](#7464), [#7501](#7501), [#7494](#7494), and [#7493](#7493) -> `docs/changelog/2026-07-24.mdx`: Selected-test risk signals, retry cleanup, full root-image validation, direct-main Hermes setup, executed PR-gate evidence, nightly history, and runner wait reporting. - [#7447](#7447) is an internal pinned-runtime prerequisite and is intentionally excluded from canonical supported-integration documentation. - [#7370](#7370) adds maintainer-only advisory reconciliation tooling and does not change supported user behavior. - [#7495](#7495) updates existing documentation and does not add a new v0.0.95 behavior claim. ## Type of Change - [ ] Code change (feature, bug fix, or refactor) - [ ] Code change with doc updates - [x] Doc only (prose changes, no code sample modifications) - [ ] Doc only (includes code sample changes) ## Quality Gates - [ ] Tests added or updated for changed behavior - [x] Existing tests cover changed behavior — justification: `test/changelog-docs.test.ts` validates the dated changelog structure, heading uniqueness, and published links. - [ ] Tests not applicable — justification: - [x] 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: ## Documentation Writer Review - [x] Documentation writer subagent reviewed the completed changes - Result: `docs-updated` - Evidence: `docs/changelog/2026-07-24.mdx`; writing rules, documentation style, factual release meaning, and published links reviewed at exact head `58b02f2bf`. - Agent: Codex documentation writer reviewer <!-- docs-review-head-sha: 58b02f2 --> <!-- docs-review-agents-blob-sha: 9c9b36d --> ## DGX Station Hardware Evidence - [ ] Tested on DGX Station - Tested commit: - Station profile/scenario: - Result: - Supporting evidence: ## Verification - [x] PR description includes a `Signed-off-by:` line and every commit appears as `Verified` in GitHub - [x] Normal `pre-commit`, `commit-msg`, and `pre-push` hooks passed, or `npm run check:diff` passed when hooks were skipped or unavailable - [x] Targeted behavior tests pass for the current change set, or tests are marked not applicable above — command/result or justification: `npx vitest run test/changelog-docs.test.ts` passed 6 tests. - [ ] Applicable broad gate passed — `npm test` for broad runtime/test-harness changes; `npm run check` for repo-wide validation/coverage changes — command/result: - [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) — the build passed with 0 errors and 2 Fern warnings. - [x] 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) --- Signed-off-by: Senthil Ravichandran <senthilr@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Documentation** * Added a new v0.0.95 changelog entry above v0.0.94. * Documented improved externally supervised gateway lifecycle ownership. * Improved snapshot restore reliability and SQLite state handling. * Tightened CLI `backup-all` behavior and host artifact verification. * Updated Windows onboarding guidance (including Ollama service reuse and CDI directory fallback). * Noted inference compatibility fixes, deeper agent failure classification, stricter base-image validation, updated Discord bot command permissions, and refined E2E release automation evidence handling. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Senthil Ravichandran <senthilr@nvidia.com>
<!-- markdownlint-disable MD041 --> ## Summary Adds regression coverage proving that an absent-hash lock replaces `openclaw.json` before sealing, so a descriptor opened before the lock cannot mutate the canonical config afterward. The production repair and broader recovery coverage already merged in #7467; this conflict resolution narrows #7396 to the remaining security invariant identified by automated review. ## Related Issue Follow-up coverage for #7382. The implementation merged in #7467. ## Changes - Opens `openclaw.json` for writing before removing `.config-hash`. - Locks the config, then writes through the retained descriptor. - Verifies the canonical config and regenerated hash remain unchanged on fresh sealed inodes. ## 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 - [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: Test-only hardening for an existing documented repair; no user-visible behavior or interface changes. - [x] Sensitive paths changed (security, policy, credentials, preflight, onboarding, inference, runner, sandbox, or messaging) - [x] Sensitive-path review completed or maintainer-approved waiver recorded — reviewer/approval link/justification: NemoClaw nine-category security review found no findings; the test-only diff strengthens descriptor/TOCTOU coverage without changing production behavior. - [ ] Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue: ## Documentation Writer Review - [x] Documentation writer subagent reviewed the completed changes - Result: `no-docs-needed` - Evidence: `origin/main...792ac3b` changes only `test/openclaw-config-guard-absent-hash.test.ts`; existing docs already describe fresh sealed config inodes and absent-hash synthesis. The focused suite passes 11/11, the combined guard suites pass 55/55, and Biome, diff check, pre-commit, commit-msg, and pre-push pass. - Agent: Codex Desktop <!-- docs-review-head-sha: 792ac3b --> <!-- docs-review-agents-blob-sha: 9c9b36d --> ## DGX Station Hardware Evidence - [ ] Tested on DGX Station - Tested commit: Not applicable - Station profile/scenario: Not applicable - Result: Not applicable - Supporting evidence: Not applicable ## Verification - [x] PR description includes a `Signed-off-by:` line and every commit appears as `Verified` in GitHub - [x] Normal `pre-commit`, `commit-msg`, and `pre-push` hooks passed, or `npm run check:diff` passed when hooks were skipped or unavailable - [x] Targeted behavior tests pass for the current change set, or tests are marked not applicable above — `vitest run --project integration test/openclaw-config-guard.test.ts test/openclaw-config-guard-absent-hash.test.ts`: 55/55 passed; the descriptor-safe follow-up passes its focused suite 11/11. - [ ] Applicable broad gate passed — not applicable to this one-test security regression addition. - [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) --- Signed-off-by: Atharv Kumaria <kumariaaatharv@gmail.com> Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com> --------- Signed-off-by: Atharv Kumaria <kumariaaatharv@gmail.com> Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com> Co-authored-by: Prekshi Vyas <prekshiv@nvidia.com>
Summary
nemoclaw <sandbox> shields up/shields downdestroy the sandbox's OpenClaw config when/sandbox/.openclaw/.config-hashis missing — the second defect in #7382 (the raw-traceback defect was fixed separately in #7421).On lock-from-mutable,
_snapshot_raw_pairrequires bothopenclaw.jsonand.config-hash, so a missing hash stat-fails three times;_force_fail_closed_lockthen fails the same pair capture (targets=None) and seversopenclaw.jsonby renaming it to.nemoclaw-rejected-openclaw.json-<32 hex>, and the subsequent rollback re-lock fails on the now-absent config — the exactCRITICAL: OpenClaw lock rollback could not restore the trusted posture … [stat-failed] … after 3 pair attemptscascade from the issue. The sandbox is then unrecoverable withoutrebuild --yes(the bytes survive under the quarantine name, but nothing reports that name).This is inconsistent by design: on lock-from-mutable the guard never trusts on-disk
.config-hashcontent —_canonical_targetsregenerates it assha256(openclaw.json)from the captured config bytes, so a hash full of garbage locks cleanly today. In mutable posture the sandbox owns both files, so absence carries strictly less signal than tolerated garbage, yet only absence triggered the destructive sever.Fix
scripts/openclaw-config-guard.py:_repair_absent_hash_for_lock— on lock-from-mutable, after_freeze(tree root:root 0700, inside the exclusive flock mutation mutex) and journal settlement, a truly absent.config-hashis synthesized from the capturedopenclaw.jsonbytes via the existing_force_replace_bytesprimitive (freshO_EXCL|O_NOFOLLOWinode, root:root 0444). Repair fires only on trueENOENT(follow_symlinks=False): a planted symlink, directory, fifo, or hardlink at the name is seen as existing and falls through to today's fail-closed rejections. Directory posture is re-verified before the repair. No trust expansion: the sealed content is byte-identical to what any mutable→lock already produces via_canonical_targets; only the co-file's existence changes. Lock-from-locked and unlock stay fail-stop, and both-files-absent stays fail-closed (characterization-tested)._force_fail_closed_lockcannot capture the pair, it now first attempts a config-only fresh publish (_snapshot_file+_force_replace_bytesfor both names +_snapshot_pairverification) and reserves the rename-sever for the truly-uncapturable case. Note the publish must use_force_replace_bytes, not_install_stored_pair— the latter begins with_snapshot_raw_pairand re-raises on the absent hash.<name>: quarantined as <rejected>to the existing; fail-closed issues:detail channel, so operators can recover the preserved bytes instead of rebuilding blind."hashSynthesized": truekey on the ok result record, so silent self-repair is visible in the guard protocol. Unknown result keys are ignored by the known-key validator in older CLIs, so this is skew-safe in both directions.src/lib/shields/openclaw-config-lock.ts:printableExcerpt()and now also applied to guard issuecode/path/detailbefore they are embedded in user-facing error strings (caps 64/256/2048) — defense in depth, since this change routes quarantine filenames and more exception text throughdetail.hashSynthesizedis validated and propagated onOpenClawConfigGuardResult.Rollout
The container-baked helper (
/usr/local/lib/nemoclaw/openclaw-config-guard.py) wins the capability probe, so the fix reaches new, rebuilt, and upgraded sandboxes; existing running sandboxes keep the old helper until rebuild. Docs now state the recovery ordering (upgrade CLI first, then rebuild — rebuilding with an old CLI restages the old guard) and the quarantine-name recovery path:docs/reference/troubleshooting.mdx— new entry for the missing-hash / quarantined-config casedocs/inference/set-up-sub-agent.mdx— the documented hand-edit workflow is exactly how users lose.config-hashdocs/manage-sandboxes/recover-rebuild-sandboxes.mdx— quarantine recovery guidanceNo changelog entry here: per
docs/CONTRIBUTING.mdthe dated changelog file is created by the pre-tag release-note PR.Scope boundary
The repair is deliberately wired into lock-from-mutable only. On lock-from-locked and unlock, the hash is the tamper-evidence seal of a root-sealed tree and its absence is a signal, so those paths stay fail-stop — pinned by characterization tests, and the docs state the asymmetry explicitly (
shields downdoes not synthesize; runshields upor regenerate the hash first).Tests
New
test/openclaw-config-guard-absent-hash.test.ts(real-python3 harness, self-contained shim; the existing guard test file is at 1491/1500 lines of its budget):hashSynthesized: true, no quarantine artifacts (fails at HEAD with the exact issue signature)hashSynthesizedhashSynthesizedmarkerstat-failed, nothing severed (characterization; passes before and after).config-hash→ repair refuses (lstat sees it as existing), lock fails closedunsafe-config-file, containment republishes a fresh regular pair, config bytes preserved.config-hash→ same fail-closed outcome (regression test for afollow_symlinksbug caught by adversarial review during development: a follow-statprobe treated a dangling symlink as truly absent and repaired it)stat-failed, nothing severed (pins the lock-only asymmetry)stat-failed, no repair, nothing severedopenclaw.jsonthrough the failed lock (fails at HEAD: config was severed)quarantined as .nemoclaw-rejected-…reported and the quarantine copy holds the original bytes (fails at HEAD: no reporting)src/lib/shields/openclaw-config-lock.test.ts: sanitizer strips non-printables and caps oversized issue text;hashSynthesizedpropagates on ok results and stays absent otherwise.All new behavior tests fail at HEAD and pass with the fix. Shields suite 253/253,
tscand Biome clean; the 16 restart-seal tests in the pre-existing guard suite fail identically at HEAD and with this change on hosts wherenode_modules/json5is not root-owned (local-env limitation, unrelated paths).Closes #7382
Signed-off-by: Dongni Yang dongniy@nvidia.com
🤖 Generated with Claude Code
Summary by CodeRabbit