From c4089c532557c506dff353baaf3ef1f03e7567c2 Mon Sep 17 00:00:00 2001 From: Kaito Udagawa Date: Thu, 10 Sep 2026 03:20:49 +0900 Subject: [PATCH 1/5] Record dependency-free workflows Dependency resolution failures and workflows without dependencies can both produce empty dependency slices. Preserve resolver failure state through planning and pass the existing no-onboard policy to Commit so it can distinguish these cases before writing new lockfile entries. This allows dependency-free workflows to receive empty lockfile entries without adding entries for unresolved or skipped workflows. --- cmd/gh-actions-lock/run.go | 2 +- internal/pin/commit.go | 6 +++++- internal/pin/plan.go | 7 ++++--- internal/pin/record.go | 2 ++ internal/pipeline/checks/finding.go | 2 ++ internal/pipeline/diagnose.go | 1 + 6 files changed, 15 insertions(+), 5 deletions(-) diff --git a/cmd/gh-actions-lock/run.go b/cmd/gh-actions-lock/run.go index 547d5882..6fd56260 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 bd3e900e..a49092b8 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 @@ -67,7 +69,9 @@ 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) { + allResolutionsFailed := wp.ResolveErr != nil && len(deps) == 0 + skipNewWorkflowEntries := copts != nil && copts.SkipNewWorkflowEntries + if !store.HasWorkflow(wfKey) && (allResolutionsFailed || skipNewWorkflowEntries) { continue } parentMap := buildParentMap(rec, wfPath) diff --git a/internal/pin/plan.go b/internal/pin/plan.go index f9c24603..8dee6103 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/record.go b/internal/pin/record.go index ad32246a..2669039c 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 0d82da92..9c884252 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 ac715eba..e1eab490 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 From 7dd8220720e0928d68762075d462e3de9e7acfaf Mon Sep 17 00:00:00 2001 From: Kaito Udagawa Date: Thu, 10 Sep 2026 03:21:05 +0900 Subject: [PATCH 2/5] Fix plan test compilation Remove a duplicate lockfile import and update the stale newSlowPathFixtures call to match its current signature. --- internal/pin/plan_test.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/internal/pin/plan_test.go b/internal/pin/plan_test.go index 3e6d6e6b..80624d40 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" @@ -798,7 +797,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{ From ca7153156e844b3c779ace3eb2fcfbb5b81f41cd Mon Sep 17 00:00:00 2001 From: Kaito Udagawa Date: Thu, 10 Sep 2026 03:21:11 +0900 Subject: [PATCH 3/5] Add dependency-free workflow tests Verify that ResolveAllRecursive errors remain available in workflow plans and that successful partial results are committed. Cover recording a dependency-free workflow while skipping unresolved workflows and new workflow entries disabled by the caller. --- internal/pin/commit_test.go | 52 +++++++++++++++++++++++++++++++++++++ internal/pin/plan_test.go | 4 +++ 2 files changed, 56 insertions(+) diff --git a/internal/pin/commit_test.go b/internal/pin/commit_test.go index fda34b82..7a0adcad 100644 --- a/internal/pin/commit_test.go +++ b/internal/pin/commit_test.go @@ -109,6 +109,58 @@ 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 TestCommitRemovesDependenciesDroppedFromWorkflow(t *testing.T) { dir := t.TempDir() workflowPath := filepath.Join(".github", "workflows", "ci.yml") diff --git a/internal/pin/plan_test.go b/internal/pin/plan_test.go index 80624d40..a4b80a0f 100644 --- a/internal/pin/plan_test.go +++ b/internal/pin/plan_test.go @@ -195,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 @@ -248,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) { From a1e7fefd609fb679925e15f040c817ea4cabed49 Mon Sep 17 00:00:00 2001 From: Kaito Udagawa Date: Thu, 10 Sep 2026 04:38:22 +0900 Subject: [PATCH 4/5] Skip rewrites for new workflows Filter workflows with no existing lockfile entry before the Commit phase rewrites workflow or self-action files when new entries are disabled. Add a regression test that verifies both source files and the lockfile remain unchanged. --- internal/pin/commit.go | 13 ++++++++++--- internal/pin/commit_test.go | 32 ++++++++++++++++++++++++++++++++ 2 files changed, 42 insertions(+), 3 deletions(-) diff --git a/internal/pin/commit.go b/internal/pin/commit.go index a49092b8..53fe79ee 100644 --- a/internal/pin/commit.go +++ b/internal/pin/commit.go @@ -31,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 { @@ -69,9 +78,7 @@ func Commit(ctx context.Context, rec *Record, store *lockfile.State, copts *Comm wfPath := wp.Path wfKey := workflowfile.KeyFromPath(wfPath) deps := pinnedByWorkflow[wfPath] - allResolutionsFailed := wp.ResolveErr != nil && len(deps) == 0 - skipNewWorkflowEntries := copts != nil && copts.SkipNewWorkflowEntries - if !store.HasWorkflow(wfKey) && (allResolutionsFailed || skipNewWorkflowEntries) { + 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 7a0adcad..9630b39d 100644 --- a/internal/pin/commit_test.go +++ b/internal/pin/commit_test.go @@ -161,6 +161,38 @@ func TestCommitPartialResolution(t *testing.T) { 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") From 150c9a4a33f70a746aa5b6b49fc4479ba0b4129e Mon Sep 17 00:00:00 2001 From: Kaito Udagawa Date: Thu, 10 Sep 2026 04:51:47 +0900 Subject: [PATCH 5/5] Test blocking resolver error exclusion Verify that planning excludes workflow reports marked with a blocking resolver error. --- internal/pin/plan_test.go | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/internal/pin/plan_test.go b/internal/pin/plan_test.go index a4b80a0f..60a812d7 100644 --- a/internal/pin/plan_test.go +++ b/internal/pin/plan_test.go @@ -618,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"