feat: [ENG-2495] FileTaskHistoryStore: Delete + Clear API

[bao-byterover]

No description provided.

[github-actions]

Claude finished @bao-byterover's task in 1m 57s —— View job

---

PR Review: feat: [ENG-2495] FileTaskHistoryStore: Delete + Clear API

Tasks

• Gather context and read changed files
• Analyze interface changes (i-task-history-store.ts)
• Analyze implementation changes (file-task-history-store.ts)
• Analyze test changes (file-task-history-store.test.ts)
• Provide overall review summary with score

---

Summary

Score: 4 / 5

Well-structured implementation that follows the existing two-tier index pattern cleanly. The tombstone-first + unlink design is sound, TDD is strictly followed with excellent test coverage for the new surface area, and the path-traversal guard is in the right place. A few correctness/consistency issues below.

---

Issues

suggestion — getById doesn't honour tombstones

file-task-history-store.ts — getById method

getById reads directly from the data file without ever consulting the index/tombstone state. The comment inside tombstoneAndUnlink states:

"Tombstone first so list/getById skip the entry even if the unlink fails"

But getById has no tombstone check — it would return a live entry if rm fails (e.g. permission error). The test getById after delete returns undefined passes only because rm succeeds in the test environment, not because the tombstone is respected.

Fix: check the index before reading the data file:

async getById(taskId: string): Promise<TaskHistoryEntry | undefined> {
  if (!TASK_ID_PATTERN.test(taskId)) return undefined
  const dedup = await this.readIndexDedup()
  const line = dedup.get(taskId)
  if (line === undefined || '_deleted' in line) return undefined
  // ...existing readFile logic
}

Fix this →

---

issue — deleteMany doesn't deduplicate input

file-task-history-store.ts — deleteMany

const live = valid.filter((id) => {
  const line = dedup.get(id)
  return line !== undefined && !('_deleted' in line)
})

If taskIds = ['a', 'a', 'a'] and 'a' is live, live is ['a', 'a', 'a']. This causes:

• 3 tombstone lines appended for the same ID (wasteful but functionally harmless)
• Return value 3 rather than 1 (misleading — interface says "count of taskIds that were live")

Add dedup before filtering:

const unique = [...new Set(valid)]
const live = unique.filter(...)

---

nitpick — misleading error-swallow in tombstoneAndUnlink

await Promise.all(
  taskIds.map((id) => rm(this.dataPath(id), {force: true}).catch(() => {})),
)

{force: true} already suppresses ENOENT. The outer .catch(() => {}) silently swallows all other errors (permissions, I/O failures). This is intentional for the race-condition test, but it makes genuine errors invisible. A comment explaining the intentional double-suppression would help future readers.

---

nitpick — atomicity bound not enforced

The comment in tombstoneAndUnlink accurately notes the ~50-id PIPE_BUF limit. Consider a process.emitWarning or at minimum a console.warn if taskIds.length > 50, so callers know when they're outside the safe window before M2.03 lands.

---

What's good

• Outside-in design: interface defines exactly what consumers need, implementation fulfills the contract cleanly.
• Tombstone-first ordering (appendFile before rm) is the correct failure mode — orphan data files are acceptable, dangling index references are not.
• Path traversal guard (TASK_ID_PATTERN) is applied consistently at every entry point. clear() is safe because it only uses taskIds already read from the validated index.
• Cache invalidation is handled correctly in all three mutating paths (via tombstoneAndUnlink → this.indexCache = undefined).
• Test suite is comprehensive: idempotency, races, filters, projectPath scoping, tombstone visibility via list + getById. TDD process clearly followed.
• DEFAULT_TERMINAL_STATUSES defaulting correctly preserves active tasks on plain clear().