Proj/query log

[bao-byterover]

Summary

• Problem: Users have zero visibility into how their curated knowledge gets used via the query/recall system. No centralized query log exists — no timestamps, no response times, no query text, no tier information.
• Why it matters: Without recall visibility, users can't prove ROI, detect knowledge gaps, or understand cache effectiveness. Curation value is half the answer — recall completes it.
• What changed:
  • ms1: brv query-log view command — list + detail views with tier/status/time filters, JSON output
  • ms2: brv query-log summary command — aggregate metrics (cache hit rate, coverage, p50/p95 response time, top topics, knowledge gaps) in text/JSON/narrative formats
  • ms3: QueryExecutor instrumentation — every query execution now automatically produces a qry-*.json log entry with tier, timing, matched docs, and search metadata via QueryLogHandler lifecycle hook
• What did NOT change (scope boundary): Query execution behavior unchanged. TUI display unchanged. MCP tools unchanged. CurateLogHandler unchanged. brv-transport-client unchanged.

Type of change

• Bug fix
• New feature
• Refactor (no behavior change)
• Documentation
• Test
• Chore (build, dependencies, CI)

Scope (select all touched areas)

• TUI / REPL
• Agent / Tools
• LLM Providers
• Server / Daemon
• Shared (constants, types, transport events)
• CLI Commands (oclif)
• Hub / Connectors
• Cloud Sync
• CI/CD / Infra

Linked issues

• Closes ENG-1887, ENG-1888, ENG-1889, ENG-1890, ENG-1895, ENG-1896, ENG-1897 (ms1)
• Closes ENG-1898, ENG-1899 (ms2)
• Closes ENG-1892, ENG-1893, ENG-1894 (ms3)
• Related ENG-1900, ENG-1901 (ms4 — not in scope)

Root cause (bug fixes only, otherwise write N/A)

N/A

Test plan

• Coverage added:
  • Unit test
  • Integration test
  • Manual verification only
• Test file(s):
  • test/unit/core/domain/entities/query-log-entry.test.ts (15 tests)
  • test/integration/infra/storage/file-query-log-store.test.ts (21 tests)
  • test/unit/infra/usecase/query-log-use-case.test.ts (19 tests)
  • test/unit/infra/usecase/query-log-summary-use-case.test.ts (30 tests)
  • test/commands/query-log/view.test.ts (16 tests)
  • test/commands/query-log/summary.test.ts (20 tests)
  • test/unit/infra/executor/query-executor.test.ts (11 tests)
  • test/unit/infra/process/query-log-handler.test.ts (13 tests)
• Key scenario(s) covered:
  • All 5 tiers (T0 exact cache, T1 fuzzy cache, T2 direct search/OOD, T3 optimized LLM, T4 full agentic)
  • All entry statuses (completed, error, cancelled, processing)
  • All flag combinations for view (--status, --tier, --since, --before, --limit, --detail, --format)
  • All flag combinations for summary (--last, --since, --before, --format text/json/narrative)
  • Cross-format consistency (text header period ↔ narrative period label)
  • Graceful degradation when query metadata missing
  • Memory-first save pattern, store eviction, concurrent task handling
  • E2E: 59 command cases verified against live daemon on testzone

User-visible changes

• New command: brv query-log view [id] — view query history with filters
• New command: brv query-log summary — aggregate recall metrics with text/JSON/narrative output
• Query executions now automatically produce log entries (no user action required)
• Summary header shows time period: Query Recall Summary (last 24h)
• Narrative format provides human-readable value story for daily reports
• Knowledge gaps section shows unanswered topics with → Run 'brv curate' hint

Evidence

• 5709 unit tests passing, 0 failing
• 59 E2E command cases verified (all pass)
• Typecheck clean, lint clean, build clean
• Cross-format comparison: text ↔ narrative ↔ JSON period labels consistent
• CurateLogHandler regression verified: curate commands still produce log entries
• Test data preserved at /Users/kakalot/byterover/testzone/test-query-log for manual verification

Checklist

• Tests added or updated and passing (npm test)
• Lint passes (npm run lint)
• Type check passes (npm run typecheck)
• Build succeeds (npm run build)
• Commits follow Conventional Commits format
• Documentation updated (if applicable)
• No breaking changes (or clearly documented above)
• Branch is up to date with main

Risks and mitigations

• Risk: ms3 adds a new transport event (task:queryResult) — if Socket.IO ordering fails, metadata arrives after onTaskCompleted.
  • Mitigation: QueryLogHandler gracefully degrades (tier/timing = undefined). Tested in unit test case 8.
• Risk: describePeriod shared function used by both text and narrative formatters — change in one affects both.
  • Mitigation: Single source of truth by design. 30 summary use case tests + 59 E2E cases verify consistency.

[github-actions]

Claude finished @leehpham's task in 5m 9s —— View job

---

PR Review: Proj/query log

Tasks:

• Gather context and understand the PR
• Review core domain entities and interfaces
• Review storage implementation
• Review use cases and formatters
• Review executor instrumentation
• Review process handler
• Review CLI commands
• Review test quality
• Post inline comments
• Post final summary

---

Overall Score: 4 / 5

A well-architected, comprehensively tested feature. The layering (entity → interface → infra → oclif) is clean, the SSOT pattern for tier/status constants is well-executed, and the atomic-write + path-traversal-prevention in FileQueryLogStore shows production-level care. The main issues are a few correctness/consistency nits in the analytics layer and one redundant transport field.

---

What works well

• SSOT for tier/status arrays: Deriving QueryLogTier, ByTier, and the Zod schema all from QUERY_LOG_TIERS as const means a tier add/remove is a one-line change. emptyByTier() is a clean companion.
• Memory-first pattern in QueryLogHandler: Setting this.tasks before the async store.save() (line 133) ensures setQueryResult() can safely arrive on the same tick without racing the disk write.
• Atomic writes + path traversal prevention: FileQueryLogStore validates IDs against ID_PATTERN before constructing paths and uses tmpPath → rename for all writes. Security-correct by design.
• Stale-processing recovery: Silently rewriting stuck processing entries as error on read (with writeAtomic to skip the prune cascade) is a thoughtful daemon-crash recovery mechanism.
• task:queryResult ordering contract: Placing setQueryResult() as a synchronous setter with a clear ordering comment in brv-server.ts is the right design — no async hop needed.
• Test coverage: 145 tests across 8 files, covering all tier/status/format combinations and the graceful-degradation path. The test helpers (makeTask, makeStore, makeQueryResult) follow project conventions.

---

Issues

1. response field in TaskQueryResultEventSchema is dead payload (src/server/core/domain/transport/schemas.ts:562)
The JSDoc says "Response string is NOT included" but it IS in the schema and IS sent. QueryLogHandler never reads queryResult.response — onTaskCompleted uses the result param from task:completed instead. Every query response is currently sent twice over the socket. Inline comment posted.

2. p50Ms/p95Ms percentile formula off for small arrays (src/server/infra/usecase/query-log-summary-use-case.ts:175)
sorted[Math.floor(n * 0.5)] returns the max value (index 1) for n=2, overestimating p50. Standard nearest-rank is sorted[Math.ceil(n * pct) - 1]. Inline comment posted with table.

3. formatEntryDuration silently drops timing for error/cancelled entries (src/server/utils/log-format-utils.ts:10)
Entries with status 'error' or 'cancelled' show — in both list and detail views even when timing.durationMs is present. Also creates a two-metric inconsistency: the list view measures completedAt - startedAt (lifecycle time) while summary uses entry.timing.durationMs (executor time). Inline comment posted with suggestion.

4. describePeriod hour boundary off by one (src/server/infra/usecase/query-log-summary-narrative-formatter.ts:50)
hours <= 25 labels a 25-hour window as "last 24h". Should be hours <= 24. Inline comment posted.

5. Project root resolution inconsistency (src/oclif/commands/query-log/view.ts:80)
query-log/view.ts uses findProjectRoot (async, transport-client) while curate/view.ts uses resolveProject() (sync, canonical brv resolver). In a worktree, resolveProject() follows the .brv pointer file to the true project root; findProjectRoot may resolve differently. Inline comment posted.

---

Minor nits (no inline comment)

• describePeriod with no time filter returns 'in the selected period' for the long format, producing the grammatically awkward sentence "No queries recorded in the selected period." A plain 'yet' would read better in that case.
• The collectGapKeywords approach tokenizes individual words; short compound queries (e.g., "deploy auth flow") become three separate topic buckets. Fine for v1 but worth flagging as a known limitation.
• ITerminal is a one-method interface (log). Fine now, but if stderr output is ever needed this will need error()/warn(). Not blocking.

[leehpham]

@bao-byterover double check where null is handled. Convert to undefined.

[github-actions]

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

---

PR Review Update: Proj/query log

Tasks:

• Gather context from previous review and new commits
• Check what changed in the "fix review" commit (5d28c4b)
• Audit null → undefined handling (per @leehpham's comment)
• Verify previous review issues were addressed
• Post updated review summary

---

All 5 previous review issues — resolved ✓

The feat: [proj/query-log] fix review commit (5d28c4b) cleanly addresses every issue raised in the prior review:

#	Issue	Status
1	response field in TaskQueryResultEventSchema was dead payload sent twice	✅ Removed from schema (schemas.ts:562) and from the emit in agent-process.ts. QueryLogHandler now uses QueryResultMetadata = Omit<QueryExecutorResult, 'response'>
2	p50Ms/p95Ms percentile formula off for small arrays	✅ Fixed: Math.floor(n * pct) → Math.ceil(n * pct) - 1 in query-log-summary-use-case.ts:174-175. Test expectations updated accordingly (p50: 300→200, p95: 2000→1900)
3	formatEntryDuration silently dropped timing for error/cancelled entries	✅ Fixed: now shows entry.timing.durationMs when present, falls back to lifecycle time (log-format-utils.ts:11-12). Tests updated from expecting '—' to '300ms'/'1.0s'
4	describePeriod hour boundary off-by-one (<= 25 should be <= 24)	✅ Fixed in both format === 'short' and long paths (narrative-formatter.ts:50,55)
5	query-log/view.ts used findProjectRoot (transport-client) instead of resolveProject()	✅ Fixed in both view.ts and summary.ts — both now use resolveProject()?.projectRoot ?? process.cwd()

---

null → undefined audit (per @leehpham)

All internal null usages have been converted:

• IQueryLogStore.getById now returns Promise<QueryLogEntry | undefined> — interface and implementation aligned
• FileQueryLogStore.getById returns undefined on invalid ID, corrupt file, Zod failure, and missing file
• All 16 test assertions updated (to.be.null → to.be.undefined, to.not.be.null → to.not.be.undefined)
• query-executor.ts: loadSources(...) fallback changed from null to undefined

Remaining null occurrences are intentional and acceptable:

• JSON.stringify(entry, null, 2) — standard Node.js API (the replacer parameter), cannot be undefined
• error === null type guards in test helpers — correct JavaScript idiom for narrowing object | null

---

Overall assessment

The fix commit is clean and complete. No new issues introduced. The codebase is now consistent with the project's undefined-over-null convention throughout all query-log modules.

Score: 5 / 5 — all review issues addressed, null convention enforced, tests updated to match corrected behavior.

[leehpham]

Looks good

[github-actions]

Claude finished @bao-byterover's task in 4m 18s —— View job

---

PR Review: Proj/query log

Tasks:

• Gather context from previous reviews and PR state
• Read key implementation files
• Review domain entities and interfaces
• Review storage and use cases
• Review CLI commands and process handler
• Review test quality
• Post inline comments
• Post final summary

---

Overall Score: 5 / 5

All 5 issues from the prior review were fixed cleanly in 5d28c4b. The null → undefined audit is thorough. The architecture is sound, the SSOT pattern is well-executed, and tests are comprehensive. Three minor nits below — none are blockers.

---

Prior Review Issues — All Resolved ✓

#	Issue	Status
1	response field in TaskQueryResultEventSchema was dead payload	✅ Removed — schema and emit both cleaned up
2	p50Ms/p95Ms percentile formula off	✅ Fixed: Math.ceil(n * pct) - 1 (lines 174–175)
3	formatEntryDuration dropped timing for error/cancelled entries	✅ Fixed: uses entry.timing.durationMs when present
4	describePeriod hour boundary <= 25 off-by-one	✅ Fixed to <= 24
5	view.ts used findProjectRoot instead of resolveProject()	✅ Both view.ts and summary.ts now use resolveProject()

---

New Findings (minor nits)

1. getNextId() is declared async unnecessarily
FileQueryLogStore.getNextId() (line 128) only calls Date.now() — no await, no I/O. Declaring it async causes every query start to allocate a microtask-tick Promise.resolve() for no reason. The interface (IQueryLogStore.getNextId(): Promise<string>) requires a Promise return type, but the implementation can return Promise.resolve(...) directly without async.

2. Stopword list is defined twice
STOPWORDS in query-log-summary-use-case.ts (line 26–65) and the inline stopwords set in query-executor.ts (line 527–558) serve similar purposes but are different lists — the executor has 'a', 'an', 'in', 'is', 'of' that the summary doesn't, and the summary has 'can', 'get', 'has', 'have', 'into', 'set', 'use' that the executor doesn't. Over time these will drift further. A shared src/server/utils/stopwords.ts would prevent that.

3. describePeriod short-format fallback produces awkward header
When only --before is given (e.g., brv query-log summary --before 2024-04-01), period.from = 0 so the relative-window branch is skipped, and the short format returns 'selected period'. The resulting header is Query Recall Summary (selected period) — the parenthetical sounds like user-defined naming rather than a time label. A more useful fallback would be 'all time' (when neither bound is set) or just omitting the parenthetical entirely, as formatSummaryText already handles the no-period case with if (periodLabel) ....

---

What works well

• Tier-based SSOT — QUERY_LOG_TIERS as const drives the TypeScript type, ByTier record, Zod schema, and empty-initializer all from one declaration. Adding a tier is a one-line change.
• Memory-first pattern in QueryLogHandler.onTaskCreate (line 136) — sets this.tasks before the async store.save() so setQueryResult() can safely merge on the same tick.
• Atomic writes + path traversal prevention in FileQueryLogStore — ID_PATTERN validation before path construction, tmp → rename for all writes. Production-grade.
• Stale-processing recovery — silently rewriting interrupted processing entries as error on read with a dedicated writeAtomic call (skipping the prune cascade) is a clean daemon-crash recovery pattern.
• Graceful degradation — if task:queryResult fails to arrive (socket drop before setQueryResult), the entry logs with tier: undefined and timing: undefined rather than failing. Tested in unit test case 8.
• test coverage — 145 tests across 8 files, all tier/status/format combos covered. Test helpers makeTask, makeStore, makeQueryResult follow project conventions.

---

[github-actions]

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

---

PR Review: Proj/query log

Tasks:

• Gather context and understand the PR
• Review recent changes since last review
• Check nits from previous review were addressed
• Review key implementation files
• Post inline comments for new issues
• Post final summary

---

Overall Score: 5 / 5

The ENG-2123 commit (40d5527) is a clean, intentional UX improvement: detail views for both brv curate view and brv query-log view now show full content line-by-line instead of silently truncating at 200/500 chars. Tests are properly updated to verify the new behavior (FULL context/response (no truncation) cases added). No regressions introduced.

---

ENG-2123 Review (latest commit)

What changed:

• curate-log-use-case.ts: Context display switched from slice(0, 200)... to line-by-line; response from slice(0, 500)... to multi-line
• query-log-use-case.ts: Response switched from truncate(response, 500) to multi-line; section header renamed "Response (truncated):" → "Response:"
• 4 new test cases across 2 files verify the behavior

This is correct. Detail view should show full content — the list view still truncates query text at 40 chars for the table column.

---

New Findings (minor nits only)

1. Inconsistent multi-line approach within curate-log-use-case.ts (src/server/infra/usecase/curate-log-use-case.ts)
Context (lines 113–117) uses N separate this.log() calls; response (line 141) uses one this.log() with embedded newlines. The sibling query-log-use-case.ts uses the single-call approach for response. If ITerminal.log ever prefixes per-call (color, timestamp), the two fields will behave differently. Inline comment posted.

2. Context continuation-line indent (curate-log-use-case.ts:116)
Continuation lines indent to col 2 while Context: content starts at col 11 — makes them look detached from the label. Tests pin this format, so flagging as a known v1 limitation. Inline comment posted.

---

Carry-Over Nits (still unaddressed from 2026-04-15 review)

#	Nit	Location
1	getNextId() declared async unnecessarily	file-query-log-store.ts:128
2	STOPWORDS defined in two places with different word sets	query-log-summary-use-case.ts:26, query-executor.ts:527
3	describePeriod short-format fallback 'selected period' produces awkward header	query-log-summary-narrative-formatter.ts:59

Inline comment re-posted for nit #1. Nits #2 and #3 remain low priority.

---

What works well

• Intentional removal of character truncation — full content in detail views is the right call. List views still protect column widths with truncate().
• Test coverage for the new behavior — "FULL context/response (no truncation)" test cases correctly lock in the new contract.
• truncate import in query-log-use-case.ts still needed — still used for list view query column truncation at line 167; no dead import.

[github-actions]

Claude finished @bao-byterover's task in 4m 26s —— View job

---

PR Review: Proj/query log

Tasks:

• Gather context and understand the PR
• Review recent changes since last review (ENG-2177)
• Check carry-over nits from previous reviews
• Review key implementation files
• Post inline comments for new issues
• Post final summary

---

Overall Score: 4.5 / 5

The ENG-2177 commit is the only new change since the last review. The core change — increasing storage limits from 100/200 to 1000 — is correct and well-tested. The three-phase test design (at old limit, at new limit, past new limit) is thorough. One blocking inconsistency surfaced; the carry-over async getNextId() nit remains open.

---

ENG-2177 Review (latest commit)

What changed:

• FileCurateLogStore.DEFAULT_MAX_ENTRIES: 100 → 1000
• FileQueryLogStore.DEFAULT_MAX_ENTRIES: 200 → 1000
• JSDoc updated to reflect new count
• New integration test (query log store, Test 21) — three-phase boundary verification
• New unit test (curate log store) — same three-phase pattern

This change is correct. The new tests properly exercise all three boundary cases and correctly use Date.now() - 500_000 for the query log test to avoid age-based pruning. The curate store test correctly uses a fixed historical timestamp since curate log has no age-based pruning.

---

Issues

1. FileCurateLogStore.getById still returns null — null audit incomplete (src/server/infra/storage/file-curate-log-store.ts:121, test/unit/infra/storage/file-curate-log-store.test.ts:431)

The prior null-audit commit (5d28c4b) converted FileQueryLogStore.getById to return undefined but missed FileCurateLogStore. Both are internal modules — CLAUDE.md is explicit: "reserve null only for external boundaries."

The new ENG-2177 test adds .to.be.null / .not.be.null assertions that are consistent with the current implementation, but they reinforce the inconsistency between the two parallel stores. The fix requires updating ICurateLogStore, FileCurateLogStore, and all test assertions together. Inline comment posted.

---

Carry-Over Nits (still unaddressed)

#	Nit	Location
1	getNextId() declared async unnecessarily (no await)	file-query-log-store.ts:128 (re-posted inline)
2	STOPWORDS defined in two places with diverging word sets	query-log-summary-use-case.ts:26, query-executor.ts:527
3	describePeriod short-format fallback 'selected period' produces awkward header	query-log-summary-narrative-formatter.ts:59

---

What works well (ENG-2177)

• Three-phase test structure verifies old limit doesn't prune, new boundary doesn't prune, and one-over triggers pruning — the right regression protection for a limit change.
• Timestamp strategy: query log tests use Date.now() - 500_000 to stay within the 30-day age window; curate log tests use a fixed historical timestamp (safe since curate has no age-based pruning). Both are correct.
• 30s timeout is appropriate given ~1000 sequential file writes per test phase.
• pruneInFlight guard in FileQueryLogStore prevents concurrent prune passes (carry-over, still correct).

[github-actions]

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

---

PR Review: Proj/query log

Tasks:

• Gather context from previous reviews and PR state
• Check recent commits since last review
• Verify blocking null audit issue (FileCurateLogStore) was addressed
• Review new changes for correctness, security, code quality
• Check carry-over nits from previous reviews
• Post inline comments for new issues
• Post final summary

---

Overall Score: 4.5 / 5

The only new commit since the last review is 85c6475 — a merge from main bringing in the env-var consolidation refactor (Refactor/env var consolidation 4 #397). The query-log feature code itself is unchanged. The merge is clean, no new issues introduced.

One blocking issue from the previous review remains open: the null → undefined audit is still incomplete for FileCurateLogStore.

---

Merge from main (85c6475) — Clean ✓

What came in:

• BRV_API_BASE_URL split into BRV_IAM_BASE_URL, BRV_COGIT_BASE_URL, BRV_LLM_BASE_URL
• New API_V1_PATH = '/api/v1' constant in src/server/constants.ts (appended at point of use, not baked into base URL env vars)
• assertRootDomain() validation prevents paths in base URL env vars
• normalizeUrl() strips trailing slashes from all env values
• Shared cwdField extracted to src/server/infra/mcp/tools/shared-schema.ts with comprehensive LLM-facing instructions
• MCP server now has an instructions field

No issues for the query-log feature from this merge. The agent-process.ts change from envConfig.llmApiBaseUrl → envConfig.llmBaseUrl is aligned with the refactor.

---

Blocking Issue — Still Open

FileCurateLogStore.getById null audit incomplete (src/server/infra/storage/file-curate-log-store.ts:121, src/server/core/interfaces/storage/i-curate-log-store.ts:12)

The 5d28c4b null-audit commit converted FileQueryLogStore.getById to return undefined but left FileCurateLogStore.getById returning null. The new ENG-2177 test adds .to.be.null / .not.be.null assertions (lines 407, 420, 431–433) that reinforce the inconsistency. Inline comment posted on the test file.

Full fix: update ICurateLogStore, FileCurateLogStore, and all test assertions together.

---

Carry-Over Nits (still unaddressed)

#	Nit	Location	Rounds open
1	getNextId() declared async unnecessarily	file-query-log-store.ts:128, file-curate-log-store.ts:142	4th
2	STOPWORDS defined twice with diverging word sets	query-log-summary-use-case.ts:26, query-executor.ts:527	2nd
3	describePeriod short-format fallback 'selected period' produces awkward header	query-log-summary-narrative-formatter.ts:59	2nd

Inline comment re-posted for nit #1.

---

What works well

All prior review issues remain resolved. The architecture, SSOT pattern, memory-first safety, atomic writes, and test coverage are all solid — no regressions from the merge.

[github-actions]

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

---

PR Review: Proj/query log

Tasks:

• Gather context from previous reviews and PR state
• Check recent commits since last review
• Verify blocking null audit issue (FileCurateLogStore) was addressed
• Check carry-over nits from previous reviews
• Review new changes for correctness, security, code quality
• Post inline comments for new issues
• Post final summary

---

Overall Score: 4.5 / 5

The only new commit since the last review is 441e6c0 — a merge from main bringing in the dreaming feature (proj/dreaming #436). The query-log feature code is unchanged. The merge is clean. One blocking issue remains open.

---

Merge from main (441e6c0) — Clean ✓

What came in:

• Full brv dream command and dream subsystem: DreamTrigger, DreamLockService, DreamStateService, DreamLogStore, DreamExecutor, undo/prune/synthesize/consolidate operations
• New preDispatchCheck hook in TransportHandlers — daemon-side gate for dream eligibility (gates 1–3). Correctly returns {eligible: true} for all non-dream task types, so query, curate, search tasks are unaffected.
• handleTaskSkippedByPreCheck in task-router.ts — sends task:completed with skip reason but does not call lifecycle hooks. This is correct: a skipped dream task never reaches QueryLogHandler.onTaskCompleted.
• New logId and projectPath fields on TaskCompletedEventSchema — additive, backward-compatible. task-router.ts now prefers hook-provided logId (curate) over executor-provided (dream), consistent with SSOT.
• idle dream dispatch: agentPool.submitTask({trigger: 'agent-idle', ...}) bypasses task-router, reaches agent-process with trigger set. CLI dreams use trigger ?? 'cli' fallback in agent-process.ts. Correct.

Conflict resolutions verified:

• constants.ts: QUERY_LOG_DIR / QUERY_LOG_ID_PREFIX preserved; DREAM_LOG_DIR / DREAM_LOG_ID_PREFIX appended cleanly.
• brv-server.ts: lifecycleHooks: [curateLogHandler, queryLogHandler] preserved; task:queryResult handler intact at line 376; dream pre-check factory and idle timeout changes are additive.

No issues introduced by this merge to the query-log feature.

---

Blocking Issue — Still Open (3rd round)

FileCurateLogStore.getById null audit incomplete (src/server/infra/storage/file-curate-log-store.ts:121, src/server/core/interfaces/storage/i-curate-log-store.ts:12)

FileCurateLogStore.getById still returns CurateLogEntry | null while the parallel FileQueryLogStore.getById was converted to CurateLogEntry | undefined in 5d28c4b. Three full review rounds have flagged this — it remains the only blocker before merge.

Full fix:

1. ICurateLogStore.getById → Promise<CurateLogEntry | undefined>
2. FileCurateLogStore.getById → return undefined on all failure paths
3. All .to.be.null / .not.be.null in test/unit/infra/storage/file-curate-log-store.test.ts → .to.be.undefined / .not.be.undefined

Fix this →

---

Carry-Over Nits (still unaddressed)

#	Nit	Location	Rounds open
1	getNextId() declared async unnecessarily (no await)	file-query-log-store.ts:128, file-curate-log-store.ts:142	5th
2	STOPWORDS defined in two places with diverging word sets	query-log-summary-use-case.ts:26, query-executor.ts:527	3rd
3	describePeriod short-format fallback 'selected period' produces awkward header	query-log-summary-narrative-formatter.ts:59	3rd

---

What works well

All prior review issues remain resolved. The architecture, SSOT pattern, memory-first safety, atomic writes, and test coverage are solid. The dreaming merge was handled cleanly — conflicts resolved correctly with no regressions to query-log behavior.