Auto-create workspace when assigned an issue with crow:auto label#211
Conversation
b9ba7ab to
f8126d9
Compare
dgershman
left a comment
There was a problem hiding this comment.
Code & Security Review
Critical Issues
None found.
Security Review
Strengths:
- Per-workspace opt-in (
autoCreateProjects) limits blast radius — repos must be explicitly listed before auto-create fires. - First-poll guard (
didCompleteInitialFetch) prevents stampeding on app launch with pre-existing assignments. - In-flight dedup set (
autoCreateInFlight) prevents double-dispatches during the window between trigger and session registration. - Case-insensitive matching throughout (repo comparison, chip dedup, save-on-dismiss) is robust.
- No new shell commands, network calls, or credential handling introduced — reuses the existing
onWorkOnIssueflow andIssueTrackerplumbing.
Concerns:
- None material. The auto-create path delegates entirely to the existing
onWorkOnIssuecallback (which types/crow-workspace {url}into the Manager terminal), so there's no new surface area for injection or privilege escalation.
Code Quality
Well done:
- Custom
init(from:)decoder withdecodeIfPresentforautoCreateProjectsensures backward compatibility with legacy configs missing the key. - Two new tests cover round-trip encoding and legacy-decode edge case.
FlowLayoutis a clean, self-containedLayoutimplementation for chip wrapping — no external dependencies.notifyAutoWorkspaceCreatedcorrectly respectsglobalMuteandsystemNotificationsEnabledgates, consistent with the existingnotifyReviewRequestpattern.- The
detectAutoCreateCandidatespurge ofautoCreateInFlightusingactiveSessionskeeps the set from growing unbounded.
Minor observations (non-blocking):
notifyAutoWorkspaceCreated(NotificationManager.swift:131) doesn't check a per-eventNotificationEventconfig (sound, enabled toggle) likenotifyReviewRequestdoes — it only checksglobalMuteandsystemNotificationsEnabled. This means users can't disable just the auto-create notification via the per-event settings UI. This is fine for a v1 since there's noNotificationEventcase for it yet, but worth noting if granular control is desired later.activeSession(for:)matches onticketURLagainstactiveSessions(status ==.active). If a session is.pausedor.inReview, it won't dedup. This seems intentional (you'd want a new workspace if the old one was paused), but worth confirming the expected UX for edge cases where a user pauses a session and then gets re-assigned to the same issue.
Summary Table
| Priority | Issue |
|---|---|
| 🟢 | Consider adding a NotificationEvent case for auto-create later for per-event toggle control |
| 🟢 | Confirm dedup behavior for paused/inReview sessions is intentional |
Recommendation: Approve — clean, well-guarded feature with good backward compatibility, proper dedup, and no security concerns. The first-fetch guard and in-flight set are solid defensive measures against the stampede/double-dispatch scenarios called out in the PR description.
dgershman
left a comment
There was a problem hiding this comment.
Code & Security Review (Second Pass)
Critical Issues
None found.
Security Review
Strengths:
- Per-workspace opt-in (
autoCreateProjects) limits blast radius — repos must be explicitly listed. - Reuses the existing
onWorkOnIssuecallback chain (types/crow-workspace {url}into the Manager terminal) — no new shell commands, network calls, or credential handling introduced. - No new user input is passed to shell commands; issue URLs flow through the same trusted path as the manual "Work on" button.
- Case-insensitive matching throughout (repo comparison, chip dedup, save-on-dismiss) prevents case-mismatch bypasses.
Concerns:
- None material. The attack surface is unchanged — all new code paths terminate at the same
TerminalManager.shared.send()call that the manual "Work on" flow uses.
Code Quality
Model layer (AppConfig.swift):
- Custom
init(from:)decoder withdecodeIfPresentforautoCreateProjectsensures backward compatibility with legacy configs. Follows the same pattern as other optional fields (provider,cli,host,alwaysInclude). CodingKeysenum explicitly lists all keys, which is correct since the custom decoder skips synthesized conformance.- Tests cover both round-trip and legacy-decode-without-key scenarios.
Tracker layer (IssueTracker.swift):
- Three-layer dedup is solid: (1)
didCompleteInitialFetchskips the first poll, (2)autoCreateInFlightprevents re-dispatch during the session-creation window, (3)activeSession(for:)skips issues already owned by an active session. autoCreateInFlightis pruned each cycle againstactiveSessions, so it doesn't grow unbounded.detectAutoCreateCandidatesruns synchronously on@MainActorafterappState.assignedIssuesis updated — no race between state update and candidate detection.didCompleteInitialFetchis set afterdetectAutoCreateCandidatesruns on the first cycle, which is correct — the first poll populates the baseline, and only subsequent polls trigger auto-create.
UI layer (WorkspaceFormView.swift):
- Save-on-dismiss flushing of the pending draft (lines 124-129) is a nice UX touch — prevents losing a typed-but-unconfirmed entry.
FlowLayoutis a clean, self-containedLayoutimplementation. ThesizeThatFits/placeSubviewscontract is correctly implemented.- Suggestion dropdown uses
autoCreateFocusedto show/hide, and filters against already-added repos with case-insensitive comparison.
Notification layer (NotificationManager.swift):
notifyAutoWorkspaceCreatedrespectsglobalMuteandsystemNotificationsEnabled, consistent with the existing notification pattern.
Yellow Items (Non-blocking)
1. Paused/inReview session dedup gap:
activeSession(for:) (AppState.swift:322) only checks activeSessions (status == .active). A session in .paused or .inReview status with the same ticketURL won't prevent re-triggering. Scenario: user pauses a session → next poll sees the issue → autoCreateInFlight was already purged (session went active then paused) → activeSession(for:) returns nil → duplicate workspace created. If this is intentional ("paused means abandoned, re-trigger is fine"), no action needed. If not, consider widening the check to include .paused and .inReview statuses.
2. Multi-issue burst on single poll:
If 5 issues match in one poll cycle, 5 separate /crow-workspace {url} commands are sent to the Manager terminal in rapid succession. The existing onBatchWorkOnIssues path (which uses /crow-batch-workspace) isn't used here. For v1 this is fine since having many repos opted in + many new assignments in a single 60s window is uncommon, but batching would be more robust.
3. No per-event notification toggle:
notifyAutoWorkspaceCreated bypasses the NotificationEvent-based config system (no .autoWorkspaceCreated case exists). Users can't selectively mute just auto-create notifications without muting everything via globalMute. Fine for v1.
Summary Table
| Priority | Issue |
|---|---|
| 🟡 | Paused/inReview sessions not checked in dedup — could re-trigger for same issue |
| 🟡 | Multiple matches in one poll cycle dispatched individually, not batched |
| 🟢 | No per-event notification toggle for auto-create |
Tests: 115 CrowCore tests pass, including 2 new tests for autoCreateProjects round-trip and legacy decode.
Recommendation: Approve — well-structured feature with solid defensive measures. The yellow items are edge cases that won't bite in normal usage. The first-fetch guard, in-flight dedup, and active-session check form a robust three-layer defense against the stampede and double-dispatch scenarios.
When IssueTracker sees an open assigned issue carrying the `crow:auto` label it fires the existing onWorkOnIssue flow (dispatches /crow-workspace to the Manager terminal) and posts a system notification, then asynchronously strips the label so the trigger is one-shot and visible across machines. The label is the sole opt-in — no per-repo or workspace-level config. Removing the label after spawn gives durable cross-machine dedup; if the API call fails we fall back to in-memory autoCreateInFlight + activeSession ticketURL matching to suppress duplicate spawns. If a labeled issue already has an active session (work picked up elsewhere), we strip the stale label without re-dispatching. Closes #194 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
f8126d9 to
f8bb3b5
Compare
Summary
When IssueTracker sees an open assigned issue carrying the
crow:autolabel it fires the existingonWorkOnIssueflow (dispatches/crow-workspace {url}to the Manager terminal) and posts a system notification, then asynchronously strips the label so the trigger is one-shot and visible across machines.The label is the sole opt-in — no per-repo or workspace-level config. Removing the label after spawn gives durable cross-machine dedup; if the API call fails we fall back to in-memory
autoCreateInFlight+ active-sessionticketURLmatching to suppress duplicate spawns within the session.If a labeled issue already has an active session (work picked up elsewhere), the stale label is stripped without re-dispatching.
Closes #194
Why label-based instead of per-repo config
Discussed in PR thread: a per-repo CSV trigger fights too many real edge cases (PR already exists, you self-assigned for visibility, bulk triage, multi-assignee races, re-assignment after completion). A per-issue label inverts the relationship — explicit user intent per ticket — and removing the label after spawn makes "labeled = work pending" a durable invariant across machines and restarts.
Test plan
make build— cleanswift test --package-path Packages/CrowCore— 113 tests passcrow:autolabel, verify within ~60s: notification appears, Manager tab activates,/crow-workspace {url}is typed into the Manager terminal, thecrow:autolabel is gone from the issuecrow:autolabel without spawning works fine on the next poll (no error)glab issue update <n> --repo <r> --unlabel crow:auto) works on a GitLab-backed workspace🤖 Generated with Claude Code