lockfile: bump parser to v0.0.2 with simplified pin keys - #62
Merged
Conversation
Bump github.com/github/actions-lockfile/go to 1313b0a which drops the branch field and renames tag to ref (optional). The lockfile now carries a single 'ref' field per dependency instead of separate tag + branch. Serialization picks the best ref for each dep: tag (semver-ish release) wins, then protected branch, then default branch — matching the DiscoverContaining priority. Branch is no longer required: deps without a discoverable ref emit an empty ref field. Updates all fixtures, goldens, and test assertions.
Adopt eff8f62 from actions-lockfile which drops the commit hash from pin keys. Keys are now `owner/repo@ref` instead of `owner/repo@ref:algo-hex`. The commit field in each entry remains the sole source of truth for integrity. - Pin struct no longer carries Algo/Hex; SHA comes from Action.Commit - Introduce lockedPin type in checks package pairing Pin + Commit - state.go Get()/AllDeps() populate dep SHA from Action.Commit - state.go Set() uses d.Ref directly (parser validates ref == pin key) - pickPreferredTag uses IsFull() for full-semver-over-major priority - All test fixtures updated to v0.0.2 format
Pin keys now use owner/repo@ref (no :sha1-hex suffix), ref field replaces tag/branch, golden_json expectations updated.
Add golden_json assertions to dbot_impostor_blocks and dbot_forgery_blocks so schema regressions in finding output are caught. Add lockfile_contains assertions to dbot stub scenarios verifying key format, ref field, and commit field presence. Add onboard_roundtrip_v002 live test against nodeselector/actions-test-fixtures that verifies a fresh onboard produces valid v0.0.2 lockfile structure. Update transitive_closure_cross_repo golden to track current main SHA.
…t deps Port the dependency coverage validation from actions-workflow-parser into the test harness as two independent assertions: - lockfile_deps_cover_direct: every pin in workflows: exists in dependencies: - lockfile_deps_cover_indirect: every ref in a dependency's uses: list exists in dependencies: Applied to composite_action_transitive, transitive_closure_cross_repo, onboard_roundtrip_v002 (both), and fresh_first_run (direct only).
cli/gh-extension-precompile@v2.1.0 no longer includes actions/setup-go in its composite deps. Update transitive_major_ref_not_narrowed to assert on actions/attest-build-provenance@v1 instead (same concept: major ref not narrowed). Fix regex patterns broken by v0.0.2 quote-wrapped keys -- switch from lockfile_comment_matches to lockfile_contains where appropriate. Add coherence assertions to transitive_major_ref_not_narrowed.
narrowVerifiedEntries upgraded already-recorded direct deps to full semver tags without consulting prevImpreciseNWO, so an already-pinned imprecise ref (e.g. actions/checkout@v4) was silently narrowed to v4.x.y on every no-op re-pin. That churns the lockfile and violates the sticky-precision invariant PlanOptions documents. Cherry-picked from nodeselector/code-quality-review (fa6c680).
Seven files had drifted from gofmt (struct-tag and map-literal alignment, a stray blank line). Pure formatting, no behavior change. This clears the way for a gofmt gate in CI.
The test workflow ran only `go test ./...` — no vet, no race detector, no formatting check — so unformatted code and data races could land unnoticed. Match the sibling gh-stack repo's lean CI: gofmt check, go vet, and go test -race -count=1, all folded into the existing go-test job (no new linter framework, no extra workflow). Add fmt, fmt-check, and vet Makefile targets so local matches CI.
Snapshot the exact bytes every status, Term*, and color-wrapper method emits across color, plain, and headless modes. These golden files fence the upcoming ui.go file-split: a pure code move must leave all three transcripts byte-identical. Regenerate intentional changes with UPDATE_GOLDEN=1, matching the existing JSON golden convention. Spinner and progress methods are excluded; they are time-based and already covered by ui_test.go.
The 1375-line ui.go mixed six concerns. Split it by responsibility with no behavior change: style.go (color/hyperlink helpers), status.go (narration + Term* summaries), spinner.go (spinner writer + lifecycle), progress.go (worker-slot API + debug tracing), text.go (width/truncation utilities). ui.go keeps the struct, constructors, mode detection, and the emit/log plumbing. Pure code movement: the UI golden characterization tests and the existing spinner tests pass unchanged, proving the output surface is byte-identical.
Primer is GitHub's design system; the stray capital R was a typo. A full-repo misspell scan found no other spelling errors.
The root command and the check subcommand were two cobra commands that both called the identical runCheck, with a code comment conceding "Root is just the default check invocation." check was a redundant alias that doubled the command surface, the help text, and the example blocks. Collapse to a single `gh actions-lock [paths]` command: delete newCheckCmd, drop the AddCommand wiring, and rename check.go to run.go to reflect that it now holds run logic rather than a command. The richer Issue-types and Exit-status help and the runner-label example fold into the root command's Long/Example so no documentation is lost. User-facing remediation strings that pointed at `gh actions-lock check` now point at `gh actions-lock`, with the integration catalog updated to match. The JSON golden (expected.json) is byte-identical, proving the diagnosis contract is preserved.
These two packages had the weakest coverage of the logic-bearing internal packages (pipeline 41%, tag 25%), almost entirely on pure helpers that are cheap to exercise directly. Cover them where the assertions are about real behavior, not line counts: pipeline: isHex / isLikelyTag / ReleasesURL / DocURLFor (URL and category classification), indexDeps and hasIssues (issue triage across error, inconclusive, and run-only categories), and the parent-attachment helpers isTransitivePin / populateInventoryParents. tag: CooldownDuration, RepoInfo.IsInternal, URL, FormatTagAge bucket boundaries, ReorderSuggestions ordering, and isTagTooNew cooldown filtering (seeded release dates, zero-cooldown short-circuit). Raises pipeline to 48% and tag to 38%. No production code changed.
These goldens existed only to fence the ui.go split (611d369): snapshot the rendered surface before the move, prove it byte-identical after. That refactor has landed and the behavioral tests in ui_test.go cover the surface, so the snapshots no longer earn their keep — they assert no meaningful behavior and just break whenever output formatting changes. The pre-existing JSON golden in cmd/ stays; it's a real diagnosis contract, not refactor scaffolding.
planWorkflow had grown to ~400 lines spanning eight distinct phases (resolve, reachability gate, full-scan metadata, direct-dep narrowing, reverse-lookup canonicalization, parent rekey, pinned-entry assembly, informational findings). The phases were already marked by section comments and status() calls but shared one giant scope, so the control flow and the per-phase detail competed for attention. Extract seven self-contained, individually-testable helpers and leave the orchestrator holding only sequencing plus the early-return gates: unresolvedEntries, reachabilityGate, collectFullScanDeps, narrowDirectDeps, reverseLookupRewrites, buildPinnedEntries, informationalEntries The orchestrator drops from 397 to 131 lines and now reads as a phase list. Behavior-preserving: deps is a slice so in-place ref/SHA mutation stays visible to the caller; dropDeps still reassigns in the parent; the parent-map rekey block stays inline (it shadows the dep package). reverseLookupRewrites returns a non-nil *Entry to signal the impostor early-return rather than returning from inside the helper. Full race suite, 20/20 integration scenarios, and the JSON diagnosis golden all unchanged.
diagnoseOneParsed was ~220 lines mixing two distinct phases: a run of mutually-exclusive terminal preconditions (load error, local-path actions, non-hosted runner, no refs, unreadable deps) that each emit one finding and bail, followed by the happy-path diagnosis (resolve, inventory, three reachability sweeps, RunChecks, finding assembly). Extract the guards into precheckWorkflow, returning (report, handled) so the orchestrator keeps its early-out visible, and lift the three reachability passes into reachabilitySweeps. The main function drops to ~80 lines and now reads as: precheck gate -> resolve -> inventory -> sweeps -> checks -> assembly. Behavior-preserving: precheckWorkflow seeds ActionRefs/ParseWarnings exactly as before; reachabilitySweeps returns the three result sets separately so their (NWO, Ref, SHA) keys stay unmixed downstream. Full race suite, 20/20 integration scenarios, and the JSON golden unchanged.
Two review comments asked to drop the Issue types and Exit status blocks from the root command's --help; they restated information the per-finding output and documented exit codes already convey. Removing them keeps the help focused on usage and the --json example. Also trims internal doc comments in plan.go and diagnose.go to three lines or less. Each was fact-checked against its function body first; the shorter versions keep the non-obvious "why" and drop the restated mechanics.
Workflow ref was @v4 but lockfile had @v4.2.0, so the refs didn't match and the tool re-resolved from scratch. Now both say @v4.2.0, testing the actual 'preserved precision on re-run' invariant.
Transitive deps keep the composite's declared ref (v1, v2, main) when it's a valid symbolic ref. Only bare-SHA refs (40/64 hex chars) are replaced by ReverseLookup's discovered tag/branch, since a SHA is not a valid symbolic ref for the lockfile. Updated scenario descriptions to match the actual contract.
The lockfile key (NWO@ref) now preserves the original SHA that the composite action.yml declared. The ref: metadata field uses the discovered tag/branch from ReverseLookup. This keeps the key stable (matching what the composite declares) while recording the symbolic ref for provenance. state.Set prefers dep.Tag, then dep.Branch over dep.Ref when the ref is a bare SHA (40/64 hex chars). plan.go restores all transitive deps' original refs unconditionally — the SHA/symbolic distinction is now handled at the serialization layer.
transitive_major_ref_not_narrowed, transitive_version_ref_nudge_suppressed, and transitive_provenance_ref_not_narrowed all used the same fixture and tested overlapping invariants. Merged into transitive_ref_preserved with all unique assertions combined.
Expressions in uses: are illegal — they're not a pinning limitation, they're an unparseable workflow entry. Updated the warning copy to reflect this.
The 'Authorize in your web browser' message was written to stdout even when --json was active, corrupting the JSON output. Now gated behind opts.jsonFields == "" in both --no-fix and fix paths.
The fixtures repo's lockfile was upgraded to v0.0.2 in-repo, which advanced the @main commit SHA.
workflowsWithNewPins only checked Resolution==Pinned, so narrowed entries (Verified + AutoFixedRef set) never triggered a lockfile write. The workflow YAML was rewritten @main→v6.0.2 but the lockfile kept the stale @main key. Now narrowed entries also mark the workflow for a Set() call, which updates the dep key and lets Save's GC drop the orphaned @main entry.
When lockfile_deps_cover_direct is set, also verify that every dep key listed under a workflow in the lockfile has a matching uses: line in the actual workflow YAML. Catches bugs where the lockfile is stale (e.g. narrowing rewrites the YAML but not the lockfile).
Non-version refs like main, canary, or releases/v4 are intentional choices. Narrowing them risks picking up unrelated semver tags (e.g. vercel/next.js framework releases instead of the action's tags). Now only imprecise semver refs (v4, v4.2) are narrowing candidates. Also updated the self-hosted runner skip hint to mention -A as an alternative to --allow-runners <labels>.
…abot Moved to dependabot category, added --no-fix and --json=findings since dependabot always consumes JSON. Adjusted exit to 1 (--no-fix with findings) and added jq assertions on the findings array.
The EnsureSentinel header was showing up in every test diff, adding noise unrelated to what the scenario is testing. Now the fixture helpers include it so diffs only show meaningful ref changes.
With graceful ReverseLookup degradation, an unreachable commit produces a reachability-unknown warning (not a hard not-pinned error). Exit is 0 since warnings don't fail --no-fix mode.
The golden breaks every time @main advances in the fixtures repo (chicken-and-egg: pushing the lockfile IS the new @main commit). Replace with structural assertions that prove correctness without pinning to exact SHAs: lockfile_contains the transitive dep key and uses: field, plus deps_cover_direct/indirect.
Same class of stdout pollution as the SSO hint — TermDetail writes to stdout which corrupts JSON output for machine consumers.
Adds a REF NARROWING section to --help explaining what gets narrowed (version-shaped refs like v4, v3.2) vs what doesn't (branches, non- version tags) and why (full semver tags are effectively immutable). Also expands the --no-narrow flag description with the same reasoning.
There was a problem hiding this comment.
⚠️ Not ready to approve
A critical panic risk was introduced in cmd/gh-actions-lock/pin_summary.go when slicing te.SHA by len(te.Ref) without a safe length guard.
Pull request overview
This PR updates gh-actions-lock to align with the upstream actions-lockfile schema changes (pin keys no longer embed commit hashes) and adjusts the resolver/pipeline/UI and test harness to support the v0.0.2 lockfile format and related behaviors.
Changes:
- Bump
github.com/github/actions-lockfile/goand migrate lockfile handling to schema v0.0.2 (owner/repo@refkeys; commit is stored only in metadata). - Rework resolver/pipeline diagnostics to remove the prior reachability/impostor machinery and rely on updated reverse-lookup + forgery/ref-moved checks.
- Expand terminal/UI + integration harness utilities (progress/status helpers, lockfile/workflow coherence assertions, interactive “push” workflow).
File summaries
| File | Description |
|---|---|
| test/scenarios/testdata/transitive_closure_cross_repo.lock | Removes obsolete scenario lockfile fixture (pre-v0.0.2 schema). |
| test/scenarios/catalog.go | Formatting/alignment adjustments in scenario catalog structs. |
| test/integration/run.rb | Updates generated lockfile version, adds new expectations, stubs tags endpoint, adds forgery scenario, and supports deleting lockfile for migration tests. |
| test/integration/harness.rb | Adds lockfile dependency coverage/coherence assertions, improves live clone failure handling, and adds interactive push command support. |
| Makefile | Strengthens local test target and adds vet, fmt, fmt-check targets. |
| internal/workflowfile/workflowfile.go | Improves warning message for expression-based uses:. |
| internal/workflowfile/workflowfile_test.go | Updates test expectation to match new warning wording. |
| internal/workflowfile/runson.go | Adds “allow all runners” wildcard handling and minor formatting updates. |
| internal/workflowfile/runson_test.go | Formatting-only updates in runner label tests. |
| internal/ui/text.go | New terminal-width and UTF-8-safe truncation helpers + pluralization. |
| internal/ui/style.go | New style/color helpers separating log-safe and terminal-only styling. |
| internal/ui/status.go | New status-prefixed narration helpers and terminal summary helpers. |
| internal/ui/progress.go | New progress worker-slot APIs plus optional JSONL progress tracing. |
| internal/tag/pure_test.go | Adds pure/unit tests for tag-related helpers (cooldown, formatting, ordering). |
| internal/resolve/reverse_lookup.go | Changes reverse-lookup to return non-fatal issues for orphaned/bare-SHA cases; adds LooksLikeSHA. |
| internal/resolve/reverse_lookup_test.go | Updates tests to match new ReverseLookup signature. |
| internal/resolve/resolver.go | Removes reachability caching/hooks and updates seeding to match new model. |
| internal/resolve/resolver_test.go | Updates options test after removing reachability override option. |
| internal/resolve/reachability_test.go | Deletes reachability cache/singleflight tests (feature removed). |
| internal/resolve/pick.go | Updates tag selection to prefer full semver tags over major-only tags. |
| internal/resolve/pick_test.go | Expands tests for full-semver preference behavior. |
| internal/resolve/errors.go | Removes ImpostorError type (model removed). |
| internal/resolve/errors_test.go | Removes tests for deleted ImpostorError. |
| internal/resolve/discover_test.go | Updates tests for orphan/tag behavior and new ReverseLookup signature; adds transport stub for issue-only behavior. |
| internal/resolve/cacheentry.go | Removes reachability cache entry type. |
| internal/resolve/cacheentry_test.go | Removes tests for deleted reach cache entry type. |
| internal/resolve/ancestry.go | Removes ReachabilityResult type (reachability subsystem removed). |
| internal/pipeline/run.go | Removes reachability prewarm + impostor enrichment; updates flow comments and collection calls accordingly. |
| internal/pipeline/resolver_test.go | Deletes tests for removed prewarm resolver/reachability behavior. |
| internal/pipeline/reach_partition.go | Deletes reachability partitioning helpers (feature removed). |
| internal/pipeline/reach_findings.go | Deletes reachability complement finding logic (feature removed). |
| internal/pipeline/reach_findings_test.go | Deletes tests for removed reachability complement logic. |
| internal/pipeline/parse.go | Removes reachability prewarm call; only resolves refs before diagnostics. |
| internal/pipeline/impostor_parity_test.go | Deletes tests for removed impostor parity paths. |
| internal/pipeline/finding_enrich_test.go | Adds tests for transitive pin detection and inventory parent population. |
| internal/pipeline/doc_urls.go | Removes impostor-doc wiring and adjusts comments for forgery context. |
| internal/pipeline/doc_urls_test.go | Adds tests for tag-likeness heuristics and doc URL behavior. |
| internal/pipeline/diagnose.go | Refactors diagnostics flow, extracting precheckWorkflow and simplifying resolver adapter usage. |
| internal/pipeline/diagnose_helpers_test.go | Adds unit tests for new helper functions (indexDeps, hasIssues). |
| internal/pipeline/checks/structural.go | Switches structural checks to use lockedPin (commit sourced from metadata). |
| internal/pipeline/checks/run.go | Introduces lockedPin, updates dependency parsing to pull commit from Action metadata. |
| internal/pipeline/checks/resolver.go | Simplifies prewarmed resolver adapter to resolution-only (no reachability). |
| internal/pipeline/checks/parsed.go | Removes SkipReachWhenUnchanged flag (reachability path removed). |
| internal/pipeline/checks/misleading.go | Updates forgery/ref-moved logic to use metadata-sourced SHA; removes live-ref impostor logic. |
| internal/pipeline/checks/impostor.go | Deletes impostor recommendation/enrichment implementation. |
| internal/pipeline/checks/impostor_test.go | Deletes tests for removed impostor enrichment. |
| internal/pipeline/checks/finding.go | Updates wording around recommended tags (no longer impostor-specific). |
| internal/pipeline/checks/category.go | Removes impostor-commit category and updates onboarding remediation text. |
| internal/pipeline/checks/category_test.go | Updates frozen category assertions and inconclusive classification. |
| internal/pin/retain_impostor_test.go | Updates test to use literal issue string after category removal. |
| internal/pin/record.go | Adds Record.Narrowed() helper to surface ref narrowing results. |
| internal/pin/plan_test.go | Removes reachability override setup; updates transitive ref tests and adds sticky-precision narrowing test. |
| internal/pin/commit.go | Updates retention logic to keep Investigate/Unresolved entries; treats narrowed refs as workflow-changing. |
| internal/lockfile/state.go | Migrates read/write to v0.0.2: commit comes from Action.Commit; records original version; supports late metadata resolver injection; adds version extraction helper. |
| internal/lockfile/state_marshal.go | Updates deterministic YAML marshal to emit ref: (no tag/branch fields). |
| internal/lockfile/convertor.go | Updates Pin conversion: pin keys no longer include algo/hex; SHA carried via Action metadata. |
| internal/ghapi/graphql_action_files.go | Improves error messaging for missing refs; adds hex detection helper. |
| internal/ghapi/client.go | Minor formatting alignment. |
| internal/ghapi/cachekey.go | Removes reachability cache key type. |
| internal/ghapi/cachekey_test.go | Removes tests for deleted reachability cache key. |
| internal/dep/dependency.go | Updates comments around Branch semantics under new schema. |
| go.sum | Bumps github.com/github/actions-lockfile/go to v0.0.4. |
| go.mod | Bumps github.com/github/actions-lockfile/go to v0.0.4. |
| cmd/gh-actions-lock/testdata/golden-json/.github/workflows/actions.lock | Updates golden lockfile fixture to v0.0.2 schema. |
| cmd/gh-actions-lock/run.go | Adds --allow-all-runners and --accept-moved, removes check subcommand wiring, suppresses some terminal-only extras during JSON output, and threads new options through planning/summary. |
| cmd/gh-actions-lock/root.go | Expands root help text, removes explicit check subcommand, and loads lockfile before auth/resolver wiring. |
| cmd/gh-actions-lock/pin_summary.go | Updates summary rendering for narrowed refs, schema upgrade notice, and SHA-shaped ref labeling. |
| cmd/gh-actions-lock/pin_summary_test.go | Updates investigation header expectations; removes impostor escalation test. |
| cmd/gh-actions-lock/onboard_gate.go | Updates onboarding remediation copy to match new command surface. |
| cmd/gh-actions-lock/lockrecovery.go | Updates recovery remediation copy to match new command surface. |
| cmd/gh-actions-lock/format/terminal.go | Removes impostor rendering and updates runner allowlist hints. |
| cmd/gh-actions-lock/format/terminal_test.go | Updates tests to exclude forgery category instead of impostor; adjusts strings accordingly. |
| cmd/gh-actions-lock/check_json_golden_test.go | Updates golden JSON contract test to use root command invocation. |
| .github/workflows/test.yml | Strengthens CI workflow with gofmt, go vet, and race-enabled tests. |
Copilot's findings
- Files reviewed: 76/82 changed files
- Comments generated: 3
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
+286
to
+288
| if te.Ref == te.SHA || (len(te.Ref) >= 40 && te.Ref == te.SHA[:len(te.Ref)]) { | ||
| label = te.NWO + "@" + short | ||
| } else { |
| ref.Ref[:12], ref.Owner, ref.Repo) | ||
| case isHexString(ref.Ref): | ||
| // Short hex — ambiguous, might be a truncated SHA | ||
| results[idx].Err = fmt.Errorf("version %q does not resolve — if this is a commit, use the full 40-character SHA", ref.Ref) |
Comment on lines
+361
to
+365
| // The action's Ref must match the pin key's ref (which is d.Ref). | ||
| // Preserve existing ref when the dep arrives without one (carried | ||
| // unchanged from a previous lockfile). | ||
| // When the dep's ref is a bare SHA (transitive dep pinned by commit), | ||
| // prefer the discovered tag or branch for the metadata ref field. |
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.
Summary
Bumps the lockfile parser to v0.0.2 (simplified pin keys) and fixes a slate of bugs discovered through iterative integration testing. The v0.0.2 schema replaces
tag:/branch:fields with a singleref:field and usesNWO@refas the canonical dependency key.Changes
Lockfile schema (v0.0.2)
go/v0.0.4ofgithub/actions-lockfileNWO@ref— tag/branch distinction removedref:gets the discovered tag/branchRef narrowing
v4) are narrowed to the highest matching full semver tag (e.g.v4.3.1) at first pinmain) and non-version tags are never touched (prevents monorepo tag pollution, e.g. vercel/next.js framework releases)--no-narrowdisables entirely; help text explains the rationaleJSON mode correctness
--jsonmode (was corrupting stdout)--jsonmode (same issue)stdout_is_jsonholds for machine consumersGraceful degradation
ReverseLookupno longer hard-fails on orphaned commits — producesreachability-unknownwarning instead--accept-movedflag re-resolves deps flagged as ref-moved/lockfile-forgeryUX improvements
-A/--allow-all-runnersflag to skip all runner label checks--allow-runnerssupports wildcard*-AflagTest infrastructure
lockfile_deps_cover_direct)transitive_closure_cross_repo— structural assertions prove correctness without breaking on@mainadvancesdbot_impostor_blocksupdated for graceful degradation behaviorRefactoring
checksubcommand)planWorkflowsplit into named phase helpersinternal/uisplit into cohesive filesgo vet, race detector,gofmtTesting
go test ./...— all passnodeselector/actions-test-fixtures(composite actions, transitive closure, impostor commits)bbq-beets/nodeselector