feat(adapters): toml_merge install strategy for codex config.toml#392
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
Bot
added
adapters
There was a problem hiding this comment.
Pull request overview
Adds a TOML-merge install strategy so vouch install-mcp codex can wire vouch into an existing .codex/config.toml (Codex’s primary project config) instead of silently skipping when the file already exists. This mirrors the existing json_merge behavior used for .claude/settings.json, keeping installs idempotent and preserving user-owned values.
Changes:
- Introduces
toml_merge: truemanifest flag and installer path that deep-merges missing TOML keys usingtomllib, with “user wins” conflict semantics. - Implements a minimal TOML serializer with round-trip validation to avoid clobbering user config when serialization can’t be done safely.
- Updates the Codex T1 manifest to use
toml_mergeand adds tests covering merge behavior, conflicts, idempotence, malformed TOML, and serializer round-trips.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
src/vouch/install_adapter.py |
Adds toml_merge manifest support, TOML deep-merge logic, and TOML serialization + safe rewrite behavior. |
tests/test_install_adapter.py |
Adds focused tests for Codex TOML merging, idempotence, conflict handling, malformed TOML skip, and serializer round-trip. |
adapters/codex/install.yaml |
Switches Codex T1 install from copy/skip to toml_merge: true with updated rationale comments. |
| fenced = bool(raw.get("fenced_append", False)) | ||
| json_merge = bool(raw.get("json_merge", False)) | ||
| toml_merge = bool(raw.get("toml_merge", False)) |
vouchdev#361 inserted `_log = logging.getLogger(...)` between two import groups in jsonl_server.py. that statement-among-imports trips ruff's E402 on every import that follows it, so `ruff check src tests` — the lint gate in ci — now fails on the whole repo. move the logger definition below the imports (where storage.py already keeps its own module logger); no behaviour change.
.codex/config.toml is codex's primary config file, so the plain-copy path silently skipped any project where codex was already configured and vouch never got wired. add a toml_merge entry flag mirroring json_merge: parse the existing destination with tomllib, deep-merge the template's tables into it (existing user values always win on conflict), and write back. flip the codex T1 entry to toml_merge. writing uses a minimal hand-rolled serializer (tables, arrays, inline tables in arrays, scalars, datetimes) so the dependency set stays unchanged; the output must survive a tomllib round-trip back to the merged data, and anything the serializer can't faithfully re-emit degrades to skipped rather than risking the user's config. closes vouchdev#384
dripsmvcp
force-pushed
the
feat/codex-toml-merge
branch
from
9c6fd31 to
1a3f1c3
Compare
|
addressed the copilot review: manifest flag parsing now requires actual yaml booleans — a mistakenly-quoted also rebased this branch onto a small |
|
LGTM! |
.codex/config.tomlis codex's primary config file — model, approval policy, and any other mcp servers all live there. the installer's plain-copy path silently skips an existing destination, so on any project where codex was already configured,vouch install-mcp codexwas a silent no-op and vouch never got wired. this is the toml equivalent of the situationjson_mergealready solves for claude-code's.claude/settings.json.this adds a
toml_merge: trueentry flag next tofenced_appendandjson_merge, with semantics mirroring_install_json_merge: parse the existing destination with stdlibtomllib, deep-merge the template's tables into it, write back through themergedresult channel. conflicts are deterministic and match the json_merge convention — the user's existing value always wins; only genuinely missing keys (including nested ones, e.g. a missingenvtable under an existing[mcp_servers.vouch]) are filled in. re-running is a flat no-op reported as skipped. the codex T1 manifest entry is flipped totoml_merge: true.on the writer decision flagged in the ticket: hand-rolled serializer, not
tomli-w, keeping the dependency set unchanged. it covers the shapes tomllib can hand us from real configs (tables, arrays, inline tables inside arrays, quoted keys, scalars, datetimes) and self-checks: the serialized output must round-trip throughtomllib.loadsback to exactly the merged data, and anything it can't faithfully re-emit degrades to skipped rather than risking the user's config. malformed existing files are likewise left untouched, matching json_merge. comments and formatting in a merged file are not preserved — the same trade-off_install_json_mergealready makes.installer-only; no kb read/write surface touched, nothing bypasses
proposals.approve(). stays project-local — never touches~/.codex/config.toml.tests cover: merge preserving unrelated tables and top-level values, idempotent re-run with stable content, fresh install writing the full template, existing-vouch-entry conflict (user wins, missing env table still added), malformed toml skipped untouched, serializer round-trip of the shipped shapes, and no-change detection. verified end-to-end via
vouch install-mcp codex --tier T1against a pre-seeded config: unrelated[mcp_servers.other]andmodelsurvive,[mcp_servers.vouch]lands next to them, and the re-run reportsalready present.closes #384