Proj/surface curate query claude

[danhdoan]

Summary

• Problem: brv query --format json returned only the response text, dropping 10+ retrieval metadata signals (matched paths, scores, resolution tier, timing) that QueryExecutor already computes internally.
• Why it matters: Downstream consumers — brv-bridge and the ByteRover Claude plugin — need this metadata to surface visible evidence of memory retrieval to the user. Today recall runs silently every turn; the user has no signal that ByteRover is working or what it retrieved. Surfacing this metadata is the foundation for the broader "Surface ByteRover in Claude Code" initiative (ENG-2537).
• What changed: Extends the brv query --format json envelope with matchedDocs[], tier, durationMs, and topScore. Plumbed via the existing query-log lifecycle hook + task-completed event so older daemons degrade gracefully.
• What did NOT change (scope boundary): --format text (default) output is byte-identical. No retrieval/scoring logic touched. No agent prompt or tool-response changes. No new fields exposed beyond the four above (richer per-doc fields like symbolKind, backlinkCount, relatedPaths deferred until consumers demonstrate need).

Type of change

• New feature
• Bug fix
• 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-2615
• Parent ticket: ENG-2537 (surface ByteRover in Claude Code)
• Related feat: surface recall metadata in brv query --format json #608 (merged into this branch)

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

N/A — this is a feature.

Test plan

• Coverage added:
  • Unit test
  • Integration test
  • Manual verification only
• Test file(s):
  • test/unit/infra/process/query-log-handler.test.ts — 4 new cases on getTaskCompletionData
  • test/commands/query.test.ts — 2 new cases on the query command's JSON envelope
• Key scenario(s) covered:
  • getTaskCompletionData returns full structured payload when all fields present
  • getTaskCompletionData handles no-task / no-result / partial cache-hit shapes
  • brv query --format json surfaces the new fields when daemon emits them
  • brv query --format json gracefully omits the fields when daemon doesn't emit them (older daemon compat)

User-visible changes

None directly. brv query text-mode output unchanged. JSON-mode output is additive — consumers that ignore unknown fields are unaffected.

This unlocks user-visible changes in downstream layers:

• brv-bridge will pass these fields through (Task 2).
• brv-claude-plugin will surface a "🧠 Retrieved N memories…" line in the Claude Code chat panel via the hook systemMessage primitive (Task 3).

Evidence

• Failing test/log before + passing after
• Trace/log snippets
• Screenshot/recording

Sample output after this change:

{
  "command": "query",
  "data": {
    "result": "...",
    "matchedDocs": [
      {"path": "auth/jwt/token.md", "score": 0.92, "title": "...", "snippet": "..."}
    ],
    "tier": 2,
    "durationMs": 47,
    "topScore": 0.92
  },
  "success": true,
  "timestamp": "..."
}

[github-actions]

Claude finished @danhdoan's task in 3m 53s —— View job

---

Review summary — Score: 4 / 5

Clean, focused, additive change that does exactly what ENG-2615 promises. Strict TDD is visible (failing-shape tests on both daemon and CLI sides), backward-compat is properly guarded by conditional spreads, and the new daemon-side surface reuses the existing ITaskLifecycleHook.getTaskCompletionData plumbing instead of inventing a new event — which is the right call. Text-mode output is verifiably untouched; JSON envelope is purely additive.

What's strong

• praise: QueryLogHandler.getTaskCompletionData mirrors CurateLogHandler symmetrically; TaskRouter's existing merge loop (task-router.ts:359-368) handles delivery atomically with task:completed, eliminating the race a separate event would have introduced.
• praise: The flatten step (searchMetadata.topScore → top-level topScore) and the comment explaining why timing is unguarded but searchMetadata is guarded preserves a non-obvious invariant — useful for future maintainers.
• praise: Both the all-fields-present and all-fields-absent CLI shapes are tested. Older-daemon graceful degradation is explicit, not assumed.
• praise: Type extension via intersection (TaskCompletedWithLogId) keeps the upstream @campfirein/brv-transport-client schema authoritative without cast escapes.

What could be tightened (none blocking)

• nitpick: query-log-handler.test.ts:328-344 — the test labels itself "cache hit shape" but uses TIER_DIRECT_SEARCH (=2). Real cache hits use TIER_EXACT_CACHE/TIER_FUZZY_CACHE (query-executor.ts:113-131). Functionally fine; just relabel or swap the tier constant.
• suggestion: Add a CLI test for the realistic partial envelope (cache-hit shape: tier+durationMs+matchedDocs but no topScore). Currently only the binary all/nothing cases are covered; the partial case is the one brv-bridge will see most often.
• issue (low): oclif/lib/task-client.ts imports QueryLogMatchedDoc/QueryLogTier from server/core/domain/entities/. Allowed by current ESLint rules, but since these are described as "the public RecallResult contract" and ENG-2537's brv-bridge will consume them, consider hoisting to src/shared/ as a follow-up.
• question: package-lock.json adds "peer": true to ~11 esbuild native bindings — looks like noise from a re-resolve, not from this feature. Worth confirming intent or reverting if accidental.
• nitpick: query.ts:185-194 — five conditional spreads on one literal is heavy; a small compactObject({durationMs, matchedDocs, tier, topScore}) helper would help if this pattern repeats in brv-bridge.

Scope/architecture check (Outside-In)

✅ The consumer (brv query --format json) drove the four exposed fields; richer per-doc fields are deferred until consumers prove need. No premature entity extraction. Pattern matches the lifecycle hook + transport-event spine the daemon already uses for HITL signals.

---

• Read changed files (query.ts, task-client.ts, query-log-handler.ts, tests)
• Cross-reference call sites (task-router.ts:345-394, query-executor.ts tier branches)
• Verify TDD: tests cover all-present + all-absent envelope shapes
• Check type-safety, undefined handling, backward compat
• Post inline comments + summary

Inline comments posted on:

• test/unit/infra/process/query-log-handler.test.ts:328-344 (test fixture mislabel)
• test/commands/query.test.ts:508 (missing partial-shape test)
• package-lock.json:5914 (incidental peer: true additions)
• src/server/infra/process/query-log-handler.ts:69-88 (praise)
• src/oclif/commands/query.ts:185-194 (readability nit)
• src/oclif/lib/task-client.ts:26-46 (cross-boundary type import — follow-up)
  · branch proj/surface-curate-query-claude

[github-actions]

Claude finished @danhdoan's task in 2m 58s —— View job

---

Re-review (post-synchronize) — Score: 5 / 5

The lockfile cleanup commits (62938cc + merge #611) close the only concrete question from the prior pass. Code-level diff is unchanged, all-additive, type-safe, and TDD-verified on both daemon and CLI sides. Ready to land.

Delta since prior review

• ✅ Lockfile noise resolved. git diff origin/main...HEAD -- package-lock.json is now empty. The 11 esbuild peer: true entries from the rebase have been reverted; commit message confirms npm ci --dry-run passes. Scope is back to the four feature files.
• ↩️ Other carried-over nits remain — all originally tagged non-blocking, restated below for the merge author's call:
  • Test fixture mislabel at test/unit/infra/process/query-log-handler.test.ts:336 (TIER_DIRECT_SEARCH under a "cache hit" label).
  • CLI partial-shape coverage at test/commands/query.test.ts:509 (binary-only — no tier+durationMs+matchedDocs without topScore test, which is the realistic warm-cache shape).
  • src/oclif/lib/task-client.ts:22 cross-boundary type import — fine for now per repo conventions, but worth hoisting to src/shared/ before brv-bridge adopts.
  • src/oclif/commands/query.ts:194 five-spread readability — only worth refactoring if the pattern recurs.

What's strong (unchanged)

• QueryLogHandler.getTaskCompletionData reuses the existing lifecycle-hook merge in TaskRouter — no new event, no race.
• The timing-unguarded / searchMetadata-guarded invariant comment captures the non-obvious tier branching.
• Backward compat is explicit, not assumed: both all-fields and no-fields envelope shapes are tested.
• TaskCompletedWithLogId intersection keeps the upstream transport-client schema authoritative.

Outside-In check

✅ Same as prior review — consumer-driven, no premature entity work.

---

Todo

• Pull latest commits and check what changed since prior review
• Read changed files (query.ts, task-client.ts, query-log-handler.ts, tests)
• Cross-reference call sites & verify TDD invariants
• Check addressed feedback from prior review (lockfile ✅; other nits unchanged)
• Post summary review with inline comments

Inline comments posted on:

• test/unit/infra/process/query-log-handler.test.ts:336 (test mislabel — carried over)
• test/commands/query.test.ts:509 (missing partial-shape test — carried over)
• src/oclif/lib/task-client.ts:22 (cross-boundary import — follow-up)
• src/server/infra/process/query-log-handler.ts:88 (praise — atomic metadata delivery)
• src/oclif/commands/query.ts:194 (heavy spreads — readability nit)
  · branch proj/surface-curate-query-claude