feat: [ENG-2497] Daemon bootstrap wiring + per-project store factory …

[bao-byterover]

No description provided.

[github-actions]

Claude finished @bao-byterover's task in 2m 21s —— View job

---

PR Review: feat [ENG-2497] Daemon bootstrap wiring + per-project store factory

• Gather context (read changed files)
• Analyze correctness, security, and code quality
• Post inline comments
• Post overall summary

---

Summary

Score: 4 / 5

Solid milestone delivery. The module-scope singleton cache is the right pattern for a single-instance daemon, the orphan-detection logic is correct, and the brv-server.ts wiring is minimal and clean. Tests cover the four key scenarios (happy path, orphan-index, orphan-data, memoization). No security issues found.

Four things worth addressing before merge:

---

Issues

task-history-store-cache.ts

Severity	Location	Finding
suggestion	L24 – testLoggerForGetStore	Module-scope mutable leaks test infrastructure into production code. Consolidate into a single _setTestOverrides or document as @internal.
issue	L39 – auditedProjects.add	Project is marked audited before the async audit settles, so a transient I/O failure permanently skips retries. Best-effort is fine; add a comment so the next reader doesn't treat it as a bug.
suggestion	L56 – log parameter default	undefined = undefined then ?? processLog is redundant. Default directly to processLog and drop effectiveLog.
nitpick	L21 – stores / auditedProjects	No eviction mechanism. Fine for daemon lifetime, but add a comment noting cleanup is deferred to avoid well-intentioned future PRs breaking the invariant.

task-history-startup-audit.test.ts

Severity	Location	Finding
issue	L171 – setTimeout(30)	Draining a fire-and-forget audit with a 30 ms wall-clock sleep is fragile under CI load. Replace with setImmediate-based flush or expose the audit promise for a handle.
nitpick	L71 – dynamic import()	Hoist to a static import at file top — module cache means it's identical, and it's cleaner.

---

What's working well

• Correctness of orphan detection — FILENAME_PATTERN strips the tsk- prefix consistently with the taskId field in TaskHistoryEntry, so set comparisons are correct.
• brv-server.ts diff — exactly two import lines + one constructor + one array entry; no unnecessary churn.
• Test setup / teardown — BRV_DATA_DIR is properly restored (including the undefined case), temp dirs are cleaned up, and resetTaskHistoryStoreCache is called in beforeEach to isolate state.
• Error handling — audit failures are caught and logged via processLog without propagating to the caller, which is exactly right for a best-effort background check.

[github-actions]

suggestion (code-smell): The testLoggerForGetStore module-scope mutable leaks test infrastructure into production code. A cleaner alternative is to make getStore accept an optional log override at call-site, or to export a factory that accepts the logger — but that changes the public API. At minimum, wrap the export in a /* @internal */ marker or consolidate the two test-seam exports into one _setTestOverrides({log}) object so the surface area is explicit.

[github-actions]

issue (reliability): The project is added to auditedProjects before the audit promise settles. If the audit fails (e.g. a transient I/O error), the project is permanently marked as audited and will never be retried — even after a daemon restart would normally re-trigger it. Since this is declared "best-effort" that's intentional, but it should be documented explicitly here (one line is enough) so future readers don't treat it as a bug.

[github-actions]

suggestion (simplification): The double-default for log (undefined = undefined → ?? processLog) is redundant. You can drop the explicit undefined default entirely and let callers pass undefined directly — or just default to processLog directly:

Suggested change

	export async function auditTaskHistory(
	export async function auditTaskHistory(
	projectPath: string,
	store: FileTaskHistoryStore,
	log: (msg: string) => void = processLog,
	): Promise<void> {
	const effectiveLog = log

This removes the intermediate effectiveLog alias too, reducing noise.

[github-actions]

nitpick (memory): stores and auditedProjects grow without bound for the lifetime of the daemon process. For a daemon that serves many short-lived projects this is fine; if a project is ever un-registered, the store stays cached. Worth a comment noting that eviction is intentionally deferred (M2.09 compaction, or a future cleanup pass) so readers don't add premature eviction logic.

[github-actions]

issue (reliability): setTimeout(30) to drain the fire-and-forget audit is fragile — on a loaded CI runner or a slow fs the 30 ms window can expire before the audit logs. Consider using a more deterministic flush, e.g. yielding with await new Promise(setImmediate) twice (one tick for the .catch continuation, one for readdir), or exposing a testable auditPromise from the first getStore call. Fix this →

[github-actions]

nitpick (test hygiene): The dynamic await import(...) inside beforeEach works because ES module specifiers are cached, but it's cleaner — and slightly faster — to hoist the import to the top of the file as a static import. The BRV_DATA_DIR env override takes effect at call time inside getProjectDataDir, not at module load time, so static import is safe here.