Add github.com org fallback for add-wizard shorthand on GHE and improve cross-host error guidance#34526
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
|
🧪 Test Quality Sentinel completed test quality analysis. |
|
✅ PR Code Quality Reviewer completed the code quality review. |
There was a problem hiding this comment.
Pull request overview
This PR improves gh aw add-wizard behavior on GitHub Enterprise (GHE) by ensuring shorthand workflow specs for certain well-known public organizations resolve against github.com, and by adding more actionable guidance when ref→SHA resolution fails on non-github.com hosts.
Changes:
- Route shorthand specs for
github/*,githubnext/*, andmicrosoft/*tohttps://github.comeven when an enterprise host is configured (applied in both CLI and parser paths). - Add a 404-specific cross-host hint when commit-SHA resolution fails on non-
github.comhosts, suggesting a fullhttps://github.com/...URL. - Update
add-wizardhelp text and add/extend tests for host fallback and the new hint behavior.
Show a summary per file
| File | Description |
|---|---|
| pkg/parser/github.go | Expands host selection to force github.com for allowlisted org owners. |
| pkg/parser/github_host_test.go | Adds tests covering org-based public-host fallback in parser host resolution. |
| pkg/cli/github.go | Updates CLI-side repo host selection to use owner-based allowlist. |
| pkg/cli/github_test.go | Extends CLI host-resolution tests for additional allowlisted orgs. |
| pkg/cli/fetch.go | Adds host-aware 404 hinting to SHA resolution failures. |
| pkg/cli/fetch_host_hint_test.go | Introduces targeted tests for the new 404 hint generation and path normalization. |
| pkg/cli/add_wizard_command.go | Updates add-wizard long help with GHE shorthand guidance. |
| pkg/cli/add_wizard_command_test.go | Verifies the new help guidance text is present. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 8/8 changed files
- Comments generated: 2
| resolvedHost := strings.TrimSpace(explicitHost) | ||
| if resolvedHost == "" { | ||
| resolvedHost = strings.TrimPrefix(strings.TrimPrefix(getGitHubHost(), "https://"), "http://") | ||
| } | ||
| if resolvedHost == "" || resolvedHost == "github.com" { |
| Note: In GitHub Enterprise repos, shorthand specs resolve on your enterprise host. | ||
| Use full https://github.com/... URLs when sourcing public github.com workflows. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Solid improvement for GHE users — the org-level host routing and actionable error hint are both well-motivated and well-tested. Two non-blocking observations above worth a quick look before merge.
Review themes
What works well
- Replacing the hard-coded repo allowlist with org-level routing is cleaner and more extensible
hostResolutionHintForNotFoundis properly isolated, testable, and handles edge cases (nil error, non-404, path normalization)- Test coverage is thorough and covers the key branches
- Help text update is clear and actionable
Observations (non-blocking)
len(parts) > 0guard inpkg/cli/github.go— always true afterstrings.SplitN; the outerifadds noise without protection- Dual suggestion in error message — when the GHE host hint is active, the SHA-based
retryCommandis also shown; since both resolve on the wrong host, the SHA retry is misleading in that context
🔎 Code quality review by PR Code Quality Reviewer · sonnet46 1.1M
| githubLog.Print("Using public GitHub host for githubnext/agentics repository") | ||
| return string(constants.PublicGitHubHost) | ||
| parts := strings.SplitN(repo, "/", 2) | ||
| if len(parts) > 0 { |
There was a problem hiding this comment.
len(parts) > 0 guard is always true: strings.SplitN always returns at least one element (even for an empty input), so this condition never filters anything — it misleads readers into thinking an empty-slice case is possible.
💡 Suggested fix
Drop the outer if and rely on the switch directly:
func getGitHubHostForRepo(repo string) string {
parts := strings.SplitN(repo, "/", 2)
switch parts[0] {
case "github", "githubnext", "microsoft":
githubLog.Printf("Using public GitHub host for %s organization repository", parts[0])
return string(constants.PublicGitHubHost)
}
// For all other repositories, use the configured GitHub host
...
}parts[0] is always valid (at worst an empty string, which the switch default handles). Removing the spurious guard makes the intent clearer.
| retryCommand := fmt.Sprintf("gh aw add %s/%s/%s@<40-char-sha>", owner, repo, workflowPath) | ||
| if hostHint, ok := hostResolutionHintForNotFound(owner, repo, ref, workflowPath, host, err); ok { | ||
| return "", fmt.Errorf( | ||
| "failed to resolve '%s' to commit SHA for '%s/%s'. Expected the GitHub API to return a commit SHA for the ref. Try: %s. %s: %w", |
There was a problem hiding this comment.
Dual-hint confusion when GHE host hint applies: when the hint is active (GHE + 404), the error message includes both the SHA-based retryCommand and the hostHint. The SHA-based suggestion (gh aw add owner/repo/path@<40-char-sha>) will also fail on the wrong host, so showing it alongside the host-switch hint sends mixed signals to the user.
💡 Suggested fix
When the host hint applies, omit or de-emphasize the SHA retry, since the root cause is host selection, not ref resolution:
if hostHint, ok := hostResolutionHintForNotFound(owner, repo, ref, workflowPath, host, err); ok {
return "", fmt.Errorf(
"failed to resolve '%s' to commit SHA for '%s/%s' on %s. %s: %w",
ref, owner, repo, resolvedHost, hostHint, err,
)
}
// SHA retry is only useful when the host is correct
return "", fmt.Errorf(
"failed to resolve '%s' to commit SHA for '%s/%s'. Try: %s: %w",
ref, owner, repo, retryCommand, err,
)This avoids suggesting a SHA-based retry that is equally likely to 404 on the wrong host.
🧪 Test Quality Sentinel Report✅ Test Quality Score: 80/100 — Excellent
📊 Metrics & Test Classification (4 tests analyzed)
Test Classification Details
Language SupportTests analyzed:
|
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
✅ Test Quality Sentinel: 80/100. Test quality is excellent — 0% of new tests are implementation tests (threshold: 30%). All 4 test functions are behavioral design tests with full edge-case coverage. Two minor non-blocking style notes posted in the review comment above.
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (201 new lines in 📄 Draft ADR committed:
📋 What to do next
Once an ADR is linked in the PR body, this gate will re-run and verify the implementation matches the decision. ❓ Why ADRs Matter
ADRs create a searchable, permanent record of why the codebase looks the way it does. Future contributors (and your future self) will thank you — particularly for decisions like the public-org allowlist in this PR, where the membership criteria need to be discoverable when the next org is added. 📋 Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in References: §26372124288
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnose and /tdd — requesting changes on two correctness issues.
📋 Key Themes & Highlights
Issues to Address
- Wrong command format in the user-facing hint (
fetch.go:232) — The hint tells users to rungh aw add-wizard <blob URL>, butadd-wizarddoesn't accept blob tree URLs. This replaces one misleading error with another. - Overly broad
microsoftorg fallback (github.go:26) — Allmicrosoft/*repos are routed to github.com on GHE without justification. An enterprise customer could legitimately hostmicrosoft/*repos on their GHE instance. - Fragile 404 detection (
fetch.go:219) —strings.Contains(err, "not found")matches unrelated local error messages; thehttp 404check alone is sufficient and precise. - Test style inconsistency — New test files use raw
t.Fatal/t.Errorfrather thantestify/assertused everywhere else in the suite.
Positive Highlights
- ✅ Clean generalisation from hard-coded repo names to an org-level switch — the right shape of fix
- ✅
hostResolutionHintForNotFoundis well-isolated and independently testable - ✅ Good path-normalisation test (leading-slash case)
- ✅ Both CLI and parser layers updated consistently, avoiding split behavior
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 1.3M
| } | ||
|
|
||
| errorText := strings.ToLower(err.Error()) | ||
| if !strings.Contains(errorText, "not found") && !strings.Contains(errorText, "http 404") { |
There was a problem hiding this comment.
[/diagnose] strings.Contains(errorText, "not found") is a broad heuristic that will also match non-404 error messages containing the phrase "not found" (e.g. "reference not found", "workflow file not found in local path").
💡 Suggestion
Match on the HTTP status code substring more precisely, or use a sentinel error type/interface if the GitHub client returns one:
// More specific: match on the HTTP 404 status code string
if !strings.Contains(errorText, "http 404") && !strings.Contains(errorText, "status 404") {
return "", false
}Alternatively, check if the error wraps an *github.ErrorResponse with StatusCode == 404. This avoids false positives from local "not found" messages.
|
|
|
|
|
@copilot review all comments, merge main recompile fix conflicts |
…ard-confusing-wf-resolution Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…-text test Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done. I reviewed and updated add/update as well. Changes are in |
|
@copilot all the pull request reviews from github-action bot |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Handled the github-actions bot review items in |
|
@copilot merge main recompile fix conflicts |
…gh-aw-add-wizard-confusing-wf-resolution # Please enter a commit message to explain why this merge is necessary, # especially if it merges an updated upstream into a topic branch. # # Lines starting with '#' will be ignored, and an empty message aborts # the commit. Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done in |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Addressed in I investigated the failing job logs and fixed the Go testifylint violations in the touched tests ( Validation run locally:
|
gh aw add-wizardshorthand specs were being resolved on the active GHE host, which caused misleading 404/ref-resolution failures for public workflow sources that exist on github.com. This change adds explicit cross-host fallback for known public orgs and improves user-facing guidance when shorthand resolution fails on enterprise hosts.Host resolution: prefer github.com for known public workflow orgs
github/*,githubnext/*, andmicrosoft/*tohttps://github.comeven in GHE contexts.Error UX: make cross-host failure actionable
https://github.com/...URL (including a concrete command example).add-wizard guidance update
add-wizardlong help text to clearly state that in GHE repos, shorthand resolves on the enterprise host and public github.com sources should be passed as full URLs.Coverage updates
github,githubnext,microsoft)add-wizardhelp messaging