From afb7854629c0d92d6224efc2791aa79d5f81d637 Mon Sep 17 00:00:00 2001 From: Jeff Martin Date: Sun, 11 Oct 2026 17:06:24 -0700 Subject: [PATCH 1/4] checks: report unused lockfile entries as errors Stale findings were emitted as warnings but Finding.IsValid treated the category as blocking, so --verify and --verify-local failed with only warnings in the output. Emit stale as an error and make IsValid depend on severity alone, so valid=false iff an error finding exists. --- .../testdata/golden-json/expected.json | 8 +- cmd/gh-actions-lock/verify_test.go | 147 ++++++++++++++++++ internal/pipeline/checks/category.go | 4 +- internal/pipeline/checks/finding.go | 19 +-- internal/pipeline/checks/finding_test.go | 23 +++ internal/pipeline/checks/run_test.go | 5 + internal/pipeline/checks/structural.go | 6 +- internal/pipeline/verify_local_test.go | 52 +++++++ 8 files changed, 241 insertions(+), 23 deletions(-) create mode 100644 internal/pipeline/checks/finding_test.go diff --git a/cmd/gh-actions-lock/testdata/golden-json/expected.json b/cmd/gh-actions-lock/testdata/golden-json/expected.json index f6a8527b..a2adf13c 100644 --- a/cmd/gh-actions-lock/testdata/golden-json/expected.json +++ b/cmd/gh-actions-lock/testdata/golden-json/expected.json @@ -57,8 +57,8 @@ "dependency": "old/dead@v1", "detail": "lockfile pins old/dead@v1 but no uses: in this workflow references it", "doc_url": "https://docs.github.com/en/actions/security-for-github-actions/security-guides/security-hardening-for-github-actions#using-third-party-actions", - "remediation": "remove the entry or re-run `gh actions-lock`", - "severity": "warning", + "remediation": "run `gh actions-lock` to prune the unused entry", + "severity": "error", "workflow": ".github/workflows/ci.yml" } ], @@ -123,8 +123,8 @@ "dependency": "old/dead@v1", "detail": "lockfile pins old/dead@v1 but no uses: in this workflow references it", "doc_url": "https://docs.github.com/en/actions/security-for-github-actions/security-guides/security-hardening-for-github-actions#using-third-party-actions", - "remediation": "remove the entry or re-run `gh actions-lock`", - "severity": "warning", + "remediation": "run `gh actions-lock` to prune the unused entry", + "severity": "error", "workflow": ".github/workflows/ci.yml" } ], diff --git a/cmd/gh-actions-lock/verify_test.go b/cmd/gh-actions-lock/verify_test.go index 70b6e79b..533c7ebc 100644 --- a/cmd/gh-actions-lock/verify_test.go +++ b/cmd/gh-actions-lock/verify_test.go @@ -4,10 +4,13 @@ import ( "fmt" "os" "path/filepath" + "slices" "strings" "testing" + "github.com/github/gh-actions-lock/cmd/gh-actions-lock/format" "github.com/github/gh-actions-lock/internal/ghapi/httpmock" + "github.com/github/gh-actions-lock/internal/pipeline/checks" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -159,3 +162,147 @@ jobs: } } } + +// TestStaleEntry_SeverityMatchesVerdict pins the contract consumers rely on: +// `valid` is false, and read-only modes exit non-zero, exactly when a finding +// has error severity. An unused lockfile entry is an error in every mode; a +// fix run prunes it and succeeds. +func TestStaleEntry_SeverityMatchesVerdict(t *testing.T) { + const workflow = ` +name: ci +on: push +jobs: + test: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4.2.1 +` + for _, tc := range []struct { + name string + mode []string + wantErr bool + }{ + {name: "verify fails", mode: []string{"--verify"}, wantErr: true}, + {name: "verify-local fails", mode: []string{"--verify-local"}, wantErr: true}, + {name: "fix run prunes", wantErr: false}, + } { + t.Run(tc.name, func(t *testing.T) { + path := writeTempWorkflow(t, workflow, + "actions/checkout@v4.2.1=sha1-"+parityTestSHA, + "actions/setup-go@v5=sha1-"+parityTestSHA, + ) + reg := &httpmock.Registry{} + args := append(slices.Clone(tc.mode), "--no-interactive", path) + + p, err := runParity(t, reg, args...) + if tc.wantErr { + require.ErrorIs(t, err, errSilent) + } else { + require.NoError(t, err) + } + + var stale []format.Finding + hasError := false + for _, f := range p.Findings { + hasError = hasError || f.Severity == string(checks.SeverityError) + if f.Category == string(checks.Stale) { + stale = append(stale, f) + } + } + require.Len(t, stale, 1, "%+v", p.Findings) + assert.Equal(t, "actions/setup-go@v5", stale[0].Dependency) + assert.Equal(t, string(checks.SeverityError), stale[0].Severity) + assert.Equal(t, !hasError, p.Valid, "valid must agree with severities") + + pins := readTempLockfilePins(t) + if tc.wantErr { + assert.Contains(t, pins, "actions/setup-go@v5", "read-only modes must not prune") + } else { + assert.NotContains(t, pins, "actions/setup-go@v5", "fix run prunes the unused entry") + } + }) + } +} + +// TestStaleEntry_TerminalShowsRemediation checks a human sees the finding +// and how to clear it in both read-only modes. +func TestStaleEntry_TerminalShowsRemediation(t *testing.T) { + for _, mode := range []string{"--verify", "--verify-local"} { + t.Run(mode, func(t *testing.T) { + path := writeTempWorkflow(t, ` +name: ci +on: push +jobs: + test: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4.2.1 +`, + "actions/checkout@v4.2.1=sha1-"+parityTestSHA, + "actions/setup-go@v5=sha1-"+parityTestSHA, + ) + _, stderr, err := runCommandWithHTTP(t, &httpmock.Registry{}, mode, "--no-interactive", path) + require.ErrorIs(t, err, errSilent) + assert.Contains(t, stderr, "Unused lockfile entry actions/setup-go@v5") + assert.Contains(t, stderr, "without "+mode) + }) + } +} + +// TestVerifyLocal_LocalActionVerdictFollowsSeverity: a local-action warning +// on a workflow the lockfile doesn't cover yet is not a failure; once the +// workflow is onboarded the same step is an error. JSON omits the warning +// (skipFindingInJSON), so the terminal run proves it is still surfaced. +func TestVerifyLocal_LocalActionVerdictFollowsSeverity(t *testing.T) { + for _, tc := range []struct { + name string + body string + pins []string + other bool // lockfile exists but covers another workflow + wantErr bool + }{ + { + name: "not onboarded warns and passes", + body: " - uses: ./my-action\n", + other: true, + }, + { + name: "onboarded errors and fails", + body: " - uses: actions/checkout@v4.2.1\n - uses: ./my-action\n", + pins: []string{"actions/checkout@v4.2.1=sha1-" + parityTestSHA}, + wantErr: true, + }, + } { + t.Run(tc.name, func(t *testing.T) { + writeTempWorkflow(t, "name: ci\non: push\njobs:\n test:\n runs-on: ubuntu-latest\n steps:\n"+tc.body, tc.pins...) + if tc.other { + writeTempLockfile(t, ".", "other.yml", []string{"actions/checkout@v4.2.1=sha1-" + parityTestSHA}) + } + require.NoError(t, os.Mkdir(".git", 0o755)) + require.NoError(t, os.Mkdir("my-action", 0o755)) + require.NoError(t, os.WriteFile(filepath.Join("my-action", "action.yml"), []byte("runs:\n using: composite\n steps: []\n"), 0o644)) + checkErr := func(err error) { + if tc.wantErr { + require.ErrorIs(t, err, errSilent) + } else { + require.NoError(t, err) + } + } + + p, err := runParity(t, &httpmock.Registry{}, "--verify-local", "--no-interactive") + checkErr(err) + assert.Equal(t, !tc.wantErr, p.Valid) + if tc.wantErr { + require.Len(t, p.Findings, 1, "%+v", p.Findings) + assert.Equal(t, string(checks.LocalAction), p.Findings[0].Category) + assert.Equal(t, string(checks.SeverityError), p.Findings[0].Severity) + } else { + assert.Empty(t, p.Findings, "non-error local-action is omitted from JSON") + } + + stdout, stderr, err := runCommandWithHTTP(t, &httpmock.Registry{}, "--verify-local", "--no-interactive") + checkErr(err) + assert.Contains(t, stdout+stderr, "local path actions") + }) + } +} diff --git a/internal/pipeline/checks/category.go b/internal/pipeline/checks/category.go index 68316dc9..1eeda3f4 100644 --- a/internal/pipeline/checks/category.go +++ b/internal/pipeline/checks/category.go @@ -20,8 +20,8 @@ const ( // RefMoved means the upstream tag now resolves to a different // SHA than what the lockfile has recorded. RefMoved Category = "ref-moved" - // Stale means the pinned SHA no longer matches what the ref - // resolves to today. + // Stale means the lockfile pins a dependency for a workflow that no + // uses: in that workflow references. Stale Category = "stale" // MisleadingSHA means a ref looks like a SHA but resolves to a // different commit. diff --git a/internal/pipeline/checks/finding.go b/internal/pipeline/checks/finding.go index 5f10e3cf..fefaeb7d 100644 --- a/internal/pipeline/checks/finding.go +++ b/internal/pipeline/checks/finding.go @@ -122,22 +122,11 @@ func (r *WorkflowReport) CountByCategory(c Category) int { return n } -// IsValid returns true for findings that don't represent integrity violations. +// IsValid reports whether f leaves the verdict valid. Severity alone decides: +// the JSON `valid` field and the exit code must agree with the severities +// consumers see, so a blocking finding has to be emitted as an error. func (f *Finding) IsValid() bool { - if f.Severity == SeverityError { - return false - } - if f.Category.IsInconclusive() { - return true - } - switch f.Category { - case Valid, RunOnly, LocalAction, RepoRenamed, ShaAsRef, RefMoved, VersionRef, OnboardingRequired, StaleWorkflow, SelfRepositoryAction, FreshTag, CooldownConfigIgnored: - return true - case NotPinned: - return f.ActionRef == nil // workflow-level is a warning - default: - return false - } + return f.Severity != SeverityError } // IsWarning returns true for findings that should render as warnings (not errors). diff --git a/internal/pipeline/checks/finding_test.go b/internal/pipeline/checks/finding_test.go new file mode 100644 index 00000000..5b22f215 --- /dev/null +++ b/internal/pipeline/checks/finding_test.go @@ -0,0 +1,23 @@ +package checks + +import ( + "testing" + + parserlock "github.com/github/actions-lockfile/go/pkg/lockfile" + "github.com/stretchr/testify/assert" +) + +func TestFindingIsValidFollowsSeverity(t *testing.T) { + categories := []Category{Stale, NotPinned, RefChanged, UnreachablePin, ShaAsRef, RefMoved, ReachabilityUnknown, StaleWorkflow, Valid} + for _, sev := range []Severity{SeverityOK, SeverityInfo, SeverityWarning, SeverityError} { + for _, cat := range categories { + t.Run(string(cat)+"/"+string(sev), func(t *testing.T) { + f := Finding{Category: cat, Severity: sev, ActionRef: &parserlock.ActionRef{Owner: "o", Repo: "r", Ref: "v1"}} + want := sev != SeverityError + assert.Equal(t, want, f.IsValid()) + r := Report{Workflows: []WorkflowReport{{Findings: []Finding{f}}}} + assert.Equal(t, want, r.IsValid()) + }) + } + } +} diff --git a/internal/pipeline/checks/run_test.go b/internal/pipeline/checks/run_test.go index 28efb04d..03894da8 100644 --- a/internal/pipeline/checks/run_test.go +++ b/internal/pipeline/checks/run_test.go @@ -218,6 +218,11 @@ func TestRunChecks(t *testing.T) { }, workflowRefs: nil, wantCategories: []Category{Stale}, + extra: func(t *testing.T, got []Finding) { + if got[0].Severity != SeverityError { + t.Fatalf("expected error severity, got %s", got[0].Severity) + } + }, }, { name: "no findings: pinned and current with reachable sha", diff --git a/internal/pipeline/checks/structural.go b/internal/pipeline/checks/structural.go index adaef727..401a77a5 100644 --- a/internal/pipeline/checks/structural.go +++ b/internal/pipeline/checks/structural.go @@ -115,6 +115,8 @@ func checkRefChanged(pw ParsedWorkflow, depPins []lockedPin) []Finding { // ref in the workflow references. If the workflow has already been // rewritten to pin by SHA, the lockfile entry (keyed by the original tag) // is still valid — surface keys both ways so we don't false-flag. +// Stale is an error: a frozen check requires the lockfile to match the +// workflows exactly, and a fix run prunes the entry. func checkStale(pw ParsedWorkflow, depPins []lockedPin) []Finding { if len(depPins) == 0 { return nil @@ -140,10 +142,10 @@ func checkStale(pw ParsedWorkflow, depPins []lockedPin) []Finding { f := Finding{ WorkflowPath: pw.Path, Category: Stale, - Severity: SeverityWarning, + Severity: SeverityError, Confidence: ConfidenceHigh, Detail: fmt.Sprintf("lockfile pins %s@%s but no uses: in this workflow references it", nwoLower(p.Owner, p.Repo), p.Ref), - Remediation: "remove the entry or re-run `gh actions-lock`", + Remediation: "run `gh actions-lock` to prune the unused entry", Dependency: &dep.Dependency{ NWO: strings.ToLower(p.NWO), Ref: p.Ref, diff --git a/internal/pipeline/verify_local_test.go b/internal/pipeline/verify_local_test.go index 54434ab2..bc86b31e 100644 --- a/internal/pipeline/verify_local_test.go +++ b/internal/pipeline/verify_local_test.go @@ -236,3 +236,55 @@ func TestVerifyLocalCoverage_LocalPathsMatchVerify(t *testing.T) { }) } } + +func TestVerifyLocalCoverage_ValidFollowsSeverity(t *testing.T) { + const sha = "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" + lock := `version: 'v0.0.2' +dependencies: + 'actions/checkout@` + sha + `': + ref: '` + sha + `' + commit: 'sha1-` + sha + `' + owner_id: 1 + repo_id: 1 + 'actions/setup-go@v5': + ref: 'v5' + commit: 'sha1-` + sha + `' + owner_id: 1 + repo_id: 2 +workflows: + '.github/workflows/ci.yml': + - 'actions/checkout@` + sha + `' +` + for _, tc := range []struct { + name string + extraPins string + wantCats []checks.Category + wantValid bool + }{ + {name: "sha-as-ref warning only is valid", wantCats: []checks.Category{checks.ShaAsRef}, wantValid: true}, + {name: "unused entry is an error", extraPins: " - 'actions/setup-go@v5'\n", wantCats: []checks.Category{checks.ShaAsRef, checks.Stale}, wantValid: false}, + } { + t.Run(tc.name, func(t *testing.T) { + store := writeTestLockfile(t, t.TempDir(), lock+tc.extraPins) + parsed := []checks.ParsedWorkflow{{ + Path: ".github/workflows/ci.yml", + Refs: []parserlock.ActionRef{{Owner: "actions", Repo: "checkout", Ref: sha}}, + }} + + report := VerifyLocalCoverage(parsed, store) + + var cats []checks.Category + hasError := false + for _, f := range report.Workflows[0].Findings { + cats = append(cats, f.Category) + hasError = hasError || f.Severity == checks.SeverityError + if f.Category == checks.Stale { + assert.Equal(t, checks.SeverityError, f.Severity) + } + } + assert.ElementsMatch(t, tc.wantCats, cats) + assert.Equal(t, tc.wantValid, report.IsValid()) + assert.Equal(t, !hasError, report.IsValid(), "valid must agree with severities") + }) + } +} From fc70bcaba57627634111a04e20f6309fd160d967 Mon Sep 17 00:00:00 2001 From: Jeff Martin Date: Sun, 11 Oct 2026 23:49:02 -0700 Subject: [PATCH 2/4] checks: require a lockfile entry for workflows without dependencies Lockfile enforcement requires every workflow to be listed, whether or not it has uses: refs. Verify modes skipped workflows with no refs, so a missing entry or an entry with unused pins passed. Report a missing entry as a fixable not-pinned error and run the stale check for these workflows. --- cmd/gh-actions-lock/command_test.go | 8 ++-- cmd/gh-actions-lock/onboard_gate.go | 13 ++++- cmd/gh-actions-lock/verify_test.go | 66 ++++++++++++++++++++++++++ internal/pipeline/checks/run.go | 3 +- internal/pipeline/checks/run_test.go | 36 ++++++++++++++ internal/pipeline/checks/structural.go | 19 ++++++++ internal/pipeline/diagnose.go | 11 +++++ internal/pipeline/verify_local.go | 5 -- internal/pipeline/verify_local_test.go | 39 ++++++++++----- 9 files changed, 176 insertions(+), 24 deletions(-) diff --git a/cmd/gh-actions-lock/command_test.go b/cmd/gh-actions-lock/command_test.go index 9d24220a..aeaf98ce 100644 --- a/cmd/gh-actions-lock/command_test.go +++ b/cmd/gh-actions-lock/command_test.go @@ -1204,18 +1204,16 @@ jobs: require.NoError(t, os.WriteFile(second, workflow, 0o600)) require.NoError(t, os.WriteFile(ignored, []byte("not a workflow\n"), 0o600)) + // No lockfile, so both workflows are untracked and the run fails. stdout, _, err := runCommandWithHTTP(t, reg, "--no-fix", "--json=valid,workflows", dir) - require.NoError(t, err) + require.ErrorIs(t, err, errSilent) var payload struct { - Valid bool `json:"valid"` Workflows []struct { - Path string `json:"path"` - Valid bool `json:"valid"` + Path string `json:"path"` } `json:"workflows"` } require.NoError(t, json.Unmarshal([]byte(stdout), &payload)) - assert.True(t, payload.Valid) require.Len(t, payload.Workflows, 2) assert.Equal(t, first, payload.Workflows[0].Path) assert.Equal(t, second, payload.Workflows[1].Path) diff --git a/cmd/gh-actions-lock/onboard_gate.go b/cmd/gh-actions-lock/onboard_gate.go index 6031ecd3..8c5f01b8 100644 --- a/cmd/gh-actions-lock/onboard_gate.go +++ b/cmd/gh-actions-lock/onboard_gate.go @@ -27,7 +27,18 @@ func gateNoOnboard(report *checks.Report) []string { refusedKeys := make(map[string]bool) for fi := range wr.Findings { f := &wr.Findings[fi] - if f.Category != checks.NotPinned || f.ActionRef == nil || f.TrackedAction { + if f.Category != checks.NotPinned || f.TrackedAction { + continue + } + if f.ActionRef == nil { + // A dependency-free workflow missing from the lockfile. + if f.Remediable && len(wr.ActionRefs) == 0 { + f.Category = checks.OnboardingRequired + f.Severity = checks.SeverityInfo + f.Detail = "workflow has no lockfile entry; --no-onboard refuses to add new workflows or actions" + f.Remediation = "onboard it first with `gh actions-lock` (without --no-onboard)" + refused = append(refused, wr.Path) + } continue } ar := f.ActionRef diff --git a/cmd/gh-actions-lock/verify_test.go b/cmd/gh-actions-lock/verify_test.go index 533c7ebc..eed7a022 100644 --- a/cmd/gh-actions-lock/verify_test.go +++ b/cmd/gh-actions-lock/verify_test.go @@ -306,3 +306,69 @@ func TestVerifyLocal_LocalActionVerdictFollowsSeverity(t *testing.T) { }) } } + +// TestEmptyWorkflow_LockfileEntryRequired: enforcement requires every +// workflow to be listed, so a workflow with no uses: still needs an entry +// and that entry must not carry pins. +func TestEmptyWorkflow_LockfileEntryRequired(t *testing.T) { + const workflow = ` +name: ci +on: push +jobs: + test: + runs-on: ubuntu-latest + steps: + - run: echo ok +` + const pin = "actions/checkout@v4=sha1-" + parityTestSHA + for _, tc := range []struct { + name string + mode []string + otherPins []string // lockfile covers only other.yml + ownPins []string // lockfile covers workflow.yml + wantCategory checks.Category + wantErr bool + wantEntry string + }{ + {name: "verify untracked", mode: []string{"--verify"}, otherPins: []string{pin}, wantCategory: checks.NotPinned, wantErr: true}, + {name: "verify-local untracked", mode: []string{"--verify-local"}, otherPins: []string{pin}, wantCategory: checks.NotPinned, wantErr: true}, + {name: "fix run adds entry", otherPins: []string{pin}, wantCategory: checks.NotPinned, wantEntry: "'.github/workflows/workflow.yml': []"}, + {name: "no-onboard leaves it", mode: []string{"--no-onboard"}, otherPins: []string{pin}, wantCategory: checks.OnboardingRequired}, + {name: "verify unused pin", mode: []string{"--verify"}, ownPins: []string{pin}, wantCategory: checks.Stale, wantErr: true}, + {name: "verify-local unused pin", mode: []string{"--verify-local"}, ownPins: []string{pin}, wantCategory: checks.Stale, wantErr: true}, + } { + t.Run(tc.name, func(t *testing.T) { + path := writeTempWorkflow(t, workflow, tc.ownPins...) + if len(tc.otherPins) > 0 { + writeTempLockfile(t, ".", "other.yml", tc.otherPins) + } + args := append(slices.Clone(tc.mode), "--no-interactive", path) + + p, err := runParity(t, &httpmock.Registry{}, args...) + if tc.wantErr { + require.ErrorIs(t, err, errSilent) + } else { + require.NoError(t, err) + } + + hasError := false + var cats []string + for _, f := range p.Findings { + hasError = hasError || f.Severity == string(checks.SeverityError) + cats = append(cats, f.Category) + } + // Fix runs report the pre-fix diagnosis, so only the read-only + // modes tie errors to the exit code. + if len(tc.mode) > 0 { + assert.Equal(t, tc.wantErr, hasError, "%+v", p.Findings) + } + assert.Equal(t, !hasError, p.Valid, "valid must agree with severities") + if tc.wantCategory != "" { + assert.Contains(t, cats, string(tc.wantCategory)) + } + if tc.wantEntry != "" { + assert.Contains(t, readTempLockfilePins(t), tc.wantEntry) + } + }) + } +} diff --git a/internal/pipeline/checks/run.go b/internal/pipeline/checks/run.go index 49bd315b..e54636fc 100644 --- a/internal/pipeline/checks/run.go +++ b/internal/pipeline/checks/run.go @@ -20,10 +20,11 @@ import ( // DocURL and ParentNWO are attached by the caller (diagnoseOneParsed) // because they need lookup tables runChecks doesn't carry. func RunChecks(ctx context.Context, pw ParsedWorkflow, lf parserlock.File, r CheckResolver) []Finding { - wfEntry, _ := lf.LookupWorkflow(workflowfile.KeyFromPath(pw.Path)) + wfEntry, tracked := lf.LookupWorkflow(workflowfile.KeyFromPath(pw.Path)) depPins, depIndex := parseWorkflowDeps(wfEntry, lf.Dependencies) var out []Finding + out = append(out, checkUntracked(pw, tracked)...) out = append(out, checkNotPinned(pw, depPins, depIndex)...) out = append(out, checkShaAsRef(pw, depIndex)...) out = append(out, checkRefChanged(pw, depPins)...) diff --git a/internal/pipeline/checks/run_test.go b/internal/pipeline/checks/run_test.go index 03894da8..d22870b1 100644 --- a/internal/pipeline/checks/run_test.go +++ b/internal/pipeline/checks/run_test.go @@ -641,3 +641,39 @@ func TestRunChecks_AllFindingsCarryConfidence(t *testing.T) { } } } + +// TestRunChecks_UntrackedWorkflow: enforcement requires every workflow to be +// listed, so a workflow with no uses: still needs an entry. +func TestRunChecks_UntrackedWorkflow(t *testing.T) { + lf := checkNewLockfile(map[string][]string{ + ".github/workflows/empty.yml": {}, + }) + for _, tc := range []struct { + name string + pw ParsedWorkflow + want []string + }{ + {name: "tracked empty workflow", pw: checkParsedWF(".github/workflows/empty.yml")}, + {name: "untracked empty workflow", pw: checkParsedWF(".github/workflows/new.yml"), want: []string{string(NotPinned)}}, + { + name: "untracked workflow with refs reports per ref", + pw: checkParsedWF(".github/workflows/new.yml", checkRef("actions", "checkout", "v4")), + want: []string{string(NotPinned)}, + }, + } { + t.Run(tc.name, func(t *testing.T) { + got := RunChecks(context.Background(), tc.pw, lf, nil) + if cats := findingCategories(got); strings.Join(cats, ",") != strings.Join(tc.want, ",") { + t.Fatalf("categories: got %v, want %v", cats, tc.want) + } + for _, f := range got { + if f.Severity != SeverityError || !f.IsRemediableNotPinned() { + t.Errorf("want remediable error, got %+v", f) + } + if (f.ActionRef != nil) != (len(tc.pw.Refs) > 0) { + t.Errorf("ActionRef: got %v, want per-ref only when the workflow has refs", f.ActionRef) + } + } + }) + } +} diff --git a/internal/pipeline/checks/structural.go b/internal/pipeline/checks/structural.go index 401a77a5..19064d33 100644 --- a/internal/pipeline/checks/structural.go +++ b/internal/pipeline/checks/structural.go @@ -8,6 +8,25 @@ import ( "github.com/github/gh-actions-lock/internal/dep" ) +// checkUntracked emits NotPinned for a workflow with no uses: refs that +// has no lockfile entry. Lockfile enforcement requires every workflow to be +// listed, dependencies or not; workflows with refs already get per-ref +// NotPinned findings. +func checkUntracked(pw ParsedWorkflow, tracked bool) []Finding { + if tracked || len(pw.Refs) > 0 { + return nil + } + return []Finding{{ + WorkflowPath: pw.Path, + Category: NotPinned, + Severity: SeverityError, + Confidence: ConfidenceHigh, + Remediable: true, + Detail: "workflow has no lockfile entry", + Remediation: "run `gh actions-lock` to add it", + }} +} + // checkNotPinned emits NotPinned for any uses: ref that has no // matching lockfile entry. When the lockfile has an entry for the same // action at a different ref, RefChanged wins, except for bare SHAs: an diff --git a/internal/pipeline/diagnose.go b/internal/pipeline/diagnose.go index 14ce82d9..5f1e0e11 100644 --- a/internal/pipeline/diagnose.go +++ b/internal/pipeline/diagnose.go @@ -263,10 +263,21 @@ func precheckWorkflow(pw checks.ParsedWorkflow, store *lockfile.State) (checks.W } if len(pw.Refs) == 0 { + // No refs to resolve, but the workflow entry itself must still be + // present and carry no unused pins. + if store != nil { + for _, f := range checks.RunChecks(context.Background(), pw, store.File(), nil) { + f.DocURL = DocURLFor(f.Category) + wr.Findings = append(wr.Findings, f) + } + } if len(pw.SelfRepositoryRefs) > 0 { wr.Findings = append(wr.Findings, selfRepositoryFinding(pw)) return wr, true } + if len(wr.Findings) > 0 { + return wr, true + } wr.Findings = append(wr.Findings, checks.Finding{ WorkflowPath: pw.Path, Category: checks.RunOnly, diff --git a/internal/pipeline/verify_local.go b/internal/pipeline/verify_local.go index 2686bae2..5b41bff8 100644 --- a/internal/pipeline/verify_local.go +++ b/internal/pipeline/verify_local.go @@ -90,11 +90,6 @@ func VerifyLocalCoverage(parsed []checks.ParsedWorkflow, store *lockfile.State) continue } - if len(pw.Refs) == 0 { - results = append(results, wr) - continue - } - // Use RunChecks with a nil resolver to get structural-only findings // (NotPinned, ShaAsRef, RefChanged, Stale). No network calls. findings := checks.RunChecks(context.Background(), pw, lf, nil) diff --git a/internal/pipeline/verify_local_test.go b/internal/pipeline/verify_local_test.go index bc86b31e..db40fd31 100644 --- a/internal/pipeline/verify_local_test.go +++ b/internal/pipeline/verify_local_test.go @@ -23,6 +23,8 @@ dependencies: workflows: '.github/workflows/ci.yml': - 'actions/checkout@v4' + '.github/workflows/self.yml': [] + '.github/workflows/empty.yml': [] ` func writeTestLockfile(t *testing.T, dir, content string) *lockfile.State { @@ -160,20 +162,33 @@ func TestVerifyLocalCoverage_MissingPin(t *testing.T) { assert.Contains(t, notPinned[0].Detail, "setup-go") } +// TestVerifyLocalCoverage_EmptyWorkflow: lockfile enforcement covers every +// workflow, so one with no uses: still needs an entry. func TestVerifyLocalCoverage_EmptyWorkflow(t *testing.T) { - dir := t.TempDir() - store := writeTestLockfile(t, dir, testLockfileContent) - - parsed := []checks.ParsedWorkflow{ - { - Path: ".github/workflows/empty.yml", - Refs: nil, - }, - } + store := writeTestLockfile(t, t.TempDir(), testLockfileContent) - report := VerifyLocalCoverage(parsed, store) - assert.True(t, report.IsValid()) - assert.Empty(t, report.Workflows[0].Findings) + t.Run("tracked is valid", func(t *testing.T) { + report := VerifyLocalCoverage([]checks.ParsedWorkflow{{Path: ".github/workflows/empty.yml"}}, store) + assert.True(t, report.IsValid()) + assert.Empty(t, report.Workflows[0].Findings) + }) + + t.Run("untracked is a fixable error", func(t *testing.T) { + report := VerifyLocalCoverage([]checks.ParsedWorkflow{{Path: ".github/workflows/new.yml"}}, store) + assert.False(t, report.IsValid()) + require.Len(t, report.Workflows[0].Findings, 1) + f := report.Workflows[0].Findings[0] + assert.Equal(t, checks.NotPinned, f.Category) + assert.Equal(t, checks.SeverityError, f.Severity) + assert.True(t, f.IsRemediableNotPinned()) + }) + + t.Run("unused pin is stale", func(t *testing.T) { + report := VerifyLocalCoverage([]checks.ParsedWorkflow{{Path: ".github/workflows/ci.yml"}}, store) + assert.False(t, report.IsValid()) + require.Len(t, report.Workflows[0].Findings, 1) + assert.Equal(t, checks.Stale, report.Workflows[0].Findings[0].Category) + }) } func TestVerifyLocalCoverage_LoadError(t *testing.T) { From c05cfa4be8c83d757875d4d27c839f436d2586e7 Mon Sep 17 00:00:00 2001 From: Jeff Martin Date: Sun, 11 Oct 2026 23:53:57 -0700 Subject: [PATCH 3/4] scenarios: give $/-only workflows an empty lockfile entry Every workflow now needs a lockfile entry, so these scenarios check that $/ refs need no pins rather than that no entry is needed. --- test/scenarios/catalog.yml | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/test/scenarios/catalog.yml b/test/scenarios/catalog.yml index f1a3c999..3537ab62 100644 --- a/test/scenarios/catalog.yml +++ b/test/scenarios/catalog.yml @@ -1160,7 +1160,7 @@ scenarios: - name: self_repository_action_valid category: workflow_parsing - description: "Bare $/ self repository reference at step level is inherently pinned — offline verify passes with no lockfile entry required" + description: "Bare $/ self repository reference at step level is inherently pinned — offline verify passes with an empty workflow entry, no pins required" needs_token: false needs_stub: false tags: [stub] @@ -1176,6 +1176,10 @@ scenarios: runs-on: ubuntu-latest steps: - uses: $/.github/actions/my-action + lockfile: | + version: 'v0.0.3' + workflows: + '.github/workflows/ci.yml': [] files: .github/actions/my-action/action.yml: | name: Local action @@ -1188,7 +1192,7 @@ scenarios: - name: self_repository_reusable_workflow_valid category: workflow_parsing - description: "$/ self repository reference at reusable-workflow call level (jobs..uses) is inherently pinned — offline verify passes" + description: "$/ self repository reference at reusable-workflow call level (jobs..uses) is inherently pinned — offline verify passes with an empty workflow entry" needs_token: false needs_stub: false tags: [stub] @@ -1202,6 +1206,10 @@ scenarios: jobs: call: uses: $/.github/workflows/reusable.yml + lockfile: | + version: 'v0.0.3' + workflows: + '.github/workflows/ci.yml': [] expect: exit: 0 output_contains: ["complete lockfile coverage"] From 233fa9925fa373ca13c4c9ec9fb128aa5aae6b36 Mon Sep 17 00:00:00 2001 From: Jeff Martin Date: Mon, 12 Oct 2026 09:53:46 -0700 Subject: [PATCH 4/4] verify: cover --no-onboard as the opt-out for untracked workflows --- cmd/gh-actions-lock/verify_test.go | 1 + 1 file changed, 1 insertion(+) diff --git a/cmd/gh-actions-lock/verify_test.go b/cmd/gh-actions-lock/verify_test.go index eed7a022..9aacb919 100644 --- a/cmd/gh-actions-lock/verify_test.go +++ b/cmd/gh-actions-lock/verify_test.go @@ -334,6 +334,7 @@ jobs: {name: "verify-local untracked", mode: []string{"--verify-local"}, otherPins: []string{pin}, wantCategory: checks.NotPinned, wantErr: true}, {name: "fix run adds entry", otherPins: []string{pin}, wantCategory: checks.NotPinned, wantEntry: "'.github/workflows/workflow.yml': []"}, {name: "no-onboard leaves it", mode: []string{"--no-onboard"}, otherPins: []string{pin}, wantCategory: checks.OnboardingRequired}, + {name: "verify no-onboard opts out", mode: []string{"--verify", "--no-onboard"}, otherPins: []string{pin}, wantCategory: checks.OnboardingRequired}, {name: "verify unused pin", mode: []string{"--verify"}, ownPins: []string{pin}, wantCategory: checks.Stale, wantErr: true}, {name: "verify-local unused pin", mode: []string{"--verify-local"}, ownPins: []string{pin}, wantCategory: checks.Stale, wantErr: true}, } {