Skip to content

fix(cli): replace config-set allow-list with mode-based new-path gate#2420

Merged
ericksoa merged 10 commits intomainfrom
fix/2400-config-set-unset-keys
Apr 26, 2026
Merged

fix(cli): replace config-set allow-list with mode-based new-path gate#2420
ericksoa merged 10 commits intomainfrom
fix/2400-config-set-unset-keys

Conversation

@laitingsheng
Copy link
Copy Markdown
Contributor

@laitingsheng laitingsheng commented Apr 24, 2026

Summary

The #1973 fix added a path validator that walked the currently-loaded config; #2400 reported that this rejected schema-valid first-time writes (the documented provider.compatible-endpoint.timeoutSeconds Ollama workaround) with the same error as a typo. An earlier attempt at a top-level allow-list improved discoverability but still couldn't catch deep typos, and would have had to be hand-maintained against OpenClaw's evolving schema.

This switches config set to a mode-based gate. Modifying an existing key still goes straight through. First-time writes prompt the user when stdin is a TTY, refuse with a discoverable opt-in when not, and accept an explicit override flag/env for scripts — so silent stub-writes stay impossible without coupling to a schema we don't own.

Related Issue

Fixes #2400

Changes

  • Drop isRecognizedConfigPath entirely. Replace with a small validateConfigDotpath helper that only enforces dotpath syntax (no empty segments) and the existing prototype-pollution segment block (proto, constructor, prototype, toString, hasOwnProperty).
  • In configSet, after the existing URL/SSRF and gateway.* guards, gate first-time writes: interactive (process.stdin.isTTY and !NEMOCLAW_NON_INTERACTIVE) prompts y/N; non-interactive refuses with a message naming the override; --config-accept-new-path or NEMOCLAW_CONFIG_ACCEPT_NEW_PATH=1 skips the gate.
  • Make configSet async to support the readline prompt; thread await through the CLI dispatcher and update its usage line.
  • Add a troubleshooting entry under docs/reference/troubleshooting.md and regenerate the autogenerated agent-skill mirror so users can discover the override flag and the rationale.
  • Tests: drop the allow-list cases, replace with focused validateConfigDotpath coverage (syntax + prototype-pollution segments anywhere in the path).

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)

Verification

  • npx prek run --all-files passes
  • npm test passes
  • Tests added or updated for new or changed behavior
  • No secrets, API keys, or credentials committed
  • Docs updated for user-facing behavior changes
  • make 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)

AI Disclosure

  • AI-assisted — tool: Claude Code

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

Summary by CodeRabbit

  • New Features
    • Syntactic validation for config paths and an opt‑in gate for creating new config keys (interactive prompt, --config-accept-new-path flag, or env var).
  • Bug Fixes
    • Rejects unsafe/malformed dotpaths (empty/prototype‑like/numeric segments) and blocks accidental creation of missing keys unless accepted.
  • Documentation
    • Added troubleshooting guidance describing prompt behavior and non‑interactive opt‑in.
  • Tests
    • Expanded tests for path validation, clobbering detection, error reasons, and new-key acceptance logic.

The #1973 fix stopped the silent stub-write, but the replacement
isRecognizedConfigPath walked the currently-loaded config with
hasOwnProperty at each segment — so any dotpath whose intermediate
objects did not yet exist was rejected with the same message as a
typo. That broke every documented per-agent override that targets an
unset namespace, including the `provider.compatible-endpoint.timeout
Seconds` workaround for the Ollama reasoning-stream timeout.

Replace the walk with a top-level-namespace allow-list
(provider, model, agent, agents, tools, mcpServers, runtime,
version) plus syntactic guards for empty segments and
prototype-pollution keys (__proto__, constructor, prototype,
toString, hasOwnProperty). The gateway namespace is still blocked by
its dedicated check at the call site. Surface the allow-list in the
error message so users get a discoverable hint instead of
trial-and-error.

Tests updated to reflect the new semantics and gain regression
coverage for first-time writes under an unset namespace (the #2400
scenario) and for prototype-pollution segments appearing anywhere in
the path, not just at the root.

Resolves #2400.

Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Apr 24, 2026

📝 Walkthrough

Walkthrough

configSet was made async and now validates dotpath syntax and safety (prototype-pollution, empty segments, numeric segments), prevents clobbering existing non-object ancestors, and gates creation of previously-unset keys via an opt-in flag/env or interactive confirmation; exports updated to surface the new validators and gate helpers, and CLI/tests/docs adjusted.

Changes

Cohort / File(s) Summary
Sandbox config logic
src/lib/sandbox-config.ts
Made configSet async; removed isRecognizedConfigPath; added and exported validateConfigDotpath, findClobberingAncestor, and classifyNewKeyGate; introduced confirmYesNo prompt helper; dotpath validation now rejects empty/leading/trailing segments, prototype-like segments, numeric segments, and prevents clobbering non-object ancestors; gating for first-time key creation implemented.
CLI wiring
src/nemoclaw.ts
Added --config-accept-new-path flag populating acceptNewPath; call to sandboxConfig.configSet(...) updated to await; help/usage text updated to document new flag.
Tests
test/config-set.test.ts
Replaced isRecognizedConfigPath assertions with validateConfigDotpath checks; added unit tests for findClobberingAncestor and classifyNewKeyGate across accept/prompt/refuse scenarios, numeric-segment blocking, and clobbering-ancestor detection.
Documentation
.agents/skills/nemoclaw-user-reference/references/troubleshooting.md, docs/reference/troubleshooting.md
Documented behavior when setting non-existent or numeric dotpaths, interactive prompt behavior, and non-interactive opt-in via --config-accept-new-path / NEMOCLAW_CONFIG_ACCEPT_NEW_PATH=1.

Sequence Diagram(s)

sequenceDiagram
  participant CLI as Nemoclaw CLI
  participant Module as sandbox-config
  participant Config as Config file
  participant Env as Env/CI
  participant User as TTY/User

  CLI->>Module: configSet(sandbox, { dotpath, value, acceptNewPath })
  Module->>Module: validateConfigDotpath(dotpath)
  alt invalid syntax / unsafe
    Module-->>CLI: error(reason)
  else valid syntax
    Module->>Config: read existing value at dotpath
    alt value exists
      Module-->>Config: write new value
      Module-->>CLI: success
    else value missing
      Module->>Env: check NEMOCLAW_CONFIG_ACCEPT_NEW_PATH or opts.acceptNewPath
      alt accepted via flag/env
        Module-->>Config: write new value
        Module-->>CLI: success
      else interactive
        Module->>User: prompt confirmYesNo("create new path?")
        User-->>Module: yes/no
        alt yes
          Module-->>Config: write new value
          Module-->>CLI: success
        else no
          Module-->>CLI: abort with error
        end
      end
    end
  end
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Poem

🐰 I nibble dotpaths, check each hop and claw,

I block the proto-bleed and gaps with one small paw.
Flag me, env me, or say yes in the night—
then I'll hop in, plant the key, and set it right.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 50.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 accurately describes the main change: replacing the allow-list approach (isRecognizedConfigPath) with a mode-based gate for new config paths.
Linked Issues check ✅ Passed All requirements from #2400 are addressed: schema-valid unset keys are no longer rejected, interactive/non-interactive modes are implemented, opt-in flag and env var are added, numeric segments are blocked, and troubleshooting docs are provided.
Out of Scope Changes check ✅ Passed All changes are directly scoped to #2400: core validation/gating logic, CLI flag integration, tests, and documentation. No unrelated modifications detected.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ 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/2400-config-set-unset-keys

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

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

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 (1)
test/config-set.test.ts (1)

119-124: Add an explicit test for the prototype denylist segment.

UNSAFE_KEY_SEGMENTS includes prototype, but this specific case is not asserted here.

Add the missing regression assertion
     it("rejects prototype-pollution segments anywhere in the path", () => {
       expect(isRecognizedConfigPath("toString")).toBe(false);
       expect(isRecognizedConfigPath("agents.constructor")).toBe(false);
+      expect(isRecognizedConfigPath("agents.prototype.config")).toBe(false);
       expect(isRecognizedConfigPath("provider.__proto__.polluted")).toBe(false);
       expect(isRecognizedConfigPath("tools.hasOwnProperty")).toBe(false);
     });
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@test/config-set.test.ts` around lines 119 - 124, Add a regression assertion
to the test "rejects prototype-pollution segments anywhere in the path" that
explicitly checks for the denylisted segment "prototype": update the test in
test/config-set.test.ts by adding an expect call using isRecognizedConfigPath
with a path that includes "prototype" (for example "provider.prototype.polluted"
or just "prototype") and assert it returns false; this ensures
UNSAFE_KEY_SEGMENTS' "prototype" entry is covered alongside the existing checks
for "toString", "constructor", "__proto__", and "hasOwnProperty".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/lib/sandbox-config.ts`:
- Around line 174-179: isRecognizedConfigPath currently treats any dotted path
that starts with a recognized top-level key as valid, allowing typos in nested
keys; update isRecognizedConfigPath to reject unknown deep paths by (1) keeping
the existing guard for falsy/unsafe segments (UNSAFE_KEY_SEGMENTS) and top-level
membership (RECOGNIZED_TOP_LEVEL_KEYS), and then (2) when keys.length > 1
validate the remainder of the path against an explicit whitelist/schema instead
of accepting any child — e.g. check the full dotpath against a
RECOGNIZED_CONFIG_PATHS set or walk a RECOGNIZED_SUBKEYS map keyed by parent
names to confirm each subsequent segment is an allowed child; ensure you
reference and preserve UNSAFE_KEY_SEGMENTS and RECOGNIZED_TOP_LEVEL_KEYS and
only return true when the full path is recognized by the chosen deep-path
whitelist/schema.

---

Nitpick comments:
In `@test/config-set.test.ts`:
- Around line 119-124: Add a regression assertion to the test "rejects
prototype-pollution segments anywhere in the path" that explicitly checks for
the denylisted segment "prototype": update the test in test/config-set.test.ts
by adding an expect call using isRecognizedConfigPath with a path that includes
"prototype" (for example "provider.prototype.polluted" or just "prototype") and
assert it returns false; this ensures UNSAFE_KEY_SEGMENTS' "prototype" entry is
covered alongside the existing checks for "toString", "constructor",
"__proto__", and "hasOwnProperty".
🪄 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: 9e8479f1-d88d-4bf7-aef2-7a81b02cb835

📥 Commits

Reviewing files that changed from the base of the PR and between 99b72c4 and 3de44d9.

📒 Files selected for processing (2)
  • src/lib/sandbox-config.ts
  • test/config-set.test.ts

Comment thread src/lib/sandbox-config.ts Outdated
Addresses the CodeRabbit note on #2420: the pure root allow-list was
materially stricter than the underlying schema. If OpenClaw adds a
new top-level namespace upstream before we update
RECOGNIZED_TOP_LEVEL_KEYS here, users who already have it populated
would see their next `config set` blocked even though the path is
demonstrably valid.

isRecognizedConfigPath now takes an optional config object. When the
full dotpath already resolves in that config, we trust the path and
bypass the allow-list — OpenClaw wrote it, so it is by definition
recognised. First-time writes (the #2400 scenario) still fall through
to the allow-list, and the syntactic + prototype-pollution guards
still apply in both branches.

Tests cover the three new cases: modify under an unlisted root
accepted, unset under an unlisted root still rejected even with a
config supplied, and prototype-pollution still rejected under every
branch.

Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

♻️ Duplicate comments (1)
src/lib/sandbox-config.ts (1)

175-180: ⚠️ Potential issue | 🟠 Major

Root-only fallback still accepts unknown deep keys (typo protection remains weakened).

At Line 180, any nested path under an allow-listed root is accepted when it does not already exist. Example: provider.compatible-endpoint.timeuotSeconds (typo) passes validation and gets written, which undermines typo detection for first-time writes.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/lib/sandbox-config.ts` around lines 175 - 180, isRecognizedConfigPath
currently permits any nested path under an allowed top-level key, which lets
typos like provider.compatible-endpoint.timeuotSeconds pass; modify the fallback
so that the top-level allowance only applies to single-segment dotpaths: keep
the existing validations and extractDotpath check, but change the final return
to require keys.length === 1 in addition to
RECOGNIZED_TOP_LEVEL_KEYS.has(keys[0]) so only root-level names (no dots) are
accepted by the fallback.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Duplicate comments:
In `@src/lib/sandbox-config.ts`:
- Around line 175-180: isRecognizedConfigPath currently permits any nested path
under an allowed top-level key, which lets typos like
provider.compatible-endpoint.timeuotSeconds pass; modify the fallback so that
the top-level allowance only applies to single-segment dotpaths: keep the
existing validations and extractDotpath check, but change the final return to
require keys.length === 1 in addition to RECOGNIZED_TOP_LEVEL_KEYS.has(keys[0])
so only root-level names (no dots) are accepted by the fallback.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 02dea3be-3812-4f54-aded-768a75730c3a

📥 Commits

Reviewing files that changed from the base of the PR and between 3de44d9 and 2189633.

📒 Files selected for processing (2)
  • src/lib/sandbox-config.ts
  • test/config-set.test.ts

…nset-keys

Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
The previous fix for #2400 introduced a top-level allow-list of config
namespaces. CodeRabbit and the issue reporter both noted that any
schema-aware allow-list will drift from upstream OpenClaw and cannot
catch deep typos either way.

Drop the validator entirely and gate first-time writes by mode instead:

* Interactive (TTY, !NEMOCLAW_NON_INTERACTIVE) — print the diff and
  prompt y/N when the key path does not yet exist in the agent config.
* Non-interactive — refuse with a clear message naming the override
  flag and env var; modify of an existing key still goes straight
  through.
* --config-accept-new-path / NEMOCLAW_CONFIG_ACCEPT_NEW_PATH=1 — skip
  the prompt/refusal so scripts can opt in explicitly.

Prototype-pollution and gateway-section guards stay in place via a
small validateConfigDotpath helper that only checks dotpath syntax.

Resolves #2400.

Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
@laitingsheng laitingsheng changed the title fix(cli): allow first-time config set under unset namespaces fix(cli): replace config-set allow-list with mode-based new-path gate Apr 25, 2026
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

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 `@src/lib/sandbox-config.ts`:
- Around line 162-173: validateConfigDotpath currently allows numeric segments
which extractDotpath interprets as array indices while setDotpath always
constructs plain objects; to avoid corrupting arrays, update
validateConfigDotpath to reject any dotpath that contains a purely numeric
segment (e.g., /^\d+$/) in addition to the existing checks. Keep references to
validateConfigDotpath, extractDotpath, setDotpath, and UNSAFE_KEY_SEGMENTS in
the message so the change is targeted: return { ok: false, reason: "numeric
segment not allowed" } for numeric segments until setDotpath is updated to
preserve/handle arrays.
- Around line 385-395: The interactive detection in sandbox-config.ts treats any
non-empty NEMOCLAW_NON_INTERACTIVE as true (interactive = !!process.stdin.isTTY
&& !process.env.NEMOCLAW_NON_INTERACTIVE), which deviates from the CLI
convention of only disabling prompts when NEMOCLAW_NON_INTERACTIVE === "1";
update the interactive check to mirror other commands by testing
process.env.NEMOCLAW_NON_INTERACTIVE === "1" (or its negation) so that only the
exact value "1" disables prompts, and keep the surrounding logic that uses
opts.acceptNewPath and prints the console errors for non-interactive flows
unchanged.
🪄 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: 14f65eab-39ad-45c7-8076-9531df1283ee

📥 Commits

Reviewing files that changed from the base of the PR and between 2189633 and d378d06.

📒 Files selected for processing (5)
  • .agents/skills/nemoclaw-user-reference/references/troubleshooting.md
  • docs/reference/troubleshooting.md
  • src/lib/sandbox-config.ts
  • src/nemoclaw.ts
  • test/config-set.test.ts
✅ Files skipped from review due to trivial changes (2)
  • docs/reference/troubleshooting.md
  • .agents/skills/nemoclaw-user-reference/references/troubleshooting.md

Comment thread src/lib/sandbox-config.ts Outdated
Comment thread src/lib/sandbox-config.ts Outdated
* validateConfigDotpath now refuses numeric segments. extractDotpath
  reads array indices but setDotpath always materialises plain
  objects, so allowing them silently corrupts arrays. Helm-style array
  editing was never advertised, never tested for write intent, and
  never worked via the previous walk-based validator either; refuse
  cleanly until it is designed in as a feature of its own.
* The first-time-write gate now treats only NEMOCLAW_NON_INTERACTIVE='1'
  as non-interactive, matching the dominant convention in
  src/nemoclaw.ts and src/lib/onboard.ts.
* Surface the array-index limitation in the troubleshooting page.

Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

🧹 Nitpick comments (1)
src/lib/sandbox-config.ts (1)

391-417: Consider adding branch tests for the new-path gate.

This block introduces important behavior modes (--config-accept-new-path, NEMOCLAW_CONFIG_ACCEPT_NEW_PATH=1, interactive confirm, and non-interactive refusal). I’d add explicit tests for each mode to prevent regressions in CLI UX and script flows.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/lib/sandbox-config.ts` around lines 391 - 417, Add automated branch tests
covering the new-path gate around the oldValue === undefined block: assert
behavior when opts.acceptNewPath === true (write allowed), when
NEMOCLAW_CONFIG_ACCEPT_NEW_PATH="1" (write allowed), when interactive TTY with
confirmYesNo returning true (write allowed) and false (process exits with
"Aborted."), and when non-interactive TTY (process exits and prints the
non-interactive refusal message). In tests mock/stub process.stdin.isTTY,
set/unset process.env.NEMOCLAW_CONFIG_ACCEPT_NEW_PATH, stub confirmYesNo, and
spy on process.exit and console.error to verify the correct branches in the code
paths referencing opts.key, opts.acceptNewPath, confirmYesNo, and the
process.env flag.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@src/lib/sandbox-config.ts`:
- Around line 391-417: Add automated branch tests covering the new-path gate
around the oldValue === undefined block: assert behavior when opts.acceptNewPath
=== true (write allowed), when NEMOCLAW_CONFIG_ACCEPT_NEW_PATH="1" (write
allowed), when interactive TTY with confirmYesNo returning true (write allowed)
and false (process exits with "Aborted."), and when non-interactive TTY (process
exits and prints the non-interactive refusal message). In tests mock/stub
process.stdin.isTTY, set/unset process.env.NEMOCLAW_CONFIG_ACCEPT_NEW_PATH, stub
confirmYesNo, and spy on process.exit and console.error to verify the correct
branches in the code paths referencing opts.key, opts.acceptNewPath,
confirmYesNo, and the process.env flag.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 2b924b37-8fb0-4c4c-8700-5a1fbfd083b1

📥 Commits

Reviewing files that changed from the base of the PR and between d378d06 and 6128583.

📒 Files selected for processing (4)
  • .agents/skills/nemoclaw-user-reference/references/troubleshooting.md
  • docs/reference/troubleshooting.md
  • src/lib/sandbox-config.ts
  • test/config-set.test.ts
✅ Files skipped from review due to trivial changes (2)
  • docs/reference/troubleshooting.md
  • .agents/skills/nemoclaw-user-reference/references/troubleshooting.md

Copy link
Copy Markdown
Contributor

@ericksoa ericksoa left a comment

Choose a reason for hiding this comment

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

Review

Good design choice decoupling from OpenClaw's schema — the old isRecognizedConfigPath was genuinely broken for documented workflows like provider.compatible-endpoint.timeoutSeconds, and maintaining a parallel allow-list would have been a drag. The mode-based gate is the right call.

CI is green (wsl-e2e pending). A few things I'd want addressed before merge:

Missing test coverage for the new-key gate

The tests cover validateConfigDotpath well, but the actual behavioral change — the oldValue === undefined branch in configSet — has no test coverage. These are the most important paths in the PR:

  • Non-interactive mode refusing a new key without --config-accept-new-path
  • Non-interactive mode accepting with --config-accept-new-path
  • The NEMOCLAW_CONFIG_ACCEPT_NEW_PATH=1 env var path

I'd ask for at least the non-interactive rejection and flag override cases before merging.

Mixed import style in sandbox-config.ts

import * as readline from "readline";  // ES import (new, line 15)

const fs = require("fs");              // CJS require (existing, line 16)

The entire rest of the file uses require(). The mixed style works because TypeScript rewrites it, but it's inconsistent. Either const readline = require("readline") to match the file, or a comment noting it's intentional for the TS migration path.

Dead code in confirmYesNo

if (!process.stdin.isTTY) {
  if (typeof process.stdin.pause === "function") process.stdin.pause();
  if (typeof process.stdin.unref === "function") process.stdin.unref();
}

The caller already gates on !!process.stdin.isTTY, so this branch never executes. Remove it or add a comment explaining the defensive intent.

Minor: step number renumbering

The diff renumbers inline comments from 6–12 to 7–13 because the new gate was inserted. This is pure noise in the diff and makes git blame less useful. Consider dropping the sequence numbers or using labels.

What looks good

  • validateConfigDotpath is clean — prototype-pollution, empty segments, and numeric index guards all make sense
  • The troubleshooting doc entry follows one-sentence-per-line and covers all three modes (interactive, non-interactive, env var)
  • The --config-accept-new-path flag name is discoverable and the error message tells you exactly what to do

- Extract classifyNewKeyGate as a pure helper so the gate is testable
  without mocking process.exit, stdin, or env. Add unit tests covering
  flag override, env override, env-not-exactly-"1", non-TTY refusal,
  NEMOCLAW_NON_INTERACTIVE=1 refusal, TTY prompt, and the precedence
  of overrides over non-interactive.
- Drop the unreachable !process.stdin.isTTY branch in confirmYesNo
  (the caller already gates on isTTY).
- Drop numeric prefixes from the inline step comments in configSet so
  future inserts no longer churn the surrounding lines and noise up
  git blame.
- Annotate the lone ESM import to explain why it sits beside the
  file's existing CJS requires.

Addresses review feedback on #2420.

Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
…nset-keys

Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

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 the current code and only fix it if needed.

Inline comments:
In `@src/lib/sandbox-config.ts`:
- Around line 421-451: The current "new key" gate treats oldValue === undefined
as a safe new path but misses the case where traversal failed because an
existing ancestor is a non-object scalar; setDotpath then clobbers that scalar.
Change the check around classifyNewKeyGate/oldValue to verify that every
existing ancestor along opts.key in config is an object (e.g., walk the dotpath
parents and ensure any present segment is typeof "object" and not null); only
treat it as a new path when oldValue === undefined AND all existing ancestors
are objects. If any existing ancestor is a non-object, refuse the write (use the
same error/exit flow instead of prompting) and include target.agentName and
opts.key in the message so the caller knows it would clobber a scalar; leave the
final setDotpath(config, opts.key, parsedValue) unchanged once the safety check
passes.
🪄 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: 1ea52e0a-3891-4228-9f10-b17c7892dd53

📥 Commits

Reviewing files that changed from the base of the PR and between 6128583 and ca9aae7.

📒 Files selected for processing (4)
  • .agents/skills/nemoclaw-user-reference/references/troubleshooting.md
  • docs/reference/troubleshooting.md
  • src/lib/sandbox-config.ts
  • test/config-set.test.ts
✅ Files skipped from review due to trivial changes (2)
  • .agents/skills/nemoclaw-user-reference/references/troubleshooting.md
  • docs/reference/troubleshooting.md

Comment thread src/lib/sandbox-config.ts
…cestor

setDotpath silently materialises plain objects at every intermediate
segment, so a path like `provider.api` written against a config where
`provider` is a string (or array, or null) used to overwrite the entire
scalar with an empty object. The new gate also could not tell that case
apart from a genuine new key.

Add findClobberingAncestor, which walks the dotpath parents and reports
the first existing segment that is not a config object so configSet can
refuse the write with a descriptive message instead of silently
clobbering. Numeric-segment refusal moves into the same helper now that
both checks describe runtime safety of the same write — validateConfig-
Dotpath stays purely syntactic (empty input, empty segments, prototype-
pollution guards).

Also switch the lone readline import to require so the file's module
style stays consistent.

Addresses review feedback on #2420.

Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

🧹 Nitpick comments (1)
src/lib/sandbox-config.ts (1)

750-762: Consider handling readline close event for robustness.

The current implementation doesn't resolve the promise if the user presses Ctrl+C or stdin closes unexpectedly (the close event fires without the question callback). In practice, Ctrl+C triggers SIGINT which terminates the process, so this is rarely a problem for CLI usage.

For robustness, you could handle the close event:

♻️ Optional: Handle readline close event
 function confirmYesNo(prompt: string): Promise<boolean> {
-  return new Promise((resolve) => {
+  return new Promise((resolve, _reject) => {
     const rl = readline.createInterface({ input: process.stdin, output: process.stderr });
+    let answered = false;
+    rl.on("close", () => {
+      if (!answered) resolve(false);
+    });
     rl.question(prompt, (answer: string) => {
+      answered = true;
       rl.close();
       resolve(/^y(es)?$/i.test(answer.trim()));
     });
   });
 }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/lib/sandbox-config.ts` around lines 750 - 762, The confirmYesNo function
can hang if stdin closes or the user triggers a readline 'close' without the
'question' callback firing; update confirmYesNo to attach an rl.on('close', ...)
handler that resolves the promise (e.g., resolve(false)) and ensure you guard
against double-resolution (use a local settled flag or remove listeners) and
still close/cleanup the readline; reference the existing confirmYesNo function
and the readline interface creation when adding this handler.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@src/lib/sandbox-config.ts`:
- Around line 750-762: The confirmYesNo function can hang if stdin closes or the
user triggers a readline 'close' without the 'question' callback firing; update
confirmYesNo to attach an rl.on('close', ...) handler that resolves the promise
(e.g., resolve(false)) and ensure you guard against double-resolution (use a
local settled flag or remove listeners) and still close/cleanup the readline;
reference the existing confirmYesNo function and the readline interface creation
when adding this handler.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 2869a0f2-7c56-4584-b601-ce0231d4bfac

📥 Commits

Reviewing files that changed from the base of the PR and between ca9aae7 and bc0f6ec.

📒 Files selected for processing (2)
  • src/lib/sandbox-config.ts
  • test/config-set.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
  • test/config-set.test.ts

@laitingsheng laitingsheng requested a review from ericksoa April 26, 2026 08:15
Copy link
Copy Markdown
Contributor

@ericksoa ericksoa left a comment

Choose a reason for hiding this comment

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

LGTM. Clean separation of pure helpers, strong test coverage, and security-critical guards (prototype-pollution, SSRF, gateway.*) preserved ahead of the new gate. Override path is discoverable for non-interactive use.

@ericksoa ericksoa merged commit 7b76df6 into main Apr 26, 2026
21 checks passed
@miyoungc miyoungc mentioned this pull request Apr 28, 2026
13 tasks
miyoungc added a commit that referenced this pull request Apr 28, 2026
## Summary
Refreshes user-facing docs for the last 24 hours of merged NemoClaw
history and bumps the docs metadata to 0.0.29, the next version after
v0.0.28. The updates are limited to behavior supported by merged PR
descriptions and diffs.

## Changes
- `docs/reference/commands.md`: documented `nemoclaw <name> policy-add
--from-file` and `--from-dir`, including custom preset review guidance,
from #2077 / commit `7720b175`.
- `docs/deployment/deploy-to-remote-gpu.md`: clarified that non-loopback
`CHAT_UI_URL` disables OpenClaw device pairing for remote browser-only
deployments, from #2449 / commit `f5ee8a4d`.
- `docs/inference/inference-options.md`: documented provider-aware
credential retry validation and the NVIDIA-only `nvapi-` prefix check,
from #2389 / commit `6f7f0c6d`.
- `docs/inference/switch-inference-providers.md`: documented
`NEMOCLAW_INFERENCE_INPUTS` for text/image-capable model metadata baked
into `openclaw.json`, from #2441 / commit `f4391892`.
- `docs/reference/troubleshooting.md`: added the Git certificate
verification entry for proxy CA propagation through `GIT_SSL_CAINFO`,
`GIT_SSL_CAPATH`, `CURL_CA_BUNDLE`, and `REQUESTS_CA_BUNDLE`, from #2345
/ commit `fa0dc1ab`.
- `docs/versions1.json` and `docs/project.json`: promoted docs version
`0.0.29`; `docs/versions1.json` omits unpublished `0.0.26`, `0.0.27`,
and `0.0.28` entries.
- `.agents/skills/nemoclaw-user-*`: regenerated derived user skill
references from the updated docs.
- Reviewed with no extra doc changes: #2575 / `d392ec07`, #2565 /
`a3231049`, #1965 / `db1ef3ca`, #1990 / `db665834`, #2495 / `7da86fa3`,
#2496 / `3192f4f4`, #2490 / `8c209058`, #2487 / `1f615e2f`, #2483 /
`5653d33a`, #2482 / `31c782c0`, #2464 / `23bb5703`, #2472 / `a54f9a34`,
and #2437 / `6bc860d7`.
- Skipped per docs policy: #2420 / `7b76df6b` touched the experimental
sandbox config path listed in `docs/.docs-skip`; #2466 / `cc15689c`
touched a skipped term and CI-only sandbox image files.

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

## Verification
<!-- Check each item you ran and confirmed. Leave unchecked items you
skipped. -->
- [x] `npx prek run --all-files` passes
- [ ] `npm test` passes — failed locally in installer-integration tests
and one onboard helper timeout; the doc-scoped hook test projects passed
under `prek`.
- [ ] Tests added or updated for new or changed behavior
- [x] No secrets, API keys, or credentials committed
- [x] Docs updated for user-facing behavior changes
- [ ] `make docs` builds without warnings (doc changes only) — build
succeeded, but local Sphinx emitted the existing version-switcher file
read message.
- [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)

## AI Disclosure
<!-- If an AI agent authored or co-authored this PR, check the box and
name the tool. Remove this section for fully human-authored PRs. -->
- [x] AI-assisted — tool: Codex

---
<!-- DCO sign-off required by CI. Run: git config user.name && git
config user.email -->
Signed-off-by: Miyoung Choi <miyoungc@nvidia.com>


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

* **New Features**
* Support for custom YAML presets in policy configuration via
--from-file and --from-dir.
* New build-time inference input option to declare accepted modalities
(text or text,image).

* **Improvements**
* Credential validation now offers interactive recovery: re-enter key,
retry, choose another provider, or exit.
* Clarified provider-specific API key prefix handling (nvapi- only
applies to NVIDIA keys).

* **Documentation**
  * TLS certificate troubleshooting for inspected networks.
* Clarified remote dashboard security/device-pairing behavior; command
docs updated; docs version bumped.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->

---------

Signed-off-by: Miyoung Choi <miyoungc@nvidia.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

2 participants