Skip to content

Commit 7162677

Browse files
nodeselectorCopilot
andcommitted
fix(pin): retain on-disk impostor pins on co-located re-pin
Fix-mode dropped an impostor-flagged dep from a workflow's pin set whenever a co-located bump forced that workflow to be rewritten, shrinking the list toward [] and trivially passing runner enforcement. Re-add the existing on-disk pin (kept direct) so the blocking finding + exit 1 still stand but the pin is never silently lost. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent 829b6b5 commit 7162677

2 files changed

Lines changed: 130 additions & 0 deletions

File tree

internal/pin/commit.go

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,9 +5,11 @@ import (
55
"fmt"
66
"os"
77
"runtime"
8+
"strings"
89

910
"github.com/github/gh-actions-pin/internal/dep"
1011
"github.com/github/gh-actions-pin/internal/lockfile"
12+
"github.com/github/gh-actions-pin/internal/pipeline/checks"
1113
"github.com/github/gh-actions-pin/internal/workflowfile"
1214
"golang.org/x/sync/errgroup"
1315
)
@@ -64,6 +66,7 @@ func Commit(ctx context.Context, rec *Record, store *lockfile.State, copts *Comm
6466
wfKey := workflowfile.KeyFromPath(wfPath)
6567
parentMap := buildParentMap(rec, wfPath)
6668
directKeys := buildDirectKeys(rec, wfPath)
69+
deps = retainImpostorPins(rec, store, wfPath, deps, directKeys)
6770
if err := store.Set(ctx, wfKey, deps, parentMap, directKeys); err != nil {
6871
return fmt.Errorf("updating lockfile for %s: %w", wfPath, err)
6972
}
@@ -115,6 +118,42 @@ func groupPinnedByWorkflow(rec *Record) map[string][]dep.Dependency {
115118
return result
116119
}
117120

121+
// retainImpostorPins re-adds the workflow's existing on-disk pins for any
122+
// impostor-flagged dep so a co-located re-pin never silently drops them.
123+
func retainImpostorPins(rec *Record, store *lockfile.State, wfPath string, deps []dep.Dependency, directKeys map[string]bool) []dep.Dependency {
124+
impostor := make(map[string]bool)
125+
for _, e := range rec.Entries {
126+
if e.Resolution != Investigate || e.Issue != string(checks.ImpostorCommit) {
127+
continue
128+
}
129+
for _, wf := range e.Workflows {
130+
if wf == wfPath {
131+
impostor[strings.ToLower(e.NWO+"@"+e.Ref)] = true
132+
}
133+
}
134+
}
135+
if len(impostor) == 0 {
136+
return deps
137+
}
138+
existing, err := store.Get(workflowfile.KeyFromPath(wfPath))
139+
if err != nil {
140+
return deps
141+
}
142+
have := make(map[string]bool, len(deps))
143+
for _, d := range deps {
144+
have[strings.ToLower(d.NWO+"@"+d.Ref)] = true
145+
}
146+
for _, d := range existing {
147+
k := strings.ToLower(d.NWO + "@" + d.Ref)
148+
if impostor[k] && !have[k] {
149+
deps = append(deps, d)
150+
directKeys[d.Key()] = true
151+
have[k] = true
152+
}
153+
}
154+
return deps
155+
}
156+
118157
func buildParentMap(rec *Record, wfPath string) map[string][]string {
119158
pm := make(map[string][]string)
120159
for _, e := range rec.Entries {
Lines changed: 91 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,91 @@
1+
package pin
2+
3+
import (
4+
"context"
5+
"os"
6+
"path/filepath"
7+
"testing"
8+
9+
"github.com/github/gh-actions-pin/internal/dep"
10+
"github.com/github/gh-actions-pin/internal/lockfile"
11+
"github.com/github/gh-actions-pin/internal/pipeline/checks"
12+
"github.com/github/gh-actions-pin/internal/workflowfile"
13+
"github.com/stretchr/testify/assert"
14+
"github.com/stretchr/testify/require"
15+
)
16+
17+
type fakeMeta struct{}
18+
19+
func (fakeMeta) RepoIDs(_ context.Context, _, _ string) (int64, int64, error) {
20+
return 1, 2, nil
21+
}
22+
23+
// A co-located bump forces a workflow rewrite; the impostor pin already on
24+
// disk must be retained, not silently dropped toward an empty pin list.
25+
func TestRetainImpostorPins_keepsExistingPinOnColocatedRepin(t *testing.T) {
26+
dir := t.TempDir()
27+
require.NoError(t, os.MkdirAll(filepath.Join(dir, ".github", "workflows"), 0o755))
28+
wfPath := filepath.Join(dir, ".github", "workflows", "ci.yml")
29+
wfKey := workflowfile.KeyFromPath(wfPath)
30+
31+
store, err := lockfile.LoadState(dir, fakeMeta{})
32+
require.NoError(t, err)
33+
34+
seed := []dep.Dependency{
35+
{NWO: "bad/impostor", Ref: "v1", Tag: "v1", Branch: "main", SHA: "1111111111111111111111111111111111111111", HashAlgo: "sha1"},
36+
{NWO: "actions/checkout", Ref: "v4", Tag: "v4", Branch: "main", SHA: "2222222222222222222222222222222222222222", HashAlgo: "sha1"},
37+
}
38+
require.NoError(t, store.Set(context.Background(), wfKey, seed, nil, nil))
39+
require.NoError(t, store.Save())
40+
41+
// Re-pin: checkout bumps (Pinned), impostor flagged Investigate and dropped from deps.
42+
rec := &Record{
43+
Entries: []Entry{
44+
{NWO: "actions/checkout", Ref: "v5", SHA: "3333333333333333333333333333333333333333", Resolution: Pinned, Direct: true, OnBranch: "main", Workflows: []string{wfPath}},
45+
{NWO: "bad/impostor", Ref: "v1", SHA: "1111111111111111111111111111111111111111", Resolution: Investigate, Issue: string(checks.ImpostorCommit), Workflows: []string{wfPath}},
46+
},
47+
}
48+
deps := []dep.Dependency{
49+
{NWO: "actions/checkout", Ref: "v5", Branch: "main", SHA: "3333333333333333333333333333333333333333", HashAlgo: "sha1"},
50+
}
51+
directKeys := map[string]bool{"actions/checkout@v5": true}
52+
53+
got := retainImpostorPins(rec, store, wfPath, deps, directKeys)
54+
55+
require.Len(t, got, 2, "impostor pin must be re-added alongside the bumped pin")
56+
assert.True(t, directKeys["bad/impostor@v1"], "retained impostor pin must stay direct")
57+
58+
// Drive the write as Commit does; the impostor pin must survive on disk.
59+
require.NoError(t, store.Set(context.Background(), wfKey, got, buildParentMap(rec, wfPath), directKeys))
60+
require.NoError(t, store.Save())
61+
62+
after, err := store.Get(wfKey)
63+
require.NoError(t, err)
64+
names := map[string]bool{}
65+
for _, d := range after {
66+
names[d.NWO] = true
67+
}
68+
assert.True(t, names["bad/impostor"], "impostor pin must survive the re-pin write")
69+
assert.True(t, names["actions/checkout"], "bumped pin must be written")
70+
assert.Len(t, after, 2, "pin list must not shrink")
71+
}
72+
73+
// With no co-located new pin, the impostor's workflow is untouched and there
74+
// is nothing to retain; the helper is a no-op on the deps it is handed.
75+
func TestRetainImpostorPins_noopWithoutImpostorFinding(t *testing.T) {
76+
dir := t.TempDir()
77+
require.NoError(t, os.MkdirAll(filepath.Join(dir, ".github", "workflows"), 0o755))
78+
wfPath := filepath.Join(dir, ".github", "workflows", "ci.yml")
79+
80+
store, err := lockfile.LoadState(dir, fakeMeta{})
81+
require.NoError(t, err)
82+
83+
rec := &Record{Entries: []Entry{
84+
{NWO: "actions/checkout", Ref: "v5", Resolution: Pinned, Workflows: []string{wfPath}},
85+
}}
86+
deps := []dep.Dependency{{NWO: "actions/checkout", Ref: "v5"}}
87+
directKeys := map[string]bool{"actions/checkout@v5": true}
88+
89+
got := retainImpostorPins(rec, store, wfPath, deps, directKeys)
90+
assert.Len(t, got, 1)
91+
}

0 commit comments

Comments
 (0)