Give the PR refresh run-loop test something real to observe

[ejc3]

Problem

testPullRequestRefreshRepositoryDiscoveryDoesNotBlockMainRunLoop used to observe repository discovery by counting calls to a stubbed git remote -v subprocess. That stopped being a real signal when #2797 moved remote discovery to in-process config parsing. The counter stayed at zero, and the remaining assertions could pass without proving that discovery had run.

Final approach

• Add GitRepositoryDiscovering for the two filesystem reads that PullRequestProbeService already performs: resolving GitHub slugs and the checked-out branch.
• Keep GitMetadataService as the only production implementation.
• Widen PullRequestProbeService and PullRequestPollService from the concrete metadata service to that protocol.
• Exercise the real polling path in the test by constructing PullRequestPollService directly with a real TabManager host and a test discovery implementation. There is no TabManager test override and no test-only seam in app source.
• Make the run-loop proof deterministic. Discovery signals that it started and blocks on a gate; the test queues a main-run-loop turn and releases discovery only after both signals arrive. If discovery occupies the main thread, the five-second cleanup deadline opens the gate and records an explicit failure. The deadline is deadlock cleanup, not the success criterion.

The protocol and its requirements have API documentation. No user-facing strings, localization data, package lockfiles, Xcode project wiring, or Swift warning budgets change.

Runtime impact

Production behavior is unchanged. The concrete dependency types become protocol existentials, while the app continues to pass the same GitMetadataService instance through the same polling and probing pipeline.

Verification

• Targeted GitHub Actions run: https://github.com/manaflow-ai/cmux/actions/runs/30895230698
• Latest-main targeted rerun: https://github.com/manaflow-ai/cmux/actions/runs/30896696485
• lint-pbxproj-test-wiring: passed (651 test files checked)
• check-package-resolved-policy.py: passed
• Edited Swift files: parse successfully
• git diff --check: passed
• Swift warning-budget diff: empty

Both targeted Actions runs execute WorkspacePullRequestSidebarTests and pass.

[coderabbitai]

📝 Walkthrough

Walkthrough

A public repository discovery protocol is introduced, adopted by GitMetadataService, and used by pull-request probing and polling. TabManager can inject an alternate discovery implementation, and the related run-loop test now uses a blocking discovery seam.

Changes

Repository discovery injection

Layer / File(s)	Summary
Repository discovery contract Packages/macOS/CmuxGit/Sources/CmuxGit/GitRepositoryDiscovering.swift, Packages/macOS/CmuxGit/Sources/CmuxGit/PullRequestProbeService.swift	Adds asynchronous repository slug and checked-out branch APIs, makes GitMetadataService conform, and updates candidate resolution to accept the protocol.
Pull-request discovery wiring Packages/macOS/CmuxSidebarGit/Sources/CmuxSidebarGit/Service/PullRequestPollService.swift, Sources/TabManager.swift	Updates polling dependencies to use GitRepositoryDiscovering and adds an optional TabManager override for pull-request discovery.
Injected discovery regression test cmuxTests/WorkspacePullRequestSidebarTests.swift	Replaces Git subprocess stubbing with a blocking repository discovery implementation and verifies discovery invocation and main run-loop responsiveness.

Estimated code review effort: 2 (Simple) | ~15 minutes

Suggested reviewers: austinywang, azooz2003-bit

---

Important

Pre-merge checks failed

Please resolve all errors before merging. Addressing warnings is optional.

❌ Failed checks (1 error, 1 warning)

Check name	Status	Explanation	Resolution
Cmux No Test Or Debug Seam In Production Source	❌ Error	Sources/TabManager.swift adds a new pullRequestRepositoryDiscovery init override used only by tests, creating a test-only seam in shipping source.	Remove the production override. If tests need observability, widen internal state and use @testable import, or move any debug-only seam to a dedicated test/debug file.
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (23 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Cmux Swift Actor Isolation	✅ Passed	PASS: The new protocol is plain Sendable/nonisolated, GitMetadataService is already a Sendable struct with nonisolated async reads, and the @MainActor wiring adds no UI/background isolation issue.
Cmux Swift Blocking Runtime	✅ Passed	Production Swift only adds a discovery protocol and DI seam; sleeps/locks/timers appear in test-only scaffolding, not app/runtime code.
Cmux Browser Automation Off-Main	✅ Passed	Diff only changes git-discovery/sidebar test seams; no browser socket routing or WebKit wait paths were touched, so this rule isn’t implicated.
Cmux Expensive Synchronous Load	✅ Passed	Production changes only add an off-main repository-discovery seam; refresh work stays in Task.detached, and no agent-history loader or main-actor load was introduced.
Cmux Cache Substitution Correctness	✅ Passed	PASS: The diff adds a fresh-read seam for PR discovery; it doesn't substitute cached state in any persistence/history/undo/snapshot path, and repo caching remains explicit.
Cmux No Hacky Sleeps	✅ Passed	The diff only touches Swift, and the only new sleep is deterministic test scaffolding; no non-Swift production/runtime delay was introduced.
Cmux Algorithmic Complexity	✅ Passed	Changed production code only adds a protocol seam and per-directory caching; no nested scalable scans, repeated sorts, or batch rescans were introduced.
Cmux Swift Concurrency	✅ Passed	No new legacy async pattern was introduced; the diff adds async protocol seams and a test-only blocking discovery stub, while existing Dispatch/Combine/Task code remains unchanged.
Cmux Swift @Concurrent	✅ Passed	The PR keeps repository discovery on a detached utility task before returning to MainActor, and adds no invalid @concurrent annotations or UI-isolated blocking async calls.
Cmux Swift Package Boundaries	✅ Passed	The reusable discovery/probe logic was placed in CmuxGit/CmuxSidebarGit packages; Sources/TabManager.swift only adds allowed app composition and test seam wiring.
Cmux Swiftpm Lockfiles	✅ Passed	The diff only touches source and test files; no Package.swift, Package.resolved, .gitignore, Xcode, or workflow files were changed, so the SwiftPM lockfile rule isn’t implicated.
Cmux Swift Logging	✅ Passed	No added or modified Swift logging in production code; the PR only changes protocol seams, injections, and tests, with no print/debugPrint/NSLog violations.
Cmux User-Facing Error Privacy	✅ Passed	Only API seams and test/docs comments changed; no user-facing error, alert, command-output, or recovery copy was added or altered.
Cmux Full Internationalization	✅ Passed	Diff only adds API seams and test/comment text; no new user-facing Swift copy or locale files were introduced.
Cmux Swiftui State Layout	✅ Passed	The diff only adds a discovery protocol, broadens service types, and wires a test seam; TabManager’s SwiftUI/ObservableObject code only gets initializer plumbing, with no new state/layout patterns.
Cmux Architecture Rethink	✅ Passed	PASS: The new protocol is a test seam only; production still uses GitMetadataService as the single discovery owner, with no new blocking repair path outside tests.
Cmux Swift Auxiliary Window Close Shortcuts	✅ Passed	The diff only changes Git discovery seams, polling deps, TabManager wiring, and a test; no new/changed standalone window, panel, or WindowGroup code.
Cmux Source Artifacts	✅ Passed	All changed paths are intentional source/test files; no logs, caches, DerivedData, tmp, or other artifact paths were added.
Cmux No Ambient Global State	✅ Passed	PASS: The PR adds an injectable protocol seam and constructor wiring; no new top-level mutable vars or singletons were introduced.
Title check	✅ Passed	The title clearly identifies the main change: improving the pull-request refresh run-loop test with a meaningful observation point.
Description check	✅ Passed	The description clearly explains the problem, implementation, runtime impact, and verification, although it does not use the repository template headings.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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

• X
• Mastodon
• Reddit
• LinkedIn

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

[cursor]

Bugbot is paused — on-demand spend limit reached

Bugbot uses usage-based billing for this team and has hit its on-demand spend limit.

A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue.

[greptile-apps]

Greptile Summary

This PR repairs a test that had silently stopped observing anything real since PR #2797 replaced the git remote -v subprocess with in-process config parsing. Rather than deleting the test, it extracts a two-method GitRepositoryDiscovering protocol from GitMetadataService, threads it through PullRequestProbeService and PullRequestPollService, and adds a nil-default injection point to TabManager.init so the test can supply a counting stub.

• New protocol (GitRepositoryDiscovering): declares repositorySlugs(forDirectory:) and checkedOutBranch(forDirectory:) as nonisolated async requirements; GitMetadataService conforms via a retroactive extension and remains the sole production implementation, so runtime behavior is unchanged.
• Type-widening in PullRequestProbeService and PullRequestPollService: callers already hold GitMetadataService which satisfies the protocol, so no call site breaks.
• Test stub: BlockingRepositoryDiscovery sleeps 30 ms per call to simulate blocking filesystem work, giving the run-loop coarse guard something real to race against; the primary assertion is invocationCounter.value > 0, which is the signal that was previously stuck at zero.

Confidence Score: 5/5

Safe to merge — production behavior is unchanged; the only new runtime path is the nil-default fallback that was already the live code path.

The protocol extraction is a pure abstraction over two methods PullRequestProbeService was already calling on GitMetadataService. Every production call site receives either gitMetadataService directly or the same value through the nil-coalescing fallback, so the shipped binary is functionally identical to main. The test improvement is honest about what it can observe and removes a non-falsifiable assertion rather than keeping it as false assurance. No actor-isolation mistakes, no new ambient global state, no test seams by the rule definitions, and no blocking primitives in production code were introduced.

Files Needing Attention: No files require special attention.

Important Files Changed

Filename	Overview
Packages/macOS/CmuxGit/Sources/CmuxGit/GitRepositoryDiscovering.swift	New public protocol with two async requirements matching the methods PullRequestProbeService already called on GitMetadataService; conformance wired via a retroactive extension, no behavior change.
Packages/macOS/CmuxGit/Sources/CmuxGit/PullRequestProbeService.swift	One parameter widened from GitMetadataService to any GitRepositoryDiscovering; call sites and body are unchanged, production callers already satisfy the protocol.
Packages/macOS/CmuxSidebarGit/Sources/CmuxSidebarGit/Service/PullRequestPollService.swift	Storage and init parameter widened to protocol type; gitMetadataService usage inside the class is unchanged, SidebarGitMetadataService continues to receive the concrete GitMetadataService directly.
Sources/TabManager.swift	Adds optional pullRequestRepositoryDiscovery parameter (defaults to nil, falls back to gitMetadataService) after the existing gitMetadataService injection point; follows the established injection pattern for all other services in this initializer.
cmuxTests/WorkspacePullRequestSidebarTests.swift	Replaces the dead command-runner counter with a protocol-conforming stub; removes MainThreadObservationBox (non-falsifiable per SE-0338) and tightens comments to be honest about what the coarse run-loop guard can and cannot catch.

Sequence Diagram

sequenceDiagram
    participant T as TabManager
    participant PPS as PullRequestPollService
    participant PRS as PullRequestProbeService
    participant GRD as GitRepositoryDiscovering
    participant GMS as GitMetadataService
    participant STUB as BlockingRepositoryDiscovery

    T->>PPS: init with discovery or gitMetadataService
    note over T,PPS: production uses GitMetadataService, test injects stub
    PPS->>PRS: resolveCandidateSeeds
    PRS->>GRD: repositorySlugs forDirectory
    GRD-->>GMS: production reads git config in-process
    GRD-->>STUB: test increments counter and sleeps 30ms
    PRS->>GRD: checkedOutBranch forDirectory
    GRD-->>GMS: production reads HEAD file
    GRD-->>STUB: test returns notARepository

Loading

_{Reviews (2): Last reviewed commit: "sidebar-git: give the PR refresh run-loo..." | Re-trigger Greptile}

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

cmuxTests/WorkspacePullRequestSidebarTests.swift (1)

521-619: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift

Make the regression signal deterministic and sensitive to the actual stall.

The batch limit means this pass runs at most three 30ms discovery calls, so main-thread discovery would block for ~90ms and still pass the 2-second ceiling. The Thread.sleep/Timer/Date measurement also violates the test timing policy.

Use a controllable blocking gate: signal that discovery started, enqueue a main-runloop completion signal, and release discovery only after that signal arrives; use a deadline only as cleanup/failure protection. This fails if discovery occupies the main thread without asserting elapsed wall-clock latency.

🤖 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 `@cmuxTests/WorkspacePullRequestSidebarTests.swift` around lines 521 - 619,
Replace the Thread.sleep/Timer/Date timing measurement in the regression test
with a controllable blocking gate in BlockingRepositoryDiscovery: signal when
discovery starts, then block until the test releases it. After triggering
updateSurfaceShellActivity, enqueue a main-runloop completion signal and release
discovery only when that signal is observed; use a deadline solely to clean up
and fail safely. Keep the invocationCounter assertion, and make the test fail
when main-thread discovery prevents the queued completion from running rather
than relying on elapsed wall-clock latency.

Source: Coding guidelines

🤖 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.

Outside diff comments:
In `@cmuxTests/WorkspacePullRequestSidebarTests.swift`:
- Around line 521-619: Replace the Thread.sleep/Timer/Date timing measurement in
the regression test with a controllable blocking gate in
BlockingRepositoryDiscovery: signal when discovery starts, then block until the
test releases it. After triggering updateSurfaceShellActivity, enqueue a
main-runloop completion signal and release discovery only when that signal is
observed; use a deadline solely to clean up and fail safely. Keep the
invocationCounter assertion, and make the test fail when main-thread discovery
prevents the queued completion from running rather than relying on elapsed
wall-clock latency.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: c8ac529a-d0d3-4289-b67f-ec83abef975f

📥 Commits

Reviewing files that changed from the base of the PR and between 5dc99c6 and e1487ce.

📒 Files selected for processing (5)

• Packages/macOS/CmuxGit/Sources/CmuxGit/GitRepositoryDiscovering.swift
• Packages/macOS/CmuxGit/Sources/CmuxGit/PullRequestProbeService.swift
• Packages/macOS/CmuxSidebarGit/Sources/CmuxSidebarGit/Service/PullRequestPollService.swift
• Sources/TabManager.swift
• cmuxTests/WorkspacePullRequestSidebarTests.swift

[cursor]

Bugbot is paused — on-demand spend limit reached

Bugbot uses usage-based billing for this team and has hit its on-demand spend limit.

A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue.

[cursor]

Bugbot is paused — on-demand spend limit reached

Bugbot uses usage-based billing for this team and has hit its on-demand spend limit.

A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue.

[austinywang]

Review resolution on the final tree:

• CodeRabbit’s production test-seam finding is fixed. Sources/TabManager.swift is no longer changed; the regression test constructs PullRequestPollService directly and attaches the real TabManager host.
• The earlier sleep/timer-based run-loop check is replaced by a deterministic blocking gate. Discovery must start and a queued main-run-loop turn must execute before the test releases discovery. The five-second deadline exists only to prevent a deadlock and records a test failure if reached.
• The docstring warning is addressed: GitRepositoryDiscovering and both public requirements are documented, as are the widened poll-service initializer parameter and existing probe API.
• Greptile’s summary reported no actionable defect, but it describes the superseded TabManager override and sleep-based test; neither exists in the final diff.
• The three Cursor comments are spend-limit notices and contain no code feedback to address.

Verification on the final implementation: targeted runs https://github.com/manaflow-ai/cmux/actions/runs/30895230698 and https://github.com/manaflow-ai/cmux/actions/runs/30896696485 both passed; test wiring, package-lock policy, Swift parsing, git diff --check, and the unchanged warning budget were also checked.