test(router): add explicit coverage for llm → semantic routing#129
test(router): add explicit coverage for llm → semantic routing#129
Conversation
|
Piggyback note for refinement governance update (applies to this PR context): We updated the refiner agent prompt to enforce a KISS project-management heuristic for tightly-coupled small scopes. Source update:
Key rule now in prompt:
Why this is relevant here:
|
Co-authored-by: mfittko <326798+mfittko@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR adds explicit test coverage for the llm → semantic routing path in the query router, closing a gap where the LLM could return "semantic" strategy on low-confidence rule paths but had no regression protection. The changes include two new test cases and unrelated agent configuration updates.
Changes:
- Added Ollama-path test verifying LLM can return semantic strategy on entity_pattern rule path
- Added OpenAI-provider parity test for semantic strategy via chat-completions API
- Updated refiner agent configuration with RFC gate workflow and KISS heuristic (unrelated to PR purpose)
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| api/src/services/query-router.test.ts | Added two new test cases for llm → semantic routing path: one for Ollama provider and one for OpenAI provider, both testing low-confidence entity_pattern query handling |
| .github/agents/refiner.agent.md | Added RFC gate workflow section and KISS project-management heuristic to refiner agent configuration (appears unrelated to test coverage work) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -65,6 +79,23 @@ If critical input is missing, ask concise clarification questions first. | |||
| - Add progress comments whenever substantive updates are made. | |||
| - Summarize decisions and rationale in each issue comment. | |||
|
|
|||
| ## KISS Project-Management Heuristic | |||
|
|
|||
| Default to the simplest tracking structure that preserves clarity. | |||
|
|
|||
| - **Do not create sub-issues** when work is tightly coupled, likely delivered in one PR, and estimated to stay small (rule of thumb: **~500 LOC or less** total change across touched files). | |||
| - In those cases, keep a **single issue** with: | |||
| - one clear implementation approach, | |||
| - required vs preferred checklist items, | |||
| - one AC/DoD/Non-goal matrix. | |||
| - Only split into sub-issues when at least one of these is true: | |||
| - distinct owners or teams are expected, | |||
| - independent sequencing/blocking relationships are required, | |||
| - meaningful parallelization is possible without merge contention, | |||
| - scope is large/uncertain enough that one issue reduces clarity. | |||
|
|
|||
| When uncertain, choose the single-issue path first and justify any split explicitly in the decision log. | |||
|
|
|||
There was a problem hiding this comment.
The PR description states this is a test-only change to add coverage for llm → semantic routing in the query router test suite. However, this file contains changes to the refiner agent configuration that add RFC gate workflow steps and a KISS project-management heuristic.
These changes appear unrelated to the stated purpose of the PR (adding test coverage) and should likely be in a separate PR focused on agent configuration improvements. Including unrelated changes makes it harder to review and understand the scope of each change.
Consider removing these agent configuration changes from this PR and submitting them separately if they are needed.
| If unresolved foundational decisions are present, refinement is not complete: it must stop at RFC issue(s) and explicitly mark deeper refinement as blocked until RFCs are resolved. | ||
|
|
There was a problem hiding this comment.
This change appears unrelated to the test coverage work described in the PR. The refiner agent configuration updates should be submitted in a separate PR if they are needed.
| - Do not add unnecessary code examples. | ||
| - Prefer generic, technology-agnostic requirement language unless stack specifics are essential. | ||
| - Keep issue comments factual, decision-oriented, and audit-friendly. No newline at end of file | ||
| - Keep issue comments factual, decision-oriented, and audit-friendly. |
There was a problem hiding this comment.
This line change appears to be whitespace-only or a minor formatting adjustment unrelated to the test coverage work. If this is part of broader agent configuration changes, those should be in a separate PR.
The router test suite covered
llm → metadata/graphand fallback/circuit behavior but had no assertion thatsemanticis accepted when the LLM returns it on a low-confidence rule path — leaving that branch silently unguarded against regressions.Changes
api/src/services/query-router.test.ts— two new tests, no production code touched:"who is AuthService") hits rule 5 at conf 0.7, mocked LLM responds{"strategy":"semantic","confidence":0.8}→ assertsmethod === "llm",strategy === "semantic",confidence === 0.8,rule === "entity_pattern"choices[0].message.contentshape → same outcome assertionsOriginal prompt
This section details on the original issue you should resolve
<issue_title>test(router): add explicit coverage for non-explicit LLM routing to semantic</issue_title>
<issue_description>## Summary
Add explicit automated coverage for non-explicit
llm -> semanticrouting inapi/src/services/query-router.test.ts, with one required generic path test and one preferred OpenAI parsing parity test.Problem Statement
Current router tests cover
llm -> metadata/graph, fallback/circuit behavior, and OpenAI model-selection behavior, but do not directly assert thatsemanticis accepted when returned by LLM on a low-confidence rule path.Without this, regressions could silently reject or bias away from semantic while still passing existing tests.
Scope
In scope
api/src/services/query-router.test.ts.llm -> semanticassertion.llm -> semanticparsing assertion.Out of scope
Implementation Approach (Single Issue)
Use one tightly-coupled implementation pass in the same test file:
Add required generic/Ollama-path regression test:
strategy="semantic",confidence=0.8.result.method === "llm"result.strategy === "semantic"result.confidence === 0.8Add preferred OpenAI provider parity test:
strategy="semantic",confidence=0.8.method,strategy,confidence).Dependencies
Validation Plan
cd api && npm test -- src/services/query-router.test.tsRisks and Mitigations
method === "llm".choices[].message.content).Decision Log (assumption -> resolution)
Proposed Definition of Done
llm -> semanticon low-confidence rule path.api/src/services/query-router.test.tspasses locally/CI with added tests.AC/DoD/Non-goal Coverage Matrix
llm -> semanticpath regressesapi/src/services/query-router.test.tsllm -> semanticon low-confidence rule path.api/src/services/query-router.test.tspasses locally/CI with added tests.💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.