Repository navigation
Report CLI permission denials instead of "No models downloaded" - #111
Conversation
The CLI reads models from the GUI app's sandbox container, which macOS protects as app-private data. A process that is not the owning app needs Full Disk Access to enumerate it, and the denial is asymmetric: stat() is permitted, so fileExists returns true, while readdir returns EPERM. Both directory enumerations in discoverDownloadedModels were wrapped in try?, which discarded that error. Discovery therefore found nothing and reported "No models downloaded. Please open Wispr.app and download at least one model" to users whose models were present and working in the GUI. No error named the path it had read, so the cause was invisible. Add FileAccess to WisprCore: a permission classifier matching EPERM and EACCES at the top level and across underlyingErrors, a directory read that throws rather than returning nil, and the Full Disk Access guidance text. It lives in WisprCore rather than the CLI so it is reachable from the test target, which depends on WisprApp and WisprCore but not on the WisprCLI executable target. In the CLI, replace both try? enumerations with a single authoritative read of the models directory, reused by the Parakeet scan. Attach the resolved path to every discovery error. Escalate a permission denial on the preferences plist for the same reason, keeping absent and malformed as "active model unknown". Print the resolved models root under --verbose. fileExists is kept only to tell "never set up" from "set up but unreadable" -- it cannot serve as a permission check, since it succeeds on protected paths. Register FileAccess.swift in the Xcode project: WisprCore uses explicit file references, not a synchronized group, so the product build would otherwise fail to find it. Correct the model path in CLAUDE.md, which documented the fallback location that does not exist on a normal install, and document the CLI and its Full Disk Access prerequisite in the README. Refs #110
There was a problem hiding this comment.
🟡 Changes recommended
The permission classifier can miss POSIX errors wrapped via the singular NSUnderlyingErrorKey, and the design specification contains stale API descriptions.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
This PR improves wispr-cli diagnostics for sandbox model-directory permission failures and documents the shared model path and Full Disk Access requirement.
Changes:
- Adds strict filesystem reads and actionable permission guidance.
- Updates CLI discovery and preferences handling.
- Adds tests, project registration, documentation, and implementation specs.
File summaries
| File | Description |
|---|---|
Sources/WisprCore/Utilities/FileAccess.swift |
Permission classification and guidance |
Sources/WisprCLI/WisprCLI.swift |
Updated discovery and error reporting |
wisprTests/FileAccessTests.swift |
Classification, read, and message tests |
wispr.xcodeproj/project.pbxproj |
Registers the new core source |
README.md |
Documents CLI usage and permissions |
CLAUDE.md |
Corrects the shared model path |
.kiro/specs/cli-model-access-errors/tasks.md |
Implementation checklist |
.kiro/specs/cli-model-access-errors/requirements.md |
Feature requirements |
.kiro/specs/cli-model-access-errors/design.md |
Technical design |
.kiro/specs/cli-model-access-errors/.config.kiro |
Spec metadata |
Review details
- Files reviewed: 10/10 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
sebsto
left a comment
There was a problem hiding this comment.
Summary — clean and shippable, nothing blocks merge.
This fixes #110: the CLI reported No models downloaded to users whose models were present but sat behind a Full Disk Access denial, because the directory enumeration was wrapped in try? and a permission failure was indistinguishable from an empty directory. The PR replaces both try? reads with a strict FileAccess.readDirectory that classifies the failure, names the resolved path in every discovery error, escalates a plist-permission denial the same way, and adds --verbose root disclosure.
The three things I checked hardest, all good:
- The core property holds. A
contentsOfDirectoryfailure can no longer reach.noDownloadedModels; it becomes.unreadable. The single-authoritative-read of the models directory (reused by the Parakeet scan) means a denial is detected in exactly one place. The regression is pinned by a test asserting the denial message never contains the word "download", and by the empty-directory-is-success test that the whole fix rests on. - Concurrency and placement follow house style.
FileReadFailure.otherstores a pre-renderedStringrather thanany Error, keeping the typeSendablewith nononisolated(unsafe)/Task.detachedescape hatch; the types arenonisolatedto matchModelPathsunderdefaultIsolation(MainActor.self). Living inWisprCore(not the CLI executable target) is the right call for testability. - The Xcode trap is handled. The new
Sources/WisprCorefile uses explicit file references, andFileAccess.swiftis registered inproject.pbxproj(build file, file reference, group child, and Sources phase). This is exactly the class whereswift buildpasses whilexcodebuildfails to find the symbol — good that it was caught and that all three CI legs are green.
Everything below is advisory / Low. Tests are thorough (20 across three suites) and idiomatic swift-testing.
`NSError.underlyingErrors` reports both `NSUnderlyingErrorKey` and `NSMultipleUnderlyingErrorsKey`, but it only descends one level. A Cocoa wrapper whose own underlying error is another wrapper therefore hid the POSIX denial beneath it, and the CLI omitted the Full Disk Access guidance for that shape. Make the traversal recursive, with a depth cap so a pathological or self-referential chain cannot recurse without bound. Add three tests: a denial two wrappers deep, a nested Cocoa no-permission error, and a 50-deep non-denial chain asserting traversal terminates. Also correct the spec: the Data Models section and task 1.1 still described a nested `CLIError.FailureReason` that the implementation never had. The real types are `FileReadFailure` and `FileReadError`, top-level in WisprCore so the classifier is reachable from the test target. Requirement 5 gains a termination criterion, which renumbers the graceful-degradation criterion to 5.4. Refs #110
|
Both review points addressed in 2e3c1c4. Per-point disposition: 1. Classifier and The stated cause is wrong: The recursion suggestion is nonetheless right, for a different reason: 2. Stale spec API descriptions ( Nothing dismissed. Tests: 3 added, Still open as a reviewer question, unchanged from the description: whether to strip the ~7 lines of pre-existing Xcode reordering churn from the |
The Error message copy section specified "The directory could not be read: <reason>" while `explain` emits "The read failed: <reason>". Changed the doc rather than the code: `explain` also renders the preferences-file failure, so directory-specific wording would be wrong there, and the subject is already named on the first line by `what`. Refs #110
There was a problem hiding this comment.
🟡 Changes recommended
Nested permission errors are truncated at five levels, and model-size discovery still suppresses filesystem failures.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
Sources/WisprCLI/WisprCLI.swift:271
- The new strict reads cover the directory listings, but discovery still calls
directorySize, whose enumerator andresourceValuesfailures are converted tonil/0. If a model subtree is present but unreadable, this path reports a valid model with a misleading 0-byte size and silently loses the filesystem error, contrary to the no-swallowed-read guarantee. Make size calculation throwing/classified and route all size call sites through the same unreadable error path.
- Files reviewed: 10/10 changed files
- Comments generated: 1
- Review effort level: Lite
There was a problem hiding this comment.
🔵 Needs a closer look
Model discovery can still silently treat unreadable model subtrees as downloaded, and repository specs retain stale model-path documentation.
Review details
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
Sources/WisprCLI/WisprCLI.swift:271
- This discovery path still feeds a recognized model into
directorySize, whose implementation below usesenumerator/resourceValueswithtry?and returns 0 on failure. If the model subtree is unreadable, the CLI can still report it as downloaded (often with a 0 MB size) instead of surfacing the read failure required by Requirement 1.4. Make the size calculation throwing and route that error throughCLIError.unreadable, or otherwise avoid treating an unreadable model as discovered.
.kiro/specs/cli-model-access-errors/requirements.md:150 - This acceptance criterion is not fully satisfied by the documentation changes:
.kiro/specs/cli-tool/design.md:143still tells contributors to discover models under~/Library/Application Support/wispr/models/, and.kiro/specs/cli-tool/requirements.md:7repeats that as the shared location (despite its glossary listing the container path). Please update or remove those stale references too, otherwise repository documentation still sends investigations to the wrong directory.
- Files reviewed: 10/10 changed files
- Comments generated: 0 new
- Review effort level: Lite
The depth cap of 5 bounded recursion but reclassified any denial nested below it as `.other`, so the CLI would omit the Full Disk Access guidance for a deeply wrapped denial. That contradicts requirement 5.2, which says a denial is recognised anywhere in the chain. Track visited `NSError` identities instead and walk every unvisited underlying error. This terminates on any graph, cyclic included, without skipping a reachable denial, and removes the arbitrary constant. `ObjectIdentifier` is a sound key here: `NSError` is a class and `as NSError` preserves reference identity for errors pulled out of `underlyingErrors`, both verified empirically. Two tests added: a denial 50 wrappers deep, which the cap would have missed, and a denial reachable only through one branch of an `NSMultipleUnderlyingErrorsKey` chain. Requirement 5.3 now also states that nesting depth must not cause a denial to be skipped. Refs #110
There was a problem hiding this comment.
🔵 Needs a closer look
Nested model-directory size reads still suppress filesystem failures and can report inaccessible models as downloaded with an incorrect 0 MB size.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
Sources/WisprCLI/WisprCLI.swift:271
- The new strict root read does not cover this nested model read:
directorySize(at:)returns0when its enumerator cannot be created and ignoresresourceValuesfailures viatry?. If a recognized Parakeet directory is present but unreadable (or becomes inaccessible after the root listing),--list-modelswill still report it as downloaded with a misleading0 MBinstead of surfacing the read failure. Make the size calculation throwing and propagate the failure through the sameFileReadError/CLIError.unreadablepath, as the discovery requirements require.
- Files reviewed: 10/10 changed files
- Comments generated: 0 new
- Review effort level: Lite
|
All five review threads resolved. CI green on
Net effect of the review: one genuine defect fixed (5), one genuine gap fixed on a false premise (1), two doc/spec corrections (2, 3), one advisory kept with evidence (4). Tests grew from 20 to 25 in the Requirement 5.3 was tightened to say that nesting depth must not cause a denial to be skipped, so the spec cannot drift back toward a cap. Two items still open for you, neither raised by a reviewer:
|
Fixes #110.
wispr-clireportedNo models downloaded. Please open Wispr.app and download at least one modelto users whose models were present and working in the GUI. The real cause was a missing Full Disk Access grant, and nothing in the output pointed at it.Why it happened
The CLI reads models from the GUI's sandbox container, which macOS protects as app-private data. A process that is not the owning app needs Full Disk Access, and the denial is asymmetric:
fileExists(atPath:)usesstat()— permitted, returnstruecontentsOfDirectory(atPath:)needsreaddir— refused, returnsEPERMDiscovery got past the existence guard, then both enumerations failed. Both were wrapped in
try?:A permission denial was therefore indistinguishable from an empty directory, and no error named the path it had read. Diagnosing one report took six rounds and ruled out several phantom causes first.
ModelPathsresolution was never at fault and is unchanged.What changed
New
FileAccessinWisprCore— permission classifier, a directory read that throws instead of returningnil, and the Full Disk Access guidance text.It sits in
WisprCorerather than the CLI for testability: the test target depends onWisprAppandWisprCore, not on theWisprCLIexecutable target, and adding a dependency on an@maintarget would need changes to bothPackage.swiftand the Xcode test target for no design benefit.CLIErrorstays a thin presentation layer.The classifier matches
NSFileReadNoPermissionErrorplusEPERMandEACCES, at the top level and acrossunderlyingErrors. TCC producesEPERM, ordinary filesystem permissions produceEACCES, and Foundation often wraps the errno in a Cocoa error. A classification miss falls back to.othercarrying the underlying description, so a miss degrades to a verbose but truthful message rather than a wrong one.In the CLI — one authoritative read of the models directory, reused by the Parakeet scan, so a denial is detected in exactly one place and the directory is no longer read twice. Every discovery error now carries the resolved path. A permission denial on the preferences plist escalates the same way, since it is the same root cause; absent and malformed still mean "active model unknown".
--verboseprints the resolved models root.fileExistsis kept only to tell "never set up" from "set up but unreadable". It cannot serve as a permission check, because it succeeds on protected paths — that is the whole trap.Xcode project —
Sources/WisprCoreuses explicit file references rather than a synchronized group, soFileAccess.swifthad to be registered or the product build fails to find it. Worth knowing:swift buildsucceeded whilexcodebuildfailed, so SPM alone would not have caught this.Docs —
CLAUDE.mddocumented the shared model path as~/Library/Application Support/wispr/models/, which is onlyModelPaths.base's fallback and does not exist on a normal install. That wrong path actively misdirected the original investigation. The README had no CLI section at all and now documents the tool and its Full Disk Access prerequisite.Before and after
Before, with a protected container:
After:
Tests
20 new tests in
wisprTests/FileAccessTests.swift, across three suites.NSUnderlyingErrorKey. Negative cases pin down thatNSFileNoSuchFileError,ENOENT, and a foreign domain carrying theEPERMnumeric value are not denials.000throws.permissionDeniedrather than returning an empty list. That last test restores the mode in adeferso cleanup can descend, and is disabled undergetuid() == 0since root bypasses directory permissions.sshd-keygen-wrapper, andtmux kill-server. One test asserts the denial message never contains the word "download", as a direct regression guard on the original bug. Two more confirm a non-permission failure reports its reason and omits the Full Disk Access advice, so a disk error does not send someone to Privacy & Security.Verification
swift build,xcodebuild -scheme WisprCLI, andxcodebuild -scheme wisprall succeed.000case exercised a realEACCESrather than being skipped.AudioEngine audio level stream terminates on stop, which is pre-existing and unrelated — it fails identically on the stashed baseline (656 tests, same single failure) and passes when the AudioEngine suite runs in isolation. Timing-sensitive audio hardware test, no audio code touched here.modelsdirectory to000: the CLI printed the message above with the real path and exit code 1, and after restoring permissions--list-modelsagain printedparakeet-v3 483 MB (active). The second half matters — it confirms the strict reads did not introduce a false failure on a working install.Still worth doing before release: the same check on a machine whose terminal genuinely lacks Full Disk Access, since that produces
EPERMrather than theEACCESa chmod produces.Not in scope
The CLI fails loudly rather than working around the permission: no fallback directory, no mirroring models to an unprotected location, no degraded mode.
Requiring every user to grant Full Disk Access to their terminal is still a poor out-of-box story. Dropping the App Sandbox from the GUI would remove this failure mode entirely, and the app ships via Homebrew cask and a notarized pkg rather than the App Store. That is a separate distribution and migration decision — existing users would likely need to re-download models — and is noted as an open question on #110.
Reviewer note
The
project.pbxprojdiff includes ~7 lines of pre-existing Xcode reordering churn that was already in my working tree before this work, alongside my 4 additions. Happy to strip it if you would rather keep the diff to just the file registration.Requirements, design, and task breakdown are in
.kiro/specs/cli-model-access-errors/.