pin summary: deduplicate impostor-commit output by nodeselector · Pull Request #60 · github/gh-actions-lock · GitHub
Skip to content

pin summary: deduplicate impostor-commit output - #60

Merged
nodeselector merged 12 commits into
mainfrom
nodeselector/fix-duplicate-output
Jun 18, 2026
Merged

nodeselector merged 12 commits into
mainfrom
nodeselector/fix-duplicate-output

Conversation

@nodeselector

Copy link
Copy Markdown
Collaborator

renderInvestigationAlerts and format.PresentResults both rendered impostor-commit findings -- the first from the pin record, the second from the checks report. When hasUnfixable was true (which impostor-commit triggers), the PresentResults call re-rendered the same findings, producing duplicate terminal output:

✗ 1 action requires maintainer action — pinned commit is not reachable from any branch
 nodeselector/actions-test-fixtures@tampered
 └─ .github/workflows/imposter-commit.yml
 ! Off-branch commits are indistinguishable from impostor commits
 → Ask the action maintainer to tag releases from a branch
 see: Using tags for release management
 ✗ IMPOSTOR-COMMIT nodeselector/actions-test-fixtures@tampered        <-- duplicate
 commit 7b403c9 not found on any branch ...                           <-- duplicate
 ! Off-branch commits are indistinguishable from impostor commits     <-- duplicate
 ↳ Ask the action maintainer to tag releases from a branch            <-- duplicate

Approach

Added a variadic excludeCategories parameter to PresentResults. The pin summary now passes ImpostorCommit and LockfileForgery as excluded categories since renderInvestigationAlerts already surfaces those with better formatting tailored to the pin flow.

The exclude set is threaded through renderErrorFindings so 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.

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.
Copilot AI review requested due to automatic review settings June 17, 2026 18:37
GitHub Advanced Security started work on behalf of nodeselector June 17, 2026 18:37
GitHub Advanced Security finished work on behalf of nodeselector June 17, 2026 18:38
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.
GitHub Advanced Security started work on behalf of nodeselector June 17, 2026 18:39
GitHub Advanced Security finished work on behalf of nodeselector June 17, 2026 18:41

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ 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 excludeCategories parameter to format.PresentResults and threaded exclusions into error rendering/counting.
  • Updated the pin summary to exclude ImpostorCommit and LockfileForgery from PresentResults when renderInvestigationAlerts has already rendered them.
  • Added terminal renderer tests to validate excluded categories are not printed.
File summaries
File Description
cmd/gh-actions-lock/pin_summary.go Passes excluded categories to PresentResults to prevent duplicate impostor/forgery output in pin summaries.
cmd/gh-actions-lock/format/terminal.go Adds category exclusions to terminal result rendering and summary counting.
cmd/gh-actions-lock/format/terminal_test.go Adds tests ensuring excluded categories are skipped and non-excluded findings still render.

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.
GitHub Advanced Security started work on behalf of nodeselector June 17, 2026 18:48
GitHub Advanced Security finished work on behalf of nodeselector June 17, 2026 18:49
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.
GitHub Advanced Security started work on behalf of nodeselector June 17, 2026 20:27
GitHub Advanced Security finished work on behalf of nodeselector June 17, 2026 20:28
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.
GitHub Advanced Security started work on behalf of nodeselector June 17, 2026 20:50
GitHub Advanced Security finished work on behalf of nodeselector June 17, 2026 20:52
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.
GitHub Advanced Security started work on behalf of nodeselector June 18, 2026 03:05
GitHub Advanced Security finished work on behalf of nodeselector June 18, 2026 03:06
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.
GitHub Advanced Security started work on behalf of nodeselector June 18, 2026 04:27
GitHub Advanced Security finished work on behalf of nodeselector June 18, 2026 04:28
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.
GitHub Advanced Security started work on behalf of nodeselector June 18, 2026 04:35
GitHub Advanced Security finished work on behalf of nodeselector June 18, 2026 04:36
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.
GitHub Advanced Security started work on behalf of nodeselector June 18, 2026 12:10
GitHub Advanced Security finished work on behalf of nodeselector June 18, 2026 12:12
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.
GitHub Advanced Security started work on behalf of nodeselector June 18, 2026 12:38
GitHub Advanced Security finished work on behalf of nodeselector June 18, 2026 12:39
The interactive shell's RESULT block now reports whether the golden
lockfile assertion passed or failed, matching the pattern used for
other expect checks.
GitHub Advanced Security started work on behalf of nodeselector June 18, 2026 12:43
GitHub Advanced Security finished work on behalf of nodeselector June 18, 2026 12:44
GitHub Advanced Security started work on behalf of nodeselector June 18, 2026 12:45
GitHub Advanced Security finished work on behalf of nodeselector June 18, 2026 12:46
@nodeselector
nodeselector merged commit 22b89d9 into main Jun 18, 2026
10 checks passed
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.

2 participants