Resolve dependencies live on every run - #119
Open
nodeselector wants to merge 4 commits into
Open
Conversation
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Live traversal can prune retained transitive pins, repository transfers remain unresolved, and failed resolutions can multiply API requests.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review tier: Balanced
Findings: 1
New issues introduced by this change (3)
| Severity | Finding |
|---|---|
 |
internal/pipeline/run.go — This does not implement the linked issue's repository-transfer outcome. Although the GraphQL… |
|
internal/pipeline/run.go — Failed refs are not cached, yet the prewarm error is discarded and DiagnoseParsed calls… |
 |
internal/pipeline/run.go — Live traversal is not safe to use as the authority for pruning the retained lockfile closure. If a… |
What changed in this PR
Makes all networked runs resolve scoped dependencies live while retaining moved pins unless explicitly advanced.
Changes:
- Removes
--rescan, lockfile cache seeding, and fast-path logic. - Surfaces moved-ref and inconclusive warnings.
- Updates documentation and tests for default live resolution.
| File | Description |
|---|---|
README.md |
Documents live resolution and --relock. |
internal/resolve/resolver.go |
Removes lockfile cache seeding. |
internal/pipeline/run.go |
Always resolves scoped workflows live. |
internal/pipeline/run_test.go |
Removes obsolete fast-path tests. |
internal/pipeline/parse.go |
Removes recorded-ref filtering. |
internal/pipeline/checks/parsed.go |
Removes fast-path helpers. |
internal/pipeline/checks/parsed_test.go |
Removes obsolete helper tests. |
internal/pipeline/checks/misleading.go |
Adds correct relock guidance. |
internal/pipeline/checks/finding.go |
Removes strict inconclusive gate helper. |
cmd/gh-actions-lock/verify.go |
Makes verification read-only. |
cmd/gh-actions-lock/verify_test.go |
Updates verification tests. |
cmd/gh-actions-lock/run.go |
Removes the rescan CLI path. |
cmd/gh-actions-lock/prune_workflow_test.go |
Adds live-resolution mocks. |
cmd/gh-actions-lock/pin_summary.go |
Surfaces live-resolution warnings. |
cmd/gh-actions-lock/pin_summary_test.go |
Tests warning detection. |
cmd/gh-actions-lock/format/terminal.go |
Renders moved and inconclusive warnings. |
cmd/gh-actions-lock/format/terminal_test.go |
Tests warning output. |
cmd/gh-actions-lock/command_test.go |
Exercises default live resolution. |
cmd/gh-actions-lock/check_json_golden_test.go |
Updates JSON test invocation. |
test/scenarios/catalog.yml |
Removes rescan scenarios and updates expectations. |
test/integration/run.rb |
Removes the rescan fixture. |
test/integration/harness.rb |
Removes interactive rescan commands. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Transitive drift is not checked, and partial relocking can persist an incomplete dependency closure.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review tier: Balanced
Findings: 2
New issues introduced by this change (2)
| Severity | Finding |
|---|---|
|
internal/pipeline/diagnose.go — RecordedDeps is only consulted after RunChecks, while RunChecks builds its moved-ref index… |
|
internal/lockfile/state.go — Dropping recorded edges whenever the parent commit changes can persist an incomplete closure.… |
Issues resolved since last review (3)
| Severity | Finding |
|---|---|
|
internal/pipeline/run.go — Live traversal is not safe to use as the authority for pruning the retained lockfile closure. If a… View resolved comment |
|
internal/pipeline/run.go — Failed refs are not cached, yet the prewarm error is discarded and DiagnoseParsed calls… View resolved comment |
|
internal/pipeline/run.go — This does not implement the linked issue's repository-transfer outcome. Although the GraphQL… View resolved comment |
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Terminal --verify can silently omit resolver failures because the new warning path only runs in fix mode.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review tier: Balanced
Findings: 1
New issues introduced by this change (1)
| Severity | Finding |
|---|---|
|
cmd/gh-actions-lock/pin_summary.go — renderResolverWarning is only reached from the fix-mode summary, but --no-fix and --verify… |
Issues resolved since last review (2)
| Severity | Finding |
|---|---|
|
internal/lockfile/state.go — Dropping recorded edges whenever the parent commit changes can persist an incomplete closure.… View resolved comment |
|
internal/pipeline/diagnose.go — RecordedDeps is only consulted after RunChecks, while RunChecks builds its moved-ref index… View resolved comment |
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Relocking can skip a moved transitive dependency when an unrelated new direct dependency is also present.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review tier: Balanced
Findings: 1
New issues introduced by this change (1)
| Severity | Finding |
|---|---|
|
internal/pin/plan.go — repinMoved does not ensure a moved transitive dependency is included in the re-resolution. If the… |
Issues resolved since last review (1)
| Severity | Finding |
|---|---|
|
cmd/gh-actions-lock/pin_summary.go — renderResolverWarning is only reached from the fix-mode summary, but --no-fix and --verify… View resolved comment |
Comment on lines
+135
to
+136
| repinMoved := repinsMoved(opts) && wr.CountByCategory(checks.RefMoved) > 0 || | ||
| opts.AcceptMoved && wr.CountByCategory(checks.UnreachablePin) > 0 |
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.
What
Remove
--rescanand make every networkedgh actions-lockrun live-resolve the current explicitly scoped dependency closure.Why
Normal runs previously trusted mutable lockfile entries while still mutating the lockfile from a shallow workflow comparison. That made first generation and later runs behave differently and required a separate mode to catch moved refs. The simpler model is: networked runs resolve live; explicit flags control whether moved pins may advance.
How
--relockor--accept-movedadvances it only after complete recursive resolution, then replaces obsolete child edges with the live graph.--verify-localoffline and normal inconclusive results non-blocking.Scope
Repository-transfer and canonical-NWO handling remain separate in PR #118. This PR stays independent and does not claim that outcome.
Risk
The behavior change increases GitHub API traffic for existing lockfiles. Dependabot does not pass
--rescan, so its CLI contract is unchanged, but its REST-only path will also live-resolve each scoped closure after it adopts this binary. Its next pinned-binary bump should stage-test request volume and latency before release.This is easy to roll back by reverting the commits.
Testing
go test ./...make test-stub(34 scenarios)script/release-test--rescanis absent