mirror of
https://github.com/PerpetualSoftware/pad.git
synced 2026-09-21 18:13:26 +00:00
xarmian
6b5e8be04e
* fix(server): a structured timeline id must not collide with a row id (BUG-2783)
A note or decision id comes from the item's fields blob, which nothing
validates on write. It can equal the id of a comment, activity or version
on the SAME item, and then two entries in one timeline payload share an
id — which the client's keyed {#each} and its by-id page dedupe resolve by
hiding one of them. The intra-structured dedupe already prevented exactly
this failure one source-boundary in; `usedIDs` simply never knew about the
other four sources.
Fixed by seeding `usedIDs` from the ids the SQL sources already own. All
three slices are in hand at the call site one line above, so this needs no
extra query and no re-ordering.
The structured side is the one that yields, and that is a decision rather
than a side effect: comment, activity and version ids are real primary keys
AND are what the client sends back as `before_id`, so moving one would break
paging. The unvalidated side moves.
Tests drive the MERGED payload through the real endpoint rather than
structuredTimelineEntries, which is the reason the gap survived: the
builder is correct in isolation, so a unit test of it vouches for the
component and not its binding to the other sources.
Two things the assertions are deliberately specific about. They name WHICH
entry moves — "two entries are present" passes trivially even unfixed,
since the collision is resolved by the client rather than the server, and
"the ids differ" would pass if the comment had been the one to yield, which
is a different bug. And the activity case asserts the row that owned the id
is still in the payload, since the structured entry yielding is only correct
if the thing it collided with survives.
Also closes a pre-existing coverage gap this work surfaced: a mutation
deleting the `for usedIDs[id]` fallback-uniqueness loop survived the entire
timeline suite. It is now covered — and the fixture took two attempts, which
is the part worth recording. A note with no id followed by a note claiming
"note-idx-0" does NOT reach that loop: the second note's raw id is already
used, so the `!usedIDs[raw]` guard diverts it to the fallback branch and it
gets a free name. That fixture passed with the loop deleted. Reaching the
loop requires the fallback NAME to be occupied when the fallback is
computed — note 0 claiming the literal "note-idx-1", note 1 having no id.
Mutation matrix, all four compiling: dropping the seeding entirely, seeding
from comments only, seeding from activities/versions only, and dropping the
fallback loop are each detected, each by the test aimed at it.
Scope note recorded in the code: the fallback half is defence rather than a
live vector across sources, because all three SQL sources mint ids with
store.newID() — including the import path, which re-mints rather than
preserving an artifact's ids — and a UUID cannot equal `note-idx-N`. Within
the structured kinds, where both ids come from the same unvalidated blob, it
is reachable.
Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X
* fix(server): make the structured id independent of the page, not of the window (BUG-2783)
Codex round 1, P1, and it refutes the approach the recon proposed and the
lead ratified — reported rather than quietly redesigned, and replaced here
because the defect was mine and introduced by the previous commit.
Seeding the dedupe map from the ids fetched FOR THIS PAGE makes a structured
entry's id depend on the page window. The three SQL windows are cursor-
dependent, so the same note takes its raw id on a page where the colliding
row is absent and a positional id on one where it is present. That id is not
merely a render key: it is the SECOND TERM OF THE CURSOR PREDICATE, so a
window-dependent id makes an entry's own sort position depend on which page
is being built, and paging can then skip or repeat it. Worse than the bug it
was fixing, and a regression I introduced — before the previous commit,
structured ids were a function of the item alone.
Replaced with a SHAPE test: a raw blob id that is UUID-shaped is refused and
takes the positional fallback. That is sound because the only ids it can
collide with are comment, activity and version rows, and every one of those
takes its id from store.newID() — uuid.New().String(). Enumerated rather
than sampled: all six INSERT sites into those three tables pass newID(), and
the import path re-mints rather than carrying an artifact's ids across. So a
blob id can only equal a row id by being a UUID.
The result needs no extra query, consults nothing outside the item, and
changes ids only for blob ids that are well-formed UUIDs — which is exactly
the colliding case. A non-colliding UUID-shaped blob id also loses its raw
id; accepted, since it cannot be distinguished from a colliding one without
consulting the very rows this must not depend on.
Four more findings from the same round, all fixed:
- No version test. A fix knowing only about comments and activities passed
the whole file — which is what the seeding version did, since it
enumerated sources by hand. Added.
- The activity test called t.Skip when it found no activity. Creating an
item writes an activity row, so absence means the fixture stopped building
what the test needs, and a skip reports that as success forever. Now
t.Fatalf. Same for two skips in the new window test.
- The comments described the client as hiding one of a colliding pair. Half
true: WITHIN a page it is a keyed-each duplicate key, which is a Svelte
ERROR; the silent drop is the ACROSS-pages append filter. Both stated.
- "The other four sources" — there are three SQL-backed ones.
Mutation matrix, 5, all compiling: dropping the shape test, inverting it,
making the helper never fire, dropping the fallback loop, and dropping the
intra-structured dedupe are each detected.
The last one first looked like a survivor. It was not — it dies on
TestItemTimeline_StructuredDuplicateIDsAreDisambiguated, whose name my
`-run TestTimeline` filter never matched. A `-run` filter narrower than the
population is the same false-green family as a mutant killed by the
compiler: the harness reports SURVIVED and the reason is the instrument.
Re-run unfiltered before believing any survivor.
Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X
* fix(server): a diverted id must not inherit an unstable one (BUG-2783)
Codex round 2. Both findings are about the REPLACEMENT, not the original
bug, and the first is a hazard my own fix widened.
P1: the positional fallback encodes the entry's array index, so inserting
or removing an entry ahead of another renumbers it — and the entry id is
the cursor's tie-breaker, so a renumbered entry can be skipped or re-shown
across a page boundary. That instability PREDATES this work and applies to
entries with an absent or duplicate id. But the shape test diverted every
UUID-shaped raw id onto that same path, which would have taken a small
population and made it a much larger one, for no benefit.
Fixed by giving a shape-refused id a DERIVED id, `<prefix>:<raw>`, instead
of a positional one. It depends only on the entry's own raw id, so it is
stable under any mutation of the blob, and the prefix is what makes it safe:
a row id is a bare UUID, so `note:<uuid>` cannot equal one. It still passes
through the duplicate guard, because a blob may contain a literal
`note:<uuid>` string of its own.
What stays on the positional path is exactly the population that has
nothing else to derive from — absent ids and duplicates. That residue is
filed as BUG-2788 with the two consequences spelled out and three candidate
fixes, rather than being quietly absorbed here.
The new test asserts the property the derived id exists for: the same entry
keeps the same id after an unrelated entry is inserted BEFORE it. That
holds for a derived id and cannot hold for a positional one — the mutant
that removes the divert fails exactly this test and nothing else.
P2: the UUID-shape premise is a property of the WRITERS, not the schema.
All three tables are `id TEXT PRIMARY KEY` with no format constraint and
migrations carry ids verbatim, so a row whose id is not a UUID — from a
future path bypassing newID(), or already present in a database this code
has never seen — would be outside what this refuses. Latent rather than
reachable through any current write or import path. Now stated in the code,
including that the enforcement which would close it belongs at the writers
or the schema and not here, since detecting it needs exactly the row lookup
this design exists to avoid.
Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X
* fix(server): close the derived namespace, and pin its contract (BUG-2783)
Codex round 3. Both findings are about the round-2 replacement.
P1, and it falsifies a scoping sentence I had just written on BUG-2788: a
blob holding BOTH `note:<uuid>` and `<uuid>` had the second derive onto the
first's KEPT id, fail the duplicate guard, and fall through to the
positional path. Two distinct, legitimate raw ids, one of them made
unstable — and not covered by the absent/duplicate exception BUG-2788
describes, so that item's scope was wrong within the hour of being filed.
The fix generalises rather than patching the case. Divert any raw id that
could be confused with a derived one — UUID-shaped OR already beginning
with `<prefix>:` — and derive by prepending. Two rules then hold together:
a kept id never begins with the prefix, a derived id always does. So the
namespaces are disjoint BY CONSTRUCTION instead of by luck, two derived ids
are equal only when their raws are equal (the duplicate case), and a bare
UUID row id can equal neither form. BUG-2788's residue is unchanged —
absent and duplicate ids, the population with nothing of their own to
derive from — but now for a sound reason; corrected on its trail.
P2: the tests could not tell this fix from its alternatives. "Stable and
not the raw id" is satisfied by a constant; the collision tests' inequality
checks are satisfied by the very positional fallback this replaced. Two new
tests assert the contract instead of a symptom — the exact `<prefix>:<raw>`
form, distinct raws deriving to distinct ids, a prefix-carrying raw id
deriving to `note:note:<uuid>`, and no entry with a usable raw id landing
on `-idx-`.
The second new test closes a loop this design owed: a derived id is
SERVER-MINTED and the client sends it back as `before_id`, which BUG-2774
taught the server to refuse when the database would. Paging from a derived
cursor is now asserted to be accepted — a new id format must not be
refusable by our own validation.
Matrix now 9, all compiling: the three from this round are stop diverting
the prefix form (dies only on the contract test, confirming the hole was
real and is now covered), derive from a constant, and drop the divert
entirely.
Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X
* fix(server): reserve BOTH structured prefixes, not the calling one (BUG-2783)
Codex round 4, and it is the round-3 hole one kind across — which is the
part worth recording, because I fixed round 3's case and called the
namespace "disjoint by construction" while the construction only held
within a single kind.
Notes and decisions are numbered by separate entryID calls but share one
`usedIDs` map. Checking `prefix+":"` therefore checked only the CALLING
kind: a note whose raw id is `decision:<uuid>` was kept, and a decision
whose raw id is `<uuid>` then derived onto exactly that string, failed the
duplicate guard, and landed on the index-dependent positional path. Same
defect, same consequence, one substitution away from the case I had just
closed.
Fixed by reserving both prefixes globally — `hasStructuredPrefix` tests the
raw id against every namespace entryID mints in, regardless of which kind is
being numbered. The invariant is now what I claimed last round: a kept id
begins with no structured prefix, a derived id begins with one, so the two
sets cannot intersect.
The new test asserts forms rather than inequality, and then mutates the
notes array to show the DECISION's id does not move — which it could not
survive if the note had pushed it onto the positional path.
Matrix now 12, all compiling. The three added here: check only the calling
kind's prefix (dies on the cross-kind test), and dropping either "note" or
"decision" from the reserved list — each dies on a different test, so the
list is not covered by one case standing in for the other.
Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X
* docs(server): write down what the id scheme closes and what it does not (BUG-2783)
Codex round 5 answered the question I asked it — enumerate every way two
entries can share an id or one entry's id can change without the entry
changing — and returned no new defect. All three residuals were already
known: the positional-fallback instability is BUG-2788, and the two row-id
gaps are the latent writers-not-schema class already documented in
looksLikeRowID.
Recording the enumeration itself, because the next reader should not have
to re-derive it and because "everything else is handled" is exactly the
kind of claim that rots silently. Six closed cases named with the mechanism
that closes each; two open ones named with where to read about them.
One consequence from that round WAS new, and it is on BUG-2788: removing
the FIRST of two entries sharing a raw id changes the survivor's id KIND,
not merely its number — while both exist the second is pushed onto the
positional fallback by the duplicate guard, and once the first is gone the
survivor keeps its own raw id. Same skip-or-repeat consequence, reached
without any index moving. It also rules out that item's option (2):
deriving from content does not help, because two entries with the same raw
id still need a tie-break. Only persisting an id at write time makes an
entry's identity independent of what its siblings do.
Also extended looksLikeRowID's scope note with the second face of the same
gap: the three SQL sources keep their ids verbatim and are not deduped
against each other, so two rows in different tables sharing an id would
collide with no structured entry involved.
Findings by round: 5, 2, 2, 1, 0-new. No behaviour change in this commit.
Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X