Skip to content

fix(trajectory): export legacy v1 sessions without entry timestamps#93814

Merged
obviyus merged 1 commit into
openclaw:mainfrom
yetval:fix/trajectory-export-legacy-v1-timestamp
Jun 25, 2026
Merged

fix(trajectory): export legacy v1 sessions without entry timestamps#93814
obviyus merged 1 commit into
openclaw:mainfrom
yetval:fix/trajectory-export-legacy-v1-timestamp

Conversation

@yetval

@yetval yetval commented Jun 16, 2026

Copy link
Copy Markdown
Contributor

Summary

Exporting a trajectory bundle for an old (version 1) session produces an empty transcript. The CLI reports Trajectory exported! and writes a bundle whose manifest.transcriptEventCount is 0, silently dropping every message in the session. This hits real users: openclaw export-trajectory / /export-trajectory run against any session file written before entry timestamps existed.

Root cause

readSessionBranch filters session entries down to canonical transcript entries, but the filter also requires a present timestamp:

// before - src/trajectory/export.ts
const entries = fileEntries.filter(
  (entry): entry is SessionEntry =>
    entry.type !== "session" &&
    isCanonicalSessionTranscriptEntry(entry) &&
    typeof (entry as { id?: unknown }).id === "string" &&
    (typeof (entry as { timestamp?: unknown }).timestamp === "string" ||
      typeof (entry as { timestamp?: unknown }).timestamp === "number"),
);

The legacy migration migrateLegacySessionEntries synthesizes an id and parentId for every entry of a version < 2 session, but it never synthesizes a timestamp (those logs predate entry timestamps). The downstream event builder already tolerates a missing timestamp: buildTranscriptEvents calls normalizeTimestamp(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) plus isCanonicalSessionTranscriptEntry is what the branch walk needs; the timestamp is normalized later.

// after - src/trajectory/export.ts
const entries = fileEntries.filter(
  (entry): entry is SessionEntry =>
    entry.type !== "session" &&
    isCanonicalSessionTranscriptEntry(entry) &&
    typeof (entry as { id?: unknown }).id === "string",
);

normalizeTimestamp (already in the event path) defaults a missing or non-finite timestamp to new 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

readSessionBranch is 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 own isRuntimeTrajectoryEvent guard, 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).
  • New test exports the transcript for a legacy v1 session without entry timestamps fails on pristine main (transcriptEventCount is 0) and passes with the patch.
  • node scripts/run-oxlint.mjs src/trajectory/export.ts src/trajectory/export.test.ts clean.
  • oxfmt --check clean on both files.
  • tsgo -p tsconfig.core.json and -p test/tsconfig/tsconfig.core.test.json clean.

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 exportTrajectoryBundle runtime driven end to end over a v1 session JSONL (header version: 1, two timestamp-less message entries), on pristine main and 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:

# BEFORE (pristine main f94a2506d2)
FAIL  exports the transcript for a legacy v1 session without entry timestamps
expected 0 to be 2 // manifest.transcriptEventCount

# AFTER (this patch, identical input)
PASS  exports the transcript for a legacy v1 session without entry timestamps
manifest.transcriptEventCount === 2; eventTypes === ["user.message","assistant.message"]

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 real exportTrajectoryBundle runtime over a synthesized v1 session file rather than a CLI invocation against an on-disk historical session.

@openclaw-barnacle openclaw-barnacle Bot added size: XS proof: supplied External PR includes structured after-fix real behavior proof. labels Jun 16, 2026
@clawsweeper

clawsweeper Bot commented Jun 17, 2026

Copy link
Copy Markdown
Contributor

Codex review: needs maintainer review before merge. Reviewed June 21, 2026, 5:40 PM ET / 21:40 UTC.

Summary
The PR removes the timestamp requirement from trajectory session branch filtering and adds a regression test for timestamp-less legacy v1 session exports.

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.

  • Legacy export behavior: 1 predicate removed. This single predicate change flips timestamp-less legacy sessions from empty transcript exports to redacted transcript-bearing diagnostic bundles.

Stored data model
Persistent data-model change detected: serialized state: src/trajectory/export.test.ts. Confirm migration or upgrade compatibility proof before merge.

Root-cause cluster
Relationship: canonical
Canonical: #93814
Summary: This PR is the only open item found for timestamp-less legacy v1 trajectory export; related merged PRs are adjacent exporter and transcript-tree history, not duplicate remaining work.

Members:

Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything.

Merge readiness
Overall: 🐚 platinum hermit
Proof: 🐚 platinum hermit
Patch quality: 🦞 diamond lobster
Result: ready for maintainer review.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Rank-up moves:

  • Maintainers should accept the legacy transcript-bearing export behavior, or request CLI proof against a historical session file if that would change the merge decision.

Risk before merge

  • [P1] Merging changes legacy v1 trajectory bundles from empty transcript exports to redacted transcript-bearing diagnostic bundles, which maintainers should explicitly accept because these bundles can contain sensitive support data.
  • [P1] The supplied proof exercises the real exporter over a synthesized v1 JSONL session rather than a full CLI run against a historical on-disk session, although the inspected CLI path delegates to the same exporter.

Maintainer options:

  1. Accept legacy transcript exports (recommended)
    Approve the branch if maintainers want timestamp-less legacy sessions to export the same redacted transcript events that current sessions already export.
  2. Request command-level proof
    Ask for one CLI export against a historical timestamp-less session file if maintainer acceptance depends on end-to-end command evidence.
  3. Pause if old bundles should stay empty
    Keep the PR paused or close it if maintainers decide legacy v1 trajectory bundles should intentionally omit transcript events.

Next step before merge

  • [P2] Human review is the next action because the remaining blocker is maintainer acceptance of the diagnostic export content/privacy change, not a mechanical code repair.

Security
Cleared: The diff routes more legacy entries through the existing redacted, owner-approved trajectory export path and does not alter auth, dependencies, workflows, or secret handling.

Review details

Best 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 changes

Label justifications:

  • P2: This is a bounded shipped trajectory export bug where legacy sessions silently produce empty transcript bundles.
  • merge-risk: 🚨 other: The PR changes the contents of sensitive diagnostic export bundles for legacy sessions, which needs maintainer privacy/product acceptance beyond CI.
  • rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🐚 platinum hermit and patch quality is 🦞 diamond lobster.
  • status: 🚀 automerge armed: This PR is in ClawSweeper's automerge lane. Sufficient (live_output): The PR body includes copied before/after live output from the real exporter over a timestamp-less v1 JSONL session, showing transcriptEventCount changing from 0 to 2 after the patch.
  • proof: sufficient: Contributor real behavior proof is sufficient. The PR body includes copied before/after live output from the real exporter over a timestamp-less v1 JSONL session, showing transcriptEventCount changing from 0 to 2 after the patch.
Evidence reviewed

PR surface:

Source -2, Tests +35. Total +33 across 2 files.

View PR surface stats
Area Files Added Removed Net
Source 1 1 3 -2
Tests 1 35 0 +35
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 2 36 3 +33

What I checked:

  • Repository policy read: The full root AGENTS.md was read; no scoped AGENTS.md owns src/trajectory, and the only maintainer note present is Telegram-specific and not applicable to this trajectory exporter PR. (AGENTS.md:1, 108d6d7eca00)
  • Current main still drops timestamp-less legacy entries: Current main migrates version < 2 session entries by synthesizing ids and parentId values, but readSessionBranch then filters canonical entries out unless their top-level timestamp is a string or number. (src/trajectory/export.ts:125, 108d6d7eca00)
  • Latest release has the same bug path: The latest release tag v2026.6.9 contains the same v1 migration and timestamp predicate, so the fix is not already shipped. (src/trajectory/export.ts:125, c645ec4555c0)
  • Timestamp normalization already tolerates missing values: buildTranscriptEvents calls normalizeTimestamp(entry.timestamp), and normalizeTimestamp returns the Unix epoch for missing or invalid timestamps, so timestamp normalization is already downstream of branch selection. (src/trajectory/export.ts:357, 108d6d7eca00)
  • PR patch is narrowly scoped: The PR head keeps the canonical-entry and id checks and removes only the timestamp predicate from readSessionBranch. (src/trajectory/export.ts:184, 050fe83b9a71)
  • Regression coverage targets the legacy shape: The added test writes a version 1 session JSONL with timestamp-less message entries and asserts that two transcript events are exported. (src/trajectory/export.test.ts:1471, 050fe83b9a71)

Likely related people:

  • steipete: Commit 43c5650 introduced readSessionBranch and the timestamp predicate now being removed, and later trajectory commits touched the same export surface. (role: introduced reader behavior; confidence: high; commits: 43c56504758c, 78010b65edd5, 1888242bd30a; files: src/trajectory/export.ts, src/trajectory/export.test.ts)
  • scoootscooob: Merged PR feat: add trajectory bundle export and default-on runtime capture #70291 added trajectory bundle export, default-on capture, command wiring, docs, and initial export tests. (role: feature introducer; confidence: high; commits: a3d9c53db299; files: src/trajectory/export.ts, src/trajectory/export.test.ts, docs/tools/trajectory.md)
  • vincentkoc: Recent merged commits changed trajectory export branch warnings/cycle handling and transcript-tree wrappers, and this account requested automerge on this PR. (role: recent area contributor and automerge requester; confidence: medium; commits: 863069e2c66a, 0b7ff665f664, 5b030418c1fb; files: src/trajectory/export.ts, src/trajectory/export.test.ts, src/config/sessions/transcript-tree.ts)
  • pgondhi987: Merged PR fix: redact trajectory exports consistently #89354 changed redaction behavior across the same trajectory export bundle surface. (role: adjacent export contributor; confidence: medium; commits: 19fb9f12993d; files: src/trajectory/export.ts, src/trajectory/export.test.ts)
  • snowzlm: Merged PR fix(agents): preserve prompt-released session metadata #93194 introduced transcript-tree branch-selection behavior now used by readSessionBranch. (role: recent adjacent contributor; confidence: medium; commits: 1a002c2d9db7; files: src/config/sessions/transcript-tree.ts, src/trajectory/export.ts, src/trajectory/export.test.ts)
What the crustacean ranks mean
  • 🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
  • 🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
  • 🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
  • 🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
  • 🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
  • 🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
  • 🌊 off-meta tidepool: rating does not apply to this item.

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
  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

@clawsweeper clawsweeper Bot added proof: sufficient ClawSweeper judged the real behavior proof convincing. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. P2 Normal backlog priority with limited blast radius. labels Jun 17, 2026
@vincentkoc

Copy link
Copy Markdown
Member

/clownfish automerge

@vincentkoc vincentkoc added the clownfish:automerge Maintainer opted this Clownfish PR into bounded ClawSweeper-reviewed automerge label Jun 19, 2026
@vincentkoc

Copy link
Copy Markdown
Member

Clownfish is on the reef for this PR. 🐠

I tagged clownfish:automerge and sent ClawSweeper over this exact head. If the sweep finds rough coral, failing checks, or needs-human, I will take another bounded repair lap and ask for a fresh review.

A maintainer can call /clownfish stop any time and I will drift this back to human review.

@vincentkoc

Copy link
Copy Markdown
Member

/clownfish automerge

@vincentkoc vincentkoc added the clawsweeper:automerge Maintainer opted this PR into bounded ClawSweeper-reviewed automerge label Jun 19, 2026
@vincentkoc

Copy link
Copy Markdown
Member

Clownfish is on the reef for this PR. 🐠

I tagged clownfish:automerge and sent ClawSweeper over this exact head. If the sweep finds rough coral, failing checks, or needs-human, I will take another bounded repair lap and ask for a fresh review.

A maintainer can call /clownfish stop any time and I will drift this back to human review.

@clawsweeper clawsweeper Bot added status: 🚀 automerge armed This PR is in ClawSweeper's automerge lane. clawsweeper:human-review Needs maintainer review before ClawSweeper can continue and removed status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. labels Jun 19, 2026
@clawsweeper

clawsweeper Bot commented Jun 19, 2026

Copy link
Copy Markdown
Contributor

🦞✅
ClawSweeper is pausing this repair loop for human review.

Source: clawsweeper[bot]
Reason: - [P2] Human review is the next action because the remaining blocker is maintainer acceptance of the diagnostic export content/privacy change, not a mechanical code repair.; Cleared: The diff routes more legacy entries through the existing redacted, owner-approved trajectory export path and does not alter auth, dependencies, workflows, or secret handling. (sha=050fe83b9a71373c5e978ed51d689d3ffdb55e15)

Why human review is needed:
This item has security-sensitive risk. ClawSweeper is pausing instead of making an autonomous change that could affect trust, credentials, permissions, or exposure.

What the maintainer can do as a next step:
If the maintainer accepts the current risk and wants ClawSweeper to continue merge gates, comment @clawsweeper approve. If the security-sensitive detail still needs changes, describe the safe path or push the fix, then comment @clawsweeper automerge. If the risk should not be automated, keep the PR paused for manual review or comment @clawsweeper stop.

I added clawsweeper:human-review and left the final call with a maintainer.

@yetval

yetval commented Jun 19, 2026

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-run

@clawsweeper

clawsweeper Bot commented Jun 19, 2026

Copy link
Copy Markdown
Contributor

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

@clawsweeper clawsweeper Bot added the merge-risk: 🚨 other 🚨 Merging this PR has meaningful risk outside the owned taxonomy. label Jun 20, 2026
@obviyus obviyus self-assigned this Jun 25, 2026
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.
@obviyus
obviyus force-pushed the fix/trajectory-export-legacy-v1-timestamp branch from 050fe83 to 5d619f9 Compare June 25, 2026 15:28
@obviyus
obviyus merged commit 446d98d into openclaw:main Jun 25, 2026
43 checks passed
@obviyus

obviyus commented Jun 25, 2026

Copy link
Copy Markdown
Contributor

Landed via rebase onto main.

  • Scoped tests: node scripts/run-vitest.mjs src/trajectory/export.test.ts -t 'legacy v1 session'; git diff --check origin/main...HEAD
  • Changelog: skipped; CHANGELOG.md is release-owned for normal PRs
  • Land commit: 5d619f9
  • Merge commit: 446d98d

Thanks @yetval!

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

Labels

clawsweeper:automerge Maintainer opted this PR into bounded ClawSweeper-reviewed automerge clawsweeper:human-review Needs maintainer review before ClawSweeper can continue clownfish:automerge Maintainer opted this Clownfish PR into bounded ClawSweeper-reviewed automerge merge-risk: 🚨 other 🚨 Merging this PR has meaningful risk outside the owned taxonomy. P2 Normal backlog priority with limited blast radius. proof: sufficient ClawSweeper judged the real behavior proof convincing. proof: supplied External PR includes structured after-fix real behavior proof. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. size: XS status: 🚀 automerge armed This PR is in ClawSweeper's automerge lane.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants