[repository-quality] 🎯 Repository Quality Improvement Report - Error Message & Diagnostics Quality #50886
Closed
Replies: 1 comment
|
This discussion was automatically closed because it expired on 2026-08-07T13:27:19.318Z.
|
0 replies
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Uh oh!
There was an error while loading. Please reload this page.
Analysis Date: 2026-08-06
Focus Area: Error Message & Diagnostics Quality
Strategy Type: Custom
Custom Area: Yes — gh-aw already codifies an error-message style guide (
.github/skills/error-messages/SKILL.md) requiring "what's wrong / what's expected / example" and constructive phrasing, but a repo-wide audit shows widespread drift from that guide acrosspkg/cli,pkg/workflow, andpkg/parser. Since gh-aw error output is the primary UX surface for CLI users debugging failing workflow compiles, closing this gap has outsized value.Executive Summary
gh-aw defines 2,403
fmt.Errorfcall sites acrosspkg/parser,pkg/workflow, andpkg/cli, plus 106 uses of the structuredNewValidationErrorhelper. The repository's own style guide asks contributors to avoid generic wrappers likefmt.Errorf("failed to X: %w", err)unless recovery guidance is added, and to makeinvalid ...messages constructive (state what's expected and give an example). A scan found 257 call sites matching the discouraged "failed to<action>: %w" generic-wrapper pattern (e.g.,"failed to marshal JSON: %w"appears 13 times,"failed to get current directory: %w"8 times,"failed to find git root: %w"8 times), and 215 bareinvalid ...messages that do not mention "expected", "for example", or a ✓ marker as the guide requires.These are heavily concentrated in
pkg/cli(176 files touch the generic-wrapper pattern) versuspkg/workflow(66) andpkg/parser(14) — meaning the CLI's user-facing surface is where drift is worst, directly impacting first-run and debugging UX forgh aw compile/gh aw run. Several of the worst offenders are duplicated verbatim across many files (e.g., "failed to get current directory" appears 8 times, "failed to find git root" 8 times), suggesting a shared helper could both de-duplicate the logic and improve messages once instead of per call-site.Recommended actions: (1) introduce small reusable error helpers in
pkg/gitutilandpkg/clifor the most duplicated low-value wraps (current directory, git root, home directory, temp directory) that add one-line recovery guidance; (2) run a targeted sweep of theinvalid %s: %w/ bareinvalidmessages inpkg/workflowvalidation code to convert them toNewValidationErrorwith asuggestionfield per the guide; (3) add a lightweight CI/lint check (or extend an existing custom linter) that flags newfmt.Errorf("failed to ...: %w")additions without accompanying context, nudging future PRs toward compliance.Full Analysis Report
Focus Area: Error Message & Diagnostics Quality
Current State Assessment
The codebase has an explicit style guide (
.github/skills/error-messages/SKILL.md) mandating:[what's wrong]. [what's expected]. [example], constructive (non-bare-negative) phrasing, and use ofNewValidationError(field, value, reason, suggestion)for config validation versusfmt.Errorffor operational/wrapping errors. This audit measured actual compliance against that guide using static grep-based heuristics (may include some false positives, e.g., legitimately unavoidable low-level I/O wraps).Metrics Collected:
fmt.Errorfcall sites (pkg/parser, pkg/workflow, pkg/cli)NewValidationErrorstructured-error usages"failed to X: %w"wrapper call sites (discouraged per guide)invalid ...messages missing "expected"/"for example"/✓pkg/cliwith generic-wrapper patternpkg/workflowwith generic-wrapper patternpkg/parserwith generic-wrapper pattern"failed to marshal JSON: %w""failed to get current directory: %w""failed to find git root: %w"Findings
Strengths
error-messagesskill) with a clear template and a table contrasting "avoid" vs "prefer" phrasing.NewValidationErrorhelper, showing an established pattern to extend rather than invent.pkg/parseris comparatively clean (only 14 files with generic wrappers), suggesting the pattern is enforceable — it's mainlypkg/clithat lags.Areas for Improvement
"failed to X: %w"wrappers with zero recovery guidance, concentrated inpkg/cli(the primary user-facing surface).invalid ...errors that don't state the expected format or give an example, violating the "what's expected"/"example" requirements of the guide..github/skills/go-linters/SKILL.mdcould catch this going forward).Detailed Analysis
Representative offending call sites (file:line):
pkg/cli/outcomes_command.go:196,pkg/cli/deps_report.go:249,pkg/cli/run_workflow_execution.go:612—fmt.Errorf("failed to marshal JSON: %w", err)pkg/gitutil/gitutil.go:134,pkg/cli/trial_repository.go:257,663—fmt.Errorf("failed to get current directory: %w", err)pkg/cli/runner_guard.go:62,pkg/cli/actionlint.go:305,pkg/cli/poutine.go:83—fmt.Errorf("failed to find git root: %w", err)pkg/workflow/repo_config.go:357—fmt.Errorf("invalid %s: %w", RepoConfigFileName, err)(bare, no expected-format guidance)Because
pkg/gitutilcentralizes low-level git helpers used by manypkg/clicommands, adding one-line recovery context there (e.g., "failed to determine current working directory — check that the process has a valid working directory and file-system permissions") would immediately upgrade dozens of downstream call sites without touching each file.🤖 Tasks for Copilot Agent
NOTE TO PLANNER AGENT: Split the following tasks into individual work items.
Improvement Tasks
Task 1: Add recovery guidance to shared gitutil error wrappers
Priority: High
Estimated Effort: Small
Focus Area: Error Message & Diagnostics Quality
Description: Update the shared helpers in
pkg/gitutil/gitutil.go(and any equivalent helper inpkg/cli) that wrap "failed to get current directory", "failed to find git root", and "failed to get home directory" errors so each includes a short actionable recovery hint per.github/skills/error-messages/SKILL.md, instead of bare%wwraps. Since these are shared low-level helpers, this single change propagates improved messages to dozens of call sites (8+ occurrences each) acrosspkg/cli.Acceptance Criteria:
pkg/gitutil/gitutil.go:134and equivalent call sites use a message following[what's wrong]. [what's expected/how to fix]formatfmt.Errorf)make test-unitpasses with no regressions in tests asserting on old error textCode Region:
pkg/gitutil/gitutil.go,pkg/cli/runner_guard.go:62,pkg/cli/actionlint.go:305,pkg/cli/poutine.go:83In pkg/gitutil/gitutil.go, find the function(s) that return fmt.Errorf("failed to get current directory: %w", err) and fmt.Errorf("failed to find git root: %w", err) (or similarly named git-root lookup helpers). Update these error messages to follow the repo's error message style guide in .github/skills/error-messages/SKILL.md: state what went wrong plus a short actionable recovery hint, e.g. fmt.Errorf("failed to determine current working directory: %w (check that the process has a valid working directory and read permissions)", err) and fmt.Errorf("failed to find git repository root: %w (run this command from inside a git repository, or use 'git init')", err). Then search pkg/cli for other files calling similar patterns (e.g. pkg/cli/trial_repository.go, pkg/cli/actionlint.go, pkg/cli/poutine.go, pkg/cli/runner_guard.go) and, where they wrap the same underlying failure locally instead of calling the shared gitutil helper, route them through the shared helper so the improved message is reused rather than duplicated. Run `go build ./...` and `make test-unit` to confirm no regressions, and run `make fmt` after editing Go files.Task 2: Deduplicate and improve "failed to marshal JSON" error wraps
Priority: Medium
Estimated Effort: Small
Focus Area: Error Message & Diagnostics Quality
Description: 13 call sites across
pkg/cli(e.g.,outcomes_command.go:196,deps_report.go:249,run_workflow_execution.go:612) use the identical bare wrapperfmt.Errorf("failed to marshal JSON: %w", err). Introduce a small shared helper function (e.g.,marshalJSONOrError(v any, context string) ([]byte, error)) in a suitable sharedpkg/cliutility file that produces a context-specific, actionable error message, and replace the duplicated call sites with calls to it.Acceptance Criteria:
make test-unitpassesCode Region:
pkg/cli/outcomes_command.go:196,pkg/cli/deps_report.go:249,pkg/cli/run_workflow_execution.go:612Search pkg/cli for all occurrences of the exact string fmt.Errorf("failed to marshal JSON: %w", err) (13 occurrences reported, including pkg/cli/outcomes_command.go:196, pkg/cli/deps_report.go:249, and pkg/cli/run_workflow_execution.go:612). Introduce a small shared helper in an appropriate existing pkg/cli utility file (do not create a new file if a suitable one already exists, e.g. a general "helpers" or "utils" file) such as: func marshalJSONOrWrap(v any, context string) ([]byte, error) { data, err := json.Marshal(v) if err != nil { return nil, fmt.Errorf("failed to marshal %s to JSON: %w", context, err) } return data, nil } Then replace each of the 13 call sites with a call to this helper, passing a short descriptive context string (e.g. "outcomes report", "dependency report", "run execution result") appropriate to that call site, per .github/skills/error-messages/SKILL.md guidance on actionable, specific error messages. Update any unit tests asserting on the old literal error string. Run `make fmt` and `make test-unit` to validate.Task 3: Convert bare
invalid %s: %wvalidation errors in pkg/workflow to NewValidationErrorPriority: High
Estimated Effort: Medium
Focus Area: Error Message & Diagnostics Quality
Description:
pkg/workflowcontains validation error sites such aspkg/workflow/repo_config.go:357(fmt.Errorf("invalid %s: %w", RepoConfigFileName, err)) that report a bare "invalid" without stating the expected format or an example, violating.github/skills/error-messages/SKILL.md. Sincepkg/workflowalready has 106 uses of the structuredNewValidationError(field, value, reason, suggestion)helper elsewhere in the codebase, migrate the bareinvalid %s: %wsites in*_validation.go-style logic (and closely related validation call sites) to useNewValidationErrorwith a concretesuggestionexample.Acceptance Criteria:
pkg/workflow/repo_config.go:357and similar bareinvalid %s: %wsites in validation-focused files are converted toNewValidationError(or, whereNewValidationErrordoesn't fit because it's a lower-level wrap, given an explicit expected-format/example suffix)make test-unitpassesCode Region:
pkg/workflow/repo_config.go:357and other*_validation.gofiles with bareinvalid %s: %wpatternsTask 4: Add a custom linter check for the discouraged generic error-wrap pattern
Priority: Medium
Estimated Effort: Medium
Focus Area: Error Message & Diagnostics Quality
Description: To prevent regression of the 257 generic
"failed to X: %w"wrappers found in this audit, add a custom Go analyzer (per the existing.github/skills/go-linters/SKILL.mdpattern for adding analyzers underpkg/linters) that flags newfmt.Errorfcalls whose format string starts with"failed to "and ends with": %w"with no additional context/recovery text, so future PRs are nudged toward the style guide rather than silently accumulating more of the same pattern.Acceptance Criteria:
pkg/lintersfollowing the conventions in.github/skills/go-linters/SKILL.md"failed to X: %w"patterns without additional contextCode Region:
pkg/linters/(new analyzer file)Following the pattern documented in .github/skills/go-linters/SKILL.md and .github/skills/pr-to-go-linter/SKILL.md, add a new custom Go static analyzer under pkg/linters that detects fmt.Errorf calls whose literal format string matches the discouraged generic-wrapper pattern described in .github/skills/error-messages/SKILL.md: a format string starting with "failed to " and ending with ": %w" with no additional recovery-guidance text in between beyond a short action description (e.g. fmt.Errorf("failed to get current directory: %w", err) should be flagged; fmt.Errorf("failed to find git repository root: %w (run this command from inside a git repository)", err) should NOT be flagged because it contains additional guidance). Implement the analyzer using the go/analysis framework consistent with existing analyzers in pkg/linters (look at an existing analyzer file there for the boilerplate: package structure, Analyzer var, run function using go/ast inspection of ast.CallExpr nodes for fmt.Errorf). Add unit tests with a small testdata Go file containing both a flagged case and a passing case, following the existing test conventions in pkg/linters. Given the audit found 257 existing occurrences, wire the analyzer to be opt-in or run in report-only/warn mode initially (do not fail existing CI) so it acts as a guardrail against new occurrences without requiring an immediate mass rewrite; document this baseline decision in the analyzer's doc comment. Run `make fmt` and existing linter test commands (e.g. via `make lint` or the project's Go test invocation for pkg/linters) to validate.📊 Historical Context
Previous Focus Areas
🎯 Recommendations
Immediate Actions (This Week)
gitutilerror wrappers (Task 1) — Priority: Highinvalid %s: %wvalidation errors inpkg/workflowtoNewValidationError(Task 3) — Priority: HighShort-term Actions (This Month)
Long-term Actions (This Quarter)
NewValidationErrormigration across all ofpkg/workflowandpkg/clivalidation logic, driving the 215 bare-invalid count toward zero — Priority: Low📈 Success Metrics
"failed to X: %w"wrapper call sites: 257 → < 100 (targeted reduction via shared helpers + linter guardrail)invalid ...messages missing expected/example: 215 → < 100NewValidationErrorusage: 106 → 150+ (increased adoption for config validation)Next Steps
Generated by Repository Quality Improvement Agent
Next analysis: 2026-08-07 — Focus area selected by diversity algorithm
All reactions