Skip to content

Add compatibility with Swift Test#99

Open
haffmaestro wants to merge 2 commits into
conorluddy:mainfrom
haffmaestro:swift-test
Open

Add compatibility with Swift Test#99
haffmaestro wants to merge 2 commits into
conorluddy:mainfrom
haffmaestro:swift-test

Conversation

@haffmaestro

@haffmaestro haffmaestro commented Jan 20, 2026

Copy link
Copy Markdown

What

  • Upgrade to Zod v4
  • Add compatibility with Swift Test

Why

The MCP wasn't working with Claude Code 2.1.12, it would error out when it did tool discovery with the following error:

2026-01-19T18:02:08.360Z [ERROR] MCP server "xc-mcp" Failed to fetch tools: MCP error -32603: Cannot
  read properties of undefined (reading '_zod')

The xcodebuild-test tool would run my Swift tests but the summary would say 0 tests ran, so I added compatibility with the Swift test output style.

@haffmaestro haffmaestro marked this pull request as ready for review January 20, 2026 15:34
@claude

claude Bot commented Jun 18, 2026

Copy link
Copy Markdown

Code Review — PR #99: Add compatibility with Swift Testing

Overview

This PR fixes two things: a Zod v4 compatibility bug that broke MCP tool discovery, and adds Swift Testing output parsing to xcodebuild-test. Both changes are valuable.


Issues

🔴 .claude/settings.local.json committed to the repo

File: .claude/settings.local.json

The .local.json suffix marks this as a personal override file that should never be tracked in git. It contains machine-specific Claude Code permissions for the contributor's local dev environment.

Action: Add .claude/settings.local.json to .gitignore and remove it from this PR. The repo already has .claude/settings.json for shared project permissions.


🟡 MCP SDK bump (1.17.11.25.2) carries new runtime dependencies

The SDK version jump is significant — it pulls in @hono/node-server, jose, ajv-formats, json-schema-typed, and moves hono / express to peer deps. The PR description mentions the _zod runtime error, which is a real fix, but the additional transitive deps aren't mentioned.

Recommendation: Add a note in the PR description that this is a required SDK upgrade (not just the Zod v4 fix) and confirm the new peer deps don't conflict with anything in the install surface.


Code Quality

✅ Zod v4 z.record() fix — correct

- environment: z.record(z.string()).optional(),
+ environment: z.record(z.string(), z.string()).optional(),

Zod v4 changed z.record(valueType) to require explicit key + value types. This fix is correct and applied consistently across idb.ts, simctl.ts, and workflows.ts.

✅ Swift Testing parsing — well structured

The regex patterns are correct for the documented Xcode 16+ output format:

Test case 'SuiteName/testName()' passed on 'Device Name' (x.xxx seconds)

The continue statement after the XCTest branch prevents double-counting on lines that might partially match both formats — good defensive coding.

get-details.ts error filter additions

Adding ** TEST FAILED ** and the Swift Testing failure patterns to errors-only is sensible and won't break existing XCTest workflows.

✅ Fallback logic using ** TEST FAILED/SUCCEEDED ** markers

The marker-based fallback (Strategy 3) is a safe last resort. The parseWarnings field giving visibility into which path was taken is a nice touch for debugging.


Test Coverage

The new test suite in xcodebuild-test.test.ts covers the key paths well:

  • Passed-only Swift Testing output
  • Failed Swift Testing output (checks failedTestsList propagation)
  • Mixed XCTest + Swift Testing output ✅ — this is the risky edge case and it's tested
  • Marker-only fallbacks for both success and failure states

One gap: no test for skipped Swift Testing tests (Test case '...' skipped pattern). Low priority but worth adding.


Summary

Status
.claude/settings.local.json in git 🔴 Must fix — remove from repo, add to .gitignore
Zod v4 z.record() fix ✅ Correct
Swift Testing parsing ✅ Well implemented
Test coverage ✅ Good (minor gap on skipped tests)
SDK bump risk 🟡 Low risk, but worth documenting in PR description

The code changes themselves are good. The blocker is the committed personal config file.

@claude

claude Bot commented Jun 18, 2026

Copy link
Copy Markdown

Code Review — PR #99: Add compatibility with Swift Test + Zod v4

Overall: 🔶 Needs changes (two blockers, rest is solid)

What this does

  1. Upgrades Zod v3 → v4, fixing the Cannot read properties of undefined (reading '_zod') crash in Claude Code 2.1.12.
  2. Adds Swift Testing framework output parsing to xcodebuild-test and get-details.
  3. Bumps @modelcontextprotocol/sdk from ^1.17.1^1.25.2.

🚨 Blocker 1 — .claude/settings.local.json committed to the repo

diff --git a/.claude/settings.local.json b/.claude/settings.local.json
new file mode 100644

This file contains personal Claude Code permission overrides (Bash(grep:*), Bash(find:*), Bash(npm run build:*)) and should not be tracked in source control. It's a developer-local file (equivalent to .env.local). Please add it to .gitignore and remove it from this PR.


🚨 Blocker 2 — Mixing three unrelated concerns in one PR

This PR bundles:

  • A Zod v4 API fix (root cause of the _zod crash)
  • A Swift Testing output parser (feature)
  • An MCP SDK major-version bump (1.171.25)

These carry very different risk profiles. The Zod fix is a genuine regression for existing users; the SDK bump is potentially breaking. Please split into at minimum two PRs: (a) the Zod v4 fix + SDK bump, (b) the Swift Testing feature.


Issues

@modelcontextprotocol/sdk ^1.25.2 is a large jump from 1.17.x — eight minor versions. The SDK changelog should be reviewed for breaking changes before bumping. The lockfile diff shows new runtime dependencies added (@hono/node-server, jose, ajv-formats, json-schema-typed) and zod moved to a peer dependency — these are non-trivial changes.

z.record(z.string(), z.string()) — the Zod v4 fix is correct. z.record() now requires two type arguments in v4. Same fix applied consistently in simctl.ts, idb.ts, and workflows.ts. ✅


Strengths

Swift Testing parsing is well done:

  • Both pass/fail formats covered (Test case 'Suite/test()' passed on 'Device')
  • Skipped test handling included
  • Fallback to ** TEST FAILED ** / ** TEST SUCCEEDED ** markers for unparseable output — smart defensive coding
  • The mismatch guard (failedTests === 0 && hasTestFailedMarker) prevents false positives

Test coverage is excellent — 5 new test cases cover: pure Swift Testing pass, pure Swift Testing fail, mixed XCTest+Swift Testing, marker-only fallback for failure, and marker-only fallback for success. These cover the real edge cases.

get-details.ts errors-only filter — adding ** TEST FAILED ** and the XCTest/Swift Testing failure line patterns is a correct, targeted improvement.


Minor

The continue statement added after the XCTest block (line ~365) is a good correctness fix — previously a line matching the XCTest pattern could theoretically also match the Swift Testing regex. The continue makes the branching explicit and efficient.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant