mirror of
https://github.com/PerpetualSoftware/pad.git
synced 2026-09-11 21:39:01 +00:00
baaa236d8369ddb5eb9e8a8a07aae7887ed0a2b0
1663 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 xarmian
|
baaa236d83 |
fix(mcp): apply the field-array conflict guard to promoted keys too (BUG-2850)
Codex round 5, one P1. The `fields` object vs `field: ["k=v"]` conflict
guard lived on the generic path, which a promoted key never reaches:
`status`, `priority`, `category`, `parent`, `role`, `assign` and `tags`
all return from the promoted branch above it. So the one ambiguity this
function did not refuse was the one on the keys that matter most.
It did not fail closed either. The promoted branch writes the top-level
param (`out["status"]`) while the array entry stays in `out["field"]`,
and both the HTTP mappers and the CLI overlay `--field` entries AFTER
the named flags — so the array silently won. `fields:{"status":"done"}`
with `field:["status=cancelled"]` cancels the item. The same shape on
`parent` relinks or detaches it.
The guard now runs first in the promoted branch, before the existing
top-level-param check, with the generic path's semantics: differing
values refuse, an equal duplicate falls through and still promotes, and
a structure against a string entry (the `tags` case) refuses as
"one key cannot be both".
This is the fourth consecutive round where the defect was a guard
written on the generic path only, and round 4's test is why: it pins
`effort`, a key that takes the generic path, so it vouched for the path
the guard is on rather than for the class of keys that skips it
(CONVE-19). The new test drives all four promoted shapes plus an
equal-duplicate control leg.
Negative control: all four conflict cases fail on the unfixed tree
(run before the fix, not against a synthetic mutant); the
equal-duplicate leg passes both before and after, so it discriminates
refuse-on-ambiguity from refuse-on-agreement.
gofmt clean · go vet clean · go test ./internal/mcp/ ok
|
||
|
xarmian
|
b23c0dbc6f |
fix(mcp): apply the null and hierarchy guards to promoted keys too (BUG-2850)
Codex round 4, and both findings are the same defect in my own round-3 fix:
ORDERING. The null guard and the hierarchy-key guard sat BELOW the branch
that promotes status/priority/category/parent/role/assign/tags onto dedicated
params, so every promoted key walked around both.
- `fields: {"tags": null}` reached the promoted branch and became a silent
no-op, instead of the refusal the null rule documents.
- `fields: {"parent": 42}` was accepted here and dropped later by the
handler — the same silent-drop shape this whole bug is about, reintroduced
by a guard I added to prevent a different instance of it.
Both guards now run before that branch, so they apply to every key. A guard a
whole class of keys bypasses is not a guard.
Pinned separately from the generic-path cases, because the generic-path tests
passed throughout: they never exercised a promoted key, which is exactly why
the hole survived round 3. Reverting the hoist fails the new test. Well-formed
promoted keys still promote — tested, so the hoist did not break promotion
while closing the bypass.
Gates: gofmt clean, go vet clean, go test ./... 29 packages ok.
Claude-Session: https://claude.ai/code/session_011Q4b1iHtJtSyMs7BA2ySxo
|
||
|
xarmian
|
f15f60831e |
fix(mcp): guard hierarchy pseudo-keys and correct the fields description (BUG-2850)
Codex round 3.
1. [P1] A structured `plan` could silently DETACH an item. `plan` is not in
padItemPromotedFieldKeys, so it fell to the generic path — and once this
branch stopped refusing structures, fields:{"plan":{…}} reached the server
natively. There extractParentLink reads any PRESENT non-string plan/parent
as a hierarchy directive, drops the key, and on update clears the existing
parent link. Lifting the nested refusal quietly opened a path where a
malformed value detaches an item from its parent.
Guarded specifically: these keys take a string ref and nothing else. Both
`parent` and `plan` are listed, so the guard does not depend on which
other set a key happens to belong to. A string ref still works — tested,
so the fix did not re-refuse the normal case while closing the hole.
2. [P2] The MCP `fields` param description still told agents that
multi_select and json non-scalars are "refused, not written". This diff
makes that false, and a schema an agent reads is the artifact that decides
what it attempts — the reporter's agent rewrote seven playbooks after
believing exactly this kind of line. Rewritten to state what is true now,
including the two remaining refusals (null, structured parent/plan) and
that structured values need the remote transport.
Gates: gofmt clean, go vet clean, go test ./... 29 packages ok. Removing the
hierarchy guard fails its test.
Claude-Session: https://claude.ai/code/session_011Q4b1iHtJtSyMs7BA2ySxo
|
||
|
xarmian
|
2cf9f0035a |
fix(mcp,server,cli): three codex round-2 findings (BUG-2850)
1. [P1] The structured-value refusal was in the wrong place and killed the fix. It went into BuildCLIArgs, which env.Dispatch runs for BOTH transports before handing off to whichever Dispatcher is configured — so it blocked the remote /mcp door too, and the native-field handling that is the whole point of this change was never reached. Moved into ExecDispatcher, which IS the stdio door. My own test could not see this: it called mapItemCreate directly, so it vouched for the mapper and not for the path that reaches it — CONVE-19's exact shape, in a unit where I had already written binding tests for the other half. The tests are now split along the two claims the first version conflated: nested values REACH the dispatcher (the remote door is unblocked), and refuseStructuredFieldsOverCLI refuses them at the CLI door naming the transport. 2. [P2] The CLI warning sat after the `--format json` early return, so the caller most likely to have sent a mistyped key — one piping stdout into a parser — was the one caller who never saw it. Moved above the return, and out of the `ref != ""` branch it was also trapped in. Still stderr. 3. [P2] A nil value in fields_patch DELETES the key (store/items.go), so reporting it as an undeclared field told the caller a field was stored that the same request removed. Filtered at the patch site, not inside UndeclaredFieldKeys, because nil means "store JSON null" on the full-fields path where reporting it is correct. Gates: gofmt clean, go vet clean, go test ./... 29 packages ok. Claude-Session: https://claude.ai/code/session_011Q4b1iHtJtSyMs7BA2ySxo |
||
|
xarmian
|
dc3fc2d50e |
feat(server,cli): name undeclared field keys on the write response (BUG-2850)
Undeclared keys are ACCEPTED — the census found 168 live values under 14 such
keys, and refusing them would break read-modify-write on items nobody edited
wrongly. But once stored, a typo and a deliberate extra field are
indistinguishable, so the write now says which keys it did not recognize.
- models.Item gains `Warnings *ItemWriteWarnings` with `undeclared_fields`,
omitempty and additive. NEW API SURFACE: item write responses carried no
warnings element before. Wrapping the response as {item, warnings} was the
alternative and would have broken every existing parser; a clean write is
byte-identical to before.
- items.UndeclaredFieldKeys consults models.IsReservedItemField rather than
re-listing the reserved set — that set exists so callers ask, and its doc
comment records what re-listing cost last time. So a write carrying
implementation_notes or github_pr reports nothing.
- fields_patch reports only the PATCHED keys. A stray key already on the item
is not something this write introduced, and naming it on every touch would
train the reader to ignore the field.
- The CLI prints one line to STDERR. Never stdout: `--format json` output is
piped into scripts, and a warning there would corrupt the JSON they parse.
- CLAUDE.md documents the element as new surface.
Controls: never attaching the warnings fails the pin; reverting the HTTP
mapper's native overlay fails the remote-door type test; dropping the
reserved-key exclusion fails its own test.
Two coverage gaps the controls FOUND rather than confirmed, both now closed:
the remote door's native overlay was covered by no MCP test at all (a revert
left the package green), and the reserved-key exclusion had no test either.
Both were written after the control survived, which is the only reason they
exist.
Gates: gofmt clean, go vet clean, go test ./... 29 packages ok.
Claude-Session: https://claude.ai/code/session_011Q4b1iHtJtSyMs7BA2ySxo
|
||
|
xarmian
|
dd919dd0a9 |
feat(mcp): carry the fields object with its JSON types intact (BUG-2850)
Second half of BUG-2850, ruled after a census: undeclared keys stay accepted
on every door, and a value's native JSON type is preserved wherever the
encoding carries one.
Only two doors carry a type at all. The remote /mcp `fields` OBJECT param
does — and the catalog was destroying it, flattening every value into
`field: ["key=value"]` before dispatch. The direct HTTP API does. The other
three (remote `field:[…]`, CLI `--field`, stdio MCP, which dispatches through
the CLI) are string-by-construction: `key=value` carries no type to preserve,
and a string is the correct and complete representation of `cost=42` typed at
a shell. So this does not "make every door preserve types" — it stops
destroying the types the object form already had.
- The catalog merge now emits the native map alongside the string entries, so
each transport takes what it can use. Both forms describe the same input;
they differ only in fidelity.
- The HTTP create and update mappers overlay the native map LAST, so it wins
over the stringified copy of itself.
- hasFieldChanges consults the native map. Without that a NESTED-ONLY update
emits no `field` entry at all, so it reported success and wrote nothing —
the silent-drop shape this bug is about, arriving through the fix for it.
PR #1159's blanket refusal of nested values is LIFTED, as ruled: its
precondition (server-side coercion) landed in
|
||
|
xarmian
|
b451fb50de |
test(server): bind the coercion to its call sites, and enforce copy/preflight agreement (BUG-2850)
CONVE-19: wiring is a claim. The previous commit threaded CoerceFields through eight validate sites; a test at the items package vouches for the function, not for any of those bindings. - Three HTTP-door tests (create, update, fields_patch) assert the stored NATIVE TYPE, not that the request returned 201 — a test that only checked the status passes on an implementation that stores the string, which is the shape the reporter described. - A text field holding "42" must stay a string in every one of them. Fixing this bug by coercing anything that parses would retype real data. - An un-coercible value must still be REFUSED with the validator's existing message, so coercion is not quietly widening what the server accepts. - TestCopyAndPreflightCoerceIdentically makes the cross-package invariant real. The preflight validates in internal/server and the copy in internal/store; both files carry a comment saying they must match, and a comment protects nobody. The assertion is agreement FIRST — whatever they do, they must do the same thing — and only then that both accept and the copy stores a number. Controls, each run against the mutated tree: - CoerceFields reduced to the identity function (the unfixed build) fails the two door tests and the items typing test. - Dropping the call at CREATE alone fails only the create test; dropping it at FIELDS_PATCH alone fails only that one. The bindings are individually covered, not covered in aggregate. - Coercing in the copy but not the preflight FAILS, and so does the reverse. Both drift directions are caught. BOUNDARY, stated rather than implied: four of the eight sites — move, bulk update, bulk move, and the migrated-schema paths they share — are wired identically but have no test that fails if that specific wiring is dropped. They are covered by the existing suites for their own behaviour, not for coercion. A follow-up should extend the door tests to them. Gates: gofmt clean, go vet clean, go test ./... 29 packages ok. Claude-Session: https://claude.ai/code/session_011Q4b1iHtJtSyMs7BA2ySxo |
||
|
xarmian
|
ae793e6fa6 |
fix(server,store,items): coerce field values to their declared types server-side (BUG-2850)
The write doors disagreed about what `key=value` means. The CLI has coerced by schema type since BUG-1125, and local stdio MCP inherits that by shelling out to the binary — but the remote /mcp transport builds its field map in ingestFieldKVP with `dst[key] = val`, so every value arrives as a string. validateFieldType then correctly refuses a string for a declared number or json field, and the net effect was that an MCP agent on that transport could not write those fields AT ALL: every attempt a 400, not a mis-typed value. Measured before writing anything (repro table on BUG-2850's trail): CLI and stdio MCP store 42 and an array; the HTTP door 400s on both; an UNDECLARED key is stored as a string on every door. items.CoerceFields(fields, schema) converts strings to the declared type — number via ParseFloat (NaN/±Inf refused, because json.Marshal cannot encode them and the ignored downstream error would silently drop the whole payload), json/multi_select via Unmarshal, checkbox via ParseBool — and is applied immediately before every Validate* call. Three deliberate non-behaviours, each with a test: - A value that will not parse is left as the string for the validator, so the existing "must be a number" error still fires. Coercion invents no error path, and cannot turn a currently-PASSING write into a failure. - Non-string values pass through untouched; an int stays an int. - Text-typed fields holding "42" stay strings. Coercing anything that parses would retype real data while fixing the bug. Not folded into ValidateFields, though that would be the single call site: a function named Validate that mutates its input is a trap, and two callers re-marshal the map they pass. THE POPULATION IS 8 CALL SITES, and finding them took two sweeps. The first was scoped to internal/server and found 7; the copy path validates in internal/store (items_cross_workspace_copy.go), which only a repo-wide sweep sees. The preflight and the store-side copy now carry cross-references to each other: the preflight exists to PREDICT the copy, they live in different packages, and that is exactly how they would drift unnoticed. The undeclared-key half of BUG-2850 is untouched and marked as a decision point in CoerceFields — refuse/warn/keep is with Dave. A test pins today's keep behaviour so the ruling lands as a deliberate change. The CLI's parseFieldFlag deliberately STAYS: it is why two of four doors are correct today, and removing it alongside its replacement would put all four at risk of one mistake. Retiring it is a follow-up. Claude-Session: https://claude.ai/code/session_011Q4b1iHtJtSyMs7BA2ySxo |
||
|
xarmian
|
70099c2724 |
Merge pull request #1239 from PerpetualSoftware/fix/bug-2848-pane-jk-anchor
fix(web): capture the pane-follow target at keypress, not when the timer fires (BUG-2848) |
||
|
xarmian
|
af24997c72 |
fix(web): re-resolve the follow target by id at fire time (BUG-2848)
Codex round 1, P2, and a real latent bug in the previous commit. Capturing the item OBJECT and reusing it 140ms later keeps a stale snapshot: a rename during the debounce changes the slug, `openItemPane` builds the URL from that slug, and the id-only existence check passes happily on the way to a dead URL. What is captured is now the IDENTITY — `targetId` — and the callback re-resolves the current row from `filteredItems` before opening it. That still follows a row that MOVED, which is the whole point of the fix, and still skips one that was DELETED, while picking up any change to the row itself. Also answering the round's second P2 in the spec rather than in code: the race is probabilistic and cannot be made deterministic without a seam in the page. The asymmetry is what makes that acceptable, and it is now written down — a round that misses the 140ms window still PASSES on a correct build, because the cursor moves, the pane follows and the intended row is where it should be. So missing costs power, not correctness; the failure mode is a false green, never a false red. Three rounds put a false green around 1 in 1700 against a build that loses the keypress 11 times in 12. pane-follow-live-list + pane-controller: 44/44 across both projects. |
||
|
xarmian
|
1a76531eb3 |
fix(web): capture the pane-follow target at keypress, not when the timer fires (BUG-2848)
The list is SSE-live and the pane-follow is debounced 140ms. The callback
re-read `filteredItems[focusedIndex]` when the timer fired, which made a
keystroke depend on the list holding still for those 140ms. It does not.
The failure was silent, and that is what made it hard to see. `j` advanced
`focusedIndex`; an item arriving during the debounce shifted every index below
it, sliding the PANED item down onto that very index; the callback read it
back, found "the focused row is already the paned item", and returned through
its own guard. No cursor move, no re-target, no error — a discarded keystroke.
The target is now captured BY IDENTITY at keypress time. The callback still
re-checks pane state and that the row still exists — identity, not position, so
a row that MOVED is followed correctly and only a row that was DELETED is
skipped.
MEASURED, because the first two explanations were both wrong.
The trail's diagnosis was that an insert leaves `focusedIndex` behind so `j`
lands on the already-open row. A snap-back $effect re-syncs the cursor to the
open item on every `filteredItems` change and prevents exactly that; a pin that
waited for the row to settle passed every candidate assertion.
So the second hypothesis was that the snap-back undoes the cursor move during
the debounce, and the fix was to suppress it while a follow is in flight.
Measured: 12 of 12 failures, WORSE than the 11 of 12 baseline. The stale index
lands on the paned item by itself; the snap-back was never the culprit.
Capture-at-keypress, same harness, same sweep size:
baseline (unfixed) 11/12 lost the keypress
suppress snap-back 12/12 lost the keypress
capture target at keypress 0/12
across all three measured properties — the pane re-targeted, the cursor moved,
and the pane landed on the row that was actually below the cursor.
The new spec CAUSES the race rather than waiting for it: it seeds a row above
the cursor, then lands a second insert across the keypress inside the debounce.
Three rounds per run, because one round caught the unfixed build 11 times in 12
and three make a false green not worth reasoning about. Counterfactual against
the unfixed controller: 3 of 4 desktop runs fail with the bug's signature —
`Expected: "DOC-16"` (the intended row) versus `Received: "DOC-15"` (the row
that was already open).
It asserts three things and none is redundant: `retargeted` alone passes if the
pane wanders anywhere; `cursorMoved` alone passes if the cursor moves and the
pane ignores it; `intended` is what pins the actual contract. A fourth that
suggests itself — "the pane agrees with the focused row" — passes VACUOUSLY on
the bug, since cursor and pane are then both stuck on the opened row. It was
measured doing that and is deliberately absent.
pane-controller.spec.ts is unchanged and still green (21/21). Its intermittent
failure was this defect, not the shared-workspace pollution it was filed as —
it just could not cause the race, so it only caught it when a sibling test's
seed happened to land in the window.
|
||
|
xarmian
|
58f50909af |
Merge pull request #1234 from PerpetualSoftware/feat/idea-2843-composer-quote-handle
feat(web): comment on a selection, with comments back under the item content (IDEA-2843) |
||
|
xarmian
|
e92f6f235d |
fix(web): the sidebar footer's Settings label wrapped once the GitHub link joined the row (BUG-2844)
MEASURED, not eyeballed. The desktop sidebar is 260px wide, less 24px of .sidebar-inner padding, so the footer row has 235px. Four 32px controls and four 8px gaps are fixed cost; .settings-btn is the only flex:1 item and got what was left — a 75px box, 51px of content after its 12px padding. The label "⚙ Settings" needs 56.3px. Five pixels short, so the gear and the word landed on separate lines and the row grew from 33.9px to 51.7px. The five pixels arrived with the GitHub link (IDEA-2711, PR #1229): a 32px control plus a fifth gap took 40px out of a box that had about 22px of slack. space-2 -> space-1 on the row returns 16px and the Settings padding another 8px, all of which lands in the one shrinkable item: a 75px content box against 56.3px of text, a 33% margin rather than the 5% either change alone would have left. Measured after: one line box, row height back to 33.86px. ONLY THE DESKTOP ARM WAS BROKEN, which the dispatch's "every width the layout supports" is what surfaced. Below 768px the sidebar is 280px AND the collapse button is gone, so the row carries four controls in 255px and the label had 135px to itself. It measured one line box before this change and still does — and both mobile legs of the new spec PASS against the unfixed CSS, which is what makes the desktop failures mean something. .github-btn also gains `flex-shrink: 0`, which every other control in the row already had. Harmless today — its automatic minimum size equals its 32px content box — but it made the row's one shrinkable item ambiguous, and .settings-btn is meant to be that item. THE TEST IS AN E2E SPEC BECAUSE NOTHING ELSE CAN HOLD IT. jsdom performs no layout, so a vitest render of Sidebar.svelte reports identical geometry with and without the bug. It asserts LINE BOXES rather than row height: height grows for other reasons and could stay put through a wrap, while Range.getClientRects() returns one rect per line, so the count is the question itself. Counterfactual run against the reverted declarations: desktop fails "Expected: 1, Received: 2", the shrinkable-control leg fails on the extra item, mobile stays green. Gates: web unit tests 115 files / 1978 tests green; svelte-check 0 errors (the 6 warnings are pre-existing, in files this does not touch); go build and go vet clean. The full Go suite was NOT re-run locally — no Go file changed — and CI covers it; naming the narrowing rather than reporting a leg I did not run. |
||
|
xarmian
|
0900be6241 |
chore(deps): bump golang.org/x/crypto to v0.56.0 (BUG-2851)
Two advisories published 2026-09-02 19:12Z (GO-2026-6354, GO-2026-6355; DoS in golang.org/x/crypto/ssh, fixed in v0.56.0) made govulncheck fail the Nix job on runners whose vulnerability database had them — intermittently across runners, not as a threshold: main at |
||
|
xarmian
|
7a7e9d669b |
fix(web): the selection toolbar is a row again (IDEA-2843)
Codex round 7. `.bubble-menu` had no layout of its own. Its buttons are themselves `display: flex`, so they are block-level and STACK — invisible while the menu held one action, wrong the moment Comment joined Extract. It also falsified the dimensions `positionMenu` clamps against, so the menu drifted over the text it points at. A row layout on the container; the expanded state opts out, since the extract form lays itself out. Layout is not observable in jsdom, so the assertion lives in the e2e: the two buttons share a row (y within 4px) and Extract sits to the right of Comment. Removing the row layout fails it in a real browser — verified. Declined, with the reason recorded in the code: "Comments 1+" can appear when the only unfetched entries are activity or versions. `+` reads as a LOWER BOUND, and a lower bound of 1 over exactly one comment is true. Knowing whether more comments exist means fetching the rest of the feed, so the alternative trades a true imprecise count for a confident wrong one. Gates: 119 files / 2005 unit tests, svelte-check 0 errors, e2e 2/2 on the selection spec against a rebuilt binary. Claude-Session: https://claude.ai/code/session_011Q4b1iHtJtSyMs7BA2ySxo |
||
|
xarmian
|
4ebad409a2 |
fix(web): gate the composer's item identity during A→B navigation (IDEA-2843)
Codex round 6. 1. [P1] During an A→B navigation `item` still holds A while loadData fetches B, so ItemTimeline received A's itemId/collectionId beside B's itemSlug — and an attachment dropped in the composer inside that window is associated with the WRONG item. The wiring is PRE-EXISTING and identical on main. What changed is the exposure: the composer used to sit behind the Activity tab, and tabs reset to Details on an item switch, so reaching it inside the load window took a deliberate tab click. It is now on the tab you land on. Widening a latent hole is the same as opening one, so it is fixed here. Fixed by feeding honest inputs rather than adding a gate: the host passes itemId/collectionId only while `itemMatchesRef`, and ItemTimeline's canEdit already derives false without them, so the composer hides until the identities agree. 2. [P2] A load failure's banner outlived it — `loadMore` set `error` and never cleared it, so a successful retry left the failure sitting beside the entries it claimed had not loaded. Newly visible because the error is mirrored to the tabs now. Test boundary, stated: the new test covers ItemTimeline's half of the gate (no identity ⇒ no composer), verified by a control that flips its default to permissive. The host's half — the `itemMatchesRef ? … : undefined` — is not unit-testable here, since ItemDetail cannot be mounted in jsdom. Gates: 119 files / 2005 unit tests, svelte-check 0 errors. E2E: 37 passed across the five affected specs. An earlier run of that same set had one failure — capstone's "stale back-settle" nav test — which did not reproduce alone or in an identical re-run, and sits outside this diff's surface (history back-settle and drill targeting; nothing here touches either). Recorded rather than dropped. Claude-Session: https://claude.ai/code/session_011Q4b1iHtJtSyMs7BA2ySxo |
||
|
xarmian
|
09a183844d |
fix(web): a submit no longer erases what arrived mid-flight; empty states wait for the last page (IDEA-2843)
Codex round 5. 1. Data loss, in the handle I added. `doSubmit` clears the composer on success, and a quote pushed in through `appendMarkdown` during the round trip was cleared with it — the quote simply vanished. The clear now requires the composer to still hold what was SENT. This also fixes a PRE-EXISTING loss by the same mechanism: text the user typed while a submit was in flight was erased too. It is the same class as the item-identity capture already guarding this path (PLAN-2105 / TASK-2112) — that one asks "is this still the same item", this one asks "is this still the same content". 2. Empty states appeared while more pages remained. A first page carrying only other kinds made a filtered view say "No versions yet." before the pages that would have contradicted it were fetched — a claim about the item made from one page of a feed. Both views now wait for the last page; until then the "Load more" button is what the reader sees. Gates: 119 files / 2004 unit tests, svelte-check 0 errors. The mid-flight fix has a negative control — restoring the unconditional clear fails the new test. Claude-Session: https://claude.ai/code/session_011Q4b1iHtJtSyMs7BA2ySxo |
||
|
xarmian
|
b82c689eda |
fix(web): pagination honours the caller's filter; the host's Load more is styled (IDEA-2843)
Codex round 4.
1. Filtered pagination, fixed properly this time. Round 1 gave the TABS a
host-side retry wrapper and left the same defect in the owner's own
button: the Comments view could also page without showing a new comment.
Fixing the instance and not the class, twice on the same defect.
`loadMore(forKinds?)` now takes the caller's view filter and the hop loop
counts only entries that filter admits, defaulting to the component's own
`visibleKinds`. Whoever pressed the button says what progress means. The
host wrapper is deleted — MAX_EMPTY_HOPS already bounds the walk, so the
six-round loop on top of it was compensation for the missing filter.
Caught while wiring it: the owner's own button was `onclick={loadMore}`,
which passed the MouseEvent as `forKinds`. svelte-check found that; no
test would have.
2. The host's "Load more" was unstyled. Its class name matches
ItemTimeline's, but Svelte scopes styles per component, so the copied
NAME got browser defaults and nothing warned — CSS is the part no test
here asserts. Styles copied over with the reason recorded. Swept the
other class names the split moved across that boundary
(entry-list, compose, timeline-header, entry-count, empty): none is used
in the host, so the population is this one.
Gates: 119 files / 2003 unit tests, svelte-check 0 errors. The pagination
fix has a negative control — restoring the unfiltered break fails the new
test, which needed descending fixture timestamps to avoid passing for the
unrelated "cursor did not move" reason.
Claude-Session: https://claude.ai/code/session_011Q4b1iHtJtSyMs7BA2ySxo
|
||
|
xarmian
|
abe8d93c10 |
fix(web): per-view titles, counts and empty states; a failed quote is not silent (IDEA-2843)
Codex round 3. 1. The split left every view describing the WHOLE feed. The comments section was headed "Timeline" with a count of every entry, so an item with three activity entries and no comments read "Timeline 3" over an empty list — a count of things the reader cannot see. And `showEmpty` was computed over the whole feed, so a view whose own slice was empty rendered nothing at all: no entries, no explanation. Title, count and empty state now describe what the view RENDERS. `title` and `emptyLabel` are props: "Comments" / "No comments yet." on Details, "No changes yet." / "No versions yet." on the tabs. The deliberate choice this reverses is mine — I passed showEmpty over the whole feed on the grounds that a filtered-out tab must not claim the item has no history. Right premise, wrong fix: the answer is to say something true about the slice, not to say nothing. 2. A failed quote was silently discarded. `appendMarkdown` returns false precisely so "did nothing" is distinguishable from "inserted" — and the only caller threw the boolean away and hid the menu, putting the silent no-op back exactly where the handle was built to remove it. A false now keeps the menu and the selection and reports it. Gates: 119 files / 2002 unit tests, svelte-check 0 errors, 14 e2e in desktop-chromium against a binary rebuilt from this tree. The discarded-return fix has a negative control — restoring the bare `onComment(...)` call fails the new test. (A first attempt at that control failed on shell quoting and silently ran against UNMUTATED code; it was re-run properly before this claim.) Claude-Session: https://claude.ai/code/session_011Q4b1iHtJtSyMs7BA2ySxo |
||
|
xarmian
|
8d7fdc22f3 |
fix(web): consult the error state everywhere success was assumed (IDEA-2843)
Codex round 2, and both findings are follow-through misses on my own round-1 fix: I added an `error` to the mirrored feed and then left the code that assumes a load succeeded reading only `entries` and `hasMore`. - A failed load rendered the error AND "No timeline entries yet." One is a statement about the ITEM; a failed load knows nothing about the item, so the pair says something false beside something true. `showEmpty` now consults `error`. - The filtered "load more" wrapper retried a dead server up to six times per click. `loadMore()` catches and resolves, leaving `hasMore` true, so the loop had no reason to stop. It bails on `error` now. CONVE-18 sweep rather than the two named instances. The population is every consumer of the mirrored feed — six read sites in the markup, four in the wrapper. Two were defects (both above). One is a deliberate non-change: the "Load more" button still renders while an error is showing, because that is the retry affordance. The class also reaches the OWNER, where the same empty-beside-error contradiction is PRE-EXISTING on main and is fixed here too, since leaving it would mean the comments view kept the bug the tabs just lost. The regression test asserts the mounted owner's DOM. A first version recomputed `entries.length === 0 && !loading && !error` and asserted that — which passes whatever the component actually renders — so it was replaced. Gates: 119 files / 2001 unit tests, svelte-check 0 errors. Reverting the owner's guard fails the new test. Claude-Session: https://claude.ai/code/session_011Q4b1iHtJtSyMs7BA2ySxo |
||
|
xarmian
|
5c0d8e9d34 |
fix(web): three codex round-1 findings on the timeline split (IDEA-2843)
All three are consequences of the two-view split that the split's own tests did not reach. 1. Multi-paragraph selections lost their paragraph breaks. The bubble menu builds `selectedText` with `textBetween(..., ' ')` because Extract uses it as an item TITLE, where newlines would be wrong — so quoting through it flattened two paragraphs into one run-on line. Worse, it made `toBlockquote`'s blank-line handling unreachable from production: that behaviour had a passing test and no call site that could produce it. The quote now re-extracts with a paragraph separator, and a test asserts the blockquote's blank line end to end. 2. Activity and Versions rendered a FAILED load as "No timeline entries yet." Loading and error were the owner's states and did not cross the mirror, so an unreachable server and an empty timeline looked identical on the tabs that only render entries. `error` joins the mirror; both states render. 3. "Load more" could visibly do nothing on a filtered view. The owner's hop loop stops as soon as a page adds an entry of ANY kind, so a page of pure comments ends it having added nothing to the changes view. Pre-existing on Versions; widened to Activity when comments moved off it. The host now pages until THIS view's list grows, bounded at six rounds — not fixed in the owner, which would have to know what the other view is rendering. Gates: 119 files / 2000 unit tests, svelte-check 0 errors. Fixes 1 and 2 have negative controls: reverting to the space-joined text fails 1, dropping `error` from the mirror fails 1. Claude-Session: https://claude.ai/code/session_011Q4b1iHtJtSyMs7BA2ySxo |
||
|
xarmian
|
346a5c92a9 |
feat(web): Comment action on the selection toolbar, quoting into the composer (IDEA-2843)
GitHub #1228. Selecting a passage in an item's content now offers Comment beside Extract; it quotes the selection as a markdown blockquote into the comment composer under the content, appending after a blank line so an in-progress draft survives. The selection is NOT consumed — unlike Extract, which replaces it with a wiki-link — so a reader can quote the same passage twice or keep reading. - toBlockquote() prefixes EVERY line including blank ones. An unprefixed blank line ends a blockquote in markdown, so quoting two paragraphs without it silently drops the second out of the quote and leaves it looking like the commenter's own words. - The action renders when the host supplies `onComment`. A composer to quote into IS the capability; a flag that is always true beside a callback that is always supplied would be two ways to say one thing. - The button's accessible name is "Comment on selection". The composer's submit button is also named "Comment", and two identically-named buttons with different effects is a real ambiguity for name-based navigation — found by the first end-to-end run failing on a locator, not on behaviour. A NEGATIVE result, measured and kept. The action was briefly gated peek-independently, reasoning that a peeking master keeps a live composer (BUG-2263) but could not act on a selection. That state does not exist: a drag-selection in a peeking master RE-ACTIVATES it (focus-follows-editing, PLAN-2179 DR-2), so a selection and a frozen master never coexist. The gate is back on `mutationsEnabled`, and e2e/selection-comment-peek.spec.ts asserts the re-activation so a future change that makes selections survive the freeze turns red there instead of quietly reopening the question. Gates: 119 files / 1998 unit tests, svelte-check 0 errors, and 37 e2e in desktop-chromium — the 2 new ones plus the 35 in the four specs the comment relocation touched, run against a binary built from this tree. Claude-Session: https://claude.ai/code/session_011Q4b1iHtJtSyMs7BA2ySxo |
||
|
xarmian
|
7aef246dcd |
feat(web): comments move under the item content; Activity keeps changes (IDEA-2843)
GitHub #1228. Reviewing an agent-written doc meant many small comments, and every one cost a trip to the Activity tab and back. TASK-2294's own spec put an activity preview on the Details panel; it never shipped, and the comments being tab-only is the half that was left. Dave ruled the full move. One component cannot render in two DOM locations, so ItemTimeline stays the SINGLE owner — one fetch, one SSE subscription, one composer — mounted under the content on Details rendering comments, and mirrors its feed out through a new bindable `feed` prop. The Activity and Versions panels render that same feed through a second TimelineEntryList. - The mirror publishes the WHOLE feed, not the owner's rendered slice. The owner renders comments only, so publishing `visibleEntries` would leave both tabs permanently empty with nothing to report. Tested, and the one-word mutation fails it. - `loadMore` rides in the mirror: pagination is a property of the ONE feed, and a tab that can show older entries but not ask for them is a dead end. - The kind partition is three shared constants with an exhaustiveness check, not literals at the mount sites. A kind in none of them renders NOWHERE — which is how note/decision shipped invisible the first time (BUG-2301). Adding a kind to TimelineEntry without routing it is now a build error or a failing test rather than a silent hole. - The comments section carries its own {#key itemSlug}: it left the block that used to provide that remount, and dropping the guard would have been invisible. It wraps only the timeline — the collab editor must never be keyed. Five e2e specs asserted comments behind the Activity tab and are updated. attachment-lifecycle's tab round-trip is preserved deliberately: its claim is that the panel is CSS-hidden rather than unmounted, so it now goes out to Activity and BACK rather than asserting against a hidden panel. Gates: 117 files / 1987 tests pass, svelte-check 0 errors. Both new properties have negative controls — publishing the rendered slice fails 1, unrouting a kind fails 2. Claude-Session: https://claude.ai/code/session_011Q4b1iHtJtSyMs7BA2ySxo |
||
|
xarmian
|
f932469380 |
refactor(web): extract TimelineEntryList from ItemTimeline, no behaviour change (IDEA-2843)
Comments move under the item content on Details while Activity keeps changes and Versions keeps versions, so the one feed has to render in two DOM locations. One component instance cannot be in two places, and the constraint on this work is ONE subscription and ONE composer — so the rendered list becomes its own presentational component and ItemTimeline stays the single owner of fetching, SSE, pagination, the attachment probe, the paint fence and every mutation. This commit is the extraction only. Nothing moves location and no behaviour changes; the second mount site is the next commit. - TimelineEntryList.svelte: the entry loop, rail chrome, the five card branches and their CSS, lifted verbatim. `listEl` is bindable because the owner's delegated lightbox listeners and imperative image-a11y pass attach to the container and stay with the owner. `showEmpty` is passed rather than derived from the rendered entries, preserving the owner's condition over the WHOLE feed — a tab that filters everything out must render an empty list, not claim the item has no history. - The comment-card callbacks are optional here with no-op defaults: a list rendering no comments has nothing to hand them, and a card that could call one only renders when the owner supplied the real handler. Evidence, and the reason it counts: the existing suite passes UNCHANGED — 116 files, 1983 tests — and svelte-check reports 0 errors. A refactor whose only claim is "nothing changed" is exactly where an untouched suite is the right instrument, but only if it actually exercises the moved markup. It does: rendering the list over an empty array instead of `entries` fails 64 tests across 7 files. Claude-Session: https://claude.ai/code/session_011Q4b1iHtJtSyMs7BA2ySxo |
||
|
xarmian
|
a2bdd904a5 |
docs,test(web): correct two claims the mutation matrix refuted (IDEA-2843)
Both corrections are to MY OWN rationale, not to behaviour. - The setContent-over-insertContentAt comment read as a defect avoided. Measured: swapping to insertContentAt leaves all five tests green, so both routes preserve the blockquote today. Restated as what it is — a preference for not depending on normalizeInline's leading-<p> rule — and marked explicitly unenforced. - The explicit `empty = editor.isEmpty` was inert: dropping it leaves the suite green, because setContent emits an update by default and onUpdate maintains the flag. Removed. The test asserting submit becomes enabled is the real guard, and it goes red if a tiptap bump flips that default. The test file's claim that its blockquote assertion catches an insertContentAt implementation was false for the same reason; it now states what the assertion does catch (a genuine flatten, verified) and what it does not. |
||
|
xarmian
|
9363dbb749 |
feat(web): imperative appendMarkdown handle on CommentEditor (IDEA-2843)
The selection toolbar's forthcoming Comment action needs to drop a
blockquote of the reader's selection into the ALREADY-MOUNTED composer.
The obvious route is a silent no-op: CommentEditor reads `content` once,
inside `new Editor({...})` in onMount, and has no $effect syncing it, so
writing the prop on a live composer drops the text with no error.
- appendMarkdown(markdown): appends after a blank line when a draft
exists, never replaces; returns false when there was nothing to insert
or no live editor, so a caller can tell 'inserted' from 'did nothing'.
- setContent (block parse) rather than insertContentAt: tiptap-markdown
overrides insertContentAt with { inline: true }, where a blockquote
survives only incidentally.
- A {#key} remount was the ruled-out alternative: it would destroy an
in-progress draft, which is what doSubmit's identity capture
(PLAN-2105 / TASK-2112) exists to protect.
Tests assert the quote TEXT and its blockquote tag, not the composer's
visibility — the broken version opens the composer too.
|
||
|
xarmian
|
704ba874d4 |
Merge pull request #1233 from PerpetualSoftware/fix/bug-2810-nul-repair
fix(store,server,cli): count and repair the legacy NUL population (BUG-2810) |
||
|
xarmian
|
e4415ddd04 |
fix(cli): name the skipped-table suspects instead of counting them (BUG-2810)
Codex round 12, polish rather than a defect. The advisory reported how many values in non-migrated tables mention a NUL escape, and then made the operator run `pad db scan-nul` to learn which — when the rows were already in hand. They are listed now, in the same shape as every other row this command prints. The test asserts the table.column appears rather than only the surrounding phrase, so a regression to a bare count fails it. |
||
|
xarmian
|
d9f3fe3881 |
fix(cli): filtering suspects out of the check also filtered them out of the report (BUG-2810)
Codex round 11. Round 10 stopped probing suspects from tables the migration does not copy, which was right — but it also dropped them from the output, while the comment two lines below still claimed "the others are still REPORTED". A legacy shadowed-NUL in activities.metadata produced no warning at all. They are now COUNTED and named, pointing at `pad db scan-nul` for detail. Counted rather than probed on purpose: whether one is actually fatal can only be answered by the destination, and asking would put them back inside the fail-closed rule the filter exists to keep them out of. The test captures stderr and asserts the advisory appears, and is mutation-verified: suppressing the notice fails it with "the suspect was filtered out of the check AND out of the report". THIS IS THE THIRD TIME on this branch that the same shape has appeared — a filter that is right about what to ACT on quietly becoming a filter on what to SAY. The first was the scan dropping suspects entirely; the second was the preflight refusing on tables it does not copy and then, fixing that, going silent about them. Each fix was correct about the action and wrong about the reporting, and each time the comment stayed true while the code stopped being. Worth naming as the pattern rather than as three unrelated defects. |
||
|
xarmian
|
39a8366060 |
fix(cli): filter suspects by table BEFORE asking the destination (BUG-2810)
Codex round 10, and it is round 9's over-refusal reintroduced through the other path. The fail-closed rule refuses on a suspect that could not be VERIFIED, and it ran over every suspect before MigratedTables was applied — so an unverifiable row in `users`, `sessions` or `activities` blocked a copy that would never have touched it. Filtering first also stops the oracle making round trips whose answer cannot matter. Two things learned writing the test, both worth more than the fix. **The first version of it proved nothing.** It used a nil destination, but the fail-closed branch only runs once there is something to ask, so it passed with and without the fix. The real fixture needs a live destination AND a genuinely unverifiable row: a NULL primary key, which SQLite permits in a declared TEXT PRIMARY KEY and no other engine does. Mutation-verified in the new shape — with the filter back in its old position the test fails with the reported symptom, "1 suspect value(s) could not be checked; nothing was migrated". **Layer B is STRICTER than the shared predicate for this shape.** The fixture would not insert until the triggers were dropped: SQLite's json_tree walks tokens rather than building a map, so it sees the NUL in the shadowed member that our Go predicate cannot. That narrows how such a row can exist at all — it must be legacy data written before the triggers, which is exactly the population BUG-2810 is about. Recorded in the fixture rather than left as a surprise for the next person whose insert is refused. |
||
|
xarmian
|
e89c8c8ab6 |
fix(cli): two ways the preflight and its remedy disagreed with the migration (BUG-2810)
Codex round 9, both confirmed against the code rather than reasoned about. **PAD_DATABASE_URL was treated as proof of a PostgreSQL deployment**, so the flow this unit prescribes broke on itself. cmd_server.go opens PostgreSQL only when PAD_DB_DRIVER=postgres; PAD_DATABASE_URL is ALSO migrate-to-pg's target, and its default. An operator who follows the preflight — refused, told to run `pad db repair-nul`, with the target URL still exported in their shell — got "This deployment is PostgreSQL ... Nothing to scan or repair" and exit 0. The remedy the refusal names did nothing, which is the failure mode this unit has now produced three separate ways. PAD_DB_DRIVER alone decides. Verified by running the real command with the target exported. **The preflight refused on tables the migration does not copy.** ExportWorkspace / ImportWorkspace read six tables, and migrate-to-pg's own help says users, platform settings and auth data are not migrated — so a NUL in users.name blocked a copy that would never touch it, demanding the operator rewrite content unrelated to the migration they asked for. Refusal is now filtered to store.MigratedTables(). Those rows are still REPORTED: `pad db scan-nul` lists them, they are real, and going quiet about a broken row because this command does not care about it would be the information-discarding the preflight was already corrected for once. The table set is pinned by REFLECTION over models.WorkspaceExport's shape, not by a regex over ExportWorkspace's SQL — TASK-2825 already established that multi-line and Sprintf-composed SQL are invisible to any source-level instrument. It fails in both directions: a new export section with no entry (a miss, ending in a half-finished migration) and a spurious entry (an over-refusal). One residual, stated rather than hidden: the export also skips SOFT-DELETED collections and items, and this filter is per-table. A NUL in a soft-deleted item still blocks. Narrowing it needs a per-row deleted_at check at every candidate, which costs more than the remaining over-refusal — the operator's way out is the same single command either way. |
||
|
xarmian
|
d86a499f56 |
fix(store): the SQLSTATE extractor indexed one string and sliced another (BUG-2810)
Codex round 7. sqlStateOf searched strings.ToUpper(msg) for the marker and then sliced the ORIGINAL message at that offset. Correct only while every byte before the marker is ASCII: Unicode case mapping changes byte LENGTH for some runes, and PostgreSQL renders messages in lc_messages, so a non-English server is not a hypothetical. One string now serves both the search and the slice, which also makes the returned code uppercase without a second conversion. Mutation-verified rather than argued: against a message carrying U+0131 (two bytes, uppercasing to a one-byte "I") the old code returns "TE 22" where the code is "22P05". That garbage happens to classify as unavailable — the safe direction — but only by luck; a different offset lands on a spurious "22" prefix and turns a check that never completed into a verdict about the value. The regression leg uses a localised message shape for that reason, and the failure it produces is the one above. |
||
|
xarmian
|
6d4c3b4b75 |
fix(store): an operational SQLSTATE is not a verdict about the value (BUG-2810)
Codex round 6, and it is the round-5 fail-open one level deeper. That round split "the server answered" from "the server did not", and I implemented the first half as "does the error carry a SQLSTATE at all" — which is wrong, because 57014 (query cancelled), 57P01 (terminated by administrator), the 08 class (connection exception) and the 53 class (out of resources) all carry SQLSTATEs while saying nothing whatever about the value. Classified as verdicts, they let the preflight proceed with an UNVERIFIED suspect, which is the exact thing the three-way split was added to stop. The test is now INVERTED: only SQLSTATE class 22 — data exception, PostgreSQL's class for "this value is wrong" — counts as a verdict about the value. `SELECT $1::jsonb` produces 22P02 for malformed JSON and 22P05 / 22021 for the NUL cases. Everything else, code or no code, means the question was not answered, and the caller refuses rather than guessing. Erring toward "unavailable" is the safe direction: its cost is a refused migration an operator re-runs, against a half-finished one they have to unpick. The coverage is split deliberately, and both halves are needed. The operational codes are from PostgreSQL's error-code table, formatted the way pgx renders them, because provoking an administrator shutdown inside a unit test is not worth it. What is NOT assumed is the rendering, or the premise that class 22 is what a bad value yields: the real-server test now extracts the SQLSTATE from a genuine malformed-value rejection and asserts it is class 22 and a completed verdict, and the closed-pool test covers the no-code path. Neither half stands on its own. sqlStateOf's own edges are pinned too — a truncated "SQLSTATE 22" must not yield a partial code that then matches a class prefix, and the marker search being case-insensitive means the extraction has to be as well. |
||
|
xarmian
|
0363c139a9 |
fix(store,cli): the oracle failed open, and it over-refuses one column (BUG-2810)
Three findings from codex round 5, all real; the third corrected a claim I had
made about the design.
**The suspect path could leave data unrepaired and exit 0.** The CLI printed
SuspectsFailed and then returned nil, checking only the violation bucket. A
script sees success; an operator who trusts the status moves on. Both buckets
now decide the exit code, and the decision is extracted into
nulRepairExitError so it is testable without a database — the bug was in the
decision, not in the repair, and a test that needs a fixture to reach it is a
test nobody writes.
**The destination oracle failed open.** Connection failures, timeouts and
read-back errors were bucketed with "the destination answered, about something
else" — reported and not refused on. So an UNVERIFIED suspect passed the
preflight, which is the defect the suspect class was added to correct arriving
by a different route.
There are now three outcomes rather than two: the server answered with a NUL
code (refuse), the server answered with another complaint about the value
(report, because a NUL preflight that quietly grew into a general one would
block migrations unrelated to this bug), and the server never answered
(REFUSE). ErrDestinationCheckUnavailable carries the third, and
TestDestinationOracleFailsClosedOnAnUnusableConnection pins it against a real
closed pool — with an open-pool control first, since a classifier that answered
"unavailable" for everything would satisfy the assertion and refuse every
migration.
**The oracle is not a perfect model of the migration, and I said it was.**
Codex claimed workspaces.settings is normalised on import, so the cast
over-refuses there. Measured rather than argued, by importing the same
shadowed-duplicate value into three columns against a real server:
workspaces.settings -> import SUCCEEDS, stored as {"a": "clean"}
items.fields -> import FAILS, SQLSTATE 22P05
collections.schema -> import FAILS, SQLSTATE 22P05
CreateWorkspace runs models.NormalizeWorkspaceSettings, a map round-trip that
drops the shadowed member. So the claim was right, and my own runtime demo
earlier on this branch — which used workspaces.settings — was showing a
spurious refusal.
The cast STAYS. That row is a value Layer B refuses on every write today and
exists only because it predates enforcement, so surviving the migration is an
accident of one column's normaliser rather than a property worth preserving,
and repair-nul clears it in one command. Deriving "would this column's writer
normalise it" is a per-column enumeration, which is the shape this cluster
keeps proving unmaintainable.
What changed is the CLAIM. The file header no longer says the oracle is "exact
in both directions" — it is exact about the VALUE and is not a model of the
MIGRATION; the refusal no longer tells an operator PostgreSQL would reject the
row, only that the value carries a NUL jsonb refuses; and the measurement and
the over-refusal are written into CheckJSONBAcceptable's doc comment and
docs/backup.md, which also now states that the check errs toward refusing.
The disposition is flagged to the lead rather than settled here: skipping
normalised columns is a scope call, not mine.
|
||
|
xarmian
|
57b7ca5f48 |
feat(store,cli): ask the destination about suspects instead of dropping them (BUG-2810)
Day-54 lead ruling on PR #1233, and the ruling names the defect precisely: the scan's own SQL pre-filter already surfaces the shadowed-duplicate row as a candidate, and `ParameterRefused` then drops it. So the preflight was discarding information it was holding and going on to promise the migration would go through. I had recorded that as an accepted residual on the grounds that closing it would violate DOC-2823's one-layer rule — but that rule is about what the enforcement layers REFUSE. It says nothing about a preflight throwing away a candidate it had in hand. **The SUSPECT class.** A pre-filter hit the predicate does not refuse. Most are doubled-backslash literals — text that writes ABOUT the escape, which is the false positive this whole predicate family exists to avoid. One member is not: a NUL in a value shadowed by a LITERAL duplicate key, which a map-model decode drops and PostgreSQL refuses. Nothing here can tell them apart, so nothing here tries: `pad db scan-nul` lists them under their own heading, apart from the violations, with what resolves them. **The destination is the oracle.** `pad db migrate-to-pg` casts each suspect on the TARGET connection — `SELECT $1::jsonb`, side-effect-free, and the very cast an INSERT performs — and refuses on 22P05 / 22021. That is exact in both directions precisely because it is not a fourth opinion of ours. Measured against a real server: the literal is ACCEPTED, the shadowed duplicate is REFUSED with 22P05, and a non-JSON value fails for a reason that is reported rather than refused on, because a NUL preflight that quietly grew into a general one would block migrations unrelated to this bug. **The repair had to be measured, not assumed, and the answer changed the design.** `textguard.Repair` leaves the shadowed value completely untouched: its scanner is gated on DocumentDecodesNULAnyShape, a map-model question that answers false for exactly this shape, so it never runs. A preflight that refused the row and printed `pad db repair-nul` would have been printing a command that does nothing to it — a remedy nobody ran (PATTE-135). So the repair reaches the class through the token-level scanner, exported for this, which rewrites the shadowed escape and still leaves the literal byte-identical because it consumes escapes in order. Suspects get their own buckets in the repair report rather than being folded into Repaired, so the dry run's promise and the run's result stay the same number. **Nothing about what any layer REFUSES changed.** textguard.KnownGaps and its pin are untouched, and TestScanNULInheritsTheRecordedKnownGaps still asserts the scan does NOT detect the shape. TestSuspectsCollapseWhenBUG2812Lands fails when the token-walk makes that false, and names every file to delete — the suspect path is a second mechanism that exists only while the predicate is blind. **One defect this found that no test did.** Running the real command against a real Postgres, the refusal announced "0 stored value(s) carry a NUL; nothing was migrated" while listing one — the count used the violations only, and the tests asserted the message CONTAINED "nothing was migrated" without reading the number. Fixed, and the assertion now reads the count. The whole loop is now verified end to end: preflight refuses, `repair-nul` fixes, the migration completes. My own prose from earlier on this branch is corrected with it. ScanNUL's doc comment, the preflight's, and docs/backup.md all said this shape passes the preflight and fails mid-copy, which the same commit makes false. |
||
|
xarmian
|
a65252dab1 |
fix(cli): print the NUL report on stdout so it can be captured (BUG-2810)
`pad db backup` and `pad db restore` keep their progress on stderr because stdout may carry the backup itself. These two commands emit no data at all, and their REPORT is the whole point — an operator piping `pad db scan-nul > affected.txt` was getting an empty file and the list on the terminal, which is the opposite of what the command is for. Report to stdout; the confirmation prompt, its warning and the Postgres not-applicable notice stay on stderr, where a prompt belongs. Verified by running the real command with 2>/dev/null and reading the list. |
||
|
xarmian
|
178b6b5010 |
fix(server,store): two more from codex rounds 3 and 4 (BUG-2810)
**The import repair could silently change what gets imported.** It decodes into map[string]any, where a repeated object member keeps only the LAST value. The TYPED decode that runs next does not agree: encoding/json unmarshals members in order into the same struct field, so two `"workspace"` objects MERGE there and collapse here. A body with duplicate members would therefore import differently with --repair-nul than without, which is outside what a flag by that name may do. It now DECLINES such a body: returns it untouched, lets the gate judge it exactly as it would without the flag, and says why in the refusal — "the payload repeats the member X, and repairing it would change which value is imported". Detection is a token walk, because a decode is what loses the information: by the time there is a map the duplicate is gone. The detector's own test carries the false positive that matters — the same member name in SIBLING objects is not a duplicate, and a single shared set of names would decline every real export, since items all carry `id`, `title`, `slug`. Rewriting such a body faithfully wants a token-preserving pass, which is BUG-2812's token-walk and not a rider on this. A real export cannot contain duplicate members (json.Marshal does not emit them), so declining costs nothing an operator meets by accident. The tally now owns the repair — decodeJSONRepairingNUL takes it and calls Apply — so the count and the declined reason come back through one object instead of a return value a caller has to remember to record. That is the same mistake this branch already made once, when the JSON path dropped the count and the header reported 0 for an import that had rewritten a value. **A row the repair could not address was reported as a failure.** A NUL in a key column the list does not protect, on a row whose violation is elsewhere, makes the address unbindable: Layer A inspects every bound parameter, including a WHERE clause's, so the lookup is refused before SQLite is asked to find the row. It landed in Failed carrying "invalid text parameter: parameter 2" — the same information phrased as a fault in the repair rather than a property of the row. Now detected up front and reported as a skip with the reason, alongside the two skips that already existed. **One finding NOT fixed, deliberately, and recorded instead.** Round 3 raised that the scan misses a NUL in a value shadowed by a LITERAL duplicate key, so such a row passes the migrate-to-pg preflight and then fails during the copy — the exact failure the preflight replaces, surviving for one shape. That is textguard.KnownGaps: a blind spot every layer shares on purpose, which DOC-2823 forbids closing in one layer alone, because layers disagreeing about one value is the defect this cluster is made of. So it is named in ScanNUL's doc comment, in the preflight's, and in docs/backup.md for the operator, and TestScanNULInheritsTheRecordedKnownGaps pins the miss and FAILS when it stops being one — the notification that BUG-2812 has landed and those three prose sites need updating. The consequence is recorded on BUG-2812's trail. Round 2's single finding was refuted rather than fixed: it predicted TestRepairFlagReachesTheNestedAndObliqueForms would fail, on a mechanism that describes the raw-byte scanner this branch had already replaced. The test passes; the outer decode resolves the oblique spelling before the walk sees it. |
||
|
xarmian
|
49bd342e4c |
fix(store,server,cli): three defects from codex round 1 (BUG-2810)
**The import flag could not repair the column it exists for.** `--repair-nul` scanned the RAW body for a live escape, which is right for a value the gate reads at the top level and wrong for the one that actually matters. An item's `fields` blob travels through an export as a STRING: a NUL escape in the stored blob marshals into the body with a DOUBLED backslash, which a raw scan must leave alone because at that layer it is literal text — while the gate refuses it anyway, since it decodes the body and re-parses that string as the document it is. So the repair now walks the DECODED body with the same classing bodyDecodesNUL uses, one verb changed: where the gate asks textguard whether a value decodes to a NUL, this asks textguard to repair it. Two walks of one shape in one package is a real risk, and the mitigation is that they are measured against the same corpus in both directions rather than reviewed for similarity — TestBodyRepairMirrorsTheGateOverTheCorpus drives every case through the body shape and asserts refused-becomes-accepted and accepted-stays-byte-identical. Two consequences worth stating. The walk also reaches the OBLIQUE spelling — the backslash written as its own escape, so the six characters never appear in the raw bytes at all — which the scanner could not, so the test that pinned that limit is replaced by one asserting the capability. And re-encoding is now possible, so it is bounded: UseNumber, so an integer wider than float64 is not silently re-emitted in scientific notation; SetEscapeHTML(false); and a body with nothing to repair is returned byte-identical rather than round-tripped. The mutation that removes UseNumber turns 9007199254740993 into ...992, and a test says so. The header is now X-Pad-Repaired-NUL-Values, because at the decoded layer an escape is not a thing that exists any more and one nested document may have carried several. **The scan could not run on the databases it exists for.** Several protected tables carry a NULLABLE workspace_id — activities, api_tokens, mcp_audit_log — and the scan selected it into a plain *string, which fails with "converting NULL to string is unsupported" and takes the scan, the repair and the migrate-to-pg preflight down with it. Every column is now scanned as sql.NullString: SQLite also permits NULL in a declared PRIMARY KEY that is neither INTEGER PRIMARY KEY nor NOT NULL, which no other engine does, and a NULL key cannot address a row for an UPDATE — such rows are reported and skipped with the reason rather than handed a WHERE that matches nothing. Verified against the unfixed code: the scan returned `scan activities.actor row: sql: Scan error ... converting NULL to string`. It needed a VIOLATING row in such a table, which is why every fixture that planted its rows in `items` missed it. **--force by accident.** The repair skipped the running-server check whenever --from was given — and the most natural --from an operator types is the path `pad db scan-nul` just printed, which IS the live database. The check is now on the resolved path (Abs + EvalSymlinks, so a symlinked data directory or a relative path still matches), and a --from naming an unrelated backup stays unguarded, which is correct: nothing is writing it. The ordering moved with it. `store.New` runs pending migrations, so the refusal now happens BEFORE the database is opened; opening first and refusing second made the guard arrive after the thing it guards against. |
||
|
xarmian
|
63da2f4f5f |
feat(store,server,cli): count and repair the legacy NUL population (BUG-2810)
Layers A and B stop the value being written. Neither makes a row that
already carries one go away, and BUG-2810's filing is what that costs: an
affected workspace exports with a 200 and re-imports with a 400, so a
self-hoster restoring their own backup is blocked with no path forward in
the product, and `pad db migrate-to-pg` fails partway through the copy
against PostgreSQL's jsonb parser rather than up front.
This is DOC-2823's S3, on Dave's day-54 rulings: U+FFFD as the replacement,
repair standalone only with a migrate-to-pg preflight that refuses and
prints the command, `--repair-nul` on import shipping default-strict.
ONE REPAIR, beside the one predicate. textguard.Repair lives next to
ParameterRefused because four layers that agree about what is REFUSED and
disagree about what a repair PRODUCES is this bug family arriving one step
later. Its contract is a property over the same corpus, in both directions:
every refused value becomes one all four layers accept, and every accepted
value comes back IDENTICAL. The second half is the load-bearing one — a
repair that tidies values nobody complained about rewrites
`{"a":"x\\u0000y"}`, six literal characters after a doubled backslash, and
corrupts it.
The JSON arm is a string-literal SCANNER, not decode-walk-remarshal, which
is what the recon write-up proposed before it was written. Re-marshalling
changes four things nobody asked to change — object key order, insignificant
whitespace, integers wider than float64, HTML-ish characters — and silently
drops one of a document's LITERAL duplicate keys, which is a gap BUG-2812
owns and the last thing a repair should do. Scanning copies every byte it
does not deliberately rewrite, so an untouched document is byte-identical
without that having to be argued. A substring replace is not equivalent and
the test that proves it took a mutation to find: a doubled-backslash literal
ALONE never reaches the scanner, so the discriminating fixture is one
document carrying a live escape AND a literal.
THE COUNT IS COMPUTED IN GO. Measured on the read path in this worktree: a
row planted with `bad<NUL>name` reads back into a Go string with all 8 bytes
and the NUL intact, while `length(name)` in the same database answers 3.
TASK-2824 found that C-truncation and concluded no DB-side REPAIR could be
trusted; the same measurement on the read path says no DB-side COUNT can be
either. SQL narrows — `instr(col, char(0))`, plus the escape prefix on
JSON-classed columns, which is textguard's own pre-filter — and never
decides. The decision stays ParameterRefused with isJSON from the shared
86-column list, i.e. Layer B's classing.
Row addressing is read from the live schema rather than a hand-kept map:
39 tables carry protected columns, one (item_wiki_links) declares no primary
key and is addressed by rowid, two have composite keys, and five have a
single key that is not `id`. The repair checks RowsAffected because an
address that stopped selecting its row would otherwise commit an UPDATE that
touched nothing and report it as repaired — the one failure an operator
cannot see in the output.
`email_optouts(email)` is both a protected column and its own primary key.
Repairing it changes the row's identity and can collide with an existing
row, which in that table means somebody starts receiving mail again. It is
reported and skipped, with the reason.
The import flag is NOT an exemption from the gate. `--repair-nul` buys the
body one repair attempt and then runs the same `bodyDecodesNUL` on the
repaired bytes, which still decides — a decode path that skipped the check
is the door BUG-2803 spent thirty rounds closing, on the endpoint carrying
the largest attacker-controlled body in the product. Only the ESCAPE form is
repaired: a raw NUL byte makes the document invalid JSON, and widening what
parses is not this flag's job. Both doors are covered, JSON and tar.gz,
because giving them different answers is how one of them keeps being
forgotten.
Postgres is settled with evidence rather than sent up as a ruling: it cannot
hold either defect (22021, 22P05) and the four-way differential test already
pins that, so the scan reports not-applicable WITH the reason rather than
returning a zero a reader could mistake for a clean database.
Spellings settled here, per the dispatch: `pad db scan-nul` and
`pad db repair-nul` as siblings rather than `repair --nul`, matching
`migrate-to-pg`'s hyphenated compound — a repair verb that errors when given
no flag is a worse shape, and there is no second repair to share it with.
scan-nul IS the dry run, so repair-nul grows no --dry-run. It refuses while
the server is running unless --force, on the `pad db restore` precedent: the
report is a claim about a database, and one somebody else is concurrently
writing makes it a claim about a moment that has passed.
docs/backup.md's section on this is rewritten. It still said the rule lives
in the binary and not the database, which S2 made false, and it pointed at
this item for a preflight and a repair that now exist. Its import examples
also showed `pad workspace import < file`, which has never worked — the file
is an argument.
Closes BUG-2810.
|
||
|
xarmian
|
ebd1886ada |
Merge pull request #1232 from PerpetualSoftware/fix/bug-2827-outbox-bound
fix(store,server): bound the event outbox's writes, claims and scrub (BUG-2827) |
||
|
xarmian
|
2dde493305 |
chore: ignore the Go build cache the codex sandbox leaves in the checkout
codex exec's sandbox runs go with GOCACHE inside the working directory, at .tmp-gocache/. On this branch a git add -A after a review round swept 5,688 of those files into a commit (created 01:44Z during round 4, committed 02:16Z); the two affected commits were rewritten without them before the PR. Ignoring the directory means the next seat's add -A cannot repeat it. |
||
|
xarmian
|
f54a0e41d4 |
docs(store,server): four comments and one log line that had stopped being true (BUG-2827)
Codex round 6. No logic finding; it confirmed the refusal ordering, the
split-budget claim and the first-tick scan as sound. Five statements in
the branch's own prose were untrue of the code as it stands:
- MaxOutboxPayloadBytes' comment still argued from 64 MiB ("two orders
below both ceilings") after the constant became 128 MiB, which is 4x
under the lowest ceiling, not two orders.
- maxOutboxClaimBytes' comment counted a scan-into-string-then-copy
transient that round 1 removed; the scan lands straight in []byte.
- maxOutboxClaimRows' comment used the item BODY mean (~2.4 KB) as the
payload mean; the measured payload mean is ~3.5 KB, so 5,000 rows is
~17 MiB, not ~12.
- emitBulkItemEventTx's early-out said the numbers the caller sees still
come from writeOutboxTx; when the early-out fires they come from the
projection, and the error says so in Measured.
- The drain's oversized-row log said "not claimed". OversizedPendingOutbox
filters only dispatched_at, so during a rolling upgrade a binary older
than the ceiling may be holding a claim on the row it names. The line
now states what the query establishes: this instance will not claim it.
The doc says why claimed_at is deliberately not a filter.
|
||
|
xarmian
|
0836808ca5 |
fix(store): drop a past-the-hop-bound event before judging its size, and correct three comments (BUG-2827)
Codex round 5. No production defect found; one ordering edge and three comments that had stopped being true. THE ORDERING. writeOutboxTx has two refusals that disagree about the mutation. The hop bound drops the event and lets the mutation stand, because only the cascade it would extend is illegitimate. The size cap fails the mutation, because there the mutation and the event are the same fact. An event that trips BOTH was judged for size first, so it failed a mutation over a row that was never going to be written. The hop drop now comes first. Unreachable today - nothing propagates a hop - which is exactly why the ordering is worth pinning before something does. TestAnOversizedEventPastTheHopBoundIsDroppedNotRefused fails with the two checks swapped back (run before the crash that interrupted this round, and again on this tree). THREE COMMENTS. The drain-limit constant said whole batches are claimed past it; the byte budget and row cap can now split one. The claim candidates' doc said every sibling; it is as many as the budget still allows. OversizedPendingOutbox's doc named the write cap while its query uses the claim ceiling, and said it ran every tick when the caller throttles it to once per five minutes. Gates: gofmt clean, golangci-lint 0 issues, full SQLite suite exit 0, full Postgres suite exit 0. |
||
|
xarmian
|
57caa7f92e |
docs,test(store): correct five comments and strengthen the shrink fixture (BUG-2827)
Codex round 4. Its one P1 does not hold, but the test it named as weak genuinely was, and five comments in this branch had drifted from the code they describe. THE P1, CHECKED RATHER THAN ARGUED. The claim measures size at candidate selection and never rechecks it, so a payload that GREW during a concurrent scrub could be claimed over the ceiling. The proposed growth path was Go's HTML escaping: json.Marshal writes < as its six-character unicode escape, where the source had one byte. Measured against Postgres 16: a one-key object whose value holds the four characters x<y>z&w, written with those characters LITERAL -> 16 bytes the same object written with < > and & as their six-character JSON unicode escapes instead -> 16 bytes Postgres parses the escapes and stores the characters, so the escaped and literal forms are the same size and the round trip cannot grow the row. On SQLite the payload is stored exactly as Go wrote it, so re-marshalling is idempotent. The rejection from round 3 stands, now on a measurement instead of an assertion about key removal. But the test defending it was weak, and codex was right about that: its fixture was one repeated ASCII letter, which cannot tell any of these encoder paths apart. It now carries <, >, & and non-ASCII, so it exercises the divergence rather than asserting past it. Mutation note worth keeping: a whitespace-padding mutant is caught on SQLite and NOT on Postgres, because jsonb discards insignificant whitespace — the mutant does not actually grow the stored row there. The faithful mutant adds a key, and that one dies on both. FIVE COMMENTS THAT SAID SOMETHING UNTRUE, all introduced by this branch: - OversizedOutboxPayloadError was documented as the write cap's error; three sites raise it, against two different limits. - "The two things Measured can name" listed three. - measuredStoredRow said "as the database stored it", but OctetLength measures what the driver hands back, which on Postgres is the ::text rendering rather than storage. - OversizedPendingOutbox described its threshold as the write cap while querying the claim ceiling. - ClaimPendingOutboxEvents still said batches are claimed whole, which the byte budget and row cap deliberately interrupt. Tests also now assert Measured at all three refusal sites — without it the field could be blank everywhere and every existing assertion still passes — and the claimability property test's refusal branch checks the row actually rolled back, so "refused" cannot be satisfied by a write that committed anyway. Gates: gofmt clean, golangci-lint 0 issues, full SQLite suite exit 0, full Postgres suite exit 0. Claude-Session: https://claude.ai/code/session_01XLtX4dbjBpApbAv3SuBcTm |
||
|
xarmian
|
213142c4b5 |
fix(store,server): honest refusal figures and a throttled oversized scan (BUG-2827)
Codex round 3. Two of three findings acted on, one rejected with an invariant test in place of the change it asked for. REFUSAL FIGURES SAID SOMETHING FALSE. The store refuses on two different measurements against two different limits — the member content before marshalling, against the write cap, and the row exactly as stored, against the claim ceiling — and both reported the number as "a %d-byte payload". For the first that is untrue: projectedBulkPayloadBytes is explicitly a lower bound, so the message named a size the payload did not have. A caller seeing two different numbers for one mutation had no way to reconcile them. OversizedOutboxPayloadError now carries what it measured, and both the error and the 413 say so. THE DIAGNOSTIC WAS THE MOST EXPENSIVE THING THE DRAIN DID, and it was most expensive when it found nothing. OversizedPendingOutbox has a non-sargable size predicate and no index to help it, so an empty result means evaluating octet_length over every pending row — on Postgres, detoasting and serializing each JSONB payload — and it ran every 5s tick. Now throttled to once every 5 minutes. Latency is the cheap thing to spend here: the rows it reports are permanently unclaimable and sit until the 7-day retention takes them, so a five-minute alarm delay changes no decision anyone makes about them. The first tick after a restart still scans, so an existing oversized row is reported promptly. REJECTED: that the claim needs to revalidate size, because a concurrent scrub could grow a payload between candidate selection and the claim UPDATE. It cannot. scrubOutboxRowTx is the only UPDATE of payload in the tree and it removes keys and re-marshals compactly, so a rewrite is strictly smaller — and shrinking is harmless, since a row judged claimable stays claimable. That is load-bearing for the claim needing no revalidation, so it is now stated at the function and pinned by TestScrubOnlyEverShrinksAPayload rather than left as an assumption for the next person adding a payload rewrite to break silently. The throttle test found its own gap on the way in. Written first against the helper, it stayed green when the call site was mutated to `if true` — a helper nothing calls is still correct in isolation. TestOutboxDrainTickConsultsTheThrottle covers the call site through the stamp the tick leaves behind. Also caught by the gate rather than by review: the field carrying the throttle clock landed on outboxDrainSettings as well as outboxDrainConfig, because the edit matched a line both structs have. Tests passed with both; lint named the dead one. Gates: gofmt clean, golangci-lint 0 issues, full SQLite suite exit 0, full Postgres suite exit 0. Mutants: growing the written payload kills TestScrubOnlyEverShrinksAPayload, removing the throttle call site kills TestOutboxDrainTickConsultsTheThrottle. One mutant discarded as unfaithful — corrupting the payload BEFORE the compare-and-swap is neutralised by the retry, which re-reads and redoes the work correctly, so it tests the retry rather than the invariant. Claude-Session: https://claude.ai/code/session_01XLtX4dbjBpApbAv3SuBcTm |
||
|
xarmian
|
46a551aa4b |
fix(store): refuse an outbox row on its STORED size, not its Go size (BUG-2827)
Codex round 2. One material defect, and the measurement that settled it
invalidates an argument the previous commit leaned on.
I had claimed the Postgres JSONB text expansion was bounded near 1.4x —
whitespace after colons and commas — which is why a 2x claim ceiling was
said to guarantee that anything writable is claimable. That is wrong.
Postgres reparses JSON numbers as `numeric` and prints them positionally,
so the expansion has no ceiling at all. Measured against Postgres 16:
{"a":1,"b":2} 13 bytes -> 16 (whitespace only, ~1.2x)
{"a":1e-100} 12 bytes -> 109 (~9x)
{"a":1e-3000} 13 bytes -> 3009 (~231x, exponent free to grow)
So no multiple of the write cap is a safe claim ceiling, and the Go-side
cap does not bound the stored row at all. Reachable rather than
theoretical: item payloads carry `fields` as a JSON *string*, whose
contents are escaped text and immune, but a bulk delta is a
map[string]any and a numeric field value from a request body arrives as
a float64 that re-marshals in exponent form. The failure it produced was
a row accepted by the write and then excluded from every claim for the
rest of its retention window — written, undeliverable, visible only as
an oversized-row log line.
Fixed where the number is actually true: the INSERT now RETURNs
octet_length of the stored payload and refuses against the claim
ceiling, rolling the caller's transaction back exactly as the
pre-marshal check does. "A row this binary wrote is a row this binary
can read back" is now established by construction instead of inferred
from an expansion argument that did not hold.
MaxOutboxClaimableBytes keeps its 2x value but loses its false
justification: its job is only to leave ordinary payloads room above the
write cap so the two rules do not fight over rounding.
Test gaps from the same round, all three closed:
- The claim-ceiling invariant was pinned only by a Postgres round-trip,
so a ceiling collapsed back to the write cap passed every default
(SQLite) run. The constants test now asserts the relation directly and
fails on either dialect.
- TestBatchSiblingQueryIsBoundedInSQL drives claimableBatchSiblings
directly: bounding the batch in the caller instead of in SQL passed
every assertion on the claim's return value while keeping exactly the
unbounded allocation the row cap was added to remove.
- The scrub's byte budget changes peak memory and nothing else, so
removing it left every outcome assertion green. A TEST-ONLY
afterOutboxScrubBatch seam makes batch count observable, which is the
one visible consequence of the budget working.
Codex also confirmed the previous round's rejected finding: keyset
paging covers every row present at the initial scan and additionally
catches later commits sorting above the cursor, so it is a superset of
the unbatched behaviour rather than a regression.
Mutation matrix, run on BOTH dialects: dropping the stored-size refusal
kills TestEverythingWrittenIsClaimable on Postgres only (correctly — it
is a Postgres defect); dropping the sibling SQL LIMIT and collapsing the
claim ceiling kill their tests on both; dropping the scrub byte break
kills TestScrubSpendsItsByteBudgetNotJustItsRowLimit.
Gates: gofmt clean, golangci-lint 0 issues, full SQLite suite exit 0,
full Postgres suite exit 0.
Claude-Session: https://claude.ai/code/session_01XLtX4dbjBpApbAv3SuBcTm
|
||
|
xarmian
|
37f26f5430 |
fix(store): close the outbox bound's remaining unbounded paths (BUG-2827)
Codex review of the previous commit. Four real defects, one finding answered rather than acted on, and one test that did not discriminate. ROW CAP ON THE CLAIM. A budget in bytes alone does not bound the per-row cost, and the batch scan is the path that shows it: siblings are collected past the row limit by design, so a batch of a million tiny rows sits comfortably inside 64 MiB while its ids, maps, OutboxEvent structs and folded delivery do not. maxOutboxClaimRows (5,000) binds only that shape — at the measured ~2.4 KB payload mean it is ~12 MiB, well inside the byte budget, which stays operative for real traffic. REFUSE BEFORE MARSHALLING. writeOutboxTx could only see the payload after json.Marshal had built it, so a member set large enough to be refused was large enough that rendering it to be refused was its own memory event. emitBulkItemEventTx now charges the members' own bytes first. It is an early-out, not a second rule: JSON only adds, so the projection is a lower bound and can never refuse something the real check would accept, and the authoritative numbers still come from writeOutboxTx. Pinned by asserting the reported size equals the projection exactly — any weaker assertion passes against the code this guards, because the late check refuses with the same error type. ONE COPY, NOT TWO. outboxEventsClaimedBy scanned each payload into a string and then converted it to []byte. On the largest single row a pass may take, that was the difference between one copy and two. A COMMENT THAT WAS FALSE. ScrubOutboxUserRefsTx's note called its subject_id arm indexed. Migrations 081/082/083 index (occurred_at,id), dispatched_at, (workspace_id,occurred_at), batch_id and (claimed_at,occurred_at) — nothing on subject_id. Corrected to say the scan it actually is. CONSIDERED AND REJECTED: that keyset paging over random uuids can miss rows. It cannot miss a row that existed when the scan began — ids are fixed, the walk is ascending over every matching row above the cursor, and a row stops matching only once scrubbed. It changes concurrent commits in the SAFE direction: the single query missed everything committed after it, while this catches those sorting above the cursor, so coverage is a superset of the unbatched version. Snapshotting all ids first would make the window describable without reference to uuid order, and was tried and reverted: it holds every matching id at once, which is an unbounded allocation of the same shape this change removes. The reasoning is now in the comment so the next reader does not re-derive it. Also accepted as residuals, both documented at their constants: a row taken alone because it exceeds a whole pass's budget still costs that pass its size, and the scrub reads one oversized legacy payload whole. Both are the price of delivering and erasing data that exists; refusing either is data loss rather than a bound. Gates on this tree: gofmt clean, golangci-lint 0 issues, full SQLite suite exit 0, full Postgres suite exit 0. Mutation matrix re-run for the new guards: dropping the row cap kills TestOutboxClaimStopsAtTheRowCap, dropping the early-out kills TestBulkEventIsRefusedBeforeItIsMarshalled, dropping the scrub cursor advance still kills TestScrubOutboxUserRefsTerminatesOnLikeFalsePositives. Claude-Session: https://claude.ai/code/session_01XLtX4dbjBpApbAv3SuBcTm |
||
|
xarmian
|
5b12d5eeeb |
fix(store,server): bound the outbox's unbounded reads and writes (BUG-2827)
item.bulk_updated marshals every cascaded member body into ONE
event_outbox row, and nothing bounded either the row or the drain's
reading of it. The v1 doc called the size deliberately unbounded and
named a follow-up condition; measurement met it.
MEASURED FIRST, against an 8,434-item instance:
widest wiki-title cascade 23 members / 175.6 KiB of bodies
widest wiki-ref cascade 50 members / 365.5 KiB
worst option rename 4,429 members / 7.34 MiB
(tasks.status="done")
marshal ratio ~1.46x (item.updated payload mean 3,518 B
against an item body mean of 2,416 B)
So renaming one status option on that instance emits ~11 MiB in a single
row today, from a user clicking rename in the collection editor. That
kills the obvious design: a write cap tight enough to bound the drain's
memory refuses routine work, and one loose enough for routine work
bounds nothing. The two therefore became two numbers.
WRITE CAP (128 MiB) in writeOutboxTx, the single INSERT INTO
event_outbox in the tree, so all seven emit paths inherit it rather than
an enumerated site list. Refusing FAILS the mutation: the hop bound in
the same function drops the event and keeps the mutation, correctly,
because there the cascade is what is illegitimate, while here the
mutation and the event are the same fact. 128 rather than 64 MiB so it
clears MaxItemRenameCascadeBytes — a cascade squeaking under that 64 MiB
bound marshals to ~96 MiB, and a 64 MiB cap here would let the vaguer
refusal preempt rename_cascade_too_large on the very renames that bound
describes. Surfaces as 413 event_payload_too_large, following that
precedent rather than inventing a second spelling.
CLAIM BUDGET (64 MiB per pass), spent by the primary candidates AND by
batch siblings. Spending it on siblings knowingly relaxes "batches are
claimed whole": read literally that rule makes the budget bypassable by
construction, since one large batch is an unbounded read no row limit
touches, and groupOutboxDeliveries already defines the split. The first
candidate is always taken whatever it costs, so the bound meant to keep
the drain alive cannot starve a row instead.
CLAIM CEILING (2x the write cap), and it is NOT the write cap — a
distinction the Postgres leg had to teach. The cap measures the Go bytes
json.Marshal produced; the claim measures what the driver hands to Scan,
which on Postgres is the JSONB ::text rendering, one space inserted
after every colon and comma. A 40,000-byte payload reads back as 40,001.
Thresholded at the same number, a payload written at exactly the cap was
admitted by the guard and then permanently excluded by the claim:
delivered to nobody, reported as nothing, reaped seven days later.
TestARowWrittenAtTheCapIsStillClaimable fails on Postgres and passes on
SQLite against the 1x version.
Rows above the ceiling are excluded IN THE PREDICATE, not filtered in
Go — filtering in Go leaves them occupying candidate slots and starves
everything behind them, which is the same jam wearing different clothes.
They are logged every tick and left pending for the existing 7-day
undispatched retention, not stamped dispatched_at, which would record
that an event went out when it did not.
SCRUB. ScrubOutboxUserRefsTx collected every LIKE-matching payload at
once — the same unbounded read through a different door. Now batched by
a keyset cursor on id. READ FULLY THEN WRITE is preserved PER BATCH, and
the cursor keeps the per-row UPDATEs in ascending id order across
batches, so batching does not quietly reintroduce the BUG-2409 deadlock
it was written to avoid.
Prose sweep: MaxItemRenameCascadeBytes' comment asserted this vector had
no bound, and emitBulkItemEventTx's asserted the payload was unbounded
by decision. Both now say what is true, the second keeping its original
reasoning because it still explains what the bound does NOT do.
Mutation matrix, each mutant compiled and run unfiltered: removing the
row-cap check, > to >=, removing the budget break, removing
always-take-one, removing the SQL size exclusion, removing the sibling
budget, removing the scrub cursor advance, and 2x to 1x — all die, the
last on Postgres only. One survivor recorded as unfaithful rather than
as a gap: setting the scrub cursor to the first row read still advances
monotonically over an ascending id > cursor query, so termination holds
and only the pass count degrades.
Not addressed, named rather than left to be found: a workspace that
outgrows the write cap cannot shrink its own cascade, so the refusal
leaves no recourse. The answer is chunking one bulk event across rows
sharing a batch_id, which the drain's fold already supports.
Claude-Session: https://claude.ai/code/session_01XLtX4dbjBpApbAv3SuBcTm
|
||
|
xarmian
|
ebe40de932 |
feat(store): add a dialect accessor for a column's scanned byte length
Dialect.OctetLength renders the byte length of a text-ish column as the driver will hand it to Scan. The "as Scan will see it" part is the whole reason this is a dialect method rather than a literal in one query. event_outbox.payload is TEXT on SQLite and JSONB on Postgres; octet_length has no jsonb overload, so Postgres needs a ::text cast, and that cast renders the PARSED value — whitespace normalized, keys reordered, duplicates collapsed — which is not the byte count that was written. Every caller of this is deciding whether it can afford to Scan a value, so the scanned count is the one they need and the stored one would quietly mislead. No behaviour change on its own; BUG-2827 is the first consumer. Claude-Session: https://claude.ai/code/session_01XLtX4dbjBpApbAv3SuBcTm |
||
|
xarmian
|
6767ac7210 |
Merge pull request #1231 from PerpetualSoftware/feat/s2-nul-layer-b
feat(store): make the NUL invariant a property of the database (DOC-2823 S2) |