Skip to content

fix(storage): validate imported Codex messages canonically - #3653

Closed
liuxiaocs7 wants to merge 1 commit into
apache:mainfrom
liuxiaocs7:fix/storage-codex-canonical-decode-test
Closed

fix(storage): validate imported Codex messages canonically#3653
liuxiaocs7 wants to merge 1 commit into
apache:mainfrom
liuxiaocs7:fix/storage-codex-canonical-decode-test

Conversation

@liuxiaocs7

Copy link
Copy Markdown
Member

Summary

Use decodeCanonicalMessage when validating messages returned by CodexSessionAdapter.

The test currently calls an undefined decodeStoredMessage, which prevents @maka/storage and the repository CI build from compiling. Adapter output is canonical in-memory data, so the canonical decoder is the appropriate boundary and matches the existing validation in the same test file.

Verification

  • npm --workspace @maka/storage run build
  • node --test packages/storage/dist/__tests__/codex-session-adapter.test.js — 9 passed
  • npx biome format packages/storage/src/__tests__/codex-session-adapter.test.ts
  • npx biome lint packages/storage/src/__tests__/codex-session-adapter.test.ts
  • git diff --check
  • npm run build now passes the previously failing Storage stage; it later encounters an unrelated existing CLI dependency/type mismatch.
  • Full Storage suite: 905 passed, 14 skipped, and one unrelated cross-process authority test failed because a child-process SQLite experimental warning was treated as error output.

AI use

Select exactly one:

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

Tool(s) and scope: Codex diagnosed the CI failure, applied the one-line decoder correction, and ran verification.

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

Use the canonical message decoder for adapter output so the storage package builds after the persisted decode boundary split.\n\nGenerated-by: Codex
@liuxiaocs7

Copy link
Copy Markdown
Member Author

Hi, @Astro-Han, could you help take a look when free, the build fails on main too, thanks!

@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 review of exact head 1643ed16799edfab0534653587ec796005f54251.

三问

  1. 要解决什么:#3520 的 item_completed 测试调用了 decodeStoredMessage。persisted decode 边界拆开之后,这个函数只接受 PersistedValue<StoredMessage>,adapter 测的是内存里的 canonical 形状,于是 @maka/storage 编不过。
  2. 怎么解决:改成 decodeCanonicalMessage,跟同一文件上面那条 presentation/tool 测试(约 L194)同一条边界。
  3. 奥卡姆:+1/−1,测试-only,没有新规范。

相对 #3520

这个 PR 的 merge-base 就是当前 mainc79e9eb40),f0b5f6ee 是祖先。base 含 #3520 本 head 上 codexCompletedItemText 结尾仍是 .join('')codex-session-adapter.ts:900)。fixture 仍把 Analyze the ima+ge. Use OpenCV.js.Use can+vas. Then process the pixels. 切开,断言期望无注入换行。没有把旧的 join('\n') 固化成期望。

「canonical」以谁为权威

不是作者给 Codex 新发明的规范。decodeCanonicalMessage@maka/core/session内存 StoredMessage 的解码器;decodeStoredMessage 是拆开之后的 persisted 解码器。adapter 输出走前一条,和 Claude adapter 测试、同文件既有断言一致。Codex 分片拼接契约仍是 #3520 合进去的 join(''),这单没动它。

CI

期望:test(只改了一个测试文件)。test completed/success。mergeable=MERGEABLE,不是 conflict;BLOCKED 是缺 required review。

无 [P0]–[P3]。GO。 未 approve(按派单由你批),未合。

@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 1643ed16: hosted test is terminal completed/success (the only check this path set triggers), mergeable=true, and there are no unresolved review threads.

The change is one line in packages/storage/src/__tests__/codex-session-adapter.test.ts: the round-trip assertion switches from decodeStoredMessage to decodeCanonicalMessage. That is the correct decoder for what the assertion actually holds — an in-memory message shape — now that decodeStoredMessage is scoped to the persisted boundary; line 194 of the same file already uses the canonical decoder for the same reason. "Canonical" here refers to the existing @maka/core/session decoder rather than a validation contract invented in this PR.

I also confirmed this branch is based on top of f0b5f6ee (#3520), so it does not reintroduce the content-part concatenation problem fixed there: codexCompletedItemText is still .join(''), and the fixtures that split image and canvas across content-part boundaries expect no injected newline.

Approving.

@Astro-Han

Copy link
Copy Markdown
Contributor

Closing this as already-landed rather than as rejected — the change was right, and it mattered.

While this was queued, main was red: packages/storage/src/__tests__/codex-session-adapter.test.ts:267 called decodeStoredMessage while the file only imported decodeCanonicalMessage, so the Build step failed and every PR based on main inherited that failure. That was a semantic conflict between two PRs that were each green on their own base — #3562 changed the import while separating the persisted decode boundaries, and #3520 added an assertion using the previous symbol. The two edits touched different lines, so git merged them without complaint.

This PR was the fix. #3656 landed the identical one-line change first, so main is green again at 3bb645e9 and the diff here is now empty — git diff origin/main <head> produces no output. There is nothing left to merge.

Thanks for catching the inconsistency independently; the review of it verified that decodeCanonicalMessage is the correct decoder for an in-memory message shape and that line 194 of the same file already used it.

@Astro-Han Astro-Han closed this Aug 24, 2026
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.

3 participants