mirror of
https://github.com/PerpetualSoftware/pad.git
synced 2026-09-10 15:05:40 +00:00
fix(cli,mcp): send raw collection slug so an alias can't shadow a real collection (BUG-2630) (#1162)
* fix(cli,mcp): send raw collection slug so an alias can't shadow a real 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
* fix(server,cli): own collection resolution server-side + capability-gate 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
* fix(cli): fail-closed capability probe + always-retry the schema lookup (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
* docs(cli): note fail-closed-on-indeterminate as a deliberate safety asymmetry (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 commit is contained in:
+36
-8
@@ -199,7 +199,14 @@ Run with --help-collections to see available collections and their status values
|
||||
client, _ := getClient()
|
||||
ws := getWorkspace()
|
||||
|
||||
collSlug := normalizeCollectionSlug(args[0])
|
||||
// Send the RAW slug the user typed; the server's exact-match-first
|
||||
// resolver (BUG-2578) handles shorthand without letting an alias
|
||||
// shadow a real collection of the same singular name (BUG-2630).
|
||||
// The alias fallback below preserves shorthand against servers that
|
||||
// predate the resolver. The schema fetch and the create both go
|
||||
// through the SAME fallback so typed --field values parse against —
|
||||
// and the item lands in — one collection, never a mismatched pair.
|
||||
rawSlug := args[0]
|
||||
title := args[1]
|
||||
|
||||
// Build fields JSON from flags
|
||||
@@ -228,7 +235,14 @@ Run with --help-collections to see available collections and their status values
|
||||
// degrades gracefully: all values stay as strings, matching
|
||||
// pre-fix behavior.
|
||||
var collSchema models.CollectionSchema
|
||||
if coll, err := client.GetCollection(ws, collSlug); err == nil {
|
||||
// The schema lookup hits exact-match-only GetCollection, which does
|
||||
// NOT resolve slugs server-side, so its not-found is never
|
||||
// authoritative about an alias — always retry (nil gate) so a typed
|
||||
// --field parses against the aliased collection's schema (Codex r2
|
||||
// P2). Best-effort: a miss degrades to string fields either way.
|
||||
if coll, err := cli.WithCollectionAliasFallback(rawSlug, nil, func(slug string) (*models.Collection, error) {
|
||||
return client.GetCollection(ws, slug)
|
||||
}); err == nil {
|
||||
_ = json.Unmarshal([]byte(coll.Schema), &collSchema)
|
||||
}
|
||||
for _, kv := range fieldFlags {
|
||||
@@ -298,7 +312,9 @@ Run with --help-collections to see available collections and their status values
|
||||
input.AgentRoleID = &role.ID
|
||||
}
|
||||
|
||||
item, err := client.CreateItem(ws, collSlug, input)
|
||||
item, err := cli.WithCollectionAliasFallback(rawSlug, client.CollectionNotFoundIsAuthoritative, func(slug string) (*models.Item, error) {
|
||||
return client.CreateItem(ws, slug, input)
|
||||
})
|
||||
if err != nil {
|
||||
// TASK-788: emit structured marker so MCP stdio classifier
|
||||
// can surface ErrPlanLimitExceeded instead of ErrServerError.
|
||||
@@ -486,7 +502,12 @@ Examples:
|
||||
var err error
|
||||
|
||||
if len(args) > 0 {
|
||||
items, err = client.ListCollectionItems(ws, normalizeCollectionSlug(args[0]), params)
|
||||
// Raw slug first so an exact collection name is never shadowed
|
||||
// by its singular alias (BUG-2630); the fallback preserves
|
||||
// shorthand against pre-resolver servers (BUG-2578).
|
||||
items, err = cli.WithCollectionAliasFallback(args[0], client.CollectionNotFoundIsAuthoritative, func(slug string) ([]models.Item, error) {
|
||||
return client.ListCollectionItems(ws, slug, params)
|
||||
})
|
||||
} else {
|
||||
items, err = client.ListItems(ws, params)
|
||||
}
|
||||
@@ -1557,9 +1578,8 @@ Examples:
|
||||
ws := getWorkspace()
|
||||
|
||||
input := map[string]any{
|
||||
"target_collection": normalizeCollectionSlug(args[1]),
|
||||
"actor": "user",
|
||||
"source": "cli",
|
||||
"actor": "user",
|
||||
"source": "cli",
|
||||
}
|
||||
|
||||
// Parse field overrides
|
||||
@@ -1575,7 +1595,15 @@ Examples:
|
||||
input["field_overrides"] = overrides
|
||||
}
|
||||
|
||||
moved, err := client.MoveItemWithForce(ws, args[0], input, force)
|
||||
// Raw target slug first so an exact collection name is never
|
||||
// shadowed by its alias (BUG-2630); the fallback preserves shorthand
|
||||
// against pre-resolver servers (BUG-2578). Retrying on
|
||||
// collection-not-found is safe: that error means the move never
|
||||
// mutated anything, so no double-write can result.
|
||||
moved, err := cli.WithCollectionAliasFallback(args[1], client.CollectionNotFoundIsAuthoritative, func(slug string) (*models.Item, error) {
|
||||
input["target_collection"] = slug
|
||||
return client.MoveItemWithForce(ws, args[0], input, force)
|
||||
})
|
||||
if err != nil {
|
||||
// IDEA-1494 R3 P1: render the open-children rejection
|
||||
// the same way the regular update path does, so
|
||||
|
||||
@@ -0,0 +1,88 @@
|
||||
package cli
|
||||
|
||||
import (
|
||||
"net/http"
|
||||
"net/http/httptest"
|
||||
"sync/atomic"
|
||||
"testing"
|
||||
)
|
||||
|
||||
// capsHandler serves /api/v1/server/capabilities with a swappable status/body
|
||||
// and counts hits, so a test can assert BOTH the verdict and whether a probe
|
||||
// was (re)issued.
|
||||
type capsHandler struct {
|
||||
status atomic.Int32
|
||||
body atomic.Value // string
|
||||
hits atomic.Int32
|
||||
}
|
||||
|
||||
func (h *capsHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
|
||||
if r.URL.Path != "/api/v1/server/capabilities" {
|
||||
w.WriteHeader(http.StatusNotFound)
|
||||
return
|
||||
}
|
||||
h.hits.Add(1)
|
||||
w.WriteHeader(int(h.status.Load()))
|
||||
if b, _ := h.body.Load().(string); b != "" {
|
||||
_, _ = w.Write([]byte(b))
|
||||
}
|
||||
}
|
||||
|
||||
func TestCollectionNotFoundIsAuthoritative_Definitive_CachesVerdict(t *testing.T) {
|
||||
for _, tc := range []struct {
|
||||
name string
|
||||
status int
|
||||
body string
|
||||
want bool
|
||||
}{
|
||||
{"resolves", http.StatusOK, `{"collection_resolution":true}`, true},
|
||||
{"advertises false", http.StatusOK, `{"collection_resolution":false}`, false},
|
||||
{"legacy 404", http.StatusNotFound, "not found", false},
|
||||
} {
|
||||
t.Run(tc.name, func(t *testing.T) {
|
||||
h := &capsHandler{}
|
||||
h.status.Store(int32(tc.status))
|
||||
h.body.Store(tc.body)
|
||||
ts := httptest.NewServer(h)
|
||||
defer ts.Close()
|
||||
c := NewClientFromURL(ts.URL)
|
||||
|
||||
if got := c.CollectionNotFoundIsAuthoritative(); got != tc.want {
|
||||
t.Fatalf("first call = %v, want %v", got, tc.want)
|
||||
}
|
||||
// A definitive verdict is cached: the second call must NOT re-probe.
|
||||
if got := c.CollectionNotFoundIsAuthoritative(); got != tc.want {
|
||||
t.Fatalf("second call = %v, want %v", got, tc.want)
|
||||
}
|
||||
if h.hits.Load() != 1 {
|
||||
t.Fatalf("expected exactly one probe (definitive verdict cached), got %d", h.hits.Load())
|
||||
}
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
func TestCollectionNotFoundIsAuthoritative_Transient_FailsClosedAndReprobes(t *testing.T) {
|
||||
// Codex r2 P1: a transient probe failure (here a 5xx) must NOT be cached as
|
||||
// "legacy" — caching it would permanently re-enable the alias retry and let
|
||||
// a write bypass the archived/hidden protection on a resolving server.
|
||||
h := &capsHandler{}
|
||||
h.status.Store(http.StatusInternalServerError)
|
||||
h.body.Store("boom")
|
||||
ts := httptest.NewServer(h)
|
||||
defer ts.Close()
|
||||
c := NewClientFromURL(ts.URL)
|
||||
|
||||
// Fail CLOSED: an indeterminate probe trusts the not-found (no retry).
|
||||
if got := c.CollectionNotFoundIsAuthoritative(); !got {
|
||||
t.Fatalf("transient probe must fail closed (true), got false")
|
||||
}
|
||||
// And it must NOT be cached: once the blip clears, the real verdict wins.
|
||||
h.status.Store(http.StatusOK)
|
||||
h.body.Store(`{"collection_resolution":false}`)
|
||||
if got := c.CollectionNotFoundIsAuthoritative(); got {
|
||||
t.Fatalf("after the blip cleared the verdict must be re-probed (false), got true")
|
||||
}
|
||||
if h.hits.Load() != 2 {
|
||||
t.Fatalf("expected a re-probe after the transient failure, got %d probes", h.hits.Load())
|
||||
}
|
||||
}
|
||||
@@ -14,6 +14,7 @@ import (
|
||||
"path/filepath"
|
||||
"strconv"
|
||||
"strings"
|
||||
"sync"
|
||||
"time"
|
||||
"unicode"
|
||||
|
||||
@@ -32,6 +33,15 @@ type Client struct {
|
||||
streamClient *http.Client
|
||||
authToken string // session or API token, sent as Authorization: Bearer
|
||||
agentName string // optional agent name, sent as X-Pad-Agent header
|
||||
|
||||
// capMu guards the lazy, cached probe of GET /server/capabilities behind
|
||||
// CollectionNotFoundIsAuthoritative. capProbed is set only once a DEFINITIVE
|
||||
// answer is cached (a 200 with the flag, or a clean 404); a transient probe
|
||||
// failure leaves capProbed false so a later call re-probes rather than
|
||||
// poisoning the cache. capResolves is the cached definitive verdict.
|
||||
capMu sync.Mutex
|
||||
capProbed bool
|
||||
capResolves bool
|
||||
}
|
||||
|
||||
func NewClient(host string, port int) *Client {
|
||||
@@ -238,6 +248,83 @@ func (c *Client) CreateItem(wsSlug, collSlug string, input models.ItemCreate) (*
|
||||
return &result, c.post("/workspaces/"+wsSlug+"/collections/"+collSlug+"/items", input, &result)
|
||||
}
|
||||
|
||||
// CollectionNotFoundIsAuthoritative reports whether a collection-not-found from
|
||||
// this server should be TRUSTED — i.e. the alias retry in
|
||||
// WithCollectionAliasFallback should be SKIPPED. It answers the question the
|
||||
// helper actually needs, which is not quite "does the server resolve": it also
|
||||
// has to say the safe thing when the answer is unknown.
|
||||
//
|
||||
// - Server advertises collection_resolution=true → true. It already tried the
|
||||
// singular/alias fallback and enforced exact-match + the archived/hidden
|
||||
// refusal (resolveItemCollectionSlug, BUG-2578/2630), so its not-found is
|
||||
// final: do not retry.
|
||||
// - Server DEFINITIVELY lacks the resolver — a clean 404 (no such endpoint) or
|
||||
// an explicit collection_resolution=false → false. Retry the legacy alias;
|
||||
// that old build never had the archived-claims protection a retry could
|
||||
// defeat, so the retry is non-regressive there.
|
||||
// - Probe is INDETERMINATE (a transport error, timeout, or 5xx) → true, and
|
||||
// the result is NOT cached. Failing CLOSED here is a DELIBERATE asymmetry:
|
||||
// the cost of failing closed on a blip is at worst one alias-shorthand
|
||||
// failure the user can simply re-run, whereas failing OPEN (retrying) risks
|
||||
// a wrong-write that bypasses the archived/hidden protection and cannot be
|
||||
// un-done. A recoverable UX miss is always the safer side of that trade.
|
||||
// Not caching matters for the same reason: a single transient blip must not
|
||||
// permanently re-enable the retry for the rest of the session. The only
|
||||
// case this could "cost" is an old server whose capabilities probe
|
||||
// transiently errors instead of returning a clean 404 — but a missing route
|
||||
// returns 404, not a transient error, so a genuine old build still retries.
|
||||
//
|
||||
// The definitive verdict is cached (static for the server's lifetime); an
|
||||
// indeterminate probe is re-tried on the next call.
|
||||
func (c *Client) CollectionNotFoundIsAuthoritative() bool {
|
||||
c.capMu.Lock()
|
||||
defer c.capMu.Unlock()
|
||||
if c.capProbed {
|
||||
return c.capResolves
|
||||
}
|
||||
resolves, definitive := c.probeCollectionResolution()
|
||||
if !definitive {
|
||||
// Fail closed without caching: trust the not-found for THIS call, but
|
||||
// re-probe next time in case the blip clears.
|
||||
return true
|
||||
}
|
||||
c.capResolves = resolves
|
||||
c.capProbed = true
|
||||
return resolves
|
||||
}
|
||||
|
||||
// probeCollectionResolution issues the one GET /server/capabilities probe and
|
||||
// classifies the outcome. definitive is true only when the server gave a clear
|
||||
// answer — HTTP 200 (resolves = the advertised flag) or HTTP 404 (resolves =
|
||||
// false: a build with no capabilities endpoint has no resolver). A transport
|
||||
// error, a 200 whose body will not decode, or any other status (e.g. a 5xx) is
|
||||
// NOT definitive.
|
||||
func (c *Client) probeCollectionResolution() (resolves, definitive bool) {
|
||||
req, err := c.newRequest("GET", "/server/capabilities", nil)
|
||||
if err != nil {
|
||||
return false, false
|
||||
}
|
||||
resp, err := c.httpClient.Do(req)
|
||||
if err != nil {
|
||||
return false, false
|
||||
}
|
||||
defer resp.Body.Close()
|
||||
switch resp.StatusCode {
|
||||
case http.StatusOK:
|
||||
var caps struct {
|
||||
CollectionResolution bool `json:"collection_resolution"`
|
||||
}
|
||||
if err := json.NewDecoder(resp.Body).Decode(&caps); err != nil {
|
||||
return false, false
|
||||
}
|
||||
return caps.CollectionResolution, true
|
||||
case http.StatusNotFound:
|
||||
return false, true
|
||||
default:
|
||||
return false, false
|
||||
}
|
||||
}
|
||||
|
||||
func (c *Client) GetItem(wsSlug, itemSlug string) (*models.Item, error) {
|
||||
var result models.Item
|
||||
if err := c.get("/workspaces/"+wsSlug+"/items/"+itemSlug, &result); err != nil {
|
||||
|
||||
@@ -0,0 +1,152 @@
|
||||
package cli
|
||||
|
||||
import (
|
||||
"errors"
|
||||
"fmt"
|
||||
|
||||
"github.com/PerpetualSoftware/pad/internal/collections"
|
||||
)
|
||||
|
||||
// withCollectionAliasFallback runs op against the RAW collection slug the user
|
||||
// typed, and — only if that fails specifically because the collection was not
|
||||
// found — retries once with the legacy singular→plural alias
|
||||
// (collections.NormalizeSlug). The result of whichever attempt succeeds is
|
||||
// returned; if both fail, the RAW attempt's error is returned.
|
||||
//
|
||||
// Why send raw first (BUG-2630). The old client behaviour normalized the slug
|
||||
// BEFORE the request, so a workspace whose collection slug IS one of the seven
|
||||
// hardcoded singulars ("plan", "task", …) had the user's exact name rewritten
|
||||
// away and their write landed in a DIFFERENT collection, silently, with a
|
||||
// success message naming the wrong one. Sending raw lets the server's
|
||||
// exact-match-first resolution (BUG-2578) win, so an exact collection name can
|
||||
// never be shadowed by its own alias.
|
||||
//
|
||||
// Why keep the alias at all. A server that predates the server-side resolver
|
||||
// (BUG-2578) matches collections by exact slug only, so a raw "task" would 404
|
||||
// there for a workspace whose collection is "tasks". The retry preserves
|
||||
// today's shorthand UX against those older servers, at the cost of one extra
|
||||
// round trip on a path that was already going to fail.
|
||||
//
|
||||
// Why the retry is keyed on collection-not-found specifically (see
|
||||
// isCollectionNotFound). If it fired on ANY failure, a request aimed at a
|
||||
// collection that really exists ("plan") but failed for another reason (a
|
||||
// validation error, an auth hiccup) would be retried against the alias
|
||||
// ("plans") and could silently succeed there — recreating BUG-2630 in a new
|
||||
// costume. Keying on the not-found error means a collection that exists is
|
||||
// never retried away from.
|
||||
//
|
||||
// REMOVAL CONDITION: once the minimum supported server is guaranteed to carry
|
||||
// the resolver (BUG-2578), the raw slug always resolves on the first attempt
|
||||
// and this fallback is dead weight — drop it and call op(rawSlug) directly.
|
||||
// That is the Option 1 cleanup tracked on BUG-2630's trail, gated on a
|
||||
// min-server-version floor that includes the resolver.
|
||||
//
|
||||
// notFoundIsAuthoritative reports whether a collection-not-found should be
|
||||
// TRUSTED — when it returns true the alias retry is skipped, because the server
|
||||
// resolves slugs itself (exact-match-first + alias fallback + archived/hidden
|
||||
// refusal) and has therefore already ruled the slug absent. It returns false
|
||||
// only for a server definitively without the resolver, where the legacy retry
|
||||
// is the right non-regressive fallback. Pass nil to FORCE the always-retry
|
||||
// behaviour — the right choice for an endpoint that does NOT resolve slugs
|
||||
// server-side (the CLI schema lookup hits exact-match-only GetCollection, so its
|
||||
// not-found is never authoritative about an alias). See
|
||||
// Client.CollectionNotFoundIsAuthoritative.
|
||||
//
|
||||
// Exported so the cmd/pad item commands can funnel their create / list / move
|
||||
// calls through this ONE implementation instead of each re-deriving the
|
||||
// send-raw-then-retry dance (BUG-2630 lead ruling: one shared helper, not
|
||||
// copied at the call sites).
|
||||
func WithCollectionAliasFallback[T any](rawSlug string, notFoundIsAuthoritative func() bool, op func(slug string) (T, error)) (T, error) {
|
||||
result, err := op(rawSlug)
|
||||
if err == nil {
|
||||
return result, nil
|
||||
}
|
||||
// The failure was not a missing collection (auth, validation, a 5xx): return
|
||||
// it untouched. Retrying such an error against the alias could silently
|
||||
// succeed on a DIFFERENT collection that does exist — BUG-2630 in a new
|
||||
// costume — so a collection that exists is never retried away from.
|
||||
if !isCollectionNotFound(err) {
|
||||
return result, err
|
||||
}
|
||||
normalized := collections.NormalizeSlug(rawSlug)
|
||||
if normalized == rawSlug {
|
||||
// No alias to try (the input is not one of the aliased singulars): the
|
||||
// slug genuinely names no collection. Name it in the error the user
|
||||
// sees, since it is their own word.
|
||||
return result, wrapCollectionNotFound(err, rawSlug)
|
||||
}
|
||||
// When the server's not-found is authoritative — it resolves collections
|
||||
// itself (BUG-2578/2630), so it already tried the alias and enforced
|
||||
// exact-match-first + the archived/hidden refusal — retrying the alias would
|
||||
// only defeat that protection (BUG-2630 #1). Only fall back to the
|
||||
// client-side retry for an OLDER server that never had the protection.
|
||||
if notFoundIsAuthoritative != nil && notFoundIsAuthoritative() {
|
||||
return result, wrapCollectionNotFound(err, rawSlug)
|
||||
}
|
||||
result2, err2 := op(normalized)
|
||||
if err2 != nil {
|
||||
if isCollectionNotFound(err2) {
|
||||
// Neither the raw slug nor its alias names a collection. Surface an
|
||||
// error naming the RAW slug the user typed, not the alias the
|
||||
// client tried on their behalf — their own words are the ones they
|
||||
// can act on.
|
||||
return result, wrapCollectionNotFound(err, rawSlug)
|
||||
}
|
||||
// The alias DOES name a real collection, but the operation failed there
|
||||
// for a substantive reason (plan limit, open children, validation,
|
||||
// forbidden). That error is about a real collection and is far more
|
||||
// useful than a misleading "collection not found" — surface it verbatim.
|
||||
return result2, err2
|
||||
}
|
||||
return result2, nil
|
||||
}
|
||||
|
||||
// wrapCollectionNotFound rewrites the server's context-free "Collection not
|
||||
// found" into one that names the slug the user actually typed, so a failed
|
||||
// `pad item create widget …` reads `collection "widget" not found` instead of a
|
||||
// bare "Collection not found". It returns a fresh *APIError with the same Code
|
||||
// (so downstream type/Code inspection — plan-limit, open-children — keeps
|
||||
// working) and an enriched Message. Non-collection errors pass through
|
||||
// untouched. Mirrors wrapItemNotFound's shape.
|
||||
func wrapCollectionNotFound(err error, rawSlug string) error {
|
||||
var apiErr *APIError
|
||||
if errors.As(err, &apiErr) && isCollectionNotFound(err) {
|
||||
return &APIError{
|
||||
Code: apiErr.Code,
|
||||
Message: fmt.Sprintf("collection %q not found", rawSlug),
|
||||
Details: apiErr.Details,
|
||||
}
|
||||
}
|
||||
return err
|
||||
}
|
||||
|
||||
// isCollectionNotFound reports whether err is the server's specific
|
||||
// "collection not found" response for an item create / list / move request. It
|
||||
// is deliberately narrow: any other failure (auth, validation, a 5xx, an
|
||||
// item-not-found) must NOT trigger the alias retry, or a request aimed at a
|
||||
// collection that really exists could be silently rerouted to a different one
|
||||
// (BUG-2630).
|
||||
//
|
||||
// The wire contract it matches is stable across the BUG-2578 server change —
|
||||
// both codes predate it, so the predicate works against old and new servers:
|
||||
// - create / list: HTTP 404, code "not_found", message "Collection not found"
|
||||
// - move: HTTP 400, code "invalid_collection"
|
||||
//
|
||||
// The message check on the "not_found" branch distinguishes a missing
|
||||
// COLLECTION from a missing ITEM, which shares the "not_found" code on other
|
||||
// endpoints — though the create/list endpoints this helper wraps only ever
|
||||
// emit "not_found" for the collection, the message guard keeps the predicate
|
||||
// honest if it is ever reused on a broader path.
|
||||
func isCollectionNotFound(err error) bool {
|
||||
var apiErr *APIError
|
||||
if !errors.As(err, &apiErr) {
|
||||
return false
|
||||
}
|
||||
switch apiErr.Code {
|
||||
case "invalid_collection":
|
||||
return true
|
||||
case "not_found":
|
||||
return apiErr.Message == "Collection not found"
|
||||
}
|
||||
return false
|
||||
}
|
||||
@@ -0,0 +1,224 @@
|
||||
package cli
|
||||
|
||||
import (
|
||||
"errors"
|
||||
"fmt"
|
||||
"testing"
|
||||
)
|
||||
|
||||
// notFoundCollErr is the create/list wire shape for a missing collection.
|
||||
func notFoundCollErr() error {
|
||||
return &APIError{Code: "not_found", Message: "Collection not found"}
|
||||
}
|
||||
|
||||
// invalidCollErr is the move wire shape for a missing target collection.
|
||||
func invalidCollErr() error {
|
||||
return &APIError{Code: "invalid_collection", Message: "Target collection not found"}
|
||||
}
|
||||
|
||||
func TestWithCollectionAliasFallback_RawSucceeds_NoRetry(t *testing.T) {
|
||||
// The shadow case (BUG-2630): the user typed a slug that IS a real
|
||||
// collection ("plan") whose alias ("plans") also exists. The raw attempt
|
||||
// succeeds, so the fallback must NOT retry — retrying would route the user
|
||||
// to the aliased collection they did not name.
|
||||
var calls []string
|
||||
got, err := WithCollectionAliasFallback("plan", nil, func(slug string) (string, error) {
|
||||
calls = append(calls, slug)
|
||||
return "landed:" + slug, nil
|
||||
})
|
||||
if err != nil {
|
||||
t.Fatalf("unexpected error: %v", err)
|
||||
}
|
||||
if got != "landed:plan" {
|
||||
t.Fatalf("got %q, want landed:plan", got)
|
||||
}
|
||||
if len(calls) != 1 || calls[0] != "plan" {
|
||||
t.Fatalf("expected exactly one call with the raw slug, got %v", calls)
|
||||
}
|
||||
}
|
||||
|
||||
func TestWithCollectionAliasFallback_CollectionNotFound_RetriesAlias(t *testing.T) {
|
||||
// Old-server compat: raw "task" 404s because the collection is "tasks" and
|
||||
// the server has no resolver. The fallback retries with the alias.
|
||||
var calls []string
|
||||
got, err := WithCollectionAliasFallback("task", nil, func(slug string) (string, error) {
|
||||
calls = append(calls, slug)
|
||||
if slug == "task" {
|
||||
return "", notFoundCollErr()
|
||||
}
|
||||
return "landed:" + slug, nil
|
||||
})
|
||||
if err != nil {
|
||||
t.Fatalf("unexpected error: %v", err)
|
||||
}
|
||||
if got != "landed:tasks" {
|
||||
t.Fatalf("got %q, want landed:tasks", got)
|
||||
}
|
||||
if len(calls) != 2 || calls[0] != "task" || calls[1] != "tasks" {
|
||||
t.Fatalf("expected raw then alias, got %v", calls)
|
||||
}
|
||||
}
|
||||
|
||||
func TestWithCollectionAliasFallback_InvalidCollectionCode_RetriesAlias(t *testing.T) {
|
||||
// The move path reports a missing target collection as invalid_collection.
|
||||
var calls []string
|
||||
_, err := WithCollectionAliasFallback("task", nil, func(slug string) (string, error) {
|
||||
calls = append(calls, slug)
|
||||
if slug == "task" {
|
||||
return "", invalidCollErr()
|
||||
}
|
||||
return "ok", nil
|
||||
})
|
||||
if err != nil {
|
||||
t.Fatalf("unexpected error: %v", err)
|
||||
}
|
||||
if len(calls) != 2 {
|
||||
t.Fatalf("expected a retry on invalid_collection, got %v", calls)
|
||||
}
|
||||
}
|
||||
|
||||
func TestWithCollectionAliasFallback_NonAliasedInput_NoRetry(t *testing.T) {
|
||||
// A slug that NormalizeSlug leaves unchanged ("widgets") has no alias to
|
||||
// try, so a not-found must return immediately without a second call.
|
||||
var calls int
|
||||
_, err := WithCollectionAliasFallback("widgets", nil, func(slug string) (string, error) {
|
||||
calls++
|
||||
return "", notFoundCollErr()
|
||||
})
|
||||
if calls != 1 {
|
||||
t.Fatalf("expected exactly one call (no alias to retry), got %d", calls)
|
||||
}
|
||||
// The error the user sees names the slug they typed (constraint 2), with
|
||||
// the collection-not-found Code preserved for downstream inspection.
|
||||
var apiErr *APIError
|
||||
if !errors.As(err, &apiErr) || apiErr.Code != "not_found" {
|
||||
t.Fatalf("expected a *APIError with Code=not_found, got %v", err)
|
||||
}
|
||||
if apiErr.Message != `collection "widgets" not found` {
|
||||
t.Fatalf("error should name the raw slug, got %q", apiErr.Message)
|
||||
}
|
||||
}
|
||||
|
||||
func TestWithCollectionAliasFallback_NonCollectionError_NoRetry(t *testing.T) {
|
||||
// Constraint 1 (lead): a request aimed at a collection that really exists
|
||||
// ("plan") but that fails for a DIFFERENT reason (here a validation error)
|
||||
// must NOT be retried against the alias ("plans"). Retrying could silently
|
||||
// succeed against the wrong collection — BUG-2630 in a new costume.
|
||||
var calls []string
|
||||
sentinel := &APIError{Code: "bad_request", Message: "Title is required"}
|
||||
_, err := WithCollectionAliasFallback("plan", nil, func(slug string) (string, error) {
|
||||
calls = append(calls, slug)
|
||||
return "", sentinel
|
||||
})
|
||||
if !errors.Is(err, sentinel) {
|
||||
t.Fatalf("expected the raw sentinel error back, got %v", err)
|
||||
}
|
||||
if len(calls) != 1 || calls[0] != "plan" {
|
||||
t.Fatalf("a non-collection error must not trigger the alias retry, got %v", calls)
|
||||
}
|
||||
}
|
||||
|
||||
func TestWithCollectionAliasFallback_BothFail_SurfacesRawError(t *testing.T) {
|
||||
// Constraint 2 (lead): when both raw and alias fail, the error the user
|
||||
// sees must name the slug they typed ("task"), not the alias tried on their
|
||||
// behalf ("tasks").
|
||||
got := "sentinel-unset"
|
||||
_, err := WithCollectionAliasFallback("task", nil, func(slug string) (string, error) {
|
||||
got = slug
|
||||
if slug == "task" {
|
||||
return "", notFoundCollErr()
|
||||
}
|
||||
return "", &APIError{Code: "not_found", Message: "Collection not found"}
|
||||
})
|
||||
if got != "tasks" {
|
||||
t.Fatalf("expected the alias to have been tried, last slug was %q", got)
|
||||
}
|
||||
var apiErr *APIError
|
||||
if !errors.As(err, &apiErr) {
|
||||
t.Fatalf("expected a *APIError, got %v", err)
|
||||
}
|
||||
if apiErr.Message != `collection "task" not found` {
|
||||
t.Fatalf("double-fail error must name the RAW slug, got %q", apiErr.Message)
|
||||
}
|
||||
}
|
||||
|
||||
func TestWithCollectionAliasFallback_AliasSubstantiveError_Surfaced(t *testing.T) {
|
||||
// Codex P2: when the raw slug misses but the alias names a REAL collection
|
||||
// that fails for a substantive reason (here a plan limit), the user must
|
||||
// see THAT error, not a misleading "collection not found" — the alias
|
||||
// collection exists, so "not found" would be a lie.
|
||||
planLimit := &APIError{Code: "plan_limit_exceeded", Message: "item limit reached"}
|
||||
_, err := WithCollectionAliasFallback("task", nil, func(slug string) (string, error) {
|
||||
if slug == "task" {
|
||||
return "", notFoundCollErr()
|
||||
}
|
||||
return "", planLimit // the "tasks" collection exists but is over limit
|
||||
})
|
||||
if !errors.Is(err, planLimit) {
|
||||
t.Fatalf("expected the substantive alias error surfaced, got %v", err)
|
||||
}
|
||||
}
|
||||
|
||||
func TestWithCollectionAliasFallback_ServerResolves_SuppressesRetry(t *testing.T) {
|
||||
// BUG-2630 #1: a server that resolves collections itself has already tried
|
||||
// the alias AND enforced exact-match + the archived/hidden refusal, so its
|
||||
// collection-not-found is AUTHORITATIVE. The client must NOT retry the alias
|
||||
// (which would defeat that protection, e.g. redirect an archived `plan` into
|
||||
// a live `plans`).
|
||||
var calls []string
|
||||
_, err := WithCollectionAliasFallback("plan", func() bool { return true }, func(slug string) (string, error) {
|
||||
calls = append(calls, slug)
|
||||
return "", notFoundCollErr()
|
||||
})
|
||||
if len(calls) != 1 || calls[0] != "plan" {
|
||||
t.Fatalf("a resolving server's not-found must not be retried, got %v", calls)
|
||||
}
|
||||
var apiErr *APIError
|
||||
if !errors.As(err, &apiErr) || apiErr.Message != `collection "plan" not found` {
|
||||
t.Fatalf("expected raw-named not-found, got %v", err)
|
||||
}
|
||||
}
|
||||
|
||||
func TestWithCollectionAliasFallback_ServerLacksResolution_Retries(t *testing.T) {
|
||||
// The other branch: an OLD server that does not advertise resolution never
|
||||
// had the archived-claims protection, so the legacy alias retry runs and is
|
||||
// non-regressive there.
|
||||
var calls []string
|
||||
got, err := WithCollectionAliasFallback("plan", func() bool { return false }, func(slug string) (string, error) {
|
||||
calls = append(calls, slug)
|
||||
if slug == "plan" {
|
||||
return "", notFoundCollErr()
|
||||
}
|
||||
return "landed:" + slug, nil
|
||||
})
|
||||
if err != nil {
|
||||
t.Fatalf("unexpected error: %v", err)
|
||||
}
|
||||
if got != "landed:plans" || len(calls) != 2 {
|
||||
t.Fatalf("expected retry into plans, got %q calls=%v", got, calls)
|
||||
}
|
||||
}
|
||||
|
||||
func TestIsCollectionNotFound(t *testing.T) {
|
||||
cases := []struct {
|
||||
name string
|
||||
err error
|
||||
want bool
|
||||
}{
|
||||
{"create/list not_found", notFoundCollErr(), true},
|
||||
{"move invalid_collection", invalidCollErr(), true},
|
||||
{"item not_found (different message)", &APIError{Code: "not_found", Message: "item TASK-9 not found"}, false},
|
||||
{"validation error", &APIError{Code: "bad_request", Message: "Title is required"}, false},
|
||||
{"plan limit", &APIError{Code: "plan_limit_exceeded", Message: "limit"}, false},
|
||||
{"wrapped not_found", fmt.Errorf("ctx: %w", notFoundCollErr()), true},
|
||||
{"non-APIError", errors.New("network down"), false},
|
||||
{"nil", nil, false},
|
||||
}
|
||||
for _, tc := range cases {
|
||||
t.Run(tc.name, func(t *testing.T) {
|
||||
if got := isCollectionNotFound(tc.err); got != tc.want {
|
||||
t.Fatalf("isCollectionNotFound(%v) = %v, want %v", tc.err, got, tc.want)
|
||||
}
|
||||
})
|
||||
}
|
||||
}
|
||||
@@ -15,7 +15,6 @@ import (
|
||||
"github.com/go-chi/chi/v5"
|
||||
"github.com/mark3labs/mcp-go/mcp"
|
||||
|
||||
"github.com/PerpetualSoftware/pad/internal/collections"
|
||||
"github.com/PerpetualSoftware/pad/internal/models"
|
||||
"github.com/PerpetualSoftware/pad/internal/server"
|
||||
)
|
||||
@@ -860,15 +859,13 @@ func mapItemCreate(input map[string]any) (method, path string, body []byte, err
|
||||
return "", "", nil, fmt.Errorf("encode body: %w", err)
|
||||
}
|
||||
|
||||
// Normalize singular/shorthand forms ("task" → "tasks", "doc" →
|
||||
// "docs", etc.) the same way the CLI does. Without this, an MCP
|
||||
// caller that mirrors a documented CLI command shape like
|
||||
// `item.create(collection: "task", ...)` would 404 against the
|
||||
// REST handler even though the same call works through
|
||||
// ExecDispatcher (which goes through normalizeCollectionSlug in
|
||||
// cmd/pad/main.go).
|
||||
collection = collections.NormalizeSlug(collection)
|
||||
|
||||
// Send the collection slug verbatim. This dispatcher runs in-process
|
||||
// against the same binary, whose create handler resolves shorthand
|
||||
// server-side with exact-match-first (BUG-2578), so the raw slug keeps
|
||||
// `item.create(collection: "task", ...)` working AND stops the old
|
||||
// client-side alias from shadowing a real collection whose slug IS a
|
||||
// singular like "task" (BUG-2630). The ExecDispatcher path (local stdio)
|
||||
// gets the same behaviour through the CLI's alias-fallback helper.
|
||||
urlPath := fmt.Sprintf("/api/v1/workspaces/%s/collections/%s/items",
|
||||
url.PathEscape(workspace), url.PathEscape(collection))
|
||||
return http.MethodPost, urlPath, body, nil
|
||||
|
||||
@@ -101,6 +101,79 @@ func TestHTTPItemCreate_AcceptsSingularOfANonDefaultCollection(t *testing.T) {
|
||||
}
|
||||
}
|
||||
|
||||
// newShadowFixture builds a workspace holding BOTH a user collection whose slug
|
||||
// IS a hardcoded singular ("plan") and its plural ("plans"), so the BUG-2630
|
||||
// shadow — the client alias rewriting "plan" to "plans" before the server sees
|
||||
// it — has a real collision to expose.
|
||||
func newShadowFixture(t *testing.T) *HTTPHandlerDispatcher {
|
||||
t.Helper()
|
||||
s := storetest.NewSQLite(t)
|
||||
srv := server.New(s)
|
||||
t.Cleanup(srv.Stop)
|
||||
|
||||
owner, err := s.CreateUser(models.UserCreate{
|
||||
Email: "shadow-owner@example.com", Name: "Owner", Password: "correct-horse-battery-staple",
|
||||
})
|
||||
if err != nil {
|
||||
t.Fatalf("CreateUser: %v", err)
|
||||
}
|
||||
ws, err := s.CreateWorkspace(models.WorkspaceCreate{Name: "Shadow WS", Slug: "shadow-ws", OwnerID: owner.ID})
|
||||
if err != nil {
|
||||
t.Fatalf("CreateWorkspace: %v", err)
|
||||
}
|
||||
if err := s.AddWorkspaceMember(ws.ID, owner.ID, "owner"); err != nil {
|
||||
t.Fatalf("AddWorkspaceMember: %v", err)
|
||||
}
|
||||
schema := `{"fields":[{"key":"status","type":"select","options":["open","done"],"default":"open"}]}`
|
||||
for _, c := range []models.CollectionCreate{
|
||||
{Name: "Plans", Slug: "plans", Prefix: "PLANS", Schema: schema},
|
||||
{Name: "Plan", Slug: "plan", Prefix: "PLAN", Schema: schema},
|
||||
} {
|
||||
if _, err := s.CreateCollection(ws.ID, c); err != nil {
|
||||
t.Fatalf("CreateCollection %s: %v", c.Slug, err)
|
||||
}
|
||||
}
|
||||
return &HTTPHandlerDispatcher{Handler: srv, UserResolver: func(context.Context) *models.User { return owner }}
|
||||
}
|
||||
|
||||
// BUG-2630 over the MCP dispatcher: an exact collection name that happens to be
|
||||
// a hardcoded singular must NOT be rewritten into its plural. The item has to
|
||||
// land in — and be listable from — the collection the caller actually named.
|
||||
func TestHTTPItemCreate_ExactSingularNotShadowedByAlias(t *testing.T) {
|
||||
d := newShadowFixture(t)
|
||||
|
||||
createCtx := WithDispatchInput(context.Background(), map[string]any{
|
||||
"workspace": "shadow-ws", "collection": "plan", "title": "belongs in plan",
|
||||
})
|
||||
res, err := d.Dispatch(createCtx, []string{"item", "create"}, nil)
|
||||
if err != nil {
|
||||
t.Fatalf("Dispatch(item create): %v", err)
|
||||
}
|
||||
if res.IsError {
|
||||
t.Fatalf("create into `plan` over MCP failed: %s", textOf(res))
|
||||
}
|
||||
|
||||
// Present in `plan` (the named collection)...
|
||||
planList := WithDispatchInput(context.Background(), map[string]any{"workspace": "shadow-ws", "collection": "plan"})
|
||||
planRes, err := d.Dispatch(planList, []string{"item", "list"}, nil)
|
||||
if err != nil {
|
||||
t.Fatalf("Dispatch(list plan): %v", err)
|
||||
}
|
||||
if !strings.Contains(textOf(planRes), "belongs in plan") {
|
||||
t.Errorf("item did not land in `plan`: %s", textOf(planRes))
|
||||
}
|
||||
|
||||
// ...and ABSENT from `plans` (the alias that used to shadow it).
|
||||
plansList := WithDispatchInput(context.Background(), map[string]any{"workspace": "shadow-ws", "collection": "plans"})
|
||||
plansRes, err := d.Dispatch(plansList, []string{"item", "list"}, nil)
|
||||
if err != nil {
|
||||
t.Fatalf("Dispatch(list plans): %v", err)
|
||||
}
|
||||
if strings.Contains(textOf(plansRes), "belongs in plan") {
|
||||
t.Errorf("item was shadowed into `plans` — BUG-2630: %s", textOf(plansRes))
|
||||
}
|
||||
}
|
||||
|
||||
// A collection that does not exist under any spelling must still fail, so the
|
||||
// fallback cannot be mistaken for "any name works".
|
||||
func TestHTTPItemCreate_UnknownCollectionStillFails(t *testing.T) {
|
||||
|
||||
@@ -28,9 +28,11 @@ import (
|
||||
// as standalone RouteMapper functions instead.
|
||||
//
|
||||
// All input keys are MCP property names (snake_case per TASK-964).
|
||||
// `collection` and `target_collection` placeholders are normalized
|
||||
// via collections.NormalizeSlug so callers can pass shorthand
|
||||
// ("task" → "tasks") without 404s.
|
||||
// Collection slugs are sent verbatim: this dispatcher runs in-process
|
||||
// against the same binary, whose item handlers resolve shorthand
|
||||
// server-side (exact-match-first, BUG-2578), so the raw slug both keeps
|
||||
// shorthand working and avoids the client-side alias shadowing a real
|
||||
// collection (BUG-2630).
|
||||
type routeSpec struct {
|
||||
// method is the HTTP method (GET / POST / PATCH / DELETE).
|
||||
method string
|
||||
@@ -85,9 +87,15 @@ func (s routeSpec) toRouteMapper() RouteMapper {
|
||||
// reply names the missing input rather than the agent receiving a
|
||||
// confusing 404 from the handler tree).
|
||||
//
|
||||
// The placeholders "collection" / "target_collection" are normalized
|
||||
// via collections.NormalizeSlug so shorthand forms like "task" work
|
||||
// the same way they do through the CLI.
|
||||
// Placeholders are substituted verbatim (path-escaped). It does NOT
|
||||
// normalize a collection slug: this dispatcher runs in-process against
|
||||
// the same binary, whose item handlers resolve shorthand server-side
|
||||
// with exact-match-first (BUG-2578), so sending the raw slug both keeps
|
||||
// shorthand working AND stops an alias from shadowing a real collection
|
||||
// of the same singular name (BUG-2630). No current routeSpec even uses a
|
||||
// {collection}/{target_collection} path placeholder — the standalone
|
||||
// item mappers build those paths directly — so this is also dead-code
|
||||
// removal in the area BUG-2630 fixed.
|
||||
func expandPath(template string, input map[string]any) (string, error) {
|
||||
var out strings.Builder
|
||||
out.Grow(len(template))
|
||||
@@ -113,9 +121,6 @@ func expandPath(template string, input map[string]any) (string, error) {
|
||||
if s == "" {
|
||||
return "", fmt.Errorf("input %q must be non-empty for path placeholder", name)
|
||||
}
|
||||
if name == "collection" || name == "target_collection" {
|
||||
s = collections.NormalizeSlug(s)
|
||||
}
|
||||
out.WriteString(url.PathEscape(s))
|
||||
i += end + 1
|
||||
}
|
||||
@@ -1197,8 +1202,11 @@ func mapItemList(input map[string]any) (string, string, []byte, error) {
|
||||
// "unknown input keys are ignored" behaviour.
|
||||
pathBase := "/api/v1/workspaces/" + url.PathEscape(workspace) + "/items"
|
||||
if coll, _ := input["collection"].(string); coll != "" {
|
||||
// Raw slug: the in-process list handler resolves shorthand
|
||||
// server-side (BUG-2578) so an exact collection name is never
|
||||
// shadowed by its alias (BUG-2630).
|
||||
pathBase = "/api/v1/workspaces/" + url.PathEscape(workspace) +
|
||||
"/collections/" + url.PathEscape(collections.NormalizeSlug(coll)) + "/items"
|
||||
"/collections/" + url.PathEscape(coll) + "/items"
|
||||
}
|
||||
|
||||
values := url.Values{}
|
||||
@@ -1496,7 +1504,10 @@ func mapItemMove(input map[string]any) (string, string, []byte, error) {
|
||||
}
|
||||
|
||||
payload := map[string]any{
|
||||
"target_collection": collections.NormalizeSlug(target),
|
||||
// Raw slug: the in-process move handler resolves shorthand
|
||||
// server-side (BUG-2578) so an exact collection name is never
|
||||
// shadowed by its alias (BUG-2630).
|
||||
"target_collection": target,
|
||||
"actor": "user",
|
||||
"source": "cli",
|
||||
}
|
||||
|
||||
@@ -66,18 +66,12 @@ func TestExpandPath_BasicSubstitution(t *testing.T) {
|
||||
}
|
||||
}
|
||||
|
||||
func TestExpandPath_NormalizesCollection(t *testing.T) {
|
||||
got, err := expandPath(
|
||||
"/api/v1/workspaces/{workspace}/collections/{collection}/items",
|
||||
map[string]any{"workspace": "docapp", "collection": "task"},
|
||||
)
|
||||
if err != nil {
|
||||
t.Fatalf("expandPath: %v", err)
|
||||
}
|
||||
if !strings.Contains(got, "/collections/tasks/") {
|
||||
t.Errorf("expected `task` to normalize to `tasks`, got %q", got)
|
||||
}
|
||||
}
|
||||
// TestExpandPath_NormalizesCollection was removed with the collection-slug
|
||||
// normalization it guarded (BUG-2630). expandPath no longer rewrites a
|
||||
// `{collection}` placeholder — the in-process handlers resolve shorthand
|
||||
// server-side (BUG-2578), and rewriting client-side let an alias shadow a real
|
||||
// collection of the same singular name. No routeSpec uses that placeholder
|
||||
// anyway; the item mappers build collection paths directly.
|
||||
|
||||
func TestExpandPath_PathEscapesSpecials(t *testing.T) {
|
||||
got, err := expandPath(
|
||||
@@ -302,15 +296,18 @@ func TestRoute_ItemList_ExplicitStatusOverridesDefault(t *testing.T) {
|
||||
|
||||
func TestRoute_ItemList_CollectionScopedPath(t *testing.T) {
|
||||
_, p, _, err := mapItemList(map[string]any{
|
||||
"workspace": "docapp", "collection": "task", // shorthand
|
||||
"workspace": "docapp", "collection": "task", // singular
|
||||
})
|
||||
if err != nil {
|
||||
t.Fatalf("err: %v", err)
|
||||
}
|
||||
// Path may carry the non_terminal=true query string from the
|
||||
// no-explicit-status branch — we only assert the path prefix here.
|
||||
if !strings.HasPrefix(p, "/api/v1/workspaces/docapp/collections/tasks/items") {
|
||||
t.Errorf("expected collection-scoped + normalized path, got %q", p)
|
||||
// The slug is sent VERBATIM (BUG-2630): the in-process list handler
|
||||
// resolves shorthand server-side (BUG-2578) and exact-match-first stops an
|
||||
// alias from shadowing a real `task` collection. Path may carry the
|
||||
// non_terminal=true query from the no-explicit-status branch — assert the
|
||||
// prefix only.
|
||||
if !strings.HasPrefix(p, "/api/v1/workspaces/docapp/collections/task/items") {
|
||||
t.Errorf("expected the raw slug in the path, got %q", p)
|
||||
}
|
||||
}
|
||||
|
||||
@@ -447,7 +444,7 @@ func TestRoute_ItemMove(t *testing.T) {
|
||||
m, p, body, err := routeTable["item move"](map[string]any{
|
||||
"workspace": "docapp",
|
||||
"ref": "BUG-3",
|
||||
"target_collection": "task", // shorthand
|
||||
"target_collection": "task", // singular, sent verbatim
|
||||
"field": []any{"priority=high"},
|
||||
})
|
||||
if err != nil {
|
||||
@@ -463,8 +460,10 @@ func TestRoute_ItemMove(t *testing.T) {
|
||||
if err := json.Unmarshal(body, &payload); err != nil {
|
||||
t.Fatalf("decode body: %v\n%s", err, body)
|
||||
}
|
||||
if payload["target_collection"] != "tasks" {
|
||||
t.Errorf("target_collection not normalized: %v", payload)
|
||||
// Verbatim, not aliased (BUG-2630): the in-process move handler resolves
|
||||
// shorthand server-side (BUG-2578) with exact-match-first.
|
||||
if payload["target_collection"] != "task" {
|
||||
t.Errorf("target_collection should be sent verbatim, got: %v", payload)
|
||||
}
|
||||
if payload["source"] != "cli" {
|
||||
t.Errorf("source not stamped: %v", payload)
|
||||
|
||||
@@ -491,23 +491,16 @@ func TestMapItemCreate_ExplicitFieldOverridesNamedFlag(t *testing.T) {
|
||||
}
|
||||
}
|
||||
|
||||
func TestMapItemCreate_NormalizesCollectionAliases(t *testing.T) {
|
||||
// MCP callers may mirror documented CLI shapes ("item create task X")
|
||||
// using singular/short collection names. The CLI normalizes via
|
||||
// collections.NormalizeSlug; the dispatcher does the same so the
|
||||
// HTTP transport doesn't 404 on a call that works through
|
||||
// ExecDispatcher. Locked into a test so the parity doesn't drift.
|
||||
cases := map[string]string{
|
||||
"task": "tasks",
|
||||
"idea": "ideas",
|
||||
"doc": "docs",
|
||||
"plan": "plans",
|
||||
"bug": "bugs",
|
||||
"convention": "conventions",
|
||||
"tasks": "tasks", // already-canonical: no change
|
||||
"my-custom": "my-custom",
|
||||
}
|
||||
for in, wantSlug := range cases {
|
||||
func TestMapItemCreate_SendsRawCollectionSlug(t *testing.T) {
|
||||
// The dispatcher sends the collection slug VERBATIM (BUG-2630). It runs
|
||||
// in-process against the same binary, whose create handler resolves
|
||||
// shorthand server-side with exact-match-first (BUG-2578), so the raw slug
|
||||
// keeps `item.create(collection: "task")` working AND stops the old
|
||||
// client-side alias from shadowing a real collection whose slug IS a
|
||||
// singular like "task". A singular that used to be rewritten must now reach
|
||||
// the server unchanged so the server can decide.
|
||||
cases := []string{"task", "idea", "doc", "plan", "bug", "convention", "tasks", "my-custom"}
|
||||
for _, in := range cases {
|
||||
t.Run(in, func(t *testing.T) {
|
||||
_, path, _, err := mapItemCreate(map[string]any{
|
||||
"workspace": "ws", "collection": in, "title": "x",
|
||||
@@ -515,9 +508,9 @@ func TestMapItemCreate_NormalizesCollectionAliases(t *testing.T) {
|
||||
if err != nil {
|
||||
t.Fatalf("mapItemCreate: %v", err)
|
||||
}
|
||||
wantPath := "/api/v1/workspaces/ws/collections/" + wantSlug + "/items"
|
||||
wantPath := "/api/v1/workspaces/ws/collections/" + in + "/items"
|
||||
if path != wantPath {
|
||||
t.Errorf("path = %q, want %q", path, wantPath)
|
||||
t.Errorf("path = %q, want %q (slug must be sent verbatim, not aliased)", path, wantPath)
|
||||
}
|
||||
})
|
||||
}
|
||||
|
||||
@@ -1,8 +1,10 @@
|
||||
package server
|
||||
|
||||
import (
|
||||
"slices"
|
||||
"strings"
|
||||
|
||||
"github.com/PerpetualSoftware/pad/internal/collections"
|
||||
"github.com/PerpetualSoftware/pad/internal/models"
|
||||
)
|
||||
|
||||
@@ -83,12 +85,14 @@ func (s *Server) resolveItemCollectionSlug(workspaceID, input string) (*models.C
|
||||
// collectionSlugCandidates returns the alternative slugs to try when `input`
|
||||
// matched nothing, in priority order and never including `input` itself.
|
||||
//
|
||||
// Only ASCII-`s` pluralization is attempted. That is not a limitation to fix
|
||||
// by reaching for an inflector: collection slugs are generated by slugify and
|
||||
// the plural convention is the house style, so `s` covers the real cases,
|
||||
// while irregular-plural guessing ("person" -> "people") would start inventing
|
||||
// mappings a user never wrote. Anything an inflector would catch, an exact
|
||||
// slug still resolves.
|
||||
// ASCII-`s` pluralization is tried first, then the fixed legacy alias map
|
||||
// (t/i/p/d, phase/phases). Neither reaches for an inflector: collection slugs
|
||||
// are generated by slugify and the plural convention is the house style, so
|
||||
// `s` covers the structural cases, while irregular-plural guessing
|
||||
// ("person" -> "people") would start inventing mappings a user never wrote.
|
||||
// Anything an inflector would catch, an exact slug still resolves. The alias
|
||||
// map is a CLOSED set carried over from the retired client-side normalizer, not
|
||||
// an open-ended guesser.
|
||||
func collectionSlugCandidates(input string) []string {
|
||||
trimmed := strings.ToLower(strings.TrimSpace(input))
|
||||
if trimmed == "" {
|
||||
@@ -111,5 +115,19 @@ func collectionSlugCandidates(input string) []string {
|
||||
if s := strings.TrimSuffix(trimmed, "s"); s != trimmed && s != "" {
|
||||
out = append(out, s)
|
||||
}
|
||||
// The legacy SEMANTIC aliases the client-side map used to apply before
|
||||
// resolution moved to the server (BUG-2630): t/i/p/d -> tasks/ideas/plans/
|
||||
// docs and phase/phases -> plans. Folded in LAST so the server owns the full
|
||||
// alias vocabulary and every client can send the raw slug — including the
|
||||
// remote MCP transport, which no longer normalizes (BUG-2630 #2). It is only
|
||||
// ever a last resort: the exact-match and the archived-claims refusal in
|
||||
// resolveItemCollectionSlug run for `input` and every candidate BEFORE this
|
||||
// one is reached, so a real (or archived-claimed) collection of the input
|
||||
// name still wins, and this never shadows or redirects around it. Aliases
|
||||
// that duplicate a structural candidate above (e.g. plan -> plans) are
|
||||
// dropped so the resolve loop does not query the same slug twice.
|
||||
if alias := collections.NormalizeSlug(trimmed); alias != trimmed && !slices.Contains(out, alias) {
|
||||
out = append(out, alias)
|
||||
}
|
||||
return out
|
||||
}
|
||||
|
||||
@@ -156,6 +156,42 @@ func TestResolveItemCollectionSlug_ExactMatchAlwaysWins(t *testing.T) {
|
||||
}
|
||||
}
|
||||
|
||||
// BUG-2630 #2 (Codex round 1). The legacy semantic aliases the client-side map
|
||||
// used to apply (t/i/p/d, phase/phases -> plans) are now resolved SERVER-side,
|
||||
// so dropping client normalization — including on the remote MCP transport,
|
||||
// which cannot retry — loses nothing. `t` must reach `tasks`.
|
||||
func TestResolveItemCollectionSlug_LegacyAbbreviationResolves(t *testing.T) {
|
||||
srv := testServer(t)
|
||||
ws := createTestWorkspaceViaAPI(t, srv) // startup template ships `tasks`
|
||||
|
||||
if rr := createItemIn(t, srv, ws, "t", "abbreviated"); rr.Code != http.StatusCreated {
|
||||
t.Fatalf("create via alias `t` = %d: %s — the server must resolve the "+
|
||||
"legacy abbreviation now that clients send it raw", rr.Code, rr.Body)
|
||||
}
|
||||
if got := itemsIn(t, srv, ws, "tasks"); len(got) != 1 || got[0] != "abbreviated" {
|
||||
t.Errorf("items in `tasks` = %v, want [abbreviated]", got)
|
||||
}
|
||||
}
|
||||
|
||||
// The alias must never OUTRANK an exact match: a workspace with a real `t`
|
||||
// collection routes `t` to `t`, not to `tasks`. Same exact-match-first property
|
||||
// the ±s fallback already honors, asserted for the alias path too.
|
||||
func TestResolveItemCollectionSlug_ExactMatchBeatsAlias(t *testing.T) {
|
||||
srv := testServer(t)
|
||||
ws := createTestWorkspaceViaAPI(t, srv) // ships `tasks`
|
||||
makeCollection(t, srv, ws, "T", "t", "TT")
|
||||
|
||||
if rr := createItemIn(t, srv, ws, "t", "belongs in t"); rr.Code != http.StatusCreated {
|
||||
t.Fatalf("create in `t` = %d: %s", rr.Code, rr.Body)
|
||||
}
|
||||
if got := itemsIn(t, srv, ws, "t"); len(got) != 1 || got[0] != "belongs in t" {
|
||||
t.Errorf("items in `t` = %v, want [belongs in t] — the alias outranked an exact match", got)
|
||||
}
|
||||
if got := itemsIn(t, srv, ws, "tasks"); len(got) != 0 {
|
||||
t.Errorf("items in `tasks` = %v, want none — `t` was misrouted to its alias", got)
|
||||
}
|
||||
}
|
||||
|
||||
// The reverse direction: a workspace whose collection is genuinely singular
|
||||
// still answers to the plural a user might type out of habit.
|
||||
func TestResolveItemCollectionSlug_PluralOfASingularCollection(t *testing.T) {
|
||||
@@ -228,6 +264,15 @@ func TestCollectionSlugCandidates(t *testing.T) {
|
||||
// first would misfile the write.
|
||||
{in: "Spec", want: []string{"spec", "specs"}},
|
||||
{in: "Specs", want: []string{"specs", "specss", "spec"}},
|
||||
// Legacy semantic aliases (BUG-2630): the abbreviation lands LAST, after
|
||||
// the structural ±s candidates, so it is only ever a last resort.
|
||||
{in: "t", want: []string{"ts", "tasks"}},
|
||||
{in: "d", want: []string{"ds", "docs"}},
|
||||
{in: "phase", want: []string{"phases", "plans"}},
|
||||
{in: "phases", want: []string{"phasess", "phase", "plans"}},
|
||||
// An alias that duplicates a structural candidate (plan -> plans is both
|
||||
// the +s form AND the NormalizeSlug alias) is not emitted twice.
|
||||
{in: "plan", want: []string{"plans"}},
|
||||
} {
|
||||
got := collectionSlugCandidates(tc.in)
|
||||
if len(got) != len(tc.want) {
|
||||
|
||||
@@ -286,11 +286,18 @@ func TestServerCapabilities_Endpoint(t *testing.T) {
|
||||
}
|
||||
|
||||
var resp struct {
|
||||
Image attachments.Capabilities `json:"image"`
|
||||
Image attachments.Capabilities `json:"image"`
|
||||
CollectionResolution bool `json:"collection_resolution"`
|
||||
}
|
||||
if err := json.Unmarshal(w.Body.Bytes(), &resp); err != nil {
|
||||
t.Fatalf("decode response: %v", err)
|
||||
}
|
||||
// BUG-2630: the CLI reads this flag to decide whether a collection-not-found
|
||||
// is authoritative (skip the alias retry) or whether it is talking to an old
|
||||
// build (retry). This build has the resolver, so it must advertise true.
|
||||
if !resp.CollectionResolution {
|
||||
t.Error("collection_resolution = false; this build resolves collections server-side")
|
||||
}
|
||||
wantFormats := []string{"png", "jpeg", "gif", "bmp", "tiff"}
|
||||
if got, want := strings.Join(resp.Image.ImageFormats, ","), strings.Join(wantFormats, ","); got != want {
|
||||
t.Errorf("ImageFormats = %q, want %q", got, want)
|
||||
|
||||
@@ -14,6 +14,18 @@ import (
|
||||
// Static for the lifetime of the binary, so clients are free to cache.
|
||||
type serverCapabilities struct {
|
||||
Image attachments.Capabilities `json:"image"`
|
||||
|
||||
// CollectionResolution is true when this build resolves a collection slug
|
||||
// server-side with exact-match-first + the singular/alias fallback and the
|
||||
// archived-claims refusal (resolveItemCollectionSlug, BUG-2578/2630). The
|
||||
// CLI reads it to decide, on a collection-not-found, whether that answer is
|
||||
// AUTHORITATIVE (this build already tried every alias, so the slug is truly
|
||||
// absent/archived/hidden — do not retry an alias and defeat the protection)
|
||||
// or whether it is talking to an older build with no resolver and should
|
||||
// fall back to the legacy client-side alias retry. Always true here; its
|
||||
// ABSENCE (an old build that 404s this endpoint or omits the field) is the
|
||||
// signal to retry.
|
||||
CollectionResolution bool `json:"collection_resolution"`
|
||||
}
|
||||
|
||||
// handleServerCapabilities reports what this build can do to the editor.
|
||||
@@ -24,7 +36,7 @@ type serverCapabilities struct {
|
||||
// rather than 500-ing — that signals to the editor "uploads still work,
|
||||
// but disable transformation tools."
|
||||
func (s *Server) handleServerCapabilities(w http.ResponseWriter, r *http.Request) {
|
||||
resp := serverCapabilities{}
|
||||
resp := serverCapabilities{CollectionResolution: true}
|
||||
if s.imageProcessor != nil {
|
||||
resp.Image = s.imageProcessor.Capabilities()
|
||||
} else {
|
||||
|
||||
Reference in New Issue
Block a user