Resolve dependencies live on every run - #119
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 |
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Recorded transitive dependencies are omitted from diagnostic inventory for current lockfile graphs.
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/pipeline/diagnose.go — RecordedDeps now contains the full lockfile closure, but the inventory populated immediately… |
Issues resolved since last review (1)
| Severity | Finding |
|---|---|
|
internal/pin/plan.go — repinMoved does not ensure a moved transitive dependency is included in the re-resolution. If the… View resolved comment |
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Transitive finding order is nondeterministic, and resolver warnings are duplicated in terminal output.
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 |
|---|---|
|
cmd/gh-actions-lock/run.go — format.PresentResults now renders ReachabilityUnknown warnings, including this same f.Detail,… |
|
internal/pipeline/checks/run.go — Adding every recorded closure entry to depIndex makes transitive findings nondeterministic:… |
Issues resolved since last review (1)
| Severity | Finding |
|---|---|
|
internal/pipeline/diagnose.go — RecordedDeps now contains the full lockfile closure, but the inventory populated immediately… View resolved comment |
Suppressed comments (1)
cmd/gh-actions-lock/pin_summary.go:99
- This repeats workflow-level resolver failures already emitted by
format.PresentResultsearlier in the fix-mode flow. When there are no unresolved record entries—the normal case for the aggregateReachabilityUnknownfinding—the same detail is printed again in the summary. Keep a single rendering path for this warning.
} else {
renderResolverWarning(console, report)
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Live parent edges can contaminate retained closures, and direct bare-SHA references can receive false movement warnings.
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 (2)
| Severity | Finding |
|---|---|
|
internal/pipeline/diagnose.go — Do not merge the live parent graph into the recorded inventory before movement is accepted. If a… |
|
internal/pipeline/checks/run.go — Adding every recorded pin here makes a symbolic lock root for a direct bare-SHA workflow reference… |
Issues resolved since last review (1)
| Severity | Finding |
|---|---|
|
internal/pipeline/diagnose.go — A direct workflow use pinned by SHA is no longer recognized when its lockfile entry retains a… View resolved comment |
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
A scoped relock can discard shared transitive edges required by an out-of-scope workflow.
Review tier: Balanced
Findings: None
Issues resolved since last review (2)
| Severity | Finding |
|---|---|
|
internal/pipeline/checks/run.go — Adding every recorded pin here makes a symbolic lock root for a direct bare-SHA workflow reference… View resolved comment |
|
internal/pipeline/diagnose.go — Do not merge the live parent graph into the recorded inventory before movement is accepted. If a… View resolved comment |
Suppressed comments (2)
Previously missed (1) — in code that hasn't changed since the last review.
test/scenarios/catalog.yml:1912
- The updated expectation retains the pin, but the scenario is still named
dbot_transient_403_drops_pin. Rename it to describe the asserted retained-pin behavior so scenario selection and failures are not misleading.
internal/lockfile/state.go:469
- When a scoped relock advances a parent shared with an out-of-scope workflow, this condition drops every previously merged
usesedge because the commit changed.usesis a global union for anNWO@Ref(including different sub-action paths), soSavecan then garbage-collect children still required by the untouched workflow, violating scoped-run preservation. Track edge contributions per workflow/path, or otherwise retain/recompute edges belonging to workflows outside the current scope before replacing the scoped graph.
if existing.Commit == commit {
for _, u := range existing.Uses {
usesSet[u] = true
}
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
A moved ref combined with a newly fixable dependency can incorrectly produce a nonzero exit.
Review tier: Balanced
Findings: None
Suppressed comments (1)
internal/pin/plan.go:136
- Default runs now emit
ref-moved, but this remains non-blocking only when the planner returns before resolving anything. If the same workflow also has an unpinned/new ref, planning continues andinformationalEntriesconverts the moved ref into anInvestigateentry; both JSON mode and the terminal summary then returnerrSilentafter successfully pinning the new ref. A moved ref should remain a warning regardless of unrelated fixable findings—excludeRefMovedfrom investigation/exit gating unless an explicit policy requires failure, and add a mixed moved+new-ref regression test.
repinMoved := repinsMoved(opts) && wr.CountByCategory(checks.RefMoved) > 0 ||
opts.AcceptMoved && wr.CountByCategory(checks.UnreachablePin) > 0
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Per-workflow scoped commits can overwrite shared global pins with conflicting partial-resolution snapshots.
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/commit.go — SetScoped is applied one workflow at a time even though dependency entries are global. If two… |
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Pathless recorded transitive resolution and order-dependent scoped graph writes can produce incorrect results.
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 (1)
| Severity | Finding |
|---|---|
|
internal/lockfile/state.go — Skipping every workflow in scopedWorkflows assumes each scoped plan will successfully replace its… |
Pre-existing issues (1)
| Severity | Finding |
|---|---|
|
internal/pin/commit.go — SetScoped is applied one workflow at a time even though dependency entries are global. If two… View comment |
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
internal/pipeline/parse.go:143
GetClosurereconstructs dependencies from lockfile pins withPath == ""(internal/lockfile/convertor.go:35-43). Converting every recorded transitive dependency back into anActionReftherefore asksResolveAllRecursiveto inspect the repository-rootaction.yml, not the subpath that the parent composite actually used. For a pin such asowner/repo/save@v1, a root composite can introduce unrelated traversal or even a false blockingCompositeLocalPathError. Resolve recorded closure entries with a ref-only lookup, or persist/recover their paths; recursive action-file discovery should start from path-aware current roots.
refs = append(refs, parserlock.ActionRef{
Owner: owner,
Repo: repo,
Path: d.Path,
Ref: d.Ref,
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Live graph reconciliation, ref casing, and integration fixture handling contain correctness issues.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review tier: Balanced
Findings: 3
New issues introduced by this change (2)
| Severity | Finding |
|---|---|
|
internal/lockfile/state.go — This always unions the prior Uses whenever the parent SHA is unchanged. A complete scan where an… |
|
internal/pin/commit.go — Lowercasing the entire NWO@​ref key conflates distinct case-sensitive Git refs such as Release… |
Pre-existing issues (1)
| Severity | Finding |
|---|---|
|
internal/lockfile/state.go — Skipping every workflow in scopedWorkflows assumes each scoped plan will successfully replace its… View comment |
Issues resolved since last review (1)
| Severity | Finding |
|---|---|
|
internal/pin/commit.go — SetScoped is applied one workflow at a time even though dependency entries are global. If two… View resolved comment |
Suppressed comments (4)
Previously missed (3) — in code that hasn't changed since the last review.
test/integration/run.rb:349
- This is a snapshot of the mutable
v4ref and feeds live-token scenarios such asdbot_onboarded_clean, which requires zero findings. Since normal runs now resolvev4live, the next upstream move will emitref-movedand break those scenarios until this constant is manually updated again. Resolve the fixture head during setup or use an immutable ref for clean live scenarios, keeping movement behavior in stubbed tests.
test/integration/run.rb:586 - The generated GraphQL query contains only the
$expr0placeholder; the actual ref is sent inbody["variables"]["expr0"]. These branches therefore never match, so themainandv4.2.0stubs always returnCHECKOUT_SHA. Select the fixture SHA from the expression variable instead.
internal/lockfile/state.go:477 ponytailis unrelated to the invariant being documented and makes this comment confusing. Remove the stray label so the comment starts with the lockfile rationale.
// ponytail: actions.lock stores a global edge union, so keep the
internal/pipeline/diagnose.go:66
- The recursive parent map is discarded here, and the live dependencies are used only for ref checks; inventory and planning below still use only the recorded graph. If a workflow switches from
owner/action/old@v1toowner/action/new@v1, the lock key and SHA are unchanged but the composite children may differ, so no finding triggers replanning and the lockfile keeps the old closure. Carry the successfully resolved live graph into planning and reconcile it when resolution is complete, while retaining the recorded graph for moved or partial resolutions.
liveDeps, _, resolveErr = r.ResolveAllRecursive(ctx, pw.Refs)
| if !isPinOrVerified(e.Resolution) { | ||
| continue | ||
| } | ||
| key := strings.ToLower(e.NWO + "@" + e.Ref) |
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Incomplete and blocked resolutions can lose recorded or partially resolved graph edges, and obsolete moved dependencies can prevent required graph replacement.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review tier: Balanced
Findings: 3
New issues introduced by this change (2)
| Severity | Finding |
|---|---|
|
internal/lockfile/state.go — Incomplete updates still contain successfully resolved edges. Skipping their entire ParentMap… |
|
internal/pipeline/diagnose.go — A CompositeLocalPathError sets LiveComplete false and causes the blocking-error branch below to… |
Pre-existing issues (1)
| Severity | Finding |
|---|---|
|
internal/pin/commit.go — Lowercasing the entire NWO@​ref key conflates distinct case-sensitive Git refs such as Release… View comment |
Issues resolved since last review (2)
| Severity | Finding |
|---|---|
|
internal/lockfile/state.go — This always unions the prior Uses whenever the parent SHA is unchanged. A complete scan where an… View resolved comment |
|
internal/lockfile/state.go — Skipping every workflow in scopedWorkflows assumes each scoped plan will successfully replace its… View resolved comment |
Suppressed comments (1)
internal/pin/plan.go:139
- This rejects the whole live graph for movement in any recorded dependency, including one that is no longer in the live closure. For example, after switching sub-action paths at the same repository/ref, an obsolete recorded child that has moved makes
movementRejectedtrue; because the root key is unchanged there is no structural finding, so planning retains the old child and never locks the new live child. Only movement for dependencies still present inwr.LiveDepsshould veto graph replacement.
movementRejected := wr.CountByCategory(checks.RefMoved) > 0 && !repinsMoved(opts) ||
wr.CountByCategory(checks.UnreachablePin) > 0 && !opts.AcceptMoved
useLiveGraph := wr.LiveComplete && (wr.LiveGraphChanged || repinMoved) && !movementRejected
| if !update.ReplaceGraph { | ||
| continue | ||
| } |
| wr.LiveDeps, wr.LiveParents = alignLiveRootKeys(liveDeps, liveParents, pw.RecordedDeps, pw.Refs) | ||
| wr.LiveComplete = recursiveErr == nil | ||
| wr.LiveGraphChanged = wr.LiveComplete && !sameGraph(wr.LiveDeps, wr.LiveParents, pw.RecordedDeps, pw.RecordedParents) |
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