Pinning: repair SHA refs from existing lock metadata - #116
Conversation
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Rekeying a composite dependency can discard its transitive dependency graph.
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 — Rekeying only this entry drops the repaired action's transitive dependency graph. On the fast path,… |
What changed in this PR
Repairs SHA-based references using symbolic metadata already stored in the lockfile.
Changes:
- Preserves tag and branch metadata in verified entries.
- Rewrites direct SHA references without API requests.
- Adds an end-to-end regression test.
| File | Description |
|---|---|
internal/pin/plan.go |
Restores symbolic refs from lock metadata. |
cmd/gh-actions-lock/selfrepository_test.go |
Tests nested self-repository action repair. |
💡 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
Partial scans can rewrite a shared local action without updating every affected workflow’s lock entry.
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 — When pinning only a subset of workflows, this rewrite can modify a $/ action file that is also… |
Issues resolved since last review (1)
| Severity | Finding |
|---|---|
|
internal/pin/plan.go — Rekeying only this entry drops the repaired action's transitive dependency graph. On the fast path,… View resolved comment |
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Scan-scope detection and dependency-edge preservation can currently reject valid runs or lose transitive lockfile entries.
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/pin/plan.go — The partial-scan guard currently rejects the command even when --no-narrow is set (or no tagger… |
|
internal/pin/plan.go — Inferring scan scope from lockfile keys makes a real full-directory scan look partial whenever the… |
|
internal/lockfile/state.go — The SHA-keyed entry is consulted only when the destination symbolic key is absent. If both keys… |
Issues resolved since last review (1)
| Severity | Finding |
|---|---|
|
internal/pin/plan.go — When pinning only a subset of workflows, this rewrite can modify a $/ action file that is also… View resolved comment |
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Partial-scan detection can reject valid full scans and unnecessarily fails when narrowing is disabled.
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/pin/plan.go — This guard also rejects partial scans with --no-narrow, even though narrowVerifiedEntries… |
 |
internal/pipeline/checks/parsed.go — SelfActionRefs is exported but has no doc comment describing how it differs from Refs and… |
|
internal/pipeline/checks/finding.go — SelfActionRefs is exported but undocumented; its local-action-only scope is important to the… |
Issues resolved since last review (3)
| Severity | Finding |
|---|---|
|
internal/lockfile/state.go — The SHA-keyed entry is consulted only when the destination symbolic key is absent. If both keys… View resolved comment |
|
internal/pin/plan.go — Inferring scan scope from lockfile keys makes a real full-directory scan look partial whenever the… View resolved comment |
|
internal/pin/plan.go — The partial-scan guard currently rejects the command even when --no-narrow is set (or no tagger… View resolved comment |
Suppressed comments (1)
internal/pin/plan.go:91
- Comparing the report only with current lockfile keys misclassifies a full scan whenever the lockfile contains a deleted workflow. The CLI intentionally discovers those stale keys and prunes them only after
Plan, so a normal full run that should prune a deleted workflow will instead hit the partial-scan error when repairing a local action. Pass the invocation's actual full/partial-scan status into planning, or exclude the already-detected stale keys before this check.
poolErr := pinpool.RunTyped(opts.Pool, ctx, "Planning pins",
items,
func(iwr indexedWR) string { return "planning " + iwr.wr.Path },
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The global sticky-ref guard can prevent the intended SHA repair, and branch repairs produce inaccurate summary output.
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/pin/plan.go — Handle the bare-SHA metadata repair before the sticky imprecise-ref guard. prevImpreciseNWO is… |
|
internal/pin/plan.go — A branch-only metadata repair also sets AutoFixedRef, so Record.Narrowed() passes this entry to… |
Issues resolved since last review (3)
| Severity | Finding |
|---|---|
|
internal/pipeline/checks/finding.go — SelfActionRefs is exported but undocumented; its local-action-only scope is important to the… View resolved comment |
|
internal/pipeline/checks/parsed.go — SelfActionRefs is exported but has no doc comment describing how it differs from Refs and… View resolved comment |
|
internal/pin/plan.go — This guard also rejects partial scans with --no-narrow, even though narrowVerifiedEntries… View resolved comment |
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
Rekeying can collide with an existing symbolic ref at a different commit and produce order-dependent lock metadata.
Review tier: Balanced
Findings: None
Issues resolved since last review (2)
| Severity | Finding |
|---|---|
|
internal/pin/plan.go — A branch-only metadata repair also sets AutoFixedRef, so Record.Narrowed() passes this entry to… View resolved comment |
|
internal/pin/plan.go — Handle the bare-SHA metadata repair before the sticky imprecise-ref guard. prevImpreciseNWO is… View resolved comment |
Suppressed comments (1)
internal/pin/plan.go:673
- Before rekeying to the metadata ref, check whether the lockfile already has that NWO@ref at a different commit. This is especially possible for branch metadata: a SHA entry may record
mainat an old commit while another workflow recordsmainat its newer commit. Both entries then collapse to the same key inState.Set, which unions incompatibleusesgraphs and overwrites one commit based on workflow order. Only perform the zero-network repair when the target entry is absent or records the same SHA; otherwise resolve or report the conflict before source files are rewritten.
e.Ref = newRef
e.AutoFixedRef = oldRef
continue
}
owner, repo := splitNWO(e.NWO)
|
I think this is related: #121 |
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
Partial scans can still rewrite unrecorded shared-action refs, and implicit SHA-edge merging can retain stale dependencies.
Review tier: Balanced
Findings: None
Issues resolved since last review (3)
| Severity | Finding |
|---|---|
|
internal/pin/plan.go — The lookup is case-insensitive for owner/repository, but the emitted rewrite key uses the… View resolved comment |
|
internal/pin/plan.go — A lock entry whose metadata ref is itself the bare SHA is loaded with that value in OnBranch… View resolved comment |
|
internal/lockfile/state.go — Rekeying can collide with an existing symbolic key at a different commit—for example, a SHA entry… View resolved comment |
Suppressed comments (2)
Previously missed (1) — in code that hasn't changed since the last review.
internal/lockfile/state.go:421
- Falling back to
d.SHAmakes this merge run for every symbolic dependency, even when no rekey occurred. If another workflow has anNWO@<sha>entry (for example, for a different sub-action path), itsusesedges get copied onto the symbolic entry; after that workflow is pruned,Savestill sees those children as reachable and cannot garbage-collect them. Only an explicitOriginalRefproves that this dependency was rekeyed from the SHA entry.
oldRef := d.OriginalRef
if oldRef == "" {
oldRef = d.SHA
}
if !isSHARef(d.Ref) && isSHARef(oldRef) {
internal/pin/plan.go:140
- This guard only examines refs already present in
wr.Inventory. A new/unrecorded ref inside a$/action bypasses it, but the slow path later runsnarrowDirectDeps/ReverseLookupandrewriteSelfActionFilescan still mutate that shared action during an explicit-workflow scan. Any unscanned workflow that also uses the action then retains a lock entry for the old ref. Please apply the partial-scan check to planned rewrites targetingSelfActionRefs(including newly resolved refs), not only metadata repairs from inventory.
for _, inv := range wr.Inventory {
key := strings.ToLower(inv.Dep.NWO) + "@" + inv.Dep.Ref
if parserlock.IsFullSha(inv.Dep.Ref) && (inv.Dep.Tag != "" || inv.Dep.Branch != "") && selfActionRefKeys[key] {
return planResult{}, fmt.Errorf("cannot rewrite %s in a shared local action during a partial workflow scan; scan all workflows", key)
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Terminal workflows containing only local actions can still have their existing lock entries replaced with an empty list.
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 — This preservation still fails when the blocked workflow contains only ./… actions. ParseAll… |
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Conflicting same-run repairs can overwrite pins, and workflow load failures can still erase existing lock entries.
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/pin/plan.go — This only checks symbolic targets already present in the store. If two SHA-keyed entries in the… |
|
internal/pipeline/parse.go — The new terminal-inventory path still misses workflow load failures: the earlier Load error… |
Pre-existing issues (1)
| Severity | Finding |
|---|---|
|
internal/pipeline/diagnose.go — This preservation still fails when the blocked workflow contains only ./… actions. ParseAll… View comment |
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
The conflict check can miss a repaired ref colliding with a live symbolic entry at a different SHA.
Review tier: Balanced
Findings: None
Issues resolved since last review (3)
| Severity | Finding |
|---|---|
|
internal/pipeline/parse.go — The new terminal-inventory path still misses workflow load failures: the earlier Load error… View resolved comment |
|
internal/pin/plan.go — This only checks symbolic targets already present in the store. If two SHA-keyed entries in the… View resolved comment |
|
internal/pipeline/diagnose.go — This preservation still fails when the blocked workflow contains only ./… actions. ParseAll… View resolved comment |
Suppressed comments (1)
internal/pin/plan.go:112
- This only compares entries that were themselves auto-fixed. A same-run live entry already using the symbolic ref (for example,
action@v3.2.0resolving to SHA B) has an emptyAutoFixedRef, so it is not compared with a metadata repair that maps a SHA key tov3.2.0at SHA A. Both entries are then committed under the same lockfile key, and whichever workflow is processed last overwrites the shared commit metadata. Include every committedPinned/Verifiedentry with a SHA in this consistency check, not only repaired entries.
if planErr != nil || entry.AutoFixedRef == "" {
continue
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Malformed workflows still enter the commit phase and can prevent other workflow fixes while leaving rewritten sources without matching lock updates.
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 — Preserving the inventory here does not keep a malformed workflow out of the write phase. With… |
Suppressed comments (1)
internal/pin/plan.go:691
- The new valid-branch fallback is not exercised: the added branch case only verifies that a SHA-valued branch is rejected, while all successful metadata-repair tests use
Tag. Add a table case withBranch: "main"that asserts both the entry rekey and source rewrite, since branch repair is explicitly supported by this path.
newRef := e.Tag
if newRef == "" {
newRef = e.OnBranch
}
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The repair and preservation paths include appropriate conflict safeguards and focused regression coverage.
Review tier: Balanced
Findings: None
Issues resolved since last review (1)
| Severity | Finding |
|---|---|
|
internal/pipeline/diagnose.go — Preserving the inventory here does not keep a malformed workflow out of the write phase. With… View resolved comment |
What
Repair bare-SHA references in
$/action files when an existing lockfile already records the matching symbolic tag or branch, and preserve existing pins when a workflow hits a terminal structural error.Fixes #114.
Fixes #121.
Why
Fresh onboarding with
v0.1.7-rc.1rewrites the reported SHA tov3.2.0, but lockfiles generated byv0.1.6can retain the SHA as the dependency key while storingv3.2.0in its metadata. The verified-entry path discarded that metadata, so upgrading left the action source at the SHA and regressed the lockfile ref back to it.Separately,
--no-migrate-local-actionsreturned from diagnosis before carrying existing lock inventory into planning. Fix mode then replaced the affected workflow's pins with an empty list.How
Testing
$/action and av0.1.6-shaped lockfile.--rescan --no-migrate-local-actionswith an existing pinned remote action.go test ./...