Jobs: don't silently drop bare-org alwaysInclude specs (fix empty repo dropdown)#517
Merged
Merged
Conversation
The Jobs form's repo dropdown is populated by expanding a workspace's alwaysInclude specs via ProviderManager.reposForSpecs. A bare org name (e.g. "securityscorecard", no "/") classified as .invalid and was silently dropped, leaving an empty dropdown with a generic "No repos found" message — so SecurityScorecard's workspace couldn't create jobs. Rather than auto-coercing bare tokens to owner/* (which could mask a malformed owner/repo typo), keep them .invalid but surface them: - New WorkspaceRepoListing (CrowCore): resolved repos + invalidSpecs, each with an optional suggested fix. - reposForSpecs now returns the listing, collecting invalid specs (with a "did you mean owner/*" suggestion for bare org names) instead of silently skipping them. Whitespace-only specs are still dropped. - JobFormView distinguishes three empty states: nothing configured / invalid spec(s) (with the suggestion) / valid specs that returned nothing (e.g. gh/glab not authenticated). - AppState, AppDelegate (incl. cache), and SettingsView updated to the richer return type. - Tests: bare org token is now surfaced (not dropped) with a suggestion; added reposForSpecs + suggestion(forInvalidSpec:) coverage. 🐦⬛ Generated with Claude Code, orchestrated by Crow Co-Authored-By: Claude <noreply@anthropic.com> Crow-Session: 01959F97-2B60-4E9D-A00A-25399AFD2B06
dgershman
approved these changes
Jun 15, 2026
dgershman
left a comment
Collaborator
There was a problem hiding this comment.
Code & Security Review
Critical Issues
None.
Security Review
Strengths:
- User-provided spec strings flow only into
SwiftUI.Text(no markup interpretation) and intoProviderManagerclassification — they never reachshell()as concatenated arguments. Invalid specs are short-circuited before any provider CLI call. - The
host/providercache key (AppDelegate.swift:916-918) includes provider + host alongsidealwaysInclude, so flipping provider/host within the TTL window won't return stale wrong-provider slugs. (Pre-existing — preserved here under the new listing type.) - New
WorkspaceRepoListingisSendable + Equatable;static let emptyis safe for the actor-boundary closure default.
Concerns:
- None.
Code Quality
- Clean refactor:
[String]→WorkspaceRepoListingis threaded through every call site (AppState,AppDelegate,SettingsView,JobFormView) consistently, with.emptyused as the closure default everywhere. No half-converted spots. classifySpecis intentionally unchanged — bare tokens stay.invalidrather than being coerced toowner/*, which (as the PR notes) would maskowner/repotypos. Good call; the typo-vs-bare-org distinction matters.- The whitespace-only skip at
ProviderManager.swift:252-253prevents"'' isn't a valid spec"noise — and thesuggestionForInvalidSpectests pin that edge case down. - The
loadGenerationstale-result guard atJobFormView.swift:177-186is preserved and applies cleanly to bothreposandinvalidSpecs. - Test coverage on the new behavior is thorough: bare-org repro (
securityscorecard), whitespace-only drop, mixed valid+invalid, suggestion edge cases (trailing*,/*, empty). - Tests pass locally (
swift test --filter ProviderManagerTests→ 52/52). CrowCore builds clean.
Consider (non-blocking)
- The empty-state caption (
JobFormView.swift:217) only renders whenrepoOptions.isEmpty. If a workspace'salwaysIncludeis mixed (e.g.[\"securityscorecard\", \"myorg/myrepo\"]), the picker showsmyrepoand the bare-org warning is silently swallowed — the user gets a working picker but no signal that one spec is malformed. The PR description scopes the fix to the empty case, so this is intentional, but a follow-up could surface invalid specs as a small caption alongside the populated picker for the mixed case. invalidSpecsis anArray(not aSet), so a duplicate bare token inalwaysIncludewould render twice. Vanishingly unlikely in practice and arguably honest about what the user wrote — not worth changing.
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. Clean fix for CROW-516; explicit scope, thorough tests, type-safe refactor through the call chain.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #516
Problem
In Settings → Jobs, creating a job for an org whose workspace config has a bare org name in
alwaysInclude(e.g. SecurityScorecard's["securityscorecard"]) showed "No repos found" with no explanation — you couldn't pick a repo, so you couldn't create the job.The Jobs dropdown is populated by expanding
alwaysIncludeviaProviderManager.reposForSpecs→classifySpec. A bare token (no/, no*) classifies as.invalidand was silently skipped, so every entry got dropped and the single generic empty-state message couldn't explain why.Fix
Rather than auto-coercing bare tokens to
owner/*(which could mask a malformedowner/repotypo — those always contain a/), bare tokens stay.invalidbut are now surfaced with an actionable hint.WorkspaceRepoListing(CrowCore, new): resolvedrepos+invalidSpecs, each with an optional suggested fix.reposForSpecsreturns the listing, collecting invalid specs (with a "did you meanowner/*" suggestion for bare org names) instead of silently skipping. Whitespace-only entries are still dropped.classifySpecitself is unchanged.JobFormViewdistinguishes three empty states: nothing configured / invalid spec(s) (with the suggestion) / valid specs that returned nothing (e.g.gh/glabnot authenticated).AppState,AppDelegate(incl. its repo cache), andSettingsViewupdated to the richer return type.Tests
classifySpecBareNameIsInvalidkept (behavior unchanged) with a clarifying comment.reposForSpecsKeepsExplicitSlugsSortedAndDedupednow also asserts the bare token is surfaced with a suggestion.reposForSpecsSurfacesBareOrgTokenWithSuggestion(the CROW-516 repro),reposForSpecsSkipsWhitespaceOnlySpecs, andsuggestionForInvalidSpecGlobsBareOrgName.make appbuilds clean; CrowProvider/CrowCore/CrowUI/Crow test suites pass. (The only failing tests are pre-existingCrowGitRebaseTeststhat need a global git identity in the sandbox — unrelated to this change.)Notes / follow-ups
alwaysIncludeshould be["securityscorecard/*"]. Not changed here.gh repo list <org>/ on-disk fallback whenalwaysIncludeyields nothing — flagged in Jobs settings: repo dropdown shows 'no repos found' — bare org name in alwaysInclude silently dropped #516 as a larger design call.🤖 Generated with Claude Code