From 2ca9dd1236a7c3a892e33eecbc6ebfaa2e64fac0 Mon Sep 17 00:00:00 2001 From: mars167 Date: Thu, 12 Feb 2026 01:46:20 +0800 Subject: [PATCH] fix: address Copilot PR #22 review comments - Extract shared helpers (isCLIError, buildRepoMapAttachment, resolveWikiDir, inferLangFromFile, filterWorkspaceRowsByLang) into sharedHelpers.ts to eliminate duplication between queryHandlers.ts and queryFilesHandlers.ts - Add path traversal protection in resolveWikiDir: reject paths that escape repoRoot - Fix wildcard SQL prefilter: convert glob patterns to SQL LIKE patterns (globToSqlLike) instead of treating raw glob as substring - Return explicit error for workspace mode in query-files since queryManifestWorkspace queries by symbol, not file name - Remove unused import (resolveLangs) from queryFilesHandlers.ts - Convert Commander.js string options to numbers in queryFilesCommand.ts (limit, maxCandidates, repoMapFiles, repoMapSymbols) - Improve tests: add proper assertions for wildcard, language filtering, empty pattern (via Zod schema), and invalid mode (via Zod schema) - Remove unused variable in empty pattern test --- package-lock.json | 10 +- src/cli/commands/queryFilesCommand.ts | 13 ++- src/cli/handlers/queryFilesHandlers.ts | 121 +++++++++---------------- src/cli/handlers/queryHandlers.ts | 59 +----------- src/cli/handlers/sharedHelpers.ts | 67 ++++++++++++++ test/queryFiles.test.ts | 87 +++++++++++------- 6 files changed, 181 insertions(+), 176 deletions(-) create mode 100644 src/cli/handlers/sharedHelpers.ts diff --git a/package-lock.json b/package-lock.json index 069bcc9..679d5bc 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,12 +1,12 @@ { "name": "@mars167/git-ai", - "version": "2.4.0", + "version": "2.4.3", "lockfileVersion": 3, "requires": true, "packages": { "": { "name": "@mars167/git-ai", - "version": "2.4.0", + "version": "2.4.3", "license": "MIT", "os": [ "darwin", @@ -451,7 +451,6 @@ "node_modules/@types/node": { "version": "25.0.9", "license": "MIT", - "peer": true, "dependencies": { "undici-types": "~7.16.0" } @@ -553,7 +552,6 @@ "node_modules/apache-arrow": { "version": "18.1.0", "license": "Apache-2.0", - "peer": true, "dependencies": { "@swc/helpers": "^0.5.11", "@types/command-line-args": "^5.2.3", @@ -971,7 +969,6 @@ "node_modules/express": { "version": "5.2.1", "license": "MIT", - "peer": true, "dependencies": { "accepts": "^2.0.0", "body-parser": "^2.2.1", @@ -2046,7 +2043,6 @@ "version": "0.21.1", "hasInstallScript": true, "license": "MIT", - "peer": true, "dependencies": { "node-addon-api": "^8.0.0", "node-gyp-build": "^4.8.0" @@ -2222,7 +2218,6 @@ "node_modules/typescript": { "version": "5.9.3", "license": "Apache-2.0", - "peer": true, "bin": { "tsc": "bin/tsc", "tsserver": "bin/tsserver" @@ -2331,7 +2326,6 @@ "node_modules/zod": { "version": "4.3.5", "license": "MIT", - "peer": true, "funding": { "url": "https://github.com/sponsors/colinhacks" } diff --git a/src/cli/commands/queryFilesCommand.ts b/src/cli/commands/queryFilesCommand.ts index b536a7c..18a3792 100644 --- a/src/cli/commands/queryFilesCommand.ts +++ b/src/cli/commands/queryFilesCommand.ts @@ -15,5 +15,16 @@ export const queryFilesCommand = new Command('query-files') .option('--repo-map-symbols ', 'Max repo map symbols per file', '5') .option('--wiki ', 'Wiki directory (default: docs/wiki or wiki)', '') .action(async (pattern, options) => { - await executeHandler('query-files', { pattern, ...options }); + const limit = parseInt(options.limit, 10); + const maxCandidates = parseInt(options.maxCandidates, 10); + const repoMapFiles = parseInt(options.repoMapFiles, 10); + const repoMapSymbols = parseInt(options.repoMapSymbols, 10); + await executeHandler('query-files', { + pattern, + ...options, + limit, + maxCandidates, + repoMapFiles, + repoMapSymbols, + }); }); diff --git a/src/cli/handlers/queryFilesHandlers.ts b/src/cli/handlers/queryFilesHandlers.ts index dc5f80c..1cadc75 100644 --- a/src/cli/handlers/queryFilesHandlers.ts +++ b/src/cli/handlers/queryFilesHandlers.ts @@ -1,85 +1,55 @@ import path from 'path'; -import fs from 'fs-extra'; import { inferWorkspaceRoot, resolveGitRoot } from '../../core/git'; import { defaultDbDir, openTablesByLang, type IndexLang } from '../../core/lancedb'; -import { queryManifestWorkspace } from '../../core/workspace'; import { inferSymbolSearchMode, type SymbolSearchMode } from '../../core/symbolSearch'; import { createLogger } from '../../core/log'; -import { resolveLangs } from '../../core/indexCheck'; -import { generateRepoMap, type FileRank } from '../../core/repoMap'; import type { CLIResult, CLIError } from '../types'; import { success, error } from '../types'; import { resolveRepoContext, validateIndex, resolveLanguages, type RepoContext } from '../helpers'; import type { SearchFilesInput } from '../schemas/queryFilesSchemas'; - -function isCLIError(value: unknown): value is CLIError { - return typeof value === 'object' && value !== null && 'ok' in value && (value as any).ok === false; -} - -async function buildRepoMapAttachment( - repoRoot: string, - options: { wiki: string; repoMapFiles: number; repoMapSymbols: number } -): Promise<{ enabled: boolean; wikiDir: string; files: FileRank[] } | { enabled: boolean; skippedReason: string }> { - try { - const wikiDir = resolveWikiDir(repoRoot, options.wiki); - const files = await generateRepoMap({ - repoRoot, - maxFiles: options.repoMapFiles, - maxSymbolsPerFile: options.repoMapSymbols, - wikiDir, - }); - return { enabled: true, wikiDir, files }; - } catch (e: any) { - return { enabled: false, skippedReason: String(e?.message ?? e) }; - } -} - -function resolveWikiDir(repoRoot: string, wikiOpt: string): string { - const w = String(wikiOpt ?? '').trim(); - if (w) return path.resolve(repoRoot, w); - const candidates = [path.join(repoRoot, 'docs', 'wiki'), path.join(repoRoot, 'wiki')]; - for (const c of candidates) { - if (fs.existsSync(c)) return c; - } - return ''; -} - -function inferLangFromFile(file: string): IndexLang { - const f = String(file); - if (f.endsWith('.md') || f.endsWith('.mdx')) return 'markdown'; - if (f.endsWith('.yml') || f.endsWith('.yaml')) return 'yaml'; - if (f.endsWith('.java')) return 'java'; - if (f.endsWith('.c') || f.endsWith('.h')) return 'c'; - if (f.endsWith('.go')) return 'go'; - if (f.endsWith('.py')) return 'python'; - if (f.endsWith('.rs')) return 'rust'; - return 'ts'; -} - -function filterWorkspaceRowsByLang(rows: any[], langSel: string): any[] { - const sel = String(langSel ?? 'auto'); - if (sel === 'auto' || sel === 'all') return rows; - const target = sel as IndexLang; - return rows.filter(r => inferLangFromFile(String((r as any).file ?? '')) === target); -} +import { + isCLIError, + buildRepoMapAttachment, + filterWorkspaceRowsByLang, +} from './sharedHelpers'; function escapeQuotes(s: string): string { return s.replace(/'/g, "''"); } +/** + * Convert a glob pattern to a SQL LIKE pattern. + * Escapes SQL LIKE wildcards (% and _), then converts glob * to % and ? to _. + */ +function globToSqlLike(pattern: string): string { + let like = pattern.replace(/\\/g, '\\\\').replace(/([%_])/g, '\\$1'); + like = like.replace(/\*/g, '%').replace(/\?/g, '_'); + return like; +} + function buildFileWhere(pattern: string, mode: SymbolSearchMode, caseInsensitive: boolean): string | null { - const safe = escapeQuotes(pattern); - if (!safe) return null; + if (!pattern) return null; const likeOp = caseInsensitive ? 'ILIKE' : 'LIKE'; if (mode === 'prefix') { + const safe = escapeQuotes(pattern); + if (!safe) return null; return `file ${likeOp} '${safe}%'`; } - if (mode === 'substring' || mode === 'wildcard') { + if (mode === 'substring') { + const safe = escapeQuotes(pattern); + if (!safe) return null; return `file ${likeOp} '%${safe}%'`; } + if (mode === 'wildcard') { + const likePattern = globToSqlLike(pattern); + const safeLike = escapeQuotes(likePattern); + if (!safeLike) return null; + return `file ${likeOp} '${safeLike}'`; + } + // For regex and fuzzy, we'll handle them in memory after fetching return null; } @@ -190,36 +160,28 @@ export async function handleSearchFiles(input: SearchFilesInput): Promise { - try { - const wikiDir = resolveWikiDir(repoRoot, options.wiki); - const files = await generateRepoMap({ - repoRoot, - maxFiles: options.repoMapFiles, - maxSymbolsPerFile: options.repoMapSymbols, - wikiDir, - }); - return { enabled: true, wikiDir, files }; - } catch (e: any) { - return { enabled: false, skippedReason: String(e?.message ?? e) }; - } -} - -function resolveWikiDir(repoRoot: string, wikiOpt: string): string { - const w = String(wikiOpt ?? '').trim(); - if (w) return path.resolve(repoRoot, w); - const candidates = [path.join(repoRoot, 'docs', 'wiki'), path.join(repoRoot, 'wiki')]; - for (const c of candidates) { - if (fs.existsSync(c)) return c; - } - return ''; -} - -function inferLangFromFile(file: string): IndexLang { - const f = String(file); - if (f.endsWith('.md') || f.endsWith('.mdx')) return 'markdown'; - if (f.endsWith('.yml') || f.endsWith('.yaml')) return 'yaml'; - if (f.endsWith('.java')) return 'java'; - if (f.endsWith('.c') || f.endsWith('.h')) return 'c'; - if (f.endsWith('.go')) return 'go'; - if (f.endsWith('.py')) return 'python'; - if (f.endsWith('.rs')) return 'rust'; - return 'ts'; -} - -function filterWorkspaceRowsByLang(rows: any[], langSel: string): any[] { - const sel = String(langSel ?? 'auto'); - if (sel === 'auto' || sel === 'all') return rows; - const target = sel as IndexLang; - return rows.filter(r => inferLangFromFile(String((r as any).file ?? '')) === target); -} +import { + isCLIError, + buildRepoMapAttachment, + filterWorkspaceRowsByLang, +} from './sharedHelpers'; export async function handleSearchSymbols(input: { keyword: string; diff --git a/src/cli/handlers/sharedHelpers.ts b/src/cli/handlers/sharedHelpers.ts new file mode 100644 index 0000000..8b70545 --- /dev/null +++ b/src/cli/handlers/sharedHelpers.ts @@ -0,0 +1,67 @@ +import path from 'path'; +import fs from 'fs-extra'; +import type { IndexLang } from '../../core/lancedb'; +import { generateRepoMap, type FileRank } from '../../core/repoMap'; +import type { CLIError } from '../types'; + +export function isCLIError(value: unknown): value is CLIError { + return typeof value === 'object' && value !== null && 'ok' in value && (value as any).ok === false; +} + +export async function buildRepoMapAttachment( + repoRoot: string, + options: { wiki: string; repoMapFiles: number; repoMapSymbols: number } +): Promise<{ enabled: boolean; wikiDir: string; files: FileRank[] } | { enabled: boolean; skippedReason: string }> { + try { + const wikiDir = resolveWikiDir(repoRoot, options.wiki); + const files = await generateRepoMap({ + repoRoot, + maxFiles: options.repoMapFiles, + maxSymbolsPerFile: options.repoMapSymbols, + wikiDir, + }); + return { enabled: true, wikiDir, files }; + } catch (e: any) { + return { enabled: false, skippedReason: String(e?.message ?? e) }; + } +} + +/** + * Resolve wiki directory, ensuring the resolved path stays within repoRoot + * to prevent path traversal attacks. + */ +export function resolveWikiDir(repoRoot: string, wikiOpt: string): string { + const w = String(wikiOpt ?? '').trim(); + if (w) { + const resolved = path.resolve(repoRoot, w); + // Prevent path traversal outside repoRoot + if (!resolved.startsWith(repoRoot + path.sep) && resolved !== repoRoot) { + return ''; + } + return resolved; + } + const candidates = [path.join(repoRoot, 'docs', 'wiki'), path.join(repoRoot, 'wiki')]; + for (const c of candidates) { + if (fs.existsSync(c)) return c; + } + return ''; +} + +export function inferLangFromFile(file: string): IndexLang { + const f = String(file); + if (f.endsWith('.md') || f.endsWith('.mdx')) return 'markdown'; + if (f.endsWith('.yml') || f.endsWith('.yaml')) return 'yaml'; + if (f.endsWith('.java')) return 'java'; + if (f.endsWith('.c') || f.endsWith('.h')) return 'c'; + if (f.endsWith('.go')) return 'go'; + if (f.endsWith('.py')) return 'python'; + if (f.endsWith('.rs')) return 'rust'; + return 'ts'; +} + +export function filterWorkspaceRowsByLang(rows: any[], langSel: string): any[] { + const sel = String(langSel ?? 'auto'); + if (sel === 'auto' || sel === 'all') return rows; + const target = sel as IndexLang; + return rows.filter(r => inferLangFromFile(String((r as any).file ?? '')) === target); +} diff --git a/test/queryFiles.test.ts b/test/queryFiles.test.ts index 427baeb..7449bef 100644 --- a/test/queryFiles.test.ts +++ b/test/queryFiles.test.ts @@ -2,6 +2,8 @@ import test from 'node:test'; import assert from 'node:assert/strict'; // @ts-ignore dist module has no typings import { handleSearchFiles } from '../dist/src/cli/handlers/queryFilesHandlers.js'; +// @ts-ignore dist module has no typings +import { SearchFilesSchema } from '../dist/src/cli/schemas/queryFilesSchemas.js'; const testPath = '.'; @@ -89,6 +91,14 @@ test('query-files: language filtering works', async () => { assert(result.ok, 'Query should succeed'); assert(Array.isArray(result.rows), 'Result should contain rows array'); + // Verify all returned rows are from ts-related files + assert( + result.rows.every((row: any) => { + const file = String(row.file ?? ''); + return file.endsWith('.ts') || file.endsWith('.tsx') || file.endsWith('.js') || file.endsWith('.jsx'); + }), + 'All results should be TypeScript/JavaScript files when lang=ts', + ); }); test('query-files: limit parameter respected', async () => { @@ -115,7 +125,7 @@ test('query-files: limit parameter respected', async () => { test('query-files: wildcard search with asterisk', async () => { const result = await handleSearchFiles({ - pattern: 'src/*/handlers', + pattern: 'src/*/handlers*', path: testPath, limit: 50, mode: 'wildcard', @@ -129,6 +139,16 @@ test('query-files: wildcard search with asterisk', async () => { }); assert(result.ok, 'Query should succeed'); + assert(Array.isArray(result.rows), 'Result should contain rows array'); + assert(result.rows.length > 0, 'Should find at least one file matching wildcard pattern'); + assert( + result.rows.every((row: any) => { + const file = String(row.file ?? ''); + // Verify the file matches the glob pattern: src/*/handlers* + return /^src\/[^/]+\/handlers/.test(file); + }), + 'All results should match the wildcard pattern src/*/handlers*', + ); }); test('query-files: fuzzy search finds partial matches', async () => { @@ -173,40 +193,41 @@ test('query-files: regex search with pattern', async () => { ); }); -test('query-files: empty pattern returns error', async () => { - const result = await handleSearchFiles({ - pattern: '', - path: testPath, - limit: 50, - mode: 'substring', - caseInsensitive: false, - maxCandidates: 1000, - lang: 'ts', - withRepoMap: false, - repoMapFiles: 20, - repoMapSymbols: 5, - wiki: '', - }); +test('query-files: empty pattern rejected by schema validation', () => { + assert.throws( + () => SearchFilesSchema.parse({ + pattern: '', + path: testPath, + limit: 50, + mode: 'substring', + caseInsensitive: false, + maxCandidates: 1000, + lang: 'ts', + withRepoMap: false, + repoMapFiles: 20, + repoMapSymbols: 5, + wiki: '', + }), + 'Empty pattern should be rejected by Zod schema validation', + ); }); -test('query-files: invalid mode handled gracefully', async () => { - const result = await handleSearchFiles({ - pattern: 'test', - path: testPath, - limit: 50, - mode: 'invalid' as any, - caseInsensitive: false, - maxCandidates: 1000, - lang: 'ts', - withRepoMap: false, - repoMapFiles: 20, - repoMapSymbols: 5, - wiki: '', - }); - - assert( - !result.ok || result.rows.length >= 0, - 'Should either fail or return empty array', +test('query-files: invalid mode rejected by schema validation', () => { + assert.throws( + () => SearchFilesSchema.parse({ + pattern: 'test', + path: testPath, + limit: 50, + mode: 'invalid', + caseInsensitive: false, + maxCandidates: 1000, + lang: 'ts', + withRepoMap: false, + repoMapFiles: 20, + repoMapSymbols: 5, + wiki: '', + }), + 'Invalid mode should be rejected by Zod schema validation', ); });