memory: merge the semantic reinforce's extra write into the live row instead of replacing it

[oranjeai]

Description

_semantic_sqlite_reinforce (nerve's Fix 7) builds the new extra in Python from a value read in
an earlier statement, then assigns the whole column. A plain SELECT takes no SQLite write
reservation, so a writer committing an extra change between that read and the flush is silently
clobbered.

Measured with that write inside the window, both its keys are gone while reinforcement_count
still looks correct. The live store carries the loss: 4.2% of rows lack extra.content_hash,
every one a reinforced row, and still growing.

Both mutations are now computed server-side in one UPDATE, then read back inside the same
still-open write transaction:

• json_set merges into the column's current value, so another writer's keys survive, and the
  increment reads json_extract on that column in the same statement, so it is atomic;
• COALESCE(NULLIF(extra, ''), '{}') normalises NULL/empty, as in memory: make the event-date sweep's writes atomic so concurrent writers are not clobbered #247. A non-object
  extra now raises rather than being healed from the cache (0 such rows of 156,859,
  but reachable);
• no RETURNING: the UPDATE promotes the transaction to a write transaction, so the SELECT
  cannot see an interleaved write. The cache is rebuilt from that value.

This also fixes the cold-cache variant: a restart leaves cached extra at {}, so a
snapshot-built column wiped content_hash. And rowcount == 0 now makes a deleted row
detectable, where the old if row: returned the ghost anyway: that path evicts the stale entry
and creates for real, in #248's shape (which conflicts textually).

_semantic_inmemory_reinforce is unchanged by design: no DB row, no window. memU's own
create_item_reinforce hash arm and update_item share this defect, but both are upstream code
needing a no-double-count wrapper, so a follow-up tracks them.

Validation: 15 new tests (20 cases, two parametrized), 3 racing inside the window; on main 16
fail, 4 pass, 0 skip. 18 mutants killed.

[oranjeai]

Internal second-model review (6 rounds, 38 findings; click to expand)

Every change here was reviewed by an independent model that did not write it, then adjudicated
against measurement rather than assertion. 38 distinct findings across 6 rounds:
30 agreed and fixed, 6 disagreed with recorded evidence, 2 dropped on my own measurement.
Nothing was overridden on prose.

#	Finding	Sev	Verdict
1	body-race-count-false - both surfaces claimed all new tests assert their race fired; only 3 enter the window	major	AGREE, fixed
2	tx-readback-liveness - the race helper fired before the merge, so the write-transaction property was unobserved	major	AGREE, fixed
3	sql-null-untested - write_extra(None) stores TEXT 'null', so the COALESCE half had no live test	major	AGREE, fixed
4	nonobject-extra-silent-noop - a non-object extra no-ops with the column unchanged	nit	AGREE, disclosed
5	pr248-conflict-undisclosed - the sibling merge is a union in intent, not a clean auto-merge	nit	AGREE, disclosed
6	revert-live-tests - two tests also pass at the merge base	major	DISAGREE (already disclosed and mutant-covered; the > 1 oracle is deliberately the consumer's own predicate)
7	tx-readback-exec-bypass - Session.exec calls super().execute, so an instance patch could not see a readback issued through exec	major	AGREE, fixed (engine-level before_cursor_execute)
8	no-damage-claim-false-live-store - the claim that the store holds no damage was FALSE	major	AGREE, fixed
9	body-union-claim-false - the sibling merge conflicts in both files	nit	AGREE, fixed
10	flagship-test-content-hash-assert-tautological - compared the column against a re-read of itself	nit	AGREE, fixed
11	readback-race-comment-dup - the same rationale at four sites	nit	AGREE, fixed
12	json-null-extra - a JSON-null extra committed the UPDATE then raised at the cache rebuild, where the previous code healed the row	major	AGREE, fixed (narrow json_type guard)
13	mutant-count-stale-two-surfaces - the mutant count was stale on two published surfaces	major	AGREE, fixed
14	store-figures-rolling-drift - the census figure moved for a third round	nit	AGREE, made drift-stable
15	reverse-sweep-clobber - the date sweep can still erase a completed reinforcement	blocker (raised 5x)	DISAGREE (see below)
16	baseline-test-liveness - four tests lack a discriminating oracle	major	DISAGREE (each has a killing mutant; a no-UPDATE mutant is killed by 11 of the then-12 tests)
17	fixture-global-leak - the patcher touches more attributes than it restores	major	DISAGREE (identical at the merge base; no ordering, selection or accumulation effect reproduces)
18	nonobject-heal-regression-claim-inverted - the message claimed the old code raised earlier; it HEALED instead	major	AGREE, fixed
19	guard-widening-mutant-survives - a widened guard left the whole class green while destroying values	nit	AGREE, fixed (new narrowness test)
20	commit-msg-store-figure-two-vintages - two census vintages in one message	nit	AGREE, fixed
21	race-comment-verbosity	nit	AGREE, folded
22	nonobject-cache-diverge - [] and arrays of pairs reported a SUCCESSFUL reinforce with a fabricated cache and nothing stored	major	AGREE, fixed
23	readback-skip-liveness - an unsafe no-engine-read implementation was laundered into a skip	major	AGREE, fixed
24	commit-msg-validation-para-stale - five validation figures stale in one paragraph while another paragraph of the same message was current	major	AGREE, fixed
25	test-commentary-volume - experiment narrative in test docstrings	nit	AGREE, trimmed
26	siblings-254-255-undisclosed - two sibling PRs postdated both surfaces	nit	AGREE, disclosed
27	last-reinforced-at-value-unasserted - only the presence of that key is asserted, never its value	nit	AGREE, noted (measured correct)
28	json-string-double-decode - projecting the JSON column decodes once already, so decoding again turned a stored text scalar into a fabricated object and reported a reinforce that never happened	blocker	AGREE, fixed (CAST(extra AS TEXT))
29	invalid-extra-partial-commit - the readback was validated AFTER the commit, so a REFUSED reinforce still advanced updated_at	major	AGREE, fixed (validate then commit, else roll back)
30	returning-presence-skip - a check keyed on the bare RETURNING keyword skipped an implementation that projects only id and still reads the value after the commit	major	AGREE, fixed (require the projection to BE extra)
31	own-r6-prbody-stale-validation-figures - my own review: two body figures staled by the previous round	major	AGREE, fixed
32	own-r6-prbody-census-denominator-drift - the census denominator moved again	nit	AGREE, fixed
33	own-r6-rc-gt-1-test-has-no-mutant - my own review: the returned-count test has no matrix row	nit	DROPPED (see below)
34	pr-body-four-stale-figures - my own review: the frozen body's validation sentence was FALSE in four of five figures (13/16/12/15 against a measured 15/20/16/18)	blocker	AGREE, fixed
35	pr-body-omits-the-disclosed-behaviour-regression - my own review: the non-object behaviour change is disclosed in the commit message but was absent from the PR body, the artifact a maintainer reads to decide the merge	major	AGREE, fixed
36	json-null-contract - normalising JSON null contradicts the "non-object raises" contract	major	DISAGREE (see below)
37	passive-regressions - four tests pass when the fix is reverted, so they do not discriminate	major	DISAGREE (see below)
38	returned-item-rc-test-is-undiscriminated - the same gap as #33, found independently in my own cold review	nit	DROPPED (see below)

Disagreed, with evidence

• Fix background task auto-resume invisible in UI #15 reverse-sweep-clobber (raised five times, refuted five times by rebuilding the
  scenario rather than by citing the earlier verdicts). Running the exact sequence the finding
  describes - the sweep reads its snapshot, a reinforce commits inside that window, the sweep
  flushes its stale whole column - yields reinforcement_count=1, the same key set, cache 2 and
  a returned count of 2 on the fixed tree AND on the unmodified base, from clean exports of
  both. Byte-identical on every field, so the necessary step "this change is the difference" is
  false: the loss is entirely the sweep's own whole-column write. _resolve_event_dates_sync is
  byte-identical between the merge base and HEAD (104 lines each), the sweep's symbols get 0 hits
  across the source diff, and memory: make the event-date sweep's writes atomic so concurrent writers are not clobbered #247 already authors the prescribed server-side merge plus the
  reverse-direction race test.
• Fix memU memorize failing silently on Bedrock provider #36 json-null-contract: json_type distinguishes exactly the shapes at issue - SQL NULL
  yields NULL, a JSON-null document yields 'null', arrays yield 'array', text scalars yield
  'text'. The normalisation arm covers the NULL family, which the guard bullet names
  explicitly; every genuinely non-object document raises, pinned by 7 parametrized cases.
  Making JSON null raise would also be a NEW regression rather than a disclosed one, because the
  previous code healed that state - which is why round 2 raised the OPPOSITE finding (feat: add NixOS support with flake.nix and installer detection #12) and
  it was agreed and fixed. The contract wording this finding asks for was added in this round.
• Add per-turn token usage tracking with cost analytics #37 passive-regressions: the observation is true and the inference is not. A regression
  guard should pass at the base; what makes it live is a discriminating mutant. Three of the
  four named tests are killed by shipped mutants (M4, M5, M12). The fourth is named by no
  shipped mutant - I had independently found the same gap in my own review (Validate images before they enter conversation context #33/Add file management: upload, download, and send_file tool #38) - so I built
  it: a mutant that strips the count signal off the returned and cached item only. Unmutated
  control 20 passed; the mutant fails 11 tests including that one. All four discriminate.
• build(deps-dev): bump flatted from 3.3.3 to 3.4.2 in /web #6 revert-live-tests: disclosed in the evidence before the finding was raised, and both
  named tests have killing mutants.
• fix: pass Anthropic API key from config to SDK subprocess #16 / Add ZIP archive extraction and expandable blockquotes in Telegram #17: as recorded above.

Dropped on my own measurement (#33, #38)

Both are my own findings, and the mutant I built to settle #37 refutes them: the returned-count
test is discriminating. The residual is real but immaterial - the shipped 18-mutant matrix has
no row for that one property, which is covered by an ad-hoc mutant recorded in the review
ledger rather than by the committed matrix.

Two rounds caught figures a previous round had already "corrected", so every published number
here was re-measured against source rather than carried forward - including this comment, which
was rebuilt from the per-round ledgers after the inherited draft was found to list 27 of the 38
findings and to omit two of the gate's own blockers.

[CLAassistant]

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

[oranjeai]

Pre-PR validation gate (click to expand)

#	Question	Answer
a	Deterministic repro?	Yes. A harness lands a third-party extra write strictly inside the read-to-write window and asserts the window was entered, so it fires every run rather than at some rate. Both keys the concurrent writer commits are gone from the row; reinforcement_count and content_hash still look correct, which is why the loss is invisible from this path.
b	Root cause explained?	Yes. The new extra is built in Python from a value read in an earlier statement, then the whole column is assigned. A plain SELECT acquires no SQLite write reservation, so the transaction is still a read transaction when a second connection commits its own extra change; the later flush emits SET extra = <that dict>, in which the other writer's keys are simply absent. The increment is snapshot-based for the same reason. journal_mode=WAL makes this likelier, not less: writers do not block readers, so the racing writer commits without contention.
c	Fix matches root cause?	Yes. It removes the snapshot rather than guarding it: both mutations are computed server-side from the live column in one UPDATE, and the merged value is read back inside the same still-open write transaction. No widened bound, no retry loop, no defensive branch. BEGIN IMMEDIATE was rejected as a band-aid: it only prevents the race, so a writer that committed before the read is still clobbered by the replace, whereas json_set merges regardless of ordering.
d	Test intent preserved / new tests added?	Yes. No existing test was weakened, removed or retagged, and the full-suite failure-name set is identical to main (extracted per test via a report hook, not by parsing output). 15 new tests, one per property, collected as 20 cases because two of them are parametrized (over four non-object shapes and three text-scalar ones); 3 of them land a competing write at a measured point and assert the race fired, while the other 12 pin single-writer properties and enter no window. Two of those cases are load-bearing beyond narrowness: dict() SUCCEEDS on [] and on an array of pairs, so without an explicit isinstance check the cache rebuild reported a reinforce that was never stored and cached a value the row does not have. The narrowness half still holds: a guard widened to != "object" silently rewrites an array extra to an object and destroys the value. Three further cases cover a stored JSON text scalar: the column is JSON-typed, so projecting it decodes once in the driver already, and decoding that result a second time turned '"{}"' into a dict that PASSED the isinstance check -- the function reported a successful reinforce while reinforcement_count never moved. The refusal path is also now observed for the timestamp: a refused merge leaves extra byte-unchanged but updated_at is a plain bound value, so each of those cases captures it BEFORE the call and asserts it did not advance. The concurrency observations are taken at the engine (before_cursor_execute), not at the session: SQLModel's Session.exec calls super().execute, a class-level lookup that skips an instance patch, so a session-level hook could not see a readback issued through exec and turned that defect into a skip instead of a failure. The coverage gap was real: every pre-existing reinforce test passes against the unfixed code, because the only third-party-key test writes its key before the reinforce and therefore exercises cold-cache seeding rather than the window.
e	Both directions demonstrated?	Yes, twice. The harness reports the concurrent keys LOST on unmodified main and SURVIVED on the fixed tree, with the window asserted as entered in both arms. The tests give 16 failed / 4 passed / 0 skipped on main source versus 0 failed / 20 passed fixed, and 8 failed / 12 passed against the immediately preceding tree (the arm that discriminates the last round: 4 of those 8 fail specifically on the updated_at-unchanged assertion and 3 with DID NOT RAISE TypeError, i.e. a reinforce reported as successful); the 4 passes pin contracts the rewrite must not break, not defects (two of them are the json-null pair: main heals that column via dict(extra or {}), so those two are regression guards on this rewrite rather than discriminators of main). The readback test now FAILS on main rather than skipping, because a no-readback implementation is only safe when it is a RETURNING one and main is neither. That property is pinned by four measured arms: the shipped tree passes, a commit-then-read-off-the-engine implementation FAILS on the RETURNING requirement (it issues no engine-visible read at all, so the earlier shape converted it into a skip), a real single-statement .returning(model.extra) implementation skips, and a .returning(model.id) implementation that still reads the value on a fresh connection after the commit FAILS -- the last of these is what a check keyed on the bare RETURNING keyword could not see, and the SAME source skips against the previous test file, which is what proves the strengthening rather than the arm is what discriminates it. A mutation matrix of 18 mutants, one per behaviour, is fully killed with an unmutated control green at both ends, 0 survived and 0 vacuous, re-run in full against the amended tree.
f	Fix is general across code paths?	Invariant: a reinforce must merge into extra, never replace it. Carriers: this arm fixed; the in-memory arm is safe by construction (no DB row, so no window) and left byte-unchanged; the event-date sweep is fixed by #247; the stale-cache entry on the zero-row path is evicted here; #248 owns the ghost-reinforce defect and this uses its shape so the merge is a union; memU's own create_item_reinforce hash arm and update_item carry the identical defect but are upstream code needing a wrapper with a no-double-count contract, so they are disclosed in the PR body and a follow-up tracks both.
g	Fix generalizes across inputs?	Yes: extra NULL, empty string, {} (cold cache after a restart), populated with third-party keys, a concurrently bumped count, and a deleted row are each covered by a test. COALESCE(NULLIF(extra, ''), '{}') is required rather than defensive, since json_set on an empty string raises "malformed JSON". The live store was re-measured at implementation time: zero rows with NULL, empty, non-valid-JSON or non-object extra in 153,210. A non-object extra is nevertheless REACHABLE (Column(JSON) validates nothing, so an ORM assignment of a list stores an array), which is why the merge now raises explicitly on that shape instead of relying on dict() happening to fail.
h	Backward compatible?	Yes. No setting, schema, serialization format or public API changes. The values written are a strict superset of before (strictly fewer keys lost), and updated_at's stored text stays byte-identical to every other writer's, which a test pins. No RETURNING is used, so no undeclared SQLite >= 3.35 requirement is introduced.
i	Invariants and contracts preserved?	Yes. Two contracts leave this function and both are asserted: the returned item's reinforcement_count > 1, which the memorize path reads to skip category linking (flipping it would create an orphan source), and cache/row agreement, which salience ranking reads off the cached item. Error and early-return paths: rowcount == 0 rolls back and falls through to a real create, and the read-back decodes either str or dict but RAISES on any non-object document, so a silent no-op merge can never be reported as a success. The vector index is left in a deliberate size drift on the zero-row path only, which is what forces the next rebuild so the replacement row stays dedup-visible; a code comment says so, and a behavioural assertion pins it rather than a counter identity, which would be false for the correct implementation.

Session id: cron:clickhouse-impl-slot-40:20260803-215400

[oranjeai]

Closing per @pufit's directive on #247: memU is being rewritten and sunset, and Nerve fixes
outside "critical performance problem" or "makes my work easier" are handled by the Nerve team.
This PR is a correctness fix in neither category, so it is closed unmerged. The analysis stays in
the description and comments if it is useful during the rewrite. No further action needed from me.