Rewrite transferred action references to canonical repositories - #118
Open
nodeselector wants to merge 3 commits into
Open
Rewrite transferred action references to canonical repositories#118nodeselector wants to merge 3 commits into
nodeselector wants to merge 3 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
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 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