From 098d404fc486d677112e76c29e661f65e49ee754 Mon Sep 17 00:00:00 2001 From: Alan Liu Date: Thu, 10 Sep 2026 21:55:35 -0400 Subject: [PATCH] Drop the .loop scratch state from the tree The /polish loop's working files -- polish-seen.md and polish-state.md -- rode into main with #127. They are per-run agent bookkeeping, not project artifacts: nothing in the tree, the build, or CI reads them, and they go stale the moment the branch they describe lands. Remove both and ignore .loop/ alongside .claude/ so a later run cannot commit them again. --- .gitignore | 1 + .loop/polish-seen.md | 174 ------------ .loop/polish-state.md | 616 ------------------------------------------ 3 files changed, 1 insertion(+), 790 deletions(-) delete mode 100644 .loop/polish-seen.md delete mode 100644 .loop/polish-state.md diff --git a/.gitignore b/.gitignore index c2e081fa2..f1e0af96c 100644 --- a/.gitignore +++ b/.gitignore @@ -20,6 +20,7 @@ ENV/ .vscode/ .idea/ .claude/ +.loop/ *.swp *.swo diff --git a/.loop/polish-seen.md b/.loop/polish-seen.md deleted file mode 100644 index fdf43c3f2..000000000 --- a/.loop/polish-seen.md +++ /dev/null @@ -1,174 +0,0 @@ -# Findings seen this run - -Keyed `file:line|summary`. Every finding ever REPORTED this run is listed, -whether it was later confirmed or refuted — dedup is against seen, not -against confirmed, so a refuted finding cannot resurface each round and keep -the loop alive forever. - -## Round 1 - -### simplification lens (10 reported, 6 more beyond its cap) - -Impact classes are the reviewer's; the orchestrator re-checked the factual -claim of every one it lists as duplicated (greps below), because a -simplification lens that invents duplication would waste a builder. - -- schema.cpp:385 | inline legacy-object refusal loop unreachable after the [[noreturn]] call at 384 | **requirement** | CONFIRMED, impact DOWNGRADED to optional -> human list, not queued -- clickhouse_client.cpp:1022 | 33-line ClientOptions construction copied into both ThreadInit overloads, plus identical 9-line catch tails | optional -- pack_index.cpp:41 | local `quoted_string` is a character-for-character copy of the exported `sql_quote` already in scope | optional | FACT-CHECKED: pack_index.cpp:42 defines it, catalog_writer.h:44 declares sql_quote, pack_index.cpp:8 already includes that header -- reader.cpp:556 | row-validation/flatten block duplicated between search and get_by_ids, both messages included | optional -- catalog_writer.cpp:61 | the byte-budget chunker exists three times (two here over different element types, one open-coded in reader.cpp with the budget as a literal) | optional -- hydration.cpp:270 | estimate and hydrate each re-implement the same group/sort/coalesce planner, and a comment says they must agree | optional -- catalog_writer.cpp:218 | `quorum_write()` defined identically in three classes; `qualified()` in three | optional -- conformance_catalog.cpp:334 | three verbatim S3Config+reader blocks, two Selection decodes, two 13-line item renderers | optional -- clickhouse_client.cpp:1192 | timed-insert metrics wrapper duplicated verbatim between the two InsertBatch implementations | optional -- reader.cpp:538 | `order` is constructed always equal to `grouped`; the sort-key join is spelled four ways in one file | optional | FACT-CHECKED: reader.cpp:538-546 appends the same value to both under one condition tested on `grouped` - -Reviewer also recorded as already tight, no findings: lease_coordinator.cpp, -version_allocator.cpp, indexer.cpp, common/json.cpp, ring/ headers. - -### correctness lens (10 reported, 5 beyond cap) - -- hydration.cpp:586 | element budget multiplies shape_product by the element count -> prod(shape)^2 | high | CONFIRMED/correctness -> QUEUED (129). Live: shape (16384,) refused at 268435456 > 64000000 where Python summarises; boundary confirmed at prod(shape)>=8001 -- clickhouse_client.cpp:111 | POSTFIELDS without POSTFIELDSIZE (NUL truncates SQL); substitute escapes only 4 chars | high | CONFIRMED all 3 parts/requirement -> QUEUED (128). Live: server recorded raw 0x0D vs Python's `\r`, and recorded SQL truncated at the NUL -- pack_index.cpp:179 + indexer.cpp:36 | pack_id interpolated into SQL unescaped while neighbours are escaped | high | CONFIRMED/correctness, severity -> med on reachability -> QUEUED (128). Live: crafted pack_id returned ok:true and appended a fully attacker-chosen inventory row -- hydration.cpp:692 + :646 | abs_max_int chosen by comparing doubles, can return negative; int64 cast from widened double | high | CONFIRMED both parts/correctness -> QUEUED (129). Worse than reported: [2**63-2, 2**63-1] summarises as INT64_MIN in BOTH min and max -- spool.cpp:277 | file I/O moved outside the lock makes the pack-conflict check racy | med | **REFUTED**. The TOCTOU mechanism is structurally real, but BOTH asserted harms were disproved by execution. "One silently overwrites the other": false — `::link` cannot overwrite, it returns EEXIST and :374-389 handles the loser path; and when intents differ the ready names differ anyway, so link never collides (10/10 runs produced two distinct intact files). "The accounting double-counts": false — entries/bytes matched on-disk truth exactly in both interleavings, and the EEXIST loser's unreserve() cancels its reservation precisely (5/5 runs). Unreachable besides: the only threaded consumer is PackSink::RunStager, where every pack id is a fresh UUID v4 assigned immediately before its builder, so the race needs a UUID collision; the conformance driver is single-threaded and re-Opens per op. - Also worth recording: the oracle IS stricter in-process (it holds its lock across the write, and refuses the second intent with PackConflictError), but threading.Lock is process-local, so for the cross-process case Python has the identical hole and parity holds. The oracle's uploader explicitly documents co-existing same-pack_id ready files as a state it must tolerate (spool.py:547-550), and its own conflict test is purely sequential — a shape native already passes. - Residual, comment-level only: spool.h:12-13 states the same-pack_id-different-intent refusal without qualifying that it is best-effort under in-process concurrency. Explicitly NOT to be "fixed" by moving the write back under the mutex — that would serialise all stagers on disk I/O (the stated reason for the split) and still not close the cross-process hole. -- spool.cpp:190 | ".." tested as substring, not component -> legal key `a..b` rejected, and the failure LATCHES the sink | med | CONFIRMED/correctness -> QUEUED (127). Live: tenant `a..b` staged fine in Python, latched the native sink to `closed` for all later submits -- pack_builder.cpp:353 | ParseUuid result discarded -> zero-UUID pack sealed, caught only after writing | med | CONFIRMED mechanism, impact DOWNGRADED to optional, severity -> low -> human list, NOT queued. Two of the finding's three assertions were REFUTED: kBadArgument IS reachable (seven ValidateMetadata checks produce it; only the *pack_id* word in the header comment has no producer), and a zero UUID is a VALID uuid both sides accept byte-identically. The real symptom is an empty footer pack_id beside a zero header UUID. Unreachable in production: the only non-test caller feeds NewPackId(), which is always canonical, and SinkConfig has no pack_id field, so unlike Python's injectable pack_id_factory there is no injection surface at all. Python's PackReader also rejects such bytes on read. -- uploader.cpp:221 | oversized pack recorded per-pack while the comment claims the oracle's up-front batch refusal | med/requirement | CONFIRMED/requirement, severity med -> **NOT queued, escalated to the human**. Live head-to-head on an identical 3-small-plus-one-oversized batch: Python raised ValueError with 0 PUTs and all 4 packs left staged; native returned ok:true with 3 PUTs, 1 pack left staged, snapshot attempted=4/uploaded=3/failed=1. Every observable differs, and the comment at :222-223 plus uploader.h:87-88 affirmatively assert the parity they violate. - WHY NOT QUEUED: the current native behaviour looks DELIBERATE, not accidental. Two purpose-named tests pin it green (`test_pack_over_the_byte_gate_fails_fast` and `test_mixed_batch_reports_oversized_pack_at_its_position`, tests/test_native_uploader.py:219,252, both asserting ok:true with uploaded_packs == 3), and docs/benchmarks.md:483 records a related accounting decision. The fix also needs an error channel added to UploadPending's signature. Choosing the oracle's all-or-nothing policy over the port's more forgiving one is a design decision with a paper trail behind it, so it is the human's call, not the loop's. The verifier notes native's policy is "arguably the more sensible" one; what is unambiguously broken either way is the comment asserting parity. -- indexer.cpp:105 | conflict failures emitted per input ref, not per distinct claimant | med | CONFIRMED/correctness, severity -> low -> QUEUED (128). Live: [A,A,B] gave native requested/failed 3/3 vs oracle 2/2. The verifier also found a SECOND divergence from the same loop needing NO duplicate refs: two interleaved conflicted identities produce native order [a1,b1,a2,b2] vs oracle [a1,a2,b1,b2], and ordering is load-bearing because IndexResult.merge truncates with failures[:failure_limit]. Reachability caveat: indexer.cpp compiles only into conformance_catalog and NativeIndexer::index has exactly one caller, that driver — so the blast radius is the parity harness itself, which is the port's contract. -- indexer.cpp:107 | NEW, found by the indexer verifier while verifying the above: the failure message is the bare "conflicting pack identity" while the oracle emits f"conflicting pack identity: {identity!r}" — an UNCONDITIONAL message divergence on every conflict, independent of claimant counting | requirement | queued with its neighbour (same emission site, same builder) -- reader.cpp:475 | cursor `captured_at_ns` passed through as a raw JSON token, no integer validation | med | CONFIRMED/correctness -> QUEUED (129). The worst outcome here is a SILENT WRONG ANSWER, not a refusal: with k[3] = 2**64 + 1700000000000002002, ClickHouse wraps modulo 2^64 (verified directly: `SELECT toUInt64('18446744073709551623')` -> 7), so native returned ok:true with a page starting at the wrapped position and `capture-1` silently missing, where the oracle raises InvalidCursorError "must fit UInt64". - The verifier drove 12 crafted values and separated the two halves honestly: 10 of 12 malformed forms DO fail closed but with the wrong error class (ClickHouseError / Code: 53 instead of the ValueError the sibling test pins), and only the oversized case yields a wrong answer. Escaping was confirmed to neutralise INJECTION (`0') OR 1=1 --` came back correctly escaped) but does nothing about numeric semantics, which is the actual claim. - Confirmed NOT already covered by this session's earlier cursor fixes: the v / field-set / member-count / fh / w checks never touch `k`, and find_uint_in works by object key so it cannot see an array element. The oversized cursor sails past all of them. - Secondary note from the verifier, same root cause: even HONEST cursors carry captured_at_ns as a quoted string and work only via ClickHouse's implicit String->UInt64 tuple coercion — that coercion is what wraps. Fix should carry it as a typed uint64_t param so it renders bare-numeric. - -### consistency lens (10 reported, 3 beyond cap) - -- schema.cpp:127 | rebuild_instruction() omits legacy_objects_, so an operator who follows it is refused forever | high/requirement | CONFIRMED/requirement, severity -> med -> QUEUED (128). Live: followed the message text three times, refused every time; Python named 10 objects and succeeded on rerun -- schema.cpp:391 | the dead legacy loop again (same as simplification #1) — DUPLICATE, already verified CONFIRMED/optional -- schema.cpp:162 | require_catalog_visibility() probes only objects_, not the legacy object whose presence refuses | med/requirement | CONFIRMED/correctness, severity -> med -> QUEUED (128). Measured probe sets off system.query_log: oracle issues 10 CHECK GRANT statements including `_pack_commit_log`, native issues 9. Head-to-head on one catalog with only the legacy object's visibility restricted: native `verify_compatibility` returned {"ok":true,"state":"complete"} and `ensure_schema` returned ok, where the oracle refused with "lacks SHOW TABLES ... Grant catalog visibility". So the native path SUCCEEDS SILENTLY in exactly the "two builds share one prefix, captures are invisible" scenario the refusal exists to catch. - NOT a duplicate of schema.cpp:127: that one is the drop-list TEXT inside a refusal (victim: an operator following a printed list); this one is the GATE, where no refusal is emitted at all. Different function, different observable, and fixing either leaves the other broken. - Verification method worth remembering: this host's ClickHouse has NO access management (`CREATE USER` -> Code: 497), so the verifier could not build a real restricted role. It reproduced grant filtering with a local HTTP shim in front of 8123 that answered CHECK GRANT with 0 and dropped the row from system.tables — the same technique the oracle's own unit test `_OneObjectHidden` uses — and said plainly that it could not create a role. The shim log was itself decisive: 3 filtered system.tables reads, 0 CHECK GRANT hits for the legacy name. - Refuted counter worth recording: "probing an absent object would refuse healthy catalogs" is false — `CHECK GRANT SHOW TABLES ON default.nonexistent` returns 1, and schema.cpp:160-161's own comment says the probe needs neither database nor objects to exist. -- catalog_writer.cpp:21 | escaper handles 4 of the 10 characters the driver escapes, under a byte-parity claim | med/requirement | CONFIRMED/requirement -> QUEUED (128), same fix as correctness #2 -- pack_sink.cpp:64 | constructor validates only num_workers; every other bound the oracle refuses becomes a latch or a silent all-drop | med/requirement | CONFIRMED but HEAVILY SCOPE-CORRECTED, impact DOWNGRADED to optional, severity low -> human list, NOT queued. The reviewer's "every OTHER bound" is false. Of the 5 bounds PipelineConfig enforces, only 3 diverge into latch/all-drop (max_queue_records, max_queue_bytes -> all-drop; max_pack_records -> a runtime LATCH, sink dead after 1 record). Two degrade benignly (max_pack_bytes -> clean per-record `too_large`; max_linger_ns=0 -> works, one pack per record). Three adjacent bounds the claim implies were REFUTED outright: overload policy is an `enum class` with no invalid value representable; a negative admission_timeout is the DOCUMENTED encoding of Python's None (pack_sink.h:67-68), not out-of-range; and spool_max_bytes=0 is already refused at Start() with a byte-identical message to the oracle. - Not queued because production construction is fully guarded one layer UP, deliberately: src/dmi/storage/capture/native_sink.py:42-56 refuses every one of these fields with "must be positive" before the C++ sink is built, and its docstring says so ("Field-by-field the same contract as the pipeline config the reference sink takes"), pinned by tests/test_native_rollback.py:129-139. The reachable surface is the conformance driver and the test-only pybind init. - Useful detail for whoever picks this up: `unsigned` types buy nothing here — 0 is exactly what the oracle refuses and is storable, and the driver's static_cast turns a negative into UINT64_MAX (max_queue_records=-1 gave an effectively unbounded queue). The verifier also refuted the scarier reading that kRecordLimit is latent with VALID configs: pack_sink.cpp:663 keeps `record_count < max_pack_records` at every loop top, so it fires only for the 0 config. Recommended fix is in Start(), not the constructor, since Start() already owns the error channel. -- pack_builder.cpp:353 | ParseUuid discarded, and the header documents a kBadArgument no path returns | med/requirement | duplicate of correctness #7 — and the "no path returns kBadArgument" half was REFUTED outright by the verifier -- spool.cpp:34 | IsUuid accepts uppercase hex; Python's ready-name grammar is lowercase-only, breaking the stated on-disk contract | med/requirement | CONFIRMED, impact DOWNGRADED to optional, severity low -> human list, NOT queued. The consequence is worse than reported where it is reachable: Python does not merely skip an uppercase ready file, it QUARANTINES it (`_quarantine`, spool.py:231-234), renaming and unaccounting the pack. Verified live in both directions — native staged an uppercase name and later recovered it as live, while the same input made Python rename it to `.dmi-pack.quarantined` with entries dropping 1 -> 0. Not queued because the only in-tree production caller feeds NewPackId(), which is lowercase by construction, so no real handoff can carry an uppercase name today. Notable house-pattern inconsistency: PackBuilder::ParseUuid is deliberately case-insensitive but CANONICALIZES to lowercase; IsUuid copied the leniency and dropped the canonicalization, then uses the raw string as a filename. Fix is a one-line deletion of the A-F branch; the verifier warns against lowercasing inside Stage instead, since the oracle refuses rather than repairs. -- s3_sign.cpp:75 | headers ordered by RAW map key while only the emitted text is lowercased | med/requirement | CONFIRMED, impact DOWNGRADED to optional -> human list, not queued. The reviewer's own example does NOT reproduce; verifier had to find `X-Amz-Meta-Zed`, then showed every production path lowercases before signing -- lease_coordinator.cpp:37 | quorum_write copied three times where the oracle documents one definition as a correctness requirement | med/optional | duplicate of simplification #7 -- reader.cpp:396 | search() ports one CaptureQuery bound and drops the siblings | med/requirement | CONFIRMED 3 of 4 sub-claims/requirement -> QUEUED (129). Live: 1025 layer_numbers served a 4-item page where Python refuses. The reviewer's "unbounded IN list" consequence was REFUTED — the server fails closed with Code: 62 - -### test-coverage lens (10 reported) - -- spool.cpp:190 | symlinked parent escapes the root: pack written outside, acknowledged staged, then invisible to recover — DEMONSTRATED by the reviewer running the driver | high/correctness | CONFIRMED/correctness, severity -> med on reachability (needs a pre-existing symlink; object_key is never caller-supplied in production) -> QUEUED (127). Loss is permanent, not just invisible: recursive_directory_iterator does not follow directory symlinks, so the pack is never uploaded and its bytes stay charged against the spool budget forever -- uploader.cpp:122 | the preflight's re-read-and-re-hash of a pre-existing object is untested; a regression would bless corrupt bytes and delete the only good copy | high/requirement | **SPLIT: (A) coverage gap CONFIRMED, (B) claimed consequence REFUTED by execution.** Net impact optional, severity low -> human list, NOT queued. - (B) is the reverse of the claim. The verifier staged a golden pack, uploaded it, restored the local copy, then mutated the REMOTE body in place at the same length with `dmi-sha256` metadata untouched — the exact "cleanup delete also failed" state. Native REFUSED: `{"ok": false, "what": "pre-existing object failed verification"}`, did not overwrite the corrupt remote, and left the good local copy intact with `spool.entries == 1`. "Blesses corrupt bytes and deletes the only good copy" is precisely what this code PREVENTS. A control run with the bytes restored returned ok and showed calls [HEAD, GET], proving the re-hash does execute on the happy path. - (A) is real and worth acting on anyway, for a reason the reviewer did not give: `test_second_upload_is_a_preflight_hit` (test_native_uploader.py:200) asserts only `puts_after == puts_before`, and that assertion SURVIVES DELETING lines 120-130 entirely — so nothing pins the re-hash at all. The mismatch sub-branch is wholly unexercised (`grep "pre-existing object failed" tests/` finds only the source). And the oracle DOES pin it: `tests/test_capture_s3.py:467 test_a_spooled_pack_survives_a_retry_that_finds_a_corrupt_remote_object` passes. Python pins a safety property the port does not. - Any test added here PASSES IMMEDIATELY — it is a regression guard for a property the code already has, with no red phase available. Recommended: add the mismatch test, and strengthen the existing preflight-hit test to assert a GET appears in the recorded calls. - Incidental, unevaluated: on mismatch Python treats it as terminal while native burns all `max_attempts` re-HEADing and re-GETting first. Both fail safe; parity of the ERROR TAXONOMY is a separate question nobody has looked at. -- hydration.cpp:475 | the catalog-descriptor <-> pack-footer binding, the only cross-check between row and object, is untested | high/requirement | **CONFIRMED and RECLASSIFIED — this coverage finding was hiding the run's worst bug.** The bare coverage claim is low/optional; what it concealed is two correctness defects, both QUEUED (129): - (1) PROMOTED, high/correctness — hydration.cpp:459, the footer-row splitter is BRACKET-BLIND, so it over-splits the shape array's own commas and every field from 19 on shifts. EVERY CAPTURE OF RANK >= 2 IS REFUSED AT HYDRATION natively while the oracle hydrates it fine: (4,3), (1,12) and (2,2,3) all fail with "catalog descriptor does not match the pack footer: field 20" where Python returns matching payload bytes. Every real activation tensor is rank >= 2. Damning detail: this repo ALREADY FIXED this exact bug class in the SELECT-row splitter (`parse_tsv_tuple` got `[`/`]` depth tracking earlier this session) and left it in the sibling footer splitter — and test_search_parity_on_a_multi_dimensional_shape:265 exists with a docstring spelling out that a non-bracket-aware splitter "breaks every real activation, since the only shapes that survive are rank 1". The lesson was written down and not applied next door. - (2) correctness — hydration.cpp:475's `kCompared` (:484-488) checks 9 fields where Python compares the whole CaptureMetadata dataclass plus the 5-tuple _record_locator (reader.py:370-382). Measured: mutating `layer_number=99` or `hook_name=evil` in the catalog row against a consistent footer gives ORACLE "FAILED metadata" vs NATIVE "OK payload_matches=True" — silently served. - Reviewer framing downgraded on one point: "the ONLY cross-check between row and object" is overstated — the phase-two payload CRC32 (hydration.cpp:542-550) also checks stored bytes. The binding is the only METADATA cross-check. - Why no test caught it: every hydrate test uses a rank-1 shape (`_stage`'s default is `shape=(len(payload),)`), the budget test refuses before the binding runs, and the one rank-3 test calls `search` only and never reaches `hydrate` — which is exactly how the footer splitter escaped. Python covers the binding three ways (test_capture_storage.py:577, :607, :611). -- pack_index.cpp:203 | ~15 refusals over externally-supplied footer bytes, none exercised natively (Python tests the whole family) | high/requirement | **CONFIRMED and RECLASSIFIED UP from coverage to correctness -> QUEUED (128).** Second coverage finding this round to be hiding a real bug. - Count: the real family is 28 sites / 24 distinct messages — exactly 15 in the cited function, so "~15" fairly counts `read_pack_descriptor_rows` but undercounts the family it enforces by ~45%. Split: 1 reached incidentally by an existing test (and its message not asserted), 1 UNREACHABLE, 26 unreached. Python's coverage is NOT patchy — a 13-case parametrized matrix plus 5 dedicated tests in test_capture_pack_format.py. - THE BUG: `render_record_row` copies every footer metadata field straight into SQL and validates only the `dtype x shape == decoded_length` product, where the oracle runs full CaptureMetadata validation at the same boundary (pack.py:467 -> model.py:124-177). Nine executed accept/refuse divergences at -O2, including shape dims above 2^31-1, rank 40, `producer_rank=-1`, `layer_number=-5`, `token_end < token_start`, `captured_at_ns=-1`, empty capture_id. The decisive case is ClickHouse-representable so it LANDS: `shape=[0, 4294967295]` fits Array(UInt32), native inserts the row, and clickhouse_reader.py:625 then refuses it on read-back — one hostile footer poisons a whole search page. ClickHouse does not fail loudly on the out-of-range integers either; it wraps silently (`-1` stored back as 4294967295 / 18446744073709551615). - Severity med, not high, because both builders validate before writing a footer (pack_builder.cpp:274-284) and the footer CRC catches bit rot, so this needs a CRC-consistent footer from a hostile or foreign producer with bucket write access. But the reader-side layer is exactly the defence-in-depth pack_index.h:1-6 promises and does not deliver, and the footer IS re-read from the store at query time in a different process (hydration.cpp:425-432). - Also queued with it: four wrong-message divergences (records-not-list, duplicate capture ID, offset-0, footer-not-json) where both sides refuse but the text differs from the oracle's. - REPORT-ONLY, not fixed: pack_index.cpp:214 "pack trailer is truncated" is DEAD CODE in the shipped build — s3_client.cpp:343 returns false unless the body length is exactly the requested length and kTrailerSize is 64, so the condition is impossible. Python's equivalent IS reachable because store.read_range may return a short slice. No test can cover it; the human decides between deleting it and making it an assertion. -- pack_index.cpp:62 | the dtype-width table is only ever driven with uint8; 13 of 14 admitted dtypes never reach it | high/requirement | **REFUTED on all three parts.** The premise is factually wrong: 6 of 14 dtypes and 3 of the 4 width branches are already exercised at the frozen baseline — uint8 (the _stage default), float16 (test_native_reader_parity_live.py:893 -> op="index"), and float8_e4m3fn/float8_e5m2/uint32/uint16 (test_fp8_and_wide_int_summaries_parity -> op="hydrate"). Commit a5e15a2 is what added them. Off by five dtypes and two width branches. - No width bug either: the verifier COMPILED the frozen `dtype_bytes` and diffed its output against `_DTYPE_BYTES` parsed out of model.py — 14 vs 14 names, zero width disagreements. And a wrong width would be fail-closed anyway: the width's only consumer is the neighbouring `logical != decoded` guard at pack_index.cpp:143-146, it is never rendered into any of the 32 output fields, and feeding a deliberately wrong width throws "record length does not match metadata dtype and shape" on the first pack. An untested entry is self-policing. - Honest limit the verifier stated: it could not run the live suites (no clickhouse_connect in its interpreter), so part B is call-graph tracing rather than execution; parts A and C ARE execution of the frozen source. - INCIDENTAL, not the reviewer's finding, worth the human's attention: pack_index.cpp:74 admits `uint64` -> 8, which is in NEITHER authoritative list, and the oracle rejects it (`unsupported dtype: 'uint64'` from CaptureMetadata.__post_init__). DtypeSupported guards only the write path, so a hand-forged footer with dtype "uint64" indexes natively and is then refused by Python. Unreachable (neither builder can emit such a pack) but note it is the SAME laxity my 129 builder deliberately closed on the hydration side, so the two files now disagree — hydration refuses uint64, pack_index admits it. Also the comment at pack_index.cpp:60-61 still says "the same ten" when the set is 14. -- hydration.cpp:101 | bfloat16 and int64 summary decode branches unexercised, and int64 order stats provably diverge from the oracle | med/requirement | not yet verified -- hydration.cpp:193 | select()'s two refusals (exceeds one page, matches nothing) never driven | med/requirement | not yet verified -- reader.cpp:443 | two cursor refusals (filter-hash mismatch, watermark ahead of head) untested natively | med/requirement | not yet verified -- pack_sink.cpp:472 | the whole staging-failure latch path unexercised: every test opens the spool at 1<<40 bytes | med/requirement | not yet verified -- indexer.cpp:74 | max_packs never tested at either side of the limit, though its byte-budget twin is | low/optional | not yet verified - -## Found by a /code-review of the LANDED 129 diff — one is a defect in my own fix - -Five findings against `7118b6b..3af38c3`. Mechanism of the first VERIFIED by -me; the other four are unverified reviewer claims. - -- reader.cpp:471 | **high, mechanism verified** — the cursor's `v` and `w` are STILL on the wrapping path that `7118b6b` claims to have closed. `find_uint_in` (reader.cpp:185) reads them via `jc::FindInt`, whose accumulator is `int64_t v = 0; v = v * 10 + digit` with NO bound check (common/json.cpp:71) — signed overflow, and only `value < 0` is checked afterwards. So `"w": 2**64+1` wraps and is accepted where `decode_cursor` raises "cursor watermark must fit UInt64"; the reviewer reports the same trick on `v` walking straight through the version gate. THIRD instance this run of the fix-one-field-miss-the-sibling pattern (after the SELECT vs footer splitters, and the hydration vs pack_index dtype laxity). Worse, my commit comment asserts `captured_at_ns` "was the one component that travelled through UNVALIDATED", which is now demonstrably false — the comment needs correcting whether or not the code does. Fix: hoist the new `number()` parse and share it with `find_uint_in`. -- reader.cpp:400 | medium, unverified — the ported bounds omit `_validate_text` on `hook_names` and the five identity fields (model.py:298-301, 308-309), so `tenant_id=""` or a >512-byte `run_id` returns an empty page where CaptureQuery raises. My dispatch explicitly scoped this out ("Hook-name text validation is still unported; the finding named only the three bounds"), so this is known-incomplete rather than missed. -- reader.cpp:516 | low, unverified — `number()` accepts leading zeros (`007`), which `json.loads` rejects outright, re-opening "native accepts what the oracle refuses" in the very field just hardened. -- clickhouse_client.cpp:52 (129) / :34 (128) | **CONFIRMED against 128's rewritten version, severity med, correctness -> QUEUED (128).** Upgraded from the reviewer's "low", and NOT moot on 128: PR 128 rewrote only the ESCAPING half of `substitute()` (inline escaper -> shared sql_quote) and left the scanning half verbatim. The inner `from` bookkeeping guards only self-recursion; the OUTER per-parameter reset to 0 is the bug. The oracle never rescans — clickhouse_driver/client.py:952 is literally `return query % escaped`. - Reachable on 128 ALONE, no 129 code needed: LeaseCoordinator::acquire (lease_coordinator.cpp:45) validates the caller-supplied holder by LENGTH ONLY, correctly matching the oracle, so `%(ttl_ns)s` is a legal 11-byte holder — and in insert()'s map, `holder` sorts before `lease_id`/`term`/`ttl_ns`, so it is substituted first and every later placeholder reaches into it. Measured off system.query_log: oracle stored `%(ttl_ns)s`, native stored `30000000000`; `%(term)s` -> `2`; `%(lease_id)s` -> hard refusal Code: 62 from doubled quotes; a plain holder matched, which is what proves the divergence is specific to a placeholder inside a value. The numeric cases are the dangerous ones — valid SQL, silently wrong value. - REVIEWER CORRECTED: their predicted cursor outcome (a silently-empty page from `tenant_id > 't'`) is WRONG. A string parameter renders WITH quotes, so `'%(tenant_id)s'` becomes `''t''` and the server refuses; verified `SELECT ('t') > (''t'')` -> Code: 62. The silent variant is 128's numeric one. Their non-injection caveat was right. - Why the parity gate I added missed it: the driver exposes a session-less `escape` op for sql_quote parity (conformance_catalog.cpp:216) but has no `substitute` equivalent, so the gate covered the escaper and never the scanner. -- conformance_catalog.cpp:496 | low, unverified — the harness casts time bounds `int64_t -> uint64_t`, so `captured_after_ns: -1` becomes UINT64_MAX and the oracle's non-negativity check has no reachable native counterpart. This is the driver-side gap the reader builder flagged and was told was out of scope. - -## Round 2 outcome — all four 129 review findings FIXED (a6b5260, 40e5ada, 343bef6, dee5d95) - -Every claim reproduced; none refuted. Red-checked independently by reverting -`reader.cpp` + `conformance_catalog.cpp` to `7a22e66`, rebuilding, and getting -exactly four failures, one per finding, then restoring to 36 passed. - -Two claims turned out WORSE than reported: -- `v = 2**64+1` does not merely pass the version gate, it served a FULL PAGE - of 4 rows where Python refuses with "unsupported cursor version". The `w` - case gives the empty page the reviewer described. -- leading zeros: `k[3]=007` did not just "page from position 7" — the page - STARTED AT capture-0, i.e. a cursor paging BACKWARDS over rows already - served. That is duplicate delivery, not a shifted offset. - -The fix hoists one bounded parse (`parse_cursor_uint`) plus a `find_token_in` -to recover the raw spelling that `jc::FindInt` destroys, and routes `v`, `w` -and `k[3]` through it. `reader.cpp` no longer calls `jc::FindInt` at all. - -NEW FINDING the builder surfaced and correctly declined — the same family -problem a fourth time: `jc::FindInt` in `native/csrc/common/json.cpp` is STILL -an unbounded `int64_t` accumulator for its ~100 other call sites (sink, store -and pack drivers, `pack_index.cpp`). The cursor path no longer depends on it, -but nothing else was audited. Fixing it there changes shared JSON semantics in -files PR 128 also touches, so it needs its own finding and its own review — -this is the root the three sibling misses grew from. - -Also flagged, deliberately not pinned: a captured bound above UINT64_MAX is -now refused at the harness ("does not fit UInt64") where the oracle accepts it -at `CaptureQuery` and fails later in its driver's parameter binding. Refusing -beat wrapping and the wire field cannot hold it, but it is a knowing -divergence rather than parity. - -## Found DURING the fix round (not by a lens) — for the human, not fixed - -- indexer.cpp `unique` list ordering | the non-conflicted list has the same ordering divergence the conflict emission had: native walks `by_identity` as a `std::map` sorted by `(store_id, pack_id)` where Python's dict yields first-appearance order. Affects descriptor/inventory row order and the published member list, so the blast radius is wider than the conflict-emission finding it sits beside. Surfaced by the builder fixing indexer.cpp:105, which correctly declined to widen its own scope. NOT verified by a verifier — treat as a lead, not a confirmed defect. -- hydration.cpp:459 rank-2 footer misalignment | the pack-footer row parser splits on `,`, so a rank-2 shape like `[128,128]` misaligns the footer field comparison and fails with "catalog descriptor does not match the pack footer: field 20" BEFORE the intended check. Hit accidentally by two different agents this run while constructing tests. Whether a legitimate rank-2 capture can be hydrated at all is an open question and is being verified as part of the hydration.cpp:475 test-coverage finding. -- minimum/maximum wire lossiness | `CoreSummaryData::minimum`/`maximum` are `double`, so int64 magnitudes above 2^53 stay lossy on the wire even now that `minimum_int`/`maximum_int` are exact. Same wire-widening decision as `abs_max_int`. Surfaced by the builder fixing hydration.cpp:692. - -## Pre-excluded (already fixed earlier this session, told to reviewers) - -These were found and fixed before this polish run, and the reviewers were -given the list so they would not re-report them. Recorded here so a later -round cannot rediscover them as "fresh": - -- catalog_writer.cpp | manifest chunk read-back compared against chunk length, not distinct count -- indexer.cpp | erase from the sequence being iterated -- indexer.cpp | batch-byte guard thrown inside its own try -- indexer.cpp | publish-exhaustion guard unreachable (break skipped the increment) -- catalog_writer.cpp | quarantine never cleared after its window -- reader.cpp | cursor built from the look-ahead row -- reader.cpp | float16 subnormal decode assembled from a double's bytes -- reader.cpp | TSV escape layering (grouped columns vs tuple contents) -- reader.cpp | cursor envelope checks missing (v, unexpected/missing fields, byte limit) -- reader.cpp | empty cursor-key component accepted via raw-token fallback -- reader.cpp | published_head sent no bounded read settings -- reader.cpp | empty watermark passed the digit check vacuously -- reader.cpp | get_by_ids lacked the resolved-tuple width check -- conformance_catalog.cpp | empty JSON array became a filter on the empty string -- conformance_catalog.cpp | four duplicated op handlers (dead second copies) -- catalog_writer.cpp | parameterized statements not byte-identical to the driver's rendering -- catalog_writer.cpp | publishes not serialised per writer; no process binding -- pack_sink.cpp | std::runtime_error thrown without -- reader.cpp/hydration.cpp | dead helpers (find_string_in, is_signed_dtype, unwired unescape_tsv) -- pack_index.cpp | unused bucket parameter diff --git a/.loop/polish-state.md b/.loop/polish-state.md deleted file mode 100644 index 4bc82e5e5..000000000 --- a/.loop/polish-state.md +++ /dev/null @@ -1,616 +0,0 @@ -# Polish run — C++ native path - -Scope: `native/csrc/` (the C++ native path), as chosen by the human. -Landing rule, also from the human: each fix lands on the PR that OWNS the -file, then the stack is forward-merged so all three PRs carry it. - -## Baseline refs (Phase 0) - -Tree clean at start. One baseline per branch, because the scope spans three -stacked PRs rather than one: - -| PR | Branch | Baseline ref | -|---|---|---| -| 127 | `feat/native-capture-pipeline` | `92c2225` | -| 128 | `feat/native-catalog-b1` | `7eac029` | -| 129 | `feat/native-c-reader` | `a4d3dcb` | - -`a4d3dcb` contains both others (verified with `git merge-base --is-ancestor`), -so review runs once against C and sees the whole native path. - -## File ownership (routes each fix to its PR) - -| PR | Owns under `native/csrc/` | -|---|---| -| 127 | `pack/`, `sink/`, `store/`, `common/`, `ring/`, root — 69 files | -| 128 | `catalog/{catalog_writer,clickhouse_client,conformance_catalog,indexer,lease_coordinator,pack_index,schema,version_allocator}.{cpp,h}` — 15 files | -| 129 | `catalog/{hydration,reader}.{cpp,h}` — 4 files | - -Nuance: `conformance_catalog.cpp` is owned by 128 but carries ops added on -129 (reader/hydration). A defect in one of those later ops routes to 129, -not to the file's owner — ownership is per defect, not per filename. - -## Baseline numbers (Phase 1), measured at `a4d3dcb` - -Measured in a dedicated detached worktree (`/tmp/opencode/polish-c`), NOT in -the shared one: another session edits `/tmp/opencode/native-wt` roughly every -20 minutes, and its in-flight edits have already contaminated one measurement -this session (3 phantom test failures traced to uncommitted `pack_builder.cpp` -work). - -| Signal | Baseline | -|---|---| -| Build errors (`make -C native cpu-goals`, from empty `build/`) | **0** | -| Build warnings, same build | **0** | -| cpu suite (`pytest -m cpu`) | **1203 passed**, 0 failed, 0 skipped | -| live suites (`clickhouse and manual and not garage`) | **128 passed, 1 failed** | -| Lint | not measured — no `.clang-tidy`/`.clang-format`, no ruff/flake8 in `pyproject.toml`. The compiler's `-Wall -Wextra` IS this project's C++ lint signal, counted above. | -| Types | not measured — no mypy/ty configured. Adding tooling is out of scope. | -| Coverage | not measured — no coverage tooling configured. Out of scope. | - -The one live failure is pre-existing and environmental, not a finding: -`test_a_role_that_cannot_see_one_object_is_told_to_grant_it_not_to_rebuild` -needs `CREATE USER` on the server, and the local standalone ClickHouse has no -access management. The same test passes in CI. It is the baseline, so a final -count of "128 passed / 1 failed" is unchanged, not a regression. - -Warning baseline is genuinely zero rather than filtered: the whole build log -is 55 lines with no `warning:` matches. Earlier in the session this build -carried four torch-header warnings; those disappeared when the torch -extension moved to C++20. - -## Rounds - -### Round 1 — verdicts - -Four lenses reported 40 findings (see `polish-seen.md`). Verifier verdicts as -they land. A verifier may CONFIRM the mechanism and still downgrade impact to -`optional`; those do NOT enter the fix queue — they go to the human list. That -distinction is the whole point of the verify step, so it is recorded per row. - -| Finding | Verdict | Impact | Queued? | Owner | -|---|---|---|---|---| -| hydration.cpp:586 squared element budget | CONFIRMED | correctness | YES | 129 | -| hydration.cpp:692/:646 int64 abs_max sign + >2^53 cast | CONFIRMED | correctness | YES | 129 | -| schema.cpp:127 rebuild_instruction omits legacy_objects_ | CONFIRMED | requirement | YES | 128 | -| reader.cpp:396 missing CaptureQuery bounds | CONFIRMED (3 of 4 sub-claims) | requirement | YES | 129 | -| schema.cpp:385 dead legacy-refusal loop | CONFIRMED | optional (downgraded from requirement) | no — human list | 128 | -| s3_sign.cpp:75 header ordering | CONFIRMED (downgraded) | optional (downgraded from requirement) | no — human list | 127 | -| pack_builder.cpp:353 ParseUuid discarded | CONFIRMED mechanism, 2 of 3 sub-claims REFUTED | optional (downgraded from correctness/requirement), sev -> low | no — human list | 127 | -| spool.cpp:190 `a..b` rejected → sink latches | CONFIRMED | correctness | YES | 127 | -| spool.cpp:190 symlink escape → silent permanent loss | CONFIRMED | correctness | YES | 127 | -| catalog_writer.cpp:21 + clickhouse_client.cpp:26/111 escaping + NUL truncation | CONFIRMED (all 3 parts) | requirement | YES | 128 | -| pack_index.cpp:179 + indexer.cpp:36 unescaped pack_id | running | | | 128 | - -Two verifiers usefully corrected their reviewer rather than rubber-stamping: - -- The SigV4 reviewer's own example (`Content-MD5`) does NOT reproduce — `'C'` - sorts before every lowercase letter either way. The verifier had to find - `X-Amz-Meta-Zed` to break it, then showed every production path already - lowercases before signing, so the hazard is reachable only through the - conformance driver. Mechanism real, blast radius nil → `optional`. -- The query-bounds reviewer claimed an unbounded `IN` list was dangerous. The - verifier disproved that specific consequence (the server refuses with - `Code: 62 Max query size exceeded` — fails closed, and `sql_quote` escapes - each name), while confirming the three real divergences. Queued for the - bounds, not for the imagined injection. - -### Round 1 — fix queue and dispatch - -Ordered high severity first, then correctness before requirement. Dispatch is -grouped BY WORKTREE, not one builder per finding: three builders running -concurrently in one worktree would race on the shared `native/build/`, and two -builders editing one function would conflict. Commit-per-fix is preserved -inside each builder, which is the invariant that actually matters for -traceability. - -| # | Finding | Sev/Impact | PR | Worktree | Builder | -|---|---|---|---|---|---| -| 1 | hydration.cpp:586 squared budget | high/corr | 129 | native-wt | dispatched | -| 2 | hydration.cpp:692 int64 order stats | high/corr | 129 | native-wt | dispatched (same builder, 2nd commit) | -| 3 | spool.cpp:190 `a..b` latch | med/corr | 127 | polish-a | dispatched | -| 4 | spool.cpp:190 symlink escape | med/corr | 127 | polish-a | dispatched (same builder, 2nd commit) | -| 5 | pack_id SQL injection | med/corr | 128 | phase-a-ci | dispatched | -| 6 | escaper 4-of-10 + NUL truncation | med/req | 128 | phase-a-ci | dispatched (2nd commit) | -| 7 | schema.cpp:127 rebuild instruction | med/req | 128 | phase-a-ci | dispatched (3rd commit) | -| 8 | reader.cpp:396 query bounds | med/req | 129 | native-wt | HELD — waits for the hydration builder to release the worktree | - -Findings added to the queue by later verdicts (all held behind the builders -already in flight, since they land in worktrees currently occupied): - -| # | Finding | Sev/Impact | PR | State | -|---|---|---|---|---| -| 9 | indexer.cpp:105 conflict emission per ref + :107 message text | low/corr + req | 128 | HELD | -| 10 | schema.cpp:162 visibility probe omits the legacy object | med/corr | 128 | HELD | -| 11 | reader.cpp:475 cursor captured_at_ns unvalidated | med/corr | 129 | HELD | - -### PR 127 — COMPLETE and independently verified - -Both 127 findings landed: `cdc5286` (component-wise key check) and `0db751d` -(weakly_canonical containment). cpu on that branch went 1198 -> 1200 passed, -0 failed, 0 skipped; +2 is exactly the two new tests and no pre-existing test -was modified. - -I did not take the builder's red-then-green claim on trust. I reverted ONLY -`native/csrc/store/spool.cpp` to `92c2225`, rebuilt both drivers, and re-ran -the new tests: - -``` -FAILED tests/test_native_spool.py::test_stage_rejects_a_key_escaping_through_a_symlinked_directory -FAILED tests/test_native_pack_sink.py::test_dotted_tenant_stages_and_does_not_latch_the_sink -2 failed, 1 passed, 20 deselected -``` - -Then restored the fix and confirmed 3 passed. So both tests are genuinely red -at the baseline and green after — the fixes are load-bearing, not decorative. -The third test in that selection (`test_object_key_parity_with_python`) passes -in BOTH states, which is the point: it only ever compared the key string and -never reached the spool guard, which is why the defect was invisible to it. - -One judgement call the builder made and flagged: the component-walk refusal -now reports "object key is invalid" (Python's `validate_object_key` wording) -while "object key escapes the spool root" is reserved for the containment -check, matching the filesystem.py / spool.py split. No test asserted on the -old native message. - -### PR 129 — first two findings landed, independently verified - -`3d59cea` (element budget) and `2626b46` (integer order statistics). Verified -the same way as 127, but from a fresh detached worktree at the commit rather -than the builder's own scratch copy: built all five drivers, ran the three -affected parity tests green, then reverted ONLY `hydration.cpp` to `a4d3dcb`, -rebuilt, and got - -``` -FAILED test_summary_parity_on_int64_beyond_the_double_mantissa -FAILED test_the_summary_element_budget_counts_each_element_once -2 failed, 1 passed -``` - -then restored and confirmed 3 passed. Both new tests are load-bearing. - -Honest note on the third: `test_summary_core_stats_parity` passes in BOTH -states. The builder widened it to assert `abs_max_int`, which it had silently -omitted, but its corpus is uint8 where `abs_max_int` is already at parity — so -that extension is a widened gate, not a proof. The two new tests carry the -proof. - -Two decisions the builder made that I checked rather than accepted: - -- It REFUSES `uint64` in the new integer decode rather than representing it. - Verified independently: `uint64` is absent from `_DTYPE_BYTES` in - `src/dmi/storage/capture/model.py` and `kDtypes` in `pack_builder.cpp` has - 14 entries without it, so there is no oracle to match and failing closed is - right. (The recent `a5e15a2 feat(capture): fp8 and the wide integers are - first-class dtypes` does NOT add uint64.) -- `abs_max_int` stays `int64_t` and SATURATES at `INT64_MAX` for the single - unrepresentable magnitude, documented in code and pinned by the test as - `min(core.abs_max, 2**63-1)`. Widening the wire field is a contract change - and stays on the human list. Same residual applies to `minimum`/`maximum`, - which are `double` on the wire and so remain lossy above 2^53 even though - `minimum_int`/`maximum_int` are now exact. - -### PR 128 — three findings landed, independently verified - -`8f23d0e` (sql_uuid validation), `359ba00` (one ten-character escaper in a new -`sql_escape.h` + `CURLOPT_POSTFIELDSIZE_LARGE`), `631c1c6` (legacy object in -the rebuild instruction). Branch counts: cpu 1202 -> 1204, live 106+1 -> 109+1, -the one failure being the same pre-existing `CREATE USER` test. - -Red check: reverted `native/csrc` and `native/Makefile` to `7eac029`, rebuilt, -and all four new live tests went red, one per finding: - -``` -FAILED test_the_parameterized_statements_are_byte_identical_too -FAILED test_an_embedded_nul_does_not_truncate_the_statement_on_the_wire -FAILED test_a_pack_id_that_is_not_a_uuid_is_refused_before_it_reaches_the_sql -FAILED test_an_earlier_builds_object_beside_this_builds_is_refused -4 failed, 1 passed -``` - -Restored: 5 passed live, 2 passed cpu. - -HONEST QUALIFICATION on the two cpu escape tests. They also fail at the -baseline, but NOT because of the defect — they fail with -`{'op': 'escape', 'ok': False, 'what': 'call open first'}`, because the -`escape` driver op is itself new. So `test_native_catalog_sql_escape.py` is a -forward regression guard, not a proof the bug existed. The builder was right -to say `test_the_ordinary_characters_are_left_alone` is not a red test (it -guards against OVER-escaping), and by the same token neither cpu test can -carry the red-green proof. The four live tests carry it. - -### Two mismatches in my dispatch brief, corrected by the builder - -Both were my error, inherited from a verifier that reasoned against -`polish-c` — which contains all three PRs at once, so it sees files that -branch 128 alone does not: - -- I listed `reader.cpp:497`, `reader.cpp:669` and `catalog_writer.cpp:806` as - sites the shared escaper would fix. `reader.cpp` does not exist on 128 at - all (it is a 129 file). The builder consolidated the four copies that do - exist on this branch instead, which is the correct reading. -- I told it to EXTEND `test_the_parameterized_statements_are_byte_identical_too`. - That test does not exist on 128 — I wrote it earlier this session on 129. - The builder added one under that name rather than inventing a different one. - -### Duplicate-test hazard from the second mismatch, checked and contained - -Because both branches now define -`test_the_parameterized_statements_are_byte_identical_too` in the same file -(129 at line 303, 128 at line 308), the forward-merge could in principle have -landed two same-named functions in one module, where Python keeps only the -last and the other silently never runs. - -Trial-merged `631c1c6` into `2626b46` in a throwaway worktree to find out. -Git raises it as a conflict, and both definitions fall inside ONE conflict -block (lines 292-433), so a human resolution is forced and no silent -shadowing is possible. Conflicts also appear in `native/Makefile` and -`native/csrc/catalog/catalog_writer.cpp`. Merge aborted, worktree removed. - -Resolution when the stack is forward-merged: keep 128's version and drop -129's. 128's is strictly stronger — all ten of the driver's characters plus -the NUL case — and 129's is the weaker original whose fixture (`it's\odd`) -only covered the two characters both implementations already agreed on. - -### Round 2 — a /code-review of the LANDED work, all five findings closed - -The human ran `/code-review` over the 129 diff. It found five things, one of -them a defect in the fix I had just landed. All five are now fixed and -red-checked; none was refuted. - -| Finding | PR | Commit | -|---|---|---| -| cursor `v`/`w` still wrapped (high) | 129 | `a6b5260` | -| `number()` accepted leading zeros | 129 | `40e5ada` | -| `search()` never validated its text filters | 129 | `343bef6` | -| harness decoded a negative bound to UINT64_MAX | 129 | `dee5d95` | -| `substitute()` rescanned from 0 per parameter | 128 | `560b229`, `33ea8d7` | - -A verification mistake of mine worth recording: my first red check of -`substitute` reported "5 passed" — a FALSE GREEN. Reverting the `.h` broke the -build (`'substitute' is not a member of 'dmi_catalog'`) and the tests ran -against a STALE binary. Same trap as earlier in the session. The CPU gate -cannot be red-checked by reverting at all, because the driver op it drives is -part of the fix; the LIVE test can, because it uses the pre-existing `acquire` -op. Redone properly with `build errors: 0` asserted in BOTH states: red at -`8ee2856`, green at HEAD. - -The `substitute` red also exposed something the verifier had missed — TWO rows -at one term with DIFFERENT holders, because `release_statement()` binds no -`ttl_ns`, so the tombstone stored the literal `%(ttl_ns)s` while the claim -stored `30000000000`. The test reads back a sorted distinct holder list rather -than a single value, so that shows up in the failure message instead of -tripping a `len(rows) == 1` guard. - -Sibling check, because this run has been bitten four times by fixing one site -and leaving its twin: `native/csrc/clickhouse_client.cpp` (a different 53 KB -file sharing the basename) has ZERO `%(` matches and no `substitute`, so it -does not carry this defect. - -### THE RUN'S MAIN TECHNICAL LESSON: fix the primitive, not the caller - -Four separate findings this run were the same shape — a defect fixed at one -call site while its siblings kept it: - -1. the SELECT-row splitter got `[`/`]` depth tracking; the footer splitter did not -2. `hydration.cpp` tightened `uint64`; `pack_index.cpp` stayed lax -3. the cursor's `captured_at_ns` was bounded; its `v` and `w` were not -4. and the root of #3: `jc::FindInt` in `common/json.cpp` is STILL an - unbounded `int64_t` accumulator for ~100 other call sites - -#4 is the open one and the most valuable next step. The cursor path no longer -depends on it, but the sink, store and pack drivers and `pack_index.cpp` all -do, and none was audited. It needs its own finding and its own review because -changing shared JSON semantics touches files PR 128 also modifies. - -### CORRECTION — I reported the rank>=2 hydration bug as live. It was already fixed. - -The verifier reasoned against the FROZEN baseline `a4d3dcb`, where the bug is -real. The branch had moved: the other session's `422dc6e` ("seven review -findings across the three PRs", Sep 7 00:19) already fixed both hydration -defects. I checked rather than taking the builder's word: - -``` -3af38c3 hydration.cpp: 595 int depth = 0; 616 ++depth; 625 if (c == ',' && depth == 0) - 231 kCatalogToFooter() 650 for (... : kCatalogToFooter()) -a4d3dcb hydration.cpp: depth matches: 0 -``` - -Present at the tip, absent at the baseline. So my previous statement to the -human — that every rank>=2 capture is refused at hydration — was true of the -frozen review baseline and NOT true of the shipping branch. Stated plainly -because it materially changes the severity of what I reported. - -What was genuinely missing was the TESTS, and that residue is not small: the -rank-2 bug survived a fix of the identical bug class in the SELECT-row -splitter precisely because the hydration suite was 100% rank 1, and the -9-field comparison survived because the only mismatch cases anyone had -written were the three fields already inside the list. - -`ea3bcd1` and `7a22e66` add them, and the builder proved each genuinely red by -temporarily reverting the exact code the finding names, then restoring: - -- removing the depth tracking -> `catalog descriptor does not match the pack - footer: field 3` on the rank-2 case while the rank-1 test passed in the same - run. (Field 3, not the verifier's field 20, because the widened comparison - now trips on the first shifted field rather than on kShape — same shift, - earlier detection.) -- narrowing the comparison back to 9 fields -> 5 failures, `layer_number`, - `hook_name`, `step_number`, `request_id`, `captured_at_ns`, with the 3 - locator cases still passing, exactly the split the docstrings claim. - -Parity suite 23 -> **32 passed**; cpu unchanged at 1203. Branch fast-forwarded -to `7a22e66` and re-verified there. - -Field accounting, since I had asked for it: Python compares 21 CaptureMetadata -fields plus the 5-tuple locator = 26. Native's `kCatalogToFooter()` compares -all 32 catalog columns, so all 26 are covered and nothing Python checks is -missing natively. The 6 extras (`pack_id`, `store_id`, `object_key`, -`object_bytes`, `pack_checksum`, `pack_record_count`) are tautological rather -than wrong — Python excludes them deliberately because they came from the row -under test — so they were left alone. - -### PR 128 — footer metadata validation landed, with one cross-branch defect I caught - -`e10653b`, `cb39890`, `2413153`. 17 bug proofs and 12 regression guards, each -labelled as such, plus a control test (`test_a_well_formed_pack_still_indexes`) -without which a reader that refused everything would pass every refusal case. -cpu 1204 unchanged, live 111 -> **140 passed**, 1 pre-existing failure. - -The poison-shape test is built in two halves so the harm is recorded -independently of the fix: it writes `shape=[0,4294967295]` through the -untouched `write_descriptors` path, shows ClickHouse storing it verbatim in -`Array(UInt32)`, shows `reader.search()` then raising, and only then shows the -footer carrying it refused at the boundary. - -CROSS-BRANCH DEFECT, caught before it shipped. Among its "additions beyond the -confirmed list" the builder narrowed `pack_index.cpp`'s dtype table to the ten -names in `_DTYPE_BYTES`. Correct against 128's own oracle — and it breaks 129. -The stack is 127 -> 128 -> 129, and the oracle GREW on 129: - -``` -128 tip _DTYPE_BYTES 10 names -129 tip _DTYPE_BYTES 14 names (+ float8_e4m3fn, float8_e5m2, uint16, uint32) -a5e15a2 "fp8 and the wide integers are first-class dtypes" -> 129 ONLY -``` - -and 129's `test_fp8_and_wide_int_summaries_parity` stages exactly those four -and drives them through `op="hydrate"` into this very function. After the -forward-merge the narrowed table would refuse dtypes 129 declares first-class, -failing that test. The builder could not see this from inside 128 — my -dispatch never told it the branch is stacked. - -Note this also RESOLVES the apparent contradiction with the earlier dtype -verifier, which found 14 oracle names and zero width disagreements: it was -reading `polish-c`, which contains all three PRs. Both agents were right about -different branches. Same root cause as the two `reader.cpp` mismatches earlier -— a verifier reasoning against the combined tree and a builder reasoning -against one branch. - -Sent back for a per-branch-correct derivation from `kDtypes` in -`pack_builder.cpp`, which is `std::array` on 128 and -`std::array` on 129 — so it tracks the oracle automatically -and the merge resolves itself instead of encoding a count that a merge can -invalidate. - -### Follow-up `8ee2856` — the derivation, and the merge verified by trial rather than argument - -`pack_index.cpp` no longer answers the dtype membership question: the gate is -`if (!dmi_pack::DtypeSupported(dtype))`, reading `kDtypes`, which tracks the -oracle per branch. It kept only the element WIDTHS, which already cover 129's -four additions (fp8 -> 1, uint16 -> 2, uint32 -> 4). No new dependency — -`pack_index.cpp` already includes `pack_builder.h` for `Crc32`. It considered -and rejected routing through the sink's `DtypeWidth`, which would have made -the reader depend on the writer's sink and needed a Makefile edit. - -Verified on this branch, name for name: `float32`/`bool`/`int64` admitted; -`uint16`, `uint32`, `float8_e4m3fn`, `float8_e5m2`, `uint64`, `float128`, -`not-a-dtype` refused — the live test reaching the same split independently. - -I trial-merged the two branch tips rather than accepting the merge-safety -argument, and it corrected one of the builder's claims: - -| file | result | -|---|---| -| `pack_builder.cpp`, `pack_builder.h` | auto-merged CLEAN, as claimed | -| `pack_index.cpp` | **CONFLICTS** — the builder said 129 never touches it, but 129's `422dc6e` did | -| `catalog_writer.cpp`, `tests/test_native_catalog_lease_live.py` | conflict (already known) | - -The `pack_index.cpp` conflict is benign and informative: BOTH branches -independently fixed the same rank-0 bug. 129 added -`if (jc::Unwrap(shape_json).empty()) return 1;` inside the old -`shape_product`; 128 replaced that function with a validated `parse_shape` -that also treats `[]` as rank-0 (`pack_index.cpp:217`) and additionally checks -bounds, overflow, and refusal ORDER against the oracle. - -RESOLUTION when forward-merging: take 128's side (`8ee2856`) for that region. -It is strictly stronger and preserves the rank-0 behaviour 129 was fixing. -Coverage survives either way — 129 pins the neighbouring case with -`test_zero_element_tensor_hydrates_to_empty_bytes` -(test_native_reader_parity_live.py:1831). Note the two are DIFFERENT cases: -`shape=[]` is rank-0 with product 1, `shape=[0]` is a zero-element tensor with -`logical_bytes == 0`. The merged tree handles both. - -Merge-safe test design worth copying: the builder noted a "native refuses -uint32" test PASSES on 128 (128's oracle refuses it too), so such a test could -never have caught the narrowing. Its ten probes instead assert AGREEMENT in -whichever direction the contract moves — for each name, both readers admit the -footer or both refuse with the same sentence. On 128 that is the 3/7 split; -after the merge four flip to admitted on both sides and the test still holds. -`uint64` is deliberately included as the case that catches a width table -quietly widening membership. - -Two stale comments it flagged and left alone, both outside the finding and -both invalidated by the merge: `pack_builder.h:83` ("one of the ten supported -names") and `record_row.h:58` ("the ten supported dtype names"). `a5e15a2` -fixes neither, so they will be stale on the merged tree. - -### COORDINATION PROBLEM with the other session - -`422dc6e` also committed MY `.loop/polish-seen.md` (+106) and -`.loop/polish-state.md` (+221). The other session is reading this ledger and -fixing findings out of it, which explains the overlap and the two reverts of -`reader.cpp`. Useful, but it means confirmed findings can be fixed twice and -a verifier's baseline can go stale mid-run. Checked the blast radius: its -`pack_index.cpp` change in that commit is only +5 lines and contains none of -the metadata-bound validation, so the 128 work now in flight is NOT -duplicated. - -### PR 128 — last two findings landed, and my prescription was wrong - -`28aff34` (one failure per distinct claimant, plus the oracle's message text) -and `6cb6f6b` (grant-probe the superseded object). Red check: reverted -`indexer.cpp` and `schema.cpp` to `631c1c6`, rebuilt, both new tests failed; -restored, both pass. cpu 1204 (unchanged — the new tests are live-marked), -live 109+1 -> 111+1. - -I told the builder to emit conflict failures "in map order". That was WRONG -and it overrode me correctly. Python's `conflicted` is a dict, and dicts are -insertion-ordered, so `conflicted.items()` yields FIRST-CONFLICT order. I -confirmed it directly: - -``` -dict order for z,w,z,w -> ['z', 'w'] -``` - -A `std::map` sorted by identity would have emitted `[w..., z...]` where the -oracle emits `[z..., w...]` — preserving the very defect class the fix -targets, since `merge` truncates `failures[:failure_limit]`. The builder used -a `conflict_order` key vector to reproduce insertion order and demonstrated -the divergence with a `[z1,w1,z2,w2]` case. It also verified its Python-repr -helper byte-for-byte against the oracle, including -`store_id = "it's\todd\\"`. - -NEW finding it surfaced and correctly declined to fix: the NON-conflicted -`unique` list has the same ordering divergence in a different loop — native -walks `by_identity` as a `std::map` (sorted by `(store_id, pack_id)`) where -Python's dict yields first-appearance order. That affects descriptor/inventory -row order and the published member list, so it is a wider blast radius than -the finding it sat next to. Recorded for the human, not fixed. - -### PR 129 — reader findings landed, after being reverted TWICE by the other session - -`7118b6b` (cursor timestamp validated, carried as a typed `uint64_t`) and -`3af38c3` (the four missing CaptureQuery bounds). Verified as before: both new -tests red against the pre-fix `reader.cpp`, green after, and the full parity -suite is `23 passed`. - -INCIDENT worth remembering. The surgical-index commit technique -(`git hash-object` + `git update-index --cacheinfo`) creates a correct commit -but leaves the shared worktree's FILE stale — the index and the working tree -disagree. The other session commits with `git commit -a`, which stages that -stale file and silently reverts the fix. It happened twice: - -| Their commit | Effect | -|---|---| -| `422dc6e` "seven review findings" | `reader.cpp | 46 +------`, test file `| 60 ------` — pure removal of the fix, no content of their own in those two files | -| `79addbb` "zero-element tensor" | `reader.cpp | 70 +-------` — reverted the reland | - -The builder re-landed a third time, additively on top of `79addbb`, preserving -their new `test_zero_element_tensor_hydrates_to_empty_bytes` verbatim. - -I then closed the loop by refreshing the two stale working-tree copies in -`/tmp/opencode/native-wt` to HEAD, which the builder was forbidden to do. -Before overwriting I checked exactly what would be lost: the diff was 186 -deletions and just 2 insertions, and both insertions were the PRE-FIX -originals — including the exact buggy line the finding names, -`after = {literal(0), literal(1), literal(2), parts[3], literal(4)};`. So -nothing of the other session's was in those files. After the refresh, -`git status` shows only my own `.loop/polish-state.md` as modified. - -LESSON for any future run sharing a worktree: the index technique is only half -a solution. Either refresh the working-tree file immediately after committing, -or do not share the worktree at all. - -### Concurrent-session hazard, and how the 129 work was isolated - -`/tmp/opencode/native-wt` is NOT clean: another session holds uncommitted -edits to `hydration.cpp`, `clickhouse_client.cpp`, `pack_index.cpp`, -`bindings_sink.cpp`, `native_pack_sink.cpp`, `spool.cpp`, `uploader.cpp`, -`native_sink.py` and `summary.py`, and that tree passed through a -non-compiling intermediate state during this run. - -The builder therefore verified in an isolated copy and committed only its own -hunks via `git hash-object -w` + `git update-index --cacheinfo`, leaving the -other session's working-tree files untouched. I confirmed both halves: -`git show --stat` on each commit lists only `hydration.cpp` and the parity -test, and `git status --porcelain` still shows all nine of their files as -modified-uncommitted. Their work is intact. - -Consequence for measurement: suite counts taken in the shared worktree would -measure their unfinished edits, not ours. Every number recorded for 129 comes -from an isolated worktree at the commit. - -Findings 3 and 4 share one code site and the verifier established that neither -change fixes the other: the component-wise check does not stop a symlink -(`link` is a legal component) and canonicalisation does not admit `a..b` (it is -rejected before any path is built). Both are required, hence one builder, two -commits. - -Both int64 sub-claims came back worse than reported: `static_cast` of -`2^63` yields `INT64_MIN` on x86-64, so a strictly positive tensor -(`[2**63-2, 2**63-1]`) summarises as maximally negative in BOTH `minimum_int` -and `maximum_int`, not merely a rounded magnitude. - -## Round 3 — the primitive, then all 89 of its call sites - -The run's recurring lesson was fixing a defect at one call site and leaving -its siblings. `jc::FindInt` was the root: an unbounded `int64_t` accumulator -shared by the whole native tree. - -`329f7a4` bounded it before the multiply behind a new `FindIntChecked` with -`IntFind { kOk, kAbsent, kOutOfRange }`, and `1c45872` migrated the one -production caller. The design point worth keeping: the accepted range is the -UNION `[-2^63, 2^64-1]`, stored as the two's-complement pattern. For a -literal in `[2^63, 2^64-1]` the OLD wrap produced the same bit pattern, so -`static_cast` was ACCIDENTALLY CORRECT, and a conformance test -depends on it (`step_number = 2**64-1`, legal per model.py). A tightening to -`INT64_MAX` would have broken parity in the other direction. - -Then the remaining 89 sites, one commit per driver (`aa7dc59`, `3c833b0`, -`6bb4aa9`, `7325269`, `ab8687e`, `cd6d039`, `18b7f9c`). Red check: reverting -the six source files put **77 tests red** with `build errors: 0` asserted, so -genuinely rebuilt. cpu 1228 -> **1331**; live unchanged at 188 plus the known -`CREATE USER` failure. - -Corrections made during that round: - -- My site count of ~92 was wrong: `grep -c` counts LINES and swept in - comments. The real figure is 89 — `reader.cpp` has ZERO (both matches are - comments), `pack_index.cpp` 2 not 3, `conformance_catalog.cpp` 46 not 47. -- The prior builder's stated reason for skipping the drivers — "a refusal - needs a new `ok:false` shape no test exercises" — did not survive contact. - NO driver needed one; each was routed through the refusal shape it already - had, and the catalog driver through the `CatalogError(kValue, ...)` its own - neighbouring lambda already throws. -- The builder corrected its own commit messages: it had described the - spool/store bounds as falling back to a 1 TiB default, but the fallback is - keyed on ZERO, so `-1` cast to `UINT64_MAX` and the capacity limit became - NO limit. Worse than first written. - -Worst thing found in that sweep: in the catalog driver an out-of-range -`uint`/`int` parameter rendered `18446744073709551615` or `-1` straight into -SQL, and out-of-range reader read-bounds LIFTED the bound rather than -tightening it — the inverse of what a bound is for. - -## Closing state - -`8a228ef` drops the now-callerless `FindInt` wrapper. It only ever existed to -return -1 for both `kAbsent` and `kOutOfRange` — the exact conflation the -migration removed — so keeping it exported left a way back to the bug. - -`b289356` removes the false parity claim at the uploader byte gate, in both -`uploader.cpp` and `uploader.h`. Behaviour is deliberately UNCHANGED: whether -to adopt the oracle's whole-batch refusal is a maintainer call, pinned by two -purpose-named tests and a `docs/benchmarks.md` entry. Only the claim that it -already matches the oracle is gone, with the divergence named in its place. - -Everything is on `feat/native-capture-pipeline` (PR #127) — the only open PR -after #128 and #129 merged mid-run. Both CI jobs green. - -Still open, for the human: - -- the uploader byte-gate POLICY (documented now, not decided) -- six confirmed-but-`optional` findings, listed in the PR description's - "deliberately not fixed" table -- the quiet-host benchmark re-measure, unchanged from Checkpoint B