fix(cli,mcp): send raw collection slug so an alias can't shadow a real collection (BUG-2630) - #1162
Merged
Merged
Conversation
…l collection (BUG-2630)
The client-side alias map (collections.NormalizeSlug) rewrote seven hardcoded
singulars ("task", "plan", …) to their plurals BEFORE the request. In a
workspace whose collection slug IS one of those singulars, the user's exact
name was rewritten away and their create/list/move landed in a DIFFERENT
collection — silently, with a success message naming the wrong one.
Fix, per lead ruling on the BUG-2630 trail, split by transport:
CLI (real HTTP, may hit a pre-resolver server) — Option 2, one shared helper
cli.WithCollectionAliasFallback: send the RAW slug first (the server's
exact-match-first resolver from BUG-2578 wins, so an exact name is never
shadowed), and retry with the alias ONLY on a collection-not-found error, only
when the alias differs. Keying on collection-not-found is load-bearing: a
request to a collection that exists but fails for another reason is never
retried into the alias (that would recreate the bug). Both the schema fetch and
the create funnel through the helper so typed --field values parse against — and
the item lands in — one collection. On a genuine double-miss the error names the
RAW slug the user typed (collection "widget" not found), not the alias.
MCP remote transport (in-process ServeHTTP against the SAME binary, which always
carries the resolver — no version skew) — drop client-side normalization
entirely and send raw. Also removed the dormant expandPath collection
normalization: no routeSpec uses a {collection}/{target_collection} path
placeholder, so the branch was dead code in the area this fixes.
Search is deliberately out of scope (filed BUG-2659): its collection is a global
c.slug=? FILTER, not a path — a miss returns 200 + zero results, not
collection-not-found, so the retry can't key on it; and handleSearch is
cross-workspace, so the per-workspace resolver has no single workspace to run
against. Cross-workspace copy is excluded too (DR-13 forbids auto-retrying the
copy mutation).
Verified live against a real server: create/list/move into a singular collection
that collides with its plural now land in the named singular; shorthand still
resolves; genuine misses error naming the raw slug. New MCP integration test
reproduces the original shadow (item → PLANS-1) when normalization is restored.
Claude-Session: https://claude.ai/code/session_017jD6t1zjxGSq47SQpZfp1V
…ate the CLI retry (BUG-2630 Codex r1) Addresses all three Codex round-1 findings, via the lead's ruling that dissolves the earlier "retry vs archived-protection" tension by making the server the sole owner of resolution semantics. Finding #2 (MCP lost the legacy abbreviations t/i/p/d and phase/phases -> plans, which the server's ±s resolver did not cover): fold the legacy alias map into collectionSlugCandidates as a LAST-resort candidate. Exact-match-first and the archived-claims refusal run for the input and every structural candidate before the alias is reached, so it never shadows or redirects around a real/archived collection. Now every client can send the raw slug — including the MCP transport that can't retry — and lose nothing. Finding #1 (the client retry re-opened the archived/hidden redirect the server deliberately refused, because not_found can't be told from absent): add a collection_resolution capability flag to GET /server/capabilities and gate the CLI retry on it. Happy path unchanged (raw slug, one request). On collection-not-found ONLY, the client probes capabilities once (cached): if the server advertises resolution, its not-found is authoritative — the slug is absent, archived, or hidden — so the client does NOT retry. Only an older server that lacks the flag (or 404s the endpoint) triggers the legacy alias retry, which is non-regressive there since old servers never had the protection. The probe fails safe toward retry. This makes the follow-up distinct-error-code bug unnecessary. Finding #3 (double-fail masked a substantive alias error as "collection not found"): the helper now surfaces a substantive alias-attempt error verbatim, and only collapses to the raw-named not-found when the alias ALSO 404s. Verified live against a resolving server: create/list/move into a singular that collides with its plural land in the named singular; the abbreviation `i` resolves to `ideas`; and after archiving `plan`, `create plan` honestly fails ("collection \"plan\" not found") instead of being retried into a live `plans`. Gates: make lint 0 issues; go test ./... green; make test-pg green. Claude-Session: https://claude.ai/code/session_017jD6t1zjxGSq47SQpZfp1V
…up (BUG-2630 Codex r2) P1: the capability probe cached ANY failure as "no resolver", so a single transient blip (timeout/5xx) permanently re-enabled the alias retry and could bypass the archived/hidden protection on a resolving server. Now the probe distinguishes a DEFINITIVE verdict (HTTP 200 with the flag, or a clean 404 = legacy build) from an INDETERMINATE one (transport error / 5xx): only definitive verdicts are cached, and an indeterminate probe fails CLOSED (trusts the not-found, no retry) without caching, so the next call re-probes. A genuine old server still returns a clean 404, so its retry is unaffected. Renamed the predicate to CollectionNotFoundIsAuthoritative to name what it actually decides. P2: the create schema lookup hits exact-match-only GetCollection, which does NOT resolve slugs server-side, so capability-gating it made `create task --field amount=3` 404 the schema fetch, skip the retry, and send amount as the string "3". The schema lookup now always retries the alias (nil gate), restoring typed-field parsing against an aliased collection's schema. Best-effort as before: a genuine miss still degrades to string fields. New client test covers the probe: definitive verdicts cache (one probe), and a transient failure fails closed AND re-probes on the next call (mutation-verified). Claude-Session: https://claude.ai/code/session_017jD6t1zjxGSq47SQpZfp1V
…symmetry (BUG-2630) Per lead review: make explicit in CollectionNotFoundIsAuthoritative's doc that failing closed on an indeterminate capability probe is deliberate — a recoverable alias-shorthand miss is the safer side of the trade vs a retry doing an un-undoable wrong-write. Comment-only. Claude-Session: https://claude.ai/code/session_017jD6t1zjxGSq47SQpZfp1V
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.
What
The client-side alias map (
collections.NormalizeSlug) rewrote seven hardcoded singulars (task,plan, …) to their plurals before the request. In a workspace whose collection slug IS one of those singulars, the user's exact name was rewritten away and their create/list/move landed in a different collection — silently, with a success message naming the wrong one.Reproduced live before the fix (from the BUG-2630 body):
pad item create plan "probe"in a workspace with bothplanandplans→ item lands inplans,planstays empty.Fix (per lead ruling on the BUG-2630 trail — split by transport)
CLI (real HTTP, may hit a server that predates the BUG-2578 resolver) — Option 2, one shared helper
cli.WithCollectionAliasFallback:--fieldvalues parse against — and the item lands in — one collection.collection "widget" not found), not the alias.MCP remote transport (in-process
ServeHTTPagainst the same binary, which always carries the resolver — no version skew) — drop client-side normalization entirely, send raw. Also removed the dormantexpandPathcollection-normalization: no routeSpec uses a{collection}/{target_collection}path placeholder, so the branch was dead code in the area this fixes.Deliberately out of scope
c.slug = ?filter, not a path — a miss returns 200 + zero results, not collection-not-found, so the retry can't key on it; andhandleSearchis cross-workspace, so the per-workspace resolver has no single workspace to resolve against. Client search normalization left status-quo (read-only, no worse than today).Evidence
plan=1,plans=0); shorthandtask→tasksstill resolves via the server resolver; genuine misses error naming the raw slug.TestHTTPItemCreate_ExactSingularNotShadowedByAlias) drives the live in-process dispatcher; mutation-restoring normalization makes it fail by reproducing the original shadow (item →PLANS-1).make lint0 issues ·go test ./...green. (No store SQL →make test-pgN/A; no web change → web checks N/A.)https://claude.ai/code/session_017jD6t1zjxGSq47SQpZfp1V