Fix gh aw add parsing for long hyphenated repository names#37960
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
gh aw add parsing issue with hyphenated repository namesgh aw add parsing for long hyphenated repository names
|
🧠 Matt Pocock Skills Reviewer failed during the skills-based review. |
|
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #37960 does not have the 'implementation' label and has only 69 new lines in business logic directories (≤100 threshold, no custom .design-gate.yml config). |
|
🧪 Test Quality Sentinel completed test quality analysis. |
There was a problem hiding this comment.
Pull request overview
Fixes gh aw add spec parsing to correctly accept workflow and package specs when the repository name is longer than the 39-character owner/org limit (common with long hyphenated repo names), by splitting validation rules for owner/org vs repo.
Changes:
- Introduced a repo-specific validator (
IsValidGitHubRepositoryName) with a 100-character max length while keeping owner/org validation at 39. - Updated CLI parsing to validate
ownerandrepowith role-correct validators in workflow-spec and repository-package-spec parsing. - Added regression tests for the new repo-name validator and for parsing a workflow spec with a long hyphenated repo name.
Show a summary per file
| File | Description |
|---|---|
| pkg/parser/github_urls.go | Adds shared validator helper, owner/repo max-length constants, and a new repo-name validator. |
| pkg/parser/github_urls_test.go | Adds unit tests for IsValidGitHubRepositoryName. |
| pkg/cli/spec.go | Updates workflow URL/spec parsing to validate repo names with the repo-specific validator. |
| pkg/cli/add_wildcard_test.go | Adds a regression test for parsing a long hyphenated repo name in a direct workflow path spec. |
| pkg/cli/add_package_manifest.go | Updates repository-package spec detection to validate repo names with the repo-specific validator. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 5/5 changed files
- Comments generated: 3
| // IsValidGitHubIdentifier checks if a string is a valid GitHub user or organization identifier. | ||
| func IsValidGitHubIdentifier(s string) bool { | ||
| return isValidGitHubNameWithMaxLength(s, maxGitHubOwnerIdentifierLength) | ||
| } |
There was a problem hiding this comment.
Updated in e31d8ee. I corrected the parser API docs in pkg/parser/README.md to split owner/org vs repository validation and added README-backed spec coverage for IsValidGitHubRepositoryName in pkg/parser/spec_test.go.
| // Validate owner and repo | ||
| if !parser.IsValidGitHubIdentifier(owner) || !parser.IsValidGitHubIdentifier(repo) { | ||
| if !parser.IsValidGitHubIdentifier(owner) || !parser.IsValidGitHubRepositoryName(repo) { | ||
| return nil, fmt.Errorf("invalid GitHub URL: '%s/%s' does not look like a valid GitHub repository", owner, repo) | ||
| } |
There was a problem hiding this comment.
Added in e31d8ee. pkg/cli/spec_github_url_test.go now includes a parseGitHubURL regression case with a long hyphenated repo segment (>39 chars).
| slashParts := strings.Split(specWithoutVersion, "/") | ||
| if len(slashParts) < 2 || slashParts[0] == "" || slashParts[1] == "" { | ||
| return nil, false, nil | ||
| } | ||
| if !parser.IsValidGitHubIdentifier(slashParts[0]) || !parser.IsValidGitHubIdentifier(slashParts[1]) { | ||
| if !parser.IsValidGitHubIdentifier(slashParts[0]) || !parser.IsValidGitHubRepositoryName(slashParts[1]) { | ||
| return nil, false, nil |
There was a problem hiding this comment.
Handled in e31d8ee. TestParseRepositoryPackageSpec now includes a long hyphenated repo-name case (>39 chars) to keep repository-package parsing covered for this regression.
|
@copilot run pr-finisher skill |
🧪 Test Quality Sentinel Report✅ Test Quality Score: 100/100 — Excellent
📊 Metrics & Test Classification (2 tests analyzed)
Test Classification Details
Language SupportTests analyzed:
Verdict
📖 Understanding Test ClassificationsDesign Tests (High Value) verify what the system does:
Implementation Tests (Low Value) verify how the system does it:
Goal: Shift toward tests that describe the system's behavioral contract — the promises it makes to its users and collaborators.
|
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
gh aw addincorrectly rejected valid workflow specs when the repository segment exceeded owner-name length assumptions (e.g., long hyphenated repo names). This caused validowner/repo/path/to/workflow.mdinputs to fail early as “not a valid workflow path or repository package.”Problem scope
Parser changes
IsValidGitHubIdentifier(existing behavior)IsValidGitHubRepositoryName(new, repo-appropriate max length)CLI integration updates
pkg/cli/spec.go) to validate owner and repo with role-correct validators.pkg/cli/add_package_manifest.go) with the same owner/repo split.Regression coverage
.mdpath.