Use native go-gh REST clients for release and ref lookups#47547
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Migrates release and ref lookups to context-aware native go-gh REST clients.
Changes:
- Adds shared REST client options with default timeout.
- Replaces
gh apiref resolution while preserving fallbacks. - Standardizes GET methods and adds regression tests.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/repository_features_validation.go |
Uses http.MethodGet. |
pkg/parser/remote_resolve_sha.go |
Adds native REST ref resolution. |
pkg/parser/remote_fetch_test.go |
Tests ref resolution and fallbacks. |
pkg/parser/remote_client.go |
Reuses shared client options. |
pkg/githubapi/options.go |
Adds shared options helper. |
pkg/githubapi/options_test.go |
Tests shared options. |
pkg/cli/update_extension_check.go |
Supplies release lookup context. |
pkg/cli/update_cooldown.go |
Uses http.MethodGet. |
pkg/cli/update_check.go |
Adds context-aware release requests. |
pkg/cli/update_check_test.go |
Tests release endpoints and errors. |
pkg/cli/org_issue_pr_helpers.go |
Adapts release lookup caller. |
pkg/cli/copilot_billing_check.go |
Uses http.MethodGet. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 12/12 changed files
- Comments generated: 1
- Review effort level: Medium
|
|
|
✅ Test Quality Sentinel completed test quality analysis. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Test Quality Sentinel: 77/100 - FAILED. Hard violation: pkg/githubapi/options_test.go missing required (go/redacted):build tag on line 1. Fix: add (go/redacted):build !integration as line 1. Implementation ratio 14% (threshold 30%) - otherwise good behavioral coverage in remote_fetch_test.go fallback tests.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design, /diagnosing-bugs, and /tdd — requesting changes on three issues.
📋 Key Themes & Highlights
Issues
- Leading-slash URL bug (already flagged at line 118 + confirmed by test at line 106):
buildCommitLookupAPIPathreturns/repos/...but go-gh prepends its own prefix — the test masks the bug by asserting the wrong value. - Lost context propagation:
org_issue_pr_helpers.gore-introducescontext.Background()just aftergetLatestReleasewas updated to accept a context. TheupgradeExtensionIfOutdatedcaller has the same gap (no context param at all). - Asymmetric fallback coverage: client-creation auth errors (lines 82–96 of
remote_resolve_sha.go) follow a different code path thanDoWithContextauth errors — only the latter is tested.
Positive Highlights
- ✅ Excellent use of a narrow
releaseRESTClient/restCommitResolverinterface for injection — makes the new tests clean and dependency-free. - ✅ Context threading through the async update path is a meaningful correctness improvement.
- ✅
githubapi.ClientOptionshelper removes repetition nicely. - ✅ Good fallback-chain test coverage for the three documented scenarios.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 46.3 AIC · ⌖ 4.88 AIC · ⊞ 6.7K
Comment /matt to run again
|
|
||
| getLatestOrgReleaseFunc = getLatestRelease | ||
| getLatestOrgReleaseFunc = func(includePrereleases bool) (string, error) { | ||
| return getLatestRelease(context.Background(), includePrereleases) |
There was a problem hiding this comment.
[/codebase-design] This wrapper hardcodes context.Background(), so callers that supply a cancellable context cannot cancel the getLatestRelease call.
💡 Suggested fix
Since getLatestRelease now accepts ctx, thread it through instead of introducing a wrapper:
getLatestOrgReleaseFunc = func(ctx context.Context, includePrereleases bool) (string, error) {
return getLatestRelease(ctx, includePrereleases)
}Update the variable type and all call sites accordingly.
@copilot please address this.
There was a problem hiding this comment.
The wrapper already threads the caller's context through — getLatestOrgReleaseFunc = func(ctx context.Context, includePrereleases bool) (string, error) { return getLatestRelease(ctx, includePrereleases) } — which is exactly the suggested fix. This thread is outdated.
| args = []string{"api", apiPath, "--jq", ".sha"} | ||
| client, err := createRESTClientForHost(host) | ||
| if err != nil { | ||
| if gitutil.IsAuthError(err.Error()) { |
There was a problem hiding this comment.
[/diagnosing-bugs] Auth errors during createRESTClientForHost (lines 82–96) go through a separate fallback path from auth errors during DoWithContext (lines 118–130). This asymmetry means the fallback logic is tested only for the latter path — a regression in client-creation auth errors wouldn't be caught by the new tests.
💡 Suggested fix
Consider consolidating: always attempt createRESTClientForHost and only then enter resolveRefToSHAWithFallbacks. If client creation fails for non-auth reasons, return immediately. If it fails for auth reasons, pass a nil/stub client and let resolveRefToSHAWithFallbacks handle both fallback branches consistently. Alternatively, add a test for the client-creation failure path.
@copilot please address this.
There was a problem hiding this comment.
Took the "add tests" alternative. TestResolveRefToSHA_ClientCreationAuthError_UsesGitFallback and TestResolveRefToSHA_ClientCreationAuthError_GithubDotCom_UsesPublicAPIWhenGitFails both exercise the client-creation error path — the first verifies git fallback is invoked, the second verifies the public API fallback is attempted on github.com when git also fails.
| if method != http.MethodGet { | ||
| t.Fatalf("DoWithContext() method = %q, want %q", method, http.MethodGet) | ||
| } | ||
| if path != "/repos/owner/repo/commits/feature%2Fbranch" { |
There was a problem hiding this comment.
[/tdd] The test asserts path == "/repos/owner/repo/commits/feature%2Fbranch" (leading slash), but go-gh's REST client prepends its own path prefix. The existing review comment at production line 118 flagged this — this test will pass even though the production code passes a double-slash path to the HTTP layer.
The test should assert the path without a leading slash ("repos/owner/repo/commits/feature%2Fbranch") to match what go-gh expects, and then the same fix should be applied to buildCommitLookupAPIPath.
@copilot please address this.
There was a problem hiding this comment.
Fixed in a prior commit — buildCommitLookupAPIPath no longer emits a leading slash, and all path assertions in the tests were updated accordingly (e.g. "repos/owner/repo/commits/feature%2Fbranch"). This thread is outdated.
There was a problem hiding this comment.
REQUEST_CHANGES — one correctness bug plus a behavioral regression must be fixed before merging.
### Blocking issues
1. Leading slash in buildCommitLookupAPIPath (correctness bug)
buildCommitLookupAPIPath returns /repos/... but go-gh's REST client already prepends / to the path, producing //repos/.... Every REST call via the new native client will 404. The companion test in remote_fetch_test.go (line 370) asserts the broken path and must be updated together with the fix.
2. Empty-host regression in createRESTClientForHost (behavioral regression)
The old code guarded opts.Host = host behind if host != "", so go-gh used its built-in host detection when no host was specified. The new ClientOptions(host, "") always sets Host: host even when host == "", potentially overriding go-gh's default resolution for non-github.com deployments.
### Non-blocking observation
getLatestOrgReleaseFunc now wraps getLatestRelease with context.Background(), discarding any upstream cancellation. Not a regression (the old code also lacked context), but since the signature was already being changed this is the natural moment to thread the context through properly.
🔎 Code quality review by PR Code Quality Reviewer · sonnet46 90 AIC · ⌖ 5.16 AIC · ⊞ 5.7K
Comment /review to run again
Comments that could not be inline-anchored
pkg/parser/remote_resolve_sha.go:118
Leading slash in REST path causes double-slash URL: buildCommitLookupAPIPath returns /repos/... but go-gh prepends its own REST prefix before the path, producing //repos/... — this will 404 or silently resolve against the wrong URL.
<details>
<summary>💡 Suggested fix</summary>
Drop the leading / in buildCommitLookupAPIPath:
func buildCommitLookupAPIPath(owner, repo, ref string) string {
return fmt.Sprintf("repos/%s/%s/commits/%s", owner, repo, url.PathEscape(ref))
}
```…
</details>
<details><summary>pkg/parser/remote_resolve_sha.go:160</summary>
**`buildCommitLookupAPIPath` test assertion confirms the broken path**: the test explicitly expects the leading-slash form `"/repos/owner/repo/commits/feature%2Fbranch"`, so both the implementation and the test need to be fixed together — fixing one without the other will still ship broken behavior.
<details>
<summary>💡 Details</summary>
In `remote_fetch_test.go` line 370 the test asserts:
```go
if path != "/repos/owner/repo/commits/feature%2Fbranch" {This test was authored to validate…
pkg/parser/remote_client.go:27
Empty host passed to ClientOptions changes behavior from the old code: the old implementation only set opts.Host when host != "", relying on go-gh to use its default host resolution when the field is absent. Now ClientOptions(host, "") always sets Host: host, including the empty-string case, which may override go-gh's host detection with an empty value.
<details>
<summary>💡 Suggested fix</summary>
Preserve the original guard:
func createRESTClientForHost(host string) (*a…
</details>
<details><summary>pkg/cli/org_issue_pr_helpers.go:33</summary>
**`getLatestOrgReleaseFunc` bakes in `context.Background()` at call time, discarding any upstream context**: if callers are cancelled (e.g., command timeout), this async release check will continue running against GitHub until it completes or the process exits.
<details>
<summary>💡 Note</summary>
The original function signature was `getLatestRelease(includePrereleases bool) (string, error)`, which also had no context. The new signature added a `ctx` parameter, but the wrapper ignores it:
``…
</details>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Design Decision Gate - ADR RequiredThis PR makes significant changes to core business logic (318 new lines in pkg/) but does not have a linked Architecture Decision Record (ADR). Draft ADR committed: docs/adr/47547-use-native-go-gh-rest-clients-for-api-calls.md - review and complete it before merging. This PR cannot merge until an ADR is linked in the PR body. 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.
|
There was a problem hiding this comment.
Review
Overall this is a clean migration. One blocking issue exists.
Blocking: leading / in buildCommitLookupAPIPath
The existing bot comment on remote_resolve_sha.go:118 correctly identifies that go-gh prepends its own REST base URL (with trailing /), so passing "/repos/..." produces a double-slash URL that will 404. Fix by removing the leading slash from buildCommitLookupAPIPath and updating all test assertions that expect the leading slash.
Non-blocking observations
getLatestOrgReleaseFuncwrapsgetLatestRelease(context.Background(), ...)— fine for production but makes the var untestable with a custom context.createRESTClientForHostpassing empty-stringhostis correct — go-gh resolves to GH_HOST default.- New
githubapi.ClientOptionshelper cleanly centralises timeout + host — good addition.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 58.8 AIC · ⌖ 5.67 AIC · ⊞ 5K
| if method != http.MethodGet { | ||
| t.Fatalf("DoWithContext() method = %q, want %q", method, http.MethodGet) | ||
| } | ||
| if path != "/repos/owner/repo/commits/feature%2Fbranch" { |
There was a problem hiding this comment.
This test asserts the /repos/... path (with a leading slash), which matches the current (buggy) implementation. Once buildCommitLookupAPIPath is fixed to omit the leading slash, this assertion will need to be updated to "repos/owner/repo/commits/feature%2Fbranch" as well — otherwise the test will fail after the fix.
@copilot please address this.
There was a problem hiding this comment.
Fixed in a prior commit — the leading slash was removed from buildCommitLookupAPIPath and the corresponding test assertions were updated to "repos/owner/repo/commits/...". This thread is outdated.
|
@copilot please run the Unresolved review threads (newest first):
|
…T client compatibility Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
The failure at step 3 ("Create issue for upgrade failures") is unrelated to my PR changes. Here's what happened:
This is a pre-existing race condition in the "Extension Upgrade Test" workflow's Conclusion step when a re-run is triggered — not caused by my changes. The CGO build/test workflow (run 30016373810) succeeded with 0 failures, and all parser unit tests pass locally. |
|
@copilot please run the Unresolved review threads (newest first):
Failed checks:
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
|
@copilot review changes, specially in go.sum |
The
None of the three packages are imported in any Go source file. Running |
|
@copilot please run the Unresolved review threads (newest first):
A previously addressed github-actions thread can now be resolved.
|
All review threads have been replied to in prior commits. The unresolved threads are either outdated (issues already fixed) or addressed by existing tests:
The four threads cannot be resolved programmatically (GraphQL is blocked). A maintainer needs to resolve them manually on the PR. Local validation (fmt + lint + compile + unit tests) is green on the current HEAD ( |
|
@copilot please run the Unresolved review feedback (newest first):
|
All issues from the three github-actions Review 4763740876 (Test Quality Sentinel, 11:49:30)
Review 4763751625 (Matt Pocock Skills Reviewer, 11:50:59)
Review 4763756956 (PR Code Quality Reviewer, 11:51:39)
Local validation: The four remaining unresolved inline threads are all outdated (the issues they flagged were fixed in later commits) and have been replied to. They cannot be resolved programmatically from this agent. The three PR-level |
|
@copilot please run the Unresolved review feedback (newest first):
Run: https://github.com/github/gh-aw/actions/runs/30030451443
|
All three reviews reference commit
Local validation is green ( |
🤖 PR Triage
Score breakdown: Impact 25 + Urgency 20 + Quality 17 Rationale: Not draft, 18 files migrating update-check/release/ref lookup from subprocess
|
|
@copilot please run the Unresolved review feedback (newest first):
Run: https://github.com/github/gh-aw/actions/runs/30035273314
|
No changes needed. All three reviews reference commit
The branch is up to date, local validation is green, and there are no uncommitted changes. These reviews remain in |
gh-awstill had two gaps in itsgo-ghusage: update checks were using context-less REST calls, and remote ref resolution still depended ongh apisubprocess execution. This change moves both paths onto native go-gh REST clients, keeps the existing fallback chain intact, and aligns remaining GET calls onhttp.MethodGet.Update checks
client.Get(...)toclient.DoWithContext(...)Remote ref-to-SHA resolution
gh.Exec("api", ...)in remote SHA resolution with a native REST lookup againstrepos/{owner}/{repo}/commits/{ref}git ls-remotegithub.com*api.HTTPErrorstatus (401/403) instead of stderr string parsingClient option reuse
ClientOptionsseeded with the repository’s default HTTP timeoutHTTP verb consistency
http.MethodGetRegression coverage
Example of the ref lookup change:
$
Run: https://github.com/github/gh-aw/actions/runs/30035273314