fix(config): validate dotpath key against known top-level sections #2071
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,6 +2,11 @@ | |
| // SPDX-License-Identifier: Apache-2.0 | ||
|
|
||
| import { describe, it, expect } from "vitest"; | ||
| 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 }; | ||
|
Comment on lines
+5
to
+9
Contributor
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:
💡 Result: Answer: [email protected] 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:
🏁 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 🧰 Tools🪛 ESLint[error] 8-8: Definition for rule '@typescript-eslint/no-require-imports' was not found. ( 🤖 Prompt for AI Agents |
||
|
|
||
| // Build must run before these tests (imports from dist/) | ||
| const { extractDotpath, setDotpath, validateUrlValue, resolveAgentConfig } = require("../dist/lib/sandbox-config"); | ||
|
|
@@ -23,6 +28,61 @@ describe("resolveAgentConfig", () => { | |
| const target = resolveAgentConfig("any-sandbox"); | ||
| expect(target.configPath.endsWith(target.configFile)).toBe(true); | ||
| }); | ||
|
|
||
| it("defaults knownSections to null when manifest is absent (e.g. fallback)", () => { | ||
| const target = resolveAgentConfig("nonexistent-sandbox"); | ||
| expect(target.knownSections).toBe(null); | ||
| }); | ||
| }); | ||
|
|
||
| describe("config set top-level section validation (manifest-aware)", () => { | ||
| // Tracks the design discussion on #2071 / #2400: the validator must NOT | ||
| // hardcode an OpenClaw-specific allowlist. It must read `config.known_sections` | ||
| // from the agent's manifest, and skip validation entirely when the agent | ||
| // doesn't declare one (so agents like Hermes with their own schema aren't | ||
| // blocked). | ||
|
|
||
| const sandboxConfigSource = fs.readFileSync( | ||
| path.join(import.meta.dirname, "..", "src", "lib", "sandbox-config.ts"), | ||
| "utf-8", | ||
| ); | ||
|
|
||
| it("does not contain a hardcoded KNOWN_TOP_LEVEL_KEYS allowlist", () => { | ||
| assert.doesNotMatch(sandboxConfigSource, /KNOWN_TOP_LEVEL_KEYS\s*=\s*new Set/); | ||
| }); | ||
|
|
||
| it("reads the allowlist from the agent's manifest via target.knownSections", () => { | ||
| assert.match(sandboxConfigSource, /target\.knownSections/); | ||
| assert.match( | ||
| sandboxConfigSource, | ||
| /if \(target\.knownSections && target\.knownSections\.length > 0\)/, | ||
| ); | ||
| }); | ||
|
|
||
| it("declares known_sections in the openclaw agent manifest", () => { | ||
| const manifest = yaml.load( | ||
| fs.readFileSync( | ||
| path.join(import.meta.dirname, "..", "agents", "openclaw", "manifest.yaml"), | ||
| "utf-8", | ||
| ), | ||
| ) as { config?: { known_sections?: string[] } }; | ||
| expect(Array.isArray(manifest.config?.known_sections)).toBe(true); | ||
| expect(manifest.config!.known_sections!.length).toBeGreaterThan(0); | ||
| // Spot-check a few essentials. | ||
| for (const key of ["llm", "sandbox", "extensions"]) { | ||
| expect(manifest.config!.known_sections).toContain(key); | ||
| } | ||
| }); | ||
|
|
||
| it("does NOT declare known_sections in the hermes agent manifest (intentionally permissive)", () => { | ||
| const manifest = yaml.load( | ||
| fs.readFileSync( | ||
| path.join(import.meta.dirname, "..", "agents", "hermes", "manifest.yaml"), | ||
| "utf-8", | ||
| ), | ||
| ) as { config?: { known_sections?: string[] } }; | ||
| expect(manifest.config?.known_sections).toBeUndefined(); | ||
| }); | ||
| }); | ||
|
|
||
| describe("config set helpers", () => { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Return
nullwhen the filtered allowlist is empty.If
config.known_sectionscontains only non-string entries, this getter currently returns[], which makesconfigSetsilently skip validation for a malformed manifest. Normalizing an empty filtered result back tonullkeeps 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