PR #4 needs changes
PR #3 is merged. PR #4 is rebased onto main, but the disk adapter has three reproduced issues.
3 open findings: 2 P1, 1 P2. No GitHub review comments posted.
+462 −0 7f1beb3Reviewed on 23 September 2026 at 7f1beb3ce6476c50168ad07158572ee149ec4912, based on 798f104e04d627446cd9c4c4a245b11bceb5303e. PR #3 merged as 798f104. PR #4 rebased without conflicts and was pushed with an explicit lease. Both rebased commits are patch-identical to the original stack commits. GitHub now lists main as its base.
Architecture and complexity
The small Node-only adapter is a sensible implementation of the existing port. The kernel remains independent of filesystem code. The short-write loop, per-handle failure latch and read-through snapshots are useful and pass independent controls.
The missing invariant is that every accepted recovery state must also be safe for the next write. Reading around a broken tail is only half of recovery. Repairing that boundary, establishing directory durability after reopen and decoding persisted records are focused changes. I see no reason to add a generic storage framework, multi-process locking or compaction to close these findings. Full-file reads are acceptable within the explicitly deferred compaction scope.
Verification
Repository typecheck passes. All 337 repository tests and all 95 previous independent review probes pass on the rebased head. The new disk suite has 8 failing cases and 3 passing controls. Its TypeScript check also passes.
The controls verify multibyte records through forced short writes, no further writes after a zero-progress failure, and normal creation/reopen with a parent-directory sync. The failures use real temporary files, with fault injection limited to write sizes and fsync failures. This is not a physical power-loss certification. The retained test called “writes a durable line per append” checks readable bytes before close, which alone does not verify fsync behavior.
Review JSONReview textReproductionsFailure output95 prior probesBaseline output
Walkthrough
PR #4 implements the InteractionJournal port with a synchronous JSONL file. It keeps Node filesystem dependencies outside the kernel and relies on the already merged recovery reader.
- append serializes one record, completes short writes, fsyncs the file and syncs the parent directory on initial creation. Write or sync failures poison that handle.
- entries reads through to disk and returns a detached frozen array. Loading rejects interior parse holes and tolerates a damaged final fragment.
- close releases the descriptor. A later append reopens it. Fourteen retained tests cover basic files, corruption and runtime restart behavior.
Files changed
| File | Change |
|---|---|
| packages/journal-file/src/index.ts | +155 |
| packages/journal-file/src/file-journal.test.ts | +259 |
| packages/journal-file/package.json | +24 |
| packages/journal-file/tsconfig.json | +8 |
| pnpm-lock.yaml | +16 |
Issues
| ID | Priority | Finding |
|---|---|---|
| F57 | P1 | Repair the append boundary after tolerating a torn tail |
| F58 | P1 | Do not infer directory durability from file existence |
| F59 | P2 | Separate torn JSON from invalid journal records |
Persistence flow
Create/load -> validate JSONL -> append -> complete writes -> fsync file -> sync parent directory -> return. Recovery must also establish a safe append boundary.
Prompt for all review comments · Ctrl+`
Prompt to re-run review · Ctrl+Space
Review comments
P1 F58. Do not infer directory durability from file existencepackages/journal-file/src/index.ts:90-96logicdata-integrity
directorySynced is initialized from existsSync. That proves the name is currently visible, not that a previous creation synced its directory. A failed first append can leave a complete, file-synced journal behind before the directory sync succeeds.
J3 injects a failure only into the first parent-directory fsync. The initial append correctly throws and poisons its handle. After close and reopen, a second append returns success, but no parent-directory fsync is attempted because the path now exists. The reopened handle has forgotten the unfinished creation durability step.
File fsync does not establish durability of its directory entry. A later successful append therefore lacks the documented guarantee that the filename survives power loss. This probe establishes the missing syscall after a real-file failure sequence; it does not simulate a physical power loss.
Fix direction. Establish parent-directory durability on the first durable append of a newly opened journal even when its path already exists, or use a protocol that proves creation durability. An existence check alone cannot carry that proof across handles.
J3: one failing case
90: const existed = existsSync(path); 91: // Validate before opening: a directory, device or FIFO at this path must not 92: // become a blocking or redirecting append target. 93: parseEntries(path); 94: 95: let descriptor: number | undefined; 96: let directorySynced = existed;
File fsync does not guarantee directory-entry persistence; the parent directory needs its own sync. Linux fsync(2).
Prompt for AI agents
P2 F59. Separate torn JSON from invalid journal recordspackages/journal-file/src/index.ts:64-78logicdata-integrity
The catch that tolerates a torn final JSON fragment also catches the loader's own record-shape errors. A complete final null, number or unrelated object is silently discarded. Separately, an object with any string kind is cast to InteractionLogEntry without checking the required data for that entry.
J4 writes a valid record followed by each of null, 42 and an unrelated object, all newline-terminated. All three open successfully instead of reporting corruption. J5 places an effect-frozen object with no effect in the interior of the file; it is also accepted. For the matching interaction, the core recovery reader accesses entry.effect.id directly at journal-recovery.ts:172.
This contradicts the adapter's stated refusal of non-entry lines and moves malformed persisted data past the typed journal boundary. These probes deliberately corrupt files; they do not claim the current runtime normally emits those records. Keep syntax truncation tolerance distinct from record validation, and report a useful line-numbered corruption error.
Fix direction. Catch JSON syntax errors separately from entry decoding. Reject complete invalid records at any position and validate required fields of recognized entry kinds before exposing InteractionLogEntry values. Share a small decoder at the journal boundary if appropriate; avoid maintaining a second divergent journal schema.
J4, J5: four failing cases
64: try {
65: const parsed: unknown = JSON.parse(trimmed);
66: if (
67: typeof parsed !== "object" ||
68: parsed === null ||
69: Array.isArray(parsed) ||
70: typeof (parsed as { kind?: unknown }).kind !== "string"
71: ) {
72: throw new Error("entry is not an object with a string kind");
73: }
74: entries.push(parsed as InteractionLogEntry);
75: } catch (error) {
76: // A crash mid-append can tear the final line (with or without its
77: // newline). A damaged interior line is a hole and must be refused.
78: if (index === lastNonEmpty) continue;
P1 F57. Repair the append boundary after tolerating a torn tailpackages/journal-file/src/index.ts:93-96logicdata-integrity
The constructor validates the file with parseEntries, but discards the location of the tolerated final fragment. handle then opens the same bytes with O_APPEND. Ignoring a broken tail while reading does not make the file safe to append to.
J1 loads an old valid record followed by an incomplete JSON fragment, then appends a new record successfully. Without a trailing newline, the new record joins the fragment and disappears on the next read. With a trailing newline, the fragment becomes an interior hole and reopening rejects the entire journal. J2 starts with a complete final JSON record missing only its newline; the next append concatenates two JSON objects and both records disappear.
These are supported recovery inputs, not concurrent-writer scenarios. The current tests stop after reading the tolerated tail. A recovery adapter must preserve the next acknowledged append too. Otherwise a successful write-ahead append can be missing when the kernel next reconstructs state.
Fix direction. Retain the last safe byte boundary and normalize the tail before accepting another append, with the repair included in the durability boundary. Alternatively fail closed until an explicit repair completes. Preserve complete records and ensure a final complete record without a newline stays separate from the next record.
J1, J2: three failing cases
Prompt for AI agents