mirror of
https://github.com/PerpetualSoftware/pad.git
synced 2026-09-22 18:43:45 +00:00
eca31efa9e95e98587bc1ef4fbf78d45cbe672e2
472 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 xarmian
|
0d0f1c9125 |
fix(items): require and bound item titles at every write door (BUG-2833, BUG-2831)
`PATCH {"title": ""}` was accepted and applied while `POST` refused the same input
with 400 "Title is required": the guard was an inline literal inside
handleCreateItem, so the sibling handler on the same field never had it. Item
titles were also unbounded, and the slug derives from the title with no
truncation, so the same input was accepted on SQLite and refused by Postgres at
the UNIQUE(workspace_id, slug) btree with an unmapped SQLSTATE 54000 — a latent
`pad db migrate` failure as well as a create-path one.
One models.NormalizeItemTitle / models.ValidateItemTitle pair now backs every
door, enforced authoritatively in store.CreateItem and store.UpdateItem so a
future door inherits the rule rather than having to repeat it. The handlers keep
a pre-lock copy that REFUSES ONLY: it may answer 400 early and must not alter
the input, because its view of the row predates the write lock.
- trim: whitespace-only titles are refused, widening the create door. Artifact
import already trimmed while create tested == "" exactly, and its comment
claimed to mirror the gate it was stricter than.
- bound: 255 runes, matching MaxDocumentTitleRunes but justified for items —
slugify emits only [a-z0-9-] at one byte per rune and truncates nothing, so
255 runes bounds the slug well under the btree index-tuple cap. That cap is
2704 bytes in practice, not the 8191 the filing quoted; both figures and the
readings behind them are in the constant's comment.
- non-retroactive: a title identical to the stored one is not a rename, is not
validated, and is dropped rather than re-applied — so rows predating the bound
stay editable and a no-op echo cannot move an item's slug.
- import coerces rather than refuses (empty -> "Untitled", over-long ->
truncated, both logged, colliding truncations resolved), matching
coerceJSONForImport's recorded disposition three lines away. Refusing would
break restoring archives of data this product already accepted.
- cross-workspace copy propagates a legacy source title, by ruling. It takes no
title from the caller, so it cannot mint one.
The guarantee that holds across every path is narrower than "every stored title
satisfies the bound", and the comments say so: no CALLER-SUPPLIED title is
stored without being validated.
Seven codex rounds, 23 findings, ending CLEAN. Two of the findings were defects
introduced by earlier fixes in this same unit — an empty-title hole opened
through the legacy-protection clause, and a handler-side decision that dropped a
concurrent rename — both recorded on BUG-2833's trail. 38 mutants; every
behavioural fix has a mutant that is the defect at its site, killed by a named
test.
Prose sweep per CONVE-23: three comments asserting item titles are unbounded,
and a cost model resting on a ~2 MiB single-request title, corrected in place —
the guards they document still hold, because the bound is non-retroactive and
the cascade charges STORED titles.
Filed rather than bundled: BUG-2836, BUG-2839, BUG-2840, BUG-2842.
Closes BUG-2833, BUG-2831.
Claude-Session: https://claude.ai/code/session_01XLtX4dbjBpApbAv3SuBcTm
|
||
|
xarmian
|
21b4d8d9c4 |
fix(store,links): remove both quadratics in the item rename cascade and bound it (BUG-2804) (#1224)
* test(store): measure the item-side rename cascade amplification (BUG-2804) Measurement only — no production code changes, no cap. The dispatch is measure-first, and BUG-2798's arithmetic is explicitly non-transferable to this path, so nothing here reuses its numbers. internal/store/items_rename_probe_test.go sweeps three axes with a k=0 negative control and a precondition that fails if the cascade would not be exercised (an empty cascade would otherwise report a flat line and read as "no amplification"). Instrument is MemStats.TotalAlloc, the same counter documents_rename_bounds_test.go uses, so the two paths compare directly. What it establishes, all measured on this machine: - 2.01x the body allocated per bracket, linear across four intervals (1 -> 8 -> 64 -> 512 -> 4096 brackets, agreeing to three significant figures). Bracket count is not independent of body size: the cheapest link is 5 bytes and nothing caps links per item, so the cost is O(C^2) in ONE linker's body, in ONE request, with no accumulation needed. Confirmed directly: doubling C multiplies allocation by 3.83 / 3.92 / 3.95, converging on 4. - 21.3x-22.2x the body per added linker at 8 brackets each, linear in k. - The outbox payload carries every rewritten body in a single row, at exactly 1.00x the body set, persisted. emitBulkItemEventTx documents this as deliberately unbounded in v1 and invites the measurement. Peak residency is reported as a coarse floor and carries no claim. The sampler sleeps between reads deliberately: ReadMemStats stops the world, and a spin loop turned a sub-second cascade into minutes of wall clock on this probe's first run. * test(store,links): decompose the per-bracket constant into its two sites (BUG-2804) Still measurement only. Checkpoint 2 reported a marginal cost of 2.01x the body per bracket without establishing what the 2 was made of. These probes account for it exactly, and the answer changes the fix shape. - internal/links/probe_alloc_test.go isolates RewriteBracketAt: 1.00x the body per call. So the rewriter is only HALF the per-bracket cost. - items_rename_probe_test.go's SELECT probe finds the other half, and it is not in the rewriter at all. cascadeTitleRename's SELECT joins item_wiki_links to items and projects s.content, returning one row per LINK, each carrying a full copy of the source body. The scan loop de-duplicates by source id only AFTER rows.Scan has allocated a fresh string per row. Measured dead flat at 1.00x the body per row against a SINGLE distinct source: 4096 rows on one 256 KiB item allocate 1.07 GB. 1.00 + 1.00 accounts for the measured 2.01. That makes TWO independent quadratics with different fixes. A single-pass rewriter would leave the SELECT's copy untouched and halve the cost rather than remove it. A third probe tests, and REFUTES, an earlier hypothesis that the second copy was the cascade's `rewritten != newContent` full-body comparison: a different-length new title measures 2.05x against the same-length 2.02x, no drop. The comparison allocates nothing, so it could never have appeared in an allocation counter. Kept as a negative result so the refuted reading is not re-derived. * refactor(links): single-pass RewriteBracketsAt, RewriteBracketAt delegates (BUG-2804 M2) The per-bracket decision moves into one unexported helper; RewriteBracketAt becomes the one-element case so the two cannot drift. Differential tests pin behaviour against the PRE-REFACTOR implementation, frozen verbatim in the test file as an oracle. Comparing RewriteBracketAt to RewriteBracketsAt-of-one would be vacuous now that the former is defined as the latter, so the oracle is what makes the test able to fail. * fix(store): bound the item rename cascade and remove both quadratics (BUG-2804) Three mechanisms, landing together because the first two are factors of one product — either alone still leaves O(C^2) while measuring "2x better". M1: cascadeTitleRename's SELECT no longer projects s.content. It returns one row per LINK, so projecting the body made the driver materialise a full copy per bracket, de-duplicated only after rows.Scan had allocated each one. Measured at 1.00x the body per row against a SINGLE source. Each source's content is now read once. M2: the rewrite is a single pass. links.RewriteBracketsAt splices every recorded bracket with one strings.Builder; RewriteBracketAt is reimplemented as the one-element case so the per-bracket decision has exactly one copy and the public contract is unchanged. M3: MaxItemRenameCascadeBytes (64 MiB) bounds the total linking-item content one rename may process, charged before each body is built so a refusal never allocates the thing it refuses. The number is derived from the live workspace's measured distribution, not inherited from documents.go, and the receipt is in the constant's doc comment. Measured, before -> after: doubling C 3.83/3.92/3.95x -> 2.04/2.21/2.06x (quadratic gone) per bracket 2.01x body -> 0.01x body per linker 21.3x body -> 7.3x body 4096-row SELECT 1,074,850,200 B -> 1,180,856 B (910x) store suite 180.9s -> 86.8s Equivalence is pinned against the PRE-REFACTOR implementation, frozen in the test file as an oracle, over a hand-enumerated behaviour corpus plus 20,000 randomised inputs; and the single pass is pinned against the descending fold the cascade used to perform, over 5,000 randomised inputs with shuffled, duplicated and corrupted offsets. The k-linear retention that remains is in the outbox member snapshots, filed separately as BUG-2827. * fix(store,server): codex R1 findings + a dialect-dependent test instrument (BUG-2804) P1 — ProjectRewrittenLen built a replacement string per bracket to measure it, reintroducing allocate-then-refuse one layer below the cap it feeds. The per-bracket helper now returns the SEGMENTS and never concatenates; projection does length arithmetic only, and the rewrite writes the parts straight into its builder, removing an allocation from the hot path too. P2 — the item rename cascade's refusal had no errors.As arm in handleUpdateItem, so a deliberate, permanent decline reached clients as a 500 implying a retry might help. Now 413 rename_cascade_too_large, composed from the error's typed fields so the internal call path is not published. Pinned by an exact-status test plus a counterfactual that a blanket-413 handler would fail. Checked the sibling write paths rather than assuming the reported one was the population: bulk update and version restore never set Title, so the cascade cannot fire there and this handler is the whole surface. P2 — `applied` counted brackets MATCHED, not brackets CHANGED, so a rewrite reproducing content byte-for-byte would still write the row, bump seq and emit events. Counting changes restores what the pre-fix whole-body comparison did. Cascade reachability is NOT established and the test says so. Separately, found by make test-pg: the refuse-before-building guard used a TotalAlloc ceiling, and the same refusal allocates ~69 MB on SQLite against ~242 MB on Postgres — the ceiling was measuring the driver. Replaced with an exact count of bodies built via a store test seam. The first version of that seam ALSO failed to discriminate: a mutation moving the cap check between the build and the seam call went undetected, so the build and the count are now one function with no third position for the defect to hide in. Gates on this tree: gofmt, go vet, go build ./... clean; go test ./... PASS on SQLite AND on Postgres (own container, private port 5446 — never the shared 5445, whose teardown would kill a sibling seat's container mid-run). * fix(links,server): codex R2 findings (BUG-2804) R2-1 — ProjectRewrittenLen advanced its cursor past a no-op bracket while RewriteBracketsAt did not, so the two disagreed about which overlapping rewrites the guard skips. On `[[A[[B]]]]` with a no-op at 0 and a change at 3, projection reported 10 bytes / 0 applied while the pass produced 13 / 1 — the cascade charged the read and never charged the rewrite it then performed, so the bound leaked on exactly the corrupt and duplicated offsets the defensive paths exist for. Projection now follows the pass, not the reverse: the pass's behaviour is pinned to the descending fold the cascade used to perform, so moving it would have changed cascade semantics under cover of a bug fix. The lockstep property test that pins this needed MIXED per-position target titles — the first version shared one title across a call and passed against the broken code, because the reproducing shape needs a no-op bracket overlapping a changing one, which a shared title cannot express. Cascade rows carry per-row target_title, so mixed is the realistic case. R2-2 — the 413 mapping covered one of THREE places handleUpdateItem reaches UpdateItemWithParentLink. The post-R1 population sweep asked whether other HANDLERS reach the store call and never asked whether this handler reaches it more than once. All three now share writeItemRenameCascadeTooLarge. The collab-edit half had a deeper cause than a missing arm. In applyContentViaCollabOnce's prune-and-direct-write fallback, a failing directWrite() had its error DISCARDED and the original collab error returned in its place, so the caller read a deterministic refusal as a recoverable routing problem and fell through to its own direct write — re-deriving the identical refusal from scratch. Measured 64 rewritten bodies built for one request against an expected 32. Deterministic failures now survive that branch, scoped to the three errors every call site already treats as final (open-children rejection, update conflict, cascade refusal). Everything else still returns the collab error, preserving the graceful-degradation contract the branch exists for. Status alone cannot discriminate the double-work fix — both behaviours end in 413, since the fall-through reaches the plain path's arm — so the store's build observer is now reachable from the server package via an exported test-support setter, and the test asserts the work count. Gates: gofmt, vet, go build ./... clean; go test ./... PASS on SQLite and Postgres; go test -race PASS on internal/links and internal/store with zero data races. Note -race on store runs 705s, past the 600s default timeout. * test(links): make the multi-bracket fold oracle independent (BUG-2804, codex R3) The descending-fold equivalence test built its oracle by folding RewriteBracketAt — which now delegates to RewriteBracketsAt, the function under test. The comparison was therefore circular: it could agree with itself while both diverged from the pre-refactor behaviour it exists to pin. This is the same vacuity I flagged and avoided for the ONE-element assertion, missed one case over in the multi-bracket one. The fold now uses rewriteBracketAtV0, the already-frozen pre-refactor primitive, which is the only genuinely independent oracle available. The test still passes, so the single pass does match true pre-refactor behaviour on every generated input including the overlapping ones — that agreement is now evidence rather than a tautology. Mutation-verified after the change: the fold test kills the dropped-sort and dropped-overlap-guard mutants, two of which it had previously left to other tests. Test-only. `git diff |
||
|
xarmian
|
ba1255881d |
fix(server): refuse a decoded NUL in a JSON request body (BUG-2803) (#1220)
* fix(server): refuse a decoded NUL in a JSON request body (BUG-2803)
The body half of BUG-2782 (path) and BUG-2784 (query). A caller-supplied
string reached a Postgres text parameter, Postgres refused it, and the
handler answered 500 — the honest answer is 400.
WHY THE TRANSPORT RULE CANNOT BE EXTENDED, which is the whole reason this
is a different fix rather than a wider middleware. ValidateQuery works
because a decoded query value is a substring of the raw query with ASCII
substitutions: the bad byte in the raw text IS the bad byte in the value.
That property fails for a JSON body — the reachable NUL arrives as the
six-character escape, all ordinary ASCII — so no request middleware can
find it without decoding the body, which is the handler's job.
MECHANISM, each premise measured against encoding/json rather than
reasoned about:
raw NUL inside a string -> decode ERR (invalid character in string literal)
raw NUL after the value -> decode ERR
the escape in a value -> decodes to a string CONTAINING a NUL
the escape in a KEY -> same
a DOUBLED backslash -> decodes to literal text, NO NUL
the uppercase spelling -> not a JSON escape at all
So the escape is the only vector and its substring is a sound FAST PATH
(absent -> no NUL possible), but not a sufficient test: a doubled
backslash carries the same six characters and decodes to text. In this
product that is not hypothetical — items and documents store markdown,
and a document about JSON escapes is an ordinary thing to write. The
exact step is json.Decoder.Token(), which returns DECODED strings, covers
object keys and arbitrary nesting (an item's fields blob), and needs no
knowledge of the destination type.
NOT REFLECTION over the decoded value, the other obvious design: it sees
[]byte fields AFTER base64 decoding, so a body carrying legitimate binary
({"b":"AQAC"} -> bytes 01 00 02) would be refused for a NUL that is not
text. A token walk sees the base64 characters. No request struct has such
a field today (searched: []byte with a json tag in internal/server and
internal/models, non-test — only models.YjsUpdate.UpdateData, which no
handler decodes from a body); the token walk is chosen so adding one
later cannot silently start rejecting valid requests.
BUFFERING IS NOT A COST. json.Decoder.Decode already holds the whole
top-level value in memory — refill accumulates into dec.buf and grows it
by doubling (encoding/json/stream.go) — so streaming never avoided the
copy. Measured on the 64 MiB workspace-import shape, total allocation:
stream+Decode 354.7 MiB, ReadAll+Unmarshal 256.5 MiB, ReadAll+Decode
512.5 MiB. Peak heap is order-dependent and does not discriminate; the
first run of that measurement showed a 0.77x peak win that vanished when
the legs were swapped, so only the allocation figure is claimed.
POPULATION, measured on Postgres 17 through the real router with a
control leg on every endpoint (92 mutating routes enumerated via
chi.Walk; 13 probed):
before: 12 of 13 DOOR (control 201 / NUL 500, SQLSTATE 22021)
after: 0 of 13 — every NUL leg 400, every control leg unchanged
Confirmed doors: workspace name, collection name, item title, item
content, item fields value, item title via PATCH, comment body, agent
role name, view name, document title, webhook secret, workspace import.
workspace-token name is UNMEASURED, not clean — its control leg 500s on
an unrelated FK in this fixture. The other 79 routes are unprobed, not
claimed clean; the completeness argument is structural instead, and
enforced by a test rather than asserted.
SECOND DEFECT, named rather than slipped in: the six handlers that
decoded straight off r.Body had no http.MaxBytesReader either — the cap
decodeJSON has always applied — so each was an unbounded body read.
Routing them through decodeJSON closes that too.
COMPATIBILITY: json.Unmarshal refuses trailing non-whitespace after the
JSON value where Decode ignored it. Deliberate, same direction as this
fix, and the only behaviour change beyond the refusal. Trailing
whitespace still passes. An EMPTY body still returns a wrapped io.EOF,
because handlers_playbooks.go reads errors.Is(err, io.EOF) as "no
arguments supplied" — caught by TestPlaybookRunAcceptsEmptyBody, which is
exactly the wiring a helper-level change is blind to.
No call site changed for the refusal itself: all 65 decodeJSON callers
already turn a decode error into a 400 carrying err.Error().
Release note: a NUL character in a JSON request body now returns 400
instead of 500 on Postgres deployments.
Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN
* fix(server): follow the NUL refusal into JSON-encoded string fields (BUG-2803)
Codex round 1 on #1220: the check scanned ONE JSON layer, and several
fields cross the wire as JSON-ENCODED STRINGS rather than nested objects
— an item's fields, a collection's schema, a workspace's settings. The
OUTER decode of {"fields":"{...}"} yields the inner document as literal
text, in which the escape is still six ordinary characters and no NUL
exists, so the single-layer token walk passed it.
MEASURED on Postgres 17 with a control leg on each, after the
single-layer check was already in place:
item.fields as a JSON-encoded string 500 control 201
collection.schema as a string 500 control 201
workspace.settings as a string 500 control 201
The error is DIFFERENT from the rest of this family, which is why it is
worth reading rather than assuming:
insert collection: ERROR: unsupported Unicode escape sequence (SQLSTATE 22P05)
22P05, not the 22021 the path and query halves produce. The outer string
is pure ASCII so it never trips the text-encoding check; this is
Postgres's own JSON parser refusing the escape inside a document bound
for jsonb, which cannot represent a NUL. After this change all three
answer 400 with their control legs unchanged.
THE FIX: when a decoded string is itself a complete JSON object or array
— the class this API re-parses downstream — walk it too, to a depth
bound of 8. Recursion terminates on its own (each level is a strict
substring of the one above); the bound keeps a hostile body from buying
many full re-parses, and AT the bound the body is refused rather than
passed uninspected, since the escape is known to be present and the walk
has stopped looking.
WHAT THIS OVER-REFUSES, by design and pinned by a test: the rule is
structural, not destination-typed, so a plain TEXT field whose ENTIRE
value is a valid JSON document carrying the escape is refused too, even
though its column would have stored it. Prose ABOUT a JSON escape does
not parse as a bare document, so the case is narrow, and a value of that
shape breaks any consumer that parses it. The destination-typed
alternative — an allow-list of the fields that arrive JSON-encoded — is
exactly correct and goes stale in silence, which is the failure mode
ValidateQuery's comment rejects when it explains why per-site query
validators could not be written.
Tests: nested documents (fields/schema/settings/array/twice-encoded),
with controls for ordinary content, a doubled backslash INSIDE the
nested document, a string that starts like JSON but does not parse, and
prose that merely mentions the escape; the over-refusal pinned as a
decision rather than left as an accident; the depth bound; and a wiring
leg through the real router on SQLite, where the write would otherwise
SUCCEED so a green cannot be the database doing the work. Fixtures build
their JSON-encoded strings with encoding/json rather than hand-written
backslashes, since the escaping rules are the subject under test.
All four new tests fail with the recursion removed.
Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN
* test(server): build the NUL-bearing timeline fixture through the store (BUG-2803)
TestTimeline_NeverEmitsACursorItWouldRefuse built its fixture through the
API on a premise its own comment stated: "a structured id comes from the
item's fields blob, which nothing validates on write". BUG-2803 made that
false — decodeJSON now refuses a body whose strings decode to a NUL,
including one nested inside a JSON-encoded `fields` string — so the API
can no longer produce the row and the test 400'd on its fixture.
Repaired rather than deleted, because the DEFENCE it covers is still
live: rows in this shape can predate the rule, and the store has no such
check of its own, so a migration, an import or any future non-HTTP writer
can still produce one. The timeline must keep refusing to hand out a
cursor it would then reject.
The fixture now writes the blob directly, injecting the six-character
JSON escape rather than a raw NUL — the blob is JSON text and both
backends reject a raw NUL in it; the NUL comes into existence when Go
DECODES the blob, which is exactly how the timeline ends up with one
inside an entry id. The test is not vacuous under the change: it asserts
the NUL-bearing id took the positional fallback, so an injection that
failed to produce a NUL fails the test rather than passing quietly.
This is the CONVE-23 case — a change that falsifies existing prose owes a
sweep for that prose. The stale sentence was found by the test failing,
not by the sweep, which is the weaker of the two ways to find it.
Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN
* docs(server): correct the timeline comment BUG-2803 falsified (CONVE-23)
The entryID fallback's comment said note and decision ids "come from the
item's fields blob and nothing validates them on write". BUG-2803 made
that half false: the HTTP API now refuses a request body whose strings
decode to a NUL, including one nested inside a JSON-encoded `fields`
string. The sentence was true when written and nothing in this branch's
diff pointed at it.
The fallback still has to exist, and the corrected comment says why:
the STORE has no such check, so rows predating the rule — and anything
writing a blob by another path, a migration, an import, a future
non-HTTP writer — can still carry one.
SWEPT AND DELIBERATELY LEFT: two nearby comments
(handlers_timeline_id_collision_test.go, handlers_timeline_structured_test.go)
also say "nothing validates them on write". Both are about id FORMAT and
DUPLICATION — an imported artifact carrying a UUID-shaped id, a
hand-written blob repeating one — and this change validates neither. In
context those sentences remain true, so they are left alone rather than
edited into noise.
Sweep command: grep -rniE "nothing validates|not validated on write|no
validation on write|unvalidated" --include=*.go internal/ cmd/ — six
further hits, all about other subjects (github_pr raw writes, terminal
schema keys, push payload format, decodeJSON's size bound).
Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN
* fix(server): scope the nested-NUL walk to JSON-encoded fields (BUG-2803)
Codex round 2 on #1220. The nesting check from the previous commit
recursed into ANY string that parsed as a JSON document, on the argument
that a structural test beats a destination-typed one. That argument was
wrong in a way I had written down as an accepted trade and should have
weighed as a defect: a plain-text `content` value holding a JSON snippet
that merely MENTIONS the escape was accepted before this branch, is
stored in a text column that has no problem with it, and was newly
refused — including on RE-IMPORT of an export carrying it.
Refusing input the server itself produced is a worse failure than the
door the unscoped recursion was closing. Measured before the fix: a
workspace whose item content held such a snippet exported 200 and
re-imported 400.
The walk now descends only under keys whose STRING value is a JSON
document something downstream re-parses: config, events, fields,
metadata, phase_data, plan_overrides, schema, settings, tags, traits.
WHY A LIST IS SAFE HERE, when ValidateQuery's comment rejects exactly
this shape for query parameters: there the set of names is unbounded by
design (parseItemListParams turns any unrecognised parameter into a field
filter), so no list could be complete. Here the set is a closed property
of the wire model — a field is JSON-encoded because a Go struct declares
it as a string holding JSON — and
TestJSONEncodedFieldKeysCoversTheModels derives it from internal/models
and fails when a new one appears. The list cannot go stale in silence.
Over-inclusion is the safe direction and the list takes it: a listed key
that is not really JSON-encoded costs one parse attempt and can only
refuse a complete JSON document carrying the escape, while a missing key
reopens a door. `traits` is listed for that reason — it carries JSON but
its declaration has no comment saying so, which is exactly how the
derivation test would have missed it, so the test asserts coverage in one
direction only and the list is allowed to be a superset.
The walk also changed shape: decoding into `any` and walking the value,
rather than a token stream, because key context is needed to know which
subtree is JSON-encoded. The []byte reasoning is unchanged and still
holds — decoding into `any` never produces a []byte, so a base64 field is
seen as its ASCII text rather than as decoded bytes that might contain a
legitimate 0x00.
Tests: text fields carrying a JSON document are ACCEPTED (five keys),
with a leg proving the same document under a JSON-encoded key is still
refused, so the pair differs only in the key; the derivation test; and
the depth-bound fixture now nests under a JSON-encoded key at every
level, since nesting under an ordinary key would never start the
recursion and would have passed for the wrong reason.
STILL OPEN, and the lead holds it: a LEGACY row whose stored fields blob
already carries the escape still exports 200 and re-imports 400. That is
data this fix cannot make importable without weakening the write-side
refusal, and the disposition (repair sweep, flagged import, or documented
acceptance) is a product ruling. Recorded on the item.
Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN
* fix(server): close the three body doors codex round 3 found (BUG-2803)
All three verified before fixing, none taken on the reviewer's word.
1. BUNDLE IMPORT BYPASSED THE REFUSAL (P1). handlers_import_bundle.go
parses pad-export.json itself rather than through decodeJSON, so the
SAME workspace import — reached with Content-Type application/gzip
instead of application/json — walked straight past the NUL check into
Postgres. The bundle's export blob is now checked with bodyDecodesNUL
before ImportWorkspace, answering the same 400. Test drives a real
tar.gz through the router with a clean-bundle control leg, because this
path answers 400 for a dozen unrelated reasons (bad gzip, out-of-order
tar, duplicate entries) and a bare 400 would prove nothing.
2. ONE CALLER SWALLOWED THE NEW ERROR (P2). handlers_admin.go's
test-email endpoint read `if err := decodeJSON(...); err != nil ||
input.To == ""` and fell back to the admin's own address, so a body
carrying a NUL answered 200. An ABSENT body legitimately means "send it
to me"; a body that is present and REFUSED is a different thing, and
collapsing the two turns a validation error into a success. The two
cases are now separated on errors.Is(err, io.EOF).
3. THE COMPLETENESS TEST COULD NOT SEE PAST TWO CALL SHAPES (P2). It
scanned for json.NewDecoder(r.Body) and io.ReadAll(r.Body), so it was
blind to io.ReadAll(io.LimitReader(r.Body, n)) — a shape ALREADY in the
package — and to any alias or helper. A completeness test that misses a
live example is worse than none, because it reads as coverage. It now
scans for the thing that cannot be spelled around, a reference to the
request body at all, and requires every FILE touching one to be
accounted for with a written reason. Both directions are asserted: an
unaccounted file fails because a door may have opened, and an accounted
file that no longer touches a body ALSO fails, so the list cannot rot
into stale excuses that quietly cover a future reader. Verified with a
positive control (an added body reference in an unlisted file fails) and
a negative one (a stale entry fails).
FOUND BY THAT WIDENED SWEEP, and fixed here rather than filed: the raw
artifact import (POST /workspaces/{ws}/import-artifact) takes TEXT, not
JSON, so it never went through decodeJSON and inherited neither the NUL
refusal nor the path/query rule — a body is neither. A raw NUL or
invalid UTF-8 reached the store and Postgres answered 22021, which the
handler turned into a 500 for what is a client error. It now applies
bindableText, the same predicate ValidatePath and ValidateQuery use, and
answers 400 invalid_body. Note the shape difference from the JSON half:
there the ESCAPE is the vector because a decoder rejects a raw NUL;
here the RAW BYTE is, because nothing is in the way.
Each fix has a mutation run against it: disabling the bundle guard fails
the bundle test, disabling the artifact guard fails the artifact test,
and both controls still pass.
Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN
* fix(server): the escape gate was unsound, and YAML has its own (BUG-2803)
Codex round 4, two P1s, both reproduced before fixing.
1. THE FAST PATH LET A REAL NUL THROUGH. bodyDecodesNUL gated on "does
the raw body contain the six-character escape". That is unsound: the
BACKSLASH itself can be written as an escape, so a body carrying
\u0000 contains no literal six-character sequence anywhere in its
raw bytes, while the OUTER decode manufactures one inside the string —
and if that string is re-parsed as a JSON document (jsonEncodedFieldKeys)
the second parse turns it into a real NUL.
Measured through the real router before the fix: the oblique spelling
answered 201 where the direct one answered 400.
The mistake was applying a fact about how a NUL is spelled INSIDE a
decoded string to the RAW BYTES, where the backslash can itself be an
escape. That is the same layer-confusion this whole bug is made of, for
the third round running.
The gate is now a BACKSLASH. Every JSON escape mechanism requires one, so
a body with no backslash has decoded strings byte-identical to its raw
bytes, and a raw NUL cannot survive the decoder — no backslash therefore
means no NUL, at any depth, however spelled. Bodies WITH one pay for an
exact answer, a larger set than before (any nested JSON carries a
backslash-quote), which is the cost of being correct. The same
correction applies to the per-string pre-filter one level down.
2. YAML HAS ITS OWN ESCAPE VOCABULARY. The raw bindableText check added
last commit passes a double-quoted scalar `title: "a\0b"` — no NUL in
the request bytes — and the YAML decode manufactures one. Measured
before the fix: that artifact imported 201 with a NUL in the item title.
The decoded artifact is now checked too: title, body, and every
frontmatter field value, walked because a playbook's `arguments` is a
nested structure rather than a scalar. Keys are checked as well as
values, on the same precautionary grounds ValidateQuery states for
query parameter names.
Same shape as the JSON half in both cases: a value that is harmless
until a SECOND parse, checked at the layer that can see it.
Tests: the oblique spelling joins the nested-document table, and the
YAML escape joins the artifact table. Each is mutation-verified —
reverting the gate to the substring fails the oblique case only, and
disabling the post-decode artifact check fails the YAML case only, with
the raw-byte cases still killed by the raw check. That per-leg
discrimination is the point: it shows each check earns its own keep
rather than being covered by its neighbour.
Prose corrected where this falsified it: jsonNULEscape's "it is the ONLY
spelling" is true of the escape and was being used to justify a filter on
the raw bytes, which is a different claim. Both now say so explicitly.
Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN
* fix(server): multipart text fields and the bundle manifest (BUG-2803)
Codex round 4's two P2s. Both are the same shape as the rest: a
caller-supplied string reaching a text comparison through a door the
earlier fixes did not cover.
1. MULTIPART TEXT FIELDS. The multipart body is deliberately exempt from
the JSON rule — its payload is binary blob content and must not be
scanned for text validity — but its TEXT fields are a different thing.
`item_id` goes to ResolveItem and into a database comparison exactly as
the query-string channel does, and that channel has been validated at
the transport since BUG-2784; the form channel was not. multipartValues
now drops values that are not bindable text, which makes an unusable
value indistinguishable from an absent one — the disposition
resolveUploadItemID already applies to empty values.
The uploaded FILENAME gets the same predicate, with a fallback to a
generic name rather than a refusal: the bytes are fine, only the label
is unusable.
A NEGATIVE RESULT worth recording, because it changed the test: a RAW
NUL in the multipart header is NOT the vector. Go's multipart reader
refuses it as a malformed MIME header line before any handler sees it
(measured: 400, "malformed MIME header line"). The reachable spelling is
the RFC 5987 encoded form, filename*=UTF-8''sh%00ot.png, which the
header parser accepts and percent-decodes afterwards. The first version
of this test used the raw form and was testing a vector that does not
exist.
2. THE BUNDLE ATTACHMENT MANIFEST. A second JSON document inside the
tar.gz, parsed directly like pad-export.json was, so it needed the same
check. Without it a NUL in a manifest string reached
rehydrateAttachment, whose failure is logged and SKIPPED — so the import
reported success while silently dropping the attachment. The
skip-on-failure behaviour is pre-existing and deliberate (a partial
restore beats none); refusing the bad INPUT is what stops it being
reached this way. Left as it is, and named rather than quietly changed.
A VACUOUS ASSERTION THE MUTATION CAUGHT, recorded because the test would
otherwise have shipped as coverage: the filename leg first asserted
`!strings.ContainsRune(body, 0)` on the RESPONSE, which is JSON — a NUL
in the filename comes back as the six-character escape, not as a 0x00,
so the check passed whether or not the fix was present. It did pass with
the fallback disabled. Now it decodes the response and asserts the
replacement name. The item_id leg had the mirror-image weakness: it
asserted "not a 500", which is the Postgres-only symptom, so on SQLite it
would have passed either way; it now asserts the request behaves exactly
like the no-value control.
Every fix in this commit has a mutation against it, and each kills only
its own leg.
Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN
* refactor(server): drop the now-unused escape constant (BUG-2803)
The gate became a backslash check, which was the last production use of
jsonNULEscape; golangci-lint's unused check failed on the next run. Its
documentation was load-bearing, so the explanation moved into
bodyDecodesNUL's comment rather than being deleted with the variable —
including the distinction that made the old gate wrong (the escape has
one spelling INSIDE a decoded string, which is not a claim about the raw
bytes).
Caught by re-running lint on the tip after the previous commit rather
than trusting the run from the tip before it.
Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN
* fix(server): rune-safe truncation and User-Agent sanitising (BUG-2803)
Codex round 5 was asked for the POPULATION rather than a confirmation —
"enumerate every remaining way a caller-supplied string can reach a
database text or jsonb parameter without passing a validity check" — and
returned three residual classes with their sinks. Two are fixed here;
the third is filed, because measuring it needs a fixture this unit
should not grow.
1. TRUNCATION CAN UNDO THE VALIDATION. Four sites cut a caller string
with a plain byte slice (name[:120], input.Name[:200]). If the boundary
lands inside a multi-byte rune the result ends in a partial sequence and
is no longer valid UTF-8 — so a value that PASSED the body check a few
frames earlier arrives at the store unbindable, and Postgres answers
22021 for a request the server already accepted.
This is the interesting one, because no input-side round could have
found it: the defect is downstream of validation, and it is invisible
with ASCII fixtures, which is what every test in that area used.
truncateBindableText walks back off continuation bytes and drops the
straddling rune. Tested with 2-, 3- and 4-byte runes so an off-by-one
walk-back cannot pass them all, and with a counterfactual leg asserting
the naive slice really does produce unbindable output for the same
input — without it the cases would pass against an implementation that
did nothing.
2. USER-AGENT REACHES TEXT COLUMNS. It lands in activities.user_agent
(three document paths, the connected-apps revoke) and
sessions.user_agent (three login paths), and no rule here sees a header.
The disposition is SANITISE, not refuse, and that is deliberate: a
header is metadata this server chose to record, not something the caller
asked for, so a malformed one must not turn an otherwise fine request
into a 400. The two sites that HASH the header are left alone — sha256
over arbitrary bytes is well defined, and changing what is hashed would
invalidate every stored UAHash.
The filing's own earlier probe had recorded User-Agent as NOT
reproducing on the item-create path. That was true and did not
generalise; these are different sinks.
3. NOT FIXED, FILED: the OAuth form-encoded bodies
(/oauth/token, /oauth/authorize/decide, /oauth/revoke,
/oauth/introspect) parse url-encoded form data outside the shared body
validator, with connection_name reaching oauth_connections.name and
client_id reaching the oauth_clients.id lookup. This was the ORIGINAL
subject of BUG-2803 before the filing was re-scoped, and it was recorded
then as unreachable without a fosite-backed fixture. That is still true,
and round 5's sink list is far more than the filing had. Filed rather
than guessed at.
Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN
* fix(server): narrow the gate, stop refusing natural-shape fields (BUG-2803)
Codex round 6 plus one measurement of my own. Three changes, one of them
a revert of something I got wrong in the previous commit.
1. THE GATE COST TOO MUCH, so it is narrower and still sound. The
previous commit gated the walk on "does the raw body contain a
backslash", which is correct but catches every body carrying nested JSON
(each `\"` is a backslash). Measured on a ~377 KB import-shaped body:
60106 allocs/op with that gate versus 30073 with the walk disabled — the
walk was running on ordinary traffic.
The gate is now the four bytes that begin any \u escape for a character
below U+0100. The argument: to manufacture the six-character NUL escape
inside a decoded string, each of its characters arrives either literally
from the raw bytes — in which case the raw contains the escape, which
begins with that prefix — or from a \u escape of its own, and the three
characters involved (backslash U+005C, 'u' U+0075, '0' U+0030) all sit
below U+0100, so those escapes begin with it too. Back to 30073
allocs/op, identical to the walk-disabled build.
That argument is the same KIND of reasoning that was wrong two rounds
ago, so it does not stand on its own: a differential test runs the gated
function against an UNGATED walk over a corpus built to attack it —
oblique backslash, upper-case hex, an escaped 'u', an escaped '0', a
doubled backslash — and fails on any disagreement. It also asserts the
corpus contains both answers, since agreement over a one-sided corpus
would be vacuous. Reverting the gate to the old substring fails it.
2. THE CHECK REFUSED THE NATURAL SHAPE OF ITS OWN FIELDS. `tags` and
`fields` accept both a JSON-encoded STRING and their natural array/object
form, and the walk propagated "this subtree is JSON-encoded" into
containers — so a free-form tag whose whole value happened to be a JSON
document was refused, though nothing re-parses it. Measured: refused
before, accepted now, while the JSON-encoded spelling of the same field
is still refused. The flag now marks only a direct STRING child of a
listed key.
3. REVERTED: I wired the three LOGIN paths to the User-Agent sanitiser
last commit, before reading store.CreateSession. It HASHES the header
and stores no text — the round-5 enumeration named "sessions.user_agent"
and I took the name for a column. The change would have been actively
harmful: login would store sha256(sanitised) while middleware_auth still
compares sha256(RAW), so every session from a client with a non-UTF-8
User-Agent would fail validation. A sink named in a review is a pointer
to verify, not a finding. The real sink is activities.user_agent, from
three document paths and the connected-apps revoke.
4. And the wiring leg codex asked for, on that real sink: a request
through the router with a malformed header, reading the STORED value out
of the activities row, with a control asserting an ordinary header is
kept VERBATIM. Unwiring the production call site fails it; the helper's
unit test does not notice.
Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN
* fix(server): apply the key rule at every level, not once (BUG-2803)
Codex round 7, both findings, the first confirmed by measurement.
1. THE RECURSION WENT ONE LEVEL TOO DEEP. Once the walk descended into
a JSON-encoded string it treated the WHOLE subtree below as
JSON-encoded, so a value nested two levels down — an ordinary string
inside a `fields` blob that happens to hold JSON text — was refused.
That is a false rejection, and the measurement says so plainly. With the
depth-2 check disabled, on Postgres 17:
depth 1 (the fields blob itself) -> 400 (correct: Postgres parses it)
depth 2 (a string INSIDE the blob) -> 201 (accepted, no error)
control -> 201
The handler parses `fields` ONCE. The inner text is re-escaped when the
blob is written, so what Postgres receives has a doubled backslash and no
escape at all. Only the document Postgres itself parses can carry a fatal
one.
The nested call now passes false rather than true, which makes this a KEY
RULE APPLIED AT EVERY LEVEL rather than a depth limit: a JSON-encoded key
INSIDE a document still recurses (pinned by a test), an ordinary one does
not. Same correction as round 6's natural-shape fix, one level further in
— I fixed the sibling case and left this one, which is CONVE-18's lesson
about my own enumeration being a sample too.
I checked whether anything re-parses a value inside the blob before
loosening this, rather than assuming: `arguments` was the candidate, and
parsePlaybookArguments asserts it is a native ARRAY (raw.([]any)) rather
than a JSON string, so it is covered by the natural-shape rule and needs
no second parse.
2. AN ERROR MESSAGE THAT SENT CLIENTS THE WRONG WAY. The OAuth dynamic
client registration handler prefixed every decode failure with "Request
body must be JSON". A body carrying a NUL is valid JSON, so that message
sends a client hunting a syntax error it does not have. The two failures
are now distinguished.
Round 7 also reports no break in normal CLI, MCP or web-client request
generation — they marshal JSON and encode paths and query parameters —
which is the first thing any round has said about the client surface.
Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN
* fix(server): complete the artifact check, make the walk path-aware (BUG-2803)
Codex round 8. It confirmed round 7's two fixes, then found two real
defects and two inaccurate comments — the comment half being the angle
the round was asked for.
1. THE ARTIFACT CHECK MISSED TWO REACHABLE FIELDS. artifactIsBindableText
walked the decoded artifact by TYPE, so it never covered Provenance —
whose strings are rendered into a Markdown footer appended to the stored
content — and never matched Arguments, declared []map[string]any, a
concrete slice type the walk's []any case does not match. A YAML NUL
escape in either reached storage.
It now MARSHALS the artifact and searches the output for the escape
encoding/json produces. A type switch over a struct that grows is a list
that goes stale in silence; marshalling covers every exported field,
including ones added later. The one thing it cannot see is invalid UTF-8
(which marshals to U+FFFD), and it does not need to: step 2 rejects that
in the request bytes, and YAML cannot manufacture it from valid input —
its escapes name code points, where \0 names a NUL.
Both new cases fail with the check disabled; the raw-byte cases still
pass, killed by the raw check, so each leg is discriminating.
2. THE WALK WAS NOT PATH-AWARE. A collection may declare a user field
literally named `schema` or `tags`. The walk consulted the wire-key list
at every level, so `{"fields":{"schema":"..."}}` treated a user field
name as a wire key and refused valid text holding a JSON example.
The key list is now consulted only OUTSIDE caller data — not under a
natural `fields` object, not inside an element of a `tags` array, not
inside a re-parsed document. Combined with round 7's fix that makes the
descent exactly one level deep BY CONSTRUCTION, which is why the depth
counter is gone: with the flag no longer inherited, a bound could never
fire, and dead protection reads as protection. The depth-bound test is
replaced by one that pins the property directly — an escape IN the
parsed document is refused, one BELOW it is accepted, and a
wire-key-shaped user field does not restart the descent.
3. THREE COMMENTS CORRECTED, all mine, all of the kind a reader would
believe without checking:
- MaxBytesReader: Close FORWARDS to the underlying body rather than
being a no-op, and with a nil writer there is no automatic 413 — the
cap surfaces as a read error the callers turn into 400. Behaviour
unchanged; only the claim was wrong.
- parseArtifactRequest said "three checks" while implementing five, and
its returns list omitted ErrArtifactUnbindableText. Both added by this
branch, which is exactly the prose a change is most likely to falsify
(CONVE-23).
- errJSONBodyNUL claimed all 65 callers surface its message. The STATUS
is uniform; the wording is not — several substitute a generic string.
4. And one in a test: the timeline fixture said both backends hold a
CHECK constraint a raw NUL violates. items.fields is a plain TEXT column
with no CHECK on SQLite. What was OBSERVED is "SQL logic error:
malformed JSON"; the likely source is an expression index over
json_extract, and that attribution is recorded as NOT verified rather
than asserted.
Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN
* fix(server): a regression this branch introduced, and the same trap again (BUG-2803)
Codex round 9. Both findings are mine, one of them a regression from the
round-8 restructure two commits ago.
1. THE ROUND-8 RESTRUCTURE REOPENED THE ORIGINAL DOOR. Taking the
JSON-encoded branch for a listed key skipped the plain "does this string
contain a NUL" check and asked only "does the document this string
carries hold an escape". Those are different questions. So
{"fields":"a<NUL escape>b"} — a direct NUL in the fields value, the very
first case this whole change closed — was accepted again.
Both checks now run. The test pins all three legs: a direct NUL in the
fields string, an escape inside the fields document, and an ordinary
fields string that must still be accepted, so the first two cannot pass
merely because everything under a listed key is refused.
2. THE ARTIFACT CHECK FELL INTO THE TRAP IT WAS WRITTEN AGAINST. It
searched the MARSHALLED bytes for the escape sequence, and a value
holding the six LITERAL characters marshals to a doubled backslash which
still contains that sequence as a substring — so valid content was
refused. Artifacts are documentation; text about a JSON escape is
exactly what one carries.
Worse than the bug: the comment I wrote asserted the ambiguity "cannot
arise here". It was the same doubled-backslash case bodyDecodesNUL exists
to resolve, one function away, and I wrote a sentence explaining why it
did not apply instead of checking. The marshalled form is now decoded
again and walked with the same machinery — the round trip is what makes
every field reachable without a type switch, the walk is what makes the
answer exact.
Its test asserts literal escape TEXT is accepted in title, body and a
field value, with a counterfactual leg asserting a real NUL in each of
those places is still refused, so acceptance cannot come from the check
doing nothing.
Reverting either fix fails its test and only its test.
Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN
* docs(backup): the one case where an export is not importable (BUG-2803)
Codex round 12, an operational pass. It found no migration or config
requirement, and two documentation gaps.
docs/backup.md promises that application-level export/import is portable
across SQLite and PostgreSQL. Since BUG-2803 that has one exception: a
workspace whose stored data contains a NUL exports fine and is refused on
import. It can only affect data written before the rule existed and only
on SQLite, which accepted it — a PostgreSQL instance never stored one.
`pad db migrate-to-pg` has the SAME problem and reports it worse: it
copies rows directly and never passes through the import guard, so a
legacy row fails against PostgreSQL's JSONB parser partway through the
copy rather than being refused up front. That is the likelier way an
operator meets this, since it is the operation that puts an entire old
SQLite database in front of PostgreSQL for the first time. Recorded on
BUG-2810, which owns the preflight and repair.
Round 12's other finding — that the PR's stated release note covered the
JSON 500-to-400 change and none of the rest — is fixed in the PR body
rather than in the tree.
Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN
* test(server): close two blind spots the tests themselves had (BUG-2803)
Codex round 13, asked whether the new TESTS are sound. Five findings;
these are the two that were self-contained. The other three are recorded
on the item with what each needs.
1. THE COMPLETENESS SCAN WAS BLIND TO FORM BODIES. It matched only
`.Body`, so FormValue / ParseForm / MultipartForm — which read the
request body just as surely — were invisible. It therefore reported full
coverage while the OAuth form-encoded handlers were entirely outside its
view. Widened, and it immediately failed on handlers_oauth.go, which is
the instrument working.
That file is now ACCOUNTED FOR AS A KNOWN GAP rather than as safe: the
OAuth handlers read form-encoded bodies that no rule in this family
covers (the transport rules see the query half of r.Form, not the body
half), tracked as BUG-2811 and needing a fosite-backed fixture to
measure. The test now STATES the gap instead of being blind to it, which
is the difference between a completeness claim and a completeness
appearance.
2. THE TRUNCATION TEST ADMITTED AN IMPLEMENTATION THAT RETURNED "". Its
assertions were: within the limit, bindable text, a prefix of the input.
An empty string satisfies all three. It now also asserts that an input
fitting the limit comes back UNCHANGED, and that no more than one rune
(4 bytes) is lost to the boundary — so a truncator that drops too much
fails, not just one that keeps too much.
Both were found by asking whether a broken implementation would pass,
which is the question CONVE-12 is about and which I had applied to the
production code and not to these two tests.
Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN
* test(server): the three remaining round-13 gaps (BUG-2803)
Codex round 13's other three findings, all of the same shape: a test
that would stay green with the production change reverted.
1. THE MANIFEST CHECK WAS UNTESTED. The bundle test built archives
containing only pad-export.json, so disabling the INDEPENDENT attachment-
manifest check left the suite green. The new test builds a bundle with
both entries, differing only in the manifest, so a refusal cannot come
from the export half. Verified by disabling each check separately: only
the matching test fails, so the two are independently covered.
2. THE TEST-EMAIL CHANGE HAD NO HANDLER-LEVEL TEST. Every existing leg
exercised decodeJSON, so reverting handlers_admin.go to default EVERY
decode failure to the admin's own address passed them all. The new test
drives the real endpoint with a wired mock sender and pins the
distinction that used to collapse: an ABSENT body still means "send it
to me" (control), an ordinary body still sends (control), and a body that
is present and refused answers 400 rather than being reinterpreted as
the default recipient.
3. THE MULTIPART LEG CHECKED ONE BYTE CLASS. A filter rejecting NULs
while letting malformed UTF-8 through would have passed it. It now drives
both, which matters because invalid UTF-8 is the class that reaches
Postgres as 22021 on a UTF8 database.
Round 13 was asked whether the new TESTS are sound — deterministic,
order-independent, and failing on broken code. It reported the fixtures
isolated and found five ways they were not discriminating. Two were
fixed in the previous commit; these are the rest.
Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN
* test(server): pin the wiring at every call site, not one (BUG-2803)
Codex round 14 confirmed round 13's five, then found the same shape one
level out: reverting a SINGLE call site back to the unsafe form left the
whole suite green, because the surviving fixtures are ASCII and a
helper's unit test does not care who calls it.
TestTextSafeHelpersAreUsedAtEveryCallSite asserts the wiring STATICALLY
rather than adding a fixture per site (an OAuth connection, a cloud
login, four audit paths). A byte-slice truncation of a caller string
fails it, and so does a raw User-Agent read outside the exempt set. Both
directions are checked: finding none of the SAFE form also fails, so a
scan that silently matched nothing cannot pass forever.
The User-Agent exemptions carry counts rather than being blanket, so a
NEW raw read in an exempt file still fails. All four reads in
handlers_auth.go are exempt because they feed a HASH — CreateSession
hashes the header and stores no text — and sanitising before hashing
would be actively harmful: login would store sha256(sanitised) while the
session check still hashes the RAW header, failing validation for every
client with a non-UTF-8 User-Agent. middleware_request_text.go's one raw
read is requestUserAgent itself.
Verified by reverting one truncation call site and one User-Agent call
site independently; each fails the test.
Round 14's third finding is fixed behaviourally rather than statically,
because the static scan cannot see it — handlers_oauth.go is already
listed for its form-body reads. TestOAuthRegisterRefusesNULBody drives
the real dynamic-registration endpoint with cloud mode and an OAuth
server wired, with a control leg registering successfully, and pins both
the refusal and the message split: the body IS valid JSON, so the answer
must not send a client hunting a syntax error.
Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN
* fix(server): match wire keys the way the decoder does (BUG-2803)
Codex round 16, asked whether this change is consistent with its siblings
in the same file and extensible by someone who did not write it. It found
a live bypass instead.
encoding/json matches an incoming key to a struct field by an exact match
first and a CASE-INSENSITIVE one otherwise, so {"Fields":...} and
{"FIELDS":...} land in ItemCreate.Fields exactly as {"fields":...} does.
The walk looked the key up case-SENSITIVELY, so it skipped the nested
document for a body the handler went on to accept, and the database
answered the original 500.
Measured before the fix: `fields` refused, `Fields` and `FIELDS`
accepted.
This is the same defect shape as everything else in this unit — a check
that agrees with one layer's rules while the layer that actually consumes
the value uses different ones — which is why the fix is a PREDICATE
rather than a wider map: the map is the vocabulary, and the matching RULE
belongs to the consumer. Someone adding a key should not also have to
remember to add its spellings.
The test drives six spellings including mixed case, with a control
asserting an unlisted key stays caller data in any casing, so this is
case-insensitive matching rather than matching everything.
Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN
* fix(server): fold keys the way encoding/json folds them (BUG-2803)
Codex round 17, first of five findings. The previous commit fixed the
ASCII half of key matching and left the Unicode half, which is this
bug's own pattern one more time.
encoding/json matches with Unicode SIMPLE FOLDING, not lower-casing.
U+017F LATIN SMALL LETTER LONG S folds to 's', so "ſchema" reaches the
`schema` struct field while strings.ToLower("ſchema") is unchanged and
missed the allowlist — a nested NUL under that spelling reached the
handler undetected.
Matching is now strings.EqualFold against each canonical key. The test
carries both a lower-case fold spelling and an upper-case one alongside
the ASCII cases, and keeps its control asserting an unlisted key stays
caller data in any casing.
The other four round-17 findings are recorded on the item rather than
patched here: they are genuine layer disagreements (duplicate keys
merging differently in a typed decode than in a map, a scan-failure
disposition on inputs the typed decode tolerates, and unknown-field
policy) whose fixes are design decisions rather than corrections, and
this seat is near its context bar. Each is written up with the
measurement it needs.
Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN
* fix(server): pin that a NUL-bearing manifest refusal keeps the partial workspace (BUG-2803)
Codex round 18. The comment on the manifest NUL branch said refusing the
input "stops it from being reached this way" and stopped there, which
reads as though the refusal undoes the import. It does not.
A plain error with a non-nil workspace keeps the partial workspace,
exactly as every other manifest failure in this loop does — the rollback
branch fires only for *importStatusError, and mid-stream manifest
failures intentionally keep what was imported (TASK-896). Returning a
rollback-shaped error here would give NUL-bearing manifests different
semantics from malformed ones, which is a change to the bundle-import
contract rather than a fix to this bug.
So the behaviour is unchanged and now DELIBERATE: the comment states it,
and the test asserts the persisted state rather than only the HTTP
answer. Mutation: routing the branch through *importStatusError makes
the refusal roll back, and the new assertion fails naming the release
note it would falsify. The pre-existing status/body assertions do not
notice.
Claude-Session: https://claude.ai/code/session_01AUvLoXsKdS5sdpYju6rj4p
* docs(server): record the four map-model disagreements as dispositions, and pin them (BUG-2803)
Lead ruling day-68 is land-and-follow: this branch lands on its measured
commits, and the token-stream rewrite is the BUG-2812 unit's spec rather
than a late restructure of an 18-commit branch under review pressure.
That makes the four open findings from rounds 16-17 something to WRITE
DOWN precisely, not something to leave in a trail comment.
The doc comment on bodyDecodesNUL now carries all four, with the one
root cause named: this scan decodes into map[string]any and the typed
decode does not agree with that model about keys. Two under-refuse
(duplicate-key merge; scan-failure passthrough) and are BUG-2812's spec
- both dissolve under a walk that never builds values. Two over-refuse
(unknown fields; case-variant duplicates) and are ACCEPTED, because
refusing is the safe direction. The asymmetry is stated rather than
smoothed over: within the map model, (1) and (4) are one defect seen
from two sides and only one of them fails safe.
Finding (3) is an observable compatibility change - a forward-compatible
field carrying a NUL escape now gets a 400 where it got a 200 - so it
goes in the release note as well as here. A qualification only protects
where the actor meets it.
All four are pinned by a test, measured on this tip rather than carried
over from the round-16/17 write-up. The two known-gap legs assert the
WRONG answer on purpose: when BUG-2812 lands they FAIL, naming the doc
comment and the release note as what to update. Both gap legs carry a
premise assertion - the same bodies with the disagreement mechanism
removed ARE detected - without which they would pass against a check
that detected nothing.
Claude-Session: https://claude.ai/code/session_01AUvLoXsKdS5sdpYju6rj4p
* test(server): wire release-note item 10 to the router, with its before-state measured (BUG-2803)
The disposition test proves bodyDecodesNUL RETURNS true for an unknown
field carrying a NUL escape. The release note claims the API answers
400. Those are different claims and only the second one is what an
operator or client author reads - CONVE-19, my own convention: a
direct-call test vouches for the component, not its binding.
Two legs, and the control is the load-bearing one. An unknown field with
an ordinary value must still be ACCEPTED, so this pins "refused for the
NUL" rather than "refused for being unknown". The handler does not
reject unknown fields; if it ever started to, the note's explanation
would be wrong while its status code stayed right, and no
status-code-only assertion could see that.
The before-state is measured rather than asserted from memory. Disabling
the check makes the same request answer 201 - which is main's behaviour,
since decodeJSONWithLimit there unmarshals straight into the typed value
and the key is dropped. So "answers 400 where it answered 200" is a
measurement in both directions, not a recollection of one.
Claude-Session: https://claude.ai/code/session_01AUvLoXsKdS5sdpYju6rj4p
* docs(backup): the NUL rule lives in the binary, not the database (BUG-2803, BUG-2813)
Codex round 19, the fresh-angle deploy/rollback/mixed-version pass.
docs/backup.md said a NUL-bearing row "can only affect data written
before that rule existed, and only on SQLite". The second half is true.
The first half is false, and the reason is the interesting part: the
guard is in decodeJSONWithLimit, so the invariant is a property of the
running BINARY, not of the database.
On SQLite any window where an older binary serves the same database can
still write one - a rollback after upgrading, a staged rollout with an
old and a new instance sharing a database, a second older instance on
the same file. The window closes, the guard returns, and the rows are
already stored, behaving exactly like genuinely old ones. A rollback is
an ordinary operational move, so this is not an exotic path.
The doc now states the binary-version dependence, says which dialect is
affected and why PostgreSQL is not (it refuses a NUL itself, at every
version), and gives the operational answer: drain writes from older
binaries before the new one serves, or roll forward rather than back.
Store-layer enforcement - so the running build stops mattering - is
filed as BUG-2813 rather than added here. It is a dialect-level change
and the day-68 ruling on this unit is land-and-follow.
The same false implication was carried by the PR's release note calling
such a workspace "legacy"; corrected there too.
Claude-Session: https://claude.ai/code/session_01AUvLoXsKdS5sdpYju6rj4p
* docs(server): cite the ruling in house style, not the team-room day counter (BUG-2803)
"lead ruling day-68" is the internal day counter, which means nothing to
anyone reading this repo and is inconsistent with every other citation
in it - the codebase cites a lead ruling by DATE or by BUG ref, never by
day-N. Replaced with the bug ref, which is the part a reader can
actually follow.
Claude-Session: https://claude.ai/code/session_01AUvLoXsKdS5sdpYju6rj4p
* docs(server): drop a commit count I had already measured as wrong, and stop asserting a cause I borrowed (BUG-2803)
Two defects in a comment I wrote an hour ago, both of the kind this
unit's trail keeps recording.
"an 18-commit branch" - the branch was 20 commits at
|
||
|
xarmian
|
50499bceb8 |
fix(server): charge workspace imports against the plan limit (BUG-2793) (#1219)
* fix(server): charge workspace imports against the plan limit (BUG-2793) `POST /workspaces` enforces the user-scoped `workspaces` plan limit before creating. `POST /workspaces/import` did not, and it mints a workspace through the same store.CreateWorkspace — so a user at their plan's limit could exceed it by exporting any workspace and importing it back. Cloud only; enforceUserPlanLimit is a no-op when cloudMode is off, so self-hosted was never affected. Dave ruled the shape on day 63: an import IS a new workspace and counts, with no exemption for re-importing something you previously owned. Export provenance is not trustworthy enough to gate billing on, and the at-limit case that deserves relief — undoing a delete — is served by the restore endpoint, which mints nothing. The call is one line; the PLACEMENT is the fix. It sits beside the #1212 consent gate, ABOVE the Content-Type dispatch, for two reasons: - handleImportWorkspaceBundle is reachable only through that dispatch, so a gate below it would cover the JSON path and leave the tar.gz path — the one that carries attachments, and the one a real export produces — wide open. - Above either body read, so a refused caller never uploads. The two paths have very different size bounds; the gate precedes both. That is not a hypothetical: the mutation matrix includes it. Moving the gate below the dispatch fails ONLY the bundle test and leaves the JSON test green, which is exactly the false confidence a placement-blind fix would have shipped. Five tests, four of them controls, because a gate is easy to get green and hard to get right: the JSON path refuses at the limit, the bundle path refuses at the limit, under-the-limit is NOT refused (a gate wired to the wrong feature key would pass the first two), self-hosted is unaffected (this must not introduce a limit where there are no plans), and a request with no resolved user is not charged — mirroring the create side's `userID != ""` guard, which is not defensive padding but the difference between "no limit applies" and a nil lookup. This is the SECOND gate on workspace creation the import door skipped; the first was the OAuth consent gate (IDEA-2756, PR #1212). Two have now diverged this way, which is the argument for the shared pre-step helper — tracked separately rather than folded in here. Gates: `go test ./...` under Postgres 17 EXIT=0; gofmt clean; `make lint` 0 issues. BUG-2793 Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN * test(server): make the import controls assert success, not merely non-refusal (BUG-2793) Codex round 1. The three control tests were vacuous and I would have shipped them. WorkspaceExport.Version defaults to 0 and the import requires 1, so the under-the-limit, self-hosted, and no-resolved-user cases were all failing with a 500 long before they reached anything this change is about. They passed because they asserted only "not 403" — and a 500 is not a 403. That made all three useless in the same direction: a fix that broke imports outright, or a gate wired to refuse everything with a non-403 status, would have sailed through them while the two refusal tests stayed green. The controls existed precisely to catch that, and could not. Fixed by setting Version: 1 and asserting the real success status, 201. A control that cannot tell success from a server error controls nothing. Mutation matrix re-run after the change, because a matrix over vacuous tests proves nothing either: removing the gate still fails exactly the two refusal tests, and the three controls now pass on genuine imports rather than on identical 500s. Gates: `go test ./...` under Postgres 17 EXIT=0; gofmt clean; `make lint` 0 issues. BUG-2793 Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN * docs(server): scope what the no-resolved-user import test actually pins (BUG-2793) Codex round 2 pointed out that this test locks in a 201 for a caller with no resolved user, and that a reader will take that as approval of userless workspace creation. It is not. The test pins the GUARD — that import behaves as create does when no user resolves — and it drives the handler directly, so it does not prove a real legacy workspace token reaches this code at all. Said so in the test rather than leaving the 201 to speak for itself, and pointed at BUG-2809 for the question it does not answer. Round 2's three findings are all real and all filed rather than folded, because this unit's ruling is specifically the plan limit on the import door: - BUG-2808 — enforceUserPlanLimit is check-then-act, so concurrent requests can exceed any cap. A property of the helper, shared with the create door and every other feature it gates; this change inherits it rather than introducing it. - BUG-2809 — import and create still enforce different preconditions on the same mint: required-name validation (a live defect — an empty name yields an empty SLUG, which is a routing key), settings normalization, source attribution, and the userless case above. Filed as a class because the mechanism is one thing and the record now shows it failing twice. The reviewer also confirmed two non-doors, which is the useful negative: autoCreateWorkspace is intentional first-workspace provisioning, and `pad db migrate-to-pg` calls ImportWorkspace directly as an operator-only migration outside HTTP entirely. BUG-2793 Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN * test(server): pin the JSON-path placement and assert "not charged" as data (BUG-2793) Codex round 3, on the tests. Two ways they could pass without proving what their names say. 1. The JSON refusal test sends VALID json, so a gate placed after the decode would still return 403 and it would stay green. The bundle test covers the gzip half of the placement claim; nothing covered the JSON half, and the code comment claims the gate sits above EITHER body read. Added an at-limit case with an undecodable body: reaching 403 rather than a decode error is only possible if nothing read the body first. 2. The no-resolved-user test asserted only a 201. A regression that quietly attributed the import to the at-limit fixture user would also return 201 and pass. It now asserts the fact instead of inferring it — the user's workspace count is unchanged across the request, and the created workspace has no owner. The mutation matrix now separates the two placements, which is the point of having both tests: - gate below the Content-Type dispatch (still above the decode) -> only the BUNDLE test fails. - gate below the JSON decode -> the bundle test AND the new JSON test fail. Neither mutation is caught by the original refusal test, which is what "passes for the wrong reason" looked like here. Gates: `go test ./...` under Postgres 17 EXIT=0; gofmt clean; `make lint` 0 issues. BUG-2793 Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN |
||
|
xarmian
|
427540706c |
fix(documents): bound the rename cascade at the title and at the projected total (BUG-2798, BUG-2796) (#1218)
* fix(documents): bound the rename cascade at the title and at the projected total (BUG-2798, BUG-2796) A document rename rewrites [[oldTitle]] into every linking document. Neither factor of the output size was bounded: titles had no length validation, and the cascade holds every rewritten body in memory before writing any of them. One rename could project 10 GB from a 500 KB input -- 20,000x, measured -- and OOM while holding the workspace rename lock. Two walls, per Dave's day-63 ruling. 1. Title length, bounded at write time (models.MaxDocumentTitleRunes = 255). Runes, not bytes: "255 characters" is what a user and a UI counter mean. Existing over-limit titles stay valid until their next rename -- no retro-breakage of stored data. 2. The cascade's projected TOTAL, bounded at 16 MiB (store.MaxRenameCascadeProjectedBytes), accumulated across the linking set and refused before the first rewrite is built. The total is the right quantity and a per-document cap would not have been. Measured, with the title bound already in place: one linker holding the largest body a 2 MiB request can carry projects 108,632,370 bytes -- 51.8x -- and the aggregate is linear in the number of linkers (108.6 / 217.3 / 434.5 MB at k = 1/2/4, allocation tracking output at ~1.02x). A per-document cap of C still admits k * C, which is the same unbounded shape one level up. The 16 MiB figure has a receipt in the constant's doc comment: it sits above the absolute ceiling of any cascade this development instance could produce (its entire wiki-linking corpus is 10,077,476 bytes) and 6.5x below the single-document attack. The refusal is permanent-shaped and deliberately NOT in ErrLinkCascadeContention's family: 413 with the projection in the message and no Retry-After. Contention means "someone got there first, try again"; this means "this rename cannot be performed as asked". Answering it from the retryable family would tell a client to retry forever. BUG-2796 folds in at the same validation point, as ruled -- a title containing wiki-link syntax is emitted raw by links.ReplaceTitle, so renaming to `A]] [[A` produced two broken links and reported success. The rule is derived from the two mechanisms that consume a stored bracket (the grammar at markdown.ts:327 and the unescaper at markdown.ts:753) rather than from a character blacklist: the first version of this fix banned `]`, `\` and `|` because all three "look like wiki-link syntax", and the round-trip test refuted two thirds of that. `|` in particular is a title shape resolveWikiBody contains a dedicated branch to support, and `[` passes the grammar untouched. Doors enumerated rather than assumed (CONVE-24): store.CreateDocument and UpdateDocument have exactly two callers between them, both HTTP handlers. No CLI, import, or seed path writes a document title. Update previously validated doc_type and status and NOT title -- the one field that drives the cascade -- so the handler tests drive real requests through both doors (CONVE-19). BUG-2798, BUG-2796 Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN * fix(documents): count retained bytes, bound the retry path, escape the cascade's LIKE pattern (BUG-2798) Codex round 1 on #1218. Three findings, all real, all fixed here. 1. The guard bounded projected OUTPUT, which bounds nothing when the new title is SHORTER than the old one. Renaming a 255-character title to a one-character title makes each 2 MiB linker project ~40 KiB while the cascade still retains its 2 MiB read for the compare-and-set, so hundreds of linkers exhaust memory while the counter reports well under the cap. The counter now sums RETAINED bytes — read plus written, both alive at once — so the cap is a statement about resident memory rather than about output. MaxRenameCascadeProjectedBytes becomes MaxRenameCascadeRetainedBytes and moves 16 -> 32 MiB, because the legitimate ceiling it clears doubles under the new metric (that instance's whole wiki-linking corpus retains ~20,154,952 bytes); the single-document attack retains 110,729,522, so it is still refused by 3.3x. 2. The compare-and-set's retry path bypassed the guard entirely. On contention it re-reads the linker and calls ReplaceTitle on whatever the winner wrote — a NEW input, bounded by nothing the scan had checked — so a content edit landing inside the cascade's window could grow a linker from harmless to enormous and walk the rename back into the amplification it would have been refused for. Each document's compare-and-set now carries the cap less what the other linkers hold, and re-checks the grown body against it. 3. The cascade's `content LIKE ?` search term went in unescaped, so a document TITLE decided how the pattern was read. `\` is the default LIKE escape character on Postgres and NOT on SQLite, so `[[Alpha\Beta]]` was searched for as itself on one dialect and as `[[AlphaBeta]]` on the other: linkers not found, cascade rewrites nothing, rename reports success, every link left stale. Silent and dialect-dependent. Codex named the backslash; `%` and `_` are the rest of the class (CONVE-18) — wildcards on both dialects, so a title carrying them selects documents that do not link it. An explicit `ESCAPE '\'` clause plus escapeLikePattern makes both dialects agree, rather than leaving SQLite correct by accident. Finding 3 also constrains finding 3 of the ORIGINAL fix: models' validator allows a lone backslash in a title on the grounds that both renderers handle it, which was true of rendering and false of cascading. That comment now records the dependency — allowing it is only correct while the cascade's pattern stays escaped. Tests, four new, each mutation-verified against the code it guards: - CountsRetainedBytesNotJustOutput — the shrinking rename. Asserts as a PRECONDITION that the projected-output total stays under the cap, so the test cannot pass for the old reason. - RetryRecheckesTheBudgetAgainstTheGrownBody — drives the real race through the afterLinkCascadeRead seam. POSTGRES ONLY and skipped loudly elsewhere: SQLite's BEGIN IMMEDIATE closes the window structurally, so a green run there would be a property of the DSN. - FindsLinkersWhoseTitleContainsABackslash — Postgres only, same reasoning inverted: SQLite is the dialect that was accidentally right. - DoesNotSpendTheBudgetOnDocumentsThatDoNotLinkTheTitle — `%` and `_`. Its first version asserted the decoy's content was untouched and passed against the unescaped pattern, because over-matched rows rewrite to themselves. The observable harm is that they spend the caller's budget, so that is what it now asserts. Mutation matrix for this round: output-only counter -> only the shrinking test fails; retry check removed -> only the retry test fails (PG); LIKE unescaped -> the budget legs fail on SQLite and the backslash test fails on PG. Gates: `go test ./...` under Postgres 17 EXIT=0; SQLite packages EXIT=0; gofmt clean; `make lint` 0 issues. BUG-2798, BUG-2796 Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN * fix(documents): tighten the retry budget, stop charging no-op rewrites, order the typed check first (BUG-2798) Codex round 3 on #1218, an edge-case angle over the new arithmetic and control flow. Three findings fixed, one declined. 1. The retry budget credited back this document's own share, on the reasoning that the retry replaces it. It does not: the original read and rewritten bodies stay reachable through `updates` while the write loop runs, so the re-read and its rewrite are allocated ON TOP of them. The bound could be exceeded by up to one document's share while the arithmetic still reported it satisfied. The budget is now the genuine headroom, `cap - retained`. 2. A concurrent edit that REMOVES the link left a body with no occurrences, which cascadeRetainedBytes still charged twice — once for the read and once for a rewritten copy that does not exist, because strings.Replace returns its input unchanged when there is nothing to replace. That could refuse an otherwise valid rename for memory the cascade never allocates. 3. The handler classified this error by PROSE before testing it by identity. The UNIQUE-constraint arm matches a substring, and the refusal error embeds the caller's title verbatim, so renaming a document to a title containing the words "UNIQUE constraint" came back as a 409 name collision — advice to pick a different name, for a rename that was refused for size and would fail identically under any name. Typed sentinel now tested first. DECLINED: unchecked int64 arithmetic in the projection. The multiplicands are derived from the length of a string already resident in memory, so overflowing int64 needs a single document body of roughly nine exabytes; and the accumulator returns as soon as it passes the cap, so it cannot run away either. Saturating arithmetic here would be guarding a state the machine cannot reach. Tests, three new, each mutation-verified: - RetryBudgetExcludesThisDocumentsOwnStrings — deliberately separate from the existing retry test, because that one catches the check being ABSENT and this one catches it being too GENEROUS. The grown body is sized to fall BETWEEN the two budgets; a body far over the cap cannot tell them apart. - ConcurrentEditThatRemovesTheLinkDoesNotRefuseTheRename — its first version sized the link-free body against the CAP rather than against the retry's real headroom, so the refusal it caught was correct behaviour and the test was wrong, not the code. Re-sized against the headroom: fits when charged once, does not when charged twice. - IsNotMisreportedAsATitleCollision — at the handler, since the defect is entirely in its classification order. Mutation matrix for this round: credit the share back -> only the tight-budget test fails; charge the no-op body twice -> only the link-removed test fails; order the substring arm first -> only the misclassification test fails. Gates: `go test ./...` under Postgres 17 EXIT=0; touched packages re-run after the lint fix EXIT=0; gofmt clean; `make lint` 0 issues. CI green on |
||
|
xarmian
|
771ec5bbaa |
fix(server): refuse invalid-UTF-8 and NUL query values at the transport (BUG-2784) (#1217)
The query-string half of BUG-2782. A caller-supplied query value reached a
Postgres text comparison, Postgres refused the parameter, and the handler
answered 500 — the honest answer is 400, because the caller asked about
something that cannot exist.
ValidateQuery is a root-router middleware beside ValidatePath, refusing 400
invalid_query when a decoded query key or value is invalid UTF-8 or carries
a NUL. Both share one predicate, bindableText.
Measured on Postgres 17 (server_encoding UTF8) at
|
||
|
xarmian
|
1933041027 |
fix(events): surface the SUBSCRIBE error and refuse callers instead of admitting a dead stream (BUG-2764) (#1215)
* fix(events): surface the SUBSCRIBE error and refuse callers instead of admitting a dead stream (BUG-2764) * fix(watchevents): surface the SUBSCRIBE error at construction and on resubscribe (BUG-2764) * docs(events): the idle-cycle prose and metric Help now name the third install-nothing reason (BUG-2764) * fix(events): retry uncovered workspaces, wait on in-flight records, refuse after Close (BUG-2764 codex round 1) * fix(events): post-loop check waits on an in-flight record before trusting a live entry (BUG-2764 codex round 2) * test(watchevents): assert what the failed subscribe leaves, not how long it takes (BUG-2764 codex round 3) * docs(events): prose says what the code guarantees — delivery failure or shutdown, replacement refusals do not count as cycles (BUG-2764 codex round 4) * test(server): the 503 mapping is asserted on both subscribe branches; docs scope the refusal to the activity stream (BUG-2764 codex round 5, BUG-2800) * fix(events): uncovered-workspace retry logs are quiet once the bus is closing (BUG-2764 codex round 6) * fix(events): the uncovered-retry log promises a retry only while subscribers remain (BUG-2764 codex round 7) |
||
|
xarmian
|
cade263fe5 |
fix(store): compare-and-set the wiki-link cascade's writes (BUG-2785) (#1213)
* fix(store): compare-and-set the wiki-link cascade's writes (BUG-2785)
A document rename cascades through every document linking the old title
as a read-modify-write across two statements: SELECT each linker's
content, rewrite the string in Go, UPDATE the row. A content edit to a
linker committing between those two statements was silently overwritten
— the cascade wrote the body it built from the version it read, with no
error and no version row for the loss.
Same lost-update shape as BUG-2770's activity-metadata merge, one table
over, and fixed the same way: the UPDATE now carries `content = ?` with
the body the cascade read, plus a bounded retry that re-reads the row
and re-applies the rewrite.
DIALECT SCOPE, which decides what the tests can prove. Reachable on
POSTGRES only. SQLite's DSN sets `_txlock=immediate`, so UpdateDocument's
db.Begin() takes the write lock at BEGIN and holds it across the whole
read→write window; a concurrent edit cannot commit inside it and
serializes on busy_timeout instead. On Postgres under READ COMMITTED each
statement takes a fresh snapshot and the stale body wins. The CAS is
therefore a no-op on SQLite by construction — the predicate always
matches, because nobody else can have written.
Two consequences, both acted on rather than noted: the new tests SKIP
loudly on SQLite instead of passing for a reason unrelated to this fix,
and the mutation matrix was run under Postgres, where removing the CAS
leaves a SQLite suite entirely green.
The retry rewrites the WINNER's body rather than replaying the original
rewrite — replaying it would reintroduce exactly the text this bug loses.
A mutation that replays instead is in the matrix.
THE ZERO-ROW RESULT NEEDS A PROBE. The UPDATE now has two predicates that
can each refuse it, and RowsAffected cannot say which did: `deleted_at IS
NULL` (the linker was archived — a documented normal outcome, stop) or
`content = ?` (a concurrent edit landed — re-read and retry). Treating
them alike either retries forever against a deleted row or discards a
live linker's rewrite, so a probe distinguishes them, as BUG-2770 needed
for the same reason. The probe reads through tx, never the pool
(BUG-2409).
On retry exhaustion the RENAME fails rather than leaving one linker
holding a title that no longer exists. The alternative — log and continue
— was considered and rejected: it trades a loud retryable failure for a
silent inconsistency, and a rename is atomic in intent. Exhausting three
attempts needs three consecutive commits to the same linker inside one
cascade.
Adds afterLinkCascadeRead, the seam between the cascade's read and its
writes. No existing seam reaches that gap: afterDocumentPreLockRead fires
before the transaction, afterDocumentPreWrite before the renamed
document's own update.
That new seam also closes a gap a previous unit recorded as permanently
open: the `deleted_at IS NULL` guard carried a note calling itself
UNTESTED because reaching its window "would cost a fifth seam". This is
that fifth seam, so the note is removed and replaced by a record of the
closure, and the archived-linker test now drives exactly that window.
Mutation matrix, run under Postgres — 5 mutants, 5 detected, including
one that is literally the pre-fix code:
M1 CAS predicate removed (the unfixed behaviour) → lost-update test
M2 soft-delete probe arm removed → archived-linker test
M3 retry budget cut to one attempt → lost-update test
M4 retry replays the original rewrite → lost-update test
M5 seam removed (control: tests must notice they never raced)
→ lost-update test
Gates: gofmt clean, lint 0 issues, govulncheck clean, full suite green on
SQLite (28 pkgs) and on Postgres 17 (28 pkgs, internal/store 346s vs 79s
— the positive control that the PG legs ran rather than skipped).
Not fixed here, filed as BUG-2795: cascadeTitleRename, the ITEM-side
cascade, has the identical defect and no lock closes its window either.
Its fix does not transfer — it rewrites by POSITION from item_wiki_links
offsets, so a retry must re-derive positions from the winner's body and
re-run replaceWikiLinks, which is a redesign of the retry unit rather
than a predicate on an UPDATE. wiki_links.go's claim that it "matches the
document rename behavior" is corrected to say which half no longer
matches.
* style(store): gofmt the CONVE-23 sweep comment (BUG-2785)
Same failure as the previous unit, same cause, and worth naming rather
than quietly fixing: gofmt wants a blank line between list items once one
item grows a second paragraph, which the note about the previously-
untested deleted_at guard made true.
I re-ran build and the full Postgres suite after those comment edits but
not lint, because the change was 'only a comment' — the exact reasoning
the previous unit's fix commit warned about in writing, one unit earlier.
The PR body's claim that all gates were re-run after the prose edits was
false and has been corrected there too.
The rule as remembered does not work. The mechanical form does: gofmt and
lint are the last action before a push, comment-only changes included.
* fix(links,server): stop a rename hanging the server, and report cascade contention honestly (BUG-2785)
Three findings from Codex round 2 on this PR. The first is a server hang.
1. ReplaceTitle could never terminate. replaceAll looped "find old in
result, splice new in" — re-searching the string it was building,
including the text it had just inserted. When the NEW title contains
the OLD link token it grows without bound.
Measured, not argued: ReplaceTitle("x [[A]] y", "A", "A]] [[A")
builds `[[A]] [[A]]`, which still contains `[[A]]`; a probe against
the old implementation ran 3s without terminating before being
killed. Document titles have no validation, so this is reachable from
user input — and the caller is inside the rename transaction holding
the workspace rename advisory lock (BUG-2778), so the hang would take
every other rename in that workspace down with it while exhausting
memory.
strings.Replace with n = -1 has the semantics that were wanted:
non-overlapping, left-to-right, over the input. Three-case regression
test, all three of which fail against the old implementation, plus a
control that catches a "fix" which terminates by doing nothing.
Pre-existing, and folded in rather than filed: three lines against a
server hang, and this PR's retry calls the helper again per attempt,
which makes it reachable more often than before.
2. Retry exhaustion surfaced as an opaque 500. The rename rolls back
cleanly and retrying can succeed, so "an internal error occurred"
tells the caller the opposite of the truth. Adds the exported
ErrLinkCascadeContention sentinel; the handler now answers 503
lock_contention with Retry-After, reusing the disposition BUG-2778
already established for 55P03/40P01.
3. That 503's message claimed the workspace was "busy with another
rename". 55P03 there is just as likely to be an ordinary content edit
holding the row, and the new arm is definitely one. It no longer
names a cause the server has not established.
Also closes the coverage gap round 2 named around this unit's own
decision: exhaustion now has a test asserting the rename ROLLS BACK
(target keeps its title) and that the concurrent editor's text survives
that rollback. cascadeRewriteAttempts becomes a var so the test can
force exhaustion at 1 rather than arranging three consecutive commits,
which would need a per-attempt hook in production code — the divergence
from its const sibling is noted where it lives.
Records two limitations in the code rather than leaving "the cascade is
safe now" to rot: the MIRROR direction is still open (a content writer
that read before this transaction can commit afterwards and reinstate
the old title — fixing it means giving ordinary content writes a CAS
too), and delete-then-restore of a linker mid-rename brings back the old
title. Both pre-existing, neither worsened here.
Mutation matrix now 7 mutants, 7 detected, run under Postgres:
M6 (%w -> %v, sentinel lost) and M7 (exhaustion swallowed) cover the new
mechanisms.
Gates re-run on the tree being pushed, tests included, after the final
comment edit rather than before it: gofmt clean, lint 0 issues, SQLite
28/28, Postgres 17 28/28 (internal/store 345s vs 79s). gofmt caught an
unformatted test file locally this time, which is the point.
* fix(store,links): correct three prose claims and close the SQLite coverage gap (BUG-2785)
Codex round 4, on SQLite semantics and prose accuracy. One finding says a
bug I FILED is wrong; that is the important one.
1. BUG-2795's premise was false, and this PR repeated it in a comment.
I filed that item claiming the item-side cascade has "the identical
defect" and that "no lock closes its window either", on the strength
of a grep for pg_advisory_xact_lock in items.go that turned up only
the parent-link locks.
It missed acquireWorkspaceSeqLock (items.go:2136), taken
UNCONDITIONALLY by every UpdateItem — content-only edits included —
immediately after Begin and long before cascadeTitleRename, and held
to COMMIT. So two item updates in a workspace fully serialize on
Postgres and an ordinary content edit CANNOT commit inside that
cascade's window. The scenario I filed is not reachable.
Not fully invalid: sweeping every `SET content` writer in
internal/store finds RemapAttachmentReferencesInWorkspace, which
rewrites items.content in its own transaction without that lock. So a
real but far narrower window survives — attachment remap versus
cascade, not user-edit versus cascade. BUG-2795 corrected on its
trail and dropped to low; the comment here now states the lock, the
one surviving writer, and stops claiming parity with the document
cascade.
I searched for the locks I expected rather than for what serializes
that path, then wrote a sentence broader than the search. Same
failure this PR's review has produced repeatedly.
2. "A title nobody should be able to write" was false — the document API
validates doc_type and status, never the title. Correcting it
surfaced a real second defect: renaming to `A]] [[A` now terminates
(round 2's fix) but writes `[[A]] [[A]]`, two links to nothing. Filed
as BUG-2796. The termination test deliberately still asserts the
COUNT rather than the output, so it does not freeze today's broken
rendering as intended behaviour.
3. The hang's blast-radius claim named the workspace rename advisory
lock without qualifying the dialect. That lock is a no-op on SQLite,
where the equivalent damage is the database-wide write lock the
transaction already holds under BEGIN IMMEDIATE. Different mechanism,
same outcome for everyone else.
Also closes the last coverage gap round 2 named. The CAS predicate runs
on SQLite in production and every concurrency test skips there, so
TestUpdateDocument_CascadeRewritesEveryLinkOnBothDialects does not skip:
two linkers, multiple links per body, plus a document that merely
contains the word and must be left alone. Verified it earns its place —
a mutant comparing against the rewritten body instead of the body that
was read compiles, and dies to this test ON SQLITE, where nothing else
would have caught it.
Documents the parallel-test constraint on the now-mutable
cascadeRewriteAttempts, with its boundary: no current t.Parallel test in
internal/store reaches this cascade, but that is a fact about today's
corpus rather than an invariant.
Gates on the pushed tree: gofmt clean, lint 0 issues, govulncheck clean,
SQLite 28/28, Postgres 17 28/28 (internal/store 349s vs 79s).
* docs(store,links): point the cascade comment at the root cause, qualify a claim in its second location (BUG-2785)
Codex round 5, probing cross-connection visibility and transaction
boundaries. Two findings, both mine, neither changing behaviour.
1. The "one surviving writer" framing was too comfortable, twice over.
RemapAttachmentReferencesInWorkspace is not a rare non-interactive
writer: bundle import reaches it on an ordinary user-triggered
import, and the workspace is ALREADY VISIBLE to its owner while it
runs — store.ImportWorkspace commits the workspace row (with
owner_id) in its own transaction before opening the one that inserts
items, and the bundle handler runs the remap as Phase 3 afterwards.
Both verified in code.
So that writer races ordinary item edits, not just the rename
cascade, and it is missing a guard outright rather than being an
exotic pairing. Filed as BUG-2797, which covers the remap itself;
BUG-2795 is now a consequence of it and says so on its trail. The
comment here points at the root cause instead of implying the
cascade's pairing is the whole story.
2. The dialect-unqualified advisory-lock claim survived in a SECOND
location. Round 4 caught it in links.go and I fixed it there; the
same sentence sat in the termination test's comment, and I never
enumerated the sites. That is CONVE-23's verify half failing exactly
as it warns: I fixed the instance I was shown rather than the
population. Both now qualified, and a sweep for remaining unqualified
copies leaves only BUG-2778's own comment, which already carries its
no-op-on-SQLite note.
Worth recording that this is the second time on BUG-2795 that a scope
sentence of mine ran ahead of the sweep supporting it — first "no lock
closes its window either" (acquireWorkspaceSeqLock did), now "not user
triggered" (import is). Both found by a review round rather than by me.
Gates on the pushed tree: gofmt clean, lint 0 issues, SQLite 28/28,
Postgres 17 28/28 (internal/store 348s vs 79s).
|
||
|
xarmian
|
91d92f184f |
feat(server): refuse workspace creation without may_create_workspaces consent (IDEA-2756) (#1212)
* feat(server): refuse workspace creation without may_create_workspaces consent (IDEA-2756)
The OAuth consent screen's "Let this app create new workspaces" checkbox
gated only the post-creation auto-add. A connection whose user left it
unticked could still create workspaces; it simply could not then see
them. A permission that does not prevent the action it names is a
consent mismatch.
Dave ruled it: the checkbox is a permission on whether the connected
token may CREATE, and it has to be true to what a user would honestly
expect from the option. The behaviour-change-for-existing-connections
argument loses to honest consent semantics.
Adds Server.requireWorkspaceCreationConsent, a shared gate at the top of
both endpoints that mint a workspace under the caller's account:
POST /api/v1/workspaces handleCreateWorkspace
POST /api/v1/workspaces/import handleImportWorkspace
Import reaches CreateWorkspace via store.ImportWorkspace, so it is the
same permission at a second door — lead-ruled as an application of the
same rationale, not a new decision. The gate sits above the Content-Type
dispatch, so it covers the tar.gz bundle path (whose only route is that
handler) and refuses before the 64 MiB body read.
Refusal is a 403, mirroring handleAuditLog's consent refusal (BUG-2102):
a hard decline rather than a narrowed response, because there is no
narrower version of creating a workspace.
Three non-refusal cases and one refusal, all but the last with a test:
- not an OAuth grant (PAT, CLI session, local stdio) — creation rides
on ordinary account authority
- ErrOAuthConnectionNotFound (pre-Phase-C grant) — ALLOW, matching the
backfill's may_create_workspaces=ON default. Deliberately asymmetric
with maybeAutoAddCreatorConnection's not-found branch, which declines
a convenience where this one would invent a refusal
- flag set — proceeds; the auto-add is unchanged
- a store I/O error — REFUSED, failing closed with a 500, because
allowing the create when the deciding state could not be read grants
a declined permission on the strength of a database blip. This is
the one branch with no test: injecting a store read failure needs a
fault-injecting store the package does not have, so it is reasoned
rather than measured
Population enumerated before the fix (CONVE-18): five CreateWorkspace
call sites, two of them HTTP endpoints reachable by an OAuth token (both
gated). Excluded with reasons: autoCreateWorkspace (signup-time, no
connection in context), workspace restore (un-deletes an existing
workspace), /oauth/claim (grants access, does not mint), cmd_db.go
(local store copy, no HTTP). Search boundary: the sweep traced
Store.CreateWorkspace callers and did not look for a path that inserts a
workspace by raw SQL.
Ten tests, all driving the real router rather than calling handlers
directly (CONVE-19). Every refusal leg asserts that no workspace of that
name exists afterwards, not merely the status code (CONVE-12) — a guard
that 403s after the write passes a status-only assertion. Seven mutants,
seven detected, including both guard-placement mutations.
MCP tool surface 0.25 -> 0.26. Behaviour bump on the v0.9/v0.16/v0.25
grounds: no tool name, action enum or param shape changed, but
pad_workspace.create now refuses a call it used to permit. Closest
precedent is v0.10; unlike v0.10 there is deliberately no escape-hatch
param, because the gate encodes a decision the USER made at consent time
and a bypass flag would be the app overriding its own grant.
CONVE-23 sweep for prose the change falsified: instructions.md told
agents the create still succeeds and to use the claim flow (it would
have sent them to claim something that was never created); the
TASK-2753 allow-list guard entry asserted the same and posed IDEA-2756
as open; the MCP catalog and CLI help described only the flag=true path;
maybeAutoAddCreatorConnection's flag-off branch is now unreachable from
its sole caller and is documented as dead code kept for contract, to be
deleted only with the guard. CLAUDE.md was already stale at v0.24 (v0.25
bumped the constant without it) — brought to v0.26 with a backfilled
v0.25 line.
The consent screen and console copy are unchanged: they were the
misleading half of this bug, and the fix makes them true.
* docs(server): state the import gate's reachability precisely (IDEA-2756)
The import-side gate is correct but currently unexercised in production,
and the first framing of this change did not say so.
WithMCPTokenIdentity is stashed by exactly one middleware, MCPBearerAuth,
mounted on /mcp alone. An OAuth connection reaches an /api/v1 handler
only through the in-process MCP dispatcher, and that dispatcher's route
table has a workspace create action but no workspace import. So no
OAuth-bound caller can reach handleImportWorkspace today.
The gate stays, and the comment now says why: adding that action later
must not silently reopen the door, which is the state a create-only fix
would have left armed.
Found on a verify pass reading the middleware mount points, not by the
tests — they synthesize the OAuth identity into the request context, so
they prove the handler's behaviour GIVEN an identity and have no opinion
about which routes supply one (CONVE-19). Codex round 2 reached the same
conclusion independently.
* fix(server): correct five overstated claims from Codex round 3 (IDEA-2756)
All five were mine, all P2, none changing the gate's behaviour — four are
claims that were broader than the code, one is a test that proved less
than its name.
1. "Only re-authorization lifts it" was wrong in five places (version.go,
README, CLAUDE.md, the MCP catalog description, CLI help). A user can
also enable the flag on the EXISTING connection via
PATCH /connected-apps/{id}/flags, which the console page drives —
instructions.md said so and contradicted the others. All five now name
both remedies, and both are still the user's, which is the part that
matters: neither is reachable by the app.
2. "This branch is UNREACHABLE ... it is dead code" on
maybeAutoAddCreatorConnection's flag-off branch was false. The gate
reads the connection and that function reads it AGAIN after creation;
a user revoking creation power from the console between those two
reads lands exactly there. It is a real second check across a real
TOCTOU window, failing in the safe direction. The claim was written
from the call graph, which cannot see a concurrent write between two
reads.
3. handlers_import_bundle.go's "Auth: any authenticated user" was made
false by this change and the concept sweep never had a chance at it —
it greps may_create / auto-add / creation power, and that sentence
contains none of them. Corrected in place.
4. The two NonOAuthCallerUnaffected tests claimed PAT, CLI session and
local stdio; each drives one PAT. The comments now state the fixture's
real scope and why one caller stands for the class (the guard branches
on an identity only MCPBearerAuth sets, so callers that skipped it are
indistinguishable) rather than implying three fixtures.
5. The JSON import refusal leg would have passed with the gate below
decodeJSONWithLimit — only the bundle leg pinned placement, and only
for gzip. Adds TestImportWorkspace_ConsentRefusalPrecedesBodyDecode
(malformed body: 400 if the gate is late, 403 if it is early),
mirroring the create-side ordering legs.
Mutation matrix now 9 mutants, 9 detected. M8 (guard below the JSON
decode) is killed by the bundle leg too, so it shows the new test is
covered rather than necessary; M9 gates the bundle path and moves only
the JSON path's guard, and dies to the new leg ALONE. That is the mutant
that justifies the test.
* docs(server): the second consent check narrows the race, it does not close it (BUG-2792)
Round 3 caught me calling maybeAutoAddCreatorConnection's flag-off
branch dead code. The replacement comment then claimed the branch means
a revoked grant cannot silently gain a workspace — which is more safety
than the code delivers, and round 4 caught that.
The read and the AddConnectionWorkspace insert below it are separate
unconditional statements, so a revocation landing BETWEEN them still
adds the workspace. The check narrows the window; it does not close it.
Filed as BUG-2792 rather than folded in: the race is pre-existing and
unchanged by IDEA-2756, and closing it needs an atomic check-and-insert
at the store layer, written and gated for both dialects — materially
more diff and risk than this handler-level guard.
Both mistakes were the same shape in opposite directions: a claim about
concurrency derived from reading the call graph, which cannot see a
concurrent write between two reads.
* style(server): gofmt the doc comment (IDEA-2756)
gofmt wants blank lines between list items once one item spans multiple
paragraphs, which the BUG-2792 note made true.
My error, and worth naming exactly: I ran build, vet and the targeted
tests on this commit but not lint, because lint had passed on the
PREVIOUS commit and the change was 'only a comment'. The gate has to run
on the tree being pushed, not on an earlier one that resembles it. CI's
golangci-lint is pinned to the same v2.11.4 the Makefile installs, so
there was no version skew to blame — the local gate would have caught
this in 51 seconds.
* docs(server): correct ten overstated prose claims from Codex round 8 (IDEA-2756)
Round 8 reviewed only the prose this change adds. Ten claims were
broader than the code. All ten are mine; none changes behaviour. Rounds
3, 4 and 7 each caught one of these, which is why round 8 was pointed at
the class rather than at a new dimension.
The substantive ones:
- "gates every endpoint that MINTS a workspace" — autoCreateWorkspace
mints from registration, bootstrap and oauth-login and is deliberately
outside this gate. The helper doc and the test header now name the two
callers and the exclusion instead of claiming universality.
- "the agent was handed a workspace it could not then see" (version.go,
README, CLAUDE.md) — only true for a connection with an EXPLICIT
allow-list. An all_current_workspaces=true connection is not gated per
slug and could see what it made. The consent mismatch is the constant;
the invisibility was its most visible symptom, not its definition.
- "ErrOAuthConnectionNotFound — a pre-Phase-C grant" asserted a cause the
code cannot know: ANY missing row takes that branch. Now stated as the
expected cause, with the limit of what the code can tell.
- "above the 64 MiB body read" conflated the two import paths. 64 MiB is
the JSON decode's bound; the bundle path has its own, much larger. The
gate precedes both, which is the property that actually matters.
- "the request context is decorated AFTER TokenAuth runs" was false, and
inherited verbatim from the sibling helper this was modelled on
(handlers_oauth_claim_test.go's doClaim), where it is also false. The
wrapper sets the identity BEFORE ServeHTTP; it survives because
nothing on the /api/v1 chain writes that key.
- "lets CreateWorkspace normalize it" — CreateWorkspace slugifies only
when the supplied slug is EMPTY, and import supplies a non-empty one,
so an imported workspace keeps the ?name= value verbatim.
- "The PAT needs a workspace to bind to" — CreateAPIToken takes
WorkspaceID as optional.
And one where the first fix was worse than the finding:
- "Every refusal leg asserts no workspace exists afterwards" was false —
the two ordering legs assert status only. My first correction ADDED
those assertions, which is the trap the finding was pointing at: a
malformed body and an empty name are rejected before creation under
every guard placement, so "no such workspace exists" is true of broken
and working code alike. Reverted; the header now states which legs
carry the counterfactual, and why the ordering legs discriminate on
status instead.
Gates re-run on the tree being pushed, not an earlier one: gofmt clean,
lint 0 issues, internal/server and internal/mcp green, mutation matrix
still 9/9.
* ci: re-trigger CI after a GitHub startup_failure (IDEA-2756)
No code change. The Go job on
|
||
|
xarmian
|
6b5e8be04e |
fix(server): a structured timeline id must not collide with a row id (BUG-2783) (#1210)
* fix(server): a structured timeline id must not collide with a row id (BUG-2783)
A note or decision id comes from the item's fields blob, which nothing
validates on write. It can equal the id of a comment, activity or version
on the SAME item, and then two entries in one timeline payload share an
id — which the client's keyed {#each} and its by-id page dedupe resolve by
hiding one of them. The intra-structured dedupe already prevented exactly
this failure one source-boundary in; `usedIDs` simply never knew about the
other four sources.
Fixed by seeding `usedIDs` from the ids the SQL sources already own. All
three slices are in hand at the call site one line above, so this needs no
extra query and no re-ordering.
The structured side is the one that yields, and that is a decision rather
than a side effect: comment, activity and version ids are real primary keys
AND are what the client sends back as `before_id`, so moving one would break
paging. The unvalidated side moves.
Tests drive the MERGED payload through the real endpoint rather than
structuredTimelineEntries, which is the reason the gap survived: the
builder is correct in isolation, so a unit test of it vouches for the
component and not its binding to the other sources.
Two things the assertions are deliberately specific about. They name WHICH
entry moves — "two entries are present" passes trivially even unfixed,
since the collision is resolved by the client rather than the server, and
"the ids differ" would pass if the comment had been the one to yield, which
is a different bug. And the activity case asserts the row that owned the id
is still in the payload, since the structured entry yielding is only correct
if the thing it collided with survives.
Also closes a pre-existing coverage gap this work surfaced: a mutation
deleting the `for usedIDs[id]` fallback-uniqueness loop survived the entire
timeline suite. It is now covered — and the fixture took two attempts, which
is the part worth recording. A note with no id followed by a note claiming
"note-idx-0" does NOT reach that loop: the second note's raw id is already
used, so the `!usedIDs[raw]` guard diverts it to the fallback branch and it
gets a free name. That fixture passed with the loop deleted. Reaching the
loop requires the fallback NAME to be occupied when the fallback is
computed — note 0 claiming the literal "note-idx-1", note 1 having no id.
Mutation matrix, all four compiling: dropping the seeding entirely, seeding
from comments only, seeding from activities/versions only, and dropping the
fallback loop are each detected, each by the test aimed at it.
Scope note recorded in the code: the fallback half is defence rather than a
live vector across sources, because all three SQL sources mint ids with
store.newID() — including the import path, which re-mints rather than
preserving an artifact's ids — and a UUID cannot equal `note-idx-N`. Within
the structured kinds, where both ids come from the same unvalidated blob, it
is reachable.
Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X
* fix(server): make the structured id independent of the page, not of the window (BUG-2783)
Codex round 1, P1, and it refutes the approach the recon proposed and the
lead ratified — reported rather than quietly redesigned, and replaced here
because the defect was mine and introduced by the previous commit.
Seeding the dedupe map from the ids fetched FOR THIS PAGE makes a structured
entry's id depend on the page window. The three SQL windows are cursor-
dependent, so the same note takes its raw id on a page where the colliding
row is absent and a positional id on one where it is present. That id is not
merely a render key: it is the SECOND TERM OF THE CURSOR PREDICATE, so a
window-dependent id makes an entry's own sort position depend on which page
is being built, and paging can then skip or repeat it. Worse than the bug it
was fixing, and a regression I introduced — before the previous commit,
structured ids were a function of the item alone.
Replaced with a SHAPE test: a raw blob id that is UUID-shaped is refused and
takes the positional fallback. That is sound because the only ids it can
collide with are comment, activity and version rows, and every one of those
takes its id from store.newID() — uuid.New().String(). Enumerated rather
than sampled: all six INSERT sites into those three tables pass newID(), and
the import path re-mints rather than carrying an artifact's ids across. So a
blob id can only equal a row id by being a UUID.
The result needs no extra query, consults nothing outside the item, and
changes ids only for blob ids that are well-formed UUIDs — which is exactly
the colliding case. A non-colliding UUID-shaped blob id also loses its raw
id; accepted, since it cannot be distinguished from a colliding one without
consulting the very rows this must not depend on.
Four more findings from the same round, all fixed:
- No version test. A fix knowing only about comments and activities passed
the whole file — which is what the seeding version did, since it
enumerated sources by hand. Added.
- The activity test called t.Skip when it found no activity. Creating an
item writes an activity row, so absence means the fixture stopped building
what the test needs, and a skip reports that as success forever. Now
t.Fatalf. Same for two skips in the new window test.
- The comments described the client as hiding one of a colliding pair. Half
true: WITHIN a page it is a keyed-each duplicate key, which is a Svelte
ERROR; the silent drop is the ACROSS-pages append filter. Both stated.
- "The other four sources" — there are three SQL-backed ones.
Mutation matrix, 5, all compiling: dropping the shape test, inverting it,
making the helper never fire, dropping the fallback loop, and dropping the
intra-structured dedupe are each detected.
The last one first looked like a survivor. It was not — it dies on
TestItemTimeline_StructuredDuplicateIDsAreDisambiguated, whose name my
`-run TestTimeline` filter never matched. A `-run` filter narrower than the
population is the same false-green family as a mutant killed by the
compiler: the harness reports SURVIVED and the reason is the instrument.
Re-run unfiltered before believing any survivor.
Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X
* fix(server): a diverted id must not inherit an unstable one (BUG-2783)
Codex round 2. Both findings are about the REPLACEMENT, not the original
bug, and the first is a hazard my own fix widened.
P1: the positional fallback encodes the entry's array index, so inserting
or removing an entry ahead of another renumbers it — and the entry id is
the cursor's tie-breaker, so a renumbered entry can be skipped or re-shown
across a page boundary. That instability PREDATES this work and applies to
entries with an absent or duplicate id. But the shape test diverted every
UUID-shaped raw id onto that same path, which would have taken a small
population and made it a much larger one, for no benefit.
Fixed by giving a shape-refused id a DERIVED id, `<prefix>:<raw>`, instead
of a positional one. It depends only on the entry's own raw id, so it is
stable under any mutation of the blob, and the prefix is what makes it safe:
a row id is a bare UUID, so `note:<uuid>` cannot equal one. It still passes
through the duplicate guard, because a blob may contain a literal
`note:<uuid>` string of its own.
What stays on the positional path is exactly the population that has
nothing else to derive from — absent ids and duplicates. That residue is
filed as BUG-2788 with the two consequences spelled out and three candidate
fixes, rather than being quietly absorbed here.
The new test asserts the property the derived id exists for: the same entry
keeps the same id after an unrelated entry is inserted BEFORE it. That
holds for a derived id and cannot hold for a positional one — the mutant
that removes the divert fails exactly this test and nothing else.
P2: the UUID-shape premise is a property of the WRITERS, not the schema.
All three tables are `id TEXT PRIMARY KEY` with no format constraint and
migrations carry ids verbatim, so a row whose id is not a UUID — from a
future path bypassing newID(), or already present in a database this code
has never seen — would be outside what this refuses. Latent rather than
reachable through any current write or import path. Now stated in the code,
including that the enforcement which would close it belongs at the writers
or the schema and not here, since detecting it needs exactly the row lookup
this design exists to avoid.
Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X
* fix(server): close the derived namespace, and pin its contract (BUG-2783)
Codex round 3. Both findings are about the round-2 replacement.
P1, and it falsifies a scoping sentence I had just written on BUG-2788: a
blob holding BOTH `note:<uuid>` and `<uuid>` had the second derive onto the
first's KEPT id, fail the duplicate guard, and fall through to the
positional path. Two distinct, legitimate raw ids, one of them made
unstable — and not covered by the absent/duplicate exception BUG-2788
describes, so that item's scope was wrong within the hour of being filed.
The fix generalises rather than patching the case. Divert any raw id that
could be confused with a derived one — UUID-shaped OR already beginning
with `<prefix>:` — and derive by prepending. Two rules then hold together:
a kept id never begins with the prefix, a derived id always does. So the
namespaces are disjoint BY CONSTRUCTION instead of by luck, two derived ids
are equal only when their raws are equal (the duplicate case), and a bare
UUID row id can equal neither form. BUG-2788's residue is unchanged —
absent and duplicate ids, the population with nothing of their own to
derive from — but now for a sound reason; corrected on its trail.
P2: the tests could not tell this fix from its alternatives. "Stable and
not the raw id" is satisfied by a constant; the collision tests' inequality
checks are satisfied by the very positional fallback this replaced. Two new
tests assert the contract instead of a symptom — the exact `<prefix>:<raw>`
form, distinct raws deriving to distinct ids, a prefix-carrying raw id
deriving to `note:note:<uuid>`, and no entry with a usable raw id landing
on `-idx-`.
The second new test closes a loop this design owed: a derived id is
SERVER-MINTED and the client sends it back as `before_id`, which BUG-2774
taught the server to refuse when the database would. Paging from a derived
cursor is now asserted to be accepted — a new id format must not be
refusable by our own validation.
Matrix now 9, all compiling: the three from this round are stop diverting
the prefix form (dies only on the contract test, confirming the hole was
real and is now covered), derive from a constant, and drop the divert
entirely.
Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X
* fix(server): reserve BOTH structured prefixes, not the calling one (BUG-2783)
Codex round 4, and it is the round-3 hole one kind across — which is the
part worth recording, because I fixed round 3's case and called the
namespace "disjoint by construction" while the construction only held
within a single kind.
Notes and decisions are numbered by separate entryID calls but share one
`usedIDs` map. Checking `prefix+":"` therefore checked only the CALLING
kind: a note whose raw id is `decision:<uuid>` was kept, and a decision
whose raw id is `<uuid>` then derived onto exactly that string, failed the
duplicate guard, and landed on the index-dependent positional path. Same
defect, same consequence, one substitution away from the case I had just
closed.
Fixed by reserving both prefixes globally — `hasStructuredPrefix` tests the
raw id against every namespace entryID mints in, regardless of which kind is
being numbered. The invariant is now what I claimed last round: a kept id
begins with no structured prefix, a derived id begins with one, so the two
sets cannot intersect.
The new test asserts forms rather than inequality, and then mutates the
notes array to show the DECISION's id does not move — which it could not
survive if the note had pushed it onto the positional path.
Matrix now 12, all compiling. The three added here: check only the calling
kind's prefix (dies on the cross-kind test), and dropping either "note" or
"decision" from the reserved list — each dies on a different test, so the
list is not covered by one case standing in for the other.
Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X
* docs(server): write down what the id scheme closes and what it does not (BUG-2783)
Codex round 5 answered the question I asked it — enumerate every way two
entries can share an id or one entry's id can change without the entry
changing — and returned no new defect. All three residuals were already
known: the positional-fallback instability is BUG-2788, and the two row-id
gaps are the latent writers-not-schema class already documented in
looksLikeRowID.
Recording the enumeration itself, because the next reader should not have
to re-derive it and because "everything else is handled" is exactly the
kind of claim that rots silently. Six closed cases named with the mechanism
that closes each; two open ones named with where to read about them.
One consequence from that round WAS new, and it is on BUG-2788: removing
the FIRST of two entries sharing a raw id changes the survivor's id KIND,
not merely its number — while both exist the second is pushed onto the
positional fallback by the duplicate guard, and once the first is gone the
survivor keeps its own raw id. Same skip-or-repeat consequence, reached
without any index moving. It also rules out that item's option (2):
deriving from content does not help, because two entries with the same raw
id still need a tie-break. Only persisting an id at write time makes an
entry's identity independent of what its siblings do.
Also extended looksLikeRowID's scope note with the second face of the same
gap: the three SQL sources keep their ids verbatim and are not deduped
against each other, so two rows in different tables sharing an id would
collide with no structured entry involved.
Findings by round: 5, 2, 2, 1, 0-new. No behaviour change in this commit.
Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X
|
||
|
xarmian
|
fab38336f4 |
fix(store): never return an activity id alongside an error (BUG-2779) (#1209)
CreateActivity returned the id of the row it had just FAILED to insert. CreateActivityDebounced returned the id of the row it had CHOSEN but not written. The item-update handler discarded the error and linked a user's comment to whatever came back, so the comment appeared under an activity entry describing a different change — attributed, by TASK-2760's agent-name rule, to whoever wrote that entry. The request answered 200. The contract is now: a non-nil error is always paired with an empty id. Three error returns changed and the function's doc states the rule, so no future caller can misuse a value that looks usable. The item-update handler CHECKS the error and logs it. It deliberately does NOT re-zero the id: with the contract fixed that would be a second mechanism for a window the first already covers, it would hide which one is load-bearing, and it would keep passing if the contract regressed. The log is the part nothing else provides. The population the filing left open: logActivityWithMetaReturningID has exactly three callers, and the two comment handlers ALREADY guarded correctly. This brings the third in line with its siblings rather than inventing a pattern — which is why the caller-side half is one line. TESTS, and two instrument failures found on the way: - The first fixture reached a different branch. Closing the database made the CANDIDATE READ fail, so the call fell back to CreateActivity and the merge's error return never executed — the assertion passed for a reason unrelated to the line under test, and the mutation restoring `return existingID, err` survived. BUG-2770's afterDebounceRead seam fires between the read and the write, which is the window that matters. - "No instrument can reach it", written on the classifier's error return, was a fact about the seams I had written rather than about the code. Review pointed at the seam I had not written; afterDebounceRefusal fires between the zero-row refusal and the probe, and that mutation now dies. 5 mutations aimed, 4 die. The survivor is the handler's log line, kept and documented in place: asserting it needs an activity write that fails while the item update succeeds, and a seam existing only so a test can read a log line is not worth a field on Server. Also corrected: a comment in the comment handler that said CreateActivity returns an id on insert failure. True when written, falsified by this change, and missed by my own sweep because I grepped the mechanism's name instead of the claim. |
||
|
xarmian
|
e4e914d399 |
fix(server): reject path segments the database cannot be asked about (BUG-2782) (#1207)
* fix(server): reject path segments the database cannot be asked about (BUG-2782) Every handler that resolves a workspace, collection, item, comment or attachment from a URL path segment passes that segment to the store verbatim, and the store binds it into a text comparison. Postgres refuses a text parameter that is not valid UTF-8 or that contains a NUL (SQLSTATE 22021 / 22P05); the driver surfaces that as a query error and the handler answers 500. SQLite accepts both bytes and matches nothing, so the same request is a clean 404 there — a dialect divergence that leaves the defect invisible to self-hosted installs and live on Pad Cloud. Measured before the fix, driving every route that carries a path parameter with one segment set to "bad-%FF-x" (247 probes, one per parameter position per method, real values elsewhere): Postgres answered 500 to 191 of them, SQLite to 0. After: 0 and 0, all 247 answered 400. Fixed with one root-level middleware rather than at ~112 chi.URLParam call sites, because this is a transport-level input rule and per-call-site fixes rely on every future route remembering. ValidatePath rejects a request whose percent-DECODED path is not valid UTF-8 or contains a NUL, before routing. It validates r.URL.Path rather than what chi hands the handler. chi routes on RawPath when non-empty and Path otherwise, and Go populates RawPath only when the client's escaping is not already canonical — Go escapes 0xff as uppercase "%FF", so the CANONICAL form any ordinary client emits is exactly the one that reaches the store decoded, and the lowercase "%ff" oddity is the harmless one. Validating the decoded path answers both identically and does not depend on chi continuing to prefer RawPath. It cannot refuse Pad's own URLs: store.slugify emits only [a-z0-9-], ids are UUIDs or hex, refs are a prefix plus digits. Valid non-ASCII segments pass through untouched — the database accepts them and they may legitimately name something. 400 rather than 404 because the request is malformed as a URI and the answer does not depend on whether anything exists, so it is not an existence oracle. Scope is the path; the query string is validated at its points of use, per BUG-2774's validCursorID. Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X * fix(server): the invalid-path rejection must look like every other API error (BUG-2782) Codex round 1, verified before acting on: ValidatePath runs on the root router, so its rejection short-circuits ABOVE the /api/v1 group's cors.Handler and jsonContentType and inherited neither. Measured — the 400 carried a JSON body sniffed as text/plain and no CORS headers at all, while a normal 404 on the same route carried Content-Type: application/json plus the full CORS set. On a cross-origin deployment (PAD_CORS_ORIGINS set) the browser refuses to let the page read a response with no Access-Control- Allow-Origin, so a debuggable 400 arrives as an opaque network error. Fixed without duplicating the CORS configuration: the group's cors.Handler is hoisted into one shared instance, the group mounts it as before, and ValidatePath serves its rejection THROUGH the same instance. Content-Type is set explicitly, since jsonContentType is mounted below and never runs for a rejection. Moving ValidatePath down into the group instead was rejected: two covered routes live outside it — the SPA catch-all and /api/v1/collab/{itemID} — and the mutant that makes that move is caught by exactly those two subtests. A genuine preflight (Origin + Access-Control-Request-Method) to an invalid path is answered 200 by the shared handler, the same as for any other path: a preflight asks whether the method and headers are permitted, not whether the resource exists. The real request that follows still gets the 400, and can now be read. Asserted rather than described. The new test compares each header on the 400 against the SAME route answered normally, for an allowed origin AND a disallowed one, so it pins parity with the API's own errors rather than a header list copied from a spec — and the disallowed-origin leg is what would fail if the rejection echoed origins the shared handler refuses. Mutation matrix, all nine verified to COMPILE first: dropping the CORS decoration and dropping the explicit Content-Type are each detected, and only by this new test. Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X * test(server): pin the ordering decision the path check makes (BUG-2782) Codex round 2, angle rotated to middleware contracts: a rejected request never reaches TokenAuth, SessionAuth, RateLimit or CSRFProtect, because ValidatePath sits on the root router above that group. The finding is factually right and the ordering is deliberate, but nothing in the diff said so and no test held it — which is the same defect shape as an undocumented invariant: true today, unenforced tomorrow. Verified rather than argued, because "bypasses the rate limiter" reads as a weakening and here the direction is inverted. Before this middleware, the same request ran SessionAuth — a store.ValidateSession round trip — then the limiter, then a handler whose query the database refused, and answered 500. It now costs a UTF-8 scan and a short JSON write with no database contact, so the unmetered path is strictly cheaper than every path the limiter protects. The answer is also constant for all inputs of this shape, independent of auth and of existence, so a flood learns nothing. And the limiter is a plain token bucket per key — no escalating ban, no durable block — so skipping it defeats no state that outlives the request. The alternative, metering it inside the /api/v1 group, trades this for a real coverage hole: the SPA catch-all and /api/v1/collab/{itemID} are mounted outside that group. The test floods 80 invalid paths from one IP (burst is 60), requires all 80 to be 400 and none 429, then requires a VALID request from the same IP to still get the resolver's 404 — proving the budget was untouched. It then asserts its own premise: the same volume of valid requests from a second IP must actually hit the limiter, because an inert limiter would produce an identical reading for the first half. Both mutants land where they should: metering the rejection fails at request 61 (burst 60 + 1, which independently confirms the constant cited above), and disabling the limiter fails the premise check rather than passing quietly. Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X * docs(server): correct four claims this branch's own measurements refute (BUG-2782) Codex round 3, angle rotated onto prose accuracy. No behaviour changes — every finding is a sentence that was stronger than what was verified, and in two cases stronger than data already sitting in this branch. 1. "It rejects exactly what the DATABASE rejects." Too strong, and inherited verbatim from validCursorID. Postgres refuses these two classes under a UTF8 database encoding; SQL_ASCII accepts the same bytes, and SQLite's sqlite3_bind_text accepts arbitrary sequences with NUL undefined rather than erroring. Pad neither creates nor configures that database — nothing issues CREATE DATABASE or sets client_encoding — so the encoding is the operator's. Now stated as what it is: the strictest reading, applied uniformly so the two backends stop disagreeing about the same request, measured against postgres:17-alpine at its defaults. A first draft of this correction replaced the overstatement with a NEW unverified claim ("the encoding Pad's migrations create"). Grepping for CREATE DATABASE found it only in test helpers. Fixing an unchecked sentence with another unchecked sentence is the same defect wearing the repair's clothes. 2. "Against unfixed code these are 500 on Postgres and 404 on SQLite." False for 56 of the 247 pre-fix probes, and my own sweep output said so — routes whose authorization or configuration gate answers before any store call (admin user lookup; attachments with no storage configured). Replaced with the pasted distribution: 500:191 404:34 403:12 401:4 503:4 400:2. 3. "Passed through untouched" oversold what this middleware guarantees. It does not touch a valid path, but chi still hands the handler the ESCAPED text whenever RawPath is populated: "caf%C3%A9" arrives as "café", the non-canonical "caf%c3%a9" arrives literally, and "%2F" never becomes a separator. Pre-existing chi behaviour, unaffected by this change, written down because the obvious reading is stronger than the truth. 4. "The request is malformed as a URI." It is not — "%FF" and "%00" are syntactically valid percent-encoded octets. The 400 is because the DECODED value cannot be a resource identifier here, which is the actual reason and a different one. Also reconciled the two probe counts that appear in this branch's history (111/94 GET-only, 247/191 all methods) so a reader meeting both does not have to guess which is wrong; they are one sweep at two widths. CONVE-23 sweep: finding 1 falsifies the same sentence in validCursorID (handlers_timeline.go, BUG-2774), which is where this branch inherited it. Corrected there too rather than left standing — the rule that comment describes is unchanged and still right; only its claim about the database was wrong. Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X * test(mcp): drive the in-process transport seam the path check walks past (BUG-2782) Codex round 4's exploration pointed at the door I had asserted rather than driven: the remote /mcp transport does not reach the server over a socket. HTTPHandlerDispatcher SYNTHESIZES an *http.Request and calls Handler.ServeHTTP in-process, so "the middleware covers every route" was a claim about a path this transport bypasses on its face. Driven, it is covered — and for a chain nothing in the tree stated: Handler is the *server.Server, chi's Mux.ServeHTTP runs mx.handler (middlewares + routeHTTP) on BOTH branches, and buildAuthedRequest forces the fresh-routing branch with a typed-nil RouteCtxKey. Every link is load-bearing and none was written down; this test is what notices if one changes. The counterfactual was worth more than the confirmation. Unfixed, an MCP agent that put an invalid byte in a ref got upstream_error on Postgres — whose hint says the failure is "usually transient, retry" — for an input that can never succeed. An agent obeying that hint retries forever. That is the retry-hostile misclassification family BUG-2675 added a code for, and this change removes an instance of it that nobody had noticed. Now validation_failed: the agent is told its INPUT is wrong. The first version of this test named upstream_error in its comment while running on SQLite, where unfixed gives item_not_found instead — an assertion that would have failed for a reason other than the one it named. The comment now states both backends separately and the fixture takes Postgres when PAD_TEST_POSTGRES_URL is set, so under make test-pg the dangerous half is what actually runs. Control leg included: a valid-but-absent ref must still return item_not_found, or a dispatcher that refused every ref would pass. Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X * chore(mcp): remove a throwaway probe that was committed by accident (BUG-2782) The probe that established the MCP seam behaviour was meant to be deleted once dispatch_http_invalid_path_test.go replaced it. The 'rm' was written as the first half of a compound command whose second half the tool layer REJECTED, so the whole command never executed — and a later 'git add -A' swept the file in. It duplicates the real test with printf-style output and no assertions. The rule this breaks is one I already hold: verify the mutation, not the report of it. I read 'rm -f X && cat > Y' as having removed X because I wrote it, when the command never ran at all. A rejected command and a successful one look identical in a transcript if you do not look. Caught by a Codex file listing showing an A for a file I believed gone. Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X * test: five corrections from the final review pass (BUG-2782) Codex round 5, judging the whole change. All five are mine; none needed a behaviour change. **A flake I built in.** The rate-limit test flooded 80 requests against a bucket of burst 60 — but a token bucket REFILLS while the loop runs, at 10/s here, so 20 tokens of headroom is 2 seconds of tolerance and a slow or -race'd run would admit all 80 and fail spuriously. The margin that matters is not flood-vs-burst but how long the loop must take for refill to cover the excess. At 400 requests that is (400-60)/10 = 34 seconds against in-process calls measured in microseconds: four orders of magnitude. The constant now carries that derivation, including the rate and burst it depends on. Both mutants still land, and metering the rejection still fails at request 61 — burst 60 + 1, unchanged by the larger flood. **A claim about MCP that JSON does not support.** The seam test's comment said an agent could put "a raw invalid byte" into a ref. Measured with encoding/json instead of assumed: raw 0xff / lone surrogate / truncated sequence → U+FFFD, valid UTF-8 raw 0x00 → JSON parse error the u0000 ESCAPE → a real NUL So exactly one of the five cases is reachable end to end over a JSON transport, and it is the one only the NUL half of validPathText refuses. The raw-byte cases stay — Dispatch is a Go API and the JSON decode is upstream of that boundary, so they assert the seam holds for callers that do not launder their strings through encoding/json — but the comment no longer offers them as evidence a JSON client can send them. **Two prose overstatements the earlier sweep missed.** The control test still said the rule rejects "only what the database rejects", which the previous commit had already established is false in the permissive direction. And TestValidatePathPostgresNoInternalError was described as reproducing the original 500 when it runs the FIXED server and can only ever observe a 400; the 500 lives in the counterfactual sweep and in the mutation matrix, and a test cannot both apply a fix and witness the bug. **One dead construction**, plus a smaller instance of the same habit: the MCP fixture built a SQLite store and discarded it in Postgres mode. My first attempt replaced the comment with one claiming the branch had been hoisted, and left the code as it was — writing the fix into the prose instead of the code, in the same hour I committed a message about not doing exactly that. Now actually branched. Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X * docs+test: seven more corrections, and one the emoji route earns (BUG-2782) Codex round 6, reading the three files as they now stand. All prose or assertion strength; one of them changes what the tests cover. **Pad DOES emit a non-ASCII path segment, and I said it never does.** `DELETE /workspaces/{ws}/comments/{commentID}/reactions/{emoji}` — the web client sends `encodeURIComponent(emoji)`. So the justification I gave for "it cannot refuse Pad's own URLs" was false in its premise while true in its conclusion, which is the worst combination: a reader checking the premise finds a counterexample and has no reason to trust the rest. It is also the best possible illustration of why the rule permits valid non-ASCII, so the control test now drives that ACTUAL route with a real emoji rather than relying on an emoji-shaped item slug — the claim is true by construction instead of by careful wording. A new mutant confirms the leg discriminates: a rule that rejects all non-ASCII (the plausible wrong version, not the absurd one) is caught there. **"The handler answers 500" was universal and is not.** Handlers that collapse a resolution error into not-found already answer 404 — the timeline handler's `err != nil || item == nil` is the example. My own measured distribution said so; the sentence did not. **"Self-hosted installs never see it" was wrong about the axis.** The split is by BACKEND, not deployment: a SQLite install never sees it, any Postgres install does — Pad Cloud and a self-hoster on Postgres alike. **A stale cross-reference of my own making.** The previous commit corrected TestValidatePathPostgresNoInternalError's claim to reproduce the 500, and left the sentence POINTING at it still saying it does. Fixing a claim at one site and leaving its pointer false is the CONVE-23 case in miniature. **The MCP test asserted too little.** validation_failed is how the dispatcher classifies ANY 400, so the test could have passed on a mapper-level refusal without ValidatePath running at all. It now pins the middleware's own message, which rides through on the hint. **Two overstatements in the same file.** "Exactly one case is reachable" should be one input CLASS (two cases carry a NUL). And the raw-byte cases do not cover "the stdio path": local stdio MCP is ExecDispatcher, which shells out to the binary and never touches this in-process door. Scope now says HTTPHandlerDispatcher and says what it does not speak for. Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X * docs(server): two qualifications the file already owed itself (BUG-2782) Codex round 7. Two, both narrow — the review is converging (7 findings last round, 2 this one), and both are internal inconsistencies rather than new ground. "Any Postgres install does" contradicted a qualification made forty lines lower in the same file, where validPathText spells out that SQL_ASCII Postgres accepts these bytes. Now says a Postgres install whose database encoding is UTF8, notes that this is initdb's default, and points at the place the qualification lives so the two cannot drift apart again. validCursorID's paragraph still described the 500 in the present tense, though BUG-2774 fixed it — it is the behaviour the guard PREVENTS, not what the endpoint does. My first attempt at this appended "past tense throughout this paragraph" and left the following sentence in the present tense, which is annotating a problem instead of fixing it. Rewritten so the tense carries the meaning without a note telling the reader to read it differently. Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X * docs(server): the SQLite half of the claim needed the same narrowing (BUG-2782) Codex round 8, one P3 and it is the mirror image of round 6's. I qualified "the handler answers 500" on the Postgres side and left the symmetric sentence — "the same request is a clean 404 there" — universal on the SQLite side, in the same paragraph. Not every request reaches a store resolution on either backend; a gate that answers first keeps its own status, and my own GET-only sweep recorded 102 x 404 alongside 5 x 403, 2 x 200, 1 x 401 and 1 x 503 on SQLite. Fixing one direction of a symmetric claim and leaving the other is a shape I have hit before and evidently do not catch by intention. The paragraph now says the divergence is in what happens once a value REACHES the store, which is the true and symmetric statement, with the distribution pasted. Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X |
||
|
xarmian
|
773222368c |
fix(store): serialize document renames and stop reading the pool inside transactions (BUG-2778) (#1208)
Two deadlocks, one in the database and one in the application. THE DATABASE ONE, which is what BUG-2778 was filed about. A document rename takes row locks in two stages inside one transaction: updateLinksInTx writes every OTHER document whose content links the old title, then the final UPDATE writes THIS document. Two concurrent renames of documents that link to each other therefore take the same two locks in opposite orders, and Postgres aborts one with SQLSTATE 40P01 — a 500 on an ordinary rename. A throwaway probe against the unfixed code deadlocked on 12 of 12 rounds; this is deterministic, not theoretical. The fix serializes renames per workspace with a dedicated advisory key (`pad:document-rename:<ws>`), taken before any row lock and whenever a title is supplied. No-op on SQLite, whose single writer cannot produce the cycle. `SET LOCAL lock_timeout = '5s'` bounds the wait, because a transaction that waits with a pool connection already in hand converts contention into pool exhaustion; the handler maps 55P03 and 40P01 to a retryable 503 rather than a generic 500. WHY NOT `ORDER BY id`, which is what I proposed when I FILED this from reading rather than from a repro: each transaction's cascade set is a single row, and the cycle is cascade-then-self, so ordering the cascade leaves it exactly as reachable. Run as a mutation, that fix fails the regression test. Reproducing the bug is what refuted my own diagnosis. THE APPLICATION ONE, found in review and larger than the filed bug. Seven production paths issued a read through the connection POOL from inside an open transaction. Under a saturated pool the second connection never arrives, so the transaction cannot finish and never releases what it holds — no SQLSTATE names this and no lock timeout breaks it. All seven now take their executor: the document and item slug scans, both version checks, the done-field lookup on the item update and move paths, the open-children guard's collection read, and the OAuth startup backfill's workspace lookup. Three of those were found only after asking for the true population rather than for a sample; two were INDIRECT (through GetCollection), which a grep for `s.db` inside transaction bodies cannot see. The instrument is a one-connection pool, which makes the hazard deterministic instead of load-dependent. ALSO FIXED, adjacent and found by the same reviews: - The rename decided from a PRE-LOCK snapshot: the lock made the cascade safe against another rename and then handed it a stale OLD TITLE. It now re-reads under the lock and decides from that row. - A concurrent soft-delete could commit mid-rename, leaving the cascade's rewrites behind while the caller was told not-found. The final UPDATE now carries `deleted_at IS NULL` with a checked row count, so the rename and its cascade land together or not at all. - Class sweep (CONVE-18) of the same unordered-scan-then-per-row-update shape: the attachment remap (items and comments) and the outbox user-ref scrub are now ordered. Scoped claim — it orders those per-row updates, not every lock in those transactions — and NOT reproduced, unlike the rename. 18 mutations aimed, 15 die. The three survivors are written down where they live: one is an equivalent mutant (the cascade is the first row-locking stage, so "lock before the cascade" and "lock at the top of the transaction" are the same order), one is a guard covering a window no seam can currently schedule, and one is a class-sweep fix with no repro. |
||
|
xarmian
|
effea01666 |
fix(server): diff the activity change list against the store's pre-image (BUG-2776) (#1206)
handleUpdateItem built the activity's human-readable change list by diffing the item it read at the TOP of the request — before the permission checks, before the store's locks — against the row the store wrote. Anything a concurrent writer committed inside that window appeared in the difference and was stamped, with this request's actor and agent name, onto whoever sent the PATCH. Agent A sets status, agent B sets priority, and B's entry reads "status: open → done; priority: low → high" over B's name. Unlike BUG-2770, where a change went MISSING, here the timeline gains a confident false statement about who did what — and BUG-2770's debounce then merges that statement forward into the coalesced row, so it outlives the request that invented it. The fix is a carrier, not new machinery. The store has re-read the row under its own locks since TASK-2533, unconditionally, and diffs THAT snapshot for its status and assignment signals; the handler simply had no way to reach it. models.Item.PreUpdate hands the same snapshot back on the returned item — `json:"-"`, transient, populated only by the update path, following the LastMutation precedent. The title, role and assignment arms move onto it too, which makes the whole list committed-vs-committed: what this transaction wrote over, versus what it wrote. A missing pre-image DROPS the change list rather than falling back to the handler's stale read. The fallback is the defect wearing a warning: an entry that says nothing is recoverable, one that names the wrong author is not. The activity row is still written; a slog.Warn names the invariant. Also fixed, same lines: the title arm only recorded a rename when the field diff had produced nothing, so a PATCH that renamed AND edited a field silently dropped the rename. Two test seams are added (Server.afterItemPreRead, Store.afterItemPreLockRead), both nil in production and both documenting their reentrancy requirement. The second exists because of a mutation that SURVIVED the first matrix: with the rival's write landing before the store call, a pre-image taken from the store's pre-lock read is indistinguishable from one taken under the lock — the instrument could not see the difference the fix is about. That mutation now dies. Seven mutations aimed, six die; the seventh (aliasing the pre-image instead of copying it) survives by design and says so in the code. The Postgres leg earned its keep again: two store assertions compared `fields` blobs byte-wise, which passes on SQLite (TEXT, exact bytes) and fails on Postgres (JSONB, re-serialised) while proving nothing either way about which snapshot the blob came from. They compare by value now. |
||
|
xarmian
|
f465d4c51e |
fix(server): a malformed before_id is a 400, not a 500 (BUG-2774) (#1205)
* fix(server): a malformed before_id is a 400, not a 500 (BUG-2774) before_id went from the query string into the cursor predicate unchecked. Postgres refuses a text parameter that is not valid UTF-8 or that carries a NUL (SQLSTATE 22021/22P05), so the store call errored and the handler answered 500 — the server announcing its own failure for a client's bad input, and SQLSTATE noise in the logs for an input problem. SQLite accepts the same bytes and matches nothing, so the identical request was a 200 there; the failure mode diverged by dialect from one line of unvalidated input. validCursorID rejects exactly what the DATABASE rejects rather than what an id should look like, and deliberately carries NO length or format bound: the structured kinds' ids come from the item's own fields blob, nothing validates them on write, and an imported artifact may carry any string — so a cap could only ever fire on a legitimate cursor, while the cost of an over-long one is a single indexed comparison against a parameter the URL length limit already bounds. Its comment says so, because the obvious review question is why there isn't one. Tests: three malformed shapes reachable from a plain URL (%FF, an embedded NUL, invalid bytes mid-string) plus three controls — a UUID, a structured note id, and a long non-ASCII id — without which "reject every before_id" would pass and paging would be dead rather than honest. A store-level test pins the PREMISE where it is real: on Postgres the query itself fails, on SQLite it succeeds and returns nothing, and both halves are asserted in the one place that sees both, each with a message saying what it means if the backend's behaviour has moved. Verified on Postgres 17 (private container, not the shared port). The handler legs fail with the validation removed. Refs: BUG-2774 Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X * fix(server): never emit a cursor the handler would refuse (codex round 1) The validation had a second direction I had not closed. A structured entry's id comes from the item's fields blob, which nothing validates on write, so a JSON \u0000 escape arrives as a real NUL on SQLite — Postgres's jsonb refuses it at the door, which is why this is a one-backend hazard. That id became the entry's id, the entry's id became next_before_id, and the client sending it back got a 400 from the validation this unit just added: the server handing out a cursor it then refuses, wedging paging on that item. Such an id now takes the positional fallback that empty and duplicate ids already take — the id has to be usable as a CURSOR, not merely unique. One condition on an existing branch rather than new machinery. Its test uses two notes, one NUL-bearing and one clean, so it distinguishes 'replaced the unusable id' from 'stopped using raw ids at all'. Verified against the unmutated branch: the NUL id is emitted verbatim and the leg fails. Refs: BUG-2774 Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X * test: assert the envelope, the emitted cursor, and split the dialect premise (codex round 3) Three test weaknesses, all mine. Status-only assertions: a bare 400 or the wrong code would have passed, and clients branch on the code rather than the number. The legs now decode the envelope and require invalid_cursor with a non-empty message. The emitted-cursor test claimed to cover next_before_id and did not: at limit=50 the fixture emits no cursor at all, so it only checked entry ids. A third note and a truncating limit make the NUL-bearing one the LAST kept entry, which is what puts it in the cursor — and the test now asserts the cursor is valid AND that paging with it returns 200. The mutant (emitted ids unchecked) still dies, now on the thing the test is named for. The dialect test is CHARACTERIZATION and now says so: it passes with or without this change, because it describes the backends rather than the handler, and that is its job — it is the premise the 400 rests on. Its 'both halves in one place' claim was also false in an unconfigured run, where NewPostgres skipped the whole test and the SQLite half never ran either. Split into subtests: the SQLite half runs everywhere, the Postgres half skips alone. Refs: BUG-2774 Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X * fix(server): a one-sided cursor is a 400, not a silent no-op (codex round 4) before_id alone could never match anything: the id is the tie-break AT the cursor instant, and before defaults to now+1m, which no row shares. So it was accepted, ignored, and the caller paged from the beginning believing they had a cursor — the accepted-and-does-nothing shape, which is the worst answer of the three available. The other direction stays supported deliberately, and has a control leg saying so: before alone is the external-client case the "g" sentinel exists for, and rejecting the pair symmetrically would break something the handler goes out of its way to serve. In scope for this unit rather than a separate filing because it is the same parameter and the same class of answer — and because BUG-2765's PR body documented "both fields or neither" as the contract, which until now nothing enforced. Codex's other round-4 finding — a structured entry id can collide with a comment/activity/version id, because the dedupe map holds only structured ids — is real, pre-existing, and a cross-source id decision rather than a validation. Filed as BUG-2783. Refs: BUG-2774, BUG-2783 Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X * docs: the cursor contract is asymmetric, and now says so (codex round 6) BUG-2765's prose said "both fields, or neither" in the handler doc comment and the TS client. This unit then enforced it in one direction only — deliberately, because `before` alone is the external-client shape the id sentinel exists to serve, while `before_id` alone matches nothing and silently pages from the beginning. The slogan was mine and the asymmetry is mine; leaving both in place would have left a reader to discover the difference from a 400. Both sites now state which one-sided form is accepted, which is refused, and why each. Refs: BUG-2774 Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X |
||
|
xarmian
|
11f67b0a98 |
fix: timeline can answer has_more=true with zero entries, and the client cannot page past it (BUG-2765) (#1202)
* fix(server): timeline returns the cursor for its next page (BUG-2765) The timeline over-fetches 3x per source and drops rows that cannot render (read/searched actions, empty-metadata updates, activities a version or a comment already stands for, collapsed autosave bursts), so a page can carry fewer entries than the rows it consumed — or none, while has_more is true. The client derived its cursor from the last RENDERED entry, which fails in two ways. With no entries it cannot form a cursor at all, so the first page is a dead end. With a fully-dropped window LATER in the history it re-sends the same cursor forever: nothing is appended, the oldest entry does not move, and paging is wedged at that position permanently. The filing named the first; the second is the one that bites an ordinary item, since a run of read activities anywhere in its history is enough. Both are the same root cause — the response says WHETHER to continue and not WHERE — so the server now returns next_before / next_before_id whenever has_more is true: - page truncated: the last entry KEPT, because the ones cut off must be re-fetched. Unchanged from what the client derived. - window exhausted: the NEWEST tail among the sources that filled their window. A short source has nothing older to come back for and must not drag the cursor forward; resuming at the oldest tail instead would step over a newer source's unexamined rows, and repeats are absorbed by the client's dedup while gaps are not recoverable. Progress is guaranteed because every candidate is a row this page fetched and the store's cursor predicate is strict. Tests: an all-dropped window returns a cursor that reaches the history behind it; paging across a dropped MIDDLE stretch terminates and yields each renderable entry exactly once; and a control leg pins that an untruncated page still resumes at its last rendered entry — without it, "always resume from the oldest row touched" passes while silently skipping what truncation cut. The two-full-source selection rule is pinned as a unit test, because a full source whose rows RENDER puts 3x limit entries on the page and takes the truncation branch instead, so the handler cannot cheaply reach it. Mutation matrix, each independently detected: cursor from rendered entries; oldest tail instead of newest; the full-window flag ignored; no cursor at all. Client half follows in the next commit. Refs: BUG-2765 Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X * fix(web): page the timeline with the server's cursor (BUG-2765) Client half. The component derived its next-page cursor from the last RENDERED entry, so a page the server had emptied by dropping rows either gave it nothing to page from (the first page, where loadMore returned early on entries.length === 0) or gave it the SAME cursor it already held (a later page, where nothing was appended and the oldest entry did not move). The second case is a permanent wedge with a live button and a running spinner. It now pages with next_before / next_before_id when the server sends them, and falls back to the last entry otherwise — which is exactly the old behaviour, including its wedge, and is there only for a server that predates the field. One press walks at most MAX_EMPTY_HOPS pages while every row keeps dropping. That bound is UX, not correctness, and its comment says so: a single hop is already correct now that the cursor advances; the loop exists so a user crossing a long run of read activity sees entries appear rather than a spinner and nothing, and it is small so a pathological item cannot turn one click into an unbounded request fan. Tests assert the cursors the component ASKS FOR, not only what it displays — a component that shows the right thing by re-fetching page one forever is the bug. Mutation matrix: ignoring the server cursor fails three of four legs (the fallback leg survives, correctly, since that is the path it pins); a single hop fails the advance leg; an unbounded hop count fails the bound leg. vitest 109 files / 1862 tests pass; vite build clean; svelte-check 0 errors (6 warnings, all pre-existing and in other files). Refs: BUG-2765 Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X * fix: cursor must clear BOTH bounds, and a later page must merge not append (codex round 1) Two findings, both real, both consequences of the cursor itself. P1 — truncation and an exhausted window are INDEPENDENT bounds, and the second is not implied by the first. A source whose rows all drop contributes nothing to the page, so the truncation cursor can sit older than that source's tail, and every unexamined row between the two falls in a gap neither page fetches. The cursor is now the NEWEST candidate across both reasons. Its regression puts one renderable activity in exactly that gap: two comments forcing truncation at limit=1, three read rows filling the activity window above them, and the row at risk in between. Run against the previous commit it comes back 0 times — the row is not late, it is gone. P2 — a later page can legitimately carry entries NEWER than the oldest one already shown, because the cursor deliberately re-covers ground when one source's window ran out before another's. Concatenating printed those below older entries. The client merges by (created_at desc, id desc) now, comparing INSTANTS rather than strings: precision is not uniform — the store writes whole seconds but a structured note can carry a sub-second timestamp — and lexicographically "…:05.123Z" sorts before "…:05Z". Mutation checks: truncation ignoring the exhausted candidate fails the new gap test; concatenating instead of merging fails the new order test and nothing else. internal/server suite green; timeline vitest 9 files / 83 tests green. Refs: BUG-2765 Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X * fix(web): stop the hop loop when the cursor does not advance (codex round 2) Against a server that predates next_before, the fallback re-derives the same last entry on every hop, so a single Load More click fired five identical requests where the pre-fix component fired one. The client cannot give an old server a cursor it does not have — that wedge is the old behaviour and stays — but amplifying it was new, and mine. A cursor that did not move cannot make progress, so the loop stops on it. Its test asserts the request COUNT, which is the only thing that distinguishes this from the behaviour it replaces: the entries rendered are identical either way. Refs: BUG-2765 Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X * fix(web): order the SSE-refresh merge too, and share one comparator (codex round 3) The refresh prepended its genuinely-new entries on the assumption that a fresh first page is always newer than everything on screen. Normally it is — but a structured note or decision carries a hand-written created_at and can arrive backdated, and the assumption was never stated, only relied on. Both merge points now go through one byNewestFirst, which is the server's own ordering and compares instants rather than strings. Two of codex's three round-3 findings are not folded in: - The refresh not adopting the response's has_more / cursor is DECLINED, not missed. The refresh re-fetches the NEWEST window, which says nothing about where the reader's paging frontier is; the stored cursor stays valid because the refresh consumes no older rows, and adopting the fresh page's has_more after the reader has paged deeper would point the cursor back at history they already hold. One wasted request, absorbed by the no-advance stop, in exchange for a correctness claim I cannot make. - firstPageIds treating any entry missing from a refreshed first page as deleted is real, pre-existing, and a semantics call about what counts as a deletion rather than a patch: filed as BUG-2773. Refs: BUG-2765, BUG-2773 Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X * test(server): derive the expected entry set instead of naming two ids (codex round 4) The test claimed every renderable entry came back exactly once and checked the two ids it had seeded. The item's own `created` activity could have vanished or repeated underneath that claim — the same partial-verification shape as asserting one direction and writing the symmetric conclusion. The expectation now comes from the store: every activity on the item minus the kinds buildTimeline drops unconditionally, compared in both directions, so a fixture that grows a row cannot fall outside what the test says it covers. Still fails with the cursor withheld. Refs: BUG-2765 Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X * docs: state the timeline cursor contract where consumers read it (codex round 5) The TypeScript type documented next_before/next_before_id, but the handler's own doc comment — what a REST consumer reads — still described before + limit only, and the API client method said nothing. A consumer following either could still derive a cursor from its last visible entry, which is precisely the invalid contract this change exists to replace. Both now state the pair, that it must be forwarded rather than re-derived, why (dropped rows make the last entry a different position, sometimes no position), and that the id is the tie-break among entries sharing a second — send both or neither. Refs: BUG-2765 Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X * fix(web): clear paging state when the timeline reloads (codex round 6) The previous item's entries stay on screen while the new one's page 1 is in flight — the list renders on `!loading || entries.length > 0` — so Load More is clickable during a switch, and its cursor was the OLD item's position aimed at the NEW item. Pre-existing in shape (the pre-fix code derived the same stale position from the same stale entries), but now it is one line to close, and the dedicated cursor variable is mine. loadTimeline replaces entries with page 1 when it resolves, so clearing the cursor and has_more before the await throws away no state that would have survived; it just stops offering paging for a position that is no longer known. Test holds the switch's fetch unresolved and asserts no request goes out in that window, with the button's absence as the observable. Fails with the two lines removed. Refs: BUG-2765 Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X |
||
|
xarmian
|
e747a1610c |
feat(session): registry keyed on the harness session, carrying the agent name; pad session list / prune (TASK-2767) (#1200)
## Summary TASK-2767 (IDEA-2750 part 2, with part 3 riding along — the keying fix and the reaping are one mechanism). The local session registry (`~/.pad/sessions`) was keyed on the pid of the `pad session register` subprocess, which is dead before anyone reads the file. One session left a new file per call and its own pid appeared in none of them; the only live identifier was the harness pid a reader could parse out of the socket path's basename. In practice nothing wrote it (zero callers in `plugin/`, `skills/`, or hooks) and nothing read it. Now: - **One record per session, keyed on the harness session pid** — `$PAD_SESSION_PID` (harness-agnostic override), else `$CLAUDE_PID` (verified present in both the tool shell and a live plugin monitor's `/proc/<pid>/environ`), else the calling process. A set-but-invalid value is an error, not a silent fall-through. - **The record carries the agent name** the session's writes are attributed to (`ResolveAgentName`: `.pad.toml agent_name` → `$PAD_AGENT` → detected runtime; `--agent` overrides, `--agent ""` is anonymous), the harness session id, and the messaging socket's identity (inode/device/mtime — the same binding the arm-state file uses). - **One owner-identity type, one verdict.** `internal/cli/session_owner.go`: `SessionOwner` + tri-state `OwnerLiveness` (`alive` / `dead` / `unknown`). `armStateOwnerAlive` is now `OwnerLiveness(...) == alive` with its file contract preserved (socket identity else mtime; headless pid + start token; fail closed). The registry pruner takes the opposite posture on `unknown`: on Windows `pidAlive` reports dead for every pid, and a reaper built on that would delete every live session's record. - **Verbs:** `pad session register [--agent]` (writes/refreshes; prunes dead records), `pad session list [--agent] [--cwd] [--all] [--format json]` (liveness per row, newest first; dead hidden unless `--all`), `pad session prune [--older-than DUR]` (dead always; unknown only under an explicit bound; alive never). Nothing on MCP — host-local filesystem state. - **Who registers:** `plugin/scripts/pad-monitor.sh` runs `pad session register` on start, BEFORE the consent gate — presence is a fact, consent is a grant, and the record is local/0600/never on the wire. - **Legacy v1 files** list as `legacy` rows: owner = socket-basename pid (else registrar pid), liveness by pid only (v1 recorded no socket identity, and the socket-without-identity rule would have judged every legacy record dead while its session ran). A legacy row can say a session exists, never who it is. Lead rulings on the four open decisions, all as built: `agent`/`--agent` vocabulary; no server-presence merge in `list`; register from the monitor script before the gate; wire follow-on (agent name on the stream) filed separately as IDEA-2750 part 2b. One ordering change from the plan's section A: pid precedence is `PAD_SESSION_PID` > `CLAUDE_PID` > self (explicit override beats detection, mirroring `PAD_AGENT` over runtime detection); the plan listed `CLAUDE_PID` first. ## Behaviour changes for existing users of `~/.pad/sessions` / `pad session register` - Registry files are keyed on the **harness session pid** (`PAD_SESSION_PID` → `CLAUDE_PID` → self), not the `pad` command's pid; repeated registrations overwrite one record instead of accumulating. - `pad session register` records the agent name, harness session id and socket identity; stores the **real path** of the cwd; prints a different text line and a different JSON shape (the full `SessionRecord`); and **rejects** an invalid `PAD_SESSION_PID` / `CLAUDE_PID` instead of silently keying on itself. - Existing v1 files are read as `legacy` rows (owner = socket-basename pid, no agent name) and dead ones are pruned by the next register. - The plugin monitor now registers (and prunes) on every start, before the consent gate. - `armStateOwnerAlive` now delegates to the shared `OwnerLiveness`; the consent gate's observable behaviour is unchanged on every platform and key type (codex round 4 traced every caller; matrix M29 pins the socket-keyed mapping). https://claude.ai/code/session_016zc6oxBvpax6Z3iQMsAJno |
||
|
xarmian
|
3a63b52334 |
fix(store): activity debounce must not merge across writers (BUG-2763) (#1201)
* fix(store): activity debounce must not merge across writers (BUG-2763)
CreateActivityDebounced coalesced two "updated" writes whenever they shared
a document, an action, a user account and the cooldown window. The account is
not the writer: an agent authenticates as the human it works for, so a human's
write and an agent's write — and two different agents' writes — matched each
other.
The surviving row is wrong in both orderings, because the merge UPDATE writes
only metadata and created_at (so the row keeps the FIRST write's actor) while
mergeActivityMeta overlays the incoming `agent` key last-writer-wins:
human then agent — row stays actor='user' and the agent's name is ignored,
so the agent's edit renders as the person's.
agent then human — row stays actor='agent' and no incoming key overlays the
stale name, so the person's edit renders under the agent's name.
Both are visible since TASK-2760 named the actor: TimelineActivityCard reads
the stamped name only when actor == 'agent'.
Refuse the merge unless the candidate row's actor AND its metadata agent name
both equal the incoming write's; on refusal fall through to CreateActivity,
the same fall-through the comment-link refusal already uses.
The check is in Go rather than in the UPDATE's predicate, where the
comment-link refusal lives (TASK-2760 codex round 6), because nothing can
invalidate it between the read and the write: actor is written once at INSERT
and no statement here updates it, and the metadata agent key can only be
rewritten by a merge that passed this same check, which leaves the name equal
to what it already was. A predicate in the UPDATE would guard a change that
cannot happen.
Tests drive the real PATCH route with and without X-Pad-Agent and read the
timeline endpoint (CONVE-19: the binding, not the store call), covering both
orderings and two agents on one account, each with a control leg asserting
that a same-writer run still coalesces — without which "never debounce" would
pass. Store-level legs cover the same matrix directly.
Refs: BUG-2763
Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X
* test(store): make the debounce identity matrix order-independent and kill the actor mutant
The first matrix run left one mutant alive: removing the actor comparison and
keeping only the agent-name comparison passed every leg. That is the pair-only-
dies-together shape, so it needed an answer rather than a note.
The reason it survived is real and worth stating: through the only production
caller, actor and agent name both derive from the X-Pad-Agent header, so they
cannot disagree, and comparing names alone catches every header-driven case.
The store's API admits the decoupled state, and the new leg is what it means —
a caller declaring an agent write without stamping a name is still not the
human. That leg kills the mutant at the layer the guard lives in; the handler
suite legitimately cannot, and the comment says so.
Also stopped asserting row order. Both writes land in the same second and
ListDocumentActivity orders by created_at alone, so ties come back driver-
dependent — stable on SQLite, unordered on Postgres, where this suite also
runs. Rows are matched by the change each write recorded instead.
Refs: BUG-2763
Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X
* fix(store): pick the debounce candidate by writer, not by recency alone (BUG-2763)
Codex round 1 on this branch: the identity guard fixed attribution and broke
the coalescing it guards. created_at is whole-second, so with the candidate
query still taking the newest row overall, a writer returning inside the window
lands on the OTHER writer's row, is refused on identity, and starts a third row
— exactly the per-save spam the debounce exists to prevent.
Select the recent rows for THIS writer instead: actor as a SQL predicate, and
the agent name matched in Go over the candidates (it lives in the metadata JSON
and this store targets two dialects whose JSON accessors differ). The first
candidate whose name matches is the row to extend; none means a new run.
maxDebounceCandidates=10 bounds the scan, not the semantics — past it the loop
fails to find the writer's own row and starts a new one, costing an extra row
and never a wrong attribution. Its comment carries that reasoning.
The regression drives agent → human → agent with real 1.1s gaps rather than
three writes in one second: the same-second form is ordered by whatever the
driver returns, so it would fail on one backend and pass on the other for
reasons unrelated to the fix. Strictly increasing timestamps make the wrong
candidate deterministically the newest row. Run against the pre-fix selection
it produces 3 rows.
Refs: BUG-2763
Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X
* docs(store): correct why the agent-name match is in Go, and sharpen the scan bound
Two comment corrections, no behaviour change.
The first claimed the name is matched in Go because the two dialects' JSON
accessors differ. They do, but this store HAS a portable accessor
(s.dialect.JSONExtractText), so that was not the reason and reads as though
nobody checked. The real reason is agreement with the renderer:
models.AgentNameFromMetadata is the twin of the accessor the timeline uses, and
a SQL predicate would disagree with it wherever a stored value is odd — a
non-string agent reads as absent in Go and as its text form in SQL, and
malformed metadata is one skipped row in Go versus a failed query on SQLite.
The second described maxDebounceCandidates as covering "the writers that
plausibly interleave", which overstates what competes: the query already
filters to one document, one account and one actor kind, so only rows carrying
a DIFFERENT agent name can crowd the writer's own out. For a human write that
is normally none, and only possible at all for rows written before this fix.
Codex round 2 read the bound as incomplete coalescing; declined on those
grounds, with the reasoning recorded where the number lives.
Refs: BUG-2763
Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X
* docs(store): three comment corrections from codex round 4
No behaviour change; each of these was a claim in a comment that did not
survive being checked.
- "mergeActivityMeta overlays agent last-writer-wins" is only half true: it
overlays keys the INCOMING metadata carries, so a write with no agent key
leaves the stale name standing. That asymmetry is precisely the agent-then-
human direction of this bug, so the comment now states both directions.
- The SQLite half of the dialect-divergence note said json_extract yields a
non-string value's "text form". It yields a native number or boolean, which
no text comparison matches. Same conclusion, correct mechanism.
- The comment-link refusal's residual window was described as ordering ("an
update that completes before the comment links its row at all"). Visibility
is the better frame: the comment row is written in its own transaction, so on
Postgres a link committing while the UPDATE's snapshot is already open is
missed exactly as an unwritten one is. Still BUG-2716's to close.
Codex's fourth finding — concurrent same-writer calls read-modify-write the
same metadata blob and the second UPDATE silently drops the first's change
entry — is real, pre-existing, and orthogonal to this fix. Filed as BUG-2770
rather than folded in.
Refs: BUG-2763, BUG-2770
Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X
* test(server): run the debounce attribution test on a real account (codex round 5)
The test's own comment said both writes ride one account. They rode none:
doAttributionRequest sends no session and no bearer token, so currentUserID was
empty and every row landed with a NULL user_id — which coalesces through the
predicate's IS NULL branch, a different path from the one the bug is about. A
regression in the authenticated handler-to-store user-id flow would have passed
here, and the claim in the comment was simply false.
Bootstrap an owner, create the workspace and item as that owner, and send every
PATCH with its bearer token, varying only X-Pad-Agent. The account is now also
ASSERTED rather than described: each entry must carry the same non-empty
user_id, so the premise fails loudly instead of quietly becoming untrue.
Mutation matrix re-run against the reworked test: dropping the agent-name match
fails two-agents-on-one-account, dropping both fails all three identity legs.
Dropping the actor predicate alone is still store-only, for the reason already
recorded there — actor and agent name both derive from X-Pad-Agent, so no
handler-path input can separate them.
Refs: BUG-2763
Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X
* docs+test: sweep the prose this change falsified, and de-vacuum one leg (codex round 7)
CONVE-23: two statements elsewhere were true when written and are not any more.
handlers_items.go said a concurrent same-user update can debounce-merge into
the not-yet-linked activity and overlay the agent stamp the comment will carry.
Since the identity guard it can only merge as the SAME writer, so the stamp it
overlays is the one the comment already carries; what survives is the
created_at bump, still BUG-2716's window.
comments_agent_name_test.go's rationale described the cross-agent
re-attribution it was built for. The same change that made that prose stale
also weakened the test: its second assertion (second != first) was left passing
on the identity guard alone. MEASURED, not assumed — with the comment-link
predicate deleted and the two agent names restored, that assertion still passed
and only the final leg caught the deletion. Every write in it now declares one
name, which puts the refusal back as the only mechanism that can split those
rows; the cross-agent shape moved to the BUG-2763 matrix.
Sweep boundary: grepped Go under internal/ and cmd/ for debounce/coalesce
vocabulary, and .go/.md/.svelte/.ts repo-wide for overlay / re-attribute /
last-writer / agent-stamp phrasing. Two other sites (reports.go,
status_transitions_backfill.go) describe coalescing as undercounting rapid
hops, which stays true — there is simply less coalescing. Web-side hits are all
client SSE-refresh debouncing, unrelated.
Codex's remaining round-7 finding — the remote /mcp dispatcher never sets
X-Pad-Agent, so every write over that transport records as the human and no
identity fix in the store can see through it — is real, pre-existing, and needs
a design call about where the name comes from. Filed as BUG-2772.
Refs: BUG-2763, BUG-2772
Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X
|
||
|
xarmian
|
d8b098fbed |
fix(watchevents): bound the resume settle window by the request context (BUG-2751) (#1197)
* fix(watchevents): bound the resume settle window by the request (BUG-2751) GET /api/v1/events/stream takes a global AND a per-user admission slot before subscribing (BUG-2726) and releases them by defer when the handler returns. The resume path could WAIT inside that window: resumeOutrunsLocalView does a Redis GET, waits 250ms for propagation, then does a second GET -- and its select waited on b.ctx, the BUS's lifetime, never the request's. A resuming client that disconnected mid-window held both slots for the remainder of it plus two round trips. The connection was gone; the capacity was not. The context is threaded through SubscribeAndReplaySince -> resumeOutrunsLocalView -> sharedCounter and MERGED with the bus context rather than replacing it. Swapping to the caller's alone would trade one leak for another: b.ctx is what lets Close cut a wait short, so dropping it leaves shutdown blocked behind a client that is still perfectly connected. internal/events lost exactly that half in its own first draft. Both endings are reasons to stop, and each has its own test that fails against the other one's implementation. ENDING THE WAIT EARLY IS HALF A FIX (codex round 1). resumeOutrunsLocalView answers false on cancellation, which reads as an ordinary converged resume, so the rest of the call went on to register a subscriber and build a replay slice for a connection that was unwinding. A cancelled caller is now declined outright, returning the same shape as the closed-bus branch -- a closed channel, never nil, because the handler treats nil as "fall back to plain Subscribe" and would have re-registered the very caller being declined. MemoryBus was CHECKED rather than assumed clear, which is the scope note this bug carries and also how it was found (BUG-2749's filing asked for this package to be checked). It has no bounded wait and no I/O, so there is nothing for a cancelled caller to stop paying for -- but it declines one too, because the two implementations must not disagree about whether a departed client ends up registered. That divergence is invisible on a single-process deployment right up until it is a leak on a clustered one. Six tests, each failing against the specific thing it names: bus-ctx-only, caller-ctx-only, an implementation that never settles, no decline on RedisBus, no decline on MemoryBus, and -- the binding one, in internal/server -- a handler that passes context.Background(). The last is what CONVE-19 asks for: the bus tests vouch for the bus honouring cancellation and say nothing about whether the handler ever hands it one. The mid-settle cancellation lands through a new positional seam rather than a sleep. A sleep-timed cancellation that arrives late does not fail safe here; it silently measures the already-cancelled path instead. Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X * fix(watchevents): decline a cancelled resume before it touches Redis (r2) Codex round 2 verified round 1's fixes and came back clean on the two dimensions that could have made this change dangerous: every caller handles the declined closed channel correctly (the SSE handler falls back to plain Subscribe only on NIL, and the CLI treats EOF as reconnect), and there is no path where a LIVE caller's context is cancelled -- the route has no timeout middleware, and bus shutdown stays separately bounded through the merged context. Its P3 was a real one: the cancellation check sat AFTER resumeOutrunsLocalView, so an already-cancelled caller still entered it and made the first Redis GET. That fails on the dead context and logs "could not read the sequence counter to validate a resume; answering from local knowledge only" at WARN -- a line that means "Redis is unhealthy" to whoever reads it. Ordinary disconnect churn would have fired it on every client that hung up a moment before its resume landed. Moved ahead of the settle path. A METRIC WAS DECLINED, with the reasoning at the code. The finding asked for a cancellation counter so operators could distinguish disconnect churn from no resume activity. A client hanging up during its own resume is ORDINARY on a mobile network, so that counter would be a number nobody can act on, sitting next to pad_watchevents_resume_gaps_total where it would read as a fault. The condition an operator does act on -- capacity held by connections that no longer exist -- is already visible in the admission counts, and this change is what keeps those honest. Debug log instead. THE FIRST INSTRUMENT FOR THIS MEASURED NOTHING. I asserted "no Redis reads" as a proxy for "no misleading log", and go-redis short-circuits a cancelled context before it touches the wire -- so no GET reaches miniredis whether or not the early decline exists, and removing it survived. The assertion is on the LOG now, through a capture handler, because the log is the only thing that distinguishes the two. Fails with the exact WARN quoted back. Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X * fix(watchevents): our own cancellation is not a Redis fault (codex r3) Round 3 approved with comments and found the twin of round 2's finding. The entry-side decline catches a caller that was ALREADY gone; a caller can also leave while the first counter GET is in flight, and the error that comes back is context.Canceled -- indistinguishable, at the sequence-counter WARN, from Redis being unreachable. On a stream where clients hang up mid-resume that line would manufacture exactly the alarm an operator would chase. Suppressed to Debug when ctx is already dead, with a test that intercepts the GET from inside miniredis's command hook -- cancelling there and failing the command is what makes it the in-flight case rather than the entry case, without any timing. Fails with the exact WARN quoted back. Also cleared by that round, recorded because each was a real question rather than a rubber stamp: no self-sustaining state; slog.SetDefault is restored and the capturing test is non-parallel so it cannot bleed into package t.Parallel() tests; and whicheverEndsFirst is a correct second package-local copy of internal/events.mergeCancellation rather than a candidate for extraction -- a four-line shared utility would be coupling two packages for nothing. Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X * fix(watchevents): classify the caller-gone case on the error, not the context Codex round 4 BLOCKED, correctly. Round 3's suppression asked ctx.Err() rather than what the error actually was, so a GENUINE Redis failure arriving while the context happened to be dead would be downgraded to Debug and disappear — the signal an operator most needs, hidden by the change meant to reduce noise. My own new test demonstrated it: it returned a real server error and asserted no WARN. Now a predicate on the error alone, callerIsGone(err), and extracting it is the substance rather than tidiness. The two integration legs I had written CANNOT tell the two forms apart: both agree on every case a live client can be made to produce on demand — a cancelled context yields a context error, a live one yields a server error — so that pair passed against the blocked implementation too. Verified by mutation rather than assumed; reverting to ctx.Err() left them green. They disagree on exactly one case, a Redis failure whose error arrives while the context is already dead, and staging that through a client is a race by construction because go-redis decides by timing which error it returns. As a predicate there is no timing, and that case is a table row. Kept all three: the predicate table for the classification, and the two integration legs for the wiring — a cancelled read stays quiet, a genuine failure still warns. The second is the control without which "no WARN" is satisfied by a bus that has stopped reporting Redis trouble at all. Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X |
||
|
xarmian
|
99ffad1bca |
feat(server): timeline comment rows carry the agent name (TASK-2760) (#1196)
* feat(server): carry the agent name onto comment rows in the timeline (TASK-2760) An agent's comment rendered under the human's name: the name is stamped only on the linked 'commented' activity, which the timeline suppresses because the comment card stands in for it. The comment list queries now LEFT JOIN that activity and surface the name as Comment.AgentName (top-level and nested replies, on the timeline and the comments endpoint alike, through one scan helper), mirrored onto comment-kind TimelineEntry.agent_name to match the actor_name idiom. The web comment card renders it verbatim in an isolated <bdi>, separate from the human author. Store join rather than a handler-side match: the two lists are paginated independently, so a handler join misses at page edges and reads as intermittently-correct attribution. Metadata is parsed in Go, not SQL, to keep the query free of a SQLite/Postgres dialect fork. * test(store): make the activity-window premise strict, not a same-second coin flip (TASK-2760) * fix(server): replies log + link their commented activity so the agent name reaches them (TASK-2760, codex r1) The dedicated reply route wrote no 'commented' activity, and the activity is the only row that carries the writing agent's name — so a reply through the web UI rendered under a generic chip no matter what the client sent. Also rewrites the README + SKILL.md claim that comments never show the name, moves the reply test onto the real route, and asserts order/limit under the join. * fix(store): exclude comment-linked activities in the timeline's activity query (TASK-2760, codex r2) buildTimeline suppressed a comment's linked activity only when that comment was on the same page; the two sources are paginated separately, so an activity could slip through as a standalone 'commented' card. The query now excludes linked rows via NOT EXISTS on idx_comments_activity (both dialects), exact regardless of either window, and the page-local guard is removed rather than kept as a dead one that reads as load-bearing. * fix(store): item-scope the comment/activity link and freeze comment-linked activities against debounce merges (TASK-2760, codex r3) The join keyed on activity id alone while nothing in the schema ties a comment's activity to its item — scope both the LEFT JOIN and the NOT EXISTS to the item. And CreateActivityDebounced could merge a later update into the 'updated' row a comment links to, overlaying its agent stamp and bumping created_at, so two agents under one set of credentials would silently re-attribute an earlier comment; comment-linked rows are no longer merge targets. Prose corrected: the linked row is a 'commented' row OR the 'updated' row of an update that carried the comment. * fix(server,web): keep the read-skew guard beside the SQL exclusion; nowrap on every 24ch agent label (TASK-2760, codex r4) The page-local guard covers a distinct failure from the query exclusion — a comment fetched then hard-deleted before the activity query runs — so it returns with that reason written down. Sweep: of the seven 24ch agent-label rules, three lacked white-space: nowrap (both timeline cards and EpisodeFeed), so a name with spaces wrapped instead of ellipsizing; the other four already had it. Prose nits corrected; the pre-link debounce race on update-with-comment is recorded on BUG-2716 with a pointer in the handler. * docs(server,cli): state the reverse read-skew at the guard and the CLI non-rendering decision (TASK-2760, codex r5) * fix(store): debounce merge refuses a comment-linked row inside the UPDATE itself (TASK-2760, codex r6) The read-then-write left a window in which a comment could link the chosen row before the merge overwrote its agent stamp. The merge is now one statement whose predicate re-checks the link under the row write, and a zero-row merge falls through to a fresh insert. Prose corrected: a later update looks past a frozen row, to an older unlinked one or a fresh one. * fix(store,test): one freeze mechanism, and the window-edge leak proven end to end (TASK-2760, matrix survivors) The debounce SELECT-side exclusion became redundant once the UPDATE's own predicate refused linked rows, and its 'look past to an older unlinked row' semantics folded a later change into an earlier entry — a linked row now simply ends the coalescing run. And the server suite could no longer tell the SQL exclusion from the restored in-memory guard, because it only exercised the same-page case; a test now drives the page-edge case codex found (comment outside its window, activity inside), where only the query can help. * fix(web): drop a duplicate nowrap in EpisodeFeed — the rule already had it (TASK-2760, codex r7) Corrects the round-4 sweep count: of seven 24ch agent-label rules, two lacked white-space: nowrap (both timeline cards), not three. |
||
|
xarmian
|
5003718802 |
fix(push): apply delivery's visibility gate to delivered_sessions (BUG-2725) (#1187)
deliveredSessionCount applied three of watchNotificationVisible's four gates, missing the first thing delivery checks: vis.allows(CollectionID, ItemID). Broadcast over-reported. Targeted was worse — the publish-skip reads this count, so the gate passed, the push went out, the stream dropped it on visibility, and the response said delivered_sessions: 1. An instruction lost behind a success. Per Dave's day-49 ruling, visibility is RE-RESOLVED at push time rather than snapshotted: membership and grants are revocable, so a value cached at connect goes wrong exactly when revocation is what makes it matter. The one input that cannot be re-resolved is the target connection's auth transport — computeWatchAccessVisibility consults isBearerAuth exactly once, inside the admin bypass, and the pushing request only knows its own. So SessionOrigin.BearerAuth is recorded at Add(). That is NOT the snapshot the ruling rejected: auth transport is a property of the connection, fixed when it opened and not revocable while held, so it cannot go stale. Armed is the precedent. SessionOrigin is kept separate from SessionIdentity because that type documents itself as self-declared and never verified; folding a server-derived security fact in there would silently retract the warning for one field. Both comments state the rule for future extenders: connection properties are admissible, derived authorization state never is. computeWatchAccessVisibility now takes a bool instead of an *http.Request, which makes the per-connection input visible in the signature and lets the count answer for a connection it is not serving. COST: "re-resolve per counted session" reads like N access checks per push. It is at most TWO, and sessionVisibility's memo makes that true by construction rather than by careful calling — every other input is per-user and identical across the sessions counted, so one varying boolean bounds the answers at two. Pinned by a test with 50 sessions. Codex round 1 (P1): the first version swallowed store errors into "not visible", reintroducing BUG-2698 through this fix — a targeted push reporting 0 SKIPS the publish, so a DB blip would drop the instruction and answer 200, in a function whose own doc comment says why 0 is load-bearing. Round 2 (P1): the same class one layer down — computeWatchAccessVisibility collapsed FOUR store failures into a denial, two discarded into underscores. Fixed as a class per CONVE-18. Resolution and policy are now separate: stream-side callers discard the error explicitly with reasons, only the counting caller propagates. Round 3 CLEAN. CONVE-23 sweep found three consumer-facing artifacts still describing the old mechanism, none on a line this diff touched: the plugin skill doc, the web push dialog, and pad push --help. All three corrected to name what actually remains rather than deleting the caveat. Plugin 0.3.1 -> 0.3.2, since installed plugins are version-pinned at install. NOT fixed, deliberately: the UNDER-count. A stream past maxSessionsPerUser receives broadcasts while never entering the registry. delivered_sessions remains an estimate with error in both directions, and every consumer-facing description now says so. Two coverage gaps recorded rather than rounded off: mutation M11 survives (the reporting test reaches only the first of four store calls, because closing the DB fails it first), and no test drives the whole chain store-fault-to-503 (the DB-close instrument kills the request earlier, so such a test would have gone green against the wrong 500 — deleted rather than relaxed). Also lands the BUG-2752 refutation sentinel: that item claimed the OAuth workspace allow-list went unenforced on /api/v1/events/stream. Refuted — no allow-list-bearing credential can authenticate to /api/v1/* at all. The test guards that format gate, so if it ever widens, the refutation's premise fails loudly instead of silently reopening a leak. Gates on the merged tip: make test 27 pkgs, make lint 0 issues, full Postgres suite 27 pkgs, govulncheck, codex CLEAN, CI 7/7. Claude-Session: https://claude.ai/code/session_01Cpr3teiHHsgcTmg2xhHA86 |
||
|
xarmian
|
72336aacb5 |
fix(events): release SSE admission slots when a client leaves mid-establishment (BUG-2749) (#1186)
`GET /api/v1/events` reserved its admission slot, then blocked in `SubscribeAndReplaySince` while the workspace's Redis subscription was dialled and — since BUG-2747 — acknowledged. Nothing propagated the request's cancellation into that wait, so a client that disconnected during establishment left a process-wide slot, a per-principal slot and a per-workspace slot held for the whole of it. The connection was gone; the capacity was not. Cancellation is now DEREGISTRATION, and `wsCounts` — which already answers "is anyone still here" — decides everything downstream. No ownership hand-off and no reaper: the arbiter already existed. (One thing IS handed off, and only one — the remainder of the confirmation wait; see below.) The two cancellation positions take different paths, and only one of them owes the joiners anything: - Before the install: the existing post-dial critical section already abandons and retires correctly when nobody is left. It needed one ordering rule — the departed establisher stops being counted IN THAT SAME SECTION, before the count is read. If joiners registered while we dialled, the count is still non-zero and they get the subscription; that is the hand-off the filing asked about, expressed as a count rather than a transfer of ownership. - During the confirmation wait: the subscription is already installed with its receive loop running, so the connection is not at risk — but the WAIT is what releases the joiners, and dropping it would admit them into a subscription Redis has not acknowledged while telling them nothing. That is BUG-2747's defect re-created at the seam between the two designs. So the remainder of the wait moves to a goroutine that finishes exactly as the caller would have: same arms, same `markUnconfirmedAdmission` on the bound, same `finishPending`. Bounded by `confirmTimeout`; no reaper needed, because teardown stays count-driven. A departure is not a refusal. `ok bool` is replaced by a `SubscribeOutcome` enum across the three `EventBus` Subscribe methods, so `SubscribeWorkspaceLimit` and `SubscribeCancelled` cannot be collapsed: answering a departed client with 429 would have written a limit refusal into the logs and counters that anyone would use to tune that limit. An enum rather than a second bool or an error because the switch has to name the case — by construction rather than by argument. Caller population, with its search boundary: 3 production implementations (events.MemoryBus, events.RedisBus, metrics.InstrumentedBus), 1 test double (server.gapEventBus, which embeds the interface), 2 production call sites (both in handlers_events.go). Searched this repo four ways — the three method names, `.Subscribe(`, method declarations, and interface embedding. collab.OpBus and watchevents.Bus are different interfaces and are out of scope; no other repo links this package. WHAT THIS DOES NOT FIX, verified in go-redis v9.22.0 rather than inferred from its doc comment (which says Subscribe "does not wait on a response from Redis" and so reads as though no dial happens on the request path — it does; only the reply is unawaited). On plaintext, dialConn derives its per-attempt deadline from the caller's context and the default dialer is net.Dialer.DialContext, so cancellation aborts the dial. Under TLS the same dialer calls tls.DialWithDialer, which takes no context, so the dial stays bounded by DialTimeout alone. On a TLS deployment this shrinks the held slot from (dial + confirm bound) to (dial), not to zero. Review round 2 (codex) found a P1 in this unit's own first draft, of exactly the shape the filing warned about. A cancellation check at the top of the establish loop could return while the caller still OWNED an unretired establishment record: section 1 had already named it the establisher, so the record stayed in pendingSubs with nobody behind it, its done channel never closed. The next subscriber for that workspace would join it and wait forever — and its own registration keeps wsCounts non-zero, so no later caller would establish either. A permanently dead stream that looks alive, produced by a guard whose only purpose was to save a dial. The guard is gone: a cancelled caller now goes THROUGH establishSubscription, which is the only code that knows how to put the record down. Regression test included, and reinstating the guard turns it red. Round 2 also found a P2 shutdown regression: routing the dial to the caller's context alone took away Close()'s ability to interrupt a stalled dial, which it had before. The dial now runs on a context ended by EITHER the caller or the bus, and each half is pinned by its own test — dropping either one is detected. Review round 1 (codex): no P1. One nit fixed as a class — three comments elsewhere in the file asserted the dial was "NOT bounded by the context we pass", which this change falsified; the sweep found and corrected all three (establishSubscription, defaultSubscribeConfirmTimeout, Subscribe). The TLS half of its P2 is filed as BUG-2754: the fix belongs at client construction, where it covers every Redis call rather than this one. Class sweep filed separately as BUG-2751 (lead-ruled: one region, one design per diff): internal/watchevents has no per-request establishment, but its resume path blocks on a 250ms settle window bound to the bus's context rather than the request's, while /api/v1/events/stream holds the same admission slots across it. Tests: five cancellation cases in internal/events (before install, during the wait alone, during the wait with a joiner, a cancelled joiner, an already-dead caller), a dial-binding assertion, and the handler-level binding in internal/server asserting the admission slot itself is released — the half of the bug that does not live in the bus. Mutation matrix, 8 mutations: 7 detected, each by the test named for it. The one survivor is the ctx term in the retry re-decide, and it survives because it is an OPTIMISATION rather than a correctness guard — a departed caller that mints a second record still establishes, deregisters and retires correctly; the term only saves a pointless dial. The code says so rather than implying the guard is load-bearing. The earlier draft's entry guard and loop-top break formed a redundant pair the matrix could only detect when both were removed. That redundancy was the smell, and round 2 found the substance under it: one of the two was not redundant, it was wrong. With it gone the entry guard is detected on its own. |
||
|
xarmian
|
0eb274fed8 |
test: close the mutation gaps a coverage audit named (BUG-2730, codex round 18)
Round 18 walked every behavioural change in the diff, named the smallest edit that would break it, and listed the ones no test caught. Twelve. All but two are now covered, and each new test was verified against the mutation it exists for: - the activity bus's gap channel coalescing (the watch twin had it, this one did not) - Redis-backed atomic subscribe-and-replay, which reaches the guarantee by a different mechanism than MemoryBus and would break alone - a resuming client being held to the per-workspace limit, so the new API is not a second door past a bound the fresh path enforces - the resume-gap report on the new path, with a fresh-subscription control so it cannot fire on every subscribe and still pass - the Redis drop metric, asserted per DROPPED SUBSCRIBER with two slow subscribers, so a report hoisted out of the fan-out loop halves the count and fails - the gap channel surviving Unsubscribe, since closing it would make a consumer's select spin - every subscribe API returning a non-nil signal - the watch handler incrementing the WATCH counter (countMidStreamResync takes a bool to choose, which is the kind of argument that gets passed the wrong way round), with both wrong-counter legs asserted - the production cooldown, which every handler test overrides, so nothing else would notice it set to zero - the wrapper's gauge on the atomic-resume path, which the previous assertion checked only for non-nil-ness Two left uncovered deliberately: an interleaving test at the handler level for the atomic API (the bus-level tests carry that guarantee and the handler cannot arrange the interleaving), and the same for the handler choosing the atomic call over subscribe-plus-EventsSince. Two existing tests were also repaired rather than kept green by luck: the ordering stress test demanded every published event and a slow reader legitimately loses some, and the new Redis atomicity test resumed against a workspace the bus was not covering. |
||
|
xarmian
|
d6480c1f02 |
revert(sse): remove the ordering barrier; its failure mode is worse than the problem (BUG-2730, codex round 16)
Round 16 found the third defect in a row inside the previous round's fix: the gap branch reset gapDrainBudget to the CURRENT queue depth on every signal, so a producer refilling faster than a slow client drains could re-raise the coalesced gap before the budget reached zero and the announcement would never fire — the exact starvation the budget was introduced to prevent, one level up. Rounds 13, 15 and 16 each found a defect in the fix from the round before. That pattern is the signal to stop patching and reassess, so I reassessed the barrier itself rather than fixing it a third time. What it prevented: a client receiving sync_required and then events queued before the hole, whose IDs re-establish a cursor below it. Bounded and self-correcting — the client was told to reconcile, and a later reconnect from such a cursor is refused by the coverage check and told again. What it risked: never announcing at all, on the connection type this whole unit exists for. Unbounded silence. A mechanism whose own failure class is worse than the one it fixes should not ship, so the barrier, its drain budget and its predicate are gone. The announcer and its cooldown stay: they answer a real feedback loop and they latch rather than drop, and their binding to both handlers is tested. The residual ordering behaviour is now documented in docs/deployment.md under what a client should do with sync_required, and in a comment at the gap branch — stated rather than left for a reader to find, which is the same posture as the rest of this unit. |
||
|
xarmian
|
7c03beb24e |
fix(sse): bound the ordering barrier by a count, not by the channel emptying (BUG-2730, codex round 15)
The barrier shipped one commit ago with a comment asserting it could not starve. That was wrong, and wrong in the way that matters: it waited for len(ch) == 0, which never happens while a publisher refills faster than a slow client drains — and the subscriber this whole signal exists for is precisely a slow one on a busy workspace. The announcement it was supposed to make could be deferred indefinitely. The wait is now bounded by the queue depth captured when the gap was latched, decremented once per event taken off the channel. Once that many have gone out, every event that predated the hole has been delivered and anything still queued arrived after it, so the ordering guarantee is satisfied and the announcement goes. Terminating by construction, and exact rather than a timeout. The decrement counts filtered events too — an invisible event occupied a queue slot like any other. An honest note on the instrument, because the first one was no good. I wrote an end-to-end test with a goroutine publishing continuously and it PASSED against the unbounded version: under most schedulings the channel does briefly empty, so the scenario is not reliably reproducible through the handler. The bound is therefore a named predicate, gapReadyToAnnounce, with the starvation case asserted directly — latched, budget spent, channel refilled — where it cannot be scheduled away. The end-to-end test stays for the ordering claim, which it does discriminate. |
||
|
xarmian
|
3a00783557 |
fix(sse): queued events go out before the gap announcement (BUG-2730, codex round 13)
Reading both handlers as state machines: the event channel and the gap channel are two arms of one select, so with both ready Go picks at random. Announcing first and then draining events the subscriber queued BEFORE the hole is the wrong order twice over — the client is told its position is untrustworthy and then immediately handed IDs that re-establish one, below the hole; and on an ID-space change those queued events belong to the space that was just abandoned. The gap is now latched and the announcement waits for an empty channel. Nothing is discarded to achieve it, and that restraint is the load-bearing part on the watch stream: a queued one-shot PUSH cannot be recovered by any reconcile, so dropping it to make the cursor tidy would destroy the only copy. Draining cannot starve the announcement either — one event per iteration, re-checked at the top, so it lands on the first iteration with nothing queued, immediately when the channel was already empty. Pinned by asserting the ORDER of the frames with twenty events queued ahead of the gap. Without the barrier that fails on the first or third event, roughly half the time per run. |
||
|
xarmian
|
b3c5ba95f5 |
style: gofmt the struct-field alignment the new Server field broke
Caught by make lint, after I chained the commit onto the same line as the gate and shipped it on a failing exit code. Same shape as the rule about never piping a gate: read the exit status before the commit runs, not alongside it. |
||
|
xarmian
|
a82bbd6b4f |
test(server): the rate limit has to be tested where it is BOUND (BUG-2730, codex round 11)
A one-survivor pass on the added tests: the announcer was tested directly and each handler test injected a single gap, so a handler that bypassed the limiter entirely and emitted sync_required straight from `case <-gaps:` passed everything — reopening the exact feedback loop the limiter exists to prevent. The same CONVE-19 shape as the wrapper: the component was vouched for, the binding was not. Both handlers now drive a burst through one connection and assert both halves of the bound: exactly ONE announcement inside the window, and one MORE after it. The second leg matters as much as the first — a handler that discarded the extras rather than latching them would satisfy the first and be this fix's own defect one layer up. The cooldown becomes a Server field so the test can narrow it. An integration test that waited out five real seconds per assertion would not have been written, which is how the gap got here. |
||
|
xarmian
|
8799e7d0cb |
docs: correct the comments this change made wrong (BUG-2730, codex round 7)
A next-maintainer read of every comment against the code it describes found nine, most of them made stale by this branch: - the watch observer and its fan-out still said a subscriber holding a stream open is told nothing about a sequence gap, which is the exact sentence this unit exists to falsify - the events interface described the gap signal as only a full-channel drop, omitting the coverage-loss scope that reaches the same channel - both SubscribeAndReplaySince doc comments still described a two-value return and an eviction-only nil - the InstrumentedBus header said it wraps without changing the interface or its implementations, in a diff that changes both - the SSE handler said a restarted Redis counter is undetectable, which BUG-2736 fixed; what stays silent is narrower And three correctness points about the new metrics, all conceded: - drops and mid-stream announcements are NOT one-to-one. Coalescing and the 5s latch turn a burst on one connection into a single announcement, so the counter measures announcements, not clients, and a large ratio means one client far behind rather than many affected. - the announcement counter increments before the write. Stated rather than changed: counting after would lose every announcement to a client that vanished mid-write, which is the population most worth seeing. - the doc said a connection is told at most once per five seconds. Only the MID-STREAM announcement is bounded; the resume signal is not, and never needed to be. A pass stripping review-history attribution from comments was reverted rather than shipped: it churned 50 files, and the surrounding code uses that attribution style throughout, so removing it here would have made this diff the inconsistent one. |
||
|
xarmian
|
b7ae022b6f |
refactor(events): every way of subscribing hands back the gap signal (BUG-2730, codex round 5)
Subscribe allocated and raised a gap channel its callers could not read, which round 5 called dead work. The read is right and the disposition is the other one: an interface method whose subscribers CANNOT be told they missed something is a silent under-delivery waiting for its first production caller, and internal/watchevents' Subscribe already returns the signal, so the asymmetry was the defect rather than the allocation. Subscribe now returns it too, on all three implementations. No production caller changes — the handlers use SubscribeIfAllowed and SubscribeAndReplaySince — so this is a test-call-site sweep plus one signature. |
||
|
xarmian
|
d936464736 |
fix(events): bound the mid-stream signal, and stop it moving existing alerts (BUG-2730, codex round 4)
Three findings from the operator-at-3am angle, all real. A pub/sub outage on a workspace with a subscriber but NO replay buffer yet was silent. dropWorkspaceCoverage returned early before telling anyone, on the reasoning that there was no coverage to end — true of the BUFFER, and beside the point for the SUBSCRIBER, which has the largest possible hole and the least evidence of it. Live subscribers are now signalled on that path while the reset metric stays suppressed: the metric measures coverage endings, the signal measures clients who may have missed something, and those are different questions. The gap channel coalesces, which bounds the queue but not the loop: once the handler consumes a signal the next drop re-arms it, so a slow client could be answered with a delta sync, made slower, and answered again. Both handlers now share a gapAnnouncer that allows one announcement per connection per 5 seconds — a delta-sync round trip, not a tuning knob — and LATCHES rather than drops, so a gap inside the window is announced when the window closes. Suppressing it would be this fix's own defect one layer up. Folding mid-stream signals into pad_*_resume_gaps_total silently changed what every existing alert on those counters measures, and a mixed-version fleet would have reported two populations under one name for the length of a rollout. They go back to counting resumes; the new population gets pad_event_midstream_resyncs_total and pad_watchevents_midstream_resyncs_total, which count CLIENTS TOLD rather than causes — one instance-wide coverage loss moves them once per subscriber while the reset counter moves once, and that ratio is the fan-out an operator wants when judging a storm. |
||
|
xarmian
|
b9dff0072e |
test: close the coverage gaps codex round 3 named (BUG-2730)
Round 3 reviewed the added tests as production code and found four things, all real: - the atomicity test never asserted the replay CARRIED anything, so an implementation that always answered "cannot vouch" and delivered everything live would have satisfied "never both" by never replaying. It now seeds two events and resumes from the first. - both handler tests would have passed for a handler that emitted sync_required and then closed the stream. The activity one now proves an ordinary event still arrives afterwards; the watch one, which has no cheap ordinary event to publish, proves the handler did not return. - the refused-connection test's end state also holds on origin/main, so it argues for nothing about the new ordering. It is a regression test for the defect that ordering introduced, and the comment now says so. - whole paths had no test: the activity RedisBus's drop, coverage-drop and ID-space-reset signalling; the watch bus's epoch-change and counter-backward arms; the InstrumentedBus delegation; the metrics adapter; and the mid-stream counter increment. All covered now. The wrapper tests matter most of the three new files: it is the only implementation that does not originate a gap channel, and in production the bus IS wrapped, so a wrapper returning nil there would have disabled the whole fix while every bus-level test stayed green (CONVE-19). |
||
|
xarmian
|
b2277641cf |
fix(events): a refused connection is not a sync_required (BUG-2730, codex round 1)
Moving the Last-Event-ID parse above the subscribe — which is what makes the atomic subscribe-and-replay possible — put the unreadable-cursor increment on the wrong side of the per-workspace admission check. A connection refused with 429 is sent nothing, and was still counted in pad_event_resume_gaps_total, whose population is signals SENT. Counted at the emission site instead, which is where it effectively was before the parse moved. Pinned with both legs: refused must not count, admitted-with-an-unreadable-cursor must. |
||
|
xarmian
|
1cee8e615c |
test(server): the quiet-control leg could not see a gap announced at connect (BUG-2730)
A mutation that made the activity handler announce a gap unconditionally at connect SURVIVED both tests. The control asserted the absence only AFTER publishing an ordinary event, and waitForFrameWithEvent reads past every frame that is not the one it wants — so the spurious sync_required went by unexamined and the window that followed was genuinely empty. The absence is now asserted first, before anything else is published, and the wait for the ordinary event refuses a sync_required instead of reading past it. Both legs fail on the mutation now. |
||
|
xarmian
|
af99762815 | test(server): the gap signal must reach the wire, not just the bus (BUG-2730, CONVE-19) | ||
|
xarmian
|
4269bdd4cc |
fix(watchevents): the same hole, told to the stream holding it open (BUG-2730)
fanOutLocally already DETECTED a gap in the received notification sequence, logged the exact id range, and raised knownFrom so a client RECONNECTING across it was honestly told sync_required. A client holding the stream OPEN across the same gap was told nothing: it stayed connected, kept receiving everything after the hole, and never saw what went missing. The instance knew; the one consumer whose correctness depended on it did not. Subscribers now carry the same capacity-1 coalescing gap channel the activity bus grew, raised from two different scopes: - per-INSTANCE, to every live subscriber, when this instance discovers a hole in what it received — a sequence gap, a counter that went backwards, an epoch change. Instance-scoped is the whole scope: the ids never arrived HERE, other instances may have them, and every subscriber registered at the moment of detection is exactly the set that was connected across the hole. - per-SUBSCRIBER, to one connection, when its channel was full. The watch SSE handler answers with sync_required mid-stream. The pad CLI monitor already clears its cursor on that event, so the client half needed no change. Refs BUG-2730. |
||
|
xarmian
|
b5615cc1b5 |
fix(events): a live subscriber is told when it has a hole (BUG-2730)
The activity bus dropped an event for a subscriber whose 64-deep channel was full, logged it, and continued. The subscriber was told nothing, so a later delivered event advanced its Last-Event-ID past the dropped ids — after which no replica would ever replay them, because every replica agrees that cursor is current. Only a full reconciliation corrected it. Every subscriber now carries a capacity-1 coalescing gap channel. It is raised when the bus cannot hand that connection an event, and when this instance's coverage of a workspace ends under it (a pub/sub flap, an undecodable message, an ID-space reset) — that second case is per- instance and reaches every subscriber of the affected workspace, because the gap is theirs collectively. The SSE handler selects on it and emits sync_required mid-stream, the same signal a resume we cannot serve gets: the web client already answers it with an incremental /changes delta, so the distinction a new event name would express is one the client would act on identically. Also folded in, because they are the same defect surface: - Subscribe-and-replay is now ONE critical section on both bus implementations (SubscribeAndReplaySince), closing the duplicate window where an event published between the two steps landed in both the replay set and the live channel. MemoryBus takes b.mu across the buffer append and the fan-out to get it; the lock order is b.mu then b.replayMu, everywhere. - internal/events gains the drop report its sibling has had since BUG-2699 (Observer.EventDropped, pad_event_events_dropped_total). The drops were log-only, so the condition this fix makes honest was not countable before it. Both resume-gap counters' help text now names what they actually count — sync_required signals, including the mid-stream ones — rather than resumes alone. Refs BUG-2730. |
||
|
xarmian
|
f3aa86503a |
fix(events): assign the in-memory id under the lock that orders the buffer (BUG-2736)
Codex round 18, pointed away from the Redis bus that seventeen rounds had concentrated on. Three findings; one belonged in this unit, one is filed, one is documented. THE ID WAS ASSIGNED BEFORE THE REPLAY LOCK, so two concurrent publishes could take N and N+1 and append in the other order. replayBuffer.since computes oldest and newest by POSITION, so a buffer holding [N+1, N] reports N as its newest and answers a resume from N+1 with sync_required — a client told to resync at the moment it was exactly current. Pre-existing in shape, but it is the same invariant this unit buys for the Redis bus with an atomic publish script, on the same buffer, for the same reason: publish order must equal id order because the buffer's own ordering assumptions are otherwise false. Fixing one and leaving the other would be half an invariant. The test drives 300 concurrent publishes and asserts both the order and its consequence — that every id in the buffer is servable as a cursor, which under the race the newest ones were not. Verified to fail 5 of 5 with the assignment moved back out. FILED, NOT FOLDED IN: BUG-2737. Neither activity bus refuses a subscription after Close, so a handler that subscribes during shutdown holds a channel nobody will close and blocks for the full 30s deadline. It is a shutdown-lifecycle defect rather than an id-space one, it spans both implementations, and internal/watchevents already fixed the identical thing in BUG-2651 — so the fix is porting a decided question, not answering one. DOCUMENTED: the SSE data body carries the event id as a JSON NUMBER, which is now around 1.8e18 and past JavaScript's MAX_SAFE_INTEGER. It cannot be removed — the Redis bus's phase-1 wire form carries the id there and nowhere else — so the frame writer now says that the "id:" field is the one a client may use, and why web's ItemEvent deliberately declares no id. Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X |
||
|
xarmian
|
d393126d80 |
test(events): close the gaps a tests-as-production-code pass found (BUG-2736)
Codex round 4, on the tests themselves. Eight findings, all real; three of them were behaviours in this diff with no test at all. NO TEST AT ALL: - The real receive path. Every reconciliation test drove fanOutFromRedis directly and the publish tests read the wire with a raw subscriber, so a regression that decoded the epoch correctly and then handed 0 to the fan-out would have passed all of them -- reconciliation silently never running in production. Now driven through Subscribe/Publish and back through Redis, with the mutation checked. - The atomic script's ordering claim. Every phase-2 test published once or ran the script sequentially, so a two-call INCR-then-PUBLISH implementation passed them all -- and that ordering is load-bearing, because the receive path reads a descending id as a counter reset. 300 concurrent publishes now assert arrival order equals id order; verified to FAIL 5 of 5 against a two-call implementation and pass 3 of 3 against the script, so the instrument discriminates rather than merely being green. - The TOML tag. The env-var test proved PAD_EVENTS_PUBLISH_EPOCH reaches the field and said nothing about the toml:"events_publish_epoch" tag -- the exact form the rollback procedure warns about, since a file value outlives an unset env var. PASSING FOR THE WRONG REASON: - The production config wiring was still unexecuted: passing an empty config.Config at both RunE call sites compiled and passed everything. The source-text guard that already counts those call sites now also requires them to pass the loaded config. - The phase-2 wire assertions accepted a well-formed payload with an empty event body. They now assert the body survives. - The Redis metrics subtest had no served-resume control, so a bus that refused every resume would have passed. It now round-trips a publish through Redis first. - TestResumeGapIsReportedForBothWaysOfNotServing never proved ws-warm HAD a buffer, so its second half could silently duplicate its first. - internal/server's cold-resume tests still sent a literal 4200, which the incarnation guard now answers before the handler's no-buffer path is reached. My own round-1 sweep of this class stopped at four packages and never looked at internal/server: reviewer-named instances are a sample (team CONVE-18), and so, evidently, are self-named ones. Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X |
||
|
xarmian
|
86b0f7508f |
docs(server): name the subscribe-then-replay window where it lives (BUG-2730)
Codex round 18 was asked to assume exactly one defect survived sixteen rounds and to find it rather than survey. What it returned is the subscribe-then-replay duplicate window — a REAL defect, and one already filed on BUG-2730 by two earlier rounds. That it went that deep and surfaced a known residual rather than a new defect is the useful result. But the code said nothing at the site, so a successor reading handleSSE would have to re-derive it, exactly as three review rounds did. Now stated where it happens: the window, what it costs (a duplicate toast and duplicate work, never a lost event), how internal/watchevents closed the same window with SubscribeAndReplaySince, and why closing it here is its own unit — a new method on events.EventBus across three implementations, folded together with the admission check SubscribeIfAllowed already performs. That is a change about DELIVERY, and this one is about COVERAGE. No behaviour change. Refs BUG-2730, BUG-2731 |
||
|
xarmian
|
f2a037e393 |
docs: nine claims about other people's code that I had not checked (BUG-2731)
Codex round 16, aimed at every factual assertion this diff makes about
code OUTSIDE it — go-redis, the SSE spec, HTTP header handling, the web
client, internal/watchevents, Prometheus. The angle was chosen because
this diff had already been caught twice asserting library behaviour that
was false, and claims about other people's code are the one class no test
in this repo can falsify.
It found nine. Every one is mine, and every one claimed more than I had
verified.
- "no reconnect in 24 seconds of probing" cited an experiment that is
not in the tree — the probe was deleted with the test it belonged to.
The MECHANISM is checkable from the library source and now says so
with the call named; the unretained number is gone.
- "the SSE `id:` field has no room for an ID-space identity" is wrong.
The spec allows an arbitrary UTF-8 event ID. What excludes it is PAD's
own contract — an int64 every deployed client already parses — which
is a stronger and more honest statement of the constraint, and it is
the one BUG-2736 has to argue against.
- "the spec defines an empty header as no position" overstated it. The
spec governs what a client SENDS. What a server does with a value it
cannot use is our policy, and the test now says so.
- "HTTP strips optional whitespace from header values" is too broad: Go
trims on the way OUT, while the incoming MIME parser only TrimLefts.
What I measured was the round trip, and the comment now claims exactly
that.
- "every gap is a full resync / full re-fetch" is wrong in three places.
The web client answers sync_required with an incremental /changes
delta and only falls back to a full refresh after a long absence or a
failure. This one matters beyond wording: the load argument for the
whole fix rests on what a gap costs a client.
- "a wrapper cannot see that a resume gap occurred" — it can see the nil;
what it cannot see is WHY. I had already corrected this in the metrics
adapter and left the overbroad version in the seam it describes.
- internal/watchevents' `since` no longer "mirrors internal/events
exactly" — that stopped being true when knownFrom went into the
latter's `since`. Now states where the two differ and why.
- "the counter returns to baseline" — a Prometheus counter only
increases; its RATE returns to baseline. Two places.
- "the only case where INCR fails while PUBLISH still reaches
subscribers" — an ACL permitting one and denying the other is another.
The test now names the SHAPE as what matters and its arrangement as
one route to it.
No behaviour changes; comments, docs and test prose only.
Separately verified while waiting on this round, and now cited rather than
asserted: the three WHATWG steps that make the empty `id:` cursor
retirement work. That claim was the one thing in the diff I had taken from
memory of a spec rather than read, and it is load-bearing — if wrong, the
feature is theatre.
Refs BUG-2731
|
||
|
xarmian
|
5cbeb52784 |
docs(events): three claims the split left describing code that is gone (BUG-2731)
Codex round 14, a post-split damage review. Two findings, both stale
prose, which is the failure mode a SUBTRACTION has: the code is correct and
the comments describe the version that was removed.
- fanOut still credited "publishScript" for the ID. The activity bus does
INCR then PUBLISH again; the script went to the migration.
- replayBuffer.knownFrom and handleSSE both listed an ID-space reset among
the things that invalidate coverage. That detector went with the
migration, so a reset Redis counter can still leave two incarnations'
IDs in one buffer.
The second matters more than a wording slip: it claimed a guarantee the
remaining code does not provide, which is the shape that gets a successor
to trust a boundary that is not there. Both now NAME the omission and point
at BUG-2736 instead of implying coverage they do not have — the same
boundary-declaration the knownFrom comment already carries.
Codex's verdict on the rest, worth recording because it is what the gate
was for: "the replay-coverage logic and corrected tests are otherwise
coherent on their own."
Refs BUG-2731, BUG-2736
|
||
|
xarmian
|
9f88e94832 |
fix(events): a resume must not be answered from coverage we never had (BUG-2731)
internal/events answered a Last-Event-ID resume with an empty-but-non-nil
slice whenever the workspace's replay buffer could not speak to the span
being asked about. The SSE handler reads that as "caught up", so the client
sat on a live stream believing it was current while everything between its
cursor and now was silently gone.
COVERAGE. replayBuffer gains knownFrom: the lowest event ID from which this
instance's coverage of a workspace can be vouched for. A resume from below
it answers nil, which the handler already turns into sync_required. Covers
a buffer that does not exist (cold start, restart, scale-up, or simply the
first connection to a workspace on this instance), a buffer that exists but
starts above the cursor — NOT full and NOT empty, reachable on any
multi-instance deployment with no eviction and no restart — and a non-zero
cursor from a previous incarnation of a single process.
knownFrom here means RECEIVING-continuity, never ID-contiguity, and the
defining comment says so with the measurement attached.
internal/watchevents has a field of the same name that ALSO detects holes
by noticing a non-consecutive ID; porting that would have been a serious
regression, because this bus has a global counter and per-workspace
buffers, so a workspace's buffer holds non-consecutive IDs by construction
(four publishes alternating across two workspaces measure as W=[1 4],
X=[2 3]). An ID-contiguity check would fire on nearly every append and turn
every resume into sync_required — the false-positive inversion of this bug.
LIFECYCLE. Coverage now ends where it really ends:
- a stopped workspace subscription drops its replay buffer. Keeping it
"in case they come back" looks like a free win and is the bug: events
published elsewhere never enter it while it goes on looking complete.
- subscriptions are generation-numbered, so a straggler from an ended
subscription cannot re-create a buffer and vouch for coverage that
ended with it — including the case where the workspace has already been
resubscribed under the stale goroutine.
- a pub/sub reconnect ends that workspace's coverage. PubSub.Channel
resubscribes transparently, so a Redis failover left a hole the buffer
had no idea about; the loop reads pubsub.Receive instead. It must
RECOVER rather than exit — returning on a transient error would leave
an instance publishing fine and receiving nothing — and it drops ONE
workspace's buffer, since a dropped subscription says nothing about any
other channel.
Subscribers are indexed by workspace because the replay buffers moved under
the same mutex (necessary for the straggler race): scanning every local
subscriber under that lock would make one hot workspace the serialization
point for every other workspace's fan-out and every resume.
Also removes Publish's local-counter fallback on a failed INCR, which
minted an ID from a process-local space and published it — every receiving
instance reads that as the counter having been reset. It bought nothing:
this bus has no local fan-out path, so an event that does not reach Redis
reaches no subscriber here either.
SIBLING. internal/watchevents had the identical cold-resume defect on its
MemoryBus — its RedisBus guards it, MemoryBus reached the buffer directly —
so a single-process instance answered a post-restart resume as caught up.
Found by a cross-artifact review pass; the guard goes in `since` so both
implementations inherit it, and is tested through SubscribeAndReplaySince
as well as EventsSince because that is the path the handler uses.
Refs BUG-2731
|
||
|
xarmian
|
bb003dd6bb |
fix: five claims the final comment-truth round found (BUG-2724, BUG-2726)
The bounded process the lead set: N rounds, an author prune pass, one final comment-truth round. This is that round's output, and the loop stops here. Two were mechanisms I had wrong, and both are the kind a reader would reuse without re-deriving: - "Different Redis DB numbers do not help" was half true. Ordinary keys ARE DB-scoped, so two installations on different DBs keep separate presence registries; it is pub/sub that ignores DBs entirely, which is why the buses cross-feed regardless. Stating it as "does not help" made the namespace look like the only fix for a problem it only half is. - A namespace cutover's client resync was attributed to the epoch check. That check needs an OLD epoch to compare against and a freshly namespaced bus has none — the resync comes from the cold replay-buffer coverage check instead (knownFrom is zero, so every resume falls below it). Same honest outcome, different mechanism, and the mechanism is what someone reasoning about a cutover would use. Three were stale or over-general after earlier changes: the admission comment still said the global limit is passed to the bus as 0 (that parameter is gone), `pad watch --help` and the plugin monitor description lumped a missing .pad.toml's hourly retry in with the 5s-to-5min backoff, and CLAUDE.md said clients must back off without the browser exception docs/deployment.md spells out. Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X |
||
|
xarmian
|
461c5a3e3d |
refactor: prune the claim surface, and turn one prose claim into a test
The review could not converge on this diff's comments because each round of corrections re-expanded the surface it was reviewing — rounds 16 and 17 found errors inside 15 and 16's fixes. That is a production rate being measured, not a backlog being drained, so the treatment is to write fewer claims rather than review the same ones again. PRUNED, ~135 comment lines: process narration. "An earlier version said X", "found by mutation testing", "codex round N caught this", the scoreboards. Every one of those is already in a commit message, which is where the archaeology belongs; in the source they are claims a future reader has to verify, about a past that no longer exists. KEPT, because they earn it and a reader would otherwise re-derive them: metric semantics, reachability boundaries, what a test does and does not discriminate, why the obvious alternative was rejected, and the hazards that cannot be enforced in code. MOVED TO A TEST, per the rule this run earned the hard way: a comment asserting countable behaviour belongs in the suite. Two test comments in internal/watchevents relied on "this constructor waits for its SUBSCRIBE to be confirmed" — prose, and the same assumption applied to the OTHER bus (which subscribes asynchronously) is what made a namespace test flake. It is now asserted with no polling and no sleep, and the mutation that removes the wait fails it. That rule generalises and is why round 17's find mattered: "counts every unservable resume" was prose, so its falseness could hide a real metric gap. Prose is for claims that cannot be asserted. Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X |
||
|
xarmian
|
35e564298b |
fix: seven more prose claims, one real metric gap, and a flaky test of my own (codex round 17)
The prose angle again, and it is still finding things — which is itself the finding: this diff's comment density is generating wrong beliefs faster than the review is removing them, in the one dimension where the defect is a reader's understanding rather than the program's behaviour. Everything below was a claim I wrote. ONE WAS A REAL GAP, not just wording. pad_watchevents_resume_gaps_total was documented as counting every unservable resume, and counted only the half decided by the shared counter. The LOCAL half — a cursor below what this instance can vouch for, from a hole or a cold start — returns nil from replaySince, becomes sync_required for the client, and reported nothing. Now counted, on the deferred path so it fires with the lock released. Its test needed a second pass to be an instrument: the first version arranged a hole and asserted the counter moved, but the shared counter disagreed too, so resumeOutrunsLocalView reported and the mutation survived. It now sets the counter to AGREE with what the instance has seen, which is the only arrangement that isolates the local path. The prose corrections, swept by grep rather than by instance this time: - MemoryBus's comment said a single-process deployment never wires an observer. cmd_server wires one, deliberately — that is what makes the drop counter meaningful there, which is a claim I had just added elsewhere. - "Every write path works with Redis down" was too strong in three places. Push answers 503 for an unresolvable targeted push and 502 push_unconfirmed on publish failure — the paths whose job IS cross-instance delivery. - Presence-failure consequences were stated as certainties in four more places after round 16 fixed one. A failure means an error was REPORTED; Redis can fail a pipeline after applying it. - The deployment metrics table still described pad_eventbus_publish_total as "Events published" after the Help string had been corrected to attempts. - The reserved-namespace rationale called prefix nesting a "collision". It is nesting; an exact collision would need the namespace to match a workspace UUID. Refused anyway, and now for the reason that is true. - A presence cutover was described as stranding one renewal interval of stale entries. It is the full 90s TTL — three intervals. AND A FLAKE OF MY OWN, caught by the full suite rather than by the targeted runs: the activity-bus namespace test asserted subscription state immediately, but that bus subscribes ASYNCHRONOUSLY (the watch bus waits for confirmation; the two differ). It now polls, and the asymmetry is named in both tests so the next reader does not assume symmetry the way I did. Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X |
||
|
xarmian
|
7c8ed3c815 |
fix: nine false or overstated claims in this diff's own prose (codex round 15)
An angle worth naming, because it found more than several code-shaped
ones did: check the COMMENTS against the CODE. This diff is
comment-heavy and its comments make specific factual claims. Nine were
wrong.
The one that mattered most was a false argument for a correct rule.
redisns.Parse rejects colons, and justified it with a collision example
that does not happen: ns "a:events" builds pad:a:events:events:<ws>, not
pad:a:events:<ws>, because the suffix is appended too. The rule stands on
its own grounds (a colon spans segments and makes the keyspace ambiguous
to read back) — but a false example is worse than none, because the next
reader trusts it.
Chasing that turned up a REAL collision needing no colon: a namespace
equal to one of Pad's own first segments nests this installation inside
the default one's keyspace. Namespace "events" puts every key under
pad:events:*, which is the default installation's activity channel space
— the exact cross-feed the namespace exists to prevent, arriving through
the namespace. Now rejected, with a control leg asserting that names
merely CONTAINING a reserved word ("events-eu", "prod-session") stay
valid.
The other eight:
- "The three keyspaces cannot drift" — overstated. Each constructor takes
its own Keys; a source-reading test is what enforces it, which is
weaker than a compiler and now says so.
- Two docs claimed both SSE endpoints incur a presence registration. Only
the watch stream registers.
- The Redis metrics section said they "stay at zero" without Redis, while
pad_redis_up is deliberately unregistered — the section contradicted
the field three lines below it.
- The presence-failure metric's HELP string still carried the blanket
"leaves sessions unlisted and untargetable" that the field comment had
already been corrected away from. Two of the four ops fail in the
opposite direction.
- A nil from MGET was described as proof the process died. Eviction, a
restart and a manual DEL produce the same nil, and this file's own doc
says eviction is indistinguishable from expiry.
- A test comment claimed to cover both corrupt-entry shapes; the second
is unreachable and the subtest is deliberately absent, as the note ten
lines down already said.
- "Enumerates every refusal path" covered per-instance and per-workspace
and not per-user — the same undercount as round 13's, one round later.
Both per-user paths added.
- The Observer contract said a go-redis drop is reported as a sequence
gap. Only if a LATER notification arrives to expose the hole: drop the
newest message on a bus that then goes quiet and nothing is reported.
Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X
|
||
|
xarmian
|
9d54f24626 |
fix(server,cli): the half of round 12's fix I missed (BUG-2726)
Codex round 13, unanchored, found that my previous commit fixed one of the two refusal paths on /api/v1/events. The admission check moved above the SSE headers; the PER-WORKSPACE check stayed below them, so half the 429s on that endpoint still carried the JSON error envelope under Content-Type: text/event-stream — the exact defect the commit said it fixed. Team CONVE-18 in its own shape: the reviewer named one instance, I fixed that instance, and the class had two members. The enumeration I owed was "how many ways can this handler refuse", and it takes ten seconds to read. Every refusal is now above the header block, with a line saying nothing below it refuses. The contract test made the same omission and is the reason this reached another round: it drove the admission bound on both endpoints and never the per-workspace one, so it agreed with a handler that was half fixed. It now enumerates all three refusal paths, and the mutation that reintroduces the defect fails it by name. Also from round 13: `pad project watch`'s 429 message named the two knobs that cover both streams and omitted PAD_SSE_MAX_PER_WORKSPACE, which is the one most likely to be the cause on a busy workspace — true as far as it went, and pointing the reader away from the answer. Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X |
||
|
xarmian
|
3e3170e915 |
fix(server,cli,docs): the consumer contract, per codex round 12 (BUG-2726)
An angle no earlier round took: what does a CLIENT see. Two of the five
findings were about consumers I had never opened.
- `pad project watch` returned "event stream returned 429: {json}" and
exited, which sends the reader looking for a bug rather than at a
limit. It now says what happened and which knobs govern it, and names
the fact that those knobs cover this stream and the agent watch stream
together. It still exits rather than backing off — it is interactive,
and a human can decide — unlike the unattended monitor, which already
folds 429 into its ladder.
- Both endpoints now answer a refusal through one helper: same status,
same code, same message, plus `Retry-After`. `/api/v1/events` was
setting `Content-Type: text/event-stream` BEFORE the admission check,
so its 429 carried the JSON error envelope under an SSE content type —
a different contract from its sibling's for the same refusal. Admission
moved above the headers, which is where it belonged anyway.
- The anonymous-caller rule was documented as if it applied to both
endpoints. It applies to `/api/v1/events` only; the watch stream
requires a resolved user and answers 401 without one.
- docs/architecture.md described one SSE endpoint and one bus. It now has
the table: two streams, two buses, different scopes and consumers, one
shared connection budget, one Redis namespace.
FILED, not fixed: the web UI's `EventSource` cannot see a 429 or a
`Retry-After` — the spec exposes neither to the page — so a refused
browser tab reconnects at a constant rate while the CLI backs off. That
asymmetry means reaching the limit sheds load from the population that
respects it and not from the one that grows fastest under it. No
server-side change closes it; the fix is a client-side reconnect wrapper.
BUG-2733, and docs/deployment.md warns operators to size the limit with
it in mind.
Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X
|
||
|
xarmian
|
a167005654 |
fix(server): register the stream gauge per metrics instance, and drop the comments the last refactor falsified (BUG-2726)
Codex round 11, reviewing the collector conversion: - The registration guard was a sync.Once per Server, which is wrong in the direction that hides: SetMetrics can install a DIFFERENT registry, and the second one would silently never get pad_stream_connections_active. An absent metric is worse than a wrong one, because a dashboard with no data reads as a deployment with no traffic. Now tracked per metrics instance, which still cannot double-register (MustRegister panics) and follows a replacement. - Two comments and a test comment still explained the deleted observer machinery — "the discarded gate keeps its observer, so its releases keep driving the gauge" — which stopped being true when that machinery was removed one commit earlier. The reasoning they were making still holds for a different reason (a replaced gate is still reachable through Server.admission(), so the scrape reads the new gate and looks plausible), so they say that instead. And heldTotal's doc still opened with the old name. Falsifying your own comments while fixing something else is the failure this diff has hit repeatedly; catching it one round later is the loop working, not the comments being unimportant. Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X |