feat(core): add ctx.dotdir — scoped dot directory API#105
Merged
zrosenbauer merged 6 commits intomainfrom Mar 24, 2026
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
🦋 Changeset detectedLatest commit: ea39498 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
…ction Adds DotDirectoryClient on the command context providing scoped filesystem operations for CLI dot directories. Includes a protection registry that middleware can populate to guard sensitive files (e.g. auth.json) from accidental handler access. Co-Authored-By: Claude <noreply@anthropic.com>
zrosenbauer
force-pushed
the
feat/dotdir
branch
from
d4125e2 to
e71584a
Compare
|
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 DotDirectory subsystem and integrates it into CommandContext. New public types and exports for dotdir APIs and errors, plus factories: createDotDirectory, createDotDirectoryClient, and createProtectionRegistry. Implements a synchronous filesystem-backed DotDirectoryClient with JSON helpers, path-traversal and symlink checks, protection enforcement, and result-typed error handling. createContext now constructs and attaches Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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}
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 `@packages/core/src/lib/dotdir/create-dot-directory-client.test.ts`:
- Around line 53-68: The test currently allows both success and failure by
branching on the Result from client.local(), making it nondeterministic; update
the test to assert success deterministically by keeping the vi.stubEnv('PWD',
tmpDir) setup (which includes creating tmpDir with a .git) and replacing the
conditional with a direct assertion that error is falsy (or null) and that
dotdir.dir contains DIR_NAME—i.e., call createDotDirectoryClient(...), run const
[error, dotdir] = client.local(), expect(error).toBeFalsy() (or
expect(error).toBeNull()) and then expect(dotdir.dir).toContain(DIR_NAME); refer
to createDotDirectoryClient and the client.local() call to locate the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 40a26443-51e2-4fa9-8763-97fd76a6801a
⛔ Files ignored due to path filters (1)
.changeset/dotdir.mdis excluded by!.changeset/**
📒 Files selected for processing (13)
packages/core/src/context/create-context.tspackages/core/src/context/types.tspackages/core/src/index.tspackages/core/src/lib/dotdir/create-dot-directory-client.test.tspackages/core/src/lib/dotdir/create-dot-directory-client.tspackages/core/src/lib/dotdir/create-dot-directory.test.tspackages/core/src/lib/dotdir/create-dot-directory.tspackages/core/src/lib/dotdir/index.tspackages/core/src/lib/dotdir/protection.test.tspackages/core/src/lib/dotdir/protection.tspackages/core/src/lib/dotdir/types.tspackages/core/src/middleware/auth/auth.test.tspackages/core/src/middleware/auth/auth.ts
packages/core/src/lib/dotdir/create-dot-directory-client.test.ts
Outdated
Show resolved
Hide resolved
DotDirectory is now the root manager on ctx.dotdir (with .global(), .local(), .protect()), and DotDirectoryClient is the scoped filesystem handle returned by .global() and .local(). Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/core/src/lib/dotdir/create-dot-directory-client.ts`:
- Around line 199-207: The returned error for jsonStringify failures currently
uses type 'fs_error' which is misleading for a serialization failure; update the
error object returned by the jsonStringify branch inside
create-dot-directory-client (the block that checks stringifyError after calling
jsonStringify(data, { pretty: true })) to use a more appropriate error type —
either change type: 'fs_error' to type: 'parse_error' for symmetry with
readJson, or introduce and use a new type like 'serialize_error' (and ensure any
callers handling error.type are updated accordingly); keep the message using
stringifyError.message and filename unchanged.
In `@packages/core/src/lib/dotdir/create-dot-directory.test.ts`:
- Around line 53-68: The test for createDotDirectory::local() is
non-deterministic because resolveLocalPath/findProjectRoot may use process.cwd()
rather than the stubbed PWD; make the test deterministic by explicitly stubbing
the resolver or process.cwd(): either mock resolveLocalPath (or the module that
exports it) to return a known Result pointing at tmpDir before calling
createDotDirectory/local(), or use vi.spyOn(process,
'cwd').mockReturnValue(tmpDir) so findProjectRoot resolves predictably;
reference the createDotDirectory factory and the local() method when applying
the stub.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 75bc2bb6-c6c4-45d5-a2cb-0c861f5345fb
📒 Files selected for processing (9)
packages/core/src/context/create-context.tspackages/core/src/context/types.tspackages/core/src/lib/dotdir/create-dot-directory-client.test.tspackages/core/src/lib/dotdir/create-dot-directory-client.tspackages/core/src/lib/dotdir/create-dot-directory.test.tspackages/core/src/lib/dotdir/create-dot-directory.tspackages/core/src/lib/dotdir/index.tspackages/core/src/lib/dotdir/protection.tspackages/core/src/lib/dotdir/types.ts
- Make local() test deterministic by mocking process.cwd() instead of using conditional assertions - Change writeJson serialization error type from fs_error to parse_error for consistency with readJson Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/core/src/lib/dotdir/create-dot-directory-client.ts`:
- Around line 66-69: The code is vulnerable to symlink escape because filesystem
calls follow symlinks; fix by validating and resolving paths: at initialization
resolve the configured dir with realpathSync and use lstatSync on each path
component to detect and reject any symlink in the configured dotdir (throw a
DotDirectoryError if found), and for every file operation in ensure(), read(),
write(), remove(), and fileExists() call realpathSync on the target path and
verify the resolved path has the dotdir realpath as its prefix (reject if it
escapes the boundary); use the existing Result/DotDirectoryError flow to return
errors when symlinks or path escapes are detected.
- Around line 42-58: checkProtection currently uses the raw filename while
filesystem ops use join(dir, filename), allowing path traversal like
"../../../etc/passwd" to bypass registry checks; modify the code paths
(checkProtection, readJson, writeJson and any call sites using join(dir,
filename)) to first normalize and resolve the filename to an absolute/canonical
path (e.g., path.resolve or fs.realpath equivalent), compute the target path =
resolve(dir, filename), verify the target path startsWith the directory root to
prevent escaping, then use that canonical target for both the protection
registry lookup (pass the normalized relative/canonical filename used by
registry) and all filesystem operations; ensure checkProtection uses the
normalized identifier that matches what registry.has(location, ...) expects and
reject access if the resolved path is outside dir unless
dangerouslyAccessProtectedFile is true.
In `@packages/core/src/lib/dotdir/create-dot-directory.test.ts`:
- Around line 9-10: The tests use mutable shared state (globalHome and tmpDir)
which is reassigned across hooks and read by the module mock; replace the shared
let bindings with a per-test fixture helper that returns local consts (e.g.,
createFixture() -> { globalHome, tmpDir }) and update each test to call that
helper and use its returned consts; then update the module mock setup to capture
the per-test values (e.g., set the mock implementation inside the test or use
jest.isolateModules/jest.doMock per test) so the mock reads the local consts
instead of the module-scoped variables; remove the top-level let declarations
and any hooks that mutate them so each test is order-independent.
- Around line 53-60: The test currently uses
expect(dotdir.dir).toContain(DIR_NAME) which is too loose; change the assertion
to verify the exact resolved local path by comparing dotdir.dir to the
resolved/joined path built from tmpDir and DIR_NAME (e.g., use path.join or
path.resolve with tmpDir and DIR_NAME) after calling createDotDirectory and
client.local(), so the test asserts that local() resolved against tmpDir rather
than matching any path containing DIR_NAME.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7fa7f9f1-52a1-48ba-b016-845a0a3068e5
📒 Files selected for processing (2)
packages/core/src/lib/dotdir/create-dot-directory-client.tspackages/core/src/lib/dotdir/create-dot-directory.test.ts
- Validate filenames resolve within the scoped directory boundary - Reject symbolic links via lstatSync check - Add path_traversal error type to DotDirectoryError - Add 5 path traversal tests covering read, write, remove, readJson, exists - Tighten local() test assertion to exact path match - Move test let bindings to module scope with JSDoc explaining the exception Co-Authored-By: Claude <noreply@anthropic.com>
… path(), seal internals - Consolidate safePath() to single lstatSync instead of existsSync + lstatSync - Route path() through safePath() for traversal/symlink protection - Reuse ensure() in write() instead of duplicating mkdirSync - Remove ProtectionRegistry/createProtectionRegistry from barrel exports Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
packages/core/src/lib/dotdir/create-dot-directory-client.ts (2)
77-92:⚠️ Potential issue | 🔴 CriticalProtected files are still bypassable through normalized filenames.
checkProtection()compares the raw caller input, butsafePath()canonicalizes the actual target later. Withauth.jsonprotected,read('./auth.json')orwrite('sub/../auth.json', ...)reaches the same file withoutdangerouslyAccessProtectedFile, because the registry lookup and filesystem target are derived from different names. Normalize first, then run the protection lookup against the canonical relative filename.Also applies to: 127-136, 164-170, 290-296
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/lib/dotdir/create-dot-directory-client.ts` around lines 77 - 92, checkProtection currently checks protection using the raw caller filename which allows bypass via inputs like './auth.json' or 'sub/../auth.json'; change it to first compute the canonical relative path with the same normalization used by safePath (i.e., resolve/normalize to the repository-relative target) and then run registry.has against that canonical filename inside checkProtection. Update all other places that perform protection checks (the other checkProtection-like calls around the file read/write handlers referenced in the diff) to use the normalized/canonical filename before calling registry.has so registry lookup and filesystem target use the same canonical name.
43-66:⚠️ Potential issue | 🔴 CriticalParent symlinks still let filesystem operations escape the scoped directory.
Line 55 only inspects the leaf path. If
resolvedDiritself, or any segment under it, is a symlink,safePath('cache/config.json')still passes becauselstatSync(resolved)follows parent symlinks and a missing target just yieldsENOENT. The laterreadFileSync/writeFileSync/unlinkSyncthen operate outside the dotdir boundary. This still needs canonical-root validation of each existing path component (or an equivalentrealpath-based parent-path check) before any filesystem access.Does Node.js fs.lstatSync() detect symlinks in parent path components, or only the final path entry? What is the recommended way to prevent readFileSync/writeFileSync/unlinkSync from escaping an allowed base directory through nested symlinks?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/lib/dotdir/create-dot-directory-client.ts` around lines 43 - 66, safePath currently only lstatSync()s the final resolved path which allows parent-directory symlinks to escape the dotdir; change it to validate canonical parent paths: compute the realpath (realpathSync) of resolvedDir once and for each requested filename compute the realpath of the deepest existing parent (or walk components from resolvedDir to the target calling lstatSync on each existing segment) and reject if any segment is a symbolic link or if the realpath of the target/parent does not start with the canonical resolvedDir path; update safePath to use these checks (replace the single lstatSync(resolved) check) so subsequent readFileSync/writeFileSync/unlinkSync cannot escape the allowed base.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/core/src/lib/dotdir/types.ts`:
- Around line 88-104: The exported DotDirectoryClient methods (read, write,
readJson, writeJson, remove) currently use positional multi-arg signatures which
will lock API shape; change each to accept a single object parameter (e.g.,
ReadParams, WriteParams, ReadJsonParams<T>, WriteJsonParams, RemoveParams or
Options variants) so callers pass named properties (filename, content/data,
options) and update the method types in types.ts (the readonly read, write,
readJson, writeJson, exists, remove entries) to use those object param types;
ensure generic readJson keeps its <T> and that optional fields remain optional
in the new param types, and update any internal references/call sites to
destructure the new param objects accordingly.
- Around line 71-73: The readJson generic signature allows callers to assert T
without runtime validation; change ReadJsonOptions<T> so schema is required
(remove optional) and add overloads on DotDirectoryClient.readJson so calls
without a schema return Result<unknown, DotDirectoryError> while calls passing
ReadJsonOptions<T> with schema return Result<T, DotDirectoryError>; update the
implementation of readJson to perform zod.parse when schema is present and cast
only after validation. Also refactor multi-parameter methods on
DotDirectoryClient (read, write, readJson, writeJson, remove) to use a single
options object parameter (object destructuring) per the project standard and
update their signatures/consumers accordingly.
---
Duplicate comments:
In `@packages/core/src/lib/dotdir/create-dot-directory-client.ts`:
- Around line 77-92: checkProtection currently checks protection using the raw
caller filename which allows bypass via inputs like './auth.json' or
'sub/../auth.json'; change it to first compute the canonical relative path with
the same normalization used by safePath (i.e., resolve/normalize to the
repository-relative target) and then run registry.has against that canonical
filename inside checkProtection. Update all other places that perform protection
checks (the other checkProtection-like calls around the file read/write handlers
referenced in the diff) to use the normalized/canonical filename before calling
registry.has so registry lookup and filesystem target use the same canonical
name.
- Around line 43-66: safePath currently only lstatSync()s the final resolved
path which allows parent-directory symlinks to escape the dotdir; change it to
validate canonical parent paths: compute the realpath (realpathSync) of
resolvedDir once and for each requested filename compute the realpath of the
deepest existing parent (or walk components from resolvedDir to the target
calling lstatSync on each existing segment) and reject if any segment is a
symbolic link or if the realpath of the target/parent does not start with the
canonical resolvedDir path; update safePath to use these checks (replace the
single lstatSync(resolved) check) so subsequent
readFileSync/writeFileSync/unlinkSync cannot escape the allowed base.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: adfda406-985f-4d4f-a3a9-c5558255068f
📒 Files selected for processing (4)
packages/core/src/lib/dotdir/create-dot-directory-client.test.tspackages/core/src/lib/dotdir/create-dot-directory-client.tspackages/core/src/lib/dotdir/index.tspackages/core/src/lib/dotdir/types.ts
- Replace toBe(true/false) with toBeTruthy/toBeFalsy - Capitalize section separator comments - Resolve stale stash conflicts in docs (keep CommandContext naming) Co-Authored-By: Claude <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
1 participant
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
DotDirectoryClientonctx.dotdirwith scoped filesystem operations for CLI dot directoriesctx.dotdir.global()returns aDotDirectoryfor~/.myapp/,ctx.dotdir.local()returnsResult<DotDirectory, DotDirectoryError>for<project>/.myapp/DotDirectoryprovides:ensure,read,write,readJson,writeJson,exists,remove,pathauth.json) from accidental handler accessauth.jsonviactx.dotdir.protect()Stacked on #104
Test plan
pnpm checkpasses (typecheck + lint + format)pnpm testpasses (780 tests — 36 new dotdir tests)dangerouslyAccessProtectedFileopt-in bypasses protectionreadJson