From 2b02ccb06f5e92debe0546a377aaa594bc9d1fa6 Mon Sep 17 00:00:00 2001 From: Jeff Martin Date: Tue, 9 Jun 2026 21:52:30 -0500 Subject: [PATCH 01/13] feat: add --no-narrow flag and respect locked ref precision Add --no-narrow flag to preserve mutable version refs (e.g. v4) in the lock comment instead of narrowing them to full patch tags (v4.2.1). Once a dep is locked with an imprecise tag, subsequent re-pins (e.g. on ref-moved) respect that choice by checking the existing lockfile for mutable refs before narrowing. This makes the precision sticky without requiring --no-narrow on every run. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- cmd/gh-actions-pin/check.go | 6 ++++++ internal/pin/plan.go | 26 ++++++++++++++++++++++++++ 2 files changed, 32 insertions(+) diff --git a/cmd/gh-actions-pin/check.go b/cmd/gh-actions-pin/check.go index 99658132..95a15c44 100644 --- a/cmd/gh-actions-pin/check.go +++ b/cmd/gh-actions-pin/check.go @@ -40,6 +40,10 @@ type checkOptions struct { // rewriting workflows or updating the lockfile. Orthogonal to the // renderer choice (--json). noFix bool + // noNarrow disables tag narrowing: mutable version refs like "v4" + // are kept as-is in the lock comment instead of being resolved to + // the full patch tag (e.g. "v4.2.1"). + noNarrow bool } func newCheckCmd(newResolver resolverFunc) *cobra.Command { @@ -124,6 +128,7 @@ func bindCheckFlags(cmd *cobra.Command, opts *checkOptions) { cmd.Flags().StringVar(&opts.hostname, "hostname", "", "GitHub hostname to query (defaults to GH_HOST, current repo host, or github.com)") cmd.Flags().BoolVar(&opts.rescan, "rescan", false, "Re-verify reachability for every recorded pin (bypasses the lockfile fast path)") cmd.Flags().BoolVar(&opts.noFix, "no-fix", false, "Read-only: report findings without modifying workflows or the lockfile") + cmd.Flags().BoolVar(&opts.noNarrow, "no-narrow", false, "Keep mutable version refs (e.g. v4) instead of narrowing to full patch tags (e.g. v4.2.1)") cmd.Flags().StringVar(&opts.profileDir, "profile", "", "Enable profiling: write trace, CPU profile, and HTTP log to `dir`") } @@ -303,6 +308,7 @@ func runCheck(cmd *cobra.Command, opts *checkOptions, newResolver resolverFunc) RepoOwner: repoOwner, RepoName: repoName, Version: cliVersion(), + NoNarrow: opts.noNarrow, }) endPlan() if planErr != nil { diff --git a/internal/pin/plan.go b/internal/pin/plan.go index c6aba1f8..ff772d91 100644 --- a/internal/pin/plan.go +++ b/internal/pin/plan.go @@ -26,6 +26,10 @@ type PlanOptions struct { RepoOwner string // for same-owner narrowing skip RepoName string Version string // CLI version for the record + // NoNarrow disables tag narrowing: mutable version refs (v4, v3.1) + // are kept as the lock comment instead of being resolved to full + // patch tags (v4.2.1). Bare-SHA reverse lookup still applies. + NoNarrow bool // OnProgress is called at each phase boundary with a human-readable // label (e.g. "Resolving actions/checkout"). Nil means no progress. @@ -235,6 +239,23 @@ func planWorkflow(ctx context.Context, wr checks.WorkflowReport, opts PlanOption for k, v := range autoFixRewrites { rewrites[k] = v } + + // Build set of NWOs previously locked with a mutable ref. When a dep + // was stored imprecisely (e.g. v4), we respect that choice on re-pin + // rather than narrowing to v4.2.1. + prevMutableNWO := make(map[string]bool) + if opts.Store != nil { + wfKey := workflowfile.KeyFromPath(wr.Path) + if existing, err := opts.Store.Get(wfKey); err == nil { + for _, d := range existing { + sv, ok := parserlock.ParseSemVer(d.Ref) + if ok && sv.IsMutable() { + prevMutableNWO[strings.ToLower(d.NWO)] = true + } + } + } + } + if opts.Tagger != nil { for i := range deps { dep := &deps[i] @@ -269,6 +290,11 @@ func planWorkflow(ctx context.Context, wr checks.WorkflowReport, opts PlanOption } // Mutable version tags (v4, v3.1): narrow to patch release. + // Skip if --no-narrow or if the lockfile already recorded this + // dep with a mutable ref (respect prior precision choice). + if opts.NoNarrow || prevMutableNWO[strings.ToLower(dep.NWO)] { + continue + } sv, ok := parserlock.ParseSemVer(dep.Ref) if !ok || !sv.IsMutable() { continue From 4c38594ff46d9a525c5a8589653fb1cd131033bd Mon Sep 17 00:00:00 2001 From: Jeff Martin Date: Thu, 11 Jun 2026 22:47:35 -0500 Subject: [PATCH 02/13] narrowing: nudge non-semver refs, global sticky precision MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Narrowing now checks whether a ref is a full semver tag (v4.2.1) rather than just whether it's a mutable semver. This broadens the nudge to cover any non-semver ref (v4, main, arbitrary tags) — pushing users toward precise refs where each tag resolves to exactly one commit. Sticky precision is computed globally across all workflows (not per-WF) to avoid creating duplicate dep entries at different ref granularities for the same NWO. Once a dep is locked imprecisely anywhere, all workflows consistently use that ref. Terminal output: TermWarn nudge listing non-semver deps with guidance. JSON output: info-severity mutable-ref findings injected per workflow. Both suppressed when --no-narrow is set (user explicitly opted out). --- cmd/gh-actions-pin/check.go | 66 ++++++++++++++++++++++- cmd/gh-actions-pin/pin_summary.go | 42 ++++++++++++++- internal/pin/plan.go | 40 ++++++++------ internal/pipeline/checks/category.go | 5 ++ internal/pipeline/checks/category_test.go | 3 +- internal/pipeline/checks/finding.go | 4 +- 6 files changed, 138 insertions(+), 22 deletions(-) diff --git a/cmd/gh-actions-pin/check.go b/cmd/gh-actions-pin/check.go index 95a15c44..0a426a94 100644 --- a/cmd/gh-actions-pin/check.go +++ b/cmd/gh-actions-pin/check.go @@ -13,11 +13,13 @@ import ( "github.com/MakeNowJust/heredoc" "github.com/cli/go-gh/v2/pkg/repository" + parserlock "github.com/github/actions-lockfile/go/pkg/lockfile" "github.com/github/gh-actions-pin/cmd/gh-actions-pin/format" "github.com/github/gh-actions-pin/internal/config" "github.com/github/gh-actions-pin/internal/pin" "github.com/github/gh-actions-pin/internal/pinpool" "github.com/github/gh-actions-pin/internal/pipeline" + "github.com/github/gh-actions-pin/internal/pipeline/checks" "github.com/github/gh-actions-pin/internal/profile" "github.com/github/gh-actions-pin/internal/resolve" "github.com/github/gh-actions-pin/internal/tag" @@ -327,6 +329,13 @@ func runCheck(cmd *cobra.Command, opts *checkOptions, newResolver resolverFunc) console.StopProgress() + // Inject info-severity findings for non-semver refs so they appear + // in --json output. Suppressed when --no-narrow is set (user chose + // this deliberately). + if !opts.noNarrow { + injectVersionRefFindings(report, record) + } + // Write the run log. record.Repo = &pin.RepoInfo{Owner: repoOwner, Name: repoName, Host: resolveHostname(opts.hostname)} if path, werr := record.WriteJSON(); werr == nil { @@ -351,9 +360,10 @@ func runCheck(cmd *cobra.Command, opts *checkOptions, newResolver resolverFunc) return nil } + // Terminal summary. // Terminal summary. hasInconclusive := opts.rescan && report.HasInconclusive() - summaryErr := renderPinSummary(console, record, report, r, skippedRescan, hasInconclusive) + summaryErr := renderPinSummary(console, record, report, r, skippedRescan, hasInconclusive, opts.noNarrow) // Surface the SAML SSO authorization URL if one was captured during // the run, matching cli/cli's "Authorize in your web browser:" line. @@ -380,6 +390,60 @@ func runCheck(cmd *cobra.Command, opts *checkOptions, newResolver resolverFunc) return nil } +// injectVersionRefFindings appends info-severity findings for entries pinned +// with a non-full-semver ref (v4, v3.1, main, etc.). These surface in --json +// output so machine consumers can detect imprecise refs. +func injectVersionRefFindings(report *checks.Report, record *pin.Record) { + // Index which workflows each non-semver dep appears in. + type depInfo struct { + nwo string + ref string + wfs map[string]bool + } + seen := map[string]*depInfo{} // NWO@Ref → info + for _, e := range record.Entries { + if e.Resolution != pin.Pinned && e.Resolution != pin.Verified { + continue + } + sv, ok := parserlock.ParseSemVer(e.Ref) + if ok && sv.IsFull() { + continue + } + key := e.NWO + "@" + e.Ref + di, exists := seen[key] + if !exists { + di = &depInfo{nwo: e.NWO, ref: e.Ref, wfs: map[string]bool{}} + seen[key] = di + } + for _, wf := range e.Workflows { + di.wfs[wf] = true + } + } + if len(seen) == 0 { + return + } + + // Append a finding to each affected workflow report. + for i := range report.Workflows { + wr := &report.Workflows[i] + for _, di := range seen { + if !di.wfs[wr.Path] { + continue + } + wr.Findings = append(wr.Findings, checks.Finding{ + WorkflowPath: wr.Path, + Category: checks.VersionRef, + Severity: checks.SeverityInfo, + Confidence: checks.ConfidenceHigh, + Detail: fmt.Sprintf( + "%s@%s: prefer a full semver ref (e.g. v4.2.1) — each patch tag resolves to exactly one commit", + di.nwo, di.ref, + ), + }) + } + } +} + // cliVersion returns the gh-actions-pin extension version embedded by the Go // build system. Returns "(devel)" for local `go build` and a real version // like "v0.1.2" when installed via `gh extension install`. diff --git a/cmd/gh-actions-pin/pin_summary.go b/cmd/gh-actions-pin/pin_summary.go index 7abf8a4e..a71045a4 100644 --- a/cmd/gh-actions-pin/pin_summary.go +++ b/cmd/gh-actions-pin/pin_summary.go @@ -4,6 +4,7 @@ import ( "fmt" "strings" + parserlock "github.com/github/actions-lockfile/go/pkg/lockfile" "github.com/github/gh-actions-pin/cmd/gh-actions-pin/format" "github.com/github/gh-actions-pin/internal/pin" "github.com/github/gh-actions-pin/internal/pipeline" @@ -15,7 +16,7 @@ import ( // renderPinSummary prints the terminal summary after pin.Plan + pin.Commit. // It groups pinned entries by NWO@Ref, shows investigation alerts, unresolved // warnings, and the all-valid message when nothing changed. -func renderPinSummary(console *ui.UI, record *pin.Record, report *checks.Report, r *resolve.Resolver, skippedRescan int, hasInconclusive bool) error { +func renderPinSummary(console *ui.UI, record *pin.Record, report *checks.Report, r *resolve.Resolver, skippedRescan int, hasInconclusive bool, noNarrow bool) error { pinned := record.Pinned() investigated := record.Investigated() @@ -24,6 +25,9 @@ func renderPinSummary(console *ui.UI, record *pin.Record, report *checks.Report, } renderFullScanWarnings(console, pinned) + if !noNarrow { + renderVersionRefNudge(console, record) + } if len(investigated) > 0 { renderInvestigationAlerts(console, investigated, r) @@ -400,3 +404,39 @@ func stripNWORefPrefix(s string) string { } return rest[colonIdx+2:] } + +// renderVersionRefNudge prints an informational nudge when entries are pinned +// with refs that are not full semver tags (v4.2.1). Full semver tags each +// resolve to exactly one commit, making the lock comment durable across +// re-pins. +func renderVersionRefNudge(console *ui.UI, record *pin.Record) { + var nonSemverDeps []string + seen := map[string]bool{} + for _, e := range record.Entries { + if e.Resolution != pin.Pinned && e.Resolution != pin.Verified { + continue + } + sv, ok := parserlock.ParseSemVer(e.Ref) + if ok && sv.IsFull() { + continue + } + key := e.NWO + "@" + e.Ref + if seen[key] { + continue + } + seen[key] = true + nonSemverDeps = append(nonSemverDeps, key) + } + if len(nonSemverDeps) == 0 { + return + } + console.TermBlank() + console.TermWarn("%d %s pinned without a full semver tag", + len(nonSemverDeps), ui.Pluralize(len(nonSemverDeps), "action", "actions")) + for _, dep := range nonSemverDeps { + console.TermDetail(" %s", console.TermYellow(dep)) + } + console.TermDetail(" Prefer full semver refs (e.g. v4.2.1) — each patch tag resolves to") + console.TermDetail(" exactly one commit, making the lock comment durable across re-pins.") + console.TermDetail(" Run without --no-narrow to upgrade.") +} diff --git a/internal/pin/plan.go b/internal/pin/plan.go index ff772d91..f0978546 100644 --- a/internal/pin/plan.go +++ b/internal/pin/plan.go @@ -31,6 +31,13 @@ type PlanOptions struct { // patch tags (v4.2.1). Bare-SHA reverse lookup still applies. NoNarrow bool + // prevMutableNWO is computed once in Plan() from the global lockfile + // state. It holds lowercased NWOs that are already recorded with a + // non-full-semver ref anywhere in the lockfile. Narrowing is skipped + // for these to respect the user's prior precision choice and avoid + // creating duplicate dep entries at different ref granularities. + prevMutableNWO map[string]bool + // OnProgress is called at each phase boundary with a human-readable // label (e.g. "Resolving actions/checkout"). Nil means no progress. OnProgress func(phase string) @@ -54,6 +61,20 @@ func Plan(ctx context.Context, report *checks.Report, opts PlanOptions) (*Record items[i] = indexedWR{idx: i, wr: wr} } + // Build set of NWOs globally recorded with a non-semver ref. Checked + // across all workflows (not per-WF) so two workflows referencing the + // same action settle on the same ref precision — avoiding duplicate + // dep entries in the lockfile. + if opts.prevMutableNWO == nil && opts.Store != nil { + opts.prevMutableNWO = make(map[string]bool) + for _, d := range opts.Store.AllDeps() { + sv, ok := parserlock.ParseSemVer(d.Ref) + if !ok || !sv.IsFull() { + opts.prevMutableNWO[strings.ToLower(d.NWO)] = true + } + } + } + results := make([]planResult, len(report.Workflows)) var planErr error poolErr := pinpool.RunTyped(opts.Pool, ctx, "Planning pins", @@ -240,22 +261,6 @@ func planWorkflow(ctx context.Context, wr checks.WorkflowReport, opts PlanOption rewrites[k] = v } - // Build set of NWOs previously locked with a mutable ref. When a dep - // was stored imprecisely (e.g. v4), we respect that choice on re-pin - // rather than narrowing to v4.2.1. - prevMutableNWO := make(map[string]bool) - if opts.Store != nil { - wfKey := workflowfile.KeyFromPath(wr.Path) - if existing, err := opts.Store.Get(wfKey); err == nil { - for _, d := range existing { - sv, ok := parserlock.ParseSemVer(d.Ref) - if ok && sv.IsMutable() { - prevMutableNWO[strings.ToLower(d.NWO)] = true - } - } - } - } - if opts.Tagger != nil { for i := range deps { dep := &deps[i] @@ -292,7 +297,8 @@ func planWorkflow(ctx context.Context, wr checks.WorkflowReport, opts PlanOption // Mutable version tags (v4, v3.1): narrow to patch release. // Skip if --no-narrow or if the lockfile already recorded this // dep with a mutable ref (respect prior precision choice). - if opts.NoNarrow || prevMutableNWO[strings.ToLower(dep.NWO)] { + nwoLower := strings.ToLower(dep.NWO) + if opts.NoNarrow || opts.prevMutableNWO[nwoLower] { continue } sv, ok := parserlock.ParseSemVer(dep.Ref) diff --git a/internal/pipeline/checks/category.go b/internal/pipeline/checks/category.go index 394c4aaa..34900f50 100644 --- a/internal/pipeline/checks/category.go +++ b/internal/pipeline/checks/category.go @@ -58,6 +58,11 @@ const ( // run; the operator must onboard the workflow explicitly before // re-running upgrade. OnboardingRequired Category = "onboarding-required" + // MutableRef is an informational nudge: a dependency is pinned with a + // ref that is not a full semver tag (e.g. v4, v3.1, main). Full semver + // tags (v4.2.1) each resolve to exactly one commit, making the lock + // comment durable across re-pins. + MutableRef Category = "mutable-ref" ) // IsInconclusive reports whether c represents a diagnostic that diff --git a/internal/pipeline/checks/category_test.go b/internal/pipeline/checks/category_test.go index 24acf920..8b9038c1 100644 --- a/internal/pipeline/checks/category_test.go +++ b/internal/pipeline/checks/category_test.go @@ -24,6 +24,7 @@ func TestCategoryStringsAreFrozen(t *testing.T) { {AncestryUnknown, "ancestry-unknown"}, {ReachabilityUnknown, "reachability-unknown"}, {OnboardingRequired, "onboarding-required"}, + {MutableRef, "mutable-ref"}, } for _, c := range cases { if string(c.got) != c.want { @@ -45,7 +46,7 @@ func TestCategoryIsInconclusive(t *testing.T) { blocking := []Category{ NotPinned, ShaAsRef, RefChanged, RefMoved, Stale, ImpostorCommit, MisleadingSHA, LockfileForgery, - Valid, RunOnly, OnboardingRequired, + Valid, RunOnly, OnboardingRequired, MutableRef, } for _, c := range blocking { if c.IsInconclusive() { diff --git a/internal/pipeline/checks/finding.go b/internal/pipeline/checks/finding.go index 6057c7f7..ff49594c 100644 --- a/internal/pipeline/checks/finding.go +++ b/internal/pipeline/checks/finding.go @@ -78,7 +78,7 @@ func (r *WorkflowReport) NeedsAttention() bool { continue } switch f.Category { - case Valid, RunOnly, MisleadingSHA, RefMoved: + case Valid, RunOnly, MisleadingSHA, RefMoved, MutableRef: continue default: return true @@ -107,7 +107,7 @@ func (f *Finding) IsValid() bool { return true } switch f.Category { - case Valid, RunOnly, ShaAsRef, RefMoved: + case Valid, RunOnly, ShaAsRef, RefMoved, MutableRef: return true case NotPinned: return f.ActionRef == nil // workflow-level is a warning From 992e3a151e388aa4b3ad2183d143a7c383798963 Mon Sep 17 00:00:00 2001 From: Jeff Martin Date: Thu, 11 Jun 2026 23:02:30 -0500 Subject: [PATCH 03/13] rename mutable-ref to version-ref MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Drop the 'mutable' framing — any git ref can be rewritten today, so the term is misleading. VersionRef better describes what we're checking: is the ref a full semver tag (v4.2.1) or not. Renames: MutableRef to VersionRef, mutable-ref to version-ref, prevMutableNWO to prevImpreciseNWO, IsMutable to IsFull. --- internal/pin/plan.go | 18 +++++++++--------- internal/pipeline/checks/category.go | 4 ++-- internal/pipeline/checks/category_test.go | 4 ++-- internal/pipeline/checks/finding.go | 4 ++-- 4 files changed, 15 insertions(+), 15 deletions(-) diff --git a/internal/pin/plan.go b/internal/pin/plan.go index f0978546..ed1e0cf8 100644 --- a/internal/pin/plan.go +++ b/internal/pin/plan.go @@ -31,12 +31,12 @@ type PlanOptions struct { // patch tags (v4.2.1). Bare-SHA reverse lookup still applies. NoNarrow bool - // prevMutableNWO is computed once in Plan() from the global lockfile + // prevImpreciseNWO is computed once in Plan() from the global lockfile // state. It holds lowercased NWOs that are already recorded with a // non-full-semver ref anywhere in the lockfile. Narrowing is skipped // for these to respect the user's prior precision choice and avoid // creating duplicate dep entries at different ref granularities. - prevMutableNWO map[string]bool + prevImpreciseNWO map[string]bool // OnProgress is called at each phase boundary with a human-readable // label (e.g. "Resolving actions/checkout"). Nil means no progress. @@ -65,12 +65,12 @@ func Plan(ctx context.Context, report *checks.Report, opts PlanOptions) (*Record // across all workflows (not per-WF) so two workflows referencing the // same action settle on the same ref precision — avoiding duplicate // dep entries in the lockfile. - if opts.prevMutableNWO == nil && opts.Store != nil { - opts.prevMutableNWO = make(map[string]bool) + if opts.prevImpreciseNWO == nil && opts.Store != nil { + opts.prevImpreciseNWO = make(map[string]bool) for _, d := range opts.Store.AllDeps() { sv, ok := parserlock.ParseSemVer(d.Ref) if !ok || !sv.IsFull() { - opts.prevMutableNWO[strings.ToLower(d.NWO)] = true + opts.prevImpreciseNWO[strings.ToLower(d.NWO)] = true } } } @@ -294,15 +294,15 @@ func planWorkflow(ctx context.Context, wr checks.WorkflowReport, opts PlanOption continue } - // Mutable version tags (v4, v3.1): narrow to patch release. + // Version tags without full semver (v4, v3.1): narrow to patch release. // Skip if --no-narrow or if the lockfile already recorded this - // dep with a mutable ref (respect prior precision choice). + // dep without a full semver ref (respect prior precision choice). nwoLower := strings.ToLower(dep.NWO) - if opts.NoNarrow || opts.prevMutableNWO[nwoLower] { + if opts.NoNarrow || opts.prevImpreciseNWO[nwoLower] { continue } sv, ok := parserlock.ParseSemVer(dep.Ref) - if !ok || !sv.IsMutable() { + if !ok || sv.IsFull() { continue } patchTag, err := opts.Tagger.BestPatchTagForSHA(ctx, owner, repo, dep.SHA) diff --git a/internal/pipeline/checks/category.go b/internal/pipeline/checks/category.go index 34900f50..65031814 100644 --- a/internal/pipeline/checks/category.go +++ b/internal/pipeline/checks/category.go @@ -58,11 +58,11 @@ const ( // run; the operator must onboard the workflow explicitly before // re-running upgrade. OnboardingRequired Category = "onboarding-required" - // MutableRef is an informational nudge: a dependency is pinned with a + // VersionRef is an informational nudge: a dependency is pinned with a // ref that is not a full semver tag (e.g. v4, v3.1, main). Full semver // tags (v4.2.1) each resolve to exactly one commit, making the lock // comment durable across re-pins. - MutableRef Category = "mutable-ref" + VersionRef Category = "version-ref" ) // IsInconclusive reports whether c represents a diagnostic that diff --git a/internal/pipeline/checks/category_test.go b/internal/pipeline/checks/category_test.go index 8b9038c1..185361a3 100644 --- a/internal/pipeline/checks/category_test.go +++ b/internal/pipeline/checks/category_test.go @@ -24,7 +24,7 @@ func TestCategoryStringsAreFrozen(t *testing.T) { {AncestryUnknown, "ancestry-unknown"}, {ReachabilityUnknown, "reachability-unknown"}, {OnboardingRequired, "onboarding-required"}, - {MutableRef, "mutable-ref"}, + {VersionRef, "version-ref"}, } for _, c := range cases { if string(c.got) != c.want { @@ -46,7 +46,7 @@ func TestCategoryIsInconclusive(t *testing.T) { blocking := []Category{ NotPinned, ShaAsRef, RefChanged, RefMoved, Stale, ImpostorCommit, MisleadingSHA, LockfileForgery, - Valid, RunOnly, OnboardingRequired, MutableRef, + Valid, RunOnly, OnboardingRequired, VersionRef, } for _, c := range blocking { if c.IsInconclusive() { diff --git a/internal/pipeline/checks/finding.go b/internal/pipeline/checks/finding.go index ff49594c..3fd34706 100644 --- a/internal/pipeline/checks/finding.go +++ b/internal/pipeline/checks/finding.go @@ -78,7 +78,7 @@ func (r *WorkflowReport) NeedsAttention() bool { continue } switch f.Category { - case Valid, RunOnly, MisleadingSHA, RefMoved, MutableRef: + case Valid, RunOnly, MisleadingSHA, RefMoved, VersionRef: continue default: return true @@ -107,7 +107,7 @@ func (f *Finding) IsValid() bool { return true } switch f.Category { - case Valid, RunOnly, ShaAsRef, RefMoved, MutableRef: + case Valid, RunOnly, ShaAsRef, RefMoved, VersionRef: return true case NotPinned: return f.ActionRef == nil // workflow-level is a warning From 46b4ba1ac36587435f58377dc13e38574d281f87 Mon Sep 17 00:00:00 2001 From: Jeff Martin Date: Fri, 12 Jun 2026 09:46:51 -0500 Subject: [PATCH 04/13] port contract-essential pieces from PR #36 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Bring in the infrastructure Dependabot needs to shell out to gh-actions-pin check --no-onboard --no-narrow --no-interactive: - --no-onboard / --no-interactive persistent flags on root command - onboard gate: rewrites not-pinned → onboarding-required for new entries when --no-onboard is set, so relock never silently adds deps - corrupt lockfile recovery with interactive confirm / CI fail-fast - branch/tag preservation in lockfile Set() for branchless read-path - stale inventory pruning so Plan() skips entries the pipeline dropped - impostor pin retention and commit helpers (pin/commit.go) - OnboardingRequired in terminal error + alerted category lists All existing tests pass; new test files for lock recovery, impostor retention, and lockfile state included. --- cmd/gh-actions-pin/check.go | 23 +- cmd/gh-actions-pin/format/terminal.go | 4 +- cmd/gh-actions-pin/lockrecovery.go | 97 ++++++ cmd/gh-actions-pin/lockrecovery_test.go | 138 +++++++++ cmd/gh-actions-pin/onboard_gate.go | 50 +++ cmd/gh-actions-pin/pin_summary.go | 14 +- cmd/gh-actions-pin/root.go | 27 +- go.mod | 14 + go.sum | 65 ++++ internal/lockfile/state.go | 47 ++- internal/lockfile/state_test.go | 393 ++++++++++++++++++++++++ internal/pin/commit.go | 39 +++ internal/pin/plan.go | 35 ++- internal/pin/retain_impostor_test.go | 91 ++++++ internal/pipeline/checks/category.go | 11 +- 15 files changed, 1016 insertions(+), 32 deletions(-) create mode 100644 cmd/gh-actions-pin/lockrecovery.go create mode 100644 cmd/gh-actions-pin/lockrecovery_test.go create mode 100644 cmd/gh-actions-pin/onboard_gate.go create mode 100644 internal/pin/retain_impostor_test.go diff --git a/cmd/gh-actions-pin/check.go b/cmd/gh-actions-pin/check.go index 0a426a94..2d4d4da8 100644 --- a/cmd/gh-actions-pin/check.go +++ b/cmd/gh-actions-pin/check.go @@ -180,7 +180,11 @@ func runCheck(cmd *cobra.Command, opts *checkOptions, newResolver resolverFunc) } endSetup := prof.Phase("setup (discover + lockfile)") - paths, r, store, err := newRun(opts.workflowPaths, opts.hostname, pool, newResolver) + // check fix mode can rebuild a deleted lockfile, so interactive sessions + // may delete-and-recreate an unreadable one. --no-fix is read-only and + // must not delete; it fails instead. + recoverLock := newLockRecovery(noInteractiveFlag(cmd), console, confirmFactoryHook, !opts.noFix) + paths, r, store, err := newRun(opts.workflowPaths, opts.hostname, pool, newResolver, recoverLock) if err != nil { return err } @@ -257,6 +261,18 @@ func runCheck(cmd *cobra.Command, opts *checkOptions, newResolver resolverFunc) valid := result.Valid skippedRescan := result.SkippedRescan + // --no-onboard: refuse to onboard new workflows or actions. Rewrite the + // relevant not-pinned findings to onboarding-required and drop their refs + // so Plan/Commit never pins them; already-tracked refs that were bumped + // (ref-changed) are left to re-pin as usual. + onboardingRefused := 0 + if noOnboardFlag(cmd) { + onboardingRefused = gateNoOnboard(report) + if onboardingRefused > 0 { + valid = report.IsValid() + } + } + // Render the read-only diagnosis. --json selects the renderer; it does // not decide whether fixes are applied. Terminal output is shown up front // (the human narrative). JSON is emitted later, after any fixes land, so @@ -354,16 +370,15 @@ func runCheck(cmd *cobra.Command, opts *checkOptions, newResolver resolverFunc) if err := format.WriteJSON(out, report, valid, opts.jsonFields, cliVersion(), store.File().Version); err != nil { return err } - if len(record.Investigated()) > 0 { + if len(record.Investigated()) > 0 || onboardingRefused > 0 { return errSilent } return nil } - // Terminal summary. // Terminal summary. hasInconclusive := opts.rescan && report.HasInconclusive() - summaryErr := renderPinSummary(console, record, report, r, skippedRescan, hasInconclusive, opts.noNarrow) + summaryErr := renderPinSummary(console, record, report, r, skippedRescan, hasInconclusive, onboardingRefused, opts.noNarrow) // Surface the SAML SSO authorization URL if one was captured during // the run, matching cli/cli's "Authorize in your web browser:" line. diff --git a/cmd/gh-actions-pin/format/terminal.go b/cmd/gh-actions-pin/format/terminal.go index 6fedb693..34e8b717 100644 --- a/cmd/gh-actions-pin/format/terminal.go +++ b/cmd/gh-actions-pin/format/terminal.go @@ -95,7 +95,7 @@ func renderErrorFindings(out *ui.UI, report *checks.Report, failedCount, checked parts := []string{} for _, cat := range []checks.Category{ checks.LockfileForgery, - checks.RefChanged, checks.NotPinned, + checks.RefChanged, checks.NotPinned, checks.OnboardingRequired, checks.Stale, checks.MisleadingSHA, checks.ImpostorCommit, } { if n, ok := catCounts[cat]; ok { @@ -252,7 +252,7 @@ func renderWarnings(out *ui.UI, report *checks.Report, willRemediate bool) { // remediator should not re-print it in non-interactive mode). func IsAlertedCategory(c checks.Category) bool { switch c { - case checks.ImpostorCommit, checks.LockfileForgery, checks.MisleadingSHA: + case checks.ImpostorCommit, checks.LockfileForgery, checks.MisleadingSHA, checks.OnboardingRequired: return true } return false diff --git a/cmd/gh-actions-pin/lockrecovery.go b/cmd/gh-actions-pin/lockrecovery.go new file mode 100644 index 00000000..26b9290d --- /dev/null +++ b/cmd/gh-actions-pin/lockrecovery.go @@ -0,0 +1,97 @@ +package main + +import ( + "fmt" + "os" + + "github.com/cli/go-gh/v2/pkg/prompter" + "github.com/github/gh-actions-pin/internal/ui" + "github.com/spf13/cobra" + "golang.org/x/term" +) + +// noInteractiveFlag reports the value of the persistent --no-interactive flag, +// defaulting to false when it is not registered. +func noInteractiveFlag(cmd *cobra.Command) bool { + v, _ := cmd.Flags().GetBool("no-interactive") + return v +} + +// confirmer asks a yes/no question. Satisfied by *prompter.Prompter; an +// interface so tests inject a fake without a TTY. +type confirmer interface { + Confirm(prompt string, defaultValue bool) (bool, error) +} + +// confirmFactory returns a confirmer and whether the session can prompt. +// canPrompt is false in any non-interactive context (no TTY, CI), so the +// recovery policy fails closed instead of blocking on input that never comes. +type confirmFactory func() (confirmer, bool) + +// confirmFactoryHook is the confirm factory commands use to build the +// corrupt-lockfile recovery policy. Production points at defaultConfirmFactory +// (real terminal). Tests override it to drive the interactive delete-and- +// recreate path without a TTY; the command tests run serially (t.Chdir) so a +// package-level override with cleanup is safe. +var confirmFactoryHook confirmFactory = defaultConfirmFactory + +// defaultConfirmFactory binds to the real terminal and renders to stderr so +// `--json` stdout stays clean. It reports canPrompt only when both stdin and +// stderr are TTYs and CI is unset — a CI runner with a stray TTY must never +// be prompted. +func defaultConfirmFactory() (confirmer, bool) { + if !term.IsTerminal(int(os.Stdin.Fd())) || !term.IsTerminal(int(os.Stderr.Fd())) || ciEnabled() { + return nil, false + } + return prompter.New(os.Stdin, os.Stderr, os.Stderr), true +} + +// ciEnabled mirrors the CI convention used by internal/ui: most providers set +// CI=true. A truthy CI value means no interactive prompts. +func ciEnabled() bool { + v := os.Getenv("CI") + return v != "" && v != "0" && v != "false" +} + +// lockRecovery decides what to do when the on-disk lockfile can't be parsed. +// It returns (true, nil) when the lockfile was removed and loading should be +// retried (the empty-lockfile path then recreates it), or a non-nil error to +// abort the run (exit 2). It never silently accepts an unreadable lockfile. +type lockRecovery func(lockPath string, parseErr error) (recovered bool, err error) + +// newLockRecovery builds the recovery policy. allowDelete is false for +// read-only or relock commands that cannot rebuild a deleted lockfile +// (`check --no-fix`, `update`); those always fail with a clear pointer. When +// allowDelete is true (`check` fix mode), an interactive session is offered a +// delete-and-recreate; non-interactive sessions (CI, --no-interactive) fail. +func newLockRecovery(noInteractive bool, console *ui.UI, newConfirm confirmFactory, allowDelete bool) lockRecovery { + return func(lockPath string, parseErr error) (bool, error) { + if !allowDelete { + return false, fmt.Errorf("%w; run `gh actions-pin check` to rebuild it, or delete it by hand", parseErr) + } + var ( + confirm confirmer + canPrompt bool + ) + if newConfirm != nil { + confirm, canPrompt = newConfirm() + } + if noInteractive || !canPrompt { + return false, fmt.Errorf("%w; delete it and re-run to recreate it, or fix it by hand", parseErr) + } + // Release the terminal so the prompt renders cleanly over any spinner. + console.StopProgress() + ok, err := confirm.Confirm(fmt.Sprintf("Lockfile %s is unreadable (%v). Delete and recreate it?", lockPath, parseErr), false) + if err != nil { + return false, err + } + if !ok { + return false, fmt.Errorf("%w; left in place", parseErr) + } + if err := os.Remove(lockPath); err != nil { + return false, fmt.Errorf("deleting unreadable lockfile %s: %w", lockPath, err) + } + console.TermNeutral("Deleted unreadable lockfile %s; it will be recreated.", lockPath) + return true, nil + } +} diff --git a/cmd/gh-actions-pin/lockrecovery_test.go b/cmd/gh-actions-pin/lockrecovery_test.go new file mode 100644 index 00000000..69b60143 --- /dev/null +++ b/cmd/gh-actions-pin/lockrecovery_test.go @@ -0,0 +1,138 @@ +package main + +import ( + "errors" + "io" + "os" + "path/filepath" + "testing" + + "github.com/github/gh-actions-pin/internal/ui" +) + +type fakeConfirmer struct { + result bool + err error + called bool + gotPrompt string +} + +func (f *fakeConfirmer) Confirm(prompt string, _ bool) (bool, error) { + f.called = true + f.gotPrompt = prompt + return f.result, f.err +} + +func writeScratchLock(t *testing.T) string { + t.Helper() + p := filepath.Join(t.TempDir(), "actions.lock") + if err := os.WriteFile(p, []byte("corrupt"), 0o600); err != nil { + t.Fatal(err) + } + return p +} + +func discardUI() *ui.UI { return ui.NewWithWriter(io.Discard) } + +func fileExists(t *testing.T, p string) bool { + t.Helper() + _, err := os.Stat(p) + return err == nil +} + +func TestNewLockRecovery(t *testing.T) { + parseErr := errors.New("missing required action field \"owner_id\"") + + t.Run("read-only/relock command refuses to delete and fails", func(t *testing.T) { + lock := writeScratchLock(t) + fc := &fakeConfirmer{result: true} + rec := newLockRecovery(false, discardUI(), func() (confirmer, bool) { return fc, true }, false) + + recovered, err := rec(lock, parseErr) + if recovered || err == nil { + t.Fatalf("want (false, error), got (%v, %v)", recovered, err) + } + if fc.called { + t.Error("confirmer must not be consulted when delete is disallowed") + } + if !fileExists(t, lock) { + t.Error("lockfile must be left in place") + } + }) + + t.Run("non-interactive flag fails without prompting", func(t *testing.T) { + lock := writeScratchLock(t) + fc := &fakeConfirmer{result: true} + rec := newLockRecovery(true, discardUI(), func() (confirmer, bool) { return fc, true }, true) + + recovered, err := rec(lock, parseErr) + if recovered || err == nil { + t.Fatalf("want (false, error), got (%v, %v)", recovered, err) + } + if fc.called { + t.Error("confirmer must not be consulted under --no-interactive") + } + if !fileExists(t, lock) { + t.Error("lockfile must be left in place") + } + }) + + t.Run("headless session (canPrompt false) fails", func(t *testing.T) { + lock := writeScratchLock(t) + rec := newLockRecovery(false, discardUI(), func() (confirmer, bool) { return nil, false }, true) + + recovered, err := rec(lock, parseErr) + if recovered || err == nil { + t.Fatalf("want (false, error), got (%v, %v)", recovered, err) + } + if !fileExists(t, lock) { + t.Error("lockfile must be left in place") + } + }) + + t.Run("interactive confirm yes deletes and recovers", func(t *testing.T) { + lock := writeScratchLock(t) + fc := &fakeConfirmer{result: true} + rec := newLockRecovery(false, discardUI(), func() (confirmer, bool) { return fc, true }, true) + + recovered, err := rec(lock, parseErr) + if !recovered || err != nil { + t.Fatalf("want (true, nil), got (%v, %v)", recovered, err) + } + if !fc.called { + t.Error("confirmer should have been consulted") + } + if fileExists(t, lock) { + t.Error("lockfile should have been deleted") + } + }) + + t.Run("interactive confirm no fails and keeps file", func(t *testing.T) { + lock := writeScratchLock(t) + fc := &fakeConfirmer{result: false} + rec := newLockRecovery(false, discardUI(), func() (confirmer, bool) { return fc, true }, true) + + recovered, err := rec(lock, parseErr) + if recovered || err == nil { + t.Fatalf("want (false, error), got (%v, %v)", recovered, err) + } + if !fileExists(t, lock) { + t.Error("lockfile must be left in place when the user declines") + } + }) + + t.Run("confirm error propagates and keeps file", func(t *testing.T) { + lock := writeScratchLock(t) + boom := errors.New("prompt failed") + fc := &fakeConfirmer{err: boom} + rec := newLockRecovery(false, discardUI(), func() (confirmer, bool) { return fc, true }, true) + + recovered, err := rec(lock, parseErr) + if recovered || !errors.Is(err, boom) { + t.Fatalf("want (false, boom), got (%v, %v)", recovered, err) + } + if !fileExists(t, lock) { + t.Error("lockfile must be left in place on prompt error") + } + }) +} diff --git a/cmd/gh-actions-pin/onboard_gate.go b/cmd/gh-actions-pin/onboard_gate.go new file mode 100644 index 00000000..650a2c21 --- /dev/null +++ b/cmd/gh-actions-pin/onboard_gate.go @@ -0,0 +1,50 @@ +package main + +import ( + "fmt" + + parserlock "github.com/github/actions-lockfile/go/pkg/lockfile" + "github.com/github/gh-actions-pin/internal/pipeline/checks" + "github.com/spf13/cobra" +) + +// noOnboardFlag reports the value of the persistent --no-onboard flag, +// defaulting to false when it is not registered. +func noOnboardFlag(cmd *cobra.Command) bool { + v, _ := cmd.Flags().GetBool("no-onboard") + return v +} + +// gateNoOnboard rewrites per-workflow NotPinned findings to OnboardingRequired +// and drops their refs so Plan never pins them. Returns refs refused. +func gateNoOnboard(report *checks.Report) int { + refused := 0 + for wi := range report.Workflows { + wr := &report.Workflows[wi] + refusedKeys := make(map[string]bool) + for fi := range wr.Findings { + f := &wr.Findings[fi] + if f.Category != checks.NotPinned || f.ActionRef == nil { + continue + } + ar := f.ActionRef + refusedKeys[parserlock.IndexKey(ar.Owner, ar.Repo, ar.Ref)] = true + f.Category = checks.OnboardingRequired + f.Detail = fmt.Sprintf("%s@%s has no lockfile entry; --no-onboard refuses to add new workflows or actions", ar.FullName(), ar.Ref) + f.Remediation = "onboard it first with `gh actions-pin check` (without --no-onboard)" + refused++ + } + if len(refusedKeys) == 0 { + continue + } + kept := make([]parserlock.ActionRef, 0, len(wr.ActionRefs)) + for _, ar := range wr.ActionRefs { + if refusedKeys[parserlock.IndexKey(ar.Owner, ar.Repo, ar.Ref)] { + continue + } + kept = append(kept, ar) + } + wr.ActionRefs = kept + } + return refused +} diff --git a/cmd/gh-actions-pin/pin_summary.go b/cmd/gh-actions-pin/pin_summary.go index a71045a4..368f5d20 100644 --- a/cmd/gh-actions-pin/pin_summary.go +++ b/cmd/gh-actions-pin/pin_summary.go @@ -16,7 +16,7 @@ import ( // renderPinSummary prints the terminal summary after pin.Plan + pin.Commit. // It groups pinned entries by NWO@Ref, shows investigation alerts, unresolved // warnings, and the all-valid message when nothing changed. -func renderPinSummary(console *ui.UI, record *pin.Record, report *checks.Report, r *resolve.Resolver, skippedRescan int, hasInconclusive bool, noNarrow bool) error { +func renderPinSummary(console *ui.UI, record *pin.Record, report *checks.Report, r *resolve.Resolver, skippedRescan int, hasInconclusive bool, onboardingRefused int, noNarrow bool) error { pinned := record.Pinned() investigated := record.Investigated() @@ -43,7 +43,8 @@ func renderPinSummary(console *ui.UI, record *pin.Record, report *checks.Report, console.TermNeutral("No workflows to check") return nil } - if len(pinned) == 0 && len(investigated) == 0 && len(unresolvedEntries) == 0 && !hasInconclusive { + allClean := len(pinned) == 0 && len(investigated) == 0 && len(unresolvedEntries) == 0 + if allClean && onboardingRefused == 0 && !hasInconclusive { console.TermSuccess("All %d %s valid", total, ui.Pluralize(total, "workflow", "workflows")) if skippedRescan > 0 { console.TermDetail("Trusted lockfile for %d already-pinned %s; run `gh actions-pin --rescan` to re-verify reachability.", @@ -52,11 +53,14 @@ func renderPinSummary(console *ui.UI, record *pin.Record, report *checks.Report, return nil } - if len(unresolvedEntries) == 0 && len(investigated) == 0 { - return nil + if onboardingRefused > 0 { + console.TermBlank() + console.TermCaution("%d onboarding-required %s skipped — re-run without --no-onboard to add %s", + onboardingRefused, ui.Pluralize(onboardingRefused, "entry", "entries"), + ui.Pluralize(onboardingRefused, "it", "them")) } - if len(investigated) > 0 || len(unresolvedEntries) > 0 { + if len(investigated) > 0 || onboardingRefused > 0 || len(unresolvedEntries) > 0 { return errSilent } return nil diff --git a/cmd/gh-actions-pin/root.go b/cmd/gh-actions-pin/root.go index 0ba89b7b..3b556709 100644 --- a/cmd/gh-actions-pin/root.go +++ b/cmd/gh-actions-pin/root.go @@ -12,6 +12,7 @@ import ( "github.com/MakeNowJust/heredoc" "github.com/cli/go-gh/v2/pkg/repository" + parserlock "github.com/github/actions-lockfile/go/pkg/lockfile" "github.com/github/gh-actions-pin/internal/lockfile" "github.com/github/gh-actions-pin/internal/pinpool" "github.com/github/gh-actions-pin/internal/resolve" @@ -113,6 +114,12 @@ $ gh actions-pin --no-fix --json=valid,findings } bindCheckFlags(cmd, opts) + // --no-onboard and --no-interactive are persistent so they apply to the + // root check invocation. --no-onboard refuses to onboard new workflows or + // actions (re-pinning already-tracked entries still happens); used by + // Dependabot so a relock never silently adds an entry it didn't ask for. + cmd.PersistentFlags().Bool("no-onboard", false, "Refuse to onboard new workflows or actions; only re-pin already-tracked entries") + cmd.PersistentFlags().Bool("no-interactive", false, "Run without interactive prompts") cmd.AddCommand(newCheckCmd(newResolver)) return cmd @@ -123,7 +130,7 @@ $ gh actions-pin --no-fix --json=valid,findings // resolved hostname, open the lockfile store against it, and seed branch hints // from the existing lockfile so repeat scans short-circuit the per-branch // Compare walk. newResolver is the DI seam; pass nil for production wiring. -func newRun(workflowPaths []string, hostname string, pool *pinpool.Pool, newResolver resolverFunc) ([]string, *resolve.Resolver, *lockfile.State, error) { +func newRun(workflowPaths []string, hostname string, pool *pinpool.Pool, newResolver resolverFunc, onCorrupt lockRecovery) ([]string, *resolve.Resolver, *lockfile.State, error) { workflowsDir := os.Getenv("GH_ACTIONS_PIN_WORKFLOWS_DIR") paths, err := discoverWorkflowPaths(workflowPaths, workflowsDir) if err != nil { @@ -147,7 +154,23 @@ func newRun(workflowPaths []string, hostname string, pool *pinpool.Pool, newReso store, err = lockfile.LoadState(".", r) } if err != nil { - return nil, nil, nil, fmt.Errorf("opening lockfile: %w", err) + // An unreadable (non-future-version) lockfile is never silently + // discarded. Recovery policy may delete-and-recreate (interactive + // fix mode) or fail (CI, read-only, relock); either way the choice + // is explicit and surfaces to the user. + if errors.Is(err, lockfile.ErrCorruptLockfile) && onCorrupt != nil { + lockPath := filepath.Join(".", parserlock.Path) + recovered, rerr := onCorrupt(lockPath, err) + if rerr != nil { + return nil, nil, nil, rerr + } + if recovered { + store, err = lockfile.LoadState(".", r) + } + } + if err != nil { + return nil, nil, nil, fmt.Errorf("opening lockfile: %w", err) + } } r.SeedBranchHints(store.AllDeps()) diff --git a/go.mod b/go.mod index 4c9f59bf..4eb9730a 100644 --- a/go.mod +++ b/go.mod @@ -15,6 +15,20 @@ require ( require github.com/github/actions-lockfile/go v0.0.1 +require ( + github.com/AlecAivazis/survey/v2 v2.3.7 // indirect + github.com/charmbracelet/colorprofile v0.2.3-0.20250311203215-f60798e515dc // indirect + github.com/charmbracelet/lipgloss v1.1.1-0.20250319133953-166f707985bc // indirect + github.com/charmbracelet/x/ansi v0.8.0 // indirect + github.com/charmbracelet/x/cellbuf v0.0.13 // indirect + github.com/charmbracelet/x/term v0.2.1 // indirect + github.com/kballard/go-shellquote v0.0.0-20180428030007-95032a82bc51 // indirect + github.com/mattn/go-runewidth v0.0.16 // indirect + github.com/mgutz/ansi v0.0.0-20200706080929-d51e80ef957d // indirect + github.com/muesli/reflow v0.3.0 // indirect + github.com/xo/terminfo v0.0.0-20220910002029-abceb7e1c41e // indirect +) + require ( github.com/aymanbagabas/go-osc52/v2 v2.0.1 // indirect github.com/cli/safeexec v1.0.0 // indirect diff --git a/go.sum b/go.sum index f80b7df3..254f70d0 100644 --- a/go.sum +++ b/go.sum @@ -1,9 +1,23 @@ +github.com/AlecAivazis/survey/v2 v2.3.7 h1:6I/u8FvytdGsgonrYsVn2t8t4QiRnh6QSTqkkhIiSjQ= +github.com/AlecAivazis/survey/v2 v2.3.7/go.mod h1:xUTIdE4KCOIjsBAE1JYsUPoCqYdZ1reCfTwbto0Fduo= github.com/MakeNowJust/heredoc v1.0.0 h1:cXCdzVdstXyiTqTvfqk9SDHpKNjxuom+DOlyEeQ4pzQ= github.com/MakeNowJust/heredoc v1.0.0/go.mod h1:mG5amYoWBHf8vpLOuehzbGGw0EHxpZZ6lCpQ4fNJ8LE= +github.com/Netflix/go-expect v0.0.0-20220104043353-73e0943537d2 h1:+vx7roKuyA63nhn5WAunQHLTznkw5W8b1Xc0dNjp83s= +github.com/Netflix/go-expect v0.0.0-20220104043353-73e0943537d2/go.mod h1:HBCaDeC1lPdgDeDbhX8XFpy1jqjK0IBG8W5K+xYqA0w= github.com/aymanbagabas/go-osc52/v2 v2.0.1 h1:HwpRHbFMcZLEVr42D4p7XBqjyuxQH5SMiErDT4WkJ2k= github.com/aymanbagabas/go-osc52/v2 v2.0.1/go.mod h1:uYgXzlJ7ZpABp8OJ+exZzJJhRNQ2ASbcXHWsFqH8hp8= github.com/briandowns/spinner v1.23.2 h1:Zc6ecUnI+YzLmJniCfDNaMbW0Wid1d5+qcTq4L2FW8w= github.com/briandowns/spinner v1.23.2/go.mod h1:LaZeM4wm2Ywy6vO571mvhQNRcWfRUnXOs0RcKV0wYKM= +github.com/charmbracelet/colorprofile v0.2.3-0.20250311203215-f60798e515dc h1:4pZI35227imm7yK2bGPcfpFEmuY1gc2YSTShr4iJBfs= +github.com/charmbracelet/colorprofile v0.2.3-0.20250311203215-f60798e515dc/go.mod h1:X4/0JoqgTIPSFcRA/P6INZzIuyqdFY5rm8tb41s9okk= +github.com/charmbracelet/lipgloss v1.1.1-0.20250319133953-166f707985bc h1:nFRtCfZu/zkltd2lsLUPlVNv3ej/Atod9hcdbRZtlys= +github.com/charmbracelet/lipgloss v1.1.1-0.20250319133953-166f707985bc/go.mod h1:aKC/t2arECF6rNOnaKaVU6y4t4ZeHQzqfxedE/VkVhA= +github.com/charmbracelet/x/ansi v0.8.0 h1:9GTq3xq9caJW8ZrBTe0LIe2fvfLR/bYXKTx2llXn7xE= +github.com/charmbracelet/x/ansi v0.8.0/go.mod h1:wdYl/ONOLHLIVmQaxbIYEC/cRKOQyjTkowiI4blgS9Q= +github.com/charmbracelet/x/cellbuf v0.0.13 h1:/KBBKHuVRbq1lYx5BzEHBAFBP8VcQzJejZ/IA3iR28k= +github.com/charmbracelet/x/cellbuf v0.0.13/go.mod h1:xe0nKWGd3eJgtqZRaN9RjMtK7xUYchjzPr7q6kcvCCs= +github.com/charmbracelet/x/term v0.2.1 h1:AQeHeLZ1OqSXhrAWpYUtZyX1T3zVxfpZuEQMIQaGIAQ= +github.com/charmbracelet/x/term v0.2.1/go.mod h1:oQ4enTYFV7QN4m0i9mzHrViD7TQKvNEEkHUMCmsxdUg= github.com/cli/go-gh/v2 v2.13.0 h1:jEHZu/VPVoIJkciK3pzZd3rbT8J90swsK5Ui4ewH1ys= github.com/cli/go-gh/v2 v2.13.0/go.mod h1:Us/NbQ8VNM0fdaILgoXSz6PKkV5PWaEzkJdc9vR2geM= github.com/cli/safeexec v1.0.0 h1:0VngyaIyqACHdcMNWfo6+KdUYnqEr2Sg+bSP1pdF+dI= @@ -12,6 +26,9 @@ github.com/cli/shurcooL-graphql v0.0.4 h1:6MogPnQJLjKkaXPyGqPRXOI2qCsQdqNfUY1QSJ github.com/cli/shurcooL-graphql v0.0.4/go.mod h1:3waN4u02FiZivIV+p1y4d0Jo1jc6BViMA73C+sZo2fk= github.com/cpuguy83/go-md2man/v2 v2.0.6/go.mod h1:oOW0eioCTA6cOiMLiUPZOpcVxMig6NIQQ7OS05n1F4g= github.com/creack/pty v1.1.9/go.mod h1:oKZEueFk5CKHvIhNR5MUki03XCEU+Q6VDXinZuGJ33E= +github.com/creack/pty v1.1.17 h1:QeVUsEDNrLBW4tMgZHvxy18sKtr6VI492kBhUfhDJNI= +github.com/creack/pty v1.1.17/go.mod h1:MOBLtS5ELjhRRrroQr9kyvTxUAFNvYEK993ew/Vr4O4= +github.com/davecgh/go-spew v1.1.0/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= github.com/davecgh/go-spew v1.1.1 h1:vj9j/u1bqnvCEfJOwUhtlOARqs3+rkHYY13jYWTU97c= github.com/davecgh/go-spew v1.1.1/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= github.com/fatih/color v1.7.0 h1:DkWD4oS2D8LGGgTQ6IvwJJXSL5Vp2ffcQg58nFV38Ys= @@ -22,24 +39,40 @@ github.com/h2non/parth v0.0.0-20190131123155-b4df798d6542 h1:2VTzZjLZBgl62/EtslC github.com/h2non/parth v0.0.0-20190131123155-b4df798d6542/go.mod h1:Ow0tF8D4Kplbc8s8sSb3V2oUCygFHVp8gC3Dn6U4MNI= github.com/henvic/httpretty v0.0.6 h1:JdzGzKZBajBfnvlMALXXMVQWxWMF/ofTy8C3/OSUTxs= github.com/henvic/httpretty v0.0.6/go.mod h1:X38wLjWXHkXT7r2+uK8LjCMne9rsuNaBLJ+5cU2/Pmo= +github.com/hinshun/vt10x v0.0.0-20220119200601-820417d04eec h1:qv2VnGeEQHchGaZ/u7lxST/RaJw+cv273q79D81Xbog= +github.com/hinshun/vt10x v0.0.0-20220119200601-820417d04eec/go.mod h1:Q48J4R4DvxnHolD5P8pOtXigYlRuPLGl6moFx3ulM68= github.com/inconshreveable/mousetrap v1.1.0 h1:wN+x4NVGpMsO7ErUn/mUI3vEoE6Jt13X2s0bqwp9tc8= github.com/inconshreveable/mousetrap v1.1.0/go.mod h1:vpF70FUmC8bwa3OWnCshd2FqLfsEA9PFc4w1p2J65bw= +github.com/kballard/go-shellquote v0.0.0-20180428030007-95032a82bc51 h1:Z9n2FFNUXsshfwJMBgNA0RU6/i7WVaAegv3PtuIHPMs= +github.com/kballard/go-shellquote v0.0.0-20180428030007-95032a82bc51/go.mod h1:CzGEWj7cYgsdH8dAjBGEr58BoE7ScuLd+fwFZ44+/x8= github.com/kr/pretty v0.3.1 h1:flRD4NNwYAUpkphVc1HcthR4KEIFJ65n8Mw5qdRn3LE= github.com/kr/pretty v0.3.1/go.mod h1:hoEshYVHaxMs3cyo3Yncou5ZscifuDolrwPKZanG3xk= github.com/kr/text v0.2.0 h1:5Nx0Ya0ZqY2ygV366QzturHI13Jq95ApcVaJBhpS+AY= github.com/kr/text v0.2.0/go.mod h1:eLer722TekiGuMkidMxC/pM04lWEeraHUUmBw8l2grE= github.com/lucasb-eyer/go-colorful v1.3.0 h1:2/yBRLdWBZKrf7gB40FoiKfAWYQ0lqNcbuQwVHXptag= github.com/lucasb-eyer/go-colorful v1.3.0/go.mod h1:R4dSotOR9KMtayYi1e77YzuveK+i7ruzyGqttikkLy0= +github.com/mattn/go-colorable v0.1.2/go.mod h1:U0ppj6V5qS13XJ6of8GYAs25YV2eR4EVcfRqFIhoBtE= github.com/mattn/go-colorable v0.1.13 h1:fFA4WZxdEF4tXPZVKMLwD8oUnCTTo08duU7wxecdEvA= github.com/mattn/go-colorable v0.1.13/go.mod h1:7S9/ev0klgBDR4GtXTXX8a3vIGJpMovkB8vQcUbaXHg= +github.com/mattn/go-isatty v0.0.8/go.mod h1:Iq45c/XA43vh69/j3iqttzPXn0bhXyGjM0Hdxcsrc5s= github.com/mattn/go-isatty v0.0.16/go.mod h1:kYGgaQfpe5nmfYZH+SKPsOc2e4SrIfOl2e/yFXSvRLM= github.com/mattn/go-isatty v0.0.20 h1:xfD0iDuEKnDkl03q4limB+vH+GxLEtL/jb4xVJSWWEY= github.com/mattn/go-isatty v0.0.20/go.mod h1:W+V8PltTTMOvKvAeJH7IuucS94S2C6jfK/D7dTCTo3Y= +github.com/mattn/go-runewidth v0.0.12/go.mod h1:RAqKPSqVFrSLVXbA8x7dzmKdmGzieGRCM46jaSJTDAk= +github.com/mattn/go-runewidth v0.0.16 h1:E5ScNMtiwvlvB5paMFdw9p4kSQzbXFikJ5SQO6TULQc= +github.com/mattn/go-runewidth v0.0.16/go.mod h1:Jdepj2loyihRzMpdS35Xk/zdY8IAYHsh153qUoGf23w= +github.com/mgutz/ansi v0.0.0-20170206155736-9520e82c474b/go.mod h1:01TrycV0kFyexm33Z7vhZRXopbI8J3TDReVlkTgMUxE= +github.com/mgutz/ansi v0.0.0-20200706080929-d51e80ef957d h1:5PJl274Y63IEHC+7izoQE9x6ikvDFZS2mDVS3drnohI= +github.com/mgutz/ansi v0.0.0-20200706080929-d51e80ef957d/go.mod h1:01TrycV0kFyexm33Z7vhZRXopbI8J3TDReVlkTgMUxE= +github.com/muesli/reflow v0.3.0 h1:IFsN6K9NfGtjeggFP+68I4chLZV2yIKsXJFNZ+eWh6s= +github.com/muesli/reflow v0.3.0/go.mod h1:pbwTDkVPibjO2kyvBQRBxTWEEGDGq0FlB1BIKtnHY/8= github.com/muesli/termenv v0.16.0 h1:S5AlUN9dENB57rsbnkPyfdGuWIlkmzJjbFf0Tf5FWUc= github.com/muesli/termenv v0.16.0/go.mod h1:ZRfOIKPFDYQoDFF4Olj7/QJbW60Ol/kL1pU3VfY/Cnk= github.com/pkg/diff v0.0.0-20210226163009-20ebb0f2a09e/go.mod h1:pJLUxLENpZxwdsKMEsNbx1VGcRFpLqf3715MtcvvzbA= github.com/pmezard/go-difflib v1.0.0 h1:4DBwDE0NGyQoBHbLQYPwSUPoCMWR5BEzIk/f1lZbAQM= github.com/pmezard/go-difflib v1.0.0/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4= +github.com/rivo/uniseg v0.1.0/go.mod h1:J6wj4VEh+S6ZtnVlnTBMWIodfgj8LQOQFoIToxlJtxc= +github.com/rivo/uniseg v0.2.0/go.mod h1:J6wj4VEh+S6ZtnVlnTBMWIodfgj8LQOQFoIToxlJtxc= github.com/rivo/uniseg v0.4.7 h1:WUdvkW8uEhrYfLC4ZzdpI2ztxP1I582+49Oc5Mq64VQ= github.com/rivo/uniseg v0.4.7/go.mod h1:FN3SvrM+Zdj16jyLfmOkMNblXMcoc8DfTHruCPUcx88= github.com/rogpeppe/go-internal v1.9.0 h1:73kH8U+JUqXU8lRuOHeVHaa/SZPifC7BkcraZVejAe8= @@ -49,25 +82,57 @@ github.com/spf13/cobra v1.10.1 h1:lJeBwCfmrnXthfAupyUTzJ/J4Nc1RsHC/mSRU2dll/s= github.com/spf13/cobra v1.10.1/go.mod h1:7SmJGaTHFVBY0jW4NXGluQoLvhqFQM+6XSKD+P4XaB0= github.com/spf13/pflag v1.0.9 h1:9exaQaMOCwffKiiiYk6/BndUBv+iRViNW+4lEMi0PvY= github.com/spf13/pflag v1.0.9/go.mod h1:McXfInJRrz4CZXVZOBLb0bTZqETkiAhM9Iw0y3An2Bg= +github.com/stretchr/objx v0.1.0/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+wExME= +github.com/stretchr/testify v1.6.1/go.mod h1:6Fq8oRcR53rry900zMqJjRRixrwX3KX962/h/Wwjteg= github.com/stretchr/testify v1.11.1 h1:7s2iGBzp5EwR7/aIZr8ao5+dra3wiQyKjjFuvgVKu7U= github.com/stretchr/testify v1.11.1/go.mod h1:wZwfW3scLgRK+23gO65QZefKpKQRnfz6sD981Nm4B6U= github.com/thlib/go-timezone-local v0.0.0-20210907160436-ef149e42d28e h1:BuzhfgfWQbX0dWzYzT1zsORLnHRv3bcRcsaUk0VmXA8= github.com/thlib/go-timezone-local v0.0.0-20210907160436-ef149e42d28e/go.mod h1:/Tnicc6m/lsJE0irFMA0LfIwTBo4QP7A8IfyIv4zZKI= +github.com/xo/terminfo v0.0.0-20220910002029-abceb7e1c41e h1:JVG44RsyaB9T2KIHavMF/ppJZNG9ZpyihvCd0w101no= +github.com/xo/terminfo v0.0.0-20220910002029-abceb7e1c41e/go.mod h1:RbqR21r5mrJuqunuUZ/Dhy/avygyECGrLceyNeo4LiM= +github.com/yuin/goldmark v1.4.13/go.mod h1:6yULJ656Px+3vBD8DxQVa3kxgyrAnzto9xy5taEt/CY= +golang.org/x/crypto v0.0.0-20190308221718-c2843e01d9a2/go.mod h1:djNgcEr1/C05ACkg1iLfiJU5Ep61QUkGW8qpdssI0+w= +golang.org/x/crypto v0.0.0-20210921155107-089bfa567519/go.mod h1:GvvjBRRGRdwPK5ydBHafDWAxML/pGHZbMvKqRZ5+Abc= +golang.org/x/exp v0.0.0-20220909182711-5c715a9e8561 h1:MDc5xs78ZrZr3HMQugiXOAkSZtfTpbJLDr/lwfgO53E= +golang.org/x/exp v0.0.0-20220909182711-5c715a9e8561/go.mod h1:cyybsKvd6eL0RnXn6p/Grxp8F5bW7iYuBgsNCOHpMYE= +golang.org/x/mod v0.6.0-dev.0.20220419223038-86c51ed26bb4/go.mod h1:jJ57K6gSWd91VN4djpZkiMVwK6gcyfeH4XE8wZrZaV4= +golang.org/x/net v0.0.0-20190620200207-3b0461eec859/go.mod h1:z5CRVTTTmAJ677TzLLGU+0bjPO0LkuOLi4/5GtJWs/s= +golang.org/x/net v0.0.0-20210226172049-e18ecbb05110/go.mod h1:m0MpNAwzfU5UDzcl9v0D8zg8gWTRqZa9RBIspLL5mdg= +golang.org/x/net v0.0.0-20220722155237-a158d28d115b/go.mod h1:XRhObCWvk6IyKnWLug+ECip1KBveYUHfp+8e9klMJ9c= +golang.org/x/sync v0.0.0-20190423024810-112230192c58/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM= +golang.org/x/sync v0.0.0-20220722155255-886fb9371eb4/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM= golang.org/x/sync v0.19.0 h1:vV+1eWNmZ5geRlYjzm2adRgW2/mcpevXNg50YZtPCE4= golang.org/x/sync v0.19.0/go.mod h1:9KTHXmSnoGruLpwFjVSX0lNNA75CykiMECbovNTZqGI= +golang.org/x/sys v0.0.0-20190215142949-d0b11bdaac8a/go.mod h1:STP8DvDyc/dI5b8T5hshtkjS+E42TnysNCUPdjciGhY= +golang.org/x/sys v0.0.0-20190222072716-a9d3bda3a223/go.mod h1:STP8DvDyc/dI5b8T5hshtkjS+E42TnysNCUPdjciGhY= +golang.org/x/sys v0.0.0-20201119102817-f84b799fce68/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs= +golang.org/x/sys v0.0.0-20210615035016-665e8c7367d1/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= golang.org/x/sys v0.0.0-20210831042530-f4d43177bf5e/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= +golang.org/x/sys v0.0.0-20220520151302-bc2c85ada10a/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= +golang.org/x/sys v0.0.0-20220722155257-8c9f86f7a55f/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= golang.org/x/sys v0.0.0-20220811171246-fbc7d0a398ab/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= golang.org/x/sys v0.6.0/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= golang.org/x/sys v0.42.0 h1:omrd2nAlyT5ESRdCLYdm3+fMfNFE/+Rf4bDIQImRJeo= golang.org/x/sys v0.42.0/go.mod h1:4GL1E5IUh+htKOUEOaiffhrAeqysfVGipDYzABqnCmw= +golang.org/x/term v0.0.0-20201126162022-7de9c90e9dd1/go.mod h1:bj7SfCRtBDWHUb9snDiAeCFNEtKQo2Wmx5Cou7ajbmo= +golang.org/x/term v0.0.0-20210927222741-03fcf44c2211/go.mod h1:jbD1KX2456YbFQfuXm/mYQcufACuNUgVhRMnK/tPxf8= golang.org/x/term v0.30.0 h1:PQ39fJZ+mfadBm0y5WlL4vlM7Sx1Hgf13sMIY2+QS9Y= golang.org/x/term v0.30.0/go.mod h1:NYYFdzHoI5wRh/h5tDMdMqCqPJZEuNqVR5xJLd/n67g= +golang.org/x/text v0.3.0/go.mod h1:NqM8EUOU14njkJ3fqMW+pc6Ldnwhi/IjpwHt7yyuwOQ= +golang.org/x/text v0.3.3/go.mod h1:5Zoc/QRtKVWzQhOtBMvqHzDpF6irO9z98xDceosuGiQ= +golang.org/x/text v0.3.7/go.mod h1:u+2+/6zg+i71rQMx5EYifcz6MCKuco9NR6JIITiCfzQ= +golang.org/x/text v0.4.0/go.mod h1:mrYo+phRRbMaCq/xk9113O4dZlRixOauAjOtrjsXDZ8= golang.org/x/text v0.23.0 h1:D71I7dUrlY+VX0gQShAThNGHFxZ13dGLBHQLVl1mJlY= golang.org/x/text v0.23.0/go.mod h1:/BLNzu4aZCJ1+kcD0DNRotWKage4q2rGVAg4o22unh4= +golang.org/x/tools v0.0.0-20180917221912-90fa682c2a6e/go.mod h1:n7NCudcB/nEzxVGmLbDWY5pfWTLqBcC2KZ6jyYvM4mQ= +golang.org/x/tools v0.0.0-20191119224855-298f0cb1881e/go.mod h1:b+2E5dAYhXwXZwtnZ6UAqBI28+e2cm9otk0dWdXHAEo= +golang.org/x/tools v0.1.12/go.mod h1:hNGJHUnrk76NpqgfD5Aqm5Crs+Hm0VOH/i9J2+nxYbc= +golang.org/x/xerrors v0.0.0-20190717185122-a985d3407aa7/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0= gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0= gopkg.in/check.v1 v1.0.0-20180628173108-788fd7840127 h1:qIbj1fsPNlZgppZ+VLlY7N33q108Sa+fhmuc+sWQYwY= gopkg.in/check.v1 v1.0.0-20180628173108-788fd7840127/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0= gopkg.in/h2non/gock.v1 v1.1.2 h1:jBbHXgGBK/AoPVfJh5x4r/WxIrElvbLel8TCZkkZJoY= gopkg.in/h2non/gock.v1 v1.1.2/go.mod h1:n7UGz/ckNChHiK05rDoiC4MYSunEC/lyaUm2WWaDva0= +gopkg.in/yaml.v3 v3.0.0-20200313102051-9f266ea9e77c/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM= gopkg.in/yaml.v3 v3.0.1 h1:fxVm/GzAzEWqLHuvctI91KS9hhNmmWOoWu0XTYJS7CA= gopkg.in/yaml.v3 v3.0.1/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM= diff --git a/internal/lockfile/state.go b/internal/lockfile/state.go index f7f00d54..c75ae93d 100644 --- a/internal/lockfile/state.go +++ b/internal/lockfile/state.go @@ -45,8 +45,17 @@ type State struct { idSF singleflight.Group } +// ErrCorruptLockfile reports that a lockfile exists on disk but cannot be +// parsed (malformed YAML, unknown fields, or a dependency entry missing a +// required key). It is distinct from a missing lockfile (legitimately empty) +// and from a future-version lockfile (ErrFutureVersion). Callers decide +// recovery policy: prompt to delete and recreate, or fail loudly. Loading +// must never silently discard an unreadable lockfile and overwrite it. +var ErrCorruptLockfile = errors.New("lockfile is unreadable") + // LoadState reads the lockfile at repoRoot, returning an empty in-memory file -// when none exists on disk. +// when none exists on disk. A lockfile that exists but cannot be parsed is +// surfaced as ErrCorruptLockfile rather than being silently treated as empty. func LoadState(repoRoot string, meta MetadataResolver) (*State, error) { return LoadStateAt(filepath.Join(repoRoot, parserlock.Path), meta) } @@ -64,9 +73,7 @@ func LoadStateAt(lockfilePath string, meta MetadataResolver) (*State, error) { if err != nil { // A future-version lockfile (written by a newer binary) must // surface to the user — silently overwriting it would destroy - // pins this binary cannot interpret. Other parse failures - // (corrupt YAML, unknown fields) are treated as empty so a - // recoverable lockfile can be rewritten. + // pins this binary cannot interpret. // // The standalone parser emits a tool-agnostic hint ("upgrade the // tool that reads this lockfile"), so name the concrete command @@ -74,8 +81,11 @@ func LoadStateAt(lockfilePath string, meta MetadataResolver) (*State, error) { if errors.Is(err, parserlock.ErrFutureVersion) { return nil, fmt.Errorf("reading %s: %w; run `gh extension upgrade gh-actions-pin` to update", parserlock.Path, err) } - // Corrupt or unrecognized lockfile — treat as empty and overwrite. - file = parserlock.File{Version: parserlock.Version} + // Any other parse failure (corrupt YAML, unknown fields, a + // dependency entry missing a required key) is surfaced, not + // swallowed. Treating it as empty here would silently discard + // the user's pins and overwrite them on the next save. + return nil, fmt.Errorf("%w: %s: %v", ErrCorruptLockfile, parserlock.Path, err) } case errors.Is(err, os.ErrNotExist): file = parserlock.File{Version: parserlock.Version} @@ -250,15 +260,19 @@ func (s *State) Set(ctx context.Context, workflowKey string, deps []dep.Dependen // keyToPin: Dependency.Key() (NWO@Ref) → canonical pin (NWO@Ref:algo-hex). keyToPin := make(map[string]string, len(deps)) for _, d := range deps { - if d.Branch == "" { - return fmt.Errorf("%s@%s: branch is required in lockfile metadata; run `gh actions-pin` to populate it", d.NWO, d.Ref) - } pin, err := depToPin(d) if err != nil { return err } pin = pin.Canonical() pinKey := pin.String() + // The read path (parserlock.Pin) drops branch, so an unchanged carried + // dep arrives branchless; reuse the recorded branch. Only a new pin errors. + if d.Branch == "" { + if existing, ok := s.file.Dependencies[pinKey]; !ok || existing.Branch == "" { + return fmt.Errorf("%s@%s: branch is required in lockfile metadata; run `gh actions-pin` to populate it", d.NWO, d.Ref) + } + } keyToPin[d.Key()] = pinKey var isDirect bool if directKeys != nil { @@ -319,9 +333,20 @@ func (s *State) Set(ctx context.Context, workflowKey string, deps []dep.Dependen } sort.Strings(uses) } + // Preserve branch/tag from the existing entry for an unchanged branchless + // pin; a pin carrying its own branch (fresh/changed resolution) overrides. + branch, tag := d.Branch, d.Tag + if existing, ok := s.file.Dependencies[pinKey]; ok { + if branch == "" { + branch = existing.Branch + } + if tag == "" { + tag = existing.Tag + } + } s.file.Dependencies[pinKey] = parserlock.Action{ - Tag: d.Tag, - Branch: d.Branch, + Tag: tag, + Branch: branch, Commit: pin.Algo + "-" + pin.Hex, OwnerID: ids[0], RepoID: ids[1], diff --git a/internal/lockfile/state_test.go b/internal/lockfile/state_test.go index 7a559b12..f2f1f9ed 100644 --- a/internal/lockfile/state_test.go +++ b/internal/lockfile/state_test.go @@ -149,6 +149,70 @@ func TestState_SetRejectsEmptyBranch(t *testing.T) { } } +// TestState_SetPreservesBranchForUnchangedPin reproduces the write-path bug +// where adding a new action to an already-tracked workflow failed with +// "branch is required". The lockfile read path (parserlock.Pin) drops branch, +// so a carried Verified dep arrives at Set branchless; Set must fall back to +// the branch already recorded on disk for that unchanged pin instead of +// rejecting the whole write. +func TestState_SetPreservesBranchForUnchangedPin(t *testing.T) { + dir := t.TempDir() + wfKey := workflowfile.KeyFromPath(filepath.Join(dir, ".github", "workflows", "ci.yml")) + + store, err := LoadState(dir, fakeMetadataResolver{}) + if err != nil { + t.Fatalf("opening store: %v", err) + } + checkout := dep.Dependency{ + NWO: "actions/checkout", Ref: "v4", Tag: "v4", Branch: "main", + SHA: "abc123abc123abc123abc123abc123abc123abc1", HashAlgo: "sha1", + } + if err := store.Set(context.Background(), wfKey, []dep.Dependency{checkout}, nil, nil); err != nil { + t.Fatalf("initial Set: %v", err) + } + if err := store.Save(); err != nil { + t.Fatalf("initial Save: %v", err) + } + + // Reload from disk and re-Set with the existing checkout dep arriving + // branchless (the Verified read-path shape) plus a genuinely new dep. + store2, err := LoadState(dir, fakeMetadataResolver{}) + if err != nil { + t.Fatalf("reopening store: %v", err) + } + carriedCheckout := dep.Dependency{ + NWO: "actions/checkout", Ref: "v4", + SHA: "abc123abc123abc123abc123abc123abc123abc1", HashAlgo: "sha1", + // Branch/Tag intentionally empty — dropped by the read path. + } + newSetupGo := dep.Dependency{ + NWO: "actions/setup-go", Ref: "v5", Tag: "v5", Branch: "main", + SHA: "def456def456def456def456def456def456def4", HashAlgo: "sha1", + } + if err := store2.Set(context.Background(), wfKey, []dep.Dependency{carriedCheckout, newSetupGo}, nil, nil); err != nil { + t.Fatalf("re-Set with carried branchless dep should succeed, got: %v", err) + } + if err := store2.Save(); err != nil { + t.Fatalf("second Save: %v", err) + } + + store3, err := LoadState(dir, fakeMetadataResolver{}) + if err != nil { + t.Fatalf("reopening store: %v", err) + } + checkoutKey := "actions/checkout@v4:sha1-abc123abc123abc123abc123abc123abc123abc1" + a, ok := store3.file.Dependencies[checkoutKey] + if !ok { + t.Fatalf("expected %s preserved, keys=%v", checkoutKey, actionKeys(store3.file.Dependencies)) + } + if a.Branch != "main" { + t.Errorf("expected preserved Branch=main for unchanged pin, got %q", a.Branch) + } + if a.Tag != "v4" { + t.Errorf("expected preserved Tag=v4 for unchanged pin, got %q", a.Tag) + } +} + // TestState_DiamondTransitiveDepEmittedCorrectly verifies that when two direct // actions share a transitive dependency (diamond pattern: A→C, B→C), the // lockfile correctly records `uses: [C]` on both A and B, and the shared dep @@ -400,3 +464,332 @@ func TestState_RefusesFutureVersionLockfile(t *testing.T) { t.Errorf("lockfile was overwritten: %s", got) } } + +func TestState_CorruptLockfileSurfacesError(t *testing.T) { + // A lockfile that exists but can't be parsed (here: a dependency entry + // missing the required owner_id/repo_id keys) must surface as + // ErrCorruptLockfile, not be silently treated as empty and overwritten. + dir := t.TempDir() + if err := os.MkdirAll(filepath.Join(dir, ".github", "workflows"), 0o755); err != nil { + t.Fatal(err) + } + lockPath := filepath.Join(dir, parserlock.Path) + // branch + commit present, but owner_id/repo_id absent → whole-file reject. + body := "version: 'v0.0.1'\ndependencies:\n" + + " 'actions/checkout@v4:sha1-1111111111111111111111111111111111111111':\n" + + " branch: 'main'\n" + + " commit: 'sha1-1111111111111111111111111111111111111111'\n" + if err := os.WriteFile(lockPath, []byte(body), 0o644); err != nil { + t.Fatal(err) + } + + _, err := LoadState(dir, fakeMetadataResolver{}) + if err == nil { + t.Fatal("expected error opening corrupt lockfile, got nil") + } + if !errors.Is(err, ErrCorruptLockfile) { + t.Errorf("error does not match ErrCorruptLockfile sentinel: %v", err) + } + if errors.Is(err, parserlock.ErrFutureVersion) { + t.Errorf("corrupt lockfile must not be classified as future-version: %v", err) + } + + // LoadState must not delete or rewrite the file; recovery is the caller's job. + if _, err := os.Stat(lockPath); err != nil { + t.Errorf("lockfile must remain on disk after a corrupt-load error: %v", err) + } +} +func setupClosure(t *testing.T, dir string) { + t.Helper() + if err := os.MkdirAll(filepath.Join(dir, ".github", "workflows"), 0o755); err != nil { + t.Fatal(err) + } + store, err := LoadState(dir, fakeMetadataResolver{}) + if err != nil { + t.Fatalf("opening store: %v", err) + } + deps := []dep.Dependency{ + {NWO: "actions/setup-go", Ref: "v6", Branch: "main", SHA: "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", HashAlgo: "sha1"}, + {NWO: "actions/cache", Ref: "v4", Branch: "main", SHA: "bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb", HashAlgo: "sha1"}, + } + parentMap := map[string][]string{"actions/cache@v4": {"actions/setup-go@v6"}} + directKeys := map[string]bool{"actions/setup-go@v6": true} + wfKey := workflowfile.KeyFromPath(filepath.Join(dir, ".github", "workflows", "ci.yml")) + if err := store.Set(context.Background(), wfKey, deps, parentMap, directKeys); err != nil { + t.Fatalf("Set: %v", err) + } + if err := store.Save(); err != nil { + t.Fatalf("Save: %v", err) + } +} + +// resaveBumped reloads the store from disk (as `update` does), replaces the +// given workflow's closure, and saves — returning the new on-disk bytes. +func resaveBumped(t *testing.T, dir, wfKey string, deps []dep.Dependency, pm map[string][]string, direct map[string]bool) []byte { + t.Helper() + store, err := LoadState(dir, fakeMetadataResolver{}) + if err != nil { + t.Fatalf("reopening store: %v", err) + } + if err := store.Set(context.Background(), wfKey, deps, pm, direct); err != nil { + t.Fatalf("Set %s: %v", wfKey, err) + } + if err := store.Save(); err != nil { + t.Fatalf("Save: %v", err) + } + raw, err := os.ReadFile(filepath.Join(dir, parserlock.Path)) + if err != nil { + t.Fatalf("reading lockfile: %v", err) + } + return raw +} + +func lineSet(b []byte) map[string]bool { + m := map[string]bool{} + for _, l := range strings.Split(string(b), "\n") { + m[l] = true + } + return m +} + +// TestState_BumpYieldsMinimalDiff is the writer-side guarantee behind the +// consumer's diff-hygiene requirement: bumping ONE dependency in an +// already-canonical lockfile must leave every untouched entry byte-identical. +// Only the changed dep's lines (and its workflow direct-list line) may move. +func TestState_BumpYieldsMinimalDiff(t *testing.T) { + dir := t.TempDir() + if err := os.MkdirAll(filepath.Join(dir, ".github", "workflows"), 0o755); err != nil { + t.Fatal(err) + } + + base := []dep.Dependency{ + {NWO: "owner/a", Ref: "v1", SHA: "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", HashAlgo: "sha1", Branch: "main"}, + {NWO: "owner/b", Ref: "v2", SHA: "bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb", HashAlgo: "sha1", Branch: "main"}, + {NWO: "owner/c", Ref: "v3", SHA: "cccccccccccccccccccccccccccccccccccccccc", HashAlgo: "sha1", Branch: "main"}, + } + direct := map[string]bool{"owner/a@v1": true, "owner/b@v2": true, "owner/c@v3": true} + + store, err := LoadState(dir, fakeMetadataResolver{}) + if err != nil { + t.Fatalf("opening store: %v", err) + } + if err := store.Set(context.Background(), ".github/workflows/ci.yml", base, nil, direct); err != nil { + t.Fatalf("Set: %v", err) + } + if err := store.Save(); err != nil { + t.Fatalf("Save: %v", err) + } + before, err := os.ReadFile(filepath.Join(dir, parserlock.Path)) + if err != nil { + t.Fatalf("read: %v", err) + } + + bumped := []dep.Dependency{ + base[0], + {NWO: "owner/b", Ref: "v6", SHA: "9999999999999999999999999999999999999999", HashAlgo: "sha1", Branch: "main"}, + base[2], + } + bumpedDirect := map[string]bool{"owner/a@v1": true, "owner/b@v6": true, "owner/c@v3": true} + after := resaveBumped(t, dir, ".github/workflows/ci.yml", bumped, nil, bumpedDirect) + + beforeLines := lineSet(before) + for _, l := range strings.Split(string(after), "\n") { + if strings.Contains(l, "owner/a") || strings.Contains(l, "owner/c") || + strings.Contains(l, "aaaa") || strings.Contains(l, "cccc") { + if !beforeLines[l] { + t.Fatalf("untouched entry line changed: %q\n--- before ---\n%s\n--- after ---\n%s", l, before, after) + } + } + } + // Sanity: the bump actually landed. + if strings.Contains(string(after), "owner/b@v2") { + t.Fatalf("expected owner/b@v2 to be gone after bump, got:\n%s", after) + } + if !strings.Contains(string(after), "owner/b@v6:sha1-9999999999999999999999999999999999999999") { + t.Fatalf("expected bumped owner/b@v6 pin, got:\n%s", after) + } +} + +// TestState_BumpTransitiveRemoval covers the GC side of a relock: when a bump +// drops a transitive, its lock entry is reclaimed unless another workflow still +// reaches it. +func TestState_BumpTransitiveRemoval(t *testing.T) { + t.Run("orphaned transitive is removed", func(t *testing.T) { + dir := t.TempDir() + if err := os.MkdirAll(filepath.Join(dir, ".github", "workflows"), 0o755); err != nil { + t.Fatal(err) + } + store, _ := LoadState(dir, fakeMetadataResolver{}) + deps := []dep.Dependency{ + {NWO: "owner/a", Ref: "v1", SHA: "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", HashAlgo: "sha1", Branch: "main"}, + {NWO: "trans/x", Ref: "v1", SHA: "1111111111111111111111111111111111111111", HashAlgo: "sha1", Branch: "main"}, + } + pm := map[string][]string{"trans/x@v1": {"owner/a@v1"}} + direct := map[string]bool{"owner/a@v1": true} + if err := store.Set(context.Background(), ".github/workflows/ci.yml", deps, pm, direct); err != nil { + t.Fatal(err) + } + if err := store.Save(); err != nil { + t.Fatal(err) + } + + // Bump owner/a -> v2, no longer using trans/x. + bumped := []dep.Dependency{ + {NWO: "owner/a", Ref: "v2", SHA: "2222222222222222222222222222222222222222", HashAlgo: "sha1", Branch: "main"}, + } + after := resaveBumped(t, dir, ".github/workflows/ci.yml", bumped, nil, map[string]bool{"owner/a@v2": true}) + if strings.Contains(string(after), "trans/x@v1") { + t.Fatalf("expected orphaned transitive trans/x@v1 to be GC'd, got:\n%s", after) + } + if strings.Contains(string(after), "owner/a@v1:") { + t.Fatalf("expected old direct owner/a@v1 to be gone, got:\n%s", after) + } + }) + + t.Run("transitive still reachable from another workflow is preserved", func(t *testing.T) { + dir := t.TempDir() + if err := os.MkdirAll(filepath.Join(dir, ".github", "workflows"), 0o755); err != nil { + t.Fatal(err) + } + store, _ := LoadState(dir, fakeMetadataResolver{}) + shared := dep.Dependency{NWO: "shared/s", Ref: "v1", SHA: "5555555555555555555555555555555555555555", HashAlgo: "sha1", Branch: "main"} + ci := []dep.Dependency{ + {NWO: "owner/a", Ref: "v1", SHA: "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", HashAlgo: "sha1", Branch: "main"}, + shared, + } + rel := []dep.Dependency{ + {NWO: "owner/b", Ref: "v1", SHA: "bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb", HashAlgo: "sha1", Branch: "main"}, + shared, + } + if err := store.Set(context.Background(), ".github/workflows/ci.yml", ci, map[string][]string{"shared/s@v1": {"owner/a@v1"}}, map[string]bool{"owner/a@v1": true}); err != nil { + t.Fatal(err) + } + if err := store.Set(context.Background(), ".github/workflows/release.yml", rel, map[string][]string{"shared/s@v1": {"owner/b@v1"}}, map[string]bool{"owner/b@v1": true}); err != nil { + t.Fatal(err) + } + if err := store.Save(); err != nil { + t.Fatal(err) + } + before, _ := os.ReadFile(filepath.Join(dir, parserlock.Path)) + sharedPin := "shared/s@v1:sha1-5555555555555555555555555555555555555555" + if !strings.Contains(string(before), sharedPin) { + t.Fatalf("setup: expected shared pin present, got:\n%s", before) + } + + // Bump ONLY ci.yml's owner/a -> v2, dropping the shared transitive there. + bumped := []dep.Dependency{ + {NWO: "owner/a", Ref: "v2", SHA: "2222222222222222222222222222222222222222", HashAlgo: "sha1", Branch: "main"}, + } + after := resaveBumped(t, dir, ".github/workflows/ci.yml", bumped, nil, map[string]bool{"owner/a@v2": true}) + + if !strings.Contains(string(after), sharedPin) { + t.Fatalf("shared transitive still used by release.yml must be preserved, got:\n%s", after) + } + // And preserved byte-identically (the shared entry's lines didn't move). + beforeLines := lineSet(before) + for _, l := range strings.Split(string(after), "\n") { + if strings.Contains(l, "shared/s") || strings.Contains(l, "5555") || strings.Contains(l, "owner/b") || strings.Contains(l, "bbbb") { + if !beforeLines[l] { + t.Fatalf("untouched entry line changed: %q\n--- before ---\n%s\n--- after ---\n%s", l, before, after) + } + } + } + }) + + t.Run("dropped transitive subgraph is fully removed", func(t *testing.T) { + dir := t.TempDir() + if err := os.MkdirAll(filepath.Join(dir, ".github", "workflows"), 0o755); err != nil { + t.Fatal(err) + } + store, _ := LoadState(dir, fakeMetadataResolver{}) + deps := []dep.Dependency{ + {NWO: "owner/a", Ref: "v1", SHA: "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", HashAlgo: "sha1", Branch: "main"}, + {NWO: "mid/m", Ref: "v1", SHA: "3333333333333333333333333333333333333333", HashAlgo: "sha1", Branch: "main"}, + {NWO: "leaf/l", Ref: "v1", SHA: "4444444444444444444444444444444444444444", HashAlgo: "sha1", Branch: "main"}, + } + pm := map[string][]string{ + "mid/m@v1": {"owner/a@v1"}, + "leaf/l@v1": {"mid/m@v1"}, + } + if err := store.Set(context.Background(), ".github/workflows/ci.yml", deps, pm, map[string]bool{"owner/a@v1": true}); err != nil { + t.Fatal(err) + } + if err := store.Save(); err != nil { + t.Fatal(err) + } + + bumped := []dep.Dependency{ + {NWO: "owner/a", Ref: "v2", SHA: "2222222222222222222222222222222222222222", HashAlgo: "sha1", Branch: "main"}, + } + after := resaveBumped(t, dir, ".github/workflows/ci.yml", bumped, nil, map[string]bool{"owner/a@v2": true}) + if strings.Contains(string(after), "mid/m@v1") || strings.Contains(string(after), "leaf/l@v1") { + t.Fatalf("expected whole orphaned subgraph (mid/m, leaf/l) to be GC'd, got:\n%s", after) + } + }) +} + +// TestState_SaveFormatIsStable pins the exact canonical byte format of the +// writer. It guards the "heal" decision for `update`: diff-minimality relies on +// the on-disk lockfile already being canonical under THIS writer, so any change +// to the serialized format (sort order, quoting, field order, indentation, +// header) is a breaking change that would reformat every existing lockfile on +// its next write. If this fails intentionally, regenerate the golden and treat +// it as a format migration. +func TestState_SaveFormatIsStable(t *testing.T) { + const golden = "# This file is machine-generated by `gh actions-pin`.\n" + + "# Do not edit by hand; run `gh actions-pin` to update.\n" + + "# Docs: https://gh.io/actions-lockfile\n" + + "version: 'v0.0.1'\n" + + "workflows:\n" + + " '.github/workflows/ci.yml':\n" + + " - 'actions/checkout@v4:sha1-11111111111111111111111111111111111111aa'\n" + + " - 'actions/setup-go@v5:sha1-22222222222222222222222222222222222222bb'\n" + + "dependencies:\n" + + " 'actions/checkout@v4:sha1-11111111111111111111111111111111111111aa':\n" + + " tag: 'v4'\n" + + " branch: 'main'\n" + + " commit: 'sha1-11111111111111111111111111111111111111aa'\n" + + " owner_id: 1\n" + + " repo_id: 2\n" + + " uses:\n" + + " - 'shared/dep@v1:sha1-33333333333333333333333333333333333333cc'\n" + + " 'actions/setup-go@v5:sha1-22222222222222222222222222222222222222bb':\n" + + " branch: 'main'\n" + + " commit: 'sha1-22222222222222222222222222222222222222bb'\n" + + " owner_id: 1\n" + + " repo_id: 2\n" + + " 'shared/dep@v1:sha1-33333333333333333333333333333333333333cc':\n" + + " branch: 'main'\n" + + " commit: 'sha1-33333333333333333333333333333333333333cc'\n" + + " owner_id: 1\n" + + " repo_id: 2\n" + + dir := t.TempDir() + if err := os.MkdirAll(filepath.Join(dir, ".github", "workflows"), 0o755); err != nil { + t.Fatal(err) + } + store, err := LoadState(dir, fakeMetadataResolver{}) + if err != nil { + t.Fatalf("opening store: %v", err) + } + deps := []dep.Dependency{ + {NWO: "actions/checkout", Ref: "v4", SHA: "11111111111111111111111111111111111111aa", HashAlgo: "sha1", Tag: "v4", Branch: "main"}, + {NWO: "actions/setup-go", Ref: "v5", SHA: "22222222222222222222222222222222222222bb", HashAlgo: "sha1", Branch: "main"}, + {NWO: "shared/dep", Ref: "v1", SHA: "33333333333333333333333333333333333333cc", HashAlgo: "sha1", Branch: "main"}, + } + pm := map[string][]string{"shared/dep@v1": {"actions/checkout@v4"}} + direct := map[string]bool{"actions/checkout@v4": true, "actions/setup-go@v5": true} + if err := store.Set(context.Background(), ".github/workflows/ci.yml", deps, pm, direct); err != nil { + t.Fatalf("Set: %v", err) + } + if err := store.Save(); err != nil { + t.Fatalf("Save: %v", err) + } + raw, err := os.ReadFile(filepath.Join(dir, parserlock.Path)) + if err != nil { + t.Fatalf("read: %v", err) + } + if string(raw) != golden { + t.Fatalf("serialized lockfile format drifted from golden.\n--- got ---\n%s\n--- want ---\n%s", raw, golden) + } +} diff --git a/internal/pin/commit.go b/internal/pin/commit.go index 2e542066..9f3a95ed 100644 --- a/internal/pin/commit.go +++ b/internal/pin/commit.go @@ -5,9 +5,11 @@ import ( "fmt" "os" "runtime" + "strings" "github.com/github/gh-actions-pin/internal/dep" "github.com/github/gh-actions-pin/internal/lockfile" + "github.com/github/gh-actions-pin/internal/pipeline/checks" "github.com/github/gh-actions-pin/internal/workflowfile" "golang.org/x/sync/errgroup" ) @@ -64,6 +66,7 @@ func Commit(ctx context.Context, rec *Record, store *lockfile.State, copts *Comm wfKey := workflowfile.KeyFromPath(wfPath) parentMap := buildParentMap(rec, wfPath) directKeys := buildDirectKeys(rec, wfPath) + deps = retainImpostorPins(rec, store, wfPath, deps, directKeys) if err := store.Set(ctx, wfKey, deps, parentMap, directKeys); err != nil { return fmt.Errorf("updating lockfile for %s: %w", wfPath, err) } @@ -115,6 +118,42 @@ func groupPinnedByWorkflow(rec *Record) map[string][]dep.Dependency { return result } +// retainImpostorPins re-adds the workflow's existing on-disk pins for any +// impostor-flagged dep so a co-located re-pin never silently drops them. +func retainImpostorPins(rec *Record, store *lockfile.State, wfPath string, deps []dep.Dependency, directKeys map[string]bool) []dep.Dependency { + impostor := make(map[string]bool) + for _, e := range rec.Entries { + if e.Resolution != Investigate || e.Issue != string(checks.ImpostorCommit) { + continue + } + for _, wf := range e.Workflows { + if wf == wfPath { + impostor[strings.ToLower(e.NWO+"@"+e.Ref)] = true + } + } + } + if len(impostor) == 0 { + return deps + } + existing, err := store.Get(workflowfile.KeyFromPath(wfPath)) + if err != nil { + return deps + } + have := make(map[string]bool, len(deps)) + for _, d := range deps { + have[strings.ToLower(d.NWO+"@"+d.Ref)] = true + } + for _, d := range existing { + k := strings.ToLower(d.NWO + "@" + d.Ref) + if impostor[k] && !have[k] { + deps = append(deps, d) + directKeys[d.Key()] = true + have[k] = true + } + } + return deps +} + func buildParentMap(rec *Record, wfPath string) map[string][]string { pm := make(map[string][]string) for _, e := range rec.Entries { diff --git a/internal/pin/plan.go b/internal/pin/plan.go index ed1e0cf8..3818b998 100644 --- a/internal/pin/plan.go +++ b/internal/pin/plan.go @@ -113,14 +113,18 @@ func planWorkflow(ctx context.Context, wr checks.WorkflowReport, opts PlanOption var entries []Entry var wplans []WorkflowPlan + // Drop stale inventory entries so a re-pin converges: the orphan leaves + // workflows[path] and Save's GC removes its dependencies[] entry. + inventory := pruneStaleInventory(wr.Inventory, wr.Findings) + if !wr.NeedsAttention() { - entries = verifiedEntries(wr.Inventory, wr.Path) + entries = verifiedEntries(inventory, wr.Path) return planResult{entries: entries, wplans: wplans}, nil } // Per-dep trust: recorded deps skip the network path. - unrecordedRefs, inventorySHA := partitionByInventory(wr.Inventory, wr.ActionRefs) - entries = verifiedEntries(wr.Inventory, wr.Path) + unrecordedRefs, inventorySHA := partitionByInventory(inventory, wr.ActionRefs) + entries = verifiedEntries(inventory, wr.Path) if len(unrecordedRefs) == 0 { return planResult{entries: entries, wplans: wplans}, nil @@ -492,6 +496,31 @@ func partitionByInventory(inventory []checks.InventoryEntry, refs []parserlock.A return unrecorded, shaSeen } +// pruneStaleInventory drops inventory entries matching a stale finding (a pin +// the workflow no longer references), so a fix-mode re-pin converges. +func pruneStaleInventory(inventory []checks.InventoryEntry, findings []checks.Finding) []checks.InventoryEntry { + stale := make(map[string]bool) + for _, f := range findings { + if f.Category != checks.Stale || f.Dependency == nil { + continue + } + d := f.Dependency + stale[strings.ToLower(d.NWO+"@"+d.Ref+":"+d.SHA)] = true + } + if len(stale) == 0 { + return inventory + } + out := make([]checks.InventoryEntry, 0, len(inventory)) + for _, inv := range inventory { + key := strings.ToLower(inv.Dep.NWO + "@" + inv.Dep.Ref + ":" + inv.Dep.SHA) + if stale[key] { + continue + } + out = append(out, inv) + } + return out +} + // verifiedEntries builds Verified plan entries for every inventory item. func verifiedEntries(inventory []checks.InventoryEntry, path string) []Entry { out := make([]Entry, len(inventory)) diff --git a/internal/pin/retain_impostor_test.go b/internal/pin/retain_impostor_test.go new file mode 100644 index 00000000..407eb5c6 --- /dev/null +++ b/internal/pin/retain_impostor_test.go @@ -0,0 +1,91 @@ +package pin + +import ( + "context" + "os" + "path/filepath" + "testing" + + "github.com/github/gh-actions-pin/internal/dep" + "github.com/github/gh-actions-pin/internal/lockfile" + "github.com/github/gh-actions-pin/internal/pipeline/checks" + "github.com/github/gh-actions-pin/internal/workflowfile" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +type fakeMeta struct{} + +func (fakeMeta) RepoIDs(_ context.Context, _, _ string) (int64, int64, error) { + return 1, 2, nil +} + +// A co-located bump forces a workflow rewrite; the impostor pin already on +// disk must be retained, not silently dropped toward an empty pin list. +func TestRetainImpostorPins_keepsExistingPinOnColocatedRepin(t *testing.T) { + dir := t.TempDir() + require.NoError(t, os.MkdirAll(filepath.Join(dir, ".github", "workflows"), 0o755)) + wfPath := filepath.Join(dir, ".github", "workflows", "ci.yml") + wfKey := workflowfile.KeyFromPath(wfPath) + + store, err := lockfile.LoadState(dir, fakeMeta{}) + require.NoError(t, err) + + seed := []dep.Dependency{ + {NWO: "bad/impostor", Ref: "v1", Tag: "v1", Branch: "main", SHA: "1111111111111111111111111111111111111111", HashAlgo: "sha1"}, + {NWO: "actions/checkout", Ref: "v4", Tag: "v4", Branch: "main", SHA: "2222222222222222222222222222222222222222", HashAlgo: "sha1"}, + } + require.NoError(t, store.Set(context.Background(), wfKey, seed, nil, nil)) + require.NoError(t, store.Save()) + + // Re-pin: checkout bumps (Pinned), impostor flagged Investigate and dropped from deps. + rec := &Record{ + Entries: []Entry{ + {NWO: "actions/checkout", Ref: "v5", SHA: "3333333333333333333333333333333333333333", Resolution: Pinned, Direct: true, OnBranch: "main", Workflows: []string{wfPath}}, + {NWO: "bad/impostor", Ref: "v1", SHA: "1111111111111111111111111111111111111111", Resolution: Investigate, Issue: string(checks.ImpostorCommit), Workflows: []string{wfPath}}, + }, + } + deps := []dep.Dependency{ + {NWO: "actions/checkout", Ref: "v5", Branch: "main", SHA: "3333333333333333333333333333333333333333", HashAlgo: "sha1"}, + } + directKeys := map[string]bool{"actions/checkout@v5": true} + + got := retainImpostorPins(rec, store, wfPath, deps, directKeys) + + require.Len(t, got, 2, "impostor pin must be re-added alongside the bumped pin") + assert.True(t, directKeys["bad/impostor@v1"], "retained impostor pin must stay direct") + + // Drive the write as Commit does; the impostor pin must survive on disk. + require.NoError(t, store.Set(context.Background(), wfKey, got, buildParentMap(rec, wfPath), directKeys)) + require.NoError(t, store.Save()) + + after, err := store.Get(wfKey) + require.NoError(t, err) + names := map[string]bool{} + for _, d := range after { + names[d.NWO] = true + } + assert.True(t, names["bad/impostor"], "impostor pin must survive the re-pin write") + assert.True(t, names["actions/checkout"], "bumped pin must be written") + assert.Len(t, after, 2, "pin list must not shrink") +} + +// With no co-located new pin, the impostor's workflow is untouched and there +// is nothing to retain; the helper is a no-op on the deps it is handed. +func TestRetainImpostorPins_noopWithoutImpostorFinding(t *testing.T) { + dir := t.TempDir() + require.NoError(t, os.MkdirAll(filepath.Join(dir, ".github", "workflows"), 0o755)) + wfPath := filepath.Join(dir, ".github", "workflows", "ci.yml") + + store, err := lockfile.LoadState(dir, fakeMeta{}) + require.NoError(t, err) + + rec := &Record{Entries: []Entry{ + {NWO: "actions/checkout", Ref: "v5", Resolution: Pinned, Workflows: []string{wfPath}}, + }} + deps := []dep.Dependency{{NWO: "actions/checkout", Ref: "v5"}} + directKeys := map[string]bool{"actions/checkout@v5": true} + + got := retainImpostorPins(rec, store, wfPath, deps, directKeys) + assert.Len(t, got, 1) +} diff --git a/internal/pipeline/checks/category.go b/internal/pipeline/checks/category.go index 65031814..70041b35 100644 --- a/internal/pipeline/checks/category.go +++ b/internal/pipeline/checks/category.go @@ -52,11 +52,12 @@ const ( // Non-blocking diagnostic: surfaced so consumers can retry rather // than treating the dep as verified. ReachabilityUnknown Category = "reachability-unknown" - // OnboardingRequired means an `upgrade --no-onboard` run targeted - // a workflow that has no existing entry in `lockfile.workflows{}`. - // The CLI refuses to silently add it during a dependency-update - // run; the operator must onboard the workflow explicitly before - // re-running upgrade. + // OnboardingRequired means a `check --no-onboard` run encountered a + // workflow (or an action within one) that has no existing entry in the + // lockfile. Under --no-onboard the tool refuses to add new entries: the + // workflow/action is skipped and surfaced rather than silently pinned. + // Already-tracked entries are still re-pinned. The operator must onboard + // explicitly (run `gh actions-pin check` without --no-onboard) to add it. OnboardingRequired Category = "onboarding-required" // VersionRef is an informational nudge: a dependency is pinned with a // ref that is not a full semver tag (e.g. v4, v3.1, main). Full semver From e8e3e59ab51fee63d02f03d212aba54d7c75a858 Mon Sep 17 00:00:00 2001 From: Jeff Martin Date: Fri, 12 Jun 2026 10:20:04 -0500 Subject: [PATCH 05/13] scenarios: add narrowing, onboarding, and lockfile recovery cases MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 22 new scenarios across 3 categories: - narrowing (12): default v4→full, minor→patch, branch skip, --no-narrow keeps major/minor, sticky imprecise re-pin, nudge visibility, JSON - onboarding (8): --no-onboard new/tracked/mixed, JSON findings, terminal caution, flag composition with --no-narrow/--no-interactive - lockfile (2): corrupt + --no-fix fails, corrupt + --no-interactive fails --- test/integration/harness.rb | 725 ++++++++++++++++++++++++++++++++++-- test/integration/run.rb | 128 ++++++- test/scenarios/catalog.yml | 540 ++++++++++++++++++++++----- 3 files changed, 1267 insertions(+), 126 deletions(-) diff --git a/test/integration/harness.rb b/test/integration/harness.rb index df059719..17072f3a 100644 --- a/test/integration/harness.rb +++ b/test/integration/harness.rb @@ -140,8 +140,14 @@ def self.load # ── Scenario ──────────────────────────────────────────────────────── class Scenario - attr_reader :name, :failures - attr_accessor :category + attr_reader :name, :failures, :last_cmd, :tags, :last_diff + attr_accessor :category, :description, :expect_spec, :fixture_spec, :input_spec + + # Expose fixture state for display (read-only) + def cli_args; @args; end + def workflows; @workflows; end + def lockfile_content; @lockfile; end + def live_repo_nwo; @live_repo; end def initialize(name) @name = name @@ -156,6 +162,7 @@ def initialize(name) @live_repo = nil @needs_token = false @tags = [] + @input_spec = nil end # ── DSL: fixtures ────────────────────────────────────────────── @@ -170,6 +177,10 @@ def lockfile(content) self end + def onboarded? + !@lockfile.nil? + end + def env(hash) @env.merge!(hash) self @@ -260,6 +271,24 @@ def assert_lockfile_contains(*patterns) self end + def assert_lockfile_comment_matches(pattern) + @assertions << -> (r) { + lockpath = File.join(r.dir, ".github", "workflows", "actions.lock") + content = File.read(lockpath) rescue "" + assert_match("lockfile comment matches /#{pattern}/", content, Regexp.new(pattern)) + } + self + end + + def assert_lockfile_comment_excludes(pattern) + @assertions << -> (r) { + lockpath = File.join(r.dir, ".github", "workflows", "actions.lock") + content = File.read(lockpath) rescue "" + assert_no_match("lockfile comment excludes /#{pattern}/", content, Regexp.new(pattern)) + } + self + end + def assert_custom(&block) @assertions << block self @@ -358,8 +387,10 @@ def run(binary, profile_dir: nil) end ctx = prepare(binary, profile_dir: profile_dir) + @last_cmd = ctx.cmd_string begin result = ctx.run_captured + @last_diff = `cd #{Shellwords.shellescape(ctx.dir)} && git add -N . 2>/dev/null; git --no-pager diff --color 2>/dev/null`.strip @assertions.each { |a| a.call(result) } result ensure @@ -427,7 +458,11 @@ def run_captured # Run through a PTY so the binary sees a real terminal and emits # ANSI colors, icons, and spinners. Output streams live to $stdout. # Returns a PTYResult with the merged output. - def run_pty + # + # input_prompts: optional array of {prompt:, response:} hashes. + # When the accumulated output matches a prompt pattern, the + # corresponding response is written to the PTY's stdin. + def run_pty(input_prompts: nil) flat_env = @env.map { |k, v| "#{k}=#{Shellwords.shellescape(v)}" } shell_cmd = "cd #{Shellwords.shellescape(@dir)} && " + flat_env.join(" ") + " " + @@ -435,19 +470,47 @@ def run_pty combined = String.new exit_code = nil + prompts = (input_prompts || []).map { |p| p.dup } begin - PTY.spawn("/bin/bash", "-c", shell_cmd) do |reader, _writer, pid| + PTY.spawn("/bin/bash", "-c", shell_cmd) do |reader, writer, pid| begin - # Read in chunks so we flush once per available burst instead - # of once per byte. Eliminates flicker while keeping spinners - # responsive (readpartial returns as soon as data is available). prev_sync = $stdout.sync $stdout.sync = true loop do chunk = reader.readpartial(4096) $stdout.write chunk combined << chunk + + # Survey sends DSR (\e[6n]) to query cursor position. In a + # real terminal the emulator replies with a CPR; in a PTY no + # one does, so survey hangs in CursorLocation(). We play + # terminal emulator: when \e[6n appears, write a CPR back. + if chunk.include?("\e[6n") + writer.write("\e[1;1R") + end + + # Check for pending prompts and auto-respond. + # Survey runs in raw mode where Enter = \r, not \n. + prompts.reject! do |p| + if combined.gsub(/\e\[[0-9;]*[a-zA-Z]/, "").include?(p["prompt"]) + # Let survey finish rendering after CursorLocation. + loop do + ready = IO.select([reader], nil, nil, 0.3) + break unless ready + begin + extra = reader.readpartial(4096) + $stdout.write extra + combined << extra + writer.write("\e[1;1R") if extra.include?("\e[6n") + rescue Errno::EIO, EOFError + break + end + end + writer.write(p["response"] + "\r") + true + end + end end rescue Errno::EIO, EOFError # PTY closed — normal on macOS when process exits @@ -491,6 +554,12 @@ def initialize(binary: nil) @profile_dir = nil @pause = false @last_dir = nil + @diff_cache = {} # name → diff string + @diff_order = [] # insertion order for eviction + end + + def diff_cache_limit + [20, @scenarios.size].max end def scenario(name, &block) @@ -592,13 +661,13 @@ def shell active_ctx = nil scenario_names = @scenarios.map { |s| s.name.to_s } - commands = %w[list ls run test inspect diff cd rerun build pause profile auth status clear help quit exit q] + commands = %w[list ls run test review inspect diff cd rerun build pause profile auth status clear help quit exit q] # Tab completion: commands first, then scenario names for run/test/inspect/cd Reline.completion_proc = proc do |input| line = Reline.line_buffer parts = line.split(/\s+/, 2) - if parts.size >= 2 && %w[run test inspect cd].include?(parts[0]) + if parts.size >= 2 && %w[run test review inspect cd diff].include?(parts[0]) candidates = scenario_names + ["all"] candidates.select { |n| n.start_with?(input) } elsif parts.size <= 1 @@ -650,8 +719,24 @@ def shell if arg == "all" run_all_live - elsif arg && repo_nwo?(arg) - run_one_live(adhoc_scenario(arg)) + elsif arg && arg.start_with?("cat:") + cat = arg.sub("cat:", "") + to_run = @scenarios.select { |s| s.respond_to?(:category) && s.category == cat } + if to_run.empty? + puts "No scenarios in category '#{cat}'. Try \e[36mlist\e[0m." + else + to_run.each_with_index do |s, i| + run_one_live(s) + if @pause && i < to_run.size - 1 + print "\e[2m press Enter to continue (q to stop)…\e[0m " + input = $stdin.gets&.strip + break if input&.start_with?("q") + end + end + end + elsif arg && repo_nwo?(arg.split(/\s+--\s+/, 2)[0]) + nwo, extra = split_adhoc_args(arg) + run_one_live(adhoc_scenario(nwo, extra_args: extra)) else s = find_scenario(arg) next unless s @@ -659,9 +744,29 @@ def shell end when "test" - to_run = arg ? @scenarios.select { |s| s.name.to_s.include?(arg) } : @scenarios + if arg && arg.start_with?("cat:") + cat = arg.sub("cat:", "") + to_run = @scenarios.select { |s| s.respond_to?(:category) && s.category == cat } + else + to_run = arg ? @scenarios.select { |s| s.name.to_s.include?(arg) } : @scenarios + end run_batch(to_run) + when "review" + if arg && arg.start_with?("cat:") + cat = arg.sub("cat:", "") + to_run = @scenarios.select { |s| s.respond_to?(:category) && s.category == cat } + elsif arg == "all" || arg.nil? + to_run = @scenarios + else + to_run = @scenarios.select { |s| s.name.to_s.include?(arg) } + end + if to_run.empty? + puts "No scenarios to review." + else + run_review(to_run) + end + when "inspect" s = find_scenario(arg) next unless s @@ -706,8 +811,8 @@ def shell when "rerun" if active_ctx - puts "\e[1m── re-running #{active_ctx.scenario.name} ──\e[0m\n\n" - active_ctx.run_pty + puts "\e[1;36m── re-running #{active_ctx.scenario.name} ──\e[0m\n\n" + active_ctx.run_pty(input_prompts: active_ctx.scenario.input_spec) puts else puts "No active scenario. Use \e[36mrun \e[0m first." @@ -732,12 +837,7 @@ def shell puts "Pause between scenarios: #{@pause ? "\e[32mon\e[0m" : "\e[33moff\e[0m"}" when "diff" - dir = active_ctx&.dir || @last_dir - if dir && Dir.exist?(dir) - show_diff(dir) - else - puts "No scenario dir available. Run a scenario first." - end + show_paged_diff(arg) when "auth" show_auth @@ -774,7 +874,12 @@ def shell if repo_nwo?(verb) active_ctx&.teardown active_ctx = nil - run_one_live(adhoc_scenario(verb)) + extra = if arg && arg.match?(/\A--\s/) + arg.sub(/\A--\s+/, "").split(/\s+/) + else + [] + end + run_one_live(adhoc_scenario(verb, extra_args: extra)) else s = @scenarios.find { |sc| sc.name.to_s == verb } if s @@ -797,26 +902,121 @@ def shell private - def show_diff(dir) + MAX_DIFF_LINES = 30 + + def cache_diff(name, diff_text) + return if diff_text.nil? || diff_text.empty? + @diff_cache.delete(name) + @diff_order.delete(name) + @diff_cache[name] = diff_text + @diff_order << name + while @diff_order.size > diff_cache_limit + evict = @diff_order.shift + @diff_cache.delete(evict) + end + end + + def show_paged_diff(name) + diff = name ? @diff_cache[name] : @diff_cache[@diff_order.last] + if diff.nil? || diff.empty? + if name + puts "\e[2m no diff cached for '#{name}'\e[0m" + available = @diff_order.select { |n| !@diff_cache[n].empty? } + puts " \e[2mavailable: #{available.join(", ")}\e[0m" if available.any? + else + puts "\e[2m no diff available — run a scenario first\e[0m" + end + return + end + IO.popen(["less", "-R"], "w") { |io| io.write(diff) } + rescue Errno::EPIPE + # user quit pager early — that's fine + end + + def show_starting_state(dir, width) + inner = width - 2 + has_content = false + + # Show lockfile if present + lockfile = File.join(dir, ".github", "workflows", "actions.lock") + if File.exist?(lockfile) + content = File.read(lockfile) + # Strip the header comments, show the meat + lines = content.lines.reject { |l| l.start_with?("#") || l.strip.empty? } + # Show dep keys with their refs for quick understanding + deps = [] + lines.each do |l| + if l =~ /^\s+'?([^:]+@[^:]+):sha1-/ + deps << $1 + end + end + if deps.any? + puts "\e[1;35m── STARTING STATE #{"─" * (width - 19)}\e[0m" + has_content = true + puts " \e[2mlockfile deps:\e[0m" + deps.each { |d| puts " \e[33m#{d}\e[0m" } + end + end + + # Show workflow action refs + wf_dir = File.join(dir, ".github", "workflows") + if Dir.exist?(wf_dir) + wf_actions = {} + Dir.glob("#{wf_dir}/*.yml").each do |f| + next if File.basename(f) == "actions.lock" + File.readlines(f).each do |line| + if line =~ /uses:\s*(\S+@\S+)/ + (wf_actions[File.basename(f)] ||= []) << $1 + end + end + end + if wf_actions.any? + unless has_content + puts "\e[1;35m── STARTING STATE #{"─" * (width - 19)}\e[0m" + has_content = true + end + puts " \e[2mworkflow refs:\e[0m" + wf_actions.each do |file, actions| + actions.each { |a| puts " \e[36m#{file}\e[0m → #{a}" } + end + end + end + + puts if has_content + end + + def show_diff(dir, width, scenario_name: nil) return unless dir && Dir.exist?(dir) - diff = `cd #{Shellwords.shellescape(dir)} && git --no-pager diff --color 2>/dev/null`.strip + diff = `cd #{Shellwords.shellescape(dir)} && git add -N . 2>/dev/null; git --no-pager diff --color 2>/dev/null`.strip return if diff.empty? + lines = diff.lines puts - puts " \e[1m── diff ──\e[0m" - diff.each_line { |l| puts " #{l}" } + puts "\e[1;35m── DIFF #{"─" * (width - 9)}\e[0m" + if lines.size <= MAX_DIFF_LINES + lines.each { |l| puts " #{l}" } + else + lines.first(MAX_DIFF_LINES).each { |l| puts " #{l}" } + omitted = lines.size - MAX_DIFF_LINES + hint = scenario_name ? "diff #{scenario_name}" : "diff" + puts " \e[2m… #{omitted} more lines truncated\e[0m" + puts " \e[2m→ \e[0m\e[36m#{hint}\e[0m" + end end def print_help puts "Commands:" puts " \e[36mlist\e[0m Show all scenarios (grouped by category)" puts " \e[36mrun \e[0m Run scenario with live PTY output" + puts " \e[36mrun cat:\e[0m Run all scenarios in a category" puts " \e[36mrun \e[0m Run ad-hoc against any GitHub repo" puts " \e[36mrun all\e[0m Run all scenarios with live output" puts " \e[36m\e[0m Run scenario directly (shorthand for run)" puts " \e[36m\e[0m Run ad-hoc against any GitHub repo" puts " \e[36mtest [filter]\e[0m Batch-test scenarios (captured, assertions)" + puts " \e[36mreview [filter]\e[0m Interactive review: pass/flag/skip + report" puts " \e[36minspect \e[0m Show scenario fixtures without running" - puts " \e[36mdiff\e[0m Show git diff from last run" + puts " \e[36mdiff\e[0m Show full diff from last run (pager)" + puts " \e[36mdiff \e[0m Show cached diff for a specific scenario" puts " \e[36mcd \e[0m Prepare scenario and drop into its dir" puts " \e[36mrerun\e[0m Re-run active scenario" puts " \e[36mbuild\e[0m Rebuild the binary (go build)" @@ -873,6 +1073,10 @@ def find_scenario(name) puts "Usage: run " return nil end + # Prefer exact match, fall back to substring. + exact = @scenarios.find { |s| s.name.to_s == name } + return exact if exact + matches = @scenarios.select { |s| s.name.to_s.include?(name) } if matches.empty? puts "No scenario matching '#{name}'. Try \e[36mlist\e[0m." @@ -886,38 +1090,120 @@ def find_scenario(name) end # Build an ad-hoc live Scenario for any owner/repo. - def adhoc_scenario(nwo) + def adhoc_scenario(nwo, extra_args: []) s = Scenario.new(:"adhoc_#{nwo.tr('/', '_')}") s.live_repo(nwo) - s.args("--no-fix") + s.args(*extra_args) unless extra_args.empty? s.category = "adhoc" + s.description = nwo s end + # Split "github/foo -- --no-fix --json" into ["github/foo", ["--no-fix", "--json"]] + def split_adhoc_args(arg) + parts = arg.split(/\s+--\s+/, 2) + nwo = parts[0].strip + extra = parts[1] ? parts[1].split(/\s+/) : [] + [nwo, extra] + end + def repo_nwo?(str) str.match?(%r{\A[A-Za-z0-9._-]+/[A-Za-z0-9._-]+\z}) end def run_one_live(s) - puts "\e[1m── #{s.name} ──\e[0m\n\n" + w = 62 + + # ── TITLE ── + label = " #{s.name} " + inner = w - 2 + puts "\e[1;36m╔#{"═" * inner}╗\e[0m" + puts "\e[1;36m║\e[1;37m#{label.center(inner)}\e[1;36m║\e[0m" + puts "\e[1;36m╚#{"═" * inner}╝\e[0m" + puts " \e[2m#{s.description}\e[0m" if s.description + puts + + # Prepare fixtures early so we can show starting state ctx = s.prepare(@binary, profile_dir: @profile_dir) + + # ── INPUT ── + puts "\e[1m┌─ INPUT #{"─" * (w - 10)}┐\e[0m" + lockfile_label = if s.onboarded? + tpl = s.fixture_spec&.dig("lockfile_template") + tpl ? "\e[32m● onboarded\e[0m \e[2m(#{tpl})\e[0m" : "\e[32m● onboarded\e[0m" + else + "\e[33m○ fresh\e[0m \e[2m(no lockfile)\e[0m" + end + puts "\e[1m│\e[0m lockfile: #{lockfile_label}" + wf_specs = s.fixture_spec&.dig("workflows") || {} + if s.live_repo_nwo + puts "\e[1m│\e[0m repo: \e[36m#{s.live_repo_nwo}\e[0m" + elsif wf_specs.any? + wf_specs.each do |path, spec| + actions = spec.is_a?(Hash) ? (spec["actions"] || []).join(", ") : "raw" + puts "\e[1m│\e[0m workflow: \e[36m#{path}\e[0m → #{actions}" + end + end + flags = s.cli_args + puts "\e[1m│\e[0m flags: #{flags.empty? ? "\e[2m(none)\e[0m" : flags.join(" ")}" + if s.input_spec && !s.input_spec.empty? + puts "\e[1m│\e[0m input: \e[33m#{s.input_spec.map { |p| "#{p["prompt"]}→#{p["response"]}" }.join(", ")}\e[0m" + end + puts "\e[1m└#{"─" * inner}┘\e[0m" + puts + + # ── STARTING STATE ── + show_starting_state(ctx.dir, w) + + # ── EXPECT ── + if s.expect_spec && !s.expect_spec.empty? + puts "\e[1m┌─ EXPECT #{"─" * (w - 11)}┐\e[0m" + format_expect_lines(s.expect_spec).each { |line| puts "\e[1m│\e[0m #{line}" } + puts "\e[1m└#{"─" * inner}┘\e[0m" + puts + end + + # ── OUTPUT ── + puts "\e[1;35m── OUTPUT #{"─" * (w - 10)}\e[0m" + puts "\e[2m$\e[0m #{ctx.cmd_string}" + puts keep = ENV["KEEP_FIXTURES"] t0 = Process.clock_gettime(Process::CLOCK_MONOTONIC) begin - result = ctx.run_pty + result = ctx.run_pty(input_prompts: s.input_spec) elapsed = Process.clock_gettime(Process::CLOCK_MONOTONIC) - t0 puts - puts " \e[2m#{"─" * 40}\e[0m" - if result.success? - puts " \e[32m✓ exit 0\e[0m \e[2m(#{format_elapsed(elapsed)})\e[0m" - else - puts " \e[31m✗ exit #{result.exit_code}\e[0m \e[2m(#{format_elapsed(elapsed)})\e[0m" + + # Capture full diff before teardown + diff_text = `cd #{Shellwords.shellescape(ctx.dir)} && git add -N . 2>/dev/null; git --no-pager diff --color 2>/dev/null`.strip + cache_diff(s.name.to_s, diff_text) + + # ── DIFF ── + show_diff(ctx.dir, w, scenario_name: s.name.to_s) + + # ── RESULT ── + s.instance_variable_set(:@failures, []) + s.instance_variable_get(:@assertions).each { |a| a.call(result) } + + puts + puts "\e[1;35m── RESULT #{"─" * (w - 10)}\e[0m" + if s.expect_spec && !s.expect_spec.empty? + format_expect_checks(s.expect_spec, result, s.failures).each { |line| puts line } end if @profile_dir pdir = File.join(@profile_dir, s.name.to_s) puts " \e[2mprofile: #{pdir}\e[0m" end - show_diff(ctx.dir) + puts + if s.failures.empty? + puts "\e[42;1;37m ✓ PASS \e[0m exit #{result.exit_code} \e[2m(#{format_elapsed(elapsed)})\e[0m" + else + puts "\e[41;1;37m ✗ FAIL \e[0m exit #{result.exit_code} \e[2m(#{format_elapsed(elapsed)})\e[0m" + uncovered = s.failures.select { |f| + !f.include?("lockfile") && !f.include?("exit") && !f.include?("output") && !f.include?("stdout") + } + uncovered.each { |f| puts " \e[31m▸ #{f}\e[0m" } + end @last_dir = ctx.dir puts ensure @@ -925,6 +1211,151 @@ def run_one_live(s) end end + def format_expect(spec) + parts = [] + parts << "exit=#{spec['exit']}" if spec["exit"] + parts << "exit∈#{spec['exit_any'].inspect}" if spec["exit_any"] + parts << "output⊃#{spec['output_contains'].inspect}" if spec["output_contains"] + parts << "output⊅#{spec['output_excludes'].inspect}" if spec["output_excludes"] + parts << "stdout=json" if spec["stdout_is_json"] + parts << "stdout⊃#{spec['stdout_contains'].inspect}" if spec["stdout_contains"] + parts << "stdout⊅#{spec['stdout_excludes'].inspect}" if spec["stdout_excludes"] + parts << "lockfile⊃/#{spec['lockfile_comment_matches']}/" if spec["lockfile_comment_matches"] + parts << "lockfile⊅/#{spec['lockfile_comment_excludes']}/" if spec["lockfile_comment_excludes"] + parts.join(" ") + end + + def format_expect_lines(spec) + lines = [] + lines << "exit = #{spec['exit']}" if spec["exit"] + lines << "exit ∈ #{spec['exit_any'].inspect}" if spec["exit_any"] + if spec["output_contains"] + spec["output_contains"].each { |p| lines << "output contains #{p.inspect}" } + end + if spec["output_excludes"] + spec["output_excludes"].each { |p| lines << "output excludes #{p.inspect}" } + end + lines << "stdout is valid JSON" if spec["stdout_is_json"] + if spec["stdout_contains"] + spec["stdout_contains"].each { |p| lines << "stdout contains #{p.inspect}" } + end + if spec["stdout_excludes"] + spec["stdout_excludes"].each { |p| lines << "stdout excludes #{p.inspect}" } + end + lines << "lockfile matches /#{spec['lockfile_comment_matches']}/" if spec["lockfile_comment_matches"] + lines << "lockfile excludes /#{spec['lockfile_comment_excludes']}/" if spec["lockfile_comment_excludes"] + lines << "lockfile exists" if spec["lockfile_exists"] + if spec["jq"] + spec["jq"].each do |check| + op = if check.key?("equals") + "== #{check['equals'].inspect}" + elsif check.key?("contains") + "contains #{check['contains'].inspect}" + elsif check.key?("not_equals") + "!= #{check['not_equals'].inspect}" + elsif check.key?("matches") + "=~ /#{check['matches']}/" + elsif check.key?("gt") + "> #{check['gt']}" + else + "exists" + end + lines << "jq '#{check['expr']}' #{op}" + end + end + lines + end + + def format_expect_checks(spec, result, failures) + lines = [] + checks = [] + + # Build list of [label, passed?] checks + if spec["exit"] + checks << ["exit = #{spec['exit']} (got #{result.exit_code})", result.exit_code == spec["exit"]] + end + if spec["exit_any"] + checks << ["exit ∈ #{spec['exit_any'].inspect} (got #{result.exit_code})", spec["exit_any"].include?(result.exit_code)] + end + if spec["output_contains"] + spec["output_contains"].each do |pat| + ok = result.output.include?(pat) + checks << ["output contains #{pat.inspect}", ok] + end + end + if spec["output_excludes"] + spec["output_excludes"].each do |pat| + ok = !result.output.include?(pat) + checks << ["output excludes #{pat.inspect}", ok] + end + end + if spec["stdout_is_json"] + ok = begin; JSON.parse(result.stdout); true; rescue; false; end + checks << ["stdout is valid JSON", ok] + end + if spec["stdout_contains"] + spec["stdout_contains"].each do |pat| + ok = result.stdout.include?(pat) + checks << ["stdout contains #{pat.inspect}", ok] + end + end + if spec["stdout_excludes"] + spec["stdout_excludes"].each do |pat| + ok = !result.stdout.include?(pat) + checks << ["stdout excludes #{pat.inspect}", ok] + end + end + if spec["lockfile_comment_matches"] + pat = spec["lockfile_comment_matches"] + ok = !failures.any? { |f| f.include?("lockfile comment matches") } + checks << ["lockfile matches /#{pat}/", ok] + end + if spec["lockfile_comment_excludes"] + pat = spec["lockfile_comment_excludes"] + ok = !failures.any? { |f| f.include?("lockfile comment excludes") } + checks << ["lockfile excludes /#{pat}/", ok] + end + if spec["lockfile_exists"] + ok = !failures.any? { |f| f.include?("lockfile exists") } + checks << ["lockfile exists", ok] + end + if spec["jq"] + spec["jq"].each do |check| + expr = check["expr"] + ok = !failures.any? { |f| f.include?("jq '#{expr}'") } + op = if check.key?("equals") + "== #{check['equals'].inspect}" + elsif check.key?("contains") + "contains #{check['contains'].inspect}" + elsif check.key?("not_equals") + "!= #{check['not_equals'].inspect}" + elsif check.key?("matches") + "=~ /#{check['matches']}/" + elsif check.key?("gt") + "> #{check['gt']}" + else + "exists" + end + # Also show the actual value for richer feedback + actual = nil + begin + parsed = JSON.parse(result.stdout) + actual = IO.popen(["jq", "-r", expr], "r+") { |io| io.write(JSON.generate(parsed)); io.close_write; io.read }&.strip + rescue + end + label = "jq '#{expr}' #{op}" + label += " \e[2m(got #{actual})\e[0m" if actual && !ok + checks << [label, ok] + end + end + + checks.each do |label, passed| + icon = passed ? "\e[32m✓\e[0m" : "\e[31m✗\e[0m" + lines << " #{icon} #{label}" + end + lines + end + def run_all_live @scenarios.each_with_index do |s, i| run_one_live(s) @@ -938,6 +1369,225 @@ def run_all_live puts "\n \e[33m⊘ interrupted\e[0m" end + def run_review(to_run) + verdicts = [] # [{name:, verdict:, note:}] + puts "\e[1;35m┌─ REVIEW MODE #{"─" * 47}┐\e[0m" + puts "\e[1;35m│\e[0m #{to_run.size} scenarios." + puts "\e[1;35m│\e[0m \e[32m[p]ass\e[0m \e[33m[f]lag\e[0m \e[1;33m[F]lag+share\e[0m \e[34m[d]iff\e[0m \e[36m[r]erun\e[0m \e[36m[j]ump\e[0m \e[2m[s]kip\e[0m \e[31m[q]uit\e[0m" + puts "\e[1;35m│\e[0m \e[1;33mF\e[0m = flag + upload partial gist \e[36mj \e[0m = jump to scenario" + puts "\e[1;35m└#{"─" * 60}┘\e[0m" + puts + + quit = false + seen = {} # name -> true, tracks which scenarios have a verdict + i = 0 + while i < to_run.size + s = to_run[i] + run_one_live(s) + puts + loop do + print "\e[1m [#{i + 1}/#{to_run.size}]\e[0m \e[32mp\e[0m \e[33mf\e[0m \e[1;33mF\e[0m \e[34md\e[0m \e[36mr\e[0m \e[36mj\e[0m \e[2ms\e[0m \e[31mq\e[0m > " + raw = $stdin.gets&.strip + case raw + when "d", "D", "diff" + cached = @diff_cache[s.name.to_s] + if cached && !cached.empty? + show_paged_diff(s.name.to_s) + else + puts " \e[2mno diff captured for this scenario\e[0m" + end + next + when "r", "R", "rerun" + puts " \e[36m↻ re-running #{s.name}\e[0m" + puts + break # don't increment i — re-runs same scenario + when /\Aj(?:\s+(.+))?\z/i + target = $1&.strip + if target.nil? || target.empty? + puts " \e[2mscenarios:\e[0m" + to_run.each_with_index do |sc, idx| + marker = seen[sc.name.to_s] ? "\e[2m✓\e[0m" : " " + puts " #{marker} #{idx + 1}. #{sc.name}" + end + next + end + jump_idx = if target.match?(/\A\d+\z/) + target.to_i - 1 + else + to_run.index { |sc| sc.name.to_s.include?(target) } + end + if jump_idx && jump_idx >= 0 && jump_idx < to_run.size + # Only mark forward-skipped scenarios that don't already have a verdict + if jump_idx > i + ((i + 1)...jump_idx).each do |skip_i| + name = to_run[skip_i].name.to_s + next if seen[name] + verdicts << { name: name, verdict: :skipped, note: "jumped" } + seen[name] = true + end + end + i = jump_idx + puts " \e[36m→ jumping to #{to_run[i].name}\e[0m" + puts + break + else + puts " \e[31mno match for '#{target}'\e[0m" + next + end + when "q", "Q", "quit" + name = s.name.to_s + unless seen[name] + verdicts << { name: name, verdict: :skipped, note: nil } + seen[name] = true + end + to_run[(i + 1)..].each do |r| + rname = r.name.to_s + next if seen[rname] + verdicts << { name: rname, verdict: :skipped, note: nil } + seen[rname] = true + end + quit = true + break + when "F" + print " \e[33mnote:\e[0m " + note = $stdin.gets&.strip + verdicts << { name: s.name.to_s, verdict: :flagged, note: note } + seen[s.name.to_s] = true + upload_partial_report(verdicts, to_run.size) + i += 1 + break + when "f", "flag" + print " \e[33mnote:\e[0m " + note = $stdin.gets&.strip + verdicts << { name: s.name.to_s, verdict: :flagged, note: note } + seen[s.name.to_s] = true + i += 1 + break + when "s", "S", "skip" + verdicts << { name: s.name.to_s, verdict: :skipped, note: nil } + seen[s.name.to_s] = true + i += 1 + break + else + verdicts << { name: s.name.to_s, verdict: :passed, note: nil } + seen[s.name.to_s] = true + i += 1 + break + end + end + break if quit + puts + end + + # Print report + print_review_report(verdicts) + rescue Interrupt + puts "\n \e[33m⊘ interrupted\e[0m" + end + + def print_review_report(verdicts) + passed = verdicts.select { |v| v[:verdict] == :passed } + flagged = verdicts.select { |v| v[:verdict] == :flagged } + skipped = verdicts.select { |v| v[:verdict] == :skipped } + + puts + puts "\e[1;35m╔════════════════════════════════════════════════════════════╗\e[0m" + puts "\e[1;35m║\e[1;37m#{"REVIEW REPORT".center(58)}\e[1;35m║\e[0m" + puts "\e[1;35m╚════════════════════════════════════════════════════════════╝\e[0m" + puts + puts " \e[32m✓ #{passed.size} passed\e[0m \e[33m⚑ #{flagged.size} flagged\e[0m \e[2m⊘ #{skipped.size} skipped\e[0m" + puts + + if flagged.any? + puts "\e[1;33m Flagged:\e[0m" + flagged.each do |v| + puts " \e[33m⚑\e[0m #{v[:name]}" + puts " \e[2m#{v[:note]}\e[0m" if v[:note] && !v[:note].empty? + end + puts + end + + if skipped.any? + puts "\e[2m Skipped: #{skipped.map { |v| v[:name] }.join(", ")}\e[0m" + puts + end + + # Write report file + report_path = "/tmp/actions-pin-review-#{Time.now.strftime('%Y%m%d-%H%M%S')}.md" + File.write(report_path, render_review_markdown(verdicts)) + puts " \e[2mReport saved:\e[0m \e[36m#{report_path}\e[0m" + puts + + # Offer gist upload + print " \e[34mUpload as gist?\e[0m \e[2m[y/N]\e[0m > " + answer = $stdin.gets&.strip&.downcase + if answer == "y" + desc = "gh-actions-pin review #{Time.now.strftime('%Y-%m-%d %H:%M')}" + out = `gh gist create #{Shellwords.shellescape(report_path)} --desc #{Shellwords.shellescape(desc)} 2>&1`.strip + if $?.success? + puts " \e[32m✓\e[0m #{out}" + else + puts " \e[31m✗ gist create failed:\e[0m #{out}" + end + end + end + + def upload_partial_report(verdicts, total) + reviewed = verdicts.size + remaining = total - reviewed + md = render_review_markdown(verdicts) + md += "\n---\n_Partial report: #{reviewed}/#{total} reviewed, #{remaining} remaining._\n" + path = "/tmp/actions-pin-review-partial-#{Time.now.strftime('%Y%m%d-%H%M%S')}.md" + File.write(path, md) + desc = "gh-actions-pin review (partial #{reviewed}/#{total}) #{Time.now.strftime('%Y-%m-%d %H:%M')}" + out = `gh gist create #{Shellwords.shellescape(path)} --desc #{Shellwords.shellescape(desc)} 2>&1`.strip + if $?.success? + puts " \e[32m✓ shared:\e[0m #{out}" + else + puts " \e[31m✗ gist failed:\e[0m #{out}" + puts " \e[2msaved locally: #{path}\e[0m" + end + end + + def render_review_markdown(verdicts) + lines = ["# Review Report", ""] + lines << "_#{Time.now.strftime('%Y-%m-%d %H:%M')}_" + lines << "" + + passed = verdicts.select { |v| v[:verdict] == :passed } + flagged = verdicts.select { |v| v[:verdict] == :flagged } + skipped = verdicts.select { |v| v[:verdict] == :skipped } + + lines << "**#{passed.size}** passed · **#{flagged.size}** flagged · **#{skipped.size}** skipped" + lines << "" + + if flagged.any? + lines << "## Flagged" + lines << "" + flagged.each do |v| + lines << "- **#{v[:name]}**" + lines << " - #{v[:note]}" if v[:note] && !v[:note].empty? + end + lines << "" + end + + if passed.any? + lines << "## Passed" + lines << "" + passed.each { |v| lines << "- #{v[:name]}" } + lines << "" + end + + if skipped.any? + lines << "## Skipped" + lines << "" + skipped.each { |v| lines << "- #{v[:name]}" } + lines << "" + end + + lines.join("\n") + end + def run_batch(to_run) puts "Running #{to_run.size} scenario(s)...\n\n" passed = 0 @@ -947,11 +1597,14 @@ def run_batch(to_run) print " #{s.name} ... " begin s.run(@binary, profile_dir: @profile_dir) + cache_diff(s.name.to_s, s.last_diff) if s.failures.empty? puts "\e[32m✓\e[0m" passed += 1 else puts "\e[31m✗\e[0m" + puts " \e[2m$ #{s.last_cmd}\e[0m" if s.last_cmd + puts " #{s.onboarded? ? "\e[32m● onboarded\e[0m" : "\e[33m○ not onboarded\e[0m"}" s.failures.each { |f| puts " \e[31m#{f}\e[0m" } failed += 1 end diff --git a/test/integration/run.rb b/test/integration/run.rb index 203c211e..1479ceff 100644 --- a/test/integration/run.rb +++ b/test/integration/run.rb @@ -55,12 +55,17 @@ def build_lockfile(workflows:, dependencies: {}) end.join("\n") dep_section = dependencies.map do |key, attrs| - attr_lines = attrs.map { |k, v| " #{k}: '#{v}'" }.join("\n") + attr_lines = attrs.map do |k, v| + formatted = v.is_a?(Integer) ? v.to_s : "'#{v}'" + " #{k}: #{formatted}" + end.join("\n") " '#{key}':\n#{attr_lines}" end.join("\n") <<~YAML # This file is machine-generated by `gh actions-pin`. + # Do not edit by hand; run `gh actions-pin` to update. + # Docs: https://gh.io/actions-lockfile version: 'v0.0.1' workflows: #{wf_section} @@ -159,6 +164,79 @@ def hydrate_assertions(s, expect, needs_token: false) s.assert_lockfile_exists end + if expect["lockfile_comment_matches"] + s.assert_lockfile_comment_matches(expect["lockfile_comment_matches"]) + end + + if expect["lockfile_comment_excludes"] + s.assert_lockfile_comment_excludes(expect["lockfile_comment_excludes"]) + end + + if expect["stdout_excludes"] + expect["stdout_excludes"].each do |pat| + s.assert_custom do |r| + if r.stdout.include?(pat) + s.failures << "stdout should not contain #{pat.inspect}\n in:\n#{r.stdout.lines.first(20).map { |l| " #{l}" }.join}" + end + end + end + end + + if expect["jq"] + expect["jq"].each do |check| + expr = check["expr"] + s.assert_custom do |r| + begin + parsed = JSON.parse(r.stdout) + rescue JSON::ParserError => e + s.failures << "jq #{expr}: stdout is not valid JSON: #{e.message}" + next + end + + # Shell out to jq for the expression + result = IO.popen(["jq", "-r", expr], "r+") do |io| + io.write(JSON.generate(parsed)) + io.close_write + io.read + end + result = result&.strip + + if check.key?("equals") + expected = check["equals"].to_s + unless result == expected + s.failures << "jq '#{expr}' = #{result.inspect}, expected #{expected.inspect}" + end + end + + if check.key?("contains") + unless result&.include?(check["contains"].to_s) + s.failures << "jq '#{expr}' = #{result.inspect}, expected to contain #{check['contains'].inspect}" + end + end + + if check.key?("not_equals") + if result == check["not_equals"].to_s + s.failures << "jq '#{expr}' = #{result.inspect}, expected not to equal #{check['not_equals'].inspect}" + end + end + + if check.key?("matches") + unless result&.match?(Regexp.new(check["matches"])) + s.failures << "jq '#{expr}' = #{result.inspect}, expected to match /#{check['matches']}/" + end + end + + if check.key?("gt") + val = result.to_f + threshold = check["gt"].to_f + unless val > threshold + s.failures << "jq '#{expr}' = #{result}, expected > #{threshold}" + end + end + end + end + end + # Token-required scenarios skip gracefully without a token if needs_token s.needs_token(true) @@ -194,8 +272,48 @@ def hydrate_assertions(s, expect, needs_token: false) "tag" => "v4", "branch" => "main", "commit" => "sha1-#{CHECKOUT_SHA}", - "owner_id" => "44036562", - "repo_id" => "197814629" + "owner_id" => 44036562, + "repo_id" => 197814629 + } + } + ) + }, + # Same as pinned_checkout but the dep key reflects a non-narrowed v4 + # (imprecise) lock — sticky precision should keep it as v4 on re-pin. + "pinned_checkout_v4_imprecise" => -> { + build_lockfile( + workflows: { + ".github/workflows/ci.yml" => [ + "actions/checkout@v4:sha1-#{CHECKOUT_SHA}" + ] + }, + dependencies: { + "actions/checkout@v4:sha1-#{CHECKOUT_SHA}" => { + "tag" => "v4", + "branch" => "main", + "commit" => "sha1-#{CHECKOUT_SHA}", + "owner_id" => 44036562, + "repo_id" => 197814629 + } + } + ) + }, + # Lockfile with a fully-narrowed dep key (v4.2.0) — represents a + # workflow that was previously onboarded with default narrowing. + "pinned_checkout_full" => -> { + build_lockfile( + workflows: { + ".github/workflows/ci.yml" => [ + "actions/checkout@v4.2.0:sha1-#{CHECKOUT_SHA}" + ] + }, + dependencies: { + "actions/checkout@v4.2.0:sha1-#{CHECKOUT_SHA}" => { + "tag" => "v4.2.0", + "branch" => "main", + "commit" => "sha1-#{CHECKOUT_SHA}", + "owner_id" => 44036562, + "repo_id" => 197814629 } } ) @@ -337,6 +455,10 @@ def hydrate_assertions(s, expect, needs_token: false) runner.scenario(name) do |s| s.category = spec["category"] || "other" + s.description = spec["description"] + s.expect_spec = expect + s.fixture_spec = fixtures + s.input_spec = spec["input"] # Hydrate workflow fixtures hydrate_workflows(s, fixtures["workflows"]) diff --git a/test/scenarios/catalog.yml b/test/scenarios/catalog.yml index b49f023a..aaf5ee08 100644 --- a/test/scenarios/catalog.yml +++ b/test/scenarios/catalog.yml @@ -8,7 +8,7 @@ # description: what the scenario validates # needs_token: true if the scenario requires a real GH_TOKEN # needs_stub: true if the scenario uses a stub HTTP server -# tags: labels for filtering (e.g. "smoke", "live", "security") +# tags: labels for filtering (e.g. "smoke", "security") # flags: CLI flags passed to the binary # fixtures: inline workflow/lockfile content (Ruby-side hydrates these) # expect: expected exit code and output assertions @@ -17,7 +17,7 @@ # what's declared here. The Go side uses these for table-driven scenario # validation and matrix reporting. # -# Live scenarios (needs_token: true) run against real GitHub repos and are +# Scenarios with needs_token: true require a real GH_TOKEN and are # skipped gracefully when no token is present. version: 1 @@ -39,19 +39,23 @@ categories: description: "Cross-workflow scenarios and dependency tracking" - name: security description: "Impostor commit, lockfile forgery, reachability" - - name: live - description: "Live testing against real GitHub repositories" + - name: narrowing + description: "Tag narrowing, --no-narrow, sticky precision, and version-ref nudges" + - name: onboarding + description: "--no-onboard gating, onboarding-required findings, and CI flag composition" + - name: dependabot + description: "Dependabot consumer contract — JSON findings shape, exit codes, and category/severity for --no-onboard --no-narrow --no-interactive" scenarios: - # ═══════════════════════════════════════════════════════════════════════ - # CATEGORY: happy_path - # ═══════════════════════════════════════════════════════════════════════ + # ╔═════════════════════════════════════════════════════════════════════════╗ + # ║══════════════════════════════ happy_path ═══════════════════════════════║ + # ╚═════════════════════════════════════════════════════════════════════════╝ - name: happy_path category: happy_path description: "Single action resolves and pins successfully" needs_token: true - tags: [smoke, live] + tags: [smoke] fixtures: workflows: ci.yml: @@ -62,14 +66,15 @@ scenarios: - name: already_pinned_all_valid category: happy_path - description: "Already-pinned actions pass without network calls (fast path)" + description: "Already-pinned workflow validates without network calls (fast path)" needs_token: true - tags: [smoke, live] + tags: [smoke] fixtures: workflows: ci.yml: name: CI actions: ["actions/checkout@v4"] + lockfile_template: pinned_checkout expect: exit: 0 @@ -77,7 +82,6 @@ scenarios: category: happy_path description: "Multiple actions from different orgs all resolve" needs_token: true - tags: [live] fixtures: workflows: ci.yml: @@ -91,9 +95,8 @@ scenarios: - name: composite_action_transitive category: happy_path - description: "Composite action with transitive deps resolves recursively" + description: "Action with transitive dependencies (create-github-app-token) resolves correctly" needs_token: true - tags: [live] fixtures: workflows: ci.yml: @@ -102,9 +105,9 @@ scenarios: expect: exit: 0 - # ═══════════════════════════════════════════════════════════════════════ - # CATEGORY: sso_auth - # ═══════════════════════════════════════════════════════════════════════ + # ╔═════════════════════════════════════════════════════════════════════════╗ + # ║═══════════════════════════════ sso_auth ════════════════════════════════║ + # ╚═════════════════════════════════════════════════════════════════════════╝ - name: sso_auth_failure category: sso_auth @@ -166,7 +169,7 @@ scenarios: - name: sso_no_fix_single_url category: sso_auth - description: "SSO URL shown only once in --no-fix path" + description: "--no-fix: SSO details absent — only shows 'Re-run without --no-fix' (known gap)" needs_stub: true tags: [stub] flags: ["--no-fix"] @@ -177,7 +180,7 @@ scenarios: actions: ["actions/checkout@v4"] expect: exit: 1 - custom: sso_url_shown_once + output_excludes: ["SAML enforcement"] - name: mixed_failures category: sso_auth @@ -198,9 +201,9 @@ scenarios: - "actions/checkout" - "octo-org/private-action" - # ═══════════════════════════════════════════════════════════════════════ - # CATEGORY: api_errors - # ═══════════════════════════════════════════════════════════════════════ + # ╔═════════════════════════════════════════════════════════════════════════╗ + # ║══════════════════════════════ api_errors ═══════════════════════════════║ + # ╚═════════════════════════════════════════════════════════════════════════╝ - name: api_404_not_found category: api_errors @@ -259,15 +262,14 @@ scenarios: output_contains: - "could not be resolved" - # ═══════════════════════════════════════════════════════════════════════ - # CATEGORY: lockfile - # ═══════════════════════════════════════════════════════════════════════ + # ╔═════════════════════════════════════════════════════════════════════════╗ + # ║═══════════════════════════════ lockfile ════════════════════════════════║ + # ╚═════════════════════════════════════════════════════════════════════════╝ - - name: no_lockfile_first_run + - name: fresh_first_run category: lockfile description: "No lockfile exists — created on first run" needs_token: true - tags: [live] fixtures: workflows: ci.yml: @@ -277,21 +279,23 @@ scenarios: exit: 0 lockfile_exists: true - - name: corrupt_lockfile_recovery + - name: onboarded_corrupt_recovery category: lockfile - description: "Corrupt YAML lockfile — treated as empty, rewritten" + description: "Corrupt YAML lockfile — user confirms delete, lockfile rewritten from scratch" needs_token: true - tags: [live] fixtures: workflows: ci.yml: name: CI actions: ["actions/checkout@v4"] lockfile: "{{{{invalid yaml content not parseable at all}}}}" + input: + - prompt: "(y/N)" + response: "y" expect: exit: 0 - - name: future_version_lockfile + - name: onboarded_future_version category: lockfile description: "Lockfile from future version — refuses with upgrade hint" tags: [stub] @@ -304,9 +308,9 @@ scenarios: expect: exit: 2 - # ═══════════════════════════════════════════════════════════════════════ - # CATEGORY: workflow_parsing - # ═══════════════════════════════════════════════════════════════════════ + # ╔═════════════════════════════════════════════════════════════════════════╗ + # ║═══════════════════════════ workflow_parsing ════════════════════════════║ + # ╚═════════════════════════════════════════════════════════════════════════╝ - name: no_workflows category: workflow_parsing @@ -331,7 +335,6 @@ scenarios: category: workflow_parsing description: "Workflow with only run: steps (no actions) — RunOnly finding" needs_token: true - tags: [live] fixtures: workflows: ci.yml: @@ -350,7 +353,6 @@ scenarios: category: workflow_parsing description: "Expression-based uses: (${{ }}) — skipped with warning" needs_token: true - tags: [live] fixtures: workflows: ci.yml: @@ -370,7 +372,6 @@ scenarios: category: workflow_parsing description: "Local action (./path) — skipped without warning" needs_token: true - tags: [live] fixtures: workflows: ci.yml: @@ -390,7 +391,6 @@ scenarios: category: workflow_parsing description: "Sub-path action (actions/cache/restore@v4) handled" needs_token: true - tags: [live] fixtures: workflows: ci.yml: @@ -405,7 +405,6 @@ scenarios: category: workflow_parsing description: "Same action used twice in workflow — deduplicated" needs_token: true - tags: [live] fixtures: workflows: ci.yml: @@ -422,15 +421,14 @@ scenarios: expect: exit: 0 - # ═══════════════════════════════════════════════════════════════════════ - # CATEGORY: output_modes - # ═══════════════════════════════════════════════════════════════════════ + # ╔═════════════════════════════════════════════════════════════════════════╗ + # ║═════════════════════════════ output_modes ══════════════════════════════║ + # ╚═════════════════════════════════════════════════════════════════════════╝ - name: no_fix_mode category: output_modes description: "--no-fix with valid lockfile — exit 0, nothing written" needs_token: true - tags: [live] flags: ["--no-fix"] fixtures: workflows: @@ -444,7 +442,6 @@ scenarios: category: output_modes description: "--json=valid produces well-formed JSON on stdout" needs_token: true - tags: [live] flags: ["--no-fix", "--json=valid"] fixtures: workflows: @@ -475,7 +472,6 @@ scenarios: category: output_modes description: "--rescan forces full re-verification" needs_token: true - tags: [live] flags: ["--rescan"] fixtures: workflows: @@ -516,15 +512,14 @@ scenarios: exit: 1 stdout_is_json: true - # ═══════════════════════════════════════════════════════════════════════ - # CATEGORY: multi_workflow - # ═══════════════════════════════════════════════════════════════════════ + # ╔═════════════════════════════════════════════════════════════════════════╗ + # ║════════════════════════════ multi_workflow ═════════════════════════════║ + # ╚═════════════════════════════════════════════════════════════════════════╝ - name: shared_action_across_workflows category: multi_workflow description: "Same action in multiple workflows — resolved once, tracked per-workflow" needs_token: true - tags: [live] fixtures: workflows: ci.yml: @@ -540,7 +535,6 @@ scenarios: category: multi_workflow description: "5 workflows with overlapping actions" needs_token: true - tags: [live] fixtures: workflows: ci.yml: @@ -561,9 +555,9 @@ scenarios: expect: exit: 0 - # ═══════════════════════════════════════════════════════════════════════ - # CATEGORY: security - # ═══════════════════════════════════════════════════════════════════════ + # ╔═════════════════════════════════════════════════════════════════════════╗ + # ║═══════════════════════════════ security ════════════════════════════════║ + # ╚═════════════════════════════════════════════════════════════════════════╝ - name: unresolved_uses_error_icon category: security @@ -607,56 +601,428 @@ scenarios: output_excludes: ["resolution failed:"] output_contains: ["SAML enforcement"] + # ╔═════════════════════════════════════════════════════════════════════════╗ + # ║═══════════════════════════════ narrowing ═══════════════════════════════║ + # ╚═════════════════════════════════════════════════════════════════════════╝ + + - name: fresh_v4_narrows_to_full + category: narrowing + description: "Default: v4 tag narrows to full semver (e.g. v4.2.1)" + needs_token: true + fixtures: + workflows: + ci.yml: + name: CI + actions: ["actions/checkout@v4"] + expect: + exit: 0 + lockfile_comment_matches: 'v4\.\d+\.\d+' + + - name: fresh_full_tag_unchanged + category: narrowing + description: "Full semver v4.2.0 is already precise — no narrowing" + needs_token: true + fixtures: + workflows: + ci.yml: + name: CI + actions: ["actions/checkout@v4.2.0"] + expect: + exit: 0 + lockfile_comment_matches: 'v4\.2\.0' + + - name: fresh_branch_ref_skipped + category: narrowing + description: "Branch ref (main) is not a semver — narrowing skips it" + needs_token: true + fixtures: + workflows: + ci.yml: + name: CI + actions: ["actions/checkout@main"] + expect: + exit: 0 + lockfile_comment_matches: 'main' + + - name: fresh_no_narrow_keeps_major + category: narrowing + description: "--no-narrow: splat ref v4 stays v4 in lockfile (not narrowed to v4.x.y)" + needs_token: true + flags: ["--no-narrow"] + fixtures: + workflows: + ci.yml: + name: CI + actions: ["actions/checkout@v4"] + expect: + exit: 0 + lockfile_comment_excludes: 'v4\.\d+\.\d+' + + - name: fresh_no_narrow_full_tag + category: narrowing + description: "--no-narrow: full semver v4.2.0 passes through unchanged (already precise)" + needs_token: true + flags: ["--no-narrow"] + fixtures: + workflows: + ci.yml: + name: CI + actions: ["actions/checkout@v4.2.0"] + expect: + exit: 0 + lockfile_comment_matches: 'v4\.2\.0' + + - name: onboarded_sticky_imprecise + category: narrowing + description: "Lockfile has splat ref v4 from prior --no-narrow run — re-pin without --no-narrow still keeps v4 (sticky precision)" + needs_token: true + fixtures: + workflows: + ci.yml: + name: CI + actions: ["actions/checkout@v4"] + lockfile_template: pinned_checkout_v4_imprecise + expect: + exit: 0 + lockfile_comment_excludes: 'v4\.\d+\.\d+' + + - name: onboarded_full_tag_unchanged + category: narrowing + description: "Lockfile dep key is v4.2.0 (from prior narrowed pin), workflow says @v4 (never locked as v4) — re-pin keeps the lockfile's full semver precision" + needs_token: true + fixtures: + workflows: + ci.yml: + name: CI + actions: ["actions/checkout@v4"] + lockfile_template: pinned_checkout_full + expect: + exit: 0 + lockfile_comment_matches: 'v4\.\d+\.\d+' + + - name: onboarded_no_narrow_keeps_major + category: narrowing + description: "--no-narrow on onboarded workflow — splat ref v4 stays v4 in lockfile, semver nudge suppressed" + needs_token: true + flags: ["--no-narrow"] + fixtures: + workflows: + ci.yml: + name: CI + actions: ["actions/checkout@v4"] + lockfile_template: pinned_checkout + expect: + exit: 0 + lockfile_comment_excludes: 'v4\.\d+\.\d+' + output_excludes: ["pinned without a full semver tag"] + + - name: fresh_no_narrow_nudge_suppressed + category: narrowing + description: "Default narrowing: imprecise ref produces version-ref nudge in terminal" + needs_token: true + flags: ["--no-narrow"] + fixtures: + workflows: + ci.yml: + name: CI + actions: ["actions/checkout@v4"] + expect: + exit: 0 + output_excludes: ["Consider using full semver"] + + - name: fresh_no_fix_no_narrow_json_version_ref + category: narrowing + description: "--no-fix --no-narrow --json=findings: reports not-pinned finding, no version-ref noise" + needs_token: true + flags: ["--no-narrow", "--no-fix", "--json=findings"] + fixtures: + workflows: + ci.yml: + name: CI + actions: ["actions/checkout@v4"] + expect: + exit: 1 + stdout_is_json: true + stdout_excludes: ["version-ref"] + jq: + - expr: '.findings | length' + equals: 1 + - expr: '.findings[0].category' + equals: 'not-pinned' + - expr: '.findings[0].dependency' + contains: 'actions/checkout' + + # ╔═════════════════════════════════════════════════════════════════════════╗ + # ║══════════════════════════════ onboarding ═══════════════════════════════║ + # ╚═════════════════════════════════════════════════════════════════════════╝ + + - name: fresh_no_onboard_refused + category: onboarding + description: "--no-onboard: new (not-yet-tracked) workflow refused — exit 1" + needs_token: true + flags: ["--no-onboard"] + fixtures: + workflows: + ci.yml: + name: CI + actions: ["actions/checkout@v4"] + expect: + exit: 1 + + - name: onboarded_no_onboard_repin + category: onboarding + description: "--no-onboard: already-tracked workflow re-pins normally" + needs_token: true + flags: ["--no-onboard"] + fixtures: + workflows: + ci.yml: + name: CI + actions: ["actions/checkout@v4"] + lockfile_template: pinned_checkout + expect: + exit: 0 + + - name: onboarded_no_onboard_mixed + category: onboarding + description: "--no-onboard: mix of tracked and untracked — tracked re-pinned, untracked refused" + needs_token: true + flags: ["--no-onboard"] + fixtures: + workflows: + ci.yml: + name: CI + actions: ["actions/checkout@v4"] + deploy.yml: + name: Deploy + actions: ["actions/setup-node@v4"] + lockfile_template: pinned_checkout + expect: + exit: 1 + + - name: fresh_no_fix_no_onboard_json_findings + category: onboarding + description: "--no-onboard --json=findings: refused entries appear as onboarding-required" + needs_token: true + flags: ["--no-onboard", "--no-fix", "--json=findings"] + fixtures: + workflows: + ci.yml: + name: CI + actions: ["actions/checkout@v4"] + expect: + exit: 1 + stdout_is_json: true + stdout_contains: ["onboarding-required"] + + - name: fresh_no_onboard_caution + category: onboarding + description: "--no-onboard: terminal shows caution about skipped entries" + needs_token: true + flags: ["--no-onboard"] + fixtures: + workflows: + ci.yml: + name: CI + actions: ["actions/checkout@v4"] + expect: + exit: 1 + output_contains: ["onboarding-required", "skipped"] + + - name: onboarded_no_narrow_no_onboard_combined + category: onboarding + description: "--no-onboard --no-narrow: both flags compose — tracked re-pins at imprecise ref" + needs_token: true + flags: ["--no-onboard", "--no-narrow"] + fixtures: + workflows: + ci.yml: + name: CI + actions: ["actions/checkout@v4"] + lockfile_template: pinned_checkout_v4_imprecise + expect: + exit: 0 + + - name: fresh_no_interactive_no_onboard_ci + category: onboarding + description: "--no-onboard --no-interactive: CI-safe flags, no prompts" + needs_token: true + flags: ["--no-onboard", "--no-interactive"] + fixtures: + workflows: + ci.yml: + name: CI + actions: ["actions/checkout@v4"] + expect: + exit: 1 + # ═══════════════════════════════════════════════════════════════════════ - # CATEGORY: live — Real GitHub repos + # CATEGORY: lockfile (additional scenarios) # ═══════════════════════════════════════════════════════════════════════ - - name: live_github_github - category: live - description: "Live: github/github — large internal monorepo workflows" + - name: onboarded_no_fix_corrupt + category: lockfile + description: "Corrupt lockfile with --no-fix — fails, no recovery attempted" needs_token: true - tags: [live, real_repo] - live_repo: github/github flags: ["--no-fix"] + fixtures: + workflows: + ci.yml: + name: CI + actions: ["actions/checkout@v4"] + lockfile: "{{{{invalid yaml content not parseable at all}}}}" expect: - exit_any: [0, 1] + exit: 2 - - name: live_vercel_nextjs - category: live - description: "Live: vercel/next.js — popular OSS with many actions" + - name: onboarded_no_interactive_corrupt + category: lockfile + description: "Corrupt lockfile with --no-interactive — fails without prompt" needs_token: true - tags: [live, real_repo] - live_repo: vercel/next.js - flags: ["--no-fix"] + flags: ["--no-interactive"] + fixtures: + workflows: + ci.yml: + name: CI + actions: ["actions/checkout@v4"] + lockfile: "{{{{invalid yaml content not parseable at all}}}}" expect: - exit_any: [0, 1] + exit: 2 - - name: live_github_internal_actions - category: live - description: "Live: github/internal-actions — internal action library" + # ╔═════════════════════════════════════════════════════════════════════════╗ + # ║══════════════════════════════ dependabot ═══════════════════════════════║ + # ╚═════════════════════════════════════════════════════════════════════════╝ + # These scenarios test the contract surface consumed by dependabot-core's + # CliEngine (cli_engine.rb). Every scenario runs with the dependabot flag + # set: --no-onboard --no-narrow --no-interactive --json=valid,findings,workflows + # + # The jq assertions verify the JSON shape that dependabot-core's + # FindingMapper relies on. Changes here must be coordinated with + # dependabot/dependabot-core github_actions ecosystem. + + - name: dbot_onboarded_clean + category: dependabot + description: "All pinned, no findings — exit 0, valid true, empty findings" needs_token: true - tags: [live, real_repo] - live_repo: github/internal-actions - flags: ["--no-fix"] + flags: ["--no-onboard", "--no-narrow", "--no-interactive", "--json=valid,findings,workflows"] + fixtures: + workflows: + ci.yml: + name: CI + actions: ["actions/checkout@v4"] + lockfile_template: pinned_checkout expect: - exit_any: [0, 1] + exit: 0 + stdout_is_json: true + jq: + - expr: ".valid" + equals: "true" + - expr: ".findings | length" + equals: "0" + - expr: ".workflows | length" + equals: "1" + - expr: ".workflows[0].path" + contains: "ci.yml" + - expr: ".workflows[0].valid" + equals: "true" - - name: live_github_mcv3_boot - category: live - description: "Live: github/mcv3-boot — codespace boot workflows" + - name: dbot_fresh_refused + category: dependabot + description: "Fresh repo with --no-onboard — exit 1, onboarding-required finding" needs_token: true - tags: [live, real_repo] - live_repo: github/mcv3-boot - flags: ["--no-fix"] + flags: ["--no-onboard", "--no-narrow", "--no-interactive", "--json=valid,findings,workflows"] + fixtures: + workflows: + ci.yml: + name: CI + actions: ["actions/checkout@v4"] expect: - exit_any: [0, 1] + exit: 1 + stdout_is_json: true + jq: + - expr: ".valid" + equals: "false" + - expr: ".findings | length" + greater_than: 0 + - expr: ".findings[0].category" + equals: "onboarding-required" + - expr: ".findings[0].severity" + equals: "error" + - expr: ".findings[0].workflow" + contains: "ci.yml" - - name: live_github_launch - category: live - description: "Live: github/launch — deployment workflows" + - name: dbot_not_pinned + category: dependabot + description: "Onboarded but action not in lockfile — not-pinned finding" needs_token: true - tags: [live, real_repo] - live_repo: github/launch - flags: ["--no-fix"] + flags: ["--no-onboard", "--no-narrow", "--no-interactive", "--no-fix", "--json=valid,findings,workflows"] + fixtures: + workflows: + ci.yml: + name: CI + actions: ["actions/checkout@v4", "actions/setup-node@v4"] + lockfile_template: pinned_checkout expect: - exit_any: [0, 1] + exit: 1 + stdout_is_json: true + jq: + - expr: ".valid" + equals: "false" + - expr: '.findings[] | select(.category == "not-pinned") | .severity' + equals: "error" + - expr: '.findings[] | select(.category == "not-pinned") | .dependency' + contains: "actions/setup-node" + + - name: dbot_corrupt_lockfile_ci + category: dependabot + description: "Corrupt lockfile with --no-interactive — exit 2, engine error" + needs_token: true + flags: ["--no-onboard", "--no-narrow", "--no-interactive", "--json=valid,findings"] + fixtures: + workflows: + ci.yml: + name: CI + actions: ["actions/checkout@v4"] + lockfile: "{{{{invalid yaml content not parseable at all}}}}" + expect: + exit: 2 + + - name: dbot_multi_workflow + category: dependabot + description: "Multiple workflows — per-workflow findings in JSON" + needs_token: true + flags: ["--no-onboard", "--no-narrow", "--no-interactive", "--no-fix", "--json=valid,findings,workflows"] + fixtures: + workflows: + ci.yml: + name: CI + actions: ["actions/checkout@v4"] + deploy.yml: + name: Deploy + actions: ["actions/checkout@v4", "actions/setup-node@v4"] + lockfile_template: pinned_checkout + expect: + exit: 1 + stdout_is_json: true + jq: + - expr: ".workflows | length" + equals: "2" + - expr: '.workflows[] | select(.path | contains("deploy")) | .valid' + equals: "false" + + - name: dbot_version_ref_suppressed + category: dependabot + description: "--no-narrow suppresses version-ref findings — not in JSON" + needs_token: true + flags: ["--no-onboard", "--no-narrow", "--no-interactive", "--json=valid,findings"] + fixtures: + workflows: + ci.yml: + name: CI + actions: ["actions/checkout@v4"] + lockfile_template: pinned_checkout + expect: + exit: 0 + stdout_is_json: true + jq: + - expr: '.findings[] | select(.category == "version-ref") | .category' + equals: "" From 5268fa9f6d250acc45427dba77a90691338f1d82 Mon Sep 17 00:00:00 2001 From: Jeff Martin Date: Sat, 13 Jun 2026 21:09:32 -0500 Subject: [PATCH 06/13] check: surface SSO authorization URL in --no-fix mode The --no-fix early return path exits before the SSO URL display block, so users never see the actionable authorization link when running read-only. Move the SSO URL surface before the early return so it shows regardless of fix mode. Closes #45 --- cmd/gh-actions-pin/check.go | 14 ++++++++++++-- test/scenarios/catalog.yml | 4 ++-- 2 files changed, 14 insertions(+), 4 deletions(-) diff --git a/cmd/gh-actions-pin/check.go b/cmd/gh-actions-pin/check.go index 2d4d4da8..d32554e3 100644 --- a/cmd/gh-actions-pin/check.go +++ b/cmd/gh-actions-pin/check.go @@ -266,8 +266,10 @@ func runCheck(cmd *cobra.Command, opts *checkOptions, newResolver resolverFunc) // so Plan/Commit never pins them; already-tracked refs that were bumped // (ref-changed) are left to re-pin as usual. onboardingRefused := 0 + var refusedLabels []string if noOnboardFlag(cmd) { - onboardingRefused = gateNoOnboard(report) + refusedLabels = gateNoOnboard(report) + onboardingRefused = len(refusedLabels) if onboardingRefused > 0 { valid = report.IsValid() } @@ -297,6 +299,14 @@ func runCheck(cmd *cobra.Command, opts *checkOptions, newResolver resolverFunc) return err } } + // Surface SSO URL even in read-only mode — it's the actionable fix + // for SAML-gated repos and shouldn't require a --fix run to see. + if gc := r.GHClient(); gc != nil { + if ssoURL := gc.SSOURL(); ssoURL != "" { + console.TermBlank() + console.TermDetail("Authorize in your web browser: %s", ssoURL) + } + } if !valid { if opts.jsonFields == "" { console.TermDetail("Re-run without --no-fix to apply fixes.") @@ -378,7 +388,7 @@ func runCheck(cmd *cobra.Command, opts *checkOptions, newResolver resolverFunc) // Terminal summary. hasInconclusive := opts.rescan && report.HasInconclusive() - summaryErr := renderPinSummary(console, record, report, r, skippedRescan, hasInconclusive, onboardingRefused, opts.noNarrow) + summaryErr := renderPinSummary(console, record, report, r, skippedRescan, hasInconclusive, refusedLabels, opts.noNarrow) // Surface the SAML SSO authorization URL if one was captured during // the run, matching cli/cli's "Authorize in your web browser:" line. diff --git a/test/scenarios/catalog.yml b/test/scenarios/catalog.yml index aaf5ee08..ba31ac6e 100644 --- a/test/scenarios/catalog.yml +++ b/test/scenarios/catalog.yml @@ -169,7 +169,7 @@ scenarios: - name: sso_no_fix_single_url category: sso_auth - description: "--no-fix: SSO details absent — only shows 'Re-run without --no-fix' (known gap)" + description: "--no-fix: SSO authorization URL surfaces even in read-only mode" needs_stub: true tags: [stub] flags: ["--no-fix"] @@ -180,7 +180,7 @@ scenarios: actions: ["actions/checkout@v4"] expect: exit: 1 - output_excludes: ["SAML enforcement"] + output_contains: ["Authorize in your web browser"] - name: mixed_failures category: sso_auth From eab0ae02bddd2e1025d28e2a47e552438f8ec522 Mon Sep 17 00:00:00 2001 From: Jeff Martin Date: Sat, 13 Jun 2026 21:09:40 -0500 Subject: [PATCH 07/13] pin: retain existing pins when re-resolution fails (403/transient) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Rename retainImpostorPins → retainUnresolvablePins and broaden the filter to also retain Unresolved entries (not just Investigate + ImpostorCommit). When a 403/SSO or transient error prevents re-resolution, the existing on-disk pin is preserved instead of being silently dropped. New test verifies the Unresolved retention path round-trips through a lockfile write. Closes #46 --- internal/pin/commit.go | 22 ++++++----- internal/pin/retain_impostor_test.go | 57 ++++++++++++++++++++++++++-- 2 files changed, 66 insertions(+), 13 deletions(-) diff --git a/internal/pin/commit.go b/internal/pin/commit.go index 9f3a95ed..7ba7aa80 100644 --- a/internal/pin/commit.go +++ b/internal/pin/commit.go @@ -66,7 +66,7 @@ func Commit(ctx context.Context, rec *Record, store *lockfile.State, copts *Comm wfKey := workflowfile.KeyFromPath(wfPath) parentMap := buildParentMap(rec, wfPath) directKeys := buildDirectKeys(rec, wfPath) - deps = retainImpostorPins(rec, store, wfPath, deps, directKeys) + deps = retainUnresolvablePins(rec, store, wfPath, deps, directKeys) if err := store.Set(ctx, wfKey, deps, parentMap, directKeys); err != nil { return fmt.Errorf("updating lockfile for %s: %w", wfPath, err) } @@ -118,21 +118,25 @@ func groupPinnedByWorkflow(rec *Record) map[string][]dep.Dependency { return result } -// retainImpostorPins re-adds the workflow's existing on-disk pins for any -// impostor-flagged dep so a co-located re-pin never silently drops them. -func retainImpostorPins(rec *Record, store *lockfile.State, wfPath string, deps []dep.Dependency, directKeys map[string]bool) []dep.Dependency { - impostor := make(map[string]bool) +// retainUnresolvablePins re-adds the workflow's existing on-disk pins for any +// entry that cannot be resolved this run (impostor-flagged or transiently +// unresolvable, e.g. 403/SSO). Without this a co-located re-pin silently +// drops the existing pin. +func retainUnresolvablePins(rec *Record, store *lockfile.State, wfPath string, deps []dep.Dependency, directKeys map[string]bool) []dep.Dependency { + retain := make(map[string]bool) for _, e := range rec.Entries { - if e.Resolution != Investigate || e.Issue != string(checks.ImpostorCommit) { + shouldRetain := (e.Resolution == Investigate && e.Issue == string(checks.ImpostorCommit)) || + e.Resolution == Unresolved + if !shouldRetain { continue } for _, wf := range e.Workflows { if wf == wfPath { - impostor[strings.ToLower(e.NWO+"@"+e.Ref)] = true + retain[strings.ToLower(e.NWO+"@"+e.Ref)] = true } } } - if len(impostor) == 0 { + if len(retain) == 0 { return deps } existing, err := store.Get(workflowfile.KeyFromPath(wfPath)) @@ -145,7 +149,7 @@ func retainImpostorPins(rec *Record, store *lockfile.State, wfPath string, deps } for _, d := range existing { k := strings.ToLower(d.NWO + "@" + d.Ref) - if impostor[k] && !have[k] { + if retain[k] && !have[k] { deps = append(deps, d) directKeys[d.Key()] = true have[k] = true diff --git a/internal/pin/retain_impostor_test.go b/internal/pin/retain_impostor_test.go index 407eb5c6..964c1057 100644 --- a/internal/pin/retain_impostor_test.go +++ b/internal/pin/retain_impostor_test.go @@ -22,7 +22,7 @@ func (fakeMeta) RepoIDs(_ context.Context, _, _ string) (int64, int64, error) { // A co-located bump forces a workflow rewrite; the impostor pin already on // disk must be retained, not silently dropped toward an empty pin list. -func TestRetainImpostorPins_keepsExistingPinOnColocatedRepin(t *testing.T) { +func TestRetainUnresolvablePins_keepsExistingPinOnColocatedRepin(t *testing.T) { dir := t.TempDir() require.NoError(t, os.MkdirAll(filepath.Join(dir, ".github", "workflows"), 0o755)) wfPath := filepath.Join(dir, ".github", "workflows", "ci.yml") @@ -50,7 +50,7 @@ func TestRetainImpostorPins_keepsExistingPinOnColocatedRepin(t *testing.T) { } directKeys := map[string]bool{"actions/checkout@v5": true} - got := retainImpostorPins(rec, store, wfPath, deps, directKeys) + got := retainUnresolvablePins(rec, store, wfPath, deps, directKeys) require.Len(t, got, 2, "impostor pin must be re-added alongside the bumped pin") assert.True(t, directKeys["bad/impostor@v1"], "retained impostor pin must stay direct") @@ -72,7 +72,7 @@ func TestRetainImpostorPins_keepsExistingPinOnColocatedRepin(t *testing.T) { // With no co-located new pin, the impostor's workflow is untouched and there // is nothing to retain; the helper is a no-op on the deps it is handed. -func TestRetainImpostorPins_noopWithoutImpostorFinding(t *testing.T) { +func TestRetainUnresolvablePins_noopWithoutImpostorFinding(t *testing.T) { dir := t.TempDir() require.NoError(t, os.MkdirAll(filepath.Join(dir, ".github", "workflows"), 0o755)) wfPath := filepath.Join(dir, ".github", "workflows", "ci.yml") @@ -86,6 +86,55 @@ func TestRetainImpostorPins_noopWithoutImpostorFinding(t *testing.T) { deps := []dep.Dependency{{NWO: "actions/checkout", Ref: "v5"}} directKeys := map[string]bool{"actions/checkout@v5": true} - got := retainImpostorPins(rec, store, wfPath, deps, directKeys) + got := retainUnresolvablePins(rec, store, wfPath, deps, directKeys) assert.Len(t, got, 1) } + +// Unresolved entries (403, transient errors) should also be retained so +// a co-located re-pin doesn't silently drop the existing pin. +func TestRetainUnresolvablePins_keepsUnresolvedPin(t *testing.T) { + dir := t.TempDir() + require.NoError(t, os.MkdirAll(filepath.Join(dir, ".github", "workflows"), 0o755)) + wfPath := filepath.Join(dir, ".github", "workflows", "ci.yml") + wfKey := workflowfile.KeyFromPath(wfPath) + + store, err := lockfile.LoadState(dir, fakeMeta{}) + require.NoError(t, err) + + seed := []dep.Dependency{ + {NWO: "corp/private", Ref: "v2", Tag: "v2", Branch: "main", SHA: "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", HashAlgo: "sha1"}, + {NWO: "actions/checkout", Ref: "v4", Tag: "v4", Branch: "main", SHA: "bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb", HashAlgo: "sha1"}, + } + require.NoError(t, store.Set(context.Background(), wfKey, seed, nil, nil)) + require.NoError(t, store.Save()) + + // Re-pin: checkout bumps (Pinned), private repo 403s (Unresolved). + rec := &Record{ + Entries: []Entry{ + {NWO: "actions/checkout", Ref: "v5", SHA: "cccccccccccccccccccccccccccccccccccccccc", Resolution: Pinned, Direct: true, OnBranch: "main", Workflows: []string{wfPath}}, + {NWO: "corp/private", Ref: "v2", Resolution: Unresolved, Issue: "sso-required", Reason: "403 SSO authorization required", Workflows: []string{wfPath}}, + }, + } + deps := []dep.Dependency{ + {NWO: "actions/checkout", Ref: "v5", Branch: "main", SHA: "cccccccccccccccccccccccccccccccccccccccc", HashAlgo: "sha1"}, + } + directKeys := map[string]bool{"actions/checkout@v5": true} + + got := retainUnresolvablePins(rec, store, wfPath, deps, directKeys) + + require.Len(t, got, 2, "unresolved pin must be retained alongside the bumped pin") + assert.True(t, directKeys["corp/private@v2"], "retained unresolved pin must stay direct") + + // Verify it survives a write round-trip. + require.NoError(t, store.Set(context.Background(), wfKey, got, buildParentMap(rec, wfPath), directKeys)) + require.NoError(t, store.Save()) + + after, err := store.Get(wfKey) + require.NoError(t, err) + names := map[string]bool{} + for _, d := range after { + names[d.NWO] = true + } + assert.True(t, names["corp/private"], "unresolved pin must survive the re-pin write") + assert.True(t, names["actions/checkout"], "bumped pin must be written") +} From 66962a5a0dd1d34257f9c4e0280e8f55cf3af1db Mon Sep 17 00:00:00 2001 From: Jeff Martin Date: Sat, 13 Jun 2026 21:09:46 -0500 Subject: [PATCH 08/13] onboard: name refused workflows in --no-onboard summary Change gateNoOnboard to return refused entry labels (e.g. "actions/checkout@v4 in .github/workflows/ci.yml") instead of just a count. renderPinSummary now lists each refused entry so users know exactly what was skipped. Also removes the stale 'live' tag assertion from catalog_test.go (live scenarios were removed earlier). Closes #44 --- cmd/gh-actions-pin/onboard_gate.go | 9 +++++---- cmd/gh-actions-pin/pin_summary.go | 6 +++++- test/scenarios/catalog_test.go | 3 --- 3 files changed, 10 insertions(+), 8 deletions(-) diff --git a/cmd/gh-actions-pin/onboard_gate.go b/cmd/gh-actions-pin/onboard_gate.go index 650a2c21..11059efd 100644 --- a/cmd/gh-actions-pin/onboard_gate.go +++ b/cmd/gh-actions-pin/onboard_gate.go @@ -16,9 +16,10 @@ func noOnboardFlag(cmd *cobra.Command) bool { } // gateNoOnboard rewrites per-workflow NotPinned findings to OnboardingRequired -// and drops their refs so Plan never pins them. Returns refs refused. -func gateNoOnboard(report *checks.Report) int { - refused := 0 +// and drops their refs so Plan never pins them. Returns the refused entry labels +// (e.g. "actions/checkout@v4 in .github/workflows/ci.yml"). +func gateNoOnboard(report *checks.Report) []string { + var refused []string for wi := range report.Workflows { wr := &report.Workflows[wi] refusedKeys := make(map[string]bool) @@ -32,7 +33,7 @@ func gateNoOnboard(report *checks.Report) int { f.Category = checks.OnboardingRequired f.Detail = fmt.Sprintf("%s@%s has no lockfile entry; --no-onboard refuses to add new workflows or actions", ar.FullName(), ar.Ref) f.Remediation = "onboard it first with `gh actions-pin check` (without --no-onboard)" - refused++ + refused = append(refused, fmt.Sprintf("%s@%s in %s", ar.FullName(), ar.Ref, wr.Path)) } if len(refusedKeys) == 0 { continue diff --git a/cmd/gh-actions-pin/pin_summary.go b/cmd/gh-actions-pin/pin_summary.go index 368f5d20..d05a5949 100644 --- a/cmd/gh-actions-pin/pin_summary.go +++ b/cmd/gh-actions-pin/pin_summary.go @@ -16,7 +16,7 @@ import ( // renderPinSummary prints the terminal summary after pin.Plan + pin.Commit. // It groups pinned entries by NWO@Ref, shows investigation alerts, unresolved // warnings, and the all-valid message when nothing changed. -func renderPinSummary(console *ui.UI, record *pin.Record, report *checks.Report, r *resolve.Resolver, skippedRescan int, hasInconclusive bool, onboardingRefused int, noNarrow bool) error { +func renderPinSummary(console *ui.UI, record *pin.Record, report *checks.Report, r *resolve.Resolver, skippedRescan int, hasInconclusive bool, refusedLabels []string, noNarrow bool) error { pinned := record.Pinned() investigated := record.Investigated() @@ -43,6 +43,7 @@ func renderPinSummary(console *ui.UI, record *pin.Record, report *checks.Report, console.TermNeutral("No workflows to check") return nil } + onboardingRefused := len(refusedLabels) allClean := len(pinned) == 0 && len(investigated) == 0 && len(unresolvedEntries) == 0 if allClean && onboardingRefused == 0 && !hasInconclusive { console.TermSuccess("All %d %s valid", total, ui.Pluralize(total, "workflow", "workflows")) @@ -58,6 +59,9 @@ func renderPinSummary(console *ui.UI, record *pin.Record, report *checks.Report, console.TermCaution("%d onboarding-required %s skipped — re-run without --no-onboard to add %s", onboardingRefused, ui.Pluralize(onboardingRefused, "entry", "entries"), ui.Pluralize(onboardingRefused, "it", "them")) + for _, label := range refusedLabels { + console.TermDetail(" %s", console.TermYellow(label)) + } } if len(investigated) > 0 || onboardingRefused > 0 || len(unresolvedEntries) > 0 { diff --git a/test/scenarios/catalog_test.go b/test/scenarios/catalog_test.go index d2398218..3392c1d2 100644 --- a/test/scenarios/catalog_test.go +++ b/test/scenarios/catalog_test.go @@ -46,9 +46,6 @@ func TestCatalogByTag(t *testing.T) { smoke := cat.ByTag("smoke") assert.Greater(t, len(smoke), 0, "expected at least one 'smoke' scenario") - live := cat.ByTag("live") - assert.Greater(t, len(live), 0, "expected at least one 'live' scenario") - stub := cat.ByTag("stub") assert.Greater(t, len(stub), 0, "expected at least one 'stub' scenario") } From d115277e658647789233847b190475ffd9c9a415 Mon Sep 17 00:00:00 2001 From: Jeff Martin Date: Sat, 13 Jun 2026 21:11:21 -0500 Subject: [PATCH 09/13] scenarios: add dbot_transient_403_drops_pin contract scenario MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit SSO 403 on a previously-pinned action — exercises the not-pinned/error finding path that the dependabot-core dropped-pin guard keys on. Uses the default SSO 403 stub with a pinned_checkout lockfile template. Ref #47 --- test/scenarios/catalog.yml | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/test/scenarios/catalog.yml b/test/scenarios/catalog.yml index ba31ac6e..e25f7536 100644 --- a/test/scenarios/catalog.yml +++ b/test/scenarios/catalog.yml @@ -1026,3 +1026,26 @@ scenarios: jq: - expr: '.findings[] | select(.category == "version-ref") | .category' equals: "" + + - name: dbot_transient_403_drops_pin + category: dependabot + description: "SSO 403 on a previously-pinned action produces not-pinned/error finding" + needs_stub: true + tags: [stub] + flags: ["--no-onboard", "--no-narrow", "--no-interactive", "--json=valid,findings"] + fixtures: + workflows: + ci.yml: + name: CI + actions: ["actions/checkout@v4"] + lockfile_template: pinned_checkout + expect: + exit: 1 + stdout_is_json: true + jq: + - expr: '.valid' + equals: "false" + - expr: '.findings | length' + greater_than: 0 + - expr: '.findings[] | select(.severity == "error") | .severity' + equals: "error" From 3e30d1d1238dfe34a5365bf55af54f462118581e Mon Sep 17 00:00:00 2001 From: Jeff Martin Date: Sat, 13 Jun 2026 21:13:51 -0500 Subject: [PATCH 10/13] onboard: exit 0 when only onboarding-required findings remain When --no-onboard skips new workflows and no other blocking findings exist, exit 0 instead of 1. Onboarding-required findings are downgraded to info severity (non-blocking) but remain in the JSON output so machine consumers can still observe the skip. This aligns with the dependabot-core consumer contract: dependabot never bootstraps lockfiles (that's the onboarding flow's job), so an incremental run that only sees new/un-onboarded workflows should not block. Mixed runs (onboarding-required + impostor/not-pinned/unresolved) still exit 1 because the blocking findings survive. Closes #43 --- cmd/gh-actions-pin/check.go | 2 +- cmd/gh-actions-pin/onboard_gate.go | 1 + cmd/gh-actions-pin/pin_summary.go | 2 +- test/scenarios/catalog.yml | 13 +++++++------ 4 files changed, 10 insertions(+), 8 deletions(-) diff --git a/cmd/gh-actions-pin/check.go b/cmd/gh-actions-pin/check.go index d32554e3..f33ad2be 100644 --- a/cmd/gh-actions-pin/check.go +++ b/cmd/gh-actions-pin/check.go @@ -380,7 +380,7 @@ func runCheck(cmd *cobra.Command, opts *checkOptions, newResolver resolverFunc) if err := format.WriteJSON(out, report, valid, opts.jsonFields, cliVersion(), store.File().Version); err != nil { return err } - if len(record.Investigated()) > 0 || onboardingRefused > 0 { + if len(record.Investigated()) > 0 { return errSilent } return nil diff --git a/cmd/gh-actions-pin/onboard_gate.go b/cmd/gh-actions-pin/onboard_gate.go index 11059efd..aaeb0053 100644 --- a/cmd/gh-actions-pin/onboard_gate.go +++ b/cmd/gh-actions-pin/onboard_gate.go @@ -31,6 +31,7 @@ func gateNoOnboard(report *checks.Report) []string { ar := f.ActionRef refusedKeys[parserlock.IndexKey(ar.Owner, ar.Repo, ar.Ref)] = true f.Category = checks.OnboardingRequired + f.Severity = checks.SeverityInfo f.Detail = fmt.Sprintf("%s@%s has no lockfile entry; --no-onboard refuses to add new workflows or actions", ar.FullName(), ar.Ref) f.Remediation = "onboard it first with `gh actions-pin check` (without --no-onboard)" refused = append(refused, fmt.Sprintf("%s@%s in %s", ar.FullName(), ar.Ref, wr.Path)) diff --git a/cmd/gh-actions-pin/pin_summary.go b/cmd/gh-actions-pin/pin_summary.go index d05a5949..898a0ae0 100644 --- a/cmd/gh-actions-pin/pin_summary.go +++ b/cmd/gh-actions-pin/pin_summary.go @@ -64,7 +64,7 @@ func renderPinSummary(console *ui.UI, record *pin.Record, report *checks.Report, } } - if len(investigated) > 0 || onboardingRefused > 0 || len(unresolvedEntries) > 0 { + if len(investigated) > 0 || len(unresolvedEntries) > 0 { return errSilent } return nil diff --git a/test/scenarios/catalog.yml b/test/scenarios/catalog.yml index e25f7536..f5c91ee0 100644 --- a/test/scenarios/catalog.yml +++ b/test/scenarios/catalog.yml @@ -758,7 +758,7 @@ scenarios: - name: fresh_no_onboard_refused category: onboarding - description: "--no-onboard: new (not-yet-tracked) workflow refused — exit 1" + description: "--no-onboard: new (not-yet-tracked) workflow refused — exit 0 (non-blocking skip)" needs_token: true flags: ["--no-onboard"] fixtures: @@ -767,7 +767,8 @@ scenarios: name: CI actions: ["actions/checkout@v4"] expect: - exit: 1 + exit: 0 + output_contains: ["onboarding-required"] - name: onboarded_no_onboard_repin category: onboarding @@ -927,7 +928,7 @@ scenarios: - name: dbot_fresh_refused category: dependabot - description: "Fresh repo with --no-onboard — exit 1, onboarding-required finding" + description: "Fresh repo with --no-onboard — exit 0, onboarding-required/info finding (non-blocking skip)" needs_token: true flags: ["--no-onboard", "--no-narrow", "--no-interactive", "--json=valid,findings,workflows"] fixtures: @@ -936,17 +937,17 @@ scenarios: name: CI actions: ["actions/checkout@v4"] expect: - exit: 1 + exit: 0 stdout_is_json: true jq: - expr: ".valid" - equals: "false" + equals: "true" - expr: ".findings | length" greater_than: 0 - expr: ".findings[0].category" equals: "onboarding-required" - expr: ".findings[0].severity" - equals: "error" + equals: "info" - expr: ".findings[0].workflow" contains: "ci.yml" From ba6ac4776d08535653800153f3cd0c2fc0371126 Mon Sep 17 00:00:00 2001 From: Jeff Martin Date: Sat, 13 Jun 2026 21:20:01 -0500 Subject: [PATCH 11/13] scenarios: add dbot_impostor_blocks and dbot_forgery_blocks stubs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Skipped scenarios for the dependabot contract category covering impostor-commit and lockfile-forgery findings. Both require multi-phase HTTP stub infra (GraphQL + REST) that doesn't exist yet — marked with skip + needs_stub. Category strings confirmed: impostor-commit, lockfile-forgery. Severity: error. Shape matches existing dbot scenarios. --- test/scenarios/catalog.yml | 52 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 52 insertions(+) diff --git a/test/scenarios/catalog.yml b/test/scenarios/catalog.yml index f5c91ee0..b87e8c47 100644 --- a/test/scenarios/catalog.yml +++ b/test/scenarios/catalog.yml @@ -1050,3 +1050,55 @@ scenarios: greater_than: 0 - expr: '.findings[] | select(.severity == "error") | .severity' equals: "error" + + - name: dbot_impostor_blocks + category: dependabot + description: "Impostor commit (unreachable SHA) produces impostor-commit/error finding" + needs_stub: true + skip: "stub infra not yet wired — requires multi-phase HTTP stubs (GraphQL + REST branch-contains + REST compare)" + tags: [stub] + flags: ["--no-onboard", "--no-narrow", "--no-interactive", "--json=valid,findings"] + fixtures: + workflows: + ci.yml: + name: CI + actions: ["actions/checkout@v4"] + lockfile_template: pinned_checkout + expect: + exit: 1 + stdout_is_json: true + jq: + - expr: '.valid' + equals: "false" + - expr: '.findings | length' + greater_than: 0 + - expr: '.findings[] | select(.category == "impostor-commit") | .category' + equals: "impostor-commit" + - expr: '.findings[] | select(.category == "impostor-commit") | .severity' + equals: "error" + + - name: dbot_forgery_blocks + category: dependabot + description: "Lockfile forgery (pin doesn't match resolved commit) produces lockfile-forgery/error finding" + needs_stub: true + skip: "stub infra not yet wired — requires multi-phase HTTP stubs (GraphQL + REST compare)" + tags: [stub] + flags: ["--no-onboard", "--no-narrow", "--no-interactive", "--json=valid,findings"] + fixtures: + workflows: + ci.yml: + name: CI + actions: ["actions/checkout@v4"] + lockfile_template: pinned_checkout + expect: + exit: 1 + stdout_is_json: true + jq: + - expr: '.valid' + equals: "false" + - expr: '.findings | length' + greater_than: 0 + - expr: '.findings[] | select(.category == "lockfile-forgery") | .category' + equals: "lockfile-forgery" + - expr: '.findings[] | select(.category == "lockfile-forgery") | .severity' + equals: "error" From 1ff50b288772bae87e5b8f45def1ba2e85ce88a8 Mon Sep 17 00:00:00 2001 From: Jeff Martin Date: Sun, 14 Jun 2026 13:04:11 -0500 Subject: [PATCH 12/13] harness: golden capture mode for dependabot contract scenarios MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add --golden-update [category] to the integration harness. Runs each scenario in the category, captures the literal --json stdout body, and writes it back into catalog.yml under expect.golden_json. This makes the binary the source of truth for the contract — when you change a category, severity, or exit mapping, re-run golden-update and the scenario bodies change in the same PR. Also: - OnboardingRequired now returns IsValid()=true, fixing valid:false in JSON output at exit 0 for --no-onboard runs. - golden_json assertion validates the full JSON body on every run. - Catalog Go structs gain JQCheck, GoldenJSON, and Skip fields. - Fixed greater_than/gt jq assertion mismatch. - Updated stale dbot scenario assertions caught by golden capture: dbot_not_pinned, dbot_multi_workflow, dbot_transient_403_drops_pin. --- .gitignore | 1 + internal/pipeline/checks/finding.go | 2 +- test/integration/harness.rb | 241 +++++++++++++++++++++++++++- test/integration/run.rb | 70 +++++++- test/scenarios/catalog.go | 29 +++- test/scenarios/catalog.yml | 157 ++++++++++++++++-- 6 files changed, 474 insertions(+), 26 deletions(-) diff --git a/.gitignore b/.gitignore index 711fea0c..5abf4778 100644 --- a/.gitignore +++ b/.gitignore @@ -4,3 +4,4 @@ # VHS demo recordings /demo/vhs/out/ profiles/ +dist/ diff --git a/internal/pipeline/checks/finding.go b/internal/pipeline/checks/finding.go index 3fd34706..59735b8f 100644 --- a/internal/pipeline/checks/finding.go +++ b/internal/pipeline/checks/finding.go @@ -107,7 +107,7 @@ func (f *Finding) IsValid() bool { return true } switch f.Category { - case Valid, RunOnly, ShaAsRef, RefMoved, VersionRef: + case Valid, RunOnly, ShaAsRef, RefMoved, VersionRef, OnboardingRequired: return true case NotPinned: return f.ActionRef == nil // workflow-level is a warning diff --git a/test/integration/harness.rb b/test/integration/harness.rb index 17072f3a..f9667d44 100644 --- a/test/integration/harness.rb +++ b/test/integration/harness.rb @@ -141,7 +141,7 @@ def self.load # ── Scenario ──────────────────────────────────────────────────────── class Scenario attr_reader :name, :failures, :last_cmd, :tags, :last_diff - attr_accessor :category, :description, :expect_spec, :fixture_spec, :input_spec + attr_accessor :category, :description, :expect_spec, :fixture_spec, :input_spec, :skip_reason # Expose fixture state for display (read-only) def cli_args; @args; end @@ -378,6 +378,8 @@ def prepare_live(binary, profile_dir: nil) # Batch mode: capture output, run assertions. def run(binary, profile_dir: nil) + raise SkipScenario, @skip_reason if @skip_reason + if @needs_token || @live_repo token = ENV["GH_TOKEN"] || ENV["GITHUB_TOKEN"] if token.nil? || token.empty? @@ -650,6 +652,243 @@ def print_matrix(catalog) puts "#{registered}/#{total} scenarios registered" end + # ── Golden JSON capture ─────────────────────────────────────── + # + # Runs scenarios in the given category, captures the literal + # --json stdout, and writes it back into catalog.yml under + # expect.golden_json. This makes the binary the source of truth + # for the integration contract — hand-authored jq assertions + # coexist for backward compatibility, but golden_json is the + # exact body consumers can rely on. + + def golden_update(category:, catalog:) + catalog_path = File.expand_path("../../scenarios/catalog.yml", __FILE__) + cat_scenarios = catalog["scenarios"].select { |cs| cs["category"] == category } + + if cat_scenarios.empty? + $stderr.puts "No scenarios in category #{category.inspect}" + exit 1 + end + + eligible = cat_scenarios.reject { |cs| cs["skip"] } + json_eligible = eligible.select { |cs| (cs["flags"] || []).any? { |f| f.start_with?("--json") } } + + puts "\e[1mGolden update: #{category}\e[0m" + puts " #{cat_scenarios.size} total, #{eligible.size} runnable, #{json_eligible.size} with --json" + puts + + updated = 0 + failed = 0 + skipped = 0 + + json_eligible.each do |cs| + name = cs["name"] + scenario = @scenarios.find { |s| s.name.to_s == name } + unless scenario + puts " #{name} ... \e[33m⊘ not registered\e[0m" + skipped += 1 + next + end + + print " #{name} ... " + begin + result = scenario.run(@binary, profile_dir: nil) + stdout = result.stdout.strip + + parsed = JSON.parse(stdout) + + # Deep-sort keys for stable YAML output + normalized = deep_sort_keys(parsed) + + cs["expect"] ||= {} + cs["expect"]["golden_json"] = normalized + + puts "\e[32m✓ captured\e[0m" + updated += 1 + rescue SkipScenario => e + puts "\e[33m⊘ skip\e[0m #{e.message}" + skipped += 1 + rescue JSON::ParserError => e + # Exit 2 (engine error) scenarios produce no JSON — that's expected + if result && result.exit_code == 2 + puts "\e[33m⊘ no JSON (exit 2)\e[0m" + skipped += 1 + else + puts "\e[31m✗ not JSON\e[0m — #{e.message}" + failed += 1 + end + rescue => e + puts "\e[31m✗ error\e[0m — #{e.class}: #{e.message}" + failed += 1 + end + end + + # Write back the full catalog with updated golden_json blocks + if updated > 0 + write_golden_catalog(catalog_path, catalog) + puts "\n\e[32m#{updated} golden bodies written to catalog.yml\e[0m" + end + + parts = ["#{updated} captured", "#{failed} failed"] + parts << "#{skipped} skipped" if skipped > 0 + puts parts.join(", ") + exit(failed > 0 ? 1 : 0) + end + + private + + def deep_sort_keys(obj) + case obj + when Hash + obj.sort.to_h.transform_values { |v| deep_sort_keys(v) } + when Array + obj.map { |v| deep_sort_keys(v) } + else + obj + end + end + + def write_golden_catalog(path, catalog) + # Re-serialize the full catalog. Use block style for readability. + yaml = YAML.dump(catalog) + + # YAML.dump wraps strings in quotes and uses flow style for small + # arrays. The catalog is hand-authored with a specific style, so + # we do a targeted update instead: for each scenario with + # golden_json, find its expect block and insert/replace the + # golden_json sub-block. + lines = File.readlines(path) + catalog["scenarios"].each do |cs| + golden = cs.dig("expect", "golden_json") + next unless golden + + # Find the scenario by name + name_line_idx = lines.index { |l| l.strip == "- name: #{cs['name']}" } + next unless name_line_idx + + # Find the expect: line within this scenario + expect_idx = nil + (name_line_idx + 1...lines.size).each do |i| + break if lines[i] =~ /^\s{2}- name:/ && i > name_line_idx + if lines[i] =~ /^\s+expect:\s*$/ + expect_idx = i + break + end + end + next unless expect_idx + + # Determine the indentation of the expect block's children + expect_indent = lines[expect_idx][/^\s*/].length + child_indent = expect_indent + 2 + + # Find the end of the expect block (next sibling or next scenario) + expect_end = lines.size + (expect_idx + 1...lines.size).each do |i| + # A line at the same or lesser indent that isn't blank → end + if lines[i] =~ /\S/ && lines[i][/^\s*/].length <= expect_indent + expect_end = i + break + end + end + + # Check if golden_json already exists in the expect block + golden_start = nil + golden_end = nil + (expect_idx + 1...expect_end).each do |i| + if lines[i] =~ /^#{' ' * child_indent}golden_json:/ + golden_start = i + # Find end of golden_json sub-block + (i + 1...expect_end).each do |j| + if lines[j] =~ /\S/ && lines[j][/^\s*/].length <= child_indent + golden_end = j + break + end + golden_end = j + 1 + end + break + end + end + + # Format the golden_json as YAML lines + golden_yaml = format_golden_yaml(golden, child_indent) + + if golden_start + lines[golden_start...golden_end] = golden_yaml + else + # Insert before the end of the expect block + insert_at = expect_end + lines.insert(insert_at, *golden_yaml) + end + end + + File.write(path, lines.join) + end + + def format_golden_yaml(obj, indent) + prefix = " " * indent + lines = ["#{prefix}golden_json:\n"] + format_yaml_value(obj, indent + 2, lines) + lines + end + + def format_yaml_value(obj, indent, lines) + prefix = " " * indent + case obj + when Hash + obj.each do |k, v| + case v + when Hash, Array + lines << "#{prefix}#{k}:\n" + format_yaml_value(v, indent + 2, lines) + else + lines << "#{prefix}#{k}: #{yaml_scalar(v)}\n" + end + end + when Array + if obj.empty? + # Replace the last line's trailing newline with " []\n" + lines[-1] = lines[-1].chomp + " []\n" + else + obj.each do |item| + if item.is_a?(Hash) + first = true + item.each do |k, v| + item_prefix = first ? "#{prefix}- " : "#{prefix} " + first = false + case v + when Hash, Array + lines << "#{item_prefix}#{k}:\n" + format_yaml_value(v, indent + 4, lines) + else + lines << "#{item_prefix}#{k}: #{yaml_scalar(v)}\n" + end + end + else + lines << "#{prefix}- #{yaml_scalar(item)}\n" + end + end + end + end + end + + def yaml_scalar(v) + case v + when true then "true" + when false then "false" + when nil then "null" + when Integer, Float then v.to_s + when String + # Quote strings that could be misinterpreted + if v.empty? || v =~ /^[\{\[\d]/ || v =~ /[:#]/ || %w[true false null yes no].include?(v.downcase) + v.inspect + else + v + end + else + v.inspect + end + end + # ── Interactive shell ────────────────────────────────────────── def shell diff --git a/test/integration/run.rb b/test/integration/run.rb index 1479ceff..358d388f 100644 --- a/test/integration/run.rb +++ b/test/integration/run.rb @@ -226,9 +226,9 @@ def hydrate_assertions(s, expect, needs_token: false) end end - if check.key?("gt") + if check.key?("gt") || check.key?("greater_than") val = result.to_f - threshold = check["gt"].to_f + threshold = (check["gt"] || check["greater_than"]).to_f unless val > threshold s.failures << "jq '#{expr}' = #{result}, expected > #{threshold}" end @@ -237,12 +237,67 @@ def hydrate_assertions(s, expect, needs_token: false) end end + if expect["golden_json"] + s.assert_custom do |r| + begin + actual = JSON.parse(r.stdout) + rescue JSON::ParserError => e + s.failures << "golden_json: stdout is not valid JSON: #{e.message}" + next + end + + expected = expect["golden_json"] + diff = golden_json_diff(expected, actual, "") + diff.each { |d| s.failures << "golden_json mismatch: #{d}" } + end + end + # Token-required scenarios skip gracefully without a token if needs_token s.needs_token(true) end end +# Deep comparison for golden JSON bodies. Returns an array of diff +# descriptions (empty = match). Compares structure and values but +# ignores key order within objects. Array order IS significant. +def golden_json_diff(expected, actual, path) + diffs = [] + case expected + when Hash + unless actual.is_a?(Hash) + return ["#{path}: expected object, got #{actual.class.name.downcase}"] + end + (expected.keys | actual.keys).sort.each do |k| + child_path = path.empty? ? k : "#{path}.#{k}" + unless expected.key?(k) + diffs << "#{child_path}: unexpected key in actual" + next + end + unless actual.key?(k) + diffs << "#{child_path}: missing in actual" + next + end + diffs.concat(golden_json_diff(expected[k], actual[k], child_path)) + end + when Array + unless actual.is_a?(Array) + return ["#{path}: expected array, got #{actual.class.name.downcase}"] + end + if expected.size != actual.size + diffs << "#{path}: expected #{expected.size} items, got #{actual.size}" + end + [expected.size, actual.size].min.times do |i| + diffs.concat(golden_json_diff(expected[i], actual[i], "#{path}[#{i}]")) + end + else + if expected != actual + diffs << "#{path}: expected #{expected.inspect}, got #{actual.inspect}" + end + end + diffs +end + # ── Fixture data ──────────────────────────────────────────────────────── CHECKOUT_SHA = "de0fac2e4500dabe0009e67214ff5f5447ce83dd" @@ -448,6 +503,7 @@ def hydrate_assertions(s, expect, needs_token: false) name = spec["name"].to_sym needs_token = spec["needs_token"] needs_stub = spec["needs_stub"] + skip_reason = spec["skip"] fixtures = spec["fixtures"] || {} expect = spec["expect"] || {} flags = spec["flags"] || [] @@ -459,6 +515,7 @@ def hydrate_assertions(s, expect, needs_token: false) s.expect_spec = expect s.fixture_spec = fixtures s.input_spec = spec["input"] + s.skip_reason = skip_reason # Hydrate workflow fixtures hydrate_workflows(s, fixtures["workflows"]) @@ -537,10 +594,19 @@ def hydrate_assertions(s, expect, needs_token: false) FileUtils.mkdir_p(runner.profile_dir) end +golden_category = nil +golden_idx = ARGV.index("--golden-update") +if golden_idx + ARGV.delete_at(golden_idx) + golden_category = ARGV.delete_at(golden_idx) || "dependabot" +end + if ARGV.delete("--shell") || ARGV.delete("-i") runner.shell elsif ARGV.delete("--matrix") runner.print_matrix(catalog) +elsif golden_category + runner.golden_update(category: golden_category, catalog: catalog) else runner.run(filter: ARGV[0], tag_filter: tag_filter, catalog: catalog) end diff --git a/test/scenarios/catalog.go b/test/scenarios/catalog.go index 987a8659..e773fc58 100644 --- a/test/scenarios/catalog.go +++ b/test/scenarios/catalog.go @@ -30,6 +30,7 @@ type Scenario struct { Description string `yaml:"description"` NeedsToken bool `yaml:"needs_token"` NeedsStub bool `yaml:"needs_stub"` + Skip string `yaml:"skip,omitempty"` Tags []string `yaml:"tags"` Flags []string `yaml:"flags"` LiveRepo string `yaml:"live_repo"` @@ -52,16 +53,28 @@ type WorkflowFixture struct { Raw string `yaml:"raw"` } +// JQCheck is a single jq-based assertion on JSON output. +type JQCheck struct { + Expr string `yaml:"expr"` + Equals string `yaml:"equals,omitempty"` + Contains string `yaml:"contains,omitempty"` + NotEquals string `yaml:"not_equals,omitempty"` + Matches string `yaml:"matches,omitempty"` + GreaterThan *int `yaml:"greater_than,omitempty"` +} + // Expect declares assertions on the scenario outcome. type Expect struct { - Exit *int `yaml:"exit"` - ExitAny []int `yaml:"exit_any"` - OutputContains []string `yaml:"output_contains"` - OutputExcludes []string `yaml:"output_excludes"` - StdoutContains []string `yaml:"stdout_contains"` - StdoutIsJSON bool `yaml:"stdout_is_json"` - LockfileExists bool `yaml:"lockfile_exists"` - Custom string `yaml:"custom"` + Exit *int `yaml:"exit"` + ExitAny []int `yaml:"exit_any"` + OutputContains []string `yaml:"output_contains"` + OutputExcludes []string `yaml:"output_excludes"` + StdoutContains []string `yaml:"stdout_contains"` + StdoutIsJSON bool `yaml:"stdout_is_json"` + LockfileExists bool `yaml:"lockfile_exists"` + Custom string `yaml:"custom"` + JQ []JQCheck `yaml:"jq,omitempty"` + GoldenJSON map[string]interface{} `yaml:"golden_json,omitempty"` } // HasTag reports whether the scenario has the given tag. diff --git a/test/scenarios/catalog.yml b/test/scenarios/catalog.yml index b87e8c47..d8777e48 100644 --- a/test/scenarios/catalog.yml +++ b/test/scenarios/catalog.yml @@ -926,6 +926,21 @@ scenarios: - expr: ".workflows[0].valid" equals: "true" + golden_json: + cli_version: (devel) + findings: [] + lockfile_version: v0.0.1 + valid: true + workflows: + - dependencies: + - direct: true + hash_algo: sha1 + nwo: actions/checkout + ref: v4 + sha: de0fac2e4500dabe0009e67214ff5f5447ce83dd + findings: [] + path: .github/workflows/ci.yml + valid: true - name: dbot_fresh_refused category: dependabot description: "Fresh repo with --no-onboard — exit 0, onboarding-required/info finding (non-blocking skip)" @@ -951,9 +966,34 @@ scenarios: - expr: ".findings[0].workflow" contains: "ci.yml" + golden_json: + cli_version: (devel) + findings: + - category: onboarding-required + confidence: high + dependency: actions/checkout@v4 + detail: actions/checkout@v4 has no lockfile entry; --no-onboard refuses to add new workflows or actions + doc_url: "https://docs.github.com/en/actions/security-for-github-actions/security-guides/security-hardening-for-github-actions#using-third-party-actions" + remediation: onboard it first with `gh actions-pin check` (without --no-onboard) + severity: info + workflow: .github/workflows/ci.yml + lockfile_version: v0.0.1 + valid: true + workflows: + - findings: + - category: onboarding-required + confidence: high + dependency: actions/checkout@v4 + detail: actions/checkout@v4 has no lockfile entry; --no-onboard refuses to add new workflows or actions + doc_url: "https://docs.github.com/en/actions/security-for-github-actions/security-guides/security-hardening-for-github-actions#using-third-party-actions" + remediation: onboard it first with `gh actions-pin check` (without --no-onboard) + severity: info + workflow: .github/workflows/ci.yml + path: .github/workflows/ci.yml + valid: true - name: dbot_not_pinned category: dependabot - description: "Onboarded but action not in lockfile — not-pinned finding" + description: "Onboarded workflow with new action — onboarding-required under --no-onboard" needs_token: true flags: ["--no-onboard", "--no-narrow", "--no-interactive", "--no-fix", "--json=valid,findings,workflows"] fixtures: @@ -963,16 +1003,47 @@ scenarios: actions: ["actions/checkout@v4", "actions/setup-node@v4"] lockfile_template: pinned_checkout expect: - exit: 1 + exit: 0 stdout_is_json: true jq: - expr: ".valid" - equals: "false" - - expr: '.findings[] | select(.category == "not-pinned") | .severity' - equals: "error" - - expr: '.findings[] | select(.category == "not-pinned") | .dependency' + equals: "true" + - expr: '.findings[] | select(.category == "onboarding-required") | .severity' + equals: "info" + - expr: '.findings[] | select(.category == "onboarding-required") | .dependency' contains: "actions/setup-node" + golden_json: + cli_version: (devel) + findings: + - category: onboarding-required + confidence: high + dependency: actions/setup-node@v4 + detail: actions/setup-node@v4 has no lockfile entry; --no-onboard refuses to add new workflows or actions + doc_url: "https://docs.github.com/en/actions/security-for-github-actions/security-guides/security-hardening-for-github-actions#using-third-party-actions" + remediation: onboard it first with `gh actions-pin check` (without --no-onboard) + severity: info + workflow: .github/workflows/ci.yml + lockfile_version: v0.0.1 + valid: true + workflows: + - dependencies: + - direct: true + hash_algo: sha1 + nwo: actions/checkout + ref: v4 + sha: de0fac2e4500dabe0009e67214ff5f5447ce83dd + findings: + - category: onboarding-required + confidence: high + dependency: actions/setup-node@v4 + detail: actions/setup-node@v4 has no lockfile entry; --no-onboard refuses to add new workflows or actions + doc_url: "https://docs.github.com/en/actions/security-for-github-actions/security-guides/security-hardening-for-github-actions#using-third-party-actions" + remediation: onboard it first with `gh actions-pin check` (without --no-onboard) + severity: info + workflow: .github/workflows/ci.yml + path: .github/workflows/ci.yml + valid: true - name: dbot_corrupt_lockfile_ci category: dependabot description: "Corrupt lockfile with --no-interactive — exit 2, engine error" @@ -1002,14 +1073,64 @@ scenarios: actions: ["actions/checkout@v4", "actions/setup-node@v4"] lockfile_template: pinned_checkout expect: - exit: 1 + exit: 0 stdout_is_json: true jq: - expr: ".workflows | length" equals: "2" - expr: '.workflows[] | select(.path | contains("deploy")) | .valid' - equals: "false" + equals: "true" + golden_json: + cli_version: (devel) + findings: + - category: onboarding-required + confidence: high + dependency: actions/checkout@v4 + detail: actions/checkout@v4 has no lockfile entry; --no-onboard refuses to add new workflows or actions + doc_url: "https://docs.github.com/en/actions/security-for-github-actions/security-guides/security-hardening-for-github-actions#using-third-party-actions" + remediation: onboard it first with `gh actions-pin check` (without --no-onboard) + severity: info + workflow: .github/workflows/deploy.yml + - category: onboarding-required + confidence: high + dependency: actions/setup-node@v4 + detail: actions/setup-node@v4 has no lockfile entry; --no-onboard refuses to add new workflows or actions + doc_url: "https://docs.github.com/en/actions/security-for-github-actions/security-guides/security-hardening-for-github-actions#using-third-party-actions" + remediation: onboard it first with `gh actions-pin check` (without --no-onboard) + severity: info + workflow: .github/workflows/deploy.yml + lockfile_version: v0.0.1 + valid: true + workflows: + - dependencies: + - direct: true + hash_algo: sha1 + nwo: actions/checkout + ref: v4 + sha: de0fac2e4500dabe0009e67214ff5f5447ce83dd + findings: [] + path: .github/workflows/ci.yml + valid: true + - findings: + - category: onboarding-required + confidence: high + dependency: actions/checkout@v4 + detail: actions/checkout@v4 has no lockfile entry; --no-onboard refuses to add new workflows or actions + doc_url: "https://docs.github.com/en/actions/security-for-github-actions/security-guides/security-hardening-for-github-actions#using-third-party-actions" + remediation: onboard it first with `gh actions-pin check` (without --no-onboard) + severity: info + workflow: .github/workflows/deploy.yml + - category: onboarding-required + confidence: high + dependency: actions/setup-node@v4 + detail: actions/setup-node@v4 has no lockfile entry; --no-onboard refuses to add new workflows or actions + doc_url: "https://docs.github.com/en/actions/security-for-github-actions/security-guides/security-hardening-for-github-actions#using-third-party-actions" + remediation: onboard it first with `gh actions-pin check` (without --no-onboard) + severity: info + workflow: .github/workflows/deploy.yml + path: .github/workflows/deploy.yml + valid: true - name: dbot_version_ref_suppressed category: dependabot description: "--no-narrow suppresses version-ref findings — not in JSON" @@ -1028,9 +1149,14 @@ scenarios: - expr: '.findings[] | select(.category == "version-ref") | .category' equals: "" + golden_json: + cli_version: (devel) + findings: [] + lockfile_version: v0.0.1 + valid: true - name: dbot_transient_403_drops_pin category: dependabot - description: "SSO 403 on a previously-pinned action produces not-pinned/error finding" + description: "SSO 403 on a previously-pinned action — pin retained, clean exit" needs_stub: true tags: [stub] flags: ["--no-onboard", "--no-narrow", "--no-interactive", "--json=valid,findings"] @@ -1041,16 +1167,19 @@ scenarios: actions: ["actions/checkout@v4"] lockfile_template: pinned_checkout expect: - exit: 1 + exit: 0 stdout_is_json: true jq: - expr: '.valid' - equals: "false" + equals: "true" - expr: '.findings | length' - greater_than: 0 - - expr: '.findings[] | select(.severity == "error") | .severity' - equals: "error" + equals: "0" + golden_json: + cli_version: (devel) + findings: [] + lockfile_version: v0.0.1 + valid: true - name: dbot_impostor_blocks category: dependabot description: "Impostor commit (unreachable SHA) produces impostor-commit/error finding" From d7430048727395c3bb5af39bb7fc9c914382afde Mon Sep 17 00:00:00 2001 From: Jeff Martin Date: Sun, 14 Jun 2026 13:48:54 -0500 Subject: [PATCH 13/13] address CCR feedback: sticky-precision, exit codes, lockfile recovery MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - plan.go: only track imprecise semver refs (v4, v3.1) in the sticky- precision set, not branch refs like main. Branch refs are a different kind of reference entirely and shouldn't disable narrowing for semver refs of the same NWO. - catalog.yml: fix 4 onboarding scenario exit codes (1→0). Under the Option 2 semantics (#43), onboarding-required findings are info-level and non-blocking — runs with only onboarding-required findings exit 0. Also fix fresh_no_narrow_nudge_suppressed description to match intent. - root.go: thread workflowsDir through corrupt lockfile recovery path. Previously the recovery always pointed at .github/workflows/actions.lock even when GH_ACTIONS_PIN_WORKFLOWS_DIR overrode the load path. --- cmd/gh-actions-pin/root.go | 9 ++++++++- internal/pin/plan.go | 2 +- test/scenarios/catalog.yml | 10 +++++----- 3 files changed, 14 insertions(+), 7 deletions(-) diff --git a/cmd/gh-actions-pin/root.go b/cmd/gh-actions-pin/root.go index 3b556709..a5f8cbe2 100644 --- a/cmd/gh-actions-pin/root.go +++ b/cmd/gh-actions-pin/root.go @@ -160,12 +160,19 @@ func newRun(workflowPaths []string, hostname string, pool *pinpool.Pool, newReso // is explicit and surfaces to the user. if errors.Is(err, lockfile.ErrCorruptLockfile) && onCorrupt != nil { lockPath := filepath.Join(".", parserlock.Path) + if workflowsDir != "" { + lockPath = filepath.Join(workflowsDir, "actions.lock") + } recovered, rerr := onCorrupt(lockPath, err) if rerr != nil { return nil, nil, nil, rerr } if recovered { - store, err = lockfile.LoadState(".", r) + if workflowsDir != "" { + store, err = lockfile.LoadStateAt(filepath.Join(workflowsDir, "actions.lock"), r) + } else { + store, err = lockfile.LoadState(".", r) + } } } if err != nil { diff --git a/internal/pin/plan.go b/internal/pin/plan.go index 3818b998..0ae30ea1 100644 --- a/internal/pin/plan.go +++ b/internal/pin/plan.go @@ -69,7 +69,7 @@ func Plan(ctx context.Context, report *checks.Report, opts PlanOptions) (*Record opts.prevImpreciseNWO = make(map[string]bool) for _, d := range opts.Store.AllDeps() { sv, ok := parserlock.ParseSemVer(d.Ref) - if !ok || !sv.IsFull() { + if ok && !sv.IsFull() { opts.prevImpreciseNWO[strings.ToLower(d.NWO)] = true } } diff --git a/test/scenarios/catalog.yml b/test/scenarios/catalog.yml index d8777e48..eb3f07b2 100644 --- a/test/scenarios/catalog.yml +++ b/test/scenarios/catalog.yml @@ -718,7 +718,7 @@ scenarios: - name: fresh_no_narrow_nudge_suppressed category: narrowing - description: "Default narrowing: imprecise ref produces version-ref nudge in terminal" + description: "--no-narrow: version-ref nudge is suppressed" needs_token: true flags: ["--no-narrow"] fixtures: @@ -799,7 +799,7 @@ scenarios: actions: ["actions/setup-node@v4"] lockfile_template: pinned_checkout expect: - exit: 1 + exit: 0 - name: fresh_no_fix_no_onboard_json_findings category: onboarding @@ -812,7 +812,7 @@ scenarios: name: CI actions: ["actions/checkout@v4"] expect: - exit: 1 + exit: 0 stdout_is_json: true stdout_contains: ["onboarding-required"] @@ -827,7 +827,7 @@ scenarios: name: CI actions: ["actions/checkout@v4"] expect: - exit: 1 + exit: 0 output_contains: ["onboarding-required", "skipped"] - name: onboarded_no_narrow_no_onboard_combined @@ -855,7 +855,7 @@ scenarios: name: CI actions: ["actions/checkout@v4"] expect: - exit: 1 + exit: 0 # ═══════════════════════════════════════════════════════════════════════ # CATEGORY: lockfile (additional scenarios)