From 871b863bd66e841abcbb7cd71f8a0f0f80e14bda Mon Sep 17 00:00:00 2001 From: teknium1 <127238744+teknium1@users.noreply.github.com> Date: Fri, 18 Sep 2026 23:38:13 -0700 Subject: [PATCH] fix(codex-runtime): keep user-owned same-name MCP tables instead of duplicating them; validate TOML before writing Reshape the salvaged strip helper (#79182) into the preserve-user policy: a `[mcp_servers.]` table the user declares outside the managed block wins, and the Hermes projection for that name is skipped and reported under `preserved_user_servers` (summary + JSON). The docs promise that anything outside the block is the user's, so silently deleting their table (the adopt policy used for `[plugins.*]`, where plugin/list owns the namespace) would break that contract for a namespace that is explicitly shared. Either way the file no longer gets two identical table headers, which codex refused to load (#79023). The rendered text is parsed with tomllib before it replaces config.toml; an unparsable result is recorded in report.errors and the existing file is left alone (also applies to --dry-run so automation sees the failure). --- hermes_cli/codex_runtime_plugin_migration.py | 90 +++++++++----------- 1 file changed, 42 insertions(+), 48 deletions(-) diff --git a/hermes_cli/codex_runtime_plugin_migration.py b/hermes_cli/codex_runtime_plugin_migration.py index 2705612ade..42506e8412 100644 --- a/hermes_cli/codex_runtime_plugin_migration.py +++ b/hermes_cli/codex_runtime_plugin_migration.py @@ -5,6 +5,7 @@ from __future__ import annotations import logging import os +import tomllib from dataclasses import dataclass, field from pathlib import Path from typing import Any, Optional @@ -31,6 +32,7 @@ class MigrationReport: migrated_plugins: list[str] = field(default_factory=list) plugin_query_error: Optional[str] = None wrote_permissions_default: Optional[str] = None + preserved_user_servers: list[str] = field(default_factory=list) errors: list[str] = field(default_factory=list) written: bool = False dry_run: bool = False @@ -56,6 +58,10 @@ class MigrationReport: lines.append(f"Codex plugin discovery skipped: {self.plugin_query_error}") if self.wrote_permissions_default: lines.append(f"Wrote default_permissions = {self.wrote_permissions_default!r}") + if self.preserved_user_servers: + lines.append( + f"Kept {len(self.preserved_user_servers)} user-owned MCP server(s) already in " + f"config.toml (Hermes projection skipped): {', '.join(self.preserved_user_servers)}") lines.extend(f"⚠ {err}" for err in self.errors) return "\n".join(lines) @@ -236,48 +242,30 @@ def _strip_unmanaged_plugin_tables(toml_text: str) -> str: return "".join(out) -def _strip_unmanaged_mcp_tables( - toml_text: str, server_names: set[str] -) -> str: - """Remove ``[mcp_servers.]`` tables that live OUTSIDE the managed - block *and* whose name is about to be re-emitted by the migration. +def _unmanaged_mcp_server_names(toml_text: str) -> set[str]: + """Names of ``[mcp_servers.]`` tables the USER owns (text outside the managed block). - This prevents duplicate TOML table headers when a server name exists - both in the user's hand-edited codex config and in Hermes' mcp_servers - config. Without this strip, the migration writes the name inside the - managed block while the user-owned copy survives outside it — Codex's - strict TOML parser then refuses to load the file. - - Only names in *server_names* are stripped; user-owned - ``[mcp_servers.*]`` entries that Hermes does NOT know about are - preserved so manual additions are not lost. + Unlike ``[plugins.*]`` — where ``plugin/list`` is the source of truth and we own the + namespace — ``mcp_servers`` is shared: the docs promise that anything outside the managed + block is the user's. A Hermes server whose name is already declared by the user is therefore + NOT re-emitted (the user's table wins and is preserved verbatim); emitting both would be a + duplicate table header, which is invalid TOML that codex refuses to load (issue #79023). """ - if not server_names: - return toml_text - lines = toml_text.splitlines(keepends=True) - out: list[str] = [] - in_mcp_table = False - for line in lines: + names: set[str] = set() + for line in toml_text.splitlines(): stripped = line.lstrip() - if _looks_like_table_header(stripped): - # Check if this is a [mcp_servers.] header where - # is in the set of servers we're about to re-emit. - if stripped.startswith("[mcp_servers."): - # Extract the server name: [mcp_servers.foo] → foo - # Handle quoted keys: [mcp_servers."foo bar"] → foo bar - inner = stripped[1:stripped.index("]")] # mcp_servers.foo - name_part = inner[len("mcp_servers."):] - # Unquote if quoted - if name_part.startswith('"') and name_part.endswith('"'): - name_part = name_part[1:-1] - if name_part in server_names: - in_mcp_table = True - continue - in_mcp_table = False - if in_mcp_table: + if not _looks_like_table_header(stripped) or not stripped.startswith("[mcp_servers."): continue - out.append(line) - return "".join(out) + # ``[mcp_servers.foo]`` -> ``foo``; ``[mcp_servers."foo bar"]`` -> ``foo bar``. + # Sub-tables (``[mcp_servers.foo.env]``) resolve to their server name ``foo``. + name_part = stripped[1:stripped.index("]")][len("mcp_servers."):].strip() + if name_part.startswith('"'): + name_part = name_part[1:name_part.index('"', 1)] + else: + name_part = name_part.split(".", 1)[0] + if name_part: + names.add(name_part) + return names def _looks_like_table_header(stripped_line: str) -> bool: @@ -474,9 +462,7 @@ def migrate( 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 + without_managed = "" if target.exists(): try: existing = target.read_text(encoding="utf-8") @@ -486,13 +472,21 @@ def migrate( without_managed = _strip_existing_managed_block(existing) if plugin_query_succeeded: without_managed = _strip_unmanaged_plugin_tables(without_managed) - # Strip user-owned [mcp_servers.*] tables for names we're about to - # re-emit inside the managed block, preventing duplicate TOML headers. - if translated: - without_managed = _strip_unmanaged_mcp_tables( - without_managed, set(translated.keys()) - ) - new_text = _insert_managed_block_at_top_level(without_managed, managed_block) + # Preserve-user policy: a name the user already declares outside the managed block is + # theirs; skip our projection for it instead of emitting a duplicate table header. + for name in sorted(_unmanaged_mcp_server_names(without_managed) & set(translated)): + del translated[name] + report.migrated.remove(name) + report.preserved_user_servers.append(name) + managed_block = render_codex_toml_section( + translated, plugins=plugins, default_permission_profile=default_permission_profile) + new_text = _insert_managed_block_at_top_level(without_managed, managed_block) + try: + tomllib.loads(new_text) + except tomllib.TOMLDecodeError as exc: + # Never replace a loadable config.toml with one codex would refuse to start on. + report.errors.append(f"refusing to write {target}: rendered config is not valid TOML ({exc})") + return report if dry_run: return report try: