Repository navigation
Re-verify locked pins on the default run, and surface verify failures - #85
Conversation
Read-only modes (--verify / --no-fix without --json) exited non-zero on a failing lockfile but printed nothing about which workflow failed or why. The error findings were rendered through the narration helpers (out.Error/out.Detail), which sink to io.Discard once check.go attaches the discard log sink for human-terminal runs. Only fix mode re-surfaced them via renderPinSummary's Term* summary, and --no-fix returns before that runs. Add format.PresentReadOnlyFailures, a Term* renderer that groups error-level findings by dependency, prints category/detail/remediation and the affected workflow paths, and reports whether any finding is auto-fixable so the "re-run to fix" hint is only shown when it's honest. Call it from the --no-fix !valid branch. --verify-local is unaffected: it renders before the discard sink is installed, so its narration already reaches the terminal.
There was a problem hiding this comment.
⚠️ Not ready to approve
The new hasFixable detection is currently too broad and can cause the CLI to print an incorrect “re-run to apply fixes” hint for non-remediable error categories.
Pull request overview
This PR improves the UX of read-only verification runs (--verify / --no-fix) by ensuring error-level findings are rendered to the terminal (rather than being lost to the narration log sink), and by only showing the “re-run to apply fixes” hint when at least one failing finding is actually auto-remediable.
Changes:
- Add
format.PresentReadOnlyFailuresto render error-level findings viaTerm*in read-only modes and return whether any finding is fixable. - Wire the new renderer into the
--no-fixinvalid-report path and gate the “Re-run without --no-fix…” hint onhasFixable. - Add regression tests covering the discard-log terminal behavior and fixable/non-fixable cases.
File summaries
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 2
- 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.
Align the read-only failure renderer with the workflow dependency
pinning docs voice/tone: humanized category labels ("Lockfile forgery"
instead of a yelled slug), plainer suggested-pin wording, and a
contextual "how to fix this" link label instead of a bare "docs".
Adds categoryLabel with a hyphen-to-space fallback so new categories
still read cleanly. Narration path (fix mode) is unchanged.
Align the category with the house term used in github/launch's lockfile
resolver, which frames this as an integrity check ("integrity check
failed" / "tampered" / "fails closed") and never says "forgery".
"Forgery" also over-asserted malice — the check also fires on benign
upstream history rewrites, not just injected pins.
Renames the identifier (LockfileForgery -> LockfileIntegrity), the
schema string ("lockfile-forgery" -> "lockfile-integrity"), the human
label ("Lockfile integrity"), the frozen-strings vocabulary, and the
doc-URL slug. Safe rename: no consumer anywhere in the org keys on the
old string, and the docs page is still unpublished. Also drops the stale
"SARIF rule IDs" claim from the Category doc comment — SARIF isn't a
surface this tool emits.
"Integrity" collided with github/launch's meaning: their lockfile integrity check compares a SHA-256 digest of the runner-supplied dependency list against a signed token claim (payload tampering in transit). Ours is a different axis entirely — whether a single pinned SHA is an ancestor of / reachable from the ref's current head. "Unreachable pin" names our actual invariant, asserts no malice (the condition also fires on legitimate upstream history rewrites), and pairs with the existing ancestry-unknown / reachability-unknown inconclusive categories as their definitive-negative counterpart.
The CI lint ran --no-fix WITHOUT --rescan, so it only checked structural drift and trusted any pin already recorded in the lockfile. A stale or unreachable pin (locked SHA no longer on the ref) sailed through CI and only blew up at Actions runtime. Add --rescan to the read-only lint invocations so CI re-resolves every recorded pin against upstream and fails on unreachable-pin / ref-moved. GH_TOKEN is already wired by actions/lint, so no new auth is needed.
The lockfile fast path trusted any ref that already had a lockfile row: IsFullyRecorded marked the workflow Resolved, which skipped all live resolution, reachability, and ancestry. A stale or unreachable pin whose row existed was never re-checked — only --rescan bypassed the fast path. So a plain `gh actions-lock` run could not catch a pin that no longer resolves to the tag it claims. Split recorded refs by mutability instead. Full-semver tags (v4.2.1) are immutable — one commit for life — so a drifted SHA is always a defect; route them through live resolution so unreachable-pin/ref-changed/ misleading-sha fire on the default path. Mutable refs (v4, v4.2, branches) legitimately move, so they stay trusted (seeded from the lockfile) until --rescan. The classification is extracted into planFastPath so it can be tested without a network resolver. SkippedRescan now counts trusted mutable refs, which the summary surfaces. --verify-local stays the explicit offline path: it short-circuits before the resolver, so it still only catches structural issues (not-pinned), not stale SHAs.
The already-pinned summary asserted "All N valid" and then, in the same breath, told you to run --rescan "to re-verify reachability" — admitting it had not actually verified what it just called valid. A reviewer flagged it as confusing, and rightly so. Now that the default run re-verifies immutable pins, that contradiction is gone: the only refs trusted without a live check are mutable ones (v4, main) that legitimately move. Reword the nudge to name exactly those and say plainly they weren't live-checked, so "valid" isn't undercut. The nudge only shows when such refs exist; an all-immutable repo just sees "All N valid". Update the already_pinned_all_valid scenario to assert the new copy and exclude the old "re-verify reachability" phrasing.
Two issues from review of the read-only failure renderer. hasFixable was inferred as !IsAlertedCategory, which treats any error that isn't in the alerted set as auto-fixable — including local-action, which the tool can't remediate at all. That printed a misleading "Re-run without --no-fix to apply fixes" hint. Drive it from a positive IsAutoFixable set (not-pinned, ref-changed, stale) instead, so integrity failures and local-path actions don't trigger the hint. renderTermFindingDetail also dropped the upstream releases link that its fix-mode twin renderFindingDetail prints for unreachable-pin findings. Read-only output now shows the same link.
The lockfile-forgery → unreachable-pin rename missed the integration scenario catalog: dbot_forgery_blocks still asserted the old category and golden JSON, so integration-stub/-live failed. Update the assertions and the cosmetic descriptions to the current category name.
For a partial-semver pin (v4) with narrowing on, the summary printed two competing messages about the same ref: the version-ref nudge said "update the uses: line to the full version to lock precisely", while the new mutable-ref line said "run --rescan to re-verify". A reviewer rightly asked which one to follow. They're not equals. Narrowing v4 → v4.2.1 is the real fix: it locks precisely AND makes the ref immutable, so the default run re-verifies it live from then on. So when narrowing is on, the version-ref nudge is the single actionable message; the --rescan line only muddies it. Gate the mutable-ref nudge on --no-narrow, where the version-ref nudge is suppressed and the trust gap would otherwise go unmentioned. Branch pins (main), which never get a version-ref nudge, keep their honest signal there. Update the already_pinned_all_valid scenario accordingly.
The three existing verify_local_* scenarios only exercise functionality; they inherit ambient GH_TOKEN / gh auth via Open3.capture3 and never block the network, so a regression where --verify-local started resolving over the network would still pass on any authed runner. The no-network guarantee rested entirely on code structure (the run.go short-circuit before resolver setup), with no test pinning it. Add a declarative env override to the scenario catalog (nil unsets a var) and a verify_local_offline_unauthed scenario that strips all tokens and points GH_HOST at a refused address (127.0.0.1:1). Any network call or auth lookup fails closed under that trap, so the scenario only passes while the offline path is truly offline. Harden the harness repro/PTY paths against nil env values so unset vars render as `unset K` instead of crashing.

Summary
This PR started as a terminal-output fix and grew to close the trust-model gap behind it: a stale or unreachable pin could sit in the lockfile and never be re-verified. It now does three things:
--rescan.1. Surface verify failures in the terminal
gh actions-lock --verify(and--no-fixwithout--json) exited non-zero on a failing lockfile but printed nothing about which workflow failed or why — the finding only existed in--jsonoutput. Root cause: for human-terminal runs,runCheckattaches anio.Discardlog sink, so narration helpers (out.Error/out.Detail) are thrown away; only theTerm*family reaches the terminal. Fix mode re-surfaces errors through the summary, but--no-fix/--verifyreturns before that runs.Added
format.PresentReadOnlyFailures, aTerm*renderer that groups error findings by dependency and prints category, detail, remediation, and affected workflow path(s). The "Re-run without --no-fix" hint now only prints when a finding is actually auto-fixable.Copy was also run through the actions-copy voice/tone pass: humanized the shouty
LOCKFILE-FORGERY-style label into "Unreachable pin", reworded the suggested-pin line, and gave the doc link an honest label.2. Re-verify immutable pins on the default run
The lockfile fast path trusted any ref that already had a lockfile row:
IsFullyRecordedmarked the workflowResolved, skipping all live resolution, reachability, and ancestry. A stale/unreachable pin whose row existed was never re-checked — only--rescanbypassed the fast path. So a plaingh actions-lockrun could not catch a pin that no longer resolves to the tag it claims.Now recorded refs are split by mutability:
v4.2.1) are immutable — one commit for life — so a drifted SHA is always a defect. They're routed through live resolution sounreachable-pin/ref-changed/misleading-shafire on the default path.v4,v4.2, branches) legitimately move, so they stay trusted (seeded from the lockfile) until--rescan.The classification is extracted into
planFastPathand unit-tested without a network resolver.Behavior shift: the default run now needs network + auth whenever a pin is full-semver (nearly always).
--verify-localremains the explicit zero-network path — it short-circuits before the resolver, so it still only catches structural issues (not-pinned), not stale SHAs.3. Re-verify in CI + honest summary
script/lockfile-lint.shnow passes--rescanto its--no-fixinvocations, so CI actually re-verifies pins instead of only checking structural drift (GH_TOKENis already wired inactions/lint/action.yml).All N validand then tell you to--rescan"to re-verify reachability" — admitting it hadn't verified what it just called valid (flagged as confusing in QA). Now that the default run re-verifies immutable pins, the nudge is rescoped to name only the mutable refs that were trusted without a live check, so "valid" isn't contradicted. It disappears entirely when every pin is immutable.Also in here: category rename
lockfile-forgery→unreachable-pin. "Forgery" over-asserted attack; a non-ancestor pin can just as easily be a legitimate upstream squash/force-push. This aligns with how github/launch's resolver frames the same check and reads honestly ("the pinned commit is not reachable from the ref's current head"). Docs are unpublished, so the slug moved with it; no org consumer keys on the old string. Deep-linkedDocURLs land in a fast-follow once the docs page ships.Testing
terminal_test.go: forgery/unreachable finding reaches the terminal and reports not-fixable; a directly-unpinned action reports fixable; a valid report stays silent. Pins the discard-log regression.planFastPath/IsImmutableReftable tests cover the immutable-vs-mutable split.already_pinned_all_validscenario asserts the new summary copy and excludes the old "re-verify reachability" phrasing.go test ./...green;go vet ./...andgofmtclean.Addresses the verify-UX feedback in github/actions-dispatch#776 and the QA note about the confusing "all valid" summary.