[repository-quality] Repository Quality Improvement Report — Error Chain Transparency & Non-Wrapping fmt.Errorf Gap

[github-actions[bot]]

🎯 Repository Quality Improvement Report - Error Chain Transparency & Non-Wrapping fmt.Errorf Gap

Analysis Date: 2026-07-17
Focus Area: Error Chain Transparency & Non-Wrapping fmt.Errorf Gap
Strategy Type: Custom
Custom Area: Yes — the codebase has a well-developed custom error type hierarchy (15 named error structs) and errors.Is/errors.As usage (121 callsites), yet 788 non-wrapping fmt.Errorf calls (37% of all fmt.Errorf) silently discard the upstream cause, making programmatic error classification brittle and stacktraces opaque.

Executive Summary

gh-aw has a mature structured error story: 15 distinct error types (WorkflowValidationError, ImportError, CompilerError, GuidedError, etc.), 121 errors.Is/errors.As callsites, and the errorfwrapv linter that catches %v-instead-of-%w misuse. Despite this foundation, 788 fmt.Errorf calls across 395 production files use no error argument at all — they create brand-new leaf errors that can never be inspected via errors.Is/errors.As. The three busiest packages account for 754/788 (96%): pkg/workflow (375), pkg/cli (286), and pkg/parser (93).

The bulk of these are validation errors that format user-visible messages. Those errors intentionally discard upstream cause because there is no upstream cause — they are leaf errors. However a large subset occurs in parser and compiler paths where an upstream structured error does exist but is swallowed by a fresh fmt.Errorf. Additionally, the repository has zero uses of golang.org/x/sync/errgroup despite 8 fire-and-forget goroutines, 4 of which (docker_images.go, bootstrap_profile_runner.go, mcp_inspect_inspector.go, update_check.go) could benefit from structured concurrency.

Five tasks below address the highest-value fixes: wrapping audit in the parser import pipeline, errgroup adoption, a linter to detect discarded-cause patterns, sentinel error promotion for the most re-matched strings, and cleanup of the strings.Contains(err.Error(), ...) suppressions.

Full Analysis Report

Focus Area: Error Chain Transparency & Non-Wrapping fmt.Errorf Gap

Current State Assessment

The codebase uses fmt.Errorf in two distinct modes:

• Wrapping (%w verb): 1,453 callsites — wraps an upstream error so errors.Is/errors.As can inspect it.
• Non-wrapping (no %w verb): 788 callsites — creates a new leaf error; any upstream cause is silently discarded.

The errorfwrapv linter catches the middle case (%v on an error argument), but it cannot flag callsites that have no error argument — those are syntactically valid leaf errors that may be missing a %w clause by accident.

Metrics Collected:

Metric	Value	Status
fmt.Errorf with %w (wrapping)	1,453	✅
fmt.Errorf without %w (non-wrapping)	788	⚠️
Non-wrapping ratio	35%	⚠️
Named error types	15	✅
errors.Is/errors.As callsites (prod)	121	✅
Sentinel var ErrXxx = errors.New	4	⚠️
strings.Contains(err.Error(), ...) in production	4	⚠️
golang.org/x/sync/errgroup usages	0	⚠️
Fire-and-forget go func() goroutines	8	⚠️

Findings

Strengths

• 15 named error types with clean Error() / Unwrap() implementations.
• errorfwrapv linter prevents %v-on-error anti-pattern.
• errstringmatch linter flags brittle strings.Contains(err.Error(), ...) — though 4 production suppressions remain.
• 1,453 wrapping calls (65%) are correctly forwarding causes.

Areas for Improvement

• [HIGH] Parser import pipeline: pkg/parser/import_schema_validation.go (11 non-wrapping), pkg/parser/schedule_parser.go (13), pkg/parser/import_field_extractor.go — these parse structured YAML and likely receive upstream YAML errors that are currently discarded.
• [HIGH] 4 strings.Contains(err.Error(), ...) production suppressions bypass errstringmatch; 2 could be replaced with typed sentinel comparisons (errors.Is).
• [MEDIUM] Only 4 sentinel var ErrXxx vars; the most frequently re-matched error strings (e.g., "already merged" matched in 2 callsites, "INSUFFICIENT_SCOPES" in 2 callsites) should be promoted to sentinels.
• [MEDIUM] golang.org/x/sync/errgroup is completely absent; 4 go func() sites in mcp_inspect_inspector.go and bootstrap_profile_runner.go manage their own channels/panics manually.
• [LOW] The errorfwrapv linter only catches %v-on-error; a complementary linter could flag fmt.Errorf("...", someErr) where someErr is passed as a non-%w argument without a format verb.

Detailed Analysis

Top non-wrapping files:

• pkg/workflow/engine_validation.go — 26 calls; all are leaf validation errors with no upstream cause. Correctly non-wrapping.
• pkg/cli/bootstrap_profile_manifest.go — 22 calls; manifest parse validation, no upstream cause. Correctly non-wrapping.
• pkg/workflow/stop_after.go — 16 calls; similarly leaf validation.
• pkg/workflow/time_delta.go — 14 calls; leaf validation.
• pkg/workflow/model_identifier.go — 14 calls; leaf validation.
• pkg/parser/schedule_parser.go — 13 calls; mixed: some lines call into a sub-parser that returns errors that are then wrapped in a fresh fmt.Errorf losing chain.
• pkg/cli/add_package_manifest.go — 13 calls; some receive yaml.Unmarshal errors that are discarded.

Production strings.Contains(err.Error(), ...) suppressions:

pkg/workflow/schedule_preprocessing.go:194  "syntax is not supported"
pkg/cli/add_interactive_git.go:22            "already merged" OR "MERGED"
pkg/cli/update_extension_check.go:438       dynamic msg variable
pkg/cli/project_command.go:322              "INSUFFICIENT_SCOPES"

Of these, "already merged"/"MERGED" and "INSUFFICIENT_SCOPES" are stable gh CLI error strings that recur in multiple files — ideal sentinel candidates.

---

🤖 Tasks for Copilot Agent

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

Improvement Tasks

Task 1: Introduce Sentinel Errors for Recurring String Matches

Priority: High
Estimated Effort: Small
Focus Area: Error Chain Transparency

Description: Promote the two most-repeated error-string patterns — "already merged"/"MERGED" (gh CLI merge status) and "INSUFFICIENT_SCOPES" (gh CLI auth scope) — to typed sentinel errors in a shared pkg/errorutil (or add to the existing errorutil package). Replace the strings.Contains(err.Error(), ...) callsites in pkg/cli/add_interactive_git.go and pkg/cli/project_command.go with errors.Is checks, removing the //nolint:errstringmatch suppressions.

Acceptance Criteria:

• var ErrAlreadyMerged = errors.New("already merged") (or equivalent) defined in pkg/errorutil (or pkg/cli/git_errors.go).
• var ErrInsufficientScopes = errors.New("INSUFFICIENT_SCOPES") defined similarly.
• pkg/cli/add_interactive_git.go:22 rewritten to errors.Is(err, ErrAlreadyMerged) without nolint:errstringmatch.
• pkg/cli/project_command.go:322 rewritten to errors.Is(err, ErrInsufficientScopes) without nolint:errstringmatch.
• Existing tests still pass (make test).

Code Region: pkg/cli/add_interactive_git.go, pkg/cli/project_command.go, pkg/errorutil/

In the gh-aw repository, promote repeated error string matches to typed sentinel errors.

1. In `pkg/cli/add_interactive_git.go` (line 22), the code does:
   ```go
   return strings.Contains(err.Error(), "already merged") || strings.Contains(err.Error(), "MERGED")

Introduce a sentinel var ErrAlreadyMerged in pkg/errorutil (or a new pkg/cli/git_errors.go file) and replace the strings.Contains check with an errors.Is guard. Remove the //nolint:errstringmatch suppression.

2. In pkg/cli/project_command.go (line 322), the code does:

   strings.Contains(err.Error(), "INSUFFICIENT_SCOPES")

   Introduce a sentinel var ErrInsufficientScopes and replace with errors.Is. Remove the //nolint:errstringmatch suppression.

3. Run make fmt && make test to confirm no regressions.

---

#### Task 2: Wrap Upstream YAML Errors in Parser Import Pipeline

**Priority**: High
**Estimated Effort**: Medium
**Focus Area**: Error Chain Transparency

**Description:** `pkg/parser/schedule_parser.go` (13 non-wrapping calls) and `pkg/cli/add_package_manifest.go` (13 non-wrapping calls) receive upstream errors from YAML unmarshalling or sub-parsers and then wrap them in a new `fmt.Errorf` *without* `%w`, silently discarding the upstream cause. Audit both files and convert callsites where an upstream error is available but not chained.

**Acceptance Criteria:**
- [ ] All `fmt.Errorf("...", ...)` calls in `pkg/parser/schedule_parser.go` that receive an error parameter use `%w` (not `%v` or discarded) or explicitly document why wrapping is not appropriate.
- [ ] Same audit applied to `pkg/cli/add_package_manifest.go`.
- [ ] `errorfwrapv` linter reports no new violations after the change.
- [ ] `make test` passes.

**Code Region:** `pkg/parser/schedule_parser.go`, `pkg/cli/add_package_manifest.go`

```markdown
In the gh-aw repository, audit `pkg/parser/schedule_parser.go` and `pkg/cli/add_package_manifest.go` for `fmt.Errorf` calls that receive an upstream error via a variable (e.g., `err`, `parseErr`) but do NOT use `%w`.

For each such call:
- If the upstream error adds diagnostic context that callers should be able to inspect via `errors.Is`/`errors.As`, change `"%v"` → `"%w"` or add `": %w"` to the format string.
- If the upstream error is intentionally discarded (e.g., it is a raw YAML internal error that leaks implementation details), add a `// intentional: discard internal YAML error` comment to document the decision.

Run `make fmt && make lint && make test` to confirm no regressions.

---

Task 3: Adopt errgroup for Structured Concurrency in mcp_inspect_inspector.go

Priority: Medium
Estimated Effort: Medium
Focus Area: Concurrency Safety

Description: pkg/cli/mcp_inspect_inspector.go launches 2 goroutines (lines 146 and 213) with manual channel management and inline recover() calls. golang.org/x/sync/errgroup is already an indirect dependency of the module. Replacing the ad-hoc pattern with errgroup.WithContext would unify error propagation, remove the manual panic recovery, and make cancellation explicit.

Acceptance Criteria:

• pkg/cli/mcp_inspect_inspector.go uses errgroup.WithContext (from golang.org/x/sync/errgroup) instead of raw go func() with manual channels.
• The goroutine at line 213 (MCP server stderr consumer) is wrapped in an errgroup Go call.
• The goroutine at line 146 (per-server command goroutine) is wrapped in an errgroup Go call.
• make fmt && make test passes.

Code Region: pkg/cli/mcp_inspect_inspector.go

In `pkg/cli/mcp_inspect_inspector.go`, replace the two fire-and-forget `go func()` goroutines with `golang.org/x/sync/errgroup`. The module is already an indirect dependency.

1. Replace the goroutine at line ~146 (launched per MCP server command) with an `errgroup.Go` call.
2. Replace the goroutine at line ~213 (stderr consumer) similarly.
3. Use `errgroup.WithContext(ctx)` so cancellation propagates automatically.
4. Remove the ad-hoc `recover()` blocks that currently swallow panics silently.
5. Run `make fmt && make test`.

---

Task 4: Linter — Flag fmt.Errorf With Error Argument But No %w Verb

Priority: Medium
Estimated Effort: Medium
Focus Area: Linter Coverage

Description: The existing errorfwrapv linter catches fmt.Errorf("%v", err) (wrong verb). There is no linter for the complementary pattern: fmt.Errorf("message: "+err.Error()) or fmt.Errorf("...", someErr) where someErr is passed as a regular interface{} argument without using %w. Add a new analyzer errorfmterrconcatcheck (or extend errorfwrapv) that flags fmt.Errorf format strings where an error-typed argument is supplied as a positional arg but not via %w.

Acceptance Criteria:

• A new (or extended) linter in pkg/linters/ detects fmt.Errorf("foo %s", err) where err implements error but the corresponding verb is not %w.
• The linter is registered in pkg/linters/linters.go and pkg/linters/doc.go.
• A spec_test.go entry covers the new analyzer.
• make lint && make test passes.

Code Region: pkg/linters/errorfwrapv/, pkg/linters/linters.go, pkg/linters/doc.go

In the gh-aw repository, extend the `errorfwrapv` linter (in `pkg/linters/errorfwrapv/errorfwrapv.go`) to also flag:

- `fmt.Errorf("...", someErr)` where `someErr` implements `error` and the corresponding format verb is `%s`, `%q`, `%v`, or a raw string concat — not `%w`.

This catches the pattern where a developer accidentally formats an error as a string instead of wrapping it, destroying the error chain.

Steps:
1. Add detection logic in `errorfwrapv.go` for non-`%w` format verbs applied to `error`-typed arguments.
2. Update `testdata/src/errorfwrapv/` with positive and negative test cases.
3. Register (or update) the entry in `pkg/linters/doc.go`.
4. Add to `spec_test.go` if not already present.
5. Run `make fmt && make lint && make test`.

---

Task 5: Promote errstringmatch Suppressions to Typed Matches in schedule_preprocessing.go and update_extension_check.go

Priority: Low
Estimated Effort: Small
Focus Area: Error Chain Transparency

Description: Two remaining strings.Contains(err.Error(), ...) production callsites — pkg/workflow/schedule_preprocessing.go:194 ("syntax is not supported") and pkg/cli/update_extension_check.go:438 (dynamic msg variable) — suppress errstringmatch with no rationale comment. Add inline rationale comments explaining why typed matching is not possible (e.g., third-party library error without a typed sentinel) or replace with typed matches if feasible.

Acceptance Criteria:

• pkg/workflow/schedule_preprocessing.go:194 has either a // errstringmatch: third-party cron library returns plain string errors comment or is refactored to use errors.Is.
• pkg/cli/update_extension_check.go:438 has a rationale comment or is refactored.
• make lint passes with no new suppressions.

Code Region: pkg/workflow/schedule_preprocessing.go, pkg/cli/update_extension_check.go

In the gh-aw repository, add rationale comments to the remaining `//nolint:errstringmatch` suppressions in production code:

1. `pkg/workflow/schedule_preprocessing.go:194` — add a comment explaining why `strings.Contains(err.Error(), "syntax is not supported")` cannot use `errors.Is` (e.g., third-party cron parser returns untyped error).
2. `pkg/cli/update_extension_check.go:438` — similarly document why the dynamic `msg` variable check cannot be replaced with a typed match.

If on inspection either case *can* be replaced with a typed sentinel (e.g., the library exports an error type), do the replacement and remove the `nolint` directive.

Run `make lint && make test`.

---

📊 Historical Context

Previous Focus Areas

Date	Focus Area	Type	Custom	Key Outcomes
2026-07-16	Options Struct Pattern Adoption for High-Arity Functions	Custom	Y	2 funcs over threshold; 5 boolean-positional explosions
2026-07-15	Compile-Time Interface Satisfaction Assertions	Custom	Y	Zero var _ Interface guards across 21 interfaces
2026-07-14	Logger Name Collision & Instantiation Hygiene	Custom	Y	3 duplicate names; 6 var log shadows
2026-07-08	Deprecated API Alias Proliferation & Zombie Callsite Cleanup	Custom	Y	3 deprecated callsites; 4 always-true guards
2026-07-07	Environment Variable Access Fragmentation	Custom	Y	45 bare nolint suppressions; 6 duplicated env vars

---

🎯 Recommendations

Immediate Actions (This Week)

1. Task 1 (Sentinel Errors) — Priority: High — small, self-contained, eliminates 2 nolint suppressions
2. Task 2 (Parser YAML wrapping audit) — Priority: High — prevents silent cause discard in user-visible import errors

Short-term Actions (This Month)

1. Task 3 (errgroup in mcp_inspect_inspector) — Priority: Medium — improves cancellation propagation
2. Task 4 (Linter extension) — Priority: Medium — prevents regressions across the 788-call surface

Long-term Actions (This Quarter)

1. Task 5 (nolint rationale documentation) — Priority: Low — improves code clarity and auditability

---

📈 Success Metrics

• Non-wrapping fmt.Errorf ratio: 35% → <25% (eliminate accidental discard in parser/CLI paths)
• Production nolint:errstringmatch suppressions: 4 → ≤2 (with rationale comments)
• errgroup adoption: 0 → ≥2 goroutine sites
• Sentinel var ErrXxx vars: 4 → ≥6

---

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 3 months

---

References:

• Workflow run: §29583259978

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

Generated by ⚡ Repository Quality Improvement Agent · 80 AIC · ⌖ 6.39 AIC · ⊞ 6.6K · ◷

• expires on Jul 18, 2026, 5:27 AM UTC-08:00