Feat/eng 2515

[bao-byterover]

No description provided.

[github-actions]

Claude finished @bao-byterover's task in 3m 29s —— View job

---

PR Review: Feat/ENG-2515

• Gather context and read changed files
• Review task-history-entry.ts and task-events.ts
• Review file-task-history-store.ts (core logic)
• Review task-router.ts
• Review test files
• Post final review summary

---

Overall Score: 4 / 5

Good, well-documented work. The three concurrent-access fixes (Category B cache-invalidation race, tombstone chunking, and flushPendingOperations) are technically sound and backed by focused regression tests. The TaskHistoryEntry migration to shared/ is the right move and is guarded by a static layer-isolation test. Minor issues noted below.

---

What's Good

suggestion (non-blocking): The epoch-based cache invalidation in doReadIndexDedup (indexEpoch) is an elegant and correct solution to the Category B race. Sampling the epoch before I/O and refusing to cache if it advanced during the pass is the canonical generation-counter pattern for this class of problem.

suggestion (non-blocking): chunkLinesByBytes is a clean pure function, easy to test in isolation, and the "oversized line emitted as its own chunk" behaviour is both correct and documented. The unit test covering 200 realistic tombstone lines gives real confidence against regression.

suggestion (non-blocking): Moving TaskHistoryEntry / TaskHistoryStatus / ToolCallEvent etc. into shared/transport/events/task-events.ts is the right layer placement. The layer-isolation.test.ts enforces the invariant statically without needing CI to exercise a runtime import.

---

Issues

issue — flushPendingOperations() is test-only but part of the public API (src/server/infra/storage/file-task-history-store.ts:301)

The doc comment says "Test-only contract: production daemon never calls this", but the method is async flushPendingOperations() on the exported class. Any production consumer of ITaskHistoryStore could accidentally call it. TypeScript has no @internal access modifier, but marking it with a /** @internal */ JSDoc annotation and omitting it from the ITaskHistoryStore interface would make the contract clear.

Fix this →

---

issue — sweepOrphanData can delete a data file for a save() that races between recoverPreRenameSaves's readdir and sweepOrphanData's readdir (src/server/infra/storage/file-task-history-store.ts:637-640)

Sequence:

1. maybeCompact() rewrites the index; post-rewrite re-read populates postRewriteLiveIds.
2. recoverPreRenameSaves calls readdir — misses a concurrent save() that writes its data file after this point.
3. save() appends its index line.
4. sweepOrphanData calls its own readdir, finds the new data file, does not see its taskId in postRewriteLiveIds, and deletes it.
5. The index now has a live entry whose data file is gone → getById returns undefined while list() returns the row.

This is a pre-existing window (not introduced here), but the C1 fix closes the pre-rename half of the race without closing this post-recover-readdir half. The simplest mitigation is to re-read the index once more inside sweepOrphanData before issuing unlinks, or to make sweepOrphanData accept the tombstone set and skip taskIds that have an index line.

Fix this →

---

nitpick — flushPendingOperations() spin-loop has no iteration ceiling (src/server/infra/storage/file-task-history-store.ts:301-311)

let last = this.pruneChain
await last
while (last !== this.pruneChain) {
  last = this.pruneChain
  await last
}

In a test that fires saves rapidly and continuously, this loop could iterate for a very long time. The documented invariant (at most one re-queued pass via pruneRequested) bounds it to two passes in normal operation, but there's no guard. A max-iteration count or a timeout would make misuse more obvious.

---

nitpick — IndexTombstoneSchema optional schemaVersion accepts any future schema version (src/server/infra/storage/file-task-history-store.ts:82-87)

schemaVersion: z.literal(TASK_HISTORY_SCHEMA_VERSION).optional(),

z.literal(1).optional() means a tombstone with schemaVersion: 2 (from a future schema bump) would fail to parse (literal mismatch). The intent is back-compat with bare {_deleted, taskId} lines (no schemaVersion key), but if the schema version is ever bumped you'll also need to update this schema or tombstones from the new version become invisible. A comment noting this is worth adding; the current doc comment only mentions the back-compat direction.

---

nitpick — toListItem in task-router.ts treats task.files inconsistently with task.folderPath (src/server/infra/process/task-router.ts:196-211)

...(task.files && task.files.length > 0 ? {files: task.files} : {}),
...(task.folderPath ? {folderPath: task.folderPath} : {}),

files requires non-empty length, but folderPath only requires truthiness. An empty-array files is silently dropped. The FileTaskHistoryStore's IndexDataLineSchema accepts files: z.array(z.string()).optional() without a length guard, so a round-trip through the store would restore an empty array, creating a subtle inconsistency between in-memory and persisted list items. This is pre-existing but worth noting.

---

Test Observations

praise — The file-task-history-store-cache-race.test.ts correctly uses 30 × 10 iterations to expose the ~10–20% per-cycle flake rate. The makeEntry factory with a staleThresholdMs: Number.POSITIVE_INFINITY guard is exactly right.

praise — layer-isolation.test.ts using readFileSync + regex on the actual .ts source is a lightweight and reliable static-analysis approach. Walking the entire src/shared/ tree means new files are automatically covered.

praise — The tombstone-chunking test suite covers all three failure modes: correct deletion count, JSON integrity, and interleaved-save parsability.

---