Skip to content

Commit 968a799

Browse files
nodeselectorCopilot
andcommitted
refactor(doctor): extract session state, comparison, and picker helpers
- Extract sessionState type from Remediator into session.go (choices, internalRefChoices, approvedRefs maps + their methods) - Extract compareSnapshots() from diagnoseOneWorkflow into compare.go with matchLiveDep() and unmatchedFindings() helpers - Extract tag picker rendering (tagLabel, defaultBranchOption) and selection handling (runPicker) into picker.go - Both handleSHAWithSuggestions and handleSHATagPicker now use shared picker infrastructure while keeping their distinct control flow Net reduction of ~210 lines from existing files. No behavior change. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent ab83427 commit 968a799

6 files changed

Lines changed: 398 additions & 305 deletions

File tree

internal/doctor/apply.go

Lines changed: 0 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -7,47 +7,6 @@ import (
77
"github.com/github/gh-actions-pin/internal/lockfile"
88
)
99

10-
// choiceKey returns a stable key for session memory: "owner/repo@SHA".
11-
func (rem *Remediator) choiceKey(dep *lockfile.Dependency) string {
12-
return dep.NWO + "@" + dep.SHA
13-
}
14-
15-
// recordChoice saves a tag choice for a dep so it can be auto-applied later.
16-
func (rem *Remediator) recordChoice(dep *lockfile.Dependency, tag string) {
17-
rem.choices[rem.choiceKey(dep)] = tag
18-
}
19-
20-
// recallChoice returns (tag, true) if we already made a choice for this dep.
21-
func (rem *Remediator) recallChoice(dep *lockfile.Dependency) (string, bool) {
22-
tag, ok := rem.choices[rem.choiceKey(dep)]
23-
return tag, ok
24-
}
25-
26-
// refKey returns a session memory key for an unpinned action ref: "owner/repo@ref".
27-
func refKey(ref lockfile.ActionRef) string {
28-
return ref.FullName() + "@" + ref.Ref
29-
}
30-
31-
// markRefsApproved records all action refs as approved for auto-pinning.
32-
func (rem *Remediator) markRefsApproved(refs []lockfile.ActionRef) {
33-
for _, ref := range refs {
34-
rem.approvedRefs[refKey(ref)] = true
35-
}
36-
}
37-
38-
// allRefsApproved returns true if every ref was already approved in a prior workflow.
39-
func (rem *Remediator) allRefsApproved(refs []lockfile.ActionRef) bool {
40-
if len(refs) == 0 {
41-
return false
42-
}
43-
for _, ref := range refs {
44-
if !rem.approvedRefs[refKey(ref)] {
45-
return false
46-
}
47-
}
48-
return true
49-
}
50-
5110
// isSHARef returns true if ref looks like a full commit SHA (40 or 64 hex chars).
5211
func isSHARef(ref string) bool {
5312
return lockfile.IsFullSHA(ref)

internal/doctor/compare.go

Lines changed: 104 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,104 @@
1+
package doctor
2+
3+
import (
4+
"fmt"
5+
"strings"
6+
7+
"github.com/github/gh-actions-pin/internal/lockfile"
8+
)
9+
10+
// compareSnapshots compares pinned dependencies against a live resolution and
11+
// returns findings for any discrepancies: ref moves, ref changes, and stale deps.
12+
func compareSnapshots(path string, existing, live []lockfile.Dependency, directNWOs map[string]bool) []Finding {
13+
liveByKey := make(map[string]lockfile.Dependency, len(live))
14+
liveByNWO := make(map[string][]lockfile.Dependency, len(live))
15+
for _, dep := range live {
16+
liveByKey[dep.Key()] = dep
17+
liveByNWO[dep.NWO] = append(liveByNWO[dep.NWO], dep)
18+
}
19+
20+
var findings []Finding
21+
for _, pinned := range existing {
22+
if lockfile.IsFullSHA(pinned.Ref) {
23+
continue
24+
}
25+
26+
resolved, ok := matchLiveDep(pinned, liveByKey, liveByNWO)
27+
if !ok {
28+
findings = append(findings, unmatchedFindings(path, pinned, liveByNWO, directNWOs)...)
29+
continue
30+
}
31+
if !strings.EqualFold(pinned.SHA, resolved.SHA) {
32+
resolvedCopy := resolved
33+
findings = append(findings, Finding{
34+
WorkflowPath: path,
35+
Category: CategoryRefMoved,
36+
Severity: SeverityError,
37+
Dependency: &pinned,
38+
Detail: fmt.Sprintf("pinned %s but ref now resolves to %s", pinned.SHA[:12], resolvedCopy.SHA[:12]),
39+
Remediation: fmt.Sprintf("update to %s with `gh actions-pin upgrade`", resolvedCopy.SHA[:12]),
40+
})
41+
}
42+
}
43+
return findings
44+
}
45+
46+
// matchLiveDep tries to find a live dependency matching the pinned one.
47+
// First by exact key, then fuzzy by NWO (same SHA or narrowed version).
48+
func matchLiveDep(pinned lockfile.Dependency, byKey map[string]lockfile.Dependency, byNWO map[string][]lockfile.Dependency) (lockfile.Dependency, bool) {
49+
if dep, ok := byKey[pinned.Key()]; ok {
50+
return dep, true
51+
}
52+
if candidates, has := byNWO[pinned.NWO]; has {
53+
for _, cand := range candidates {
54+
if strings.EqualFold(cand.SHA, pinned.SHA) {
55+
return cand, true
56+
}
57+
if IsNarrowedVersion(cand.Ref, pinned.Ref) {
58+
return cand, true
59+
}
60+
}
61+
}
62+
return lockfile.Dependency{}, false
63+
}
64+
65+
// unmatchedFindings produces findings for a pinned dep that has no live match:
66+
// either a ref change (direct dep with different ref) or stale (orphaned).
67+
func unmatchedFindings(path string, pinned lockfile.Dependency, liveByNWO map[string][]lockfile.Dependency, directNWOs map[string]bool) []Finding {
68+
if directNWOs[pinned.NWO] {
69+
if candidates, has := liveByNWO[pinned.NWO]; has && len(candidates) > 0 {
70+
newDep := candidates[0]
71+
if newDep.Ref != pinned.Ref {
72+
refOwner, refRepo := pinned.OwnerRepo()
73+
return []Finding{{
74+
WorkflowPath: path,
75+
Category: CategoryRefChanged,
76+
Severity: SeverityWarning,
77+
Dependency: &pinned,
78+
ActionRef: &lockfile.ActionRef{
79+
Owner: refOwner,
80+
Repo: refRepo,
81+
Ref: newDep.Ref,
82+
},
83+
Detail: fmt.Sprintf("ref changed from %s to %s in workflow — re-pin to update", pinned.Ref, newDep.Ref),
84+
Remediation: "re-pin to match the new ref",
85+
}}
86+
}
87+
}
88+
}
89+
90+
detail := "no longer in workflow — will be cleaned up"
91+
remediation := "re-resolve to remove orphaned dependency"
92+
if !directNWOs[pinned.NWO] {
93+
detail = "transitive dependency no longer discovered from upstream composite action"
94+
remediation = "re-resolve to clean up"
95+
}
96+
return []Finding{{
97+
WorkflowPath: path,
98+
Category: CategoryStale,
99+
Severity: SeverityInfo,
100+
Dependency: &pinned,
101+
Detail: detail,
102+
Remediation: remediation,
103+
}}
104+
}

internal/doctor/diagnose.go

Lines changed: 6 additions & 95 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,6 @@ package doctor
22

33
import (
44
"fmt"
5-
"strings"
65

76
"github.com/github/gh-actions-pin/internal/lockfile"
87
"github.com/github/gh-actions-pin/internal/resolver"
@@ -166,100 +165,8 @@ func diagnoseOneWorkflow(path string, r *resolver.Resolver) WorkflowReport {
166165
})
167166
}
168167

169-
depsByKey := make(map[string]lockfile.Dependency)
170-
for _, dep := range existingDeps {
171-
depsByKey[dep.Key()] = dep
172-
}
173-
liveByKey := make(map[string]lockfile.Dependency)
174-
// Multi-value NWO index for transitive fuzzy matching.
175-
liveByNWO := make(map[string][]lockfile.Dependency)
176-
for _, dep := range liveDeps {
177-
liveByKey[dep.Key()] = dep
178-
liveByNWO[dep.NWO] = append(liveByNWO[dep.NWO], dep)
179-
}
180-
181-
// Check each existing dep against live resolution.
182-
for _, existing := range existingDeps {
183-
if lockfile.IsFullSHA(existing.Ref) {
184-
continue
185-
}
186-
187-
live, ok := liveByKey[existing.Key()]
188-
if !ok {
189-
// Fuzzy match by NWO: same repo, same SHA or narrowed version.
190-
if candidates, has := liveByNWO[existing.NWO]; has {
191-
for _, cand := range candidates {
192-
if strings.EqualFold(cand.SHA, existing.SHA) {
193-
live = cand
194-
ok = true
195-
break
196-
}
197-
if IsNarrowedVersion(cand.Ref, existing.Ref) {
198-
live = cand
199-
ok = true
200-
break
201-
}
202-
}
203-
}
204-
}
205-
if !ok {
206-
// Direct ref change?
207-
if directNWOs[existing.NWO] {
208-
if candidates, has := liveByNWO[existing.NWO]; has && len(candidates) > 0 {
209-
newDep := candidates[0]
210-
if newDep.Ref != existing.Ref {
211-
refOwner, refRepo := existing.OwnerRepo()
212-
wr.Findings = append(wr.Findings, Finding{
213-
WorkflowPath: path,
214-
Category: CategoryRefChanged,
215-
Severity: SeverityWarning,
216-
Dependency: &existing,
217-
ActionRef: &lockfile.ActionRef{
218-
Owner: refOwner,
219-
Repo: refRepo,
220-
Ref: newDep.Ref,
221-
},
222-
Detail: fmt.Sprintf("ref changed from %s to %s in workflow — re-pin to update", existing.Ref, newDep.Ref),
223-
Remediation: "re-pin to match the new ref",
224-
})
225-
continue
226-
}
227-
}
228-
}
229-
// Transitive dep no longer discovered, or stale direct dep.
230-
if !directNWOs[existing.NWO] {
231-
wr.Findings = append(wr.Findings, Finding{
232-
WorkflowPath: path,
233-
Category: CategoryStale,
234-
Severity: SeverityInfo,
235-
Dependency: &existing,
236-
Detail: "transitive dependency no longer discovered from upstream composite action",
237-
Remediation: "re-resolve to clean up",
238-
})
239-
} else {
240-
wr.Findings = append(wr.Findings, Finding{
241-
WorkflowPath: path,
242-
Category: CategoryStale,
243-
Severity: SeverityInfo,
244-
Dependency: &existing,
245-
Detail: "no longer in workflow — will be cleaned up",
246-
Remediation: "re-resolve to remove orphaned dependency",
247-
})
248-
}
249-
continue
250-
}
251-
if !strings.EqualFold(existing.SHA, live.SHA) {
252-
liveCopy := live
253-
wr.Findings = append(wr.Findings, Finding{
254-
WorkflowPath: path,
255-
Category: CategoryRefMoved,
256-
Severity: SeverityError,
257-
Dependency: &existing,
258-
Detail: fmt.Sprintf("pinned %s but ref now resolves to %s", existing.SHA[:12], live.SHA[:12]),
259-
Remediation: fmt.Sprintf("update to %s with `gh actions-pin upgrade`", liveCopy.SHA[:12]),
260-
})
261-
}
262-
}
168+
// Compare pinned deps against live resolution.
169+
wr.Findings = append(wr.Findings, compareSnapshots(path, existingDeps, liveDeps, directNWOs)...)
263170

264171
// Build set of NWOs with ref-changed findings to avoid duplicate "not pinned" findings.
265172
refChangedNWOs := make(map[string]bool)
@@ -270,6 +177,10 @@ func diagnoseOneWorkflow(path string, r *resolver.Resolver) WorkflowReport {
270177
}
271178

272179
// Check for missing deps (action in workflow but not pinned).
180+
depsByKey := make(map[string]lockfile.Dependency, len(existingDeps))
181+
for _, dep := range existingDeps {
182+
depsByKey[dep.Key()] = dep
183+
}
273184
for _, ref := range refs {
274185
key := ref.FullName() + "@" + ref.Ref
275186
if _, ok := depsByKey[key]; !ok {

0 commit comments

Comments
 (0)