fix(codex): surface tool-progress + interim commentary on codex_app_server runtime (#33200) - #33294
fix(codex): surface tool-progress + interim commentary on codex_app_server runtime (#33200)#33294xxxigm wants to merge 3 commits into
Conversation
Adds ``make_codex_app_server_event_bridge(agent)`` plus four small mapping helpers (``_codex_item_to_tool_name`` / ``_codex_item_to_args`` / ``_codex_item_to_preview`` / ``_codex_item_completion_payload``) that translate codex JSON-RPC ``item/*`` notifications into the exact shape Hermes' gateway UI callbacks expect — tool names match ``CodexEventProjector`` so the progress bubbles and the projected ``tool_calls`` entries use the same identifiers. No behaviour change yet: the next commit wires the bridge into ``run_codex_app_server_turn`` (NousResearch#33200).
…time (NousResearch#33200) Pass ``on_event=make_codex_app_server_event_bridge(agent)`` when spawning the per-session ``CodexAppServerSession``. The session has always had a raw event hook but ``run_codex_app_server_turn`` never supplied one, so Discord / Telegram / TUI users saw nothing while codex was working — only the final answer landed. Now each ``item/started`` for a tool-shaped item fires ``tool_progress_callback("tool.started", ...)``, ``item/completed`` fires the matching ``"tool.completed"`` with duration + result, ``item/agentMessage/delta`` flows through ``_fire_stream_delta`` and each completed ``agentMessage`` surfaces through ``_emit_interim_assistant_message`` so the gateway's ``already_streamed`` dedupe keeps interim commentary in the channel without duplicating text the stream already showed.
…earch#33200) 42 tests across five suites: * ``TestCodexItemToToolName`` / ``TestCodexItemToArgs`` / ``TestCodexItemToPreview`` / ``TestCodexItemCompletionPayload`` — pin the per-type mapping so the synthetic tool name + args the UI sees match what ``CodexEventProjector`` writes into messages. * ``TestStreamDeltaDispatch`` / ``TestToolProgressDispatch`` / ``TestAgentMessageInterimDispatch`` — drive each Codex notification shape through the bridge and assert the right agent callback fires with the right arguments (including the duration / is_error / result kwargs the gateway renders). * ``TestBridgeRobustness`` — defensive paths: non-dict notifications, missing params, raising callbacks (must not tear down the codex turn loop), and agents without callbacks registered (cron / gateway-less contexts). * ``TestBridgeWiredInRuntime`` — integration guard that ``run_codex_app_server_turn`` actually constructs the session with ``on_event=<bridge>``, preventing a future refactor from silently regressing live progress visibility again.
|
Thanks for tracing the app-server event path and supplying focused callback coverage. The underlying issue is only partially superseded: current main now surfaces tool-start breadcrumbs, but it still lacks the completion and assistant/reasoning display paths proposed here. Problems
Suggested changes
Automated hermes-sweeper review. |
…on show_commentary Follow-ups on top of @xxxigm's salvaged bridge (#33294): - Remove the now-dead narrow item/started-only mapper from #38835 (_codex_note_to_tool_progress) — the full bridge supersedes it and keeps the same tool-name contract; its tests are repointed at the bridge helpers. - Preserve main's request_routing/approval-bypass wiring on the CodexAppServerSession constructor (landed after the PR was filed). - Gate agentMessage interim delivery on display.show_commentary so the app-server runtime honors the same toggle as the codex_responses commentary channel (tool progress is unaffected). - Add json import (bridge helpers use json.dumps) and modernize the wiring test's stub agent for main's usage-accounting attributes.
|
Merged via PR #66142 — your three commits were cherry-picked onto current main with your authorship preserved in git log (e840cca, 7b63c49, 68d5368). Worth noting: you submitted the complete bridge on May 27, before the narrower Thanks for the thorough work — the test coverage made this an easy salvage. |
… commentary on show_commentary Follow-ups on top of @xxxigm's salvaged bridge (NousResearch#33294): - Remove the now-dead narrow item/started-only mapper from NousResearch#38835 (_codex_note_to_tool_progress) — the full bridge supersedes it and keeps the same tool-name contract; its tests are repointed at the bridge helpers. - Preserve main's request_routing/approval-bypass wiring on the CodexAppServerSession constructor (landed after the PR was filed). - Gate agentMessage interim delivery on display.show_commentary so the app-server runtime honors the same toggle as the codex_responses commentary channel (tool progress is unaffected). - Add json import (bridge helpers use json.dumps) and modernize the wiring test's stub agent for main's usage-accounting attributes.
… commentary on show_commentary Follow-ups on top of @xxxigm's salvaged bridge (NousResearch#33294): - Remove the now-dead narrow item/started-only mapper from NousResearch#38835 (_codex_note_to_tool_progress) — the full bridge supersedes it and keeps the same tool-name contract; its tests are repointed at the bridge helpers. - Preserve main's request_routing/approval-bypass wiring on the CodexAppServerSession constructor (landed after the PR was filed). - Gate agentMessage interim delivery on display.show_commentary so the app-server runtime honors the same toggle as the codex_responses commentary channel (tool progress is unaffected). - Add json import (bridge helpers use json.dumps) and modernize the wiring test's stub agent for main's usage-accounting attributes.
What does this PR do?
Restores live tool-progress bubbles and interim assistant commentary on Discord / Telegram / TUI when the active provider runs on
openai_runtime: codex_app_server. Fixes the silent-channel UX described in #33200.The
codex_app_serverruntime hands the entire turn to a subprocess and short-circuits the normal Hermes tool loop, sotool_progress_callback,_fire_stream_deltaand_emit_interim_assistant_messagenever fire while codex is working — only the final answer lands.CodexAppServerSessionhas always exposed a rawon_eventhook, butrun_codex_app_server_turnsimply never supplied one.This PR ships the missing bridge in three small commits:
feat(codex)— mapping helpers + bridge factory. Four pure-dict helpers translate codexitem/*payloads into the Hermes-shape (tool name, args, preview, result+is_error), thenmake_codex_app_server_event_bridge(agent)wraps them into a singleon_event(note)callable. Tool names matchCodexEventProjectorso the progress bubble and the projectedtool_callsentry agree on the identifier.fix(codex)— wire the bridge into the runtime. Passon_event=make_codex_app_server_event_bridge(agent)when constructing the per-sessionCodexAppServerSession. ~7-line change, no other behaviour shift.test(codex)— 42 regression tests. Pin the mapping contract per type, the dispatch contract per Codex event, defensive paths (non-dict notifications, missing params, raising callbacks, agents without callbacks), and one integration guard that assertsrun_codex_app_server_turnactually wires the bridge — so a future refactor can't silently regress this again.Related Issue
Fixes #33200
Type of Change
Changes Made
agent/codex_runtime.py— adds_codex_item_to_tool_name/_codex_item_to_args/_codex_item_to_preview/_codex_item_completion_payloadplusmake_codex_app_server_event_bridge(agent), then wireson_event=intoCodexAppServerSessioninsiderun_codex_app_server_turn. Exports the factory in__all__.tests/agent/test_codex_app_server_event_bridge.py— 42 new tests acrossTestCodexItemToToolName,TestCodexItemToArgs,TestCodexItemToPreview,TestCodexItemCompletionPayload,TestStreamDeltaDispatch,TestToolProgressDispatch,TestAgentMessageInterimDispatch,TestBridgeRobustness, andTestBridgeWiredInRuntime.Translation map
item/startedforcommandExecution/fileChange/mcpToolCall/dynamicToolCalltool_progress_callback("tool.started", name, preview, args)item/completedfor sametool_progress_callback("tool.completed", name, None, None, duration=…, is_error=…, result=…)item/agentMessage/delta_fire_stream_delta(text)item/reasoning/delta_fire_reasoning_delta(text)item/completedforagentMessage_emit_interim_assistant_message({"role": "assistant", "content": text})_emit_interim_assistant_messagealready calls_interim_content_was_streamedto setalready_streamed=Truewhen the stream-delta path showed the same text, so adapters stay dedup-safe.Backwards compatible —
CodexAppServerSession(on_event=...)has always been an optional kwarg; tests and cron / non-interactive contexts that never set the agent callbacks see no change (the bridge is a no-op when the agent has no callbacks registered).How to Test
End-to-end behaviour after the fix, on a turn that runs a shell command:
Before the fix, the same turn produced one final message and no live signal of any kind.
Checklist
feat(codex):,fix(codex):,test(codex):)