Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 39 additions & 0 deletions docs/adr/40258-cache-repository-owner-type-per-compilation-run.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
# ADR-40258: Cache Repository Owner-Type Lookup Per Compilation Run

**Date**: 2026-06-19
**Status**: Draft

## Context

When determining whether to suppress the `copilot-requests: write` permission tip for individual-user repositories, the compiler calls `gh api /users/<owner>` ("Checking repository owner type...") to classify the owner as a `User` or `Organization`. This classification depends only on the owner login, not on the individual workflow file. A single `gh aw compile` run compiles every workflow in the repository on one shared `Compiler` instance, so a repository with N Copilot workflows previously issued N identical `/users/<owner>` round-trips — wasted latency and API quota for a value that never changes within a run. An owner-type cache (`ownerTypeCache map[string]string`, keyed by owner login) was introduced to deduplicate these lookups, and this PR finalizes that strategy by making the cache's initialization eager and unconditional.

## Decision

We will cache the repository owner-type lookup once per owner login on the `Compiler` instance, so that `repositoryOwnerIsIndividualUser` performs at most one `gh api /users/<owner>` call per owner for the lifetime of a compilation run. We will initialize `ownerTypeCache` eagerly in `NewCompiler()` — alongside the existing `actionPinWarnings` and `priorManifests` caches — rather than lazily on first use. This makes the always-initialized invariant explicit at construction time and lets `repositoryOwnerIsIndividualUser` drop its defensive nil-guard, keeping the lookup path linear and free of incidental initialization branches.

## Alternatives Considered

### Alternative 1: Lazy initialization with a nil-guard at the call site
Keep `ownerTypeCache` zero-valued (nil) and initialize it on first access inside `repositoryOwnerIsIndividualUser`. This was the prior behavior. Rejected because it spreads the cache's lifecycle across two locations, forces every reader to re-assert the invariant, and is inconsistent with the sibling caches that are already initialized in `NewCompiler()`.

### Alternative 2: No caching — look up owner type on every call
Issue `gh api /users/<owner>` each time the permission tip is evaluated. Rejected because it reintroduces N redundant identical API calls per multi-workflow compile run, adding network latency and consuming API rate limit for a value that is constant within the run.

## Consequences

### Positive
- At most one owner-type API call per owner per compilation run, eliminating redundant round-trips for multi-workflow repositories.
- The always-initialized cache invariant is established in one place (`NewCompiler()`), consistent with the other compiler caches.
- The lookup path in `repositoryOwnerIsIndividualUser` is simpler without the nil-guard branch.

### Negative
- The cache lives for the full lifetime of the `Compiler` instance; a long-lived or reused compiler would not observe an owner's type changing between `User` and `Organization` mid-process (an acceptable trade-off given owner type is effectively stable).
- A negative result (empty string from a prior API error) is also cached, so a transient `gh api` failure suppresses retries for that owner for the rest of the run.

### Neutral
- The cache is keyed by owner login only, so two repositories under the same owner share one entry.
- Correctness of the eager-init refactor is covered by `compiler_owner_type_cache_test.go`, which exercises cache-hit, malformed-slug, cross-compilation reuse, and non-nil-map invariants.

---

*This is a DRAFT ADR generated by the [Design Decision Gate](https://github.com/github/gh-aw/actions/runs/27828047748) workflow. The PR author must review, complete, and finalize this document before the PR can merge.*
116 changes: 116 additions & 0 deletions pkg/workflow/compiler_owner_type_cache_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,116 @@
//go:build !integration

package workflow

import (
"testing"
)

// TestRepositoryOwnerIsIndividualUser_CacheHit verifies that repositoryOwnerIsIndividualUser
// returns the cached owner type without making a network call when the owner is already
// in the cache. This ensures the owner-type check is performed at most once per repo
// during a single compilation run.
func TestRepositoryOwnerIsIndividualUser_CacheHit(t *testing.T) {
tests := []struct {
name string
cachedType string
expectedResult bool
}{
{
name: "cached User type returns true",
cachedType: "User",
expectedResult: true,
},
{
name: "cached Organization type returns false",
cachedType: "Organization",
expectedResult: false,
},
{
name: "cached empty string (API error) returns false",

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/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.

cachedType: "",
expectedResult: false,
},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
c := NewCompiler()
c.SetRepositorySlug("myowner/myrepo")

// Pre-populate the cache so no network call is made.
// If the cache is not consulted, RunGH would be called without a real gh binary
// available in unit tests, causing the function to return false even for the
// "User" case — the test would then fail on that case.
c.ownerTypeCache["myowner"] = tt.cachedType

got := c.repositoryOwnerIsIndividualUser()
if got != tt.expectedResult {
t.Errorf("repositoryOwnerIsIndividualUser() = %v, want %v (cached owner type %q)",
got, tt.expectedResult, tt.cachedType)
}
})
}
}

// TestRepositoryOwnerIsIndividualUser_MalformedSlug verifies that the function
// returns false immediately when repositorySlug is missing or malformed, without
// consulting the cache or making a network call.
func TestRepositoryOwnerIsIndividualUser_MalformedSlug(t *testing.T) {
tests := []struct {
name string
slug string
}{
{name: "empty slug", slug: ""},
{name: "no slash", slug: "owneronly"},
{name: "trailing slash only", slug: "/"},
{name: "missing owner", slug: "/repo"},
{name: "missing repo", slug: "owner/"},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
c := NewCompiler()
c.SetRepositorySlug(tt.slug)

got := c.repositoryOwnerIsIndividualUser()
if got {
t.Errorf("repositoryOwnerIsIndividualUser() = true for malformed slug %q, want false", tt.slug)
}
})
}
}

// TestRepositoryOwnerIsIndividualUser_CacheSharedAcrossCompilations verifies that
// the owner-type cache persists across multiple calls on the same Compiler instance,
// which represents multiple workflow files compiled in a single `gh aw compile` run.
func TestRepositoryOwnerIsIndividualUser_CacheSharedAcrossCompilations(t *testing.T) {
c := NewCompiler()
c.SetRepositorySlug("myorg/repo-a")

// Seed the cache as if a previous call already resolved the owner type.
c.ownerTypeCache["myorg"] = "Organization"

// Simulate compiling a second workflow in the same repo (different repo name, same owner).
c.SetRepositorySlugIfUnlocked("myorg/repo-b")

// The cache entry for "myorg" must be reused — no network call is made.
got := c.repositoryOwnerIsIndividualUser()
if got {
t.Error("repositoryOwnerIsIndividualUser() = true for Organization owner, want false")
}

// The cache should still hold the original value (not been cleared or overwritten).
if val := c.ownerTypeCache["myorg"]; val != "Organization" {
t.Errorf("ownerTypeCache[myorg] = %q after second call, want %q", val, "Organization")
}
}

// TestRepositoryOwnerIsIndividualUser_CacheInitializedByNewCompiler verifies that
// NewCompiler initializes ownerTypeCache so callers never encounter a nil map panic.
func TestRepositoryOwnerIsIndividualUser_CacheInitializedByNewCompiler(t *testing.T) {
c := NewCompiler()
if c.ownerTypeCache == nil {
t.Fatal("NewCompiler() left ownerTypeCache nil; expected an initialized map")
}
}
5 changes: 3 additions & 2 deletions pkg/workflow/compiler_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,7 @@ type Compiler struct {
requireDocker bool // If true, fail validation when Docker is not available instead of silently skipping
ghesCompatFromCLI bool // If true, GHES compat was requested via --ghes CLI flag (takes precedence over aw.json)
ghesArtifactCompat bool // If true, emit GHES-compatible v3.x pins for artifact actions instead of the latest v7/v8
ownerTypeCache map[string]string // Cached GitHub owner type ("User"/"Organization"/"") keyed by owner login
ownerTypeCache map[string]string // Cached GitHub owner type ("User"/"Organization"/"") keyed by owner login; not goroutine-safe (Compiler is used sequentially)
}

// NewCompiler creates a new workflow compiler with functional options.
Expand Down Expand Up @@ -136,7 +136,8 @@ func NewCompiler(opts ...CompilerOption) *Compiler {
artifactManager: NewArtifactManager(),
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)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/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 sequentially

Or, if compilation is ever parallelized, upgrade to sync.Map for consistency with the established pattern.

gitRoot: gitRoot, // Auto-detected git root
}

// Apply functional options
Expand Down
6 changes: 3 additions & 3 deletions pkg/workflow/permissions_compiler_validator.go
Original file line number Diff line number Diff line change
Expand Up @@ -186,15 +186,15 @@ func (c *Compiler) repositoryOwnerIsIndividualUser() bool {
return false
}

if c.ownerTypeCache == nil {
c.ownerTypeCache = make(map[string]string)
}
ownerType, cached := c.ownerTypeCache[owner]

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/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]string

The nil-guard was inexpensive; removing it transfers an implicit contract to every future test author.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 map

Neither 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.

if !cached {
workflowLog.Printf("Checking owner type for: %s", owner)
output, err := RunGH("Checking repository owner type...", "api", "/users/"+owner, "--jq", ".type")
if err != nil {
workflowLog.Printf("Could not determine owner type for %q: %v", owner, err)
// Cache the empty string 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.
c.ownerTypeCache[owner] = ""
return false
}
Expand Down
2 changes: 1 addition & 1 deletion pkg/workflow/safe_jobs.go
Original file line number Diff line number Diff line change
Expand Up @@ -325,7 +325,7 @@ func extractSafeJobsFromFrontmatter(frontmatter map[string]any) map[string]*Safe
if safeOutputsMap, ok := safeOutputs.(map[string]any); ok {
if jobs, exists := safeOutputsMap["jobs"]; exists {
if jobsMap, ok := jobs.(map[string]any); ok {
c := &Compiler{} // Create a temporary compiler instance for parsing
c := NewCompiler() // Create a temporary compiler instance for parsing
return c.parseSafeJobsConfig(jobsMap)
}
}
Expand Down
2 changes: 1 addition & 1 deletion pkg/workflow/safe_outputs_config.go
Original file line number Diff line number Diff line change
Expand Up @@ -722,7 +722,7 @@ func (c *Compiler) extractSafeOutputsConfig(frontmatter map[string]any) *SafeOut
// Handle jobs (safe-jobs must be under safe-outputs)
if jobs, exists := outputMap["jobs"]; exists {
if jobsMap, ok := jobs.(map[string]any); ok {
c := &Compiler{} // Create a temporary compiler instance for parsing
c := NewCompiler() // Create a temporary compiler instance for parsing
config.Jobs = c.parseSafeJobsConfig(jobsMap)
}
}
Expand Down
Loading