Conversation
…y routing Co-authored-by: mfittko <326798+mfittko@users.noreply.github.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1f917d6ea4
ℹ️ 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".
There was a problem hiding this comment.
Pull request overview
Pins a unified POST /query response contract across routing strategies, updates the CLI to accept a user-selected strategy and display strategy-aware output, and aligns documentation to the new routing/graph response model.
Changes:
- Added/updated TypeScript types to represent the canonical query response shape, including
routingandgraphstructures. - Extended CLI query command with
--strategy/--verboseand strategy-aware output formatting (routing line, metadata filter match, graph documents section). - Updated API reference, CLI docs, architecture docs, and skill docs to describe the multi-strategy router and unified response envelope.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
cli/src/lib/types.ts |
Introduces unified response types (QueryResponse, RoutingDecision, GraphResult, etc.). |
cli/src/lib/api-client.ts |
Extends query() to optionally send strategy and return a typed QueryResponse. |
cli/src/commands/query.ts |
Adds --strategy / --verbose options and implements strategy-aware output formatting. |
docs/09-api-reference.md |
Documents strategy, routing, and expanded graph response example and tables. |
docs/03-cli.md |
Documents new CLI flags and describes per-strategy output behavior. |
docs/01-architecture.md |
Updates /query architecture description and adds router sequence diagram. |
skill/SKILL.md |
Updates skill documentation to include strategy, routing, and updated response examples. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@copilot address all review comments |
…nsistency, tests Co-authored-by: mfittko <326798+mfittko@users.noreply.github.com>
mfittko
left a comment
There was a problem hiding this comment.
Full review complete — approving.
What I verified:
- Unified query response contract types are aligned in CLI (
QueryResponse,RoutingDecision,GraphResult, extendedGraphMeta). - CLI strategy behavior is implemented and validated:
--strategyvalidation,autoomission,--verboserouting line behavior, metadata filter-match output, and graph documents section. - Strategy propagation now covers all query paths, including
/query,/query/download-first, and/query/fulltext-first. - Docs updated consistently across API reference, CLI docs, architecture, and skill guide.
- Build + test evidence:
cli:npm test(191 passing) andnpm run buildpassing.api:npm run buildpassing.apitests are largely green; 3src/db.test.tsfailures are environment-specific (database "raged" does not exist) and unrelated to this PR’s logic.
- All review threads are resolved (0 unresolved).
Given current branch state (including 8fb25b7), this is ready to merge.
|
Final AC/DoD evidence summary for #117 (PR #122) Acceptance Criteria
Definition of Done
Final noteThe remaining review blocker (strategy propagation to download/fulltext query paths) was fixed in commit |
cli/src/lib/types.ts: addQueryResultItem,QueryResponse,RoutingDecision,GraphResult,GraphEntity,GraphEdge,GraphMeta(with full server-side fields + index signature),EntityPath,EntityDocumenttypes; keep oldQueryResultas deprecatedcli/src/lib/api-client.ts: addstrategyparam toquery(), return typedQueryResponsecli/src/commands/query.ts: add--strategyand--verboseflags with validation (allowed set + "auto" → undefined); strategy-aware output formatting (routing line, filter match, graph documents section); fixscopedResponsesto spreadoutpreservingokdocs/09-api-reference.md: fix request example (nographExpand+strategymix); updategraphpresence condition; expandgraph.metafield table (full server-side fields); fix metadatatextnote to reflect actual behaviordocs/03-cli.md: add--strategy,--verbose,--since,--until,--filterField,--filterCombineto flag table; add strategy-aware output subsection with per-strategy examplesdocs/01-architecture.md: extend POST /query bullet; add Query Router sequence diagram; update hybrid sequence diagramskill/SKILL.md: fix filter examples to use key-value format (remove legacymustarray); expand graph meta example with full traversal metadata fields; addstrategyto query parameters table; add Query Strategies subsection--verbose, metadata routing+filter match, graph routing+documents section, empty graph documents suppressionOriginal prompt
This section details on the original issue you should resolve
<issue_title>feat(api+cli+docs): Unified query response contract for multi-strategy routing</issue_title>
<issue_description>## Parent
Summary
Standardize
POST /queryresponse shape across all strategy modes, add strategy-aware CLI output, and update all documentation. This is the final integration point that pins the unified contract and ensures all consumers — API clients, CLI, and docs — are aligned.Dependencies
routingfield in responseGraphResultshapeMerge Order
Scope
In scope
--strategy,--verboseflags (new in this issue)Out of scope
/query/download-firstor/query/fulltext-first--since/--untilCLI flags (already implemented in feat(query): Metadata strategy engine for structured and temporal filters #116)Response Contract
Top-Level Envelope
oktrueresultsQueryResultItem[]graphGraphResultroutingRoutingDecisionPer-Item Result
id"baseId:chunkIndex"or arbitrary stringscore1.0for metadata-onlysourcetextpayloadRouting Decision
strategymetadata | graph | semantic | hybridmethodexplicit | rule | llm | rule_fallback | defaultconfidenceruledurationMsGraph Result
entities[]depth,isSeed,descriptionper entityrelationships[]source,target,typeedgespaths[]documents[]metaentityCount,capped,timedOut,warnings[]CLI Changes
New Flags (owned by this issue)
--strategy <name>--verboseExisting Flags (owned by #116, verified here)
--since <date>--until <date>Strategy-Aware Output
Routing line (shown when strategy ≠ semantic OR when
--verbose):Output per strategy:
flowchart LR S[Strategy] --> SE[semantic] S --> ME[metadata] S --> GR[graph] S --> HY[hybrid] SE --> SE_OUT["#N score=X<br/>collection: …<br/>source: …<br/>snippet: …"] ME --> ME_OUT["#N score=1.00<br/>routing: metadata …<br/>filter match: field=val …"] GR --> GR_OUT["#N score=X<br/>routing: graph …<br/>snippet: …<br/>--- graph documents ---"] HY --> HY_OUT["#N score=X<br/>routing: hybrid …<br/>snippet: …<br/>--- graph documents ---"]✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.