diff --git a/adapters/codex/install.yaml b/adapters/codex/install.yaml index 7ba68b2f..3e41ef45 100644 --- a/adapters/codex/install.yaml +++ b/adapters/codex/install.yaml @@ -3,8 +3,12 @@ # Codex reads `/.codex/config.toml` (also `~/.codex/config.toml` for # user-global). We ship the project-local form so `vouch install-mcp codex` # doesn't touch home-directory state -- see issue #179 scope decision. +# +# `.codex/config.toml` is codex's *primary* config file (model, approval +# policy, other MCP servers), so T1 deep-merges into an existing one instead +# of skipping it -- see issue #384. Existing user values always win. host: codex pretty: OpenAI Codex CLI tiers: T1: - - { src: config.toml, dst: .codex/config.toml } + - { src: config.toml, dst: .codex/config.toml, toml_merge: true } diff --git a/src/vouch/install_adapter.py b/src/vouch/install_adapter.py index ba4f0b1c..abc426af 100644 --- a/src/vouch/install_adapter.py +++ b/src/vouch/install_adapter.py @@ -12,6 +12,9 @@ inside a `` ... `` block (``InstallResult.appended``). If the fence already exists, the file is treated as skipped -- so reruns of ``vouch install-mcp`` stay flat-noop. +* **settings.json with ``json_merge`` / config.toml with ``toml_merge``** -> + an existing destination is deep-merged into instead of skipped + (``InstallResult.merged``); the user's existing values always win. Tiers stack from T1 (the minimum: MCP wire) through T4 (full integration: slash commands and host-side hooks). Each manifest declares only the tiers @@ -27,8 +30,11 @@ from __future__ import annotations +import datetime import json +import re import shutil +import tomllib from dataclasses import dataclass, field from pathlib import Path from typing import Any @@ -75,6 +81,7 @@ class _FileEntry: dst: str # path relative to the target directory fenced_append: bool = False # CLAUDE.md-style: append inside our fence json_merge: bool = False # settings.json-style: deep-merge into existing + toml_merge: bool = False # config.toml-style: deep-merge into existing @dataclass(frozen=True) @@ -135,10 +142,32 @@ def _load_manifest(host: str) -> _Manifest: raise AdapterError( f"{host}: install.yaml tier {tier_name}: every entry needs a non-empty `dst`" ) - fenced = bool(raw.get("fenced_append", False)) - json_merge = bool(raw.get("json_merge", False)) + def _flag(name: str, raw: Any = raw, tier_name: str = tier_name) -> bool: + # Require an actual YAML boolean. `bool(raw.get(...))` would + # coerce a mistakenly-quoted `toml_merge: "false"` (a + # non-empty string) to True and silently enable a merge + # strategy, so reject anything that isn't a real bool. + val = raw.get(name, False) + if not isinstance(val, bool): + raise AdapterError( + f"{host}: install.yaml tier {tier_name}: `{name}` must be " + f"a boolean, got {type(val).__name__} ({val!r})" + ) + return val + + fenced = _flag("fenced_append") + json_merge = _flag("json_merge") + toml_merge = _flag("toml_merge") + if fenced + json_merge + toml_merge > 1: + raise AdapterError( + f"{host}: install.yaml tier {tier_name}: entry sets more than " + f"one of fenced_append/json_merge/toml_merge; pick one strategy" + ) parsed_entries.append( - _FileEntry(src=src, dst=dst, fenced_append=fenced, json_merge=json_merge) + _FileEntry( + src=src, dst=dst, fenced_append=fenced, + json_merge=json_merge, toml_merge=toml_merge, + ) ) if parsed_entries: parsed[tier_name] = parsed_entries @@ -224,6 +253,10 @@ def install(adapter: str, *, target: Path, tier: str = "T4") -> InstallResult: _install_json_merge(src, dst, result, entry.dst) continue + if entry.toml_merge: + _install_toml_merge(src, dst, result, entry.dst) + continue + if dst.exists(): result.skipped.append(entry.dst) continue @@ -389,3 +422,144 @@ def _install_json_merge( result.merged.append(rel_dst) else: result.skipped.append(rel_dst) + + +def _merge_toml(src: dict[str, Any], dst: dict[str, Any]) -> bool: + """Recursively add ``src`` keys missing from ``dst`` in place. Returns + True if ``dst`` changed. Same never-clobber convention as + :func:`_merge_settings`: on any conflict — a key present on both sides + with non-table values, or with mismatched types — the user's existing + ``dst`` value wins and only genuinely missing nested keys are filled in. + Idempotent: re-merging the same ``src`` reports no change. + """ + changed = False + for key, src_val in src.items(): + if key not in dst: + dst[key] = src_val + changed = True + continue + dst_val = dst[key] + if ( + isinstance(src_val, dict) + and isinstance(dst_val, dict) + and _merge_toml(src_val, dst_val) + ): + changed = True + return changed + + +_BARE_TOML_KEY = re.compile(r"[A-Za-z0-9_-]+") + + +def _toml_key(key: str) -> str: + if _BARE_TOML_KEY.fullmatch(key): + return key + # TOML basic strings share JSON's escape rules, so json.dumps is a + # valid quoted-key serializer. + return json.dumps(key) + + +def _toml_inline(value: Any) -> str: + if isinstance(value, bool): + return "true" if value else "false" + if isinstance(value, int): + return str(value) + if isinstance(value, float): + if value != value or value in (float("inf"), float("-inf")): + raise ValueError(f"non-finite float not serialized: {value!r}") + return repr(value) + if isinstance(value, str): + return json.dumps(value) + if isinstance(value, (datetime.datetime, datetime.date, datetime.time)): + return value.isoformat() + if isinstance(value, list): + return "[" + ", ".join(_toml_inline(v) for v in value) + "]" + if isinstance(value, dict): + pairs = ", ".join( + f"{_toml_key(str(k))} = {_toml_inline(v)}" for k, v in value.items() + ) + return "{" + pairs + "}" + raise ValueError(f"unsupported TOML value type: {type(value).__name__}") + + +def _emit_toml_table( + table: dict[str, Any], path: list[str], lines: list[str] +) -> None: + plain = [(k, v) for k, v in table.items() if not isinstance(v, dict)] + subs = [(k, v) for k, v in table.items() if isinstance(v, dict)] + # A header is only needed for the table's own keys, or to make an empty + # table exist at all; sub-table headers imply their parents. + if path and (plain or not subs): + if lines: + lines.append("") + lines.append("[" + ".".join(_toml_key(p) for p in path) + "]") + for key, value in plain: + lines.append(f"{_toml_key(str(key))} = {_toml_inline(value)}") + for key, value in subs: + _emit_toml_table(value, [*path, str(key)], lines) + + +def _toml_dumps(data: dict[str, Any]) -> str: + """Serialize the merged config back to TOML. + + Deliberately minimal — covers the shapes tomllib can produce from the + configs we merge into (tables, arrays, inline tables inside arrays, + scalars, datetimes), not the whole spec. Lists containing tables are + emitted as arrays of inline tables rather than ``[[table]]`` blocks. + Raises ValueError on anything it can't faithfully re-emit; the caller + treats that as "leave the user's file alone". + """ + lines: list[str] = [] + _emit_toml_table(data, [], lines) + return "\n".join(lines) + "\n" if lines else "" + + +def _install_toml_merge( + src: Path, dst: Path, result: InstallResult, rel_dst: str +) -> None: + """config.toml-style: deep-merge our tables into a pre-existing TOML + file instead of skipping it. ``.codex/config.toml`` is codex's primary + config file, so a plain copy-or-skip would leave vouch unwired on any + project where codex is already configured (vouchdev/vouch#384). + + States mirror :func:`_install_json_merge`: + + * dst missing -> copy fresh (``written``) + * dst exists, merge adds keys -> merge + rewrite (``merged``) + * dst exists, nothing to add -> skip (``skipped``); already installed + * dst exists, unparseable -> skip (``skipped``); never clobber the user + + Rewriting re-serializes the whole file (comments and formatting are not + preserved — same trade-off ``_install_json_merge`` already makes). The + serialized result must survive a tomllib round-trip back to the merged + data; anything the minimal serializer can't faithfully re-emit degrades + to ``skipped`` rather than risking the user's config. + """ + if not dst.exists(): + dst.parent.mkdir(parents=True, exist_ok=True) + shutil.copy2(src, dst) + result.written.append(rel_dst) + return + + try: + dst_data = tomllib.loads(dst.read_text(encoding="utf-8")) + src_data = tomllib.loads(src.read_text(encoding="utf-8")) + except (OSError, UnicodeDecodeError, tomllib.TOMLDecodeError): + # Malformed or unreadable user file — leave it untouched. + result.skipped.append(rel_dst) + return + + if not _merge_toml(src_data, dst_data): + result.skipped.append(rel_dst) + return + + try: + text = _toml_dumps(dst_data) + if tomllib.loads(text) != dst_data: + raise ValueError("serializer round-trip mismatch") + except (ValueError, tomllib.TOMLDecodeError): + result.skipped.append(rel_dst) + return + + dst.write_text(text, encoding="utf-8") + result.merged.append(rel_dst) diff --git a/src/vouch/jsonl_server.py b/src/vouch/jsonl_server.py index 8aea8cab..97b4753c 100644 --- a/src/vouch/jsonl_server.py +++ b/src/vouch/jsonl_server.py @@ -40,8 +40,6 @@ from .capabilities import capabilities as build_caps from .context import build_context_pack from .logging_config import configure_logging - -_log = logging.getLogger("vouch.jsonl_server") from .models import ProposalStatus from .page_filters import filter_pages from .proposals import ( @@ -65,6 +63,8 @@ ) from .synthesize import synthesize +_log = logging.getLogger("vouch.jsonl_server") + # Per-request actor override. The HTTP transport sets this from the # X-Vouch-Agent header so audit attribution is correct without mutating # process-wide env (each ThreadingHTTPServer request thread gets its own diff --git a/tests/test_install_adapter.py b/tests/test_install_adapter.py index 83036e8c..1a11d283 100644 --- a/tests/test_install_adapter.py +++ b/tests/test_install_adapter.py @@ -319,6 +319,189 @@ def test_install_openclaw_is_idempotent(tmp_path: Path) -> None: } +# --- codex: config.toml deep-merge (vouchdev/vouch#384) --------------------- + + +def test_codex_toml_merges_into_existing_config(tmp_path: Path) -> None: + """User already has .codex/config.toml — codex's *primary* config file. + The old plain-copy path silently skipped it, so vouch never got wired on + any project where codex was already configured. toml_merge adds + [mcp_servers.vouch] while preserving every unrelated table and value.""" + import tomllib + + codex_dir = tmp_path / ".codex" + codex_dir.mkdir() + (codex_dir / "config.toml").write_text( + 'model = "gpt-5"\napproval_policy = "never"\n\n' + '[mcp_servers.other]\ncommand = "other-server"\nargs = ["--fast"]\n', + encoding="utf-8", + ) + result = install("codex", target=tmp_path, tier="T1") + data = tomllib.loads((codex_dir / "config.toml").read_text(encoding="utf-8")) + + # user content preserved + assert data["model"] == "gpt-5" + assert data["approval_policy"] == "never" + assert data["mcp_servers"]["other"]["command"] == "other-server" + assert data["mcp_servers"]["other"]["args"] == ["--fast"] + + # vouch content merged in + assert data["mcp_servers"]["vouch"]["command"] == "vouch" + assert data["mcp_servers"]["vouch"]["args"] == ["serve"] + assert data["mcp_servers"]["vouch"]["env"]["VOUCH_AGENT"] == "codex" + + assert ".codex/config.toml" in result.merged + assert ".codex/config.toml" not in result.skipped + assert ".codex/config.toml" not in result.written + + +def test_codex_toml_merge_is_idempotent(tmp_path: Path) -> None: + codex_dir = tmp_path / ".codex" + codex_dir.mkdir() + (codex_dir / "config.toml").write_text('model = "gpt-5"\n', encoding="utf-8") + install("codex", target=tmp_path, tier="T1") + first = (codex_dir / "config.toml").read_text(encoding="utf-8") + second = install("codex", target=tmp_path, tier="T1") + after = (codex_dir / "config.toml").read_text(encoding="utf-8") + + assert first == after # no change on re-run + assert ".codex/config.toml" in second.skipped + assert ".codex/config.toml" not in second.merged + + +def test_codex_toml_fresh_install_writes_template(tmp_path: Path) -> None: + import tomllib + + result = install("codex", target=tmp_path, tier="T1") + cfg = tmp_path / ".codex" / "config.toml" + assert cfg.is_file() + data = tomllib.loads(cfg.read_text(encoding="utf-8")) + assert data["mcp_servers"]["vouch"]["command"] == "vouch" + assert ".codex/config.toml" in result.written + assert ".codex/config.toml" not in result.merged + + +def test_codex_toml_existing_vouch_entry_wins(tmp_path: Path) -> None: + """Conflict convention matches _install_json_merge: never clobber the + user. An existing [mcp_servers.vouch] value stays; only genuinely + missing keys (here the env table) are filled in.""" + import tomllib + + codex_dir = tmp_path / ".codex" + codex_dir.mkdir() + (codex_dir / "config.toml").write_text( + '[mcp_servers.vouch]\ncommand = "/opt/custom/vouch"\nargs = ["serve", "--debug"]\n', + encoding="utf-8", + ) + result = install("codex", target=tmp_path, tier="T1") + data = tomllib.loads((codex_dir / "config.toml").read_text(encoding="utf-8")) + + # the user's conflicting values win, deterministically + assert data["mcp_servers"]["vouch"]["command"] == "/opt/custom/vouch" + assert data["mcp_servers"]["vouch"]["args"] == ["serve", "--debug"] + # the missing env table is deep-merged in + assert data["mcp_servers"]["vouch"]["env"]["VOUCH_AGENT"] == "codex" + assert ".codex/config.toml" in result.merged + + +def test_codex_toml_malformed_existing_is_skipped(tmp_path: Path) -> None: + codex_dir = tmp_path / ".codex" + codex_dir.mkdir() + (codex_dir / "config.toml").write_text("= not valid toml [", encoding="utf-8") + before = (codex_dir / "config.toml").read_text(encoding="utf-8") + result = install("codex", target=tmp_path, tier="T1") + # unreadable user file is left untouched, not clobbered + assert (codex_dir / "config.toml").read_text(encoding="utf-8") == before + assert ".codex/config.toml" in result.skipped + assert ".codex/config.toml" not in result.merged + + +def test_toml_dumps_roundtrips_shipped_shapes() -> None: + """The hand-rolled serializer must faithfully re-emit everything tomllib + can hand it from the config shapes we merge into: nested tables, arrays, + inline tables inside arrays, quoted keys, and scalar types.""" + import tomllib + + from vouch.install_adapter import _toml_dumps + + data = { + "model": "gpt-5", + "temperature": 0.5, + "retries": 3, + "verbose": True, + "tags": ["a", "b"], + "weird key.name": "quoted", + "profiles": [{"name": "fast"}, {"name": "safe"}], + "mcp_servers": { + "vouch": { + "command": "vouch", + "args": ["serve"], + "env": {"VOUCH_AGENT": "codex"}, + }, + }, + } + assert tomllib.loads(_toml_dumps(data)) == data + + +def test_merge_toml_reports_no_change_when_subset() -> None: + from vouch.install_adapter import _merge_toml + + dst = {"a": {"b": 1, "c": [1, 2]}, "top": "x"} + src = {"a": {"b": 999}} # conflicting value: dst wins, nothing to add + assert _merge_toml(src, dst) is False + assert dst["a"]["b"] == 1 + + +def _write_manifest(tmp_path: Path, host: str, body: str, monkeypatch) -> None: + """Point the loader at a throwaway adapters dir holding one manifest.""" + import vouch.install_adapter as ia + + (tmp_path / host).mkdir(parents=True) + (tmp_path / host / "install.yaml").write_text(body, encoding="utf-8") + monkeypatch.setattr(ia, "ADAPTERS_DIR", tmp_path) + + +def test_manifest_non_boolean_flag_is_rejected(tmp_path: Path, monkeypatch) -> None: + """A quoted `"false"` is a non-empty string; bool() would read it as + True and silently enable a merge. The loader must reject it.""" + from vouch.install_adapter import _load_manifest + + _write_manifest(tmp_path, "badhost", ( + "host: badhost\n" + "tiers:\n" + " T1:\n" + ' - { src: a, dst: b, toml_merge: "false" }\n' + ), monkeypatch) + with pytest.raises(AdapterError, match="`toml_merge` must be a boolean"): + _load_manifest("badhost") + + +def test_manifest_multiple_strategies_rejected(tmp_path: Path, monkeypatch) -> None: + from vouch.install_adapter import _load_manifest + + _write_manifest(tmp_path, "badhost", ( + "host: badhost\n" + "tiers:\n" + " T1:\n" + " - { src: a, dst: b, json_merge: true, toml_merge: true }\n" + ), monkeypatch) + with pytest.raises(AdapterError, match="more than one of"): + _load_manifest("badhost") + + +def test_manifest_boolean_flags_still_accepted(tmp_path: Path, monkeypatch) -> None: + from vouch.install_adapter import _load_manifest + + _write_manifest(tmp_path, "okhost", ( + "host: okhost\n" + "tiers:\n" + " T1:\n" + " - { src: a, dst: b, toml_merge: true }\n" + ), monkeypatch) + manifest = _load_manifest("okhost") + assert manifest.tiers["T1"][0].toml_merge is True + + # --- error paths ----------------------------------------------------------