mirror of
https://github.com/PerpetualSoftware/pad.git
synced 2026-09-10 15:05:40 +00:00
xarmian
6a37512227
* 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.
59 lines
2.7 KiB
Go
59 lines
2.7 KiB
Go
package server
|
|
|
|
import (
|
|
"fmt"
|
|
|
|
"github.com/PerpetualSoftware/pad/internal/kernelevents"
|
|
)
|
|
|
|
// The SSE wire names, DERIVED from the events/1 taxonomy rather than written
|
|
// out again here.
|
|
//
|
|
// SPEC-3 §"The choke point owns the canonical→surface name mapping" is the
|
|
// reason this file exists: SSE's snake_case vocabulary and the webhook
|
|
// dot-form vocabulary drifted because nothing tied them together, each being
|
|
// hand-passed at its own call sites. v1.5 pins what "derive" means — NAME
|
|
// derivation, not delivery path. Webhooks deliver through the outbox drain;
|
|
// 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. What changes is only where the NAME
|
|
// comes from.
|
|
//
|
|
// DERIVED AT INIT, not per publish, and that is the whole safety argument. A
|
|
// canonical event with no SSE surface is a programming error at these call
|
|
// sites — every one of them is a compile-time constant — so the check belongs
|
|
// where a failure is impossible to miss and impossible to reach a user:
|
|
// process start. A per-request lookup would have to decide what to do with the
|
|
// "no surface" answer in a void helper, and every available answer (log and
|
|
// drop, publish under an empty name) is worse than not starting.
|
|
//
|
|
// Several canonical events derive the SAME name: status_changed and moved both
|
|
// surface as item_updated, because the SSE vocabulary is coarser than events/1
|
|
// and the UI has never distinguished them. The finer name is what the webhook
|
|
// wire and bindings receive.
|
|
var (
|
|
sseItemCreated = mustSurfaceSSE(kernelevents.ItemCreated)
|
|
sseItemUpdated = mustSurfaceSSE(kernelevents.ItemUpdated)
|
|
sseItemMoved = mustSurfaceSSE(kernelevents.ItemMoved)
|
|
sseItemArchived = mustSurfaceSSE(kernelevents.ItemDeleted)
|
|
sseItemRestored = mustSurfaceSSE(kernelevents.ItemRestored)
|
|
sseItemsBulk = mustSurfaceSSE(kernelevents.ItemBulkUpdated)
|
|
sseCommentCreated = mustSurfaceSSE(kernelevents.CommentCreated)
|
|
sseCommentUpdated = mustSurfaceSSE(kernelevents.CommentUpdated)
|
|
)
|
|
|
|
// mustSurfaceSSE resolves a canonical event's SSE wire name or panics.
|
|
//
|
|
// The panic is deliberate and it fires at package init, before the server
|
|
// listens: a canonical event that reaches here without an SSE surface means
|
|
// the taxonomy and this file disagree, and the only honest outcomes are
|
|
// "publish nothing" (silently breaking the live UI) or "refuse to start".
|
|
// Refusing to start is the one a person notices.
|
|
func mustSurfaceSSE(canonical string) string {
|
|
name, ok := kernelevents.SurfaceSSE(canonical)
|
|
if !ok {
|
|
panic(fmt.Sprintf("kernelevents: %q has no SSE surface name", canonical))
|
|
}
|
|
return name
|
|
}
|