Consolidate 13 hand-rolled sync.Once+error caches onto syncutil.OnceLoader

[Copilot]

pkg/syncutil.OnceLoader[T] already implements the sync.Once + cached-value + cached-error contract, but 13 call sites across pkg/parser and pkg/workflow hand-rolled the same pattern independently — each a separate chance to get the mutex/visibility semantics wrong, and none with OnceLoader's test-friendly Reset()/Override() escape hatches.

Changes

• pkg/parser/schema_compiler.go: main workflow, MCP config, repo config, and AW manifest schema loaders, plus the two parsed-schema-doc caches (6 sites → 6 OnceLoader fields)
• pkg/parser/schema_deprecation.go: shallow and deep deprecated-fields caches
• pkg/workflow/schema_validation.go / awf_config.go: GitHub Actions schema and AWF config schema loaders
• pkg/workflow/samples_validation.go: compiled tool schemas + sorted safe-output field names
• pkg/workflow/imports.go: safe output type keys
• pkg/workflow/model_aliases.go: builtin model aliases, plus the builtin-only alias map identity cache (retains its unsafe-derived pointer for map identity comparison, now hardened to reset on load failure so a panic-recover retry can't be stuck with a stale/zeroed pointer)

No functional or behavioral change — each site preserves its original cache-the-error semantics.

Every site collapses from 3 package-level vars (*Once, cached value, cached error) to one:

// before
var (
	compiledSchemaOnce sync.Once
	compiledSchema     *jsonschema.Schema
	schemaCompileError error
)

func getCompiledSchema() (*jsonschema.Schema, error) {
	compiledSchemaOnce.Do(func() {
		compiledSchema, schemaCompileError = compileSchema(...)
	})
	return compiledSchema, schemaCompileError
}

// after
var compiledSchemaLoader syncutil.OnceLoader[*jsonschema.Schema]

func getCompiledSchema() (*jsonschema.Schema, error) {
	return compiledSchemaLoader.Get(func() (*jsonschema.Schema, error) {
		return compileSchema(...)
	})
}

• Fixes [refactor] Consolidate 13 hand-rolled sync.Once+error caches onto syncutil.OnceLoader #50747

[Copilot]

Pull request overview

Consolidates schema and metadata caches onto syncutil.OnceLoader, reducing duplicated synchronization logic.

Changes:

• Migrates parser schema and deprecation caches.
• Migrates workflow schema, tool, output, and alias caches.
• Adds retry handling for the builtin-only alias map.

Show a summary per file

File	Description
pkg/parser/schema_compiler.go	Migrates six schema caches.
pkg/parser/schema_deprecation.go	Migrates deprecated-field caches.
pkg/workflow/schema_validation.go	Migrates GitHub Actions schema caching.
pkg/workflow/awf_config.go	Migrates AWF schema caching.
pkg/workflow/samples_validation.go	Migrates tool and sorted-name caches.
pkg/workflow/imports.go	Migrates safe-output key caching.
pkg/workflow/model_aliases.go	Migrates model-alias caches and recovery handling.

Review details

Tip

Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

• Files reviewed: 7/7 changed files
• Comments generated: 2
• Review effort level: Balanced

[Copilot]

Fixed in 716c1b2: restored getBuiltinOnlyAliasMap to the infallible sync.Once cache pattern (builtinOnlyAliasMapOnce + shared map/id), removing the OnceLoader retry/reset behavior from this excluded site.

[Copilot]

Fixed in 716c1b2: restored the sorted safe-output field-name cache to infallible sync.Once (sortedSafeOutputFieldNamesOnce) so this site is no longer using OnceLoader and stays out of the 13 error-caching scope.

[github-actions]

⚠️ Security scanning failed for Test Quality Sentinel. Review the logs for details.

No test files were added or modified in this PR. PR #50934 only modifies production code files (7 Go source files in pkg/parser/ and pkg/workflow/). Test Quality Sentinel found no behavioral tests to analyze.

[github-actions]

⚠️ Security scanning failed for Design Decision Gate 🏗️. Review the logs for details.

No ADR enforcement needed: PR does not have the implementation label and has 82 new lines of code in business logic directories (threshold is 100).

[github-actions]

⚠️ Security scanning failed for Matt Pocock Skills Reviewer. Review the logs for details.

[github-actions]

⚠️ PR Code Quality Reviewer failed during code quality review.

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.

[github-actions]

🧪 Test Quality Sentinel Report

PR #50934: "Consolidate 13 hand-rolled sync.Once+error caches onto syncutil.OnceLoader"

Analysis Result

No test files were added or modified in this PR.

This refactoring consolidates 13 hand-rolled sync.Once + error cache patterns across multiple files in pkg/parser/ and pkg/workflow/ into a centralized syncutil.OnceLoader utility.

Files changed (production code only):

• pkg/parser/schema_compiler.go (23 +, 40 −)
• pkg/parser/schema_deprecation.go (10 +, 23 −)
• pkg/workflow/awf_config.go (6 +, 10 −)
• pkg/workflow/imports.go (4 +, 9 −)
• pkg/workflow/model_aliases.go (20 +, 20 −)
• pkg/workflow/samples_validation.go (12 +, 21 −)
• pkg/workflow/schema_validation.go (7 +, 12 −)

Test files changed: 0

Test Coverage Assessment

Since no behavioral tests were added or modified, existing test suites will validate this refactoring through integration and regression coverage. The consolidation maintains API compatibility and behavioral equivalence with the original scattered caches.

Score: N/A — No behavioral tests to analyze

Recommendation: ✅ No test quality issues identified. Proceed with existing test coverage validation during CI.

🧪 Test quality analysis by Test Quality Sentinel · haiku45 · 13 AIC · ⊞ 7.7K · ◷
_{Comment /review to run again}

[github-actions]

✅ Test Quality Sentinel: No test files added or modified. This refactoring PR modifies only production code files (7 Go files consolidating cache patterns). Existing test suites will validate behavioral equivalence during CI.

[github-actions]

Review: Consolidate 13 hand-rolled sync.Once+error caches onto syncutil.OnceLoader

Clean, mechanical refactoring with consistent application across 7 files.

Observations

Correctness ✅
All converted sites faithfully preserve cache-the-error semantics. OnceLoader.Get holds the mutex for the full loader execution, maintaining the single-invocation guarantee.

builtinOnlyAliasMapID visibility ⚠️
Line 91 in model_aliases.go sets builtinOnlyAliasMapID = mapHeaderPointer(data) inside the loader closure (under OnceLoader's mutex), but isBuiltinOnlyAliasMap reads it without any lock. The same exposure existed with sync.Once — worth verifying with go test -race that no new race is introduced by the reset-on-panic path.

Infallible sites wrapped in OnceLoader (existing review comment at samples_validation.go:171)
Minor: the sorted-field-names cache discards a synthesized nil error. Not blocking.

No security concerns introduced.

Verdict

No blocking issues. The PR achieves its stated goal cleanly and reduces the error-prone boilerplate surface area significantly.

🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 43.6 AIC · ⊞ 5.4K

[github-actions]

Skills-Based Review 🧠

Applied /codebase-design — requesting one change for a data-safety issue surfaced by the new Reset() path.

📋 Key Themes & Highlights

Key Themes

• Data race introduced by Reset(): builtinOnlyAliasMapID is a plain uintptr written under builtinOnlyAliasMapLoader’s internal mutex but read bare in isBuiltinOnlyAliasMap. The original sync.Once guaranteed a single write, so unsynchronized readers were safe. OnceLoader.Reset() breaks that guarantee — a concurrent Reset + retry cycle races against readers of the ID. Fix: atomic.Uintptr.

Positive Highlights

• ✅ Clean, consistent mechanical substitution across 7 files — the pattern is easy to audit
• ✅ OnceLoader.Get is simpler and harder to misuse than the three-variable hand-roll
• ✅ Error-caching semantics are preserved identically at all 13 sites
• ✅ awf_config.go and schema_validation.go refactors are textbook-clean
• ✅ The sortedSafeOutputFieldNamesLoader infallible closure is a pragmatic choice given SortedKeys cannot fail

🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 100.9 AIC · ⊞ 7.1K
_{Comment /matt to run again}

Comments that could not be inline-anchored

pkg/workflow/model_aliases.go:112

[/codebase-design] Data race: builtinOnlyAliasMapID is written inside builtinOnlyAliasMapLoader.Get’s mutex (line 91) but read in isBuiltinOnlyAliasMap without any synchronization — and Reset() makes re-runs of the loader possible, turning this into an observable race under the Go memory model.

<details>
<summary>💡 Suggested fix</summary>

Replace the bare uintptr field with atomic.Uintptr:

builtinOnlyAliasMapID atomic.Uintptr // replaces plain uintptr

Then use `at…

[pelikhan]

@copilot run pr-finisher skill

Dob

[Copilot]

@copilot run pr-finisher skill

Done. I ran the PR finisher pass and pushed 716c1b2, restoring the two infallible caches to sync.Once while keeping the 13 sync.Once+error cache migrations on OnceLoader.

[github-actions]

🎉 This pull request is included in a new release.

Release: v0.86.0