feat(mcp): route project-qualified paths in the POSIX tools - #1421
Merged
Conversation
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: bfd26bafa9
ℹ️ 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".
This was referenced Sep 1, 2026
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6224e1626a
ℹ️ 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".
Member
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6224e1626a
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1c57ccefcf
ℹ️ 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".
phernandez
force-pushed
the
1415-posix-project-routing
branch
from
September 1, 2026 18:51
1c57cce to
c940782
Compare
This was referenced Sep 1, 2026
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ab60585e07
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b71d9d0a77
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a004ac3ddb
ℹ️ 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".
Projects become mount points (#1415): every posix verb accepts <project>/path — exactly the prefixed identifiers tool outputs and stored permalinks produce — resolved by one shared helper the CLI inherits. Explicit project params win only on agreement; disagreement refuses naming both. ls with no project lists active projects as the root directory. In multi-project configs an unqualified path that matches no project refuses with the copyable project list; single project configs keep resolving unqualified paths unchanged. Motivated by measured agent behavior in the #1398 A/B runs: agents faithfully quote prefixed output identifiers, omit the project arg (shell affordances prime cwd thinking), and the stateless default was silently wrong — both surfaces wrote a perfect relation into the wrong project. Known scope gap for cloud/factory-mode surfaces noted in review; follow-up tracked on #1415. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014pmKq6bqCi6Zp6BTHuZjrp Signed-off-by: phernandez <paul@basicmachines.co>
The mount view and the routing resolver read different sources, so a cloud tenant could advertise /research in ls '/' while routing asked the local config, found nothing, refused nothing, and fell through to the default project. In a team workspace that default is one shared mutable is_default flag, so an unqualified call could silently read or write another member's project. Root cause of why the existing multi-project refusal never fired in the hosted server: BasicMemoryConfig always materializes a placeholder 'main' project, so config.projects is never empty and len(config.projects) > 1 was never a usable signal for a cloud session. Both surfaces now read one addressable_projects() set — config locally, the session's project listing in factory/cloud mode, memoized per MCP request. The refusal counts that same set, so anything ls '/' advertises is addressable, asserted as a property over the advertised list rather than a hand-picked example. Nothing consults is_default. Single-project workspaces still resolve unqualified references, and the local path adds no new call. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014pmKq6bqCi6Zp6BTHuZjrp Signed-off-by: phernandez <paul@basicmachines.co>
Workspace-qualified parsing ran before the advertised-project lookup, so
when a project's permalink was also an accessible workspace slug, the
next segment naming a project in that workspace won. With /team
advertised and workspace team holding project docs, cat('team/docs/x')
read workspace team's docs project instead of the docs directory in the
advertised team project — another project's data, from a name ls /
promised.
The advertised list is a promise, so it now claims the first segment
first; workspace-qualified spellings resolve only when no mount matches.
The collision this creates is pinned rather than swallowed: while a
project's permalink equals a workspace slug, that workspace's other
projects lose their qualified path spelling. They stay addressable
through the project parameter, and pairing the two now raises a prefix
conflict instead of quietly agreeing, which is what teaches the escape.
Two side effects worth having: mount-prefixed paths skip workspace
discovery entirely, and ls 'research' and ls 'research/notes' now agree
on the project spelling.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014pmKq6bqCi6Zp6BTHuZjrp
Signed-off-by: phernandez <paul@basicmachines.co>
Three review findings on the #1415 path resolver, all in the mount table that `ls "/"` advertises and `resolve_project_path_route` routes by. Keep advertised mounts bound to their workspace. A hosted session's own route is one tenant, but `get_project_client` re-resolves a bare project name against every accessible workspace, and project names are unique only inside one of them. With the session on a non-default workspace and the same project permalink in the default one, `resolve_workspace_project_ from_index` picked the default workspace's copy by its is_default flag on any call before a workspace was cached — so `cat("docs/x")` could read a different tenant's project under a name the session's own root had just advertised. `AddressableProject` now carries the project's external_id and `ProjectPathRoute` hands it on as `project_id`, which the index resolves exactly. An explicit workspace-qualified project still wins and drops the mount id with it, so that escape hatch is unchanged. Recognize workspace-qualified project roots. Workspace-qualified memory URLs require three segments, because `memory://main/notes` has to stay readable as project `main`. A posix path only reaches that parse after the mount table declined its leading segment, so nothing addressable can be meant by it and the two-segment form is unambiguous — but it was rejected anyway, leaving `ls "acme/docs/notes"` resolving while `ls "acme/docs"`, that same project's root, had no spelling at all and fell through to the multi-project refusal. The looser `split_workspace_route_segments` parse now serves the posix resolver only; memory URLs keep the strict form. Handle multi-segment project permalinks in mounts. Project names may contain '/', and generate_permalink keeps it, so a project 'Research/2026' advertises the two-segment mount '/research/2026'. Matching only the first segment listed that mount at the root and then could not enter it; the whole permalink now has to match, longest first. Regression tests cover each: the cross-workspace binding (id lands in the session's workspace where the bare name still falls to the default flag), the workspace-qualified root, the multi-segment mount and its shorter sibling, and the fail-soft/fail-loud split on a workspace that cannot be discovered. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014pmKq6bqCi6Zp6BTHuZjrp Signed-off-by: phernandez <paul@basicmachines.co>
Rule 4 of resolve_project_path_route called the generic identifier
detector, which — when no advertised mount claimed the leading segments
and the input was not '<workspace>/<project>/<path>' — fell back to
resolving the bare first segment against every accessible workspace.
In a hosted session that is a cross-tenant read. With the current
workspace holding only 'research' and another accessible workspace
holding 'notes', the ordinary project-relative path `cat("notes/foo")`
resolved to ProjectPathRoute(project='acme/notes', path='foo') and
served the other workspace's project.
This is the same class of bug as the preceding commit, reached by a
different path: there a bare project name was re-resolved across
workspaces and fell to the is_default flag; here an unqualified first
path segment does. Both violate the rule this PR establishes — an
identifier that names no addressable mount is refused, not resolved to
a plausible candidate somewhere else.
Rule 4 now parses only fully qualified routes: both segments must match,
the first an accessible workspace slug and the second a project inside
that workspace. _detect_workspace_project_root and
_detected_route_remainder collapse into one _detect_workspace_qualified_-
route that handles the pathless root and the path form together and
returns the remainder from the same parse that matched the route, so the
two can no longer disagree about how many segments were consumed. That
remainder helper existed only to special-case the bare-prefix fallback,
which is the leak.
Unchanged: advertised mounts still win the leading segments, explicit
project='acme/docs' still routes, 'acme/docs/notes' and the 'acme/docs'
root still resolve. An unqualified name that is not addressable here now
raises UnqualifiedPathRefusedError naming this session's own mounts.
detect_project_from_identifier_prefix keeps the bare-prefix fallback for
read_note and search, which have no mount table and no refusal rule; it
gains a direct test, since the posix resolver no longer exercises it.
Signed-off-by: phernandez <paul@basicmachines.co>
Two invariants the posix routing layer kept restating per call site, and one cost that fell out of fixing the second. Project identity is a whole permalink, not a segment. Mount matching learned that last commit; the workspace parser had rediscovered the same wrong assumption, so a workspace holding 'Research/2026' parsed 'acme/research/2026/notes' as project 'research' plus path '2026/notes' and either missed or, beside a real 'research', served the wrong project. The fix is not a third careful parser: split_project_permalink_prefix is now the only function that turns a path into a project plus a project-relative path, and it takes the candidate set as an argument, because how many segments a project consumes is a fact about the known projects and never about the string. Mount routing, workspace routes, and local-config prefix detection all go through it. What closes the class is the deletion, not the helper: split_workspace_identifier_segments, split_workspace_route_segments, and split_workspace_memory_url_segments are gone. They were the shape-only parses that could express "the project is segment one", and nothing is left to copy from. What remains is split_workspace_slug_prefix, which splits only the workspace slug — genuinely one segment — and hands the rest to the matcher. Reverting the longest-match loop alone now fails both the mount test and the workspace test, which is the evidence they are one rule. A path a routed verb returns must be a path the resolver accepts. A qualified 'ls research' strips the mount before the project-scoped API sees it, so the API answers '/notes', and feeding that back refused as unqualified — or opened a different project mounted as 'notes'. Verbs that can strip a prefix now re-attach it through one function. Only addressing fields move: directory_path and file_path are what callers feed back. permalink is deliberately left alone, being an identity with its own canonical form. Two tests that asserted the qualified and --project spellings return identical payloads now assert the round trip instead: the payloads legitimately differ by addressing frame, and frame-independence was what broke the loop. bm tree resolved twice. Once find returns qualified paths, the root tree strips has to carry the prefix too, so find_listing returns the listing and that root from one resolution. A CLI invocation carries no FastMCP context, so the second resolution was a second project-list round trip on every cloud call, and a second workspace index build for a qualified path. Memory-URL strictness is unchanged and now stated where it is enforced: resolve_workspace_qualified_identifier requires a path after the project, which is what keeps 'memory://main/notes' readable as project 'main'. The posix resolver keeps the pathless form and says why at its own call site. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014pmKq6bqCi6Zp6BTHuZjrp Signed-off-by: phernandez <paul@basicmachines.co>
phernandez
force-pushed
the
1415-posix-project-routing
branch
from
September 1, 2026 23:02
a004ac3 to
d862000
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d862000757
ℹ️ 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".
…lashes Two findings on a004ac3, both mine, and the second narrows a claim I made. Re-qualification corrupted note content. qualify_routed_paths walked the payload rewriting any key spelled file_path or directory_path, and a note's frontmatter is free-form user YAML in that same payload. A note whose author wrote `file_path: imports/source.md` came back as `second-project/imports/source.md`, disagreeing with both its own content and the file on disk — canonical content silently altered in what we hand back. Transport metadata and note content can spell a key the same way and only position tells them apart, so one rule still decides whether to requalify and with what (_route_prefix), and each response schema now says where: qualify_note_paths touches the top-level file_path only, qualify_listing_paths walks nodes and their children. frontmatter and permalink are never touched. The escape hatch failed for slash-bearing project names. With mount 'Research/2026' detected and project='acme/Research/2026' passed, _project_routes_agree split both identifiers on the first slash, read the detected mount as workspace 'Research' plus project '2026', and rejected two agreeing spellings as a conflict. That is worth stating plainly rather than patching quietly: my "the class is closed" claim was too broad. What I closed was one manifestation — turning a path into (project, project-relative path), which now has one implementation that cannot be called without the candidate set. The same root fact, that a project name may contain '/', has a second manifestation I left open: splitting an identifier into (workspace, project). split_qualified_project_identifier still guessed, and six call sites depended on the guess. Converted in this pass rather than one per report: - _project_routes_agree and its prefer_explicit twin are replaced by _agreed_route_project, which returns the project both spellings name and the spelling that wins from one comparison. It needs no candidate set because a workspace slug is exactly one segment, so the qualified spelling is the bare one plus exactly one leading segment. That accepts 'acme/Research/2026' for 'Research/2026' while still rejecting it for a project named '2026', which a plain suffix test would have got wrong. - resolve_workspace_project_from_index now tries the whole identifier as a project permalink before reading its first segment as a workspace, so a cloud project named 'Research/2026' is routable at all. The v2 project router already resolved exact-first; this matches it. - get_project_client's per-project-config branch uses the config key verbatim, since it came from config.projects and is the project name by construction. - resolve_project_and_path strips only the workspace slug it actually knows from context instead of guessing one. - unqualified_project_identifier is deleted; it had no remaining caller. split_qualified_project_identifier survives with a docstring saying what it cannot answer and that it is a fallback after an exact lookup misses. Both its remaining callers now order it that way. The honest statement is therefore: path-splitting is closed by construction; identifier-splitting is correct by ordering at both call sites, not by construction. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014pmKq6bqCi6Zp6BTHuZjrp 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: b61e391fca
ℹ️ 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".
In a cloud session mounting one project, an ordinary relative path left that
project whenever its leading segments happened to name an accessible workspace
and a project inside it. With 'research' the sole mount and workspace 'acme'
holding 'docs', cat("acme/docs/foo") served acme's docs project instead of
reading acme/docs/foo inside research. Passing project="research" did not
rescue it: the detected workspace route conflicted with the named project and
raised instead, so the path could neither stay home on its own nor be pinned
there.
Route versus path is not decidable by parsing. 'acme/docs/foo' is a well-formed
workspace route and a well-formed folder path, and neither the string nor the
set of existing projects recovers which the caller meant. So this is a
precedence order, deliberately, and it is written down above the section. One
question settles it — does the call already say which project it means?
1. An explicit project (param or env constraint) says so. The remaining path
is inside it and nothing reroutes. A prefix naming a different addressable
mount still conflicts: that is a contradiction in one call, not ambiguity.
2. Otherwise, with several projects addressable, an unqualified path cannot
resolve at all — it refuses. Reading the leading segments as a route is
then the only way the input can mean anything, so route wins.
3. Otherwise one project is addressable, the path already resolves inside it,
and route parsing would take a working read and send it to another tenant.
Path wins.
Mounts keep sitting above all three: a name `ls /` advertises always addresses
that mount. In code this is two conditions on rule 4 rather than a special
case, and they are the whole rule.
Unchanged: unaddressable unqualified names still refuse, mount precedence and
the prefix conflict still hold, 'acme/docs/notes' and the 'acme/docs' root still
resolve wherever an unqualified path would have refused anyway, and memory-URL
semantics are untouched.
One existing test moved from one mounted project to two:
test_cloud_workspace_project_root_surfaces_a_failed_workspace is about the
failed-workspace error, and workspace routes are now only parsed where an
unqualified path could not resolve.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014pmKq6bqCi6Zp6BTHuZjrp
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: 8214e043ce
ℹ️ 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".
Regression from the exact-first ordering in the previous commit, reported on that change. With workspace 'acme' holding 'docs' and another accessible workspace holding a project literally named 'acme/docs', the posix route 'acme/docs/x' resolved its workspace correctly, then handed on only the qualified *name* — and the index, preferring a whole-permalink match, read it as the other tenant's project and ran the call there. Fixed at both levels, because they fail independently. The index now tries both readings, qualified first. Qualified wins because it names a workspace explicitly: taking it for some other workspace's whole permalink runs the call against a tenant the caller did not name. The whole-permalink reading stays as the fallback, which is what makes a slash-bearing project name routable at all — 'Research/2026' still resolves where no workspace is called 'Research'. This is the same shape of answer as the route-versus-path order: two legitimate readings, one written-down precedence, not a parser that pretends to know. And the route no longer throws the answer away. _detect_workspace_qualified_route already holds the resolved entry, so it hands on that entry's external_id the way the mount table has since mounts were bound to their workspace. A route that resolved an entry now always carries its id, so nothing downstream re-resolves it by a name that can collide. With the id present the index's ordering is not even consulted for this path — the ordering fix is what covers the explicit project= spelling, which has no id to carry. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014pmKq6bqCi6Zp6BTHuZjrp Signed-off-by: phernandez <paul@basicmachines.co>
Two P2s, both about a mount that the routing rules cannot enter. A project name whose permalink is empty is unaddressable. generate_permalink reduces pure punctuation or emoji to "", so a project named '!!!' or a fire emoji advertised itself at the root as directory_path '/', was indistinguishable from every other such project in the mount list, and could not be entered by the path it advertised — feeding '/' back re-invoked the mount listing. Refused at add_project, the one boundary that creates projects, so an unaddressable mount cannot come into existence rather than being coped with downstream. Names still only need one letter, digit, or CJK character. The pathless workspace root now gets the same permission as the path form. In a locally routed session holding cloud credentials, 'acme/docs/note' resolved through workspace discovery while 'acme/docs' — that same project's root, the thing ls needs to enter it — refused, because the shared gate demands three segments outside factory or explicit-cloud sessions. That three-segment requirement belongs to *identifier* detection, which serves read_note and search: they have no mount table to decline the segments first and no refusal rule behind them, so only the unmistakable form may reach for the network. The posix resolver has both, and by the time it asks, the mount table has declined, no project was named, and several are addressable — so an unqualified path refuses anyway and route parsing takes nothing away. The gate is split in two named pieces that say which question each answers. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014pmKq6bqCi6Zp6BTHuZjrp 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: 1f6194f534
ℹ️ 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".
Follow-up on the precedence gate from 8214e04, reported against it. A factory session whose connection-time workspace holds no projects fell through both halves of the order: 'acme/docs/note' skipped workspace parsing because the session did not address more than one project, and then skipped the refusal for the same reason, so it routed with project=None into the empty workspace instead of the accessible project the caller had named. The order already said why this is wrong, and the code said "more than one" where the reasoning said "exactly one". Path wins over route because the path already resolves inside the session's sole project; with no projects there is nothing for it to resolve inside, so route parsing takes nothing away — and an empty mount table cannot refuse on the caller's behalf either. The gate now reads != 1, and the note above the section says exactly one and why. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014pmKq6bqCi6Zp6BTHuZjrp 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: 8fcac4204e
ℹ️ 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".
Follow-up on the empty-permalink validation in 8fcac42, reported against it. That check stripped slashes before testing, so a project named '/foo' passed: its permalink is '/foo', stripping leaves 'foo', and the name looked fine. The mount view then advertised '//foo' while the resolver, which strips leading slashes off the candidate before matching, could never match the stored '/foo' — the advertised mount could not be entered, and in a single-project setup the path fell through to the default instead. The right test is the one the resolver actually performs: it matches a permalink segment by segment, so every segment must be non-empty. That covers both shapes at once — '' (pure punctuation or emoji) and '/foo' (empty leading segment) — and it is the same rule split_project_permalink_prefix applies to candidate paths, where an empty interior segment already refuses rather than being repaired. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014pmKq6bqCi6Zp6BTHuZjrp 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: 19662cb7e7
ℹ️ 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".
Follow-up on the precedence gate, reported against it, and a cost I saw when
writing that gate and should have reported rather than accepted quietly.
Rule 2 has always stripped a path prefix that agrees with the explicit project.
That only worked for spellings the mount table recognizes; a workspace-qualified
project reached agreement solely through rule 4's discovery, which the
precedence order now declines to run when a project was named. So the caller's
own two spellings of one project stopped agreeing:
cat("acme/docs/foo", project="acme/docs") -> path 'acme/docs/foo'
which asks the already-selected docs project for a nested path that is not
there. It also broke the round trip the requalification fix established, since
a routed ls("acme/docs") returns exactly that prefixed form.
The agreement never needed the network. It is the caller's own two spellings of
one project, so it is decidable locally: match the explicit project's permalink
against the candidate's leading segments with the same primitive everything else
uses, and strip on a hit. That runs before the rule 4 gate and leaves the gate
alone — a prefix naming something *other* than the named project still stays
part of the path, which is the behaviour the previous commit established.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014pmKq6bqCi6Zp6BTHuZjrp
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: 6f1bd64b55
ℹ️ 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".
Reverses the mount-count half of the precedence gate from 8214e04, because it made this layer's own emitted addresses unroutable. In a factory session whose sole mount is 'docs', ls("docs", project="acme/docs") routes to acme/docs, strips the agreeing prefix, and requalifies its children as 'acme/docs/...'. Replaying one without the project argument hit the len(addressable) != 1 gate, skipped workspace parsing, and read the sole mount's same-named path in the *other* tenant. A navigation path returned for one workspace silently read another. The two findings are in direct tension and both are silent wrong-project reads, so the tie cannot break on harm. It breaks on whose string it is. The canonical qualified form is one we emit and publish as an address, so it has to route back. A user-typed relative path that coincidentally spells a real accessible workspace *and* a real project inside it is a collision, and it has an explicit, documented fix: name the project, and rule 1 keeps the whole path inside it — which is exactly what the earlier finding asked for and what rule 3b now makes work for the qualified spelling too. Coincidence loses to the published address. Rule 2 therefore no longer consults the mount count. It still requires both halves to match a real workspace and a real project in it, so a merely workspace-shaped path like 'notes/2026/foo' matches nothing and stays relative on its own — that case is pinned alongside the replay case. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014pmKq6bqCi6Zp6BTHuZjrp 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: f66b3b7dd3
ℹ️ 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".
Distinct names can normalize to one permalink — 'My Docs' beside 'my-docs' —
and the permalink is the address, so that is one address for two projects.
ls("/") advertised both at /my-docs while the mount lookup kept whichever sorted
last, so every qualified read under that prefix went to one project and the
other was unreachable. Worse, the mount lookup and _canonicalize_project_name
disagreed about which one: the route reported 'My Docs' while the mount entry
was 'my-docs', so a path could read one project's content under the other's
name.
Refused in two places, because they cover different populations.
add_project rejects a name whose permalink matches an existing project's, which
is where the second of the pair would be created. Only a config written before
this check, or edited by hand, can still hold a collision.
For those, the resolver refuses the route instead of picking. A silent
wrong-project read is worse than a loud failure, so AmbiguousMountError names
both projects and points at the two ways out — rename one, or pass project=
with the exact name. ls("/") still lists both, which is where the collision is
visible.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014pmKq6bqCi6Zp6BTHuZjrp
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: 195c48b481
ℹ️ 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".
Regression from the qualified-first ordering in 1f6194f, reported against it. The probe that decides whether the qualified reading resolves carried its own inline matcher — one pass over the workspaces taking the first of slug, tenant_id, or display name to hit. match_workspace_identifier gives slugs *global* precedence over tenant ids over display names, so the two disagreed whenever one workspace's display name equalled another's slug. With 'foo' as one workspace's display name and another's slug, resolving 'foo/docs' probed the display-name workspace, found no 'docs' in it, called the qualified reading a miss, and fell back to a whole-permalink project literally named 'foo/docs' in a third workspace. A route naming a real slug read across two workspace boundaries. The precedence now has one definition. find_workspace_identifier holds it and returns None when nothing matches, so a caller probing whether a segment *is* a workspace can ask without catching; match_workspace_identifier is that plus the not-found error. Same shape as the other fixes in this series: the bug was a second implementation of a rule that already existed, so the fix is to delete the second one rather than teach it the same lesson. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014pmKq6bqCi6Zp6BTHuZjrp 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: 52754f96a7
ℹ️ 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".
With local projects 'docs' and 'team/docs' both present,
cat("team/docs/note", project="docs") detected the 'team/docs' mount and then
the shape heuristic read its extra leading segment as a workspace qualifier for
'docs'. The two spellings "agreed", the explicit selection was discarded, and
the call read note from team/docs instead of raising ProjectPrefixConflictError.
Two real projects were treated as one project spelled two ways.
Identity now settles it, and the strings are never consulted when it can. In
the explicit branch `detected` is always a mount — rule 4 does not run when a
project was named, and rule 3b sets detected to the explicit value itself — so
when `explicit` also names an addressable project, both sides have a resolved
local identity. Same project, they agree; different projects, that is a
contradiction in one call and it raises, whatever their shapes suggest.
The shape comparison is not removed, and is worth being explicit about. It now
runs in exactly one case: `explicit` names no addressable project, so it
addresses another workspace. There is no local identity to compare against, and
resolving one would need the workspace discovery that rule 1 deliberately skips
when a project was named — so the segment-count rule stays as the fallback for
identities this session cannot resolve, rather than as a shortcut around ones
it can. That keeps project='acme/docs' and project='other/second-project'
working as the documented cross-workspace escape hatch.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014pmKq6bqCi6Zp6BTHuZjrp
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: 1303e8a79b
ℹ️ 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".
The ambiguity check from 9ba36c5 raised while building the lookup table, before knowing whether the requested path used the duplicated permalink. With 'My Docs' and 'my-docs' in a config, every non-empty path in the session failed — an unrelated 'other/note' among them — and so did cat("note", project="Other"), which is the exact-name escape hatch the error message itself recommends and which never goes through that lookup at all. A stale pair of entries somewhere in the config broke the whole session. An ambiguity should fail the calls that depend on it and no others, so the loop now records the colliding pair instead of rejecting the table, and the error fires only when the permalink a path actually claimed is one of them. It is not weakened: a path naming both projects still refuses rather than picking, and still names both so the caller can act. That makes the suggested escape hatch real, and the second half makes it correct. canonicalize_project_name matched by permalink alone, so a caller who named one colliding project exactly got whichever the config listed first — possibly the other project. It now takes an exact configured name before falling back to permalink matching, the same exact-before-fuzzy ordering the v2 project router and the workspace index already use. Naming either side exactly now reaches that side. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014pmKq6bqCi6Zp6BTHuZjrp 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.
Why
Implements #1415: projects become mount points in the POSIX tool namespace. Motivated by measured agent behavior in the #1398 A/B runs — tool outputs return project-prefixed identifiers, inputs demanded a separate
projectarg, agents faithfully quote outputs and omit the arg (shell affordances prime cwd-thinking), and the stateless default was silently wrong. In run 5, both surfaces' agents wrote a perfect relation into the wrong project.Stacked on #1416 (
1398-eval-test-fixes).What changed
mcp/project_context.py): first path/identifier segment naming an active project routes there with the remainder — inputs now accept exactly what outputs produce. The CLI verbs inherit it through the shared layer.projectparam + path prefix disagreeing refuses naming both; a workspace-qualified explicit param keeps its workspace (review catch — the agree-branch initially discarded it).ls /lists active projects as the root directory — in-band discovery, the mount-point view.no project 'x' — active projects: …); single-project configs resolve unqualified paths unchanged.Validated by the eval it came from (run
at-b0442a6f5cd7, Sonnet 5)The entire wrong-scope failure class vanished.
curate-connect— which agents had "solved" into the wrong project on every prior run — passed on both surfaces. Posix's metadata-search went 0/3 → 3/3 (agents brute-force it at ~2× rich's token cost, which is the remaining efficiency gap: grep/ls/find have no structured frontmatter predicates). Where the surfaces overlap cleanly, posix stays 17–24% cheaper (manual 23.8k vs 31.3k; curate 74.7k vs 89.9k). Aggregate tokens/completed: rich 72.9k, posix 88.0k (+21%, driven by the metadata brute-forcing and navigational chattiness — 13.8 vs 7.8 mean calls).Remaining failures are genuine task behavior (decoy discrimination on both surfaces, one formatting miss, one incomplete status update, one token-budget stop) — none are scope errors.
Verification
ty— clean;tests/mcp1030 passed;tests/cli933 passed (21 new resolver tests + 16 routing tests; additive-only test diffs)🤖 Generated with Claude Code
https://claude.ai/code/session_014pmKq6bqCi6Zp6BTHuZjrp