SqliteStore backend + annotation, audit, and query result cache tools by data-douser · Pull Request #169 · advanced-security/codeql-development-mcp-server

data-douser · 2026-03-25T11:53:15Z

Relates to #163.
Resolves #165.

📝 Update Information

Primitive Details

Type: Tool
Name: annotation_*, audit_*, query_results_cache_*, SqliteStore
Update Category: Feature Enhancement, Bug Fix, Performance

⚠️ CRITICAL: PR SCOPE VALIDATION

This PR is for updating an existing MCP server primitive and must ONLY include these file types:

✅ ALLOWED FILES:

Server implementation files (server/src/**/*.ts)
Updated primitive implementations
Modified registration files (server/src/tools/*.ts)
Updated or new test files (server/test/**/*.ts)
Documentation updates (README.md, server docs)
Updated type definitions (server/src/types/*.ts)
Modified supporting library files (server/src/lib/*.ts)
Configuration updates if needed (package.json, tsconfig.json)

🚫 FORBIDDEN FILES:

Files unrelated to the primitive update
Temporary or test output files
IDE configuration files
Log files or debug output
Analysis or summary files

Rationale: This PR should contain only the files necessary to update and test the primitive.

🚨 PRs that include forbidden files will be rejected and must be revised.

🛑 MANDATORY PR VALIDATION CHECKLIST

BEFORE SUBMITTING THIS PR, CONFIRM:

ONLY server implementation files are included
NO temporary or output files are included
NO unrelated configuration files are included
ALL existing tests continue to pass
NEW functionality is properly tested

Impact Scope: Extensive changes

Update Metadata

Breaking Changes: Yes — annotation_search parameter renamed from query to search; query_results_cache_retrieve grep parameter removed
API Compatibility: Changed (see Breaking Changes below)
Performance Impact: Improved — FTS4 indexed annotation search replaces full-table LIKE scan; debounced flush; true LRU metadata cache (max 256 entries, re-insert on hit)

🎯 Changes Description

Current Behavior

Session persistence backed by lowdb (JSON file)
No annotation, audit, or query result cache tools
annotation_search exposed a query parameter wired to a LIKE-based substring scan — no indexing, slow at scale
query_results_cache_retrieve included a grep parameter for filtering cached content by text pattern; SARIF format only used result-level retrieval when resultIndices or fileFilter was explicitly supplied
extractQueryMetadata performed two separate statSync + readFileSync calls (TOCTOU race) and leaked the file descriptor on cache hits
Metadata cache eviction was FIFO by insertion time (not true LRU by access)
getCacheSarifSubset fallback used maxResults as maxLines and returned line counts as totalResults/returnedResults, mixing result and line concepts
result-processor.ts had duplicate import statements from ./cli-executor and no unit tests

Updated Behavior

Session persistence migrated to SqliteStore — a unified SQLite backend (via sql.js asm.js build) with three tables: sessions, annotations, query_result_cache
14 new opt-in MCP tools registered under ENABLE_ANNOTATION_TOOLS=true:
- Annotation tools: annotation_create, annotation_list, annotation_search, annotation_delete
- Audit tools: audit_store_findings, audit_list_findings, audit_add_notes, audit_clear_repo
- Cache tools: query_results_cache_lookup, query_results_cache_retrieve, query_results_cache_clear, query_results_cache_compare
annotation_search now uses SQLite FTS4 (annotations_fts virtual table, unicode61 tokenizer) for case-insensitive, token-based full-text search; parameter renamed query → search; prefix matching via * suffix supported
query_results_cache_retrieve has the grep parameter removed entirely; SARIF format always routes through getCacheSarifSubset so maxResults and result-level filters apply consistently
extractQueryMetadata opens a single file descriptor, reads and fstats through it in a try/finally block that always calls closeSync(fd) — no TOCTOU, no FD leak
Metadata cache now uses true LRU eviction: cache hits re-insert the key (delete + set) to refresh Map iteration order; eviction targets the least-recently-used entry
getCacheSarifSubset fallback now uses a dedicated FALLBACK_MAX_LINES = 500 constant (independent of maxResults) and returns totalResults: 0, returnedResults: 0 to signal non-SARIF content, avoiding the results/lines concept mix
result-processor.ts import from ./cli-executor consolidated into a single alphabetically-ordered statement
SqliteStore.flush() uses write-to-temp + atomic rename on POSIX; on Windows the fallback is an in-place overwrite (documented as not crash-safe)
Unit tests added for result-processor.ts covering computeQueryCacheKey, getDefaultExtension, interpretBQRSFile, and processQueryRunResults

Motivation

Replace lowdb with a proper relational store that supports indexed search, structured result caching, and annotation/audit workflows. Address correctness bugs (TOCTOU, FD leak, timestamp format mismatch, Windows rename failure, FIFO-not-LRU eviction, SARIF fallback semantics) and API inconsistencies (grep in cache tools, query vs search naming, duplicate imports) surfaced during code review.

🔄 Before vs. After Comparison

Functionality Changes

// BEFORE: annotation_search used LIKE substring scan with 'query' param
server.tool('annotation_search', 'Substring search … SQL LIKE matching.', {
  query: z.string().describe('Search term — matched as a substring …'),
  …
}, async ({ query, … }) => store.listAnnotations({ search: query, … }));

// AFTER: annotation_search uses FTS4 with 'search' param
server.tool('annotation_search', 'Full-text search … SQLite FTS token-based MATCH …', {
  search: z.string().describe('Full-text search query … SQLite FTS MATCH syntax; use * for prefix matching.'),
  …
}, async ({ search, … }) => store.listAnnotations({ search, … }));

API Changes

// annotation_search — BEFORE:
const oldSchema = { query: z.string(), category?: string, limit?: number };

// annotation_search — AFTER:
const newSchema = { search: z.string(), category?: string, limit?: number };

// query_results_cache_retrieve — BEFORE (included grep):
const oldSchema = {
  cacheKey: z.string(),
  lineRange?: [number, number],
  resultIndices?: [number, number],
  fileFilter?: string,
  grep?: string,          // REMOVED
  maxLines?: number,
  maxResults?: number,
};

// query_results_cache_retrieve — AFTER (grep removed; SARIF always uses getCacheSarifSubset):
const newSchema = {
  cacheKey: z.string(),
  lineRange?: [number, number],
  resultIndices?: [number, number],
  fileFilter?: string,
  maxLines?: number,
  maxResults?: number,
};

Output Format Changes

// query_results_cache_retrieve SARIF output — BEFORE (only when resultIndices or fileFilter provided):
{ "totalResults": 42, "returnedResults": 10, "truncated": true, "results": [ … ] }

// query_results_cache_retrieve SARIF output — AFTER (always for SARIF format; maxResults always applied):
{ "totalResults": 42, "returnedResults": 10, "truncated": true, "results": [ … ] }

// getCacheSarifSubset fallback (non-SARIF content) — BEFORE:
{ "totalResults": <lineCount>, "returnedResults": <lineCount>, "truncated": bool }

// getCacheSarifSubset fallback (non-SARIF content) — AFTER:
{ "totalResults": 0, "returnedResults": 0, "truncated": bool }

🧪 Testing & Validation

Test Coverage Updates

Existing Tests: All 1098 existing tests continue to pass
New Test Cases: sqlite-store.test.ts (32 tests), result-processor.test.ts (23 new tests) covering computeQueryCacheKey, getDefaultExtension, interpretBQRSFile, processQueryRunResults
Regression Tests: True LRU eviction test; maxLines-only subset test replaces removed grep test; enableAnnotationTools consistent in all monitoring-tools mocks
Edge Case Tests: Inclusive resultIndices range clamping; Windows rename fallback; FD always closed on cache hit; SARIF fallback returns neutral result fields

Validation Scenarios

Backward Compatibility: All pre-existing session-based tests pass; monitoring tools tests updated for new SqliteStore construction pattern
New Functionality: FTS4 search returns correct results for token-based and prefix queries; SARIF always applies maxResults cap; LRU eviction reorders Map on hit
Error Handling: JSON.parse fallback in query_results_cache_retrieve for non-SARIF content; safe closeSync in extractQueryMetadata; SARIF fallback returns totalResults: 0 to signal non-SARIF
Performance: Debounced 200 ms flush coalesces rapid writes; FTS4 indexed search avoids full-table LIKE scan; true LRU bounded cache (max 256 entries)

Test Results

Unit Tests: All pass (1098/1098)
Integration Tests: Validated with existing integration test suite
Manual Testing: Validated with real query and annotation scenarios
Performance Testing: No regressions detected

📋 Implementation Details

Files Modified

Core Implementation: server/src/lib/sqlite-store.ts — SqliteStore with FTS4, true LRU metadata cache, debounced flush, atomic rename, idempotent initialize, accurate Windows fallback comment, fixed SARIF fallback semantics
Core Implementation: server/src/lib/query-results-evaluator.ts — true LRU cache (delete + re-insert on hit), FD-safe metadata extraction
Supporting Libraries: server/src/lib/result-processor.ts — consolidated duplicate cli-executor imports; server/src/lib/database-resolver.ts; server/src/lib/query-resolver.ts; server/src/lib/codeql-version.ts
Type Definitions: server/src/types/sql-js.d.ts
Tool Registration: server/src/tools/annotation-tools.ts — annotation_search uses search param and FTS description; server/src/tools/audit-tools.ts; server/src/tools/cache-tools.ts — grep removed, SARIF always uses getCacheSarifSubset
Tests: server/test/src/lib/sqlite-store.test.ts; server/test/src/lib/result-processor.test.ts (new, 23 tests); server/test/src/tools/monitoring-tools.test.ts — enableAnnotationTools consistent across all mocks; server/test/src/lib/session-data-manager.test.ts

Code Changes Summary

Algorithm Improvements: True LRU eviction (delete + re-insert on hit); FTS4 replaces LIKE scan; SARIF always uses result-level retrieval; SARIF maxResults always applied; SARIF fallback uses separate FALLBACK_MAX_LINES constant
Error Handling: FD leak fixed (try/finally + closeSync); Windows rename fallback documented; JSON.parse try/catch for non-SARIF fallback content; SARIF fallback returns neutral result fields
Performance Optimization: Debounced flush (200 ms); true LRU bounded metadata cache; FTS4 indexed annotation search
Type Safety: Full TypeScript coverage; grep removed from getCacheContentSubset options
Input Validation: Zod schemas updated — annotation_search uses search; query_results_cache_retrieve drops grep
Output Format: SARIF subset always returned for SARIF format; consistent JSON envelope; SARIF fallback uses neutral fields

Dependencies

Updated Dependencies: lowdb removed, sql.js added (asm.js build — zero native/WASM dependency, bundles inline)

🔍 Quality Improvements

Bug Fixes

Issue: extractQueryMetadata leaked file descriptors on cache hit and had a TOCTOU race (stat then read)
Root Cause: Early return before closeSync; separate statSync/readFileSync calls
Solution: Single openSync with try/finally block containing closeSync(fd) in all exit paths
Prevention: Code review guideline to avoid stat-then-read; enforced by repo instructions
Issue: Metadata cache described as LRU but implemented as FIFO (Map.get does not update insertion order)
Root Cause: Cache hit returned early without re-inserting the key
Solution: Cache hit now does metadataCache.delete(queryPath); metadataCache.set(queryPath, cached) to refresh Map order
Prevention: Updated comment and unit test validate true LRU behaviour
Issue: getCacheSarifSubset fallback mixed "results" and "lines" concepts — passed maxResults as maxLines and returned line counts as totalResults/returnedResults
Root Cause: Single fallback path reused SARIF-specific parameter names for line-based fallback
Solution: Fallback now uses FALLBACK_MAX_LINES = 500 and returns totalResults: 0, returnedResults: 0 to signal non-SARIF content
Prevention: Clearly separate SARIF-concept fields from line-concept fields
Issue: flush() could corrupt the DB on Windows because renameSync fails when destination exists; comment claimed crash-safe for all platforms
Root Cause: fs.renameSync is not atomic-replace on Windows
Solution: Catch rename errors and fall back to writeFileSync directly on the target path; comment updated to accurately state the Windows fallback is not crash-safe
Prevention: Documented platform-specific guarantees in JSDoc
Issue: annotation_search parameter named query but store expected search; tool description said "LIKE" but FTS4 was used
Root Cause: API/documentation mismatch introduced when FTS4 replaced LIKE
Solution: Renamed to search, updated description to FTS semantics
Issue: query_results_cache_retrieve included grep for filtering cached content by text pattern; MCP tools must be self-sufficient
Root Cause: grep-based filtering concept leaked into the MCP tool API
Solution: Removed grep entirely; SARIF format now always uses result-level retrieval
Issue: result-processor.ts had two separate import statements from ./cli-executor
Root Cause: Import added incrementally without consolidating
Solution: Single alphabetically-ordered import statement
Issue: monitoring-tools.test.ts had one getConfig mock missing enableAnnotationTools, causing type inconsistency
Root Cause: Field added later without updating all mocks
Solution: enableAnnotationTools: false added to all four mocked config objects

Performance Improvements

True LRU annotation metadata cache: O(log n) Map re-insertion on hit; eviction removes least-recently-used entry
FTS4 annotation search: O(log n) token lookup vs O(n) full-table LIKE scan
Debounced flush: 200 ms debounce coalesces rapid writes into a single disk write

Code Quality Enhancements

Readability: Consolidated duplicate fs imports in query-results-evaluator.ts; consolidated duplicate cli-executor imports in result-processor.ts; removed grep concept from cache tool API
Maintainability: FTS triggers keep index in sync automatically; backfill migration handles schema upgrades; SARIF fallback semantics clearly separated
Testability: Per-test isolated temp dirs via createProjectTempDir; 23 new result-processor.test.ts tests; all monitoring-tools mocks consistent
Reusability: SqliteStore cleanly separates sessions, annotations, and cache into typed methods

🔗 References

Related Issues/PRs

Related PRs: Part of seclab-taskflow-integration: Multi-repo CodeQL analysis, annotation store, and query result caching #163

Validation Materials

Test Cases: server/test/src/lib/sqlite-store.test.ts (32 tests), server/test/src/lib/result-processor.test.ts (23 tests)
FTS4 validation: Verified unicode61 tokenizer, trigger-based sync, and count-based backfill migration against sql.js asm.js build
LRU validation: Verified delete + re-insert on hit produces correct eviction order under max-size pressure

🚀 Compatibility & Migration

Backward Compatibility

Breaking Changes: See below

Breaking Changes

Changes Made:

annotation_search: parameter renamed from query to search
query_results_cache_retrieve: grep parameter removed entirely
getCacheSarifSubset fallback: totalResults/returnedResults now return 0 for non-SARIF content (was line count)
lowdb replaced by sql.js; existing session data (ephemeral JSON) is not migrated

Migration Guide:

Replace annotation_search({ query: "term" }) with annotation_search({ search: "term" })
Remove any grep parameter from query_results_cache_retrieve calls; use lineRange/maxLines for line-based formats and resultIndices/fileFilter/maxResults for SARIF
If calling getCacheSarifSubset directly, check for totalResults === 0 to detect the non-SARIF fallback path

Timeline:

Effective immediately on merge

API Evolution

Enhanced Parameters: resultIndices, fileFilter, maxResults now consistently applied for all SARIF retrievals
Improved Responses: SARIF always returns structured { totalResults, returnedResults, truncated, results } envelope; fallback returns totalResults: 0 to signal non-SARIF
Better Error Messages: JSON.parse fallback returns raw text instead of throwing
Maintained Contracts: Core session storage, annotation CRUD, and cache CRUD contracts preserved

👥 Review Guidelines

For Reviewers

Please verify:

Testing Instructions

# Full test suite
npm install
npm run build
npm test

# SqliteStore-specific tests
npm test -- --grep "SqliteStore"

# result-processor tests
npm test -- --grep "result-processor"

# Code quality checks
npm run lint
npm run format

Validation Checklist

Regression Testing: All 1098 unit tests pass
New Feature Testing: FTS4 search, true LRU eviction, SARIF always-retrieve, maxResults cap validated
Performance Testing: No regressions; flush debounced; LRU bounded and correct
Error Testing: FD close in all paths; Windows rename fallback documented; JSON.parse fallback; SARIF fallback neutral fields
Integration Testing: Works with existing monitoring and session tools
Documentation Review: Tool descriptions accurate for FTS semantics and removed parameters

📊 Impact Assessment

Performance Impact

Memory Usage: Bounded true-LRU metadata cache (max 256 entries); sql.js asm.js bundled inline
Execution Time: FTS4 O(log n) vs LIKE O(n); debounced flush reduces I/O; LRU re-insert on hit is O(1)
Throughput: Rapid writes coalesced into single disk write per 200 ms window

Server Impact

Startup Time: No significant impact
Runtime Stability: Idempotent initialize(); FD always closed; timer cleared on close()
Resource Usage: Reasonable; true LRU cache bounded at 256 entries
Concurrent Usage: Single-threaded Node.js; no additional locking needed

AI Assistant Impact

Enhanced Accuracy: FTS4 returns semantically relevant annotations; SARIF consistently applies maxResults; LRU keeps frequently-used query metadata hot
Better Coverage: 14 new opt-in tools for annotation, audit, and cache workflows
Improved Reliability: No FD leak; no TOCTOU race; Windows-safe persistence documented; SARIF fallback unambiguous
Enhanced User Experience: Cleaner API without grep; consistent SARIF envelope; unambiguous fallback fields

🔄 Deployment Strategy

Rollout Considerations

Safe Deployment: All new tools opt-in via ENABLE_ANNOTATION_TOOLS=true
Monitoring: Appropriate logging on store initialization and tool registration
Rollback Plan: Revert to previous commit; session data is ephemeral

Post-Deployment Validation

Monitoring: Log warnings if CodeQL CLI version differs from target
Error Tracking: Watch for FTS MATCH syntax errors from callers using old query parameter; watch for callers passing grep to cache retrieve

Update Methodology: This update follows best practices:

✅ Comprehensive backward compatibility analysis
✅ Thorough testing of all changes (1098 unit tests passing)
✅ Performance impact assessment (true LRU, FTS4, debounced flush)
✅ Clear documentation of breaking changes and migration guide
✅ Robust error handling improvements (FD leak, Windows rename, JSON.parse, SARIF fallback)
✅ Maintained code quality standards (consolidated imports, removed grep, consistent mocks)

✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.

github-actions · 2026-03-25T11:53:31Z

Dependency Review

The following issues were found:

✅ 0 vulnerable package(s)
✅ 0 package(s) with incompatible licenses
✅ 0 package(s) with invalid SPDX license definitions
⚠️ 1 package(s) with unknown licenses.

See the Details below.

Snapshot Warnings

⚠️: No snapshots were found for the head SHA 07a87a7.

Ensure that dependencies are being submitted on PR branches and consider enabling retry-on-snapshot-warnings. See the documentation for more information and troubleshooting advice.

License Issues

server/package.json

Package	Version	License	Issue Type
sql.js	^1.14.1	Null	Unknown License

OpenSSF Scorecard

Package

Version

Score

Details

npm/sql.js

1.14.1

🟢 3.4

Details

Check	Score	Reason
Code-Review	⚠️ 2	Found 6/30 approved changesets -- score normalized to 2
Dangerous-Workflow	🟢 10	no dangerous workflow patterns detected
Packaging	⚠️ -1	packaging workflow not detected
Binary-Artifacts	🟢 10	no binaries found in the repo
Maintained	🟢 4	3 commit(s) and 2 issue activity found in the last 90 days -- score normalized to 4
Token-Permissions	⚠️ 0	detected GitHub workflow tokens with excessive permissions
Pinned-Dependencies	🟢 6	dependency not pinned by hash detected -- score normalized to 6
CII-Best-Practices	⚠️ 0	no effort to earn an OpenSSF best practices badge detected
Security-Policy	⚠️ 0	security policy file not detected
Fuzzing	⚠️ 0	project is not fuzzed
License	🟢 9	license file detected
Branch-Protection	⚠️ 0	branch protection not enabled on development/release branches
Signed-Releases	⚠️ 0	Project has not signed or included provenance with any releases.
SAST	⚠️ 0	SAST tool is not run on all commits -- score normalized to 0

npm/sql.js

^1.14.1

Unknown

Scanned Files

package-lock.json
server/package.json

Copilot

Pull request overview

This PR migrates the server’s persistence layer from lowdb to a unified sql.js-backed SqliteStore, and introduces opt-in MCP tools for annotations, audit workflows, and query result caching while refactoring CLI-related logic into smaller modules.

Changes:

Replace lowdb session persistence with SqliteStore (sessions + annotations + query result cache tables).
Add new opt-in MCP tools: annotation_*, audit_*, and query_results_cache_*.
Refactor query/database resolution and query-result processing (interpretation + auto-caching) into dedicated modules.

Reviewed changes

Copilot reviewed 18 out of 20 changed files in this pull request and generated 9 comments.

Show a summary per file

File	Description
server/test/src/tools/monitoring-tools.test.ts	Updates monitoring-tool tests to account for async store initialization and new config flag.
server/test/src/lib/sqlite-store.test.ts	Adds unit tests for `SqliteStore` sessions, annotations, persistence, and cache behaviors.
server/test/src/lib/session-data-manager.test.ts	Updates tests to await async initialization after migrating persistence backend.
server/src/types/sql-js.d.ts	Adds minimal typings for `sql.js` asm.js import + core DB surface.
server/src/types/monitoring.ts	Adds `enableAnnotationTools` config flag (default false).
server/src/tools/cache-tools.ts	Introduces `query_results_cache_*` tools (lookup/retrieve/clear/compare).
server/src/tools/audit-tools.ts	Introduces `audit_*` tools layered on annotations.
server/src/tools/annotation-tools.ts	Introduces `annotation_*` tools for CRUD + search.
server/src/lib/sqlite-store.ts	Implements the new unified SQLite persistence backend + cache subset retrieval.
server/src/lib/session-data-manager.ts	Migrates session persistence from lowdb to `SqliteStore`; adds `getStore()`.
server/src/lib/result-processor.ts	Extracts query result interpretation/evaluation and auto-cache pipeline.
server/src/lib/query-results-evaluator.ts	Adds query metadata caching with mtime-based invalidation.
server/src/lib/query-resolver.ts	Extracts query-path resolution via `codeql resolve queries`.
server/src/lib/database-resolver.ts	Extracts database-path resolution and caches results in-memory.
server/src/lib/codeql-version.ts	Adds target/actual CodeQL version tracking and mismatch warning.
server/src/lib/cli-tool-registry.ts	Integrates extracted resolvers/result processor and fixes output propagation + predicate handling.
server/src/lib/cli-executor.ts	Wires version detection into startup validation; re-exports version helpers.
server/src/codeql-development-mcp-server.ts	Registers new annotation/audit/cache tools at startup and initializes session manager.
server/package.json	Removes `lowdb`, adds `sql.js`.
package-lock.json	Updates lockfile to reflect dependency swap (remove lowdb/steno, add sql.js).

Comments suppressed due to low confidence (2)

server/src/lib/sqlite-store.ts:425

updateAnnotation sets updated_at = datetime('now'), which produces a different string format than the ISO timestamps written in createAnnotation. This can cause incorrect ordering when sorting by updated_at. Consider updating this to use the same format as inserts.

    setClauses.push("updated_at = datetime('now')");

    const db = this.ensureDb();
    db.run(
      `UPDATE annotations SET ${setClauses.join(', ')} WHERE id = $id`,
      params as Record<string, string | number | null>,
    );

server/src/lib/session-data-manager.ts:49

Docstring says “sql.js WASM init is async”, but this store uses the asm.js build. Please adjust the wording so it’s accurate (async init is still true, but it’s not specifically WASM).

  /**
   * Initialize the database and ensure it's properly set up.
   * Must be awaited before any session operations (sql.js WASM init is async).
   */

server/test/src/lib/sqlite-store.test.ts

server/src/lib/sqlite-store.ts

server/src/lib/session-data-manager.ts

server/test/src/tools/monitoring-tools.test.ts

server/src/lib/query-results-evaluator.ts

server/src/lib/sqlite-store.ts

server/src/tools/audit-tools.ts

server/src/tools/cache-tools.ts

- Fix TOCTOU in query-results-evaluator (openSync/fstatSync/readFileSync(fd)) - Use datetime('now') consistently for annotation timestamps - Debounce flush() with 200ms coalescing via scheduleFlush() - Fix resultIndices to inclusive [start, end] range with clamping - Fix WASM→asm.js comments in session-data-manager - Fix audit-tools header comment (no separate ENABLE_AUDIT_TOOLS flag) - Add separate maxResults parameter for SARIF in cache-tools - Use createProjectTempDir() in all test files - Fix monitoring-tools test init order (mock before initialize) - Add store.close() in session-data-manager test afterEach

Copilot

Pull request overview

Copilot reviewed 19 out of 23 changed files in this pull request and generated 6 comments.

server/src/lib/query-results-evaluator.ts

server/src/lib/sqlite-store.ts

server/test/src/tools/monitoring-tools.test.ts

- Bound metadataCache to 256 entries with oldest-first eviction - Make SqliteStore.initialize() idempotent (close existing db first) - Fix TOCTOU in initialize(): try readFileSync directly instead of existsSync - Atomic flush: write to temp file + renameSync to prevent corruption - Clarify annotation_search uses substring LIKE matching, not FTS - Close store in monitoring-tools test afterEach to prevent timer leaks

Copilot

Pull request overview

Copilot reviewed 19 out of 23 changed files in this pull request and generated 5 comments.

server/src/lib/query-results-evaluator.ts

server/src/lib/sqlite-store.ts

server/src/tools/cache-tools.ts

data-douser · 2026-03-27T13:18:20Z

@copilot apply changes based on the comments in this thread

…fallback, JSON.parse fallback Agent-Logs-Url: https://github.com/advanced-security/codeql-development-mcp-server/sessions/7ff5a5d7-9ac1-4964-8b54-e233d7253301 Co-authored-by: data-douser <70299490+data-douser@users.noreply.github.com>

Copilot · 2026-03-27T13:38:47Z

@copilot apply changes based on the comments in this thread

All changes from that review thread have been applied in commit 5e57b03:

FD leak — extractQueryMetadata now uses try/finally with closeSync(fd) to ensure the fd is always released.
Duplicate fs imports — merged into a single alphabetically-ordered import.
Windows renameSync — flush now catches rename errors and falls back to writeFileSync directly on the target.
FTS4 for annotations — full-text search replaces LIKE (see other reply).
JSON.parse fallback — query_results_cache_retrieve wraps JSON.parse in try/catch and returns raw text when the content is not valid JSON.

Copilot

Pull request overview

Copilot reviewed 19 out of 23 changed files in this pull request and generated 2 comments.

server/src/tools/annotation-tools.ts

server/src/tools/cache-tools.ts

data-douser · 2026-03-27T13:59:42Z

@copilot apply changes based on the comments in this thread

Copilot · 2026-03-27T14:16:11Z

@copilot apply changes based on the comments in this thread

Applied in commit 6fbfa7e:

annotation_search: renamed query → search, updated description to FTS semantics (token-based MATCH, * prefix matching, case-insensitive via unicode61 tokenizer).
query_results_cache_retrieve: removed grep entirely; SARIF format now always routes through getCacheSarifSubset (with maxResults) regardless of whether resultIndices/fileFilter are provided.

- Fix TOCTOU in query-results-evaluator (openSync/fstatSync/readFileSync(fd)) - Use datetime('now') consistently for annotation timestamps - Debounce flush() with 200ms coalescing via scheduleFlush() - Fix resultIndices to inclusive [start, end] range with clamping - Fix WASM→asm.js comments in session-data-manager - Fix audit-tools header comment (no separate ENABLE_AUDIT_TOOLS flag) - Add separate maxResults parameter for SARIF in cache-tools - Use createProjectTempDir() in all test files - Fix monitoring-tools test init order (mock before initialize) - Add store.close() in session-data-manager test afterEach

package-lock.json

data-douser · 2026-03-27T15:59:41Z

@copilot apply changes based on the comments in this thread

Copilot · 2026-03-27T16:10:52Z

@copilot apply changes based on the comments in this thread

Applied all changes from that review thread in commit 7c71547:

Duplicate imports — result-processor.ts now has a single import { CLIExecutionResult, executeCodeQLCommand, getActualCodeqlVersion } from './cli-executor'.
Unit tests — added server/test/src/lib/result-processor.test.ts (23 tests covering computeQueryCacheKey, getDefaultExtension, interpretBQRSFile, and processQueryRunResults including the auto-caching and no-cache paths).
Windows rename comment — updated to accurately state that the fallback on Windows is an in-place overwrite (not crash-safe).
Missing enableAnnotationTools — added enableAnnotationTools: false to the first getConfig mock in monitoring-tools.test.ts.
SARIF fallback semantics — fallback now uses a fixed FALLBACK_MAX_LINES = 500 (not maxResults) and returns totalResults: 0, returnedResults: 0 to signal that the content is not valid SARIF, avoiding the lines/results concept mix.
LRU cache — addressed via comment 3001803915 above.

Replace lowdb with sql.js (asm.js build) for zero-dependency SQLite persistence. Bundle inline with esbuild — no native modules, no external deps at runtime. SqliteStore provides three tables: - sessions: session tracking (migrated from lowdb) - annotations: key-value annotation store with categories and metadata - query_result_cache: BQRS/SARIF result caching with subset retrieval New tools (gated by ENABLE_ANNOTATION_TOOLS env var): - annotation_create, annotation_list, annotation_search, annotation_delete - audit_store_findings, audit_list_findings, audit_add_notes, audit_clear_repo - query_results_cache_lookup, query_results_cache_retrieve, query_results_cache_clear, query_results_cache_compare Code refactoring for maintainability: - Extract database-resolver.ts from cli-tool-registry.ts - Extract query-resolver.ts from cli-tool-registry.ts - Extract result-processor.ts from cli-tool-registry.ts - Extract codeql-version.ts from cli-executor.ts Bug fixes: - Fix params.output not propagated to proce- Fix params.output not propagated to proce- Fix params.output not propagated txternal predicate conditions for direct query paths Closes #165

- Fix TOCTOU in query-results-evaluator (openSync/fstatSync/readFileSync(fd)) - Use datetime('now') consistently for annotation timestamps - Debounce flush() with 200ms coalescing via scheduleFlush() - Fix resultIndices to inclusive [start, end] range with clamping - Fix WASM→asm.js comments in session-data-manager - Fix audit-tools header comment (no separate ENABLE_AUDIT_TOOLS flag) - Add separate maxResults parameter for SARIF in cache-tools - Use createProjectTempDir() in all test files - Fix monitoring-tools test init order (mock before initialize) - Add store.close() in session-data-manager test afterEach

- Bound metadataCache to 256 entries with oldest-first eviction - Make SqliteStore.initialize() idempotent (close existing db first) - Fix TOCTOU in initialize(): try readFileSync directly instead of existsSync - Atomic flush: write to temp file + renameSync to prevent corruption - Clarify annotation_search uses substring LIKE matching, not FTS - Close store in monitoring-tools test afterEach to prevent timer leaks

…fallback, JSON.parse fallback Agent-Logs-Url: https://github.com/advanced-security/codeql-development-mcp-server/sessions/7ff5a5d7-9ac1-4964-8b54-e233d7253301 Co-authored-by: data-douser <70299490+data-douser@users.noreply.github.com>

…ics; always apply SARIF path for SARIF format Agent-Logs-Url: https://github.com/advanced-security/codeql-development-mcp-server/sessions/219712ee-4c28-4b51-9da5-961020112e6e Co-authored-by: data-douser <70299490+data-douser@users.noreply.github.com>

Copilot

Pull request overview

Copilot reviewed 19 out of 23 changed files in this pull request and generated 6 comments.

server/src/lib/sqlite-store.ts

server/src/tools/cache-tools.ts

server/src/lib/query-results-evaluator.ts

server/src/lib/result-processor.ts

server/src/lib/sqlite-store.ts

Copilot

Pull request overview

Copilot reviewed 19 out of 23 changed files in this pull request and generated 10 comments.

Comments suppressed due to low confidence (1)

server/src/tools/annotation-tools.ts:123

annotation_update accepts any number for id, including non-integers and negatives, but annotations.id is an integer primary key. Tighten the schema to an integer (and typically positive) to avoid confusing “not found” behavior for invalid inputs.

      id: z.number().describe('The annotation ID to update.'),
      content: z.string().optional().describe('New content (replaces existing).'),
      label: z.string().optional().describe('New label (replaces existing).'),

server/src/tools/cache-tools.ts

server/src/tools/audit-tools.ts

server/src/tools/cache-tools.ts

server/src/lib/sqlite-store.ts

server/src/tools/annotation-tools.ts

server/src/lib/query-results-evaluator.ts

server/test/src/tools/monitoring-tools.test.ts

server/test/src/lib/sqlite-store.test.ts

server/src/tools/annotation-tools.ts

Copilot

Pull request overview

Copilot reviewed 22 out of 26 changed files in this pull request and generated 10 comments.

Comments suppressed due to low confidence (1)

server/test/src/tools/monitoring-tools.test.ts:227

This test mocks sessionDataManager.getConfig() to set storageLocation: testStorageDir, but the singleton SessionDataManager’s underlying SqliteStore is created from its constructor config and won’t follow the mocked storage location. That makes the test directory setup/cleanup ineffective and risks cross-test pollution. Prefer stubbing sessionDataManager.getStore() to a per-test SqliteStore(testStorageDir) instance (initialized in the test), or refactor SessionDataManager.initialize() to build the store from config.

      // Mock config BEFORE initializing so the store uses the test directory
      vi.spyOn(sessionDataManager, 'getConfig').mockReturnValue({
        storageLocation: testStorageDir,
        autoTrackSessions: true,
        retentionDays: 90,

server/src/lib/sqlite-store.ts

server/test/src/tools/audit-tools.test.ts

server/test/src/tools/annotation-tools.test.ts

server/src/tools/cache-tools.ts

server/src/tools/annotation-tools.ts

server/src/lib/session-data-manager.ts

server/test/src/tools/cache-tools.test.ts

Copilot · 2026-03-28T03:46:51Z

server/src/tools/audit-tools.ts

+    {
+      owner: z.string().describe('Repository owner.'),
+      repo: z.string().describe('Repository name.'),
+      limit: z.number().optional().describe('Maximum number of results.'),


audit_list_findings accepts limit as an unconstrained z.number(). Negative values can disable SQLite limits and return unbounded rows; non-integers can cause runtime errors. Consider validating this as a positive integer (and optionally enforcing a reasonable maximum).

Suggested change

limit: z.number().optional().describe('Maximum number of results.'),

limit: z

.number()

.int()

.positive()

.max(1000)

.optional()

.describe('Maximum number of results (1–1000).'),

server/src/lib/sqlite-store.ts

- annotation_list/annotation_search limit: z.number().int().positive() - annotation_list offset: z.number().int().nonnegative() - audit_list_findings limit: z.number().int().positive().max(1000) - query_results_cache_lookup: add limit param (default 50, max 500) - listAnnotations: wrap FTS MATCH in try/catch; return [] on syntax error - listAnnotations: emit LIMIT -1 when only offset is provided (SQLite requirement) - listCacheEntries: add limit filter support - SessionDataManager.initialize(): recreate SqliteStore when storageLocation changed since construction (fixes test isolation and runtime config updates) - sqlite-store tests: 3 new cases for FTS error, offset-only, cache limit

Copilot AI review requested due to automatic review settings March 25, 2026 11:53

data-douser mentioned this pull request Mar 25, 2026

Integration tests for annotation, audit, cache, and CallGraphFromTo tools #170

Draft

Copilot started reviewing on behalf of data-douser March 25, 2026 11:53 View session

Copilot AI reviewed Mar 25, 2026

View reviewed changes

Copilot AI review requested due to automatic review settings March 27, 2026 01:11

Copilot started reviewing on behalf of data-douser March 27, 2026 01:12 View session

Copilot AI reviewed Mar 27, 2026

View reviewed changes

data-douser requested a review from Copilot March 27, 2026 02:32

Copilot started reviewing on behalf of data-douser March 27, 2026 02:33 View session

Copilot AI reviewed Mar 27, 2026

View reviewed changes

Copilot started work on behalf of data-douser March 27, 2026 13:21 View session

Copilot finished work on behalf of data-douser March 27, 2026 13:42

Copilot AI review requested due to automatic review settings March 27, 2026 13:44

Copilot started reviewing on behalf of data-douser March 27, 2026 13:45 View session

data-douser self-assigned this Mar 27, 2026

data-douser added the enhancement New feature or request label Mar 27, 2026

Copilot AI reviewed Mar 27, 2026

View reviewed changes

server/src/tools/annotation-tools.ts Outdated Show resolved Hide resolved

server/src/tools/cache-tools.ts Outdated Show resolved Hide resolved

data-douser linked an issue Mar 27, 2026 that may be closed by this pull request

seclab-taskflow-integration: Multi-repo CodeQL analysis, annotation store, and query result caching #163

Open

data-douser removed a link to an issue Mar 27, 2026

seclab-taskflow-integration: Multi-repo CodeQL analysis, annotation store, and query result caching #163

Open

Copilot started work on behalf of data-douser March 27, 2026 13:59 View session

Copilot finished work on behalf of data-douser March 27, 2026 14:19

github-license-compliance bot found potential problems Mar 27, 2026

View reviewed changes

package-lock.json Fixed Show fixed Hide fixed

Copilot started work on behalf of data-douser March 27, 2026 15:59 View session

Copilot AI review requested due to automatic review settings March 27, 2026 16:10

data-douser review requested due to automatic review settings March 27, 2026 16:10

Copilot finished work on behalf of data-douser March 27, 2026 16:14

data-douser and others added 7 commits March 27, 2026 14:31

Fix server build

523dcc1

Sync package-lock.json & server/dist/**

5c4f5b8

Copilot AI review requested due to automatic review settings March 27, 2026 20:35

data-douser force-pushed the dd/sqlite-annotation-cache branch from 7c71547 to 5c4f5b8 Compare March 27, 2026 20:35

Copilot started reviewing on behalf of data-douser March 27, 2026 20:36 View session

Copilot AI reviewed Mar 27, 2026

View reviewed changes

Fixes for PR review feedback

b30393a

data-douser marked this pull request as ready for review March 28, 2026 03:02

data-douser requested review from a team and enyil as code owners March 28, 2026 03:02

Copilot AI review requested due to automatic review settings March 28, 2026 03:02

Copilot started reviewing on behalf of data-douser March 28, 2026 03:02 View session

Copilot AI reviewed Mar 28, 2026

View reviewed changes

Address latest PR review feedback

d2a36f8

data-douser requested a review from Copilot March 28, 2026 03:41

Copilot started reviewing on behalf of data-douser March 28, 2026 03:42 View session

Copilot AI reviewed Mar 28, 2026

View reviewed changes

Conversation

data-douser commented Mar 25, 2026 • edited by Copilot AI Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

📝 Update Information

Primitive Details

⚠️ CRITICAL: PR SCOPE VALIDATION

🛑 MANDATORY PR VALIDATION CHECKLIST

Update Metadata

🎯 Changes Description

Current Behavior

Updated Behavior

Motivation

🔄 Before vs. After Comparison

Functionality Changes

API Changes

Output Format Changes

🧪 Testing & Validation

Test Coverage Updates

Validation Scenarios

Test Results

📋 Implementation Details

Files Modified

Code Changes Summary

Dependencies

🔍 Quality Improvements

Bug Fixes

Performance Improvements

Code Quality Enhancements

🔗 References

Related Issues/PRs

Validation Materials

🚀 Compatibility & Migration

Backward Compatibility

Breaking Changes

API Evolution

👥 Review Guidelines

For Reviewers

Testing Instructions

Validation Checklist

📊 Impact Assessment

Performance Impact

Server Impact

AI Assistant Impact

🔄 Deployment Strategy

Rollout Considerations

Post-Deployment Validation

Uh oh!

github-actions bot commented Mar 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Dependency Review

Snapshot Warnings

License Issues

server/package.json

OpenSSF Scorecard

Scanned Files

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

data-douser commented Mar 25, 2026 •

edited by Copilot AI

Loading

github-actions bot commented Mar 25, 2026 •

edited

Loading