Skip to content

fix: make text-mode progress output usable in CI#890

Merged
jdx merged 9 commits intomainfrom
claude/zealous-agnesi-645e62
Apr 30, 2026
Merged

fix: make text-mode progress output usable in CI#890
jdx merged 9 commits intomainfrom
claude/zealous-agnesi-645e62

Conversation

@jdx
Copy link
Copy Markdown
Owner

@jdx jdx commented Apr 30, 2026

Summary

Cleans up hk's text-mode (CI / piped stderr) progress output. The previous behavior was unreadable in GitHub Actions logs: raw [9A[80D[0J cursor-control escapes leaked into the log, every status change duplicated, failure stderr was suppressed, and a step matching hundreds of files dumped ~4KB of paths into every progress line. After this PR a 14-step pre-commit run reads as a clean append-only stream with one full diagnostic block per failure.

What changed

Bumped clx 2.0 → 2.0.1 (clx#86)

  • refresh_once() is now a no-op in ProgressOutput::Text so terminal-state transitions and stop() don't leak UI escape codes
  • render_text_mode() dedupes consecutive identical job lines per job (multiple props updated in quick succession no longer print the same line N times)
  • next_operation() resets the dedup cache so multi-operation transitions are never silently swallowed

Failure summaries shown in text mode (src/hook.rs)

  • HookContext.failed_steps: Mutex<HashSet<String>> tracks failures so the end-of-run summary survives the step_contexts.shift_remove that fires when each step finishes
  • In text mode, the per-step output summary block is now emitted for failed steps by default — successful steps stay quiet (their output already streamed). HK_SUMMARY_TEXT=1 still forces every step's summary to print
  • Docs updated for HK_SUMMARY_TEXT to reflect the new default behavior

Bounded progress message rendering (src/step/runner.rs, src/tera.rs, src/step_job.rs)

  • New Context::for_display() returns a tera context where files / workspace_files are rewritten to first_token … when more than one file matches. The runner renders twice — display version for the progress message, full version for execution
  • truncate_progress_message() caps the formatted progress message at 2048 printable chars with a trailing , ANSI-aware so escape sequences don't get split mid-cluster
  • Dropped the truncate_text filter from step_job.rs body_text — clx's filter clamped to (term_width - 20) which is 60 chars in non-TTY environments, cutting exactly the diagnostic detail you need to debug a CI failure

Examples — dbg step matching 98 .rs files in CI:

# before:
  dbg – 98 files – **/*.rs – ! rg -e 'dbg!' bin/generate_docs.rs build/generate_builtins.rs ... [continues for 98 paths] ...

# after:
  dbg – 98 files – **/*.rs – ! rg -e 'dbg!' bin/generate_docs.rs …

Failed step in CI:

# before (excerpt — full block was ~85 lines of escape-code-littered duplication):
[9A[80D[0J✔ files - Fetching all files in repo (671 files)
✗ demo-fail
✗ demo-fail
✗ demo-fail
✗ demo-fail – ERROR
[9A[80D[0J✔ files - Fetching all files in repo (671 files)
... (escape-code redraws repeat for every aborted sibling step)

# after:
  demo-fail – 98 files – **/*.rs – echo "..." (multi-line script)
  demo-fail – this step is intentionally failing so we can see what the output looks like
  demo-fail – line two of stderr
  demo-fail – line three of stdout
✗ demo-fail
✗ demo-fail – ERROR

demo-fail stderr:
this step is intentionally failing so we can see what the output looks like
line two of stderr
line three of stdout

Test plan

  • cargo test — 149 pass (11 new across src/tera.rs::tests and src/step::runner::tests)
  • mise run test:bats test/git.bats — 4 pass under both libgit2 and nolibgit2 (assertions updated for first_file … display)
  • mise run test:bats test/output_summary.bats test/output_summary_no_duplicate.bats test/output_summary_check_first.bats — 11 pass
  • Verified end-to-end in CI via the (now-removed) demo-fail scaffolding step — see runs 25166877825 (before) and 25168039915 (after)

🤖 Generated with Claude Code

Comment thread hk.pkl Outdated
echo "line three of stdout"
exit 1
"""#
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Deliberately failing CI step must not be merged

High Severity

The demo-fail step is a temporary debugging aid that unconditionally fails with exit 1. If this lands on the default branch, it will cause every CI run to fail on any commit that touches **/*.rs files. The PR description's test plan calls for removing it before merge, but it's still present.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 94f6568. Configure here.

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces a temporary, intentionally failing linter step named 'demo-fail' to the hk.pkl configuration to capture and inspect CI failure output. Feedback suggests isolating this step using a specific profile to prevent it from disrupting local developer workflows by causing pre-commit or pre-push hooks to fail globally.

Comment thread hk.pkl Outdated
Comment on lines +27 to +38
// TEMP: deliberately failing step to capture the ugly clx failure output in CI.
// Remove once we've fixed the formatting in a follow-up.
["demo-fail"] = new Step {
glob = "**/*.rs"
check =
#"""
echo "this step is intentionally failing so we can see what the output looks like"
echo "line two of stderr" 1>&2
echo "line three of stdout"
exit 1
"""#
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

Adding a deliberately failing step to the global linters mapping will cause the pre-commit and pre-push hooks to fail for all developers working on this branch. This can be disruptive to local workflows.

Consider isolating this step using a specific profile so it only runs in the CI environment where you intend to capture the output. You can then trigger it in CI by passing the --profile flag.

  // TEMP: deliberately failing step to capture the ugly clx failure output in CI.
  // Remove once we've fixed the formatting in a follow-up.
  ["demo-fail"] = new Step {
    profiles = List("ci-debug")
    glob = "**/*.rs"
    check =
      #"""
      echo "this step is intentionally failing so we can see what the output looks like"
      echo "line two of stderr" 1>&2
      echo "line three of stdout"
      exit 1
      """#
  }

@greptile-apps
Copy link
Copy Markdown

greptile-apps Bot commented Apr 30, 2026

Greptile Summary

This PR improves CI/text-mode output readability by: (1) tracking failed steps in HookContext.failed_steps and only emitting end-of-run output summaries for those steps in text mode by default (UI mode is unchanged), (2) rendering a truncated display variant of {{files}}/{{workspace_files}} for progress messages to keep CI log lines bounded, and (3) removing the truncate_text filter from text-mode progress bodies so CI diagnostics are no longer clipped. New unit tests cover the token-splitting and truncation helpers.

Confidence Score: 5/5

Safe to merge once the demo-fail step and [patch.crates-io] branch pin are removed (both noted in existing threads).

No P0 or P1 issues found. All logic changes are well-reasoned, well-commented, and backed by unit tests. The summary-display condition is correct, the token-splitter correctly handles multi-byte UTF-8, and the dual-render approach for display vs. execution is sound.

No files require special attention beyond the pre-merge checklist items already tracked in existing review threads.

Important Files Changed

Filename Overview
src/hook.rs Adds failed_steps: Mutex<HashSet> to HookContext, mark_step_failed helper, and revised summary display logic that filters to failed-only in text mode.
src/step/output.rs Calls mark_step_failed after the check_first early-return guard so only the failing run phase marks the step.
src/step/runner.rs Adds truncate_progress_message helper and renders commands twice — once via for_display() for the progress message, once in full for execution.
src/tera.rs Adds Context::for_display(), truncate_quoted_list, and split_quoted_tokens with thorough unit tests covering quoted paths, escapes, and edge cases.
src/step_job.rs Removes truncate_text filter from the body_text template so text-mode CI output is no longer truncated mid-diagnostic.
test/git.bats Updates bats assertion to match new truncated-file-list display format.
Cargo.toml Bumps clx to 2.0.1; still carries a [patch.crates-io] entry pinning a branch (noted in existing review thread).

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[Hook run completes] --> B[clx::progress::stop]
    B --> C{in_text_mode?}
    C -->|No — UI mode| D[Enter output loop]
    C -->|Yes| E{HK_SUMMARY_TEXT=1?}
    E -->|Yes — force_summary| D
    E -->|No| F{failed_steps not empty?}
    F -->|No| G[Skip all summaries]
    F -->|Yes| D
    D --> H[For each step in output_by_step]
    H --> I{in_text_mode && !force_summary?}
    I -->|No| J[Print step summary]
    I -->|Yes| K{step in failed_steps?}
    K -->|Yes| J
    K -->|No| L[Skip — already streamed live]
    J --> M{More steps?}
    L --> M
    M -->|Yes| H
    M -->|No| N[End]
    G --> N
Loading

Reviews (7): Last reviewed commit: "fix(step): cap progress message length" | Re-trigger Greptile

Comment thread hk.pkl Outdated
Comment thread Cargo.toml Outdated
Copy link
Copy Markdown

@cursor cursor Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 1 potential issue.

There are 2 total unresolved issues (including 1 from previous review).

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit eb79985. Configure here.

Comment thread Cargo.toml Outdated
jdx and others added 6 commits April 30, 2026 10:08
This is a temporary step that always exits 1, used to surface what hk
check failures look like in GHA logs so we can fix the rendering in a
follow-up commit on this PR.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Two changes to make hk's CI/non-TTY output readable when a step fails:

clx fix (jdx/clx#fix-text-mode-rendering, patched in via Cargo):
  - refresh_once() no longer emits UI-mode cursor escape sequences when
    output is set to Text mode. set_status(Done|Failed|Warn) and stop()
    both call refresh_once(), so previously every state change leaked
    raw [9A[80D[0J into the log.
  - render_text_mode() deduplicates consecutive identical job lines so
    the same status doesn't get printed twice when callers update
    several props in a row.

hk side:
  - Track failing step names on HookContext (step_contexts are removed
    as soon as each step finishes, so the existing status was already
    gone by the time the summary ran).
  - In text mode, always print the per-step output summary for failed
    steps so the diagnostic is captured in full rather than truncated
    one-line-at-a-time via the streaming `message` prop. Successful
    steps stay quiet in text mode (their output already streamed);
    HK_SUMMARY_TEXT=1 still forces every step to print.

The `demo-fail` step from the previous commit stays in for one more
CI run to capture the cleaned-up output; it'll be deleted before merge.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The per-job body_text template ran every message and ensembler_stdout line
through clx's truncate_text filter, which clamps to (terminal_width - 20).
In a non-TTY environment console::Term::stderr().size() returns 80 cols by
default, so each line was being cut to ~60 chars with a trailing `…` —
losing exactly the diagnostic detail you need to debug a CI failure.

Drop the filter for body_text. UI-mode body still uses `flex` because the
in-place renderer needs bounded widths; CI log viewers handle wrapping.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The per-job progress message embedded the fully-rendered run command —
which expands {{files}} into the literal list of matched paths. In
text mode (CI logs, piped stderr) a step matching 98 .rs files dumped
~4KB of file paths per status update; in UI mode the `flex` filter
hid it but the cost was still there.

Use the un-rendered template for display instead. The file count and
glob pattern shown right next to it already tell the reader what's
being processed; the actual list of paths adds no diagnostic value
in the progress line. The fully rendered command is still what we
hand to the shell for execution.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The progress message now embeds the un-rendered command template
(e.g. \`echo '{{files}}'\`) instead of expanding {{files}} into the
literal path list — paths still surface via streamed stdout / -v
trace. The git.bats merge-base test was asserting the rendered form;
update the three assertions to match the new contract while keeping
the streamed-stdout assertions unchanged.
Replaces the earlier "show un-rendered template" approach. The
previous fix (a14e77e) traded one diagnostic detail (knowing what
files actually got handed to the tool) for another (CI logs not
filling with 4KB of paths). Better: render the command with
{{files}} and {{workspace_files}} truncated to "first_file …" when
there are multiple files. Steps matching one file still show that
file inline, steps matching hundreds show the command shape plus
one concrete example.

Adds Context::for_display() that returns a clone with the file
lists rewritten, plus split_quoted_tokens / truncate_quoted_list
helpers that respect ShellType::quote-style escaping (so paths
with spaces like 'a b.txt' don't get split mid-token). Step
runner renders twice — display version for the progress message,
full version for execution.

Updates the git.bats merge-base assertions to expect the new
form: 1 file → 'feature.txt', 2 files → 'feature.txt …'.
@jdx jdx force-pushed the claude/zealous-agnesi-645e62 branch from a4c77c2 to 06a6abd Compare April 30, 2026 15:09
jdx and others added 3 commits April 30, 2026 10:10
clx 2.0.1 ships the text-mode rendering fixes from clx#86, so the
[patch.crates-io] git-branch override is no longer needed. Drop it
and bump the dependency.

Also remove the deliberately-failing demo-fail step from hk.pkl —
it served its purpose for capturing the before/after CI output and
should not ship to main.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The per-job progress message embeds the rendered run command, which can
be a multi-line shell script or a generated command with hundreds of
inline args. With clx's text-mode escape-code suppression in place, an
unbounded message would still flood CI logs on every prop update.

Cap the formatted message at 2048 printable chars with a trailing `…`,
ANSI-aware so escape sequences don't get split mid-cluster or counted
toward the budget. The cap lives in hk because different consumers want
different bounds — clx itself stays mode-agnostic.
@jdx jdx changed the title fix: clean up clx output when an hk step fails fix: make text-mode progress output usable in CI Apr 30, 2026
@jdx jdx merged commit 1a1aafa into main Apr 30, 2026
21 checks passed
@jdx jdx deleted the claude/zealous-agnesi-645e62 branch April 30, 2026 15:32
@jdx jdx mentioned this pull request Apr 30, 2026
jdx added a commit that referenced this pull request Apr 30, 2026
### 🐛 Bug Fixes

- **(hook)** do not stage fixes when fail_on_fix=true by
[@jdx](https://github.com/jdx) in
[#892](#892)
- use site domain for plausible data-domain by
[@jdx](https://github.com/jdx) in
[#886](#886)
- make text-mode progress output usable in CI by
[@jdx](https://github.com/jdx) in
[#890](#890)

### 📚 Documentation

- prefix GitHub star count with ★ glyph by
[@jdx](https://github.com/jdx) in
[#883](#883)

### 🔍 Other Changes

- **(release)** dedupe sponsor section in release notes by
[@jdx](https://github.com/jdx) in
[#881](#881)
- switch analytics from gtm/goatcounter to plausible by
[@jdx](https://github.com/jdx) in
[#885](#885)
- migrate to namespace.so runners by [@jdx](https://github.com/jdx) in
[#891](#891)

<!-- CURSOR_SUMMARY -->
---

> [!NOTE]
> **Low Risk**
> Low risk release bookkeeping: version bumps, regenerated docs, and
lockfile dependency updates with no functional code changes in this PR.
> 
> **Overview**
> Bumps `hk` to **v1.44.3** and adds the corresponding `CHANGELOG.md`
release entry.
> 
> Regenerates versioned docs/CLI artifacts to reference `1.44.3`
(package URLs and generated `commands.json`/`index.md`) and updates
`Cargo.lock` with dependency resolution changes (notably `jni`,
`rustls*`, `reqwest`, `wasm-bindgen`, and `thiserror` unification).
> 
> <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit
ab7b72e. Bugbot is set up for automated
code reviews on this repo. Configure
[here](https://www.cursor.com/dashboard/bugbot).</sup>
<!-- /CURSOR_SUMMARY -->

Co-authored-by: mise-en-dev <123107610+mise-en-dev@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant