feat: surface recall metadata in brv query --format json#608
Conversation
Extend the `brv query --format json` envelope with `matchedDocs`, `tier`, `durationMs`, and `topScore` so downstream consumers (brv-bridge, the ByteRover Claude plugin) can surface evidence of memory retrieval to the user. The data already exists in QueryExecutor and is persisted to query-log entries — this just exposes it on the public CLI surface. Implementation: - QueryLogHandler.getTaskCompletionData() exposes the structured payload via the existing lifecycle-hook contract; TaskRouter automatically merges it into the task:completed event sent to the originating client. - task-client.ts extends TaskCompletedWithLogId and TaskCompletionResult with the new optional fields; older daemons that don't emit them degrade gracefully (fields stay undefined). - query.ts conditionally spreads the new fields into the JSON envelope so consumers don't see undefined keys for non-query tasks. Tests: - 4 new cases for getTaskCompletionData (all-fields, no-task, no-result, partial cache-hit shape). - 2 new cases for the query command's JSON envelope (fields surfaced when present; gracefully omitted when absent). Parent ticket: ENG-2537.
|
Claude finished @RyanNg1403's task in 2m 55s —— View job PR Review: feat: [ENG-2615] surface recall metadata in brv query --format jsonTasks
Overall Score: 4 / 5Summary: Clean, well-scoped implementation. The lifecycle-hook pattern for surfacing metadata is a good fit, backward compatibility is handled correctly (conditional spreads ensure older daemons / non-query tasks emit unchanged JSON), and the test coverage is thorough. Three items worth addressing — one is a doc inaccuracy that will confuse future readers, one is a dead-code guard, and one is an architectural concern about a cross-boundary import that deserves a follow-up ticket. Findings
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4662fb33e8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
RyanNg1403
changed the title
Three changes from PR review: 1. Drop the dead `if (timing !== undefined)` guard in QueryLogHandler.getTaskCompletionData() — `timing` is required on QueryExecutorResult (every executor branch sets it), so the guard is unreachable. Inline the assignment instead. 2. Fix the JSDoc on TaskCompletedWithLogId in task-client.ts — it listed the QueryExecutorResult-shaped names (`timing`/`searchMetadata`) but the actual type uses the flattened public-contract names (`durationMs`/`topScore`). Updated to match. 3. Refresh package-lock.json so the axios entry resolves to 1.16.0 (matching the manifest pin) instead of the 1.15.0 left behind by the rebase. Restores deterministic-install agreement.
|
Claude finished @RyanNg1403's task in 1m 55s —— View job PR Review: feat: [ENG-2615] surface recall metadata in brv query --format jsonTasks
Overall Score: 4.5 / 5Summary: Clean, well-scoped implementation. The refactor commit ( What the refactor commit fixed ✅
Remaining findings
|
* feat: [ENG-2615] surface recall metadata in brv query --format json Extend the `brv query --format json` envelope with `matchedDocs`, `tier`, `durationMs`, and `topScore` so downstream consumers (brv-bridge, the ByteRover Claude plugin) can surface evidence of memory retrieval to the user. The data already exists in QueryExecutor and is persisted to query-log entries — this just exposes it on the public CLI surface. Implementation: - QueryLogHandler.getTaskCompletionData() exposes the structured payload via the existing lifecycle-hook contract; TaskRouter automatically merges it into the task:completed event sent to the originating client. - task-client.ts extends TaskCompletedWithLogId and TaskCompletionResult with the new optional fields; older daemons that don't emit them degrade gracefully (fields stay undefined). - query.ts conditionally spreads the new fields into the JSON envelope so consumers don't see undefined keys for non-query tasks. Tests: - 4 new cases for getTaskCompletionData (all-fields, no-task, no-result, partial cache-hit shape). - 2 new cases for the query command's JSON envelope (fields surfaced when present; gracefully omitted when absent). Parent ticket: ENG-2537. * refactor: address review feedback on getTaskCompletionData and JSDoc Three changes from PR review: 1. Drop the dead `if (timing !== undefined)` guard in QueryLogHandler.getTaskCompletionData() — `timing` is required on QueryExecutorResult (every executor branch sets it), so the guard is unreachable. Inline the assignment instead. 2. Fix the JSDoc on TaskCompletedWithLogId in task-client.ts — it listed the QueryExecutorResult-shaped names (`timing`/`searchMetadata`) but the actual type uses the flattened public-contract names (`durationMs`/`topScore`). Updated to match. 3. Refresh package-lock.json so the axios entry resolves to 1.16.0 (matching the manifest pin) instead of the 1.15.0 left behind by the rebase. Restores deterministic-install agreement. * chore: drop esbuild peer:true entries from lockfile These 11 entries were introduced by an earlier 'npm install' in commit 4662fb3 (PR #608) running with a different npm CLI version that re-resolves esbuild's optionalDependencies as peer entries. They are descriptive metadata only, do not affect npm ci installs, and are not present on main. Drop them so proj -> main lockfile diff is empty. Verified: 'npm ci --dry-run' passes against the reverted lockfile. --------- Co-authored-by: Nguyễn Thuận Phát <nguyenthuanphatvl@gmail.com>
2 participants
Summary
brv query --format jsoncollapses everything into a plain-textresultfield, so downstream consumers (brv-bridge, the ByteRover Claude plugin) can't tell which docs informed the answer or how the answer was reached. ByteRover's "memory used" evidence stays invisible.🧠 ByteRover returns N memories: …line in Claude Code consumes structured retrieval metadata; this PR is what surfaces it at the source.matchedDocs,tier,durationMs,topScore. Plumb via the existing lifecycle-hook pattern.--format text) byte-identical. No new search/scoring logic — only exposes whatQueryExecutoralready computes. No MCP tool changes.Type of change
Scope (select all touched areas)
query.tsquery-log-handler.tstask-client.tstypesLinked issues
N/A — Linear linkage is via branch name and commit subjects.
Root cause (bug fixes only, otherwise write
N/A)N/A.
Test plan
test/unit/infra/process/query-log-handler.test.ts— 4 new cases forgetTaskCompletionData(all-fields, no-task, no-result, partial cache-hit shape).test/commands/query.test.ts— 2 new cases for the JSON envelope (fields surfaced when present, gracefully omitted when absent — older daemons or non-query tasks).matchedDocs; absent-field graceful degradation.User-visible changes
brv query --format json "X"now returns:```json
{
"command": "query",
"data": {
"event": "completed",
"matchedDocs": [{"path": "auth/jwt-tokens.md", "score": 0.92, "title": "JWT tokens"}, …],
"result": "…synthesised prose…",
"status": "completed",
"taskId": "uuid",
"tier": 2,
"durationMs": 184,
"topScore": 0.92
},
"success": true,
"timestamp": "…"
}
```
--format text(default) output is byte-identical to before.Evidence
npx mocha test/unit/infra/process/query-log-handler.test.ts: 17/17 pass;npx mocha test/commands/query.test.ts: 23/23 pass.Checklist
npm test)npm run lint)npm run typecheck)npm run build)data.resultkeep workingproj/surface-curate-query-claudeRisks and mitigations
durationMs,topScoreflat). The internalQueryLogEntrykeeps its nestedtiming.durationMs/searchMetadata.topScoreshape — only the publictask:completedevent payload and CLI envelope are flat.Implementation notes
titlefield onQueryLogMatchedDocis required (not optional) —QueryExecutor.buildMatchedDocs()always populates it. Tightened from the proposed schema. Easy to relax if a future executor change changes the contract.snippet,lastCurated,symbolKind,symbolPath,backlinkCount,relatedPaths,overviewPath,origin) —QueryExecutordoesn't populate them today. Additive — easy to add back.