Skip to content

perf: cache builtin alias map and DFS cycle-check to fix CompileMCPWorkflow regression #44996

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
Copilot wants to merge 2 commits into from
+72 −1
Closed

perf: cache builtin alias map and DFS cycle-check to fix CompileMCPWorkflow regression #44996

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ab7f825
Initial plan
Copilot Jul 11, 2026
519a9f1
perf: cache builtin alias map and DFS cycle-check for ~30% CompileMCP…
Copilot Jul 11, 2026
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
45 changes: 45 additions & 0 deletions pkg/workflow/model_alias_validation.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import (
"os"
"strconv"
"strings"
"sync"

"github.com/github/gh-aw/pkg/console"
"github.com/github/gh-aw/pkg/setutil"
Expand All @@ -34,6 +35,13 @@ import (

var modelAliasValidationLog = newValidationLogger("model_alias")

// builtinCycleCheckOnce caches the result of detectCircularModelAliases on the
// pure-builtin alias map. Builtins are immutable, so the result never changes.
var (
builtinCycleCheckOnce sync.Once
builtinCycleCheckErr error
)

// ─── Known-parameter validation ───────────────────────────────────────────────

// ValidateEffortParam validates the "effort" parameter value (V-MAF-002).
Expand Down Expand Up @@ -241,6 +249,43 @@ func (c *Compiler) warnUnrecognizedModelParams(identifiers []string, markdownPat
func detectCircularModelAliases(aliasMap map[string][]string, markdownPath string) error {
modelAliasValidationLog.Printf("Checking for circular alias references in %d aliases", len(aliasMap))

// Fast path: when aliasMap has exactly the same key set as the raw builtins it
// IS the builtin map (MergeImportedModelAliases only ever adds new keys — it never
// removes or overrides them — so equal key sets imply identical content). Builtins
// are immutable, so we run the DFS once and cache the result for all future calls.
rawBuiltins, err := loadBuiltinModelAliases()

@github-actions github-actions Bot Jul 13, 2026

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.

[/diagnosing-bugs] loadBuiltinModelAliases() is still called on every invocation of detectCircularModelAliases, even when the fast path fires. The DFS is cached, but the key-set comparison and the loadBuiltinModelAliases call itself add overhead on each compilation.

💡 Suggested optimisation

Use pointer identity instead of key-set equality: since builtinOnlyAliasMap() always returns the same pointer, you can compare map headers:

// At package level (model_alias_validation.go)
var builtinAliasMapPtr map[string][]string

func init() { builtinAliasMapPtr = builtinOnlyAliasMap() }

func detectCircularModelAliases(aliasMap map[string][]string, markdownPath string) error {
    if isBuiltinOnly := reflect.ValueOf(aliasMap).Pointer() == reflect.ValueOf(builtinAliasMapPtr).Pointer(); isBuiltinOnly {
        // ... cached DFS
    }
    return detectCircularModelAliasesImpl(aliasMap, markdownPath)
}

Or more idiomatically, pass a bool isBuiltinOnly flag from the call site (validateModelAliasMap) to avoid loading builtins altogether in the fast path.

@copilot please address this.

if err != nil {
modelAliasValidationLog.Printf("Warning: could not load builtin model aliases for cycle-check fast path, falling back to full validation: %v", err)

@github-actions github-actions Bot Jul 13, 2026

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.

loadBuiltinModelAliases() + O(n) key scan runs on every call even for non-builtin maps: the fast-path check itself adds overhead for the common case of workflows with custom models.

💡 Details

For every compilation that has custom models (i.e., non-nil importedModels or frontmatterModels), detectCircularModelAliases now does:

  1. loadBuiltinModelAliases() — returns cached data, cheap but not free (sync.Once unlock, two pointer reads).
  2. isBuiltinOnlyAliasMap() — iterates all N keys of aliasMap checking map membership in rawBuiltins.

For the builtin-only fast path this is a net win. For the customModels path, it is pure overhead added before the actual DFS.

If this function is on the hot path for custom-model compilations too, the scan should be gated behind the pointer-identity check suggested in the other comment (which is O(1) and requires no iteration):

if aliasMap == builtinOnlyAliasMap() {
    // run/return cached DFS
}
// fall through to full DFS — no loadBuiltinModelAliases call at all

This eliminates both the loadBuiltinModelAliases() call and the O(n) key scan for all non-builtin callers.

} else if isBuiltinOnlyAliasMap(aliasMap, rawBuiltins) {
builtinCycleCheckOnce.Do(func() {
// Pass an empty markdownPath: the cached result is shared across all
// compilations, so any path embedded in an error message would be
// misleading for callers other than the first one.
builtinCycleCheckErr = detectCircularModelAliasesImpl(rawBuiltins, "")

@github-actions github-actions Bot Jul 13, 2026

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] No regression test exists for the builtinCycleCheckOnce caching path. If the sync.Once wrapper is accidentally removed or the fast path condition breaks, compilation will silently regress without any failing test.

💡 Suggested test 
func TestDetectCircularModelAliases_BuiltinFastPath(t *testing.T) {
    builtins := BuiltinModelAliases()
    // First call — populates the cache
    err1 := detectCircularModelAliases(builtins, "/path/a.md")
    require.NoError(t, err1)
    // Second call — must use cache (same result, no panic)
    err2 := detectCircularModelAliases(builtins, "/path/b.md")
    require.NoError(t, err2)
    // Non-builtin map — must NOT use cache
    custom := map[string][]string{"custom": {"gpt-4o"}}
    for k, v := range builtins { custom[k] = v }
    err3 := detectCircularModelAliases(custom, "/path/c.md")
    require.NoError(t, err3)
}

This guards the fast-path branch and ensures the key-set equality check behaves correctly for both the hit and miss cases.

@copilot please address this.

})
return builtinCycleCheckErr
}

return detectCircularModelAliasesImpl(aliasMap, markdownPath)
}

// isBuiltinOnlyAliasMap reports whether aliasMap has exactly the same set of keys as
// rawBuiltins. Because MergeImportedModelAliases never modifies existing builtin keys

@github-actions github-actions Bot Jul 13, 2026

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.

Fragile key-equality assumption can return a wrong cached cycle-check result: the comment claims equal key sets imply identical content, but this is only true as long as callers go through MergeImportedModelAliases. Any direct construction of an alias map with the same keys but different values will hit the cached builtin DFS result instead of running a fresh check.

💡 Details and suggested fix

The logic is:

  1. Load the raw builtins.
  2. If aliasMap has the same key set → it must be the builtin map → return cached DFS result.

The invariant relies on MergeImportedModelAliases never overriding existing builtin keys. That's documented in the current code, but:

  • It is a cross-function invariant with no compile-time enforcement.
  • Any test or future code path that builds an alias map directly (e.g. in unit tests constructing WorkflowData by hand) with the same key count as the builtins will silently skip cycle detection.
  • If a bug in MergeImportedModelAliases ever does overwrite a key, the cycle check becomes wrong with no visible failure.

Safer approach: Instead of a key-set equality heuristic, use pointer identity. Cache a pointer to the builtin map from builtinOnlyAliasMap() and compare:

if aliasMap == builtinOnlyAliasMap() {  // pointer equality — exact same map
    ...
}

This is O(1), guaranteed-correct, and eliminates the indirect reasoning about key sets entirely. The fast path in MergeImportedModelAliases already returns the same singleton, so this will fire exactly when intended.

// (it only appends new ones), equal key sets mean equal content.
func isBuiltinOnlyAliasMap(aliasMap, rawBuiltins map[string][]string) bool {
if len(aliasMap) != len(rawBuiltins) {
return false
}
for k := range aliasMap {
if _, exists := rawBuiltins[k]; !exists {
return false
}
}
return true
}
Comment on lines +272 to +285

@github-actions github-actions Bot Jul 13, 2026

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.

@github-actions github-actions Bot Jul 13, 2026

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.

The key-set equality check in isBuiltinOnlyAliasMap is O(n) and relies on the invariant that equal key-sets imply equal values. This is a fragile assumption that will silently break if MergeImportedModelAliases ever allows a caller to shadow a builtin key without adding a new key. Since MergeImportedModelAliases(nil,nil) now returns the exact singleton produced by builtinOnlyAliasMap(), a simpler and more reliable identity check is possible using a pointer sentinel stored when the once fires, removing both the O(n) iteration and the brittleness. @copilot please address this.


// detectCircularModelAliasesImpl is the actual DFS cycle-check implementation.
func detectCircularModelAliasesImpl(aliasMap map[string][]string, markdownPath string) error {
// visited tracks keys for which all DFS descendants have been fully explored
// (no cycle detected from that key).
visited := map[string]struct {
Expand Down
28 changes: 27 additions & 1 deletion pkg/workflow/model_aliases.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,11 @@ var (
builtinModelAliasesOnce sync.Once
builtinModelAliasesData map[string][]string
builtinModelAliasesErr error

// mergedBuiltinOnlyOnce caches the result of MergeImportedModelAliases(nil, nil).
// Callers MUST treat the returned map as read-only (must not modify it).
mergedBuiltinOnlyOnce sync.Once
mergedBuiltinOnlyMap map[string][]string
)

func loadBuiltinModelAliases() (map[string][]string, error) {
Expand All @@ -64,6 +69,16 @@ func loadBuiltinModelAliases() (map[string][]string, error) {
return builtinModelAliasesData, builtinModelAliasesErr
}

// builtinOnlyAliasMap returns a shared, read-only copy of the builtin model alias map.
// This is the same as calling BuiltinModelAliases() but the copy is allocated only once.
// The returned map MUST NOT be modified by callers.
func builtinOnlyAliasMap() map[string][]string {
mergedBuiltinOnlyOnce.Do(func() {
mergedBuiltinOnlyMap = BuiltinModelAliases()
})
return mergedBuiltinOnlyMap
}

// BuiltinModelAliases returns the built-in model alias map that covers the main
// model families supported by gh-aw. The returned map is a freshly allocated
// copy so callers may freely modify it.
Expand Down Expand Up @@ -120,9 +135,20 @@ func BuiltinModelAliases() map[string][]string {
// 3. Main workflow frontmatter aliases (highest priority — main workflow file wins)
//
// If both importedModels and frontmatterModels are nil/empty, the builtin aliases are
// returned as-is (identical to MergeModelAliases(nil)).
// returned as a shared read-only map (callers MUST NOT modify it). This avoids an
// allocation-heavy deep copy on every compilation when no custom models are defined.
func MergeImportedModelAliases(importedModels []map[string][]string, frontmatterModels map[string][]string) map[string][]string {
modelAliasesLog.Printf("Merging model aliases: %d import(s), %d frontmatter override(s)", len(importedModels), len(frontmatterModels))

// Fast path: no customisation needed — return a shared read-only builtin map.
// Callers that only read the result (WorkflowData.ModelMappings consumers) benefit
// from avoiding the O(n) deep copy on every compilation.
if len(importedModels) == 0 && len(frontmatterModels) == 0 {
result := builtinOnlyAliasMap()

@github-actions github-actions Bot Jul 13, 2026

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.

[/diagnosing-bugs] MergeImportedModelAliases(nil, nil) now returns a shared cached map, but nothing in Go prevents a caller from mutating workflowData.ModelMappings and corrupting the cache for all subsequent compilations in the same process.

💡 Suggested mitigation

Add a test that verifies mutating the returned map does not affect subsequent calls:

func TestMergeImportedModelAliases_CachedMapIsolation(t *testing.T) {
    m1 := MergeImportedModelAliases(nil, nil)
    originalLen := len(m1)
    // Simulate an accidental write by a caller
    for k := range m1 { delete(m1, k); break }
    m2 := MergeImportedModelAliases(nil, nil)
    require.Len(t, m2, originalLen, "mutating returned map must not corrupt the shared cache")
}

This test will currently fail — making the risk concrete. The fix could be returning a read-only wrapper, or defensive copying in builtinOnlyAliasMap for all write-capable callers.

@copilot please address this.

modelAliasesLog.Printf("Fast path: returning shared builtin alias map (%d entries)", len(result))

@github-actions github-actions Bot Jul 13, 2026

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.

The fast path returns a shared map, but the doc-comment change only adds a sentence at the top — the function signature still returns map[string][]string with no indication to users of the public API that it can return a read-only reference. Callers outside this package (e.g., model_aliases_import_test.go) receive this map and the read-only contract is only communicated via the updated comment. Consider returning the map wrapped in a defensive copy check in tests, or adding a // go:noinline sentinel comment to at least surface the caveat in godoc prominently. This is low severity but could silently corrupt the singleton if an external caller ever ranges over the map and appends to a slice value. @copilot please address this.

return result
Comment on lines +146 to +149

@github-actions github-actions Bot Jul 13, 2026

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.

Shared map with no mutation enforcement: MergeImportedModelAliases now returns a cached singleton in the fast path; any caller that writes to the map will silently corrupt all future compilations.

💡 Details and suggested fix

The fast path at lines 143–151 returns builtinOnlyAliasMap(), which is the package-level singleton mergedBuiltinOnlyMap. Go maps are reference types — assigning the returned value to a struct field and then writing to it (even transitively, e.g. via maps.Copy) mutates the singleton.

Current callers look safe:

  • workflow_builder.go:154 stores the result in WorkflowData.ModelMappings, which is then read-only by awf_config.go:530.

But the contract is enforced only by a comment, and MergeImportedModelAliases is a public function. Any new caller that does:

result := MergeImportedModelAliases(nil, nil)
result["my-model"] = []string{"acme/model"}  // silently corrupts the singleton

will corrupt every subsequent compilation without any test catching it.

Fix options (in order of preference):

  1. Return a copy always in the nil/nil case (cheap: 52 entries, already the pre-diff behaviour).
  2. Introduce a typed read-only wrapper so the compiler enforces immutability at the call site.
  3. At minimum, add a test that calls MergeImportedModelAliases(nil, nil) twice and verifies modifications to one result do not affect the other.

Option 1 is one line and eliminates the whole class of bug without a fragile API contract.

}

merged := BuiltinModelAliases()

// Layer 2 — imported models (first import to define a key wins among imports).
Expand Down
Loading