Skip to content

fix(agent): surface Codex commentary items as interim messages - #60453

Closed
davidrobertson wants to merge 2 commits into
NousResearch:mainfrom
davidrobertson:fix/codex-responses-interim-commentary
Closed

fix(agent): surface Codex commentary items as interim messages#60453
davidrobertson wants to merge 2 commits into
NousResearch:mainfrom
davidrobertson:fix/codex-responses-interim-commentary

Conversation

@davidrobertson

@davidrobertson davidrobertson commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

Summary

  • surface Codex Responses phase="commentary" message items through the existing interim assistant callback when top-level assistant content is empty
  • pass extracted structured commentary through the existing secret redactor before it reaches Telegram/Discord or another interim callback
  • keep phase="analysis" and phase="final_answer" hidden from interim delivery
  • add regression coverage for non-streaming gateway surfaces that rely on interim assistant messages before tool calls

Why

Codex Responses can store user-facing mid-turn narration in structured codex_message_items instead of top-level assistant content. _emit_interim_assistant_message() only inspected content, so non-streaming gateway surfaces could miss natural progress commentary before tool execution even though the callback path was configured.

This keeps the fix narrow: only explicit phase="commentary" output text is surfaced, while provider scratchpad/analysis remains hidden.

Verification

Fresh branch based on current origin/main (1c473bc6a):

  • python -m pytest tests/run_agent/test_run_agent_codex_responses.py -k 'interim_commentary or commentary_only or codex_commentary' -q -o 'addopts='6 passed, 80 deselected
  • python -m pytest tests/run_agent/test_run_agent_codex_responses.py -q -o 'addopts='87 passed
  • python -m pytest tests/agent/test_redact.py -q -o 'addopts='149 passed
  • python -m pytest tests/run_agent/test_provider_parity.py -k 'codex or commentary or message_items' -q -o 'addopts='38 passed, 55 deselected
  • python -m pytest tests/gateway -k 'commentary or interim or progress' -q -o 'addopts='119 passed, 1 skipped, 8800 deselected
  • python -m compileall -q run_agent.py tests/run_agent/test_run_agent_codex_responses.py gateway/run.py gateway/stream_consumer.py → exit 0
  • python -m ruff check run_agent.py tests/run_agent/test_run_agent_codex_responses.pyAll checks passed! (with existing invalid # noqa warning at run_agent.py:107)
  • git diff --check → exit 0

Current-main compatibility check (origin/main at 46e87b14f): both PR commits cherry-picked cleanly into a detached worktree, then:

  • Codex Responses run-agent suite: 88 passed
  • redaction suite: 149 passed
  • provider parity selection: 38 passed, 55 deselected
  • gateway commentary/interim/progress selection: 145 passed, 1 skipped, 9040 deselected
  • Ruff, compileall, and git diff --check: passed

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

Copy link
Copy Markdown
Collaborator

This was generated by AI during triage.

Related to #59831 and the merged #58868: this PR re-surfaces Codex phase=commentary as interim narration via run_agent.py's _emit_interim_assistant_message, while #59831 does the same via agent/codex_runtime.py's commentary routing — same goal, different code-site, so competing (not duplicate). Both restore commentary that the merged #58868 routed to the hidden reasoning channel. Canonical thread: #24933. Maintainer to reconcile direction.

@rungmc357

Copy link
Copy Markdown
Contributor

I independently reproduced this against current main with a Codex Responses tool-call turn whose top-level assistant content is empty and whose user-facing narration exists only in a phase="commentary" message item. This PR targets the correct seam: the existing interim-assistant callback is configured, but _emit_interim_assistant_message() sees no normal content and silently returns.

Focused local verification of the same behavior passed:

  • full Codex Responses run-agent suite: 87 passed
  • Telegram/Discord gateway progress suites: 66 passed
  • end-to-end regression: commentary item -> normalized assistant message -> interim callback before tool execution

One output-safety issue is worth fixing before merge: _extract_codex_interim_visible_text() currently sends text copied directly from codex_message_items. That bypasses the redact_sensitive_text() pass applied to ordinary assistant content in build_assistant_message(). A commentary item containing a credential-like value could therefore reach Telegram/Discord even though the equivalent normal assistant content would be redacted.

Suggested contract:

visible = "\n\n".join(parts).strip()
if visible:
    visible = self._strip_think_blocks(visible).strip()
    from agent.redact import redact_sensitive_text
    visible = redact_sensitive_text(visible)
return visible

And a regression test should construct the token dynamically so secret scanners do not rewrite the fixture:

secret = "sk-" + ("A" * 32)
# put `secret` in a phase="commentary" output_text item
agent._emit_interim_assistant_message(message)
assert secret not in observed[0]

The phase boundary in this PR is right: only commentary should be visible; analysis and final_answer must remain excluded from interim extraction.

@davidrobertson

Copy link
Copy Markdown
Contributor Author

Addressed in 38e901128.

Structured Codex phase="commentary" text now passes through redact_sensitive_text() after think-block stripping and before the interim callback. The regression constructs the credential dynamically so secret scanners do not rewrite the fixture, and proves the raw token cannot reach the callback.

Verification:

  • red/green regression: failed before the implementation change, passed afterward
  • Codex Responses run-agent suite: 87 passed
  • redaction suite: 149 passed
  • provider parity selection: 38 passed
  • gateway commentary/interim/progress selection: 119 passed, 1 skipped
  • Ruff, compileall, and git diff --check: passed

@alt-glitch alt-glitch added the platform/telegram Telegram bot adapter label Jul 14, 2026
@teknium1

Copy link
Copy Markdown
Contributor

Thanks for the focused fix. The premise is still present on current main: agent/codex_responses_adapter.py:1217-1240 keeps phase="commentary" out of assistant content while preserving codex_message_items, and run_agent.py:4747-4750 therefore returns without calling the existing interim callback for commentary-only turns.

The proposed extraction is appropriately narrow: it accepts only phase="commentary" / output_text items, leaves analysis and final-answer phases out of this delivery path, strips think blocks, and applies redact_sensitive_text() before callback delivery. agent/conversation_loop.py:4815-4816 invokes this callback before tool execution, matching the stated gateway use case.

No verified correctness issues found in the two-file diff. The related #59831 and #62396 propose alternative delivery lanes and remain a maintainer design-selection question rather than a defect in this patch.

Automated hermes-sweeper review.

@alt-glitch alt-glitch removed comp/gateway Gateway runner, session dispatch, delivery platform/telegram Telegram bot adapter labels Jul 15, 2026
@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
@100yenadmin

Copy link
Copy Markdown

Superseded by #65653, which preserves this PR’s two original commits and David Robertson’s authorship, then adds the lifecycle, retry/dedup, multi-item, fallback, compatibility, and first-class streaming fixes found during adversarial validation against current main. The replacement PR includes the full design rationale, diagrams, and 521 focused passing tests. Thank you for identifying the correct interim-assistant seam and adding the redaction fix here.

@alt-glitch alt-glitch added needs-decision Awaiting maintainer decision before any implementation and removed 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 labels Jul 16, 2026
@teknium1

Copy link
Copy Markdown
Contributor

Merged via PR #66115 — your two commits were cherry-picked onto current main with your authorship preserved in git log (136ade2, a15397d). @100yenadmin built the lifecycle/dedup hardening and the separate live-stream commentary channel on top in #65653, and we added a display.show_commentary toggle. Thanks for the original implementation!

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 needs-decision Awaiting maintainer decision before any implementation P2 Medium — degraded but workaround exists provider/openai OpenAI / Codex Responses API sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants