fix: add binaries to policy presets + safe config I/O (#676, #692, #606)#782
Closed
cluster2600 wants to merge 6 commits intoNVIDIA:mainfrom
Closed
fix: add binaries to policy presets + safe config I/O (#676, #692, #606)#782cluster2600 wants to merge 6 commits intoNVIDIA:mainfrom
cluster2600 wants to merge 6 commits intoNVIDIA:mainfrom
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:
📝 WalkthroughWalkthroughAdds a new safe JSON config I/O module with explicit EACCES handling and remediation errors; refactors credentials and registry to use it; validates presets include Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Suggested reviewers
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)
Comment |
![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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
nemoclaw-blueprint/policies/presets/npm.yaml (1)
12-25:⚠️ Potential issue | 🟠 MajorUse
access: fullinstead oftls: terminatewith path-based rules for npm and yarn registries.npm and yarn rely on CONNECT tunneling to proxy traffic. TLS termination with path-based rules will intercept and break CONNECT tunneling, causing authentication failures and connection drops. Switch to
access: fulland rely on the binaries allowlist for the security boundary instead of path filtering.- host: registry.npmjs.org port: 443 access: full enforcement: enforce - host: registry.yarnpkg.com port: 443 access: full enforcement: enforce🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nemoclaw-blueprint/policies/presets/npm.yaml` around lines 12 - 25, Replace the TLS termination and path-based rules for the npm and yarn registry entries (hosts "registry.npmjs.org" and "registry.yarnpkg.com") with full-access entries: remove "tls: terminate", remove the "rules" block (and any "protocol: rest" path-based filtering), and set "access: full" while keeping "port: 443" and "enforcement: enforce"; rely on the binaries allowlist for security instead of path filtering.nemoclaw-blueprint/policies/presets/docker.yaml (1)
12-43:⚠️ Potential issue | 🟠 MajorSwitch docker.yaml endpoints from
tls: terminatetoaccess: full.Docker registries use HTTP CONNECT tunneling for authentication flow, similar to npm and pypi package managers. Using
tls: terminatewith L7 rules breaks CONNECT tunneling, causing authentication failures. This preset should useaccess: fullto allow transparent proxying of CONNECT requests, matching the fix applied to npm and pypi presets in commit e1097a6.Note: npm.yaml and pypi.yaml in the current state have reverted to
tls: terminate(regression from commit e1097a6); they should also be switched back toaccess: full.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nemoclaw-blueprint/policies/presets/docker.yaml` around lines 12 - 43, Replace the L7 TLS termination with transparent proxying for Docker registries: in the host blocks for registry-1.docker.io, auth.docker.io, nvcr.io, and authn.nvidia.com in docker.yaml, change "tls: terminate" to "access: full" so HTTP CONNECT tunneling for auth works; also apply the same change to the npm.yaml and pypi.yaml presets (they were reverted) to restore the earlier commit e1097a6 behavior.nemoclaw-blueprint/policies/presets/pypi.yaml (1)
12-32:⚠️ Potential issue | 🟠 MajorComplete the PyPI preset for real
piptraffic.This still misses two common happy paths from issue
#676:python -m pipis not covered by apip/pip3-only allowlist, andprotocol: rest+tls: terminateis still the wrong model for PyPI’s CONNECT-tunneled traffic. As written, the preset can keep returning 403 or breaking installs even afterbinarieswas added.Suggested fix
pypi: name: pypi endpoints: - host: pypi.org port: 443 - protocol: rest enforcement: enforce - tls: terminate - rules: - - allow: { method: GET, path: "/**" } + access: full - host: files.pythonhosted.org port: 443 - protocol: rest enforcement: enforce - tls: terminate - rules: - - allow: { method: GET, path: "/**" } + access: full binaries: - { path: /usr/local/bin/openclaw } - { path: /usr/local/bin/claude } + - { path: /usr/bin/python } + - { path: /usr/bin/python3 } - { path: /usr/bin/pip } - { path: /usr/bin/pip3 } + - { path: /usr/local/bin/python } + - { path: /usr/local/bin/python3 } - { path: /usr/local/bin/pip } - { path: /usr/local/bin/pip3 }Based on learnings: PyPI/package-manager presets should use
access: fullinstead ofprotocol: rest+tls: terminate, because TLS termination breaks CONNECT-based proxying.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nemoclaw-blueprint/policies/presets/pypi.yaml` around lines 12 - 32, The preset currently uses protocol: rest with tls: terminate for the PyPI hosts and only whitelists pip/pip3 binaries, which breaks CONNECT-tunneled installs and python -m pip; update the two host entries (host: pypi.org and host: files.pythonhosted.org) to use access: full instead of protocol: rest and remove tls: terminate, and extend the binaries list to include python interpreters (e.g., python and python3 locations such as /usr/bin/python, /usr/bin/python3, /usr/local/bin/python, /usr/local/bin/python3) so module-invoked pip (python -m pip) is allowed.
🧹 Nitpick comments (4)
bin/lib/config-io.js (2)
53-54: Temp file name collision risk in concurrent writes.Using only
process.pidin the temp filename could cause collisions ifwriteConfigFileis called concurrently for the same file path within the same process (e.g., from parallel async operations). Consider adding a random suffix or counter.♻️ Proposed fix
- const tmpFile = filePath + ".tmp." + process.pid; + const tmpFile = filePath + ".tmp." + process.pid + "." + Math.random().toString(36).slice(2, 8);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/lib/config-io.js` around lines 53 - 54, The temp filename generation in writeConfigFile currently uses only process.pid which can collide on concurrent writes; update the tmpFile creation (variable tmpFile in the writeConfigFile function) to include an additional unique suffix (e.g., a random hex string, timestamp, or an incrementing counter) so each invocation produces a distinct temporary filename for the same filePath, then use that tmpFile for the atomic write and rename flow.
77-93: Silent fallback on corrupt JSON may mask configuration issues.Returning
defaultValueon JSON parse errors (line 90-91) could hide misconfigured files from users. Consider logging a warning when a file exists but cannot be parsed.💡 Optional: Add warning for corrupt config files
function readConfigFile(filePath, defaultValue) { try { if (fs.existsSync(filePath)) { return JSON.parse(fs.readFileSync(filePath, "utf-8")); } } catch (err) { if (err.code === "EACCES") { throw new ConfigPermissionError( `Cannot read config file: ${filePath}`, filePath, err ); } - // Corrupt JSON or other non-permission error — return default + // Corrupt JSON or other non-permission error — warn and return default + if (err instanceof SyntaxError) { + console.warn(`Warning: Config file ${filePath} contains invalid JSON, using defaults`); + } } return defaultValue; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/lib/config-io.js` around lines 77 - 93, The readConfigFile function currently swallows JSON parse errors and returns defaultValue silently; update readConfigFile to detect when fs.existsSync(filePath) is true but JSON.parse throws (excluding EACCES), log or warn with the filePath and parse error details (e.g., use an existing logger or console.warn) so users are notified of a corrupt/malformed config, but keep the current behavior of returning defaultValue and still throwing ConfigPermissionError for EACCES; reference the readConfigFile function and the existing EACCES handling when making the change.bin/lib/runner.js (1)
52-53: Consider redacting before truncating.Currently the command is sliced to 80 characters before redaction. If a secret spans the 80-character boundary, part of it could be exposed in the error message.
Safer ordering
- console.error(` Command failed (exit ${result.status}): ${redactSecrets(cmd.slice(0, 80))}`); + console.error(` Command failed (exit ${result.status}): ${redactSecrets(cmd).slice(0, 80)}`);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/lib/runner.js` around lines 52 - 53, The current error message truncates the command before calling redactSecrets, which can reveal partial secrets; change the call order so you first build the full command string (the existing cmd array), call redactSecrets on the complete command (redactSecrets(cmd.join(' ') or the existing string form)), then truncate the redacted result to 80 characters for the console.error output; update the console.error that currently uses redactSecrets(cmd.slice(0, 80)) to redactSecrets(...) then .slice(0, 80) so no secrets can be split by the truncation.bin/lib/policies.js (1)
123-124: Validation result is intentionally unused.The
validatePreset()return value is not used to gate preset application, which means presets without binaries will still be applied (with a warning). This seems intentional for backward compatibility, but consider whether blocking might be more appropriate now that all presets have been updated.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/lib/policies.js` around lines 123 - 124, The call to validatePreset(presetContent, presetName) currently ignores its return value so presets lacking binaries still proceed; update the calling code (where validatePreset is invoked) to capture the return result and gate application so that when validatePreset(...) indicates invalid/missing binaries you skip applying the preset (or throw/return an error) and log the existing warning; reference validatePreset, presetContent and presetName in your change and ensure the flow short-circuits preset application when validation fails.
🤖 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/config-io.js`:
- Around line 111-113: The HOME fallback is using the literal "~" which won't
expand to an actual home directory; update buildRemediation to use os.homedir()
as the fallback (or forgo process.env.HOME and call os.homedir() directly), e.g.
set home = process.env.HOME || os.homedir(), ensure the os module is
required/imported, and keep using path.join to build nemoclawDir so the
remediation path is a real filesystem path rather than "~/.nemoclaw".
In `@test/config-io.test.js`:
- Around line 55-80: The test currently relies on POSIX chmod behavior to
trigger EACCES; instead, in the tests around ensureConfigDir replace the
chmod-driven scenarios with a stub/mock of the specific fs call that
ensureConfigDir uses (e.g., fs.access or fs.mkdir or fs.stat as used by
ensureConfigDir) to throw an error object with { code: "EACCES" } so the code
path that maps EACCES to ConfigPermissionError is exercised deterministically;
keep the existing real-permission smoke checks only behind a platform and uid
guard (skip on Windows or when process.getuid() === 0) so CI/root containers
don't break — update the cases at lines noted (55-80, 112-122, 145-155) to use
the stub approach and assert the same ConfigPermissionError properties.
In `@test/policies.test.js`:
- Around line 113-116: The test "has a binaries section" currently checks for a
raw substring in the string returned by policies.loadPreset("local-inference");
instead parse the YAML output (using the project's YAML parser, e.g., js-yaml
safeLoad/safeLoadAll) from policies.loadPreset and assert on the parsed
structure: verify that the top-level network_policies key exists and that each
expected policy entry contains a binaries key (and that binaries is the expected
type/shape), rather than using content.includes("binaries:"); update the related
assertions in the same test block (and the other tests at lines ~150-180) to
validate network_policies.*.binaries structurally.
In `@test/runner.test.js`:
- Around line 269-276: Current test only checks for the literal echo form and
can miss other unsafe expansions; update the "walkthrough.sh does not echo raw
API key value" test so it asserts that the script does NOT contain any unescaped
expansion of NVIDIA_API_KEY in output commands (e.g., detect patterns like
echo/printf/... followed by $NVIDIA_API_KEY or ${NVIDIA_API_KEY} with optional
quoting/whitespace). Replace the src.includes check with a negative regex test
against src (e.g.,
/(echo|printf|cat|printf)\s+.*(\$NVIDIA_API_KEY|\${NVIDIA_API_KEY})/ or similar)
and keep the same assertion message.
- Around line 255-266: The current test allows false-positives because it only
checks for any "env:" substring in the setupSpark body; update the assertion to
inspect the actual command construction or call site inside async function
setupSpark instead of a generic body.includes check: locate the command
invocation or spawn/exec call created by setupSpark (search for the function
name setupSpark and the call sites that build the command string or pass
options), then assert that there is no literal "NVIDIA_API_KEY=" concatenated
into the command string (e.g., not present in the argument passed to spawn/exec)
unless the API key appears only inside an env object passed as the options
parameter (e.g., an object literal with an env property), and replace the
current body/assert that uses match/body with a regex or parse that targets the
specific call site to validate that NVIDIA_API_KEY is only set via an env option
and not embedded in the command string.
---
Outside diff comments:
In `@nemoclaw-blueprint/policies/presets/docker.yaml`:
- Around line 12-43: Replace the L7 TLS termination with transparent proxying
for Docker registries: in the host blocks for registry-1.docker.io,
auth.docker.io, nvcr.io, and authn.nvidia.com in docker.yaml, change "tls:
terminate" to "access: full" so HTTP CONNECT tunneling for auth works; also
apply the same change to the npm.yaml and pypi.yaml presets (they were reverted)
to restore the earlier commit e1097a6 behavior.
In `@nemoclaw-blueprint/policies/presets/npm.yaml`:
- Around line 12-25: Replace the TLS termination and path-based rules for the
npm and yarn registry entries (hosts "registry.npmjs.org" and
"registry.yarnpkg.com") with full-access entries: remove "tls: terminate",
remove the "rules" block (and any "protocol: rest" path-based filtering), and
set "access: full" while keeping "port: 443" and "enforcement: enforce"; rely on
the binaries allowlist for security instead of path filtering.
In `@nemoclaw-blueprint/policies/presets/pypi.yaml`:
- Around line 12-32: The preset currently uses protocol: rest with tls:
terminate for the PyPI hosts and only whitelists pip/pip3 binaries, which breaks
CONNECT-tunneled installs and python -m pip; update the two host entries (host:
pypi.org and host: files.pythonhosted.org) to use access: full instead of
protocol: rest and remove tls: terminate, and extend the binaries list to
include python interpreters (e.g., python and python3 locations such as
/usr/bin/python, /usr/bin/python3, /usr/local/bin/python,
/usr/local/bin/python3) so module-invoked pip (python -m pip) is allowed.
---
Nitpick comments:
In `@bin/lib/config-io.js`:
- Around line 53-54: The temp filename generation in writeConfigFile currently
uses only process.pid which can collide on concurrent writes; update the tmpFile
creation (variable tmpFile in the writeConfigFile function) to include an
additional unique suffix (e.g., a random hex string, timestamp, or an
incrementing counter) so each invocation produces a distinct temporary filename
for the same filePath, then use that tmpFile for the atomic write and rename
flow.
- Around line 77-93: The readConfigFile function currently swallows JSON parse
errors and returns defaultValue silently; update readConfigFile to detect when
fs.existsSync(filePath) is true but JSON.parse throws (excluding EACCES), log or
warn with the filePath and parse error details (e.g., use an existing logger or
console.warn) so users are notified of a corrupt/malformed config, but keep the
current behavior of returning defaultValue and still throwing
ConfigPermissionError for EACCES; reference the readConfigFile function and the
existing EACCES handling when making the change.
In `@bin/lib/policies.js`:
- Around line 123-124: The call to validatePreset(presetContent, presetName)
currently ignores its return value so presets lacking binaries still proceed;
update the calling code (where validatePreset is invoked) to capture the return
result and gate application so that when validatePreset(...) indicates
invalid/missing binaries you skip applying the preset (or throw/return an error)
and log the existing warning; reference validatePreset, presetContent and
presetName in your change and ensure the flow short-circuits preset application
when validation fails.
In `@bin/lib/runner.js`:
- Around line 52-53: The current error message truncates the command before
calling redactSecrets, which can reveal partial secrets; change the call order
so you first build the full command string (the existing cmd array), call
redactSecrets on the complete command (redactSecrets(cmd.join(' ') or the
existing string form)), then truncate the redacted result to 80 characters for
the console.error output; update the console.error that currently uses
redactSecrets(cmd.slice(0, 80)) to redactSecrets(...) then .slice(0, 80) so no
secrets can be split by the truncation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 157b818e-cbd9-45a3-996f-e33af9525b0b
⛔ Files ignored due to path filters (1)
research/results.tsvis excluded by!**/*.tsv
📒 Files selected for processing (25)
bin/lib/config-io.jsbin/lib/credentials.jsbin/lib/onboard.jsbin/lib/policies.jsbin/lib/registry.jsbin/lib/runner.jsbin/nemoclaw.jsnemoclaw-blueprint/policies/presets/discord.yamlnemoclaw-blueprint/policies/presets/docker.yamlnemoclaw-blueprint/policies/presets/huggingface.yamlnemoclaw-blueprint/policies/presets/jira.yamlnemoclaw-blueprint/policies/presets/local-inference.yamlnemoclaw-blueprint/policies/presets/npm.yamlnemoclaw-blueprint/policies/presets/outlook.yamlnemoclaw-blueprint/policies/presets/pypi.yamlnemoclaw-blueprint/policies/presets/slack.yamlnemoclaw-blueprint/policies/presets/telegram.yamlscripts/install.shscripts/telegram-bridge.jsscripts/walkthrough.shtest/config-io.test.jstest/install-preflight.test.jstest/policies.test.jstest/runner.test.jsuninstall.sh
Comment on lines
+55
to
+80
| it("throws ConfigPermissionError when directory is not writable", () => { | ||
| fs.mkdirSync(testDir, { recursive: true, mode: 0o700 }); | ||
| fs.chmodSync(testDir, 0o500); // read + execute only | ||
| const subDir = path.join(testDir, "sub"); | ||
| assert.throws( | ||
| () => ensureConfigDir(subDir), | ||
| (err) => { | ||
| assert.ok(err instanceof ConfigPermissionError); | ||
| assert.equal(err.code, "EACCES"); | ||
| assert.ok(err.remediation.includes("chown")); | ||
| return true; | ||
| } | ||
| ); | ||
| }); | ||
|
|
||
| it("throws ConfigPermissionError when existing dir is read-only", () => { | ||
| fs.mkdirSync(testDir, { recursive: true, mode: 0o500 }); | ||
| assert.throws( | ||
| () => ensureConfigDir(testDir), | ||
| (err) => { | ||
| assert.ok(err instanceof ConfigPermissionError); | ||
| assert.ok(err.message.includes("not writable")); | ||
| return true; | ||
| } | ||
| ); | ||
| }); |
Contributor
There was a problem hiding this comment.
Avoid chmod-driven EACCES assertions in unit tests.
These cases depend on POSIX permission semantics and a non-root test user, so they can flip on Windows or containerized/root CI even when config-io is correct. Since the behavior under test is the EACCES mapping, stub the relevant fs call to throw { code: "EACCES" } and keep any real permission smoke test behind a platform/uid guard.
Also applies to: 112-122, 145-155
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/config-io.test.js` around lines 55 - 80, The test currently relies on
POSIX chmod behavior to trigger EACCES; instead, in the tests around
ensureConfigDir replace the chmod-driven scenarios with a stub/mock of the
specific fs call that ensureConfigDir uses (e.g., fs.access or fs.mkdir or
fs.stat as used by ensureConfigDir) to throw an error object with { code:
"EACCES" } so the code path that maps EACCES to ConfigPermissionError is
exercised deterministically; keep the existing real-permission smoke checks only
behind a platform and uid guard (skip on Windows or when process.getuid() === 0)
so CI/root containers don't break — update the cases at lines noted (55-80,
112-122, 145-155) to use the stub approach and assert the same
ConfigPermissionError properties.
Contributor
Author
There was a problem hiding this comment.
Fair point about root/containerised CI. In practice the NemoClaw test suite runs on Ubuntu runners as a non-root user, so the chmod approach works reliably here. Happy to stub fs if it becomes flaky, but it hasn't been an issue so far.
Contributor
There was a problem hiding this comment.
@cluster2600, that's a fair and reasonable call — controlled non-root Ubuntu runners make the POSIX permission semantics reliable here. I'll defer to your judgment on this. Thanks for the context!
✏️ Learnings added
Learnt from: cluster2600
Repo: NVIDIA/NemoClaw PR: 782
File: test/config-io.test.js:54-75
Timestamp: 2026-03-24T11:21:29.520Z
Learning: In the NVIDIA/NemoClaw repo, the test suite (including `test/config-io.test.js`) runs on Ubuntu CI runners as a non-root user. Chmod-driven EACCES assertions (e.g., `fs.chmodSync(dir, 0o500)` to simulate permission errors) are intentional and reliable in this environment. Do not flag these as fragile or platform-unsafe unless there is evidence of root/Windows CI usage.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: Ryuketsukami
Repo: NVIDIA/NemoClaw PR: 146
File: test/policies.test.js:62-69
Timestamp: 2026-03-24T02:55:40.487Z
Learning: When reviewing Vitest-based tests in this repo (where Vitest is configured with `globals: true`), don’t flag common assertion/test helpers as “undefined” when they’re used without imports. Specifically, allow Vitest global usages such as `describe`, `it`, `expect`, `assert`, and `chai` without explicit imports—e.g., don’t complain that `assert.ok`, `assert.equal`, or `assert.deepEqual` are undefined, since they’re provided by Vitest’s global setup.
Learnt from: Ryuketsukami
Repo: NVIDIA/NemoClaw PR: 146
File: test/policies.test.js:24-25
Timestamp: 2026-03-24T02:55:46.698Z
Learning: For test files in this repo (e.g., `test/**`), Vitest may be configured with `globals: true`. In that setup, it is intentional to use Vitest APIs (like `describe`/`it` and assertion helpers such as `assert.*` and `expect`) without importing them explicitly. If `globals: true` is enabled for the relevant Vitest config, do not flag missing imports for these globals.
test/policies.test.js
Outdated
Comment on lines
+113
to
+116
| it("has a binaries section", () => { | ||
| const content = policies.loadPreset("local-inference"); | ||
| assert.ok(content.includes("binaries:"), "must have binaries section (ref: #676)"); | ||
| }); |
Contributor
There was a problem hiding this comment.
Make the new binaries regressions structural.
These assertions only look for raw substrings. A top-level or mis-indented binaries: block would still pass here even though network_policies.*.binaries is what fixes the 403 in #676. Please parse the YAML or at least assert the expected key placement instead of includes("binaries:").
Also applies to: 150-180
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/policies.test.js` around lines 113 - 116, The test "has a binaries
section" currently checks for a raw substring in the string returned by
policies.loadPreset("local-inference"); instead parse the YAML output (using the
project's YAML parser, e.g., js-yaml safeLoad/safeLoadAll) from
policies.loadPreset and assert on the parsed structure: verify that the
top-level network_policies key exists and that each expected policy entry
contains a binaries key (and that binaries is the expected type/shape), rather
than using content.includes("binaries:"); update the related assertions in the
same test block (and the other tests at lines ~150-180) to validate
network_policies.*.binaries structurally.
Contributor
Author
There was a problem hiding this comment.
No YAML parser in the project — same reasoning as PR #781. The string checks are tight enough for what we're validating (presence of the binaries block), and consistent with the rest of the test suite.
Contributor
There was a problem hiding this comment.
@cluster2600, understood — no YAML parser in the project and the includes() pattern is consistent across the whole test suite. I'll withdraw the suggestion.
🐇 hops off the soapbox
✏️ Learnings added
Learnt from: cluster2600
Repo: NVIDIA/NemoClaw PR: 782
File: test/policies.test.js:0-0
Timestamp: 2026-03-24T11:21:32.128Z
Learning: In NVIDIA/NemoClaw, test files (e.g., `test/policies.test.js`) intentionally use raw substring checks (`content.includes("binaries:")`, `content.includes("network_policies:")`, etc.) to validate YAML preset structure rather than parsing YAML. This is a deliberate project convention — no YAML parser dependency is added just for tests. Do not flag these substring assertions as insufficient or recommend switching to structural YAML parsing.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: Ryuketsukami
Repo: NVIDIA/NemoClaw PR: 146
File: test/policies.test.js:62-69
Timestamp: 2026-03-24T02:55:40.487Z
Learning: When reviewing Vitest-based tests in this repo (where Vitest is configured with `globals: true`), don’t flag common assertion/test helpers as “undefined” when they’re used without imports. Specifically, allow Vitest global usages such as `describe`, `it`, `expect`, `assert`, and `chai` without explicit imports—e.g., don’t complain that `assert.ok`, `assert.equal`, or `assert.deepEqual` are undefined, since they’re provided by Vitest’s global setup.
Learnt from: Ryuketsukami
Repo: NVIDIA/NemoClaw PR: 146
File: test/policies.test.js:24-25
Timestamp: 2026-03-24T02:55:46.698Z
Learning: For test files in this repo (e.g., `test/**`), Vitest may be configured with `globals: true`. In that setup, it is intentional to use Vitest APIs (like `describe`/`it` and assertion helpers such as `assert.*` and `expect`) without importing them explicitly. If `globals: true` is enabled for the relevant Vitest config, do not flag missing imports for these globals.
cluster2600
force-pushed
the
fix/04-presets-and-config-io
branch
from
87364ce to
73a488e
Compare
Contributor
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 `@scripts/telegram-bridge.js`:
- Around line 105-114: The spawned SSH call using spawn(...) relies on
SendEnv/NVIDIA_API_KEY but offers no detection or clear error when the remote
sshd drops the variable; update the logic around spawn/proc to check the child
process output and exit code for a missing API key error (or detect within the
child if NVIDIA_API_KEY is absent) and log a descriptive error that mentions
SendEnv and the sshd AcceptEnv requirement (reference symbols: safeSessionId,
cmd, spawn call that creates proc, NVIDIA_API_KEY, confPath, SANDBOX);
alternatively add a short comment next to the cmd/spawn block explaining that
the remote sshd must have AcceptEnv NVIDIA_API_KEY in sshd_config and that
failures will cause the agent to error due to a missing API key.
In `@test/runner.test.js`:
- Line 261: The test has a mismatched-parentheses bug in the expectation: fix
the call to includes on the body variable by closing the includes(...) before
applying the negation or by asserting false; e.g., update the expect expression
referencing expect, body.includes and toBe so that the includes call is closed
and the negation is outside the parentheses (or use .toBe(false)) to make the
assertion syntactically valid.
- Line 267: The test line has a mismatched parenthesis in the expect call — fix
the assertion in test/runner.test.js by closing the includes(...) call and make
the intent explicit: either wrap the negation like expect(!src.includes('echo "
export NVIDIA_API_KEY=$NVIDIA_API_KEY"')).toBe(true) or (preferably) assert
absence with expect(src.includes('echo " export
NVIDIA_API_KEY=$NVIDIA_API_KEY"')).toBe(false); update the includes() and
expect() usage accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 69bc5581-9baa-49a8-b4ee-8b64b29dccfb
📒 Files selected for processing (23)
bin/lib/config-io.jsbin/lib/credentials.jsbin/lib/policies.jsbin/lib/registry.jsbin/lib/runner.jsbin/nemoclaw.jsnemoclaw-blueprint/policies/presets/discord.yamlnemoclaw-blueprint/policies/presets/docker.yamlnemoclaw-blueprint/policies/presets/huggingface.yamlnemoclaw-blueprint/policies/presets/jira.yamlnemoclaw-blueprint/policies/presets/npm.yamlnemoclaw-blueprint/policies/presets/outlook.yamlnemoclaw-blueprint/policies/presets/pypi.yamlnemoclaw-blueprint/policies/presets/slack.yamlnemoclaw-blueprint/policies/presets/telegram.yamlscripts/install.shscripts/telegram-bridge.jsscripts/walkthrough.shtest/config-io.test.jstest/install-preflight.test.jstest/policies.test.jstest/runner.test.jsuninstall.sh
✅ Files skipped from review due to trivial changes (7)
- nemoclaw-blueprint/policies/presets/npm.yaml
- nemoclaw-blueprint/policies/presets/pypi.yaml
- test/install-preflight.test.js
- nemoclaw-blueprint/policies/presets/jira.yaml
- nemoclaw-blueprint/policies/presets/slack.yaml
- bin/lib/config-io.js
- test/config-io.test.js
🚧 Files skipped from review as they are similar to previous changes (9)
- bin/lib/policies.js
- nemoclaw-blueprint/policies/presets/huggingface.yaml
- nemoclaw-blueprint/policies/presets/telegram.yaml
- bin/nemoclaw.js
- nemoclaw-blueprint/policies/presets/docker.yaml
- nemoclaw-blueprint/policies/presets/discord.yaml
- scripts/walkthrough.sh
- scripts/install.sh
- bin/lib/credentials.js
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
test/runner.test.js (1)
264-268:⚠️ Potential issue | 🟠 MajorBroaden the walkthrough leak detection beyond one exact echo line.
Line 267 only blocks one literal form and can miss equivalent unsafe expansions (
printf, different spacing/quoting,${NVIDIA_API_KEY}), letting leaks slip through with a green test.🔧 Suggested test hardening
- // The script should reference the variable name, not expand it unsafely - expect(src.includes('echo " export NVIDIA_API_KEY=$NVIDIA_API_KEY"')).toBe(false); + // Block unescaped NVIDIA_API_KEY expansion in output-producing commands + const unsafeOutputExpansion = + /\b(?:echo|printf|cat)\b[^\n]*(?<!\\)\$(?:\{NVIDIA_API_KEY\}|NVIDIA_API_KEY)\b/; + expect(unsafeOutputExpansion.test(src)).toBe(false);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/runner.test.js` around lines 264 - 268, The test "walkthrough.sh does not echo raw API key value" only checks one exact echo string; update it to detect any unsafe expansions of NVIDIA_API_KEY by asserting the script does NOT match a regex that catches common leak patterns (e.g. commands like echo or printf containing $NVIDIA_API_KEY or ${NVIDIA_API_KEY}, or export/assignment that interpolates the variable), for example replace the current includes() assertion with a negative regex test against src such as /\b(?:echo|printf|export|=)\b[^;\n]*\$\{?NVIDIA_API_KEY\}?/ to catch various spacing/quoting variants and ensure the test fails on any unsafe expansion of NVIDIA_API_KEY.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@test/runner.test.js`:
- Around line 264-268: The test "walkthrough.sh does not echo raw API key value"
only checks one exact echo string; update it to detect any unsafe expansions of
NVIDIA_API_KEY by asserting the script does NOT match a regex that catches
common leak patterns (e.g. commands like echo or printf containing
$NVIDIA_API_KEY or ${NVIDIA_API_KEY}, or export/assignment that interpolates the
variable), for example replace the current includes() assertion with a negative
regex test against src such as
/\b(?:echo|printf|export|=)\b[^;\n]*\$\{?NVIDIA_API_KEY\}?/ to catch various
spacing/quoting variants and ensure the test fails on any unsafe expansion of
NVIDIA_API_KEY.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1c0a1846-fbf5-475b-9dd6-027d72466d29
📒 Files selected for processing (2)
bin/lib/config-io.jstest/runner.test.js
✅ Files skipped from review due to trivial changes (1)
- bin/lib/config-io.js
cluster2600
force-pushed
the
fix/04-presets-and-config-io
branch
from
1e4ec59 to
6c492fa
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
test/runner.test.js (1)
130-178: Add one integration test for runner error-path redaction.These tests validate
redactSecretsdirectly, but they don’t assert redaction behavior inrun()/runInteractive()log output. A focused test around failed command logging would catch regressions in the call-site flow.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/runner.test.js` around lines 130 - 178, Add an integration test that exercises the runner's error-path to ensure call-site logging is redacted: require runnerPath and call run(...) (and/or runInteractive(...)) with a failing command whose output or env contains a secret token (e.g., nvapi- or ghp_ token), capture the logger output by stubbing/mocking the runner's logger/console used by run/runInteractive, then assert the captured logs do NOT contain the raw secret string and DO contain "<REDACTED>" (and that redactSecrets still behaves the same); reference the run, runInteractive and redactSecrets symbols when locating where to hook the logger and assert redaction.
🤖 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/runner.js`:
- Around line 52-53: The log currently slices cmd before redaction which can
leak partial secrets; update both occurrences that call console.error (the one
using cmd.slice(0, 80) and the similar block at lines 67-68) to first call
redactSecrets on the full cmd string/array (e.g., redactSecrets(cmd) or
redactSecrets(cmd.join(' ')) as appropriate) and then truncate the
already-redacted result for display (e.g., take .slice(0, 80) from the redacted
string); keep the same message formatting and still use
process.exit(result.status || 1).
In `@test/policies.test.js`:
- Around line 171-176: The test "every preset binaries section includes
openclaw" in policies.test.js currently iterates over policies.listPresets() and
calls policies.loadPreset(p.name) expecting "/usr/local/bin/openclaw" for every
preset, but npm.yaml and pypi.yaml lack that binary so the assertion fails; fix
by changing the test to either (A) only assert inclusion for presets that should
include openclaw (filter out presets by name e.g. skip "npm" and "pypi"), or (B)
update the preset files (npm.yaml and pypi.yaml) to add
"/usr/local/bin/openclaw" to their binaries section; locate the test using the
it(...) block and the calls to policies.listPresets() and
policies.loadPreset(p.name) to implement the chosen fix.
---
Nitpick comments:
In `@test/runner.test.js`:
- Around line 130-178: Add an integration test that exercises the runner's
error-path to ensure call-site logging is redacted: require runnerPath and call
run(...) (and/or runInteractive(...)) with a failing command whose output or env
contains a secret token (e.g., nvapi- or ghp_ token), capture the logger output
by stubbing/mocking the runner's logger/console used by run/runInteractive, then
assert the captured logs do NOT contain the raw secret string and DO contain
"<REDACTED>" (and that redactSecrets still behaves the same); reference the run,
runInteractive and redactSecrets symbols when locating where to hook the logger
and assert redaction.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e3063028-7bd1-461c-99f2-61d53ab7edaf
📒 Files selected for processing (19)
bin/lib/config-io.jsbin/lib/credentials.jsbin/lib/policies.jsbin/lib/registry.jsbin/lib/runner.jsnemoclaw-blueprint/policies/presets/discord.yamlnemoclaw-blueprint/policies/presets/docker.yamlnemoclaw-blueprint/policies/presets/huggingface.yamlnemoclaw-blueprint/policies/presets/jira.yamlnemoclaw-blueprint/policies/presets/outlook.yamlnemoclaw-blueprint/policies/presets/slack.yamlnemoclaw-blueprint/policies/presets/telegram.yamlscripts/install.shscripts/telegram-bridge.jstest/config-io.test.jstest/install-preflight.test.jstest/policies.test.jstest/runner.test.jsuninstall.sh
✅ Files skipped from review due to trivial changes (9)
- nemoclaw-blueprint/policies/presets/discord.yaml
- nemoclaw-blueprint/policies/presets/huggingface.yaml
- uninstall.sh
- nemoclaw-blueprint/policies/presets/jira.yaml
- nemoclaw-blueprint/policies/presets/slack.yaml
- scripts/telegram-bridge.js
- bin/lib/config-io.js
- test/config-io.test.js
- nemoclaw-blueprint/policies/presets/telegram.yaml
🚧 Files skipped from review as they are similar to previous changes (5)
- nemoclaw-blueprint/policies/presets/outlook.yaml
- bin/lib/credentials.js
- nemoclaw-blueprint/policies/presets/docker.yaml
- test/install-preflight.test.js
- bin/lib/registry.js
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
test/policies.test.js (2)
171-178: Test name is slightly misleading with skip list.The title says “every preset” but excludes
npmandpypi. Consider renaming to reflect scope (e.g., “non-package presets include openclaw”).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/policies.test.js` around lines 171 - 178, Update the test title to accurately reflect the skipped presets: change the it(...) description from "every preset binaries section includes openclaw" to something like "non-package presets include openclaw" (or "presets excluding npm and pypi include openclaw") so the name matches the skip logic; leave the body using the skip array, policies.listPresets(), and policies.loadPreset(p.name) unchanged.
180-190: Test title doesn’t match asserted presets.This block includes
docker, so “package manager presets” is imprecise. Consider renaming or splitting docker into its own test.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/policies.test.js` around lines 180 - 190, The test titled "package manager presets include their tool binaries" asserts on npm, pypi and docker presets but the title only mentions package managers; either rename the test to reflect all three presets (e.g., include docker) or split into two tests (one for package managers using policies.loadPreset("npm") and policies.loadPreset("pypi") and another for docker using policies.loadPreset("docker")). Update the test title(s) accordingly and keep the existing assertions for policies.loadPreset and the expected binary paths.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test/policies.test.js`:
- Around line 171-178: Update the test title to accurately reflect the skipped
presets: change the it(...) description from "every preset binaries section
includes openclaw" to something like "non-package presets include openclaw" (or
"presets excluding npm and pypi include openclaw") so the name matches the skip
logic; leave the body using the skip array, policies.listPresets(), and
policies.loadPreset(p.name) unchanged.
- Around line 180-190: The test titled "package manager presets include their
tool binaries" asserts on npm, pypi and docker presets but the title only
mentions package managers; either rename the test to reflect all three presets
(e.g., include docker) or split into two tests (one for package managers using
policies.loadPreset("npm") and policies.loadPreset("pypi") and another for
docker using policies.loadPreset("docker")). Update the test title(s)
accordingly and keep the existing assertions for policies.loadPreset and the
expected binary paths.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7632c6b2-66a3-4257-9daf-976440289054
📒 Files selected for processing (2)
bin/lib/runner.jstest/policies.test.js
Contributor
|
Hey @cluster2600 — #961 just landed and likely introduced merge conflicts here since both touch |
2 tasks
cluster2600
force-pushed
the
fix/04-presets-and-config-io
branch
from
4c167aa to
87873ae
Compare
Contributor
Author
|
Rebased onto main with the #961 changes. Conflicts in credentials.js, registry.js, and runner.js resolved. |
cluster2600
force-pushed
the
fix/04-presets-and-config-io
branch
from
87873ae to
704b214
Compare
wscurran
added
bug
Contributor
|
✨ Thanks for submitting this PR with a detailed summary, it addresses a bug with policy presets and proposes a fix to improve the reliability of NemoClaw, which could enhance the user experience. |
cluster2600
force-pushed
the
fix/04-presets-and-config-io
branch
from
704b214 to
531cce8
Compare
This was referenced Apr 1, 2026
cv
added a commit
that referenced
this pull request
Apr 1, 2026
…ript modules (#1240) ## Summary - Extract ~210 lines of pure, side-effect-free functions from the 3,800-line `onboard.js` into **5 typed TypeScript modules** under `src/lib/`: - `gateway-state.ts` — gateway/sandbox state classification from openshell output - `validation.ts` — failure classification, API key validation, model ID checks - `url-utils.ts` — URL normalization, text compaction, env formatting - `build-context.ts` — Docker build context filtering, recovery hints - `dashboard.ts` — dashboard URL resolution and construction - Add **56 co-located unit tests** (`src/lib/*.test.ts`) for the extracted modules - Set up CLI TypeScript compilation: `tsconfig.src.json` compiles `src/` → `dist/` as CJS - `onboard.js` imports from compiled `dist/lib/` output — transparent to callers - Pre-commit hook updated to build TS and include `dist/lib/` in coverage These functions are **not touched by any #924 blocker PR** (#781, #782, #819, #672, #634, #890), so this extraction is safe to land immediately. ## Test plan - [x] 598 CLI tests pass (542 existing + 56 new) - [x] `tsc -p tsconfig.src.json` compiles cleanly - [x] `tsc -p tsconfig.cli.json` type-checks cleanly - [x] `tsc -p jsconfig.json` type-checks cleanly - [x] Coverage ratchet passes with `dist/lib/` included Closes #1237. Relates to #924 (shell consolidation). 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Improved sandbox-creation recovery hints and targeted remediation commands. * Smarter dashboard URL resolution and control-UI URL construction. * **Bug Fixes** * More accurate gateway and sandbox state detection. * Enhanced classification of validation/apply failures and safer model/key validation. * Better provider URL normalization and loopback handling. * **Tests** * Added comprehensive tests covering new utilities. * **Chores** * CLI now builds before CLI tests; CI/commit hooks updated to run the CLI build. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
laitingsheng
pushed a commit
that referenced
this pull request
Apr 2, 2026
…ript modules (#1240) ## Summary - Extract ~210 lines of pure, side-effect-free functions from the 3,800-line `onboard.js` into **5 typed TypeScript modules** under `src/lib/`: - `gateway-state.ts` — gateway/sandbox state classification from openshell output - `validation.ts` — failure classification, API key validation, model ID checks - `url-utils.ts` — URL normalization, text compaction, env formatting - `build-context.ts` — Docker build context filtering, recovery hints - `dashboard.ts` — dashboard URL resolution and construction - Add **56 co-located unit tests** (`src/lib/*.test.ts`) for the extracted modules - Set up CLI TypeScript compilation: `tsconfig.src.json` compiles `src/` → `dist/` as CJS - `onboard.js` imports from compiled `dist/lib/` output — transparent to callers - Pre-commit hook updated to build TS and include `dist/lib/` in coverage These functions are **not touched by any #924 blocker PR** (#781, #782, #819, #672, #634, #890), so this extraction is safe to land immediately. ## Test plan - [x] 598 CLI tests pass (542 existing + 56 new) - [x] `tsc -p tsconfig.src.json` compiles cleanly - [x] `tsc -p tsconfig.cli.json` type-checks cleanly - [x] `tsc -p jsconfig.json` type-checks cleanly - [x] Coverage ratchet passes with `dist/lib/` included Closes #1237. Relates to #924 (shell consolidation). 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Improved sandbox-creation recovery hints and targeted remediation commands. * Smarter dashboard URL resolution and control-UI URL construction. * **Bug Fixes** * More accurate gateway and sandbox state detection. * Enhanced classification of validation/apply failures and safer model/key validation. * Better provider URL normalization and loopback handling. * **Tests** * Added comprehensive tests covering new utilities. * **Chores** * CLI now builds before CLI tests; CI/commit hooks updated to run the CLI build. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This was referenced Apr 2, 2026
cv
requested changes
Apr 2, 2026
Contributor
cv
left a comment
cv
left a comment
There was a problem hiding this comment.
The config-io module and preset binary fixes are valuable — both address real user-facing issues.
Two things need addressing before this can land:
1. Drop the runner.js secret redaction. #1246 just merged with a comprehensive implementation covering stdio interception, URL credentials, error fields, and stack traces. The redactSecrets() here is now superseded and will conflict. Please rebase onto main and remove the runner.js changes.
2. Resolve the merge conflict. There are <<<<<<< HEAD markers visible in test/runner.test.js. A rebase onto current main should clean this up (especially now that #1246 has landed).
After the rebase, the remaining scope (config-io.js atomic writes, permission error handling, preset binaries validation) is clean and ready to go. Thanks @cluster2600 for keeping this one alive.
…VIDIA#693) RFC1918 addresses were blocked by the egress proxy because no policy entry existed for host.openshell.internal. Add a local-inference preset that whitelists ports 11434 (Ollama) and 8000 (vLLM) with binary restrictions, and auto-suggest it during onboard when a local inference provider is selected.
Policy presets without a binaries section cause 403 errors because the egress proxy has no approved binary list and denies all traffic. - Add binaries (openclaw, claude) to all 9 affected presets - Package manager presets additionally allow their tool binaries (pip for pypi, npm/node/yarn for npm, docker for docker) - Add validatePreset() that warns on missing binaries at apply time - Add 3 regression tests ensuring all presets have binaries - Fix installer HTTPS remediation test (shadow real nemoclaw binary) - Fix installer to print GitHub URL when binary not found
…IA#606, NVIDIA#719) Add config-io.js module providing atomic JSON config file writes (write-to-temp + rename) with proper EACCES error detection and user-facing remediation hints. Refactor credentials.js and registry.js to use the shared module instead of raw fs calls. When ~/.nemoclaw/ is owned by root or has wrong permissions, users now get a clear error message with fix commands instead of a cryptic EACCES stack trace.
Fix mismatched parentheses in three expect() assertions that would cause syntax errors at runtime. Use os.homedir() instead of "~" as the HOME fallback in buildRemediation(). Signed-off-by: Maxime Grenu <maxime.grenu@gmail.com>
- Remove runner.js secret redaction (superseded by NVIDIA#1246) - Fix CREDS_FILE → getCredsFile() in credentials.js - Restore fs require in registry.js (needed by lock code) - Restore local-inference.yaml preset - Fix stale conflict marker in runner tests Signed-off-by: Maxime Grenu <maxime.grenu@gmail.com>
- Add fs require back to credentials.js and registry.js (needed by lock code and resolveHomeDir alongside config-io) - Restore openclaw/claude binaries in npm and pypi presets - Update preset count test from 9 to 10 (local-inference added) - Update registry save test to match config-io error format Signed-off-by: Maxime Grenu <maxime.grenu@gmail.com>
cluster2600
force-pushed
the
fix/04-presets-and-config-io
branch
from
531cce8 to
b6c3a96
Compare
Contributor
Author
|
Addressed both points:
871/875 tests pass (1 pre-existing preflight failure, 3 skipped). The scope is now just:
|
cv
added a commit
that referenced
this pull request
Apr 2, 2026
…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>
3 tasks
Contributor
|
Superseded by #1370 (config-io + preset validation) and #1246 (runner secret redaction). Both have landed or are ready to merge on current main without the merge conflicts this PR had. Thanks @cluster2600 for the original work. |
cv
added a commit
that referenced
this pull request
Apr 2, 2026
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>
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
3 participants
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
Policy presets: all 9 presets now include a
binariessection.validatePreset()warns on missing binaries.Config I/O: new
config-io.jsmodule with atomic writes (tmp + rename) andEACCES→ConfigPermissionErrorwith remediation hints.sequenceDiagram participant App participant ConfigIO participant FS App->>ConfigIO: writeConfigFile(path, data) ConfigIO->>FS: write to path.tmp ConfigIO->>FS: rename path.tmp → path Note over ConfigIO,FS: On EACCES → ConfigPermissionError with hint18 new tests. Closes #676, #692, #606.
Test plan
Summary by CodeRabbit
New Features
Bug Fixes
Chores
Tests