diff --git a/cmd/gh-actions-lock/run.go b/cmd/gh-actions-lock/run.go index 547d588..6fd5626 100644 --- a/cmd/gh-actions-lock/run.go +++ b/cmd/gh-actions-lock/run.go @@ -399,7 +399,7 @@ func runCheck(cmd *cobra.Command, opts *checkOptions, newResolver resolverFunc) // Commit: write all changes to disk atomically (fast local I/O, no // spinner label — it finishes before the user could read one). endCommit := prof.Phase("pin.Commit (disk writes)") - if err := pin.Commit(ctx, record, store, nil); err != nil { + if err := pin.Commit(ctx, record, store, &pin.CommitOptions{SkipNewWorkflowEntries: noOnboardFlag(cmd)}); err != nil { console.StopProgress() return fmt.Errorf("committing pins: %w", err) } diff --git a/internal/pin/commit.go b/internal/pin/commit.go index bd3e900..53fe79e 100644 --- a/internal/pin/commit.go +++ b/internal/pin/commit.go @@ -18,6 +18,8 @@ import ( type CommitOptions struct { // OnProgress is called at each phase boundary. Nil means no progress. OnProgress func(phase string) + // SkipNewWorkflowEntries forces the Commit phase to skip workflows with no existing lockfile entry. + SkipNewWorkflowEntries bool } // Commit writes a planned Record to disk: rewrites workflow files and @@ -29,6 +31,15 @@ func Commit(ctx context.Context, rec *Record, store *lockfile.State, copts *Comm if copts != nil && copts.OnProgress != nil { progress = copts.OnProgress } + if copts != nil && copts.SkipNewWorkflowEntries { + workflows := rec.Workflows[:0] + for _, wp := range rec.Workflows { + if store.HasWorkflow(workflowfile.KeyFromPath(wp.Path)) { + workflows = append(workflows, wp) + } + } + rec.Workflows = workflows + } // Phase 1: Rewrite workflow files (uses: line changes). if len(rec.Workflows) > 0 { @@ -67,7 +78,7 @@ func Commit(ctx context.Context, rec *Record, store *lockfile.State, copts *Comm wfPath := wp.Path wfKey := workflowfile.KeyFromPath(wfPath) deps := pinnedByWorkflow[wfPath] - if len(deps) == 0 && !store.HasWorkflow(wfKey) { + if len(deps) == 0 && !store.HasWorkflow(wfKey) && wp.ResolveErr != nil { continue } parentMap := buildParentMap(rec, wfPath) diff --git a/internal/pin/commit_test.go b/internal/pin/commit_test.go index fda34b8..9630b39 100644 --- a/internal/pin/commit_test.go +++ b/internal/pin/commit_test.go @@ -109,6 +109,90 @@ func TestBuildDirectKeys(t *testing.T) { assert.NotContains(t, keys, "g/h@v4", "investigate should be excluded") } +func TestCommitDependencyFreeWorkflow(t *testing.T) { + tests := []struct { + name string + resolveErr error + skipNewWorkflowEntries bool + wantEntry bool + }{ + {"records empty entry", nil, false, true}, + {"skips unresolved workflow", assert.AnError, false, false}, + {"skips onboarding", nil, true, false}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + dir := t.TempDir() + workflowPath := filepath.Join(".github", "workflows", "ci.yml") + require.NoError(t, os.MkdirAll(filepath.Join(dir, filepath.Dir(workflowPath)), 0o755)) + require.NoError(t, os.WriteFile(filepath.Join(dir, workflowPath), []byte("on: push\n"), 0o644)) + t.Chdir(dir) + + store, err := lockfile.LoadState(dir, fakeMeta{}) + require.NoError(t, err) + rec := &Record{Workflows: []WorkflowPlan{{Path: workflowPath, ResolveErr: tt.resolveErr}}} + + require.NoError(t, Commit(context.Background(), rec, store, &CommitOptions{SkipNewWorkflowEntries: tt.skipNewWorkflowEntries})) + assert.Equal(t, tt.wantEntry, store.HasWorkflow(workflowPath)) + }) + } +} + +func TestCommitPartialResolution(t *testing.T) { + dir := t.TempDir() + workflowPath := filepath.Join(".github", "workflows", "ci.yml") + require.NoError(t, os.MkdirAll(filepath.Join(dir, filepath.Dir(workflowPath)), 0o755)) + require.NoError(t, os.WriteFile(filepath.Join(dir, workflowPath), []byte("on: push\n"), 0o644)) + t.Chdir(dir) + + store, err := lockfile.LoadState(dir, fakeMeta{}) + require.NoError(t, err) + rec := &Record{ + Entries: []Entry{{ + NWO: "actions/checkout", Ref: "v4", SHA: strings.Repeat("a", 40), + Resolution: Pinned, Workflows: []string{workflowPath}, Direct: true, + }}, + Workflows: []WorkflowPlan{{Path: workflowPath, ResolveErr: assert.AnError}}, + } + + require.NoError(t, Commit(context.Background(), rec, store, nil)) + deps, err := store.Get(workflowPath) + require.NoError(t, err) + assert.Len(t, deps, 1) +} + +func TestCommitSkipNewWorkflowEntriesPreventsRewrites(t *testing.T) { + dir := t.TempDir() + workflowPath := filepath.Join(".github", "workflows", "ci.yml") + actionPath := filepath.Join(".github", "actions", "local", "action.yml") + oldUses := "actions/checkout@" + strings.Repeat("a", 40) + newUses := "actions/checkout@v4.2.0" + workflowContent := []byte("on: push\njobs:\n test:\n runs-on: ubuntu-latest\n steps:\n - uses: " + oldUses + "\n") + actionContent := []byte("name: local\nruns:\n using: composite\n steps:\n - uses: " + oldUses + "\n") + for path, content := range map[string][]byte{workflowPath: workflowContent, actionPath: actionContent} { + require.NoError(t, os.MkdirAll(filepath.Join(dir, filepath.Dir(path)), 0o755)) + require.NoError(t, os.WriteFile(filepath.Join(dir, path), content, 0o644)) + } + t.Chdir(dir) + + store, err := lockfile.LoadState(dir, fakeMeta{}) + require.NoError(t, err) + rec := &Record{Workflows: []WorkflowPlan{{ + Path: workflowPath, + Rewrites: map[string]string{oldUses: newUses}, + SelfActionFiles: []string{actionPath}, + }}} + + require.NoError(t, Commit(context.Background(), rec, store, &CommitOptions{SkipNewWorkflowEntries: true})) + workflowAfter, err := os.ReadFile(workflowPath) + require.NoError(t, err) + actionAfter, err := os.ReadFile(actionPath) + require.NoError(t, err) + assert.Equal(t, workflowContent, workflowAfter) + assert.Equal(t, actionContent, actionAfter) + assert.False(t, store.HasWorkflow(workflowPath)) +} + func TestCommitRemovesDependenciesDroppedFromWorkflow(t *testing.T) { dir := t.TempDir() workflowPath := filepath.Join(".github", "workflows", "ci.yml") diff --git a/internal/pin/plan.go b/internal/pin/plan.go index f9c2460..8dee610 100644 --- a/internal/pin/plan.go +++ b/internal/pin/plan.go @@ -134,7 +134,7 @@ type planResult struct { func planWorkflow(ctx context.Context, wr checks.WorkflowReport, opts PlanOptions, status func(string)) (planResult, error) { var entries []Entry var wplans []WorkflowPlan - if wr.SkipCommit { + if wr.SkipCommit || wr.BlockingResolverError { return planResult{entries: verifiedEntries(wr.Inventory, wr.Path)}, nil } for _, finding := range wr.Findings { @@ -204,7 +204,7 @@ func planWorkflow(ctx context.Context, wr checks.WorkflowReport, opts PlanOption if resolveErr != nil { entries = append(entries, unresolvedEntries(wr, unrecordedRefs, deps, resolveErr)...) if len(deps) == 0 { - wplans = append(wplans, WorkflowPlan{Path: wr.Path, SelfActionFiles: wr.SelfActionFiles}) + wplans = append(wplans, WorkflowPlan{Path: wr.Path, SelfActionFiles: wr.SelfActionFiles, ResolveErr: resolveErr}) return planResult{entries: entries, wplans: wplans}, nil } // Fall through with partial deps to pin what we can. @@ -295,10 +295,11 @@ func planWorkflow(ctx context.Context, wr checks.WorkflowReport, opts PlanOption Path: wr.Path, Rewrites: rewrites, SelfActionFiles: wr.SelfActionFiles, + ResolveErr: resolveErr, }) } else if len(wplans) == 0 { // Keep the workflow in the plan so its lockfile entry is updated. - wplans = append(wplans, WorkflowPlan{Path: wr.Path, SelfActionFiles: wr.SelfActionFiles}) + wplans = append(wplans, WorkflowPlan{Path: wr.Path, SelfActionFiles: wr.SelfActionFiles, ResolveErr: resolveErr}) } // Build entries for all pinned deps (skip any already emitted from inventory). diff --git a/internal/pin/plan_test.go b/internal/pin/plan_test.go index 3e6d6e6..60a812d 100644 --- a/internal/pin/plan_test.go +++ b/internal/pin/plan_test.go @@ -10,7 +10,6 @@ import ( parserlock "github.com/github/actions-lockfile/go/pkg/lockfile" "github.com/github/gh-actions-lock/internal/ghapi/httpmock" - "github.com/github/gh-actions-lock/internal/lockfile" "github.com/github/gh-actions-lock/internal/pinpool" "github.com/github/gh-actions-lock/internal/resolve" "github.com/github/gh-actions-lock/internal/tag" @@ -196,6 +195,8 @@ func TestPlanWorkflow_PartialResolutionFailure(t *testing.T) { require.Len(t, pinned, 1, "expected exactly one pinned entry") assert.Equal(t, "good/action", pinned[0].NWO) assert.Equal(t, goodSHA, pinned[0].SHA) + require.Len(t, result.wplans, 1) + assert.Error(t, result.wplans[0].ResolveErr) } // TestPlanWorkflow_AllResolutionsFail verifies that when ALL refs in a @@ -249,6 +250,8 @@ func TestPlanWorkflow_AllResolutionsFail(t *testing.T) { assert.Equal(t, Unresolved, e.Resolution, "expected %s to be Unresolved", e.NWO) assert.Contains(t, e.Reason, "not found") } + require.Len(t, result.wplans, 1) + assert.Error(t, result.wplans[0].ResolveErr) } func newTransitivePlanFixture(t *testing.T, compSHA, transSHA string) (*resolve.Resolver, *pinpool.Pool, *tag.Lister) { @@ -615,6 +618,18 @@ func TestPlanExcludesLoadFailuresFromCommit(t *testing.T) { assert.Equal(t, blocked.Path, record.Entries[0].Workflows[0]) } +func TestPlanExcludesBlockingResolverErrorsFromCommit(t *testing.T) { + record, err := Plan(context.Background(), &checks.Report{ + Workflows: []checks.WorkflowReport{{ + Path: ".github/workflows/ci.yml", + BlockingResolverError: true, + }}, + }, PlanOptions{Pool: pinpool.New(2, nil)}) + require.NoError(t, err) + + assert.Empty(t, record.Workflows) +} + func TestPlanWorkflow_SelfRepositoryDependencyIsNotRewrittenOnFastPath(t *testing.T) { const sha = "abc1230000000000000000000000000000000000" @@ -798,7 +813,7 @@ func TestNoNarrow_BareSHA(t *testing.T) { }) t.Run("partial scan rejects unrecorded shared action rewrite", func(t *testing.T) { - resolver, tagger, wr, _ := newSlowPathFixtures(t, false) + resolver, tagger, wr, _ := newSlowPathFixtures(t) wr.SelfActionRefs = append([]parserlock.ActionRef(nil), wr.ActionRefs...) _, err := planWorkflow(context.Background(), wr, PlanOptions{ diff --git a/internal/pin/record.go b/internal/pin/record.go index ad32246..2669039 100644 --- a/internal/pin/record.go +++ b/internal/pin/record.go @@ -50,6 +50,8 @@ type Entry struct { type WorkflowPlan struct { Path string Rewrites map[string]string + // ResolveErr preserves the error returned by ResolveAllRecursive. + ResolveErr error // SelfActionFiles are in-repo action definition files reached from this // workflow through `$/…`. The same rewrites apply to their `uses:` lines. SelfActionFiles []string diff --git a/internal/pipeline/checks/finding.go b/internal/pipeline/checks/finding.go index 0d82da9..9c88425 100644 --- a/internal/pipeline/checks/finding.go +++ b/internal/pipeline/checks/finding.go @@ -65,6 +65,8 @@ type WorkflowReport struct { Findings []Finding // SkipCommit prevents terminal parse failures from entering the write phase. SkipCommit bool + // BlockingResolverError indicates that diagnosis classified a resolver error as blocking. + BlockingResolverError bool // ActionRefs are all remote dependency roots attributed to the workflow, // including refs found inside in-repo `$/…` actions. ActionRefs []parserlock.ActionRef diff --git a/internal/pipeline/diagnose.go b/internal/pipeline/diagnose.go index ac715eb..e1eab49 100644 --- a/internal/pipeline/diagnose.go +++ b/internal/pipeline/diagnose.go @@ -102,6 +102,7 @@ func diagnoseOneParsed(ctx context.Context, pw checks.ParsedWorkflow, r *resolve blockingResolverError = true } if blockingResolverError { + wr.BlockingResolverError = true return wr } // Low: we're surfacing the resolver failure itself, not a