Repository navigation
Fix six bugs from the code audit, and cut 1.0.1 - #4
Merged
Merged
Conversation
osascript prints booleans as the strings "true" and "false", so Int(result) was always nil and every true result was received as false. Compare the string case-insensitively, keeping the integer coercion as a fallback so this path stays consistent with executeAppleScript, where NSAppleScript reports booleans through int32Value. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
waitUntilExit() ran before the pipe was drained: output larger than the pipe buffer (~64 KB) blocks the child on write while the parent waits for it to exit, deadlocking permanently. A .json listing of a large folder reaches that size easily. readDataToEndOfFile() now runs first and returns at EOF. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The sequential replacingOccurrences loop rescanned text it had already substituted, so a value containing a placeholder name was itself rewritten by a later key: a file named $folderPath.txt passed as the folderPath variable had the folderPath value spliced into its name. Values stay escaped, so this was silent data corruption rather than injection. The script is now walked once, matching the longest key at each $ and appending its substitution to the result. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
toHFSPath() prepended the startup disk name to every path, so /Volumes/Backup/notes.txt became Macintosh HD:Volumes:Backup:notes.txt instead of Backup:notes.txt. An HFS path is volume-relative, and /Volumes/Backup on the startup disk is only a mount point, so the result pointed at nothing. The volume name and mount point are now read from the path itself, which also covers disk images and network shares, and handles a volume whose mount directory was renamed on a name collision. The path must exist for the volume to be read; both overloads document this, and the path tests use NSHomeDirectory() instead of a fictional path. startupDiskName() had no callers left and is removed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
CODE_REVIEW.md is a working document for the current audit and is not meant to be tracked. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
An NSNumber bridges to Bool whatever it holds, so NSNumber(value: 1) as? Bool is true: a count arriving from JSONSerialization or from Objective-C matched the boolean case and was substituted as true instead of 1. Numbers are now unwrapped to the Swift type they actually carry before any type matching, using the CoreFoundation type to tell a boolean from a number and a floating-point value from an integral one. The switch keeps one case per Swift type, all of them reachable. Float and UInt now reach the numeric cases rather than falling through to the escaped-text default. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
resetAutomationPermission returned true whenever the process launched, ignoring its exit status, so a tccutil failure was reported as a successful reset. The status is now checked and logged when non-zero. The two remaining uses of localizedDescription are replaced by String(describing:), matching the rest of the file: localizedDescription returns a system-localized string, and the package messages are English only, as stated in README.md and CONTRIBUTING.md. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The throws list for executeAppleScript(_:with:) promised failedToReadOutput, which it never throws: an unparseable record or JSON result yields nil. Both execution methods documented a return type without [String: Any], which .record and .json produce. The record parser was documented as requiring alphanumeric keys, while it reads everything up to the first colon and rejects only an empty key. The .bool rows in README.md and API.md described integer coercion alone, now that text is read as well. The CLI method was described as throwing only on a launch failure, contradicting the table below it. Documents that the command-line method passes the substituted script to osascript as an argument, readable by any local process through ps, and updates toHFSPath() for the volume-relative behaviour. Fixes a typo in the AppleScriptObject property list. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The Unreleased section becomes 1.0.1 and gains a Fixed list for the six bugs found in the audit, three of which change what an existing call returns. A fresh, empty Unreleased section and the compare links follow. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Covers the three deterministic fixes and the one that needs execution: a substituted value is no longer rescanned for placeholders, an NSNumber renders as its value rather than as true, numbers surviving JSONSerialization keep their type, and a boolean result from the command-line method is read from the text osascript prints. Each test was checked against the pre-fix sources and fails there, so they guard the behaviour rather than merely describe it. The osascript tests run self-contained scripts, driving no application and needing no Automation permission, which the file header now notes. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
SwiftLint runs with --strict and rejected the double space after a comma, left over from aligning the two script declarations. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The macOS runner image ships with aws/tap configured, and recent Homebrew versions warn about untrusted taps on every brew command, which surfaced as an annotation on the lint job. Untapping it before installing SwiftLint keeps that output clean. The || true keeps the job green once the runner image stops shipping that tap. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this changes
Fixes the six bugs found in a full audit of the Swift sources, corrects the documentation where it described behaviour the code did not have, and records the result as 1.0.1. One commit per issue.
aef05c8.boolresults fromexecuteAppleScriptViaCommandLine(_:with:)were alwaysfalse.osascriptprintstrue/falseas text, andInt("true")isnil. The string is now compared, with integer coercion kept as a fallback so both execution paths agree.cf777d53c1403a$folderPath.txtpassed asfolderPathhad that variable's value spliced into its name. The script is now walked once, matching the longest key at each$.39af9detoHFSPath()prepended the startup disk name to every path, so/Volumes/Backup/notes.txtbecameMacintosh HD:Volumes:Backup:notes.txtinstead ofBackup:notes.txt. The volume name and mount point now come from the path itself.e0161b9NSNumberbridges toBoolwhatever it holds, soNSNumber(value: 1)fromJSONSerializationor Objective-C was substituted astrueinstead of1. Numbers are unwrapped to the Swift type they actually carry before any type matching.a5f8452resetAutomationPermission(for:)returnedtruewhenevertccutillaunched, ignoring its exit status.4ef0079covers the documentation corrections, and95b1c9dthe changelog.5956262adds the local review notes to.gitignore.Why
Three of these produce silently wrong results rather than errors: a
.boolscript that answerstrueis read asfalse, a filename that happens to contain a variable name is corrupted, and a count arriving asNSNumberis substituted astrue. The pipe deadlock hangs the calling thread permanently, and is reachable from a.jsonlisting of a large folder.How it was tested
swift buildandswift testpass (22 tests). The two path-conversion tests were updated: they used/Users/roger/Desktop, which does not exist, andtoHFSPath()now reads the volume from the file system, so they useNSHomeDirectory().Four regression tests were added, one per fix that can be pinned deterministically. Each was run against the pre-fix sources and fails there, so they guard the behaviour rather than merely describe it:
testSubstitutedValueIsNotRescannedset p to "Roger.txt", thenamevariable spliced into a filename that contained its placeholdertestNSNumberIsNotRenderedAsBooleantrue true, a count of1rendered as a booleantestJSONNumbersKeepTheirTypetrue true 1.5, the same throughJSONSerializationtestCommandLineBooleanResultsfalsefor a script returningtrueThe last one executes, unlike the rest of the suite. It runs
return trueandreturn falsethroughosascript, which drives no application and needs no Automation permission, so it is safe in CI; the file header notes the exception.Checklist
swift buildandswift testpass.///doc comment describing purpose, parameters and return value.toHFSPath()now requires the path to exist and returnsnilotherwise, where it previously returned a string for any input.FloatandUIntvariables now render as numeric literals instead of escaped text.resetAutomationPermission(for:)now returnsfalsewhentccutilexits non-zero.