fix: route MCP requests per project root#65
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2189ba0141
ℹ️ 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".
src/index.ts
Outdated
| if ( | ||
| project.indexState.status === 'indexing' || | ||
| project.indexState.status === 'ready' || | ||
| project.stopWatcher | ||
| ) { |
There was a problem hiding this comment.
initProject can be entered concurrently (e.g., main() calls it after server.connect, and a tool request can call it through resolveProjectForTool at the same time), but this guard only checks status/stopWatcher before any lock is set. Because the function then does awaited migration/reindex checks while state is still idle, two callers can both pass this check and each start performIndexing and startFileWatcher, which can trigger duplicate full indexes and leave an extra watcher running.
Useful? React with 👍 / 👎.
Greptile SummaryThis PR replaces the single global Key findings:
Confidence Score: 2/5
Sequence DiagramsequenceDiagram
participant C as MCP Client
participant S as Server (index.ts)
participant PS as project-state.ts
participant IP as initProject()
participant T as Tool Handler
C->>S: tools/call { name, arguments: { project_directory?, ... } }
S->>S: normalizeArgs()
alt tool not in TOOLS registry
S->>T: dispatchTool(name, args, primaryProject)
T-->>C: result
else known tool
S->>S: resolveProjectForTool(args)
alt project_directory provided
S->>S: parseProjectDirectory(args.project_directory)
alt clientRootsEnabled && not in knownRoots
S-->>C: error { errorCode: "unknown_project" }
else
S->>PS: registerKnownRoot(path) [if new]
S->>PS: getOrCreateProject(rootPath)
PS-->>S: ProjectState
S->>IP: initProject(rootPath, debounceMs)
IP->>IP: guard check (status/stopWatcher)
IP->>IP: migrateToNewStructure()
IP->>IP: shouldReindex()
IP->>IP: void performIndexing(project) [if needed]
IP->>IP: startFileWatcher()
IP-->>S: void
end
else no project_directory
alt availableRoots.length !== 1
S-->>C: error { errorCode: "ambiguous_project", availableRoots: [...] }
else single root
S->>PS: getOrCreateProject(rootPath)
S->>IP: initProject(rootPath, debounceMs)
IP-->>S: void
end
end
alt INDEX_CONSUMING tool
S->>S: ensureValidIndexOrAutoHeal(project)
alt IndexCorruptedError
S->>S: void performIndexing(project) [background]
S-->>C: { status: "indexing", message: "retry shortly" }
end
end
S->>T: dispatchTool(name, args, createToolContext(project))
T-->>S: ToolResponse
S->>S: inject indexSignal into response [if present]
S-->>C: ToolResponse
end
Last reviewed commit: 2189ba0 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
src/index.ts
Outdated
| async function initProject(rootPath: string, debounceMs: number): Promise<void> { | ||
| const project = getOrCreateProject(rootPath); | ||
|
|
||
| // Skip if already initialized | ||
| if ( | ||
| project.indexState.status === 'indexing' || | ||
| project.indexState.status === 'ready' || | ||
| project.stopWatcher | ||
| ) { | ||
| return; | ||
| } | ||
|
|
||
| // Migrate legacy structure | ||
| try { | ||
| const legacyPaths = makeLegacyPaths(project.rootPath); | ||
| const migrated = await migrateToNewStructure(project.paths, legacyPaths); | ||
| if (migrated && process.env.CODEBASE_CONTEXT_DEBUG) { | ||
| console.error(`[DEBUG] Migrated to .codebase-context/ structure: ${project.rootPath}`); | ||
| } | ||
| } catch { | ||
| // Non-fatal | ||
| } | ||
|
|
||
| // Check if indexing is needed | ||
| const needsIndex = await shouldReindex(project.paths); | ||
| if (needsIndex) { | ||
| if (process.env.CODEBASE_CONTEXT_DEBUG) { | ||
| console.error(`[DEBUG] Starting indexing: ${project.rootPath}`); | ||
| } | ||
| void performIndexing(project); | ||
| } else { | ||
| if (process.env.CODEBASE_CONTEXT_DEBUG) { | ||
| console.error(`[DEBUG] Index found. Ready: ${project.rootPath}`); | ||
| } | ||
| project.indexState.status = 'ready'; | ||
| project.indexState.lastIndexed = new Date(); | ||
| } | ||
|
|
||
| // Start file watcher | ||
| project.stopWatcher = startFileWatcher({ | ||
| rootPath: project.rootPath, | ||
| debounceMs, | ||
| onChanged: () => { | ||
| const shouldRunNow = project.autoRefresh.onFileChange( | ||
| project.indexState.status === 'indexing' | ||
| ); | ||
| if (!shouldRunNow) { | ||
| if (process.env.CODEBASE_CONTEXT_DEBUG) { | ||
| console.error( | ||
| `[file-watcher] Index in progress — queueing auto-refresh: ${project.rootPath}` | ||
| ); | ||
| } | ||
| return; | ||
| } | ||
| if (process.env.CODEBASE_CONTEXT_DEBUG) { | ||
| console.error( | ||
| `[file-watcher] Changes detected — incremental reindex starting: ${project.rootPath}` | ||
| ); | ||
| } | ||
| void performIndexing(project, true); | ||
| } | ||
| }); | ||
| } |
There was a problem hiding this comment.
initProject has a concurrent-initialization race condition. The guard check at lines 824-829 evaluates before several await points (migration at line 835, shouldReindex at line 844). If two tool calls arrive concurrently for the same newly-registered root, both will pass the guard (status is 'idle', stopWatcher is undefined), proceed through the async section, and both will fire void performIndexing(project), launching two overlapping indexers against the same root. The second startFileWatcher call then overwrites project.stopWatcher, leaking the first watcher.
Fix by setting project.indexState.status = 'indexing' synchronously before the first await, so concurrent calls see a non-idle status and skip initialization:
| async function initProject(rootPath: string, debounceMs: number): Promise<void> { | |
| const project = getOrCreateProject(rootPath); | |
| // Skip if already initialized | |
| if ( | |
| project.indexState.status === 'indexing' || | |
| project.indexState.status === 'ready' || | |
| project.stopWatcher | |
| ) { | |
| return; | |
| } | |
| // Migrate legacy structure | |
| try { | |
| const legacyPaths = makeLegacyPaths(project.rootPath); | |
| const migrated = await migrateToNewStructure(project.paths, legacyPaths); | |
| if (migrated && process.env.CODEBASE_CONTEXT_DEBUG) { | |
| console.error(`[DEBUG] Migrated to .codebase-context/ structure: ${project.rootPath}`); | |
| } | |
| } catch { | |
| // Non-fatal | |
| } | |
| // Check if indexing is needed | |
| const needsIndex = await shouldReindex(project.paths); | |
| if (needsIndex) { | |
| if (process.env.CODEBASE_CONTEXT_DEBUG) { | |
| console.error(`[DEBUG] Starting indexing: ${project.rootPath}`); | |
| } | |
| void performIndexing(project); | |
| } else { | |
| if (process.env.CODEBASE_CONTEXT_DEBUG) { | |
| console.error(`[DEBUG] Index found. Ready: ${project.rootPath}`); | |
| } | |
| project.indexState.status = 'ready'; | |
| project.indexState.lastIndexed = new Date(); | |
| } | |
| // Start file watcher | |
| project.stopWatcher = startFileWatcher({ | |
| rootPath: project.rootPath, | |
| debounceMs, | |
| onChanged: () => { | |
| const shouldRunNow = project.autoRefresh.onFileChange( | |
| project.indexState.status === 'indexing' | |
| ); | |
| if (!shouldRunNow) { | |
| if (process.env.CODEBASE_CONTEXT_DEBUG) { | |
| console.error( | |
| `[file-watcher] Index in progress — queueing auto-refresh: ${project.rootPath}` | |
| ); | |
| } | |
| return; | |
| } | |
| if (process.env.CODEBASE_CONTEXT_DEBUG) { | |
| console.error( | |
| `[file-watcher] Changes detected — incremental reindex starting: ${project.rootPath}` | |
| ); | |
| } | |
| void performIndexing(project, true); | |
| } | |
| }); | |
| } | |
| async function initProject(rootPath: string, debounceMs: number): Promise<void> { | |
| const project = getOrCreateProject(rootPath); | |
| // Skip if already initialized | |
| if ( | |
| project.indexState.status === 'indexing' || | |
| project.indexState.status === 'ready' || | |
| project.stopWatcher | |
| ) { | |
| return; | |
| } | |
| // Mark immediately before any await so concurrent calls see a non-idle status | |
| project.indexState.status = 'indexing'; | |
| // Migrate legacy structure | |
| try { | |
| const legacyPaths = makeLegacyPaths(project.rootPath); | |
| const migrated = await migrateToNewStructure(project.paths, legacyPaths); |
src/index.ts
Outdated
| indexSignal = await ensureValidIndexOrAutoHeal(); | ||
| if (indexSignal.action === 'rebuild-failed') { | ||
| indexSignal = await ensureValidIndexOrAutoHeal(project); | ||
| if (indexSignal.action === 'rebuild-started' || indexSignal.action === 'rebuild-failed') { |
There was a problem hiding this comment.
ensureValidIndexOrAutoHeal only ever returns action: 'rebuild-started' (or 'served'); it never generates 'rebuild-failed'. This condition is unreachable dead code and should be removed to avoid confusing future readers.
| if (indexSignal.action === 'rebuild-started' || indexSignal.action === 'rebuild-failed') { | |
| if (indexSignal.action === 'rebuild-started') { |
| if (requestedProjectDirectory) { | ||
| const requestedRootKey = normalizeRootKey(requestedProjectDirectory); | ||
| const knownRootPath = knownRoots.get(requestedRootKey); | ||
|
|
||
| if (clientRootsEnabled && availableRoots.length > 0 && !knownRootPath) { | ||
| return { | ||
| ok: false, | ||
| response: buildProjectSelectionError( | ||
| 'unknown_project', | ||
| 'Requested project is not part of the active MCP roots.' | ||
| ) | ||
| }; | ||
| } | ||
|
|
||
| const rootPath = knownRootPath ?? registerKnownRoot(requestedProjectDirectory); | ||
| const project = getOrCreateProject(rootPath); | ||
| await initProject(project.rootPath, watcherDebounceMs); | ||
| return { ok: true, project }; | ||
| } |
There was a problem hiding this comment.
Dynamically registered project roots via project_directory are permanently added to knownRoots and never evicted during the session (unless syncKnownRoots is called by the MCP client). Once a second distinct root is registered this way, availableRoots.length > 1, causing all subsequent tool calls without project_directory to fail with ambiguous_project error — locking the session into ambiguity mode for the remainder of its lifetime.
This behavior is tested and functional, but the session-lock behavior is not surfaced in the project_directory parameter description. Consider either:
- Documenting this clearly in the
project_directoryschema description (insrc/tools/index.ts), or - Scoping dynamically registered roots to the current request only — i.e., skip calling
registerKnownRootfor ad-hocproject_directorylookups and resolve them in-process without mutating session state.
| async function resolveProjectForTool(args: Record<string, unknown>): Promise<ProjectResolution> { | ||
| const requestedProjectDirectory = parseProjectDirectory(args.project_directory); | ||
| const availableRoots = getKnownRootPaths(); | ||
|
|
||
| if (requestedProjectDirectory) { | ||
| const requestedRootKey = normalizeRootKey(requestedProjectDirectory); | ||
| const knownRootPath = knownRoots.get(requestedRootKey); | ||
|
|
||
| if (clientRootsEnabled && availableRoots.length > 0 && !knownRootPath) { | ||
| return { | ||
| ok: false, | ||
| response: buildProjectSelectionError( | ||
| 'unknown_project', | ||
| 'Requested project is not part of the active MCP roots.' | ||
| ) | ||
| }; | ||
| } | ||
|
|
||
| const rootPath = knownRootPath ?? registerKnownRoot(requestedProjectDirectory); | ||
| const project = getOrCreateProject(rootPath); | ||
| await initProject(project.rootPath, watcherDebounceMs); | ||
| return { ok: true, project }; | ||
| } |
There was a problem hiding this comment.
No existence check is performed on user-supplied project_directory before registering and initializing it. If the path doesn't exist:
migrateToNewStructuresilently fails (caught and ignored)shouldReindexreturnstrue(fs.access on a non-existent path throws)- Fire-and-forget
performIndexingfails and setsproject.indexState.status = 'error' startFileWatcheris invoked on a non-existent path
The tool call then returns success with a broken project state, producing confusing downstream errors instead of a clear unknown_project response.
Add validation after parseProjectDirectory and before registerKnownRoot:
if (requestedProjectDirectory) {
try {
const stat = await fs.stat(requestedProjectDirectory);
if (!stat.isDirectory()) {
return {
ok: false,
response: buildProjectSelectionError('unknown_project',
`project_directory is not a directory: ${requestedProjectDirectory}`)
};
}
} catch {
return {
ok: false,
response: buildProjectSelectionError('unknown_project',
`project_directory does not exist: ${requestedProjectDirectory}`)
};
}
// ... rest of the method
}
1 participant
Summary
Verification
Closes #63