diff --git a/crates/codegraph-core/src/db/repository/graph_read.rs b/crates/codegraph-core/src/db/repository/graph_read.rs index aa21de34e..52b216839 100644 --- a/crates/codegraph-core/src/db/repository/graph_read.rs +++ b/crates/codegraph-core/src/db/repository/graph_read.rs @@ -719,7 +719,7 @@ fn validate_triage_role(role: Option<&str>) -> napi::Result<()> { fn build_triage_query( kind: Option<&str>, role: Option<&str>, - file: Option<&str>, + file: Option<&[String]>, no_tests: bool, ) -> (String, Vec>) { let kinds_to_use: Vec<&str> = match kind { @@ -759,9 +759,7 @@ fn build_triage_query( sql.push_str(&format!(" {}", test_filter_clauses("n.file"))); } if let Some(f) = file { - sql.push_str(&format!(" AND n.file LIKE ?{idx} ESCAPE '\\'")); - param_values.push(Box::new(format!("%{}%", escape_like(f)))); - idx += 1; + push_file_filter(&mut sql, &mut param_values, &mut idx, "n.file", f); } if let Some(r) = role { if r == "dead" { @@ -1033,7 +1031,7 @@ impl NativeDatabase { &self, scope_name: String, kind: Option, - file: Option, + file: Option>, ) -> napi::Result> { let conn = self.conn()?; @@ -1048,8 +1046,7 @@ impl NativeDatabase { idx += 1; } if let Some(ref f) = file { - sql.push_str(&format!(" AND file LIKE ?{idx} ESCAPE '\\'")); - param_values.push(Box::new(format!("%{}%", escape_like(f)))); + push_file_filter(&mut sql, &mut param_values, &mut idx, "file", f); } sql.push_str(" ORDER BY file, line"); @@ -1065,53 +1062,35 @@ impl NativeDatabase { .map_err(|e| napi::Error::from_reason(format!("find_nodes_by_scope collect: {e}"))) } - /// Find nodes by qualified name with optional file filter. + /// Find nodes by qualified name with optional (multi-value) file filter. #[napi] pub fn find_node_by_qualified_name( &self, qualified_name: String, - file: Option, + file: Option>, ) -> napi::Result> { let conn = self.conn()?; + let mut sql = "SELECT * FROM nodes WHERE qualified_name = ?1".to_string(); + let mut param_values: Vec> = + vec![Box::new(qualified_name)]; + let mut idx = 2; if let Some(ref f) = file { - let pattern = format!("%{}%", escape_like(f)); - let mut stmt = conn - .prepare_cached( - "SELECT * FROM nodes WHERE qualified_name = ?1 AND file LIKE ?2 ESCAPE '\\' ORDER BY file, line", - ) - .map_err(|e| { - napi::Error::from_reason(format!( - "find_node_by_qualified_name prepare: {e}" - )) - })?; - let rows = stmt - .query_map(params![qualified_name, pattern], read_node_row) - .map_err(|e| { - napi::Error::from_reason(format!("find_node_by_qualified_name: {e}")) - })?; - rows.collect::, _>>().map_err(|e| { - napi::Error::from_reason(format!("find_node_by_qualified_name collect: {e}")) - }) - } else { - let mut stmt = conn - .prepare_cached( - "SELECT * FROM nodes WHERE qualified_name = ?1 ORDER BY file, line", - ) - .map_err(|e| { - napi::Error::from_reason(format!( - "find_node_by_qualified_name prepare: {e}" - )) - })?; - let rows = stmt - .query_map(params![qualified_name], read_node_row) - .map_err(|e| { - napi::Error::from_reason(format!("find_node_by_qualified_name: {e}")) - })?; - rows.collect::, _>>().map_err(|e| { - napi::Error::from_reason(format!("find_node_by_qualified_name collect: {e}")) - }) + push_file_filter(&mut sql, &mut param_values, &mut idx, "file", f); } + sql.push_str(" ORDER BY file, line"); + + let mut stmt = conn.prepare_cached(&sql).map_err(|e| { + napi::Error::from_reason(format!("find_node_by_qualified_name prepare: {e}")) + })?; + let params_ref: Vec<&dyn rusqlite::types::ToSql> = + param_values.iter().map(|p| p.as_ref()).collect(); + let rows = stmt + .query_map(params_ref.as_slice(), read_node_row) + .map_err(|e| napi::Error::from_reason(format!("find_node_by_qualified_name: {e}")))?; + rows.collect::, _>>().map_err(|e| { + napi::Error::from_reason(format!("find_node_by_qualified_name collect: {e}")) + }) } /// Find nodes matching a name pattern with fan-in count. @@ -1189,7 +1168,7 @@ impl NativeDatabase { &self, kind: Option, role: Option, - file: Option, + file: Option>, no_tests: Option, ) -> napi::Result> { validate_triage_kind(kind.as_deref())?; @@ -1219,7 +1198,7 @@ impl NativeDatabase { #[napi] pub fn list_function_nodes( &self, - file: Option, + file: Option>, pattern: Option, no_tests: Option, ) -> napi::Result> { @@ -1230,7 +1209,7 @@ impl NativeDatabase { #[napi] pub fn iterate_function_nodes( &self, - file: Option, + file: Option>, pattern: Option, no_tests: Option, ) -> napi::Result> { @@ -1893,7 +1872,7 @@ impl NativeDatabase { /// Shared implementation for list_function_nodes / iterate_function_nodes. fn query_function_nodes( &self, - file: Option, + file: Option>, pattern: Option, no_tests: Option, ) -> napi::Result> { @@ -1909,9 +1888,7 @@ impl NativeDatabase { let mut idx = 1; if let Some(ref f) = file { - sql.push_str(&format!(" AND n.file LIKE ?{idx} ESCAPE '\\'")); - param_values.push(Box::new(format!("%{}%", escape_like(f)))); - idx += 1; + push_file_filter(&mut sql, &mut param_values, &mut idx, "n.file", f); } if let Some(ref p) = pattern { sql.push_str(&format!(" AND n.name LIKE ?{idx} ESCAPE '\\'")); diff --git a/src/db/repository/base.ts b/src/db/repository/base.ts index 9ab137a86..9f346fe17 100644 --- a/src/db/repository/base.ts +++ b/src/db/repository/base.ts @@ -75,7 +75,7 @@ export class Repository implements IRepository { throw new Error('not implemented'); } - findNodeByQualifiedName(_qualifiedName: string, _opts?: { file?: string }): NodeRow[] { + findNodeByQualifiedName(_qualifiedName: string, _opts?: { file?: string | string[] }): NodeRow[] { throw new Error('not implemented'); } diff --git a/src/db/repository/in-memory-repository.ts b/src/db/repository/in-memory-repository.ts index 28708c2ee..05d918aa4 100644 --- a/src/db/repository/in-memory-repository.ts +++ b/src/db/repository/in-memory-repository.ts @@ -267,7 +267,10 @@ export class InMemoryRepository extends Repository { return nodes.sort((a, b) => a.file.localeCompare(b.file) || a.line - b.line); } - findNodeByQualifiedName(qualifiedName: string, opts: { file?: string } = {}): NodeRow[] { + findNodeByQualifiedName( + qualifiedName: string, + opts: { file?: string | string[] } = {}, + ): NodeRow[] { let nodes = [...this.#nodes.values()].filter((n) => n.qualified_name === qualifiedName); { diff --git a/src/db/repository/native-repository.ts b/src/db/repository/native-repository.ts index a6a35ad4e..03d294304 100644 --- a/src/db/repository/native-repository.ts +++ b/src/db/repository/native-repository.ts @@ -9,7 +9,6 @@ */ import Database from 'better-sqlite3'; -import { warn } from '../../infrastructure/logger.js'; import { ConfigError } from '../../shared/errors.js'; import type { AdjacentEdgeRow, @@ -304,45 +303,53 @@ export class NativeRepository extends Repository { } findNodesByScope(scopeName: string, opts: QueryOpts = {}): NodeRow[] { - // Unlike findNodesWithFanIn/fnDeps, this native binding only accepts a single - // file (no CLI command currently wires -f/--file to it); take the first value - // rather than crash if a caller ever passes the repeatable-flag array form. - // TODO(#1815): widen the native binding to Vec so multi-file scoping - // isn't silently truncated once a caller wires -f/--file to this method. const files = normalizeFileFilter(opts.file); - if (files.length > 1) { - warn( - `findNodesByScope: received ${files.length} files, only using the first ("${files[0]}") — multi-file scoping not yet supported natively (see #1815)`, - ); - } - const [file] = files; - return this.#ndb.findNodesByScope(scopeName, opts.kind ?? null, file ?? null).map(toNodeRow); + return this.#ndb + .findNodesByScope(scopeName, opts.kind ?? null, files.length > 0 ? files : null) + .map(toNodeRow); } - findNodeByQualifiedName(qualifiedName: string, opts: { file?: string } = {}): NodeRow[] { - return this.#ndb.findNodeByQualifiedName(qualifiedName, opts.file ?? null).map(toNodeRow); + findNodeByQualifiedName( + qualifiedName: string, + opts: { file?: string | string[] } = {}, + ): NodeRow[] { + const files = normalizeFileFilter(opts.file); + return this.#ndb + .findNodeByQualifiedName(qualifiedName, files.length > 0 ? files : null) + .map(toNodeRow); } listFunctionNodes(opts: ListFunctionOpts = {}): NodeRow[] { + const files = normalizeFileFilter(opts.file); return this.#ndb - .listFunctionNodes(opts.file ?? null, opts.pattern ?? null, opts.noTests ?? null) + .listFunctionNodes( + files.length > 0 ? files : null, + opts.pattern ?? null, + opts.noTests ?? null, + ) .map(toNodeRow); } iterateFunctionNodes(opts: ListFunctionOpts = {}): IterableIterator { + const files = normalizeFileFilter(opts.file); const rows = this.#ndb - .iterateFunctionNodes(opts.file ?? null, opts.pattern ?? null, opts.noTests ?? null) + .iterateFunctionNodes( + files.length > 0 ? files : null, + opts.pattern ?? null, + opts.noTests ?? null, + ) .map(toNodeRow); return rows[Symbol.iterator](); } findNodesForTriage(opts: TriageQueryOpts = {}): TriageNodeRow[] { try { + const files = normalizeFileFilter(opts.file); return this.#ndb .findNodesForTriage( opts.kind ?? null, opts.role ?? null, - opts.file ?? null, + files.length > 0 ? files : null, opts.noTests ?? null, ) .map(toTriageNodeRow); diff --git a/src/db/repository/nodes.ts b/src/db/repository/nodes.ts index 250c4d634..11d3ec6df 100644 --- a/src/db/repository/nodes.ts +++ b/src/db/repository/nodes.ts @@ -333,7 +333,7 @@ export function findNodesByScope( export function findNodeByQualifiedName( db: BetterSqlite3Database, qualifiedName: string, - opts: { file?: string } = {}, + opts: { file?: string | string[] } = {}, ): NodeRow[] { const fc = buildFileConditionSQL(opts.file ?? '', 'file'); if (fc.sql) { diff --git a/src/db/repository/sqlite-repository.ts b/src/db/repository/sqlite-repository.ts index 1ef1210f3..126d69b21 100644 --- a/src/db/repository/sqlite-repository.ts +++ b/src/db/repository/sqlite-repository.ts @@ -128,7 +128,7 @@ export class SqliteRepository extends Repository { return findNodesByScope(this.#db, scopeName, opts); } - findNodeByQualifiedName(qualifiedName: string, opts?: { file?: string }): NodeRow[] { + findNodeByQualifiedName(qualifiedName: string, opts?: { file?: string | string[] }): NodeRow[] { return findNodeByQualifiedName(this.#db, qualifiedName, opts); } diff --git a/src/domain/analysis/implementations.ts b/src/domain/analysis/implementations.ts index ea6703839..2c9ca2092 100644 --- a/src/domain/analysis/implementations.ts +++ b/src/domain/analysis/implementations.ts @@ -12,7 +12,13 @@ import { findMatchingNodes } from './symbol-lookup.js'; export function implementationsData( name: string, customDbPath: string, - opts: { noTests?: boolean; file?: string; kind?: string; limit?: number; offset?: number } = {}, + opts: { + noTests?: boolean; + file?: string | string[]; + kind?: string; + limit?: number; + offset?: number; + } = {}, ) { return withRepo(customDbPath, (repo) => { const noTests = opts.noTests || false; @@ -49,7 +55,13 @@ export function implementationsData( export function interfacesData( name: string, customDbPath: string, - opts: { noTests?: boolean; file?: string; kind?: string; limit?: number; offset?: number } = {}, + opts: { + noTests?: boolean; + file?: string | string[]; + kind?: string; + limit?: number; + offset?: number; + } = {}, ) { return withRepo(customDbPath, (repo) => { const noTests = opts.noTests || false; diff --git a/src/features/triage.ts b/src/features/triage.ts index 95e7d1559..b102d52dd 100644 --- a/src/features/triage.ts +++ b/src/features/triage.ts @@ -4,6 +4,7 @@ import { DEFAULT_WEIGHTS, scoreRisk } from '../graph/classifiers/risk.js'; import { loadConfig } from '../infrastructure/config.js'; import { warn } from '../infrastructure/logger.js'; import { isTestFile } from '../infrastructure/test-filter.js'; +import { ConfigError } from '../shared/errors.js'; import { paginateResult } from '../shared/paginate.js'; import type { CodegraphConfig, Role, TriageNodeRow } from '../types.js'; @@ -108,7 +109,7 @@ interface TriageDataOpts { sort?: string; config?: CodegraphConfig; weights?: Partial; - file?: string; + file?: string | string[]; kind?: string; role?: Role; limit?: number; @@ -163,8 +164,15 @@ export function triageData( role: opts.role || undefined, }); } catch (err: unknown) { - warn(`triage query failed: ${(err as Error).message}`); - return { items: [], summary: EMPTY_SUMMARY(weights) }; + // Only invalid kind/role (validated inside findNodesForTriage, surfaced as + // ConfigError) degrade gracefully to an empty result — every other failure + // (e.g. an internal query bug) must propagate rather than masquerade as + // "no symbols match" with exit code 0. + if (err instanceof ConfigError) { + warn(`triage query failed: ${err.message}`); + return { items: [], summary: EMPTY_SUMMARY(weights) }; + } + throw err; } const filtered = noTests ? rows.filter((r) => !isTestFile(r.file)) : rows; diff --git a/src/presentation/triage.ts b/src/presentation/triage.ts index 2b8b5e472..26587ab17 100644 --- a/src/presentation/triage.ts +++ b/src/presentation/triage.ts @@ -10,7 +10,7 @@ interface TriageOpts { sort?: string; kind?: string; role?: string; - file?: string; + file?: string | string[]; minScore?: number; limit?: number; offset?: number; diff --git a/src/types.ts b/src/types.ts index 3d3c0bf8f..db488d71d 100644 --- a/src/types.ts +++ b/src/types.ts @@ -272,7 +272,8 @@ export interface QueryOpts { /** Options for listFunctionNodes / iterateFunctionNodes. */ export interface ListFunctionOpts { - file?: string; + /** Partial-match file filter. Repeatable on the CLI (`-f/--file`), so a single value arrives as a one-element array. */ + file?: string | string[]; pattern?: string; noTests?: boolean; } @@ -282,7 +283,8 @@ export interface TriageQueryOpts { kind?: string; role?: Role; noTests?: boolean; - file?: string; + /** Partial-match file filter. Repeatable on the CLI (`-f/--file`), so a single value arrives as a one-element array. */ + file?: string | string[]; } /** @@ -303,7 +305,7 @@ export interface Repository { bulkNodeIdsByFile(file: string): NodeIdRow[]; findNodeChildren(parentId: number): ChildNodeRow[]; findNodesByScope(scopeName: string, opts?: QueryOpts): NodeRow[]; - findNodeByQualifiedName(qualifiedName: string, opts?: { file?: string }): NodeRow[]; + findNodeByQualifiedName(qualifiedName: string, opts?: { file?: string | string[] }): NodeRow[]; listFunctionNodes(opts?: ListFunctionOpts): NodeRow[]; iterateFunctionNodes(opts?: ListFunctionOpts): IterableIterator; findNodesForTriage(opts?: TriageQueryOpts): TriageNodeRow[]; @@ -2563,9 +2565,12 @@ export interface NativeDatabase { findNodesByScope( scopeName: string, kind: string | null | undefined, - file: string | null | undefined, + file: string[] | null | undefined, + ): NativeNodeRow[]; + findNodeByQualifiedName( + qualifiedName: string, + file: string[] | null | undefined, ): NativeNodeRow[]; - findNodeByQualifiedName(qualifiedName: string, file: string | null | undefined): NativeNodeRow[]; findNodesWithFanIn( namePattern: string, kinds: string[] | null | undefined, @@ -2574,16 +2579,16 @@ export interface NativeDatabase { findNodesForTriage( kind: string | null | undefined, role: string | null | undefined, - file: string | null | undefined, + file: string[] | null | undefined, noTests: boolean | null | undefined, ): NativeTriageNodeRow[]; listFunctionNodes( - file: string | null | undefined, + file: string[] | null | undefined, pattern: string | null | undefined, noTests: boolean | null | undefined, ): NativeNodeRow[]; iterateFunctionNodes( - file: string | null | undefined, + file: string[] | null | undefined, pattern: string | null | undefined, noTests: boolean | null | undefined, ): NativeNodeRow[]; diff --git a/tests/integration/cli.test.ts b/tests/integration/cli.test.ts index 6ff06f02a..1d5b0fad7 100644 --- a/tests/integration/cli.test.ts +++ b/tests/integration/cli.test.ts @@ -250,6 +250,52 @@ describe('CLI smoke tests', () => { expect(data.level).toBe('directory'); }); + // Regression tests for #1815: `-f/--file` is a repeatable Commander option + // (collectFile) that always produces a string[], even for a single use. + // triage's native composite path (findNodesForTriage) used to forward that + // array straight into a napi binding typed for a single String; the failure + // was silently swallowed into an empty result with exit code 0 instead of + // crashing or surfacing an error (see query -f test above for #1726 context + // on the underlying array-vs-String bug class). + test('triage -f scopes results to a single file without crashing or silently emptying', () => { + const out = run('triage', '-f', 'math.js', '--db', dbPath, '--json'); + const data = JSON.parse(out); + expect(data.items.length).toBeGreaterThan(0); + for (const it of data.items) expect(it.file).toContain('math.js'); + }); + + test('triage with a single -f excludes non-matching files', () => { + const out = run('triage', '-f', 'math.js', '--kind', 'function', '--db', dbPath, '--json'); + const data = JSON.parse(out); + const names = data.items.map((it) => it.name); + expect(names).toContain('add'); + expect(names).not.toContain('sumOfSquares'); + }); + + test('triage supports repeated -f (multi-file scoping)', () => { + const out = run('triage', '-f', 'math.js', '-f', 'utils.js', '--db', dbPath, '--json'); + const data = JSON.parse(out); + const files = new Set(data.items.map((it) => it.file)); + expect([...files].some((f) => f.includes('math.js'))).toBe(true); + expect([...files].some((f) => f.includes('utils.js'))).toBe(true); + expect([...files].some((f) => f.includes('index.js'))).toBe(false); + }); + + // ─── Interfaces / Implementations ─────────────────────────────────── + test('interfaces -f does not crash when scoping by file', () => { + const out = run('interfaces', 'Calculator', '-f', 'utils.js', '--db', dbPath, '--json'); + const data = JSON.parse(out); + expect(data).toHaveProperty('results'); + expect(Array.isArray(data.results)).toBe(true); + }); + + test('implementations -f does not crash when scoping by file', () => { + const out = run('implementations', 'Calculator', '-f', 'utils.js', '--db', dbPath, '--json'); + const data = JSON.parse(out); + expect(data).toHaveProperty('results'); + expect(Array.isArray(data.results)).toBe(true); + }); + // ─── Audit --quick (formerly explain) ────────────────────────────── test('audit --quick --json returns structural summary', () => { const out = run('audit', 'math.js', '--quick', '--db', dbPath, '--json'); diff --git a/tests/integration/triage.test.ts b/tests/integration/triage.test.ts index e7bdcb864..89924643a 100644 --- a/tests/integration/triage.test.ts +++ b/tests/integration/triage.test.ts @@ -5,7 +5,9 @@ */ import { beforeAll, describe, expect, test } from 'vitest'; +import { InMemoryRepository } from '../../src/db/repository/in-memory-repository.js'; import { triageData } from '../../src/features/triage.js'; +import { ConfigError } from '../../src/shared/errors.js'; import { createTestRepo } from '../helpers/fixtures.js'; // ─── Fixture ────────────────────────────────────────────────────────── @@ -263,4 +265,31 @@ describe('triage', () => { expect(core.roleWeight).toBe(1.0); expect(leaf.roleWeight).toBe(0.2); }); + + // Regression tests for #1815: findNodesForTriage failures used to be + // unconditionally swallowed into an empty result (exit 0), masking real + // internal errors (e.g. the native array-vs-String crash) as "no symbols + // match". Only the validated-input ConfigError case should degrade + // gracefully; anything else must propagate. + test('propagates a non-ConfigError failure from findNodesForTriage instead of swallowing it', () => { + class BrokenRepo extends InMemoryRepository { + override findNodesForTriage(): never { + throw new Error('boom: internal query failure'); + } + } + expect(() => triageData(null, { repo: new BrokenRepo(), limit: 100 })).toThrow( + 'boom: internal query failure', + ); + }); + + test('degrades gracefully (empty result) on a ConfigError from findNodesForTriage', () => { + class InvalidOptsRepo extends InMemoryRepository { + override findNodesForTriage(): never { + throw new ConfigError('Invalid kind: bogus (expected one of function, method, class)'); + } + } + const result = triageData(null, { repo: new InvalidOptsRepo(), limit: 100 }); + expect(result.items).toEqual([]); + expect(result.summary.total).toBe(0); + }); });