Skip to content

fix(storage): import Codex item_completed messages - #3520

Merged
Astro-Han merged 2 commits into
apache:mainfrom
sunrioa:codex/fix-codex-item-completed-import
Aug 23, 2026
Merged

fix(storage): import Codex item_completed messages#3520
Astro-Han merged 2 commits into
apache:mainfrom
sunrioa:codex/fix-codex-item-completed-import

Conversation

@sunrioa

@sunrioa sunrioa commented Aug 22, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Read Codex 0.149 event_msg.item_completed records for UserMessage, Reasoning, and AgentMessage.
  • Preserve message IDs, turn IDs, timestamps, and active model metadata.
  • Derive imported-session titles from completed user items while retaining support for the legacy user_message and agent_* event shapes.
  • Continue treating event_msg as the presentation-message source so that corresponding response_item mirrors are not imported twice.
  • Add a sanitized Codex 0.149 regression fixture covering title discovery, message conversion, terminal state, canonical decoding, and duplicate suppression.

Fixes #3516
Refs #2499

Verification

  • Focused codex-session-adapter suite: 9 passed.
  • NODE_NO_WARNINGS=1 npm --workspace @maka/storage run test:dist: 899 passed, 14 skipped.
  • npm run lint
  • npm run format:check
  • npm run build
  • npm run typecheck
  • npm run check:asf-headers
  • git diff --check
  • Real rollout conversion smoke test restored 1 user message, 2 assistant messages, and 1 terminal turn-state message without importing response mirrors twice.
  • Before-fix evidence and environment details are available in Codex Desktop 0.149 conversations import with an empty transcript #3516.

Without NODE_NO_WARNINGS=1, one unrelated managed-dependency environment test treats Node.js's experimental node:sqlite warning on child-process stderr as a failure. The full storage suite passes when that warning is disabled.

Review focus

The adapter keeps the legacy and Codex 0.149 presentation formats side by side. event_msg remains the authority for user, reasoning, and assistant presentation messages, while response_item remains responsible for tool calls and tool results. This preserves the existing duplicate-prevention boundary.

AI use

Select exactly one:

  • No generative tool made a substantive contribution
  • Generative tooling made a substantive contribution

Tool(s) and scope:

OpenAI Codex assisted with the rollout-format investigation, implementation, regression tests, local verification, and drafting this description. I reproduced the issue and reviewed the final diff, regression coverage, and test results before submission.

Checklist

  • Tests cover the change and fail without it
  • Lint, format, typecheck and the affected suites pass locally

Does this PR entail a change in behavior?

  • Yes — described under Summary above
  • No

@liugddx

liugddx commented Aug 23, 2026

Copy link
Copy Markdown
Member

Review at exact head 317b38e5af5950a1365ca3f5e3141839830c1430.

The focused adapter change closes the Codex 0.149 item_completed parsing gap as scoped. I found no blocking defect in the three changed files. The two findings below came from real Desktop import/rendering verification; both are pre-existing renderer-range concerns and should be tracked outside this PR rather than expanding it.

P2 (follow-up, not introduced here) — apps/desktop/src/main/desktop-transcript-replica.ts:267 — a byte-bounded navigation range can end in the middle of one turn

Mechanism: loadAround(sequence) requests a forward page capped at 512 KiB. A tool-heavy turn can exceed that budget before its terminal assistant message. The sparse replica then publishes only the turn prefix. Because the turn ID is now resident, a later prompt-rail click takes the local revealTurn path in packages/ui/src/chat-view.tsx:440-444 instead of loading the missing continuation, so the suffix is not reachable from that turn.

Concrete trigger: in a real imported Codex transcript, one turn occupied sequences 0–53. Navigation rendered only the early prefix (approximately through sequence 18); the sequence-52 final assistant message—2,444 characters containing headings, a list, and a fenced code block—remained present in SQLite but absent from the DOM.

Fix options:

  • Enforcement: make target navigation materialize a complete bounded turn, continuing pages to the target turn's terminal sequence (with an explicit oversized-turn policy).
  • Claim/scope: keep this PR limited to parsing/persistence and track complete turn-range rendering under perf(desktop): make session data flow incremental and bounded #2913 Phase 3.

Recommendation for this PR: the second option. Do not widen #3520; record the first option as a renderer follow-up. This is a product P2, not a regression introduced by this diff.

P3 (follow-up, not introduced here) — packages/ui/src/prompt-anchor-rail.tsx:526 — multiple user prompts in one Codex turn produce duplicate React keys

Mechanism: the landmark index can contain multiple user-message entries with the same turnId, while the rail maps every entry with key={turn.turnId}. The transcript itself preserves and renders the prompts in sequence, but the rail emits duplicate-key warnings and cannot give the two prompt ticks distinct identity.

Concrete trigger: the real import contained one turn with three user prompts and another with two. Both prompts in the latter were visible in the transcript, while the rail logged duplicate keys for that shared turn ID.

Fix options:

  • Enforcement: key/navigate each prompt landmark by stable (turnId, sequence) identity.
  • Claim/UI policy: coalesce the rail to one tick per turn.

Recommendation: use (turnId, sequence) if the rail remains “one tick per prompt”; handle separately from #3520.

Verification performed:

  • exact-head storage build: pass;
  • CodexSessionAdapter: 9/9 tests pass;
  • Biome on the two changed TS files and git diff --check: pass;
  • seven real item_completed rollouts: recognized source-event counts matched converted message counts, with no response-mirror duplicates;
  • one complete persisted import: 1,560/1,560 source messages matched exactly; zero duplicate message IDs, zero empty visible messages, 674/674 paired tool calls/results, and zero timestamp regressions;
  • a fully resident turn rendered fenced code, inline code, links, lists, and tool cards with the expected DOM structure.

Coverage limit: the imported real-world samples had no non-empty reasoning summary, so real Electron rendering of the new Reasoning path was not exercised; that path is covered by the sanitized adapter fixture.

Merge verdict: ready for the focused adapter fix; track the renderer P2/P3 outside this PR.

简体中文

本次按 exact head 317b38e5 审查。#3520 修改的三处文件中没有发现阻塞问题,新版 Codex item_completed 的解析和持久化修复符合当前范围。

实机导入和页面检查发现两个既有的 Renderer 后续问题,但都不建议塞进 #3520 扩大 PR:

  1. P2:按 512 KiB 加载历史时可能从同一个长 turn 中间截断。 真实样本中,一个 0–53 序列的 turn 只显示了前半段;序列 52 的 2,444 字最终回复、标题、列表和代码块仍在 SQLite 中,但没有进入 DOM。建议在 perf(desktop): make session data flow incremental and bounded #2913 Phase 3 中让目标导航加载完整 turn,并为超大 turn 定义明确策略。
  2. P3:同一 turn 内多次用户追加消息会让 prompt rail 使用重复的 turnId key。 正文仍按顺序显示,但 rail 会产生 React duplicate-key 警告。若继续保持“一条提问一个 tick”,建议使用 (turnId, sequence) 作为稳定身份。

验证包括:针对性测试 9/9、构建/格式检查通过;7 个真实新版 rollout 的事件与转换计数一致;一份完整导入的 1,560 条消息与源记录逐条一致,工具调用/结果 674/674 配对,且没有重复 ID、空正文或时间倒序。代码块、链接、列表和工具卡片在完整加载的 turn 中均正常渲染。

真实样本没有非空 reasoning summary,因此新版 Reasoning 的真实 Electron 格式尚未覆盖,目前只有 sanitized fixture 的 adapter 测试证据。

结论:#3520 可以按聚焦修复合并;Renderer 的 P2/P3 应单独跟踪,不扩大本 PR。

@sunrioa
sunrioa marked this pull request as ready for review August 23, 2026 16:10

@Astro-Han Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed exact head 317b38e. Coverage: upstream OpenAI Codex TurnItem/UserMessageItem/AgentMessageItem/ReasoningItem shapes; missing and extra fields; content-part ordering; response-item mirror suppression; IDs, turns, timestamps, model metadata, title discovery, and terminal conversion. Exclusions: private user rollout contents and future unreleased Codex schemas. Local Core→Storage builds and the focused 9/9 adapter suite are green; hosted exact-head test is green. Current verdict: NO-GO with one inline P2. The branch is 77 commits behind main but a current-main merge-tree probe is clean.

Comment thread packages/storage/src/codex-session-adapter.ts Outdated
Match the upstream Codex contract by concatenating completed user and agent text parts without injecting separators. Strengthen the regression fixture so split-word boundaries fail under the previous behavior.

Generated-by: OpenAI Codex

@jackwener jackwener left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Independent re-review of exact head 0b4af5e4e3e48d40b41e04033dd23ce78046e35b。不继承 317b38e5 上的 NO-GO。

三问

  1. 要解决什么:Codex 0.149 把展示历史改成 item_completed,旧适配器只认 user_message / agent_*,导入会丢对话。
  2. 怎么解决:从 event_msg.item_completed 读 UserMessage / AgentMessage / Reasoning;response_item 的 message/reasoning 镜像仍然不收,避免双份;tool call 仍走 response_item
  3. 奥卡姆:三条 item 类型 + 文本拼接 + catalog 首条用户文本,范围对。

旧 [P2](317b38e5 的 join \n

这个 head 上 已经关掉codexCompletedItemText 对 UserMessage / AgentMessage 是 join('')。新 fixture 把 image / canvas 拆在相邻 text part 里,旧的 join('\n') 会断言失败。Reasoning 的 summary_text 仍用 join('\n'),那是条目之间的分隔,不是把一个词切开。

CI

期望:test。未改 package.json / sandbox。
这个 head 的 CI 是 fork action_requiredcheck-runs total_count=0。空不等于绿。workflow 放行之前门禁不能过。

代码侧 GO(仅 [P3])。门禁 NO-GOaction_required)。未 approve,未合。

if (!activeTurnIsExplicit) {
activeTurnId = generatedCodexId(expectedSessionId, 'turn', record.line);
}
const text = codexCompletedItemText(item) || codexCompletedItemMediaText(item);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P3] codexCompletedItemText(item) || codexCompletedItemMediaText(item):同一条 UserMessage 里只要有非空 text part,后面的 local_image / audio 就不会变成 [Image] / [Audio]。新 fixture 在两段文本中间夹了 local_image,只为了钉 join(''),导入结果是纯文本,图被丢掉。旧 user_message 路径也是「有 message 就不用 media placeholder」,所以不是新回归,只是 0.149 的混合 content 仍然没有图。不挡这个 PR。

@Astro-Han Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gate check at exact head 0b4af5e4: hosted test is terminal completed/success (the only check this path set triggers) and mergeable=true.

I independently re-verified my earlier [P2] on the content-part concatenation contract at this head: codexCompletedItemText now ends in .join(''), so multi-part text is concatenated without an invented separator, matching upstream UserMessageItem::message(). That thread is resolved. The remaining open thread is a [P3] on the text/media fallback ordering.

Approving.

@Astro-Han
Astro-Han merged commit f0b5f6e into apache:main Aug 23, 2026
1 check passed
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.

Codex Desktop 0.149 conversations import with an empty transcript

4 participants