fix(trajectory): export legacy v1 sessions without entry timestamps#93814
Conversation
|
Codex review: needs maintainer review before merge. Reviewed June 21, 2026, 5:40 PM ET / 21:40 UTC. Summary PR surface: Source -2, Tests +35. Total +33 across 2 files. Reproducibility: yes. Source inspection shows current main and v2026.6.9 synthesize ids for version < 2 entries and then filter out timestamp-less canonical entries before timestamp normalization can run; the PR body also supplies before/after exporter output for the same shape. Review metrics: 1 noteworthy metric.
Stored data model Root-cause cluster Members:
Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything. Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Risk before merge
Maintainer options:
Next step before merge
Security Review detailsBest possible solution: Land the narrow exporter fix and regression test after maintainers accept that legacy v1 bundles should include redacted transcript events. Do we have a high-confidence way to reproduce the issue? Yes. Source inspection shows current main and v2026.6.9 synthesize ids for version < 2 entries and then filter out timestamp-less canonical entries before timestamp normalization can run; the PR body also supplies before/after exporter output for the same shape. Is this the best way to solve the issue? Yes. Removing the timestamp predicate in readSessionBranch is the narrowest maintainable fix because branch selection needs canonical entry identity, while buildTranscriptEvents already owns timestamp normalization. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 108d6d7eca00. Label changesLabel justifications:
Evidence reviewedPR surface: Source -2, Tests +35. Total +33 across 2 files. View PR surface stats
What I checked:
Likely related people:
What the crustacean ranks mean
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics. How this review workflow works
|
|
/clownfish automerge |
|
Clownfish is on the reef for this PR. 🐠 I tagged A maintainer can call |
|
/clownfish automerge |
|
Clownfish is on the reef for this PR. 🐠 I tagged A maintainer can call |
|
🦞✅ Source: Why human review is needed: What the maintainer can do as a next step: I added |
|
@clawsweeper re-run |
|
🦞🧹 I asked ClawSweeper to review this item again. |
readSessionBranch filtered out every entry lacking a string or number timestamp. Sessions written before entry timestamps existed (version 1) have ids and parentIds synthesized by the legacy migration but no entry timestamp, so all entries were dropped and the exported bundle reported transcriptEventCount 0. The transcript event builder already defaults a missing timestamp via normalizeTimestamp, so the filter clause was both wrong and redundant. Drop it; entry identity plus the canonical-entry check is what the branch walk needs.
050fe83 to
5d619f9
Compare
Summary
Exporting a trajectory bundle for an old (version 1) session produces an empty transcript. The CLI reports
Trajectory exported!and writes a bundle whosemanifest.transcriptEventCountis0, silently dropping every message in the session. This hits real users:openclaw export-trajectory//export-trajectoryrun against any session file written before entry timestamps existed.Root cause
readSessionBranchfilters session entries down to canonical transcript entries, but the filter also requires a presenttimestamp:The legacy migration
migrateLegacySessionEntriessynthesizes anidandparentIdfor every entry of aversion < 2session, but it never synthesizes atimestamp(those logs predate entry timestamps). The downstream event builder already tolerates a missing timestamp:buildTranscriptEventscallsnormalizeTimestamp(entry.timestamp), which defaults an absent value to the epoch. That tolerance is dead code, because the filter strips the timestamp-less entries before they ever reach it. Result: every entry of a timestamp-less v1 session is filtered out and the bundle is empty.Fix
Drop the timestamp clause from the filter. Entry identity (
id) plusisCanonicalSessionTranscriptEntryis what the branch walk needs; the timestamp is normalized later.normalizeTimestamp(already in the event path) defaults a missing or non-finite timestamp tonew Date(0).toISOString(), so v3 sessions with real timestamps are unaffected and v1 sessions now export with epoch-defaulted timestamps instead of vanishing.Why this is the right boundary
readSessionBranchis the single owner of which persisted entries enter the exported branch. The timestamp requirement contradicted the migration contract (v1 entries legitimately have no entry timestamp) and the normalization contract (the builder already defaults missing timestamps). Removing the clause makes those two contracts reachable instead of adding a second compensating path. No sibling surface re-derives this filter; runtime event ingestion is separate and uses its ownisRuntimeTrajectoryEventguard, which is unchanged.Verification
node scripts/run-vitest.mjs src/trajectory/export.test.ts- full 24-test export suite green with the fix (23 existing + 1 new regression).exports the transcript for a legacy v1 session without entry timestampsfails on pristinemain(transcriptEventCountis0) and passes with the patch.node scripts/run-oxlint.mjs src/trajectory/export.ts src/trajectory/export.test.tsclean.oxfmt --checkclean on both files.tsgo -p tsconfig.core.jsonand-p test/tsconfig/tsconfig.core.test.jsonclean.Real behavior proof
Behavior addressed: trajectory export of a legacy v1 session (no entry timestamps) drops the entire transcript and reports
transcriptEventCount: 0.Real environment tested: the real
exportTrajectoryBundleruntime driven end to end over a v1 session JSONL (headerversion: 1, two timestamp-less message entries), on pristinemainand on the patched tree with identical input.Exact steps or command run after this patch:
node scripts/run-vitest.mjs src/trajectory/export.test.ts -t "legacy v1 session".Evidence after fix:
Observed result after fix: the v1 session exports both messages; the bundle is no longer empty.
What was not tested: did not run the full
pnpm check/pnpm build(no dynamic-import or packaging surface touched); proof is the realexportTrajectoryBundleruntime over a synthesized v1 session file rather than a CLI invocation against an on-disk historical session.