Apply /simplify cleanups across the codebase#80
Open
alexkroman wants to merge 15 commits into
Open
Conversation
Ran the four cleanup review angles (reuse, simplification, efficiency,
altitude) over the whole tree and applied the findings that are behavior-
preserving.
Reuse — one definition per rule:
- release-lib.sh gains `require_tools` (with an optional `--hint`) and
`parse_yaml_scalar`; release.sh / release-{bump,build,install,publish}.sh
and dev-build.sh now share them instead of carrying four copies of the
preflight loop, a fifth copy of `require_project_version`, and a second
copy of `info`/`die`. The YAML read now matches the key as a whole field,
so `CFBundleVersion` can't also match `CFBundleVersionSomethingElse`.
- check.sh gains `tool_ready`, collapsing seven copy-pasted optional-linter
guards (each with its own skip note and `cd`).
- reset-install.sh reads the keychain service from `BUNDLE_ID` rather than
respelling the bundle id, so the reset can't half-miss on a rotation.
- SoundPack gains `fromPersisted`, mirroring `TriggerKey.fromPersisted`, so
the store and the @AppStorage picker share one decode-with-default rule.
- FocusCapture reads the focused element's role through the file's own
checked `stringValue` reader instead of a second inline CF downcast.
- Tests: the canned PCM blob moves to `StubPCM.aboveMinimum` (was restated
three times), the defaults-key literals become their public constants, the
UI suite uses `UITestIdentifiers.validAPIKey` instead of a raw sentinel,
an `anyDescendant(identified:)` helper replaces four copies of the
type-agnostic element query, and the session suites use the existing
`makeSession` fixture (including removing a private shadowing copy).
Simplification:
- One `pulsingOpacity` view modifier replaces the duplicated raised-cosine
breathing in TranscribingLabel and RecordingTag, and one
`overlayAnimationInterval` replaces the 20 Hz literal spelled three times.
- The phase→OverlayUIState, OverlayUIState→label, and phase→MenuBarStatus
mappings become `@Test(arguments:)` tables (the precedent
DictationKeyGateTests sets), which also surfaced that `.noTarget` had no
menu-bar row — now covered.
- Dropped three AppCoordinator forwarders whose only callers were the
UITEST_HOOKS harness; it drives `session.press()/release()/cancel()`.
- Dropped the single-use `captureSampleRate` alias.
- `SoundPack.all` is private now: the picker builds from `groups`/`voices(in:)`
and only `find(id:)` ever used it, so its doc no longer overstates its role.
Efficiency:
- OverlayBridge.pushLevel skips the assignment when the level is unchanged;
@observable invalidates on assignment, and the meter floors ambient to
exactly 0, so silent ticks were rebuilding the bar row 20x/s for nothing.
- The ready-screen logo is loaded once instead of per body evaluation.
- check.sh gives the TSan/ASan passes their own --scratch-path so the three
differently-flagged builds stop invalidating each other's caches.
Altitude:
- TranscriptionPrompt.build guards on `TranscriptionContext.isEmpty` rather
than re-deriving the emptiness test field by field, so a newly added
context signal can't be silently dropped.
- The 0...1 mic-level contract is enforced once at the app's ingress seam
instead of clamped again three layers deep.
- `SoundPack.isSilent` replaces `group == nil` spelled three times.
- `SyncSTTLimits.durationMs(ofPCMBytes:rate:)` owns the PCM→ms derivation
both the capture and upload logs were computing separately.
Docs: corrected AGENTS.md's paste-path paragraph (pasteSettleNanos ->
pasteSettleDuration, HID tap -> annotated session tap, and the settle/restore
runs on a chained follow-up task rather than blocking `insert`), the
`#if DEBUG` -> `#if UITEST_HOOKS` harness gate in AGENTS.md and the UITests
README, the macOS 26 -> 15 platform claims, and five stale "~30 Hz" meter
references now that meterInterval is 20 Hz.
Skipped, deliberately — behavior changes or work beyond a cleanup pass:
generalizing `isElectronApp` to an AX-opaque probe, replacing
`waitUntilFrontmost`'s poll with the activation notification, snapshotting the
clipboard at press time, reordering `setPhase(.recording)` before
`setTargetApp`, mapping `.injecting` to `.processing`, making the permission
poll event-driven, a `PipelinePhase.setupBlocker` classification, lazy AX
probes in the focus capture, emitting `synth` as a generated SoundPack field
(needs the sound generator re-run on a Mac), and folding
AppCoordinator+UITesting.swift into UITestSupport.swift (needs an xcodegen
regen). Also left alone: KeyTermsStepView writes its defaults slot twice per
keystroke with two different normalization rules, which is a real (minor) bug
— but the clean fix orphans the public `KeyTermsStore.set`, and whether that
stays public is an API call, not a cleanup.
Verified with `scripts/check.sh --portable` (actionlint, prettier, xmllint,
markdownlint, shellcheck, release.test.sh). The Swift build, tests,
sanitizers, and lint gates run on CI (macos-26), which is the authority.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HREvcTT8LHjs8vmQZ8eK5L
Follow-up to 22dca2a from a full-codebase code review. Scoped to defects that commit introduced, plus docs and tests it left stale — no behavior changes to pre-existing product code. - scripts/check.sh: `tool_ready` masked a failed `cd`. Used as an `if` condition the function runs with errexit suppressed, so a failing `cd "$REPO_ROOT"` fell through to `return 0` and would have run the linter from the wrong directory (`shellcheck scripts/*.sh` globbing nothing). Now `cd … || return 1`. Also `${2:-}` for the install hint, so a future one-arg call can't abort the script under `set -u` — and only on a machine where the tool is missing, the hardest path to notice. - ReadyView: dropped `nonisolated(unsafe)` from the hoisted logo. The app target sets SWIFT_DEFAULT_ACTOR_ISOLATION: MainActor, so a plain `private static let` infers @mainactor and `body` reads it there either way. The attribute was only correct while `NSImage` stays non-Sendable; if an SDK marks it `Sendable` the attribute becomes an "unnecessary" warning, and the target builds with SWIFT_TREAT_WARNINGS_AS_ERRORS. - BlurtUITests/README.md: still described the harness calling `AppCoordinator.beginDictation`/`endDictation`/`cancelDictation`, the three forwarders 22dca2a deleted. - AGENTS.md / BlurtUITests/README.md: both documented `RUN_UI_TESTS=1 scripts/check.sh` and claimed the UI suite is "kept out of the default required gate". `RUN_UI_TESTS` appears nowhere in scripts/, and check.sh runs uitest.sh and leaks.sh unconditionally outside --portable. - release.test.sh: pinned what the whole-field key match actually buys. 22dca2a's message cited `CFBundleVersionSomethingElse` as the hazard, which was wrong — the old `/CFBundleVersion:/` already required the colon, so that case never matched. The two that did break are a prefixed key (`MyCFBundleVersion:` returned 99) and a commented-out key (`# CFBundleVersion:` returned the literal string `CFBundleVersion:`). All three are now cases. - SyncSTTLimitsTests: pinned `durationMs`, which 22dca2a added with no direct test — including the `rate: 0` guard that replaced a latent `Int(inf)` trap. Verified with `scripts/check.sh --portable`. The Swift side still needs CI (macos-26); a review pass over 22dca2a found no build-breakers, and confirmed the `@Test(arguments:)` labeled-tuple tables resolve through swift-testing's generic single-collection overload. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HREvcTT8LHjs8vmQZ8eK5L
Each was pre-existing (none introduced by the /simplify pass) and each is a real behavior change, so each comes with a regression test where the engine can reach it. Ordered by user impact. 1. A dead trigger press after any keyless dictation end. Nothing fed a terminal `PipelinePhase` back into `DictationKeyGate`, so a dictation that ended without a key event — the auto-release cap on a long hold, or a press refused (no API key) / failed (no input device) — left the gate `.latched`. A latched `modifierDown` returns `.none`, and the `modifierUp` after it returns `.stop`, which no-ops on an already-terminal session: the user's entire next press did nothing, and if it was a hold they spoke a whole utterance into a session that never started. Easiest repro is unplugging the mic — press 1 flashes red, press 2 is silent, press 3 works. `DictationKeyTap.syncAfterTerminalPhase()` now clears the gate, driven from `AppCoordinator.render` on every terminal phase (a no-op in normal flows, where the gate is already idle). Resets unconditionally rather than skipping while the key is held: the dictation is over, so there is no state worth keeping, and a later key-up finds `modifierIsDown == false` and routes to `.none`. Two `DictationKeyRouterTests` cases pin both shapes. 2. Clipboard data loss around every paste. `currentItems()` returned `[]` both when the pasteboard was empty and when `pasteboardItems` failed, and `restore` called `clearContents()` before checking it had anything to write — so a degraded snapshot emptied the user's clipboard instead of restoring it. Now `snapshot() -> PasteboardSnapshot?` distinguishes unreadable (nil, skip the restore entirely) from genuinely empty (restore by clearing, which is faithful); `restore` builds the replacement items before clearing, checks `writeObjects`, and falls back to a plain-text floor. When nothing is reproducible it leaves the pasteboard alone rather than emptying it. Promised representations still can't be copied — that is inherent to snapshot-and-restore and is now documented rather than implied away. 3. `APIKeyStepView` could silently revert a rotated key. The view is mounted in both the wizard and Settings, each with its own `@State`, and its `savedKey`/`draft` mirror was seeded once in `onAppear`. Rotating the key in Settings then clicking Change -> Update in the wizard pre-filled the stale old key and wrote it back over the new one, with a success UI. The mirror is gone: presence is read from the observable `apiKey.hasAPIKey`, and `draft` is seeded from the Keychain at edit-entry time. 4. A password could reach the STT prompt. Secure-field redaction keyed on AX role only. AppKit also expresses secure-ness through `kAXSubroleAttribute`, and an element can report a generic `AXTextField` role with only its subrole saying "password" — those read as ordinary text and their contents went into `config.prompt` (and, with developer mode on, into `dictations.jsonl`). `isSecureField(role:subrole:)` now checks both, and `captureFieldContext` fails *closed*: an unreadable role is treated as secure, since it can't be shown not to be. Costs priming for an already-degraded app; the alternative cost is a password on the wire. 5. `priorText` sliced a trimmed string with an untrimmed caret offset. The fallback path read `kAXValueAttribute` through the shared trimming reader, but `caret` is a UTF-16 offset into the *original* value. Trailing whitespace: "Hello " with caret 6 fell through to the whole tail "Hello", destroying the trailing-space signal `withLeadingSeparator` reads, so the field got "Hello world". Leading whitespace (any indented editor line): " Hello world" with caret 8 returned "Hello wo" — text from *after* the caret, which was then sent to AssemblyAI as prior-cursor priming. Added a non-trimming `rawStringValue` for value reads; `stringValue` still trims for the label-ish attributes that want it. Docs updated for the three changes that alter documented behavior (the secure field detection, the clipboard restore contract, and the gate resync). Verified with `scripts/check.sh --portable`. The Swift side needs CI (macos-26); test expectations for the caret slices and `durationMs` were computed independently and match. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HREvcTT8LHjs8vmQZ8eK5L
Second batch from the codebase review — the findings that are verifiable in a Linux sandbox (scripts, workflow, docs), committed separately from the Swift fixes so this half is provably green. - release-publish.sh refuses to publish a DMG not built from HEAD. Building at commit A and then pulling any commit that doesn't touch CFBundleShortVersionString let this script tag commit B and publish A's binary under it — the release page and the bits disagreeing, with the staple check, the clean-tree check and SHA256SUMS all still passing. release.sh already made this comparison to decide whether to rebuild (`dmg_already_built`); the publish step is where it protects users. - leaks.sh reports a scan that couldn't run. Under `set -euo pipefail` a non-matching grep exited 1 and pipefail aborted the script *at the assignment*, so a `leaks` invocation that failed to attach printed nothing and the `:-no leak summary` fallback was unreachable — CI showed a bare non-zero exit indistinguishable from a real leak. Now `|| true` on the grep, an explicit die when there is no summary, and a `kill -0` liveness check before scanning. - check.yml's change filter fails open. `changed="$(git diff …)"` ran unguarded under `bash -e`, so a failing diff (unreachable base sha after a force-push, a fork edge case) failed the `changes` job, which made `check` SKIP — and GitHub reports a skipped required check to branch protection as passing. The whole macOS gate could be bypassed on a green-looking PR. - check.sh's coverage gate fails instead of vanishing. A missing profdata, test binary, or renamed test bundle turned the >=80% floor into a printed note while the script still exited 0. Each is now a hard error. - reset-install.sh completes the reset when lsregister fails. Its pipeline was the body of an `if` under errexit, so a non-zero `lsregister -dump` exited before the defaults, Keychain, and log cleanup — a partial reset reported as success. Every other step in that script is already `|| true`-guarded. - parse_yaml_scalar strips single quotes too; `'1.2.3'` previously came back with the quotes and would have produced a `Blurt-'1.2.3'.dmg` filename. Added that case to release.test.sh, and corrected the comment the last commit got wrong: `CFBundleVersionSomethingElse` was never the hazard (the old regex already required the colon). The cases that did break are a prefixed key (`MyCFBundleVersion:` returned the wrong value) and a commented-out key (`# CFBundleVersion:` returned the literal key name) — both now pinned. - release-lib.sh documents that `require_project_version` must be called as a plain assignment: `local v="$(…)"` swallows the substitution's exit status, so the die prints and execution continues with an empty version. No caller does this today, but the old comment invited it and release.sh is full of `local`s. Verified: scripts/check.sh --portable, shellcheck, actionlint, release.test.sh. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HREvcTT8LHjs8vmQZ8eK5L
Third batch from the codebase review. All pre-existing; ordered by severity. - APIKeyStore's memo turned a transient Keychain failure into a permanent "no API key" lockout (HIGH). `KeychainStore.get()` collapsed every non-success status to nil, so `errSecAuthFailed` / `errSecInteractionNotAllowed` — a locked login keychain, or the user denying the item's ACL prompt, which re-signing the app forces — was memoized as `.loaded(nil)` for the process lifetime. The wizard then claimed setup was incomplete and every press failed `.apiKeyMissing`, with no recovery short of relaunching or retyping the key. This was a regression in kind: before the memo each press re-read the Keychain, so a transient failure self-healed. `KeychainStore.read()` now returns `.absent` / `.value` / `.unavailable(OSStatus)`, and only the first two are cached. `get()` is kept as the lossy convenience with a doc warning that memoizing callers must use `read()`. - APIKeyStore.set was not atomic with get. The write, read-back, and memo update now happen under one lock, so two overlapping writers can't leave the memo disagreeing with the Keychain (`hasKey` true for a key it no longer holds — a 401 the user can't explain until relaunch). - APIKeyValidator rejected good keys on any unexpected 4xx (MEDIUM). A blanket `400..<500 -> .invalid` also caught statuses that say nothing about the key: AssemblyAI retiring or moving the endpoint (404/405/410), or a proxy/captive portal interposing a 403/451. Because `.invalid` makes `APIKeySubmission` refuse to persist, the user was hard-blocked from finishing setup with a key that works fine for dictation. Narrowed to 401/403/400/422; everything else falls through to `.unreachable`, the outcome designed for "can't determine". - `.injecting` projected to `.idle`, blinking the pill out mid-dictation (MEDIUM). The shell reads an idle projection as "dismiss", so every dictation ran a spurious fade-out before "Pasted" — and when the injector's activation wait ran long (up to 350 ms), the fade completed, ordering the panel out and tearing down the pill's content before it faded back in. Worst when the user switches apps mid-transcribe. Now maps to `.processing`, holding the pill up continuously from transcribe through paste; the pinned test row moved with it. - The transcribe POST had no timeout. Only `warmUp()` set one, so the real request inherited URLRequest's 60 s default — and that is a data-*idle* timeout, not a wall clock, so a trickling connection could hold the pill on "Transcribing…" far longer with no progress and no retry. Set to 45 s: the server's own ~30 s deadline (it answers 504 past it, per the API reference) plus headroom to upload a 120 s clip. - `bridge.level` was never reset on dismiss, so each dictation's bars opened at the previous dictation's loudness for one frame — `MicCapture.stop()` cancels the meter task without a final zero yield. Cleared in `dismissPanel()`, the shared final step of every dismiss path. - DictationSession+Pipeline read `contextStream` before an await and cleared it after, so a cancelled pipeline could clear a *newer* press's stream and send that utterance with `context: nil` — losing `baseInstruction` too, which is what suppresses `[Speaker]`-style markers in the pasted text. The window is microseconds; the stream is now taken out of actor state in the same turn it's read, making the invariant local rather than scheduling-dependent. - makeRecordingInjector called the REAL NSRunningApplication.activate(). Four separator tests yanked the developer's foreground app and could fail spuriously when `liveTargetApp()`'s unordered pick landed on a background-only process whose activate() returns false. Stubbed. Also corrected TranscriptionPrompt's `characterCap` docs: the Sync API reference documents no cap on `config.prompt` (the 4096 figure belongs to `conversation_context`, where over-cap content is trimmed rather than rejected), so the "a longer prompt fails the whole request" claim was wrong. The constant stays as a deliberate sanity bound. Verified with scripts/check.sh --portable. Swift needs CI (macos-26). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HREvcTT8LHjs8vmQZ8eK5L
![aikido-pr-checks[bot]](https://avatars.githubusercontent.com/in/898896?s=60&v=4){kind=small}
Comment on lines
+131
to
+133
| pb.clearContents() | ||
| // `writeObjects` can refuse the batch; fall through to the text floor | ||
| // rather than leaving the pasteboard empty. |
There was a problem hiding this comment.
restore(_:) clears NSPasteboard before confirming writeObjects succeeds; if writeObjects fails and saved.plainText is nil, clipboard is left empty instead of preserved.
Show fix
Suggested change
| pb.clearContents() | |
| // `writeObjects` can refuse the batch; fall through to the text floor | |
| // rather than leaving the pasteboard empty. | |
| // Only clear if we have a text fallback, since writeObjects can refuse the | |
| // batch and we'd otherwise leave the pasteboard empty. | |
| guard saved.plainText != nil else { return } | |
| pb.clearContents() |
Details
✨ AI Reasoning
1) The code is trying to preserve clipboard contents on restore failures.
2) In the branch where materialized items exist, it clears the clipboard before calling the batch write.
3) If that write is refused and no text fallback exists, there is no subsequent write, so the clipboard remains empty.
4) This directly conflicts with the stated intent to leave contents unchanged when nothing can be restored, indicating a real control-flow bug rather than style.
Reply @AikidoSec feedback: [FEEDBACK] to get better review comments in the future.
Reply @AikidoSec ignore: [REASON] to ignore this issue.
More info
Collaborator
Author
There was a problem hiding this comment.
The diagnosis is right, but I'm not taking this suggestion — it would trade a near-impossible failure for a common one.
guard saved.plainText != nil else { return } before pb.clearContents() skips the item write whenever the clipboard has no plain-string flavor. So an image-only or file-only clipboard — copy a screenshot, copy a file in Finder — would never be restored at all, because plainText is nil for both. That's a far more common clipboard than a refused batch write.
The case you identified is real but requires all four of: the snapshot succeeded, at least one item materialized, writeObjects refused the batch, and there was no plain-string flavor. writeObjects returning false on a freshly-cleared pasteboard holding valid NSPasteboardItems is a programming error rather than a runtime condition, and NSPasteboard exposes no way to test a write before clearing — clearContents() is what establishes ownership, so the write cannot be validated first.
What was genuinely wrong was the comment claiming the pasteboard is left alone "since clearing it would discard content we cannot replace", which overstates the guarantee. Fixed in afb5fc1 by documenting the residual case precisely, including why the item write deliberately isn't gated on plainText.
Generated by Claude Code
There was a problem hiding this comment.
Pull request overview
This PR applies a broad “cleanup pass” across the Blurt app + BlurtEngine, aiming to reduce duplication, tighten invariants, and make behavior-critical mappings and guards more explicit—while keeping behavior effectively the same (aside from a few intentional robustness improvements like safer clipboard restore and better failure handling in scripts).
Changes:
- Consolidates duplicated logic in scripts (release/dev/check) and improves guard correctness (tool preflight, YAML parsing, coverage gating, git-diff fail-open in CI).
- Strengthens engine/app invariants and race-safety (context-stream clearing, secure-field detection, clipboard snapshot/restore semantics, overlay state mappings + tests).
- Refactors and expands tests to use shared fixtures/helpers and table-driven projections to prevent future “missing-case” gaps.
Reviewed changes
Copilot reviewed 58 out of 58 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| Tests/BlurtEngineTests/TriggerKeyStoreTests.swift | Uses public defaults key constant instead of string literal. |
| Tests/BlurtEngineTests/SystemClipboardTests.swift | Updates tests to cover full snapshot/restore and degraded snapshot behavior. |
| Tests/BlurtEngineTests/SyncSTTLimitsTests.swift | Adds arithmetic unit test for PCM-bytes → duration conversion. |
| Tests/BlurtEngineTests/Stubs/StubPCM.swift | Introduces shared canned PCM blob for stubs above min audio floor. |
| Tests/BlurtEngineTests/Stubs/StubMicCapture.swift | Reuses StubPCM.aboveMinimum instead of duplicating PCM sizing. |
| Tests/BlurtEngineTests/Stubs/InjectorTestSupport.swift | Prevents real app activation during tests by stubbing activation. |
| Tests/BlurtEngineTests/Stubs/GatedStopMic.swift | Reuses shared PCM fixture for stop-race suites. |
| Tests/BlurtEngineTests/SoundPackStoreTests.swift | Uses SoundPackStore.defaultsKey constant. |
| Tests/BlurtEngineTests/OverlayUIStateTests.swift | Converts phase/state mappings + labels to table-driven tests. |
| Tests/BlurtEngineTests/MenuBarStatusTests.swift | Converts phase→menu-bar mappings to a table-driven test (covers .noTarget). |
| Tests/BlurtEngineTests/KeychainStoreTests.swift | Adds coverage for distinguishing absent keychain item vs stored value. |
| Tests/BlurtEngineTests/HotkeyRaceTests.swift | Reuses shared PCM fixture for race tests. |
| Tests/BlurtEngineTests/FocusCaptureTests.swift | Adds coverage for untrimmed prior-slice and secure role/subrole logic. |
| Tests/BlurtEngineTests/DictationSessionSubmitTests.swift | Switches tests to shared makeSession fixture. |
| Tests/BlurtEngineTests/DictationSessionGuardTests.swift | Switches tests to shared makeSession fixture. |
| Tests/BlurtEngineTests/DictationPerformanceTests.swift | Reuses shared session fixture in performance harness. |
| Tests/BlurtEngineTests/DictationKeyRouterTests.swift | Adds tests pinning reset behavior after keyless terminal ends. |
| Sources/BlurtEngine/STT/TranscriptionPrompt.swift | Uses context.isEmpty to gate prompt building and clarifies cap rationale. |
| Sources/BlurtEngine/STT/SyncSTTLimits.swift | Adds shared duration-derivation helper for logs/callers. |
| Sources/BlurtEngine/STT/AssemblyAITranscriber.swift | Adds explicit request timeout and factors duration logging via SyncSTTLimits. |
| Sources/BlurtEngine/Pipeline/OverlayUIState.swift | Fixes .injecting overlay projection to avoid pill blink/fade-out. |
| Sources/BlurtEngine/Pipeline/DictationSession+Pipeline.swift | Fixes potential context-stream race by clearing before awaiting. |
| Sources/BlurtEngine/Injection/SystemClipboard.swift | Introduces full PasteboardSnapshot and safer restore semantics. |
| Sources/BlurtEngine/FocusCapture/FocusCapture.swift | Uses shared readers, fails-closed secure-field detection, preserves caret offsets. |
| Sources/BlurtEngine/Config/KeychainStore.swift | Adds ReadResult to distinguish absent vs unavailable reads. |
| Sources/BlurtEngine/Config/APIKeyValidator.swift | Narrows which HTTP statuses count as “invalid key” vs “unreachable”. |
| Sources/BlurtEngine/Config/APIKeyStore.swift | Avoids memoizing transient keychain read failures; makes set/get memo safer. |
| Sources/BlurtEngine/Audio/SoundPackStore.swift | Centralizes persisted-id decode via SoundPack.fromPersisted. |
| Sources/BlurtEngine/Audio/SoundPack.swift | Adds isSilent, makes catalog all private, adds fromPersisted. |
| Sources/BlurtEngine/Audio/MicCapture.swift | Uses shared duration derivation; updates meter-rate wording to 20 Hz. |
| scripts/reset-install.sh | Uses bundle id for keychain service and adds lsregister skip behavior. |
| scripts/release.test.sh | Adds regression tests for improved YAML scalar parsing. |
| scripts/release.sh | Uses shared require_tools. |
| scripts/release-publish.sh | Uses shared tool guard and adds build-provenance (built SHA == HEAD) check. |
| scripts/release-lib.sh | Adds shared tool guard + YAML scalar parsing; centralizes version parsing. |
| scripts/release-install.sh | Uses shared require_tools. |
| scripts/release-bump.sh | Uses shared tool guard + shared project version reader. |
| scripts/release-build.sh | Uses shared tool guard with optional install hint. |
| scripts/leaks.sh | Hardens leaks scan flow and makes grep pipeline safe under pipefail. |
| scripts/dev-build.sh | Sources shared release-lib helpers and uses require_tools. |
| scripts/check.sh | Adds tool_ready, hardens coverage gate, and isolates sanitizer scratch paths. |
| BLURTENGINE.md | Updates docs to reflect 20 Hz meter and safer clipboard restore behavior. |
| App/Blurt/BlurtUITests/SettingsUITests.swift | Reuses shared identifiers and element query helper. |
| App/Blurt/BlurtUITests/ReadyViewUITests.swift | Reuses shared anyDescendant element query helper. |
| App/Blurt/BlurtUITests/README.md | Updates harness documentation to match current UITEST_HOOKS + session APIs. |
| App/Blurt/BlurtUITests/DictationPipelineUITests.swift | Reuses anyDescendant helper; updates wording for session APIs. |
| App/Blurt/BlurtUITests/BlurtUITestSupport.swift | Adds XCUIElement.anyDescendant(identified:) helper + valid key constant usage. |
| App/Blurt/Blurt/Wizard/Steps/SoundStepView.swift | Uses SoundPack.fromPersisted for consistent decode/defaulting. |
| App/Blurt/Blurt/Wizard/Steps/APIKeyStepView.swift | Avoids stale cached key state across wizard/settings; uses hasKey as source of truth. |
| App/Blurt/Blurt/Wizard/ReadyView.swift | Caches ready logo once per process to avoid repeated decode on main thread. |
| App/Blurt/Blurt/UITestSupport.swift | Harness now drives DictationSession directly (press/release/cancel). |
| App/Blurt/Blurt/Overlay/OverlayWindowController.swift | Clamps once at ingress, skips redundant level assignments, resets level on hide. |
| App/Blurt/Blurt/Overlay/OverlayView.swift | Unifies pulse/breath animation logic; caps redraw interval; uses already-clamped level. |
| App/Blurt/Blurt/Hotkey/DictationKeyTap.swift | Adds syncAfterTerminalPhase() to clear stale latched gate state. |
| App/Blurt/Blurt/AppCoordinator+UITesting.swift | Updates comments to match harness driving session directly. |
| App/Blurt/Blurt/AppCoordinator.swift | Clears hotkey gate latch on terminal phases; adjusts wording. |
| AGENTS.md | Updates platform claims and corrects multiple architecture/docs details. |
| .github/workflows/check.yml | Makes changed-files detection fail-open so required check can’t be bypassed by diff errors. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+45
to
+48
| static func durationMs(ofPCMBytes byteCount: Int, rate: Int = sampleRate) -> Int { | ||
| guard rate > 0 else { return 0 } | ||
| return Int((Double(byteCount / bytesPerSample) / Double(rate)) * 1000) | ||
| } |
| [ -d "$dest" ] && "$LSREGISTER" -f "$dest" >/dev/null 2>&1 && echo " registered: $dest" || true | ||
| done | ||
| fi | ||
| fi || info "LaunchServices re-registration skipped (lsregister failed)" |
Comment on lines
+27
to
+30
| /// Wall-clock cap for the transcribe round trip: the server's own ~30 s | ||
| /// inference deadline plus headroom to upload a 120 s clip. | ||
| private static let requestTimeoutSeconds: TimeInterval = 45 | ||
|
|
CI's only failure on this PR: FocusCapture.swift:428:1: error: File Length Violation: File should contain 400 lines or less: currently contains 428 (file_length) The review fixes in 70adb62 (the non-trimming `rawStringValue` reader and the role-or-subrole secure-field check, plus their rationale comments) pushed the file 28 lines over the budget. SwiftLint runs `--strict`, so the warning is an error; it isn't installable in the Linux sandbox (GitHub release downloads are blocked by the network policy), so this only surfaced on macos-26. Moved the editable-target detection into FocusCapture+Editability.swift — `editableRoles`, `isEditableTarget`, `isElectronApp`, and `hasEditableFocusedElement` — following the precedent AGENTS.md documents for `DictationSession` (+Commands/+Observation/+Pipeline exist for exactly this budget). The seam is cohesive rather than arbitrary: everything moved serves `KeyInjector`'s pre-paste "no beep" guard, while FocusCapture.swift proper serves the press-time context capture that primes the STT prompt. FocusCapture.swift is now 341 lines and the new file 97. `systemFocusedElement()` and `stringValue()` widen from private to internal: `private` in a type's body isn't visible from an extension in another file, and the moved `hasEditableFocusedElement` calls both. Commented at each site so the widening doesn't read as an oversight. No behavior change — this is a pure file-level move. `Sources/BlurtEngine/` is an SPM target that globs its directory, so adding a file needs no xcodegen regen and can't drift the committed pbxproj. Everything else in that CI run passed: 325 engine tests in 56 suites, the coverage gate, ThreadSanitizer, AddressSanitizer, the xcodegen drift check, the app build, 23 UI tests, the leak scan (0 leaks), and swift-format. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HREvcTT8LHjs8vmQZ8eK5L
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 59 out of 59 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (2)
Sources/BlurtEngine/STT/SyncSTTLimits.swift:48
durationMscurrently performs integer division (byteCount / bytesPerSample) before converting toDouble, which truncates and can under-report the audio duration (even though this value is used in diagnostic logs). Convert toDoublefirst so the derivation is mathematically correct for any byte count.
static func durationMs(ofPCMBytes byteCount: Int, rate: Int = sampleRate) -> Int {
guard rate > 0 else { return 0 }
return Int((Double(byteCount / bytesPerSample) / Double(rate)) * 1000)
}
scripts/reset-install.sh:43
reset-install.shusesinfoinfi || info ..., but this script doesn't defineinfo(and runs withset -euo pipefail), so the error path will itself fail. Also,fi || …will not catch failures inside thelsregister -dump | …pipeline becauseset -ewill exit before reachingfi. Consider handling the failure explicitly and usingechofor the skip note.
Comment on lines
+59
to
+62
| /// thread-safe. Returns `true` whenever AX can't be consulted (process not | ||
| /// trusted) or can't resolve a focused element, so an unknowable state still | ||
| /// attempts the paste — the injector's own trust check then handles the | ||
| /// missing-permission case. |
Three of the four findings were real; the fourth had a correct diagnosis but a fix that would have regressed image-only clipboards. 1. reset-install.sh (Copilot) — a bug I introduced in 8499c0c, and worse than reported. `fi || info "…"` called `info`, which this script never defines and never sources from release-lib.sh, so under `set -euo pipefail` an lsregister failure would exit 127 on "command not found" — aborting the reset even earlier than the bug I was trying to fix. And it could never have fired anyway: making the whole `if` the left operand of `||` suppresses errexit for its body, so `fi` reports the *last* command's status, which is a `for` loop ending in `|| true`. Guarded the failing pipeline directly instead, matching the `|| true` convention the rest of the file already uses. Verified that a failing dump now prints its note and execution reaches the defaults, Keychain, and log cleanup. (Copilot's second point — that `fi || …` also fires when the `if` condition is false — is wrong: bash returns 0 for an `if` whose condition is false and which has no else. Confirmed empirically.) 2. AssemblyAITranscriber (Copilot) — my own inconsistency. The commit that added the timeout described URLRequest's 60 s default as "a data-idle timeout, not a wall clock", then labelled the replacement a "wall-clock cap". It is an idle timeout. Reworded to say what it actually bounds — stalls, not elapsed time — and why that's the right shape: the server's ~30 s deadline already bounds a request that keeps making progress, so a live-but-slow inference must not be cut off client-side. 3. SyncSTTLimits.durationMs (Copilot) — `byteCount / bytesPerSample` was integer division before the Double conversion, truncating a trailing partial sample. Unreachable for well-formed S16LE (even byte counts) and bounded by 0.0625 ms at 16 kHz on a log line, so not a live defect — but there's no reason for the expression to be wrong, so it now converts before dividing. Every value the tests pin is unchanged, including the odd-byte case. 4. SystemClipboard (Aikido) — correct that `restore` can leave the pasteboard empty when items materialized, `writeObjects` refused them, and there was no plain-string flavor. Not taking the suggested fix: gating the item write on `plainText != nil` would refuse to restore an image-only or file-only clipboard, which is far more common than a refused batch write. NSPasteboard offers no way to test a write before clearing, and `writeObjects` failing on a freshly-cleared pasteboard holding valid items is a programming error rather than a runtime condition. Documented the residual case precisely instead of leaving a comment that implies a guarantee the code doesn't provide. Verified with scripts/check.sh --portable and shellcheck. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HREvcTT8LHjs8vmQZ8eK5L
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 59 out of 59 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
Sources/BlurtEngine/FocusCapture/FocusCapture+Editability.swift:62
- The doc comment says
hasEditableFocusedElement()returnstruewhen AX can't resolve a focused element, but the implementation returnsfalsein that case (so the injector copies instead of beeping ⌘V). This mismatch makes the behavior hard to reason about and is likely to confuse future edits.
/// thread-safe. Returns `true` whenever AX can't be consulted (process not
/// trusted) or can't resolve a focused element, so an unknowable state still
/// attempts the paste — the injector's own trust check then handles the
/// missing-permission case.
| ## Transcription prompt | ||
|
|
||
| `Sources/BlurtEngine/STT/TranscriptionPrompt.swift` builds the instruction passed as the Sync STT request's `config.prompt` (see `AssemblyAITranscriber`). It's unit-tested in `Tests/BlurtEngineTests/TranscriptionPromptTests.swift`. Every built prompt opens with the fixed `baseInstruction` — `"Transcribe without speaker labels, audio event descriptions, or emotion markers."` — a negative-exclusion clause that suppresses the annotation markers (`[Speaker]`, `[door creaks]`, `[laughing]`) the model would otherwise paste into the user's text. There is deliberately **no** language directive: pinning the prompt to English hurt non-English transcription, so language is left to the model's own detection — don't reintroduce a "Transcribe in English"-style clause. `build(context:)` wraps that pivot in _contextual priming_: prior-cursor text, the selected text (the highlighted run the dictation will replace, so the model is primed on what's being rewritten — read via `kAXSelectedTextAttribute` and skipped in secure fields), a topic hint from the window title, a destination sentence from the app/field, and inline keyword boosting from the user's key terms. It's phrased per AssemblyAI's Universal-3 Pro prompting guidance (positive/authoritative wording, no "Don't"/"Avoid"/"Never") and stays under the API's 4096-character cap. Note: a "remove filler words (um, uh, like)" _content_ directive is **not** in the model's trained instruction set, so it's a no-op and was deliberately dropped — don't reintroduce it. `build(context:)` returns `nil` when there's no usable context, and passing `prompt: nil` to the transcriber omits the field so the server applies its own default prompt. | ||
| `Sources/BlurtEngine/STT/TranscriptionPrompt.swift` builds the instruction passed as the Sync STT request's `config.prompt` (see `AssemblyAITranscriber`). It's unit-tested in `Tests/BlurtEngineTests/TranscriptionPromptTests.swift`. Every built prompt opens with the fixed `baseInstruction` — `"Transcribe without speaker labels, audio event descriptions, or emotion markers."` — a negative-exclusion clause that suppresses the annotation markers (`[Speaker]`, `[door creaks]`, `[laughing]`) the model would otherwise paste into the user's text. There is deliberately **no** language directive: pinning the prompt to English hurt non-English transcription, so language is left to the model's own detection — don't reintroduce a "Transcribe in English"-style clause. `build(context:)` wraps that pivot in _contextual priming_: prior-cursor text, the selected text (the highlighted run the dictation will replace, so the model is primed on what's being rewritten — read via `kAXSelectedTextAttribute` and skipped in secure fields — detected by AX role **or** subrole, failing closed when the role can't be read, so a password can't reach the prompt), a topic hint from the window title, a destination sentence from the app/field, and inline keyword boosting from the user's key terms. It's phrased per AssemblyAI's Universal-3 Pro prompting guidance (positive/authoritative wording, no "Don't"/"Avoid"/"Never") and stays under the API's 4096-character cap. Note: a "remove filler words (um, uh, like)" _content_ directive is **not** in the model's trained instruction set, so it's a no-op and was deliberately dropped — don't reintroduce it. `build(context:)` returns `nil` when there's no usable context, and passing `prompt: nil` to the transcriber omits the field so the server applies its own default prompt. |
The PR-diff review found that periphery has never run on this branch: check.sh runs swiftlint before periphery and aborts under `set -e`, so the file_length failure masked both. Two declarations it would reach: - `SoundPack.find(id:)` -> internal. Its only cross-module caller was `SoundStepView`, replaced by `fromPersisted` earlier in this PR; the `isSilent` doc four declarations above already cites this exact rule. - `KeychainStore.get()` deleted. Both callers moved to `read()` when the memo fix landed, leaving only test assertions periphery can't see. Its own doc said any caching caller must use `read()` — there are none left to protect. The five assertions now use `read()`, which is also more precise about absent vs. value. The design calls, made: 1. Key Terms field — fixed the double write; kept `KeyTermsStore.set`. `@AppStorage` is now the only writer. Normalization already happens on read (`get` trims, `parse` trims and dedupes), so a blank field still reads as no terms. `set` stays because BLURTENGINE.md is an embedder-facing guide and a write API is legitimately useful there — if periphery disagrees, CI will say so, which beats speculating about it a second time. 2. `isEditableTarget` — left the permissive bias alone. Refusing a custom text view that reports an unknown role is a worse, more common regression than the speculative slider paste, and the denylist fix needs per-app validation with Accessibility Inspector. Took the free part instead: `hasInsertionPoint` now requires the range to actually decode rather than trusting `.success` alone, so the signal means what its name says. 3. `hideOverlay` — corrected the docs rather than the behavior. Disarming the tap would make dictation depend on `ensureRunning()` succeeding again, and a tap that fails to reinstall is a far worse failure than an error flash. `WizardController` already surfaces the setup window on the same edge. 4. Press-time AX capture moved off the cooperative pool onto a concurrent Dispatch queue. `captureFieldContext` makes ~6 synchronous cross-process AX round trips bounded only by the 1 s messaging timeout, and the cooperative pool is core-count-sized without overcommit — so repeated press/cancel cycles against a beachballing app could park every cooperative thread and stall the runtime, including this actor. Did NOT lower `axMessagingTimeoutSeconds`: that trades prompt priming for latency and wants measurement on a Mac. 5. `release-publish.sh` creates the release as a draft, verifies the re-downloaded assets (sha + staple), then flips it live with `--latest`. `--latest` repoints the URL README.md links, so the old order exposed a possibly-truncated upload for the length of the verify step. 6. Added a `gate` job that runs `if: always()` and fails unless `check` succeeded or was skipped by the docs-only design. GitHub reports a skipped required check to branch protection as passing, so anything that makes `check` skip bypasses the macOS gate on a green-looking PR. This only protects the repo once branch protection requires `gate` — a repo settings change I can't make. Not taken: switching `X-AAI-Model` from the `u3-sync-pro` legacy alias to the canonical `universal-3-5-pro`, and moving key terms to the dedicated `config.keyterms_prompt` field. Both change the live STT request with no way to hear the result here. Also from the review: acted on `router.reset()`'s discarded-recording result in `syncAfterTerminalPhase` (if the phase-stream loop lags, dictation N's terminal phase can clear N+1's live gate state, which would otherwise run to the ~115 s auto-release cap); pinned the APIKeyValidator narrowing with a 404/405/410/451 case and renamed the test that still claimed all 4xx are invalid; corrected three docs still asserting the API 4096-character prompt cap the source now says doesn't exist; moved three misplaced `release.test.sh` cases under the right header; and corrected a dismissPanel comment that overstated `hide()` coverage. Verified with scripts/check.sh --portable, actionlint, shellcheck, release.test.sh. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HREvcTT8LHjs8vmQZ8eK5L
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 63 out of 63 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
Sources/BlurtEngine/FocusCapture/FocusCapture+Editability.swift:62
- Doc comment contradicts the implementation: when AX is trusted but there’s no focused element, the function returns false (copy-only) to avoid beeping ⌘V into non-editable UI. The comment currently says this case returns true.
/// thread-safe. Returns `true` whenever AX can't be consulted (process not
/// trusted) or can't resolve a focused element, so an unknowable state still
/// attempts the paste — the injector's own trust check then handles the
/// missing-permission case.
Comment on lines
21
to
+25
| private func withClipboardRestored(_ body: () throws -> Void) rethrows { | ||
| let pb = NSPasteboard.general | ||
| let saved = pb.string(forType: .string) | ||
| let clip = SystemClipboard() | ||
| let saved = clip.snapshot() | ||
| defer { | ||
| pb.clearContents() | ||
| if let saved { pb.setString(saved, forType: .string) } | ||
| if let saved { clip.restore(saved) } |
Four things were sitting on the app side of the engine/shell boundary, each re-deriving or hiding a decision the engine should own. AGENTS.md's split is that the engine owns pipeline logic, persistence stores, and pure state projections — partly so `swift test` can cover them, since the app has only XCUITests. 1. "Which failures mean setup isn't finished" was pattern-matched twice. `OverlayUIState` matched `.failed(.apiKeyMissing)` to render calm idle, and `AppCoordinator.render` matched the same case again to decide navigation. Two copies of one engine rule, in two layers. `PipelinePhase.setupBlocker` (over an internal `BlurtError.isSetupBlocker`) now classifies it once and both derive from it. That matters for the obvious next blocker — a press-time mic-permission check — where missing the engine site gives a red flash and missing the shell site gives a press that silently does nothing. Renamed `onMissingAPIKey` to `onSetupBlocked`, since it no longer means only that. 2. The pill's dragged origin was persisted by the AppKit controller under two private `UserDefaults` keys, so no reset sweep knew about them — a pill dragged during a UI-test run survived into later runs, exactly the staleness `PersistedSettings.allDefaultsKeys` exists to prevent. Now an engine `OverlayOriginStore` in the roster, next to the `OverlayPlacement` clamping it feeds. It also fixes a latent read bug: `double(forKey:)` reports 0 for a missing key, so a half-written pair used to place the pill at an implied origin instead of falling back to the default. 3. `bottomOffset: 80 - Self.shadowMargin` put placement policy at the call site. `OverlayPlacement` exists so placement is unit-tested, but the clearance and the pill-vs-panel correction were outside it — changing the shadow margin moved the pill 28 pt and no test noticed. Added `defaultBottomClearance` and `panelOrigin(…shadowMargin:)`; the shell now passes only what it owns. 4. The overlay's redraw cap was `1.0 / 20.0` with a comment claiming it matched `MicCapture.meterInterval` — which was `private`, so nothing enforced it. `meterIntervalSeconds` is now public and the single definition; the meter's `Duration` derives from it and the view reads it. New tests for all of it (the point of moving logic to the engine): the clearance and shadow correction, the origin store's round-trip / half-written / clear / in-roster behavior, and the setup-blocker classification. Also from the PR-diff review: `SoundPack.find(id:)` -> internal and `KeychainStore.get()` deleted (periphery never ran on this branch — check.sh aborts at swiftlint before reaching it), plus the earlier design calls. Verified with scripts/check.sh --portable. Swift needs CI. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HREvcTT8LHjs8vmQZ8eK5L
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 70 out of 70 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
Sources/BlurtEngine/Pipeline/DictationSession.swift:213
DispatchQueue.asyncexecutes a@Sendableclosure. This block capturescaptured(anNSRunningApplication?, non-Sendable) viacaptured?.processName, which can trip Swift 6 strict-concurrency checks (warnings-as-errors) and fail the build. Snapshot theprocessName(aString?, Sendable) before dispatching and capture only that inside the closure.
Both were guesses at API behavior rather than compatibility with anything real,
so nothing needs to keep them.
- The error-body decode tried an `error` key first, then `message`, then
`detail`. The API reference documents exactly two shapes — `{error_code,
message}` for the request/audio/server errors (400, 413, 415, 500, 503, 504)
and `{detail}` for auth and rate limiting (401, 429). `error` appears in none
of them; the comment justifying it said the field name had "been seen" varying,
which the reference doesn't support. Now reads `message` then `detail`, and the
test that pinned the `error` key is gone.
- `X-AAI-Model` sent `u3-sync-pro`. That's documented as a legacy alias —
`universal-3-5-pro` is the canonical value and the only entry in the endpoint's
schema enum. Switched, and updated the four docs plus the guardrails skill that
named the alias.
Deliberately KEPT: the raw-body arm of `errorMessage`. It isn't compatibility
with an old API shape — it's what turns a response the API never promised (a
proxy's HTML 502, a captive-portal page) into something diagnosable instead of a
bare status code. Removing it would make failures harder to debug, which isn't
what dropping fallbacks is for. Its doc now says so explicitly.
Also left alone, because "unreleased" doesn't make them safe to remove: the
decode-with-default fallbacks on persisted values (`TriggerKey.fromPersisted`,
`SoundPack.fromPersisted`, `KeyTermsStore`'s trim-on-read). Those guard against
an unknown or absent `UserDefaults` value rather than an older client, so
removing them would misbehave on garbage input, not just drop compatibility.
One option not taken: the documented error body also carries a machine-readable
`error_code` (`audio_too_short`, `inference_timeout`, …) that could sharpen the
messages users see. That's a feature, not a fallback removal.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HREvcTT8LHjs8vmQZ8eK5L
From a full boundary audit of the repo, on top of the four in e1e59e8. - The trigger and sound-pack pickers wrote the raw `@AppStorage` slot (`triggerKeyCode = newValue.rawValue`) instead of the store, so `TriggerKeyStore.triggerKey`'s and `SoundPackStore.soundPack`'s setters had no production caller at all — only engine tests. AGENTS.md already describes the intended design as a picker that "writes `TriggerKeyStore`". Both now write through the store, with `@AppStorage` kept for what its own doc claims it's for: observing the key so the view re-renders. Behavior is identical today because the read path (`fromPersisted`) was already shared — the cost was that a change to the persisted encoding would keep `swift test` green while the pickers wrote the old form, surfacing as a silently dead rebind. (Key terms stays a genuine exception: its `set` normalizes, which is what fought the user's keystrokes.) - `RecentDictations.Entry.justNowThreshold` is published, and ReadyView derives its timestamp refresh cadence from it instead of a bare `30` justified by a comment. Same unenforceable coupling `MicCapture.meterIntervalSeconds` just fixed; this was the remaining instance. - `PermissionsStepView`'s footer named the trigger key via a one-shot `TriggerKeyStore()` read. Settings is reachable with ⌘, while that page is showing, so a rebind left the footer naming the old key. Now `@AppStorage` + `fromPersisted`, matching ReadyView / MenuBarScene / HotkeyStepView. - Removed `SoundPack.synth`. Its doc said "for the ready-screen credit" — ReadyView has no such credit. Its only consumer appended it to each picker row, under a section header that already names the synth ("Yamaha DX7 · ROM1A" → "Brass 1 · Yamaha DX-7"), so the credit was redundant where it did appear. It also derived a value by prefix-matching a *display* string, so adding a third synth to the generated catalog would have silently returned nil. Docs: BLURTENGINE.md called `phaseStream()` a single-observer stream that supersedes previous callers — it keeps a dictionary of continuations and is explicitly multi-observer, so hosts were told the opposite of the contract. It also listed right ⌃ as a trigger key; `TriggerKey` has only right ⌘, right ⌥, fn. NOT done, with reasons: - Folding a mic-permission check into the engine's `readinessCheck` (so a revoked mic routes to setup instead of flashing red, and the documented-but-dead `.microphonePermissionDenied` becomes real). It looks like wiring but isn't: `PermissionsChecker.micGranted()` is `recordPermission == .granted`, so `.undetermined` — first run, never asked — reads false. Gating the press on it would refuse the very first dictation and never trigger the system prompt. Doing it properly means `PermissionStatus.microphone` distinguishing denied from undetermined, which is a design change to the permissions model. - Moving `APIKeySubmission.Outcome`'s user-facing copy and its inline-vs-alert classification into the engine. The repo's convention says yes (`BlurtError.errorDescription`, `OverlayUIState.accessibilityLabel`, `noticeDwellSeconds` are all engine-side), but for a package documented as embeddable there's a real argument that a host owns its own wording. Worth a decision rather than a drive-by. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HREvcTT8LHjs8vmQZ8eK5L
Comment on lines
+130
to
+135
| if !pbItems.isEmpty { | ||
| pb.clearContents() | ||
| // `writeObjects` can refuse the batch; fall through to the text floor | ||
| // rather than leaving the pasteboard empty. | ||
| if pb.writeObjects(pbItems) { return } | ||
| } |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 77 out of 77 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
Sources/BlurtEngine/FocusCapture/FocusCapture+Editability.swift:62
- Docstring says
hasEditableFocusedElement()returns true when AX can't resolve a focused element, but the implementation returns false in that case (so the injector copies instead of pasting/beeping). Update the comment to match the actual behavior to avoid future callers relying on the wrong contract.
Comment on lines
+61
to
+64
| // A blanket `400..<500` also caught the cases that say nothing about the key | ||
| // — AssemblyAI retiring or moving this endpoint (404/405/410), or a corporate | ||
| // proxy or captive portal interposing its own 403/451 page — and told the | ||
| // user their good key was rejected. Everything else falls through to |
`PersistedSettings.allDefaultsKeys` gained OverlayOriginStore's two keys when the store moved into the engine, but the roster test still pinned four keys and named only the four stores — so the suite failed on CI (the one issue in an otherwise green 332-test run) at exactly the assertion it exists to make. Name both new keys in `rosterCoversEveryStore` and raise the count to 6, with the "why" recorded there: a point costs two keys, and they were the keys whose staleness motivated the roster in the first place. The duplicate membership assertion in `OverlayOriginStoreTests` goes away — one assertion site, in the suite that owns the roster. Also reflow the `PersistedSettings` doc comment, which the same change had left wrapped mid-clause at 111 columns. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HREvcTT8LHjs8vmQZ8eK5L
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 78 out of 78 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (2)
Sources/BlurtEngine/FocusCapture/FocusCapture+Editability.swift:62
- The doc comment for
hasEditableFocusedElement()says it returnstruewhen AX “can’t resolve a focused element”, but the implementation returnsfalsein that case (and treats it as non-editable). Please adjust the comment so it matches the actual behavior and doesn’t mislead callers.
Sources/BlurtEngine/Config/APIKeyValidator.swift:64 - This comment cites a proxy/captive-portal “403/451 page” as an example of a 4xx that says nothing about the key, but the current switch still classifies 403 as
.invalid. Either treat 403 as.unreachable, or (at least) remove 403 from this explanatory text so it matches the code’s policy.
// A blanket `400..<500` also caught the cases that say nothing about the key
// — AssemblyAI retiring or moving this endpoint (404/405/410), or a corporate
// proxy or captive portal interposing its own 403/451 page — and told the
// user their good key was rejected. Everything else falls through to
Comment on lines
32
to
36
| ForEach(SoundPack.groups, id: \.self) { group in | ||
| Section(group) { | ||
| ForEach(SoundPack.voices(in: group)) { pack in | ||
| Text(displayLabel(for: pack)).tag(pack) | ||
| Text(pack.label).tag(pack) | ||
| } |
`KeychainStore.read()`'s introduction left two blank lines before `set(_:)`, which swift-format rejects ([RemoveLine] / [TrailingWhitespace] at 47-49). It was the only formatting error in the tree, and it sat behind the roster test failure that aborted check.sh before the swift-format step ever ran. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HREvcTT8LHjs8vmQZ8eK5L
SwiftLint had not run on this branch since the file_length failure two rounds ago — check.sh aborts at the first failing step, so the roster test and then swift-format each masked it. It reports three errors, all from the same batch of changes: - FocusCapture.swift:92 and :224 — orphaned_doc_comment. Splitting out FocusCapture+Editability.swift added a plain `//` note explaining why each symbol is internal rather than private, and putting it *between* the doc comment and the declaration detaches the doc comment. Move the note above the `///` block, which keeps it out of the rendered documentation either way. - ReadyView.swift:118 — closure_parameter_position. The TimelineView cadence expression pushed `timeline in` onto its own line. Hoist the cadence to `timestampRefresh`, which shortens the header enough for the parameter to sit on the opening-brace line and gives the derivation a name instead of a four-line comment. Swept the whole tree for both patterns rather than fixing only the reported lines: these two sites are the only doc-comment/plain-comment inversions, and there are no other closure parameters stranded after an opening brace. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HREvcTT8LHjs8vmQZ8eK5L
Comment on lines
+50
to
+53
| public static let meterIntervalSeconds: Double = 0.05 | ||
|
|
||
| /// `meterIntervalSeconds` as the `Duration` the meter task sleeps for. | ||
| private static let meterInterval = Duration.seconds(meterIntervalSeconds) |
periphery — running for the first time on this branch, after swift-format and swiftlint finally cleared — reports `KeyTermsStore.set(_:)` as unused, and it is right. Unlike the trigger-key and sound-pack setters (which the pickers now write through), this one has no production caller *by design*: `KeyTermsStepView` binds `@AppStorage` straight to the defaults key, because routing writes through a normalizing setter deleted a trailing space as the user typed it — the trimmed value came back through the binding as an external change. That note already lived on the view; it now lives on the store too, so the absence of a setter reads as deliberate rather than as an oversight to "fix". Keeping the method would have been worse than deleting it: it is exactly the footgun the view's comment warns against, sitting in the engine's public API inviting a future caller to reintroduce the bug. The three tests that exercised it now write the defaults slot directly, which is what production actually does. That makes them a better test of the real contract — normalization is on the read side — and adds the two cases the setter tests couldn't express: an unset key and a field the user emptied to "" both read as no terms, so the prompt never carries an empty vocabulary clause. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HREvcTT8LHjs8vmQZ8eK5L
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 80 out of 80 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
Sources/BlurtEngine/Config/APIKeyValidator.swift:65
- The comment says a proxy/captive portal can interpose a "403/451" page (which would imply 403 should fall through to
.unreachable), but the switch explicitly treats 403 as.invalid. Either treat 403 as unreachable or adjust the comment so it doesn't claim 403 is a non-key-related status.
// A blanket `400..<500` also caught the cases that say nothing about the key
// — AssemblyAI retiring or moving this endpoint (404/405/410), or a corporate
// proxy or captive portal interposing its own 403/451 page — and told the
// user their good key was rejected. Everything else falls through to
// `.unreachable`, the outcome designed for "couldn't determine".
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.
Ran the four cleanup review angles (reuse, simplification, efficiency,
altitude) over the whole tree and applied the findings that are behavior-
preserving.
Reuse — one definition per rule:
require_tools(with an optional--hint) andparse_yaml_scalar; release.sh / release-{bump,build,install,publish}.shand dev-build.sh now share them instead of carrying four copies of the
preflight loop, a fifth copy of
require_project_version, and a secondcopy of
info/die. The YAML read now matches the key as a whole field,so
CFBundleVersioncan't also matchCFBundleVersionSomethingElse.tool_ready, collapsing seven copy-pasted optional-linterguards (each with its own skip note and
cd).BUNDLE_IDrather thanrespelling the bundle id, so the reset can't half-miss on a rotation.
fromPersisted, mirroringTriggerKey.fromPersisted, sothe store and the @AppStorage picker share one decode-with-default rule.
checked
stringValuereader instead of a second inline CF downcast.StubPCM.aboveMinimum(was restatedthree times), the defaults-key literals become their public constants, the
UI suite uses
UITestIdentifiers.validAPIKeyinstead of a raw sentinel,an
anyDescendant(identified:)helper replaces four copies of thetype-agnostic element query, and the session suites use the existing
makeSessionfixture (including removing a private shadowing copy).Simplification:
pulsingOpacityview modifier replaces the duplicated raised-cosinebreathing in TranscribingLabel and RecordingTag, and one
overlayAnimationIntervalreplaces the 20 Hz literal spelled three times.mappings become
@Test(arguments:)tables (the precedentDictationKeyGateTests sets), which also surfaced that
.noTargethad nomenu-bar row — now covered.
UITEST_HOOKS harness; it drives
session.press()/release()/cancel().captureSampleRatealias.SoundPack.allis private now: the picker builds fromgroups/voices(in:)and only
find(id:)ever used it, so its doc no longer overstates its role.Efficiency:
@observable invalidates on assignment, and the meter floors ambient to
exactly 0, so silent ticks were rebuilding the bar row 20x/s for nothing.
differently-flagged builds stop invalidating each other's caches.
Altitude:
TranscriptionContext.isEmptyratherthan re-deriving the emptiness test field by field, so a newly added
context signal can't be silently dropped.
instead of clamped again three layers deep.
SoundPack.isSilentreplacesgroup == nilspelled three times.SyncSTTLimits.durationMs(ofPCMBytes:rate:)owns the PCM→ms derivationboth the capture and upload logs were computing separately.
Docs: corrected AGENTS.md's paste-path paragraph (pasteSettleNanos ->
pasteSettleDuration, HID tap -> annotated session tap, and the settle/restore
runs on a chained follow-up task rather than blocking
insert), the#if DEBUG->#if UITEST_HOOKSharness gate in AGENTS.md and the UITestsREADME, the macOS 26 -> 15 platform claims, and five stale "~30 Hz" meter
references now that meterInterval is 20 Hz.
Skipped, deliberately — behavior changes or work beyond a cleanup pass:
generalizing
isElectronAppto an AX-opaque probe, replacingwaitUntilFrontmost's poll with the activation notification, snapshotting theclipboard at press time, reordering
setPhase(.recording)beforesetTargetApp, mapping.injectingto.processing, making the permissionpoll event-driven, a
PipelinePhase.setupBlockerclassification, lazy AXprobes in the focus capture, emitting
synthas a generated SoundPack field(needs the sound generator re-run on a Mac), and folding
AppCoordinator+UITesting.swift into UITestSupport.swift (needs an xcodegen
regen). Also left alone: KeyTermsStepView writes its defaults slot twice per
keystroke with two different normalization rules, which is a real (minor) bug
— but the clean fix orphans the public
KeyTermsStore.set, and whether thatstays public is an API call, not a cleanup.
Verified with
scripts/check.sh --portable(actionlint, prettier, xmllint,markdownlint, shellcheck, release.test.sh). The Swift build, tests,
sanitizers, and lint gates run on CI (macos-26), which is the authority.
Co-Authored-By: Claude Opus 5 noreply@anthropic.com
Claude-Session: https://claude.ai/code/session_01HREvcTT8LHjs8vmQZ8eK5L