Skip to content

Fix skip-comment string typing, boolean comparison bugs, and failure reporting in format workflows#383

Merged
greenc-FNAL merged 3 commits intomaintenance/phlexbot-format-rollupfrom
copilot/sub-pr-380
Mar 4, 2026
Merged

Fix skip-comment string typing, boolean comparison bugs, and failure reporting in format workflows#383
greenc-FNAL merged 3 commits intomaintenance/phlexbot-format-rollupfrom
copilot/sub-pr-380

Conversation

Copy link
Contributor

Copilot AI commented Mar 4, 2026

Several bugs introduced in the @phlexbot format consolidation: skip-comment declared as boolean in reusable workflows causing type mismatch when passed as "true", two boolean-vs-string comparison bugs that silently suppressed "no changes" comments, a folded run: >- block that merged two shell commands onto one line breaking header guard detection, and combine-results not detecting or surfacing sub-workflow failures.

Changes

  • skip-comment as string throughout: All 7 fix workflows change type: boolean / default: falsetype: string / default: "false". Eliminates type mismatch with callers that pass "true".

  • handle-fix-commit comparison fixes:

    • steps.check_changes.outputs.changes == false== 'false' — action outputs are strings; boolean comparison always evaluated to false, suppressing the "No changes to apply" comment.
  • header-guards-check.yaml broken script:

    • run: >- folded the || echo "has_issues=true" >> "$GITHUB_OUTPUT" and cat check_output.txt onto a single line, making both unreachable. Converted to run: |.
  • format-all.yaml failure reporting: Collect results now checks needs.<job>.result in addition to outputs. Failed or cancelled sub-workflows surface as ❌ <tool> workflow <result> in the combined comment and cause the aggregator job to exit non-zero. Post combined comment gains if: always() to ensure the comment posts even when failures are detected.


🔒 GitHub Advanced Security automatically protects Copilot coding agent pull requests. You can protect all pull requests by enabling Advanced Security for your repositories. Learn more about Advanced Security.

…uards run script, failure reporting

Co-authored-by: greenc-FNAL <2372949+greenc-FNAL@users.noreply.github.com>
Copilot AI changed the title [WIP] Combine reports from all @phlexbot format workflows Fix skip-comment string typing, boolean comparison bugs, and failure reporting in format workflows Mar 4, 2026
@greenc-FNAL
Copy link
Contributor

@phlexbot format

@github-actions
Copy link
Contributor

github-actions bot commented Mar 4, 2026

No automatic jsonnetfmt fixes were necessary.

@github-actions
Copy link
Contributor

github-actions bot commented Mar 4, 2026

No automatic YAML formatter fixes were necessary.

@github-actions
Copy link
Contributor

github-actions bot commented Mar 4, 2026

No automatic cmake-format fixes were necessary.

@github-actions
Copy link
Contributor

github-actions bot commented Mar 4, 2026

No automatic header-guards fixes were necessary.

@github-actions
Copy link
Contributor

github-actions bot commented Mar 4, 2026

No automatic clang-format fixes were necessary.

@github-actions
Copy link
Contributor

github-actions bot commented Mar 4, 2026

No automatic markdownlint fixes were necessary.

@greenc-FNAL greenc-FNAL marked this pull request as ready for review March 4, 2026 00:47
Copilot AI review requested due to automatic review settings March 4, 2026 00:47
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Fixes several regressions in the consolidated @phlexbot format workflow set by aligning input types, correcting string-vs-boolean comparisons, repairing a broken folded shell script block, and improving failure propagation/visibility from sub-workflows.

Changes:

  • Standardize skip-comment as a string input (default "false") across all reusable “*-fix” workflows to match how callers pass "true".
  • Fix handle-fix-commit conditional logic to compare action outputs as strings (e.g., 'false'), restoring “no changes” reporting.
  • Improve format-all result aggregation to surface failed/cancelled sub-workflows in the combined comment and fail the aggregator job when any sub-workflow fails.

Reviewed changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
.github/workflows/yaml-fix.yaml Change skip-comment workflow_call input to string to match caller usage.
.github/workflows/python-fix.yaml Change skip-comment workflow_call input to string to match caller usage.
.github/workflows/markdown-fix.yaml Change skip-comment workflow_call input to string to match caller usage.
.github/workflows/jsonnet-format-fix.yaml Change skip-comment workflow_call input to string to match caller usage.
.github/workflows/header-guards-fix.yaml Change skip-comment workflow_call input to string to match caller usage.
.github/workflows/cmake-format-fix.yaml Change skip-comment workflow_call input to string to match caller usage.
.github/workflows/clang-format-fix.yaml Change skip-comment workflow_call input to string to match caller usage.
.github/workflows/header-guards-check.yaml Fix broken folded run: >- block by using a proper multiline script (`run:
.github/workflows/format-all.yaml Aggregate needs.<job>.result and outputs to report failures and post a combined comment reliably.
.github/actions/handle-fix-commit/action.yaml Fix output comparison to treat steps.*.outputs.* as strings ('false').

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated no new comments.

@greenc-FNAL greenc-FNAL merged commit c47f455 into maintenance/phlexbot-format-rollup Mar 4, 2026
62 checks passed
@greenc-FNAL greenc-FNAL deleted the copilot/sub-pr-380 branch March 4, 2026 00:59
greenc-FNAL added a commit that referenced this pull request Mar 4, 2026
greenc-FNAL added a commit that referenced this pull request Mar 4, 2026
* Combine reports from all `@phlexbot format` workflows

- Dedicated new workflow `format-all.yaml` is the sole responder to
  `@phlexbot format` instead of multiple "fix" workflows.
- Specific "fix" workflows are called via `workflow_call` and results
  combined into a single comment to reduce clutter in the PR
  conversation history.
- The `handle-fix-commit` reusable action will skip posting generated
  comments when called via `workflow_call`, allowing multiple messages
  to be consolidated into a single PR comment.
- Indicate workflow completion on invocation comment

* Update workflow documentation

* Prettier fixes

* Apply suggestions from code review

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

* Apply review feedback: string skip-comment, fix comparisons, header-guards run script, failure reporting

Co-authored-by: greenc-FNAL <2372949+greenc-FNAL@users.noreply.github.com>

* Run results collection step with Bash

- Per #383 (comment)

* fix: guard inputs.skip-comment in issue_comment workflows and update docs

- Replace `inputs.skip-comment` with `github.event_name == 'workflow_call' && inputs.skip-comment || 'false'` in all 7 fix workflows so that issue_comment triggers don't access the unavailable inputs context
- Remove redundant `&& inputs.skip-comment != 'true'` from reaction step conditions that already gate on `github.event_name == 'issue_comment'`
- Update format-all.yaml patch message to include artifact name and apply instructions
- Update REUSABLE_WORKFLOWS.md: change skip-comment examples from boolean true to string "true", and type descriptions from boolean to string

Co-authored-by: greenc-FNAL <2372949+greenc-FNAL@users.noreply.github.com>

* Apply suggestions from code review

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

* Fix ref descriptions, docs examples, eyes reaction, patch artifact message

Co-authored-by: greenc-FNAL <2372949+greenc-FNAL@users.noreply.github.com>

* Address remaining Copilot review comments

- Per #385 (review)

* Address latest review

- Per #385 (review)

* Harden Remove eyes reaction step in all fix workflows

Wrap listForIssueComment call in try/catch so a transient API error does
not fail the workflow. Narrow the bot-match predicate from the broad
r.user.type === 'Bot' to r.user && r.user.login === 'github-actions[bot]'
so only the reaction posted by phlexbot's pre-check step is removed.

Use destructured { data: reactions } assignment for consistency with
format-all.yaml.

* Declare on.workflow_call.outputs in all fix workflows

format-all.yaml reads changes/pushed/commit_sha_short/patch_name from
needs.<job>.outputs.* for each called fix workflow. GitHub Actions only
surfaces these to the caller when they are explicitly re-exported under
on.workflow_call.outputs; job-level outputs alone are not visible to
calling workflows.

Add on.workflow_call.outputs sections to all seven fix workflows,
mapping to the corresponding apply-job outputs.

* Guard PR lookup in handle-fix-commit against missing issue context

The Get PR maintainer_can_modify property step uses context.issue.number,
which is only populated when the root trigger is an issue_comment event.
When a fix workflow is triggered via workflow_dispatch, context.issue.number
is undefined, causing the pulls.get API call to fail whenever formatting
changes are found.

Note: workflow_call from format-all.yaml (triggered by issue_comment) is
not affected — GitHub propagates the event payload to called workflows so
context.issue.number is populated there. The bug is specific to the
workflow_dispatch path.

Default to 'false' for maintainer_can_modify when no PR context is
available. The commit path's first condition (same-repo check) still
allows pushes to proceed correctly for workflow_dispatch runs.

---------

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: greenc-FNAL <2372949+greenc-FNAL@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.

3 participants