Rewrite transferred action references to canonical repositories - #118
Rewrite transferred action references to canonical repositories#118nodeselector wants to merge 19 commits into
Conversation
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Mandatory rewrites can be silently skipped, and dependency filtering can invalidate transfer directness tracking.
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/pin/plan.go — A transfer rewrite must not be best-effort. RewriteActionRefs skips anchored uses: scalars, and… |
|
internal/pin/plan.go — Rebuild the rewrite tracker after lookupIssues filter deps: filtering can shift indices, while… |
What changed in this PR
Canonicalizes renamed or transferred action repositories across resolution, source rewriting, and lockfile generation.
Changes:
- Preserves original and canonical repository identities during resolution.
- Rewrites direct workflow and local-action references while rejecting remote-composite transfers.
- Adds canonicalization and integration coverage.
| File | Description |
|---|---|
test/scenarios/catalog.yml |
Adds a transfer rewrite scenario. |
internal/resolve/discovery.go |
Tracks original refs and transfer errors. |
internal/pipeline/diagnose.go |
Exposes live resolution details. |
internal/pipeline/checks/resolver.go |
Resolves original aliases from prewarmed data. |
internal/pipeline/checks/finding.go |
Extends workflow reports. |
internal/pin/plan.go |
Plans canonical rewrites and transfer validation. |
internal/pin/plan_test.go |
Tests transfer planning. |
internal/lockfile/direct_tracker.go |
Recognizes transferred direct refs. |
internal/ghapi/graphql_action_files.go |
Reads canonical GraphQL repository names. |
internal/ghapi/graphql_action_files_test.go |
Tests GraphQL canonicalization. |
internal/dep/dependency.go |
Stores and merges original refs. |
internal/dep/dependency_test.go |
Tests transferred-source deduplication. |
cmd/gh-actions-lock/command_test.go |
Tests CLI rewriting and rejection flows. |
💡 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
🔵 Needs a closer look
REST-based resolution paths still lose canonical repository identity and can retain stale transferred references.
Review tier: Balanced
Findings: None
Issues resolved since last review (2)
| Severity | Finding |
|---|---|
|
internal/pin/plan.go — Rebuild the rewrite tracker after lookupIssues filter deps: filtering can shift indices, while… View resolved comment |
|
internal/pin/plan.go — A transfer rewrite must not be best-effort. RewriteActionRefs skips anchored uses: scalars, and… View resolved comment |
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
internal/ghapi/graphql_action_files.go:32
- Transferred repositories are still missed whenever
ResolveActionFilesuses its REST-only or anonymous fallback path.resolveAnonymousreturns the requestedOwner/Repoand never setsOriginalNWO, even though its HTTP client follows repository redirects, so the resolver creates noOriginalRefs; the workflow and lockfile therefore retain the stale NWO (and remote-composite redirects are not rejected). Capture the canonical repository from the REST response/final URL and populate these identity fields consistently for both resolution paths.
Owner string
Repo string
OriginalNWO string
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Read-only verification modes do not surface transferred repositories because transfer validation occurs only during planning.
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 — Transfer identity is only retained for pin.Plan, but read-only commands return immediately after… |
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Read-only verification misses transfers, while partial failures can misclassify successful redirects and retain stale lock entries.
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/pin/plan.go — A successful redirect is represented only by its canonical key, but unresolvedEntries below marks… |
|
internal/pipeline/checks/resolver.go — --no-fix/--verify returns before the planner runs (cmd/gh-actions-lock/run.go:331-357), and… |
Issues resolved since last review (1)
| Severity | Finding |
|---|---|
|
internal/pipeline/diagnose.go — Transfer identity is only retained for pin.Plan, but read-only commands return immediately after… View resolved comment |
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Existing stale lockfiles still hide transfers during normal runs because resolver cache prewarming bypasses canonical identity discovery.
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 — Normal runs with an existing lockfile still miss this transfer.… |
Issues resolved since last review (2)
| Severity | Finding |
|---|---|
|
internal/pipeline/checks/resolver.go — --no-fix/--verify returns before the planner runs (cmd/gh-actions-lock/run.go:331-357), and… View resolved comment |
|
internal/pin/plan.go — A successful redirect is represented only by its canonical key, but unresolvedEntries below marks… View resolved comment |
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
REST resolution and default mutable-ref fast paths can still preserve stale transferred repository identities.
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/run.go — A normal run still skips live resolution when an existing lockfile contains only mutable refs such… |
Issues resolved since last review (1)
| Severity | Finding |
|---|---|
|
internal/pin/plan.go — Normal runs with an existing lockfile still miss this transfer.… View resolved comment |
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
internal/ghapi/graphql_action_files.go:33
- Repository transfers are still invisible in the supported REST-only/anonymous fallback path.
ResolveActionFilescallsresolveAnonymouswhenGH_ACTIONS_LOCK_DEPENDABOT_PROXY=1(and for eligible GraphQL failures), but that function always returns the requested owner/repo and never setsOriginalNWO; the HTTP client follows GitHub's redirect, so resolution succeeds under the stale identity and no rewrite is planned. Capture the final redirected NWO (or fetch canonical repository metadata) in the REST path and populate these fields consistently.
Owner string
Repo string
OriginalNWO string
Path string
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Deduplication can retain stale dependency data, and rewrite validation can modify or reject non-action uses fields.
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/dep/dependency.go — This keeps every field from the first dependency and only merges provenance. In a partially… |
Issues resolved since last review (1)
| Severity | Finding |
|---|---|
|
cmd/gh-actions-lock/run.go — A normal run still skips live resolution when an existing lockfile contains only mutable refs such… View resolved comment |
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
internal/workflowfile/rewrite.go:141
- This traversal treats every mapping key named
usesas an action reference, whereas extraction intentionally ignores locations such asenv.usesandwith.uses(workflowfile.go:422-426,workflowfile_test.go:434-449). If one of those data fields contains the same old reference, an anchor there can incorrectly block an otherwise writable transfer; without an anchor it is counted and rewritten as ordinary data. Restrict required matching and rewriting to the GitHub Actions-supporteduseslocations used bywalkUses.
if keyNode.Value == "uses" {
target := valueNode
if valueNode.Kind == yaml.AliasNode && valueNode.Alias != nil {
target = valueNode.Alias
}
if target.Kind == yaml.ScalarNode {
oldValue := strings.TrimSpace(target.Value)
if _, ok := replacements[oldValue]; ok {
matches[oldValue]++
if anchored || valueNode.Kind == yaml.AliasNode || target.Anchor != "" {
blocked = oldValue
}
}
}
}
walk(valueNode, anchored, depth+1)
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
A known REST-detected redirect can still pass verification when subsequent GraphQL resolution fails.
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/run.go — A successful identity lookup that returns a different canonical NWO is reduced to false, so the… |
Issues resolved since last review (1)
| Severity | Finding |
|---|---|
|
internal/dep/dependency.go — This keeps every field from the first dependency and only merges provenance. In a partially… View resolved comment |
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Identity lookup and resolution failures can still allow stale transferred references to pass or finish successfully without rewriting.
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/run.go — When REST identifies a transfer but GraphQL resolution fails, this adds only a RefChanged finding… |
|
internal/pipeline/run.go — A failed repository-identity request is converted to canonical == "", and this condition then… |
Issues resolved since last review (1)
| Severity | Finding |
|---|---|
|
internal/pipeline/run.go — A successful identity lookup that returns a different canonical NWO is reduced to false, so the… View resolved comment |
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
REST fallback and mutable-lockfile transitive paths can still preserve stale repository identities.
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/run.go — This only validates repositories named by the workflow's direct mutable refs. For a fully recorded… |
|
internal/ghapi/graphql_action_files.go — Canonicalization only occurs for GraphQL results. ResolveActionFiles also returns… |
Issues resolved since last review (2)
| Severity | Finding |
|---|---|
|
internal/pipeline/run.go — A failed repository-identity request is converted to canonical == "", and this condition then… View resolved comment |
|
internal/pipeline/run.go — When REST identifies a transfer but GraphQL resolution fails, this adds only a RefChanged finding… View resolved comment |
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Recorded stale transitive edges can incorrectly produce blocking transfer findings after live resolution proves they were removed.
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/pipeline/run.go — This reintroduces transfers found only in the recorded lockfile closure even after live resolution… |
Pre-existing issues (1)
| Severity | Finding |
|---|---|
|
internal/ghapi/graphql_action_files.go — Canonicalization only occurs for GraphQL results. ResolveActionFiles also returns… View comment |
Issues resolved since last review (1)
| Severity | Finding |
|---|---|
|
internal/pipeline/run.go — This only validates repositories named by the workflow's direct mutable refs. For a fully recorded… View resolved comment |
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
The security-sensitive resolver, rewrite, and lockfile changes span multiple interacting execution paths and warrant final human validation.
Review tier: Balanced
Findings: None
Issues resolved since last review (2)
| Severity | Finding |
|---|---|
|
internal/pipeline/run.go — This reintroduces transfers found only in the recorded lockfile closure even after live resolution… View resolved comment |
|
internal/ghapi/graphql_action_files.go — Canonicalization only occurs for GraphQL results. ResolveActionFiles also returns… View resolved comment |
What changed
nameWithOwnerwhen an action repository was renamed or transferred.$/action references from the old NWO to the canonical NWO, preserving subpaths and refs.--rescan.Why
The CLI previously discarded
nameWithOwner, reported a successful lockfile, and left the runner unable to resolve the stale repository identity.Testing
go test ./...ruby test/integration/run.rb transferred_repository_rewritten--rescan, immediate--verify-local, local$/rewrite planning, remote-composite rejection, and canonical dedup coverageFixes #110