Don't count about-to-be-pinned workflows as failed#86
Open
nodeselector wants to merge 2 commits into
Open
Conversation
The pre-remediation summary computed its failed count by treating every workflow where !IsValid() as a failure. A workflow whose only non-valid finding is a dep-level NotPinned is exactly what the current run is about to pin, so a plain onboarding run reported "N of N workflows failed: N not-pinned" for work it was actively doing. Exclude remediable-only workflows (willRemediate and every non-valid finding is NotPinned) from the failed count, gate the error block on a non-zero failed count, and drop not-pinned from the error parts when remediating. Read-only and verify surfaces (willRemediate=false) still count not-pinned as a genuine coverage gap. Confined to the terminal renderer and its test; finding classification (checks/IsValid/NeedsAttention) is unchanged. Resolves github/actions-dispatch#802
There was a problem hiding this comment.
⚠️ Not ready to approve
The new “remediable-only” logic can incorrectly suppress real failures because NotPinned is also used for non-remediable workflow errors (e.g., load/deps errors), and the current exclusion logic can hide the only failure reason.
Pull request overview
Adjusts the terminal renderer’s failure summary so workflows that are about to be auto-pinned (dep-level not-pinned) don’t get reported as “failed” during a remediation run, avoiding misleading onboarding output.
Changes:
- Adds
isRemediableOnlyto detect workflows that are only failing due to dep-levelNotPinnedwhenwillRemediateis true. - Updates
PresentResultsto exclude remediable-only workflows fromfailedCount, and to conditionally suppressnot-pinnedin the error summary during remediation. - Adds a focused regression test covering onboarding-only output vs mixed failures vs read-only behavior.
File summaries
| File | Description |
|---|---|
| cmd/gh-actions-lock/format/terminal.go | Refines “failed workflows” counting and error-summary rendering to treat about-to-be-pinned workflows as remediable (not failed). |
| cmd/gh-actions-lock/format/terminal_test.go | Adds regression coverage for remediation vs read-only summary output around not-pinned. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 3
- Review effort level: Low
Note
Your feedback helps us improve the quality of this feature.
Please use 👍 or 👎 to tell us whether this assessment is correct.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
The #802 remediable-only logic treated any workflow whose only invalid finding was not-pinned as "about to be pinned", but diagnose.go also reports fatal workflow load / deps-read failures as workflow-level not-pinned (SeverityError, no ActionRef). Those are not remediable, so suppressing them let a genuinely broken workflow drop out of the failed count, and the unconditional not-pinned exclusion could blank out the only reason in the summary line. Restrict remediable-only to dep-level not-pinned (non-nil ActionRef), and exclude not-pinned from the summary only when another category already explains a failure. Update the regression test to model dep-level not-pinned at its production SeverityError and add fatal-load-error cases. Adds a catalog scenario asserting a fresh multi-workflow pin run does not report "N of N workflows failed".
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
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.
What
The pre-remediation summary line counted workflows that this run is about to pin as failures. A plain onboarding run (everything unpinned) shouted:
even though nothing had actually failed — the run was in the middle of pinning those very workflows.
Why
PresentResultsincmd/gh-actions-lock/format/terminal.gocomputed its failed count by treating every workflow where!IsValid()as failed. A dep-levelNotPinnedfinding makes a workflow!IsValid(), so about-to-be-pinned workflows landed in the "failed" bucket.Fix (rendering-only)
isRemediableOnly(wr, willRemediate): true when the run will remediate and every non-valid finding in the workflow isNotPinned.failedCount, gate the error block onfailedCount > 0, and dropnot-pinnedfrom the error parts when remediating.willRemediate == false) still count not-pinned as a genuine coverage gap — unchanged.Confined to the terminal renderer and its test. Finding classification (
checkscategories,IsValid,NeedsAttention,diagnose.go) is untouched.Tests
terminal_test.gocovers: pure onboarding emits no failed line; a genuine failure alongside an onboarding workflow reports1 of 2 workflows failedwithoutnot-pinned; read-only still counts not-pinned as a gap.Resolves github/actions-dispatch#802