Conversation
There was a problem hiding this comment.
Pull request overview
Updates the GoFortress pre-commit workflow to improve failure diagnostics by persisting cleaned check output to a log file, surfacing it in the GitHub Actions job summary, and uploading it as an artifact; also bumps the go-pre-commit tool version.
Changes:
- Bump
GO_PRE_COMMIT_VERSIONtov1.7.0. - On pre-commit failure, write cleaned output to
pre-commit-output.logand show truncated details in the workflow summary. - Upload the failure log as a retained artifact for later inspection.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| .github/workflows/fortress-pre-commit.yml | Adds failure log persistence, summary rendering of error output, and artifact upload of the log. |
| .github/env/10-pre-commit.env | Updates go-pre-commit tool version to v1.7.0. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| # Persist cleaned output to log file for summary and artifact upload | ||
| # Use printf to avoid echo misinterpreting leading -n/-e in output | ||
| printf '%s\n' "$CHECKS_OUTPUT" | \ | ||
| sed -E 's/\x1b\[[0-9;]*[mGKH]//g' | \ | ||
| sed 's/\xc2\x9b\[[0-9;]*[mGKH]//g' | \ | ||
| sed 's/�\[[0-9;]*[mGKH]//g' | \ | ||
| sed 's/�//g' | \ | ||
| tr -d '\033' > pre-commit-output.log |
There was a problem hiding this comment.
The ANSI/control-character stripping pipeline is duplicated here and earlier in the step when printing output. Duplicating it increases the chance the console output and persisted log diverge over time; consider factoring this into a single reusable variable/function (e.g., compute cleaned output once, then both print and write the file).
| # Persist cleaned output to log file for summary and artifact upload | ||
| # Use printf to avoid echo misinterpreting leading -n/-e in output | ||
| printf '%s\n' "$CHECKS_OUTPUT" | \ | ||
| sed -E 's/\x1b\[[0-9;]*[mGKH]//g' | \ |
There was a problem hiding this comment.
The comment says printf is used to avoid echo misinterpreting leading -n/-e, but the step still uses echo "$CHECKS_OUTPUT" | ... earlier to render the cleaned output to logs. Either update the earlier output path to use printf as well, or adjust this comment so it doesn't imply the problem is fully addressed.
| echo "<details>" >> $GITHUB_STEP_SUMMARY | ||
| echo "<summary>Click to expand full output</summary>" >> $GITHUB_STEP_SUMMARY | ||
| echo "" >> $GITHUB_STEP_SUMMARY | ||
| echo '```' >> $GITHUB_STEP_SUMMARY | ||
| head -200 pre-commit-output.log >> $GITHUB_STEP_SUMMARY | ||
| echo '```' >> $GITHUB_STEP_SUMMARY |
There was a problem hiding this comment.
The summary text says "Click to expand full output", but the workflow only appends head -200 of the log. This is misleading; either include the full log content (if safe) or adjust the wording to indicate it is truncated (e.g., "first 200 lines").
| # -------------------------------------------------------------------- | ||
| # Upload pre-commit results (only present on failure) | ||
| # -------------------------------------------------------------------- | ||
| - name: 📤 Upload pre-commit results | ||
| if: always() | ||
| uses: ./.github/actions/upload-artifact-resilient |
There was a problem hiding this comment.
The section header says "Upload pre-commit results (only present on failure)", but this step runs unconditionally with if: always() (it just ignores missing files). Either make the step conditional on the log file existing, or update the header/comment to match the actual behavior.
|
LGTM! |
What Changed
GO_PRE_COMMIT_VERSIONfromv1.6.2tov1.7.0in.github/env/10-pre-commit.envpre-commit-output.logfile when checks fail, using multiplesedcommands to strip ANSI escape sequences and control charactersWhy It Was Necessary
Testing Performed
Impact / Risk