fix(core,storage): unify the Codex thread source gate - #3702
Conversation
|
@Astro-Han when you have a moment, would you mind taking a look at this one? Status: CI is green, and GitHub reports it as mergeable against current No rush — flagging it since it is review-ready and I would rather not let it drift into conflict. Happy to rebase or split it if that makes review easier. |
6a3c67b to
87ea505
Compare
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Thanks for consolidating the Codex source parsing and removing the duplicate adapter predicate. A shared gate is the right direction, and the adapter-level tests describe the intended token set clearly.
The remaining issue is at the composition boundary. I left one inline P1 because the primary SQLite scanner still applies its old SQL allowlist before the shared gate can run, so the normal product path does not receive several newly supported sources. The rollout fallback also does not read or check payload.source, meaning unsupported and subagent rollouts can still pass when SQLite is unavailable.
In other words, the shared helper is correct, but it has not yet become the authority for both scanner paths. The PR is also currently conflicting with main; after resolving the scanner boundary, it will need a rebase and fresh exact-head CI.
Reviewed with Codex and an independent @Reviewer agent. I verified the exact head, adapter and store call paths, SQLite prefilter, rollout fallback, test coverage, and current merge state.
中文对照
谢谢你收敛 Codex source 解析并删除 adapter 中重复的 predicate。建立共享 gate 是正确方向，adapter 层测试也清楚描述了预期支持的 token。
剩余问题位于真正的 composition boundary。我留了一条 P1：主要的 SQLite scanner 仍然会在共享 gate 执行前使用旧 SQL allowlist 过滤数据，因此正常产品路径仍然无法看到多个新支持的 source。没有 SQLite 时使用的 rollout fallback 也没有读取和检查 payload.source，所以 unsupported 或 subagent rollout 仍可能进入列表。
也就是说，共享 helper 本身是正确的，但它还没有真正成为两条 scanner 路径的 authority。这个 PR 当前也与 main 冲突；修复 scanner 边界后，还需要 rebase 并重新跑精确 head 的 CI。
本次审查使用了 Codex 和一个独立的 @Reviewer 子代理；我核对了精确 head、adapter 与 store 调用路径、SQLite 预过滤、rollout fallback、测试覆盖和当前合并状态。
The foreign-session scanner and the Codex Session adapter each owned a
private set of eligible `source` tokens, so the same Codex thread could be
visible through one surface and invisible through the other:
- bare `exec` was accepted by the adapter but dropped by the scanner;
- bare `atlas`/`chatgpt` and wrapped `{"custom":"cli"}` / `{"custom":"vscode"}`
were accepted by the scanner but dropped by the adapter;
- a NULL `source` column was admitted by the adapter but dropped by the
scanner, hiding threads written by older Codex schemas.
Make `CODEX_SUPPORTED_THREAD_SOURCES` in `@maka/core/foreign-session` the
single authority (`cli`, `exec`, `vscode`, `atlas`, `chatgpt`) and delete the
adapter's duplicate gate. `codexSourceToken` now also accepts an
already-parsed object, which is the shape rollout `session_meta` payloads
arrive in, and the new `isSupportedCodexThreadSource` states the
absent-is-eligible rule once instead of at each call site. Internal
subagent threads (`{"subagent":{…}}`) still resolve to no token and stay
out of both surfaces.
Closes apache#3693
87ea505 to
05f4056
Compare
|
Rebased onto current Conflicts and how they were resolved:
Net diff is unchanged at +112/-40 across the same 4 files, and nothing under Verified locally on the exact pushed head (this repo is npm workspaces +
On your inline P1: still open, and deliberately not fixed in this push. You are right on both counts — @Astro-Han no rush at all, but since the rebase you asked for is done and CI is running on the exact head, this is ready for another look whenever it suits you. |
Generated-by: OpenAI Codex
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Thanks for consolidating the Codex thread source gate into the shared core/storage boundary. The normal SQLite-backed path now uses one source policy across scanning and import, which fixes the real drift without creating a second catalog authority.
I found no P0 or P1 issues on exact head dd591d148b7d7a4a64c24d8503a7630a998840c1. One non-blocking P2 remains in the recovery fallback: when SQLite is unavailable, rollout JSONL metadata currently drops the source field before the shared gate, so unsupported/subagent sessions may still appear in the picker and be importable. This requires the fallback path, a user selection, and remains reversible, so it should be fixed but does not block the normal-path correction. Approving.
AI-assisted review: Codex helped inspect the source-policy authority, SQLite and JSONL paths, import reachability, tests, and exact-head CI. I verified the severity and final approval decision.
中文对照
谢谢把 Codex thread 的 source gate 收敛到共享的 core/storage 边界。正常 SQLite 路径现在让扫描和导入共用一套 source policy，修复了真实漂移，也没有建立第二套 catalog 权威。
精确 head 上没有 P0/P1。恢复 fallback 中还有一个不阻塞的 P2：SQLite 不可用时，rollout JSONL metadata 会在共享 gate 前丢掉 source 字段，因此 unsupported/subagent session 仍可能出现在选择器中并被导入。它需要进入 fallback、再由用户主动选择，而且可逆，所以应该修，但不阻塞正常路径的修正。批准。
本次评审使用 Codex 辅助检查 source policy 权威、SQLite/JSONL 路径、导入可达性、测试和精确 head CI；分级和最终批准决定由我确认。
Use one shared source policy for Codex thread discovery and import across the core and SQLite-backed paths.
2 participants
Summary
Closes #3693.
The foreign-session scanner (
@maka/core/foreign-session) and the Codex Session adapter (@maka/storage) each owned a private set of eligible Codexsourcetokens. The sets drifted, so the same Codex thread could be visible through one surface and invisible through the other:sourceon the threadexecatlas,chatgpt(bare){"custom":"cli"},{"custom":"vscode"}The last two rows are divergences beyond the table in the issue. The NULL case is the one that bites hardest in practice: older Codex schemas have no
sourcecolumn at all, so the scanner hid every legacy thread that the adapter happily catalogued.CODEX_SUPPORTED_THREAD_SOURCESin@maka/core/foreign-sessionis now the single authority, and the adapter's duplicateisRootCodexSourceis deleted.On the union. The issue asks for a ruling on whether bare
execbelongs. It does: #2502's own description lists "rootcli,exec, andvscodeSessions" as what the adapter surfaces, andexechas been in the adapter's set since its first commit (23f624b). The scanner's set predates it (#1057, #1208) and simply never learned about headlesscodex execruns. The unified set is thereforecli,exec,vscode,atlas,chatgpt— the union, adoptingexecrather than dropping it.Two supporting changes fall out of the merge:
codexSourceTokennow also accepts an already-parsed object, which is the shape rolloutsession_metapayloads arrive in. Previously the adapter needed its own recursion to handle that; now one function covers the bare token, the JSON object string, and the parsed object.isSupportedCodexThreadSourcestates the absent-is-eligible rule once, instead of having each call site re-derive it (the scanner spelled itrow.source !== undefined && token === undefined, the adapter spelled it as an earlyreturn true, and only one of the two also handlednull).Internal subagent threads (
{"subagent":{"thread_spawn":{…}}}) resolve to no token in either form and stay out of both surfaces, unchanged.Verification
npm --workspace @maka/core run test— 658 pass, 0 fail (clean + build +node --test)npm --workspace @maka/storage run test— 927 pass, 14 skipped, 0 failnpm exec -- biome checkon the four changed files — no fixes appliedNew coverage:
packages/core/src/__tests__/foreign-session.test.ts— bareexec; already-parsed objects ({custom:'atlas'}→atlas,{custom:'unknown'}→ undefined, subagent → undefined); a table-driven check that every token in the set resolves in both the bare and{"custom":…}forms; NULLsourcecolumn treated as absent rather than unsupported.packages/storage/src/__tests__/codex-session-adapter.test.ts— a catalog listing seeded with all five sources in both forms plus a subagent thread, asserting the adapter now lists exactly what the scanner accepts and still excludes the subagent.The storage test was checked against a negative control: reverting only
codex-session-adapter.tsand rebuilding fails it withwrapped cli was dropped, so it exercises the merged gate rather than passing vacuously.