fix(store): round-trip VAL_BUFFER through ext_store instead of encoding it as null

[Nitjsefnie]

What does this PR do?

store_json_encode encoded VAL_BUFFER as JSON null, so a buffer stored through ext_store came back as null and the data was silently gone. This adds a tagged encoding the decoder recognises and rebuilds as a real VAL_BUFFER, shape included.

Related Issues and Pull Requests

Fixes #805

Changes

• src/ext_store.c: VAL_BUFFER moves off the null path onto a real encoder. Format is {"_eigs_buffer":[…],"_eigs_shape":[rows,cols]}; the decoder resolves both keys by name, so key order is not load-bearing, and it runs at the object's closing brace so a buffer is rebuilt at any nesting depth. The -Werror=switch exhaustiveness you landed in runtime: ValType switches go exhaustive, finishing #738 — first build caught real drift #806 is preserved — no default: reintroduced, the remaining VAL_NULL/VAL_FN/VAL_BUILTIN/VAL_JSON_RAW/VAL_TEXT_BUILDER cases stay enumerated.
• CHANGELOG.md: the format change and the collision disclosure below.
• tests/test_store.eigs: 30 new checks (22 -> 52 in that file), mid-file in the group position the file already uses.

Why tagged and not a numeric array. A buffer carries an observable shape as well as a type — shape of buf reads rows/cols back (eigenscript.h:321) — so the array option loses both, and a store that silently changes a value's type on round-trip is the same class of bug as the one being fixed.

Element width is %.17g, matching trace.c's buffer tape (trace.c:1254), which is also the nearest existing precedent for "disambiguate and rebuild the real type" — its b[…] form does exactly that on replay. There is no pre-existing type-tag convention in ext_store.c to follow; the only reserved names are the _-prefixed record fields (_id, _store, _store_id, _type), and _eigs_ collides with nothing in the tree.

Non-finite elements ride as "nan"/"inf"/"-inf", and that is not defensive dead code. ne_matmul_buf accumulates with a bare += and no num_guard (builtins_tensor.c:39-48), so matmul on large values genuinely reaches +Inf. A bare inf token would fail the strict parser and cost the whole record, not just the buffer — i.e. without this the change would have made one case strictly worse than it is today.

Testing

Merge-base 14c3a07. Release suite 3394/3394, exit 0. ASan+UBSan with detect_leaks=1 3398/3398, exit 0, zero sanitizer findings and no LeakSanitizer tally line. Section [56] EigenStore Database reports PASS: all 52 store checks in both. tools/doc_drift_check.sh exit 0 after the CHANGELOG edit.

The new checks are mutation-proven against a pre-fix build of main:

FAIL: buffer field decodes as a buffer — expected buffer, got none
Error line 101: len: null has no length

and Tests: 52 | Pass: 52 | Fail: 0 on this branch. The check count is 30, not the 32 I first wrote. Coverage includes a zero byte, invalid-UTF-8 bytes, non-finite elements, nesting inside a list, persistence across a close/reopen, and four near-miss dicts that must not decode as buffers.

Follow-ups / Known Limitations

• The tag is ambiguous with user data, and I would rather state the exact shape than claim it is impossible. A dict decodes as a buffer only if it has exactly the two keys _eigs_buffer and _eigs_shape, the body is a list of numbers or the three sentinel strings, the shape is two non-negative integers, and the shape invariant holds. Anything else stays a dict — verified live for a third key, a non-list body, a foreign string element and an inconsistent shape. But a nested field genuinely shaped like the tag does come back a buffer; I confirmed that rather than assuming it away. A top-level record is never at risk, since store_put stamps _id and so it always carries a third key. I considered encode-side escaping and rejected it: it cannot fix data already on disk, and it adds a second ambiguity class of its own.
• STORE_VERSION deliberately not bumped (ext_store_internal.h:10). store_read_header rejects any version ≠ STORE_VERSION, so bumping would make every existing database unopenable in order to recover a value that was already lost. Old records decode exactly as before (they contain no tag), and an older binary reading a new file sees a plain dict rather than a parse failure.
• One behaviour change worth your attention: a buffer now costs real bytes where null cost four, so a large buffer can push a record past STORE_MAX_RECORD_SIZE and raise record exceeds page size. That is the same per-record limit a long list already hits, and a loud refusal seems like the right outcome for this issue — but it is a change.
• One new check asserts that a matmul overflow really is non-finite, which couples it to ne_matmul_buf being unguarded. If you later add num_guard there, that check goes red for a reason unrelated to the store. I kept it so the sentinel path cannot silently stop being covered, but it is easy to drop if you'd rather.
• The sibling trace.c / write_value_ptr_full gap you noted in ext_store silently encodes VAL_BUFFER as null — buffers are lost on store round-trip #805's last paragraph is untouched, as you filed it.

Checklist

• make test passes locally — 3394/3394 release, 3398/3398 under ASan+UBSan with leak detection
• New builtins have signature comments and docs in docs/BUILTINS.md — n/a, no new builtins
• New library functions follow conventions in docs/STDLIB.md — n/a
• New examples have a comment header explaining what they demonstrate — n/a
• CHANGELOG.md updated (if user-facing change)

Generated by Claude Opus 5 (brief, implementation, review)

[Nitjsefnie]

CodeQL flagged the two float comparisons on ext_store.c:321 (alerts 197/198). I checked it rather than acting on it, and the result splits — which turned out to be the useful part.

The alert itself is a false positive. Line 321 is an integrality test — round-trip the stored dimension through int and compare — so exact == is the correct operator and a tolerance would be the bug: 2.0000000000000004 is not a row count. The operands are not arithmetic results either; store_json_parse_number is a bare strtod off the page bytes, so there is no accumulated error for the heuristic to be worried about.

Eleven constructed boundary cases, fed to the decoder directly by hand-building a byte-valid store file (the encoder normalises integral values to %d, so a near-integer can't reach the decoder through store_put):

_eigs_shape	decision
[2,2], 4 elems	accept
[2.0000000000000004,2] — 1 ULP above 2	reject
[9007199254740993,1] — 2^53+1	reject
[1e300,0]	reject
[-0.0,-0.0]	accept as 1-D
[2147483648,1] — INT_MAX+1	reject
[2147483647,0] — INT_MAX	accept
[-1,1] · [2.5,2] · [-1e300,0]	reject
[0,5], 0 elems	accept (degenerate)

11/11 correct, in both directions. The second row is the direct refutation of what the alert implies.

But line 320 — the cast that produces those operands — was a real defect, and it is now fixed. int rows = (int)rv->data.num is undefined when the double is outside int's range (C11 6.3.1.4p1), and the decoder is handed whatever double the file contains. It fires on 4 of the 11 cases above.

Worth knowing why the ASan+UBSan job never saw it: GCC's -fsanitize=undefined does not include float-cast-overflow. I verified that rather than recalling it, then rebuilt the interpreter with the check enabled:

ext_store.c:320:29: runtime error: 1e+300 is outside the range of representable values of type 'int'
ext_store.c:320:29: runtime error: 9.0072e+15 is outside the range of representable values of type 'int'
ext_store.c:320:29: runtime error: 2.14748e+09 is outside the range of representable values of type 'int'
ext_store.c:320:29: runtime error: -1e+300 is outside the range of representable values of type 'int'

The answers were right only because x86-64's cvttsd2si returns INT_MIN on overflow and line 321 then rejected it — correct by hardware accident, not by the language, and not something -O2 is obliged to preserve.

The fix range-gates before narrowing, which also folds in the now-redundant rows < 0 || cols < 0 check and rejects NaN via the negated form. Post-fix: byte-identical decisions on all 11 cases, zero runtime error: lines. Suite 3394/3394, [56] 52/52.

Two things for you rather than for the diff:

• If CodeQL still flags line 321, it should be dismissed as "intentional exact integrality test on a parser-produced value" rather than given an epsilon. The table above is the justification.
• The same UB pattern is pre-existing in the encoder and I have not touched it. store_json_encode's number path is if (n == (int)n && fabs(n) < 1e15) — && evaluates left to right, so (int)n runs before the magnitude guard. Confirmed with the same instrumented build. Out of scope here; happy to file it or fix it separately, whichever you prefer.

One correction to my own PR body: the branch adds 30 counted checks, not 32 (22 → 52 in that file). I have corrected the body.

[InauguralPhysicist]

Approving. Verified on current main (8de9715 + #811):

• Planted fault reproduced exactly: your test file against main's ext_store.c fails with FAIL: buffer field decodes as a buffer — expected buffer, got none; with your ext_store.c restored, [56] reports all 52 checks and the standalone run is 52/52 under ASan+UBSan with detect_leaks=1 — the new decoder paths (near-miss rejections, sentinels, nesting, reopen) are sanitizer-clean.
• The decoder's hostile-input posture is right: range-gate before the int narrowing with the UB citation, integrality by exact equality (correct operator, as you say — a tolerance would be the bug), the shape invariant in int64 so rows*cols can't overflow, elements validated before the allocation, and the triple-set restricted to what the constructors can produce. The xcalloc + manual-fill construction matches the buffer idiom used by read_bytes_buf and friends, so no allocator-discipline mismatch.
• The judgment calls all land where I'd have put them: tagged over numeric-array (type fidelity is the point of the fix), STORE_VERSION not bumped (bricking every existing database to recover already-lost data would be backwards), the collision surface stated as a real residual rather than argued away — and the _id stamp making top-level records immune is the observation that makes it acceptable.
• Keep the matmul-coupled sentinel check. You offered to drop it; don't. It pins the only coverage of the non-finite path, and if num_guard ever lands in ne_matmul_buf the failure will be loud, explainable, and a one-line test fix — better than the path silently going uncovered.

Merging.