feat(cli): tighten MCP routing and add dev-aware user-scoped config install

[mike-inkeep]

Summary

This PR removes the CLI MCP server's single-project compatibility behavior and replaces it with a strict routing contract for project-scoped tool calls. Tool calls now route by explicit cwd first, otherwise by the client's only advertised root, and otherwise fail clearly instead of guessing from the startup cwd.

It also keeps --port as the explicit single-target override, normalizes cwd values before config/server cache lookups, and simplifies Claude Desktop to the same npx @inkeep/open-knowledge mcp entry as the project-local editors while surfacing --force guidance when an existing MCP entry differs from the recommended config.

Key decisions

• Project-scoped MCP routing now blocks on roots/list the first time it needs client roots, and invalidates that cache on roots/list_changed, instead of falling back to the startup cwd or inferring from other tool arguments.
• cwd normalization is centralized as absolute-path plus best-effort realpath canonicalization so config resolution and project-server discovery share stable cache keys across symlinked paths.
• --port remains the explicit debug/single-target path and bypasses project selection entirely.
• open-knowledge init no longer silently overwrites existing MCP entries by default. It now distinguishes an identical entry from a conflicting one and tells the user to rerun with --force when the existing entry differs from current defaults.

Validation

• cd packages/cli && bunx tsc --noEmit
• cd packages/cli && bun test src/mcp/server.test.ts src/mcp/server-discovery.test.ts src/config/loader.test.ts src/commands/init.test.ts
• bun run check still fails on pre-existing open-knowledge-app tests in packages/app/src/editor/source-polish/view-plugin.test.ts and packages/app/src/editor/source-polish/broken-ref-field.test.ts; no CLI package failures were introduced by this change.

Manual QA

No manual QA performed.

[changeset-bot]

🦋 Changeset detected

Latest commit: b2bcbd8

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 5 packages

Name	Type
@inkeep/open-knowledge	Minor
@inkeep/open-knowledge-core	Minor
@inkeep/open-knowledge-server	Minor
@inkeep/open-knowledge-app	Minor
@inkeep/open-knowledge-desktop	Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[inkeep-internal-ci]

PR Review Summary

(4) Total Issues | Risk: Medium

🟠⚠️ Major (2) 🟠⚠️

🟠 1) server-discovery.ts:312-318 Race condition in concurrent cache access

Issue: The createProjectServerUrlResolver function has a potential race condition where concurrent calls can interleave between checking the cache and setting it. Two calls for the same effectiveCwd arriving within the same tick could both miss the cache, both call ensure(), and one result could overwrite the other.

Why: While the impact is limited (the server URL should be deterministic for the same cwd), concurrent auto-starts could spawn duplicate server processes before the first one writes its lock file. This is especially relevant during bursty MCP tool usage.

Fix: Consider using a pending-promise pattern (similar to pendingRootsLoad in createProjectRoutingResolver) to deduplicate in-flight resolutions:

const pending = new Map<string, Promise<string | undefined>>();

return async (cwd?: string): Promise<string | undefined> => {
  const effectiveCwd = await normalizeCwd(cwd ?? opts.startupCwd);
  const now = Date.now();
  const cached = cache.get(effectiveCwd);
  if (cached && cached.expiresAt > now) return cached.url;
  
  // Dedupe concurrent resolutions
  let pendingResolve = pending.get(effectiveCwd);
  if (!pendingResolve) {
    pendingResolve = (async () => {
      // ... resolution logic
    })().finally(() => pending.delete(effectiveCwd));
    pending.set(effectiveCwd, pendingResolve);
  }
  return pendingResolve;
};

Refs:

• server-discovery.ts:312-318
• server.ts:66-88 — reference pattern with pendingRootsLoad

🟠 2) normalize-cwd.ts:12-18 Silent fallback on realpath failure obscures issues

Issue: The normalizeCwd function catches all realpath errors silently and falls back to the absolute path. While the JSDoc documents this behavior, no logging occurs when the fallback is triggered.

Why: When symlink resolution fails (e.g., broken symlinks, permission issues, or paths that don't exist), operators have no visibility into why cache keys might not be stable across calls. This can cause subtle cache misses where the same logical path sometimes uses realpath and sometimes doesn't, depending on transient filesystem state.

Fix: Add a debug-level log when the fallback path is taken:

export async function normalizeCwd(cwd: string): Promise<string> {
  const absolute = resolve(cwd);
  try {
    return await realpath(absolute);
  } catch (err) {
    // Fallback to absolute path when realpath fails (path doesn't exist, broken symlink, etc.)
    // This is intentional per the JSDoc, but worth logging for debugging cache key stability.
    if (process.env.DEBUG) {
      console.error(`[normalize-cwd] realpath failed for ${absolute}: ${err instanceof Error ? err.message : err}`);
    }
    return absolute;
  }
}

Refs:

• normalize-cwd.ts:12-18

Inline Comments:

• 🟠 Major: server-discovery.ts:314 Race condition in concurrent cache access
• 🟠 Major: normalize-cwd.ts:12 Silent fallback without logging

🟡 Minor (2) 🟡

🟡 1) server-discovery.ts:228,250,253,265 Inconsistent error message prefix

Issue: Error messages in ensureServerRunning use OK: prefix (e.g., OK: spawn failed:, OK: server did not start) while most other error messages in the codebase use Error: prefix.

Why: Inconsistent error prefixes make log aggregation and error categorization harder. Users may not recognize OK: as an error prefix.

Fix: Consider aligning to a consistent prefix pattern across the codebase.

Refs:

• server-discovery.ts:228

🟡 2) shared.ts:74-91 New helper functions lack unit tests

Issue: resolveProjectConfigContext and resolveProjectServerContext are new exported functions in shared.ts but there are no corresponding unit tests for these functions.

Why: These functions encapsulate the core cwd → config → server resolution logic used by all MCP tools. Edge cases (thrown errors, missing config, etc.) should have explicit test coverage.

Fix: Add unit tests covering: (1) successful resolution, (2) resolveCwd throwing, (3) resolveConfig throwing, (4) resolveServerUrl throwing.

Refs:

• shared.ts:74-91

Inline Comments:

• 🟡 Minor: server-discovery.ts:228 Error message prefix inconsistency
• 🟡 Minor: shared.ts:74 Missing unit tests for helper functions

💭 Consider (1) 💭

💭 1) server-discovery.test.ts:1 Test organization could be improved

Issue: Tests for server-discovery.ts are well-written but could benefit from grouping into nested describe blocks by function (decideAutoStart, ensureServerRunning, createProjectServerUrlResolver).

Why: Nested describes improve test discoverability and make it easier to run subsets of tests during development.

Refs:

• server-discovery.test.ts

---

💡 APPROVE WITH SUGGESTIONS

Summary: This PR implements a clean strict routing contract for MCP project selection. The architecture is sound — using MCP roots/list for project context, normalizing paths with realpath, and maintaining TTL-based caches. The main concerns are around a potential race condition in concurrent cache access (fixable with the pending-promise pattern already used elsewhere in this PR) and silent error handling in normalizeCwd. Neither blocks merge, but addressing the race condition before heavy multi-client usage would be wise.

Discarded (3)

Location	Issue	Reason Discarded
server-discovery.ts:328	Host value uses config.server.host instead of opts.host	This is intentional — opts.host is for CLI-level decisions while config-resolved host is correct for per-project server discovery
server.ts:322	effectiveCwd fallback to resolveCwd() without explicit cwd	This is by design — the routing resolver handles the fallback logic with proper error messages
init.ts	MCP config diffing uses JSON.stringify	Deep equality via stringify is appropriate here given the simple object shapes involved

Reviewers (6)

Reviewer	Returned	Main Findings	Consider	While You're Here	Inline Comments	Pending Recs	Discarded
pr-review-standards	4	1	0	0	1	0	2
pr-review-tests	2	1	1	0	1	0	0
pr-review-consistency	2	1	0	0	1	0	1
pr-review-errors	2	1	0	0	1	0	1
pr-review-appsec	1	0	0	0	0	0	1
pr-review-types	1	0	0	0	0	0	1
Total	12	4	1	0	4	0	6

[inkeep-internal-ci]

PR Review Summary

(4) Total Issues | Risk: Medium

🕐 Pending Recommendations (2)

Prior review flagged these issues — they remain ACTIVE (code unchanged since last review):

• 🟠 server-discovery.ts:338 Race condition in createProjectServerUrlResolver concurrent cache access — consider pending-promise deduplication pattern
• 🟠 normalize-cwd.ts:12 Silent fallback on realpath failure hides path resolution issues — add logging for non-ENOENT errors

🟡 Minor (1) 🟡

🟡 1) server-discovery.ts:243,270,288 Error message prefix inconsistent with MCP tool convention

Issue: Error messages use OK: prefix (e.g., OK: spawn failed:, OK: server did not start within...) while all other MCP tool error messages use Error: prefix.

Why: The Error: prefix is used across 50+ occurrences in MCP tool handlers. Agents may pattern-match on this prefix to identify tool-level failures. The OK: prefix is novel and undocumented.

Fix: Replace OK: with Error: in the three throw statements at lines 243, 270, and 287-289.

Refs:

• shared.ts:42-43 — established Error: pattern

Inline Comments:

• 🟠 Major: review.txt:1 Transient review artifact committed to repository
• 🟡 Minor: loader.ts:153 Same cache race pattern as server-discovery.ts

💭 Consider (2) 💭

💭 1) mcp/server-discovery.test.ts Test file organization

Issue: Tests for decideAutoStart and ensureServerRunning live in commands/mcp.test.ts, not co-located with the implementation in server-discovery.ts.

Why: Developers modifying server-discovery.ts and running only its co-located test file would miss regressions in these critical functions. The functions ARE tested (thoroughly) — they're just in a different file.

Fix: Either move those tests to server-discovery.test.ts or add a comment pointing to mcp.test.ts.

💭 2) shared.ts:76-120 New helper functions lack direct unit tests

Issue: resolveProjectConfigContext and resolveProjectServerContext have no direct unit tests. They're exercised transitively through tool integration tests.

Why: These are the centralized error-handling wrappers all tools use. Edge cases (resolveCwd throws, resolveConfig throws) may not be directly covered.

Fix: Add ~30 lines of unit tests covering success/failure paths for both functions.

Refs:

• Prior review comment

---

💡 APPROVE WITH SUGGESTIONS

Summary: This PR implements a clean strict routing contract for MCP project selection. The architecture is sound — using MCP roots/list for project context, normalizing paths with realpath, and maintaining TTL-based caches. The prior review's two MAJOR findings (race condition in server URL resolver, silent realpath fallback) remain unaddressed but are not blockers. The new finding is a review.txt file that was accidentally committed and should be removed before merge.

Before merging:

1. Remove review.txt from the commit
2. Consider addressing the pending race condition suggestion in server-discovery.ts (or accept it as low-risk given server lock file protection)

Discarded (3)

Location	Issue	Reason Discarded
review.txt findings	Claims tools don't expose cwd parameter	Outdated — all 19 routed tools now have cwd via ROUTED_CWD_DESCRIPTION
init.ts conflict detection	Claims user-managed fields cause false conflicts	Outdated — isCompatible now checks only managed fields via hasMatchingManagedFields
keepalive never attaches	Claims keepalive fails in multi-project mode	Needs verification — getKeepaliveCwd returns activeProjectCwd after any tool call succeeds

Reviewers (4)

Reviewer	Returned	Main Findings	Consider	While You're Here	Inline Comments	Pending Recs	Discarded
pr-review-standards	2	0	0	0	1	0	1
pr-review-tests	8	0	2	0	0	0	6
pr-review-errors	0	0	0	0	0	0	0
pr-review-consistency	2	1	0	0	1	0	0
Total	12	1	2	0	2	0	7

Note: pr-review-errors returned 0 findings because all significant error handling issues were already covered by the prior review.

[inkeep-internal-ci]

PR Review Summary

(6) Total Issues | Risk: Medium

🕐 Pending Recommendations (4)

Prior review flagged these issues — they remain ACTIVE (code unchanged since last review):

• 🟠 server-discovery.ts:338 Race condition in createProjectServerUrlResolver concurrent cache access — consider pending-promise deduplication pattern
• 🟠 normalize-cwd.ts:12 Silent fallback on realpath failure hides path resolution issues — add logging for non-ENOENT errors
• 🟡 server-discovery.ts:243 Error message prefix inconsistency — use Error: instead of OK:
• 🟠 review.txt:1 Transient review artifact committed to repository — remove before merge

🟠⚠️ Major (1) 🟠⚠️

🟠 1) .changeset/ Missing changeset for CLI public surface changes

Issue: This PR makes significant changes to the CLI's MCP routing behavior (strict routing contract, project-scoped tool calls, cwd normalization) but has no changeset describing these changes. The deleted claude-desktop-init-cwd.md was from a different merged PR (#221).

Why: Per release engineering conventions, public surface changes require a changeset. The CLI's MCP routing behavior is user-facing — agents relying on the previous fallback-to-startup-cwd behavior will now receive errors. The PR description notes "tool calls now route by explicit cwd first, otherwise by the client's only advertised root, and otherwise fail clearly instead of guessing" which is a behavioral change requiring documentation.

Fix: Create a new changeset via bun run changeset:

---
"@inkeep/open-knowledge": minor
---

feat(cli): enforce strict MCP routing — tool calls now require explicit `cwd` parameter or single client root, instead of falling back to startup cwd

Refs:

• changeset-bot comment — "No Changeset found"

🟡 Minor (3) 🟡

🟡 1) logger.ts, keepalive.ts, exec.ts Silent error swallowing in multiple catch blocks

Issue: Several catch blocks silently swallow errors without any logging:

• logger.ts:97 — appendFileSync errors when OK_LOG_FILE is set
• keepalive.ts:173 — WebSocket error events
• exec.ts:503,538 — enrichPath/enrichDirectory and batch backlink fetch failures

Why: When these operations fail, operators have no visibility into what went wrong. This makes debugging significantly harder, especially for the logging subsystem itself where silent failures defeat the purpose of logging.

Fix: Add console.debug or console.warn logging before swallowing each error.

Inline Comments:

• 🟡 Minor: logger.ts:97 Silent swallow of appendFileSync errors
• 🟡 Minor: keepalive.ts:176 WebSocket error listener swallows errors
• 🟡 Minor: exec.ts:503 enrichPath errors swallowed
• 🟡 Minor: exec.ts:538 Batch backlink fetch failure swallowed

🟡 2) normalize-cwd.ts, logger.ts Missing co-located unit tests for new utilities

Issue: The new normalize-cwd.ts utility and logger.ts module have no co-located test files, unlike other utilities in the same directories (frontmatter.test.ts, server-discovery.test.ts).

Why: The codebase convention is "Tests co-located with source: foo.test.ts next to foo.ts". These are central utilities for the new routing logic.

Fix: Add normalize-cwd.test.ts covering: (1) absolute path resolution, (2) symlink canonicalization, (3) ENOENT fallback. Add logger.test.ts covering debug gating and child logger behavior.

Refs:

• packages/cli/src/utils/frontmatter.test.ts — sibling test file pattern

🟡 3) get-preview-url.test.ts Missing error path test coverage

Issue: The new tests verify that buildGetPreviewUrlResult passes cwd to config resolvers, but do not test the error handling when resolveCwd throws (e.g., no client roots and no explicit cwd).

Why: If resolveCwd throws, the tool should return a user-friendly error, not crash. The test shows the happy path but not the failure path.

Fix: Add a test case: resolveCwd: async () => { throw new Error('No client roots'); } and verify the result contains {ok: false, ...} with the expected error message.

Refs:

• shared.ts:76-93 — resolveProjectConfigContext error handling

💭 Consider (2) 💭

💭 1) logger.ts:47 Logger method signature inconsistent with server package

Issue: McpLogger uses (msg, err?, ctx?) signature while server's PinoLogger uses (data, message) matching Pino convention.

Why: Cross-package consistency reduces cognitive load. Consider documenting the intentional divergence.

Inline Comments:

• 💭 Consider: logger.ts:47 Logger signature inconsistency

💭 2) biome.jsonc Unrelated tooling bump bundled with feature PR

Issue: Biome schema version bumped from 2.4.11 to 2.4.12 as a transitive lockfile update.

Why: Bundling unrelated dependency updates with feature PRs makes it harder to diagnose regressions. Consider splitting or noting explicitly.

---

💡 APPROVE WITH SUGGESTIONS

Summary: This PR implements a clean strict routing contract for MCP project selection. The architecture is sound — using MCP roots/list for project context, normalizing paths with realpath, and maintaining TTL-based caches. The prior review's two MAJOR findings (race condition in server URL resolver, silent realpath fallback) remain unaddressed in code but are documented as pending recommendations. The main new finding is a missing changeset for this behavioral change — agents will start receiving errors instead of fallback behavior, which warrants release documentation.

Before merging:

1. Remove review.txt from the commit
2. Add a changeset describing the strict routing behavioral change
3. Consider addressing the pending race condition suggestion (or accept it as low-risk given server lock file protection)

Discarded (5)

Location	Issue	Reason Discarded
loader.ts:59	YAML parse error truncates line info	Low impact — yaml library's error.message already includes position info in most cases
server.ts:98	roots/list error type not distinguished	Acceptable — the error is logged and re-thrown with a user-facing message
exec.ts:385	stat failure in classifyPaths silent	ENOENT is expected for non-existent paths; other errors are rare edge cases
editors.test.ts	Missing isCompatible/mergeManagedFields tests	Covered transitively through init.test.ts integration tests
server.test.ts:67	Test verifies schema not routing behavior	Unit tests for routing resolver provide adequate coverage

Reviewers (5)

Reviewer	Returned	Main Findings	Consider	While You're Here	Inline Comments	Pending Recs	Discarded
pr-review-standards	0	0	0	0	0	0	0
pr-review-consistency	4	0	1	0	1	0	2
pr-review-errors	8	1	0	0	4	0	3
pr-review-tests	5	1	0	0	0	0	4
pr-review-devops	3	1	1	0	0	0	1
Total	20	3	2	0	5	0	10

Note: pr-review-standards returned 0 findings because all major issues were already covered in prior reviews.

[mike-inkeep]

Addressed the current review pass in fa64d6a7: added in-flight per-cwd dedupe in both routed resolvers, switched MCP spawn failures to the established Error: prefix, warned on non-ENOENT normalizeCwd() failures and OK_LOG_FILE write failures, removed review.txt, moved the auto-start tests next to server-discovery.ts, added direct shared-helper / preview-url error-path / logger / normalize-cwd coverage, and added .changeset/strict-mcp-routing.md. Verification: bun test "src/mcp/server-discovery.test.ts" "src/config/loader.test.ts" "src/mcp/tools/shared.test.ts" "src/mcp/tools/get-preview-url.test.ts" "src/utils/normalize-cwd.test.ts" "src/mcp/logger.test.ts", bun run typecheck, and bunx biome check on the touched files. For the logger API-shape suggestion, I kept the message-first MCP logger and documented the intentional divergence instead of changing every CLI call site in this PR.

[inkeep-internal-ci]

PR Review Summary

(0) Total Issues | Risk: Low

This is a well-executed refactor that implements a strict routing contract for MCP project selection. The architecture is sound: using MCP roots/list for project context, normalizing paths with realpath, and maintaining TTL-based caches with proper pending-promise deduplication.

What was done well

1. Clean routing abstraction — createProjectRoutingResolver provides a single source of truth for cwd resolution with clear error states (NO_CLIENT_ROOTS_ERROR, MULTIPLE_ROOTS_ERROR, ROOTS_UNAVAILABLE_ERROR)

2. Race condition prevention — Both createProjectServerUrlResolver and createProjectConfigResolver use the pending-promise deduplication pattern, preventing duplicate server spawns and config loads

3. Comprehensive test coverage — The new test files (server-discovery.test.ts, logger.test.ts, normalize-cwd.test.ts, editors.test.ts) provide good coverage of the new functionality

4. Structured logging — The new McpLogger provides JSON-structured logs with session correlation IDs, improving debuggability for MCP stdio sessions

5. Prior feedback addressed — All issues from previous reviews have been resolved:

   • ✅ Race condition in concurrent cache access — fixed with pending-promise pattern
   • ✅ Silent realpath fallback — now warns on non-ENOENT errors
   • ✅ Error message prefix — changed to Error: consistently
   • ✅ review.txt artifact — removed
   • ✅ Shared helper tests — added
   • ✅ Auto-start tests moved — co-located with implementation

💭 Consider (2) 💭

Inline Comments:

• 💭 Consider: keepalive.ts:176 WebSocket error event lacks diagnostic context
• 💭 Consider: server.ts:106 Error type classification in roots/list failure

🟡 Minor (4) — Test Coverage Opportunities 🟡

These are opportunities to strengthen test coverage for edge cases. None are blocking.

🟡 1) server-discovery.test.ts Missing test for synchronous spawn throw path

Issue: The implementation has a try/catch around the spawn call (lines 230-238) to handle EACCES or PATH resolution errors that throw synchronously. The test suite doesn't explicitly exercise this sync-throw path.

Why: If the sync-throw path breaks during refactoring, the error message formatting could regress without detection.

Fix: Add a test with spawn: () => { throw new Error('EACCES'); } and verify the error message format.

🟡 2) server-discovery.test.ts Missing test for async spawn error event path

Issue: The implementation attaches a child.on('error') handler to capture async spawn errors. The test suite doesn't exercise this path where an error event fires during polling.

Why: Async spawn errors (especially ENOENT when npx/node isn't found) are a real failure mode.

Fix: Add a test with a mock child that emits an 'error' event during the poll loop.

🟡 3) server.test.ts No test for concurrent roots resolution deduplication

Issue: The pendingRootsLoad pattern deduplicates concurrent root resolution calls. While server-discovery.test.ts validates the same pattern for server URL resolution, there's no equivalent test for resolveCwd.

Why: The pattern could regress during refactoring. Lower priority since the pattern is validated elsewhere.

🟡 4) loader.test.ts Missing test for config cache expiration

Issue: The createProjectConfigResolver has a cacheMs parameter for TTL, but no test verifies cache entries actually expire and reload.

Why: The TTL logic could regress, though this is low-impact since config rarely changes mid-session.

---

✅ APPROVE

Summary: This PR delivers a clean, well-tested implementation of strict MCP project routing. All prior review feedback has been addressed. The remaining suggestions are minor test coverage enhancements that don't block merge. Ship it! 🚀

Discarded (1)

Location	Issue	Reason Discarded
search.ts:116	buildSearchResult throws instead of returning structured error	Consistency with exec tool's structured errors is a valid point, but this is a pre-existing pattern and the PR's scope is routing — not error response normalization

Reviewers (4)

Reviewer	Returned	Main Findings	Consider	While You're Here	Inline Comments	Pending Recs	Discarded
pr-review-standards	0	0	0	0	0	0	0
pr-review-tests	4	4	0	0	0	0	0
pr-review-consistency	0	0	0	0	0	0	0
pr-review-errors	3	0	2	0	2	0	1
Total	7	4	2	0	2	0	1

[inkeep-internal-ci]

💭 Consider: Error type classification in roots/list failure

Issue: The loadRoots() catch block logs the error generically as "roots/list unavailable" and rethrows ROOTS_UNAVAILABLE_ERROR. This loses the distinction between expected errors (client doesn't support roots/list) and unexpected ones (network timeout, protocol error).

Why: During debugging, operators seeing "roots/list unavailable" cannot tell if the client simply doesn't support the feature (expected, fallback to explicit cwd) or if there's a transport-layer problem that might affect other MCP calls.

Fix: Consider adding error type classification to the log context:

catch (err) {
  const errorType = err instanceof Error && 'code' in err 
    ? (err as any).code 
    : 'unknown';
  opts.logger?.warn('roots/list unavailable', {
    error: err instanceof Error ? err.message : String(err),
    errorType,
  });
  throw new ProjectRoutingError(ROOTS_UNAVAILABLE_ERROR);
}

Refs:

• server.ts:99-107

[inkeep-internal-ci]

PR Review Summary

(0) Total Issues | Risk: Low

This PR implements a clean strict routing contract for MCP project selection. The architecture is sound: using MCP roots/list for project context, normalizing paths with realpath, and maintaining TTL-based caches with proper pending-promise deduplication.

What was done well

1. Clean routing abstraction — createProjectRoutingResolver provides a single source of truth for cwd resolution with clear error states (NO_CLIENT_ROOTS_ERROR, MULTIPLE_ROOTS_ERROR, ROOTS_UNAVAILABLE_ERROR)

2. Race condition prevention — Both createProjectServerUrlResolver and createProjectConfigResolver use the pending-promise deduplication pattern, preventing duplicate server spawns and config loads

3. Comprehensive test coverage — The new test files (server-discovery.test.ts, logger.test.ts, normalize-cwd.test.ts, editors.test.ts) provide excellent coverage. The latest commit (0d3e3b6) adds tests for both sync spawn throws and async spawn error events.

4. Structured logging — The new McpLogger provides JSON-structured logs with session correlation IDs, improving debuggability for MCP stdio sessions

5. All prior feedback addressed:

   • ✅ Race condition in concurrent cache access — fixed with pending-promise pattern
   • ✅ Silent realpath fallback — now warns on non-ENOENT errors
   • ✅ Error message prefix — changed to Error: consistently
   • ✅ review.txt artifact — removed
   • ✅ Shared helper tests — added
   • ✅ Auto-start tests moved — co-located with implementation
   • ✅ Error type classification in roots/list failure — added classifyRootsLoadError()
   • ✅ WebSocket error event diagnostics — now emits debug breadcrumb
   • ✅ Sync spawn throw test — added
   • ✅ Async spawn error event test — added

Changes Since Last Review

Commit 0d3e3b6 ("fix(cli): follow up on routing review suggestions") addressed the remaining minor test coverage suggestions from the prior APPROVED review:

• Added classifyRootsLoadError() helper and errorType field in roots/list error logging
• Added debug breadcrumb in WebSocket error handler instead of silent swallow
• Added test for synchronous spawn throw path (EACCES)
• Added test for async spawn error event path (ENOENT)
• Extended keepalive and server routing test coverage

---

✅ APPROVE

Summary: This is a well-executed refactor that delivers a clean, well-tested implementation of strict MCP project routing. All prior review feedback has been addressed, including the minor test coverage enhancements suggested in the last review. The routing contract is clear (explicit cwd → single client root → error), the caching strategy prevents race conditions, and the logging infrastructure supports operator debugging. Ship it! 🚀

Reviewers (1)

Reviewer	Returned	Main Findings	Consider	While You're Here	Inline Comments	Pending Recs	Discarded
orchestrator	0	0	0	0	0	0	0
Total	0	0	0	0	0	0	0

Note: This is a re-review following an APPROVED status. All prior findings have been resolved.

[inkeep-internal-ci]

PR Review Summary

(0) Total Issues | Risk: Low

This PR implements a clean strict routing contract for MCP project selection. The prior automated review (2026-04-21T01:33:32Z) approved this PR after all feedback was addressed.

Changes Since Last Review

The latest commit (2c495f5) is a merge from origin/main bringing in unrelated changes:

• Electron desktop app infrastructure (new packages/desktop/)
• UI components (OkBlob rage-click, ConnectingBanner grace period)
• bootServer extraction from CLI to server package (D35)
• CI matrix sharding for quality gate

Impact on routing changes: None. The CLI routing implementation (server.ts, server-discovery.ts, normalize-cwd.ts, tools/*) is unchanged since approval.

What Was Done Well

1. Clean routing abstraction — createProjectRoutingResolver provides a single source of truth for cwd resolution with clear error states (NO_CLIENT_ROOTS_ERROR, MULTIPLE_ROOTS_ERROR, ROOTS_UNAVAILABLE_ERROR)

2. Race condition prevention — Both createProjectServerUrlResolver and createProjectConfigResolver use the pending-promise deduplication pattern, preventing duplicate server spawns and config loads

3. Comprehensive test coverage — New test files (server-discovery.test.ts, logger.test.ts, normalize-cwd.test.ts, editors.test.ts) provide excellent coverage

4. All prior feedback addressed:

   • ✅ Race condition in concurrent cache access — fixed with pending-promise pattern
   • ✅ Silent realpath fallback — now warns on non-ENOENT errors
   • ✅ Error message prefix — changed to Error: consistently
   • ✅ review.txt artifact — removed
   • ✅ Shared helper tests — added
   • ✅ Auto-start tests — co-located with implementation
   • ✅ Error type classification in roots/list — added classifyRootsLoadError()

---

✅ APPROVE

Summary: This is a well-executed refactor that delivers a clean, strict routing contract for MCP project selection. All prior review feedback has been addressed. The merge from main does not affect the routing implementation. Ship it! 🚀

Reviewers (1)

Reviewer	Returned	Main Findings	Consider	While You're Here	Inline Comments	Pending Recs	Discarded
orchestrator	0	0	0	0	0	0	0
Total	0	0	0	0	0	0	0

Note: This is a re-review following an APPROVED status. The merge from main introduced no changes to the routing implementation under review.

[inkeep-internal-ci]

PR Review Summary

(0) Total Issues | Risk: Low

This PR implements a clean strict routing contract for MCP project selection. The prior automated reviews (3 APPROVED verdicts) have all confirmed that previous feedback was addressed.

Changes Since Last Approval

The latest commits (de36c77, b75811b, 2c495f5) are merge commits from origin/main plus a housekeeping commit that:

• Reorders dependencies alphabetically in package.json
• Narrows export visibility (makes resolveCodexHomePath, EditorMcpResult, LaunchJsonResult, etc. private)
• Removes unused re-exports from mcp.ts

Impact on routing changes: None. The CLI routing implementation (server.ts, server-discovery.ts, normalize-cwd.ts, tools/*) is unchanged since the last approval.

What Was Done Well

1. Clean routing abstraction — createProjectRoutingResolver provides a single source of truth for cwd resolution with clear error states (NO_CLIENT_ROOTS_ERROR, MULTIPLE_ROOTS_ERROR, ROOTS_UNAVAILABLE_ERROR)

2. Race condition prevention — Both createProjectServerUrlResolver and createProjectConfigResolver use the pending-promise deduplication pattern, preventing duplicate server spawns and config loads

3. Comprehensive test coverage — The new test files (server-discovery.test.ts at 699 lines, server.test.ts at 395 lines, logger.test.ts at 93 lines) provide excellent coverage of the new functionality

4. Structured logging — The new McpLogger provides JSON-structured logs with session correlation IDs, improving debuggability for MCP stdio sessions

5. All prior feedback addressed:

   • ✅ Race condition in concurrent cache access — fixed with pending-promise pattern
   • ✅ Silent realpath fallback — now warns on non-ENOENT errors
   • ✅ Error message prefix — changed to Error: consistently
   • ✅ review.txt artifact — removed
   • ✅ Shared helper tests — added
   • ✅ Auto-start tests — co-located with implementation
   • ✅ Error type classification in roots/list — added classifyRootsLoadError()

Prior Feedback Status

The one previously ACTIVE automated comment at server.ts:114 about error type classification in roots/list failure has been addressed:

// Line 121-124: Now includes errorType in log context
opts.logger?.warn('roots/list unavailable', {
  error: err instanceof Error ? err.message : String(err),
  errorType: classifyRootsLoadError(err),
});

---

✅ APPROVE

Summary: This is a well-executed refactor that delivers a clean, strict routing contract for MCP project selection. All prior review feedback has been addressed. The merge from main does not affect the routing implementation. The architecture is sound — using MCP roots/list for project context, normalizing paths with realpath, and maintaining TTL-based caches with proper pending-promise deduplication. Ship it! 🚀

Reviewers (1)

Reviewer	Returned	Main Findings	Consider	While You're Here	Inline Comments	Pending Recs	Discarded
orchestrator	0	0	0	0	0	0	0
Total	0	0	0	0	0	0	0

Note: This is a re-review following 3 prior APPROVED verdicts. The routing implementation is unchanged since the last approval — only merge commits from main have been added.

[inkeep-internal-ci]

PR Review Summary

(0) Total Issues | Risk: Low

This PR implements a clean strict routing contract for MCP project selection. This is a re-review following 4 prior APPROVED verdicts — all prior feedback has been addressed.

Changes Since Last Approval

The latest commits (b2bcbd8, 2b81347) since the last approval (0d3e3b6) are:

• Merge commits from origin/main bringing in unrelated changes (Electron desktop infrastructure, UI components, bootServer extraction)
• Housekeeping: narrowed export visibility (made DecideAutoStartInput, EnsureServerRunningOptions, CreateProjectServerUrlResolverOptions, ProjectRoutingResolver, etc. private instead of exported)
• Fixed a flaky test

Impact on routing changes: None. The CLI routing implementation (server.ts, server-discovery.ts, normalize-cwd.ts, tools/*) is unchanged since the last approval.

What Was Done Well

1. Clean routing abstraction — createProjectRoutingResolver provides a single source of truth for cwd resolution with clear error states (NO_CLIENT_ROOTS_ERROR, MULTIPLE_ROOTS_ERROR, ROOTS_UNAVAILABLE_ERROR)

2. Race condition prevention — Both createProjectServerUrlResolver and createProjectConfigResolver use the pending-promise deduplication pattern, preventing duplicate server spawns and config loads

3. Comprehensive test coverage — ~1,400 lines of new tests across server-discovery.test.ts (699 lines), server.test.ts (395 lines), logger.test.ts (93 lines), normalize-cwd.test.ts (39 lines), and editors.test.ts (170 lines)

4. Structured logging — The new McpLogger provides JSON-structured logs with session correlation IDs, improving debuggability for MCP stdio sessions

5. All prior feedback addressed:

   • ✅ Race condition in concurrent cache access — fixed with pending-promise pattern
   • ✅ Silent realpath fallback — now warns on non-ENOENT errors
   • ✅ Error message prefix — changed to Error: consistently
   • ✅ review.txt artifact — removed
   • ✅ Shared helper tests — added
   • ✅ Auto-start tests — co-located with implementation
   • ✅ Error type classification in roots/list — added classifyRootsLoadError() with errorType in log context
   • ✅ Changeset added — strict-mcp-routing.md properly describes the behavioral change

Prior Feedback Status

The one previously ACTIVE automated comment at server.ts:114 about error type classification has been addressed:

// Lines 121-124 now include errorType in log context:
opts.logger?.warn('roots/list unavailable', {
  error: err instanceof Error ? err.message : String(err),
  errorType: classifyRootsLoadError(err),
});

---

✅ APPROVE

Summary: This is a well-executed refactor that delivers a clean, strict routing contract for MCP project selection. All prior review feedback has been addressed. The merge commits from main do not affect the routing implementation. The architecture is sound — using MCP roots/list for project context, normalizing paths with realpath, and maintaining TTL-based caches with proper pending-promise deduplication. Ship it! 🚀

Reviewers (1)

Reviewer	Returned	Main Findings	Consider	While You're Here	Inline Comments	Pending Recs	Discarded
orchestrator	0	0	0	0	0	0	0
Total	0	0	0	0	0	0	0

Note: This is a re-review following 4 prior APPROVED verdicts. The routing implementation is unchanged since the last approval — only merge commits from main and export visibility housekeeping have been added.