fix(mcp): address Codex P1/P2 review issues in deferred scan#349
Conversation
Covers three scenarios before any MCP merge: 1. issue-346 regression: spawn from cwd=/, roots handshake, tools verify 2. Normal mode: explicit positional root, immediate scan, no roots needed 3. No-roots client: spawn from /, stays alive gracefully with 0 files All 17 tests pass. Documented in AGENTS.md as pre-merge verification step. Generated with [Devin](https://cli.devin.ai/docs) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
P1 #1 (triggered reset): reset triggered=false in triggerScanFromRoots when getDataDir fails, so next valid roots/list response can retry instead of silently never scanning. P1 #2 (joinable scan thread): store the spawned scanBg thread in DeferredScan.scan_thread; join it during shutdown instead of detaching. This eliminates the use-after-free where the background scan thread could touch store/explorer after mainImpl returns. Also fix the deferred watcher: replace watcherDeferredLoop (which only waited for scan_done) to run watcher.incrementalLoop directly after scan completes, removing the second detached thread spawn from triggerScanFromRoots. Both the scan thread and the watcher thread are now fully joinable. P2 (explicit root): DeferredScan.resolved_root captures the abs_root accepted from roots/list so watcherDeferredLoop can pass it to incrementalLoop without any extra allocation. All 17 E2E tests pass. zig build test passes. Generated with [Devin](https://cli.devin.ai/docs) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Benchmark Regression ReportThresholds: 10.00% and 50,000 ns absolute delta
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8f9f401b54
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| std.process.exit(1); | ||
| }; | ||
| const mcp_deferred_root = std.mem.eql(u8, cmd, "mcp") and std.mem.eql(u8, root, "."); | ||
| const mcp_deferred_root = std.mem.eql(u8, cmd, "mcp") and std.mem.eql(u8, root, ".") and !root_is_explicit; |
There was a problem hiding this comment.
Preserve deferred scan for
${workspaceFolder} fallback
Adding !root_is_explicit to the deferred-root gate makes codedb "${workspaceFolder}" mcp run immediate indexing after the fallback rewrites the literal placeholder to ".", instead of waiting for roots/list. In clients that do not expand ${workspaceFolder} and spawn from / (the issue-346 context), this now resolves to /, fails isIndexableRoot, and exits before MCP roots negotiation can recover the real project path.
Useful? React with 👍 / 👎.
Summary
Addresses all three Codex code-review issues flagged on PR #348 before merging
release/0.2.579→main.P1 #1 — Reset
triggeredwhentriggerFnfails silentlytriggerScanFromRootsnow resetstriggered=falseifgetDataDirfailsP1 #2 — Joinable scan and watcher threads (was use-after-free)
DeferredScannow hasscan_thread: ?std.Thread = nullandresolved_root: []const u8 = ""triggerScanFromRootsstores thescanBghandle inctx.scan_threadinstead of detaching itwatcherDeferredLoopnow runswatcher.incrementalLoopdirectly afterscan_donefires — removing the second detached thread spawndeferred.scan_threadbefore returning, eliminating the use-after-free ofstore/explorerP2 —
codedb . mcptreated as deferred (regression)root_is_explicit: boolflag inmain.zig; set when the user passes an explicit path argumentmcp_deferred_rootcheck now gates on!root_is_explicit—codedb . mcpscans immediately as expectedTest plan
zig build test— all unit tests passpython3 scripts/e2e_mcp_test.py— 17/17 pass (S1 deferred/roots, S2 explicit-root, S3 no-roots-client)Generated with Devin