fix(core,storage): unify the Codex thread source gate - #3702
Open
cat0825 wants to merge 1 commit into
Open
Conversation
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
Contributor
Author
|
@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. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.