refactor: split query-engine.ts into retrieval stages#31
Conversation
Original prompt from Stanislau
|
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
📝 WalkthroughWalkthroughSplit the monolithic query pipeline into staged runtime modules: query normalization, candidate seeding, candidate ranking, bounded frontier grounding expansion, deterministic answer projection, and optional synthesis orchestration; QueryEngine now orchestrates these stages and passes stage outputs through the trace/result surfaces. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant QE as QueryEngine
participant N as QueryNormalizer
participant S as CandidateSeeder
participant R as CandidateRanker
participant F as FrontierExpander
participant P as AnswerProjector
participant Synth as SynthesisAdapter
Client->>QE: query(question)
QE->>N: normalizeQuery(question, snapshot)
N-->>QE: NormalizedQuery
QE->>S: seedCandidates(normalized, snapshot, session)
S-->>QE: SeedingResult (candidateMap, frontierCandidates)
QE->>R: rankCandidates(question, candidateMap, snapshot)
R-->>QE: RankingResult (initialNodeIds, initialAnchorIds, routeWins)
QE->>F: expandGrounding(question, candidates, initialNodeIds, initialAnchorIds, frontierCandidates, frontierKeys, snapshot)
F-->>QE: GroundingResult (selectedNodeIds, selectedAnchorIds, retrievalHops, graphExpansions, followedReferences, sufficient)
QE->>P: buildPatternAnswer / buildRouteAnswer / answerPartCDrafts(...)
P-->>QE: QueryResult (deterministic)
QE->>Synth: synthesizeAnswer(question, mode, trace, nodes, slices, deterministicResult, synthesizer)
Synth-->>QE: QueryResult (merged/synthesized)
QE-->>Client: final QueryResult
Estimated code review effort🎯 4 (Complex) | ⏱️ ~55 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the QueryEngine into six specialized modules to improve maintainability and separation of concerns. Feedback identifies several performance bottlenecks in graph traversal and filtering within the new modules, recommending the use of pre-filtered neighborEdges and Set-based lookups to optimize retrieval efficiency.
| for (const nodeId of selectedNodeIds) { | ||
| for (const edge of snapshot.relationGraph) { | ||
| if (edge.from !== nodeId) { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
This graph traversal is highly inefficient. Iterating over the entire relationGraph for every node in selectedNodeIds during each retrieval hop results in $O(Hops \times SelectedNodes \times TotalEdges)$ complexity. This will cause significant performance degradation as the graph size increases. Since CompiledNode already contains pre-filtered neighborEdges, you should iterate over those instead.
for (const nodeId of selectedNodeIds) {
const node = snapshot.compiledNodes[nodeId];
if (!node) continue;
for (const edge of node.neighborEdges) {
There was a problem hiding this comment.
Fixed in 3f5fedd — now iterates node.neighborEdges instead of scanning the entire relationGraph for each selected node.
| const relations = snapshot.relationGraph | ||
| .filter((edge) => ids.includes(edge.from) && ids.includes(edge.to)) | ||
| .map((edge) => ({ from: edge.from, relation: edge.relation, to: edge.to })); |
There was a problem hiding this comment.
Filtering the entire relationGraph to find edges between a small set of ids is inefficient ($O(TotalEdges \times Ids)$). Using a Set for lookups and iterating over the neighborEdges of the selected nodes would be significantly faster.
const idSet = new Set(ids);
const relations = ids.flatMap((id) =>
(snapshot.compiledNodes[id]?.neighborEdges ?? [])
.filter((edge) => idSet.has(edge.to))
.map((edge) => ({ from: edge.from, relation: edge.relation, to: edge.to })),
);There was a problem hiding this comment.
Fixed in 3f5fedd — now uses Set + neighborEdges instead of scanning the full relationGraph. Applied to both buildRouteAnswer and buildPatternAnswer.
| } | ||
| }; | ||
|
|
||
| for (const nodeId of selectedNodeIds) { |
There was a problem hiding this comment.
buildFrontier re-scans all selectedNodeIds in every hop. However, nodes processed in previous hops have already had their neighbors added to the queue. To optimize the retrieval process, you should only iterate over the "frontier" nodes (those newly added in the immediately preceding hop).
There was a problem hiding this comment.
Good point on the redundant work. However, changing to frontier-only iteration would alter the ranking behavior — previously seen nodes could have updated candidate scores from earlier hops that change their neighbors' relative priority. Since this is a refactoring PR (behavioral parity with the original), I'll keep the current approach. The selected Set already skips re-registering already-picked nodes, so the main cost is re-evaluating their neighbors — bounded by MAX_HOPS × selectedNodes × avgNeighbors which stays small with the neighborEdges fix already applied.
There was a problem hiding this comment.
Pull request overview
Refactors the runtime retrieval pipeline by splitting the previously monolithic QueryEngine implementation into discrete stage modules, leaving QueryEngine as an orchestrator that wires the stages together while preserving the existing public API.
Changes:
- Introduces stage modules for query normalization, candidate seeding/ranking, frontier expansion, answer projection, and synthesis handoff.
- Rewrites
src/runtime/query-engine.tsto delegate retrieval/answer logic to the new stages and keep orchestration + public methods. - Extracts deterministic answer construction and optional local synthesis behavior into dedicated helpers.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/runtime/query-engine.ts | Converts QueryEngine into a slim orchestrator delegating to stage modules and preserving public methods (query/trace/inspect/readDoc/...). |
| src/runtime/query-normalizer.ts | Adds normalization + signal detection (ids/lexemes/routes/family/status) with a typed NormalizedQuery contract. |
| src/runtime/candidate-seeder.ts | Adds candidate seeding logic (pattern/route scoring, lexeme links, index descriptions, heuristics, session context) returning a typed SeedingResult. |
| src/runtime/candidate-ranker.ts | Adds candidate sorting + route-vs-pattern selection and initial node/anchor selection, with RankingResult. |
| src/runtime/frontier-expander.ts | Adds bounded frontier expansion and grounding sufficiency checks, returning a typed GroundingResult. |
| src/runtime/answer-projector.ts | Extracts deterministic answer projection (route/pattern/Part C drafts), synthesis slice prep, and confidence/gaps helpers. |
| src/runtime/synthesis-adapter.ts | Extracts optional local synthesizer invocation with deterministic fallback and error-to-gap reporting. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let score = scoreOverlap( | ||
| queryTokens, | ||
| `${indexNode.title} ${indexNode.description} ${indexNode.path.join(' ')}`, | ||
| ); | ||
| const normalizedDescription = normalizeForLookup(indexNode.description); | ||
| if ( | ||
| normalizedDescription && | ||
| normalizedDescription.length > 8 && | ||
| normalizeForLookup(question).includes(normalizedDescription) | ||
| ) { |
There was a problem hiding this comment.
In addIndexDescriptionCandidates(), normalizeForLookup(question) is recomputed inside the loop for every indexNode (line 155). This is unnecessary work per query and can be avoided by precomputing the normalized question once before iterating, then reusing it for the substring check.
There was a problem hiding this comment.
Fixed in 3f5fedd — normalizeForLookup(question) is now precomputed as normalizedQ once before the loop.
| } | ||
|
|
||
| const MAX_SELECTED_ANCHORS = 12; | ||
|
|
||
| export function rankCandidates( |
There was a problem hiding this comment.
MAX_SELECTED_ANCHORS is redefined here and also in other retrieval-stage modules (e.g., src/runtime/frontier-expander.ts). Duplicating shared retrieval limits across modules makes it easy for these values to drift and change behavior inconsistently. Consider centralizing these constants (e.g., in src/runtime/constants.ts or a retrieval-specific constants module) and importing them from all stages.
There was a problem hiding this comment.
Fixed in 3f5fedd — all retrieval limit constants are now centralized in src/runtime/constants.ts.
| } from './types.js'; | ||
|
|
||
| const MAX_HOPS = 6; | ||
| const MAX_SELECTED_ANCHORS = 12; | ||
|
|
||
| export interface GroundingResult { |
There was a problem hiding this comment.
MAX_HOPS and MAX_SELECTED_ANCHORS are defined locally in this stage while other stages/orchestrator have their own copies of related limits (e.g., MAX_SELECTED_ANCHORS in src/runtime/candidate-ranker.ts). To avoid accidental divergence in retrieval behavior, consider centralizing these retrieval-limit constants in a shared module and importing them here.
There was a problem hiding this comment.
Fixed in 3f5fedd — MAX_HOPS, MAX_SELECTED_ANCHORS, MAX_EXCLUDED, and MAX_SYNTHESIS_SLICES are now centralized in src/runtime/constants.ts and imported by all stage modules.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/runtime/query-engine.ts (1)
111-116:⚠️ Potential issue | 🟠 MajorDon't switch to a route answer just because expansion touched a route node.
selectedNodeIdscan contain route nodes added during grounding even when the ranker did not actually choose a route result. This branch will then emit a route narrative and route-default metadata instead of the pattern answer traced from the winning support set. Gate route projection on the ranker's route-win signal, not on the mere presence of any route node in the final selection.Possible direction
- const routeNodeId = trace.selectedNodeIds.find( - (nodeId) => this.snapshot.compiledNodes[nodeId]?.kind === 'route', - ); + const routeNodeId = trace.routeWins + ? trace.selectedNodeIds.find( + (nodeId) => this.snapshot.compiledNodes[nodeId]?.kind === 'route', + ) + : undefined;Also carry
routeWinsthroughTraceResultfromranking.routeWinsintrace().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/runtime/query-engine.ts` around lines 111 - 116, The code currently chooses buildRouteAnswer whenever trace.selectedNodeIds contains any route node (routeNodeId), which can be set during grounding even if the ranker didn't pick a route; instead, propagate the ranker's route-win signal through TraceResult (carry ranking.routeWins from trace()) and change the branch to use that signal (e.g., trace.routeWins or trace.ranking.routeWins) to decide between buildRouteAnswer and buildPatternAnswer; update the trace() implementation to include routeWins from ranking.routeWins and then gate the route projection on that new field rather than the presence of a route node in trace.selectedNodeIds.
♻️ Duplicate comments (3)
src/runtime/frontier-expander.ts (1)
85-123:⚠️ Potential issue | 🟠 MajorCompute sufficiency from the same bounded anchor set you return.
After anchors are appended here,
sufficientandRetrievalHop.sufficientAfterare computed from the unbounded list, but the returned trace truncates anchors back toMAX_SELECTED_ANCHORS. That letstrace.sufficientdescribe support callers never receive. Re-cap before each sufficiency check and derive hop state from that same bounded set.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/runtime/frontier-expander.ts` around lines 85 - 123, The sufficiency check uses the unbounded selectedAnchorIds but the returned trace truncates anchors to MAX_SELECTED_ANCHORS; before calling isGroundingSufficient (and before pushing into retrievalHops), compute a boundedAnchors = unique(selectedAnchorIds).slice(0, MAX_SELECTED_ANCHORS) and use that (and unique(selectedNodeIds)) when calling isGroundingSufficient so retrievalHops.sufficientAfter reflects the same anchor set you return; update the retrievalHops.push entry to record sufficientAfter based on the boundedAnchors rather than the full list.src/runtime/answer-projector.ts (1)
198-206:⚠️ Potential issue | 🟠 MajorUse real anchor IDs for Part C citations.
expandCitations()resolves throughsnapshot.anchorMap, so'Part C catalog'will always come backnot_found. Return actual anchor IDs from the grouped patterns here, or[]if this answer has no expandable evidence.Possible fix
export function answerPartCDrafts( question: string, mode: AnswerMode, snapshot: Snapshot, rebuilt: boolean, ): QueryResult { const grouped = getPartCDraftsByCluster(snapshot.patternGraph.nodes); const ids = Object.values(grouped).flatMap((patterns) => patterns.map((pattern) => pattern.id)); + const citations = unique( + Object.values(grouped).flatMap((patterns) => + patterns.flatMap((pattern) => pattern.sectionIds), + ), + ); const answerLines = Object.entries(grouped).flatMap(([cluster, patterns]) => { const rows = patterns.map((pattern) => `- ${pattern.id} - ${pattern.title}`); return rows.length > 0 ? [`${cluster}:`, ...rows] : [`${cluster}:`, '- none']; }); return buildResult(question, mode, rebuilt, snapshot, { answer: answerLines.join('\n'), ids, relations: [], constraints: [ 'Only rows whose Part is Part C and whose status is Draft are included.', 'Cluster labels come from the top-of-file catalog and stay local to the source monolith.', ], - citations: ['Part C catalog'], + citations, confidence: 0.95, gaps: [], status: 'ok',🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/runtime/answer-projector.ts` around lines 198 - 206, The citations field currently contains a literal string ('Part C catalog') which won't resolve via expandCitations() because it expects anchor IDs from snapshot.anchorMap; update the code that returns buildResult(...) to populate the citations array with the actual anchor IDs gathered from the grouped patterns/evidence (the same place you compute groupedPatterns or evidence IDs) or an empty array if none exist, ensuring the field uses those anchor ID strings rather than the human-readable 'Part C catalog' so expandCitations(snapshot.anchorMap, citations) can successfully resolve them.src/runtime/candidate-seeder.ts (1)
227-243:⚠️ Potential issue | 🟠 MajorDon't reuse session context for every ID-free query.
This still returns
truefor any query with no detected IDs, so unrelated standalone questions get biased by the previous session's selections. Please require follow-up/anaphora cues here, and considerlastSelectedRouteIdas valid prior context too.Possible fix
function shouldApplySessionContext( normalizedQuestion: string, detectedIds: string[], sessionState?: RetrievalSessionState, ): boolean { - if (!sessionState || sessionState.lastSelectedNodeIds.length === 0) { + if (!sessionState) { return false; } - if (detectedIds.length === 0) { - return true; - } - - return /\bit\b|\bthat\b|\bthose\b|\bthem\b|\bconnect\b|\brelate\b|\balso\b/.test( - normalizedQuestion, - ); + const hasPriorSelection = + sessionState.lastSelectedNodeIds.length > 0 || Boolean(sessionState.lastSelectedRouteId); + const followUpCue = + /\bit\b|\bthat\b|\bthose\b|\bthem\b|\bconnect\b|\brelate\b|\balso\b/.test( + normalizedQuestion, + ); + + return hasPriorSelection && detectedIds.length === 0 && followUpCue; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/runtime/candidate-seeder.ts` around lines 227 - 243, The function shouldApplySessionContext currently treats any query with no detected IDs as follow-up and returns true; change it so session context is only applied when there are explicit anaphora/follow-up cues in normalizedQuestion and when sessionState contains prior context (either lastSelectedNodeIds non-empty or lastSelectedRouteId set). Concretely, update the initial guard to return false if sessionState is missing or both sessionState.lastSelectedNodeIds is empty and sessionState.lastSelectedRouteId is falsy, then when detectedIds.length === 0 return the regex.test(normalizedQuestion) && (sessionState.lastSelectedNodeIds.length > 0 || Boolean(sessionState.lastSelectedRouteId)); keep the existing regex and parameter names (normalizedQuestion, detectedIds, sessionState, lastSelectedNodeIds, lastSelectedRouteId).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/runtime/query-engine.ts`:
- Around line 111-116: The code currently chooses buildRouteAnswer whenever
trace.selectedNodeIds contains any route node (routeNodeId), which can be set
during grounding even if the ranker didn't pick a route; instead, propagate the
ranker's route-win signal through TraceResult (carry ranking.routeWins from
trace()) and change the branch to use that signal (e.g., trace.routeWins or
trace.ranking.routeWins) to decide between buildRouteAnswer and
buildPatternAnswer; update the trace() implementation to include routeWins from
ranking.routeWins and then gate the route projection on that new field rather
than the presence of a route node in trace.selectedNodeIds.
---
Duplicate comments:
In `@src/runtime/answer-projector.ts`:
- Around line 198-206: The citations field currently contains a literal string
('Part C catalog') which won't resolve via expandCitations() because it expects
anchor IDs from snapshot.anchorMap; update the code that returns
buildResult(...) to populate the citations array with the actual anchor IDs
gathered from the grouped patterns/evidence (the same place you compute
groupedPatterns or evidence IDs) or an empty array if none exist, ensuring the
field uses those anchor ID strings rather than the human-readable 'Part C
catalog' so expandCitations(snapshot.anchorMap, citations) can successfully
resolve them.
In `@src/runtime/candidate-seeder.ts`:
- Around line 227-243: The function shouldApplySessionContext currently treats
any query with no detected IDs as follow-up and returns true; change it so
session context is only applied when there are explicit anaphora/follow-up cues
in normalizedQuestion and when sessionState contains prior context (either
lastSelectedNodeIds non-empty or lastSelectedRouteId set). Concretely, update
the initial guard to return false if sessionState is missing or both
sessionState.lastSelectedNodeIds is empty and sessionState.lastSelectedRouteId
is falsy, then when detectedIds.length === 0 return the
regex.test(normalizedQuestion) && (sessionState.lastSelectedNodeIds.length > 0
|| Boolean(sessionState.lastSelectedRouteId)); keep the existing regex and
parameter names (normalizedQuestion, detectedIds, sessionState,
lastSelectedNodeIds, lastSelectedRouteId).
In `@src/runtime/frontier-expander.ts`:
- Around line 85-123: The sufficiency check uses the unbounded selectedAnchorIds
but the returned trace truncates anchors to MAX_SELECTED_ANCHORS; before calling
isGroundingSufficient (and before pushing into retrievalHops), compute a
boundedAnchors = unique(selectedAnchorIds).slice(0, MAX_SELECTED_ANCHORS) and
use that (and unique(selectedNodeIds)) when calling isGroundingSufficient so
retrievalHops.sufficientAfter reflects the same anchor set you return; update
the retrievalHops.push entry to record sufficientAfter based on the
boundedAnchors rather than the full list.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b5c9da4a-d6b1-438b-beb1-c27f0e5f014f
📒 Files selected for processing (6)
src/runtime/answer-projector.tssrc/runtime/candidate-ranker.tssrc/runtime/candidate-seeder.tssrc/runtime/constants.tssrc/runtime/frontier-expander.tssrc/runtime/query-engine.ts
✅ Files skipped from review due to trivial changes (1)
- src/runtime/constants.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/runtime/candidate-ranker.ts
![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: 9a97a7f144
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| const followUpCue = /\bit\b|\bthat\b|\bthose\b|\bthem\b|\bconnect\b|\brelate\b|\balso\b/.test( | ||
| normalizedQuestion, | ||
| ); | ||
| return detectedIds.length === 0 && followUpCue; |
There was a problem hiding this comment.
The new shouldApplySessionContext gate only applies session reuse when a pronoun-like cue is present, which drops context for many valid follow-ups that omit IDs but also omit those cue words (for example, a second-turn question like "What constraints apply?"). Before this refactor, any no-ID follow-up reused session state; with this change, retrieval can regress to unrelated candidates and lose previously selected nodes in the same session.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This is an intentional design choice made during CodeRabbit review (see earlier thread). The previous behavior (return true for any ID-free query) was a concrete bug — it biased every standalone query with stale session state, even completely unrelated ones.
The current gate requires both: (a) no explicit IDs, (b) follow-up cue words (it, that, those, them, connect, relate, also), and (c) prior session state exists. A question like "What constraints apply?" without any cue words is treated as a standalone query, which is the safer default — it avoids silently injecting context from a prior unrelated question.
If real-world usage shows the cue list is too narrow, it can be expanded in a follow-up without reverting to the overly broad "any ID-free query reuses session" behavior.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/runtime/frontier-expander.ts (1)
85-121:⚠️ Potential issue | 🟠 MajorPersist the capped anchor set between hops.
selectedAnchorIdskeeps growing pastMAX_SELECTED_ANCHORS; onlyboundedAnchorIdsis capped. On the next iteration, Line 60 and Lines 77-78 still consult the unbounded working set, so role coverage and duplicate filtering can depend on anchors that callers will never receive in the final trace. Rebind the working set to the capped list after each hop, and record only the bounded delta inaddedAnchorIds.Possible fix
export function expandGrounding( question: string, candidates: TraceCandidate[], initialNodeIds: string[], initialAnchorIds: string[], frontierCandidates: FrontierCandidate[], frontierKeys: Set<string>, snapshot: Snapshot, ): GroundingResult { const graphExpansions: GraphExpansion[] = []; const selectedNodeIds = unique(initialNodeIds); - const selectedAnchorIds = unique(initialAnchorIds).slice(0, MAX_SELECTED_ANCHORS); + let selectedAnchorIds = unique(initialAnchorIds).slice(0, MAX_SELECTED_ANCHORS); const retrievalHops: RetrievalHop[] = []; const followedReferences: FollowedReference[] = []; const candidateScoreById = new Map( candidates.map((candidate) => [candidate.nodeId, candidate.score] as const), ); @@ - selectedNodeIds.push(...addedNodeIds); - selectedAnchorIds.push(...addedAnchorIds); + selectedNodeIds.push(...addedNodeIds); + const previousAnchorIds = selectedAnchorIds; + selectedAnchorIds = unique([...selectedAnchorIds, ...addedAnchorIds]).slice( + 0, + MAX_SELECTED_ANCHORS, + ); + const boundedAddedAnchorIds = selectedAnchorIds.filter( + (anchorId) => !previousAnchorIds.includes(anchorId), + ); @@ - const boundedAnchorIds = unique(selectedAnchorIds).slice(0, MAX_SELECTED_ANCHORS); sufficient = isGroundingSufficient( question, unique(selectedNodeIds), - boundedAnchorIds, + selectedAnchorIds, snapshot, ); retrievalHops.push({ iteration, reason: picked.reason, addedNodeIds, - addedAnchorIds, + addedAnchorIds: boundedAddedAnchorIds, sufficientAfter: sufficient, }); } - const finalAnchorIds = unique(selectedAnchorIds).slice(0, MAX_SELECTED_ANCHORS); return { selectedNodeIds: unique(selectedNodeIds), - selectedAnchorIds: finalAnchorIds, + selectedAnchorIds, retrievalHops, followedReferences, graphExpansions, sufficient, };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/runtime/frontier-expander.ts` around lines 85 - 121, selectedAnchorIds is never reduced to the capped set so subsequent hops consult anchors that won't be returned; after computing boundedAnchorIds = unique(selectedAnchorIds).slice(0, MAX_SELECTED_ANCHORS) rebind the working set to that capped list (e.g., selectedAnchorIds = boundedAnchorIds) so role coverage and duplicate filtering use the same anchors callers will receive, and update how addedAnchorIds are recorded so retrievalHops stores only the delta of the bounded set for that iteration (compute addedAnchorIds as the difference between the previous boundedAnchorIds and the new boundedAnchorIds before rebinding); apply this change around the isGroundingSufficient call and the retrievalHops push in the loop that builds graphExpansions/retrievalHops.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/runtime/frontier-expander.ts`:
- Around line 85-121: selectedAnchorIds is never reduced to the capped set so
subsequent hops consult anchors that won't be returned; after computing
boundedAnchorIds = unique(selectedAnchorIds).slice(0, MAX_SELECTED_ANCHORS)
rebind the working set to that capped list (e.g., selectedAnchorIds =
boundedAnchorIds) so role coverage and duplicate filtering use the same anchors
callers will receive, and update how addedAnchorIds are recorded so
retrievalHops stores only the delta of the bounded set for that iteration
(compute addedAnchorIds as the difference between the previous boundedAnchorIds
and the new boundedAnchorIds before rebinding); apply this change around the
isGroundingSufficient call and the retrievalHops push in the loop that builds
graphExpansions/retrievalHops.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e82ae78a-2814-4f36-94f9-6978c87b425f
📒 Files selected for processing (8)
src/mcp/tool-contracts.tssrc/runtime/answer-projector.tssrc/runtime/candidate-seeder.tssrc/runtime/frontier-expander.tssrc/runtime/query-engine.tssrc/runtime/query-normalizer.tssrc/runtime/synthesis-adapter.tssrc/runtime/types.ts
✅ Files skipped from review due to trivial changes (1)
- src/mcp/tool-contracts.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/runtime/query-normalizer.ts
- src/runtime/candidate-seeder.ts
- src/runtime/answer-projector.ts
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6ad642b973
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| const statusTerms = Object.keys(snapshot.indexes.statusIndex).filter((term) => | ||
| questionTokens.has(term), |
There was a problem hiding this comment.
Using questionTokens.has(term) only matches exact single-token statuses, so natural queries like “show drafts” or “show stubs” no longer detect draft/stub and therefore skip status-based candidate boosts in seedCandidates (detected.statusTerms feeds snapshot.indexes.statusIndex[...]). Before this refactor, status detection used substring matching and these plural forms were recognized; with the new logic retrieval can drift to unrelated nodes for status-focused questions.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid catch — the original code uses a hardcoded list ['draft', 'stable', 'stub', 'transitional'] with normalizedQuestion.includes(term) (substring matching), but the refactored code changed to Object.keys(snapshot.indexes.statusIndex) with questionTokens.has(term) (exact token matching). Both the key set and matching method diverged.
Fixed in 8055d0b: restored the original hardcoded token list with substring matching.
devin-ai-integration
Bot
force-pushed
the
devin/1776041613-split-query-engine
branch
from
8055d0b to
b53ec34
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
devin-ai-integration
Bot
force-pushed
the
devin/1776041613-split-query-engine
branch
from
32fed27 to
a49b7dd
Compare
devin-ai-integration
Bot
force-pushed
the
devin/1776041613-split-query-engine
branch
from
a49b7dd to
f877deb
Compare
Code ReviewOverviewSplits the 1614-LOC
Net +1362/-1110. The churn includes new module headers, so true code movement is larger than the diff count suggests. Strengths
Things to verify carefully before mergeCircular importsSix new modules that all reference types and likely each other. Shared stateThe old Tight coupling to #28 and #32
Performance regression riskSplitting a hot path across module boundaries can cost if Bun doesn't inline. The query path runs on every Testing before mergeRun Nits
RecommendationLand this with #28 and #32 in one coherent batch. Alone, it's a mechanical refactor with real review risk around boundary crossings and shared state. Together with the seed-rule extraction and the contract tests, the whole shape clicks into place. |
|
@venikman Verified all four concerns: 1. Circular imports — NoneClean DAG. Each stage imports only from shared utilities ( 2. Shared state — Clean pass-throughAll stages are free functions. 3. Tight coupling to #28 and #32Agreed — these three should land together. #28 moves heuristic seed rules to the snapshot (the seeder would consume 4. Performance regression riskModule boundary overhead in Bun is negligible for this case — all stages are synchronous function calls within a single LOC metric (nit)
|
Split the 1,614-line monolithic QueryEngine class into focused stage modules: 1. query-normalizer.ts — question normalization, ID/lexeme/route detection 2. candidate-seeder.ts — pattern/route/lexeme scoring, heuristic seeds, session context 3. candidate-ranker.ts — ranking, route-wins logic, initial node selection 4. frontier-expander.ts — bounded graph expansion with hop budget and role-coverage 5. answer-projector.ts — route/pattern/Part C answers, confidence, gaps 6. synthesis-adapter.ts — optional LLM synthesis handoff with deterministic fallback QueryEngine is now an orchestrator that wires stages together and exposes query(), trace(), inspect(), readDoc(), inspectAnchor(), expandCitations(). All 39 existing tests pass without modification. Closes #2 Co-Authored-By: Stanislau <nedbailov375426@gmail.com>
…query - answer-projector.ts: use Set + neighborEdges instead of full relationGraph scan for route and pattern relation filtering - frontier-expander.ts: use node.neighborEdges instead of scanning entire relationGraph in buildFrontier - candidate-seeder.ts: precompute normalizeForLookup(question) once outside the indexMap loop - constants.ts: centralize MAX_HOPS, MAX_SELECTED_ANCHORS, MAX_EXCLUDED, MAX_SYNTHESIS_SLICES retrieval limits - All stage modules now import shared constants from constants.ts Co-Authored-By: Stanislau <nedbailov375426@gmail.com>
…ules - synthesis-adapter: wrap isAvailable() in try block for fallback safety - candidate-seeder: require follow-up cues + prior selection for session reuse - query-normalizer: use token-aware status term matching via statusIndex keys - frontier-expander: enforce bounded anchor budget before sufficiency checks - answer-projector: use real sectionId anchors for Part C citations - query-engine: gate route projection on ranking.routeWins, not node scan - types/tool-contracts: add routeWins to TraceResult and traceResultSchema Co-Authored-By: Stanislau <nedbailov375426@gmail.com>
Co-Authored-By: Stanislau <nedbailov375426@gmail.com>
Rebind the working anchor set to the bounded slice after each hop so that duplicate filtering, role coverage, and sufficiency checks all operate on the same capped anchor set that callers will receive in the final trace. Co-Authored-By: Stanislau <nedbailov375426@gmail.com>
- shouldApplySessionContext: unconditionally apply session context when no explicit IDs are detected (matching original behavior) - Status term detection: use hardcoded token list with substring matching instead of dynamic index keys with exact token matching Co-Authored-By: Stanislau <nedbailov375426@gmail.com>
- shouldApplySessionContext: remove Boolean(lastSelectedRouteId) check that widened session applicability beyond original behavior (only lastSelectedNodeIds.length gates session context in original code) - answerPartCDrafts: restore citations to ['Part C catalog'] instead of computed sectionIds, matching original query-engine.ts behavior Co-Authored-By: Stanislau <nedbailov375426@gmail.com>
devin-ai-integration
Bot
force-pushed
the
devin/1776041613-split-query-engine
branch
from
f877deb to
e55ca6e
Compare
|
Rebased on main (post-#28 merge) and resolved conflicts:
Typecheck, lint, CI all green. |
What
Split the 1,614-line monolithic
QueryEngineclass into six focused retrieval stage modules.QueryEnginebecomes a slim orchestrator wiring stages together and exposingquery(),trace(),inspect(),readDoc(),inspectAnchor(), andexpandCitations().Why
Closes #2 — the monolithic
query-engine.tsmixed seven responsibilities (question interpretation, candidate seeding, ranking, frontier expansion, trace emission, answer projection, synthesis brokering) under one class. This split enables stage-local reasoning, independent testing, and clearer fault attribution.Type
feat— new capabilityfix— bug fixrefactor— code restructuringdocs— documentation onlychore— maintenance (deps, CI, cleanup)Changes
query-normalizer.ts— Question normalization, ID/lexeme/route/family/status detectioncandidate-seeder.ts— Scoring patterns, routes, lexemes, index descriptions, heuristic seeds, session contextcandidate-ranker.ts— Ranking, route-wins logic, initial node/anchor selectionfrontier-expander.ts— Bounded graph expansion with hop budget and role-coverage sufficiencyanswer-projector.ts— Route answers, pattern answers, Part C drafts, confidence, gapssynthesis-adapter.ts— Optional LLM synthesis handoff with deterministic fallbackquery-engine.ts— Rewritten as orchestrator only; wires the six stages togetherEach stage has typed input/output interfaces (
NormalizedQuery,SeedingResult,RankingResult,GroundingResult).Note: Rebased on main after PR #36 merge. Runtime module count baseline is now 15 (was 14, due to added
path-resolution.ts), target ≥20.Validation
bun run lintpasses locallybun run checkpasses locallybun run testpasses locally (41 existing + new stage tests)bun run buildsucceeds (if runtime/server code touched)bun run docs:buildsucceeds (if docs touched)Boundary check
FPF-spec.mdonly — no additional corpora addedsrc/mcp/tool-contracts.tsAgent metadata
Requested by: @venikman