mirror of
https://github.com/PerpetualSoftware/pad.git
synced 2026-10-25 14:00:40 +00:00
cacf492d95412ecbbaf1ba7e7d8cb0413da90fcd
1855 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Matt Faltyn
|
cacf492d95 | feat(agent): install compact shared skill | ||
|
Matt Faltyn
|
328e0a4706 | feat(agent): add on-demand guide | ||
|
Matt Faltyn
|
d89587b4a8 | test(agent): add prompt payload budgets | ||
|
xarmian
|
c1e57ee557 |
fix(comments): the "commented" activity and the comment commit in one transaction (BUG-2716) (#1388)
handleCreateComment and handleCreateReply wrote the activity row first, in its own transaction, then the comment. The order is forced — the comment carries the activity's id under a foreign key — so a comment failure left an orphan "commented" entry on the item timeline with nothing behind it, and the request answered 500. Measured before the change: after a forced comment INSERT failure the item carried two "commented" activities and one comment, at both sites. One store call, CreateCommentWithActivity, writes both rows in one transaction: the activity through an executor-taking split of CreateActivity, then the existing comment body — attachment-ref stamp, INSERT, read-back, comment.created outbox event — on the same *sql.Tx. A failed comment now takes its activity with it. The user-write touch moves after the commit so a failed comment is not counted as a write. The third site the trail enumerates — an item update carrying a comment — is deliberately unchanged (lead-ruled): its "updated" activity records a write that has already committed and must survive a comment failure, on both of its branches. Said in a comment at the site so it is not re-filed. Tests, both dialects, per site: a comment-INSERT seam in the commit-hook shape forces the failure; the request answers 500, the comment does not exist, and the "commented" activity count is unchanged. The pre-fix control was red on the activity-count line at both sites. |
||
|
xarmian
|
c3480834c2 |
fix(import): a kept partial bundle import is attached to its importer instead of left ownerless (BUG-2709) (#1386)
* fix(import): a kept partial bundle import is attached to its importer instead of left ownerless (BUG-2709) BUG-2709 is the parent filing of the husk class closed door by door in BUG-2715, BUG-3087 and BUG-3094. Measured at main before this change: its first claim (the seeder is non-atomic) is still true of the function but a half-seeded workspace is no longer a persisted outcome on any door; its second claim (ImportWorkspace mints the row outside its transaction) is false at main. The one residual is the bundle door's mid-stream KEEP path (TASK-896): a plain error after pad-export.json was imported — an undecodable manifest, an unreadable blob — keeps the partial workspace, and the owner row was only ever added on success. So the kept workspace was live, ownerless, absent from the importer's list, not restorable, 403 on read and on delete, and holding its slug so a retry landed on `name-2`, while the 400 told them it existed. The owner is now attached on that branch through addOwnerOrCompensate. The helper is FATAL, so the outcome is one of two, and the response says which from a read of the row rather than from the helper's error text: kept and reachable (named by slug), or removed because the owner row could not be written. A caller with no resolved user has nobody to attach and keeps today's answer. TASK-896's rollback-or-partial design is not decided here; the kept state is simply no longer an orphan. Tests, both dialects: the keep path as a member — the workspace is in the importer's list, readable and deletable, and the message names it (pre-fix control red on the list line and both reads); and the compensation leg via the commit-hook seam — removed, not restorable, message says removed. * fix(import): the keep-arm message claims only existence and unconfirmed ownership per Codex review (round 2) addOwnerOrCompensate keeps the workspace on two of its error arms — the membership read failed (an ack-lost commit may have landed the owner row) or a row exists with a non-owner role that still permits reading — so a read of the row cannot establish reachability. The message now says the workspace still exists and ownership could not be confirmed; both arms are covered through the membershipCheck seam. |
||
|
dependabot[bot]
|
b7d19e68c8 |
chore(ci)(deps): bump DeterminateSystems/nix-installer-action (#1367)
Bumps [DeterminateSystems/nix-installer-action](https://github.com/determinatesystems/nix-installer-action) from 22 to 23. - [Release notes](https://github.com/determinatesystems/nix-installer-action/releases) - [Commits](https://github.com/determinatesystems/nix-installer-action/compare/ef8a148080ab6020fd15196c2084a2eea5ff2d25...3138316df39ed29be04236d7ffc686fa525866aa) --- updated-dependencies: - dependency-name: DeterminateSystems/nix-installer-action dependency-version: '23' dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> |
||
|
xarmian
|
8ca7e55159 |
fix(import): a rejected bundle import leaves no restorable husk, and the shared removal reclaims blobs before it purges (BUG-3094) (#1385)
* fix(import): a rejected bundle import leaves no restorable husk, and the shared removal reclaims blobs before it purges (BUG-3094) The bundle import's validation-reject rollback (a duplicate pad-export.json or manifest.json, an unsafe entry name, a blob ahead of its manifest) called DeleteWorkspace, a soft delete. ListDeletedWorkspaces is scoped by owner_id and ImportWorkspace writes no member row — the owner row lands only after success — so the rejected import sat in the importer's deleted-workspaces list for 30 days, and restoring it returned a workspace the importer could neither read nor delete (measured: 403 on both, for a member). The fourth member of the class BUG-3087 closed on the cloud auto-create door; the same helper now removes it. What this door has that the auto-create doors did not: the rollback fires after attachment blobs may have been rehydrated. The orphan GC discovers reclaimable blobs THROUGH tombstoned rows, and PurgeWorkspaceData hard-deletes those rows without touching bytes — so the helper applied as-is freed the husk and leaked the blob for good (measured: blob still on disk, no row left to find it by). removeUnusableWorkspace now lists the workspace's attachments after the soft delete and, when there are any, reclaims them with the retention sweeper's own reclaimWorkspaceBlobs before purging; when reclaim is refused (transient error, or an upload of the same content in flight) it stops after the soft delete, logs the residual at Error, and leaves the tombstones plus the soft-deleted row for the sweeper, which finishes with the same ordering. Doors without blobs are unchanged. Tests, both dialects: a rejected import with a real blob leaves no husk, no rows, and an empty backend — red before on the husk and the blob; the refusal leg via the in-flight hash mark keeps the tombstones and the blob, logs the residual, and one driven sweep tick then purges and reclaims. * fix(import): serialize reclaim-then-purge across the door and the sweeper per Codex review (round 1) The reclaimer's dedupe guard counts OTHER workspaces' rows for a storage_key and skips a shared blob's delete on the strength of them, which is sound only while no second sequence can purge those rows in between. The sweeper was the only caller and ran as one goroutine; removeUnusableWorkspace made it two, so two concurrent rejected imports carrying the same blob would each skip the delete and both purge, leaving a blob no row can find. One mutex, held across list → reclaim → purge in both; the sweeper's per-candidate step is extracted so the lock discipline lives in one place. |
||
|
xarmian
|
08236c7459 |
test(web): enumerate $state declarations by STATEMENT in the fence-source core (BUG-3084 M12) (#1384)
* test(web): enumerate $state declarations by STATEMENT in the fence-source core (BUG-3084 M12)
Four page guards carried `let\s+(\w+)\s*(?::[^=]*)?=\s*\$state` with the m flag.
The negated class admits a newline, so an uninitialised typed `let` on the
line above a `$state` declaration matched as ONE declaration named after the
wrong variable and the real one was never enumerated — the dashboard page has
exactly that pair, and its guard went red for a name that is not $state while
silently missing the one that is. The narrow fix ([^=\n]) closes that case and
opens the opposite one, a genuine annotation spanning lines.
`stateDeclarations()` walks each `let` statement to its terminating `;` at
bracket depth zero and asks whether it initialises with $state; it fails
closed on a statement that never ends. The core's suite drives the adjacency
case with the legacy regex as the control that must stay red, the multi-line
generic, an object type with semicolons inside, a string with a semicolon, and
the FenceSource binding. All four guards now read the core. Receipt: on the
library, roles and collection pages the legacy regex and the new enumerator
agree (9, 22, 49); on the dashboard they differ by exactly pollTimer→onboardingDismissed.
* test(web): the $state enumerator reads the grammar it walks, per Codex review (round 1)
Four constructed shapes, each accepted by the compiler, each read wrong by
the first scanner's bare depth counter: a regex literal containing a brace
swallowed every declaration after it; only the first binding of a
multi-binding let was inspected; a generic default (<T = string>) was taken
for the initialiser; and two statements joined by automatic semicolon
insertion recreated the wrong-name bug the hoist exists to close. Brackets
are now skipped with the core's own string/regex-aware matchDelimiter,
statement-level regex literals use its previous-significant-character rule,
a depth-zero comma outside a type annotation starts the next binding, angle
brackets are tracked only in type position, and a newline followed by a
statement keyword ends an uninitialised or already-initialised binding.
Destructuring patterns are walked and refused only when backed by $state.
Each shape is a fixture; the receipt on the real pages is unchanged.
* test(web): the $state enumerator's outer search is literal-aware and reads type arguments, per Codex review (round 2)
Three more compiler-accepted shapes: a type-argument list on the initialiser whose comma split the bindings; a parenthesised initialiser; and the outer let search matching inside a template literal and enumerating a phantom. The search is now the same literal-aware walk the statement scanner uses; angle brackets after the initialiser are tracked when they directly follow an identifier; a parenthesised $state is still $state. Each shape is a fixture; the receipt on the real pages is unchanged.
* test(web): type arguments are decided by their call paren and initialiser bodies are descended, per Codex review (round 3)
An unspaced comparison (1<2) opened an angle depth that suppressed every boundary to end of input; a let nested in an initialiser's function body was skipped, though the regex this replaces enumerated it. A < after the initialiser is now a type-argument list only when its matching > is immediately followed by (, and bracket bodies are walked for nested declarations rather than skipped. Both are fixtures; the receipt on the real pages is unchanged.
* test(web): interpolation bodies are walked, an enclosing closer ends a binding, nested type arguments keep depth, per Codex review (round 4)
A template's ${…} body is code and was skipped with the text around it; a binding with no semicolon ran past the } of its enclosing block into the next declaration; a nested < inside an open type-argument list did not increment the depth, so the inner > exposed a type comma as a binding separator and the guard threw on valid code. Each is a fixture; the receipt on the real pages is unchanged.
* test(web): whitespace in type arguments, arrow-returned regexes, type members named let, and the population table, per Codex review (round 5)
Three more constructed shapes fixed with fixtures. Five rounds each found new members of one class — grammar the scanner had not been told about — so per CONVE-35 the class is now written down: the doc comment carries the table of what the scanner reads, refuses, and does not read, bounded by the receipt that on every page in the family it agrees with the legacy regex except for the dashboard swap that motivated the hoist.
|
||
|
xarmian
|
9aa6c7b762 |
fix(cloud): auto-create removes a workspace it cannot seed or administer, and leaves no restorable husk (BUG-3087) (#1383)
Two instances of the same class on the cloud auto-create door, both pre-existing and both left out of BUG-2715 by that unit's ruling. SEED FAILURE was logged at Warn and skipped, and the signup reported success. Measured on the trail: the user got a workspace the dashboard and the agent bootstrap both flagged needs_onboarding, whose onboard playbook the seed never wrote (`playbooks/onboard` answered 404), and nothing re-runs the seed — autoCreateWorkspace has exactly three callers, all signup doors, and the store's zero-collection rescue has no callers at all. The door now removes the workspace through removeUnusableWorkspace and the signup still succeeds: the user row, the session and the verification email already exist by then, so a 5xx would 409 on retry for an error that was never about the account, and the console's empty state is the honest picture. No seed retry. The ABSENT arm called DeleteWorkspace, a soft delete. ListDeletedWorkspaces is scoped by owner_id, not membership, so the husk sat in the user's deleted-workspaces list for 30 days and restoring it returned a workspace with no member row — measured: 403 on every read and on the delete. The same helper replaces the bare soft delete. BUG-3026's four arms are untouched: the helper changes what the removal IS, not when it fires, and it adds the other-members guard this arm never had. Tests assert the query the console runs — ListDeletedWorkspaces(owner) EMPTY — on both paths and both dialects; the ABSENT leg was run against the pre-fix code and went red on exactly that line. A seed-failure test seam is added in the shape of the existing commit hooks, consulted once after the template's collections exist, because nothing else in the tree can fail a seed on a fresh workspace without breaking the store around it. handlers_import_bundle.go:171 has the same husk on its validation-reject rollback; filed as BUG-3094, not fixed here. |
||
|
xarmian
|
364c94fe02 |
fix(web): fence the dashboard's load on the signed-in identity (BUG-3084 surface 4) (#1382)
* fix(web): fence the dashboard's load on the signed-in identity (BUG-3084 surface 4) The workspace dashboard keyed its load on (user, workspace) and dropped the board before reloading, so an identity change that reached the effect flush replaced the board — but a load continuation queued BEFORE that flush committed the previous user's board first, and the Retry state of a failed load belonged to whoever had issued it. Every path (the keyed effect, the 30 s poll, the sync subscription, Retry, the create-collection modal) funnels into load(), so the fence lives there: an entry capture, a check between the two awaits so a lost identity issues no fetch, and a check on both commit arms. The finally arm stays seq-gated only, the library's asymmetry. An identity listener closes the two modals (the Connect modal mints a claim code for the signed-in user) and deliberately does NOT reload, because the keyed effect already does. Two instruments on the #1370 pattern: a source guard that enumerates the population (1 async fn, 1 timer, 7 effects, every load() caller, every $state dispositioned) and a behavioural suite with a control per refusal leg, a recovery leg that keeps polling, and a reactive userId in the family mock — the keyed recovery cannot be observed through a plain variable. The shared core's header gains the exemption rule from checkpoint 21. * test(web): the untrack rule states its real consequence on the dashboard (BUG-3084 surface 4, mutation M6) * test(web): inspect each load() caller's body, not merely count the calls, per Codex review (round 2) Codex constructed an extra api.dashboard.get(...).then(dash => dashboard = dash) beside Retry's load(wsSlug): the guard counted the call and never read the callback, so the previous user's board painted over the new one through a site the fence in load() never sees. Each of the five caller bodies is now delimited and held to delegating with no request, no continuation, and no write beyond its named allowance; the set is closed by count. Retry's overtaken load is a driven leg. |
||
|
dependabot[bot]
|
24ec0add38 |
chore(deps)(deps): bump the npm-minor-and-patch group across 1 directory with 4 updates (#1364)
Bumps the npm-minor-and-patch group with 4 updates in the /web directory: [dompurify](https://github.com/cure53/DOMPurify), [@playwright/test](https://github.com/microsoft/playwright), [marked](https://github.com/markedjs/marked) and [vite](https://github.com/vitejs/vite/tree/HEAD/packages/vite). Updates `dompurify` from 3.4.14 to 3.4.15 - [Release notes](https://github.com/cure53/DOMPurify/releases) - [Commits](https://github.com/cure53/DOMPurify/compare/3.4.14...3.4.15) Updates `@playwright/test` from 1.62.1 to 1.63.0 - [Release notes](https://github.com/microsoft/playwright/releases) - [Commits](https://github.com/microsoft/playwright/compare/v1.62.1...v1.63.0) Updates `marked` from 18.0.11 to 18.0.12 - [Release notes](https://github.com/markedjs/marked/releases) - [Commits](https://github.com/markedjs/marked/compare/v18.0.11...v18.0.12) Updates `vite` from 8.2.2 to 8.3.0 - [Release notes](https://github.com/vitejs/vite/releases) - [Changelog](https://github.com/vitejs/vite/blob/main/packages/vite/CHANGELOG.md) - [Commits](https://github.com/vitejs/vite/commits/create-vite@8.3.0/packages/vite) --- updated-dependencies: - dependency-name: "@playwright/test" dependency-version: 1.63.0 dependency-type: direct:development update-type: version-update:semver-minor dependency-group: npm-minor-and-patch - dependency-name: dompurify dependency-version: 3.4.15 dependency-type: direct:production update-type: version-update:semver-patch dependency-group: npm-minor-and-patch - dependency-name: marked dependency-version: 18.0.12 dependency-type: direct:development update-type: version-update:semver-patch dependency-group: npm-minor-and-patch - dependency-name: vite dependency-version: 8.3.0 dependency-type: direct:development update-type: version-update:semver-minor dependency-group: npm-minor-and-patch ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> |
||
|
github-actions[bot]
|
86609dc3f8 |
chore(nix): heal vendorHash for the module set in 10ffd924ef (TASK-2954)
|
||
|
dependabot[bot]
|
10ffd924ef |
chore(deps)(deps): bump the go-minor-and-patch group across 1 directory with 6 updates (#1380)
Bumps the go-minor-and-patch group with 3 updates in the / directory: [github.com/jackc/pgx/v5](https://github.com/jackc/pgx), [golang.org/x/crypto](https://github.com/golang/crypto) and [golang.org/x/image](https://github.com/golang/image). Updates `github.com/jackc/pgx/v5` from 5.10.0 to 5.11.0 - [Release notes](https://github.com/jackc/pgx/releases) - [Changelog](https://github.com/jackc/pgx/blob/master/CHANGELOG.md) - [Commits](https://github.com/jackc/pgx/compare/v5.10.0...v5.11.0) Updates `golang.org/x/crypto` from 0.56.0 to 0.57.0 - [Commits](https://github.com/golang/crypto/compare/v0.56.0...v0.57.0) Updates `golang.org/x/image` from 0.45.0 to 0.46.0 - [Commits](https://github.com/golang/image/compare/v0.45.0...v0.46.0) Updates `golang.org/x/term` from 0.45.0 to 0.46.0 - [Commits](https://github.com/golang/term/compare/v0.45.0...v0.46.0) Updates `golang.org/x/text` from 0.41.0 to 0.42.0 - [Release notes](https://github.com/golang/text/releases) - [Commits](https://github.com/golang/text/compare/v0.41.0...v0.42.0) Updates `golang.org/x/tools` from 0.48.0 to 0.49.0 - [Release notes](https://github.com/golang/tools/releases) - [Commits](https://github.com/golang/tools/compare/v0.48.0...v0.49.0) --- updated-dependencies: - dependency-name: github.com/jackc/pgx/v5 dependency-version: 5.11.0 dependency-type: direct:production update-type: version-update:semver-minor dependency-group: go-minor-and-patch - dependency-name: golang.org/x/crypto dependency-version: 0.57.0 dependency-type: direct:production update-type: version-update:semver-minor dependency-group: go-minor-and-patch - dependency-name: golang.org/x/image dependency-version: 0.46.0 dependency-type: direct:production update-type: version-update:semver-minor dependency-group: go-minor-and-patch - dependency-name: golang.org/x/term dependency-version: 0.46.0 dependency-type: direct:production update-type: version-update:semver-minor dependency-group: go-minor-and-patch - dependency-name: golang.org/x/text dependency-version: 0.42.0 dependency-type: direct:production update-type: version-update:semver-minor dependency-group: go-minor-and-patch - dependency-name: golang.org/x/tools dependency-version: 0.49.0 dependency-type: direct:production update-type: version-update:semver-minor dependency-group: go-minor-and-patch ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> |
||
|
xarmian
|
25b687a6cf |
test(schema): scope the decode guard by MODULE PATH, not by vendor location (BUG-2685 follow-up) (#1381)
The Nix red was fixed at
|
||
|
github-actions[bot]
|
4694b646bb |
chore(nix): heal vendorHash for the module set in 44ba666b5c (TASK-2954)
|
||
|
xarmian
|
44ba666b5c |
fix(schema): a grandfathered reserved-key FieldDef never reaches an item-field path (BUG-2685) (#1377)
`validateNoReservedFieldKeys` (BUG-2674) stops a collection schema NEWLY
declaring `implementation_notes` / `decision_log` / `github_pr` / `convention`,
but deliberately grandfathers an existing declaration — so the shape survives in
any workspace that had it before that gate landed, and every path that reasons
about fields BY CONSULTING A SCHEMA then meets a FieldDef it should not honour.
Such a declaration is wrong in both directions at once, which is what rules out
refusing the write:
- it REFUSES the system's own value. `pad item note` / `pad item decide`
append an entry and PATCH the whole fields blob back; a legacy
`implementation_notes: text` fails that ARRAY on validateFieldType, so the
collection's items cannot be annotated at all.
- it ACCEPTS a caller's junk. The same declaration waves a bare string
through, where the append helpers then refuse to touch it (BUG-2627 part 3).
And a legacy `required` declaration refuses ordinary creates over a field no
client surface offers.
ONE PREDICATE, AT THE DECODE. A type-resolved pass over the tree counted 76
sites that consult a `CollectionSchema.Fields` and only 38 that DECODE one. A
rule applied at the consultations is 76 patches that every future consumer has
to remember, which is the shape of the four review rounds this bug was filed out
of — each found another site. Every consultation inherits whatever the decode
produced, so `models.UnmarshalItemFieldSchema` strips reserved FieldDefs there,
and 23 item-field decode sites now call it. 9 move/copy decodes already strip at
the point of use via `items.SchemaForMigratedFields` (whose implementation now
delegates to the same strip, so there is one). 6 keep the raw schema because the
collection DEFINITION is the subject — notably the grandfather test's own
`prevSchema`, which stripped would reclassify every existing declaration as
newly introduced and refuse every update to such a collection.
AND AN ENFORCEMENT, TYPE-RESOLVED. A predicate nothing enforces is a convention
the next consumer has to remember. `TestReservedFieldDecodeSitesAreClassified`
walks the tree with go/packages and requires every raw decode into a
SCHEMA-SHAPED type to be on an allow-list WITH A REASON; a sibling test rejects
dead entries, and a third pins the site COUNT so the guard cannot go blind while
passing. A destination counts by SHAPE — a struct with a `json:"fields"` member
that is a slice of structs — not by name, which is what catches `bootstrapSchema`
for what it IS rather than through a registry of mirrors.
The type checker was earned, not chosen. The first guard approximated types by
reading declarations with go/ast, and FIVE consecutive review rounds each named
more DESTINATION SPELLINGS it could not see: a bare pointer parameter; an
inferred-type var; new(T); an aliased models import, `var x = new(T)`,
`dst := &s`, an inline element type; a type alias, `dst := helper()`, a closure
parameter. Fixing them one at a time was measuring how many spellings a reviewer
could think of rather than whether the guard was sound (CONVE-35). go/packages
answers the question the approximation was approximating, including for the
spellings nobody has thought of yet.
Cost, measured rather than predicted: `go mod tidy` moves golang.org/x/tools
from go.mod's indirect block to its direct one and leaves go.sum BYTE-IDENTICAL
— the module set does not change, so nix/package.nix's vendorHash is unaffected.
x/tools was already in the graph with its full module zip hash in go.sum.
The 17-spelling fixture under internal/models/testdata/decodeshapes outlived the
guard it was written for and is kept as a REGRESSION TEST on the resolver: it
fails if a future change to the shape test, the callee test, or the
destination-argument rule narrows what can be resolved. It earned its keep
immediately — it caught that a Go type ALIAS materialises as *types.Alias and
walked straight past an unwrap loop that handled only Pointer and Named.
The guard skips VENDORED paths, and that is not a tidiness rule. It passed
locally and FAILED the Nix job, which builds from a `vendor/` tree that `./...`
then walks: 57 sites instead of 20, every extra one a dependency's own decode
caught by the interface arm. A guard whose verdict depends on whether the
checkout happens to be vendored is not a guard. Reproduced locally with
`go mod vendor` before the fix and re-run after, rather than inferred from the
CI log.
Out of scope, stated rather than implied: a full `fields` blob reaching item
CREATE is still a mint site for a hand-written reserved key (undeclared keys are
ACCEPTED — BUG-2850 chose accept-and-name after a census found 168 live values
under 14 such keys). That is a different class, whose remedy is refusal and
whose risk is the system writers; a test pins that this change does not silently
alter it. Collapsing the now-idempotent `SchemaForMigratedFields` calls is left
alone too — the copy preflight distinguishes the raw source schema from the
stripped one in its reporting, so that is a second change with its own surface.
Claude-Session: https://claude.ai/code/session_019EmDd3Wy8b3cPG1TZBSzBu
|
||
|
xarmian
|
5e278aca09 |
fix(web): a reactive epoch read re-armed the collection page's search under the new identity (BUG-3084) (#1379)
`authStore.identityEpoch` is `$state`. An `$effect` that reads it SYNCHRONOUSLY takes a dependency on it, so an identity change re-runs that effect. The collection page's search dispatch read it inside the effect body, and the consequence is the sharpest defect this unit has found: 1. the identity moves; 2. the effect re-runs, because it depends on the epoch it reads; 3. it `clearTimeout`s the armed debounce and arms a new one — still reading `searchQuery`, which holds the PREVIOUS user's typed text; 4. it captures `epochAtSchedule` as the NEW epoch; 5. 200ms later the timer fires, its fence passes because the epoch it captured is current, and the previous session's query goes to the server under the new identity with its results rendered. The fence never failed and was never missing. It was RE-CREATED by the framework at the one moment it needed to stay put. That is a third face of this unit's problem: #1372 never re-stamped, #1375 re-stamped too early, and this re-stamps automatically at exactly the wrong time. Why every instrument on this page missed it ---------------------------------------------------------------------- The behavioural suite faked `authStore` with `let epoch = 0` — a plain closure variable. That models the store's VALUES correctly and its REACTIVITY not at all, so every "did this effect re-run?" question in the suite was silently answering "does this compile?". Ten source assertions, ten behavioural legs, a 10/10 mutation matrix, four codex rounds and two lead reads passed over it. Backing the fake with a real `$state` signal turned an EXISTING leg red on the first run — "the debounced body search does not fire after an identity move" — which is how the defect was found at all. The general form, now in the shared core's header: A test double that models a dependency's VALUES but not its REACTIVITY turns every question about re-running into a question about compiling. Where the subject is a framework effect, the double's reactivity IS the thing under test. What changed ---------------------------------------------------------------------- - The search dispatch untracks its epoch read, keeping "the epoch when this query was scheduled" and losing the re-arm. - `web/src/test/identityEpochMock.svelte.ts` is the family's reactive epoch double, with `isEpochReactive()` so a suite that forgets to bind fails loudly instead of measuring nothing. The collection, roles and settings suites are migrated and each carries a PRECONDITION leg asserting the epoch is reactive. Only collection went red; roles and settings are clean under the faithful mock. - The identity-change listener clears per-session state — the previous user's typed input, open dialogs and navigation intent — enumerated against all 49 of the page's `$state` declarations with every non-member named and justified. `searchResultRank` is the sharp one: it holds ids ranked by a query the PREVIOUS identity ran, and once the search effect stopped re-running on an identity change, nothing else cleared it. - `syncBodyOnly()` joins the shared core: "is this read tracked?" is a question about synchronous execution, not about text. A read inside a nested callback — the SSE subscription's `pageIdentityHeld()` — creates no dependency, and a text-level rule flags it, hiding the one site that matters among false positives. - `effectBlocks()` joins the core as the population no other enumerator reached. - The source guard forbids an undispositioned tracked epoch read inside an `$effect`, on this page, the roles board and the library page. A DISPOSITION TABLE, not a ban, and that is a correction ---------------------------------------------------------------------- The instrument found three tracked reads across the four fenced pages, and two of them WANT the dependency: - collection search dispatch — the defect. Untracked. - collection bootstrap/deltaSync — intended. It reads `authStore.userId` synchronously and on purpose, so it already re-runs on an identity change; untracking the epoch would change nothing and misdescribe it. - roles laneData sync — intended. `identityEpochAtLoad` was made `$state` in #1375 precisely so that effect re-evaluates. So each tracked read must be untracked OR named with its reason, and an undispositioned one fails. Known consequence ---------------------------------------------------------------------- `searchQuery` is synced to the URL, so clearing it rewrites the address on an identity change. Deliberate — the previous session's query sitting in the next user's address bar is the same leak one hop later — and stated in the code rather than left to be discovered. Evidence ---------------------------------------------------------------------- - Mutation matrix 4/4 detected, positive control green. C4 (killing the family mock's reactivity) turns 20 legs red across three suites, which is the direct measure of how much was resting on it. - The search-text clear is caught by BOTH a source assertion and a driven leg, verified by re-running its mutant. - `npx vitest run` from `web/`: 226 files / 3123 tests, 0 failures, on the tree rebased onto `d2ceab61`. - `npx svelte-check --threshold error`: 0 errors. Codex round 1 ---------------------------------------------------------------------- Seven findings, one P1, all fixed — and FOUR of them were my own instruments failing open, which is the part worth reading. - **P1: the reload restored the query from the URL.** Clearing `searchQuery` did not clear `?q=`, and `loadUrlFilters()` — which the identity reload calls on success — put it straight back, after which it dispatched under the new epoch. Codex reproduced `api.search('secret', …)` completing after the reload. Fixed with TWO mechanisms, because they fail differently: a latch consumed by the next `loadUrlFilters()` (deterministic), and the URL rewrite (best-effort — it races the page's own URL-sync effect, which is exactly why the latch exists). A leg suppressing the rewrite isolates the latch, because with both in place the rewrite always wins in-harness and a mutant deleting the latch SURVIVED. My code comment had claimed the address was rewritten before any of this was true. It is true now. - **P2: `isEpochReactive()` never touched the mock.** It read and wrote its own module signal, so it returned true with both hooks disconnected and all four PRECONDITION legs passed against exactly the non-reactive double they exist to detect. A guard that cannot fail for the case it is named after IS that case. It now takes the mock's own getter and bump, and removing a suite's binding turns it red. - **P2: `syncBodyOnly()` hid real dependencies.** Its premise — a read inside a nested function is not tracked — is false: an IIFE, a `forEach` callback, a Promise executor and everything before an async function's first await all run synchronously, and Svelte tracks through ordinary calls. DELETED. What is genuinely deferred (a subscription callback, a timer) is now NAMED in each page's disposition table, where the claim can be read and argued instead of buried in a regex. - **P2: any `untrack(` anywhere exempted a whole effect.** So `const e = captureIdentity(); untrack(() => unrelated());` passed with the capture tracked. Replaced by `untrackedSpans()`, which exempts a read only if that read is lexically inside an untrack call. - **P2: `effectBlocks()` took the first `{` after the call**, so an expression-bodied arrow returned a nested timer's empty body and the rule passed on a tracked capture. It now REFUSES any form but a brace-bodied arrow. A wrong span is a silent pass; a refusal is red. - **P2: busy flags released by stale continuations.** `creatingNew`, `savingView` and `savingDrafts` were cleared unconditionally on the grounds that a stuck flag disables its affordance for ever — true until `resetPerSessionState()` started clearing them, at which point the same release lets a previous-identity continuation open a gate the NEW user's in-flight operation is holding. Same reversal the roles board made in its round 4. One of them sat inside a lost-identity exit and now writes nothing at all. - **P3: an exemption citing a reset that does not happen.** `defaultViewApplied` was exempted as "reset by the load it gates"; `loadCollection` never resets it, so a same-route identity reload kept the previous session's completed gate and skipped the new user's default view. It is reset now and the false justification is gone. Mutation matrix after the round: U2, U3, U4, U5 and the earlier C1-C4 detected, positive controls green. U1 (making `isEpochReactive` read its own signal again) SURVIVES and is recorded as UNFAITHFUL rather than as a hole: with the binding in place both paths agree, so the mutant does not create the condition the guard is for. The control that does is removing a suite's `bindReactiveEpoch` call, which turns that suite's PRECONDITION red — run and confirmed. Full suite after that round: 226 files / 3131 tests, 0 failures. Codex round 2 ---------------------------------------------------------------------- No P1. Five P2 and a P3, all fixed. The important one is that round 1's own fix had made the guard unable to catch this PR's defect. - **The disposition table exempted the defect itself.** Entries keyed on an effect skipped the WHOLE effect, so the search entry — written for the reads inside its 200ms timer — also covered the synchronous capture that was the bug. Codex removed that capture's `untrack` and the rule reported ZERO offenders. Entries now name WHICH READ TOKENS they vouch for, and the search entry deliberately does not include `captureIdentity(`. Verified: restoring the defect now turns the guard red as well as the behavioural leg. - **The SSE entry allows every read token**, which is only defensible while that effect reads nothing before it subscribes. That is now asserted directly rather than assumed. - **`untrackedSpans` exempted argument evaluation.** `untrack((captureIdentity(), () => {}))` runs the capture BEFORE untrack is called, so the read is tracked; a compiled probe confirmed the effect re-runs. The exempt span now starts at the arrow, and a call whose argument is not a bare `() => …` is refused. - **`effectBlocks` still mis-delimited** a parameter-default arrow (`$effect((unused = () => {}) => …)`), returning an empty body and reporting nothing. The argument must now be a bare `()` arrow. - **Whitespace bypassed detection** — `captureIdentity ()` and `authStore . identityEpoch` matched nothing. Both are unchanged runtime reads. - **The latch was scoped to "the next load".** A navigation the NEW user makes to another `?q=` link consumed it, discarding a query that was theirs. It now records the VALUE belonging to the session that ended, so only that query is suppressed. A leg drives exactly that sequence. - **The URL rewrite dropped the pane history state**, defaulting it to `{}` and losing `paneDepth` / `paneOwned`, after which closing a drilled pane took the replace-delete path and Back could reopen earlier pane entries. `updateUrlFilters` preserves it; this rewrite does now too. Full suite after that round: 226 files / 3136 tests, 0 failures. Codex round 3 ---------------------------------------------------------------------- **No P1 or P2 in the page code.** Both findings are in the guards, and both are constructions that defeated them — reproduced, then closed, then re-run as controls. - **The timer exemption was token-scoped but not POSITIONAL.** It covered every `identityHeld(` in the search effect regardless of where it sat, so adding one synchronously beside the untracked capture put the dependency straight back while the rule reported nothing. Entries now carry an `afterMarker`, and a read before it is never exempt. - **`matchBrace` did not understand regex literals.** `const p = /}/;` inside an effect closed the body early and every read after it vanished — silently, with the effect still enumerated. A page carrying `/}/` before its search logic kept all 17 effects and produced zero offenders with the defect restored. The scanner already tracked strings and template interpolations; it now tracks regex literals too, HANDLED rather than refused, because the codebase uses them and a guard must cover the grammar actually in use. My first attempt refused instead and false-positived on a `/` inside a template literal — which is why the detection belongs in the scanner that knows lexical context, not in a regex over the body. Both controls now fail the guard: a synchronous `identityHeld()` beside the capture, and a regex literal plus the defect restored. Full suite: 226 files / 3138 tests, 0 failures; svelte-check 0 errors. Claude-Session: https://claude.ai/code/session_01PDYjP67KTAcqpFpo3ijAAA |
||
|
xarmian
|
d2ceab6120 |
fix(web): fence every async commit point on the library page on the signed-in identity (BUG-3084) (#1378)
Surface 3 of 7. Three async handlers and FOUR DEFERRED TIMERS — a population of 7, not the 3 a function-level count sees. The originally filed survey reported 3; the widened instrument's number is confirmed again here on the current tree rather than carried from the survey. What this fixes ---------------------------------------------------------------------- `activateConvention` and `activatePlaybook` COMPOSE A WRITE from state chosen under the previous identity: the entry the user clicked came from a list `loadData` fetched then. A click after an identity change is the current epoch, so every entry capture passes and the previous session's choice is activated into whatever workspace is on screen now. Both now take `pageIdentityHeld()` at entry, the same question the roles board's drag handlers needed. The four timers are the distinctive class. Each fires three seconds after its handler returned and clears `toast` — by which time the page may belong to someone else and be showing THEIR toast. Clearing it then is a commit by a session that has ended. Driven with fake timers rather than reasoned about. `loadData` had no identity bookkeeping at all, and its `$effect` is keyed on `wsSlug` alone, so nothing re-ran it when the identity moved. It now has an entry capture, per-arm fences, a generation guard, and an `onIdentityChange` subscription that resets transient state before re-loading. The rules carried from surfaces 1 and 2, not re-derived ---------------------------------------------------------------------- - The epoch is re-stamped AFTER the awaited state is assigned, never before the await, and on BOTH arms. Too early and the guard vouches for data it has not replaced; missing on a path and a transient error pins the fence shut for ever. Bounded on both sides in the guard. - A continuation that has LOST the identity writes no shared interaction state — `resetTransientState()` owns that reset, and a stale continuation writing it again can only clobber what the NEW user is doing. `activatingTitle` gates both handlers at their first line, so this is not cosmetic: a stale release permits a second concurrent activation. - `loadGen` gates the state assignments, not merely `loading`. Different question from the identity fence and neither implies the other — both loads can belong to the same user, which is what a workspace switch produces here. - The transient-state reset is ENUMERATED against the file's own `$state` declarations, with every non-member named and justified, so a new piece of interaction state fails the guard rather than waiting for a reviewer (CONVE-35). Evidence ---------------------------------------------------------------------- - Mutation matrix 8/8 detected, positive control green. Four of the eight are caught by a BEHAVIOURAL leg independently of the source guard, so the semantics are measured and not merely the syntax. - One mutant (L6, a stale `finally` releasing the activate gate) survived its first instrument. The instrument was fixed rather than the mutant dropped — the continuation rule cannot govern a `finally`, because the success-path check always sits in its window, so the two arms are named explicitly instead of pretending a regex expresses "which check governs which statement". - `npx vitest run` from `web/`: 226 files / 3104 tests, 0 failures. - `npx svelte-check --threshold error`: 0 errors. Codex round 1 ---------------------------------------------------------------------- No P1. Three P2, all taken. - **Double loads on every identity change.** `loadData` reads `authStore.identityEpoch` synchronously via `captureIdentity()` before its first await, so the `$effect` took a dependency on the epoch and re-ran on an identity change — on top of the listener that already re-loads for that event. Eight requests where four were wanted, with `loadGen` quietly discarding half: correct on screen, a mystery in a network log. Untracked, naming the workspace as the real key, the way settings (#1370) suppresses the same class. **This is a class, and the merged collection page has it too** (#1374's listener plus a `$effect` calling `loadCollection`, which reads the epoch the same way). Out of scope here — merged code, a different surface — and reported to the lead as its own unit rather than widened into this PR. - **A fenced timer that clears nothing passed every test.** The stale-timer leg proves the toast SURVIVES a timer armed by the previous session; it says nothing about the timer doing its job when the identity is unchanged, and the source guard only looked for `identityHeld(` in the callback. Fixing a stale-write by making the write never happen is the failure this family already shipped once, as a fence that latched shut (#1372). Added: a control leg that advances the clock under an unchanged identity and requires the toast to clear, and a guard assertion that every timer still assigns `toast = null`. - **`activatePlaybook` had no driven coverage** — every behavioural leg clicked the convention button, so the near-identical playbook handler was vouched for only by source text. It now has its own control and its own click-after-the-change leg. Mutation matrix: 11/11 detected at this round, both positive controls green. Codex round 2 ---------------------------------------------------------------------- No P1. Two P2, both the same rule as round 1's and both taken — the instrument could not tell a commit correctly REFUSED from a commit that never existed. - **The load path had no driven leg at all.** The source guard permits the fence to sit after the assignments, and nothing held a load open across an identity change to notice. The implementation ordering was right; the instrument could not say so. Added a leg that holds the reload open, moves the identity, then resolves it with different content and requires that content not to be painted. - **Two of the four timers live in CATCH arms, and nothing executed them.** A dead branch carrying both `identityHeld()` and `toast = null` satisfied the source guard and the whole suite — source text vouching for a path nothing runs. This round drove the CONVENTION failure arm and the playbook success arm, plus a stale-timer leg on the convention failure path. It did NOT reach the playbook failure arm; round 3 found that, and the wording here has been corrected rather than left reading as though the catch-arm class had been closed. Per the lead's ruling, the rule those three rounds keep re-teaching is now in the shared instrument's header (`identityFenceSource.ts`), so surfaces 4-7 meet it before writing their first assertion: every rule in that core asks a REFUSAL question, none can distinguish refusal from absence, and a surface therefore owes one case that keeps USING the page after an identity change, one unchanged-identity case per fenced commit proving it still happens, and a driven leg per handler rather than per handler SHAPE. Mutation matrix: 14/14 detected at this round, three positive controls green. Codex round 3 ---------------------------------------------------------------------- No P1, and no correctness defect in the page code at all — both findings are coverage, both on the playbook path, and one of them is that round 2's own write-up overclaimed. - `activatePlaybook`'s LATE-RESPONSE fence had no driven leg. Round 2 gave that handler a control and a click-after-the-change leg, which between them prove the request starts and that a preflight refusal works, and say nothing about the fence after the await. Removing that fence left the entire suite passing. - The playbook FAILURE timer — the fourth of four — still had no driven leg. Round 2 named "two of the four timers live in catch arms" as the problem and then drove one of them. The prose above is corrected in place; a write-up that describes a class and fixes half of it is how a reviewer stops being able to trust the next one. Both now have driven legs, and both mutants (dropping the post-await fence; unfencing the playbook failure timer) are detected. Round 3 also confirmed the load-window leg discriminates: it starts from a rendered baseline, holds a second load, supplies unique content, and moves the epoch before resolving, so without the fence that content would be painted. Mutation matrix: 17/17 detected, four positive controls green. Full suite: 226 files / 3114 tests, 0 failures; svelte-check 0 errors. Claude-Session: https://claude.ai/code/session_01PDYjP67KTAcqpFpo3ijAAA |
||
|
xarmian
|
cee0267456 |
fix(web): fence every async commit point on the roles board on the signed-in identity (BUG-3084) (#1375)
Surface 2 of 7. Six async handlers, all with awaits; no markup arrows, nested callbacks or timers — a count the widened instrument confirms. Ruled next by exposure rather than by count: handleDndFinalize does not merely report a stale result, it COMPOSES A WRITE from state captured under the previous identity. It sets update.assigned_user_id = currentUserId, and currentUserId is read from /auth/session by loadData, so after an identity change a drag persists the previous user's id as an assignee. The server cannot refuse it — assigning to another user is a legitimate operation. A wrong row, not a wrong screen. Both fence questions are needed here, and a behavioural leg proved it rather than the reasoning. An entry capture asks whether the identity moved since THIS work started; both drag handlers' problem is that their work starts correctly and reads state loaded EARLIER, so a drag begun after an identity change passes every entry capture and still writes the previous session's assignee. The re-stamp position is the substantive fix ---------------------------------------------------------------------- loadData used to re-stamp identityEpochAtLoad BEFORE its await. That makes pageIdentityHeld() answer TRUE for the whole reload round-trip while currentUserId and lanes are still the previous session's — the guard vouching for data it has not yet replaced. It is the mirror image of the regression #1374 repaired: there the reference was never re-stamped and the fence latched shut; here it was re-stamped too early and the fence opened onto stale state. One line, two failure modes. It now re-stamps after the awaited state is assigned, and on both arms, so a transient error cannot pin the page inert. Measured, not argued. The window is currently unreachable through the DOM because the markup's {#if loading} branch unmounts the lanes for the duration — so the surface was defended by a rendering detail rather than by its fence. Gating that skeleton on orderedLanes.length === 0 (the obvious "don't blank the board on refresh" change) made the drag go through and persist u1. The source guard's own assertion had encoded the defective position as the contract, requiring the re-stamp before the first await; it is corrected and bounded on both sides. Reconciling with #1374, on the property rather than the code ---------------------------------------------------------------------- #1374 clears surface 1's rendered state before re-loading; this branch re-stamps late instead. Both hold one property: a recovery must never leave a fence vouching for state it has not replaced. The guard asserts that property as a disjunction, so a later surface may hold it either way and neither-way fails. Clearing on top of the late re-stamp was tried and removed: mutants that dropped `currentUserId = null` and `lanes = []` SURVIVED the whole suite, which is evidence those lines were inert here, not a gap in the instruments. Swept the rest of the family (CONVE-18): the collection page (#1374) and the settings page (#1370) both re-stamp early too, and both are defended because their identity-change path clears the state first — settings via the load effect's untrack block, the collection page via #1374's listener. No live defect on main. Also here ---------------------------------------------------------------------- - withoutCatchArms joins the shared core, and surface 1's guard is back-ported to it so the family does not carry two answers to one question. The narrower window shipped in #1372 reports correct code as unguarded whenever a handler commits AFTER its try/catch rather than inside it, which handleLaneDrop does. - The laneData sync effect is fenced, which is what stops a fenced exit pinning the board on stale lane data. Round 1 solved that by releasing isDragging at every fenced exit; round 4 reversed it — see below. - A "did it ever resume?" leg, the instrument this family was missing: move the identity, let the page recover, then keep using it and assert the write lands under the NEW user. Every other leg asks "did it refuse?", and an instrument set that only removes protection can only measure under-protection. - The NO_ENTRY_CAPTURE exemption list is now empty. loadData was exempted as "the writer of the value others read" — true, and it never asked WHEN it writes, which is where this bug lived. Evidence ---------------------------------------------------------------------- - Against the page on main: 9 of 9 guard assertions or behavioural legs fail, all 3 controls pass. - Mutation matrix 3/3 detected on this round's additions, positive control green; 8/8 on the earlier round. - npx vitest run from web/: 224 files / 3072 tests, 0 failures. - npx svelte-check --threshold error: 0 errors. Codex round 2 ---------------------------------------------------------------------- Five findings, all fixed here, each with an instrument that goes red without the fix: - Three more handlers write state the PREVIOUS identity chose and carried only an entry capture, which a click after the change always passes: saveRole and deleteRole write editingRoleId (taken from the previous board), and submitNewItem creates into a collection chosen from the previous list. The modals are mounted outside the {#if loading} branch, so unlike the board they stay usable for the whole reload. All three now take pageIdentityHeld() at entry, and the identity-change listener closes both modals — a fence stops the write, it does not stop the reading. - No generation guard on `loading`: an older loadData's unconditional finally cleared it while a newer reload was in flight, repainting the board over the previous identity's lanes. Added, the way settings does it. - The laneData sync effect ran regardless of whether the page still held the identity, so the optimistic lanes handleDndFinalize writes could be painted after an identity change during its failure path's `await loadData()`. Fenced there rather than by making the `isDragging` release conditional — the release must stay unconditional or the board pins for ever. - The error arm re-stamped over state nothing had replaced. Its comment claimed the listener had cleared it, which was true when written and false two commits later when those clears were removed — authored and falsified inside one unit. It now clears the state itself and says why the re-stamp is still owed. - The recovery-property guard was satisfiable by the defect: it proved only that no re-stamp precedes the first await, not that it follows the state it vouches for. Strengthened. Mutation matrix: 10/10 detected on these two rounds. Codex round 3 ---------------------------------------------------------------------- One P1, three P2, one P3 — and all five are one class: transient interaction state that survives an identity change. Rounds 2 and 3 each returned members of it one at a time, which is CONVE-35's signal that the loop was measuring the enumeration rather than the code. So it is enumerated: resetTransientState() is called from the identity listener, and the source guard holds that function against the file's own $state declarations, with every non-member named and justified. A new piece of interaction state now fails that guard rather than waiting for a reviewer. The P1 is the sharp one and is #1372's failure mode in a new variable: `isDragging` disables the $effect that repopulates `laneData`, and the {#if loading} branch destroys the dnd zones without dispatching `finalize` — so a drag in progress when the identity moves left it true with no handler remaining to clear it, and the board never repopulated. A behavioural leg drives exactly that: start a drag, move the identity, let the reload land, then keep using the board. Also: an unauthenticated load left the previous user id in place and then re-stamped the fence over it; the recovery-property guard's clear-first branch checked only currentUserId and not lanes. Mutation matrix: 15/15 detected across the first three rounds. Codex round 4 ---------------------------------------------------------------------- No P1. Two P2 and a P3, all taken. The interesting one is a REVERSAL of round 1's fix. Round 1 required every fenced exit in handleDndFinalize to release `isDragging`, because nothing else would and the board would pin. Round 3 gave that reset to the identity listener, which runs synchronously on the epoch bump and therefore always before a stale continuation resumes — at which point the same release became a way for a previous-identity continuation to cancel a drag the NEW user had started. The rule is now: a continuation that has lost the identity writes no shared interaction state at all, because someone else owns it. A guard enumerates every lost-identity exit and the shared state it may not touch, and it immediately found one exit I had missed, still carrying round 1's comment. The same ownership problem applied to submitNewItem's `finally`, which now clears `newItemSaving` only under a held identity — a deliberate asymmetry with loadData's `loading`, which stays unconditional because nothing else clears it and pinning the board at the skeleton is the worse failure. That asymmetry is stated in both places. `loadGen` also now gates loadData's state assignments, not just `loading`. Gating only the busy flag reads as though the overlapping-load race were handled while a slow first load could still overwrite a newer one — and the identity fence does not cover it, since both loads can belong to the same user. Mutation matrix: 19/19 detected across the first four rounds. One mutant (M22) survived its first instrument and the instrument was fixed rather than the mutant dropped. Codex round 5 ---------------------------------------------------------------------- No P1. Two P2, both taken, and the round confirmed the two things it was asked to confirm: the auth store bumps the epoch and notifies listeners synchronously, so resetTransientState() does always run before a stale continuation resumes (the round-4 reversal rests on that); and the loading/newItemSaving asymmetry is right. - The reversal was incomplete. `handleDndFinalize`'s recovery path checks the identity, then awaits loadData(), then released `isDragging` unconditionally — a check BEFORE an await says nothing about a change DURING it. The source assertion could not see it because it only looked inside guard arms; it now also covers continuations, under the rule "checked since the last await" rather than "checked on the same line". The first spelling of that rule flagged two CORRECT writes in handleLaneDrop, which was the guard measuring its own formatting preference rather than the property. - The shared source core did not fail closed outside the formatting it expects. `withoutCatchArms` treated `}catch` as "no catch arm", so the success-path guard would go on to scan FAILURE code — measured, not argued: with the old delimiter, a saveRole whose only identity check sat in a `}catch` arm PASSED that assertion, and fails it now. The async-function enumerator likewise skipped declarations outside its tab-indented shape, which is invisible because a shorter enumeration makes every "these are missing a fence" list shorter; it now cross-checks against a loose count and refuses rather than under-report. Mutation matrix: 21/21 detected across five rounds (3 + 7 + 5 + 4 + 2), positive control green each time, plus a discriminating counterfactual for each fail-closed change. Full suite on the rebased tree: 224 files / 3083 tests, 0 failures; svelte-check 0 errors. Claude-Session: https://claude.ai/code/session_01PDYjP67KTAcqpFpo3ijAAA |
||
|
xarmian
|
aed1ebb101 |
fix(server): a workspace create answers with a usable workspace or with nothing (BUG-2715) (#1376)
Four doors created a workspace, added the creator as owner, and discarded the result. A failure left a workspace that exists, is seeded, and that NOBODY CAN ADMINISTER — returned to the caller as a 201. TASK-2658 made the error visible and left it non-fatal; this makes it fatal. FOUR DOORS, NOT THE TWO THE BUG NAMES. Its title says "ownerless workspace on create, consumed invitation on signup", but workspace import (handlers_workspaces.go) and bundle import (handlers_import_bundle.go) carry the identical pattern and both cite BUG-2715 in their own comments. A fix written to the title would have left two live instances of the same defect: an imported workspace nobody can administer is the same object as a created one. The three create-family doors share one helper. handlers_members.go's authenticated accept-invitation door and the cloud auto-create door were already fatal and are unchanged. RECONCILE BEFORE DESTROYING, which is why this is not "delete on error". AddWorkspaceMember returns the raw tx.Commit() error, so a commit whose acknowledgement was lost LANDS THE ROW and reports failure — deleting there destroys a workspace whose membership actually succeeded. That is BUG-3026, already paid for once on the cloud door, and the four arms here are its four arms: unreadable (keep, refuse), absent (delete, refuse), present-as-owner (reconcile to success), present-with-another-role (keep, refuse). What differs is the outcome, not the reasoning — that door reconciles to a successful auto-create, these must refuse. WHY A COMPENSATION RATHER THAN ONE TRANSACTION, measured rather than assumed. The ruling's first choice was create + seed + owner in a single transaction. SeedCollectionsFromTemplate issues no SQL of its own but orchestrates five public Store methods; none has an executor-taking variant — createWorkspaceQ is the ONLY execQueryer helper in the entire store — and two of them (CreateCollection, and CreateItem via tryCreateItem) open transactions of their own. That is a store-wide refactor of the two principal write paths, and it is filed separately with this measurement attached. The crash window that buys is stated in the code at the compensation: between the failed owner-add and the delete, this process can die, and the husk it leaves is exactly the state being prevented. Small, not zero, and only the transaction closes it. The compensating delete is soft, which is sufficient: uniqueWorkspaceSlug filters `deleted_at IS NULL`, so the slug is released and a retry reclaims the name rather than landing on `name-2`. THE SIGNUP DOOR is an ordering fix. The membership error was discarded and the invitation consumed on the next line, so a failure burned the code AND left the user with no access — unrecoverable, since an invitation cannot be redeemed twice. Membership first and fatal, accept second: a refusal now leaves the invitation OPEN and the user retries with the same code. handlers_members.go's sibling has always done exactly this. Its second write stays non-fatal here, a deliberate divergence stated in the code: there the caller is an existing authenticated user and a 500 is cleanly retryable, whereas here the account was created by this same request and refusing would report failure for a registration that succeeded, with the retry landing on duplicate-email. THE MaxOpenConns(1) CLAIM IS SETTLED RATHER THAN DEFERRED. BUG-2778/BUG-2409 were being carried as a hazard attaching to this change; measured, the compensation's calls are sequential — no read is issued while a transaction on this path holds a connection — and the door completes under a single connection. The leg proving it is shown able to go RED: issuing the membership check on a held connection hangs it to the timeout. CODEX ROUND 1 closed five findings, three of which were mine being wrong in a comment rather than in code — the failure mode where a false explanation forecloses the question for the next reader: - The deleted-workspaces list is scoped by `workspaces.owner_id`, NOT by membership as I had written, so a soft delete alone would surface the husk in the creator's restorable list and restoring it would hand back the ownerless workspace. The removal is now soft-delete then PurgeWorkspaceData, which also takes the workspace ID rather than the mutable slug. - The signup refusal claimed the user could "retry with the same code". With the account persisted that was false — the retry meets duplicate-email. The account is now rolled back too, following the verification-token path's existing precedent directly above it. - Absence of THIS caller's owner row is not absence of every member. The ABSENT arm now proves the workspace has no other members before removing it, and keeps it when the member list cannot be read. Nine mutants, each killed by its own legs, including the deadlock negative control; two that would not compile were reported as non-verdicts and re-run in a form that builds. Gates: make test exit 0, make test-pg exit 0 (the ack-loss shape is a Postgres failure mode, so SQLite-green alone would not have been evidence), make lint 0 issues, gofmt clean, go vet clean. Claude-Session: https://claude.ai/code/session_019EmDd3Wy8b3cPG1TZBSzBu |
||
|
xarmian
|
fb7e39e85c |
fix(web): the collection page's identity fence must be able to recover (BUG-3084) (#1374)
Repair for #1372, found by codex on the next surface in the family. `pageIdentityHeld()` compares against `identityEpochAtLoad`, and only `loadCollection` re-stamps that value. The load runs from an effect keyed on the ROUTE — `wsSlug` / `collSlug` / `showArchived` — and never because the identity moved. `routes/+layout.svelte` deliberately does NOT reload the tab on an anonymous -> signed-in transition, so the page stays mounted with a stale epoch and every `pageIdentityHeld()` is false FOR EVER. The consequence is silent and permanent: the SSE and sync subscription callbacks on the collection page return at their first line, so the board stops receiving live updates and stops healing renames, with nothing on screen to say so. An anonymous viewer on a public collection who signs in is enough to reach it. The fix is that the stale-data condition the fence DETECTS has to be RESOLVED, not refused indefinitely. The page subscribes to `authStore.onIdentityChange` and re-runs its load, which re-stamps the epoch and replaces the previous session's view with this one's — the state the fence existed to stop being shown in the first place. The order is the store's contract rather than luck: the epoch is bumped BEFORE listeners run, and `notifyIdentityChange`'s own comment places the obligation on a listener to load for the identity signed in NOW rather than re-issue anything captured earlier. This listener reads the LIVE route values and captures nothing. The settings page (#1370) does NOT need this and was checked rather than assumed: its load effect is keyed on `${sessionUserId}\n${wsSlug}`, so an identity change re-runs it by itself. Two further defects found in the same review, both fixed here because both are in the code this change is about: - The listener CLEARS before it re-loads. `loadCollection` leaves the current collection, saved views, members and progress on screen for the whole round-trip, so re-loading alone would keep the previous user's private collection visible across a sign-out. The fences stop stale async COMMITS; they say nothing about state already rendered. `items` is not cleared here — it derives from `localIndex`, whose own identity listener drops the previous user's rows. - `leaveDiscard` replays `pendingNav` without an identity check, so a draft dialog answered after a sign-in navigates the new user to the previous user's destination. It uses `pageIdentityHeld()` rather than an entry capture, because the intent was formed when the previous user tried to leave and a capture taken at the dialog's answer is already the new epoch. The drafts are still discarded — they are the previous session's and clearing them is the safe direction — only the navigation is withheld. This is a commit point with NO AWAIT, which is a population neither the function-level enumeration nor the await-counting rules can reach. BUG-3084's body predicted this shape exists and said it would be found by reading; it was found by a reviewer instead, which is the same thing arriving later. It is asserted by name rather than enumerated, because "every synchronous function that replays captured intent" is not a set a grep can produce, and a rule that cannot enumerate its population should not pretend to. Instruments, each shown able to go red for the case it is named for: - The source guard gains three assertions — the subscription exists, it re-runs the load (a listener that merely cleared state would leave the epoch stale, which is the defect wearing a different shape), and the clear precedes the load. Surfaces 3-7 copy this guard, so the property is asserted where it will be inherited. - The behavioural suite gains a recovery case that drives the real sequence — bump the epoch, THEN notify, which is the order the store uses — and asserts the page re-loads, that the previous collection is not still rendered while it does, and that an SSE event afterwards is acted on again. Its fake auth store now dispatches listeners, since what a listener does is the whole subject. Against the page as it stands on main, exactly the two new tests fail and the other 21 pass. Each of the three fixes was mutated individually: all three detected (2, 1 and 3 failures respectively). Gates: `npx vitest run` from web/ — 221 files / 3035 tests, 0 failures; `npx svelte-check --threshold error` — 0 errors. Codex rounds 1-3: two P1s, CLEAN, CLEAN. Claude-Session: https://claude.ai/code/session_01PDYjP67KTAcqpFpo3ijAAA |
||
|
xarmian
|
4e81480d0a |
fix(editor): block-drag global listeners defer to a frontmost viewer (BUG-2453) (#1373)
`block-drag-handle.ts` installs four WINDOW-level listeners and none of them asked whether anything was in front of the editor. Background inertness stops NEW interaction with the editor; it does not stop a gesture that already owns the pointer. So a block drag in flight when the attachment viewer opened kept running against an editor that was `inert` and invisible behind the viewer's backdrop — and the drop still landed. This is the deferred half of TASK-2430, which taught seven global owners to stand down and closed with "Deferred: svelte-dnd-action + editor block-drag global gestures." FOUR OWNERS, NOT THE TWO THE BUG NAMES. It names the mouse pair; `touchmove` and `touchend` are installed on the same line with the identical hole, so a mouse-only fix would have left every touch device unguarded. Each handler consults the shared arbitration helper — `isBlockedByModal`, never a raw lease query — with `editorView.dom` as the owner, the surface asking to act rather than `event.target`, which during a straddling gesture is whatever the pointer happens to be over. ABORT, NOT EARLY-RETURN, the choice `ItemGraph.abortPan` already makes for its captured pan. An early return leaves `dragging` latched, the block at 20% opacity, the ghost under the cursor and `pointerEvents: none` on the editor — a half-drag frozen onto a surface that is now behind a viewer, with nothing left to deliver the event that would end it. The teardown reuses `cancelDrag`, which already existed and had NO callers; unlike `endDrag` it never calls `executeMove`, so aborting cannot commit the reorder. The gates sit above the branch structure, not inside the `dragging` arm: `mouseup` and `touchend` each also open the block menu on a press that never moved, and a guard placed in the drag arm would still pop a menu onto the inert editor beneath the viewer. `isBlockedByModal` returns false on an empty lease stack, so with no viewer open every path is untouched. Each of the four owners has an inert-while-frontmost leg and the contract's empty-stack regression, and the empty-stack legs assert what ships TODAY — the trap TASK-2430 shipped and reverted twice, where a test labelled a regression actually specified a change. Codex round 2 closed two more, both in the abort's own teardown: `abortDrag` left `activeBlock` populated where `endDrag` nulls it, and the comment beside it claimed to clear two pending flags when `cancelDrag` already owned one of them. The first is consistency rather than a defect closed — no path was found that acts on a stale `activeBlock` — and the comment now says so instead of implying a bug was fixed. Six mutants, each killed by its own legs: removing any of the four gates; replacing the abort with a bare early return; and pointing the owner at a viewer-contained node. The reorder itself is proven suppressed, on both `mouseup` and `touchend`, by legs that drag the SECOND block. That detail is load-bearing: jsdom gives every element a zero `getBoundingClientRect`, so `dropPosAtY` scores all candidates alike and keeps the first — for a drag of block 1 that is the slot the node is already in, so the drop runs and changes nothing and a doc-equality assertion passes either way. An earlier revision of this change asserted exactly that and was green with the gate deleted. Dragging block 2 makes the first candidate position 0, the paragraphs swap, and the assertion has something to see; each reorder leg is paired with an empty-stack leg proving the drag still reorders with no viewer, so a future harness break cannot turn it back into a vacuous pass. Claude-Session: https://claude.ai/code/session_019EmDd3Wy8b3cPG1TZBSzBu |
||
|
xarmian
|
7fef073326 |
fix(web): fence every async commit point on the collection page on the signed-in identity (BUG-3084) (#1372)
Surface 1 of the seven BUG-3006 (#1370) named and did not survey. The collection page has twenty-one async commit points and mentions the identity epoch nowhere, so after an identity change an in-flight or still-clickable one can report the previous session's result to whoever is signed in now, write it into the page, write it into a shared store, or navigate them into it. WHY IT NEEDS A FENCE AT ALL, since routes/+layout.svelte reloads the tab on an identity change: that reload is CONDITIONAL, and this page sits in every window it leaves open. - `'' -> user` (sign-IN from anonymous) returns early with no reload and no clear, so an in-flight handler runs to completion and commits under the new identity. Nothing else covers this transition at all. - `user -> ''` (sign-out) clears persistent state but does not reload, because a reload aborts the sign-out sites' own navigation. - `user -> user` (swap) reloads, leaving the pre-reload window the workspace layout's own SSE-disconnect comment already names. auth.svelte.ts's comment claiming the workspace layout keys its leaf-page block on the epoch (`{#key authStore.identityEpoch}`) to remount route components describes a design BUG-3005 (#1325) replaced with that reload; no such key block exists in the tree. Corrected in place, because a fence added here while that comment still says route components are re-created would read as redundant to the next person. THE POPULATION IS 21, NOT THE 15 THE ITEM'S SURVEY IMPLIES. That survey counted top-level `async function` declarations and inline markup arrows, which cannot see async callbacks passed to subscriptions and timers. This page has four — an `$effect` IIFE, the SSE subscription, the sync subscription, and the debounced body search — plus two deferred timers. Every one of the 15 declarations has an await and commits after it. TWO CAPTURES, on #1370's grounds. `identityHeld(captured)` asks whether the identity moved since THIS WORK started, captured at each handler's ENTRY; comparing against the page's load epoch instead is defeated by a concurrent load re-stamping it. `pageIdentityHeld()` asks whether the PAGE still belongs to the signed-in user, which is the only question that works for the two subscription callbacks: they arrive when the server says so, which in the case that matters is after the epoch already moved, so a capture taken at their entry is the new value and detects nothing. Neither replaces the route guards and they do not replace it. `loadSeq` / `collectionGen` / `renameNav` / the `ws !== wsSlug` snapshots ask WHICH ROUTE an answer describes; `localIndex.upsert`'s `sinceEpoch` / `fencedIds` / `movedOutFloor` refusals ask which PROJECTION SCOPE a row belongs to, and that store's own comment says all three move only on this tab's resync. The worst case is the bulk Undo toast. `runBulkOn` hands its `onAction` to the global toast store and the archive and move flows re-enter `runBulkOn` from it, so it deliberately outlives the page: B could click A's Undo and re-archive or move A's items, and be shown the count. It captures at ISSUE time and compares at CLICK time, #1370's delete-workspace shape. Three writes are deliberately NOT fenced, each with its reason in the code: `metaLoading = false` in loadCollection's `finally` (must run on every exit path or the spinner is permanent on a page that did not remount, and it discloses nothing), and the `creatingNew` / `savingView` busy flags. A `throw` re-raised for BoardView's optimistic unwind stays unfenced while its TOAST is fenced — the unwind must happen for either user, the message must not. The zero-delay `bypassNavGuard` timer IS fenced: "0ms is too short for an identity to move" is a claim about a race supported only by the window looking small. TWO INSTRUMENTS, and they are not redundant. - `collectionIdentityFence.test.ts` owns the POPULATION, built on a new shared core (`src/test/identityFenceSource.ts`) whose only parameter is the file path, since six more surfaces follow. It enumerates all four populations, requires an entry capture before each first await, at least one check per await, a check on the SUCCESS path specifically, and that no handler reads the re-stamped page epoch. An unrecognised handler FAILS rather than being skipped. Against the page on main, 7 of its 9 assertions fail. - `collectionIdentityFence.svelte.test.ts` owns the SEMANTICS. It mounts the page, holds a request open, moves the identity, resolves, and asserts no commit lands — four commit points, one per timing class, plus the pageIdentityHeld case. Against the page on main all five identity legs fail and all four controls pass, which is the discrimination that makes the legs mean anything. THE GUARD CORE FOUND A DEFECT IN ITSELF, and how it surfaced is the reason the core has its own suite. `runBulkOn`'s signature carries a braced parameter type (`opts?: { undo?: (ids) => void }`), and delimiting a declaration's body as "the first `{` after the keyword" lands on that type — so the handler carrying the worst commit point on the page was measured as a thirty-character body with no awaits, and every assertion about it passed vacuously. It went red only because one assertion looked for a string living past the type. `delimitDeclaration` takes the last `{` on the line closing the parameter list and THROWS otherwise, since a guard that hunts for a plausible brace always finds one. The core's matcher is string- and template-aware for the same reason, and its interpolation counter is kept separate from the pair being matched — folding them together closes a paren match early, silently. Both are driven by the core's suite rather than asserted in a comment. Mutation matrix: 9/9 detected, with a positive control that scores a zero-test run as INSTRUMENT-FAIL rather than as survival. Two mutants survived the first pass, both holes in the tests rather than in the fix — a deleted success-path check hiding behind a catch-arm check (closed by the success-path assertion), and the deleted `pageIdentityHeld()` still being caught by the entry capture in the only SSE leg that existed (closed by a leg where the identity moves BEFORE the event arrives). Gates: `npx vitest run` from web/ — 220 files / 3026 tests, 0 failures; `npx svelte-check --threshold error` — 0 errors. Built (`npx vite build` + `make build-go`) and run against that binary: bug-2601-sse-rename-retag, pane-async-race (9 cases) and bug-2602-collection-adopt-race, 11 passed. `pane-follow-live-list.spec.ts` fails on this machine — 3/3 with this change and 4/4 with this file reverted to origin/main, bit-identical — so it is not this branch's; filed as BUG-3085. Claude-Session: https://claude.ai/code/session_01PDYjP67KTAcqpFpo3ijAAA |
||
|
xarmian
|
25c6681009 |
fix(web): reconcile a list response that predates a workspace create (BUG-2981) (#1371)
`workspaceStore.loadAll` commits its list wholesale and `create` appends, and the two fence themselves independently and not against each other. `loadAll`'s `isLatest()` orders list calls against other LIST calls; `create`'s membership token orders selection against navigation. Neither pair spans this one. So a list request issued BEFORE a create legitimately answers without the new workspace, and committing that answer after the append erased it — with no error anywhere, until the next full reload. `current` is set separately and survives, which is what makes the symptom quiet: the workspace is open and usable while absent from the switcher and the sidebar list. Pre-existing rather than introduced by BUG-2978, verified against origin/main: `create` there does the same wholesale append into the same array. RECONCILE RATHER THAN DISCARD. The older response is not stale in the ordering sense — the server is right about what existed when it answered — so the newer half is the local create, not the whole list. `loadAll` captures a create counter before issuing, and on commit re-appends any create newer than that capture which the response does not already contain (matched on id, so a create the server DOES report is not duplicated). Discarding the response instead would throw away every other change it carried and would leave `recoverIfMissing`, which joins a run to populate an empty list, with nothing. Termination is the `seq > createSeqAtStart` half: an entry survives only a response ISSUED before the create completed. Any later response is authoritative about it, so a workspace since deleted or hidden is dropped rather than re-appended forever. THE CLASS, asked before choosing. `workspaces` has exactly three writers — this append, `loadAll`'s commit, and the identity reset's clear — and `create` is the only path that mutates the list locally. Import, accept-invitation, reorder, delete and restore all hand the change to the server and re-read: import and reorder await `loadAll()`, the other three navigate. None can be answered by an older request, because `createKeyedSingleFlight.run` ALWAYS issues and never joins, so their re-read is necessarily newer than the mutation it follows. A one-member class, for a structural reason rather than by luck. Four legs, driven by hand and released in the losing order. The first fails against the unfixed store (reports `['old']`); the other three are the fix's own failure modes, which the first cannot see — duplicating a workspace the server did report, resurrecting one created and then deleted, and carrying one user's create into the next user's list. Mutation matrix: removing the reconciliation, the id dedupe, or the termination rule each kills exactly its own leg. The reset's `pendingCreates = []` is REDUNDANT and now says so. Removing it fails no test: a run in flight at the reset cannot commit (`invalidate()` strips its `isLatest`), and any later run drops the entry by the termination rule. Kept because both of those live in other code that someone could change without reading this file, and the test leg's comment records that it pins the end-to-end property rather than that clear. Claude-Session: https://claude.ai/code/session_019EmDd3Wy8b3cPG1TZBSzBu |
||
|
xarmian
|
17acbe4cc1 |
fix(web): fence every settings-page commit point on the signed-in identity (BUG-3006) (#1370)
The workspace settings page fenced its LOAD on the signed-in user (BUG-2991)
and none of its MUTATIONS. Each handler is its own async function with its
own awaits and its own toast, and the load fence has no relationship to
them — so after a logout/login an in-flight or still-clickable one could
report the previous session's result to whoever is signed in now, commit it
into the settings UI, or act on intent captured for a different account.
The worst case is the delete-workspace Undo toast. It is handed to the
global toast store and `handleDeleteWorkspace` navigates to /console two
lines later, so it deliberately outlives the page: B could click A's Undo,
restore A's workspace if they happen to have rights there, and be navigated
to A's owner URL — and when the server refuses, B is still shown the
workspace's name.
TWO captures, because there are two questions and neither answers the other.
`identityHeld(captured)` asks "has the identity moved since THIS WORK
started?" Each handler captures at ENTRY, before its first await, and passes
that value in. An earlier version compared against the page's LOAD epoch,
which codex round 1 showed is wrong in a reachable way: an identity change
fires the page's own load effect, `load()` re-stamps that epoch to the new
value, and an older in-flight mutation then compared new against new and
committed. A handler's own entry is the only capture a concurrent load
cannot re-stamp underneath it.
`pageIdentityHeld()` asks "does this PAGE still belong to the signed-in
user?" For a control still clickable because it was rendered from the
previous session's data, the intent was formed at LOAD and the epoch has
already moved by the time the click arrives — so an entry capture is the
current value and detects nothing. The markup's copy-link button is that
shape, and is the only user of it.
Both are called through ONE named helper each rather than inline
comparisons, so a source guard can ENUMERATE the call sites and fail closed
on a handler it does not recognise.
The population is 14, not the six families the item names. Three timing
classes, and only the first is what an await-site fence obviously covers:
- single await then commit (most handlers);
- `handleInvite`, two awaits, checked after each — and one branch writes
the CLIPBOARD, a side effect that leaves the page entirely;
- `saveName` / `saveContext`, which schedule a 2s `setTimeout` that
writes page state AFTER the await returned. A check at the await says
nothing about who is signed in when the timer fires, so the timer
bodies carry their own.
The 14th is an inline async arrow in the MARKUP (the invitation copy-link
button), which no function-level enumeration can see. The instrument that
found it is described below.
Also fenced, each for a reason that is not obvious: `toggleAccessPanel`,
which is a READ whose commit paints one member's access rights into the
panel; `saveCollectionAccess`'s REVERT path, which writes values captured
before the await under the previous identity; and every FAILURE toast,
which is the half the server cannot bound — the request is refused for the
new user and the message reaches them anyway.
The Undo toast does NOT use the page helper, and that asymmetry is the
point: the callback outlives the page, so `identityHeld` is wrong for it
twice over — the variable it reads may belong to a dead page, and a later
load re-stamps it. It captures its own epoch at DELETE time and compares at
CLICK time, then re-checks after its own await, since the identity can move
between the click and the response.
Not changed this unit: the shared toast store, and the sibling routes codex
named as the search boundary (collection page, item pane, roles page).
Those are a follow-up carrying both instruments' counts, not a silent
widening.
Two instruments, and they are not redundant:
- `settingsIdentityFence.test.ts` owns the POPULATION. It enumerates the
async functions in the script and the inline async arrows in the
markup, and requires each to capture at entry, to check at least once
per await, and never to read the page load epoch — the last carrying
round 1's finding into the enumeration so the re-stamped value can
never be what a handler compares against. An unrecognised handler
FAILS rather than being skipped. Against the unfixed page all 7 of its
assertions fail.
Its first version asserted only that a fence was MENTIONED somewhere in
a body, which permits the failure it exists to prevent: an occurrence
after the write it was meant to guard satisfies it. Counting checks
against awaits is a lower bound, not a proof — it cannot see WHERE they
sit — and the behavioural suite is what establishes that.
- `settingsIdentityFence.svelte.test.ts` owns the SEMANTICS. It mounts
the page, holds a request open, moves the identity, then resolves, and
asserts no commit lands. Four commit points, one per timing class plus
the callback that outlives its page: member removal, the rename status
timer, the Undo toast, and the collection-access REVERT — the only
commit whose values are captured pre-await and written post-await.
Against the unfixed page its 4 identity legs fail and its 5 controls
pass, which is the discrimination that makes the legs mean anything.
The access legs needed a second state change to mean anything at all
(codex round 2). The revert restores values captured BEFORE the await,
so saving from the panel's resting state makes it write what is already
on screen — invisible, and an unfenced revert passed. They now choose
`specific`, start the save, then change the panel to `all` while the
request is in flight, so a revert STOMPS the visible state back and the
control can watch it happen.
Four things the second file had to learn the hard way, each now a comment
where the next reader will hit it. The real auth store does not bump its
epoch until an identity has been ESTABLISHED and then changed, so a test
calling `clear()` on a store no session was loaded into establishes the
baseline and measures nothing — its own guard on the bump caught that.
`waitFor` polls on a timer, so installing fake timers before the mount
hangs the setup at the 5s ceiling and reads exactly like a broken page. The
active tab is persisted in the URL hash, which jsdom keeps for the life of
the file, so a test that opened Members leaves the next one's fresh render
on Members with the General controls absent. And the timer legs install the
fake clock BEFORE resolving the request: a `setTimeout` scheduled while real
timers are live is not on the fake clock, so advancing it moves nothing —
the first version flipped identity before the resolve instead, which made
the await fence return early so the timer was never scheduled at all, and
it passed while measuring the await fence a second time.
Also stated rather than implied: this suite drives four of the fourteen
commit points, chosen one per timing class, and the mocked store moves only
the epoch so the real identity-change RELOAD path never fires here. The
source guard is the instrument that speaks about all fourteen.
Gates: `npx vitest run` 217 files / 2991 tests, 0 failures; `svelte-check`
0 errors.
Claude-Session: https://claude.ai/code/session_01PDYjP67KTAcqpFpo3ijAAA
|
||
|
xarmian
|
93693dc689 |
fix(store): a relation ref falls back to matching by number only when its prefix names no live collection (BUG-3082) (#1369)
Item numbers are workspace-unique and sequential ACROSS collections, so
`CONVE-1` and `SECRE-1` are never both real. `itemByRefQ` — the relation
rung's ref lookup — missed on prefix and fell through to
`WHERE item_number = ?` with the prefix DISCARDED, and the item it found
was usually inside the field's declared collection, so the
`item.CollectionID != targetID` check above it passed and nothing was
raised at any layer.
Measured on
|
||
|
xarmian
|
3cc3ba5e01 |
fix(web): flush the collaborative teardown on the events a suspended tab actually delivers (BUG-3030) (#1363)
* fix(web): flush the collaborative teardown on the events a suspended tab delivers (BUG-3030) Claude-Session: https://claude.ai/code/session_01PDYjP67KTAcqpFpo3ijAAA * fix(web): re-arm the teardown latch when a cancelled beforeunload leaves the page alive codex round 2. 'Stay' cancels the navigation without firing pageshow or visibilitychange, so the latch stayed set for the life of the page and every later teardown flushed nothing — a worse loss than the one being fixed. Claude-Session: https://claude.ai/code/session_01PDYjP67KTAcqpFpo3ijAAA * test(web): make the teardown source guard enumerate handlers and fail closed codex round 4. It rejected two literal spellings and checked runTeardownFlush() appeared somewhere in the block, so a new handler writing through any other call passed; and its function-body slicer failed OPEN when indexOf returned -1, scanning most of a 7,900-line file. Claude-Session: https://claude.ai/code/session_01PDYjP67KTAcqpFpo3ijAAA * test(web): measure that one committed teardown writes exactly once (BUG-3030) Lead withheld GO on this measurement rather than a reading: beforeunload -> [setTimeout 0] -> pagehide, and if the timer wins the same bytes go out twice. Measured 1 and 1, with no-edit controls at 0. Adds the jsdom leg for a hidden tab nobody edited writing nothing. Claude-Session: https://claude.ai/code/session_01PDYjP67KTAcqpFpo3ijAAA |
||
|
xarmian
|
a8f6fa7cae |
fix(items): an injected schema default takes the same type check a supplied value takes (BUG-3079) (#1362)
* fix(items): an injected schema default takes the same type check a supplied value takes (BUG-3079)
`ValidateFieldsDetailed` assigned a schema default and `continue`d past
`validateFieldType`, so the SAME BYTES were refused through one door and stored
through the other, decided only by who put them there. On a `status` retyped to
`multi_select` whose scalar default survived the retype:
POST .../items {"fields":"{\"status\":\"open\"}"} -> 400 must be an array of strings
POST .../items {"fields":"{}"} -> 201 stored {"status":"open"}
The second is the corruption. A scalar sitting in a list field is what BUG-3016,
BUG-3057, BUG-3067, BUG-3068 and BUG-3074 have each been defending against one
surface at a time, and it needs no retype of its own — only a schema whose
`default` and `type` disagree, which nothing prevents at collection-update time
either. It is reachable by any caller that creates without naming the field:
`pad item create`, the MCP create action, a plain API POST.
DROP RATHER THAN REFUSE, ruled on the trail. Nobody in the request typed the
value, and refusing would make a whole collection uncreatable — through surfaces
that never mention the field — over a schema defect its author must fix
elsewhere. It is the disposition TASK-2878 already chose for the one case that
had its own pass, now applied to every type.
A REQUIRED field is the exception, and it is the rule finishing its sentence
rather than a carve-out: dropping leaves the field absent, and an absent
required field is what IssueRequired reports. The message names the default as
the cause, because "field is required" pointing at a request that never
mentioned the field sends its reader to the wrong place.
THE VALIDATOR WAS NOT THE ONLY SITE. Dropping one step earlier silently deleted
the input two later passes were reporting FROM, so the drop kept happening and
the report vanished — DR-6 divergence in the direction that is harder to notice,
since the field is simply absent afterwards. The full suite caught it as
TestCopyEndpoint_NonStringDestinationDefaultDropsWithOrWithoutANullOverride, a
TASK-2878 test asserting the preflight reports a non-reference default as
`invalid_shape`; after the change it reported neither carried nor dropped. The
copy preflight and the cross-workspace copy door now fold the validator's drops
into the same bucket under the same `invalid_shape` reason — the same statement
in the same words, rather than a second spelling for one fact. The preflight's
`relationDropReason` map is renamed `dropReasonByKey`, because relations are now
one of two sources rather than the only one.
Prose the change falsified, swept (CONVE-23): four in-tree comments asserted
that `ValidateFields` skips its own type check, two of them load-bearing
arguments for why the relation pass exists. Each now says what is still true —
the shape half moved to the validator, and the relation pass keeps the half only
it can do, whether a shape-valid string names a live, visible item. Also the
`warnings.dropped_fields` contract note on `models.ItemWriteWarnings`, which
said "today that is one case".
Tool surface 0.34 -> 0.35. A write door stores something different from what it
stored before, which is the v0.30 / v0.29 / v0.16 behaviour-bump ground, and
`dropped_fields` widens from the single relation case v0.29 introduced to a
field of any type. The drift guard caught instructions.md and README.md;
CLAUDE.md swept by hand.
Evidence: 10 tests. Every unit test is written as a PAIR — supplied and injected
— because neither half proves it alone: "the default is dropped" is compatible
with a validator that refuses everything, and "a supplied value is refused" was
already true. Controls that a good default still applies, at the unit and at the
door, for the same reason. Full Go suite green (30 packages).
Claude-Session: https://claude.ai/code/session_019EmDd3Wy8b3cPG1TZBSzBu
* refactor(items): ValidateFieldsWithDrops returns ([]string, error) (BUG-3079)
ST1008 — error is the last return value. The signature went out as
`(error, []string)`, which reads backwards at every call site and is a lint
failure, not a style preference.
Mechanical: the definition, the wrapper above it, four production call sites
(create, update, bulk field update, cross-workspace copy) and one test call
site. No behaviour change.
Caught by `make lint`, which I had not run before opening the PR — `go test`
and `go vet` were both green and neither says anything about return order.
The test call site would have failed the suite too, but only AFTER the swap;
before it, the wrong order compiled everywhere.
make lint: 0 issues. Full Go suite green (30 packages).
Claude-Session: https://claude.ai/code/session_019EmDd3Wy8b3cPG1TZBSzBu
|
||
|
xarmian
|
7dc4931323 |
fix(db): a finished SQLite→Postgres migration marks its source so the abandoned file refuses writes (BUG-3072, BUG-3077) (#1361)
* probe(BUG-3072): measure SQLite auto-reprepare against a deferred append Not a keeper — the three legs become the proving tests. Claude-Session: https://claude.ai/code/session_01PDYjP67KTAcqpFpo3ijAAA * wip(BUG-3072): snapshot + marker + refusal triggers, with BUG-3077 folded in Claude-Session: https://claude.ai/code/session_01PDYjP67KTAcqpFpo3ijAAA * docs(BUG-3072): sweep the prose the snapshot and the marker falsified (CONVE-23) Claude-Session: https://claude.ai/code/session_01PDYjP67KTAcqpFpo3ijAAA * fix(BUG-3072): correct the snapshot justification to what was measured The design was argued on pooled reads diverging under a held transaction. Measured: they do not — SQLite has one write lock, so while BEGIN IMMEDIATE is held nothing else commits and a pooled read sees what the tx sees. The *Q threading stays, on the narrower and true grounds, and the property the argument now rests on is pinned by a test. Claude-Session: https://claude.ai/code/session_01PDYjP67KTAcqpFpo3ijAAA * test(BUG-3072): cover the two marker arms nothing else reached Claude-Session: https://claude.ai/code/session_01PDYjP67KTAcqpFpo3ijAAA * fix(BUG-3072): measure the outcome of a failed commit instead of asserting it codex round 2. Every other failure arm can honestly promise the source is unchanged because a rollback undoes the work; the commit arm cannot, and it was claiming so anyway. It now reads the marker on the pool and distinguishes landed / not-landed / genuinely-unknown. Claude-Session: https://claude.ai/code/session_01PDYjP67KTAcqpFpo3ijAAA |
||
|
xarmian
|
d2de0536c2 |
fix(web): six create paths ask the status field's type before defaulting it (BUG-3078) (#1360)
Six create paths independently read `statusField.options[0]` into a new item's
`status` with no type check. `options` survives a retype — the premise BUG-3057,
BUG-3067, BUG-3068 and BUG-3074 all turn on — so on a collection whose `status`
was retyped to `multi_select`, `number` or `checkbox` while keeping its options,
every one of them sent a scalar into a field that cannot hold it and the server
refused the create. Not a degraded create: no item, an error, and no route to a
good outcome. Quick capture simply stopped working on that collection.
The sites were QuickCaptureSheet, the Sidebar quick-add, EditorBubbleMenu, and
the collection page's three create paths. One of the six sits INSIDE the
function BUG-3057 fixed, six lines above its `laneWriteValue` call: the
conversion landed on the lane value and not on the status default directly
above it.
All six now call one `createDefaultFields`, which routes the default through
`laneWriteValue` — the same converter the drag and quick-create paths use, so
this cannot drift from them. `createDefaultCallers.test.ts` pins that there is
one copy, in the style `laneKeyCallers.test.ts` already uses for the sibling
question about a lane key.
CONVERT WHERE POSSIBLE, OMIT ONLY WHERE NOT, and the asymmetry is measured
rather than assumed. Against the real create handler, on a retyped collection
whose stale scalar default also survived:
supplied {"status":"open"} -> 400, "must be an array of strings"
omitted {} -> 201, stored {"status":"open"}
converted {"status":["open"]} -> 201, stored {"status":["open"]}
omitted, no stale default -> 201, stored {}
The omit arm lands but stores an unchecked scalar, because
`ValidateFieldsDetailed` assigns a schema default and continues past
`validateFieldType`. So omitting everywhere would have traded a loud refusal for
a silent corrupt write on exactly the collections this bug is about. Converting
is the only arm that both lands and stores a correctly-typed value; omit is kept
for `number` and `checkbox`, where no conversion exists and the alternative is
no item at all. That server-side hole is filed separately — it is reachable
today by any caller that creates without a status, so this routes onto an
existing path rather than opening one.
Codex round 1 found that `multi_relation` still went out as a scalar:
`laneWriteValue` passes a relation key straight through BY DESIGN, because a
lane key for a relation is already the target's item id. This caller holds no
lane key — it holds a leftover `select` option, which is never an id — so the
same string is a good value there and a bad one here, and the refusal belongs
in this module rather than in `laneWriteValue`, which would break the lane path
it was written for. `relation` is the same class and was found by asking what
else reaches that branch; both refusals were measured (400 `"open" does not
name an item in collection "tasks"`, and 400 `must be an array of strings`).
Evidence: 19 tests over the four arms and the six-caller guard. Counterfactual:
reverting the six callers to origin/main fails all eight guard assertions and
leaves both of its controls green. Mutation 6/6 killed, none build-broken — including a mutant that fixes only
the `multi_relation` codex named and leaves `relation` — and
one of them, dropping the `ok` check, SURVIVED the first version of these tests:
`laneWriteValue`'s refusal carries no `value`, so the mutant assigned `undefined`
and `toEqual({})` treats an undefined-valued key as absent. The omit cases now
assert key ABSENCE via `Object.keys`, which is the contract they meant all along. The guard keys
on the assignment shape rather than one spelling, since three sites wrote
`f => f.key` and three `(f) => f.key`; it carries a positive control over all
four spellings the sites used, so it cannot silently become a regex that matches
nothing. Full web suite 2954/2954, svelte-check 0 errors.
Claude-Session: https://claude.ai/code/session_019EmDd3Wy8b3cPG1TZBSzBu
|
||
|
xarmian
|
02240532de |
fix(web): the board lane's bulk move offers only destinations the field can hold (BUG-3074) (#1359)
* fix(web): the board lane's bulk move offers only destinations the field can hold (BUG-3074) A lane's ⋯ menu builds "Move all to" from `statusField.options` and nothing asked whether `status` was still a field one of those options could be written to. `options` SURVIVES a retype, so a `status` retyped to `multi_select` — a legitimately groupable type, so no grouping refusal fires and BoardView's `!isRelationGroup && !groupingRefusal` gate lets the menu through — rendered named destinations and sent a scalar into a list field. The server refused it once per item: an affordance promising a write that could not land. The existing `moveTargets.length > 0` guard looks like it covers this and does not. It withholds the entry for a field with no options, which is why `number` and `checkbox` appear safe — until the field HAD options before the retype, and then it offers real-looking lanes for a field that can hold none of them. `moveTargets` now filters through `laneKeyIsBulkMovable`, which asks the question via `laneWriteValue` rather than against a type list, so the menu cannot drift from the drag and quick-create paths. The bulk endpoint carries its destination as `Status string` (`handlers_items_bulk.go`), so the writes it can express are exactly the ones that resolve to a string. `multi_select` is WITHHELD rather than converted, and that is a semantic call rather than a plumbing one: `laneWriteValue` is happy to produce `['done']`, but "Move all to X" has no honest meaning on a multi-valued field — an item can sit in several lanes at once, so the verb would have to choose between replacing the whole list and swapping one element, with no way to say which it did. A `multi_relation` is refused grouping outright for the same reason. Evidence: 26 tests over the predicate and the binding, the binding tested by rendering the menu because a unit test of the predicate vouches for the predicate, not for the menu consulting it (CONVE-19). Each negative case carries a positive control in the same shape, since the pre-existing length guard withholds the entry for unrelated reasons. Counterfactual: reverting the one filter clause fails exactly the three retype cases and leaves the three controls green. Four mutants on the predicate (drop the string check, invert it, always true, always false) all killed, none build-broken. Full web suite 2949/2949, svelte-check 0 errors. Claude-Session: https://claude.ai/code/session_019EmDd3Wy8b3cPG1TZBSzBu * docs(web): rewrite two comments that read as fragments when line-wrapped (BUG-3074) Comments only; no behaviour change, and the 26 tests and svelte-check were re-run on the result. Both sentences were grammatically complete but scanned as truncated, and one was also imprecise, so this fixes the substance rather than only the wrapping. `laneWriteValue.ts` ended on a dangling appositive ("— the drift laneKeyCallers.test.ts exists to prevent for the key itself"). It now says what that guard is evidence OF: the same question about a lane key had already grown four private spellings across three surfaces, which had drifted to different answers. The `bulkMove` test header claimed a bare-absence test "would pass on the unfixed tree for half the cases and prove nothing for the other half". There is no such split. None of the three negative cases here would pass unfixed — the counterfactual run shows all three failing — because each gives the field a full option list, which is what the unfixed code offers. The vacuous case is the one this file does NOT contain: a field with no options, which the pre-existing `moveTargets.length > 0` guard withholds on either tree. The header now says that, which is the actual reason every case here has options. Claude-Session: https://claude.ai/code/session_019EmDd3Wy8b3cPG1TZBSzBu |
||
|
xarmian
|
a03c6d60bd |
fix(web): ten surfaces ask a field type before printing its value (BUG-3067) (#1358)
* fix(web): four schema-less surfaces ask the field's type before printing it (BUG-3067)
A `status` or `priority` field retyped to a `relation` stores a STRING — an
item id — so any surface that reads the value BY NAME and prints it shows a
raw uuid where a word belongs. BUG-3016 fixed every site that already had a
`FieldDef` in hand; these are the ones that read by name with no schema.
THE POPULATION WAS WIDENED THREE TIMES, twice by review and once here, so
this commit runs the instrument the item's own trail prescribes before
touching anything: grep the two KEY NAMES across `web/src`, rather than
grepping for field-rendering shapes. That found two sites nobody had listed
(`QuickActionsMenu` and `quick-action-preview`, see below) and confirmed the
four fixed here.
The body's premise that these need PLUMBING turns out to be too pessimistic,
and that is worth recording because it is what made the unit cheap:
`collectionStore` is a singleton any component may import, and every one of
these rows already carries its own `collection_slug`. `ChildItems` was even
importing `collectionStore` and `parseSchema` already — for the collection
PICKER, never for the row it was drawing. No prop threading was required
anywhere.
One shared helper (`$lib/collections/categoricalFieldValue`) rather than four
guards, for the reason `laneKeyCallers.test.ts` exists: four call sites asking
one question in four spellings is how the question comes to have four answers,
which is this class's documented history. Its core is pure — it takes the
collections rather than reading the store — so it tests without a store mock.
An unloaded store FAILS CLOSED: a row renders without its chip for a moment
rather than printing a value nothing has vouched for. Withholding a chip is
recoverable and printing an id is the bug. Asserted, and documented as the one
empty answer that is not an error.
`graph/DetailCard` gets both of its reads: `priority` from the item's fields
and `status` from the graph NODE, which is a server projection carrying no
schema — but it carries the collection slug, which is all the question needs,
and it renders before the item fetch lands.
Tests. The pure helper has its own; a source-level guard pins the CLASS (every
surface routes through the shared question, none answers it privately), since
a mutant that re-inlines a correct private copy is invisible to every
behavioural assertion; and `DetailCard` is driven end to end because it reads
both keys from two different places. Counterfactuals measured, not assumed:
reverting `NestedChildren` fails two legs of the source guard, reverting
`CommandPalette` fails one (its import leg — it renders `{meta.status}`, the
guarded function's OUTPUT, so the mustache leg cannot cover it, and the test
says so), and reverting `DetailCard` prints the raw uuid TWICE, which is the
defect reproduced directly.
NOT in this commit: `QuickActionsMenu.svelte` and `quick-action-preview.ts`
hold the same pair of schema-less by-name reads, but they substitute into a
PROMPT rather than into a chip, so "withhold it" may be the wrong remedy —
and the two are one site with two implementations that must agree, since the
preview exists to show what copying produces. Recorded on the trail for a
decision rather than folded in silently.
BUG-3067
Claude-Session: https://claude.ai/code/session_019EmDd3Wy8b3cPG1TZBSzBu
* fix(web): a fifth surface, a workspace-scoping hole, and one helper instead of two (BUG-3067, round 1)
Three findings, all from the adversarial round, all real.
1. A MEMBER I DISMISSED WITHOUT CHECKING. `playbooks/+page.svelte` reads
`fields.status` by name, prints it through `statusLabel`, and keys its card
styling off it. I had put it in the non-member bucket with the reason
"system collection, a retype is not reachable the same way" — and wrote,
in the same comment, that I would check rather than assume. I did not.
`handleUpdateCollection` has NO `is_system` gate on schema edits, so the
playbooks `status` can be retyped exactly like any user collection's.
The surface list in the source guard is data now, so a missed member is a
missing ROW someone can see rather than a silent absence.
(`conventions/+page.svelte` really is a non-member, re-checked this round:
it compares and WRITES `fields.status`, never renders it as a chip.)
2. A SLUG IS ONLY UNIQUE WITHIN A WORKSPACE. `collectionStore.collections` is
one global array that DELIBERATELY retains the previous workspace's
collections while the next load is in flight — the store documents this
itself, from BUG-1461. Two workspaces can each have a `tasks` whose
`priority` is a select in one and a relation in the other, so during that
window a slug-only lookup can answer with the wrong workspace's TYPE, in
either direction: printing an id, or withholding a good value.
`fieldDefFor` and both value forms now take the store's own
`collectionsAreFreshFor(wsSlug)` answer and fail closed when it is false —
the same direction as an unloaded store. It DEFAULTS to fresh, pinned by
its own leg: a surface with no workspace slug in hand must not be silently
blanked, and the stale window is the caller's to close where it can.
3. "ONE SHARED HELPER" WAS LITERALLY FALSE. `graph/DetailCard` called
`categoricalChipValue` — the lower-level dependency — directly for its
`status` read, because that read starts from a graph NODE rather than an
Item. Added `categoricalValueForSlug` so the projection case is served by
the same module, and the source guard now REFUSES an import of the
lower-level helper instead of accepting it as an alternative. The guard
previously treated the bypass as a valid spelling, which is how it passed
a file that was performing the bypass.
BUG-3067
Claude-Session: https://claude.ai/code/session_019EmDd3Wy8b3cPG1TZBSzBu
* fix(web): three more surfaces, and the freshness guard is now actually used (BUG-3067, round 2)
Round 2 found that my enumeration was wrong a SECOND time, and that the
previous commit described a guard nobody called.
1. "BOTH VALUE FORMS NOW TAKE THE STORE'S OWN collectionsAreFreshFor(wsSlug)"
WAS FALSE IN PRODUCTION. The parameter existed, defaulted to true, and no
call site passed it — so the cross-workspace hole was documented and tested
and not actually closed anywhere. A guard nobody uses reads, in a commit
message, exactly like a guard that works.
It is passed now at every surface that can reach a workspace slug:
`ChildItems` and `NestedChildren` from their `wsSlug` prop, `playbooks` and
the dashboard from `page.params.workspace`. The two graph surfaces have no
workspace slug in hand and are named below rather than quietly left.
2. THREE MORE MEMBERS, two fixed here. The dashboard's active AND starred
cards (one block, twice on the page) and `graph/ItemGraph`'s selection panel
all render SERVER PROJECTIONS — values the server itself obtained by name
through `extractFieldValue` — so a sweep for client-side `parseFields` reads
could not see them. That is the lesson rather than the fix: the class is "a
value obtained BY NAME", and where the name was resolved does not matter.
`OpenChildrenDialog` is the third and is NOT fixed: its DTO carries `ref`,
`title` and `status` and no collection identifier at all, so the declared
type is unanswerable client-side. It needs a server change, which is a
different unit.
3. The `playbooks` lookup hardcodes the slug `'playbooks'` and therefore does
not survive a collection rename. It inherits that from the page, which
already loads its items and its collection by the same literal — so the
page is already broken after a rename and this adds no new assumption. Said
out loud rather than left for the next reader to discover.
The source guard's surface list grew by the two fixed surfaces. It remains an
import-and-mustache check: it cannot prove the imported helper is CALLED, only
that the raw read is not rendered and the lower-level helper is not reached
past. Stated here because the previous commit's phrasing implied more.
BUG-3067
Claude-Session: https://claude.ai/code/session_019EmDd3Wy8b3cPG1TZBSzBu
* fix(web): narrow the staleness question, and close three rows the table exposed (BUG-3067, round 3)
Round 3 produced the POPULATION TABLE instead of another search, which is what
CONVE-35 asks for once successive rounds keep returning new members of one
class — at that point the rounds are measuring the enumeration, not the code.
The table found a regression I shipped last round and three rows I had called
covered or uncoverable.
1. MY ROUND-2 FRESHNESS GUARD BLANKED VALID CHIPS. It asked the store's
`collectionsAreFreshFor(wsSlug)`, which is false in TWO situations: the
loaded collections belong to another workspace, and no load has been stamped
at all. Only the first is a reason to withhold.
The second is routine rather than exceptional: `playbooks` and the dashboard
both fetch collections into PAGE-LOCAL state and never stamp the shared
store, so the stamp comes from the workspace layout. Treating unstamped as
stale blanked every status pill on those pages until the layout's load
landed — and permanently if that load failed while their own fetch
succeeded. Withholding a value for a load nobody is waiting on is not
fail-closed, it is just wrong.
`collectionsNotStaleFor` asks only "does the stamp name a DIFFERENT
workspace", with one spelling shared by all six call sites. The unloaded
case was always covered by the data — an empty array has no field to find —
which is the argument for narrowing the flag rather than widening it.
2. A FILE IS NOT A RENDERER. `ChildItems` draws twice: interactive rows
(`priority`) and a print list (`status`). Two passes fixed the first and
reported the file done, because both were looking for the first hit per
file. The guard now carries a second-renderer row that the file-level leg
cannot satisfy.
3. `OpenChildrenDialog` IS fixable client-side. I filed it as needing a server
change, claiming its DTO carried no collection identifier. It carries
`collection_slug` — the link rendered two lines from the status is built
from it. A claim about a shape I had not opened, in an item whose entire
history is enumeration claims that were not checked.
4. `ItemGraph` has a `workspace` prop, so my "the two graph surfaces have no
workspace slug in hand" was false for one of them. It passes staleness now.
`graph/DetailCard` genuinely has none and is named rather than left silent.
Also corrected: the stale "five surfaces" phrasing, and the source guard's
overstated reach — it proves the files and keys in its own list, and does not
sweep server projections, print markup, share routes or prompt substitution.
It says so now.
Still uncovered, deliberately and by name: the individual public-share route
(IDEA-3066, entangled with per-target redaction) and the `QuickActionsMenu` /
`quick-action-preview` pair, which substitutes into a PROMPT rather than a chip
— an empty substitution and a resolved title are both defensible and it is one
site with two implementations that must agree.
BUG-3067
Claude-Session: https://claude.ai/code/session_019EmDd3Wy8b3cPG1TZBSzBu
* fix(web): prompt variables resolve to a title, and two pages use the schema they fetched (BUG-3067, round 4)
The lead's ruling on the quick-action row, plus round 4's finding that my own
staleness plumbing was asking about the wrong array.
1. THE PROMPT SUBSTITUTION, per the ruling on the trail. A `status` or
`priority` retyped to a relation stores an item id, and both
`QuickActionsMenu` and `quick-action-preview` pasted that id straight into a
prompt the user hands to an agent. Chips answer this by WITHHOLDING; a
prompt variable cannot, because `{status}` has to become something and a
blank is a lie of a different kind. So, matching #1352: resolve to the
target's TITLE, fall back to its REF when the row will not resolve, never
emit the raw id, join a list with ', '. An unresolvable id yields empty only
because nothing true is left to say, not by choice.
The structural half matters as much: the two files held byte-identical
copies, and the preview module's own doc says it MIRRORS the menu so the
preview shows what copying produces — which is precisely what makes a drift
between them invisible, since the artifact that would reveal it is the one
built to match. One function now, in the preview module, imported by the
menu, with a guard asserting neither file carries the old spelling.
2. TWO PAGES NOW USE THE SCHEMA THEY ALREADY FETCHED. Round 3 narrowed the
staleness question; round 4 pointed out I was asking it about the wrong
array. The dashboard fetches collections alongside its dashboard under a
sequence guard, and `playbooks` fetches its collection under a documented
stale-response guard — and both then resolved the field type from the SHARED
store, whose stamp is set by the workspace layout and not by them. So a page
whose own fetch had succeeded could still render blank because a different
global had not loaded.
Both resolve from their own state now. The staleness question does not arise
there: the fetch is already workspace-guarded. The playbooks read also uses
that object's OWN slug, so it survives a rename of the playbooks collection
where the hardcoded literal would not — the rest of the page still addresses
it by literal, so the page as a whole does not, and the comment says so
rather than implying the rename is handled.
Still open from round 4, named rather than silently left: `graph/DetailCard`
and `OpenChildrenDialog` pass no staleness argument and have no workspace slug
in hand, so they default to permissive; and nine of the ten guarded renderers
have no behavioural coverage, only the source guard plus `DetailCard`'s.
Corrected: "six call sites" was wrong — there are seven guarded invocations and
two unguarded — and the source guard's comment claimed per-surface render tests
prove two surfaces call the helper, when only `DetailCard` has such coverage.
BUG-3067
Claude-Session: https://claude.ai/code/session_019EmDd3Wy8b3cPG1TZBSzBu
* fix(web): three defects round 5 found, plus both of the lead's ruled rows (BUG-3067, round 5)
1. I IMPLEMENTED THE RULING'S FALLBACK AND THEN DESCRIBED IT WRONGLY. My
comment and commit said the substitution falls back to the REF "when the
index cannot resolve the row". It does not, and it cannot: a ref is built
FROM the row (its collection prefix and item number), so an id that resolves
to nothing yields no row and therefore no ref. The fallback is for a row that
resolves but has NO TITLE, which is what the code does and what the test
pins. An unresolvable id yields empty because nothing true is left to say —
the one case the ruling's "never empty by choice" does not reach. Prose
corrected rather than code changed; the code was right.
2. `EditCollectionModal` called `contextFromItem` WITHOUT a resolver, which I
had defaulted to one that resolves nothing — so every relation-valued
`{status}`/`{priority}` in that preview rendered empty, quietly disagreeing
with the menu the preview exists to mirror. A default that silently produces
the wrong answer for an existing caller is worse than a required argument;
the caller passes the local-index resolver now.
3. "NO SCHEMA YET" IS NOT "DRAFT". The playbooks page's two loaders are
independent — `loadPlaybooks` clears `loading` on its own — so cards render
while `playbooksCollection` is still null. The helper answers '' for that,
which fell through to the `'draft'` default and reported every card as Draft:
a MISREPORT, not a missing chip, and permanent if the schema fetch fails. The
raw stored value stands in while the schema is unknown, and the declared
answer takes over the moment it lands.
THE LEAD'S TWO RULED ROWS, both closed:
- The permissive staleness default was allowed to stand only on a grep showing
every mount site sits under the `[workspace]` layout. `DetailCard` has ONE
mount site and it qualifies — and the slug is threaded anyway, because a prop
is checkable where an argument about mount sites goes stale the first time
someone mounts it elsewhere. `OpenChildrenDialog` FAILS that test: it is
mounted twice in the ROOT layout, so it can be on screen while the store holds
another workspace entirely. It takes the workspace from the route.
- Behavioural coverage: a parametrised leg over the shared helper across all ten
renderer shapes, plus the control that would catch a helper returning '' for
everything. A `NestedChildren` mount leg was written and REMOVED rather than
shipped — it asserted only the id's absence from a self-fetching component
with no precondition that any row rendered, so it passed on an empty
container. Making it real means mocking that component's loading, at which
point it measures the mock. The file says so instead of carrying the green.
BUG-3067
Claude-Session: https://claude.ai/code/session_019EmDd3Wy8b3cPG1TZBSzBu
* fix(web): my own fallback printed the id it was written to hide (BUG-3067, round 6)
1. THE REGRESSION I PREDICTED AND SHIPPED ANYWAY. Round 5's playbooks fix stood
the RAW stored value in while the schema was still null, reasoning that it is
what the page showed before this unit. For a retyped status the raw value IS
the id — so the fallback printed exactly what the unit exists to prevent, for
the duration of the window it was meant to fix, and worse than the "Draft"
misreport it replaced. I named this as the finding I most expected from the
round, in the round's own prompt, and it was there.
The honest answer for an unknown schema is to claim NOTHING: no status chip
and no draft/deprecated styling until the schema lands. The card still
renders with its title, trigger and steps.
2. A CONTROL LEG FOUND A REAL HOLE the moment the DetailCard test started
passing its now-required `wsSlug`: `collectionsNotStaleFor` compared the
stamp against `null` alone, so an `undefined` fell through to the equality
and answered STALE for every workspace. The store's field is typed
`string | null`, which is why it read as safe — but a guard whose answer
depends on WHICH empty it is handed is not a guard. Both spellings now mean
"nothing stamped"; the mutant reverting it dies.
The same test was passing no `wsSlug` at all and hiding it behind `as never`,
so it had been exercising a branch the component no longer takes. Fixed, and
its store mock now supplies the stamp so the legs run production's path.
3. `OpenChildrenDialog`'s staleness check is INERT on routes with no workspace
param — the console and the root, where this root-layout dialog can also
appear. That follows from treating an absent slug as permissive, which is
deliberate (blanking a value for a caller that cannot ask the question is the
failure that already cost this unit two pages of chips). Documented in the
component rather than closed: closing it needs the dialog to learn which
workspace the ITEM belongs to, which its DTO does not carry.
Also removed the stale "five surfaces" / "six call sites" counts from comments
that had outlived them — numbers in prose go stale silently, and this branch has
now changed that count four times.
Still open and named, not silently dropped: `playbooks/[slug]/+page.svelte`
assigns `fields.status` into a form control and writes it back. That is an
EDITOR rather than a renderer — a different sub-class, since the failure is a
relation id round-tripping through a select rather than being displayed — and it
wants its own unit.
BUG-3067
Claude-Session: https://claude.ai/code/session_019EmDd3Wy8b3cPG1TZBSzBu
* fix(web): a label that contradicted its own action, and a guard fed the wrong input (BUG-3067, round 7)
1. WITHHOLDING THE CHIP BROKE A BUTTON. Round 6 made the playbooks list claim
nothing while its schema is unknown, which was right for the chip — and the
toggle's label is computed from that same `status`, so every card read "Mark
as Draft" regardless of what it held, while the handler read the real stored
value and did something else. A label contradicting its own action is worse
than the wrong chip round 6 was fixing. The button waits for the schema
instead of guessing.
That is this unit's shape one more time: a value withheld at one site is a
value some other site was reading.
2. `OpenChildrenDialog` WAS COMPARING AGAINST A SLUG THAT MAY BE WRONG. Round 5
gave it the route's workspace param; round 7 pointed out the dialog is
root-mounted and its request can outlive a navigation, so that param can name
a workspace the children do not belong to. A guard fed the wrong input does
not fail safe — it answers "stale" for a store that is correct and "fresh"
for one that is not, confidently either way. Worse than no comparison.
It makes none now and says so. The real fix is for the children DTO to carry
the item's workspace, which it does not. The residual risk is rated and
stated: the value is still type-checked against whatever schema is loaded, so
the failure mode is a withheld or shown chip under a same-slug collision
between two workspaces — not a printed id, which is the thing this unit
exists to prevent.
3. The REF-fallback misstatement was still sitting in the ruling's own test
file, one commit after I corrected it in the implementation. Same wrong
sentence, second artifact — which is the pre-push habit this seat already
owes and did not run: a claim corrected in the source is not corrected in the
file that pins it.
BUG-3067
Claude-Session: https://claude.ai/code/session_019EmDd3Wy8b3cPG1TZBSzBu
* fix(web): sort on the declared value too, and stop pretending to compare (BUG-3067, round 8)
Round 8 returned the whole withheld-value table I asked for instead of one more
instance, which is what finally made this converge.
1. THE SORT WAS STILL READING THE RAW VALUE. Withholding the chip fixed what a
playbook card SHOWED and left what it was ordered BY — the same value wearing
a different hat. A retyped status stores an item id, which is not an order
key: `STATUS_ORDER` misses it and every such row collapses to the same
default rank. The sort asks the same question the cards ask now.
This is the third instance of one shape in this branch, and the reason to
state it as a shape: a value withheld at one site is a value some other site
was reading. Round 7 found it in a button LABEL, round 8 in a SORT KEY. The
table is on the trail so the next person starts from it.
2. NOTHING PRETENDS TO COMPARE ANY MORE. `OpenChildrenDialog` was left holding
`notStale = $derived(true)` and an unused import — a comparison that reads
like a guard and is a constant. Both are gone: the call simply does not pass
the argument, and the comment says why no comparison is possible here rather
than leaving a variable to imply one. Two dead imports on the dashboard page
went with them.
3. The dialog's prose still carried the round-5 paragraph calling the route
workspace the "honest source", directly above the round-7 paragraph
explaining that it is not. Corrected: one account, not two contradicting
ones, with the residual risk rated in it.
Left alone deliberately, from the same table: `isDone` / `doneCount` in
`ChildItems` and `NestedChildren` still test the raw status against the terminal
list. A relation id matches nothing there, so such a child reads as NOT DONE —
which is the honest answer for a status nobody can interpret, and changing it
would make a checkbox depend on a schema fetch.
BUG-3067
Claude-Session: https://claude.ai/code/session_019EmDd3Wy8b3cPG1TZBSzBu
* perf(web): resolve the sort's field once, through a door the module was missing (BUG-3067, round 9)
Round 9 found no functional defect. Three things, all small, and one of them is
the source guard earning its keep.
1. The playbooks sort called the helper INSIDE the comparator, so it reparsed
the collection's schema JSON on every comparison — O(n log n) parses of one
string to answer a question that is per-COLLECTION, not per-row. Resolved
once in a `$derived` and reused.
2. MY OWN GUARD CAUGHT THE FIX. Hoisting meant holding a `FieldDef` and needing
a way to answer with it, and the shortest path was to import
`categoricalChipValue` — the lower-level dependency the guard forbids. The
test failed the moment I ran it, which is the whole point: a bypass is how a
shared question acquires a second answer, and round 1 of this branch found
`DetailCard` doing exactly that. The guard was right and the module was
missing a door; `categoricalValueForField` is that door.
3. Two comments outlived their facts. `categoricalFieldValue.ts` still counted
the call sites, a number this branch has changed five times — it names them
without counting now. And `schemalessReaders.test.ts` claimed "per-surface
render tests prove two of the surfaces call it", which was never true after
the render test for the second surface was deleted as vacuous; it now states
exactly what it proves (no surface IN ITS OWN LIST answers privately) and
what it cannot (it does not sweep server projections, print markup, share
routes or prompt substitution, and cannot tell an imported helper that is
called from one that is ignored).
BUG-3067
Claude-Session: https://claude.ai/code/session_019EmDd3Wy8b3cPG1TZBSzBu
|
||
|
xarmian
|
5e869b285e |
fix(export): mark bundle bodies that were stale when written, and refuse the migration that would abandon them (BUG-3032) (#1356)
* wip(items): mark bundle items whose body is behind the live doc (BUG-3032 doors 1-2) * wip: doors 4 and 6 (BUG-3032) * wip: store test for the gate predicate (BUG-3032) * wip: door 3 import stale-body count + tests (BUG-3032) * docs(backup): the migration refuses on unflushed collab edits; bundles mark stale bodies (BUG-3032) * docs: discharge BUG-3032's open-decision conditional in the artifact door and the population guard * fix(export): the migration gate's population cannot diverge from the bundle's (BUG-3032) * docs(claude): Go build caches never go under /tmp — it is a tmpfs, so it is RAM * wip: codex round 1 P1 + P2 fixes (BUG-3032) * fix(db): refuse a migration whose bundle went stale after the pre-flight check (BUG-3032, codex r1 P1); print the import's stale-body count (P2) * fix(db): refuse to migrate while a server can append, and parse the stale-body header (BUG-3032, codex r2 P1+P2) * docs: narrow the running-server claim to what the probe measures (BUG-3032) * wip: codex r3 fixes + web parity test (BUG-3032) * fix(db): fail closed when the running-server check cannot run; match Go's header parsing on the web (BUG-3032, codex r3) * fix(cli): the Go and web readers of the stale-body header accept exactly the same values (BUG-3032, codex r4) * docs: correct two comment claims about integer representability and Atoi's leniency (BUG-3032, codex r5 nits) |
||
|
xarmian
|
3e5796eeab |
fix(web): a card status chip writes status, not the group field (BUG-3068) (#1357)
* fix(web): a card's status chip writes status, not the group field (BUG-3068)
`ListView` and `BoardView` each took one `onStatusChange` prop and handed it
to two consumers that mean different things: the drop handler, which means
"put this item in this lane" and names the GROUP field, and `ItemCard`'s
status chip, which means "set this item's status". The page's writer defaults
its target to `groupField`, so the chip's value went wherever the board was
grouped. The TABLE arm was fixed in BUG-3057 (#1353) by naming `status` at
the call site — it could, because its chip is its only consumer of the prop.
Split the prop: `onLaneChange` for the drop, `onStatusChange` for the chip,
with the page passing `handleStatusChange` for the first and the
`'status'`-named form for the second — the shape TableView already carried.
The two views failed DIFFERENTLY, which the bug report did not have:
- ListView was handed real status options, so grouped by `priority` the
chip cycled statuses correctly and delivered the result to
`fields.priority`.
- BoardView was handed its LANES as status options, so `ItemCard`'s
`statusOptions.indexOf(fields.status)` looked a status up in a list of
priorities, MISSED, and returned -1 — making the next index 0. Every
chip click wrote the FIRST LANE from wherever the card was, while the
label went on showing the status it was not changing. That write is
type-coherent, so BUG-3057's conversion accepted it silently.
So the board's `cardStatusOptions` now reads the `status` field's declared
options instead of the lanes. Grouped BY status the two are the same array,
so that case is unchanged.
The chip is no longer withheld on relation-grouped or refusal-grouped lists
and boards. It was withheld only because its write could reach the group
field; BoardView's own comment named repointing it at the real status field
as the alternative it was declining as out of scope. This is that unit.
Tests. `chipWritesStatus.svelte.test.ts` is new; 3 of its 10 legs go red
against the unfixed components and the file says which and why the rest
cannot — the list's defect is a DESTINATION defect, invisible to a mock that
cannot observe the caller's wiring, so the drop legs are what make the split
observable and `laneKeyCallers.test.ts` asserts the page's half (now counting
call sites, since BUG-3068 made two more sites carry the string its single
`toContain` matched). Four existing legs asserted the chip's ABSENCE under
relation/refusal grouping; they assert the click and its destination now,
which is strictly stronger than the absence was.
Two guards in `ItemCard` protect a retyped `status` from the board's new
options source, and both are pinned here after the first fixture pinned
neither: a relation-typed fixture is saved by `chippable` OR by the
value-shape test, so each leg now uses the type only its own guard can
withhold. Mutation-checked in both directions.
BUG-3068
Claude-Session: https://claude.ai/code/session_019EmDd3Wy8b3cPG1TZBSzBu
* fix(web): a read-only viewer gets no clickable status chip (BUG-3068, round 1)
Codex round 1 on this branch. Nothing has ever gated the status chip on
`canEdit` — `ItemCard` does not take the prop, and neither view passed one
through — so a read-only viewer was offered a clickable chip whose write the
server then refused.
PRE-EXISTING, not a regression from the previous commit: measured against
|
||
|
xarmian
|
bba2da6825 |
fix(web): the user menu's Sign out is inside the panel, not below its edge (BUG-2985) (#1355)
The sign-out button was there the whole time. It was rendering 59px below
the bottom of the panel that was supposed to contain it.
Measured in a browser before touching anything, three viewport heights,
identical at all of them:
.user-dropdown height 393
.menu-panel max-height 340, clientH 338, overflow-y auto
Sign out top 409, bottom 443
panel bottom 384
So the panel ends at 384 with a clean edge and no scrollbar — overlay
scrollbars paint nothing until you scroll — and the menu LOOKS complete.
"Connect a project…" is cut off too. Not viewport-dependent, not
cloud-only: that measurement is a self-hosted instance with no Billing
row, so cloud is worse.
The cap stays, since it is what keeps a menu off a short viewport. What
changes is that the panel stops being the SCROLLER for this one menu:
`Menu` takes an opt-in `bodyScroll` that makes it a clipped flex column,
and the dropdown becomes a scrolling body plus a pinned divider and Sign
out. Shortening the menu would only move the cliff — Billing, Admin and
the Resources block all come and go — so the account action is kept out
of the scrolling region entirely. Every other consumer of the primitive
is untouched; the flag defaults false.
`min-height: 0` on both flex boxes is load-bearing: a flex item defaults
to `min-height: auto` and refuses to shrink below its content, which
would push the pinned row back out of the panel — the bug restored, with
the markup looking right.
The behavioural instrument is the e2e leg, and it is deliberately NOT a
`toBeVisible()` check: an element scrolled out of an `overflow: auto`
container still has a box, so the pre-fix menu would have PASSED one. It
asserts the item's bottom edge against the panel's, and it goes red on a
binary built from the unfixed tree — 442.6 vs 384.5, the same 58px.
Claude-Session: https://claude.ai/code/session_019EmDd3Wy8b3cPG1TZBSzBu
|
||
|
xarmian
|
e0f54c5588 |
fix(web): a workspace you cannot reach is asked about once, not once per tick (BUG-2983) (#1354)
* fix(web): a workspace you cannot reach is asked about once, not once per tick (BUG-2983)
The report is titled 403. There is no 403 anywhere in it — every request
is a 404 until the rate limiter starts answering 429, and that is the bug.
`RequireWorkspaceAccess` resolves through `GetWorkspacesBySlugForUser`,
so a workspace the caller is not a member of does not resolve and is
answered `404 not_found` — the same answer a DELETED workspace gives,
deliberately, so a non-member cannot tell "exists but forbidden" from
"does not exist". 403 is reserved for cases where membership exists and
something narrower denies it. The client's entire access-revoked
machinery keys on 403, so it never fired for the commonest case there is:
following a link into someone else's workspace.
WHY IT REPEATED, measured rather than assumed. `bootstrap`'s SYNCHRONOUS
prefix reads `$state` (`bootstrapState`, `pendingResync`, the SvelteMap)
and writes it, and it runs before `loadData`'s first `await` — i.e.
inside `ItemDetail`'s route `$effect` tracking scope. Every write
re-fired the effect, which called `bootstrap`, which wrote again. On
SUCCESS that converges, which is why it went unseen: the next pass hits
the `'ready'` early return before any fetch. A failure lands on
`'error'`, which is not an early-return state, so every pass refetched.
An effect driven 12 ticks made 12 `/items-index` requests against a 404
and exactly 1 against a success. The four endpoints in the report repeat
together because they are one `Promise.all` in the re-running `loadData`.
The fix is one seam and one early return:
- A 404 on a workspace-scoped path with NO sub-resource identifier fires
the same stop-asking-and-purge handler 403 does. Narrower than the 403
parser on purpose: that one accepts `/workspaces/{ws}/items/{ref}`,
which is right for a refused read and wrong for a missing one — a 404
there is usually a dead link, and purging a readable workspace over it
would be worse than the storm. Keyed on request shape because the
workspace 404 and an item 404 carry the same `not_found` code today;
BUG-3069 files the server-side code that would replace the inference.
- `bootstrap` returns early on a terminal refusal, and that return reads
a PLAIN Set and writes nothing. Position and non-reactivity are both
the fix: a reactive flag, or any write in that branch, would leave the
effect spinning at full speed with only the network removed — the same
defect, now invisible. A leg counts EFFECT RUNS, not just requests, so
that masking cannot pass.
- The refusal is keyed by (user, workspace) and cleared on identity
change: signing in as someone who IS a member must not inherit it.
The banner splits from the generic retry box, because the two need
opposite affordances — that one offers Retry, and retrying this is what
produced the storm. Wording per the lead's ruling: "isn't available to
you, or no longer exists", never "access revoked", which would assert the
half the server deliberately withheld.
Tests: 6 seam legs (both qualifying paths fire; a missing ITEM and a
missing COLLECTION do not; the 403 half still fires on both shapes, as
the control for the whole change) and 6 store legs (stops asking, stops
RE-RUNNING, the identity split, a reachable-workspace control, and an
in-file counterfactual that reproduces the unbounded pre-fix behaviour in
the same harness). Removing the early return fails two of them.
Claude-Session: https://claude.ai/code/session_019EmDd3Wy8b3cPG1TZBSzBu
* fix(web): stamp the asking identity, and opt endpoints into the 404 seam (BUG-2983)
Both from the enumeration round, both real.
IDENTITY (P1). The refusal was recorded against whoever was signed in when
the ANSWER arrived. A response can outlive the identity that asked — sign
out or switch users mid-flight — so a 404 landing after the switch would
refuse the workspace to a user who never asked, possibly a member of it,
and permanently: the identity-change clear has already run by then.
Requests now carry the identity captured BEFORE they leave, supplied by a
provider the layout registers (the same no-store-import idiom the
access-revoked handler uses), and the handler drops any refusal whose
stamp does not match the current user.
ALLOW-LIST (P2). "A 404 at depth 3 is the workspace" was too broad: a
workspace-level endpoint may have its own not_found — a singleton never
created, a feature-gated route — and one of those would purge a workspace
the caller reads fine. The seam is destructive, so endpoints opt IN:
items-index, items-changes, items, collections. That is the local-first
read model's own load set, and the only not_found any of them can produce
is the workspace's. Same posture as the 403 parser's whitelist.
Four existing layout tests mocked `$lib/api/client` with a partial object
and threw on the new export before any assertion ran. Stubbed, with the
reason in place.
Legs: an endpoint outside the list does not fire, with a control that the
listed ones still do; the scope carries the issue-time identity across a
switch made while the request is in flight; an unregistered provider
leaves it undefined (SSR / tests), which the handler reads as "unknown,
do not second-guess" rather than as anonymous; and a source guard that
the layout registers the provider and compares before recording.
Claude-Session: https://claude.ai/code/session_019EmDd3Wy8b3cPG1TZBSzBu
|
||
|
xarmian
|
9e121bde64 |
fix(web): a board lane writes a value the schema can hold, not the lane key (BUG-3057) (#1353)
* fix(web): a board lane writes a value the schema can hold, not the lane key (BUG-3057)
A lane key is a STRING projection of a field value — that is what lets a
view group by anything. Both write paths on the collection page assigned
that string straight back to the field: `fields[groupField] = newValue` on
the drag path, `defaultFields[groupField] = groupValue` on create-in-lane.
The item was filed as silent retyping. It is not: the server REFUSES these
writes. Measured against `internal/items.ValidatePartialFields`, the
validator behind `fields_patch`:
number "0" -> field "score" must be a number
number "" -> field "score" must be a number
checkbox "false" -> field "shipped" must be a boolean
multi_select "c" -> field "tags" must be an array of strings
So the symptom is FAILURE, not corruption: on a board whose group field is
not string-shaped, a legitimate drag between lanes and a legitimate
create-in-lane do not take. Nothing bad is stored, and there is nothing to
migrate.
`multi_select` is the reachable one — the display-settings picker offers
select AND multi_select, so no retype is needed to get there. The rest
arrive the way BUG-3041's and BUG-3016's cases do: a field retyped under a
`board_group_by` that still names it, or a setting written through the API.
`laneWriteValue` converts through the declared type: a number gets a
number (including 0), a checkbox gets a bool for the two spellings the
projection makes and nothing else, a multi_select gets a one-element array,
and everything string-shaped keeps its key unchanged. The UNCATEGORIZED
lane means "no value", whose honest spelling differs by type — `''` for a
select (a legal clear), `[]` for a multi_select, and the patch path's
DELETE sentinel `null` for a number or checkbox, which have no empty
string to hold. A lane key that cannot be a value at all (a stale `open`
lane over a retyped number field) refuses the write, toasts, and throws,
so BoardView unwinds its optimistic move instead of showing it as landed.
Tests: 8 unit legs on the converter, and the same table pinned from the GO
side (`TestLaneKeyWrites_BUG3057`) — that test is not a test of the fix,
it is a test of the facts the fix was derived from, so a validator change
that started accepting the string forms cannot leave the module correct
against a validator that has moved. The call sites are guarded at the
SOURCE, beside BUG-3053's read-side guards, because a behaviour test of
the converter stays green when a call site stops calling it. All 4 guard
legs fail against the unfixed page.
One prior guard updated: BUG-3049's "patches only the group field" leg
anchored on the literal `[groupField]: newValue`. The patched KEY is what
it is about and is unchanged; the value spelling moved. Re-anchored with
the reason in place.
Claude-Session: https://claude.ai/code/session_019EmDd3Wy8b3cPG1TZBSzBu
* fix(web): the table's status chip writes STATUS, not the lane field (BUG-3057)
From the enumeration round, verified in the wiring rather than taken on
report. `handleStatusChange` is one callback serving two intents that do
not name the same field: a drag between lanes means "put this item in THIS
LANE" (the group field), and a status chip means "set this item's STATUS"
(the `status` field). The group field was hardcoded, so the TABLE's status
chip — whose options come from the `status` schema field — wrote its value
into whatever `list_group_by` named. On a table grouped by `priority`,
clicking a status chip set the priority.
Pre-existing, and reachable with no retype: `list_group_by` takes any
select field from the display-settings picker. What this unit changed is
its failure mode, not its existence — with the type conversion in place a
status word sent to a number-typed group field is now refused and toasted
rather than written, so leaving it would have meant shipping a change that
alters an existing defect's behaviour without saying so.
The writer takes its target field explicitly, defaulting to `groupField`
so every drag caller is unchanged; the table names `status` at its call
site. It can, because the chip is its only consumer of that prop.
NOT fixed here: `ListView` and `BoardView` have the same conflation with
two consumers behind ONE prop — their cards' status chips send status
values to the group field too. That needs a second callback threaded
through both components into ItemCard, which is a prop-surface change with
its own test surface. Filed as BUG-3068, and cited from the writer's doc
comment. ListView's existing comment shows the defect was already
diagnosed there and closed for one arm only (a relation group field), the
arm that had a unit in flight at the time.
Claude-Session: https://claude.ai/code/session_019EmDd3Wy8b3cPG1TZBSzBu
* fix(web): a multi_select lane writes its COMBINATION, not the joined key (BUG-3057)
Lead's catch on the review, measured before changing anything.
`laneValue(['a','b'])` is `'a,b'`, and ListView MINTS a lane for every
distinct value it finds, so a list grouped by a multi_select really does
show an `a,b` lane. (The board seeds its lanes from the field's options
instead, so the same two-tag item lands in UNCATEGORIZED there — measured
both ways.) A drop onto that lane means "give this item that lane's tags".
Wrapping the key wrote `["a,b"]` — one tag, not two, and not a tag anyone
has. Measured on the pre-fix module:
write for lane "a,b" = {"ok":true,"value":["a,b"]}
Both outcomes were wrong and neither was silent-and-harmless: with options
declared the server REFUSES it (`"a,b"` is not in allowed options, now
pinned from the Go side), and with no options declared it stores a tag
that exists nowhere.
The arm now inverts the projection against the DECLARED OPTIONS rather
than splitting blindly, so an option containing a comma is not torn in
half: the key IS an option -> that option; every comma-part is an option
-> those options in lane order; no options declared -> split, the plain
inverse of the join; otherwise -> the key whole. Rule 1 before rule 2 is a
stated tie-break, because with options `['a,b','a','b']` the lane is
genuinely ambiguous — the projection is lossy — and the reading that names
an option that EXISTS wins.
Six legs, including the two-tag lane, a single-tag CONTROL (without it,
"always split" and "always wrap" each pass for the wrong reason on one
side), a comma-in-option leg, the ambiguous tie-break, and the no-options
split. Mutating the arm back to the wrap fails exactly two of them and
leaves the control and the comma leg green, which is what those two are
for.
Claude-Session: https://claude.ai/code/session_019EmDd3Wy8b3cPG1TZBSzBu
|
||
|
xarmian
|
721a638f5a |
fix(web): table, share and card surfaces render a relation, not its stored id (BUG-3016) (#1352)
* fix(web): table and share surfaces render a relation, not its stored id (BUG-3016) A relation field stores an item ID. Four surfaces resolve it and show `REF · title` — the properties chip (U2), the board lane, the list group heading and the filter chip (U7). The table was the fifth and did not: both cell arms printed `fields[field.key]`, so a column showed a UUID. Authenticated table (TableView.svelte): resolves through the SAME `narrowRelationRow` / `relationChipFor` the other four use — id-only (never slug, because free text like "red" would resolve to whatever is slugged red today, and slugs are mutable) and scoped to the field's declared target collection. One chip for a `relation`, N in stored order for a `multi_relation`, "Unresolved reference" for a value that names nothing, and a `(deleted)` note for a dangling target. No private copy of the rules: a divergence shows up as a table cell and a properties chip disagreeing about one id. The relation arm comes FIRST, and that ordering is load-bearing twice. `field.options` survives a retype in the schema editor (BUG-3041), so a field keyed `status` that is now a relation satisfied the status arm's guard and rendered a CLICKABLE status chip — whose click writes a status string into a relation field. That is the defect the board and list withhold their status chip for, arriving through the column instead of the lane. Asking the schema first answers both. Public share surfaces (PublicTableView, PublicItemExpansion): a share payload carries values with no index behind them, which is why U7 refused relation GROUPING there rather than labelling lanes with ids. A column is not refusable the same way — hiding it silently changes what the owner shared — so the cell says what it holds: "Linked item" / "N linked items", with a tooltip saying the name is not part of the share. That placeholder is a boundary, not a shrug (lead ruling). Hydrating ref+title into the payload is a VISIBILITY question, not a payload one: the target may sit in a collection the owner never shared and the viewer is anonymous, so it needs the same per-target redaction U4 built for authenticated reads. Filed as IDEA-3066, which also carries the one surface this change could not reach — `publicShareItemDTO`'s single-item share sends `fields` with no schema, so its client cannot tell which key is a relation. The read-shape normalisation moved out of FieldEditor to `relationValuesOf` so the table uses it verbatim; FieldEditor keeps its write-side hold in front of it. Tests: 8 legs on the table cell (both narrowings asserted through the COMPONENT, since `narrowRelationRow`'s own tests stay green if the call site reverts — CONVE-19), 6 on the two public surfaces, 5 units on `relationValuesOf`, each with a control that fails if the fix were "stop printing field values". Verified against the unfixed tree: 6 of 8 and 3 of 6 fail there; the passing ones are the controls and the empty-value leg, which are not the defect. Two legs in TableView.listValue.svelte.test.ts asserted the id WAS shown — honest under BUG-3041, when nothing here could resolve a relation, and superseded now. Rewritten to the new invariant with that recorded in place. Claude-Session: https://claude.ai/code/session_019EmDd3Wy8b3cPG1TZBSzBu * fix(web): a card's status chip asks the schema, not only the value's shape (BUG-3016) Second half of the same class, from the enumeration round. My first pass scoped the population to surfaces that render ARBITRARY schema fields, and that was too narrow: a surface reading a field BY NAME is in the class too, because the NAME can be retyped. ItemCard, PublicItemCard and PublicListView read `status` / `priority` and then test the VALUE's shape — "is it a string". That is the right question for a `multi_relation` (its value is an array, so it falls out) and the wrong one for a scalar `relation`, whose value IS a string. A `status` retyped to a relation rendered its stored item id as a status pill, title-cased by formatLabel; on ItemCard the pill is CLICKABLE, so cycling it wrote a status word into a relation field. Withheld rather than resolved: these pills are categorical summaries, and "Linked item" is not a status. The item's own properties panel renders the reference properly. Public side goes through one `categoricalChipValue` helper so the two components cannot drift. Not fixed here, filed instead: ChildItems, NestedChildren and the graph DetailCard have the same defect with a different cause — they render children from any collection and hold no schema at all, so the fix is plumbing rather than a guard (BUG-3067). Instrument correction worth recording: the first version of the two public legs asserted `not.toContain(RELATION_ID)` and SURVIVED a mutation that removed the guard — `formatLabel` replaces `-` with spaces and title-cases, so a rendered uuid reaches the DOM as "F47ac10b 58cc …" and the assertion could not fail. Re-aimed at the pill ELEMENT; both legs go red on the same mutant now, and the comment says so in place. Claude-Session: https://claude.ai/code/session_019EmDd3Wy8b3cPG1TZBSzBu |
||
|
xarmian
|
d2973b24d7 |
feat(items): expected_seq — an OCC token that can tell two same-second writes apart (BUG-3037) (#1351)
* feat(items): expected_seq — the strong OCC token (BUG-3037) * test(items): the same-second race, with each token (BUG-3037) * test(cli): the summary shape carries the seq token (BUG-3037, matrix hole) * test(web): the pane sends the seq token, with the cached-row arm (BUG-3037) * test(web): the OCC retry re-reads the token through tokenOf (BUG-3037, matrix hole) * docs: CLAUDE.md carries the v0.34 tool-surface entry (BUG-3037) * test(e2e): the pane's field write is guarded by seq (BUG-3037) * fix(mcp,webmcp): forward the OCC token and refuse a fractional seq (codex round 1) * test(items): the reproduction no longer races the wall clock (BUG-3037) * fix(items): seq decides when both OCC tokens are sent (codex round 2) * fix(cli): --expected-seq is sent by presence, so an explicit 0 is not dropped (codex round 3) * fix(cli): render a seq conflict with the token the caller sent (codex round 4) * docs(mcp): agent instructions recommend expected_seq (codex round 5) |
||
|
xarmian
|
809249aaef |
test(e2e): assert the ctrl-click tab through CDP, not through a page event (BUG-2993) (#1350)
`pane-content-link-anchors.spec.ts` failed about one run in three with
`browserContext.waitForEvent('page')` timing out after a ctrl-click. It was read
as a product defect for three separate passes.
It is not. The tab really opens. Four instrumented rounds over 61 runs, 19
failures, established the whole chain: Chromium accepts the click
(`Page.frameRequestedNavigation`, reason `anchorClick`, correct url), creates a
page target, navigates it to that url and keeps it — and Playwright's
BrowserContext never adopts the target, never emits `page`, and never lists it
in `context.pages()`. Measured 22 seconds after the click with the target alive:
one page in the context, and the `waitForEvent` had burned its full timeout.
So the property under test — "a ctrl-click opens a new tab at the item's url" —
was asserted through a channel that does not reliably report it, and every run
the suite dropped was a run where the product did the right thing.
`ctrlClickOpensItemTab` observes the browser-level Target domain instead, which
fired correctly in all 19 recorded failures. Two legs, cheapest first: the
anchor's href contract, then a new page target reaching `/docs/<segment>`. The
tab is closed through `Target.closeTarget`, since no Playwright `Page` exists
for it.
THE ONE THING THAT DOES NOT WORK is polling `context.pages()`, and it is the
first thing anyone reaches for: the page never enters that list at all. Said in
the helper's comment so the next person does not spend the rounds.
Both legs shown able to go red, each for its own reason and neither masking the
other: a wrong segment fails on the href leg (`href … should end with …`), and a
valid href with a click that opens no tab fails on the target leg (`no NEW page
target reached … targets seen: []`). The first control only proved the cheap
leg, which is why the second exists.
Gate: 30 consecutive runs of the whole file, 0 failures — against a prior rate
of ~1 in 3 over 61. Run time also drops from ~18s to ~11s, since nothing waits
out a 20s timeout any more.
Not a retry and not a dropped assertion: the spec asserts the same property,
through a channel that can see it.
Instrument and full evidence: branch `bug-2993-ctrl-click-popup` (not for
merge), and the BUG-2993 trail.
Claude-Session: https://claude.ai/code/session_019EmDd3Wy8b3cPG1TZBSzBu
|
||
|
xarmian
|
70e3659526 |
fix(items): twelve item-field writers send a patch instead of replacing the blob (BUG-3049 / BUG-3055) (#1349)
* wip(items): convert full-blob item writers to fields_patch (BUG-3049) * test(items): interleaving tests for the CLI and bulk doors (BUG-3049) * test(webmcp): pad_item update merges instead of replacing (BUG-3055) * test(web): per-door guards for the browser full-blob writers (BUG-3049) * docs(web): correct the BUG-2273 note the patch conversion falsified (BUG-3049) * style: gofmt the new server test (BUG-3049) * test(webmcp): repoint the dispatch update leg at the patch contract (BUG-3055) * test(e2e): status move preserves a concurrently written field (BUG-3049) * test(e2e): hold the stale-copy window open with the stream blocked (BUG-3049) * test(webmcp): label the non-detecting leg as such (codex round 2) * fix(items): refuse an unreadable fields snapshot in the bulk door (codex round 3) * fix(items): re-decide auto-populated dates under the write lock (codex round 4) * docs(types): ItemUpdate.fields is a full replace; updates use fields_patch (BUG-3049) |
||
|
xarmian
|
1c15462f34 |
fix(web): a list item grouped by 0 or false no longer vanishes (BUG-3053) (#1348)
* fix(web): a list item grouped by 0 or false no longer vanishes (BUG-3053) Group a list by a number field and an item scoring 0 disappears — no lane, no Uncategorized, nothing. Same for a checkbox holding `false`. ListView's two passes disagreed about what "no group" means. The lane list read the RAW field value and tested it for FALSINESS, so `0` and `false` were filed as ungrouped and only the `''` lane was created for them. The bucketing pass stored those items under the STRINGIFIED value, `'0'` and `'false'` — keys no lane pointed at. Rendering reads `groupData['']`, which is empty. A falsiness test standing in for an emptiness test, and the two halves disagreeing was the whole defect: either one alone would have been visible. One predicate now answers it, in `$lib/collections/boardColumns`, read by both passes: `laneValue` normalises (absent, null and `''` are the only no-value cases; `0` and `false` get `'0'` and `'false'` like any other value) and `isUngrouped` asks the emptiness question. Both take a RAW value and normalise, which is the load-bearing part — with a `string` parameter `!value` and `value === ''` are the same function, and a mutant swapping them cannot be killed. They are opposites only for a raw value, and a raw value reaching that door is what happened here. BoardView was NOT affected, and not by luck: `bucketByColumn` already stringified before testing, so `0` became the truthy `'0'` and landed in a real lane or in Uncategorized — visible either way. The conflation was specific to the view that DISCOVERS its groups from the items. Pinned with tests so a change to the shared helper cannot move it silently. Swept the class rather than the instance: both views also carried a byte-identical private `formatLabel` with its own falsiness test, one step downstream, which would have labelled a `0` lane "Uncategorized" the moment it was handed an unnormalised value. Now one `formatLaneLabel`, normalising for the same reason. Reproduced before fixing: three legs red, and a fourth green — absent, null and `''` must still fold into Uncategorized, which is the behaviour this must not break. That fourth leg failing first is what caught a wrong selector in my own harness, which had made the first two legs fail for the wrong reason. Ten mutants, eight killed. Two survive: reverting `groupValueFor` to the unnormalised cast, and one `isUngrouped` call site. Both are equivalent under the current consumers — `isUngrouped` normalises at its own door, so the drop cannot come back, and JS object-key coercion quietly agrees that `result[0]` and `result['0']` are one bucket. Kept and documented: normalising at that point is what makes the function's declared return type true and keeps the conversion at one place, rather than resting on a coincidence of key coercion plus every downstream consumer remembering to normalise — the arrangement that produced this bug. Claude-Session: https://claude.ai/code/session_019EmDd3Wy8b3cPG1TZBSzBu * fix(web): consolidate the lane-key question after review found two more copies (BUG-3053) Codex round 1, enumeration-prompted. One finding refuted a claim I had written into the source; three more were real. **A defect my own fix made reachable (finding 1).** `handleFinalize` compared the normalised lane key against the RAW field value, so an item dropped back into its own lane read as a move: `0 !== '0'`, `false !== 'false'`. The "move" then wrote the STRING '0' into a number field. Unreachable before this fix only because the item was dropped from the view and could not be dragged at all, which is why it arrives WITH the fix rather than before it. Both sides normalise now. That also refutes what I had documented. I wrote that reverting `groupValueFor` to the unnormalised cast was an equivalent mutant, on the reasoning that `isUngrouped` normalises at its own door and object-key coercion hides the rest. It does — for the two passes I was looking at. The drop handler is a third consumer and it compares with `!==`, where coercion does nothing. The mutant is now killed by a test that drives the real `finalize` event. **The population was four, not two (finding 8).** The commit before this one said the class was two copies of the lane-label function. The public share view has its own `groupItems` carrying a third inlined copy of the normalise-and-ask pair — correct, but only because it stringifies BEFORE testing, which is exactly the step ListView skipped in one of its two passes. It reads the shared helpers now, and a test covers grouping a public board by 0. Its `formatLabel` deliberately stays private: it title-cases field KEYS as well as values, strips hyphens as well as underscores, and has no Uncategorized case. A different function that happens to share a name. **A source guard, because consolidation cannot be measured by behaviour.** A mutant that re-inlines a CORRECT private copy is invisible to every assertion in the suite — that is what a refactor means. "There is one copy" is a fact about the source, so `laneKeyCallers.test.ts` asserts it where it lives, with a control leg proving its patterns can fire. **Stale prose (finding 12).** The claim that arrays and objects "simply won't match a known option" is false: a board option whose text equals the stringification matches it, and a view that discovers its lanes mints one named after it either way. Corrected to what is actually guaranteed — they stay visible. Thirteen mutants, twelve killed. The survivor is one `isUngrouped` call site where the argument is always a string, so it and `!value` are the same function; kept and documented, because that equivalence holds only while every caller normalises first, which is the assumption that stopped holding and produced this bug. Two faults in my own harness, both caught by a counterfactual leg I wrote for exactly that purpose: the drop event carried the wrong item id, so the handler found no item and skipped the comparison entirely — a green measuring nothing — and the source guard's first version forbade shareView a `formatLabel` it is right to keep. Claude-Session: https://claude.ai/code/session_019EmDd3Wy8b3cPG1TZBSzBu * fix(web): the board's drop comparison was the fifth consumer, not a checked one (BUG-3053) Lead review of #1348 found a member I had reported as covered. `commitColumnMove`'s non-relation arm compared the RAW field value against the lane string: const currentValue = isRelationGroup ? relationLaneValueFor(…) : fields[groupField]; if (!groupingRefusal && currentValue !== targetColumn) { … } So a card scoring 0, dropped back into its own lane, reads as a move — `0 !== '0'` — and the "move" writes the STRING '0' into a number field. WORSE THAN THE LIST'S COPY, not equal to it. BoardView has bucketed through `laneValue` since IDEA-2275, so a 0-scored card has always been visible in lane '0' and has always been draggable: this one is live on main today. The identical line in ListView was unreachable until the item stopped being dropped from the view, which is the only reason that one arrived with its fix rather than before. RE-RAN THE ENUMERATION rather than fixing the named instance, since the named instance is what I did last time. Every comparison against a lane key (`!== targetColumn`, `!== groupName`, `!== columnValue`, `!== colValue`) and every `[groupField]` read that reaches one: - shareView.ts:389, ListView groupValueFor, ListView handleFinalize, boardColumns bucketByColumn — all normalise. - BoardView commitColumnMove — the raw one. Fixed here. - Every other lane comparison in both views has a lane key on BOTH sides (colValue vs draggedColumn / openMenuColumn / dragOverColumn, groupName vs confirmArchiveGroup), so none can see a raw value. That is the whole population for the READ side, and it is now five of five. Two WRITE sites are reported and deliberately not touched, both in the collection page: `handleStatusChange` writes the lane key into the field, and the create-in-lane path pre-fills it. A real cross-lane drop therefore still writes the string '0' into a number field. That is a different defect — type loss on a legitimate write, not a spurious write — and it is outside the diff boundary this GO was bound to. Three legs driving BoardView's real `finalize` event: a 0-valued card and a false-valued card dropped into their own lane write nothing, and a genuine cross-lane drop still does. Verified RED against the unfixed arm (E14). Also fixed: the source guard added last commit was itself broken by this one. It found the boardColumns import by matching a single LINE, and BoardView's import grew to four lines — so the matched line was `} from '…';`, containing no names. It went red on an unmutated tree and "killed" an unrelated mutant while it was there. It reads the whole import statement now, with a control leg for the wrapped shape. An instrument keyed on formatting measures the formatting. Claude-Session: https://claude.ai/code/session_019EmDd3Wy8b3cPG1TZBSzBu |
||
|
xarmian
|
74448dd5ce |
fix(web): a typed edit survives the previous keystroke echoing back (BUG-3039) (#1347)
* fix(web): a typed edit survives the previous keystroke echoing back (BUG-3039)
Type `a` into a text field, and while that write is in flight type `b`. The `a`
response lands, the parent re-props `value`, and the value-track effect cancels
the pending `ab` timer. Only `a` is ever saved, and `ab` is taken off the screen
too — no error anywhere. No ticket is taken for the second write, so PLAN-2857
U4's ordering model cannot see it either: that model orders writes that were
DISPATCHED, and this one never is.
The effect is there for a real reason — a value arriving from SSE, a collab
peer, or an item switch must replace what the field shows — and it could not
tell that case from its own write coming home. The comment claimed it did not
need to, because the timer clears `hasPending` before calling onchange: true
only while no new typing starts between the send and the echo, which is a 500ms
debounce plus a round trip, which is the window a person types in.
Three cases now. The item changed — retarget, drop. `value` changed to something
we did not send — theirs is newer, drop. `value` changed to exactly what we last
sent for THIS item — our echo, keep the newer keystrokes.
**The fence is the item, not the text**, via a new optional `itemId` prop the
pane passes. The text cannot answer it: the next item's value can equal what we
just sent for this one, and flushing then writes item A's typing onto item B —
the Codex round 3 [P1] this effect was originally built for. A caller that gives
no id still gets the echo check; it has nothing to retarget between, so "same
subject?" answers true rather than answering nothing.
Keeping the WRITE was not enough, because `value={value ?? ''}` is a one-way
binding Svelte re-applies on every prop change. A display hold — the last thing
we sent wins over the prop until the round trip catches up, the same shape as
`relationValues`' hold, asked for one string instead of a list — keeps the
character on screen. It holds the raw typed string, so a half-typed number (`1.`,
`-`) is not rewritten under the cursor.
A second, pre-existing hole closed on the way: the effect tracked only `value`,
so a retarget to a row holding the SAME text did not re-run it and the abandoned
keystrokes stayed armed to flush onto the new row.
Reproduced before fixing: two legs red against the unfixed tree, one per half of
the report, and the two legs describing what must SURVIVE green. Eleven mutants,
ten killed. The eleventh is `void itemId`, measured redundant — in this Svelte
version an effect re-runs on every assignment of a prop it reads, equal or not,
which is the same behaviour round 8 hit as a hazard — and kept anyway, because
the logic depends on `itemId` and declaring that beats resting on an incidental
re-run a future equality short-circuit would remove silently. The comment says
so rather than leaving a reader to rediscover it.
Claude-Session: https://claude.ai/code/session_019EmDd3Wy8b3cPG1TZBSzBu
* fix(web): settle the typed hold, and correct the premise this was built on (BUG-3039)
Codex round 1, enumeration-prompted. One finding falsified a premise the whole
unit rested on; five more were real.
**The premise was wrong (finding 8).** I reported that ItemDetail retargets
FieldEditor across an item switch. It does not: `ItemDetail.svelte:5425` wraps
the fields panel in `{#key itemSlug}`, and the comment above it (PLAN-2105 /
TASK-2112) says the sidebar "remounts on every item switch so a stale field
continuation is discarded" — the containment exists for exactly the hazard I
reported as open. I took it from FieldEditor's OWN comment, written at Codex
round 3, before PLAN-2105 added the key; I never checked whether the inherited
claim was still true, and the thing falsifying it was 35 lines above the tag.
That stale comment is now corrected in place, with what it cost recorded beside
it, along with every place this diff repeated it. The "second hole" this unit
reported — a retarget to a row with identical text — does not exist, and is
withdrawn rather than fixed.
`itemId` stays, demoted to what it actually is: not load-bearing, because the
pane remounts, but kept so the echo check does not silently become wrong for a
caller that does not. The component cannot see a `{#key}` in another file, which
is the same cross-component premise that just went stale underneath this work.
**A pin I had argued could not happen (finding 3).** I wrote "nothing is pinned
here" defending the design I had just written. False: a refused write leaves
`awaitingEcho` and `typedDisplay` set with no release, so rejected text sits on
screen over a server holding something else. That is R8-2's failure arriving in
the typed path, and it takes R8-2's fix — release on the consumer's SETTLEMENT
as well as on prop agreement. `updateField` is already `async`, so the signal
was there; the stamp is compared by object identity so an older write's
settlement cannot release a newer write's record.
Also fixed: ±1 stepping computed its base from the prop, which lags anything
sent but not echoed, so "type 5, then +1" sent oldValue+1 and lost the 5
(finding 1, pre-existing); an older echo released a newer send's display hold,
showing the earlier value until the newer one landed (findings 2 and 4); and
`typedDisplay` survived a field swap or a readonly flip, painting one field's
draft over another's value (finding 5).
Nineteen mutants, seventeen killed. Two survive deliberately and say so in the
source: `void itemId` (unreachable for two independent reasons, kept because the
logic depends on it) and one `typedDisplay = null` that states an invariant every
other path already maintains. A third survivor was neither — a `seeding` guard
nothing could kill, which is the tell that it decided nothing; deleted.
Two of my own test legs were wrong before they were right. The field-swap and
readonly legs changed `value` as well as the subject, so the ordinary drop branch
covered them and both mutants survived; the window only the subject check covers
is AFTER a flush, when the hold is live and nothing is pending. And a number leg
asserted a half-typed `1.` survives an unrelated prop change, on the premise that
nothing had been sent — `Number('1.')` is 1, so it schedules like any edit, and
the prop arriving as something else is a change the guard is supposed to win.
Claude-Session: https://claude.ai/code/session_019EmDd3Wy8b3cPG1TZBSzBu
|
||
|
xarmian
|
b3ca3a9691 |
fix(web): vitest refuses a project it cannot run instead of dropping it (BUG-3045) (#1346)
* fix(web): vitest refuses a project it cannot run instead of dropping it (BUG-3045) The web suite is three vitest projects. Two of them — `jsdom`, which owns every `*.svelte.test.ts`, and `idb` — were registered only if their devDependencies resolved, and simply OMITTED otherwise. So an incomplete install narrowed the run instead of failing it: `npm test` exited 0 having executed none of the rendered-component suites, and nothing in the output said so. That is a gate that passes when it cannot run — the shape scripts/ci-audit.mjs was taught to refuse in BUG-2881, where an unreachable advisory service was indistinguishable from a clean audit. Two halves, because narrowing has two causes and they deserve opposite answers: - An unresolvable dependency now REFUSES the run. The check sits at module scope in vitest.config.ts, so it lands while the config is still loading: vitest reports a startup error and exits 1 rather than falling back to a default config. The message names every unrunnable project and every missing package, so one `npm ci` fixes the whole set. - DELIBERATE narrowing is not refused — `--project` and file filters are legitimate — so a reporter prints how many of the registered projects actually ran files. A full run says `3/3 ran test files`; `--project node` says `1/1`; a single-file run says `1/3` and names the projects that ran nothing. A narrowed run no longer looks identical to a complete one. No escape hatch. All five packages are declared devDependencies, and CI does a full `npm ci`, so there is no legitimate call this refuses — only the state a green run must not be reported from. The decision and both messages live in src/test/vitestProjects.ts with direct tests. Fifteen mutants, all killed: thirteen by their own test leg, two by the config refusal itself (a mutant that breaks the decision for the REAL dependency set stops the suite before any test runs). There is deliberately no test that the package names resolve — it could never run, for exactly that reason. Claude-Session: https://claude.ai/code/session_019EmDd3Wy8b3cPG1TZBSzBu * fix(web): close the fall-through hole and prove the config uses the table (BUG-3045) Codex round 1, enumeration-prompted. Three findings acted on, one refuted. **A test file no project runs (finding 3).** The refusal covers a project that cannot run; it did nothing about a file that no project's glob matches. A `*.spec.ts`, `*.test.tsx` or `*.test.js` under src/ is never collected, never fails, and the run exits 0 — this bug's own class, one layer down. Codex checked the tree and found no member today, which is exactly when the guard is cheap: the first one to appear would otherwise arrive silently. `projectForTestFile` now answers which project owns a path, the three globs moved beside it so the config and the guard read ONE declaration rather than two copies, and a test walks src/ and names any orphan. It asserts its own precondition first — an empty file list would make every later assertion vacuous, which is the failure mode this whole bug is about. **The tests vouched for functions, not for the wiring (finding 8, CONVE-19).** Nothing would have noticed if vitest.config.ts had stopped calling any of this, gone back to probing dependencies inline, or dropped a project. Four tests now read the real config: it registers exactly node/idb/jsdom, that set equals `node` plus the requirements table, it keeps vitest's default reporter beside the project-count one, and each project routes with the shared globs. **Prose that overstated the refusal's reach (findings 1, 2, 4, 6).** The refusal is unconditional and precedes `--project`, so `vitest --project node` is refused too when the jsdom deps are missing, even though node alone would have run. Kept — an incomplete install is a broken state and stopping on it whatever subset was asked for is the honest answer — but the comment claiming deliberate narrowing is never refused was false in exactly the case the refusal fires, and now says so. Also written down: the line counts FILES, so it does not prove an assertion executed; an idle project is printed rather than failed; and `--reporter=<name>` replaces configured reporters, so `npm test -- --reporter=dot` prints no summary at all. **Refuted by measurement (finding 5).** Codex read the CLI source and concluded `context.projects` survives `--project` filtering, so a narrowed run would print `1/3` rather than the documented `1/1`. Running it says otherwise, twice: `--project node` prints `1/1 ran test files — node (70 files)` with no idle line, and `--project jsdom` prints `0/1` naming only jsdom. Under the claimed behaviour both would have carried the other two projects. No change made. Seven more mutants on the new legs, all killed, for 22/22 across the unit. N5 (drop the default reporter) is worth its own note: it exited 1 but my runner called it a runner error, because the mutation removes the very output lines the classifier reads. Re-run through `--reporter=default` on the CLI, it dies by its named test. Third instrument in this unit whose question depended on something the mutant controlled — the tsc gate and the kill-door grep were the first two. Claude-Session: https://claude.ai/code/session_019EmDd3Wy8b3cPG1TZBSzBu |
||
|
xarmian
|
80e32b4e0a |
fix(items): eleven hand-built doors carry the stale-body marker (BUG-3033) (#1344)
* docs: make test-pg is not safe against itself in one worktree (BUG-3033) CLAUDE.md said the target is safe across worktrees, which is true and incomplete. TEST_PG_PROJECT is basename + cksum of the absolute path, so two overlapping runs in ONE worktree share a compose project: the second `up -d --wait` attaches to the first run's container, and whichever run finishes first executes the unconditional `down -v` and destroys the other's database mid-test. Measured on the BUG-3000 trail, where the survivor's connection-refused failures were first read as evidence about the code. The note also records that a PASSING run carries ~41 redis connection-refused lines, so that noise is not the diagnostic — the failure text is. Claude-Session: https://claude.ai/code/session_01PDYjP67KTAcqpFpo3ijAAA * fix(items): six hand-built doors carry the stale-body marker (BUG-3033) BUG-3000 put content_state on models.Item, so every door that serialises the struct inherits it. These build their own shape and inherit nothing: - PlaybookRunResponse.Body (handlers_playbooks.go) - AgentBootstrapConvention.Content (handlers_bootstrap.go) - BootstrapIncludeItem.Content, bodies mode (handlers_bootstrap.go) - the artifact export's markdown+YAML (handlers_artifact_export.go) - the MCP item resource's composed markdown (mcp/resources.go) - `pad playbook show` / `pad playbook run`, which decode narrow structs of their own and print the body The last two are in the unit because an agent reads what the CLI prints, not the wire: a marker the server sets and the CLI drops is a fix its reader never sees — the same gap this bug describes for the MCP resource, whose fetcher reads stdout and cannot surface a successful command's stderr. BootstrapIncludeItem is a fifth server door the bug does not name, found by this unit's sweep and folded in under CONVE-18 rather than filed: same file, same handler, same shape as the conventions door beside it. The artifact export is a portable format, so the marker goes into the existing provenance block as an additive omitempty key with NO format_version bump. Decode uses a plain yaml.Unmarshal with no KnownFields, so an older binary ignores the key; format_version is compared with exact equality, so a bump would reject artifacts in both directions for an added optional key. A current item's bytes are unchanged. It signals rather than refuses; BUG-3032 owns the signal/refuse/accept decision for the bundle format, and an additive key forecloses neither of the other two. Every marker is emitted only where a real body is — the includes site sets it inside the same wantBodies guard that sets Content — so a metadata-only projection cannot claim its absent body is stale. Claude-Session: https://claude.ai/code/session_01PDYjP67KTAcqpFpo3ijAAA * test(items): both-directions coverage for the six BUG-3033 doors One test per door, both directions on one item, ABSENCE ASSERTED FIRST. The order is the instrument: a marker stamped unconditionally satisfies every positive assertion in these files, so a presence leg proves something only after the same door has been shown to stay silent about a current row. Assertions are on the raw wire form — a decoded map, the exported bytes, the rendered markdown — rather than on a Go struct, because a struct round-trip passes even when the field is never serialised. That is exactly the failure this bug is: a value computed correctly in the store and dropped by a projection on the way out. Four legs beyond the plain both-directions pair: - PlaybookShowResponse inherits the marker through an embed nobody tested. Pinned, because a refactor to an explicit projection — the shape BootstrapCollection and BootstrapRole already took for size — would drop it silently. - A metadata-mode include must carry NO marker even when its item is stale, since it serves no body; the premise (that the group really withheld the body) is asserted before the conclusion. - A current artifact must not merely lack the value but lack the KEY, and a marked one must differ by exactly one line and still decode with body and fields intact. Those are the compatibility claims made testable rather than asserted in a comment. - An UNRECOGNISED content_state must not render as this unit's specific claim about a live editor. The vocabulary is shared with BUG-2995's content_outcome and is expected to grow. The CLI and MCP legs drive the command and the resource rather than the helper and the formatter (CONVE-19): a direct call vouches for the function while leaving the narrow decode struct — where the defect actually lives — free to omit the key. Claude-Session: https://claude.ai/code/session_01PDYjP67KTAcqpFpo3ijAAA * docs: the read-side content_state marker (BUG-3033, CONVE-23 sweep) Three consumer-facing artifacts described the write-side warning and said nothing about its read-side counterpart, so an agent reading them would still conclude a stale read is silent. That was already true before this change — BUG-3000 shipped the marker without documenting it outside the field's own doc comment — and adding six more doors is what makes it worth closing rather than inheriting. - CLAUDE.md gains a read-responses bullet beside the three write warnings: what the value means, that it is emitted only where a real body is, that the window is UNBOUNDED because nothing server-side flushes, and that the correct response is re-read later, never re-send. - internal/mcp/instructions.md had a sentence saying a get before the flush "answers with the PREVIOUS content" — true and now incomplete, since the get says so itself. It also gains the case an agent cannot infer from its own write: someone else's tab holds the edits and you wrote nothing. pad_playbook's entry says a marked body may be superseded, and that this is a reason to re-read, not to refuse to run. - internal/mcp/version.go gains a POST-0.31 NO BUMP entry. The disposition is BUG-2304's, argued at length in the BUG-2995 entry above it: nothing is dropped or retyped, and a current item renders exactly as before. The item resource is the half worth naming — a text resource has no additive key, so the marker is a line of markdown, and a consumer diffing that text sees a new block when and only when the body it is about is stale. Claude-Session: https://claude.ai/code/session_01PDYjP67KTAcqpFpo3ijAAA * docs: contract comments name the marker they now emit (BUG-3033) formatItemAsMarkdown's doc listed the blocks it composes and this change added one; the artifact's compatibility claim now cites the instrument that proves it rather than asserting it (testdata/*.golden.md pin the encoded bytes exactly and were not touched to land the key). Claude-Session: https://claude.ai/code/session_01PDYjP67KTAcqpFpo3ijAAA * fix(cli): pad item edit warns before seeding $EDITOR from a stale body (BUG-3033) A seventh door, found by re-sweeping the population, and the only one whose consequence is not a confusing read. `pad item edit` is a read-modify-write over the WHOLE body: it seeds the editor from the row and PATCHes the entire edited text back with no concurrency token. When the row is behind the live collaborative document, the editor is seeded with content that predates edits which exist and are durable in the op-log, and the save replaces them with a version derived from a state before them — visible to neither the person editing nor the person whose tab holds them. Unlike an ordinary lost update this needs only ONE writer plus an open tab; the other side is the unflushed state of a document nobody is typing in. The warning is printed BEFORE the editor launches, which is the only point at which it can still change what the user does — afterwards it would be read beside an "Updated TASK-5" line. Its wording differs from its two siblings for the reason a third one exists at all: the consequence is that saving destroys something, not that the text is old. It warns and proceeds. Whether `edit` should refuse, prompt, or send a DIFF instead of the whole body is a behaviour decision with its own item (BUG-3035, which also asks whether `item update --stdin` joins any gate `edit` grows), not something to settle inside a sweep for missing markers. Two tests, and the second is the load-bearing one: the gating table vouches for the helper, and the end-to-end leg vouches for the BINDING — a fake editor announces itself on its own stderr, which OpenInEditor wires to os.Stderr, so the ordering claim is an assertion about one stream rather than about presence. Both mutants (drop the call; move it after the editor) are killed by that leg alone. Claude-Session: https://claude.ai/code/session_01PDYjP67KTAcqpFpo3ijAAA * fix(items): playbook summaries carry the marker; two tests stop passing wrongly Codex round 1, three findings, all valid. P2 — an eighth door, and this unit's population table missed it with the precedent already in the tree. A playbook `summary` is the body's first paragraph truncated, so a stale body makes a stale summary, and an agent ROUTES on that description. BUG-3000 put the marker on cli.ItemSummary.ContentPreview on exactly this reasoning ("a content_preview is content"); calling the same bytes a summary does not change where they came from. Carried on AgentBootstrapPlaybookMeta, which both the bootstrap blob and the /playbooks endpoint render, and through both CLI renderers — one stderr line naming the stale refs rather than a per-row marker, since a per-row marker on a long list is noise and a bare count sends the reader back to diff it by hand. Set only when a summary was actually emitted: no summary, no claim. It also does not mark the other metadata, which comes from the fields blob that the collaborative document does not hold. P3 — the edit tests accepted an always-on warning. They supplied only a stale response, so a call site forced to warn unconditionally passed them, and the helper's gating table cannot see a wiring error. Now both directions at the COMMAND boundary, with two premises asserted before any conclusion: the fake editor ran, and the edited body was really PATCHed. P3 — the artifact test accepted a corrupted body. It compared line counts and asserted only that the decoded body was non-empty, so an export that replaced the body with unrelated text passed. Now the EXACT body on both exports, plus byte equality after removing the marker line and normalising exported_at — with a check that the normalisation removed something, so the comparison cannot pass by stripping both sides. Claude-Session: https://claude.ai/code/session_01PDYjP67KTAcqpFpo3ijAAA * fix(items): backlink + search snippets carry the marker; add the class guard Codex round 2: two more doors and two test gaps, all valid. P2 — backlink snippets. models.Backlink.Snippet is a window cut from the SOURCE item's body, so a stale body makes a stale snippet. This unit's sweep could not see it: its vantage point was internal/server, internal/mcp and internal/cli, and this shape is built in the STORE. The predicate needed a non-default table alias (the backlink queries call items `s`), so contentStateSQL became contentStateSQLFor(alias) with the old constant as its "i" case — a function rather than a second copy, since two hand-maintained spellings of this predicate would be two chances to drift. The marker describes the SOURCE of each link, never the target the caller asked about, and a test asserts marking the target changes nothing. P2 — `pad item search` and `pad item backlinks` printed body-derived snippets and dropped the marker the wire already carried. Both now use one shared renderer with the playbook summaries, so the surfaces cannot word the same fact three ways. P3 — the edit test asserted only that a PATCH occurred, though it claimed to exercise saving; it now asserts the exact content that reached the server. P3 — the summary test had one stale row, which cannot tell a correct renderer from one naming every ref or only the first; it now interleaves stale and current entries, asserts the complete set and EXACTLY ONE warning line, and covers the bootstrap renderer that had no test at all. CONVE-35 — three rounds each found another member of this class, which means the reviews were measuring my enumeration rather than the code. So the table is now a test: every site turning an item body into text under another name is listed with its ruling, keyed on the exact call LINES with multiplicity, and anything else fails closed. Its first draft keyed on file+helper and a negative control caught the hole: a SECOND call site in an already-listed file passed silently — and handlers_bootstrap.go, which is listed, is exactly where a new projection would be written. Four controls now run: a new compiling call site goes red, a broken walk root goes red, a removed site goes red, and comments mentioning a helper stay green. The guard states what it cannot see — a door built on a helper it does not know — and its failure message asks for the helper, not just the site. Claude-Session: https://claude.ai/code/session_01PDYjP67KTAcqpFpo3ijAAA * fix(items): title-only search results drop the body marker; guard parses Go Codex round 3, six findings, all valid. P2, and a defect this unit INTRODUCED: Store.Search's direct-ref and numeric paths set Snippet = Item.Title and clear the body, but kept the BODY's content_state — so the new CLI renderer announced that a TITLE was derived from a stale body. Cleared at the source rather than patched in the renderer, which restores models.Item.ContentState's documented invariant and fixes every other consumer of those results at the same time. The FTS path beside it keeps the marker deliberately: there the snippet IS cut from the body, so the marker still qualifies text the result serves. A test asserts both directions, because "clear it everywhere" would delete the signal this unit exists for. P2 — the enumeration guard was itself fail-open. It passed a one-line function calling a helper, a helper assigned to a local and called through it, a comment inside the call expression, and an argument changed below the opening line of a multiline call, while FAILING on prose and strings that merely mentioned a helper. Wrong in both directions is the signature of a scanner not reading the language it claims to read, so it now parses Go: call expressions matched on the AST, where comments and line breaks are not facts. A helper NAME used as a value is reported too and fails closed, since the call through an alias is invisible. All four of codex's bypasses now go red and both false-positive cases stay green. P2 — FTSSnippet was missing from the helper list entirely, so four existing derivation sites were outside the table. Added and ruled; the stated boundary was understating itself and now says so. P2 — browser renderers (BacklinksPanel, CommandPalette) drop the marker. The wire and the TypeScript type now carry it; where a stale badge belongs in a panel or a palette is a UI decision, filed as BUG-3050 on the same grounds BUG-3033 was split from BUG-3000. P3 — cross-workspace backlinks had no coverage: clearing ContentState on every result of that query left the same-workspace test and the existing cross-workspace tests all green. P3 — the snippet tests had ONE stale row while their comment claimed interleaving, the same gap fixed for summaries one test earlier; a renderer reporting only the first stale source passed them. Both now use multiple stale rows separated by current ones, and that mutant dies on all four legs. Claude-Session: https://claude.ai/code/session_01PDYjP67KTAcqpFpo3ijAAA * fix(items): close the guard's helper-named bypass; seven tests stop passing wrongly Codex round 4, enumeration-prompted. No new Go door — its census of 434 non-test files found the two remaining Go omissions are already filed (workspace export on BUG-3032, cross-workspace copy attached to it) and the browser set belongs to BUG-3050. Everything below is in-class. The guard had a real bypass. A declaration whose own NAME was a helper got a special traversal that checked CallExprs and returned false, so the value-use arm never ran inside it: a compiling, callable `func (p ReviewProjection) PlaybookSummary() string { render := contentPreview; return render(p.Content) }` served unmarked stale prose while the guard passed, and renaming the method made it fail. The arm was redundant as well as wrong — notAValueUse already marks every FuncDecl.Name — so it is gone. It also skipped a `build` directory it never disclosed, which hid a real file at internal/server/build/. Three doc claims it made were false and are corrected rather than softened: "anywhere under internal/ or cmd/" (it skips four directories, now named), "helper values fail closed" (they did not), and that the browser surface is a pure rendering choice (the share paths do field copies that discard the marker before any rendering decision). It now states plainly that its claim is narrow — no UNRULED USE of four helpers — and must not be cited as proof that every door carries the marker. Seven mutations survived the tests, all the same species: they asserted the MARKER without asserting the BODY it qualifies. Replacing a served body with "WRONG BODY", dropping it entirely, or suppressing stale summaries and snippets while keeping the warnings all passed. Every door test now pins the exact text, and the backlink premise finally sees the snippet it is about — cli.Dim writes to color.Output, so an os.Stdout swap missed it and an earlier draft recorded that as a limit and asserted row headings instead, which is precisely what let that mutation live. Also: the numeric direct-ref path had no coverage (the test queried a prefixed ref only), backlink source-correlation passed when replaced with "any op-log row exists anywhere", and the FTS leg never asserted a snippet was produced. Search's surviving-marker comment claimed it always qualifies served text; an FTS title match on an empty-bodied item is the counter-example, and the comment now says so. Claude-Session: https://claude.ai/code/session_01PDYjP67KTAcqpFpo3ijAAA |
||
|
xarmian
|
5db34255f7 |
fix(web): a field value that is not a string no longer takes the view down (BUG-3041) (#1345)
* fix(web): a field value that is not a string no longer takes the view down
BUG-3041. `norm()` in `fieldColors.ts` did `value?.toLowerCase()`, and optional
chaining guards null and undefined — not a wrong TYPE. Hand it an array and
rendering threw `value?.toLowerCase is not a function`, taking the whole board
or table down rather than one chip.
It is reachable because a field's DECLARED type is not a promise about what is
STORED. `status` and `priority` are conventional key names, not reserved ones;
the schema editor will type any key however it likes; and nothing rewrites the
values already in the blob when a type changes. So a `status` that used to be a
`multi_select` can hold an array while its schema says `multi_relation` — the
same premise round 6 of TASK-2999 had to correct about `options`.
THE CLASS, grepped at the first instance rather than patched at the named one
(CONVE-18/CONVE-35). Three copies of ONE question: `fieldColors.norm`,
`TableView.selectValueColor` and `FieldEditor.getStatusColor` were the same
"canonical value to colour" function written out three times, each carrying its
own unguarded `.toLowerCase()`. Two of the three were reachable with a
non-string, and hardening them one at a time is how the third stays broken.
- The five exported helpers now take `unknown` and are total over it.
`formatFieldLabel` had NO guard at all, so it threw on null as readily as on
an array, and callers were compensating by hand with `?? ''` one site at a
time. Typed `unknown` rather than `string` deliberately: a `string` parameter
the compiler cannot enforce is a claim, not a check — every caller reads from
a `Record<string, any>` — and that claim is what hid this until it happened.
- `canonicalValueColor` is the single copy; TableView and FieldEditor call it.
THE CHIPS ASK THE VALUE'S SHAPE, NOT THE FIELD'S TYPE. `ItemCard` and
`TableView` gated their status/priority chips on truthiness, and a non-empty
array is truthy. They now require a string. Asking about the value rather than
the type is the point: a type allow-list goes stale the next time a type is
added, and this question does not. Nor is it cosmetic on the table — that chip
is CLICKABLE, and cycling it wrote a scalar into a list field, which the server
refuses.
Instruments, 26 tests across three files, every leg shown red against the
unfixed code:
- `fieldColors.test.ts` drives the whole JSON input domain — array, empty
array, number, boolean, object, null, undefined — rather than the one
non-string the report happened to name, and asserts each reads as "no
canonical value" rather than merely not throwing. Controls: strings resolve
exactly as before, and an unknown STRING ('shipped') stays distinguishable
from an unreadable value.
- The board leg renders the reported configuration and asserts the card is
drawn with no chip; its control keeps the ordinary string chip.
- `TableView.listValue.svelte.test.ts` is a new render harness for the table's
own copy of the defect, including the clickable-chip half.
Four mutants, each killed by its own leg: the unguarded `norm` (10 legs), each
chip gate reverted (one leg each), and both reverted together — the true
pre-fix tree, which reproduces the filed error text exactly.
Gates, each exit code read from a redirect rather than a piped summary line:
`svelte-check` 0 / 0 errors · `npm run test` 0, 2651 passed / 185 files ·
`npm run build` 0 · `check:tiptap-pins` 0 · `audit:ci` 0 · `make lint` 0 / 0
issues · `make test` 0, 0 FAIL / 30 ok · gofmt 0 files. `make test-pg` not owed
— no SQL changed.
BUG-3041
Claude-Session: https://claude.ai/code/session_019EmDd3Wy8b3cPG1TZBSzBu
* fix(web): the enumeration round's in-class findings, including one defect and one regression of mine
ONE enumeration-prompted codex round on the frozen tip. Category (b) — did the
consolidation change behaviour — came back CLEAN, with the comparison written
out. Categories (a) and (c) returned a large pre-existing class that is filed,
not fixed here. What lands here is the in-class remainder, and three of the
items are about this unit's own work.
A DEFECT IN THE CODE I JUST WROTE. `norm(value) in STATUS_COLORS` answers TRUE
for `__proto__`, and `STATUS_COLORS['constructor']` is a FUNCTION — so
`hasCanonicalStatus` said yes to a status the palette has never heard of, and
`statusColor` handed its callers an object or a function where its signature
promises a colour string. `?? MUTED` cannot catch either, because both are
truthy, and no non-JSON input is needed: these are plain strings a user can
type into a text field. Both lookups now use `Object.hasOwn`.
A REGRESSION I INTRODUCED. The first version of the table's gate required a
string outright, which silently removed the missing-status setter: with no
status stored, that chip renders empty and CLICKING IT sets the first option,
which is how a row gets its first status. `{}` and `{status:''}` began
behaving differently for no reason a user could see. Absent is admitted
alongside a string again — the non-string case, which is the actual defect,
stays gated.
A CLAIM I OVERSTATED. The previous commit says "three copies of ONE question".
There were TWO copies of the composite resolver (TableView, FieldEditor);
`norm` is a third unguarded lowercase but not a third copy of that function.
The comment in `fieldColors.ts` now says so, and says the first write-up got
it wrong.
FOUR OF MY OWN TEST LEGS PROVED LESS THAN THEY CLAIMED, which is the part
worth reading:
- A table leg supplied an array to a branch that has ALWAYS required a string,
so it measured the fallback text and PASSED AGAINST THE ORIGINAL
IMPLEMENTATION — codex restored the old TableView and watched it stay green.
Replaced with a string fixture that reaches the resolver, asserting the
resolver's answer, with a control for a word in neither vocabulary.
- The palette controls compared the implementation against itself
(`canonicalValueColor('critical') === priorityColor('critical')`), so muting
every priority colour left all 48 tests green. The colours are now written
out as literals.
- Deleting every ItemCard priority chip left the suite green: the control
covered status alone. It now covers both.
- "No chip" asserted the absence of a CSS class, which an inert span would
satisfy. The leg now clicks every control in the row and asserts no write
reached the handler, with a control proving the same click DOES cycle a
valid status.
One leg was also honest about what it cannot prove: the two vocabularies are
disjoint, so no fixture built from them distinguishes "statuses first" from
"priorities first". The leg pins the observable consequence and says where the
ordering question will land if they ever overlap.
Mutation matrix re-run: the four mutants codex reported as SURVIVING — muted
priorities, deleted priority chip, bare `in` lookup, string-only table gate —
now each die on their own leg. Nine mutants total, all killed.
Gates, each exit code read from a redirect rather than a piped summary line:
`svelte-check` 0 / 0 errors · `npm run test` 0, 2669 passed / 185 files ·
`npm run build` 0 · `check:tiptap-pins` 0 · `audit:ci` 0 · `make lint` 0 / 0
issues · `make test` 0, 0 FAIL / 30 ok · gofmt 0 files. `make test-pg` not owed
— no SQL changed.
BUG-3041
Claude-Session: https://claude.ai/code/session_019EmDd3Wy8b3cPG1TZBSzBu
|
||
|
xarmian
|
d5085bba00 |
feat(items): multi_relation — an ordered list of references (TASK-2999 / PLAN-2857 U4) (#1342)
* feat(items): multi_relation shape validation, DSL target, and rename propagation (TASK-2999 / PLAN-2857 U4, rows 1/17/18)
The first three rows of U4's population table, chosen because two of them are
the rows the task's own 5-row scope table did not contain and both fail
SILENTLY.
The table came first (posted on TASK-2999). A mechanical enumeration of every
site keyed on the literal "relation" finds 37 decision points in 21 files — 21
Go sites in 9 files, 16 web sites in 12 — against a scope table naming 5. By
CONVE-35 that is a measurement of the enumeration, not a longer to-do list, so
the deliverable under review is the table.
ROW 1 — internal/items/validate.go, `case "multi_relation"`.
Shape only; this package is DB-free, so whether an element names a live item
stays the store's question, exactly as for a scalar relation. Two rules
DIVERGE from the scalar case, ruled day 64 as NEW rules rather than inherited
ones: an empty or whitespace-only element is REFUSED (the scalar resolver skips
such a value), and `[]` is accepted as a shape while `required` is enforced
against RESOLVED elements elsewhere. Skipping is how scalar relation ended up
with three stored spellings for "no target"; inside an ORDERED array it would
also silently change the element count.
There was nothing coherent to inherit, which is BUG-3028, filed with line
references: a required scalar relation is satisfied by "", because
validateFieldType only asserts val.(string) and the resolver then skips the
empty value at relation_referents.go:232-235 and 1504-1507. Its fix is
convergence on U4's rule, deliberately ordered AFTER U4.
ROW 18 — internal/collections/dsl.go.
`multi_relation` joins the relation branch rather than falling to the Options
arm. Cardinality changes how many targets a VALUE names, never how many a FIELD
declares. Left out, `tags:multi_relation` parses happily into Options=["people"]
with no target — the exact unwritable field the v0.31 bump existed to stop the
DSL minting, one type later. Error messages now name the type the caller typed.
ROW 17 — internal/store/collections.go, retargetRelationsInSchemaJSON.
BUG-2873 rename propagation. A multi_relation declares its target the same way,
so a rename that skipped it left the field pointing at a slug that names
nothing: no error, the schema still parses, and every later write refused with
`target_missing` for a rename nobody would connect to it. This function had NO
test coverage before now (verified by grep across internal/**/*_test.go), and
it is the highest-consequence row in the table.
EVERY new test was run against the unfixed code and fails there:
* dsl.go reverted to `relation` only -> 3 of 4 new DSL tests FAIL
* the `case "multi_relation"` block deleted -> all 7 refusal legs FAIL
* the retarget condition reverted -> MovesBothRelationTypes FAILS,
naming the orphaned field
The accept-legs deliberately carry no weight on their own: an unknown field
type falls through validateFieldType with no check, so "accepts an array" stays
green against a tree that has never heard of multi_relation. The refusal legs
are the instrument, and the counterfactual above is what says so.
Three counterfactual legs guard the other direction, where a fix goes too far:
multi_select still takes its options list and gets no Collection; scalar
relation still accepts "" (BUG-3028's territory, not U4's to change here) and
still refuses an array; and a non-relation field carrying a `collection` key is
NOT retargeted, which would otherwise pass every assertion in that test.
Verified by reading rather than assumed: U5 already did U4's index half.
relation_links.go handles `case []any:` with ordinals and its schema matcher
already names multi_relation; migration 065/088 carries `ordinal` explicitly so
U4 needs no ALTER. Two rows there stay OPEN in the table with that stated — the
SQL backfill's array handling, and what a scalar->array retype reindexes.
Gates on this tip: make test 0 FAIL / 30 ok packages, make lint 0 issues,
gofmt clean. web/build generated in this worktree by `npx vite build` so the
Go gates embed this tree (CONVE-34 corollary); no web/src change in this
commit.
Claude-Session: https://claude.ai/code/session_019EmDd3Wy8b3cPG1TZBSzBu
* feat(store): multi_relation resolution by extracting the one relation ladder (TASK-2999 / PLAN-2857 U4, rows 2/14/15)
Rows 2, 14 and 15 of U4's population table. The shape of the change is an
EXTRACTION, not a second implementation: `ResolveRelationReferentsQ`'s
per-value UUID→ref→title ladder becomes `resolveRelationValueQ`, and both the
scalar path and the new `resolveMultiRelationField` call it.
One place knows the ladder, one place decides `VisibilityChecked`, one place
tells `ambiguous` from `wrong_collection` from `not_found`. That function holds
two separate rulings on `VisibilityChecked` — one of them a window deliberately
left to BUG-3012 — and a second copy of it would look correct while diverging
the moment either gained a case.
Rows 14 and 15, the two server doors, need no change of their own: they call
the store function and inherit the array handling. Stated rather than left
implicit, because "no diff" and "not covered" look identical in a table.
THREE DECISIONS, none of them visible from the code alone:
* ORDER AND LENGTH ARE PRESERVED. An unresolvable element stays VERBATIM in
the output; only resolved elements are rewritten to their canonical id.
Dropping one would make the stored length depend on which elements were
malformed, and the ratified model is an ordered list whose identity IS the
array — the same thing U5's `ordinal` column already commits to.
* NOTHING IS WRITTEN BACK UNLESS EVERY ELEMENT RESOLVED. The doors turn any
issue into a refusal, so a partially canonicalised blob is a write the
caller never asked for on a request that is about to fail.
* A FIELD WITH NO TARGET COLLECTION REPORTS ONCE FOR THE FIELD, not once per
element. Every element fails for the identical reason, and N copies bury
the one thing the caller can fix.
DUPLICATES get a new reason, `duplicate_referent`, with its `Message()` case
and its `RelationIssueReasons()` entry. Detected AFTER resolution, which is why
it cannot be a shape check: the duplicated elements are usually DIFFERENT
STRINGS naming one item — a UUID and a ref, a ref and an exact title — so
nothing that cannot resolve them can see the duplication. The issue names the
SECOND occurrence, the one the caller can remove. Refused rather than silently
de-duplicated: a caller who sent a target twice either erred or thinks
multiplicity means something, and storing one copy answers neither honestly.
THE PARITY GATE DID ITS JOB. `TestCopyPreflightDropReasonsAreRenderedByTheDialog`
went red on `duplicate_referent` the moment the constant existed, and stayed red
until the reason was in the TS type union, in `COPY_DROP_REASONS`, and in the
`MESSAGES` map. That is BUG-2674's failure — a reason shipping and rendering to
a user as a raw enum — prevented instead of repeated.
`copyDropReasons.ts`'s header says "The test cannot catch a reason added to Go
and not added here". True of THAT module's vitest test, and it reads as though
the direction is uncovered: I believed it and was part-way into building a
second instrument before finding the Go gate. A paragraph now names the gate.
MUTANTS — four, each killed by exactly the test that should kill it:
* multi_relation routing removed -> all four resolver tests FAIL
* duplicate check removed -> DuplicateAfterResolution only
* write-back made unconditional -> OneBadElementRefusesAndWritesNothing only
* output order reversed -> CanonicalisesEveryElementAndKeepsOrder only
`EmptyArrayIsNotAnIssue` survives all four. Recorded rather than counted: it is
a CONTROL against a resolver that invents an issue for `[]`, not a detector of
any of these.
The risk this commit carries is the extraction moving SCALAR behaviour, and the
U1/U6 suites are the instrument for it: full `internal/store` 0 failures, full
`internal/server` ok in 436s, 0 failures.
Gates: make test 0 FAIL / 30 ok packages · make lint 0 issues · gofmt clean ·
svelte-check 0 errors · copyDropReasons vitest 3 passed.
Claude-Session: https://claude.ai/code/session_019EmDd3Wy8b3cPG1TZBSzBu
* refactor(models): IsRelation / IsMultiRelation predicates, and the field-level sites that join them (TASK-2999 / PLAN-2857 U4, rows 8/12/13 + 16's predicate)
Rows 8, 12 and 13 of U4's population table, plus the instrument that makes the
table maintainable.
WHY A PREDICATE AT ALL. The contract for this unit found 37 decision points
keyed on the literal "relation" with NO constant behind any of them, so "which
code decides relation behaviour?" was a question only a string search could
answer — and the string-search answer is wrong the moment a second relation
type exists, which is what this unit adds. `FieldDef.IsRelation()` and
`FieldDef.IsMultiRelation()` make it a symbol search, and `RelationFieldTypes()`
makes the vocabulary enumerable rather than greppable.
The two predicates are deliberately different questions. FIELD-level code asks
IsRelation: does this field have a target collection, should a rename touch it,
is it worth hydrating. VALUE-walking code has to ask IsMultiRelation too,
because the shapes differ — and a single predicate answering both the same way
would make that distinction inexpressible.
WIDENED (field-level, body already shape-agnostic):
* relationTargetsUnavailable — availability is a property of the FIELD
* relationTargetSlug — returns the declared target; cardinality is
not part of that question
* relationSchemasForWorkspace — which schemas are worth hydrating at all
* RelationKeysPresent — presence only; it never reads the value
NOT WIDENED, and this is the more interesting half. `HydrateRelationTargetsQ`
keeps `def.Type != "relation"` with the reason written at the site. Its body
assumes ONE value per field — `perItem[item][key]` holds a single
`relationWant`, and `raw.(string)` skips an array silently — so widening the
predicate there would read as "multi handled" while hydrating nothing. I made
that widening, read the body, and reverted it.
Hydrating a `multi_relation` needs a WIRE-SHAPE decision first: whether
`relation_targets[key]` becomes a list or gains a parallel key. That is a v0.31
contract change, not a predicate edit, so it stays an OPEN row on the table
rather than being smuggled in as a one-liner.
The same trap is why `models.RelationFieldTypes`'s doc comment says explicitly
that the migration and default-resolution sites name their types by hand on
purpose: widening them with IsRelation alone would treat an array as a scalar.
TESTS. The predicates are pinned against every other field type this repo
ships, listed explicitly rather than derived so a new type forces a decision
instead of inheriting one, plus a leg asserting RelationFieldTypes agrees with
IsRelation — two expressions of one fact drift, and nothing else can notice.
Gates: make test 0 FAIL / 30 ok packages · make lint 0 issues · gofmt clean ·
full internal/store ok (157s), internal/items, internal/collections,
internal/models all ok.
Claude-Session: https://claude.ai/code/session_019EmDd3Wy8b3cPG1TZBSzBu
* feat(mcp): multi_relation hydration, migrate/default sites, and the v0.32 surface bump (TASK-2999 / PLAN-2857 U4, rows 4-7/9-11)
Closes the rest of U4's Go half. Rows 4-7 (MigrateRelationReferentsQ), 9-10 (the
two default-resolution passes) and 11 (read-side hydration), plus the surface
bump the read shape owes.
ROWS 4-7 AND 9-10 — the four migrate sites and the default passes asked
`_, isStr := raw.(string)`, which is the right question for exactly one of the
two types. Asked of a `multi_relation` it calls every legitimate array
malformed — and since those sites DELETE what they judge malformed, the answer
decides whether a valid value survives. Three small helpers now carry the
type-aware questions:
* relationValueShapeIsValid — string for `relation`, array of strings for
`multi_relation`
* relationValueIsCleared — the "no referent" form, which for an array is
`[]` or an array of blanks. The origin split skips a cleared value so it
lands in no bucket; without this an array the caller deliberately emptied
would be bucketed as a referent, resolve to nothing, and be reported as a
drop nobody could act on
* relationIssueValue — which value a FIELD-level issue names
`relationIssueValue` returns a SCALAR value UNTRIMMED, deliberately. The call
site it replaced read `raw.(string)` and reported it verbatim, and BUG-2870
settled that a caller's value bytes are carried as sent — so trimming would
quietly change what an existing scalar drop reports, a behaviour change
belonging to no row of this table. The multi case trims per element because it
is CHOOSING which element to name, not reporting one it was handed.
The cross-workspace carry now drops a `multi_relation` once for the whole field,
never per element, because the ruling is that the value drops WHOLE — so an
element count is not something the destination learns and an import cannot
change it.
ROW 11 — hydration, on the lead's ruling: `relation_targets[key]` carries EITHER
the scalar object v0.31 emitted OR a JSON ARRAY of those objects in stored order.
ONE map, not a parallel key: a consumer rendering "what does this field point
at?" should not have to ask two questions and merge them, and the field's type
already says which shape to expect.
`models.RelationTargetSet` holds either shape with a custom marshaller. THE
COMPATIBILITY CLAIM IS THAT A SCALAR ENTRY IS BYTE-IDENTICAL, and the test pins
it against a LITERAL rather than against another call into this code — comparing
it to itself would pass against any self-consistent change, including one that
renamed every key. It unmarshals both shapes too, because the CLI decodes item
JSON the server produced, and a marshaller without a matching unmarshaller is how
a shape change reaches a consumer as a decode error instead of as data.
Position is preserved: every stored element gets an entry, including one that
names nothing, which hydrates ID-only — the same honest shape a dangling scalar
gets. Dropping it would silently shorten the list for exactly the legacy rows
that need explaining, and a consumer could no longer line the list up against
`fields`. Visibility redaction is element-wise: a list with one visible and one
invisible target redacts only the second, since redacting the whole list
withholds what the caller may see and redacting none discloses what they may not.
An EMPTY list marshals as `[]`, distinct from the key being absent: absent means
"nothing hydrated this field", `[]` means "this field holds no references".
v0.32, AND A VERSION COLLISION STATED RATHER THAN RESOLVED SILENTLY. PR #1337
also claims 0.32 and was opened first; `gh pr view 1337` says OPEN and main is
still 0.31, and the standing instruction is to take 0.33 only once it HAS merged.
So this takes 0.32 and flags the contest: whichever merges SECOND owes a
renumber, in the constant and in the entry. 0.33 is free if the reviewer would
rather not have the contest at all.
The bump is owed by the read shape, not by the type. Every value rule here is
reachable only through a field whose schema says `multi_relation`, which no
existing schema can contain — but a strictly-typed client parsing
`relation_targets` will fail to decode an array where it expects an object, the
moment its workspace acquires such a field.
DOCS SWEPT, all three consumers: `internal/mcp/instructions.md` (the agent-facing
contract, with the four rules that are NOT guessable from scalar behaviour),
`README.md` (heading, `tool_surface_version`, changelog sentence), and CLAUDE.md —
which the 0.27 unit missed while sweeping the other two, and which says so in its
own text.
Test adaptations: `scalarHydrated` / `scalarTarget` helpers in the store and
server suites unwrap a scalar entry and FAIL on a list rather than returning a
zero value, because a scalar field hydrating as a list is precisely the
regression this type makes possible and a quiet zero would surface it as a
confusing assertion two lines later.
Gates: make test 0 FAIL / 30 ok packages · make lint 0 issues (it caught an
unformatted file in internal/cli that would otherwise have been pushed) ·
internal/mcp drift gates green on all three documents.
Claude-Session: https://claude.ai/code/session_019EmDd3Wy8b3cPG1TZBSzBu
* test(store): pin the reverse index's array handling, and renumber to v0.33 (TASK-2999 / PLAN-2857 U4, rows 20-22)
Closes the last three rows of U4's Go half — all three as QUESTIONS rather than
work, and the answer in each case is that U5 already built it right. That is not
a reason to leave them unpinned: reading a `case []any:` arm is not the same as
knowing the index holds the rows, and the whole point of the population table is
that a row's state is recorded rather than assumed.
ROW 20 — a `multi_relation` write indexes one row per element with the array
position in `ordinal`. The control is the part worth having: reversing the stored
array must reverse the ordinals, so an extractor emitting a constant 0 cannot
pass by the fixture happening to be sorted its way.
ROW 21 — the BACKFILL covers arrays, not just the write hook. Worth separating
because the two paths reach the index differently (per-write vs every live item
from a clean table) and because migration 065's own comment says a derived query
"on a multi_relation array silently matches nothing", which is easy to misread as
being about this path. It is not — the backfill reaches the same Go extractor via
`relationKeysFromSchemaJSON`, which has matched `multi_relation` since U5.
The test clears the rows by hand AND clears the once-only
`platform_settings` marker first. Both are preconditions without which it proves
nothing: the write hook has already populated those rows, so a backfill that did
nothing at all would pass, and a backfill that reported `Skipped` would too. It
asserts `!res.Skipped` for the same reason.
ROW 22 — a scalar->array RETYPE reindexes against what is STORED, not against
what the new schema says the shape should be. The ruling called a garbage retype
a U4 defect; it is not one, and this records WHY rather than leaving the row open.
The extractor is VALUE-shaped — it switches on the JSON type it finds — so after
a retype the still-scalar blob indexes as one row at ordinal 0, which honestly
describes what the item holds. Indexing by the DECLARED cardinality instead would
invent rows the blob does not contain.
MUTANT: disabling the extractor's `case []any:` arm kills rows 20 and 21. The
retype test SURVIVES it, correctly — its blob is a scalar, so that arm is not on
its path. Stated rather than counted, because a surviving mutant is worth
explaining either way.
VERSION RENUMBERED TO 0.33 on a lead ruling, in all four documents (version.go,
instructions.md, README.md, CLAUDE.md). 0.32 is now RESERVED for PR #1337, which
claims it and was opened first. The reasoning is in the changelog entry: version
numbers are not scarce, and two branches claiming one means whichever merges
second renumbers under merge pressure — which is exactly when a version bump is
least likely to be checked. A gap in the sequence is cheaper than that, so 0.32
may simply be skipped if #1337 is ever abandoned.
Gates: make test 0 FAIL / 30 ok packages · make lint 0 issues · gofmt clean ·
internal/mcp drift gates green on all three versioned documents.
Claude-Session: https://claude.ai/code/session_019EmDd3Wy8b3cPG1TZBSzBu
* feat(web): the type union, the schema editor, and the five field-level web sites (TASK-2999 / PLAN-2857 U4, web rows 1-5 + 11-15)
First batch of U4's web half: the sites where the right answer is "this field
points at other items, cardinality is irrelevant". Value-shaped surfaces — the
item-pane picker, the filter bar, board/list grouping — are NOT in this commit,
because each needs a decision about what a multi-valued answer means and a
predicate widening would paper over that.
`$lib/items/relationFieldTypes.ts` mirrors the Go predicates with the same split:
`isRelationType` for field-level questions, `isMultiRelationType` for value-level
ones, `isResolvableRelation` for the type+target pairing that appears at five
sites. Placed in `lib/items/` rather than under `components/` because
non-component code (`shareView`, `copyNeedsValue`) asks the same questions, and a
predicate living under `components/` invites a second copy for everything that
is not one.
SITES, and what each would have done left alone:
* `types/index.ts` — the FieldDef union gains `multi_relation`. Without it
every other change here is a type error.
* `field-editor-types.ts` FIELD_TYPES — the schema editor can now DECLARE one.
A type the API accepts and the editor cannot offer is a half-shipped
surface: the only route in would be the CLI's DSL or a hand-written schema,
which is a gap with a workaround, not a feature.
* Create/EditCollectionModal (3 sites) — these preserve `def.collection` only
when the type matched. Left alone, a `multi_relation` saved with NO target,
and every later write to it was refused with `target_missing` — silently
from the user's side, since the editor had shown them the target they
picked.
* `shareView.isPublicGroupable` — a public share refuses to group by a
relation because the payload carries values with no index behind them. A
`multi_relation` is strictly worse, each value being an ARRAY of ids, so a
shared view grouped by one would render a group per stringified array.
* `copyNeedsValue` (2 sites) — the copy dialog's collectable/uncollectable
split, including IDEA-2899's distinction between naming NO target and naming
an unavailable one, which had to survive the widening.
* `[collection]/+page.svelte` (2 sites) — the relation-keys set, and the
DESTRUCTIVE lane-order guard that would otherwise write item ids into the
schema as `options`.
TESTS are at two levels on purpose. The predicate is pinned against every other
field type the app ships, listed explicitly so a new type forces a decision. The
SITES are then asserted through their own functions, because a test on the
predicate proves nothing about whether a given site calls it (CONVE-19) — each
with the scalar control that would catch a widening gone too far: `multi_select`
still groupable on a share, a non-relation copy row unaffected, FIELD_TYPES
still carrying what it had.
Counterfactual, run: reverting all three widenings turns 4 of the 8 wiring legs
red. The other 4 are controls and correctly survive.
ONE TEST REPOINTED, and it is worth reading. TASK-2998's source guard on
`handleGroupReorder` read for the literal `type === 'relation'`. Replacing that
with the predicate made the guard STRONGER — it now covers both types — and the
test went red for a spelling change. It now reads for the predicate CALL, plus a
leg requiring the guard to test the REORDERED field (`s.fields[idx].type`) rather
than merely calling the predicate about something. A source guard reading for a
spelling fails whenever the code improves; that is the cost of the technique, and
the comment now says so at the site.
Gates: web 2546 passed / 180 files (2545 before, +1 new file) · svelte-check 0
errors · vite build clean.
Claude-Session: https://claude.ai/code/session_019EmDd3Wy8b3cPG1TZBSzBu
* feat(web): filtering a multi_relation is MEMBERSHIP (TASK-2999 / PLAN-2857 U4, web rows 8 + 12)
U7 deferred this to U4 in as many words, in both the function's doc comment and a
test, rather than guess: a `multi_relation` stores an ARRAY and no amount of
trimming makes `===` match one, so equality would have returned EMPTY for every
filter, forever, and silently.
Defined now as MEMBERSHIP: an item matches when its list CONTAINS the target. It
is the only reading of "filter by this target" that is true of a list.
Membership, not POSITION. An item whose second reference is Red matches a filter
for Red. Order is part of the stored value — it round-trips, and the reverse index
keeps it in `ordinal` — but it is not part of this question.
Elements are trimmed individually, on the scalar case's own reasoning: the write
path has refused padded values since TASK-2878, so there is nothing to migrate,
only old rows to read correctly. A non-string element is ignored rather than
thrown on; a corrupt blob must not take out the whole view, and an element that
is not a reference cannot match anyway.
`FilterBar` now offers BOTH relation types. Its picker needs no change — it still
returns ONE target id; what differs is the question asked of the stored value.
A U7 TEST WAS REWRITTEN, not deleted. `refuses a non-string, including the
multi_relation ARRAY shape` asserted that an array matched nothing, and explained
that the deferral was deliberate. That assertion now states the OPPOSITE of the
shipped behaviour. It keeps its non-string legs under an honest name, and the
comment records that U4 closed the deferral — the deferral was real and the record
of it belongs somewhere, which is the U4 block the array legs moved into. This is
the shape where a prior unit's prose asserts the absence of the thing you just
built; the prose has to move when the absence does.
Counterfactual, run: removing the membership arm turns 3 of the 6 new legs red.
The other three are the scalar control and the negative cases, which correctly
survive — a change that only ever looked at arrays would pass everything else and
break every existing relation filter, which is what that control is for.
NOT in this commit, and it needs a ruling rather than a guess: GROUPING by a
`multi_relation`. The same "membership" answer would put one item in N lanes,
which breaks the invariant U7 worked to preserve — `bucketByColumn` puts every
item in EXACTLY ONE lane, and U7's proving row asserts it by placing six items and
counting. So grouping is either a different rule (an item appears once per
target, which is a rendering change, not a predicate widening) or a refusal like
the one public shares already give. Raised on the trail; BoardView and ListView
are untouched meanwhile.
Gates: web 2551 passed / 180 files · svelte-check 0 errors · vite build clean.
Claude-Session: https://claude.ai/code/session_019EmDd3Wy8b3cPG1TZBSzBu
* feat(web): grouping by a multi_relation is REFUSED, with the reason said out loud (TASK-2999 / PLAN-2857 U4, web rows 6 + 9 + 10)
Lead ruling, option (2) of the three I put up: refuse, same disposition a public
share already gives relation grouping, and never fall back silently.
WHY REFUSE rather than render. `bucketByColumn` puts every item in EXACTLY ONE
lane, and U7's proving row asserts that by placing six items and counting. An
item with three references belongs to three. The alternatives were rendering the
item once per target — which leaves a drag between lanes meaning something
undefined (add? move? replace?) — or grouping by the FIRST element, which
silently answers a different question than the user asked. The multi-lane
rendering is IDEA-3034 if it is ever wanted.
`relationGroupingRefusal` returns two distinct reasons, and the distinction is
the point: `no_target` is fixable by editing the schema, `multi_valued` is not.
One message for both would make a distinction the type draws invisible to the
person reading it, so a test asserts the two sentences DIFFER.
THE "NEVER SILENTLY" HALF IS MOST OF THE WORK. Before this, a `multi_relation`
group field fell through both views' scalar predicate, was treated as an ordinary
field, and — having no `options` — put every item in UNCATEGORIZED. An
Uncategorized lane with no explanation reads as "none of these items has a
value", which is false and unactionable.
* ListView already had the mechanism: `relationWithoutTarget` drops it to
ungrouped. That flag now derives from the refusal, so it covers both reasons
instead of one, and a notice renders above the list.
* BoardView cannot be "ungrouped" — a board IS lanes — so its fallback stays
the single lane it already produced. What changes is that it says why.
The notice is plain and inline rather than a dismissible toast, deliberately: it
describes a standing property of the view's CONFIGURATION, not an event, so it
has to still be there after a reload.
ROW 6 also lands here: the schema editor's per-field "Relates to" picker now
shows for both relation types. FIELD_TYPES already offered `multi_relation` in
the type dropdown, so without this the editor let a user pick the type and then
gave them no way to say what it points at — every field declared through the UI
would have been refused on its first write with `target_missing`.
ONE THING WORTH RECORDING that needed no change: the board/list GROUP-BY PICKER
(`DisplaySettingsEditor`, fed by `selectFieldKeys`) only ever offered `select` and
`multi_select` fields. It never offered relation fields at all, so "the picker
does not offer a multi_relation" was already true by construction, and U7's
relation grouping is reachable only through a saved view or a hand-set
`board_group_by`. Verified rather than assumed — I went looking for the picker to
change it and found it already correct, which is the more useful half of the
ruling to have checked.
Counterfactual, run: removing the `multi_valued` arm turns exactly the leg that
should go red, red. Two control legs — a relation WITH a target still groups,
every ordinary type still groups — guard the opposite failure, which would
silently un-group every relation board U7 shipped.
Gates: web 2556 passed / 180 files · svelte-check 0 errors · vite build clean.
Claude-Session: https://claude.ai/code/session_019EmDd3Wy8b3cPG1TZBSzBu
* feat(web): the item-pane picker edits a multi_relation (TASK-2999 / PLAN-2857 U4, web row W7)
W7 is the last of the 29 population rows and the one the rest of U4's web half
exists to make usable: a type the schema editor can declare but no item pane can
EDIT is still half-shipped.
A PARAMETERISATION, not a widening. The four resolution steps — resolve the row,
name its state, format its ref, build its href — were $derived values closing
over the single scalar `value`. They are now functions of ONE raw reference, and
the scalar path calls them with its single element, so there is one
implementation rather than a scalar one and a multi copy that drift. The chip
snippet takes the reference the same way. `relationValues` is the list both types
render from: one element for a `relation`, N for a `multi_relation`, none when
empty.
The 40 existing scalar tests are the instrument that this moved nothing; they
were not touched. Negative control run before trusting them: forcing
`relationHrefFor` to null turns one red, so they do read the parameterised path.
What the list behaviour is, all of it ruled in U4 and none re-litigated here:
* ORDERED. Chips render in stored order, a choice APPENDS to the end, and
Remove drops by POSITION — the user pointed at a row, not at an id.
* `[]` is the one spelling of none. The last Remove writes the empty array, not
the scalar's `''` (which this type's validator refuses); normalising it to an
absent key is the write door's job and stays there.
* DUPLICATES are prevented by not offering them: the picker is handed the
field's own elements to hide. The exclusion set is the RAW stored elements
because the write door canonicalises every value to its target's UUID, so a
stored element IS the id — which also covers the element `localIndex` cannot
resolve, the one whose target the cold picker can still hand back from the
server.
A write-side duplicate guard was written and removed. It compared against the
same set the picker is given, so nothing the picker could offer could reach it
and no mutant could kill it — an unfalsifiable branch reading as rigour. The
server refuses a duplicate outright (`RelationTargetDuplicate`) rather than
de-duplicating, so the backstop behind the exclusion is a refusal, not a silent
one-copy write. A resolved-id half of the exclusion set went the same way for the
same reason: under canonicalisation it is the same set, and the mutant that
narrowed it away survived.
Affordances are per-element (Remove) plus one for the list (+ Add). The scalar's
Change/Clear pair does not translate — "Change" would have to mean "replace
everything". The picker's open/closed rule IS the scalar's, unchanged: open when
the field is empty or when asked for, closed once a choice lands.
Instrument: 22 new tests in `FieldEditor.multiRelation.svelte.test.ts`. Run
against the PARENT COMMIT's FieldEditor: 21 fail, and the one that passes is the
scalar CONTROL, which is what a control is for. That run is also why one leg
grew an assertion — "an empty field opens the picker with no Add button" passed
unfixed, because an empty value used to fall into the SCALAR branch, which also
opens a picker and also has no Add button. It now asserts the SHAPE the picker
writes, which is what tells the two branches apart.
Mutation matrix, each mutant compiled and each killed by the test that should
kill it:
| mutant | died in |
|---|---|
| append -> replace | APPENDS + inline-create-appends |
| last Remove writes `''` | the `[]` leg only |
| `excludeIds` dropped | all three duplicate legs |
| element order sorted | the order leg + 5 others |
| blank/non-string elements kept | the dropped-elements leg only |
| chip handler ignores its own row | the pane-open leg only |
| non-array value read as one reference | that leg only |
| empty list no longer auto-opens the picker | both picker-visibility legs |
| multi falls through to the scalar branch | 10 legs |
Two mutants SURVIVED a first pass and are the reason two things above are
smaller than they were: the blanks filter (the titles assertion could not see
extra EMPTY elements — the test now counts rendered units) and the write-side
duplicate guard (unreachable, removed). Both are recorded rather than patched
around.
TASK-2999 / PLAN-2857 U4
Claude-Session: https://claude.ai/code/session_019EmDd3Wy8b3cPG1TZBSzBu
* test(server): the Q2 round-trip proving test for multi_relation (TASK-2999 / PLAN-2857 U4)
The design row asks for one property end to end: write a `multi_relation` value
as a list of REFS through the write door, read it back hydrated, export the
workspace and re-import it, and assert the stored array is identical and every
element still resolves. Negative leg: one unresolvable element refuses the WHOLE
write, naming that element, never a partial store.
AT THE DOOR, not in internal/store, and that is the point of the row. Every
component underneath already has direct tests — the resolver's canonicalisation
and ordering, the hydrator's list shape, the export remap. Those vouch for the
components and say nothing about whether the doors are BOUND to them: a create
handler that never reaches the resolver, or an import that never rewrites an id
inside an array, passes every one of them. A round trip is a claim about the
seams, which is exactly what no component test can make.
The export/import half is the VERIFICATION the design asked for rather than a
rebuild: `remapFieldIDs` substitutes quoted JSON tokens, so it reaches ids inside
arrays without knowing the schema. Nothing was added there. This is the test that
would go red if someone replaced that substitution with a scalar-shaped walk.
Three targets, not two, and supplied in an order that is neither sorted nor
reverse-sorted by id, ref or title — with two elements a reversed array and a
sorted one are the same observation, and "order preserved" would be satisfied by
a resolver that sorted.
Mutants, each compiled, each killed by the test that should kill it:
| mutant | died in |
|---|---|
| export remap skips values containing an array | the round-trip test |
| resolved array reversed before write-back | the round-trip test |
| a multi_relation hydrates as a SCALAR (first element) | the round-trip test |
| an unresolvable element reported as no issue | the refusal test |
Stated rather than counted, because the difference matters: the "no item was
created" assertion in the refusal leg is a REGRESSION GUARD, not evidence. The
refusal happens before any insert today, so no mutant short of reordering the
handler turns it red and I did not build one. It guards a future
create-then-validate ordering, which the 400 alone would not distinguish. The
comment at the site says so.
The third test is a CONTROL: a door that refused EVERY array satisfies both
assertions of the refusal leg, and this file would then read as proof of a
working feature.
Gates: `make lint` 0 issues · gofmt clean · `internal/server` ok (364s) ·
`internal/store` ok (165s) · `internal/items` ok. All on SQLite — `testServer`
calls `storetest.NewSQLite` unconditionally, so nothing built on it is
Postgres-backed regardless of which make target runs it.
TASK-2999 / PLAN-2857 U4
Claude-Session: https://claude.ai/code/session_019EmDd3Wy8b3cPG1TZBSzBu
* fix(items): every site that dispatches on field type now knows multi_relation (TASK-2999 / PLAN-2857 U4)
Codex round 1 on the U4 tip returned five findings. All five were real, and
enumerating the class they belong to found NINE. They are one defect wearing
nine costumes: a site that dispatches on FIELD TYPE and was never extended for
the new one. I built the WEB population table for exactly this reason and never
built the GO one.
The instrument is two greps, and the second is load-bearing:
`grep -rn '"relation"' --include=*.go internal/ cmd/` finds 13 sites, but two of
the gaps live in switches that enumerate field TYPES and never mention
`relation` at all, so a relation-literal sweep cannot see them. The class is
"dispatches on field type", not "mentions relation".
## The nine
**Visibility (the P1).** A `multi_relation` element was never visibility-checked.
The store resolver takes `canSee`, but only its TITLE rung consulted it; the
UUID and REF rungs were judged by the SERVER's post-resolution pass, which is
gated on the field type. So an editor with no access to the target collection
could put a hidden item's ref inside an array and receive 201 with its canonical
UUID — the existence oracle U1 exists to close, reopened by the array door.
Per the lead's ruling the check moved INTO the shared per-element resolver,
where both types already resolve, rather than being duplicated in the server
pass — gating a security check on a type was the defect, so the fix is one site
and not two. Order matters there and has its own test: the visibility answer
comes BEFORE the wrong-collection answer, because that reason's message names a
live item and would disclose existence first. `VisibilityChecked` is now set on
both arms, which supersedes the note saying the server re-resolves a ref-derived
`wrong_collection`; that also closes BUG-3012's TOCTOU window for values
reaching this rung, since there is no longer a second lookup to race.
**The empty list** (lead ruling, P17). `validate.go` said normalising `[]` to an
absent key was "the write door's job". No door did it. It now happens at the one
entry every write passes through, in the two spellings the two write shapes
need: a FULL write's `[]` becomes an absent key, and the ordinary required
machinery refuses it for free; a PARTIAL write's `[]` becomes the patch path's
nil deletion sentinel, because deleting the key from a patch would turn "empty
this field" into "do nothing" while reporting success.
**Required** was enforced for scalars only, so a required `multi_relation` had
no enforcement anywhere: the key is deleted after validation has passed, and
nothing re-checks it.
**Defaults**, twice. `ValidateFields` assigns a schema default and skips its own
type check, so the late pass is the ONLY thing between an injected default and
the blob — and it skipped arrays, so a schema defaulting to `42` stored 42, and
one defaulting to refs stored refs. The invisible-default drop skipped them
twice over: once on the type gate and again on a `.(string)` that fails on an
array, which is an accident rather than a decision.
**The CLI could not write the type at all.** Both coercion sites JSON-parse only
`json` and `multi_select`, so `--field 'members=["PEOPL-1"]'` arrived as a raw
string and the validator refused a shape the caller never sent.
**The copy preflight** reported `collection_unavailable: false` for a
multi_relation aimed at a deleted collection, so the dialog offered a picker it
could never fill and explained nothing. Codex did not name this one.
**A refused board ate every drag.** `isRelationGroup` is `type === 'relation'`,
so a multi_relation board took the scalar arm: `currentValue` is the stored
ARRAY, `targetColumn` a lane string, never equal, so every drop fired a write
that would replace the list with a scalar. The server refuses it and the reorder
is silently dropped — while the refusal notice rendered correctly the whole
time, which is what hid it.
## Instruments
25 new tests across four packages, each fix with its own. Mutation matrix, one
mutant per fix, every mutant compiled, every one KILLED by the test that should
kill it:
| mutant | died in |
|---|---|
| visibility block removed | all three visibility legs |
| visibility moved AFTER the collection check | the seen-target control only |
| required gate reverted | the required leg only |
| full-write normalisation removed | both full-write legs |
| partial spelling made a delete | both partial legs |
| late-default gate reverted | all four default legs |
| invisible-default multi branch removed | that leg only |
| preflight gate reverted | that leg only |
| coerce case reverted | the coercion leg |
| CLI case reverted | the CLI leg |
| board refusal gate removed | the board guard |
Controls throughout, because most of these legs assert a refusal and a fix that
refused EVERYTHING would satisfy them: a visible element still resolves, a nil
predicate still means "not asked" rather than "deny", a `wrong_collection` a
caller may see keeps its specific message, a key present before validation is
still the caller's value and not a default, an empty `multi_select` is untouched,
and a scalar relation still takes a bare string at both coercion sites.
Prose swept where this falsified it (CONVE-23): validate.go's arm no longer
claims a door does the normalising, `ValidateFieldsDetailed`'s doc now says it
runs one mutation before its traversal, and `cmd_item.go`'s "all other types …
relation — string is correct" no longer names relation flatly.
## Gates
`make test` 0 FAIL / 30 ok packages · `make lint` 0 issues · gofmt clean ·
`svelte-check` 0 errors · web vitest 2580 passed · `vite build` clean, and
`web/build` regenerated so the Go gates embed this tree. `make test-pg` NOT run
for this commit: no SQL changed, only Go-side type dispatch.
TASK-2999 / PLAN-2857 U4
Claude-Session: https://claude.ai/code/session_019EmDd3Wy8b3cPG1TZBSzBu
* fix(store): three defects in the previous commit's own fixes (TASK-2999 / PLAN-2857 U4, codex round 2)
Round 2 found three, and all three were in round 1's CORRECTIONS rather than in
the original code. That is the shape worth naming: I was reviewing the defect
and trusting the fix.
**A lenient element reader.** My `relationDefaultList` mapped a non-string
element to `""` on the reasoning that one bad element should not make the value
un-listable — and the resolver SKIPS a blank element, so `default: [42]` was
accepted, stored as `[""]`, and reported to nobody. A list is ONE value: an
element nobody can resolve makes the default unusable, exactly as a non-array
does. It is now strict about both non-string and blank elements, which is the
same shape rule `ValidateFields` applies to a caller's array, differing only in
disposition (dropped, not refused).
**An empty default on a required field, deleted in silence.** Validation had
just been satisfied by the PRESENCE of that default; deleting it afterwards with
no issue left a required field absent with nothing to refuse it.
`RequiredRelationIssues` promotes by key, so an issue is the only way a door
hears about it. Reported only when the field is REQUIRED: on an optional one an
empty default is a sensible way to say "starts with nothing", and a warning
there would be a false alarm on a schema doing nothing wrong.
**The order bug, and the sharpest of the three.** Every door captures relation
provenance with `RelationKeysPresent` BEFORE `ValidateFields` runs, so that
function saw the `[]` the new normalisation was about to remove. Counting it as
caller input marked the key "not a default"; validation then injected the schema
default into the hole normalisation had made; and the late-default pass skipped
the key on the strength of that mark. A caller sending `members: []` against
`default: 42` stored 42 — a value nobody sent, through the seam between two
correct-looking passes. `RelationKeysPresent` now reads an empty list as absent,
by the same rule, which is the one-site fix: the key is then a DEFAULT to every
later pass, which is exactly what it became.
Instruments: 5 new tests. Mutants, each killed by the test that should kill it —
lenient elements, blanks tolerated, the required report removed, the empty-list
rule reverted, and the empty-list rule LEAKING onto the scalar type (whose empty
spelling has its own meaning, BUG-3028).
One mutant SURVIVED a first pass and is recorded rather than quietly re-rolled:
"map a non-string element to empty string" is not a faithful negation of the
non-string rule, because its own output then trips the BLANK guard two lines
later and dies for the wrong reason. The faithful mutant stringifies instead —
`42` becomes `"42"`, which is neither blank nor resolvable — and that one is
killed by the non-string leg alone.
Gates: `make test` 0 FAIL / 30 ok · `make lint` 0 issues · gofmt clean.
TASK-2999 / PLAN-2857 U4
Claude-Session: https://claude.ai/code/session_019EmDd3Wy8b3cPG1TZBSzBu
* fix(store): three more seams the empty-list rule opened (TASK-2999 / PLAN-2857 U4, codex round 3)
Two of round 3's three were again in the previous round's fixes, and they are
the same class as round 1's: a SECOND path that does not share the rule. The
lead's ruling put the empty-list normalisation at the one validation entry every
write passes through; these are the passes that run BESIDE it and had their own
idea of what a value is.
**A cleared list survived a move.** `MigrateRelationReferents` SKIPPED a cleared
relation value so it would be neither refused nor reported — correct for a
scalar `""`, which validation accepts and which is a legal stored spelling. For
a LIST it is not enough: the migrate doors validate AFTER resolving, so an array
of blanks stayed in the map and failed the shape check. Reachable as a
destination-schema DEFAULT, since `ValidateFields` assigns one without
type-checking it — so the same schema default returned 400 on an ordinary move
and was quietly dropped on a create. The list is now DELETED there; the scalar
is untouched, and a control pins that asymmetry.
**The bulk door resolved a default as though it were typed.** `applyBulkOp`
snapshots relation provenance and then validates, which injects defaults; its
relation pass collected "the keys `changes` names, read out of the
post-validation map". Those are different sets the moment normalisation removes
an empty list before the injection: the map then held a DEFAULT under a name the
caller had used. An unresolvable default must be dropped and reported, never
refused — one bad default in a schema would otherwise fail every bulk update
into that collection. It now gates on the snapshot that was already being taken
three lines above.
**Hydration depended on the rest of the batch.** A stored `multi_relation`
holding no resolvable reference got no `relation_targets` entry when read alone,
and an empty list when read beside an item that carried one — identical stored
bytes, two answers, decided by the batch. An absent key says "nothing hydrated
this field" and `[]` says "this field points at nothing"; those are different
statements and a caller cannot be handed either at random. The early return is
gone rather than special-cased: falling through costs nothing (the chunk loop
does not run on an empty id list) and duplicates nothing, where a second render
path is exactly how the two answers would drift apart again.
Instruments: 5 new tests. Mutants, each killed by the test that should kill it —
the cleared list skipped rather than deleted, the deletion LEAKING onto the
scalar spelling, the hydration early return restored, the bulk gate removed, and
the bulk gate widened to skip every relation key. The last two are a pair: one
proves the gate fires, the other proves it still lets a genuinely supplied value
be refused.
Gates: `make test` 0 FAIL / 30 ok · `make lint` 0 issues · gofmt clean · web
vitest 2580 passed.
TASK-2999 / PLAN-2857 U4
Claude-Session: https://claude.ai/code/session_019EmDd3Wy8b3cPG1TZBSzBu
* refactor(store): one shape reader composes the next, and the three questions are named (TASK-2999 / PLAN-2857 U4)
CONVE-35's move rather than another review round: rounds 1, 2 and 3 each found a
new member of a class, which measures the enumeration and not the code. The
class rounds 2 and 3 belong to is THREE READERS OF ONE SHAPE — `relationArray-
Elements`, `relationDefaultList` and `relationListShape`, plus a fourth across
the package boundary in `normalizeEmptyRelationLists`. Both rounds found the
wrong one being asked in the wrong place.
`relationDefaultList` was a second parser with the same two cases as the general
reader. It now COMPOSES it and adds exactly one rule — no blank elements —
because that one rule is the entire difference and a line saying so cannot drift
from a copy that repeats the parsing.
`relationListShape` stays separate and that is deliberate, not an oversight: it
must admit an array with non-string elements, since a caller's malformed array is
still a value they supplied and only an EMPTY one is the absent spelling. The
three questions are now written down together where the first reader is defined,
so a fifth reader has to declare its question before it gets written.
Mutants on the composition, both killed: the blank rule dropped, and the shape
result ignored.
Gates: `make lint` 0 issues · `make test` 0 FAIL.
TASK-2999 / PLAN-2857 U4
Claude-Session: https://claude.ai/code/session_019EmDd3Wy8b3cPG1TZBSzBu
* fix(store): a caller's malformed override is refused, a schema's is dropped (TASK-2999 / PLAN-2857 U4, codex round 4)
Both of round 4's findings were in round 3's fix, which is the third round
running where the review found my CORRECTION rather than the original code.
Round 3 added a deletion for a cleared `multi_relation` on the migrate path, and
put it BEFORE the origin split — so it treated a caller's malformed override
exactly like a schema's malformed default. Those get opposite dispositions on
purpose: a value someone ASSERTED is refused, a value nobody asserted is dropped,
and "silently clear the field" is neither.
**`[" "]` is not a cleared field.** It is a list of ONE reference that happens to
be blank, a shape every write door refuses. Counting it as cleared made a move
with `field_overrides: {"members":[" "]}` return 200 with the field absent: the
caller's own malformed value vanished instead of being refused. "Cleared" now
means the EMPTY list and nothing else, which is the same spelling the validators
normalise — the analogy with the scalar `" "` was the mistake.
**A mixed default `[validID, " "]` still 400'd a copy.** The destination-default
bucket used the LENIENT shape reader ("is this an array of strings"), so it
passed, resolution skipped the blank, and the shape check afterwards refused it —
on an optional field, for a default the caller never wrote. That bucket now uses
a strict reader, and the strict/lenient split has a named helper explaining why
the two origins are held to different standards: a refusal is visible to the
person writing the request, a drop is not.
Instruments: 6 new tests. Mutants: blanks counted as cleared again (killed by the
supplied-override leg), the default bucket back to the lenient check (killed by
both default legs), and the empty list no longer clearing (killed by its own leg).
ONE MUTANT SURVIVED and it found a coverage hole older than this unit. Applying
the strict list reader to the SCALAR type as well — which would drop EVERY scalar
relation default on a copy or move — survived the entire `internal/store` suite,
first under a `-run` filter narrower than its population and then unfiltered too.
Nothing covered the scalar arm of that bucket at all. Two tests now do, and the
mutant dies in the first of them.
Gates: `make test` 0 FAIL / 30 ok · `make lint` 0 issues · gofmt clean.
TASK-2999 / PLAN-2857 U4
Claude-Session: https://claude.ai/code/session_019EmDd3Wy8b3cPG1TZBSzBu
* fix(store,web): the third origin and the second view (TASK-2999 / PLAN-2857 U4, codex round 5)
Both findings were incomplete ENUMERATIONS of mine — the same failure round 1
was about, twice more, on smaller sets.
**Three origins, and I had settled two.** Rounds 1-4 gave SUPPLIED the refuse
disposition and DESTINATION DEFAULT the drop disposition, and left CARRIED on
the lenient shape check. A carried value is asserted by nobody either —
TASK-2878's rule is that it is dropped, never refused — so a carried
`[validID, " "]` passing the lenient check meant resolution skipped the blank
and the shape check afterwards REFUSED an ordinary same-workspace move on an
optional field, which is the exact outcome that rule exists to prevent. The
helper is renamed from `relationDefaultShapeIsUsable` to
`relationDroppableShapeIsUsable`, because naming it for ONE of its two origins
is what let the other keep the wrong check for a round.
**Two views implement grouping, and I fixed one.** ListView had both of the
defects BoardView had: the drop handler compared the stored ARRAY against a lane
string and fired `onStatusChange(item, "")` on every reorder, and the status chip
was withheld only for `isRelationGroup`, so a refused multi_relation board left
it live to write a scalar into a list.
THE BOARD'S CHIP GATE WAS REMOVED AFTER BEING WRITTEN. A mutant reverting it
survived, and the reason is structural rather than a weak test: a refused
grouping takes the `field?.options ?? []` branch of `columns`, a multi_relation
declares no options, so `ItemCard.statusCyclable` (which needs
`statusOptions.length > 1`) is already false. The gate was unreachable by
construction — the same disposition W7's write-side duplicate guard got. ListView
genuinely needs its gate, because its `statusOptions` is a CALLER PROP rather
than its own lanes, and its mutant dies. Both facts are written at their sites.
TableView needs neither: no grouping, and its chip is gated on
`field.key === 'status' && field.options`, which a multi_relation cannot satisfy.
Instruments: 8 new tests. Mutants: carried bucket back to lenient (killed),
carried bucket dropping every list (killed by the control), ListView drag gate
reverted (killed), ListView chip gate reverted — BOTH halves, since either half
alone suppresses the chip and a one-half mutant dies for the wrong reason
(killed), and the chip withheld ALWAYS (killed by both views' ordinary-grouping
controls).
A VACUOUS TEST was caught by its own control, which is why the control was
written first: my ListView chip leg omitted `statusOptions`, which is a prop of
that view and not something it derives, so the chip never rendered and the
assertion held against any build. The control failed, which is the only reason I
looked.
Gates: `make test` 0 FAIL / 30 ok · `make lint` 0 issues (run three times — one
earlier run exited 3 inside a piped command that swallowed its message; three
clean unpiped runs since, and no explanation offered for the one, since I cannot
reconstruct it) · gofmt clean · web vitest 2585 passed · `svelte-check` 0 errors
· `vite build` clean.
TASK-2999 / PLAN-2857 U4
Claude-Session: https://claude.ai/code/session_019EmDd3Wy8b3cPG1TZBSzBu
* fix(web,server): the guard I deleted was reachable, and an empty override is not "your value" (TASK-2999 / PLAN-2857 U4, codex round 6)
**Round 5's reasoning was wrong and this restores what it removed.** I added a
refusal gate to BoardView's status chip, saw the mutant reverting it SURVIVE, and
concluded the gate was unreachable: a refused grouping takes the
`field?.options ?? []` branch of `columns`, and a `multi_relation` declares no
options, so the chip cannot cycle anyway.
Every step of that is true of the schemas anyone would WRITE and none of it is
enforced. Nothing strips `options` when a field's type changes, so a
`multi_select` retyped to `multi_relation` in the schema editor keeps them,
`columns` is non-empty, and the chip cycles — writing a scalar into a list. A
claim about schemas is not a claim about code; the preflight's own
`collection`-on-a-`select` comment says exactly that about a neighbouring field,
and I had read it earlier the same day.
So the surviving mutant was evidence about my FIXTURE, not about the guard. The
test now builds the retained-options schema and the mutant dies. The lesson is
narrower than "mutants lie": a survivor is a question, and the first question is
whether the fixture can reach the case — which is the same
check-the-faithfulness step that caught two survivors earlier in this unit and
which I skipped on this one because the conclusion was convenient.
**An empty override is not the caller's value.** The preflight already treats an
explicit `null` override as "leave this unset": it drops the key from `final` AND
from `origin`, so the destination default validation injects is reported as a
default. An empty `multi_relation` list is the same statement in the other
spelling — normalisation removes it, the default lands in its place — and it took
the other branch, so the preflight labelled that default `from: "override"`. On
the one surface whose entire job is to predict what a copy will do, attributing a
value to a caller who never sent it is the whole failure.
`items.IsEmptyRelationList` is now exported and both the normalisation and the
preflight ask it, rather than the preflight re-deriving a rule that lives in
another package.
Instruments: 4 new tests. Mutants: the preflight branch reverted (killed), the
preflight dropping EVERY override (killed by the control), and the board chip
gate reverted in BOTH halves (killed — where round 5's identical mutant survived,
which is the measurement that matters here).
Gates, each run unpiped after an earlier piped one swallowed a lint failure:
`make lint` exit 0 / 0 issues · `make test` exit 0, 0 FAIL / 30 ok · gofmt 0
files · `svelte-check` exit 0 / 0 errors · web vitest 2585 passed · `vite build`
exit 0.
TASK-2999 / PLAN-2857 U4
Claude-Session: https://claude.ai/code/session_019EmDd3Wy8b3cPG1TZBSzBu
* fix(items,server,web): every spelling of none, every refusal-gated affordance, and a lost update in my own picker (TASK-2999 / PLAN-2857 U4, codex round 7)
Round 7 was asked to ENUMERATE rather than confirm, and returned five. Three
were siblings of round 6's fix — which is what an enumeration round is for, and
what six confirmation rounds had not produced.
**One sweep replaces round 6's point fix.** An empty `multi_relation` is about to
be removed by normalisation, and the destination default lands in its place, so
whatever `origin` says about the removed value gets said about the default
instead. Round 6 put that check in the preflight's OVERRIDE loop. That loop sees
the caller's RAW bytes, so `"[]"` is still a string there and only becomes an
empty list once `CoerceFields` runs; and it sees only overrides, so a CARRIED
empty list kept `from: "migrated"` by the same mechanism. One sweep over the
coerced map covers the override, the carried value and both string spellings,
because by that line they are all the same value.
**NULL is the third spelling.** On a full write the traversal treats a
present-but-nil key as absent for required-ness and defaults but does not REMOVE
it, so `{"members": null}` was stored — a third representation of "no targets"
beside the absent key and the empty list. Normalised on the full path only: on a
patch, nil is the deletion sentinel and the spelling being normalised TO.
**The lane create controls are refusal-gated too.** A `multi_relation` that
retained `options` produces NAMED lanes, so neither `isUncategorized` nor
`isRelationGroup` withheld the lane "+" or the lane menu's add: creating there
sent the lane string as the relation value. Third member of the affordance class
after the drag and the status chip — everything that writes the GROUP VALUE has
to ask the refusal.
**A lost update in W7's own picker.** Every list edit is a WHOLE-LIST write, and
they all derived from `value`, which only catches up after the server round trip:
remove A then B from [A,B,C] sent [B,C] and then [A,C], and the parent's 409
refetch-and-retry can persist the second, putting A back. The component now holds
the list it last SENT as the base for the next edit, released as soon as the prop
agrees by CONTENT.
THE FIRST VERSION OF THAT FENCE ONLY WORKED IN THE TEST. I cleared the hold from
an `$effect` reading `wsSlug` and `field.key` directly, which re-runs on every
prop assignment whether or not anything changed — so an ordinary parent
re-render mid-write released the hold and the stale prop came back as the base.
The two-click test never saw it, because nothing re-renders between two clicks.
A surviving mutant ("never release the hold") is what pointed at it, and the leg
that now pins it re-renders with IDENTICAL props mid-write. The hold is stamped
with the identity it was sent for and compared, which asks the question the fence
is for instead of one that merely correlates with it.
Instruments: 12 new tests. 12 mutants, all killed, including the four that
survived earlier passes and were each chased to a cause: a fixture that could not
reach the case, a fixture whose two outcomes coincided, a guard masked by its
neighbour, and the effect-timing one above.
Gates, unpiped: `make lint` exit 0 / 0 issues · `make test` exit 0, 0 FAIL / 30
ok · gofmt 0 files · `svelte-check` exit 0 / 0 errors · web vitest 2592 passed ·
`vite build` exit 0.
TASK-2999 / PLAN-2857 U4
Claude-Session: https://claude.ai/code/session_019EmDd3Wy8b3cPG1TZBSzBu
* fix(store,server): two more spellings, and a shape answer that depended on the route (TASK-2999 / PLAN-2857 U4, codex round 8)
Round 8 returned five. Two are members of classes already closed in this unit and
are fixed here; the other three open NEW classes and are deliberately NOT fixed —
they are the first rows of the resume table on TASK-2999, per the lead's
disposition for this round.
**The coerced NIL spelling.** `CoerceFields` JSON-parses a `multi_relation`
string, so the override `"null"` reaches the preflight's post-coercion sweep as a
nil — a spelling `IsEmptyRelationList` deliberately does not claim, since nil is
the PATCH path's deletion sentinel and that rule belongs to the validators. At
the sweep the question is only "will this value survive to be described", and a
nil will not, so the destination default that replaces it was again labelled
`from: "override"`.
Scoped to the list type, and a CONTROL pins that: a `json` field's `"null"`
coerces to nil too, and there nil is a legitimate stored value rather than a
spelling of absence. A mutant dropping every coerced nil survived until that
control existed.
**A shape answer that depended on which door asked.** `CoerceFields` runs BEFORE
`ValidateFields` injects a default, so an injected default is uncoerced at the
create door and coerced at the migrate door, where the copy coerces after
merging. A default written as the JSON TEXT `["<id>"]` was therefore stored by
one path and dropped as `invalid_shape` by the other — the same schema, the same
value, two answers chosen by the route. Droppable-shape judgements now run the
value through the real coercion first; the control is that text parsing to a
NUMBER is still not a list.
Mutants, all killed: the raw-value judgement restored, the shape check made
unconditional, the nil arm removed from the sweep, and the nil arm widened past
multi_relation.
Gates, unpiped: `make lint` exit 0 / 0 issues · `make test` exit 0, 0 FAIL / 30
ok · gofmt 0 files · `svelte-check` exit 0 / 0 errors · web vitest 2592 passed ·
`vite build` exit 0.
TASK-2999 / PLAN-2857 U4
Claude-Session: https://claude.ai/code/session_019EmDd3Wy8b3cPG1TZBSzBu
* fix(web): two whole-list writes to one field can no longer lose the newer one
Codex round 8, R8-1 and R8-2, fixed as one unit because they are two halves
of one question: which write to this field is current.
R8-1, the lost update. Every multi_relation edit is a WHOLE-LIST write, so
two of them in flight are not deltas the server can merge — each asserts the
complete value. Remove A then B from [A,B,C]: both PATCHes carry the same
expected_updated_at, the second commits first, and the first then 409s,
refetches and REPLAYS [B,C]. A is back on the row, with no error raised
anywhere. Round 7 fixed where the second edit got its BASE; this is the
ordering, which is a different mechanism.
R8-2, the hold outliving its write. The hold released only on prop
AGREEMENT, and a value the server REFUSES never comes back — so a rejected
clear of a required field kept showing the rejected list forever and ignored
every later server value.
The shared model is `$lib/items/fieldWriteOrder`: two counters per target, a
ticket at dispatch and a high-water mark of what WROTE, because each
single-counter version answers a different question than the one being asked
(harness note ticket-plus-high-water-request-ordering). The dispatch counter
gates the RETRY — the one place counting dispatches is right, since a retry
is a new send of an OLD body and nothing here discards a response. The
applied mark gates every write of `item`, giving newest-wins among responses
that resolve out of order. The field editor takes the same tickets so an
older write settling cannot release a newer one's hold, and the hold now ends
when the write is OVER: prop agreement, or the consumer's own settlement
where it gives one (`onchange` may now return a promise; returning nothing is
unchanged and still supported, which is what the consumers with no server
behind them do).
The pane's OCC retry loop is EXTRACTED rather than guarded in place:
ItemDetail is ~7,900 lines and not renderable, so the defect site would
otherwise only ever be grepped for spellings. It is now driven directly,
conflict by conflict, through the interleaving that loses the update.
Instruments — 23 tests, three files:
- fieldWriteOrder.test.ts drives the real loop, with a NEGATIVE CONTROL per
single-counter alternative: no supersession question at all, and counting
applies instead of dispatches, each of which puts A back.
- FieldEditor.multiRelation: a refused write stops holding and shows server
truth, the next edit bases on the prop again, an older write settling does
not release a newer one's hold, and a CONTROL that a consumer answering
nothing still holds (round 7 preserved).
- itemDetailOrdersFieldWrites.test.ts guards the binding (CONVE-19), with its
own limits written down.
Mutation matrix, 7 mutants. Killed: settlement release disabled (3 legs),
release without the ticket check (1), the catch's supersession check, one
claim gate, the `order` wiring, a per-item instead of per-field target.
SURVIVED and each chased rather than re-rolled: moving `take` to just before
the first send is behaviour-preserving and not a defect, so that leg was
REMOVED rather than kept as rigour nothing can falsify; dropping the flush
before release survives the whole file and is KEPT and labelled, because
unlike W7's duplicate guard it is not unreachable by construction — the
signature permits a consumer that resolves before it assigns.
The proximity bound in the binding guard has its receipt: assignments sit at
35/35/60/156/207 characters from their gate, deleting one gate pushes two to
359 and 410, so the bound is 300. It was 400 first, which caught only the
farther of the two and by ten characters.
TASK-2999 / PLAN-2857 U4
Claude-Session: https://claude.ai/code/session_019EmDd3Wy8b3cPG1TZBSzBu
* fix(web): a board that refuses to group stops grouping (R8-3)
Codex round 8, R8-3. Round 7 gated every affordance that WRITES the group
value and left the LANES reading it, so the board went on bucketing under a
notice reading "Showing everything ungrouped."
The route is the same false premise round 6 corrected one derivation away:
nothing strips `options` when a field's type changes, so a `multi_select`
retyped to `multi_relation` keeps them — and a lane key is the STRINGIFIED
array, so `['id-red']` and `['id-red','id-blue']` match options `id-red` and
`id-red,id-blue` and the two cards land in two scalar-labelled lanes.
`columns` is now empty when grouping is refused, which sends every item to
UNCATEGORIZED and leaves exactly the one lane the notice describes. One lane
rather than keeping the notice and dropping it: it is the smaller change, it
makes the sentence true, and it matches the day-64 ruling that this unit
refuses grouping rather than reinventing it. IDEA-3034 holds the multi-lane
alternative. Taken as the lead's stated DEFAULT — no ruling on the product
question had reached this window.
WHAT THIS DID TO THE ROUND 5/6/7 GUARDS, measured rather than assumed. With
no named lanes, three of the four affordance gates are subsumed by the lane
they now cannot reach: deleting `groupingRefusal` from `cardStatusOptions`,
from the lane "+", or from the lane menu's add leaves all 63 tests in that
directory green. The drag one is NOT subsumed — its mutant still dies. The
gates are KEPT, because each affordance asking the refusal directly is what
made round 7's class enumerable; what is not kept is the pretence, so both
legs that stopped discriminating say so in place, naming the leg that does
fail when this regresses.
A forcing term that showed the lane even when empty was written and then
REMOVED: `items.length === 0` renders EmptyState instead of the lanes, so no
mutant could reach it. W7's disposition, not a comment explaining a guard
nothing can falsify.
Instruments: one leg with the finding's own fixture — two cards, retained
options matching their stringified values — asserting ONE lane holding BOTH,
with the refusal and both cards as preconditions so "one lane" is neither
"not refused" nor "a dropped card". Reverting `columns` turns it and the
round-7 lane-control leg red.
Gates on the tip, each run unpiped: `make lint` exit 0 / 0 issues · `make
test` exit 0, 0 FAIL / 30 ok · gofmt 0 files · `svelte-check` exit 0 / 0
errors · web vitest 2616 passed · `vite build` exit 0 with web/build
regenerated. `make test-pg` not run and not owed — no SQL in any commit.
TASK-2999 / PLAN-2857 U4
Claude-Session: https://claude.ai/code/session_019EmDd3Wy8b3cPG1TZBSzBu
* fix(web): the round-8 enumeration's in-class findings, including two this unit introduced
ONE enumeration-prompted codex round on the frozen tip, per the lead's
disposition: not "is this right?" but "enumerate every remaining way a write
can leave the client and the server disagreeing". It returned 36 items across
four categories and said no category was empty — which is CONVE-24 working:
an enumeration is the only round that can return "none left" as a finding,
and this one did not.
Fixed here: the members of classes this unit had already closed, and the two
defects the unit's OWN round-8 fix introduced.
MINE, both in the open-children path, both from gating too widely:
1. The DECLINED branch took a claim. Declining writes nothing — it re-props
the value the client already had — but claiming advanced the applied mark,
so a write that had genuinely COMMITTED and was still in flight lost its
claim on return. Server changed, pane showing the old value, nothing left
to correct it. A branch that asserts no new server truth must not take the
mark for one.
2. A successful FORCED write was discarded on supersession. If the newer
write then failed without writing, the server held the confirmed value and
the pane held the old one. `claim` asks the question that matters — has
anything newer actually WRITTEN — and supersession does not.
IN-CLASS, four more:
3. The confirm callback SENDS, and asked about supersession only after it
returned. The dialog can sit open indefinitely; checking afterwards
suppresses the display of a write that already reached the server.
4. `submitOrderedOCC` asked only about supersession after its refetch, so
switching items during that await still produced another PATCH at the item
the pane had left — invisible, because the caller discards the response,
and landing on the row all the same. Both questions now, both times.
5. The hold's release: prop AGREEMENT is a coincidence test, not an answer.
Remove C from [A,B,C] and add it straight back, and the second write holds
a list equal to the prop nobody has changed — the effect read that as
confirmation and released mid-write, the first write's answer became the
base, and C was lost by the mechanism built to stop that. Settlement now
owns the release where it exists; agreement keeps it where it does not.
A synchronously-throwing consumer no longer strands the hold, and the
agreement path checks the identity stamp it had been ignoring.
6. The lane menu's bulk "Move all to" is the FIFTH affordance that writes the
group value; round 7 enumerated four. It offers `statusField.options` as
destinations, so a `status` retyped to `multi_relation` with its options
retained offers lanes that do not exist and sends a scalar into a list.
A SOURCE FILE WAS BINARY, and that is worth its own paragraph because it made
this unit's own verification lie. `fieldWriteTarget` was written with a real
NUL BYTE where the source should carry the two-character escape for one —
identical at runtime, but `file` reports "data" and grep then returns NOTHING
AND EXIT 1 for every pattern, which is indistinguishable from "not present".
A restore-and-verify grep over that file came back empty for a symbol four
lines below it. It shipped in a9f58ccb; git still shows that path as `Bin` in
this diff.
INSTRUMENTS — 9 new legs, every one shown red against the unfixed code:
the agreement release (its first fixture gave the same answer either way and
was rebuilt so it discriminates), the retarget stamp, the synchronous throw,
the post-refetch item switch, the bulk move (its first version passed against
a menu that never opened — it now proves the menu opened, with an entry that
is not gated on the refusal), and four source-guard legs pinning the two
dispositions above, the claim-as-GATE spelling codex showed was passable with
the `if` removed, and the callback's guard-before-send order. Two error
assertions now compare IDENTITY rather than class, which they were shown to
need.
NOT FIXED, filed as the lead ruled — new classes go into the package as
follow-up items, not another round: BUG-3036 (whole-item responses unordered
against each other), BUG-3037 (OCC cannot separate two writes inside one
second — the client guard never even runs), BUG-3038 (a whole-list retry
replays instead of re-deriving), BUG-3039 (a debounced edit cancelled by the
previous keystroke's response), BUG-3040 (a refused write leaves the rejected
value in scalar inputs), BUG-3041 (a conventional key typed multi_relation
crashes card rendering), BUG-3042, BUG-3043, BUG-3044, BUG-3045 (`npm test`
can exit 0 having run no .svelte.test.ts at all), BUG-3046, BUG-3047,
BUG-3049 (full-fields writes without OCC undo a field patch ON THE SERVER),
TASK-3048.
One existing test now describes a KNOWN LOSS and says so: the
ordinary-conflict leg asserts that a third party's addition disappears, which
is what the code does and not what it should do (BUG-3038). Invert it when
that lands.
Gates on the tip, each run unpiped: `make lint` exit 0 / 0 issues · `make
test` exit 0, 0 FAIL / 30 ok · gofmt 0 files · `svelte-check` exit 0 / 0
errors · web vitest 2625 passed · `vite build` exit 0 with web/build
regenerated. `make test-pg` not run and not owed — no SQL in any commit.
TASK-2999 / PLAN-2857 U4
Claude-Session: https://claude.ai/code/session_019EmDd3Wy8b3cPG1TZBSzBu
* test(web): stop the deliberate uncaught error failing the whole vitest run
CI's Web check went red at
|
||
|
xarmian
|
185194caf6 |
feat(items): content_state marks a body the server knows is stale (BUG-3000) (#1343)
* feat(items): content_state marks a body the server knows is stale (BUG-3000) A read of items.content could serve a body the server KNEW was behind the item's live collaborative document, with no way for the caller to tell. BUG-2995 fixed the write response; this is the read side. The window is UNBOUNDED, which is what makes this a marker rather than a documentation note. Nothing server-side moves applier content into items.content: PruneSweep prunes and never flushes, and its candidate query requires MAX(op-log.id) <= content_flushed_op_log_id, so an item in exactly this state is EXCLUDED from the sweep. The content is durable (those rows are retained BECAUSE they are unflushed) and the row stays behind until a tab next opens the item. ON THE ITEM, NOT THE DOOR. Reads return a bare item or array — there is no warnings envelope on the read side — so a per-door marker would be N changes across a population that grows whenever someone adds a handler returning a models.Item. An additive omitempty field on the struct is inherited by every door by construction, and a consumer that does not know it sees byte-identical responses. EXISTS, not MAX: the op-log's (item_id, id) index makes this a seek that stops at the first qualifying row, where the sweeper has to aggregate across the whole per-item range. Same index, different cost class — which is why the sweeper is the wrong precedent to copy here even though it is the obvious one. The marker tracks the CONTENT COLUMN, not the row. Four queries select an empty-string literal for content (dashboard and child batching, the index and changes readers); marking those would be a false signal about a body they never return, so they are exempt and say so at the site. ListItems builds both columns together for the same reason, so NoContent can never mark an omitted body. The population is 15 SELECT sites, not the 10 an items.go-only sweep finds: search.go and item_stars.go carry content too, and ListStarredItems shares the scanItems helper across that boundary — which is how the first pass failed, at runtime rather than at compile time. Summaries carry content_preview, so ItemSummary gets the field too; a preview is content. RECEIPT in the code: measured with a counterfactual arm, because a one-armed timing cannot attribute cost. 200 items on SQLite, best-of-5: with the predicate 2.44 / 4.00 / 3.81ms at 0 / 2k / 20k op-log rows, without it 3.58 / 2.86 / 3.27ms. The arms are indistinguishable and at 0 rows the arm WITHOUT was slower, so the honest statement is "below the noise floor at this scale", not "free". Untested scales are named in the comment. Tests assert both directions on the same item — a marker that is always on is indistinguishable from one that is always right. Mutation-tested: an always-true predicate is killed only by the clearing leg, a NoContent leak only by the omitted-body leg, and a never-set marker by all three. Both backends; the Postgres leg verified to RUN, not skip. Also corrected under CONVE-23: warnContentPendingFlush's comment said the flush question was "not established". It now is, and it says so. Claude-Session: https://claude.ai/code/session_01PDYjP67KTAcqpFpo3ijAAA * docs(items): restore an orphaned doc comment; name the export door (BUG-3000, codex r1) Two corrections from round 1, one of them mine to the population claim. The contentStateSQL const was inserted between getItemScanQ's doc comment and its func, orphaning that comment and attaching it to the const. Moved above it. POPULATION CORRECTION. Codex flagged content reads outside the 15 projections, and one is real: Store.ExportWorkspace selects content straight into models.ItemExport, so an export taken during a pending-flush window captures the stale body — and ImportWorkspace writes it back as canonical in the destination, where the op-log that held the real content does not exist. Filed as BUG-3032 rather than folded in: ItemExport is a portable bundle schema, not models.Item, so marking it is a format change the import path has to tolerate, and its query does not even use the i. alias contentStateSQL requires. Checked and correctly OUT: wiki_links.go's cascade re-read and items.go's in-tx re-read (internal rewrites, never returned to a caller), RemapAttachmentReferencesInWorkspace (same), the documents/templates/versions tables (different subjects), and item_versions (BUG-3031, split out by ruling). Claude-Session: https://claude.ai/code/session_01PDYjP67KTAcqpFpo3ijAAA * test(items): cover every spliced door; mark the public share DTO (BUG-3000, codex r2) Round 2's P1 and the coverage gap behind it, plus a correction to my own comment. THE COVERAGE GAP IS THE IMPORTANT HALF. The content-state tests exercised two of the fifteen spliced queries, so "the suite is green" was never evidence the other thirteen were column/Scan aligned — and a mismatch there is a RUNTIME error that leaves go build green, which is exactly how the first implementation pass failed. TestEveryContentBearingReadDoorCarriesTheMarker now drives all thirteen reachable store doors against one stale item. It is an alignment guard first and a behaviour test second: a door returning the right marker necessarily has a matching destination. ListStarredItems gets its own leg by name — it reaches the shared scanItems helper from outside items.go, which is the boundary that broke. P1, folded in rather than filed: publicShareItemDTO is an explicit allow-list, so it does not inherit the marker. A public share is the door whose reader is least able to notice on their own — an anonymous viewer has no editor, no op-log, nothing to compare against. Added only when set, so the key set stays byte-identical in the common case and TestMovedTo_ShareLinkNeverCarriesPointer's pin is undisturbed. MY OWN ERROR, corrected: the scripted exemption comment landed on ItemsModifiedSince instead of GetChildItemsForParents, so a comment reading "NO contentStateSQL here, this query selects an empty-string literal" sat directly above a query that has the predicate and selects i.content. A non-unique anchor edited the wrong match. Both exemptions are now on the functions they describe, verified by a check that reads what is actually below each one. Four further doors build their own shape and are filed as BUG-3033 rather than folded in, because each needs a rendering decision rather than a field copy: artifact export, playbook run, bootstrap convention bodies, and the MCP item resource (verified directly — the JSON it fetches now carries the field, and formatItemAsMarkdown re-renders without it). The playbook and bootstrap halves rank first there: an agent ACTS on those bodies. make test 0 FAIL; make test-pg exit 0, 0 FAIL, no banner, all 13 door legs PASS on Postgres too; make lint 0 issues. Claude-Session: https://claude.ai/code/session_01PDYjP67KTAcqpFpo3ijAAA * test(items): close two coverage holes in the coverage guard (BUG-3000, codex r3) Both P2s are holes in the test built to close holes, which is the part worth saying. ResolveItemIncludeDeleted was called with a SLUG. That falls through to GetItemBySlugIncludeDeleted, so the leg passed while measuring a different function and this one's own ref SELECT — a separately spliced query — stayed unexercised. Now driven by REF, with ResolveItem added alongside. The enumeration ran SQLite only. The FTS branches are DIALECT-SPECIFIC: ListItems (search) and Search take different SELECTs per backend, so a SQLite-only run left the Postgres variants of those spliced queries uncovered — exactly the runtime-mismatch hazard the test exists for. The whole enumeration is now parameterised over both backends: 14 door legs each. Two P3s: The CLI warning fired inside the empty-body check, so an item whose STORED body is empty while its document holds the real text — this bug's own shape — printed nothing and warned nothing. It now warns before the check, which is the case where a reader has least to go on. The TypeScript type was `string`, broader than the server's vocabulary. It is the literal 'applied_pending_flush', matching ItemWriteWarnings.content_outcome. Gates VERIFIED at this commit: make test 0 FAIL, make lint 0 issues, gofmt clean, and all 14 door legs passing on BOTH backends. make test-pg is being re-run alone: the previous attempt overlapped a second run of the same target in this worktree, which shares a compose project, so its teardown killed the other's database mid-run — 47 connection-refused lines and failures confined to pre-existing Postgres tests. That run is INDETERMINATE, not red, and is not being reported as a pass either. Claude-Session: https://claude.ai/code/session_01PDYjP67KTAcqpFpo3ijAAA * fix(share): the public COLLECTION share carries the staleness marker too (BUG-3000) Night 19's P2, and it is the same door class I folded in for the single-item share one round earlier — I fixed one allow-list and missed its sibling twelve lines away. Both public-share doors build an explicit allow-list rather than serialising models.Item, so neither inherits the marker the way a struct-serialising door does. Both serve an ANONYMOUS reader, who is the least able to notice staleness unaided: no editor, no op-log, nothing to compare the body against. Set only when non-empty, mirroring the sibling, so the key set stays byte-identical whenever the row is current — which is what the existing exact-shape pin on the item DTO depends on, and what a marker that were always present would break while satisfying every positive assertion. The test covers BOTH doors in BOTH directions on one item: absence asserted FIRST so the presence assertions cannot be read as an always-on marker. Mutation-tested independently — removing the collection half kills only the collection leg, removing the item half kills only the item leg. No cross-talk, so neither door is riding on the other's coverage, which is exactly how this one was missed. A note left at the site rather than in this message alone: if a third allow-list over item bodies ever appears, these should become one helper instead of a third copy. make test 0 FAIL; make test-pg exit 0, 0 FAIL, no banner (run alone — an overlapping run of the same target in one worktree shares a compose project and its teardown kills the other's database mid-run); make lint 0 issues; gofmt clean. Claude-Session: https://claude.ai/code/session_01PDYjP67KTAcqpFpo3ijAAA |
||
|
xarmian
|
fc32147165 |
fix(cloud): reconcile before destroying a workspace whose owner-add lost its ack (BUG-3026) (#1341)
* fix(cloud): reconcile before destroying a workspace whose owner-add lost its ack (BUG-3026) The cloud auto-create path soft-deleted the workspace it had just created successfully, whenever AddWorkspaceMember's commit landed and reported an error anyway. AddWorkspaceMember is a plain INSERT against PRIMARY KEY (workspace_id, user_id) returning the raw tx.Commit() error. A lost acknowledgement puts the row on disk and reports failure; the handler's retry then hits the primary key and fails deterministically; the handler concluded the owner could not be added and deleted the workspace. The deletion fired PRECISELY when the write had succeeded — and on exactly the "dropped Postgres connection" the retry was written for, so the guard was most dangerous in the case it existed to handle. Found by the CONVE-18 class sweep for BUG-2994: same defect (a store write retried after an error whose outcome is ambiguous), worse consequence. FIXED AT THE CALLER, NOT IN THE STORE, and this is the load-bearing decision. Making the INSERT idempotent is the obvious fix and is wrong: AddWorkspaceMember has nine call sites, and at handlers_members.go the duplicate-key error is a live race backstop behind an IsWorkspaceMember/409 check. ON CONFLICT DO NOTHING would turn a lost race into a silent success there; DO UPDATE would be a privilege bug, silently applying the second of two racing invites, so inviting an existing owner as viewer could demote them with no error. The destroy is one call site — verified, not assumed: the three sibling callers all log and continue, and BUG-2715 ruled that acceptance deliberately. So the cloud path's delete is the outlier and the reconcile belongs there. Three outcomes, not two: LANDED reconciles to success; ABSENT keeps the original delete, since the workspace really is unreachable; UNREADABLE keeps the workspace, because destroying one on a state nobody could read is worse than the recoverable orphan three other call sites already accept. Tests: the LANDED arm on BOTH backends through a commit seam that commits for real and returns an error on top, with an anti-vacuity leg asserting the row is genuinely present (without it the test would also pass against a seam that merely failed the insert — the case the existing ghost-user FK test covers and which was never broken). The UNREADABLE arm gets a server seam, because a COUNT over two indexed columns has no natural failure and that arm is the one a later simplification would silently drop. A third test pins that the ABSENT arm still deletes, so a fix that merely stopped deleting fails it. Verified red against the unfixed reconcile on both backends, green with it. make test 0 FAIL; make test-pg exit 0, 0 FAIL, no NO-TESTS-EXECUTED banner, Postgres leg PASS not SKIP; make lint 0 issues. Claude-Session: https://claude.ai/code/session_01PDYjP67KTAcqpFpo3ijAAA * fix(cloud): reconcile on ROLE, not existence, before destroying (BUG-3026, codex r1) Codex round 1 found the reconcile asked the wrong question. It used IsWorkspaceMember, which answers existence, but what the write attempted was an OWNER row — so a row carrying any other role reconciled to SUCCESS while the user could not administer their own auto-created workspace (owner > editor > viewer). Two questions, not one, and the first draft conflated them: "did MY write land?" and "is it safe to destroy this workspace?". Existence answers the second; only the role answers the first. So the reconcile is now four arms over GetWorkspaceMember's (row, error): UNREADABLE -> keep. Destroying on a state nobody could read is the worst outcome. ABSENT -> delete, exactly as before. The workspace really is unreachable. OWNER -> reconciled to success. OTHER ROLE -> keep, logged loudly. Not a success (they cannot administer it) and not a deletion (somebody has access). Deliberately NOT repaired by writing the role: this path cannot tell an ack-lost write from another actor's deliberate one, and escalating a permission on that ambiguity is the wrong default. Also from round 1: - The retry is now pinned by its LOG LINE, not by a commit count. The obvious pin — assert two commits — encodes a mechanism that does not hold: the retry's INSERT dies on the primary key BEFORE reaching tx.Commit, precisely because the first attempt's row is on disk. Measured, not assumed. The commit count is asserted at exactly 1 and the comment says why that one IS the evidence the first write landed. Verified the log pin discriminates: removing the retry fails it. - The test finds its workspace by OWNER rather than by position. ListWorkspaces is ORDER BY name ASC, so after[len(after)-1] was this workspace only by accident of today's empty fixture. - The anti-vacuity leg now checks the ROLE too, not just presence. New test for the fourth arm. All three new tests verified red against the unfixed reconcile; the two pre-existing tests and the ABSENT-arm control pass both ways, as they should. make test 0 FAIL; make test-pg exit 0, 0 FAIL, no banner, Postgres leg PASS not SKIP; make lint 0 issues. Claude-Session: https://claude.ai/code/session_01PDYjP67KTAcqpFpo3ijAAA * test(cloud): each reconcile arm asserts its OWN arm, not a shared outcome (BUG-3026, codex r2) Round 2 caught that two of the three new tests did not discriminate the arm they are named for. Both the UNREADABLE and the WRONG-ROLE arms keep the workspace — and so does the owner-success arm, so a workspace count of before+1 is satisfied by all three. Folding wrong-role back into "a row exists means success" — the exact defect round 1 found — would have passed the test written to prevent it. Each now asserts its own log line and, as a negative leg, the ABSENCE of the success line. Mutation-tested rather than argued: collapsing role==owner to member != nil kills only the wrong-role test, and collapsing the read-error arm kills only the unreadable test. Clean discrimination, no cross-talk. Also from round 2: the retry's log assertion is emitted immediately BEFORE the retry call, so it pins that the retry BRANCH was entered, not that the second INSERT ran. Nothing between them can skip it, so the distinction is theoretical today — but the line reads stronger than it is, and the comment now says so rather than letting a reader take it as proof. -race -count=2 clean (slog.SetDefault is process-global; no test here is parallel). make test 0 FAIL; make lint 0 issues. Claude-Session: https://claude.ai/code/session_01PDYjP67KTAcqpFpo3ijAAA * test(cloud): key the negative assertions on this arm's own phrase (BUG-3026, codex r3) "reconciled to success" is also emitted by the collab version-restore reconcile (internal/collab/manager.go), so a negative assertion keyed on it could only ever produce a false FAILURE here — never a false pass — but it is a string another subsystem owns, for no benefit. Both now key on "the owner row is present", which is unique to this arm. Re-verified the wrong-role mutant still dies after the change. make test 0 FAIL; make test-pg exit 0, 0 FAIL, no banner, Postgres leg PASS not SKIP; make lint 0 issues. Claude-Session: https://claude.ai/code/session_01PDYjP67KTAcqpFpo3ijAAA |
||
|
David Barkhausen
|
1a041b33ff |
feat(cli): pad token rotate — new secret in place, old one dies with the write (#1237 follow-up) (#1330)
The rotate endpoint (POST /auth/tokens/{id}/rotate) existed server-side
with no CLI in front of it; the #1237 review asked for it as its own PR.
- exact id only, same discipline as revoke: a typo is the server's 404
surfaced, never a wrong token rotated (which kills a live secret
somewhere)
- the new secret prints exactly once (count-pinned) with the store-it-now
notice, and the output says plainly that the old secret already stopped
working — RotateAPIToken replaces the hash in place, no grace window,
so the wording must not imply one
- --expires-in <days> posts a new expiry (server-capped by max lifetime);
without it no expires_in is posted and the original expiry is preserved
(test-pinned both ways)
- README documents rotate beside create/list/revoke, including that
rotate shares create's session_required gate (#1267)
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
|