feat(workflow): cache repository owner-type API call once per compilation run#40258
Conversation
Initialize ownerTypeCache in NewCompiler so the owner-type API call
("Check repository owner type...") is made at most once per repo during
a single compilation run. Add unit tests that verify the cache is
consulted before making a network call and that it persists across
multiple workflow compilations on the same Compiler instance.
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…izes) Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR reduces redundant gh api /users/<owner> lookups during gh aw compile by ensuring the repository owner-type cache is initialized once per Compiler instance and then reused across workflow compilations in the same run.
Changes:
- Initialize
ownerTypeCacheeagerly inNewCompiler()to make the cache lifecycle explicit. - Remove the redundant nil-guard in
repositoryOwnerIsIndividualUser()now that the cache is always initialized byNewCompiler(). - Add focused unit tests to cover cache-hit behavior, malformed slugs, cache persistence across multiple compilations, and non-nil initialization.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/compiler_types.go | Eagerly initializes ownerTypeCache in NewCompiler() to support per-run caching. |
| pkg/workflow/permissions_compiler_validator.go | Removes now-unnecessary nil initialization inside repositoryOwnerIsIndividualUser(). |
| pkg/workflow/compiler_owner_type_cache_test.go | Adds unit tests validating cache hits, malformed slug handling, cross-compilation reuse, and initialization. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 3/3 changed files
- Comments generated: 0
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
|
✅ Test Quality Sentinel completed test quality analysis. |
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (122 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 MatterADRs create a searchable, permanent record of why the codebase looks the way it does. Even a small performance/caching change — like deduplicating per-owner API round-trips across a multi-workflow compile — is worth recording so future contributors understand the trade-off (cache lifetime, cached negative results) without re-deriving it from the diff. 📋 Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in
References: §27828047748
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /zoom-out, /tdd, and /improve-codebase-architecture — commenting with observations, no blocking issues.
📋 Key Themes & Highlights
Key Themes
- Nil-guard removal is load-bearing — the codebase has ~15 tests using
&Compiler{}struct literals that bypassNewCompiler(). None reachrepositoryOwnerIsIndividualUser()today, but removing the guard silently transfers that safety contract to every future test author. Consider restoring it or documenting the invariant on the field. - Sticky-false-on-error — a transient API failure caches
""and suppresses thecopilot-requeststip for the entire run. This is the correct fail-safe default but is undocumented. A one-line comment on the function would help future readers. - Goroutine-safety assumption — other API-backed caches in the package use
sync.Map; a note that this map is intentionally not goroutine-safe (single-threaded compilation) prevents misreads. - Commit type — the PR is tagged
featbut the user-facing behaviour is unchanged;perforrefactoris more accurate for changelog generation.
Positive Highlights
- ✅ Excellent test-to-production ratio: 120 lines of tests for a 3-line production change.
- ✅ Tests cover all four key axes: cache-hit, malformed slug, cross-compilation persistence, and
NewCompilernil-safety. - ✅ The test for
TestRepositoryOwnerIsIndividualUser_CacheInitializedByNewCompileris a good invariant test that will catch futureNewCompiler()regressions. - ✅ Eager initialization in
NewCompiler()makes the intent explicit and consistent withactionPinWarningsandpriorManifests.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 81.3 AIC · ⌖ 9.32 AIC · ⊞ 6.9K
| if c.ownerTypeCache == nil { | ||
| c.ownerTypeCache = make(map[string]string) | ||
| } | ||
| ownerType, cached := c.ownerTypeCache[owner] |
There was a problem hiding this comment.
[/zoom-out] The nil-guard removal is safe today because no existing caller reaches this path with a bare &Compiler{}, but the codebase has 15+ tests that construct &Compiler{} struct literals directly (e.g. frontmatter_extraction_security_test.go, unified_prompt_creation_test.go). If any future test covering copilot-permissions validation uses that pattern, the write at c.ownerTypeCache[owner] = "" (line 195 of the post-PR file) will panic.
💡 Suggested mitigation
The simplest fix is to restore the nil-guard as cheap defensive belt-and-suspenders:
if c.ownerTypeCache == nil {
c.ownerTypeCache = make(map[string]string)
}Alternatively, document the invariant on the struct field in compiler_types.go:
// ownerTypeCache is always initialized by NewCompiler().
// Bare &Compiler{} struct literals will panic on first write.
ownerTypeCache map[string]stringThe nil-guard was inexpensive; removing it transfers an implicit contract to every future test author.
| expectedResult: false, | ||
| }, | ||
| { | ||
| name: "cached empty string (API error) returns false", |
There was a problem hiding this comment.
[/tdd] The "cached empty string (API error) returns false" case correctly tests the cache-hit read path, but there is no coverage for the write side: when RunGH fails, "" is stored in the cache, and the copilot-requests tip is silently suppressed for the entire compilation run — even if the failure was transient (e.g. a momentary network blip).
This is likely the intended fail-safe behaviour (uncertain owner type → show tip), but the "sticky" aspect (no retry within a run) is easy to miss. A brief doc comment on the test or on repositoryOwnerIsIndividualUser noting this tradeoff explicitly would save a future reader from having to re-derive it.
💡 Suggested doc comment (in permissions_compiler_validator.go)
// Note: a failed API lookup caches "" so subsequent calls for the same owner
// also return false without retrying. This is intentional: fail-safe means
// "show the tip when uncertain" and avoids N retry round-trips per run.| actionPinWarnings: make(map[string]bool), // Initialize warning cache | ||
| priorManifests: make(map[string]*GHAWManifest), | ||
| gitRoot: gitRoot, // Auto-detected git root | ||
| ownerTypeCache: make(map[string]string), // Initialize owner-type cache (keyed by owner login) |
There was a problem hiding this comment.
[/improve-codebase-architecture] The rest of the package's API-backed caches (repositoryFeaturesCache, unquoteYAMLKeyCache) use sync.Map with explicit goroutine-safety documentation in repository_features_validation.go. This map is per-Compiler instance and compilation is currently sequential, so a plain map is correct today — but there is no comment stating that assumption.
Adding a brief note keeps future readers from having to reverse-engineer why the pattern differs:
ownerTypeCache: make(map[string]string), // Not goroutine-safe; Compiler is used sequentiallyOr, if compilation is ever parallelized, upgrade to sync.Map for consistency with the established pattern.
There was a problem hiding this comment.
REQUEST_CHANGES — one blocking issue, one minor cleanup
The caching goal is correct and the eager-initialization pattern is a genuine improvement. Two issues need addressing before merge.
🔴 Blocking — nil-guard removal creates latent panic path
The removed nil-guard was protecting against assignment to entry in nil map panics on Compiler instances created via &Compiler{} struct literals (two exist in production code; many more in tests). The PR's claim that the map is "always initialized" is only true for the NewCompiler() code path. Fix: restore the nil-guard, or enforce the invariant by converting the two &Compiler{} call sites to NewCompiler().
See inline comment on permissions_compiler_validator.go:189.
🟡 Minor — dead cache seedings in malformed-slug test mislead future readers
All slugs in TestRepositoryOwnerIsIndividualUser_MalformedSlug return before reaching the cache, so the ownerTypeCache pre-seedings at lines 77–78 are never read and the accompanying comment about bypass risk is misleading. Remove them.
See inline comment on compiler_owner_type_cache_test.go:77.
🔎 Code quality review by PR Code Quality Reviewer · 142.1 AIC · ⌖ 7.46 AIC · ⊞ 5.1K
| if c.ownerTypeCache == nil { | ||
| c.ownerTypeCache = make(map[string]string) | ||
| } | ||
| ownerType, cached := c.ownerTypeCache[owner] |
There was a problem hiding this comment.
Nil-guard removal is based on an incomplete invariant and leaves a latent nil-map write panic.
💡 Details and suggested fix
The PR claims the guard is "now-redundant" because NewCompiler() always initialises ownerTypeCache. That is only true for instances created through NewCompiler(). Two production call sites create &Compiler{} struct literals that bypass the constructor entirely:
safe_jobs.go:328 c := &Compiler{}
safe_outputs_config.go:725 c := &Compiler{}
Reading from a nil map (line 189) is safe in Go — returns zero value + ok=false. But the writes below are not:
c.ownerTypeCache[owner] = "" // line 195 — panics on nil map
c.ownerTypeCache[owner] = ownerType // line 199 — panics on nil mapNeither current call site reaches repositoryOwnerIsIndividualUser(), so there is no active bug today. But the nil-guard was the only thing standing between a future refactor and a assignment to entry in nil map panic. Restoring it is a one-liner with zero runtime cost:
if c.ownerTypeCache == nil {
c.ownerTypeCache = make(map[string]string)
}Alternatively: change the two &Compiler{} call sites to use NewCompiler() and document the invariant on the struct so future contributors can rely on it safely.
| c.SetRepositorySlug(tt.slug) | ||
| // Populate the cache with "User" for any conceivable owner — if the slug | ||
| // parsing were bypassed and a lookup happened, it would incorrectly return true. | ||
| c.ownerTypeCache["owneronly"] = "User" |
There was a problem hiding this comment.
Both cache seedings in this test are dead code — all malformed slugs return before the cache is consulted.
💡 Details
Every slug in this table triggers the early-return guard at line 184:
if !ok || owner == "" || repo == "" {
return false
}before execution ever reaches the cache lookup. Specifically:
| slug | strings.Cut result | guard fires on |
|---|---|---|
"" |
ok=false | !ok |
"owneronly" |
ok=false | !ok |
"/" |
owner="", repo="" | owner=="" |
"/repo" |
owner="" | owner=="" |
"owner/" |
repo="" | repo=="" |
Because the function returns before reaching the cache, the seedings at lines 77–78 are never read. The test comment's claim that the seedings guard against "incorrectly return true" is therefore misleading — there is no code path through which that could happen.
Remove the two dead assignments (or replace them with a comment explaining why they're absent) to keep the test signal clear.
🧪 Test Quality Sentinel Report
📊 Metrics & Test Classification (10 scenarios across 4 functions analyzed)
Go: 1 ( Build tag: Inflation context: The test file is new and covers the full caching behavioral contract. The production change was a refactoring (moving lazy initialization into
|
|
@copilot run pr-finisher skill (fix call sites) |
…w feedback
- Change &Compiler{} in safe_jobs.go and safe_outputs_config.go to
NewCompiler() so ownerTypeCache is always initialized, eliminating
the latent nil-map write panic if those paths ever reach
repositoryOwnerIsIndividualUser()
- Add doc comment explaining the fail-safe caching behavior for API
errors (empty string cached, no retry within a run)
- Note that ownerTypeCache is not goroutine-safe (Compiler is sequential)
- Remove dead cache seedings in TestRepositoryOwnerIsIndividualUser_MalformedSlug
(all malformed slugs return before the cache is consulted)
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Fixed in the latest commit. The two |
The
gh api /users/<owner>call ("Checking repository owner type...") was invoked once per compiled workflow file when determining whether to suppress thecopilot-requests: writetip for individual-user repos. With N Copilot workflows in a repo, this meant N identical API round-trips pergh aw compilerun.Changes
compiler_types.go— InitializeownerTypeCache map[string]stringeagerly inNewCompiler()alongsideactionPinWarningsandpriorManifests, making the intent explicit and consistent.permissions_compiler_validator.go— Remove the now-redundant nil-guard fromrepositoryOwnerIsIndividualUser; the map is always initialized.compiler_owner_type_cache_test.go(new) — Unit tests covering:"User","Organization", and""(prior API error) — pre-populating the cache and asserting the correct result without a network callrepositorySlugreturnsfalseimmediatelyCompilerinstance (the multi-file case)NewCompiler()always produces an initialized (non-nil) map