feat: [ENG-1899] Create brv query-log summary oclif command

[bao-byterover]

No description provided.

[github-actions]

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

---

PR Review — feat: [ENG-1899] brv query-log summary — Score: 4/5

• Gather context and understand the PR
• Read changed files
• Review command implementation (summary.ts)
• Review use case interface and implementation
• Review narrative formatter
• Review tests
• Review supporting changes (time-filter, view.ts files)
• Post inline comments
• Post final summary

---

Summary

Well-structured scaffold for the brv query-log summary command. The outside-in design is followed correctly — the oclif command drives the interface, the use case fulfils it, and the formatter is a pure function tested in isolation. The createDependencies testability pattern is consistent with QueryLogView. Test coverage is thorough with good fixture helpers.

Two correctness issues in the narrative formatter need attention before the real aggregation (ENG-1898) lands; they will produce misleading output for any non-24h time window.

Issues

Severity	Location	Description
Correctness	narrative-formatter.ts:3	EMPTY_STATE_MESSAGE hardcodes "last 24 hours" — incorrect for --last 7d etc.
Correctness	narrative-formatter.ts:41	"today" is wrong for multi-day windows (--last 7d, --last 30d)
Duplication	summary.ts:71	parseTime is now triplicated across CurateView, QueryLogView, and QueryLogSummary
Style	summary.test.ts:229,246,263	as type assertions violate CLAUDE.md conventions; use a type guard
UX	query-log-summary-use-case.ts:21	Time filters are silently ignored in the stub — no feedback to the user
Docs	summary.ts:36	--last silently wins over --since; undocumented; consider exclusive: ['since']
Nitpick	summary.ts:64	Spread-object pattern for optional flags is verbose; direct pass of undefined is idiomatic
Nitpick	i-query-log-summary-use-case.ts:46	Inline options type on run() could be a named QueryLogSummaryOptions type
Suggestion	use-case.test.ts:86	"does not throw" test is weak; add an assertion on terminal output

Positives

• Clean separation: command → use case → pure formatter
• createDependencies injection pattern makes the command unit-testable without real filesystem
• Narrative formatter tests are comprehensive and well-structured with reusable fixture helpers
• null → undefined alignment across time-filter.ts and its callers is a good convention cleanup
• --before-only path (no implicit 24h window) is handled correctly and tested

[github-actions]

issue (correctness): The empty-state message hardcodes "the last 24 hours", but this function is called regardless of the actual time window the user requested. If a user runs brv query-log summary --last 7d and gets zero results, they'll see a misleading message.

The formatter is currently a pure function with no knowledge of the queried window — you could either (a) pass the time range into the function so it can render a context-aware message, or (b) keep it generic and remove the specific duration reference.

Suggested change

	const EMPTY_STATE_MESSAGE =
	const EMPTY_STATE_MESSAGE =
	'No queries recorded in the selected time window. Your knowledge base is ready — try asking a question!'

[github-actions]

issue (correctness): "today" is semantically wrong when the user queries --last 7d or --last 30d. The narrative will say "Your team asked 312 questions today" for a 30-day window, which is misleading.

Consider making the time-window reference dynamic (passing period info to the formatter) or using a neutral phrase like "in the selected period". Fix this →

[github-actions]

suggestion (ux): The after and before options are accepted but silently ignored in the stub. A user running brv query-log summary --last 7d gets an identical response to a user running with no flags — with no indication that the filter wasn't applied.

Consider logging a brief notice alongside the placeholder, e.g.:

// inside the TODO block, before format dispatch:
const rangeNote = options.after ?? options.before
  ? ' (time filters will apply once aggregation is implemented)'
  : ''

Or simply add a comment in the text output that this is pending real aggregation. This avoids user confusion during the scaffolding phase.

[github-actions]

suggestion (docs): The --last flag silently takes precedence over --since (see resolveAfter), but neither flag's description documents this. A user providing both will get no warning and no indication that --since was ignored.

Consider adding exclusive: ['since'] to the --last flag definition so oclif rejects the combination, or at minimum document the precedence in the description string:

Suggested change

	description: 'Relative time window (e.g., 1h, 24h, 7d, 30d). Default: 24h',
	last: Flags.string({
	description: 'Relative time window (e.g., 1h, 24h, 7d, 30d). Default: 24h. Takes precedence over --since.',
	exclusive: ['since'],
	}),

[github-actions]

nitpick (style): The spread-object pattern for optional fields is more verbose than necessary. Passing undefined-valued keys has no effect on the callee when the options type uses optional properties (after?: number):

Suggested change

	await useCase.run({
	await useCase.run({
	after,
	before,
	format,
	})

This is cleaner and TypeScript will correctly infer that after/before may be undefined.

[github-actions]

issue (duplication): parseTime is now implemented identically in three commands: CurateView, QueryLogView, and QueryLogSummary. This is the third copy of the same private method. Consider extracting it to src/oclif/lib/time-filter.ts (which already owns the parsing logic) as a named helper, e.g.:

// in time-filter.ts
export function parseTimeFilterOrThrow(
  value: string,
  flagName: string,
  errorFn: (msg: string, opts: {exit: number}) => never,
): number {
  const ts = parseTimeFilter(value)
  if (ts === undefined) {
    errorFn(
      `Invalid time value for ${flagName}: "${value}". Use ISO date (2024-01-15) or relative (30m, 1h, 24h, 7d, 2w).`,
      {exit: 2},
    )
  }
  return ts
}

Or at minimum, leave a // TODO: extract shared parseTime comment so it's tracked. Fix this →

[github-actions]

issue (style): as type assertions are banned by the project's TypeScript conventions (CLAUDE.md: "Avoid as Type assertions - use type guards or proper typing instead"). This pattern appears in three error-handling tests (lines 229, 246, 263).

Use a type guard instead:

function isOclifError(err: unknown): err is {oclif: {exit: number}} {
  return (
    typeof err === 'object' &&
    err !== null &&
    'oclif' in err &&
    typeof (err as Record<string, unknown>).oclif === 'object'
  )
}

// then in the catch block:
if (isOclifError(error)) {
  expect(error.oclif.exit).to.equal(2)
} else {
  expect.fail('Expected an oclif error')
}

Or factor it into the test helpers so all three tests share it. Fix this →

[github-actions]

nitpick: IQueryLogSummaryUseCase is an interface with only a run method — the method signature on run uses an inline object type rather than referencing the existing QueryLogSummary* types. Since format is already QueryLogSummaryFormat, consider extracting the options parameter to a named type for consistency with similar use-case interfaces in the codebase, and to make it easier to extend later:

export type QueryLogSummaryOptions = {
  after?: number
  before?: number
  format?: QueryLogSummaryFormat
}

export interface IQueryLogSummaryUseCase {
  run(options: QueryLogSummaryOptions): Promise<void>
}

[github-actions]

suggestion: The options forwarding test only verifies "does not throw" when after/before are provided. Since the use case currently ignores these options (the TODO for ENG-1898 scaffolds a zero summary regardless), this test doesn't assert much. Consider adding an assertion that the terminal still receives output — this documents the expected behavior more precisely and will catch a regression if the stub is changed to throw on unrecognized options:

it('does not throw when after/before are provided', async () => {
  const {terminal, useCase} = createUseCase(sandbox)

  await useCase.run({after: 1000, before: 2000, format: 'text'})

  expect(terminal.log.calledOnce).to.be.true
})

[github-actions]

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

---

PR Review — feat: [ENG-1899] brv query-log summary — Score: 4/5

• Gather context and read changed files
• Compare against previous review issues
• Review narrative formatter fixes
• Review command implementation updates
• Review test changes
• Post final summary

---

What Was Fixed

The "fix review" commit (2bcfb96) addressed four of the nine previous issues cleanly:

Previous Issue	Status
as type assertions in tests	✅ Replaced with proper isOclifExitError guard + expectOclifExit helper
Spread-object pattern for optional flags	✅ Simplified to useCase.run({after, before, format})
--last precedence undocumented	✅ Description now reads "Takes precedence over --since."
"does not throw" test too weak	✅ Now asserts terminal.log.calledOnce

---

Remaining Issues

issue (correctness) — query-log-summary-narrative-formatter.ts:3

EMPTY_STATE_MESSAGE still hardcodes "the last 24 hours". A user running brv query-log summary --last 7d with zero results will see a misleading message:

No queries recorded in the last 24 hours. Your knowledge base is ready...

The formatter is a pure function with no knowledge of the actual queried window. The simplest fix — keeping it window-agnostic — is to use a neutral phrase:

const EMPTY_STATE_MESSAGE =
  'No queries recorded in the selected time window. Your knowledge base is ready — try asking a question!'

This also has a secondary effect: the use-case test at line 67 (use-case.test.ts) is coupled to this string — expect(output).to.include('No queries recorded in the last 24 hours') — so when the constant is updated that test will need to be updated too.

Fix this →

issue (correctness) — query-log-summary-narrative-formatter.ts:41

"today" in buildOverviewParagraph is still hardcoded:

`Your team asked ${totalQueries} questions today. `

For a --last 30d window this will say "Your team asked 312 questions today" — clearly wrong. A neutral phrase keeps it accurate for all windows:

`Your team asked ${totalQueries} questions in the selected period. `

Fix this →

issue (duplication) — summary.ts:67

parseTime is now triplicated across curate/view.ts:87, query-log/view.ts:96, and query-log/summary.ts:67. All three are byte-for-byte identical. This is worth extracting before the next command is added. A parseTimeFilterOrThrow helper in src/oclif/lib/time-filter.ts would eliminate the duplication (the suggested signature was in the previous review; it still applies).

Fix this →

nitpick — i-query-log-summary-use-case.ts:46

The inline options object on run() is still anonymous. Since QueryLogSummaryFormat is already a named export, the options type could also be named for consistency with other use-case interfaces and to make it easier to extend:

export type QueryLogSummaryOptions = {
  after?: number
  before?: number
  format?: QueryLogSummaryFormat
}

export interface IQueryLogSummaryUseCase {
  run(options: QueryLogSummaryOptions): Promise<void>
}

nitpick — UX, query-log-summary-use-case.ts:21

Time filters are still silently ignored in the stub. This is acceptable for a scaffold, but a brief comment in the terminal output would prevent user confusion (e.g. "Note: time filters apply once aggregation is implemented in ENG-1898"). This is low priority but consider flagging it before wider rollout.

---

Summary

Good follow-up — the style and documentation issues were all addressed correctly. The two correctness issues in the narrative formatter (EMPTY_STATE_MESSAGE and "today") remain and will produce misleading output as soon as real data flows through. They are the highest priority items before ENG-1898 lands. The parseTime deduplication is worth doing whenever there's a convenient moment, since a fourth copy of this method in a future command will be harder to clean up.