fix(scripts): drop session-bundle imports deleted with the JSONL session tree - #2029
Closed
Astro-Han wants to merge 1 commit into
Closed
fix(scripts): drop session-bundle imports deleted with the JSONL session tree#2029Astro-Han wants to merge 1 commit into
Astro-Han wants to merge 1 commit into
Conversation
…ion tree #1994 made SQLite the sole operational authority and removed SESSION_BUNDLE_PORTABLE_SESSION_DIRECTORIES and SESSION_BUNDLE_PORTABLE_SESSION_FILES along with the JSONL session tree they classified, but scripts/measure-session-bundle.mjs still imported them. The module failed to load, so the whole test file errored out. Drop the two imports and the classification branch they fed. With 'sessions' no longer in SESSION_BUNDLE_STATE_ENTRIES, that branch was already unreachable — the top-level portability check rejects the entry first. This restores module loading and the extended script suite. It does not revive the harness's runtime path: findSessionId still requires sessions/<id>/session.jsonl, which no export produces since #1994.
Contributor
Author
|
Superseded by #1920, which landed the same fix ( |
Astro-Han
pushed a commit
that referenced
this pull request
Aug 6, 2026
) (#2263) * fix(storage): import legacy JSONL session transcripts into SQLite (#2260) After the JSONL->SQLite cutover (#1994, #2029), sessions created before the switch stayed on disk as sessions/<id>/session.jsonl but never appeared in the UI: the new storage layer only reads SQLite and there was no migration path (issue #2260). Add a one-time importer (importLegacySessionsOnce) that scans the legacy sessions directory, decodes each schemaVersion:1 transcript with the pre-#1994 compatibility rules (backend remapping, missing-field defaults), creates the session under its original id via the idempotent createStableSession path, appends the decoded messages, and restores the original lifecycle timestamps and flags. Design: - Idempotency key is the session id itself (probeStableSessionCreate), so re-runs and concurrent first launches converge without duplicates. - Per-file atomicity: a transcript imports fully or is skipped and reported; corrupt records are never laundered into the authoritative store. Failures never block startup or other files. - Legacy files are retained as migration evidence. - Wired into createSessionStore: list/listCatalogPage/listHeaders await the lazy import so upgraded installs see their pre-cutover sessions. * fix(storage): drop console diagnostic from legacy import wiring The import result logging used console.error, which the repository check-console audit rejects for new call sites. Remove the log entirely and harden the lazy import to swallow unexpected errors (best-effort semantics): a legacy-import failure must never block session listing. * fix(storage): address #2260 review — validate before write, probe first, marker/subagent fail-closed, resume gate, observable results Addresses Astro-Han's review (P1 + P2s): - **P1 per-file atomicity**: the decoded header AND the post-create header patch are now validated through normalizeSessionHeader BEFORE any store write. Previously updateHeader (the third of three transactions) could throw after create+append committed, leaving a permanent partial session that later probes would report as skipped forever. - **P2 marker laundering**: a session_transcript marker file with no backing SQLite row (restored backup, copied sessions/, reset DB) now fails closed instead of being fabricated into a fake session — matching the pre-#1994 reader's contract. - **P2 legacy field loss**: decodeLegacySessionHeader preserves subagentParent/Runtime/Spawn/Workspace, thinkingLevel, lastReadMessageId. Legacy subagent children route through createSubagent (parent lineage kept); an incomplete spawn identity fails the file instead of flattening the child into a top-level session. - **P2 resume gate**: readHeaderSnapshot/readMessagesSnapshot now await the lazy import, so 'maka --resume <legacy-id>' no longer misses pre-cutover sessions on the first post-upgrade run. - **P2 observability**: ensureLegacyImported retains the result and logs failures/imported counts instead of swallowing them; LegacySessionImportResult now splits skipped into existing vs collision. - **P2 steady-state cost**: the idempotency probe now runs BEFORE the file read, so every launch skips known ids without touching their transcripts. - **P2 torn tail**: an incomplete final line (interrupted append) is skipped like the pre-#1994 strict reader, instead of failing the whole file. Tests: 11/11 in legacy-session-import (added lazy-list, resume, torn-tail, marker, subagent fail-closed, field-preservation cases); session-store + sqlite-session-metadata-store + foreign-session-store 70/70; full storage suite 702 pass, 2 pre-existing env failures (dugite git binary + root tsconfig load) verified unrelated via stash. * chore: allow-list session-store.ts for legacy import diagnostics check-console.mjs flagged the new console.error/warn/info sites in session-store.ts (legacy JSONL import outcome diagnostics) as unlisted. Same pattern as the existing automation-store.ts allow-list entry — best-effort import diagnostics, no credentials or provider payloads. * chore: retry CI — test_headless 'settles background child sessions at the task-run deadline' flaked on the previous run (identical headless code passed two runs ago; local 30/30 green; no headless files touched by this PR) * fix(storage): refactor legacy session import onto a single-transaction store API Addresses #2263 review round 3: collapse the importer's probe -> create -> append -> update choreography (three transactions, constant fingerprint, fidelity patch, in-memory latch, resume gate) into one store-level importSession primitive. - sqlite-session-metadata-store: importSession(header, messages, projection) writes the header row (with historical timestamps/flags) and all messages in one transaction. Idempotent by primary key (INSERT OR IGNORE), so concurrent first launches converge on one winner with no create claims; tombstoned ids are never resurrected; a failure mid-transaction rolls back, so a partial session can never persist (closes the crash-window P1). - legacy-session-import: read -> decode -> normalizeSessionHeader (pre-write validation) -> one importSession call. Subagent children now import under their own legacy id with lineage preserved instead of a fresh UUID (fixes the phantom-session P1). Torn-tail tolerance tightened to the pre-#1994 strict-reader semantics: only a final line of a file with no trailing newline whose parse failure is an unclosed bracket is skipped; truncated lines ending in a newline and garbage tails fail the file. - session-store: memoized import latch moves into ensureReady(), which every public method already awaits, so desktop/CLI/headless/--resume are all covered with zero per-caller wiring; appendMessages and closeAfterReady now await ensureReady() (pre-existing gaps). Import diagnostics are kept observable through the existing console allow-list. - tests: payload pins (header model/status, deepEqual messages[0]), concurrent double-import, id collision, header-only, absent sessions/, empty file, garbage tail, truncated-with-newline, whole-run failure containment, and subagent legacy-id round-trip; 19/19 legacy import tests, full storage suite 713 pass (1 pre-existing dugite-binary env failure). * fix(storage): probe legacy session ids before reading transcripts Restores the probe-before-read steady-state cost from review round 2 on the single-transaction design: the importer now asks the store whether a session id already exists (live or tombstoned) before opening or parsing its file, so every launch of an upgraded install pays a directory listing plus per-id SQLite existence checks. importSession remains the idempotency authority — a race between the probe and the write still converges on one winner via the primary key. Adds hasSession to the store surface and a test that corrupts the on-disk transcript between runs to pin that a skipped id is never re-read. --------- Co-authored-by: cat0825 <cat0825@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
scripts/measure-session-bundle.mjsfails to load onmain, taking the whole extended script suite down with it:#1994 made SQLite the sole operational authority and removed the JSONL session tree along with the two constants that classified it —
SESSION_BUNDLE_PORTABLE_SESSION_DIRECTORIESandSESSION_BUNDLE_PORTABLE_SESSION_FILES— but the script kept importing them.Drop the two imports and the
topLevel === 'sessions'branch they fed. That branch was already unreachable:sessionsis no longer inSESSION_BUNDLE_STATE_ENTRIES, so the top-level portability check rejects such an entry before it can be reached. Net effect is 26 deleted lines and one rewritten import.Known remaining gap
This restores module loading, not the harness's runtime correctness.
findSessionIdstill requiressessions/<id>/session.jsonl, which no export has produced since #1994, so an actual measurement run would fail on its own. Deciding whether to port the harness to the operational database or retire it needs a storage-bundle judgement call, so it is tracked separately rather than guessed at here.Verification
node --test scripts/measure-session-bundle.test.mjs— 2 passed, 0 failed (fails to even load onmain).npm run test:scripts:extended— 15 passed, 0 failed.npm run lint,npm run format:check— clean.Why main is green with this broken
main's CI never reaches the script. In the latestmainrun,test_workspacessucceeds with the step itself skipped:ci.ymlselects surfaces from a two-dot diff betweenpull_request.base.shaandpull_request.head.sha. A push tomaindiffs only that merge, soscriptModestaysnoneand the extended step never runs — regardless of the script being broken.It surfaced on #2028 only because that branch was five commits behind
main, so the two-dot diff also carried the inverse of those commits, including the rootpackage.jsonchange froma7d17e5bd.package.jsonis inFULL_SUITE_FILES(scripts/ci-test-plan.mjs:11), which forcesscriptMode: 'full'and runs the extended scripts. Rebasing #2028 onto currentmainturns it green again without touching this bug.Confirmed independently by running the test on a clean detached
origin/maincheckout, where it fails identically. So the breakage is real onmainand merely invisible tomain's own CI path.