fix: rethrow non-ENOENT in preExistingContent read, clarify drain JSDoc

- preExistingContent try/catch now only swallows ENOENT; rethrows EACCES,
  EISDIR, etc. to prevent data-loss where late-block retraction would delete
  a file instead of restoring prior content
- drainPostHookActions JSDoc clarifies that per-action isolation only holds
  when onError does not rethrow; callers wanting fail-fast (tests) can use
  a rethrowing onError intentionally

Addresses greptile review: data-loss risk + docstring precision.
This commit is contained in:
zeroaltitude 2026-03-09 12:31:10 -07:00
parent 65e0158bf8
commit bfa49e8e74
No known key found for this signature in database
GPG Key ID: 77592FB1C703882E
2 changed files with 15 additions and 2 deletions

View File

@ -397,8 +397,18 @@ const saveSessionToMemory: HookHandler = async (event) => {
await fs.mkdir(memoryDir, { recursive: true });
try {
preExistingContent = await fs.readFile(memoryFilePath, "utf-8");
} catch {
} catch (err: unknown) {
// File doesn't exist yet — normal case, nothing to preserve.
// Rethrow non-ENOENT errors (EACCES, EISDIR, etc.) to avoid silently
// losing preExistingContent, which would cause late-block retraction
// to delete the file instead of restoring a prior session's history.
if (
err instanceof Error &&
"code" in err &&
(err as NodeJS.ErrnoException).code !== "ENOENT"
) {
throw err;
}
}
await writeFileWithinRoot({
rootDir: memoryDir,

View File

@ -313,7 +313,10 @@ export async function triggerInternalHook(event: InternalHookEvent): Promise<voi
* *during* its own execution, those entries land in the already-cleared source
* array and will persist after this function returns a subsequent drain
* call will execute them.
* Errors are caught per-action so one failure doesn't block others.
* Errors are caught per-action via try/catch. Per-action isolation (one
* failure doesn't block others) holds only when `onError` does not rethrow.
* If `onError` rethrows, subsequent actions are skipped callers that want
* fail-fast semantics (e.g. tests) can use this intentionally.
*
* Exported for use in tests that need to drain post-hook actions without
* going through the full triggerInternalHook pipeline, ensuring they