Skip to content

Add sidebar "Add label crow:merge to PR" context-menu action (CROW-502)#503

Merged
dhilgaertner merged 2 commits into
mainfrom
feature/crow-502-merge-label-context-menu
Jun 12, 2026
Merged

Add sidebar "Add label crow:merge to PR" context-menu action (CROW-502)#503
dhilgaertner merged 2 commits into
mainfrom
feature/crow-502-merge-label-context-menu

Conversation

@dhilgaertner

Copy link
Copy Markdown
Contributor

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:merge label 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 $TMPDIR like enableAutoMerge). A protocol-extension default throws .unimplemented, so GitLab/Corveil backends need no changes.
  • AppStateonAddMergeLabel closure, isAddingMergeLabel in-flight flag (cleared on delete), and canAddMergeLabel(for:) backed by an injected canAddMergeLabelResolver — the same ADR 0005 decoupling used by canSetProjectStatus, keeping CrowUI free of a CrowProvider dependency.
  • IssueTracker.addMergeLabel(sessionID:) — mirrors markInReview's capability gate / defer in-flight flag / log + scope-warning error handling. New repoSlug(fromPRURL:) parses owner/repo from a PR web URL (distinct from the git-remote extractSlug).
  • AppDelegate — wires the resolver + closure next to onMarkInReview.
  • SessionListView — self-gating addMergeLabelButton included 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 build passes.
  • Right-click a session with a GitHub PR → item appears; without a PR → absent; non-capable provider → absent.
  • Selecting it adds crow:merge to the PR (verifiable via gh pr view <url> --json labels) and creates the repo label if missing.

🤖 Generated with Claude Code

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
@dhilgaertner dhilgaertner requested a review from dgershman as a code owner June 12, 2026 16:47

@dgershman dgershman left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code & Security Review

Critical Issues

None.

Security Review

Strengths:

  • GitHubCodeBackend.addMergeLabel uses direct argv (no sh -c), eliminating shell interpolation around prURL, and pins cwd to $TMPDIR so gh cannot infer an unintended repo from a developer's cwd — same hardened pattern as enableAutoMerge.
  • Capability gating is layered correctly: AppDelegate resolver → AppState.canAddMergeLabel → UI hides the item; IssueTracker.addMergeLabel(sessionID:) re-guards on .autoMergeLabel; the protocol-extension default throws .unimplemented so a non-capable backend slipping through fails loudly instead of silently no-op'ing.
  • AppState.canAddMergeLabelResolver follows ADR 0005 — CrowUI stays free of a CrowProvider dependency.
  • SessionService.deleteSession cleans up isAddingMergeLabel, 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.


🐦‍⬛ Reviewed by Crow via Claude Code

…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
@dhilgaertner

Copy link
Copy Markdown
Contributor Author

Thanks for the review! Addressed in d9c31c4:

  • 🟡 Tests for repoSlug(fromPRURL:) — added IssueTrackerPRSlugTests in IssueTrackerReconcileTests.swift, mirroring the extractSlug battery: GitHub PR URL, GitLab /-/merge_requests shape (with nested groups), query string, fragment, http scheme, missing scheme (→ empty), and empty/whitespace input (→ empty). All pass.
  • 🟢 Empty repoSlug downgrade (ride-workspace: detect existing PRs and use their branch instead of creating new ones #3)addMergeLabel now guards on a non-empty slug and bails with a warning, rather than skipping ensureMergeLabel and still attempting gh pr edit --add-label (which would fail when the repo lacks the label). The contract of the menu item now matches its behavior.
  • 🟢 Unreachable insufficientScope catch (Integrate Claude Code hooks and event system for session activity tracking #2) — dropped. As you noted, the gh code backend only propagates ShellRunnerError.nonZeroExit, so the branch was dead; the generic catch covers all failures.
  • 🟢 User-visible success/failure feedback (Ghostty terminal: mouse position offset causes incorrect text selection and click targeting #4) — deferring per your "out of scope" note: addMergeLabel matches markInReview's log-only convention, and there's no existing per-row toast/status surface to reuse. Worth a follow-up that gives both actions visible feedback together.

@dhilgaertner dhilgaertner requested a review from dgershman June 12, 2026 16:56

@dgershman dgershman left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code & Security Review

Critical Issues

None.

Security Review

Strengths:

  • addMergeLabel(prURL:) runs gh pr edit <url> --add-label crow:merge as direct argv (no sh -c), so the PR URL cannot inject shell metacharacters. Matches the established enableAutoMerge pattern (GitHubCodeBackend.swift:255–263).
  • Sets cwd: NSTemporaryDirectory() so gh cannot 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), and IssueTracker.addMergeLabel re-checks backend.capabilities.contains(.autoMergeLabel) before issuing any command. A spoofed onAddMergeLabel invocation 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), preventing gh 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: canAddMergeLabelResolver injection in AppDelegate.swift:848–854 keeps CrowUI free of CrowProvider (same pattern as canSetProjectStatusResolver).
  • isAddingMergeLabel lifecycle is correct — set/cleared via defer, and cleaned up in SessionService.deleteSession (SessionService.swift:978) alongside isMarkingInReview. 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.
  • addMergeLabelButton is correctly self-gating (hidden when no PR or capability missing) and present in both the shared sessionContextMenu and the inline Reviews-section menu, matching the spec.
  • Consistent with markInReview's style: capability guard → defer in-flight flag → do/catch with truncated error logging. The simplified catch (after the previous round's "drop dead catch" review) is correct — ensureMergeLabel and addMergeLabel don't surface ProviderError.insufficientScope or .unimplemented at this layer.

Consider (green, non-blocking)

  • Success uses NSLog, failure uses print(\"[IssueTracker] ...\"). The user gets no UI feedback either way — they have to check the PR to confirm. Matches markInReview, so consistent rather than worse. A future pass could surface a toast/HUD or a .failure icon for both flows, but out of scope here.
  • repoSlug(fromPRURL:) accepts a single-segment path (e.g. https://github.com/owner returns \"owner\"). Downstream gh rejects 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.


🐦‍⬛ Reviewed by Crow via Claude Code

@dgershman dgershman added the crow:merge Crow auto-merge on green label Jun 12, 2026
@dhilgaertner dhilgaertner merged commit cf7400e into main Jun 12, 2026
2 checks passed
@dhilgaertner dhilgaertner deleted the feature/crow-502-merge-label-context-menu branch June 12, 2026 20:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

crow:merge Crow auto-merge on green

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Sidebar: right-click a session with a PR → "Add label crow:merge to PR" context-menu action

2 participants