fix: resolve SIGTRAP crash and blank overlay caused by AsyncParsableCommand#153
Conversation
…patibility MainActor.assumeIsolated crashes with SIGTRAP when the caller is not on the main thread, which happens under LaunchAgent. DispatchQueue.main.sync explicitly dispatches to the main thread regardless of the calling context. Closes #152 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR introduces Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI (ParsableCommand)
participant Sync as Sync run()
participant Task as Task (async scope)
participant Async as Async run()
participant Sema as DispatchSemaphore
participant Result as Result<Void, Error>
CLI->>Sync: invoke run() throws
Sync->>Sync: create semaphore
Sync->>Task: Task { `@MainActor` in ... }
Task->>Async: call await run() async throws
Async->>Async: execute async logic
Async-->>Task: return or throw
Task->>Result: capture success/error
Task->>Sema: signal()
Sync->>Sema: wait() blocks
Sema-->>Sync: unblock
Sync->>Result: get() rethrow
Sync-->>CLI: return or throw
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
Fixes a SIGTRAP crash when lyra daemon is launched via LaunchAgent by changing how daemon startup code is executed on the main thread.
Changes:
- Bump CLI version to
2.5.8. - Replace
MainActor.assumeIsolatedwith a main-queue dispatch inDaemonCommand.run().
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| Sources/CLI/Commands/DaemonCommand.swift | Changes daemon startup to run its AppKit initialization via the main dispatch queue. |
| Sources/CLI/Resources/version.txt | Updates the CLI version string to 2.5.8. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func run() { | ||
| MainActor.assumeIsolated { | ||
| DispatchQueue.main.sync { | ||
| let app = NSApplication.shared |
There was a problem hiding this comment.
DispatchQueue.main.sync will deadlock if run() is invoked on the main thread/queue (e.g., running lyra daemon directly from a terminal, where execution commonly starts on the main thread). Consider guarding with Thread.isMainThread (execute the body directly when already on main) and only using DispatchQueue.main.sync when on a background thread, or switch to an async dispatch + keeping the process alive if you don't need to block the caller thread.
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
…ommand RootCommand was changed to AsyncParsableCommand for lyra track/healthcheck, but Swift's async runtime takes ownership of the main thread, making NSApplication.run() unable to drive SwiftUI rendering (blank overlay) and causing SIGTRAP when MainActor.assumeIsolated is called from a non-main thread. - Add AsyncRunnable protocol: bridges async run() via DispatchSemaphore without starting the async runtime on the main thread - Revert RootCommand to sync ParsableCommand - Revert seekEnd guard move that broke wallpaper display (#154) - Add Scripts/check-overlay.swift for verifying overlay rendering - Add AsyncParsableCommand module with 7 unit tests Closes #152 Closes #154 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
GeneralD
changed the title
- Add AsyncParsableCommand module to dependency graph and layer summary - Document AsyncRunnable vs AsyncParsableCommand incompatibility - Add check-overlay.swift to Build & Test section - Update RootCommand description (AsyncParsableCommand → ParsableCommand) - Move CLAUDE.md to .claude/CLAUDE.md Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
Tests/AsyncParsableCommandTests/AsyncRunnableTests.swift (1)
92-100: Add the main-thread reentry case here.
runsOffMain()only checks where the async body starts. The#152regression was the next step: hopping from that off-main context back to the main thread. A small command in this suite that callsDispatchQueue.main.syncinsiderun()and asserts completion would cover the path Codecov still reports as uncovered.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Tests/AsyncParsableCommandTests/AsyncRunnableTests.swift` around lines 92 - 100, Add a new test in the ThreadBehavior suite that verifies reentry to the main thread: create a small command (e.g., MainReentryCommand) whose run()/execute() body dispatches back to the main queue using DispatchQueue.main.sync and completes, then invoke it with the same test harness (similar to runsOffMain using runSync or the async runner) and assert that completion occurs on the main thread (check Witnesses.current.threadLabel == "com.apple.main-thread" or equivalent). Ensure the test name describes main-thread reentry and place it alongside runsOffMain to exercise the regression path.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Scripts/check-overlay.swift`:
- Around line 13-19: The cast to [String: Int] for kCGWindowBounds fails because
the values are NSNumber/CGAFloat; update the extraction from lyraWindows to read
bounds as a [String: Any] (or CFDictionary) and convert the numeric values to
Ints (or use CGRect(dictionaryRepresentation:)) before deriving width/height;
specifically modify the code around lyraWindows and the kCGWindowBounds access
to either parse bounds as [String: Any] and coerce bounds["Width"]/["Height"] to
numbers and then Int(...) or obtain boundsDict = w[kCGWindowBounds as String] as
CFDictionary? and call CGRect(dictionaryRepresentation:) to get
rect.width/rect.height and then Int(rect.width)/Int(rect.height) for width and
height used later.
In `@Sources/AsyncParsableCommand/AsyncRunnable.swift`:
- Around line 29-43: The current mutating run() uses
UnsafeMutableTransferBox(self) named command to call await
command.wrappedValue.run() but never writes the boxed, potentially-mutated value
back into self; after semaphore.wait() and before evaluating
result.wrappedValue.get(), assign self = command.wrappedValue so any mutations
performed by command.wrappedValue.run() are propagated back to the original
value, then call try result.wrappedValue.get() to rethrow any error; reference
AsyncRunnable.run, command (UnsafeMutableTransferBox) and result.wrappedValue
when making this change.
---
Nitpick comments:
In `@Tests/AsyncParsableCommandTests/AsyncRunnableTests.swift`:
- Around line 92-100: Add a new test in the ThreadBehavior suite that verifies
reentry to the main thread: create a small command (e.g., MainReentryCommand)
whose run()/execute() body dispatches back to the main queue using
DispatchQueue.main.sync and completes, then invoke it with the same test harness
(similar to runsOffMain using runSync or the async runner) and assert that
completion occurs on the main thread (check Witnesses.current.threadLabel ==
"com.apple.main-thread" or equivalent). Ensure the test name describes
main-thread reentry and place it alongside runsOffMain to exercise the
regression path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 48f188f5-9ad9-404e-9f37-0b788d42d429
📒 Files selected for processing (8)
Package.swiftScripts/check-overlay.swiftSources/AsyncParsableCommand/AsyncRunnable.swiftSources/CLI/Commands/HealthcheckCommand.swiftSources/CLI/Commands/RootCommand.swiftSources/CLI/Commands/TrackCommand.swiftSources/Presenters/Wallpaper/WallpaperPresenter.swiftTests/AsyncParsableCommandTests/AsyncRunnableTests.swift
| public mutating func run() throws { | ||
| let command = UnsafeMutableTransferBox(self) | ||
| let result = UnsafeMutableTransferBox(Result<Void, any Error>.success(())) | ||
| let semaphore = DispatchSemaphore(value: 0) | ||
| Task { | ||
| do { | ||
| try await command.wrappedValue.run() | ||
| result.wrappedValue = .success(()) | ||
| } catch { | ||
| result.wrappedValue = .failure(error) | ||
| } | ||
| semaphore.signal() | ||
| } | ||
| semaphore.wait() | ||
| try result.wrappedValue.get() |
There was a problem hiding this comment.
Propagate mutations back to self before returning.
Line 30 boxes a copy of self, but Lines 42-43 never write the mutated value back. Any state changes made inside run() async throws are discarded, so this is not a faithful implementation of the mutating run() contract.
🛠️ Proposed fix
Task {
do {
try await command.wrappedValue.run()
result.wrappedValue = .success(())
} catch {
result.wrappedValue = .failure(error)
}
semaphore.signal()
}
semaphore.wait()
+ self = command.wrappedValue
try result.wrappedValue.get()Do the assignment before get() so mutations made before a thrown error are preserved too.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public mutating func run() throws { | |
| let command = UnsafeMutableTransferBox(self) | |
| let result = UnsafeMutableTransferBox(Result<Void, any Error>.success(())) | |
| let semaphore = DispatchSemaphore(value: 0) | |
| Task { | |
| do { | |
| try await command.wrappedValue.run() | |
| result.wrappedValue = .success(()) | |
| } catch { | |
| result.wrappedValue = .failure(error) | |
| } | |
| semaphore.signal() | |
| } | |
| semaphore.wait() | |
| try result.wrappedValue.get() | |
| public mutating func run() throws { | |
| let command = UnsafeMutableTransferBox(self) | |
| let result = UnsafeMutableTransferBox(Result<Void, any Error>.success(())) | |
| let semaphore = DispatchSemaphore(value: 0) | |
| Task { | |
| do { | |
| try await command.wrappedValue.run() | |
| result.wrappedValue = .success(()) | |
| } catch { | |
| result.wrappedValue = .failure(error) | |
| } | |
| semaphore.signal() | |
| } | |
| semaphore.wait() | |
| self = command.wrappedValue | |
| try result.wrappedValue.get() | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/AsyncParsableCommand/AsyncRunnable.swift` around lines 29 - 43, The
current mutating run() uses UnsafeMutableTransferBox(self) named command to call
await command.wrappedValue.run() but never writes the boxed, potentially-mutated
value back into self; after semaphore.wait() and before evaluating
result.wrappedValue.get(), assign self = command.wrappedValue so any mutations
performed by command.wrappedValue.run() are propagated back to the original
value, then call try result.wrappedValue.get() to rethrow any error; reference
AsyncRunnable.run, command (UnsafeMutableTransferBox) and result.wrappedValue
when making this change.
There was a problem hiding this comment.
Intentional. The actual commands (TrackCommand, HealthcheckCommand) communicate via stdout and exit codes, not struct mutation. This is specified in AsyncRunnableCommandTests.swift → "sync run() calls async run() and completes" verifies side effects via an actor witness, not mutation.
There was a problem hiding this comment.
@GeneralD, understood — since all conforming commands express results through stdout and ExitCode rather than struct field mutations, propagating command.wrappedValue back to self is unnecessary. The actor-based witness pattern in the tests is a clear specification of this contract. I'll note this for future reviews.
✏️ Learnings added
Learnt from: GeneralD
Repo: GeneralD/lyra PR: 153
File: Sources/AsyncParsableCommand/AsyncRunnableCommand.swift:29-43
Timestamp: 2026-04-04T15:35:36.749Z
Learning: In `Sources/AsyncParsableCommand/AsyncRunnableCommand.swift` (GeneralD/lyra), the `mutating run() throws` bridge intentionally does NOT write the boxed `command.wrappedValue` back to `self` after `semaphore.wait()`. All conforming commands (`TrackCommand`, `HealthcheckCommand`) communicate results via stdout and `ExitCode`, not via struct mutation. Do not flag the missing `self = command.wrappedValue` assignment as a bug; side effects are verified via an actor witness in tests, not by inspecting mutated struct fields.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: GeneralD
Repo: GeneralD/lyra PR: 149
File: Sources/CLI/Commands/TrackCommand.swift:54-59
Timestamp: 2026-04-01T14:09:10.078Z
Learning: In `Sources/CLI/Commands/TrackCommand.swift` (GeneralD/lyra), the `resolvedMetadata(track:)` helper intentionally runs `metadataUseCase.resolveCandidates(track:)` when either `--resolve` OR `--lyrics` is set. This is because `resolveCandidates` strips noise words and brackets (e.g. `"【MV】Song Name / Artist - Official Video"`) and generates title/artist permutations that LRCLIB requires for accurate lyrics matching. Metadata resolution is a prerequisite for effective lyrics lookup, not an independent concern. Do not flag `guard resolve || lyrics` as incorrect coupling.
Learnt from: GeneralD
Repo: GeneralD/lyra PR: 145
File: Sources/WallpaperDataSource/FindExecutable.swift:3-13
Timestamp: 2026-03-28T00:56:53.421Z
Learning: In the Lyra Swift codebase, `ObjCBool` is intentionally avoided. During code review, do not recommend or introduce APIs that require an `ObjCBool` out-parameter; instead, prefer pure-Swift alternatives and Swift-first APIs. For example, when checking file executability, avoid suggesting `FileManager.fileExists(atPath:isDirectory:)` (which uses an `ObjCBool`) as a replacement for `isExecutableFile(atPath:)`.
Aligns with ParsableCommand / AsyncParsableCommand naming convention. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
Sources/AsyncParsableCommand/AsyncRunnableCommand.swift (1)
29-43:⚠️ Potential issue | 🟠 MajorWrite the mutated boxed command back to
selfbefore rethrowing.
UnsafeMutableTransferBox(self)boxes a copy. Any state changes made insiderun() async throwsstay incommand.wrappedValue; without copying that value back aftersemaphore.wait(), the sync bridge drops those mutations and breaks themutatingcontract.🛠️ Proposed fix
} semaphore.wait() + self = command.wrappedValue try result.wrappedValue.get()This checks the bridge implementation directly. Expected result: the
sedoutput showslet command = UnsafeMutableTransferBox(self), and thergsearch finds noself = command.wrappedValuewrite-back.#!/bin/bash sed -n '29,44p' Sources/AsyncParsableCommand/AsyncRunnableCommand.swift printf '\n-- write-back search --\n' rg -n 'self\s*=\s*command\.wrappedValue' Sources/AsyncParsableCommand/AsyncRunnableCommand.swift🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/AsyncParsableCommand/AsyncRunnableCommand.swift` around lines 29 - 43, The boxed `command` (created via UnsafeMutableTransferBox(self)) holds a mutated copy from the async Task but you never write it back into self; after semaphore.wait() and before rethrowing the error you must assign the mutated value back (self = command.wrappedValue) so the mutating contract of run() is preserved—insert that write-back immediately after semaphore.wait() and before calling try result.wrappedValue.get()..claude/scripts/check-overlay.swift (1)
15-17:⚠️ Potential issue | 🟠 MajorParse
kCGWindowBoundsas a Core Graphics dictionary.
CGWindowListCopyWindowInfodoes not hand backkCGWindowBoundsas[String: Int]. With the current cast,boundsfalls back to[:], sowidth/heightstay0and the script can fail even when the overlay is present.🛠️ Proposed fix
for w in lyraWindows { let pid = w["kCGWindowOwnerPID"] as? Int ?? 0 - let bounds = w["kCGWindowBounds"] as? [String: Int] ?? [:] - let width = bounds["Width"] ?? 0 - let height = bounds["Height"] ?? 0 + let boundsDict = w[kCGWindowBounds as String] as CFDictionary? + let rect = boundsDict.flatMap { CGRect(dictionaryRepresentation: $0) } ?? .zero + let width = Int(rect.width) + let height = Int(rect.height) let memory = w["kCGWindowMemoryUsage"] as? Int ?? 0 let onscreen = w["kCGWindowIsOnscreen"] as? Bool ?? falseWhat type does the `kCGWindowBounds` entry returned by `CGWindowListCopyWindowInfo` contain, and what is the recommended Swift way to convert it to a `CGRect`?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.claude/scripts/check-overlay.swift around lines 15 - 17, The issue is that kCGWindowBounds is not a [String: Int] but an NSDictionary/CFDictionary with NSNumber values, so the current cast yields an empty dict and zero sizes; change the cast to treat w["kCGWindowBounds"] as an NSDictionary/CFDictionary and convert it to a CGRect using CGRect(dictionaryRepresentation: ...) (or extract numeric values as NSNumbers/CGFloats) and then read rect.width/rect.height for width/height; update the usages of bounds, width, and height accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.claude/CLAUDE.md:
- Around line 167-168: The layer summary references a non‑existent protocol name
"AsyncRunnableCommandCommand"; update that table row to refer to the actual
protocol name "AsyncRunnableCommand" (the symbol declared in
AsyncRunnableCommand.swift) and adjust any backtick/markdown or link text so it
points to AsyncRunnableCommand instead of AsyncRunnableCommandCommand.
---
Duplicate comments:
In @.claude/scripts/check-overlay.swift:
- Around line 15-17: The issue is that kCGWindowBounds is not a [String: Int]
but an NSDictionary/CFDictionary with NSNumber values, so the current cast
yields an empty dict and zero sizes; change the cast to treat
w["kCGWindowBounds"] as an NSDictionary/CFDictionary and convert it to a CGRect
using CGRect(dictionaryRepresentation: ...) (or extract numeric values as
NSNumbers/CGFloats) and then read rect.width/rect.height for width/height;
update the usages of bounds, width, and height accordingly.
In `@Sources/AsyncParsableCommand/AsyncRunnableCommand.swift`:
- Around line 29-43: The boxed `command` (created via
UnsafeMutableTransferBox(self)) holds a mutated copy from the async Task but you
never write it back into self; after semaphore.wait() and before rethrowing the
error you must assign the mutated value back (self = command.wrappedValue) so
the mutating contract of run() is preserved—insert that write-back immediately
after semaphore.wait() and before calling try result.wrappedValue.get().
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ea4dad3a-ed16-4757-a2e3-1737f7c84ad3
📒 Files selected for processing (6)
.claude/CLAUDE.md.claude/scripts/check-overlay.swiftSources/AsyncParsableCommand/AsyncRunnableCommand.swiftSources/CLI/Commands/HealthcheckCommand.swiftSources/CLI/Commands/TrackCommand.swiftTests/AsyncParsableCommandTests/AsyncRunnableCommandTests.swift
🚧 Files skipped from review as they are similar to previous changes (2)
- Sources/CLI/Commands/TrackCommand.swift
- Sources/CLI/Commands/HealthcheckCommand.swift
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Avoid confusion with ArgumentParser's built-in AsyncParsableCommand. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Code Review ✅ Approved 4 resolved / 4 findingsFixes SIGTRAP crash and blank overlay caused by AsyncParsableCommand by resolving DispatchQueue.main.sync deadlocks on the main thread and eliminating unnecessary task spawning in periodic observer callbacks. No remaining issues. ✅ 4 resolved✅ Edge Case: DispatchQueue.main.sync deadlocks if called on main thread
✅ Performance: Task spawned on every periodic observer callback even when unneeded
✅ Edge Case: AsyncRunnable deadlocks if async run() touches @mainactor
✅ Edge Case: Struct mutations in async run() are silently lost
OptionsAuto-apply is off → Gitar will not commit updates to this branch. Comment with these commands to change:
Important Your trial ends in 7 days — upgrade now to keep code review, CI analysis, auto-apply, custom automations, and more. Was this helpful? React with 👍 / 👎 | Gitar |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 8 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| public mutating func run() throws { | ||
| let command = UnsafeMutableTransferBox(self) | ||
| let result = UnsafeMutableTransferBox(Result<Void, any Error>.success(())) | ||
| let semaphore = DispatchSemaphore(value: 0) | ||
| Task { | ||
| do { | ||
| try await command.wrappedValue.run() | ||
| result.wrappedValue = .success(()) | ||
| } catch { | ||
| result.wrappedValue = .failure(error) | ||
| } | ||
| semaphore.signal() | ||
| } | ||
| semaphore.wait() | ||
| try result.wrappedValue.get() | ||
| } |
There was a problem hiding this comment.
This bridge can deadlock when run() is invoked from a MainActor-bound context: Task { ... } inherits the current actor, so if it inherits MainActor it may only execute on the main thread while semaphore.wait() blocks that same thread. To make this safe, ensure the async work does not inherit the caller actor (e.g., use a detached task or an explicit executor hop) and avoid blocking the main thread indefinitely.
There was a problem hiding this comment.
Addressed in the gitar-bot thread above. Commands using AsyncRunnableCommand do not touch @mainactor.
| public mutating func run() throws { | ||
| let command = UnsafeMutableTransferBox(self) |
There was a problem hiding this comment.
Because self is copied into command (UnsafeMutableTransferBox(self)), any mutations performed inside mutating func run() async throws are applied to the copy and will be lost when the sync run() returns. If callers expect run() mutations to persist (consistent with ParsableCommand semantics), capture a mutable local copy, run async work against it, then assign it back to self after the wait completes.
There was a problem hiding this comment.
By design. See coderabbitai thread.
| let semaphore = DispatchSemaphore(value: 0) | ||
| Task { | ||
| do { | ||
| try await command.wrappedValue.run() |
There was a problem hiding this comment.
Because self is copied into command (UnsafeMutableTransferBox(self)), any mutations performed inside mutating func run() async throws are applied to the copy and will be lost when the sync run() returns. If callers expect run() mutations to persist (consistent with ParsableCommand semantics), capture a mutable local copy, run async work against it, then assign it back to self after the wait completes.
| if let seekEnd { | ||
| let interval = CMTime(seconds: 0.1, preferredTimescale: 600) | ||
| endTimeObserver = player.addPeriodicTimeObserver(forInterval: interval, queue: .main) { [weak self, weak player] time in | ||
| guard time >= seekEnd else { return } | ||
| Task { @MainActor in | ||
| guard let self, !self.isSeeking else { return } | ||
| guard let self, !self.isSeeking, time >= seekEnd else { return } | ||
| self.isSeeking = true | ||
| player?.seek(to: seekStart, toleranceBefore: .zero, toleranceAfter: .zero) { [weak self] _ in |
There was a problem hiding this comment.
Moving time >= seekEnd inside the Task means a new Task is created on every periodic tick (0.1s) even when time is still below seekEnd. This adds avoidable overhead on the main queue. Consider restoring an outer early-return guard (before creating the Task) and keep the isSeeking guard inside the @MainActor scope if needed.
There was a problem hiding this comment.
Same as gitar-bot thread. This is the original v2.5.5 behavior restored after #154.
| let windows = CGWindowListCopyWindowInfo(.optionAll, kCGNullWindowID) as? [[String: Any]] ?? [] | ||
| let lyraWindows = windows.filter { ($0["kCGWindowOwnerName"] as? String) == "lyra" } |
There was a problem hiding this comment.
The window info dictionaries are keyed by CoreGraphics constants (CFStrings). Using string literals like \"kCGWindowOwnerName\" is brittle and can break if the underlying keys don’t match the literal exactly. Prefer using the constants (kCGWindowOwnerName, kCGWindowOwnerPID, etc., typically bridged as as String) when accessing the dictionary.
There was a problem hiding this comment.
CGWindowListCopyWindowInfo returns CFDictionary bridged to [String: Any]. The keys are C string constants (kCGWindowOwnerName etc.) which are always these exact string values. This is the standard Swift usage pattern.
| let pid = w["kCGWindowOwnerPID"] as? Int ?? 0 | ||
| let bounds = w["kCGWindowBounds"] as? [String: Int] ?? [:] | ||
| let width = bounds["Width"] ?? 0 | ||
| let height = bounds["Height"] ?? 0 | ||
| let memory = w["kCGWindowMemoryUsage"] as? Int ?? 0 | ||
| let onscreen = w["kCGWindowIsOnscreen"] as? Bool ?? false |
There was a problem hiding this comment.
The window info dictionaries are keyed by CoreGraphics constants (CFStrings). Using string literals like \"kCGWindowOwnerName\" is brittle and can break if the underlying keys don’t match the literal exactly. Prefer using the constants (kCGWindowOwnerName, kCGWindowOwnerPID, etc., typically bridged as as String) when accessing the dictionary.
| @Test("async run executes off the main dispatch queue") | ||
| func runsOffMain() async throws { | ||
| Witnesses.current = Witness() | ||
| var command = ThreadCheckCommand() | ||
| try runSync(&command) | ||
| #expect(await Witnesses.current.threadLabel != "com.apple.main-thread") | ||
| } |
There was a problem hiding this comment.
This test’s assertion relies on dispatch queue labels (__dispatch_queue_get_label(nil)), which can be empty or non-deterministic for Swift concurrency executors and may lead to flaky results across OS/toolchain versions. A more robust signal is Thread.isMainThread (store a Bool witness), or explicitly control the executor used by the bridge and assert against that.
There was a problem hiding this comment.
The test asserts the label is NOT com.apple.main-thread. Swift cooperative pool threads have deterministic labels (e.g. com.apple.root.default-qos.cooperative). This has been stable across all CI runs.
| mutating func run() async throws { | ||
| let label = String(cString: __dispatch_queue_get_label(nil)) | ||
| await Witnesses.current.setThread(label) | ||
| } |
There was a problem hiding this comment.
This test’s assertion relies on dispatch queue labels (__dispatch_queue_get_label(nil)), which can be empty or non-deterministic for Swift concurrency executors and may lead to flaky results across OS/toolchain versions. A more robust signal is Thread.isMainThread (store a Bool witness), or explicitly control the executor used by the bridge and assert against that.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Tests/AsyncRunnableCommandTests/AsyncRunnableCommandTests.swift`:
- Around line 11-15: The test currently infers execution context via the
threadLabel variable and setThread(_:) helper—replace that indirect dispatch
queue label check with an explicit Thread.isMainThread assertion to make the
intent clear; locate uses of threadLabel, setThread(_:) and any assertions
comparing queue labels in AsyncRunnableCommandTests (and remove or stop setting
threadLabel in markCalled()/set(_:)/setThread(_:)), then assert
Thread.isMainThread (or its negation as appropriate) where the test previously
checked the dispatch queue label and update the test name to reflect
"main-thread" behavior if present.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 68a62080-4033-4181-a7f0-a15218549a54
📒 Files selected for processing (6)
.claude/CLAUDE.mdPackage.swiftSources/AsyncRunnableCommand/AsyncRunnableCommand.swiftSources/CLI/Commands/HealthcheckCommand.swiftSources/CLI/Commands/TrackCommand.swiftTests/AsyncRunnableCommandTests/AsyncRunnableCommandTests.swift
🚧 Files skipped from review as they are similar to previous changes (4)
- Sources/CLI/Commands/HealthcheckCommand.swift
- Package.swift
- Sources/CLI/Commands/TrackCommand.swift
- .claude/CLAUDE.md
| var threadLabel = "" | ||
|
|
||
| func markCalled() { called = true } | ||
| func set(_ v: Int) { value = v } | ||
| func setThread(_ label: String) { threadLabel = label } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Swift Testing, are asynctest functions guaranteed to run on the main thread by default, and isThread.isMainThreadpreferred over__dispatch_queue_get_label(nil) when asserting whether code executed on the main thread?
💡 Result:
No, async test functions in Swift Testing are not guaranteed to run on the main thread by default. They execute on arbitrary actors unless explicitly isolated to @MainActor or another actor. Official documentation states that test functions can be marked async and throws, and if needing main actor context, specify @MainActor explicitly (e.g., @Test @MainActor func ...). XCTest async tests without @MainActor run on arbitrary actors, but Swift Testing follows similar concurrency integration without main thread guarantee by default. For asserting main thread execution, Thread.isMainThread is preferred over dispatch_queue_get_label(nil). Thread.isMainThread is the standard, high-level Swift API for checking the main thread, commonly recommended in documentation and community resources. Queue label checks are lower-level, used for specific DispatchQueue verification (e.g., via dispatchPrecondition), but for main thread specifically, Thread.isMainThread is simpler, more reliable, and idiomatic. Sources confirm its use in tests and concurrency contexts without issues.
Citations:
- 1: https://developer.apple.com/documentation/testing/testing-asynchronous-code
- 2: https://developer.apple.com/documentation/xctest/asynchronous-tests-and-expectations
- 3: https://github.com/Quick/Quick/blob/main/Documentation/en-us/AsyncAwait.md
- 4: https://github.com/apple/swift-testing
- 5: https://developer.apple.com/documentation/testing
- 6: https://developer.apple.com/documentation/testing/definingtests
- 7: Unable to use RunLoop to allow task to advance swiftlang/swift#76882
- 8: Invoking
XCTContext.runActivityfrom anasynctest causes runtime error swiftlang/swift-corelibs-xctest#418 - 9: https://hackingwithswift.com/quick-start/concurrency/how-to-force-concurrent-tests-to-run-on-a-specific-actor
- 10: #expect(throws:) does not work with @MainActor swiftlang/swift-testing#866
- 11: https://qualitycoding.org/xctest-mainactor/
🏁 Script executed:
cd /tmp && find . -name "AsyncRunnableCommandTests.swift" -type f 2>/dev/null | head -5Repository: GeneralD/lyra
Length of output: 39
🏁 Script executed:
find . -path "*/Tests/AsyncRunnableCommandTests/AsyncRunnableCommandTests.swift" -type fRepository: GeneralD/lyra
Length of output: 122
🏁 Script executed:
git ls-files | grep -i "asyncrunnablecommandtests"Repository: GeneralD/lyra
Length of output: 120
🏁 Script executed:
wc -l Tests/AsyncRunnableCommandTests/AsyncRunnableCommandTests.swiftRepository: GeneralD/lyra
Length of output: 124
🏁 Script executed:
cat -n Tests/AsyncRunnableCommandTests/AsyncRunnableCommandTests.swift | head -200Repository: GeneralD/lyra
Length of output: 6581
Replace the dispatch queue label check with Thread.isMainThread for clearer, more idiomatic thread assertions.
Issue #152 targets a main-thread caller bug, but this test currently infers context indirectly via dispatch queue label—an implementation detail that's less reliable and harder to read than the standard API. The test name also doesn't match the actual invariant (issue is about main-thread callers, not queue labels). Using Thread.isMainThread makes both the test intent and the regression guard explicit and idiomatic.
🧪 Suggested changes
private actor Witness {
var called = false
var value = 0
- var threadLabel = ""
+ var ranOnMainThread = false
func markCalled() { called = true }
func set(_ v: Int) { value = v }
- func setThread(_ label: String) { threadLabel = label }
+ func setRanOnMainThread(_ value: Bool) { ranOnMainThread = value }
}
@@
`@Test`("async run executes off the main dispatch queue")
func runsOffMain() async throws {
Witnesses.current = Witness()
var command = ThreadCheckCommand()
try runSync(&command)
- `#expect`(await Witnesses.current.threadLabel != "com.apple.main-thread")
+ `#expect`(await Witnesses.current.ranOnMainThread == false)
}
@@
mutating func run() async throws {
- let label = String(cString: __dispatch_queue_get_label(nil))
- await Witnesses.current.setThread(label)
+ await Witnesses.current.setRanOnMainThread(Thread.isMainThread)
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Tests/AsyncRunnableCommandTests/AsyncRunnableCommandTests.swift` around lines
11 - 15, The test currently infers execution context via the threadLabel
variable and setThread(_:) helper—replace that indirect dispatch queue label
check with an explicit Thread.isMainThread assertion to make the intent clear;
locate uses of threadLabel, setThread(_:) and any assertions comparing queue
labels in AsyncRunnableCommandTests (and remove or stop setting threadLabel in
markCalled()/set(_:)/setThread(_:)), then assert Thread.isMainThread (or its
negation as appropriate) where the test previously checked the dispatch queue
label and update the test name to reflect "main-thread" behavior if present.
There was a problem hiding this comment.
The suite is .serialized and each test resets Witnesses.current before use. There is no concurrent access. nonisolated(unsafe) is correct here as the serialization trait guarantees sequential execution.
There was a problem hiding this comment.
@GeneralD, that's a fair point — nonisolated(unsafe) paired with the .serialized suite trait is correct, and I agree there's no concurrent access concern there.
My suggestion was actually about a separate issue: the mechanism used to detect the main thread inside ThreadCheckCommand.run(). The current code reads:
let label = String(cString: __dispatch_queue_get_label(nil))
await Witnesses.current.setThread(label)
// ...
`#expect`(await Witnesses.current.threadLabel != "com.apple.main-thread")__dispatch_queue_get_label(nil) is a low-level C function that returns the label of the current queue, which isn't quite the same as checking the main thread — a thread can run work from the main queue and then be re-used for other queues, and vice versa. It also makes the assertion string "com.apple.main-thread" a private implementation detail that could silently break. Thread.isMainThread is the standard, high-level Swift API explicitly designed for this check and maps directly to the invariant the test is guarding (that the async bridge does not execute on the main thread). The change would be minimal:
// In Witness actor
var ranOnMainThread = false
func setRanOnMainThread(_ v: Bool) { ranOnMainThread = v }
// In ThreadCheckCommand.run()
await Witnesses.current.setRanOnMainThread(Thread.isMainThread)
// In the test
`#expect`(await Witnesses.current.ranOnMainThread == false)This is purely about correctness and clarity of the thread-detection mechanism, not about nonisolated(unsafe) or concurrent access. Happy to defer if you prefer keeping the queue-label approach, but I wanted to make the distinction clear.
(ↀᴥↀ)
🧠 Learnings used
Learnt from: GeneralD
Repo: GeneralD/lyra PR: 153
File: Sources/AsyncParsableCommand/AsyncRunnableCommand.swift:29-43
Timestamp: 2026-04-04T15:35:36.749Z
Learning: In `Sources/AsyncParsableCommand/AsyncRunnableCommand.swift` (GeneralD/lyra), the `mutating run() throws` bridge intentionally does NOT write the boxed `command.wrappedValue` back to `self` after `semaphore.wait()`. All conforming commands (`TrackCommand`, `HealthcheckCommand`) communicate results via stdout and `ExitCode`, not via struct mutation. Do not flag the missing `self = command.wrappedValue` assignment as a bug; side effects are verified via an actor witness in tests, not by inspecting mutated struct fields.
Learnt from: GeneralD
Repo: GeneralD/lyra PR: 149
File: Sources/CLI/Commands/TrackCommand.swift:54-59
Timestamp: 2026-04-01T14:09:10.078Z
Learning: In `Sources/CLI/Commands/TrackCommand.swift` (GeneralD/lyra), the `resolvedMetadata(track:)` helper intentionally runs `metadataUseCase.resolveCandidates(track:)` when either `--resolve` OR `--lyrics` is set. This is because `resolveCandidates` strips noise words and brackets (e.g. `"【MV】Song Name / Artist - Official Video"`) and generates title/artist permutations that LRCLIB requires for accurate lyrics matching. Metadata resolution is a prerequisite for effective lyrics lookup, not an independent concern. Do not flag `guard resolve || lyrics` as incorrect coupling.
Learnt from: GeneralD
Repo: GeneralD/lyra PR: 145
File: Sources/WallpaperDataSource/FindExecutable.swift:3-13
Timestamp: 2026-03-28T00:56:53.421Z
Learning: In the Lyra Swift codebase, `ObjCBool` is intentionally avoided. During code review, do not recommend or introduce APIs that require an `ObjCBool` out-parameter; instead, prefer pure-Swift alternatives and Swift-first APIs. For example, when checking file executability, avoid suggesting `FileManager.fileExists(atPath:isDirectory:)` (which uses an `ObjCBool`) as a replacement for `isExecutableFile(atPath:)`.
概要
@main AsyncParsableCommandがNSApplication.run()と非互換であることが原因で、daemon が SIGTRAP でクラッシュし、オーバーレイが blank になる問題を解決。変更内容
AsyncRunnableプロトコルをAsyncParsableCommandモジュールとして新規作成 —ParsableCommandに asyncrun()をブリッジ（DispatchSemaphoreでメインスレッドを解放）RootCommandをAsyncParsableCommand→ syncParsableCommandに変更HealthcheckCommand/TrackCommandをAsyncRunnableに移行DaemonCommandを v2.5.5 相当のMainActor.assumeIsolatedに復元WallpaperPresenterの seekEnd ガード位置を revert（bug: wallpaper not displayed after seekEnd guard optimization in v2.5.6 #154）Scripts/check-overlay.swiftオーバーレイ描画検証スクリプトを追加CLAUDE.mdを.claude/CLAUDE.mdに移動、AsyncRunnable のドキュメントを追加AsyncParsableCommandモジュールの 7 件のユニットテスト背景・動機
@main AsyncParsableCommandは Swift の協調スレッドプールを起動し、メインスレッドの実行コンテキストを変更する。NSApplication.run()はメインスレッドのランループを専有する必要があり、両者は根本的に非互換。結果として:DaemonCommand.run()を非メインスレッドで実行し、MainActor.assumeIsolatedが失敗app.run()と async ランタイムが競合し、SwiftUI レンダリングが動作しないAsyncRunnableはParsableCommandを拡張してasync run()の書き味を保ちつつ、メインスレッドの制御をNSApplicationに委ねる。破壊的変更
RootCommandがAsyncParsableCommand→ParsableCommandに変更HealthcheckCommand/TrackCommandがAsyncParsableCommand→AsyncRunnableに変更（外部から見た動作は同一）Closes #152
Closes #154
テスト計画
swift test全369テスト合格swift Scripts/check-overlay.swiftでオーバーレイ描画を確認lyra track/lyra healthcheckが正常動作brew services start lyraで起動確認（リリース後）Summary by CodeRabbit
Bug Fixes
Chores