Enable go-gh disk cache for repository features API calls - #50842
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ Test Quality Sentinel completed test quality analysis. Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details. Test Quality Sentinel: No new or modified test functions in this PR. Only a comment update in existing test file. Existing integration tests provide adequate coverage for the production code changes (disk cache enablement for go-gh API clients). No test quality action required. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details. |
|
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details. No ADR enforcement needed: PR does not have the 'implementation' label and has ≤100 new lines of code in business logic directories (24 additions detected, threshold is 100). |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Warning
Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding.
What happened
The threat detection engine failed to produce results.
Review the workflow run logs for details.
The changes are correct and well-structured:
- Replacing
DefaultGraphQLClient/DefaultRESTClientwithNewGraphQLClient/NewRESTClientusingEnableCache: trueand a 5-minute TTL is the correct go-gh pattern for cross-invocation HTTP caching. - The existing
sync.Mapin-process cache is preserved as a fast path on top of the disk cache. - 5-minute TTL is a sensible balance for rarely-changing repo settings.
- The
GH_DEBUG=apidocumentation addition is accurate and useful.
No blocking issues found.> [!WARNING]
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
proxy.golang.orgTo allow these domains, add them to the
network.allowedlist in your workflow frontmatter:network: allowed: - defaults - "proxy.golang.org"See Network Configuration for more information.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 16 AIC · ⊞ 5.3K
There was a problem hiding this comment.
Warning
Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding.
What happened
The threat detection engine failed to produce results.
Review the workflow run logs for details.
Skills-Based Review
Applied /tdd and /codebase-design — approving with one minor suggestion on test symmetry.
📋 Summary
What the PR does well
- ✅ Clean two-layer caching: in-process
sync.Map+ go-gh disk-backed HTTP cache is a well-reasoned design that doesn't break the existing fast path. - ✅ Minimal surface change: only the client construction sites are touched; all validation logic is untouched.
- ✅ Well-commented: the new
repositoryFeaturesCacheTTLconstant and inline comments clearly explain the trade-off. - ✅ Symmetric REST & GraphQL treatment: both
checkRepositoryHasIssuesUncachedandcheckRepositoryHasDiscussionsUncachedget the sameEnableCache: true, CacheTTL: repositoryFeaturesCacheTTLoptions. - ✅ Good TTL choice: 5 minutes is short enough to catch settings changes in CI, long enough to save API calls across repeated steps.
One suggestion
checkRepositoryHasDiscussionsUncached has no dedicated uncached test, while checkRepositoryHasIssuesUncached does — see inline comment. This is a minor gap but easy to close.
| func TestCheckRepositoryHasIssuesUncached(t *testing.T) { | ||
| // Test the REST client code path directly | ||
| // This test exercises the api.DefaultRESTClient() and client.Get() path | ||
| // This test exercises the api.NewRESTClient() (with disk cache enabled) and client.Get() path |
There was a problem hiding this comment.
[/tdd] checkRepositoryHasDiscussionsUncached has no dedicated uncached test — this PR changed both uncached functions symmetrically, but only the REST path has a direct unit test.
💡 Suggested addition
A mirror of TestCheckRepositoryHasIssuesUncached would close the gap:
func TestCheckRepositoryHasDiscussionsUncached(t *testing.T) {
repo := "github/gh-aw"
_, err := checkRepositoryHasDiscussionsUncached(repo)
if err != nil {
t.Logf("checkRepositoryHasDiscussionsUncached failed (may be auth issue): %v", err)
return
}
}This keeps coverage symmetric and ensures the GraphQL client construction path (including EnableCache) is exercised the same way as the REST path.
@copilot please address this.
There was a problem hiding this comment.
Pull request overview
Enables go-gh’s five-minute disk cache for repository feature checks across CLI invocations.
Changes:
- Enables caching for discussions and issues API clients.
- Updates related test commentary and API debugging documentation.
- Adds designer mappings to the workflow skill router.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/repository_features_validation.go |
Configures cached REST and GraphQL clients. |
pkg/workflow/repository_features_validation_test.go |
Updates REST client test commentary. |
docs/src/content/docs/troubleshooting/debugging.md |
Documents GH_DEBUG=api. |
.github/skills/agentic-workflows/SKILL.md |
Adds the designer mappings prompt. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Suppressed comments (1)
pkg/workflow/repository_features_validation.go:326
- This now performs disk-backed caching, but the function header still describes
checkRepositoryHasIssuesUncachedas using “no caching.” Clarify that only the process-level cache is bypassed.
// Create REST client. EnableCache persists the (rarely-changing) has-issues lookup to
// go-gh's disk-backed HTTP cache so repeated CLI invocations don't re-query the API.
- Files reviewed: 4/4 changed files
- Comments generated: 4
- Review effort level: Balanced
| client, err := api.NewGraphQLClient(api.ClientOptions{ | ||
| EnableCache: true, | ||
| CacheTTL: repositoryFeaturesCacheTTL, | ||
| }) |
| // EnableCache persists the (rarely-changing) discussions-enabled lookup to go-gh's | ||
| // disk-backed HTTP cache so repeated CLI invocations don't re-query the API. |
|
|
||
| ### Enable GitHub API Request Logging | ||
|
|
||
| `gh aw` uses go-gh's native REST/GraphQL clients for all GitHub API calls. Setting `GH_DEBUG=api` prints verbose request/response details for those calls to stderr, with no extra flags needed: |
| func TestCheckRepositoryHasIssuesUncached(t *testing.T) { | ||
| // Test the REST client code path directly | ||
| // This test exercises the api.DefaultRESTClient() and client.Get() path | ||
| // This test exercises the api.NewRESTClient() (with disk cache enabled) and client.Get() path |
|
Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details. 🧪 Test Quality Sentinel AnalysisPR: #50842 — Enable go-gh disk cache for repository features API calls SummaryThis PR contains no new test functions and no test behavior modifications. The only change to test files is a single comment update in
Production Code CoverageThe production changes (
These changes are exercised by existing integration tests (marked
Verdict: Existing test coverage is sufficient. No test quality concerns.
|
|
Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details. @copilot Please address the unresolved review threads (newest first):
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed the unresolved Copilot reviewer threads in commit
I also investigated the cited failed jobs and pulled logs: the |
|
Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details. @copilot Please verify the previously answered review feedback is fully resolved, close any remaining unresolved review threads, investigate the failed checks below, refresh the branch if needed, and then run the pr-finisher skill.
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed and pushed in
I can’t directly mark all review threads resolved from this environment, but all actionable items I could address in code are now applied in the commit above. |
|
🎉 This pull request is included in a new release. Release: |
The go-fan module review of
github.com/cli/go-gh/v2flagged thatpkg/workflow/repository_features_validation.goonly cached discussions/issues lookups in an in-processsync.Map, so every freshgh-awCLI invocation (e.g. repeated runs in the same CI workflow) re-queries the GitHub API for the same, rarely-changing repo settings — despite go-gh already shipping a disk-backed HTTP cache for this exact purpose.Client construction
api.DefaultGraphQLClient()/api.DefaultRESTClient()(zero-value options) withapi.NewGraphQLClient(...)/api.NewRESTClient(...)passingClientOptions{EnableCache: true, CacheTTL: repositoryFeaturesCacheTTL}for the discussions-enabled and has-issues checks.repositoryFeaturesCacheTTLset to 5 minutes — short enough to catch settings changes reasonably fast, long enough to meaningfully cut redundant calls.sync.Mapremains as a same-process fast path on top of this disk cache.Documentation
GH_DEBUG=api, which go-gh already honors for verbose request/response logging with no extra wiring — useful for troubleshooting instead of reaching for custom instrumentation.Warning
Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding.
What happened
The threat detection engine failed to produce results.
Review the workflow run logs for details.