Skip to content

fix(history): bound external CLI replay and turn-index rebuild#504

Draft
ShiboSheng wants to merge 31 commits into
developfrom
codex/issue-443-bounded-external-replay
Draft

fix(history): bound external CLI replay and turn-index rebuild#504
ShiboSheng wants to merge 31 commits into
developfrom
codex/issue-443-bounded-external-replay

Conversation

@ShiboSheng

@ShiboSheng ShiboSheng commented Jul 23, 2026

Copy link
Copy Markdown
Collaborator

Summary

Validation

  • Rust targeted suites passed: session_persistence turn_index, orgtrack_core, org2 event pipeline, agent_core including fix(RAM): replace full-buffer output IPC with bounded appends #425 stress, and perf_utils
  • cargo check --workspace --all-targets passed
  • pnpm typecheck passed
  • full Vitest passed: 694 files and 6,197 tests
  • rendered Issue272 replay passed against the real 351,503,887-byte Codex JSONL: first-open growth 62.2 MiB, settled and repeat-cycle growth 0 MiB, 26 visible events
  • real 3.187 GiB SQLite DB migration and reopen passed: v10 to v12, first open 1.80 s, reopen 50 ms, peak RSS growth 100.3 MiB
  • 30 MiB to 300 MiB cold-index scaling stayed within the bounded-memory gate, and the 327.1 MiB streamed export reproduced the expected hash
  • git diff --check passed

Known external or develop baselines

  • pnpm lint exits 0 with the existing develop hook-dependency warning
  • pnpm check:circular still reports the single cycle already present unchanged on develop
  • strict workspace-wide Clippy is blocked by existing develop warnings; strict orgtrack_core Clippy passes after allowing only the unchanged develop complexity baselines
  • the live dual-instance sharing and presence spec was invoked, but all 12 cases skipped at its credential gate because the six E2E cloud variables are unavailable; this remains an external acceptance blocker and is not reported as a pass

Relates to #443

Large SQLite-backed sessions previously collected every event row and copied full argument, result, and content JSON before determining whether the row could contribute turn metadata. That made index migration memory grow with the complete transcript.

Fold rusqlite rows directly into turn accumulators, classify tool payload requirements before copying large fields, and publish rebuilt summaries transactionally. Unknown tools retain the conservative metadata behavior so the optimization does not change native SDE results.

Verification:
- `cargo test -p session_persistence turn_index`: PASS
- `cargo test -p orgtrack_core`: PASS

Pre-commit hook ran. Total eslint: 0, total circular: 1
The storage-specific replay drivers require incremental SQLite BLOB access, native file notifications, compressed streaming support, stable hashing, and process-level RSS assertions that were not enabled in the existing workspace dependency set.

Enable the required rusqlite BLOB feature and add the notify, flate2, sha2, and libc wiring used by replay indexing, streamed export, and performance tests. Keep the lockfile aligned with the workspace manifests.

Verification:
- `cargo check --workspace --all-targets`: PASS on the final branch
- Cargo lockfile resolution completed with hooks enabled

Pre-commit hook ran. Total eslint: 0, total circular: 1
External history sources do not share one storage format: some append JSONL, some update SQLite or WAL rows, Cursor and Warp use specialized records, and Cline rewrites a whole JSON document. Treating them as one full-history loader caused repeated parsing and session-sized memory growth.

Add the authoritative 15-source registry, storage-specific JSONL, SQLite, structured, and whole-document drivers, compact replay index tables, stable cursors, bounded windows, payload locators, and source conformance fixtures. Each source keeps its own parsing and tool-pairing semantics while sharing the same public replay contract.

Verification:
- `cargo test -p orgtrack_core`: 453 passed, 3 ignored
- 30 MiB and 300 MiB replay performance fixtures: PASS
- Real 335 MiB Codex Issue 272 core replay: PASS

Pre-commit hook ran. Total eslint: 0, total circular: 1
A compact source index alone did not remove the memory amplification because application commands, managed CLI mirrors, EventStore hydration, metadata, provenance, and export paths could still request or materialize complete histories.

Add the Rust replay commands, watcher lifecycle, bounded EventStore application, managed transcript routing, streamed export, collaboration snapshot ingestion, and compact metadata consumers. Keep replay state separate from native SDE persistence and reject mismatched source or session identities.

Verification:
- `cargo test -p org2 agent_sessions::event_pipeline`: 403 passed
- 327 MiB streamed export hash and buffer-bound test: PASS
- Native SDE replay-command isolation assertions: PASS

Pre-commit hook ran. Total eslint: 0, total circular: 1
Even with bounded Rust readers, renderer and background consumers could preserve the old full-transcript behavior by loading all chunks, sending events back to Rust, or building complete arrays for pagination, Fork, Cloud, Raw Transcript, and collaboration flows.

Introduce the typed replay RPC and SessionCore transport, route imported and managed CLI sessions through bounded windows and deltas, virtualize transcript payload access, and migrate Fork, Cloud, hover, provenance, turn loading, and session switching. Native SDE adapters continue using their existing persisted-database mode.

Verification:
- Full Vitest suite on the final branch: 6,197 passed
- `pnpm typecheck`: PASS
- Replay RPC schema, request guard, pagination, lifecycle, Fork, Cloud, and transcript tests: PASS

Pre-commit hook ran. Total eslint: 0, total circular: 1
Bring the issue branch up to the develop snapshot ending at build v1.2.1 so the replay implementation is evaluated against the current source parsers, session directory, cloud synchronization, and dispatch behavior.

Resolve source-adapter and cloud-consumer conflicts by retaining bounded replay ownership of external history while incorporating the develop-side parsing and lifecycle changes. No protected local artifacts are included.

Verification:
- Subsequent branch-wide Rust and Vitest gates recorded in the Issue 443 audit: PASS
- Final branch tree is checked independently after the message-only rewrite

Pre-commit hook ran. Total eslint: 0, total circular: 1
Cline can expose a temporarily invalid whole-document rewrite and Cursor CLI can temporarily present an incompatible lineage. Re-evaluating the same rejected physical snapshot on every poll wasted CPU and repeatedly exercised large parsing paths.

Persist a compact rejection watermark keyed by source identity, parser version, size, timestamp, and fingerprint. Continue serving the previous valid generation until the physical source changes, then clear the watermark only after a valid generation is published atomically.

Verification:
- Rejected Cline and Cursor CLI snapshot regression tests: PASS
- `cargo test -p orgtrack_core`: PASS

Pre-commit hook ran. Total eslint: 0, total circular: 1
The first prewarm implementation returned a bounded SessionEvent array to JavaScript and then sent the same array back to Rust for EventStore application. That preserved an avoidable Rust to JS to Rust copy and allowed stale prewarm completions to outlive a session visit.

Read, normalize, persist Shell payloads, and apply the bounded window directly in Rust. Add prewarm episode guards, reject native SDE identities before allocating replay state, and keep foreground watchers independent from read-only prewarming.

Verification:
- Replay RPC schema and Cursor prewarm guard tests: PASS
- `cargo test -p org2 agent_sessions::event_pipeline`: PASS
- Native SDE external-replay call count remains zero

Pre-commit hook ran. Total eslint: 0, total circular: 1
Several secondary paths could still retain session-sized data after the initial adapter work: replay caches lacked a complete byte policy, payload roots could be decoded eagerly, and Shell, CLI, exporter, and provenance consumers could bypass bounded projection.

Add byte-aware LRU and TTL cache policy, explicit payload encodings and bounded body projections, canonical Shell artifacts, serialized cache access, source routing contracts, and bounded loaders for CLI, export, and provenance. Preserve exact range hydration for callers that explicitly request the original payload.

Verification:
- 30 MiB to 300 MiB bounded-growth tests: PASS
- 10 MiB Shell subprocess and RSS plateau stress tests: PASS
- `cargo test -p agent_core`: 3,090 passed, 2 ignored
- `cargo test -p perf_utils`: 68 passed

Pre-commit hook ran. Total eslint: 0, total circular: 1
Small unit fixtures did not prove the hard memory, IPC, cursor, reset, lifecycle, and source-registration requirements behind Issue 443, and they did not protect the native SDE and prior memory fixes from regression.

Add deterministic large-history performance cases, cache and schema assertions, rendered replay coverage, and independent architecture and frontend audit reports. Cover hard event limits, source parity, unchanged polling, streamed payloads, and the Issue 425, 435, and 446 compatibility boundaries.

Verification:
- Bounded replay performance and cache tests: PASS
- Replay RPC contract tests: PASS
- Rendered chat replay scenarios: PASS

Pre-commit hook ran. Total eslint: 0, total circular: 1
Update the issue branch to the develop snapshot that makes release downloads use bounded retries, avoiding a late integration against stale updater and application scaffolding.

The merge required no replay-specific behavior change. The bounded external-history implementation and native SDE boundary remain unchanged.

Verification:
- Subsequent branch-wide Rust and Vitest gates recorded in the Issue 443 audit: PASS
- Final branch tree is checked independently after the message-only rewrite

Pre-commit hook ran. Total eslint: 0, total circular: 1
Concurrent opens and secondary readers could update the same compact replay cache at the same time, producing avoidable duplicate work and risking stale state publication across OpenCode, export, history, and provenance consumers.

Serialize cache mutation per source session, keep read-only consumers on bounded query paths, and prevent stale renderer retention from reintroducing a released replay window. Preserve generation and revision guards while avoiding duplicate EventStore work.

Verification:
- `cargo test -p orgtrack_core`: PASS
- `cargo test -p org2 agent_sessions::event_pipeline`: PASS
- Session-scoped retention and replay schema tests: PASS

Pre-commit hook ran. Total eslint: 6, total circular: 0
Synthetic fixtures could not reproduce the real cost of opening, switching away from, and reopening the 335 MiB Codex Issue 272 session through the rendered Tauri application.

Add an isolated real-session E2E path that discovers the source through the production rescan command, opens the bounded replay, performs ten release cycles, samples the complete application process tree, and compares native memory attribution with vmmap.

Verification:
- Real Issue 272 rendered E2E: PASS
- First-open growth: 62.2 MiB within the 400 MiB limit
- Ten-cycle measured-tail and backend step growth: 0 MiB
- Native versus vmmap difference: 26.6 MiB within tolerance

Pre-commit hook ran. Total eslint: 6, total circular: 1
Bring in the develop change that hydrates the initial chat page through block loaders so Issue 443 is tested against the current chat initialization path rather than an obsolete renderer flow.

The merge required no replay-specific conflict resolution. Imported sessions remain bounded-replay consumers and native sessions retain the develop block-loader behavior.

Verification:
- Subsequent SessionCore and chat rendering tests: PASS
- Final branch-wide Vitest and Rust gates recorded in the Issue 443 audit: PASS

Pre-commit hook ran. Total eslint: 6, total circular: 0
Sampling memory only one second after release confused delayed allocator reclamation with retained replay state and produced a renderer-dominated false failure without proving an actual leak.

Keep the 250 MiB acceptance threshold unchanged but sample a bounded 30-second idle window after explicit release. Record the lifecycle rationale and verify that request, watcher, payload, and EventStore ownership remain released while the allocator settles.

Verification:
- Real Issue 272 idle-release E2E: PASS
- Settled growth after the bounded idle window: 0 MiB
- No threshold was relaxed to manufacture the pass

Pre-commit hook ran. Total eslint: 6, total circular: 0
Integrate the develop runtime incremental-scan work before final acceptance so replay routing, source discovery, session aggregation, and collaboration behavior are evaluated on the same code that will receive Issue 443.

Resolve conflicts across Codex indexing, shared session event types, replay commands, source descriptors, SessionCore adapters, and collaboration imports. Preserve develop incremental scanning while retaining the exhaustive 15-source bounded replay contract and native SDE separation.

Verification:
- Merge-conflict focused Vitest suite: 58 passed
- Full Rust and Vitest gates were rerun after conflict resolution

Pre-commit hook ran. Total eslint: 6, total circular: 0
Update the issue branch to the develop snapshot that expands usage trends before running the final static, Rust, real SQLite, and rendered Issue 272 acceptance gates.

The merge introduced no product conflict in bounded replay. Follow-up test fixtures were updated separately to reflect the current native history-mode and webview observer behavior.

Verification:
- Relevant post-merge targeted tests: PASS
- Full Vitest suite after fixture alignment: PASS
- Final real-data gates were rerun on the merged tree

Pre-commit hook ran. Total eslint: 6, total circular: 0
The latest develop merge changed native history-mode classification and webview visibility observation, leaving two test fixtures modeled on obsolete behavior. Final reports also needed the current real-data measurements rather than pre-merge evidence.

Align the native SDE reconciliation fixture with persisted-database mode, update the webview harness for IntersectionObserver and ResizeObserver, tighten real-session discovery, and record the final architecture and rendered-E2E results. No production behavior is changed by the fixture updates.

Verification:
- Updated targeted Vitest cases: 7 passed
- Full Vitest suite: PASS
- Real 3.0 GiB SQLite migration and reopen: PASS
- Real 335 MiB Issue 272 rendered E2E: PASS

Pre-commit hook ran. Total eslint: 6, total circular: 0
Merge the final develop snapshot that persists the selected organization so the Issue 443 branch is based on the latest shared application state before delivery.

The merge required no bounded-replay conflict resolution. The final static checks, Rust suites, real SQLite migration, and rendered Issue 272 lifecycle test were evaluated after this synchronization.

Verification:
- `pnpm typecheck` and full Vitest suite: PASS
- Required Rust package suites: PASS
- Real SQLite and Issue 272 rendered acceptance: PASS

Pre-commit hook ran. Total eslint: 6, total circular: 0
The audit reports still contained intermediate counts and pre-final-merge wording, which made it difficult for reviewers to distinguish completed acceptance evidence from external infrastructure that remained unavailable.

Record the final Vitest count, real SQLite migration and reopen measurements, Issue 272 first-open and lifecycle memory results, native vmmap comparison, and the explicit cloud credential blocker. Keep baseline circular and workspace clippy findings separate from Issue 443 results.

Verification:
- Final audit evidence cross-checked against completed command output
- `git diff --check`: PASS

Pre-commit hook ran. Total eslint: 6, total circular: 0
@ShiboSheng
ShiboSheng force-pushed the codex/issue-443-bounded-external-replay branch from 92f2065 to 1f34dae Compare July 23, 2026 15:50
@Harry19081

Copy link
Copy Markdown
Member

Reviewed with fable:

Verdict: high-quality engineering with four blocking-caliber problems. This is really a re-architecture (~53k production/test lines across 290 files), not a "fix": a new bounded-replay subsystem for 15 external CLI sources, a v10→v12 streamed turn-index rebuild, a cloud-transport rewrite, and a staged collaboration-snapshot ingest. The code is unusually disciplined — parameterized SQL throughout, checked arithmetic, TTL'd/capped registries, an atomic rollback-tested migration, and tests that were strengthened rather than gutted. But five parallel deep-dives (Rust replay, event-pipeline commands, turn-index/CLI, frontend sync, frontend API/UI) plus cross-checks surfaced the following, in priority order.

Blocking findings

  1. Arbitrary file write reachable from the webview. external_replay_stream_export (src-tauri/src/agent_sessions/event_pipeline/commands/external_replay.rs:1104) accepts destination_path: String straight from IPC and will create_dir_all the parent and fs::rename over any user-writable path — no scoping to dialog-picked locations, no symlink checks. I verified this directly: the path flows unvalidated into stream_replay_export. A compromised renderer gets an arbitrary-destination overwrite primitive whose content embeds transcript payloads. The frontend happens to pass dialog-chosen paths; the backend must not rely on that.

  2. Snapshot ingest can destructively rewrite any native session. validate_session_id (collaboration_snapshot_ingest.rs:310) accepts any agentsession-* id on a prefix + no-slash check, and publish_staged_snapshot then runs DELETE FROM events WHERE session_id=?1 (line 2339) and re-inserts renderer-supplied rows, converting the session into a snapshot-backed fork. agentsession- is the general native-session prefix — nothing verifies the target is a fresh fork id rather than an existing session with real history. One bad or hostile renderer call erases a native session's persisted transcript, directly undercutting the PR's own "native sessions keep their persisted-db path" invariant. Verified in code.

  3. Cursor IDE/Windsurf KV driver: three inconsistent numbering schemes. In stream_kv_bubbles (replay/sqlite_driver.rs:723-830), turn headers use raw header-array indexes, events use a deduplicated value-present ordinal, and the two disagree again on leading assistant bubbles. I verified the core asymmetry: the skip path increments ordinal unconditionally while the emit path increments only when the KV row exists. On messy real stores (duplicate/empty bubbleIds, missing bubble rows, assistant-first conversations — all anticipated by the code itself) this yields wrong turn windows, perpetual re-hydration (indexed count never matches header event_count), duplicate events at two sequences, or a hard sync failure via PK collision. The conformance fixture is clean, so no test can catch any of it.

  4. New E2E hard-fails every default run. The "issue-443-real-codex" scenario (tests/e2e/specs/core/chat-rendering-ui.spec.mjs:2393) runs whenever the scenario filter is unset and throws immediately if E2E_ISSUE_443_REAL_CODEX_SESSION_ID is missing — no auto-skip. It also needs a real Codex session on disk and macOS vmmap. Meanwhile the previous external-history reload E2E was re-pointed at a native sdeagent-* session, so the PR's headline path has zero default-suite E2E coverage and unfiltered CI goes red on any machine without the author's fixture.

Major operational risks
5. Global writer stall during exports. for_each_imported_replay_chunk (src-tauri/src/orgtrack/exporter/loaders.rs:1150) holds the process-wide sessions-writer mutex while streaming entire replay pages and writing the export file to disk; reconcile_imported_session holds it plus a BEGIN IMMEDIATE per session, and imported-session metadata reads now route through the writer too (history_commands.rs:102). A multi-hundred-MiB export stalls all live event persistence for its full duration. The path already validates a pinned generation/revision cursor at the end — fail-and-retry would work without excluding all writers.

  1. CLI content search became all-or-nothing. content_index::update now collects per-session bodies with collect::<Result<...>>()? — a single failing session (file deleted after cataloging, one chunk over the 512 KiB IPC budget) aborts the entire search command. The old code indexed that session with an empty body and moved on (orgtrack-cli/src/content_index.rs).

  2. Legacy oversized cloud rows become permanently un-importable. Both layers now hard-reject any stored wire over 256 KiB — validateWirePageResponse in src/features/Org2Cloud/org2CloudSyncClient.ts:489 and validate_page_contract (collaboration_snapshot_ingest.rs:963) — and there is no re-chunking path for legacy V1 rows. Cross-checking confirmed the upload side is fine (Rust splits oversized events into ≤256 KiB attachment-V2 frames, and post-fork turns are correctly included in fork spools), but any pre-existing cloud session containing one oversized row can no longer be imported or forked unless a server-side migration rewrites it. Needs an explicit answer before merge.

Moderate
Memory-bound guarantees have no CI enforcement. All five RSS/acceptance stress tests are #[ignore]d; two require env vars pointing at files only the author has. The always-on suite enforces IO byte counters and window caps (genuinely strong), but the headline memory bound is manual-only. Relatedly, JSONL drivers read_until with no line-size cap (jsonl_driver.rs:175) — the real invariant is "bounded by the largest source line", and the Qoder 32 MiB guard checks after buffering.
LIKE wildcard injection in session resolution (replay/index.rs:1839): the suffix-match fallback interpolates the session key unescaped, so ids containing _/% can resolve to a different session of the same source. Same-user data only, but it defeats the registry's routing guarantee.
Qoder sequence saturation: next_sequence steps by 10^15 with saturating_add — after ~9,223 records every event lands on i64::MAX and silently overwrites its predecessor (jsonl_driver.rs:40).
Fallback byte-range reads misreport offsets when a read starts mid-UTF-8-scalar, shifting subsequent pages (jsonl_driver.rs:1490, codex_jsonl.rs:1210); multi-span reconstruction silently misaligns when a span stops yielding (the artifact path does this correctly; the fallback is untested for it).
Codex sessions show zero tokens/impact until replay-indexed — catalog rows now hardcode zeros and completed sessions are never proactively indexed, so fresh or rescanned sessions display 0 in listings and the usage dashboard until opened. Deliberate, but a visible regression from full-metadata-at-discovery.
Two frontend races: loadOlder doesn't increment the request epoch, so it can overwrite a just-completed refresh with a merge built from the pre-refresh snapshot (useSessionRawTranscript.ts:97); backward pagination stalls permanently when a page returns zero renderable events, with no retry affordance after a failed page.
open_window registers epoch state before identity validation (external_replay.rs:325) — the prewarm path deliberately validates first; spoofed ids can LRU-evict live sessions' replay tickets (nuisance-DoS, self-healing). The foreground store publish also lacks the linearized manager→stores→epochs coupling that prewarm was explicitly hardened with.
Torn cloud epoch rewrite: the old atomic rewrite RPC is now first-page-rewrite + N appends; a crash mid-sequence leaves a truncated-but-consistent-looking server transcript a teammate can fork until the next push pass heals it (org2CloudSessionSync.ts:1080).
Minor (grouped)
i18n: new raw-transcript strings (exportAll, exportSuccess, exportFailed, copyTooLarge, newerReleased) exist in no locale file; the payload-expander uses common:showMore, so it renders "Show more" instead of "Read payload" everywhere. (The uncommitted sessions.json edits sitting in your working tree look like exactly these keys — they never made it into the branch.)
Hygiene: cloud spool SQLite files go to the world-readable system temp dir and linger after renderer crashes until the next prepare; watcher registry prunes only on acquire, so the last notify handle can outlive its use for the process lifetime (both capped/bounded).
Silent fallbacks: ingestRemoteSnapshot swallows every non-abort forward-delta error into an unlogged full re-download (collabSnapshotIngest.ts:243); FNV-1a change-detection hashes can (rarely) skip a changed event.
Consistency nits: collaborationSnapshotIngest.abort uses z.void() where the PR's own TauriUnit pattern exists to handle Rust ()→null; KV/structured drivers rebuild all turn headers on every delta (O(total events) write amplification per poll); CLI show CSV/MD output now differs between processor and non-processor runs of the same session.
Scope creep: unrelated docs modified (WorktreeDataFlow, BranchPalette, ChatPanel audits, orgtrack-plugins/triggers-design.md); legacy pre-upgrade fork handoffs degrade to "No usable transcript items were found" (intentional and tested, but one-shot context loss).
What holds up (verified, not just claimed)
The v12 rebuild is genuinely one BEGIN IMMEDIATE transaction — atomic, idempotent, with a rollback test proving the old generation survives failures; the streamed export has byte-for-byte golden and SHA-256 boundary tests plus a correct temp+fsync+rename publish. The native-SDE isolation guard is real: fail-closed chokepoints (resolve_target, reject_bounded_replay_full_load) with non-trivial tests including a zero-DB-probe counter. RPC schemas enforce the 10-turn/200-event/4 MiB/256 KiB budgets with real tests. Production code is essentially panic-free, locks are never held across .await, every global registry is bounded, and the big test-file diffs are rewrites tracking deleted code, not weakened assertions.

Recommendation
Request changes. Before merge: (1) validate/scope the export destination path, (2) restrict snapshot ingest so it can never target an agentsession-* id that has existing non-snapshot history, (3) fix or fixture-test the Cursor KV numbering, (4) make the issue-443 E2E auto-skip without its env var, and (5) answer the legacy >256 KiB cloud-row migration question. Strongly recommended alongside: stop holding the global writer across streamed exports, restore per-session error tolerance in CLI content indexing, and land the missing i18n keys. The collaboration_snapshot_ingest subsystem would honestly have been safer as its own PR, but it is load-bearing here, not opportunistic.

@Harry19081

Copy link
Copy Markdown
Member

What the PR is actually solving
One problem, stated in issue #443: opening an external CLI session's history loaded the entire transcript into memory. Before this PR, when ORGII showed a session imported from Codex, Claude Code, Cursor, etc., nearly every consumer — open, refresh, hover card, fork, raw transcript, export, cloud push — called a "load full transcript" API. That means a 350 MB Codex JSONL or a 3 GB Cursor SQLite got fully parsed in Rust and largely shipped to the renderer. Memory was unbounded by design, and it showed up as the RSS blowups documented in the linked issues (#272, #425).

The fix's core idea is small: replace "load everything" with bounded windows (max 10 turns / 200 events / 4 MiB per IPC response), backed by a persistent SQLite index of compact rows plus payload locators — pointers back into the source file so large payloads are range-read in ≤256 KiB slices only when actually viewed. The PR's own numbers: first open of the 335 MB Codex session costs 62 MB once, then repeat opens/polls cost ~0.

Why that costs 60k lines
The idea is small; the blast radius isn't. Roughly (measured from the diff):

~20k — the new replay subsystem in orgtrack-core. "Bounded" can't be implemented generically because the 15 supported sources store sessions in genuinely different shapes: append-only JSONL logs, Cursor's SQLite KV bubble store, whole-JSON-file formats, Qoder's sidecar. So there are six storage drivers plus the shared index, cache policy, registry, metadata projection, and content-addressed payload artifacts — and each driver must handle incremental sync, torn tails mid-write, UTF-8-safe range reads, and turn-header reconstruction on its own format. The 15-source × per-format-driver matrix is the single biggest multiplier (and, per my earlier pass, carries some avoidable copy-paste between drivers).
~11k — the app-side command layer. external_replay.rs + the file watcher: window open/poll/read/prewarm commands, generation/epoch guards so stale results can't land, bounded cloud-spool encoding, streamed export. About a third of that file is inline tests.
~3.6k — collaboration snapshot ingest, a forced consequence. Once full hydration is fail-closed, the old cloud import path — where the renderer shipped a complete SessionEvent[] — is illegal by the PR's own rule. It had to be rebuilt as a staged, hash-verified, byte-bounded ingest in Rust. Not opportunistic, but it's a whole subsystem riding along.
~12k — rewriting every consumer that assumed full transcripts. This is the long tail that makes it 290 files: exporter, CLI show/search/content index, turn-metadata projector, provenance backfill, turn-index rebuild (now streamed + transactional, v10→v12), and the frontend half — the Org2Cloud transport rewrite, TeamCollaboration sync, transcript dialog turned into a windowed/paginated view. Each old full-load API was then deleted, which is most of the 6.8k deletions.
~14–16k total tests/fixtures (8k in dedicated test/fixture files + inline Rust test modules): conformance suite across all 15 sources, perf/RSS harnesses, and wholesale rewrites of the cloud-sync test files whose old shape tested deleted code.
The honest assessment
The scope is coherent — one invariant ("no full transcript hydration anywhere") enforced across every path that touches external history, and the invariant is what forces the breadth: a single surviving full-load caller voids the guarantee. But it didn't have to be one PR. The cloud/collab transport rewrite (~15k across both languages) and the snapshot-ingest subsystem are separable phases that could have shipped behind a temporary carve-out, and the review findings reflect that: the two worst bugs (the ingest wipe primitive, the export path write) both live in the riding-along subsystems, not in the core bounded-replay idea.

@Harry19081

Copy link
Copy Markdown
Member

Worth requesting on this PR

  1. A shared driver-utility module under replay/ (~600–800 lines deleted, kills two live bug classes). The six storage drivers each re-implement the same helpers privately:

utf8_boundary_at_or_before/after — identical copies in jsonl_driver.rs:1643 and codex_jsonl.rs:1362
the FNV-1a hash — five copies under two names (content_hash in jsonl/codex, hash_parts in sqlite/structured/whole_json), a textbook naming-overload smell
rebuild_turns — byte-near-identical in sqlite_driver.rs:1481 and structured_driver.rs:1617
raw read_until(b'\n') line reading in three drivers, with a size cap only in qoder_sidecar (checked after buffering)
payload range-from-text — structured_driver.rs:2260 has the correct implementation; jsonl and codex each carry a private near-copy with the same UTF-8 offset bug
That last pair is the strongest argument: the same bug appearing in exactly the two hand-rolled copies while the shared-able version is correct is the definition of duplication cost. One read_bounded_line with a pre-enforced cap also fixes the unbounded-line-buffering finding in all three drivers at once.

  1. One definition of the attachment-V2 wire format. Right now the format (magic ORGII-REPLAY-ATTACHMENT-V2\0, 176 KiB chunks, 256 KiB wire cap) is independently defined in three places: the encoder in external_replay.rs:1132-1140, the decoder in collaboration_snapshot_ingest.rs:35-40, and a full third implementation in TypeScript (replayAttachmentCodec.ts, 384 lines, self-described as test/reference-only). Extract a small Rust cloud_wire module shared by encoder and decoder, and delete the TS codec in favor of testing against Rust-produced golden fixtures — that also removes the TS decoder's zip-bomb gap before anyone "promotes" it to production.

  2. Unify the window-publish path in external_replay.rs. The prewarm path has the hardened, linearized apply_prewarm_window_if_current (validate identity first, then registry+store+epoch as one operation); the foreground open/poll/read paths are a parallel check-then-act implementation of the same job. Making foreground use the same validate-then-publish routine deletes a parallel path and fixes two review findings for free (epoch registration before identity validation; the non-linearizable store publish).

  3. Ingest should reuse the existing identity resolver. collaboration_snapshot_ingest.rs rolls its own validate_session_id prefix check instead of routing through the exhaustive resolve_target/driver classifier the rest of the PR is built on. Sharing the resolver — plus a precondition that the target is snapshot-backed or empty — is simultaneously the fix for the wipe-any-native-session bug and the removal of a second, weaker validation layer.

Better as follow-ups

  1. Split external_replay.rs (10.6k lines): the segment/attachment encoders, cloud spool management, epoch registries, and watch-set logic are four separable modules currently welded to the command surface. No line savings, but it's the difference between reviewable and not.
  2. Fold the legacy full loaders into the replay drivers. The PR's own audit doc says the old per-source loaders survive only as "parser/test compatibility surfaces" — meaning every source still has two parsers. Making legacy loaders thin wrappers over the driver parsers (or deleting test-only ones) removes a parallel parse path per source, but it's 15-source surgery that shouldn't grow this PR.
  3. Break down other large files.
  4. Trivial: use the PR's own TauriUnit helper for collaborationSnapshotIngest.abort instead of z.void().

Net effect of the pre-merge items: roughly 1.5–2k lines deleted, three findings fixed structurally rather than patched, and the drift-prone format/validation logic reduced to single sources of truth. If you want, I can draft this as review comments on the PR.

@Harry19081

Copy link
Copy Markdown
Member

Reuse already happens, ad hoc, in three of six drivers:

whole_json_driver imports structured_driver::{compact_chunk, range_from_text, rebuild_turns} — the model citizen
codex_jsonl imports jsonl_driver::{boundary_fingerprint, compact_tool_args}
qoder_sidecar imports from jsonl_driver
but sqlite_driver imports nothing — it keeps private copies of rebuild_turns and the hash even though structured_driver already exports shared pub(super) versions that whole_json_driver consumes
So there's no principled boundary being protected here — each driver author just decided locally whether to import or copy. That inconsistency is the tell that a sanctioned shared home is missing, not that the formats are too different to share.

The three extractions, biggest first

  1. An append-log sync template for the two JSONL drivers. I compared jsonl_driver::sync and codex_jsonl::sync side by side: they are line-for-line the same skeleton — decode cursor → open → seek to byte_offset → BufReader → loop { record line start, read_until(b'\n'), torn-tail check, parse, upsert } — differing only in the per-record normalization (Codex's span/background-output handling vs the generic mapping). That's the classic template-method extraction: one shared sync_append_log(tx, cfg, normalize_record) where each driver supplies only its normalizer. Codex already reaching into jsonl_driver for helpers shows the module boundary isn't load-bearing. This is where the serious lines go — and it's also where the duplicated loop carries duplicated bugs (the identical unbounded read_until, the identical UTF-8 offset bug in read_payload).

  2. sqlite_driver should consume structured_driver's existing shared surface — rebuild_turns, range_from_text, the hash — exactly as whole_json_driver already does. Zero new abstraction needed; the export already exists. The genuinely KV-specific part (bubble headers, the numbering logic that has the correctness bug) stays put.

  3. A formal driver trait to replace the duck-typed convention. Every driver exports the same shape by naming convention — cursor_fingerprint/cursor_lineage_matches, sync, read_payload, fallback counters — and index.rs hand-wires them in six-way match arms at roughly eight dispatch sites (cursor checks at lines 144–180, sync at 268–387, payload at 1536–1571). A ReplayDriver trait with a small capability surface (plus an optional sidecar/KV-hydrate extension for qoder/cursor) collapses those match blocks into one dispatch table. The PR's authors clearly care about this — they built the exhaustive replay_driver_kind classifier specifically so a new source can't compile without a driver — and a trait gives them that same compile-time exhaustiveness with far less repetition.

The payoff and the caveat
Combined with the helper module from before, this is roughly 1.5–2k more lines out, but the strategic win is bigger: source #16 becomes a config entry plus a normalizer function instead of a new 1.5–2.5k-line file that re-copies the loop — and this codebase demonstrably keeps adding sources (15 and counting).

The caveat: don't push past that. The per-format sync bodies for the SQLite/structured/whole-JSON families do genuinely different work (KV bubble traversal vs. table diffing vs. array diffing), and forcing all six under one universal template would be the wrong abstraction. The high-confidence extractions are the ones above, where the code is already identical or already half-shared.

@Harry19081

Copy link
Copy Markdown
Member

Worth consolidating

  1. Four hand-rolled bounded-registry implementations in the command layer. external_replay.rs + the watcher each implement their own "map with TTL + cap + eviction" from scratch:

replay request epochs — TTL 5 min, cap 64, LRU (external_replay.rs:4814)
prewarm epochs — literally identical constants, separate implementation (:4816)
cloud spools — TTL 10 min + lease refcounts (:1141)
watch registry — TTL 3 min + byte budget + evict_lru_until_capacity (external_replay_watcher.rs:18-21, 442, 480)
plus the prune gate (cap 16). One generic BoundedTtlMap<K, V> with a pluggable eviction policy replaces four-plus prune/evict/expire implementations. The replay/prewarm epoch pair is the immediate win — same constants, same semantics, two code paths (and it's exactly where the validate-before-register asymmetry bug lives; one implementation means one place to get the ordering right).

  1. Two parallel usage/token extractors for the same fields. Token/impact parsing exists in both sources/codex/app/meta.rs (the legacy discovery-time parse, now mostly hardcoding zeros) and imported_history/catalog.rs (the new replay-index backfill, 32 extraction sites). The PR made the backfill canonical but left the legacy extractor in place — which is also why the "sessions show 0 tokens until opened" regression exists. Pick one: either the catalog backfill is the single extractor and meta.rs's token code is deleted, or discovery reuses the catalog's extraction on its 1 MiB prefix. Right now the two can silently diverge.

  2. Five names for one concept: "is this persisted cursor still valid?" Across the six drivers the same responsibility is spelled cursor_fingerprint + cursor_matches_source (jsonl, codex, whole_json), cursor_signature + cursor_lineage_matches (qoder), cursor_schema_version + database_schema_version (sqlite, structured — duplicated between them), and cursor_lineage_matches again with a different signature (structured). index.rs then stitches these variants together in per-family conditionals (lines 116–180). This is the audit skill's Layer 3/4 problem — same semantics, five vocabularies. One trait method returning a CursorVerdict { Reusable, Rebuild(reason) } folds all of it, and the schema-version pair dedups immediately even without the trait.

  3. Wire budgets declared in four layers with no shared source. The 256 KiB / 4 MiB / 10-turn / 200-event caps are independently declared in core replay/mod.rs, the command layer (three separate 256 * 1024 consts in external_replay.rs alone, plus ingest's MAX_WIRE_BYTES), the TS zod schemas, and the TS client (REPLAY_ATTACHMENT_WIRE_MAX_BYTES). A mismatch here doesn't fail loudly — it manifests as the exact class of compatibility cliff we flagged on the 256 KiB legacy rows. The PR already invented the right pattern for this — its TS↔Rust source-registry parity test — so the cheap version is a budgets parity test; the better version is generating the TS constants/schema bounds from the Rust definitions. (Small rider: the triplicated payload_fallback_decodes test counters fold into the shared driver module for free.)

Deliberately not consolidating
TurnDraftFolder (session-persistence) vs rebuild_turns (replay index) — both fold event streams into turns, but over different schemas, different domains (native vs imported), with an equivalence-tested legacy contract on the native side. Merging them would couple the two databases' semantics for cosmetic savings.
TS transport guards (externalReplayTransport single-flighting, lease epochs) vs Rust epoch registries — they look similar but guard different failure domains (renderer lifecycle vs backend state). The frontend reviewer confirmed both are individually correct; a shared abstraction has nowhere real to live across the IPC boundary.
Full consolidated priority order across everything we've discussed: driver-common module + append-log template (fixes live bugs), ingest-resolver reuse (fixes the wipe bug), epoch-registry merge (fixes the ordering bug), V2 wire format single-sourcing, budgets parity, then the naming/trait cleanup as the tail. Happy to write this up as PR comments or a docs/architecture-audit report in the repo's format if you want to hand it to the author.

- split the external replay app bridge by window, payload, export,
  cloud, shell, lifecycle, collaboration, and managed-chunk concerns
- separate replay storage drivers by source family and move large
  inline test modules into focused test files
- replace stale Cursor-only and misleading legacy terminology with
  source-neutral bounded-replay names
- align replay generation, revision, episode, and request-token terms
  across Rust and TypeScript
- remove dead compatibility aliases introduced during the #443
  migration and format all branch-touched files
- preserve Tauri commands, wire values, database schemas, parser
  versions, replay limits, and native SDE behavior

No behavior or wire-format changes.

Refs #443
…bounded-external-replay

# Conflicts:
#	src-tauri/crates/orgtrack-core/src/sources/codex/app/index.rs
#	src-tauri/crates/orgtrack-core/src/sources/codex/app/meta.rs
#	src-tauri/crates/orgtrack-core/src/sources/imported_history/mod.rs
#	src-tauri/crates/orgtrack-core/src/store/sqlite/schema.rs
#	src/features/Org2Cloud/org2CloudBackendAdapter.test.ts
#	src/features/Org2Cloud/org2CloudBackendAdapter.ts

Pre-commit hook ran. Total eslint: 1, total circular: 1
Move bounded replay export destination selection into the trusted Rust/Tauri layer so renderer input can only suggest a file name. Harden export writes with existing-parent validation, symlink and directory refusal, exclusive private temp files, fsync, and atomic replacement while preserving the old destination on failure.

Classify collaboration snapshot destinations inside the same immediate transaction used for publication. Allow imported sessions, fresh native forks, and existing snapshots, but reject replacement of ordinary native Agent events and keep their data intact.

Update the RPC/UI cancellation contract and add focused Rust, schema, and UI tests for traversal stripping, missing parents, symlinks, atomic failure cleanup, and native snapshot refusal.

Refs #443

Pre-commit hook ran. Total eslint: 1, total circular: 1
Build one source-neutral KV order after filtering blank, undefined, duplicate, and stale composer headers, then append persisted fallback bubbles in stable database order. Derive event sequences, lazy turn ranges, compact turn headers, source counts, and fallback user-turn boundaries from that same order.

Apply the identical contract to Cursor IDE and Windsurf, and bump both adapters to parser version 2 so existing compact indexes rebuild instead of retaining mismatched ordinals.

Add a shared dirty-store fixture covering missing rows, duplicate headers, composer lag, fallback user turns, contiguous pagination, stable IDs, and exact turn ranges. The full orgtrack_core suite passes with 472 tests and 4 existing ignored stress cases.

Refs #443

Pre-commit hook ran. Total eslint: 2, total circular: 0
- cap JSONL and Qoder sidecar records before allocation and preserve cursors on torn or oversized input
- centralize UTF-8 payload ranges so adjusted offsets make progress without skipping later decoded spans
- replace correctness fingerprints with length-delimited SHA-256 while preserving compatibility event IDs
- reject Qoder sequence overflow and rebuild sidecar cursors under parser version 4
- treat imported-session suffix wildcards literally across replay lookup and managed CLI dedup
- share storage-neutral turn folding and add regression coverage for every boundary

Refs #443

Pre-commit hook ran. Total eslint: 2, total circular: 0
- release the global sessions writer lock before pinned replay export I/O and revalidate the final source revision before publication
- retry and skip individual CLI content-index failures without discarding healthy session updates
- serialize Raw Transcript refresh and backward pagination with a shared request token and immutable replay-version guard
- scan filtered backward pages with a bounded loop and expose an explicit retry affordance instead of stalling pagination
- cover writer-lock release, retry bounds, empty-page progress, and replay-version mismatch behavior

Refs #443

Pre-commit hook ran. Total eslint: 2, total circular: 0
- validate replay identities before allocating foreground or prewarm request tickets
- share one bounded request registry and atomically publish foreground/prewarm windows under the manager-store-registry lock order
- move Attachment V2 framing, validation, encoder/decoder constants, and golden coverage into one authoritative Rust module
- remove the duplicate test-only TypeScript attachment codec and reuse the Rust frame contract in collaboration ingest tests
- source replay and Cloud transport limits from a shared budget manifest with Rust parity assertions
- preserve Tauri commands, JSON fields, database schemas, provider IDs, and native SDE EventStore behavior

Refs #443

Pre-commit hook ran. Total eslint: 2, total circular: 0
- admit one bounded pre-V2 Cloud row for streaming Rust verification while keeping new Attachment V2 rows under the 256 KiB physical limit
- stream legacy base64 decoding, wire measurement, and page hashing without allocating duplicate session-sized buffers
- upload external replay frozen rows to private Storage in bounded batches and retain only compact object references in the renderer
- publish complete native and external replay epochs with one atomic rewrite RPC so crashes cannot expose a truncated transcript
- keep old epochs authoritative until final publication and leave interrupted uploads unreachable
- cover legacy migration, V2 fail-closed behavior, storage capability failure, rewrite conflicts, cancellation, and large atomic rewrites

Preserves the existing Cloud RPC names, segment schema, replay source IDs, parser versions, and native EventStore behavior.

Refs #443

Pre-commit hook ran. Total eslint: 2, total circular: 0
- store Cloud replay spools in an owner-only ORGII runtime directory and schedule TTL cleanup even when the renderer disappears
- retire expired external-history watcher handles without waiting for another acquire while preserving the three-minute lease boundary
- seed an isolated Codex JSONL fixture for the default rendered suite and exercise the production bounded open, render, and release path
- auto-skip the real Issue 272 memory scenario when its explicit session id is absent while failing fast when that scenario is requested directly
- localize Raw Transcript export, payload-range, and bounded-window notices across every shipped locale
- log forward-delta fallback before a bounded authoritative snapshot rebuild and share the Rust unit-response schema
- remove redundant replay-export references surfaced by Clippy without changing pinned generation semantics

Preserves replay commands, wire values, storage schemas, limits, adapter semantics, and native SDE behavior.

Refs #443

Pre-commit hook ran. Total eslint: 2, total circular: 0
- apply rustfmt to the remaining branch-touched replay watermark and provider test files
- record final Rust, Vitest, circular-dependency, SQLite, and large-session performance results
- document the rendered Issue 272 run and Command+5 manual lifecycle verification
- refresh the UI audit with the final localized Raw Transcript coverage
- preserve replay behavior, wire formats, database schemas, source IDs, limits, and native SDE routing

No behavior or wire-format changes.

Refs #443

Pre-commit hook ran. Total eslint: 2, total circular: 0
- merge upstream/develop at 2c8d18a
- retain bounded replay changes across the automatically merged orgtrack cache, repository identity, fork, and rendered-chat test files
- incorporate the latest README and org2AI repository identity updates
- preserve all protected artifacts and checkpoint files outside the commit

No manual conflict resolution or #443 behavior change was required.

Refs #443

Pre-commit hook ran. Total eslint: 2, total circular: 0
@ShiboSheng ShiboSheng changed the title fix(history): bound external CLI replay and turn-index rebuild fix(history): make external CLI replay and turn indexing memory-bounded Jul 25, 2026
@ShiboSheng ShiboSheng changed the title fix(history): make external CLI replay and turn indexing memory-bounded fix(history): bound external CLI replay and turn-index rebuild Jul 25, 2026
@ShiboSheng

Copy link
Copy Markdown
Collaborator Author

@Harry19081 Thanks for the detailed review. Replying to the blocking/operational findings in this comment.

I treated the four blocking findings, the legacy Cloud migration question, and the strongly recommended operational fixes as pre-merge work rather than documentation-only follow-ups.

Blocking and major findings

Finding Resolution Commit / evidence
Renderer-controlled arbitrary export destination Fixed. The renderer now supplies only a suggested file name; trusted Rust code selects and validates the destination. Missing parents, directories, symlinks, and unsafe replacement targets are rejected. Export uses an owner-private temp file, fsync, and atomic replacement while preserving an existing destination on failure. 25a35bdd9 plus traversal, symlink, atomic-failure, and preservation tests
Snapshot ingest could erase an existing native session Fixed. Destination classification and the snapshot/empty/native precondition now run inside the same immediate publication transaction. Imported sessions, existing snapshots, and fresh empty native forks are allowed; ordinary native Agent history is rejected and left intact. 25a35bdd9 plus native-history refusal/rollback tests
Cursor IDE/Windsurf used inconsistent KV numbering Fixed structurally. Blank/missing/duplicate/stale headers and persisted fallback bubbles are first folded into one canonical source-neutral order; event sequences, turn ranges, counts, headers, and fallback user boundaries all derive from that same order. Both adapters were bumped to parser v2 so stale indexes rebuild. 0afe47820 plus a shared dirty-store differential fixture
Default E2E required the author's real Issue 272 session Fixed. The default rendered suite seeds an isolated Codex JSONL fixture and exercises the production rescan → bounded open → render → release path. The real Issue 272 scenario auto-skips when not requested, and fails fast only when explicitly selected without its session id. 65dacbcee; post-merge isolated rendered spec passes
Legacy Cloud rows over 256 KiB had no migration path Fixed. One bounded pre-V2 row may enter the trusted Rust streaming verifier; it is incrementally base64-decoded, measured, hashed, reframed into Attachment V2, uploaded in bounded batches, and published only through a complete atomic epoch rewrite. New V2 physical rows remain capped at 256 KiB. 98f454e89 plus legacy migration, V2 fail-closed, cancellation, storage-failure, OCC, and large atomic-rewrite tests
Export held the process-wide sessions writer Fixed. The global writer is released before pinned replay export I/O; the final source revision/generation is revalidated before publication so stale output fails instead of blocking all writers. 8417d354e writer-lock regression test
CLI content indexing became all-or-nothing Fixed. Per-session failures are logged/skipped and healthy sessions continue indexing; retry work remains bounded. 8417d354e
Torn Cloud epoch rewrite Fixed. Old epochs remain authoritative until all bounded segments/object references are ready, then one atomic rewrite publishes the complete epoch. Interrupted uploads remain unreachable. 98f454e89

Moderate/minor findings

  • JSONL line allocation, UTF-8 offsets, multi-span reads, wildcard matching, Qoder overflow, and FNV collision risk: fixed together in ec0e7e413. All append-log readers use a pre-enforced bounded-line reader; payload ranges share UTF-8-safe progress semantics; hashes are length-delimited SHA-256; SQL suffix wildcards are escaped; sequence overflow rebuilds/fails instead of saturating.
  • Raw Transcript refresh/older-page race and zero-renderable-page stall: fixed in 8417d354e with one request token/version guard, bounded backward scanning, and an explicit retry state.
  • Identity validation before request registration and foreground/prewarm publication ordering: fixed in 92b8db2cd by using one bounded request registry and the same validate-then-publish lock order for both paths.
  • Missing Raw Transcript localization, spool privacy/cleanup, watcher lifetime, silent forward-delta fallback, and Rust unit response schema: fixed in 65dacbcee. All 13 shipped locales are covered; spools live in an owner-only ORGII runtime directory with TTL cleanup; expired watchers retire without a future acquire; fallback is logged; collaboration abort uses the shared TauriUnit contract.
  • Memory enforcement: private real datasets and macOS vmmap remain explicit environment-gated acceptance tests, but the always-on suite now enforces bounded line/row reads, window/IPC/range limits, unchanged-poll no-op behavior, source conformance, and an isolated rendered fixture. Deterministic 30→300 MiB and real-data RSS evidence is recorded without pretending CI owns those private files.

Deliberate boundaries retained

  • Codex listing token/impact values are backfilled from the compact replay index rather than restoring a discovery-time full scan. A never-opened cold session may temporarily show zero; this is the bounded-discovery tradeoff, not a hidden full-loader fallback.
  • Legacy pre-upgrade fork handoff remains fail-closed when no bounded usable items exist.
  • CSV/Markdown presentation parity and O(total-turn-header) write amplification are not mixed into the fix(RAM): replace full hydration with bounded replay across all external CLI histories #443 correctness/security fixes; current compatibility output and bounded-memory behavior are pinned by tests.

The final tree has also passed the real 335.2 MiB Issue 272 open/reopen/lifecycle run, the 3.187 GiB SQLite migration/reopen run, 711 Vitest files / 6,426 tests, 482 orgtrack_core tests, 416 event-pipeline tests, and the documented native-SDE isolation checks.

@ShiboSheng

Copy link
Copy Markdown
Collaborator Author

@Harry19081 Replying to the scope/blast-radius analysis in this comment.

I agree with the framing: the user-visible bug is one invariant — normal external-history operations must never require complete transcript hydration — but enforcing it removes the old full-array contract from every producer and consumer at once.

Cloud/collaboration ingest, Fork, Raw Transcript, export, metadata/hover, managed CLI reopen, and EventStore application remain in this PR because leaving any one of them on the full-load API would preserve the same failure class. The implementation was nevertheless reorganized so those concerns are independently reviewable rather than welded into one command file:

  • core replay contract/index/storage adapters;
  • application command bridge and lifecycle;
  • collaboration staged ingest;
  • Cloud wire/spool/atomic publication;
  • frontend SessionCore transport;
  • secondary consumers;
  • focused conformance, security, performance, and rendered tests.

The post-review refactor keeps every new production file below 1,500 lines and does not change the public replay wire, provider IDs, parser compatibility, limits, or native SDE behavior.

@ShiboSheng

Copy link
Copy Markdown
Collaborator Author

@Harry19081 Replying to the structural pre-merge recommendations in this comment.

The high-confidence consolidations were implemented, including the ones that fixed live bug classes:

  1. Shared driver utilities: replay/drivers/common.rs is now the sanctioned home for pre-enforced bounded line reads, UTF-8 boundary/range handling, length-delimited content digests, and storage-neutral turn folding. JSONL, Codex, Qoder, SQLite, structured, and whole-JSON drivers consume those helpers. This removed the duplicated offset, unbounded-line, hash, and turn-folding behavior addressed by ec0e7e413.
  2. One Attachment V2 implementation: framing, limits, encoding/decoding validation, and golden coverage now live in the Rust replay_cloud_wire module. The duplicate test-only TypeScript codec was deleted; collaboration ingest tests use Rust-produced frames. This landed in 92b8db2cd.
  3. One foreground/prewarm publish discipline: identity validation happens before request state allocation, and both paths use the same bounded request registry plus manager → store → registry publication order. This landed in 92b8db2cd.
  4. Snapshot destination resolution: collaboration publication now reuses the authoritative session classification and enforces snapshot-backed/empty/imported eligibility inside the publication transaction. Existing native Agent history is protected by 25a35bdd9.
  5. Tauri unit response: collaboration abort and external replay unit commands use the shared TauriUnitSchema.

The recommended file split was also brought into this PR rather than deferred:

  • the former 10,638-line external_replay.rs is split into window, target, request guard, payload, export, Cloud, Shell, handoff, collaboration, managed-chunk, wire-budget, and focused test modules;
  • the former 3,636-line collaboration ingest file is split into staging, wire, schema, publish, and tests;
  • replay model/index/drivers/tests, orgtrack CLI show, exporter tests, Cloud tests, Raw Transcript tests, and the dedicated rendered external-replay spec were separated by responsibility;
  • source-neutral replay naming replaced the transitional Cursor-only/legacy terms.

No new #443 production file exceeds 1,500 lines. The current largest files are the Codex JSONL driver at 1,370 lines, Qoder sidecar at 1,316, and managed-chunk bridge at 1,301.

@ShiboSheng

Copy link
Copy Markdown
Collaborator Author

@Harry19081 Replying to the additional driver-boundary analysis in this comment.

I agreed with the diagnosis that ad-hoc reuse was the problem, but stopped short of forcing every storage family behind one universal runtime trait.

What changed:

  • append-log drivers now share the mechanical invariants that carried the duplicated bugs: bounded complete-line reads, torn-tail cursor behavior, UTF-8-safe ranges, stable digests, previews, and common turn folding;
  • SQLite and structured/whole-JSON drivers now consume the same digest/range/turn helpers instead of private copies;
  • storage dispatch is backed by one exhaustive replay_driver_kind classifier shared by sync and payload reads, with a registry parity test that fails when a built-in source has no explicit driver;
  • Cursor CLI and Warp have distinct cursor types, and row-store versus KV SQLite cursors are no longer presented as one misleading generic cursor;
  • provider-specific normalization, tool pairing, stable event IDs, pending Codex background/Shell state, WAL/KV traversal, and whole-document publication remain inside their adapters.

I did not introduce a trait/object dispatch layer or one callback-heavy sync_append_log state machine. The final boundary keeps compile-time exhaustive source coverage while avoiding an abstraction that would hide materially different cursor and transaction semantics. The identical safety primitives are shared; provider event semantics are not.

The resulting structure is under replay/drivers/{common,jsonl,sqlite,structured,whole_json} and is covered by the same conformance suite for append, truncate, replace, torn tail, WAL/checkpoint, VACUUM, payload range, and generation reset.

@ShiboSheng

Copy link
Copy Markdown
Collaborator Author

@Harry19081 Replying to the consolidation priority list in this comment.

The final implementation follows the proposed priority where the semantics were truly identical:

  • Replay/prewarm request state: unified in external_replay/request_guard.rs with one request-state vocabulary, validation order, ticket model, TTL/cap policy, and linearized publication (92b8db2cd).
  • Cloud spool and watcher registries: kept as separate types because leases/refcounts and byte-budgeted notify handles have different eviction contracts, but their orphan cleanup and bounded lifetime are now explicit; owner-private spool cleanup and expired watcher retirement landed in 65dacbcee.
  • Cursor terminology: generation, revision, source identity, replay target, episode, request token, row-store cursor, KV cursor, Cursor CLI cursor, and Warp cursor are now distinct names. A universal CursorVerdict trait was not added; the exhaustive driver classifier and typed cursors provide the fail-closed coverage without erasing storage-specific rebuild reasons.
  • Wire budgets: the 10-turn, 200-event, 4 MiB, and 256 KiB values now come from src/shared/externalReplayBudgets.json, with Rust/TypeScript parity assertions and RPC schema tests (92b8db2cd).
  • Shared driver mechanics: bounded line reading, UTF-8 payload ranges, SHA-256 digests, and turn folding live in replay/drivers/common.rs (ec0e7e413).
  • Attachment V2: one authoritative Rust wire implementation; the duplicate TS codec is gone (92b8db2cd).

The token/impact cold-catalog behavior remains intentionally index-backed: discovery does not revive a prefix/full transcript parser just to avoid temporary zero values before bounded indexing. That tradeoff is documented and keeps the primary memory invariant intact.

The architectural cleanup was completed in fc45732d3; follow-up hardening and final evidence are in 25a35bdd9 through 46250987a.

@ShiboSheng

Copy link
Copy Markdown
Collaborator Author

@Harry19081 Review feedback update for the analysis starting at the main review:

  • all four blocking findings are fixed;
  • the legacy >256 KiB Cloud migration path is implemented;
  • the global writer stall, CLI all-or-nothing indexing, Raw Transcript races, unsafe cursor/range/hash cases, request publication ordering, localization, spool/watcher cleanup, and torn Cloud epoch are addressed;
  • the high-confidence driver, Attachment V2, request-registry, budget, resolver, naming, and module-splitting recommendations are incorporated;
  • deliberately retained tradeoffs and non-fix(RAM): replace full hydration with bounded replay across all external CLI histories #443 follow-ups are called out in the individual responses rather than being reported as fixed.

Final local evidence on the current develop merge:

  • 711 Vitest files / 6,426 tests passed;
  • orgtrack_core: 482 passed / 4 intentionally ignored;
  • event pipeline: 416 passed;
  • agent_core: 3,088 passed, with both sandbox-only loopback cases rerun successfully outside the restriction;
  • pnpm typecheck, workspace cargo check, branch rustfmt --check, and git diff --check passed;
  • 5,849 files checked with 0 circular dependencies;
  • real 335.2 MiB Issue 272, 3.187 GiB SQLite migration/reopen, 30→300 MiB growth, and 327.1 MiB streamed export gates passed;
  • the rendered Issue 272 lifecycle and Command+5 manual pass showed bounded reopen, no idle polling, correct unsubscribe/release, and no A→B→A stale result.

The live dual-instance Cloud sharing/presence matrix remains explicitly BLOCKED, not passed: all 12 scenarios reach the credential gate but require unavailable E2E_CLOUD_* credentials.

The PR remains Draft for re-review.

@ShiboSheng

Copy link
Copy Markdown
Collaborator Author

End-to-end context, review history, and current status

This comment records the complete path from the original incident to the current Draft so a reviewer does not need to reconstruct it from the issue, commits, audit documents, and earlier review comments.

1. What originally went wrong

ORGII's external-history path treated a CLI session as one complete in-memory transcript.

For a Codex/Claude-style JSONL session, opening or refreshing the session could:

  1. read the growing JSONL again from the beginning;
  2. parse the full history into ActivityChunk[];
  3. copy the full result through Rust → JavaScript → Rust;
  4. replace/upsert it through EventStore and the projection worker;
  5. display only the most recent part.

The original 29 MiB reproduction became approximately 47.5 MiB before the later IPC, EventStore, snapshot, and worker copies. WebContent reached approximately 1.27 GiB Physical Footprint.

This was not only a five-second active-session polling problem. Inactive or completed external sessions could still trigger the same full hydration through metadata, hover, provenance, Fork, Cloud, Raw Transcript, or export.

Three earlier issues remained valid but did not fix this path:

#443 was the separate external-history parse/copy problem.

During investigation, a second compounding hotspot was confirmed: the SQLite turn-index rebuild collected all event rows and copied/parsing their large JSON fields before folding turns. A real session reached approximately 8.2 GiB Working Set / 8.6 GiB Private and did not finish its index migration.

2. Why the scope became larger than Codex

Codex supplied the reproducible JSONL failure, but a Codex-only byte cursor would leave the same full-hydration API available to every other source and consumer.

The registered sources use different physical storage:

  • append-oriented JSONL: Codex, Claude Code, WorkBuddy, Trae, Qoder, OMP, Qoder CLI;
  • SQLite/WAL row stores: OpenCode, Mimo Code, ZCode;
  • SQLite/KV: Cursor IDE, Windsurf;
  • manifest/blob: Cursor CLI;
  • SQLite task BLOB: Warp;
  • whole-document JSON: Cline.

JSONL byte offsets are not valid SQLite cursors, and neither model can be applied to a whole-document rewrite. The shared layer therefore had to be a bounded replay contract, while source identity, incremental reads, reset detection, tool pairing, stable event IDs, and payload location remained storage/provider-specific.

The issue comments were updated to make two boundaries explicit:

  1. all 15 built-in sources and managed CLI reopen paths must be covered;
  2. the streamed SQLite turn-index rebuild is required but does not replace the external replay adapters.

3. What the first implementation changed

The implementation replaced complete transcript hydration with:

  • a Rust-owned exhaustive 15-source registry;
  • storage-specific incremental/rebuild adapters;
  • a compact, rebuildable index containing generation/revision, turn/event headers, stable IDs, hashes, and payload locators;
  • bounded open, delta poll, older-window/turn, payload-range, handoff, and streamed-export operations;
  • a default recent window with hard caps of 10 turns, 200 events, and 4 MiB serialized IPC;
  • 8 KiB ordinary previews, 32 KiB Shell previews, and 256 KiB payload ranges;
  • true no-op polling for unchanged sources;
  • EventStore application of bounded windows/deltas rather than a full replace;
  • active/visible watcher ownership, single-flight updates, byte-bounded cache policy, explicit release, and A→B→A late-result guards.

Large content keeps one canonical representation:

The full-history consumers were migrated together: normal open/refresh, metadata/hover/provenance, Fork, Cloud, Raw Transcript, JSON/Markdown/CSV export, managed CLI reopen/reconcile, and collaboration snapshot ingest.

Native SDE Agent remains on its existing persisted-database/EventStore/FSM path. Tests assert that a native session makes zero external replay RPC calls.

The SQLite hotspot was changed independently to stream rusqlite::Rows, classify metadata projection requirements before copying/parsing large JSON, preserve conservative behavior for unknown tools, and atomically publish rebuilt summaries.

4. What the review found

The first deep review correctly described this as a cross-layer re-architecture and found four blocking-caliber issues:

  1. renderer-controlled export destinations could overwrite an arbitrary user-writable path;
  2. collaboration snapshot ingest could target and erase an existing native session;
  3. Cursor IDE/Windsurf KV replay used inconsistent ordering/numbering schemes;
  4. the default rendered E2E required the author's private Issue 272 session and failed without it.

It also required an explicit migration answer for legacy Cloud rows over 256 KiB and identified major operational problems:

  • global sessions-writer lock held across streamed export;
  • CLI content indexing aborted all sessions after one per-session failure;
  • torn/non-atomic Cloud epoch publication.

The review additionally found bounded-line, wildcard, Qoder overflow, UTF-8 range, hashing, Raw Transcript race/pagination, request-registration ordering, localization, spool/watcher cleanup, and fallback-observability problems.

The subsequent architecture comments recommended:

  • a sanctioned shared driver-utility layer;
  • one Attachment V2 wire implementation;
  • one foreground/prewarm validate-and-publish path;
  • reuse of the authoritative session resolver for snapshot ingest;
  • shared replay budgets and clearer cursor/request terminology;
  • splitting the 10,638-line application bridge and other large modules;
  • avoiding duplicate helpers while not forcing genuinely different storage engines into one false abstraction.

5. How the review feedback was addressed

Commit Main result
25a35bdd9 Trusted export destination selection, symlink/target protection, atomic replacement, transactional snapshot target classification, and native-history refusal
0afe47820 One canonical Cursor IDE/Windsurf KV order shared by events, turns, ranges, counts, and fallback bubbles; parser-version rebuild and dirty-store fixture
ec0e7e413 Pre-enforced bounded JSONL reads, shared UTF-8 ranges, length-delimited SHA-256, escaped suffix matching, checked Qoder sequencing, and common turn folding
8417d354e Release global writer before export I/O, tolerate per-session CLI indexing failures, serialize Raw Transcript requests, bounded empty-page scanning, and retry state
92b8db2cd Validate identity before ticket allocation, one bounded request registry/publication order, one Rust Attachment V2 implementation, and shared Rust/TypeScript replay budgets
98f454e89 Streaming migration for bounded legacy Cloud rows, object-backed bounded batches, and one atomic complete-epoch publication
65dacbcee Default isolated rendered Codex fixture, explicit real-session opt-in, 13-locale Raw Transcript text, owner-private spool cleanup, watcher retirement, and logged bounded fallback
fc45732d3 Split oversized modules, move tests by responsibility, remove dead Cursor-only aliases, and normalize replay/source/target/generation/revision/episode/request-token naming
46250987a Final formatting, audits, real-data evidence, and manual Command+5 verification

The high-confidence common code now lives in replay/drivers/common.rs: bounded complete-line reading, UTF-8-safe ranges, stable content digests, and storage-neutral turn folding. Attachment V2 has one Rust source of truth. Replay/prewarm request state uses one registry and publication discipline. Budgets come from one shared manifest with parity tests.

We deliberately did not force all storage families behind one universal runtime trait or callback-heavy sync template. Mechanical safety invariants are shared; provider normalization, Codex pending/Shell state, SQLite/WAL/KV traversal, Cursor/Warp lineage, and Cline whole-document publication remain explicit adapters.

The original 10,638-line application bridge and 3,636-line collaboration ingest file were split by responsibility. No new #443 production file exceeds 1,500 lines; the largest is 1,370 lines.

6. How the complete result was verified

Automated/current-tree evidence:

  • session_persistence turn index: 13 passed, 1 intentionally ignored;
  • orgtrack_core: 482 passed, 4 intentionally ignored;
  • ORG2 event pipeline: 416 passed;
  • agent_core: 3,088 passed, with both sandbox-only loopback cases rerun successfully outside the restriction;
  • perf_utils: 68 passed;
  • orgtrack_cli: 42 passed;
  • full Vitest: 711 files / 6,426 tests passed;
  • TypeScript typecheck, workspace Cargo check, branch rustfmt, and diff check passed;
  • 5,849 files checked with zero circular dependencies;
  • post-push Frontend CI and Rust Clippy CI both passed.

Real-data/performance evidence:

  • Issue 272 Codex JSONL: 351,503,887 bytes / 335.2 MiB, 52,477 source rows, 19,404 indexed events;
  • cold core open returned a 200-event/10-turn window in 19.637 s with 39.4 MiB peak RSS growth;
  • unchanged poll parsed zero bytes/rows and emitted zero work; reopen completed in 55.64 ms;
  • ten rendered open/release cycles returned 26 events each with zero measured-tail, backend-step, and settled-idle staircase growth;
  • native memory 609.9 MiB versus vmmap 566.7 MiB, within the fix(RAM): replace summed RSS with platform-native app memory metrics #435 tolerance;
  • 3.187 GiB real SQLite clone migrated 5,227 events / 199 turns from v11 to v12 in 595.138 ms and reopened in 44.888 ms with 93.0 MiB peak RSS growth;
  • extending the deterministic source from 30 MiB to 300 MiB added 52.5 MiB rather than growing with total history;
  • 327.1 MiB streamed export reproduced the expected SHA-256 with 16.1 MiB peak RSS growth;
  • Computer Use opened the real Issue 272 session from the visible sidebar and used Command+5: one bounded merge per reopen, zero calls during 12 idle seconds, normal unsubscribe on release, and no A→B→A stale result.

The original real databases and external CLI files were never committed and were tested through isolated/read-only copies or roots.

7. Current state

  • Base: latest develop at 2c8d18a10.
  • Head: f9d20143e.
  • PR is mergeable and GitHub reports a clean merge state.
  • Frontend CI and Rust Clippy CI are green.
  • The original PR title/description were restored to preserve review continuity.
  • Detailed feedback responses and current evidence are recorded in the comments.
  • The PR remains Draft for re-review.

One external acceptance item is still explicitly BLOCKED, not passed: all 12 dual-instance Cloud sharing/presence scenarios reach their live credential gate but require unavailable E2E_CLOUD_* credentials.

The remaining reviewer decision is therefore not whether the known blocking code findings were ignored—they were fixed and tested—but whether the final architecture, deliberate adapter boundaries, and documented external Cloud credential blocker are acceptable for this PR.

Refs #443

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants