feat(codex): add Codex session transcript adapter support#816
feat(codex): add Codex session transcript adapter support#816rehan6025 wants to merge 5 commits into
Conversation
|
✅ Health: 7.6 (unchanged) 📋 At a glance
📊 Full report · ⭐ Star Repowise · 📥 Install bot · Last updated 2026-07-15 10:54 UTC |
|
Thanks for this @rehan6025, the adapter is wired in cleanly and the seam is right. One blocking issue and a few smaller things. Blocking: discover() returns nothing in real usage, and the discover test fails. The last commit switched to a non-recursive glob: return sorted(path for path in root.glob("*.jsonl") if path.is_file()) But Codex writes rollouts into a date-nested tree (~/.codex/sessions/YYYY/MM/DD/*.jsonl), which the module docstring and #785 both call out. A non-recursive *.jsonl only matches files sitting directly under sessions/, so it finds nothing in the real layout and no transcripts ever reach the miners. I ran the branch to confirm. test_discover_lists_jsonl_sorted creates nested files and currently fails: tests/unit/sessions/test_codex_adapter.py::test_discover_lists_jsonl_sorted FAILED So the "pytest passes" box isn't accurate on the current head. Switching to root.rglob("*.jsonl") fixes both the real-world discovery and the test. Worth double checking whatever motivated the non-recursive change, since it goes against the documented Codex layout. Fixture isn't real-shaped. tests/unit/sessions/data/codex_session.jsonl uses an invented flat shape (top-level type: "assistant", top-level text and tool_calls[]). Real Codex rollouts, which _fill_response_item and _event_kind are actually built for, use session_meta / response_item / event_msg with a nested payload. The inline test does exercise the real shapes, so the code path is covered, but #785 specifically asks for a real-shaped fixture and to confirm the format first. Suggest replacing the fixture with an actual multi-line rollout. No miner-level test. #785's "done when" includes the decisions and demand miners producing non-empty output on a fixture with the relevant tool calls. Right now the tests only check normalize() field mapping. Since tool-name normalization is the load-bearing part, one end-to-end test through a miner would guard against regressions the field checks miss. Smaller stuff:
The tool-name aliases line up with what the demand miner keys on, so that part looks good. The recursive-glob fix is the one that has to land; the rest is follow-up. |
|
Thanks for the detailed review! I'll address the recursive discovery issue and work through the remaining feedback, then push an update. |
|
Thanks @rehan6025, this is a solid update. Discovery is fixed and using iter_glob instead of plain rglob is better than what I suggested, it's the repo's own pruned walker so you get the junk-dir skipping for free. parse_timestamp is imported now, the fixture is real-shaped, and all 90 tests in tests/unit/sessions/ pass. Two things to sort before this lands. The miner tests are green but hollow. The fixture is two sessions concatenated (a session_meta on line 1 and another on line 5), and every tool call in the second one is dropped on the floor: lines 8, 10 and 14 have no call_id or id, and _fill_response_item needs one before it appends a ToolUse. Run lines 5-15 on their own and you get zero decisions and zero demand. Everything both miner tests assert is coming from the single search call on line 3. That also means the decision test is pinning a misattribution: decision kind=explicit_choice files=['pkg/app.py'] The Flask quote lives in session 2 and is about outputs/sqlite-signup-demo/app.py. It only gets pkg/app.py because trailing_files is a running window still holding session 1's search hit. So assert decision.files == ["pkg/app.py"] is locking in cross-session bleed. Give the exec calls real call_ids and that assertion should change to the file the decision is actually about. A one-session fixture would sidestep the bleed entirely and is closer to what a real rollout looks like anyway. session_id is picking up message ids. normalize() falls back to payload.get("id"). On session_meta that's correct, the payload id really is the session. On any other response_item it's the message or tool id: event session_ids: ['0199f5aa-real-session-uuid', 'tool_09', 'msg_042'] MINED decision: kind=explicit_choice files=['pkg/app.py'] This one has teeth: decisions.py stamps session_id=event.session_id onto every candidate (330, 355, 376), persists it to staging, and the injection-feedback loop keys correction_quotes(session_id) on it (661-669). Every Codex decision ends up under its own fake session, so that loop can never match. The Claude adapter reads one field with no id fallback, which is why it never hit this. Codex only carries the session id on session_meta, so it needs threading through the iteration rather than guessing per line. While you're in there: ToolResult hardcodes is_error=False, and the dead-end miner keys on _tool_failure(result.payload, result.is_error), so that miner currently can't fire for Codex at all. Claude does is_error=bool(block.get("is_error")). Happy to take that as a follow-up if you'd rather keep this PR tight, just let me know which. Minor, all mechanical: ruff check reports 3 errors on the branch (unused datetime import, and I001 on both files), ruff format wants both files, and codex.py is still missing its trailing newline. ruff check --fix && ruff format clears the lot. |
|
Codex represents many operations through the generic exec/bash tool, with the actual operation embedded in the command string. The demand miner currently recognizes only search_codebase/get_answer. Should the Codex adapter classify shell commands (e.g. rg) into those logical tool names, or should the miners be extended to understand Codex's generic bash tool? |
|
Hi sir @RaghavChamadiya , I just wanted to ask, I believe the adapter should normalize Codex's generic exec tool into canonical tool names (e.g. search_codebase) because the miners only consume canonical names. My remaining question is what heuristic you'd prefer for distinguishing search commands from other shell commands. |
Summary
Related Issues
Test Plan
pytest)ruff check .)npm run build) (if frontend changes)Checklist