mirror of
https://github.com/PerpetualSoftware/pad.git
synced 2026-09-23 11:03:41 +00:00
xarmian
2ed6e71ad3
* feat(store): transactional event outbox + events/1 item taxonomy (TASK-2658, SPEC-3)
Phase-0 unit 2 of PLAN-2656, store half. Events are now written to an
outbox in the SAME transaction as the mutation that produced them, so a
committed mutation cannot lose its event and a rolled-back one cannot
leak one. Nothing drains the outbox yet — behaviour is unchanged.
- migrations 081 / pgmigrations 059: event_outbox. Deliberately no FKs on
workspace_id / subject_id: an outbox row must outlive its subject, or
item.deleted cascades away exactly when it matters. Retention, not
referential integrity, bounds the table.
- internal/kernelevents: the closed events/1 name set (SPEC-3 v1.3) with
IsCanonical enforcing the closure rule at the choke point.
- store/event_outbox.go: writeOutboxTx (tx-scoped, hard-fails the
mutation rather than degrading to best-effort), the item payload shape
(snapshot EMBEDDED so query/1 predicates apply verbatim, prior_status
alongside as the envelope pseudo-field), and the drain-side primitives.
- item.created / updated / status_changed / moved / deleted / restored
emitted from inside their mutations' transactions, from in-tx snapshot
read-backs rather than caller input.
- SPEC-3 v1.3 disjoint-delta rule: canonical events partition a
mutation's delta and a mutation emits every event whose slice changed.
The seam diffs slices rather than branching on "was this a status
update" — branching drops the item.updated half of a mixed update.
- ImportWorkspace stays silent per the SPEC-3 ruling, commented at the
INSERT so it reads as a decision. insertItemTx's "every creation side
effect lives in this one place" comment corrected: it is API-path only,
and import is the counterexample two units have now been misled by.
* feat(store): comment / attachment / member events on the outbox (TASK-2658)
Completes the store half of the choke point. Same rule throughout: the
event is written on the mutation's own transaction, from an in-tx
read-back rather than caller input.
- comment.created / comment.updated. GetComment gains a Queryer form so
the emit reads through the tx: a pool read takes a different
connection and cannot see the uncommitted write, so it would return
the PRE-write row and the event would describe a state that is not the
one committing (mutation-verified).
- attachment.added, gated to user-visible originals. Variants are
attachment rows too — a thumbnail carries parent_id plus a variant tag
— so an ungated emit announces three events per image upload, two for
files no user added. Transform outputs stay admitted: no parent, and a
user did add them.
- member.joined. AddWorkspaceMember becomes transactional to carry it; a
self-committing INSERT plus a separate emit is the shape that loses
events on a crash.
item.bulk_updated is NOT here, and not by omission: bulk is a handler
loop over per-item store mutations, each already emitting canonically
from its own transaction. There is no bulk transaction to write it in,
so the batch event is delivery-side aggregation — it belongs with the
drain in TASK-2714, where SPEC-3's per-member binding evaluation is
already satisfied by the per-item rows.
* fix(store): emit item.deleted for a cross-workspace move's source archive (TASK-2658)
Self-caught during the diff review. archiveItemForCopyTx deliberately
REPRODUCES DeleteItem's UPDATE inside the copy's transaction rather than
calling it, so it did not inherit DeleteItem's new emit: a cross-workspace
move archived the source silently while an ordinary archive of the same
item announced itself. Invisible until something drains the outbox, at
which point moves would just stop being observable.
Same ordering as DeleteItem — snapshot in-tx BEFORE the UPDATE, while the
row is still live, because SPEC-3 requires the final pre-archive state.
Also amends the file's DR-14 header. DR-14 says no fanout inside the
transaction because a rollback would leak the event; an outbox row written
on the SAME transaction rolls back WITH the copy, so that rationale does
not reach it. The three things DR-14 actually names — activity row, SSE
publish, webhook — still happen post-commit at the caller, unchanged. A
documented decision should not be silently contradicted by the code.
* fix(store): compare the move's event slices against an in-tx pre-move snapshot (TASK-2658)
Codex round 1, P2 — a defect in my own round-1 code. MoveItemWithPreCheck
refreshes `existing` in-tx only on the precheck path; on the no-precheck
path it stays the PRE-LOCK pool read. The emit block compared it against
the post-move in-tx snapshot, violating a precondition documented on
itemUpdatedSliceChanged itself (both snapshots must come from getItemTx,
or rendering differences read as changes), and a stale CollectionID makes
the item.moved decision wrong outright.
Adds a dedicated `preMove` in-tx snapshot and tightens the read: it used
to tolerate a failure by silently keeping the pre-lock value, which only
degraded from_status. It now also decides which events fire, so a
degraded read is no longer an acceptable outcome — under a held lock on a
row just resolved live, an error or missing row means something is wrong.
* feat(store): item.bulk_updated for store-side bulk mutations; purge the outbox (TASK-2658)
Codex rounds 1 and 2. Two more item-mutation write paths emitted nothing,
and both are single-transaction bulk mutations, so their emits are WRITES
and belong in this unit rather than with the drain:
- collections.go: renaming a select OPTION rewrites items.fields on every
row carrying the old value.
- wiki_links.go: renaming an item rewrites the CONTENT of every item that
links to it by title.
Each emits ONE in-tx item.bulk_updated rather than per-row item.updated:
the user performed one action, and per-row fan-out is the flood TASK-1668
already decided against. Per-member snapshots keep item-level bindings
evaluable, which is what makes batching safe (SPEC-3 v1.1). Payload size
is deliberately unbounded in v1 — capping members silently drops binding
evaluation for the tail, and dropping `content` would break exactly the
bindings the wiki cascade exists for.
Also from round 2:
- Workspace purge now deletes event_outbox. It has no FK by design (a row
must outlive its subject), so nothing deleted it on the purge's behalf,
and payloads hold full item content and comment bodies — a purged
workspace's text would have stayed readable indefinitely. Added to
wsChildTables so the exhaustive-purge test covers it.
- Documented that ListPendingOutboxEvents is deliberately cross-workspace
and unauthorized, and must never be reachable from a request path.
- The two callers that discarded AddWorkspaceMember's error now log it.
Not fatal (that is BUG-2715), but this unit made the call transactional
and so gave it a new way to fail; widening a swallowed error without
making it visible is how a failure mode goes unnoticed.
* fix(store): classification correctness + dialect-neutral payload validation (TASK-2658)
Codex round 3, five findings.
A REAL SILENT-EVENT BUG in the classifier. The done-key mask ran
unconditionally, but the status machinery (extractFieldValue) only reads a
done-key value when it is a JSON STRING. So on a collection whose done
field holds a number, `{"stage":1}` → `{"stage":2}` produced NO EVENT AT
ALL: status_changed could not see it, and the mask deleted the key from
both snapshots so item.updated could not either. Now the key is masked
only when both sides hold a string there — exactly the condition under
which status_changed will describe it. When it will not, the change falls
back to item.updated's slice, where something can.
Payload JSON is now validated in Go. The column types DISAGREED: Postgres
JSONB rejects malformed JSON at the INSERT, SQLite's TEXT accepts it, so
the same bad payload failed a mutation on one backend and silently
persisted an undeliverable event on the other.
Corrected an overclaim of my own: the exclusion-list comment said a new
column is compared by default. True only of columns that reach
models.Item's JSON — last_restore_seq and the content-flush watermarks are
invisible to the diff no matter what the list says. Unreachable today
(every caller that moves them also writes content or fields), but not
structurally guaranteed, and now written down as a constraint on adding
persisted columns.
Tests: a custom done-field key (every previous classification test used
"status", so a classifier hard-coded to that key would have passed them
all), non-string and non-object blobs, malformed payload rejection, and
the bulk test now asserts member IDENTITY and the delta rather than a
count and a substring.
* fix: comment-accuracy sweep + no-op comment gate + enumerate the remaining discards (TASK-2658)
Codex round 4, aimed at the claims my own comments make. Three of them
were false or overclaiming, which is the point of pointing a review round
at your own prose.
- taxonomy.go and migration 081 described the END STATE — a drain loop, a
unified SSE/webhook vocabulary — as if it existed. Both now say plainly
that nothing drains the table, that the legacy hand-calls still fire
unchanged, and that the mapping and retirement are TASK-2714. A comment
describing the intended end state in the present tense is how a reader
concludes a feature is broken.
- The hop bound and the §L5 quota text read as running behaviour. Nothing
propagates a hop yet (no binding kernel), so every production write
leaves it 0 and the depth check is exercised only by tests. Said so,
and recorded the surfacing obligation as an obligation.
- The re-delete comment was wrong TWICE. The zero-row return exits before
the nil-snapshot guard, so that guard does not participate in re-delete
at all — it is what keeps this correct if the order or predicate ever
changes. My round-3 "correction" swapped one wrong mechanism for
another because I reasoned from a mutation result instead of the code.
Real behaviour fixes in the same round:
- A no-op comment edit no longer emits. The UPDATE matches on id alone,
so re-saving an identical body touched the row and emitted
comment.updated; the row-count check never suppressed it. Comparing the
body does, which also makes comment.updated consistent with the item
events.
- applyFieldMigrationsTx returns 0, not totalAffected, when emission
fails. Every error there rolls the caller's transaction back, so the
count described writes that never committed.
- Two MORE callers still discarded AddWorkspaceMember's error (the JSON
import and bundle import paths). Round 2 named two; I fixed those two
and did not enumerate. All nine call sites checked this time; the two
remaining discards now log.
Filed BUG-2716: the activity row commits before the comment and cannot be
reordered (the comment carries its id), so a failed comment write leaves
an orphan "commented" activity. Documented at the call site.
* fix(store): partition item.bulk_updated by the members' own workspace (TASK-2658)
Found in my own multi-tenancy probe while round 5 ran, not by the oracle.
emitBulkItemEventTx published every member under the workspace the CALLER
passed. For the collection-option rename that is right. The wiki-title
cascade is not so obviously safe: its source query selects on
target_item_id alone and carries each source row's workspace_id per-row
rather than assuming the renamed item's, so a member in another workspace
is not excluded by construction. That would have put one workspace's item
content on another workspace's webhook.
Whether it is reachable through today's queries is not the question worth
answering — "unreachable" is a property of the current query, not of this
function. Partitioning costs one map and makes it impossible.
Population, per CONVE-18: five emit helpers. Four derive the workspace
from the subject row itself (item, comment, attachment) or from the
membership being written (member.joined), so they are correct by
construction. One — bulk — took a caller-supplied id, and is fixed.
* fix(store): prior_status must be present on a transition FROM an empty status (TASK-2658)
Codex round 6, spec-conformance angle. SPEC-3 §Bindings makes prior_status
the envelope pseudo-field that lets a predicate filter "nonterminal →
terminal". An item can transition FROM no status at all — "" → "open" is a
real status change and item.status_changed fires for it — but `omitempty`
on a plain string dropped the key entirely, leaving a predicate unable to
tell "the prior status was empty" from "this event carries no prior
status".
Now a *string: nil on every event that has no prior status, and
present-and-possibly-empty on item.status_changed, where the empty value
is data. My original reasoning — that an empty string should never appear
"where a prior status is meaningless" — was right about the events where
it is meaningless and wrong about the one where it is not.
Also documents the bulk-snapshot read cost at itemSnapshotsTx rather than
leaving it to be discovered: N sequential joined reads under the caller's
lock, which roughly doubles an already-N-long hold (the migration loop it
serves already issues N sequential UPDATEs under that lock by design).
Batching it is BUG-2718; BUG-2717 covers the redundant post-commit re-read
on move and restore. Both spun off rather than folded, because each adds
an unreviewed path to a change that has been through six review rounds.
* fix(store): keep assignee name and email out of event payloads (TASK-2658)
Found in my own privacy-lifecycle probe while round 7 ran; round 7
independently reported the wider class.
An outbox payload is a frozen snapshot that outlives its subject by
design. Account deletion's de-identify pass (DeleteAccountAtomic) nulls
identity on LIVE rows so a departed user stops being legible — it cannot
reach a frozen payload. Every item event for an assigned item was
carrying the assignee's NAME AND EMAIL, and nothing drains or prunes the
table today, so those stayed readable indefinitely.
The rule applied, stated as a rule rather than a proxy: remove directly
identifying personal data, keep opaque identifiers and row state.
assigned_user_id stays — a predicate filters on it, and once the account
is gone it is a dangling reference to nobody.
Population enumerated rather than fixed one instance at a time (CONVE-18):
five payload shapes reach the outbox. Item-single and item-bulk carried
JOIN-populated name + email and are scrubbed. Comment (`author`),
attachment (`uploaded_by`) and member.joined (`user_id`) carry only their
own row's columns. Exactly one shape needed it, and what made it stand out
is that it was the only one carrying a join rather than the row.
* feat(store): comment.deleted + attachment.removed, ref-only (TASK-2658, SPEC-3 v1.4)
Round 7's privacy-lifecycle findings, resolved by adding the vocabulary
the conflict was missing rather than by deleting rows.
Without a delete marker, a hard-deleted subject's undispatched
created/updated rows were the ONLY record it ever existed — forcing a
false choice between dropping committed events (breaking the outbox
guarantee) and delivering deleted content forever. With one: the create
event still delivers, the deletion is announced REF-ONLY, and retention
prunes both. Privacy of a frozen payload is temporal, which makes the
drain load-bearing for privacy and not only for delivery (TASK-2714).
REF-ONLY is the contract, not a detail. A deletion event must not re-ship
what it deletes — the consumer needs to reconcile its model, not receive a
copy of what the user removed. Sharper for attachments, whose full
snapshot carries filename, content hash and STORAGE KEY: a locator for
bytes the system just reclaimed. Deliberately asymmetric with
item.deleted, whose subject is an archive and stays addressable.
- DeleteComment becomes transactional and emits comment.deleted. Refs are
read before the DELETE, because afterwards there is no row to read.
- ClaimSoftDeletedAttachment emits attachment.removed. The transaction
does not weaken the BUG-2415 claim protocol: the claim's conditionality
lives in the DELETE's WHERE clause, unchanged.
- ClaimNeverAttachedAttachment stays SILENT, deliberately. It reclaims
rows that were never attached to an item, and attachment.added fires
only for attachments written against a live item — so those rows never
announced their arrival, and announcing their removal would hand a
consumer a deletion for an id it has never seen. Tested as an asymmetry,
not left to inference.
- HardDeleteAttachment has no production caller; not wired.
No outbox row is ever deleted on subject death. That was my first
instinct and it was wrong: it trades a real durability guarantee for a
partial privacy one, through the privacy door.
* fix(store): make the attachment.removed gate symmetric with attachment.added (TASK-2658)
Codex round 8, and it falsified a claim I had written into the code as
verified one commit earlier.
I checked that never-attached implies never-announced — true, and the
verification stands: no path sets attachments.item_id back to NULL, and
every birth path producing a NULL item_id is non-emitting. Then I stated
the conclusion for BOTH directions, which does not follow. Rows reach
ClaimSoftDeletedAttachment having never emitted attachment.added by at
least three routes: VARIANTS (written silently because they carry a
parent, then tombstoned by their original's cascade), attachments cloned
by a cross-workspace copy, and attachments created by workspace import.
So the path announced removals for subjects no consumer had ever seen.
The emit now carries the SAME gate as attachment.added — a user-visible
original, attached to an item — so the two are symmetric by construction
rather than by argument. That closes the variant route, which is the
systematic one, and the test asserts the premise (the variant emitted
nothing on creation) before asserting the conclusion.
Residue, stated rather than papered over: an import- or copy-created
attachment still passes the gate while never having announced itself. The
failure mode is noise rather than harm — an unknown id in a delete is
ignorable, where announced-but-never-retracted would leave stale state —
and the cause is the deliberate silence of the import and copy paths.
Round 8 returned CLEAN on the ref-only payloads, the transaction wrapping
(contractually — it does broaden the SQLite writer-lock window, which is
inherent to making the delete and the emit atomic), scrubItemPII, and the
prior_status pointer.
* fix(store): derive subject_kind from the taxonomy instead of trusting the caller (TASK-2658)
Codex round 9, run explicitly as a convergence round — asked to find what
eight rounds would systematically miss rather than to re-check what they
covered. It found this, which is a fair answer to that question.
writeOutboxTx derived subject_kind only when the caller left it blank, so
a non-empty value was taken as given. subject_kind is a pure function of
the event name: a caller-supplied value can only agree with the taxonomy
or be wrong, and a wrong one persists silently and misroutes the event at
drain time — item.created stored as subject_kind "comment" would be routed
as a comment. Every existing test passed either the correct value or none,
which is exactly the blind spot that lets a defect survive review rounds
aimed elsewhere.
Now derived unconditionally. A caller that supplied a DIFFERENT kind
believes something false about the taxonomy, so that is an error rather
than a silent overwrite: correcting the row quietly would fix one write
and leave the belief in place.
* fix(store): stamp occurred_at rather than accepting it, and enumerate the rest of the class (TASK-2658)
Round 9 found that subject_kind was caller-trusted. Rather than fix the
named instance and wait for a review to name the next one (CONVE-18), I
enumerated the class: of the eight fields on OutboxEvent, event_type is
validated against the closed set, payload is validated as non-empty JSON,
hop is bounded, subject_kind is now derived, and id defaults but fails
LOUDLY on a duplicate. occurred_at was the remaining member with the same
shape of silent harm — SPEC-3 pins time-relative `within` predicates to
it, so a supplied value quietly changes how a predicate evaluates. It is
now stamped at write time; no caller sets it, and "the moment the event
was written" is the only honest value while the write is transactional
with the mutation.
That leaves workspace_id and subject_id as genuine caller inputs. Neither
is derivable, both are checked at their own call sites, and the bulk
emitter partitions by member workspace rather than trusting the one it is
handed. The enumeration is in the code so the next reader does not redo it.
* refactor(store): payload families, an honest helper name, proportionate comments (TASK-2658)
Codex round 10, run as a maintainability convergence round — read the diff
as someone who has to live with it for two years and did not write it.
Three findings, all fair.
PAYLOAD FAMILIES. The emitter helpers take an arbitrary event name and
writeOutboxTx validated only canonical MEMBERSHIP — so a caller could pair
item.created with a ref-only deletion payload and the write would be
accepted, having validated the half that was already obviously correct.
Each canonical event now declares its payload shape in the taxonomy, every
emit site declares what it marshalled, and the two are checked against each
other. The declaration is write-side only and never stored: the event name
already determines the shape, and persisting it would create a second
source of truth that could disagree with the first. A test walks the
canonical set so the two maps cannot drift.
HONEST NAME. itemSnapshotsTx is now outboxMemberSnapshotsTx, because it is
not a general "read these items" helper: it de-duplicates, silently skips
rows that no longer resolve, and scrubs assignee identity. Any of those
makes a general-purpose caller's result quietly incomplete rather than
wrong-looking, and the old name invited exactly that reuse.
PROPORTIONATE COMMENTS. Every canonical event now carries compact contract
documentation — comment.*, member.joined and pack.* had none, and pack.*
now says plainly that nothing emits it yet so a reader does not hunt for a
producer. In the other direction, three comments that had grown into
accounts of how I got something wrong are trimmed to the invariant and the
counterexample. The process belongs on the task trail and the identity
doc; the code should carry what is true.
* fix(kernelevents): one taxonomy table — round 10's family map could fail open (TASK-2658)
Codex round 11 BLOCKED on a defect round 10 introduced, which is the
review loop doing exactly what my own rule says it should: when a fix
introduces a mechanism, the mechanism needs the next round's attention
more than the original bug did.
The defect: writeOutboxTx discarded the ok from PayloadFamily. A canonical
event missing from the separate family map would resolve to the empty
family — which a caller declaring nothing then MATCHES. The check would
pass precisely when it had no idea what the answer should be, and the two
maps keyed on the same names were free to drift into that state.
Fixed structurally rather than by adding the missing ok test: subject kind
and payload family now live in ONE canonical table entry per event. A
second map is a second source of truth; co-locating makes the drift
unrepresentable instead of tested-for, and the compiler requires both
fields so a new event cannot arrive half-declared.
The fail-closed arm stays as a guard for a future table that separates
them again, and its comment says plainly that it is UNREACHABLE today —
verified by mutation: disabling it changes no test, because the mismatch
check catches every reachable case. A guard whose comment implies it is
the protection, when something else is doing the work, is the kind of
claim this unit has cost me several times.
The test now checks both directions: every canonical event resolves a
subject kind AND a family, and a non-canonical name resolves neither —
the second leg being the one that matters, since an unknown name must
report ok=false rather than an empty string a caller would match.