fix(config): validate dotpath key against known top-level sections#2071
fix(config): validate dotpath key against known top-level sections#2071Sanjays2402 wants to merge 2 commits intoNVIDIA:mainfrom
Conversation
📝 WalkthroughWalkthroughTop-level config key validation for Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant CLI as "nemoclaw CLI"
participant Resolver as "resolveAgentConfig"
participant AgentDef as "AgentDefinition (manifest)"
participant Sandbox as "configSet / sandbox-config"
participant FS as "Sandbox FS (openclaw.json)"
CLI->>Resolver: request agent config target (agent id)
Resolver->>AgentDef: read manifest.raw.config
AgentDef-->>Resolver: knownConfigSections (list or null)
Resolver->>Sandbox: pass AgentConfigTarget{..., knownSections}
CLI->>Sandbox: config set --key a.b.c --value v
Sandbox->>Sandbox: topLevel = split(key)[0]
alt knownSections is non-empty
Sandbox->>Sandbox: if topLevel ∉ knownSections -> log error + exit(1)
else knownSections null/empty
Sandbox->>FS: write key/value into openclaw.json
FS-->>CLI: success
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
Review rate limit: 8/10 reviews remaining, refill in 7 minutes and 58 seconds. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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`:
- Line 339: The step comment "// 8. Write to temp file in the agent's native
format" was inserted but the following step comment(s) still use "// 8.",
causing duplicated step numbers; update the subsequent step comment(s) in
src/lib/sandbox-config.ts (the later "// 8." occurrences) to the correct
incremented numbers (e.g., change the next "// 8." to "// 9." and continue
renumbering any further step comments) so the sequence is consistent; search for
the nearby comment text and adjust the comment tokens accordingly.
🪄 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: Pro Plus
Run ID: f1bba34c-4ebe-4ef0-b301-1fb462860a32
📒 Files selected for processing (1)
src/lib/sandbox-config.ts
|
✨ Thanks for submitting this PR that proposes a fix for the config validation to prevent invalid dotpaths, which could help improve the reliability of the configuration. Possibly related open issues: |
|
Thanks for tackling this — the underlying problem from #2019 is real and the error message UX is good. A couple of concerns with the approach though: The allowlist is OpenClaw-specific, but the code path is agent-generic. configSet uses resolveAgentConfig() to dynamically resolve the agent — NemoClaw already ships a Hermes agent (agents/hermes/manifest.yaml) whose config uses completely different top-level keys (model, provider, etc.). This allowlist would reject every valid Hermes config write. This conflicts with an existing design decision. There's already a "new key gate" at line ~481 (the classifyNewKeyGate call) that was specifically introduced to handle this problem without coupling to a hardcoded schema — see issue #2400 and the comment at line 481: "without coupling the validator to OpenClaw's evolving config schema." That issue documents a previous attempt at schema-coupled validation that was reverted because it blocked valid-but-not-yet-written keys. This PR reintroduces the same coupling. The allowlist may already be incomplete. The original issue's repro uses the key inference.endpoint, but inference isn't in the allowlist. If OpenClaw adds new top-level sections in future releases, they'd be silently blocked here until someone updates this list. A more robust approach might be:
Would you be open to reworking the validation to be agent-aware rather than hardcoded? Happy to discuss the design. |
Reject unknown top-level config keys in 'config set' to prevent writing unrecognized paths that OpenClaw silently ignores. Closes NVIDIA#2019 Signed-off-by: Sanjays2402 <51058514+Sanjays2402@users.noreply.github.com>
Per @brandonpelfrey's review on NVIDIA#2071: the original PR hardcoded an OpenClaw-specific allowlist directly in src/lib/sandbox-config.ts, which would have rejected every valid Hermes-agent config write (model, provider, etc.) and re-introduced the schema coupling that NVIDIA#2400 explicitly avoided. This commit pivots the validator to be agent-aware: - Each agent's manifest.yaml may declare 'config.known_sections' under the existing config block. agent-defs.ts now exposes that as AgentDefinition.knownConfigSections (string[] | null). - AgentConfigTarget gains a 'knownSections' field, populated from the resolved agent's manifest. - configSet() validates the top-level dotpath ONLY when the resolved agent declares known_sections. Agents that omit the field skip validation entirely, so NemoClaw stays decoupled from any one agent's evolving config schema. - agents/openclaw/manifest.yaml declares its known sections (llm, tts, stt, mcp, sandbox, extensions, personas, customization, inference, plugins, skills, ui, logging) so the original NVIDIA#2019 typo ('inferenc.endpoint') is still caught for OpenClaw users. - agents/hermes/manifest.yaml intentionally does NOT declare a list, so Hermes config writes (model, provider, ...) are unaffected. - New tests in test/config-set.test.ts verify the source no longer contains a hardcoded KNOWN_TOP_LEVEL_KEYS allowlist, that the validator reads from target.knownSections, and that the openclaw manifest declares the allowlist while the hermes manifest does not. Step comments inside configSet() were also renumbered (the original PR left a duplicate '// 8.', flagged by CodeRabbit). Signed-off-by: Sanjays2402 <51058514+Sanjays2402@users.noreply.github.com>
Sanjays2402
force-pushed
the
fix/config-set-key-validation
branch
from
d15f1a9 to
9077e7c
Compare
|
Thanks @brandonpelfrey — you're absolutely right, the original allowlist was wrong on every count you raised. Reworked the design to be agent-aware rather than hardcoded: What changed
TestsNew tests in
All commits DCO-signed. Force-pushed to |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
test/config-set.test.ts (1)
38-85: Prefer a behavioral test over source-string assertions.These checks only grep the implementation and manifests, so they can pass even if the runtime validation regresses. A small end-to-end test that calls
configSetwith an invalid top-level key and asserts the exit/message would be much more durable.🤖 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 38 - 85, Replace the brittle source-string grep tests with a behavioral end-to-end test that invokes the CLI/configSet flow: call the command (or exported function) that implements configSet with a manifest-aware target (one that has config.known_sections set, e.g. the OpenClaw manifest) and pass a config containing an invalid top-level key; assert the process exits non-zero (or the function throws) and that the error message mentions unknown/invalid top-level section(s). Keep one lightweight assertion that sandbox-config.ts reads target.knownSections (or remove it if you fully replace with the behavioral test), but ensure the new test exercises the runtime path that checks target.knownSections in sandbox-config.ts so regressions are caught at runtime instead of by string matching.
🤖 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/agent-defs.ts`:
- Around line 161-166: The getter knownConfigSections currently returns an empty
array when cfg.known_sections contains no string entries, violating the
documented contract and causing configSet to skip validation; update the
knownConfigSections getter (in src/lib/agent-defs.ts) to normalize the filtered
result so that if the filtered array of strings is empty it returns null instead
of [], i.e. after filtering (s): s is string ensure you check filtered.length
and return null when zero, otherwise return the filtered string array.
In `@test/config-set.test.ts`:
- Around line 5-9: Remove the stale eslint suppression comment before the
require call in test/config-set.test.ts: delete the line "//
eslint-disable-next-line `@typescript-eslint/no-require-imports`" so the file uses
the intentional CommonJS require for the yaml constant (const yaml =
require("js-yaml") as { load: (s: string) => unknown };) without dead
suppression comments.
---
Nitpick comments:
In `@test/config-set.test.ts`:
- Around line 38-85: Replace the brittle source-string grep tests with a
behavioral end-to-end test that invokes the CLI/configSet flow: call the command
(or exported function) that implements configSet with a manifest-aware target
(one that has config.known_sections set, e.g. the OpenClaw manifest) and pass a
config containing an invalid top-level key; assert the process exits non-zero
(or the function throws) and that the error message mentions unknown/invalid
top-level section(s). Keep one lightweight assertion that sandbox-config.ts
reads target.knownSections (or remove it if you fully replace with the
behavioral test), but ensure the new test exercises the runtime path that checks
target.knownSections in sandbox-config.ts so regressions are caught at runtime
instead of by string matching.
🪄 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: e7f2040f-363e-452a-a16c-135c272dbb3d
📒 Files selected for processing (4)
agents/openclaw/manifest.yamlsrc/lib/agent-defs.tssrc/lib/sandbox-config.tstest/config-set.test.ts
| get knownConfigSections(): string[] | null { | ||
| const cfg = (raw.config as Record<string, unknown>) || {}; | ||
| const sections = cfg.known_sections; | ||
| if (!Array.isArray(sections) || sections.length === 0) return null; | ||
| return sections.filter((s): s is string => typeof s === "string"); | ||
| }, |
There was a problem hiding this comment.
Return null when the filtered allowlist is empty.
If config.known_sections contains only non-string entries, this getter currently returns [], which makes configSet silently skip validation for a malformed manifest. Normalizing an empty filtered result back to null keeps the documented contract intact.
🛠️ Suggested fix
get knownConfigSections(): string[] | null {
const cfg = (raw.config as Record<string, unknown>) || {};
const sections = cfg.known_sections;
if (!Array.isArray(sections) || sections.length === 0) return null;
- return sections.filter((s): s is string => typeof s === "string");
+ const knownSections = sections.filter((s): s is string => typeof s === "string");
+ return knownSections.length > 0 ? knownSections : null;
},🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/agent-defs.ts` around lines 161 - 166, The getter knownConfigSections
currently returns an empty array when cfg.known_sections contains no string
entries, violating the documented contract and causing configSet to skip
validation; update the knownConfigSections getter (in src/lib/agent-defs.ts) to
normalize the filtered result so that if the filtered array of strings is empty
it returns null instead of [], i.e. after filtering (s): s is string ensure you
check filtered.length and return null when zero, otherwise return the filtered
string array.
| import assert from "node:assert"; | ||
| import fs from "node:fs"; | ||
| import path from "node:path"; | ||
| // eslint-disable-next-line @typescript-eslint/no-require-imports | ||
| const yaml = require("js-yaml") as { load: (s: string) => unknown }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check if the test file exists and read the relevant lines
cat -n test/config-set.test.ts | head -20Repository: NVIDIA/NemoClaw
Length of output: 1115
🏁 Script executed:
# Check the entire js-yaml import section and surrounding context
cat -n test/config-set.test.tsRepository: NVIDIA/NemoClaw
Length of output: 8313
🏁 Script executed:
# Look for eslint configuration files
find . -maxdepth 2 -type f \( -name ".eslintrc*" -o -name "eslint.config.*" \) | head -20Repository: NVIDIA/NemoClaw
Length of output: 108
🏁 Script executed:
# Check if js-yaml is in package.json and what version
cat package.json | grep -A 5 -B 5 "js-yaml" || echo "Not found in grep, checking full file"Repository: NVIDIA/NemoClaw
Length of output: 814
🏁 Script executed:
# Check js-yaml module to understand if it supports ESM
ls -la node_modules/js-yaml 2>/dev/null | head -20 || echo "node_modules not available, checking package.json exports"Repository: NVIDIA/NemoClaw
Length of output: 41
🏁 Script executed:
# Look for how js-yaml is used elsewhere in the codebase
rg "js-yaml" --type ts --type js -A 2 -B 2Repository: NVIDIA/NemoClaw
Length of output: 674
🏁 Script executed:
# Check the main eslint configuration
cat eslint.config.mjs | head -100Repository: NVIDIA/NemoClaw
Length of output: 2966
🏁 Script executed:
# Check if `@typescript-eslint/no-require-imports` rule is mentioned
grep -r "no-require-imports" eslint.config.mjs || echo "Rule not found in eslint.config.mjs"Repository: NVIDIA/NemoClaw
Length of output: 95
🏁 Script executed:
# Look up js-yaml package to see if it has ESM exports
cat package.json | jq '.dependencies' || cat package.json | grep -A 10 '"dependencies"'Repository: NVIDIA/NemoClaw
Length of output: 128
🏁 Script executed:
# Check if js-yaml has ESM support by looking at common patterns
web_search
js-yaml npm package ESM import support version 4.1Repository: NVIDIA/NemoClaw
Length of output: 156
🌐 Web query:
js-yaml npm version 4.1.1 ESM import support
💡 Result:
Answer: js-yaml@4.1.1 does not provide native ESM import support for Node.js as a package-level feature; it’s primarily CommonJS, and the project historically declined to rewrite for ESM/ES6 import syntax. In practice, you can still use it from ESM via default/CommonJS interop, or by importing a dedicated ESM build from a CDN path (not the npm package entrypoint).
Citations:
- 1: https://www.npmjs.com/package/js-yaml
- 2: Support ES6 Import Syntax nodeca/js-yaml#531
- 3: Convert library code to ESM to improve generated ESM exports nodeca/js-yaml#598
- 4: https://app.unpkg.com/js-yaml@4.1.1
🏁 Script executed:
# Verify: check if there are any other uses of this suppression in the codebase
rg "@typescript-eslint/no-require-imports" --type ts --type jsRepository: NVIDIA/NemoClaw
Length of output: 1033
🏁 Script executed:
# Double-check the test section of eslint.config.mjs for any missed rules
grep -A 30 'files:.*test' eslint.config.mjsRepository: NVIDIA/NemoClaw
Length of output: 1640
Remove the stale eslint suppression comment.
The @typescript-eslint/no-require-imports rule is not configured in eslint.config.mjs, so the suppression has no effect and should be removed as dead code. The require() here is intentional; eslint's test configuration explicitly allows require in test files to support CommonJS interop (e.g., js-yaml v4.1.1, which lacks native ESM support). No import pattern change is needed.
🧰 Tools
🪛 ESLint
[error] 8-8: Definition for rule '@typescript-eslint/no-require-imports' was not found.
(@typescript-eslint/no-require-imports)
🤖 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 5 - 9, Remove the stale eslint
suppression comment before the require call in test/config-set.test.ts: delete
the line "// eslint-disable-next-line `@typescript-eslint/no-require-imports`" so
the file uses the intentional CommonJS require for the yaml constant (const yaml
= require("js-yaml") as { load: (s: string) => unknown };) without dead
suppression comments.
ericksoa
added
the
status: superseded
|
Thanks @Sanjays2402 for the contribution here. This PR correctly targeted the #2019 config-set validation gap and helped move the design discussion toward avoiding silent writes for typoed config paths. We have since landed the accepted fix in #2036, which resolved #2019 using the current new-key gate design: unknown paths are refused by default in non-interactive mode unless the user explicitly opts in to creating a new path. That keeps the validator schema-free per the #2400 design constraints while still protecting users from accidental typo writes. Since the issue is now resolved on main by that approach, I am closing this PR as superseded rather than rejected on merit. |
4 participants
Summary
nemoclaw config setaccepted any dotpath, including invalid top-level keys that OpenClaw doesn't recognize. The config was silently written with no validation. This PR adds an allowlist that validates the top-level key from the dotpath against known sections and surfaces a clear error listing the valid options when the key is unknown.Related Issue
Fixes #2019
Changes
Type of Change
Verification
npx prek run --all-filespassesnpm testpassesmake docsbuilds without warnings (doc changes only)Manual repro:
nemoclaw config set bogus.key value— before: silently written. After: rejected with a listing of valid sections.AI Disclosure
Summary by CodeRabbit