Add sidebar "Add label crow:merge to PR" context-menu action (CROW-502)#503
Conversation
Right-clicking a session row that has a PR now offers "Add label crow:merge to PR", which ensures the label exists in the repo and adds it to the PR so the existing auto-merge watcher can pick it up — no need to leave Crow. - CodeBackend: new addMergeLabel(prURL:) (GitHub: `gh pr edit --add-label`), with a protocol-default throw so non-capable backends inherit it. - AppState: onAddMergeLabel closure, isAddingMergeLabel in-flight flag, and canAddMergeLabel(for:) capability resolver (ADR 0005 injection so CrowUI stays decoupled from CrowProvider). - IssueTracker.addMergeLabel(sessionID:) mirrors markInReview's capability gate / in-flight / error handling; repoSlug(fromPRURL:) parses owner/repo from a PR web URL. - AppDelegate wires the resolver + closure; SessionService clears the in-flight flag on delete. - SessionListView: self-gating addMergeLabelButton shown in the shared row menu and the inline Reviews menu; hidden when no PR or capability absent. 🐦⬛ Generated with Claude Code, orchestrated by Crow Co-Authored-By: Claude <noreply@anthropic.com> Crow-Session: 7CD33545-55C4-4C1D-8904-1126C7A16D9E
dgershman
left a comment
There was a problem hiding this comment.
Code & Security Review
Critical Issues
None.
Security Review
Strengths:
GitHubCodeBackend.addMergeLabeluses direct argv (nosh -c), eliminating shell interpolation aroundprURL, and pinscwdto$TMPDIRsoghcannot infer an unintended repo from a developer's cwd — same hardened pattern asenableAutoMerge.- Capability gating is layered correctly:
AppDelegateresolver →AppState.canAddMergeLabel→ UI hides the item;IssueTracker.addMergeLabel(sessionID:)re-guards on.autoMergeLabel; the protocol-extension default throws.unimplementedso a non-capable backend slipping through fails loudly instead of silently no-op'ing. AppState.canAddMergeLabelResolverfollows ADR 0005 — CrowUI stays free of a CrowProvider dependency.SessionService.deleteSessioncleans upisAddingMergeLabel, preventing the per-session dictionary from leaking after deletion.
Concerns:
- None.
Code Quality
🟡 Yellow — Missing unit tests for IssueTracker.repoSlug(fromPRURL:) (Sources/Crow/App/IssueTracker.swift:1431).
The sibling parsers extractHost(fromRemote:) and extractSlug(fromRemote:) already have a small test battery in Tests/CrowTests/IssueTrackerReconcileTests.swift:151+ covering the supported URL shapes and empty inputs. repoSlug(fromPRURL:) is a new pure, nonisolated static function with several real-world inputs to disambiguate — https://github.com/owner/repo/pull/123, the GitLab …/group/sub/repo/-/merge_requests/12 shape, URLs with query strings or fragments, missing scheme, empty input — and an empty return value silently weakens the ensureMergeLabel step (see Green #2). Add a parallel test block in IssueTrackerReconcileTests.swift (or a new file) so future URL-shape changes don't regress this quietly.
🟢 Green — Unreachable catch ProviderError.insufficientScope (Sources/Crow/App/IssueTracker.swift:2348).
GitHubCodeBackend.addMergeLabel (and ensureMergeLabel) only propagate ShellRunnerError.nonZeroExit from gh; neither classifies stderr into ProviderError.insufficientScope. The catch branch therefore can't fire today — calling reportScopeWarning("merge label") will never happen even when the underlying gh pr edit fails with HTTP 403 / Resource not accessible by integration. Either classify the shell error in the backend (mirroring classifyGraphQLError) or drop the dead branch and lean on the generic catch. Defensible as forward-looking if you'd rather leave it; flagging so the intent is explicit.
🟢 Green — Empty repoSlug silently downgrades ensureMergeLabel (Sources/Crow/App/IssueTracker.swift:2343-2346).
if !repo.isEmpty { try await backend.ensureMergeLabel(repo: repo) } swallows the malformed-URL case and still calls addMergeLabel(prURL:). If the parser ever misses a URL shape and the repo doesn't already have a crow:merge label, gh pr edit --add-label crow:merge fails and the user sees only a print line. Consider bailing out (no slug → no action + warning) so the user-visible behavior matches the contract of the menu item.
🟢 Green — No user-visible feedback on success/failure.
addMergeLabel(sessionID:) only emits NSLog/print. Consistent with markInReview, so not blocking, but the menu action looks identical to an inert no-op from the user's perspective — particularly painful when gh pr edit fails for an auth reason. A toast or a transient state in appState.prStatus[sessionID] would close the loop. (Out of scope of this PR if the rest of the codebase doesn't do it.)
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, 3 Green] findings. The Yellow item (missing tests for repoSlug(fromPRURL:)) should land in the same round-trip; the Green items are optional.
…ROW-502) - Add IssueTrackerPRSlugTests covering repoSlug(fromPRURL:) for the GitHub PR / GitLab MR shapes, query strings, fragments, http scheme, missing scheme, and empty input (Yellow). - addMergeLabel now bails with a warning when the repo slug can't be parsed, instead of calling addMergeLabel with no ensureMergeLabel and half-doing the action (Green #3). - Drop the unreachable `catch ProviderError.insufficientScope` — the gh code backend only throws ShellRunnerError, so the generic catch covers it (Green #2). 🐦⬛ Generated with Claude Code, orchestrated by Crow Co-Authored-By: Claude <noreply@anthropic.com> Crow-Session: 7CD33545-55C4-4C1D-8904-1126C7A16D9E
|
Thanks for the review! Addressed in d9c31c4:
|
dgershman
left a comment
There was a problem hiding this comment.
Code & Security Review
Critical Issues
None.
Security Review
Strengths:
addMergeLabel(prURL:)runsgh pr edit <url> --add-label crow:mergeas direct argv (nosh -c), so the PR URL cannot inject shell metacharacters. Matches the establishedenableAutoMergepattern (GitHubCodeBackend.swift:255–263).- Sets
cwd: NSTemporaryDirectory()soghcannot infer the wrong repo from the surrounding cwd — important because the operator's terminal may be inside a different worktree. - Capability-gated twice: UI hides the button when
canAddMergeLabel(for:)is false (SessionListView.swift:336–345), andIssueTracker.addMergeLabelre-checksbackend.capabilities.contains(.autoMergeLabel)before issuing any command. A spoofedonAddMergeLabelinvocation still no-ops on non-capable backends. - The default protocol-extension implementation throws
.unimplemented, so a slipped-through call to GitLab/Corveil degrades to a logged error rather than a silent half-action. repoSlug(fromPRURL:)returns""for malformed URLs and the caller bails loudly (IssueTracker.swift:2349–2354), preventinggh label create --repo ""from being attempted.
Concerns:
- None blocking. The PR URL is trusted because it originates from
appState.links(for:), which is populated from provider responses, not user free-text.
Code Quality
- ADR 0005 decoupling is preserved:
canAddMergeLabelResolverinjection inAppDelegate.swift:848–854keepsCrowUIfree ofCrowProvider(same pattern ascanSetProjectStatusResolver). isAddingMergeLabellifecycle is correct — set/cleared viadefer, and cleaned up inSessionService.deleteSession(SessionService.swift:978) alongsideisMarkingInReview. No leak when a session is deleted mid-flight.repoSlug(fromPRURL:)test suite (IssueTrackerReconcileTests.swift:220–251) covers GitHub, GitLab nested groups, query/fragment suffixes, http scheme, missing scheme, and empty input. Good signal that future URL-shape regressions get caught.addMergeLabelButtonis correctly self-gating (hidden when no PR or capability missing) and present in both the sharedsessionContextMenuand the inline Reviews-section menu, matching the spec.- Consistent with
markInReview's style: capability guard →deferin-flight flag →do/catchwith truncated error logging. The simplifiedcatch(after the previous round's "drop dead catch" review) is correct —ensureMergeLabelandaddMergeLabeldon't surfaceProviderError.insufficientScopeor.unimplementedat this layer.
Consider (green, non-blocking)
- Success uses
NSLog, failure usesprint(\"[IssueTracker] ...\"). The user gets no UI feedback either way — they have to check the PR to confirm. MatchesmarkInReview, so consistent rather than worse. A future pass could surface a toast/HUD or a.failureicon for both flows, but out of scope here. repoSlug(fromPRURL:)accepts a single-segment path (e.g.https://github.com/ownerreturns\"owner\"). Downstreamghrejects that cleanly, so it's not a defect — just noting that the parser is permissive on shape. Not worth tightening.
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, 2 Green] findings.
Closes #502
What
Adds a right-click context-menu item on sidebar session rows — "Add label crow:merge to PR" — shown only when the session has a PR link and the active code provider supports the auto-merge label (GitHub). Selecting it ensures the
crow:mergelabel exists in the repo, then adds it to the PR so the existing auto-merge watcher can pick it up — without leaving Crow.How
CodeBackend.addMergeLabel(prURL:)— new protocol method (GitHub:gh pr edit <url> --add-label crow:merge, run with direct argv in$TMPDIRlikeenableAutoMerge). A protocol-extension default throws.unimplemented, so GitLab/Corveil backends need no changes.AppState—onAddMergeLabelclosure,isAddingMergeLabelin-flight flag (cleared on delete), andcanAddMergeLabel(for:)backed by an injectedcanAddMergeLabelResolver— the same ADR 0005 decoupling used bycanSetProjectStatus, keeping CrowUI free of a CrowProvider dependency.IssueTracker.addMergeLabel(sessionID:)— mirrorsmarkInReview's capability gate /deferin-flight flag / log + scope-warning error handling. NewrepoSlug(fromPRURL:)parsesowner/repofrom a PR web URL (distinct from the git-remoteextractSlug).AppDelegate— wires the resolver + closure next toonMarkInReview.SessionListView— self-gatingaddMergeLabelButtonincluded in the shared row menu (Jobs/Active/In Review/Completed) and the inline Reviews menu; hidden (not disabled) when there's no PR or the capability is absent.Testing
make buildpasses.crow:mergeto the PR (verifiable viagh pr view <url> --json labels) and creates the repo label if missing.🤖 Generated with Claude Code