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 b0192871 when I
counted it this session, and is more now. 18 came from the previous
checkpoint's own miscount, which I had ALREADY identified and written up
before I typed it again here. A number that arrives inside a sentence
about something else does not feel like a claim, which is exactly why it
survives. The count is incidental to the argument, so it is gone rather
than corrected - a figure that has to be maintained to stay true is a
liability in a doc comment.

"this branch's one regression came from exactly that" - the ruling's
reasoning, restated by me as a verified fact. The regression I know
about came from wiring a fix off a reviewer-named sink list without
reading the mechanism, which is adjacent to "restructuring late under
review pressure" but is not the same mechanism, and I did not check
whether it is the one the ruling meant. Now attributed to the ruling and
stated as its reasoning, with the part I can defend - the review loop
finding something in nearly every round indicates a design problem -
carrying the argument.

Claude-Session: https://claude.ai/code/session_01AUvLoXsKdS5sdpYju6rj4p

* fix(server): sanitise the MCP audit tool_name, and correct three claims wider than their evidence (BUG-2803)

Codex round 20, asked for a POPULATION rather than a confirmation
(CONVE-24). It returned a covered list AND four findings; this commit
carries the two that belong to this unit plus the doc corrections.

## The door: MCP audit is a second reader, not a pass-through

parseMCPRequestBody runs its OWN json.Unmarshal and binds the decoded
method / params.name to mcp_audit_log.tool_name, TEXT NOT NULL. A
six-character NUL escape therefore arrives as a real NUL: PostgreSQL
refuses the audit INSERT with 22021 - the exact symptom this unit exists
to remove - and SQLite stores an unprintable tool name. Nothing upstream
catches it; the /mcp transport decodes the JSON-RPC envelope itself
rather than through decodeJSON, so the body rule never sees the request.

Measured before fixing: the decoded name reached the column intact.

This unit's own completeness map had CERTIFIED that reader as safe, on
the grounds that "decoding still happens in the MCP dispatcher". That is
true and it does not bear on what this middleware persists - a correct
description of a mechanism, with no question asked about what it does,
sitting in the one artifact whose job is to say the population is
covered. Corrected there too.

Disposition is SANITISE, not refuse, following the User-Agent precedent
from earlier in this unit, and the rule now lives in one extracted
helper (sanitiseStoredText) with the reasoning attached: the body rule
refuses because the caller asked to store that value; this serves
metadata the SERVER elected to record, where failing the write would
lose the audit row for precisely the request most worth auditing.

Both caller-derived returns are cleaned inside parseMCPRequestBody, so
both call sites - the ok path and the denied path - are covered at the
choke point rather than at either caller. Both are tested: params.name
AND the method path. Mutations un-sanitising each one compile and kill
only their own leg.

## Three claims corrected, all wider than their evidence

- "all 65 call sites" in server.go: measured 70. Removed rather than
  corrected, because the number has to be maintained to stay true and
  says nothing the sentence needs.
- docs/backup.md said a NUL "cannot be stored in a text or JSON column"
  absolutely, two paragraphs above my own text explaining that SQLite
  accepts one. Now stated as what it is: an application rule Pad
  enforces on both dialects, which is exactly why it has to be enforced.
- artifact_import.go said such a value "cannot be stored under any
  encoding this product supports". Refuses, not cannot - stating a
  policy as a capability tells the next reader SQLite enforces
  something it does not.

## Filed, not fixed

BUG-2814 - guarded writes re-emit at-rest NULs (move/copy/restore/
fields-patch), propagating a legacy value to rows that never had one.
Distinct from BUG-2813: that one is about writing a NUL while an old
binary serves, this is the fixed binary SPREADING one already present.
Both dissolve under the same store-layer enforcement, so they are filed
to be designed together rather than patched at each of a long and moving
list of re-emit sites.

Declined: round 20 also reported the release-note assertions as
unsupported. They live in the PR body, which a read-only sandbox cannot
see - the claim is about the reviewer's visibility, not the diff.

Claude-Session: https://claude.ai/code/session_01AUvLoXsKdS5sdpYju6rj4p

* fix(server): sanitise before testing for emptiness, so the audit fallback survives (BUG-2803)

Codex round 21 ranked this the most dangerous un-probed lens, and it is
a boundary my own round-20 fix created.

parseMCPRequestBody tested env.Method == "" and p.Name == "" BEFORE
sanitising. A value made entirely of NUL escapes is non-empty as
decoded and empty once cleaned, so it passed over the fallback and was
then blanked - storing an empty tool_name in a TEXT NOT NULL column.
That is exactly the silent drop the "(unknown)" / "tools/call"
fallbacks exist to prevent; the function's own doc comment says so.

Measured before fixing: both shapes returned an empty tool_name.

Fixed by ordering rather than by adding guards - clean first, then test
- so the invariant is structural instead of something each return has
to remember. Same by-construction preference as the symmetric-gate fix
earlier in this unit.

Worth recording that my first patch was WRONG in a way that compiled:
I put the sanitise above the json.Unmarshal that populates env, so the
method would always have been empty. Caught by printing the patched
function and reading it, not by trusting the script saying "patched".

Claude-Session: https://claude.ai/code/session_01AUvLoXsKdS5sdpYju6rj4p

* fix(server): classify MCP audit on the raw method, and trim only JSON whitespace (BUG-2803)

Codex round 22. Two P2s, both measured before fixing.

## A forgeable audit row - my own regression from the round-21 fix

The round-21 change reordered sanitise-before-compare so the fallback
would survive an all-NUL value. That reorder made the CLASSIFICATION
read the sanitised method, so "tools/<NUL>call" cleaned up INTO the
literal "tools/call" and the parser then lifted params.name and hashed
the arguments for a method that was never tools/call.

Measured: tool_name="pad_item" with a full 64-character args_hash - an
audit row indistinguishable from a genuine pad_item call, mintable by
anyone who can send a request. Worse than the review described it.

Fixed by splitting the two jobs, which were never the same job:
dispatch decisions read what the client actually SENT; sanitising is
for the value that gets STORED. The round-21 boundary is preserved -
a method empty only after cleaning still falls back to "(unknown)".

Fixing one boundary and creating another in the same function is worth
naming: the reorder was correct for the case it addressed and I did not
ask what else read that value.

## Go whitespace is not JSON whitespace

The empty-body shortcut used bytes.TrimSpace, i.e. unicode.IsSpace,
which strips \v, \f, U+00A0 and more. encoding/json accepts none of
them. So a body of just \v trimmed to empty, returned io.EOF, and an
EOF-tolerant caller - playbook run treats errors.Is(err, io.EOF) as "no
arguments supplied" and runs anyway - took a syntactically invalid body
for an ABSENT one.

Now trims exactly the four bytes JSON calls whitespace. The test drives
both directions, because only the pair discriminates: real JSON
whitespace must still shortcut to EOF or the playbook contract breaks,
and non-JSON whitespace must not or the divergence survives. Reverting
to TrimSpace compiles and fails three legs.

Claude-Session: https://claude.ai/code/session_01AUvLoXsKdS5sdpYju6rj4p

* test(server): give the walker an independent oracle, not one that shares its code (BUG-2803)

Codex round 22, finding 3. TestBodyDecodesNULGateAgreesWithAnUngatedWalk
compares the gated function against an "ungated" reference that calls
the SAME production valueDecodesNUL. That is valid for what the test
claims - it pins the raw-prefix GATE - but it structurally cannot see a
defect in the WALKER, because such a defect is present identically on
both sides and cancels.

That matters here specifically: every walker defect this unit has had
lived in traversal, descent, or key matching (rounds 1, 2, 4, 16, 17),
which is exactly the part the differential cannot check.

Added a second implementation of the contract, written in the test and
deliberately not calling the production walker. It shares encoding/json
and jsonEncodedFieldKeys; it does NOT share traversal, descent, or
key-matching. It is iterative with an explicit stack rather than
recursive, so a recursion-shaped bug cannot reproduce in it by accident.

Demonstrated rather than argued. With the nested-document descent
removed from the production walker - a mutant that reopens the exact
door this unit exists to close, and which compiles:

  differential (gate vs ungated)   ok      <- blind, as the finding said
  independent oracle               FAIL    <- catches it

The corpus is also asserted to contain BOTH answers, since two walkers
that always answer false agree perfectly.

Claude-Session: https://claude.ai/code/session_01AUvLoXsKdS5sdpYju6rj4p

* test(server): make the body-reader inventory type-aware, and state what it still cannot see (BUG-2803)

Codex round 22, finding 4. The inventory that claims every request-body
reader is accounted for was lexical, and wrong in three ways - all in
the direction that matters for a test whose job is to say nothing is
invisible:

  - it recognised only the variable names r and req, so a handler
    holding its request as httpReq or orig was INVISIBLE;
  - it matched inside COMMENTS, so prose could make a file look scanned;
  - the manually-listed traits field was already evidence of the
    model-regex blind spot.

My first fix broadened the pattern to any identifier. That was worse,
and worth recording: it matched every unrelated .Body field - input.Body
in comments, fetched.Body in url import, comment.Body, art.Body,
sidecarErr.Body - flagging five files that read no request body at all.
The only route to green would have been listing those five as
accounted, and an accounting entry HIDES future readers in its file. A
false entry is worse than a missing one, so I abandoned that approach
rather than tuning the regex.

Now keyed on the TYPE via go/ast: collect identifiers declared
*http.Request in a function signature, then find reader selectors on
exactly those identifiers. Names stop mattering, comments are not in the
AST, and .Body on anything else is not a match.

Positive control, run rather than argued: a handler taking httpReq
*http.Request and reading httpReq.Body is FLAGGED by the new scan, and
matched zero times by the old regex.

Two limits now stated in the test, because an unqualified completeness
claim is exactly how the MCP audit reader got certified safe while
persisting a decoded NUL: accounting is per FILE rather than per call
site, and only signature-declared requests are seen - one stashed in a
struct field or captured by a closure is not a parameter.

Claude-Session: https://claude.ai/code/session_01AUvLoXsKdS5sdpYju6rj4p

* fix(server): mark a cleaned audit identity, and repair two vacuous tests of my own (BUG-2803)

Codex round 23, plus a defect in my own instruments that the mutation
matrix found and the tests hid.

## Cleaning is lossy, so a cleaned identity was forgeable

Round 22 closed the coarse version: sanitising before classifying let
"tools/<NUL>call" become a genuine tools/call. Classifying on the raw
method fixed that. But sanitising still COLLAPSES distinct inputs onto
one output, so "pad_<NUL>item" stored exactly what "pad_item" stores -
same tool_name, same args_hash - and anyone able to send a request could
mint an audit row and a Prometheus label attributed to a real call.

Cleaning and identity are different jobs. sanitiseStoredTextChanged now
reports whether anything was removed, and an identity that only became
well-formed by cleaning is marked. The cleaned text is kept, so the row
stays diagnosable; the marker keeps it distinguishable. Descriptive text
(User-Agent) keeps the unmarked helper - nothing decides anything on it.

The parenthesised form is what this file already uses for a synthesised
value, and a real method or tool name does not begin with "(", so the
marker cannot itself be forged by choosing a clever name.

## Two of my own tests were vacuous, found by a surviving mutant

I wrote nul := "\u0000" in the round-21 and round-23 tests, which in Go
is the NUL CHARACTER, not the six-character escape text. Those bodies
were malformed JSON that encoding/json rejected, so neither test ever
reached the path it named. The comment on the line said "the escape, not
the character"; the code did the opposite, and the correct form was
already three lines away in the round-20 test.

Nothing in the test output showed this. It surfaced only because the
marker mutation SURVIVED, and because a surviving mutant was treated as
a question - does the test not discriminate, or did it not run - rather
than as either answer.

Both repaired and both now kill their mutants: removing the marker fails
with tool_name="pad_item" and a matching 64-character hash; removing
the emptiness guard fails with "(sanitised) " instead of "(unknown)".

Correction for the record: the round-21 checkpoint said that fix was
measured failing before the fix. That measurement used the broken
literal. The finding was real and the fix is right, but it is only
properly established as of this commit.

Claude-Session: https://claude.ai/code/session_01AUvLoXsKdS5sdpYju6rj4p

* test(server): use the canonical escNULLiteral helper, not a local literal (BUG-2803)

The helper is assembled from bytes precisely so this escape cannot decay
into the NUL character it describes, and its comment says so: written as
a Go literal it is one backslash away from being the NUL itself.

I rolled a local one in three tests anyway, and two of them decayed
exactly as that comment predicted - vacuous until the mutation matrix
caught them. The safeguard existed, was documented, and I walked past
it; using it is the only version of this fix that cannot recur.

Claude-Session: https://claude.ai/code/session_01AUvLoXsKdS5sdpYju6rj4p

* fix(metrics): bound the cleaned-identity marker as a metric label, and correct a false cardinality claim (BUG-2803, BUG-2817)

Codex round 24, which enumerated CONSUMERS of the values this unit
changed rather than asking again whether the guard is right. Most of
that enumeration came back FINE, which is the useful half; three
findings did not.

## The marker must not reach Prometheus as part of a name

The cleaned-identity marker is right for the audit ROW - an operator
reading one row needs to know which tool it resembles. It is wrong for
a metric SERIES: "(sanitised) pad_item" and "pad_item" would be two
series per user and per status, for a distinction no aggregate query
asks. metricsToolLabel collapses the marked form to the bare marker, so
it costs exactly ONE extra label value in total and that value is a
constant rather than anything a caller supplies.

Two tests, and the second exists because the first is not enough. The
direct-call test proves the collapse function collapses. The WIRING test
proves the emit path calls it - CONVE-19, my own convention. Measured:
with the call removed from recordMCPCallMetrics, the direct-call test
stays green and the wiring test fails naming the leaked label.

## A cardinality claim that was never true

internal/metrics documented the tool label as "bounded by the catalog
(~7 tools today)" with arithmetic resting on that. The value is
whatever the caller put in params.name, recorded even for requests that
dispatch later rejects, so an authenticated caller can mint a series per
request. The comment now says so and points at BUG-2817, filed with the
fix shape and the two wrinkles it has to decide - the catalog lives in
internal/mcp, and legitimate JSON-RPC methods are not catalog tools.

That unboundedness is PRE-EXISTING and not this unit's to fix; bounding
the marker's own contribution is, which is why the collapse is here and
the rest is filed.

Also corrected: I wrote BUG-2815 into two comments before filing, and
the filing came back BUG-2817. Predicting an identifier is the same
class of claim as predicting a count.

Claude-Session: https://claude.ai/code/session_01AUvLoXsKdS5sdpYju6rj4p

* fix: keep a storable extension in the filename fallback, and sync the rename draft (BUG-2803)

Codex round 24, the two remaining consumer findings. Both trace to this
unit, and both are cases where a value was made SAFE without asking what
reads it.

## The filename fallback was lossier than its sibling

An unstorable upload name became a bare "upload" - no extension - while
the empty-name fallback two lines below has always produced
"upload.bin". The unusable part of "sh<NUL>ot.png" is the STEM; ".png"
is ordinary text, and it is what consumers dispatch on:
Content-Disposition, the web download anchor, bundle export naming, and
, whose documented contract is handing a path to
something that opens files by extension. That command was measurably
affected - it treats any non-empty stored name as authoritative, so its
MIME-based extension fallback never ran and the temp file was
extensionless.

Fixed at the source: a storable extension survives the fallback,
bounded to 16 bytes so a hostile name cannot smuggle a long tail
through. The CLI keeps a defensive extension fallback for any
extensionless stored name, which also covers rows written before this.

Both directions are tested: "sh<NUL>ot.png" now stores "upload.png",
and "shot.p<NUL>ng" - where the EXTENSION is the unusable part - still
stores bare "upload". Without the second leg, "keep the extension"
could quietly become "keep whatever trails the last dot" and reintroduce
the value the fallback exists to remove. Dropping the extension again
fails the first leg.

## A rename that could never come clean

saveName replaced the app object but never updated the draft, so when
the server normalised the name the draft stayed as typed, the equality
check never matched, Save stayed enabled, and each press re-sent the
same request. The server caps at 120 BYTES via rune-safe truncation
while the input allows 120 CHARACTERS, so any multibyte name near the
limit diverges.

The draft is now assigned the value the server actually STORED rather
than compared for length, which stays correct for any future
normalisation. svelte-check: 0 errors.

Claude-Session: https://claude.ai/code/session_01AUvLoXsKdS5sdpYju6rj4p

* fix: reserve the synthesised-value namespace, and stop a fallback carrying an unvetted extension (BUG-2803, BUG-2818, BUG-2819)

Codex round 25, which probed whether the values this unit SYNTHESISES
can themselves be attacked. Earlier rounds asked whether the guard
refuses bad input; this asked what the substitutes are worth.

## A fallback must not carry an extension the product would refuse

Preserving a storable extension was right; bindableText was the wrong
bar for it. Control characters are valid UTF-8 and not NUL, so they are
storable - and they are STRIPPED when the name is written into
Content-Disposition. So ".s<VT>vg" passes the extension blocklist, which
sees no known extension, and reaches the client as ".svg".

attachments.SafeFallbackExtension now requires a KNOWN, ALLOWED
extension, so a synthesised name can only carry a suffix the product
already accepts on the ordinary path. Tested both ways: an obfuscated
.svg and an unknown .foo are both dropped to bare "upload", while
.png still survives.

That divergence is PRE-EXISTING on the ordinary path, where the caller's
name is stored as given and no fallback is involved - filed as BUG-2818
with the fix shape. This change only declines to add a second door.

## A mutation exposed a guard that could not fire

I first wrote an explicit alphanumeric loop in that predicate as well.
Removing it changed nothing: no key in extMIMEMap contains a
non-alphanumeric character, so the map lookup already excluded every
obfuscated suffix. Keeping an unreachable guard whose comment claims it
stops control characters would have misdescribed which line does the
work - so the loop is gone, and TestExtMIMEMapKeysArePlain enforces the
property it was relying on. A guard that survives its own mutation is a
question, not a clearance.

## The marker was forgeable, so the namespace is reserved

Marking only what cleaning changed was not enough. A caller may name a
tool "(unknown)" - what the parser returns for a malformed body - or
"(sanitised) pad_item", and a genuine request then records the same
identity as a substituted one. The older sentinels always had this;
the new marker inherited it.

A leading "(" is now reserved for values this server synthesises, and
any caller value entering that namespace is marked too, so the two
never collide. Cost stated: an MCP tool genuinely named with a leading
"(" is recorded marked; tool names are identifiers in every catalog
this server knows.

The principled fix for the whole class is a provenance FIELD rather than
sentinel strings in a caller-controlled namespace. That is BUG-2819 - it
is a migration on two tables, and the same trick cannot rescue attachment
filenames, which are legitimately named with parentheses.

Claude-Session: https://claude.ai/code/session_01AUvLoXsKdS5sdpYju6rj4p

* test(server): fix the independent oracle, which was wrong in a branch its corpus omitted (BUG-2803)

Codex round 26, finding 5, and it lands on the instrument I introduced
two rounds ago to check the walker.

The oracle descended into a listed key's JSON document whenever it met
one - including when that key appeared INSIDE a natural object that was
itself under a listed key. Production does not: a natural object or
array under a listed key is USER DATA, because the server marshals it
and nothing re-parses it, so a listed key appearing inside it is an
ordinary field name rather than a document marker.

Measured on {"fields":{"schema":"<escape text>"}}: production=false,
oracle=true. Production is RIGHT and the oracle was wrong, so had that
body been in the corpus the test would have failed and pointed at the
production walker.

It was not in the corpus. That is the part worth keeping: the test
already asserted its corpus was not one-sided - that BOTH answers
appear - and that check passed while a whole branch of the contract went
unexercised. Both answers appearing is not the same property as every
branch being covered, and I had treated it as though it were.

Fixed by giving the oracle the same user-data rule, and both bodies are
now in the corpus - the natural-object case that must answer false, and
its string-valued counterpart that must answer true.

Re-verified that the correction did not blunt it: with the production
nested descent removed, the oracle still fails.

Claude-Session: https://claude.ai/code/session_01AUvLoXsKdS5sdpYju6rj4p

* fix: refuse path-component filenames, and single-source the MIME extension table (BUG-2803)

Codex round 26, findings 2 and 3.

## ".." is not a filename, it is a path component

The server guard listed "", "." and "/" but not "..", which survives
bindableText. filepath.Ext("..") is "." - non-empty - so an extension
check waves it through too, and a consumer joining it onto a directory
gets that directory's PARENT. The CLI builds its temp path exactly that
way.

Both ends fixed, deliberately independently. The server now rejects any
name that is only dots or carries a separator, checked on the trimmed
form so "..." and "./" do not each need a case. The CLI sanitises the
name it receives regardless: a client that builds a local path out of a
remote string should not depend on the remote end having sanitised it,
and this CLI talks to whatever instance it is pointed at.

Tested with "..", "...", "./" and "a/b", with an ordinary name as the
premise leg. Restoring the old narrow guard fails it.

## Two tables for one relationship

The CLI kept its own MIME-to-extension table and it had drifted: images
and video but not gzip, tar, XML, YAML, TOML, HTML, JavaScript or
several documents the server has always allowed. So the extension
fallback added in round 24 silently did nothing for exactly the types
whose viewers most depend on it.

The CLI now delegates to attachments.ExtensionForMIME, and the second
table is gone. Measured after: gzip .gz, tar .tar, html .html, js .js,
pdf .pdf.

The reverse map needs one choice per type where several extensions
share one, and those preferences are asserted to name types the forward
map actually uses - because the first version listed "text/yaml", which
this map does not use (it says application/yaml), so that preference
could never fire. Same class as the alphanumeric guard removed in the
previous commit, caught the same way.

Also recorded against myself: I destroyed both new functions mid-edit by
running git checkout on a file with uncommitted work, to "revert an
approach". That is a documented trap I have hit before and had written
down. The committed function survived; the uncommitted ones did not.

Claude-Session: https://claude.ai/code/session_01AUvLoXsKdS5sdpYju6rj4p

* test(server): make the body-reader scan scope-aware, wrong in both directions before (BUG-2803)

Codex round 26, finding 4. The scan used ONE flat name-set per top-level
function, which is wrong in both directions at once:

  - a function literal inside a handler was scanned with the OUTER
    function's request names, so an unrelated inner variable that
    happened to be called r was FALSELY flagged;
  - a request arriving only as a function literal's own parameter was
    INVISIBLE, because literals were never given names of their own.

A false flag in this test is not harmless. The only way to green is to
add the file to the accounted list, and an accounting entry HIDES every
future reader in that file - so a false positive here converts directly
into a blind spot later. That is the same trap that made me abandon the
broadened regex two commits ago.

Now walks a SCOPE at a time. Each scope inherits its parent's request
names, drops any it shadows with a parameter of a different type, and
adds its own. Local aliases (req := r) are picked up as well, since that
is an ordinary thing for a handler to do and the alias reads the same
body.

Three controls, run rather than argued:

  closure parameter reader   -> FLAGGED
  local alias reader         -> FLAGGED
  shadowed inner variable    -> not flagged

The first two were invisible to the previous scanner, which never gave
literals their own names, and the third is the false positive it
produced - both by reading the code this replaces.

Limits restated honestly rather than left as they were, since two of
them are now closed. Still invisible: a request in a struct field, one
from a context, and one whose type reaches http.Request through an alias
or embedded field. This matches the literal spelling rather than
resolving types; closing those means the type checker, not the parser.

Claude-Session: https://claude.ai/code/session_01AUvLoXsKdS5sdpYju6rj4p

* fix: stop the reverse MIME map emitting BLOCKED extensions, and close four instrument gaps (BUG-2803)

Codex round 27 returned "do not merge yet" with three P1s. All of them
are mine, from the previous two commits.

## The reverse map turned a refusal list into a source of extensions

extMIMEMap is the FORWARD table used to REFUSE uploads - it deliberately
lists .svg, .exe, .com so those extensions can be recognised and
rejected. Reversing it wholesale meant ExtensionForMIME("image/svg+xml")
answered ".svg", where the old CLI table answered nothing, and
 names a local file with that.

So I closed an SVG door two commits ago and reopened one through the
MIME helper. Blocked types now get no reverse mapping at all, and the
test asserts it with a premise leg (the map must CONTAIN a blocked type,
or the assertion never runs). Removing the exclusion fails naming .svg,
.com and .msi.

## The oracle was closer, not identical

Production descends only into a JSON DOCUMENT - a string whose trimmed
form starts with { or [. The oracle unmarshalled any valid JSON, so a
SCALAR under a listed key made it answer true where production answers
false. Closer to production is not a usable oracle; only identical is.
Aligned, and the scalar case is in the corpus.

## The scan was still not scope-aware, and could now MISS a reader

A nested block shared the enclosing name-set, so
{ r := &http.Response{}; r.Body.Read(nil) } was FALSELY flagged. And the
shadowing rule deleted a name rebound to http.Request BY VALUE - which
still shares the Body, since it is an interface holding the same reader
- so that read became invisible. Blocks are now their own scope and a
value request counts.

## The controls I claimed were not in the suite

Round 27 was right: I had run them as throwaway probes and deleted them,
so nothing held the scanner to them. The scanner is now a package-level
helper and TestBodyReaderScanDiscriminates drives it over ten synthetic
files - six that must be detected (plain, unconventional name, closure
parameter, alias, value copy, form reader) and four that must not
(no request, shadowed by a closure parameter, rebound in a nested block,
mentioned only in a comment).

## And an over-refusal of my own making

The filename guard rejected any dot-only name and anything containing a
separator. Only "." and ".." are path components; "..." is an ordinary
POSIX filename, and filepath.Base has already reduced "a/b" to "b", so
the separator test was dead on this platform and removed rather than
left looking load-bearing. Preservation controls now pin that
legitimate names survive.

Also removed: an unused id parameter on safeLocalFilename.

Claude-Session: https://claude.ai/code/session_01AUvLoXsKdS5sdpYju6rj4p

* test(server): only a DEFINE can rebind a name, and cover the idiomatic reassignment (BUG-2803)

Found by probing my own previous commit rather than by a review round -
the first time in this sequence I have caught the adjacent breakage
before the next round did.

The scope rules deleted a request name on ANY assignment whose right
side was not a request identifier. That is wrong in the dangerous
direction, and it fires on the most idiomatic line in Go HTTP code:

    r = r.WithContext(ctx)
    io.ReadAll(r.Body)      // <- invisible to the scan

WithContext is a call, so the name was dropped and every later read went
unseen. Measured before the fix: MISSED.

The correct rule is type-sound. Go is statically typed, so a plain
cannot change a variable's type: if it held a request before, it holds
one after. Only a DEFINE introduces a new binding that can be something
else. So the delete is now gated on token.DEFINE, which is both more
correct and simpler than what it replaces.

Three controls added, and the two that would have caught this are the
ones I had not written: a WithContext reassignment, and readers inside
an if body and a for body - the last two because making every nested
block its own scope is exactly the kind of change that could have
started missing them. Thirteen controls now, six negative.

Reverting to delete-on-any-assignment compiles and fails the
WithContext leg.

Claude-Session: https://claude.ai/code/session_01AUvLoXsKdS5sdpYju6rj4p

* fix: make the body-reader scan conservative by design, and reduce filenames cross-platform (BUG-2803)

Codex round 28. Two findings, and the first is the fifth consecutive
round to find a FALSE NEGATIVE in the same instrument.

## Stop modelling scopes; change the error direction instead

Rounds 24 through 28 each found another way the scope-modelling scan
missed a real reader: a value-copied request, a plain
r = r.WithContext(ctx), a mixed r, ok := ... that reuses an existing
variable, and if/for/switch initialisers and case clauses whose scopes
it did not model. Each fix closed one case and left another. That is a
design telling me something, not a run of bad luck.

The two error directions are not symmetric here. A false NEGATIVE hides
a body reader, which is the entire thing this test exists to prevent. A
false POSITIVE costs one human review and an accounting entry with a
reason attached. So the scanner now OVER-APPROXIMATES on purpose: any
name bound to an http.Request anywhere in the file counts for the whole
file, aliases are followed to a fixed point, and names are never
un-bound. Every scope-shaped false negative becomes structurally
impossible.

The cost is real and is now asserted rather than discovered: two
controls that previously expected "not flagged" - a name shadowed by a
closure parameter, and one rebound in a nested block - now assert
CONSERVATIVELY FLAGGED, so the bias is on the record. Three of round
28's named misses are added as controls and pass: mixed short
declaration, switch case, if-initialiser shadow. Sixteen controls, and
the accounting test still passes against the real package - so the
over-approximation costs nothing today.

Exactness needs go/types with a real package load, which is a bigger
instrument than this test warrants. The comment says so, and names the
signal that would justify building it: an accounted entry whose reason
is "the scan over-flagged".

## A filename safe on this OS is not safe on the consumer's

filepath.Base is platform-specific, so on Unix it leaves a backslash
alone - and the stored name is consumed cross-platform. A Windows client
joining a stored "..\evil.png" onto a directory traverses upward.

Reduced to the leaf under BOTH separator conventions. This normalises
rather than refuses, which is less lossy than replacing the whole name
and keeps round 27's point that a backslash is legitimate on Unix.
Removing the reduction compiles and fails the new test.

Claude-Session: https://claude.ai/code/session_01AUvLoXsKdS5sdpYju6rj4p

* test(server): count readers per accounted file, add two missing reader methods, correct a false reason (BUG-2803)

Codex round 29, and its central point was aimed at my REASONING, not my
code. It was right.

## The over-approximation argument was wrong for per-file accounting

I justified a deliberately conservative scanner by saying a false
positive costs one review and one accounting entry. That is not what it
costs. Once a file is listed, a NEW reader added to it is covered by the
existing entry and the test stays green - so a false positive does not
cost a review, it permanently blinds the list for that file. My own
comment already recorded that hazard two commits earlier, and I argued
past it anyway.

The fix is to make the entry carry a COUNT of reader expressions rather
than a yes/no. Adding a reader to an accounted file now changes the
number and fails, so the entry must be re-read and its reason
re-justified. It churns exactly when a body reader is added or removed,
which is when a human should look.

Demonstrated: inserting r.PostFormValue into handlers_tokens.go - an
already-accounted file - is FLAGGED. Before this it was absorbed
silently.

The conservative bias stays, because the false-negative classes it
eliminates are real and the count now removes the reason it was
expensive.

## Two real reader methods were missing

MultipartReader STREAMS the body and FormFile triggers multipart parsing
of it. Neither was in the selector list, and FormFile is used in
production in handlers_attachments.go - so the list was incomplete
against code that exists, not hypothetically.

## A reason in the list was simply false

handlers_tokens.go was accounted as "a nil/ContentLength check only - it
never reads the body". It guards on those and then calls decodeJSON. A
wrong reason is the same defect as a missing entry: both let a reader
pass as reviewed.

## And I guessed the counts

I wrote plausible numbers for the per-file counts and every one was
wrong; the test reported the real ones on its first run. Same habit this
branch keeps catching - a figure written from expectation reads exactly
like a figure that was counted. They are measured now and the comment
says so.

Also recorded: my first verification script failed to apply its mutation
and still printed a verdict, which I nearly banked. It now aborts unless
the mutation is present in the file.

Claude-Session: https://claude.ai/code/session_01AUvLoXsKdS5sdpYju6rj4p

* fix(attachments,cli): close the closing round's two view defects, correct two overclaiming comments (BUG-2803)

The closing enumeration (successor seat, per the lead's convergence ruling)
returned two real attachment-view defects and two comments claiming more than
their code delivers. Fixed here; the round's two design-scale findings are
filed instead (BUG-2820 scanner precision via go/types, BUG-2822
Windows-unstorable filename forms).

- Reverse MIME map: four ALLOWED spellings (text/xml, text/yaml,
  application/javascript, audio/webm) had no reverse extension because no
  extMIMEMap entry uses them as its value — `pad attachment view` wrote an
  extensionless temp file for exactly the types the delegation was built to
  fix. Population measured against the whole allowlist: these four, no more.
  An alias table closes them; TestEveryAllowedMIMEHasAnExtension asserts the
  class property over the allowlist (a future allowlist entry with no reverse
  extension fails), plus alias hygiene (allowed keys only, no forward-derived
  collisions, alias extensions must map to ALLOWED types so the table can
  never mint a refused extension). Mutation-verified: removing the alias
  application fails the test on all four types.

- safeLocalFilename: a trailing dot survived every check and
  filepath.Ext("photo.") is "." — non-empty — so the MIME-extension fallback
  never fired and the temp file dispatched on no extension. Trailing dots are
  now stripped (cannot empty the name; dots-only names already returned
  early). The CLI guard also gains its first direct tests, including the
  backslash and traversal refusals that previously rode untested.
  Mutation-verified: removing the TrimRight fails both trailing-dot cases.

- Two comment corrections, same defect class the accounting list itself
  names (a wrong reason reads as review): the handlers_cloud.go entry said
  bodyHasCloudSecret "restores" the body — it restores the first 64 KiB and
  drops the tail, a bound that file documents; and the accounting test's
  header said its scan "cannot be spelled around" while its own KNOWN LIMITS
  block lists the spellings that get around it (struct field, context value,
  type alias). The header now matches the limits block.

* fix(server,attachments,cli): close closing-round-2's scanner blind spot and four stale comments (BUG-2803)

Closing round 2 (successor seat) found no product defects; all four findings
were in instruments and comments. Each verified against the code, then fixed:

- The alias fixed-point resolved only identifier RHS (`req := r`), so a
  dereferenced copy (`c := *r; io.ReadAll(c.Body)`) was an invisible body
  reader — and unlike the disclosed type-level classes, this one was not in
  the KNOWN LIMITS block. The copy shares the Body (an interface holding the
  same reader). StarExpr operands now join the alias set; a new control pins
  the case. Mutation-verified: reverting the StarExpr handling fails the
  control. The type-level classes (struct field, context value, type alias)
  remain disclosed and are BUG-2820's territory.

- The KNOWN LIMITS block said shadowed request names are "correctly
  ignored" while the controls deliberately assert they are conservatively
  OVER-FLAGGED — stale prose from the scope-aware era, falsified by the
  round-28 conservative flip that never touched those lines.

- The reverse-map stability loop compared only PREFERRED entries across
  rebuilds; it now compares the entire map (sizes and every mapping) against
  the first build. Boundary stated honestly: every multi-spelling type today
  is preference-pinned, so the full-map comparison discriminates only when a
  future non-preferred multi-spelling entry appears — that future entry is
  what it guards.

- Three orphaned/wrong comments: a `mimeForExt` doc block glued above
  ExtensionForMIME (the function it described is gone); the old hardcoded
  extension-table doc glued above safeLocalFilename (falsified by the
  delegation it predates); and two "120 chars" claims where the cap is 120
  BYTES rune-safe via truncateBindableText — the consent form's
  maxlength=120 counts characters, so a multibyte name passes the client
  and is still truncated server-side, which is now what the comments say.

* fix(cli,server,docs): close the attachment-view path escape, and closing-round-3's instrument and prose findings (BUG-2803)

Closing round 3 found the branch's first product defect since round 17, in
BRANCH-ADJACENT code the round-24 fallback extension work made reachable: the
`pad attachment view` id fallback joined the RAW id onto its temp dir, and the
client sent the id into the URL path UNESCAPED. An id is a CLI argument, but
the documented agent flow harvests it from item content ("pad-attachment:"
refs other workspace members write), so a traversal-shaped "id" could
re-route the HEAD/GET to a different endpoint whose 200 then vouched for it,
and the write escaped the temp dir. Both halves fixed and both
mutation-verified through a new command-level test: reverting the fallback
sanitize demonstrably wrote OUTSIDE the sandboxed TMPDIR; reverting the
PathEscape put a raw "../../" on the recorded wire.

- internal/cli: url.PathEscape(attachmentID) at both id-bearing client sites
  (HeadAttachment, DownloadAttachment — the enumerated population).
- cmd/pad: the id fallback runs through safeLocalFilename, generic
  "attachment" when nothing survives; view's long help no longer claims the
  filename is used "without rewriting the extension" — it describes the
  reduction and the MIME-extension append, and says why the CLI is stricter
  than the server (the name is written to YOUR filesystem).
- cmd/pad: attachmentViewCmd gets its first command-level test (CONVE-19 —
  the helper tests vouched for the component, not its wiring): disposition
  name, extensionless+MIME append, id fallback, traversal containment with a
  wire-escaping control, generic fallback.

Instrument and prose findings, each verified before fixing:

- The KNOWN LIMITS disclosure now names the ordinary alias forms the
  fixed-point does not walk (var-spec, call-derived, named results, range
  bindings) — they were in BUG-2820's filing but not in the in-file
  disclosure, which is what let the round read them as unfiled. The scanner
  itself deliberately does NOT grow another parser patch; go/types is the
  filed fix.
- TestTextSafeHelpersAreUsedAtEveryCallSite pins EXACT occurrence counts
  (measured: 1 declaration + 4 call sites each) instead of a >=4 floor a
  removed call site could hide under.
- middleware_mcp_audit: two stacked comment copies rested non-forgeability
  on "real names do not begin with (" — the exact reasoning round 25
  retired; the const doc now points at auditLabel's namespace-reservation
  rule, which is what actually makes the marker non-forgeable.
- docs/backup.md said repair is needed before "the export or migration" goes
  through, contradicting its own "exports fine" three paragraphs up — it is
  the IMPORT or migration that fails; the export succeeds either way.

* fix(server,cli): decode chunked watch bodies, refuse dot-segment attachment ids, correct two texts (BUG-2803)

Closing round 4 found one PRE-EXISTING product defect and one residue of the
round-3 fix, plus two wrong texts. Each verified before fixing:

- Watch creation gated its body decode on `ContentLength > 0`, so a CHUNKED
  request (ContentLength == -1) had its body silently DROPPED — the caller's
  predicate ignored, an unconditional watch created, 200 returned. The
  population of ContentLength gates in the package is exactly two:
  handlers_tokens.go already used the `!= 0` form, watches now matches it,
  with io.EOF tolerated so the documented no-body-is-valid contract holds
  for an empty chunked body too. Three handler-level tests discriminate the
  cases; the mutation (condition back to `> 0`) fails the two it should and
  passes the empty-body control. The accounting instrument then flagged the
  new `r.Body != nil` reference in the file — its exact job — and the file
  is now accounted with a measured reader count of 1.

- url.PathEscape leaves exact "." and ".." UNCHANGED, so those two ids still
  reached the wire as live dot segments for a proxy or server to normalize —
  the escaping added in round 3 did not cover them. Both id-bearing client
  sites now share attachmentIDPathSegment, which refuses exactly those two
  values before any request (a real id is a UUID; the refusal cannot fire on
  one). Mutation-verified: removing the refusal fails the new subtest, which
  also asserts zero requests reach a recording stub.

- The artifact rejection text said "NUL byte"; the same refusal fires for a
  NUL manufactured by a YAML escape during parsing, where no raw NUL byte
  exists — now "NUL character", in the handler message and the error var.

- A test comment claimed the User-Agent reaches sessions.user_agent as
  text; sessions store only ua_hash, as the accounting list's own exemption
  states two hundred lines up. The sentence now agrees with it.

* fix(attachments,server): remove a can't-fire MIME preference and a stale filename-guard sentence (BUG-2803)

Closing round 5 is down to two P3 comment defects; both verified and fixed:

- preferredExtensions "preferred" .md over a .markdown that has never been
  in the forward map — a line that cannot fire, the exact class this
  branch's own instruments hunt (the alphanumeric guard, the text/yaml
  preference, the charset loop). Entry removed; shortest-wins picks .md as
  the only candidate, unchanged. The preference-hygiene test now asserts
  every entry has a real competitor (>= 2 forward-map spellings), and the
  counterfactual — re-adding the entry — fails it.

- The upload filename guard still carried round 26's "checking the trimmed
  form rather than listing spellings" sentence directly above round 27's
  code that does the opposite (exact "." / ".." comparisons, longer dot
  runs deliberately preserved). The stale layer is gone; the surviving
  paragraph already records why.

* fix(server,docs): drop a dead test fixture, stop claiming the failing row is named (BUG-2803)

Closing round 6 returned one P3 — TestDecodeJSONTrimsOnlyJSONWhitespace
booted a full testServer it never used (`_ = srv`), dressing a direct
decodeJSON test in router coverage it does not have. Removed.

Its enumeration also re-read docs/backup.md against the code: "the failing
row is named in the error" is true of neither leg — the import answers 400
naming the RULE it refused on (the NUL check is body-wide and knows no row),
and `pad db migrate-to-pg` reports which WORKSPACE's copy failed. The doc
now says exactly that, and that locating the value is manual until
BUG-2810's preflight lands.

* fix(server,cli): retire a stale byte-search claim, close two instrument gaps from closing round 7 (BUG-2803)

Round 7 found no production defects; three instrument/comment findings:

- artifactIsBindableText's doc comment still asserted the round-8 byte-search
  approach and that "the ambiguity cannot arise here" — directly above the
  round-9 body comment recording that assertion as simply wrong and doing the
  round-trip walk instead. The doc paragraph now describes the round trip
  and points at the body's history.

- TestBodyReaderScanDiscriminates listed MultipartReader and FormFile in the
  scanner's selector set but had no control for either, so their removal
  from that list was undetectable. Two controls added.

- The attachment-view test proved nothing about the MIME delegation: every
  case used image/png, which the OLD hand-rolled table also knew, so a stale
  local table passed. Two cases added — application/gzip (a type round 26
  found missing from that table) must gain .gz, and blocked image/svg+xml
  must gain nothing. Mutation-verified: a stale-table mutant that answers
  only for png fails the gzip case. (First mutant attempt didn't build —
  unused import — and was not counted as a detection.)

The per-file same-count substitution gap round 7 restated is declined as
filed, not fixed: BUG-2820's filing already specifies per-call-site
accounting via go/types as the fix that retires the per-file count
workaround; the KNOWN LIMITS closing line now carries that ref.

* fix(attachments,server): sweep two pre-BUG-2413 disposition comments, pin the manifest refusal to 400 (BUG-2803)

Closing round 8 found no production defects; two evidence findings, verified
then fixed:

- Two comments still described the PRE-BUG-2413 disposition policy: the
  RenderChip mode doc said the HTTP layer serves every chip inline, and the
  read-path doc derived Content-Disposition from RenderMode. The live policy
  is the explicit fail-closed ServeInline allowlist — most chip types are
  served as "attachment". Both now say so and record the history.

- TestImportBundle_RefusesNULInManifest accepted any status >= 400, so the
  documented 400 could decay into a 500 unnoticed. Pinned to
  http.StatusBadRequest.

2026-08-30 21:30:19 -04:00

..

loadtest-collab

fix(loadtest): codex catch-up review fixes (TASK-1270) (#470)

2026-05-09 16:41:07 -04:00

pad

fix(server): refuse a decoded NUL in a JSON request body (BUG-2803) (#1220)

2026-08-30 21:30:19 -04:00