fix(files): optimize exact path and directory search - #1778
techotaku39 wants to merge 6 commits into
Conversation
There was a problem hiding this comment.
Findings
- None.
Summary
- Review mode: initial
- No issues found in the added or modified lines. Residual testing risk: the route tests mock the CLI RPC boundary and do not cover a successful Windows directory search or absolute/UNC path handling through a real Windows CLI.
Testing
- Not run (automation). Suggested coverage: a Windows integration test for successful trailing-separator directory search plus drive-qualified and UNC absolute paths.
HAPI Bot
…xact-path # Conflicts: # web/src/routes/sessions/files.test.tsx
There was a problem hiding this comment.
Findings
- [Major] Keep path-qualified file mentions searchable while typing —
GET /filesis also used by the Codex/Copilot @-mention autocomplete atweb/src/router.tsx:705. The new exact-path branch intercepts every non-wildcard query containing a separator, so partial input such as@src/roustats the literal incomplete path and returns no file suggestions until the full path is entered. Evidence:hub/src/web/routes/git.ts:434.
Suggested fix:const fileQuery = /[\\/]/.test(search) && !/[*?]/.test(search) ? `*${search}*` : search const response = await api.searchSessionFiles(sessionId, fileQuery, 50)
Summary
Review mode: follow-up after new commits
- One Major regression found: path-qualified Codex/Copilot file mentions lose incremental autocomplete.
Testing
- Not run (automation). Add a composer autocomplete test proving a partial path such as
@src/roustill returns matching files.
HAPI Bot
| }) | ||
| } | ||
|
|
||
| const exactFileSearchPath = getExactFileSearchPath(normalizedQuery, sessionPath) |
There was a problem hiding this comment.
[MAJOR] Path-qualified file mentions lose incremental suggestions
This branch handles every non-wildcard path query as exact. The same endpoint is called by Codex/Copilot mention autocomplete (web/src/router.tsx:705), so typing @src/rou now stats src/rou and returns no suggestions until a complete file path is entered.
Suggested fix:
const fileQuery = /[\\/]/.test(search) && !/[*?]/.test(search)
? `*${search}*`
: search
const response = await api.searchSessionFiles(sessionId, fileQuery, 50)Add an autocomplete regression test for a partial path.
There was a problem hiding this comment.
Findings
- [Major] Reject symlink/junction escapes before recursive path search — the new directory branch treats the result of
statFilesas authorization and then passes that path explicitly to ripgrep. However,statFilesnow derivestypewithstat(), which follows symlinks/junctions, while its containment check is only lexical. A workspace entry such aslink -> /outsideis therefore accepted as a directory, andrg --files -- linkenumerates files outside the session workspace. Evidencehub/src/web/routes/git.ts:395,cli/src/modules/common/handlers/directories.ts:114.
Suggested fix:Apply the same canonical containment check at the CLI ripgrep execution boundary immediately before spawning, so a symlink swap cannot bypass the earlier metadata check.const workspaceRealPath = await realpath(workingDirectory) const targetRealPath = await realpath(resolve(workingDirectory, path)) if (!validatePath(targetRealPath, workspaceRealPath).valid) { return { path } } const stats = await stat(targetRealPath)
Summary
- Review mode: follow-up after new commits
- One Major security regression found. The prior incremental file-mention finding is addressed by the latest commit.
Testing
- Not run (automation). Add a regression test with an in-workspace symlink/junction targeting an outside directory and assert that a trailing-separator search returns no files and never invokes ripgrep for that target.
HAPI Bot
|
|
||
| const directoryArgs = ['--files'] | ||
| if (directorySearchPath) { | ||
| directoryArgs.push('--', directorySearchPath) |
There was a problem hiding this comment.
[MAJOR] Recursive path search can escape through symlinks/junctions
This explicit search root is authorized only by the preceding statFiles result. On the CLI, that handler uses lexical validatePath() and then stat(); stat() follows an in-workspace symlink/junction to an outside directory and reports type: 'directory'. Ripgrep also follows a directory symlink when it is supplied as an explicit path, so a query such as link/ can enumerate outside filenames and return their metadata.
Suggested fix:
const workspaceRealPath = await realpath(workingDirectory)
const targetRealPath = await realpath(resolve(workingDirectory, path))
if (!validatePath(targetRealPath, workspaceRealPath).valid) {
return { path }
}
const stats = await stat(targetRealPath)Perform the canonical containment check again at the CLI ripgrep execution boundary immediately before spawning (or use a dedicated scoped-directory-search RPC), and add a symlink/junction escape test.
There was a problem hiding this comment.
Findings
- [Minor] Resolve the canonical workspace root once per stat batch -
statFilescalls the new helper for every result, and the helper repeatsrealpath(workingDirectory)each time. A default search can return 200 paths (maximum 500), so every ordinary fuzzy search now queues hundreds of identical filesystem resolutions in addition to the required per-file lookup; this can materially increase latency on network or slow filesystems. Evidencecli/src/modules/common/handlers/directories.ts:114,cli/src/modules/common/pathSecurity.ts:46.
Suggested fix:const resolvedWorkingDirectory = await realpath(workingDirectory) const entries = await Promise.all(data.paths.map(async (path) => { const resolvedTarget = await realpath(resolve(workingDirectory, path)) if (!validatePath(resolvedTarget, resolvedWorkingDirectory).valid) { return { path } } const stats = await stat(resolvedTarget) // ... }))
Summary
- Review mode: follow-up after new commits
- One Minor performance regression found. The previous symlink/junction containment finding is resolved in the latest diff. Residual risk: Windows junction behavior was reviewed statically only.
Testing
- Not run (automation). At review time:
integration,windows-codex-mcp, anddrift-gatepassed; the maintestcheck was still pending.
HAPI Bot
| const entries = await Promise.all(data.paths.map(async (path) => { | ||
| try { | ||
| const stats = await stat(resolve(workingDirectory, path)) | ||
| const resolvedPath = await resolveRealPathWithinWorkingDirectory(path, workingDirectory) |
There was a problem hiding this comment.
[MINOR] Resolve the canonical workspace root once per batch
This helper invocation repeats realpath(workingDirectory) for every path. The file-search route requests metadata for up to 200 files by default and 500 at the API limit, so the change adds the same root lookup hundreds of times per fuzzy search, on top of each target's realpath and stat. Hoist the canonical root out of the per-path callback (or pass it into the helper).
Suggested fix:
const resolvedWorkingDirectory = await realpath(workingDirectory)
const entries = await Promise.all(data.paths.map(async (path) => {
const resolvedTarget = await realpath(resolve(workingDirectory, path))
if (!validatePath(resolvedTarget, resolvedWorkingDirectory).valid) {
return { path }
}
const stats = await stat(resolvedTarget)
// ...
}))There was a problem hiding this comment.
Findings
- No Blocker, Major, Minor, or Nit findings.
Summary
- Review mode: follow-up after new commits
- Reviewed the latest PR diff in full. The prior repeated canonical-workspace lookup finding is resolved by reusing one realpath(workingDirectory) result per stat batch.
- No inadequate coverage found for the changed exact-path, recursive-directory, symlink-containment, query-state, or empty-state behavior.
Testing
- Not run: the review security policy prohibits executing PR code. GitHub test check at posting time: SUCCESS.
HAPI Bot
…xact-path # Conflicts: # web/src/routes/sessions/files.test.tsx
There was a problem hiding this comment.
Findings
- No actionable issues found in the latest PR diff.
Summary
Review mode: follow-up after new commits
Reviewed all 18 changed files at d4f2d098aa5e23e24697dfb05a80bd0b023d294d, using the prior review and comparison diff as context. No findings met the reporting confidence threshold. Residual risk: platform-specific filesystem behavior remains unverified by this review.
Testing
- Not run (automation): the review security policy prohibits executing PR code. Main CI
testcheck was still in progress when checked. - Suggested coverage: successful Windows directory searches and allowed in-workspace symlinks through the CLI/RPC path; current directory-route tests mock RPC results.
HAPI Bot
Summary
/or Windows\.Problem / Motivation
File preview searches previously routed path-like queries through workspace-wide ripgrep scanning. This made exact path searches unnecessarily slow and allowed unrelated matching paths to appear.
Directory queries ending with a path separator were also treated as file paths instead of scoped directory searches.
User Impact
src/foo/andsrc\foo\recursively list files below that directory.git.tsretain global fuzzy matching.Validation
bun typecheck— passed.bun test hub/src/web/routes/git.test.ts— 13 passed.bun test cli/src/modules/common/handlers/directories.test.ts— 4 passed.bun run test -- src/routes/sessions/files.test.tsxfromweb/— 4 passed.pwsh -NoProfile -File .\scripts\Invoke-HapiTaskPlaywright.ps1 -Name fix-file-search-exact-path -Suite Root -TestArgs terminal-wrap-fidelity.spec.ts— 2 passed.bun run build— passed.git diff --check— passed.Risk / Rollback
acda8257to roll back.Related Issues
None
AI Disclosure
Implemented and validated with OpenAI Codex (GPT-5.6).