Repository navigation
fix: adapt Codex sessions to current DSH format and preserve tool arguments - #3
Conversation
…open Imported Codex sessions were emitted as turn/start -> user/message with no step region, but DSH's format v0->v1->v2->v3 migration only acquires its "system head" at the first step/start. Any surface event before that was rejected with "format v2 surface before first step cannot acquire a system head without changing chronology", so sessions imported before DSH 0.9.0 failed to open (issue Walvez#2). buildDshEvents now opens a step/start before the first surface event of each turn (user/message when the turn starts with a user message, and for the coordinate-less surface events in general), matching the native agent-loop ordering. The first assistant reuses that leading step; subsequent assistants advance to their own step, preserving the existing step-pairing invariants the token meter enforces. Validated by running the fixed converter output through the real @deepseek-ai/dsh-session-format v0->v1->v2->v3 migration chain (passes) and added regression tests (test/convert-order.test.mjs) asserting every surface event is preceded by a step/start. Refs Walvez#2
The previous step-ordering fix addressed the read-side migration failure,
but DSH 0.9.0's session-persistence backend now WRITES only the current v3
format: encodeCurrentHeader throws
"encodeCurrent requires Session format v3"
for a v0 header, so the importer's `persistence.create({ version: 0, ... })`
aborts and `/import-codex` fails with that error.
The importer now migrates the imported v0 header + events through the real
@deepseek-ai/dsh-session-format v0->v1->v2->v3 chain (the same one the harness
runs on read) before create/append, so the stored artifact is genuine v3:
- new lib/session-migrate.mjs: migrateToCurrent(header, events) resolves the
migration packages from the DSH app install (or DSH_CHECKOUT / profile
node_modules), chains migrateHeader for the header and runs each stage over
the events, producing a v3 header (isSeeded=false) and v3 events (promoted
system/message head + embedded assistant/message stream that v3 restore
requires).
- lib/import-service.js: builds the header, migrates it + events to v3, then
create(currentHeader) / append(id, currentEvents). Falls back to v0 when the
migration runtime is unavailable (e.g. bare unit tests).
- test/import-service.test.mjs: regression asserting an imported session is
written as v3 (header version 3, isSeeded=false, assistant stream, and a
promoted system/message) when the migration resolves.
Validated: full v0->v1->v2->v3 migration chain passes on the fixed converter
output; the migrated v3 output carries the required stream/system fields; the
full 45-test suite is green.
Refs Walvez#2
The v0 logical header requires `delegationDepth` (HEADER_REQUIRED in dsh-session-format-v0-to-v1), but the importer's minimal header omits it, so the v0->v1 migrateHeader throws 'format v1 header lacks required member delegationDepth' and migrateToCurrent catches it and falls back to v0, re-triggering 'encodeCurrent requires Session format v3' (issue Walvez#2). Normalize the source header (default delegationDepth 0) for both the chained migrateHeader and the per-stage createStage source header.
DSH 0.9.0+ session-persistence writes only v3 and migrates pre-v3
artifacts on read via the v0->v1->v2->v3 chain, which acquires its
'system head' at the FIRST step/start. Any surface event (user/message /
assistant/message / tool/result) before it makes the migration throw
'format v2 surface before first step cannot acquire a system head ...',
leaving imported sessions unreadable (symptom: 'encodeCurrent requires
Session format v3', 'session already exists', 'V2' display, and
'Failed to load history ... surface before first step').
Old-imported v0 logs emitted turn/start -> user/message with no leading
step region, exactly this case.
- repairEvents: STEP_TYPES now includes user/message and opens a
step/start before any orphan surface, so the v0->v3 migration invariant
holds (track currentTurn from turn/start; user/message carries no
data.turn/step).
- loadEvents: dsh-session no longer exports decodeStorageRecord in DSH
0.9.1; decode logical rows directly (plugin-written artifacts are
logical {type,seq,time,data,surfaceOp}).
- migrateArtifactToCurrent (new): repair + migrate v0->v3 and rewrite
the artifact in current format, validating every row encodes via the
real session-format catalog before writing (keeps .bak).
- repairSessionStore: auto-migrate pre-v3 artifacts, and default root
to /Users/kenleo/Library/Application Support/dsh-desktop/harness/sessions (was ~/.dsh/sessions which never exists for
DSH Desktop).
The token meter (isClean/measure) cannot read token-usage for imported
content and throws on cold replay ('Cannot read properties of undefined
(reading get)' / invalid settlement fields), so it gave a false 'bad'
signal for already-v3 sessions. Running --fix would then try to rewrite
them based on that false signal and risk breaking sessions that open
fine.
- measure(): Session.create now uses SESSION_FORMAT_VERSION and
isSeeded/delegationDepth (DSH 0.9.1 rejects version:0 headers).
- repairSessionStore(): already-v3 artifacts are counted as ok and left
untouched; meter-based step-pairing repair only runs when
options.meterRepair is explicitly set (--fix now purely does the
needed v0->v3 migration).
DSH 0.9.0 refactored ctx.sessionPersistence to a handle-based API:
create(header,{inheritedEventCount}) returns a write handle, open(id,'read'
|'write') returns a handle, list() returns {header,revision,sizeBytes}
snapshots, and there is no append(id,...)/inspect(id) on the service object.
- persistence-facade.mjs (new): normalizePersistence detects the legacy vs
0.9.0 shape and returns a facade exposing the plugin's familiar calls
(list/inspect/append/createAndAppend) mapped onto the handle API; legacy
services are proxied with createAndAppend added.
- import-service.js / attach-workspaces.mjs / export-codex.js: normalize the
persistence object at each entry point and write new sessions via
createAndAppend (create+append through one handle).
|
Thanks for working on both the step ordering and v3 write issues. The ordering regression tests cover the key converter cases, but I think two items need to be addressed before merging:
Once these are addressed and the checks run on the updated head, I can review it again. I am leaving the PR unmerged for now. |
…0.0 v4)
- session-migrate discovers the dsh-session-format-vN-to-v{N+1} chain
dynamically and migrates to the highest stage the resolved base ships
(v3 on 0.9.x, v4 on 0.10.x); V3-to-V4 uses the
createSessionFormatV3ToV4([]) child-evidence factory (imported sessions
are roots). The running DSH app wins the base tie-break so package
versions never mix; the plugin's own node_modules is the dev/test
fallback (real packages now devDependencies @ 0.1.7-rc.2).
- import-codex no longer falls back to a v0 write: migration failures
fail loudly with the session id and root cause (review item 1).
- buildDshEvents: user-kind titles must carry EMPTY messageSeqs (v4
assertReleasedV4Relationships); fallback titles cite the first user
message. Caught by the new reopen validation (review item 2).
- tests: stub persistence rejects pre-v3 writes like real DSH; the
current-format write path is REQUIRED (no early return); new write-gate
test encodes the header+events through the real catalog and restores
the artifact via restoreReleasedV4Artifact. Full suite 48/48.
- repair-sessions now scans session.v<current>.jsonl.zstd (DSH 0.10.x naming) alongside the legacy session.jsonl.zstd, prefers the versioned log when both exist, writes migrated artifacts under the versioned name (an old-named rewrite stayed invisible to the new persistence), and reports stale/migratable logs in dry runs instead of counting them nowhere. dshPackage resolution gains the macOS app bundle base. - import-codex merges paginated continuation rollout segments (rollout-<ts>-<uuid>_<fork>.jsonl, same session id, delta-only) into ONE candidate per id: the second segment used to call create() again and abort the whole batch with 'session … already exists'. - tests: segment merge + listSessionFiles preference; 50/50.
…er-title citations - migrateToCurrent now starts the chain at header.version (v3 store artifacts go straight to v4) instead of force-resetting to v0, which failed stored-log repair with 'format v0 contains unknown historical event type system/message'. - repair pre-step clears messageSeqs on user-kind session/title events (legal under v3, rejected by the v4 relationship rule) so migrated artifacts restore cleanly. - 51/51.
…repair
repairEvents used to force inserted step/start markers to step 1; on logs
that already carry their own step numbering the duplicate step failed the
v4 lifecycle rule ('step/start does not match the open turn and next
step'). Track the next expected step per turn (steps restart at 1 on
turn/start per the v4 relationship rule) and continue from there.
51/51.
…nestly
- repairEvents now treats stored markers as REGION STRUCTURE only and
renormalizes numbers: duplicate step/start opens become start…end…start,
surface events re-attach to their open region, orphan surfaces after the
last turn/end (bare-appended paginated segments) open a fresh turn, and
an unclosed turn is closed before the next one opens. Fixes the v4
lifecycle failures ('step/start does not match the open turn and next
step') on logs written by the old segment-appending importer.
- migrateArtifactToCurrent skips logs containing compaction events with an
explicit reason: their shadowedRange citations name exact surface spans
that normalization would invalidate; they need compaction-aware
restructuring and are left untouched.
- verified on the real store: 146/146 session.v4.jsonl.zstd pass the real
catalog createRestore(validation:'current') decode path. 51/51 tests.
|
Both review items are addressed, plus a batch of hardening we found while validating against a real DSH 0.10.0 install (current format is now v4). Review item 1 — no more silent v0 fallback: Review item 2 — required test path with the real packages: the four Additional hardening from real-machine validation on DSH 0.10.0:
Real-store validation: a desktop install upgraded to 0.10.0 with ~105 pre-v4 imported sessions — after the repair every Ready for re-review — happy to adjust if you'd rather split the store-repair parts into a separate PR. |
a723ab2 to
8a8c8e5
Compare
|
Thanks for the update. I re-reviewed head I reproduced the following remaining issues before merging:
These are reproducible gaps beyond the existing 51 passing cases, so I recommend keeping the PR unmerged until they are addressed. |
…lidation, error propagation - diffImportEvents: a same-turn assistant continuation (or any slice starting mid-structure) is now repacked into ONE fresh self-contained turn (turn = oldTurn+1, steps re-derived, seqs continuing the old log) — the old wrap produced turn/start(2) followed by step/end(1) and step-2 events still referring to turn 1, failing the real restorer on old.events + suffix. - repairEvents: the synthetic turn/end for an implied boundary is emitted AFTER closing the open step (v4: turn/end requires no open step). - migrateArtifactToCurrent: before touching the store the migrated artifact is now replayed through the catalog's own reader with validation:'current' (full restoration + relationship check) — encoding alone passed artifacts the real restorer rejects. - persistence-facade: a failed inspect() propagates instead of returning empty history (the importer skips the session rather than appending the whole source); createAndAppend/append surface flush/close failures after cleanup. - tests: same-turn and new-turn combined-artifact restore assertions, failed read skips, failed flush surfaces. 56/56.
|
All four points are addressed in 1. Same-turn assistant continuation — 2. Repair writing unrestorable artifacts — the synthetic 3. Read errors as empty history — 4. Swallowed durability failures — Happy to iterate if anything else comes up. |
|
Re-reviewed
There is one remaining continuation edge case in First import while a reply is still pending: import a rollout containing only The equal-user-count branch initializes This is a content-loss case in the existing continuation logic, so I recommend resolving it before merging. Apart from this remaining case, the previous four review findings are closed. |
diffImportEvents' equal-user-count branch incremented its assistant counter before comparing it with oldAssistants, so whenever old's last turn had no assistant yet the cut position never matched and the whole continuation was dropped (reported 'updated 0, skipped 1' with the answer silently lost). The same defect also dropped the reply when an earlier complete turn existed, and the new-user branch dropped a pending turn's reply when a further user followed. Rollouts are append-only, so old's users and assistants are prefixes of next's: the first new content is now the earlier of the (oldAssistants)-th assistant (0-based) and the (match)-th user. A slice that continues old's last turn is repacked into one fresh turn (wrapAsFreshTurn); complete new turns keep their converted structure, renumbered after it. Regression tests cover pending-question, pending+new-user, and multi-turn-pending shapes, each asserting the stored answer, combined-artifact restoration through the real restoreReleasedV4Artifact, and idempotence on the next unchanged import.
|
Fixed in Root cause — Fix — rollouts are append-only, so Regression tests (all through
Ready for re-review. |
|
Re-review of
I have no remaining blocking findings in the code paths reviewed. From the code review and automated testing perspective, this is suitable for merge. GitHub's latest CI run is still One documentation follow-up: please update the PR description to match the final implementation (dynamic current-format/v4 support, no v0 fallback, 58 tests, and the expanded repair/API-adapter scope). The current description still describes the earlier v3/fallback/45-test implementation. Thanks for addressing the reproducible cases. |
|
Thanks for the re-review. The PR description is now rewritten to match the final implementation (dynamic current-format discovery from the stored version, no v0 fallback, handle-based API adaptation, repair scope, 58 tests) — the old text described the earlier v3-era state. On the two open checklist items: the CI |
|
Small pointer on the CI gate: the |
|
Thanks for clarifying the maintainer-only CI approval and your DSH 0.10.0 desktop validation. We now have a separate local acceptance result on the maintainer's currently installed runtime, which changes deployment readiness for that environment:
This does not contradict your 0.10.0 validation: it identifies a legacy-v0 compatibility regression on the runtime the maintainer actually uses. My earlier code-review clearance applies to the migration-equipped/current-format scenarios tested, not to this legacy runtime. Please clarify the minimum supported DSH version. If legacy v0 remains supported, select the v0 write path based on positively identifying the host's supported format; do NOT reintroduce the blanket catch-and-v0 fallback on migration errors for newer hosts. If the PR intentionally drops that runtime, please document/enforce the minimum version with a clear actionable error and an upgrade path rather than failing at package resolution. We can then choose whether to upgrade the maintainer's runtime or preserve backward compatibility before deployment/merge acceptance. For now, local acceptance is not complete, so please keep the PR unmerged while this compatibility decision is resolved. |
Imported Codex sessions can fail to open when their event ordering or stored format differs from the installed DSH runtime. This change writes and validates the runtime’s current format, preserves incremental replies, and retains standard function-call arguments.
Fixes #2.
Changes
.bakbackup. Local repair acceptance used temporary copies only.function_call.arguments, including empty strings and object inputs, while retaining legacy input fallback and custom-tool input behavior. Add regression tests.@deepseek-ai/[email protected]peer version. Document official npm/CLI@deepseek-ai/[email protected], session format v4, installation/activation requirements, and text-only reverse-export scope. Desktop application versions are independent.zstdCLI; newer Node uses the native API. Isolate the MCP test fixture and always dispose its timer.Validation
Validated head:
d17b494f537ba9a12cbed20281900b2b7a175c0d.0.2.0-rc.2without a version exemption.skip-source/skip-workspaceresults reflected sample eligibility rules.Reverse plugin export preserves user/assistant text, not tool traces or the original thread id; native DSH archive export retains the stored event log. Real Codex Desktop list indexing and full MCP-mirroring acceptance were not tested. No npm publication or production-plugin activation is part of this PR merge.