fix(core): guard searchProjectContent and listSessions against readdir TOCTOU - #213
Conversation
…r TOCTOU A project's folder can vanish between listProjects() returning it and a later per-project readdir (cross-PC sync, manual deletion) — the exact TOCTOU window listProjects itself already guards against (Issue #103). searchProjectContent (search.ts) lacked this guard: an ENOENT there would kill the entire searchSessions Effect via Effect.all's fail-fast semantics, losing content-search results from every other project. Flagged as an Internal Code Review finding on PR #163, deferred as out of scope for that PR (only `export` keywords were touched there). listSessions (crud.ts) has the identical unguarded readdir, and is hit even earlier — searchSessions calls it during Phase 1 title search, unconditionally, before Phase 2 content search ever runs. A regression test written against only the search.ts fix caught this: the test still failed with the same class of error, just originating from listSessions instead. Both call sites now mirror listProjects' existing catchAll pattern (narrow to ENOENT/ENOTDIR, propagate other errors like EACCES). Follow-up tracked separately: #211 (hermetic webview HTTP test fixture migration — the other PR #163 finding) Signed-off-by: DrumRobot <drumrobot43@gmail.com>
Two problems found while writing the regression test for the previous
commit's fix:
1. Applying the same guard to listSessions() (crud.ts) broke its existing
contract: an MCP integration test asserts listSessions('non-existent
-project') should throw, since callers can pass an arbitrary/typo'd
project name and silently returning [] would hide that bug. The TOCTOU
guard only makes sense when a project name came from a just-completed
listProjects() enumeration (so it's known to have existed a moment
ago) — that context is specific to searchSessions' Phase 1 title
search, not to listSessions itself. Reverted crud.ts; the guard now
wraps the listSessions(project.name) call site inside search.ts only.
2. isMissingFolderError(error) returned false even for a genuine ENOENT,
because listSessions' plain-form Effect.tryPromise(() => fs.readdir(...))
(no explicit `catch` mapping) wraps rejections in Effect's
UnknownException, with the real NodeJS.ErrnoException nested under
`.error`/`.cause` rather than at the top level. isMissingFolderError
now unwraps both, so it works regardless of which tryPromise form the
effect being guarded happens to use.
Full workspace test suite (core 505, mcp 26, vscode-extension 67, web 102)
green after this change.
Signed-off-by: DrumRobot <drumrobot43@gmail.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughSession search now skips projects whose folders disappear during content or title searches. It propagates unrelated filesystem errors, classifies wrapped missing-folder errors, logs skipped projects, and adds integration coverage. The pre-push helper preserves command status and output. ChangesSession search resilience
Estimated code review effort: 3 (Moderate) | ~20 minutes Mergeability Score: ⚪ Minimal · up to The change narrowly guards disappearing project folders during search while preserving the existing error contract for direct session listing, with all reported checks passing; no actionable merge-blocking risk remains beyond normal review. Sequence Diagram(s)sequenceDiagram
participant Search
participant FileSystem
participant Logger
Search->>FileSystem: Read project directory
FileSystem-->>Search: Return entries or error
alt ENOENT or ENOTDIR
Search->>Logger: Log skipped project
Search-->>Search: Continue search
else Other filesystem error
Search-->>Search: Propagate error
end
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
packages/core/src/paths.integration.test.tsESLint skipped: missing config or dependency (missing-dependency). The ESLint configuration references a package that is not available in the sandbox. packages/core/src/session/search.test.tsESLint skipped: the ESLint configuration for this file references a package that is not available in the sandbox. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/core/src/session/search.test.ts`:
- Around line 313-324: Update the fs.readdir mocks in
packages/core/src/session/search.test.ts at lines 313-324 and 349-364 to reject
on the third matching call, targeting the content-search invocation after
listProjects and listSessions. Add a separate title-search missing-folder test
so title-search ENOENT handling is covered independently.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: ae10d2c9-bff9-4bca-823b-0e210a9e149b
📒 Files selected for processing (3)
packages/core/src/session/search.test.tspackages/core/src/session/search.tspackages/core/src/utils.ts
DrumRobot
left a comment
DrumRobot
left a comment
There was a problem hiding this comment.
Internal Code Review — requesting-code-review
Dispatched because Copilot Code Review is unavailable on this org (
orgs/es6kr/copilot/billing→seat_breakdown.total: 0,seat_management_setting: disabled) — auto-fallback per consolidate Step 2.4, second independent perspective alongside CodeRabbit.
Strengths
- Correct architectural placement of the guard. Git history shows commit
613e794first tried putting the guard insidelistSessions(crud.ts), thenee64bfcreverted that because it brokepackages/mcp/src/__tests__/tools.test.ts(should throw error for non-existent project), and correctly relocated the guard to the call site insearch.ts's title-search phase instead. That MCP test still exists and still asserts throw-on-missing-project; the fullpackages/mcpsuite (26 tests) passes at HEAD. isMissingFolderError'sUnknownExceptionunwrap is correct and necessary.crud.ts'slistSessionsstill uses the plainEffect.tryPromise(() => fs.readdir(...))form, which Effect wraps inUnknownException(the real error lands under.error/.cause). Confirmed via instrumented run: a genuine ENOENT fromlistSessionsreally does arrive at the guard wrapped, and the unwrap correctly extractscode: 'ENOENT'.searchProjectContent's own guard (search.ts:82-90) is implemented correctly — verified in isolation (see below): it correctly narrows to ENOENT/ENOTDIR and correctly propagates EACCES.- Full
packages/coresuite (28 files / 505 tests),packages/mcpsuite (3 files / 26 tests),tsc --noEmit, andeslinton the touched files all pass clean at HEAD.
Issues
Critical (Must Fix)
None found.
Important (Should Fix)
packages/core/src/session/search.test.ts:334-369— the EACCES test never reachessearchProjectContent; it validates the title-search phase's passthrough instead, under a misleading name. Verified via an isolated, instrumented reproduction (stack-trace markers at eachreaddircall site + an entry marker insidesearchProjectContent): for this test, call #1 (listProjects's per-entry readdir) succeeds, call #2 (listSessions, insidesearchSessions's title-search Phase 1) rejects with EACCES, andsearchProjectContentis never entered —Effect.gen'syield* Effect.all(titleSearchEffects, ...)must fully resolve before Phase 2 (content search) is reached, so with only one project, Phase 1's failure aborts the whole generator before Phase 2 code runs..rejects.toThrow()still passes, but for the wrong reason.- Fix, verified empirically: bump
deniedCallCount > 1→deniedCallCount > 2(isolates call #3,searchProjectContent's own readdir). Applying this exact change and rerunning confirmssearchProjectContentis now entered and the rejection now originates from inside its own guard — the underlying implementation is correct, this is a pure test-isolation gap, not a production defect.
- Fix, verified empirically: bump
- No test isolates the title-search phase's own guard (
search.ts:219-241). Both new tests live underdescribe('searchProjectContent TOCTOU safety', ...). The ENOENT test (lines 313-324) legitimately requires both guards to be correct to pass (confirmed by instrumentation —searchProjectContent's debug log and entry marker both fire for it), so it's not invalid, just not isolated to "content-search only" as the describe name implies. After fixing issue 1 above, the only (accidental) coverage ofsearchSessions's title-search EACCES passthrough disappears — recommend a smalldescribe('searchSessions title-search TOCTOU safety', ...)block with its own dedicated ENOENT-skip and EACCES-propagate tests, mirroring the existing content-search pattern.
Minor (Nice to Have)
searchBySessionId(search.ts:130-141, not touched by this PR) still uses a broadEffect.catchAll(() => Effect.succeed([]))on its own per-project readdir, unconditionally swallowing all errors including EACCES — inconsistent with the narrow-guard convention this PR just established two call sites away in the same file. Pre-existing, not a regression; worth a follow-up for consistency.- When content search (Phase 2) hits a genuine non-ENOENT error on one project among several,
Effect.all(contentSearchEffects, {concurrency: 5})fails fast and the wholesearchSessionscall rejects — losing the already-successful Phase 1 title results too. Pre-existing behavior (mirrorslistProjects's "propagate non-ENOENT loudly" philosophy on purpose), not introduced by this PR. Worth a note for a future PR (e.g.Effect.allwith{ mode: 'either' }+ partial-result reporting), not blocking here.
CodeRabbit finding cross-check
CodeRabbit flagged the same underlying issue on search.test.ts:313-324 / 349-364 ("Make the mocks target the content-search readdir call... EACCES rejects during title search and never tests searchProjectContent") at 🟡 Minor. I independently reproduced and confirmed the claim (see Important #1 above) — it is VALID, and I'd elevate it to 🟠 Important: the isolated reproduction shows searchProjectContent's own non-ENOENT passthrough currently has zero intentional test coverage (the only place it looked covered was accidental). Precise fix + a note on the title-search test gap this fix would otherwise open (Important #2) are both above.
Recommendations
- Apply the two-part fix: bump the EACCES test's threshold to isolate call #3, and add a dedicated title-search-guard test block so neither guard's non-ENOENT passthrough is left uncovered.
- Low-priority follow-up (separate PR): align
searchBySessionId's error handling with the narrow ENOENT/ENOTDIR convention.
Assessment
Ready to merge? With fixes
Reasoning: The production code (search.ts, utils.ts) is correct and thoroughly verified — searchProjectContent's EACCES passthrough works exactly as intended once exercised directly, and the full core (505) + mcp (26) suites plus typecheck/lint are all green. The one real gap is test-hygiene: one of the two new tests validates the wrong code path under a misleading name, leaving the guard it claims to test without real coverage. This is a same-PR, low-risk, mechanical fix (one number change + one small new test).
AI Review Summary — receiving-code-review
Verification note (finding 1/2): independently reproduced by the Internal Code Review via an isolated, instrumented copy of the package (call-site stack traces + an entry marker inside Fix applied (commit Production-code verification: full PR Test Plan: 7/7 items checked ( ✅ MERGED 2026-08-13 — squash commit |
…S test The EACCES test for searchProjectContent's non-ENOENT passthrough guard was failing one readdir call too early: deniedCallCount > 1 let the error hit Phase 1's listSessions() call (call 2 of 3: listProjects sessionCount -> title-search listSessions -> searchProjectContent), so the test actually exercised title-search's own catchAll re-throwing EACCES and aborting Effect.all before Phase 2 was ever entered. searchProjectContent's own EACCES-passthrough guard had zero intentional coverage. - Bump the threshold to deniedCallCount > 2 so the failure lands on searchProjectContent's own readdir call (call 3), isolating the test to that guard specifically. - Add a dedicated "searchSessions title-search TOCTOU safety" test that exercises the call-2 case directly, so the guard doesn't lose its only (accidental) coverage now that the searchProjectContent test no longer passes through it. Addresses the pending CodeRabbit + Internal Code Review findings from PR review (Important: independently reproduced test-isolation gap). Signed-off-by: Hayoung Jeong <drumrobot43@gmail.com>
husky runs .husky/pre-push under `sh -e` (errexit). The previous
`out=$("$@" 2>&1); status=$?` form let the assignment inherit the wrapped
command's failure exit code, so a failing command killed the script on
that line before `status=$?` or `echo "$out"` ever ran — every pre-push
failure surfaced only as "husky - pre-push script failed (code 1)" with
no diagnostic text, regardless of what actually failed inside build/
typecheck/test.
Change the assignment to `out=$("$@" 2>&1) || status=$?` so the failing
command is no longer the last element of the statement, keeping errexit
from firing on that line while still capturing the real exit code.
Verified in isolation under `sh -e`: a function that prints diagnostic
lines and returns non-zero now has its output correctly echoed by
run_quiet, both for a bare `false` (no output) and for output-producing
failures.
Signed-off-by: Hayoung Jeong <drumrobot43@gmail.com>
'returns null for non-existent project' occasionally exceeds vitest's default 5000ms test timeout on the dynamic re-import used to pick up mocked modules, unrelated to this PR's own changes (observed on unmodified main too). Raise the per-test timeout to 15000ms rather than changing the test's behavior. Signed-off-by: Hayoung Jeong <drumrobot43@gmail.com>
1 participant
Summary
Guards two TOCTOU windows where a project's folder disappearing mid-search (cross-PC sync, manual deletion) would kill the entire
searchSessionsresult instead of just skipping that project — mirroring the patternlistProjects()already uses (Issue #103).searchProjectContent(content search, Phase 2) — flagged as an Internal Code Review finding on PR test: add coverage for session search and webview session page #163, deferred as out of scope there (onlyexportkeywords were touched in that PR).listSessions(project.name)call site insidesearchSessions' title search (Phase 1) — found while writing the regression test for the fix above. It's hit earlier and more easily than Phase 2, since Phase 1 runs unconditionally.Why not guard
listSessionsitself?First attempt did exactly that (mirroring
listProjects' guard directly insidecrud.ts), but the full workspace test suite caught a real regression: an MCP integration test assertslistSessions('non-existent-project')should throw — a legitimate contract, since a caller passing an arbitrary/typo'd project name should see an error, not a silently-empty result. The TOCTOU guard only makes sense when the project name came from alistProjects()enumeration moments earlier (so it's known to have existed) — that context is specific tosearchSessions' Phase 1, so the guard now wraps thelistSessions(project.name)call site there instead of the shared function.Also fixed:
isMissingFolderErrordidn't unwrapEffect'sUnknownExceptionlistSessions'sEffect.tryPromise(() => fs.readdir(...))uses the plain-function form (no explicit{ try, catch }), so a rejection gets wrapped in Effect'sUnknownExceptionwith the realNodeJS.ErrnoExceptionnested under.error/.cause.isMissingFolderErrornow unwraps both, so it works regardless of whichtryPromiseform the guarded effect happens to use.Test plan
pnpm build(core) — cleanpnpm typecheck(core) — cleanpnpm lint(core) — 0 new warnings (17 pre-existing, all in untouched files)search.test.ts) — Red-Green verified: fail without the fix (UnknownException), pass with itFollow-up
The other PR #163 finding (hermetic webview HTTP test fixture migration) is tracked separately: #211
Summary by CodeRabbit
Bug Fixes
Tests