{{ message }}
Repository navigation
pin summary: deduplicate impostor-commit output - #60
Merged
Merged
Conversation
renderInvestigationAlerts already renders impostor-commit and lockfile-forgery findings from the pin record. The hasUnfixable gate then called PresentResults which re-rendered the same findings from the checks report, producing duplicate output. Pass those categories as excludeCategories to PresentResults so they're skipped in the error findings block and summary line.
The hasUnfixable condition included impostor-commit, so
PresentResults still fired even with the exclude categories fix.
This produced a stale summary line counting pre-fix not-pinned
findings ("3 of 3 workflows failed: 6 not-pinned").
Split the gate: hasUnfixable still controls the allClean check and
errSilent return, but a narrower check (LocalAction and
SelfHostedRunner only) now controls the PresentResults call.
There was a problem hiding this comment.
⚠️ Not ready to approve
Excluding all error categories can still produce an empty "failed:" error summary line, so the renderer needs a guard (and a regression test) to suppress the error block when nothing remains after exclusions.
Pull request overview
This PR addresses duplicated terminal output in the pin flow by allowing the terminal renderer to suppress specific finding categories when they’re already rendered elsewhere with pin-specific formatting.
Changes:
- Added a variadic
excludeCategoriesparameter toformat.PresentResultsand threaded exclusions into error rendering/counting. - Updated the pin summary to exclude
ImpostorCommitandLockfileForgeryfromPresentResultswhenrenderInvestigationAlertshas already rendered them. - Added terminal renderer tests to validate excluded categories are not printed.
File summaries
Copilot's findings
- Files reviewed: 3/3 changed files
- Comments generated: 2
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.
Comment on lines
45
to
47
| if !valid && checked > 0 { | ||
| renderErrorFindings(out, report, failedCount, checked) | ||
| renderErrorFindings(out, report, failedCount, checked, exclude) | ||
| } |
Comment on lines
+507
to
+510
Look up the latest full semver tag for each repo and show it as a concrete suggestion (e.g. '→ latest: v6.1.2'). Repos that have no semver releases at all are suppressed from the nudge entirely, since the advice to use full semver refs is not actionable for them. Adds per-repo dedup via a map to avoid redundant API calls when multiple entries share the same NWO.
When a dep is direct in one workflow but transitive in another (via composite expansion), the transitive workflow's attribution was being dropped from the 'Pinned N actions' output. The display code only iterated Direct entries, so shared NWO@Ref groups never picked up workflows from their transitive counterparts. Add a second pass that merges workflow lists from transitive entries into existing direct groups. Purely-transitive deps (no direct counterpart) remain hidden.
Transitive deps discovered by composite expansion that have no direct counterpart were invisible in the terminal output. This made it look like the lockfile was missing deps when it was actually complete. Now purely transitive deps appear dimmed at the end of the pinned list with a 'via parent@ref' attribution. The header also shows the count: 'Pinned 3 actions (+ 1 transitive) across 3 workflows'. Adds planWorkflow-level integration test confirming the full cross-ref transitive closure (nested-composite@updated -> simple-composite@main -> cross/dep@main) survives the plan pipeline.
When one ref in a batch fails (e.g. SSO 403 for actions/checkout), the early return skipped the entire BFS loop. Transitive deps from composite actions that DID resolve were never discovered, producing an incomplete lockfile. Accumulate errors across BFS iterations and return them after the full traversal completes, so successful refs still get their composite expansion.
When two workflows share a dep, the second Set call clobbered the uses: list written by the first. A dep that is a transitive parent in workflow A but a direct leaf in workflow B lost its uses: edges, causing Save's GC to orphan-collect the transitive children. Merge the incoming uses set with any existing entry's uses list so edges from all workflows survive.
Diamond deps, depth-3 chains, direct+transitive overlap, cross-org transitive deps, and the uses-merge clobber scenario in one golden test. Verified it fails on the pre-fix code (composite-b loses its uses:[leaf] when a second workflow's Set overwrites without merge). Replaces the simpler SetMergesUsesAcrossWorkflows test.
Clones nodeselector/actions-test-fixtures and asserts the lockfile contains the cross-repo transitive dep (fixtures-b@main) and uses: chains. Catches both the BFS early-return bug and the Set clobber bug in a single end-to-end assertion. Also wires lockfile_contains from catalog into the test harness.
Replace loose lockfile_contains patterns with an exact golden file comparison. The expected lockfile lives at test/scenarios/testdata/transitive_closure_cross_repo.lock and the harness diffs it byte-for-byte against the generated output. Also adds lockfile_golden as a reusable catalog field for any future scenario that needs exact lockfile verification.
The interactive shell's RESULT block now reports whether the golden lockfile assertion passed or failed, matching the pattern used for other expect checks.
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.

renderInvestigationAlertsandformat.PresentResultsboth rendered impostor-commit findings -- the first from the pin record, the second from the checks report. WhenhasUnfixablewas true (which impostor-commit triggers), thePresentResultscall re-rendered the same findings, producing duplicate terminal output:Approach
Added a variadic
excludeCategoriesparameter toPresentResults. The pin summary now passesImpostorCommitandLockfileForgeryas excluded categories sincerenderInvestigationAlertsalready surfaces those with better formatting tailored to the pin flow.The exclude set is threaded through
renderErrorFindingsso excluded categories are skipped in both the per-finding detail rendering and the summary line counts. Existing callers (e.g.check.go) pass no exclusions and behave identically.