refactor(cli): migrate credentials, registry, and policies to TypeScript#1382
Closed
refactor(cli): migrate credentials, registry, and policies to TypeScript#1382
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>
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>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughMigrates core bin/lib implementations to new TypeScript sources under Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Policies
participant Shell as "openshell/gh CLI"
participant FS as "Filesystem (temp file)"
participant Registry
User->>Policies: request applyPreset(sandbox, preset)
Policies->>Policies: validate sandbox & preset
Policies->>Shell: run policy get <sandbox> (may fail)
Shell-->>Policies: currentPolicy (or fallback)
Policies->>Policies: merge preset into currentPolicy (YAML/text)
Policies->>FS: write merged policy to temp file (0600)
FS-->>Policies: temp file path
Policies->>Shell: run policy set <sandbox> <temp file>
Shell-->>Policies: command result
Policies->>FS: remove temp file
Policies->>Registry: withLock -> update sandbox.policies (append)
Registry-->>Policies: updated sandbox entry
Policies-->>User: success/failure
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
src/lib/registry.ts (1)
143-150:getDefault()fallback behavior may be unexpected.When
defaultSandboxis set but refers to a non-existent sandbox, this function falls back to the first sandbox in the list. This could mask data inconsistencies wheredefaultSandboxpoints to a deleted sandbox. Consider whether silently returning a different sandbox is the intended behavior, or if returningnullwould be more appropriate to signal the inconsistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/registry.ts` around lines 143 - 150, The current getDefault() in registry.ts silently falls back to the first sandbox when data.defaultSandbox is set but missing; update getDefault() to treat a dangling defaultSandbox as an inconsistency and return null instead of returning names[0]. Specifically, after calling load(), check if data.defaultSandbox exists and is present in data.sandboxes; if it is return it, otherwise do not fall back to the first key — return null (or optionally log/validate the inconsistency elsewhere). Keep references to load(), data.defaultSandbox and data.sandboxes when making the change.src/lib/credentials.ts (1)
47-68: Minor optimization opportunity ingetCredsFile.
getCredsDir()is called twice when_credsFileis null: once at line 65 and again at line 66. This is a minor inefficiency.💡 Optional: Avoid double call
function getCredsFile(): string { // Ensure dir cache is up to date with current HOME - getCredsDir(); - if (!_credsFile) _credsFile = path.join(getCredsDir(), "credentials.json"); + const dir = getCredsDir(); + if (!_credsFile) _credsFile = path.join(dir, "credentials.json"); return _credsFile; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/credentials.ts` around lines 47 - 68, getCredsFile calls getCredsDir() twice when _credsFile is null; avoid the duplicate call by calling getCredsDir() once, storing the result in a local variable (e.g., dir = getCredsDir()), and then building _credsFile = path.join(dir, "credentials.json"); keep the existing cache-refresh behavior tied to getCredsDir() and reference the symbols getCredsFile, getCredsDir, and _credsFile when making the change.src/lib/config-io.test.ts (1)
75-93: Consider skipping this test on Windows.The chmod-based write failure test relies on Unix filesystem permissions and will behave differently on Windows where
chmodhas limited effect. If Windows CI support is needed, consider conditionally skipping this test.💡 Optional: Skip on Windows
- it("cleans up temp file on write failure", () => { + it("cleans up temp file on write failure", { skip: process.platform === "win32" }, () => {Or use
it.skipIf:it.skipIf(process.platform === "win32")("cleans up temp file on write failure", () => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/config-io.test.ts` around lines 75 - 93, The failing chmod-based test "cleans up temp file on write failure" relies on Unix permissions and should be skipped on Windows; update the test to conditionally skip when process.platform === "win32" (e.g., wrap or gate the it(...) with a platform check or use a skip helper) so the cleanup assertion for writeConfigFile still runs on Unix CI, and keep the existing fs.chmodSync(tmpDir/readonly) and tmp file assertions unchanged for non-Windows runs.
🤖 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/credentials.ts`:
- Around line 319-323: The CREDS_DIR and CREDS_FILE const exports are evaluated
once at import and become stale if HOME changes; change them to be dynamic by
exporting accessor functions or property getters that call getCredsDir() and
getCredsFile() at access time (e.g., replace the const exports CREDS_DIR and
CREDS_FILE with exported functions like _getCredsDir/_getCredsFile or define
exported getters that invoke getCredsDir/getCredsFile), and update any
consumers/tests to call the accessors instead of relying on the frozen const
values.
In `@src/lib/policies.ts`:
- Around line 253-260: The read-modify-write on sandbox policies is vulnerable
to a race: instead of calling registry.getSandbox(...) then mutating and calling
registry.updateSandbox(...), wrap the entire sequence in
registry.withLock(sandboxName, ...) or add/use an atomic
registry.appendPolicy(sandboxName, presetName) that checks presence and appends
under the lock; update the code paths that currently call registry.getSandbox
and registry.updateSandbox to use the locking closure (or the new appendPolicy)
so the include-check and push are performed atomically (referencing
registry.getSandbox, registry.updateSandbox, registry.withLock, and a proposed
registry.appendPolicy).
---
Nitpick comments:
In `@src/lib/config-io.test.ts`:
- Around line 75-93: The failing chmod-based test "cleans up temp file on write
failure" relies on Unix permissions and should be skipped on Windows; update the
test to conditionally skip when process.platform === "win32" (e.g., wrap or gate
the it(...) with a platform check or use a skip helper) so the cleanup assertion
for writeConfigFile still runs on Unix CI, and keep the existing
fs.chmodSync(tmpDir/readonly) and tmp file assertions unchanged for non-Windows
runs.
In `@src/lib/credentials.ts`:
- Around line 47-68: getCredsFile calls getCredsDir() twice when _credsFile is
null; avoid the duplicate call by calling getCredsDir() once, storing the result
in a local variable (e.g., dir = getCredsDir()), and then building _credsFile =
path.join(dir, "credentials.json"); keep the existing cache-refresh behavior
tied to getCredsDir() and reference the symbols getCredsFile, getCredsDir, and
_credsFile when making the change.
In `@src/lib/registry.ts`:
- Around line 143-150: The current getDefault() in registry.ts silently falls
back to the first sandbox when data.defaultSandbox is set but missing; update
getDefault() to treat a dangling defaultSandbox as an inconsistency and return
null instead of returning names[0]. Specifically, after calling load(), check if
data.defaultSandbox exists and is present in data.sandboxes; if it is return it,
otherwise do not fall back to the first key — return null (or optionally
log/validate the inconsistency elsewhere). Keep references to load(),
data.defaultSandbox and data.sandboxes when making the change.
🪄 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: f27d896b-dcb9-487a-92c1-e06e72e90388
📒 Files selected for processing (11)
bin/lib/config-io.jsbin/lib/credentials.jsbin/lib/policies.jsbin/lib/registry.jssrc/lib/config-io.test.tssrc/lib/config-io.tssrc/lib/credentials.tssrc/lib/policies.tssrc/lib/registry.tstest/credentials.test.jstest/registry.test.js
Comment on lines
+319
to
+323
| // Direct exports for TS consumers and jsconfig type checking. | ||
| // These are evaluated at import time but the HOME-aware cache | ||
| // ensures they stay correct across HOME changes in tests. | ||
| export const CREDS_DIR: string = getCredsDir(); | ||
| export const CREDS_FILE: string = getCredsFile(); |
Contributor
There was a problem hiding this comment.
CREDS_DIR and CREDS_FILE exports will be stale if HOME changes after import.
The comment claims these stay correct across HOME changes, but they're evaluated once at import time as const assignments. Tests that stub HOME after importing will see stale values from these exports.
Consider using getters or documenting that consumers should use _getCredsDir()/_getCredsFile() for dynamic resolution.
💡 Suggested fix: Use getters for dynamic resolution
-// Direct exports for TS consumers and jsconfig type checking.
-// These are evaluated at import time but the HOME-aware cache
-// ensures they stay correct across HOME changes in tests.
-export const CREDS_DIR: string = getCredsDir();
-export const CREDS_FILE: string = getCredsFile();
+// Direct exports for TS consumers and jsconfig type checking.
+// Note: These are evaluated at import time. Use _getCredsDir()/_getCredsFile()
+// for dynamic resolution when HOME may change (e.g., in tests).
+export const CREDS_DIR: string = getCredsDir();
+export const CREDS_FILE: string = getCredsFile();Or, if dynamic behavior is required:
// Use Object.defineProperty for lazy evaluation
let _exportedCredsDir: string | undefined;
let _exportedCredsFile: string | undefined;
export const CREDS_DIR = {
get value() { return getCredsDir(); }
}.value; // This won't work for const exports
// Alternative: just export the functions and document usage🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/credentials.ts` around lines 319 - 323, The CREDS_DIR and CREDS_FILE
const exports are evaluated once at import and become stale if HOME changes;
change them to be dynamic by exporting accessor functions or property getters
that call getCredsDir() and getCredsFile() at access time (e.g., replace the
const exports CREDS_DIR and CREDS_FILE with exported functions like
_getCredsDir/_getCredsFile or define exported getters that invoke
getCredsDir/getCredsFile), and update any consumers/tests to call the accessors
instead of relying on the frozen const values.
Comment on lines
+253
to
+260
| const sandbox = registry.getSandbox(sandboxName); | ||
| if (sandbox) { | ||
| const pols = sandbox.policies || []; | ||
| if (!pols.includes(presetName)) { | ||
| pols.push(presetName); | ||
| } | ||
| registry.updateSandbox(sandboxName, { policies: pols }); | ||
| } |
Contributor
There was a problem hiding this comment.
Race condition between getSandbox() and updateSandbox().
The sequence getSandbox → modify policies → updateSandbox is not atomic. Between reading the sandbox state (line 253) and updating it (line 259), another concurrent process could modify the policies array, causing those changes to be overwritten.
Consider using withLock from the registry module to wrap this entire read-modify-write sequence, or add an atomic appendPolicy function to the registry that handles the add-if-not-present logic internally.
🔒 Proposed fix using withLock
+import { withLock } from "./registry";
- const sandbox = registry.getSandbox(sandboxName);
- if (sandbox) {
- const pols = sandbox.policies || [];
- if (!pols.includes(presetName)) {
- pols.push(presetName);
- }
- registry.updateSandbox(sandboxName, { policies: pols });
- }
+ withLock(() => {
+ const sandbox = registry.getSandbox(sandboxName);
+ if (sandbox) {
+ const pols = sandbox.policies || [];
+ if (!pols.includes(presetName)) {
+ pols.push(presetName);
+ }
+ registry.updateSandbox(sandboxName, { policies: pols });
+ }
+ });Note: Since updateSandbox also acquires the lock internally, you may need to implement a non-locking variant or use a reentrant lock pattern.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/policies.ts` around lines 253 - 260, The read-modify-write on sandbox
policies is vulnerable to a race: instead of calling registry.getSandbox(...)
then mutating and calling registry.updateSandbox(...), wrap the entire sequence
in registry.withLock(sandboxName, ...) or add/use an atomic
registry.appendPolicy(sandboxName, presetName) that checks presence and appends
under the lock; update the code paths that currently call registry.getSandbox
and registry.updateSandbox to use the locking closure (or the new appendPolicy)
so the include-check and push are performed atomically (referencing
registry.getSandbox, registry.updateSandbox, registry.withLock, and a proposed
registry.appendPolicy).
wscurran
added
enhancement: refactoring
NemoClaw CLI
The selectFromList function was added to policies.js on main (via #1370) after our TS migration branched. Add the typed implementation to keep the TS module in sync. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
Author
|
Closing this PR as stale/superseded. I re-checked it against current
Leaving #1370 open for maintainer review. |
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
2 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
Convert the last 3 modules that were blocked by #782 to TypeScript:
credentials.js(327 lines) →src/lib/credentials.tsregistry.js(246 lines) →src/lib/registry.tspolicies.js(303 lines) →src/lib/policies.tsDepends on #1370 (config-io module) — merge that first.
716 CLI tests pass. Coverage ratchet passes. Relates to #924.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests