Extract TaskBackend + CodeBackend protocols (#410)

[dgershman]

Summary

• Decouples gh/glab shell-outs from call sites behind two distinct protocols: TaskBackend (issues/labels/status) and CodeBackend (PRs/merge labels). The split exists because tasks and code are independent dimensions — a Corveil-tasked session can pair with a GitHub/GitLab CodeBackend, and a non-coding task needs no CodeBackend at all.
• Adds a ShellRunner protocol so backends are unit-testable without a real shell. Production uses ProcessShellRunner; tests use a recording fake.
• ProviderManager becomes a factory (taskBackend(for:) / codeBackend(for:) / taskBackend(forURL:)), backends are Sendable value types handed across the actor boundary.
• Capability flags (TaskCapability / CodeCapability) replace if session.provider == .github guards.
• Stub Corveil task backend proves the abstraction holds for a third provider — every method throws .unimplemented cleanly.
• SessionService recovery paths (ticket-title fetch, PR-link lookup) migrate to the new abstraction with a fallback to the inline shell-out when no ProviderManager is wired (no behavior change in production).
• ADR 0005 captures the spec.

Closes #410.

Architecture

See ADR 0005 — task/code separation, capability flags, ShellRunner injection, Corveil-as-task-only.

Test plan

• swift build --arch arm64 clean
• Existing 205 CrowCore + 35 ProviderManagerTests + 152 root CrowTests all green
• 15 new BackendsTests covering each backend's command vector, capability flags, error paths, and the Corveil stub
• Manual smoke: GitHub and GitLab sessions remain identical pre/post-refactor

Follow-ups (filed separately, no `crow:auto`)

• Migrate `AutoRespondCoordinator` CLI string selection to backends
• Migrate UI provider guards to capability checks
• Add `Session.codeProvider` for cross-backend pairing

🐦‍⬛ Generated with Claude Code, orchestrated by Crow

[dhilgaertner]

Code & Security Review

Clean, well-documented refactor. The protocol split (TaskBackend / CodeBackend), ShellRunner injection, capability flags, and the Corveil stub all hang together, and ADR 0005 explains the why well. swift test on CrowProvider is green — all 15 new BackendsTests pass plus the 35 ProviderManagerTests. (Full swift build couldn't run here: Frameworks/GhosttyKit.xcframework has no binary artifact in this checkout — an environment limitation, not a PR defect.)

Two should-fix items below before merge; both are mechanical.

Security Review

Strengths:

• No command injection: ProcessShellRunner spawns /usr/bin/env with an arguments array (no sh -c), and every backend passes url/repo/branch/labels as discrete argv elements — never concatenated into a shell string. Untrusted URL/branch values cannot break out.
• The pipe is drained (readDataToEndOfFile()) before waitUntilExit(), avoiding the >64KB deadlock — the comment is accurate.
• GITLAB_HOST is passed via the env dictionary, not interpolated.

Concerns: none.

Code Quality

Yellow — GitHubTaskBackend declares .projectBoardStatus but setTaskStatus throws .unimplemented (GitHubTaskBackend.swift:62-71)
The TaskBackend protocol contract states the capability gates the method: "Gates setTaskStatus. Without this capability that method throws." GitHub declares the capability and throws, which directly contradicts that contract. The whole point of capability flags is to let callers do if backend.capabilities.contains(.projectBoardStatus) { try await backend.setTaskStatus(...) } — a future caller that trusts the flag will hit a runtime throw on GitHub. No live caller exercises this yet (confirmed: only SessionService uses the new API, and only fetchTask/linkedPR), so it's latent — but it's a landmine being committed. Safer to not declare .projectBoardStatus until the migration lands; then capability-gated callers correctly skip it and the flag stays honest. (.batchedQuery is documented as informational, so that one is fine.)

Yellow — findPRLink runs gh pr list twice when no PR exists (SessionService.swift:1151-1164)
The new backend path is an if let … pr = try? … linkedPR(…) whose body returns. When linkedPR returns nil (the common "no PR found" recovery case), the if let pr fails and execution falls through to the legacy guard let prOutput = try? await shell("gh", "pr", "list", …) block — which issues the identical gh pr list command a second time. Since providerManager is now always wired in production (AppDelegate.swift:431), every no-PR recovery pays for a redundant network round-trip. The fetchTask path just above (:1129-1138) gets this right with if manager … else if legacy …; findPRLink should mirror that structure (legacy only as the providerManager == nil fallback) rather than an unconditional fall-through.

Green (consider)

• detectProviderNonisolated (ProviderManager.swift:55-71) duplicates detectProvider verbatim with a "keep both in sync" comment. Extracting the shared body into one nonisolated static the actor method wraps would remove the drift risk entirely.
• GitHubCodeBackend.ensureMergeLabel (GitHubCodeBackend.swift:51) re-throws as nonZeroExit(exitCode: 1, …), discarding the real exit code. Re-throw the original error to preserve it.
• GitLabTaskBackend.fetchTask derives the title from output.components(separatedBy: .newlines).first — the first line of glab issue view text output, which may be a header rather than the title. Acceptable for v1; worth a follow-up if GitLab issue titles matter downstream.

Summary Table

Color	Meaning	Verdict effect
Red	Must fix	Request changes
Yellow	Should fix	Request changes
Green	Consider	Approve allowed

Recommendation: Request Changes — driven by [0 Red, 2 Yellow, 3 Green] findings. Both Yellow items are small, localized fixes; addressing them (plus the optional Green cleanups) in this round avoids a follow-up trip.

🐦‍⬛ Reviewed by Crow via Claude Code

[dhilgaertner]

Code & Security Review

Reviewed the full diff (18 files, +904/-15) for PR #411 — extraction of TaskBackend + CodeBackend protocols behind a ShellRunner-injectable factory. This is a clean, behavior-preserving refactor with strong test coverage.

Verification

• swift test --package-path Packages/CrowProvider → 50 tests green (15 new BackendsTests + 35 existing ProviderManagerTests).
• Grepped every switch over Provider repo-wide — all sites (ClaudeLauncher, CodexLauncher, ProviderManager ×4, IssueTracker) now handle the new .corveil case, so the additive enum case introduces no exhaustiveness compile breaks. (Full app target couldn't be built locally — GhosttyKit.xcframework binary is absent in the review checkout — but package tests + switch audit cover the changed logic.)

Security Review

Strengths:

• All CLI invocations go through Process.arguments (argv), never a shell string — URL/branch/repo values can't inject commands.
• No secrets logged; NSLog only emits PR number + branch.
• GITLAB_HOST is sourced from config, merged via ShellEnvironment.merging.

No security concerns.

Code Quality

Strengths:

• ProcessShellRunner drains the pipe before waitUntilExit() on a detached task — this is strictly more correct than the legacy ProviderManager.shell (line 292-294), which waits-then-reads and can deadlock on >64KB output. The new abstraction quietly fixes that latent bug.
• Sendable value-type backends + nonisolated let factory state read across the actor boundary is sound; nonisolated factory methods only touch immutable Sendable storage.
• Capability flags correctly model contract-vs-implementation: .projectBoardStatus is deliberately withheld while setTaskStatus throws, and the test asserts that, so capability-gated callers aren't lied to.
• ensureMergeLabel swallows only the "already exists" non-zero exit and rethrows everything else — correct narrow catch.
• Recovery-path migrations (parseTicketInfo title fetch, findPRLink) preserve production behavior and avoid double-issuing the CLI on the common "no PR" path.

Summary Table

Color	Meaning	Verdict effect
Red	Must fix	Request changes
Yellow	Should fix	Request changes
Green	Consider	Approve allowed

Consider (Green — non-blocking)

• 🟢 GitLabCodeBackend.linkedPR (glab mr list -F json) and GitLabTaskBackend.setLabels have no command-vector tests, unlike their GitHub counterparts. Low risk today since the only wired CodeBackend caller (findPRLink) hardcodes .github and the wired TaskBackend caller exercises fetchTask (tested) — but add command-vector coverage when these GitLab paths get wired up.
• 🟢 findPRLink routes through the new CodeBackend abstraction but still hardcodes .github rather than detecting the repo's provider. Behavior is identical to pre-PR (was always gh pr list), and the description scopes provider-guard migration as an explicit follow-up — just flagging that GitLab MR linking remains unsupported until then.
• 🟢 ProcessShellRunner merges stderr into the stdout pipe that backends then JSON-parse; a stray gh/glab stderr warning could corrupt a parse. This matches the pre-existing shell() behavior, so no regression.

Recommendation: Approve — driven by [0 Red, 0 Yellow, 3 Green] findings. Well-scoped, well-tested, behavior-preserving, and it incidentally hardens a deadlock-prone shell path.

🐦‍⬛ Reviewed by Crow via Claude Code