8 Commits

Author	SHA1	Message	Date
xarmian	6a37512227	feat(server): outbox drain — webhooks delivered from the choke point (TASK-2714) (#1173) ... * test(store): pin the events/1 taxonomy as an independent copy (TASK-2714) TestCanonicalEventsAreFullyDeclared iterated kernelevents.Canonical() and asserted each entry resolved something non-empty. That check cannot fail for any table the compiler accepts: eventSpec requires both fields, so a corrupted table — an entry deleted, an entry added, item.deleted quietly rebased onto the ref-only payload — passed its own validation. A test that agrees with whatever the table says is not a test of the table. The sixteen name/subject/family triples are now written out as literals, so the test DISAGREES with the table when the table moves. The wire strings behind the name constants are pinned separately, because the triple map is keyed on literals and a renamed constant would otherwise slip through as long as the constant and the table moved together. Ordered as this unit's first commit because TASK-2714 edits that table (the handler-path bulk mapping): an independent copy earns its keep at the moment of the edit, not before. Mutation matrix, 4/4 caught: drop member.joined (17 -> 15 count mismatch and a missing-name error), rehome item.deleted onto ref_only (family mismatch), rename ItemMoved's wire string to item.move (constant leg), add an undeclared item.frobnicated entry (count + undeclared-name + non-canonical legs). The fourth reported "survived" on its first run because the sed never matched the table's alignment — the mutation was verified present in the file before the result was believed. TASK-2714 requirement 4 (lead pass on #1172). * feat(store): max-age prune for undispatched outbox rows (TASK-2714) Requirement 3's missing half. PruneDispatchedOutbox filters on dispatched_at IS NOT NULL, so a row that can never be delivered — a workspace whose only webhook was deleted, an endpoint that 4xxs forever — is unreachable by it and keeps its frozen payload indefinitely. That matters because SPEC-3 makes payload privacy TEMPORAL. An outbox payload is a frozen snapshot and account deletion's de-identify posture reaches only live rows, so the retention window is the whole privacy claim; a window only one of its two halves can close is not a window. The trade is stated in the doc comment rather than left to be inferred: at-least-once holds WITHIN the retention window and not past it, which is why the caller's max-age must be far larger than any retry schedule. Deleting rather than stamping the rows dispatched is deliberate — a dispatched stamp would be a lie in the durable record, and this table is the only evidence of what the kernel emitted. Mutation matrix, 2/2 caught: drop the dispatched_at IS NULL clause (prunes the aged DISPATCHED row too, handing retention two owners with different windows), drop the occurred_at cutoff (prunes a young pending row a retry is still owed). The test asserts its own premise — all three seeded rows are confirmed present before the survivor checks, which would otherwise pass for a reason unrelated to the prune. No caller yet: the drain loop wires it up in the next commit. * feat(events): derive SSE names from the taxonomy; retire item.updated_with_comment (TASK-2714) SPEC-3 §"the choke point owns the canonical→surface name mapping". SSE's snake_case vocabulary and the webhook dot-form vocabulary drifted because nothing tied them together — each was hand-passed at its own call sites. This ties them. v1.5 pins what "derive" means: NAME derivation, not delivery path. SSE stays direct-published at the mutation site, because it carries request-scoped attribution (Actor / ActorName / Source) that a frozen outbox payload deliberately does not hold; only its NAME now comes from the taxonomy. Moving SSE behind the drain is TASK-2722. - eventSpec gains an `sse` field — ONE table, not a second map, for the reason round 11 of the last unit established: a separate map can disagree with the first and fails open exactly when it matters. Empty is a real value (attachment, member and pack events have no SSE surface) and SurfaceSSE reports false for it, so silence can't be mistaken for a name. - Several canonical events derive the SAME SSE name — status_changed and moved both surface as item_updated — because the SSE vocabulary is coarser than events/1 and the UI never distinguished them. The finer name is what the webhook wire and bindings get. - The 12 canonical SSE publish sites take their names from derived package vars, resolved AT INIT. Every call site is a compile-time constant, so a missing surface is a startup panic rather than a per-request decision between "log and drop" and "publish under an empty name". - handlers_item_links.go keeps the events.ItemUpdated literal, commented: link mutations are silent in events/1 (v1.5), so there is no canonical name to derive from. TASK-2723 carries link.created / link.removed. - item.updated_with_comment retired (v1.2, Dave's ruling). One producer deleted; the events.ItemUpdatedWithComment constant deleted with it — it had no producer and no web consumer (grepped .go/.ts/.svelte), so leaving it would leave a name a future publisher could reach for. The compat guard is what makes this a refactor rather than a wire change: TestDerivedSSENamesMatchTheLegacyWireVocabulary asserts each derived name equals the events.* constant clients are pinned to. A derivation producing "item.created" or "item_deleted" would break the live UI while every other Go test still passed. Mutation matrix, 3/3 caught: rename item.deleted's SSE surface to item_deleted (both the taxonomy test and the compat guard fail), split item.moved onto its own SSE name (same), make SurfaceSSE return (spec.sse, ok) so no-surface events fail open (the taxonomy test's silence leg names all four). Running total 9/9. go test ./internal/server ./internal/store ./internal/events: all green. * feat(webhooks): synchronous DeliverEvent seam with per-endpoint outcome (TASK-2714) Requirements 1 and 2. Dispatch returns once its per-hook goroutines are spawned and reports nothing, so a drain built on it would stamp rows dispatched while the HTTP requests were still in flight — losing exactly the events the outbox exists to make unlosable. DeliverEvent blocks and tallies. - Delivery carries WorkspaceID / EventID / Event / OccurredAt / Payload. OccurredAt is the EVENT's timestamp, not dispatch time: SPEC-3 pins time-relative binding predicates to it, so stamping time.Now() would make every consumer's notion of when a mutation happened depend on how backed up the queue was. Payload is json.RawMessage — []byte would base64 the snapshot into a string that is valid JSON and completely unusable. - WebhookPayload gains ID, the consumer dedupe key SPEC-3 §Delivery guarantees already told consumers to use. Before this, that instruction named a field nobody could see. omitempty, because the "webhook.test" ping is not a kernel event, has no outbox row, and must not invent an id. - DeliveryOutcome counts rather than a status, because one event fans out to N endpoints and the answers differ. Three distinctions the drain branches on: Matched==0 is SUCCESS (a webhook-less workspace is owed nothing; reading it as undelivered would back up every event in every such workspace until retention deleted it); Permanent does not hold the event pending (re-sending to an endpoint that will reject it again costs the queue its progress); Transient does. Retryable() states the ack rule once instead of letting each caller re-derive it. - A returned error is reserved for the SERVER's failures — listing hooks, marshalling. Those must not ack: nothing was attempted, so the event is still owed in full. - Dispatch keeps its async shape for its one remaining caller and says so. deliver() now returns the outcome it always computed; the async path discards it. Mutation matrix, 6/6 caught: stamp dispatch time instead of occurred_at; drop the envelope id; pass the payload as []byte (base64); deliver asynchronously and assume success (the synchronous leg names it exactly); count a permanent rejection as transient; swallow a store failure into a zero outcome (the test prints the outcome that would have acked an undelivered event). Running total 15/15. go test ./internal/webhooks green. * feat(store): batch_id correlation for handler-path bulk mutations (TASK-2714) F2's write half. A lane-wide bulk action is a handler LOOP over per-item store mutations with no enclosing transaction, so each member writes its own canonical outbox row — which is what keeps SPEC-3's per-member binding evaluation free, and also means that without a marker the drain would put 200 item.deleted events on the webhook wire for a 200-item lane archive: exactly the flood TASK-1668's batch event exists to prevent. RECORDED, NEVER INFERRED (SPEC-3 v1.5). The schema-free alternative was grouping pending rows by workspace and a time window, which would fold two unrelated single updates into somebody's bulk event whenever they landed in the same tick. A wire event saying "these five items changed together" is only true if something recorded that they did. - migrations 082 / pgmigrations 060: nullable event_outbox.batch_id, no FK (a batch is not a row anywhere, it is a name the handler minted), plus a partial index on the pending set. - store.MutationOption / WithEventBatch: variadic, because every existing call site is a single-item mutation with nothing to declare and making all of them pass a zero value would bury the one case that matters. - The handler mints one id per bulk OPERATION, before the loop and unconditionally — deciding mid-loop whether a run "counts as" a batch would make the correlation depend on how far the loop got. POPULATION CORRECTED: my escalation said four store methods; it is FIVE. archive (DeleteItem), restore (RestoreItem), move (MoveItemWithPreCheck), field update (UpdateItemWithPreCheck) and assign (UpdateItem) are the complete set of mutating store calls handlers_items_bulk.go makes — restore was the one I missed, which is CONVE-18's exact lesson arriving one level up. The test drives all five rather than sampling, because the failure is per-method: a signature that accepts the option and never threads it compiles, passes everything else, and silently un-batches one of the six bulk verbs. Mutation matrix, 5/5 caught across the four distinct emit sites: drop the stamp on the update path (both Update legs fail), on delete, on restore, on move. The delete mutation first read as SURVIVED — it had made the package fail to BUILD (opt then unused), and the grep for test-level FAIL lines printed nothing. The compiler catch is the stronger result, but the instrument mis-reported it, so it was re-run with opt kept alive and the test named it directly. go test ./internal/store ./internal/server green. * test(server): anchor the SSE compat guard to the client's literal strings (TASK-2714) The guard compared the derivation against events.* — the Go side. A coordinated rename of the taxonomy AND the constants passes that, and is exactly the change that breaks the browser: the client is pinned to the STRINGS, in web/src/lib/services/sse.svelte.ts's ITEM_EVENTS. The wanted column is now a literal copy of what the client listens for, with the file named. events.* is asserted alongside as a second leg, so a drift between the Go constants and the client is attributed rather than merely reported. Same disagree-with-the-table principle as the taxonomy test, one layer out: this file has to be edited by hand when the wire vocabulary intentionally changes, and that edit is when someone goes and changes the client too. Mutation matrix, 2/2, each hitting only its own leg: rename events.ItemCreated to the dot-form with the taxonomy untouched (drift leg fires), and make the taxonomy publish the dot-form on SSE (browser leg fires). Running total 22/22. Lead's catch on the day-49 review of commit 33662da0. * feat(store): outbox claim protocol with lease and whole-batch claiming (TASK-2714) F3. Every instance of a cloud deployment runs the drain, so an unclaimed pending row is delivered once PER INSTANCE by construction. SPEC-3 permits duplicates — consumers dedupe on the event id — but "occasionally, after a crash" and "always, once per instance" are different promises, and only the first is one a consumer can budget for. - migrations 083 / pgmigrations 061: claimed_at + claimed_by, dialect-uniform conditional UPDATE (BUG-2415's orphan-GC protocol). Postgres FOR UPDATE SKIP LOCKED plus a separate SQLite path would be two implementations of one behaviour, only one of which runs where it matters. - claimed_at doubles as the lease: an instance that dies between claiming and dispatching must not strand its rows, and at-least-once is exactly what makes re-claiming safe. - BATCHES ARE CLAIMED WHOLE, past the limit. The limit is a throughput knob; letting it split a batch would make one bulk operation arrive as two wire events each reporting a partial member count. - MarkOutboxAttemptFailed RELEASES the claim rather than letting it expire. A transient failure means the event is owed and nothing is in flight; on a single-instance deployment the lease would otherwise be the only reason a retry ever waited. THE EXCLUSIVITY TEST WAS VACUOUS AND THE MATRIX CAUGHT IT. Removing the availability predicate from the claim UPDATE left it green: the candidate query already filters held rows, so single-threaded the end state is identical (CONVE-12 — another mechanism produces it). That implementation double-claims every row two instances select in the same moment, which is the entire bug. claimOutboxIDs is now split out so a test can drive the arbiter with a deliberately STALE candidate list, and the same mutation fails it by name. Mutation matrix, 4/4: drop the UPDATE's availability predicate (survived the first test, named by the race test); drop the batch expansion; keep the claim on a failed attempt; and the vacuity finding above. Running total 26/26. go test ./internal/store green. * feat(server): the outbox drain — claim, fold, deliver, retain (TASK-2714) The half of SPEC-3's choke point that turns stored events into delivered ones. 2a built the fill side; until this, the table filled and nothing read it. NOT STARTED YET, deliberately: the hand-called dispatchWebhook sites are still in place, so wiring the loop here would double-deliver every canonical event. Starting it is the next commit, together with deleting them — the unit's behaviour edge, kept as one reviewable diff. - Two declared payload shapes for item.bulk_updated (SPEC-3 v1.6). The store-side single-tx producers know every member at write time and embed snapshots; the handler-path HEADER knows the operation, the shared delta and the member refs, with snapshots living on the members' own rows. Declared rather than loosened: stuffing placeholder snapshots to satisfy a single-shape check would be a lie in the durable record, and dropping the gate would drop it on the one event with two producers. - EmitBulkHeaderEvent + bulkEventDelta: the delta is captured where it is KNOWN. By the time the drain sees member rows they carry post-mutation snapshots, and a diff of a snapshot against nothing is not a delta. - The fold: header plus whatever member rows of that batch are still undispatched. Members whose header is not in this claim deliver individually — not a fallback, the defined behaviour for the window between the loop committing and the header landing. batch_id is on the wire so a consumer can tie the singles to the batch. - Per-unit acking: a folded batch is many rows and ONE delivery, so a partially acked batch would re-deliver. - Retention runs every tick, both halves. The undispatched one is the privacy bound; PruneDispatchedOutbox looks like it covers retention until you notice which rows it can never see. TWO REAL BUGS THE TESTS FOUND, both in this commit's own code: 1. DEFAULTS APPLIED ONLY IN StartOutboxDrain. A tick reached directly ran with a ZERO undispatched max age, making the retention cutoff `now` and deleting the entire pending set on its first pass. Every test does this, and so would any future admin-triggered drain. Fixed by construction — one resolver both entry points call — with a refusal guard behind it. 2. THE GUARD'S FIRST TEST WAS VACUOUS AND PASSED WITH THE GUARD REMOVED. RFC3339 is second-granular, so a row written in the same second as a zero-window cutoff survives `occurred_at < cutoff` either way: the end state was reachable by another mechanism, and that mechanism was the clock. runOutboxRetention now returns its refusal so the test asserts the refusal rather than the survival, plus a positive control. Mutation matrix, 7/7 after the instrument fix: ack regardless of outcome; ack only when something succeeded (permanent failures would wedge the queue); fold without acking its members; drop members that have no header instead of delivering them; remove the retention guard; remove the resolver's max-age default. Two mutations initially read as survivors — one had failed to build, one met the vacuous test — and both are recorded above rather than counted as passes. Running total 33/33. go test ./internal/server ./internal/store green. * feat(server): deliver canonical webhooks from the drain, not from the handlers (TASK-2714) The unit's behaviour edge, kept as its own commit. The drain starts, and the nine remaining hand-called dispatchWebhook sites go: comment.created, comment.updated, item.created (x2 — plain and copy), item.updated, item.deleted (x2), item.moved, item.bulk_updated. Each was verified to have an outbox producer before its deletion, not assumed to. The Server.dispatchWebhook helper goes with them — it had no production callers left. Three copy tests used it as a probe and now call s.webhooks.Dispatch directly, which is what it did. WHAT CHANGES ON THE WIRE, stated plainly because "no behaviour change" would be false here: - TIMING. Deliveries were post-commit and inline; they are now up to one drain interval (5s default) later. In exchange a delivery survives a crash: the event is committed with the mutation it describes. - THE DISJOINT-DELTA RULE ARRIVES (SPEC-3 v1.3, ruled in 2a). A bare status flip now emits item.status_changed ONLY, where the hand-call always emitted item.updated. A mixed update emits both. This was ruled while the webhook surface has no known consumers; it is the same grounding as the v1.2 fold. - PAYLOADS. The envelope gains `id` (the dedupe key SPEC-3 already told consumers to use), and `timestamp` is now the event's occurred_at rather than dispatch time. Item snapshots come from the in-transaction read-back and are PII-scrubbed — the joined assignee name and email are gone, deliberately (see scrubItemPII: a frozen payload outlives account de-identification). - item.bulk_updated carries batch_id, the shared delta, and the member snapshots folded in from the member rows. Two copy tests needed real changes, not cosmetic ones: the DR-14 emission matrix they assert (which workspace hears what) is unchanged, but nothing arrives until a drain pass runs, and the fixture's own backlog — member joins, filler items — would otherwise be reported as the copy's output. The observer now drains once before the receivers are registered, which is what its "baseline" has always meant, and drainWebhooks runs a pass before collecting. go test ./internal/server ./internal/store ./internal/webhooks green. * fix: codex round 1 — unbatched bulk verbs, member dedup, comment overclaims (TASK-2714) THE P1, and it is CONVE-18 for the third time in this unit: batchID was threaded into the bulk helpers' SIGNATURES but not passed at three of the six store CALLS (set-priority/move-status via UpdateItemWithPreCheck, tag/untag via UpdateItem, move via MoveItemWithPreCheck). Those verbs' member rows stayed unbatched while the header was still written — N individual wire deliveries plus a header claiming they were a batch. My store-level test could not see it. It called the five store methods directly with the option, so it proved the option WORKS and said nothing about whether the handler passes it. TestBulkItems_EveryVerbStampsOneBatchID drives all seven legs through the HTTP handler and asserts every row of the operation shares one non-empty batch id with exactly one header. Reverting one stamp fails it by name (set-priority and move-status both). Writing that test also surfaced two legs that asserted nothing: untag and assign were no-ops in the fixture (no such tag; nothing assigned), so no member events existed at all. Both now perform real mutations, and the leg fails if fewer than two rows appear. Also from round 1: - FOLD DEDUPS MEMBERS. The disjoint-delta rule means one member can write two or three rows (a move that also changes status emits item.moved AND item.status_changed), so the folded payload listed the same item repeatedly while `count` reported ITEMS — the wire event contradicting itself. Keeps the LAST snapshot per id; an unreadable snapshot is kept rather than dropped. - BULK MOVE DELTA carries both collection and status when both were sent. bulkMoveCollection applies req.Status as a field override, so a move-with-status changes two things. - FIVE COMMENT OVERCLAIMS, all mine or inherited and all now matching the code: the taxonomy package doc still said nothing drains the outbox; "every event produces exactly one payload shape" predates the batch event's second shape; two places said "the dispatcher runs item-level selectors against each member snapshot" when no binding engine exists and the dispatcher filters on event NAME only; my own retirement comment said this path emits "item.updated + comment.created" transactionally, when the item half is whichever slice moved (a status-only update emits status_changed) and the comment is a separate transaction; migration 083 described the claim as one statement doing both the select and the mark. One finding recorded rather than fixed: affectedIDs counts rows TOUCHED, not rows semantically changed, so an all-no-op operation writes a header with a count and no members. Verified against origin/main — the webhook this replaces fired on the identical condition with the identical count, so it is inherited, and narrowing it is a wire change to count/item_ids that belongs with a contract version rather than a delivery refactor. Gates: build clean, make lint 0 issues, go test ./internal/... green, make test-pg exit 0 / 3463 PASS / 0 FAIL with the new outbox tests verified present in the Postgres run. * fix: codex round 2 — batch correlation on the wire, prior_status survival (TASK-2714) Round 2 was aimed at round 1's own fixes, and that is where both P1s were. - BATCH_ID REACHED ONLY THE FOLDED HALF. A member delivered individually — the window this whole design accounts for — carried an item snapshot with no batch anywhere in it, while three comments claimed consumers could correlate the singles with the batch. They could not. batch_id is now an ENVELOPE field on every delivery of a batched event, singles included, which is the only place a consumer can read it for a member. - FOLD DEDUP COULD DROP prior_status. A mixed update writes item.status_changed (carrying the transition) and item.updated (not); round 1's last-wins kept the later row and silently lost the one field a "nonterminal → terminal" binding needs, in exactly the case that produces both rows. The snapshot is still last-wins — every field IS fresher on the later row — but prior_status is carried forward, because it is envelope metadata only one of the two events ever has. - Sibling scans deduped: a 100-row candidate slice from one batch ran the same query 100 times. - The "only when something actually changed" comment on the bulk emission condition is corrected rather than left to be re-derived: the condition is that a row was TOUCHED without erroring. Untagging a tag nobody has succeeds on every row and changes nothing, so the header fires with a count while the store writes no member events. Same inherited asymmetry round 1 recorded; now the comment says it where the code is. - MY OWN COUNTS WERE WRONG IN THREE PLACES, which is the number-discipline lesson landing on documentation instead of a report: the handler test said "six verbs" while driving seven legs and "three of the six verbs" for what was three CALL SITES across four verbs; the store test implied it covered the verbs when it covers entry points, and now says out loud that it is not sufficient alone — round 1's bug lived one layer above it. Mutations, 2/2 on the new fixes: deliver singles with an empty batch id (the member leg names it twice, once per member); revert the dedupe to plain last-wins (the prior_status leg names it). go test ./internal/... green. * fix: codex round 3 — ack and release are conditioned on the claim (TASK-2714) The P1, and it is round 2's area again: claim tokens were minted and never checked. MarkOutboxDispatched and MarkOutboxAttemptFailed matched on the row id alone, so once a lease expired, a slow pass could still reach rows a newer pass legitimately owned — a late ack stamping a row the new holder is mid-delivery on, and a late release CLEARING a live claim and handing the event to a third pass. Reachable, not theoretical: a workspace's endpoints are delivered sequentially, each with three attempts and a 10s timeout, and the "well under a minute" estimate behind the lease default is an estimate rather than a bound. Both writes now carry the token and condition on claimed_by, and an empty token is refused outright rather than matching NULL. OutboxEvent carries ClaimToken so the drain never has to track it separately. A stale ack matches zero rows, which is exactly right — the event has become the new claim's problem. Also round 3, all P3: - The fold's "embedded VERBATIM" claim now names its one exception: a deduped survivor is re-encoded to carry prior_status across. Non-duplicate members are untouched bytes. - Four stale comments corrected where they live, not just where they were introduced: migration 081 still said nothing drained the table and webhooks fired from hand-calls; createItemChecked's summary still ended in "webhook dispatch"; handlers_watch_notify still called publishBulkItemsEvent "the SSE/webhook bulk path"; and two copies of the PII rationale said nothing drains or prunes, when the window is now bounded (bounded is not zero, which is why the scrub still does the work). Mutations, 2/2: drop claimed_by from the ack (the stale-ack leg fires), drop it from the release (the stale-release leg fires, naming the instance that took the freed row). Both mutations were verified present in the file first — the initial pair silently failed to apply and reported green, which is the third instrument mis-report this unit. Gates: go test ./internal/... green, make lint 0 issues. * fix: codex round 4 — retention spares live claims, token refusal is unconditional (TASK-2714) - RETENTION COULD DELETE A ROW MID-DELIVERY. Every instance runs retention, so one instance's prune could remove an old undispatched row another instance was actively delivering: the delivery would succeed while the ack matched zero rows, and a crash in that window loses an event the outbox had already committed. Live claims are now exempt, using the same lease predicate the claim itself uses. An EXPIRED claim stays fair game — that is what expiry means — and the test asserts both directions. - THE EMPTY-TOKEN REFUSAL SAT BEHIND THE EMPTY-ID SHORT-CIRCUIT, so MarkOutboxDispatched("", nil) returned nil: a contract that depended on the argument it was not about. Token check first. - The taxonomy comment claimed per-member events for ALL bulk mutations. True only of the handler path; the store-side single-transaction producers have no loop, and for those the snapshots INSIDE the payload are the only per-member view there is. Both mechanisms now named, since the distinction is visible in the payloads. - ListPendingOutboxEvents is documented as the diagnostic reader. The drain claims; a reader finding two pending-row queries should not have to guess which one production uses. Mutation, 1/1: drop the claim predicate from the prune (the new test names the count). Verified applied before the result was read. Round 4 also found a REGRESSION I am not fixing here because it is a fork: create-with-parent webhooks carry a pre-link snapshot. CreateItem writes the item.created row in its own transaction, SetParentLink runs in a separate one, and main's hand-called webhook dispatched the RE-READ item — so the parent and the post-link seq were visible then and are not now. Escalated with a recommendation (emit item.updated from SetParentLink's own transaction, which also covers the general case); it sits close enough to SPEC-3 v1.5's link-silence ruling that it is not mine to infer. go test ./internal/... green. * fix: SetParentLink emits item.updated on its own transaction (TASK-2714) Codex round 4's regression, ruled (a) by the lead with the F4 boundary made mechanical rather than inferred (SPEC-3 v1.6): a mutation that writes the ITEM'S OWN ROW emits item.updated; a relationship-graph link, which writes only the links table, stays silent. A parent link advances seq and flips the is_unparented bit, so it is on the emitting side of that line. The regression it closes: createItemChecked calls SetParentLink AFTER CreateItem has already committed item.created with a pre-link snapshot, then re-reads the item for its response. Main's hand-called webhook dispatched that re-read, so a consumer saw the parent and the post-link seq; under the drain the frozen created row was all there was, with nothing to correct it. created(pre-link) then updated(post-link) is a true history. Placed in setParentLinkOnce, not in the shared setParentLinkTx: UpdateItemWithParentLink reuses that core inside the item-update transaction and already emits from the field diff, so the shared site would double-emit. The snapshot comes from getItemTx, and the test enforces that rather than the comment doing it alone — mutating the read to the pool's GetItem fails on the seq assertion, because a different connection cannot see the uncommitted write and would emit the pre-link row under a post-link event. Mutations, 2/2: delete the emit (no event after linking); read the snapshot from the pool (seq is the create's). Plus a control leg asserting the PARENT emits nothing — the link does not write its row. go test ./internal/... green. * fix: codex round 5 — parent-only updates emit; the parent-emit claim is narrowed to the truth (TASK-2714) Round 5 aimed at round 4's own fix and found two P1s in it. Four-for-four on that angle now. 1. THE PARENT-ONLY UPDATE PATH STILL EMITTED NOTHING. SetParentLink's fix covers its own transaction; UpdateItemWithParentLink writes the hierarchy inside the ITEM-UPDATE transaction and emits from a snapshot DIFF — and a parent write leaves nothing in a snapshot to diff, since items.parent_id is legacy and untouched, the link lives in its own table, and seq/updated_at are excluded as metadata. So a fields_patch carrying only `parent` mutated the row and emitted zero events. The emitter now takes hierarchyChanged from the caller, which knows what it wrote; the diff cannot see it and must not have to. Covers set AND clear, with a control leg asserting a genuinely empty update still emits nothing — without it the fix could be "always emit", which would undo the disjoint-delta rule. 2. MY OWN ROUND-4 COMMENT AND TEST OVERCLAIMED. Both said the event carries a "post-link snapshot"; the payload is the item ROW, and the parent EDGE is not on it — IsUnparented is populated only by the local-first index queries, so the test's is_unparented assertion passed VACUOUSLY against an absent field. What the emit actually restores is the row change (a fresh seq and updated_at), which is exactly what main's hand-called webhook carried: it dispatched the handler's post-link re-read, the same scan. The comment now says that, and the test asserts the ABSENCE so the next reader cannot infer linkage data that has never been on this wire. Third instance this unit of the same shape: a partial verification written up as a complete one. Also round 5, both comment-level: - handlers_item_links.go said item-link mutations are silent in events/1. True per link TYPE, not per handler: parent crosses the "writes the item's own row" line and emits, blocks/blocked-by and implements do not. The comment now states the criterion and names the consequence — this handler publishes SSE for both kinds, so the SSE and events/1 pictures deliberately differ. - EmitBulkHeaderEvent's guard said "a bulk operation that changed nothing is not an event" while being an empty-LIST guard. Corrected in place with the reason it stays: the webhook it replaced fired on the identical condition with the identical count, so narrowing it is a wire change for a contract version, not a fix. Mutation, 1/1: drop the hierarchy force (the parent-only leg fails by name). Verified applied before the result was read. go test ./internal/... green. * fix: codex round 6 — parent DETACH emits on every route (TASK-2714) Round 6 aimed at round 5's fix and found two more in it. Five-for-five. 1. DETACH WAS SILENT ON TWO OF THREE ROUTES. Attach emitted from SetParentLink and from the update path, but ClearParentLink (its own transaction) and DeleteItemLink on a parent row (what DELETE /links/{id} actually calls) wrote the item's row and emitted nothing. A consumer's model would keep a parent the user had removed, with every attach route observable — the worst shape for this kind of gap, because the wire looks healthy. Routes are now enumerated in one test rather than sampled. 2. hierarchyChanged MEANT "PROVIDED", NOT "CHANGED". Clearing an already-unparented item deletes zero rows; round 5's flag still forced item.updated, putting an event on a public wire for a mutation that did not happen. clearParentLinkTx now reports whether it removed a link and the flag comes from that. The set branch stays unconditional — it is a DELETE-then-INSERT and bumps the row either way. IMPLEMENTS IS FLAGGED, NOT DECIDED. It bumps the same row (so the v1.6 mechanical criterion would include it) but it is a relationship-graph link (so v1.5's silence would exclude it). The contract does not resolve that case, and inventing an answer inside a delivery refactor is how a public wire acquires an event nobody ruled on. Recorded at the call site and raised with the lead. Mutations, 2/2: drop the parent-detach emit from DeleteItemLink (the route's leg fails); treat provided as changed on the clear branch (the no-op leg fails by name). The second mutation first failed to BUILD — `removed` then unused, which is the compiler catching it and my grep reading the empty result as a pass — so it was re-run with the variable kept alive. Fourth instrument mis-report this unit; all four are on the record. Gates: go test ./internal/... green, make lint 0 issues, make test-pg exit 0 on the pre-round-6 tip (re-run pending on the final tip). * fix: codex round 7 — the batch delta matches the mutation (TASK-2714) Round 7 returned no P1s; the parent-detach work from round 6 came back clean on transaction scope, lock ordering, error paths and duplicate emissions. - BULK DELTA REPORTED THE REQUEST, NOT THE COMMIT. bulkEventDelta echoed raw request values while the mutation normalizes: bulkTagUpdate trims added tags and skips ones that go empty, and the store's assignment SET clause gives a NON-EMPTY id precedence over the clear flag (BUG-2566). So a request with both an id and clear=true announced a clear while the row was assigned — the delta describing the opposite of what committed. This is the one field of the batch payload the drain cannot derive, so nothing downstream corrects it: whatever it says is what a consumer believes. Now normalized the same way, including untag matching RAW because the mutation removes by exact match. - THE implements COMMENT WAS WRONG, and it was mine from round 5: it listed implements with the silent link types when implements DOES bump the source row, exactly as parent does. Corrected in both places, and the case is stated as UNRESOLVED rather than settled — the mechanical criterion (writes the item's row) would have it emit, v1.5's relationship-link silence would not, and deciding it inside a delivery refactor would put an event nobody ruled on onto a public wire. With the lead. Mutations, 2/2: give the clear flag precedence over a non-empty id (the precedence leg fails, naming the row it would misdescribe); stop trimming added tags (the trim leg fails). The first mutation initially failed to build and was rewritten to compile before its result was believed. Gates on the round-6 tip: go test ./internal/... green, make lint 0 issues, make test-pg exit 0 / 3445 PASS / 0 FAIL with 13 of this unit's new test legs verified present in the Postgres output. Re-run pending on the final tip. * fix: codex round 8 — tag delta dedups, link SSE name derives (TASK-2714) Two P2s, both small and both the same shape: a claim in a comment that the code did not quite meet. - THE TAG DELTA TRIMMED BUT DID NOT DEDUPE, while bulkTagUpdate does both — it skips a tag already in its `seen` set. So tag ["foo", " foo "] added one tag and advertised two, under a comment saying the delta is normalized "the same way the mutation does". Round 7 fixed half of that sentence; this fixes the other half. - handlers_item_links.go PUBLISHED UNDER THE events.ItemUpdated LITERAL. The wire value happens to match, which is exactly why it was worth changing: it recreates the drift the central mapping exists to prevent, one rename away from being wrong. The name now derives like every other SSE site, and the comment separates the two facts a reader has to keep apart — the NAME derives, the events/1 EVENT still does not exist for relationship links. Mutation, 1/1: drop the `seen` check from the delta's tag loop (the dedup test names the duplicated value). go test ./internal/... green. * fix: codex round 9 — untag delta dedups, version restore derives its SSE name (TASK-2714) Both are the same shape as round 8's, one layer further out. - THE UNTAG DELTA STILL ECHOED DUPLICATES. bulkTagUpdate builds a removal SET, so ["foo","foo"] removes one tag; the delta advertised two. The two verbs normalize DIFFERENTLY — tag trims and dedups, untag dedups but matches raw, because removal is by exact string — and the delta now mirrors each side's own rule rather than applying one of them to both. - handlers_item_versions.go PUBLISHED A RAW "item_updated" STRING. A second source of SSE vocabulary, and the harder kind to find: it does not even reference the events package, so a grep for events.ItemUpdated misses it. Now derived like every other site. go test ./internal/... green. Gates on the round-8 tip: make test-pg exit 0, 3447 PASS, 0 FAIL, with 150 lines of this unit's own test legs verified present in the Postgres output.	2026-08-20 23:33:26 -04:00
xarmian	aeb80883f0	fix(webhooks): track delivery goroutines + bounded retry (BUG-2012) (#864) ... * fix(webhooks): track delivery goroutines + bounded retry (BUG-2012) Webhook deliveries ran in untracked `go d.deliver(...)` goroutines that write to the store — the BUG-842 shutdown-race class that goAsync was built to prevent — and had no retry. - Inject a `spawn func(func())` into Dispatcher (SetSpawn). Server wires s.goAsync via SetWebhookDispatcher so deliveries are tracked on s.bg (Stop() waits for in-flight deliveries) and inherit goAsync's panic recovery (BUG-2011). Nil spawn falls back to a plain goroutine, so standalone Dispatcher usage is unchanged. - Add a bounded in-goroutine retry: up to 3 attempts with linear backoff on transient failures (network error / timeout / 5xx). Permanent failures (4xx, SSRF block, malformed URL) stop immediately. The final outcome is recorded once via UpdateWebhookFailure. - Tests: delivery runs on the injected spawn; transient 5xx retries to the cap; permanent 4xx does not; a recovered transient records success. Claude-Session: https://claude.ai/code/session_019knGmnHcx5rrgWXQ8V8DZS * fix(webhooks): classify redirect-block + non-5xx as permanent (Codex review) Second Codex review of PR #864 found two retry-classification gaps: - P2: an SSRF-blocked (or looping) redirect surfaces as an error from client.Do (via CheckRedirect), which the retry loop treated as transient — so a redirect to an internal target was retried 3x with backoff. Wrap a sentinel (errRedirectRejected) in checkRedirect and match it with errors.Is (url.Error unwraps to it) to classify these as permanent — attempted once, no retries. - P3: the status switch treated every non-2xx/non-4xx as transient. Narrow transient to 5xx only; 4xx/3xx-no-Location/1xx are permanent, matching the stated "network error / timeout / 5xx" retry policy. Adds TestDispatcher_RedirectBlockIsPermanent. Claude-Session: https://claude.ai/code/session_019knGmnHcx5rrgWXQ8V8DZS	2026-07-08 12:02:49 -04:00
xarmian	ce46b6f190	fix(webhooks): enforce SSRF guard at dial time + widen deny-list (#836) ... The webhook SSRF guard only validated the literal URL string at parse time, so HTTP 302 redirects and DNS rebinding both reached internal IPs (cloud metadata, RFC1918, docker services). Enforce the guard at connect time: the delivery client's dialer Control callback re-checks the actual resolved IP before the socket connects (closing the DNS-rebind TOCTOU), and CheckRedirect re-runs ValidateWebhookURL on every hop with a redirect cap. Proxy is pinned nil so HTTP(S)_PROXY can't bypass the dialer's check. Widen isPrivateIP to also deny CGNAT (100.64/10), IETF protocol assignments (192.0.0/24), benchmarking (198.18/15), Class E (240/4), broadcast, all multicast (224/4, ff00::/8), and the TEST-NET / IPv6 documentation ranges. Fixes BUG-1993. Claude-Session: https://claude.ai/code/session_01BoPkYhKqMiWPYmxQigeWsA	2026-07-07 15:23:25 -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	157ca4e88f	chore: bump Go toolchain to 1.26 (TASK-763) (#247) ... * chore: bump Go toolchain to 1.26 (TASK-763) Bump Go from 1.25 to 1.26 across all toolchain pins: - go.mod — go 1.25.0 → go 1.26.0 - Dockerfile — golang:1.25-alpine → golang:1.26-alpine - .github/workflows/ci.yml — three setup-go steps (Go, Go-Postgres, E2E jobs) - .github/workflows/release.yml — release pipeline No `toolchain` directive: the repo is pre-launch with no external contributors yet, so we set the floor where we want it (hard requirement). Verified locally before commit: - golangci-lint v2.11.4 builds and runs under Go 1.26.2 (pinned in CI) - golang:1.26-alpine and 1.26.2-alpine images present on Docker Hub - go build ./... clean - go vet ./... clean - go test ./... all pass Parent: PLAN-644 (OSS Repo Hygiene and Launch Polish). * chore: gofmt -w under Go 1.26 (TASK-763) Apply Go 1.26's gofmt to the codebase. ~41 files reformatted, all struct-tag whitespace realignment — no semantic changes. Verified: - gofmt -l ./cmd ./internal returns empty after - go build ./... still clean - go test ./... still passes (run before commit) Bundling the gofmt diff with the toolchain bump in the same PR because the formatting drift is a direct consequence of moving from 1.25 to 1.26; splitting them creates a mandatory two-PR ordering for no value. Parent: PLAN-644. * docs: bump documented Go floor to 1.26 (TASK-763) Match go.mod's hard 1.26.0 requirement in the source-build instructions. Caught by Codex review round 1 on PR #247. - README.md:158 — "Go 1.25+" → "Go 1.26+" - CONTRIBUTING.md:9 — "Go 1.25+" → "Go 1.26+"	2026-04-25 11:35:19 -04:00
xarmian	e7f4448028	feat: add readiness probe and structured logging (TASK-161) ... - Add /health/live (liveness) and /health/ready (readiness with DB check) endpoints - Add Store.Ping() for database connectivity verification - Create internal/logging package using stdlib log/slog - Support PAD_LOG_LEVEL (debug/info/warn/error) and PAD_LOG_FORMAT (text/json) env vars - Add structured request logging middleware replacing chi's default Logger - Migrate all log.Printf calls to slog with proper levels and key-value attrs - Exempt health probe endpoints from auth middleware	2026-04-05 15:02:54 +00:00
xarmian	8aa6481421	PHASE-12: Security Hardening for Pad Cloud (#67) ... * feat: enforce RBAC role checks on all mutation endpoints (TASK-150) Add requireMinRole helper and role enforcement to 30+ mutation handlers. Viewers are now blocked from all state-changing operations, editors can mutate items/docs/comments/views but not collections/webhooks/workspace settings, and only owners can perform administrative operations. Includes 11 integration tests with real auth covering viewer/editor/owner access across items, collections, documents, comments, agent roles, item links, and workspace operations. * fix: scope search results to user's workspaces (TASK-151) Search without a ?workspace= param previously returned results from all workspaces in the database. Now the handler resolves the authenticated user's workspace memberships and passes their IDs to the store query, ensuring results only include items from workspaces the user belongs to. Fresh installs (no users) retain unscoped search for backward compat. Includes integration test proving cross-workspace isolation. * fix: add webhook URL validation and SSRF protection (TASK-152) Webhook creation now validates URLs before accepting them: only HTTP(S) schemes allowed, embedded credentials rejected, private/reserved IPs blocked (loopback, RFC1918, link-local, cloud metadata 169.254.169.254), and hostnames are DNS-resolved to verify they don't point to private IPs. Defense-in-depth check also added to the dispatcher's deliver function so existing webhooks with unsafe URLs are blocked at delivery time. * feat: add CSRF protection with double-submit cookie pattern (TASK-153) Implements CSRF middleware that validates X-CSRF-Token header matches the pad_csrf cookie on all state-changing API requests. Bearer token auth, auth endpoints, and fresh installs are exempt. The frontend client reads the CSRF cookie and attaches the header on mutations. * feat: add per-endpoint rate limiting middleware (TASK-154) Adds IP-based rate limiting for auth endpoints (5/min login, 3/hr password reset, 5/hr registration) and user-based limits for API (100/min) and search (30/min). Uses golang.org/x/time/rate with automatic stale-entry cleanup. Adds chi RealIP middleware for correct client IP behind proxies. Returns 429 with Retry-After. * fix: sanitize error responses and remove PII from logs (TASK-155) Replace all writeError(500, err.Error()) calls with writeInternalError that logs the real error server-side and returns a generic message to clients. Remove email addresses, user IDs, and password reset tokens from log output to prevent PII leakage. * feat: add security headers, configurable CORS, and secure cookies (TASK-160) Add SecurityHeaders middleware (CSP, X-Frame-Options, nosniff, Referrer-Policy, Permissions-Policy). Make CORS origins configurable via PAD_CORS_ORIGINS env var. Add PAD_SECURE_COOKIES for TLS deployments (sets Secure flag on session/CSRF cookies and enables HSTS). Also adds X-CSRF-Token to CORS allowed headers. * fix: address PR review — lazy router init and trusted IP for rate limits Fix two issues flagged by Codex: 1. CORS/HSTS config was ignored because setupRouter() ran in New() before SetCORSOrigins/SetSecureCookies were called. Now uses sync.Once to lazily build the router on first ServeHTTP/Listen. 2. Rate limiter read X-Real-IP directly from untrusted headers, allowing clients to spoof IPs. Now uses RemoteAddr only (which chimiddleware.RealIP already sanitizes from trusted proxy headers).	2026-04-05 10:26:00 -04:00
xarmian	86722d7607	feat: Add webhooks, saved views, enhanced dashboard, and dependencies backend ... Webhooks: - Full subsystem with HMAC-SHA256 signing, event filtering, auto-disable after 10 failures, test delivery endpoint - Migration 010_webhooks.sql, model, store CRUD, dispatcher with tests - Wired into item create/update/delete/move and comment create handlers Dashboard API: - active_items: Returns actual in-progress items with refs and priorities - active_item_count on collections (excludes terminal statuses) - Blocked item detection in attention (via dependency links) - isDoneStatus expanded to include cancelled/rejected/fixed/implemented Saved Views: - Store CRUD, API handlers, routes for per-collection saved views - View config stores filters, sort, view_type Activity: - Source filtering support (web/cli/agent) in activity list endpoint	2026-03-28 13:55:06 +00:00