docs: clarify roots-first multi-project contract#85
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Greptile SummaryThis PR does two things simultaneously: it tightens the public multi-project documentation to an explicit "roots-first" contract, and it delivers a transitive dependency audit (per-project What changed
Notable observations
Confidence Score: 5/5Safe to merge — all remaining findings are minor style/performance suggestions with no correctness impact on the shipped feature. No P0 or P1 issues found. The three comments are all P2: a minor per-call Set allocation in src/utils/language-detection.ts (minor allocation regression for callers without a pre-built set); scripts/run-vitest.mjs (motivation undocumented).
|
| Filename | Overview |
|---|---|
| src/core/analyzer-registry.ts | Adds AnalyzerSelectionOptions interface and updates findAnalyzer/analyzeFile to prefer a named analyzer and accept extra file extensions per-project; logic is correct with safe fallthrough. |
| src/core/indexer.ts | Adds projectOptions to IndexerOptions, wires preferredAnalyzer / extraFileExtensions through to registry calls and include-pattern generation; includes graceful fallback and warn when preferred analyzer is absent. |
| src/utils/language-detection.ts | Renames codeExtensions to baseCodeExtensions and updates isCodeFile / getSupportedExtensions to merge extra extensions; callers that pass a pre-built Set are fast, but callers that omit the argument now allocate a new Set per call instead of reusing a module-level constant. |
| src/core/file-watcher.ts | Moves TRACKED_EXTENSIONS build inside startFileWatcher so it can include project-local extraExtensions; correctly parameterises isTrackedSourcePath to avoid a shared global set. |
| src/project-state.ts | Introduces ProjectRuntimeOverrides to replace the single extraExcludePatterns field; clean refactor with no regressions, default value {} maintained in factory. |
| src/server/config.ts | Adds analyzerHints to ProjectConfig and a parseStringArray helper; thorough whitespace trimming, type-guard filtering, and empty-object pruning match test expectations exactly. |
| src/index.ts | Wires runtimeOverrides (preferred analyzer + extra extensions) into indexer and file-watcher construction; adds buildProjectRuntimeOverrides and normalizeRuntimeExtensions helpers with consistent normalisation logic. |
| scripts/run-vitest.mjs | New thin wrapper that spawns vitest via process.execPath to avoid bin-resolution issues; correct signal re-raising and -- stripping, but the motivation for switching from vitest run is not documented. |
| tests/indexer-analyzer-hints.test.ts | New test file covering preferred-analyzer selection, incremental re-index with extra extensions, missing-analyzer fallback, and no-hints baseline; coverage is thorough. |
| tests/multi-project-routing.test.ts | Adds a regression test for a corrupted explicit project that should trigger a background rebuild rather than fall back to cwd; mock and restore of listRoots is correctly scoped in try/finally. |
| tests/mcp-client-templates.test.ts | New doc-assertion tests that verify template JSON validity, README client mentions, and capabilities.md roots-first wording; locks the public contract via test. |
| tests/search-decision-card.test.ts | Removes all as any casts with a typed getToolCallHandler helper, adds process.argv / CODEBASE_ROOT save-and-restore in afterEach, and switches to rmWithRetries for cleanup robustness. |
| docs/client-setup.md | New per-client setup guide extracted from README; covers all seven clients, HTTP and stdio modes, routing contract, single-project fallback, and local build testing. |
| package.json | Adds docs/client-setup.md and templates to the published files list, pins path-to-regexp@8.4.0 and brace-expansion@5.0.5 overrides, and delegates test to the new wrapper script. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["~/.codebase-context/config.json\n(projects[].analyzerHints)"] -->|loadServerConfig| B[ProjectConfig]
B -->|buildProjectRuntimeOverrides| C[ProjectRuntimeOverrides\npreferredAnalyzer\nextraExcludePatterns\nextraSourceExtensions]
C -->|applyServerConfig| D[ProjectState.runtimeOverrides]
D -->|performIndexingOnce| E[CodebaseIndexer\nIndexerOptions.projectOptions]
D -->|ensureProjectWatcher| F[FileWatcher\nextraExtensions]
E -->|getProjectOptions / validate| G{Analyzer registered?}
G -->|yes| H[AnalyzerSelectionOptions\npreferredAnalyzer + extraFileExtensions]
G -->|no| I[warn + preferredAnalyzer = undefined]
H --> J[AnalyzerRegistry.findAnalyzer]
I --> J
J -->|preferredAnalyzer set AND canAnalyze OR isExtraExtension| K[Return preferred analyzer]
J -->|fallthrough| L[Return highest-priority matching analyzer]
F -->|getSupportedExtensions + extra| M[trackedExtensions Set\nper-project]
M --> N[isTrackedSourcePath per file]
Comments Outside Diff (1)
-
src/index.ts, line 1531-1538 (link)Stale
runtimeOverridesnot cleared when config removes all hintsapplyServerConfigonly callsproject.runtimeOverrides = runtimeOverrideswhenObject.keys(runtimeOverrides).length > 0. If a user removes allanalyzerHintsandexcludePatternsfrom the config file and the server reloads config (e.g. via a future hot-reload path), the old overrides would silently persist on the in-memoryProjectStateuntil the process is restarted.Consider always assigning:
const runtimeOverrides = buildProjectRuntimeOverrides(proj); const project = getOrCreateProject(proj.root); project.runtimeOverrides = runtimeOverrides; // always reset, even if empty
This is a no-op for the current startup-only path, but makes the logic safe if config reload is ever added.
Reviews (1): Last reviewed commit: "docs: clarify roots-first multi-project ..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
src/utils/language-detection.ts
Outdated
| /** | ||
| * Check if a file is a code file | ||
| */ | ||
| export function isCodeFile(filePath: string): boolean { | ||
| export function isCodeFile( | ||
| filePath: string, | ||
| extensions?: Iterable<string> | ReadonlySet<string> | ||
| ): boolean { | ||
| const ext = path.extname(filePath).toLowerCase(); | ||
| return codeExtensions.has(ext); | ||
| const supportedExtensions = | ||
| extensions instanceof Set ? extensions : buildCodeExtensions(extensions); |
There was a problem hiding this comment.
isCodeFile allocates a new Set on every call without a pre-built set
Before this PR, isCodeFile used a module-level codeExtensions constant that was built once. The new implementation calls buildCodeExtensions(undefined) — which creates new Set(baseCodeExtensions) — on every invocation where no pre-built set is passed. Callers inside the indexer already pass this.supportedCodeExtensions, so the hot path is fine. But any call site in the CLI layer (or other consumers) that omits the argument now pays an O(n) set-copy on every file check instead of an O(1) lookup.
A simple fix is to cache the default set:
// module level, built once
const defaultCodeExtensions: ReadonlySet<string> = buildCodeExtensions();
export function isCodeFile(
filePath: string,
extensions?: Iterable<string> | ReadonlySet<string>
): boolean {
const ext = path.extname(filePath).toLowerCase();
const supportedExtensions =
extensions instanceof Set ? extensions : (extensions ? buildCodeExtensions(extensions) : defaultCodeExtensions);
return supportedExtensions.has(ext);
}| import { spawn } from 'node:child_process'; | ||
| import { fileURLToPath } from 'node:url'; | ||
|
|
||
| const forwardedArgs = process.argv.slice(2); | ||
| const vitestArgs = | ||
| forwardedArgs[0] === '--' ? forwardedArgs.slice(1) : forwardedArgs; | ||
|
|
||
| const vitestEntrypoint = fileURLToPath( | ||
| new URL('../node_modules/vitest/vitest.mjs', import.meta.url) | ||
| ); | ||
|
|
||
| const child = spawn(process.execPath, [vitestEntrypoint, 'run', ...vitestArgs], { | ||
| stdio: 'inherit', | ||
| env: process.env | ||
| }); | ||
|
|
||
| child.on('exit', (code, signal) => { | ||
| if (signal) { | ||
| process.kill(process.pid, signal); | ||
| return; | ||
| } | ||
|
|
||
| process.exit(code ?? 1); | ||
| }); | ||
|
|
||
| child.on('error', (error) => { | ||
| console.error('[test] Failed to start vitest:', error); | ||
| process.exit(1); | ||
| }); |
There was a problem hiding this comment.
vitest run with a custom wrapper is undocumented
The test script was changed from vitest run to node scripts/run-vitest.mjs. The wrapper works correctly — signal re-raising, -- stripping, and exit-code forwarding are all fine. However, there's no comment explaining why this was necessary (e.g. Node/pnpm bin-resolution issue in certain environments, or a specific CI quirk).
A one-line comment at the top of the file would help future contributors understand why the direct invocation was abandoned:
// Spawns vitest via process.execPath to avoid bin-resolution failures when
// Node is invoked directly (e.g. `node scripts/run-vitest.mjs`) without pnpm.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
* docs: restructure README and add client setup guide README cut from 707 to ~220 lines. Per-client config blocks, pipeline internals, and eval harness moved to dedicated docs where they belong. Screenshots and CLI previews moved up before the setup details. - docs/client-setup.md: new file with all 7 client configs (stdio + HTTP where supported), fallback single-project setup, and local build testing - docs/capabilities.md: add practical routing examples and selection_required response shape (moved from README) - CONTRIBUTING.md: add eval harness section (moved from README) - templates/mcp/stdio/.mcp.json + http/.mcp.json: copy-pasteable config templates - tests/mcp-client-templates.test.ts: regression tests for template validity and README/capabilities doc coverage - package.json: add docs/client-setup.md to files array * Update tests/mcp-client-templates.test.ts Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com> --------- Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
- Cache defaultCodeExtensions at module level in isCodeFile so callers that pass no pre-built set avoid a per-call Set allocation - Add motivation comment to scripts/run-vitest.mjs explaining why the wrapper exists instead of invoking vitest directly - Always assign runtimeOverrides in applyServerConfig even when empty, so stale overrides don't persist if config hints are later removed
PatrickSys
force-pushed
the
feat/multi-project-contract-hardening
branch
from
f983b27 to
41c252a
Compare
1 participant
Summary
Verification
Notes