From 36d30bccf280162113429576d5221129e97f7a3e Mon Sep 17 00:00:00 2001 From: Danny Gershman Date: Wed, 3 Jun 2026 19:34:20 -0400 Subject: [PATCH 1/2] Migrate UI provider guards to capability checks (#413) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two SwiftUI sites gated the "Mark as In Review" affordance with `session.provider == .github`. Replace those guards with `appState.canSetProjectStatus(for: session)`, computed via a capability resolver wired in AppDelegate that consults `TaskBackend.capabilities.contains(.projectBoardStatus)` — UI no longer knows about specific providers (ADR 0005). `GitHubTaskBackend` now declares `.projectBoardStatus`. The capability's contract is loosened to mean "the provider supports project-board status as a UI concept"; the `setTaskStatus` GraphQL migration is still deferred, and execution continues to route through the legacy `IssueTracker.markInReview` path via the unchanged `onMarkInReview` closure until that follow-up lands. Adds `AppStateCapabilityTests` for the new accessor and updates `BackendsTests` to expect the declared capability. 🐦‍⬛ Generated with Claude Code, orchestrated by Crow Co-Authored-By: Claude Crow-Session: 9439397C-463C-44E4-BE20-0E6A63303634 --- .../CrowCore/Sources/CrowCore/AppState.swift | 16 +++++ .../AppStateCapabilityTests.swift | 60 +++++++++++++++++++ .../Backends/GitHubTaskBackend.swift | 26 ++++---- .../CrowProviderTests/BackendsTests.swift | 12 ++-- .../Sources/CrowUI/SessionDetailView.swift | 2 +- .../Sources/CrowUI/SessionListView.swift | 2 +- Sources/Crow/App/AppDelegate.swift | 8 +++ 7 files changed, 109 insertions(+), 17 deletions(-) create mode 100644 Packages/CrowCore/Tests/CrowCoreTests/AppStateCapabilityTests.swift diff --git a/Packages/CrowCore/Sources/CrowCore/AppState.swift b/Packages/CrowCore/Sources/CrowCore/AppState.swift index 0865fdc..b6253ff 100644 --- a/Packages/CrowCore/Sources/CrowCore/AppState.swift +++ b/Packages/CrowCore/Sources/CrowCore/AppState.swift @@ -424,6 +424,22 @@ public final class AppState { terminals(for: sessionID).contains(where: { $0.isManaged }) } + /// Resolves whether a session's task backend declares the + /// `.projectBoardStatus` capability. Wired by `AppDelegate` using + /// `ProviderManager.taskBackend(for:)`. CrowUI does not depend on + /// CrowProvider, so the capability lookup is injected as a closure + /// (same pattern as `onMarkInReview`, `onListWorkspaceRepos`). + /// Defaults to `nil` so unwired contexts (tests, previews) treat the + /// capability as absent. See ADR 0005. + public var canSetProjectStatusResolver: ((Session) -> Bool)? + + /// Whether the session's provider supports setting project-board status, + /// based on the `TaskBackend` capability set. Replaces the previous + /// `session.provider == .github` UI guards. See ADR 0005. + public func canSetProjectStatus(for session: Session) -> Bool { + canSetProjectStatusResolver?(session) ?? false + } + public func primaryWorktree(for sessionID: UUID) -> SessionWorktree? { worktrees[sessionID]?.first(where: { $0.isPrimary }) ?? worktrees[sessionID]?.first } diff --git a/Packages/CrowCore/Tests/CrowCoreTests/AppStateCapabilityTests.swift b/Packages/CrowCore/Tests/CrowCoreTests/AppStateCapabilityTests.swift new file mode 100644 index 0000000..9c21130 --- /dev/null +++ b/Packages/CrowCore/Tests/CrowCoreTests/AppStateCapabilityTests.swift @@ -0,0 +1,60 @@ +import Foundation +import Testing +@testable import CrowCore + +// MARK: - AppState Capability Tests +// +// Cover the capability accessor that replaces UI provider guards +// (`session.provider == .github`). The accessor delegates to a resolver +// closure wired by AppDelegate using `ProviderManager.taskBackend(for:)` — +// these tests use stub closures to verify the wiring contract rather than +// the live capability lookup, which is covered separately by +// `BackendsTests.testGitHubTaskBackendDeclaresCapabilities`. See ADR 0005. + +@MainActor @Test func canSetProjectStatusReturnsFalseWhenResolverUnset() { + let appState = AppState() + let session = Session(name: "no-resolver", provider: .github) + #expect(appState.canSetProjectStatus(for: session) == false) +} + +@MainActor @Test func canSetProjectStatusReturnsResolverFalse() { + let appState = AppState() + appState.canSetProjectStatusResolver = { _ in false } + let session = Session(name: "resolver-false", provider: .github) + #expect(appState.canSetProjectStatus(for: session) == false) +} + +@MainActor @Test func canSetProjectStatusReturnsResolverTrue() { + let appState = AppState() + appState.canSetProjectStatusResolver = { _ in true } + let session = Session(name: "resolver-true", provider: .github) + #expect(appState.canSetProjectStatus(for: session) == true) +} + +@MainActor @Test func canSetProjectStatusPassesSessionToResolver() { + let appState = AppState() + let target = Session(name: "passed-through", provider: .gitlab) + var observed: Session? + appState.canSetProjectStatusResolver = { session in + observed = session + return session.provider == .gitlab + } + let result = appState.canSetProjectStatus(for: target) + #expect(observed?.id == target.id) + #expect(result == true) +} + +@MainActor @Test func canSetProjectStatusDelegatesForNilProvider() { + // The accessor itself does not short-circuit on `provider == nil`; + // that lives in the resolver body wired by AppDelegate. This documents + // the contract so a future refactor doesn't quietly move the gating. + let appState = AppState() + var called = false + appState.canSetProjectStatusResolver = { _ in + called = true + return false + } + let session = Session(name: "no-provider") + _ = appState.canSetProjectStatus(for: session) + #expect(called == true) +} diff --git a/Packages/CrowProvider/Sources/CrowProvider/Backends/GitHubTaskBackend.swift b/Packages/CrowProvider/Sources/CrowProvider/Backends/GitHubTaskBackend.swift index fd27b66..fc34610 100644 --- a/Packages/CrowProvider/Sources/CrowProvider/Backends/GitHubTaskBackend.swift +++ b/Packages/CrowProvider/Sources/CrowProvider/Backends/GitHubTaskBackend.swift @@ -5,18 +5,20 @@ import CrowCore /// /// Capabilities declared: /// - `.batchedQuery` — `listAssigned`-style fetches use one GraphQL call. +/// - `.projectBoardStatus` — GitHub Projects v2 is the UI surface this +/// capability gates. The `setTaskStatus` GraphQL migration is still +/// pending; the legacy `IssueTracker.markInReview` path (see +/// `IssueTracker.swift:2549`) executes the mutation today via the +/// `onMarkInReview` closure, so calling `setTaskStatus` on this backend +/// directly still throws `.unimplemented`. UI guards branch on this +/// capability instead of `provider == .github` (see ADR 0005); execution +/// falls through to the legacy path until the migration lands. /// -/// Note: `.projectBoardStatus` is intentionally *not* declared until the -/// `markInReview` GraphQL migration lands (see `setTaskStatus` below and -/// IssueTracker.swift:2539). The flag is contract: declaring it means a -/// capability-gated caller can call `setTaskStatus` and expect success. -/// We add the flag when the implementation arrives, not before. -/// -/// See ADR 0005 for the protocol contract and ADR 0005's Context section for why +/// See ADR 0005 for the protocol contract and its Context section for why /// task ops are separate from code ops. public struct GitHubTaskBackend: TaskBackend { public let provider: Provider = .github - public let capabilities: Set = [.batchedQuery] + public let capabilities: Set = [.batchedQuery, .projectBoardStatus] private let shellRunner: ShellRunner @@ -65,10 +67,12 @@ public struct GitHubTaskBackend: TaskBackend { public func setTaskStatus(url: String, status: TicketStatus) async throws { // Real implementation requires the GraphQL project-item lookup + mutation - // sequence currently inlined at IssueTracker.swift:2539. That migration is + // sequence currently inlined at IssueTracker.swift:2549. That migration is // deferred to a follow-up PR — see ADR 0005 references. - // The capability is declared so callers don't fall into the throw path - // before the migration lands. + // UI guards key off `.projectBoardStatus` (declared above); execution + // currently routes through `IssueTracker.markInReview` via the + // `onMarkInReview` closure rather than through this method, so the + // throw is unreached on the live path until the migration lands. throw ProviderError.unimplemented( "GitHubTaskBackend.setTaskStatus: migration of markInReview project-board mutation pending" ) diff --git a/Packages/CrowProvider/Tests/CrowProviderTests/BackendsTests.swift b/Packages/CrowProvider/Tests/CrowProviderTests/BackendsTests.swift index 66e542b..8c9234b 100644 --- a/Packages/CrowProvider/Tests/CrowProviderTests/BackendsTests.swift +++ b/Packages/CrowProvider/Tests/CrowProviderTests/BackendsTests.swift @@ -32,10 +32,14 @@ final class BackendsTests: XCTestCase { func testGitHubTaskBackendDeclaresCapabilities() { let backend = GitHubTaskBackend(shellRunner: FakeShellRunner()) XCTAssertEqual(backend.provider, .github) - // .projectBoardStatus intentionally NOT declared until the markInReview - // GraphQL migration lands. Declaring it while setTaskStatus throws would - // lie to capability-gated callers. See ADR 0005. - XCTAssertFalse(backend.capabilities.contains(.projectBoardStatus)) + // `.projectBoardStatus` declares the UI surface (GitHub Projects v2). + // The `setTaskStatus` GraphQL migration is still pending — execution + // currently routes through IssueTracker.markInReview via the + // onMarkInReview closure, so direct `setTaskStatus` calls on this + // backend still throw .unimplemented (asserted by + // testGitHubTaskBackendSetTaskStatusThrowsUnimplemented). See ADR 0005 + // and issue crow#413 for the UI guard migration. + XCTAssertTrue(backend.capabilities.contains(.projectBoardStatus)) XCTAssertTrue(backend.capabilities.contains(.batchedQuery)) } diff --git a/Packages/CrowUI/Sources/CrowUI/SessionDetailView.swift b/Packages/CrowUI/Sources/CrowUI/SessionDetailView.swift index 027f74c..9ee05eb 100644 --- a/Packages/CrowUI/Sources/CrowUI/SessionDetailView.swift +++ b/Packages/CrowUI/Sources/CrowUI/SessionDetailView.swift @@ -173,7 +173,7 @@ public struct SessionDetailView: View { if session.status == .active, session.ticketURL != nil, - session.provider == .github { + appState.canSetProjectStatus(for: session) { if appState.isMarkingInReview[session.id] == true { ProgressView() .controlSize(.small) diff --git a/Packages/CrowUI/Sources/CrowUI/SessionListView.swift b/Packages/CrowUI/Sources/CrowUI/SessionListView.swift index b4071d5..6b7a8c2 100644 --- a/Packages/CrowUI/Sources/CrowUI/SessionListView.swift +++ b/Packages/CrowUI/Sources/CrowUI/SessionListView.swift @@ -300,7 +300,7 @@ public struct SessionListView: View { if session.status == .active, session.ticketURL != nil, - session.provider == .github { + appState.canSetProjectStatus(for: session) { Button { appState.onMarkInReview?(session.id) } label: { diff --git a/Sources/Crow/App/AppDelegate.swift b/Sources/Crow/App/AppDelegate.swift index 27f312d..2bb1c80 100644 --- a/Sources/Crow/App/AppDelegate.swift +++ b/Sources/Crow/App/AppDelegate.swift @@ -715,6 +715,14 @@ final class AppDelegate: NSObject, NSApplicationDelegate { self?.jobScheduler?.runNow(jobID) } + appState.canSetProjectStatusResolver = { [providerManager] session in + guard let provider = session.provider else { return false } + return providerManager + .taskBackend(for: provider) + .capabilities + .contains(.projectBoardStatus) + } + appState.onMarkInReview = { [weak tracker] id in Task { await tracker?.markInReview(sessionID: id) } } From a713a44d5842a8387f1bf2b63528d3d5af629019 Mon Sep 17 00:00:00 2001 From: Danny Gershman Date: Wed, 3 Jun 2026 19:40:50 -0400 Subject: [PATCH 2/2] Align TaskBackend and ADR 0005 docs with loosened .projectBoardStatus contract MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review on PR #419 flagged that the protocol doc on `setTaskStatus`, the `.projectBoardStatus` enum case, and the ADR 0005 example all still encoded the old strict contract ("declaring the capability means setTaskStatus succeeds"). With `GitHubTaskBackend` now declaring the capability while `setTaskStatus` still throws .unimplemented (legacy IssueTracker.markInReview executes), those docs directly contradicted the implementation and could mislead a future caller of setTaskStatus. Rewrite each to reflect the loosened "UI surface" meaning: the capability gates UI affordances; method callability is a separate concern that may still throw .unimplemented until a backend's implementation lands. Split the ADR's single example into two (UI-gating vs method-calling) so the distinction is explicit. 🐦‍⬛ Generated with Claude Code, orchestrated by Crow Co-Authored-By: Claude Crow-Session: 9439397C-463C-44E4-BE20-0E6A63303634 --- .../Sources/CrowProvider/TaskBackend.swift | 17 +++++++++++++---- .../adr/0005-task-and-code-backend-protocols.md | 13 +++++++++++-- 2 files changed, 24 insertions(+), 6 deletions(-) diff --git a/Packages/CrowProvider/Sources/CrowProvider/TaskBackend.swift b/Packages/CrowProvider/Sources/CrowProvider/TaskBackend.swift index cc8d95a..0f7ddb3 100644 --- a/Packages/CrowProvider/Sources/CrowProvider/TaskBackend.swift +++ b/Packages/CrowProvider/Sources/CrowProvider/TaskBackend.swift @@ -31,8 +31,14 @@ public protocol TaskBackend: Sendable { func setLabels(url: String, add: [String], remove: [String]) async throws /// Set the project-board status for a task. - /// Capability-gated: only call when `capabilities.contains(.projectBoardStatus)`. - /// Calling without the capability throws `ProviderError.unimplemented`. + /// Capability-gated for **UI surfacing**: callers gate the affordance on + /// `capabilities.contains(.projectBoardStatus)` (see ADR 0005). The + /// capability does *not* guarantee this method is implemented — a backend + /// may declare it to surface the UI while routing execution through a + /// legacy path (e.g. `GitHubTaskBackend` declares the capability but + /// throws here; `IssueTracker.markInReview` performs the mutation until + /// the `setTaskStatus` GraphQL migration lands). Calling without the + /// capability also throws `ProviderError.unimplemented`. func setTaskStatus(url: String, status: TicketStatus) async throws } @@ -42,8 +48,11 @@ public protocol TaskBackend: Sendable { /// becomes `if taskBackend.capabilities.contains(.projectBoardStatus)`, so adding /// a third provider (or extending an existing one) doesn't ripple through call sites. public enum TaskCapability: Sendable, Hashable { - /// Can set project-board status (GitHub Projects v2 today). - /// Gates `setTaskStatus`. Without this capability that method throws. + /// Declares that the provider exposes a project-board status concept the + /// UI should surface (GitHub Projects v2 today). Gates UI affordances such + /// as the "Mark as In Review" button. Does **not** guarantee that calling + /// `setTaskStatus` succeeds — see `setTaskStatus` for the implementation + /// caveat. Backends without this capability hide the affordance entirely. case projectBoardStatus /// Can fulfill `listAssigned`-style queries in a single batched call rather diff --git a/docs/adr/0005-task-and-code-backend-protocols.md b/docs/adr/0005-task-and-code-backend-protocols.md index d2754e6..7b18158 100644 --- a/docs/adr/0005-task-and-code-backend-protocols.md +++ b/docs/adr/0005-task-and-code-backend-protocols.md @@ -26,15 +26,24 @@ Crow exposes two independent protocols under `CrowProvider`: - **`TaskBackend`** — tracker side. Owns issue/task lifecycle: `fetchTask`, `listAssigned`, `setLabels`, `setTaskStatus`, `assign`, `createTask`. - **`CodeBackend`** — VCS side. Owns PR lifecycle: `linkedPR`, `prStates`, `ensureMergeLabel`, `fetchCrowAuthoredCommits`. -Each backend declares its **capabilities** as a `Set` (`TaskCapability` and `CodeCapability`). Callers branch on capabilities, not on provider identity. Examples: +Each backend declares its **capabilities** as a `Set` (`TaskCapability` and `CodeCapability`). Callers branch on capabilities, not on provider identity. Capabilities gate UI affordances and call-site decisions; they do not by themselves guarantee a method's implementation is wired (a backend may declare a UI-facing capability while routing execution through a legacy path — see `.projectBoardStatus` on `GitHubTaskBackend`, where `IssueTracker.markInReview` performs the mutation until the `setTaskStatus` GraphQL migration lands). Examples: ```swift +// UI gating — the direct replacement for `if session.provider == .github { ... }`. +if taskBackend.capabilities.contains(.projectBoardStatus) { + showInReviewButton() +} + +// Calling a capability-gated method. Backends that declare the capability may +// still throw `.unimplemented` if their implementation is pending; callers +// that exercise the method directly must be prepared for that and either fall +// back to a legacy path or surface the error. if taskBackend.capabilities.contains(.projectBoardStatus) { try await taskBackend.setTaskStatus(url: url, status: .inReview) } ``` -This is the direct replacement for `if session.provider == .github { ... }`. New backends declare what they support and the call site asks; no `switch` over a closed set of providers. +New backends declare what they support and the call site asks; no `switch` over a closed set of providers. Backends take a `ShellRunner` (a thin `protocol` over `Process()` + `ShellEnvironment.shared`) at init, so unit tests inject a `FakeShellRunner` that records command vectors and returns canned JSON. The three near-duplicate `private func shell` implementations in `ProviderManager`, `IssueTracker`, and `SessionService` collapse into one `ProcessShellRunner`.