[repository-quality] Repository Quality Improvement Report - Panic-in-Library-Code Governance Gaps (2026-07-29)

[github-actions[bot]]

🎯 Repository Quality Improvement Report - Panic-in-Library-Code Governance Gaps

Analysis Date: 2026-07-29
Focus Area: Panic Usage in Library Code — Linter Contract Enforcement & sync.OnceValue Blind Spot
Strategy Type: Custom
Custom Area: Yes — the repo has a purpose-built panicinlibrarycode analyzer (pkg/linters/panic-in-library-code) enforcing "no panic in pkg/, return errors instead" with narrow, auditable exemptions (BUG-prefixed messages, init(), sync.Once.Do closures, and documented "panics on/if" contracts). No prior run has audited whether the exemption mechanism itself has coverage gaps or whether existing exempted panics are actually reachable from external/user-controlled input, which would violate the linter's own intent.

Executive Summary

The panicinlibrarycode analyzer (pkg/linters/panic-in-library-code/panic-in-library-code.go) is well-designed but has one structural blind spot: its isInSyncOnceDoFuncLit exemption only recognizes the classic sync.Once.Do(func(){...}) pattern (matched via a Do selector call), not the newer sync.OnceValue(func() T {...}) / sync.OnceFunc idioms introduced in Go 1.21. The repo already uses sync.OnceValue in three places (pkg/workflow/engine_definition_loader.go:41, pkg/workflow/permissions_toolset_data.go:39, pkg/console/console.go:25), two of which contain or could contain panics. Today those panics happen to pass only because their messages start with "BUG:" — an incidental, not principled, protection. Any future sync.OnceValue initializer that panics without a BUG: prefix (e.g., propagating a raw err) will silently bypass linting despite being architecturally identical to the already-exempted sync.Once.Do pattern.

A second finding: of the ~14 non-test, non-testdata panics remaining in pkg/, several (pkg/workflow/model_aliases.go:96, pkg/workflow/claude_tools.go:147, pkg/actionpins/actionpins.go:267) rely on the BUG:-prefix exemption or the documented-contract exemption inconsistently — some have the required "Panics on/if" doc phrase, others don't, making it unpredictable which panics are "sanctioned" vs. accidentally passing. Two panics in pkg/actionpins/actionpins.go (empty-SHA guards) are reachable indirectly from dynamically-resolved action SHAs and hardcoded pin data — plausible defense-in-depth for release-time data corruption, but not clearly bounded to build-time-only invocation, so a corrupted or malicious action_pins.json merged via PR could crash the compiler for any workflow. Tightening the analyzer's Once-family detection, standardizing doc-contract phrasing, and adding tests that assert sync.OnceValue/sync.OnceFunc closures are correctly linted will close a real enforcement gap in an otherwise strong control the team already invested in.

Full Analysis Report

Focus Area: Panic Usage in Library Code — Linter Contract Enforcement

Current State Assessment

The custom panicinlibrarycode Go analyzer runs as part of the linter suite (registered in pkg/linters/registry.go:107, documented in pkg/linters/doc.go:38, spec-tested in pkg/linters/spec_test.go:136). It flags panic() in any pkg/ package (packages under cmd/ or named main are exempt), with four narrow exemptions: message starts with BUG:, call is inside init(), call is inside a sync.Once.Do(func(){...}) closure, or the enclosing function's doc comment contains "panics on/if" / "panic on/if".

Metrics Collected:

Metric	Value	Status
Non-test panic() calls in pkg/	14	⚠️
sync.OnceValue/OnceFunc usages repo-wide	3	⚠️
Of those, closures containing panic()	2 (permissions_toolset_data.go, model_aliases.go's sync.Once.Do — 1 covered, 1 not)	⚠️
Panics correctly exempted by documented-contract phrase	4/8 eligible sites	⚠️
Analyzer test coverage for sync.OnceValue/OnceFunc closures	0	❌
panicinlibrarycode registered + spec-tested	Yes	✅

Findings

Strengths

• Purpose-built analyzer with clear intent (return errors, not panic, in library code) and multiple thoughtful exemptions (BUG-prefix, init, sync.Once, documented contracts).
• Registered in pkg/linters/registry.go and covered by pkg/linters/spec_test.go and doc_sync_test.go, so the linter itself won't silently disappear from CI.
• Existing "BUG:"-prefixed panics (e.g. pkg/workflow/domains.go:740, pkg/workflow/permissions_toolset_data.go:44) correctly signal internal-invariant violations rather than expected error paths.

Areas for Improvement

• [High] isInSyncOnceDoFuncLit in pkg/linters/panic-in-library-code/panic-in-library-code.go:95-115 only matches X.Do(func(){...}) calls (checks sel.Sel.Name == "Do" and isSyncOnceType). It does not recognize sync.OnceValue(func() T {...}) or sync.OnceFunc(func(){...}), both stdlib idioms the repo already uses (pkg/workflow/engine_definition_loader.go:41, pkg/workflow/permissions_toolset_data.go:39). Panics inside these closures currently pass only incidentally via the BUG:-prefix exemption; a future contributor adding panic(err) (no BUG: prefix) inside a sync.OnceValue initializer would slip past the linter entirely.
• [Medium] Inconsistent doc-contract phrasing: pkg/actionpins/actionpins.go:265 (FormatPinnedActionReference) and pkg/workflow/cache.go:42 (cacheMemoryDirFor) both have proper "Panics if/on ..." doc comments and are legitimately exempted, but pkg/workflow/claude_tools.go:141 (prepareClaudeToolsForAllowedList) has no doc comment at all — it passes only via the BUG: message prefix, meaning any future refactor that rewords the panic message without noticing the missing doc contract would break silently (the panic would then be flagged, or worse, would remain unflagged for the wrong reason).
• [Medium] pkg/actionpins/actionpins.go:166 (loadActionPinsData) and :174 panic on malformed/empty-SHA embedded action_pins.json data. This is defensible as build-time data integrity checking, but the function has no explicit "only called during package init" guarantee in code (it's invoked via getActionPins() → sync.OnceValue-style caching per pkg/actionpins/actionpins.go:128) — worth confirming test coverage exists proving this can only execute against the embedded, CI-validated JSON and never a runtime-supplied path.
• [Low] No test in pkg/linters/panic-in-library-code/panic-in-library-code_test.go's testdata exercises a sync.OnceValue/sync.OnceFunc closure case, so the gap above (High finding) has no regression test guarding it either way.

Detailed Analysis

The core risk is asymmetry between exemption intent and exemption implementation. The linter's sync.Once.Do exemption exists because Do closures execute exactly once, typically during lazy initialization of program-invariant data (embedded JSON, computed constants) — a legitimate panic-on-corruption use case distinct from ordinary request/compile-time error handling. sync.OnceValue/sync.OnceFunc (Go 1.21+) are direct successors to that exact pattern and are already used for equivalent purposes in this codebase (loadBuiltinEngineDefinitionMap, getToolsetPermissionsMap). Extending isInSyncOnceDoFuncLit (or adding a sibling check, e.g. isInOnceValueOrOnceFuncArg) to detect sync.OnceValue(<FuncLit>) / sync.OnceFunc(<FuncLit>) call patterns closes this gap cleanly, using the same cur.Enclosing + parent *ast.CallExpr traversal already present in the function, just checking the callee identifier/selector against "OnceValue"/"OnceFunc" package-qualified names instead of a .Do selector.

---

🤖 Tasks for Copilot Agent

NOTE TO PLANNER AGENT: Split the following tasks into individual work items.

Improvement Tasks

Task 1: Extend panicinlibrarycode analyzer to recognize sync.OnceValue/OnceFunc closures

Priority: High
Estimated Effort: Small
Focus Area: Panic Usage in Library Code

Description: The isInSyncOnceDoFuncLit helper in pkg/linters/panic-in-library-code/panic-in-library-code.go only exempts panics inside sync.Once.Do(func(){...}) closures. Add detection for sync.OnceValue(func() T {...}) and sync.OnceFunc(func(){...}) call expressions so panics inside these lazy-initializer closures are treated the same way (an intentional, documented pattern), preventing accidental future bypasses that currently rely only on incidental BUG:-prefix matching.

Acceptance Criteria:

• New helper (e.g. isInOnceValueOrOnceFuncArg) detects sync.OnceValue(<FuncLit>) and sync.OnceFunc(<FuncLit>) patterns analogous to the existing Do-selector check.
• shouldSkipPanic wires in the new helper alongside isInSyncOnceDoFuncLit.
• Testdata added under pkg/linters/panic-in-library-code/testdata/ exercising both an exempted sync.OnceValue panic and a non-exempted ordinary panic for regression coverage.
• go test ./pkg/linters/panic-in-library-code/... passes.

Code Region: pkg/linters/panic-in-library-code/panic-in-library-code.go (lines 88-132)

In pkg/linters/panic-in-library-code/panic-in-library-code.go, the isInSyncOnceDoFuncLit
function only exempts panics inside `sync.Once.Do(func(){...})` closures by checking that
the enclosing call's selector is named "Do" on a sync.Once-typed receiver. Extend this to
also recognize sync.OnceValue(func() T {...}) and sync.OnceFunc(func(){...}) call
expressions, which are the modern stdlib equivalents already used in
pkg/workflow/engine_definition_loader.go and pkg/workflow/permissions_toolset_data.go.

Add a new helper function that walks up from the panic call's enclosing FuncLit to its
parent CallExpr (reuse the existing cur.Enclosing((*ast.FuncLit)(nil)) traversal pattern)
and checks whether the CallExpr.Fun is a *ast.SelectorExpr with Sel.Name in
{"OnceValue", "OnceFunc"} whose package (via pass.TypesInfo.Uses) is "sync". Wire this
helper into shouldSkipPanic alongside the existing isInSyncOnceDoFuncLit check.

Add testdata files under pkg/linters/panic-in-library-code/testdata/src/panicinlibrarycode/
with one case showing a panic inside a sync.OnceValue closure (should NOT be flagged) and
verify existing non-exempted panic cases still report correctly. Run
`go test ./pkg/linters/panic-in-library-code/...` to confirm.

---

Task 2: Add missing "Panics if" doc contract to prepareClaudeToolsForAllowedList

Priority: Medium
Estimated Effort: Small
Focus Area: Panic Usage in Library Code

Description: pkg/workflow/claude_tools.go:141 (prepareClaudeToolsForAllowedList) panics with a BUG:-prefixed message but has no doc comment describing the panic contract, unlike comparable functions such as FormatPinnedActionReference and cacheMemoryDirFor. Add a doc comment following the established "Panics if ..." convention so the panic contract is discoverable from documentation alone, independent of the incidental BUG: message-prefix exemption.

Acceptance Criteria:

• Doc comment added above prepareClaudeToolsForAllowedList explaining when/why it panics, using the phrase "panics if" or "panics on" to align with the linter's documented-contract exemption.
• No behavior change to the function itself.
• make fmt run after edit.

Code Region: pkg/workflow/claude_tools.go (lines 141-148)

In pkg/workflow/claude_tools.go, the function prepareClaudeToolsForAllowedList (around
line 141) panics with "BUG: computeAllowedClaudeToolsString should only receive neutral
tools, not claude section tools" when the input tools map already contains a "claude" key,
but has no doc comment. Add a doc comment directly above the function signature following
the repo's established pattern (see FormatPinnedActionReference in
pkg/actionpins/actionpins.go or cacheMemoryDirFor in pkg/workflow/cache.go for examples),
using the phrase "Panics if" so it satisfies the panicinlibrarycode linter's documented-
panic-contract exemption independent of the BUG: message prefix. Example:

// prepareClaudeToolsForAllowedList expands neutral tool definitions into Claude-specific
// format. Panics if tools already contains a "claude" section key, since callers must only
// ever pass neutral tool definitions at this stage (an internal invariant violation).

Run `make fmt` after the change and confirm no other tests are affected.

---

Task 3: Verify build-time-only reachability of action_pins.json panic paths

Priority: Medium
Estimated Effort: Medium
Focus Area: Panic Usage in Library Code

Description: loadActionPinsData in pkg/actionpins/actionpins.go:162-175 panics on JSON unmarshal failure or empty-SHA entries in the embedded action_pins.json. Confirm (via test or code comment/assertion) that this path is only ever exercised against the compile-time embedded file and never against any runtime/user-supplied data, and add a regression test that a corrupted action_pins.json (if ever merged) would be caught by an existing CI check (e.g., go generate/go vet/dedicated validation test) before reaching production, rather than only failing at panic time in a user's compiler run.

Acceptance Criteria:

• Confirm (document or test) that loadActionPinsData is only called with the //go:embed-sourced action_pins.json, never a caller-supplied byte slice.
• Add or locate an existing CI-run test that validates action_pins.json has no empty SHAs / is valid JSON, independent of the panic path, so bad data is caught at PR time, not at every compiler invocation.
• Document findings in a code comment near loadActionPinsData if reachability is confirmed safe, or file a follow-up if a gap is found.

Code Region: pkg/actionpins/actionpins.go (lines 128-176)

In pkg/actionpins/actionpins.go, loadActionPinsData (lines 162-176) panics if the embedded
action_pins.json is malformed or contains entries with empty SHA values. Investigate all
call sites of loadActionPinsData (via getActionPins() at line 128) to confirm this function
is exclusively invoked with the go:embed'd action_pins.json content and never with any
external or user-controllable byte slice — if a malicious/corrupted PR could alter this
embedded file, any user running `gh aw compile` afterward would crash via panic rather than
getting a clear CI-time validation failure.

Check whether an existing test (search pkg/actionpins/*_test.go) already validates the
real embedded action_pins.json for empty SHAs and JSON validity independent of this panic
path — if such a test does not exist, add one so CI fails fast on bad data at PR-review time
rather than only within the panic in production compiles. Document the reachability
guarantee as a code comment above loadActionPinsData once confirmed.

---

📊 Historical Context

Previous Focus Areas

Date	Focus Area	Type	Custom	Key Outcomes
2026-07-17	Error Chain Transparency & Non-Wrapping fmt.Errorf Gap	Custom	Y	788 non-wrapping fmt.Errorf calls identified
2026-07-22	Output Stream Abstraction Gap — Hardcoded os.Stdout	Custom	Y	167 hardcoded os.Stdout refs in pkg/cli/
2026-07-28	CLI Flag Boilerplate Duplication & Legacy-Alias Drift	Custom	Y	Duplicated flag registration across 5 command files
2026-07-29	Panic Usage in Library Code — Linter Contract Enforcement	Custom	Y	sync.OnceValue blind spot in panicinlibrarycode analyzer; inconsistent doc contracts

---

🎯 Recommendations

Immediate Actions (This Week)

1. Extend isInSyncOnceDoFuncLit to cover sync.OnceValue/sync.OnceFunc — Priority: High

Short-term Actions (This Month)

1. Standardize "Panics if/on" doc comments across all exempted panic sites — Priority: Medium
2. Confirm build-time-only reachability of action_pins.json panic paths — Priority: Medium

Long-term Actions (This Quarter)

1. Consider a lint rule requiring a doc-contract comment on every panic in pkg/, even BUG-prefixed ones, to remove reliance on message-string matching entirely — Priority: Low

---

📈 Success Metrics

• sync.OnceValue/OnceFunc closures correctly linted: 0/2 → 2/2
• Panic sites with proper doc contracts: 4/8 → 8/8

---

Next Steps

1. Review and prioritise the tasks above
2. Assign tasks to Copilot coding agent via planner agent
3. Track progress on improvement items
4. Re-evaluate this focus area in 4-6 weeks

---

Generated by Repository Quality Improvement Agent
Next analysis: 2026-07-30 — Focus area selected by diversity algorithm

Warning

Firewall blocked 1 domain

The following domain was blocked by the firewall during workflow execution:

• proxy.golang.org

To allow these domains, add them to the network.allowed list in your workflow frontmatter:

network:
  allowed:
    - defaults
    - "proxy.golang.org"

See Network Configuration for more information.

Generated by ⚡ Repository Quality Improvement Agent · aut00 · 77.6 AIC · ⌖ 9.38 AIC · ⊞ 9.2K · ◷

• expires on Jul 30, 2026, 5:31 AM UTC-08:00

[github-actions[bot]]

This discussion was automatically closed because it expired on 2026-07-30T13:31:57.585Z.

Closed by Workflow

[github-actions[bot]]

This discussion was automatically closed because it expired on 2026-07-30T13:31:57.585Z.

Closed by Workflow