Repository navigation
Add cmux config doctor CLI - #3454
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds a top-level ChangesConfig command, doctor workflow, JSONC and CLI wiring
Sequence Diagram(s)sequenceDiagram
autonumber
participant User as "User (shell)"
participant CLI as "cmux CLI"
participant FS as "File system"
participant JSONC as "JSONCParser"
participant Socket as "App socket"
rect rgba(200,200,255,0.5)
User->>CLI: cmux config doctor [--path <file>] [--json]
CLI->>FS: discover candidate config paths
FS-->>CLI: candidate path list
CLI->>JSONC: preprocess & parse target file
JSONC-->>CLI: parsed payload or error
CLI->>User: print JSON or human-readable report
end
rect rgba(200,255,200,0.5)
User->>CLI: cmux config reload
CLI->>Socket: connect (socketPath)
CLI->>Socket: send "reload_config"
Socket-->>CLI: ack / error
CLI->>User: print reload result
end
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly Related PRs
Poem
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (11 passed)
✨ Finishing Touches🧪 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 |
Greptile SummaryThis PR adds
Confidence Score: 5/5Safe to merge — the new config command is cleanly isolated, the socket-routing logic is correct, and the JSONCParser tightening is well-scoped. All changed paths behave correctly under their tested inputs. The two findings are minor inconsistencies (a missing byte count in one error branch, and an unguarded empty-string argument) that don't affect exit codes, discovery, or correctness of the validation result. No blocking concerns in routing, parsing, or file I/O. No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["cmux config <args>"] --> B{configCommandDoesNotNeedSocket?}
B -- yes --> C[runConfigCommand\nsocketPath: nil]
B -- no --> D[Resolve socket path]
D --> E[runConfigCommand\nsocketPath: resolved]
C --> F{subcommand}
E --> F
F -- "help / --help" --> G[print configUsage]
F -- "path / paths" --> H[printSettingsPaths]
F -- "docs / documentation" --> I[runDocsCommand settings]
F -- "doctor / check / validate" --> J[runConfigDoctor]
F -- reload --> K{socketPath present?}
F -- unknown --> L[throw CLIError]
K -- no --> M[throw: requires socket]
K -- yes --> N[client.send reload_config]
J --> O{--path given?}
O -- yes --> P[explicit targets\nmissingIsError: true]
O -- no --> Q[defaultConfigDoctorTargets\nprimary + project walk + legacy]
P --> R[configDoctorFinding per target]
Q --> R
R --> S{errorCount > 0?}
S -- yes --> T[print report\nthrow CLIError exit 1]
S -- no --> U[print report\nexit 0]
Reviews (4): Last reviewed commit: "Document config doctor output contract" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| guard args.count == 1 else { | ||
| throw CLIError(message: "Usage: cmux config docs") | ||
| } | ||
| if wantsJSON, let reference = docsReference(for: "settings") { | ||
| print(jsonString(docsPayload(reference))) | ||
| } else if let reference = docsReference(for: "settings") { | ||
| printDocsReference(reference) | ||
| } |
There was a problem hiding this comment.
docs subcommand when reference is absent
Both branches of the docs case are if wantsJSON, let reference = ... / else if let reference = ..., so if docsReference(for: "settings") ever returns nil the command exits 0 with no output and no error message. The runDocsCommand path correctly throws a CLIError when the reference is missing; the same guard-and-throw pattern should be used here for consistency and debuggability.
// suggested approach
guard let reference = docsReference(for: "settings") else {
throw CLIError(message: "Settings docs reference not found.")
}
if wantsJSON {
print(jsonString(docsPayload(reference)))
} else {
printDocsReference(reference)
}There was a problem hiding this comment.
Addressed by routing config docs through runDocsCommand, which already guards the missing reference path and throws a CLIError.
— Claude Code
| launchIfNeeded: false | ||
| ) | ||
| defer { client.close() } | ||
| let response = try client.send(command: "reload_config") | ||
| if response.hasPrefix("ERROR:") { |
There was a problem hiding this comment.
configCommandDoesNotNeedSocket defaults to true for unrecognized subcommands
configCommandDoesNotNeedSocket returns true for every subcommand that is not literally "reload". If a future subcommand added to runConfigCommand needs a real socket and the developer forgets to update this function, it will silently receive CLISocketPathResolver.defaultSocketPath instead of the env-resolved path — wrong socket, no compile-time or runtime warning. Consider an allowlist of no-socket subcommands, or at minimum add a comment cross-referencing both functions so authors know they must be kept in sync.
| config_path = home / ".config" / "cmux" / "cmux.json" | ||
| config_path.parent.mkdir(parents=True) | ||
| config_path.write_text( | ||
| """ | ||
| { | ||
| // JSONC comments and trailing commas are valid in cmux.json. | ||
| "schemaVersion": 1, | ||
| "app": { | ||
| "appearance": "system", | ||
| }, | ||
| } | ||
| """, | ||
| encoding="utf-8", | ||
| ) | ||
|
|
||
| ok_result = run_cli(cli_path, ["--json", "config", "doctor", "--path", str(config_path)], home) | ||
| if ok_result.returncode != 0: | ||
| failures.append(f"valid JSONC returned {ok_result.returncode}: {ok_result.stderr}") | ||
| else: | ||
| payload = json.loads(ok_result.stdout) | ||
| finding = payload["findings"][0] | ||
| if payload["ok"] is not True or finding["status"] != "ok": | ||
| failures.append(f"valid JSONC was not ok: {ok_result.stdout}") | ||
| if "app" not in finding["keys"] or "schemaVersion" not in finding["keys"]: | ||
| failures.append(f"valid JSONC keys missing: {ok_result.stdout}") | ||
|
|
||
| config_path.write_text("{\n", encoding="utf-8") | ||
| bad_result = run_cli(cli_path, ["--json", "config", "doctor", "--path", str(config_path)], home) | ||
| if bad_result.returncode == 0: | ||
| failures.append("invalid JSON returned success") | ||
| else: | ||
| payload = json.loads(bad_result.stdout) | ||
| finding = payload["findings"][0] | ||
| if payload["ok"] is not False or finding["status"] != "error": | ||
| failures.append(f"invalid JSON did not report an error: {bad_result.stdout}") | ||
| if "cmux config doctor found 1 error(s)" not in bad_result.stderr: | ||
| failures.append(f"invalid JSON stderr was unexpected: {bad_result.stderr}") | ||
|
|
||
| if failures: |
There was a problem hiding this comment.
The test only exercises the --path <explicit> code path. defaultConfigDoctorTargets, findProjectConfigPath, and the legacy-path deduplication logic (the most complex parts of the feature) have zero coverage. Running cmux config doctor with no flags — the most common invocation — is never called. At minimum, a third case invoking run_cli(cli_path, ["--json", "config", "doctor"], home) against the synthesised ~/.config/cmux/cmux.json would cover the primary-config scan path.
There was a problem hiding this comment.
Addressed by adding the no-flag config doctor path to tests/test_cli_config_doctor.py, including primary config discovery from a nested workspace.
— Claude Code
| var paths: [String] = [] | ||
| var index = 0 | ||
| while index < arguments.count { | ||
| let argument = arguments[index] | ||
| if argument == "--path" { |
There was a problem hiding this comment.
parseConfigDoctorOptions appends any non-flag argument directly to paths, so cmux config doctor /path/to/file works as a positional shorthand for --path /path/to/file. Neither configUsage() nor docs/cli-contract.md documents this form. Either remove the positional fallback to keep the interface tight, or add it to the help string and CLI contract.
There was a problem hiding this comment.
Addressed by rejecting positional config doctor paths and keeping --path as the only file argument form.
— Claude Code
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
CLI/cmux.swift (1)
2066-2075: 💤 Low valueConsider adding a clarifying comment for the two-phase
configdispatch.The intent here is an optimization: skip socket resolution for subcommands that don't need it (doctor, path, docs, help). The same
command == "config"check appears again after socket resolution (Line 2117) for thereloadcase. Without a comment, a reader may not immediately understand why there are two separateconfigblocks.Also,
CLISocketPathResolver.defaultSocketPathis passed as a placeholder since it won't be used by any branch reached via theconfigCommandDoesNotNeedSocketguard, but a comment would make that intention explicit and prevent a future maintainer from accidentally adding a socket-dependent subcommand torunConfigCommandwithout updatingconfigCommandDoesNotNeedSocket.✏️ Suggested clarification
+ // Early-return for config subcommands that don't need a live socket + // (doctor, path, docs, help, unknown). The socketPath arg is unused by + // these branches; "reload" falls through to the post-resolution block below. if command == "config", configCommandDoesNotNeedSocket(commandArgs) { try runConfigCommand( commandArgs: commandArgs, - socketPath: CLISocketPathResolver.defaultSocketPath, + socketPath: CLISocketPathResolver.defaultSocketPath, // unused for no-socket cmds explicitPassword: socketPasswordArg, jsonOutput: jsonOutput ) return }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CLI/cmux.swift` around lines 2066 - 2075, Add a clarifying inline comment above the first `if command == "config", configCommandDoesNotNeedSocket(commandArgs) { ... }` explaining this is a two-phase dispatch: we short-circuit and call `runConfigCommand` with a placeholder `CLISocketPathResolver.defaultSocketPath` for subcommands that do not need the socket (e.g., doctor/path/docs/help) to avoid premature socket resolution, and note that `config` is checked again later after socket resolution to handle socket-dependent subcommands like `reload`; also add a reminder to update `configCommandDoesNotNeedSocket` if any new config subcommand begins to require the socket so callers of `runConfigCommand` aren’t inadvertently passed a placeholder path.docs/cli-contract.md (1)
316-318: ⚡ Quick winConsider adding a "Config subcommands" section to "Command Families".
Every other multi-subcommand family (Auth, VM, Themes, Browser, Hooks, Docs, Settings) has a dedicated table in the "Command Families" section documenting each subcommand's contract.
configonly appears in the help probe string, leavingdoctor,path,docs, andreloadcontracts undocumented in a scannable form.📄 Suggested addition (after the Settings subcommands table, ~line 292)
+Config subcommands: + +| Command | Contract | +| --- | --- | +| `config doctor [--path <file>]`, `config check`, `config validate` | Validate JSONC syntax of one or more config files. Uses default discovery when `--path` is absent. Exits 0 on success, 1 on any error. Supports `--json`. Works without a socket. | +| `config path` | Print cmux.json paths, docs URL, schema URL, backup reminder, and reload command without a socket. | +| `config docs` | Print the same output as `docs settings` without a socket. | +| `config reload` | Ask cmux to reload configuration. Requires a socket. |🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/cli-contract.md` around lines 316 - 318, Add a new "Config subcommands" section in docs/cli-contract.md (placed after the Settings subcommands table) that documents each config subcommand in a scannable table format: list `doctor`, `path`, `docs`, and `reload` as rows with their Usage, Description, and Flags/Args columns; ensure the table mirrors the style and column names used by other command-family tables (e.g., Auth/VM/Themes) so readers can quickly see the contract for Config subcommands.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CLI/CMUXCLI`+DocsSettings.swift:
- Around line 784-790: In configDoctorErrorMessage(_ error: Error) prefer
extracting the parser-specific debug message from (error as
NSError).userInfo[NSDebugDescriptionErrorKey] first and return it if non-empty;
if that key is absent or empty, fall back to String(describing: error) and then
finally to nsError.localizedDescription, ensuring parser-specific JSON errors
are preserved for cmux config doctor.
- Around line 646-664: The search in findProjectConfigPath currently walks up to
root and can match directories; change it to stop before entering the user's
home directory (use FileManager.default.homeDirectoryForCurrentUser.path and
compare standardized paths and return nil when current == home) and when testing
each candidate, use fileExists(atPath:isDirectory:) to ensure the candidate
exists and is not a directory (isDirectory == false) before returning its
standardized path; update references in the function to use these checks for the
candidates array (the ".cmux/cmux.json" and "cmux.json" entries).
In `@tests/test_cli_config_doctor.py`:
- Around line 76-81: Wrap the unguarded JSON parsing and list access in
try/except blocks to catch json.JSONDecodeError and IndexError: around the
json.loads(ok_result.stdout) and the finding = payload["findings"][0] access,
catch these exceptions and append a helpful failure string to failures
(including ok_result.stdout and the exception message) instead of letting the
exception propagate; repeat the same pattern for the second block that parses
the other result (the second json.loads and findings[0]) so both locations use
guarded parsing and clearly report FAIL entries on parse/index errors.
---
Nitpick comments:
In `@CLI/cmux.swift`:
- Around line 2066-2075: Add a clarifying inline comment above the first `if
command == "config", configCommandDoesNotNeedSocket(commandArgs) { ... }`
explaining this is a two-phase dispatch: we short-circuit and call
`runConfigCommand` with a placeholder `CLISocketPathResolver.defaultSocketPath`
for subcommands that do not need the socket (e.g., doctor/path/docs/help) to
avoid premature socket resolution, and note that `config` is checked again later
after socket resolution to handle socket-dependent subcommands like `reload`;
also add a reminder to update `configCommandDoesNotNeedSocket` if any new config
subcommand begins to require the socket so callers of `runConfigCommand` aren’t
inadvertently passed a placeholder path.
In `@docs/cli-contract.md`:
- Around line 316-318: Add a new "Config subcommands" section in
docs/cli-contract.md (placed after the Settings subcommands table) that
documents each config subcommand in a scannable table format: list `doctor`,
`path`, `docs`, and `reload` as rows with their Usage, Description, and
Flags/Args columns; ensure the table mirrors the style and column names used by
other command-family tables (e.g., Auth/VM/Themes) so readers can quickly see
the contract for Config subcommands.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1ad9b1f9-2539-4fa0-8f3e-438fc5f851f4
📒 Files selected for processing (5)
CLI/CMUXCLI+DocsSettings.swiftCLI/cmux.swiftGhosttyTabs.xcodeproj/project.pbxprojdocs/cli-contract.mdtests/test_cli_config_doctor.py
| payload = json.loads(ok_result.stdout) | ||
| finding = payload["findings"][0] | ||
| if payload["ok"] is not True or finding["status"] != "ok": | ||
| failures.append(f"valid JSONC was not ok: {ok_result.stdout}") | ||
| if "app" not in finding["keys"] or "schemaVersion" not in finding["keys"]: | ||
| failures.append(f"valid JSONC keys missing: {ok_result.stdout}") |
There was a problem hiding this comment.
Unhandled json.JSONDecodeError and IndexError produce opaque CI failures instead of clean FAIL: messages.
Both json.loads() calls (lines 76 and 88) and both findings[0] accesses (lines 77 and 89) are unguarded. If the CLI emits non-JSON output (e.g., an unexpected crash message, a migration banner, or empty stdout on error), the exception propagates out of main() and exits with a Python traceback rather than a diagnostic FAIL: entry — making it harder to distinguish a test-infrastructure problem from a feature regression.
🛡️ Proposed fix with guarded JSON parsing helper
+def _parse_json_output(raw: str, label: str, failures: list[str]) -> dict | None:
+ try:
+ return json.loads(raw)
+ except json.JSONDecodeError as exc:
+ failures.append(f"{label}: stdout is not valid JSON ({exc}): {raw!r}")
+ return None
+
+
def main() -> int:
...
ok_result = run_cli(cli_path, ["--json", "config", "doctor", "--path", str(config_path)], home)
if ok_result.returncode != 0:
failures.append(f"valid JSONC returned {ok_result.returncode}: {ok_result.stderr}")
else:
- payload = json.loads(ok_result.stdout)
- finding = payload["findings"][0]
- if payload["ok"] is not True or finding["status"] != "ok":
- failures.append(f"valid JSONC was not ok: {ok_result.stdout}")
- if "app" not in finding["keys"] or "schemaVersion" not in finding["keys"]:
- failures.append(f"valid JSONC keys missing: {ok_result.stdout}")
+ payload = _parse_json_output(ok_result.stdout, "valid JSONC", failures)
+ if payload is not None:
+ findings = payload.get("findings", [])
+ if not findings:
+ failures.append(f"valid JSONC: findings array is empty: {ok_result.stdout}")
+ else:
+ finding = findings[0]
+ if payload.get("ok") is not True or finding.get("status") != "ok":
+ failures.append(f"valid JSONC was not ok: {ok_result.stdout}")
+ if "app" not in finding.get("keys", []) or "schemaVersion" not in finding.get("keys", []):
+ failures.append(f"valid JSONC keys missing: {ok_result.stdout}")
config_path.write_text("{\n", encoding="utf-8")
bad_result = run_cli(cli_path, ["--json", "config", "doctor", "--path", str(config_path)], home)
if bad_result.returncode == 0:
failures.append("invalid JSON returned success")
else:
- payload = json.loads(bad_result.stdout)
- finding = payload["findings"][0]
- if payload["ok"] is not False or finding["status"] != "error":
- failures.append(f"invalid JSON did not report an error: {bad_result.stdout}")
- if "cmux config doctor found 1 error(s)" not in bad_result.stderr:
- failures.append(f"invalid JSON stderr was unexpected: {bad_result.stderr}")
+ payload = _parse_json_output(bad_result.stdout, "invalid JSON", failures)
+ if payload is not None:
+ findings = payload.get("findings", [])
+ if not findings:
+ failures.append(f"invalid JSON: findings array is empty: {bad_result.stdout}")
+ else:
+ finding = findings[0]
+ if payload.get("ok") is not False or finding.get("status") != "error":
+ failures.append(f"invalid JSON did not report an error: {bad_result.stdout}")
+ if "cmux config doctor found 1 error(s)" not in bad_result.stderr:
+ failures.append(f"invalid JSON stderr was unexpected: {bad_result.stderr}")Also applies to: 88-93
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/test_cli_config_doctor.py` around lines 76 - 81, Wrap the unguarded
JSON parsing and list access in try/except blocks to catch json.JSONDecodeError
and IndexError: around the json.loads(ok_result.stdout) and the finding =
payload["findings"][0] access, catch these exceptions and append a helpful
failure string to failures (including ok_result.stdout and the exception
message) instead of letting the exception propagate; repeat the same pattern for
the second block that parses the other result (the second json.loads and
findings[0]) so both locations use guarded parsing and clearly report FAIL
entries on parse/index errors.
There was a problem hiding this comment.
Addressed by adding guarded JSON parsing and finding extraction helpers so failures report clean FAIL entries instead of tracebacks.
— Claude Code
There was a problem hiding this comment.
@lawrencecchen, great — the guarded parsing helpers (parse_json_output and first_finding) are exactly the right pattern here. Clean FAIL: diagnostics will make CI failures much easier to triage. Thanks for the follow-up!
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
CLI/CMUXCLI+Config.swift (1)
431-444:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPrefer
String(describing:)beforelocalizedDescriptionhere.
localizedDescriptionfrequently collapses JSON/Cocoa parse failures into a generic message, which makesconfig doctorless actionable. Keep the debug description first, then fall back toString(describing: error), and only uselocalizedDescriptionlast.Suggested fix
private static func configDoctorErrorMessage(_ error: Error) -> String { let nsError = error as NSError if let debug = nsError.userInfo[NSDebugDescriptionErrorKey] as? String { let trimmed = debug.trimmingCharacters(in: .whitespacesAndNewlines) if !trimmed.isEmpty { return trimmed } } - let localized = nsError.localizedDescription.trimmingCharacters(in: .whitespacesAndNewlines) - if !localized.isEmpty { - return localized - } let described = String(describing: error).trimmingCharacters(in: .whitespacesAndNewlines) - return described.isEmpty ? "unknown config parse error" : described + if !described.isEmpty { + return described + } + let localized = nsError.localizedDescription.trimmingCharacters(in: .whitespacesAndNewlines) + return localized.isEmpty ? "unknown config parse error" : localized }Based on learnings: Use
String(describing: error)instead oferror.localizedDescriptionwhen formatting errors in the cmux Swift CLI because it preserves the full cause.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@CLI/CMUXCLI`+Config.swift around lines 431 - 444, The error formatting in configDoctorErrorMessage currently falls back to localizedDescription before String(describing:), which can hide useful parse details; update the function to check NSDebugDescription first (nsError.userInfo[NSDebugDescriptionErrorKey]), then use String(describing: error) trimmed for non-empty content, and only if that is empty use nsError.localizedDescription, finally defaulting to "unknown config parse error"; ensure you reference and modify the private static func configDoctorErrorMessage(_ error: Error) accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CLI/cmux.swift`:
- Around line 19951-19956: The help/usage text block in CLI/cmux.swift omits the
`check` and `validate` aliases for the `doctor` command; locate the usage string
or array that currently reads "config <doctor|path|docs|reload>" and update it
to include the aliases (e.g. "config <doctor|check|validate|path|docs|reload>"
or otherwise list "doctor (aliases: check, validate)") so the `--help` output
shows those aliases; update the single place where that usage snippet appears
(search for the exact string in CLI/cmux.swift) and ensure formatting matches
the surrounding help lines.
- Around line 2067-2078: The early-path branch correctly keeps "reload" out of
the no-socket flow via configCommandDoesNotNeedSocket; update two things to fix
the help text and remove fragile coupling: 1) Change
runConfigCommand(socketPath: String) to accept an optional socketPath: String?
and, in the no-socket branch where you currently pass
CLISocketPathResolver.defaultSocketPath, pass nil instead (the call site using
runConfigCommand in this snippet). 2) Inside runConfigCommand add an explicit
guard that rejects/avoids any socket operations when socketPath == nil (return
an error or print and exit), making the no-socket intent explicit. Also update
configUsage() to list the undocumented aliases (help, check, validate, paths,
documentation) so usage output matches accepted subcommands. Ensure references:
configCommandDoesNotNeedSocket, runConfigCommand,
CLISocketPathResolver.defaultSocketPath, and configUsage are changed
accordingly.
In `@CLI/CMUXCLI`+Config.swift:
- Around line 315-331: The guard using fileManager.fileExists(atPath:
target.path) treats directories as files, so when target.path is a directory
Data(contentsOf:) fails; change the existence check to detect directories (use
FileManager.fileExists(atPath:isDirectory:) or attributesOfItem) and if
isDirectory return a ConfigDoctorFinding for that target (use the same shape as
the missing error: label: target.label, displayPath: target.displayPath, path:
target.path, status: "error" or "invalid", and a message like "path is a
directory, expected a file") instead of attempting Data(contentsOf:
URL(fileURLWithPath: target.path)).
In `@GhosttyTabs.xcodeproj/project.pbxproj`:
- Line 1759: Move the shared JSONCParser.swift and any dependent doctor core
files into a new SwiftPM library target (create/update Package.swift with a new
product and target), make the parser and any used types public if needed, update
the app and cmux-cli targets to import the new module instead of referencing the
file, remove JSONCParser.swift from the app target source list in Xcode (and any
duplicate copies), and adjust unit tests to depend on the new package target;
ensure the new package target builds for local Xcode integration and update any
import statements that referenced internal symbols to the new module name.
In `@tests/test_cli_config_doctor.py`:
- Around line 16-18: The current check accepts directories because
os.access(..., os.X_OK) is true for executable/searchable dirs; update the
conditional that assigns/returns explicit (the variable explicit derived from
os.environ.get("CMUX_CLI_BIN") or os.environ.get("CMUX_CLI")) to also require it
is a regular file (use os.path.isfile(explicit)) before checking os.access and
returning it, i.e. only return explicit when os.path.exists(explicit) and
os.path.isfile(explicit) and os.access(explicit, os.X_OK).
---
Duplicate comments:
In `@CLI/CMUXCLI`+Config.swift:
- Around line 431-444: The error formatting in configDoctorErrorMessage
currently falls back to localizedDescription before String(describing:), which
can hide useful parse details; update the function to check NSDebugDescription
first (nsError.userInfo[NSDebugDescriptionErrorKey]), then use
String(describing: error) trimmed for non-empty content, and only if that is
empty use nsError.localizedDescription, finally defaulting to "unknown config
parse error"; ensure you reference and modify the private static func
configDoctorErrorMessage(_ error: Error) accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7ee0fbf8-ba0d-4c59-a0cf-a328ccaef3fc
📒 Files selected for processing (7)
CLI/CMUXCLI+Config.swiftCLI/CMUXCLI+DocsSettings.swiftCLI/cmux.swiftGhosttyTabs.xcodeproj/project.pbxprojSources/JSONCParser.swiftdocs/cli-contract.mdtests/test_cli_config_doctor.py
| B900002FA1B2C3D4E5F60719 /* CMUXCLI+ThemeSupport.swift in Sources */, | ||
| B900002EA1B2C3D4E5F60719 /* CMUXCLI+Themes.swift in Sources */, | ||
| B9000033A1B2C3D4E5F60719 /* CMUXCLI+TopRendering.swift in Sources */, | ||
| C0DEF0B10000000000000003 /* JSONCParser.swift in Sources */, |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy lift
Move the shared config parser behind a package boundary.
JSONCParser.swift is now compiled into both the app and cmux-cli, but it still lives in the app target source tree. Since this is pure Foundation parsing logic reused across surfaces, keeping it wired directly through the Xcode targets will keep future config-doctor changes coupled to project wiring. Please extract the parser (and ideally the doctor core that depends on it) into a SwiftPM target and import it from the app/CLI/tests instead.
As per coding guidelines "Extract reusable domain logic used by more than one surface (Mac app, CLI, daemon, tests, previews, debug tooling, future iOS/shared code) into a dedicated SwiftPM package target instead of duplicating or centralizing in the app target".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@GhosttyTabs.xcodeproj/project.pbxproj` at line 1759, Move the shared
JSONCParser.swift and any dependent doctor core files into a new SwiftPM library
target (create/update Package.swift with a new product and target), make the
parser and any used types public if needed, update the app and cmux-cli targets
to import the new module instead of referencing the file, remove
JSONCParser.swift from the app target source list in Xcode (and any duplicate
copies), and adjust unit tests to depend on the new package target; ensure the
new package target builds for local Xcode integration and update any import
statements that referenced internal symbols to the new module name.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
| private func runConfigDoctor(arguments: [String], jsonOutput: Bool) throws -> ConfigDoctorReport { | ||
| let options = try parseConfigDoctorOptions(arguments) | ||
| let targets = options.paths.isEmpty | ||
| ? defaultConfigDoctorTargets() | ||
| : options.paths.enumerated().map { index, rawPath in | ||
| let path = Self.absoluteConfigPath(rawPath) | ||
| return ConfigDoctorTarget( | ||
| label: "custom \(index + 1)", | ||
| displayPath: Self.tildePath(path), | ||
| path: path, | ||
| missingIsError: true | ||
| ) | ||
| } | ||
| let findings = targets.map(configDoctorFinding(for:)) | ||
| let report = ConfigDoctorReport(findings: findings) | ||
|
|
||
| if jsonOutput { | ||
| print(jsonString(report.payload)) | ||
| } else { | ||
| printConfigDoctorReport(report) | ||
| } | ||
| return report | ||
| } |
There was a problem hiding this comment.
--path flag is stripped before parseConfigDoctorOptions can see it
docsSettingsArguments constructs arguments via head.filter { !$0.hasPrefix("-") }, which discards every --prefixed token. For commandArgs = ["doctor", "--path", "/tmp/cmux.json"] this produces doctorArgs = ["/tmp/cmux.json"]; parseConfigDoctorOptions then throws on the orphaned path value. The --path=<file> form is also silently stripped. Every invocation matching the documented usage (cmux config doctor --path .cmux/cmux.json) exits 1, and the test suite ok_result case fails on any correctly compiled build. The fix is to parse commandArgs directly in the doctor branch rather than routing through docsSettingsArguments.
There was a problem hiding this comment.
Not applicable on the current head. docsSettingsArguments preserves --path and --path=..., only removing --json; both documented forms pass against the tagged pr3454 CLI.
— Claude Code
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: a73032af-4c9e-4c2b-9fd5-7046c2ac4bc7
📒 Files selected for processing (4)
CLI/CMUXCLI+Config.swiftCLI/cmux.swiftdocs/cli-contract.mdtests/test_cli_config_doctor.py
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Dismissed as stale after the requested CodeRabbit fixes were addressed in follow-up commits and the CodeRabbit check passed.
This branch was successfully deployed
1 participant
Summary
cmux config doctor.cmux config path,cmux config docs, andcmux config reloadaliases around existing settings and reload flows.Testing
./scripts/reload.sh --tag omuxcfgpassed.--pathexits 1.cmux config --helpandcmux config pathpassed against the tagged CLI.Issues
Summary by cubic
Adds
cmux config doctorto validatecmux.json(JSONC) without a socket, plusconfig path|paths,config docs, andconfig reload. Documents the JSON output contract and tightens trailing comma checks.New Features
cmux config doctor(check,validate): validates JSONC for default targets (primary, project-level.cmux/cmux.jsonorcmux.jsondiscovered up to HOME, legacy files when present) or explicit--path/--path=.... Supports--json, printsok,error_count,findings(withlabel,display_path,path,status,ok,keys, optionalmessageandbytes), plusreload_command,docs_url, andschema_url; exits non‑zero on errors.~and relative paths; rejects unknown flags, bare positional paths, and directory paths with clear errors.config help|path|paths|docs|doctorrun without a socket;config reloaduses the socket flow.cmux --helpand topic help includeconfig.config docsmirrorsdocs settings.config path|pathsprint docs/schema URLs and the reload command.Bug Fixes
JSONCParser: rejects invalid trailing comma sequences with a clear "invalid trailing comma" error; flags empty files and non‑object top‑level values as errors.Written for commit bd00624. Summary will update on new commits.
Summary by CodeRabbit
New Features
Documentation
Tests
Improvements