Replace StubCorveilTaskBackend with real CorveilTaskBackend (CROW-495)#501
Conversation
Mirrors JiraTaskBackend over the `corveil` CLI surface so Corveil-tasked sessions work end-to-end (paired with a GitHub/GitLab CodeBackend via Session.codeProvider). Declares `[batchedQuery, projectBoardStatus]` — batched on the assumption corveil#1364's bulk `--ids` has landed; the project-board status surface is real, with `.inReview` mapping lossily to corveil's `in_progress` (no review-distinct intermediate exists). - New `CorveilTaskBackend.swift` with `CorveilConfig` + `CorveilTaskID` parser; all six TaskBackend methods build `corveil task …` argv via the shared `ShellRunner`. Auth-failure output rewrites to a clear "run `corveil login`" hint, matching the Jira pattern. - `ProviderManager` factory swaps the `.corveil` arm to the real backend and `fetchTicket` delegates Corveil URLs the same way Jira does; URL detection takes an `additionalCorveilHosts` list parallel to GitLab. - `WorkspaceInfo.corveilHost` (Codable, decoded with `decodeIfPresent`) plus a "Corveil host" row in `WorkspaceFormView` and a `Corveil` tag in the Task Backend picker. - `IssueTracker` dedupes per-workspace `CorveilConfig`s and adds a `fetchCorveilIssues` path next to `fetchJiraIssues`. - Removes `StubCorveilTaskBackend.swift` and the associated stub test; ADR 0005 swaps the Corveil row in the Decision and Migration tables and documents the `.inReview → in_progress` asymmetry. - New `CorveilTaskBackendTests.swift` (23 tests against `FakeShellRunner`, covering argv shape, JSON parsing, status round-trips, and auth-error surfacing); `ProviderManagerTests` gains five Corveil routing tests. 🐦⬛ Generated with Claude Code, orchestrated by Crow Co-Authored-By: Claude <[email protected]> Crow-Session: F5DEB949-5B6F-4878-9D00-306312BBE797
dhilgaertner
left a comment
There was a problem hiding this comment.
Code & Security Review
Real CorveilTaskBackend cleanly mirrors JiraTaskBackend, the ADR 0005 updates are accurate, and the test coverage is strong (23 new backend tests + 5 routing tests; all 86 CrowProvider tests pass locally). One parity issue blocks approval.
Critical Issues
None.
Code Quality
🟡 listAssigned queries only --status open and silently drops in-progress tasks
CorveilTaskBackend.listAssigned fetches the open half with corveil task list --assignee @me --status open (Backends/CorveilTaskBackend.swift:131-137). Corveil's status vocabulary is open / in_progress / closed, where in_progress is a distinct status value, so --status open filters server-side to literally status == open and excludes in-progress work.
This breaks parity with the sibling backends, which both return everything not closed and treat the active/review stage as an orthogonal dimension:
- GitHub:
assignee:@me state:open type:issue—state:open= not-closed; in-progress is a project-board field (GitHubTaskBackend.swift:48). - Jira:
assignee = currentUser() AND statusCategory != Done(JiraTaskBackend.swift:47).
The concrete consequence: setTaskStatus maps .inProgress/.inReview → in_progress (CorveilTaskBackend.swift:262-269), which is exactly what happens when a Crow session starts work on a ticket. On the next IssueTracker.refresh poll, fetchCorveilIssues → listAssigned(includeClosed: false) will no longer return that task, so the ticket you're actively working vanishes from the assigned board.
The code itself shows the intent mismatch: parseAssigned for the open call passes statusOverride: nil and maps in_progress → .inProgress (CorveilTaskBackend.swift:314), and testListAssignedSendsAtMeAndOpenStatus (CorveilTaskBackendTests.swift:716-735) feeds an in_progress task into the open response and asserts projectStatus == .inProgress — but the live --status open query will never deliver that task to the parser. The unit test passes only because FakeShellRunner returns whatever it's handed regardless of the argv filter.
Suggested fix: fetch not-closed (e.g. issue both --status open and --status in_progress, or use a "not closed" filter if corveil supports one) so in-progress tasks remain on the board, matching GitHub/Jira semantics.
Security Review
Strengths:
- No secrets handled — auth is delegated entirely to the
corveilCLI (corveil login), consistent with the Jira/GitLab pattern. - All CLI args are passed as a discrete argv array via
ShellRunner(no shell string interpolation), so user-supplied titles/labels/URLs can't inject shell commands. - URL/id parsing in
CorveilTaskID.parseis strict (requires a numeric suffix) and rejects unparseable input cleanly.
Concerns: None.
Minor (non-blocking)
- 🟢
taskBackend(forURL:)detects a self-hosted host but the.corveilfactory case ignoreshost(it threadsCorveilConfig, nothost), so the host-built URL fallback won't fire for URL-driven ops on self-hosted instances. This matches the existing Jira path and the fallback is rarely taken post-corveil#1363, so impact is low — just noting it. - 🟢
looksUnauthenticatedsubstring-matches"unauthorized"/"not authenticated", which could theoretically misclassify an unrelated error as an auth hint. Identical toJiraTaskBackendand only affects hint text — fine as-is.
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, 1 Yellow, 2 Green] findings.
Addresses PR #501 review feedback. Corveil's `--status` is an exact match on a single status value, not "not closed" semantics — so a `--status open` query excludes in_progress tasks. The concrete consequence: a task we just moved to in_progress via `setTaskStatus(.inProgress)` would vanish from the assigned board on the next `IssueTracker.refresh` poll because `fetchCorveilIssues` → `listAssigned(includeClosed: false)` would no longer return it. `listAssigned` now issues one call for `--status open` and one for `--status in_progress`, merging the results into the open half so the "not closed" parity matches GitHub (`state:open`) and Jira (`statusCategory != Done`). - Factors out the per-status query into a `listByStatus(_:)` helper. - Replaces `testListAssignedSendsAtMeAndOpenStatus` (which fed an in_progress task into the "open" response — a no-op under the old argv filter) and updates `testListAssignedIssuesSecondCallWhenIncludeClosed` to expect three calls (open + in_progress + closed). - Updates ADR 0005's TaskBackend status row to describe the fan-out. 🐦⬛ Generated with Claude Code, orchestrated by Crow Co-Authored-By: Claude <[email protected]> Crow-Session: F5DEB949-5B6F-4878-9D00-306312BBE797
dhilgaertner
left a comment
There was a problem hiding this comment.
Code & Security Review
Mirrors the established JiraTaskBackend shape over the corveil CLI cleanly. Verified locally: swift test --package-path Packages/CrowProvider passes (XCTest suite + 49 swift-testing cases, including the 23 new CorveilTaskBackendTests and 6 Corveil routing tests); CrowCore builds clean. (CrowUI/root app couldn't link here due to a missing GhosttyKit.xcframework binary — an environment limitation unrelated to this PR; those touched files are mechanical and follow the existing Jira/GitLab patterns.)
Critical Issues
None.
Security Review
Strengths:
- All
corveilinvocations go throughShellRunner.run(args:env:cwd:), whichProcessShellRunnerexecutes via/usr/bin/envwith an argv vector — no shell interpolation, so titles/bodies/labels/logins carrying shell metacharacters cannot inject commands. - No secrets handled in-process: auth lives entirely in the
corveilCLI (corveil login), and thecorveilHostfield is explicitly URL-routing-only (documented inAppConfig.swift:9-13). looksUnauthenticatedsurfaces a clear, non-sensitive "runcorveil login" hint instead of leaking raw failure internals.
Concerns: None.
Code Quality
Solid throughout. A few optional Green / consider notes (none blocking):
- Self-hosted URL detection is inert at runtime.
additionalCorveilHostsis only ever populated in tests —AppDelegateconstructsProviderManager()with no args, so pasting a self-hosted Corveil ticket URL into the attach flow routes to.github. This exactly mirrors the existingadditionalGitLabHostsgap (also never wired into the liveProviderManager), and the polling/listing path works fine sinceIssueTrackerpassesCorveilConfig(host:)directly. The PR's own test plan leaves this smoke-test item unchecked. Consistent pre-existing limitation, reasonable as a follow-up — flagging so it's a conscious choice. listAssignedopen-half is all-or-nothing.listByStatus("open")andlistByStatus("in_progress")share onedo/catch, so a transient failure on thein_progresscall discards the already-fetchedopenresults for that cycle (CorveilTaskBackend.swift:135-144). Self-heals next poll and matches the best-effort degrade contract, but preserving theopenresults on a partial failure would be slightly more robust.- Minor duplication: the host→
/dashboard/tasks/{id}URL build appears in bothbrowseURL(for:)and inline inparseAssigned(CorveilTaskBackend.swift:249-254and325-330) — could share one helper.
Summary Table
| Color | Meaning | Verdict effect |
|---|---|---|
| Red | Must fix | Request changes |
| Yellow | Should fix | Request changes |
| Green | Consider | Approve allowed |
Recommendation: Approve — driven by [0 Red, 0 Yellow, 3 Green] findings.
Summary
JiraTaskBackendover thecorveilCLI:corveil task get/list/update/createfor all sixTaskBackendmethods. Declares[batchedQuery, projectBoardStatus]..corveilfactory case +fetchTicketdelegation inProviderManager; adds anadditionalCorveilHostsURL-routing list parallel to GitLab.WorkspaceInfo.corveilHost(Codable), a "Corveil host" row + newCorveiltask-backend tag inWorkspaceFormView, and a per-workspaceCorveilConfigfan-out inIssueTracker.refresh.StubCorveilTaskBackend.swiftand the associated stub test; updates ADR 0005 Decision + Migration tables and documents the.inReview → in_progresslossy mapping.CorveilTaskBackendTests.swift(23 tests againstFakeShellRunner) + five Corveil routing tests inProviderManagerTests.Status mapping: Crow → Corveil is
backlog/ready → open,inProgress/inReview → in_progress(the only lossy mapping — corveil has no review-distinct status),done → closed. Reverse:open → .ready,in_progress → .inProgress,closed → .done.Closes #495.
Test plan
swift test --package-path Packages/CrowProvider— 86 tests, 0 failures (46 BackendsTests + 23 new CorveilTaskBackendTests + 17 JiraTaskBackendTests + 49 ProviderManagerTests).swift test --package-path Packages/CrowCore— 271 tests, 0 failures (WorkspaceInfo Codable round-trip with newcorveilHostfield included).swift test --package-path Packages/CrowUI— 36 tests, 0 failures.swift test --filter IssueTrackerat repo root — 120 tests, 0 failures (no regression from the newfetchCorveilIssuespath).rg StubCorveilTaskBackend Sources/ Packages/ docs/returns zero matches.corveilHostround-trips through the config file. Pastehttps://corveil.io/dashboard/tasks/42into the ticket-attach flow —ProviderManager.detectshould route to.corveilandfetchTaskshould run (failing only oncorveil login, not the oldunimplementedstub error).🤖 Generated with Claude Code