Skip to content

fix(codex): surface commentary as interim assistant prose - #59831

Closed
ruizanthony wants to merge 1 commit into
NousResearch:mainfrom
ruizanthony:fix/codex-commentary-interim
Closed

fix(codex): surface commentary as interim assistant prose#59831
ruizanthony wants to merge 1 commit into
NousResearch:mainfrom
ruizanthony:fix/codex-commentary-interim

Conversation

@ruizanthony

Copy link
Copy Markdown
Contributor

Summary

  • route Codex/Harmony commentary phase deltas to the interim_assistant channel as one assembled message
  • keep Codex/Harmony analysis phase deltas in the private reasoning path
  • keep commentary out of final assistant text / output_text, preserving the existing final-answer behavior

Why

Recent Codex runtime changes correctly stopped treating commentary as final assistant text, but routed both commentary and analysis through the reasoning callback. In WebUI and gateway surfaces where raw reasoning is hidden, that removed the useful high-level “why I’m doing this next” narration and left users with only tool cards or generic tool labels.

commentary is intended as user-facing mid-turn narration; analysis is private reasoning. This patch restores that distinction.

Tests

  • python3 -m py_compile agent/codex_runtime.py
  • python3 -m pytest tests/run_agent/test_run_agent_codex_responses.py -q

@alt-glitch alt-glitch added type/bug Something isn't working comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint comp/gateway Gateway runner, session dispatch, delivery platform/telegram Telegram bot adapter provider/openai OpenAI / Codex Responses API P2 Medium — degraded but workaround exists labels Jul 6, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

This was generated by AI during triage.

Related to recently-merged #58868 (the teknium1 salvage that closed the #24933 commentary-leak family) — this PR partially reverses its direction: #58868 routed both commentary and analysis to the hidden reasoning channel, whereas this splits them so commentary is re-surfaced as user-visible interim narration and only analysis stays private. Flagging for a maintainer to reconcile the intended commentary behavior.

@anthonytrance

Copy link
Copy Markdown

Confirmed against current origin/main at b9b463f3bd6517b76687d9b3c9dea1e62f01f9e1 and with a deterministic Responses stream fixture.

The scope is narrower than generic "text before tools" bugs:

  • Affected: the codex_responses path when the backend emits an assistant message item with phase: "commentary", and the UI has reasoning hidden but interim_assistant_messages enabled. Recent OpenAI Codex/Responses models are the main case.
  • Not affected by this root cause: Chat Completions models/providers, Responses turns that emit no commentary item, or surfaces deliberately displaying the reasoning callback. Those can have similar symptoms through other paths, such as [Bug]: Text content before tool calls is not delivered on Telegram (only visible in CLI) #6067, but not through this phase-routing bug.
  • Final-answer text and tool execution are unaffected. The regression is specifically that user-facing commentary is sent only to on_reasoning_delta, so gateways with show_reasoning: false discard it instead of using the existing interim-assistant rail.

This PR has the right architectural direction. Two robustness cases seem worth covering before merge:

  1. It assembles commentary only from response.output_text.delta. The Responses stream also provides finalized text in response.output_text.done.text, and the authoritative completed message in response.output_item.done.item.content. Using those as fallbacks prevents a completed commentary message from being lost when no delta chunks arrive or the stream is truncated after finalized text.
  2. Tracking commentary by item_id or output_index, rather than a single active phase buffer, avoids losing or misrouting text if output-item events are interleaved.

Suggested behavioral tests:

  • commentary deltas produce exactly one interim message and no reasoning/final-answer text
  • output_text.done with no preceding deltas still produces one interim message
  • output_item.done content with neither delta nor text-done events still produces one interim message
  • a later reasoning-summary delta still reaches on_reasoning_delta
  • two output items identified by item_id/output_index cannot overwrite each other's state

I validated the fallback behavior locally with six focused compatibility tests, plus the existing Hermes commentary/final-answer parser tests. This should make the upstream fix reliable without coupling it to Telegram streaming or tool-progress settings.

@ruizanthony
ruizanthony force-pushed the fix/codex-commentary-interim branch from 74210c6 to d8165ce Compare July 13, 2026 13:48

@teknium1 teknium1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for separating commentary from private analysis. The current-main premise is confirmed: agent/codex_runtime.py:721-729 routes both phases to reasoning, while gateway reasoning is hidden by default.

Problems

  • agent/codex_runtime.py:1024-1025 sends raw commentary to the interim callback. That callback only strips think blocks (run_agent.py:4747-4753); normal assistant content is redacted at agent/chat_completion_helpers.py:1138-1147. Please redact this new visible path and add a dynamic credential-token regression.
  • Retry de-duplication keys include item_id (agent/codex_runtime.py:765-770 in this PR), but the test repeats msg_commentary in both attempts (tests/run_agent/test_run_agent_codex_responses.py:828,850). It does not cover a retry returning a different item id.

Suggested changes

  • Reuse the existing secret-redaction behavior before _emit_interim_assistant_message.
  • Define a retry-stable identity or add a changed-item-id retry fixture that proves exactly-once delivery without merging distinct commentary.

Automated hermes-sweeper review.

Comment thread agent/codex_runtime.py
agent._fire_reasoning_delta(text)

def _on_commentary_message(text: str) -> None:
agent._emit_interim_assistant_message({"role": "assistant", "content": text})

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This newly visible path bypasses the normal assistant-content redaction boundary: _emit_interim_assistant_message only strips think blocks before calling the gateway callback. Please redact text here (or in that helper) and add a dynamic sk-... fixture proving an interim callback cannot receive the raw token.

SimpleNamespace(
type="response.output_item.added",
output_index=0,
item=SimpleNamespace(type="message", id="msg_commentary", phase="commentary"),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The retry fixture repeats the first attempt's msg_commentary id. Because the implementation's emission key includes item_id, add a variant with a different second-attempt id to define whether retry de-duplication is intended to survive newly issued response-item identities.

@teknium1 teknium1 added sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 15, 2026
@teknium1

Copy link
Copy Markdown
Contributor

Closing — the commentary-channel work landed via PR #66115 (salvage of #65653, which built on #60453). Your PR was the earliest submission in this cluster and correctly identified the problem: commentary deserved a visible path separate from private reasoning. The merged implementation converged on the same commentary→interim routing with additional lifecycle/dedup hardening and a display.show_commentary config toggle. Credited you in the merge PR body — thanks for getting to this first.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint comp/gateway Gateway runner, session dispatch, delivery P2 Medium — degraded but workaround exists platform/telegram Telegram bot adapter provider/openai OpenAI / Codex Responses API sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants