feat: [ENG-1888] Define IQueryLogStore interface

[bao-byterover]

No description provided.

[github-actions]

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

---

PR Review: feat: [ENG-1888] Define IQueryLogStore interface

• Gather context and read changed files
• Analyze diff and review code
• Post inline comments for specific issues
• Provide overall summary with score

---

Summary

This PR extends IQueryLogStore with getNextId() and save(), adds stub implementations in FileQueryLogStore, and updates the test factory to include the new stubs. The code is clean and the stub pattern is consistent with the existing style — but there are two design concerns worth addressing before merge.

---

Issues Found

1. getNextId() belongs on the store interface (design concern) — see inline

ID generation is a domain/use-case concern, not a storage contract. The store's responsibility is persisting and retrieving entries. Putting getNextId() on IQueryLogStore means callers must depend on the store just to mint an ID — and it prevents the use case from generating IDs independently (e.g. with Date.now(), which already matches the qry-{timestamp} format used throughout the tests). Consider making it an implementation detail of FileQueryLogStore called internally by save(), or removing it from the interface entirely.

2. save() / getNextId() stubs added without consuming tests (outside-in violation) — see inline

The makeStore() helper now includes stubs for both new methods, but no test in the file exercises them through QueryLogUseCase. Per the repo's Outside-In principle (CLAUDE.md), interface methods should be driven by a failing consumer test before appearing in the interface — not added speculatively ahead of the use case that needs them.

3. "Best-effort" JSDoc is misleading — see inline

save() returns Promise<void> which can still reject, making the "best-effort" description contradictory. Either the method should swallow errors internally (true best-effort), or the doc should say "callers should handle I/O errors."

---

Score: 2 / 5

The stub pattern and TypeScript are clean, but the two structural issues (ID generation on the store interface, interface methods without consumer tests) are direct violations of the repo's documented architecture principles and should be resolved before merge.

[github-actions]

concern (design): getNextId() on the storage interface conflates ID generation with persistence. The store's contract should be limited to reading/writing entries; ID generation is a domain/use-case concern.

If IDs must be monotonic relative to persisted data (e.g. derived from a sequence counter in the log directory), that could justify it — but even then, a separate IIdGenerator or a factory function scoped to the store would keep the interface focused. As-is, callers cannot generate an ID without touching the store.

Could this be pushed down to the FileQueryLogStore implementation (a non-interface method) and called internally by save() when entry.id is absent? Or generated by the use case with Date.now() since existing IDs already look like qry-{timestamp}?

Fix this →

[github-actions]

nitpick: "Best-effort — callers should handle errors" is an unusual contract for a Promise<void>. If errors can be thrown, that's normal async behavior, not "best-effort." True best-effort would mean the method swallows errors internally and never rejects.

Consider either: (a) removing "Best-effort" and just saying "Callers should handle I/O errors." or (b) having the method return Promise<{ok: boolean}> or catch-and-log internally if truly best-effort is the intent.

[github-actions]

issue (outside-in): getNextId and save stubs are wired into the factory but no test in this file calls them or asserts on them. Per the repo's Outside-In principle, interface methods should be driven by a concrete consumer test before they appear in the interface.

If QueryLogUseCase doesn't yet call save() or getNextId(), these stubs are forward-looking placeholders rather than tests that validate current behavior — which is an Inside-Out smell. The stubs should be added (and the interface methods should be added) once the use case tests that exercise them exist.

Fix this →