perf(sessions): probe transcript message presence without payload decoding - #138200
Conversation
|
🦞👀 Pull request received. I will update this pull request when review starts. |
|
Codex review: blocked before merge. Reviewed September 4, 2026, 9:07 AM ET / 13:07 UTC. ClawSweeper reviewWhat this changesThe PR adds an index-backed SQLite transcript-presence query so embedded-session bootstrap can detect message history without decoding canonical message payloads. Merge readiness⛔ Blocked before merge - 2 items remain Keep open for normal maintainer acceptance. Current main still decodes transcript rows for this presence check; the proposed accessor-owned index probe preserves the raw fallback for unclassified imports, and no correctness or security defect was found. Automatic cleanup is not appropriate because the PR is marked maintainer and Repository State records a MEMBER author. Priority: P3 Review scores
Verification
How this fits togetherEmbedded agent bootstrap determines whether persisted conversation history exists before invoking context-engine setup. The session accessor reads the agent SQLite transcript store and returns that presence fact to the embedded runner. flowchart LR
A[Embedded session bootstrap] --> B[Transcript presence request]
B --> C[Session SQLite accessor]
C --> D{Identity index has message}
D -->|Yes| E[Return presence without decoding]
D -->|No or unclassified| F[Inspect unclassified raw rows]
E --> G[Context-engine bootstrap]
F --> G
Decision needed
Why: The implementation is focused and no defect was found, but this maintainer-labeled PR is not eligible for automated closure or final acceptance. Before merge
Agent review detailsSecurityNone. PR surfaceSource +57, Tests +164, Other -1. Total +220 across 7 files. View PR surface stats
Review metrics
Technical reviewBest possible solution: Retain the accessor-owned indexed probe and raw fallback, then have the session/persistence owner approve the focused performance change on its exact green head. Do we have a high-confidence way to reproduce the issue? Not applicable: this PR improves an internal persistence read path rather than reporting a user-facing failure. The added test provides a concrete source-level performance scenario with a large canonical transcript row. Is this the best way to solve the issue? Yes. The session accessor owns the SQLite identity index and durable transcript reads, while the embedded runner remains responsible only for choosing the supplied in-memory manager when one exists. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against a220f55bbbe1. LabelsLabel changes:
Label justifications:
EvidenceWhat I checked:
Likely related people:
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
History |
466377d to
81b3d68
Compare
81b3d68 to
2e71ca3
Compare
2e71ca3 to
fd666ae
Compare
sylvesterkaczmarek
left a comment
There was a problem hiding this comment.
The persistence owner is the right place for this presence query. Sharing one SQLite snapshot across indexed and raw fallback probes is important; otherwise concurrent identity classification could turn a physically present message into a false negative.
|
Merged via squash.
|
What Problem This Solves
Embedded session bootstrap decodes a transcript message just to determine whether history exists. Large stored messages therefore allocate unnecessary payload strings and objects before context preparation. A synthetic 16 MiB message made twenty presence checks take 258 ms and grow RSS by 224 MiB.
Why This Change Was Made
Add a typed Kysely presence query at the session accessor boundary using the existing transcript identity index. Canonical message history is answered from index metadata. Exact SQLite imports and id-less rows can legitimately lack identities, and an identity type can be NULL, so unclassified rows retain the existing tolerant JSON parser. Both probes share a deferred SQLite snapshot so concurrent classification cannot hide an always-present message. The raw probe excludes a single set of classified sequences; this avoids SQLite choosing a quadratic per-event identity join. Physical history includes messages before reset or on older branches.
The persistence owner answers the domain query, keeping caller-side payload inspection out of future database adapters. SQLite schema and indexes remain unchanged.
User Impact
Existing embedded sessions avoid decoding stored message payloads during the bootstrap presence check. Detached-session precedence, CLI presence semantics, imported rows, native evidence, malformed rows, and deep JSON keep their existing behavior.
Evidence
idx_agent_transcript_event_sequence.Independent actual embedded-bootstrap-helper validation passed on commit
466377d80d1d589b5ca4cbc83ff2f2190eae5688: twenty reads of 16 MiB canonical history behind reset took 2.27 ms total, detached in-memory ownership remained authoritative, native evidence alone was excluded, nullable identity types were read correctly, and SQLite integrity passed. Materialized source tree455524a787f5a197b3e8ce9e3751ed51749a3393matched before/after.Refreshed onto main
c861706c2d8af6300a8597ebe3bd9f2b96ffcb2dto acquire the independently landed recall-fixture fix in #138123. The feature patch is unchanged. Independent native validation passed again on exact head81b3d68253470b9f9cf323927b08f931596ee132, tree9c14f5854d07f580e5d4b7e89ea270d31c5bac08. The actual embedded helper passed large canonical history, detached ownership, native-evidence exclusion, nullable identity fallback, and SQLite integrity. All 46 bootstrap/import/context tests, including landed eager batching, and fresh P0–P2 review passed. All synthetic processes and state were cleaned up; source stayed clean.Final candidate verification
Final candidate
fd666ae7a105b6dd49fb3354cd4a336a581e0658is rebased onto91aad5cfcf054c05e4c0ac78c0156d07a7a8e6bf. The feature patch is unchanged, and the persistence owners, callers, codecs, and schema contracts have no relevant intervening drift. The shared CI fixture failure was independently repaired in #138238; the canonical repair passed all 1,666 tests in the original 112-file runner configuration on the rebased cron candidate.All eight database changes were exercised together on the final combined staged tree
8fa80406d1564bac0cc3968df20be6ee12d93dc1: 228 tests across 13 files passed. A separate native run used one synthetic state root to verify retained KV/blob facade objects across physical connection reopen, corrupt-sweep rollback, real cron command receipts across Gateway restart, stale/exact delivery acknowledgement and media custody, 1,201-message branch context/presence, transcript byte preservation through two memory reindexes, current-writer success, strict-closure refusal, and stale successor append/branch rejection. A fresh process reopened the final state and passed integrity checks. Source remained unchanged; synthetic state and Gateway processes were removed. This is local macOS runtime proof; hosted platform/security CI remains a separate required gate.