fix(hook): remove dup output and preserve check_first diagnostics by nkakouros · Pull Request #784 · jdx/hk

nkakouros · 2026-03-29T22:24:53Z

Summary

Remove handle_script_failed output dump from friendly_error in main.rs. The per-step output_by_step summary already displays all failing steps' output — friendly_error was printing the first failing step's combined output again, causing duplication and misattribution (e.g., ghalint output appearing to be part of zizmor's section).
Preserve check_first diagnostic output when cancelled by fail_fast. For check_diff/check_list_files steps, the check output was lost if another step failed first (triggering cancellation before the second run).

Context

When multiple steps fail, the end-of-run output looked like:

ghalint-workflow stderr:
[ghalint errors]

zizmor stderr:
[zizmor output]
Error running ghalint run     ← from handle_script_failed, looks like part
of zizmor
[ghalint errors repeated]     ← duplicate

handle_script_failed only showed the first ScriptFailed in the error chain, so with fail_fast=false only one step's output was repeated while others were ignored.

Test plan

test/output_bugs_repro.bats — 4 regression tests covering duplicate
output, multi-step failures, stdout output_summary, and HK_SUMMARY_TEXT
Existing test/output_summary.bats and
test/output_summary_check_first.bats pass
cargo test — 152 passed
Full test:bats suite — no new failures

🤖 Generated with Claude Code

gemini-code-assist

Code Review

This pull request refactors script failure handling to ensure consistent output and preserve diagnostics during early exits. It removes the redundant handle_script_failed function, updates write_output_file to handle its own errors, and introduces logic in the execution step to save check outputs if a hook is cancelled by fail_fast. New tests verify that error messages are not duplicated in the summary. Feedback was provided to harden write_output_file against potential panics when resolving parent directories and to optimize string allocations when stripping ANSI codes.

greptile-apps · 2026-03-29T22:29:51Z

Greptile Summary

This PR fixes two output correctness issues: it removes the handle_script_failed call from friendly_error that was re-printing the first failing step's output after output_by_step had already shown it, and it preserves check_first diagnostic output when a fail_fast cancellation races ahead of the second run. The combined field is added to CheckListFailed to supply complete output to save_output_summary.

Confidence Score: 5/5

Safe to merge — all prior review concerns are resolved and no new issues found.

All P0/P1 concerns from the previous review round (combined interleaved output, silent error demotion, redundant CLI flag) are fixed. The let-chain syntax used in execution.rs is valid since the project pins rust-version = 1.88.0. No new defects introduced.

No files require special attention.

Important Files Changed

Filename	Overview
src/main.rs	Removes handle_script_failed and its duplicate output; friendly_error now only writes output.log and exits for ScriptFailed errors
src/error.rs	Adds combined field to CheckListFailed variant to carry interleaved output alongside stdout/stderr
src/step/execution.rs	Stashes check_first output and saves diagnostics via save_output_summary when the hook is cancelled (fail_fast) before the second run
src/step/runner.rs	Passes combined_output when constructing CheckListFailed, completing the data flow needed for the new combined field
test/output_summary_no_duplicate.bats	New regression tests for single and multi-step duplicate output; two tests present covering the core scenarios

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[Step with check_first] --> B[Run check / check_list_files]
    B -->|Ok| C[Return early — nothing to fix]
    B -->|CheckListFailed| D[Stash check_first_output\nstdout + stderr + combined]
    D --> E{fail_fast cancelled?}
    E -->|Yes| F[save_output_summary\nwith check diagnostics\nis_failure = true]
    E -->|No| G[Run fixer / second pass]
    G -->|Ok| H[save_output_summary\nsecond-run output]
    G -->|Err| I[status_errored\nreturn Err]

    J[friendly_error] --> K{ScriptFailed\nin chain?}
    K -->|Yes| L[write_output_file\nprocess::exit]
    K -->|No| M[Return Err —\nfull eyre trace]

_{Reviews (3): Last reviewed commit: "Merge branch 'main' into fix/friendly-er..." | Re-trigger Greptile}

…heck_first output Three fixes for step output display: 1. Remove handle_script_failed output dump from friendly_error. The per-step output_by_step summary already shows all failing steps' output. friendly_error was duplicating the first failing step's output after the summary, causing misattribution (e.g., ghalint output appearing to be part of zizmor's section). 2. Preserve check_first diagnostic output when cancelled by fail_fast. For check_diff/check_list_files steps with check_first, the check output was lost if another step failed first (triggering fail_fast cancellation before the second run). Now the check output is stashed and saved to the summary if the second run is skipped. 3. Add regression tests for output display bugs. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>

PR #772 forced Combined output on failure to avoid losing diagnostics, but this changed the summary header label (e.g., "lint output:" instead of "lint stderr:"), breaking #784's tests. Use combined content on failure but keep the configured label. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>

## Summary - PR #772 forced `Combined` output on failure to avoid losing diagnostics, but this changed the summary header label (e.g., `lint output:` instead of `lint stderr:`), breaking #784's tests. - Uses combined *content* on failure (preserving #772's intent) but keeps the configured *label* so the header matches `output_summary`. - Fixes CI failures on #806. ## Test plan - [x] `test/output_summary_no_duplicate.bats` passes locally (both tests) - [ ] CI passes on this PR 🤖 Generated with [Claude Code](https://claude.com/claude-code)  --- > [!NOTE] > **Low Risk** > Low risk, small change limited to end-of-run output summarization; main risk is subtle behavior differences in how failure output is labeled (and stored) for downstream consumers/tests. > > **Overview** > On step failure, the summary now saves **combined output content** to avoid losing diagnostics, but preserves the step’s configured `output_summary` *mode/label* instead of forcing `Combined`. > > `output_summary = "hide"` is still respected even on failure, preventing any summary output from being recorded. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit f0090e4. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup>  Co-authored-by: Claude Opus 4.6 (1M context) <[email protected]>

@nkakouros

### 🐛 Bug Fixes - **(hook)** remove dup output and preserve check_first diagnostics by [@nkakouros](https://github.com/nkakouros) in [#784](#784) - **(hook)** show combined output for failed steps by [@nkakouros](https://github.com/nkakouros) in [#772](#772) - **(hook)** preserve configured output_summary label on failure by [@jdx](https://github.com/jdx) in [#808](#808) ### 📦️ Dependency Updates - update rust crate pklr to 0.4.1 by [@jhult](https://github.com/jhult) in [#805](#805) ### New Contributors - @jhult made their first contribution in [#805](#805)  --- > [!NOTE] > **Low Risk** > Primarily a version bump and doc/metadata regeneration, plus routine dependency lockfile updates; minimal behavioral risk beyond potential upstream crate changes. > > **Overview** > Bumps `hk` to **v1.41.1** and adds the corresponding `CHANGELOG.md` release entry. > > Regenerates/version-aligns published artifacts and docs (CLI docs JSON/markdown, `hk.usage.kdl`, and all docs/examples that reference the release `package://.../v1.41.1` URLs). > > Updates `Cargo.lock` for dependency bumps (notably `pklr 0.4.1`, `indexmap 2.14.0`/`hashbrown 0.17.0`, and `tokio 1.51.1`). > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit 0a2a092. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup>  Co-authored-by: mise-en-dev <[email protected]>

This MR contains the following updates: | Package | Update | Change | |---|---|---| | [hk](https://github.com/jdx/hk) | patch | `1.41.0` → `1.41.1` | MR created with the help of [el-capitano/tools/renovate-bot](https://gitlab.com/el-capitano/tools/renovate-bot). **Proposed changes to behavior should be submitted there as MRs.** --- ### Release Notes <details> <summary>jdx/hk (hk)</summary> ### [`v1.41.1`](https://github.com/jdx/hk/blob/HEAD/CHANGELOG.md#1411---2026-04-10) [Compare Source](jdx/hk@v1.41.0...v1.41.1) ##### 🐛 Bug Fixes - **(hook)** remove dup output and preserve check\_first diagnostics by [@nkakouros](https://github.com/nkakouros) in [#784](jdx/hk#784) - **(hook)** show combined output for failed steps by [@nkakouros](https://github.com/nkakouros) in [#772](jdx/hk#772) - **(hook)** preserve configured output\_summary label on failure by [@jdx](https://github.com/jdx) in [#808](jdx/hk#808) ##### 📦️ Dependency Updates - update rust crate pklr to 0.4.1 by [@jhult](https://github.com/jhult) in [#805](jdx/hk#805) ##### New Contributors - [@jhult](https://github.com/jhult) made their first contribution in [#805](jdx/hk#805) </details> --- ### Configuration 📅 **Schedule**: (UTC) - Branch creation - At any time (no schedule defined) - Automerge - At any time (no schedule defined) 🚦 **Automerge**: Enabled. ♻ **Rebasing**: Whenever MR is behind base branch, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this MR and you won't be reminded about this update again. --- - [ ] If you want to rebase/retry this MR, check this box --- This MR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).

gemini-code-assist Bot reviewed Mar 29, 2026

View reviewed changes

Comment thread src/main.rs

nkakouros marked this pull request as draft March 29, 2026 22:27

greptile-apps Bot reviewed Mar 29, 2026

View reviewed changes

Comment thread src/step/execution.rs

Comment thread src/main.rs

Comment thread test/output_summary_no_duplicate.bats

nkakouros force-pushed the fix/friendly-error-duplicate branch from 4522b34 to 29094f5 Compare March 29, 2026 22:33

nkakouros force-pushed the fix/friendly-error-duplicate branch from 2391a9d to c428282 Compare March 29, 2026 22:41

[autofix.ci] apply automated fixes

2b4a965

nkakouros marked this pull request as ready for review March 29, 2026 23:10

Merge branch 'main' into fix/friendly-error-duplicate

fdb3244

jdx merged commit 8b03a30 into jdx:main Apr 9, 2026
18 checks passed

jdx mentioned this pull request Apr 9, 2026

chore: release v1.41.1 #806

Merged

jdx mentioned this pull request Apr 10, 2026

fix(hook): preserve configured output_summary label on failure #808

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix(hook): remove dup output and preserve check_first diagnostics#784

fix(hook): remove dup output and preserve check_first diagnostics#784
jdx merged 3 commits intojdx:mainfrom
nkakouros:fix/friendly-error-duplicate

nkakouros commented Mar 29, 2026

Uh oh!

gemini-code-assist Bot left a comment

Uh oh!

Uh oh!

greptile-apps Bot commented Mar 29, 2026 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

nkakouros commented Mar 29, 2026

Summary

Context

Test plan

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

greptile-apps Bot commented Mar 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Greptile Summary

Confidence Score: 5/5

Important Files Changed

Flowchart

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

greptile-apps Bot commented Mar 29, 2026 •

edited

Loading