[repository-quality] 🎯 Repository Quality Improvement Report - Error Handling Consistency & Panic Safety (2026-08-20)

[github-actions[bot]]

🎯 Repository Quality Improvement Report - Error Handling Consistency & Panic Safety

Analysis Date: 2026-08-20
Focus Area: Error Handling Consistency & Panic Safety in Library Code (Custom)
Strategy Type: Custom
Custom Area: Yes — the repo has a dedicated errorfwrapv, panic-in-library-code, and errstringmatch linter already, but a codebase-wide sweep shows the linters are not catching every risky pattern (init-time sync.Once panics are exempted as "startup" and several hotspot files still concentrate un-wrapped fmt.Errorf(%v, err) calls that block errors.Is/errors.As composition). This is a natural, high-leverage follow-up to yesterday's "Monolithic File Risk Reduction" run and complements the existing linter suite rather than duplicating it.

Executive Summary

gh-aw already invests in static analysis for error hygiene (errorfwrapv, panic-in-library-code, errstringmatch custom linters), which is a strength relative to most Go projects. However, a repository-wide grep sweep found meaningful residual risk: 919 of 2,504 non-test fmt.Errorf call sites (~37%) still format an error with %v instead of %w, concentrated in a handful of CLI hotspot files (init.go, dispatch.go, run_push.go, upgrade_command.go, audit.go). These call sites silently break errors.Is/errors.As chains for callers and tests. Separately, several pkg/workflow and pkg/actionpins files use panic() inside lazily-initialized (sync.Once) accessors for embedded JSON data — these are defensible as "should never happen in a released build" but currently rely only on the panic-in-library-code linter's exemption for init-like patterns rather than an explicit, documented contract, so a future refactor could unintentionally introduce a runtime panic path reachable from user input.

Three fragile strings.Contains(err.Error(), "...") checks were also found outside test/linter fixtures (project_command.go, update_extension_check.go, add_interactive_git.go), each currently exempted or unflagged, that would break silently if the wrapped CLI's (e.g. gh) error text changes. Recommended actions: (1) wrap the top-offending files' errors with %w, (2) add doc comments + nolint justification consistency for the intentional panic() sites in pkg/workflow/pkg/actionpins, and (3) replace the three brittle string-matching error checks with structured detection helpers in pkg/cli/errorutil.

Full Analysis Report

Focus Area: Error Handling Consistency & Panic Safety in Library Code

Current State Assessment

The codebase has custom golangci-lint-style analyzers (pkg/linters/errorfwrapv, pkg/linters/panic-in-library-code, pkg/linters/errstringmatch) registered in pkg/linters/registry.go, indicating error-handling hygiene is already a recognized priority. Despite this, raw grep counts show substantial un-wrapped error formatting still in production code, and a small number of panic() calls in pkg/workflow / pkg/actionpins lazy-loaders that are not caught by the linter because they occur in sync.Once/package-init-adjacent contexts.

Metrics Collected:

Metric	Value	Status
fmt.Errorf calls (non-test, non-linter)	2,504	⚠️
...without %w wrapping	919 (~37%)	⚠️
errors.New sentinel-style declarations	558	✅
Files using errors.Is	44	⚠️ (low relative to Errorf count)
Files using errors.As	40	⚠️
panic() calls outside cmd/, tests, linters	16	⚠️
Brittle strings.Contains(err.Error(), ...) (non-test/linter)	3	⚠️
Custom error-hygiene linters already registered	3 (errorfwrapv, panic-in-library-code, errstringmatch)	✅
TODO/FIXME markers in non-test Go code	23	✅

Findings

Strengths

• Dedicated static-analysis linters already enforce %w wrapping, ban panics in library code, and flag brittle error-string matching — this is above-average investment in error-handling rigor.
• errors.New sentinel usage (558 occurrences) shows an established pattern for comparable/typed errors.
• Existing nolint:errstringmatch comments show the team already documents deliberate exceptions (e.g., add_interactive_git.go:22, errstringmatch.go:36) rather than silently suppressing.

Areas for Improvement

• ⚠️ Medium: 919 fmt.Errorf(..., err) call sites lack %w, concentrated in pkg/cli/init.go (20), pkg/cli/dispatch.go (19), pkg/cli/run_push.go (18), pkg/workflow/compiler_orchestrator_engine.go (16), pkg/cli/upgrade_command.go (16). These break errors.Is/errors.As for calling code and tests that need to distinguish error types (e.g., "not found" vs "network" vs "auth" failures).
• ⚠️ Medium: pkg/workflow/model_aliases.go (lines 91, 148) and pkg/actionpins/data.go (line 67) panic() on failure to load embedded JSON data via sync.Once. These are defensible ("should never happen because the JSON ships with the binary") but are undocumented as an intentional invariant beyond a one-line comment, and are exempt from panic-in-library-code only incidentally rather than via an explicit (nolint/redacted) with rationale, making future refactors risk accidentally invoking the "impossible" panic path with dynamic (non-embedded) data.
• ⚠️ Low: pkg/cli/project_command.go:384 and pkg/cli/update_extension_check.go:439 use strings.Contains(err.Error(), ...) without a (nolint/redacted):errstringmatch explanation (unlike the already-documented case in add_interactive_git.go:22), inconsistent with the project's own convention of justifying such exceptions.
• ✅ Low priority: pkg/cli/gateway_logs_mcp.go:269 has a stale // TODO: Implement token-usage correlation for MCP tool calls. with no tracking issue reference.

Detailed Analysis

The %w-wrapping gap matters most in pkg/cli, where CLI commands often need to distinguish "not found" vs "permission" vs "network" failures to print user-friendly messages or exit codes (errorutil.IsNotFoundError is already used in project_command.go:384, showing there's a real need for typed error introspection elsewhere too). Fixing the top 5 hotspot files (98 of the 919 non-wrapped sites, ~11%) would meaningfully raise the fraction of errors that support errors.Is/errors.As without a repo-wide mechanical rewrite.

The lazy-load panic() sites in model_aliases.go, agentic_engine.go, permissions_toolset_data.go, pi_engine.go, mcp_setup_gateway.go, and mcp_setup_safe_outputs.go all follow the same shape: unmarshal embedded JSON once, panic on error. Because the JSON is (go/redacted):embed-ed, this can only fail if a future PR corrupts the embedded file — a build-time-detectable condition. Adding an explicit init-time self-check (e.g., a small TestBuiltinDataIsValid test already may exist, but it should be paired with a linter suppression comment and a one-line "documented panic contract" note, matching the pattern already demonstrated in panic-in-library-code's own test fixture at line 86: // should not be flagged — documented panic contract).

---

🤖 Tasks for Copilot Agent

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

Improvement Tasks

Task 1: Wrap errors with %w in top CLI hotspot files

Priority: Medium
Estimated Effort: Medium
Focus Area: Error Handling Consistency

Description: In pkg/cli/init.go, pkg/cli/dispatch.go, and pkg/cli/run_push.go, convert fmt.Errorf("...: %v", err) (or similar non-%w formatting of an error argument) call sites to use %w so wrapped errors support errors.Is/errors.As. Do not change call sites where the underlying error type deliberately should not be exposed (e.g., logging-only Printf calls, which are not fmt.Errorf and should be left alone). Run the existing errorfwrapv linter (make lint or the project's linter target) after changes to confirm all remaining un-wrapped call sites in these three files are gone or justified with (nolint/redacted):errorfwrapv and a rationale comment.

Acceptance Criteria:

• All fmt.Errorf calls in pkg/cli/init.go, pkg/cli/dispatch.go, pkg/cli/run_push.go that pass an error value use %w instead of %v/%s, unless justified with an inline (nolint/redacted):errorfwrapv comment
• errorfwrapv linter passes clean on these three files
• Existing unit tests still pass (make test-unit)

Code Region: pkg/cli/init.go, pkg/cli/dispatch.go, pkg/cli/run_push.go

In pkg/cli/init.go, pkg/cli/dispatch.go, and pkg/cli/run_push.go, find every fmt.Errorf(...) call that formats an `err` (or similarly named error) variable using %v or %s and change the verb to %w so the returned error preserves error-chain compatibility with errors.Is/errors.As. Do not touch logger.Printf/log-only statements — only fmt.Errorf calls that construct and return/pass along an error value. After editing, run the repository's errorfwrapv linter (see pkg/linters/errorfwrapv) against these files and fix or explicitly nolint any remaining flagged occurrence with a one-line justification comment. Run `make fmt` and `make test-unit` afterward and ensure everything passes.

---

Task 2: Document the intentional panic contract in embedded-JSON lazy loaders

Priority: Low
Estimated Effort: Small
Focus Area: Panic Safety

Description: In pkg/workflow/model_aliases.go (lines ~91 and ~148) and pkg/actionpins/data.go (~line 67), the panic() calls fire only if the embedded ((go/redacted):embed) JSON data fails to unmarshal — a condition that should be impossible at runtime because the data ships inside the binary and is validated by existing tests. Add a doc comment directly above each panic call explaining this is a deliberate "should never happen" invariant guarded by build-time embedding and existing data-validation tests, matching the "documented panic contract" pattern already demonstrated in pkg/linters/panic-in-library-code/testdata/.../panicinlibrarycode.go:86. Verify a test exists (or add one if missing) that unmarshal-validates the actual embedded JSON files so a corrupt data file fails CI before shipping, not at runtime via panic.

Acceptance Criteria:

• Each of the 6 panic call sites (model_aliases.go x2, data.go x1, agentic_engine.go, permissions_toolset_data.go, pi_engine.go, mcp_setup_gateway.go, mcp_setup_safe_outputs.go) has a one-line comment stating the panic is an intentional, build-time-guarded invariant
• A test exists in each package that validates the embedded JSON/data files parse successfully, so failures are caught in CI rather than at runtime
• No behavioral change to the panics themselves — this is a documentation/testing task only

Code Region: pkg/workflow/model_aliases.go, pkg/actionpins/data.go, pkg/workflow/agentic_engine.go, pkg/workflow/permissions_toolset_data.go, pkg/workflow/pi_engine.go, pkg/workflow/mcp_setup_gateway.go, pkg/workflow/mcp_setup_safe_outputs.go

Review the panic() calls in pkg/workflow/model_aliases.go (getBuiltinOnlyAliasMap and BuiltinModelAliases), pkg/actionpins/data.go (getActionPins), pkg/workflow/agentic_engine.go, pkg/workflow/permissions_toolset_data.go, pkg/workflow/pi_engine.go, pkg/workflow/mcp_setup_gateway.go, and pkg/workflow/mcp_setup_safe_outputs.go. All of these panic only if embedded (go:embed) JSON/config data fails to load or marshal — a condition that should be unreachable in a correctly built binary. For each site, add a short doc comment immediately above the panic() call explaining this is an intentional "documented panic contract" for an unreachable build-time-guarded invariant (following the pattern already used in pkg/linters/panic-in-library-code/testdata/src/panicinlibrarycode/panicinlibrarycode.go around line 86). Then check whether each package already has a test that unmarshals/validates its embedded data file; if not, add one so a corrupted embedded file fails `go test` in CI instead of only surfacing as a runtime panic. Do not change the panic behavior itself. Run `make fmt` and `make test-unit` after changes.

---

Task 3: Justify or replace brittle err.Error() string matching

Priority: Low
Estimated Effort: Small
Focus Area: Error Handling Consistency

Description: pkg/cli/project_command.go:384 and pkg/cli/update_extension_check.go:439 use strings.Contains(err.Error(), "...") to detect specific failure conditions from external command output (e.g., GitHub API/gh CLI text), which is flagged by the project's own errstringmatch linter philosophy but currently lacks the (nolint/redacted):errstringmatch justification comment used elsewhere (e.g., pkg/cli/add_interactive_git.go:22). Either (a) replace the check with a structured error type/sentinel where the underlying call already returns one, or (b) if no structured alternative exists (e.g., wrapping an external CLI's free-text output), add a (nolint/redacted):errstringmatch comment explaining why string matching is unavoidable, consistent with the existing convention.

Acceptance Criteria:

• pkg/cli/project_command.go:384 and pkg/cli/update_extension_check.go:439 either use structured error detection or carry a (nolint/redacted):errstringmatch comment with rationale
• errstringmatch linter run against these files shows no unexplained findings
• Existing tests covering these code paths still pass

Code Region: pkg/cli/project_command.go:384, pkg/cli/update_extension_check.go:439

In pkg/cli/project_command.go around line 384 and pkg/cli/update_extension_check.go around line 439, review the strings.Contains(err.Error(), "...") checks. Determine whether the underlying call (e.g., a GitHub API client or gh CLI wrapper) already exposes a structured/typed error you could use instead (similar to errorutil.IsNotFoundError already used alongside the check in project_command.go:384). If a structured check is feasible, replace the string match with it. If the error truly only comes back as free-text CLI/API output with no structured alternative, add a `(nolint/redacted):errstringmatch` comment directly above the line explaining why, matching the style already used in pkg/cli/add_interactive_git.go:22. Run the project's errstringmatch linter against both files afterward to confirm no unexplained findings remain, then run `make fmt` and `make test-unit`.

---

Task 4: Resolve or track the stale token-usage correlation TODO

Priority: Low
Estimated Effort: Small
Focus Area: Code Quality

Description: pkg/cli/gateway_logs_mcp.go:269 contains // TODO: Implement token-usage correlation for MCP tool calls. with no linked tracking issue and no indication of current status. Investigate whether this feature is still needed; if so, replace the bare TODO with a reference to a tracked GitHub issue (create one if none exists) so the work is discoverable and prioritizable; if the functionality has since been implemented elsewhere or is no longer needed, remove the stale comment.

Acceptance Criteria:

• pkg/cli/gateway_logs_mcp.go:269 TODO either references a tracked issue number or is removed as obsolete
• If a new issue is created, it briefly describes the token-usage correlation gap and where it applies (MCP gateway logs)

Code Region: pkg/cli/gateway_logs_mcp.go:269

Investigate the TODO comment at pkg/cli/gateway_logs_mcp.go:269 ("TODO: Implement token-usage correlation for MCP tool calls."). Check whether this functionality has since been implemented elsewhere in the MCP gateway logging code (search pkg/workflow and pkg/cli for "token" and "correlation" near MCP log handling). If it's still an open gap, replace the bare TODO comment with a reference to a tracked GitHub issue (open a new issue describing the gap if one does not already exist) so the work is discoverable. If the feature is no longer relevant or has been implemented, remove the stale comment. Run `make fmt` after any changes.

---

📊 Historical Context

Previous Focus Areas

Date	Focus Area	Type	Custom	Key Outcomes
2026-08-19	Monolithic File Risk Reduction (Code Organization)	Custom	Y	Identified concentration of 1000+ LOC Go files in pkg/cli and pkg/parser; 4 tasks generated
2026-08-20	Error Handling Consistency & Panic Safety in Library Code	Custom	Y	919 un-wrapped fmt.Errorf sites found; 6 panic sites in lazy-loaders documented; 3 brittle err.Error() checks flagged

---

🎯 Recommendations

Immediate Actions (This Week)

1. Wrap %w in top 3 CLI hotspot files (init.go, dispatch.go, run_push.go) — Priority: Medium

Short-term Actions (This Month)

1. Document panic contracts in the 6 embedded-JSON lazy-loader sites and add validation tests — Priority: Low
2. Justify or replace the 3 brittle err.Error() string-match checks — Priority: Low

Long-term Actions (This Quarter)

1. Consider extending the errorfwrapv linter's scope or running a repo-wide automated %w migration pass across all ~919 remaining call sites — Priority: Low

---

📈 Success Metrics

• Un-wrapped fmt.Errorf call sites: 919 → <800 (after top-hotspot fixes)
• Documented panic contracts in lazy-loaders: 0/6 → 6/6
• Brittle err.Error() checks with rationale: 1/3 → 3/3

---

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 1 quarter (or sooner if reused)

---

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

Generated by ⚡ Repository Quality Improvement Agent · auto · 39.4 AIC · ⌖ 8.25 AIC · ⊞ 9.9K · ◷

• expires on Aug 21, 2026, 5:16 AM UTC-08:00