Repository navigation
Let Computer Use toggles save past unrelated cmux.json issues - #14183
Conversation
A Computer Use toggle, or a cmux-settings write, should not be refused because cmux.json already had an unrelated issue such as a key from a newer build. It should still be refused when the change itself creates one. Also covers an existing --receipt file returning a structured conflict and every helper message reading as a complete sentence. These fail on main: the whole-file check refuses the unrelated issue, the receipt collision raises FileExistsError, and undoConflict ends in a colon with nothing after it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The Computer Use toggles validated the whole cmux.json, so an issue the toggle never touched (a key from a newer build, an invalid value on another path) blocked it with an alert. The store now validates the candidate and, only when it has issues, the file on disk, and refuses the issues the change adds, identified by path and message. The cmux-settings helper follows the same rule. A validator run that fails without per-path issues still refuses the write outright. An invalid_config refusal is now a structured JSON conflict listing the added issues instead of free-text error lines, a receipt path that exists or can't be created returns a structured conflict instead of a traceback, and undoConflict reads as a complete sentence in all nine locales. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
All contributors have signed the CLA ✍️ ✅ |
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: manaflow-ai/cmux/.coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (11)
Included review availability: Your plan provides up to 10 included reviews per hour; 1 remains after this review. 📝 WalkthroughWalkthroughNative settings writes and the cmux-settings helper now reject only validation issues introduced by a change. The helper also returns distinct conflicts for existing receipt files and receipt paths that cannot be created. ChangesConfig mutation validation
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Bug fix Sequence Diagram(s)sequenceDiagram
participant MutationCaller
participant JSONConfigStore
participant CmuxConfigSemanticValidator
participant GlobalSchema
participant ConfigFile
MutationCaller->>JSONConfigStore: Request mutation
JSONConfigStore->>CmuxConfigSemanticValidator: Compare candidate with baseline
CmuxConfigSemanticValidator->>GlobalSchema: Validate candidate
GlobalSchema-->>CmuxConfigSemanticValidator: Candidate issues
opt Candidate has issues
CmuxConfigSemanticValidator->>GlobalSchema: Validate baseline
GlobalSchema-->>CmuxConfigSemanticValidator: Baseline issues
end
CmuxConfigSemanticValidator-->>JSONConfigStore: Return introduced issues
alt No introduced issues
JSONConfigStore->>ConfigFile: Write candidate
else Introduced issues exist
JSONConfigStore-->>MutationCaller: Reject mutation with issues
end
Merge Risk: ⚪ Minimal · up to The reported undo and validation behaviors match their documented contracts. The change is mergeable after normal checks. Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (2 errors, 2 warnings)
✅ Passed checks (21 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 14.81% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 27 functions across 7 files. (4 skipped: 4 unsupported.) Full details: Cmux Algorithmic ComplexityExplanation
Resolution Build a hash-based identity set for baseline issues once, using each issue's Full details: Cmux User-Facing Error PrivacyExplanation The production Resolution Do not forward validator stderr, stdout, or untrusted validator messages to users. Convert validator failures to a fixed, product-level message with safe recovery guidance, and keep raw details in sanitized internal diagnostics only. Apply the same sanitization to Full details: Description checkExplanation The description gives a detailed summary, testing results, known limitation, and localization status. It does not include the required Demo Video or screenshots section, the Checklist section, or an explicit Testing heading. Resolution Add the required Demo Video section with a video or screenshots for this behavior change. Add the Checklist section and mark each applicable item, including test coverage, localization audit, documentation, and review status. Rename Verification to Testing or provide the required Testing section explicitly.
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
teamleaderleo
left a comment
teamleaderleo
left a comment
There was a problem hiding this comment.
Independent review of c0c5c8b. No blocking findings; this is ready to merge.
Issue identity. issuesIntroduced(by:over:) (CmuxConfigSemanticValidator+IntroducedIssues.swift:26-27) and the helper's known set (cmux-settings:672) do a set difference on path + message. That is enough because the validator already deduplicates its output, and every message is built from the schema constraint or the value's JSON type, never from the value itself. A new issue can only match an existing one at the same path. So the gap is confined to a subtree that already fails validation, which is the limit docs/config-transactions.md states. One more instance of that limit: an object already over maxProperties (notifications.soundOverrides, 256) reports the same $.notifications.soundOverrides: must contain at most 256 key(s) after each added key, so the write can keep growing it. Adding a key there already requires the file to be invalid, and a toggle write on another path can't make a valid file invalid, so I don't think this needs code, but the docs could name it.
Race. Swift validates the baseline against document.root, the same parse the candidate starts from. That parse happens under JSONConfigWriteLock (JSONConfigStore.swift:424, :498), and publish(..., expected: document.originalData) (:511) refuses the write if another editor changed the file. The helper takes baseline from the same load_settings result inside mutation_lock (cmux-settings:783, :790). The revision is read just before that, and the write checks it (:872). If a non-cooperating editor lands between those two reads, the write is refused. It is never accepted against a stale baseline.
No-op writes. The jsonObjectsEqual early return (JSONConfigStore.swift:461) publishes nothing, so skipping validation there can't change what's on disk. The helper already skipped validation on unchanged writes before this PR.
Output consumers. Only tests/test_cli_config_doctor.py:376 and :396 read the helper's stderr. They check for the substring $.app.appearance, which the JSON issues array still contains, and validate keeps its error: lines. The skills and docs only mention the new shape.
Evidence. The helper tests passed at the PR head in a separate worktree: test_cmux_settings_transactions.py (22), test_cmux_settings_jsonc.py (20), and test_cmux_settings_supported_paths.py (7). In CI, the macos / swift-package-tests log (job 107554355079) shows that validatedMutationRefusesOnlyIssuesItIntroduces() passed in CmuxSettings (401 tests). validatedModelWritesPastUnrelatedPreExistingIssues() and validatedModelRefusesAnIssueItIntroduces() passed in CmuxSettingsUI (184 tests). These are executed tests, not just compiled ones.
Not a regression, but worth knowing: an undo whose receipt replaced an invalid value (neon → dark) is still refused, because restoring neon adds an issue the file doesn't have. The old whole-file check refused this too.
ff6c6dc ci: stop restoring an iOS GhosttyKit cache nothing saves (manaflow-ai#14189) 2827231 ci: seed DerivedData on the 12 vCPU macOS 26 pool (manaflow-ai#14188) ae46aa9 Let Computer Use toggles save past unrelated cmux.json issues (manaflow-ai#14183) a785270 test: fail loudly when portal rendering authority denies a fixture's tab id (manaflow-ai#13937) 9a1dea0 test: pin which terminal tabs get an agent mark after manaflow-ai#14062 (manaflow-ai#14177) 91bcb28 test: run the change-area tests in parallel workers (manaflow-ai#14193) 6d203e8 test: await the geometry publish in the equalize-splits shortcut case (manaflow-ai#13916) 48f1adf ci: balance the guard legs the macOS gate waits on (manaflow-ai#14186) 38117cd test: settle the split's reparent-focus suppression before focus feedback (manaflow-ai#14049) a12a0b8 ci: neutralize Swift sources without a per-character loop (manaflow-ai#14169) 2b6ca4c ci: stop counting queue time on cancelled jobs as runner minutes (manaflow-ai#14187) 2837f22 test: stop gating terminal focus on key status the app host cannot grant (manaflow-ai#13948) 23c0ce2 test: give each detect-step run its own cmux-ci scratch files (manaflow-ai#14185) a13ea28 ci: skip Mac lanes that bundled scripts and guard-only lints cannot fail (manaflow-ai#14179) b59f34f ci: restore Swift packages and a compilation cache for iOS uploads (manaflow-ai#14180) bcca243 profiling: poll child processes every 0.1 s instead of every second (manaflow-ai#14170) 76d6176 refactor: move 45 leaf browser files into CmuxBrowser (manaflow-ai#14092) bf13034 test: fail the Desktop drop fast instead of restarting the app host (manaflow-ai#14076) 51d486b ci: start guards and web beside Fast static checks (manaflow-ai#14176) # Conflicts: # .github/workflows/ci-guards.yml # .github/workflows/ci.yml # .github/workflows/ios-appstore-upload.yml # .github/workflows/ios-testflight.yml # .github/workflows/nightly.yml # .github/workflows/seed-derived-data.yml # .github/workflows/test-ios.yml
1 participant
Follow-up to #13254. The Computer Use toggles in Settings refused to save whenever cmux.json already had an unrelated issue, such as a key written by a newer cmux build or an invalid value on another path, because the validated write checked the whole file. Now a validated write is refused only for issues the change itself adds.
JSONConfigStore.setWithReceipt,resetWithReceipt, andundovalidate the candidate. If it has issues, they also validate the file on disk and refuse only the candidate's issues the file didn't already have. An issue is identified by its path and message. The validator builds messages from the schema constraint, not the config value, so the pair is stable. A valid candidate still costs one validation. A no-op write can't add an issue, so it's no longer validated.The
cmux-settingshelper validated the whole file before #13254 too. I moved it to the same rule so the helper and the app agree on what blocks a write. The extracmux config validaterun happens only when the candidate already fails. If a validator run fails without reporting per-path issues (an older CLI, a crash), the write is still refused outright, because there's nothing to compare. Three more helper fixes from the #13254 review:{"status": "conflict", "code": "invalid_config", ..., "issues": [{"path", "message"}]}on stderr instead of free-texterror:lines.cmux-settings validatekeeps its human-readable output.--receiptfile now returns areceipt_existsconflict before anything is written, instead of an uncaughtFileExistsErrortraceback. The newreceiptExistsmessage is in all 9 locales. A receipt path that can't be created (missing parent, permissions) returnsreceipt_unwritablewith the existingreceiptInvalidmessage.undoConflictmessage ended in "…before changing it:" with nothing after the colon. It's now a complete sentence in all 9 locales. The key is still returned in its own field. The app's Swift string is unchanged because the app appends the path after the colon.Verification
test(settings)(614c1df) adds the regressions first. On that commit, all 4 new helper tests fail for the expected reasons: the unrelated issue refuses the write, the write printserror:text instead of JSON, the conflict for an existing receipt isn't structured, anden.undoConflictends in a colon. The fix commit adds tests for a validator that fails without per-path issues and for a receipt path that can't be created. At HEAD,tests/test_cmux_settings_transactions.py(22),test_cmux_settings_jsonc.py(20), andtest_cmux_settings_supported_paths.py(7) pass. The new helper tests run the real validation path against a fakecmux config validate.JSONValueModelTransactionTestsnow asserts that a toggle writes past an unknown key plus an invalidapp.appearance, and that a write adding an issue is refused with only that issue. The test that asserted the old refusal is gone.JSONConfigTransactionTests.validatedMutationRefusesOnlyIssuesItIntroducescovers the store directly. These haven't run locally: CmuxSettings doesn't build on Linux because of SwiftUI, Darwin, andString(localized:bundle:). Instead, I compiled the real validator sources, the embedded schema, and the newissuesIntroduced(by:over:)file on Linux (Swift 6.1.3) with a localization shim, and ran them against the real schema. For a baseline with an unknown key and an invalid appearance, the toggle added no issues. A mistypednotifications.dockBadgeadded only$.notifications.dockBadge: expected boolean, got string. The package tests run in CI.docs/config-transactions.md.python3 scripts/verify-local.py: all checks pass (test-wiringpassed with--timeout 1100; it exceeds the default 60 s here). A local sweep of the workflow guard steps found no failures caused by this change. The steps that failed need CI env vars (RUNNER_TEMP,CMUX_TEST_REGISTRY_BASE_SHA) or submodules this worktree doesn't have, or timed out locally.config_mutation_messages.jsonhas the same keys in all 9 locales (en, ja, de, fr, es, ar, zh-Hans, zh-Hant, ko), and a test enforces that no message ends in a colon.Resources/Localizable.xcstringsis unchanged. The xcstrings and localization parity checks pass.🤖 Generated with Claude Code
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Summary by CodeRabbit
Bug Fixes
Documentation