feat: v2.3.0 — query-aware retrieval + query_context MCP tool (closes… - #11
Conversation
There was a problem hiding this comment.
Pull request overview
Implements SigMap v2.3.0 “query-aware retrieval”: rank context files by relevance to a free-text query, expose it via a new --query CLI flag and a new MCP tool query_context, and bump the project version/tests/changelog accordingly.
Changes:
- Added zero-dependency tokenizer + ranker modules for query-based relevance scoring.
- Added
--queryCLI mode and MCPquery_contexttool (8th MCP tool) to return ranked results. - Bumped versions to
2.3.0and updated integration tests + changelog.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| test/integration/retrieval.test.js | Adds integration coverage for tokenizer/ranker, CLI --query, and MCP query_context. |
| test/integration/mcp-v14.test.js | Updates MCP tool-count gate to 8 and asserts query_context exists. |
| test/integration/mcp-server.test.js | Updates MCP tool-count gate to 8 and asserts query_context exists. |
| test/integration/analyze.test.js | Updates --version expectation to 2.3.0. |
| src/retrieval/tokenizer.js | New tokenizer for query/symbol/path tokenization with stop-word removal. |
| src/retrieval/ranker.js | New relevance ranker + sig-index builder + table/JSON formatters. |
| src/mcp/tools.js | Adds MCP tool definition for query_context. |
| src/mcp/server.js | Wires query_context handler into MCP dispatch + bumps MCP server version. |
| src/mcp/handlers.js | Implements queryContext() MCP handler backed by the ranker. |
| src/config/defaults.js | Adds retrieval defaults (topK, recencyBoost). |
| package.json | Bumps package version to 2.3.0. |
| gen-context.js | Adds CLI --query mode and bundles new retrieval modules + version bump. |
| CHANGELOG.md | Documents v2.3.0 additions and validation gates. |
| * @returns {{ file: string, score: number, sigs: string[], tokens: number }[]} | ||
| */ | ||
| function rank(query, sigIndex, opts) { | ||
| if (!query || typeof query !== 'string') return []; |
There was a problem hiding this comment.
rank() claims to support empty queries (fallback to top-K by signature count), but the initial guard if (!query || typeof query !== 'string') return []; returns early for query === '', making the empty-query branch unreachable. Adjust the guard to only reject non-strings (or explicitly handle empty string before the guard) so rank('') follows the intended behavior.
| if (!query || typeof query !== 'string') return []; | |
| if (typeof query !== 'string') return []; |
| // Bonus: appears directly in a function/class/method name line | ||
| const nameLineMatch = sigs.some((sig) => { | ||
| const nt = tokenize(sig.replace(/[^a-zA-Z0-9_\s]/g, ' ')); | ||
| return nt.includes(qt); | ||
| }); | ||
| if (nameLineMatch) score += w.symbolMatch; |
There was a problem hiding this comment.
scoreFile() re-tokenizes every signature line inside nameLineMatch for each query token that hits sigTokenSet, which can become expensive on larger context files. Consider precomputing a per-file (or per-sig) token set for “name line” matching once per file, then reuse it during scoring.
| // Gate 2: tools/list returns 8 tools (v2.3+) | ||
| // ───────────────────────────────────────────────────────────── | ||
| test('tools/list returns exactly 7 tools', () => { | ||
| withTempProject((dir) => { | ||
| const [res] = mcpCall({ jsonrpc: '2.0', method: 'tools/list', id: 2 }, dir); | ||
| assert.ok(res.result, 'Should have result'); | ||
| assert.ok(Array.isArray(res.result.tools), 'tools should be array'); | ||
| assert.strictEqual(res.result.tools.length, 7); | ||
| assert.strictEqual(res.result.tools.length, 8); | ||
| const names = res.result.tools.map((t) => t.name); |
There was a problem hiding this comment.
The test description still says "returns exactly 7 tools" but the assertion now checks for 8 tools. Update the test name to match the new expectation to avoid confusion when reading failures.
| @@ -88,10 +88,11 @@ test('tools/list returns exactly 7 tools', () => { | |||
| const [res] = mcpCall({ jsonrpc: '2.0', method: 'tools/list', id: 1 }, dir); | |||
| assert.ok(res.result, 'Should have result'); | |||
| assert.ok(Array.isArray(res.result.tools), 'tools should be array'); | |||
| assert.strictEqual(res.result.tools.length, 7, `Expected 7 tools, got ${res.result.tools.length}`); | |||
| assert.strictEqual(res.result.tools.length, 8, `Expected 8 tools, got ${res.result.tools.length}`); | |||
| const names = res.result.tools.map((t) => t.name); | |||
There was a problem hiding this comment.
The test description still says "returns exactly 7 tools" but the assertion now checks for 8 tools. Update the test name to match the new expectation to avoid confusion when reading failures.
| */ | ||
|
|
||
| const assert = require('assert'); | ||
| const fs = require('fs'); |
There was a problem hiding this comment.
fs is required but never used in this test file. Consider removing the unused import to keep the test clean (and to satisfy any linting/static checks).
| const fs = require('fs'); |
| const topK = topIdx >= 0 ? Math.min(Math.max(1, parseInt(args[topIdx + 1], 10) || 10), 25) | ||
| : ((config && config.retrieval && config.retrieval.topK) || 10); | ||
| const recencyBoost = (config && config.retrieval && config.retrieval.recencyBoost) || 1.5; | ||
| const results = rank(query, index, { topK, recencyBoost }); |
There was a problem hiding this comment.
recencyBoost is read from config and passed into rank(), but the current ranker only applies a recency multiplier when opts.recencySet is provided (and recencySet is never computed/passed here). As a result, config.retrieval.recencyBoost is effectively a no-op. Either compute a recencySet (e.g., from recent git commits) and pass it into rank(), or remove/defers the recencyBoost plumbing until the signal is actually supported.
| const results = rank(query, index, { topK, recencyBoost }); | |
| let recencySet = null; | |
| try { | |
| const { execFileSync } = require('child_process'); | |
| const gitOutput = execFileSync( | |
| 'git', | |
| ['log', '--name-only', '--pretty=format:', '-n', '20'], | |
| { cwd, encoding: 'utf8', stdio: ['ignore', 'pipe', 'ignore'] } | |
| ); | |
| const recentFiles = gitOutput | |
| .split(/\r?\n/) | |
| .map((line) => line.trim()) | |
| .filter(Boolean) | |
| .map((line) => line.replace(/\\/g, '/')); | |
| if (recentFiles.length) { | |
| recencySet = new Set(recentFiles); | |
| } | |
| } catch (_) { | |
| recencySet = null; | |
| } | |
| const rankOpts = recencySet ? { topK, recencyBoost, recencySet } : { topK, recencyBoost }; | |
| const results = rank(query, index, rankOpts); |
| const topK = topIdx >= 0 ? Math.min(Math.max(1, parseInt(args[topIdx + 1], 10) || 10), 25) | ||
| : ((config && config.retrieval && config.retrieval.topK) || 10); |
There was a problem hiding this comment.
--top 0 (or topK: 0) currently gets coerced to the default 10 because of parseInt(...) || 10. Since you already clamp to [1, 25], consider avoiding || 10 and instead default only on NaN/missing input, so 0 correctly clamps to 1 (or triggers a validation error).
| const topK = topIdx >= 0 ? Math.min(Math.max(1, parseInt(args[topIdx + 1], 10) || 10), 25) | |
| : ((config && config.retrieval && config.retrieval.topK) || 10); | |
| const parsedArgTopK = topIdx >= 0 ? parseInt(args[topIdx + 1], 10) : NaN; | |
| const configTopKRaw = (config && config.retrieval) ? config.retrieval.topK : undefined; | |
| const parsedConfigTopK = (configTopKRaw === undefined || configTopKRaw === null) ? NaN : parseInt(configTopKRaw, 10); | |
| const requestedTopK = !Number.isNaN(parsedArgTopK) | |
| ? parsedArgTopK | |
| : (!Number.isNaN(parsedConfigTopK) ? parsedConfigTopK : 10); | |
| const topK = Math.min(Math.max(1, requestedTopK), 25); |
| const index = buildSigIndex(cwd); | ||
| if (index.size === 0) return 'No signatures indexed. Run: node gen-context.js'; | ||
|
|
||
| const topK = Math.min(Math.max(1, parseInt(args.topK, 10) || 10), 25); |
There was a problem hiding this comment.
If a client passes topK: 0, parseInt(args.topK, 10) || 10 coerces it to 10 rather than clamping to 1. Since you clamp to [1, 25] anyway, default only when the parsed value is NaN/missing so invalid values behave predictably.
| const topK = Math.min(Math.max(1, parseInt(args.topK, 10) || 10), 25); | |
| const parsedTopK = parseInt(args.topK, 10); | |
| const topK = Math.min(Math.max(1, Number.isNaN(parsedTopK) ? 10 : parsedTopK), 25); |
… #8) (#10)