Skip to content

Consolidate repository and semantic-version utilities #54148

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
pelikhan merged 3 commits into from
Aug 20, 2026
Merged

Consolidate repository and semantic-version utilities #54148

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b91e1a5
Initial plan
Copilot Aug 20, 2026
7467d2c
Consolidate repo and semver helpers
Copilot Aug 20, 2026
007159b
Cover invalid host-qualified repo override
Copilot Aug 20, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 3 additions & 4 deletions pkg/cli/list_workflows_command.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import (
"github.com/github/gh-aw/pkg/constants"
"github.com/github/gh-aw/pkg/logger"
"github.com/github/gh-aw/pkg/parser"
"github.com/github/gh-aw/pkg/repoutil"
"github.com/github/gh-aw/pkg/stringutil"
"github.com/spf13/cobra"
)
Expand Down Expand Up @@ -251,12 +252,10 @@ func getRemoteWorkflowFiles(ctx context.Context, repoSpec, workflowPath string,
}

// Parse owner/repo
repoParts := strings.Split(repoPart, "/")
if len(repoParts) != 2 {
owner, repo, err := repoutil.SplitRepoSlug(repoPart)
if err != nil {
return nil, fmt.Errorf("invalid repository format: %s (expected owner/repo or owner/repo@ref)", repoSpec)
}
owner = repoParts[0]
repo = repoParts[1]

if verbose && !jsonOutput {
fmt.Fprintf(os.Stderr, "Fetching workflow files from %s/%s@%s (path: %s)\n", owner, repo, ref, workflowPath)
Expand Down
11 changes: 4 additions & 7 deletions pkg/cli/logs_run_processor.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import (
"github.com/github/gh-aw/pkg/constants"
"github.com/github/gh-aw/pkg/fileutil"
"github.com/github/gh-aw/pkg/parser"
"github.com/github/gh-aw/pkg/repoutil"
"github.com/github/gh-aw/pkg/stringutil"
"github.com/sourcegraph/conc/pool"
)
Expand Down Expand Up @@ -69,13 +70,9 @@ type runArtifactsConcurrentOptions struct {
func buildConcurrentDownloadParams(outputDir string, verbose bool, repoOverride string, artifactFilter []string, evalsOnly bool, artifactSets []string) concurrentRunDownloadParams {
var dlHost, dlOwner, dlRepo string
if repoOverride != "" {
// Accepted formats: "owner/repo" or "HOST/owner/repo".
parts := strings.SplitN(repoOverride, "/", 3)
switch len(parts) {
case 3: // HOST/owner/repo
dlHost, dlOwner, dlRepo = parts[0], parts[1], parts[2]
case 2: // owner/repo
dlOwner, dlRepo = parts[0], parts[1]
ownerRepo, host := repoutil.NormalizeRepoForAPI(repoOverride)
if owner, repo, err := repoutil.SplitRepoSlug(ownerRepo); err == nil {

@github-actions github-actions Bot Aug 20, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This change silently accepts malformed --repo overrides and proceeds with empty download coordinates, so the command can fall back to the current repository or fail much later with a misleading error instead of rejecting bad input at the boundary.

💡 Why this needs to fail fast

Before this refactor, owner/ still produced dlOwner="owner", which was dubious but at least preserved the user's input shape. The new code is worse: NormalizeRepoForAPI("owner/") returns owner/, SplitRepoSlug rejects it, and the error is discarded. That leaves dlHost, dlOwner, and dlRepo empty, so downstream code can behave as if no override was provided.

That is a correctness regression because an explicitly invalid override should not be treated like "use the default repo". It hides user mistakes and makes debugging much harder.

A minimal fix is to validate once and return an error when repoOverride is malformed, e.g.:

ownerRepo, host := repoutil.NormalizeRepoForAPI(repoOverride)
owner, repo, err := repoutil.SplitRepoSlug(ownerRepo)
if err != nil {
    return concurrentRunDownloadParams{}, fmt.Errorf("invalid repository override %q: expected owner/repo or HOST/owner/repo", repoOverride)
}
dlHost, dlOwner, dlRepo = host, owner, repo

If changing the helper signature is too invasive, add a dedicated validator before calling this helper and cover the invalid-input path with a failing test rather than asserting empty fields.

dlHost, dlOwner, dlRepo = host, owner, repo
Comment on lines +74 to +75
}
}
evalsArtifactRequested := isEvalsArtifactRequested(evalsOnly, artifactSets)
Expand Down
24 changes: 24 additions & 0 deletions pkg/cli/logs_run_processor_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,30 @@ import (
"github.com/stretchr/testify/require"
)

func TestBuildConcurrentDownloadParams_RepoOverride(t *testing.T) {
tests := []struct {
name string
repoOverride string
wantHost string
wantOwner string
wantRepo string
}{
{name: "owner and repo", repoOverride: "owner/repo", wantOwner: "owner", wantRepo: "repo"},
{name: "host owner and repo", repoOverride: "ghe.example/owner/repo", wantHost: "ghe.example", wantOwner: "owner", wantRepo: "repo"},
{name: "empty component", repoOverride: "owner/"},
{name: "empty host-qualified component", repoOverride: "ghe.example/owner/"},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
params := buildConcurrentDownloadParams("", false, tt.repoOverride, nil, false, nil)
assert.Equal(t, tt.wantHost, params.dlHost)
assert.Equal(t, tt.wantOwner, params.dlOwner)
assert.Equal(t, tt.wantRepo, params.dlRepo)
})
}
}

func TestRunHasEvals(t *testing.T) {
tests := []struct {
name string

@github-actions github-actions Bot Aug 20, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[/tdd] The buildConcurrentDownloadParams tests cover empty-component inputs but don't cover the repoOverride == "" path (no-op case). Since dlHost/dlOwner/dlRepo default to "", a test should confirm the zero values remain when repoOverride is empty string — otherwise a future change to the early-exit condition could silently break callers.

💡 Suggested test case 
{name: "empty override", repoOverride: "", wantHost: "", wantOwner: "", wantRepo: ""},

@copilot please address this.

Expand Down
6 changes: 3 additions & 3 deletions pkg/cli/preconditions.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import (

"github.com/github/gh-aw/pkg/console"
"github.com/github/gh-aw/pkg/logger"
"github.com/github/gh-aw/pkg/repoutil"
"github.com/github/gh-aw/pkg/workflow"
)

Expand Down Expand Up @@ -164,11 +165,10 @@ func parseJSON(data []byte, v any) error {
func checkUserPermissionsShared(repoSlug string, verbose bool) (bool, error) {
preconditionsLog.Print("Checking user permissions")

parts := strings.Split(repoSlug, "/")
if len(parts) != 2 {
owner, repo, err := repoutil.SplitRepoSlug(repoSlug)
if err != nil {
return false, fmt.Errorf("invalid repository format: %s", repoSlug)
}
owner, repo := parts[0], parts[1]

hasAccess, err := checkRepositoryAccess(owner, repo)
if err != nil {
Expand Down
7 changes: 3 additions & 4 deletions pkg/cli/project_command.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import (
"github.com/github/gh-aw/pkg/console"
"github.com/github/gh-aw/pkg/errorutil"
"github.com/github/gh-aw/pkg/logger"
"github.com/github/gh-aw/pkg/repoutil"
"github.com/github/gh-aw/pkg/workflow"
"github.com/spf13/cobra"
)
Expand Down Expand Up @@ -419,12 +420,10 @@ func linkProjectToRepo(ctx context.Context, projectId, repoSlug string, verbose
console.LogVerbose(verbose, "Linking project to repository: "+repoSlug)

// Parse repo slug
parts := strings.Split(repoSlug, "/")
if len(parts) != 2 {
repoOwner, repoName, err := repoutil.SplitRepoSlug(repoSlug)
if err != nil {
return fmt.Errorf("repository slug '%s' is not in owner/repo format. Expected '<owner>/<repo>'. Example: github/gh-aw", repoSlug)
}
repoOwner := parts[0]
repoName := parts[1]

// Get repository ID
repoIdQuery := `query($owner: String!, $name: String!) { repository(owner: $owner, name: $name) { id } }`
Expand Down
7 changes: 3 additions & 4 deletions pkg/cli/repo.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import (
"strings"

"github.com/github/gh-aw/pkg/logger"
"github.com/github/gh-aw/pkg/repoutil"
"github.com/github/gh-aw/pkg/syncutil"
"github.com/github/gh-aw/pkg/workflow"
)
Expand All @@ -27,8 +28,7 @@ func getCurrentRepoSlugUncached() (string, error) {
repoSlug := strings.TrimSpace(string(output))
if repoSlug != "" {
// Validate format (should be owner/repo)
parts := strings.Split(repoSlug, "/")
if len(parts) == 2 && parts[0] != "" && parts[1] != "" {
if _, _, err := repoutil.SplitRepoSlug(repoSlug); err == nil {
repoLog.Printf("Successfully got repository slug via gh CLI: %s", repoSlug)
return repoSlug, nil
}
Expand All @@ -55,8 +55,7 @@ func getCurrentRepoSlugUncached() (string, error) {
}

// Validate format (should be owner/repo)
parts := strings.Split(repoPath, "/")
if len(parts) != 2 || parts[0] == "" || parts[1] == "" {
if _, _, err := repoutil.SplitRepoSlug(repoPath); err != nil {
repoLog.Printf("Invalid repository format: %s", repoPath)
return "", fmt.Errorf("invalid repository format: %s. Expected format: owner/repo. Example: github/gh-aw", repoPath)
}
Expand Down
14 changes: 2 additions & 12 deletions pkg/cli/semver.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,6 @@
package cli

import (
"github.com/github/gh-aw/pkg/logger"
"github.com/github/gh-aw/pkg/semverutil"
)

var semverLog = logger.New("cli:semver")
import "github.com/github/gh-aw/pkg/semverutil"

// isSemanticVersionTag checks if a ref string looks like a semantic version tag
// Uses golang.org/x/mod/semver for proper semantic version validation
Expand All @@ -16,10 +11,5 @@ func isSemanticVersionTag(ref string) bool {
// parseVersion parses a semantic version string and returns a *semverutil.SemanticVersion.
// Uses golang.org/x/mod/semver for proper semantic version parsing.
func parseVersion(v string) *semverutil.SemanticVersion {
semverLog.Printf("Parsing semantic version: %s", v)
parsed := semverutil.ParseVersion(v)
if parsed == nil {
semverLog.Printf("Invalid semantic version: %s", v)
}
return parsed
return semverutil.ParseVersion(v)
}
1 change: 1 addition & 0 deletions pkg/cli/setup_command_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -222,6 +222,7 @@ func TestRunSetupRepositoryCheck_AcceptsCaseInsensitiveSlugMatch(t *testing.T) {

func TestValidateSetupRepositoryCheckOptions_RejectsEmptyRepoComponents(t *testing.T) {
tests := []SetupRepositoryCheckOptions{
{Repo: "/"},
{Repo: "/repo"},
{Repo: "owner/"},
}
Expand Down
7 changes: 3 additions & 4 deletions pkg/cli/setup_repository.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import (
"github.com/github/gh-aw/pkg/errorutil"
"github.com/github/gh-aw/pkg/gitutil"
"github.com/github/gh-aw/pkg/logger"
"github.com/github/gh-aw/pkg/repoutil"
"github.com/github/gh-aw/pkg/workflow"
)

Expand Down Expand Up @@ -352,10 +353,8 @@ func validateSetupRepositoryCheckOptions(opts SetupRepositoryCheckOptions) error
}

func isValidOwnerRepoSlug(repo string) bool {
parts := strings.Split(repo, "/")
return len(parts) == 2 &&
strings.TrimSpace(parts[0]) != "" &&
strings.TrimSpace(parts[1]) != ""
owner, name, err := repoutil.SplitRepoSlug(repo)
return err == nil && strings.TrimSpace(owner) != "" && strings.TrimSpace(name) != ""

@github-actions github-actions Bot Aug 20, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[/codebase-design] isValidOwnerRepoSlug applies strings.TrimSpace on top of SplitRepoSlug, creating a subtle inconsistency: SplitRepoSlug accepts " /repo" (non-empty, whitespace-only owner) but this function then rejects it. The canonical validator is no longer sufficient on its own.

💡 Suggestion

Either push whitespace trimming into SplitRepoSlug so it is truly canonical, or document the divergence with a comment:

func isValidOwnerRepoSlug(repo string) bool {
    // SplitRepoSlug does not trim whitespace; trim here to reject slugs like "  /repo".
    owner, name, err := repoutil.SplitRepoSlug(repo)
    return err == nil && strings.TrimSpace(owner) != "" && strings.TrimSpace(name) != ""
}

A test case {Repo: " /repo"} would make this explicit and prevent silent regression.

@copilot please address this.

}

func runSetupRepositoryCheckWithRuntime(opts SetupRepositoryCheckOptions, runtime setupRepositoryRuntime) error {
Expand Down
4 changes: 2 additions & 2 deletions pkg/cli/trial_repository.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import (
"github.com/github/gh-aw/pkg/fileutil"
"github.com/github/gh-aw/pkg/gitutil"
"github.com/github/gh-aw/pkg/logger"
"github.com/github/gh-aw/pkg/repoutil"
"github.com/github/gh-aw/pkg/workflow"
)

Expand Down Expand Up @@ -75,8 +76,7 @@ func trialRepositoryActionsSettingsURL(repoSlug string) string {
func ensureTrialRepository(repoSlug string, cloneRepoSlug string, forceDeleteHostRepo bool, dryRun bool, verbose bool) error {
trialRepoLog.Printf("Ensuring trial repository: %s (cloneRepo=%s, forceDelete=%v, dryRun=%v)", repoSlug, cloneRepoSlug, forceDeleteHostRepo, dryRun)

parts := strings.Split(repoSlug, "/")
if len(parts) != 2 || parts[0] == "" || parts[1] == "" {
if _, _, err := repoutil.SplitRepoSlug(repoSlug); err != nil {
return fmt.Errorf("invalid repository slug format: %s. Expected format: owner/repo. Example: github/gh-aw", repoSlug)
}

Expand Down
5 changes: 2 additions & 3 deletions pkg/workflow/awf_config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,7 @@ import (
"github.com/github/gh-aw/pkg/constants"
"github.com/github/gh-aw/pkg/jsonutil"
"github.com/github/gh-aw/pkg/logger"
"github.com/github/gh-aw/pkg/semverutil"
"github.com/github/gh-aw/pkg/setutil"
"github.com/github/gh-aw/pkg/syncutil"
"github.com/github/gh-aw/pkg/workflow/compilerenv"
Expand Down Expand Up @@ -455,9 +456,7 @@ func buildAWFConfigSchemaURL(firewallConfig *FirewallConfig) string {
return "https://github.com/github/gh-aw-firewall/releases/latest/download/awf-config.schema.json"
}
// Ensure version has the 'v' prefix required by GitHub release tag URLs.
if !strings.HasPrefix(version, "v") {
version = "v" + version
}
version = semverutil.EnsureVPrefix(version)
return fmt.Sprintf("https://github.com/github/gh-aw-firewall/releases/download/%s/awf-config.schema.json", version)
}

Expand Down
Loading