Repository navigation
refactor(storage): separate tool bodies and compact semantic directories - #6520
Conversation
Generated-by: Codex
Preserve exact canonical bytes with immutable shared projection bodies and header slots. Resolve frozen ranges on the original SQLite worker using its existing statement cache, and keep full replay, transfer, and collection in the same owner lifecycle. Generated-by: Codex
Keep one body reference on each event row, enforce atomic collection, and measure prefix API time separately from complete result validation. Generated-by: Codex
Prepare canonical headers and original projection bytes on the SQLite worker, then decode owned events on a joined blocking task. Drain both stages before releasing the original transaction, preserving prefix budgets and raw evidence. Verify checkpoint tool history after source collection and import. Record chronological benchmark samples and isolate RootStore opening cost. Generated-by: Codex
Decode borrowed headers and original projection slices before advancing each SQLite row. Remove the temporary body string, transfer queue and independent decoder while preserving the original snapshot, budgets and raw evidence. Align the cleanup scheduling test with upstream's readable Root contract, retaining sibling drainage, confirmed outcomes and failed shutdown evidence. Generated-by: Codex
Order canonical headers and references before resolving bodies by primary key on the same SQLite worker and transaction. Reuse one body statement while decoding borrowed rows and recording original canonical evidence. Reject missing bodies without publishing partial canonical bytes. Generated-by: Codex
Read tool projection ranges directly from one immutable body and remove unused cross-header streaming. Share captured source validation and range dispatch through CanonicalRead on the original SQLite worker. Cover original and malformed canonical artifact tokens through the real capture path, including continued reads after malformed-token rejection. Generated-by: Codex
Astro-Han
left a comment
There was a problem hiding this comment.
Automated review notice: This comment was posted by an automated review agent. It is not an independent human review and does not replace one.
This is a focused migration/SQL-constraint and release-inventory check at a2b7d44a9a37aa56f5d78ffc1836e8a307d98e89; it is not a complete review of the 138-file storage rewrite. I inspected migrations 0013/0014, their registration/cleanup path, the canonical migration and recipe code, and the changed Cargo inputs. One confirmed P2 remains: the three new dependency-input changes are absent from the source-bound notice inventory, as described inline.
The base already has two stale SDK fingerprints. Correcting only those inherited fingerprints in an isolated source copy still leaves the actual notice verifier failing on this PR: Cargo.toml, Cargo.lock and platform/event-log/Cargo.toml are three additional mismatches. Correcting all five recorded fingerprints makes the same verifier pass. This distinguishes the new inventory gap from the inherited one; it does not validate license-generation output.
I also executed the unchanged material triggers from migration 0014 in nine isolated SQLite fixture checks: shared-body lifetime, last-reference clearing/deletion, rollback, immutable bodies/references, foreign-key protection and invalid length/offset/kind rejection all passed. The fixture uses a minimal event table and supplies already separated material; it does not run the Rust migration functions, Postcard conversion, SQLx upgrade, real prepared-checkpoint recovery or production readers. An actual merge calculation against Rust dfc556ebf50ee5f6cfd71ddfa2d1c097faf27fa6 is clean.
This machine has no Rust toolchain. The reported 382 tests, v12 checkpoint upgrade and performance measurements remain the contributor's evidence. I have not established the safety or performance of the full rewrite. Hosted CI is still running; passing dependency/Computer Use checks does not override the locally reproduced inventory failure.
Automated review notice: This comment was posted by an automated review agent operated by Astro-Han. It is not an independent human review and does not replace one.
| [workspace.dependencies] | ||
| mxc-sdk = { git = "https://github.com/microsoft/mxc", rev = "05a430bc17a3e1b3fa2131524fc52e4319e07298" } | ||
| flatbuffers = "25.12.19" | ||
| postcard = { version = "1.1.3", default-features = false, features = ["alloc"] } |
There was a problem hiding this comment.
[P2] Refresh the notice inventory for the new dependency inputs
This dependency change, the corresponding lockfile change and platform/event-log/Cargo.toml are all hashed inputs of the release notice verifier, but products/maka/cli/notices.json is unchanged. They introduce three mismatches beyond the two stale SDK entries already present on the base. Running the actual verifier after correcting only the inherited SDK entries still fails; correcting these three additional fingerprints in a disposable copy makes it pass. Please regenerate and review the source-bound inventory against the final dependency graph; otherwise normal checks and release preparation remain blocked even after the base's SDK mismatch is repaired.
Automated review notice: This comment was posted by an automated review agent operated by Astro-Han. It is not an independent human review and does not replace one.
Regenerate notices against the final dependency graph and bundled product. The license document is unchanged; refresh the three Cargo inputs and two inherited SDK fingerprints. Generated-by: Codex
My earlier source-inventory P2 is resolved by #6520 commit The later #6523 replaces that mechanism with actual component/license coverage checks. The completed product worker for Rust
|
The final-head run 41273119557 has finished: 1,980 passed, one failed, 26 skipped. The failure is #6529 changes the launcher fixture's watchdog scope; its source change is not a completed runtime validation. Current Rust CI is still running. My earlier inventory P2 remains resolved, while full storage/migration acceptance remains outside the scope of that fix check.
|
Astro-Han
left a comment
There was a problem hiding this comment.
Automated review notice: This comment was posted by an automated review agent. It is not an independent human review and does not replace one.
Additional, explicitly scoped review at final head 9341667e5105d19648df29166e27c4d7b336d1d8: 22 changed files covering canonical body storage, ingress/replay, prefix reads, and frozen canonical ranges. This extends the earlier migration/inventory review; it is not a review of all 139 files. No confirmed new P0–P2 issue found in this slice.
I read all of src/canonical.rs, canonical/{event,migration,read,recipe,source,write}.rs, canonical_insert.rs, schema.rs, prefix.rs, captured canonical.rs and artifact_identity.rs, and migration 0014. I also read every submitted hunk in append.rs, append/{history,ingress}.rs, connection.rs, context/{evidence/roots,proof}.rs, context/semantic/captured.rs, tool_payloads.rs, and transcript/evidence.rs, following the relevant surrounding code. Paths in this paragraph are under platform/event-log/. All 22 reviewed file blobs are unchanged between this head and current rust-preview 810305ca65040a578fb148f2b2c196e6a07a809c.
Specific checks:
canonical/recipe.rs:39–113extracts a borrowed raw JSON token and replaces precisely that token withnull;:133–278checks logical length, slot bounds, placeholder, and supplied body length before reconstructing exact bytes.canonical/event.rs:59–108verifies that the slot identifies the successful tool projection before typed decoding, rather than accepting anothernullin the header.canonical/write.rs:27–70checks the existing bytes after digest deduplication.append/ingress.rs:42–100requires an existing transaction and publishes the body reference and derived projections on the original worker. Replay comparison inappend.rs:295reconstructs the original event instead of comparing only headers.prefix.rs:72–129budgets the logical canonical byte length, orders headers without carrying large payloads through the sort, then resolves bodies inside the same transaction. The segmentedRootDigest::recordincontext/evidence/roots.rs:66retains one length prefix for the original JSON field; splitting the bytes does not add three separate hash fields.context/semantic/captured/canonical.rs:38–139revalidates the captured locator and dispatches tool ranges through immutable bodies while model ranges remain in headers.canonical/source.rs:120–174rejects invalid metadata, overflow, and ranges outside the original body slot. I traced both chunked and sequential captured reads and the artifact-identity range reader through this boundary.
I executed 12 isolated SQLite checks using migration 0014's actual event_bodies DDL and material triggers with a minimal event table. These include shared-body last-reference collection, rollback, immutable references, invalid recipes, distinct original JSON encodings, and a real two-connection WAL check: a pinned reader can still read the original body after another connection deletes the final event, and a new snapshot sees collection. This exercises the SQL constraints and SQLite snapshot behavior, not the Rust reader or SQLx migration runner.
The previously reported notice-inventory P2 remains resolved by the final manifest update. This supplemental review makes no new CI or release-success claim. I did not rerun Rust, Postcard decoding, genuine schema-12 checkpoint recovery, bundle/copy/import round trips, or the author's benchmarks/382-test suite. Directory packing/codec changes, the remaining query-site changes, transfer/collection integration, and benchmark validity are outside this 22-file slice. A complete review of those remaining areas and an actual old-database upgrade are still outstanding.
Automated review notice: This comment was posted by an automated review agent operated by Astro-Han. It is not an independent human review and does not replace one.
Astro-Han
left a comment
There was a problem hiding this comment.
Automated review notice: This comment was posted by an automated review agent. It is not an independent human review and does not replace one.
Further scoped review at 9341667e5105d19648df29166e27c4d7b336d1d8: 36 additional changed files concerning directory encoding/pages, copy and bundle transfer, archive reads, and material collection. These are distinct from the 22 files in my earlier core review. This still does not complete the 139-file review. No confirmed new P0–P2 finding in this slice.
I read the complete directory codec and its tests, directory migration module, captured directory page implementation, and migration 0013. I read all submitted hunks in the remaining semantic-directory/source/write files, all ten changed bundle files, session copy/import/material/removal, the three archive files, artifact-history retention, and the associated checkpoint/material/import/media tests. I followed the surrounding publication, owner-selection, and transaction code where the changed queries rely on it.
The main checks were:
platform/event-log/src/context/semantic/directory/codec.rs:29–35requires complete Postcard consumption. Migration 0013 converts the three descriptor columns; ordinary writers encode descriptors once.context/semantic/captured/directory.rs:41–198packs explicit lengths and locator fields into the supplied buffer, rejects an undersized first record, and validates record bounds before decoding. Continuation cursors advance only after a record fits. The persisted descriptor declarations now explicitly document their Serde layout as a storage contract.bundle/export.rs:97–134batches event identities in groups of 128, reconstructs original event JSON in the existing read transaction, and rejects missing records.bundle/import/publish.rs:138–178passes that original JSON back through canonical ingress. Artifact validation retains local/inherited/revision ownership branches; the frozen-media witness retains the exact owner's history, canonical freeze boundary, MIME, and copied-artifact mapping.sessions/material.rs:108–137reconstructs the original event before computing its retained proof/digest, then clears all canonical reference fields in the same collection transaction. Archive candidate sizes use the stored SQLite-normalized projection length, while actual archive reads reconstruct the full projection. Copy membership and revision predicates continue to use header fields that remain present after splitting the tool projection.
Verification: Eight isolated SQLite checks passed using the actual extracted export, artifact-selection, and frozen-witness SQL. They cover owner isolation, local/inherited/revision cursor ordering, a same-sequence revision, the 128/129 export boundary, missing/collected records, and canonical freeze ordering. The migration-13 rollback check executes its actual SQL with deliberately substituted conversion functions; it verifies transaction rollback, not Postcard conversion.
I also read the actual hosted logs from run 41323889906. Eleven relevant Rust tests each report PASS: all six codec round-trip/golden cases, atomic directory migration/reopen, logical-versus-physical span bounds, checkpoint collection/reopen/import, shared-projection last-owner collection, and the canonical artifact-token regression. All 36 reviewed files have identical blobs at this PR head, current rust-preview 810305ca65040a578fb148f2b2c196e6a07a809c, and that run's a9bb6ab5 head. This is independently inspected hosted evidence; I did not execute these Rust tests locally. The run overall remains red because of the separate, previously reported todo_tests.rs:169 assertion failure.
This slice changes physical schema-13/14 storage handling, without a new wire epoch. The old notice-inventory finding remains resolved. Remaining query-site changes, benchmark code/evidence, and a genuine old-database Prepared-checkpoint upgrade still need review or direct validation. I have not independently reproduced the author's performance tables or their complete 382-test claim, and the observed hosted tests do not establish those broader claims.
Automated review notice: This comment was posted by an automated review agent operated by Astro-Han. It is not an independent human review and does not replace one.
Astro-Han
left a comment
There was a problem hiding this comment.
Automated review notice: This comment was posted by an automated review agent. It is not an independent human review and does not replace one.
I have finished reading the remaining 81 changed files at 9341667e5105d19648df29166e27c4d7b336d1d8, including the complete 775-line Rust benchmark and 153-line Python driver. Together with my 22-file core review and 36-file directory/transfer review, this covers all submitted diff hunks in the 139 changed files. No confirmed new P0–P2 finding emerged from this final source review. This completes source coverage; performance and genuine old-database recovery remain separate validation limits.
The remaining query changes retain header-only reads for facts whose fields remain inline, including invocation/model/dispatch identity, message state, approval, recovery and usage metadata. Consumers that need a successful tool projection instead select its canonical reference and reconstruct the event. In particular, context-tail accounting uses canonical_bytes, skips canonical material for accepted model items and archived tools, and retains the archive writer. Observation delivery still produces a lightweight summary; it does not need the full projection. Run-source reads retain the owner-prefix fence and compare the closing event's invocation with the opening event before returning a complete run.
Four isolated SQLite comparisons passed using the actual old/new context-tail and observation-delivery SQL: full tool-body reconstruction and logical byte budgets; accepted-model-item exclusion; archived-body exclusion and archive-writer identity; and identical observation summaries with foreign-owner exclusion. The fixtures substitute controlled lineage/archive predicates and reconstruct selected bytes in Python. They exercise the changed SQL and selected values, not the Rust decoder or the complete production reader.
I also inspected six additional PASS entries in the actual hosted logs of run 41323889906: canonical tool projection/reopen/import, observation bootstrap/catch-up/reopen, SQL-owner cleanup on startup failure, catalog migration/history backfill, committed-prefix replay/reopen, and indexed ordinal fences on copied runs. These are later hosted regression results at a9bb6ab5, not a local execution or an exact-original-head acceptance run. The overall run still fails on the separately reported todo_tests.rs:169 assertion.
The benchmark source supports several important limits already disclosed in the PR:
- It validates matching fixtures and exact-byte/digest results, retains individual latency samples, hashes binaries, and includes unique body markers. The reported fixture storage savings therefore do not establish content-sharing gains.
- The benchmark performs a validating/warming first pass. Subsequent read timings are not cold-cache measurements. Caller-charged buffer peaks also do not measure total process memory.
- The checked-in Python driver's ordinary summary takes medians of per-run medians and uses at least five runs. The reported four-pair median-of-ratios tables instead depend on the separate private protocol described in the PR. Reading this driver cannot independently recompute those tables, the 234 quantiles, or the 17 flagged ratios.
The PR explicitly retains the raw reports/protocol/audits locally rather than attaching them. I could not access that directory on this machine and have not independently reproduced the performance percentages or the claimed genuine schema-12 Prepared-checkpoint kill/upgrade/recovery experiment. Publishing those raw reports, the paired execution schedule and binary provenance, and a reproducible old-database fixture/probe would make those claims independently checkable. The hosted tests and isolated SQL checks above do not establish them.
For applicability, 132 of the 139 reviewed file blobs match current rust-preview 810305ca65040a578fb148f2b2c196e6a07a809c. Seven have subsequent changes: workspace Cargo manifest/lock, context read.rs and safety.rs, transcript navigation queries, native owner tests, and CLI notices. This review covers the submitted PR versions of those files. I did not run Rust locally or independently verify the author's complete 382-test result. The previously resolved notice-inventory finding remains closed.
Automated review notice: This comment was posted by an automated review agent operated by Astro-Han. It is not an independent human review and does not replace one.
Summary
Large successful tool projections currently travel inside canonical event JSON and SQLite sort records. Store their exact original JSON tokens once in immutable, content-addressed
event_bodies; keep headers and references inevent_log. Prefix reads sort metadata before borrowing each body on the original SQLite worker. Exact-byte consumers reconstruct the original JSON; typed readers decode without joining it first. Atomic publication, frozen evidence, authorization, copy/import and last-reference collection retain the original ownership contracts.Encode semantic directory descriptors once with Postcard and pack fixed locators into caller-charged pages. Compact JSON, Postcard and FlatBuffers were compared, as were raw and framed bodies; retain Postcard/raw only. This is not general-purpose body compression.
Verification
Following review, fully regenerated release notices (1360 components / 693 texts); the 2,681,462-byte notice document is unchanged. Updated five stale source fingerprints, including the three Cargo inputs and two inherited SDK inputs. Notice verification/regression test, dependency inventory, ASF headers and pre-commit checks pass. The follow-up commit changes only
products/maka/cli/notices.json; storage source and measured binary provenance remain unchanged.Event-log: 382/382 tests passed, covering migration, exact replay, shared-body lifetime, frozen reads, copy/import, bundle transfer and checkpoints.
Related engine filters: 49/49; native filters: 6/6. Following the earlier upstream refresh, native 6/6 and the affected cleanup test 1/1 passed again. The new real canonical-artifact case rejects an incomplete token with a success-returning callback, then successfully reuses the original token.
Event-log clippy all targets/all features with warnings denied; rustdoc; workspace format; ASF headers; manual pre-commit hooks; release benchmark build passed. Format/ASF passed again after the final rebase. A workspace-wide test run and baseline red-run of every new test are not claimed.
A genuine upstream v12 Prepared checkpoint, killed at the prepared boundary, upgraded to schema 14 and recovered the same manifest/receipt, four exact events, tool bytes and prefix digest. Bundle validation, foreign keys, reopen and idempotent retry passed; temporary probe hooks were restored byte-for-byte.
Final fixed comparison: 48 fresh processes, four AB/BA-balanced pairs × six scenes, 100 mixed iterations and 13 operations. All 234 descriptive quantiles, raw outputs, fixtures, hashes and execution order independently audited; dedicated advisor accepted the explicit UX tradeoffs below.
Read performance and tradeoffs
Each percentage is the median of paired candidate/baseline ratios; milliseconds are median paired differences. They are separate statistics. Four pairs are descriptive evidence, with no confidence interval or universal equivalence claim.
prefix_readprefix_readmodel_viewsequential_alltail_64_allLarge prefix gains hold in every pair. Dense
prefix_readp50 improves by 10.09–11.21 ms; large export p50 improves by 60.44–69.14 ms.The slower rows are supported costs, not dismissed as noise. Actual 100-call totals give small
model_viewmedian extra costs of 1.132 ms repeated / 4.564 ms entropy.sequential_allscans all 64 small bodies: 100 complete batches cost a median 19.944 ms extra (pair range +0.522 to +176.896 ms).tail_64_allreads four tool-body tails: its 100-batch total difference has median −0.047 ms, while its p99 is consistently slower. These are aggregate passes/model-preparation calls, not per-frame or per-token costs. Current production search finds nowith_sequential_sourcecaller; the API remains supported.Dense range/sequential and repeated-large text-leaf tails vary across pairs/order. 17/234 median ratios exceed 1.05; the other 217 are not statistical passes. Existing-database reopen shows no consistently slower scene; entropy-large has median ratio +4.99% / median delta +14.00 ms, with one +198.82 ms pair. Admission used the declared quiet-load gate, but whole-process pressure includes the benchmark itself and some outside work returned. Every observation is retained, with no causal attribution or sample replacement. Earlier 120-process cohorts and the user-requested process-114 repeat remain separate.
All 17 median-ratio flags above +5%
first_64_allsequential_alltail_64_allfirst_64_allfirst_64_allmodel_viewmodel_viewsequential_allsequential_allcross_frame_repeated_64_alldescriptor_leaf_textmodel_viewtail_64_allfirst_64_allmodel_viewmodel_viewtail_64_allStorage and migration costs
RootStore::openmedians +19.4–31.2% (+0.82–1.74 s), slower in 29/30 pairs; dense write sums +18–21%, many other write sums +5–13%. These are descriptive creation costs, not ordinary existing-database reopen. The 60 creation reports used the earlier two-table cohort ondfc0e2323, baseline binary90a3d87d…/ candidate740c34c0…; subsequent reader optimizations preserve the storage/write format. Pair gaps, boot cohorts and resource-observer limitations remain disclosed rather than treated as causal proof.Review focus and reproduction
The complete diff is 139 files, +5179/−1002 (net +4177). Many query sites switch to header-only predicates. The final simplification removes the unused cross-segment reader and duplicate canonical-range dispatch (net −103, including a real regression test). The two-file, 928-line manual benchmark is retained as an independent regression/reproduction tool.
Review migrations 0013/0014,
canonical/{recipe,write,source,read,event}.rs,prefix.rs, directory encoding and captured readers. No experimental codecs, second SQL owner, body cache or additional publication state machine remains.Benchmark sources: Rust fixture and byte oracles, manual comparison driver. Copy the identical example into the baseline checkout (harness-only addition), then build each variant with
cargo build --locked --release -p maka-event-log --example event_storage_bench; the manual driver acceptsoriginal=/path/to/baseline candidate=/path/to/candidate --output /fresh/path --iterations 100 --body-pattern repeated(repeat withentropy). Its standard mode creates fresh fixtures with at least five runs; the reported final check instead used the private fixed four-pair schedule on retained matching fixtures.Measured candidate source:
ee2a979fe, release SHA-25668f5701493d715eff9b19427489f9d0dfa20b38787dbf47124c81b7de698b52b; baseline source:2ba23ef3e, SHA-2565d767d3ca440bfaa99b4c8b04abaf77437430592c25025efdb84d6ef5db3e7ba. Linked source/dependency comparisons prove equivalence through the final basedfc556ebfand heada2b7d44a9; all 138 changed files are byte-identical across the final rebase. Raw reports/protocol/audits are retained locally under~/.cache/maka-event-storage-layout, not uploaded with this PR. The tables above are the reviewer-visible evidence summary.AI use
Codex authored implementation, tests and benchmark workflow. A dedicated GPT-6-astra high advisor reviewed correctness, deletion necessity, validation scope, final results and source equivalence. Commits retain
Generated-by: Codextrailers.Checklist
Does this PR entail a change in behavior?