refactor(runtime): parse codex exec --json streams into agent events - #6924
Conversation
PR Summary by QodoParse Codex JSON streams into normalized runtime events
AI Description
Diagram
High-Level Assessment
Files changed (13)
|
|
🤖 Review · Commit: |
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
00b99b9 to
9867b2e
Compare
|
🤖 Review · Commit: |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
|
🤖 Review · Commit: |
86d89f6 to
d981fe6
Compare
|
🤖 Finished Review · ✅ Success · Started 7:19 PM UTC · Completed 7:35 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $8.14 |
|
Risk Assessment: moderate (2/5) DetailsModerate risk: large additive PR (13 files, 1935 lines) but all newly added files with no modifications to existing code, no protected paths, no dependency changes, no CI changes. The high change-size signal (blast=large) is offset by low scores across all other Tier 1 dimensions. All files are new so rollback is trivial. Tier 1 signals unchanged from prior assessment; score preserved at 2. Previous runRisk Assessment: moderate (2/5) DetailsModerate risk: large additive PR (13 files, 1935 lines) but all newly added files with no modifications to existing code, no protected paths, no dependency changes, no CI changes. The high change-size signal (blast=large) is offset by low scores across all other Tier 1 dimensions. All files are new so rollback is trivial. Score unchanged from prior assessment. Previous run (2)Risk Assessment: moderate (2/5) DetailsModerate risk: large additive PR (13 files, 1935 lines) but all newly added files with no modifications to existing code, no protected paths, no dependency changes, no CI changes. The high change-size signal (blast=large) is offset by low scores across all other Tier 1 dimensions. All files are new so rollback is trivial. Score unchanged from prior assessment. Previous run (3)Risk Assessment: moderate (2/5) DetailsModerate risk: large additive PR (13 files, 1917 lines) but all newly added files with no modifications to existing code, no protected paths, no dependency changes, no CI changes. All files are new so rollback is trivial. The security label on the linked issue elevates Tier 3 but the change is a well-scoped stream parser within a planned 5-PR stack. |
|
Looks good to me Previous runReviewFindingsLow
Next steps:
Previous run (2)ReviewFindingsLow
Next steps:
Previous run (3)ReviewFindingsMedium
Low
Next steps:
|
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
🤖 Review · Commit: |
e6146ba to
436ad37
Compare
|
🤖 Finished Review · ❌ Failure (validation failed after 2 iteration(s)) · Started 8:02 PM UTC · Completed 8:44 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high |
436ad37 to
a128b87
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
a128b87 to
28fd77c
Compare
Adds parseCodexStream, the JSONL parser for `codex exec --json`, mapping
codex's ThreadEvent stream onto the runtime-neutral AgentEvent vocabulary
so the codex runtime adapter can drive the same renderer, metrics and
exit-code override as Claude Code and pi.
New, self-contained files only (no existing file is touched), so this sits
under the image-pin PR without conflicting:
* internal/runtime/codex_progress.go — parseCodexStream, applyCodexMetrics,
isCodexStreamCapture, codexStreamVerdict / parseCodexTranscriptFile.
* internal/runtime/codex_progress_test.go — table-driven over every fixture.
* internal/runtime/testdata/codex/ — fixtures, regen.sh and a README
carrying the event struct list.
Mapping: command_execution -> Bash, file_change -> Write (add) / Edit
(update, delete) per change, mcp_tool_call -> mcp__<server>__<tool>,
web_search -> WebSearch, collab_tool_call -> Agent, agent_message -> text,
reasoning -> thinking, todo_list ignored. Summaries are redacted before they
are truncated, and only failed or declined calls surface their output.
Three behaviours were verified in codex's own event processor rather than
assumed from the struct definitions, and drive the verdict logic:
* turn.completed.usage is the thread's *cumulative* usage, not the turn
delta, so successive values replace each other instead of summing.
* an `error` item is a warning (config warning, deprecation notice, model
reroute) and a top-level `error` event is parked as last_critical_error
with the run still going — neither fails the run on its own.
* an interrupted turn emits no terminal event at all, and `codex exec` can
exit 0 regardless, so a stream with no terminal event is reported as an
incomplete failure.
basic_run.jsonl is a live capture from @openai/codex@0.152.1 on gpt-5.6-luna
with the working directory redacted; the rest are hand-authored to the
rust-v0.152.1 structs to cover shapes a happy path never produces.
TotalCostUSD and Model stay the runner's to fill: the stream carries neither.
Refs #6920
Assisted-by: Claude (implementation)
Signed-off-by: Wayne Sun <gsun@redhat.com>
…ring as failures
Codex and Grok review findings on the stream parser.
**The verdict could inherit a previous turn's outcome.** `terminal` recorded
the last terminal event but was never cleared, so turn.completed →
turn.started → EOF reported a run that died mid-second-turn as a clean
finish. A turn.started now reopens the outcome — the previous terminal event
described that turn, not this one — in both directions, and whichever
terminal event arrives last decides. A turn.failed also counts toward
NumTurns now: it consumed a prompt and did work. It carries no usage on the
wire (only turn.completed does), so the token counters keep the last
completed turn's snapshot.
`codex exec` shuts down after its first completed turn today, so this is not
reachable through the runner yet; it is through `codex exec resume` and
through anything that tees several turns into one file, which
ParseTranscriptFile will be handed.
**Warnings must not render as failures.** The renderer prints every
ErrorEvent with StepFail, so emitting one for a codex `error` item — which
the processor produces for config warnings, deprecation notices and model
reroutes — painted red failure lines across runs that succeeded. The same
went for a non-terminal top-level `error`, which the processor parks and
carries on from. Neither emits an AgentEvent now: AgentEvent has no
informational kind, and RetryEvent promises an attempt/limit/delay these do
not have. Both stay in output.jsonl, which is kept as a run artifact, and
the top-level message is still parked as the reported reason when the stream
then ends without a terminal event. turn.failed still emits an ErrorEvent —
that one is a real failure.
**Capture detection was a substring scan.** The claim that JSON escaping
made it safe held only for a marker quoted in text; one nested as a real key
in another envelope, {"payload":{"type":"turn.completed"}}, still matched.
Detection is structural now: a line is unmarshalled and its top-level type
checked against the ThreadEvent set. A bare top-level "error" is in the set
but does not decide on its own, being far too generic to identify a codex
stream. pi's isPiStreamCapture has the same shape of gap with far less
exposure; noted in a comment rather than changed here.
That corrected a test assertion written against the old scan: a real
item.updated line whose agent text quotes "turn.completed" *is* a codex
capture, and structural detection says so.
Smaller hardening: token deltas floor at zero, so a non-monotonic usage
snapshot cannot report negative tokens; a failed file_change that never
named a path still reports one failed tool call instead of nothing; MCP
server and tool names are redacted before the mcp__<server>__<tool> name is
built, since they reach CI annotations; regen.sh installs with
--ignore-scripts and replaces the working directory literally, so a temp
path holding regex metacharacters cannot corrupt the fixture; and the live
fixture's thread id is asserted by shape, so re-capturing it needs no test
edit.
Documented, not changed: ToolCalls counts one per changed path in a
file_change item, because a single apply_patch touching N files is N edits —
what Claude Code's per-file Edit/Write calls would count — so the metric
stays comparable across runtimes. Assistant text is passed through
unredacted, as on pi and Claude Code; the renderer sanitizes it for display.
Refs #6920
Assisted-by: Claude (implementation), Codex gpt-5.6-sol (review), Grok (review)
Signed-off-by: Wayne Sun <gsun@redhat.com>
…eline dropping Qodo review of the codex stream parser. Two counting bugs, both of which inflated reported usage. Codex nests its usage categories, following the OpenAI Responses API: input_tokens is the whole input *including* the cached and cache-write parts, and output_tokens is the whole output *including* reasoning. Claude Code and pi report the opposite — Anthropic's convention, where cache and reasoning are disjoint from input and output — and that is what RunMetrics means and what the renderer sums for its total. The parser passed codex's numbers through unchanged, so every cached and reasoning token was counted twice: the live fixture's 41,615 real tokens rendered as ~83,000. The arithmetic is visible in the fixture itself — 27,386 cached plus 13,925 cache-write against an input_tokens of 41,320, nine tokens of genuinely new input. codexUsage.counters() now subtracts the subsets, flooring at zero, and the test asserts the five normalized counters sum to 41,615. Second, emitTokensDelta floored a shrinking snapshot's delta at zero but still moved the baseline down to it, so the next increase was measured from the smaller value and counted again: 500 -> 300 -> 500 emitted 700 tokens for a thread that used 500. The snapshot is now a per-field high-water mark, which makes every delta non-negative by construction rather than by clamping, and the ResultEvent reports that mark rather than the last snapshot. The three-snapshot case is a test. Both are properties of the wire format rather than of any one run, so the testdata README records them next to the cumulative-usage note. Refs #6920 Assisted-by: Claude (implementation), Qodo (review) Signed-off-by: Wayne Sun <gsun@redhat.com>
…r runtimes Review bot findings on the codex stream parser, all three about consistency with the runtimes that came before it. The fixtures were `.jsonl` while testdata/pi and testdata/opencode use `.ndjson` for exactly the same thing. Renamed, with the references in the test, regen.sh and the README moved with them. The runtime's own tee'd artifact stays `output.jsonl` — that name is the runner's contract, not a fixture convention. The truncation markers were three ASCII dots; claude_progress.go and pi_progress.go both use the single U+2026 ellipsis. Switched, and the length assertions now expect one rune rather than three. The third is a display bug rather than a convention. A `failed` command_execution whose exit_code was present had its label *replaced* by "exit N", so an item that failed while reporting exit 0 rendered as "$ cmd (exit 0)" — indistinguishable from a success. The status is the finding and the code only qualifies it, so it now reads "failed (exit N)", with the exit-0-but-failed case in the table tests. Refs #6920 Assisted-by: Claude (implementation), fullsend review bot (review) Signed-off-by: Wayne Sun <gsun@redhat.com>
28fd77c to
1518000
Compare
|
🤖 Finished Review · ✅ Success · Started 11:20 PM UTC · Completed 11:35 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $4.49 |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
The block comment in codex_progress.go still called the live capture basic_run.jsonl after the fixtures moved to the .ndjson convention. Refs #6920 Assisted-by: Claude (implementation) Signed-off-by: Wayne Sun <gsun@redhat.com>
|
🤖 Finished Review · ✅ Success · Started 11:46 PM UTC · Completed 11:59 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $2.55 |
Outdated: the findings were addressed in later commits (fixture names, failed-command label, token accounting, stale comment) and every thread is resolved; dismissed so the stack can enter the merge queue.
fullsend-ai-review
Bot
added
the
ready-for-merge
|
🤖 Finished Retro · ✅ Success · Started 12:21 AM UTC · Completed 12:37 AM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $7.02 |
Retro: PR #6924 — Codex stream parserPR #6924 was a large additive PR (+1935 lines, 13 new files) implementing a Codex JSONL stream parser ( Key finding: correctness sub-agent dimension driftThe fullsend review bot's correctness sub-agent (opus-tier) drifted entirely into convention/style review on its first successful pass (run 33672393449, $8.14). Despite receiving explicit verification checklist items for the token-counting arithmetic ( Meanwhile, qodo-code-review found both critical bugs in the same code on its single pass:
Both bugs were fixed by the author before human review. Evidence for existing issues
Workflow summary
Proposals filed
|
2 participants
Summary
Parses the
codex exec --jsonJSONL stream (Codex CLI 0.152.1) into fullsendAgentEvents andRunMetrics, mirroringpi_progress.go. New files only.parseCodexStream(oneResultEventper call, likeparsePiStream),applyCodexMetrics, structural capture detection (isCodexStreamCapture),codexStreamVerdict/parseCodexTranscriptFilefor the tee'd capture, fixtures undertestdata/codex/(one real capture fromnpx @openai/codex@0.152.1plus hand-authored failed / error / malformed / truncated / multi-turn / unknown-type streams),regen.sh, README.turn.completed.usageis the thread's cumulative total (replace, never sum; deltas emitted per turn); the itemtypeis a sibling ofid(nodetailsobject); onlyturn.failedis fatal —erroritems and non-terminal top-levelerrors emit no event (they stay inoutput.jsonl); a turn with no terminal event is incomplete even when codex exits 0;turn.startedresets the terminal state so multi-turn tees are judged by their last turn.command_execution→Bash,file_changeadd →Write/ update|delete →Edit(one event per changed path),mcp_tool_call→mcp__<server>__<tool>,web_search→WebSearch,collab_tool_call→Agent.Review rounds
sol + Grok: HIGH (terminal state inherited across turns) and both MEDIUMs (failed turns counted zero turns; warnings rendered as failures) fixed in the second commit, plus structural detection, delta floor, regen hygiene.
Squash title:
refactor(runtime): parse codex exec --json streams into agent events(the first commit saysfeat; per COMMITS.md a parser is not user-recognisable capability).Part of a five-PR stack for #6920 (Codex as an agent runtime): A image pin → B stream parser → C OpenAI credential seeder → D runtime core (ADR 0099) → E enable + docs. Each PR is reviewable on its own diff; they merge bottom-up. Plan and verified Codex facts:
research/fullsend-codex-runtime-plan.mdin the ai-workspace-public research repo (to be linked once pushed).Refs #6920
Assisted-by: Claude (implementation and review orchestration), Codex gpt-5.6-sol (review), Grok 4.6 (review)