55 Commits

Author	SHA1	Message	Date
xarmian	b437cc582d	feat: item reminders — the fire-at-an-instant primitive, and one overdue rule for all four surfaces (IDEA-2641, closes #1010) (#1244) ... * feat(store): item reminders — the fire-at-an-instant primitive (IDEA-2641) Adds the storage, the scheduler tick, and the canonical event for one-shot item reminders (GitHub #1010). Nothing in Pad acted at a target time before this: a due_date makes an item show up as overdue once somebody asks the dashboard, so "revisit TASK-X on the 1st" had to live in an external cron. A TABLE, NOT A SCHEMA-FIELD ANNOTATION. The design sketch proposed marking schema date fields with a `reminds: true` key on models.FieldDef; recon overturned it. Such a key does not survive an ordinary collection edit, two independent ways: the web editor destructures each field into an EditableField and rebuilds a fresh definition key-by-key on save, so unknown keys are dropped (`pattern` and `unique_scope` survive only because two lines were hand-added for them), and models.CollectionSchema has fixed fields with no catch-all, so any Go unmarshal+marshal round-trip strips unknown properties — the hazard retargetRelationFieldsTx mutates raw JSON to avoid. Both failures are silent and both disarm a whole collection's reminders at once. It is the same defect class that moved traits out of the schema column in TASK-2657. The table also gives the lifecycle a home. A reminder is armed, then fired, then acknowledged, and a re-arm returns it to armed — per-reminder state a field definition has nowhere to keep. remind_at is an RFC3339 UTC instant, deliberately not a `date` schema value: those admit both YYYY-MM-DD and full RFC3339 and are compared against the SERVER'S LOCAL calendar day. A fire-at time cannot carry that ambiguity. The remaining timezone question for due_date is filed separately. Firing is one transaction per reminder carrying BOTH the fired_at write and the outbox insert. That pairing is the point: a fired_at committed without its event is a reminder that silently notifies nobody and can never be retried, because the row has left the armed set; an event without fired_at fires every tick forever. The UPDATE's own `fired_at IS NULL` predicate is the arbiter, so two instances ticking at once produce exactly one winner. item.reminder_due is admitted to the closed events/1 set as v1.2, with a new PayloadReminder family and no SSE name. The subject is the REMINDER, not the item: two reminders can be armed on one item, so an item-subject event could not say which fired, and the reminder id is what an acknowledgement addresses. A new payload family rather than reusing the item snapshot for the same reason — a snapshot would validate and still not answer the only question the event exists to answer. No SSE name in v1 because the poll surface is the contract; adding one later is additive, removing one is not. Ack is explicit and nothing else acks. An item reaching a terminal status deliberately does NOT ack: that would make every status write a reminder mutation, and it would silently consume a reminder set to fire after the work was done. * feat(server): reminder surfaces, and one shared overdue rule for all four Second half of IDEA-2641: the HTTP surface, the scheduler tick's wiring, and the fix for the finding that justified the unit — `ready` / `next` did no date handling at all. OVERDUE NOW HAS ONE IMPLEMENTATION. It used to live inline in the dashboard's attention loop, which meant `pad project stale` inherited it (it filters that very list) and the recommendation surface never saw it. So a deadline reached the two surfaces that REPORT on work and never the one an agent PULLS from. overdue.go is now the only place that decides, and all four call it. Two behaviour changes fall out, both deliberate: - An overdue item bypasses the orphan branch's high/critical priority gate. That gate was where a deadline quietly stopped: a low-priority item three weeks late was reported by `stale` and never suggested by `next`. - Overdue sorts above in-progress. The list is capped at three, so a rank below in-progress would not merely order the deadline lower — on any workspace with three things in flight it would keep an overdue item off the surface entirely, which is indistinguishable from not shipping this. The server-local-today comparison is UNCHANGED and known to be wrong for multi-timezone deployments; it is filed as its own item with the cloud case stated. Changing what "overdue" means on every existing instance inside a change about where the rule LIVES is the kind of behaviour change nobody reviews. Fired reminders reach `next` / `ready` two ways, from one filtered list: PendingReminders is the addressable form (it carries the id an ack needs), and a prepended suggestion is the rendered form. They are prepended AFTER the cap rather than entered as ranking candidates — a reminder is not a task competing on priority, and whether it appeared should not depend on how busy the workspace is. Terminal-item reminders are FILTERED from the surface, never acked. Acking on terminal status would couple every status write to reminder state and would consume a reminder armed to fire after the work was done. The row stays exactly as the user left it; the distinction is observable, and asserted. Three guard tests caught this change and each was answered rather than silenced: - The request-body reader guard was right: the handlers now go through decodeJSON, inheriting the NUL refusal and the size cap. - The canonical-events guard was right: item.reminder_due is admitted to the duplicated contract table as SPEC-3 v1.7, with the reminder subject kind and the new payload family. SPEC-3's own text owes the same amendment. - The NUL census asked for a decision on eight new columns. None carries caller text: ids and FKs are server-generated, four are the server clock, and remind_at is now re-parsed and re-formatted in the STORE as well as at the edge — so the stored value is always machine-produced from a parsed time and no caller bytes reach the column. The doc comment that used to say "the caller normalizes" protected nothing. Regenerating the baseline also found that GEN_NUL_BASELINE=1, which the test's own instructions name, was never implemented — the flag did nothing, so the documented path was hand-editing the file. Implemented, so the next reader gets the mechanism the instructions promise. * test(reminders): the lifecycle, the four surfaces, and 22 killed mutants Every test here was designed against a specific mutation and the mutation was RUN. A green suite proves nothing about a suite nobody tried to break, and three of the mutants I first wrote were not experiments at all. Store (10 mutants, all killed): candidate predicate <= flipped to >=; the event emission lifted out of the fire transaction; the fire UPDATE's `fired_at IS NULL` arbiter removed; the RowsAffected check ignored; re-arm clearing fired_at but not acked_at; ack losing `fired_at IS NOT NULL`; the poll surface losing `acked_at IS NULL`; normalizeRemindAt no longer refusing; it dropping .UTC(); GetReminder losing its workspace scope. Surfaces (12, all killed): the priority gate no longer bypassing on overdue; the sort no longer ranking overdue first; attention leaving the shared helper; the reason losing its OVERDUE prefix; the comparison flipped to >; terminal items no longer skipped; terminal reminders no longer filtered; the filter ACKING instead of hiding; reminders appended instead of prepended; the tick running on a far-future clock; ack answering 200 for an unfired reminder; parseRemindAt accepting a bare date. THREE MUTANTS DID NOT COUNT ON THE FIRST PASS and were rewritten. Two failed to compile (`if false` orphaned a variable; deleting a parse orphaned an import) and one had an anchor matching two call sites. A non-compiling mutant emits zero FAIL lines and reads exactly like a surviving one — it invents a hole that is not there — so the harness reports BUILD-FAIL and ANCHOR-BAD as outcomes distinct from SURVIVED. It also restores files from an in-memory copy rather than `git checkout`, which would delete uncommitted work in the tree. ONE MUTANT GENUINELY SURVIVED and the test was at fault, not the mutant: appending rather than prepending reminder suggestions was undetectable because the fixture had a single item, so the reminder sat at index 0 either way. The fixture now fills the three-item cap with in-progress work, where an appended reminder lands fourth and vanishes. Faithful mutant, weak test — checked in that order. The same lesson shapes the four-surface fixture: it is a LOW-priority open orphan, because that is the case the old code handled worst. A high-priority task would have made the ready/next leg pass against the unfixed tree, which is a green that measures nothing. Negative controls throughout: a future deadline is not overdue and does not reach the gate bypass; a tick with nothing due fires nothing; a completed item is neither overdue nor suggested. Without them a helper that reported every date, or a tick that fired everything, would satisfy every positive leg. The lead's pin is asserted in both directions: a fired reminder on a done item is ABSENT from the surface and PRESENT and still unacknowledged in the table. Asserting only the absence would pass against an implementation that consumed the row, which is the behaviour the pin exists to forbid. * feat(mcp): pad_item.remind + ack-reminder, ToolSurfaceVersion 0.28 An agent that can RECEIVE a reminder but not set one has half the primitive. The poll surface is pad_project.next / ready, both long exposed, so reminders already reached agents — what was missing is the other half: deferring a piece of work is exactly the moment an agent knows when it wants to be asked again, and it had no way to say so. Two additive actions, two optional params. Nothing existing moved, so a v0.27 consumer enumerating neither is unaffected — the v0.13 / v0.11 / v0.8 disposition, which likewise wired existing CLI verbs onto the catalog. remind_at REFUSES a bare date rather than reading it as midnight. Worth stating because the `date` schema type accepts YYYY-MM-DD and a caller will reasonably try it here: a bare date names a 24-hour span, and choosing an hour inside it would fire at a time nobody picked. Re-arm and disarm stay CLI-only. Both address a reminder by an id the agent would have to list first, and no listing action exists on this surface — a door with no handle. Adding them later is additive. Five guards had to be taught, and each was answered on its merits rather than excluded: the HTTP parity test (route mappers added, so the actions work on the remote transport rather than being advertised and unrouted), the read-only catalog's cmdhelp fixture and expected cmdPath map, the field- conflict classifier (remind_at / reminder_id are NOT field writers — a reminder is a row in its own table addressed by its own id, so listing them as classified sources would have pointed detectFieldConflicts at something that is not a field source), and the instructions.md / README action tables. That machinery is why the version bump is safe to make now, and it earned its keep on this change: every one of the five failed on the first build after the catalog entry landed. CONVE-23 sweep for prose this falsifies: - SPEC-3 (DOC-2653) amended to v1.7 in the room, recording item.reminder_due with its new subject kind and payload family — the first canonical event with no user mutation behind it, since a scheduler tick produces it. - CLAUDE.md gains the reminder routes, the CLI verbs, and the v0.28 entry. It was also stale at 0.26 with NO v0.27 entry at all: the 0.27 unit swept instructions.md and README.md and missed this file. Both added. - skills/pad/SKILL.md gains the verbs and a routing entry, including the two things an agent will get wrong — the time is an instant, so ask for a time of day rather than picking one, and finishing the item does not acknowledge the reminder. * fix(reminders): codex round 1 — four findings, all real, all with a pin Round 1 found four defects and refuted none of them. Each fix carries a test that fails against the code as it was, and each of those was mutation-checked. **P1 — pending reminders bypassed item-level visibility.** Every other dashboard section reads `allItems`, which the store already scoped to the caller's collections AND their granted item ids. The pending-reminder list is a direct workspace-wide query and inherited none of that, so a guest holding a grant on ONE item could read the refs and titles of every other item in the collection through its reminders — an item-level leak wearing a notification's clothes. Now filtered with the same `isItemVisibleToGuest` call the sibling sections use. The test's two items share a COLLECTION on purpose: a collection-level filter was already applied, so separate collections would have made it pass against the unfixed code. **P1 — soft-deleted items could starve the queue permanently.** Candidate selection ignored `deleted_at`, and `fireOneReminder` rolls back when it finds the item gone — which leaves the reminder ARMED and therefore a candidate again on the next pass. Candidates are ordered oldest-first and bounded by a limit, so enough archived reminders fill every batch and no live reminder ever fires. Silent, too: the tick reports zero fired and looks idle. Excluded in the candidate query rather than skipped downstream, so those rows never occupy a slot; the reminders themselves are kept, so restoring an item restores its reminder with it — asserted, because a fix that reaped them would pass the starvation test alone. **P2 — the pass stopped at the first failing reminder.** The per-reminder transaction exists precisely so one unfireable row cannot hold back the rest, and `return fired, err` made that comment false — with candidates oldest-first, one persistently broken old reminder blocks every newer one forever. Now continues and joins the errors, so a pass that fired seven and failed three reports both halves rather than reading as clean. The loop is split behind an injected seam because a real mid-transaction failure is not reachable from outside: the database refuses the corrupt rows that would cause one (verified — invalid JSON in items.fields is rejected by the schema). **P2 — suggestions dropped the reminder id.** The docs tell an agent to acknowledge what it sees in next/ready, and the payload carried no handle: a stateless poller could read the reminder and had no way to retire it, so it would be shown the same item forever. `DashboardSuggestion` now carries `reminder_id` (omitempty), `pad project next` prints the exact ack command, and the test acks with the id the surface handed out rather than merely checking the field is populated — a wrong-but-present id satisfies equality with itself. Four mutants, four killed; one was rewritten first because its anchor matched two call sites and was therefore not an experiment. * fix(reminders): codex round 2 — four findings, all real **`--rearm` was unusable.** `ExactArgs(1)` forced an item ref that the rearm branch then ignored, so the flag could not be reached without supplying a ref that was silently discarded. Now `MaximumNArgs(1)`, with each mode checked explicitly: a ref is required to arm, and a ref supplied ALONGSIDE `--rearm` is refused rather than ignored — it names an item the reminder may not even belong to, and quietly dropping it is how a user learns nothing about the reminder they just moved. **`unremind --format json` emitted plain text**, breaking the parseable-output contract every sibling command honours. **The MCP `ref` param did not list `remind`.** Agents read that flat description to decide what to send, so an action missing from it is an invalid call waiting to happen. It now also says what `ack-reminder` takes instead, and why: a reminder is addressed by its own id because an item can carry several. **Fractional seconds fired early.** `time.Parse` accepts `09:00:00.900Z` and `Format(RFC3339)` drops the fraction, so it was stored as `09:00:00Z` and fired 900ms BEFORE the moment the caller named — silently, having rewritten their value on the way in. Seconds are genuinely the stored resolution (the column is compared as a string against a whole-second clock, and the tick runs every 30s), so the only question was which way to resolve it, and truncation resolved it the wrong way. `NormalizeInstant` now rounds UP: at most a second of lateness, in exchange for a guarantee that can be stated — a reminder never fires before the instant it was set for. Late is a reminder; early is a wrong answer. Whole seconds round-trip exactly, which is asserted, because an implementation that added a second unconditionally would otherwise pass. Three mutants for this round, three killed (round-up→truncate, round-up→unconditional-add, MaximumNArgs→ExactArgs). Thirty across the unit. Two fixes carry no dedicated test and it is worth being explicit rather than implying coverage: the `--format json` branch on `unremind` is a one-line output change with no server-free way to drive it, and the MCP `ref` description is prose the drift tests do not read — they assert an action is DOCUMENTED, not that a param's sentence lists it. * docs(reminders): the ack id is on the surface an agent polls, not only on the arm response CONVE-23 follow-through on the round-1 fix. Both agent-facing docs told a caller to acknowledge a reminder with the id "returned when you armed it" — true, and useless to the caller that matters: a poller reading next/ready never armed anything. The suggestion now carries reminder_id and `pad project next` prints the exact ack command, so the docs say that instead. The prose was written before the fix existed, which is exactly the case CONVE-23 is about: a change that makes an instruction stale without touching the file the instruction lives in. * test(reminders): bind the tick LOOP to the work, not just the pass (CONVE-19) Every other test in this file calls runReminderTick directly. That vouches for the component and says nothing about whether anything ever calls it — a tick that is never started is indistinguishable, from those tests, from one that is. It is the convention's exact case, and the failure I recorded on my own identity doc three times in one unit: I test the component and not the binding. Driven through the injectable tick channel so the assertion pins a SPECIFIC pass instead of racing a 30-second ticker, and polled to a bounded deadline so a loop that never runs FAILS rather than hanging the suite. Mutant: drop `s.runReminderTick()` from the select and this goes red while every direct-call test stays green. Killed. The idempotence leg exists because a second Start spawning a second loop would leave one running after Stop, making the BUG-842 drain invariant false for this sweeper specifically — the one property a copied lifecycle is most likely to get right by accident and least likely to be checked. The cmd/pad call site (cmd_server.go, alongside StartTokenReaper) stays verified by inspection: a source-scanning guard for it would be an instrument asserting facts about source, which is code with an adversary and not worth it for one line that sits in the middle of five identical neighbours. * fix(reminders): codex round 3 — a deferred reminder fired anyway, and the poll surface was unbounded **A re-arm mid-pass did not stop the fire.** The candidate scan selects an id; before the UPDATE runs, a `--rearm` can move that reminder into the future. Re-arm clears `fired_at`, so a predicate checking only `fired_at IS NULL` still matched — the pass fired a reminder the user had just deferred and emitted its event. The re-arm cannot undo that: it can clear the mark, but the event is already on the outbox and at-least-once means a consumer has seen it. The fire UPDATE now revalidates `remind_at <= nowTS` against the SAME nowTS the candidate scan used. Same-value deliberately: the arbiter and the scan must agree about when this pass is, or a reminder could pass one and fail the other for no reason but clock drift inside a single pass. **The poll surface was unbounded.** Every fired-and-unacknowledged reminder was loaded and turned into a suggestion prepended to a list that is otherwise capped at three, so a workspace with five hundred unacknowledged reminders returned five hundred suggestions — in the dashboard response, the hottest read in the product, growing until somebody acknowledged them. Two bounds, because they are two different guarantees: the query takes a window (default 50, oldest-fired first, so it holds what has waited longest), and the prepended suggestions are capped at 5 so `suggested_next` stays a recommendation rather than a second inbox. The full set stays addressable in `pending_reminders`. Truncation is REPORTED as a boolean, not a count. A count would have to be post-visibility-filter to be true for the caller reading it, and the store cannot compute that — the filter runs per item, above. "There are more than you can see here" is the strongest claim the data supports, so it is the one made. Four mutants; two killed outright, two survived and were run down under CONVE-28: - **Uncapped suggestions survived because the fixture had ONE reminder** — capped and uncapped are the same list at n=1. That is the SECOND time a single-item fixture hid a count-or-order property in this file. Fixture now arms eight; it also asserts all eight remain in `pending_reminders`, so the cap is pinned to the recommendation and not to the data. - **Removing the SQL LIMIT survived, correctly, and the test comment now says so.** The Go slice cap bounds the PAYLOAD; the SQL LIMIT bounds the DATABASE'S work. Only the first is observable at this level — with the LIMIT gone the response is still bounded, while the query silently goes back to materialising every pending row before discarding most of them. That is a memory and I/O property with no assertion available here, so it is stated as a coverage boundary rather than papered over with a green that would not have measured it. * docs(reminders): the fire predicate arbitrates against two actors, not one CONVE-23 inside the file the round-3 fix touched. The comment described the UPDATE as an arbiter for concurrent TICKS, which is what it was written for and is why I did not re-read it when asked whether a user edit could race the pass. It now says what it actually defends against, and names the general shape: an arbiter is only an arbiter with respect to the writers it can see. * fix(reminders): codex round 4 — the round-3 bound recreated the round-1 starvation Round 3 bounded the poll surface. Round 4 caught what that bound did: the query took the first N rows and the dashboard then discarded the ones it could not show — hidden items, unauthorised items, completed items — so N such rows hide a visible reminder behind them indefinitely, with no continuation to reach it. That is the SAME defect I had removed from the fire path one round earlier, reintroduced in the read path within the hour. The general form is worth stating because I clearly did not hold it: **a bounded window is only safe when the discarding happens BEFORE the bound.** Filtering above a limit is a starvation every time, and it does not matter what the filter is for. Two halves, because the two filters are not the same kind of thing: **Visibility is now scoped IN SQL**, using the same collection-id / item-id sets every other dashboard section gets through `allItems` — the same three-way shape as ItemListParams, where holding both collection grants and item grants is an OR. Invisible rows no longer occupy the window at all, which is strictly better than filtering them out afterwards and is what the sibling sections have always done. **Terminality is paged**, because SQL cannot evaluate it — a collection's schema defines which statuses are terminal. The collector refills from the next page when a page comes back short, bounded by a max scan so a workspace full of completed items cannot turn a dashboard read into a table scan. The bound is 10x the window: the common shape fills on the first page, and the pathological shape terminates in a fixed number of indexed reads. Stopping at the scan bound reports truncation, which is honest — there may be more, and we did not look. The empty-scope case is a THIRD state that reads like the second: nil CollectionIDs means unrestricted, a non-nil EMPTY slice means this caller sees no collections. Without an explicit guard they collapse, because the switch matches none of its cases at length zero and adds no clause at all — so "nothing visible" would return the whole workspace. Three mutants, one survived: the empty-scope guard, because no dashboard-level test produces that state (callers that would are refused earlier by workspace access). Faithful mutant, missing test — it now has a direct one, with a sanity leg so a build returning nothing cannot pass it by accident. A guard for a state nothing exercises is exactly the one that rots. * fix(reminders): codex round 5 — the MCP action I shipped did not work over stdio **P1: local stdio MCP `remind` was unusable.** cmdhelp derives positionals by regex from a command's `Use` string, and `<instant>` inside `remind <ref> --remind-at <instant>` matched — it became a second REQUIRED positional, so dispatch failed with `missing required argument "instant"`. The action was advertised on a transport where it could not run. **The MCP catalog's own tests did not catch it, and the reason is the finding.** That suite builds its cmdhelp document BY HAND: I wrote `Args: mkArgs("ref")` in it, so the fixture agreed with what I meant rather than with what the CLI says. Five parity and drift tests passed against a document I authored to match my own intention — the "a test that agrees with whatever the table says is not a test of the table" shape, which the canonical-events test warns about in its own comment two packages away. The new test reads the REAL command tree via cmdhelp.Build, which is the only thing in this repo that can disagree with me about what the CLI declares. **P2: `pad project ready` withheld the ack handle** that `next` prints. Showing a fired reminder on the surface an agent polls while withholding the id it needs to retire it means the same entry comes back on every poll, forever. **P2: suggestions asserted a collection they did not have.** The orphan branch admits ANY collection — its own comment claimed it gated on tasks "mirroring the active-plan branch", and that comment was simply false — while the output hardcoded `Collection: "tasks"` and the reason said "Open task". Pre-existing for high-priority items since BUG-1082; my overdue bypass widened it to any overdue item, which is how it surfaced. Fixed by carrying the item's REAL collection rather than by narrowing the branch: narrowing would silently drop the non-task items this has surfaced for a year, and the defect is the mislabelling, not the inclusion. The false comment is replaced with what the code actually does. The first version of that test used an overdue IDEA and SKIPPED — ideas use `new`, and the branch requires `open` or an active status, so it never became a candidate. A test that cannot fire is a failed reconstruction, not a pass; the fixture is now a bug-like collection whose vocabulary contains `open`, which is the population the defect can actually reach. Three mutants, three killed. Forty-one across the unit. * fix(reminders): codex round 6 — reminders fired from soft-deleted workspaces **P1, and the only defect in this unit whose consequence leaves the process.** Workspace soft-delete deliberately keeps items for the 30-day restore window, so the candidate query's filter on the ITEM's deleted_at found nothing wrong — and the tick kept firing, emitting outbound webhook events for a workspace whose owner had deleted it, possibly while deleting their account. Both queries now join workspaces and require `w.deleted_at IS NULL`. Nothing is destroyed: a restored workspace resumes firing, which the test asserts, because "stops firing" and "is destroyed" are very different answers to someone who restores a workspace and only one of them is right. That test first failed for the WRONG REASON and the fixture was at fault: it counted every outbox row in the workspace, and item creation writes its own, so the assertion was satisfiable by the fixture itself and discriminated nothing. Scoped to the reminder event type. **`Use: "remind <ref>"` declared a requirement the command contradicts.** cmdhelp derives the machine-readable arg spec from that string, and `--rearm` takes no ref — so the published contract said "required" for something optional. The requirement is CONDITIONAL, which cmdhelp cannot express, so the honest declaration is `[ref]` plus the explicit check that names both call shapes. The round-5 test grew a `required` column, which is what makes this observable at all: asserting only the arg NAMES would have passed. **The pad_item tool description omitted both new actions.** The params were declared and the actions dispatched, but the prose an agent reads to decide what a tool can do did not mention them — discoverable only by someone who already knew to look. It now describes both, including the two things an agent gets wrong: remind_at is an instant, and nothing but an explicit ack retires a fired reminder. Three mutants, three killed. Forty-four across the unit. * fix(reminders): codex round 7 — one predicate for the scan and the arbiter Third instance of one class, so this fixes the SHAPE rather than the instance. The class: the candidate scan filters on something the fire transaction does not revalidate, so a change committed between them fires a reminder that no longer qualifies. Round 3 was a re-armed instant. Round 1's soft-deleted item was the same thing caught from the other side. Round 7 is a workspace deleted between the scan and the fire — the round-6 fix added the condition to the SCAN only, and the arbiter went on not knowing about it. Fixing those one at a time is what let the third happen. `reminderFireable` is now a single string that both sites reference: the scan asks it and the fire UPDATE re-asks it, so they cannot disagree, and a fourth condition is one edit in one place rather than two edits someone has to remember are paired. Written as a correlated EXISTS on item_reminders.item_id rather than a JOIN precisely so the identical text is valid in both a SELECT and an UPDATE, and the scan drops its table alias so the two uses are the same characters. What deliberately stays outside it: `fired_at IS NULL` and `remind_at <= ?` live on the reminder row itself, are already spelled identically at both sites, and folding them in would need a parameter order the shared form cannot express. Said in the comment so the omission reads as a decision. Both directions are now tested at the arbiter — a workspace deleted mid-pass and an item deleted mid-pass — because the item case previously relied on the item load coming back nil, and someone simplifying the EXISTS down to the workspace check alone would otherwise still see green. Three mutants, three killed: the arbiter dropping the shared predicate, and the predicate dropping each of its two halves. Forty-seven across the unit. * fix(reminders): codex round 8 — workspace export silently dropped every reminder WorkspaceExport is a hand-maintained field list, so a new table joins it only if someone remembers. Reminders did not: a backup/restore, or a SQLite→Postgres migration via `pad db migrate-to-pg`, dropped every pending reminder with nothing in the destination to show anything had gone. The line that list has always drawn is item-scoped workspace CONTENT (comments, links, versions — exported) versus per-user state (stars, watches — not). A reminder has no user column and hangs off an item, which puts it on the exported side. Stating the rule rather than just adding the field, because the next person adding a table needs to know which side they are on. LIFECYCLE MARKS ARE CARRIED, not reset. A fired-and-unacknowledged reminder is still owed to whoever armed it, so it arrives pending; an armed one whose instant has passed fires once on the destination's first tick, which is what would have happened had the workspace never moved. Re-arming everything on import would invent a schedule the user did not set. NULL rather than empty string for the unset marks — the lifecycle is defined by NULL-ness, and "" would make a never-fired reminder read as fired at "". TestMigratedTablesCoversTheExport caught the second half, which I would have missed: `pad db migrate-to-pg`'s NUL preflight decides what to REFUSE on from MigratedTables, so a table the migration copies and the preflight does not know about is a gap in exactly the guard that exists to prevent one. Added there too, with the reason it can never actually fire — every column is machine-produced, so it is listed for coverage rather than expectation — and the "six tables" prose it falsified is now seven. Two mutants, two killed: export dropping the block, and import discarding the marks. Forty-nine across the unit. * test(reminders): state the fire-path invariant and pin it from the invariant The lead's read on why rounds 4 and 7 were the same class: the fire path had no stated invariant, so each fix defended an instance. This states it, and derives the pin from the paragraph rather than from the bug history. THE INVARIANT: the candidate scan is a hint and may be assumed to prove nothing. Every condition that made a row a candidate is re-asserted inside the transaction that marks it fired, in the same statement that does the marking, so checking and writing are one atomic act. Worded as "the scan proves nothing" rather than as a list on purpose — a list invites the next person to add a condition to the scan and stop, which is exactly what happened four times here. TestFirePathInvariant is the pin: one table, one row per scan-side condition, each invalidating that condition in the window between the scan and the fire and asserting the same three things — nothing fires, no event leaves, the reminder is not consumed. The earlier per-defect tests are folded in as rows; they said the same thing one instance at a time, which is how four of these shipped. Adding a fifth condition to the scan without a row here should feel like an omission. It carries a positive control, because four cases that all assert nothing happens would pass against a build that never fires at all. The matrix immediately falsified a claim in the paragraph I had just written. I wrote that the item load inside the transaction is "for the payload, not for the check"; removing the item half of reminderFireable alone changes no observable behaviour, because the load then returns nil and the deferred rollback undoes the write. Item liveness is defended TWICE and a single-mutant experiment cannot say which guard is carrying it — removing both is what kills the test. Both are kept, the predicate is named as primary (the row never matches, so no write happens at all), and the asymmetry is stated: workspace liveness has no second line, which is why dropping ITS half does fail the pin. Six mutants: five singles plus the pair. Five killed alone; the item single survives by design and is documented as such rather than left as an unexplained green. Fifty-five across the unit. * fix(reminders): codex round 9 — one legacy row could hide every reminder **P1: items.item_number is NULLABLE and I scanned it into an int.** Migration 006 added the column to existing rows, so a pre-numbering item still carries NULL — and scanning NULL into an int fails the Scan, which fails the QUERY, which degrades the whole pending-reminder section. One old row, and the feature is dark for everyone in that workspace. ListWatchesForUser, which this query was modelled on, uses sql.NullInt64 for exactly this column. I copied its shape and dropped the part that handles the column's actual nullability — the same way of being wrong as the round-5 cmdhelp fixture: borrowing a form without borrowing what it knows. The legacy row now carries no ref rather than a fabricated "PREFIX-0", which would name a different item. **P1: export shipped reminders that import could only discard.** The items section filters on deleted_at IS NULL, so a soft-deleted item is not in the bundle and its reminder can never be reunited with it. My comment claimed the item_links rationale — round-trip the raw graph so a restore reunites them — which is true for links and false here, because links keep soft-deleted endpoints in the bundle and items do not. A link is a row ABOUT two items; a reminder whose item is absent is a dangling schedule. **P2: import wrote remind_at raw.** Import is a writer, and a bundle is not necessarily one this server produced — hand-edited, or from another instance. A local offset or a bare date would land in the one column every comparison downstream treats as a UTC instant, firing early, late, or never. It now normalizes like every other door. An unparseable value is SKIPPED with a warning rather than failing the restore, matching the lenient import-side precedent already in this file, and the raw value's LENGTH is logged rather than its content. Three mutants, three killed; two needed rewriting because the single-line form did not compile — reverting the nullable scan also requires reverting the render, and dropping the normalization orphans a variable. PROCESS FAULT, recorded because it makes this round's findings weaker than they look: I edited the tree while this review was reading it — committed the invariant work and ran five mutation experiments, which write and restore source, over the same files. A review binds to the tree it read and I moved it underneath. Every finding above was re-verified against the current tree before being acted on, and the next round runs with no concurrent edits. * fix(reminders): codex round 10 — one orphaned item aborted a whole restore An ORPHANED item — one whose collection is missing from the bundle — still gets an itemMap entry. It has to: the entry is written before the skip because parent resolution inside the same loop reads the map for items it has not reached yet. So `itemMap[x] != ""` is satisfied by an id that names no row, and inserting a foreign key to it fails (SQLite enforces FKs here via the DSN's `_pragma=foreign_keys(on)`; Postgres always does). The pre-existing mapping is the sharp edge. The aggravating half was mine: this loop treated a failed reminder insert as FATAL, where item_links and item_versions both skip, so one orphaned item carrying a reminder rolled back an entire 900-item workspace restore. A reminder is the least critical thing in a bundle and it had the strictest failure handling in the file. Both halves fixed: the loop gates on items that actually landed, and a failed insert warns and skips like its siblings. TWO GUARDS THAT ONLY DIE TOGETHER, and this is measured rather than assumed. Reverting either alone leaves the test green — with the map gate restored the skip survives the FK failure, and with the fatal return restored the gate means the insert never fails. Removing both is what fails it. They are kept as a pair because they defend the same failure at different depths (prevent the bad write / survive a bad write arriving some other way), and the pair is recorded in the code so a future reader does not delete one as dead after watching its mutant survive. Second time this shape appeared today; the first was item liveness on the fire path. The bundle in the test is hand-built, because ExportWorkspace cannot produce an orphan — which is the reason it needed a test. That shape only arrives from a hand-edited or foreign bundle, and surviving those is what import is for. Three mutants: two singles that survive by design, plus the pair that kills. Sixty-one across the unit. * fix(reminders): codex round 11 — four contract slips, one of them another unit's **suggested_next returned up to eight entries against a cap of three.** Round 3 prepended reminders PAST the list's own cap, reasoning they should not compete for slots. Every consumer — the web dashboard, `pad project next`, `pad project ready` — is written for three. Worse, it silently falsified a decision recorded elsewhere: BootstrapDashboard deliberately has no suggested_next_overflow_count BECAUSE this list is capped at three upstream, and its comment names raising that cap as the moment to add one. My change made another unit's reasoning wrong in a file I never opened. The combined list is now trimmed back to three, reminders still leading — a reminder can push a task suggestion out, which is the right way round, and the full set stays addressable in pending_reminders. My first version of that trim used `limit`, which is REASSIGNED above to len(candidates) — so on a workspace whose only entries are reminders it would have truncated to zero, killing precisely the case the surface exists for. Caught by reading the surrounding lines before running anything; it has its own test now. **pending_reminders was uncapped in the bootstrap projection.** BootstrapDashboard embeds *DashboardResponse, so every new field joins the boot payload automatically — here, a window of up to 50, which is the budget PLAN-1410 spent a unit trimming. Capped at 5 with an overflow count, under its own constant rather than borrowing bootstrapAttentionCap: they answer different questions and a future change to one must not silently move the other. **Truncation was reported from the wrong question.** The collector used the store's `more` flag, which answers "is there another PAGE", not "did I read all of THIS one" — so a window filling part way through the final page reported that the caller had seen everything while unread rows sat behind the fill point. The paging bounds are now injectable so the case is testable at all: building it with a window of 50 needs ~75 rows in a specific pattern, with a window of 3 it is four. **Import accepted acked-without-fired**, which is not one of the lifecycle's three states. Such a row fires, is excluded from the pending surface because it is already acked, and can never be acknowledged because AckReminder requires acked_at IS NULL — an event emitted into permanent invisibility. The acknowledgement is dropped and the schedule kept, since an ack of something that never fired means nothing. Five mutants, five killed (one rewritten — removing the flag orphans a variable). Sixty-six across the unit. * fix(reminders): codex round 12 — a read is not a hold; scope the arm; ack from the ack Four P2s from round 12 (two independent runs, both landing on the same line of the fire path), each closed at the layer where it lives: - fireOneReminder pins the item and workspace rows FOR NO KEY UPDATE on Postgres before the arbiter UPDATE. reminderFireable re-asserted liveness at the predicate's instant and nothing held it to the commit instant; under READ COMMITTED an archival could commit in between and the event left the process about a deleted resource. Same idiom and same lock strength as CreateAttachmentForLiveItem; SQLite is excluded by its BEGIN IMMEDIATE, not skipped for convenience. Two PG-only pins verify "blocked" in pg_stat_activity, not by elapsed time; the pin-removed mutant fails both. - CreateReminder asserts "live item of THIS workspace" in the INSERT's own SELECT and returns ErrReminderItemGone otherwise. The table had an FK and no same-workspace constraint; a mismatched pair fed another workspace's title to this one's dashboard and webhooks. Handler maps it to 404. - AckReminder matches every fired row (COALESCE keeps the first ack, updated_at moves only when acked_at does), so a no-match means exactly "not fired at the instant of the ack". The handler no longer decides 409-vs-200 from the row it read before the UPDATE. - The invariant paragraph gains its missing sentence: "at that instant" means the commit instant, and the pin is what makes the predicate's instant and the commit instant the same one. Round-12 caveat carried: both runs were static reads (sandbox blocked Go's build cache), so "four" is a floor, not a measurement. Refs IDEA-2641 * fix(reminders): codex round 13 — a reminder's workspace must agree with its item's, at every read Every reader scoped by r.workspace_id and then joined the item without asserting the two agree. No door writes a disagreeing row today (CreateReminder derives the pair from the item; import maps within the workspace), and the table has nothing that forbids one — so a hand-edited bundle, a future move door, or a direct write would carry one workspace's item into another's dashboard, export, and webhooks. The identity goes into reminderFireable (scan + arbiter), the Postgres row pin, ListPendingReminders and the export query. One test writes the row raw — the only way one can exist — and asserts it is inert at each site; the predicate-removed mutant scans and fires it. Refs IDEA-2641 * fix(reminders): codex round 14 — the by-id and by-item reads assert the same identity as every other read GetReminder scoped by the row's own workspace_id and ListRemindersForItem by item_id alone, so a row whose two columns disagree — the class rounds 12 and 13 closed at the scan, the arbiter, the pin, the pending surface and the export — was still readable through the two reads that reach a single row. reminderOwned is that identity on its own, without the liveness half those two reads must not have (a fired reminder on an archived item is history worth showing). The write paths reach a row only through GetReminder, so scoping it scopes them; a row no door can write needs no door to delete it. ListRemindersForItem now takes the workspace its caller already resolved the item in. The raw-row test asserts both reads refuse the row from both sides; the reminderOwned-removed mutant surfaces it through GetReminder. Refs IDEA-2641 * fix(reminders): codex round 16 — an archived item's reminders are readable, and its verbs say "archived" The doors resolved the item live. Listing an archived item's reminders answered 409 from a GET, and ack/re-arm/delete answered a bare 404 for a reminder that exists on an item that exists — while the store, since round 14, deliberately keeps that history readable. The API already has a posture for archived items: GET reads them, mutations answer 409 "archived … restore it before editing" (writeItemResolveError). The list now follows handleGetItem; the lifecycle verbs load the item include-deleted, run the visibility check first, and then answer the same 409 every other item mutation does. One test walks archive → list 200 / ack 409 / arm 409 → restore → ack 200 on the same rows. Refs IDEA-2641 * fix(reminders): codex round 17 — one suggestion per item, the archived 409 by slug, and the door courtesy named Three findings on the server pass. (1) An item that was both a fired reminder and an ordinary candidate appeared in suggested_next twice; the ordinary entry is dropped, the reminder entry (which carries the ack id) stays, and two reminders on one item remain two entries. (2) Round 16's 409 for an archived item's reminder was written by re-resolving item.Ref, which is derived and empty for a legacy item with no item_number — so the class most likely to be legacy fell through to a bare 404. The slug is handed over instead. (3) The archived check in resolveReminderForWrite is check-then-write, and an archive landing in between lets the verb through: accepted and documented — it is the posture of every item mutation here (UpdateItem's UPDATE has no liveness clause), the outcome is benign, and putting liveness in AckReminder's WHERE would re-create the no-match ambiguity round 12 removed. Refs IDEA-2641	2026-09-04 11:36:11 -04:00
xarmian	63da2f4f5f	feat(store,server,cli): count and repair the legacy NUL population (BUG-2810) ... Layers A and B stop the value being written. Neither makes a row that already carries one go away, and BUG-2810's filing is what that costs: an affected workspace exports with a 200 and re-imports with a 400, so a self-hoster restoring their own backup is blocked with no path forward in the product, and `pad db migrate-to-pg` fails partway through the copy against PostgreSQL's jsonb parser rather than up front. This is DOC-2823's S3, on Dave's day-54 rulings: U+FFFD as the replacement, repair standalone only with a migrate-to-pg preflight that refuses and prints the command, `--repair-nul` on import shipping default-strict. ONE REPAIR, beside the one predicate. textguard.Repair lives next to ParameterRefused because four layers that agree about what is REFUSED and disagree about what a repair PRODUCES is this bug family arriving one step later. Its contract is a property over the same corpus, in both directions: every refused value becomes one all four layers accept, and every accepted value comes back IDENTICAL. The second half is the load-bearing one — a repair that tidies values nobody complained about rewrites `{"a":"x\\u0000y"}`, six literal characters after a doubled backslash, and corrupts it. The JSON arm is a string-literal SCANNER, not decode-walk-remarshal, which is what the recon write-up proposed before it was written. Re-marshalling changes four things nobody asked to change — object key order, insignificant whitespace, integers wider than float64, HTML-ish characters — and silently drops one of a document's LITERAL duplicate keys, which is a gap BUG-2812 owns and the last thing a repair should do. Scanning copies every byte it does not deliberately rewrite, so an untouched document is byte-identical without that having to be argued. A substring replace is not equivalent and the test that proves it took a mutation to find: a doubled-backslash literal ALONE never reaches the scanner, so the discriminating fixture is one document carrying a live escape AND a literal. THE COUNT IS COMPUTED IN GO. Measured on the read path in this worktree: a row planted with `bad<NUL>name` reads back into a Go string with all 8 bytes and the NUL intact, while `length(name)` in the same database answers 3. TASK-2824 found that C-truncation and concluded no DB-side REPAIR could be trusted; the same measurement on the read path says no DB-side COUNT can be either. SQL narrows — `instr(col, char(0))`, plus the escape prefix on JSON-classed columns, which is textguard's own pre-filter — and never decides. The decision stays ParameterRefused with isJSON from the shared 86-column list, i.e. Layer B's classing. Row addressing is read from the live schema rather than a hand-kept map: 39 tables carry protected columns, one (item_wiki_links) declares no primary key and is addressed by rowid, two have composite keys, and five have a single key that is not `id`. The repair checks RowsAffected because an address that stopped selecting its row would otherwise commit an UPDATE that touched nothing and report it as repaired — the one failure an operator cannot see in the output. `email_optouts(email)` is both a protected column and its own primary key. Repairing it changes the row's identity and can collide with an existing row, which in that table means somebody starts receiving mail again. It is reported and skipped, with the reason. The import flag is NOT an exemption from the gate. `--repair-nul` buys the body one repair attempt and then runs the same `bodyDecodesNUL` on the repaired bytes, which still decides — a decode path that skipped the check is the door BUG-2803 spent thirty rounds closing, on the endpoint carrying the largest attacker-controlled body in the product. Only the ESCAPE form is repaired: a raw NUL byte makes the document invalid JSON, and widening what parses is not this flag's job. Both doors are covered, JSON and tar.gz, because giving them different answers is how one of them keeps being forgotten. Postgres is settled with evidence rather than sent up as a ruling: it cannot hold either defect (22021, 22P05) and the four-way differential test already pins that, so the scan reports not-applicable WITH the reason rather than returning a zero a reader could mistake for a clean database. Spellings settled here, per the dispatch: `pad db scan-nul` and `pad db repair-nul` as siblings rather than `repair --nul`, matching `migrate-to-pg`'s hyphenated compound — a repair verb that errors when given no flag is a worse shape, and there is no second repair to share it with. scan-nul IS the dry run, so repair-nul grows no --dry-run. It refuses while the server is running unless --force, on the `pad db restore` precedent: the report is a claim about a database, and one somebody else is concurrently writing makes it a claim about a moment that has passed. docs/backup.md's section on this is rewritten. It still said the rule lives in the binary and not the database, which S2 made false, and it pointed at this item for a preflight and a repair that now exist. Its import examples also showed `pad workspace import < file`, which has never worked — the file is an argument. Closes BUG-2810.	2026-09-02 14:53:31 +00:00
xarmian	ba1255881d	fix(server): refuse a decoded NUL in a JSON request body (BUG-2803) (#1220) ... * fix(server): refuse a decoded NUL in a JSON request body (BUG-2803) The body half of BUG-2782 (path) and BUG-2784 (query). A caller-supplied string reached a Postgres text parameter, Postgres refused it, and the handler answered 500 — the honest answer is 400. WHY THE TRANSPORT RULE CANNOT BE EXTENDED, which is the whole reason this is a different fix rather than a wider middleware. ValidateQuery works because a decoded query value is a substring of the raw query with ASCII substitutions: the bad byte in the raw text IS the bad byte in the value. That property fails for a JSON body — the reachable NUL arrives as the six-character escape, all ordinary ASCII — so no request middleware can find it without decoding the body, which is the handler's job. MECHANISM, each premise measured against encoding/json rather than reasoned about: raw NUL inside a string -> decode ERR (invalid character in string literal) raw NUL after the value -> decode ERR the escape in a value -> decodes to a string CONTAINING a NUL the escape in a KEY -> same a DOUBLED backslash -> decodes to literal text, NO NUL the uppercase spelling -> not a JSON escape at all So the escape is the only vector and its substring is a sound FAST PATH (absent -> no NUL possible), but not a sufficient test: a doubled backslash carries the same six characters and decodes to text. In this product that is not hypothetical — items and documents store markdown, and a document about JSON escapes is an ordinary thing to write. The exact step is json.Decoder.Token(), which returns DECODED strings, covers object keys and arbitrary nesting (an item's fields blob), and needs no knowledge of the destination type. NOT REFLECTION over the decoded value, the other obvious design: it sees []byte fields AFTER base64 decoding, so a body carrying legitimate binary ({"b":"AQAC"} -> bytes 01 00 02) would be refused for a NUL that is not text. A token walk sees the base64 characters. No request struct has such a field today (searched: []byte with a json tag in internal/server and internal/models, non-test — only models.YjsUpdate.UpdateData, which no handler decodes from a body); the token walk is chosen so adding one later cannot silently start rejecting valid requests. BUFFERING IS NOT A COST. json.Decoder.Decode already holds the whole top-level value in memory — refill accumulates into dec.buf and grows it by doubling (encoding/json/stream.go) — so streaming never avoided the copy. Measured on the 64 MiB workspace-import shape, total allocation: stream+Decode 354.7 MiB, ReadAll+Unmarshal 256.5 MiB, ReadAll+Decode 512.5 MiB. Peak heap is order-dependent and does not discriminate; the first run of that measurement showed a 0.77x peak win that vanished when the legs were swapped, so only the allocation figure is claimed. POPULATION, measured on Postgres 17 through the real router with a control leg on every endpoint (92 mutating routes enumerated via chi.Walk; 13 probed): before: 12 of 13 DOOR (control 201 / NUL 500, SQLSTATE 22021) after: 0 of 13 — every NUL leg 400, every control leg unchanged Confirmed doors: workspace name, collection name, item title, item content, item fields value, item title via PATCH, comment body, agent role name, view name, document title, webhook secret, workspace import. workspace-token name is UNMEASURED, not clean — its control leg 500s on an unrelated FK in this fixture. The other 79 routes are unprobed, not claimed clean; the completeness argument is structural instead, and enforced by a test rather than asserted. SECOND DEFECT, named rather than slipped in: the six handlers that decoded straight off r.Body had no http.MaxBytesReader either — the cap decodeJSON has always applied — so each was an unbounded body read. Routing them through decodeJSON closes that too. COMPATIBILITY: json.Unmarshal refuses trailing non-whitespace after the JSON value where Decode ignored it. Deliberate, same direction as this fix, and the only behaviour change beyond the refusal. Trailing whitespace still passes. An EMPTY body still returns a wrapped io.EOF, because handlers_playbooks.go reads errors.Is(err, io.EOF) as "no arguments supplied" — caught by TestPlaybookRunAcceptsEmptyBody, which is exactly the wiring a helper-level change is blind to. No call site changed for the refusal itself: all 65 decodeJSON callers already turn a decode error into a 400 carrying err.Error(). Release note: a NUL character in a JSON request body now returns 400 instead of 500 on Postgres deployments. Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN * fix(server): follow the NUL refusal into JSON-encoded string fields (BUG-2803) Codex round 1 on #1220: the check scanned ONE JSON layer, and several fields cross the wire as JSON-ENCODED STRINGS rather than nested objects — an item's fields, a collection's schema, a workspace's settings. The OUTER decode of {"fields":"{...}"} yields the inner document as literal text, in which the escape is still six ordinary characters and no NUL exists, so the single-layer token walk passed it. MEASURED on Postgres 17 with a control leg on each, after the single-layer check was already in place: item.fields as a JSON-encoded string 500 control 201 collection.schema as a string 500 control 201 workspace.settings as a string 500 control 201 The error is DIFFERENT from the rest of this family, which is why it is worth reading rather than assuming: insert collection: ERROR: unsupported Unicode escape sequence (SQLSTATE 22P05) 22P05, not the 22021 the path and query halves produce. The outer string is pure ASCII so it never trips the text-encoding check; this is Postgres's own JSON parser refusing the escape inside a document bound for jsonb, which cannot represent a NUL. After this change all three answer 400 with their control legs unchanged. THE FIX: when a decoded string is itself a complete JSON object or array — the class this API re-parses downstream — walk it too, to a depth bound of 8. Recursion terminates on its own (each level is a strict substring of the one above); the bound keeps a hostile body from buying many full re-parses, and AT the bound the body is refused rather than passed uninspected, since the escape is known to be present and the walk has stopped looking. WHAT THIS OVER-REFUSES, by design and pinned by a test: the rule is structural, not destination-typed, so a plain TEXT field whose ENTIRE value is a valid JSON document carrying the escape is refused too, even though its column would have stored it. Prose ABOUT a JSON escape does not parse as a bare document, so the case is narrow, and a value of that shape breaks any consumer that parses it. The destination-typed alternative — an allow-list of the fields that arrive JSON-encoded — is exactly correct and goes stale in silence, which is the failure mode ValidateQuery's comment rejects when it explains why per-site query validators could not be written. Tests: nested documents (fields/schema/settings/array/twice-encoded), with controls for ordinary content, a doubled backslash INSIDE the nested document, a string that starts like JSON but does not parse, and prose that merely mentions the escape; the over-refusal pinned as a decision rather than left as an accident; the depth bound; and a wiring leg through the real router on SQLite, where the write would otherwise SUCCEED so a green cannot be the database doing the work. Fixtures build their JSON-encoded strings with encoding/json rather than hand-written backslashes, since the escaping rules are the subject under test. All four new tests fail with the recursion removed. Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN * test(server): build the NUL-bearing timeline fixture through the store (BUG-2803) TestTimeline_NeverEmitsACursorItWouldRefuse built its fixture through the API on a premise its own comment stated: "a structured id comes from the item's fields blob, which nothing validates on write". BUG-2803 made that false — decodeJSON now refuses a body whose strings decode to a NUL, including one nested inside a JSON-encoded `fields` string — so the API can no longer produce the row and the test 400'd on its fixture. Repaired rather than deleted, because the DEFENCE it covers is still live: rows in this shape can predate the rule, and the store has no such check of its own, so a migration, an import or any future non-HTTP writer can still produce one. The timeline must keep refusing to hand out a cursor it would then reject. The fixture now writes the blob directly, injecting the six-character JSON escape rather than a raw NUL — the blob is JSON text and both backends reject a raw NUL in it; the NUL comes into existence when Go DECODES the blob, which is exactly how the timeline ends up with one inside an entry id. The test is not vacuous under the change: it asserts the NUL-bearing id took the positional fallback, so an injection that failed to produce a NUL fails the test rather than passing quietly. This is the CONVE-23 case — a change that falsifies existing prose owes a sweep for that prose. The stale sentence was found by the test failing, not by the sweep, which is the weaker of the two ways to find it. Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN * docs(server): correct the timeline comment BUG-2803 falsified (CONVE-23) The entryID fallback's comment said note and decision ids "come from the item's fields blob and nothing validates them on write". BUG-2803 made that half false: the HTTP API now refuses a request body whose strings decode to a NUL, including one nested inside a JSON-encoded `fields` string. The sentence was true when written and nothing in this branch's diff pointed at it. The fallback still has to exist, and the corrected comment says why: the STORE has no such check, so rows predating the rule — and anything writing a blob by another path, a migration, an import, a future non-HTTP writer — can still carry one. SWEPT AND DELIBERATELY LEFT: two nearby comments (handlers_timeline_id_collision_test.go, handlers_timeline_structured_test.go) also say "nothing validates them on write". Both are about id FORMAT and DUPLICATION — an imported artifact carrying a UUID-shaped id, a hand-written blob repeating one — and this change validates neither. In context those sentences remain true, so they are left alone rather than edited into noise. Sweep command: grep -rniE "nothing validates|not validated on write|no validation on write|unvalidated" --include=*.go internal/ cmd/ — six further hits, all about other subjects (github_pr raw writes, terminal schema keys, push payload format, decodeJSON's size bound). Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN * fix(server): scope the nested-NUL walk to JSON-encoded fields (BUG-2803) Codex round 2 on #1220. The nesting check from the previous commit recursed into ANY string that parsed as a JSON document, on the argument that a structural test beats a destination-typed one. That argument was wrong in a way I had written down as an accepted trade and should have weighed as a defect: a plain-text `content` value holding a JSON snippet that merely MENTIONS the escape was accepted before this branch, is stored in a text column that has no problem with it, and was newly refused — including on RE-IMPORT of an export carrying it. Refusing input the server itself produced is a worse failure than the door the unscoped recursion was closing. Measured before the fix: a workspace whose item content held such a snippet exported 200 and re-imported 400. The walk now descends only under keys whose STRING value is a JSON document something downstream re-parses: config, events, fields, metadata, phase_data, plan_overrides, schema, settings, tags, traits. WHY A LIST IS SAFE HERE, when ValidateQuery's comment rejects exactly this shape for query parameters: there the set of names is unbounded by design (parseItemListParams turns any unrecognised parameter into a field filter), so no list could be complete. Here the set is a closed property of the wire model — a field is JSON-encoded because a Go struct declares it as a string holding JSON — and TestJSONEncodedFieldKeysCoversTheModels derives it from internal/models and fails when a new one appears. The list cannot go stale in silence. Over-inclusion is the safe direction and the list takes it: a listed key that is not really JSON-encoded costs one parse attempt and can only refuse a complete JSON document carrying the escape, while a missing key reopens a door. `traits` is listed for that reason — it carries JSON but its declaration has no comment saying so, which is exactly how the derivation test would have missed it, so the test asserts coverage in one direction only and the list is allowed to be a superset. The walk also changed shape: decoding into `any` and walking the value, rather than a token stream, because key context is needed to know which subtree is JSON-encoded. The []byte reasoning is unchanged and still holds — decoding into `any` never produces a []byte, so a base64 field is seen as its ASCII text rather than as decoded bytes that might contain a legitimate 0x00. Tests: text fields carrying a JSON document are ACCEPTED (five keys), with a leg proving the same document under a JSON-encoded key is still refused, so the pair differs only in the key; the derivation test; and the depth-bound fixture now nests under a JSON-encoded key at every level, since nesting under an ordinary key would never start the recursion and would have passed for the wrong reason. STILL OPEN, and the lead holds it: a LEGACY row whose stored fields blob already carries the escape still exports 200 and re-imports 400. That is data this fix cannot make importable without weakening the write-side refusal, and the disposition (repair sweep, flagged import, or documented acceptance) is a product ruling. Recorded on the item. Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN * fix(server): close the three body doors codex round 3 found (BUG-2803) All three verified before fixing, none taken on the reviewer's word. 1. BUNDLE IMPORT BYPASSED THE REFUSAL (P1). handlers_import_bundle.go parses pad-export.json itself rather than through decodeJSON, so the SAME workspace import — reached with Content-Type application/gzip instead of application/json — walked straight past the NUL check into Postgres. The bundle's export blob is now checked with bodyDecodesNUL before ImportWorkspace, answering the same 400. Test drives a real tar.gz through the router with a clean-bundle control leg, because this path answers 400 for a dozen unrelated reasons (bad gzip, out-of-order tar, duplicate entries) and a bare 400 would prove nothing. 2. ONE CALLER SWALLOWED THE NEW ERROR (P2). handlers_admin.go's test-email endpoint read `if err := decodeJSON(...); err != nil || input.To == ""` and fell back to the admin's own address, so a body carrying a NUL answered 200. An ABSENT body legitimately means "send it to me"; a body that is present and REFUSED is a different thing, and collapsing the two turns a validation error into a success. The two cases are now separated on errors.Is(err, io.EOF). 3. THE COMPLETENESS TEST COULD NOT SEE PAST TWO CALL SHAPES (P2). It scanned for json.NewDecoder(r.Body) and io.ReadAll(r.Body), so it was blind to io.ReadAll(io.LimitReader(r.Body, n)) — a shape ALREADY in the package — and to any alias or helper. A completeness test that misses a live example is worse than none, because it reads as coverage. It now scans for the thing that cannot be spelled around, a reference to the request body at all, and requires every FILE touching one to be accounted for with a written reason. Both directions are asserted: an unaccounted file fails because a door may have opened, and an accounted file that no longer touches a body ALSO fails, so the list cannot rot into stale excuses that quietly cover a future reader. Verified with a positive control (an added body reference in an unlisted file fails) and a negative one (a stale entry fails). FOUND BY THAT WIDENED SWEEP, and fixed here rather than filed: the raw artifact import (POST /workspaces/{ws}/import-artifact) takes TEXT, not JSON, so it never went through decodeJSON and inherited neither the NUL refusal nor the path/query rule — a body is neither. A raw NUL or invalid UTF-8 reached the store and Postgres answered 22021, which the handler turned into a 500 for what is a client error. It now applies bindableText, the same predicate ValidatePath and ValidateQuery use, and answers 400 invalid_body. Note the shape difference from the JSON half: there the ESCAPE is the vector because a decoder rejects a raw NUL; here the RAW BYTE is, because nothing is in the way. Each fix has a mutation run against it: disabling the bundle guard fails the bundle test, disabling the artifact guard fails the artifact test, and both controls still pass. Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN * fix(server): the escape gate was unsound, and YAML has its own (BUG-2803) Codex round 4, two P1s, both reproduced before fixing. 1. THE FAST PATH LET A REAL NUL THROUGH. bodyDecodesNUL gated on "does the raw body contain the six-character escape". That is unsound: the BACKSLASH itself can be written as an escape, so a body carrying \u0000 contains no literal six-character sequence anywhere in its raw bytes, while the OUTER decode manufactures one inside the string — and if that string is re-parsed as a JSON document (jsonEncodedFieldKeys) the second parse turns it into a real NUL. Measured through the real router before the fix: the oblique spelling answered 201 where the direct one answered 400. The mistake was applying a fact about how a NUL is spelled INSIDE a decoded string to the RAW BYTES, where the backslash can itself be an escape. That is the same layer-confusion this whole bug is made of, for the third round running. The gate is now a BACKSLASH. Every JSON escape mechanism requires one, so a body with no backslash has decoded strings byte-identical to its raw bytes, and a raw NUL cannot survive the decoder — no backslash therefore means no NUL, at any depth, however spelled. Bodies WITH one pay for an exact answer, a larger set than before (any nested JSON carries a backslash-quote), which is the cost of being correct. The same correction applies to the per-string pre-filter one level down. 2. YAML HAS ITS OWN ESCAPE VOCABULARY. The raw bindableText check added last commit passes a double-quoted scalar `title: "a\0b"` — no NUL in the request bytes — and the YAML decode manufactures one. Measured before the fix: that artifact imported 201 with a NUL in the item title. The decoded artifact is now checked too: title, body, and every frontmatter field value, walked because a playbook's `arguments` is a nested structure rather than a scalar. Keys are checked as well as values, on the same precautionary grounds ValidateQuery states for query parameter names. Same shape as the JSON half in both cases: a value that is harmless until a SECOND parse, checked at the layer that can see it. Tests: the oblique spelling joins the nested-document table, and the YAML escape joins the artifact table. Each is mutation-verified — reverting the gate to the substring fails the oblique case only, and disabling the post-decode artifact check fails the YAML case only, with the raw-byte cases still killed by the raw check. That per-leg discrimination is the point: it shows each check earns its own keep rather than being covered by its neighbour. Prose corrected where this falsified it: jsonNULEscape's "it is the ONLY spelling" is true of the escape and was being used to justify a filter on the raw bytes, which is a different claim. Both now say so explicitly. Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN * fix(server): multipart text fields and the bundle manifest (BUG-2803) Codex round 4's two P2s. Both are the same shape as the rest: a caller-supplied string reaching a text comparison through a door the earlier fixes did not cover. 1. MULTIPART TEXT FIELDS. The multipart body is deliberately exempt from the JSON rule — its payload is binary blob content and must not be scanned for text validity — but its TEXT fields are a different thing. `item_id` goes to ResolveItem and into a database comparison exactly as the query-string channel does, and that channel has been validated at the transport since BUG-2784; the form channel was not. multipartValues now drops values that are not bindable text, which makes an unusable value indistinguishable from an absent one — the disposition resolveUploadItemID already applies to empty values. The uploaded FILENAME gets the same predicate, with a fallback to a generic name rather than a refusal: the bytes are fine, only the label is unusable. A NEGATIVE RESULT worth recording, because it changed the test: a RAW NUL in the multipart header is NOT the vector. Go's multipart reader refuses it as a malformed MIME header line before any handler sees it (measured: 400, "malformed MIME header line"). The reachable spelling is the RFC 5987 encoded form, filename*=UTF-8''sh%00ot.png, which the header parser accepts and percent-decodes afterwards. The first version of this test used the raw form and was testing a vector that does not exist. 2. THE BUNDLE ATTACHMENT MANIFEST. A second JSON document inside the tar.gz, parsed directly like pad-export.json was, so it needed the same check. Without it a NUL in a manifest string reached rehydrateAttachment, whose failure is logged and SKIPPED — so the import reported success while silently dropping the attachment. The skip-on-failure behaviour is pre-existing and deliberate (a partial restore beats none); refusing the bad INPUT is what stops it being reached this way. Left as it is, and named rather than quietly changed. A VACUOUS ASSERTION THE MUTATION CAUGHT, recorded because the test would otherwise have shipped as coverage: the filename leg first asserted `!strings.ContainsRune(body, 0)` on the RESPONSE, which is JSON — a NUL in the filename comes back as the six-character escape, not as a 0x00, so the check passed whether or not the fix was present. It did pass with the fallback disabled. Now it decodes the response and asserts the replacement name. The item_id leg had the mirror-image weakness: it asserted "not a 500", which is the Postgres-only symptom, so on SQLite it would have passed either way; it now asserts the request behaves exactly like the no-value control. Every fix in this commit has a mutation against it, and each kills only its own leg. Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN * refactor(server): drop the now-unused escape constant (BUG-2803) The gate became a backslash check, which was the last production use of jsonNULEscape; golangci-lint's unused check failed on the next run. Its documentation was load-bearing, so the explanation moved into bodyDecodesNUL's comment rather than being deleted with the variable — including the distinction that made the old gate wrong (the escape has one spelling INSIDE a decoded string, which is not a claim about the raw bytes). Caught by re-running lint on the tip after the previous commit rather than trusting the run from the tip before it. Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN * fix(server): rune-safe truncation and User-Agent sanitising (BUG-2803) Codex round 5 was asked for the POPULATION rather than a confirmation — "enumerate every remaining way a caller-supplied string can reach a database text or jsonb parameter without passing a validity check" — and returned three residual classes with their sinks. Two are fixed here; the third is filed, because measuring it needs a fixture this unit should not grow. 1. TRUNCATION CAN UNDO THE VALIDATION. Four sites cut a caller string with a plain byte slice (name[:120], input.Name[:200]). If the boundary lands inside a multi-byte rune the result ends in a partial sequence and is no longer valid UTF-8 — so a value that PASSED the body check a few frames earlier arrives at the store unbindable, and Postgres answers 22021 for a request the server already accepted. This is the interesting one, because no input-side round could have found it: the defect is downstream of validation, and it is invisible with ASCII fixtures, which is what every test in that area used. truncateBindableText walks back off continuation bytes and drops the straddling rune. Tested with 2-, 3- and 4-byte runes so an off-by-one walk-back cannot pass them all, and with a counterfactual leg asserting the naive slice really does produce unbindable output for the same input — without it the cases would pass against an implementation that did nothing. 2. USER-AGENT REACHES TEXT COLUMNS. It lands in activities.user_agent (three document paths, the connected-apps revoke) and sessions.user_agent (three login paths), and no rule here sees a header. The disposition is SANITISE, not refuse, and that is deliberate: a header is metadata this server chose to record, not something the caller asked for, so a malformed one must not turn an otherwise fine request into a 400. The two sites that HASH the header are left alone — sha256 over arbitrary bytes is well defined, and changing what is hashed would invalidate every stored UAHash. The filing's own earlier probe had recorded User-Agent as NOT reproducing on the item-create path. That was true and did not generalise; these are different sinks. 3. NOT FIXED, FILED: the OAuth form-encoded bodies (/oauth/token, /oauth/authorize/decide, /oauth/revoke, /oauth/introspect) parse url-encoded form data outside the shared body validator, with connection_name reaching oauth_connections.name and client_id reaching the oauth_clients.id lookup. This was the ORIGINAL subject of BUG-2803 before the filing was re-scoped, and it was recorded then as unreachable without a fosite-backed fixture. That is still true, and round 5's sink list is far more than the filing had. Filed rather than guessed at. Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN * fix(server): narrow the gate, stop refusing natural-shape fields (BUG-2803) Codex round 6 plus one measurement of my own. Three changes, one of them a revert of something I got wrong in the previous commit. 1. THE GATE COST TOO MUCH, so it is narrower and still sound. The previous commit gated the walk on "does the raw body contain a backslash", which is correct but catches every body carrying nested JSON (each `\"` is a backslash). Measured on a ~377 KB import-shaped body: 60106 allocs/op with that gate versus 30073 with the walk disabled — the walk was running on ordinary traffic. The gate is now the four bytes that begin any \u escape for a character below U+0100. The argument: to manufacture the six-character NUL escape inside a decoded string, each of its characters arrives either literally from the raw bytes — in which case the raw contains the escape, which begins with that prefix — or from a \u escape of its own, and the three characters involved (backslash U+005C, 'u' U+0075, '0' U+0030) all sit below U+0100, so those escapes begin with it too. Back to 30073 allocs/op, identical to the walk-disabled build. That argument is the same KIND of reasoning that was wrong two rounds ago, so it does not stand on its own: a differential test runs the gated function against an UNGATED walk over a corpus built to attack it — oblique backslash, upper-case hex, an escaped 'u', an escaped '0', a doubled backslash — and fails on any disagreement. It also asserts the corpus contains both answers, since agreement over a one-sided corpus would be vacuous. Reverting the gate to the old substring fails it. 2. THE CHECK REFUSED THE NATURAL SHAPE OF ITS OWN FIELDS. `tags` and `fields` accept both a JSON-encoded STRING and their natural array/object form, and the walk propagated "this subtree is JSON-encoded" into containers — so a free-form tag whose whole value happened to be a JSON document was refused, though nothing re-parses it. Measured: refused before, accepted now, while the JSON-encoded spelling of the same field is still refused. The flag now marks only a direct STRING child of a listed key. 3. REVERTED: I wired the three LOGIN paths to the User-Agent sanitiser last commit, before reading store.CreateSession. It HASHES the header and stores no text — the round-5 enumeration named "sessions.user_agent" and I took the name for a column. The change would have been actively harmful: login would store sha256(sanitised) while middleware_auth still compares sha256(RAW), so every session from a client with a non-UTF-8 User-Agent would fail validation. A sink named in a review is a pointer to verify, not a finding. The real sink is activities.user_agent, from three document paths and the connected-apps revoke. 4. And the wiring leg codex asked for, on that real sink: a request through the router with a malformed header, reading the STORED value out of the activities row, with a control asserting an ordinary header is kept VERBATIM. Unwiring the production call site fails it; the helper's unit test does not notice. Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN * fix(server): apply the key rule at every level, not once (BUG-2803) Codex round 7, both findings, the first confirmed by measurement. 1. THE RECURSION WENT ONE LEVEL TOO DEEP. Once the walk descended into a JSON-encoded string it treated the WHOLE subtree below as JSON-encoded, so a value nested two levels down — an ordinary string inside a `fields` blob that happens to hold JSON text — was refused. That is a false rejection, and the measurement says so plainly. With the depth-2 check disabled, on Postgres 17: depth 1 (the fields blob itself) -> 400 (correct: Postgres parses it) depth 2 (a string INSIDE the blob) -> 201 (accepted, no error) control -> 201 The handler parses `fields` ONCE. The inner text is re-escaped when the blob is written, so what Postgres receives has a doubled backslash and no escape at all. Only the document Postgres itself parses can carry a fatal one. The nested call now passes false rather than true, which makes this a KEY RULE APPLIED AT EVERY LEVEL rather than a depth limit: a JSON-encoded key INSIDE a document still recurses (pinned by a test), an ordinary one does not. Same correction as round 6's natural-shape fix, one level further in — I fixed the sibling case and left this one, which is CONVE-18's lesson about my own enumeration being a sample too. I checked whether anything re-parses a value inside the blob before loosening this, rather than assuming: `arguments` was the candidate, and parsePlaybookArguments asserts it is a native ARRAY (raw.([]any)) rather than a JSON string, so it is covered by the natural-shape rule and needs no second parse. 2. AN ERROR MESSAGE THAT SENT CLIENTS THE WRONG WAY. The OAuth dynamic client registration handler prefixed every decode failure with "Request body must be JSON". A body carrying a NUL is valid JSON, so that message sends a client hunting a syntax error it does not have. The two failures are now distinguished. Round 7 also reports no break in normal CLI, MCP or web-client request generation — they marshal JSON and encode paths and query parameters — which is the first thing any round has said about the client surface. Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN * fix(server): complete the artifact check, make the walk path-aware (BUG-2803) Codex round 8. It confirmed round 7's two fixes, then found two real defects and two inaccurate comments — the comment half being the angle the round was asked for. 1. THE ARTIFACT CHECK MISSED TWO REACHABLE FIELDS. artifactIsBindableText walked the decoded artifact by TYPE, so it never covered Provenance — whose strings are rendered into a Markdown footer appended to the stored content — and never matched Arguments, declared []map[string]any, a concrete slice type the walk's []any case does not match. A YAML NUL escape in either reached storage. It now MARSHALS the artifact and searches the output for the escape encoding/json produces. A type switch over a struct that grows is a list that goes stale in silence; marshalling covers every exported field, including ones added later. The one thing it cannot see is invalid UTF-8 (which marshals to U+FFFD), and it does not need to: step 2 rejects that in the request bytes, and YAML cannot manufacture it from valid input — its escapes name code points, where \0 names a NUL. Both new cases fail with the check disabled; the raw-byte cases still pass, killed by the raw check, so each leg is discriminating. 2. THE WALK WAS NOT PATH-AWARE. A collection may declare a user field literally named `schema` or `tags`. The walk consulted the wire-key list at every level, so `{"fields":{"schema":"..."}}` treated a user field name as a wire key and refused valid text holding a JSON example. The key list is now consulted only OUTSIDE caller data — not under a natural `fields` object, not inside an element of a `tags` array, not inside a re-parsed document. Combined with round 7's fix that makes the descent exactly one level deep BY CONSTRUCTION, which is why the depth counter is gone: with the flag no longer inherited, a bound could never fire, and dead protection reads as protection. The depth-bound test is replaced by one that pins the property directly — an escape IN the parsed document is refused, one BELOW it is accepted, and a wire-key-shaped user field does not restart the descent. 3. THREE COMMENTS CORRECTED, all mine, all of the kind a reader would believe without checking: - MaxBytesReader: Close FORWARDS to the underlying body rather than being a no-op, and with a nil writer there is no automatic 413 — the cap surfaces as a read error the callers turn into 400. Behaviour unchanged; only the claim was wrong. - parseArtifactRequest said "three checks" while implementing five, and its returns list omitted ErrArtifactUnbindableText. Both added by this branch, which is exactly the prose a change is most likely to falsify (CONVE-23). - errJSONBodyNUL claimed all 65 callers surface its message. The STATUS is uniform; the wording is not — several substitute a generic string. 4. And one in a test: the timeline fixture said both backends hold a CHECK constraint a raw NUL violates. items.fields is a plain TEXT column with no CHECK on SQLite. What was OBSERVED is "SQL logic error: malformed JSON"; the likely source is an expression index over json_extract, and that attribution is recorded as NOT verified rather than asserted. Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN * fix(server): a regression this branch introduced, and the same trap again (BUG-2803) Codex round 9. Both findings are mine, one of them a regression from the round-8 restructure two commits ago. 1. THE ROUND-8 RESTRUCTURE REOPENED THE ORIGINAL DOOR. Taking the JSON-encoded branch for a listed key skipped the plain "does this string contain a NUL" check and asked only "does the document this string carries hold an escape". Those are different questions. So {"fields":"a<NUL escape>b"} — a direct NUL in the fields value, the very first case this whole change closed — was accepted again. Both checks now run. The test pins all three legs: a direct NUL in the fields string, an escape inside the fields document, and an ordinary fields string that must still be accepted, so the first two cannot pass merely because everything under a listed key is refused. 2. THE ARTIFACT CHECK FELL INTO THE TRAP IT WAS WRITTEN AGAINST. It searched the MARSHALLED bytes for the escape sequence, and a value holding the six LITERAL characters marshals to a doubled backslash which still contains that sequence as a substring — so valid content was refused. Artifacts are documentation; text about a JSON escape is exactly what one carries. Worse than the bug: the comment I wrote asserted the ambiguity "cannot arise here". It was the same doubled-backslash case bodyDecodesNUL exists to resolve, one function away, and I wrote a sentence explaining why it did not apply instead of checking. The marshalled form is now decoded again and walked with the same machinery — the round trip is what makes every field reachable without a type switch, the walk is what makes the answer exact. Its test asserts literal escape TEXT is accepted in title, body and a field value, with a counterfactual leg asserting a real NUL in each of those places is still refused, so acceptance cannot come from the check doing nothing. Reverting either fix fails its test and only its test. Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN * docs(backup): the one case where an export is not importable (BUG-2803) Codex round 12, an operational pass. It found no migration or config requirement, and two documentation gaps. docs/backup.md promises that application-level export/import is portable across SQLite and PostgreSQL. Since BUG-2803 that has one exception: a workspace whose stored data contains a NUL exports fine and is refused on import. It can only affect data written before the rule existed and only on SQLite, which accepted it — a PostgreSQL instance never stored one. `pad db migrate-to-pg` has the SAME problem and reports it worse: it copies rows directly and never passes through the import guard, so a legacy row fails against PostgreSQL's JSONB parser partway through the copy rather than being refused up front. That is the likelier way an operator meets this, since it is the operation that puts an entire old SQLite database in front of PostgreSQL for the first time. Recorded on BUG-2810, which owns the preflight and repair. Round 12's other finding — that the PR's stated release note covered the JSON 500-to-400 change and none of the rest — is fixed in the PR body rather than in the tree. Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN * test(server): close two blind spots the tests themselves had (BUG-2803) Codex round 13, asked whether the new TESTS are sound. Five findings; these are the two that were self-contained. The other three are recorded on the item with what each needs. 1. THE COMPLETENESS SCAN WAS BLIND TO FORM BODIES. It matched only `.Body`, so FormValue / ParseForm / MultipartForm — which read the request body just as surely — were invisible. It therefore reported full coverage while the OAuth form-encoded handlers were entirely outside its view. Widened, and it immediately failed on handlers_oauth.go, which is the instrument working. That file is now ACCOUNTED FOR AS A KNOWN GAP rather than as safe: the OAuth handlers read form-encoded bodies that no rule in this family covers (the transport rules see the query half of r.Form, not the body half), tracked as BUG-2811 and needing a fosite-backed fixture to measure. The test now STATES the gap instead of being blind to it, which is the difference between a completeness claim and a completeness appearance. 2. THE TRUNCATION TEST ADMITTED AN IMPLEMENTATION THAT RETURNED "". Its assertions were: within the limit, bindable text, a prefix of the input. An empty string satisfies all three. It now also asserts that an input fitting the limit comes back UNCHANGED, and that no more than one rune (4 bytes) is lost to the boundary — so a truncator that drops too much fails, not just one that keeps too much. Both were found by asking whether a broken implementation would pass, which is the question CONVE-12 is about and which I had applied to the production code and not to these two tests. Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN * test(server): the three remaining round-13 gaps (BUG-2803) Codex round 13's other three findings, all of the same shape: a test that would stay green with the production change reverted. 1. THE MANIFEST CHECK WAS UNTESTED. The bundle test built archives containing only pad-export.json, so disabling the INDEPENDENT attachment- manifest check left the suite green. The new test builds a bundle with both entries, differing only in the manifest, so a refusal cannot come from the export half. Verified by disabling each check separately: only the matching test fails, so the two are independently covered. 2. THE TEST-EMAIL CHANGE HAD NO HANDLER-LEVEL TEST. Every existing leg exercised decodeJSON, so reverting handlers_admin.go to default EVERY decode failure to the admin's own address passed them all. The new test drives the real endpoint with a wired mock sender and pins the distinction that used to collapse: an ABSENT body still means "send it to me" (control), an ordinary body still sends (control), and a body that is present and refused answers 400 rather than being reinterpreted as the default recipient. 3. THE MULTIPART LEG CHECKED ONE BYTE CLASS. A filter rejecting NULs while letting malformed UTF-8 through would have passed it. It now drives both, which matters because invalid UTF-8 is the class that reaches Postgres as 22021 on a UTF8 database. Round 13 was asked whether the new TESTS are sound — deterministic, order-independent, and failing on broken code. It reported the fixtures isolated and found five ways they were not discriminating. Two were fixed in the previous commit; these are the rest. Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN * test(server): pin the wiring at every call site, not one (BUG-2803) Codex round 14 confirmed round 13's five, then found the same shape one level out: reverting a SINGLE call site back to the unsafe form left the whole suite green, because the surviving fixtures are ASCII and a helper's unit test does not care who calls it. TestTextSafeHelpersAreUsedAtEveryCallSite asserts the wiring STATICALLY rather than adding a fixture per site (an OAuth connection, a cloud login, four audit paths). A byte-slice truncation of a caller string fails it, and so does a raw User-Agent read outside the exempt set. Both directions are checked: finding none of the SAFE form also fails, so a scan that silently matched nothing cannot pass forever. The User-Agent exemptions carry counts rather than being blanket, so a NEW raw read in an exempt file still fails. All four reads in handlers_auth.go are exempt because they feed a HASH — CreateSession hashes the header and stores no text — and sanitising before hashing would be actively harmful: login would store sha256(sanitised) while the session check still hashes the RAW header, failing validation for every client with a non-UTF-8 User-Agent. middleware_request_text.go's one raw read is requestUserAgent itself. Verified by reverting one truncation call site and one User-Agent call site independently; each fails the test. Round 14's third finding is fixed behaviourally rather than statically, because the static scan cannot see it — handlers_oauth.go is already listed for its form-body reads. TestOAuthRegisterRefusesNULBody drives the real dynamic-registration endpoint with cloud mode and an OAuth server wired, with a control leg registering successfully, and pins both the refusal and the message split: the body IS valid JSON, so the answer must not send a client hunting a syntax error. Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN * fix(server): match wire keys the way the decoder does (BUG-2803) Codex round 16, asked whether this change is consistent with its siblings in the same file and extensible by someone who did not write it. It found a live bypass instead. encoding/json matches an incoming key to a struct field by an exact match first and a CASE-INSENSITIVE one otherwise, so {"Fields":...} and {"FIELDS":...} land in ItemCreate.Fields exactly as {"fields":...} does. The walk looked the key up case-SENSITIVELY, so it skipped the nested document for a body the handler went on to accept, and the database answered the original 500. Measured before the fix: `fields` refused, `Fields` and `FIELDS` accepted. This is the same defect shape as everything else in this unit — a check that agrees with one layer's rules while the layer that actually consumes the value uses different ones — which is why the fix is a PREDICATE rather than a wider map: the map is the vocabulary, and the matching RULE belongs to the consumer. Someone adding a key should not also have to remember to add its spellings. The test drives six spellings including mixed case, with a control asserting an unlisted key stays caller data in any casing, so this is case-insensitive matching rather than matching everything. Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN * fix(server): fold keys the way encoding/json folds them (BUG-2803) Codex round 17, first of five findings. The previous commit fixed the ASCII half of key matching and left the Unicode half, which is this bug's own pattern one more time. encoding/json matches with Unicode SIMPLE FOLDING, not lower-casing. U+017F LATIN SMALL LETTER LONG S folds to 's', so "ſchema" reaches the `schema` struct field while strings.ToLower("ſchema") is unchanged and missed the allowlist — a nested NUL under that spelling reached the handler undetected. Matching is now strings.EqualFold against each canonical key. The test carries both a lower-case fold spelling and an upper-case one alongside the ASCII cases, and keeps its control asserting an unlisted key stays caller data in any casing. The other four round-17 findings are recorded on the item rather than patched here: they are genuine layer disagreements (duplicate keys merging differently in a typed decode than in a map, a scan-failure disposition on inputs the typed decode tolerates, and unknown-field policy) whose fixes are design decisions rather than corrections, and this seat is near its context bar. Each is written up with the measurement it needs. Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN * fix(server): pin that a NUL-bearing manifest refusal keeps the partial workspace (BUG-2803) Codex round 18. The comment on the manifest NUL branch said refusing the input "stops it from being reached this way" and stopped there, which reads as though the refusal undoes the import. It does not. A plain error with a non-nil workspace keeps the partial workspace, exactly as every other manifest failure in this loop does — the rollback branch fires only for *importStatusError, and mid-stream manifest failures intentionally keep what was imported (TASK-896). Returning a rollback-shaped error here would give NUL-bearing manifests different semantics from malformed ones, which is a change to the bundle-import contract rather than a fix to this bug. So the behaviour is unchanged and now DELIBERATE: the comment states it, and the test asserts the persisted state rather than only the HTTP answer. Mutation: routing the branch through *importStatusError makes the refusal roll back, and the new assertion fails naming the release note it would falsify. The pre-existing status/body assertions do not notice. Claude-Session: https://claude.ai/code/session_01AUvLoXsKdS5sdpYju6rj4p * docs(server): record the four map-model disagreements as dispositions, and pin them (BUG-2803) Lead ruling day-68 is land-and-follow: this branch lands on its measured commits, and the token-stream rewrite is the BUG-2812 unit's spec rather than a late restructure of an 18-commit branch under review pressure. That makes the four open findings from rounds 16-17 something to WRITE DOWN precisely, not something to leave in a trail comment. The doc comment on bodyDecodesNUL now carries all four, with the one root cause named: this scan decodes into map[string]any and the typed decode does not agree with that model about keys. Two under-refuse (duplicate-key merge; scan-failure passthrough) and are BUG-2812's spec - both dissolve under a walk that never builds values. Two over-refuse (unknown fields; case-variant duplicates) and are ACCEPTED, because refusing is the safe direction. The asymmetry is stated rather than smoothed over: within the map model, (1) and (4) are one defect seen from two sides and only one of them fails safe. Finding (3) is an observable compatibility change - a forward-compatible field carrying a NUL escape now gets a 400 where it got a 200 - so it goes in the release note as well as here. A qualification only protects where the actor meets it. All four are pinned by a test, measured on this tip rather than carried over from the round-16/17 write-up. The two known-gap legs assert the WRONG answer on purpose: when BUG-2812 lands they FAIL, naming the doc comment and the release note as what to update. Both gap legs carry a premise assertion - the same bodies with the disagreement mechanism removed ARE detected - without which they would pass against a check that detected nothing. Claude-Session: https://claude.ai/code/session_01AUvLoXsKdS5sdpYju6rj4p * test(server): wire release-note item 10 to the router, with its before-state measured (BUG-2803) The disposition test proves bodyDecodesNUL RETURNS true for an unknown field carrying a NUL escape. The release note claims the API answers 400. Those are different claims and only the second one is what an operator or client author reads - CONVE-19, my own convention: a direct-call test vouches for the component, not its binding. Two legs, and the control is the load-bearing one. An unknown field with an ordinary value must still be ACCEPTED, so this pins "refused for the NUL" rather than "refused for being unknown". The handler does not reject unknown fields; if it ever started to, the note's explanation would be wrong while its status code stayed right, and no status-code-only assertion could see that. The before-state is measured rather than asserted from memory. Disabling the check makes the same request answer 201 - which is main's behaviour, since decodeJSONWithLimit there unmarshals straight into the typed value and the key is dropped. So "answers 400 where it answered 200" is a measurement in both directions, not a recollection of one. Claude-Session: https://claude.ai/code/session_01AUvLoXsKdS5sdpYju6rj4p * docs(backup): the NUL rule lives in the binary, not the database (BUG-2803, BUG-2813) Codex round 19, the fresh-angle deploy/rollback/mixed-version pass. docs/backup.md said a NUL-bearing row "can only affect data written before that rule existed, and only on SQLite". The second half is true. The first half is false, and the reason is the interesting part: the guard is in decodeJSONWithLimit, so the invariant is a property of the running BINARY, not of the database. On SQLite any window where an older binary serves the same database can still write one - a rollback after upgrading, a staged rollout with an old and a new instance sharing a database, a second older instance on the same file. The window closes, the guard returns, and the rows are already stored, behaving exactly like genuinely old ones. A rollback is an ordinary operational move, so this is not an exotic path. The doc now states the binary-version dependence, says which dialect is affected and why PostgreSQL is not (it refuses a NUL itself, at every version), and gives the operational answer: drain writes from older binaries before the new one serves, or roll forward rather than back. Store-layer enforcement - so the running build stops mattering - is filed as BUG-2813 rather than added here. It is a dialect-level change and the day-68 ruling on this unit is land-and-follow. The same false implication was carried by the PR's release note calling such a workspace "legacy"; corrected there too. Claude-Session: https://claude.ai/code/session_01AUvLoXsKdS5sdpYju6rj4p * docs(server): cite the ruling in house style, not the team-room day counter (BUG-2803) "lead ruling day-68" is the internal day counter, which means nothing to anyone reading this repo and is inconsistent with every other citation in it - the codebase cites a lead ruling by DATE or by BUG ref, never by day-N. Replaced with the bug ref, which is the part a reader can actually follow. Claude-Session: https://claude.ai/code/session_01AUvLoXsKdS5sdpYju6rj4p * docs(server): drop a commit count I had already measured as wrong, and stop asserting a cause I borrowed (BUG-2803) Two defects in a comment I wrote an hour ago, both of the kind this unit's trail keeps recording. "an 18-commit branch" - the branch was 20 commits at b0192871 when I counted it this session, and is more now. 18 came from the previous checkpoint's own miscount, which I had ALREADY identified and written up before I typed it again here. A number that arrives inside a sentence about something else does not feel like a claim, which is exactly why it survives. The count is incidental to the argument, so it is gone rather than corrected - a figure that has to be maintained to stay true is a liability in a doc comment. "this branch's one regression came from exactly that" - the ruling's reasoning, restated by me as a verified fact. The regression I know about came from wiring a fix off a reviewer-named sink list without reading the mechanism, which is adjacent to "restructuring late under review pressure" but is not the same mechanism, and I did not check whether it is the one the ruling meant. Now attributed to the ruling and stated as its reasoning, with the part I can defend - the review loop finding something in nearly every round indicates a design problem - carrying the argument. Claude-Session: https://claude.ai/code/session_01AUvLoXsKdS5sdpYju6rj4p * fix(server): sanitise the MCP audit tool_name, and correct three claims wider than their evidence (BUG-2803) Codex round 20, asked for a POPULATION rather than a confirmation (CONVE-24). It returned a covered list AND four findings; this commit carries the two that belong to this unit plus the doc corrections. ## The door: MCP audit is a second reader, not a pass-through parseMCPRequestBody runs its OWN json.Unmarshal and binds the decoded method / params.name to mcp_audit_log.tool_name, TEXT NOT NULL. A six-character NUL escape therefore arrives as a real NUL: PostgreSQL refuses the audit INSERT with 22021 - the exact symptom this unit exists to remove - and SQLite stores an unprintable tool name. Nothing upstream catches it; the /mcp transport decodes the JSON-RPC envelope itself rather than through decodeJSON, so the body rule never sees the request. Measured before fixing: the decoded name reached the column intact. This unit's own completeness map had CERTIFIED that reader as safe, on the grounds that "decoding still happens in the MCP dispatcher". That is true and it does not bear on what this middleware persists - a correct description of a mechanism, with no question asked about what it does, sitting in the one artifact whose job is to say the population is covered. Corrected there too. Disposition is SANITISE, not refuse, following the User-Agent precedent from earlier in this unit, and the rule now lives in one extracted helper (sanitiseStoredText) with the reasoning attached: the body rule refuses because the caller asked to store that value; this serves metadata the SERVER elected to record, where failing the write would lose the audit row for precisely the request most worth auditing. Both caller-derived returns are cleaned inside parseMCPRequestBody, so both call sites - the ok path and the denied path - are covered at the choke point rather than at either caller. Both are tested: params.name AND the method path. Mutations un-sanitising each one compile and kill only their own leg. ## Three claims corrected, all wider than their evidence - "all 65 call sites" in server.go: measured 70. Removed rather than corrected, because the number has to be maintained to stay true and says nothing the sentence needs. - docs/backup.md said a NUL "cannot be stored in a text or JSON column" absolutely, two paragraphs above my own text explaining that SQLite accepts one. Now stated as what it is: an application rule Pad enforces on both dialects, which is exactly why it has to be enforced. - artifact_import.go said such a value "cannot be stored under any encoding this product supports". Refuses, not cannot - stating a policy as a capability tells the next reader SQLite enforces something it does not. ## Filed, not fixed BUG-2814 - guarded writes re-emit at-rest NULs (move/copy/restore/ fields-patch), propagating a legacy value to rows that never had one. Distinct from BUG-2813: that one is about writing a NUL while an old binary serves, this is the fixed binary SPREADING one already present. Both dissolve under the same store-layer enforcement, so they are filed to be designed together rather than patched at each of a long and moving list of re-emit sites. Declined: round 20 also reported the release-note assertions as unsupported. They live in the PR body, which a read-only sandbox cannot see - the claim is about the reviewer's visibility, not the diff. Claude-Session: https://claude.ai/code/session_01AUvLoXsKdS5sdpYju6rj4p * fix(server): sanitise before testing for emptiness, so the audit fallback survives (BUG-2803) Codex round 21 ranked this the most dangerous un-probed lens, and it is a boundary my own round-20 fix created. parseMCPRequestBody tested env.Method == "" and p.Name == "" BEFORE sanitising. A value made entirely of NUL escapes is non-empty as decoded and empty once cleaned, so it passed over the fallback and was then blanked - storing an empty tool_name in a TEXT NOT NULL column. That is exactly the silent drop the "(unknown)" / "tools/call" fallbacks exist to prevent; the function's own doc comment says so. Measured before fixing: both shapes returned an empty tool_name. Fixed by ordering rather than by adding guards - clean first, then test - so the invariant is structural instead of something each return has to remember. Same by-construction preference as the symmetric-gate fix earlier in this unit. Worth recording that my first patch was WRONG in a way that compiled: I put the sanitise above the json.Unmarshal that populates env, so the method would always have been empty. Caught by printing the patched function and reading it, not by trusting the script saying "patched". Claude-Session: https://claude.ai/code/session_01AUvLoXsKdS5sdpYju6rj4p * fix(server): classify MCP audit on the raw method, and trim only JSON whitespace (BUG-2803) Codex round 22. Two P2s, both measured before fixing. ## A forgeable audit row - my own regression from the round-21 fix The round-21 change reordered sanitise-before-compare so the fallback would survive an all-NUL value. That reorder made the CLASSIFICATION read the sanitised method, so "tools/<NUL>call" cleaned up INTO the literal "tools/call" and the parser then lifted params.name and hashed the arguments for a method that was never tools/call. Measured: tool_name="pad_item" with a full 64-character args_hash - an audit row indistinguishable from a genuine pad_item call, mintable by anyone who can send a request. Worse than the review described it. Fixed by splitting the two jobs, which were never the same job: dispatch decisions read what the client actually SENT; sanitising is for the value that gets STORED. The round-21 boundary is preserved - a method empty only after cleaning still falls back to "(unknown)". Fixing one boundary and creating another in the same function is worth naming: the reorder was correct for the case it addressed and I did not ask what else read that value. ## Go whitespace is not JSON whitespace The empty-body shortcut used bytes.TrimSpace, i.e. unicode.IsSpace, which strips \v, \f, U+00A0 and more. encoding/json accepts none of them. So a body of just \v trimmed to empty, returned io.EOF, and an EOF-tolerant caller - playbook run treats errors.Is(err, io.EOF) as "no arguments supplied" and runs anyway - took a syntactically invalid body for an ABSENT one. Now trims exactly the four bytes JSON calls whitespace. The test drives both directions, because only the pair discriminates: real JSON whitespace must still shortcut to EOF or the playbook contract breaks, and non-JSON whitespace must not or the divergence survives. Reverting to TrimSpace compiles and fails three legs. Claude-Session: https://claude.ai/code/session_01AUvLoXsKdS5sdpYju6rj4p * test(server): give the walker an independent oracle, not one that shares its code (BUG-2803) Codex round 22, finding 3. TestBodyDecodesNULGateAgreesWithAnUngatedWalk compares the gated function against an "ungated" reference that calls the SAME production valueDecodesNUL. That is valid for what the test claims - it pins the raw-prefix GATE - but it structurally cannot see a defect in the WALKER, because such a defect is present identically on both sides and cancels. That matters here specifically: every walker defect this unit has had lived in traversal, descent, or key matching (rounds 1, 2, 4, 16, 17), which is exactly the part the differential cannot check. Added a second implementation of the contract, written in the test and deliberately not calling the production walker. It shares encoding/json and jsonEncodedFieldKeys; it does NOT share traversal, descent, or key-matching. It is iterative with an explicit stack rather than recursive, so a recursion-shaped bug cannot reproduce in it by accident. Demonstrated rather than argued. With the nested-document descent removed from the production walker - a mutant that reopens the exact door this unit exists to close, and which compiles: differential (gate vs ungated) ok <- blind, as the finding said independent oracle FAIL <- catches it The corpus is also asserted to contain BOTH answers, since two walkers that always answer false agree perfectly. Claude-Session: https://claude.ai/code/session_01AUvLoXsKdS5sdpYju6rj4p * test(server): make the body-reader inventory type-aware, and state what it still cannot see (BUG-2803) Codex round 22, finding 4. The inventory that claims every request-body reader is accounted for was lexical, and wrong in three ways - all in the direction that matters for a test whose job is to say nothing is invisible: - it recognised only the variable names r and req, so a handler holding its request as httpReq or orig was INVISIBLE; - it matched inside COMMENTS, so prose could make a file look scanned; - the manually-listed traits field was already evidence of the model-regex blind spot. My first fix broadened the pattern to any identifier. That was worse, and worth recording: it matched every unrelated .Body field - input.Body in comments, fetched.Body in url import, comment.Body, art.Body, sidecarErr.Body - flagging five files that read no request body at all. The only route to green would have been listing those five as accounted, and an accounting entry HIDES future readers in its file. A false entry is worse than a missing one, so I abandoned that approach rather than tuning the regex. Now keyed on the TYPE via go/ast: collect identifiers declared *http.Request in a function signature, then find reader selectors on exactly those identifiers. Names stop mattering, comments are not in the AST, and .Body on anything else is not a match. Positive control, run rather than argued: a handler taking httpReq *http.Request and reading httpReq.Body is FLAGGED by the new scan, and matched zero times by the old regex. Two limits now stated in the test, because an unqualified completeness claim is exactly how the MCP audit reader got certified safe while persisting a decoded NUL: accounting is per FILE rather than per call site, and only signature-declared requests are seen - one stashed in a struct field or captured by a closure is not a parameter. Claude-Session: https://claude.ai/code/session_01AUvLoXsKdS5sdpYju6rj4p * fix(server): mark a cleaned audit identity, and repair two vacuous tests of my own (BUG-2803) Codex round 23, plus a defect in my own instruments that the mutation matrix found and the tests hid. ## Cleaning is lossy, so a cleaned identity was forgeable Round 22 closed the coarse version: sanitising before classifying let "tools/<NUL>call" become a genuine tools/call. Classifying on the raw method fixed that. But sanitising still COLLAPSES distinct inputs onto one output, so "pad_<NUL>item" stored exactly what "pad_item" stores - same tool_name, same args_hash - and anyone able to send a request could mint an audit row and a Prometheus label attributed to a real call. Cleaning and identity are different jobs. sanitiseStoredTextChanged now reports whether anything was removed, and an identity that only became well-formed by cleaning is marked. The cleaned text is kept, so the row stays diagnosable; the marker keeps it distinguishable. Descriptive text (User-Agent) keeps the unmarked helper - nothing decides anything on it. The parenthesised form is what this file already uses for a synthesised value, and a real method or tool name does not begin with "(", so the marker cannot itself be forged by choosing a clever name. ## Two of my own tests were vacuous, found by a surviving mutant I wrote nul := "\u0000" in the round-21 and round-23 tests, which in Go is the NUL CHARACTER, not the six-character escape text. Those bodies were malformed JSON that encoding/json rejected, so neither test ever reached the path it named. The comment on the line said "the escape, not the character"; the code did the opposite, and the correct form was already three lines away in the round-20 test. Nothing in the test output showed this. It surfaced only because the marker mutation SURVIVED, and because a surviving mutant was treated as a question - does the test not discriminate, or did it not run - rather than as either answer. Both repaired and both now kill their mutants: removing the marker fails with tool_name="pad_item" and a matching 64-character hash; removing the emptiness guard fails with "(sanitised) " instead of "(unknown)". Correction for the record: the round-21 checkpoint said that fix was measured failing before the fix. That measurement used the broken literal. The finding was real and the fix is right, but it is only properly established as of this commit. Claude-Session: https://claude.ai/code/session_01AUvLoXsKdS5sdpYju6rj4p * test(server): use the canonical escNULLiteral helper, not a local literal (BUG-2803) The helper is assembled from bytes precisely so this escape cannot decay into the NUL character it describes, and its comment says so: written as a Go literal it is one backslash away from being the NUL itself. I rolled a local one in three tests anyway, and two of them decayed exactly as that comment predicted - vacuous until the mutation matrix caught them. The safeguard existed, was documented, and I walked past it; using it is the only version of this fix that cannot recur. Claude-Session: https://claude.ai/code/session_01AUvLoXsKdS5sdpYju6rj4p * fix(metrics): bound the cleaned-identity marker as a metric label, and correct a false cardinality claim (BUG-2803, BUG-2817) Codex round 24, which enumerated CONSUMERS of the values this unit changed rather than asking again whether the guard is right. Most of that enumeration came back FINE, which is the useful half; three findings did not. ## The marker must not reach Prometheus as part of a name The cleaned-identity marker is right for the audit ROW - an operator reading one row needs to know which tool it resembles. It is wrong for a metric SERIES: "(sanitised) pad_item" and "pad_item" would be two series per user and per status, for a distinction no aggregate query asks. metricsToolLabel collapses the marked form to the bare marker, so it costs exactly ONE extra label value in total and that value is a constant rather than anything a caller supplies. Two tests, and the second exists because the first is not enough. The direct-call test proves the collapse function collapses. The WIRING test proves the emit path calls it - CONVE-19, my own convention. Measured: with the call removed from recordMCPCallMetrics, the direct-call test stays green and the wiring test fails naming the leaked label. ## A cardinality claim that was never true internal/metrics documented the tool label as "bounded by the catalog (~7 tools today)" with arithmetic resting on that. The value is whatever the caller put in params.name, recorded even for requests that dispatch later rejects, so an authenticated caller can mint a series per request. The comment now says so and points at BUG-2817, filed with the fix shape and the two wrinkles it has to decide - the catalog lives in internal/mcp, and legitimate JSON-RPC methods are not catalog tools. That unboundedness is PRE-EXISTING and not this unit's to fix; bounding the marker's own contribution is, which is why the collapse is here and the rest is filed. Also corrected: I wrote BUG-2815 into two comments before filing, and the filing came back BUG-2817. Predicting an identifier is the same class of claim as predicting a count. Claude-Session: https://claude.ai/code/session_01AUvLoXsKdS5sdpYju6rj4p * fix: keep a storable extension in the filename fallback, and sync the rename draft (BUG-2803) Codex round 24, the two remaining consumer findings. Both trace to this unit, and both are cases where a value was made SAFE without asking what reads it. ## The filename fallback was lossier than its sibling An unstorable upload name became a bare "upload" - no extension - while the empty-name fallback two lines below has always produced "upload.bin". The unusable part of "sh<NUL>ot.png" is the STEM; ".png" is ordinary text, and it is what consumers dispatch on: Content-Disposition, the web download anchor, bundle export naming, and , whose documented contract is handing a path to something that opens files by extension. That command was measurably affected - it treats any non-empty stored name as authoritative, so its MIME-based extension fallback never ran and the temp file was extensionless. Fixed at the source: a storable extension survives the fallback, bounded to 16 bytes so a hostile name cannot smuggle a long tail through. The CLI keeps a defensive extension fallback for any extensionless stored name, which also covers rows written before this. Both directions are tested: "sh<NUL>ot.png" now stores "upload.png", and "shot.p<NUL>ng" - where the EXTENSION is the unusable part - still stores bare "upload". Without the second leg, "keep the extension" could quietly become "keep whatever trails the last dot" and reintroduce the value the fallback exists to remove. Dropping the extension again fails the first leg. ## A rename that could never come clean saveName replaced the app object but never updated the draft, so when the server normalised the name the draft stayed as typed, the equality check never matched, Save stayed enabled, and each press re-sent the same request. The server caps at 120 BYTES via rune-safe truncation while the input allows 120 CHARACTERS, so any multibyte name near the limit diverges. The draft is now assigned the value the server actually STORED rather than compared for length, which stays correct for any future normalisation. svelte-check: 0 errors. Claude-Session: https://claude.ai/code/session_01AUvLoXsKdS5sdpYju6rj4p * fix: reserve the synthesised-value namespace, and stop a fallback carrying an unvetted extension (BUG-2803, BUG-2818, BUG-2819) Codex round 25, which probed whether the values this unit SYNTHESISES can themselves be attacked. Earlier rounds asked whether the guard refuses bad input; this asked what the substitutes are worth. ## A fallback must not carry an extension the product would refuse Preserving a storable extension was right; bindableText was the wrong bar for it. Control characters are valid UTF-8 and not NUL, so they are storable - and they are STRIPPED when the name is written into Content-Disposition. So ".s<VT>vg" passes the extension blocklist, which sees no known extension, and reaches the client as ".svg". attachments.SafeFallbackExtension now requires a KNOWN, ALLOWED extension, so a synthesised name can only carry a suffix the product already accepts on the ordinary path. Tested both ways: an obfuscated .svg and an unknown .foo are both dropped to bare "upload", while .png still survives. That divergence is PRE-EXISTING on the ordinary path, where the caller's name is stored as given and no fallback is involved - filed as BUG-2818 with the fix shape. This change only declines to add a second door. ## A mutation exposed a guard that could not fire I first wrote an explicit alphanumeric loop in that predicate as well. Removing it changed nothing: no key in extMIMEMap contains a non-alphanumeric character, so the map lookup already excluded every obfuscated suffix. Keeping an unreachable guard whose comment claims it stops control characters would have misdescribed which line does the work - so the loop is gone, and TestExtMIMEMapKeysArePlain enforces the property it was relying on. A guard that survives its own mutation is a question, not a clearance. ## The marker was forgeable, so the namespace is reserved Marking only what cleaning changed was not enough. A caller may name a tool "(unknown)" - what the parser returns for a malformed body - or "(sanitised) pad_item", and a genuine request then records the same identity as a substituted one. The older sentinels always had this; the new marker inherited it. A leading "(" is now reserved for values this server synthesises, and any caller value entering that namespace is marked too, so the two never collide. Cost stated: an MCP tool genuinely named with a leading "(" is recorded marked; tool names are identifiers in every catalog this server knows. The principled fix for the whole class is a provenance FIELD rather than sentinel strings in a caller-controlled namespace. That is BUG-2819 - it is a migration on two tables, and the same trick cannot rescue attachment filenames, which are legitimately named with parentheses. Claude-Session: https://claude.ai/code/session_01AUvLoXsKdS5sdpYju6rj4p * test(server): fix the independent oracle, which was wrong in a branch its corpus omitted (BUG-2803) Codex round 26, finding 5, and it lands on the instrument I introduced two rounds ago to check the walker. The oracle descended into a listed key's JSON document whenever it met one - including when that key appeared INSIDE a natural object that was itself under a listed key. Production does not: a natural object or array under a listed key is USER DATA, because the server marshals it and nothing re-parses it, so a listed key appearing inside it is an ordinary field name rather than a document marker. Measured on {"fields":{"schema":"<escape text>"}}: production=false, oracle=true. Production is RIGHT and the oracle was wrong, so had that body been in the corpus the test would have failed and pointed at the production walker. It was not in the corpus. That is the part worth keeping: the test already asserted its corpus was not one-sided - that BOTH answers appear - and that check passed while a whole branch of the contract went unexercised. Both answers appearing is not the same property as every branch being covered, and I had treated it as though it were. Fixed by giving the oracle the same user-data rule, and both bodies are now in the corpus - the natural-object case that must answer false, and its string-valued counterpart that must answer true. Re-verified that the correction did not blunt it: with the production nested descent removed, the oracle still fails. Claude-Session: https://claude.ai/code/session_01AUvLoXsKdS5sdpYju6rj4p * fix: refuse path-component filenames, and single-source the MIME extension table (BUG-2803) Codex round 26, findings 2 and 3. ## ".." is not a filename, it is a path component The server guard listed "", "." and "/" but not "..", which survives bindableText. filepath.Ext("..") is "." - non-empty - so an extension check waves it through too, and a consumer joining it onto a directory gets that directory's PARENT. The CLI builds its temp path exactly that way. Both ends fixed, deliberately independently. The server now rejects any name that is only dots or carries a separator, checked on the trimmed form so "..." and "./" do not each need a case. The CLI sanitises the name it receives regardless: a client that builds a local path out of a remote string should not depend on the remote end having sanitised it, and this CLI talks to whatever instance it is pointed at. Tested with "..", "...", "./" and "a/b", with an ordinary name as the premise leg. Restoring the old narrow guard fails it. ## Two tables for one relationship The CLI kept its own MIME-to-extension table and it had drifted: images and video but not gzip, tar, XML, YAML, TOML, HTML, JavaScript or several documents the server has always allowed. So the extension fallback added in round 24 silently did nothing for exactly the types whose viewers most depend on it. The CLI now delegates to attachments.ExtensionForMIME, and the second table is gone. Measured after: gzip .gz, tar .tar, html .html, js .js, pdf .pdf. The reverse map needs one choice per type where several extensions share one, and those preferences are asserted to name types the forward map actually uses - because the first version listed "text/yaml", which this map does not use (it says application/yaml), so that preference could never fire. Same class as the alphanumeric guard removed in the previous commit, caught the same way. Also recorded against myself: I destroyed both new functions mid-edit by running git checkout on a file with uncommitted work, to "revert an approach". That is a documented trap I have hit before and had written down. The committed function survived; the uncommitted ones did not. Claude-Session: https://claude.ai/code/session_01AUvLoXsKdS5sdpYju6rj4p * test(server): make the body-reader scan scope-aware, wrong in both directions before (BUG-2803) Codex round 26, finding 4. The scan used ONE flat name-set per top-level function, which is wrong in both directions at once: - a function literal inside a handler was scanned with the OUTER function's request names, so an unrelated inner variable that happened to be called r was FALSELY flagged; - a request arriving only as a function literal's own parameter was INVISIBLE, because literals were never given names of their own. A false flag in this test is not harmless. The only way to green is to add the file to the accounted list, and an accounting entry HIDES every future reader in that file - so a false positive here converts directly into a blind spot later. That is the same trap that made me abandon the broadened regex two commits ago. Now walks a SCOPE at a time. Each scope inherits its parent's request names, drops any it shadows with a parameter of a different type, and adds its own. Local aliases (req := r) are picked up as well, since that is an ordinary thing for a handler to do and the alias reads the same body. Three controls, run rather than argued: closure parameter reader -> FLAGGED local alias reader -> FLAGGED shadowed inner variable -> not flagged The first two were invisible to the previous scanner, which never gave literals their own names, and the third is the false positive it produced - both by reading the code this replaces. Limits restated honestly rather than left as they were, since two of them are now closed. Still invisible: a request in a struct field, one from a context, and one whose type reaches http.Request through an alias or embedded field. This matches the literal spelling rather than resolving types; closing those means the type checker, not the parser. Claude-Session: https://claude.ai/code/session_01AUvLoXsKdS5sdpYju6rj4p * fix: stop the reverse MIME map emitting BLOCKED extensions, and close four instrument gaps (BUG-2803) Codex round 27 returned "do not merge yet" with three P1s. All of them are mine, from the previous two commits. ## The reverse map turned a refusal list into a source of extensions extMIMEMap is the FORWARD table used to REFUSE uploads - it deliberately lists .svg, .exe, .com so those extensions can be recognised and rejected. Reversing it wholesale meant ExtensionForMIME("image/svg+xml") answered ".svg", where the old CLI table answered nothing, and names a local file with that. So I closed an SVG door two commits ago and reopened one through the MIME helper. Blocked types now get no reverse mapping at all, and the test asserts it with a premise leg (the map must CONTAIN a blocked type, or the assertion never runs). Removing the exclusion fails naming .svg, .com and .msi. ## The oracle was closer, not identical Production descends only into a JSON DOCUMENT - a string whose trimmed form starts with { or [. The oracle unmarshalled any valid JSON, so a SCALAR under a listed key made it answer true where production answers false. Closer to production is not a usable oracle; only identical is. Aligned, and the scalar case is in the corpus. ## The scan was still not scope-aware, and could now MISS a reader A nested block shared the enclosing name-set, so { r := &http.Response{}; r.Body.Read(nil) } was FALSELY flagged. And the shadowing rule deleted a name rebound to http.Request BY VALUE - which still shares the Body, since it is an interface holding the same reader - so that read became invisible. Blocks are now their own scope and a value request counts. ## The controls I claimed were not in the suite Round 27 was right: I had run them as throwaway probes and deleted them, so nothing held the scanner to them. The scanner is now a package-level helper and TestBodyReaderScanDiscriminates drives it over ten synthetic files - six that must be detected (plain, unconventional name, closure parameter, alias, value copy, form reader) and four that must not (no request, shadowed by a closure parameter, rebound in a nested block, mentioned only in a comment). ## And an over-refusal of my own making The filename guard rejected any dot-only name and anything containing a separator. Only "." and ".." are path components; "..." is an ordinary POSIX filename, and filepath.Base has already reduced "a/b" to "b", so the separator test was dead on this platform and removed rather than left looking load-bearing. Preservation controls now pin that legitimate names survive. Also removed: an unused id parameter on safeLocalFilename. Claude-Session: https://claude.ai/code/session_01AUvLoXsKdS5sdpYju6rj4p * test(server): only a DEFINE can rebind a name, and cover the idiomatic reassignment (BUG-2803) Found by probing my own previous commit rather than by a review round - the first time in this sequence I have caught the adjacent breakage before the next round did. The scope rules deleted a request name on ANY assignment whose right side was not a request identifier. That is wrong in the dangerous direction, and it fires on the most idiomatic line in Go HTTP code: r = r.WithContext(ctx) io.ReadAll(r.Body) // <- invisible to the scan WithContext is a call, so the name was dropped and every later read went unseen. Measured before the fix: MISSED. The correct rule is type-sound. Go is statically typed, so a plain cannot change a variable's type: if it held a request before, it holds one after. Only a DEFINE introduces a new binding that can be something else. So the delete is now gated on token.DEFINE, which is both more correct and simpler than what it replaces. Three controls added, and the two that would have caught this are the ones I had not written: a WithContext reassignment, and readers inside an if body and a for body - the last two because making every nested block its own scope is exactly the kind of change that could have started missing them. Thirteen controls now, six negative. Reverting to delete-on-any-assignment compiles and fails the WithContext leg. Claude-Session: https://claude.ai/code/session_01AUvLoXsKdS5sdpYju6rj4p * fix: make the body-reader scan conservative by design, and reduce filenames cross-platform (BUG-2803) Codex round 28. Two findings, and the first is the fifth consecutive round to find a FALSE NEGATIVE in the same instrument. ## Stop modelling scopes; change the error direction instead Rounds 24 through 28 each found another way the scope-modelling scan missed a real reader: a value-copied request, a plain r = r.WithContext(ctx), a mixed r, ok := ... that reuses an existing variable, and if/for/switch initialisers and case clauses whose scopes it did not model. Each fix closed one case and left another. That is a design telling me something, not a run of bad luck. The two error directions are not symmetric here. A false NEGATIVE hides a body reader, which is the entire thing this test exists to prevent. A false POSITIVE costs one human review and an accounting entry with a reason attached. So the scanner now OVER-APPROXIMATES on purpose: any name bound to an http.Request anywhere in the file counts for the whole file, aliases are followed to a fixed point, and names are never un-bound. Every scope-shaped false negative becomes structurally impossible. The cost is real and is now asserted rather than discovered: two controls that previously expected "not flagged" - a name shadowed by a closure parameter, and one rebound in a nested block - now assert CONSERVATIVELY FLAGGED, so the bias is on the record. Three of round 28's named misses are added as controls and pass: mixed short declaration, switch case, if-initialiser shadow. Sixteen controls, and the accounting test still passes against the real package - so the over-approximation costs nothing today. Exactness needs go/types with a real package load, which is a bigger instrument than this test warrants. The comment says so, and names the signal that would justify building it: an accounted entry whose reason is "the scan over-flagged". ## A filename safe on this OS is not safe on the consumer's filepath.Base is platform-specific, so on Unix it leaves a backslash alone - and the stored name is consumed cross-platform. A Windows client joining a stored "..\evil.png" onto a directory traverses upward. Reduced to the leaf under BOTH separator conventions. This normalises rather than refuses, which is less lossy than replacing the whole name and keeps round 27's point that a backslash is legitimate on Unix. Removing the reduction compiles and fails the new test. Claude-Session: https://claude.ai/code/session_01AUvLoXsKdS5sdpYju6rj4p * test(server): count readers per accounted file, add two missing reader methods, correct a false reason (BUG-2803) Codex round 29, and its central point was aimed at my REASONING, not my code. It was right. ## The over-approximation argument was wrong for per-file accounting I justified a deliberately conservative scanner by saying a false positive costs one review and one accounting entry. That is not what it costs. Once a file is listed, a NEW reader added to it is covered by the existing entry and the test stays green - so a false positive does not cost a review, it permanently blinds the list for that file. My own comment already recorded that hazard two commits earlier, and I argued past it anyway. The fix is to make the entry carry a COUNT of reader expressions rather than a yes/no. Adding a reader to an accounted file now changes the number and fails, so the entry must be re-read and its reason re-justified. It churns exactly when a body reader is added or removed, which is when a human should look. Demonstrated: inserting r.PostFormValue into handlers_tokens.go - an already-accounted file - is FLAGGED. Before this it was absorbed silently. The conservative bias stays, because the false-negative classes it eliminates are real and the count now removes the reason it was expensive. ## Two real reader methods were missing MultipartReader STREAMS the body and FormFile triggers multipart parsing of it. Neither was in the selector list, and FormFile is used in production in handlers_attachments.go - so the list was incomplete against code that exists, not hypothetically. ## A reason in the list was simply false handlers_tokens.go was accounted as "a nil/ContentLength check only - it never reads the body". It guards on those and then calls decodeJSON. A wrong reason is the same defect as a missing entry: both let a reader pass as reviewed. ## And I guessed the counts I wrote plausible numbers for the per-file counts and every one was wrong; the test reported the real ones on its first run. Same habit this branch keeps catching - a figure written from expectation reads exactly like a figure that was counted. They are measured now and the comment says so. Also recorded: my first verification script failed to apply its mutation and still printed a verdict, which I nearly banked. It now aborts unless the mutation is present in the file. Claude-Session: https://claude.ai/code/session_01AUvLoXsKdS5sdpYju6rj4p * fix(attachments,cli): close the closing round's two view defects, correct two overclaiming comments (BUG-2803) The closing enumeration (successor seat, per the lead's convergence ruling) returned two real attachment-view defects and two comments claiming more than their code delivers. Fixed here; the round's two design-scale findings are filed instead (BUG-2820 scanner precision via go/types, BUG-2822 Windows-unstorable filename forms). - Reverse MIME map: four ALLOWED spellings (text/xml, text/yaml, application/javascript, audio/webm) had no reverse extension because no extMIMEMap entry uses them as its value — `pad attachment view` wrote an extensionless temp file for exactly the types the delegation was built to fix. Population measured against the whole allowlist: these four, no more. An alias table closes them; TestEveryAllowedMIMEHasAnExtension asserts the class property over the allowlist (a future allowlist entry with no reverse extension fails), plus alias hygiene (allowed keys only, no forward-derived collisions, alias extensions must map to ALLOWED types so the table can never mint a refused extension). Mutation-verified: removing the alias application fails the test on all four types. - safeLocalFilename: a trailing dot survived every check and filepath.Ext("photo.") is "." — non-empty — so the MIME-extension fallback never fired and the temp file dispatched on no extension. Trailing dots are now stripped (cannot empty the name; dots-only names already returned early). The CLI guard also gains its first direct tests, including the backslash and traversal refusals that previously rode untested. Mutation-verified: removing the TrimRight fails both trailing-dot cases. - Two comment corrections, same defect class the accounting list itself names (a wrong reason reads as review): the handlers_cloud.go entry said bodyHasCloudSecret "restores" the body — it restores the first 64 KiB and drops the tail, a bound that file documents; and the accounting test's header said its scan "cannot be spelled around" while its own KNOWN LIMITS block lists the spellings that get around it (struct field, context value, type alias). The header now matches the limits block. * fix(server,attachments,cli): close closing-round-2's scanner blind spot and four stale comments (BUG-2803) Closing round 2 (successor seat) found no product defects; all four findings were in instruments and comments. Each verified against the code, then fixed: - The alias fixed-point resolved only identifier RHS (`req := r`), so a dereferenced copy (`c := *r; io.ReadAll(c.Body)`) was an invisible body reader — and unlike the disclosed type-level classes, this one was not in the KNOWN LIMITS block. The copy shares the Body (an interface holding the same reader). StarExpr operands now join the alias set; a new control pins the case. Mutation-verified: reverting the StarExpr handling fails the control. The type-level classes (struct field, context value, type alias) remain disclosed and are BUG-2820's territory. - The KNOWN LIMITS block said shadowed request names are "correctly ignored" while the controls deliberately assert they are conservatively OVER-FLAGGED — stale prose from the scope-aware era, falsified by the round-28 conservative flip that never touched those lines. - The reverse-map stability loop compared only PREFERRED entries across rebuilds; it now compares the entire map (sizes and every mapping) against the first build. Boundary stated honestly: every multi-spelling type today is preference-pinned, so the full-map comparison discriminates only when a future non-preferred multi-spelling entry appears — that future entry is what it guards. - Three orphaned/wrong comments: a `mimeForExt` doc block glued above ExtensionForMIME (the function it described is gone); the old hardcoded extension-table doc glued above safeLocalFilename (falsified by the delegation it predates); and two "120 chars" claims where the cap is 120 BYTES rune-safe via truncateBindableText — the consent form's maxlength=120 counts characters, so a multibyte name passes the client and is still truncated server-side, which is now what the comments say. * fix(cli,server,docs): close the attachment-view path escape, and closing-round-3's instrument and prose findings (BUG-2803) Closing round 3 found the branch's first product defect since round 17, in BRANCH-ADJACENT code the round-24 fallback extension work made reachable: the `pad attachment view` id fallback joined the RAW id onto its temp dir, and the client sent the id into the URL path UNESCAPED. An id is a CLI argument, but the documented agent flow harvests it from item content ("pad-attachment:" refs other workspace members write), so a traversal-shaped "id" could re-route the HEAD/GET to a different endpoint whose 200 then vouched for it, and the write escaped the temp dir. Both halves fixed and both mutation-verified through a new command-level test: reverting the fallback sanitize demonstrably wrote OUTSIDE the sandboxed TMPDIR; reverting the PathEscape put a raw "../../" on the recorded wire. - internal/cli: url.PathEscape(attachmentID) at both id-bearing client sites (HeadAttachment, DownloadAttachment — the enumerated population). - cmd/pad: the id fallback runs through safeLocalFilename, generic "attachment" when nothing survives; view's long help no longer claims the filename is used "without rewriting the extension" — it describes the reduction and the MIME-extension append, and says why the CLI is stricter than the server (the name is written to YOUR filesystem). - cmd/pad: attachmentViewCmd gets its first command-level test (CONVE-19 — the helper tests vouched for the component, not its wiring): disposition name, extensionless+MIME append, id fallback, traversal containment with a wire-escaping control, generic fallback. Instrument and prose findings, each verified before fixing: - The KNOWN LIMITS disclosure now names the ordinary alias forms the fixed-point does not walk (var-spec, call-derived, named results, range bindings) — they were in BUG-2820's filing but not in the in-file disclosure, which is what let the round read them as unfiled. The scanner itself deliberately does NOT grow another parser patch; go/types is the filed fix. - TestTextSafeHelpersAreUsedAtEveryCallSite pins EXACT occurrence counts (measured: 1 declaration + 4 call sites each) instead of a >=4 floor a removed call site could hide under. - middleware_mcp_audit: two stacked comment copies rested non-forgeability on "real names do not begin with (" — the exact reasoning round 25 retired; the const doc now points at auditLabel's namespace-reservation rule, which is what actually makes the marker non-forgeable. - docs/backup.md said repair is needed before "the export or migration" goes through, contradicting its own "exports fine" three paragraphs up — it is the IMPORT or migration that fails; the export succeeds either way. * fix(server,cli): decode chunked watch bodies, refuse dot-segment attachment ids, correct two texts (BUG-2803) Closing round 4 found one PRE-EXISTING product defect and one residue of the round-3 fix, plus two wrong texts. Each verified before fixing: - Watch creation gated its body decode on `ContentLength > 0`, so a CHUNKED request (ContentLength == -1) had its body silently DROPPED — the caller's predicate ignored, an unconditional watch created, 200 returned. The population of ContentLength gates in the package is exactly two: handlers_tokens.go already used the `!= 0` form, watches now matches it, with io.EOF tolerated so the documented no-body-is-valid contract holds for an empty chunked body too. Three handler-level tests discriminate the cases; the mutation (condition back to `> 0`) fails the two it should and passes the empty-body control. The accounting instrument then flagged the new `r.Body != nil` reference in the file — its exact job — and the file is now accounted with a measured reader count of 1. - url.PathEscape leaves exact "." and ".." UNCHANGED, so those two ids still reached the wire as live dot segments for a proxy or server to normalize — the escaping added in round 3 did not cover them. Both id-bearing client sites now share attachmentIDPathSegment, which refuses exactly those two values before any request (a real id is a UUID; the refusal cannot fire on one). Mutation-verified: removing the refusal fails the new subtest, which also asserts zero requests reach a recording stub. - The artifact rejection text said "NUL byte"; the same refusal fires for a NUL manufactured by a YAML escape during parsing, where no raw NUL byte exists — now "NUL character", in the handler message and the error var. - A test comment claimed the User-Agent reaches sessions.user_agent as text; sessions store only ua_hash, as the accounting list's own exemption states two hundred lines up. The sentence now agrees with it. * fix(attachments,server): remove a can't-fire MIME preference and a stale filename-guard sentence (BUG-2803) Closing round 5 is down to two P3 comment defects; both verified and fixed: - preferredExtensions "preferred" .md over a .markdown that has never been in the forward map — a line that cannot fire, the exact class this branch's own instruments hunt (the alphanumeric guard, the text/yaml preference, the charset loop). Entry removed; shortest-wins picks .md as the only candidate, unchanged. The preference-hygiene test now asserts every entry has a real competitor (>= 2 forward-map spellings), and the counterfactual — re-adding the entry — fails it. - The upload filename guard still carried round 26's "checking the trimmed form rather than listing spellings" sentence directly above round 27's code that does the opposite (exact "." / ".." comparisons, longer dot runs deliberately preserved). The stale layer is gone; the surviving paragraph already records why. * fix(server,docs): drop a dead test fixture, stop claiming the failing row is named (BUG-2803) Closing round 6 returned one P3 — TestDecodeJSONTrimsOnlyJSONWhitespace booted a full testServer it never used (`_ = srv`), dressing a direct decodeJSON test in router coverage it does not have. Removed. Its enumeration also re-read docs/backup.md against the code: "the failing row is named in the error" is true of neither leg — the import answers 400 naming the RULE it refused on (the NUL check is body-wide and knows no row), and `pad db migrate-to-pg` reports which WORKSPACE's copy failed. The doc now says exactly that, and that locating the value is manual until BUG-2810's preflight lands. * fix(server,cli): retire a stale byte-search claim, close two instrument gaps from closing round 7 (BUG-2803) Round 7 found no production defects; three instrument/comment findings: - artifactIsBindableText's doc comment still asserted the round-8 byte-search approach and that "the ambiguity cannot arise here" — directly above the round-9 body comment recording that assertion as simply wrong and doing the round-trip walk instead. The doc paragraph now describes the round trip and points at the body's history. - TestBodyReaderScanDiscriminates listed MultipartReader and FormFile in the scanner's selector set but had no control for either, so their removal from that list was undetectable. Two controls added. - The attachment-view test proved nothing about the MIME delegation: every case used image/png, which the OLD hand-rolled table also knew, so a stale local table passed. Two cases added — application/gzip (a type round 26 found missing from that table) must gain .gz, and blocked image/svg+xml must gain nothing. Mutation-verified: a stale-table mutant that answers only for png fails the gzip case. (First mutant attempt didn't build — unused import — and was not counted as a detection.) The per-file same-count substitution gap round 7 restated is declined as filed, not fixed: BUG-2820's filing already specifies per-call-site accounting via go/types as the fix that retires the per-file count workaround; the KNOWN LIMITS closing line now carries that ref. * fix(attachments,server): sweep two pre-BUG-2413 disposition comments, pin the manifest refusal to 400 (BUG-2803) Closing round 8 found no production defects; two evidence findings, verified then fixed: - Two comments still described the PRE-BUG-2413 disposition policy: the RenderChip mode doc said the HTTP layer serves every chip inline, and the read-path doc derived Content-Disposition from RenderMode. The live policy is the explicit fail-closed ServeInline allowlist — most chip types are served as "attachment". Both now say so and record the history. - TestImportBundle_RefusesNULInManifest accepted any status >= 400, so the documented 400 could decay into a 500 unnoticed. Pinned to http.StatusBadRequest.	2026-08-30 21:30:19 -04:00
xarmian	e747a1610c	feat(session): registry keyed on the harness session, carrying the agent name; pad session list / prune (TASK-2767) (#1200) ... ## Summary TASK-2767 (IDEA-2750 part 2, with part 3 riding along — the keying fix and the reaping are one mechanism). The local session registry (`~/.pad/sessions`) was keyed on the pid of the `pad session register` subprocess, which is dead before anyone reads the file. One session left a new file per call and its own pid appeared in none of them; the only live identifier was the harness pid a reader could parse out of the socket path's basename. In practice nothing wrote it (zero callers in `plugin/`, `skills/`, or hooks) and nothing read it. Now: - **One record per session, keyed on the harness session pid** — `$PAD_SESSION_PID` (harness-agnostic override), else `$CLAUDE_PID` (verified present in both the tool shell and a live plugin monitor's `/proc/<pid>/environ`), else the calling process. A set-but-invalid value is an error, not a silent fall-through. - **The record carries the agent name** the session's writes are attributed to (`ResolveAgentName`: `.pad.toml agent_name` → `$PAD_AGENT` → detected runtime; `--agent` overrides, `--agent ""` is anonymous), the harness session id, and the messaging socket's identity (inode/device/mtime — the same binding the arm-state file uses). - **One owner-identity type, one verdict.** `internal/cli/session_owner.go`: `SessionOwner` + tri-state `OwnerLiveness` (`alive` / `dead` / `unknown`). `armStateOwnerAlive` is now `OwnerLiveness(...) == alive` with its file contract preserved (socket identity else mtime; headless pid + start token; fail closed). The registry pruner takes the opposite posture on `unknown`: on Windows `pidAlive` reports dead for every pid, and a reaper built on that would delete every live session's record. - **Verbs:** `pad session register [--agent]` (writes/refreshes; prunes dead records), `pad session list [--agent] [--cwd] [--all] [--format json]` (liveness per row, newest first; dead hidden unless `--all`), `pad session prune [--older-than DUR]` (dead always; unknown only under an explicit bound; alive never). Nothing on MCP — host-local filesystem state. - **Who registers:** `plugin/scripts/pad-monitor.sh` runs `pad session register` on start, BEFORE the consent gate — presence is a fact, consent is a grant, and the record is local/0600/never on the wire. - **Legacy v1 files** list as `legacy` rows: owner = socket-basename pid (else registrar pid), liveness by pid only (v1 recorded no socket identity, and the socket-without-identity rule would have judged every legacy record dead while its session ran). A legacy row can say a session exists, never who it is. Lead rulings on the four open decisions, all as built: `agent`/`--agent` vocabulary; no server-presence merge in `list`; register from the monitor script before the gate; wire follow-on (agent name on the stream) filed separately as IDEA-2750 part 2b. One ordering change from the plan's section A: pid precedence is `PAD_SESSION_PID` > `CLAUDE_PID` > self (explicit override beats detection, mirroring `PAD_AGENT` over runtime detection); the plan listed `CLAUDE_PID` first. ## Behaviour changes for existing users of `~/.pad/sessions` / `pad session register` - Registry files are keyed on the **harness session pid** (`PAD_SESSION_PID` → `CLAUDE_PID` → self), not the `pad` command's pid; repeated registrations overwrite one record instead of accumulating. - `pad session register` records the agent name, harness session id and socket identity; stores the **real path** of the cwd; prints a different text line and a different JSON shape (the full `SessionRecord`); and **rejects** an invalid `PAD_SESSION_PID` / `CLAUDE_PID` instead of silently keying on itself. - Existing v1 files are read as `legacy` rows (owner = socket-basename pid, no agent name) and dead ones are pruned by the next register. - The plugin monitor now registers (and prunes) on every start, before the consent gate. - `armStateOwnerAlive` now delegates to the shared `OwnerLiveness`; the consent gate's observable behaviour is unchanged on every platform and key type (codex round 4 traced every caller; matrix M29 pins the socket-keyed mapping). https://claude.ai/code/session_016zc6oxBvpax6Z3iQMsAJno	2026-08-25 15:31:16 -04:00
xarmian	ea139272ce	fix(server,watchevents): shared session presence + honest push acceptance (BUG-2698, BUG-2699) (#1175) ... Two coupled defects in the push path, fixed as one unit because 2699's honest-acceptance signature is the substrate 2698's fix reports through. BUG-2699 — Bus.Publish reports acceptance. The endpoint returned 200 pushed:true for a publish that was dropped, because Publish returned nothing and swallowed every failure. An error is two outcomes and they are kept apart: ErrBusClosed proves nothing was published (503 unavailable, safe to resend), while any other error means UNCONFIRMED — go-redis retries a command whose reply was lost, which is why the publish script already carries a dedupe token — and gets 502 push_unconfirmed, deliberately off the web client's safe-to-resend list. MemoryBus was the worse case, not the exempt one: neither implementation checked `closed`, and the in-process one dropped silently with no log at all. Seven production call sites, not the six the item named; the six best-effort producers discard through one named helper, and an AST-based test fails when a new producer publishes directly. BUG-2698 — RedisSessionPresence. A session-targeted push was resolved against the answering replica's presence registry, and the handler skips the publish when the target is absent, so a POST landing on A for a session held on B dropped the instruction and answered delivered_sessions:0. Fixed at the REGISTRY rather than the gate: a shared registry makes the snapshot right, which makes the picker complete and restores the gate's original premise, so the existing skip becomes correct for the reason it was written. Entry and index are written atomically under a TTL renewed by a goroutine that lives exactly as long as the connection; a crashed process stops renewing and Redis clears it. Staleness is unchanged and now stated in full: ~30s for a dropped client, ~90s for a dead instance. delivered_sessions becomes nullable — null means published-but-uncountable, never zero — documented as three states at every consumer. 35 Codex review rounds. Notable: a per-user registry cap was added and then removed after three consecutive rounds found defects inside it and a fourth was asked whether it belonged in this PR at all; a context bound was documented, disproved by its own test (go-redis does not apply a command context to connection establishment — 5.0s measured against a 150ms ctx), and rewritten to say what is true. Every fix was mutation-checked; one instrument was deleted for passing on broken code and one for not asserting its own premise. Filed rather than folded in: BUG-2724 (Redis keyspace namespacing + Cluster), BUG-2725 (delivered_sessions is an estimate with error in both directions), BUG-2726 (no concurrent-connection limit on the watch stream), BUG-2727 (Redis absent from readiness/metrics; silent subscriber loss), BUG-2728 (epoch-reset resume lead). Gates: build · make lint 0 issues · go test ./... (25 pkgs) · svelte-check 0 errors · vitest 1738 passed · CI 7/7 including Go (PostgreSQL) and Nix.	2026-08-21 20:43:20 -04:00
David Barkhausen	5784d907c0	feat(cli): PAD_TOKEN environment override for stored credentials (#879) (#1160) ... * feat(cli): PAD_TOKEN environment override for stored credentials (#879) Layer 1 of #879: if PAD_TOKEN is set, the CLI uses it as the bearer token and skips the credential-store lookup — gh's GH_TOKEN convention. Reads never write credentials.json, so a read-only override sidesteps the multi-agent identity contention completely; the store is never touched under the override. Per the acceptance grounding notes: - NewClientFromURL resolves PAD_TOKEN before the per-server store lookup (the single token-attachment chokepoint). - whoami no longer lies under the override: it skips the store short-circuit and reports the effective identity via a real /me fetch, with an 'Auth: PAD_TOKEN environment override' line. - auth login/logout print a gh-style stderr notice when the override is active. logout additionally pins its server-side session invalidation to the STORED token — an unpinned Logout() after the constructor change would have invalidated the env token's session — and skips the server call when there is no stored session. - pad init's status line and server info's report disclose the override (env_token_override field; the auth probe uses the token every other command would use). Zero behaviour change when PAD_TOKEN is unset. Token minting stays web-only; a minimal 'pad token' CLI is offered as a follow-up. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(cli): review round 1 — init fails on a rejected PAD_TOKEN; login shortcut skipped under the override; logout asymmetry documented Per the PR #1160 round-1 review: - Bug 1: pad init's auth step no longer falls back to stored credentials when a set PAD_TOKEN is rejected — it fails with the distinct rejected-token message (mirroring whoami), which also makes the status line's override disclosure truthful. Test drives the real padInitCmd flow and asserts the stored identity is never consulted. - Bug 2: login's 'Already logged in as <stored user>' shortcut is skipped when the override is active — it reads the store, and firing it right after envTokenNotice contradicted the notice. A second test pins the unchanged no-override shortcut behaviour. - Doc ask: the deliberate logout asymmetry (the env token's own session is never invalidated; its lifecycle belongs to the minter, GH_TOKEN posture) is now stated in env_token.go's doc comment and the README PAD_TOKEN section. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>	2026-08-20 07:54:13 -04:00
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	bc68b84848	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	2026-08-19 08:13:29 -04:00
xarmian	e40df6b31c	feat(cli): pad session arm/disarm/status + consent config resolution (PLAN-2613 S2, TASK-2617) (#1149) ... * feat(cli): pad session arm/disarm/status + consent config resolution (PLAN-2613 S2, TASK-2617) The S2 CLI contract S3's plugin skills and S4's web composer build against. S1 gated push delivery on a server-side armed bit declared at stream connect; nothing decided WHETHER to arm or sent the declaration. S2 adds both, defaulting off everywhere. - ResolveAutoArm (internal/cli/arm_consent.go): pure consent resolver. .pad.toml [push] auto_arm is the only per-repo enabler (D4); a per-user config auto_arm=false vetoes it (deny-wins); default off. Config surfaces: PadToml.Push.AutoArm + config.Config.Push.AutoArm (*bool, unset != false), both nil-safe. - Wire contract: StreamSessionIdentity.Armed sends ?armed=true on the event stream — S1's server gate finally has a sender. The monitor announces armed = live local arm OR resolved auto_arm, so a repo opt-in works end to end with a safe default-off skew. - Verbs pad session arm/disarm/status: arm/disarm manage a per-session local arm-state file; status reports the resolved local/auto decision plus the server's own armed/connected counts (new Client.ListSessions), degrading gracefully when padd is unreachable. - Arm-state file (session_arm_state.go): keyed per session by CLAUDE_CODE_MESSAGING_SOCKET (cwd fallback for headless, secondary to auto_arm). Mandatory liveness — a dead-owner file (socket vanished / pid gone) reads as disarmed and is reaped, so a crashed session can never arm a future monitor. Local client state only; the server's armed bit stays the sole delivery authority. Claude-Session: https://claude.ai/code/session_017jD6t1zjxGSq47SQpZfp1V * fix(cli): address Codex R1 on push-consent (fail-closed config, owner-identity liveness) - HIGH-1: user config.toml read now fails CLOSED. config.LoadPushConfigAutoArm reads the [push] auto_arm value strictly — absent → no opinion, but present-but-unparseable → error — and ResolveAutoArmFromDisk refuses to auto-arm when it can't confirm the user's veto (was: swallowed by the lenient config.Load and treated as no-opinion). - HIGH-2: arm-state liveness now checks owner IDENTITY, not just presence. Socket-keyed files record the socket's mtime and require an exact match, so a reused socket path can't revive a stale file. Headless files record a Linux /proc start-time token (portable fallback documented) to reject a reused pid. - MED-1: arm-state writes are atomic (temp + rename) and reaping is non-destructive (re-checks staleness before removing) — a concurrent re-arm is never clobbered. - MED-2: pad session status applies the .pad.toml URL override, so it queries the same server the monitor connects to. - LOW: malformed arm-state files are now reaped (safe now that writes are atomic — a corrupt file can't be a torn in-progress write). Claude-Session: https://claude.ai/code/session_017jD6t1zjxGSq47SQpZfp1V * fix(cli): address Codex R2 on push-consent (atomic config write, stronger owner identity) - HIGH-1: Config.Save() is now atomic (temp + rename), so a monitor reconnecting while `pad configure` rewrites config.toml can't read a truncated/partial file, miss a [push] auto_arm=false veto, and arm. - finding 2: socket owner identity now uses inode+device (unix) as the primary signal, with mtime as the non-unix fallback — a rebound socket or a lingering stale node at the same path gets a new inode and is rejected, closing the mtime-collision / reused-node gaps. - finding 3: headless liveness fails closed when a proc-start token was recorded but can't be re-verified (was: fell back to bare pid-liveness, which a reused pid passes); zombies (state 'Z') now report not-alive. - finding 5: `pad session status` applies an explicit --url override too, not just the .pad.toml one. - finding 4 (connect-time TOCTOU): documented as an accepted, bounded residual — a disarm racing an in-flight connect is corrected on the next reconnect; fully closing it needs S3's server-side disarm-on-open signal. Claude-Session: https://claude.ai/code/session_017jD6t1zjxGSq47SQpZfp1V	2026-08-17 22:44:31 -04:00
xarmian	625cab9984	fix: bound item history and stop resolving bodies nobody reads (BUG-2608) (#1147) ... * fix: bound item history and stop resolving bodies nobody reads (BUG-2608) Item history was unbounded on every surface, and summary mode paid for what it discarded: the endpoint resolved EVERY version by walking the item's whole reverse-patch chain, and both the CLI and the MCP dispatcher then projected that away to metadata. An item edited under collab records a version every few seconds while someone types, so this is routinely hundreds of full-content reconstructions per history call, for output that shows none of them. Two independent fixes, because they address different costs. SUMMARY SKIPS THE WALK. `?summary=true` returns metadata from the raw rows and never resolves a patch. That is the dominant win: the resolution was pure waste for every caller except --full. Content and is_diff are cleared TOGETHER — an empty body still claiming to be a reverse patch would tell a consumer to resolve something that is not there. LIMIT BOUNDS THE WINDOW, newest-first. That direction is not a preference: with reverse patches, reconstructing any version means walking back from current content, so a newest-end window is the cheap prefix of that walk while an older one still pays for everything above it. That is also why there is deliberately no offset — it would advertise a pagination whose later pages cost the same as no bound at all. Absent limit stays UNBOUNDED on the endpoint, following the item-list precedent (maxItemListQueryLimit: "a zero/absent limit is left unbounded — this only clamps an explicit oversized request"). The defaults live on the CLIENTS, where a token budget is actually known: `pad item history` defaults to 50 with --limit to change it, and the MCP catalog action injects 50 (max 300, the same pair list and backlinks already use). A server that truncates a request nobody bounded is a silent-truncation trap for third-party API consumers. The MCP default goes in the CATALOG action rather than either dispatcher, so it reaches BOTH transports — HTTP reads it off the input, and stdio receives it as the CLI's new --limit through BuildCLIArgs. ToolSurfaceVersion 0.20 -> 0.21 with a changelog entry, plus instructions.md and README, per the 2304-family contract discipline. Additive param bump: `limit` already existed, nothing changed shape, and a v0.20 consumer that sends no limit now gets the newest 50 instead of all — which is the fix, not a break in it. The restore and single-version-expand paths still resolve the FULL chain, and a test pins that: bounding their walk would strand exactly the old versions those paths exist to reach. Eight mutations, each failing only the leg it targets. Three fixture problems surfaced that way and are worth naming, because each made a test that could not fail: - force_version in a PATCH body does nothing (`json:"-"` on ItemUpdate), so the throttle collapsed six edits into one version; varying the source per edit is what actually records them. - an 8-byte body is cheaper stored whole than as a patch, so no version was ever is_diff=true and the is_diff assertion was inert. The fixture now uses a body large enough that the store really stores patches. - the cmdhelp test fixture lacked the new --limit flag, so BuildCLIArgs silently dropped it. Verified against the REAL cmdhelp tree that the flag is present and typed int, so the fixture mirrors the CLI rather than flattering it. * docs: bring CLAUDE.md to v0.21 and name why the two result caps differ (BUG-2608) Codex round 1, both findings. CLAUDE.md still described the MCP surface as v0.20 — stale because of my own bump, in the document every agent working this repo reads first. README and instructions.md are held to the version by a test; CLAUDE.md is not, which is exactly why it drifts. The cap "mismatch" (MCP max 300, endpoint clamp 500) is deliberate layering, not an oversight — item lists have the identical split (300 in the catalog, 1000 at the endpoint) because the two answer different questions: an agent token budget is only knowable in the catalog, while the endpoint's clamp is a server-resource ceiling on what any caller may ask for. But nothing said so anywhere, so a reader comparing the two numbers had no way to tell design from accident — which is precisely the report Codex filed. Now stated at the constant and in CLAUDE.md, including why the versions ceiling is LOWER than the list one (resolving a version can cost a patch application per row, not just a row read) and why an absent limit is left unbounded at the endpoint. * fix+test: honest truncation notice, armed fixtures, and the residual named (BUG-2608) Codex round 2, both findings, and the second is the more useful one. CLI TRUNCATION NOTICE was wrong in both directions: it compared the response length against the requested limit, so an item with exactly N versions was reported as truncated, and a --limit above the server's ceiling was clamped there and reported as complete. It now asks for ONE MORE row than it shows and reports truncation only when that extra row comes back. The one case this still cannot detect — an ask above the server's own ceiling, where the probe row is clamped away with everything else — is stated in the code rather than papered over by hardcoding the server's constant in the CLI. UNDER-ARMED FIXTURES. The unbounded test seeded 5 versions, so a server quietly defaulting to 50 would have passed the assertion that denies exactly that; it now seeds 60. The clamp test seeded 2 and could not observe a clamp at all; the clamp is now asserted directly against a parseItemVersionsLimit function extracted for the purpose, over the inputs a URL can really carry (absent, 0, negative, unparseable, either side of the ceiling). That extraction replaced my own first attempt, which was worse than no test: it re-implemented the clamp arithmetic in the test body and asserted the result against itself. It could not have failed. THE RESIDUAL, NAMED RATHER THAN IMPLIED. Codex's sharpest point is that the summary tests cannot detect "resolve everything, then clear the fields" — verified by mutation: pointing the summary branch at the resolving reader leaves every handler test green, because the response is byte-identical either way. So the performance claim does not rest on them. It rests on the handler's summary branch calling ListItemVersionsPage (one reviewable line) plus a new store test proving that reader really returns unresolved rows rather than quietly resolving them — mutation-verified from the other side by making the resolver a passthrough. The test file says all of this, including that an end-to-end assertion would need a patch-application counter in the production path, and why that is not worth it when the cost of being wrong is performance rather than correctness. * fix(cli): don't resolve for table output, guard the probe overflow, finish the CLAUDE.md bump (BUG-2608) Codex round 3, four findings. --full was treated as "content needed" regardless of output format, but the table path prints no bodies at any setting — so `pad item history --full` without --format json made the server walk the entire patch chain to build content the CLI then dropped. That is the exact waste this bug is about, reintroduced through the flag meant to opt into it. Content is now resolved only when it will actually be shown. The limit+1 probe overflowed at MaxInt: it wrapped negative, the client omitted the parameter, and a request the user bounded came back unbounded — the opposite of the ask. Guarded. The truncation notice's documented blind spot was understated: it is AT the server ceiling as well as above it, since the probe row is clamped away with everything else. Wording corrected rather than resolved — the CLI still does not duplicate the server's constant, because a copied ceiling goes stale silently and asking for hundreds of versions is already opting out of a bound. Two more CLAUDE.md sites still called v0.19 current; I fixed only the first on the previous pass. That document describes the contract in three places and I updated one, which is its own small lesson about grepping for every instance of a claim rather than the first. Live-verified against a real server: --limit 2 truncates and says so, --limit 4 on a 4-version item says nothing (the false positive Codex found), --limit 0 returns everything, --full --format json carries bodies, and the default JSON shape carries metadata only. * fix(server): clamp an out-of-range limit instead of treating it as unbounded (BUG-2608) Codex round 4. `?limit=9223372036854775808` made strconv.Atoi return ErrRange, which my parser lumped in with 'unparseable' and mapped to 0 — unbounded. So an absurdly large number defeated the very ceiling the function exists to impose, which is worse than no clamp at all: the bound looked enforced and was not. Atoi hands back the saturated value alongside ErrRange, so a range-positive input is now clamped to the ceiling — it is an oversized ASK, not a malformed one. Range-negative still falls through to unbounded, matching a plain negative. Regression cases cover both overflow directions, and the mutation that reinstates the old behaviour fails on the exact input. This is the input-domain lesson again: I enumerated absent, zero, negative and non-numeric, and stopped at what a person would plausibly type rather than at what the TYPE admits. * docs(cli): say in --help that a large --limit is capped server-side (BUG-2608) Codex round 6, and the third time this ceiling came up — which is the tell that the disposition was in the wrong place. I had documented it twice in CODE comments, where the person affected never looks. `--help` says what the flag does, and it said 'show only the newest N versions (0 = all)', which is false for an N above the server's cap. Now stated in both the flag help and the command's long description, including that the truncation notice cannot detect that case and that --limit 0 is the way to ask for a complete history. Deliberately not naming the number: a constant duplicated into help text goes stale silently, and 'capped server-side' is the part a user needs in order to act. Behaviour unchanged — this is the artifact catching up with it, which is the same correction the BUG-2301 sentinel comment and the instructions.md overclaim both came down to. * test: seed versions through the store so the fixture stops tripping the rate limiter (BUG-2608) CI caught this and it is my defect, not a flake. Arming the unbounded test above the plausible default meant seeding 60 versions, and the fixture did that with 60 HTTP PATCHes in a burst — which trips the server's rate limiter. It passed locally and in the Go job and failed under Nix, which is the signature of a test that was always going to fail intermittently rather than one that broke. Seeding now goes through the store. That is not a weakening: versions are recorded by the store on either path, and the endpoint under test is the READ side, so seeding underneath the transport costs the assertions nothing while removing a burst the server is entitled to refuse. The three things that make this fixture work are now stated where someone would otherwise undo them by accident — the large body (a small one is stored whole, so no version is ever is_diff and every diff assertion goes vacuous), the rotating source (the throttle collapses same-(actor, source) bursts into one version), and the store-not-HTTP seeding with the rate-limit reason attached. Re-verified after the change: the fixture still records more than 50 versions and still produces reverse-patch rows, and the default-cap mutation now uses the REALISTIC default of 50 rather than the 3 I first tested with — the old 5-version fixture could only have caught an implausibly small cap.	2026-08-17 19:02:53 -04:00
xarmian	c84cf7437c	feat(sessions): announce session identity on the event stream (PLAN-2558 S2, TASK-2560) (#1094) ... * feat(sessions): announce session identity on the event stream (TASK-2560) PLAN-2558 S2. S1 gave the presence registry a count of anonymous uuids; this makes each row nameable, which is what S3 needs for an honest empty state and S5 needs for a target picker. A monitor now announces itself when it opens the stream: X-Pad-Session-Label (the working directory's basename) and X-Pad-Session-Pid. The server sanitizes both and stores them on the LiveSession; GET /api/v1/sessions returns them. TRANSPORT. The task body sketched "the stream connect carries it" without picking a mechanism and explicitly left the call open. Headers, because a query param would put the label and pid into every access-log line (this server logs path= for each request) and any proxy log in front of it — which is the same "don't let local detail travel further than it needs to" the privacy line below is about — and a separate registration POST would need its own correlation to the connection it describes, plus a matching lifecycle, when the registry entry already lives and dies with the stream. Headers ride the request that exists and sit alongside Last-Event-ID, already doing this job on this endpoint. Cost, written into the code rather than discovered later: a browser EventSource cannot set headers, so a future web-tab consumer needs a deliberate query-param fallback or a fetch-based SSE reader. PRIVACY. The basename crosses, never the full cwd — "/home/dave/Dev/ docapp" additionally hands over a home directory and usually an account name for no gain — and messaging_socket_path never leaves the machine. Pinned by a test rather than by the implementation being one line. WHAT THIS DELIBERATELY DOESN'T DO: read ~/.pad/sessions/. The task framed S2 as giving `pad session register` its first consumer, and the monitor cannot honestly be one. Registry entries are written by whatever process ran that command — a different pid — and the only matchable fields are pid and cwd, so two agent sessions in one checkout are indistinguishable and "pick the newest" is a coin flip that would put a confident wrong name in the S5 picker. Process ancestry settles it exactly and is platform-specific (this binary ships for macOS and Windows). The monitor's own cwd basename and pid are never wrong and answer the question the label exists to answer; correlating a stream to the agent session that spawned it needs an identifier the harness passes down, which is worth doing when something needs it and worth not faking until then. Also moves S1's STALENESS doc block, which sat above LiveSession.Label where it read as documenting the name rather than the whole entry. Tests: sanitizer units (whitespace collapse, control-char stripping, rune-not-byte truncation), header wiring, the end-to-end labelled session, the unannounced-client compatibility leg (a pre-S2 monitor must still register and still stream), a hostile-input leg over the wire, the client's omit-when-unset behaviour, and the basename promise. Measured rather than assumed: Go's server answers 400 to a header value containing a control byte before any handler runs (verified with a raw socket, since Go's own client refuses to send one and the two refusals are indistinguishable from a normal client test). So that arm of the sanitizer is unreachable over HTTP; it stays as defence in depth for the next caller in, and both the comment and the wire test say so instead of the test quietly passing because the transport refused the input. Mutation-tested four ways, each revert grep-verified: handler ignoring the parsed identity, monitor sending the full cwd, dropping the truncation, and the client always setting the headers. Refs TASK-2560, PLAN-2558 * fix(cli): sanitize the session label client-side per Codex review (round 1) Codex round 1's only finding, and it is a bigger deal than a missing label. Unix directory names may contain control bytes — "doc\napp" is a legal directory — and Go's http.Client REFUSES to send a request whose header value holds one: Do returns "invalid header field value" and nothing is transmitted. In the monitor that is indistinguishable from an unreachable padd, so the retry loop backs off and tries again, forever, printing nothing by contract. A user who named a directory that way would simply stop receiving notifications, with no signal anywhere. The server cannot defend against a request that never arrives. Reproduced before fixing, with a real directory and a real client, rather than reasoned about from the error message. Sanitizing in NewWatchEventsStreamRequest rather than in monitorSessionIdentity: the invariant is "this function never builds an unsendable request", which belongs at the point where a value becomes a header, not at one caller. The client's cap (256 runes) is deliberately looser than and independent of the server's (64): the server decides what a label should look like, the client only has to keep the request sane, and neither has to track the other to stay correct. The regression test does the ROUND TRIP instead of inspecting the header, because the header contents were never the bug — http.Header.Set stores anything, so an assertion on the value passes against the broken version too. Only attempting the request tells the two apart. Mutation-verified: reverting the sanitizer fails the test with exactly the "invalid header field value" error from the field report.	2026-08-14 19:03:33 -04:00
xarmian	da6ce642da	feat(push): pad push — user-authored instruction dispatch to agent sessions (IDEA-2544 Phase 1) (#1090) ... * feat(push): add pad push <ref> -m vertical (IDEA-2544 Phase 1) Self-addressed, human-to-harness dispatch over the existing watch-events bus/stream: CLI -> POST .../items/{itemSlug}/push -> a new KindPush Notification (carrying the generalized TargetUserID addressed-to field KindAsk will later share) -> watchNotificationVisible delivers it back to the pushing user's own connected monitor sessions. Transient, fire-and-forget by design (no migration, no durable inbox) since assignment already covers the durable-notification case and this is meant to be the explicit, no-inference dispatch verb instead. * docs(plugin): document the push notification contract (IDEA-2544 Phase 1) Push is the one notification kind that IS an instruction rather than a passive fact, so it gets its own lead bullet in the plugin skill's notification-etiquette section (ahead of the read-only/park default, which it explicitly lifts) and a mention in the monitor's description. The embed-source skills/pad/SKILL.md has no notification section to mirror this into (the two files diverge by design) and is left untouched. * fix(push): reject over-long push messages instead of unbounded Summary Comments truncate their notification Summary to a preview (the full body is still fetchable), but a push message IS the payload — silently truncating it would corrupt the instruction with nothing to recover it from. Add maxPushMessageLen (4096, measured post-collapse) and reject anything over it with a 400 rather than truncating; state the same bound in `pad push --help` so it's discoverable before a 400, not only from one. * fix(push): close the watch-fallthrough leak, disambiguate SKILL.md exceptions Codex round 1 P1: watchNotificationVisible's push branch only returned early on a MATCH — a non-target caller fell through to the watch-map check below it, so anyone holding an unconditional (or predicated) watch on the item received every push addressed to every OTHER user, instruction text included. Push is addressed private dispatch, not an item-level fact watchers have a legitimate claim on (unlike assignment, which watchers are expected to see per `pad watch --help`) — the branch now returns unconditionally for KindPush, gating strictly on TargetUserID and never reaching the watch-map fallback either way. Pinned explicitly since Phase 4's session targeting is expected to inherit this same exclusivity. Also (codex P2): reworded the SKILL.md notification-etiquette bullets — the new push exception and the pre-existing assignment/ask exception literally contradicted each other ("the ONE narrow exception" claimed singularity after push had already claimed exception status). Now explicitly enumerated as the first and second exceptions to the never-write rule. * test(cli): pin that PushItem inherits X-Pad-Agent (BUG-2542 rebase) Verified, not assumed: PushItem builds its request via c.post -> c.newRequest like every other mutating client method (CreateWatch included), so the attribution fix's client.agentName wiring covers it for free with zero code changes needed on this branch. Adds a live httptest assertion rather than trusting the code-path read alone — the same shape as TestClientSendsResolvedAgentHeader, scoped to PushItem specifically since that's the one method this PR added. * fix(push): disambiguate workspace in the monitor line and skill contract Codex round 2 P1: the watch-events stream is user-scoped ACROSS every workspace a caller has watches in, but formatMonitorLine printed only ItemRef/Kind/Actor/Summary and dropped the Workspace field the wire payload already carried — a session linked to workspace A receiving a notification for workspace B would resolve the wrong item (or 404) with no signal in the line that anything was off. Fixed universally, not push-only: grepped plugin/ and skills/ for anything parsing "PAD ..." lines and found none — the Claude Code plugin host ingests the stdout line as free-text notification prose, formatMonitorLine's only real consumer is its own fmt.Println, so there is no wire-format consumer a workspace prefix could break. The ambiguity predates push (any watched item across workspaces already had it); push just makes the consequence sharper because it carries an instruction rather than a passive fact. SKILL.md's push bullet now tells the agent to resolve with `pad --workspace <workspace> item show <ref>` using the slug read off the notification line, not a bare `pad item show <ref>`. * fix(push): respect --format json instead of hardcoding plain text Codex round 2 P2: pushCmd's RunE ignored the global format flag and always printed "Pushed <ref>", silently discarding --format json. - server.pushResponse replaces the bare map the handler wrote before — a typed {ref, workspace, pushed, message} shape, with workspace resolved to the CANONICAL slug via s.getWorkspace (not merely echoed from whatever the URL contained), matching the same disambiguation need the round-2 P1 fix addressed for the monitor line. - cli.PushItem now returns (*PushResult, error) instead of discarding the response body. - pushCmd checks formatFlag == "json" and calls cli.PrintJSON, mirroring runCreateWatch's existing pattern. internal/cli/agent_identity_test.go's TestPushItemSendsResolvedAgentHeader needed a one-line update for PushItem's new two-value return — caught by `go vet ./...`, not `go build ./...` (which doesn't compile test files); folding vet into my own pre-flight going forward.	2026-08-13 18:41:46 -04:00
xarmian	212d59e7c6	fix(cli,server): make agent attribution actually happen (BUG-2542) (#1088) ... * fix(cli,server): make agent attribution actually happen (BUG-2542) Agent CLI writes were recorded as the human whose credentials they used. Three independent defects, each verified by reading the path AND by probing a live instance — the item deliberately held the mechanism open, so none of this is inherited. 1. THE HEADER WAS NEVER SENT. actorFromRequest sets actor="agent" on one signal: the X-Pad-Agent header. The only code that sets it took the value from `agent_name` in .pad.toml and nowhere else — no environment detection, no session detection. This repo's .pad.toml has only `workspace`, so the header has never been sent from here and every agent write has looked human. ResolveAgentName now resolves .pad.toml → $PAD_AGENT → detected runtime. 2. ITEM CREATE DISCARDED THE ACTOR. createItemChecked called actorFromRequest and kept only the source (`_, src :=`), never setting input.CreatedBy, so store.CreateItem fell through to its "user" default — even for an agent that DID send the header. Comments have always stamped it correctly; item creation silently did not, which made the skill's own contract false on its own terms. 3. SINGLE-ITEM PATCH NEVER STAMPED LastModifiedBy. Bulk ops do (handlers_items_bulk.go); the single-item path did not, so an item edited only by agents read as human-edited. Only entries VERIFIED against a live session belong in the runtime detection table, so it has exactly one: Claude Code exports CLAUDECODE=1 to child processes, confirmed by reading a pad subprocess's environment inside one. Guessing at Cursor/Windsurf/Aider variable names would put unverified claims in a shipped binary and misattribute silently when wrong; those set $PAD_AGENT until someone confirms a signature. WHAT THIS DOES NOT DO, stated in the code and the skill rather than left for someone to assume: the header is client-supplied and self-declared. An agent that omits it is indistinguishable from the human it borrows credentials from, and a human running `! pad ...` inside an agent's terminal inherits that environment and is attributed to the agent. This makes the trail HONEST, not VERIFIED — it is not a basis for machine-verifiable human-approval provenance, which needs a channel the agent cannot author at all. The incident behind this item is exactly that distinction: an agent's relay of a human's words was recorded indistinguishably from the human typing them. Contract corrected in both skill copies, since the item's first question was which of contract and behavior was wrong. It was the contract: it promised automatic agent attribution that only ever applied to workspaces that had opted in. Tests, each mutation-tested against its own defect reverted alone: - TestResolveAgentName — precedence plus the negative that makes it mean something: a plain human shell must still resolve to "". Fails 2/5 reverted. - TestItemAttribution_AgentVsHuman — agent and human legs for create, update and the create-stamp-survives-edit invariant. Fails on the create stamp reverted; fails 2/2 on the update stamp reverted. The update leg deliberately uses the OTHER writer: insertItemTx seeds last_modified_by FROM created_by, so a same-writer edit passes whether or not the PATCH stamps anything — the first version of this test did exactly that and passed its own counterfactual. Caught only because each fix was reverted separately. - TestItemAttribution_ExplicitBodyValueWins — an explicit body value still beats the header. End-to-end on a live instance through the real CLI, no .pad.toml opt-in: agent session → created_by/last_modified_by/comment all `agent`; same binary with CLAUDECODE stripped → all `user`. Claude-Session: https://claude.ai/code/session_01QGbUKZBAZoWdEgiTNWsXag * fix(server): artifact import wrote a UUID into created_by (BUG-2542) Found by Codex while reviewing the attribution fix. handleImportArtifact set `input.CreatedBy = u.ID`, which is the wrong DOMAIN for the field rather than merely the wrong value: created_by holds the role — "user" or "agent" — and consumers compare it against those literals (CommentThread.svelte, TimelineVersionCard.svelte). An imported item matched neither and rendered as neither. It also would have defeated the fix in the parent commit at this path: a non-empty CreatedBy suppresses the actor stamp, so imports would have kept a UUID while every other create path started recording the actor. The line contradicted the comment directly above it, which said Source was being left blank precisely so createItemChecked could stamp it "like every other create path". Now both fields are left blank and stamped together. The user's identity has its own home — the items.created_by_user_id column — which no create path currently populates. That is a separate gap and is not widened into this change. Claude-Session: https://claude.ai/code/session_01QGbUKZBAZoWdEgiTNWsXag * fix: close the remaining attribution bypasses Codex found (BUG-2542) Review found no P1s and three P2 families beyond the artifact-import bug already fixed in 6fac5dec. Two are closed here; two are deliberately not, and the reasons matter more than the diff. CLOSED — paths that asserted "user" and so SUPPRESSED the new stamp, which made them worse after the parent commit rather than merely stale: - cmd/pad/notes.go sent CreatedBy/LastModifiedBy = "user" from the CLIENT on all four note/decision writes. An explicit body value beats the header by design, so every agent note claimed a human wrote it, and would have kept claiming it. The client shouldn't assert an attribution it cannot know; all four now leave it to the server. - handlers_item_versions.go hardcoded LastModifiedBy "user" / Source "web" on restore, so an agent-driven restore recorded itself as a human web edit. Now stamped from the request. Also closed Codex's nit that the tests injected X-Pad-Agent directly and never proved the resolver reaches the wire — TestClientSendsResolvedAgentHeader runs the real client against an httptest server and asserts the header, with a human-shell leg asserting its ABSENCE. Fails when the client wiring is reverted. And the Source assertion now pins "web" rather than merely non-empty. NOT CLOSED, on purpose: - Collab flush. An agent PATCH stamps `agent`, then the browser's later ?source=collab-snapshot PATCH stamps `user`. Codex reads that as lost attribution; I'm not convinced it's wrong — the browser really is the writer of that flush, and the agent's edit is already recorded on the PATCH that carried it. Deciding whose name belongs on a human-flushed doc containing agent edits is a semantics call about what last_modified_by MEANS, not a bug I should settle inside a fix commit. Filed rather than guessed. - Move paths don't touch last_modified_by at all. That predates this change and is the same question (is a move an edit?), so it goes with the above. Claude-Session: https://claude.ai/code/session_01QGbUKZBAZoWdEgiTNWsXag * fix(cli): note/decision entries self-declare instead of going authorless Self-caught regression from the previous commit, found by checking the thing I changed rather than assuming it behaved like its neighbours. I removed the CLI's hardcoded CreatedBy: "user" from note and decision entries on the reasoning that applies to every OTHER write in that file: an explicit value suppresses the server's stamp, so the client should stay quiet and let the request context decide. That reasoning does not reach these two. The entries live INSIDE the item's fields JSON, which the server stores as an opaque blob and never parses for attribution — so nothing downstream fills the gap, and blanking it would have written authorless notes. Worse than the bug I was fixing: "user" was at least right half the time. They now carry cli.ActorKind() — the same self-declared signal as the header, reduced to the user/agent enum the field holds. Its doc says plainly that this is the ONE place a client should assert attribution, and why, so the next person doesn't generalise it back the wrong way. The item-level LastModifiedBy in the same functions stays server-stamped; that half of the previous commit was right. TestActorKind covers agent, explicit PAD_AGENT, and human-shell legs. Claude-Session: https://claude.ai/code/session_01QGbUKZBAZoWdEgiTNWsXag * fix(server): stamp the actor on non-parent item links (BUG-2542) Last P2 from the review. Parent links pass the actor to SetParentLink; every other link type (blocks / blocked-by / relates / implements) goes through CreateItemLink, which the CLI calls without created_by, so the store defaulted it to "user" and an agent's `pad item block` recorded a human. Same one-line shape as the create path, explicit body value still wins. TestItemLinkAttribution_AgentVsHuman covers both writers; fails on the agent leg when the stamp is reverted, control passes either way. That closes every actor-dropping path the review found except the two filed as IDEA-2549 (collab flush, move), which are semantics questions about what last_modified_by means rather than defects — Codex agrees the deferral holds if the field means content author, and flags that they become real follow-ups if we decide it means sender-of-write. Claude-Session: https://claude.ai/code/session_01QGbUKZBAZoWdEgiTNWsXag * test(server): table-drive every non-parent link type (BUG-2542) Codex nit: the link regression only covered `blocks`, and "shared routing makes the other types fine" was doing the work. It cost nothing to stop assuming, and the table earned itself on the first run — my initial list included `blocked-by`, which is CLI surface sugar that inverts source/target into a `blocks` row rather than a stored link type. The API rejects it with a 400, on BOTH writer legs, which is also how that failure reads differently from an attribution one. Now covers blocks / related / implements / supersedes / split_from against both writers. One precision fix owed on 06938079's message: it says "THE HEADER WAS NEVER SENT". Not true in general — a workspace with agent_name in .pad.toml did send it, which is exactly how I probed the behaviour before fixing it. Accurate version: the header was absent for anything that had not opted in, which is every workspace I can see, including this repo's. The body of that commit says it correctly; the headline overstates. Claude-Session: https://claude.ai/code/session_01QGbUKZBAZoWdEgiTNWsXag * style: gofmt notes.go after the attribution edit (BUG-2542) Removing the hardcoded LastModifiedBy from the two ItemUpdate literals left the surviving fields aligned to a column that no longer had a member, so gofmt disagreed and CI's golangci-lint failed the Go job in 42s. The real fault is upstream of the whitespace: my gates line for #1088 read "go test ./... green · Codex to CLEAN" and lint was simply not in it. The omission in the report and the failure in CI are the same fact — I reported a matrix that did not include the axis that broke. `make lint` runs the pinned suite CI runs and takes seconds; it belongs in every report I make, alongside test and build. Claude-Session: https://claude.ai/code/session_01QGbUKZBAZoWdEgiTNWsXag	2026-08-13 16:21:10 -04:00
xarmian	ec7fd027fc	feat(server,cli): watches, user-scoped event stream, plugin monitor command — PLAN-2469 Phase 1 (TASK-2533) (#1082) ... * feat(store): race-free status/assignment mutation signal (TASK-2533) Adds models.Item.LastMutation (ItemMutationSignal), populated inside the SAME transaction that already writes status_transitions / assigned_user_id in UpdateItemWithParentLink and MoveItemWithPreCheck. This is the foundation for TASK-2533's watch-notification pipeline: a before/after snapshot taken in the HTTP handler layer would race concurrent writers of the same item, so the signal is computed where the authoritative diff already happens, in-transaction. * feat(store): watches table migration, both drivers (TASK-2533) watches(id, workspace_id, user_id, item_id, predicate, created_at) per DOC-2479's subscription-table design: durable, server-side subscriptions that survive both the plugin-monitor process and a padd restart. uq_watches_user_item makes `pad watch <ref>` idempotent (re-watching upserts the predicate). Wires watches into the workspace-purge child-delete list, mirroring item_stars. * feat(watchevents): add in-process notification bus (TASK-2533) New package: a global (not per-workspace) in-process pub/sub bus carrying watch-worthy Notifications (status-change / assignment / comment; ask reserved in the enum with no producer yet — see the follow-up server commit). Bus is an interface specifically so a Redis-backed implementation can slot in later without touching any caller; only MemoryBus exists today. Package doc comment states the single-process/multi-instance limitation explicitly, mirroring internal/events' shape. * feat(store): watches CRUD (TASK-2533) models.Watch + Store.CreateWatch (upsert on user+item)/GetWatchByUserItem/ ListWatchesForUser (unscoped by workspace — a watch is personal, and the event-stream handler needs every watch a caller holds across all their workspaces)/DeleteWatch. * feat(server): watch/nudge event stream + CRUD endpoints (TASK-2533) GET /api/v1/events/stream (DOC-2479): a user-scoped, cross-workspace SSE stream, filtered server-side to the caller's watches (with optional --until field=value predicate) plus "addressed to you" — narrowed to assignment-to-you only for Phase 1, confirmed with the dispatcher: this codebase has neither a Collection.Kind field nor any user->active-role binding to ground DOC-2479's "human-gate-shaped collection targets your active role" half mechanically. watchevents.KindAsk stays in the wire enum with no producer. `pad session register` is the natural future hook for a session-carried role identity. POST/DELETE .../items/{slug}/watch, GET /api/v1/watches (unscoped, mirrors /auth/tokens' shape for a personal, not workspace, resource). Producer wiring (TASK-2533 audit) publishes from every live mutation path that can produce a LastMutation signal or a new comment: handleUpdateItem (incl. its collab sub-paths and the comment-attached-to-update path, which bypasses handleCreateComment entirely), handleMoveItem, handleCreateComment, item creation with an initial assignee, and the bulk-items loop (covers archive/restore/move/set-priority/tag/untag/assign uniformly via one call site). Named, not silent, bypasses: import bundle, status_transitions backfill, workspace restore/purge — none are live human-facing mutations. Known Phase-1 tradeoff, flagged not fixed: bulk mutations are NOT batched into one notification the way the existing SSE/webhook bulk path is — a bulk-assign of N items surfaces N individual notifications. Each is still correctly scoped by the recipient's own watches/addressed-to-you filter (a narrower audience than the workspace-wide SSE firehose the existing batching protects), so this is a noise-discipline tradeoff, not a leak. * feat(cli): pad watch + pad session register (TASK-2533) pad watch <ref> [--until field=value] creates/upserts a durable watch; pad watch list / pad watch remove <ref> are the hygiene companions the dispatcher asked to be included explicitly rather than silently added. pad watch --stream --for-session is the plugin-monitor command: one stdout line per matching event ("PAD TASK-214 -> kind (actor): summary"), silent on startup with no .pad.toml (hourly retry) or an unreachable padd (backoff retry) per DOC-2479's noise-discipline contract. The retry/ backoff math and line formatting are pure, unit-tested functions; the actual sleep loop is not (per the dispatcher's ask). pad session register writes ~/.pad/sessions/<pid>.json (pid, cwd, CLAUDE_CODE_MESSAGING_SOCKET when set) -- forward-looking infra for Phase 3's live-sessions/presence surface; nothing consumes it yet in Phase 1/2. * fix(server): comment replies never published a watch notification (TASK-2533) Codex round 1 finding 2 (verified real, not a false positive): handleCreateReply is a SEPARATE code path from handleCreateComment — it calls store.CreateComment directly via POST .../comments/{id}/replies, not POST .../comments — and was missing the watch-notification hook entirely. A reply to a comment on a watched item produced zero notification. Same kind=comment publish as the top-level path, plus a regression test covering the reply route specifically. * fix(server): re-check current access before serving/delivering watches (TASK-2533) Codex round 1 finding 1: ListWatchesForUser filtered only by user_id — a watch row survives a revoked workspace membership or grant (nothing deletes it), so GET /api/v1/watches and the event-stream's notification filter could keep leaking item title/ref, workspace slug, actor, and summary for access the caller no longer has. Adds Store.ListWatchesForUser's ItemCollectionID column (needed for the visibility check) and server.filterWatchesByCurrentAccess, which mirrors computeSSEVisibility's RBAC resolution (handlers_events.go) — admin bypass, VisibleCollectionIDs for member/guest full-collection access, GuestVisibleResources for item-level grants — grouped by workspace since a caller's watches can span many, unlike a single SSE connection scoped to one. Fails closed on any lookup error. Wired into handleListWatches here; the event-stream's loadWatchPredicates call site picks up the same filter in the next commit, which also restructures that function's Subscribe/replay sequence and therefore touches the same lines. * fix(watchevents): atomic ID assignment + subscribe-and-replay (TASK-2533) Codex round 1, findings 3 and 4 (same subsystem, fixed together): Finding 4 — sequence assignment and replay-buffer insertion happened under SEPARATE locks in MemoryBus.Publish. Two concurrent Publish calls could append to the ring buffer out of ID order, corrupting since()'s ordering assumptions (it walks the ring oldest→newest assuming monotonic IDs). Fixed by unifying seq assignment, buffer append, and the subscriber-list snapshot under one lock; the (already non-blocking) fan-out send still happens after releasing it. Finding 3 — GET /api/v1/events/stream called Subscribe() and, later (when resuming via Last-Event-ID), EventsSince() as two separate calls. A Notification published in the window between them landed in BOTH the replay result and the live channel, double-delivering it. Bus gains SubscribeAndReplaySince(sinceID), which atomically subscribes and reads the replay buffer under the SAME lock; the stream handler now uses it whenever a Last-Event-ID is present (this commit carries that call-site change, plus the finding-1 loadWatchPredicates filter wiring from the previous commit — both land in the same lines of this function). Adds a concurrent-publish ID-ordering test and a subscribe-then- concurrent-publish no-duplicate test, both run with -race. * fix(cli): monitor silent-start ordering + sync_required handling (TASK-2533) Codex round 1, findings 5 and 6: Finding 5 (P1) — runWatchMonitor called getClient() once, before the loop and before the .pad.toml check. getClient() -> getConfiguredConfig() os.Exit(1)s when unconfigured with no TTY, or launches an INTERACTIVE configuration wizard when one is attached — either way a direct violation of DOC-2479's silent-start contract, which requires "not ready yet" to be a silent retry, never a crash or a prompt. Adds monitorClient(), which builds the client the same way but returns a plain error instead of exiting or prompting; client construction now happens INSIDE the loop, after the .pad.toml gate, on every iteration, and its failure folds into the existing padd-unreachable backoff path. Finding 6 (P2) — streamWatchEvents ignored "sync_required" (the server's signal that the requested Last-Event-ID was evicted from its replay buffer), so a stale cursor got resent on every reconnect forever. Now clears the cursor on sync_required so the next reconnect is a fresh, non-resuming subscription instead. Both covered by tests that assert the goroutine returns promptly on context cancellation (proving no os.Exit / no blocking prompt was hit, since the test process itself is still running to observe the return) and that streamWatchEvents clears/re-tracks the cursor correctly around sync_required. * fix(server): uniform current-access gate for watch AND addressed-to-you delivery (TASK-2533) Codex round 2, findings 1 and 2 — same subsystem (watch/nudge delivery access control), fixed together; finding 2 explicitly falsifies finding 1's fix's own admin-bypass argument, so this replaces that reasoning rather than patching around it. Finding 1 (confirmed real): VisibleCollectionIDs / GuestVisibleCollectionIDs deliberately over-widen for navigation — a collection ID is included if the caller has an item grant on ANY item inside it, explicitly leaving item-level narrowing to the caller (their own doc comments say so). computeWatchAccessVisibility used that over-wide set directly as the "fully visible" gate, so a guest granted item A was treated as having full access to A's WHOLE collection, including an ungranted sibling item B. Fixed by building the "genuinely full access" set from GuestVisibleResources' fullCollectionIDs (populated only from direct collection_grants, never widened by an item grant) + GetMemberCollectionAccess / ListSystemCollectionIDs for an actual member — exactly computeSSEVisibility's own fullCollSet construction, not an approximation of it. Finding 2 (confirmed real): the addressed-to-you (KindAssignment) branch in watchNotificationVisible returned true unconditionally, with NO access check. validateAssignmentScope (internal/store/items.go) only checks WORKSPACE membership, never collection access, so an item can be assigned to a "specific"-access member whose granted collections don't include it at all — an ordinary assignment, no revocation timing required. Fixed by gating EVERY notification kind — watch-matched and addressed-to-you alike — through the SAME watchAccessVisibility check before either branch runs. watchevents.Notification gains CollectionID so the check has what it needs without a second lookup; the stream handler resolves it lazily per workspace via a small connection-scoped cache (workspaces aren't known in advance for addressed-to-you the way watch workspaces are), cleared on the same reval tick that reloads the watches map. This also required replacing computeWatchAccessVisibility's admin-bypass argument, not just its code: "every call site filters the caller's OWN watches" stopped being a sufficient justification once addressed-to-you (which is fundamentally about *this* caller's own assignment activity across every workspace) shares the same gate — a bearer-borne admin token unconditionally trusted for that is exactly BUG-1616's blast radius. Now mirrors computeSSEVisibility's cookie-vs-bearer distinction exactly. Tests: guest-with-item-grant no longer sees a sibling item's watch or stream notification (filter-level and HTTP/SSE-level); an assignment outside a restricted member's granted collections is denied at both levels; addressed-to-you is proven still gated (denied with no access, visible once granted) as a pure unit test. * fix(store): always re-read existing under lock, not just for precheck/patch updates (TASK-2533) Codex round 2 finding 4, verified real: updateItemWithParentLinkOnce's `existing` snapshot was only refreshed under the write lock when precheck != nil, ExpectedUpdatedAt != "", or FieldsPatch != nil — any update touching none of those (e.g. a plain title-only PATCH) kept the STALE pre-tx `existing` for the rest of the function, including the LastMutation assignment-delta comparison added in TASK-2533's first round. A concurrent OTHER transaction's assignment change landing between this transaction's pre-tx read and its lock acquisition would get misattributed to THIS transaction: a title-only update could report a spurious, wrongly-attributed AssignmentChanged for a transition it never made, duplicating the one the other transaction already reported correctly (or missing a real one, depending on interleaving). The status-transition capture already defended against exactly this with its own separate conditional re-read; the assignment-delta capture added later did not replicate that guard. Fixed by making the re-read unconditional — once, right after the locks are held, before any SET- clause building or the UPDATE itself — so every existing.* comparison in this function is race-free by construction, not by each caller remembering to guard itself. Also removes the now-redundant duplicate re-read the status code had of its own. Reproduces the exact race deterministically using UpdateItemWithPreCheck's precheck hook as a synchronization point (TX2's assignment change blocks mid-transaction while TX1's title-only update races its own pre-tx read against it) — the new test fails reliably against the pre-fix code and passes reliably (including under -race, and in Postgres mode) against the fix. * fix(watchevents): send under the same lock Unsubscribe/Close use (TASK-2533) Codex round 2 finding 3, confirmed real and high-severity: Publish snapshotted subscriber channels under the lock, released it, and only then sent to them. A concurrent Unsubscribe or Close could close one of those channels in the window between the snapshot and the send — a send on a closed channel PANICS in Go, which crashes the whole padd process, not just one subscriber's connection. The reasoning for releasing the lock before sending ("a slow subscriber would stall everyone else") didn't hold up: the send is already non-blocking (select/default — a full channel is dropped-and-logged, never awaited), so holding the lock through it costs nothing and closes the window structurally. Adds a hammer test (many iterations of concurrent Publish / Subscribe / Unsubscribe / Close, short-lived churned channels, recover()-wrapped so a regression fails cleanly instead of crashing the whole `go test` run) that reproduces "send on closed channel" dozens of times per run against the pre-fix code (plus an independent -race detection) and passes cleanly, repeatedly, against the fix. * fix(server): re-fetch the user, not just the vis map, on each reval tick (TASK-2533) Codex round 3, confirmed real: watchVisCache captured *models.User ONCE at connect time (newWatchVisCache) and never re-fetched it; reset() cleared only the per-workspace visibility map. computeSSEVisibility's own doc comment explains why it re-fetches the user fresh on every call — "so mid-stream role changes (admin demotion, user.disabled flips) take effect on the next tick" — and the round-2 commit claimed to mirror that "exactly," but only carried over the collection/bearer logic, not the re-fetch itself. Net effect: a demoted or disabled admin kept fullAccess on an open stream (both watch-matched and addressed-to-you delivery, since both go through this same cache) until reconnect. Adds watchVisCache.refreshUser, called by both the constructor and reset() so the cadence matches computeSSEVisibility's actual cadence in handlers_events.go (that function is invoked once at connect and again only on each membershipCheck tick — never per event — so "per cache reset" here is the same cadence, not a narrower one). Deliberately fails CLOSED (not open-to-stale like computeSSEVisibility's own transient-error fallback) on a fetch error, a deleted user, or a disabled user — a nudge stream's wrong failure mode is delivering a fact to someone who shouldn't see it, not a dropped UI update, so this trades computeSSEVisibility's availability-leaning fallback for a stricter one and says so in the comment rather than repeating the "mirrors exactly" claim the fix falsified. Tests: a unit-level pair (mirroring handlers_events_revalidation_test.go's existing admin-demotion/disable coverage of the analogous SSE gap exactly) proves an admin loses fullAccess after a demotion + reset(), and a disabled user is denied outright; an HTTP/SSE-level test proves a live stream stops delivering entirely once its connected user is disabled and a reval tick passes. All three reproduce the bug reliably against the pre-fix code and pass cleanly against the fix. The HTTP-level test deliberately runs serially (not t.Parallel()): it mutates the package-level watchListRevalInterval var, which every other parallel watch-stream test in this package also reads via its own ticker — writing to it from a t.Parallel() test raced against those reads under -race (misattributed by the race detector to a whole cluster of unrelated concurrently-running tests before this was diagnosed). Full server package -race pass is clean after the fix. * fix(server): decouple vis-cache reset from watch-list reload success (TASK-2533) Codex round 4, confirmed real: on a reval tick, if ListWatchesForUser errored, the handler's `continue` skipped visCache.reset() entirely — the two were coupled, with reset() only reachable on the reload's success path. A demoted or disabled user's stale identity/visibility (round 3's fix) stayed live for exactly as long as that UNRELATED query kept failing, so the round-3 leak reopens for the duration of any watch-list reload error. Fixed by running visCache.reset() first, unconditionally, before attempting the watch-list reload. On a reload failure, the stale watch list is kept (its own staleness is already bounded by watchListRevalInterval's "eventually consistent" contract) but is now gated by the FRESH visCache regardless — a demoted/disabled user is denied via visCache even while the watch list itself lags a tick. Chose this over dropping all delivery for the tick (the other option the finding offered) because tying stream availability to an unrelated query's transient health seemed like the wrong tradeoff; the comment at the call site states this choice explicitly. Adds a watchPredicatesLoadFault test seam on *Server (mirrors the existing restoreAckFault pattern) so the reload failure can be forced deterministically without breaking the DB connection for the whole test. Reproduces the exact bug: forces the reload to fail on every tick while concurrently disabling the connected user, and asserts addressed-to-you delivery (which depends only on visCache, never the watch list) is denied anyway. Fails reliably against a reverted (pre-fix, coupled) version of the reval branch and passes cleanly against the fix. Full server package -race pass, full suite (SQLite + Postgres) pass, lint clean — this is the pre-PR verification matrix; round 5 will be a narrow re-verify of this fix only. * fix(server): bound stale watch set under persistent reload failure; atomic test seam (TASK-2533) Codex round 5, two P2s, both confirmed real: Finding 1 — `watches = fresh` only ran on the reload's success path, so under a PERSISTENT (not single-tick) reload failure the watch set stayed live indefinitely: a dead watch (removed, item deleted) kept matching forever, and a watch created during the outage was silently missed forever — visCache (round 4) gates current ACCESS, not whether a watch still legitimately exists, so it couldn't catch this on its own. Fixed by tracking consecutive reload failures and clearing the watch set once maxConsecutiveWatchReloadFailures (3 ticks) is crossed, failing closed on watch-matched delivery specifically while addressed-to-you delivery (visCache-only, unaffected either way) continues throughout. Updated the tradeoff comment at the call site so the "eventually-consistent" claim now matches the bounded, not unbounded, behavior it actually describes. Finding 2 — the watchPredicatesLoadFault test seam was a plain `func() error` field, written by a test AFTER the SSE stream's background goroutine was already running and reading it on every reval tick: genuinely racy, unlike restoreAckFault's own use of the identical field shape, which is set once, synchronously, before the single HTTP request that reads it — goroutine creation's happens-before edge makes THAT usage safe without any extra synchronization. Verified restoreAckFault does not share the flaw and left it untouched. Fixed the watch seam with atomic.Pointer[func() error] instead. Test for finding 1: forces maxConsecutiveWatchReloadFailures+1 consecutive reload failures via the (now-atomic) fault seam and asserts watch-matched delivery is suppressed once the bound is crossed while addressed-to-you keeps delivering, then clears the fault and confirms watch-matched delivery resumes on the next successful reload — a bounded outage response, not a one-way ratchet. Fails reliably against the bound disabled, passes cleanly restored. This is the (re-run) pre-PR verification matrix per the dispatcher: SQLite + Postgres + full-suite -race + lint + gofmt, all clean. Round 6 is a narrow re-verify of these two fixes only. * test(store): bound the concurrent mutation-signal test's wait (TASK-2533) CI-triage follow-up: PR #1082's plain Postgres step hit go test's default 10-minute per-binary timeout. Investigated whether any store test added by this branch scales with runner slowness (lock-wait defaults, sleep-based polling, transaction-hold durations): - Watches CRUD tests (8): 0.63-0.80s each under Postgres, isolated and in the full 741-test package run. - Mutation-signal tests (6), including the precheck-hook two-transaction race test: 0.48-0.80s each; the race test held at 0.48-0.51s across 10 consecutive runs (no variance) and across the full-package run. - Full store package under Postgres: 279.17s and 277.12s across two runs on this branch, matching the ~275s/297s baseline team-lead measured locally and on PR #1081 — no reproducible slowdown from anything this branch adds. No pathological test found locally. The one test with genuine cross-goroutine DB lock contention (TestLastMutation_AssignmentDelta_ NotMisattributedUnderConcurrentWrite) had an unbounded wg.Wait() as its only unbounded wait — TX2's release was already unconditional (fixed 50ms sleep, not gated on TX1's progress), so there's no deadlock risk, but there was no ceiling on how long legitimate lock contention could stretch it under a slow/shared runner. Replaced with a bounded 10s wait that fails fast with a diagnostic instead of silently consuming test-binary budget if it's ever exceeded. Verified the regression test still fails reliably (5/5) against a revert of the round-2 fix it guards. Could not reproduce the CI timeout locally; likely the pre-existing ~297s CI baseline (already noted as close to the 10-minute ceiling) plus environmental variance on the shared runner, not a specific test this branch adds.	2026-08-12 15:50:41 -04:00
Dan Finn	20b061902b	fix(cli): surface actionable errors for cloud-mode setup failures ... When a user picks Cloud mode during `pad init` but doesn't have a Pad Cloud account, the CLI hits the cloud server and surfaces raw server errors like "Missing CSRF token" — which is an implementation detail that gives no indication of what went wrong or how to fix it. This patch: - Splits the ModeCloud and ModeRemote branches in printSetupRequiredHint so cloud users see "sign up or switch to local" instead of the generic "run pad auth setup on the server" message. - Returns a cloud-specific error from pad init when setup_required is true in cloud mode. - Intercepts csrf_error responses in the CLI HTTP client and replaces the raw server message with an actionable "run pad auth login" message, since the CLI never sends CSRF cookies and this error always indicates a stale or mismatched session.	2026-08-03 16:22:11 -04:00
xarmian	1e48a7a1dd	feat(cli): add pad item copy for cross-workspace copy and move (TASK-2366) ... Wraps PLAN-2357's two endpoints behind one command: pad item copy <ref> --to-workspace <slug> --collection <slug> [--dry-run] [--archive-source] [--field key=value ...] --dry-run renders the preflight's three contract buckets (carried / dropped / needs_value) and DR-15's full warning set. Every bucket header and every warning line prints unconditionally, zeros and empties included: omitting a zero would make "no attachments" indistinguishable from "this CLI does not report attachments", and DR-17's whole point is that none of it is silent. Schema-supplied strings are escaped and list members quoted, so a comma or newline in an option value cannot forge an entry or a row. --format json emits the endpoint's own response. json.Indent is a lexical transform, so key order, unmodelled fields and int64 precision all survive; the bytes are never round-tripped through a Go value. DR-13, the no-retry obligation. There is no idempotency key, so a blind re-run duplicates the item. Four mechanisms, each with a test: 1. the mutating copy runs on its own *http.Client AND its own transport. The transport half is the one that matters: retry in Go is almost always a RoundTripper wrapper, which a merely-dedicated http.Client would inherit. A plain *http.Transport is cloned so proxy/TLS config carries; a wrapper is not used at all; 2. its body is hidden behind an opaque reader, leaving Request.GetBody nil so net/http's own nothing-written replay cannot fire; 3. redirects are refused rather than followed with the POST body; 4. failures are classified into three exclusive outcomes, because each licenses a different thing to say. UNKNOWN (transport failure, 500 copy_failed) sends the user to check the destination and never suggests a retry. COMMITTED-BUT-UNREPORTED (a 2xx whose body could not be read or decoded) exits ZERO -- a non-zero exit would tell a script the copy did not happen, which is the DR-13 duplicate arrived at through the reporting layer. A 4xx is a refusal made before any write and passes through plainly. The same asymmetry governs stdout: a write failure on the dry run is an error (nothing happened), while a write failure after the copy committed goes to stderr and leaves the exit code at 0. Refuse to guess. The preflight always runs first (it is read-only), and a non-empty needs_value refuses before any mutating request, naming each field and the exact --field flags to add. Mirrors the web dialog's disabled confirm rather than round-tripping the user into an error they could have been shown. --field values are typed against the DESTINATION collection's schema, so a number lands as a number. A malformed --field is a hard error here rather than the silent skip `pad item create` does: this command's contract is "you were told what to supply", and dropping a supplied value would make the refusal a lie. The response types in internal/cli mirror internal/server's. That is a layering choice, not a cycle -- nothing in server imports cli, and the mirror test imports server freely. It follows the posture already recorded in internal/cli/bootstrap.go: this package is the HTTP client and does not depend on the server package. An external cli_test package walks both response shapes and fails on any JSON contract drift. MCP is deliberately untouched: no pad_item.action: copy, and ToolSurfaceVersion stays 0.15.	2026-07-31 02:23:24 +00:00
Yui(ゆい)	9f4704a31f	fix(cli): wrap comment not-found errors (#910)	2026-07-11 01:17:52 -04:00
xarmian	c62658a11b	fix(cli): root usage/error/format hygiene (TASK-2031, BUG-2032) (#893) ... * fix(cli): silence usage on runtime errors, echo not-found input, validate --format TASK-2031 + BUG-2032 (PLAN-1985). SilenceUsage + FlagErrorFunc keeps flag-error help; GetItem/UpdateItem/DeleteItem wrap not_found with ref+workspace; PersistentPreRunE rejects invalid --format; honest markdown advertising. * fix(cli): return enriched *APIError for not_found to preserve concrete type Team-lead P2: itemNotFoundError changed the concrete error type, so direct err.(*cli.APIError) assertions (which do not unwrap) stopped matching not_found — notably bulk-update's per-row code capture (cmd_item.go:2043), dropping code:"not_found" from the JSON envelope. Return a fresh *APIError (same Code/Details, enriched Message) instead of a wrapper type; APIError.Error() returns Message so the clean one-line message is unchanged. Both err.(*APIError) and errors.As now match. Test adds a direct-assertion + Details-passthrough lock-in.	2026-07-09 21:50:36 -04:00
xarmian	bed933d7fd	feat(items): field-level PATCH + conflict envelope + read-only version history (TASK-2022) (#876) ... * feat(items): field-level PATCH + conflict envelope + version history Adds three related item-update primitives (TASK-2022 / IDEA-1480): - Field-level merge: PATCH `fields_patch` shallow-merges onto the item's current fields INSIDE the write transaction (null deletes a key), so concurrent single-field updates no longer clobber each other via the full-blob read-modify-write. `pad item update` and the MCP `pad_item.update` action now send only the changed keys. - Optimistic concurrency: optional `expected_updated_at` on update; on mismatch the store returns *UpdateConflictError and the handler emits the pad-structured-error/v1 conflict envelope (HTTP 409, code=update_conflict). Surfaced on CLI (`--expected-updated-at`) and MCP (`expected_updated_at`). - Read-only version history: `pad item history <ref>` (alias `versions`) and MCP `pad_item.history`, reusing the existing item_versions store + versions endpoint (no new store, no schema change). MCP ToolSurfaceVersion bumped 0.9 -> 1.0 (new action + param; update behavior change). No migration required. Claude-Session: https://claude.ai/code/session_019knGmnHcx5rrgWXQ8V8DZS * fix(items): address Codex review — dispatcher fields_patch, OCC ordering, date/required guards Round 1+2 review fixes for TASK-2022: - HTTP MCP dispatcher (dispatch_http_advanced.go) now sends fields_patch (only changed keys) instead of a client-side merged full fields blob, and forwards expected_updated_at — remote MCP callers get the same race-free merge + optimistic concurrency the CLI/HTTP paths do. - ValidatePartialFields rejects null-deleting a schema-declared REQUIRED field (would otherwise persist a blob the full-update validator rejects). - Open-children guard on the fields_patch path merges the patch onto the IN-TX locked row inside the precheck (not a stale pre-lock preview), so a priority-only patch can't false-fire the guard. - Optimistic-concurrency check now runs BEFORE the open-children precheck in the store, so a stale expected_updated_at yields update_conflict (not open_children) — single in-tx re-read shared by both. - Date auto-population on the patch path only fills an EMPTY current date; an existing end_date the caller isn't touching is preserved. Tests added for each fix. Claude-Session: https://claude.ai/code/session_019knGmnHcx5rrgWXQ8V8DZS	2026-07-08 16:47:34 -04:00
xarmian	c127a5f965	fix(playbooks): enforce draft gate server-side + expose status (BUG-2020) (#874) ... * fix(playbooks): enforce draft gate server-side + expose status (BUG-2020) pad_playbook run / POST /playbooks/{ref}/run now refuse a playbook whose status isn't "active" with a structured playbook_not_active error. Adds an allow_draft escape hatch across all surfaces: JSON body field, CLI --allow-draft flag, MCP boolean param, and an "allow-draft" bareword in raw_args (stripped before strict parsing). status is now echoed on both the run and get responses. Bumps ToolSurfaceVersion 0.9 -> 0.10 and updates the drift-guarded docs (instructions.md, README.md) plus CLAUDE.md. Claude-Session: https://claude.ai/code/session_019knGmnHcx5rrgWXQ8V8DZS * fix(playbooks): forward allow_draft on WebMCP + refresh mcp serve help Codex review follow-up for BUG-2020: - WebMCP dispatcher + api client now forward allow_draft so the browser surface can use the draft-gate escape hatch the catalog advertises. - `pad mcp serve --help` refreshed from the stale "v0.4 / eight tools" text to the current v0.10 / nine-tool surface (incl. pad_library). Claude-Session: https://claude.ai/code/session_019knGmnHcx5rrgWXQ8V8DZS * fix(playbooks): type playbooks.get() with top-level status (BUG-2020) Codex P3 follow-up: the get response now returns Item & { status }. Claude-Session: https://claude.ai/code/session_019knGmnHcx5rrgWXQ8V8DZS	2026-07-08 15:28:21 -04:00
xarmian	f5b437a65f	feat(server): workspace restore + deleted-list endpoints (TASK-1970) (#827) ... Foundation for PLAN-1969 (user-recoverable workspace soft-delete). A workspace delete only stamps workspaces.deleted_at; items/collections/ members are untouched, hidden transitively. Restore clears deleted_at so everything re-surfaces intact. Store (internal/store/workspaces.go): - RestoreWorkspace(slug): UPDATE ... SET deleted_at = NULL WHERE slug=? AND deleted_at IS NOT NULL. Returns sql.ErrNoRows (-> 404) when no soft-deleted row matched (already live or purged). - ListDeletedWorkspaces(userID, cutoff): owner-scoped, deleted_at within the window, ordered deleted_at DESC. Account-deleted workspaces have no live owner, so they never leak. - GetDeletedWorkspaceBySlug(slug): resolves a soft-deleted row (the normal resolvers filter deleted_at IS NULL) so the handler can tell 403 from 404. - Dual-dialect via s.q/s.dialect; no migration (deleted_at already exists). Handlers (internal/server/handlers_workspaces.go): - POST /api/v1/workspaces/{slug}/restore: owner-only; 404 not-restorable, 403 non-owner, 200 + restored workspace; logs a "restored" activity. - GET /api/v1/workspaces/deleted: owner-scoped list with per-entry purge_at + days_left, both derived from workspacePurgeRetention so restore and the purge sweeper share ONE 30-day window (no drift). - Both routed outside the /{slug} RequireWorkspaceAccess subrouter (which resolves only live workspaces); restore enforces owner authz inline. CLI client (internal/cli/client.go): RestoreWorkspace + ListDeletedWorkspaces. TS type (web/src/lib/types/index.ts): Workspace.deleted_at + DeletedWorkspace. Tests: store (resurface-intact; double-restore/live -> ErrNoRows; window boundary 29d IN / 31d OUT + owner-scoping) and handler (owner-only 403, 404 live/unknown, 200 restore, owner-scoped deleted-list). Green on SQLite and Postgres (make test-pg); golangci-lint clean. Closes TASK-1970 Claude-Session: https://claude.ai/code/session_01HxBkAMiFBtCRJ2tKSCt3ST	2026-07-06 10:07:34 -04:00
xarmian	665f1918a7	feat(cli): headless admin bootstrap via --email/--name/--password (BUG-988) (#761) ... * feat(cli): headless admin bootstrap via --email/--name/--password (BUG-988) Adds non-interactive flags to `pad auth setup` and `pad init` so agents running inside Claude Code or other non-TTY environments can bootstrap a fresh Pad instance without hitting interactive prompts that block forever. - New flags --email, --name, --password on both commands; all three must be supplied together when any one is present (clear error naming the missing flag otherwise). Checked before the remote-mode guard so the headless path works on any server host — the loopback gate is enforced server-side. - runHeadlessSetup() drives the existing POST /api/v1/auth/bootstrap endpoint directly, saves credentials, and respects --format json (emits a LoginResponse-shaped object with user + token). Already-initialized conflict produces a structured JSON error object under --format json. - `pad init` slots headless bootstrap into the bootstrap step only; the rest of init (config, workspace creation, skill install) continues. - Hardens readPassword() with an early non-TTY guard (generic message). - Hardens promptAndBootstrap() with a bootstrap-specific non-TTY guard (points at --email/--name/--password flags) so --cli-prompt on a pipe exits immediately rather than blocking. - Extends `pad init` non-TTY error message to mention the new flags. - Five new tests in cmd/pad/setup_headless_test.go covering success, missing-flag validation, non-TTY guard, already-initialized conflict, and the full init flow including workspace creation. NOTE: --password is visible in process listings (inherent to flag-based injection). Env-var bootstrap (PAD_ADMIN_*) is the tracked follow-up. Claude-Session: https://claude.ai/code/session_01WK9cUjxniBBAihGD5ygjDr * fix(cli): thread bootstrap token, factor shared core, restore readPassword fallback Round-1 codex findings: 1. Bootstrap token not sent on headless path (BLOCKER) Add BootstrapWithToken(email, name, password, token) to cli.Client that sets X-Bootstrap-Token when token is non-empty. Export ReadBootstrapToken from internal/cli/bootstrap.go (was readBootstrapToken) so cmd/pad can call it. Extract doHeadlessBootstrap(cfg, client, email, name, password) as the shared core for both setupCmd and padInitCmd: reads the on-disk token best-effort (absent → empty → loopback gate still covers that case), calls BootstrapWithToken, saves credentials, sets auth token on client. Both headless paths now go through this single function — no divergence. 2. readPassword bufio fallback removed by accident (REGRESSION) Restore the pre-round-1 bufio fallback in readPassword so piped-password flows (e.g. pad auth login --interactive in CI) keep working. The bootstrap wedge is already prevented by the top-of-promptAndBootstrap TTY guard; the generic readPassword fallback is only reached by non-bootstrap callers. 3. Shared core (CLEANUP) padInitCmd now calls doHeadlessBootstrap instead of duplicating Bootstrap + saveCredentials + SetAuthToken. The --format json asymmetry (init vs setup) is resolved by design: pad init is a multi-step flow; for machine-readable bootstrap output agents should use `pad auth setup --email … --format json`. Documented in the inline comment on the headless branch in padInitCmd. Tests added: TestHeadlessSetupSendsBootstrapToken, TestHeadlessSetupNoTokenFileOK, TestReadPasswordFallback. Update internal/cli/bootstrap_test.go for the rename. Claude-Session: https://claude.ai/code/session_01WK9cUjxniBBAihGD5ygjDr * BUG-988 round-2: surface token-read errors, wrap 403 with hint, rescope readPassword test doHeadlessBootstrap: distinguish os.ErrNotExist (absent token → best-effort empty, proceed without header) from other read errors (permissions, etc. → surface with the file path so operators can diagnose rather than silently hitting a confusing 403). Wrap 403/forbidden from BootstrapWithToken with an actionable multi-bullet hint covering loopback gate, token-file path, and PAD_BYPASS_SETUP_TOKEN. ReadBootstrapToken (internal/cli/bootstrap.go): add %w to the ErrNotExist branch so errors.Is(err, os.ErrNotExist) propagates to callers; existing tests and --cli-prompt hint text preserved. TestReadPasswordFallback → TestReadPasswordBufioFallback: rescoped to assert readPassword isolation only; added comment citing BUG-1886 (pre-existing doInteractiveLogin double-bufio.Reader bug). BUG-1886 filed in docapp.	2026-06-24 10:50:33 -04:00
xarmian	285a58e40e	feat(artifact): pad item export/import CLI commands (#756) ... * feat(artifact): pad item export/import CLI commands Phase 3 of PLAN-1867. - pad item export <ref> [-o file] — writes a playbook/convention as a portable <slug>.pad.md artifact (or stdout via -o -). - pad item import <file> — POSTs the artifact (or stdin via -), prints the new draft ref + slug and any server warnings (coerced fields, renamed slug). Adds ExportItemArtifact/ImportArtifact client methods. Implements TASK-1876, TASK-1877. Claude-Session: https://claude.ai/code/session_01KmxkPxLksjf1pmrZDpsnTJ * fix(artifact): harden CLI export file write Addresses Codex Phase-3 review: - filenameFromContentDisposition reduces to filepath.Base with safe fallbacks — a hostile Content-Disposition can't traverse/abs-write. - export writes atomically (temp + Sync + Rename) like attachment download, so a failed write can't truncate/leave a partial artifact. Claude-Session: https://claude.ai/code/session_01KmxkPxLksjf1pmrZDpsnTJ	2026-06-22 12:51:20 -04:00
xarmian	248f7c5ede	feat(items): expose item restore via CLI + MCP (TASK-1828) (#734) ... Adds the agent-facing restore surface so an archived item discovered via `pad item list --all` can be recovered without dropping to the web UI. The server already had restore end-to-end (Store.RestoreItem + handleRestoreItem at POST /items/{ref}/restore, used by the web UI and bulk ops); this wires the two missing surfaces: - CLI: `pad item restore <ref>` (cli.Client.RestoreItem → the existing endpoint, which resolves the ref include-deleted server-side). Mirrors `pad item delete`'s structured JSON envelope: {ref, title, restored: true}. - MCP: pad_item action=restore via passThrough(["item","restore"]). Restore is non-destructive, so it's safe to expose. The action auto-joins the schema's action enum (derived from the Actions map) and is documented in the tool description. Conflict case (slug/invocation_slug reclaimed while archived) is already handled by handleRestoreItem (409) and surfaced by the client's handleResponse. Tests: restore endpoint already covered (handlers_items_test.go); restore added to the MCP catalog<->cmdhelp bijection + dispatch tests. Child of BUG-1791 (TASK-1827 shipped in #733).	2026-06-15 15:49:51 -04:00
xarmian	1b1068537c	feat(tags): workspace tag enumeration endpoint + cross-collection filter (TASK-1653) (#658) ... * feat(tags): workspace tag enumeration endpoint + cross-collection filter (TASK-1653) Foundation for the tags feature (PLAN-1652 / IDEA-1649). The write path and per-collection ?tag= filter already existed; this adds tag enumeration and a verified cross-collection read so a single tag can group items of any type. - store: dialect.JSONArrayElements unnests a JSON text-array column (json_each on SQLite, jsonb_array_elements_text on Postgres); Store.ListWorkspaceTags returns distinct tags + item counts, ordered by count desc then tag asc, with the same collection/item ACL filters as ListItems so counts never leak hidden items. - server: GET /workspaces/{ws}/tags (handleListTags), respecting collection visibility + guest item grants. - models: TagCount{tag,count}. - cli: client.ListTags + `pad tag list`. - web: api.tags.list + TagCount type (items.list already forwards `tag`). - tests: store-level (cross-collection aggregation, collection scoping, non-nil-empty = empty, archived excluded) and handler-level (a Task + an Idea sharing one tag; GET /tags counts + ordering). Parent: PLAN-1652. * fix(tags): count distinct items per tag, not tag occurrences per Codex review (round 1) COUNT(DISTINCT i.id) so an item with duplicate tags (e.g. ["ux","ux"]) is counted once — the write path doesn't enforce per-item tag uniqueness. Adds a regression test.	2026-05-29 23:43:02 -04:00
xarmian	949ae03c88	feat(cli,mcp): pad project report + pad_project report action (TASK-1635) (#641) ... Expose the report aggregation (TASK-1630) to agents: - CLI: `pad project report [--window day|week|2wk|month] [--collections a,b]` fetches GET /workspaces/{ws}/report and renders a colored summary (totals, per-bucket throughput, completed-by-collection, status distribution); --format json prints the raw payload. - client.GetReport HTTP method. - MCP: pad_project gains action=report (passThrough to `project report`) with window + collections params; catalog-readonly test stub + expected maps updated. Parent: PLAN-1628.	2026-05-29 09:08:45 -04:00
xarmian	342679a364	Standardize plan-limit error envelope across HTTP/MCP/CLI/UI (TASK-788) (#628) ... * fix: limit-hit responses were actively broken — garbled toasts, no upgrade signal The limit enforcement responses (plan_limit_exceeded on 403) used a flat body shape {"error": "plan_limit_exceeded", ...} that is incompatible with every consumer: the frontend PadApiError parser, the CLI parseError path, and the MCP classifyHTTPStatusKind all expect {"error": {"code": ..., "message": ...}}. As a result, hitting any of the 5 plan limits (items, members, workspaces, api_tokens, webhooks) produced garbled toasts with undefined message text and zero upgrade signal. Fix: - writePlanLimitError now emits the standard nested error envelope with a human-readable message sentence and limit details in error.details. - CLI parseError now correctly surfacing the message (net positive, no code change needed). - MCP classifyHTTPStatusKind: adds ErrPlanLimitExceeded to the taxonomy and the allowedStructuredErrorCodes whitelist so 403 plan-limit errors pass through with code + details rather than collapsing to ErrPermissionDenied (TASK-788). - Frontend: exports isPlanLimitError() type-guard and planLimitMessage() formatter from client.ts; all 4 limit-hit write call sites (item create, member invite, workspace create, token create) now branch on the code and show an upgrade-signal message pointing at /console/billing. - Test: updates handlers_workspace_cap_test.go to the new body shape; adds TestPlanLimitError_ResponseShape covering members_per_workspace limit hit. TASK-788 * fix(R1): cover MCP stdio path, 5 more item-create sites, polish message wording Finding A — MCP stdio transport was missing plan-limit coverage: - cli/client.go: add PlanLimitDetails struct, AsPlanLimit() helper, and WritePlanLimitError() that emits the pad-structured-error/v1 marker so the MCP stdio classifier can lift code + details instead of falling through to ErrServerError. - cmd/pad/main.go: wire the WritePlanLimitError branch into all three CreateItem call sites (item create, convention activate, playbook activate). - internal/mcp: add TestClassifyHTTPStatus_PlanLimitPreservesCodeAndDetails, TestClassifyHTTPStatus_Generic403FallsToPermissionDenied, TestClassifyExecError_PlanLimitMarkerLiftsStructuredPayload, and TestClassifyExecError_PlanLimitWithoutMarkerFallsThrough. Note: extractUpstreamErrorEnvelope already parses details (json.RawMessage field) — the codex concern about it being silently empty was a false alarm; no fix needed there. Finding B — 5 more item-create entry points were unguarded: - EditorBubbleMenu.svelte (inline wiki-link capture) - Sidebar.svelte (quick-add) - roles/+page.svelte (board new-item, was console.error only; adds toastStore) - conventions/+page.svelte - playbooks/+page.svelte (both create and duplicate paths) B1/B2 polish — server message is now statement-of-fact only, no doubled upgrade verb. planLimitMessage() drops "Upgrade to Pro to add more." (each surface appends its own CTA). limitStr uses hyphenated adjective form "3-member" / "10-item" (compound modifier before "limit"). TASK-788 * feat(task-788): extend MCP-stdio plan-limit coverage to workspace, invite, webhook Wire WritePlanLimitError into three additional CLI command error paths so the MCP stdio classifier surfaces ErrPlanLimitExceeded with details instead of falling through to ErrServerError: - workspaceCreateCmd: check before fmt.Errorf wraps the APIError - inviteCmd: check before returning the raw error - webhooksCreateCmd: check before returning the raw error Add TestClassifyExecError_PlanLimitWorkspaceCreate to exercise the full workspace-create stdio round-trip through classifyExecError, asserting ErrPlanLimitExceeded code, feature="workspaces", limit, and upgrade_url. Token create intentionally left bare (agents don't drive token creation).	2026-05-25 11:46:41 -04:00
xarmian	8e7d4040fd	feat(backlinks): server-side reverse index for [[...]] (Phase 1) (#620) ... * feat(backlinks): server-side reverse index for [[...]] wiki-links (Phase 1) First phase of PLAN-1593. Today [[REF]] is parsed only at render time on the client and there's no way to ask "who links to TASK-5?" without a full-text scan. This change adds a materialized reverse index (item_wiki_links) that's written every time an item's content changes and exposes it via REST + CLI. Phase 1 covers ref-form links only (`[[TASK-5]]` / `[[TASK-5|Display]]`). Phase 2 (TASK-1595) will extend to titles + cross-workspace; Phase 3 (TASK-1596) adds the web UI panel + MCP action. What lands here: * Migrations 061 (SQLite) and 040 (Postgres) create item_wiki_links with partial indexes on target_item_id, (target_workspace_id, target_ref), and target_title — the schema accommodates all 5 wiki-link forms up-front so Phase 2 doesn't ALTER. * internal/links/extract.go is the canonical parser. It strips fenced and inline code regions before extracting [[...]] occurrences, so example refs in docs / code blocks don't pollute the index. Phase 1 emits only WikiLinkKindRef rows; title and workspace_ref kinds parse successfully but are gated out until Phase 2. * internal/store/wiki_links.go (replaceWikiLinks + GetBacklinks + helpers) handles write-time bookkeeping and the read query. Resolution to target_item_id happens at parse time inside the same transaction as the items INSERT/UPDATE, so partial state never lands. Broken refs (target_item_id IS NULL) intentionally persist — they feed a future broken-links report. * internal/store/wiki_links_backfill.go + cmd/pad/main.go hook the idempotent backfill into server startup. Existing items get indexed on first boot after the migration; subsequent boots are near-no-ops via an EXISTS short-circuit. * internal/store/items.go is amended in two places: tryCreateItem always calls replaceWikiLinks (empty content → no-op DELETE), and UpdateItemWithPreCheck re-parses whenever input.Content was supplied. * internal/server/handlers_backlinks.go serves `GET /api/v1/workspaces/{ws}/items/{itemSlug}/backlinks` with visibility + guest-grant filtering on the source items. * internal/cli/client.go adds GetBacklinks; cmd/pad/main.go adds the `pad item backlinks <ref>` command (registered in groups.go). Behavior decisions (per PLAN-1593): - code blocks excluded (fenced + inline) - self-links filtered at query time (kept in storage) - repeated mentions stored as separate rows by position - ordering: source updated_at DESC, position ASC Tests: - internal/links/extract_test.go: 26 sub-cases covering ref/title/ workspace-ref discrimination, code-block exclusion (fenced + inline + unclosed fence), position-is-byte-offset (UTF-8 safety), and edge inputs. - internal/store/wiki_links_test.go: 8 integration tests covering the create/update/delete/self-link/broken-ref/repeated/code-block scenarios plus backfill idempotence. All pass. `make check` clean (lint + go test + web build). Refs: TASK-1594, PLAN-1593, IDEA-1577 * fix(backlinks): visibility-aware pagination + case-insensitive refs per Codex review (round 1) Two fixes from Codex code review: P1 — GetBacklinks now takes a visibleCollectionIDs []string argument that's applied INSIDE the SQL WHERE clause. Previously the handler fetched LIMIT raw rows and filtered visible ones in Go, so a restricted user asking for limit=50 could receive an empty page even when later visible backlinks existed. Pushing visibility into SQL makes LIMIT/OFFSET count visible rows. nil → no restriction (owners, editors, root tokens) [] → see nothing (returns early, no SQL) [..] → AND s.collection_id IN (?, ?, ...) Item-level guest grants still apply post-fetch — they're rare enough that the residual page shrink is acceptable and pushing them into SQL would balloon the query. P2 — refPattern now accepts mixed/lowercase refs and parseBody canonicalizes the prefix to uppercase at the single chokepoint. Previously the renderer accepted `[[task-5]]` as a real link (its REF_PATTERN is case-insensitive) but the indexer's ^[A-Z]... pattern silently dropped it — divergent parsing on the same input. Storage shape is canonical uppercase so the (workspace, prefix, number) lookup against collections.prefix (also uppercase) has one shape. New helper: canonicalizeRef("task-5") → "TASK-5". Regressions: internal/links/extract_test.go + TestCanonicalizeRef — helper unit tests + TestExtractWikiLinks_RefVsTitleFallback updated to assert mixed/lowercase parses-as-ref-and-uppercases + edge-case test renamed from "lowercase ref" to "number-led not a ref" (lowercase IS a ref now per Codex P2) internal/store/wiki_links_test.go + TestWikiLinks_MixedCaseRefIndexed — `[[task-5]]` produces a backlink row whose target_ref is "TASK-5" + TestWikiLinks_VisibilityAwarePagination — three sub-cases: nil → all 3, visible-only limit=2 → 2 visible rows (not 1 with hidden one consuming a slot), empty → 0 All call sites updated (8 in tests + 1 in handler). `make check` clean (lint + tests + web build). Refs: TASK-1594, PLAN-1593 * fix(backlinks): SQL-level item-grant filter per Codex review (round 2) Round 1 fixed pagination for collection-level visibility but Codex round 2 correctly flagged the same class of bug at the item-grant layer: `visibleCollectionIDs` returns the UNION (full grants ∪ collections containing granted items), and the handler then filtered each row's item-level visibility in Go AFTER fetching — letting hidden rows in a granted-item's collection consume LIMIT slots. The refactor moves the precise predicate into SQL. New shape: type BacklinksVisibility struct { Unrestricted bool // admin / full-access member FullCollectionIDs []string // direct collection grants GrantedItemIDs []string // item-level grants } // SQL predicate when Unrestricted=false: // AND (s.collection_id IN (?...) OR s.id IN (?...)) This matches `guestResourceFilter` (which returns the precise primitives), so the handler now passes them straight through and drops the post-fetch filter loop entirely. Pagination is correct for guests, restricted members, and unrestricted users alike. New test: TestWikiLinks_ItemGrantPagination — guest with item-grant on ONE item in an otherwise-hidden collection sees exactly that one item; hidden siblings in the same collection do NOT leak in, and limit=2 returns 1 row (not silently shrunken). Other call sites updated: - TestWikiLinks_VisibilityAwarePagination → uses BacklinksVisibility{FullCollectionIDs: ...} and BacklinksVisibility{} for the no-access case. - 8 existing tests → BacklinksVisibility{Unrestricted: true}. - handlers_backlinks.go → no longer calls visibleCollectionIDs; uses guestResourceFilter exclusively and skips the Go-side filter. Verification: - make check clean - All TestWikiLinks_* pass Refs: TASK-1594, PLAN-1593 * fix(backlinks): scan EXISTS into bool not int for Postgres parity (Codex round 3) `SELECT EXISTS(...)` returns boolean on Postgres but integer 0/1 on SQLite. Scanning into `int` happened to work on SQLite (the modernc.org driver coerces) but would fail on Postgres — silently disabling the backfill short-circuit there and meaning upgraded Postgres installs wouldn't populate backlinks for pre-existing content until each item got edited. Fix: scan into bool. Both database/sql drivers in use (modernc.org/ sqlite and lib/pq) coerce their native representation into Go's bool, so this single shape works on both engines. make check clean. Refs: TASK-1594, PLAN-1593 * fix(backlinks): allow CommonMark 0-3 space indented fences per Codex (round 5) Round 5 flagged two edge cases in the code-stripping pass: 1. Multi-backtick inline code (``see [[X]]``) — traced through the parser; my permissive close-on-next-backtick logic already covers it correctly (range = [opener-start, after-closer-run]). Added a regression test to lock this in: TestExtractWikiLinks_CodeBlocksExcluded / "multi-backtick inline code excludes ref" 2. Indented fenced blocks — CommonMark allows 0-3 leading spaces of indentation before a fence opener (4+ spaces makes it an indented code block, a different construct). My fencedCodeRanges only matched fences at column 0, so ` ```\n[[X]]\n```` ` would render as code in the UI but leak a false backlink. Fixed both fencedCodeRanges (opener) and findFenceCloser (closer) to skip up to 3 leading spaces, with a hard cap at 4 (which would be indented-code, not a fence). Regression test: TestExtractWikiLinks_CodeBlocksExcluded / "indented fenced block (CommonMark 0-3 spaces)" Not addressed: - Round-4 escape-body parity finding. extract.go mirrors renderMarkdown's regex (web/src/lib/utils/markdown.ts:300), which is the actual render-time link parser; wikiLinksToMarkdown's more permissive escape grammar is editor-serializer-side and the renderer can't even consume its escaped output. Indexing what the user actually sees as a link is the correct invariant. make check clean. Refs: TASK-1594, PLAN-1593 * fix(backlinks): tilde fences + strict closer lines per Codex (round 6) Two CommonMark conformance gaps in the code-block stripping pass: 1. Tilde-fenced code blocks (~~~) were ignored. marked() treats them the same as backtick fences, so a [[REF]] inside a tilde block would render as code in the UI but leak as a false backlink. Fixed by parameterizing fenceChar across fencedCodeRanges and findFenceCloser, with separate handling for the backtick-specific "no backtick in info string" rule (CommonMark §4.5). 2. Closer-line strictness — CommonMark requires the closing fence line to contain only the fence + optional trailing spaces. The previous accept-any-fence-prefixed-line check would terminate a still-open fence prematurely on a line like ```not-closed, leaking later refs in the still-rendered code block. Refs reside in 4 new sub-tests under TestExtractWikiLinks_CodeBlocksExcluded: - tilde fence excludes refs inside - tilde fence with language tag - mixed fence types don't pair - closer-line strictness — backticks plus other text is not a closer - closer-line strictness — trailing spaces OK make check clean. Refs: TASK-1594, PLAN-1593 * fix(backlinks): inline code closer must match opener length per Codex (round 7) CommonMark §6.1 requires an inline-code span opened with N backticks to close on a run of EXACTLY N backticks. The previous "close on next backtick run of any length" logic would prematurely end the excluded range on a stray single backtick inside a ``...`` span, leaking any [[REF]] in the latter half of the code text as a false backlink. Concrete failure case: ``has ` inside [[X-1]] and more`` → old: range [0, 7], [[X-1]] indexed (bug) → new: range [0, end-of-closer], [[X-1]] excluded (correct) Fix: track the opener-run length and scan only for matching-length closer runs. Wrong-length runs in between are code text. Two new regression sub-cases under TestExtractWikiLinks_CodeBlocksExcluded: - inline code closer matches opener length — the main case - single-backtick span unaffected by adjacent multi-backtick run — asserts the opposite direction (opener=1 doesn't close on ``) Not addressed: - Re-flagged round-4/round-7 escape-body parity finding. extract.go intentionally mirrors renderMarkdown's regex (markdown.ts:300), not wikiLinksToMarkdown's more permissive escape grammar (markdown.ts:461). renderMarkdown is the actual link parser at display time; its regex rejects escaped-`]` bodies, so any link with an escaped `]` in its body is NOT shown as a clickable link in the UI. Indexing it would produce phantom backlinks the user can't see. The wikiLinksToMarkdown permissive grammar is paranoid serialization that the renderer can't consume — that's a pre-existing inconsistency in the editor pipeline, not a backlinks bug. make check clean (lint + tests + web build). Refs: TASK-1594, PLAN-1593 * fix(backlinks): rune-align snippet end-edge to keep UTF-8 valid (Codex round 8) The previous snippetAround() trimmed `start` to a rune boundary (so the leading edge of the snippet was always at a valid codepoint) but left `end` as a raw +40-byte clamp. When that landed in the middle of a multi-byte rune — common around emoji or accented text — the resulting slice was invalid UTF-8 and the JSON encoder would emit replacement characters in backlink snippets. Fix: same forward-advance pattern at the end as at the start. Continuation bytes (10xxxxxx) get skipped until we land on a leading byte. Going forward keeps the snippet anchored slightly past the match rather than slightly before it, which is a small UX win (emoji or accented text right after the link survives intact). Regression test: TestWikiLinks_SnippetIsValidUTF8 — pads body with enough 4-byte emoji on each side that the ±40-byte window cuts through one; asserts utf8.ValidString on the resulting snippet. make check clean (lint + tests + web build). Refs: TASK-1594, PLAN-1593 * fix(backlinks): inline code spans cross newlines, break on blank lines (Codex round 9) CommonMark §6.1: an inline-code span can cross single newlines but terminates at a blank line (a line containing no chars or only whitespace, which ends the enclosing paragraph). My previous scanner broke at every newline, so multi-line spans like `pre [[INSIDE-1]] post` would treat the opener as unclosed and leak [[INSIDE-1]] as a false backlink. Fixed by: 1. The newline branch in the closer scan now peeks ahead via the new isBlankLineAt() helper. Same-paragraph newlines are traversed; blank-line breaks terminate the span unmatched. 2. isBlankLineAt() treats any line with only space/tab as blank (mirroring CommonMark's blank-line definition). Three new regression sub-cases under TestExtractWikiLinks_CodeBlocksExcluded: - inline code spans single newline (CommonMark §6.1) - inline code breaks at blank line (paragraph boundary) - inline code breaks at whitespace-only blank line Trade-off: a truly-unclosed inline backtick now consumes from the opener up to the next blank line instead of just the rest of the line. False-positive on wiki-links in that span, but the surface area is small (unclosed backticks are rare in published prose) and matches the renderer's behavior. make check clean. Refs: TASK-1594, PLAN-1593 * fix(backlinks): accept escaped wiki-link bodies per editor grammar (Codex round 10) After 3 rounds of disagreement, capitulating on the escape-body parity finding. My position was technically correct for the CURRENT renderMarkdown behavior (which uses [^\]]+ and can't parse escaped- bracket bodies), but the editor's wikiLinksToMarkdown grammar at markdown.ts:461 explicitly produces such bodies — making the renderer's regex the inconsistent half of the pipeline, not mine. Mirroring the editor's grammar in the extractor makes the index forward-compatible: when the renderer eventually gets fixed, no change here is needed. The cost is a few "phantom" rows in the interim (indexed links the renderer doesn't currently display as clickable), but those are harmless and aligned with author intent. Changes: - wikiLinkPattern now uses `\[\[((?:\\.|[^\]\\])+)\]\]` — mirrors markdown.ts:461 verbatim. - New splitOnUnescapedPipe() helper — scans for the first `|` that isn't preceded by `\`. Mirrors splitWikiBody at markdown.ts:664. - New unescapeWikiBody() helper — undoes `\]`, `\|`, `\\` escapes in display text and key. Mirrors unescapeWikiBody at markdown.ts:657. - parseBody() now uses both helpers — split on unescaped `|`, unescape both sides. Regression coverage: - TestExtractWikiLinks_EscapedBodyChars (5 sub-cases): escaped `]`, escaped `|`, escaped `\`, non-escape backslash passes through, Position still points at opening `[[` despite escapes. - TestSplitOnUnescapedPipe + TestUnescapeWikiBody: direct unit tests for the helpers (round-trip safety vs the editor's escape/unescape pair). make check clean. Refs: TASK-1594, PLAN-1593 * fix(backlinks): preserve display text verbatim per Codex round 11 P3 The previous parseBody trimmed the display side of [[X|Display]] but the WikiLinkRef.Display contract promises verbatim storage and the renderer at markdown.ts doesn't trim either. Trimming would silently diverge on padded display text like [[TASK-1| spaces ]] (renderer keeps the spaces, extractor stripped them). Fix: drop TrimSpace from the suffix half of the split. Keep trimming the key/ref side because refPattern is anchored — a leading or trailing space in the key would force the body to fall through to the title kind even though the renderer resolves it as a ref. Regression test: TestExtractWikiLinks_EscapedBodyChars / "display text preserved verbatim (no TrimSpace)" make check clean. Refs: TASK-1594, PLAN-1593 * fix(backlinks): distinguish empty display override from no-pipe per Codex round 12 [[REF|]] (explicit empty display) and [[REF]] (no display) are distinct shapes in the editor: splitWikiBody returns displayOverride="" for the former, null for the latter, and the renderer uses `displayOverride ?? title` (nullish coalescing, NOT empty-string fallback) so "" is preserved. The previous extractor collapsed both into display_text=NULL, violating verbatim-display preservation for the empty-string edge case. Fix: - WikiLinkRef gains a HasDisplay bool. parseBody sets HasDisplay=true iff splitOnUnescapedPipe found a pipe; downstream uses HasDisplay (not Display!="") to decide whether to persist the override. - replaceWikiLinks in store: NullString.Valid is keyed off HasDisplay. display_text='' for explicit empty, NULL for no override. Regression coverage: - internal/links/extract_test.go: "explicit empty display override is distinguished from no pipe" - internal/store/wiki_links_test.go: TestWikiLinks_EmptyDisplayDistinct (two-source assert: NOT NULL for [[REF|]], NULL for [[REF]]) make check clean. Refs: TASK-1594, PLAN-1593 * fix(backlinks): pointer-typed DisplayText to preserve empty distinction over JSON (Codex round 13) Round 12 added HasDisplay on the parser side and made the store preserve display_text='' vs NULL on the DB row, but the wire model collapsed the distinction at JSON-serialization time: DisplayText string `json:"display_text,omitempty"` `omitempty` drops empty strings, so [[REF|]] (empty override) and [[REF]] (no override) serialized identically on the API and CLI JSON output. The end-to-end goal of round 12 wasn't reached. Fix: change DisplayText to *string. nil → no override (field omitted from JSON via omitempty), pointer to "" → explicit empty override (field present with empty value). The store's NullString.Valid drives the assignment, so the SQL round-trip matches the JSON shape. Knock-on: the CLI's `pad item backlinks` now dereferences the pointer and prints both populated and empty overrides ("displayed as: "). Regression coverage: - TestWikiLinks_EmptyDisplayDistinct extended to assert withBL.DisplayText is non-nil-pointing-at-"" and noBL.DisplayText is nil after a GetBacklinks round-trip. make check clean. Refs: TASK-1594, PLAN-1593	2026-05-23 23:09:44 -04:00
xarmian	de1beb47a9	feat(cli): pad library get + list --full + server-side category filter (TASK-1562) (#613) ... CLI layer for PLAN-1560 (`pad_library` MCP tool + matching CLI surface). Wires the HTTP work landed in TASK-1561 through to the `pad library` subcommands. ## `pad library list` changes - `--category` is now a server-side filter (the old client-side display-only skip-loop is dead and removed). - New `--full` flag. Default JSON output for playbooks now returns the `summary` field (first non-heading paragraph, ~240 char cap) instead of the full `content`; `--full` opts back into full bodies for callers that want to pipe everything. - Table output gains a summary hint line under each playbook and a `/pad <slug>` chip when an invocation slug is declared, so the library becomes self-documenting as a discovery surface. - `--type` now validates explicitly instead of silently producing an empty list for unknown values. ## NEW `pad library get <title>` Calls `GET /api/v1/library/entry?title=X` and renders either a conventions card (title, category, trigger, surfaces, enforcement, commands, body) or a playbooks card (title, category, trigger, scope, invocation slug, argument count, body). Conventions-first precedence matches `pad library activate`. JSON output returns the full envelope. 404 errors return a clean `not found in library: "<title>"` message with exit code 1. ## CLI client - `GetConventionLibrary(category)` — pass category as a server-side query param. - `GetPlaybookLibrary(category, summary)` — same plus the summary toggle; `summary=true` strips Content and returns Summary instead. - NEW `GetLibraryEntry(title)` returning `*LibraryEntryResponse`. - `LibraryPlaybook` gained an omitempty `Summary` field so a single type round-trips both the legacy and summary shapes. ## Drive-by Switched `/library/entry` 400/404 from a flat `{error: "..."}` body to the canonical `writeError(code, message)` envelope used by the rest of the API. The CLI's `parseError` now hands back a typed `APIError` that `pad library get` pattern-matches on `Code=="not_found"` for the clean 404 message. Updated `TestLibraryEntry_MissingTitle` and `_NotFound` to assert the new envelope. ## Verification go build / go vet / go test ./... all green. golangci-lint clean on cmd/pad/..., internal/cli/..., internal/server/.... End-to-end smoke tests via the installed binary confirmed: list summary mode, list --full, list --category filter, get convention card, get playbook envelope, get 404 exit-1, --type validation. Parent: PLAN-1560. Unblocks TASK-1563 (MCP catalog wiring).	2026-05-21 16:59:57 -04:00
xarmian	db87b47754	fix(cli): pin server URL in .pad.toml for remote workspaces (BUG-1535) (#595) ... * fix(cli): pin server URL in .pad.toml for remote workspaces (BUG-1535) Two fixes: 1. Replace stale api.getpad.dev references with app.getpad.dev in the --url flag help, NewClientFromURL doc, and Config.URL doc. Also fix internal/mcp/dispatch_http.go's comment to use the canonical mcp.getpad.dev/mcp URL. 2. Persist the server URL into .pad.toml when linking a directory to a non-local workspace. WriteWorkspaceLink now takes a serverURL arg; pad init / workspace link / workspace switch pass cfg.BaseURL() when Mode != local. getConfig() reads .pad.toml's URL as an override above ~/.pad/config.toml and below the --url flag, so commands like `pad collection list` from a remote-linked directory hit the right server without --url on every call. Passing --url explicitly also promotes local → remote so the directory pin is written even when the existing global config has mode=local. * fix(cli): scope .pad.toml URL override to client paths per Codex review (round 1) Round 1 review flagged that applying the .pad.toml URL override inside getConfig() contaminates server/admin commands: pad server start would advertise the wrong PublicLinkBaseURL, and pad auth setup would refuse to run locally because Mode flipped to remote. Extract the override into applyPadTomlOverride() and call it only from client-API entry points — getConfiguredConfig() and the pad init client phase. Server/admin commands (pad server start/stop, pad auth setup, pad auth configure) keep using raw getConfig() and are unaffected. Also skip the override when --url was explicitly passed (LoadedFromFlags), so the flag retains unambiguous priority. * fix(cli): preserve .pad.toml URL on workspace link/switch per Codex review (round 2) Round 2 review noted workspace link / workspace switch reached the server via getClient() (override applied) but then wrote the new .pad.toml URL using a raw getConfig() — which would drop or miswrite the url field when relinking inside a remote-pinned directory whose global config is local. Reuse the cfg returned by getClient() for padTomlURLFor so the write matches the API client.	2026-05-19 17:02:46 -04:00
xarmian	aec67e202e	feat(mcp): workspace.create + workspace.claim actions + claim-code mechanics (TASK-1521) (#582) ... Phase B for PLAN-1519. Adds two MCP actions so agents can bring a workspace into an OAuth connection without re-auth — the agent-first onboarding story IDEA-1517 §1 set out to fix. pad_workspace.action: create - New action on the shared catalog (stdio + cloud both pick it up). - POSTs to /api/v1/workspaces; handler auto-adds the new workspace to the calling OAuth connection's allow-list (added_by='agent-create') when the grant carries may_create_workspaces=true. Phase A wired the oauth_connection_workspaces table this writes to. PAT / CLI-session callers fall through silently (no request_id → no side effect). - Backed by a new non-interactive `pad workspace create <name>` Cobra command for the stdio MCP shell-out path. `pad workspace init` remains the guided human flow. pad_workspace.action: claim - New POST /api/v1/oauth/claim endpoint redeems a 6-digit stateless HMAC code minted from (user_id, workspace_id, 5-min time bucket) with a sliding 5–10 minute lifetime. Constant-time compare. Code format derived per IDEA-1517 §4. - Verifies workspace membership before code (privilege-escalation guard); uniform 404 envelope so the endpoint can't be used to probe existence vs. membership. - Side effect inserts a row in oauth_connection_workspaces with added_by='claim'. Idempotent — re-claiming returns 200 with already_added=true. - 412 connection_not_persisted when the OAuth grant predates Phase C (no oauth_connections row); 412 claim_disabled when the deployment hasn't wired SetClaimSecret. - `pad workspace claim <code> --workspace <slug>` Cobra command backs the stdio MCP shell-out. MCP server instructions - Appended IDEA-1517 §5 paragraph teaching agents the claim flow as a peer top-level section. Same string lands universally on every MCP handshake response (stdio + cloud both read instructions.md). Tests - 10 claim-code unit tests: determinism, zero-pad, length-prefix collision guard, current/previous bucket accept, aged-out reject, wrong-everything rejects, short-secret fails closed. - 7 handler tests: 412 when secret disabled, 400/404/401 vocabulary, PAT caller note path, 412 connection_not_persisted, idempotent insert. - 5 MCP-catalog tests: actions registered, schema params advertised, description mentions both actions, route mappers produce correct HTTP shape, routeTable carries the entries. - Bumped the existing read-only catalog bijection + fixture-input fixtures so the new actions resolve cleanly. Parent: PLAN-1519.	2026-05-18 00:43:46 -04:00
xarmian	f76520f6e7	feat(cli,mcp): expose 'collection delete' via CLI and MCP catalog (TASK-1511) (#573) ... * feat(cli,mcp): expose 'collection delete' via CLI and MCP catalog (TASK-1511) Mirrors TASK-1510 (collection update). The HTTP handler at handlers_collections.go::handleDeleteCollection already supported DELETE on a collection (owner-only, soft-deletes the collection and every item in it). Wires both agent-facing surfaces: - internal/cli/client.go: new DeleteCollection client method. - cmd/pad: new 'pad collection delete <slug>' Cobra subcommand (no --force; the help text is the confirmation contract). - internal/mcp/catalog_collection: 'delete' action passes through to the CLI; tool description updated. - internal/mcp/dispatch_http_routes: simple routeSpec entry for DELETE /api/v1/workspaces/{workspace}/collections/{slug}. No custom mapper needed — no body, no field coercion. Pairs with TASK-1510 as the second adaptation primitive for the /pad onboard playbook (TASK-1499): when the onboard interview discovers a seeded collection that doesn't fit the project, the agent now has a way to remove it before creating the right one. Tests: - TestRouteTable_CollectionDelete (route substitutes correctly) - catalog_readonly bijection + liveCmdhelpDoc fake updated. Parent: PLAN-1496. * docs: correct collection delete contract per Codex review (round 1) Two findings on PR #573 — both documentation, no code behavior change: 1. CLI Long help / Short blurb / MCP description claimed delete "removes seeded collections" and the onboard use case targets template-seeded collections. But store.DeleteCollection refuses any collection where is_default=true, and every template seed is is_default=true. The advertised use case wouldn't actually work. Updated docs to clarify: delete is for USER-CREATED collections; template seeds must be adapted via 'pad collection update'. 2. Both CLI help and MCP description claimed "AND every item in it" gets archived. The store delete path only sets collections.deleted_at and never touches items. The web UI hides them via the join, but raw API queries still surface them. Updated docs to be honest: items are NOT cascaded. Captured the underlying behavior limitation as a follow-up: IDEA-1513 ("Lift is_default restriction on collection delete or add a cascade-items option") — surfaces options 1-4 for lifting the guard plus the items-orphan issue. Parent: PLAN-1496, addressing Codex round 1 on PR #573 / TASK-1511. * docs: tighten collection delete contract per Codex review (round 2) Three P3 documentation-drift findings: 1. internal/cli/client.go::DeleteCollection Go doc still said "and all items in it" — missed it in round 1. Updated to describe the actual behavior (collections.deleted_at only; items orphaned with soft-deleted collection_id; is_default rejected). 2. CLI Long help and MCP description claimed restore is available "via the API," but there is no restore endpoint and no RestoreCollection client method. Recovery is database-backup only. Both docs updated. 3. catalog_collection.go:33 slug ParamDef only mentioned action=update; action=delete needs it too. And the headline description still said "list, create, and update" — three actions when there are now four. Both fixed. Parent: PLAN-1496, addressing Codex round 2 on PR #573 / TASK-1511. * docs: update pad_collection action lists in instructions.md + README (round 3) Codex round 3 finding: two top-level reference docs still advertised pad_collection as list/create only. internal/mcp/instructions.md is embedded into the MCP initialize() handshake instructions — stale guidance there means MCP clients miss update/delete entirely. README's catalog table had the same drift. Parent: PLAN-1496, Codex round 3 on PR #573 / TASK-1511.	2026-05-17 02:15:37 -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	4bbd0a210d	feat(collections): widen LibraryPlaybook with InvocationSlug + Arguments (TASK-1398) (#527) ... * feat(collections): widen LibraryPlaybook with InvocationSlug + Arguments (TASK-1398) Adds two optional fields to LibraryPlaybook: - InvocationSlug — kebab-case slug for `/pad <slug>` routing (PLAN-1377) - Arguments — argument spec mirroring the body's `## Arguments` section Both fields are tagged with `omitempty` so existing library entries (none of which set them) serialize unchanged. seedPlaybookFromLibrary() now forwards both into the seeded item's Fields JSON only when set, matching the shape ShipPlaybook() already writes. This is the foundational task that unblocks T2 through T6 of the playbook library overhaul. Parent: PLAN-1397. * fix(library): propagate invocation_slug + arguments through activation paths per Codex review (round 1) Codex round 1 caught that the activation paths for library playbooks rebuild the fields map and drop the new fields, so any library entry declaring invocation_slug/arguments would lose `/pad <slug>` routing after activation. Fixed in three places: - internal/cli/client.go LibraryPlaybook (the client-side mirror used by the CLI `pad library activate` command) - cmd/pad/main.go libraryActivate (CLI subprocess activation) - internal/mcp/dispatch_http_slice4.go dispatchLibraryActivate (MCP pad_project action=library-activate) All three now forward invocation_slug and arguments only when set, matching ShipPlaybook()'s shape exactly. Web client (web/src/lib/api/client.ts) activation payload is T2's explicit scope — left for that PR.	2026-05-13 00:26:09 -04:00
xarmian	fed4b60e65	feat(playbook): add pad playbook CLI (list/show/run) + endpoints (TASK-1382) (#520) ... * feat(playbook): add pad playbook CLI (list/show/run) + endpoints (TASK-1382) PLAN-1377 T5 — first-class invokable-procedure surface. Three HTTP endpoints + three CLI subcommands, with a strict CLI arg parser and a side-effect-free run path. Endpoints GET /workspaces/{ws}/playbooks — metadata array, same projection as bootstrap GET /workspaces/{ws}/playbooks/{ref} — full item, resolved by invocation_slug | ref | slug POST /workspaces/{ws}/playbooks/{ref}/run — bind args, return body + bound + unbound. No execution; the agent owns step playback. Resolution resolvePlaybook walks invocation_slug first (the /pad <slug> user-facing identifier), then falls back to ResolveItem (UUID / ref / slug). A stray TASK-5 hitting /playbooks/TASK-5 returns 404 instead of leaking a non-playbook into the surface. Arg parsing ParsePlaybookCLIArgs implements the strict rules from PLAN-1377: - Required positional args first, in declared order. - Flag types: bareword presence sets true. - Other types: key=value form (number is parsed as float64, enum is validated against declared options). The server takes either pre-parsed args (MCP / programmatic callers) OR raw CLI tokens (CLI caller); merge logic prefers explicit args over raw_args. The CLI sends raw_args, so there is one parser implementation and no drift risk. CLI pad playbook list — table or json pad playbook show <slug|ref> [--format] — markdown / json pad playbook run <slug|ref> [args...] — body + bound args Client method cli.Client.{ListPlaybooks, ShowPlaybook, RunPlaybook(args, rawArgs)} — runtime callers pass args; CLI passes rawArgs. Tests TestPlaybookList, TestPlaybookShowByInvocationSlug, TestPlaybookShowByRef, TestPlaybookShowRejectsNonPlaybook, TestPlaybookRunBindsArgs (with-args, missing-required, with-raw-args), TestParsePlaybookCLIArgsErrors, TestPlaybookListEmptyShape. Parent: PLAN-1377. * fix(playbook): tighten arg parsing per Codex round 1 (TASK-1382) P2.1 — Positional binding now skips optional and flag-typed slots. The PLAN-1377 contract says ONLY required args fill positional slots; other typed args must be key=value. Without this, a spec with an optional arg before a required one (e.g. [merge-strategy?, target!]) bound the caller's bareword TASK-7 to merge-strategy instead of target. P2.2 — number coercion now uses strconv.ParseFloat with NaN/Inf rejection. Sscanf(%g) was sloppy: it accepted '1abc' as 1 (partial match) and accepted NaN/Inf, which json.Marshal then refused after the handler had already written a 200 header. P3 — empty-body run requests now decode cleanly. The decodeJSON wrapper folds io.EOF into 'invalid JSON: EOF' so the previous err.Error() != "EOF" check never fired. errors.Is(err, io.EOF) on the unwrapped chain handles the wrapping correctly. Tests: TestPlaybookRunAcceptsEmptyBody, TestParsePlaybookCLIArgsOptionalNotPositional, TestCoercePlaybookValueNumberRejectsBadInput. Parent: PLAN-1377.	2026-05-12 18:29:11 -04:00
xarmian	24f0445efa	feat(bootstrap): single-roundtrip /pad context-load endpoint (TASK-1379) (#518) ... * feat(bootstrap): single-roundtrip /pad context-load endpoint (TASK-1379) Implements the agent bootstrap surface for PLAN-1377. One HTTP call replaces the four separate /pad context-loading invocations (workspace + collections + conventions + roles + playbooks) the skill used to make, cutting ~200-400ms of startup latency on every /pad command. Wire shape: - GET /api/v1/workspaces/{ws}/agent/bootstrap returns AgentBootstrap: workspace { slug, name, id }, user { name, email, id }, collections [...], conventions [...always-on, status=active], roles [...], playbooks [metadata-only — no bodies], dashboard {...}, recent_activity [... 24h]. - pad bootstrap [--format json|markdown] CLI wrapper. JSON is the canonical wire format that the /pad skill consumes; markdown is a human-readable summary for quick terminal inspection. Implementation notes: - Single source of truth: Server.BuildAgentBootstrap. The HTTP handler is a thin wrapper, and TASK-1380 will reuse it from three MCP surfaces (resource + set_workspace embed + pad_meta tool action). - Dashboard reuse: handleGetDashboard's body extracted into buildDashboardResponse(workspaceID, r) returning (*DashboardResponse, error). The HTTP handler is now a thin wrapper; bootstrap calls the builder directly to embed dashboard data without a second roundtrip. - Playbook bodies are deliberately NOT included — metadata only (~80 bytes/entry vs 5-10KB) so the bootstrap stays small for workspaces with many playbooks. Full bodies load on invocation. - Convention bodies ARE included for the always-on/active subset (must be agent-known up front); trigger-specific conventions stay load-on-demand. - Empty slices serialize as [] not null so the agent doesn't need defensive nil checks. Tests: - TestBootstrapEmptyWorkspace, TestBootstrapEmptyArraysNotNull, TestBootstrapIncludesPlaybookMetadata, TestPlaybookSummaryPrefersFirstParagraph. Parent: PLAN-1377. * fix(bootstrap): respect collection visibility + guest grants (TASK-1379) Codex round 1: BuildAgentBootstrap was bypassing visibility filters, so a guest admitted by RequireWorkspaceAccess could read collections, conventions, and playbook metadata they don't have access to. Now mirrors handleListCollections + handleGetDashboard: 1. Resolve visibleCollectionIDs(r, workspaceID) once. 2. Filter the Collections array through isCollectionVisible. 3. Gate the conventions + playbooks sub-queries on whether the caller can see those collections at all (presence in the filtered slice implies visibility). 4. Empty slices for inaccessible sub-resources, so the wire shape stays consistent — no missing keys to confuse the agent skill. 5. Pass-r=nil callers (future MCP in-process dispatchers) keep the 'full visibility' shortcut, but the doc comment now spells out that those callers MUST verify access out-of-band. Parent: PLAN-1377. * fix(bootstrap): apply guest item-level grants + recompute role counts (TASK-1379) Codex round 2: P1 — Item-level guest grants now flow into the convention + playbook sub-queries. visibleCollectionIDs alone admits the conventions / playbooks collection if a guest has ANY item grant inside it; without ItemIDs filtering, those queries then return the whole always-on convention body set or every playbook's metadata. Now mirrors the handleListItems shape: resolve (fullCollIDs, grantedItemIDs) via guestResourceFilter, and pass the (collIDs, itemIDs) tuple through to collectAlwaysOnConventions + collectPlaybookMetadata so a guest with a single grant only sees that one item. P2 — AgentRole.ItemCount is now recomputed from the visible item set for restricted callers, matching handleListAgentRoles. Without this, a guest could read role counts computed across all workspace items and infer hidden activity per role. Helper signatures updated to accept (collIDs, itemIDs); the doc comments explain the nil/non-nil semantics so future callers can't silently regress this. Parent: PLAN-1377. * fix(bootstrap): rewrite collection counts from visible set for guests (TASK-1379) Codex round 3: Collection.item_count + active_item_count are computed across the whole collection by ListCollections, so a guest with one item grant in a collection still received the collection in the filtered list but with hidden counts. Reuse the visible item set (already computed for role counts) to recompute collection item_count for restricted callers. active_item_count is set equal to item_count to avoid a separate done-rules buildup the bootstrap consumers don't depend on — better a self-consistent number than a leaked one. Parent: PLAN-1377.	2026-05-12 17:51:05 -04:00
xarmian	d915cc3cf8	feat(cli): per-server credentials in credentials.json with v1 → v2 migration (TASK-1228) (#435) ... Implements IDEA-1226. ~/.pad/credentials.json is now a map keyed by server URL so one developer machine can stay logged in to multiple Pad instances simultaneously — `apm/` repo on Pad Cloud, `target/` repo on local, `testing/` repo on staging — without each `pad init --url <other>` clobbering the previous server's credentials. ## On-disk format v2 (new): { "version": 2, "credentials": { "https://app.getpad.dev": {"token": "...", "user_id": "...", ...}, "http://127.0.0.1:7777": {"token": "...", "user_id": "...", ...} } } v1 (legacy, read-only): {"server_url": "...", "token": "...", "user_id": "...", ...} Reads transparently migrate v1 → v2 in memory; writes always emit v2. Side-effect-free reads — the on-disk file stays v1 until login/logout/ setup triggers a Save, which is when migration becomes durable. This keeps `pad <read-only-command>` from rewriting credentials.json on every invocation just because the binary upgraded. ## API Replaces the three top-level helpers (LoadCredentials / SaveCredentials / DeleteCredentials) with a CredentialStore type: - LoadStore() (*CredentialStore, error) - (s).Get(serverURL) *Credentials // nil-receiver safe - (s).Set(serverURL, *Credentials) - (s).Delete(serverURL) - (s).Save() error - WipeCredentialsFile() error // file-level — replaces DeleteCredentials URL canonicalization is built in: trailing slash + surrounding whitespace are stripped before lookup/store, so http://x:7777 and http://x:7777/ hit the same bucket. Same rule cmd/pad/server_info.go was already applying via its now-redundant normalizeURL — removed. No top-level `default` field. The configured server (cfg.BaseURL() from ~/.pad/config.toml or --url) is always the source of truth for "which server am I targeting" — a separate `default` would create a second source of truth and the split-brain bugs that follow. ## Behavioral changes - `pad init --url <other>` against a server you've authed to before now reuses the saved credential instead of clobbering it. - `pad auth logout` removes only the configured server's entry. Other servers' tokens stay intact (pre-fix: wiped the whole file). - `pad auth whoami` reads only the entry matching the configured server. - Single-server users see no behavior change — one entry, identical shape per entry, identical UX. ## Compat shims removed LoadCredentials / SaveCredentials / DeleteCredentials are deleted outright (no // Deprecated lifecycle) — they're internal package helpers with no external API contract. All 10 call sites in cmd/pad/ and internal/cli/ are migrated to the per-server API in this PR. ## Tests internal/cli/credentials_test.go (15 tests): - File missing / empty → empty store (callers don't need nil checks) - v1 format reads + migrates in memory - v1 with empty token → empty store (no phantom entries) - v1 migration is durable on first Save (file flips to v2) - v2 round-trip preserves multiple entries - Set adds + replaces; mirrors URL into ServerURL field - Delete keeps siblings (multi-server keystone behavior) - Delete on absent key is a no-op - Nil receiver Get/Delete don't panic (NewClientFromURL relies on this) - URL normalization (trailing slash + whitespace) - Save preserves all entries across the file boundary - Save uses 0600 permissions - WipeCredentialsFile removes the file + is idempotent - Garbage file errors loudly (so we never silently lose data) Existing tests unchanged. Full suite + lint + web-check green. Closes: TASK-1228. Implements: IDEA-1226.	2026-05-07 20:23:13 -04:00
xarmian	5e27989ab8	feat(attachment): add pad attachment view|show|list CLI surfaces (IDEA-898) (#321) ... * feat(attachment): add `pad attachment view|show|list` CLI surfaces (IDEA-898) Agents and CLI users had no first-class way to fetch attachment bytes through the API: the only path to read an `` reference was to read the raw blob out of `~/.pad/attachments/<storage_key>`, which bypasses workspace ACLs, doesn't work on Pad Cloud / remote / Postgres deployments, skips the variant pipeline (TASK-872 / TASK-879 / TASK-880), and breaks when storage moves to S3. Three new subcommands wrap the existing REST endpoints: - `pad attachment view <id> [-o path]` — agent-friendly: with no `-o`, fetches to a fresh OS temp directory using the stored filename and prints just the absolute path on stdout (so `$(pad attachment view <id>)` composes cleanly into shell pipelines). Reuses `download`'s atomic temp-then-rename pattern via a shared helper. - `pad attachment show <id>` — HEAD-based metadata only; surfaces MIME, size, filename, ETag, Last-Modified. - `pad attachment list [--item REF] [--category X] [--attached|--unattached] [--collection ID] [--sort ...] [--limit N] [--offset N]` — workspace list. The `--item REF` flag resolves a TASK-5-style ref to a UUID client-side and passes it to a new `item_id` query param on the list endpoint (server side: AttachmentListFilters.ItemID, ~6 lines in the store + 1 in the handler). Skill update: `skills/pad/SKILL.md` gains a "Working with attachments" subsection plus a CLI Reference entry, both ending in the hard rule that agents must NEVER read directly from `~/.pad/attachments/`. * style(cli): gofmt AttachmentListParams field alignment CI's golangci-lint v2 flagged this with the gofmt formatter (configured with simplify: true in .golangci.yml). The contiguous Sort/Limit/Offset block at the end of the struct needs uniform column alignment — gofmt considers the doc comment above Sort attached to that field rather than a block separator, so the three int/string fields get aligned together. Verified locally with `golangci-lint run --timeout=5m ./...` (v2.11.4 to match CI) — 0 issues. Local make lint only runs `go vet ./...` and `golangci-lint` wasn't installed, which is why this slipped through; filing a separate follow-up to mirror the CI checks in the local workflow.	2026-04-30 16:06:08 -04:00
xarmian	134f55045d	feat(attachments): import workspace bundle with rehydrate + UUID remap (TASK-885) (#306) ... * feat(attachments): import workspace bundle with attachment rehydrate + UUID remap (TASK-885) POST /workspaces/import now accepts a tar.gz bundle (Content-Type: application/gzip) and rebuilds the workspace + attachments + items in one round trip. JSON imports still work — content-type dispatch in handleImportWorkspace routes the request. Three-phase flow: 1. Walk the tar, capture pad-export.json + manifest.json + every attachment blob into memory. 2. Run the existing ImportWorkspace path to create the workspace + collections + items + comments + links + versions. New IDs are generated; item.slug is preserved (the existing remap path doesn't re-slugify). 3. For each manifest entry, rehydrate the blob through the storage backend (re-validate MIME + re-hash defensively, don't trust the manifest), insert a fresh attachments row. Build an oldID→newID map keyed on attachment uuid. 4. Walk every imported item's content + fields, replace "pad-attachment:OLD" with "pad-attachment:NEW" in one transactional pass. Refresh FTS afterward (direct UPDATE bypasses triggers). Phase 2 errors per-attachment are logged and skipped — the workspace keeps importing rather than rolling back. The import handler returns the new workspace and the operator can inspect logs for any attachment that didn't make it. CLI: - pad workspace export now defaults to --bundle (.tar.gz) since pad import handles bundles. --json reverts to legacy items-only. - pad import auto-detects format by file extension (.tar.gz / .tgz → application/gzip). Other extensions go through the legacy JSON path. - New Client.PostRawWithContentType for explicit-content-type POSTs. Tests: - TestImportBundle_RoundTrip: upload → embed in markdown → export source → import to FRESH server → verify attachment list has 1 row with new UUID → item content rewritten to new UUID and old UUID is gone → download new blob matches original bytes. - TestImportBundle_LegacyJSONStillWorks: JSON content-type still hits the legacy path. - TestImportBundle_RejectsBadGzip: garbage gzip body returns 400. Parent: PLAN-866. With TASK-884 + TASK-885 merged, the round-trip acceptance criterion (export → import → images intact) is met. * fix(attachments): stream import end-to-end per Codex (round 1) Two memory regressions Codex caught on PR #306: P1 (server). importBundle was buffering every blob into a map[string][]byte during a first pass, then iterating the manifest on a second pass. A 2 GiB bundle full of 25 MiB attachments would pin ~2 GiB of heap. Reworked to single-pass streaming: pad-export.json → import workspace + build slug→id map attachments/manifest.json → index entries by tar path attachments/<uuid>.<ext> → look up entry, rehydrate now The export bundler always writes pad-export.json + manifest.json BEFORE any blob (deterministic order from handlers_export_bundle.go), so this works without buffering. Bundles that violate the ordering — a third-party tool that writes blobs first — return 400 with a clear error. Memory footprint now bounded by the largest single blob (≤25 MiB) regardless of bundle size. Stale blobs without a manifest entry are skipped (their bytes io.Copy'd to io.Discard so the tar reader stays in sync). Unknown top-level entries (forward-compat for future bundle additions) are also consumed and ignored rather than left dangling. P2 (CLI). pad import used os.ReadFile, buffering the entire bundle client-side before posting. Switched to os.Open + a new Client.PostStreamWithContentType helper that streams the body directly into the request — together with the server-side fix, import is end-to-end streaming. Tests: - TestImportBundle_RejectsOutOfOrderTar: hand-crafted bundle with a blob before pad-export.json returns 400 with "ordering" in the message. - existing TestImportBundle_RoundTrip / LegacyJSONStillWorks / RejectsBadGzip continue to pass under the new streaming flow. * fix(cli): give streaming endpoints a 1h timeout per Codex (round 2) Codex P1 round 2: PostStreamWithContentType + RawStream were both using the shared 10s-timeout httpClient. The default works fine for normal API calls but kills a multi-GiB bundle import or export over anything slower than a local network — Client.Timeout fires mid-stream with "Client.Timeout exceeded". Added a dedicated streamClient on Client with a 1h timeout, used by both RawStream (export bundle download) and PostStreamWithContentType (import bundle upload). 1h is generous enough for ~100 MB/s uplinks shipping a 350 GiB bundle and still caps a hung connection eventually. The 10s default stays in place for every other call — short timeouts are the right SLA for normal API requests and protect the CLI from hanging on a wedged server. * fix(attachments): make import bundle cap configurable per Codex (round 3) Codex P1: the 2 GiB import cap was hard-coded with a comment promising operator override "later" — but no setter existed, so workspaces over 2 GiB stream out fine on export and fail on re-import. Added Server.SetImportBundleMaxBytes wired from the PAD_IMPORT_BUNDLE_MAX_BYTES env var in cmd/pad/main.go. Mirrors the existing PAD_ATTACHMENT_MAX_BYTES pattern. Default stays at 2 GiB so the typical workspace works without configuration; operators with larger exports can raise it without recompiling. The per-blob cap (importBlobMaxBytes = 25 MiB) is intentionally kept constant — it bounds in-flight memory regardless of total bundle size, and a 25 MiB-per-blob ceiling matches the upload handler's default, so a bundle can never smuggle larger blobs than the upload endpoint accepts. * fix(attachments): scale per-blob import cap with PAD_ATTACHMENT_MAX_BYTES per Codex (round 4) Codex P1 round 4: importBlobMaxBytes was hard-coded at 25 MiB but the upload handler's per-file cap is configurable via PAD_ATTACHMENT_MAX_BYTES. An operator who raised the upload cap to allow 50 MiB attachments could export a workspace successfully (WorkspaceAttachmentsForExport doesn't gate on size) but the re-import would reject every blob over 25 MiB. Replaced the const with effectiveBlobMaxBytes() which reads s.attachmentMaxBytes (or falls back to defaultAttachmentMaxBytes). The pad-export.json cap also scales with this value (4×) so a content-heavy workspace doesn't trip its own JSON ceiling on a server with raised attachment limits. Error message on a too-large blob now points the operator at PAD_ATTACHMENT_MAX_BYTES so they know which knob to turn rather than digging through code to find the cap. * fix(attachments): independent metadata cap for bundle import per Codex (round 5) Codex P2 round 5: tying pad-export.json + manifest.json caps to PAD_ATTACHMENT_MAX_BYTES regressed deployments that LOWER the attachment cap. A 1 MiB attachment cap would force metadata to fit in 4 MiB / 1 MiB respectively — but metadata size scales with workspace item count, not attachment blob sizes, so a tight upload limit shouldn't gate it. Added importMetadataMaxBytes = 100 MiB constant for both metadata files. effectiveBlobMaxBytes() still drives the per-blob cap which genuinely tracks attachment-upload policy.	2026-04-29 18:56:43 -04:00
xarmian	a0336e0248	feat(attachments): bundle attachments + manifest in workspace export (TASK-884) (#305) ... * feat(attachments): bundle attachments + manifest in workspace export (TASK-884) GET /workspaces/{ws}/export?format=tar streams a gzip'd tar bundle: pad-export.json # the existing WorkspaceExport JSON attachments/manifest.json # uuid → {filename, mime, size, hash, ...} attachments/<uuid>.<ext> # original blobs only — no thumbnails Default (no ?format) keeps returning JSON so existing automation hitting the endpoint without a query param continues to work unchanged. The CLI's pad workspace export now opts into the bundle by default; pass --json for the legacy items-only output. Implementation: - store.WorkspaceAttachmentsForExport returns originals only (parent_id IS NULL); thumbnails are re-derived on import via the existing pipeline so shipping them would double the bundle size. - handleExportWorkspaceBundle streams chunks straight into the response writer rather than buffering — a workspace with multi- GB of attachments would otherwise pin that much memory. - AttachmentManifest is versioned (separate from WorkspaceExport version) so the bundle layout can evolve independently. - bundleAttachmentPath is exported (lowercase package fn) so the import path in TASK-885 can resolve manifest entries to tar entries without duplicating the filename logic. - CLI gates against writing binary tar.gz to a TTY and appends the conventional extension when -o is passed without one. Tests: - TestExportBundle_RoundTrip: two uploads → bundle contains pad-export.json + manifest + 2 blobs whose bytes match the uploads + manifest decodes cleanly + WorkspaceExport decodes. - TestExportBundle_HidesThumbnails: synthetic thumbnail row, the manifest excludes it. - TestExportBundle_LegacyJSONStillWorks: no ?format param returns application/json with a decodable WorkspaceExport (backward compat regression guard). Parent: PLAN-866. TASK-885 (import path + UUID remap) consumes the manifest produced here. * fix(attachments): stream export bundle + revert default to JSON per Codex (round 1) Two findings from Codex on PR #305: 1. CLI buffered the entire response in memory via RawGet → io.ReadAll, defeating the server-side streaming design and risking OOM on a multi-GB bundle. Added Client.RawStream which copies the response body straight into an io.Writer; export now opens the target file and streams directly into it. 2. Default tar.gz output broke `pad export → pad import` round trip because the import handler still only accepts JSON. Reverted the CLI default to JSON; bundle is now opt-in via --bundle. The flag docstring notes that TASK-885 will flip the default once import handles bundles. * fix(attachments): surface tar/gzip close errors and truncation per Codex (round 2) Codex round 2 finding: deferred tw.Close() / gzw.Close() ignored errors. If a backend returned fewer bytes than size_bytes claimed, io.Copy returned nil, the tar writer's "missed N bytes" trip fired at Close, and the handler still completed a 200 OK with a corrupt bundle that gunzip would later refuse to decompress — silently from the operator's perspective. Two changes: 1. The deferred close now logs both tw.Close() and gzw.Close() errors with structured context, so a corruption-on-finalize trip shows up in the operator log. 2. streamAttachmentToTar checks the bytes-copied count against a.SizeBytes after io.Copy and returns a per-attachment error when they disagree. The error is logged with attachment_id + storage_key so an operator can correlate the corruption with the row to investigate. Regression test: TestExportBundle_TruncatedBlobLogsError forces a size_bytes/blob desync via direct UPDATE and asserts the resulting bundle bytes don't decode cleanly. (HTTP status stays 200 because headers are already on the wire by the time we detect the desync; that's an inherent limitation of mid-stream errors, but the new logs + close-error surfacing make the failure observable.) * fix(attachments): X-Bundle-Status trailer for export-stream success per Codex (round 3) Codex P1 round 3: even with the per-blob truncation log + tar/gzip close-error logs, mid-stream failures looked successful to clients. The CLI's RawStream finished without a transport error, the file landed on disk, and "Exported workspace" printed regardless of whether the bundle was actually complete. Two complementary signals now mark a clean stream: 1. HTTP trailer X-Bundle-Status. The handler declares the trailer in the initial Trailer header and sets it to "ok" only after tw.Close() and gzw.Close() both return without error. CLI checks the trailer after streaming and discards the file + returns error if it's absent or non-"ok". 2. The handler skips the deferred clean close on the error path, leaving the gzip footer unwritten. A client that ignores the trailer (curl, third-party tooling) still sees a corrupt gzip stream that gunzip refuses to decompress. CLI: pad workspace export --bundle now removes any partial output file on failure rather than leaving a corrupt one behind. Client.RawStream signature changed to return (bytes, *http.Response, error) so callers can inspect resp.Trailer; the only caller is the export command. Tests: TestExportBundle_TruncatedBlobAbortsStream now asserts both signals (trailer absent + gzip/tar can't fully decode), and TestExportBundle_SuccessTrailer pins the happy-path trailer.	2026-04-29 18:30:43 -04:00
xarmian	fc1c47f124	feat(attachments): CLI + TypeScript clients + types (TASK-873) (#290) ... * feat(attachments): CLI + TypeScript clients + types (TASK-873) Rounds out the API surface with Go and TS client methods + a \`pad attachment\` Cobra subcommand for ops debugging. internal/cli/client.go AttachmentUploadResult struct mirrors POST /attachments JSON. UploadAttachment streams a multipart file part via io.Pipe — never buffers the upload in memory. itemRef is optional. Uses a fresh http.Client with a 5-minute timeout per request so a 25 MiB upload over a constrained link doesn't trip the package-shared 10s default. DownloadAttachment streams the bytes into the caller's writer, returning Content-Type + total bytes copied. Optional ?variant= parameter for thumbnails (server falls back to original silently per TASK-872). cmd/pad/main.go pad attachment upload <item-ref|-> <path> [--filename NAME] pad attachment download <id> <out|-> [--variant thumb-sm|thumb-md] Item arg accepts an issue ref (TASK-5) or slug; "-" means no parent. Out arg "-" streams to stdout (with status messages on stderr) so callers can pipe into image viewers etc. Resolves the item via GetItem first so a typo'd ref fails fast with a useful error. List + delete subcommands intentionally omitted — those endpoints ship with TASK-881 (storage usage) and the future GC task. Adding client methods that hit 404s would mislead callers; same logic kept the upload response's "url" out of TASK-871 until TASK-872 wired GET. web/src/lib/types/index.ts Attachment interface mirroring the Go model (pointer types → optional). AttachmentUploadResult interface for the upload response shape. web/src/lib/api/client.ts api.attachments.upload(workspaceSlug, file, itemId?) — multipart POST via direct fetch (skips shared request() because that helper hard-codes Content-Type: application/json). Carries CSRF, cookies, and the same 401 → /login redirect. api.attachments.downloadUrl(workspaceSlug, attachmentId, variant?) is a pure URL builder so callers can wire <img src> directly without going through fetch. End-to-end smoke verified: pad attachment upload TASK-869 /tmp/tiny.png # uploads PNG pad attachment download <id> /tmp/dl.png # bytes are identical cmp /tmp/tiny.png /tmp/dl.png # PASS Verification go build ./... — clean go vet ./... — clean go test ./... — all packages pass cd web && npm run build — clean make install — server restarts on the new binary Parent: PLAN-866. * fix(cli): atomic download — write to temp + rename so a failed download doesn't truncate the destination per Codex review (round 1) P2: pad attachment download <bad-id> /existing/file used to wipe the existing file on auth/network/404 errors because os.Create truncated before the request was even attempted. The bytes were never written because DownloadAttachment errored out, but the destination was already 0 bytes — a footgun for anyone running the CLI in scripts. Fix: for the file-path case, write to a sibling .tmp via os.CreateTemp in the destination directory, fsync, close, then os.Rename only on success. Same atomic-write pattern as FSStore.Put. The defer cleans up the .tmp on any error path. The stdout case (outPath == "-") is unchanged — bytes already streamed to stdout can't be rolled back, so any partial write is just visible to the caller as a short payload. Verified end-to-end: echo X > /tmp/existing.png pad attachment download not-a-real-id /tmp/existing.png # errors cat /tmp/existing.png # still "X" — file untouched * docs(cli): clarify os.Rename atomic-replace behavior on Windows (Codex round 2 disagreement) Round 2 flagged this as P2: "os.Rename does not replace an existing destination on Windows." That is technically incorrect for modern Go. Verified directly against the Go stdlib source: src/internal/syscall/windows/syscall_windows.go: func Rename(oldpath, newpath string) error { ... return MoveFileEx(from, to, MOVEFILE_REPLACE_EXISTING) } MoveFileEx with MOVEFILE_REPLACE_EXISTING atomically replaces an existing destination on Windows. This has been the behavior since Go 1.5 (2015), so every version of Go this codebase supports already gets the desired replace-on-rename semantics on every platform. Added an inline code comment so future readers don't worry about the same false alarm. No code-path change.	2026-04-29 12:48:57 -04:00
xarmian	7cda0d7896	feat: rebrand to Perpetual Software + new tagline (IDEA-832) (#273) ... Migrates from xarmian/pad to PerpetualSoftware/pad across the entire repo and updates the product subtitle to "Collaborate with your AI agents". Go module rename - go.mod: github.com/xarmian/pad → github.com/PerpetualSoftware/pad - All Go imports updated across cmd/pad, internal/{cli,server,store, models,collections,items,events,metrics,webhooks} (~130 files) - Test fixtures with the literal repo slug ("xarmian/pad" in JSON shapes, SSH/HTTPS git URL strings, workspace_context fixtures) also updated, including the secondary repo entry (xarmian/pad-web → PerpetualSoftware/pad-web — pad-web was also moved to the org per branch context) Docs / config - README badges, install instructions, brew tap, Docker image, source build path, sponsor link (sponsor link kept as personal @xarmian) - Subtitle: "Project management for developers and AI agents." → "Collaborate with your AI agents." (README, manifests, web layout meta, .goreleaser homebrew description) - CONTRIBUTING.md, SECURITY.md, skills/INSTALL.md - .goreleaser.yaml: homebrew_casks owner, GHCR image, release github owner, cosign cert-identity regex, comments - .github/workflows/release.yml: tap/release comments - deploy/k8s/deployment.yaml: container image - docs/deployment.md: clone URL - web/static/{site.webmanifest,manifest.json}: description - web/src/routes/+layout.svelte: meta description + og:description Brew tap path is PerpetualSoftware/tap/pad (CamelCase, matches GitHub user case). GHCR image is ghcr.io/perpetualsoftware/pad (lowercased per GHCR's URL normalization). CODEOWNERS @xarmian and FUNDING.yml github: xarmian intentionally retained — those are the personal maintainer / sponsor account, separate from the org repo. Verification: go build ./..., go test ./... (all pkgs pass), web build, and make install all clean (TASK-844, TASK-845).	2026-04-28 12:26:39 -04:00
xarmian	9072e49b17	feat: add CLI commands for item starring (#120) ... Add star/unstar/starred CLI commands (PLAN-564, TASK-570): - pad item star <ref> — star an item - pad item unstar <ref> — unstar an item - pad item starred [--all] [--format json] — list starred items Client methods: StarItem, UnstarItem, ListStarredItems.	2026-04-14 20:51:03 -04:00
xarmian	7ca0463e70	feat: browser-based CLI authentication flow (#97) ... Replace the email/password terminal prompt in `pad auth login` with a browser-based auth flow. The CLI creates a pending session, prints a URL the user opens in their browser (works for localhost, remote VPS, or Pad Cloud), and polls until the session is approved. - Add CLI auth session endpoints (create, poll, approve) - Add browser approval page at /auth/cli/{code} - Rewrite `pad auth login` to use browser flow by default - Keep `pad auth login --interactive` as email/password fallback - Add login page redirect param support for post-login bounce-back - Add SQLite and PostgreSQL migrations for cli_auth_sessions table Closes PLAN-539, IDEA-404	2026-04-13 10:11:16 -04:00
xarmian	5606b22007	fix: address 6 security findings from Codex review of TOTP 2FA ... HIGH fixes: - Login-verify no longer accepts bare user_id. Now requires an HMAC-signed, IP-bound, 5-minute challenge token issued during login (prevents password bypass via known user ID + TOTP code) - Recovery codes are SHA-256 hashed before storage; plaintext is returned to the user once and never persisted MEDIUM fixes: - ConsumeRecoveryCode uses a DB transaction to prevent concurrent double-consumption of the same recovery code - EnableTOTP is atomic: WHERE clause requires totp_secret match to prevent TOCTOU race between setup and verify calls - /auth/2fa/login-verify now uses the strict Auth rate limiter (5 req/min/IP) instead of the general API limiter - CLI login detects requires_2fa response and prompts for TOTP code instead of silently saving empty credentials	2026-04-08 20:30:39 +00:00
xarmian	872f08aa84	feat: add compliance audit trail with IP/UA tracking ... Extend the activities table to capture IP address and user agent for all state-changing operations. Add audit events for auth (login, logout, register, bootstrap, password changes), workspace management (member invite/remove, role changes), token lifecycle, and admin settings. - SQLite migration recreates activities table with nullable workspace_id, new ip_address/user_agent columns, and relaxed CHECK constraints - PostgreSQL migration adds columns and drops constraints - New ListAuditLog store method with action/actor/workspace/date filters - GET /api/v1/audit-log endpoint (admin-only) - CLI: pad workspace audit-log [--days N] [--actor X] [--action X]	2026-04-06 01:55:06 +00:00
xarmian	be576d9e24	feat: agent roles — role-based (user, role) assignment for items (#58) ... * feat: agent roles — role-based (user, role) assignment for items (#PHASE-9) Introduce agent roles as a first-class concept for human-agent work assignment. Roles describe capability specializations (Planner, Implementer, Reviewer, etc.) and items can be assigned to a (user, role) pair, enabling natural handoff workflows between different AI tools. Migration: - New `agent_roles` table (workspace-scoped, slug-unique) - `assigned_user_id` + `agent_role_id` columns on `items` with FKs - Removed legacy `assignee` text field from Tasks schema Backend: - AgentRole model + full CRUD store/API - All item queries updated with LEFT JOINs to resolve assignment - Item list filtering by assigned_user_id and agent_role_id - Role transitions tracked in activity feed metadata CLI: - `pad role list/create/delete` commands - `--role` and `--assign` flags on item create/update/list - Assignment displayed in `pad item show` output Web: - TypeScript types + API client for agent roles - Role badge on item cards in list/board views - Assignment display on item detail page * fix: enforce workspace-scoped assignments and fail fast on unresolved --assign filter Addresses code review feedback from PR #58: P1: Add validateAssignmentScope() to the store layer, called by both CreateItem and UpdateItem. Verifies that assigned_user_id belongs to the workspace (via IsWorkspaceMember) and agent_role_id exists in the workspace (via GetAgentRole) before writing. Prevents cross-workspace assignment leaks. P2: The CLI `pad item list --assign <name>` now errors instead of silently returning unfiltered results when the member lookup fails or no workspace member matches the provided name.	2026-04-03 21:16:38 -04:00
xarmian	35f3dd1da4	feat(conventions): add structured metadata for TASK-133 (#51) ... * feat(conventions): add structured metadata for TASK-133 * fix(web): add workspace update type for CI	2026-04-02 18:39:51 -04:00
xarmian	b59f50982f	feat(auth): add local bootstrap setup for TASK-118 (#37) ... * feat(auth): add local bootstrap setup for TASK-118 * fix(auth): honor setup-required bootstrap flow	2026-04-02 05:13:17 -04:00
xarmian	a2a25b176a	refactor(auth): make setup state explicit for TASK-117 (#36)	2026-04-01 22:21:26 -04:00