Repository navigation
Don't narrow transitive composite dependencies - #54
Merged
Merged
Conversation
Narrowing and reverse-lookup ref rewriting were applied to every resolved dependency, including transitive deps discovered from a composite action's action.yml. Those refs belong to the composite author and never appear in our workflow YAML, so rewriting them is pure churn — and could invent refs the composite never declared (e.g. narrowing actions/setup-go@v5 to v5.6.0, or reverse-looking-up a bare-SHA subpath into the malformed pin actions/attest-build-provenance@predicate@1.1.4). Gate narrowing and ref rewriting to direct deps only. Transitive deps keep the ref the composite declares; ReverseLookup still runs to populate their tag/branch metadata (the lockfile write requires a branch), but its ref rewrites are suppressed for them. Same gate added to narrowVerifiedEntries for already-recorded entries. Adds DirectTracker.IsDirect and a planWorkflow regression test that fails if a transitive dep is narrowed.
Two narrowing-category scenarios pin the rule that transitive deps of a composite action keep the ref the composite declares. Both consume the real reproducer cli/gh-extension-precompile@v2.1.0: - transitive_major_ref_not_narrowed: actions/setup-go@v5 must stay v5, not narrow to v5.x.y. - transitive_provenance_ref_not_narrowed: actions/attest-build-provenance@v1 (a second-hop dep that also declares a bare-SHA subpath internally) must stay v1, not narrow to v1.x.y. Verified live: both pass on the fixed binary and fail on the pre-fix binary.
There was a problem hiding this comment.
✅ Ready to approve
The direct-only gating for narrowing/ref rewrites aligns with the PR goal and is covered by new unit and catalog tests, with only a minor test-stubbing nit noted.
Note: this review does not count toward required approvals for merging.
Pull request overview
This PR updates the pinning/narrowing pipeline to avoid rewriting refs for transitive dependencies discovered via composite actions’ action.yml, limiting narrowing and ref normalization to workflow-direct uses: entries only. This reduces churn in regenerated lockfiles and prevents accidental/invented ref rewrites in composite-internal dependency chains.
Changes:
- Added
DirectTracker.IsDirect(i)and used it as the single source of truth for “directness” when deciding whether to narrow or rewrite refs. - Updated
planWorkflowto (a) only narrow direct deps, and (b) preserve transitive deps’ declared refs acrossReverseLookupwhile still retaining discovered tag/branch metadata. - Updated
narrowVerifiedEntriesto only narrow already-recorded entries that are markedDirect, plus added new unit + catalog scenarios covering transitive cases.
File summaries
| File | Description |
|---|---|
test/scenarios/catalog.yml |
Adds catalog scenarios to assert transitive deps pulled in by a composite are not narrowed. |
internal/pin/plan.go |
Gates narrowing + ref rewrite behavior on workflow-direct deps; preserves transitive refs across ReverseLookup. |
internal/pin/plan_test.go |
Adds a unit test ensuring transitive deps aren’t narrowed even when a full-semver tag exists at the same SHA. |
internal/lockfile/direct_tracker.go |
Adds IsDirect(i) helper to query index-aligned directness consistently. |
Copilot's findings
- Files reviewed: 4/4 changed files
- Comments generated: 1
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.
| }), | ||
| ) | ||
|
|
||
| // Reverse-lookup stubs (branch + tag listing) for both repos. |
The reverse-lookup path calls GET repos/{owner}/{repo} to learn the default
branch. The test left it unstubbed, so DiscoverContaining silently ran its
default-branch-unknown fallback. Stub the repo-metadata call for both repos so
the test exercises the representative path. Addresses PR review feedback.
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.
Why
When a workflow uses a composite action,
gh actions-lockwas rewriting the refs of the composite's internal (transitive) dependencies, not just the actions that literally appear in the workflow YAML. Those refs belong to the composite author, so narrowing them is pure churn, and in some cases it invented refs the composite never declared.Concretely, on a fresh regen of a repo that uses
cli/gh-extension-precompile@v2.1.0:actions/setup-go@v5(declared by the composite) got narrowed tov5.6.0actions/attest-build-provenance@v1got narrowed tov1.4.4@pin likeactions/attest-build-provenance@predicate@1.1.4:sha1-...Approach
Scope narrowing and ref rewriting to direct deps only. A dep is direct when its ref literally appears in a workflow
uses:line; everything discovered from a composite'saction.ymlis transitive.DirectTracker.IsDirect(i)(index-aligned with the resolved deps) as the single source of truth for directness.planWorkflow, gate the narrowing loop onIsDirect(i), and preserve transitive deps' declared refs acrossReverseLookup. ReverseLookup still runs on transitive deps because it populates the tag/branch metadata the lockfile write requires, but its ref rewrites are suppressed for them.Direct-only gate innarrowVerifiedEntriesfor already-recorded entries.Tests
TestPlanWorkflow_DoesNotNarrowTransitiveDepsbuilds a composite -> transitive graph where the transitive dep has a narrower full-semver tag at the same commit, and asserts the transitive ref is left alone. Verified it fails when the gate is removed.narrowingscenarios (transitive_major_ref_not_narrowed,transitive_provenance_ref_not_narrowed) drive the realcli/gh-extension-precompile@v2.1.0reproducer. Verified live that both pass on the fixed binary and fail on the pre-fix binary.Note for reviewers
There is a separate, intentionally-unfixed gap: an incremental run will not backfill a lockfile that is already missing its transitive closure. Fixing that would force network calls on the fast path, which we want to keep fast.
--rescan(or deleting the lockfile) heals it. This PR does not touch that path.