Fix: rename reserved AppleScript variable in getGroupMembers - #4
oichtental wants to merge 2 commits into
Conversation
…etGroupMembers `line` is a reserved word in AppleScript, causing groups members to crash with "Zugriff nicht erlaubt" (-10003). Also add waitUntilCompleted() to MCP server so it doesn't exit prematurely. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
📝 WalkthroughWalkthroughUpdates AppleScript execution to drain process output safely, adds timeout errors, and builds contact records from bulk properties. The server now waits for completion. The Swift SDK requirement and Git ignore rules are updated. ChangesContactbook runtime and repository updates
Estimated code review effort: 4 (Complex) | ~45 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
|
I hit this same issue with the reserved 'line' and I confirm this fix works. |
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
Sources/Core/Services/ContactsService.swift (2)
62-65: 🚀 Performance & Scalability | 🔵 Trivial | 🏗️ Heavy liftReplace the sleep-poll loop with an event-driven wait.
runAppleScriptis a synchronous method on theContactsServiceactor. The loop blocks the calling cooperative-pool thread for up totimeoutseconds.lookupByPhonepasses 180 s. Concurrent MCP tool calls therefore hold a Swift concurrency thread while no work runs.Use
process.terminationHandlerwith awithCheckedThrowingContinuation, or run the blocking wait on a dedicated thread outside the actor. That keeps the timeout behavior and frees the executor thread.🤖 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 `@Sources/Core/Services/ContactsService.swift` around lines 62 - 65, Replace the sleep-poll loop in runAppleScript with an event-driven process wait using process.terminationHandler and a withCheckedThrowingContinuation, while preserving the existing timeout behavior and termination-result handling. Ensure the continuation resumes exactly once on process termination or timeout, without blocking the ContactsService actor’s cooperative executor thread.
138-156: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winFetch only the requested number of contacts.
The AppleScript block reads 13 properties for every person, then the Swift limit trims the output. Fetch
people 1 thru min(limit ?? 50, count of people)after guarding against zero people. This avoids transferring the full database when the user requests only one record.🤖 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 `@Sources/Core/Services/ContactsService.swift` around lines 138 - 156, Update the AppleScript around the Contacts fetch to determine the requested limit and total people count, return safely when there are zero people, and fetch only people 1 thru min(limit ?? 50, count of people). Remove the later full-result trimming via n while preserving the existing property extraction and output behavior.
🤖 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 `@Sources/Core/Services/ContactsService.swift`:
- Around line 160-170: Update the AppleScript address-building logic in
parseContacts to use a blank-on-missing helper for street, city, state, ZIP, and
country values, rather than calling txt directly. Define the helper next to txt
and apply it to every address component so missing fields contribute empty
strings and never the literal “missing value”.
- Around line 67-79: Update the process cleanup around terminate() and
ioGroup.wait() to enforce bounded reader cleanup: allow the existing SIGTERM
grace period, then send SIGKILL if the process remains running, and bound the
normal-exit ioGroup.wait() rather than waiting indefinitely. Ensure both timeout
and normal exit paths cannot leave reader threads or pipe descriptors blocked.
---
Nitpick comments:
In `@Sources/Core/Services/ContactsService.swift`:
- Around line 62-65: Replace the sleep-poll loop in runAppleScript with an
event-driven process wait using process.terminationHandler and a
withCheckedThrowingContinuation, while preserving the existing timeout behavior
and termination-result handling. Ensure the continuation resumes exactly once on
process termination or timeout, without blocking the ContactsService actor’s
cooperative executor thread.
- Around line 138-156: Update the AppleScript around the Contacts fetch to
determine the requested limit and total people count, return safely when there
are zero people, and fetch only people 1 thru min(limit ?? 50, count of people).
Remove the later full-result trimming via n while preserving the existing
property extraction and output behavior.
🪄 Autofix
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 Plus
Run ID: 447a399d-c9cb-4ea0-960d-4c0db8591034
📒 Files selected for processing (2)
Package.swiftSources/Core/Services/ContactsService.swift
| if process.isRunning { | ||
| process.terminate() | ||
| return "" | ||
| _ = ioGroup.wait(timeout: .now() + 5) | ||
| // Surface the timeout instead of returning "" — an empty string parses as | ||
| // "no contacts", which is indistinguishable from a genuinely empty result | ||
| // and silently hides the real failure from callers (and from MCP clients). | ||
| throw ContactsError.operationFailed( | ||
| "AppleScript timed out after \(Int(timeout))s (Contacts.app may be busy or syncing)" | ||
| ) | ||
| } | ||
|
|
||
| let outputData = outputPipe.fileHandleForReading.readDataToEndOfFile() | ||
| let errorData = errorPipe.fileHandleForReading.readDataToEndOfFile() | ||
| // Both reads end when osascript closes its descriptors on exit. | ||
| ioGroup.wait() |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Bound the reader cleanup and escalate to SIGKILL.
Two problems exist in the exit paths.
Line 68 calls terminate(), which sends SIGTERM. If osascript does not exit, the two reader closures stay blocked in readDataToEndOfFile() forever. Line 69 bounds only the caller's wait, not the reader threads. Each timed-out call then leaks two threads on the concurrent queue and both pipe file descriptors.
Line 79 calls ioGroup.wait() with no bound. The reads finish only when every writer descriptor closes. If osascript leaves a child process that inherited the write end, this call never returns, and the timeout that this change adds is bypassed.
Escalate to SIGKILL after the grace period, and bound the normal wait.
🛡️ Proposed fix
if process.isRunning {
process.terminate()
- _ = ioGroup.wait(timeout: .now() + 5)
+ if ioGroup.wait(timeout: .now() + 5) == .timedOut {
+ kill(process.processIdentifier, SIGKILL)
+ _ = ioGroup.wait(timeout: .now() + 2)
+ }
// Surface the timeout instead of returning "" — an empty string parses as
// "no contacts", which is indistinguishable from a genuinely empty result
// and silently hides the real failure from callers (and from MCP clients).
throw ContactsError.operationFailed(
"AppleScript timed out after \(Int(timeout))s (Contacts.app may be busy or syncing)"
)
}
// Both reads end when osascript closes its descriptors on exit.
- ioGroup.wait()
+ if ioGroup.wait(timeout: .now() + 5) == .timedOut {
+ throw ContactsError.operationFailed(
+ "AppleScript output could not be read after osascript exited"
+ )
+ }🤖 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 `@Sources/Core/Services/ContactsService.swift` around lines 67 - 79, Update the
process cleanup around terminate() and ioGroup.wait() to enforce bounded reader
cleanup: allow the existing SIGTERM grace period, then send SIGKILL if the
process remains running, and bound the normal-exit ioGroup.wait() rather than
waiting indefinitely. Ensure both timeout and normal exit paths cannot leave
reader threads or pipe descriptors blocked.
| set addrOut to "" | ||
| set streets to item i of stAll | ||
| if streets is not missing value then | ||
| repeat with j from 1 to (count of streets) | ||
| set part to my txt(item j of streets) | ||
| if part is "missing value" then set part to "" | ||
| set part to part & ", " & my txt(item j of (item i of ctAll)) & ", " & my txt(item j of (item i of staAll)) & " " & my txt(item j of (item i of zipAll)) & ", " & my txt(item j of (item i of cnAll)) | ||
| if addrOut is not "" then set addrOut to addrOut & ";;;" | ||
| set addrOut to addrOut & part | ||
| end repeat | ||
| end if |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Address parts insert the literal text missing value.
Line 164 maps a missing street to an empty string. Lines 166 does not do the same for city, state, zip, and country. txt returns the literal string "missing value" for those fields.
A contact with a street but no state then produces Hauptstr. 1, Berlin, missing value 10115, DE. parseContacts compares only the complete field against "missing value" at line 575, so the text reaches the CLI output and the MCP JSON.
Add a blank-on-missing helper and use it for every address part.
🐛 Proposed fix
on cleanTxt(v)Add the helper next to txt:
+ on blankTxt(v)
+ set s to my txt(v)
+ if s is "missing value" then return ""
+ return s
+ end blankTxt
+
on cleanTxt(v)Then use it for all address parts:
repeat with j from 1 to (count of streets)
- set part to my txt(item j of streets)
- if part is "missing value" then set part to ""
- set part to part & ", " & my txt(item j of (item i of ctAll)) & ", " & my txt(item j of (item i of staAll)) & " " & my txt(item j of (item i of zipAll)) & ", " & my txt(item j of (item i of cnAll))
+ set part to my blankTxt(item j of streets)
+ set part to part & ", " & my blankTxt(item j of (item i of ctAll)) & ", " & my blankTxt(item j of (item i of staAll)) & " " & my blankTxt(item j of (item i of zipAll)) & ", " & my blankTxt(item j of (item i of cnAll))
if addrOut is not "" then set addrOut to addrOut & ";;;"
set addrOut to addrOut & part
end repeat🤖 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 `@Sources/Core/Services/ContactsService.swift` around lines 160 - 170, Update
the AppleScript address-building logic in parseContacts to use a
blank-on-missing helper for street, city, state, ZIP, and country values, rather
than calling txt directly. Define the helper next to txt and apply it to every
address component so missing fields contribute empty strings and never the
literal “missing value”.
2 participants
Summary
groups memberscrash:lineis a reserved word in AppleScript, causing error-10003("Zugriff nicht erlaubt" / access not allowed). Renamed torecordLineto match the convention used in all other AppleScript blocks inContactsService.swift.waitUntilCompleted()so the MCP server stays alive afterstart().Test plan
contactbook groups members "Privat"— returns 70 members (was crashing before)contactbook groups members "Sophia" --json— returns valid JSONcontactbook contacts list,contacts search,contacts get,lookup— all still work🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Chores