From 7dc2afc0fe61998de4c1203d1c920bbb05f31691 Mon Sep 17 00:00:00 2001 From: Leo Li Date: Wed, 23 Sep 2026 03:46:50 -0700 Subject: [PATCH 1/3] test(settings): enforce that advertised cmux.json paths are actually 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 --- tests/test-execution.toml | 4 + ...est_settings_configuration_review_paths.py | 142 ++++++++++++++++++ 2 files changed, 146 insertions(+) create mode 100755 tests/test_settings_configuration_review_paths.py diff --git a/tests/test-execution.toml b/tests/test-execution.toml index 1923506be239..2a740d3af40a 100644 --- a/tests/test-execution.toml +++ b/tests/test-execution.toml @@ -794,6 +794,10 @@ lane = "linux-guard" path = "tests/test_cmux_settings_supported_paths.py" lane = "linux-guard" +[[test]] +path = "tests/test_settings_configuration_review_paths.py" +lane = "linux-guard" + [[test]] path = "tests/test_codex_feed_hooks.py" lane = "legacy" diff --git a/tests/test_settings_configuration_review_paths.py b/tests/test_settings_configuration_review_paths.py new file mode 100755 index 000000000000..1d6b61cc5ef2 --- /dev/null +++ b/tests/test_settings_configuration_review_paths.py @@ -0,0 +1,142 @@ +#!/usr/bin/env python3 +"""Every cmux.json path a settings row advertises must be one the store accepts. + +`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 that. A row could declare +`configurationReview: .json("terminal.textEditingGestures")` while the store +rejected the path, so the row displayed a cmux.json key that silently did +nothing when a user wrote it. +""" + +import re +import unittest +from pathlib import Path + +REPO_ROOT = Path(__file__).resolve().parents[1] +SUPPORTED = REPO_ROOT / "Sources" / "CmuxSettingsFileStore+SupportedPaths.swift" +UI_ROOT = REPO_ROOT / "Packages" / "macOS" / "CmuxSettingsUI" / "Sources" +SOURCE_ROOTS = (REPO_ROOT / "Sources", REPO_ROOT / "Packages") + +# `configurationReview: .json("a", "b")` may list several paths for one row. +REVIEW = re.compile(r"configurationReview:\s*\.json\(([^)]*)\)") +STRING = re.compile(r'"([^"]+)"') +# Entries in the supported set may be symbolic, e.g. PaneChromeSettings.fooKey. +SYMBOL = re.compile(r"^([A-Z][A-Za-z0-9_]*)\.([A-Za-z0-9_]+)\s*,?$") + + +def _resolve_symbol(type_name, member): + """Find `static let = ""` inside ``'s file.""" + pattern = re.compile( + r"static\s+let\s+" + re.escape(member) + r"\s*(?::\s*String\s*)?=\s*\"([^\"]+)\"" + ) + for root in SOURCE_ROOTS: + for path in root.rglob("*.swift"): + text = path.read_text(encoding="utf-8", errors="replace") + if type_name not in text: + continue + found = pattern.search(text) + if found: + return found.group(1) + return None + + +def supported_paths(): + resolved, unresolved = set(), [] + for raw in SUPPORTED.read_text(encoding="utf-8").splitlines(): + line = raw.strip() + if not line or line.startswith("//"): + continue + literal = STRING.search(line) + if literal: + resolved.add(literal.group(1)) + continue + symbol = SYMBOL.match(line) + if symbol: + value = _resolve_symbol(*symbol.groups()) + if value: + resolved.add(value) + else: + unresolved.append(line) + return resolved, unresolved + + +def advertised_paths(): + for path in sorted(UI_ROOT.rglob("*.swift")): + text = path.read_text(encoding="utf-8", errors="replace") + for match in REVIEW.finditer(text): + args = match.group(1) + # Skip non-literal forms such as `.json(catalog.app.foo.id)`; those + # name a catalog id that cannot be read without type information. + for value in STRING.findall(args): + line = text.count("\n", 0, match.start()) + 1 + yield value, path.relative_to(REPO_ROOT), line + + +# Rows that already advertised an unsupported path when this guard was added. +# Each is a real defect: the row shows a cmux.json key that does nothing when a +# user writes it. They are recorded rather than fixed here so the guard can stop +# new instances immediately; see the tracking issue. Fixing one means deleting +# its entry below, which this test enforces, so the list can only shrink. +KNOWN_UNSUPPORTED = frozenset({ + "app.globalFontMagnification", + "automation.codexIntegration", + "cloud.beta.machines.enabled", + "computerUse.enabled", + "computerUse.showInMenuBar", + "customSidebars.renderer", + "shortcuts.showModifierHoldHints", +}) + + +class ConfigurationReviewPathsTests(unittest.TestCase): + def test_every_advertised_path_is_supported(self): + supported, unresolved = supported_paths() + self.assertTrue(supported, "parsed no supported paths; the guard would pass vacuously") + missing = [] + for value, rel, line in advertised_paths(): + if value in KNOWN_UNSUPPORTED: + continue + # Object-valued settings are listed at their root, and the store + # permits descendant paths beneath them (e.g. shortcuts.bindings). + parts = value.split(".") + ancestors = {".".join(parts[: i + 1]) for i in range(len(parts))} + if not (ancestors & supported): + missing.append(f"{rel}:{line} advertises {value!r}") + self.assertEqual( + missing, + [], + "settings rows advertise cmux.json paths the file store does not accept, " + "so writing them into cmux.json does nothing. Add each to " + "`supportedSettingsJSONPaths` and to the matching " + "`*SettingsFileMapping` in CmuxSettingsJSONPathSupport.swift.\n " + + "\n ".join(missing) + + ( + "\n(unresolved symbolic entries in the supported set: " + + ", ".join(unresolved) + + ")" + if unresolved + else "" + ), + ) + + + def test_known_unsupported_list_has_no_stale_entries(self): + """A fixed row must be removed from the allowlist, so it can only shrink.""" + supported, _ = supported_paths() + advertised = {value for value, _, _ in advertised_paths()} + stale = sorted( + path + for path in KNOWN_UNSUPPORTED + if path not in advertised or path in supported + ) + self.assertEqual( + stale, + [], + "these paths are no longer unsupported-and-advertised, so delete them " + "from KNOWN_UNSUPPORTED: " + ", ".join(stale), + ) + + +if __name__ == "__main__": + unittest.main() From 1feaa1a55a8b669fdb9b4aa65e9f048e9b8b3e7e Mon Sep 17 00:00:00 2001 From: Leo Li Date: Wed, 23 Sep 2026 03:57:57 -0700 Subject: [PATCH 2/3] ci: run the settings configuration-review guard in the preflight lane 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 --- .github/workflows/ci-guards.yml | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/.github/workflows/ci-guards.yml b/.github/workflows/ci-guards.yml index e90310ab6b39..0c3b40c88b31 100644 --- a/.github/workflows/ci-guards.yml +++ b/.github/workflows/ci-guards.yml @@ -159,6 +159,10 @@ jobs: if: ${{ matrix.group == 'preflight' }} run: python3 tests/test_write_sidebar_extension_point.py + - name: Validate settings rows advertise supported cmux.json paths + if: ${{ matrix.group == 'preflight' }} + run: python3 tests/test_settings_configuration_review_paths.py + - name: Validate Localizable.xcstrings catalog structure if: ${{ matrix.group == 'preflight' }} run: python3 tests/test_localizable_xcstrings_structure.py From 6cdb5259c1bded55acc49ffdc014824b63411a6f Mon Sep 17 00:00:00 2001 From: Leo Li Date: Wed, 23 Sep 2026 08:06:18 -0700 Subject: [PATCH 3/3] test(settings): say what this guard actually proves, and stop two false 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 --- ...CmuxSettingsFileStore+SupportedPaths.swift | 2 + ...est_settings_configuration_review_paths.py | 77 +++++++++++++++---- 2 files changed, 62 insertions(+), 17 deletions(-) diff --git a/Sources/CmuxSettingsFileStore+SupportedPaths.swift b/Sources/CmuxSettingsFileStore+SupportedPaths.swift index dac7a82b4913..5236980a255b 100644 --- a/Sources/CmuxSettingsFileStore+SupportedPaths.swift +++ b/Sources/CmuxSettingsFileStore+SupportedPaths.swift @@ -28,6 +28,7 @@ extension CmuxSettingsFileStore { "app.reorderOnNotification", "app.sendAnonymousTelemetry", "app.confirmQuit", + "app.globalFontMagnification", "app.warnBeforeQuit", "app.warnBeforeClosingTab", "app.warnBeforeClosingTabXButton", @@ -153,5 +154,6 @@ extension CmuxSettingsFileStore { "fileEditor.tabWidth", "fileExplorer.doubleClickAction", "shortcuts.bindings", + "shortcuts.showModifierHoldHints", ] } diff --git a/tests/test_settings_configuration_review_paths.py b/tests/test_settings_configuration_review_paths.py index 1d6b61cc5ef2..897879c6e350 100755 --- a/tests/test_settings_configuration_review_paths.py +++ b/tests/test_settings_configuration_review_paths.py @@ -1,12 +1,26 @@ #!/usr/bin/env python3 -"""Every cmux.json path a settings row advertises must be one the store accepts. +"""Every cmux.json path a settings row advertises must appear in the declared +supported-path set. `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 that. A row could declare -`configurationReview: .json("terminal.textEditingGestures")` while the store -rejected the path, so the row displayed a cmux.json key that silently did -nothing when a user wrote it. +review." Nothing enforced that, so a row could advertise a path absent from the +set and nobody noticed. + +Be precise about what this proves. `supportedSettingsJSONPaths` has **no +production consumer** -- it is read by this guard and one test, 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 is a consistency check between +two declarations (the UI row and the documented set), not proof that writing the +key does anything. + +The gap is real in both directions. `canvas.paneGap` and +`canvas.snappingEnabled` are advertised by rows, are listed in the supported +set, and therefore pass this guard -- yet `root["canvas"]` is never read by any +parser, so writing them does nothing. Catching that class needs an oracle +derived from the parsers; see the tracking issue. Until then, a pass here means +"the row and the documented set agree", nothing stronger. """ import re @@ -26,18 +40,32 @@ def _resolve_symbol(type_name, member): - """Find `static let = ""` inside ``'s file.""" + """Find `static let = ""` in the file that DECLARES the type. + + Matching on "the file mentions the type name" picks the first file in + filesystem order that merely references it, which is both wrong and + machine-dependent. `settingsPath` is already declared by two different + types, so the collision class exists. A decoy that resolves to a shorter + path would silently widen the ancestor match below and hide real failures, + so an ambiguous resolution is reported rather than guessed at. + """ + declares = re.compile( + r"\b(?:enum|struct|class|extension|actor|protocol)\s+" + re.escape(type_name) + r"\b" + ) pattern = re.compile( r"static\s+let\s+" + re.escape(member) + r"\s*(?::\s*String\s*)?=\s*\"([^\"]+)\"" ) + values = set() for root in SOURCE_ROOTS: for path in root.rglob("*.swift"): text = path.read_text(encoding="utf-8", errors="replace") - if type_name not in text: + if not declares.search(text): continue found = pattern.search(text) if found: - return found.group(1) + values.add(found.group(1)) + if len(values) == 1: + return values.pop() return None @@ -73,19 +101,24 @@ def advertised_paths(): yield value, path.relative_to(REPO_ROOT), line -# Rows that already advertised an unsupported path when this guard was added. -# Each is a real defect: the row shows a cmux.json key that does nothing when a -# user writes it. They are recorded rather than fixed here so the guard can stop -# new instances immediately; see the tracking issue. Fixing one means deleting -# its entry below, which this test enforces, so the list can only shrink. +# Rows advertising a path absent from the supported set when this guard was +# added. Each was checked against the parsers by hand: none of these five has a +# reader, so writing them into cmux.json genuinely does nothing. +# `cloud`, `computerUse` and `customSidebars` have no top-level case in the +# section dispatch at all, and the parsed `automation` section has no +# `codexIntegration` key. See the tracking issue. +# +# This list is NOT automatically ratcheted -- nothing compares it to a baseline, +# so a new failure could be parked here in the same change that introduces it. +# The staleness test below only reaps entries that have since become supported +# or are no longer advertised. Treat additions as needing review on their own +# merits. KNOWN_UNSUPPORTED = frozenset({ - "app.globalFontMagnification", "automation.codexIntegration", "cloud.beta.machines.enabled", "computerUse.enabled", "computerUse.showInMenuBar", "customSidebars.renderer", - "shortcuts.showModifierHoldHints", }) @@ -122,13 +155,23 @@ def test_every_advertised_path_is_supported(self): def test_known_unsupported_list_has_no_stale_entries(self): - """A fixed row must be removed from the allowlist, so it can only shrink.""" + """A path that became supported, or lost its row, must leave the list.""" supported, _ = supported_paths() advertised = {value for value, _, _ in advertised_paths()} + + def is_supported(path): + # Mirror the ancestor matching the main test uses. Checking exact + # membership instead would strand an entry that became supported + # via an ancestor, leaving it permanently un-reapable dead weight. + parts = path.split(".") + return bool( + {".".join(parts[: i + 1]) for i in range(len(parts))} & supported + ) + stale = sorted( path for path in KNOWN_UNSUPPORTED - if path not in advertised or path in supported + if path not in advertised or is_supported(path) ) self.assertEqual( stale,