chore: improved log message for failing hooks#5603
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRefactors GCP credential handling, changes impersonation option composition, introduces formatted hook execution error messages with unit tests, wraps tflint execution errors to preserve underlying errors, and adds a hook-exit-code fixture plus an integration test asserting non-zero hook reporting. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
denis256
force-pushed
the
5596-hook-eror-message
branch
from
50ff568 to
92cf5d1
Compare
yhakbar
reviewed
Feb 26, 2026
internal/tflint/tflint.go
Outdated
| func (err ErrorRunningTflint) Error() string { | ||
| return fmt.Sprintf("Error while running tflint with args: %v", err.args) | ||
| if err.Err != nil { | ||
| return fmt.Sprintf("Error while running tflint with args: %v: %s", err.Args, err.Err) |
Collaborator
There was a problem hiding this comment.
Suggested change
| return fmt.Sprintf("Error while running tflint with args: %v: %s", err.Args, err.Err) | |
| return fmt.Sprintf("Error encountered while running tflint with args: '%v': %s", err.Args, err.Err) |
yhakbar
reviewed
Feb 26, 2026
internal/tflint/tflint.go
Outdated
| return fmt.Sprintf("Error while running tflint with args: %v: %s", err.Args, err.Err) | ||
| } | ||
|
|
||
| return fmt.Sprintf("Error while running tflint with args: %v", err.Args) |
Collaborator
There was a problem hiding this comment.
Suggested change
| return fmt.Sprintf("Error while running tflint with args: %v", err.Args) | |
| return fmt.Sprintf("Error encountered while running tflint with args: '%v'", err.Args) |
yhakbar
reviewed
Feb 26, 2026
internal/runner/run/hook.go
Outdated
| } | ||
|
|
||
| if exitCodeErr == nil && output != "" { | ||
| return fmt.Sprintf("Hook %q (command: %s) failed with exit code %d:\n%s", hookName, cmd, exitCode, output) |
Collaborator
There was a problem hiding this comment.
Suggested change
| return fmt.Sprintf("Hook %q (command: %s) failed with exit code %d:\n%s", hookName, cmd, exitCode, output) | |
| return fmt.Sprintf("Hook %q (command: %s) exited with non-zero exit code %d:\n%s", hookName, cmd, exitCode, output) |
yhakbar
reviewed
Feb 26, 2026
internal/runner/run/hook.go
Outdated
| return fmt.Sprintf("Hook %q (command: %s) failed with exit code %d:\n%s", hookName, cmd, exitCode, output) | ||
| } | ||
|
|
||
| if exitCodeErr == nil { |
yhakbar
reviewed
Feb 26, 2026
internal/runner/run/hook.go
Outdated
| } | ||
|
|
||
| if exitCodeErr == nil { | ||
| return fmt.Sprintf("Hook %q (command: %s) failed with exit code %d", hookName, cmd, exitCode) |
Collaborator
There was a problem hiding this comment.
Suggested change
| return fmt.Sprintf("Hook %q (command: %s) failed with exit code %d", hookName, cmd, exitCode) | |
| return fmt.Sprintf("Hook %q (command: %s) exited with non-zero exit code %d", hookName, cmd, exitCode) |
yhakbar
reviewed
Feb 26, 2026
internal/runner/run/hook.go
Outdated
| } | ||
| } | ||
|
|
||
| return fmt.Sprintf("Hook %q failed: %s", hookName, err.Error()) |
Collaborator
There was a problem hiding this comment.
Suggested change
| return fmt.Sprintf("Hook %q failed: %s", hookName, err.Error()) | |
| return fmt.Sprintf("Hook %q failed to execute: %s", hookName, err.Error()) |
Assuming this is correct. I'm not sure about that.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
test/integration_hooks_test.go (1)
480-483: Strengthen this test to assert hook name and command context too.The new assertions cover exit code + output, but the PR goal also includes clearer hook identity/command context. Add assertions for those substrings so regressions in message context are caught.
Suggested assertion extension
output := stderr.String() // Error message should show exit code and the actual hook output + assert.Contains(t, output, `Hook "hook_exit_nonzero"`) + assert.Contains(t, output, "command:") assert.Contains(t, output, `exited with non-zero exit code 2`) assert.Contains(t, output, "lint warning: something is wrong")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/integration_hooks_test.go` around lines 480 - 483, The test currently asserts the exit code and hook output but misses checking hook identity and command context; update the assertions around the local variable output (in test/integration_hooks_test.go) to also assert that output contains the hook name (e.g., the hook identifier string used in the test) and the executed command context (e.g., the command invocation or arguments shown in logs), by adding two assert.Contains calls referencing the same output variable so the failure message includes both the hook name and the command string.internal/runner/run/hook.go (1)
41-41: Render command arguments with quoting to avoid ambiguous diagnostics.Joining raw args with spaces can misrepresent commands when args contain spaces/quotes. Quoting each token makes the logged command deterministic and copy/paste-safe.
Suggested update
import ( "context" "fmt" + "strconv" "slices" "strings" "sync" @@ - cmd := strings.Join(append([]string{processErr.Command}, processErr.Args...), " ") + parts := append([]string{processErr.Command}, processErr.Args...) + for i := range parts { + parts[i] = strconv.Quote(parts[i]) + } + cmd := strings.Join(parts, " ")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/runner/run/hook.go` at line 41, The logged command is built by joining processErr.Command and processErr.Args raw which misrepresents tokens containing spaces or quotes; update the construction of cmd (where processErr.Command and processErr.Args are used) to quote each token deterministically (e.g., map strconv.Quote over the command and each arg or otherwise shell-escape each token) and then join the quoted tokens with spaces so the diagnostic is copy/paste-safe and unambiguous.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@internal/runner/run/hook.go`:
- Line 41: The logged command is built by joining processErr.Command and
processErr.Args raw which misrepresents tokens containing spaces or quotes;
update the construction of cmd (where processErr.Command and processErr.Args are
used) to quote each token deterministically (e.g., map strconv.Quote over the
command and each arg or otherwise shell-escape each token) and then join the
quoted tokens with spaces so the diagnostic is copy/paste-safe and unambiguous.
In `@test/integration_hooks_test.go`:
- Around line 480-483: The test currently asserts the exit code and hook output
but misses checking hook identity and command context; update the assertions
around the local variable output (in test/integration_hooks_test.go) to also
assert that output contains the hook name (e.g., the hook identifier string used
in the test) and the executed command context (e.g., the command invocation or
arguments shown in logs), by adding two assert.Contains calls referencing the
same output variable so the failure message includes both the hook name and the
command string.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
internal/gcphelper/config.gointernal/runner/run/hook.gointernal/runner/run/hook_internal_test.gointernal/tflint/tflint.gotest/integration_hooks_test.go
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/gcphelper/config.go (1)
104-118: Good fix for impersonation, but the comment is now stale.Passing
clientOpts...toCredentialsTokenSourcecorrectly allows impersonation to use explicitly provided credentials as the source identity, rather than always relying on ADC. This is the right behavior.However, the comment on lines 105-107 now contradicts the implementation — it states the impersonate library "uses Application Default Credentials internally as the source identity," but with this change, it uses the gathered
clientOptsfirst (falling back to ADC only if none were provided).📝 Suggested comment update
// Handle service account impersonation. - // When impersonation is configured, the impersonation token source replaces - // any base credentials. The impersonate library uses Application Default - // Credentials internally as the source identity. + // When impersonation is configured, the impersonation token source replaces + // any base credentials. The impersonate library uses the provided credentials + // (or Application Default Credentials if none were provided) as the source identity. if gcpCfg != nil && gcpCfg.ImpersonateServiceAccount != "" {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/gcphelper/config.go` around lines 104 - 118, Update the stale comment above the impersonation block to reflect the actual behavior: note that when gcpCfg.ImpersonateServiceAccount is set we call impersonate.CredentialsTokenSource with the current clientOpts so the impersonation token source will use the explicitly provided credentials as the source identity (falling back to ADC only if no clientOpts are supplied), instead of claiming the impersonate library always uses Application Default Credentials.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@internal/gcphelper/config.go`:
- Around line 104-118: Update the stale comment above the impersonation block to
reflect the actual behavior: note that when gcpCfg.ImpersonateServiceAccount is
set we call impersonate.CredentialsTokenSource with the current clientOpts so
the impersonation token source will use the explicitly provided credentials as
the source identity (falling back to ADC only if no clientOpts are supplied),
instead of claiming the impersonate library always uses Application Default
Credentials.
Resolve conflict in internal/gcphelper/config.go: use `env` variable with two-line pattern to keep envCreds in outer scope. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
yhakbar
approved these changes
Mar 3, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
When a hook (like tflint or a custom linter) exits with a non-zero code, the error message now shows the command, exit code, and actual output instead of the generic "Failed to execute ...
exit status 2".
Fixes #5596.
TODOs
Read the Gruntwork contribution guidelines.
Release Notes (draft)
Added / Removed / Updated [X].
Migration Guide
Summary by CodeRabbit
New Features
Bug Fixes
Tests