[#185] Warn on corrupt/unreadable files in ConfigurationDiscovery by bguidolim · Pull Request #193 · mcs-cli/mcs

bguidolim · 2026-03-01T23:40:29Z

Context

ConfigurationDiscovery used guard...try?...else { return } for every discovery method, collapsing distinct failure scenarios (file missing vs. corrupt vs. permission denied) into the same silent empty result. Users running mcs export got incomplete exports with no indication of why artifacts were missing.

Changes

Split 9 try? patterns into fileExists check (silent early return for missing files) followed by do/catch with output.warn(...) for corrupt/unreadable files
Affected methods: discoverMCPServers, discoverSettings, extractHookCommands, discoverFiles, listFiles, discoverClaudeContent, discoverGitignoreEntries
Kept try? only for symlink resourceValues detection inside a .filter closure (benign fallback)
All method signatures remain non-throwing; discovery stays best-effort

Acceptance Criteria

Missing files → silent return (unchanged behavior)
Corrupt/unreadable files → [WARN] message with error details
Export continues after warnings (never aborts)
All 585 existing tests pass
Clean build with no warnings

Testing Steps

swift build — compiles cleanly
swift test — all 585 tests pass
mcs export /tmp/test-export on a working project — same output, no warnings
Corrupt ~/.claude.json (e.g. echo "{{bad" > ~/.claude.json) → [WARN] Could not parse .claude.json as JSON — file may be corrupt, export continues without MCP servers

Closes #185

…nDiscovery - Split guard...try?...else patterns into fileExists (silent return) and do/catch (output.warn) across 9 locations - Preserves best-effort discovery design while giving users diagnostic feedback for corrupt/unreadable files - Keep try? only for symlink detection in filter closure (benign fallback)

Copilot

Pull request overview

This PR improves ConfigurationDiscovery diagnostics during mcs export by warning when configuration files/directories exist but are unreadable or corrupt, while keeping missing files silent and preserving best-effort behavior.

Changes:

Replaced several try?-based silent failures with fileExists + do/catch and output.warn(...) on read/parse failures.
Added warnings for partial failures when deserializing/serializing remaining settings JSON.
Kept discovery APIs non-throwing so export continues after warnings.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-03-01T23:43:02Z

Sources/mcs/Export/ConfigurationDiscovery.swift

+        guard let json = try? JSONSerialization.jsonObject(with: data) as? [String: Any] else {
+            output.warn("Could not parse \(claudeJSONPath.lastPathComponent) as JSON — file may be corrupt")
            return
        }



JSONSerialization.jsonObject(with:) can throw, but this uses try? so the warning loses the underlying parse error details. To match the goal of warning with actionable error context, switch this to a do/catch and include error.localizedDescription (and optionally distinguish 'not a dictionary' from 'invalid JSON').

Suggested change

guard let json = try? JSONSerialization.jsonObject(with: data) as? [String: Any] else {

output.warn("Could not parse \(claudeJSONPath.lastPathComponent) as JSON — file may be corrupt")

return

}

let jsonAny: Any

do {

jsonAny = try JSONSerialization.jsonObject(with: data)

} catch {

output.warn("Could not parse \(claudeJSONPath.lastPathComponent) as JSON: \(error.localizedDescription)")

return

}

guard let json = jsonAny as? [String: Any] else {

output.warn("Expected top-level JSON object in \(claudeJSONPath.lastPathComponent) — file may be misconfigured")

return

}

Copilot · 2026-03-01T23:43:02Z

Sources/mcs/Export/ConfigurationDiscovery.swift

+        do {
+            settings = try Settings.load(from: settingsPath)
+        } catch {
+            output.warn("Could not parse \(settingsPath.lastPathComponent): \(error.localizedDescription)")


These catch blocks warn "Could not parse ..." but Settings.load(from:) can also fail due to I/O (e.g. permission denied) rather than JSON parsing. Consider changing the wording to "Could not load" / "Could not read or parse" so the warning remains accurate across failure modes.

Suggested change

output.warn("Could not parse \(settingsPath.lastPathComponent): \(error.localizedDescription)")

output.warn("Could not read or parse \(settingsPath.lastPathComponent): \(error.localizedDescription)")

Copilot · 2026-03-01T23:43:03Z

Sources/mcs/Export/ConfigurationDiscovery.swift

+            settings = try Settings.load(from: settingsPath)
+        } catch {
+            output.warn("Could not parse \(settingsPath.lastPathComponent): \(error.localizedDescription)")
+            return [:]


Same as above: this warns "Could not parse ..." but the thrown error may be an I/O failure (unreadable file) rather than a JSON parse failure. Adjusting the message to "Could not load" / "Could not read or parse" would avoid misleading diagnostics.

… try? - Fold hook-command extraction into discoverSettings (returns [String: String]?) to eliminate duplicate Settings.load call - Convert last try? on JSONSerialization in discoverMCPServers to do/catch for consistent error surfacing

Copilot

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 3 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-03-01T23:52:56Z

Sources/mcs/Export/ConfigurationDiscovery.swift

+        do {
+            settings = try Settings.load(from: settingsPath)
+        } catch {
+            output.warn("Could not parse \(settingsPath.lastPathComponent): \(error.localizedDescription)")


The warning message in this catch labels all failures as "Could not parse", but Settings.load(from:) can also throw for I/O errors (e.g., permission denied / unreadable file). Consider changing this to a more accurate message (e.g., "Could not load …") or branching on the error so read failures and JSON decode failures are described correctly.

Suggested change

output.warn("Could not parse \(settingsPath.lastPathComponent): \(error.localizedDescription)")

output.warn("Could not load \(settingsPath.lastPathComponent): \(error.localizedDescription)")

Copilot · 2026-03-01T23:52:56Z

Sources/mcs/Export/ConfigurationDiscovery.swift

+    private func discoverSettings(at settingsPath: URL, hooksDir: URL, into config: inout DiscoveredConfiguration) -> [String: String]? {
+        let settings: Settings


hooksDir is passed into discoverSettings but is not used anywhere in the implementation. If it’s no longer needed, remove the parameter (and update the call site) to reduce API noise; if it is needed, please use it (e.g., for more specific warnings).

Copilot · 2026-03-01T23:52:56Z

Sources/mcs/Export/ConfigurationDiscovery.swift

+        do {
+            files = try fm.contentsOfDirectory(at: hooksDir, includingPropertiesForKeys: nil)
+        } catch {
+            output.warn("Could not read hooks directory: \(error.localizedDescription)")


This warning doesn’t identify which hooks directory failed to read (global vs project, and the path/name), which can make it hard to diagnose in multi-scope runs. Consider including hooksDir.path or hooksDir.lastPathComponent in the message (similar to listFiles).

Suggested change

output.warn("Could not read hooks directory: \(error.localizedDescription)")

output.warn("Could not read hooks directory at \(hooksDir.path): \(error.localizedDescription)")

- Use "Could not load" instead of "Could not parse" for Settings.load failures (covers both I/O and decode errors) - Include hooks directory path in warning for easier diagnostics

Copilot

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-03-02T00:00:14Z

Sources/mcs/Export/ConfigurationDiscovery.swift

+        let settings: Settings
+        do {
+            settings = try Settings.load(from: settingsPath)
+        } catch {
+            output.warn("Could not load \(settingsPath.lastPathComponent): \(error.localizedDescription)")
+            return nil


The warning in the catch uses “Could not parse …” but Settings.load(from:) can also throw for non-parse failures (e.g., read/permission errors from Data(contentsOf:)). Consider wording this as “Could not load …” or “Could not read/parse …” so the message matches the actual failure mode.

Copilot AI review requested due to automatic review settings March 1, 2026 23:40

Copilot started reviewing on behalf of bguidolim March 1, 2026 23:40 View session

Copilot AI reviewed Mar 1, 2026

View reviewed changes

bguidolim requested a review from Copilot March 1, 2026 23:49

Copilot started reviewing on behalf of bguidolim March 1, 2026 23:49 View session

Copilot AI reviewed Mar 1, 2026

View reviewed changes

[#185] Remove unused hooksDir parameter from discoverSettings

8527310

bguidolim requested a review from Copilot March 1, 2026 23:56

Copilot started reviewing on behalf of bguidolim March 1, 2026 23:56 View session

[#185] Improve warning message accuracy

495c324

- Use "Could not load" instead of "Could not parse" for Settings.load failures (covers both I/O and decode errors) - Include hooks directory path in warning for easier diagnostics

bguidolim enabled auto-merge (squash) March 1, 2026 23:58

Copilot AI reviewed Mar 2, 2026

View reviewed changes

bguidolim merged commit 97d3252 into main Mar 2, 2026
3 checks passed

bguidolim deleted the feature/185-discovery-silent-failures branch March 2, 2026 00:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[#185] Warn on corrupt/unreadable files in ConfigurationDiscovery#193

[#185] Warn on corrupt/unreadable files in ConfigurationDiscovery#193
bguidolim merged 4 commits intomainfrom
feature/185-discovery-silent-failures

bguidolim commented Mar 1, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Mar 1, 2026

Uh oh!

Copilot AI Mar 1, 2026

Uh oh!

Copilot AI Mar 1, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Mar 1, 2026

Uh oh!

Copilot AI Mar 1, 2026

Uh oh!

Copilot AI Mar 1, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Mar 2, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

-        guard let json = try? JSONSerialization.jsonObject(with: data) as? [String: Any] else {
-            output.warn("Could not parse \(claudeJSONPath.lastPathComponent) as JSON — file may be corrupt")
-            return
-        }
+        let jsonAny: Any
+        do {
+            jsonAny = try JSONSerialization.jsonObject(with: data)
+        } catch {
+            output.warn("Could not parse \(claudeJSONPath.lastPathComponent) as JSON: \(error.localizedDescription)")
+            return
+        }
+        guard let json = jsonAny as? [String: Any] else {
+            output.warn("Expected top-level JSON object in \(claudeJSONPath.lastPathComponent) — file may be misconfigured")
+            return
+        }

	output.warn("Could not parse \(settingsPath.lastPathComponent): \(error.localizedDescription)")
	output.warn("Could not read or parse \(settingsPath.lastPathComponent): \(error.localizedDescription)")

	output.warn("Could not parse \(settingsPath.lastPathComponent): \(error.localizedDescription)")
	output.warn("Could not load \(settingsPath.lastPathComponent): \(error.localizedDescription)")

		private func discoverSettings(at settingsPath: URL, hooksDir: URL, into config: inout DiscoveredConfiguration) -> [String: String]? {
		let settings: Settings

	output.warn("Could not read hooks directory: \(error.localizedDescription)")
	output.warn("Could not read hooks directory at \(hooksDir.path): \(error.localizedDescription)")

Conversation

bguidolim commented Mar 1, 2026

Context

Changes

Acceptance Criteria

Testing Steps

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Mar 1, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 1, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 1, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Mar 1, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 1, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 1, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Mar 2, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants