1625 Commits

This Branch

This Branch

All Branches

Author	SHA1	Message	Date
xarmian	63da2f4f5f	feat(store,server,cli): count and repair the legacy NUL population (BUG-2810) ... Layers A and B stop the value being written. Neither makes a row that already carries one go away, and BUG-2810's filing is what that costs: an affected workspace exports with a 200 and re-imports with a 400, so a self-hoster restoring their own backup is blocked with no path forward in the product, and `pad db migrate-to-pg` fails partway through the copy against PostgreSQL's jsonb parser rather than up front. This is DOC-2823's S3, on Dave's day-54 rulings: U+FFFD as the replacement, repair standalone only with a migrate-to-pg preflight that refuses and prints the command, `--repair-nul` on import shipping default-strict. ONE REPAIR, beside the one predicate. textguard.Repair lives next to ParameterRefused because four layers that agree about what is REFUSED and disagree about what a repair PRODUCES is this bug family arriving one step later. Its contract is a property over the same corpus, in both directions: every refused value becomes one all four layers accept, and every accepted value comes back IDENTICAL. The second half is the load-bearing one — a repair that tidies values nobody complained about rewrites `{"a":"x\\u0000y"}`, six literal characters after a doubled backslash, and corrupts it. The JSON arm is a string-literal SCANNER, not decode-walk-remarshal, which is what the recon write-up proposed before it was written. Re-marshalling changes four things nobody asked to change — object key order, insignificant whitespace, integers wider than float64, HTML-ish characters — and silently drops one of a document's LITERAL duplicate keys, which is a gap BUG-2812 owns and the last thing a repair should do. Scanning copies every byte it does not deliberately rewrite, so an untouched document is byte-identical without that having to be argued. A substring replace is not equivalent and the test that proves it took a mutation to find: a doubled-backslash literal ALONE never reaches the scanner, so the discriminating fixture is one document carrying a live escape AND a literal. THE COUNT IS COMPUTED IN GO. Measured on the read path in this worktree: a row planted with `bad<NUL>name` reads back into a Go string with all 8 bytes and the NUL intact, while `length(name)` in the same database answers 3. TASK-2824 found that C-truncation and concluded no DB-side REPAIR could be trusted; the same measurement on the read path says no DB-side COUNT can be either. SQL narrows — `instr(col, char(0))`, plus the escape prefix on JSON-classed columns, which is textguard's own pre-filter — and never decides. The decision stays ParameterRefused with isJSON from the shared 86-column list, i.e. Layer B's classing. Row addressing is read from the live schema rather than a hand-kept map: 39 tables carry protected columns, one (item_wiki_links) declares no primary key and is addressed by rowid, two have composite keys, and five have a single key that is not `id`. The repair checks RowsAffected because an address that stopped selecting its row would otherwise commit an UPDATE that touched nothing and report it as repaired — the one failure an operator cannot see in the output. `email_optouts(email)` is both a protected column and its own primary key. Repairing it changes the row's identity and can collide with an existing row, which in that table means somebody starts receiving mail again. It is reported and skipped, with the reason. The import flag is NOT an exemption from the gate. `--repair-nul` buys the body one repair attempt and then runs the same `bodyDecodesNUL` on the repaired bytes, which still decides — a decode path that skipped the check is the door BUG-2803 spent thirty rounds closing, on the endpoint carrying the largest attacker-controlled body in the product. Only the ESCAPE form is repaired: a raw NUL byte makes the document invalid JSON, and widening what parses is not this flag's job. Both doors are covered, JSON and tar.gz, because giving them different answers is how one of them keeps being forgotten. Postgres is settled with evidence rather than sent up as a ruling: it cannot hold either defect (22021, 22P05) and the four-way differential test already pins that, so the scan reports not-applicable WITH the reason rather than returning a zero a reader could mistake for a clean database. Spellings settled here, per the dispatch: `pad db scan-nul` and `pad db repair-nul` as siblings rather than `repair --nul`, matching `migrate-to-pg`'s hyphenated compound — a repair verb that errors when given no flag is a worse shape, and there is no second repair to share it with. scan-nul IS the dry run, so repair-nul grows no --dry-run. It refuses while the server is running unless --force, on the `pad db restore` precedent: the report is a claim about a database, and one somebody else is concurrently writing makes it a claim about a moment that has passed. docs/backup.md's section on this is rewritten. It still said the rule lives in the binary and not the database, which S2 made false, and it pointed at this item for a preflight and a repair that now exist. Its import examples also showed `pad workspace import < file`, which has never worked — the file is an argument. Closes BUG-2810.	2026-09-02 14:53:31 +00:00
xarmian	ebd1886ada	Merge pull request #1232 from PerpetualSoftware/fix/bug-2827-outbox-bound ... fix(store,server): bound the event outbox's writes, claims and scrub (BUG-2827)	2026-09-02 09:33:44 -04:00
xarmian	2dde493305	chore: ignore the Go build cache the codex sandbox leaves in the checkout ... codex exec's sandbox runs go with GOCACHE inside the working directory, at .tmp-gocache/. On this branch a git add -A after a review round swept 5,688 of those files into a commit (created 01:44Z during round 4, committed 02:16Z); the two affected commits were rewritten without them before the PR. Ignoring the directory means the next seat's add -A cannot repeat it.	2026-09-02 12:33:08 +00:00
xarmian	f54a0e41d4	docs(store,server): four comments and one log line that had stopped being true (BUG-2827) ... Codex round 6. No logic finding; it confirmed the refusal ordering, the split-budget claim and the first-tick scan as sound. Five statements in the branch's own prose were untrue of the code as it stands: - MaxOutboxPayloadBytes' comment still argued from 64 MiB ("two orders below both ceilings") after the constant became 128 MiB, which is 4x under the lowest ceiling, not two orders. - maxOutboxClaimBytes' comment counted a scan-into-string-then-copy transient that round 1 removed; the scan lands straight in []byte. - maxOutboxClaimRows' comment used the item BODY mean (~2.4 KB) as the payload mean; the measured payload mean is ~3.5 KB, so 5,000 rows is ~17 MiB, not ~12. - emitBulkItemEventTx's early-out said the numbers the caller sees still come from writeOutboxTx; when the early-out fires they come from the projection, and the error says so in Measured. - The drain's oversized-row log said "not claimed". OversizedPendingOutbox filters only dispatched_at, so during a rolling upgrade a binary older than the ceiling may be holding a claim on the row it names. The line now states what the query establishes: this instance will not claim it. The doc says why claimed_at is deliberately not a filter.	2026-09-02 12:33:08 +00:00
xarmian	0836808ca5	fix(store): drop a past-the-hop-bound event before judging its size, and correct three comments (BUG-2827) ... Codex round 5. No production defect found; one ordering edge and three comments that had stopped being true. THE ORDERING. writeOutboxTx has two refusals that disagree about the mutation. The hop bound drops the event and lets the mutation stand, because only the cascade it would extend is illegitimate. The size cap fails the mutation, because there the mutation and the event are the same fact. An event that trips BOTH was judged for size first, so it failed a mutation over a row that was never going to be written. The hop drop now comes first. Unreachable today - nothing propagates a hop - which is exactly why the ordering is worth pinning before something does. TestAnOversizedEventPastTheHopBoundIsDroppedNotRefused fails with the two checks swapped back (run before the crash that interrupted this round, and again on this tree). THREE COMMENTS. The drain-limit constant said whole batches are claimed past it; the byte budget and row cap can now split one. The claim candidates' doc said every sibling; it is as many as the budget still allows. OversizedPendingOutbox's doc named the write cap while its query uses the claim ceiling, and said it ran every tick when the caller throttles it to once per five minutes. Gates: gofmt clean, golangci-lint 0 issues, full SQLite suite exit 0, full Postgres suite exit 0.	2026-09-02 12:06:02 +00:00
xarmian	57caa7f92e	docs,test(store): correct five comments and strengthen the shrink fixture (BUG-2827) ... Codex round 4. Its one P1 does not hold, but the test it named as weak genuinely was, and five comments in this branch had drifted from the code they describe. THE P1, CHECKED RATHER THAN ARGUED. The claim measures size at candidate selection and never rechecks it, so a payload that GREW during a concurrent scrub could be claimed over the ceiling. The proposed growth path was Go's HTML escaping: json.Marshal writes < as its six-character unicode escape, where the source had one byte. Measured against Postgres 16: a one-key object whose value holds the four characters x<y>z&w, written with those characters LITERAL -> 16 bytes the same object written with < > and & as their six-character JSON unicode escapes instead -> 16 bytes Postgres parses the escapes and stores the characters, so the escaped and literal forms are the same size and the round trip cannot grow the row. On SQLite the payload is stored exactly as Go wrote it, so re-marshalling is idempotent. The rejection from round 3 stands, now on a measurement instead of an assertion about key removal. But the test defending it was weak, and codex was right about that: its fixture was one repeated ASCII letter, which cannot tell any of these encoder paths apart. It now carries <, >, & and non-ASCII, so it exercises the divergence rather than asserting past it. Mutation note worth keeping: a whitespace-padding mutant is caught on SQLite and NOT on Postgres, because jsonb discards insignificant whitespace — the mutant does not actually grow the stored row there. The faithful mutant adds a key, and that one dies on both. FIVE COMMENTS THAT SAID SOMETHING UNTRUE, all introduced by this branch: - OversizedOutboxPayloadError was documented as the write cap's error; three sites raise it, against two different limits. - "The two things Measured can name" listed three. - measuredStoredRow said "as the database stored it", but OctetLength measures what the driver hands back, which on Postgres is the ::text rendering rather than storage. - OversizedPendingOutbox described its threshold as the write cap while querying the claim ceiling. - ClaimPendingOutboxEvents still said batches are claimed whole, which the byte budget and row cap deliberately interrupt. Tests also now assert Measured at all three refusal sites — without it the field could be blank everywhere and every existing assertion still passes — and the claimability property test's refusal branch checks the row actually rolled back, so "refused" cannot be satisfied by a write that committed anyway. Gates: gofmt clean, golangci-lint 0 issues, full SQLite suite exit 0, full Postgres suite exit 0. Claude-Session: https://claude.ai/code/session_01XLtX4dbjBpApbAv3SuBcTm	2026-09-02 12:05:57 +00:00
xarmian	213142c4b5	fix(store,server): honest refusal figures and a throttled oversized scan (BUG-2827) ... Codex round 3. Two of three findings acted on, one rejected with an invariant test in place of the change it asked for. REFUSAL FIGURES SAID SOMETHING FALSE. The store refuses on two different measurements against two different limits — the member content before marshalling, against the write cap, and the row exactly as stored, against the claim ceiling — and both reported the number as "a %d-byte payload". For the first that is untrue: projectedBulkPayloadBytes is explicitly a lower bound, so the message named a size the payload did not have. A caller seeing two different numbers for one mutation had no way to reconcile them. OversizedOutboxPayloadError now carries what it measured, and both the error and the 413 say so. THE DIAGNOSTIC WAS THE MOST EXPENSIVE THING THE DRAIN DID, and it was most expensive when it found nothing. OversizedPendingOutbox has a non-sargable size predicate and no index to help it, so an empty result means evaluating octet_length over every pending row — on Postgres, detoasting and serializing each JSONB payload — and it ran every 5s tick. Now throttled to once every 5 minutes. Latency is the cheap thing to spend here: the rows it reports are permanently unclaimable and sit until the 7-day retention takes them, so a five-minute alarm delay changes no decision anyone makes about them. The first tick after a restart still scans, so an existing oversized row is reported promptly. REJECTED: that the claim needs to revalidate size, because a concurrent scrub could grow a payload between candidate selection and the claim UPDATE. It cannot. scrubOutboxRowTx is the only UPDATE of payload in the tree and it removes keys and re-marshals compactly, so a rewrite is strictly smaller — and shrinking is harmless, since a row judged claimable stays claimable. That is load-bearing for the claim needing no revalidation, so it is now stated at the function and pinned by TestScrubOnlyEverShrinksAPayload rather than left as an assumption for the next person adding a payload rewrite to break silently. The throttle test found its own gap on the way in. Written first against the helper, it stayed green when the call site was mutated to `if true` — a helper nothing calls is still correct in isolation. TestOutboxDrainTickConsultsTheThrottle covers the call site through the stamp the tick leaves behind. Also caught by the gate rather than by review: the field carrying the throttle clock landed on outboxDrainSettings as well as outboxDrainConfig, because the edit matched a line both structs have. Tests passed with both; lint named the dead one. Gates: gofmt clean, golangci-lint 0 issues, full SQLite suite exit 0, full Postgres suite exit 0. Mutants: growing the written payload kills TestScrubOnlyEverShrinksAPayload, removing the throttle call site kills TestOutboxDrainTickConsultsTheThrottle. One mutant discarded as unfaithful — corrupting the payload BEFORE the compare-and-swap is neutralised by the retry, which re-reads and redoes the work correctly, so it tests the retry rather than the invariant. Claude-Session: https://claude.ai/code/session_01XLtX4dbjBpApbAv3SuBcTm	2026-09-02 01:41:47 +00:00
xarmian	46a551aa4b	fix(store): refuse an outbox row on its STORED size, not its Go size (BUG-2827) ... Codex round 2. One material defect, and the measurement that settled it invalidates an argument the previous commit leaned on. I had claimed the Postgres JSONB text expansion was bounded near 1.4x — whitespace after colons and commas — which is why a 2x claim ceiling was said to guarantee that anything writable is claimable. That is wrong. Postgres reparses JSON numbers as `numeric` and prints them positionally, so the expansion has no ceiling at all. Measured against Postgres 16: {"a":1,"b":2} 13 bytes -> 16 (whitespace only, ~1.2x) {"a":1e-100} 12 bytes -> 109 (~9x) {"a":1e-3000} 13 bytes -> 3009 (~231x, exponent free to grow) So no multiple of the write cap is a safe claim ceiling, and the Go-side cap does not bound the stored row at all. Reachable rather than theoretical: item payloads carry `fields` as a JSON *string*, whose contents are escaped text and immune, but a bulk delta is a map[string]any and a numeric field value from a request body arrives as a float64 that re-marshals in exponent form. The failure it produced was a row accepted by the write and then excluded from every claim for the rest of its retention window — written, undeliverable, visible only as an oversized-row log line. Fixed where the number is actually true: the INSERT now RETURNs octet_length of the stored payload and refuses against the claim ceiling, rolling the caller's transaction back exactly as the pre-marshal check does. "A row this binary wrote is a row this binary can read back" is now established by construction instead of inferred from an expansion argument that did not hold. MaxOutboxClaimableBytes keeps its 2x value but loses its false justification: its job is only to leave ordinary payloads room above the write cap so the two rules do not fight over rounding. Test gaps from the same round, all three closed: - The claim-ceiling invariant was pinned only by a Postgres round-trip, so a ceiling collapsed back to the write cap passed every default (SQLite) run. The constants test now asserts the relation directly and fails on either dialect. - TestBatchSiblingQueryIsBoundedInSQL drives claimableBatchSiblings directly: bounding the batch in the caller instead of in SQL passed every assertion on the claim's return value while keeping exactly the unbounded allocation the row cap was added to remove. - The scrub's byte budget changes peak memory and nothing else, so removing it left every outcome assertion green. A TEST-ONLY afterOutboxScrubBatch seam makes batch count observable, which is the one visible consequence of the budget working. Codex also confirmed the previous round's rejected finding: keyset paging covers every row present at the initial scan and additionally catches later commits sorting above the cursor, so it is a superset of the unbatched behaviour rather than a regression. Mutation matrix, run on BOTH dialects: dropping the stored-size refusal kills TestEverythingWrittenIsClaimable on Postgres only (correctly — it is a Postgres defect); dropping the sibling SQL LIMIT and collapsing the claim ceiling kill their tests on both; dropping the scrub byte break kills TestScrubSpendsItsByteBudgetNotJustItsRowLimit. Gates: gofmt clean, golangci-lint 0 issues, full SQLite suite exit 0, full Postgres suite exit 0. Claude-Session: https://claude.ai/code/session_01XLtX4dbjBpApbAv3SuBcTm	2026-09-02 00:53:29 +00:00
xarmian	37f26f5430	fix(store): close the outbox bound's remaining unbounded paths (BUG-2827) ... Codex review of the previous commit. Four real defects, one finding answered rather than acted on, and one test that did not discriminate. ROW CAP ON THE CLAIM. A budget in bytes alone does not bound the per-row cost, and the batch scan is the path that shows it: siblings are collected past the row limit by design, so a batch of a million tiny rows sits comfortably inside 64 MiB while its ids, maps, OutboxEvent structs and folded delivery do not. maxOutboxClaimRows (5,000) binds only that shape — at the measured ~2.4 KB payload mean it is ~12 MiB, well inside the byte budget, which stays operative for real traffic. REFUSE BEFORE MARSHALLING. writeOutboxTx could only see the payload after json.Marshal had built it, so a member set large enough to be refused was large enough that rendering it to be refused was its own memory event. emitBulkItemEventTx now charges the members' own bytes first. It is an early-out, not a second rule: JSON only adds, so the projection is a lower bound and can never refuse something the real check would accept, and the authoritative numbers still come from writeOutboxTx. Pinned by asserting the reported size equals the projection exactly — any weaker assertion passes against the code this guards, because the late check refuses with the same error type. ONE COPY, NOT TWO. outboxEventsClaimedBy scanned each payload into a string and then converted it to []byte. On the largest single row a pass may take, that was the difference between one copy and two. A COMMENT THAT WAS FALSE. ScrubOutboxUserRefsTx's note called its subject_id arm indexed. Migrations 081/082/083 index (occurred_at,id), dispatched_at, (workspace_id,occurred_at), batch_id and (claimed_at,occurred_at) — nothing on subject_id. Corrected to say the scan it actually is. CONSIDERED AND REJECTED: that keyset paging over random uuids can miss rows. It cannot miss a row that existed when the scan began — ids are fixed, the walk is ascending over every matching row above the cursor, and a row stops matching only once scrubbed. It changes concurrent commits in the SAFE direction: the single query missed everything committed after it, while this catches those sorting above the cursor, so coverage is a superset of the unbatched version. Snapshotting all ids first would make the window describable without reference to uuid order, and was tried and reverted: it holds every matching id at once, which is an unbounded allocation of the same shape this change removes. The reasoning is now in the comment so the next reader does not re-derive it. Also accepted as residuals, both documented at their constants: a row taken alone because it exceeds a whole pass's budget still costs that pass its size, and the scrub reads one oversized legacy payload whole. Both are the price of delivering and erasing data that exists; refusing either is data loss rather than a bound. Gates on this tree: gofmt clean, golangci-lint 0 issues, full SQLite suite exit 0, full Postgres suite exit 0. Mutation matrix re-run for the new guards: dropping the row cap kills TestOutboxClaimStopsAtTheRowCap, dropping the early-out kills TestBulkEventIsRefusedBeforeItIsMarshalled, dropping the scrub cursor advance still kills TestScrubOutboxUserRefsTerminatesOnLikeFalsePositives. Claude-Session: https://claude.ai/code/session_01XLtX4dbjBpApbAv3SuBcTm	2026-09-02 00:11:06 +00:00
xarmian	5b12d5eeeb	fix(store,server): bound the outbox's unbounded reads and writes (BUG-2827) ... item.bulk_updated marshals every cascaded member body into ONE event_outbox row, and nothing bounded either the row or the drain's reading of it. The v1 doc called the size deliberately unbounded and named a follow-up condition; measurement met it. MEASURED FIRST, against an 8,434-item instance: widest wiki-title cascade 23 members / 175.6 KiB of bodies widest wiki-ref cascade 50 members / 365.5 KiB worst option rename 4,429 members / 7.34 MiB (tasks.status="done") marshal ratio ~1.46x (item.updated payload mean 3,518 B against an item body mean of 2,416 B) So renaming one status option on that instance emits ~11 MiB in a single row today, from a user clicking rename in the collection editor. That kills the obvious design: a write cap tight enough to bound the drain's memory refuses routine work, and one loose enough for routine work bounds nothing. The two therefore became two numbers. WRITE CAP (128 MiB) in writeOutboxTx, the single INSERT INTO event_outbox in the tree, so all seven emit paths inherit it rather than an enumerated site list. Refusing FAILS the mutation: the hop bound in the same function drops the event and keeps the mutation, correctly, because there the cascade is what is illegitimate, while here the mutation and the event are the same fact. 128 rather than 64 MiB so it clears MaxItemRenameCascadeBytes — a cascade squeaking under that 64 MiB bound marshals to ~96 MiB, and a 64 MiB cap here would let the vaguer refusal preempt rename_cascade_too_large on the very renames that bound describes. Surfaces as 413 event_payload_too_large, following that precedent rather than inventing a second spelling. CLAIM BUDGET (64 MiB per pass), spent by the primary candidates AND by batch siblings. Spending it on siblings knowingly relaxes "batches are claimed whole": read literally that rule makes the budget bypassable by construction, since one large batch is an unbounded read no row limit touches, and groupOutboxDeliveries already defines the split. The first candidate is always taken whatever it costs, so the bound meant to keep the drain alive cannot starve a row instead. CLAIM CEILING (2x the write cap), and it is NOT the write cap — a distinction the Postgres leg had to teach. The cap measures the Go bytes json.Marshal produced; the claim measures what the driver hands to Scan, which on Postgres is the JSONB ::text rendering, one space inserted after every colon and comma. A 40,000-byte payload reads back as 40,001. Thresholded at the same number, a payload written at exactly the cap was admitted by the guard and then permanently excluded by the claim: delivered to nobody, reported as nothing, reaped seven days later. TestARowWrittenAtTheCapIsStillClaimable fails on Postgres and passes on SQLite against the 1x version. Rows above the ceiling are excluded IN THE PREDICATE, not filtered in Go — filtering in Go leaves them occupying candidate slots and starves everything behind them, which is the same jam wearing different clothes. They are logged every tick and left pending for the existing 7-day undispatched retention, not stamped dispatched_at, which would record that an event went out when it did not. SCRUB. ScrubOutboxUserRefsTx collected every LIKE-matching payload at once — the same unbounded read through a different door. Now batched by a keyset cursor on id. READ FULLY THEN WRITE is preserved PER BATCH, and the cursor keeps the per-row UPDATEs in ascending id order across batches, so batching does not quietly reintroduce the BUG-2409 deadlock it was written to avoid. Prose sweep: MaxItemRenameCascadeBytes' comment asserted this vector had no bound, and emitBulkItemEventTx's asserted the payload was unbounded by decision. Both now say what is true, the second keeping its original reasoning because it still explains what the bound does NOT do. Mutation matrix, each mutant compiled and run unfiltered: removing the row-cap check, > to >=, removing the budget break, removing always-take-one, removing the SQL size exclusion, removing the sibling budget, removing the scrub cursor advance, and 2x to 1x — all die, the last on Postgres only. One survivor recorded as unfaithful rather than as a gap: setting the scrub cursor to the first row read still advances monotonically over an ascending id > cursor query, so termination holds and only the pass count degrades. Not addressed, named rather than left to be found: a workspace that outgrows the write cap cannot shrink its own cascade, so the refusal leaves no recourse. The answer is chunking one bulk event across rows sharing a batch_id, which the drain's fold already supports. Claude-Session: https://claude.ai/code/session_01XLtX4dbjBpApbAv3SuBcTm	2026-09-01 23:15:05 +00:00
xarmian	ebe40de932	feat(store): add a dialect accessor for a column's scanned byte length ... Dialect.OctetLength renders the byte length of a text-ish column as the driver will hand it to Scan. The "as Scan will see it" part is the whole reason this is a dialect method rather than a literal in one query. event_outbox.payload is TEXT on SQLite and JSONB on Postgres; octet_length has no jsonb overload, so Postgres needs a ::text cast, and that cast renders the PARSED value — whitespace normalized, keys reordered, duplicates collapsed — which is not the byte count that was written. Every caller of this is deciding whether it can afford to Scan a value, so the scanned count is the one they need and the stored one would quietly mislead. No behaviour change on its own; BUG-2827 is the first consumer. Claude-Session: https://claude.ai/code/session_01XLtX4dbjBpApbAv3SuBcTm	2026-09-01 23:14:40 +00:00
xarmian	6767ac7210	Merge pull request #1231 from PerpetualSoftware/feat/s2-nul-layer-b ... feat(store): make the NUL invariant a property of the database (DOC-2823 S2)	2026-09-01 17:56:57 -04:00
xarmian	978af702f7	fix(store): sweep by pattern rather than a name list, and quote the dropped identifier ... My round-6 "class sweep" was a hand-written list of column NAMES, which is an enumeration one level up — and round 7 found exactly the two names nobody thought to write down: event_outbox.claimed_by and oauth_connection_workspaces.added_by. Five rounds have now each named unprotected columns. So the sweep is a PATTERN (*_by, plus actor/author/source/owner) and it is a TEST, which is the difference between having done it once and it staying done. A new *_by column fails the day it is added, with a message saying which side of the decision it needs. It carries its own instrument check: if the pattern matches nothing at all it fails as broken rather than passing as clean. 262 triggers over 131 columns. Second finding, and it would have been a startup failure rather than a silent one: trigger names read from sqlite_master were interpolated into DROP TRIGGER unquoted. A legal identifier matching the GLOB — one containing a hyphen — produces a syntax error, which fails the restoration, which migrate propagates, which fails startup. The one place a stray trigger is most likely to have an unusual name is the one place this code has to survive it. Quoted, with the embedded-quote case escaped. Full Go suite green on SQLite and Postgres 17; lint 0 issues. Claude-Session: https://claude.ai/code/session_01XLtX4dbjBpApbAv3SuBcTm	2026-09-01 21:16:17 +00:00
xarmian	5d2ae30ca2	fix(store): sweep the attribution-column class, and treat a stray trigger as unhealthy ... Codex named eight unprotected attribution columns on documents, versions and comments. Rounds 2, 3 and 6 have now each named one or two of these, which is the signal that the reviewer is sampling a POPULATION rather than finding instances — CONVE-18's whole point. So I enumerated the class: every created_by / last_modified_by / source / actor / author / *_by column in the schema. SIXTEEN were unprotected, against the eight named. All sixteen are in, including the ones that are server-set today (granted_by, invited_by, uploaded_by), on Ruling 2's posture for the second ring: an instr trigger is near-free, and litigating each one is how the last three rounds went. 258 triggers over 129 columns. Second finding, also real: the health check asked only whether every EXPECTED trigger was present, so a database carrying an EXTRA pad_nul_ trigger read as healthy. That is not cosmetic — a stray left by a partial restore or a manual edit can abort legitimate writes, and the check meant to notice would report the database fine forever. Extras now count as unhealthy, and the existing drop-then-recreate removes them. The test installs a stray that refuses everything, proves a legitimate write is broken by it, and then measures the repair. Full Go suite green on SQLite and Postgres 17; lint 0 issues. Claude-Session: https://claude.ai/code/session_01XLtX4dbjBpApbAv3SuBcTm	2026-09-01 21:16:17 +00:00
xarmian	214b888811	fix(store): normalize IF NOT EXISTS, and make the restoration report its work ... [P1] The definition comparison NEVER matched, so every startup dropped and recreated all 226 triggers under an immediate write lock. SQLite stores "CREATE TRIGGER" while the generator renders "CREATE TRIGGER IF NOT EXISTS", and the comparison was verbatim. Normalized now. [P1] A schema_migrations query ERROR was folded in with "not applied" behind a nolint:nilerr, so a migrated database with a read failure would start successfully with the invariant unenforced — enforcement silently absent in exactly the case the check could not run. Only the table being ABSENT means "earlier than that migration"; a query failure propagates. THE PART WORTH KEEPING is how long the first one hid. Its no-op test asserted the trigger COUNT was unchanged — true of a full drop-and-recreate. Told that, I asserted sqlite_master ROWIDs instead, and the mutation STILL survived: SQLite reuses rowids when the drops and creates happen in one transaction in the same order. Two observables, both about whether the state LOOKS the same afterwards, when the question was whether anything was rebuilt. So the operation reports what it did. ensureNULTriggersReporting returns whether it restored, and the test asserts on that — an observable that cannot be satisfied by doing the work and leaving things looking tidy. The query-error case is forced by renaming schema_migrations' version column, with a control on the healthy database first so the failure is the corruption and not the fixture. Both fixes are mutation-verified. Full Go suite green on SQLite and Postgres 17; lint 0 issues. Claude-Session: https://claude.ai/code/session_01XLtX4dbjBpApbAv3SuBcTm	2026-09-01 21:16:17 +00:00
xarmian	5872647389	fix(store): restore triggers after every migration, gated on its own ... Codex round 4's one correctness finding: the restoration ran once, AFTER the whole migration chain and the FTS validation. A table-rebuild migration drops that table's triggers, so the invariant was unenforced for it across every remaining migration — and a concurrent raw writer (an old binary on the same file, the population Layer B exists for) could commit a violating row in that gap. It runs after EVERY migration now, narrowing the window to the moment between one migration's commit and the next statement. That fix immediately broke a fresh install, which the suite caught at once: on the early migrations the tables do not exist yet, so the trigger SQL failed with "no such table". The restoration is gated on its own migration having been applied — before that there is nothing to restore, and absence is the answer rather than an error. THE RESIDUAL IS STATED, not papered over. The DROP lives in a rebuild migration's own transaction and the recreate in another, so the window cannot be closed from here — only narrowed. Making an EXISTING violating row go away is S3's repair sweep, which is needed regardless: rows written before S2 shipped are the same problem arriving by a different route. The code says so at the point where a reader would otherwise assume the restoration is total. Full Go suite green on SQLite and Postgres 17; lint 0 issues. Claude-Session: https://claude.ai/code/session_01XLtX4dbjBpApbAv3SuBcTm	2026-09-01 21:16:17 +00:00
xarmian	12ea3f4d9a	fix(store): compare trigger DEFINITIONS, and inspect inside the transaction ... [High] The restoration had a window BEFORE the one round 2 closed. Round 2 put the CREATE statements in a transaction; round 3 found that the "are any missing?" check still ran outside it, so a raw writer could commit an invalid row between the check and the lock. The transaction is opened first now — the DSN carries _txlock=immediate, so Begin takes the write lock — and the inspection runs through it. [High] The check compared trigger NAMES, which can never repair a stale one: CREATE TRIGGER IF NOT EXISTS sees a same-name no-op body and does nothing, forever. It compares DEFINITIONS now, against the same rendered text the migration is generated from, and drops before recreating. A mutation reverting it survived until the stale-trigger test existed — the test installs a no-op trigger with the right name, proves the COUNT is correct and protection is gone, then measures the repair. [High] Two more caller-controlled columns. views.view_type is `viewType := input.ViewType` with "list" only as a fallback, on create and update. item_links.link_type is normalized on the ordinary create path and written VERBATIM by ImportWorkspace — the second-write-path shape for the third time in this unit, and the second time it was the import that had it. [Medium] The classifier test grepped nulguard.go for the classifier's name, so a dead or commented call would satisfy it. There is a fake driver now that returns a marker-bearing error from each of the four entry points, and the test asserts the caller receives the TYPED error — routing exercised rather than read. Two mutations, one per entry-point shape, confirm it discriminates. [Low] Two refusal assertions accepted ANY error. items.fields also carries migration 056's JSON constraint, so "some error" was satisfiable by a value that never reached a trigger; both assert the marker now. The disjointness assertion added in round 2 earned itself immediately: it caught both newly-protected columns still sitting in the baseline. 226 triggers over 113 columns. Full Go suite green on SQLite and Postgres 17; lint 0 issues. Claude-Session: https://claude.ai/code/session_01XLtX4dbjBpApbAv3SuBcTm	2026-09-01 21:16:17 +00:00
xarmian	f789eaac21	fix(store): protect items.slug against the import path, and make restoration atomic ... [P1] items.slug was unprotected, and the exclusion was true of one write path and false of another — the lesson this cluster keeps re-teaching. The API path derives the slug through slugify, whose [a-z0-9-] output cannot carry a NUL. ImportWorkspace has its OWN INSERT and writes the BUNDLE's slug verbatim: importCoercedSlug returns it unchanged whenever it is inside the length bound, so a crafted bundle puts any bytes it likes there. That is code I wrote in S1. [P1] Six more caller-controlled columns were unprotected: items.created_by, items.last_modified_by, items.source, item_versions.created_by, item_versions.source, item_links.created_by — the handlers let a request body's value win over the server's own. Plus custom_templates.icon. [P1] Trigger restoration was not atomic: 206 CREATE statements outside a transaction leave a window where some tables are protected and others are not, and a concurrent writer can commit an invalid row inside it. It runs in one transaction now. [P1] The restoration check compared a COUNT and matched with LIKE. A database with the right number of triggers but one missing and one extra read as healthy, and IF NOT EXISTS would then never repair the missing one. It compares the SET now, and matches with GLOB — LIKE's `_` is a single-character wildcard, so the old pattern also matched names this code never generates. [P2] The census baseline still listed columns that had become protected, and the test never asserted the two sets are disjoint — so losing a column's protection would have put it back in `unaccounted`, found it still listed, and passed. Disjointness is asserted and the baseline regenerated. [P2] And the finding I want on the record, because my first fix for it was worse than the gap. Codex was right that testing classifyTriggerRefusal with a synthetic error would pass even if the wrapper stopped calling it. I added an "integration" leg that wrote through a guarded connection and asserted the typed error came back. It PASSED — and the refusal came from LAYER A, whose error is the same TYPE, so errors.As succeeded while the trigger was never involved. There is no value that Layer A accepts and Layer B refuses: both implement the same predicate, and the four-way differential test asserts they agree on the whole corpus. The unreachability IS the property, so a reachable case would be testing a disagreement we work to prevent. The leg is replaced by a structural one asserting every wrapper error path routes through the classifier, and the comment says why there is no end-to-end alternative rather than implying the gap was closed. Full Go suite green on SQLite and Postgres 17; lint 0 issues. Claude-Session: https://claude.ai/code/session_01XLtX4dbjBpApbAv3SuBcTm	2026-09-01 21:16:17 +00:00
xarmian	46c55e32f3	fix(store): re-assert triggers after a table rebuild, and correct two inherited classifications ... [P1] A future table rebuild would have silently removed the protection. SQLite drops a table's triggers with the table, this codebase rebuilds tables to change constraints (migrations 025, 055, 056, 057, 068, 072), and migration 084 would never run again because it is recorded as applied. The FTS equivalent warns and moves on, which is the right cost for a derived index and the wrong one for a data invariant: a missing FTS trigger breaks search visibly, a missing NUL trigger is silently no protection against the exact writer Layer B exists for. The triggers are now RE-ASSERTED after every migration pass, and the test proves the loss is real before measuring the restoration. [P1] Six caller-influenced columns were unprotected. workspaces.slug, collections.slug, views.slug and agent_roles.slug are all `slug := input.Slug` with slugify only as a FALLBACK — the census's exclusion note was about items.slug, which is genuinely derived (ItemCreate has no Slug field), and I read it as covering slugs generally. Plus comment_reactions.emoji and oauth_clients.logo_url. [P1/P2] Two classifications were wrong in the census and I inherited both. agent_roles.tools is FREE TEXT — migration 019 says so in as many words ("free-text notes about preferred tools/models") — and classing it JSON would refuse a user's note that happens to be valid JSON carrying an escape. And of the six oauth request columns, only session_data is JSON: RequestForm is `.Encode()`, and scopes/granted_scopes/audience/granted_audience are `strings.Join(..., " ")`. The census extended oauth_clients' jsonStringList classing across tables that do not use it. 20 columns reclassified. [P2] The migration's header told readers to run a generator I had deleted. It is a real test now, skipped unless GEN_NUL_TRIGGERS is set — an artifact that instructs you to run something nonexistent is worse than one with no instructions. [P2] The pin compared trigger NAMES and counts, which would pass a wrong BEFORE UPDATE OF clause, a wrong predicate, or a changed marker — the parts that do the work. It compares the whole rendered text byte for byte now, and reports the first differing line. [P2] The census matched TEXT/CHAR/CLOB only, so a column declared JSON or with no declared type — both BLOB affinity, both holding text fine — would have slipped past. Widened to match what CANNOT hold text. It immediately surfaced item_yjs_updates.update_data, which is correct: that is the binary column Layer A exempts, and it is now recorded as an explicit exclusion rather than hidden by a filter. The table filter also matched "_fts" anywhere in a name, which would skip a real table called something like user_fts_settings. [P2] The differential leg claimed persistence and only checked that no error came back. It reads the value back and compares it now. [P2] Trigger refusals set Ordinal 0 against a documented 1-based field, rendering as "parameter 0". A database refusal knows the COLUMN, not the parameter position, so it sets no ordinal and Error() says so. Full Go suite green on SQLite and Postgres 17; lint 0 issues. Claude-Session: https://claude.ai/code/session_01XLtX4dbjBpApbAv3SuBcTm	2026-09-01 21:16:17 +00:00
xarmian	054b192e5d	feat(store): make the NUL invariant a property of the DATABASE (DOC-2823 S2) ... Closes BUG-2813's old-binary half. S1 put the rule in the binary, which cannot help in the window this bug is about: an older binary serving the same SQLite file has no guard, so a rollback, a staged rollout or a second instance writes rows the invariant forbids. A trigger is enforced by the FILE. 194 BEFORE INSERT/UPDATE triggers over 97 columns — TASK-2825's 86-column census plus the second ring Ruling 2 admitted wholesale. ONE LIST, THREE CONSUMERS, which is the shape TASK-2825 asked for. The migration is GENERATED from internal/store/nulcolumns.go, a guard test compares that list against the LIVE migrated schema, and a second test pins the generated SQL against the list so neither can drift from the other by hand. The census guard is a BASELINE rather than a demand that all 405 text columns be classified. The census classified the 86 that can carry caller text and left ids, timestamps and hashes unenumerated; requiring an entry for each would be 300 lines nobody reads. The 301 known-outside columns are recorded, and ANY change to that set — added, renamed, removed — fails and asks for a decision. The predicate is TASK-2824's, measured on the driver Pad actually embeds: instr for the raw byte (length() C-truncates and cannot be used), and json_tree for the decoded escape, in values AND keys, with a json_valid guard because json_tree raises on a non-document. The doubled-backslash case stays literal, which is the false positive this predicate family exists to avoid. Trigger aborts are classified into the SAME typed error Layer A produces, so a caller cannot tell which layer refused and the handler's existing 400 mapping covers both. That discharges Ruling 2's condition for admitting the second ring: a header-derived user agent or IP hitting a trigger must not surface as a 500 or a broken login. THE FOURTH LEG IS LIT. S1 built the differential harness with three legs and left this one dark; all 17 corpus cases now agree across the HTTP gate, Layer A at the driver, Layer B via a real UNGUARDED SQLite write, and native Postgres. Four independent enforcers, one corpus — the property DOC-2823 named as the actual deliverable. Two consumers needed changes, both legitimate. The FTS trigger census excludes the new triggers by prefix, so adding a protected column does not require editing that test. And a timeline test that INJECTS a NUL to build its fixture now brackets the injection by dropping and restoring the two triggers — it needs a row that violates the invariant, because what it tests is the handling of legacy data, and Layer B exists to stop such a row being written. SQLite only. Postgres refuses a NUL in text natively and an escape decoding to one in jsonb, so it already owns the rule there. Full Go suite green on SQLite and Postgres 17; lint 0 issues. Claude-Session: https://claude.ai/code/session_01XLtX4dbjBpApbAv3SuBcTm	2026-09-01 21:16:17 +00:00
xarmian	14e2341a68	Merge pull request #1230 from PerpetualSoftware/feat/s1-nul-store-seam ... feat(store): enforce the NUL invariant at the driver (DOC-2823 S1)	2026-09-01 11:47:03 -04:00
xarmian	f83de5b904	fix(store,textguard): close codex round 4 — stop chasing shapes, and correct a claim ... [P1] A valuer returning a valuer passed the guard: resolution happened once, so an outer valuer could hand the guard a clean inner value while pgx went on to evaluate it and send a NUL. Resolution is to a FIXED POINT now, bounded so a cyclic valuer is refused rather than looped on. [P1] The text detector still missed shapes pgx binds unconverted, and the next round would have named five more. Two rounds were already spent widening it one shape at a time — exact string, then *string and named types, then json.RawMessage — which is a losing game against a permissive driver. So it is an ALLOW-LIST now: the guard enumerates what this store actually binds (strings, binary blobs, numbers, booleans, times, NULL) and REFUSES anything else rather than passing an unclassifiable value through unchecked. That is the default every previous version had backwards. The full suite on both dialects passes, which is the evidence that the vocabulary is complete. [P2] The typed-nil rule was broader than database/sql's: it nil'd every nil pointer valuer, where the library only does so when the pointer's ELEMENT type implements Valuer and otherwise calls the nil-safe method. Copied exactly now. [P1 residual] And a rationale of mine was FALSE. The duplicate-key gap entry said Postgres "keeps the last too, so it accepts this as well - the two agree today, which is why it is a recorded gap rather than a dialect split." Measured on Postgres 17, a duplicate-key document whose SHADOWED value carries the escape is REJECTED with "unsupported Unicode escape sequence", while the all-clean control deduplicates fine: the parser processes the scalar BEFORE duplicate elimination. So it IS a dialect split — Postgres refuses, this guard accepts, SQLite stores — and I had asserted the opposite without checking. The entry stays in KnownGaps because closing it means replacing the shared predicate's decode with a token walk, which is BUG-2812/S4 and moves both layers together; fixing Layer A alone is the divergence DOC-2823 forbids. The native-Postgres leg ran Corpus only, so the recorded gaps were never measured against the database at all — which is how that wrong rationale survived. It covers KnownGaps now and reports the split explicitly. Full Go suite green on SQLite and Postgres 17; lint 0 issues. Claude-Session: https://claude.ai/code/session_01XLtX4dbjBpApbAv3SuBcTm	2026-09-01 15:09:30 +00:00
xarmian	cc8bf888d2	fix(store,server): close codex round 3 — inspect what is actually bound ... Eight findings, four P1. The three parameter ones share a cause: the guard inspected one value and forwarded another. [P1] Value() was called for the CHECK and the original driver.Valuer was forwarded, so pgx called it again — a stateful valuer could show the guard clean text and the database a NUL. Resolution now happens once and the resolved value is written back into the argument. [P1] A typed-nil valuer, (*sql.NullString)(nil), was called directly and panicked. database/sql special-cases it as SQL NULL; so does the guard now. [P1] Only an exact `string` was recognised. pgx implements NamedValueChecker and binds *string, named string types and json.RawMessage unconverted, so each carried text the guard never saw. Resolution is by reflected KIND now, not by a list of types. [P1] Registration opened a probe connection for BOTH drivers, so creating a SQLite store attempted a live PostgreSQL connection against whatever host the environment happened to name — network access as a side effect of opening a local file — and swallowed the error, skipping the guarantees in exactly the case the check could not run. The assertions moved to connect and prepare time, where the object being asserted about is the one in hand. [P2] The statement-interface assertions were claimed in a COMMENT and existed nowhere. guardStmt forwards neither NamedValueChecker nor ColumnConverter, so a driver gaining one would silently lose its own argument conversion. Asserted now, and the parity test covers the statement level too. [P2] The cross-workspace copy gave a guard refusal its ambiguous "may or may not have landed" message. That refusal fires at parameter binding, before any statement executes — nothing landed, and telling the caller to reconcile invites exactly the manual work DR-13's wording exists to prevent. [P2] The Valuer integration test passed for the wrong reason by default: SQLite's driver lacks NamedValueChecker, so database/sql unwrapped the NullString before the guard ran. The three properties are unit-tested now, independent of which driver is present. And the NULL control asserted only that the write succeeded — it reads the row back and asserts SQL NULL, which an incorrect conversion to "" would otherwise have passed. Full Go suite green on SQLite and Postgres 17; lint 0 issues. Claude-Session: https://claude.ai/code/session_01XLtX4dbjBpApbAv3SuBcTm	2026-09-01 14:29:07 +00:00
xarmian	74c786d590	fix(store,server): close codex round 2 — the wrapper must mirror its base ... Seven findings; two were P1 and one of them was a real hole on Postgres only. [P1] driver.Valuer bypassed the guard. checkParams type-asserted `string`, and pgx implements NamedValueChecker — so it ACCEPTS a sql.NullString unchanged rather than letting database/sql's converter unwrap it, and the guard never saw the text. Measured before the fix: a NUL-bearing sql.NullString on Postgres passed Layer A entirely and was refused by the server as SQLSTATE 22021, i.e. a 500, while the identical value on SQLite got the typed 400 — the dialect split reappearing in the response shape. wiki_links.go binds sql.NullString today. [P1] driver.DriverContext was dropped, so sql.Open used a legacy connector that ignores the context and a cancelled request could leave a pgx dial running to its 60-second timeout. The rest share one cause, and it is the thing to remember: database/sql BRANCHES on whether an optional interface is present, so a wrapper advertising one the base lacks CHANGES behaviour rather than adding a no-op. Measured, the two drivers differ — pgx has no Validator, sqlite has no conn NamedValueChecker and no DriverContext — and the single wrapper type claimed all of them. So the wrapper now MIRRORS its base: four conn variants over the two interfaces that vary, a separate driver type for DriverContext, and the non-varying ones asserted at registration so a driver bump fails loudly instead of degrading. I made the same mistake inside the fix — implementing OpenConnector unconditionally, which broke every SQLite open — and then a third time, where guardConnector.Driver() returned the inner wrapper and a pgx pool reported no DriverContext. The third was caught by a new parity test on its first run, not by review: it asserts wrapped and base advertise EXACTLY the same interfaces. Also: both gap guardrails SKIPPED when their slice was empty, so deleting an entry made the suite green — the opposite of their purpose. They assert counts now. And the oracle test compared the two walkers without pinning any answer, which is how round 27 left both wrong about scalars; known answers are pinned against Postgres, and doing that caught me re-pinning one from a stale comment. Full Go suite green on SQLite and Postgres 17; lint 0 issues. Claude-Session: https://claude.ai/code/session_01XLtX4dbjBpApbAv3SuBcTm	2026-09-01 13:57:07 +00:00
xarmian	ef792bdf35	fix(store,server,textguard): close codex round 1 on S1 — seven findings ... All seven verified before acting; two were production regressions I had introduced and one was a defect older than this branch. [High] The wrapper HID the optional connection interfaces. Measured: the raw modernc conn implements Pinger, SessionResetter and Validator; the wrapped one implemented none, so Ping succeeded without pinging, pooled connections stopped being reset, and dead connections stayed in the pool. Each is now forwarded, falling back to exactly what database/sql does for a conn lacking it. NamedValueChecker is forwarded too, or the wrapper silently narrowed the argument types pgx accepts. [High] Statement-level driver.ErrSkip was wrong. At the CONN level ErrSkip is the documented fallback signal; at the STATEMENT level database/sql propagates it, so a base statement without the context interface would have failed rather than degraded. Both now fall back to the positional form, refusing a NAMED argument rather than binding it by position. [High] Scalar JSON documents bypassed the check — and the database settles it: SELECT ('"a<escape>b"')::jsonb; ERROR: unsupported Unicode escape sequence A bare JSON string is a complete jsonb document. The object/array-only shape test is right for the HTTP gate's "is this a nested document" question and wrong for the store's "will a jsonb parser read this". Both now use a widened form, so the gate refuses it at the door instead of leaving it to the store. That widening broke the gate's independent oracle, which had been NARROWED in BUG-2803 round 27 to make the two walkers agree. The disagreement was real; the direction of the fix was not — nobody measured which one matched Postgres, and it was the oracle. Restored, with the measurement in the comment. Two implementations made to agree are not thereby correct. [High] The 400 mapping was not the single funnel I claimed. createItemChecked, bulk ops and the cross-workspace copy carry their own error envelopes by design. One shared CLASSIFIER now serves all of them; the envelopes stay different, the classification and wording do not. Copy keeps its retry-discouraging message deliberately (PLAN-2357 DR-13). [Medium] The over-refusal justification was FALSE as written. It claimed a JSON-shaped text value carrying a live escape "is a value Postgres would refuse the moment anything parsed it". Nothing parses a text column; Postgres stores it fine. The trade is now stated honestly, and pinned: textguard. StoreOverRefusals records the case, and a test fails when it is paid down. [Medium] Duplicate JSON keys are a real under-refusal, inherited from the shared predicate's map model. DOC-2823 requires Layer A NOT to fix it alone, so it is recorded in textguard.KnownGaps with a test that fails when it CLOSES — the signal that BUG-2812's token-walk landed. [Medium] Test weaknesses, and the fixes caught a real one. The census now sees ALTER TABLE ... ADD COLUMN. The store corpus leg now requires accepted cases to SUCCEED, which immediately exposed a case passing for the wrong reason: a non-JSON value written to the fields column failed on SQLite's own JSON parser, never reaching the guard. Full Go suite green on SQLite and Postgres 17; lint 0 issues. Claude-Session: https://claude.ai/code/session_01XLtX4dbjBpApbAv3SuBcTm	2026-09-01 12:54:19 +00:00
xarmian	3baf984b41	feat(server,store): the differential test's three live legs + the 400 mapping ... Completes S1's deliverable except Layer B, which stays dark until S2. THE THREE LEGS, all driving the SAME corpus: - HTTP gate (internal/server): each case enters as a request body, classed the way a request is — by KEY NAME, "fields" for JSON, "content" for text. - Layer A at the driver (internal/store): each case is a real write, classed by column. - Native Postgres (internal/store): each case is a real INSERT through the RAW pgx driver, no guard in the path, so what is measured is Postgres's own verdict rather than ours reflected back. All 15 cases agree across all three. That is the calibration result the design needed: the guard neither over-refuses (breaking writes Postgres accepts) nor under-refuses (leaving the dialect split BUG-2831 was about). The Postgres leg additionally asserts the SQLSTATE, because "some error occurred" would have been satisfied by a typo in the test's own SQL. THE 400 MAPPING is in writeInternalError — the single funnel every 500 already passes through, so one insertion covers every handler present and future. The item-title unit had to find the same error block three times in one function before a structural test caught the third; this avoids the enumeration entirely. 400 rather than 500 because the request is understood and will be refused identically on retry, and 400 specifically because it is what the gate answers for the SAME value refused at the door — two statuses for one rule would be the layers disagreeing in the response instead of the predicate. The residual is stated rather than hidden: a value can reach the store from something the SERVER composed (BUG-2814's re-emit population), and for those a 400 misattributes the fault. It is still better than 500, and if that case ever needs its own status it needs its own error type first. Mutation matrix extended: 3 more mutants, all killed — mapping removed (500 returns), mapping swallowing every error (the control leg), and the gate ceasing to delegate to the shared core, which is the layers-diverge scenario the corpus exists to catch. Full Go suite green on SQLite and Postgres 17; lint 0 issues. Claude-Session: https://claude.ai/code/session_01XLtX4dbjBpApbAv3SuBcTm	2026-09-01 12:06:08 +00:00
xarmian	976fff4060	test(store): measure the write guard's cost, and report it at the width it holds ... S1 named this measurement as owed: the guard touches EVERY statement, reads included, so a byte scan being "obviously cheap" is an assumption, not a result. Per-call, against a typical read's parameters: 33.5 ns, ZERO allocations. The []byte exemption costs 20 ns and never touches the bytes. A realistic JSON fields blob with no escape costs 32.5 ns and zero allocations, which is the important one — the expensive arm is gated behind the escape pre-filter, so it runs only for a value that actually contains the escape, and never on the read flood. The one expensive case (2167 ns, 16 allocs) is a value carrying an escape, and 256 KiB of prose scans in 7.4 us with no allocation. The end-to-end pair — the same GetItem guarded and unguarded — is reported but deliberately NOT quoted as a percentage. Run-to-run spread inside each group is ~25 us against an expected difference of ~34 ns; three orders of magnitude apart. Six samples cannot resolve that, and "3.6% slower" would be noise read as signal. What it DOES resolve is the allocation profile: 7034 B and 158 allocs on both sides, identical, which is a real invariant. One benchmark silently measured nothing at first: its "harmless escape" was textguard.EscNUL, which decodes to a NUL, so every iteration was refused and the case produced no line at all. It builds a A now, and the comment says why — the same escape-literal trap this unit has hit five times. Claude-Session: https://claude.ai/code/session_01XLtX4dbjBpApbAv3SuBcTm	2026-09-01 11:12:18 +00:00
xarmian	52c480c6d2	feat(store): enforce the NUL invariant at the driver (DOC-2823 S1 Layer A) ... Closes BUG-2814 and the current-binary half of BUG-2813: the fixed binary can no longer WRITE a decoded NUL from any path, request or re-emit. The guard is a database/sql driver wrapper, not a seam in this package, and the two measurements that decided it are in nulguard.go's header. The design named the Queryer seam; Queryer is Query + QueryRow, its own doc says "the read-only subset", and no store write passes through it. Writes reach the driver by four receivers — db.Exec (139 sites), tx.Exec (99), stmt.Exec (2 prepared), and a passed executor — and a wrapper at the *sql.DB level cannot see the prepared ones at all. The deeper reason is this cluster's own lesson: a seam every write site must be EDITED to route through is an enumeration wearing a seam's costume, and nothing stops the next site taking the raw handle. The wrapper never reads SQL text, only bound parameters, so Sprintf-built statements are covered without being understood and a new call site is covered before it is written. It covers Query as well as Exec, which is not symmetry: three writes ride the Query path today (UPDATE ... RETURNING in password_resets and email_verification, INSERT ... RETURNING in yjs_updates). A single-line regex for that shape found ZERO; a structural scan found all three, each a multi-line raw-string literal. CLASSING: string parameters are checked, []byte is exempt. The invariant is about text and JSON columns, and in this store []byte binds BINARY — item_yjs_updates.update_data, the only BLOB/BYTEA column in either schema. The first version checked []byte and refused every Yjs op-log append; the existing collab suite caught it immediately. The exemption is pinned by a census test that fails when a new binary column appears, because the supporting sweep was source-level and this unit is a catalogue of source-level sweeps missing things. Mutation matrix: 13 mutants, 12 killed by named tests. Four survived first and each taught something rather than needing a weaker claim — the Query path, both prepared routes, and a Prepare branch that turned out to be DEAD for both drivers (database/sql routes to PrepareContext when the conn implements it), so that duplicate was removed rather than tested. Full Go suite green on SQLite and Postgres 17. Claude-Session: https://claude.ai/code/session_01XLtX4dbjBpApbAv3SuBcTm	2026-09-01 04:47:57 +00:00
xarmian	d0644a60fc	refactor(textguard): extract the one decoded-NUL predicate and its corpus (DOC-2823 S1) ... Groundwork for S1's store-layer guard. The predicate lived in internal/server and could not be reached from internal/store, so a store guard would have had to reimplement it — two implementations of "does this decode to a NUL", which is the disagreement class this whole cluster exists because of. internal/textguard is a leaf package holding the predicate and the adversarial corpus. The HTTP gate now DELEGATES to it rather than carrying its own copy: stringIsJSONDocument and nestedDocumentDecodesNUL become one-line forwards, and valueDecodesNUL's unclassed arm is textguard's walk verbatim. Its existing 418-line corpus passes unchanged, which is the extraction's proof. What is deliberately NOT shared is named in the package doc: how a layer decides a value IS JSON. The gate derives that from request-body KEY NAMES; the store derives it from the COLUMN a parameter is bound to. Those cannot be merged, so every entry point takes the classification as an argument and the differential test pins that both derivations agree on one corpus. The corpus ships in the package rather than in a _test.go because three packages measure against it and three copies would drift. Fifteen cases, each carrying the reasoning or measurement that put it there — including the doubled-backslash false positive that made the raw-byte parity filter unsound, the raw-NUL-inside-a-JSON-value regression codex round 9 caught, and the nested-document asymmetry where the same escape is fatal one layer up. The escape text and the NUL character are constants, and a test asserts they did not decay into each other by comparing against a value it BUILDS rather than one it types. That mistake was made three times during BUG-2803 and twice more while writing this file. Claude-Session: https://claude.ai/code/session_01XLtX4dbjBpApbAv3SuBcTm	2026-09-01 04:24:45 +00:00
xarmian	576ae21c09	Merge pull request #1229 from PerpetualSoftware/feat/idea-2712-attachment-preview ... feat(attachments): preview markdown and plain-text attachments; surface the repo link in the nav	2026-09-01 00:22:28 -04:00
xarmian	100da86188	feat(nav): surface the repo link in the sidebar footer ... A running instance had no visible connection to the project it is, so people went to a search engine. The reporter called it "purely a QOL addition"; what makes it worth a commit is that the link already EXISTED and was simply unreachable — buried in the user-menu dropdown under Resources. This is a discoverability fix, not a new capability. Icon-only, in `.footer-row` beside the collapse / theme / bell controls, because that row is already where this instance's chrome lives. The mark carries no accessible name of its own, so the anchor has an `aria-label` naming both the destination and the new tab, and the SVG is `aria-hidden` so it is not announced twice. `$lib/brand/links.ts` exists because the URL was already written twice in `UserMenuResources` and this would have been the third copy. A URL duplicated across every surface that shows it is a rename that goes half-applied. The module holds addresses only — list ORDER stays with the surface that renders a list, since the two lists are deliberately different lengths and the ordering contract lives in docs/brand.md. The tests assert the binding, and the last one asserts the property the others cannot: comparing a rendered href to the imported constant passes identically for a component that hardcoded the same string, because both sides end up as the same characters. That is a question about SOURCE, so a narrow guard reads the two consumers and asserts the literal is absent while `GITHUB_REPO_URL` is present — so the absence means "imported" rather than "the link was deleted". Named files rather than a glob, so it cannot fail for an unrelated file nor quietly stop covering these two. Closes #1168	2026-09-01 03:44:02 +00:00
xarmian	7f640a9c40	feat(attachments): render markdown and plain-text attachments in the viewer ... The arm itself, plus the render seam and the browser proof. `renderMarkdownDocument` is a third thin wrapper in FRONT of the shared `marked` pipeline, following `renderMarkedWithAttachments`'s precedent rather than standing up a second renderer. It omits two things deliberately. No wiki-link resolution: an attached `.md` was authored elsewhere, so resolving its `[[brackets]]` against whatever workspace is showing it would silently retarget a foreign document's links at local items — BUG-2830's hazard entered through the front door. No attachment context, and it CLEARS the module-level context for the duration rather than merely leaving it alone: a nested call from a resolver or a `missing` hook that itself renders markdown would otherwise inherit the outer document's workspace and resolver. Save, clear, restore, mirroring the sibling wrapper. Sanitization is inherited, not re-derived, so an attached document is governed by the same allowlist as item content. Plain text does NOT go through the pipeline. A `.txt` renders in a `<pre>` as text, because interpreting a plain-text file's asterisks as formatting would misrepresent its content; Svelte escapes it, so that path emits no HTML at all. THE FALLBACK ARM'S CONDITION CHANGED, and this is the part to check hardest. It read `shownRenderer !== 'raster-image'`, which was correct while the union had one member and would have drawn "No preview available" over every text document the moment it had two. It is now `=== null` — the registry's actual "no renderer claims this" answer — which says what it means and stays correct when 'pdf' lands. INTERACTION, all of it found by review or re-reading rather than by the unit suite: - The wheel handler consumed every wheel before its exclusions, so the card could never scroll. The text exclusion returns BEFORE `preventDefault`, the opposite of every other exclusion there: the others want the wheel swallowed, this one wants it delivered. Scroll chaining to the inert page is stopped by `overscroll-behavior: contain`, so the guarantee the `preventDefault` provided is kept. - The full-bleed layer took `pointer-events: auto`, so a click on the empty area targeted it and backdrop-close was broken for this arm alone. The layer is inert; the CARD is the interactive surface. - `touch-action` INTERSECTS down the ancestor chain, so the card's `pan-y` could never override the stage's `none` and a phone could not scroll at all. The stage gives up its pan claim on this arm only. - The card had no tab stop. The viewer's arrow keys are its own next/previous navigation, so a keyboard-only user could open a document and never reach past its first screen. `tabindex="0"` plus `role="document"` and the filename label; the linter warning is suppressed narrowly with its reason at the site. - The focus-handoff effect tracked `loader.phase` — the IMAGE loader, which this arm disposes, so it never changes there. This is the only arm whose CONTENT is focusable, so a reload unmounted a focused link and stranded focus outside the modal. - `resolvedSize` and `revalidateToken` ride the load key SCOPED to the text arm. The key is shared, so an unconditional append re-ran the effect for the raster arm too and restarted image loads. The token is there because a parent RESTORE drives the image loader through the metadata probe's answer but cannot see a failed text GET's `error` phase — without it, a preview that 404'd while archived stayed permanently errored after the restore that fixed it. E2E, because three of these guarantees are CSS mechanisms and the jsdom suite injects no component styles — `getComputedStyle` there returns the engine default for every element, so two assertions written for them could not fail and were deleted rather than banked as coverage. `web/e2e/attachment-text-preview.spec.ts` covers the render, backdrop close, scrolling with no leak to the surface behind, and selection. Its FIRST RUN is what caught the feature rendering raw source, which the whole green unit suite could not see. CONVE-23 sweep on the prose this falsified: the ADMISSION vs THE ARM block enumerated 'raster-image loads bytes / null is no-bytes' as a two-way split, and the fallback arm described itself as "an entry the viewer cannot draw as an image". Both rewritten, plus a paragraph on why two byte-loading arms leave the no-bytes invariant unchanged in kind. `.pad-e2e-*/` is gitignored: a shared checkout runs concurrent suites, so each seat points `PAD_E2E_DATA_DIR` at its own, and those hold a generated encryption key and a multi-MB WAL. `item-attachment-strip.spec.ts` changes here because it is a CONSEQUENCE of this arm, not a separate concern. Two of its tests uploaded a `text/plain` file and asserted the viewer showed "No preview available" over it — true when written, false by design once text previews. CI caught them; my sweep had not, because I swept the unit tests and stated that boundary nowhere, which reads identically to a complete sweep (CONVE-18's amended half). The fix keeps each test's SUBJECT — both are producer→host wiring tests whose named subject is the fallback arm — and moves the vehicle to PDF, which keeps every property the fixture was chosen for while remaining unclaimed by any renderer. It carries a note saying it will go red again when PLAN-2393 builds the `'pdf'` slot, and that the red is the design: pick the next unclaimed type, never weaken the assertion. Closes #1169	2026-09-01 03:44:02 +00:00
xarmian	eca31efa9e	feat(attachments): add the viewer text loader with two independent size gates ... The markdown/plain-text counterpart to `viewerImageLoader`. The image loader hands a URL to an `<img>` and lets the browser fetch, decode, cache and cancel; text has no such element, so this module owns all three of the things that come for free there: - cancellation: no `src` reassignment drops the previous request, so every load carries an `AbortController` and repointing aborts it - staleness: a late `await` can resolve after the user has navigated, so every completion is checked against the issuing token AND the active id (the no-`{#key}` switch-safety class) - the size bound: the browser will stream a 20 MB log renamed `.md` into memory quite happily TWO SIZE GATES, neither subsuming the other. The metadata gate refuses before any request and saves the transfer. The response gate bounds the bytes actually read, and exists because `LightboxImage.size_bytes` is `number | null` BY DECLARATION — an emitter knows only what its own surface gave it — so the metadata gate passes vacuously for any entry that arrived without a size. Deleting either leaves a real hole, and the tests name which hole each one covers. The gates are restated at the request CHOKEPOINT rather than only at the renderer, following the image loader's stated reason: the renderer showing nothing is not the same as the loader asking for nothing. Tests assert 'fetch was never called', not 'nothing was displayed'. Fetching a type the server will not inline is not the risk; markdown is served `Content-Disposition: attachment` and `fetch()` is unimpeded by that. What keeps it safe is that the bytes never become active same-origin content — PLAN-2393 DR-6 is honoured by the allowlist, which admits neither HTML nor JavaScript, not by the disposition header. Response-derived overflow reports NO figure. In that branch the declared size is absent or demonstrably wrong, and echoing it produces "This file is 10 B — too large to preview" — a sentence that reads as a viewer bug rather than a file problem. Only the metadata branch, which has a trustworthy number, reports one. The bound is measured in BYTES on both the streaming and the `response.text()` leg, and the streaming leg decodes with `{ stream: true }` so a chunk boundary splitting a multi-byte character cannot corrupt the text. The module is honest about where the fallback is weaker: it bounds what is RENDERED everywhere, but what is HELD only on the streaming path, whose fallback audience is jsdom. 20 tests. The stream fixture's `text()` THROWS, so an implementation that ignored the stream and buffered everything cannot pass the tests written to forbid that; the fixture reports read count and cancellation so a test can assert the read STOPPED, since draining-then-measuring reaches the same `too-large` and only the read count tells them apart. `retry` is inert outside `error` — for `too-large` the guard is dead code, but while LOADING it is the only thing preventing two concurrent requests for one entry, a case a surviving mutant exposed. Refs IDEA-2712	2026-09-01 03:44:02 +00:00
xarmian	cd2bf7f977	feat(attachments): claim markdown and plain text for an in-app text renderer ... Adds the third preview predicate GitHub #1169 needs, and widens the surface-renderer union onto the 'text' slot PLAN-2393 reserved. `canPreviewAsText` is deliberately NOT an edit to either existing predicate. `canOpenInViewer` (DR-16) asks what the in-app IMAGE viewer may decode; `canBrowserPreview` (DR-5) asks what the BROWSER may be handed in a new tab, where for `text/markdown` the honest answer stays no because the browser downloads it. Widening DR-5 to reach #1169 would route markdown to the Open-in-new-tab action and reproduce the exact download-instead-of-render behaviour the issue reports. The new question — do WE fetch the bytes and render them ourselves — is the only one #1169 asks, and it gets its own predicate. `canBrowserPreview` and its tests are untouched, which is the signal that nothing widened. MIME-exact, never by category: the server's `CategoryText` CONTAINS the force-download bucket (`text/html`, `text/javascript`, `application/javascript` — `internal/attachments/mime.go`), so a category test would admit exactly the types PLAN-2393 DR-6 forbids inlining. An allowlist excludes them by construction. Deliberately outside the server mirror: `inlineSafe` in mime.go declares itself the mirror of `VIEWER_MIMES` + `BROWSER_PREVIEW_MIMES`, the set the server may send `Content-Disposition: inline`. This set must never join it — we never ask the browser to inline these bytes, we `fetch()` them (which a download disposition does not impede) and render sanitized HTML ourselves. That is what lets `text/markdown` preview in-app while still being served as an attachment. `isMarkdownAttachment` consults the FILENAME, and that is the path that actually fires rather than belt-and-braces: an uploaded `.md` is stored as `text/plain`. `ValidateUpload` sniffs the bytes with `http.DetectContentType`, which answers `text/plain` for prose, and returns the SNIFFED entry; the extension is used only to REJECT a mismatch, and `.md` → `text/markdown` shares `CategoryText`, so nothing rejects. Measured: `ValidateUpload([]byte("# Heading\n..."), "preview.md")` returns `mime="text/plain"`. The MIME check stays first (an explicitly-typed row is honoured whatever it is named); the extension fallback is GATED on the MIME already being in the set, so a filename can never widen what previews. Server side filed as BUG-2841. Set is smaller than what we could render: csv/tsv/json/xml/yaml/toml are allowlisted uploads and all left out, because each has an obviously better rendering this unit does not build, and shipping them raw now would make that rendering a regression later. Tests pin the exclusions with that reason. `TEXT_PREVIEW_MAX_BYTES` carries its receipt: measured over 25 repo `*.md` files (p50 3.6 KB, max 82 KB) and 63 workspace doc bodies (p50 5.0 KB, max 23 KB); 1 MiB is ~12x the largest observed, sits under the 25 MiB upload bound so it has a live range, and errs toward rendering. The comment states the limit honestly — the cap always bounds what is RENDERED, but it saves the TRANSFER only when the size is known before the load. One existing assertion changed meaning rather than breaking: the renderer test asserted `text/plain` has no renderer. It now asserts 'text'. That is the contract change, made explicitly and annotated. Two force-download assertions are labelled as regression guards rather than evidence — they pass against origin/main too. Refs IDEA-2712	2026-09-01 03:44:02 +00:00
xarmian	0d0f1c9125	fix(items): require and bound item titles at every write door (BUG-2833, BUG-2831) ... `PATCH {"title": ""}` was accepted and applied while `POST` refused the same input with 400 "Title is required": the guard was an inline literal inside handleCreateItem, so the sibling handler on the same field never had it. Item titles were also unbounded, and the slug derives from the title with no truncation, so the same input was accepted on SQLite and refused by Postgres at the UNIQUE(workspace_id, slug) btree with an unmapped SQLSTATE 54000 — a latent `pad db migrate` failure as well as a create-path one. One models.NormalizeItemTitle / models.ValidateItemTitle pair now backs every door, enforced authoritatively in store.CreateItem and store.UpdateItem so a future door inherits the rule rather than having to repeat it. The handlers keep a pre-lock copy that REFUSES ONLY: it may answer 400 early and must not alter the input, because its view of the row predates the write lock. - trim: whitespace-only titles are refused, widening the create door. Artifact import already trimmed while create tested == "" exactly, and its comment claimed to mirror the gate it was stricter than. - bound: 255 runes, matching MaxDocumentTitleRunes but justified for items — slugify emits only [a-z0-9-] at one byte per rune and truncates nothing, so 255 runes bounds the slug well under the btree index-tuple cap. That cap is 2704 bytes in practice, not the 8191 the filing quoted; both figures and the readings behind them are in the constant's comment. - non-retroactive: a title identical to the stored one is not a rename, is not validated, and is dropped rather than re-applied — so rows predating the bound stay editable and a no-op echo cannot move an item's slug. - import coerces rather than refuses (empty -> "Untitled", over-long -> truncated, both logged, colliding truncations resolved), matching coerceJSONForImport's recorded disposition three lines away. Refusing would break restoring archives of data this product already accepted. - cross-workspace copy propagates a legacy source title, by ruling. It takes no title from the caller, so it cannot mint one. The guarantee that holds across every path is narrower than "every stored title satisfies the bound", and the comments say so: no CALLER-SUPPLIED title is stored without being validated. Seven codex rounds, 23 findings, ending CLEAN. Two of the findings were defects introduced by earlier fixes in this same unit — an empty-title hole opened through the legacy-protection clause, and a handler-side decision that dropped a concurrent rename — both recorded on BUG-2833's trail. 38 mutants; every behavioural fix has a mutant that is the defect at its site, killed by a named test. Prose sweep per CONVE-23: three comments asserting item titles are unbounded, and a cost model resting on a ~2 MiB single-request title, corrected in place — the guards they document still hold, because the bound is non-retroactive and the cascade charges STORED titles. Filed rather than bundled: BUG-2836, BUG-2839, BUG-2840, BUG-2842. Closes BUG-2833, BUG-2831. Claude-Session: https://claude.ai/code/session_01XLtX4dbjBpApbAv3SuBcTm	2026-08-31 22:45:46 -04:00
xarmian	36c5e45b03	Merge pull request #1226 from PerpetualSoftware/fix/bug-2830-2834-grammar-parity ... fix(links): wiki-link grammar parity — Go/JS divergence and slug-prefix retarget (BUG-2830, BUG-2834, BUG-2832)	2026-08-31 20:03:40 -04:00
xarmian	b2c303c4bb	docs(links,store,models): cite markdown.ts by symbol, and check it (BUG-2832) ... Go comments describe the web renderer constantly and cite it by LINE NUMBER. Nothing verifies those citations — they cross a language boundary, so no compiler, test or linter has ever checked one — and they had drifted onto unrelated code. This converts all 32 to `markdown.ts::symbolName` form and adds the check that makes the conversion worth something. Scope note, because this is wider than the rider it was dispatched as. The BUG-2834 commit added the pattern constant near the top of markdown.ts, shifting the file by +45 lines and invalidating EVERY line citation into it — including the three BUG-2832 had confirmed were still accurate. Leaving 13 knowingly-wrong citations because they sit outside the files this unit otherwise touched is not the neutral option when this branch is what broke them. Happy to split this commit back out if the lead would rather hold the rider to its stated bound. While converting, five of the filing's six "suspect, not established" citations were settled by reading the shifted positions: :307, :478-481, :485, :513 and :516 point at a @param doc line, unescapeDocLinks, REF_PATTERN, the tail of parseCrossWorkspaceBody, and findItemByRef respectively. All substantively stale, not merely off-by-lines. That answers the filing's open question. Two guard tests, per the filing's own proposed fix shape: TestMarkdownCitationsNameLiveSymbols verifies every cited symbol is really declared in markdown.ts. This is the check a line number could never have. TestMarkdownCitationsAreNotLineNumbers bans the line-number form, so the fix cannot erode the next time someone reads a number off their editor gutter. The first version of the symbol check FAILED its negative control and that is the part worth reading. It asked strings.Contains(ts, "function "+sym) — a PREFIX match. Renaming resolveWikiBody to resolveWikiBodyRENAMED leaves "function resolveWikiBody" a substring of the renamed declaration, so the guard stayed green through precisely the rename it exists to catch. It passed its first real run and would have shipped as coverage. Fixed by requiring the following character to be one that cannot continue a JS identifier; the control now fires and names the symbol. Both guards are non-vacuity-asserted: the sweep fails if it finds fewer than 50 Go files, and the symbol check fails if it finds no citations at all. Currently verifying 7 distinct symbols across 29 citation sites. The line-number guard earned its keep before being committed — it caught three citations silently reverted when a file was restored from a snapshot taken before the conversion.	2026-08-31 23:24:27 +00:00
xarmian	6114954236	fix(links,store): decide bracket qualification by the old title (BUG-2830) ... An item whose LITERAL title starts with its own collection's slug plus a slash — say "tasks/Setup" in collection "tasks" — stores an index row byte-identical to a genuinely collection-qualified reference to an item titled "Setup". Both read target_title = "tasks/Setup", collSlug = "tasks". Renaming to "Renamed", the first must become [[Renamed]] and the second [[tasks/Renamed]]. bracketRewriteAt inferred "this was qualified" from targetTitle merely STARTING WITH collSlug + "/", which answers the second case for both. So renaming the literal-titled item emitted [[tasks/Renamed]] — converting a literal-title reference into a qualified one, resolved by a different rule. What that costs depends on what else the workspace holds, and both cases are measured: - With an item LITERALLY titled `tasks/Renamed` (slash included), the link is STOLEN outright. resolveTitleTx tries an exact full-title match before the qualified fallback, so that item wins and the renamed item loses its backlink. Under the unfixed code the decoy gains 1 backlink. Silent retarget, exactly as filed. - With no such item, the emitted bracket still finds the renamed item, because collSlug is that item's own collection and it now carries the new title. The cost there is ambiguity wherever a same-titled sibling exists, plus a later collection move breaking `[[tasks/X]]` where `[[X]]` would have followed. Repro delivered before the fix, as the filing required: RewriteBracketAt("see [[tasks/Setup]] here", 4, "tasks/Setup", "Renamed", "tasks") => "see [[tasks/Renamed]] here" with the correct answer depending on information the function did not have. The discriminator is the renamed item's OLD title, and the cascade has had it all along — cascadeTitleRename takes oldTitle and simply never passed it down. It now rides on TitleEscaper (per-cascade, like everything else there), and qualifiedFor decides by COMPARISON: targetTitle == oldTitle -> literal targetTitle == collSlug + "/" + oldTitle -> qualified neither -> index drift, refuse Literal wins when both could apply, and that is the correct precedence rather than a convenient tiebreak: the renderer's stage 1 beats stage 2, so a row pointing at this item resolved literally. resolveBrokenTitleLinks already makes the same stage-1-over-stage-2 ruling for the same reason — the discriminator existed in the codebase and was thrown away before reaching the rewriter. oldTitle is a required parameter of NewTitleEscaper rather than an optional setter, so a caller that forgets it fails to compile instead of silently getting the old behaviour back. NO byte-length precondition guards the fold comparisons. strings.EqualFold is Unicode simple case folding and case-equivalent strings can differ in byte length — EqualFold("K", "K") (KELVIN SIGN) is true at 1 byte vs 3 — so a length check is not a cheap pre-filter but a strictly narrower predicate, and it made a qualified bracket whose title folds across lengths read as index drift, leaving the link stale. The slug boundary is still located by byte offset, which IS sound: collection slugs are ASCII-lowercase by construction (store.slugify). The frozen pre-refactor oracle is deliberately NOT updated — it is an oracle, not live code. BUG-2830 is added to the named list of intentional divergences from it, and the guarded corpus reaches the new function through v0OldTitle, which states what the old implementation implicitly assumed. Inputs where that assumption was WRONG cannot be produced by the derivation and are pinned by name instead. TestProjectRewrittenLen_IsLockstepWithTheRealPass grew a totalApplied assertion: lockstep is trivially true when both sides refuse everything, and this change makes the rewriter refuse more. It applies 5499 rewrites, so it is measuring something. The codex-R2 overlap fixture was respelled for the same reason — its two unrelated target titles are a shape a real cascade cannot produce, so it would have decayed into two no-ops and lost the regression; `[[A[[A]]]]` reproduces the no-op-then-overlapping-change shape with reachable inputs and asserts it applies exactly one. Negative-controlled four ways: reverting qualifiedFor to the prefix rule kills the case-A regression in both its homes while the twin correctly survives; making it accept drift kills the drift test (added because the first mutation run showed that branch was unreachable by the whole suite); the fold-length regression fails without the EqualFold fix; and passing the WRONG oldTitle at the store call site kills five tests including three pre-existing ones — the binding control, since the unit tests pin qualifiedFor and only that shows the cascade hands it the right value, which was the entire bug. The severity above took two wrong turns before it was measured, and both are recorded in the tests rather than quietly corrected. I first asserted the retarget with a decoy that could not be stolen; then, finding that decoy inert, concluded retargeting was impossible and wrote that into a production doc comment. Neither conclusion came from reading resolveTitleTx — both generalised one fixture's result. The two store fixtures now split along exactly that line and each says which case it pins.	2026-08-31 23:24:27 +00:00
xarmian	b6afbb8fca	fix(links,web): align the JS wiki-link grammar with Go's . (BUG-2834) ... The Go and JS wiki-link patterns were byte-identical source text and did not mean the same thing. Both spelled the escape alternative `\\.`, but Go's RE2 `.` excludes only LF while ECMAScript's also excludes CR, U+2028 and U+2029. A body with a backslash immediately before one of those three was INDEXED by the server and NOT RENDERED by the client: the backlink panel claimed a link the document refused to draw. CRLF line endings make the CR case the plausible one. Measured on both sides before deciding anything, over nine code points. Exactly three diverge; VT, FF and U+0085 agree, which bounds the divergence at precisely ECMAScript's LineTerminator set minus LF rather than leaving it open at "some whitespace controls". Aligns JS UP to Go (`\\[^\n]`) rather than narrowing Go, for three independent reasons: the grammar's other alternative already admits RAW CR/LS/PS in both languages, so narrowing Go would make `[[A<CR>B]]` legal and `[[A\<CR>B]]` illegal; narrowing Go would stop ExtractWikiLinks returning rows it currently returns, and the next reconcile would DELETE them, which is BUG-2805's damage shape; and markdown.ts's own splitWikiBody already treats `\<CR>` as an escape pair, so only its regex disagreed. LF stays excluded on both sides — scanBracketBody depends on that and is untouched. Also collapses the two duplicate regex literals in markdown.ts into one exported WIKI_LINK_PATTERN_SOURCE. Exported as source text, not a RegExp object, because a `/g` regex carries lastIndex and sharing one between a replace() and a test's exec() would couple them through it. The harness is the part meant to outlive the fix. testdata/wiki_grammar_corpus.json is read by BOTH languages, and carries expectations derived from the grammar spec rather than from either implementation — comparing the two implementations to each other would have reproduced the exact blind spot that hid this, since looking identical is what they already did. Pure ASCII with every control character as a \uXXXX escape: an early probe typed U+2028/U+2029 into a shell heredoc, silently lost them, and would have "confirmed" the bug on two cases that were actually spaces. JS assertions are split across the node and jsdom projects because renderMarkdown finishes through DOMPurify and returns '' without a DOM, where it would fail for a reason unrelated to the grammar. Same split, same reason, as the existing markdown.shareAttachments pair; the jsdom file carries a leg proving the DOM path is live, since '' satisfies every not.toContain assertion. Negative-controlled both ways: reverting the JS pattern fails exactly 9 assertions (3 corpus + 3 per call site) and nothing else; narrowing the GO pattern to JS semantics fails the same 3 cases from the other side, so both halves are live instruments rather than tests that cannot fail.	2026-08-31 23:23:56 +00:00
dependabot[bot]	99ec711813	chore(ci)(deps): bump anchore/sbom-action/download-syft (#1222) ... Bumps the actions-minor-and-patch group with 1 update: [anchore/sbom-action/download-syft](https://github.com/anchore/sbom-action). Updates `anchore/sbom-action/download-syft` from 0.24.0 to 0.24.2 - [Release notes](https://github.com/anchore/sbom-action/releases) - [Changelog](https://github.com/anchore/sbom-action/blob/main/RELEASE.md) - [Commits](https://github.com/anchore/sbom-action/compare/e22c389904149dbc22b58101806040fa8d37a610...3ad7283483fc7af8ff2b4ea19663c2d5ca935e26) --- updated-dependencies: - dependency-name: anchore/sbom-action/download-syft dependency-version: 0.24.2 dependency-type: direct:production update-type: version-update:semver-patch dependency-group: actions-minor-and-patch ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>	2026-08-31 17:15:42 -04:00
dependabot[bot]	459ba370ec	chore(deps)(deps): bump the npm-minor-and-patch group (#1223) ... Bumps the npm-minor-and-patch group in /web with 16 updates: | Package | From | To | | --- | --- | --- | | [@tiptap/core](https://github.com/ueberdosis/tiptap/tree/HEAD/packages/core) | `3.30.2` | `3.30.5` | | [@tiptap/extension-bubble-menu](https://github.com/ueberdosis/tiptap/tree/HEAD/packages/extension-bubble-menu) | `3.30.2` | `3.30.5` | | [@tiptap/extension-code-block-lowlight](https://github.com/ueberdosis/tiptap/tree/HEAD/packages/extension-code-block-lowlight) | `3.30.2` | `3.30.5` | | [@tiptap/extension-collaboration](https://github.com/ueberdosis/tiptap/tree/HEAD/packages/extension-collaboration) | `3.30.2` | `3.30.5` | | [@tiptap/extension-collaboration-caret](https://github.com/ueberdosis/tiptap/tree/HEAD/packages/extension-collaboration-caret) | `3.30.2` | `3.30.5` | | [@tiptap/extension-link](https://github.com/ueberdosis/tiptap/tree/HEAD/packages/extension-link) | `3.30.2` | `3.30.5` | | [@tiptap/extension-placeholder](https://github.com/ueberdosis/tiptap/tree/HEAD/packages-deprecated/extension-placeholder) | `3.30.2` | `3.30.5` | | [@tiptap/extension-table](https://github.com/ueberdosis/tiptap/tree/HEAD/packages/extension-table) | `3.30.2` | `3.30.5` | | [@tiptap/extension-task-item](https://github.com/ueberdosis/tiptap/tree/HEAD/packages/extension-task-item) | `3.30.2` | `3.30.5` | | [@tiptap/extension-task-list](https://github.com/ueberdosis/tiptap/tree/HEAD/packages/extension-task-list) | `3.30.2` | `3.30.5` | | [@tiptap/pm](https://github.com/ueberdosis/tiptap/tree/HEAD/packages/pm) | `3.30.2` | `3.30.5` | | [@tiptap/starter-kit](https://github.com/ueberdosis/tiptap/tree/HEAD/packages/starter-kit) | `3.30.2` | `3.30.5` | | [@tiptap/suggestion](https://github.com/ueberdosis/tiptap/tree/HEAD/packages/suggestion) | `3.30.2` | `3.30.5` | | [mermaid](https://github.com/mermaid-js/mermaid) | `11.17.0` | `11.17.2` | | [svelte-dnd-action](https://github.com/isaacHagoel/svelte-dnd-action) | `0.9.78` | `0.9.79` | | [marked](https://github.com/markedjs/marked) | `18.0.10` | `18.0.11` | Updates `@tiptap/core` from 3.30.2 to 3.30.5 - [Release notes](https://github.com/ueberdosis/tiptap/releases) - [Changelog](https://github.com/ueberdosis/tiptap/blob/v3.30.5/packages/core/CHANGELOG.md) - [Commits](https://github.com/ueberdosis/tiptap/commits/v3.30.5/packages/core) Updates `@tiptap/extension-bubble-menu` from 3.30.2 to 3.30.5 - [Release notes](https://github.com/ueberdosis/tiptap/releases) - [Changelog](https://github.com/ueberdosis/tiptap/blob/v3.30.5/packages/extension-bubble-menu/CHANGELOG.md) - [Commits](https://github.com/ueberdosis/tiptap/commits/v3.30.5/packages/extension-bubble-menu) Updates `@tiptap/extension-code-block-lowlight` from 3.30.2 to 3.30.5 - [Release notes](https://github.com/ueberdosis/tiptap/releases) - [Changelog](https://github.com/ueberdosis/tiptap/blob/v3.30.5/packages/extension-code-block-lowlight/CHANGELOG.md) - [Commits](https://github.com/ueberdosis/tiptap/commits/v3.30.5/packages/extension-code-block-lowlight) Updates `@tiptap/extension-collaboration` from 3.30.2 to 3.30.5 - [Release notes](https://github.com/ueberdosis/tiptap/releases) - [Changelog](https://github.com/ueberdosis/tiptap/blob/v3.30.5/packages/extension-collaboration/CHANGELOG.md) - [Commits](https://github.com/ueberdosis/tiptap/commits/v3.30.5/packages/extension-collaboration) Updates `@tiptap/extension-collaboration-caret` from 3.30.2 to 3.30.5 - [Release notes](https://github.com/ueberdosis/tiptap/releases) - [Changelog](https://github.com/ueberdosis/tiptap/blob/v3.30.5/packages/extension-collaboration-caret/CHANGELOG.md) - [Commits](https://github.com/ueberdosis/tiptap/commits/v3.30.5/packages/extension-collaboration-caret) Updates `@tiptap/extension-link` from 3.30.2 to 3.30.5 - [Release notes](https://github.com/ueberdosis/tiptap/releases) - [Changelog](https://github.com/ueberdosis/tiptap/blob/v3.30.5/packages/extension-link/CHANGELOG.md) - [Commits](https://github.com/ueberdosis/tiptap/commits/v3.30.5/packages/extension-link) Updates `@tiptap/extension-placeholder` from 3.30.2 to 3.30.5 - [Release notes](https://github.com/ueberdosis/tiptap/releases) - [Changelog](https://github.com/ueberdosis/tiptap/blob/v3.30.5/packages-deprecated/extension-placeholder/CHANGELOG.md) - [Commits](https://github.com/ueberdosis/tiptap/commits/v3.30.5/packages-deprecated/extension-placeholder) Updates `@tiptap/extension-table` from 3.30.2 to 3.30.5 - [Release notes](https://github.com/ueberdosis/tiptap/releases) - [Changelog](https://github.com/ueberdosis/tiptap/blob/v3.30.5/packages/extension-table/CHANGELOG.md) - [Commits](https://github.com/ueberdosis/tiptap/commits/v3.30.5/packages/extension-table) Updates `@tiptap/extension-task-item` from 3.30.2 to 3.30.5 - [Release notes](https://github.com/ueberdosis/tiptap/releases) - [Commits](https://github.com/ueberdosis/tiptap/commits/v3.30.5/packages/extension-task-item) Updates `@tiptap/extension-task-list` from 3.30.2 to 3.30.5 - [Release notes](https://github.com/ueberdosis/tiptap/releases) - [Commits](https://github.com/ueberdosis/tiptap/commits/v3.30.5/packages/extension-task-list) Updates `@tiptap/pm` from 3.30.2 to 3.30.5 - [Release notes](https://github.com/ueberdosis/tiptap/releases) - [Changelog](https://github.com/ueberdosis/tiptap/blob/v3.30.5/packages/pm/CHANGELOG.md) - [Commits](https://github.com/ueberdosis/tiptap/commits/v3.30.5/packages/pm) Updates `@tiptap/starter-kit` from 3.30.2 to 3.30.5 - [Release notes](https://github.com/ueberdosis/tiptap/releases) - [Changelog](https://github.com/ueberdosis/tiptap/blob/v3.30.5/packages/starter-kit/CHANGELOG.md) - [Commits](https://github.com/ueberdosis/tiptap/commits/v3.30.5/packages/starter-kit) Updates `@tiptap/suggestion` from 3.30.2 to 3.30.5 - [Release notes](https://github.com/ueberdosis/tiptap/releases) - [Changelog](https://github.com/ueberdosis/tiptap/blob/v3.30.5/packages/suggestion/CHANGELOG.md) - [Commits](https://github.com/ueberdosis/tiptap/commits/v3.30.5/packages/suggestion) Updates `mermaid` from 11.17.0 to 11.17.2 - [Release notes](https://github.com/mermaid-js/mermaid/releases) - [Commits](https://github.com/mermaid-js/mermaid/compare/mermaid@11.17.0...mermaid@11.17.2) Updates `svelte-dnd-action` from 0.9.78 to 0.9.79 - [Changelog](https://github.com/isaacHagoel/svelte-dnd-action/blob/master/release-notes.md) - [Commits](https://github.com/isaacHagoel/svelte-dnd-action/commits) Updates `marked` from 18.0.10 to 18.0.11 - [Release notes](https://github.com/markedjs/marked/releases) - [Commits](https://github.com/markedjs/marked/compare/v18.0.10...v18.0.11) --- updated-dependencies: - dependency-name: "@tiptap/core" dependency-version: 3.30.5 dependency-type: direct:production update-type: version-update:semver-patch dependency-group: npm-minor-and-patch - dependency-name: "@tiptap/extension-bubble-menu" dependency-version: 3.30.5 dependency-type: direct:production update-type: version-update:semver-patch dependency-group: npm-minor-and-patch - dependency-name: "@tiptap/extension-code-block-lowlight" dependency-version: 3.30.5 dependency-type: direct:production update-type: version-update:semver-patch dependency-group: npm-minor-and-patch - dependency-name: "@tiptap/extension-collaboration" dependency-version: 3.30.5 dependency-type: direct:production update-type: version-update:semver-patch dependency-group: npm-minor-and-patch - dependency-name: "@tiptap/extension-collaboration-caret" dependency-version: 3.30.5 dependency-type: direct:production update-type: version-update:semver-patch dependency-group: npm-minor-and-patch - dependency-name: "@tiptap/extension-link" dependency-version: 3.30.5 dependency-type: direct:production update-type: version-update:semver-patch dependency-group: npm-minor-and-patch - dependency-name: "@tiptap/extension-placeholder" dependency-version: 3.30.5 dependency-type: direct:production update-type: version-update:semver-patch dependency-group: npm-minor-and-patch - dependency-name: "@tiptap/extension-table" dependency-version: 3.30.5 dependency-type: direct:production update-type: version-update:semver-patch dependency-group: npm-minor-and-patch - dependency-name: "@tiptap/extension-task-item" dependency-version: 3.30.5 dependency-type: direct:production update-type: version-update:semver-patch dependency-group: npm-minor-and-patch - dependency-name: "@tiptap/extension-task-list" dependency-version: 3.30.5 dependency-type: direct:production update-type: version-update:semver-patch dependency-group: npm-minor-and-patch - dependency-name: "@tiptap/pm" dependency-version: 3.30.5 dependency-type: direct:production update-type: version-update:semver-patch dependency-group: npm-minor-and-patch - dependency-name: "@tiptap/starter-kit" dependency-version: 3.30.5 dependency-type: direct:production update-type: version-update:semver-patch dependency-group: npm-minor-and-patch - dependency-name: "@tiptap/suggestion" dependency-version: 3.30.5 dependency-type: direct:production update-type: version-update:semver-patch dependency-group: npm-minor-and-patch - dependency-name: mermaid dependency-version: 11.17.2 dependency-type: direct:production update-type: version-update:semver-patch dependency-group: npm-minor-and-patch - dependency-name: svelte-dnd-action dependency-version: 0.9.79 dependency-type: direct:production update-type: version-update:semver-patch dependency-group: npm-minor-and-patch - dependency-name: marked dependency-version: 18.0.11 dependency-type: direct:development update-type: version-update:semver-patch dependency-group: npm-minor-and-patch ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>	2026-08-31 17:15:38 -04:00
xarmian	69a6ebfe69	fix(links,store): make the rename cascade escape-aware in both directions (BUG-2805) (#1225) ... * fix(links,store): make the rename cascade escape-aware in both directions (BUG-2805) Item titles containing `]`, `|` or `\` did not round-trip through a rename. Repro through the real API on TASK-2826 (Rook) with hex receipts; this fixes both directions plus a third half that repro did not name. Direction 1 — MATCHING. The rewriter compared the RAW bracket body against the unescaped title, so a link stored as `[[Weird \] Title]]` never matched. The link was left naming the OLD title while the reparse flipped its index row to broken: content and index disagreeing, with the staleness visible to the user. Matching now runs on the UNESCAPED body, full-body first and then the split-on-unescaped-pipe form — the same preference order the parser and the renderer use, which is what keeps literal-pipe titles like "A|B" working. Direction 2 — EMISSION. The rewriter emitted the new title by plain concatenation. A title containing `]` produced a bracket the grammar cannot parse, so the reparse found no link and DELETED the index row — permanent damage from an ordinary rename, since a later rename has no row left to cascade. A title containing `\]` produced valid syntax for a DIFFERENT title. Emission now escapes, byte-for-byte the same rule as the editor's escapeWikiBody (markdown.ts:748). Third half, not in the filing or the repro — the close-scan. The rewriter found its closing `]]` with strings.Index, which is not the grammar: in `[[A\]]]` it stops at the `]` belonging to the `\]` escape. That was latent only while the rewriter never emitted escapes; this fix makes escaped bodies routine, so each rename would have corrupted what the last one wrote. scanBracketBody now implements the parser's body production exactly. Allocation discipline carried forward from BUG-2804 R5: escaping happens ONCE per cascade via a TitleEscaper, not per source and not per bracket. A cascade renames one item to one title, but the projection and rewrite run once per source and the scan bound admits very many sources when their titles are short — escaping inside those calls would multiply an unbounded title by an unbounded source count, which is the R5 defect in a new costume. The projection path measures with escapedWikiBodyLen and builds nothing. The frozen V0 oracle is now scoped rather than blanket: it stays authoritative for bodies with no escape characters, and every deliberate departure is listed by name with its reason in TestRewriteBracketAt_IntentionalDivergencesFromV0. One departure is outside BUG-2805's scope and called out there: an empty body `[[]]` is not a link under the parser's `+` production, so the scanner refuses it where V0 would have MINTED a link from a non-link on a drifted offset. Two accidental compatibilities the repro identified as non-reproducing directions are pinned so the fix cannot trade two silent successes for two silent failures: the legacy full-body-with-pipe fallback, and unescapeWikiBody's leniency toward a stray backslash. Instruments: a 4000-iteration property that every emitted title parses back to itself; a scanner/parser parity property; end-to-end cascade tests for both directions including a PERMANENCE check, since a fix that merely delayed the index-row destruction by one rename would otherwise pass. Gates: gofmt, vet, go build ./... clean; go test ./... PASS on SQLite and Postgres; go test -race PASS on internal/links and internal/store; zero failures, zero data races. * test(links): lock the scanner's delimiter edges + correct a stale renderer citation (BUG-2805) Codex R1 named scanBracketBody's edges as a review priority. Probing them against the parser as oracle found no disagreement — all ten shapes agree, including trailing backslash at EOF, `]]]` runs, an escape consuming the would-be close, and the two where BOTH refuse (`[[]]`, and a bare `]` mid-body). Locked in one assertion per shape so a failure names WHICH edge broke rather than printing a random string. Separately, and more substantive: extract.go carried a stale cross-file claim. It said renderMarkdown at markdown.ts:300 uses a simpler `[^\]]+` regex and therefore REJECTS escaped bodies, making escaped links index-only and never clickable. That is no longer true. markdown.ts:300 is inside a doc comment now, and BOTH wiki-link regexes in that file — renderMarkdown at :326 and wikiLinksToMarkdown at :625 — use this package's exact escape-aware production; the renderer comment at :323-325 names BUG-1744 as the change that aligned them. The staleness is load-bearing for BUG-2805 in the direction that makes the bug WORSE: the stale link a rename left behind was a working, clickable link rather than an invisible one. It had also been quoted forward into TASK-2826's repro as a live constraint, which is how a stale comment turns into a shared wrong premise. Verified by reading both regexes rather than by re-citing the comment. * fix(links): refuse empty-title emission; agree with the parser on backslash-LF (BUG-2805, codex R2) Codex R2, triaged per finding with a receipt. The orchestrator did not spot-check these, so each was confirmed or refuted here first. P1 — CONFIRMED as a test weakness, REFUTED as irrecoverable. The added test counted index rows, so it would have passed with the row present and target_item_id cleared. Measured: after a rename to "" the content survives, the row survives, target_item_id goes NULL, and renaming back RESTORES it. So the broken state is correct rather than damage — no title-form link can resolve to an item with no title, and NULL is exactly what a renderer would resolve — and it is recoverable precisely because the CONTENT was never destroyed. The test now asserts the full post-state plus the recovery leg, which is what distinguishes "broken but honest" from "irrecoverable". Separately and genuinely destructive, found while probing the new TitleEscaper's zero value: an empty new title emitted `[[]]`, which is not a link under the parser's `+` production, so the reparse DELETED the row. Reachable two ways — a zero-value escaper, and UpdateItem accepting a rename to "" because the empty-title guard lives only in handleCreateItem. The rewriter now refuses to emit an empty title segment in both the build and the projection path, kept in lockstep. The door-level validation gap is BUG-2833. P2 backslash-LF — CONFIRMED and introduced by this diff. Go's regexp `.` does not match a newline, so `\\.` cannot consume one and the parser rejects the body; scanBracketBody treated backslash-ANY as a pair and accepted it. Now excludes LF only, which is what the Go parser does. Measured both ways before and after. Establishing that surfaced a PRE-EXISTING divergence, filed as BUG-2834: the Go and JS wiki-link regexes are byte-identical source text, but `.` excludes only LF in Go and all four line terminators in JS (measured with node), so Go indexes a backslash-CR link the renderer will not render. P2 qualified-vs-literal — PRE-EXISTING. Byte-identical output between the frozen V0 oracle and current for both slug variants, so this diff neither introduced nor changed it. Matches BUG-2830's mechanism, already filed. P2 cross-workspace parity — PRE-EXISTING. Two receipts: extract.go's diff against main is comment-only, and no line of this diff touches cross-workspace parsing. Gates: gofmt, vet, go build ./... clean; go test ./... PASS on SQLite and Postgres; go test -race PASS on internal/links and internal/store; zero failures, zero data races.	2026-08-31 15:33:40 -04:00
xarmian	21b4d8d9c4	fix(store,links): remove both quadratics in the item rename cascade and bound it (BUG-2804) (#1224) ... * test(store): measure the item-side rename cascade amplification (BUG-2804) Measurement only — no production code changes, no cap. The dispatch is measure-first, and BUG-2798's arithmetic is explicitly non-transferable to this path, so nothing here reuses its numbers. internal/store/items_rename_probe_test.go sweeps three axes with a k=0 negative control and a precondition that fails if the cascade would not be exercised (an empty cascade would otherwise report a flat line and read as "no amplification"). Instrument is MemStats.TotalAlloc, the same counter documents_rename_bounds_test.go uses, so the two paths compare directly. What it establishes, all measured on this machine: - 2.01x the body allocated per bracket, linear across four intervals (1 -> 8 -> 64 -> 512 -> 4096 brackets, agreeing to three significant figures). Bracket count is not independent of body size: the cheapest link is 5 bytes and nothing caps links per item, so the cost is O(C^2) in ONE linker's body, in ONE request, with no accumulation needed. Confirmed directly: doubling C multiplies allocation by 3.83 / 3.92 / 3.95, converging on 4. - 21.3x-22.2x the body per added linker at 8 brackets each, linear in k. - The outbox payload carries every rewritten body in a single row, at exactly 1.00x the body set, persisted. emitBulkItemEventTx documents this as deliberately unbounded in v1 and invites the measurement. Peak residency is reported as a coarse floor and carries no claim. The sampler sleeps between reads deliberately: ReadMemStats stops the world, and a spin loop turned a sub-second cascade into minutes of wall clock on this probe's first run. * test(store,links): decompose the per-bracket constant into its two sites (BUG-2804) Still measurement only. Checkpoint 2 reported a marginal cost of 2.01x the body per bracket without establishing what the 2 was made of. These probes account for it exactly, and the answer changes the fix shape. - internal/links/probe_alloc_test.go isolates RewriteBracketAt: 1.00x the body per call. So the rewriter is only HALF the per-bracket cost. - items_rename_probe_test.go's SELECT probe finds the other half, and it is not in the rewriter at all. cascadeTitleRename's SELECT joins item_wiki_links to items and projects s.content, returning one row per LINK, each carrying a full copy of the source body. The scan loop de-duplicates by source id only AFTER rows.Scan has allocated a fresh string per row. Measured dead flat at 1.00x the body per row against a SINGLE distinct source: 4096 rows on one 256 KiB item allocate 1.07 GB. 1.00 + 1.00 accounts for the measured 2.01. That makes TWO independent quadratics with different fixes. A single-pass rewriter would leave the SELECT's copy untouched and halve the cost rather than remove it. A third probe tests, and REFUTES, an earlier hypothesis that the second copy was the cascade's `rewritten != newContent` full-body comparison: a different-length new title measures 2.05x against the same-length 2.02x, no drop. The comparison allocates nothing, so it could never have appeared in an allocation counter. Kept as a negative result so the refuted reading is not re-derived. * refactor(links): single-pass RewriteBracketsAt, RewriteBracketAt delegates (BUG-2804 M2) The per-bracket decision moves into one unexported helper; RewriteBracketAt becomes the one-element case so the two cannot drift. Differential tests pin behaviour against the PRE-REFACTOR implementation, frozen verbatim in the test file as an oracle. Comparing RewriteBracketAt to RewriteBracketsAt-of-one would be vacuous now that the former is defined as the latter, so the oracle is what makes the test able to fail. * fix(store): bound the item rename cascade and remove both quadratics (BUG-2804) Three mechanisms, landing together because the first two are factors of one product — either alone still leaves O(C^2) while measuring "2x better". M1: cascadeTitleRename's SELECT no longer projects s.content. It returns one row per LINK, so projecting the body made the driver materialise a full copy per bracket, de-duplicated only after rows.Scan had allocated each one. Measured at 1.00x the body per row against a SINGLE source. Each source's content is now read once. M2: the rewrite is a single pass. links.RewriteBracketsAt splices every recorded bracket with one strings.Builder; RewriteBracketAt is reimplemented as the one-element case so the per-bracket decision has exactly one copy and the public contract is unchanged. M3: MaxItemRenameCascadeBytes (64 MiB) bounds the total linking-item content one rename may process, charged before each body is built so a refusal never allocates the thing it refuses. The number is derived from the live workspace's measured distribution, not inherited from documents.go, and the receipt is in the constant's doc comment. Measured, before -> after: doubling C 3.83/3.92/3.95x -> 2.04/2.21/2.06x (quadratic gone) per bracket 2.01x body -> 0.01x body per linker 21.3x body -> 7.3x body 4096-row SELECT 1,074,850,200 B -> 1,180,856 B (910x) store suite 180.9s -> 86.8s Equivalence is pinned against the PRE-REFACTOR implementation, frozen in the test file as an oracle, over a hand-enumerated behaviour corpus plus 20,000 randomised inputs; and the single pass is pinned against the descending fold the cascade used to perform, over 5,000 randomised inputs with shuffled, duplicated and corrupted offsets. The k-linear retention that remains is in the outbox member snapshots, filed separately as BUG-2827. * fix(store,server): codex R1 findings + a dialect-dependent test instrument (BUG-2804) P1 — ProjectRewrittenLen built a replacement string per bracket to measure it, reintroducing allocate-then-refuse one layer below the cap it feeds. The per-bracket helper now returns the SEGMENTS and never concatenates; projection does length arithmetic only, and the rewrite writes the parts straight into its builder, removing an allocation from the hot path too. P2 — the item rename cascade's refusal had no errors.As arm in handleUpdateItem, so a deliberate, permanent decline reached clients as a 500 implying a retry might help. Now 413 rename_cascade_too_large, composed from the error's typed fields so the internal call path is not published. Pinned by an exact-status test plus a counterfactual that a blanket-413 handler would fail. Checked the sibling write paths rather than assuming the reported one was the population: bulk update and version restore never set Title, so the cascade cannot fire there and this handler is the whole surface. P2 — `applied` counted brackets MATCHED, not brackets CHANGED, so a rewrite reproducing content byte-for-byte would still write the row, bump seq and emit events. Counting changes restores what the pre-fix whole-body comparison did. Cascade reachability is NOT established and the test says so. Separately, found by make test-pg: the refuse-before-building guard used a TotalAlloc ceiling, and the same refusal allocates ~69 MB on SQLite against ~242 MB on Postgres — the ceiling was measuring the driver. Replaced with an exact count of bodies built via a store test seam. The first version of that seam ALSO failed to discriminate: a mutation moving the cap check between the build and the seam call went undetected, so the build and the count are now one function with no third position for the defect to hide in. Gates on this tree: gofmt, go vet, go build ./... clean; go test ./... PASS on SQLite AND on Postgres (own container, private port 5446 — never the shared 5445, whose teardown would kill a sibling seat's container mid-run). * fix(links,server): codex R2 findings (BUG-2804) R2-1 — ProjectRewrittenLen advanced its cursor past a no-op bracket while RewriteBracketsAt did not, so the two disagreed about which overlapping rewrites the guard skips. On `[[A[[B]]]]` with a no-op at 0 and a change at 3, projection reported 10 bytes / 0 applied while the pass produced 13 / 1 — the cascade charged the read and never charged the rewrite it then performed, so the bound leaked on exactly the corrupt and duplicated offsets the defensive paths exist for. Projection now follows the pass, not the reverse: the pass's behaviour is pinned to the descending fold the cascade used to perform, so moving it would have changed cascade semantics under cover of a bug fix. The lockstep property test that pins this needed MIXED per-position target titles — the first version shared one title across a call and passed against the broken code, because the reproducing shape needs a no-op bracket overlapping a changing one, which a shared title cannot express. Cascade rows carry per-row target_title, so mixed is the realistic case. R2-2 — the 413 mapping covered one of THREE places handleUpdateItem reaches UpdateItemWithParentLink. The post-R1 population sweep asked whether other HANDLERS reach the store call and never asked whether this handler reaches it more than once. All three now share writeItemRenameCascadeTooLarge. The collab-edit half had a deeper cause than a missing arm. In applyContentViaCollabOnce's prune-and-direct-write fallback, a failing directWrite() had its error DISCARDED and the original collab error returned in its place, so the caller read a deterministic refusal as a recoverable routing problem and fell through to its own direct write — re-deriving the identical refusal from scratch. Measured 64 rewritten bodies built for one request against an expected 32. Deterministic failures now survive that branch, scoped to the three errors every call site already treats as final (open-children rejection, update conflict, cascade refusal). Everything else still returns the collab error, preserving the graceful-degradation contract the branch exists for. Status alone cannot discriminate the double-work fix — both behaviours end in 413, since the fall-through reaches the plain path's arm — so the store's build observer is now reachable from the server package via an exported test-support setter, and the test asserts the work count. Gates: gofmt, vet, go build ./... clean; go test ./... PASS on SQLite and Postgres; go test -race PASS on internal/links and internal/store with zero data races. Note -race on store runs 705s, past the 600s default timeout. * test(links): make the multi-bracket fold oracle independent (BUG-2804, codex R3) The descending-fold equivalence test built its oracle by folding RewriteBracketAt — which now delegates to RewriteBracketsAt, the function under test. The comparison was therefore circular: it could agree with itself while both diverged from the pre-refactor behaviour it exists to pin. This is the same vacuity I flagged and avoided for the ONE-element assertion, missed one case over in the multi-bracket one. The fold now uses rewriteBracketAtV0, the already-frozen pre-refactor primitive, which is the only genuinely independent oracle available. The test still passes, so the single pass does match true pre-refactor behaviour on every generated input including the overlapping ones — that agreement is now evidence rather than a tautology. Mutation-verified after the change: the fold test kills the dropped-sort and dropped-overlap-guard mutants, two of which it had previously left to other tests. Test-only. `git diff 8ddfdfd6 -- . ':!*_test.go'` is empty, so the Postgres and store -race results recorded for 8ddfdfd6 carry over unchanged. * fix(store): bound the cascade SCAN, not just the rewrite loop (BUG-2804, codex R4) `works` holds one entry per matching LINK ROW, each carrying that row's target_title, and item titles have no length bound. So the scan retained rows x title bytes before the per-source loop fired, and the content-bytes cap could not see it. The doc comment on MaxItemRenameCascadeBytes claimed the opposite — that the cascade's retention was O(1) in the linker count, with the k-linear remainder attributed to the outbox (BUG-2827). Both halves were wrong: the outbox is a different vector, and this function had k-linear retention of its own. That comment was mine, and it is the claim this unit is named after. The scan is now charged against the same budget and refuses DURING the scan, so at the moment of refusal the process holds only rows already counted. One budget covers both phases deliberately: index rows and content bytes are different quantities, but both are memory this one rename makes the server hold, and a second constant would be a second thing to tune with no separate meaning. Measured: refusal fires at 67,113,728 bytes against the 67,108,864 cap — 4,864 past crossing — with zero bodies built, on a fixture carrying 64 rows of 2 MiB titles whose full scan would have reached ~134 MB. Three test corrections, all mine: - The attack I first designed is impossible. A link to a T-byte title costs T bytes of body text, so content and retained-title bytes are COUPLED and content is always larger. The fixture guard caught it. The property that separates the two halves is WHEN the refusal fires, not size. - Two tests pinned an exact admitted count, which the new scan charge shifts by a source; one of them also used a body size that divided the cap evenly, so refuse-after-build totalled exactly the cap and could not have been detected. Both now assert that the work actually done fits the budget. - The scan test is SQLite-scoped with a measured reason: on Postgres the huge-title fixture cannot be built at all, because items carries UNIQUE(workspace_id, slug), the slug is derived from the title without truncation, and the btree tuple limit is 8191 bytes. Filed separately as BUG-2831. The bound itself is NOT dialect-specific — row count is unbounded on both backends. Gates: gofmt, vet, go build ./... clean; go test ./... PASS on SQLite and Postgres; go test -race PASS on internal/links and internal/store; zero failures, zero data races. * perf(links): never build the replacement in the projection path (BUG-2804, codex R5) bracketRewriteAt composed `collSlug + "/" + newTitle` BEFORE the match check, so every bracket paid for it including the leave-alone exit — and ProjectRewrittenLen calls it once per rewrite BEFORE the cascade's cap can fire. R1's shape, one layer down. It does not document away. BUG-2831 established that item titles carry no validation bound, so a multi-megabyte newTitle is admissible and a bracket-dense body projected gigabytes of immediately-discarded allocation ahead of the refusal that exists to prevent exactly that. The helper now MATCHES FIRST and returns a DESCRIPTION — a `qualified` flag for the slug prefix plus a display-suffix slice of the body — so nothing proportional to newTitle is built anywhere in the projection path. The real pass writes "[[", collSlug, "/", newTitle, suffix, "]]" as separate segments. bracketUnchanged compares segment-wise rather than concatenating a candidate, which would have reintroduced the same allocation to answer a comparison. The `ttLower + "|"` concatenation is gone too, spelled out as the same predicate on the same two already-lowered strings. Measured, 4096 brackets against a 1 MiB newTitle: pre-R5 shape 4,328,782,464 bytes 1.0079x newTitle per bracket now 196,704 bytes 0.0000x The second row is a new probe; the first is that probe run against a mutant carrying the old shape, not arithmetic. No semantic change, and the frozen pre-refactor oracle is what establishes that: the full differential suite passes, including 20,000 randomised inputs and the projection/pass lockstep property. The slice-growth transient stays as documented. Gates: gofmt, vet, go build ./... clean; go test ./... PASS on SQLite and Postgres; go test -race PASS on internal/links and internal/store; zero failures, zero data races.	2026-08-31 12:59:14 -04:00
xarmian	0e2cb06add	chore(nix): bump package version to 0.15.0 ahead of the v0.15.0-rc.1 tag ... Claude-Session: https://claude.ai/code/session_01HvAuiZ7JaWyCqqyV99LyWt v0.15.0-rc.1 v0.15.0	2026-08-31 01:32:36 +00:00
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
xarmian	50499bceb8	fix(server): charge workspace imports against the plan limit (BUG-2793) (#1219) ... * fix(server): charge workspace imports against the plan limit (BUG-2793) `POST /workspaces` enforces the user-scoped `workspaces` plan limit before creating. `POST /workspaces/import` did not, and it mints a workspace through the same store.CreateWorkspace — so a user at their plan's limit could exceed it by exporting any workspace and importing it back. Cloud only; enforceUserPlanLimit is a no-op when cloudMode is off, so self-hosted was never affected. Dave ruled the shape on day 63: an import IS a new workspace and counts, with no exemption for re-importing something you previously owned. Export provenance is not trustworthy enough to gate billing on, and the at-limit case that deserves relief — undoing a delete — is served by the restore endpoint, which mints nothing. The call is one line; the PLACEMENT is the fix. It sits beside the #1212 consent gate, ABOVE the Content-Type dispatch, for two reasons: - handleImportWorkspaceBundle is reachable only through that dispatch, so a gate below it would cover the JSON path and leave the tar.gz path — the one that carries attachments, and the one a real export produces — wide open. - Above either body read, so a refused caller never uploads. The two paths have very different size bounds; the gate precedes both. That is not a hypothetical: the mutation matrix includes it. Moving the gate below the dispatch fails ONLY the bundle test and leaves the JSON test green, which is exactly the false confidence a placement-blind fix would have shipped. Five tests, four of them controls, because a gate is easy to get green and hard to get right: the JSON path refuses at the limit, the bundle path refuses at the limit, under-the-limit is NOT refused (a gate wired to the wrong feature key would pass the first two), self-hosted is unaffected (this must not introduce a limit where there are no plans), and a request with no resolved user is not charged — mirroring the create side's `userID != ""` guard, which is not defensive padding but the difference between "no limit applies" and a nil lookup. This is the SECOND gate on workspace creation the import door skipped; the first was the OAuth consent gate (IDEA-2756, PR #1212). Two have now diverged this way, which is the argument for the shared pre-step helper — tracked separately rather than folded in here. Gates: `go test ./...` under Postgres 17 EXIT=0; gofmt clean; `make lint` 0 issues. BUG-2793 Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN * test(server): make the import controls assert success, not merely non-refusal (BUG-2793) Codex round 1. The three control tests were vacuous and I would have shipped them. WorkspaceExport.Version defaults to 0 and the import requires 1, so the under-the-limit, self-hosted, and no-resolved-user cases were all failing with a 500 long before they reached anything this change is about. They passed because they asserted only "not 403" — and a 500 is not a 403. That made all three useless in the same direction: a fix that broke imports outright, or a gate wired to refuse everything with a non-403 status, would have sailed through them while the two refusal tests stayed green. The controls existed precisely to catch that, and could not. Fixed by setting Version: 1 and asserting the real success status, 201. A control that cannot tell success from a server error controls nothing. Mutation matrix re-run after the change, because a matrix over vacuous tests proves nothing either: removing the gate still fails exactly the two refusal tests, and the three controls now pass on genuine imports rather than on identical 500s. Gates: `go test ./...` under Postgres 17 EXIT=0; gofmt clean; `make lint` 0 issues. BUG-2793 Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN * docs(server): scope what the no-resolved-user import test actually pins (BUG-2793) Codex round 2 pointed out that this test locks in a 201 for a caller with no resolved user, and that a reader will take that as approval of userless workspace creation. It is not. The test pins the GUARD — that import behaves as create does when no user resolves — and it drives the handler directly, so it does not prove a real legacy workspace token reaches this code at all. Said so in the test rather than leaving the 201 to speak for itself, and pointed at BUG-2809 for the question it does not answer. Round 2's three findings are all real and all filed rather than folded, because this unit's ruling is specifically the plan limit on the import door: - BUG-2808 — enforceUserPlanLimit is check-then-act, so concurrent requests can exceed any cap. A property of the helper, shared with the create door and every other feature it gates; this change inherits it rather than introducing it. - BUG-2809 — import and create still enforce different preconditions on the same mint: required-name validation (a live defect — an empty name yields an empty SLUG, which is a routing key), settings normalization, source attribution, and the userless case above. Filed as a class because the mechanism is one thing and the record now shows it failing twice. The reviewer also confirmed two non-doors, which is the useful negative: autoCreateWorkspace is intentional first-workspace provisioning, and `pad db migrate-to-pg` calls ImportWorkspace directly as an operator-only migration outside HTTP entirely. BUG-2793 Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN * test(server): pin the JSON-path placement and assert "not charged" as data (BUG-2793) Codex round 3, on the tests. Two ways they could pass without proving what their names say. 1. The JSON refusal test sends VALID json, so a gate placed after the decode would still return 403 and it would stay green. The bundle test covers the gzip half of the placement claim; nothing covered the JSON half, and the code comment claims the gate sits above EITHER body read. Added an at-limit case with an undecodable body: reaching 403 rather than a decode error is only possible if nothing read the body first. 2. The no-resolved-user test asserted only a 201. A regression that quietly attributed the import to the at-limit fixture user would also return 201 and pass. It now asserts the fact instead of inferring it — the user's workspace count is unchanged across the request, and the created workspace has no owner. The mutation matrix now separates the two placements, which is the point of having both tests: - gate below the Content-Type dispatch (still above the decode) -> only the BUNDLE test fails. - gate below the JSON decode -> the bundle test AND the new JSON test fail. Neither mutation is caught by the original refusal test, which is what "passes for the wrong reason" looked like here. Gates: `go test ./...` under Postgres 17 EXIT=0; gofmt clean; `make lint` 0 issues. BUG-2793 Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN	2026-08-27 23:56:55 -04:00
xarmian	427540706c	fix(documents): bound the rename cascade at the title and at the projected total (BUG-2798, BUG-2796) (#1218) ... * fix(documents): bound the rename cascade at the title and at the projected total (BUG-2798, BUG-2796) A document rename rewrites [[oldTitle]] into every linking document. Neither factor of the output size was bounded: titles had no length validation, and the cascade holds every rewritten body in memory before writing any of them. One rename could project 10 GB from a 500 KB input -- 20,000x, measured -- and OOM while holding the workspace rename lock. Two walls, per Dave's day-63 ruling. 1. Title length, bounded at write time (models.MaxDocumentTitleRunes = 255). Runes, not bytes: "255 characters" is what a user and a UI counter mean. Existing over-limit titles stay valid until their next rename -- no retro-breakage of stored data. 2. The cascade's projected TOTAL, bounded at 16 MiB (store.MaxRenameCascadeProjectedBytes), accumulated across the linking set and refused before the first rewrite is built. The total is the right quantity and a per-document cap would not have been. Measured, with the title bound already in place: one linker holding the largest body a 2 MiB request can carry projects 108,632,370 bytes -- 51.8x -- and the aggregate is linear in the number of linkers (108.6 / 217.3 / 434.5 MB at k = 1/2/4, allocation tracking output at ~1.02x). A per-document cap of C still admits k * C, which is the same unbounded shape one level up. The 16 MiB figure has a receipt in the constant's doc comment: it sits above the absolute ceiling of any cascade this development instance could produce (its entire wiki-linking corpus is 10,077,476 bytes) and 6.5x below the single-document attack. The refusal is permanent-shaped and deliberately NOT in ErrLinkCascadeContention's family: 413 with the projection in the message and no Retry-After. Contention means "someone got there first, try again"; this means "this rename cannot be performed as asked". Answering it from the retryable family would tell a client to retry forever. BUG-2796 folds in at the same validation point, as ruled -- a title containing wiki-link syntax is emitted raw by links.ReplaceTitle, so renaming to `A]] [[A` produced two broken links and reported success. The rule is derived from the two mechanisms that consume a stored bracket (the grammar at markdown.ts:327 and the unescaper at markdown.ts:753) rather than from a character blacklist: the first version of this fix banned `]`, `\` and `|` because all three "look like wiki-link syntax", and the round-trip test refuted two thirds of that. `|` in particular is a title shape resolveWikiBody contains a dedicated branch to support, and `[` passes the grammar untouched. Doors enumerated rather than assumed (CONVE-24): store.CreateDocument and UpdateDocument have exactly two callers between them, both HTTP handlers. No CLI, import, or seed path writes a document title. Update previously validated doc_type and status and NOT title -- the one field that drives the cascade -- so the handler tests drive real requests through both doors (CONVE-19). BUG-2798, BUG-2796 Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN * fix(documents): count retained bytes, bound the retry path, escape the cascade's LIKE pattern (BUG-2798) Codex round 1 on #1218. Three findings, all real, all fixed here. 1. The guard bounded projected OUTPUT, which bounds nothing when the new title is SHORTER than the old one. Renaming a 255-character title to a one-character title makes each 2 MiB linker project ~40 KiB while the cascade still retains its 2 MiB read for the compare-and-set, so hundreds of linkers exhaust memory while the counter reports well under the cap. The counter now sums RETAINED bytes — read plus written, both alive at once — so the cap is a statement about resident memory rather than about output. MaxRenameCascadeProjectedBytes becomes MaxRenameCascadeRetainedBytes and moves 16 -> 32 MiB, because the legitimate ceiling it clears doubles under the new metric (that instance's whole wiki-linking corpus retains ~20,154,952 bytes); the single-document attack retains 110,729,522, so it is still refused by 3.3x. 2. The compare-and-set's retry path bypassed the guard entirely. On contention it re-reads the linker and calls ReplaceTitle on whatever the winner wrote — a NEW input, bounded by nothing the scan had checked — so a content edit landing inside the cascade's window could grow a linker from harmless to enormous and walk the rename back into the amplification it would have been refused for. Each document's compare-and-set now carries the cap less what the other linkers hold, and re-checks the grown body against it. 3. The cascade's `content LIKE ?` search term went in unescaped, so a document TITLE decided how the pattern was read. `\` is the default LIKE escape character on Postgres and NOT on SQLite, so `[[Alpha\Beta]]` was searched for as itself on one dialect and as `[[AlphaBeta]]` on the other: linkers not found, cascade rewrites nothing, rename reports success, every link left stale. Silent and dialect-dependent. Codex named the backslash; `%` and `_` are the rest of the class (CONVE-18) — wildcards on both dialects, so a title carrying them selects documents that do not link it. An explicit `ESCAPE '\'` clause plus escapeLikePattern makes both dialects agree, rather than leaving SQLite correct by accident. Finding 3 also constrains finding 3 of the ORIGINAL fix: models' validator allows a lone backslash in a title on the grounds that both renderers handle it, which was true of rendering and false of cascading. That comment now records the dependency — allowing it is only correct while the cascade's pattern stays escaped. Tests, four new, each mutation-verified against the code it guards: - CountsRetainedBytesNotJustOutput — the shrinking rename. Asserts as a PRECONDITION that the projected-output total stays under the cap, so the test cannot pass for the old reason. - RetryRecheckesTheBudgetAgainstTheGrownBody — drives the real race through the afterLinkCascadeRead seam. POSTGRES ONLY and skipped loudly elsewhere: SQLite's BEGIN IMMEDIATE closes the window structurally, so a green run there would be a property of the DSN. - FindsLinkersWhoseTitleContainsABackslash — Postgres only, same reasoning inverted: SQLite is the dialect that was accidentally right. - DoesNotSpendTheBudgetOnDocumentsThatDoNotLinkTheTitle — `%` and `_`. Its first version asserted the decoy's content was untouched and passed against the unescaped pattern, because over-matched rows rewrite to themselves. The observable harm is that they spend the caller's budget, so that is what it now asserts. Mutation matrix for this round: output-only counter -> only the shrinking test fails; retry check removed -> only the retry test fails (PG); LIKE unescaped -> the budget legs fail on SQLite and the backslash test fails on PG. Gates: `go test ./...` under Postgres 17 EXIT=0; SQLite packages EXIT=0; gofmt clean; `make lint` 0 issues. BUG-2798, BUG-2796 Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN * fix(documents): tighten the retry budget, stop charging no-op rewrites, order the typed check first (BUG-2798) Codex round 3 on #1218, an edge-case angle over the new arithmetic and control flow. Three findings fixed, one declined. 1. The retry budget credited back this document's own share, on the reasoning that the retry replaces it. It does not: the original read and rewritten bodies stay reachable through `updates` while the write loop runs, so the re-read and its rewrite are allocated ON TOP of them. The bound could be exceeded by up to one document's share while the arithmetic still reported it satisfied. The budget is now the genuine headroom, `cap - retained`. 2. A concurrent edit that REMOVES the link left a body with no occurrences, which cascadeRetainedBytes still charged twice — once for the read and once for a rewritten copy that does not exist, because strings.Replace returns its input unchanged when there is nothing to replace. That could refuse an otherwise valid rename for memory the cascade never allocates. 3. The handler classified this error by PROSE before testing it by identity. The UNIQUE-constraint arm matches a substring, and the refusal error embeds the caller's title verbatim, so renaming a document to a title containing the words "UNIQUE constraint" came back as a 409 name collision — advice to pick a different name, for a rename that was refused for size and would fail identically under any name. Typed sentinel now tested first. DECLINED: unchecked int64 arithmetic in the projection. The multiplicands are derived from the length of a string already resident in memory, so overflowing int64 needs a single document body of roughly nine exabytes; and the accumulator returns as soon as it passes the cap, so it cannot run away either. Saturating arithmetic here would be guarding a state the machine cannot reach. Tests, three new, each mutation-verified: - RetryBudgetExcludesThisDocumentsOwnStrings — deliberately separate from the existing retry test, because that one catches the check being ABSENT and this one catches it being too GENEROUS. The grown body is sized to fall BETWEEN the two budgets; a body far over the cap cannot tell them apart. - ConcurrentEditThatRemovesTheLinkDoesNotRefuseTheRename — its first version sized the link-free body against the CAP rather than against the retry's real headroom, so the refusal it caught was correct behaviour and the test was wrong, not the code. Re-sized against the headroom: fits when charged once, does not when charged twice. - IsNotMisreportedAsATitleCollision — at the handler, since the defect is entirely in its classification order. Mutation matrix for this round: credit the share back -> only the tight-budget test fails; charge the no-op body twice -> only the link-removed test fails; order the substring arm first -> only the misclassification test fails. Gates: `go test ./...` under Postgres 17 EXIT=0; touched packages re-run after the lint fix EXIT=0; gofmt clean; `make lint` 0 issues. CI green on b09aca12. BUG-2798, BUG-2796 Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN * test(documents): close four ways the cascade-bound tests could pass for the wrong reason (BUG-2798) Codex round 4, aimed at the TESTS rather than the code. No production behaviour changes here; four instruments that were weaker than they read. 1. Every retained-byte case exceeded the cap under `max(read, rewritten)` as well as under `read + rewritten`, so none of them could tell the two arithmetics apart — and taking the larger would hold twice the cap. Added CountsBothStringsNotTheLargerOne: an ordinary same-length rename over content totalling ~60% of the cap, which the sum refuses and the max admits. Its preconditions assert both halves of that gap. 2. Nothing pinned the 255 itself. Every length case derived its inputs from MaxDocumentTitleRunes, so changing the constant to 512 left them all green. That is fine for arithmetic and wrong for this number: it is a product decision Dave ruled, and a silent change to it silently changes how much amplification the cheap door lets through. Deliberately NOT done for MaxRenameCascadeRetainedBytes, which is mine and carries a measured receipt that is expected to be re-measured. 3. The oversize tests asserted only THAT a rename is refused, never that it is refused BEFORE the amplified string is built — which is the entire point of the guard. Moving links.ReplaceTitle above it would have kept them green. Added RefusesBeforeBuildingTheRewrittenBody, measuring cumulative allocation with a ~20x margin: refusing costs the one body it had to scan, building first costs ~108 MB. Verified by mutation — with the guard moved after the rewrite it reports 110,748,144 bytes against a 52,428,800 ceiling. This filing warned that measuring memory to prove the ABSENCE of amplification is flaky by construction. That still holds for the shape it described, a peak-RSS floor. This is the opposite: a generous ceiling on a deterministic counter, with the two outcomes twenty times apart. 4. The "reports the projection" assertions checked for the words `maximum` and `bytes`, which a message saying "maximum bytes exceeded" would satisfy while telling a caller nothing. They now require the cap's actual value and a real byte count. Mutation matrix: charge only the larger string -> only CountsBothStrings fails; move the rewrite above the guard -> only RefusesBeforeBuilding fails. Seventeen mutations across four rounds, each detected by the test that should catch it. Gates: `go test ./...` under Postgres 17 EXIT=0; gofmt clean; `make lint` 0 issues. BUG-2798 Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN * fix(documents): compose the 413 from typed fields instead of splicing err.Error() (BUG-2798) Codex round 5, on the side effects of a REFUSED rename. Side effects were otherwise clean — rollback removes versions, link rewrites, attachment stamps and the title change, and no activity row, SSE event or webhook is emitted — but the response body was built by appending err.Error() to a public sentence. That published whatever any layer had wrapped around the error on its way up. Today that is "update links: store: ", which is a call path clients have no business seeing; tomorrow it is whatever the next wrapper adds, with no decision point in between. The response is now composed from typed fields on a new store.RenameCascadeTooLargeError (NewTitle, Retained, Max), reached with errors.As. Unwrap keeps errors.Is(err, ErrRenameCascadeTooLarge) true, so every existing sentinel check is unaffected. Both FIGURES stay in the message, deliberately: Dave's day-63 ruling asked the refusal to state what it would hold and what the cap is, so "split the rename" is actionable advice rather than a shrug. The reviewer read those numbers as a content-size oracle; that framing does not survive the trust boundary — a rename requires `editor`, documents are readable at `viewer`, so the caller can already read every document the figure summarises and learns nothing from it. What they had no business receiving was the internal call path, and that is what changed. This is round 3's lesson applied in the other direction. There, prose was being used to CLASSIFY an error and should have been identity. Here, prose was being used to REPORT one and should have been data. The test now asserts both halves: the real byte counts are present (not merely the word "maximum"), and the strings "update links:" and "store:" are ABSENT. Mutation-verified — splicing err.Error() back in fails on both leaked prefixes. Gates: `go test ./...` under Postgres 17 EXIT=0; gofmt clean; `make lint` 0 issues. BUG-2798 Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN * docs(documents): correct eight claims the prose made that the code does not (BUG-2798) Codex round 6, aimed at the comments rather than the code. Eight findings, all mine, all real, no behaviour changed. This is the failure mode my own trail keeps naming — code right, prose broader than the sweep, always in the same direction — so they are corrected individually rather than smoothed over. Stale after the round-1 rename: - models cited store.MaxRenameCascadeProjectedBytes, which no longer exists. - the constant's own hostile figure read 110,729,522; it is 110,729,520. - the HTTP test said each body is ~135 KB; the formula produces 264,790 bytes. Claims wider than what is true: - The round-trip test's header stated a biconditional over the whole validator. False: a 300-rune title round-trips perfectly and is still refused, for the unrelated reason that it is an amplification factor. The property is about the SYNTAX rule, over titles inside the length bound, and now says so. - The mirrored grammar/unescaper comment claimed that a TypeScript change would make this test start disagreeing. It cannot — they are static copies, and nothing in the repository fails when the two drift. Replaced with what the duplication actually buys and what it does not. - The cap's receipt used `items` measurements to conclude the guard "cannot fire on honest use" for DOCUMENTS, having itself noted that instance's documents table is empty. The proxy is reasonable and it is an assumption, not a measurement of the guarded path; the inference is now named, with the narrower claim that survives without it. - The 413's comment cited the image_too_large precedent as a bound on OUTPUT while this guard bounds retained read-plus-write. What carries across is the shape — a small request refused for what handling it would cost — not the quantity. - TestRenameCascade_RefusesTheSingleDocumentAttack described the 2 MiB / 110,729,520-byte shape it does not build; it sizes from the cap (1,271,000 bytes retaining 67,108,800). The full-strength shape is exercised by the allocation test, and the comment now points there instead of describing a body that is not in the function. One figure was replaced by measurement rather than corrected by arithmetic: the allocation test claimed a "~20x margin". Both sides are now measured and stated separately — refusing allocates 2,114,624 bytes, ~24.8x under the 52,428,800 ceiling; building the rewrite first allocates 110,748,144, ~2.1x OVER it, taken from running the test against the mutation rather than computed. The smaller margin is the binding one, since it is the gap a regression must cross to be caught, and saying "~20x" hid that. Gates: `go test` on the three touched packages under Postgres 17 EXIT=0; gofmt clean; `make lint` 0 issues. BUG-2798 Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN * fix(documents): stop charging the cascade budget for rows the rewriter cannot touch (BUG-2798) Codex round 7. Third instance of one class, and the one I stopped short of when I extended the previous two. The SELECT that finds candidate linkers is a LIKE; the thing that rewrites them is links.ReplaceTitle. They do not agree on what counts as a match, so the SELECT returns a SUPERSET — and every row in the difference was charged to the caller's retained-byte budget and then handed a no-op UPDATE. Instances one and two were `%` and `_`, wildcards on both dialects, closed by the ESCAPE clause. This is the case half, and it splits the OTHER way from the backslash bug: SQLite's LIKE is ASCII case-insensitive by default while Postgres's is case-sensitive, so renaming `Alpha` scans every body holding `[[alpha]]` on SQLite only. ReplaceTitle is case-sensitive on both and will never touch them, so enough case-variant content could push an otherwise valid rename to a 413 — on one dialect, for content that was never in scope. The fix is to skip a row with no case-sensitive occurrence outright, which closes both halves: no budget is spent, and no pointless UPDATE is issued for a body the cascade was never going to change. The authority on what is a linker is the rewriter's own count, not the pattern that proposed the candidate. The test runs on BOTH dialects deliberately, unlike its Postgres-only backslash sibling: on Postgres it asserts the behaviour was already correct, which is what makes it a regression test rather than a SQLite quirk shim. It also asserts the case variants are left byte-identical — `[[alpha]]` is a different link, not a missed one. Mutation-verified: restoring the charge fails it with 33,554,790 bytes against the 33,554,432 cap. Gates: `go test ./...` under Postgres 17 EXIT=0; gofmt clean; `make lint` 0 issues. An earlier attempt at that gate died on host disk exhaustion, not on this diff — 373 stale go-tmp directories from crashed runs, cleared, re-run green. BUG-2798 Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN * fix(documents): report the whole operation's size when a retry is refused (BUG-2798) Codex round 8, on concurrency and the rest of the rename transaction. The advisory-lock lifetime, the CAS loop's termination, the ordering against attachment stamps and version writes, and the new skip's effect on the transaction's invariants all came back clean. One P2 stood. The retry path bounded correctly and REPORTED wrongly. It compared the re-read body against the headroom — right — and then named only that body in the error. Everything the scan counted is still held, so the operation's real size is the scan total plus the re-read, and a refusal could therefore say it would hold 16,777,200 bytes against a limit of 33,554,432: a refusal whose own figures do not justify it, which reads as a server bug rather than as advice you can act on. The compare-and-set is now handed the scan TOTAL instead of a pre-computed budget, so the same number both bounds and explains: refuse when scanTotal + grown exceeds the cap, and report scanTotal + grown. The test now asserts the refusal justifies itself — the figure reported must exceed the cap it cites — via the typed error added in round 5, which is what makes that property checkable at all rather than a string comparison. Mutation-verified: reporting the re-read alone fails it with exactly the 16,777,200-against-33,554,432 shape. Gates: `go test ./...` under Postgres 17 EXIT=0; gofmt clean; `make lint` 0 issues. BUG-2798 Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN * fix(documents): refuse titles whose links the cascade cannot find; count retry buffers (BUG-2798) Codex round 9, asking what is MISSING rather than what is wrong. Three findings: two fixed, one already filed. ## Titles validated against the wrong layer The validator ACCEPTED `Alpha|Beta` and `Alpha\Beta`, and I defended that choice with a test. Both round-trip perfectly — the renderer reads each back as exactly the title it started from — and the first version of this fix banned them on vibes, so being shown that was a real correction. It was still the wrong call, for a reason that test could not see. A link to such a title can be STORED escaped, as `[[Alpha\|Beta]]`, and the rename cascade searches for the raw `[[Alpha|Beta]]` only. It does not find those links, so the rename succeeds and leaves them pointing at a title that no longer exists — silently, which is BUG-2796's defect wearing different syntax. So the property a title has to satisfy is stricter than the one I tested: not "the renderer reads it back", but "the renderer reads it back AND the cascade can find its links". Validating against the layer that DISPLAYS a title while the layer that MAINTAINS it disagrees is the same mistake as the unescaped LIKE, met from the other side — twice in one unit, which is the part worth noticing. `[` stays accepted: the cascade's search term matches it literally, so it passes the stricter property too. The two characters get their own test rather than a row in the round-trip table, because the table asserts a biconditional and these are refused for a reason that predicate deliberately does not model. That test asserts its own premise — each title must still round-trip — so if that ever stops being true it fails rather than passing for a new reason. ## Retry buffers were not counted rewriteLinkerCAS bounded each retry against the scan total plus THAT attempt. Earlier attempts' buffers become unreachable when expected/next are reassigned, but unreachable is not reclaimed, so a run of failures could hold several copies while the arithmetic counted one. Now accumulated across attempts, which is conservative — it counts garbage as if live — and errs toward refusing, which is the safe direction for a memory bound. The loop is capped at cascadeRewriteAttempts, so it cannot grow without end. ## Already filed Item renames remaining unbounded, and item titles lacking this validation, are real and out of this unit's scope: BUG-2804 and BUG-2805, filed after round 2 with the code verified rather than taken on report. Gates: `go test ./...` under Postgres 17 EXIT=0; gofmt clean; `make lint` 0 issues. BUG-2798, BUG-2796 Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN * fix(documents): validate a title only when the rename actually changes it (BUG-2798) Codex round 10, on back-compatibility. This one is a regression THIS fix introduced, not one it inherited, and it falsified a promise the fix makes about itself in three places. "Enforced at write time; existing titles stay valid until their next rename" is Dave's ruling, and it is repeated in the constant's doc comment and in two commit messages. Validating every SUPPLIED title broke it for the most ordinary shape of an edit there is: a client that PATCHes the whole object, title included, to change the content. Under that, a document with a legacy title became uneditable rather than merely un-renameable — the opposite of grandfathering. Grandfathering is not something you get by validating at write time. It is something you get by not validating a write that is not a rename. The check now fires only when the supplied title DIFFERS from the stored one, which is also exactly the test the store already applies before cascading, so the validation and the work it guards now agree on what counts as a rename. The regression test seeds its legacy document through the store, because the title it needs can no longer be created through the API — which is precisely the population the grandfathering clause exists for. Three legs: the echoed-title content edit succeeds, a title-less content PATCH succeeds, and renaming to another invalid title is still refused. The last is the control; without it, deleting the validation entirely would pass. Filed rather than folded: BUG-2806, existing documents whose links are stored in escaped form are still orphaned by a rename. Round 9 stopped NEW titles of that shape being created; it did not repair the ones already stored, and the asymmetry — the product refusing to create a shape it still mishandles — belongs in the record rather than in this PR, which is nine commits deep on a bound it has already outgrown. Gates: `go test ./...` under Postgres 17 EXIT=0; gofmt clean; `make lint` 0 issues. BUG-2798 Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN * fix(documents): validate the rename under the lock, not against a pre-lock read (BUG-2798) Codex round 11. Round 10 moved title validation behind "only when the title actually changes", which is the right rule and was applied at the wrong place. The handler compares the supplied title against a document it read BEFORE the rename lock. UpdateDocument re-reads under the lock. Those can disagree: echo a legacy title back on a content edit while another request renames the document, and the handler sees "unchanged, skip validation" while the store sees a genuine rename — and writes the legacy title through with nothing having checked it. The rule is unchanged; the enforcement point moved to where the rename is actually decided. The store validates inside the transaction, on the same branch that triggers the cascade, and returns a typed InvalidDocumentTitleError carrying the reason. The handler keeps its pre-lock check, which is still worth having — it gives the common case a fast 400 without opening a transaction — and gains an arm that surfaces the store's refusal for the case its own check could not see. Grandfathering survives intact, because the store's check sits on the title-actually-changed branch, which is the same condition the handler uses. The test calls the store directly rather than reproducing the race: driving the interleaving would test the scheduler, while the property worth pinning is that the store refuses regardless of what a caller did. Its control leg is the grandfathering case — a content edit echoing the unchanged legacy title must still succeed — so validating everything here would fail it, which is round 10's regression restated as a guard. Mutation-verified: removing the store-side check leaves the handler tests green and fails this one with a nil error. Gates: `go test ./...` under Postgres 17 EXIT=0; gofmt clean; `make lint` 0 issues. BUG-2798 Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN * perf(documents): stop counting every linker's occurrences twice (BUG-2798) Codex round 13, on what the guard costs the SUCCESS path rather than what it blocks on the failure path. The cascade counts occurrences because the size guard needs that number before it is willing to build anything. It then handed the same content to links.ReplaceTitle, whose strings.Replace with n < 0 counts it again. Every ordinary rename therefore paid a second full pass over every linking document for a number it already had. links.ReplaceTitleN takes the count the caller already computed. It is a separate function rather than an optional parameter because the obligation is real and silent when broken: passing a number that is too small does not error, it leaves later occurrences unrewritten, which on this path means links left pointing at a title that no longer exists. A name at the call site is cheaper than a comment nobody reads. NO measured speedup is claimed, and the doc comment says so. This removes one linear pass from a path that also allocates a full copy of the same content and issues a write per linker, so the saving is real but not obviously significant. It is here because doing the same work twice needs a reason and there was not one — not because a benchmark asked for it. The test asserts equivalence with ReplaceTitle across several shapes, including the new-title-embeds-old case, and its counterfactual leg asserts that an under-count visibly DIVERGES — if it did not, the caller's obligation would be imaginary and the API misleading. Round 13 also confirmed two things worth recording: no quadratic scan across linkers, and the ESCAPE clause does not materially change the query plan because the leading `%` already forced a content scan. Gates: `go test ./...` under Postgres 17 EXIT=0; gofmt clean; `make lint` 0 issues. BUG-2798 Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN * docs(store): record the consequence the cascade cap necessarily has (BUG-2798) Codex round 14, on authorization and abuse. The refusal paths came back authorization-clean: reachable only after the workspace-access and `editor` checks, with viewers, guests, non-members and cross-workspace document IDs stopped first, and the reported byte figure exposing nothing an editor cannot already read. One consequence stands, and it is a property of having a cap at all rather than a defect in this one: once a workspace's documents linking a title exceed 32 MiB, that title can no longer be renamed, and any editor can put it in that state. Recorded in the constant's doc comment rather than fixed here, because the comparison that matters is with what it replaces. The same input previously took the server down for everyone; it now denies one operation to a role that can already delete every document in the workspace. Trading an unbounded OOM for a bounded, legible refusal is the point of the guard, not a gap in it. What IS missing is that the state has no exit but manual cleanup, with nothing telling an operator which documents to clean. That is a quota-and-recovery question rather than a cascade question, and it is filed as IDEA-2807 with three candidate shapes and an argument for the cheapest one — a state you can get out of is a different severity from one you cannot. The filing also says what is NOT established: no real workspace is known to approach the cap, and this fix's own receipt suggests none does, so it is a trap that exists rather than one anyone has fallen into. The adjacent concern the review raised — repeated near-cap renames contending for the rename lock and the connection pool — is noted there too, with the observation that the pre-existing behaviour was strictly worse, since each attempt was unbounded. Gates: `go test ./...` under Postgres 17 EXIT=0; gofmt clean; `make lint` 0 issues. BUG-2798 Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN * revert(links): remove ReplaceTitleN — it optimised nothing (BUG-2798) Codex round 15 caught a claim of mine that was simply false. Reverting the functional half of c00606b0. I added ReplaceTitleN so the cascade could hand strings.Replace the occurrence count it had already computed, and wrote that this "removes one linear pass from a path that also allocates a full copy". It does not. strings.Replace calls Count UNCONDITIONALLY, before it looks at n: func Replace(s, old, new string, n int) string { if old == new || n == 0 { return s } // Compute number of replacements. if m := Count(s, old); m == 0 { return s } else if n < 0 || m < n { n = m } Read from this machine's GOROOT this turn, rather than recalled. Passing n constrains how many replacements are APPLIED; it does not skip the count. So the function bought nothing and cost something: a second way to do the same thing, carrying an obligation that fails SILENTLY when broken — an under-count leaves later occurrences unrewritten, which on this path means links pointing at a title that no longer exists. API surface with a silent failure mode and no payoff is worse than no API, so it goes rather than getting a corrected comment. The failure is the one my own trail keeps naming: I asserted a mechanism without reading it. What makes this instance worse than the earlier ones is that I wrote a careful hedge — "no measured speedup is claimed" — which reads as rigour while the sentence beside it stated the mechanism as fact. Declining to measure a claim is not the same as checking it, and the hedge made the unchecked claim look examined. The occurrence count stays where it is: the guard genuinely needs it before it will build anything, and computing it there is not redundant with anything the guard can avoid. Also declined this round, as already filed: renaming a legacy title with `|`, `\` or `]` leaves escaped links stale — that is BUG-2806, filed at round 10 with the mechanism verified in code. Gates: `go test ./...` under Postgres 17 EXIT=0; gofmt clean; `make lint` 0 issues. BUG-2798 Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN	2026-08-27 22:45:27 -04:00
xarmian	771ec5bbaa	fix(server): refuse invalid-UTF-8 and NUL query values at the transport (BUG-2784) (#1217) ... The query-string half of BUG-2782. A caller-supplied query value reached a Postgres text comparison, Postgres refused the parameter, and the handler answered 500 — the honest answer is 400, because the caller asked about something that cannot exist. ValidateQuery is a root-router middleware beside ValidatePath, refusing 400 invalid_query when a decoded query key or value is invalid UTF-8 or carries a NUL. Both share one predicate, bindableText. Measured on Postgres 17 (server_encoding UTF8) at 19330410: 8 GET endpoints x 54 parameter names — every name any handler reads — each probe from its own source IP because the api limiter is keyed on ip:. invalid-UTF-8 value: 276 x 200, 56 x 400, 100 x 500 -> 432 x 400 NUL value: 276 x 200, 56 x 400, 100 x 500 -> 432 x 400 control: 376 x 200, 56 x 400, 0 x 500 -> unchanged Zero 500s in the control, so the 100 are attributable to the value. The error is `invalid byte sequence for encoding "UTF8": 0xff (SQLSTATE 22021)`. WHY A TRANSPORT RULE, when BUG-2782 planned per-site validators on BUG-2774's validCursorID model. Reading the mechanism retired that plan: parseItemListParams folds every parameter it does not recognise into a field filter, so ?email=, ?type= and ?anything-at-all= reach a text comparison exactly as ?search= does — 98 of the 100 failures are those two endpoints. The set of names is unbounded by design, so there is no finite list of points to validate. WHY IT IS NOT A NARROWING of what callers may send, which was BUG-2782's objection. That objection is sound against a charset rule and does not reach this one: bindableText requires only valid UTF-8 with no NUL, every legitimate value here is text, and text is valid UTF-8 in any language. CONTRACT CHANGE: the timeline's before_id answers invalid_query rather than invalid_cursor, since the transport rule runs first. Same 400, same client-error contract, less specific code. validCursorID is NOT dead — two of its three call sites read ids from the item's own fields blob, which no request middleware sees — and that reasoning is recorded at the function definition where someone would land before deleting it. Keys are validated precautionarily: an invalid-UTF-8 parameter NAME did not reproduce a 500 in the sweep (7 x 200, 1 x 400), and why it survives is unread, so they are checked rather than assumed safe. Gates: go test ./... green; full Postgres suite -timeout=45m green (28 packages, own container, not the shared port); -race on internal/server green; lint 0 issues; vuln clean; gofmt clean. Mutation matrix 14/14, including the unwiring mutation run against the Postgres leg to confirm it fails with the ORIGINAL 500 rather than merely failing. Ten adversarial review rounds. The first two found a vacuous test (one item in the workspace meant a handler ignoring ?search passed it) and a self-contradicting proof. Later rounds found false statements, including one where my own sweep CLAIM was false. Two real defects were found AFTER the first MERGE verdict, which is why the rounds continued. Filed not folded: BUG-2803, an escaped NUL in a JSON body reaching the store on every JSON write path — a different surface needing decode-time rather than transport-level validation. Release note: invalid UTF-8 or NUL bytes in query parameters now return 400 instead of 500 on Postgres deployments. Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN	2026-08-27 10:35:39 -04:00
xarmian	d28c28e97a	docs(plugin,mcp,readme): the push monitor is consent-gated — say so where agents and operators read (PLAN-2613 S5, TASK-2620) (#1216)	2026-08-27 01:09:10 -04:00
xarmian	1933041027	fix(events): surface the SUBSCRIBE error and refuse callers instead of admitting a dead stream (BUG-2764) (#1215) ... * fix(events): surface the SUBSCRIBE error and refuse callers instead of admitting a dead stream (BUG-2764) * fix(watchevents): surface the SUBSCRIBE error at construction and on resubscribe (BUG-2764) * docs(events): the idle-cycle prose and metric Help now name the third install-nothing reason (BUG-2764) * fix(events): retry uncovered workspaces, wait on in-flight records, refuse after Close (BUG-2764 codex round 1) * fix(events): post-loop check waits on an in-flight record before trusting a live entry (BUG-2764 codex round 2) * test(watchevents): assert what the failed subscribe leaves, not how long it takes (BUG-2764 codex round 3) * docs(events): prose says what the code guarantees — delivery failure or shutdown, replacement refusals do not count as cycles (BUG-2764 codex round 4) * test(server): the 503 mapping is asserted on both subscribe branches; docs scope the refusal to the activity stream (BUG-2764 codex round 5, BUG-2800) * fix(events): uncovered-workspace retry logs are quiet once the bus is closing (BUG-2764 codex round 6) * fix(events): the uncovered-retry log promises a retry only while subscribers remain (BUG-2764 codex round 7)	2026-08-27 00:13:04 -04:00