-
Notifications
You must be signed in to change notification settings - Fork 0
fix: address Copilot review comments from PR #22 #23
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -15,5 +15,16 @@ export const queryFilesCommand = new Command('query-files') | |||||||||||||||||||||||||||||
| .option('--repo-map-symbols <n>', 'Max repo map symbols per file', '5') | ||||||||||||||||||||||||||||||
| .option('--wiki <dir>', 'Wiki directory (default: docs/wiki or wiki)', '') | ||||||||||||||||||||||||||||||
| .action(async (pattern, options) => { | ||||||||||||||||||||||||||||||
| await executeHandler('query-files', { pattern, ...options }); | ||||||||||||||||||||||||||||||
| const limit = parseInt(options.limit, 10); | ||||||||||||||||||||||||||||||
|
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💡 SUGGESTION: 缺乏参数验证提示 CLI 参数错误时未向用户输出友好的错误信息,用户无法得知应如何修正 建议: 捕获无效输入并输出用法提示,帮助用户正确使用命令 |
||||||||||||||||||||||||||||||
| 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, | ||||||||||||||||||||||||||||||
|
Comment on lines
+18
to
+28
|
||||||||||||||||||||||||||||||
| 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, | |
| await executeHandler('query-files', { | |
| pattern, | |
| ...options, |
| Original file line number | Diff line number | Diff line change | ||
|---|---|---|---|---|
| @@ -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, | ||||
|
||||
| filterWorkspaceRowsByLang, |
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -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; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 📝 NIT: 类型守卫可进一步优化 使用 建议: 使用可选链和更严格的类型检查
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| 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) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. catch 块中使用 建议: 使用
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| 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 { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+29
to
+33
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| 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) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 使用 建议: 改用 path.relative 进行安全校验
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| 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; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+30
to
+46
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * 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; | |
| } | |
| * Check whether `candidate` is inside (or equal to) `root`, using a relative path | |
| * comparison. Both arguments must be absolute, normalized paths. | |
| */ | |
| function isPathInside(root: string, candidate: string): boolean { | |
| const rel = path.relative(root, candidate); | |
| if (rel === '') return true; | |
| return rel !== '..' && !rel.startsWith('..' + path.sep); | |
| } | |
| /** | |
| * Resolve wiki directory, ensuring the resolved path stays within repoRoot | |
| * (taking symlinks into account) to prevent path traversal attacks. | |
| */ | |
| export function resolveWikiDir(repoRoot: string, wikiOpt: string): string { | |
| const w = String(wikiOpt ?? '').trim(); | |
| const rootReal = fs.realpathSync(repoRoot); | |
| if (w) { | |
| const resolved = path.resolve(repoRoot, w); | |
| // If the target exists, validate using realpath to avoid symlink escapes. | |
| if (fs.existsSync(resolved)) { | |
| const resolvedReal = fs.realpathSync(resolved); | |
| if (!isPathInside(rootReal, resolvedReal)) { | |
| throw new Error(`Wiki directory must be inside the repository root: ${resolvedReal}`); | |
| } | |
| return resolvedReal; | |
| } | |
| // For non-existent paths, fall back to lexical containment check against the | |
| // realpath of repoRoot. This still prevents obvious escapes like "../../..". | |
| if (!isPathInside(rootReal, resolved)) { | |
| throw new Error(`Wiki directory must be inside the repository root: ${resolved}`); | |
| } | |
| return resolved; | |
| } | |
| const candidates = [path.join(repoRoot, 'docs', 'wiki'), path.join(repoRoot, 'wiki')]; | |
| for (const c of candidates) { | |
| if (!fs.existsSync(c)) continue; | |
| const cReal = fs.realpathSync(c); | |
| if (isPathInside(rootReal, cReal)) { | |
| return cReal; | |
| } | |
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 SUGGESTION: 函数参数和返回值缺少类型定义
函数签名使用 any[] 类型,丢失了 TypeScript 的类型安全优势
建议: 定义明确的接口类型来描述 rows 的结构
| export function filterWorkspaceRowsByLang(rows: any[], langSel: string): any[] { | |
| interface WorkspaceRow { | |
| file: string; | |
| // 其他字段... | |
| } | |
| export function filterWorkspaceRowsByLang(rows: WorkspaceRow[], langSel: string): WorkspaceRow[] { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
parseInt 在处理非数字字符串时返回 NaN(如
--limit abc),后续 executeHandler 收到 NaN 可能导致静默错误或意外行为建议: 添加验证逻辑,过滤无效值:
const limit = /^-?\d+$/.test(options.limit) ? parseInt(options.limit, 10) : undefined;
或使用默认值兜底