fix: tolerate non-object server values on load; keep named sets across a merge (#72, #73) #77

2026-07-20T12:12:19-04:00

the_og commented

2026-07-20 12:12:19 -04:00

Two silent-failure bugs found auditing the v1.3.0 features. Both are in the "quietly does the wrong thing" class rather than the "throws something you can act on" class.

Closes #72
Closes #73

#72 — non-object server value crashed the load

extract_servers called dict() on every server value. A config that's valid JSON but holds a non-object server takes the app down:

{ "mcpServers": { "foo": "oops" } }

Three things made it worse than a normal parse error:

The strict parse succeeds (the JSON is well-formed), so the repair pipeline never sees it.
The call sat outside _load_profile's try/except — unhandled traceback, with current_profile already reassigned but servers not.
The #54 schema lint could never fire on this, the single most likely hand-edit mistake, because the load died first.

That last point is the one that bothered me: a file that's valid JSON but structurally off is precisely what this project exists to handle gracefully.

Fix. Malformed values are preserved verbatim on ServerEntry.raw and written back untouched on Save, so nothing is silently deleted — the cardinal rule holds. lint_servers names the offending entry instead of the app dying.

raw uses a NO_RAW sentinel rather than defaulting to None. My first pass used None and my own test caught it: {"mcpServers": {"foo": null}} is legal JSON and a genuine malformed-entry case, so None has to mean "the config said null", not "there was nothing here."

data stays a dict for every consumer; set_data() retires the raw value when the user edits, so that can't be forgotten at a call site. extract_servers also moved inside the load guard as defence in depth — a load failure should leave the previous profile intact rather than half-swapping the window.

#73 — Merge & save silently discarded named server sets

The stale-file Merge & save path reloads from disk and re-applies the user's servers:

fresh = core.load_config(path)
core.apply_servers(fresh, self.servers)   # only mcpServers + _disabledMcpServers
self.full_config = fresh

Named sets live under _bccServerSets in that same file. apply_servers doesn't write them, so a set saved during the session was dropped from disk and then from memory — no warning, on the path the user picks because it sounds like the safe one. Same class as the #8 drag-and-drop overwrite.

Fix. BCC-authored keys are declared in BCC_OWNED_KEYS and carried over by carry_owned_keys, which returns genuinely contested keys so the status line can say "kept your _bccServerSets (the file on disk had a different copy)" rather than picking a side in silence.

Declaring the keys in one tuple is the actual fix — the bug was structural, and the next BCC-authored key would have reintroduced it.

One deliberate asymmetry: a key absent locally is left alone on disk. We can't distinguish "user deleted their last set" from "user never had sets and another machine just added some," and silently deleting someone else's data is the worse of the two failures. Documented in the docstring and covered by a test.

Tests

374 passed, 1 skipped (+16 new). Ruff check and format clean.

New coverage: every non-dict shape (str, int, list, None, bool, float) survives load; only the bad entry is flagged; malformed values round-trip through Save unchanged; editing retires raw; lint reports malformed alongside normal warnings; carry_owned_keys carries, detects conflict, deep-copies, and stays one-directional; plus an end-to-end merge test asserting the set survives and unrelated external keys are preserved.

Review notes

import copy added to bcc_core for the deep copy in carry_owned_keys — without it, later edits to the in-memory set would leak into the written config.
No behaviour change on well-formed configs; the normal round-trip is asserted unchanged.
bcc.py needed a ruff format pass after the edit.

Two silent-failure bugs found auditing the v1.3.0 features. Both are in the "quietly does the wrong thing" class rather than the "throws something you can act on" class. Closes #72 Closes #73 --- ## #72 — non-object server value crashed the load `extract_servers` called `dict()` on every server value. A config that's **valid JSON** but holds a non-object server takes the app down: ```json { "mcpServers": { "foo": "oops" } } ``` Three things made it worse than a normal parse error: 1. The strict parse *succeeds* (the JSON is well-formed), so the repair pipeline never sees it. 2. The call sat outside `_load_profile`'s try/except — unhandled traceback, with `current_profile` already reassigned but `servers` not. 3. The #54 schema lint could never fire on this, the single most likely hand-edit mistake, because the load died first. That last point is the one that bothered me: a file that's valid JSON but structurally off is precisely what this project exists to handle gracefully. **Fix.** Malformed values are preserved verbatim on `ServerEntry.raw` and written back untouched on Save, so nothing is silently deleted — the cardinal rule holds. `lint_servers` names the offending entry instead of the app dying. `raw` uses a `NO_RAW` sentinel rather than defaulting to `None`. My first pass used `None` and my own test caught it: `{"mcpServers": {"foo": null}}` is legal JSON and a genuine malformed-entry case, so `None` has to mean "the config said null", not "there was nothing here." `data` stays a dict for every consumer; `set_data()` retires the raw value when the user edits, so that can't be forgotten at a call site. `extract_servers` also moved inside the load guard as defence in depth — a load failure should leave the previous profile intact rather than half-swapping the window. ## #73 — Merge & save silently discarded named server sets The stale-file **Merge & save** path reloads from disk and re-applies the user's servers: ```python fresh = core.load_config(path) core.apply_servers(fresh, self.servers) # only mcpServers + _disabledMcpServers self.full_config = fresh ``` Named sets live under `_bccServerSets` in that same file. `apply_servers` doesn't write them, so a set saved during the session was dropped from disk and then from memory — no warning, on the path the user picks *because it sounds like the safe one*. Same class as the #8 drag-and-drop overwrite. **Fix.** BCC-authored keys are declared in `BCC_OWNED_KEYS` and carried over by `carry_owned_keys`, which returns genuinely contested keys so the status line can say "kept your `_bccServerSets` (the file on disk had a different copy)" rather than picking a side in silence. Declaring the keys in one tuple is the actual fix — the bug was structural, and the next BCC-authored key would have reintroduced it. **One deliberate asymmetry:** a key absent locally is left alone on disk. We can't distinguish "user deleted their last set" from "user never had sets and another machine just added some," and silently deleting someone else's data is the worse of the two failures. Documented in the docstring and covered by a test. --- ## Tests 374 passed, 1 skipped (+16 new). Ruff check and format clean. New coverage: every non-dict shape (`str`, `int`, `list`, `None`, `bool`, `float`) survives load; only the bad entry is flagged; malformed values round-trip through Save unchanged; editing retires `raw`; lint reports malformed alongside normal warnings; `carry_owned_keys` carries, detects conflict, deep-copies, and stays one-directional; plus an end-to-end merge test asserting the set survives *and* unrelated external keys are preserved. ## Review notes - `import copy` added to `bcc_core` for the deep copy in `carry_owned_keys` — without it, later edits to the in-memory set would leak into the written config. - No behaviour change on well-formed configs; the normal round-trip is asserted unchanged. - `bcc.py` needed a `ruff format` pass after the edit.

the_og added 1 commit 2026-07-20 12:12:19 -04:00

fix: tolerate non-object server values on load; keep named sets across a merge

CI / Lint (ruff) (pull_request) Successful in 13s

Details

CI / Tests (py3.10 / ubuntu-latest) (pull_request) Successful in 12s

Details

CI / Tests (py3.12 / ubuntu-latest) (pull_request) Successful in 11s

Details

CI / Tests (py3.13 / ubuntu-latest) (pull_request) Successful in 12s

Details

CI / Catalog signature (pull_request) Successful in 8s

Details

CI / Tests (py3.12 / windows-latest) (pull_request) Has been cancelled

Details

da20eb2fdb

Two silent-failure bugs found auditing the v1.3.0 features.

#72 -- extract_servers called dict() on every server value, so a config
that was valid JSON but held a non-object server ("foo": "oops", a
number, a list, null) raised on load. The strict parse succeeded, so the
repair path never saw it, and the call sat outside the load try/except:
an unhandled traceback with the window half-swapped to the new profile.
It also made the #54 schema lint unreachable for the most likely
hand-edit mistake -- the load died before the linter ran.

Malformed values are now preserved verbatim on ServerEntry.raw (behind a
NO_RAW sentinel, since a literal JSON null is itself a malformed entry
worth keeping) and written back untouched on Save, so nothing is silently
deleted. lint_servers names the offending entry instead.

#73 -- the stale-file "Merge & save" path reloaded the file from disk and
re-applied the user's servers, but apply_servers only writes mcpServers
and _disabledMcpServers. Named server sets live under _bccServerSets in
the same file, so a set saved that session was dropped from disk and then
from memory, with no warning, on the path the user picks because it
sounds like the safe one.

BCC-owned keys are now declared in BCC_OWNED_KEYS and carried across by
carry_owned_keys, which reports genuinely contested keys so the status
line can say so. Deliberately one-directional: a key absent locally is
left alone on disk, because 'user deleted their last set' and 'another
machine just added sets' are indistinguishable and deleting someone
else's data is the worse failure.

Closes #72
Closes #73

the_og merged commit 05b00a40c0 into main

2026-07-20 12:49:23 -04:00

the_og referenced this issue from a commit

2026-07-20 12:50:11 -04:00

Merge branch 'main' into feat/75-theming

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: the_og/better-claude-config#77