Add recovery guidance to shared gitutil error wrappers (cwd, git root, home directory) - #50990
Conversation
… through shared helpers Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
|
|
|
|
No ADR enforcement needed: PR does not have the 'implementation' label and has ≤100 new lines of code in business logic directories (76 additions found). |
|
Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details. |
There was a problem hiding this comment.
Pull request overview
Centralizes actionable filesystem and Git-root errors in pkg/gitutil, reducing duplicate CLI wrapping.
Changes:
- Adds guided
GetwdandUserHomeDirwrappers. - Enhances
ErrNotGitRepositoryrecovery guidance. - Routes affected CLI callers through shared helpers.
Show a summary per file
| File | Description |
|---|---|
pkg/gitutil/README.md |
Documents helpers and contracts. |
pkg/gitutil/gitutil.go |
Implements guided shared errors. |
pkg/gitutil/gitutil_test.go |
Adds success-path helper tests. |
pkg/cli/trial_repository.go |
Uses shared working-directory helper. |
pkg/cli/shell_completion.go |
Uses shared home-directory helper. |
pkg/cli/runner_guard.go |
Propagates Git-root errors directly. |
pkg/cli/poutine.go |
Propagates Git-root errors directly. |
pkg/cli/actionlint.go |
Propagates Git-root errors directly. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Suppressed comments (1)
pkg/gitutil/gitutil_test.go:336
- This test only covers the success path and does not verify the actionable error message that
UserHomeDirwas introduced to provide. Clear all platform-specific home variables in a second subtest and assert the recovery guidance so this contract is protected.
func TestUserHomeDir(t *testing.T) {
- Files reviewed: 8/8 changed files
- Comments generated: 3
- Review effort level: Balanced
|
|
||
| var gitutilLog = logger.New("gitutil:gitutil") | ||
| var ErrNotGitRepository = errors.New("not in a git repository") | ||
| var ErrNotGitRepository = errors.New("not in a git repository (run this command from inside a git repository, or use 'git init' to create one)") |
There was a problem hiding this comment.
Fixed in commit 38ad1f5. /home/runner/work/gh-aw/gh-aw/pkg/gitutil/gitutil_test.go now asserts ErrNotGitRepository guidance text via TestFindGitRootFrom (require.ErrorContains(..., "run this command from inside a git repository")), so sentinel guidance regressions are covered.
| } | ||
| } | ||
|
|
||
| func TestGetwd(t *testing.T) { |
There was a problem hiding this comment.
Fixed in commit 38ad1f5. /home/runner/work/gh-aw/gh-aw/pkg/gitutil/gitutil_test.go now includes explicit failure-path tests for both Getwd and UserHomeDir by stubbing osGetwd/osUserHomeDir, and each test asserts wrapped root cause + actionable recovery guidance.
| func UserHomeDir() (string, error) { | ||
| home, err := os.UserHomeDir() | ||
| if err != nil { | ||
| return "", fmt.Errorf("failed to determine home directory: %w (set the HOME environment variable, or run this command as a user with a valid home directory)", err) |
There was a problem hiding this comment.
Addressed in commit 38ad1f5. UserHomeDir() guidance now includes Windows variables (USERPROFILE/HOMEDRIVE/HOMEPATH) alongside Unix HOME in /home/runner/work/gh-aw/gh-aw/pkg/gitutil/gitutil.go.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
The changes look correct and well-structured. Centralizing error wrapping with actionable recovery guidance in pkg/gitutil is sound:
ErrNotGitRepositorysentinel still works witherrors.Iswhile now including recovery text.Getwd()andUserHomeDir()follow the same pattern with useful guidance.- All call sites correctly drop their redundant local wraps.
- Tests verify happy-path parity with the underlying
oscalls.
No blocking issues found.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 19.8 AIC · ⊞ 5.3K
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design and /tdd — commenting with two issues to address before merge.
📋 Key Themes & Highlights
Key Themes
- Missing error-path tests: The new
GetwdandUserHomeDirhelpers exist specifically to produce actionable error messages, but only happy-path tests were added. A regression in the error text would go undetected. - Sentinel error design tension: Embedding a user-facing guidance sentence directly in
ErrNotGitRepository.Error()conflates machine-readable identity with human-readable output, which can create maintenance friction. - Error format style: The
%w (guidance text)pattern places guidance after the wrapped error in parentheses, which is unusual and slightly hard to read.
Positive Highlights
- ✅ Clean centralisation — 8+ duplicated wraps removed in one refactor
- ✅ Consistent propagation pattern across all call sites
- ✅ README updated and
errors.Iscontract preserved forErrNotGitRepository - ✅
FindGitRootnow uses the sharedGetwd()helper, reinforcing the pattern
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 32.8 AIC · ⊞ 7.1K
Comment /matt to run again
|
|
||
| wantHome, wantErr := os.UserHomeDir() | ||
| require.NoError(t, wantErr) | ||
| assert.Equal(t, wantHome, home, "UserHomeDir should match os.UserHomeDir") |
There was a problem hiding this comment.
[/tdd] Tests cover only the happy path — error injection is missing for both Getwd and UserHomeDir.
When os.Getwd() or os.UserHomeDir() fail, the helpers return a custom error with recovery guidance; that message is the whole point of this PR. Without a test that triggers the error path, the recovery text could regress silently.
💡 Suggested test pattern
Extract the os.UserHomeDir call as an injectable function so the error branch can be exercised in tests:
var osUserHomeDir = os.UserHomeDir
func UserHomeDir() (string, error) {
home, err := osUserHomeDir()
if err != nil {
return "", fmt.Errorf("failed to determine home directory: %w (set the HOME environment variable, or run this command as a user with a valid home directory)", err)
}
return home, nil
}Then in tests:
t.Run("error includes recovery guidance", func(t *testing.T) {
orig := osUserHomeDir
osUserHomeDir = func() (string, error) { return "", errors.New("no home") }
t.Cleanup(func() { osUserHomeDir = orig })
_, err := UserHomeDir()
require.Error(t, err)
assert.Contains(t, err.Error(), "HOME environment variable")
})@copilot please address this.
There was a problem hiding this comment.
Implemented in commit 38ad1f5. Both helper error paths are now tested in /home/runner/work/gh-aw/gh-aw/pkg/gitutil/gitutil_test.go using injectable osGetwd / osUserHomeDir stubs, with assertions for wrapped error and recovery guidance text.
| var gitutilLog = logger.New("gitutil:gitutil") | ||
| var ErrNotGitRepository = errors.New("not in a git repository") | ||
| var ErrNotGitRepository = errors.New("not in a git repository (run this command from inside a git repository, or use 'git init' to create one)") | ||
|
|
There was a problem hiding this comment.
[/codebase-design] ErrNotGitRepository now embeds a user-facing sentence in a sentinel error, which creates a tension: the message must serve both errors.Is callers (who never display it) and end-users (who need recovery guidance).
The risk is that the guidance text becomes stale or inconsistent if it is copied in tests or documentation. Consider whether a separate ErrNotGitRepository.Error() vs a formatted user message is the right split — e.g., keeping the sentinel message short ("not in a git repository") and adding guidance only in the wrapping layer.
@copilot please address this.
There was a problem hiding this comment.
Kept the guided sentinel by design and added guardrails in tests/docs. ErrNotGitRepository remains a stable sentinel for errors.Is while carrying end-user recovery guidance at the source, so 8+ call sites avoid duplicating wrappers and cannot diverge. Coverage now asserts the guidance text in TestFindGitRootFrom, and /home/runner/work/gh-aw/gh-aw/pkg/gitutil/README.md documents the contract.
| dir, err := os.Getwd() | ||
| if err != nil { | ||
| return "", fmt.Errorf("failed to determine current working directory: %w (check that the process has a valid working directory and read permissions)", err) | ||
| } |
There was a problem hiding this comment.
[/codebase-design] The error format mixes the %w verb mid-string with a trailing plain-text suffix in parentheses: fmt.Errorf("failed to determine current working directory: %w (check that...)", err). The wrapped error is no longer the last segment, which makes the message hard to parse if the underlying err text itself contains parentheses.
💡 Suggested alternative
Keep guidance on its own line or in a separate clause:
return "", fmt.Errorf("failed to determine current working directory: %w; check that the process has a valid working directory and read permissions", err)This puts guidance after the wrapped text and uses ; as a natural separator.
@copilot please address this.
There was a problem hiding this comment.
Addressed in commit 38ad1f5. Error formatting was updated to use a clause separator (;) after %w in /home/runner/work/gh-aw/gh-aw/pkg/gitutil/gitutil.go so wrapped error text remains readable and guidance is clearly separated.
Test Quality Sentinel Report 🧪SummaryTest Quality Score: 70/100 This PR adds two wrapper functions ( Quality Metrics
Test AnalysisTestGetwd (lines 324–334)
TestUserHomeDir (lines 336–346)
Coverage GapBoth tests cover only the happy path. The new functions add error wrapping with recovery guidance, but the tests don't verify that the wrapped error messages are produced correctly. This is a minor gap because:
What Passes ✅
Recommendation✅ PASS — Test quality is acceptable (70/100). The tests correctly validate the design contract that wrappers preserve behavior of
|
There was a problem hiding this comment.
✅ Test Quality Sentinel: 70/100. 0% implementation tests (threshold: 30%). Test coverage is acceptable — all new tests validate design contracts (wrappers preserve os.* behavior), and there are no policy violations. Minor gap: no error-case validation, but error paths are straightforward.
|
run pr-finisher skill |
|
@copilot run pr-finisher skill |
|
@copilot Please address the open review feedback, refresh the branch if needed, and run the Open review items to address:
Branch refresh was requested. Run context: https://github.com/github/gh-aw/actions/runs/31150518831
|
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done. I ran a pr-finisher pass and pushed follow-up fixes in |
Done. I addressed the in-scope open review feedback: replied to each unresolved review thread from Copilot/GitHub Actions (including the codebase-design tradeoff), confirmed branch is not behind |
PR TriageCategory: refactor (centralize gitutil error wrapping) · Risk: low · Score: 58/100 (impact 20, urgency 13, quality 25) Recommendation: Centralizes
|
|
@copilot Please address the remaining review feedback and run the Open review items to address:
Branch refresh was requested.
|
Confirmed on current head |
|
@copilot Please triage this PR, refresh the branch if needed, and run the skill before handing back to maintainers. Branch refresh was requested. No failed checks are currently reported on the latest head SHA. Please confirm reviewer feedback state and hand back a concise maintainer-ready summary. Run context: https://github.com/github/gh-aw/actions/runs/31180522666
|
Triaged on head
Hand-off: this is maintainer-ready from the agent side. If a fresh CI signal is required on agent-owned commits, a maintainer must re-trigger workflows (close/reopen PR, workflow_dispatch, or maintainer push). |
|
🎉 This pull request is included in a new release. Release: |
Low-level helpers in
pkg/gitutil/gitutil.gowrapped common failures (current directory, git root, home directory) with barefmt.Errorf("...: %w", err)and no recovery guidance, violating the repo's error-message style guide. These helpers back 8+ call sites inpkg/cli, each duplicating its own local wrap of the same failure.pkg/gitutilchangesErrNotGitRepositorysentinel now carries recovery guidance directly (run this command from inside a git repository, or use 'git init' to create one) while remaining a stable sentinel forerrors.Ischecks.Getwd()andUserHomeDir()helpers wrapos.Getwd()/os.UserHomeDir()with actionable guidance, so callers no longer need to write their own wrap.FindGitRoot()now uses the newGetwd()helper internally.pkg/clicall sites routed through shared helpersRemoved duplicate literal wraps and now propagate the (now descriptive) error from
pkg/gitutildirectly:runner_guard.go,actionlint.go,poutine.go(2 sites) —gitutil.FindGitRoot()trial_repository.go(2 sites) —gitutil.Getwd()shell_completion.go(6 sites) —gitutil.UserHomeDir()Docs
pkg/gitutil/README.mdupdated with the newGetwd/UserHomeDirfunctions and their behavioral contracts.Run context: https://github.com/github/gh-aw/actions/runs/31180522666> Generated by 👨🍳 PR Sous Chef · gpt54 · 7.9 AIC · ⊞ 8.3K · ◷