Skip to content

test(macos): restructure BurnOSX for testability, add BurnRunner seam and CI - #491

Merged
willwashburn merged 4 commits into
mainfrom
claude/zealous-maxwell-2aienr
Jul 3, 2026
Merged

test(macos): restructure BurnOSX for testability, add BurnRunner seam and CI#491
willwashburn merged 4 commits into
mainfrom
claude/zealous-maxwell-2aienr

Conversation

@willwashburn

@willwashburnwillwashburn commented Jul 3, 2026

Copy link
Copy Markdown
Member

Motivation

The macOS menu bar app (apps/macos, BurnOSX) has zero test infrastructure. Recent reports of slowness and memory-leak-shaped behavior need tests to reproduce and guard against regressions, but the package is a single executable target with no seams for injecting fakes and no CI running on macOS. This is the foundation PR: it makes the app testable and adds the CI that will run those tests. Follow-up PRs (lifecycle/leak tests, history-store tests, soak tests) stack on top of this branch.

What changed

Package restructure (apps/macos/Package.swift) — split the single Burn executable target into three:

  • BurnCore (.target, path: Sources/Burn) — all existing app logic + Resources (so Bundle.module keeps resolving).
  • Burn (.executableTarget, depends on BurnCore, path: Sources/Main) — a thin @main entry point only. Executable product name is unchanged, so build.sh/release.sh (which copy $BIN_PATH/Burn and glob *.bundle) keep working with no edits.
  • BurnTests (.testTarget, depends on BurnCore).

The @main struct BurnApp moved to Sources/Main/BurnApp.swift; AppDelegate + MenuBarIcon stayed in BurnCore (old BurnApp.swift renamed to AppDelegate.swift). AppDelegate was made public with a public override init() so the executable's @NSApplicationDelegateAdaptor can adopt it. Nothing else was made public — tests use @testable import BurnCore.

BurnRunner seam (BurnLedger.swift) — introduced protocol BurnRunner: Sendable and moved the Process/Pipe/timeout resolution machinery into actor SystemBurnRunner: BurnRunner. BurnLedger now takes an injectable runner (init(runner: BurnRunner = SystemBurnRunner())) and its cost/summary/timeseries call await runner.run(args). The ingest watch resolves the bundled binary via runner.bundledBinaryURL(). Production behavior is identical.

Injectability for stores/view models:

  • UsageHistoryStore — added init(fileURL:) (default init now delegates to it).
  • UsageViewModelinit(providers:history:ledger:autostart:), all defaulted; autostart: false lets tests skip timers/network.
  • LiveBurnViewModelinit(ledger:).

Smoke tests (Tests/BurnTests/BurnLedgerParsingTests.swift) — a FakeRunner returning canned JSON drives cost/summary/timeseries parsing (token-field summation, fractional + plain ISO8601 timestamps, nil-on-garbage) and asserts the emitted burn summary --provider … --since … --json arg shape.

CI (.github/workflows/macos-app-tests.yml) — pull_request (paths-filtered to apps/macos/** + the workflow) and workflow_dispatch, runs swift build + swift test on macos-14. The Swift package imports AppKit/SwiftUI so it can't build on the Linux ci.yml job.

Note: written without a local Swift toolchain (Linux container, AppKit imports) — CI is the first real compile.

Follow-ups

Three PRs will stack on this branch: lifecycle/leak tests, history-store tests, and soak tests. The public/internal seams above are the shape they build on.


Generated by Claude Code

Review in cubic

Split the single Burn executable target into a BurnCore library (all app
logic, resources) + a thin Burn executable (@main only) + a BurnTests
test target, so the app logic can be unit-tested via `@testable import
BurnCore`.
Introduce a BurnRunner seam in BurnLedger: the Process/Pipe/timeout
machinery moves into SystemBurnRunner (the production runner), and
BurnLedger takes an injectable runner so tests can feed canned JSON.
Behavior in production is unchanged.
Add injectability to the stores/view models (UsageHistoryStore file URL,
UsageViewModel providers/history/ledger/autostart, LiveBurnViewModel
ledger) and smoke tests covering cost/summary/timeseries parsing and the
emitted `burn summary` args.
Add a macos-app-tests workflow that builds and tests apps/macos on a
macOS runner (the Swift package needs AppKit/SwiftUI and can't build on
the Linux CI).
@coderabbitai

coderabbitaiBot commented Jul 3, 2026

Copy link
Copy Markdown

Review Change Stack

Warning

Review limit reached

@willwashburn, you've reached your PR review limit, so we couldn't start this review.

Next review available in:6 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 4a90bd17-57df-460b-ae21-e91a14ce6730

📥 Commits

Reviewing files that changed from the base of the PR and between 694535c and 900b02c.

📒 Files selected for processing (4)
  • .github/workflows/macos-app-tests.yml
  • apps/macos/Sources/Burn/BurnLedger.swift
  • apps/macos/Sources/Burn/UsageViewModel.swift
  • apps/macos/Tests/BurnTests/BurnLedgerParsingTests.swift
📝 Walkthrough

Walkthrough

The macOS Swift package is split into BurnCore library, Burn executable, and BurnTests targets. BurnLedger's process execution is extracted into an injectable BurnRunner/SystemBurnRunner abstraction. View models and UsageHistoryStore gain dependency injection. A new app entry point and CI workflow are added, plus parsing unit tests.

Changes

macOS app refactor and testing

Layer / File(s)Summary
Package target split
apps/macos/Package.swift
Splits the package into a BurnCore library target, a Burn executable target under Sources/Main, and a new BurnTests test target.
AppDelegate export and new entry point
apps/macos/Sources/Burn/AppDelegate.swift, apps/macos/Sources/Main/BurnApp.swift
Removes the SwiftUI entry point from AppDelegate.swift, makes AppDelegate public, and adds a new `@main` BurnApp using `Settings { EmptyView() }` with `NSApplicationDelegateAdaptor`.
BurnRunner abstraction
apps/macos/Sources/Burn/BurnLedger.swift
Introduces `BurnRunner` protocol and `SystemBurnRunner` actor for tool resolution and subprocess execution; `cost`, `summary`, `timeseries`, and `startIngestWatch` now use an injected runner.
Dependency injection wiring
apps/macos/Sources/Burn/LiveBurnViewModel.swift, apps/macos/Sources/Burn/UsageViewModel.swift, apps/macos/Sources/Burn/UsageHistory.swift
LiveBurnViewModel and UsageViewModel accept injected ledger/history/providers instead of always using shared singletons; UsageHistoryStore adds `init(fileURL:)` delegated from a convenience initializer.
Parsing tests and CI workflow
apps/macos/Tests/BurnTests/BurnLedgerParsingTests.swift, .github/workflows/macos-app-tests.yml
Adds a FakeRunner test double and tests for cost/summary/timeseries parsing, plus a CI workflow that builds and tests the Swift package on macos-14.

Estimated code review effort: 4 (Complex) | ~50 minutes

Sequence Diagram(s)

sequenceDiagram
participant LiveBurnViewModel
participant BurnLedger
participant SystemBurnRunner
participant CLI as burn CLI
LiveBurnViewModel->>BurnLedger: timeseries(provider, since, bucket)
BurnLedger->>SystemBurnRunner: run(args)
SystemBurnRunner->>SystemBurnRunner: resolve bundled or path tool
SystemBurnRunner->>CLI: execute subprocess
CLI-->>SystemBurnRunner: stdout
SystemBurnRunner-->>BurnLedger: stdout or nil
BurnLedger-->>LiveBurnViewModel: parsed result or nil
Loading

Possibly related PRs

  • AgentWorkforce/burn#478: Builds on the ledger-backed BurnLedger spend functionality that this PR's BurnRunner refactor further modifies.
  • AgentWorkforce/burn#481: Overlaps with this PR's move of BurnLedger's subprocess capture/timeout logic into SystemBurnRunner.

Poem

A rabbit split the burrow in two,
BurnCore below, and Burn peeks through.
A runner hops to fetch the cost,
No shared singleton state is lost.
Tests confirm each timestamp true —
🐇 hop, build, and test anew!

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check nameStatusExplanation
Title check✅ PassedThe title accurately summarizes the main macOS testability refactor and CI addition.
Description check✅ PassedThe description is clearly related to the changeset and matches the restructuring, seams, tests, and macOS CI work.
Docstring Coverage✅ PassedNo functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check✅ PassedCheck skipped because no linked issues were found for this pull request.
Out of Scope Changes check✅ PassedCheck skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch claude/zealous-maxwell-2aienr

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@gemini-code-assistgemini-code-assistBot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request refactors the macOS app structure by extracting the core logic into a "BurnCore" library target and introducing a "BurnRunner" protocol to decouple CLI execution from "BurnLedger". This enables dependency injection and allows unit testing of ledger parsing with a fake runner. Feedback highlights two critical issues: first, the "SystemBurnRunner" actor uses synchronous blocking calls ("DispatchGroup.wait" and "usleep") that can block the cooperative thread pool; second, an actor reentrancy race condition in "startIngestWatch()" could allow multiple background watch processes to be spawned concurrently during an asynchronous suspension point.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment on lines +34 to +115
func run(_ args: [String]) async -> String? {
switch resolveTool() {
case .bundled(let url):
// Self-contained Rust binary — exec directly, no shell needed.
return capture { $0.executableURL = url; $0.arguments = args }
case .path:
// Run through a login shell so nvm/Homebrew PATH (and the `node` the
// npm `burn` shim needs) resolve even when launched from Finder.
let command = "burn " + args.map(shellQuote).joined(separator: " ")
return loginShell(command)
case .missing, .unknown:
return nil
}
}

func bundledBinaryURL() async -> URL? {
if case .bundled(let url) = resolveTool() { return url }
return nil
}

private func resolveTool() -> Tool {
if case .unknown = tool {
if let url = Bundle.main.url(forAuxiliaryExecutable: "burn"),
FileManager.default.isExecutableFile(atPath: url.path) {
tool = .bundled(url)
} else if !(loginShell("command -v burn")?
.trimmingCharacters(in: .whitespacesAndNewlines) ?? "").isEmpty {
tool = .path
} else {
tool = .missing
}
}
return tool
}

private func loginShell(_ command: String) -> String? {
capture {
$0.executableURL = URL(fileURLWithPath: "/bin/zsh")
$0.arguments = ["-lc", command]
}
}

/// Runs a configured process and returns stdout, or `nil` on failure /
/// nonzero exit / timeout. The timeout stops a hung `burn` from wedging the
/// actor and queuing follow-up spend requests behind it.
private func capture(_ configure: (Process) -> Void, timeout: TimeInterval = 30) -> String? {
let process = Process()
configure(process)
let stdout = Pipe()
process.standardOutput = stdout
process.standardError = Pipe()
do {
try process.run()
} catch {
return nil
}
// Read stdout to EOF (which arrives when the process exits) and reap it
// on a background queue; bound the wait with a timeout. This blocks the
// runner actor until the process truly finishes or is killed — which,
// because the actor serializes calls, guarantees only one `burn`
// subprocess can ever be alive at a time (no pile-up). Avoids the
// `terminationHandler` race that could let capture() return while the
// child kept running.
let group = DispatchGroup()
group.enter()
var output = Data()
DispatchQueue.global(qos: .utility).async {
output = stdout.fileHandleForReading.readDataToEndOfFile()
process.waitUntilExit()
group.leave()
}
if group.wait(timeout: .now() + timeout) == .timedOut {
process.terminate() // SIGTERM…
usleep(200_000)
if process.isRunning { // …then SIGKILL if it ignores it
kill(process.processIdentifier, SIGKILL)
}
return nil
}
guard process.terminationStatus == 0 else { return nil }
return String(data: output, encoding: .utf8)
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

The capture method in SystemBurnRunner uses DispatchGroup.wait(timeout:) and usleep, which are synchronous blocking calls. Since SystemBurnRunner is an actor, executing these blocking calls directly on the cooperative thread pool can lead to thread starvation or deadlocks, especially under heavy load or timeouts.

We should refactor this to be fully asynchronous by running the blocking read/wait on a background queue and using withCheckedContinuation along with a non-blocking Task.sleep to implement the timeout.

func run(_ args:[String])async->String?{switchawaitresolveTool(){case.bundled(let url):
// Self-contained Rust binary — exec directly, no shell needed.
returnawaitcapture{ $0.executableURL = url; $0.arguments = args }case.path:
// Run through a login shell so nvm/Homebrew PATH (and the `node` the
// npm `burn` shim needs) resolve even when launched from Finder.
letcommand="burn "+ args.map(shellQuote).joined(separator:"")returnawaitloginShell(command)case.missing,.unknown:returnnil}}func bundledBinaryURL()async->URL?{if case .bundled(let url)=awaitresolveTool(){return url }returnnil}privatefunc resolveTool()async->Tool{if case .unknown = tool {iflet url =Bundle.main.url(forAuxiliaryExecutable:"burn"),FileManager.default.isExecutableFile(atPath: url.path){
tool =.bundled(url)}elseif !(awaitloginShell("command -v burn")?.trimmingCharacters(in:.whitespacesAndNewlines)??"").isEmpty {
tool =.path
}else{
tool =.missing
}}return tool
}privatefunc loginShell(_ command:String)async->String?{awaitcapture{
$0.executableURL =URL(fileURLWithPath:"/bin/zsh")
$0.arguments =["-lc", command]}}
/// Runs a configured process and returns stdout, or `nil` on failure /
/// nonzero exit / timeout. The timeout stops a hung `burn` from wedging the
/// actor and queuing follow-up spend requests behind it.
privatefunc capture(_ configure:(Process)->Void, timeout:TimeInterval=30)async->String?{letprocess=Process()configure(process)letstdout=Pipe()
process.standardOutput = stdout
process.standardError =Pipe()do{try process.run()}catch{returnnil}
// Read stdout to EOF and wait for process exit on a background queue
// without blocking the cooperative thread pool.
letdataTask=Task{awaitwithCheckedContinuation{ continuation inDispatchQueue.global(qos:.utility).async{letdata= stdout.fileHandleForReading.readDataToEndOfFile()
process.waitUntilExit()
continuation.resume(returning: data)}}}
// Race the data reading task against the timeout
letresult=awaitwithTaskGroup(of:Data?.self){ group in
group.addTask{await dataTask.value
}
group.addTask{try?awaitTask.sleep(nanoseconds:UInt64(timeout *1_000_000_000))returnnil}letfirst=await group.next()??nil
group.cancelAll()return first
}if result ==nil{
process.terminate() // SIGTERM…
try?awaitTask.sleep(nanoseconds:200_000_000)if process.isRunning { // …then SIGKILL if it ignores it
kill(process.processIdentifier, SIGKILL)}returnnil}guard process.terminationStatus ==0,let data = result else{returnnil}returnString(data: data, encoding:.utf8)}

Comment on lines +240 to +242
func startIngestWatch() async {
guard watchProcess == nil else { return }
guard case .bundled(let url) = resolveTool() else { return }
guard let url = await runner.bundledBinaryURL() else { return }

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

Because startIngestWatch() is an asynchronous function on an actor, it suspends at await runner.bundledBinaryURL(). During this suspension, another call to startIngestWatch() can enter the actor and also find watchProcess == nil (since the first call hasn't set it yet). This leads to multiple background watch processes being spawned concurrently, leaking processes and wasting resources. To prevent this, re-verify that watchProcess == nil immediately after the suspension point.

Suggested change
func startIngestWatch()async{
guard watchProcess ==nilelse{return}
guardcase .bundled(let url)=resolveTool()else{return}
guardlet url =await runner.bundledBinaryURL()else{return}
func startIngestWatch()async{
guard watchProcess ==nilelse{return}
guardlet url =await runner.bundledBinaryURL()else{return}
guardwatchProcess ==nilelse{return}

@chatgpt-codex-connectorchatgpt-codex-connectorBot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit:694535ce06

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines 241 to +242
guard watchProcess == nil else { return }
guard case .bundled(let url) = resolveTool() else { return }
guard let url = await runner.bundledBinaryURL() else { return }

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Recheck watch state after awaiting the runner

When the Live tab is closed quickly, LiveBurnView.onDisappear calls stopIngestWatch() while this method can be suspended at await runner.bundledBinaryURL(). Because actor methods are reentrant across awaits, stopIngestWatch() can observe no process and return, after which this method resumes and starts burn ingest --watch anyway, leaving the background watcher running while the live view is stopped. Recheck/cancel the start after the await (or track desired running state) before spawning the process.

Useful? React with 👍 / 👎.

… state after await
swift test failed on macOS CI: XCTUnwrap takes an autoclosure, which cannot
contain await. Evaluate the async call first, then unwrap.
Also close the actor-reentrancy race in startIngestWatch: a stopIngestWatch
arriving while start is suspended resolving the bundled binary now cancels
the spawn instead of being silently undone.

@coderabbitaicoderabbitaiBot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🧹 Nitpick comments (1)
apps/macos/Sources/Burn/BurnLedger.swift (1)

79-115: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

stderr pipe is never drained.

process.standardError = Pipe() is set but nothing reads it. If burn writes enough to stderr to fill the OS pipe buffer, the child blocks on write() and capture() only recovers via the 30s timeout instead of returning promptly (and any stderr diagnostics are silently discarded).

♻️ Drain stderr alongside stdout
 let stdout = Pipe()
process.standardOutput = stdout
- process.standardError = Pipe()+ let stderr = Pipe()+ process.standardError = stderr
do {
try process.run()
} catch {
return nil
}
...
var output = Data()
DispatchQueue.global(qos: .utility).async {
output = stdout.fileHandleForReading.readDataToEndOfFile()
+ _ = stderr.fileHandleForReading.readDataToEndOfFile()
process.waitUntilExit()
group.leave()
}
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/macos/Sources/Burn/BurnLedger.swift` around lines 79 - 115, The
`capture(_ configure:timeout:)` helper sets `process.standardError` to a pipe
but never reads from it, which can block `burn` and discard diagnostics. Update
`capture` to drain stderr alongside stdout, using a second reader/async read on
the stderr pipe while the process runs. Keep the timeout/termination behavior
intact, and return or otherwise preserve stderr only if needed for debugging.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/macos-app-tests.yml:
- Around line 26-27: The Checkout step in the macOS app test workflow is
persisting credentials unnecessarily. Update the actions/checkout usage in the
workflow to explicitly disable token persistence by setting persist-credentials
to false, since this job only builds and tests and does not need git push
access. Use the Checkout step as the anchor point for the change.
In `@apps/macos/Tests/BurnTests/BurnLedgerParsingTests.swift`:
- Line 37: The test in BurnLedgerParsingTests is using await directly inside
XCTUnwrap’s autoclosure, which Swift 6 rejects. Update each affected call site
in the ledger parsing tests (the cost/provider checks in the same test methods)
by awaiting the async result first, storing it in a local variable, and then
passing that variable into XCTUnwrap. Apply the same pattern to the other
flagged XCTUnwrap usages in the test file so all await calls are outside the
autoclosure.
- Around line 7-28: `FakeRunner.run(_:)` is calling `NSLock.lock()`/`unlock()`
directly inside an async method, which breaks under Swift 6 concurrency. Move
the mutation of `recorded` into a synchronous helper on `FakeRunner` and have
`run(_:)` call that helper before returning, so the lock is only used in
non-async context. Keep `capturedArgs` protected by the same `lock`/`recorded`
access pattern.
---
Nitpick comments:
In `@apps/macos/Sources/Burn/BurnLedger.swift`:
- Around line 79-115: The `capture(_ configure:timeout:)` helper sets
`process.standardError` to a pipe but never reads from it, which can block
`burn` and discard diagnostics. Update `capture` to drain stderr alongside
stdout, using a second reader/async read on the stderr pipe while the process
runs. Keep the timeout/termination behavior intact, and return or otherwise
preserve stderr only if needed for debugging.
🪄 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: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: de3562f2-0aa6-4e18-874c-8054299b2cd5

📥 Commits

Reviewing files that changed from the base of the PR and between 4c3da37 and 694535c.

📒 Files selected for processing (9)
  • .github/workflows/macos-app-tests.yml
  • apps/macos/Package.swift
  • apps/macos/Sources/Burn/AppDelegate.swift
  • apps/macos/Sources/Burn/BurnLedger.swift
  • apps/macos/Sources/Burn/LiveBurnViewModel.swift
  • apps/macos/Sources/Burn/UsageHistory.swift
  • apps/macos/Sources/Burn/UsageViewModel.swift
  • apps/macos/Sources/Main/BurnApp.swift
  • apps/macos/Tests/BurnTests/BurnLedgerParsingTests.swift

Comment thread.github/workflows/macos-app-tests.yml
Comment threadapps/macos/Tests/BurnTests/BurnLedgerParsingTests.swift
Comment threadapps/macos/Tests/BurnTests/BurnLedgerParsingTests.swift Outdated

@cubic-dev-aicubic-dev-aiBot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

5 issues found and verified against the latest diff

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/macos/Sources/Burn/BurnLedger.swift">
<violation number="1" location="apps/macos/Sources/Burn/BurnLedger.swift:105">
P2: `DispatchGroup.wait(timeout:)` and `usleep` are synchronous blocking calls executed inside an actor method, which runs on Swift's cooperative thread pool. Blocking a cooperative thread can cause thread starvation for other concurrent tasks. Consider offloading the blocking wait to a detached queue and bridging back with `withCheckedContinuation`, or using async-native alternatives like `Task.sleep` for the timeout delay.</violation>
<violation number="2" location="apps/macos/Sources/Burn/BurnLedger.swift:242">
P1: Actor reentrancy issue: `watchProcess == nil` is checked *before* the `await runner.bundledBinaryURL()` suspension point, but isn't rechecked after it. During the suspension, another call can enter this actor method (or `stopIngestWatch()` can run), so the guard is stale upon resumption. This can lead to multiple watch processes being spawned or a process being started after `stop` was requested. Re-verify `watchProcess == nil` after the `await` before spawning the process.</violation>
</file>
<file name=".github/workflows/macos-app-tests.yml">
<violation number="1" location=".github/workflows/macos-app-tests.yml:29">
P3: Missing explicit Xcode version selection. The macos-14 runner's default Xcode changes when GitHub updates the runner image (e.g., semiannual macOS/Xcode releases), which can break the Swift build silently. Add an xcode-select step or XCODE_VERSION env var tied to the project's minimum Xcode/Swift version, so CI stays reproducible across runner image refreshes.</violation>
</file>

Reply with feedback, questions, or to request a fix.

Re-trigger cubic

guard watchProcess == nil else { return }
guard case .bundled(let url) = resolveTool() else { return }
watchDesired = true
guard let url = await runner.bundledBinaryURL() else { return }

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1: Actor reentrancy issue: watchProcess == nil is checked before the await runner.bundledBinaryURL() suspension point, but isn't rechecked after it. During the suspension, another call can enter this actor method (or stopIngestWatch() can run), so the guard is stale upon resumption. This can lead to multiple watch processes being spawned or a process being started after stop was requested. Re-verify watchProcess == nil after the await before spawning the process.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/macos/Sources/Burn/BurnLedger.swift, line 242:
<comment>Actor reentrancy issue: `watchProcess == nil` is checked *before* the `await runner.bundledBinaryURL()` suspension point, but isn't rechecked after it. During the suspension, another call can enter this actor method (or `stopIngestWatch()` can run), so the guard is stale upon resumption. This can lead to multiple watch processes being spawned or a process being started after `stop` was requested. Re-verify `watchProcess == nil` after the `await` before spawning the process.</comment>
<file context>
@@ -123,9 +237,9 @@ actor BurnLedger {
+ func startIngestWatch() async {
guard watchProcess == nil else { return }
- guard case .bundled(let url) = resolveTool() else { return }
+ guard let url = await runner.bundledBinaryURL() else { return }
let process = Process()
process.executableURL = url
</file context>
Suggested change
guardlet url =await runner.bundledBinaryURL()else{return}
guardlet url =await runner.bundledBinaryURL()else{return}
guard watchProcess ==nilelse{return} // re-check after suspension

Comment threadapps/macos/Sources/Burn/UsageViewModel.swift Outdated
process.waitUntilExit()
group.leave()
}
if group.wait(timeout: .now() + timeout) == .timedOut {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2: DispatchGroup.wait(timeout:) and usleep are synchronous blocking calls executed inside an actor method, which runs on Swift's cooperative thread pool. Blocking a cooperative thread can cause thread starvation for other concurrent tasks. Consider offloading the blocking wait to a detached queue and bridging back with withCheckedContinuation, or using async-native alternatives like Task.sleep for the timeout delay.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/macos/Sources/Burn/BurnLedger.swift, line 105:
<comment>`DispatchGroup.wait(timeout:)` and `usleep` are synchronous blocking calls executed inside an actor method, which runs on Swift's cooperative thread pool. Blocking a cooperative thread can cause thread starvation for other concurrent tasks. Consider offloading the blocking wait to a detached queue and bridging back with `withCheckedContinuation`, or using async-native alternatives like `Task.sleep` for the timeout delay.</comment>
<file context>
@@ -1,16 +1,138 @@
+ process.waitUntilExit()
+ group.leave()
+ }
+ if group.wait(timeout: .now() + timeout) == .timedOut {
+ process.terminate() // SIGTERM…
+ usleep(200_000)
</file context>

- name: Checkout
uses: actions/checkout@v6

- name: Build

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P3: Missing explicit Xcode version selection. The macos-14 runner's default Xcode changes when GitHub updates the runner image (e.g., semiannual macOS/Xcode releases), which can break the Swift build silently. Add an xcode-select step or XCODE_VERSION env var tied to the project's minimum Xcode/Swift version, so CI stays reproducible across runner image refreshes.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At .github/workflows/macos-app-tests.yml, line 29:
<comment>Missing explicit Xcode version selection. The macos-14 runner's default Xcode changes when GitHub updates the runner image (e.g., semiannual macOS/Xcode releases), which can break the Swift build silently. Add an xcode-select step or XCODE_VERSION env var tied to the project's minimum Xcode/Swift version, so CI stays reproducible across runner image refreshes.</comment>
<file context>
@@ -0,0 +1,33 @@
+ - name: Checkout
+ uses: actions/checkout@v6
+
+ - name: Build
+ run: swift build --package-path apps/macos
+
</file context>

Comment thread.github/workflows/macos-app-tests.yml
- Drain the subprocess stderr pipe so a chatty child can't fill the 64KB
buffer and stall until the timeout; box stdout data instead of mutating
a captured var (Swift 6 error).
- Scope FakeRunner's NSLock use inside a synchronous helper (lock/unlock
are noasync).
- Normalize UsageViewModel's initial selection to an available provider
when an injected provider map lacks the saved one.
- Set persist-credentials: false on the test workflow checkout.
@willwashburnClaude

Copy link
Copy Markdown
MemberAuthor

Review findings triage (Codex, CodeRabbit, cubic):

Fixed in 6c0fdce

  • await inside XCTUnwrap's autoclosure (build breaker) — awaits hoisted to locals at all five sites.
  • startIngestWatch actor-reentrancy race (Codex P2 / cubic P1) — added a watchDesired flag set by stop and rechecked (along with watchProcess == nil) after the await, so a stop that interleaves with a suspended start wins.

Fixed in 82e1ace

  • stderr pipe never drained in capture() — now drained on a background queue so a chatty child can't fill the 64KB buffer and stall to the timeout; also boxed the stdout buffer instead of mutating a captured var (Swift 6 error).
  • NSLock.lock()/unlock() in FakeRunner's async method (noasync) — scoped into a synchronous withLock helper.
  • Injected provider maps could leave selectedProvider pointing at a missing key, making refresh() a silent no-op — initial selection now normalizes to an available provider.
  • persist-credentials: false on the workflow checkout.

Deliberately not addressed here

  • Blocking DispatchGroup.wait/usleep inside the runner actor (cubic P2): real, but the blocking is bounded by the timeout and the serialization is intentional (guarantees at most one burn subprocess). A stacked PR is about to touch this exact code path (injectable timeout for soak tests); converting to a continuation-based wait belongs there rather than conflicting with it.
  • Pinning the Xcode version (cubic P3): release-macos.yml doesn't pin either; keeping the two macOS workflows consistent. Worth doing for both together if image churn ever bites.

Generated by Claude Code

…init
Swift forbids using self before all stored properties are initialized;
the previous normalization read self.providers and captured self in a
closure before menuBarIcon was set.
@willwashburn
willwashburn merged commit f78046c into mainJul 3, 2026
5 checks passed
@willwashburn
willwashburn deleted the claude/zealous-maxwell-2aienr branch July 3, 2026 15:48
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@willwashburn@claude