refactor(codex): one shared constant for the hermes-tools MCP server name
The name of Hermes' MCP callback for the codex app-server runtime was spelled as a string literal in five places (the server itself, the runtime migration that writes `[mcp_servers.hermes-tools]`, the Kanban worker override launcher, the elicitation auto-accept handler, the display-name stripper and the switch report) and had already drifted once (#111707). Define it once in agent/transports/hermes_tools_mcp_server.py — the module that IS the server and whose module-level imports are stdlib only, so every higher layer (transports, agent/codex_runtime, hermes_cli) can import it without a cycle — and read it everywhere. Two invariant tests in tests/agent/transports/: the worker's `-c mcp_servers.<name>.env.*` overrides only ever target an entry the migration really writes to config.toml (red on the pre-fix base: `{'hermes-mcp'}`), and non-owned launches emit no override at all. Refs #111707
This commit is contained in:
@@ -14,6 +14,7 @@ from types import SimpleNamespace
|
||||
from typing import Any, Callable, Dict, List
|
||||
|
||||
from agent.stream_single_writer import claim_stream_writer, stream_writer_is_current
|
||||
from agent.transports.hermes_tools_mcp_server import HERMES_TOOLS_MCP_SERVER_NAME
|
||||
from agent.usage_anchor import set_usage_anchor
|
||||
|
||||
logger = logging.getLogger(__name__)
|
||||
@@ -195,7 +196,6 @@ def _record_codex_app_server_compaction(agent, turn, *, approx_tokens: int | Non
|
||||
_CODEX_TOOL_ITEM_TYPES = frozenset({"commandExecution", "fileChange", "mcpToolCall", "dynamicToolCall", "webSearch"})
|
||||
# Internal MCP server wrapping Hermes' native tools: its inner dispatch has no tool_progress_callback, so the
|
||||
# codex-level mcpToolCall IS the display event and the mcp.hermes-tools.* prefix is stripped (users see Hermes tools).
|
||||
_INTERNAL_MCP_SERVER = "hermes-tools"
|
||||
_STATIC_TOOL_NAMES = {"commandExecution": "exec_command", "fileChange": "apply_patch", "webSearch": "web_search"}
|
||||
_STABLE_ID_PREFIXES = {"commandExecution": "exec", "fileChange": "apply_patch"}
|
||||
_MCP_LIKE_ITEM_TYPES = {"mcpToolCall", "dynamicToolCall"}
|
||||
@@ -212,7 +212,7 @@ def _codex_item_to_tool_name(item: dict) -> str:
|
||||
item_type = item.get("type") or ""
|
||||
if item_type == "mcpToolCall":
|
||||
server, tool = item.get("server") or "mcp", item.get("tool") or "unknown"
|
||||
return tool if server == _INTERNAL_MCP_SERVER else f"mcp.{server}.{tool}"
|
||||
return tool if server == HERMES_TOOLS_MCP_SERVER_NAME else f"mcp.{server}.{tool}"
|
||||
if item_type == "dynamicToolCall":
|
||||
return item.get("tool") or "dynamic"
|
||||
return _STATIC_TOOL_NAMES.get(item_type) or item_type or "unknown"
|
||||
|
||||
@@ -18,6 +18,7 @@ from dataclasses import dataclass
|
||||
from typing import Any, Optional
|
||||
|
||||
from agent.deadline import kill_process_tree
|
||||
from agent.transports.hermes_tools_mcp_server import HERMES_TOOLS_MCP_SERVER_NAME
|
||||
from tools.environments.local import hermes_subprocess_env
|
||||
|
||||
MIN_CODEX_VERSION = (0, 125, 0)
|
||||
@@ -100,13 +101,14 @@ class CodexAppServerClient:
|
||||
)
|
||||
# Native shell children remain unowned. Only Hermes' managed MCP tool
|
||||
# endpoint acts for this worker; grant it scope via its existing per-server
|
||||
# environment, never by granting the whole executor process ownership.
|
||||
# environment (the entry the runtime migration registers), never by granting
|
||||
# the whole executor process ownership.
|
||||
owned_task = os.environ.get("HERMES_KANBAN_TASK") and is_dispatcher_owned_worker_context()
|
||||
if owned_task:
|
||||
for key in (*KANBAN_ENV_KEYS, "HERMES_KANBAN_DB", "HERMES_KANBAN_BOARD"):
|
||||
if key in os.environ:
|
||||
cmd += ["-c", f"mcp_servers.hermes-tools.env.{key}={json.dumps(os.environ[key])}"]
|
||||
cmd += ["-c", f'mcp_servers.hermes-tools.env.{DELEGATED_CHILD_ENV_MARKER}=""']
|
||||
cmd += ["-c", f"mcp_servers.{HERMES_TOOLS_MCP_SERVER_NAME}.env.{key}={json.dumps(os.environ[key])}"]
|
||||
cmd += ["-c", f'mcp_servers.{HERMES_TOOLS_MCP_SERVER_NAME}.env.{DELEGATED_CHILD_ENV_MARKER}=""']
|
||||
spawn_env = delegated_child_subprocess_env(spawn_env)
|
||||
# Kanban workers must write handoff/status to the board DB outside the
|
||||
# workspace: keep the sandbox on, add the Kanban root as writable.
|
||||
|
||||
@@ -21,6 +21,7 @@ from agent.codex_responses_adapter import _format_responses_error
|
||||
from agent.redact import redact_sensitive_text
|
||||
from agent.transports.codex_app_server import CodexAppServerClient, CodexAppServerError
|
||||
from agent.transports.codex_event_projector import CodexEventProjector, ProjectionResult
|
||||
from agent.transports.hermes_tools_mcp_server import HERMES_TOOLS_MCP_SERVER_NAME
|
||||
|
||||
logger = logging.getLogger(__name__)
|
||||
|
||||
@@ -569,7 +570,7 @@ class CodexAppServerSession:
|
||||
def _respond_elicitation(self, params: dict) -> dict:
|
||||
"""MCP elicitation: auto-accept our own hermes-tools server (opted in by enabling the runtime;
|
||||
exposes nothing codex's shell can't do); decline others so the user opts in via codex's own flow."""
|
||||
action = "accept" if (params.get("serverName") or "") == "hermes-tools" else "decline"
|
||||
action = "accept" if (params.get("serverName") or "") == HERMES_TOOLS_MCP_SERVER_NAME else "decline"
|
||||
return {"action": action, "content": None, "_meta": None}
|
||||
|
||||
_SERVER_REQUEST_HANDLERS: dict[str, Callable[..., dict]] = {
|
||||
|
||||
@@ -16,6 +16,12 @@ from typing import Any, Optional
|
||||
|
||||
logger = logging.getLogger(__name__)
|
||||
|
||||
# The ``[mcp_servers.<name>]`` key under which the runtime migration registers this server. Every
|
||||
# codex-side reference to it (worker ``-c mcp_servers.<name>.env.*`` overrides, elicitation
|
||||
# auto-accept, display-name stripping) must use this constant: a drifted name materialises a
|
||||
# second env-only entry that codex rejects at bootstrap ("invalid transport").
|
||||
HERMES_TOOLS_MCP_SERVER_NAME = "hermes-tools"
|
||||
|
||||
# JSON Schema type -> Python type mapping for signature generation
|
||||
_JSON_TO_PY = {"string": str, "integer": int, "number": float, "boolean": bool, "array": list, "object": dict}
|
||||
|
||||
@@ -63,7 +69,7 @@ def _build_server() -> Any:
|
||||
from model_tools import get_tool_definitions, handle_function_call
|
||||
|
||||
mcp = MCPServer(
|
||||
"hermes-tools",
|
||||
HERMES_TOOLS_MCP_SERVER_NAME,
|
||||
instructions=(
|
||||
"Hermes Agent's tool surface, exposed for use inside a Codex "
|
||||
"session. Use these for capabilities Codex's built-in toolset "
|
||||
|
||||
@@ -9,6 +9,8 @@ from dataclasses import dataclass, field
|
||||
from pathlib import Path
|
||||
from typing import Any, Optional
|
||||
|
||||
from agent.transports.hermes_tools_mcp_server import HERMES_TOOLS_MCP_SERVER_NAME
|
||||
|
||||
logger = logging.getLogger(__name__)
|
||||
|
||||
|
||||
@@ -425,9 +427,9 @@ def migrate(
|
||||
if default_permission_profile:
|
||||
report.wrote_permissions_default = default_permission_profile
|
||||
if expose_hermes_tools:
|
||||
translated["hermes-tools"] = _build_hermes_tools_mcp_entry()
|
||||
if "hermes-tools" not in report.migrated:
|
||||
report.migrated.append("hermes-tools")
|
||||
translated[HERMES_TOOLS_MCP_SERVER_NAME] = _build_hermes_tools_mcp_entry()
|
||||
if HERMES_TOOLS_MCP_SERVER_NAME not in report.migrated:
|
||||
report.migrated.append(HERMES_TOOLS_MCP_SERVER_NAME)
|
||||
managed_block = render_codex_toml_section(
|
||||
translated, plugins=plugins, default_permission_profile=default_permission_profile)
|
||||
new_text = managed_block
|
||||
|
||||
@@ -88,10 +88,10 @@ def _migration_lines(config: dict) -> list[str]:
|
||||
"""Run the ~/.codex/config.toml migration and describe it; failures are non-fatal."""
|
||||
lines: list[str] = []
|
||||
try:
|
||||
from hermes_cli.codex_runtime_plugin_migration import migrate
|
||||
from hermes_cli.codex_runtime_plugin_migration import HERMES_TOOLS_MCP_SERVER_NAME, migrate
|
||||
mig_report = migrate(config)
|
||||
# The hermes-tools callback is internal plumbing — surfaced separately below.
|
||||
user_servers = [s for s in mig_report.migrated if s != "hermes-tools"]
|
||||
user_servers = [s for s in mig_report.migrated if s != HERMES_TOOLS_MCP_SERVER_NAME]
|
||||
if user_servers:
|
||||
lines.append(f"Migrated {len(user_servers)} MCP server(s): {', '.join(user_servers)}")
|
||||
if mig_report.migrated_plugins:
|
||||
@@ -104,7 +104,7 @@ def _migration_lines(config: dict) -> list[str]:
|
||||
lines.append(
|
||||
f"Default sandbox: {mig_report.wrote_permissions_default} "
|
||||
f"(no approval prompt on every write)")
|
||||
if "hermes-tools" in mig_report.migrated:
|
||||
if HERMES_TOOLS_MCP_SERVER_NAME in mig_report.migrated:
|
||||
lines.extend(_HERMES_TOOLS_CALLBACK_NOTE)
|
||||
lines.append(f" (config: {mig_report.target_path})")
|
||||
for err in mig_report.errors:
|
||||
|
||||
87
tests/agent/transports/test_codex_worker_mcp_overrides.py
Normal file
87
tests/agent/transports/test_codex_worker_mcp_overrides.py
Normal file
@@ -0,0 +1,87 @@
|
||||
"""Kanban worker MCP overrides must target the server entry the runtime migration registers.
|
||||
|
||||
Regression for #111707: dispatcher-owned workers on the codex app-server runtime injected
|
||||
``mcp_servers.hermes-mcp.env.*`` while the migration writes ``[mcp_servers.hermes-tools]``;
|
||||
codex then saw an env-only entry with no transport and refused to start.
|
||||
"""
|
||||
|
||||
import subprocess
|
||||
import tomllib
|
||||
|
||||
import pytest
|
||||
|
||||
from agent.delegation_context import DELEGATED_CHILD_ENV_MARKER, KANBAN_ENV_KEYS, non_dispatcher_owned_context
|
||||
from agent.transports import codex_app_server as cas
|
||||
from hermes_cli.codex_runtime_plugin_migration import migrate
|
||||
|
||||
|
||||
class _RecordingPopen:
|
||||
commands: list[list[str]] = []
|
||||
|
||||
def __init__(self, cmd, *args, **kwargs):
|
||||
type(self).commands.append(list(cmd))
|
||||
self.stdin = self.stdout = self.stderr = None
|
||||
self.pid = 1
|
||||
self.returncode = None
|
||||
|
||||
def poll(self):
|
||||
return None
|
||||
|
||||
def terminate(self):
|
||||
pass
|
||||
|
||||
def wait(self, timeout=None):
|
||||
return 0
|
||||
|
||||
def kill(self):
|
||||
pass
|
||||
|
||||
|
||||
@pytest.fixture
|
||||
def launch(monkeypatch, tmp_path):
|
||||
"""Return ``launch(env) -> list[str]`` of the ``mcp_servers.*`` overrides in the worker argv."""
|
||||
_RecordingPopen.commands = []
|
||||
monkeypatch.setattr(subprocess, "Popen", _RecordingPopen)
|
||||
for key in (*KANBAN_ENV_KEYS, DELEGATED_CHILD_ENV_MARKER, "HERMES_KANBAN_DB", "HERMES_KANBAN_BOARD"):
|
||||
monkeypatch.delenv(key, raising=False)
|
||||
|
||||
def _launch(env: dict[str, str]) -> list[str]:
|
||||
with monkeypatch.context() as ctx:
|
||||
for key, value in env.items():
|
||||
ctx.setenv(key, value)
|
||||
client = cas.CodexAppServerClient(codex_bin="codex", codex_home=str(tmp_path / "codex"))
|
||||
client._closed = True
|
||||
cmd = _RecordingPopen.commands.pop()
|
||||
return [arg for arg in cmd if arg.startswith("mcp_servers.")]
|
||||
|
||||
return _launch
|
||||
|
||||
|
||||
def _migrated_server_names(tmp_path) -> set[str]:
|
||||
codex_home = tmp_path / "codex"
|
||||
report = migrate({"mcp_servers": {}}, codex_home=codex_home, discover_plugins=False, expose_hermes_tools=True)
|
||||
assert not report.errors
|
||||
return set(tomllib.loads((codex_home / "config.toml").read_text(encoding="utf-8"))["mcp_servers"])
|
||||
|
||||
|
||||
def test_worker_overrides_target_the_migrated_server(launch, tmp_path):
|
||||
"""Producer/consumer contract: every ``-c mcp_servers.<name>.env.*`` override the worker
|
||||
launcher emits names an entry the migration actually writes to config.toml."""
|
||||
overrides = launch({
|
||||
"HERMES_KANBAN_TASK": "11111111-1111-4111-8111-111111111111",
|
||||
"HERMES_KANBAN_RUN_ID": "42",
|
||||
"HERMES_KANBAN_DB": str(tmp_path / "board" / "kanban.db"),
|
||||
})
|
||||
assert overrides, "dispatcher-owned worker must scope the managed MCP endpoint"
|
||||
targeted = {arg.split(".env.", 1)[0].removeprefix("mcp_servers.") for arg in overrides}
|
||||
migrated = _migrated_server_names(tmp_path)
|
||||
assert targeted <= migrated, f"overrides target {targeted - migrated}, which codex has no transport for"
|
||||
assert any(arg.startswith(f"mcp_servers.{next(iter(targeted))}.env.HERMES_KANBAN_TASK=") for arg in overrides)
|
||||
|
||||
|
||||
def test_only_dispatcher_owned_workers_get_mcp_overrides(launch):
|
||||
"""An ordinary launch and a worker that does not own the dispatcher's task emit no
|
||||
``mcp_servers.*`` override at all (nothing to scope, nothing for codex to reject)."""
|
||||
assert launch({}) == []
|
||||
with non_dispatcher_owned_context():
|
||||
assert launch({"HERMES_KANBAN_TASK": "11111111-1111-4111-8111-111111111111"}) == []
|
||||
Reference in New Issue
Block a user