Skip to content

fix(codex-runtime): pin hermes-tools MCP launch context - #32812

Open
dso2ng wants to merge 2 commits into
NousResearch:mainfrom
dso2ng:fix/codex-hermes-tools-mcp-launch
Open

fix(codex-runtime): pin hermes-tools MCP launch context#32812
dso2ng wants to merge 2 commits into
NousResearch:mainfrom
dso2ng:fix/codex-hermes-tools-mcp-launch

Conversation

@dso2ng

@dso2ng dso2ng commented May 26, 2026

Copy link
Copy Markdown
Contributor

Summary

  • pin the generated hermes-tools MCP server entry to the Hermes source root with cwd
  • prepend the source root to PYTHONPATH so python -m agent.transports.hermes_tools_mcp_server remains importable when Codex starts MCP servers from arbitrary project directories
  • prefer the checkout venv/.venv Python for the MCP subprocess, falling back to the current interpreter for packaged installs
  • add regression coverage for venv selection, cwd/PYTHONPATH pinning, and migrated TOML output

Why

Codex can launch configured MCP servers from a project working directory that is not the Hermes checkout. In that case the generated hermes-tools callback can fail before the MCP initialize response with ModuleNotFoundError: No module named 'agent', surfacing only as a generic MCP handshake failure.

Duplicate PR check

I searched open PRs for exact terms around hermes_tools_mcp_server, hermes-tools, PYTHONPATH, cwd, codex_runtime_plugin_migration, and the initialize-response startup failure. No open PR appears to fix this launch-context issue. Related but non-duplicate PRs include schema/tool-surface work (#26432, #31279), migration rerun behavior (#26532), and generic MCP startup/timeout handling (#29764, #29853, #24508).

Tests

  • /home/dso2ng/.hermes/hermes-agent/venv/bin/python -m pytest tests/hermes_cli/test_codex_runtime_plugin_migration.py -q (71 passed)
  • /home/dso2ng/.hermes/hermes-agent/venv/bin/python -m ruff check hermes_cli/codex_runtime_plugin_migration.py tests/hermes_cli/test_codex_runtime_plugin_migration.py

@alt-glitch alt-glitch added type/bug Something isn't working P3 Low — cosmetic, nice to have comp/cli CLI entry point, hermes_cli/, setup wizard codex labels May 26, 2026
@dso2ng
dso2ng force-pushed the fix/codex-hermes-tools-mcp-launch branch from a15f3f5 to c61df30 Compare May 29, 2026 11:18
@dso2ng

dso2ng commented May 29, 2026

Copy link
Copy Markdown
Contributor Author

Refresh update: I rebased this branch onto current origin/main and resolved the test-file conflict while preserving the new upstream migration-test coverage and this PR's hermes-tools MCP launch-context assertions.

Updated head: c61df3015

Targeted local verification on the updated head:

  • git diff --check origin/main...HEAD -> passed
  • python -m py_compile hermes_cli/codex_runtime_plugin_migration.py tests/hermes_cli/test_codex_runtime_plugin_migration.py -> passed
  • python -m pytest tests/hermes_cli/test_codex_runtime_plugin_migration.py -o 'addopts=' -q -> 71 passed
  • non-ASCII diff guard -> non_ascii_added_lines=0

GitHub Actions is still blocked at the fork workflow approval gate (action_required, zero jobs started). Once a maintainer approves/runs the workflows, I can follow up on any real CI failures.

@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 addressing a real launch-context gap: current main's hermes_cli/codex_runtime_plugin_migration.py:557-606 still emits a callback without a cwd and only propagates PYTHONPATH when it was already set.

Problems

  • hermes_cli/codex_runtime_plugin_migration.py:566 orders venv/bin/python3 before .venv/bin/python3. AGENTS.md:216 establishes .venv as the preferred checkout environment, and current main contains both executable paths. This can select a different environment than the documented one for the MCP callback.

Suggested changes

  • Put .venv candidates ahead of venv on POSIX and Windows, and cover the both-present case in tests/hermes_cli/test_codex_runtime_plugin_migration.py.
  • Update the generated-entry example at website/docs/user-guide/features/codex-app-server-runtime.md:354-359 to include cwd.

Automated hermes-sweeper review.

server has Hermes' optional deps (notably mcp), then fall back to the
current interpreter for packaged installs.
"""
candidates = [

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.

venv is selected before .venv, but AGENTS.md:216 defines .venv as the preferred checkout environment and current main has both. Please put .venv candidates first and add a both-present regression test so the callback does not select a different virtualenv than the documented runtime.

@dso2ng
dso2ng force-pushed the fix/codex-hermes-tools-mcp-launch branch from c61df30 to 9c20c53 Compare July 13, 2026 14:36
@dso2ng

dso2ng commented Jul 13, 2026

Copy link
Copy Markdown
Contributor Author

Thanks — addressed on current head 9c20c5359218f6135205dbc6b921639c70424921.

  • Reordered the MCP callback Python candidates so the documented checkout .venv wins over legacy venv whenever both are present, on POSIX and Windows paths.
  • Added a both-present regression fixture that asserts .venv/bin/python3 is selected.
  • Updated the generated hermes-tools MCP TOML example to include its cwd, matching the emitted callback entry.

Targeted verification:

python -m py_compile hermes_cli/codex_runtime_plugin_migration.py tests/hermes_cli/test_codex_runtime_plugin_migration.py
python -m pytest tests/hermes_cli/test_codex_runtime_plugin_migration.py -o 'addopts=' -q
72 passed in 1.61s

git diff --check origin/main...HEAD
non_ascii_added_lines=0

Please re-review when convenient.

@teknium1 teknium1 added sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-platform-windows Sweeper risk: may break or behave differently on native Windows sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users labels Jul 13, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

codex comp/cli CLI entry point, hermes_cli/, setup wizard P3 Low — cosmetic, nice to have sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-platform-windows Sweeper risk: may break or behave differently on native Windows type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants