6 Commits

Author	SHA1	Message	Date
xarmian	449ac109e9	fix(server,mcp,cli): refuse system-metadata keys in fields_patch + retry-hostile error code (BUG-2627 part 2, BUG-2675) (#1166) ... * fix(server,mcp,cli): refuse system-metadata keys in fields_patch + retry-hostile error code (BUG-2627 part 2, BUG-2675) Part 2 of BUG-2627 closes the door that mints the defect parts 1 and 3 dealt with: `--field implementation_notes=<json>` stored the entries as a JSON-ENCODED STRING, which is invisible to every reader and — since part 3's guard — disables `pad item note` on that item until the row is repaired. Refused SERVER-SIDE in `fields_patch`, not at the CLI as the item's scope line proposed. The deviation is deliberate and recorded on the trail: the CLI is one of three clients, and all three lower a user field-setter into the same key (`pad item update --field` at cmd_item.go, the MCP `field` param via dispatch_http_advanced.go on remote, and stdio by shelling out to that CLI). One gate closes all three; a CLI-only refusal would have left remote MCP writing the key. Both call sites were read, and the CLI's lowering is now pinned by a test rather than left as an assumption. Scope, stated because it is deliberate: this closes UPDATE only. The full `fields` blob stays open because that door is SHARED — `pad item note` / `decide` / `github link` send one, and so does convention activation via BuildConventionItemFields -> ItemCreate. Closing it would break the system writers the gate exists to protect. Item create therefore remains a mint site, tracked with the rest of that surface in BUG-2685. The refusal message is per-key: implementation_notes -> `pad item note`, decision_log -> `pad item decide`, github_pr -> the GitHub link flow, and `convention` refuses WITHOUT naming a command, because none writes it. PATTE-135 wants a remedy that works in the failing state; a single "use pad item note" line would have been wrong for three of the four keys. BUG-2675 rides along on one ToolSurfaceVersion bump, as ruled. The append refusal from part 3 reached MCP agents as `server_error` — not our fault, and not transient, so agents could reasonably retry a failure that is deterministic forever. New closed-set code `stored_state_unreadable`, emitted on BOTH transports: HTTP classifies the sentinel error directly, stdio via a `pad-structured-error/v1:` marker the CLI now writes for its own local refusal (the first marker generated without an upstream APIError). v0.16-then-v0.17 is what a one-transport fix costs. Also here: - items.ReservedOverrideKeys -> ReservedFieldKeysIn. The second caller passes a patch, not an override map, and the old doc comment said fields_patch was an open exposure — true until this commit. - `Extract* returns nil for THREE reasons` -> FOUR. The comment listed four; the count was corrected everywhere except the code. - Consumer-read artifacts updated where the claim is ACTED on, not only where it is documented: instructions.md (incl. a "do not retry this code" section), the catalog `field` param description, `pad item update --help`, README. Gates: build · make lint · go test ./... · make test-pg · Codex. Eleven-mutation matrix run against the new tests; every one killed by an assertion (two were rewritten after killing by compile error / surviving, which proves nothing). Claude-Session: https://claude.ai/code/session_017jD6t1zjxGSq47SQpZfp1V * fix(server,mcp): honest remedy when the stored value is already unreadable; name the MCP-facing code (Codex round 1) Three findings from the pre-push review, all real: P2 — the refusal named `pad item note` unconditionally, but on an item whose stored value is ALREADY undecodable that command refuses too (part 3's guard). The caller was routed in a circle: field write refused -> run the note -> refused -> back again. That is exactly the failure PATTE-135 exists to prevent, and my own trail had reasoned the remedy was safe on the strength of the HEALTHY case only. The message now inspects the item's stored value and, when the key is unparseable, says so and points at the one action that works in that state (inspection), noting that the repair needs a full `fields` write no CLI flag exposes. P2 — two doc claims were false where an actor reads them. The catalog said reserved keys are refused "on every action that accepts field", which includes CREATE, and create is deliberately NOT gated; and both the catalog and instructions.md named `validation_error` (the HTTP code) where an MCP client actually receives `validation_failed`. Both corrected, and the create exception is now stated rather than implied by omission — an agent that reads only "refused on update" will otherwise assume create is fine, which is how a hole gets used. nit — the destructive-downstream sentence claimed every reserved key becomes unreadable and trips an append guard. True only for the two append-backed keys; github_pr and convention are simply overwritten. The clause is now per-key, because a confident wrong explanation is worse than a vague right one. Two more mutations run against the new branch: always-readable (the circular remedy returns) and never-readable (the working remedy disappears) — both killed by assertions. Claude-Session: https://claude.ai/code/session_017jD6t1zjxGSq47SQpZfp1V * fix(models,mcp,cli): one appendability predicate, per-key docs, stdio hint parity (Codex round 2) Five findings, all real. P2 — the message's readability check and the guard it describes were two different decodes. Mine unmarshalled into []json.RawMessage; the guard uses []ItemImplementationNote. A stored `[1]` passed mine and fails the guard, so the message would again have prescribed a command that refuses — the same circularity round 1 caught, through a narrower door. Replaced with models.StructuredFieldIsAppendable, which ASKS the guard rather than re-deriving it, plus an agreement test over 12 shapes x 2 keys that compares the predicate against the real Append* helpers. Verified by restoring the RawMessage version: the table catches it on `[1]`. P2 — stdio lost the new code's hint. Remote MCP told the agent retrying is pointless and how to inspect; stdio got the code with an empty hint, because the CLI's marker envelope carried none and the classifier parsed none. Both fixed, with the hint hoisted into paired constants (the same duplication StructuredErrorMarker already uses) and the test comparing the two TRANSPORTS' envelopes rather than either against a literal. P2 — doc text was still false for `convention`: the catalog, the instructions and `--help` all said reserved keys are maintained by note/decide/the GitHub flow, which is true of three of the four. Each key now names its own writer, and `convention` names library activation. Also dropped the `malformed_override` advertisement — that is the SERVER's code; an MCP client sees validation_failed for both refusals. nit — the classification test called structuredAppendErrorResult directly, so deleting either dispatcher call site left it green. Added dispatcher-level tests driving the real server + store, asserting the code, the hint, and that the item's stored fields are byte-identical afterwards. Mutation-verified by reverting the note call site. Claude-Session: https://claude.ai/code/session_017jD6t1zjxGSq47SQpZfp1V * fix(items,models,mcp): github_pr stays writable through fields_patch; no nil-map panic (Codex round 3) P1 — the gate refused `github_pr`, and that was wrong. My model was "system writers use the full fields blob, user setters use fields_patch", which holds for three of the four reserved keys and fails for this one: `pad github link` needs a local git checkout and the `gh` CLI, so it is excluded from remote MCP BY NAME, and internal/mcp/dispatch_http.go's noRemoteEquivalent map tells remote agents in so many words to use `item update --field github_pr=...` instead. For that audience the patch door is not a bypass of the writer — it IS the writer. So the refusal deleted a documented capability from remote agents, and answered with a message naming a command they cannot run: the same circular remedy round 1 caught, aimed this time at the people the gate was meant to help. items.PatchRefusedFieldKeysIn now exempts the key and records the rule being applied — refuse a raw write where a real writer exists — rather than the list it produces. Whether remote agents should get a proper PR-link action, so the key can be closed too, is a product question and is left as one. P2 — the hint told agents to read the bad value with `pad_item action=get`. They cannot: stripDuplicatedFieldsKeys removes implementation_notes and decision_log from every MCP response's fields blob, and the top-level arrays come from the extractor, which returns nil for exactly this shape. The value is invisible on the whole surface. The hint now says so and routes to a human, who can read it with `pad item show --format json`. P2 — `fields` holding a literal `null` unmarshals into a NIL map with no error, and both Append* helpers assign into what they get back, so `pad item note` PANICKED ("assignment to entry in nil map") instead of appending. Reproduced, fixed in parseMutableItemFields, and pinned by a test that fails on a panic rather than taking the process down. An absent blob and a null blob mean the same thing to every caller. Pre-existing, but it sits in the function family this bug is about and the message was about to recommend the command that panics. nit — README claimed a "closed eight-code taxonomy" (17 codes, and I had just added one) and read as if create lowers into fields_patch. Claude-Session: https://claude.ai/code/session_017jD6t1zjxGSq47SQpZfp1V * fix(models,mcp): predicate matches the append on malformed blobs; stop promising a broken workaround (Codex round 4) P1 — round 3 exempted `github_pr` from the update gate on the strength of noRemoteEquivalent's documented workaround. That workaround does not work: ingestFieldKVP (remote) and parseFieldFlag (CLI, and so stdio) both store a `field` value as a STRING, so the PR data lands double-encoded and no link appears — the BUG-2627 shape one key over. Filed as BUG-2696 with the three candidate fixes; NOT folded in, because the narrowest of them changes how every field value is typed. The exemption stands regardless: refusing would leave remote agents with strictly less than a broken door. What changes is what we may PROMISE. The catalog, instructions.md, version.go and README said "this is how you link a PR"; they now say the door is open and broken, and to hand PR linking to a human. Advertising a capability that isn't there is the failure mode this whole unit keeps circling. P2 — StructuredFieldIsAppendable returned TRUE when the whole fields blob was unparseable, on the reasoning that a broken outer blob is a different problem. True of the cause, irrelevant to the caller: the Append* helpers bail on that same parse, so the message again named a command that fails. It now returns false, which is simply the honest answer to the question asked, and the agreement table grew a malformed-outer-blob leg — the gap that let the disagreement through. P2 — the message claimed a raw field write always stores something Pad cannot read back. That holds for the CLI and MCP (a `--field` value is typed by schema lookup and these keys are in no schema) but not for a direct REST caller sending a valid array, who is refused for ownership reasons alone. Reworded to say both parts. nit — a misplaced parenthetical in the README read as if item CREATE lowers into fields_patch. It does not; it sends the full blob. Claude-Session: https://claude.ai/code/session_017jD6t1zjxGSq47SQpZfp1V * fix(mcp,models): stop the remote hint advertising the broken PR workaround; classify an unparseable blob as retry-hostile (Codex round 5) P1 — I corrected four artifacts that pointed agents at the github_pr field write and missed the fifth: noRemoteEquivalent's own text, which IS the message a remote agent receives when it calls `github link`, and which Codex had quoted at me in round 3 to establish the workaround existed. The nearest artifact to the actor was the one I did not open. Both entries now say there is no working remote path and name BUG-2696, with a test pinning the negative so a future edit cannot quietly reinstate the advice while the write is still broken. P2 — a fields blob that will not parse at all produced a bare parse error, so `note` / `decide` reached agents as `server_error`: transient- looking, and therefore retried, for a failure that is as deterministic as the per-key one BUG-2675 exists for. Both Append* helpers now wrap that parse failure in ErrStructuredFieldUnreadable, which both transports already classify, and the malformed-blob test asserts the sentinel rather than just an error. Claude-Session: https://claude.ai/code/session_017jD6t1zjxGSq47SQpZfp1V * docs(mcp,cli): qualify what an agent can actually see when the state is unreadable (Codex round 6 nit) Round 5 widened stored_state_unreadable to cover a fields blob that fails to parse outright, which made half of its own hint false: MCP's normalization strips a broken structured KEY (so `get` hides it), but leaves an unparseable BLOB as a raw string (so `get` shows it). The hint and instructions.md asserted the first case for both. Now stated per layer, in the two paired constants and the instructions. The reason it is worth the words rather than being cut: an agent told 'you cannot see this' does not look, and would have missed a value that was in fact right there in the response it already had. Claude-Session: https://claude.ai/code/session_017jD6t1zjxGSq47SQpZfp1V * fix(mcp): classify the move/copy reserved-key refusal as validation on stdio too (Codex round 7) P2 — carried over from v0.22, surfaced because THIS bump documents the two reserved-key refusals as agreeing across transports. The move/copy message ("Field(s) reserved for system metadata and not settable here") matched none of the stdio validation patterns, so the same deterministic 400 arrived as validation_failed on remote and server_error on stdio — and server_error reads as transient, so an agent retries a refusal that can never pass. One pattern added, plus a test that drives both real classifiers with the real server message text for both refusals, so a reworded message that stops matching fails here rather than in the field. nit — the github_pr exemption is UPDATE-only; move and copy still refuse it, because there the argument is BUG-2674's (an override reintroduces the key the migration just dropped), not this one's. The catalog and instructions said "not refused" without that qualifier. Claude-Session: https://claude.ai/code/session_017jD6t1zjxGSq47SQpZfp1V * fix(mcp): cover the copy path's own refusal wording in the stdio classifier (Codex round 8) P2 — round 7 fixed the MOVE wording; the copy path words the same class of refusal differently ("Destination collection has no field(s): ..."), so it kept arriving as server_error on stdio and validation_failed on remote. Third message in one family, and the round-7 test used the move text for every case, which is why it missed this. The parity table now carries all three real messages plus a control leg using one the pattern list already covered — without it the table could pass by matching everything. Recorded in the pattern list's comment rather than left implicit: matching prose is a stopgap, the structural fix is the pad-structured-error/v1 marker that carries the code instead of inferring it, and until a refusal emits one, this test is where a new wording has to be added. Claude-Session: https://claude.ai/code/session_017jD6t1zjxGSq47SQpZfp1V * test(mcp): use the real upstream codes in the parity fixtures (Codex round 9 nit) The copy legs carried `validation_error` where the handlers actually emit `malformed_override` and `invalid_override`. The 400 branch ignores the body code today, so the test passed either way — which is exactly why the fixture mattered: it was quietly recording a wrong contract, and a future code-aware classifier would regress against a table that agrees with it. Claude-Session: https://claude.ai/code/session_017jD6t1zjxGSq47SQpZfp1V * docs(mcp): the upstream code is not forwarded to MCP clients (Codex round 10 nit) The catalog said the server's own code (validation_error / malformed_override) appears in the MCP message. It does not: the 400 branch emits code=validation_failed with a fixed "Validation failed." message and the server's text in the HINT, discarding the finer-grained code. Reworded to say what an agent actually receives, and to say that telling the two refusals apart means reading the message. Also carried the update-only qualifier on the github_pr exemption into the README, matching the catalog and instructions. Claude-Session: https://claude.ai/code/session_017jD6t1zjxGSq47SQpZfp1V * docs(items): state the exemption predicate, not the exemption list (lead ruling) The lead's ruling on the github_pr reversal: make the REASON what the code says, so the next key added to reserved metadata is evaluated against 'does this audience have a real writer?' rather than pattern-matched onto a list that happened to be wrong for one key. Claude-Session: https://claude.ai/code/session_017jD6t1zjxGSq47SQpZfp1V	2026-08-19 23:17:24 -04:00
xarmian	ae62c097ca	refactor(mcp): consolidate project next/standup/changelog onto REST endpoints (TASK-1916) (#802) ... * refactor(mcp): consolidate project next/standup/changelog onto REST endpoints (TASK-1916) dispatchProjectNext/Standup/Changelog were a second server-side copy of the next/standup/changelog reshaping contract, written before TASK-1894 shipped dedicated REST endpoints for the same data. Replace the ~200 lines of duplicate reshaping with thin proxies that validate workspace (preserving the pad_set_workspace hint) and forward to GET /next|/standup|/changelog, relying on packageHTTPResponse's existing array-wrap (BUG-985) and the REST handlers' own days-default and per-status best-effort semantics rather than replicating them. dispatch_http_slice4.go is deleted; its unrelated dispatchLibraryActivate moves to dispatch_http_library.go now that the file's other three methods are gone. KEEP IN SYNC comments across handlers_project_intel.go/server.go/tests collapse from "three reproductions" to "CLI + REST, MCP proxies to REST." * fix(server): pass nav-lenient visibleIDs to changelog's parent enrichment (codex R1 P1, TASK-1916) handleGetProjectChangelog passed guestResourceFilter's narrowed collIDs into enrichItemsWithParent instead of the nav-lenient visibleCollectionIDs set handleListItems uses for the same enrichment call. For a guest whose granted item's parent lives in an item-grant-only collection (nav-visible but excluded from the narrowed full-access set), this silently dropped the parent link fields, causing itemMatchesParentFilter to exclude the item from ?parent= results even though the guest can otherwise see it. The root cause predates TASK-1916 (introduced alongside the REST endpoint in TASK-1894), but this consolidation imports it into MCP wire behavior via dispatchProjectChangelog's proxy, so it's in scope to fix here. projectIntelVisibility now returns the unnarrowed visibleCollectionIDs result (navVisibleIDs) alongside the existing (collIDs, itemIDs) pair; handleGetProjectChangelog uses navVisibleIDs for enrichItemsWithParent while keeping collIDs for the list query, mirroring handleListItems' pattern exactly. handleGetProjectStandup and handleGetProjectNext have no parallel enrichItemsWithParent call (verified by reading both, and buildDashboardResponse) so neither needed the same treatment. Added TestProjectChangelogEndpoint_GuestParentFilter_ItemGrantOnlyCollection, confirmed to fail against the pre-fix code and pass against the fix.	2026-07-03 22:29:40 -04:00
xarmian	6433cc51ea	feat(mcp): pad_library catalog tool + ToolSurfaceVersion 0.5 (TASK-1563) (#615) ... * feat(mcp): pad_library catalog tool + ToolSurfaceVersion 0.5 (TASK-1563) MCP catalog wiring for PLAN-1560 (`pad_library` MCP tool + matching CLI surface). Closes IDEA-1514 — pure-MCP agents (notably the /pad onboard playbook from PLAN-1496) can now browse and activate library entries without shelling out. ## New tool `pad_library` joins the v0.5 catalog as the ninth resource × action tool. Three actions, all passThrough to the `pad library` CLI: - `list` — Browse conventions + playbooks. Defaults to summary mode for playbooks (compact bodies via the ?summary=true endpoint flag); conventions always carry full content. Optional type / category / full inputs. - `get` — Full body of one entry by exact title. Conventions-first precedence mirrors `activate`. - `activate` — Create a workspace item from a library entry by title. `Workspace: true` on the tool — list/get ignore it; activate validates and uses it. The schema-level declaration gives activate automatic pad_set_workspace session-default resolution (same precedent as pad_meta's mixed-workspace actions). ## Dispatcher extensions - `dispatchLibraryList` forwards `category` to BOTH endpoints and passes `summary=true` to the playbook endpoint by default (unless input.full=true). MCP-default summary mode keeps agent context budgets tight; CLI default already aligned in TASK-1562. - `library get` added to the routeTable as a clean GET to /api/v1/library/entry with `title` mapped to the query string. Cleaner than another explicit dispatcher case — matches playbook list / playbook show shape. ## Version bump ToolSurfaceVersion bumped from 0.4 → 0.5. Pure addition; no existing tool/action/param/bootstrap shapes changed. Backwards-compatible for any v0.4 consumer that doesn't enumerate the new tool. Documented in version.go with the same comment-block structure as prior bumps. ## Test coverage - catalog_readonly_test.go — pad_library added to the want{} map; three library action → cmdPath entries in expected{}; library list / get / activate added to liveCmdhelpDoc stubs. - dispatch_http_project_test.go — 4-case table test (defaults, category, full=true, category+full) pins category/summary query-param forwarding; library get routing test confirms the routeTable entry resolves. ## Live MCP verification - `initialize` handshake advertises padToolSurface.version=0.5. - `pad_meta version` returns tool_surface_version=0.5. - `pad_library list type=playbooks category=agent-workflows` returns 4 playbooks in summary mode (content stripped, summary populated, invocation_slug + arguments present). - `pad_library get title='Ship tasks'` returns {type: playbook, playbook: {…, content (9512 chars), invocation_slug: ship}}. Parent: PLAN-1560. Unblocks TASK-1564 (cleanups). * fix(onboard): update playbook body to use pad_library MCP tool per Codex review (round 1) Codex P2 on PR #615: the /pad onboard playbook body in internal/collections/playbook_library_onboard.go still told MCP-only agents that the library catalog was "not yet exposed as an MCP tool" and to work from memory — directly contradicting the pad_library tool this PR just landed and breaking the main advertised consumer of the new surface. Updated step B3 (conventions) to mention both surfaces side-by-side (`pad library list --type conventions` / `pad_library` with `action: list, type: conventions`), and rewrote step B5 (playbooks) the same way so the activate path doesn't drift either. Pre-PLAN-1560 IDEA-1514 reference removed from the body — the idea is now closed. No test pins the playbook body content; `make check` passes; the playbook seed still validates against the playbooks collection schema since trigger/scope/invocation_slug/arguments are unchanged. Closes the onboard-side scope of TASK-1564 (stale dispatch_http_slice4 hint + CHANGELOG still pending there).	2026-05-21 19:18:19 -04:00
xarmian	e59d3904c9	feat(server): refuse to mark item terminal while it has open children (IDEA-1494) (#571) ... * feat(server): refuse to mark item terminal while it has open children (IDEA-1494) Server-side guard inside handleUpdateItem that rejects a non-terminal → terminal done-field transition when the item still has at least one non-terminal child. Returns HTTP 409 with code=open_children plus a structured details payload listing each blocking child's {ref, title, status, collection_slug} so MCP-driven agents can self-recover (ship the listed children, then retry) and the CLI can render the same list verbatim. Escape hatch: `--force` on `pad item update` / `pad item bulk-update` and `force: true` on the MCP pad_item.action: update / bulk-update inputs both forward into the same ItemUpdate.Force transport field the handler consumes before any store mutation. Trigger conditions are tight: the PATCH must change the done-field key (resolved via TerminalValuesForDoneField against the parent's schema + settings) AND the new value must be terminal AND the current value must NOT already be terminal. Terminal → terminal and no-op terminal transitions bypass the guard; only entering the terminal set is gated. Per-child evaluation uses the child's own collection schema so hierarchical workspaces with custom typed collections work without extra plumbing. Tests cover: rejection with one open child (with mutation-safety assertion on the parent), no children, all-terminal children, --force override, no-op terminal → terminal, terminal → terminal, non-terminal → non-terminal, custom collection terminal_options honored, and a parent task (not a plan) — IDEA-1494 optional extra #3. MCP coverage asserts --force round-trips through both ExecDispatcher and HTTPHandlerDispatcher and is omitted when force=false. * fix(server): open-children guard round 2 — visibility, MCP pass-through, TOCTOU (IDEA-1494) Three Codex round-1 issues, each fixed with the recommended shape: P1 — visibility leak. The 409 response previously listed every blocking child by ref/title/status, including children in collections the caller couldn't see. The INVARIANT still evaluates against ALL children (it's a data-integrity gate — a restricted user must not be able to close a parent whose blockers they can't see), but the response payload now filters to caller-visible children only. Hidden blockers surface as a new `details.hidden_blocker_count` field plus an alternate human message when every blocker is hidden ("blocked by N open children you don't have access to"). Mirrors the visibility helpers (`visibleCollectionIDs` + `isItemVisibleToGuest`) used by the per-parent progress endpoint so the two paths can't drift. P2 — MCP code/details pass-through. The HTTP classifier was collapsing 409 into the generic `conflict` code and dropping `details`; the stdio classifier was matching the human "cannot " message against the validation regex and surfacing `validation_failed`. Both now surface `open_children` with the structured details intact: - HTTP: classifyHTTPStatusKind's 409 branch extracts the upstream code; any non-empty, non-"conflict" code is passed through with its `details` RawMessage. Generalizes beyond open_children — any future structured 409 from a handler gets the same treatment. - Stdio: the CLI writes a `pad-error: {json}\n` marker line on stderr before the human-readable block (single source of truth for both views), and classifyExecError detects the marker and lifts the envelope verbatim. Marker is duplicated as a const between internal/cli and internal/mcp to avoid pulling the cli package into the classifier just for one string. A new ErrOpenChildren error code constant + `Details json.RawMessage` field on ErrorPayload back the wire shape. P2 — TOCTOU. The guard previously ran in the handler before the store transaction began; a concurrent child insert / child status flip could slip between the children-list read and the parent's UPDATE. Fix: - New `Store.UpdateItemWithPreCheck(id, input, precheck)` runs the caller's invariant check inside the same tx, after acquiring the workspace seq lock AND a new parent-children advisory lock keyed on the parent ID. UpdateItem is now a thin wrapper passing nil. - Every UpdateItem unconditionally acquires the parent-children advisory lock for its own parent (if any) AND for itself-as-parent, in a fixed order (parent first) so two updaters touching the same parent always grab that key before the more-specific one — no AB/BA deadlock. - New `GetChildItemsTx` reads via the caller's tx; on Postgres the advisory lock provides the snapshot guarantee (DISTINCT precludes `FOR UPDATE`), on SQLite the global BEGIN IMMEDIATE write lock serializes all writers. - Handler now passes a precheck closure into UpdateItemWithPreCheck at all three call sites (collab-snapshot path, applier-direct-write path, main path). The guard's openChildrenGuardError sentinel is unwrapped after each call so the 409 surfaces cleanly. Tests: - TestOpenChildrenGuard_VisibilitySanitizesPayload — restricted editor sees parent + visible child, hidden child contributes to hidden_blocker_count, no leak of ref/title/slug. - TestOpenChildrenGuard_AllBlockersHiddenSurfaceGenericMessage — open_children=[], hidden_blocker_count>0, message mentions "you don't have access to." - TestOpenChildrenGuard_TOCTOURace — 8 iterations of a child-flip racing a parent-terminal update; asserts the forbidden outcome (parent=completed AND child=open) never occurs. - TestClassifyHTTPStatus_OpenChildrenPreservesCodeAndDetails + inverse generic-409 test. - TestClassifyExecError_OpenChildrenMarkerLiftsStructuredPayload + no-marker-falls-through inverse. * fix(server): open-children guard round 3 — 7 Codex findings closed (IDEA-1494) P1 — visibility fail-closed. The handler was swallowing visibleCollectionIDs errors, leaving visIDs==nil which the guard treats as unrestricted, leaking hidden-child metadata. Now surfaces the error as 500 BEFORE installing the precheck. Test: TestOpenChildrenGuard_VisibilityLookupErrorFailsClosed closes the store DB and asserts no 409+children leak. P1 — link mutations acquire the advisory lock. SetParentLink, ClearParentLink, CreateItemLink (when link_type ∈ childLinkTypes via new isChildLinkType helper), DeleteItemLink (same condition), and RestoreItem now take `pad:parent-children:<id>` in canonical sorted order via new AcquireParentChildrenLocks helper. SetParentLink locks BOTH old and new parents (re-parenting case). Race test TestOpenChildrenGuard_LinkMutationRace asserts the forbidden "link-committed-before-parent-flip AND parent flip succeeded" never occurs by comparing link.created_at to parent.updated_at. Documented semantics: status-wins + link-after-commit is legal under the invariant "no open children EXIST AT THE MOMENT of transition" — the post-condition variant ("no open child may EVER attach to a terminal parent") is intentionally deferred. P1 — MoveItem bypass closed. New MoveItemWithPreCheck mirrors UpdateItemWithPreCheck — acquires workspace seq lock + parent-children locks, re-reads in tx, runs caller precheck. handleMoveItem builds the same guard closure using the DESTINATION schema for done-field resolution (conservative — honors the schema the item moves INTO). CLI gains `pad item move --force`, client gains MoveItemWithForce that appends `?force=true` to the move endpoint. MCP catalog + mapItemMove forward `force` through the route mapper. Tests: TestOpenChildrenGuard_MoveItem_RejectsTerminalWithOpenChildren and …_ForceOverrides. P2 — pre-tx field-read TOCTOU. UpdateItemWithPreCheck and MoveItemWithPreCheck now re-read the item via new getItemTx INSIDE the tx (after locks) and pass that fresh snapshot to the precheck closure; the precheck classifies the transition against the in-tx view, not the handler-side pre-tx capture. Handler precheck closure swaps `currentFieldsJS` from the in-tx snapshot. Test: TestOpenChildrenGuard_PrecheckReadsInTxSnapshot stages a between-load status mutation and asserts the precheck observes the post-mutation fields. P2 — bulk-update carries structured errors. cmd/pad/main.go's updateFailure struct extended with Code + Details (json.RawMessage). When client.UpdateItem returns *cli.APIError, the row preserves the structured envelope. Human-text output also renders the open-children list inline. Chose JSON-envelope route over per-row stderr markers because bulk-update already produces a structured envelope and ExecDispatcher returns stdout verbatim on exit-0 — no classifier change needed. Test: TestBulkUpdateStructuredFailuresCarryOpenChildrenDetails confirms the wire shape the CLI lifts. P3 — marker hardening. Marker bumped to versioned form `pad-structured-error/v1:` (was `pad-error:`). cli.StructuredErrorMarker + mcp.structuredErrorMarker kept in lockstep with cross-references. mcp.allowedStructuredErrorCodes whitelists known codes (currently just open_children); unknown codes fall back to regex classification. Marker must start the line after whitespace trim (embedded markers ignored). Last-marker-wins to defeat pre-emption attacks. Tests: TestClassifyExecError_{UnknownStructuredCode,OldMarkerVersion, MarkerEmbeddedMidLine,LastMarker}. P3 — soft-deleted collection schemas honored. New GetCollectionAnyState mirrors childrenDoneFiltersForParent's inclusion rule; guard uses it so a child still attached to a soft-deleted collection is evaluated against ITS schema (custom terminal_options) instead of the default- status fallback (which would mis-classify and false-block). Test: TestOpenChildrenGuard_SoftDeletedCollectionSchemaHonored seeds a custom collection, soft-deletes it while a child remains, and asserts the terminal status is correctly recognized. Comprehensive store-mutation audit results recorded in the PR description (every method touching items.fields / items.collection_id or item_links). * fix(server): open-children guard round 4 — multi-parent locks, enum parity, PATCH atomicity (IDEA-1494) Four Codex round-3 (blast-radius lens) findings, each fixed with the recommended shape. P1 — multi-parent lock set. acquireParentChildrenLocksForUpdate and RestoreItem previously used `LIMIT 1` against item_links, so a child with BOTH a `parent` link to P1 AND an `implements` link to P2 only locked one of them. The other parent's open-children precheck could race against the child's status flip and miss it. Fix: new listParentChildLockKeys helper runs the same query GetChildItems' inclusion rule uses (childLinkTypes), returns ALL distinct parent target_ids, and feeds them into the canonical multi-lock helper. Both UpdateItemWithPreCheck and RestoreItem now acquire locks on {self} ∪ {all-parents-via-childLinkTypes}. Test: TestOpenChildrenGuard_MultiParentChildLocksAll races a child status flip against terminal-updates on both parents simultaneously. P2 — lock-order asymmetry. The pre-fix codebase had multiple lock- acquisition shapes: parent-then-self in acquireParentChildrenLocksForUpdate, single-key in RestoreItem / CreateItemLink / DeleteItemLink / ClearParentLink, and a sorted multi-key in SetParentLink. Two concurrent callers using different ad-hoc orderings could AB/BA deadlock. Fix: removed the per-call-site AcquireParentChildrenLock helper entirely. Every site now goes through AcquireParentChildrenLocks (the canonical sorted multi-lock helper) — including ones that need only one ID (the variadic call still sorts a one-element slice). The helper's doc comment explicitly states the contract: "Ad-hoc single-key acquisition outside this helper is FORBIDDEN — two call sites taking distinct keys in different orders WILL deadlock." Test: TestOpenChildrenGuard_NoDeadlockUnderReverseOrderConcurrency runs reverse-order re-parents with a 5-second timeout; assertion fails on hang. P2 — HTTP/stdio code-surface parity. Round 2's HTTP pass-through ("any non-conflict upstream code") silently widened the ErrorCode enum beyond stdio's allow-list (`open_children` only). Agents saw different code surfaces depending on which dispatcher delivered the response. Fix: HTTP 409 branch in classifyHTTPStatusKind now consults the same allowedStructuredErrorCodes whitelist stdio does. Codes outside the set collapse to ErrConflict (no details), matching what stdio does for an unknown-code structured marker. Doc on allowedStructuredErrorCodes updated to make the dual-consumer contract explicit: "Adding a new structured code is a TWO-WAY change." Tests: TestClassifyHTTPStatus_UnknownConflictCodeFallsBackToErrConflict and TestStructuredErrorCodeParityAcrossTransports. P3 — PATCH atomicity. A combined PATCH with `parent` + `status=terminal` on an item with open children used to commit the parent-link change INLINE (before the guard ran) and then reject the field write. Caller saw 409 but the parent had already moved. Fix: parent-link mutation is now DEFERRED — captured into outer- scope vars during fields validation, executed AFTER UpdateItemWithPreCheck succeeds. A guard rejection returns before the link write block, so on rejection the link is untouched. Documented choice: "reorder, don't tx-wrap" — wrapping SetParentLink into the same store tx would require threading a *sql.Tx through the SetParentLink API (which is also called from the handler_item_links path); reordering is the smaller surgery and gives the correct outcome on the failure direction. A residual window remains in the OTHER direction (field write commits, link write fails) — not made worse by the reorder, and called out inline for a future tx-wrap pass. Test: TestOpenChildrenGuard_PatchAtomicRejectionPreservesParentLink sets up target → oldParent → openChild, sends PATCH {parent=newParent, status=completed}, asserts 409 AND target.parent_link still points at oldParent. * fix(server): open-children guard — emit details.open_children as [] not null on hidden-only rejection (IDEA-1494)	2026-05-17 00:15:52 -04:00
xarmian	42f6ce96e1	fix(mcp): normalize error envelope shape + extend code taxonomy + actionable hints (TASK-1077/1078/1079) (#388) ... Three independent improvements bundled as one PR because they all touch the same dispatcher error-emission surface; landing them piecemeal would churn the same lines repeatedly. ## TASK-1077 — uniform envelope shape Pre-fix some dispatchers emitted plain-string errors via `mcp.NewToolResultErrorf("%s: %s failed: %s", ...)`. Same underlying 404 surfaced in three different shapes across the surface (item lookup → structured envelope; note/decide → "item note: prefetch: 404 ..."; bulk-update per-row → bare error string). Inconsistent shape made it hard for agents to reason about errors uniformly. Three new helpers in errors.go: - validationFailedResult(cmdKey, msg, fixHint) — replaces the "X is required" / "invalid Y" chain across every dispatcher. - dispatcherErrorResult(cmdKey, op, err) — replaces the internal "build request: %s" / "encode body: %s" / "parse current: %s" chain. Always emits ErrServerError with a programmer-readable Hint. - upstreamHTTPErrorResult(...) — wraps every in-handler prefetch / sub-call HTTP failure through classifyHTTPStatusKind so the shape matches the main pipeline's responses exactly. Every NewToolResultErrorf call site in internal/mcp/dispatch_http*.go + catalog.go retrofitted. bulk-update's per-row `Error string` field flipped to `Error *ErrorPayload` so every row failure carries the same {code, message, hint} shape as a top-level failure. ## TASK-1078 — resource-kind-aware error codes Pre-fix every 4xx 404 collapsed to ErrItemNotFound regardless of what was being read; pad_workspace list returning 404 (route missing) reported `code: "item_not_found"` despite the call having nothing to do with items. Pre-fix every 5xx collapsed to ErrServerError, indistinguishable from dispatcher internal failures. Three new codes in errors.go: - ErrNotFound — resource-shaped 404s that AREN'T item lookups (collection, listing endpoint, link target, attachment). - ErrUpstreamError — 5xx with a structured body (transient backend failure). Distinct from ErrServerError (catch-all for dispatcher internal + un-mapped 4xx). - ErrBackendUnreachable — reserved for transport-level failures (DNS / connection refused / 5xx with no body); not yet emitted by classifyHTTPStatus but available for future transport-aware classification. - ErrWorkspaceRequired — reserved for the multi-workspace-token "ambiguous default" case (TASK-1076's deferred sister error; constant available even though dispatcher doesn't emit it yet). New ResourceKind enum (item/workspace/collection/listing/link/ attachment/unknown) lets callers tell the classifier what they were reading. classifyHTTPStatusKind is the new entry point; classifyHTTPStatus preserved as a legacy adapter for callers that haven't been retrofitted (pass ResourceUnknown → falls back to pre-TASK-1078 behaviour). Every retrofit call site passes its known kind + ref/slug, so 404s now route through the right code with a contextual message ("Item TASK-7 not found.", "Workspace foo not visible.", "Collection tasks not found.", etc.). ## TASK-1079 — actionable hints Pre-fix `hint` was usually `"404 page not found"` (chi's default NotFound body verbatim) or the upstream JSON envelope re-stringified. Either way: zero diagnostic value, sometimes outright misleading (double-stringified JSON in a hint field is hostile). Per-code hint generators in errors.go: - itemMissingHint — names the ref + route + suggests pad_item search / list as recovery. - workspaceMissingHint — names the slug + route + composes with the existing available_workspaces enrichment. - notFoundHintFor — kind-aware: collection 404 → "use pad_collection list to enumerate"; listing 404 → "verify the route matches the server's API surface (build version may be stale)"; etc. - authHintFor / permissionHintFor — point at re-auth / scope check. - upstreamHintFor — flags 5xx as "usually transient — retry once or check pad logs." extractUpstreamMessage parses pad's own structured `{error:{message}}` envelope when the upstream backend returned one, so hints lift the inner human-readable message out instead of dumping the literal JSON. Falls back to the raw body when the JSON shape doesn't match (no parse failure noise). ## Tests - TestDispatcher_AllErrorsUseStructuredEnvelope walks every special-case + link dispatcher's missing-required-input error path; pins the shape (code, message, hint all set; hint never just "404 page not found"). Adding a new dispatcher that uses NewToolResultErrorf will fail this test — it's the regression gate the DOD wants. - TestClassifyHTTPStatus_KindAware pins each ResourceKind → expected ErrorCode mapping for 404s. - TestClassifyHTTPStatus_HintsAreActionable pins that hints reference the actual route + ref + recovery tools, AND forbids the bare "404 page not found" passthrough that triggered Bug 17. - TestExtractUpstreamMessage covers the 7 input shapes the helper can see (structured envelope, empty inner, missing inner field, unparseable, wrong shape, empty, with extra fields). - Two existing tests updated to reflect the new shapes: TestClassifyHTTPStatus 5xx cases now expect ErrUpstreamError; TestMakeFanOutHandler_UnknownAction + TestActionEnv_Dispatch_ UnknownCmdPath substring searches updated for JSON-encoded quotes. ## Behavior diff agents will observe Same underlying 404, three example error envelopes: pad_item show TASK-MISSING: code: "item_not_found" message: "Item not found." hint: "Item \"TASK-MISSING\" not found. Route: /api/v1/.../items/TASK-MISSING. Try `pad_item search` or `pad_item list` to find the right ref." pad_workspace list (route 404): code: "unknown_workspace" message: "Workspace not visible to this session." hint: "Route: /api/v1/workspaces. Available workspaces: docapp, pad-web." pad_project dashboard (workspace doesn't exist): code: "unknown_workspace" message: "Workspace \"missing\" is not visible to this session." hint: "Workspace \"missing\" not visible. Route: /api/v1/workspaces/missing/dashboard. Available workspaces: docapp." Backend 500: code: "upstream_error" message: "pad item show failed: backend returned 500" hint: "Backend returned 500. Usually transient — retry once or check pad logs for the underlying error. Route: ..."	2026-05-02 22:10:12 -04:00
xarmian	1fc41f2f4a	feat(mcp): project intel + collection create + library list + bulk-update + note/decide (TASK-968 partial) (#348) ... * feat(mcp): wire project intel + collection create + library list + bulk-update + note/decide (TASK-968 partial) Continues TASK-968 past PR #347's stars/roles/webhooks slice. Nine more commands land here, one more joins noRemoteEquivalent. New commands: - project next → alias /dashboard (matches CLI's verbatim --format json output) - project ready → custom: extracts suggested_next as {count, results} - project stale → custom: filters dashboard.attention to interesting types (stalled/blocked/ overdue/orphaned_task), sorted - collection create → custom: parses --fields DSL (key:type[:opts];...) into CollectionSchema, builds settings - library list → composes /convention-library + /playbook-library based on --type - item bulk-update → iterates refs with per-item RMW; per-item failures surface in results rather than aborting - item note → RMW append using models.AppendImplementationNote - item decide → RMW append using models.AppendDecisionLogEntry Extended noRemoteEquivalent: - project reconcile → shells out to `gh` CLI for live PR state, same locality reasoning as the github commands The project-intelligence dispatchers reproduce the CLI's --format json shapes exactly: - `next` returns dashJSON verbatim (CLI does the same) - `ready` returns {count, results} extracted from suggested_next - `stale` returns {count, results} after filterAgentAttention's type filter + (type, ItemRef, ItemTitle) sort Aliasing all three to /dashboard would diverge — agents would see an unexpected wrapper shape. `item bulk-update` mirrors the CLI's per-item RMW loop, including the "existing fields survive" guarantee. The response shape {updated, total, results[]} makes per-item outcomes available so the agent can inspect what succeeded vs. failed without re-querying. Per-item refs accept string / []string / []any for schema-permissive callers. `item note` / `item decide` reuse models.AppendImplementationNote / AppendDecisionLogEntry so CLI-created and MCP-created entries are indistinguishable. The created_by field uses the requesting user's name (or email fallback) so audit trails work in multi-user MCP deployments — the CLI hardcodes "user" since it's single-user-per- process. Tests: - Per-command happy + missing-input rejection. - project ready/stale shape pinned (count, results); stale's filter + sort order verified. - parseCollectionFieldsDSL pinned: title-cased labels, status:select auto-required+default, malformed entries rejected, empty input returns empty fields[]. - library list type-filter skips other endpoint when --type set; unknown --type rejected with clear error. - bulk-update: per-item failure doesn't abort batch; existing fields survive RMW; status/priority required gating. - note/decide: AppendImplementationNote/Decision entries land in fields with correct created_by user label. - project reconcile rejected with stable noRemoteEquivalent message. - Integration smoke against real *server.Server: create+bulk-update +note → project ready/stale → collection create → library list. Cumulative TASK-968 progress: 33/~50 commands wired across PR #346, #347, this PR. Remaining sections: project standup + changelog (multi-call composition); library activate (model-helper composition); attachments (multipart, separate PR). Parent: PLAN-943. * fix(mcp): preserve all dashboard.attention fields by switching to map-based decoding per Codex review (round 1) Codex caught that projectAttention's typed-struct round-trip dropped the `collection` field from the dashboard's attention entries — and would have dropped any future field additions silently. Same risk applied to projectSuggestion. Fix: stop decoding into a reduced typed struct. The dispatcher now unmarshals dashboard JSON into map[string]any, pulls named arrays via dashboardArrayField helper, and operates on the maps directly through filterAgentAttention's filter+sort. Result: every field the server emitted on each attention/suggestion entry survives to the response, no maintenance burden when handlers add new fields. filterAgentAttention now operates on []map[string]any with typed-string asserts at the comparator. Same filter set (stalled / blocked / overdue / orphaned_task) and same (type, item_ref, item_title) sort order — output ordering still pinned by the existing test. Test added: TestDispatch_ProjectStale_PreservesAllFields seeds an attention entry with every documented field PLUS a forward-compat `future_field` and asserts all of them flow through to the response. That regression-pins the wire-shape forwarder behaviour. Parent: PLAN-943.	2026-05-01 14:09:59 -04:00