fix(security): strip credentials from migration snapshots and enforce blueprint digest#769
Merged
cv merged 2 commits intoNVIDIA:mainfrom Mar 27, 2026
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughSnapshot format bumped to v3. Added config-file credential sanitization (removes Changes
Sequence Diagram(s)sequenceDiagram
participant CLI
participant Migration as MigrationService
participant FS as FileSystem
participant Logger
CLI->>Migration: createSnapshotBundle(persist, blueprintPath?)
Migration->>FS: copyDirectory(hostState -> snapshot, { stripCredentials: true })
FS-->>Migration: copied files (filtered)
Migration->>FS: sanitizeConfigFile(snapshot/openclaw.json)
FS-->>Migration: sanitized file written (0600)
alt blueprintPath provided
Migration->>FS: read(blueprintPath)
FS-->>Migration: blueprint bytes
Migration->>Migration: compute sha256 -> manifest.blueprintDigest
end
Migration->>FS: write(snapshot bundle + manifest)
Migration-->>CLI: return bundle path
CLI->>Migration: restoreSnapshotToHost(snapshotDir, blueprintPath?)
Migration->>FS: read(manifest)
alt manifest.blueprintDigest present
Migration->>FS: read(blueprintPath)
FS-->>Migration: blueprint bytes
Migration->>Migration: compute sha256
alt digests match
Migration->>FS: copyDirectory(snapshot -> host, { stripCredentials: false })
Migration-->>CLI: success
else digests mismatch / invalid
Logger->>CLI: log error (digest mismatch/invalid/missing blueprint)
Migration-->>CLI: failure
end
else no blueprintDigest
Migration->>FS: copyDirectory(snapshot -> host)
Migration-->>CLI: success
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Pull request overview
This PR hardens NemoClaw migration snapshots by preventing credential material from crossing into the sandbox bundle and by adding blueprint integrity verification via a recorded SHA-256 digest, alongside bumping the snapshot format version to v3.
Changes:
- Exclude
auth-profiles.jsonduring snapshot copying and add recursive credential-field stripping +gatewayremoval for the sandbox-bundledopenclaw.json. - Record
blueprintDigestat snapshot time and validate it on restore (empty/missing digest becomes a hard failure when the field is present). - Bump
SNAPSHOT_VERSIONfrom 2 → 3 and add/extend unit tests for the above behaviors.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
nemoclaw/src/commands/migration-state.ts |
Adds credential sanitization helpers, filters credential files during snapshot copy, records/validates blueprint digest, and bumps snapshot version. |
nemoclaw/src/commands/migration-state.test.ts |
Updates version assertion and adds tests for file exclusion, credential stripping, and blueprint digest record/verify behavior. |
Comments suppressed due to low confidence (1)
nemoclaw/src/commands/migration-state.ts:694
- The snapshot directory still receives an unsanitized copy of
openclaw.json(and, whenhasExternalConfig, an unsanitized externalopenclaw.jsonviacopyFileSync). Only the sandbox-bundle config is sanitized later. If the goal is to “strip credentials from migration snapshots” (per PR title/description), consider sanitizing the config file(s) insideparentDir/openclawandparentDir/configas well (or avoid copying them entirely) so secrets never land in the snapshot artifacts on disk.
const snapshotStateDir = path.join(parentDir, "openclaw");
copyDirectory(hostState.stateDir, snapshotStateDir, { stripCredentials: true });
if (hostState.configPath && hostState.hasExternalConfig) {
const configSnapshotDir = path.join(parentDir, "config");
mkdirSync(configSnapshotDir, { recursive: true });
const configSnapshotPath = path.join(configSnapshotDir, "openclaw.json");
copyFileSync(hostState.configPath, configSnapshotPath);
chmodSync(configSnapshotPath, 0o600);
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@nemoclaw/src/commands/migration-state.ts`:
- Around line 656-660: The snapshot currently contains unsanitized credentials
because only the sandbox copy is scrubbed; update the flow that writes the raw
and external config copies (the code that writes to snapshotDir/openclaw and
snapshotDir/config/openclaw.json) to run sanitizeConfigFile(configPath) on the
snapshot copy as well (i.e., sanitize immediately after creating/writing the
snapshot copy or sanitize the source before writing both copies), ensuring
sanitizeConfigFile is invoked for both the snapshot and sandbox-bundle paths;
apply the same change for the other occurrence around the code handling lines
686-693.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 88a8e253-dfcb-438f-9503-1a159a44069d
📒 Files selected for processing (2)
nemoclaw/src/commands/migration-state.test.tsnemoclaw/src/commands/migration-state.ts
3 tasks
Contributor
|
Thanks for submitting this PR along with a detailed summary, it reconciles two previous issues into a comprehensive solution that closes credential exposure and blueprint integrity gaps, which could improve the security of the NemoClaw project. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
nemoclaw/src/commands/migration-state.test.ts (2)
713-715: Assert SHA-256 format, not just non-empty digest.A non-empty check is weak; this test should lock expected digest shape to protect the integrity guarantee.
Proposed test assertion
- expect(typeof bundle.manifest.blueprintDigest).toBe("string"); - expect((bundle.manifest.blueprintDigest ?? "").length).toBeGreaterThan(0); + expect(bundle.manifest.blueprintDigest).toMatch(/^[a-f0-9]{64}$/);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nemoclaw/src/commands/migration-state.test.ts` around lines 713 - 715, Replace the weak non-empty assertion for bundle.manifest.blueprintDigest with a strict SHA-256 format check: in the test that references bundle.manifest.blueprintDigest (in migration-state.test.ts) assert that it's a string matching a 64-hex-character pattern (e.g. /^[a-f0-9]{64}$/i) so the digest shape is validated rather than just non-empty.
1357-1386: Add restore coverage for v3 snapshots withblueprintDigest: null.
createSnapshotBundleemitsnullwhen no blueprint is provided; restore behavior for this exact v3 manifest shape is currently untested.Suggested additional test
+ it("restore succeeds for v3 snapshot when blueprintDigest is null", () => { + const logger = makeLogger(); + const origHome = process.env.HOME; + process.env.HOME = "/home/user"; + try { + const manifest: SnapshotManifest = { + version: 3, + createdAt: "2026-03-01T00:00:00.000Z", + homeDir: "/home/user", + stateDir: "/home/user/.openclaw", + configPath: null, + hasExternalConfig: false, + externalRoots: [], + warnings: [], + blueprintDigest: null, + }; + addFile("/snapshots/snap1/snapshot.json", JSON.stringify(manifest)); + addDir("/snapshots/snap1/openclaw"); + addFile("/snapshots/snap1/openclaw/openclaw.json", JSON.stringify({ restored: true })); + + const result = restoreSnapshotToHost("/snapshots/snap1", logger); + expect(result).toBe(true); + } finally { + if (origHome === undefined) { + delete process.env.HOME; + } else { + process.env.HOME = origHome; + } + } + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nemoclaw/src/commands/migration-state.test.ts` around lines 1357 - 1386, The test needs to cover v3 manifests where blueprintDigest is explicitly null: update the manifest in the "restore succeeds when manifest has no blueprintDigest (backward compat)" test to include blueprintDigest: null (matching createSnapshotBundle's emitted shape) so SnapshotManifest (v3) is exercised, and verify restoreSnapshotToHost("/snapshots/snap1", logger) still returns true; ensure SnapshotManifest, createSnapshotBundle, and restoreSnapshotToHost are the referenced symbols to locate the relevant logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@nemoclaw/src/commands/migration-state.test.ts`:
- Around line 51-57: The cpSync mock's filter callback currently only receives
the source path, but Node's fs.cpSync filter is called with (src, dest); update
the mock in cpSync (and its type annotation) to accept and call filter with both
arguments—pass the current source key and the resolved destination path (dest +
relative) when invoking opts.filter—so the mock matches production behavior and
surface destination-dependent filter bugs; adjust the vi.fn signature to (src:
string, dest: string, opts?: { filter?: (source: string, dest: string) =>
boolean }) and call opts.filter(k, dest + relative).
---
Nitpick comments:
In `@nemoclaw/src/commands/migration-state.test.ts`:
- Around line 713-715: Replace the weak non-empty assertion for
bundle.manifest.blueprintDigest with a strict SHA-256 format check: in the test
that references bundle.manifest.blueprintDigest (in migration-state.test.ts)
assert that it's a string matching a 64-hex-character pattern (e.g.
/^[a-f0-9]{64}$/i) so the digest shape is validated rather than just non-empty.
- Around line 1357-1386: The test needs to cover v3 manifests where
blueprintDigest is explicitly null: update the manifest in the "restore succeeds
when manifest has no blueprintDigest (backward compat)" test to include
blueprintDigest: null (matching createSnapshotBundle's emitted shape) so
SnapshotManifest (v3) is exercised, and verify
restoreSnapshotToHost("/snapshots/snap1", logger) still returns true; ensure
SnapshotManifest, createSnapshotBundle, and restoreSnapshotToHost are the
referenced symbols to locate the relevant logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c0c1ff81-53f4-40cf-ba39-a2a4d5bc307a
📒 Files selected for processing (1)
nemoclaw/src/commands/migration-state.test.ts
Comment on lines
+51
to
57
| cpSync: vi.fn((src: string, dest: string, opts?: { filter?: (source: string) => boolean }) => { | ||
| // Shallow copy: copy all entries whose path starts with src | ||
| for (const [k, v] of store) { | ||
| if (k === src || k.startsWith(src + "/")) { | ||
| if (opts?.filter && !opts.filter(k)) continue; | ||
| const relative = k.slice(src.length); | ||
| store.set(dest + relative, { ...v }); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
What is the current Node.js TypeScript signature of fs.cpSync's options.filter callback (does it receive source and destination)?
💡 Result:
The current Node.js TypeScript signature of fs.cpSync's options.filter callback is (source: string, destination: string) => boolean | Promise. Yes, it receives both source and destination paths. This is confirmed by multiple TypeScript-compatible Node.js implementations (Bun, Deno) showing the exact signature in CopySyncOptions.filter, a Node.js GitHub issue #49092 using filter: (source, _destination), and @types/node DefinitelyTyped PR #61042 updating fs.cp filter types to accept Promise with source/destination parameters matching JSDoc.
Citations:
- 1: "Experimental" fs.cp(), fs.cpSync(), fsPromises.cp() methods don't handle recursion when there is a filter nodejs/node#49092
- 2: https://bun.com/reference/node/fs/cpSync
- 3: https://docs.deno.com/api/node/fs/~/CopySyncOptions
- 4: https://bun.sh/reference/node/fs/cpSync
- 5: https://bun.com/reference/node/fs/CopySyncOptions
- 6: [@type/node] options.filter of fs.cp() can return a Promise DefinitelyTyped/DefinitelyTyped#61042
Align the cpSync filter mock signature with Node's API.
The mock currently passes only source to filter; the actual Node.js API passes both source and destination. This mismatch can hide bugs when production filter logic depends on the destination path.
Proposed fix
- cpSync: vi.fn((src: string, dest: string, opts?: { filter?: (source: string) => boolean }) => {
+ cpSync: vi.fn(
+ (
+ src: string,
+ dest: string,
+ opts?: { filter?: (source: string, destination: string) => boolean },
+ ) => {
// Shallow copy: copy all entries whose path starts with src
for (const [k, v] of store) {
if (k === src || k.startsWith(src + "/")) {
- if (opts?.filter && !opts.filter(k)) continue;
const relative = k.slice(src.length);
- store.set(dest + relative, { ...v });
+ const target = dest + relative;
+ if (opts?.filter && !opts.filter(k, target)) continue;
+ store.set(target, { ...v });
}
}
- }),
+ }),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@nemoclaw/src/commands/migration-state.test.ts` around lines 51 - 57, The
cpSync mock's filter callback currently only receives the source path, but
Node's fs.cpSync filter is called with (src, dest); update the mock in cpSync
(and its type annotation) to accept and call filter with both arguments—pass the
current source key and the resolved destination path (dest + relative) when
invoking opts.filter—so the mock matches production behavior and surface
destination-dependent filter bugs; adjust the vi.fn signature to (src: string,
dest: string, opts?: { filter?: (source: string, dest: string) => boolean }) and
call opts.filter(k, dest + relative).
ericksoa
approved these changes
Mar 25, 2026
Contributor
ericksoa
left a comment
ericksoa
left a comment
There was a problem hiding this comment.
LGTM — solid credential stripping and blueprint digest verification. Pushed a trivial lint fix (prefer-optional-chain) to get CI green. Thanks @gn00295120!
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
nemoclaw/src/commands/migration-state.test.ts (1)
51-57:⚠️ Potential issue | 🟡 MinorAlign the
cpSyncfilter mock with Node’s(source, destination)callback shape.At Line 51 and Line 55, the mock types/calls
filteras unary (sourceonly). This can hide destination-dependent filter behavior in code under test.Proposed fix
- cpSync: vi.fn((src: string, dest: string, opts?: { filter?: (source: string) => boolean }) => { + cpSync: vi.fn( + ( + src: string, + dest: string, + opts?: { filter?: (source: string, destination: string) => boolean }, + ) => { // Shallow copy: copy all entries whose path starts with src for (const [k, v] of store) { if (k === src || k.startsWith(src + "/")) { - if (opts?.filter && !opts.filter(k)) continue; const relative = k.slice(src.length); - store.set(dest + relative, { ...v }); + const target = dest + relative; + if (opts?.filter && !opts.filter(k, target)) continue; + store.set(target, { ...v }); } } - }), + }),What is the current Node.js/@types/node signature for fs.cpSync options.filter? Does it receive both source and destination path arguments?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nemoclaw/src/commands/migration-state.test.ts` around lines 51 - 57, The cpSync mock currently types and calls opts.filter as a unary (source) function; change its type to opts?: { filter?: (src: string, dest: string) => boolean } and when invoking the filter inside the loop call opts.filter(k, dest + relative) (or opts.filter(k, dest) if you want the raw destination base) so the mock matches Node's (source, destination) signature; update the vi.fn(...) declaration for cpSync accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@nemoclaw/src/commands/migration-state.test.ts`:
- Around line 51-57: The cpSync mock currently types and calls opts.filter as a
unary (source) function; change its type to opts?: { filter?: (src: string,
dest: string) => boolean } and when invoking the filter inside the loop call
opts.filter(k, dest + relative) (or opts.filter(k, dest) if you want the raw
destination base) so the mock matches Node's (source, destination) signature;
update the vi.fn(...) declaration for cpSync accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7b91f5f9-dd41-4586-82d7-51c2a5de068f
📒 Files selected for processing (1)
nemoclaw/src/commands/migration-state.test.ts
Contributor
Author
|
Thanks for the lint fix and LGTM @ericksoa! The Could you amend the commit message from: to: That should unblock the merge. Thanks! |
Contributor
Author
|
Hi, the merge is blocked by the |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
nemoclaw/src/commands/migration-state.ts (1)
715-738:⚠️ Potential issue | 🔴 CriticalOmit
blueprintDigestunless a blueprint was actually provided.At Line 726, the manifest always includes
blueprintDigest, and the default value from Line 715 isnull. BecauserestoreSnapshotToHost()treats the presence of that key as “verification required”, every v3 snapshot created withoutblueprintPathbecomes unrestorable. Using!== undefinedhere also avoids silently treating""as “no blueprint”.🐛 Minimal fix
- const blueprintDigest = options.blueprintPath ? computeFileDigest(options.blueprintPath) : null; - const manifest: SnapshotManifest = { version: SNAPSHOT_VERSION, createdAt: new Date().toISOString(), homeDir: hostState.homeDir, stateDir: hostState.stateDir, configPath: hostState.configPath, hasExternalConfig: hostState.hasExternalConfig, externalRoots, warnings: hostState.warnings, - blueprintDigest, }; - if (options.blueprintPath) { + if (options.blueprintPath !== undefined) { const digest = computeFileDigest(options.blueprintPath); if (!digest) { throw new Error( `Cannot compute blueprint digest for ${options.blueprintPath}. ` + "The file may be missing or unreadable.", ); } manifest.blueprintDigest = digest; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nemoclaw/src/commands/migration-state.ts` around lines 715 - 738, The manifest currently always includes a blueprintDigest key (set to null when no blueprintPath), which triggers restoreSnapshotToHost to require verification; change the logic so blueprintDigest is only computed and added to the manifest when a blueprint was explicitly provided. Specifically: remove blueprintDigest from the initial manifest literal, compute the digest only if options.blueprintPath !== undefined (not just truthy), throw if computeFileDigest returns falsy, and then assign manifest.blueprintDigest = digest; this ensures snapshots without a provided blueprint omit the key entirely and avoids treating "" as “no blueprint.” Reference: blueprintDigest variable, manifest object, options.blueprintPath, computeFileDigest, and restoreSnapshotToHost.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@nemoclaw/src/commands/migration-state.ts`:
- Around line 549-559: sanitizeConfigFile currently returns silently when JSON5
parsing yields a non-object (e.g., []), leaving an invalid openclaw.json in the
snapshot; change the behavior to fail-closed by removing or erroring when parsed
is not a plain object. In sanitizeConfigFile, after JSON5.parse and the checks
on parsed, do not simply return on invalid data — either throw an error or
unlink the configPath (using functions like unlinkSync) so the invalid file is
not left in the staging snapshot; keep the rest of the logic (delete
config["gateway"], stripCredentials, writeFileSync, chmodSync) unchanged when
parsed is a valid object. Ensure the change targets the sanitizeConfigFile
function so callers will observe the failure/cleanup instead of silently
proceeding.
- Around line 504-523: The credential-detection misses separator-style names
(snake_case/kebab-case); update isCredentialField to normalize the key before
checking by lowercasing and stripping/replacing common separators (e.g., convert
"_" and "-" to "") or collapsing non-alphanumeric chars, then test that
normalizedKey against CREDENTIAL_FIELDS and CREDENTIAL_FIELD_PATTERN; update
CREDENTIAL_FIELDS and/or CREDENTIAL_FIELD_PATTERN only if needed, but the
primary fix is normalizing the input key prior to calling
CREDENTIAL_FIELDS.has(...) and CREDENTIAL_FIELD_PATTERN.test(...), referencing
the existing symbols CREDENTIAL_FIELDS, CREDENTIAL_FIELD_PATTERN, and
isCredentialField.
---
Outside diff comments:
In `@nemoclaw/src/commands/migration-state.ts`:
- Around line 715-738: The manifest currently always includes a blueprintDigest
key (set to null when no blueprintPath), which triggers restoreSnapshotToHost to
require verification; change the logic so blueprintDigest is only computed and
added to the manifest when a blueprint was explicitly provided. Specifically:
remove blueprintDigest from the initial manifest literal, compute the digest
only if options.blueprintPath !== undefined (not just truthy), throw if
computeFileDigest returns falsy, and then assign manifest.blueprintDigest =
digest; this ensures snapshots without a provided blueprint omit the key
entirely and avoids treating "" as “no blueprint.” Reference: blueprintDigest
variable, manifest object, options.blueprintPath, computeFileDigest, and
restoreSnapshotToHost.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1acfa686-bddd-4627-9ad6-9bb27daacd66
📒 Files selected for processing (2)
nemoclaw/src/commands/migration-state.test.tsnemoclaw/src/commands/migration-state.ts
gn00295120
force-pushed
the
security/strip-credentials-and-blueprint-digest
branch
from
08e6e79 to
40007ac
Compare
… blueprint digest Reconciles NVIDIA#156 and NVIDIA#743 into a single comprehensive solution: - Filter auth-profiles.json at copy time via cpSync filter (from NVIDIA#743) - Recursive stripCredentials() with pattern-based field detection for deep config sanitization (from NVIDIA#156: CREDENTIAL_FIELDS set + CREDENTIAL_FIELD_PATTERN regex) - Remove gateway config section (contains auth tokens) from sandbox openclaw.json - Blueprint digest verification (SHA-256): recorded at snapshot time, validated on restore, empty/missing digest is a hard failure - computeFileDigest() throws when blueprint file is missing instead of silently returning null - Sanitize both snapshot-level and sandbox-bundle openclaw.json copies - Backward compatible: old snapshots without blueprintDigest skip validation - Bump SNAPSHOT_VERSION 2 → 3 Supersedes NVIDIA#156 and NVIDIA#743.
gn00295120
force-pushed
the
security/strip-credentials-and-blueprint-digest
branch
from
40007ac to
38c2406
Compare
TSavo
pushed a commit
to wopr-network/nemoclaw
that referenced
this pull request
Mar 28, 2026
* fix: improve gateway lifecycle recovery (NVIDIA#953) * fix: improve gateway lifecycle recovery * docs: fix readme markdown list spacing * fix: tighten gateway lifecycle review follow-ups * fix: simplify tokenized control ui output * fix: restore chat route in control ui urls * refactor: simplify ansi stripping in onboard * fix: shorten control ui url output * fix: move control ui below cli next steps * fix: swap hard/soft ulimit settings in start script (NVIDIA#951) Fixes NVIDIA#949 Co-authored-by: KJ <kejones@nvidia.com> * chore: add cyclomatic complexity lint rule (NVIDIA#875) * chore: add cyclomatic complexity rule (ratchet from 95) Add ESLint complexity rule to bin/ and scripts/ to prevent new functions from accumulating excessive branching. Starting threshold is 95 (current worst offender: setupNim in onboard.js). Ratchet plan: 95 → 40 → 25 → 15. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * chore: ratchet complexity to 20, suppress existing violations Suppress 6 functions that exceed the threshold with eslint-disable comments so we can start enforcing at 20 instead of 95: - setupNim (95), setupPolicies (41), setupInference (22) in onboard.js - deploy (22), main IIFE (27) in nemoclaw.js - applyPreset (24) in policies.js Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * chore: suppress complexity for 3 missed functions preflight (23), getReconciledSandboxGatewayState (25), sandboxStatus (27) --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * docs: add host-side config and state file locations to README (NVIDIA#903) Signed-off-by: peteryuqin <peter.yuqin@gmail.com> * chore: add tsconfig.cli.json, root execa, TS coverage ratchet (NVIDIA#913) * chore: add tsconfig.cli.json, root execa, TS coverage ratchet Foundation for the CLI TypeScript migration (PR 0 of the shell consolidation plan). No runtime changes — config, tooling, and dependency only. - tsconfig.cli.json: strict TS type-checking for bin/ and scripts/ (noEmit, module: preserve — tsx handles the runtime) - scripts/check-coverage-ratchet.ts: pure TS replacement for the bash+python coverage ratchet script (same logic, same tolerance) - execa ^9.6.1 added to root devDependencies (used by PR 1+) - pr.yaml: coverage ratchet step now runs the TS version via tsx - .pre-commit-config.yaml: SPDX headers cover scripts/*.ts, new tsc-check-cli pre-push hook - CONTRIBUTING.md: document typecheck:cli task and CLI pre-push hook - Delete scripts/check-coverage-ratchet.sh Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * Apply suggestion from @brandonpelfrey * chore: address PR feedback — use types_or, add tsx devDep - Use `types_or: [ts, tsx]` instead of file glob for tsc-check-cli hook per @brandonpelfrey's suggestion. - Add `tsx` to devDependencies so CI doesn't re-fetch it on every run per CodeRabbit's suggestion. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(ci): ignore GitHub "Apply suggestion" commits in commitlint * fix(ci): lint only PR title since repo is squash-merge only Reverts the commitlint ignores rule from the previous commit and instead removes the per-commit lint step entirely. Individual commit messages are discarded at merge time — only the squash-merged PR title lands in main and drives changelog generation. Drop the per-commit lint, keep the PR title check, and remove the now-unnecessary fetch-depth: 0. * Revert "fix(ci): lint only PR title since repo is squash-merge only" This reverts commit 1257a47. * Revert "fix(ci): ignore GitHub "Apply suggestion" commits in commitlint" This reverts commit c395657. * docs: fix markdownlint MD032 in README (blank line before list) * refactor: make coverage ratchet script idiomatic TypeScript - Wrap in main() with process.exitCode instead of scattered process.exit() - Replace mutable flags with .map()/.some() over typed MetricResult[] - Separate pure logic (checkMetrics) from formatting (formatReport) - Throw with { cause } chaining instead of exit-in-helpers - Derive CoverageThresholds from METRICS tuple (single source of truth) - Exhaustive switch on CheckStatus discriminated union * refactor: remove duplication in coverage ratchet script - Drop STATUS_LABELS map; inline labels in exhaustive switch - Extract common 'metric coverage is N%' preamble in formatResult - Simplify ratchetedThresholds: use results directly (already in METRICS order) instead of re-scanning with .find() per metric - Compute 'failed' once in main, pass into formatReport to avoid duplicate .some() scan * refactor: simplify coverage ratchet with FP patterns - Extract classify() as a named pure function (replaces nested ternary) - loadJSON takes repo-relative paths, eliminating THRESHOLD_PATH and SUMMARY_PATH constants (DRY the join-with-REPO_ROOT pattern) - Drop CoverageMetric/CoverageSummary interfaces (only pct is read); use structural type at the call site instead - Inline ratchetedThresholds (one-liner, used once) - formatReport derives fail/improved from results instead of taking a pre-computed boolean (let functions derive from data, don't thread derived state) - sections.join("\n\n") replaces manual empty-string pushing - Shorter type names (Thresholds, Status, Result) — no ambiguity in a single-purpose script * refactor: strip coverage ratchet to failure-only output prek hides output from commands that exit 0, so ok/improved reporting was dead code. Remove Status, Result, classify, formatResult, formatReport, and the ratcheted-thresholds suggestion block. The script now just filters for regressions and prints actionable errors on failure. --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Co-authored-by: Brandon Pelfrey <bpelfrey@nvidia.com> * fix: use CONNECT tunnel for WebSocket endpoints in Discord/Slack presets (NVIDIA#438) * fix: use CONNECT tunnel for WebSocket endpoints in Discord/Slack presets The egress proxy's HTTP idle timeout (~2 min) kills long-lived WebSocket connections when endpoints are configured with protocol:rest + tls:terminate. Switch WebSocket endpoints to access:full (CONNECT tunnel) which bypasses HTTP-level timeouts entirely. Discord: - gateway.discord.gg → access:full (WebSocket gateway) - Add PUT/PATCH/DELETE methods for discord.com (message editing, reactions) - Add media.discordapp.net for attachment access Slack: - Add wss-primary.slack.com and wss-backup.slack.com → access:full (Socket Mode WebSocket endpoints) Partially addresses NVIDIA#409 — the policy-level fix enables WebSocket connections to survive. The hardcoded 2-min timeout in openshell-sandbox still affects any protocol:rest endpoints with long-lived connections. Related: NVIDIA#361 (WhatsApp Web, same root cause) * fix: correct comment wording for media endpoint and YAML formatting * fix: standardize Node.js minimum version to 22.16 (NVIDIA#840) * fix: remove unused RECOMMENDED_NODE_MAJOR from scripts/install.sh Shellcheck flagged it as unused after the min/recommended merge. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: enforce full semver >=22.16.0 in installer scripts The runtime checks only compared the major Node.js version, allowing 22.0–22.15 to pass despite package.json requiring >=22.16.0. Use the version_gte() helper for full semver comparison in both installers. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: harden version_gte and align fallback message Guard version_gte() against prerelease suffixes (e.g. "22.16.0-rc.1") that would crash bash arithmetic. Also update the manual-install fallback message to reference MIN_NODE_VERSION instead of hardcoded "22". Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: update test stubs for Node.js 22.16 minimum and add Node 20 rejection test - Bump node stub in 'succeeds with acceptable Node.js' from v20.0.0 to v22.16.0 - Bump node stub in buildCurlPipeEnv from v22.14.0 to v22.16.0 - Add new test asserting Node.js 20 is rejected by ensure_supported_runtime --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: harden installer and onboard resiliency (NVIDIA#961) * fix: harden installer and onboard resiliency * fix: address installer and debug review follow-ups * fix: harden onboard resume across later setup steps * test: simplify payload extraction in onboard tests * test: keep onboard payload extraction target-compatible * chore: align onboard session lint with complexity rule * fix: harden onboard session safety and lock handling * fix: tighten onboard session redaction and metadata handling * fix(security): strip credentials from migration snapshots and enforce blueprint digest (NVIDIA#769) Reconciles NVIDIA#156 and NVIDIA#743 into a single comprehensive solution: - Filter auth-profiles.json at copy time via cpSync filter (from NVIDIA#743) - Recursive stripCredentials() with pattern-based field detection for deep config sanitization (from NVIDIA#156: CREDENTIAL_FIELDS set + CREDENTIAL_FIELD_PATTERN regex) - Remove gateway config section (contains auth tokens) from sandbox openclaw.json - Blueprint digest verification (SHA-256): recorded at snapshot time, validated on restore, empty/missing digest is a hard failure - computeFileDigest() throws when blueprint file is missing instead of silently returning null - Sanitize both snapshot-level and sandbox-bundle openclaw.json copies - Backward compatible: old snapshots without blueprintDigest skip validation - Bump SNAPSHOT_VERSION 2 → 3 Supersedes NVIDIA#156 and NVIDIA#743. * fix(sandbox): export proxy env vars with full NO_PROXY and persist across reconnects (NVIDIA#1025) * fix(sandbox): export proxy env vars with full NO_PROXY and persist across reconnects OpenShell injects NO_PROXY=127.0.0.1,localhost,::1 into the sandbox, missing inference.local and the gateway IP (10.200.0.1). This causes LLM inference requests to route through the egress proxy instead of going direct, and the proxy gateway IP itself gets proxied. Add proxy configuration block to nemoclaw-start.sh that: - Exports HTTP_PROXY, HTTPS_PROXY, and NO_PROXY with inference.local and the gateway IP included - Persists via /etc/profile.d/nemoclaw-proxy.sh (root) or ~/.profile (non-root fallback) so values survive OpenShell reconnect injection - Supports NEMOCLAW_PROXY_HOST / NEMOCLAW_PROXY_PORT overrides The non-root fallback ensures the fix works in environments like Brev where containers run without root privileges. Tested on DGX Spark (ARM64) and Brev VM (x86_64). Verified NO_PROXY contains inference.local and 10.200.0.1 inside the live sandbox after connect. Ref: NVIDIA#626, NVIDIA#704 Ref: NVIDIA#704 (comment) * fix(sandbox): write proxy config to ~/.bashrc for interactive reconnect sessions OpenShell's `sandbox connect` spawns `/bin/bash -i` (interactive, non-login), which sources ~/.bashrc — not ~/.profile or /etc/profile.d/*. The previous approach wrote to ~/.profile and /etc/profile.d/, neither of which is sourced by `bash -i`, so the narrow OpenShell-injected NO_PROXY persisted in live interactive sessions. Changes: - Write proxy snippet to ~/.bashrc (primary) and ~/.profile (login fallback) - Export both uppercase and lowercase proxy variants (NO_PROXY + no_proxy, HTTP_PROXY + http_proxy, etc.) — Node.js undici prefers lowercase no_proxy over uppercase NO_PROXY when both are set - Add idempotency guard to prevent duplicate blocks on container restart - Update tests: verify .bashrc writing, idempotency, bash -i override behavior, and lowercase variant correctness Tested on DGX Spark (ARM64) and Brev VM (x86_64) with full destroy + re-onboard + live `env | grep proxy` verification inside the sandbox shell via `openshell sandbox connect`. Ref: NVIDIA#626 * fix(sandbox): replace stale proxy values on restart with begin/end markers Use begin/end markers in .bashrc/.profile proxy snippet so _write_proxy_snippet replaces the block when PROXY_HOST/PORT change instead of silently keeping stale values. Adds test coverage for the replacement path. Addresses CodeRabbit review feedback on idempotency gap. * fix(sandbox): resolve sandbox user home dynamically when running as root When the entrypoint runs as root, $HOME is /root — the proxy snippet was written to /root/.bashrc instead of the sandbox user's home. Use getent passwd to look up the sandbox user's home when running as UID 0; fall back to /sandbox if the user entry is missing. Addresses CodeRabbit review feedback on _SANDBOX_HOME resolution. --------- Co-authored-by: Carlos Villela <cvillela@nvidia.com> * fix(policies): preset application for versionless policies (Fixes NVIDIA#35) (NVIDIA#101) * fix(policies): allow preset application for versionless policies (Fixes NVIDIA#35) Fixes NVIDIA#35 Signed-off-by: Deepak Jain <deepujain@gmail.com> * fix: remove stale complexity suppression in policies --------- Signed-off-by: Deepak Jain <deepujain@gmail.com> Co-authored-by: Kevin Jones <kejones@nvidia.com> * fix: restore routed inference and connect UX (NVIDIA#1037) * fix: restore routed inference and connect UX * fix: simplify detected local inference hint * fix: remove stale local inference hint * test: relax connect forward assertion --------- Signed-off-by: peteryuqin <peter.yuqin@gmail.com> Signed-off-by: Deepak Jain <deepujain@gmail.com> Co-authored-by: KJ <kejones@nvidia.com> Co-authored-by: Emily Wilkins <80470879+epwilkins@users.noreply.github.com> Co-authored-by: Carlos Villela <cvillela@nvidia.com> Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Co-authored-by: Peter <peter.yuqin@gmail.com> Co-authored-by: Brandon Pelfrey <bpelfrey@nvidia.com> Co-authored-by: Benedikt Schackenberg <6381261+BenediktSchackenberg@users.noreply.github.com> Co-authored-by: Lucas Wang <lucas_wang@lucas-futures.com> Co-authored-by: senthilr-nv <senthilr@nvidia.com> Co-authored-by: Deepak Jain <deepujain@users.noreply.github.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
ksapru
pushed a commit
to ksapru/NemoClaw
that referenced
this pull request
Mar 30, 2026
… blueprint digest (NVIDIA#769) Reconciles NVIDIA#156 and NVIDIA#743 into a single comprehensive solution: - Filter auth-profiles.json at copy time via cpSync filter (from NVIDIA#743) - Recursive stripCredentials() with pattern-based field detection for deep config sanitization (from NVIDIA#156: CREDENTIAL_FIELDS set + CREDENTIAL_FIELD_PATTERN regex) - Remove gateway config section (contains auth tokens) from sandbox openclaw.json - Blueprint digest verification (SHA-256): recorded at snapshot time, validated on restore, empty/missing digest is a hard failure - computeFileDigest() throws when blueprint file is missing instead of silently returning null - Sanitize both snapshot-level and sandbox-bundle openclaw.json copies - Backward compatible: old snapshots without blueprintDigest skip validation - Bump SNAPSHOT_VERSION 2 → 3 Supersedes NVIDIA#156 and NVIDIA#743.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Reconciles #156 and #743 into a single comprehensive solution that closes both credential exposure and blueprint integrity gaps.
Credential Sanitization (two layers)
cpSyncfilter excludesauth-profiles.jsonfrom snapshot bundles — credentials never touch the snapshot directory.stripCredentials()with pattern-based detection (CREDENTIAL_FIELDSset +CREDENTIAL_FIELD_PATTERNregex) sanitizes all credential fields inopenclaw.json— catchesapiKey,token,secret,password,resolvedKey, plus patterns likeaccessToken,privateKey,clientSecret.gatewayconfig section (contains auth tokens) is deleted entirely — sandbox entrypoint regenerates it.Blueprint Digest Verification (from #743)
""is a hard failure (fixes the silent bypass)blueprintDigestskip validationVersion Bump
SNAPSHOT_VERSION2 → 3Why a new PR?
gatewaykeyNeither #156 nor #743 is a strict superset of the other. This PR takes the best of both.
Attack chain (now broken at multiple points)
Test plan
migration-state.test.ts(41 existing + 10 new)tsc --noEmit)Supersedes #156 and #743.
Summary by CodeRabbit
New Features
Tests