mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-07-21 16:18:55 +00:00
fix(codex): reconcile app-server bridge with #38835, gate commentary 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.
This commit is contained in:
Teknium
parent
68d5368f38
commit
60419dfb4b
3 changed files with 83 additions and 88 deletions
|
|
@ -16,6 +16,7 @@ compatibility.
|
|||
|
||||
from __future__ import annotations
|
||||
|
||||
import json
|
||||
import logging
|
||||
import os
|
||||
import time
|
||||
|
|
@ -25,61 +26,6 @@ from typing import Any, Callable, Dict, List
|
|||
logger = logging.getLogger(__name__)
|
||||
|
||||
|
||||
def _codex_note_to_tool_progress(note: dict) -> tuple[str, str, dict] | None:
|
||||
"""Map a Codex app-server ``item/started`` notification to a Hermes
|
||||
tool-progress event ``(tool_name, preview, args)``.
|
||||
|
||||
The Codex app-server runtime processes ``item/started`` notifications for
|
||||
command execution, file changes, and MCP/dynamic tool calls, but never
|
||||
surfaced them as Hermes tool-progress events — so gateways (Telegram, etc.)
|
||||
showed no verbose "running X" breadcrumbs on this route while every other
|
||||
provider did (#38835). Returns None for items that aren't tool-shaped.
|
||||
"""
|
||||
if not isinstance(note, dict) or note.get("method") != "item/started":
|
||||
return None
|
||||
params = note.get("params") or {}
|
||||
item = params.get("item") or {}
|
||||
if not isinstance(item, dict):
|
||||
return None
|
||||
|
||||
item_type = item.get("type") or ""
|
||||
if item_type == "commandExecution":
|
||||
command = item.get("command") or ""
|
||||
return "exec_command", command, {"command": command, "cwd": item.get("cwd") or ""}
|
||||
|
||||
if item_type == "fileChange":
|
||||
changes = item.get("changes") or []
|
||||
preview = "file changes"
|
||||
if isinstance(changes, list) and changes:
|
||||
paths = [
|
||||
str(change.get("path"))
|
||||
for change in changes
|
||||
if isinstance(change, dict) and change.get("path")
|
||||
]
|
||||
if paths:
|
||||
preview = ", ".join(paths[:3])
|
||||
if len(paths) > 3:
|
||||
preview += f", +{len(paths) - 3} more"
|
||||
return "apply_patch", preview, {"changes": changes}
|
||||
|
||||
if item_type == "mcpToolCall":
|
||||
server = item.get("server") or "mcp"
|
||||
tool = item.get("tool") or "unknown"
|
||||
args = item.get("arguments") or {}
|
||||
if not isinstance(args, dict):
|
||||
args = {"arguments": args}
|
||||
return f"mcp.{server}.{tool}", tool, args
|
||||
|
||||
if item_type == "dynamicToolCall":
|
||||
tool = item.get("tool") or "unknown"
|
||||
args = item.get("arguments") or {}
|
||||
if not isinstance(args, dict):
|
||||
args = {"arguments": args}
|
||||
return tool, tool, args
|
||||
|
||||
return None
|
||||
|
||||
|
||||
def _coerce_usage_int(value: Any) -> int:
|
||||
if isinstance(value, bool):
|
||||
return 0
|
||||
|
|
@ -558,6 +504,11 @@ def make_codex_app_server_event_bridge(agent) -> Callable[[dict], None]:
|
|||
text = item.get("text") or ""
|
||||
if not isinstance(text, str) or not text.strip():
|
||||
return
|
||||
# display.show_commentary=false — mid-turn narration stays off the
|
||||
# visible interim path on this runtime too (same contract as the
|
||||
# codex_responses commentary channel).
|
||||
if not getattr(agent, "show_commentary", True):
|
||||
return
|
||||
emit = getattr(agent, "_emit_interim_assistant_message", None)
|
||||
if emit is None:
|
||||
return
|
||||
|
|
|
|||
|
|
@ -426,6 +426,23 @@ class TestAgentMessageInterimDispatch:
|
|||
}))
|
||||
agent.tool_progress_callback.assert_not_called()
|
||||
|
||||
def test_show_commentary_off_suppresses_interim(self):
|
||||
"""display.show_commentary=false silences agentMessage interim
|
||||
delivery on the app-server runtime (same contract as the
|
||||
codex_responses commentary channel)."""
|
||||
agent = _make_stub_agent()
|
||||
agent.show_commentary = False
|
||||
bridge = make_codex_app_server_event_bridge(agent)
|
||||
bridge(_item_completed({
|
||||
"type": "agentMessage", "id": "am-5", "text": "I'll check config.",
|
||||
}))
|
||||
agent._emit_interim_assistant_message.assert_not_called()
|
||||
# Tool progress is unaffected by the commentary toggle.
|
||||
bridge(_item_started({
|
||||
"type": "commandExecution", "id": "cmd-1", "command": "ls",
|
||||
}))
|
||||
agent.tool_progress_callback.assert_called_once()
|
||||
|
||||
|
||||
class TestBridgeRobustness:
|
||||
def test_non_dict_notification_is_ignored(self):
|
||||
|
|
@ -544,6 +561,16 @@ class TestBridgeWiredInRuntime:
|
|||
valid_tool_names=set(),
|
||||
_sync_external_memory_for_turn=lambda **_: None,
|
||||
_spawn_background_review=lambda **_: None,
|
||||
# Usage accounting attrs read by _record_codex_app_server_usage.
|
||||
session_api_calls=0,
|
||||
session_prompt_tokens=0,
|
||||
session_completion_tokens=0,
|
||||
session_reasoning_tokens=0,
|
||||
session_cached_tokens=0,
|
||||
session_total_tokens=0,
|
||||
context_compressor=None,
|
||||
event_callback=None,
|
||||
_session_db=None,
|
||||
)
|
||||
|
||||
codex_runtime.run_codex_app_server_turn(
|
||||
|
|
|
|||
|
|
@ -12,6 +12,7 @@ Verifies that:
|
|||
|
||||
from __future__ import annotations
|
||||
|
||||
from types import SimpleNamespace
|
||||
from unittest.mock import MagicMock, patch
|
||||
|
||||
import pytest
|
||||
|
|
@ -694,48 +695,64 @@ class TestSessionRetirementOnRunAgent:
|
|||
|
||||
|
||||
class TestCodexToolProgressBridge:
|
||||
"""#38835: Codex app-server item/started notifications must surface as
|
||||
Hermes tool-progress so gateways show verbose breadcrumbs on this route."""
|
||||
"""#38835 / #33200: Codex app-server item notifications must surface as
|
||||
Hermes tool-progress so gateways show verbose breadcrumbs on this route.
|
||||
The original item/started-only mapper was superseded by the full event
|
||||
bridge (make_codex_app_server_event_bridge); these tests pin the same
|
||||
mapping contract against the bridge helpers."""
|
||||
|
||||
def test_mapper_command_execution(self):
|
||||
from agent.codex_runtime import _codex_note_to_tool_progress
|
||||
note = {"method": "item/started", "params": {"item": {
|
||||
"type": "commandExecution", "command": "ls -la", "cwd": "/tmp"}}}
|
||||
name, preview, args = _codex_note_to_tool_progress(note)
|
||||
assert name == "exec_command"
|
||||
assert preview == "ls -la"
|
||||
assert args == {"command": "ls -la", "cwd": "/tmp"}
|
||||
from agent.codex_runtime import (
|
||||
_codex_item_to_args,
|
||||
_codex_item_to_preview,
|
||||
_codex_item_to_tool_name,
|
||||
)
|
||||
item = {"type": "commandExecution", "command": "ls -la", "cwd": "/tmp"}
|
||||
assert _codex_item_to_tool_name(item) == "exec_command"
|
||||
assert _codex_item_to_preview(item) == "ls -la"
|
||||
assert _codex_item_to_args(item) == {"command": "ls -la", "cwd": "/tmp"}
|
||||
|
||||
def test_mapper_file_change(self):
|
||||
from agent.codex_runtime import _codex_note_to_tool_progress
|
||||
note = {"method": "item/started", "params": {"item": {
|
||||
from agent.codex_runtime import (
|
||||
_codex_item_to_preview,
|
||||
_codex_item_to_tool_name,
|
||||
)
|
||||
item = {
|
||||
"type": "fileChange",
|
||||
"changes": [{"path": "a.py"}, {"path": "b.py"}]}}}
|
||||
name, preview, args = _codex_note_to_tool_progress(note)
|
||||
assert name == "apply_patch"
|
||||
assert preview == "a.py, b.py"
|
||||
"changes": [{"path": "a.py"}, {"path": "b.py"}],
|
||||
}
|
||||
assert _codex_item_to_tool_name(item) == "apply_patch"
|
||||
assert _codex_item_to_preview(item) == "a.py, b.py"
|
||||
|
||||
def test_mapper_mcp_and_dynamic_tool_calls(self):
|
||||
from agent.codex_runtime import _codex_note_to_tool_progress
|
||||
mcp = {"method": "item/started", "params": {"item": {
|
||||
"type": "mcpToolCall", "server": "fs", "tool": "read", "arguments": {"p": 1}}}}
|
||||
name, preview, args = _codex_note_to_tool_progress(mcp)
|
||||
assert name == "mcp.fs.read"
|
||||
assert preview == "read"
|
||||
assert args == {"p": 1}
|
||||
from agent.codex_runtime import (
|
||||
_codex_item_to_args,
|
||||
_codex_item_to_tool_name,
|
||||
)
|
||||
mcp = {"type": "mcpToolCall", "server": "fs", "tool": "read", "arguments": {"p": 1}}
|
||||
assert _codex_item_to_tool_name(mcp) == "mcp.fs.read"
|
||||
assert _codex_item_to_args(mcp) == {"p": 1}
|
||||
|
||||
dyn = {"method": "item/started", "params": {"item": {
|
||||
"type": "dynamicToolCall", "tool": "web_search", "arguments": {"q": "x"}}}}
|
||||
assert _codex_note_to_tool_progress(dyn)[0] == "web_search"
|
||||
dyn = {"type": "dynamicToolCall", "tool": "web_search", "arguments": {"q": "x"}}
|
||||
assert _codex_item_to_tool_name(dyn) == "web_search"
|
||||
|
||||
def test_mapper_ignores_non_tool_items_and_other_methods(self):
|
||||
from agent.codex_runtime import _codex_note_to_tool_progress
|
||||
# agentMessage / reasoning items are not tool-shaped
|
||||
assert _codex_note_to_tool_progress({"method": "item/started", "params": {
|
||||
"item": {"type": "agentMessage", "text": "hi"}}}) is None
|
||||
# non-item/started methods
|
||||
assert _codex_note_to_tool_progress({"method": "item/completed", "params": {}}) is None
|
||||
assert _codex_note_to_tool_progress({}) is None
|
||||
def test_bridge_ignores_non_tool_items_and_other_methods(self):
|
||||
from agent.codex_runtime import make_codex_app_server_event_bridge
|
||||
events = []
|
||||
agent = SimpleNamespace(
|
||||
tool_progress_callback=lambda *a, **kw: events.append(a),
|
||||
_fire_stream_delta=None,
|
||||
_fire_reasoning_delta=None,
|
||||
_emit_interim_assistant_message=None,
|
||||
)
|
||||
on_event = make_codex_app_server_event_bridge(agent)
|
||||
# agentMessage started items are not tool-shaped
|
||||
on_event({"method": "item/started", "params": {
|
||||
"item": {"type": "agentMessage", "text": "hi"}}})
|
||||
# malformed / empty notes
|
||||
on_event({"method": "item/completed", "params": {}})
|
||||
on_event({})
|
||||
assert events == []
|
||||
|
||||
def test_session_wired_with_on_event_that_fires_tool_progress(self, monkeypatch):
|
||||
"""The session is constructed with an on_event hook that, when fed an
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue