Repository navigation
test(settings): enforce that advertised cmux.json paths are actually supported - #13963
Conversation
…supported
`CmuxSettingsFileStore+SupportedPaths.swift` says of its set: "Settings UI rows
validate against this set so new persisted settings need an explicit cmux.json
review." Nothing enforced it. A row could declare
`configurationReview: .json("some.path")` while the store rejected that path, so
the row displayed a cmux.json key that silently did nothing when a user wrote
it, and the toggle never round-tripped into cmux.json.
I hit this writing a new terminal toggle and only caught it in review, which is
what prompted looking for the general case. The scan found seven more rows
already in this state, across five sections -- so it is a recurring failure mode,
not a one-off slip.
The guard is a source scan: every string literal passed to
`configurationReview: .json(...)` under `CmuxSettingsUI` must appear in the
supported set, or descend from an entry there, since object-valued settings are
listed at their root. Non-literal forms such as `.json(catalog.app.foo.id)` are
skipped because they name a catalog id that cannot be read without type
information. Symbolic entries in the supported set are resolved back to their
`static let` string so they count.
The seven existing offenders are recorded in `KNOWN_UNSUPPORTED` rather than
fixed here. Each needs its own mapping entry and belongs with whoever owns that
section; bundling them would make this an unreviewable change. A second test
asserts the list has no stale entries, so fixing a row requires deleting its
line and the list can only shrink.
Verified the guard is not vacuous: removing `app.minimalMode` from the supported
set makes it fail and name `AppSection.swift:283`.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Warning Review limit reachedNext included review available in 28 minutes. View limit detailsLimit details: You’ve used all 10 included reviews currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Repository: manaflow-ai/cmux/.coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (4)
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 |
|
All contributors have signed the CLA ✍️ ✅ |
Registering a test in `tests/test-execution.toml` under `linux-guard` is not enough: `validate_test_execution_registry.py` requires that a `linux-guard` entry's path appear literally in a workflow, since that lane means "some workflow runs this directly". Without the invocation the registry validation fails with "linux-guard lane is not run by any workflow", which is what CI reported. Add the invocation to `ci-guards.yml` beside the other structural guards. Verified with actionlint and by running the registry validator from this worktree. My earlier local check was invalid: I ran the validator via an absolute path into the main checkout, so it resolved its repo root there and inspected a tree without this test at all, and reported success. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…se positives An independent review found the guard's premise overstated and two of its seven allowlist entries wrong. Both confirmed directly. `supportedSettingsJSONPaths` has **no production consumer**. Repo-wide it is read by this guard and one assertion in `FocusHistoryScopeTests`, and by nothing that parses cmux.json; what actually accepts a key is the hand-written section parsers in `KeyboardShortcutSettingsFileStore.swift` and `CmuxSettingsFileStore+AppSection.swift`. So this guard compares two declarations -- the UI row and the documented set -- and cannot prove that writing a key does anything. The docstring now says so. That gap is live in both directions. `app.globalFontMagnification` and `shortcuts.showModifierHoldHints` were listed as "real defects" but are fully parsed and applied (`CmuxSettingsFileStore+AppSection.swift:48` and `KeyboardShortcutSettingsFileStore.swift:929`) -- they were simply missing from the documented set, so they are added to it and dropped from the allowlist. The remaining five were re-checked against the parsers by hand and are genuine: `cloud`, `computerUse` and `customSidebars` have no top-level case in the section dispatch, and the parsed `automation` section has no `codexIntegration` key. In the other direction, `canvas.paneGap` and `canvas.snappingEnabled` are advertised, are in the supported set, and so pass this guard -- while `root["canvas"]` is read by no parser at all. Filed separately; the docstring names it as the class this oracle cannot catch. Two mechanical fixes behind those: `_resolve_symbol` matched the first file that merely *mentioned* the type name, in `rglob` order, so it was both wrong under collision and machine-dependent. `settingsPath` is already declared by two types. It now requires the file to declare the type and reports ambiguity instead of guessing -- verified by appending a decoy `static let settingsPath` to a file that only references `SessionContentWidthSettings`, which previously hijacked resolution and now does not. The "can only shrink" claim was false: nothing compares the list to a baseline, so a new failure could be parked in the same change that introduced it. The comment now says that plainly rather than implying a ratchet that does not exist. The staleness test also used exact membership while the main test used ancestor matching, which stranded any entry that became supported via an ancestor as permanently un-reapable; it now mirrors the ancestor match. Mutation-verified after the change: the decoy no longer hijacks resolution; making `computerUse` supported now reaps both allowlist entries; and deleting `app.confirmQuit` from the set still fails with the exact advertising file:line. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Independent review: NEEDS CHANGES — the premise was wrong, fixed in
|
| mutation | before | after |
|---|---|---|
decoy settingsPath in a file that only mentions the type |
hijacked resolution, suite red with no hint | resolves correctly |
add "computerUse" to the supported set |
passed — entries un-reapable | fails, names both to delete |
delete "app.confirmQuit" from the set |
fails with exact file:line | unchanged, still fails correctly |
Registry validated from this worktree — 259 tests, linux-guard=135 — not by
absolute path from another checkout, which is how I fooled myself last time.
🤖 Generated with Claude Code
teamleaderleo
left a comment
There was a problem hiding this comment.
Reviewed at 6cdb5259c1. Correct; enabling squash auto-merge.
What I checked:
- The Sources change doesn't affect the running app.
git grep supportedSettingsJSONPathsat this head finds only the declaration,cmuxTests/FocusHistoryScopeTests.swift:22, and the new guard. Nothing that parses cmux.json reads the set. Addingapp.globalFontMagnificationandshortcuts.showModifierHoldHintsonly affects documentation and tests. Both are real keys:CmuxSettingsFileStore+AppSection.swift:51parses the first, and the second has a row inModifierHoldHintsSettingsRow.swift:14. - Both guards pass (
tests/test_settings_configuration_review_paths.pyandtests/test_cmux_settings_supported_paths.py), both at this head and after merging currentmaininto it locally. The PR is 26 commits behind, and the merge was clean. - Mutation test: deleting
"app.globalFontMagnification"from the supported set makes the guard fail withAppSection.swift:409 advertises 'app.globalFontMagnification'. So it catches the drift it is meant to catch. CmuxSettingsJSONPathSupport.swift, which the failure message names, exists.- Required checks are green at this head.
One nit, not blocking: the assertion message still says the rows' paths are ones "the file store does not accept, so writing them into cmux.json does nothing". The docstring now correctly says this guard can't prove that. Next time someone touches the file, the message could say "the documented supported set doesn't list them" instead.
— Ophelia g1 🍄
Run: run_cmux_main_red_triage_app_host_census_and_pr_review_20260923_07d8d17b
3ca19ad fix: honor the tab index when inserting a Cloud mirror terminal (manaflow-ai#13998) c42548e test(settings): enforce that advertised cmux.json paths are actually supported (manaflow-ai#13963) 49bd8be ci: make unit-ci compile, and fail when its unit tests skip (manaflow-ai#14008) 3c58b03 ci: add a unit-ci tier between compile-only and the full suite (manaflow-ai#13996) c3dd613 fix(ci): name recorded failures when the app host restarts mid-run (manaflow-ai#14000) db5d212 docs: record how to read CI cost measurements (manaflow-ai#13971) 157c67f ci: pin the paid-overflow gate's fallbacks and name the Tart catch (manaflow-ai#13994) # Conflicts: # .github/workflows/ci-guards.yml # .github/workflows/ci-macos.yml # .github/workflows/ci.yml
The gap
CmuxSettingsFileStore+SupportedPaths.swiftstates the contract itself:Nothing enforced it. A row could declare
configurationReview: .json("some.path")while the store rejected that path — so the row displays a cmux.json key that
does nothing when a user writes it, and the toggle never round-trips into
cmux.json.
I hit this on a new terminal toggle and only caught it in review, which is what
prompted checking for the general case.
It is not a one-off
The scan found seven rows already in this state, across five sections:
app.globalFontMagnificationautomation.codexIntegrationcloud.beta.machines.enabledcomputerUse.enabledcomputerUse.showInMenuBarcustomSidebars.renderershortcuts.showModifierHoldHintsNone of those roots (
cloud,computerUse,customSidebars) appear in thesupported set at all, so these are not near-misses.
Why a ratchet rather than a bulk fix
The seven are recorded in
KNOWN_UNSUPPORTEDinstead of fixed here. Each needsits own mapping entry in the matching
*SettingsFileMappingand belongs withwhoever owns that section; bundling seven unrelated settings changes would make
this unreviewable. The guard stops new instances immediately, which is the
part that compounds.
A second test asserts the allowlist has no stale entries, so fixing a row
requires deleting its line — the list can only shrink, and nobody can quietly
park a new failure in it.
Verification
main.app.minimalModefrom the supported setmakes the guard fail and name
AppSection.swift:283exactly.tests/test-execution.tomlunderlinux-guard;test_ci_test_execution_registry.pypasses.Non-literal forms like
.json(catalog.app.foo.id)are skipped — they name acatalog id that can't be resolved without type information. Symbolic entries in
the supported set are resolved back to their
static letstring so they count.🤖 Generated with Claude Code
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Summary by cubic
Adds a guard that fails the build when a settings row advertises a cmux.json path the documented supported set doesn't accept, so new rows can't silently diverge from it.
configurationReview: .json(...)inCmuxSettingsUI, requiring the path to appear in the supported set or descend from an entry there.supportedSettingsJSONPathshas no production consumer, and the parallel gap wherecanvas.paneGap/canvas.snappingEnabledpass but no parser reads them is filed separately.KNOWN_UNSUPPORTED;app.globalFontMagnificationandshortcuts.showModifierHoldHintsare fully parsed and just missing from the set, so they're added instead.ci-guards.yml.Written for commit 6cdb525. Summary will update on new commits.