fix(cli): add config-io module for safe config file I/O and preset validation#1370
fix(cli): add config-io module for safe config file I/O and preset validation#1370
Conversation
…lidation Add src/lib/config-io.ts with atomic JSON read/write (temp + rename), EACCES error handling with user-facing remediation hints, and directory permission enforcement. - Refactor credentials.js to use readConfigFile/writeConfigFile - Refactor registry.js to use readConfigFile/writeConfigFile - Add validatePreset() to policies.js (warns on missing binaries section) - ConfigPermissionError with actionable remediation (sudo chown / rm) - Co-located tests for config-io module Fixes #692, #606. Supersedes the config-io and preset validation parts of #782 (without the runner.js redaction, which landed separately in #1246). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughCreated new Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application
participant CIO as config-io
participant FS as Filesystem
App->>CIO: writeConfigFile(path, data)
CIO->>CIO: ensureConfigDir(parent)
CIO->>FS: mkdir -p (0o700)
FS-->>CIO: success or EACCES
alt Permission Denied
CIO->>CIO: throw ConfigPermissionError<br/>(with remediation)
CIO-->>App: ConfigPermissionError
else Success
CIO->>FS: write to temp file (0o600)
FS-->>CIO: fd
CIO->>FS: renameSync(temp, target)
FS-->>CIO: success or EACCES
alt Rename Failed
CIO->>FS: unlink temp file
CIO->>CIO: throw ConfigPermissionError
CIO-->>App: ConfigPermissionError
else Success
CIO-->>App: void (success)
end
end
sequenceDiagram
participant App as Application
participant CIO as config-io
participant FS as Filesystem
App->>CIO: readConfigFile(path, default)
CIO->>FS: stat/access file
FS-->>CIO: exists, readable, or error
alt EACCES
CIO->>CIO: throw ConfigPermissionError
CIO-->>App: ConfigPermissionError
else File Not Found
CIO-->>App: return default
else File Exists
CIO->>FS: readFileSync(path)
FS-->>CIO: content
CIO->>CIO: JSON.parse(content)
alt Parse Error
CIO-->>App: return default
else Parse Success
CIO-->>App: return parsed<T>
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 docstrings
🧪 Generate unit tests (beta)
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: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@bin/lib/policies.js`:
- Around line 65-73: The current check in validatePreset using
presetContent.includes("binaries:") can be fooled by comments or scalars;
instead parse the YAML and detect a real top-level key. Replace the substring
check in validatePreset with a YAML parse (e.g., yaml.parse / js-yaml.safeLoad)
of presetContent and then check for Object.prototype.hasOwnProperty.call(parsed,
"binaries") (and that parsed.binaries is not undefined/null); if parsing fails,
log a warning and treat the preset as missing the binaries section so the
original warning is emitted for presetName.
In `@src/lib/config-io.ts`:
- Around line 29-41: The current buildRemediation() message assumes sudo is
available; update it to include non-sudo fallback actions so environments
without sudo get actionable guidance. In the buildRemediation() function
(reference: nemoclawDir and HOME usage) add alternative suggestions such as
recreating the config under the current user's home (e.g., remove or move the
directory if writable), instructing to remove the directory without sudo when
the user owns it (e.g., rm -rf $HOME/.nemoclaw), and advising to relocate or
initialize config in a user-writable path (for example creating a new config
under $HOME or specifying an alternative CONFIG_HOME), so the error message
covers both sudo and non-sudo environments. Ensure the text clearly
distinguishes when sudo is required vs when the non-sudo command applies.
🪄 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
Run ID: b236f71c-5208-4de3-9a9b-e2b0d59af3a1
📒 Files selected for processing (7)
bin/lib/config-io.jsbin/lib/credentials.jsbin/lib/policies.jsbin/lib/registry.jssrc/lib/config-io.test.tssrc/lib/config-io.tstest/registry.test.js
| function validatePreset(presetContent, presetName) { | ||
| if (!presetContent.includes("binaries:")) { | ||
| console.warn( | ||
| ` Warning: preset '${presetName}' has no binaries section — ` + | ||
| `this will cause 403 errors in the sandbox (ref: #676)`, | ||
| ); | ||
| return false; | ||
| } | ||
| return true; |
There was a problem hiding this comment.
Detect the actual YAML key, not a raw substring.
presetContent.includes("binaries:") also matches comments, descriptions, or other scalar text, so a preset can still lack the real binaries section and skip the warning. Please parse the preset or at least anchor the check to a YAML key.
Suggested fix
function validatePreset(presetContent, presetName) {
- if (!presetContent.includes("binaries:")) {
+ let parsed = null;
+ try {
+ parsed = YAML.parse(presetContent);
+ } catch {
+ parsed = null;
+ }
+ if (
+ !parsed ||
+ typeof parsed !== "object" ||
+ !Object.prototype.hasOwnProperty.call(parsed, "binaries")
+ ) {
console.warn(
` Warning: preset '${presetName}' has no binaries section — ` +
`this will cause 403 errors in the sandbox (ref: `#676`)`,
);
return false;
}
return true;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bin/lib/policies.js` around lines 65 - 73, The current check in
validatePreset using presetContent.includes("binaries:") can be fooled by
comments or scalars; instead parse the YAML and detect a real top-level key.
Replace the substring check in validatePreset with a YAML parse (e.g.,
yaml.parse / js-yaml.safeLoad) of presetContent and then check for
Object.prototype.hasOwnProperty.call(parsed, "binaries") (and that
parsed.binaries is not undefined/null); if parsing fails, log a warning and
treat the preset as missing the binaries section so the original warning is
emitted for presetName.
| const { readConfigFile, writeConfigFile } = require("./config-io"); | ||
|
|
There was a problem hiding this comment.
Lock acquisition still bypasses the new config permission handling.
save() now wraps the final file write, but every mutating path reaches acquireLock() first. Permission failures creating ~/.nemoclaw or ${REGISTRY_FILE}.lock still escape as plain fs errors, so the new remediation never reaches the main registerSandbox / updateSandbox / removeSandbox / setDefault flows that issue #692 targets.
Also applies to: 124-129
| function buildRemediation(): string { | ||
| const home = process.env.HOME || os.homedir(); | ||
| const nemoclawDir = path.join(home, ".nemoclaw"); | ||
| return [ | ||
| " To fix, run one of:", | ||
| "", | ||
| ` sudo chown -R $(whoami) ${nemoclawDir}`, | ||
| ` # or, if the directory was created by another user:`, | ||
| ` sudo rm -rf ${nemoclawDir} && nemoclaw onboard`, | ||
| "", | ||
| " This usually happens when NemoClaw was first run with sudo", | ||
| " or the config directory was created by a different user.", | ||
| ].join("\n"); |
There was a problem hiding this comment.
Remediation still assumes sudo exists.
Issue #692 explicitly includes environments where sudo is unavailable, but both suggested fixes here still require it. In those installs the new ConfigPermissionError is still not actionable. Please add a non-sudo fallback, e.g. recreating config under a user-writable HOME or config directory.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/config-io.ts` around lines 29 - 41, The current buildRemediation()
message assumes sudo is available; update it to include non-sudo fallback
actions so environments without sudo get actionable guidance. In the
buildRemediation() function (reference: nemoclawDir and HOME usage) add
alternative suggestions such as recreating the config under the current user's
home (e.g., remove or move the directory if writable), instructing to remove the
directory without sudo when the user owns it (e.g., rm -rf $HOME/.nemoclaw), and
advising to relocate or initialize config in a user-writable path (for example
creating a new config under $HOME or specifying an alternative CONFIG_HOME), so
the error message covers both sudo and non-sudo environments. Ensure the text
clearly distinguishes when sudo is required vs when the non-sudo command
applies.
Convert the last 3 blocked-by-#782 CJS modules to TypeScript: - credentials.js → src/lib/credentials.ts - registry.js → src/lib/registry.ts - policies.js → src/lib/policies.ts 716 CLI tests pass. Coverage ratchet passes. Depends on #1370 (config-io module). Relates to #924. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
ericksoa
left a comment
ericksoa
left a comment
There was a problem hiding this comment.
Nice consolidation — the deduplication of atomic-write logic from credentials.js and registry.js into a shared module is clean, and the ConfigPermissionError with remediation hints is a real UX win for #692.
A few items worth considering before merge (inline comments below).
| this.remediation = remediation; | ||
| if (cause) this.cause = cause; | ||
| } | ||
| } |
There was a problem hiding this comment.
The old credentials.js had fs.chmodSync(dir, 0o700) to enforce permissions on every write, even for pre-existing directories. mkdirSync only applies mode at creation time — if ~/.nemoclaw already exists at 0o755, it stays that way. Would it make sense to add a chmodSync after the mkdirSync to preserve the old hardening behavior?
| dir, | ||
| err as Error, | ||
| ); | ||
| } |
There was a problem hiding this comment.
Minor: existsSync + readFileSync has a small TOCTOU window. Since the catch already handles errors, could this just call readFileSync directly and catch ENOENT alongside corrupt JSON? Simpler and avoids the race.
|
|
||
| const fs = require("fs"); | ||
| const path = require("path"); | ||
| const { readConfigFile, writeConfigFile } = require("./config-io"); |
There was a problem hiding this comment.
acquireLock() does its own fs.mkdirSync(path.dirname(REGISTRY_FILE), ...) before save()/load() are ever reached. If ~/.nemoclaw is root-owned, users hit a plain fs EACCES error without the new remediation. Would it make sense to use ensureConfigDir here too?
| return false; | ||
| } | ||
| return true; | ||
| } |
There was a problem hiding this comment.
validatePreset returns false on missing binaries but applyPreset ignores the return value and continues applying the preset. If the point of #676 is preventing 403 errors, should this bail out early when validation fails?
2 participants
Summary
src/lib/config-io.ts: atomic JSON read/write (temp + rename), EACCES error handling withConfigPermissionErrorand user-facing remediation hintscredentials.jsto usereadConfigFile/writeConfigFile(removes manual fs + chmod)registry.jsto usereadConfigFile/writeConfigFile(removes manual atomic write)validatePreset()topolicies.js— warns when a preset lacksbinaries:section (All policy presets and default telegram/discord policies return 403 due to missingbinariessection #676)Supersedes the config-io and preset validation parts of #782 (without the runner.js redaction, which landed separately in #1246). Fixes #692, #606.
Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests
Refactor