mirror of
https://github.com/PerpetualSoftware/pad.git
synced 2026-09-23 02:53:31 +00:00
xarmian
50a442d048
* fix(server): resolve collection slugs against the workspace's real collections (BUG-2578) `pad item create spec` failed with "Collection not found" in a workspace whose collections include `specs`, because the singular forms live in collections.NormalizeSlug — a hardcoded switch over the DEFAULT templates' names, called from the CLI and the MCP dispatcher, both CLIENT side and neither with any view of the workspace. So a template-defined or user-created collection got no shorthand, and the spec template's central object was the one thing with no way to abbreviate it while peripheral `idea` had one. Resolving on the SERVER is what makes this general: the workspace's collection list only exists here, so one resolver covers the CLI, the remote MCP transport, the web UI and any direct API consumer, instead of teaching each client the same trick. `spec` is not in the client map, so it already arrives intact; a test in internal/mcp pins that pass-through, since a future map entry for it would silently take the fix away from MCP agents. EXACT MATCH ALWAYS WINS, and that is the property the design turns on. The fallbacks fire only when the input names no collection at all, so the resolver can never redirect a request that already succeeded — which is what makes it safe to add underneath five existing call sites. It has its own test, with the mutation that inverts the order failing it. Deliberately NOT wired into store.GetCollectionBySlug. That has 23 call sites including authorization paths (authz_cross_workspace, handlers_grants, handlers_share_links), and fuzzy resolution inside a function used for permission checks is how a check and the action it guards come to disagree about which collection they mean. Scope is the five user-typed item operations: create, list, move, bulk move, cross-workspace copy. Internal derivations (artifact import's collectionSlugForKind) and the web-only progress endpoints keep exact matching. Two things worth noting for whoever reads this next: The list handler resolved the collection for its visibility gate and then filtered items by the RAW url parameter, so a singular returned 200 with an empty list — a resolve-then-pass-the-unresolved-value bug my own wiring introduced, caught by the test that asserts listing works, not by the one that asserts creating does. This does NOT fix the sibling defect the re-derivation turned up: the client map SHADOWS an exact match, so in a workspace holding both `plans` and a user-created `plan`, `pad item create plan` silently files into `plans`. Verified still reproducing after this change, because the rewrite happens before the server sees the slug. Filed as BUG-2630 with a live repro; the lead ruled option 2 (send raw, retry on collection-not-found) and it rides a later PR, since changing wire behaviour is a compatibility call rather than part of this fix. * fix(server): canonicalize the resolved slug downstream in bulk move and items-index (BUG-2578) Codex round 1, and both findings are the same defect class as the one my own list test caught: resolve the collection, then keep using the caller's raw input for everything downstream. Bulk move is the one that matters, and it was reachable only BECAUSE the resolver made `spec` succeed at all — so the inconsistency arrived with this change rather than predating it. req.Collection is compared against item.CollectionSlug to decide whether the op even IS a cross-collection move, written into activity metadata as to_collection, and used as the SSE scope the arrival event is addressed to. Left raw, a move into `specs` would log a to_collection of "spec" that no reader can look up, address the arrival event to a lane no client watches, and — for an item already in `specs` — compare unequal and categorise a same-collection no-op as a move. Canonicalized once up front rather than at each of the four use sites, so a fifth use cannot reintroduce it. items-index filtered by exact slug too, so `?collection=spec` returned an empty index rather than an error. The web client sends canonical slugs and is unaffected; this is for direct API consumers, and it keeps the same exact-match-wins property, so no existing query changes meaning. A slug that resolves to nothing is passed through untouched, preserving today's behaviour. Both are mutation-verified: removing the canonicalization fails the activity assertion with the literal to_collection "spec", and removing the index resolution returns the empty result set. * test: cover cross-workspace copy and drive the MCP claim end to end (BUG-2578) Codex round 2, two coverage gaps, both real. The cross-workspace copy call site was wired to the resolver and never exercised: every existing copy test passes an exact slug, so reverting that line would have gone unnoticed. Now covered through BOTH halves — preflight and the mutating copy — because they resolve the destination separately, and a preflight that accepts a name the copy then rejects is the worse of the two failures. Mutation-verified: reverting the call site fails it with "Destination collection not found". The MCP test was scoped to what the dispatcher BUILDS — that the slug is passed through rather than rewritten — and its comment said so, but a URL assertion is a claim about the dispatcher, not about what an agent receives. Since the bug's body makes a claim about MCP agents specifically, that claim now has a test that drives the real server and store over the transport: create in `spec`, then LIST by the same shorthand, because an agent that can create something it cannot then list is not fixed. Mutation-verified: removing the server fallback fails it with the exact user-visible error the bug reports. The pass-through test stays. It guards a different thing — that a future entry in the client-side alias map would silently take the server fix away from MCP by rewriting the slug before it arrives — and has its own control (adding `spec` to the map fails it). The copy fixture uses a permissive destination schema on purpose: the shared dstSchemaJSON has required fields the source item does not carry, and a validation rejection would mask the resolution result under test. * fix(server): case-fold before pluralizing, pin the list by ID, resolve the bulk target once (BUG-2578) Codex round 3, three findings, all correct. CANDIDATE ORDER (P1). Pluralization was tried before the case-folded form, so `Spec` resolved to `specs` in a workspace holding both `spec` and `specs`. That is the same misfiling the exact-match-wins rule exists to prevent, reached by a different route: `Spec` names `spec` more closely than it names that name's plural. Folded form now goes first. My own candidate test had the wrong order baked into its expectation, which is why it did not catch this — the new end-to-end case asserts where the write actually lands, and both fail on the old order. LIST PINNED BY ID (P1). Visibility was checked against coll.ID and the query then filtered on a SLUG. A slug can be freed by a rename or delete and taken by another collection in between, so the response could carry a different collection's items — possibly one the caller cannot see. The ID cannot be reassigned, and both filters are ANDed, so a concurrent rename now yields an empty list rather than someone else's rows. Note this predates the diff in kind: the handler filtered by the RAW slug before, with the same gap. BULK RESOLVES ONCE, AND NOW THAT IS TRUE (P2). The previous commit canonicalized the target up front and said it did so "rather than resolving it per-item further down" — but the per-item path went on calling the resolver for every row, so a 300-item batch with an unresolvable target could run ~1,200 lookups. The comment and the commit message both overstated the code. The resolved collection is now threaded through applyBulkOp into bulkMoveCollection, an unresolvable target fails the request up front instead of once per item, and the claim matches the implementation. That last one is the failure I keep meeting from different sides: the code was defensible and the sentence describing it was not true. Worth naming plainly rather than quietly fixing, because a reviewer reading that comment would have had no reason to check. * fix(server): revert the CollectionIDs pin — it was a visibility leak, not a scope filter (BUG-2578) Codex round 4. The P1 is a hole I opened one commit earlier, and it is the worst thing on this branch. To close a slug-reuse race I "pinned" the collection-item list by setting params.CollectionIDs to the resolved collection, and wrote a comment asserting the two filters were ANDed so a concurrent rename would fail safe. I did not read the query. CollectionIDs and ItemIDs are a PERMISSION PAIR and the store combines them with OR — "in a fully-granted collection, OR specifically granted". So pinning CollectionIDs while the item-grant branch of the same handler set ItemIDs rewrote the caller's grants into `collection_id IN (this) OR id IN (granted)`, handing a caller whose only claim on the collection is ONE item grant every item in it. Reverted. The race it was meant to fix is filed as BUG-2631, WITH the reason this fix is wrong, because setting CollectionIDs is the obvious move and the next person will reach for it too; the real fix needs a scoping parameter distinct from the permission pair. A regression test now covers the leak over both auth classes, and it fails with the ungranted sibling in the response body when the pin is reinstated. Every other test in that file uses an unrestricted owner, which is precisely why none of them noticed — the property was invisible to the whole fixture family I had been writing. Two round-4 P2s, both fixed: The bulk endpoint refused an unresolvable target with a 400 while an existing-but-hidden target failed per item inside a normal 200 envelope. That status difference is an existence oracle — a restricted caller can probe slugs and learn which collections they may not see exist. Unresolvable targets now take the same per-item path, which is also the pre-change behaviour, and a test asserts the two responses are indistinguishable. items-index discarded the resolver's error and continued with the raw alias, answering a database failure with a successful EMPTY index. It now surfaces the error. The lesson I am taking, since it is the second time today the same shape bit: I asserted a mechanism (AND semantics) in a comment without reading the code that implements it, and the comment made the change look considered. Last time that produced a wrong explanation on a trail; this time it produced a permission bypass. * docs+test: correct three overstatements and strengthen the oracle test (BUG-2578) Codex round 5. Three of the four findings are my own prose claiming more than the code does — the same failure mode this branch has now produced four times, so it is worth fixing rather than shrugging at. The resolver's doc said a singular form works for "every collection". It handles a trailing ASCII `s`, so `spec`/`specs` resolves and `category`/`categories` does not. The doc now says "a regular singular/plural pair", names the limit, and points at the paragraph explaining why -s is a deliberate stopping point rather than a gap to close with an inflector. bulkMoveCollection's doc said its targetColl parameter "is never nil". The immediately preceding commit made it deliberately nil for an unresolved target — that is what keeps a hidden and a nonexistent collection failing identically — and the function has a nil check three lines down. Now says so. The MCP test's comment implied the transport. It drives the dispatcher against a real in-process server, which proves the resolution reaches an MCP tool call; it does not go over the remote /mcp HTTP transport or its OAuth layer. Scope stated in the test so nobody reads more into a green run. The fourth is a real test weakness: the existence-oracle test compared only HTTP status, so an implementation returning both cases inside a 200 envelope with different error codes would have passed while still leaking. It now compares the per-item failure shape too, with item ids stripped since those legitimately differ, and a non-JSON body compared verbatim rather than normalized to empty — which would have made two different errors look identical. Mutation-verified: changing only the unresolved-target error code, leaving the status alone, now fails it. Round 5's P1 — that cross-workspace copy requires workspace-level edit on the destination before any collection-grant check, so a destination collection grant is unusable — is NOT addressed here and is not mine to judge on this branch. The ordering predates this diff (I only swapped the lookup call), and the scope constructor is explicitly named CrossWorkspaceWorkspaceOnlyScope, which reads deliberate rather than accidental. Raised with the lead as an unverified observation rather than filed as a defect, since I have not read PLAN-2357's authorization design and would be filing a design question dressed as a bug. * test: read the failure field the endpoint actually emits (BUG-2578) Codex round 6. normalizeBulkFailures decoded failed[].message; the endpoint emits failed[].error (bulkItemFailure). So the message half of the existence-oracle comparison decoded to the empty string for every row and compared equal always — dead since the moment I added it to close exactly that gap, and my mutation had changed the code AND the message together, so it failed on the code and told me nothing about the message. Fixed, and re-verified with a mutation that leaves the status and the error code identical and changes only the message: it now fails. The struct carries a note that the field names mirror bulkItemFailure, since an invented name here fails silently rather than loudly. Third time on this branch that a test I wrote to be rigorous was not, and the tell each time was that I checked it passed on good code without checking WHICH part of it could fail. * fix(server): an archived collection blocks the alias instead of handing its name away (BUG-2578) Codex round 7, and it took a real judgement call rather than a mechanical fix. GetCollectionBySlug skips soft-deleted rows, so with an archived `spec` alongside a live `specs`, the exact lookup missed and the alias fallback picked up `specs` — archiving a collection would quietly start routing its writes into a different one, and a later restore would leave those items stranded where they were rerouted. I first read this as acceptable: an archived collection is not a writable target, so resolving to the live neighbour looks like the alias feature doing its job. What decided it the other way is that this branch already refuses exactly this trade on the client side. BUG-2630's whole complaint is that a silent misroute into a different collection is worse than an honest error, and the same reasoning cannot be right there and wrong here just because the redirect happens to be convenient. Archived rows now claim their name: the exact form returns not-found rather than falling through. The narrow store method (ArchivedCollectionClaimsSlug) answers a boolean rather than returning the row, because an archived collection is never a valid target — it only blocks the name, and returning it would invite a caller to use it. Covered end to end with the fixture armed first (the collection resolves to itself while live, so the assertion is about the archive edge and not about the resolver being broken generally), and mutation-verified: removing the guard fails it with the item sitting in `specs`. * fix(server): run the archived-name guard for every candidate, not just the input (BUG-2578) Codex round 8. The previous commit checked the archived claim only for the raw input, so an archived `spec` beside a live `specs` still let `Spec` through: the exact form missed, the case-folded candidate `spec` found no LIVE row (GetCollectionBySlug skips soft-deleted), and resolution walked on to `specs`. The archived name was stepped over by a spelling of itself. Restructured so the sequence is uniform — the raw input and every fallback ask the same two questions in the same order, is there a live collection with this name and does an archived one claim it. That is also easier to reason about than a guard bolted in front of a loop, which is how the hole existed. Mutation-verified with the previous shape restored: guarding index 0 only fails the new test with the item sitting in `specs`.