feat: [ENG-1887] Define QueryLogEntry entity with discriminated union

[bao-byterover]

No description provided.

[github-actions]

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

---

PR Review: feat: [ENG-1887] Define QueryLogEntry entity with discriminated union

• Gather context and read changed files
• Review entity definition and discriminated union design
• Review use-case changes
• Review test coverage and TDD compliance
• Post final review with score

---

Summary

This PR refactors QueryLogEntry to move matchedDocs, searchMetadata, taskId, tier (now optional), and timing into the shared QueryLogBase, so all status variants can carry these fields. It also renames resultsFound/totalResults → resultCount/totalFound and adds title to QueryLogMatchedDoc. The use-case is updated to remove status guards, and a new entity test file is added.

Overall this is a clean, well-structured change. There are a few issues worth addressing.

---

Issues

suggestion src/server/core/domain/entities/query-log-entry.ts:45

timing?: {durationMs: number}

The timing field is added to QueryLogBase but is never read anywhere — not in the use-case, not in any display logic, and the only test is expect(processingEntry.timing).to.be.undefined. Either use it (e.g., prefer timing.durationMs over computing completedAt - startedAt in formatEntryDuration) or remove it. Dead optional fields silently inflate the public API of the entity.

Fix this →

---

suggestion src/server/core/domain/entities/query-log-entry.ts:26 + src/server/infra/usecase/query-log-use-case.ts:100-102

// entity
export type QueryLogMatchedDoc = {
  path: string
  score: number
  title: string   // ← added
}

// use-case (display loop)
for (const doc of entry.matchedDocs) {
  this.log(`  [${doc.score.toFixed(2)}] ${doc.path}`)   // title never rendered
}

title was added to QueryLogMatchedDoc but is never rendered in either the detail view or the list detail view. If it is needed (e.g., to show a human-readable document name), add it to the display. If it's not needed yet, remove it — don't add fields to the type before they have a consumer (per the Outside-In principle in CLAUDE.md).

Fix this →

---

issue src/server/core/domain/entities/query-log-entry.ts:38-45 (semantic concern)

type QueryLogBase = {
  matchedDocs: QueryLogMatchedDoc[]   // required, even on 'processing' and 'error'
  searchMetadata?: QueryLogSearchMetadata
  ...
}

Moving matchedDocs (required, not optional) to the base means a processing or error entry is forced to carry matchedDocs: []. This trades type-safety for display convenience — the original design where matchedDocs lived only on completed was semantically precise. Consider: could matchedDocs become populated mid-flight (incremental results) on processing entries? If yes, the base placement makes sense. If no, making it optional (matchedDocs?: QueryLogMatchedDoc[]) or reverting it back to the completed variant and using a type predicate in the use-case would be more correct. As-is, all test fixtures must artificially pass matchedDocs: [] for non-completed entries.

---

nitpick test/unit/core/domain/entities/query-log-entry.test.ts:7-52

The top-level const processingEntry: QueryLogEntry = { ... } declarations are type-level compile checks, not runtime test data. This is a valid pattern, but it means some "tests" only verify shape (e.g., expect(doc.title).to.equal('Test') where doc is already typed). The comment // Type-level compile checks makes it clear — just worth knowing that removing these from the describe blocks would make the distinction even cleaner.

---

nitpick test/unit/infra/usecase/query-log-use-case.test.ts:18-28

makeProcessingEntry defaults tier: 0, but tier is now optional in the entity. This is mildly inconsistent with the entity design change (optional tier is the motivating reason to show '—' in the list). A fixture with no tier would better exercise the optional path and validate the '—' fallback for processing entries specifically.

---

What's done well

• The discriminated union pattern is correct and idiomatic — status as the discriminant, with variant-specific fields cleanly added per branch.
• Renaming resultsFound/totalResults → resultCount/totalFound improves consistency.
• The use-case updates are minimal and correct: status guards removed where the base now provides the fields.
• The tier === undefined ? '—' : ... guard is placed consistently in both list and detail display paths.
• The new entity test file is well-structured with distinct sections for narrowing, base fields, constants, and type compile checks.
• taskId addition is a necessary field for associating log entries with tasks — a good addition.

---

Score: 3.5 / 5

Solid design and good test coverage. The two dead fields (timing and title) are the main concrete issues to resolve; the semantic question around matchedDocs on the base is worth a deliberate decision before this merges.