mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-07-08 13:12:08 +00:00
fix: keep Codex commentary phase out of user-visible text
This commit is contained in:
parent
372c0b5f45
commit
ea125dd62e
3 changed files with 139 additions and 14 deletions
|
|
@ -1166,15 +1166,23 @@ def _normalize_codex_response(
|
|||
if item_type == "message":
|
||||
item_phase = getattr(item, "phase", None)
|
||||
normalized_phase = None
|
||||
is_commentary_phase = False
|
||||
if isinstance(item_phase, str):
|
||||
normalized_phase = item_phase.strip().lower()
|
||||
if normalized_phase in {"commentary", "analysis"}:
|
||||
saw_commentary_phase = True
|
||||
is_commentary_phase = True
|
||||
elif normalized_phase in {"final_answer", "final"}:
|
||||
saw_final_answer_phase = True
|
||||
message_text = _extract_responses_message_text(item)
|
||||
if message_text:
|
||||
content_parts.append(message_text)
|
||||
# Responses ``commentary``/``analysis`` phase text is scratch/tool
|
||||
# preamble for the model/provider protocol, not user-visible final
|
||||
# answer text. Preserve the exact message item for replay/cache
|
||||
# continuity, but do not expose it as assistant content where the
|
||||
# gateway/CLI/interim callbacks can leak it to the user.
|
||||
if not is_commentary_phase:
|
||||
content_parts.append(message_text)
|
||||
raw_message_item: Dict[str, Any] = {
|
||||
"type": "message",
|
||||
"role": "assistant",
|
||||
|
|
@ -1269,7 +1277,11 @@ def _normalize_codex_response(
|
|||
))
|
||||
|
||||
final_text = "\n".join([p for p in content_parts if p]).strip()
|
||||
if not final_text and hasattr(response, "output_text"):
|
||||
if (
|
||||
not final_text
|
||||
and hasattr(response, "output_text")
|
||||
and not (saw_commentary_phase and not saw_final_answer_phase)
|
||||
):
|
||||
out_text = getattr(response, "output_text", "")
|
||||
if isinstance(out_text, str):
|
||||
final_text = out_text.strip()
|
||||
|
|
|
|||
|
|
@ -500,6 +500,14 @@ def _event_field(event: Any, name: str, default: Any = None) -> Any:
|
|||
return value if value is not None else default
|
||||
|
||||
|
||||
def _item_field(item: Any, name: str, default: Any = None) -> Any:
|
||||
"""Field access for nested Response items (attr-style SDK object or dict)."""
|
||||
value = getattr(item, name, None)
|
||||
if value is None and isinstance(item, dict):
|
||||
value = item.get(name, default)
|
||||
return value if value is not None else default
|
||||
|
||||
|
||||
def _raise_stream_error(event: Any) -> None:
|
||||
"""Raise a ``_StreamErrorEvent`` from a ``type=error`` SSE frame.
|
||||
|
||||
|
|
@ -562,6 +570,7 @@ def _consume_codex_event_stream(
|
|||
collected_text_deltas: List[str] = []
|
||||
has_tool_calls = False
|
||||
first_delta_fired = False
|
||||
active_message_phase: str | None = None
|
||||
terminal_status: str = "completed"
|
||||
terminal_usage: Any = None
|
||||
terminal_response_id: str = None
|
||||
|
|
@ -595,9 +604,26 @@ def _consume_codex_event_stream(
|
|||
if event_type == "error":
|
||||
_raise_stream_error(event)
|
||||
|
||||
# Track the phase of the active streamed message item. Codex/Harmony
|
||||
# ``commentary``/``analysis`` text is protocol/tool preamble, not the
|
||||
# final answer. We still collect completed output items for replay, but
|
||||
# suppress those deltas from live user-visible callbacks.
|
||||
if event_type == "response.output_item.added":
|
||||
item = _event_field(event, "item")
|
||||
item_type = _item_field(item, "type", "")
|
||||
if item_type == "message":
|
||||
phase = _item_field(item, "phase", None)
|
||||
active_message_phase = phase.strip().lower() if isinstance(phase, str) else None
|
||||
else:
|
||||
active_message_phase = None
|
||||
if "function_call" in str(item_type):
|
||||
has_tool_calls = True
|
||||
continue
|
||||
|
||||
if "output_text.delta" in event_type or event_type == "response.output_text.delta":
|
||||
delta_text = _event_field(event, "delta", "")
|
||||
if delta_text:
|
||||
is_commentary_delta = active_message_phase in {"commentary", "analysis"}
|
||||
if delta_text and not is_commentary_delta:
|
||||
collected_text_deltas.append(delta_text)
|
||||
if not has_tool_calls:
|
||||
if not first_delta_fired:
|
||||
|
|
|
|||
|
|
@ -629,6 +629,71 @@ def test_run_codex_stream_returns_collected_items_when_stream_ends_without_termi
|
|||
assert response.output == [output_item]
|
||||
|
||||
|
||||
def test_consume_codex_stream_suppresses_commentary_phase_deltas(monkeypatch):
|
||||
from agent.codex_runtime import _consume_codex_event_stream
|
||||
|
||||
commentary_item = SimpleNamespace(
|
||||
type="message",
|
||||
phase="commentary",
|
||||
status="completed",
|
||||
content=[SimpleNamespace(type="output_text", text="I’ll call the tool now.")],
|
||||
)
|
||||
function_item = SimpleNamespace(
|
||||
type="function_call",
|
||||
id="fc_1",
|
||||
call_id="call_1",
|
||||
name="terminal",
|
||||
arguments="{}",
|
||||
)
|
||||
streamed = []
|
||||
|
||||
response = _consume_codex_event_stream(
|
||||
_FakeCreateStream([
|
||||
SimpleNamespace(type="response.created"),
|
||||
SimpleNamespace(
|
||||
type="response.output_item.added",
|
||||
item=SimpleNamespace(type="message", phase="commentary"),
|
||||
),
|
||||
SimpleNamespace(type="response.output_text.delta", delta="I’ll call the tool now."),
|
||||
SimpleNamespace(type="response.output_item.done", item=commentary_item),
|
||||
SimpleNamespace(
|
||||
type="response.output_item.added",
|
||||
item=SimpleNamespace(type="function_call"),
|
||||
),
|
||||
SimpleNamespace(type="response.output_item.done", item=function_item),
|
||||
SimpleNamespace(type="response.completed", response=SimpleNamespace(status="completed")),
|
||||
]),
|
||||
model="gpt-5-codex",
|
||||
on_text_delta=streamed.append,
|
||||
)
|
||||
|
||||
assert streamed == []
|
||||
assert response.output == [commentary_item, function_item]
|
||||
assert response.output_text == ""
|
||||
|
||||
|
||||
def test_consume_codex_stream_keeps_final_answer_phase_deltas(monkeypatch):
|
||||
from agent.codex_runtime import _consume_codex_event_stream
|
||||
|
||||
streamed = []
|
||||
response = _consume_codex_event_stream(
|
||||
_FakeCreateStream([
|
||||
SimpleNamespace(type="response.created"),
|
||||
SimpleNamespace(
|
||||
type="response.output_item.added",
|
||||
item=SimpleNamespace(type="message", phase="final_answer"),
|
||||
),
|
||||
SimpleNamespace(type="response.output_text.delta", delta="visible answer"),
|
||||
SimpleNamespace(type="response.completed", response=SimpleNamespace(status="completed")),
|
||||
]),
|
||||
model="gpt-5-codex",
|
||||
on_text_delta=streamed.append,
|
||||
)
|
||||
|
||||
assert streamed == ["visible answer"]
|
||||
assert response.output_text == "visible answer"
|
||||
|
||||
|
||||
def test_run_codex_stream_surfaces_failed_status_in_final_response(monkeypatch):
|
||||
"""A ``response.failed`` terminal event is reflected on the returned object."""
|
||||
agent = _build_agent(monkeypatch)
|
||||
|
|
@ -1156,7 +1221,7 @@ def test_run_conversation_codex_tool_round_trip(monkeypatch):
|
|||
responses = [_codex_tool_call_response(), _codex_message_response("done")]
|
||||
monkeypatch.setattr(agent, "_interruptible_api_call", lambda api_kwargs: responses.pop(0))
|
||||
|
||||
def _fake_execute_tool_calls(assistant_message, messages, effective_task_id):
|
||||
def _fake_execute_tool_calls(assistant_message, messages, effective_task_id, *_args):
|
||||
for call in assistant_message.tool_calls:
|
||||
messages.append(
|
||||
{
|
||||
|
|
@ -1347,7 +1412,7 @@ def test_run_conversation_codex_replay_payload_keeps_call_id(monkeypatch):
|
|||
|
||||
monkeypatch.setattr(agent, "_interruptible_api_call", _fake_api_call)
|
||||
|
||||
def _fake_execute_tool_calls(assistant_message, messages, effective_task_id):
|
||||
def _fake_execute_tool_calls(assistant_message, messages, effective_task_id, *_args):
|
||||
for call in assistant_message.tool_calls:
|
||||
messages.append(
|
||||
{
|
||||
|
|
@ -1382,7 +1447,7 @@ def test_run_conversation_codex_continues_after_incomplete_interim_message(monke
|
|||
]
|
||||
monkeypatch.setattr(agent, "_interruptible_api_call", lambda api_kwargs: responses.pop(0))
|
||||
|
||||
def _fake_execute_tool_calls(assistant_message, messages, effective_task_id):
|
||||
def _fake_execute_tool_calls(assistant_message, messages, effective_task_id, *_args):
|
||||
for call in assistant_message.tool_calls:
|
||||
messages.append(
|
||||
{
|
||||
|
|
@ -1569,9 +1634,25 @@ def test_normalize_codex_response_marks_commentary_only_message_as_incomplete(mo
|
|||
)
|
||||
|
||||
assert finish_reason == "incomplete"
|
||||
assert "inspect the repository" in (assistant_message.content or "")
|
||||
assert (assistant_message.content or "") == ""
|
||||
assert assistant_message.codex_message_items
|
||||
assert assistant_message.codex_message_items[0]["phase"] == "commentary"
|
||||
assert "inspect the repository" in assistant_message.codex_message_items[0]["content"][0]["text"]
|
||||
|
||||
|
||||
def test_normalize_codex_response_does_not_fallback_to_output_text_for_commentary_only(monkeypatch):
|
||||
agent = _build_agent(monkeypatch)
|
||||
from agent.codex_responses_adapter import _normalize_codex_response
|
||||
|
||||
response = _codex_commentary_message_response("I’ll call the tool now.")
|
||||
response.output_text = "I’ll call the tool now."
|
||||
|
||||
assistant_message, finish_reason = _normalize_codex_response(response)
|
||||
|
||||
assert finish_reason == "incomplete"
|
||||
assert (assistant_message.content or "") == ""
|
||||
assert assistant_message.codex_message_items[0]["phase"] == "commentary"
|
||||
|
||||
def test_normalize_codex_response_final_answer_overrides_top_level_incomplete(monkeypatch):
|
||||
from agent.codex_responses_adapter import _normalize_codex_response
|
||||
|
||||
|
|
@ -1974,7 +2055,7 @@ def test_run_conversation_codex_continues_after_commentary_phase_message(monkeyp
|
|||
]
|
||||
monkeypatch.setattr(agent, "_interruptible_api_call", lambda api_kwargs: responses.pop(0))
|
||||
|
||||
def _fake_execute_tool_calls(assistant_message, messages, effective_task_id):
|
||||
def _fake_execute_tool_calls(assistant_message, messages, effective_task_id, *_args):
|
||||
for call in assistant_message.tool_calls:
|
||||
messages.append(
|
||||
{
|
||||
|
|
@ -1990,11 +2071,17 @@ def test_run_conversation_codex_continues_after_commentary_phase_message(monkeyp
|
|||
|
||||
assert result["completed"] is True
|
||||
assert result["final_response"] == "Architecture summary complete."
|
||||
commentary_messages = [
|
||||
msg for msg in result["messages"]
|
||||
if msg.get("role") == "assistant" and msg.get("finish_reason") == "incomplete"
|
||||
]
|
||||
assert commentary_messages
|
||||
assert all((msg.get("content") or "") == "" for msg in commentary_messages)
|
||||
assert any(
|
||||
msg.get("role") == "assistant"
|
||||
and msg.get("finish_reason") == "incomplete"
|
||||
and "inspect the repo structure" in (msg.get("content") or "")
|
||||
for msg in result["messages"]
|
||||
"inspect the repo structure" in item["content"][0]["text"]
|
||||
for msg in commentary_messages
|
||||
for item in (msg.get("codex_message_items") or [])
|
||||
if item.get("phase") == "commentary"
|
||||
)
|
||||
assert any(msg.get("role") == "tool" and msg.get("tool_call_id") == "call_1" for msg in result["messages"])
|
||||
|
||||
|
|
@ -2010,7 +2097,7 @@ def test_run_conversation_codex_continues_after_ack_stop_message(monkeypatch):
|
|||
]
|
||||
monkeypatch.setattr(agent, "_interruptible_api_call", lambda api_kwargs: responses.pop(0))
|
||||
|
||||
def _fake_execute_tool_calls(assistant_message, messages, effective_task_id):
|
||||
def _fake_execute_tool_calls(assistant_message, messages, effective_task_id, *_args):
|
||||
for call in assistant_message.tool_calls:
|
||||
messages.append(
|
||||
{
|
||||
|
|
@ -2051,7 +2138,7 @@ def test_run_conversation_codex_continues_after_ack_for_directory_listing_prompt
|
|||
]
|
||||
monkeypatch.setattr(agent, "_interruptible_api_call", lambda api_kwargs: responses.pop(0))
|
||||
|
||||
def _fake_execute_tool_calls(assistant_message, messages, effective_task_id):
|
||||
def _fake_execute_tool_calls(assistant_message, messages, effective_task_id, *_args):
|
||||
for call in assistant_message.tool_calls:
|
||||
messages.append(
|
||||
{
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue