fix(mcp): route identifiers by the session's own project set - #1439
Merged
Conversation
Two defects in the same family — an identifier or path resolved or returned wrongly by the MCP routing layer. #1432: detect_project_from_identifier_prefix, the front door for read_note, search, read_content, edit_note, delete_note, build_context and the note resource, fell back to searching the account-wide workspace index for a bare first segment. It answered with a project in whichever accessible workspace held that permalink, preferring the is_default one, so an ordinary project-relative path silently read another workspace's same-named project. The same split took exactly one segment, so a project whose permalink spans two ('Research/2026') was unreachable at all. The fallback is deleted rather than narrowed. A path-shaped identifier now names a project only when a project set the session already holds claims its leading segments, claimed by the one splitter that takes a candidate set: local config first, then addressable_projects — the same mount table `ls /` advertises and resolve_project_path_route routes by, which in a hosted session is exactly the projects of the workspace the session is bound to. Mounts are consulted before workspace-qualified routes, matching the posix resolver's precedence, so `ls team/docs/x` and read_note("team/docs/x") can no longer disagree about what 'team' names; a mount hit now builds no workspace index at all. Refusal is deliberately not adopted here: on this surface a first segment naming no project is the common case ('specs/search-spec'), so the answer for it stays None and the caller's active project applies. Detection returns DetectedProjectRoute — the project plus the external_id that pins it — because a name is unique only inside one workspace and handing on the name alone let it be re-resolved into a different one. Callers forward both, exactly as the posix verbs forward ProjectPathRoute's pair. The memory:// half of #1432: resolve_project_and_path split one leading segment off and resolved it through /v2/projects/resolve, so 'memory://research/2026/notes/x' offered 'research' — a real, different project in a config that has one — which came back as the active project and was cached over the project the client was opened for. The routed project's own permalink now claims the prefix with no lookup at all. The name-resolving branch below keeps its single-segment reading: no set in hand names that project, and buying the multi-segment reading there would cost a project-list fetch on every cross-project memory:// read on top of the resolve it already pays. #1435: find's metadata arm answered with search hits that skipped the re-qualification ls, cat and find's listing arm perform, so a routed find --meta returned project-relative paths that cat then refused — or opened in a different project mounted under that name. qualify_search_paths is the third response shape's qualifier, applied before field projection so both shapes of the metadata response carry one spelling. The verb guard could not catch that: find was already in its set and the gap was one arm inside it. A companion guard drives one routed call per response shape and asserts every qualifier the module defines actually fired and that every returned path routes back. Stated precisely, because neither closes everything: a new response shape needs a new qualifier and fails the shape guard until a routed call exercises it, a new path-accepting verb fails the verb guard, and what covers a new arm of an existing verb is that every shape a routed verb answers with already has a qualifier. Closes #1432 Closes #1435 Refs #1438 Signed-off-by: phernandez <paul@basicmachines.co>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![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: 7e3ce58da2
ℹ️ 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".
…ct caches
addressable_projects memoizes the session's own project listing for one MCP
request, and a hosted session's request outlives a project lifecycle change:
an agent calls `ls "/"`, then create_memory_project, then addresses the new
project. invalidate_project_caches cleared the active project and the
account-wide workspace index but not that snapshot, so a project created
mid-session was unaddressable by name and a deleted one kept routing to its
dead external_id until the session expired.
Reproduced against this branch: after `ls "/"` populates the snapshot,
`ls "/"` keeps advertising the old mount table and
detect_project_from_identifier_prefix("new-project/notes/x") answers None,
while a deleted project still resolves with its removed external_id.
The exposure is not new to identifier detection — the same snapshot already
decided `ls "/"` and resolve_project_path_route — but routing detection through
it made read_note and search_notes newly depend on a cache nothing invalidated,
so it is fixed here. invalidate_project_caches now clears all three
project-shaped caches and says so, and invalidate_session_project_list lives
beside the cache it clears, so a fourth cache has one obvious function to join.
Workspace state is deliberately left alone: active_workspace and
available_workspaces hold tenant id, slug and type, which no project lifecycle
change touches and which no MCP tool can change at all — none creates or
deletes a workspace. A test pins that split so neither half drifts.
The cloud_session fixture now serves a mutable tenant listing, so a test can
add or remove a project the way the tools do.
Refs #1432
Signed-off-by: phernandez <paul@basicmachines.co>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c1bbae6601
ℹ️ 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".
Invalidation covers the project changes this session caused. A project created out of band — by a teammate, or by the CLI — was invisible for the life of the session, because `session_project_list` is session state: fastmcp keys set_state by session id and persists it across tool calls, contrary to the "memoized for one MCP request" comment this corrects. Reproduced against this branch: after `ls "/"` populates the snapshot, a project appearing in the tenant listing is absent from both `ls "/"` and detect_project_from_identifier_prefix, indefinitely. The loader now refetches once the snapshot passes 60 seconds. The bound lives there rather than at a call site, so identifier detection, the posix path resolver and the `ls "/"` mount view inherit one freshness policy and none of them holds a retry of its own — a miss-triggered refresh could not have covered `ls "/"` at all, since listing the mounts has no miss to trigger on. Age is the trigger, not a lookup miss, and that is the design point. The sibling snapshot refreshes on a miss (resolve_workspace_project_identifier, #956) because its miss is terminal — an explicitly named project that must resolve or raise — so the retry costs one listing per failing call. A miss against the mount table is the ordinary answer instead: most path-shaped identifiers are not project references, which is why detection returns None for them rather than refusing. A miss-triggered refresh there would put a fetch on the hottest path in the tool surface and let one non-existent identifier drive a listing per lookup. An age bound cannot be influenced by what the caller asks: at most one extra listing per interval per session, and none for a session that does not read. It remains far below what this path cost before #1432, when an unqualified prefix rebuilt the whole account-wide workspace index on every miss. The fetch time is stamped on every fetch, including a refetch that found nothing changed, so a run of misses against a genuinely absent project costs one listing per interval rather than one per lookup. A missing or future timestamp reads as stale, so state from a previous deploy and worker clock skew both expire early rather than pin a snapshot forever. Refs #1432 Signed-off-by: phernandez <paul@basicmachines.co>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #1432, closes #1435. Part of #1438.
Both are "an identifier or path is resolved or returned wrongly" in the MCP layer, and both come down to one question: what does a path-shaped identifier mean for a consumer with no project set in hand?
Reproduction (verified against
origin/mainbefore any change)#1432 — bare prefix leaves the session's own workspace. Session bound to workspace
beta(projects:research);acmeis another accessible workspace (is_default) holdingnotes:#1432 (comment) — multi-segment project unreachable. Session's own listing holds
Research/2026:#1432 (comment) —
memory://routes to a different project than the client is open for. Config holds bothresearchandResearch/2026:It also writes that wrong project into the request's
active_projectcache.#1435 —
find --metaresults don't round-trip:The decision for #1432
Three options were on the table: restrict the fallback to the session's workspace, adopt the posix refusal with a candidate list, or prefer the session workspace and fall back.
Chosen: restrict to the session's own set — and make it the same set, with the same precedence, that the posix resolver already uses.
resolve_project_path_routerefuses an unaddressable first segment because on that surface apathwhose first segment names nothing is an error. Onread_note/search, a first segment that names no project is the common input —specs/search-specis an ordinary permalink, and a search query is arbitrary text. This function is detection, not resolution:Nonemeans "not a project reference", and the active project is a correct answer.addressable_projects()is the only thing that can answer "the session's own workspace" in the mode the bug was reported in. The account-wide index cannot: it lists every accessible workspace with nothing marking which is the session's.addressable_projects()asks the session's own project-less route, which in a hosted session is the bound tenant.split_project_permalink_prefix, the existing splitter that takes a candidate set — which is also why the multi-segment half falls out for free. The deleted code was a second path splitter (_split_project_prefix, one segment) feeding a second lookup that resolved by form across workspaces.ls team/docs/x(mountteam) andread_note("team/docs/x")(workspaceteam) disagreed. This is also the cheaper order: a mount hit now builds no workspace index at all, where the deleted fallback always did.Detection returns
DetectedProjectRoute(project, project_id). Returning the name alone was not enough:get_project_clientre-resolves a bare name through the account-wide index, which prefers theis_defaultworkspace on a duplicate permalink — the reported repro. Callers forward both fields, exactly as the posix verbs forwardProjectPathRoute's pair.The
memory://half, and the costresolve_project_and_pathnow lets the project it is already routed to claim its own permalink with the same splitter — no lookup, no fetch, and it fixes the multi-segment case for every caller (all seven detect the prefix first).The name-resolving branch below it keeps the single-segment guess, deliberately. Weighed explicitly: it is reached only when no set in hand names the project, it already spends a
/v2/projects/resolveround trip, and giving it the multi-segment reading would mean a project-list fetch on every cross-projectmemory://read on top of that resolve. Local sessions pay nothing for the claim (config is the set); cloud sessions pay one per-request memoized listing that replaces workspace discovery plus per-workspace listings.Not overstated: a
memory://URL naming a multi-segment project other than the one already routed to, with detection bypassed (an explicitprojectparam), still resolves by the single-segment name. Every path that detects first — read_note, search, read_content, edit_note, delete_note, build_context, the note resource — is covered.Behaviour this removes
A session whose own route lists no projects (a stateless deployment with no injected client factory) loses bare-prefix routing; the
<workspace>/<project>/<path>spelling still works, and that session'sls /already advertised no mounts. Routing and the mount view now agree everywhere.#1435
qualify_search_pathsis the third response shape's qualifier, applied to the metadata arm's payload before field projection so both shapes of that response carry one spelling._find_by_metadatanow takes the wholeProjectPathRouteinstead of three fields pulled off it — it needs the route again to re-qualify.The guard.
test_path_accepting_verbs_are_the_ones_covered_belowcould not catch this:findwas already in its set and the gap was one arm inside it. Added a companion that drives one routed call per response shape, asserts everyqualify_*helper the module defines actually fired, and asserts every path those calls returned reads the same note throughcat. Precisely what the two guards close, and what they don't: a new response shape needs a new qualifier and fails the shape guard until a routed call exercises it; a new path-accepting verb fails the verb guard; neither can force a new arm to re-qualify — what covers that is that every shape a routed verb answers with already has a qualifier, so a new arm either reuses one or introduces one the shape guard then reports.Test evidence
Each hunk reverted individually, with the rest of the change in place:
detect_project_from_identifier_prefixtest_detect_project_prefix_stays_inside_the_sessions_own_workspace,test_detect_project_prefix_prefers_a_mount_over_a_same_named_workspace,test_detect_project_prefix_claims_a_multi_segment_projectresolve_project_and_pathtest_memory_url_prefix_is_claimed_by_the_routed_projects_own_permalinkqualify_search_pathsin_find_by_metadatatest_find_meta_returned_paths_route_back_to_the_same_project,test_every_response_qualifier_is_exercised_by_a_routed_calltest_find_meta_returned_paths_route_back_to_the_same_projectasserts a routedfind --metaresult — projected and unprojected — fed back intocatreads the same note.test_detect_project_from_identifier_prefix_falls_back_to_bare_project_name, which pinned the deleted behaviour, is replaced by the three tests above. Two tests that asserted the workspace-qualified route forwards a name without its id now assert it forwards the id.Verification
🤖 Generated with Claude Code
https://claude.ai/code/session_014pmKq6bqCi6Zp6BTHuZjrp