mirror of
https://github.com/PerpetualSoftware/pad.git
synced 2026-09-20 17:43:26 +00:00
987fc79fdeadb12df8dffc736a76bd8a3a336a2c
755 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 xarmian
|
552230bbea |
fix(web): a cleared picker owes a refresh even on an unchanged scope (TASK-2877)
Codex review round 13, and it collapses round 12's fix into a simpler one. `lastScope` means "the scope the rows on screen answer for". The not-ready branch REMOVES those rows, so afterwards they answer for nothing — which is what null says, and the next run therefore owes a refresh whether or not the scope itself moved. Leaving the old value there meant rehydrating on the SAME workspace and collection compared equal, so a server-sourced picker took the early return and sat empty permanently: its rows were cleared and nothing was left to re-query it. Round 12 deferred the COMMIT past the early return to keep a cold-window scope change from being forgotten. With this invalidation in place that deferral changed no outcome — its mutant could not be killed — so it went and the commit moved back to where the value is computed. One rule stated once, rather than two mechanisms aimed at two halves of it. Also hardened the mutation harness, after it bit: a harness timeout kills the runner with SIGTERM, which does not run `finally`, so an earlier killed run left the working tree MUTATED. I then read a pre-existing test "failing" in that tree and had a plausible defect and a fix half-written before checking the file — the failure was M31's mutant, not my change. The runner now restores from its backups on SIGTERM/SIGINT/SIGHUP. Cheap, and the alternative is reasoning about code nobody wrote. Matrix: 35 mutants, all killed; baseline and restore both 97/97. |
||
|
xarmian
|
60fe815300 |
fix(web): a scope refresh stays owed until a run serves it (TASK-2877)
Codex review round 12, and the tail of round 11's fix. `lastScope` was committed as soon as the effect computed it, before the not-ready branch — which clears the picker and gives up WITHOUT serving the scope. So a scope change arriving while the workspace state is dropped was recorded as handled by the run that handled nothing: at hydration `scopeChanged` read false, a server-sourced picker took the early return, and it sat empty until the user retyped or it remounted. Committed now only by a run that is actually going to serve the scope. Leaving it stale is what keeps the refresh owed. Matrix: 35 mutants, all killed; baseline and restore both 96/96. The new one — committing `lastScope` early again — dies on the added leg. |
||
|
xarmian
|
268594e57e |
fix(web): a scope change re-queries a server-sourced picker too (TASK-2877)
Codex review round 11 — the tail of round 10's fix, and mine.
The refresh effect now tracks the scope, but it returns early for
server-sourced non-empty queries. That early return is right for an index
DELTA — the index is not that caller's source of truth, and a request per
delta is the rate-limiter pressure the debounce exists to avoid — and
wrong for a scope CHANGE, where the rows on screen are answers to a
different question and stay selectable under the new scope. A scope change
happens when a schema is edited or a pane is retargeted, not per delta, so
the rate-limiter argument does not reach it.
Two lines that looked like guards went, both measured rather than argued:
* `void collection` — the scope pair reads `collection` to build itself,
which IS the subscription, so the separate read added nothing and its
mutant could not be killed.
* the `lastScope !== null` first-run guard — at mount the query box is
empty, and the only reader of `scopeChanged` needs a non-empty query,
so the first run cannot change an outcome either way.
`lastScope` starts null rather than seeded from the props: seeding
captured their mount-time values outside a reactive scope, which
svelte-check flagged (`state_referenced_locally`) — two warnings this
branch introduced and has now removed. svelte-check is back to the six
pre-existing warnings in files this branch does not touch.
Matrix: 34 mutants, all killed; baseline and restore both 95/95.
|
||
|
xarmian
|
74d6564c4e |
fix(web): the picker's collection scope is a tracked input (TASK-2877)
Codex review round 10. The refresh effect read `collection` inside `untrack`, so a relation field whose declared target CHANGES under an open picker — a schema edit, or an SSE-driven collection refresh; `ItemDetail` does not remount the picker for either — kept listing rows from the collection it used to point at, still selectable under the new scope. Everything else in that effect is untracked to keep it off the keystroke path, and the scope was swept up in that. But `collection` is not a per-keystroke value: it is the question the results answer. Predates this unit — it arrived with the U3 extraction (TASK-2862) — and is fixed here rather than filed because U8 makes `collection` load-bearing in a new way: it is now the destination an inline create writes to, so a stale scope means rows from one collection listed beside a create row aimed at another. The test drives the change through a NEW single-prop setter on `ItemPickerProbe`, not through `rerender`. That distinction is the whole reason the probe exists, and its own header says so: `rerender` replaces the entire props object and re-runs the effect whether or not it tracks the prop under test, so a rerender-driven version of this test passes against the untracked build. Verified rather than assumed — the mutant that restores `untrack` dies against the setter version. Matrix: 32 mutants, all killed; baseline and restore both 94/94. |
||
|
xarmian
|
ded64ce232 |
docs(web): record why three races are deliberately not fenced (TASK-2877)
Codex review round 9, two P1s, both declined — and the reasoning goes
beside the fences rather than into a commit message, which is the lesson
round 7 taught when a round-3 decline was re-raised because a reviewer
reading the diff had no way to see it.
A CONCURRENT FIELD CHANGE (SSE, another tab) landing mid-POST is ordinary
last-write-wins on a field the user is actively editing, and it is what
every other type in this component already does — a text field blurred
after a remote change overwrites it too. The race is adjudicated at the
server: `ItemDetail.updateField` sends `expected_updated_at` and
refetch-retries a 409 (BUG-2273 / IDEA-1480). Fencing it here would make
relation fields alone behave differently from every other field, on a rule
the item's own optimistic-concurrency check already enforces.
A LOST RESPONSE on a create that committed is real and is not fixable
here. `item create` has no idempotency key and titles are not unique
(colliding slugs get `-2` suffixes, `store.uniqueSlug`). Nothing
auto-retries — a retry is a person clicking Create again with the picker's
state in front of them — and the repo's standing rule for the identical
shape is exactly that ("Never retry it automatically" for `item copy`).
Filed as IDEA-2880. Deliberately NOT patched client-side: checking for a
same-title item before retrying would rest on the same ranked, paged,
possibly-stale evidence the create row itself rests on, and would look
like a guarantee the client cannot make.
No behaviour change; gates re-run rather than assumed — 2104 web tests,
svelte-check 0 errors.
|
||
|
xarmian
|
c516871328 |
fix(web): read page completeness from the page, not from total (TASK-2877)
Codex review round 8, one P1, and the mechanism checks out in the server
source rather than only in the abstract.
Round 7 gated the cold answer on `(res.total ?? rows.length) <= rows.length`.
`store.search` makes that unreliable in exactly the case it was guarding:
when the count query errors it sets `total = -1`, floors it to 0, and then
floors it again to `len(results)` — "Ensure total is never less than actual
results", `internal/store/search.go:604-608`. So a broken count is
indistinguishable on the wire from an exact-fit page, and the check calls
it complete. The `?? rows.length` fallback was the same mistake a second
time: unknown read as fine, which is the polarity error rounds 3 and 5
already went around on `coldFailed`.
Completeness now comes from the PAGE: a page SHORTER than the limit the
server echoes back is proof there is no next page, and that holds whatever
the count did. A full page is not proof either way, so it does not count
as an answer. No `total` in the decision at all.
The U8 fixtures now carry the real response shape. `total`, `limit` and
`offset` are non-optional on `SearchResponse` and the Go handler always
sends them, so `{ results: [] }` was not a smaller version of a real
response — it was one that cannot occur, and it was quietly deciding the
very question these tests are about.
Matrix: 31 mutants, all killed; baseline and restore both 93/93. M28b —
the previous `total`-based implementation — SURVIVED at first, and the
fixture was why: it asserted against `total: 84, limit: 2`, which both
implementations reject. The leg that discriminates is the floored one
(`total: 2` on a full page of 2 with 84 really matching), i.e. the shape
the server actually emits when the count fails. A mutant that survives
because the fixture never reproduces the real failure is a fixture
finding, not a code finding.
|
||
|
xarmian
|
ff44e917ae |
fix(web): count the 401 drop; a truncated page is not an answer (TASK-2877)
Codex review round 7. Two taken, one answered in the code. P1 — `resetGenerationFor` counted `reset()` and missed the OTHER drop. `bootstrap()`'s unauthorized/forbidden branch clears `state.items`, resets the MiniSearch index and wipes the persisted cache without going through `reset()`, so the fence added in round 6 did not see the revocation case it exists for. Both droppers now call one `markWorkspaceDropped(ws)` helper. Two call sites, because deleting the state entry and clearing rows in place are genuinely different operations; the helper is what makes the pairing greppable, and a test fails if a third site starts clearing rows without it. That test is STRUCTURAL, and deliberately so. Reaching the 401 branch through the front door needs a warm cache plus a pending resync plus a 401 from /items-changes — a fixture larger than the invariant it would check, and I tried it first. The invariant that actually has to hold is "clearing rows and counting the drop travel together". The site-count assertion is what keeps it honest: a NEW clear site fails loudly rather than going silently unexamined, which is how this kind of instrument usually rots. Its own mutant (the 401 branch stops counting) dies. P2 — a TRUNCATED cold page is not an answer to "does this exact title exist"; the row may be on a page nobody fetched. `SearchResponse` carries `total`, so `coldAnswered` now requires a complete page. Same defect as trusting the local ranker's window, arriving from the server side — the third variant of one mistake, which is why the rule is now stated once and asked everywhere: offer only where something authoritative has answered. P2 (query change mid-create) was raised for the second time, having been declined in round 3 with reasons that lived only in a commit message — which a reviewer reading the diff never sees. The reasoning is now a comment beside the fences: the three that exist each stand for an act meaning "not this one" (escaping out, choosing another row, landing on a different item or workspace); typing is mid-thought, the user did ask for the item being created, and cancelling would orphan that row with the field still empty. A decision worth keeping is worth putting where the next reader is looking. Matrix: 29 mutants, all killed; baseline and restore both 93/93. |
||
|
xarmian
|
7b32e57cc9 |
fix(web): a dropped workspace needs an identity signal, not an epoch (TASK-2877)
Codex review round 6, and it corrects the reasoning round 5 shipped.
Round 5 fenced the create on `scopeEpochFor(ws) === epoch` and recorded
the residual as needing a coincidence — a purge plus resyncs landing back
on the captured number. That was wrong, and wrong in the direction that
matters: `reset()` deletes the state and the replacement starts at
`scopeEpoch` 0, which is ALSO the value whenever no projection resync has
ever run. That is the ordinary case, so the equality check passed
trivially across exactly the event it was added to catch. A residual I
called exotic was the default path.
The fix is the signal the store did not expose: `resetGenerationFor(ws)`,
a monotonic per-workspace count of drops, deliberately kept OUTSIDE the
`workspaces` map because `reset()` deletes that entry. Both existing
counters — `scopeEpoch` and the internal `generation` — live on the state
object and restart with its replacement; they are safe only because their
readers hold a REFERENCE to the object, which a caller outside the module
cannot. It is bumped even when the reset found no state to drop, so a
purge racing a first bootstrap does not read as no purge.
`createRelationTarget` now asks two questions rather than one:
* `indexStillOurs()` — is this the index the request was authorized
against? It gates the UPSERT, which was previously unconditional on
the argument that a real row belongs in the index. That argument does
not survive a purge: a brand-new id was never in `upsert`'s fenced
set (nothing to fence — the row did not exist when the purge ran), so
the write lands and is persisted to IDB, resurrecting a row into a
workspace the user may have just lost access to. This is the gap
BUG-2098's own comment describes.
* `stillWaiting()` — is the user still waiting on THIS create? It gates
the link and the toast, and it is now ONE predicate rather than two
hand-copied condition lists. The failure path had drifted from the
success path by exactly the reset half (round 6 P2); sharing the
predicate is what stops that recurring.
Matrix: 27 mutants, all killed; baseline and restore both 91/91. New
store surface carries its own suite, including a CONTROL asserting that
`scopeEpochFor` genuinely cannot answer this question — if that ever stops
holding, the cheaper round-5 fence was sufficient after all and this
accessor should go.
|
||
|
xarmian
|
f7bb735771 |
fix(web): state the cold rule positively; catch the epoch reset (TASK-2877)
Codex review round 5, two P1s, both about `localIndex.reset()` — the sign-out / 403-purge / deleted-workspace path. THE FLAG WAS THE WRONG WAY ROUND. `coldFailed` asked "did the last search fail", and that was false in three states that are not answers at all: before the first request, after a failure, and after a reset drops every row while the query sits in the box. Each one read as "fine" and put a create row on screen backed by nothing. Inverted to `coldAnswered` — set in exactly one place, by the event that earns it, and cleared wherever the answer stops describing what is in the box. A flag that must be cleared everywhere is one that will be missed somewhere; this is the same defect arriving twice (round 3 caught the failure case, round 5 the reset case) because the polarity made silence indistinguishable from success. THE EPOCH FENCE HAD TO BE TWO-SIDED. `upsert`'s own guard refuses a captured epoch BELOW the current one, which catches a resync. But `reset()` DELETES the workspace state and the next bootstrap starts a fresh one at `scopeEpoch` 0 — so a captured 7 is not below 0, sails through, and links a row minted under an identity that no longer holds. `createRelationTarget` now requires equality. The residual is in the code comment rather than papered over: a reset plus resyncs landing back on exactly the captured number would compare equal, which an exposed reset generation would catch and this does not. Also dropped the `loading` term from `showCreate`. It and the per-query `coldAnswered` reset were a redundant PAIR — each survived removal while the other stood, which is one guard and one line that looks like a guard, not defence in depth (this repo has a note about exactly that shape). `coldAnswered` is the one kept: it states the rule (something authoritative has answered FOR THIS QUERY) where `loading` is a UI state that correlates with it. Matrix: 24 mutants, all killed; baseline and restore both 85/85. Killing the per-query reset needed `aria-expanded`, not the row's absence — with `loading` still gating the MARKUP, `.picker-create` is missing either way and asserting on it measures the branch instead of the rule. Third time this suite has been fooled by that same separation. Re-verified end to end in a real browser on this exact build: create row offered for a non-matching query and keyboard-reachable; Enter created COLO-6 "Chartreuse" in COLORS (colors 2 -> 3, cars unchanged) with `status: approved` — the schema's declared default, which the "+ New" `options[0]` heuristic would have gotten wrong; the car's field holds that id; a second pass at the same text offers the existing row and no create; Escape leaves the value untouched; no bare UUID anywhere on the page. |
||
|
xarmian
|
6a335dd120 |
fix(web): absence is only evidence from a settled index; fence the error toast (TASK-2877)
Codex review round 4. Two taken, one declined. P1 — `bootstrapState === 'ready'` was the wrong authority for the create row. It coexists with `pendingResync`: `localIndex` hydrates from the IDB cache and serves those rows while delta-sync catches up, so during that window an item that EXISTS can be missing from the snapshot. The create row is derived from ABSENCE, and a cache snapshot cannot support that inference — presence still can, since the row was real when it was cached. `indexCanProveAbsence()` is asked ONLY by `showCreate`; search and listing keep using `isWarm`, because showing cached rows during a resync is right and it is only the "therefore no such item exists" step the cache cannot bear. The window is seconds and a duplicate outlives it. That leaves one rule across the whole unit, applied in four places now: offer only where something authoritative has answered. Cold is authorized by `/search` (the server answered); a settled index is authorized by the in-RAM collection; a resyncing index and a failed search authorize nothing. P2 — the failure path was unfenced while the success path was not, so a create the user escaped out of, or one belonging to a workspace they have since left, still threw its error over whatever they were looking at. Same three conditions, same reasoning: the difference between reporting and not is whether they are still waiting on it. DECLINED, with reasons, so it is not re-flagged: the "A->B->A gap" in the workspace fence. The classic gap bites when an identifier can be REBOUND to a different object between capture and compare. Here the pair (workspace slug, item slug) is what the fence compares, and the parent subtree is keyed on the item slug, so returning to the same pair returns to the SAME item — applying the create there is correct, not stale. Item refs are sequential and never reused, so the identifier cannot be rebound within a workspace. Matrix: 22 mutants, all killed; baseline and restore both 82/82. Four anchors went stale this round because the fence now appears on two paths and matched twice — the harness refused to score them rather than silently mutating the wrong copy, which is the reason it checks. |
||
|
xarmian
|
761e6e2453 |
fix(web): a failed cold search is not evidence that nothing matched (TASK-2877)
Codex review round 3 P2. `coldSearch`'s catch leaves exactly the state a
successful empty answer leaves — no rows, not loading — and the result
list is right to render both as "No results". The create row is not: an
empty answer is evidence that no such item exists; a failed one is no
evidence at all, and offering to create on no evidence is how a duplicate
gets minted while the index is cold and the network is unhappy. Same rule
the permission gate already follows — no answer must not read as
permission.
A `coldFailed` flag now separates the two, and where it is CLEARED was
settled by the matrix rather than by symmetry. Three reset sites looked
obviously needed and three mutants removing them survived:
* the cold branch of `runQuery` — `loading` is true for that entire
window and already suppresses the row, and both `coldSearch` branches
assign the flag outright when the request settles;
* the empty-query branch — covered twice over, since an empty query
offers no create row at all;
* the workspace-reset effect — same as the first.
All three are gone rather than carrying a comment claiming a protection
they do not provide, which is the disposition this plan's own U3 note
records for an unkillable guard. The ONE reachable reset is the warm
branch: it is the only path that produces a fresh verdict without going
through `coldSearch`, so without it a single network blip suppresses the
affordance for the rest of the session even once the authoritative in-RAM
answer is available. That one has a test, and its mutant dies.
Round 3 also raised a P1 I am NOT taking: typing a new query while a
create is in flight does not cancel it. The three fences that exist —
escape, picking another row, retargeting — each stand for an act that
means "not this one". Typing is not such an act; it is mid-thought, and
the user did explicitly ask for the item that is being created. Treating
it as a cancel would leave the created row orphaned and the field unset,
which is a worse outcome than a field that ends up holding exactly what
was asked for. Told to Codex in the next round rather than left to be
re-flagged.
Matrix: 20 mutants, all killed; baseline and restore both 80/80.
|
||
|
xarmian
|
5dffd734c1 |
fix(web): fence the create against cancel and against a workspace switch (TASK-2877)
Codex review round 2, two P1s, both confirmed at the lines they name. CANCEL. `oncancel` only closed the picker, so backing out did not supersede an in-flight create — the pending promise then resolved and selected an item the user had just declined. Backing out is as explicit a choice as picking a different row, and now bumps the same counter. WORKSPACE SWITCH. `ItemDetail` keys its fields subtree on `itemSlug` ALONE, so switching workspaces to an item carrying the SAME ref — and every workspace has a TASK-5 — reuses this component rather than remounting it, and `destroyed` never fires. The completion then wrote an item ID from the previous workspace into the new workspace's item. `createRelationTarget` already captured `ws` and `collSlug` before the request; it now compares them to the live props before applying, which is the DR-6b shape `ChildItems.submitCreate` uses for the same reason. The `localIndex.upsert` still runs ahead of all three fences and still uses the CAPTURED workspace: the item genuinely exists in the workspace it was created in, and the fences are about where the VALUE is written, not about hiding a real row. Matrix now 17 mutants, all killed; baseline and restore both 78/78. The two added here — cancel not bumping the counter, and the ws/collection comparison removed — are what stand in for having seen these two tests red before the fix, since pin and fix landed in one edit. |
||
|
xarmian
|
83e4abc966 |
fix(web): fence the in-flight create; ask the index, not the ranking (TASK-2877)
Codex review round 1, three findings, all confirmed by reading the code
they name rather than taken on the report.
P1 — the create completion had no fence, and there are two ways past it.
`ItemDetail` wraps its fields section in `{#key itemSlug}`, so an item
switch DESTROYS this component; the promise survives, and `onchange` calls
into the persistent parent, whose `updateField` builds its PATCH against
whatever item is current at CALL time. A create started on car A therefore
wrote its colour onto car B. Separately the picker stays open across the
round trip, so the user can settle on another row (or clear the field)
before it lands — and last-write-wins is the wrong rule there, because the
later write is an explicit choice and the earlier one is a promise they
have moved past. A `destroyed` flag and a supersede counter, checked
together, close both. The `localIndex.upsert` deliberately runs BEFORE the
fences: the row exists on the server whatever happened locally, and
withholding it would leave a picker offering to create it a second time.
P2 — `targetCollection` read the global collection list with no freshness
gate, so during a workspace switch a slug match against the PREVIOUS
workspace's rows yielded a foreign collection ID, and `canEditCollection`
answered about that. Same gate `knownCollectionSlugs` already had, which
this derivation was missing.
P2 — the exact-title suppression was asking the RANKING. `warmSearch`
requests `limit + excluded.size` hits, so an exact row the ranker placed
outside that window is simply absent from `rawResults` and the picker
offers a duplicate. The question has an authoritative answer in
`localIndex`, already in RAM, so the warm path now scans the collection
directly. The `rawResults` check stays and is NOT redundant: while the
index is cold there is nothing to scan, and the server's rows are the only
evidence the row exists — pinned by its own leg, which is what killed the
mutant that removed it.
Mutation matrix now 15 mutants, all killed; baseline and restore both
76/76. Two rounds of it earned their keep beyond the fixes: M3 SURVIVED
once the index scan landed, and the mutant was faithful — the suite had no
cold-path exact-match leg, so the surviving mutant found a real hole in my
tests rather than a redundant line in the code.
|
||
|
xarmian
|
e331342450 |
feat(web): relation fields create their target inline, permission-gated (TASK-2877)
PLAN-2857 U8, caller half. `FieldEditor` hands the picker an `oncreate`
only when the viewer may create in the field's DECLARED TARGET, so it
decides both of the unit's gates by deciding whether to pass one.
The gate is `canEditCollection` on the target collection — the same
predicate behind the collection page's "+ New" — asked about where the
item would LAND, not about where the user is standing. It needs the
collection's ID, which only the loaded collection list carries; a target
the list does not know yields no create row, because "no answer" must not
read as "allowed".
NO FIELD VALUES ARE SENT, and that is a decision with a receipt. The
server fills every missing key that declares a `Default` and stores the
defaulted map (`items.ValidateFields`, then "Marshal validated/defaulted
fields back" in `createItemChecked`), so the schema's own answer is
already the right one. The collection page's "+ New" guesses
`status.options[0]` instead; driven live against a Colors collection whose
status options are [draft, approved] with `default: approved`, the created
row came back `{"status":"approved"}` — the declared default, which that
heuristic would have gotten wrong. The cost is that a target carrying a
REQUIRED field with no default refuses the create; that surfaces as a
toast naming the field, which is the honest outcome for a row this picker
cannot fill in.
The new item is upserted into `localIndex` under the epoch captured BEFORE
the request (BUG-2098 — a projection resync landing mid-flight means the
response was authorized under a scope that no longer applies). That upsert
is what makes the picker's exact-title suppression true on the very next
keystroke; without it the same text offers to create a second item.
Mutation matrix, all killed: creating in a collection other than the
declared target, the permission gate removed, the upsert removed, and the
epoch read after the request rather than before.
|
||
|
xarmian
|
322b461606 |
feat(web): the scoped picker offers an inline create row (TASK-2877)
PLAN-2857 U8, picker half. When a scoped picker's query matches nothing — or nothing EXACTLY — it offers a trailing "Create "<query>" in <collection>" row, keyboard-reachable like any other row. The affordance is opt-in at the call site: it appears only when the host passes `oncreate`, which is how both of U8's scope rules are expressed without this component knowing either. "Relation fields only" is the Relationships tab passing nothing; the permission gate is the caller's, because "may this user create in the target collection" is the collection-level `canEditCollection` cascade that lives in the workspace store. Result rows and the create row become ONE `options` list, in render and keyboard order, so arrowing onto the create row needs no special case and cannot fall out of step with what is on screen. `activeId` already addressed rows by identity; the create row takes a NUL-prefixed sentinel id in the same namespace, which no UUID can collide with. Two suppressions carry weight and both are pinned: * EXACT-TITLE. Tested against `rawResults` — the source's answer before exclusion and the row bound — because an exact match pushed past `limit` or excluded by the caller would otherwise read as "no such item" and offer to mint a duplicate of a row that exists. This IS the no-duplicate half of the unit's proving test: there is no create-time uniqueness check anywhere, because the second pass at the same text never reaches a create. * LOADING. Mid-flight, "nothing matched" is not yet known. The one assertion that can fail here is `aria-expanded`, not the row's absence: the markup renders the loading branch INSTEAD of the listbox, so a build that offered the row mid-flight would still show no `.picker-create` and merely leak a combobox announcing itself expanded over no listbox. That is trap #1 from this plan's false-green note, met in my own diff. Re-entrant creates are dropped while one is in flight, so two Enters inside a single round trip cannot mint two items — a duplicate the exact-title check cannot catch, since no row exists yet to match. Mutation matrix, all killed: exact-title suppression removed (3 tests), re-entrancy guard removed, `loading` term removed, `collection` term removed, Enter dispatching over `results` (the pre-U8 line), create row prepended rather than trailing. |
||
|
xarmian
|
6f7c09a44a |
fix(web): judge a relation's collection only when the list and index agree (TASK-2868)
Codex round 2, P1, real — and it is the retag window my round-1 fix left open. `retagCollection` moves the indexed ROWS onto the new slug immediately; `collectionStore.loadCollections(ws)` is fired next to it with `void` — not awaited, and its rejection swallowed. So between those two there is a state where the collection list still holds the OLD slug (making the declared target read as 'live') while the row already carries the NEW one. Judging the mismatch there reported the value as "Unresolved reference" — and because that refetch is unawaited and its failure unobserved, the state is PERMANENT when it fails, not a paint-frame flicker. The fix reframes what the mismatch is evidence OF. A collection mismatch means the value is wrong only when the collection list and the item index agree about the world — that is, when the current list knows BOTH the declared target and the row's own collection. Two ways they disagree, and neither is the value's fault: the target was renamed away (round 1), or the rename reached the index before the list (this round). Requiring both slugs to be known collapses both to "don't judge", while a genuine cross-collection value — target `colors`, row `tasks`, both live — still resolves to null. That is also why this is not fixed by invalidating freshness: `collectionsAreFreshFor` answers "loaded for this workspace", not "current", and teaching it about pending/failed refreshes is a store-wide change to serve one consumer. The agreement test needs nothing new. New control leg alongside it, because two "don't judge" guards in a row are one edit away from never judging: both slugs live, mismatch, still rejected. Mutation matrix 17 of 17 killed (N17 new — judge as soon as the target reads live, i.e. round 1's shape; N11/N14/N15 anchors refreshed). Gates: `npm run check` 1093 files 0 errors, 6 pre-existing warnings; full web suite 123 files / 2063 tests green. Context 55.8% at this boundary (`session-shape`, which lives at /home/dave/claude/bin and is not on PATH — my earlier "not measured" reports read `command -v` failing as the tool not existing). |
||
|
xarmian
|
09f80b3d4f |
fix(web): a renamed target collection must not read as lost data (TASK-2868)
Codex round 1 on this unit, P1, correct — and it is a defect the PREVIOUS
commit introduced.
`models.FieldDef.Collection` holds the target's SLUG (the schema editor binds
`<option value={c.slug}>`), and `store.UpdateCollection` re-slugifies on rename
without migrating the relation definitions that point at it — the string
"relation" does not appear in that file at all. Meanwhile `localIndex.applyRetag`
correctly moves the indexed ROWS onto the new slug. So after a rename the field
and the rows disagree, and the collection check added last commit reported every
stored value as "Unresolved reference": a schema problem presenting to the user
as lost data, on data that is completely fine.
Now three-valued. The collection check applies only while the declared target
still names a LIVE collection; a stale target falls back to id-only resolution
so the chip keeps rendering, and the field goes read-only because a picker aimed
at a renamed collection would list nothing (`getByCollection` and
`localSearch` both filter on that slug). `'unknown'` — the collection list not
yet loaded — is deliberately NOT read as stale: that is absence of evidence, and
treating it as stale would flash every relation field into read-only on first
paint.
Filed **BUG-2873** for the root cause, with both candidate fixes (migrate
dependent schemas on rename, or store the collection ID) and the argument that
the second is what PLAN-2857's own "store the ID, titles change" reasoning
implies for the collection pointer too.
**Two of the three new tests were wrong first, in ways that let a mutant live.**
- The stale-target test set up the STORE's collection list but left the row's
`collection_slug` matching `field.collection` — so field and row agreed, and
the mutant making the check unconditional passed. A rename retags the rows;
modelling only half of it reconstructs a scenario that cannot fail.
- The unknown-vs-stale test asserted the chip renders. A stale target also
renders the chip, so it could not tell the two apart. It asserts EDITABILITY
now, which is the only thing that actually differs.
Mutation matrix 16 of 16 killed, including the four new ones (N14 stale target
invalidates values, N15 unknown reads as stale, N16 stale target stays editable,
plus N11 refreshed for the new shape).
Gates: `npm run check` 1093 files 0 errors, 6 pre-existing warnings; full web
suite 123 files / 2061 tests green; `go build ./...` ok, gofmt clean.
|
||
|
xarmian
|
5079a532c9 |
fix(web): resolve a relation by id in its own collection; collapse the picker (TASK-2868)
Three defects, all found by driving the Cars/Colors example from IDEA-2856 in a real browser against a locally built binary. None of them showed up in the twelve component tests, and two are mine. **1. A legacy free-text value rendered as a working reference.** `localIndex.findByIdOrSlug` resolves by id OR SLUG, so the string `"red"` — exactly what the old text fallback has been writing into these fields — resolved to the item slugged `red` and rendered as a live chip. The field's contract is that it stores an item ID; a slug match makes the chip lie about what is stored, and slugs are mutable, so the same value could point elsewhere tomorrow. Now resolves by id only, and the browser leg that was meant to prove "a legacy value reads as unresolved" is the one that caught it. **2. It could resolve into the WRONG COLLECTION.** That helper is workspace-wide, so a relation declared against `colors` would render an item from `tasks` sharing the identifier. This is the same defect PLAN-2857's recon recorded against the server's `ResolveItem` — I wrote that finding down in the design doc and then reproduced it in my own client code a few hours later. **3. The field showed a permanently-open search box.** The first browser pass rendered the chip, the picker input still holding the query, and the result list still listing the row just chosen — the same item three times, under every relation field on the page. A field shows its VALUE; the picker is for changing it. Now: chip + Change / Clear, picker on demand, closing when a choice is made. Also **filed BUG-2872** rather than absorbing it: the activity timeline on the same page renders a relation change as `color: → <uuid>`. The panel now honours IDEA-2856's "never a bare UUID"; that surface does not. The e2e invariant is scoped to the field row on purpose, so the gap is recorded rather than hidden by loosening the assertion to the page body. Mutation matrix 13 of 13 killed. N13 (emit the value but leave the picker open) survived the first pass — the test asserted the emit and not the CLOSE, which is the half the browser pass had rejected. Two mutants that had to die separately do: N1 (drop the deleted branch) kills leg (b), N2 (unresolved reads as deleted) kills leg (c), so the two states are genuinely distinguished rather than sharing a branch. Gates: `npm run check` 1093 files 0 errors, 6 pre-existing warnings; full web suite 123 files / 2058 tests green. |
||
|
xarmian
|
a04233aaa9 |
feat(web): relation fields render a linked chip and edit through the picker (TASK-2868)
PLAN-2857 U2. Absorbs the relation half of TASK-2216.
Before this, `relation` fell through `fields/FieldEditor.svelte`'s `{:else}`
text fallback: an editable free-text input in edit mode, and `{value ?? '—'}`
in display mode. Since `internal/items/validate.go:275` accepts ANY string for
a relation, that combination did not merely fail to edit — it SAVED. Typing
"red" into a relation field stored the literal string and showed no error, and
the display arm rendered a raw UUID when the value happened to be one. So U2 is
closing a silent corruption hole, not adding an editor to a read-only field.
**Three render states, not two.** A value that resolves to nothing and a value
whose target was deleted are different facts about the item, and the third is
the COMMON case on existing data — arbitrary strings are what the old fallback
has been writing. All three resolve locally: `localIndex` holds soft-deleted
rows alongside live ones (`getByCollection` filters them out rather than
dropping them), so a dangling target is a row carrying `deleted_at`. No fetch,
no loading state. The invariant across every state, asserted in every leg: a
raw item ID never reaches the user.
**The branch is gated on `wsSlug` AND `field.collection`, and the second call
site sits on the far side of that gate deliberately.** `CopyItemDialog` builds
its `FieldDef` from a preflight row whose shape carries no `collection`
(`ItemCopyPreflightNeedsValue`), and it copies ACROSS workspaces — so an
unscoped picker there would offer SOURCE-workspace items as the value for a
DESTINATION-workspace field, and look authoritative doing it. A free-text box
at least looks like something the user owns. Read-only is the honest state
until TASK-2869 (U2b) extends the preflight contract; U1 makes the garbage
write a 400 in the meantime.
That gate is a fact about the CALLERS, invisible from the component's own
render tests, so both sides are asserted at the call sites
(`fieldEditorRelationCallers.test.ts`) — including that `toFieldDef` still
builds from a shape with no `collection`, which fails loudly when U2b lands
rather than letting the gate drift.
Pin first, per team CONVE-29: the test file was written and run BEFORE the
branch existed — 4 failed / 2 passed, the two passers being the gate legs,
which pass vacuously while no picker exists anywhere. That is why they ship
with a control leg that mounts one.
One pin leg was STRENGTHENED rather than relaxed when it failed against the new
code: leg (a) asserted "some anchor exists" and failed because the test passed
no `username`, which is what builds the href. The fix was to give it one and
assert the exact href, plus a new leg (a2) for the resolved-but-no-route case —
where the chip degrades to a non-link and must still name the item rather than
degrade to the raw value, which is precisely what the old arm did.
Gates: `npm run check` 1092 files 0 errors, 6 pre-existing warnings; full web
suite 122 files / 2048 tests green.
|
||
|
xarmian
|
cd5c5702d4 |
feat(web): give ItemPicker a source model; keep the Relationships tab on server FTS (TASK-2862)
Lead ruling on PR #1241, and the right call. The extraction had silently moved the add-relationship search onto the warm local path, and `localIndex` strips `content` by design — so a user who links an item by a phrase they remember from its BODY lost that, with no signal anything had changed. Consistency with the other pickers does not buy back a capability under CONVE-139. `source` is now an explicit MODEL choice, not a performance one: 'index' (default) — `localSearch` over title / ref / tags / parent / field values, no network call, server only as a cold fallback. Right for a RELATION field, where you are choosing a row from a known collection and know what it is called. U2 onward take this. 'server' — always `/search`, whose FTS also indexes body content. Right for the Relationships tab, where you are finding an item you remember rather than one you can name, and what it did before this component existed. ItemDetail passes it. An empty-query LISTING stays on the index for both: it is not a search, `/search` cannot answer one (it requires a `q`), and the rows are local either way. Only QUERIES follow `source`. Two supporting changes fall out of it rather than being bolted on: **`rawResults` + a derived `results`.** The exclusion filter is now part of the derivation, so a late `excludeIds` — `ItemDetail` loads `itemLinks` asynchronously — re-filters on its own. Without that, honouring a late exclusion on the server-backed caller would have meant re-issuing the request, which is the rate-limiter pressure the debounce exists to avoid. The refresh effect no longer needs `excludeIds` as a dependency at all, and server-sourced QUERIES are explicitly not re-run on an index delta. **The highlight is an ID, not an index.** `activeId` is state; `activeIndex` derives from it. Identity survives the list changing underneath — a delta, a late exclusion — where an index silently moves the highlight onto whatever slid into that position. This deletes the hand-rolled preserve/restore that lived in the effect, so no future site that changes the list has to remember to do it. Pins, per the ruling: the server caller queries `/search` with a hydrated index and never touches `localSearch`; the control leg asserts the default source on the same warm index never reaches the network; and a source-level test asserts ItemDetail's call site still carries `source="server"` — a regression invisible from the component's own tests, which is why it is asserted at the call site. Verified in a real browser against a locally built binary with a marker string present ONLY in an item's body and never in its title, so the local index cannot answer it: the Relationships picker finds it, arrows to it, and creates the link. Mutation matrix 20 of 20 killed, including the three new ones — ignore `source` (3 failed), re-query on a delta (1 failed), drop `source="server"` at the call site (1 failed). Gates: `npm run check` 1092 files 0 errors, 6 pre-existing warnings; full web suite 121 files / 2041 tests green. |
||
|
xarmian
|
4b0a818178 |
fix(web): drop stale rows on an index reset; re-filter on a late exclusion set (TASK-2862)
Codex round 4, both findings real. **P1 — an index reset left rows on screen.** `localIndex.reset()` (sign-out, a 403 membership purge, a deleted workspace) drops the workspace state and resets the search index, which bumps the epoch — so the refresh effect DID run, saw a non-`ready` state, and returned. Rows the viewer may no longer be allowed to see stayed listed and selectable, and a cold response already in flight could still add more. The effect now tears down on that path: invalidate `seq`, cancel the debounce, clear the results and the highlight. I wrote that teardown behind a "only if we are showing or awaiting something" guard first. The mutant removing the guard could not be killed, and working out why showed the guard is dead — the one non-ready run that reaches it with nothing to clear is the cold mount, where it changes nothing either way. So the guard went, rather than acquiring a comment claiming it protects something. The control leg stays: an ordinary cold mount must still complete its own search. **P2 — `excludeIds` arriving late did not re-filter.** `ItemDetail` loads `itemLinks` asynchronously, so a picker opened before that resolves was offering items already linked to the source; clicking one is a duplicate-link write the user did not know they were making. The effect now tracks the exclusion set too. **The first test for it was a false green, and that is the more useful half.** Driving the prop change through testing-library's `rerender` REPLACES the whole props object, which re-runs the refresh effect whether or not it tracks `excludeIds` — so M20 (delete that dependency) survived while the production path, where a real parent changes one prop, was broken. Adding "the typed query survived, so this was not a remount" did not help: the instrument was wrong in a different way than suspected. It now runs through `ItemPickerProbe.svelte`, a test-only host in the shape of `FreezeProbe` / `GuardProbe`, which changes exactly one prop. M20 dies against it. Mutation matrix rebuilt and re-run, 18 of 18 killed. The harness itself was rewritten this round after a regex edit corrupted its escapes and reported seventeen false ANCHOR MISSes — anchors are now generated with explicit tabs into a JSON file rather than hand-escaped in source. Verified again in a real browser against a locally built binary, including the round-4 P2 path end to end: link a target, re-open the picker, and confirm the now-linked item is no longer offered. Gates: `npm run check` 1092 files 0 errors, 6 pre-existing warnings; full web suite 121 files / 2035 tests green. |
||
|
xarmian
|
03ec276cac |
fix(web): refresh on the search epoch, not the workspace cursor (TASK-2862)
Codex round 3, one P2, correct — and it catches that round 2's fix tracked the wrong signal. `localIndex.upsert()` and `remove()` — optimistic creates and edits, the 403 purge — mutate `state.items` and mirror to `localSearch` WITHOUT advancing `state.cursor`. So a picker wired to the cursor stayed stale after exactly the mutations a user is most likely to cause while it is open. `localSearch.epoch(ws)` is the right dependency and strictly dominates the cursor: it is bumped by every write to the search index, and every `localIndex` path that touches `state.items` mirrors there — verified one by one this round: `applyDelta` (three sites), `upsert`, `remove`, `removeByCollection`, `applyRetag` (behind `retagCollection`), `reset`. It also exists for precisely this consumer shape; its own doc comment describes an `$effect` re-deriving search results, added for the identical staleness bug in TASK-1364 round 3. I should have found that before reaching for the cursor. The distinction is pinned by a test that bumps the epoch and asserts the cursor never moved, and by mutant M16 (track the cursor instead — 3 failed). The double deliberately KEEPS `cursorFor` rather than dropping it, so a build wired to the cursor still runs and simply fails to refresh: a mutant that dies of a TypeError would prove nothing about which signal is correct. Mutation matrix, 15 of 15 killed (M13 rewritten, M16 new). Gates: `npm run check` 1091 files 0 errors, 6 pre-existing warnings; full web suite 121 files / 2031 tests green. |
||
|
xarmian
|
3de173a2d4 |
fix(web): keep an open picker current with the local index (TASK-2862)
Codex round 2. One finding accepted and fixed, one declined with the
measurement — see below.
**Accepted: an open picker held a stale COPY of the rows.** `results` is a
snapshot, so an SSE delta landing while the picker was on screen left it
listing what the workspace used to contain until the query changed or it
remounted. This is the same missing dependency as round 1's cold-mount finding,
so the two collapse into one effect: it now tracks the bootstrap state AND the
workspace cursor (`$state` on `WorkspaceState`, bumped by every applied delta
batch), and re-lists on either.
Re-listing on every delta made the round-1 empty-query guard the wrong shape —
it would have frozen a typed query's results for as long as the user kept
typing nothing. Replaced with something better: the re-list now PRESERVES the
highlighted row **by id**, recomputing its index against the new results. A
blind re-list takes back a row the user arrowed down to; keeping the old INDEX
is worse still, silently moving the highlight onto whatever slid into that
position. If the row is gone, the highlight clears. Both wrong variants are
pinned as mutants (M14, M15).
**Declined: "empty-picker Escape does not reach pane handling."** Re-raised
from round 1 as a behaviour rather than a prose issue. Measured before
declining:
- Pre-change, the add-relationship box was a bare `<input type="text">` with
no keydown handler at all, so Escape there did nothing. Same today when a
caller omits `oncancel`. Not a regression.
- Both pane hosts bail on text-entry targets BEFORE running the escape stack
(`[collection]/+page.svelte:2321`, `[collection]/[slug]/+page.svelte:465`),
with the rule stated in their own comments: "Text-editing targets own ESC
locally". Making the picker hand Escape upward would contradict a
route-level policy, which is not this unit's to change.
- The one caller today, ItemDetail, DOES pass `oncancel`, so in the shipped
configuration Escape clears the query and then closes the form — strictly
more than it did before.
The prop doc says plainly what omitting `oncancel` costs, so U2 cannot acquire
the gap by accident.
Mutation matrix re-run on the changed tree, 13 of 13 killed (M11 rewritten for
the new effect; M13/M14/M15 new — drop the cursor dependency, re-list blind,
preserve the index instead of the identity).
Gates: `npm run check` 1091 files 0 errors, 6 pre-existing warnings; full web
suite 121 files / 2030 tests green.
|
||
|
xarmian
|
3c63bfbd9d |
fix(web): re-list a scoped picker after hydration; correct the Escape prose (TASK-2862)
Codex round 1, both findings P2, both real. The second one is the more useful of the two because what it caught was a claim I made without reading the code it was about. **A scoped picker mounted before the local index hydrated stayed empty.** `recent()` ran on mount and on input only, and returns [] while the index is cold — so a relation field opened during hydration (U2's case) showed nothing until the user typed, then silently started working. A `$effect` now re-lists when the bootstrap state reaches `ready`. It tracks that read ONLY; `query` is read inside `untrack`, because reading it reactively would re-run the effect on every keystroke and race `oninput` — the same reason `onMount` owns the first run. The refresh is guarded on an empty query, and that guard is now pinned by its own test: without it, hydration landing mid-session resets `activeIndex` and takes back a row the user had already arrowed down to — a keystroke they never made, at a moment they cannot predict. **The Escape comment was wrong.** It said that declining to stopPropagation "correctly lets Escape reach the pane". It does not: both pane hosts' keydown handlers call `isTextEntryTarget(target)` and RETURN before they run the escape stack (`[collection]/+page.svelte:2321`, `[collection]/[slug]/+page.svelte:465`), on the deliberate rule that a text field owns Escape locally. So a picker with no `oncancel` leaves Escape with no owner at all — which is also what the inline search this replaces did, so it is not a regression, but it is not what I wrote either. I had the escapeStack file open and read the STACK rather than its DRIVER. Fixed in three places: the `oncancel` prop doc now says plainly what omitting it does, the handler comment states the real mechanism and marks the stopPropagation as belt-and-braces rather than load-bearing, and the control test is renamed off the false claim — it asserts what it actually establishes (the picker does not consume a key it has nothing to do with, including `defaultPrevented === false`). Test-harness note: `bootstrapStateFor` is now backed by a `SvelteMap` in the suite instead of a `vi.fn` return value. A bare mock return is not reactive, so a test driving it that way cannot tell a working refresh from a missing one — the first version of the hydration test passed against both. `SvelteMap` is a runtime class, so a plain `.ts` file gets the tracked get / triggering set that the real store gets from `$state`. Mutation matrix re-run on the changed tree, 12 of 12 killed — the two new ones being M11 (drop the hydration re-list, 1 failed) and M12 (drop its empty-query guard, 1 failed; survived until the selection test above was added). Gates: `npm run check` 1091 files 0 errors, 6 pre-existing warnings; full web suite 121 files / 2028 tests green. |
||
|
xarmian
|
17ec291908 |
fix(web): align the type-select to the picker input, not to the whole picker (TASK-2862)
Caught in a real browser, not by a test. `.add-link-controls` is a flex row and the picker is a COLUMN (input, then its result list), so the row's default `align-items: stretch` made the "Related / Blocks / …" select as tall as the input PLUS the open result list. Before the extraction the results were a sibling of this row, so the question never came up. `align-items: flex-start` restores the pre-extraction look. Verified against a locally built binary on both Playwright projects — desktop-chromium and mobile-chromium (Pixel 7) — driving the real flow: the form opens focused, the input carries `role=combobox`, typing filters, ArrowDown sets `aria-activedescendant` to the first option's id, Enter creates the link (read back from `/links`) and closes the form. At the mobile width the row keeps its shape and long titles ellipsis rather than overflow. |
||
|
xarmian
|
43c33d1aeb |
test(web): make the picker's fence and consumption guards actually discriminate (TASK-2862)
Two tests from the previous commit passed against builds with the thing they
name deleted. Both are fixed here, and the mutants that exposed them now die.
**The staleness fence.** The first version released a superseded response while
the picker was still `loading`, and asserted the stale row was not in the DOM.
It passed with `if (mySeq !== seq) return;` removed — the loading branch renders
instead of the result list, so the row was in `results` and merely off screen.
The scenario now lets the SECOND request land first, so the picker is settled
and the fence is the only thing between the stale row and the DOM.
**The unmount leg.** "Unmount, resolve, assert the row is absent" cannot fail:
an unmounted component renders nothing whatever the teardown does. Replaced
with the half that IS observable — closing the picker inside the debounce
window means the request is never sent. `onDestroy`'s `seq` bump is kept and
now says in a comment that no test can kill its removal, rather than being
defended by one that would pass either way.
**The consumption guard.** `/<ItemPicker\b/` matches inside an HTML comment, so
commenting the mount out left all five source assertions green — the guard
passed against the exact regression it exists to catch. It now runs against a
comment-stripped copy, which also removes a false `{#key itemSlug}` match from
the explanatory comment above the real directive.
Mutation matrix, all ten killed (each applied to the shipped tree, suite run,
tree restored from a COPY — never `git checkout`):
M1 drop the row cap ...................... 2 failed
M2 always take the server path ........... 1 failed
M3 never take the server path ............ 3 failed
M4 drop the staleness check .............. 1 failed (survived before this commit)
M5 stop consuming Escape ................. 1 failed
M6 drop aria-activedescendant ............ 1 failed
M7 ignore the exclusion set .............. 1 failed
M8 drop the collection scope on /search .. 1 failed
M10 drop onDestroy's clearTimeout ......... 1 failed
M9 comment out ItemDetail's mount ........ 3 failed (survived before this commit)
|
||
|
xarmian
|
a85016972a |
feat(web): extract the add-relationship search into a shared ItemPicker (TASK-2862)
PLAN-2857 U3. The relation-field editor (U2) and the Relationships tab need
the same "find an item and choose it" control; today it exists once, inline in
ItemDetail, closing over that component's `item` and `itemLinks`. This lifts it
out so U2 mounts it rather than copying it.
WHERE THE CANDIDATES COME FROM. The design pass ruled OUT the dropdown-vs-
search threshold PLAN-2857 asked for. `localIndex` is a workspace-wide in-RAM
read model of every item — `/workspaces/{ws}/items-index` takes no limit
parameter — and `localSearch` is a MiniSearch index built over it, so a target
collection's rows are already in memory, already ranked, and cost no network
call. The threshold was pricing a round-trip that does not happen. One control,
always filter-shaped, correct at three items and at three thousand; the tests
assert the rendered row count is a function of `limit`, not of collection size.
The server `/search` endpoint stays as the COLD path — used while the local
index has not hydrated, which is also the behaviour ItemDetail had before this
change. It is not a mode the user can select. Only that path is debounced: the
debounce exists to keep per-keystroke requests off the rate limiter, and the
warm path issues no requests to limit.
FENCES. The per-query `seq` moves into the picker and additionally fires on
unmount. The item-switch fence is NOT duplicated: ItemDetail already mounts
this region inside `{#key itemSlug}` (PLAN-2105 / TASK-2112), so a switch
destroys the picker and its continuation with it — a source-level test pins
that the picker stays inside an OPEN key block, since "a key exists somewhere
above" would pass with the picker outside all of them.
New behaviour beyond the extraction, both required by the unit:
- keyboard navigation (arrows / Enter / Escape) with aria-activedescendant,
which the inline search never had;
- a scoped picker opens with the target collection listed most-recently-
updated first, so it is useful before the user types.
Escape consumes only what it actually closes and stops propagation only then —
the page's Escape driver is a bubble-phase window listener feeding
`runTopEscape`, so an empty picker with no `oncancel` correctly lets the key
reach the pane. There is a control leg for exactly that.
`formatItemRef` is widened from `Item` to `Pick<Item, 'item_number' |
'collection_prefix'>`: the picker deals in `ItemIndexRow` (`Omit<Item,
'content' | 'moved_to'>`), which carries both fields but is not an `Item`.
Strictly wider, so every existing caller still type-checks.
The result-list CSS moves into the picker rather than being shared by class
name — Svelte scopes styles per component, and the note already in ItemDetail
records what reusing a class name across that boundary costs. The picker sizes
everything in `em` so a host sets the scale once on its own wrapper.
Verification: `npm run check` 0 errors (the 6 warnings are pre-existing and in
other files); 15 new component tests + 5 source-level consumption tests green.
|
||
|
xarmian
|
af24997c72 |
fix(web): re-resolve the follow target by id at fire time (BUG-2848)
Codex round 1, P2, and a real latent bug in the previous commit. Capturing the item OBJECT and reusing it 140ms later keeps a stale snapshot: a rename during the debounce changes the slug, `openItemPane` builds the URL from that slug, and the id-only existence check passes happily on the way to a dead URL. What is captured is now the IDENTITY — `targetId` — and the callback re-resolves the current row from `filteredItems` before opening it. That still follows a row that MOVED, which is the whole point of the fix, and still skips one that was DELETED, while picking up any change to the row itself. Also answering the round's second P2 in the spec rather than in code: the race is probabilistic and cannot be made deterministic without a seam in the page. The asymmetry is what makes that acceptable, and it is now written down — a round that misses the 140ms window still PASSES on a correct build, because the cursor moves, the pane follows and the intended row is where it should be. So missing costs power, not correctness; the failure mode is a false green, never a false red. Three rounds put a false green around 1 in 1700 against a build that loses the keypress 11 times in 12. pane-follow-live-list + pane-controller: 44/44 across both projects. |
||
|
xarmian
|
1a76531eb3 |
fix(web): capture the pane-follow target at keypress, not when the timer fires (BUG-2848)
The list is SSE-live and the pane-follow is debounced 140ms. The callback
re-read `filteredItems[focusedIndex]` when the timer fired, which made a
keystroke depend on the list holding still for those 140ms. It does not.
The failure was silent, and that is what made it hard to see. `j` advanced
`focusedIndex`; an item arriving during the debounce shifted every index below
it, sliding the PANED item down onto that very index; the callback read it
back, found "the focused row is already the paned item", and returned through
its own guard. No cursor move, no re-target, no error — a discarded keystroke.
The target is now captured BY IDENTITY at keypress time. The callback still
re-checks pane state and that the row still exists — identity, not position, so
a row that MOVED is followed correctly and only a row that was DELETED is
skipped.
MEASURED, because the first two explanations were both wrong.
The trail's diagnosis was that an insert leaves `focusedIndex` behind so `j`
lands on the already-open row. A snap-back $effect re-syncs the cursor to the
open item on every `filteredItems` change and prevents exactly that; a pin that
waited for the row to settle passed every candidate assertion.
So the second hypothesis was that the snap-back undoes the cursor move during
the debounce, and the fix was to suppress it while a follow is in flight.
Measured: 12 of 12 failures, WORSE than the 11 of 12 baseline. The stale index
lands on the paned item by itself; the snap-back was never the culprit.
Capture-at-keypress, same harness, same sweep size:
baseline (unfixed) 11/12 lost the keypress
suppress snap-back 12/12 lost the keypress
capture target at keypress 0/12
across all three measured properties — the pane re-targeted, the cursor moved,
and the pane landed on the row that was actually below the cursor.
The new spec CAUSES the race rather than waiting for it: it seeds a row above
the cursor, then lands a second insert across the keypress inside the debounce.
Three rounds per run, because one round caught the unfixed build 11 times in 12
and three make a false green not worth reasoning about. Counterfactual against
the unfixed controller: 3 of 4 desktop runs fail with the bug's signature —
`Expected: "DOC-16"` (the intended row) versus `Received: "DOC-15"` (the row
that was already open).
It asserts three things and none is redundant: `retargeted` alone passes if the
pane wanders anywhere; `cursorMoved` alone passes if the cursor moves and the
pane ignores it; `intended` is what pins the actual contract. A fourth that
suggests itself — "the pane agrees with the focused row" — passes VACUOUSLY on
the bug, since cursor and pane are then both stuck on the opened row. It was
measured doing that and is deliberately absent.
pane-controller.spec.ts is unchanged and still green (21/21). Its intermittent
failure was this defect, not the shared-workspace pollution it was filed as —
it just could not cause the race, so it only caught it when a sibling test's
seed happened to land in the window.
|
||
|
xarmian
|
58f50909af |
Merge pull request #1234 from PerpetualSoftware/feat/idea-2843-composer-quote-handle
feat(web): comment on a selection, with comments back under the item content (IDEA-2843) |
||
|
xarmian
|
e92f6f235d |
fix(web): the sidebar footer's Settings label wrapped once the GitHub link joined the row (BUG-2844)
MEASURED, not eyeballed. The desktop sidebar is 260px wide, less 24px of .sidebar-inner padding, so the footer row has 235px. Four 32px controls and four 8px gaps are fixed cost; .settings-btn is the only flex:1 item and got what was left — a 75px box, 51px of content after its 12px padding. The label "⚙ Settings" needs 56.3px. Five pixels short, so the gear and the word landed on separate lines and the row grew from 33.9px to 51.7px. The five pixels arrived with the GitHub link (IDEA-2711, PR #1229): a 32px control plus a fifth gap took 40px out of a box that had about 22px of slack. space-2 -> space-1 on the row returns 16px and the Settings padding another 8px, all of which lands in the one shrinkable item: a 75px content box against 56.3px of text, a 33% margin rather than the 5% either change alone would have left. Measured after: one line box, row height back to 33.86px. ONLY THE DESKTOP ARM WAS BROKEN, which the dispatch's "every width the layout supports" is what surfaced. Below 768px the sidebar is 280px AND the collapse button is gone, so the row carries four controls in 255px and the label had 135px to itself. It measured one line box before this change and still does — and both mobile legs of the new spec PASS against the unfixed CSS, which is what makes the desktop failures mean something. .github-btn also gains `flex-shrink: 0`, which every other control in the row already had. Harmless today — its automatic minimum size equals its 32px content box — but it made the row's one shrinkable item ambiguous, and .settings-btn is meant to be that item. THE TEST IS AN E2E SPEC BECAUSE NOTHING ELSE CAN HOLD IT. jsdom performs no layout, so a vitest render of Sidebar.svelte reports identical geometry with and without the bug. It asserts LINE BOXES rather than row height: height grows for other reasons and could stay put through a wrap, while Range.getClientRects() returns one rect per line, so the count is the question itself. Counterfactual run against the reverted declarations: desktop fails "Expected: 1, Received: 2", the shrinkable-control leg fails on the extra item, mobile stays green. Gates: web unit tests 115 files / 1978 tests green; svelte-check 0 errors (the 6 warnings are pre-existing, in files this does not touch); go build and go vet clean. The full Go suite was NOT re-run locally — no Go file changed — and CI covers it; naming the narrowing rather than reporting a leg I did not run. |
||
|
xarmian
|
7a7e9d669b |
fix(web): the selection toolbar is a row again (IDEA-2843)
Codex round 7. `.bubble-menu` had no layout of its own. Its buttons are themselves `display: flex`, so they are block-level and STACK — invisible while the menu held one action, wrong the moment Comment joined Extract. It also falsified the dimensions `positionMenu` clamps against, so the menu drifted over the text it points at. A row layout on the container; the expanded state opts out, since the extract form lays itself out. Layout is not observable in jsdom, so the assertion lives in the e2e: the two buttons share a row (y within 4px) and Extract sits to the right of Comment. Removing the row layout fails it in a real browser — verified. Declined, with the reason recorded in the code: "Comments 1+" can appear when the only unfetched entries are activity or versions. `+` reads as a LOWER BOUND, and a lower bound of 1 over exactly one comment is true. Knowing whether more comments exist means fetching the rest of the feed, so the alternative trades a true imprecise count for a confident wrong one. Gates: 119 files / 2005 unit tests, svelte-check 0 errors, e2e 2/2 on the selection spec against a rebuilt binary. Claude-Session: https://claude.ai/code/session_011Q4b1iHtJtSyMs7BA2ySxo |
||
|
xarmian
|
4ebad409a2 |
fix(web): gate the composer's item identity during A→B navigation (IDEA-2843)
Codex round 6. 1. [P1] During an A→B navigation `item` still holds A while loadData fetches B, so ItemTimeline received A's itemId/collectionId beside B's itemSlug — and an attachment dropped in the composer inside that window is associated with the WRONG item. The wiring is PRE-EXISTING and identical on main. What changed is the exposure: the composer used to sit behind the Activity tab, and tabs reset to Details on an item switch, so reaching it inside the load window took a deliberate tab click. It is now on the tab you land on. Widening a latent hole is the same as opening one, so it is fixed here. Fixed by feeding honest inputs rather than adding a gate: the host passes itemId/collectionId only while `itemMatchesRef`, and ItemTimeline's canEdit already derives false without them, so the composer hides until the identities agree. 2. [P2] A load failure's banner outlived it — `loadMore` set `error` and never cleared it, so a successful retry left the failure sitting beside the entries it claimed had not loaded. Newly visible because the error is mirrored to the tabs now. Test boundary, stated: the new test covers ItemTimeline's half of the gate (no identity ⇒ no composer), verified by a control that flips its default to permissive. The host's half — the `itemMatchesRef ? … : undefined` — is not unit-testable here, since ItemDetail cannot be mounted in jsdom. Gates: 119 files / 2005 unit tests, svelte-check 0 errors. E2E: 37 passed across the five affected specs. An earlier run of that same set had one failure — capstone's "stale back-settle" nav test — which did not reproduce alone or in an identical re-run, and sits outside this diff's surface (history back-settle and drill targeting; nothing here touches either). Recorded rather than dropped. Claude-Session: https://claude.ai/code/session_011Q4b1iHtJtSyMs7BA2ySxo |
||
|
xarmian
|
09a183844d |
fix(web): a submit no longer erases what arrived mid-flight; empty states wait for the last page (IDEA-2843)
Codex round 5. 1. Data loss, in the handle I added. `doSubmit` clears the composer on success, and a quote pushed in through `appendMarkdown` during the round trip was cleared with it — the quote simply vanished. The clear now requires the composer to still hold what was SENT. This also fixes a PRE-EXISTING loss by the same mechanism: text the user typed while a submit was in flight was erased too. It is the same class as the item-identity capture already guarding this path (PLAN-2105 / TASK-2112) — that one asks "is this still the same item", this one asks "is this still the same content". 2. Empty states appeared while more pages remained. A first page carrying only other kinds made a filtered view say "No versions yet." before the pages that would have contradicted it were fetched — a claim about the item made from one page of a feed. Both views now wait for the last page; until then the "Load more" button is what the reader sees. Gates: 119 files / 2004 unit tests, svelte-check 0 errors. The mid-flight fix has a negative control — restoring the unconditional clear fails the new test. Claude-Session: https://claude.ai/code/session_011Q4b1iHtJtSyMs7BA2ySxo |
||
|
xarmian
|
b82c689eda |
fix(web): pagination honours the caller's filter; the host's Load more is styled (IDEA-2843)
Codex round 4.
1. Filtered pagination, fixed properly this time. Round 1 gave the TABS a
host-side retry wrapper and left the same defect in the owner's own
button: the Comments view could also page without showing a new comment.
Fixing the instance and not the class, twice on the same defect.
`loadMore(forKinds?)` now takes the caller's view filter and the hop loop
counts only entries that filter admits, defaulting to the component's own
`visibleKinds`. Whoever pressed the button says what progress means. The
host wrapper is deleted — MAX_EMPTY_HOPS already bounds the walk, so the
six-round loop on top of it was compensation for the missing filter.
Caught while wiring it: the owner's own button was `onclick={loadMore}`,
which passed the MouseEvent as `forKinds`. svelte-check found that; no
test would have.
2. The host's "Load more" was unstyled. Its class name matches
ItemTimeline's, but Svelte scopes styles per component, so the copied
NAME got browser defaults and nothing warned — CSS is the part no test
here asserts. Styles copied over with the reason recorded. Swept the
other class names the split moved across that boundary
(entry-list, compose, timeline-header, entry-count, empty): none is used
in the host, so the population is this one.
Gates: 119 files / 2003 unit tests, svelte-check 0 errors. The pagination
fix has a negative control — restoring the unfiltered break fails the new
test, which needed descending fixture timestamps to avoid passing for the
unrelated "cursor did not move" reason.
Claude-Session: https://claude.ai/code/session_011Q4b1iHtJtSyMs7BA2ySxo
|
||
|
xarmian
|
abe8d93c10 |
fix(web): per-view titles, counts and empty states; a failed quote is not silent (IDEA-2843)
Codex round 3. 1. The split left every view describing the WHOLE feed. The comments section was headed "Timeline" with a count of every entry, so an item with three activity entries and no comments read "Timeline 3" over an empty list — a count of things the reader cannot see. And `showEmpty` was computed over the whole feed, so a view whose own slice was empty rendered nothing at all: no entries, no explanation. Title, count and empty state now describe what the view RENDERS. `title` and `emptyLabel` are props: "Comments" / "No comments yet." on Details, "No changes yet." / "No versions yet." on the tabs. The deliberate choice this reverses is mine — I passed showEmpty over the whole feed on the grounds that a filtered-out tab must not claim the item has no history. Right premise, wrong fix: the answer is to say something true about the slice, not to say nothing. 2. A failed quote was silently discarded. `appendMarkdown` returns false precisely so "did nothing" is distinguishable from "inserted" — and the only caller threw the boolean away and hid the menu, putting the silent no-op back exactly where the handle was built to remove it. A false now keeps the menu and the selection and reports it. Gates: 119 files / 2002 unit tests, svelte-check 0 errors, 14 e2e in desktop-chromium against a binary rebuilt from this tree. The discarded-return fix has a negative control — restoring the bare `onComment(...)` call fails the new test. (A first attempt at that control failed on shell quoting and silently ran against UNMUTATED code; it was re-run properly before this claim.) Claude-Session: https://claude.ai/code/session_011Q4b1iHtJtSyMs7BA2ySxo |
||
|
xarmian
|
8d7fdc22f3 |
fix(web): consult the error state everywhere success was assumed (IDEA-2843)
Codex round 2, and both findings are follow-through misses on my own round-1 fix: I added an `error` to the mirrored feed and then left the code that assumes a load succeeded reading only `entries` and `hasMore`. - A failed load rendered the error AND "No timeline entries yet." One is a statement about the ITEM; a failed load knows nothing about the item, so the pair says something false beside something true. `showEmpty` now consults `error`. - The filtered "load more" wrapper retried a dead server up to six times per click. `loadMore()` catches and resolves, leaving `hasMore` true, so the loop had no reason to stop. It bails on `error` now. CONVE-18 sweep rather than the two named instances. The population is every consumer of the mirrored feed — six read sites in the markup, four in the wrapper. Two were defects (both above). One is a deliberate non-change: the "Load more" button still renders while an error is showing, because that is the retry affordance. The class also reaches the OWNER, where the same empty-beside-error contradiction is PRE-EXISTING on main and is fixed here too, since leaving it would mean the comments view kept the bug the tabs just lost. The regression test asserts the mounted owner's DOM. A first version recomputed `entries.length === 0 && !loading && !error` and asserted that — which passes whatever the component actually renders — so it was replaced. Gates: 119 files / 2001 unit tests, svelte-check 0 errors. Reverting the owner's guard fails the new test. Claude-Session: https://claude.ai/code/session_011Q4b1iHtJtSyMs7BA2ySxo |
||
|
xarmian
|
5c0d8e9d34 |
fix(web): three codex round-1 findings on the timeline split (IDEA-2843)
All three are consequences of the two-view split that the split's own tests did not reach. 1. Multi-paragraph selections lost their paragraph breaks. The bubble menu builds `selectedText` with `textBetween(..., ' ')` because Extract uses it as an item TITLE, where newlines would be wrong — so quoting through it flattened two paragraphs into one run-on line. Worse, it made `toBlockquote`'s blank-line handling unreachable from production: that behaviour had a passing test and no call site that could produce it. The quote now re-extracts with a paragraph separator, and a test asserts the blockquote's blank line end to end. 2. Activity and Versions rendered a FAILED load as "No timeline entries yet." Loading and error were the owner's states and did not cross the mirror, so an unreachable server and an empty timeline looked identical on the tabs that only render entries. `error` joins the mirror; both states render. 3. "Load more" could visibly do nothing on a filtered view. The owner's hop loop stops as soon as a page adds an entry of ANY kind, so a page of pure comments ends it having added nothing to the changes view. Pre-existing on Versions; widened to Activity when comments moved off it. The host now pages until THIS view's list grows, bounded at six rounds — not fixed in the owner, which would have to know what the other view is rendering. Gates: 119 files / 2000 unit tests, svelte-check 0 errors. Fixes 1 and 2 have negative controls: reverting to the space-joined text fails 1, dropping `error` from the mirror fails 1. Claude-Session: https://claude.ai/code/session_011Q4b1iHtJtSyMs7BA2ySxo |
||
|
xarmian
|
346a5c92a9 |
feat(web): Comment action on the selection toolbar, quoting into the composer (IDEA-2843)
GitHub #1228. Selecting a passage in an item's content now offers Comment beside Extract; it quotes the selection as a markdown blockquote into the comment composer under the content, appending after a blank line so an in-progress draft survives. The selection is NOT consumed — unlike Extract, which replaces it with a wiki-link — so a reader can quote the same passage twice or keep reading. - toBlockquote() prefixes EVERY line including blank ones. An unprefixed blank line ends a blockquote in markdown, so quoting two paragraphs without it silently drops the second out of the quote and leaves it looking like the commenter's own words. - The action renders when the host supplies `onComment`. A composer to quote into IS the capability; a flag that is always true beside a callback that is always supplied would be two ways to say one thing. - The button's accessible name is "Comment on selection". The composer's submit button is also named "Comment", and two identically-named buttons with different effects is a real ambiguity for name-based navigation — found by the first end-to-end run failing on a locator, not on behaviour. A NEGATIVE result, measured and kept. The action was briefly gated peek-independently, reasoning that a peeking master keeps a live composer (BUG-2263) but could not act on a selection. That state does not exist: a drag-selection in a peeking master RE-ACTIVATES it (focus-follows-editing, PLAN-2179 DR-2), so a selection and a frozen master never coexist. The gate is back on `mutationsEnabled`, and e2e/selection-comment-peek.spec.ts asserts the re-activation so a future change that makes selections survive the freeze turns red there instead of quietly reopening the question. Gates: 119 files / 1998 unit tests, svelte-check 0 errors, and 37 e2e in desktop-chromium — the 2 new ones plus the 35 in the four specs the comment relocation touched, run against a binary built from this tree. Claude-Session: https://claude.ai/code/session_011Q4b1iHtJtSyMs7BA2ySxo |
||
|
xarmian
|
7aef246dcd |
feat(web): comments move under the item content; Activity keeps changes (IDEA-2843)
GitHub #1228. Reviewing an agent-written doc meant many small comments, and every one cost a trip to the Activity tab and back. TASK-2294's own spec put an activity preview on the Details panel; it never shipped, and the comments being tab-only is the half that was left. Dave ruled the full move. One component cannot render in two DOM locations, so ItemTimeline stays the SINGLE owner — one fetch, one SSE subscription, one composer — mounted under the content on Details rendering comments, and mirrors its feed out through a new bindable `feed` prop. The Activity and Versions panels render that same feed through a second TimelineEntryList. - The mirror publishes the WHOLE feed, not the owner's rendered slice. The owner renders comments only, so publishing `visibleEntries` would leave both tabs permanently empty with nothing to report. Tested, and the one-word mutation fails it. - `loadMore` rides in the mirror: pagination is a property of the ONE feed, and a tab that can show older entries but not ask for them is a dead end. - The kind partition is three shared constants with an exhaustiveness check, not literals at the mount sites. A kind in none of them renders NOWHERE — which is how note/decision shipped invisible the first time (BUG-2301). Adding a kind to TimelineEntry without routing it is now a build error or a failing test rather than a silent hole. - The comments section carries its own {#key itemSlug}: it left the block that used to provide that remount, and dropping the guard would have been invisible. It wraps only the timeline — the collab editor must never be keyed. Five e2e specs asserted comments behind the Activity tab and are updated. attachment-lifecycle's tab round-trip is preserved deliberately: its claim is that the panel is CSS-hidden rather than unmounted, so it now goes out to Activity and BACK rather than asserting against a hidden panel. Gates: 117 files / 1987 tests pass, svelte-check 0 errors. Both new properties have negative controls — publishing the rendered slice fails 1, unrouting a kind fails 2. Claude-Session: https://claude.ai/code/session_011Q4b1iHtJtSyMs7BA2ySxo |
||
|
xarmian
|
f932469380 |
refactor(web): extract TimelineEntryList from ItemTimeline, no behaviour change (IDEA-2843)
Comments move under the item content on Details while Activity keeps changes and Versions keeps versions, so the one feed has to render in two DOM locations. One component instance cannot be in two places, and the constraint on this work is ONE subscription and ONE composer — so the rendered list becomes its own presentational component and ItemTimeline stays the single owner of fetching, SSE, pagination, the attachment probe, the paint fence and every mutation. This commit is the extraction only. Nothing moves location and no behaviour changes; the second mount site is the next commit. - TimelineEntryList.svelte: the entry loop, rail chrome, the five card branches and their CSS, lifted verbatim. `listEl` is bindable because the owner's delegated lightbox listeners and imperative image-a11y pass attach to the container and stay with the owner. `showEmpty` is passed rather than derived from the rendered entries, preserving the owner's condition over the WHOLE feed — a tab that filters everything out must render an empty list, not claim the item has no history. - The comment-card callbacks are optional here with no-op defaults: a list rendering no comments has nothing to hand them, and a card that could call one only renders when the owner supplied the real handler. Evidence, and the reason it counts: the existing suite passes UNCHANGED — 116 files, 1983 tests — and svelte-check reports 0 errors. A refactor whose only claim is "nothing changed" is exactly where an untouched suite is the right instrument, but only if it actually exercises the moved markup. It does: rendering the list over an empty array instead of `entries` fails 64 tests across 7 files. Claude-Session: https://claude.ai/code/session_011Q4b1iHtJtSyMs7BA2ySxo |
||
|
xarmian
|
a2bdd904a5 |
docs,test(web): correct two claims the mutation matrix refuted (IDEA-2843)
Both corrections are to MY OWN rationale, not to behaviour. - The setContent-over-insertContentAt comment read as a defect avoided. Measured: swapping to insertContentAt leaves all five tests green, so both routes preserve the blockquote today. Restated as what it is — a preference for not depending on normalizeInline's leading-<p> rule — and marked explicitly unenforced. - The explicit `empty = editor.isEmpty` was inert: dropping it leaves the suite green, because setContent emits an update by default and onUpdate maintains the flag. Removed. The test asserting submit becomes enabled is the real guard, and it goes red if a tiptap bump flips that default. The test file's claim that its blockquote assertion catches an insertContentAt implementation was false for the same reason; it now states what the assertion does catch (a genuine flatten, verified) and what it does not. |
||
|
xarmian
|
9363dbb749 |
feat(web): imperative appendMarkdown handle on CommentEditor (IDEA-2843)
The selection toolbar's forthcoming Comment action needs to drop a
blockquote of the reader's selection into the ALREADY-MOUNTED composer.
The obvious route is a silent no-op: CommentEditor reads `content` once,
inside `new Editor({...})` in onMount, and has no $effect syncing it, so
writing the prop on a live composer drops the text with no error.
- appendMarkdown(markdown): appends after a blank line when a draft
exists, never replaces; returns false when there was nothing to insert
or no live editor, so a caller can tell 'inserted' from 'did nothing'.
- setContent (block parse) rather than insertContentAt: tiptap-markdown
overrides insertContentAt with { inline: true }, where a blockquote
survives only incidentally.
- A {#key} remount was the ruled-out alternative: it would destroy an
in-progress draft, which is what doSubmit's identity capture
(PLAN-2105 / TASK-2112) exists to protect.
Tests assert the quote TEXT and its blockquote tag, not the composer's
visibility — the broken version opens the composer too.
|
||
|
xarmian
|
100da86188 |
feat(nav): surface the repo link in the sidebar footer
A running instance had no visible connection to the project it is, so people went to a search engine. The reporter called it "purely a QOL addition"; what makes it worth a commit is that the link already EXISTED and was simply unreachable — buried in the user-menu dropdown under Resources. This is a discoverability fix, not a new capability. Icon-only, in `.footer-row` beside the collapse / theme / bell controls, because that row is already where this instance's chrome lives. The mark carries no accessible name of its own, so the anchor has an `aria-label` naming both the destination and the new tab, and the SVG is `aria-hidden` so it is not announced twice. `$lib/brand/links.ts` exists because the URL was already written twice in `UserMenuResources` and this would have been the third copy. A URL duplicated across every surface that shows it is a rename that goes half-applied. The module holds addresses only — list ORDER stays with the surface that renders a list, since the two lists are deliberately different lengths and the ordering contract lives in docs/brand.md. The tests assert the binding, and the last one asserts the property the others cannot: comparing a rendered href to the imported constant passes identically for a component that hardcoded the same string, because both sides end up as the same characters. That is a question about SOURCE, so a narrow guard reads the two consumers and asserts the literal is absent while `GITHUB_REPO_URL` is present — so the absence means "imported" rather than "the link was deleted". Named files rather than a glob, so it cannot fail for an unrelated file nor quietly stop covering these two. Closes #1168 |
||
|
xarmian
|
7f640a9c40 |
feat(attachments): render markdown and plain-text attachments in the viewer
The arm itself, plus the render seam and the browser proof. `renderMarkdownDocument` is a third thin wrapper in FRONT of the shared `marked` pipeline, following `renderMarkedWithAttachments`'s precedent rather than standing up a second renderer. It omits two things deliberately. No wiki-link resolution: an attached `.md` was authored elsewhere, so resolving its `[[brackets]]` against whatever workspace is showing it would silently retarget a foreign document's links at local items — BUG-2830's hazard entered through the front door. No attachment context, and it CLEARS the module-level context for the duration rather than merely leaving it alone: a nested call from a resolver or a `missing` hook that itself renders markdown would otherwise inherit the outer document's workspace and resolver. Save, clear, restore, mirroring the sibling wrapper. Sanitization is inherited, not re-derived, so an attached document is governed by the same allowlist as item content. Plain text does NOT go through the pipeline. A `.txt` renders in a `<pre>` as text, because interpreting a plain-text file's asterisks as formatting would misrepresent its content; Svelte escapes it, so that path emits no HTML at all. THE FALLBACK ARM'S CONDITION CHANGED, and this is the part to check hardest. It read `shownRenderer !== 'raster-image'`, which was correct while the union had one member and would have drawn "No preview available" over every text document the moment it had two. It is now `=== null` — the registry's actual "no renderer claims this" answer — which says what it means and stays correct when 'pdf' lands. INTERACTION, all of it found by review or re-reading rather than by the unit suite: - The wheel handler consumed every wheel before its exclusions, so the card could never scroll. The text exclusion returns BEFORE `preventDefault`, the opposite of every other exclusion there: the others want the wheel swallowed, this one wants it delivered. Scroll chaining to the inert page is stopped by `overscroll-behavior: contain`, so the guarantee the `preventDefault` provided is kept. - The full-bleed layer took `pointer-events: auto`, so a click on the empty area targeted it and backdrop-close was broken for this arm alone. The layer is inert; the CARD is the interactive surface. - `touch-action` INTERSECTS down the ancestor chain, so the card's `pan-y` could never override the stage's `none` and a phone could not scroll at all. The stage gives up its pan claim on this arm only. - The card had no tab stop. The viewer's arrow keys are its own next/previous navigation, so a keyboard-only user could open a document and never reach past its first screen. `tabindex="0"` plus `role="document"` and the filename label; the linter warning is suppressed narrowly with its reason at the site. - The focus-handoff effect tracked `loader.phase` — the IMAGE loader, which this arm disposes, so it never changes there. This is the only arm whose CONTENT is focusable, so a reload unmounted a focused link and stranded focus outside the modal. - `resolvedSize` and `revalidateToken` ride the load key SCOPED to the text arm. The key is shared, so an unconditional append re-ran the effect for the raster arm too and restarted image loads. The token is there because a parent RESTORE drives the image loader through the metadata probe's answer but cannot see a failed text GET's `error` phase — without it, a preview that 404'd while archived stayed permanently errored after the restore that fixed it. E2E, because three of these guarantees are CSS mechanisms and the jsdom suite injects no component styles — `getComputedStyle` there returns the engine default for every element, so two assertions written for them could not fail and were deleted rather than banked as coverage. `web/e2e/attachment-text-preview.spec.ts` covers the render, backdrop close, scrolling with no leak to the surface behind, and selection. Its FIRST RUN is what caught the feature rendering raw source, which the whole green unit suite could not see. CONVE-23 sweep on the prose this falsified: the ADMISSION vs THE ARM block enumerated 'raster-image loads bytes / null is no-bytes' as a two-way split, and the fallback arm described itself as "an entry the viewer cannot draw as an image". Both rewritten, plus a paragraph on why two byte-loading arms leave the no-bytes invariant unchanged in kind. `.pad-e2e-*/` is gitignored: a shared checkout runs concurrent suites, so each seat points `PAD_E2E_DATA_DIR` at its own, and those hold a generated encryption key and a multi-MB WAL. `item-attachment-strip.spec.ts` changes here because it is a CONSEQUENCE of this arm, not a separate concern. Two of its tests uploaded a `text/plain` file and asserted the viewer showed "No preview available" over it — true when written, false by design once text previews. CI caught them; my sweep had not, because I swept the unit tests and stated that boundary nowhere, which reads identically to a complete sweep (CONVE-18's amended half). The fix keeps each test's SUBJECT — both are producer→host wiring tests whose named subject is the fallback arm — and moves the vehicle to PDF, which keeps every property the fixture was chosen for while remaining unclaimed by any renderer. It carries a note saying it will go red again when PLAN-2393 builds the `'pdf'` slot, and that the red is the design: pick the next unclaimed type, never weaken the assertion. Closes #1169 |
||
|
xarmian
|
eca31efa9e |
feat(attachments): add the viewer text loader with two independent size gates
The markdown/plain-text counterpart to `viewerImageLoader`. The image
loader hands a URL to an `<img>` and lets the browser fetch, decode,
cache and cancel; text has no such element, so this module owns all
three of the things that come for free there:
- cancellation: no `src` reassignment drops the previous request, so
every load carries an `AbortController` and repointing aborts it
- staleness: a late `await` can resolve after the user has navigated, so
every completion is checked against the issuing token AND the active
id (the no-`{#key}` switch-safety class)
- the size bound: the browser will stream a 20 MB log renamed `.md` into
memory quite happily
TWO SIZE GATES, neither subsuming the other. The metadata gate refuses
before any request and saves the transfer. The response gate bounds the
bytes actually read, and exists because `LightboxImage.size_bytes` is
`number | null` BY DECLARATION — an emitter knows only what its own
surface gave it — so the metadata gate passes vacuously for any entry
that arrived without a size. Deleting either leaves a real hole, and the
tests name which hole each one covers.
The gates are restated at the request CHOKEPOINT rather than only at the
renderer, following the image loader's stated reason: the renderer
showing nothing is not the same as the loader asking for nothing. Tests
assert 'fetch was never called', not 'nothing was displayed'.
Fetching a type the server will not inline is not the risk; markdown is
served `Content-Disposition: attachment` and `fetch()` is unimpeded by
that. What keeps it safe is that the bytes never become active
same-origin content — PLAN-2393 DR-6 is honoured by the allowlist, which
admits neither HTML nor JavaScript, not by the disposition header.
Response-derived overflow reports NO figure. In that branch the declared
size is absent or demonstrably wrong, and echoing it produces "This file
is 10 B — too large to preview" — a sentence that reads as a viewer bug
rather than a file problem. Only the metadata branch, which has a
trustworthy number, reports one.
The bound is measured in BYTES on both the streaming and the
`response.text()` leg, and the streaming leg decodes with
`{ stream: true }` so a chunk boundary splitting a multi-byte character
cannot corrupt the text. The module is honest about where the fallback
is weaker: it bounds what is RENDERED everywhere, but what is HELD only
on the streaming path, whose fallback audience is jsdom.
20 tests. The stream fixture's `text()` THROWS, so an implementation
that ignored the stream and buffered everything cannot pass the tests
written to forbid that; the fixture reports read count and cancellation
so a test can assert the read STOPPED, since draining-then-measuring
reaches the same `too-large` and only the read count tells them apart.
`retry` is inert outside `error` — for `too-large` the guard is dead
code, but while LOADING it is the only thing preventing two concurrent
requests for one entry, a case a surviving mutant exposed.
Refs IDEA-2712
|
||
|
xarmian
|
cd2bf7f977 |
feat(attachments): claim markdown and plain text for an in-app text renderer
Adds the third preview predicate GitHub #1169 needs, and widens the surface-renderer union onto the 'text' slot PLAN-2393 reserved. `canPreviewAsText` is deliberately NOT an edit to either existing predicate. `canOpenInViewer` (DR-16) asks what the in-app IMAGE viewer may decode; `canBrowserPreview` (DR-5) asks what the BROWSER may be handed in a new tab, where for `text/markdown` the honest answer stays no because the browser downloads it. Widening DR-5 to reach #1169 would route markdown to the Open-in-new-tab action and reproduce the exact download-instead-of-render behaviour the issue reports. The new question — do WE fetch the bytes and render them ourselves — is the only one #1169 asks, and it gets its own predicate. `canBrowserPreview` and its tests are untouched, which is the signal that nothing widened. MIME-exact, never by category: the server's `CategoryText` CONTAINS the force-download bucket (`text/html`, `text/javascript`, `application/javascript` — `internal/attachments/mime.go`), so a category test would admit exactly the types PLAN-2393 DR-6 forbids inlining. An allowlist excludes them by construction. Deliberately outside the server mirror: `inlineSafe` in mime.go declares itself the mirror of `VIEWER_MIMES` + `BROWSER_PREVIEW_MIMES`, the set the server may send `Content-Disposition: inline`. This set must never join it — we never ask the browser to inline these bytes, we `fetch()` them (which a download disposition does not impede) and render sanitized HTML ourselves. That is what lets `text/markdown` preview in-app while still being served as an attachment. `isMarkdownAttachment` consults the FILENAME, and that is the path that actually fires rather than belt-and-braces: an uploaded `.md` is stored as `text/plain`. `ValidateUpload` sniffs the bytes with `http.DetectContentType`, which answers `text/plain` for prose, and returns the SNIFFED entry; the extension is used only to REJECT a mismatch, and `.md` → `text/markdown` shares `CategoryText`, so nothing rejects. Measured: `ValidateUpload([]byte("# Heading\n..."), "preview.md")` returns `mime="text/plain"`. The MIME check stays first (an explicitly-typed row is honoured whatever it is named); the extension fallback is GATED on the MIME already being in the set, so a filename can never widen what previews. Server side filed as BUG-2841. Set is smaller than what we could render: csv/tsv/json/xml/yaml/toml are allowlisted uploads and all left out, because each has an obviously better rendering this unit does not build, and shipping them raw now would make that rendering a regression later. Tests pin the exclusions with that reason. `TEXT_PREVIEW_MAX_BYTES` carries its receipt: measured over 25 repo `*.md` files (p50 3.6 KB, max 82 KB) and 63 workspace doc bodies (p50 5.0 KB, max 23 KB); 1 MiB is ~12x the largest observed, sits under the 25 MiB upload bound so it has a live range, and errs toward rendering. The comment states the limit honestly — the cap always bounds what is RENDERED, but it saves the TRANSFER only when the size is known before the load. One existing assertion changed meaning rather than breaking: the renderer test asserted `text/plain` has no renderer. It now asserts 'text'. That is the contract change, made explicitly and annotated. Two force-download assertions are labelled as regression guards rather than evidence — they pass against origin/main too. Refs IDEA-2712 |
||
|
xarmian
|
b6afbb8fca |
fix(links,web): align the JS wiki-link grammar with Go's . (BUG-2834)
The Go and JS wiki-link patterns were byte-identical source text and did not mean the same thing. Both spelled the escape alternative `\\.`, but Go's RE2 `.` excludes only LF while ECMAScript's also excludes CR, U+2028 and U+2029. A body with a backslash immediately before one of those three was INDEXED by the server and NOT RENDERED by the client: the backlink panel claimed a link the document refused to draw. CRLF line endings make the CR case the plausible one. Measured on both sides before deciding anything, over nine code points. Exactly three diverge; VT, FF and U+0085 agree, which bounds the divergence at precisely ECMAScript's LineTerminator set minus LF rather than leaving it open at "some whitespace controls". Aligns JS UP to Go (`\\[^\n]`) rather than narrowing Go, for three independent reasons: the grammar's other alternative already admits RAW CR/LS/PS in both languages, so narrowing Go would make `[[A<CR>B]]` legal and `[[A\<CR>B]]` illegal; narrowing Go would stop ExtractWikiLinks returning rows it currently returns, and the next reconcile would DELETE them, which is BUG-2805's damage shape; and markdown.ts's own splitWikiBody already treats `\<CR>` as an escape pair, so only its regex disagreed. LF stays excluded on both sides — scanBracketBody depends on that and is untouched. Also collapses the two duplicate regex literals in markdown.ts into one exported WIKI_LINK_PATTERN_SOURCE. Exported as source text, not a RegExp object, because a `/g` regex carries lastIndex and sharing one between a replace() and a test's exec() would couple them through it. The harness is the part meant to outlive the fix. testdata/wiki_grammar_corpus.json is read by BOTH languages, and carries expectations derived from the grammar spec rather than from either implementation — comparing the two implementations to each other would have reproduced the exact blind spot that hid this, since looking identical is what they already did. Pure ASCII with every control character as a \uXXXX escape: an early probe typed U+2028/U+2029 into a shell heredoc, silently lost them, and would have "confirmed" the bug on two cases that were actually spaces. JS assertions are split across the node and jsdom projects because renderMarkdown finishes through DOMPurify and returns '' without a DOM, where it would fail for a reason unrelated to the grammar. Same split, same reason, as the existing markdown.shareAttachments pair; the jsdom file carries a leg proving the DOM path is live, since '' satisfies every not.toContain assertion. Negative-controlled both ways: reverting the JS pattern fails exactly 9 assertions (3 corpus + 3 per call site) and nothing else; narrowing the GO pattern to JS semantics fails the same 3 cases from the other side, so both halves are live instruments rather than tests that cannot fail. |
||
|
xarmian
|
ba1255881d |
fix(server): refuse a decoded NUL in a JSON request body (BUG-2803) (#1220)
* fix(server): refuse a decoded NUL in a JSON request body (BUG-2803)
The body half of BUG-2782 (path) and BUG-2784 (query). A caller-supplied
string reached a Postgres text parameter, Postgres refused it, and the
handler answered 500 — the honest answer is 400.
WHY THE TRANSPORT RULE CANNOT BE EXTENDED, which is the whole reason this
is a different fix rather than a wider middleware. ValidateQuery works
because a decoded query value is a substring of the raw query with ASCII
substitutions: the bad byte in the raw text IS the bad byte in the value.
That property fails for a JSON body — the reachable NUL arrives as the
six-character escape, all ordinary ASCII — so no request middleware can
find it without decoding the body, which is the handler's job.
MECHANISM, each premise measured against encoding/json rather than
reasoned about:
raw NUL inside a string -> decode ERR (invalid character in string literal)
raw NUL after the value -> decode ERR
the escape in a value -> decodes to a string CONTAINING a NUL
the escape in a KEY -> same
a DOUBLED backslash -> decodes to literal text, NO NUL
the uppercase spelling -> not a JSON escape at all
So the escape is the only vector and its substring is a sound FAST PATH
(absent -> no NUL possible), but not a sufficient test: a doubled
backslash carries the same six characters and decodes to text. In this
product that is not hypothetical — items and documents store markdown,
and a document about JSON escapes is an ordinary thing to write. The
exact step is json.Decoder.Token(), which returns DECODED strings, covers
object keys and arbitrary nesting (an item's fields blob), and needs no
knowledge of the destination type.
NOT REFLECTION over the decoded value, the other obvious design: it sees
[]byte fields AFTER base64 decoding, so a body carrying legitimate binary
({"b":"AQAC"} -> bytes 01 00 02) would be refused for a NUL that is not
text. A token walk sees the base64 characters. No request struct has such
a field today (searched: []byte with a json tag in internal/server and
internal/models, non-test — only models.YjsUpdate.UpdateData, which no
handler decodes from a body); the token walk is chosen so adding one
later cannot silently start rejecting valid requests.
BUFFERING IS NOT A COST. json.Decoder.Decode already holds the whole
top-level value in memory — refill accumulates into dec.buf and grows it
by doubling (encoding/json/stream.go) — so streaming never avoided the
copy. Measured on the 64 MiB workspace-import shape, total allocation:
stream+Decode 354.7 MiB, ReadAll+Unmarshal 256.5 MiB, ReadAll+Decode
512.5 MiB. Peak heap is order-dependent and does not discriminate; the
first run of that measurement showed a 0.77x peak win that vanished when
the legs were swapped, so only the allocation figure is claimed.
POPULATION, measured on Postgres 17 through the real router with a
control leg on every endpoint (92 mutating routes enumerated via
chi.Walk; 13 probed):
before: 12 of 13 DOOR (control 201 / NUL 500, SQLSTATE 22021)
after: 0 of 13 — every NUL leg 400, every control leg unchanged
Confirmed doors: workspace name, collection name, item title, item
content, item fields value, item title via PATCH, comment body, agent
role name, view name, document title, webhook secret, workspace import.
workspace-token name is UNMEASURED, not clean — its control leg 500s on
an unrelated FK in this fixture. The other 79 routes are unprobed, not
claimed clean; the completeness argument is structural instead, and
enforced by a test rather than asserted.
SECOND DEFECT, named rather than slipped in: the six handlers that
decoded straight off r.Body had no http.MaxBytesReader either — the cap
decodeJSON has always applied — so each was an unbounded body read.
Routing them through decodeJSON closes that too.
COMPATIBILITY: json.Unmarshal refuses trailing non-whitespace after the
JSON value where Decode ignored it. Deliberate, same direction as this
fix, and the only behaviour change beyond the refusal. Trailing
whitespace still passes. An EMPTY body still returns a wrapped io.EOF,
because handlers_playbooks.go reads errors.Is(err, io.EOF) as "no
arguments supplied" — caught by TestPlaybookRunAcceptsEmptyBody, which is
exactly the wiring a helper-level change is blind to.
No call site changed for the refusal itself: all 65 decodeJSON callers
already turn a decode error into a 400 carrying err.Error().
Release note: a NUL character in a JSON request body now returns 400
instead of 500 on Postgres deployments.
Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN
* fix(server): follow the NUL refusal into JSON-encoded string fields (BUG-2803)
Codex round 1 on #1220: the check scanned ONE JSON layer, and several
fields cross the wire as JSON-ENCODED STRINGS rather than nested objects
— an item's fields, a collection's schema, a workspace's settings. The
OUTER decode of {"fields":"{...}"} yields the inner document as literal
text, in which the escape is still six ordinary characters and no NUL
exists, so the single-layer token walk passed it.
MEASURED on Postgres 17 with a control leg on each, after the
single-layer check was already in place:
item.fields as a JSON-encoded string 500 control 201
collection.schema as a string 500 control 201
workspace.settings as a string 500 control 201
The error is DIFFERENT from the rest of this family, which is why it is
worth reading rather than assuming:
insert collection: ERROR: unsupported Unicode escape sequence (SQLSTATE 22P05)
22P05, not the 22021 the path and query halves produce. The outer string
is pure ASCII so it never trips the text-encoding check; this is
Postgres's own JSON parser refusing the escape inside a document bound
for jsonb, which cannot represent a NUL. After this change all three
answer 400 with their control legs unchanged.
THE FIX: when a decoded string is itself a complete JSON object or array
— the class this API re-parses downstream — walk it too, to a depth
bound of 8. Recursion terminates on its own (each level is a strict
substring of the one above); the bound keeps a hostile body from buying
many full re-parses, and AT the bound the body is refused rather than
passed uninspected, since the escape is known to be present and the walk
has stopped looking.
WHAT THIS OVER-REFUSES, by design and pinned by a test: the rule is
structural, not destination-typed, so a plain TEXT field whose ENTIRE
value is a valid JSON document carrying the escape is refused too, even
though its column would have stored it. Prose ABOUT a JSON escape does
not parse as a bare document, so the case is narrow, and a value of that
shape breaks any consumer that parses it. The destination-typed
alternative — an allow-list of the fields that arrive JSON-encoded — is
exactly correct and goes stale in silence, which is the failure mode
ValidateQuery's comment rejects when it explains why per-site query
validators could not be written.
Tests: nested documents (fields/schema/settings/array/twice-encoded),
with controls for ordinary content, a doubled backslash INSIDE the
nested document, a string that starts like JSON but does not parse, and
prose that merely mentions the escape; the over-refusal pinned as a
decision rather than left as an accident; the depth bound; and a wiring
leg through the real router on SQLite, where the write would otherwise
SUCCEED so a green cannot be the database doing the work. Fixtures build
their JSON-encoded strings with encoding/json rather than hand-written
backslashes, since the escaping rules are the subject under test.
All four new tests fail with the recursion removed.
Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN
* test(server): build the NUL-bearing timeline fixture through the store (BUG-2803)
TestTimeline_NeverEmitsACursorItWouldRefuse built its fixture through the
API on a premise its own comment stated: "a structured id comes from the
item's fields blob, which nothing validates on write". BUG-2803 made that
false — decodeJSON now refuses a body whose strings decode to a NUL,
including one nested inside a JSON-encoded `fields` string — so the API
can no longer produce the row and the test 400'd on its fixture.
Repaired rather than deleted, because the DEFENCE it covers is still
live: rows in this shape can predate the rule, and the store has no such
check of its own, so a migration, an import or any future non-HTTP writer
can still produce one. The timeline must keep refusing to hand out a
cursor it would then reject.
The fixture now writes the blob directly, injecting the six-character
JSON escape rather than a raw NUL — the blob is JSON text and both
backends reject a raw NUL in it; the NUL comes into existence when Go
DECODES the blob, which is exactly how the timeline ends up with one
inside an entry id. The test is not vacuous under the change: it asserts
the NUL-bearing id took the positional fallback, so an injection that
failed to produce a NUL fails the test rather than passing quietly.
This is the CONVE-23 case — a change that falsifies existing prose owes a
sweep for that prose. The stale sentence was found by the test failing,
not by the sweep, which is the weaker of the two ways to find it.
Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN
* docs(server): correct the timeline comment BUG-2803 falsified (CONVE-23)
The entryID fallback's comment said note and decision ids "come from the
item's fields blob and nothing validates them on write". BUG-2803 made
that half false: the HTTP API now refuses a request body whose strings
decode to a NUL, including one nested inside a JSON-encoded `fields`
string. The sentence was true when written and nothing in this branch's
diff pointed at it.
The fallback still has to exist, and the corrected comment says why:
the STORE has no such check, so rows predating the rule — and anything
writing a blob by another path, a migration, an import, a future
non-HTTP writer — can still carry one.
SWEPT AND DELIBERATELY LEFT: two nearby comments
(handlers_timeline_id_collision_test.go, handlers_timeline_structured_test.go)
also say "nothing validates them on write". Both are about id FORMAT and
DUPLICATION — an imported artifact carrying a UUID-shaped id, a
hand-written blob repeating one — and this change validates neither. In
context those sentences remain true, so they are left alone rather than
edited into noise.
Sweep command: grep -rniE "nothing validates|not validated on write|no
validation on write|unvalidated" --include=*.go internal/ cmd/ — six
further hits, all about other subjects (github_pr raw writes, terminal
schema keys, push payload format, decodeJSON's size bound).
Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN
* fix(server): scope the nested-NUL walk to JSON-encoded fields (BUG-2803)
Codex round 2 on #1220. The nesting check from the previous commit
recursed into ANY string that parsed as a JSON document, on the argument
that a structural test beats a destination-typed one. That argument was
wrong in a way I had written down as an accepted trade and should have
weighed as a defect: a plain-text `content` value holding a JSON snippet
that merely MENTIONS the escape was accepted before this branch, is
stored in a text column that has no problem with it, and was newly
refused — including on RE-IMPORT of an export carrying it.
Refusing input the server itself produced is a worse failure than the
door the unscoped recursion was closing. Measured before the fix: a
workspace whose item content held such a snippet exported 200 and
re-imported 400.
The walk now descends only under keys whose STRING value is a JSON
document something downstream re-parses: config, events, fields,
metadata, phase_data, plan_overrides, schema, settings, tags, traits.
WHY A LIST IS SAFE HERE, when ValidateQuery's comment rejects exactly
this shape for query parameters: there the set of names is unbounded by
design (parseItemListParams turns any unrecognised parameter into a field
filter), so no list could be complete. Here the set is a closed property
of the wire model — a field is JSON-encoded because a Go struct declares
it as a string holding JSON — and
TestJSONEncodedFieldKeysCoversTheModels derives it from internal/models
and fails when a new one appears. The list cannot go stale in silence.
Over-inclusion is the safe direction and the list takes it: a listed key
that is not really JSON-encoded costs one parse attempt and can only
refuse a complete JSON document carrying the escape, while a missing key
reopens a door. `traits` is listed for that reason — it carries JSON but
its declaration has no comment saying so, which is exactly how the
derivation test would have missed it, so the test asserts coverage in one
direction only and the list is allowed to be a superset.
The walk also changed shape: decoding into `any` and walking the value,
rather than a token stream, because key context is needed to know which
subtree is JSON-encoded. The []byte reasoning is unchanged and still
holds — decoding into `any` never produces a []byte, so a base64 field is
seen as its ASCII text rather than as decoded bytes that might contain a
legitimate 0x00.
Tests: text fields carrying a JSON document are ACCEPTED (five keys),
with a leg proving the same document under a JSON-encoded key is still
refused, so the pair differs only in the key; the derivation test; and
the depth-bound fixture now nests under a JSON-encoded key at every
level, since nesting under an ordinary key would never start the
recursion and would have passed for the wrong reason.
STILL OPEN, and the lead holds it: a LEGACY row whose stored fields blob
already carries the escape still exports 200 and re-imports 400. That is
data this fix cannot make importable without weakening the write-side
refusal, and the disposition (repair sweep, flagged import, or documented
acceptance) is a product ruling. Recorded on the item.
Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN
* fix(server): close the three body doors codex round 3 found (BUG-2803)
All three verified before fixing, none taken on the reviewer's word.
1. BUNDLE IMPORT BYPASSED THE REFUSAL (P1). handlers_import_bundle.go
parses pad-export.json itself rather than through decodeJSON, so the
SAME workspace import — reached with Content-Type application/gzip
instead of application/json — walked straight past the NUL check into
Postgres. The bundle's export blob is now checked with bodyDecodesNUL
before ImportWorkspace, answering the same 400. Test drives a real
tar.gz through the router with a clean-bundle control leg, because this
path answers 400 for a dozen unrelated reasons (bad gzip, out-of-order
tar, duplicate entries) and a bare 400 would prove nothing.
2. ONE CALLER SWALLOWED THE NEW ERROR (P2). handlers_admin.go's
test-email endpoint read `if err := decodeJSON(...); err != nil ||
input.To == ""` and fell back to the admin's own address, so a body
carrying a NUL answered 200. An ABSENT body legitimately means "send it
to me"; a body that is present and REFUSED is a different thing, and
collapsing the two turns a validation error into a success. The two
cases are now separated on errors.Is(err, io.EOF).
3. THE COMPLETENESS TEST COULD NOT SEE PAST TWO CALL SHAPES (P2). It
scanned for json.NewDecoder(r.Body) and io.ReadAll(r.Body), so it was
blind to io.ReadAll(io.LimitReader(r.Body, n)) — a shape ALREADY in the
package — and to any alias or helper. A completeness test that misses a
live example is worse than none, because it reads as coverage. It now
scans for the thing that cannot be spelled around, a reference to the
request body at all, and requires every FILE touching one to be
accounted for with a written reason. Both directions are asserted: an
unaccounted file fails because a door may have opened, and an accounted
file that no longer touches a body ALSO fails, so the list cannot rot
into stale excuses that quietly cover a future reader. Verified with a
positive control (an added body reference in an unlisted file fails) and
a negative one (a stale entry fails).
FOUND BY THAT WIDENED SWEEP, and fixed here rather than filed: the raw
artifact import (POST /workspaces/{ws}/import-artifact) takes TEXT, not
JSON, so it never went through decodeJSON and inherited neither the NUL
refusal nor the path/query rule — a body is neither. A raw NUL or
invalid UTF-8 reached the store and Postgres answered 22021, which the
handler turned into a 500 for what is a client error. It now applies
bindableText, the same predicate ValidatePath and ValidateQuery use, and
answers 400 invalid_body. Note the shape difference from the JSON half:
there the ESCAPE is the vector because a decoder rejects a raw NUL;
here the RAW BYTE is, because nothing is in the way.
Each fix has a mutation run against it: disabling the bundle guard fails
the bundle test, disabling the artifact guard fails the artifact test,
and both controls still pass.
Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN
* fix(server): the escape gate was unsound, and YAML has its own (BUG-2803)
Codex round 4, two P1s, both reproduced before fixing.
1. THE FAST PATH LET A REAL NUL THROUGH. bodyDecodesNUL gated on "does
the raw body contain the six-character escape". That is unsound: the
BACKSLASH itself can be written as an escape, so a body carrying
\u0000 contains no literal six-character sequence anywhere in its
raw bytes, while the OUTER decode manufactures one inside the string —
and if that string is re-parsed as a JSON document (jsonEncodedFieldKeys)
the second parse turns it into a real NUL.
Measured through the real router before the fix: the oblique spelling
answered 201 where the direct one answered 400.
The mistake was applying a fact about how a NUL is spelled INSIDE a
decoded string to the RAW BYTES, where the backslash can itself be an
escape. That is the same layer-confusion this whole bug is made of, for
the third round running.
The gate is now a BACKSLASH. Every JSON escape mechanism requires one, so
a body with no backslash has decoded strings byte-identical to its raw
bytes, and a raw NUL cannot survive the decoder — no backslash therefore
means no NUL, at any depth, however spelled. Bodies WITH one pay for an
exact answer, a larger set than before (any nested JSON carries a
backslash-quote), which is the cost of being correct. The same
correction applies to the per-string pre-filter one level down.
2. YAML HAS ITS OWN ESCAPE VOCABULARY. The raw bindableText check added
last commit passes a double-quoted scalar `title: "a\0b"` — no NUL in
the request bytes — and the YAML decode manufactures one. Measured
before the fix: that artifact imported 201 with a NUL in the item title.
The decoded artifact is now checked too: title, body, and every
frontmatter field value, walked because a playbook's `arguments` is a
nested structure rather than a scalar. Keys are checked as well as
values, on the same precautionary grounds ValidateQuery states for
query parameter names.
Same shape as the JSON half in both cases: a value that is harmless
until a SECOND parse, checked at the layer that can see it.
Tests: the oblique spelling joins the nested-document table, and the
YAML escape joins the artifact table. Each is mutation-verified —
reverting the gate to the substring fails the oblique case only, and
disabling the post-decode artifact check fails the YAML case only, with
the raw-byte cases still killed by the raw check. That per-leg
discrimination is the point: it shows each check earns its own keep
rather than being covered by its neighbour.
Prose corrected where this falsified it: jsonNULEscape's "it is the ONLY
spelling" is true of the escape and was being used to justify a filter on
the raw bytes, which is a different claim. Both now say so explicitly.
Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN
* fix(server): multipart text fields and the bundle manifest (BUG-2803)
Codex round 4's two P2s. Both are the same shape as the rest: a
caller-supplied string reaching a text comparison through a door the
earlier fixes did not cover.
1. MULTIPART TEXT FIELDS. The multipart body is deliberately exempt from
the JSON rule — its payload is binary blob content and must not be
scanned for text validity — but its TEXT fields are a different thing.
`item_id` goes to ResolveItem and into a database comparison exactly as
the query-string channel does, and that channel has been validated at
the transport since BUG-2784; the form channel was not. multipartValues
now drops values that are not bindable text, which makes an unusable
value indistinguishable from an absent one — the disposition
resolveUploadItemID already applies to empty values.
The uploaded FILENAME gets the same predicate, with a fallback to a
generic name rather than a refusal: the bytes are fine, only the label
is unusable.
A NEGATIVE RESULT worth recording, because it changed the test: a RAW
NUL in the multipart header is NOT the vector. Go's multipart reader
refuses it as a malformed MIME header line before any handler sees it
(measured: 400, "malformed MIME header line"). The reachable spelling is
the RFC 5987 encoded form, filename*=UTF-8''sh%00ot.png, which the
header parser accepts and percent-decodes afterwards. The first version
of this test used the raw form and was testing a vector that does not
exist.
2. THE BUNDLE ATTACHMENT MANIFEST. A second JSON document inside the
tar.gz, parsed directly like pad-export.json was, so it needed the same
check. Without it a NUL in a manifest string reached
rehydrateAttachment, whose failure is logged and SKIPPED — so the import
reported success while silently dropping the attachment. The
skip-on-failure behaviour is pre-existing and deliberate (a partial
restore beats none); refusing the bad INPUT is what stops it being
reached this way. Left as it is, and named rather than quietly changed.
A VACUOUS ASSERTION THE MUTATION CAUGHT, recorded because the test would
otherwise have shipped as coverage: the filename leg first asserted
`!strings.ContainsRune(body, 0)` on the RESPONSE, which is JSON — a NUL
in the filename comes back as the six-character escape, not as a 0x00,
so the check passed whether or not the fix was present. It did pass with
the fallback disabled. Now it decodes the response and asserts the
replacement name. The item_id leg had the mirror-image weakness: it
asserted "not a 500", which is the Postgres-only symptom, so on SQLite it
would have passed either way; it now asserts the request behaves exactly
like the no-value control.
Every fix in this commit has a mutation against it, and each kills only
its own leg.
Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN
* refactor(server): drop the now-unused escape constant (BUG-2803)
The gate became a backslash check, which was the last production use of
jsonNULEscape; golangci-lint's unused check failed on the next run. Its
documentation was load-bearing, so the explanation moved into
bodyDecodesNUL's comment rather than being deleted with the variable —
including the distinction that made the old gate wrong (the escape has
one spelling INSIDE a decoded string, which is not a claim about the raw
bytes).
Caught by re-running lint on the tip after the previous commit rather
than trusting the run from the tip before it.
Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN
* fix(server): rune-safe truncation and User-Agent sanitising (BUG-2803)
Codex round 5 was asked for the POPULATION rather than a confirmation —
"enumerate every remaining way a caller-supplied string can reach a
database text or jsonb parameter without passing a validity check" — and
returned three residual classes with their sinks. Two are fixed here;
the third is filed, because measuring it needs a fixture this unit
should not grow.
1. TRUNCATION CAN UNDO THE VALIDATION. Four sites cut a caller string
with a plain byte slice (name[:120], input.Name[:200]). If the boundary
lands inside a multi-byte rune the result ends in a partial sequence and
is no longer valid UTF-8 — so a value that PASSED the body check a few
frames earlier arrives at the store unbindable, and Postgres answers
22021 for a request the server already accepted.
This is the interesting one, because no input-side round could have
found it: the defect is downstream of validation, and it is invisible
with ASCII fixtures, which is what every test in that area used.
truncateBindableText walks back off continuation bytes and drops the
straddling rune. Tested with 2-, 3- and 4-byte runes so an off-by-one
walk-back cannot pass them all, and with a counterfactual leg asserting
the naive slice really does produce unbindable output for the same
input — without it the cases would pass against an implementation that
did nothing.
2. USER-AGENT REACHES TEXT COLUMNS. It lands in activities.user_agent
(three document paths, the connected-apps revoke) and
sessions.user_agent (three login paths), and no rule here sees a header.
The disposition is SANITISE, not refuse, and that is deliberate: a
header is metadata this server chose to record, not something the caller
asked for, so a malformed one must not turn an otherwise fine request
into a 400. The two sites that HASH the header are left alone — sha256
over arbitrary bytes is well defined, and changing what is hashed would
invalidate every stored UAHash.
The filing's own earlier probe had recorded User-Agent as NOT
reproducing on the item-create path. That was true and did not
generalise; these are different sinks.
3. NOT FIXED, FILED: the OAuth form-encoded bodies
(/oauth/token, /oauth/authorize/decide, /oauth/revoke,
/oauth/introspect) parse url-encoded form data outside the shared body
validator, with connection_name reaching oauth_connections.name and
client_id reaching the oauth_clients.id lookup. This was the ORIGINAL
subject of BUG-2803 before the filing was re-scoped, and it was recorded
then as unreachable without a fosite-backed fixture. That is still true,
and round 5's sink list is far more than the filing had. Filed rather
than guessed at.
Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN
* fix(server): narrow the gate, stop refusing natural-shape fields (BUG-2803)
Codex round 6 plus one measurement of my own. Three changes, one of them
a revert of something I got wrong in the previous commit.
1. THE GATE COST TOO MUCH, so it is narrower and still sound. The
previous commit gated the walk on "does the raw body contain a
backslash", which is correct but catches every body carrying nested JSON
(each `\"` is a backslash). Measured on a ~377 KB import-shaped body:
60106 allocs/op with that gate versus 30073 with the walk disabled — the
walk was running on ordinary traffic.
The gate is now the four bytes that begin any \u escape for a character
below U+0100. The argument: to manufacture the six-character NUL escape
inside a decoded string, each of its characters arrives either literally
from the raw bytes — in which case the raw contains the escape, which
begins with that prefix — or from a \u escape of its own, and the three
characters involved (backslash U+005C, 'u' U+0075, '0' U+0030) all sit
below U+0100, so those escapes begin with it too. Back to 30073
allocs/op, identical to the walk-disabled build.
That argument is the same KIND of reasoning that was wrong two rounds
ago, so it does not stand on its own: a differential test runs the gated
function against an UNGATED walk over a corpus built to attack it —
oblique backslash, upper-case hex, an escaped 'u', an escaped '0', a
doubled backslash — and fails on any disagreement. It also asserts the
corpus contains both answers, since agreement over a one-sided corpus
would be vacuous. Reverting the gate to the old substring fails it.
2. THE CHECK REFUSED THE NATURAL SHAPE OF ITS OWN FIELDS. `tags` and
`fields` accept both a JSON-encoded STRING and their natural array/object
form, and the walk propagated "this subtree is JSON-encoded" into
containers — so a free-form tag whose whole value happened to be a JSON
document was refused, though nothing re-parses it. Measured: refused
before, accepted now, while the JSON-encoded spelling of the same field
is still refused. The flag now marks only a direct STRING child of a
listed key.
3. REVERTED: I wired the three LOGIN paths to the User-Agent sanitiser
last commit, before reading store.CreateSession. It HASHES the header
and stores no text — the round-5 enumeration named "sessions.user_agent"
and I took the name for a column. The change would have been actively
harmful: login would store sha256(sanitised) while middleware_auth still
compares sha256(RAW), so every session from a client with a non-UTF-8
User-Agent would fail validation. A sink named in a review is a pointer
to verify, not a finding. The real sink is activities.user_agent, from
three document paths and the connected-apps revoke.
4. And the wiring leg codex asked for, on that real sink: a request
through the router with a malformed header, reading the STORED value out
of the activities row, with a control asserting an ordinary header is
kept VERBATIM. Unwiring the production call site fails it; the helper's
unit test does not notice.
Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN
* fix(server): apply the key rule at every level, not once (BUG-2803)
Codex round 7, both findings, the first confirmed by measurement.
1. THE RECURSION WENT ONE LEVEL TOO DEEP. Once the walk descended into
a JSON-encoded string it treated the WHOLE subtree below as
JSON-encoded, so a value nested two levels down — an ordinary string
inside a `fields` blob that happens to hold JSON text — was refused.
That is a false rejection, and the measurement says so plainly. With the
depth-2 check disabled, on Postgres 17:
depth 1 (the fields blob itself) -> 400 (correct: Postgres parses it)
depth 2 (a string INSIDE the blob) -> 201 (accepted, no error)
control -> 201
The handler parses `fields` ONCE. The inner text is re-escaped when the
blob is written, so what Postgres receives has a doubled backslash and no
escape at all. Only the document Postgres itself parses can carry a fatal
one.
The nested call now passes false rather than true, which makes this a KEY
RULE APPLIED AT EVERY LEVEL rather than a depth limit: a JSON-encoded key
INSIDE a document still recurses (pinned by a test), an ordinary one does
not. Same correction as round 6's natural-shape fix, one level further in
— I fixed the sibling case and left this one, which is CONVE-18's lesson
about my own enumeration being a sample too.
I checked whether anything re-parses a value inside the blob before
loosening this, rather than assuming: `arguments` was the candidate, and
parsePlaybookArguments asserts it is a native ARRAY (raw.([]any)) rather
than a JSON string, so it is covered by the natural-shape rule and needs
no second parse.
2. AN ERROR MESSAGE THAT SENT CLIENTS THE WRONG WAY. The OAuth dynamic
client registration handler prefixed every decode failure with "Request
body must be JSON". A body carrying a NUL is valid JSON, so that message
sends a client hunting a syntax error it does not have. The two failures
are now distinguished.
Round 7 also reports no break in normal CLI, MCP or web-client request
generation — they marshal JSON and encode paths and query parameters —
which is the first thing any round has said about the client surface.
Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN
* fix(server): complete the artifact check, make the walk path-aware (BUG-2803)
Codex round 8. It confirmed round 7's two fixes, then found two real
defects and two inaccurate comments — the comment half being the angle
the round was asked for.
1. THE ARTIFACT CHECK MISSED TWO REACHABLE FIELDS. artifactIsBindableText
walked the decoded artifact by TYPE, so it never covered Provenance —
whose strings are rendered into a Markdown footer appended to the stored
content — and never matched Arguments, declared []map[string]any, a
concrete slice type the walk's []any case does not match. A YAML NUL
escape in either reached storage.
It now MARSHALS the artifact and searches the output for the escape
encoding/json produces. A type switch over a struct that grows is a list
that goes stale in silence; marshalling covers every exported field,
including ones added later. The one thing it cannot see is invalid UTF-8
(which marshals to U+FFFD), and it does not need to: step 2 rejects that
in the request bytes, and YAML cannot manufacture it from valid input —
its escapes name code points, where \0 names a NUL.
Both new cases fail with the check disabled; the raw-byte cases still
pass, killed by the raw check, so each leg is discriminating.
2. THE WALK WAS NOT PATH-AWARE. A collection may declare a user field
literally named `schema` or `tags`. The walk consulted the wire-key list
at every level, so `{"fields":{"schema":"..."}}` treated a user field
name as a wire key and refused valid text holding a JSON example.
The key list is now consulted only OUTSIDE caller data — not under a
natural `fields` object, not inside an element of a `tags` array, not
inside a re-parsed document. Combined with round 7's fix that makes the
descent exactly one level deep BY CONSTRUCTION, which is why the depth
counter is gone: with the flag no longer inherited, a bound could never
fire, and dead protection reads as protection. The depth-bound test is
replaced by one that pins the property directly — an escape IN the
parsed document is refused, one BELOW it is accepted, and a
wire-key-shaped user field does not restart the descent.
3. THREE COMMENTS CORRECTED, all mine, all of the kind a reader would
believe without checking:
- MaxBytesReader: Close FORWARDS to the underlying body rather than
being a no-op, and with a nil writer there is no automatic 413 — the
cap surfaces as a read error the callers turn into 400. Behaviour
unchanged; only the claim was wrong.
- parseArtifactRequest said "three checks" while implementing five, and
its returns list omitted ErrArtifactUnbindableText. Both added by this
branch, which is exactly the prose a change is most likely to falsify
(CONVE-23).
- errJSONBodyNUL claimed all 65 callers surface its message. The STATUS
is uniform; the wording is not — several substitute a generic string.
4. And one in a test: the timeline fixture said both backends hold a
CHECK constraint a raw NUL violates. items.fields is a plain TEXT column
with no CHECK on SQLite. What was OBSERVED is "SQL logic error:
malformed JSON"; the likely source is an expression index over
json_extract, and that attribution is recorded as NOT verified rather
than asserted.
Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN
* fix(server): a regression this branch introduced, and the same trap again (BUG-2803)
Codex round 9. Both findings are mine, one of them a regression from the
round-8 restructure two commits ago.
1. THE ROUND-8 RESTRUCTURE REOPENED THE ORIGINAL DOOR. Taking the
JSON-encoded branch for a listed key skipped the plain "does this string
contain a NUL" check and asked only "does the document this string
carries hold an escape". Those are different questions. So
{"fields":"a<NUL escape>b"} — a direct NUL in the fields value, the very
first case this whole change closed — was accepted again.
Both checks now run. The test pins all three legs: a direct NUL in the
fields string, an escape inside the fields document, and an ordinary
fields string that must still be accepted, so the first two cannot pass
merely because everything under a listed key is refused.
2. THE ARTIFACT CHECK FELL INTO THE TRAP IT WAS WRITTEN AGAINST. It
searched the MARSHALLED bytes for the escape sequence, and a value
holding the six LITERAL characters marshals to a doubled backslash which
still contains that sequence as a substring — so valid content was
refused. Artifacts are documentation; text about a JSON escape is
exactly what one carries.
Worse than the bug: the comment I wrote asserted the ambiguity "cannot
arise here". It was the same doubled-backslash case bodyDecodesNUL exists
to resolve, one function away, and I wrote a sentence explaining why it
did not apply instead of checking. The marshalled form is now decoded
again and walked with the same machinery — the round trip is what makes
every field reachable without a type switch, the walk is what makes the
answer exact.
Its test asserts literal escape TEXT is accepted in title, body and a
field value, with a counterfactual leg asserting a real NUL in each of
those places is still refused, so acceptance cannot come from the check
doing nothing.
Reverting either fix fails its test and only its test.
Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN
* docs(backup): the one case where an export is not importable (BUG-2803)
Codex round 12, an operational pass. It found no migration or config
requirement, and two documentation gaps.
docs/backup.md promises that application-level export/import is portable
across SQLite and PostgreSQL. Since BUG-2803 that has one exception: a
workspace whose stored data contains a NUL exports fine and is refused on
import. It can only affect data written before the rule existed and only
on SQLite, which accepted it — a PostgreSQL instance never stored one.
`pad db migrate-to-pg` has the SAME problem and reports it worse: it
copies rows directly and never passes through the import guard, so a
legacy row fails against PostgreSQL's JSONB parser partway through the
copy rather than being refused up front. That is the likelier way an
operator meets this, since it is the operation that puts an entire old
SQLite database in front of PostgreSQL for the first time. Recorded on
BUG-2810, which owns the preflight and repair.
Round 12's other finding — that the PR's stated release note covered the
JSON 500-to-400 change and none of the rest — is fixed in the PR body
rather than in the tree.
Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN
* test(server): close two blind spots the tests themselves had (BUG-2803)
Codex round 13, asked whether the new TESTS are sound. Five findings;
these are the two that were self-contained. The other three are recorded
on the item with what each needs.
1. THE COMPLETENESS SCAN WAS BLIND TO FORM BODIES. It matched only
`.Body`, so FormValue / ParseForm / MultipartForm — which read the
request body just as surely — were invisible. It therefore reported full
coverage while the OAuth form-encoded handlers were entirely outside its
view. Widened, and it immediately failed on handlers_oauth.go, which is
the instrument working.
That file is now ACCOUNTED FOR AS A KNOWN GAP rather than as safe: the
OAuth handlers read form-encoded bodies that no rule in this family
covers (the transport rules see the query half of r.Form, not the body
half), tracked as BUG-2811 and needing a fosite-backed fixture to
measure. The test now STATES the gap instead of being blind to it, which
is the difference between a completeness claim and a completeness
appearance.
2. THE TRUNCATION TEST ADMITTED AN IMPLEMENTATION THAT RETURNED "". Its
assertions were: within the limit, bindable text, a prefix of the input.
An empty string satisfies all three. It now also asserts that an input
fitting the limit comes back UNCHANGED, and that no more than one rune
(4 bytes) is lost to the boundary — so a truncator that drops too much
fails, not just one that keeps too much.
Both were found by asking whether a broken implementation would pass,
which is the question CONVE-12 is about and which I had applied to the
production code and not to these two tests.
Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN
* test(server): the three remaining round-13 gaps (BUG-2803)
Codex round 13's other three findings, all of the same shape: a test
that would stay green with the production change reverted.
1. THE MANIFEST CHECK WAS UNTESTED. The bundle test built archives
containing only pad-export.json, so disabling the INDEPENDENT attachment-
manifest check left the suite green. The new test builds a bundle with
both entries, differing only in the manifest, so a refusal cannot come
from the export half. Verified by disabling each check separately: only
the matching test fails, so the two are independently covered.
2. THE TEST-EMAIL CHANGE HAD NO HANDLER-LEVEL TEST. Every existing leg
exercised decodeJSON, so reverting handlers_admin.go to default EVERY
decode failure to the admin's own address passed them all. The new test
drives the real endpoint with a wired mock sender and pins the
distinction that used to collapse: an ABSENT body still means "send it
to me" (control), an ordinary body still sends (control), and a body that
is present and refused answers 400 rather than being reinterpreted as
the default recipient.
3. THE MULTIPART LEG CHECKED ONE BYTE CLASS. A filter rejecting NULs
while letting malformed UTF-8 through would have passed it. It now drives
both, which matters because invalid UTF-8 is the class that reaches
Postgres as 22021 on a UTF8 database.
Round 13 was asked whether the new TESTS are sound — deterministic,
order-independent, and failing on broken code. It reported the fixtures
isolated and found five ways they were not discriminating. Two were
fixed in the previous commit; these are the rest.
Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN
* test(server): pin the wiring at every call site, not one (BUG-2803)
Codex round 14 confirmed round 13's five, then found the same shape one
level out: reverting a SINGLE call site back to the unsafe form left the
whole suite green, because the surviving fixtures are ASCII and a
helper's unit test does not care who calls it.
TestTextSafeHelpersAreUsedAtEveryCallSite asserts the wiring STATICALLY
rather than adding a fixture per site (an OAuth connection, a cloud
login, four audit paths). A byte-slice truncation of a caller string
fails it, and so does a raw User-Agent read outside the exempt set. Both
directions are checked: finding none of the SAFE form also fails, so a
scan that silently matched nothing cannot pass forever.
The User-Agent exemptions carry counts rather than being blanket, so a
NEW raw read in an exempt file still fails. All four reads in
handlers_auth.go are exempt because they feed a HASH — CreateSession
hashes the header and stores no text — and sanitising before hashing
would be actively harmful: login would store sha256(sanitised) while the
session check still hashes the RAW header, failing validation for every
client with a non-UTF-8 User-Agent. middleware_request_text.go's one raw
read is requestUserAgent itself.
Verified by reverting one truncation call site and one User-Agent call
site independently; each fails the test.
Round 14's third finding is fixed behaviourally rather than statically,
because the static scan cannot see it — handlers_oauth.go is already
listed for its form-body reads. TestOAuthRegisterRefusesNULBody drives
the real dynamic-registration endpoint with cloud mode and an OAuth
server wired, with a control leg registering successfully, and pins both
the refusal and the message split: the body IS valid JSON, so the answer
must not send a client hunting a syntax error.
Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN
* fix(server): match wire keys the way the decoder does (BUG-2803)
Codex round 16, asked whether this change is consistent with its siblings
in the same file and extensible by someone who did not write it. It found
a live bypass instead.
encoding/json matches an incoming key to a struct field by an exact match
first and a CASE-INSENSITIVE one otherwise, so {"Fields":...} and
{"FIELDS":...} land in ItemCreate.Fields exactly as {"fields":...} does.
The walk looked the key up case-SENSITIVELY, so it skipped the nested
document for a body the handler went on to accept, and the database
answered the original 500.
Measured before the fix: `fields` refused, `Fields` and `FIELDS`
accepted.
This is the same defect shape as everything else in this unit — a check
that agrees with one layer's rules while the layer that actually consumes
the value uses different ones — which is why the fix is a PREDICATE
rather than a wider map: the map is the vocabulary, and the matching RULE
belongs to the consumer. Someone adding a key should not also have to
remember to add its spellings.
The test drives six spellings including mixed case, with a control
asserting an unlisted key stays caller data in any casing, so this is
case-insensitive matching rather than matching everything.
Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN
* fix(server): fold keys the way encoding/json folds them (BUG-2803)
Codex round 17, first of five findings. The previous commit fixed the
ASCII half of key matching and left the Unicode half, which is this
bug's own pattern one more time.
encoding/json matches with Unicode SIMPLE FOLDING, not lower-casing.
U+017F LATIN SMALL LETTER LONG S folds to 's', so "ſchema" reaches the
`schema` struct field while strings.ToLower("ſchema") is unchanged and
missed the allowlist — a nested NUL under that spelling reached the
handler undetected.
Matching is now strings.EqualFold against each canonical key. The test
carries both a lower-case fold spelling and an upper-case one alongside
the ASCII cases, and keeps its control asserting an unlisted key stays
caller data in any casing.
The other four round-17 findings are recorded on the item rather than
patched here: they are genuine layer disagreements (duplicate keys
merging differently in a typed decode than in a map, a scan-failure
disposition on inputs the typed decode tolerates, and unknown-field
policy) whose fixes are design decisions rather than corrections, and
this seat is near its context bar. Each is written up with the
measurement it needs.
Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN
* fix(server): pin that a NUL-bearing manifest refusal keeps the partial workspace (BUG-2803)
Codex round 18. The comment on the manifest NUL branch said refusing the
input "stops it from being reached this way" and stopped there, which
reads as though the refusal undoes the import. It does not.
A plain error with a non-nil workspace keeps the partial workspace,
exactly as every other manifest failure in this loop does — the rollback
branch fires only for *importStatusError, and mid-stream manifest
failures intentionally keep what was imported (TASK-896). Returning a
rollback-shaped error here would give NUL-bearing manifests different
semantics from malformed ones, which is a change to the bundle-import
contract rather than a fix to this bug.
So the behaviour is unchanged and now DELIBERATE: the comment states it,
and the test asserts the persisted state rather than only the HTTP
answer. Mutation: routing the branch through *importStatusError makes
the refusal roll back, and the new assertion fails naming the release
note it would falsify. The pre-existing status/body assertions do not
notice.
Claude-Session: https://claude.ai/code/session_01AUvLoXsKdS5sdpYju6rj4p
* docs(server): record the four map-model disagreements as dispositions, and pin them (BUG-2803)
Lead ruling day-68 is land-and-follow: this branch lands on its measured
commits, and the token-stream rewrite is the BUG-2812 unit's spec rather
than a late restructure of an 18-commit branch under review pressure.
That makes the four open findings from rounds 16-17 something to WRITE
DOWN precisely, not something to leave in a trail comment.
The doc comment on bodyDecodesNUL now carries all four, with the one
root cause named: this scan decodes into map[string]any and the typed
decode does not agree with that model about keys. Two under-refuse
(duplicate-key merge; scan-failure passthrough) and are BUG-2812's spec
- both dissolve under a walk that never builds values. Two over-refuse
(unknown fields; case-variant duplicates) and are ACCEPTED, because
refusing is the safe direction. The asymmetry is stated rather than
smoothed over: within the map model, (1) and (4) are one defect seen
from two sides and only one of them fails safe.
Finding (3) is an observable compatibility change - a forward-compatible
field carrying a NUL escape now gets a 400 where it got a 200 - so it
goes in the release note as well as here. A qualification only protects
where the actor meets it.
All four are pinned by a test, measured on this tip rather than carried
over from the round-16/17 write-up. The two known-gap legs assert the
WRONG answer on purpose: when BUG-2812 lands they FAIL, naming the doc
comment and the release note as what to update. Both gap legs carry a
premise assertion - the same bodies with the disagreement mechanism
removed ARE detected - without which they would pass against a check
that detected nothing.
Claude-Session: https://claude.ai/code/session_01AUvLoXsKdS5sdpYju6rj4p
* test(server): wire release-note item 10 to the router, with its before-state measured (BUG-2803)
The disposition test proves bodyDecodesNUL RETURNS true for an unknown
field carrying a NUL escape. The release note claims the API answers
400. Those are different claims and only the second one is what an
operator or client author reads - CONVE-19, my own convention: a
direct-call test vouches for the component, not its binding.
Two legs, and the control is the load-bearing one. An unknown field with
an ordinary value must still be ACCEPTED, so this pins "refused for the
NUL" rather than "refused for being unknown". The handler does not
reject unknown fields; if it ever started to, the note's explanation
would be wrong while its status code stayed right, and no
status-code-only assertion could see that.
The before-state is measured rather than asserted from memory. Disabling
the check makes the same request answer 201 - which is main's behaviour,
since decodeJSONWithLimit there unmarshals straight into the typed value
and the key is dropped. So "answers 400 where it answered 200" is a
measurement in both directions, not a recollection of one.
Claude-Session: https://claude.ai/code/session_01AUvLoXsKdS5sdpYju6rj4p
* docs(backup): the NUL rule lives in the binary, not the database (BUG-2803, BUG-2813)
Codex round 19, the fresh-angle deploy/rollback/mixed-version pass.
docs/backup.md said a NUL-bearing row "can only affect data written
before that rule existed, and only on SQLite". The second half is true.
The first half is false, and the reason is the interesting part: the
guard is in decodeJSONWithLimit, so the invariant is a property of the
running BINARY, not of the database.
On SQLite any window where an older binary serves the same database can
still write one - a rollback after upgrading, a staged rollout with an
old and a new instance sharing a database, a second older instance on
the same file. The window closes, the guard returns, and the rows are
already stored, behaving exactly like genuinely old ones. A rollback is
an ordinary operational move, so this is not an exotic path.
The doc now states the binary-version dependence, says which dialect is
affected and why PostgreSQL is not (it refuses a NUL itself, at every
version), and gives the operational answer: drain writes from older
binaries before the new one serves, or roll forward rather than back.
Store-layer enforcement - so the running build stops mattering - is
filed as BUG-2813 rather than added here. It is a dialect-level change
and the day-68 ruling on this unit is land-and-follow.
The same false implication was carried by the PR's release note calling
such a workspace "legacy"; corrected there too.
Claude-Session: https://claude.ai/code/session_01AUvLoXsKdS5sdpYju6rj4p
* docs(server): cite the ruling in house style, not the team-room day counter (BUG-2803)
"lead ruling day-68" is the internal day counter, which means nothing to
anyone reading this repo and is inconsistent with every other citation
in it - the codebase cites a lead ruling by DATE or by BUG ref, never by
day-N. Replaced with the bug ref, which is the part a reader can
actually follow.
Claude-Session: https://claude.ai/code/session_01AUvLoXsKdS5sdpYju6rj4p
* docs(server): drop a commit count I had already measured as wrong, and stop asserting a cause I borrowed (BUG-2803)
Two defects in a comment I wrote an hour ago, both of the kind this
unit's trail keeps recording.
"an 18-commit branch" - the branch was 20 commits at
|
||
|
xarmian
|
31c8d426dd |
fix(web): suppress notes/decision change pills in the activity feed (BUG-2628) (#1211)
* fix(web): suppress notes/decision change pills in the activity feed (BUG-2628)
Dave's ruling, option 3. Implementation notes and decision-log entries have
had their own timeline cards since BUG-2301, so a change pill for them can
only restate the card above it. On items whose notes predate the write-time
summarizer, it restates it badly: the frozen activity metadata holds the
whole notes array as a Go map literal and the pill renders as a wall of text
that dwarfs every real change on the card.
Suppressed at RENDER time and deliberately not at write time. The activity
row legitimately records THAT notes changed and that belongs in the audit
trail; what the ruling removes is showing it as a pill. Dropping it from the
record would be option 2 — rejected in the filing for destroying the
original — wearing a different hat. One filter in parseFieldChanges covers
both surfaces that render pills (the timeline card and the activity page).
THE FIELD-NAME FILTER ALONE DOES NOT FIX THE REPORTED SYMPTOM, which I found
by measuring against the 97 legacy rows in the live database rather than
assuming the obvious fix worked:
today pills=198 longest=2952 over-200-chars=77
suppress the two names pills=121 longest=2952 over-200-chars=1
+ field-key guard pills=120 longest= 15 over-200-chars=0
The parser splits on ";" and a legacy blob contains semicolons, so a
fragment of a note's own prose parses as a change whose "field name" is a
paragraph of markdown — and that fragment is the LONGEST pill on the worst
row. Suppressing by field name cannot reach it. Requiring a field name to
look like one (lowercase identifier, which is what the server emits: schema
keys plus title/role/assigned) does.
The guard cannot refuse a legitimate pill: across 4000+ current-format rows
carrying 4329 pills, it drops zero. That control is what distinguishes it
from a length heuristic.
Also corrected a pre-existing comment on the function, which claimed
segments without a "from → to" transition are dropped. They are not:
"field: → value" splits into ["", "value"], two parts, and is kept with an
empty `from`. What is actually dropped is a segment with no arrow, or one
with more than one. I only noticed because the false comment made me write a
test expectation that failed — worth fixing where the next reader will hit it.
Mutation matrix, 4: dropping the suppression set, dropping the key guard,
suppressing only implementation_notes, and loosening the key guard to accept
anything — each detected, and the two rules die on different tests.
Gates: vitest 1889 passed (111 files), svelte-check 0 errors, vite build
clean. No Go changes.
Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X
* fix(web): finish the suppression on the surfaces that render raw (BUG-2628)
Codex round 1. Two findings, both mine, and the first meant the previous
commit did not fix the bug on the surface the ruling was about.
**The audit page renders the RAW change string when the parsed list is
empty.** Suppressing every pill on a legacy row therefore made the wall of
Go-map text REAPPEAR there — my change made that surface worse, not better,
for exactly the rows the bug is about. The dashboard is simpler and was
never covered: it renders `meta.changes` verbatim with no parsing at all.
So the suppression has to be a property of the STRING, not of the pill list.
`formatChangesForDisplay` drops segments on the same two rules and keeps
every survivor verbatim — including ones `parseFieldChanges` will not turn
into a pill, such as a value containing more than one arrow. That is what a
fallback is for, and it keeps working.
**The key guard was too strict, and my justification for it was too strong.**
It was `^[a-z][a-z0-9_]*$`, reasoned from "the server emits schema keys plus
title/role/assigned, all lowercase". Nothing constrains a collection's field
keys to lowercase — handlers_collections.go compares them with a plain `==`,
no case folding — so `Status` and `resolution-v2` are legal keys whose pills
that pattern would have dropped silently.
Replaced with a STRUCTURAL test: non-empty, no whitespace, length <= 64.
That is what actually separates a key from the prose fragment the guard
exists to reject.
The measurement I had could not tell the two guards apart, and the reason is
worth keeping: all 72 field keys in the live database satisfy both forms, so
the control leg showed zero legitimate pills dropped for BOTH. **A control
showing a guard refuses nothing HERE is not evidence that it cannot refuse
something legitimate** — the same partial-verification shape I have hit
before, this time in a control I built myself and trusted. Measured both on
the same data: identical on the legacy rows (longest surviving pill 15
chars, none over 200) and identical on 6000+ current rows (6385 pills kept,
zero dropped). Same effect, strictly smaller risk.
Mutation matrix, 6: dropping suppression or the key guard in either function,
loosening the guard to accept whitespace, and suppressing only one of the two
fields — each detected, each by the test aimed at it.
Gates: vitest 1894 passed (111 files), svelte-check 0 errors.
Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X
* docs(web): record the enumerated scope of the suppression (BUG-2628)
Codex round 2, asked to ENUMERATE every surface that renders an activity's
change string rather than to spot-check the two I had just fixed. It
returned the full inventory, and the result splits cleanly in two.
Deliberately unsanitized, now stated in the code: the REST endpoints,
`--format json`, the bootstrap payload and the MCP tools all return the
metadata verbatim. That is the design, not a gap — the row records THAT
notes changed, the ruling was about not SHOWING it as a pill, and a client
asking for the raw record must still get it. Sanitizing the wire would be
the destroy-the-original option the filing rejected, arriving by a
different route.
Not covered and filed as BUG-2789 rather than silently pulled in: the CLI
(`pad project activity` in both output modes, `pad workspace audit-log`)
prints the string with no parsing, and the admin console audit log renders
it verbatim through a generic metadata fallback. The CLI needs a Go twin of
formatChangesForDisplay — the filing points at the AgentNameFromMetadata /
agentNameOf precedent and warns that Go's unicode.IsSpace and JS's \\s
disagree in both directions, which matters because the guard is a
whitespace test. The admin audit log may want the OPPOSITE answer, since a
forensic tool showing less than the record contains is arguably the wrong
fix; bounding the rendered length would be the alternative. Both are
product calls this bug's ruling does not settle.
No behaviour change in this commit.
Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X
* docs+test: state the guard's real limits instead of overclaiming (BUG-2628)
Codex round 3. Three findings, no redesign — all three are places where my
claims were stronger than what the code does, and two of them are now
pinned as tests rather than corrected in prose and left to rot.
**The guard is a heuristic and both error directions are real.** I wrote
that a structural test "is what actually separates a key from prose". It
is not, and nothing could be: the server joins with "; " and the consumer
splits on ";", so a value containing a semicolon is indistinguishable from
two changes. Concretely, a fragment whose prose before the colon is a
single word still produces a pill — `…[map[details:Root; foo: a → b]]`
yields a stray `foo`. And a legitimate key with a space, or over 64
characters, is droppable; nothing validates key shape on collection create.
What the guard actually delivers is the measured claim, and that one
stands: across the 97 legacy rows the longest surviving pill goes from
2952 characters to 15, and none exceeds 200. It removes the WALL. It does
not promise zero fragments, and the comment now says so.
**"Preserved VERBATIM" was false.** formatChangesForDisplay trims each
survivor and rejoins with a canonical "; ", so boundary whitespace and
separator spacing are normalized; and a legitimate value containing a
semicolon loses its tail, because the orphaned fragment fails the field-key
rule. Now described as keeping CONTENT, with both normalizations named.
Two new tests pin the limits so they are known rather than latent: the
single-word fragment that still produces a pill (asserting the survivor is
small, which is the property that matters), and the semicolon-bearing value
that loses its tail. A limit with a test is a decision; a limit in a comment
is a hope.
One test renamed from "emits no pill for a legacy blob" to "removes the
wall from a legacy blob" — the old name asserted the absolute the code does
not provide.
Filed IDEA-2790 for the durable fix: emit changes as structured metadata
alongside the string, so no consumer parses a display format. It does not
help the frozen legacy rows, which is why the heuristic stays either way,
but it would shrink BUG-2789 considerably. The comments here point at it.
Gates: vitest 1897 passed (111 files), svelte-check 0 errors, vite build
clean.
Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X
* fix(web): the sanitizer must reject fragments, not just bad keys (BUG-2628)
Codex round 4, and it found a gap in my MEASUREMENT, not just my code.
formatChangesForDisplay filtered on the field-key rule alone, so a fragment
from inside a serialized notes array whose text before the colon is short
and unspaced — `id:note-1775153870894988317 summary:…` — posed as a key and
was kept. The audit page's fallback then rendered it: the wall, back on the
surface this function was written for.
Requiring at least one arrow drops it. Not exactly one: a value containing
"→" is a real change that parseFieldChanges declines to make a pill of, and
showing it is precisely what a fallback is for. Both directions are pinned
by different tests, so neither rule stands in for the other.
Measured on the 97 legacy rows: the longest sanitized output falls from 285
characters to 54. On 6000+ current-format rows the new rule drops exactly
ONE segment — `title: Strip '…' from README`, itself the orphaned tail of a
title change that fragmented on a semicolon in the title, so dropping it is
correct rather than a cost.
THE PART WORTH KEEPING: every figure I had quoted for this fix — 2952 to
15, 77 oversized pills to zero — was measured on parseFieldChanges. I never
measured formatChangesForDisplay against the corpus at all, and reported
the pill numbers as though they covered the fix. They covered half of it,
and the unmeasured half was leaking 285 characters of blob to the surface
the ruling was actually about. Two functions, one measurement, and the
write-up did not distinguish them.
That is the same shape as the guard-control error from round 1 — a
measurement that proves something narrower than the sentence it is attached
to — hit twice in one unit, in the same direction both times.
Gates: vitest 1898 passed (111 files), svelte-check 0 errors.
Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X
* docs(web): record every way a legacy blob can still reach a reader (BUG-2628)
Codex round 5, asked to enumerate rather than spot-check: for the three
surfaces this PR covers, list every distinct path by which a legacy notes
blob could still reach a user. It returned the complete table and found no
path the previous rounds had missed.
Three handled, two open. The two open rows — a key-shaped fragment with
exactly one arrow, and one with several — are the format ambiguity itself
and are not closable by any shape test, since the server joins with "; "
and the consumer splits on ";". Both already have tests, so they are known
limits rather than latent surprises, and IDEA-2790 is the fix that removes
the class rather than bounding it.
The table is in the code because the enumeration is the deliverable. A
reader asking "is this fully closed?" now gets the answer and its shape
without re-deriving it, and the honest answer is BOUNDED, not closed: on
the 97 legacy rows the longest surviving pill is 15 characters and the
longest sanitized fallback string is 54, from 2952 and 285.
Findings by round: 2, 2, 3, 1, 0-new. No behaviour change in this commit.
Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X
|