-
Notifications
You must be signed in to change notification settings - Fork 975
fix(checkpointing): skip checkpoint when cwd is the home directory #1294
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -10,6 +10,7 @@ | |||||
| * @module CheckpointStoreLive | ||||||
| */ | ||||||
| import { randomUUID } from "node:crypto"; | ||||||
| import { homedir } from "node:os"; | ||||||
|
|
||||||
| import { Effect, Layer, FileSystem, Path } from "effect"; | ||||||
|
|
||||||
|
|
@@ -91,6 +92,19 @@ const makeCheckpointStore = Effect.gen(function* () { | |||||
| Effect.gen(function* () { | ||||||
| const operation = "CheckpointStore.captureCheckpoint"; | ||||||
|
|
||||||
| // Skip checkpointing for the home directory — git add -A on ~/ scans | ||||||
| // thousands of unrelated files and will time out. | ||||||
| const resolvedCwd = yield* Effect.try(() => path.resolve(input.cwd)); | ||||||
| const home = yield* Effect.try(() => homedir()); | ||||||
|
||||||
| const home = yield* Effect.try(() => homedir()); | |
| const home = yield* Effect.try(() => path.resolve(homedir())); |
Copilot
AI
Mar 22, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This introduces a new behavioral branch (skipping checkpoint capture when cwd is the home directory) but there’s no direct regression test covering it. Consider adding a test that sets cwd to os.homedir() and asserts captureCheckpoint fails with CheckpointInvariantError (and that no git commands are executed), so this guard doesn’t regress.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
path.resolve(input.cwd)andhomedir()are synchronous/non-throwing; wrapping them inEffect.try(() => …)unnecessarily introduces anUnknownExceptionfailure path (and can widen the effect error type beyondCheckpointStoreError). Prefer computing these values directly (or useEffect.sync/Effect.succeed), and if you want to guard against unexpected exceptions, map them into aCheckpointInvariantErrorso the service’s typed error channel stays consistent.