fix(mcp/opencode): don't clobber an unparseable opencode.json on register - #1661
Conversation
PR governanceThis PR follows the template and is marked ready for human review. |
JerrettDavis
left a comment
There was a problem hiding this comment.
Reviewed the OpenCode registrar write-path guard and tests. The change keeps read-only tolerance for malformed JSON while making full-file rewrites refuse an existing unparseable config, which addresses the data-loss risk without broadening behavior elsewhere. The valid-config merge test also covers preserving unrelated keys and existing MCP servers.
I updated the branch from main; CI/governance is rerunning on that merge commit.
JerrettDavis
left a comment
There was a problem hiding this comment.
Still looks good. I re-reviewed the write path after the latest update: read-only callers retain tolerant parsing, while the full-file rewrite path now refuses a present-but-unparseable opencode.json so user config is not clobbered.
I pushed a test-only fix for the remaining stale expectation that still asserted the old destructive behavior. Local focused verification passed: python -m pytest tests/test_mcp_registry_opencode.py -q. The commit was made with --no-verify so no local commit hooks were executed. CI has re-queued after the push.
6c7fc9b to
bbc516b
Compare
bbc516b to
eec022c
Compare
JerrettDavis
left a comment
There was a problem hiding this comment.
The latest head is red in CI, so I am moving this back to changes requested.
Current failing check: test (2) on run 29115340570.
Failure:
tests/test_mcp_registry_opencode.py::test_register_server_on_malformed_config_file- Expected
RegisterStatus.REGISTERED, gotRegisterStatus.FAILED.
This is directly in the malformed OpenCode config path this PR changes, so please update the implementation or the regression test expectation and rerun the shard.
…ster
_write_entry read opencode.json through _read_json -- which returns {}
on JSONDecodeError -- then rewrote the whole file with only
{"mcp": {...}}. OpenCode configs are commonly hand-edited / JSONC, so a
file with a comment or a stray trailing comma parsed as {} and the
rewrite wiped everything else: theme, model, provider, and any other
MCP servers the user had.
Add _read_json_for_write (returns {} only for an absent/empty file,
raises _MalformedConfigError when the file is present but not a JSON
object) and use it in _write_entry, which now returns FAILED with an
actionable message instead of overwriting. _read_json is unchanged for
read-only callers.
Tests: register against malformed configs leaves the bytes untouched and
returns FAILED; register against a valid config still merges and
preserves theme/model plus a pre-existing MCP server.
Same class of fix as the Claude registrar.
The clobber guard makes register on a present-but-unparseable opencode.json refuse to overwrite (FAILED) and preserve the file, so the pre-existing test_register_server_on_malformed_config_file (which asserted REGISTERED / 'preserving nothing') now asserts FAILED and byte-for-byte preservation.
eec022c to
f7b0859
Compare
|
Fixed and rebased on latest main. The failing I'd updated that test in the original PR, but a rebase conflict resolution reverted it to the upstream version. I've re-applied the update - it now asserts FAILED and that the malformed file is preserved byte-for-byte, matching the guard (and my |
JerrettDavis
left a comment
There was a problem hiding this comment.
Looks good now. The stale malformed-config expectation is corrected, CI is green, and the focused OpenCode registrar suite passes locally:
uv run pytest tests/test_mcp_registry_opencode.py -q -> 36 passed.
The write path now refuses to overwrite a present-but-unparseable opencode.json, while read-only callers remain tolerant and valid configs still merge without dropping unrelated keys.
CodexRegistrar.register_server only refused to clobber a user-managed entry when get_server() returned one. But get_server() returns None both for an unparseable config.toml and for an mcp_servers / mcp_servers.<name> that is present-but-not-a-table. In those cases register_server fell through to _write_block, which blindly appends a [mcp_servers.<name>] table -- appending into an unparseable file, or creating a duplicate [mcp_servers.headroom] key next to a non-table entry (e.g. `headroom = "..."`), which tomllib/codex then reject, destroying a previously-valid user config. Add _unmergeable_reason() and refuse (FAILED, file untouched) when the existing file can't be safely merged, mirroring the claude (headroomlabs-ai#1660) and opencode (headroomlabs-ai#1661) guards. Adds tests for unparseable TOML, non-table mcp_servers.headroom, and non-table mcp_servers.
…2062) ## Description `CodexRegistrar.register_server` (`headroom/mcp_registry/codex.py`) guards against clobbering a user-managed `[mcp_servers.<name>]` entry — but **only inside the `if existing is not None` branches**. `existing` comes from `get_server`, which returns `None` in two cases that are *not* "nothing there": 1. the `config.toml` is **unparseable** (`_load_toml` catches `TOMLDecodeError` and returns `{}`), and 2. `mcp_servers` (or `mcp_servers.<name>`) is present but **not a table** (`get_server` returns `None` via its `isinstance` guards). With `existing is None`, all three protection branches are skipped and control falls straight to `_write_block`, which blindly appends a fresh `[mcp_servers.<name>]` table. So for a **valid** TOML file like: ```toml [mcp_servers] headroom = "not-a-table" ``` `register_server(headroom_spec)` appends `[mcp_servers.headroom]`, producing a file that defines `mcp_servers.headroom` **both** as a string and as a table — a duplicate key that `tomllib`/codex then reject, **corrupting a previously-valid user config**. The unparseable-file case similarly appends our block into a file that can't be parsed. This is the exact Codex sibling of the claude (#1660) and opencode (#1661) clobber-guard fixes; codex never received it. Closes: no issue filed — found while auditing the registrars for the #1660/#1661 class. ## Fix Add `_unmergeable_reason(name)` — returns why the existing file can't be safely merged (present but unparseable, or a non-table `mcp_servers` / `mcp_servers.<name>`), else `None`. In `register_server`, when `existing is None`, refuse with `RegisterStatus.FAILED` (leaving the file untouched) instead of appending. Absent/empty/valid configs are unaffected. ## Type of Change - [x] Bug fix (non-breaking change that fixes an issue) ## Changes Made - `headroom/mcp_registry/codex.py`: add `_unmergeable_reason`; refuse in `register_server` when the existing config is unparseable or defines a non-table `mcp_servers`/`mcp_servers.<name>`. - `tests/test_mcp_registry/test_codex_registrar.py`: add tests for unparseable TOML, non-table `mcp_servers.headroom`, and non-table `mcp_servers` (all refuse + file untouched). ## Testing - [x] New regression tests added (`tests/test_mcp_registry/test_codex_registrar.py`) - [x] Linting/formatting clean — run with the CI-pinned `ruff==0.15.17` - [ ] Full `pytest` deferred to CI (local-OOM reason below). ```text $ uvx ruff@0.15.17 check headroom/mcp_registry/codex.py tests/test_mcp_registry/test_codex_registrar.py All checks passed! ``` ## Real Behavior Proof - Environment: Windows 11, Python 3.10, headroom from this branch. Importing `headroom` pulls in the torch/transformers stack and a full `pytest` gets OOM-killed on this box, so I verified the `_unmergeable_reason` logic with a dependency-free script (stdlib `tomllib`) and left the full pytest to CI. - Exact command / steps: ran the two clobber cases (non-table entry, unparseable TOML) and the safe cases (absent/empty/valid/other-server) through the guard. - Observed result: the guard refuses exactly the two corrupting cases and allows every valid config: ```text REFUSE [non-table entry (valid TOML)]: non-table mcp_servers.headroom REFUSE [unparseable TOML]: not valid TOML (Invalid value (at line 1, column 8)) ALLOW [absent]: reason=None ALLOW [empty]: reason=None ALLOW [valid, no mcp_servers]: reason=None ALLOW [valid, mcp_servers table w/ other server]: reason=None CODEX CLOBBER-GUARD VERIFIED (refuses non-table/unparseable; allows valid configs) ``` - Not tested: a live `codex` launch reading the config (mocked in the registrar tests). Full local `pytest` deferred to CI (OOM, per above). ## Review Readiness - [x] I have performed a self-review - [x] This PR is ready for human review ## Checklist - [x] My code follows the project's style guidelines - [x] I have performed a self-review of my code - [x] I have commented my code, particularly in hard-to-understand areas - [ ] I have made corresponding changes to the documentation - [x] My changes generate no new warnings - [x] I have added tests that prove my fix is effective or that my feature works - [ ] New and existing unit tests pass locally with my changes — ran lint + a standalone logic check; full pytest deferred to CI (local OOM, disclosed above) - [x] I have updated the CHANGELOG.md if applicable ## Additional Notes - Completes the registrar clobber-guard trio (claude #1660, opencode #1661, codex here); no new dependencies. - @JerrettDavis tagging you — same class you already reviewed for claude/opencode, just the codex side. Thanks! --------- Co-authored-by: JerrettDavis <mxjerrett@gmail.com>
…ster (headroomlabs-ai#1661) ## Description `OpencodeRegistrar._write_entry` does a full-file read-modify-write of `opencode.json`: ```python data = _read_json(self._config_path) # returns {} on JSONDecodeError mcp = data.setdefault("mcp", {}) mcp[spec.name] = _spec_to_entry(spec) _write_json(self._config_path, data) # overwrites the ENTIRE file ``` `_read_json` returns `{}` for a file that exists but doesn't parse. OpenCode configs are commonly hand-edited and JSONC-ish (comments, trailing commas), so a file that doesn't strictly parse gets silently rewritten as just `{"mcp": {"headroom": {...}}}` — **destroying the user's `theme`, `model`, `provider`, and any other MCP servers**. No backup. This is the same class of data-loss bug as the Claude registrar (separate PR); this one is `headroom/mcp_registry/opencode.py`. Closes: no issue filed — found while auditing the MCP registry config-write paths. ## Fix Keep `_read_json` (returning `{}`) for read-only callers. Add `_read_json_for_write` for the rewrite path: it returns `{}` only when the file is **absent or empty**, and raises `_MalformedConfigError` when the file is present but not a JSON object. `_write_entry` catches it and returns `FAILED` with an actionable message instead of overwriting. Absent/empty → registers fresh (unchanged); valid → merges, all keys preserved (unchanged); present-but-invalid → left untouched. ## Type of Change - [x] Bug fix (non-breaking change that fixes an issue) ## Changes Made - `headroom/mcp_registry/opencode.py`: add `_read_json_for_write` + `_MalformedConfigError`; `_write_entry` uses it and returns `FAILED` (without writing) when `opencode.json` is present-but-unparseable. `_read_json` unchanged for read-only callers. - `tests/test_mcp_registry_opencode.py`: regression tests — register against malformed configs leaves the bytes untouched and returns `FAILED`; register against a valid config still merges and preserves `theme`/`model` plus a pre-existing MCP server. - `CHANGELOG.md`: Bug Fixes entry under Unreleased. ## Testing - [x] New tests added for the fixed behavior - [x] Linting passes (`ruff check`) and formatting is clean (`ruff format --check`) - [ ] Full `pytest` deferred to CI (local-OOM reason below). ```text $ uv run ruff check headroom/mcp_registry/opencode.py tests/test_mcp_registry_opencode.py All checks passed! ``` ## Real Behavior Proof - Environment: Windows 11, Python 3.12.11, headroom built from this branch. Importing `headroom` loads the torch/transformers stack; a full `pytest` gets OOM-killed on this box, so I verified the write-path logic with a dependency-free script and left the full pytest to CI. - Exact command / steps: the write-path logic here is identical to the Claude registrar fix, so I verified it with the same standalone script — replicated `_read_json_for_write` + the read-modify-write flow (only stdlib, no `headroom` import) against real temp files, exercising absent, empty, four malformed variants, and a valid config carrying unrelated keys. - Observed result: absent/empty register fresh; every malformed variant returns FAILED and the on-disk bytes are unchanged (no clobber); a valid config merges the new server while unrelated keys survive: ```text OK: absent -> fresh register OK: empty -> fresh register OK: malformed -> FAILED, original bytes preserved (no clobber) OK: valid config -> merged, unrelated keys preserved MCP CONFIG-WRITE LOGIC VERIFIED ``` - Not tested: driving a real `opencode` install end-to-end (didn't want to touch a real config); the file-write path is exercised directly by the regression tests. Full local `pytest` deferred to CI (OOM, per above). ## Review Readiness - [x] I have performed a self-review - [x] This PR is ready for human review ## Checklist - [x] My code follows the project's style guidelines - [x] I have performed a self-review of my code - [x] I have commented my code, particularly in hard-to-understand areas - [ ] I have made corresponding changes to the documentation - [x] My changes generate no new warnings - [x] I have added tests that prove my fix is effective or that my feature works - [ ] New and existing unit tests pass locally with my changes — ran lint + a standalone logic check; full pytest deferred to CI (local OOM, disclosed above) - [x] I have updated the CHANGELOG.md if applicable ## Additional Notes - Companion to the Claude-registrar fix (same root cause, different file). No new dependencies. This does not touch OpenCode's `opencode.jsonc` file-selection (handled elsewhere) — it only hardens the existing `opencode.json` write against clobbering.
Description
OpencodeRegistrar._write_entrydoes a full-file read-modify-write ofopencode.json:_read_jsonreturns{}for a file that exists but doesn't parse. OpenCodeconfigs are commonly hand-edited and JSONC-ish (comments, trailing commas), so a
file that doesn't strictly parse gets silently rewritten as just
{"mcp": {"headroom": {...}}}— destroying the user'stheme,model,provider, and any other MCP servers. No backup.This is the same class of data-loss bug as the Claude registrar (separate PR);
this one is
headroom/mcp_registry/opencode.py.Closes: no issue filed — found while auditing the MCP registry config-write paths.
Fix
Keep
_read_json(returning{}) for read-only callers. Add_read_json_for_writefor the rewrite path: it returns{}only when the fileis absent or empty, and raises
_MalformedConfigErrorwhen the file ispresent but not a JSON object.
_write_entrycatches it and returnsFAILEDwith an actionable message instead of overwriting.
Absent/empty → registers fresh (unchanged); valid → merges, all keys preserved
(unchanged); present-but-invalid → left untouched.
Type of Change
Changes Made
headroom/mcp_registry/opencode.py: add_read_json_for_write+_MalformedConfigError;_write_entryuses it and returnsFAILED(without writing) whenopencode.jsonis present-but-unparseable._read_jsonunchanged for read-only callers.tests/test_mcp_registry_opencode.py: regression tests — register against malformed configs leaves the bytes untouched and returnsFAILED; register against a valid config still merges and preservestheme/modelplus a pre-existing MCP server.CHANGELOG.md: Bug Fixes entry under Unreleased.Testing
ruff check) and formatting is clean (ruff format --check)pytestdeferred to CI (local-OOM reason below).Real Behavior Proof
headroomloads the torch/transformers stack; a fullpytestgets OOM-killed on this box, so I verified the write-path logic with a dependency-free script and left the full pytest to CI._read_json_for_write+ the read-modify-write flow (only stdlib, noheadroomimport) against real temp files, exercising absent, empty, four malformed variants, and a valid config carrying unrelated keys.opencodeinstall end-to-end (didn't want to touch a real config); the file-write path is exercised directly by the regression tests. Full localpytestdeferred to CI (OOM, per above).Review Readiness
Checklist
Additional Notes
opencode.jsoncfile-selection (handled elsewhere) — it only hardens the existingopencode.jsonwrite against clobbering.