fix(codex) when using /list command, correctly read codex project session names - #1639
Conversation
chenhg5
left a comment
There was a problem hiding this comment.
Conclusion: Comment (DRAFT, advisory — no request-changes)
Overall assessment:
- Core fix is correct, test coverage is solid, diff is contained in
agent/codex, and the PR description is exemplary./listnow properly readssession_index.jsonl'sthread_nameand filters out subagent rollouts. 3rd-party author, recommend keeping--commentuntil DRAFT is flipped to ready-for-review.
Review scope:
- Read
agent/codex/list.goand the newagent/codex/list_test.go. - Ran
go test -race ./agent/codex/... -count=1(ok 2.575s); new 3 tests pass standalone with-racetoo. - Focused on correctness of subagent filtering, error paths in
session_index.jsonlparsing, blast radius.
✅ What looks good:
- Root-cause analysis is accurate: subagent rollout files may contain a copied parent meta, the old parser was always overwriting
sessionID/sessionCwd/sessionSource, which misclassified subagents as their parent. The newif sessionID != "" { continue }locks the first meta — a tight, well-targeted minimal fix. - Using Codex's own
session_index.jsonlas the thread-name source (instead of inventing a parallel naming scheme) avoids drift with the Codex picker. - Subagent filtering aligns with the existing pattern in
agent/gemini/gemini.go:540andagent/antigravity/antigravity.go:540, bringing codex to parity. - Tests use
Agent.ListSessionsas a black box andt.TempDir(), locking in three independent invariants (subagent filtered, copied parent meta not adopted, thread_name overrides user-message summary). - errcheck follow-up is strictly scoped to the new
defercalls; pre-existingdefer f.Close()at list.go:118/255 and context_usage.go:193 are left alone. - PR description is gold standard: what/why/root-cause/user-impact/validation, with concrete 118-rollout → 5 top-level validation data.
🟠 Should improve (non-blocking; author can decide whether to bundle before ready-for-review):
- The thread_name override skips the 60-rune
Summarytruncation inparseCodexSessionFile(list.go:64–66 vs 204–206). Codex currently emits short thread_names so no real-world impact, but if Codex ever generates longer names, IM cards will receive uncapped content. Recommend addingif len([]rune(title)) > 60 { title = string([]rune(title)[:60]) + "..." }before the assignment, plus aTestAgentListSessions_LongThreadNameTruncatedtest to lock the invariant. isSubagentSessionSourcereturns true for{"subagent": null}(list.go:218–225). Codex doesn't actually emitnullso impact is zero, but for strict shape matching one could read the value and require it to be non-null. Lower priority than the cap above.
🔵 Optional:
- Test name
TestAgentListSessions_ExcludesSubagentRolloutWithCopiedParentMetais verbose;TestAgentListSessions_FirstSessionMetaWinsOverCopiedParentwould directly telegraph the invariant. loadCodexSessionTitlessilently returns nil onos.Openfailure; aslog.Warnwould help diagnose permissions/IO issues.- The defensive
if sessionID != "" { continue }deserves a one-line comment explaining the first-meta-wins rationale (the PR description has it; the source doesn't).
❓ Questions: None.
Testing / Risk:
- Verified: PR ships with 3 new unit tests + author's local 118-rollout end-to-end validation; local
go test -race ./agent/codex/... -count=1is all ok;go build/go vetclean. - Uncovered: corrupt
session_index.jsonlline (per code it skips, but no test locks that); latest-wins behavior with multiple index entries for same ID after rename; rollout exists but no index entry fallback. All reasonable as follow-ups.
DRAFT note: PR is currently in DRAFT. Per QA policy, even with --approve allowed for 3rd-party, I don't approve while in DRAFT — leaving a comment for the author to convert to ready. If you want to be conservative, you can land the P2 #1 (60-rune title cap + corresponding test) in the same PR; otherwise the current diff is good to merge as-is.
Next step: Author flips the PR from DRAFT to ready-for-review. If desired, include the P2 #1 (60-rune title cap + test); otherwise the diff is sufficient — QA has no hard blocker.
…, drop duplicated agyConfigDir block
Sync main to fork's next-publishable identifier. Content: upstream v1.5.0-beta.3 (P1 stability chenhg5#1693) + codex /list fix chenhg5#1639 + 6 fork-local commits (mimocode adapter etc.) per merge commit 55159fb.
What changed
session_metaentry as the rollout identity so copied parent metadata cannot turn subagent rollouts into duplicate top-level sessions./listresults.session_index.jsonl, with the existing user-message summary as a fallback.Why
/listcould show many internal subagent rollouts and display concrete user messages instead of the project session names shown by Codex. This made session counts and names differ between cc-connect and Codex for the same project.Root cause
Subagent rollout files can contain copied parent history with additional
session_metaentries. The parser kept overwriting the rollout identity, so some subagent files were misclassified as duplicate parent sessions. The list implementation also derived summaries only from transcript messages and did not consult Codex's session-name index.User impact
Telegram
/list,/switch, and other session-list consumers now see the same top-level Codex sessions and generated session names as the Codex project session picker.Validation
go test ./agent/codex -count=1go test ./...was attempted; unrelated existing Windows path/permission assertions, a missingweb/dist, and theagent/piWindows build currently prevent a clean repository-wide run.