From 97cb06de85e14d9e8b250c9d565bfd902815c6a1 Mon Sep 17 00:00:00 2001 From: Jeff Martin Date: Mon, 31 Aug 2026 13:35:47 -0700 Subject: [PATCH 01/20] actions-lock: resolve dependencies live by default --- README.md | 7 +- cmd/gh-actions-lock/check_json_golden_test.go | 2 +- cmd/gh-actions-lock/command_test.go | 109 +++--- cmd/gh-actions-lock/format/terminal.go | 12 +- cmd/gh-actions-lock/format/terminal_test.go | 19 +- cmd/gh-actions-lock/pin_summary.go | 42 ++- cmd/gh-actions-lock/pin_summary_test.go | 22 ++ cmd/gh-actions-lock/prune_workflow_test.go | 21 +- cmd/gh-actions-lock/run.go | 42 +-- cmd/gh-actions-lock/verify.go | 9 +- cmd/gh-actions-lock/verify_test.go | 33 +- internal/pipeline/checks/finding.go | 15 - internal/pipeline/checks/misleading.go | 2 +- internal/pipeline/checks/parsed.go | 73 +--- internal/pipeline/checks/parsed_test.go | 29 -- internal/pipeline/parse.go | 20 +- internal/pipeline/run.go | 108 +----- internal/pipeline/run_test.go | 318 ------------------ internal/resolve/resolver.go | 19 -- test/integration/harness.rb | 13 +- test/integration/run.rb | 5 - test/scenarios/catalog.yml | 68 +--- 22 files changed, 186 insertions(+), 802 deletions(-) delete mode 100644 internal/pipeline/checks/parsed_test.go delete mode 100644 internal/pipeline/run_test.go diff --git a/README.md b/README.md index 56fcec95..f7793fd6 100644 --- a/README.md +++ b/README.md @@ -35,9 +35,10 @@ After the initial run to onboard workflows, you will need to run `gh actions-loc A full-directory run (`gh actions-lock` with no path arguments) also prunes lockfile entries for workflows that have been deleted from `.github/workflows/`, dropping any dependencies left orphaned by the removal. Scoped runs that name specific workflows never prune out-of-scope entries. -Pins to branches or partial versions (e.g. `main`, `v4`) are trusted from the -lockfile and not re-resolved on a normal run. To bump them to the current -upstream commit, run: +Normal networked runs live-resolve the dependency closure for the workflows in +scope. If a branch or partial version (e.g. `main`, `v4`) has moved, the command +reports the movement but retains the recorded commit. To permit advancing those +pins to the current upstream commit, run: ```bash gh actions-lock --relock diff --git a/cmd/gh-actions-lock/check_json_golden_test.go b/cmd/gh-actions-lock/check_json_golden_test.go index 996827b3..416704e4 100644 --- a/cmd/gh-actions-lock/check_json_golden_test.go +++ b/cmd/gh-actions-lock/check_json_golden_test.go @@ -94,7 +94,7 @@ func TestCheckCommand_JSONGolden(t *testing.T) { stdout, _, err := runCommandWithHTTP(t, reg, // The fixture's lockfile addresses the workflow as // .github/workflows/ci.yml, so we run check on that exact path. - "--rescan", "--no-fix", "--json=valid,findings,workflows,dependencies", + "--no-fix", "--json=valid,findings,workflows,dependencies", ".github/workflows/ci.yml", ) // We expect findings (ref-changed + stale), so the command exits diff --git a/cmd/gh-actions-lock/command_test.go b/cmd/gh-actions-lock/command_test.go index 11cc69a0..9db180b6 100644 --- a/cmd/gh-actions-lock/command_test.go +++ b/cmd/gh-actions-lock/command_test.go @@ -54,7 +54,7 @@ jobs: ) stdout, _, err := runCommandWithHTTP(t, reg, - "--rescan", "--no-fix", "--json=valid,findings", workflowPath, + "--no-fix", "--json=valid,findings", workflowPath, ) require.NoError(t, err) @@ -244,7 +244,7 @@ jobs: ) stdout, _, err := runCommandWithHTTP(t, reg, - "--rescan", "--no-fix", "--json=valid,findings", workflowPath, + "--no-fix", "--json=valid,findings", workflowPath, ) require.NoError(t, err) @@ -310,7 +310,7 @@ jobs: ) stdout, _, err := runCommandWithHTTP(t, reg, - "--rescan", "--no-fix", "--json=valid,findings", workflowPath, + "--no-fix", "--json=valid,findings", workflowPath, ) require.ErrorIs(t, err, errSilent, "JSON mode should exit non-zero for forgery findings") @@ -372,7 +372,7 @@ jobs: ) stdout, _, err := runCommandWithHTTP(t, reg, - "--rescan", "--no-fix", "--json=valid,findings", workflowPath, + "--no-fix", "--json=valid,findings", workflowPath, ) require.NoError(t, err, "ref-moved is a warning, should not error") @@ -430,7 +430,7 @@ jobs: ) stdout, _, err := runCommandWithHTTP(t, reg, - "--rescan", "--no-fix", "--json=valid,findings", workflowPath, + "--no-fix", "--json=valid,findings", workflowPath, ) require.NoError(t, err, "ref-moved is a warning, should not error") @@ -509,7 +509,7 @@ jobs: // Test per-workflow dependencies view stdout, _, err := runCommandWithHTTP(t, reg, - "--rescan", "--no-fix", "--json=workflows", workflowPath, + "--no-fix", "--json=workflows", workflowPath, ) require.NoError(t, err) @@ -584,7 +584,7 @@ jobs: ) stdout, _, err := runCommandWithHTTP(t, reg, - "--rescan", "--no-fix", "--json=workflows", workflowPath, + "--no-fix", "--json=workflows", workflowPath, ) require.NoError(t, err) @@ -635,7 +635,7 @@ jobs: // --json with no value should use the default fields (valid,findings,workflows) stdout, _, err := runCommandWithHTTP(t, reg, - "--rescan", "--no-fix", "--json", workflowPath, + "--no-fix", "--json", workflowPath, ) require.NoError(t, err) @@ -650,25 +650,22 @@ jobs: assert.NotContains(t, raw, "dependencies", "default --json should not include top-level dependencies to avoid duplication with workflows") } -// TestCheck_SeedFromLockfile_SkipsHTTPForCachedDeps verifies that -// SeedFromLockfile pre-warms the resolution cache so known deps skip -// network calls, while new deps still resolve from the network. -// The workflow has two deps: checkout (in lockfile) and setup-go (not in -// lockfile). Only setup-go should hit the HTTP mock. -func TestCheck_SeedFromLockfile_SkipsHTTPForCachedDeps(t *testing.T) { +// TestCheck_DefaultRun_ResolvesRecordedAndNewDeps proves existing lockfile +// entries do not suppress live resolution. +func TestCheck_DefaultRun_ResolvesRecordedAndNewDeps(t *testing.T) { reg := &httpmock.Registry{} defer reg.Verify(t) checkoutSHA := "de0fac2e4500dabe0009e67214ff5f5447ce83dd" setupGoSHA := "4a3601121dd01d1626a1e23e37211e3254c1c06c" - // Only register an HTTP stub for setup-go (the NEW dep). - // No stub for checkout — the seed must serve it from cache. + // Both the recorded and new dependency must resolve live. reg.Register( - httpmock.GraphQLForRepo("actions", "setup-go"), + httpmock.GraphQLForRepo("actions", "checkout"), httpmock.JSONResponse(map[string]any{ "data": map[string]any{ - "a0": testRepoResponse("actions/setup-go", setupGoSHA, nodeActionYAML), + "a0": testRepoResponse("actions/checkout", checkoutSHA, nodeActionYAML), + "a1": testRepoResponse("actions/setup-go", setupGoSHA, nodeActionYAML), }, }), ) @@ -699,14 +696,11 @@ jobs: require.NoError(t, os.WriteFile(filepath.Join(dir, ".github", "workflows", "actions.lock"), []byte(lockYAML), 0o600)) t.Chdir(dir) - // Run WITHOUT --rescan so SeedFromLockfile is active. stdout, _, err := runCommandWithHTTP(t, reg, "--no-fix", "--json=valid,findings", ".github/workflows/workflow.yml", ) // setup-go is resolved but not yet pinned → "not-pinned" finding → errSilent. - // That's expected: we're testing that checkout was served from cache, not - // that the overall check passes. require.ErrorIs(t, err, errSilent) var payload struct { @@ -715,8 +709,7 @@ jobs: } require.NoError(t, json.Unmarshal([]byte(stdout), &payload)) - // The finding should be about setup-go being unpinned, NOT about checkout. - // If checkout required an HTTP call, reg.Verify would fail (no stub registered). + // The finding should be about setup-go being unpinned, not checkout. require.Len(t, payload.Findings, 1) assert.Equal(t, "not-pinned", payload.Findings[0].Category) assert.Contains(t, payload.Findings[0].Dependency, "setup-go") @@ -733,13 +726,13 @@ func TestCheck_NoFix_WritesNothing(t *testing.T) { checkoutSHA := "de0fac2e4500dabe0009e67214ff5f5447ce83dd" setupGoSHA := "4a3601121dd01d1626a1e23e37211e3254c1c06c" - // setup-go is the unpinned (new) dep — it resolves from the mock. checkout - // is seeded from the lockfile and must not hit the network. + // Both the recorded and new dependency resolve from the mock. reg.Register( - httpmock.GraphQLForRepo("actions", "setup-go"), + httpmock.GraphQLForRepo("actions", "checkout"), httpmock.JSONResponse(map[string]any{ "data": map[string]any{ - "a0": testRepoResponse("actions/setup-go", setupGoSHA, nodeActionYAML), + "a0": testRepoResponse("actions/checkout", checkoutSHA, nodeActionYAML), + "a1": testRepoResponse("actions/setup-go", setupGoSHA, nodeActionYAML), }, }), ) @@ -852,11 +845,9 @@ jobs: assert.Contains(t, pins, "actions/setup-go", "autofix should have pinned setup-go") } -// TestCheck_Rescan_DetectsRefMovementDespiteLockfile is a regression test -// ensuring --rescan does NOT seed the resolution cache. If seeding occurred, -// the resolver would return the stale lockfile SHA and the ref-moved finding -// would be suppressed — exactly the bug we fixed. -func TestCheck_Rescan_DetectsRefMovementDespiteLockfile(t *testing.T) { +// TestCheck_DefaultRun_DetectsRefMovementDespiteLockfile proves recorded +// mutable refs resolve live on every networked run. +func TestCheck_DefaultRun_DetectsRefMovementDespiteLockfile(t *testing.T) { reg := &httpmock.Registry{} defer reg.Verify(t) @@ -897,7 +888,7 @@ jobs: ) stdout, _, err := runCommandWithHTTP(t, reg, - "--rescan", "--no-fix", "--json=valid,findings", workflowPath, + "--no-fix", "--json=valid,findings", workflowPath, ) // ref-moved is a warning (valid=true), not an error. require.NoError(t, err, "ref-moved is a warning, should not error") @@ -915,10 +906,7 @@ jobs: hasRefMoved = true } } - assert.True(t, hasRefMoved, - "--rescan must detect ref movement (stale lockfile SHA vs live SHA); "+ - "if this fails, SeedFromLockfile is poisoning the resolution cache during rescan: %+v", - payload.Findings) + assert.True(t, hasRefMoved, "default runs must detect ref movement: %+v", payload.Findings) } func TestCheckCommand_JSONDeduplicatesDependencies(t *testing.T) { @@ -927,9 +915,15 @@ func TestCheckCommand_JSONDeduplicatesDependencies(t *testing.T) { reg := &httpmock.Registry{} defer reg.Verify(t) - // No HTTP stub: both workflows are fully recorded in the lockfile, so - // the fast path skips every network round-trip. The dedup logic under - // test operates purely on the inventory built from disk. + checkoutSHA := "de0fac2e4500dabe0009e67214ff5f5447ce83dd" + reg.Register( + httpmock.GraphQLForRepo("actions", "checkout"), + httpmock.JSONResponse(map[string]any{ + "data": map[string]any{ + "a0": testRepoResponse("actions/checkout", checkoutSHA, nodeActionYAML), + }, + }), + ) wf1 := writeTempWorkflow(t, ` name: ci @@ -1063,9 +1057,8 @@ func TestCheckCommand_LoadErrorFailsFixMode(t *testing.T) { } // TestCheck_Relock_BumpsMovedBranchRef covers github/actions-dispatch#751: -// a branch ref (main) whose upstream head advanced is trusted as-is on a -// normal run and merely flagged ref-moved under --rescan. --relock must -// re-resolve it and rewrite the lockfile to the new live SHA. +// a branch ref (main) whose upstream head advanced is detected on a normal +// run. --relock permits rewriting the lockfile to the new live SHA. func TestCheck_Relock_BumpsMovedBranchRef(t *testing.T) { reg := &httpmock.Registry{} @@ -1125,13 +1118,31 @@ jobs: } // TestCheck_DefaultRun_DoesNotBumpBranchRef is the counterpart to the relock -// test: without --relock a mutable branch ref is trusted from the lockfile -// (fast path, no network) and its recorded SHA is left as-is. +// test: without --relock a moved branch ref is detected but its recorded SHA +// is left as-is. func TestCheck_DefaultRun_DoesNotBumpBranchRef(t *testing.T) { reg := &httpmock.Registry{} - defer reg.Verify(t) // no stubs should be hit on the fast path + defer reg.Verify(t) staleSHA := "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" + liveSHA := "bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb" + reg.Register( + httpmock.GraphQLForRepo("example", "action"), + httpmock.JSONResponse(map[string]any{ + "data": map[string]any{ + "a0": testRepoResponse("example/action", liveSHA, nodeActionYAML), + }, + }), + ) + reg.Register( + httpmock.REST("GET", "repos/example/action/compare/"), + httpmock.JSONResponse(map[string]any{ + "status": "ahead", + "merge_base_commit": map[string]any{ + "sha": staleSHA, + }, + }), + ) workflowPath := writeTempWorkflow(t, ` name: ci @@ -1145,9 +1156,11 @@ jobs: "example/action@main=sha1-"+staleSHA, ) - _, _, err := runCommandWithHTTP(t, reg, workflowPath) + _, stderr, err := runCommandWithHTTP(t, reg, workflowPath) require.NoError(t, err) + assert.Contains(t, stderr, "gh actions-lock --relock") + assert.NotContains(t, stderr, "All 1 workflow valid") assert.Contains(t, readTempLockfilePins(t), staleSHA, - "a default run must not bump a trusted branch ref") + "a default run must not bump a moved branch ref without --relock") } diff --git a/cmd/gh-actions-lock/format/terminal.go b/cmd/gh-actions-lock/format/terminal.go index 818bee11..d62102e1 100644 --- a/cmd/gh-actions-lock/format/terminal.go +++ b/cmd/gh-actions-lock/format/terminal.go @@ -360,16 +360,6 @@ func renderWarnings(out *ui.UI, report *checks.Report, willRemediate bool) { if !isTransitive { bareSHADeps = append(bareSHADeps, key) } - case f.Category == checks.RefMoved: - // TODO: surface ref-moved warnings once the `gh actions-lock - // update` path exists. Today the guidance ("run gh actions-lock - // to update") is wrong — a plain re-run trusts the lockfile and - // repins nothing; only --rescan even detects the movement. Until - // there's a command that actually advances a moved ref, swallow - // these rather than print misleading instructions. - case f.Category.IsInconclusive() && - strings.Contains(f.Remediation, "transitive dependency"): - // transitive reachability unknown: silently swallowed default: otherDetailWarnings = append(otherDetailWarnings, key) } @@ -406,7 +396,7 @@ func renderWarnings(out *ui.UI, report *checks.Report, willRemediate bool) { wg := warnMap[key] f := wg.finding depKey := f.DepKey() - if f.Category.IsInconclusive() && f.Severity == checks.SeverityWarning { + if (f.Category == checks.RefMoved || f.Category.IsInconclusive()) && f.Severity == checks.SeverityWarning { label := depKey if label == "" { label = f.WorkflowPath diff --git a/cmd/gh-actions-lock/format/terminal_test.go b/cmd/gh-actions-lock/format/terminal_test.go index c31082f1..0c311e16 100644 --- a/cmd/gh-actions-lock/format/terminal_test.go +++ b/cmd/gh-actions-lock/format/terminal_test.go @@ -35,7 +35,7 @@ func TestPresentResults_WarningsReachTerminal(t *testing.T) { notWanted []string }{ { - name: "ref-moved warning is swallowed until update path exists", + name: "ref-moved warning surfaces with relock guidance", findings: []checks.Finding{{ WorkflowPath: ".github/workflows/a.yml", Category: checks.RefMoved, @@ -48,11 +48,11 @@ func TestPresentResults_WarningsReachTerminal(t *testing.T) { }, ObservedSHA: "2222222222222222222222222222222222222222", Detail: "ref v1 now resolves to 222222222222, lockfile pins 111111111111", + Remediation: "re-run `gh actions-lock --relock` to advance the lock entry", }}, - notWanted: []string{ - "moved upstream", - "compare/111111111111...222222222222", - "run `gh actions-lock` to update", + wantOutput: []string{ + "ref v1 now resolves to 222222222222", + "gh actions-lock --relock", }, }, { @@ -208,7 +208,7 @@ func TestPresentResults_RemediateHints(t *testing.T) { notWanted: []string{"bare SHA", "resolving below"}, }, { - name: "ref-moved is swallowed (deferred to update path)", + name: "ref-moved shows relock hint", willRemediate: true, findings: []checks.Finding{{ WorkflowPath: ".github/workflows/a.yml", @@ -222,10 +222,11 @@ func TestPresentResults_RemediateHints(t *testing.T) { }, ObservedSHA: "2222222222222222222222222222222222222222", Detail: "ref v1 now resolves to 222222222222", + Remediation: "re-run `gh actions-lock --relock` to advance the lock entry", }}, - notWanted: []string{ - "moved upstream", - "run `gh actions-lock` to update", + wantOutput: []string{ + "ref v1 now resolves to 222222222222", + "gh actions-lock --relock", }, }, { diff --git a/cmd/gh-actions-lock/pin_summary.go b/cmd/gh-actions-lock/pin_summary.go index c7f9bc28..f321df9a 100644 --- a/cmd/gh-actions-lock/pin_summary.go +++ b/cmd/gh-actions-lock/pin_summary.go @@ -65,7 +65,7 @@ func reportHasNonInvestigatedUnfixableErrors(report *checks.Report) bool { // 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(ctx context.Context, console *ui.UI, record *pin.Record, report *checks.Report, r *resolve.Resolver, skippedRescan int, hasInconclusive bool, refusedLabels []string, noNarrow bool, acceptMoved bool, originalVersion string, prunedWorkflows []string) error { +func renderPinSummary(ctx context.Context, console *ui.UI, record *pin.Record, report *checks.Report, r *resolve.Resolver, refusedLabels []string, noNarrow bool, acceptMoved bool, originalVersion string, prunedWorkflows []string) error { pinned := record.Pinned() investigated := record.Investigated() narrowed := record.Narrowed() @@ -95,6 +95,8 @@ func renderPinSummary(ctx context.Context, console *ui.UI, record *pin.Record, r unresolvedEntries := record.Unresolved() if len(unresolvedEntries) > 0 { renderUnresolvedWarnings(console, unresolvedEntries) + } else { + renderResolverWarning(console, report) } total := len(report.Workflows) @@ -114,22 +116,11 @@ func renderPinSummary(ctx context.Context, console *ui.UI, record *pin.Record, r } onboardingRefused := len(refusedLabels) - allClean := len(pinned) == 0 && len(investigated) == 0 && len(unresolvedEntries) == 0 + allClean := len(pinned) == 0 && len(investigated) == 0 && len(unresolvedEntries) == 0 && !reportHasLiveWarnings(report) hasUnfixable := reportHasUnfixableErrors(report, acceptMoved) - if allClean && !hasUnfixable && onboardingRefused == 0 && !hasInconclusive { + if allClean && !hasUnfixable && onboardingRefused == 0 { console.TermBlank() console.TermSuccess("All %d %s valid", total, ui.Pluralize(total, "workflow", "workflows")) - if noNarrow && skippedRescan > 0 { - // Mutable refs (v4, main) were trusted without a live check. - // With narrowing on, the version-ref nudge above already tells - // the user to pin precisely — which also buys live - // re-verification — so we don't add a competing --rescan line. - // Under --no-narrow that nudge is suppressed, so this is the - // only place the trust gap and its escape hatch surface. - console.TermDetail("%d mutable %s trusted without a live check — branch or partial-version pins (e.g. v4, main) that can move; run `gh actions-lock --rescan` to re-verify %s.", - skippedRescan, ui.Pluralize(skippedRescan, "ref", "refs"), - ui.Pluralize(skippedRescan, "it", "them")) - } return nil } @@ -164,6 +155,29 @@ func renderPinSummary(ctx context.Context, console *ui.UI, record *pin.Record, r return nil } +func reportHasLiveWarnings(report *checks.Report) bool { + for _, wr := range report.Workflows { + for _, f := range wr.Findings { + if f.Category == checks.RefMoved || f.Category.IsInconclusive() { + return true + } + } + } + return false +} + +func renderResolverWarning(console *ui.UI, report *checks.Report) { + for _, wr := range report.Workflows { + for _, f := range wr.Findings { + if f.Category == checks.ReachabilityUnknown && f.DepKey() == "" { + console.TermWarn("Dependency verification was inconclusive") + console.TermDetail("%s", f.Detail) + return + } + } + } +} + // renderCooldownFindings surfaces the fresh-tag nudge on the terminal in fix // mode, so it shows even on a clean pin where PresentResults renders nothing. // Cooldown-ignored notices are surfaced earlier by PresentResults (both modes). diff --git a/cmd/gh-actions-lock/pin_summary_test.go b/cmd/gh-actions-lock/pin_summary_test.go index 836383df..142c7e53 100644 --- a/cmd/gh-actions-lock/pin_summary_test.go +++ b/cmd/gh-actions-lock/pin_summary_test.go @@ -69,6 +69,28 @@ func TestReportHasUnfixableErrors_ClassifiesWorkflowNotPinned(t *testing.T) { } } +func TestReportHasLiveWarnings(t *testing.T) { + tests := []struct { + name string + category checks.Category + want bool + }{ + {name: "ref moved", category: checks.RefMoved, want: true}, + {name: "inconclusive", category: checks.ReachabilityUnknown, want: true}, + {name: "valid", category: checks.Valid}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + report := &checks.Report{Workflows: []checks.WorkflowReport{{ + Findings: []checks.Finding{{Category: tt.category}}, + }}} + if got := reportHasLiveWarnings(report); got != tt.want { + t.Errorf("reportHasLiveWarnings() = %v, want %v", got, tt.want) + } + }) + } +} + func TestRenderInvestigationAlerts_DeduplicatesByNWORef(t *testing.T) { entries := []pin.Entry{ { diff --git a/cmd/gh-actions-lock/prune_workflow_test.go b/cmd/gh-actions-lock/prune_workflow_test.go index dbf3dbb8..27616187 100644 --- a/cmd/gh-actions-lock/prune_workflow_test.go +++ b/cmd/gh-actions-lock/prune_workflow_test.go @@ -16,13 +16,20 @@ import ( // writeStaleLockfileRepo builds a scratch repo whose lockfile records two // workflows — workflow.yml (present on disk, uses checkout@v6) and deleted.yml -// (no file on disk, uses setup-go@v6) — and chdirs into it. Both pins are -// mutable v6 refs so they're trusted from the lockfile without any network -// call. Returns the lockfile path. -func writeStaleLockfileRepo(t *testing.T) string { +// (no file on disk, uses setup-go@v6) — and chdirs into it. Returns the +// lockfile path. +func writeStaleLockfileRepo(t *testing.T, reg *httpmock.Registry) string { t.Helper() checkoutSHA := "de0fac2e4500dabe0009e67214ff5f5447ce83dd" setupGoSHA := "4a3601121dd01d1626a1e23e37211e3254c1c06c" + reg.Register( + httpmock.GraphQLForRepo("actions", "checkout"), + httpmock.JSONResponse(map[string]any{ + "data": map[string]any{ + "a0": testRepoResponse("actions/checkout", checkoutSHA, nodeActionYAML), + }, + }), + ) dir := t.TempDir() require.NoError(t, os.MkdirAll(filepath.Join(dir, ".github", "workflows"), 0o755)) @@ -58,7 +65,7 @@ func TestCheck_FullScan_PrunesDeletedWorkflow(t *testing.T) { reg := &httpmock.Registry{} defer reg.Verify(t) - lockPath := writeStaleLockfileRepo(t) + lockPath := writeStaleLockfileRepo(t, reg) // No explicit workflow-path arg → full scan → prune authority. _, _, err := runCommandWithHTTP(t, reg, "--json=valid,workflows") @@ -80,7 +87,7 @@ func TestCheck_PartialInvocation_DoesNotPrune(t *testing.T) { reg := &httpmock.Registry{} defer reg.Verify(t) - lockPath := writeStaleLockfileRepo(t) + lockPath := writeStaleLockfileRepo(t, reg) _, _, err := runCommandWithHTTP(t, reg, "--json=valid,workflows", ".github/workflows/workflow.yml") require.NoError(t, err) @@ -98,7 +105,7 @@ func TestCheck_NoFix_ReportsStaleWorkflow(t *testing.T) { reg := &httpmock.Registry{} defer reg.Verify(t) - lockPath := writeStaleLockfileRepo(t) + lockPath := writeStaleLockfileRepo(t, reg) lockBefore, err := os.ReadFile(lockPath) require.NoError(t, err) diff --git a/cmd/gh-actions-lock/run.go b/cmd/gh-actions-lock/run.go index 0b7a3d32..e81bd8f5 100644 --- a/cmd/gh-actions-lock/run.go +++ b/cmd/gh-actions-lock/run.go @@ -31,10 +31,6 @@ type checkOptions struct { workflowPaths []string jsonFields string hostname string - // rescan forces a full reachability re-verification of every recorded - // pin, bypassing the fast path that trusts the lockfile. Useful for - // audits or when a CI policy requires re-attestation on every run. - rescan bool // profileDir, when non-empty, enables profiling: execution trace, // CPU profile, and HTTP request log are written to files in this dir. profileDir string @@ -61,8 +57,7 @@ type checkOptions struct { // Unlike acceptMoved it does NOT accept unreachable-pin findings // (possible tampering), which stay hard errors. relock bool - // verify is a convenience alias for --rescan --no-fix: full - // re-verification of every pin with a non-zero exit on any finding. + // verify runs a read-only network verification. verify bool // verifyLocal performs a zero-network static check: every action ref // in scanned workflows must have a corresponding lockfile entry. @@ -79,7 +74,6 @@ func bindCheckFlags(cmd *cobra.Command, opts *checkOptions) { cmd.Flags().StringVar(&opts.jsonFields, "json", "", "Output JSON with the specified `fields` (valid,findings,workflows,dependencies)") cmd.Flags().Lookup("json").NoOptDefVal = "valid,findings,workflows" 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 semver (e.g. v4.2.1).\n"+ @@ -90,7 +84,7 @@ func bindCheckFlags(cmd *cobra.Command, opts *checkOptions) { cmd.Flags().BoolVarP(&opts.allowAllRunners, "allow-all-runners", "A", false, "Deprecated no-op: runner restrictions have been removed") cmd.Flags().BoolVar(&opts.acceptMoved, "accept-moved", false, "Re-resolve deps flagged as ref-moved or unreachable-pin to their current live SHA") cmd.Flags().BoolVar(&opts.relock, "relock", false, "Bump moved branch/version refs (e.g. main, v4) to their current upstream SHA; leaves unreachable pins as errors") - cmd.Flags().BoolVar(&opts.verify, "verify", false, "Full re-verification of every pin (equivalent to --rescan --no-fix)") + cmd.Flags().BoolVar(&opts.verify, "verify", false, "Read-only network verification of every pin") cmd.Flags().BoolVar(&opts.verifyLocal, "verify-local", false, "Offline lockfile coverage check: verify every action ref has a lockfile entry.\n"+ "No network calls, no authentication required — ideal for pre-commit hooks.") @@ -109,8 +103,8 @@ func (opts *checkOptions) validateOutputFlags() error { if opts.verify && opts.verifyLocal { return fmt.Errorf("--verify and --verify-local are mutually exclusive") } - if opts.verifyLocal && (opts.rescan || opts.acceptMoved || opts.relock) { - return fmt.Errorf("--verify-local cannot be combined with --rescan, --accept-moved, or --relock because it runs offline") + if opts.verifyLocal && (opts.acceptMoved || opts.relock) { + return fmt.Errorf("--verify-local cannot be combined with --accept-moved or --relock because it runs offline") } return nil } @@ -176,18 +170,6 @@ func runCheck(cmd *cobra.Command, opts *checkOptions, newResolver resolverFunc) if err != nil { return err } - // Pre-warm resolver caches from the lockfile so repeat runs skip - // redundant GraphQL and REST calls. Skipped when --rescan is set: - // a full re-verification must hit the network to detect ref movement. - // --accept-moved and --relock both imply --rescan (must detect what - // moved before re-pinning). - if opts.acceptMoved || opts.relock { - opts.rescan = true - } - trustLockfileCaches := !opts.rescan - if trustLockfileCaches { - r.SeedFromLockfile(store.AllDeps()) - } endSetup() opts.workflowPaths = paths @@ -257,12 +239,9 @@ func runCheck(cmd *cobra.Command, opts *checkOptions, newResolver resolverFunc) Resolver: r, Store: store, Pool: pool, - Rescan: opts.rescan, Profile: prof, } - // Defer spinner start until actual network work begins. The fast path - // (everything trusted from the lockfile) returns before resolve fires, - // so the spinner never appears and there's no flicker. + // Defer spinner start until actual network work begins. if showSpinner { var once sync.Once runOpts.OnResolveProgress = func(done, total int) { @@ -286,7 +265,6 @@ func runCheck(cmd *cobra.Command, opts *checkOptions, newResolver resolverFunc) report := result.Report valid := result.Valid - skippedRescan := result.SkippedRescan // Read-only modes never touch the lockfile, so surface stale entries as // non-blocking info findings instead of pruning them. Fix mode prunes @@ -451,20 +429,12 @@ func runCheck(cmd *cobra.Command, opts *checkOptions, newResolver resolverFunc) } // Terminal summary. - hasInconclusive := opts.rescan && report.HasInconclusive() - summaryErr := renderPinSummary(ctx, console, record, report, r, skippedRescan, hasInconclusive, refusedLabels, opts.noNarrow, opts.acceptMoved, store.OriginalVersion(), staleWorkflows) + summaryErr := renderPinSummary(ctx, console, record, report, r, refusedLabels, opts.noNarrow, opts.acceptMoved, store.OriginalVersion(), staleWorkflows) if summaryErr != nil { return summaryErr } - // --rescan strict gate: inconclusive reachability is a hard failure when - // the user explicitly requested a full re-verification. Without this, - // inconclusive findings (e.g. SAML-blocked branch listing) silently pass - // and the "✓ All N workflows valid" message is misleading. - if hasInconclusive { - return errSilent - } return nil } diff --git a/cmd/gh-actions-lock/verify.go b/cmd/gh-actions-lock/verify.go index 68bfc100..cf0bc37f 100644 --- a/cmd/gh-actions-lock/verify.go +++ b/cmd/gh-actions-lock/verify.go @@ -2,8 +2,8 @@ package main // Verification modes for gh actions-lock: // -// --verify Full re-verification of every pin against the network. -// Equivalent to --rescan --no-fix. Requires auth. +// --verify Read-only verification of every pin against the network. +// Requires auth. // // --verify-local Offline static coverage check. Every action ref must have // a lockfile entry. No network, no auth — ideal for pre-commit. @@ -20,11 +20,10 @@ import ( "github.com/github/gh-actions-lock/internal/ui" ) -// applyVerifyFlags expands --verify into its constituent flags. Called at the -// top of runCheck before any work begins. +// applyVerifyFlags makes --verify read-only. Called at the top of runCheck +// before any work begins. func applyVerifyFlags(opts *checkOptions) { if opts.verify { - opts.rescan = true opts.noFix = true } } diff --git a/cmd/gh-actions-lock/verify_test.go b/cmd/gh-actions-lock/verify_test.go index 9ab2326f..22194c1c 100644 --- a/cmd/gh-actions-lock/verify_test.go +++ b/cmd/gh-actions-lock/verify_test.go @@ -8,34 +8,24 @@ import ( func TestApplyVerifyFlags(t *testing.T) { tests := []struct { - name string - opts checkOptions - wantRescan bool - wantNoFix bool + name string + opts checkOptions + wantNoFix bool }{ { - name: "verify sets rescan and noFix", - opts: checkOptions{verify: true}, - wantRescan: true, - wantNoFix: true, + name: "verify sets noFix", + opts: checkOptions{verify: true}, + wantNoFix: true, }, { - name: "no verify leaves flags alone", - opts: checkOptions{}, - wantRescan: false, - wantNoFix: false, - }, - { - name: "verify with existing rescan keeps both", - opts: checkOptions{verify: true, rescan: true}, - wantRescan: true, - wantNoFix: true, + name: "no verify leaves flags alone", + opts: checkOptions{}, + wantNoFix: false, }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { applyVerifyFlags(&tt.opts) - assert.Equal(t, tt.wantRescan, tt.opts.rescan) assert.Equal(t, tt.wantNoFix, tt.opts.noFix) }) } @@ -52,11 +42,6 @@ func TestValidateOutputFlags_VerifyConflicts(t *testing.T) { opts: checkOptions{verify: true, verifyLocal: true}, wantErr: "mutually exclusive", }, - { - name: "verify-local and rescan conflict", - opts: checkOptions{verifyLocal: true, rescan: true}, - wantErr: "offline", - }, { name: "verify-local and accept-moved conflict", opts: checkOptions{verifyLocal: true, acceptMoved: true}, diff --git a/internal/pipeline/checks/finding.go b/internal/pipeline/checks/finding.go index 48c7f6af..86c9dba2 100644 --- a/internal/pipeline/checks/finding.go +++ b/internal/pipeline/checks/finding.go @@ -181,21 +181,6 @@ func (r *Report) IsValid() bool { return true } -// HasInconclusive reports whether the report contains any inconclusive -// findings (reachability-unknown, ancestry-unknown). These are treated as -// warnings by default, but callers that need a strict gate (e.g. --rescan) -// can use this to fail when verification couldn't complete. -func (r *Report) HasInconclusive() bool { - for _, wr := range r.Workflows { - for _, f := range wr.Findings { - if f.Category.IsInconclusive() { - return true - } - } - } - return false -} - // IsValid returns true if no findings represent integrity violations. func (wr *WorkflowReport) IsValid() bool { for _, f := range wr.Findings { diff --git a/internal/pipeline/checks/misleading.go b/internal/pipeline/checks/misleading.go index 19db956e..1ee64e38 100644 --- a/internal/pipeline/checks/misleading.go +++ b/internal/pipeline/checks/misleading.go @@ -132,7 +132,7 @@ func checkOneRefMoved(ctx context.Context, pw ParsedWorkflow, ref parserlock.Act f.Severity = SeverityWarning f.Confidence = ConfidenceHigh f.Detail = fmt.Sprintf("ref %s now resolves to %s, lockfile pins %s", ref.Ref, parserlock.ShortSHA(sha), parserlock.ShortSHA(pin.SHA())) - f.Remediation = "re-run `gh actions-lock` to refresh the lock entry" + f.Remediation = "re-run `gh actions-lock --relock` to advance the lock entry" } return f, true } diff --git a/internal/pipeline/checks/parsed.go b/internal/pipeline/checks/parsed.go index de73caa5..bbd8a921 100644 --- a/internal/pipeline/checks/parsed.go +++ b/internal/pipeline/checks/parsed.go @@ -1,8 +1,6 @@ package checks import ( - "strings" - parserlock "github.com/github/actions-lockfile/go/pkg/lockfile" "github.com/github/gh-actions-lock/internal/dep" ) @@ -30,74 +28,7 @@ type ParsedWorkflow struct { ParseWarnings []string LoadErr error DepsErr error - // Resolved, when true, instructs DiagnoseParsed to run this - // workflow's diagnostics with a nil resolver. Network-bound checks - // (ref-moved) are skipped and the engine relies on - // purely structural validation against the on-disk lockfile. Caller - // is asserting "this workflow is already fully resolved" — typically - // set on the fast path when every direct ref in the workflow is - // already recorded in the lockfile. + // Resolved instructs DiagnoseParsed to skip network-bound checks for a + // workflow with structural blockers. Resolved bool } - -// PartitionRefs splits refs into recorded (matching a lockfile entry by -// NWO@Ref or NWO@SHA) and unrecorded (need network resolution). When an -// error prevented loading refs or deps, everything is unrecorded. -func (pw ParsedWorkflow) PartitionRefs() (recorded, unrecorded []parserlock.ActionRef) { - if pw.LoadErr != nil || pw.DepsErr != nil { - return nil, pw.Refs - } - if len(pw.Refs) == 0 { - return nil, nil - } - haveDep := make(map[string]bool, len(pw.ExistingDeps)*2) - for _, d := range pw.ExistingDeps { - nwo := strings.ToLower(d.NWO) - haveDep[nwo+"@"+d.Ref] = true - if d.SHA != "" { - haveDep[nwo+"@"+strings.ToLower(d.SHA)] = true - } - } - for _, r := range pw.Refs { - if haveDep[strings.ToLower(r.Owner+"/"+r.Repo)+"@"+r.Ref] { - recorded = append(recorded, r) - } else { - unrecorded = append(unrecorded, r) - } - } - return recorded, unrecorded -} - -// IsFullyRecorded returns true when every direct ref has a matching -// lockfile entry — the steady-state happy path. -func (pw ParsedWorkflow) IsFullyRecorded() bool { - _, unrecorded := pw.PartitionRefs() - return len(pw.Refs) == 0 || len(unrecorded) == 0 -} - -// IsImmutableRef reports whether ref is a full semver tag (e.g. v4.2.1), -// which resolves to exactly one commit for its entire lifetime. Full semver -// pins are re-verified against upstream on the default path; mutable refs -// (v4, v4.2, branches) are trusted until --rescan because they legitimately -// move. -func IsImmutableRef(ref string) bool { - sv, ok := parserlock.ParseSemVer(ref) - return ok && sv.IsFull() -} - -// RecordedDeps returns the subset of ExistingDeps whose NWO@Ref or -// NWO@SHA matches one of the given recorded refs. -func (pw ParsedWorkflow) RecordedDeps(recorded []parserlock.ActionRef) []dep.Dependency { - refKeys := make(map[string]bool, len(recorded)) - for _, r := range recorded { - refKeys[strings.ToLower(r.Owner+"/"+r.Repo)+"@"+r.Ref] = true - } - var out []dep.Dependency - for _, d := range pw.ExistingDeps { - nwo := strings.ToLower(d.NWO) - if refKeys[nwo+"@"+d.Ref] || refKeys[nwo+"@"+strings.ToLower(d.SHA)] { - out = append(out, d) - } - } - return out -} diff --git a/internal/pipeline/checks/parsed_test.go b/internal/pipeline/checks/parsed_test.go deleted file mode 100644 index 8706637d..00000000 --- a/internal/pipeline/checks/parsed_test.go +++ /dev/null @@ -1,29 +0,0 @@ -package checks - -import ( - "testing" - - "github.com/stretchr/testify/assert" -) - -func TestIsImmutableRef(t *testing.T) { - tests := []struct { - ref string - want bool - }{ - {"v4.2.1", true}, // full semver: one commit for life - {"4.2.1", true}, // full semver without leading v - {"v4", false}, // major-only: moves as patches land - {"v4.2", false}, // major.minor: moves - {"v4.2.1-rc1", false}, // prerelease: not a full stable tag - {"main", false}, // branch - {"master", false}, // branch - {"de0fac2e4500dabe0009e67214ff5f5447ce83dd", false}, // bare SHA - {"", false}, - } - for _, tt := range tests { - t.Run(tt.ref, func(t *testing.T) { - assert.Equal(t, tt.want, IsImmutableRef(tt.ref)) - }) - } -} diff --git a/internal/pipeline/parse.go b/internal/pipeline/parse.go index ac9e7b1a..42ad040b 100644 --- a/internal/pipeline/parse.go +++ b/internal/pipeline/parse.go @@ -2,7 +2,6 @@ package pipeline import ( "context" - "strings" parserlock "github.com/github/actions-lockfile/go/pkg/lockfile" "github.com/github/gh-actions-lock/internal/dep" @@ -106,27 +105,10 @@ func mergeStrings(groups ...[]string) []string { // across all parsed workflows. Use the returned slices to pre-warm the // resolver caches once before per-workflow diagnostics. func CollectResolvable(parsed []checks.ParsedWorkflow) ([]parserlock.ActionRef, []dep.Dependency) { - return collectResolvable(parsed, nil) -} - -// CollectUnrecordedResolvable is like CollectResolvable but excludes refs -// whose NWO@Ref key appears in recordedKeys. Deps whose key is in -// recordedKeys are also excluded. Use this when per-dep lockfile trust -// has already seeded the resolver cache for recorded deps, so only -// genuinely new refs need network resolution. -func CollectUnrecordedResolvable(parsed []checks.ParsedWorkflow, recordedKeys map[string]bool) ([]parserlock.ActionRef, []dep.Dependency) { - return collectResolvable(parsed, recordedKeys) -} - -func collectResolvable(parsed []checks.ParsedWorkflow, excludeKeys map[string]bool) ([]parserlock.ActionRef, []dep.Dependency) { seenRef := make(map[ghapi.ActionRef]bool) var refs []parserlock.ActionRef for _, pw := range parsed { for _, ref := range pw.Refs { - nwoRef := strings.ToLower(ref.Owner+"/"+ref.Repo) + "@" + ref.Ref - if excludeKeys[nwoRef] { - continue - } key := ghapi.ForActionRef(ref.Owner, ref.Repo, ref.Path, ref.Ref) if seenRef[key] { continue @@ -140,7 +122,7 @@ func collectResolvable(parsed []checks.ParsedWorkflow, excludeKeys map[string]bo for _, pw := range parsed { for _, dep := range pw.ExistingDeps { key := dep.Key() - if excludeKeys[key] || seenDep[key] { + if seenDep[key] { continue } seenDep[key] = true diff --git a/internal/pipeline/run.go b/internal/pipeline/run.go index 46aa908a..6482e795 100644 --- a/internal/pipeline/run.go +++ b/internal/pipeline/run.go @@ -2,10 +2,7 @@ package pipeline import ( "context" - "strings" - parserlock "github.com/github/actions-lockfile/go/pkg/lockfile" - "github.com/github/gh-actions-lock/internal/dep" "github.com/github/gh-actions-lock/internal/lockfile" "github.com/github/gh-actions-lock/internal/pinpool" "github.com/github/gh-actions-lock/internal/pipeline/checks" @@ -19,7 +16,6 @@ type RunOptions struct { Resolver *resolve.Resolver Store *lockfile.State Pool *pinpool.Pool - Rescan bool // re-verify all pins end-to-end // Resolver UX hooks — set these for interactive spinner mode. OnResolveProgress func(done, total int) @@ -29,13 +25,11 @@ type RunOptions struct { // RunResult bundles the pipeline output. type RunResult struct { - Report *checks.Report - Valid bool - SkippedRescan int // mutable recorded refs (v4, branches) trusted without a live re-check + Report *checks.Report + Valid bool } -// Run executes the full diagnostic pipeline: parse → trust-check → -// resolve → diagnose. +// Run executes the full diagnostic pipeline: parse → resolve → diagnose. func Run(ctx context.Context, opts RunOptions) (*RunResult, error) { r := opts.Resolver prof := opts.Profile @@ -49,62 +43,16 @@ func Run(ctx context.Context, opts RunOptions) (*RunResult, error) { return nil, ctx.Err() } - // Fast path: trust fully-recorded workflows. For partially-recorded - // workflows, seed the resolver cache with recorded deps so only - // unrecorded refs hit the network. - // - // Immutable full-semver pins (e.g. v4.2.1) are NOT trusted blindly: - // they're routed through live resolution + ancestry so a stale or - // unreachable pin is caught on the default path, not just under - // --rescan. Mutable recorded refs (v4, v4.2, branches) legitimately - // move, so they stay trusted (seeded from the lockfile) until --rescan. - skippedRescan := 0 - var seedDeps []dep.Dependency - recordedKeys := make(map[string]bool) + // Structural blockers are terminal at diagnose time. Do not perform + // unrelated network work for a workflow the planner must reject. for i := range parsed { - // Structural blockers are terminal at diagnose time. Do not perform - // unrelated network work for a workflow the planner must reject. if len(parsed[i].LocalPaths) > 0 || len(parsed[i].SelfRepositoryRefErrs) > 0 || len(parsed[i].SelfRepositoryResolutionErrs) > 0 { parsed[i].Resolved = true - continue - } - if opts.Rescan { - continue - } - plan := planFastPath(parsed[i]) - // Mutable recorded refs are trusted without a live re-check - // (surfaced in the summary so the operator can --rescan them). - skippedRescan += len(plan.mutableRefs) - if plan.resolved { - parsed[i].Resolved = true - continue - } - // Seed only the mutable recorded deps so they resolve from - // the lockfile (trusted); immutable and unrecorded refs are - // left to resolve live from the network. - rd := parsed[i].RecordedDeps(plan.mutableRefs) - seedDeps = append(seedDeps, rd...) - for _, rr := range plan.mutableRefs { - recordedKeys[strings.ToLower(rr.Owner+"/"+rr.Repo)+"@"+rr.Ref] = true } } - // Seed the resolver cache with lockfile entries for recorded deps - // in partially-recorded workflows. This makes the pipeline - // self-sufficient: diagnoseOneParsed re-resolves ALL refs per - // workflow, and seeded entries become free cache hits. - // - // Trust boundary: seeded entries have no actionYML, so the BFS in - // ResolveAllRecursive won't discover new transitive deps through - // them. This is intentional — the same trust model as - // IsFullyRecorded, which skips resolution entirely. If the - // lockfile's transitive closure is incomplete, --rescan detects it. - if r != nil && len(seedDeps) > 0 { - r.SeedFromLockfile(dep.Dedup(seedDeps)) - } - // Collect unresolved workflows for network work. var unresolved []checks.ParsedWorkflow for _, pw := range parsed { @@ -112,7 +60,7 @@ func Run(ctx context.Context, opts RunOptions) (*RunResult, error) { unresolved = append(unresolved, pw) } } - refs, _ := CollectUnrecordedResolvable(unresolved, recordedKeys) + refs, _ := CollectResolvable(unresolved) // Phase 2: Resolve. if r == nil { @@ -149,47 +97,7 @@ func Run(ctx context.Context, opts RunOptions) (*RunResult, error) { valid := report.IsValid() return &RunResult{ - Report: report, - Valid: valid, - SkippedRescan: skippedRescan, + Report: report, + Valid: valid, }, nil } - -// fastPathPlan describes how the pre-resolution fast path treats one -// recorded workflow. -type fastPathPlan struct { - // resolved is true when the workflow needs no live resolution: it has - // no refs, is a local-path action, or every recorded ref is a trusted - // mutable pin. - resolved bool - // mutableRefs are recorded refs (v4, v4.2, branches) trusted from the - // lockfile without a live re-check. - mutableRefs []parserlock.ActionRef -} - -// planFastPath decides, without touching the network, whether a parsed -// workflow can skip live resolution and which of its recorded refs are -// trusted mutable pins. Immutable full-semver pins (v4.2.1) are never -// trusted blindly: their presence forces live resolution so a stale or -// unreachable pin is caught on the default path, not just under --rescan. -func planFastPath(pw checks.ParsedWorkflow) fastPathPlan { - // Local-path workflows are handled at diagnose time; don't waste - // network calls resolving their refs. - if len(pw.LocalPaths) > 0 { - return fastPathPlan{resolved: true} - } - recorded, unrecorded := pw.PartitionRefs() - - var mutable []parserlock.ActionRef - immutableCount := 0 - for _, rr := range recorded { - if checks.IsImmutableRef(rr.Ref) { - immutableCount++ - } else { - mutable = append(mutable, rr) - } - } - - resolved := len(pw.Refs) == 0 || (len(unrecorded) == 0 && immutableCount == 0) - return fastPathPlan{resolved: resolved, mutableRefs: mutable} -} diff --git a/internal/pipeline/run_test.go b/internal/pipeline/run_test.go deleted file mode 100644 index 380feff9..00000000 --- a/internal/pipeline/run_test.go +++ /dev/null @@ -1,318 +0,0 @@ -package pipeline - -import ( - "testing" - - parserlock "github.com/github/actions-lockfile/go/pkg/lockfile" - "github.com/github/gh-actions-lock/internal/dep" - "github.com/github/gh-actions-lock/internal/pipeline/checks" - "github.com/stretchr/testify/assert" -) - -func ref(owner, repo, path, ref string) parserlock.ActionRef { - return parserlock.ActionRef{Owner: owner, Repo: repo, Path: path, Ref: ref} -} - -func mkDep(nwo, ref, sha string) dep.Dependency { - return dep.Dependency{NWO: nwo, Ref: ref, SHA: sha} -} - -func TestPlanFastPath(t *testing.T) { - tests := []struct { - name string - pw checks.ParsedWorkflow - wantResolved bool - wantMutable int - }{ - { - name: "all mutable recorded → trusted, no live resolution", - pw: checks.ParsedWorkflow{ - Refs: []parserlock.ActionRef{ref("actions", "checkout", "", "v4")}, - ExistingDeps: []dep.Dependency{mkDep("actions/checkout", "v4", "aaa")}, - }, - wantResolved: true, - wantMutable: 1, - }, - { - name: "immutable recorded pin → forces live resolution (the #819 fix)", - pw: checks.ParsedWorkflow{ - Refs: []parserlock.ActionRef{ref("actions", "checkout", "", "v4.2.1")}, - ExistingDeps: []dep.Dependency{mkDep("actions/checkout", "v4.2.1", "aaa")}, - }, - wantResolved: false, - wantMutable: 0, - }, - { - name: "mixed immutable + mutable → resolve live, trust the mutable one", - pw: checks.ParsedWorkflow{ - Refs: []parserlock.ActionRef{ - ref("actions", "checkout", "", "v4.2.1"), - ref("actions", "setup-go", "", "v5"), - }, - ExistingDeps: []dep.Dependency{ - mkDep("actions/checkout", "v4.2.1", "aaa"), - mkDep("actions/setup-go", "v5", "bbb"), - }, - }, - wantResolved: false, - wantMutable: 1, - }, - { - name: "unrecorded ref → resolve live, nothing trusted", - pw: checks.ParsedWorkflow{ - Refs: []parserlock.ActionRef{ref("actions", "checkout", "", "v4")}, - ExistingDeps: nil, - }, - wantResolved: false, - wantMutable: 0, - }, - { - name: "no refs → resolved, nothing to do", - pw: checks.ParsedWorkflow{}, - wantResolved: true, - wantMutable: 0, - }, - { - name: "local-path workflow → resolved, refs untouched", - pw: checks.ParsedWorkflow{ - LocalPaths: []string{"./my-local-action"}, - Refs: []parserlock.ActionRef{ref("actions", "checkout", "", "v4.2.1")}, - }, - wantResolved: true, - wantMutable: 0, - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - plan := planFastPath(tt.pw) - assert.Equal(t, tt.wantResolved, plan.resolved, "resolved") - assert.Len(t, plan.mutableRefs, tt.wantMutable, "mutableRefs") - }) - } -} - -func TestPartitionRefs(t *testing.T) { - tests := []struct { - name string - pw checks.ParsedWorkflow - wantRecordedLen int - wantUnrecordLen int - wantRecordedNWOs []string - }{ - { - name: "all recorded", - pw: checks.ParsedWorkflow{ - Refs: []parserlock.ActionRef{ - ref("actions", "checkout", "", "v4"), - ref("actions", "setup-go", "", "v5"), - }, - ExistingDeps: []dep.Dependency{ - mkDep("actions/checkout", "v4", "aaa"), - mkDep("actions/setup-go", "v5", "bbb"), - }, - }, - wantRecordedLen: 2, - wantUnrecordLen: 0, - }, - { - name: "none recorded", - pw: checks.ParsedWorkflow{ - Refs: []parserlock.ActionRef{ - ref("actions", "checkout", "", "v4"), - }, - ExistingDeps: nil, - }, - wantRecordedLen: 0, - wantUnrecordLen: 1, - }, - { - name: "mixed: one recorded one not", - pw: checks.ParsedWorkflow{ - Refs: []parserlock.ActionRef{ - ref("actions", "checkout", "", "v4"), - ref("mmastrac", "mmm-matrix", "", "v1"), - }, - ExistingDeps: []dep.Dependency{ - mkDep("actions/checkout", "v4", "aaa"), - }, - }, - wantRecordedLen: 1, - wantUnrecordLen: 1, - wantRecordedNWOs: []string{"actions/checkout"}, - }, - { - name: "load error makes everything unrecorded", - pw: checks.ParsedWorkflow{ - Refs: []parserlock.ActionRef{ - ref("actions", "checkout", "", "v4"), - }, - ExistingDeps: []dep.Dependency{ - mkDep("actions/checkout", "v4", "aaa"), - }, - LoadErr: assert.AnError, - }, - wantRecordedLen: 0, - wantUnrecordLen: 1, - }, - { - name: "empty refs", - pw: checks.ParsedWorkflow{}, - wantRecordedLen: 0, - wantUnrecordLen: 0, - }, - { - name: "sub-action path collapses to NWO level", - pw: checks.ParsedWorkflow{ - Refs: []parserlock.ActionRef{ - ref("actions", "cache", "save", "v4"), - ref("actions", "cache", "restore", "v4"), - }, - ExistingDeps: []dep.Dependency{ - mkDep("actions/cache", "v4", "ccc"), - }, - }, - wantRecordedLen: 2, // both sub-actions match the dep - wantUnrecordLen: 0, - }, - { - name: "bare SHA ref matches by SHA", - pw: checks.ParsedWorkflow{ - Refs: []parserlock.ActionRef{ - ref("actions", "checkout", "", "de0fac2e4500dabe0009e67214ff5f5447ce83dd"), - }, - ExistingDeps: []dep.Dependency{ - mkDep("actions/checkout", "v6.0.2", "de0fac2e4500dabe0009e67214ff5f5447ce83dd"), - }, - }, - wantRecordedLen: 1, - wantUnrecordLen: 0, - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - recorded, unrecorded := tt.pw.PartitionRefs() - assert.Len(t, recorded, tt.wantRecordedLen) - assert.Len(t, unrecorded, tt.wantUnrecordLen) - - if len(tt.wantRecordedNWOs) > 0 { - var gotNWOs []string - for _, r := range recorded { - gotNWOs = append(gotNWOs, r.Owner+"/"+r.Repo) - } - assert.Equal(t, tt.wantRecordedNWOs, gotNWOs) - } - }) - } -} - -func TestIsFullyRecorded(t *testing.T) { - t.Run("fully recorded", func(t *testing.T) { - pw := checks.ParsedWorkflow{ - Refs: []parserlock.ActionRef{ - ref("actions", "checkout", "", "v4"), - }, - ExistingDeps: []dep.Dependency{ - mkDep("actions/checkout", "v4", "aaa"), - }, - } - assert.True(t, pw.IsFullyRecorded()) - }) - - t.Run("partially recorded", func(t *testing.T) { - pw := checks.ParsedWorkflow{ - Refs: []parserlock.ActionRef{ - ref("actions", "checkout", "", "v4"), - ref("actions", "setup-go", "", "v5"), - }, - ExistingDeps: []dep.Dependency{ - mkDep("actions/checkout", "v4", "aaa"), - }, - } - assert.False(t, pw.IsFullyRecorded()) - }) - - t.Run("no refs", func(t *testing.T) { - pw := checks.ParsedWorkflow{} - assert.True(t, pw.IsFullyRecorded()) - }) -} - -func TestRecordedDeps(t *testing.T) { - pw := checks.ParsedWorkflow{ - ExistingDeps: []dep.Dependency{ - mkDep("actions/checkout", "v4", "aaa"), - mkDep("actions/setup-go", "v5", "bbb"), - mkDep("third-party/tool", "v1", "ccc"), - }, - } - recorded := []parserlock.ActionRef{ - ref("actions", "checkout", "", "v4"), - ref("third-party", "tool", "", "v1"), - } - - got := pw.RecordedDeps(recorded) - assert.Len(t, got, 2) - - keys := make(map[string]bool) - for _, d := range got { - keys[d.Key()] = true - } - assert.True(t, keys["actions/checkout@v4"]) - assert.True(t, keys["third-party/tool@v1"]) - assert.False(t, keys["actions/setup-go@v5"]) -} - -func TestCollectUnrecordedResolvable(t *testing.T) { - parsed := []checks.ParsedWorkflow{ - { - Refs: []parserlock.ActionRef{ - ref("actions", "checkout", "", "v4"), - ref("actions", "setup-go", "", "v5"), - ref("mmastrac", "mmm-matrix", "", "v1"), - }, - ExistingDeps: []dep.Dependency{ - mkDep("actions/checkout", "v4", "aaa"), - mkDep("actions/setup-go", "v5", "bbb"), - mkDep("mmastrac/mmm-matrix", "v1", "ccc"), - }, - }, - } - - recordedKeys := map[string]bool{ - "actions/checkout@v4": true, - "actions/setup-go@v5": true, - } - - refs, deps := CollectUnrecordedResolvable(parsed, recordedKeys) - - // Only the unrecorded ref should be collected. - assert.Len(t, refs, 1) - assert.Equal(t, "mmastrac", refs[0].Owner) - assert.Equal(t, "mmm-matrix", refs[0].Repo) - - // Only the unrecorded dep should be collected. - assert.Len(t, deps, 1) - assert.Equal(t, "mmastrac/mmm-matrix", deps[0].NWO) -} - -func TestCollectUnrecordedResolvable_NilExclude(t *testing.T) { - parsed := []checks.ParsedWorkflow{ - { - Refs: []parserlock.ActionRef{ - ref("actions", "checkout", "", "v4"), - ref("actions", "setup-go", "", "v5"), - }, - ExistingDeps: []dep.Dependency{ - mkDep("actions/checkout", "v4", "aaa"), - }, - }, - } - - // With nil recordedKeys, CollectResolvable and CollectUnrecordedResolvable - // should return the same results. - refsAll, depsAll := CollectResolvable(parsed) - refsFiltered, depsFiltered := CollectUnrecordedResolvable(parsed, nil) - - assert.Equal(t, refsAll, refsFiltered) - assert.Equal(t, depsAll, depsFiltered) -} diff --git a/internal/resolve/resolver.go b/internal/resolve/resolver.go index 24776e06..dcf9772a 100644 --- a/internal/resolve/resolver.go +++ b/internal/resolve/resolver.go @@ -128,25 +128,6 @@ func (r *Resolver) SeedBranchHints(deps []dep.Dependency) { } } -// SeedFromLockfile pre-warms the resolution cache so repeat runs skip -// redundant API calls. Do NOT call with --rescan: seeding would hide -// ref movement. -func (r *Resolver) SeedFromLockfile(deps []dep.Dependency) { - for _, d := range deps { - if d.SHA == "" || d.Ref == "" { - continue - } - owner, repo := d.OwnerRepo() - if owner == "" || repo == "" { - continue - } - r.cache.Put( - ghapi.ForActionRef(owner, repo, d.Path, d.Ref), - resolvedEntry{dep: d}, - ) - } -} - // --- Accessors --- // Hostname returns the GitHub host the resolver is targeting. diff --git a/test/integration/harness.rb b/test/integration/harness.rb index 4a334a1a..bb70c356 100644 --- a/test/integration/harness.rb +++ b/test/integration/harness.rb @@ -1193,17 +1193,14 @@ def shell system(sub_env, ENV.fetch("SHELL", "/bin/bash"), chdir: ctx.dir) puts "\nBack in integration shell. Scenario dir still live at #{ctx.dir}" - when "rerun", "rescan" + when "rerun" if active_ctx w = 62 - rescan = (verb == "rescan" || arg == "--rescan") - mode_label = rescan ? "re-scanning" : "re-running" - puts "\e[1;36m── #{mode_label} #{active_ctx.scenario.name} ──\e[0m" - extra = rescan ? ["--rescan"] : [] - puts "\e[2m$\e[0m #{active_ctx.cmd_string(extra_args: extra)}" + puts "\e[1;36m── re-running #{active_ctx.scenario.name} ──\e[0m" + puts "\e[2m$\e[0m #{active_ctx.cmd_string}" puts t0 = Process.clock_gettime(Process::CLOCK_MONOTONIC) - result = active_ctx.run_pty(input_prompts: active_ctx.scenario.input_spec, extra_args: extra) + result = active_ctx.run_pty(input_prompts: active_ctx.scenario.input_spec) elapsed = Process.clock_gettime(Process::CLOCK_MONOTONIC) - t0 puts @@ -1560,8 +1557,6 @@ def print_help 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 (keeps lockfile state)" - puts " \e[36mrerun --rescan\e[0m Re-run with --rescan flag" - puts " \e[36mrescan\e[0m Shorthand for rerun --rescan" puts " \e[36medit\e[0m Open active scenario dir in $EDITOR" puts " \e[36mdone\e[0m Teardown active scenario context" puts " \e[36mbuild\e[0m Rebuild the binary (go build)" diff --git a/test/integration/run.rb b/test/integration/run.rb index 00391db9..17a3dc93 100644 --- a/test/integration/run.rb +++ b/test/integration/run.rb @@ -709,11 +709,6 @@ def wire_checkout_fresh(s, token) end s.env("GH_TOKEN" => "gho_fake_mixed_test_token") }, - rescan_inconclusive_fails: ->(s) { - s.stub_server { |srv| sso_403_all(srv) } - s.env("GH_TOKEN" => "gho_fake_rescan_token") - }, - # Error icon / record / prefix scenarios: SSO 403 as a reliable failure trigger unresolved_uses_error_icon: ->(s) { s.stub_server { |srv| sso_403_all(srv) } diff --git a/test/scenarios/catalog.yml b/test/scenarios/catalog.yml index 396407df..4f1d5e15 100644 --- a/test/scenarios/catalog.yml +++ b/test/scenarios/catalog.yml @@ -34,7 +34,7 @@ categories: - name: workflow_parsing description: "Workflow file parsing edge cases" - name: output_modes - description: "CLI flag combinations (--json, --no-fix, --rescan)" + description: "CLI flag combinations (--json, --no-fix, --verify)" - name: multi_workflow description: "Cross-workflow scenarios and dependency tracking" - name: security @@ -84,10 +84,7 @@ scenarios: lockfile_contains: ["'actions/checkout@v4':"] lockfile_comment_excludes: 'actions/checkout@v4\.\d' # v4 is a partial-semver pin: with narrowing on (default), the - # version-ref nudge is the single actionable message (pin precisely, - # which also earns live re-verification). The summary must NOT add a - # competing --rescan line, and must not resurrect the old "re-verify - # reachability" contradiction. + # version-ref nudge is the actionable message to pin precisely. output_contains: ["pinned without a full semver tag", "lock precisely"] output_excludes: ["trusted without a live check", "re-verify reachability"] @@ -647,36 +644,6 @@ scenarios: stdout_is_json: true stdout_contains: ["findings"] - - name: rescan_mode - category: output_modes - description: "--rescan forces full re-verification" - needs_token: true - flags: ["--rescan"] - fixtures: - workflows: - ci.yml: - name: CI - actions: ["actions/checkout@v4"] - expect: - exit: 0 - lockfile_deps_cover_direct: true - - - name: rescan_inconclusive_fails - category: output_modes - description: "--rescan with SSO-blocked verification — fails, no 'All valid'" - needs_stub: true - tags: [stub] - flags: ["--rescan"] - fixtures: - workflows: - ci.yml: - name: CI - actions: ["actions/checkout@v4"] - lockfile_template: pinned_checkout - expect: - exit: 1 - output_excludes: ["All", "valid"] - - name: no_fix_json_combined category: output_modes description: "--no-fix --json combined produces JSON without modifying disk" @@ -694,7 +661,7 @@ scenarios: - name: verify_fresh_unpinned category: output_modes - description: "--verify with unpinned deps — exits 1, read-only (equivalent to --rescan --no-fix)" + description: "--verify with unpinned deps — exits 1 and leaves files unchanged" needs_token: true flags: ["--verify"] fixtures: @@ -819,20 +786,6 @@ scenarios: exit: 1 output_contains: ["mutually exclusive"] - - name: verify_local_conflict_rescan - category: output_modes - description: "--verify-local --rescan is rejected — verify-local is offline" - needs_stub: false - flags: ["--verify-local", "--rescan"] - fixtures: - workflows: - ci.yml: - name: CI - actions: ["actions/checkout@v4"] - expect: - exit: 1 - output_contains: ["offline"] - # ╔═════════════════════════════════════════════════════════════════════════╗ # ║════════════════════════════ multi_workflow ═════════════════════════════║ # ╚═════════════════════════════════════════════════════════════════════════╝ @@ -1956,7 +1909,7 @@ scenarios: valid: true - name: dbot_transient_403_drops_pin category: dependabot - description: "SSO 403 on a previously-pinned action — pin retained, clean exit" + description: "SSO 403 on a previously-pinned action — pin retained with an inconclusive warning and clean exit" needs_stub: true tags: [stub] flags: ["--no-onboard", "--no-narrow", "--no-interactive", "--json=valid,findings"] @@ -1974,7 +1927,11 @@ scenarios: - expr: '.valid' equals: "true" - expr: '.findings | length' - equals: "0" + equals: "1" + - expr: '.findings[0].category' + equals: "reachability-unknown" + - expr: '.findings[0].severity' + equals: "warning" lockfile_contains: - "version: 'v0.0.2'" @@ -1982,11 +1939,6 @@ scenarios: - "ref: 'v4'" - "commit: 'sha1-de0fac2e4500dabe0009e67214ff5f5447ce83dd'" - golden_json: - cli_version: (devel) - findings: [] - lockfile_version: v0.0.2 - valid: true - name: dbot_impostor_blocks category: dependabot description: "Orphaned commit (no tag or branch) produces reachability-unknown warning in JSON findings" @@ -2013,7 +1965,7 @@ scenarios: description: "Stale pin (lockfile SHA not reachable from the ref head) produces unreachable-pin/error finding" needs_stub: true tags: [stub] - flags: ["--no-onboard", "--no-narrow", "--no-interactive", "--rescan", "--json=valid,findings"] + flags: ["--no-onboard", "--no-narrow", "--no-interactive", "--json=valid,findings"] fixtures: workflows: ci.yml: From 021f3a48dea800d37cb7d426c9232c06b8c35b58 Mon Sep 17 00:00:00 2001 From: Jeff Martin Date: Mon, 31 Aug 2026 14:42:32 -0700 Subject: [PATCH 02/20] Preserve recorded closures during live resolution --- cmd/gh-actions-lock/check_json_golden_test.go | 21 +-- cmd/gh-actions-lock/command_test.go | 147 +++++++++++++++--- internal/lockfile/state.go | 67 +++++++- internal/lockfile/state_test.go | 79 ++++++++++ internal/pipeline/checks/parsed.go | 2 + internal/pipeline/diagnose.go | 6 +- internal/pipeline/finding_enrich.go | 19 +++ internal/pipeline/parse.go | 50 +++--- internal/pipeline/run.go | 2 +- internal/resolve/cacheentry.go | 1 + internal/resolve/discovery.go | 30 ++-- internal/resolve/resolver_test.go | 22 +++ 12 files changed, 373 insertions(+), 73 deletions(-) diff --git a/cmd/gh-actions-lock/check_json_golden_test.go b/cmd/gh-actions-lock/check_json_golden_test.go index 416704e4..8e8e60b9 100644 --- a/cmd/gh-actions-lock/check_json_golden_test.go +++ b/cmd/gh-actions-lock/check_json_golden_test.go @@ -40,6 +40,7 @@ func TestCheckCommand_JSONGolden(t *testing.T) { checkoutSHA = "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" setupGoSHA = "bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb" cacheSHA = "cccccccccccccccccccccccccccccccccccccccc" + staleSHA = "dddddddddddddddddddddddddddddddddddddddd" helperSHA = "eeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee" ) @@ -55,9 +56,7 @@ func TestCheckCommand_JSONGolden(t *testing.T) { " - uses: actions/cache@v4\n" + " - uses: helper/only-transitive@v1\n" - // Direct refs from the workflow: checkout@v6, setup-go@v6, cache@v3. - // The resolver batches them into a single GraphQL request keyed by - // owner/name pairs (a0/a1/a2). + // Direct refs and the recorded closure are resolved in one GraphQL batch. reg.Register( httpmock.GraphQLForRepo("actions", "checkout"), httpmock.JSONResponse(map[string]any{ @@ -65,19 +64,9 @@ func TestCheckCommand_JSONGolden(t *testing.T) { "a0": testRepoResponse("actions/checkout", checkoutSHA, nodeActionYAML), "a1": testRepoResponse("actions/setup-go", setupGoSHA, compositeYAML), "a2": testRepoResponse("actions/cache", cacheSHA, nodeActionYAML), - }, - }), - ) - - // Transitive batch discovered from the setup-go composite: cache@v4 - // (same NWO as a direct workflow ref) and helper/only-transitive@v1 - // (transitive-only, gives us a populated required_by[] in the JSON). - reg.Register( - httpmock.GraphQLForRepo("actions", "cache"), - httpmock.JSONResponse(map[string]any{ - "data": map[string]any{ - "a0": testRepoResponse("actions/cache", cacheSHA, nodeActionYAML), - "a1": testRepoResponse("helper/only-transitive", helperSHA, nodeActionYAML), + "a3": testRepoResponse("actions/cache", cacheSHA, nodeActionYAML), + "a4": testRepoResponse("helper/only-transitive", helperSHA, nodeActionYAML), + "a5": testRepoResponse("old/dead", staleSHA, nodeActionYAML), }, }), ) diff --git a/cmd/gh-actions-lock/command_test.go b/cmd/gh-actions-lock/command_test.go index 9db180b6..7a3098ab 100644 --- a/cmd/gh-actions-lock/command_test.go +++ b/cmd/gh-actions-lock/command_test.go @@ -27,14 +27,7 @@ func TestCheckCommand_JSONWithHTTPMocks(t *testing.T) { httpmock.JSONResponse(map[string]any{ "data": map[string]any{ "a0": testRepoResponse("actions/checkout", "de0fac2e4500dabe0009e67214ff5f5447ce83dd", nodeActionYAML), - }, - }), - ) - reg.Register( - httpmock.GraphQLForRepo("actions", "setup-go"), - httpmock.JSONResponse(map[string]any{ - "data": map[string]any{ - "a0": testRepoResponse("actions/setup-go", "4a3601121dd01d1626a1e23e37211e3254c1c06c", nodeActionYAML), + "a1": testRepoResponse("actions/setup-go", "4a3601121dd01d1626a1e23e37211e3254c1c06c", nodeActionYAML), }, }), ) @@ -155,6 +148,28 @@ func writeTempLockfile(t *testing.T, repoDir, wfName string, pinStrings []string require.NoError(t, os.WriteFile(p, []byte(sb.String()), 0o600)) } +func writeTempCompositeLockfile(t *testing.T, parentSHA, childNWO, childRef, childSHA string) { + t.Helper() + content := "version: '" + parserlock.Version + "'\n" + + "dependencies:\n" + + " 'example/action@main':\n" + + " ref: 'main'\n" + + " commit: 'sha1-" + parentSHA + "'\n" + + " owner_id: 1\n" + + " repo_id: 1\n" + + " uses:\n" + + " - '" + childNWO + "@" + childRef + "'\n" + + " '" + childNWO + "@" + childRef + "':\n" + + " ref: '" + childRef + "'\n" + + " commit: 'sha1-" + childSHA + "'\n" + + " owner_id: 2\n" + + " repo_id: 2\n" + + "workflows:\n" + + " '.github/workflows/workflow.yml':\n" + + " - 'example/action@main'\n" + require.NoError(t, os.WriteFile(filepath.Join(".github", "workflows", "actions.lock"), []byte(content), 0o600)) +} + // readTempLockfilePins returns the raw pin strings from the actions.lock file // in the current working directory. Useful for assertions in write/upgrade // tests that previously inspected the workflow YAML directly. @@ -231,6 +246,7 @@ func TestCheck_Reachable(t *testing.T) { }, }), ) + workflowPath := writeTempWorkflow(t, ` name: ci on: push @@ -296,7 +312,6 @@ func TestCheck_UnreachablePin_NotAncestor(t *testing.T) { }, }), ) - workflowPath := writeTempWorkflow(t, ` name: ci on: push @@ -464,20 +479,13 @@ func TestCheckCommand_JSONDependenciesWithRequiredBy(t *testing.T) { compositeYAML := "name: Setup Go\nruns:\n using: composite\n steps:\n - uses: actions/cache/save@v4\n" - // Per-ref resolution queries (parallel resolver resolves one ref per worker). + // Direct refs are resolved in one batch. reg.Register( httpmock.GraphQLForRepo("actions", "checkout"), httpmock.JSONResponse(map[string]any{ "data": map[string]any{ "a0": testRepoResponse("actions/checkout", "de0fac2e4500dabe0009e67214ff5f5447ce83dd", nodeActionYAML), - }, - }), - ) - reg.Register( - httpmock.GraphQLForRepo("actions", "setup-go"), - httpmock.JSONResponse(map[string]any{ - "data": map[string]any{ - "a0": testRepoResponse("actions/setup-go", "d35c59abb061a4a6fb18e82ac0862c26744d6ab5", compositeYAML), + "a1": testRepoResponse("actions/setup-go", "d35c59abb061a4a6fb18e82ac0862c26744d6ab5", compositeYAML), }, }), ) @@ -1164,3 +1172,106 @@ jobs: assert.Contains(t, readTempLockfilePins(t), staleSHA, "a default run must not bump a moved branch ref without --relock") } + +func TestCheck_DefaultRun_RetainsMovedCompositeRecordedClosure(t *testing.T) { + reg := &httpmock.Registry{} + defer reg.Verify(t) + + oldParentSHA := "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" + liveParentSHA := "bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb" + oldChildSHA := "cccccccccccccccccccccccccccccccccccccccc" + newChildSHA := "dddddddddddddddddddddddddddddddddddddddd" + liveComposite := "name: Composite\nruns:\n using: composite\n steps:\n - uses: new/child@v2\n" + + reg.Register( + httpmock.GraphQLForRepo("example", "action"), + httpmock.JSONResponse(map[string]any{ + "data": map[string]any{ + "a0": testRepoResponse("example/action", liveParentSHA, liveComposite), + "a1": testRepoResponse("old/child", oldChildSHA, nodeActionYAML), + }, + }), + ) + reg.Register( + httpmock.GraphQLForRepo("new", "child"), + httpmock.JSONResponse(map[string]any{ + "data": map[string]any{ + "a0": testRepoResponse("new/child", newChildSHA, nodeActionYAML), + }, + }), + ) + reg.Register( + httpmock.REST("GET", "repos/example/action/compare/"), + httpmock.JSONResponse(map[string]any{ + "status": "ahead", + "merge_base_commit": map[string]any{"sha": oldParentSHA}, + }), + ) + + workflowPath := writeTempWorkflow(t, ` +name: ci +on: push +jobs: + test: + runs-on: ubuntu-latest + steps: + - uses: example/action@main +`) + writeTempCompositeLockfile(t, oldParentSHA, "old/child", "v1", oldChildSHA) + + stdout, _, err := runCommandWithHTTP(t, reg, "--json=findings", workflowPath) + require.NoError(t, err) + assert.NotContains(t, stdout, `"category": "stale"`) + + lock := readTempLockfilePins(t) + assert.Contains(t, lock, "sha1-"+oldParentSHA) + assert.Contains(t, lock, "'old/child@v1'") + assert.NotContains(t, lock, "'new/child@v2'") +} + +func TestCheck_DefaultRun_RetainsRecordedClosureAfterPartialResolution(t *testing.T) { + reg := &httpmock.Registry{} + defer reg.Verify(t) + + parentSHA := "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" + childSHA := "cccccccccccccccccccccccccccccccccccccccc" + composite := "name: Composite\nruns:\n using: composite\n steps:\n - uses: old/child@v1\n" + reg.Register( + httpmock.GraphQLForRepo("example", "action"), + httpmock.JSONResponse(map[string]any{ + "data": map[string]any{ + "a0": testRepoResponse("example/action", parentSHA, composite), + "a1": nil, + }, + "errors": []any{ + map[string]any{ + "type": "FORBIDDEN", + "message": "Resource protected by organization SAML enforcement.", + "path": []any{"a1"}, + "extensions": map[string]any{ + "saml_failure": true, + }, + }, + }, + }), + ) + workflowPath := writeTempWorkflow(t, ` +name: ci +on: push +jobs: + test: + runs-on: ubuntu-latest + steps: + - uses: example/action@main +`) + writeTempCompositeLockfile(t, parentSHA, "old/child", "v1", childSHA) + + stdout, _, err := runCommandWithHTTP(t, reg, "--json=findings", workflowPath) + require.NoError(t, err) + assert.Contains(t, stdout, `"category": "reachability-unknown"`) + assert.NotContains(t, stdout, "no registered HTTP stubs") + + lock := readTempLockfilePins(t) + assert.Contains(t, lock, "sha1-"+parentSHA) + assert.Contains(t, lock, "'old/child@v1'") +} diff --git a/internal/lockfile/state.go b/internal/lockfile/state.go index 00f96ec3..25bb46c1 100644 --- a/internal/lockfile/state.go +++ b/internal/lockfile/state.go @@ -228,12 +228,64 @@ func (s *State) PruneWorkflows(keep map[string]bool) []string { func (s *State) Get(workflowKey string) ([]dep.Dependency, error) { s.mu.Lock() defer s.mu.Unlock() - deps, ok := s.file.Workflows[workflowKey] + pins, ok := s.file.Workflows[workflowKey] if !ok { return nil, nil } - out := make([]dep.Dependency, 0, len(deps)) - for _, raw := range deps { + return s.dependenciesForPins(pins, workflowKey) +} + +// GetClosure returns the recorded transitive closure and child-to-parent graph +// for workflowKey. +func (s *State) GetClosure(workflowKey string) ([]dep.Dependency, dep.ParentMap, error) { + s.mu.Lock() + defer s.mu.Unlock() + roots, ok := s.file.LookupWorkflow(workflowKey) + if !ok { + return nil, nil, nil + } + pins := make([]string, 0, len(roots)) + queued := make(map[string]bool) + for _, root := range roots { + if !queued[root] { + pins = append(pins, root) + queued[root] = true + } + } + parents := make(dep.ParentMap) + for i := 0; i < len(pins); i++ { + parentPin := pins[i] + parent, ok := parserlock.ParsePin(parentPin) + if !ok { + return nil, nil, fmt.Errorf("invalid pin %q in %s for workflow %q", parentPin, parserlock.Path, workflowKey) + } + action, ok := s.file.Dependencies[parentPin] + if !ok { + continue + } + for _, childPin := range action.Uses { + child, ok := parserlock.ParsePin(childPin) + if !ok { + return nil, nil, fmt.Errorf("invalid pin %q in %s for workflow %q", childPin, parserlock.Path, workflowKey) + } + childKey := pinToDep(child).Key() + parents[childKey] = append(parents[childKey], pinToDep(parent).Key()) + if !queued[childPin] { + pins = append(pins, childPin) + queued[childPin] = true + } + } + } + deps, err := s.dependenciesForPins(pins, workflowKey) + if err != nil { + return nil, nil, err + } + return deps, parents, nil +} + +func (s *State) dependenciesForPins(pins []string, workflowKey string) ([]dep.Dependency, error) { + out := make([]dep.Dependency, 0, len(pins)) + for _, raw := range pins { pin, ok := parserlock.ParsePin(raw) if !ok { return nil, fmt.Errorf("invalid pin %q in %s for workflow %q", raw, parserlock.Path, workflowKey) @@ -406,12 +458,15 @@ func (s *State) Set(ctx context.Context, workflowKey string, deps []dep.Dependen ref = d.Branch } } + commit := d.HashAlgoOrDetect() + "-" + d.SHA if existing, ok := s.file.Dependencies[pinKey]; ok { if ref == "" { ref = existing.Ref } - for _, u := range existing.Uses { - usesSet[u] = true + if existing.Commit == commit { + for _, u := range existing.Uses { + usesSet[u] = true + } } } var uses []string @@ -424,7 +479,7 @@ func (s *State) Set(ctx context.Context, workflowKey string, deps []dep.Dependen } s.file.Dependencies[pinKey] = parserlock.Action{ Ref: ref, - Commit: d.HashAlgoOrDetect() + "-" + d.SHA, + Commit: commit, OwnerID: ids[0], RepoID: ids[1], Uses: uses, diff --git a/internal/lockfile/state_test.go b/internal/lockfile/state_test.go index 39630d3d..fe374c3b 100644 --- a/internal/lockfile/state_test.go +++ b/internal/lockfile/state_test.go @@ -497,6 +497,85 @@ func setupClosure(t *testing.T, dir string) { } } +func TestState_GetClosureTraversesRecordedGraph(t *testing.T) { + dir := t.TempDir() + setupClosure(t, dir) + store, err := LoadState(dir, fakeMetadataResolver{}) + if err != nil { + t.Fatal(err) + } + + deps, parents, err := store.GetClosure(".github/workflows/ci.yml") + if err != nil { + t.Fatal(err) + } + if len(deps) != 2 { + t.Fatalf("expected direct and transitive dependencies, got %+v", deps) + } + gotParents := parents["actions/cache@v4"] + if len(gotParents) != 1 || gotParents[0] != "actions/setup-go@v6" { + t.Fatalf("expected recorded parent edge, got %v", gotParents) + } +} + +func TestState_SetPreservesOrReplacesRecordedClosureByParentCommit(t *testing.T) { + t.Run("unchanged parent keeps recorded children", func(t *testing.T) { + dir := t.TempDir() + setupClosure(t, dir) + parent := dep.Dependency{ + NWO: "actions/setup-go", + Ref: "v6", + Branch: "main", + SHA: "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", + HashAlgo: "sha1", + } + after := resaveBumped( + t, + dir, + ".github/workflows/ci.yml", + []dep.Dependency{parent}, + nil, + map[string]bool{parent.Key(): true}, + ) + if !strings.Contains(string(after), "actions/cache@v4") { + t.Fatalf("recorded child must survive an incomplete resolution:\n%s", after) + } + }) + + t.Run("advanced parent replaces recorded children", func(t *testing.T) { + dir := t.TempDir() + setupClosure(t, dir) + parent := dep.Dependency{ + NWO: "actions/setup-go", + Ref: "v6", + Branch: "main", + SHA: "9999999999999999999999999999999999999999", + HashAlgo: "sha1", + } + child := dep.Dependency{ + NWO: "new/child", + Ref: "v2", + Branch: "main", + SHA: "2222222222222222222222222222222222222222", + HashAlgo: "sha1", + } + after := resaveBumped( + t, + dir, + ".github/workflows/ci.yml", + []dep.Dependency{parent, child}, + map[string][]string{child.Key(): {parent.Key()}}, + map[string]bool{parent.Key(): true}, + ) + if strings.Contains(string(after), "actions/cache@v4") { + t.Fatalf("advanced parent must drop its obsolete child:\n%s", after) + } + if !strings.Contains(string(after), "new/child@v2") { + t.Fatalf("advanced parent must record its live child:\n%s", after) + } + }) +} + // 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 { diff --git a/internal/pipeline/checks/parsed.go b/internal/pipeline/checks/parsed.go index bbd8a921..19860c3d 100644 --- a/internal/pipeline/checks/parsed.go +++ b/internal/pipeline/checks/parsed.go @@ -25,6 +25,8 @@ type ParsedWorkflow struct { SelfRepositoryRefErrs []string SelfRepositoryResolutionErrs []string ExistingDeps []dep.Dependency + RecordedDeps []dep.Dependency + RecordedParents dep.ParentMap ParseWarnings []string LoadErr error DepsErr error diff --git a/internal/pipeline/diagnose.go b/internal/pipeline/diagnose.go index 337f0ffd..59bc6a17 100644 --- a/internal/pipeline/diagnose.go +++ b/internal/pipeline/diagnose.go @@ -70,7 +70,7 @@ func diagnoseOneParsed(ctx context.Context, pw checks.ParsedWorkflow, r *resolve var resolvedParents dep.ParentMap if r != nil { var resolveErr error - liveDeps, resolvedParents, resolveErr = r.ResolveAllRecursive(ctx, pw.Refs) + liveDeps, resolvedParents, resolveErr = r.ResolveAllRecursive(ctx, resolvableRefs(pw)) if resolveErr != nil { blockingResolverError := false if resolve.IsCompositeLocalPath(resolveErr) { @@ -126,7 +126,7 @@ func diagnoseOneParsed(ctx context.Context, pw checks.ParsedWorkflow, r *resolve } parentMap := map[string][]string{} if r != nil { - parentMap = resolvedParents + parentMap = mergeParentMaps(pw.RecordedParents, resolvedParents) populateInventoryParents(wr.Inventory, parentMap) } @@ -136,7 +136,7 @@ func diagnoseOneParsed(ctx context.Context, pw checks.ParsedWorkflow, r *resolve } rawFindings := checks.RunChecks(ctx, pw, store.File(), checkR) - depByKey := indexDeps(pw.ExistingDeps) + depByKey := indexDeps(pw.RecordedDeps) for _, f := range rawFindings { if f.Category == checks.Stale && isTransitivePin(f, depByKey, parentMap) { continue diff --git a/internal/pipeline/finding_enrich.go b/internal/pipeline/finding_enrich.go index bd07f6ae..df1fbfd5 100644 --- a/internal/pipeline/finding_enrich.go +++ b/internal/pipeline/finding_enrich.go @@ -60,3 +60,22 @@ func populateInventoryParents(inventory []checks.InventoryEntry, parentMap map[s } } } + +func mergeParentMaps(maps ...dep.ParentMap) dep.ParentMap { + merged := make(dep.ParentMap) + for _, parentMap := range maps { + for child, parents := range parentMap { + seen := make(map[string]bool, len(merged[child])) + for _, parent := range merged[child] { + seen[parent] = true + } + for _, parent := range parents { + if !seen[parent] { + merged[child] = append(merged[child], parent) + seen[parent] = true + } + } + } + } + return merged +} diff --git a/internal/pipeline/parse.go b/internal/pipeline/parse.go index 42ad040b..78134b82 100644 --- a/internal/pipeline/parse.go +++ b/internal/pipeline/parse.go @@ -4,7 +4,6 @@ import ( "context" parserlock "github.com/github/actions-lockfile/go/pkg/lockfile" - "github.com/github/gh-actions-lock/internal/dep" "github.com/github/gh-actions-lock/internal/ghapi" "github.com/github/gh-actions-lock/internal/lockfile" "github.com/github/gh-actions-lock/internal/pinpool" @@ -21,7 +20,7 @@ import ( func Diagnose(ctx context.Context, paths []string, r *resolve.Resolver, store *lockfile.State, pool *pinpool.Pool) *checks.Report { parsed := ParseAll(paths, store) if r != nil { - refs, _ := CollectResolvable(parsed) + refs := CollectResolvable(parsed) if len(refs) > 0 { _, _, _ = r.ResolveAllRecursive(ctx, refs) } @@ -64,6 +63,13 @@ func ParseAll(paths []string, store *lockfile.State) []checks.ParsedWorkflow { } else { pw.ExistingDeps = deps } + closure, parents, closureErr := store.GetClosure(wfKey) + if closureErr != nil { + pw.DepsErr = closureErr + } else { + pw.RecordedDeps = closure + pw.RecordedParents = parents + } } out = append(out, pw) } @@ -101,14 +107,13 @@ func mergeStrings(groups ...[]string) []string { return values } -// CollectResolvable returns the deduplicated union of refs and existing deps -// across all parsed workflows. Use the returned slices to pre-warm the -// resolver caches once before per-workflow diagnostics. -func CollectResolvable(parsed []checks.ParsedWorkflow) ([]parserlock.ActionRef, []dep.Dependency) { +// CollectResolvable returns the deduplicated union of current workflow refs +// and recorded closure refs across all parsed workflows. +func CollectResolvable(parsed []checks.ParsedWorkflow) []parserlock.ActionRef { seenRef := make(map[ghapi.ActionRef]bool) var refs []parserlock.ActionRef for _, pw := range parsed { - for _, ref := range pw.Refs { + for _, ref := range resolvableRefs(pw) { key := ghapi.ForActionRef(ref.Owner, ref.Repo, ref.Path, ref.Ref) if seenRef[key] { continue @@ -117,19 +122,28 @@ func CollectResolvable(parsed []checks.ParsedWorkflow) ([]parserlock.ActionRef, refs = append(refs, ref) } } - seenDep := make(map[string]bool) - var deps []dep.Dependency - for _, pw := range parsed { - for _, dep := range pw.ExistingDeps { - key := dep.Key() - if seenDep[key] { - continue - } - seenDep[key] = true - deps = append(deps, dep) + return refs +} + +func resolvableRefs(pw checks.ParsedWorkflow) []parserlock.ActionRef { + refs := append([]parserlock.ActionRef(nil), pw.Refs...) + current := make(map[ghapi.NWORef]bool, len(pw.Refs)) + for _, ref := range pw.Refs { + current[ghapi.ForNWORef(ref.Owner, ref.Repo, ref.Ref)] = true + } + for _, d := range pw.RecordedDeps { + owner, repo := d.OwnerRepo() + if current[ghapi.ForNWORef(owner, repo, d.Ref)] { + continue } + refs = append(refs, parserlock.ActionRef{ + Owner: owner, + Repo: repo, + Path: d.Path, + Ref: d.Ref, + }) } - return refs, deps + return refs } // DiagnoseParsed runs the engine diagnostics for each pre-parsed workflow. diff --git a/internal/pipeline/run.go b/internal/pipeline/run.go index 6482e795..2855b065 100644 --- a/internal/pipeline/run.go +++ b/internal/pipeline/run.go @@ -60,7 +60,7 @@ func Run(ctx context.Context, opts RunOptions) (*RunResult, error) { unresolved = append(unresolved, pw) } } - refs, _ := CollectResolvable(unresolved) + refs := CollectResolvable(unresolved) // Phase 2: Resolve. if r == nil { diff --git a/internal/resolve/cacheentry.go b/internal/resolve/cacheentry.go index 69408689..c0d59ba8 100644 --- a/internal/resolve/cacheentry.go +++ b/internal/resolve/cacheentry.go @@ -12,6 +12,7 @@ import "github.com/github/gh-actions-lock/internal/dep" type resolvedEntry struct { dep dep.Dependency actionYML string + err error } // tagPeel records the outcome of a PeelTagObject lookup so repeated checks diff --git a/internal/resolve/discovery.go b/internal/resolve/discovery.go index 0665d7c9..c991d384 100644 --- a/internal/resolve/discovery.go +++ b/internal/resolve/discovery.go @@ -292,6 +292,7 @@ func (r *Resolver) resolveWithActionYMLParallel(ctx context.Context, refs []reso type resolveResult struct { dep dep.Dependency yml string + err error ok bool } results := make([]resolveResult, len(refs)) @@ -299,29 +300,32 @@ func (r *Resolver) resolveWithActionYMLParallel(ctx context.Context, refs []reso var uncachedIdx []int for i, request := range refs { if entry, ok := r.cache.Get(cacheKey(request.ref)); ok { - results[i] = resolveResult{dep: entry.dep, yml: entry.actionYML, ok: true} + results[i] = resolveResult{dep: entry.dep, yml: entry.actionYML, err: entry.err, ok: entry.err == nil} } else { uncachedIdx = append(uncachedIdx, i) } } - flatten := func() ([]dep.Dependency, []string) { + flatten := func() ([]dep.Dependency, []string, error) { var deps []dep.Dependency var ymls []string + var errs []error for _, res := range results { + if res.err != nil { + errs = append(errs, res.err) + } if !res.ok { continue } deps = append(deps, res.dep) ymls = append(ymls, res.yml) } - return deps, ymls + return deps, ymls, errors.Join(errs...) } total := len(uncachedIdx) if total == 0 { - deps, ymls := flatten() - return deps, ymls, nil + return flatten() } // Grow the rolling resolve total by the new uncached refs at this depth. @@ -379,7 +383,6 @@ func (r *Resolver) resolveWithActionYMLParallel(ctx context.Context, refs []reso }, func(ctx context.Context, _ int, b actionBatch) error { res := r.gh.ResolveActionFiles(ctx, b.reqs) - var errs []error for j, idx := range b.idxs { ref := refs[idx].ref if j < len(res) && res[j].Err == nil { @@ -391,18 +394,23 @@ func (r *Resolver) resolveWithActionYMLParallel(ctx context.Context, refs []reso } r.cache.Put(cacheKey(ref), resolvedEntry{dep: d, actionYML: res[j].ActionYML}) results[idx] = resolveResult{dep: d, yml: res[j].ActionYML, ok: true} - } else if j < len(res) && res[j].Err != nil { - errs = append(errs, fmt.Errorf("%s@%s: %w", ref.NWO(), ref.Ref, res[j].Err)) + } else { + resolveErr := fmt.Errorf("%s@%s: no resolution result", ref.NWO(), ref.Ref) + if j < len(res) { + resolveErr = fmt.Errorf("%s@%s: %w", ref.NWO(), ref.Ref, res[j].Err) + } + r.cache.Put(cacheKey(ref), resolvedEntry{err: resolveErr}) + results[idx] = resolveResult{err: resolveErr} } done := resolveDone.Add(1) r.FireResolveProgress(int(done), int(resolveTotal.Load())) } - return errors.Join(errs...) + return nil }, ) - deps, ymls := flatten() - return deps, ymls, poolErr + deps, ymls, cachedErr := flatten() + return deps, ymls, errors.Join(poolErr, cachedErr) } // selectLatestTag returns the highest semver tag from a list of tag names. diff --git a/internal/resolve/resolver_test.go b/internal/resolve/resolver_test.go index a80ed59d..65f875b0 100644 --- a/internal/resolve/resolver_test.go +++ b/internal/resolve/resolver_test.go @@ -950,6 +950,28 @@ func TestResolveAllRecursivePartialFailureCachedGood(t *testing.T) { } } +func TestResolveAllRecursiveCachesFailures(t *testing.T) { + reg := &httpmock.Registry{} + defer reg.Verify(t) + reg.Register( + httpmock.GraphQLForRepo("bad", "private"), + httpmock.JSONResponse(map[string]any{ + "data": map[string]any{"a0": nil}, + }), + ) + + r, err := New("github.com", pinpool.New(2, nil), WithTransport(reg)) + require.NoError(t, err) + refs := []parserlock.ActionRef{{Owner: "bad", Repo: "private", Ref: "main"}} + + _, _, firstErr := r.ResolveAllRecursive(context.Background(), refs) + require.Error(t, firstErr) + _, _, secondErr := r.ResolveAllRecursive(context.Background(), refs) + require.Error(t, secondErr) + assert.Equal(t, firstErr.Error(), secondErr.Error()) + assert.Len(t, reg.Requests, 1, "a failed ref should be requested once per resolver run") +} + // gitRefHeadResponse returns a git/ref response for an exact branch match. func gitRefHeadResponse(name, sha string) any { return map[string]any{ From bb4d799d3802762a42a2b809ad62843a59ac8d47 Mon Sep 17 00:00:00 2001 From: Jeff Martin Date: Mon, 31 Aug 2026 15:05:58 -0700 Subject: [PATCH 03/20] Validate closure before relocking --- cmd/gh-actions-lock/command_test.go | 110 ++++++++++++++++++++++++++++ internal/pin/plan.go | 12 ++- internal/pipeline/checks/run.go | 10 +++ 3 files changed, 129 insertions(+), 3 deletions(-) diff --git a/cmd/gh-actions-lock/command_test.go b/cmd/gh-actions-lock/command_test.go index 7a3098ab..efeb885c 100644 --- a/cmd/gh-actions-lock/command_test.go +++ b/cmd/gh-actions-lock/command_test.go @@ -1275,3 +1275,113 @@ jobs: assert.Contains(t, lock, "sha1-"+parentSHA) assert.Contains(t, lock, "'old/child@v1'") } + +func TestCheck_Relock_BumpsMovedTransitive(t *testing.T) { + reg := &httpmock.Registry{} + defer reg.Verify(t) + + parentSHA := "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" + oldChildSHA := "bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb" + liveChildSHA := "cccccccccccccccccccccccccccccccccccccccc" + composite := "name: Composite\nruns:\n using: composite\n steps:\n - uses: old/child@v1\n" + reg.Register( + httpmock.GraphQLForRepo("example", "action"), + httpmock.JSONResponse(map[string]any{ + "data": map[string]any{ + "a0": testRepoResponse("example/action", parentSHA, composite), + "a1": testRepoResponse("old/child", liveChildSHA, nodeActionYAML), + }, + }), + ) + reg.Register( + httpmock.REST("GET", "repos/old/child/compare/"), + httpmock.JSONResponse(map[string]any{ + "status": "ahead", + "merge_base_commit": map[string]any{"sha": oldChildSHA}, + }), + ) + + workflowPath := writeTempWorkflow(t, ` +name: ci +on: push +jobs: + test: + runs-on: ubuntu-latest + steps: + - uses: example/action@main +`) + writeTempCompositeLockfile(t, parentSHA, "old/child", "v1", oldChildSHA) + + _, _, err := runCommandWithHTTP(t, reg, "--relock", "--no-narrow", workflowPath) + require.NoError(t, err) + + lock := readTempLockfilePins(t) + assert.Contains(t, lock, "sha1-"+parentSHA) + assert.Contains(t, lock, "sha1-"+liveChildSHA) + assert.NotContains(t, lock, "sha1-"+oldChildSHA) +} + +func TestCheck_Relock_PreservesClosureOnPartialResolution(t *testing.T) { + reg := &httpmock.Registry{} + defer reg.Verify(t) + + oldParentSHA := "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" + liveParentSHA := "bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb" + oldChildSHA := "cccccccccccccccccccccccccccccccccccccccc" + liveComposite := "name: Composite\nruns:\n using: composite\n steps:\n - uses: new/child@v2\n" + reg.Register( + httpmock.GraphQLForRepo("example", "action"), + httpmock.JSONResponse(map[string]any{ + "data": map[string]any{ + "a0": testRepoResponse("example/action", liveParentSHA, liveComposite), + "a1": testRepoResponse("old/child", oldChildSHA, nodeActionYAML), + }, + }), + ) + reg.Register( + httpmock.GraphQLForRepo("new", "child"), + httpmock.JSONResponse(map[string]any{ + "data": map[string]any{"a0": nil}, + "errors": []any{ + map[string]any{ + "type": "FORBIDDEN", + "message": "Resource protected by organization SAML enforcement.", + "path": []any{"a0"}, + "extensions": map[string]any{ + "saml_failure": true, + }, + }, + }, + }), + ) + reg.Register( + httpmock.REST("GET", "repos/example/action/compare/"), + httpmock.JSONResponse(map[string]any{ + "status": "ahead", + "merge_base_commit": map[string]any{"sha": oldParentSHA}, + }), + ) + + workflowPath := writeTempWorkflow(t, ` +name: ci +on: push +jobs: + test: + runs-on: ubuntu-latest + steps: + - uses: example/action@main +`) + writeTempCompositeLockfile(t, oldParentSHA, "old/child", "v1", oldChildSHA) + + stdout, _, err := runCommandWithHTTP(t, reg, + "--relock", "--no-narrow", "--json=findings", workflowPath, + ) + require.NoError(t, err) + assert.Contains(t, stdout, `"category": "reachability-unknown"`) + + lock := readTempLockfilePins(t) + assert.Contains(t, lock, "sha1-"+oldParentSHA) + assert.NotContains(t, lock, "sha1-"+liveParentSHA) + assert.Contains(t, lock, "'old/child@v1'") + assert.NotContains(t, lock, "'new/child@v2'") +} diff --git a/internal/pin/plan.go b/internal/pin/plan.go index aac509e9..1b72bb76 100644 --- a/internal/pin/plan.go +++ b/internal/pin/plan.go @@ -132,7 +132,8 @@ func planWorkflow(ctx context.Context, wr checks.WorkflowReport, opts PlanOption // 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, opts.AcceptMoved, opts.Relock) - repinMoved := repinsMoved(opts) && wr.CountByCategory(checks.RefMoved) > 0 + repinMoved := repinsMoved(opts) && wr.CountByCategory(checks.RefMoved) > 0 || + opts.AcceptMoved && wr.CountByCategory(checks.UnreachablePin) > 0 if !wr.NeedsAttention() && !repinMoved { entries = verifiedEntries(inventory, wr.Path) @@ -179,7 +180,13 @@ func planWorkflow(ctx context.Context, wr checks.WorkflowReport, opts PlanOption status("resolving " + wr.Path) deps, parentMap, resolveErr := opts.Resolver.ResolveAllRecursive(ctx, unrecordedRefs) if resolveErr != nil { - entries = append(entries, unresolvedEntries(wr, unrecordedRefs, deps, resolveErr)...) + unresolved := unresolvedEntries(wr, unrecordedRefs, deps, resolveErr) + if repinMoved { + entries = append(verifiedEntries(wr.Inventory, wr.Path), unresolved...) + wplans = append(wplans, WorkflowPlan{Path: wr.Path, SelfActionFiles: wr.SelfActionFiles}) + return planResult{entries: entries, wplans: wplans}, nil + } + entries = append(entries, unresolved...) if len(deps) == 0 { wplans = append(wplans, WorkflowPlan{Path: wr.Path, SelfActionFiles: wr.SelfActionFiles}) return planResult{entries: entries, wplans: wplans}, nil @@ -602,7 +609,6 @@ func pruneStaleInventory(inventory []checks.InventoryEntry, findings []checks.Fi return out } -// repinsMoved reports whether this run re-resolves benign ref-moved deps. func repinsMoved(opts PlanOptions) bool { return opts.Relock || opts.AcceptMoved } diff --git a/internal/pipeline/checks/run.go b/internal/pipeline/checks/run.go index 49bd315b..fd911cd8 100644 --- a/internal/pipeline/checks/run.go +++ b/internal/pipeline/checks/run.go @@ -31,6 +31,16 @@ func RunChecks(ctx context.Context, pw ParsedWorkflow, lf parserlock.File, r Che if r != nil { out = append(out, checkMisleadingSha(ctx, pw, r)...) + for _, d := range pw.RecordedDeps { + pin, ok := parserlock.ParsePin(d.Key()) + if !ok { + continue + } + depIndex[pin.IndexKey()] = lockedPin{ + Pin: pin, + Commit: d.HashAlgoOrDetect() + "-" + d.SHA, + } + } refMoved := checkRefMovedAndForgery(ctx, pw, depIndex, r) out = append(out, refMoved...) } From 374e1668b6872b9ed45d9a1f7934ef8f7347a06c Mon Sep 17 00:00:00 2001 From: Jeff Martin Date: Mon, 31 Aug 2026 15:14:34 -0700 Subject: [PATCH 04/20] Verify: surface dependency resolution failures --- cmd/gh-actions-lock/command_test.go | 39 +++++++++++++++++++++++++++++ cmd/gh-actions-lock/run.go | 3 +++ 2 files changed, 42 insertions(+) diff --git a/cmd/gh-actions-lock/command_test.go b/cmd/gh-actions-lock/command_test.go index efeb885c..cb8049f4 100644 --- a/cmd/gh-actions-lock/command_test.go +++ b/cmd/gh-actions-lock/command_test.go @@ -1385,3 +1385,42 @@ jobs: assert.Contains(t, lock, "'old/child@v1'") assert.NotContains(t, lock, "'new/child@v2'") } + +func TestCheck_VerifySurfacesResolverFailure(t *testing.T) { + reg := &httpmock.Registry{} + defer reg.Verify(t) + reg.Register( + httpmock.GraphQLForRepo("example", "action"), + httpmock.JSONResponse(map[string]any{ + "data": map[string]any{"a0": nil}, + "errors": []any{ + map[string]any{ + "type": "FORBIDDEN", + "message": "Resource protected by organization SAML enforcement.", + "path": []any{"a0"}, + "extensions": map[string]any{ + "saml_failure": true, + }, + }, + }, + }), + ) + + workflowPath := writeTempWorkflow(t, ` +name: ci +on: push +jobs: + test: + runs-on: ubuntu-latest + steps: + - uses: example/action@main +`, + "example/action@main=sha1-aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", + ) + + _, stderr, err := runCommandWithHTTP(t, reg, "--verify", workflowPath) + require.NoError(t, err) + assert.Contains(t, stderr, "Dependency verification was inconclusive") + assert.Contains(t, stderr, "could not re-resolve actions") + assert.Equal(t, 1, strings.Count(stderr, "Dependency verification was inconclusive")) +} diff --git a/cmd/gh-actions-lock/run.go b/cmd/gh-actions-lock/run.go index e81bd8f5..4cc852fb 100644 --- a/cmd/gh-actions-lock/run.go +++ b/cmd/gh-actions-lock/run.go @@ -323,6 +323,9 @@ func runCheck(cmd *cobra.Command, opts *checkOptions, newResolver resolverFunc) console.TermDetail("%s", hint) } } + if opts.jsonFields == "" { + renderResolverWarning(console, report) + } if !valid { if opts.jsonFields == "" { hasFixable := format.PresentReadOnlyFailures(console, report) From 74b0d14c0a5aaf23315e3354b1bf56c6069bb70a Mon Sep 17 00:00:00 2001 From: Jeff Martin Date: Tue, 1 Sep 2026 07:21:22 -0700 Subject: [PATCH 05/20] Relock: refresh moved transitives with new roots --- cmd/gh-actions-lock/command_test.go | 15 +++++++++++++-- internal/pin/plan.go | 9 +++------ 2 files changed, 16 insertions(+), 8 deletions(-) diff --git a/cmd/gh-actions-lock/command_test.go b/cmd/gh-actions-lock/command_test.go index cb8049f4..c886ba94 100644 --- a/cmd/gh-actions-lock/command_test.go +++ b/cmd/gh-actions-lock/command_test.go @@ -1276,20 +1276,22 @@ jobs: assert.Contains(t, lock, "'old/child@v1'") } -func TestCheck_Relock_BumpsMovedTransitive(t *testing.T) { +func TestCheck_Relock_BumpsMovedTransitiveAlongsideNewDirect(t *testing.T) { reg := &httpmock.Registry{} defer reg.Verify(t) parentSHA := "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" oldChildSHA := "bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb" liveChildSHA := "cccccccccccccccccccccccccccccccccccccccc" + newDirectSHA := "dddddddddddddddddddddddddddddddddddddddd" composite := "name: Composite\nruns:\n using: composite\n steps:\n - uses: old/child@v1\n" reg.Register( httpmock.GraphQLForRepo("example", "action"), httpmock.JSONResponse(map[string]any{ "data": map[string]any{ "a0": testRepoResponse("example/action", parentSHA, composite), - "a1": testRepoResponse("old/child", liveChildSHA, nodeActionYAML), + "a1": testRepoResponse("new/direct", newDirectSHA, nodeActionYAML), + "a2": testRepoResponse("old/child", liveChildSHA, nodeActionYAML), }, }), ) @@ -1300,6 +1302,13 @@ func TestCheck_Relock_BumpsMovedTransitive(t *testing.T) { "merge_base_commit": map[string]any{"sha": oldChildSHA}, }), ) + reg.Register( + httpmock.REST("GET", `repos/new/direct$`), + httpmock.JSONResponse(map[string]any{ + "id": 3, + "owner": map[string]any{"id": 2}, + }), + ) workflowPath := writeTempWorkflow(t, ` name: ci @@ -1309,6 +1318,7 @@ jobs: runs-on: ubuntu-latest steps: - uses: example/action@main + - uses: new/direct@v1 `) writeTempCompositeLockfile(t, parentSHA, "old/child", "v1", oldChildSHA) @@ -1317,6 +1327,7 @@ jobs: lock := readTempLockfilePins(t) assert.Contains(t, lock, "sha1-"+parentSHA) + assert.Contains(t, lock, "sha1-"+newDirectSHA) assert.Contains(t, lock, "sha1-"+liveChildSHA) assert.NotContains(t, lock, "sha1-"+oldChildSHA) } diff --git a/internal/pin/plan.go b/internal/pin/plan.go index 1b72bb76..09b4a72a 100644 --- a/internal/pin/plan.go +++ b/internal/pin/plan.go @@ -160,12 +160,9 @@ func planWorkflow(ctx context.Context, wr checks.WorkflowReport, opts PlanOption unrecordedRefs, inventorySHA := partitionByInventory(inventory, wr.ActionRefs) entries = verifiedEntries(inventory, wr.Path) - // A moved *transitive* dep is pruned from inventory but is not a direct - // ActionRef, so partitionByInventory marks nothing unrecorded and the - // verified fast path would silently drop it instead of bumping it. Force - // the workflow's direct roots through recursive resolution so the moved - // transitive is re-pinned to its current SHA. - if len(unrecordedRefs) == 0 && repinMoved { + // A moved transitive is absent from the direct ActionRefs, so refresh the + // complete scoped closure whenever movement is accepted. + if repinMoved { unrecordedRefs, inventorySHA = partitionByInventory(nil, wr.ActionRefs) entries = verifiedEntries(nil, wr.Path) } From 8fffc1d5f2b75f39bb749e6aab7b4af293a5c249 Mon Sep 17 00:00:00 2001 From: Jeff Martin Date: Tue, 1 Sep 2026 07:34:58 -0700 Subject: [PATCH 06/20] Pipeline: retain recorded transitive inventory --- cmd/gh-actions-lock/check_json_golden_test.go | 4 +-- cmd/gh-actions-lock/command_test.go | 34 ++++++++----------- .../.github/workflows/actions.lock | 1 - .../testdata/golden-json/expected.json | 10 ++++-- internal/pipeline/diagnose.go | 17 ++++------ internal/pipeline/finding_enrich.go | 6 ++-- 6 files changed, 34 insertions(+), 38 deletions(-) diff --git a/cmd/gh-actions-lock/check_json_golden_test.go b/cmd/gh-actions-lock/check_json_golden_test.go index 8e8e60b9..03b34ac0 100644 --- a/cmd/gh-actions-lock/check_json_golden_test.go +++ b/cmd/gh-actions-lock/check_json_golden_test.go @@ -65,8 +65,8 @@ func TestCheckCommand_JSONGolden(t *testing.T) { "a1": testRepoResponse("actions/setup-go", setupGoSHA, compositeYAML), "a2": testRepoResponse("actions/cache", cacheSHA, nodeActionYAML), "a3": testRepoResponse("actions/cache", cacheSHA, nodeActionYAML), - "a4": testRepoResponse("helper/only-transitive", helperSHA, nodeActionYAML), - "a5": testRepoResponse("old/dead", staleSHA, nodeActionYAML), + "a4": testRepoResponse("old/dead", staleSHA, nodeActionYAML), + "a5": testRepoResponse("helper/only-transitive", helperSHA, nodeActionYAML), }, }), ) diff --git a/cmd/gh-actions-lock/command_test.go b/cmd/gh-actions-lock/command_test.go index c886ba94..77fc8e84 100644 --- a/cmd/gh-actions-lock/command_test.go +++ b/cmd/gh-actions-lock/command_test.go @@ -555,25 +555,20 @@ jobs: } } -func TestCheckCommand_JSONDependenciesInfersRequiredByWithoutComments(t *testing.T) { +func TestCheckCommand_JSONDependenciesIncludesRecordedClosure(t *testing.T) { reg := &httpmock.Registry{} defer reg.Verify(t) - compositeYAML := "name: Setup Go\nruns:\n using: composite\n steps:\n - uses: actions/cache/save@v4\n" + parentSHA := "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" + childSHA := "bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb" + compositeYAML := "name: Composite\nruns:\n using: composite\n steps:\n - uses: example/action@v1\n" reg.Register( - httpmock.GraphQLForRepo("actions", "setup-go"), + httpmock.GraphQLForRepo("example", "action"), httpmock.JSONResponse(map[string]any{ "data": map[string]any{ - "a0": testRepoResponse("actions/setup-go", "d35c59abb061a4a6fb18e82ac0862c26744d6ab5", compositeYAML), - }, - }), - ) - reg.Register( - httpmock.GraphQLForRepo("actions", "cache"), - httpmock.JSONResponse(map[string]any{ - "data": map[string]any{ - "a0": testRepoResponse("actions/cache", "5a3ec84eff668545956fd18022155c47e93e2684", nodeActionYAML), + "a0": testRepoResponse("example/action", parentSHA, compositeYAML), + "a1": testRepoResponse("example/action", childSHA, nodeActionYAML), }, }), ) @@ -585,11 +580,9 @@ jobs: test: runs-on: ubuntu-latest steps: - - uses: actions/setup-go@v6 -`, - "actions/setup-go@v6", - "actions/cache@v4", - ) + - uses: example/action@main +`) + writeTempCompositeLockfile(t, parentSHA, "example/action", "v1", childSHA) stdout, _, err := runCommandWithHTTP(t, reg, "--no-fix", "--json=workflows", workflowPath, @@ -603,17 +596,18 @@ jobs: } require.NoError(t, json.Unmarshal([]byte(stdout), &payload)) require.Len(t, payload.Workflows, 1) + require.Len(t, payload.Workflows[0].Dependencies, 2) var transitiveDep *format.Dependency for i := range payload.Workflows[0].Dependencies { - if payload.Workflows[0].Dependencies[i].NWO == "actions/cache" { + if payload.Workflows[0].Dependencies[i].Ref == "v1" { transitiveDep = &payload.Workflows[0].Dependencies[i] break } } - require.NotNil(t, transitiveDep, "transitive dep actions/cache should be present") + require.NotNil(t, transitiveDep, "recorded transitive dependency should be present") assert.False(t, transitiveDep.Direct) - assert.Equal(t, []string{"actions/setup-go@v6"}, transitiveDep.RequiredBy) + assert.Equal(t, []string{"example/action@main"}, transitiveDep.RequiredBy) } func TestCheckCommand_JSONDefaultFieldsExcludesDependencies(t *testing.T) { diff --git a/cmd/gh-actions-lock/testdata/golden-json/.github/workflows/actions.lock b/cmd/gh-actions-lock/testdata/golden-json/.github/workflows/actions.lock index 19293eba..8ce711bf 100644 --- a/cmd/gh-actions-lock/testdata/golden-json/.github/workflows/actions.lock +++ b/cmd/gh-actions-lock/testdata/golden-json/.github/workflows/actions.lock @@ -33,5 +33,4 @@ workflows: - 'actions/checkout@v6' - 'actions/setup-go@v6' - 'actions/cache@v4' - - 'helper/only-transitive@v1' - 'old/dead@v1' diff --git a/cmd/gh-actions-lock/testdata/golden-json/expected.json b/cmd/gh-actions-lock/testdata/golden-json/expected.json index f6a8527b..601fdb5f 100644 --- a/cmd/gh-actions-lock/testdata/golden-json/expected.json +++ b/cmd/gh-actions-lock/testdata/golden-json/expected.json @@ -2,10 +2,13 @@ "cli_version": "\u003cMASKED\u003e", "dependencies": [ { - "direct": true, + "direct": false, "hash_algo": "sha1", "nwo": "actions/cache", "ref": "v4", + "required_by": [ + "actions/setup-go@v6" + ], "sha": "cccccccccccccccccccccccccccccccccccccccc" }, { @@ -68,10 +71,13 @@ { "dependencies": [ { - "direct": true, + "direct": false, "hash_algo": "sha1", "nwo": "actions/cache", "ref": "v4", + "required_by": [ + "actions/setup-go@v6" + ], "sha": "cccccccccccccccccccccccccccccccccccccccc" }, { diff --git a/internal/pipeline/diagnose.go b/internal/pipeline/diagnose.go index 59bc6a17..64cf01cd 100644 --- a/internal/pipeline/diagnose.go +++ b/internal/pipeline/diagnose.go @@ -58,9 +58,9 @@ func diagnoseOneParsed(ctx context.Context, pw checks.ParsedWorkflow, r *resolve wr.Findings = append(wr.Findings, selfRepositoryFinding(pw)) } - directNWOs := make(map[ghapi.Repo]bool, len(pw.Refs)) + directRefs := make(map[ghapi.NWORef]bool, len(pw.Refs)) for _, ref := range pw.Refs { - directNWOs[ghapi.ForRepo(ref.Owner, ref.Repo)] = true + directRefs[ghapi.ForNWORef(ref.Owner, ref.Repo, ref.Ref)] = true } // Resolve live state: hits cache when ParseAll's caller pre-warmed the @@ -116,19 +116,16 @@ func diagnoseOneParsed(ctx context.Context, pw checks.ParsedWorkflow, r *resolve } } - for _, dep := range pw.ExistingDeps { + for _, dep := range pw.RecordedDeps { owner, repo := dep.OwnerRepo() wr.Inventory = append(wr.Inventory, checks.InventoryEntry{ Dep: dep, File: pw.Path, - Direct: directNWOs[ghapi.ForRepo(owner, repo)], + Direct: directRefs[ghapi.ForNWORef(owner, repo, dep.Ref)], }) } - parentMap := map[string][]string{} - if r != nil { - parentMap = mergeParentMaps(pw.RecordedParents, resolvedParents) - populateInventoryParents(wr.Inventory, parentMap) - } + parentMap := mergeParentMaps(pw.RecordedParents, resolvedParents) + populateInventoryParents(wr.Inventory, parentMap) var checkR checks.CheckResolver if r != nil && liveDeps != nil { @@ -141,7 +138,7 @@ func diagnoseOneParsed(ctx context.Context, pw checks.ParsedWorkflow, r *resolve if f.Category == checks.Stale && isTransitivePin(f, depByKey, parentMap) { continue } - attachParent(&f, depByKey, directNWOs, parentMap) + attachParent(&f, depByKey, directRefs, parentMap) f.DocURL = DocURLFor(f.Category) wr.Findings = append(wr.Findings, f) } diff --git a/internal/pipeline/finding_enrich.go b/internal/pipeline/finding_enrich.go index df1fbfd5..67bb0b9c 100644 --- a/internal/pipeline/finding_enrich.go +++ b/internal/pipeline/finding_enrich.go @@ -14,15 +14,15 @@ import ( // and a Dependency synthesized from the workflow ref / lockfile pin. This // is purely about pointing the user at the composite that pulled in a // transitively-pinned dep. -func attachParent(f *checks.Finding, depByKey map[string]dep.Dependency, directNWOs map[ghapi.Repo]bool, parentMap map[string][]string) { +func attachParent(f *checks.Finding, depByKey map[string]dep.Dependency, directRefs map[ghapi.NWORef]bool, parentMap map[string][]string) { if f.Dependency == nil { return } owner, repo := f.Dependency.OwnerRepo() - if directNWOs[ghapi.ForRepo(owner, repo)] { + if directRefs[ghapi.ForNWORef(owner, repo, f.Dependency.Ref)] { return } - // Prefer the dep snapshot from the workflow's ExistingDeps (it has the + // Prefer the dep snapshot from the workflow's RecordedDeps (it has the // canonical NWO casing the parent map keys with). Synthesised deps // already match — but the indexed lookup is cheap regardless. key := f.Dependency.Key() From 90c7b03b8c3958775a57bea6f7abb54435f61d0a Mon Sep 17 00:00:00 2001 From: Jeff Martin Date: Tue, 1 Sep 2026 07:36:02 -0700 Subject: [PATCH 07/20] CI: retry CodeQL analysis From 5884a531c2f5cb469fe326fbd36d57e2f9aee9db Mon Sep 17 00:00:00 2001 From: Jeff Martin Date: Tue, 1 Sep 2026 08:00:00 -0700 Subject: [PATCH 08/20] Checks: stabilize transitive movement findings --- cmd/gh-actions-lock/command_test.go | 2 +- internal/pipeline/checks/misleading.go | 9 +++++++- internal/pipeline/checks/run_test.go | 32 ++++++++++++++++++++++++++ 3 files changed, 41 insertions(+), 2 deletions(-) diff --git a/cmd/gh-actions-lock/command_test.go b/cmd/gh-actions-lock/command_test.go index 77fc8e84..705583a7 100644 --- a/cmd/gh-actions-lock/command_test.go +++ b/cmd/gh-actions-lock/command_test.go @@ -1426,6 +1426,6 @@ jobs: _, stderr, err := runCommandWithHTTP(t, reg, "--verify", workflowPath) require.NoError(t, err) assert.Contains(t, stderr, "Dependency verification was inconclusive") - assert.Contains(t, stderr, "could not re-resolve actions") assert.Equal(t, 1, strings.Count(stderr, "Dependency verification was inconclusive")) + assert.Equal(t, 1, strings.Count(stderr, "could not re-resolve actions")) } diff --git a/internal/pipeline/checks/misleading.go b/internal/pipeline/checks/misleading.go index 1ee64e38..46bff138 100644 --- a/internal/pipeline/checks/misleading.go +++ b/internal/pipeline/checks/misleading.go @@ -3,6 +3,7 @@ package checks import ( "context" "fmt" + "sort" "strings" parserlock "github.com/github/actions-lockfile/go/pkg/lockfile" @@ -76,10 +77,16 @@ func checkRefMovedAndForgery(ctx context.Context, pw ParsedWorkflow, depIndex ma // Check transitive deps (recorded in lockfile but not directly in the // workflow). These can also drift when upstream releases new versions. - for indexKey, pin := range depIndex { + var transitiveKeys []string + for indexKey := range depIndex { if directKeys[indexKey] { continue } + transitiveKeys = append(transitiveKeys, indexKey) + } + sort.Strings(transitiveKeys) + for _, indexKey := range transitiveKeys { + pin := depIndex[indexKey] parsed, ok := parserlock.ParsePin(indexKey) if !ok { continue diff --git a/internal/pipeline/checks/run_test.go b/internal/pipeline/checks/run_test.go index 5a443cc6..98810bc3 100644 --- a/internal/pipeline/checks/run_test.go +++ b/internal/pipeline/checks/run_test.go @@ -569,6 +569,38 @@ func TestRunChecks(t *testing.T) { } } +func TestCheckRefMovedAndForgery_TransitiveOrderIsStable(t *testing.T) { + alpha, ok := parserlock.ParsePin("alpha/action@v1") + if !ok { + t.Fatal("parse alpha pin") + } + zeta, ok := parserlock.ParsePin("zeta/action@v1") + if !ok { + t.Fatal("parse zeta pin") + } + depIndex := map[string]lockedPin{ + alpha.IndexKey(): {Pin: alpha, Commit: "sha1-" + shaCheckoutV3}, + zeta.IndexKey(): {Pin: zeta, Commit: "sha1-" + shaSetupGoV5}, + } + r := &stubCheckResolver{ + refs: map[stubRefKey]string{ + {"alpha", "action", "v1"}: shaCheckoutV4, + {"zeta", "action", "v1"}: shaImpostor, + }, + ancestry: map[stubAncestryKey]resolve.AncestryStatus{ + {"alpha", "action", shaCheckoutV3, shaCheckoutV4}: resolve.AncestryConfirmed, + {"zeta", "action", shaSetupGoV5, shaImpostor}: resolve.AncestryConfirmed, + }, + } + + for range 32 { + got := checkRefMovedAndForgery(context.Background(), ParsedWorkflow{Path: ".github/workflows/ci.yml"}, depIndex, r) + if len(got) != 2 || got[0].Dependency.NWO != "alpha/action" || got[1].Dependency.NWO != "zeta/action" { + t.Fatalf("transitive findings are not stable: %#v", got) + } + } +} + // TestRunChecks_AllFindingsCarryConfidence is the fail-fast guard the // confidence-axis card requires: every finding emitted by any check // path must carry a non-empty Confidence. A zero value here would mean From bbab407dd50cecaf4037a481de57bf6b6068dd3b Mon Sep 17 00:00:00 2001 From: Jeff Martin Date: Tue, 1 Sep 2026 08:00:54 -0700 Subject: [PATCH 09/20] CI: retry CodeQL analysis From 8a644d16990fb9df9de9ec4e66579fa2c0ca3bc9 Mon Sep 17 00:00:00 2001 From: Jeff Martin Date: Tue, 1 Sep 2026 08:15:06 -0700 Subject: [PATCH 10/20] Pipeline: retain symbolic roots for SHA uses --- cmd/gh-actions-lock/command_test.go | 58 +++++++++++++++++++++++++++++ internal/pipeline/diagnose.go | 7 +++- internal/pipeline/finding_enrich.go | 11 +++++- 3 files changed, 72 insertions(+), 4 deletions(-) diff --git a/cmd/gh-actions-lock/command_test.go b/cmd/gh-actions-lock/command_test.go index 705583a7..3d0fccb2 100644 --- a/cmd/gh-actions-lock/command_test.go +++ b/cmd/gh-actions-lock/command_test.go @@ -610,6 +610,64 @@ jobs: assert.Equal(t, []string{"example/action@main"}, transitiveDep.RequiredBy) } +func TestCheckCommand_BareSHAKeepsSymbolicLockRootDirect(t *testing.T) { + reg := &httpmock.Registry{} + defer reg.Verify(t) + + sha := "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" + reg.Register( + httpmock.GraphQLForRepo("example", "action"), + httpmock.JSONResponse(map[string]any{ + "data": map[string]any{ + "a0": testRepoResponse("example/action", sha, nodeActionYAML), + "a1": testRepoResponse("example/action", sha, nodeActionYAML), + }, + }), + ) + reg.Register( + httpmock.REST("GET", "repos/example/action/branches"), + httpmock.JSONResponse([]any{}), + ) + reg.Register( + httpmock.REST("GET", "repos/example/action/branches"), + httpmock.JSONResponse([]any{}), + ) + reg.Register( + httpmock.REST("GET", "repos/example/action/tags"), + httpmock.JSONResponse([]any{ + map[string]any{"name": "v1", "commit": map[string]any{"sha": sha}}, + }), + ) + + workflowPath := writeTempWorkflow(t, ` +name: ci +on: push +jobs: + test: + runs-on: ubuntu-latest + steps: + - uses: example/action@`+sha+` +`, + "example/action@v1=sha1-"+sha, + ) + + stdout, _, err := runCommandWithHTTP(t, reg, + "--no-narrow", "--json=workflows", workflowPath, + ) + require.NoError(t, err) + + var payload struct { + Workflows []struct { + Dependencies []format.Dependency `json:"dependencies"` + } `json:"workflows"` + } + require.NoError(t, json.Unmarshal([]byte(stdout), &payload)) + require.Len(t, payload.Workflows, 1) + require.Len(t, payload.Workflows[0].Dependencies, 1) + assert.True(t, payload.Workflows[0].Dependencies[0].Direct) + assert.Contains(t, readTempLockfilePins(t), " - 'example/action@v1'") +} + func TestCheckCommand_JSONDefaultFieldsExcludesDependencies(t *testing.T) { reg := &httpmock.Registry{} defer reg.Verify(t) diff --git a/internal/pipeline/diagnose.go b/internal/pipeline/diagnose.go index 64cf01cd..324c3be2 100644 --- a/internal/pipeline/diagnose.go +++ b/internal/pipeline/diagnose.go @@ -8,6 +8,7 @@ import ( "fmt" "strings" + parserlock "github.com/github/actions-lockfile/go/pkg/lockfile" "github.com/github/gh-actions-lock/internal/dep" "github.com/github/gh-actions-lock/internal/ghapi" "github.com/github/gh-actions-lock/internal/lockfile" @@ -61,6 +62,9 @@ func diagnoseOneParsed(ctx context.Context, pw checks.ParsedWorkflow, r *resolve directRefs := make(map[ghapi.NWORef]bool, len(pw.Refs)) for _, ref := range pw.Refs { directRefs[ghapi.ForNWORef(ref.Owner, ref.Repo, ref.Ref)] = true + if parserlock.IsFullSha(ref.Ref) { + directRefs[ghapi.ForNWORef(ref.Owner, ref.Repo, strings.ToLower(ref.Ref))] = true + } } // Resolve live state: hits cache when ParseAll's caller pre-warmed the @@ -117,11 +121,10 @@ func diagnoseOneParsed(ctx context.Context, pw checks.ParsedWorkflow, r *resolve } for _, dep := range pw.RecordedDeps { - owner, repo := dep.OwnerRepo() wr.Inventory = append(wr.Inventory, checks.InventoryEntry{ Dep: dep, File: pw.Path, - Direct: directRefs[ghapi.ForNWORef(owner, repo, dep.Ref)], + Direct: isDirectDependency(dep, directRefs), }) } parentMap := mergeParentMaps(pw.RecordedParents, resolvedParents) diff --git a/internal/pipeline/finding_enrich.go b/internal/pipeline/finding_enrich.go index 67bb0b9c..63d6b94f 100644 --- a/internal/pipeline/finding_enrich.go +++ b/internal/pipeline/finding_enrich.go @@ -1,6 +1,8 @@ package pipeline import ( + "strings" + "github.com/github/gh-actions-lock/internal/dep" "github.com/github/gh-actions-lock/internal/ghapi" "github.com/github/gh-actions-lock/internal/pipeline/checks" @@ -18,8 +20,7 @@ func attachParent(f *checks.Finding, depByKey map[string]dep.Dependency, directR if f.Dependency == nil { return } - owner, repo := f.Dependency.OwnerRepo() - if directRefs[ghapi.ForNWORef(owner, repo, f.Dependency.Ref)] { + if isDirectDependency(*f.Dependency, directRefs) { return } // Prefer the dep snapshot from the workflow's RecordedDeps (it has the @@ -34,6 +35,12 @@ func attachParent(f *checks.Finding, depByKey map[string]dep.Dependency, directR } } +func isDirectDependency(d dep.Dependency, directRefs map[ghapi.NWORef]bool) bool { + owner, repo := d.OwnerRepo() + return directRefs[ghapi.ForNWORef(owner, repo, d.Ref)] || + d.SHA != "" && directRefs[ghapi.ForNWORef(owner, repo, strings.ToLower(d.SHA))] +} + // isTransitivePin reports whether the finding refers to a dep reached via // composite expansion (i.e. has parents in the parent map). func isTransitivePin(f checks.Finding, depByKey map[string]dep.Dependency, parentMap map[string][]string) bool { From b2bb38fe15e81457139507933dd66cbe640bf30a Mon Sep 17 00:00:00 2001 From: Jeff Martin Date: Tue, 1 Sep 2026 08:15:14 -0700 Subject: [PATCH 11/20] CI: retry CodeQL analysis From 310d8095171a124df4451a01e113d4e1ff4dc785 Mon Sep 17 00:00:00 2001 From: Jeff Martin Date: Tue, 1 Sep 2026 08:29:07 -0700 Subject: [PATCH 12/20] Lockfile: preserve recorded graph until movement is accepted --- cmd/gh-actions-lock/command_test.go | 121 ++++++++++++++++++++----- internal/dep/dependency.go | 11 +++ internal/pipeline/checks/misleading.go | 8 ++ internal/pipeline/diagnose.go | 19 +--- internal/pipeline/finding_enrich.go | 12 +-- 5 files changed, 126 insertions(+), 45 deletions(-) diff --git a/cmd/gh-actions-lock/command_test.go b/cmd/gh-actions-lock/command_test.go index 3d0fccb2..af87c542 100644 --- a/cmd/gh-actions-lock/command_test.go +++ b/cmd/gh-actions-lock/command_test.go @@ -477,6 +477,9 @@ func TestCheckCommand_JSONDependenciesWithRequiredBy(t *testing.T) { reg := &httpmock.Registry{} defer reg.Verify(t) + checkoutSHA := "de0fac2e4500dabe0009e67214ff5f5447ce83dd" + setupGoSHA := "d35c59abb061a4a6fb18e82ac0862c26744d6ab5" + cacheSHA := "5a3ec84eff668545956fd18022155c47e93e2684" compositeYAML := "name: Setup Go\nruns:\n using: composite\n steps:\n - uses: actions/cache/save@v4\n" // Direct refs are resolved in one batch. @@ -484,8 +487,8 @@ func TestCheckCommand_JSONDependenciesWithRequiredBy(t *testing.T) { httpmock.GraphQLForRepo("actions", "checkout"), httpmock.JSONResponse(map[string]any{ "data": map[string]any{ - "a0": testRepoResponse("actions/checkout", "de0fac2e4500dabe0009e67214ff5f5447ce83dd", nodeActionYAML), - "a1": testRepoResponse("actions/setup-go", "d35c59abb061a4a6fb18e82ac0862c26744d6ab5", compositeYAML), + "a0": testRepoResponse("actions/checkout", checkoutSHA, nodeActionYAML), + "a1": testRepoResponse("actions/setup-go", setupGoSHA, compositeYAML), }, }), ) @@ -494,7 +497,7 @@ func TestCheckCommand_JSONDependenciesWithRequiredBy(t *testing.T) { httpmock.GraphQLForRepo("actions", "cache"), httpmock.JSONResponse(map[string]any{ "data": map[string]any{ - "a0": testRepoResponse("actions/cache", "5a3ec84eff668545956fd18022155c47e93e2684", nodeActionYAML), + "a0": testRepoResponse("actions/cache", cacheSHA, nodeActionYAML), }, }), ) @@ -508,12 +511,31 @@ jobs: steps: - uses: actions/checkout@v6 - uses: actions/setup-go@v6 -`, - "actions/checkout@v6", - "actions/setup-go@v6", - // Transitive dependency (via actions/setup-go@v6). - "actions/cache@v4", - ) +`) + lock := "version: '" + parserlock.Version + "'\n" + + "dependencies:\n" + + " 'actions/checkout@v6':\n" + + " ref: 'v6'\n" + + " commit: 'sha1-" + checkoutSHA + "'\n" + + " owner_id: 1\n" + + " repo_id: 1\n" + + " 'actions/setup-go@v6':\n" + + " ref: 'v6'\n" + + " commit: 'sha1-" + setupGoSHA + "'\n" + + " owner_id: 2\n" + + " repo_id: 2\n" + + " uses:\n" + + " - 'actions/cache@v4'\n" + + " 'actions/cache@v4':\n" + + " ref: 'v4'\n" + + " commit: 'sha1-" + cacheSHA + "'\n" + + " owner_id: 3\n" + + " repo_id: 3\n" + + "workflows:\n" + + " '.github/workflows/workflow.yml':\n" + + " - 'actions/checkout@v6'\n" + + " - 'actions/setup-go@v6'\n" + require.NoError(t, os.WriteFile(filepath.Join(".github", "workflows", "actions.lock"), []byte(lock), 0o600)) // Test per-workflow dependencies view stdout, _, err := runCommandWithHTTP(t, reg, @@ -668,6 +690,44 @@ jobs: assert.Contains(t, readTempLockfilePins(t), " - 'example/action@v1'") } +func TestCheckCommand_BareSHASkipsSymbolicRefMovement(t *testing.T) { + reg := &httpmock.Registry{} + defer reg.Verify(t) + + sha := "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" + movedSHA := "bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb" + reg.Register( + httpmock.GraphQLForRepo("example", "action"), + httpmock.JSONResponse(map[string]any{ + "data": map[string]any{ + "a0": testRepoResponse("example/action", sha, nodeActionYAML), + "a1": testRepoResponse("example/action", movedSHA, nodeActionYAML), + }, + }), + ) + + workflowPath := writeTempWorkflow(t, ` +name: ci +on: push +jobs: + test: + runs-on: ubuntu-latest + steps: + - uses: example/action@`+sha+` +`, + "example/action@v1=sha1-"+sha, + ) + + stdout, _, err := runCommandWithHTTP(t, reg, + "--no-fix", "--json=findings", workflowPath, + ) + require.NoError(t, err) + assert.NotContains(t, stdout, `"category": "ref-moved"`) + assert.NotContains(t, stdout, `"category": "ancestry-unknown"`) + assert.NotContains(t, stdout, `"category": "unreachable-pin"`) + assert.NotContains(t, stdout, "gh actions-lock --relock") +} + func TestCheckCommand_JSONDefaultFieldsExcludesDependencies(t *testing.T) { reg := &httpmock.Registry{} defer reg.Verify(t) @@ -1244,14 +1304,6 @@ func TestCheck_DefaultRun_RetainsMovedCompositeRecordedClosure(t *testing.T) { }, }), ) - reg.Register( - httpmock.GraphQLForRepo("new", "child"), - httpmock.JSONResponse(map[string]any{ - "data": map[string]any{ - "a0": testRepoResponse("new/child", newChildSHA, nodeActionYAML), - }, - }), - ) reg.Register( httpmock.REST("GET", "repos/example/action/compare/"), httpmock.JSONResponse(map[string]any{ @@ -1269,16 +1321,39 @@ jobs: steps: - uses: example/action@main `) - writeTempCompositeLockfile(t, oldParentSHA, "old/child", "v1", oldChildSHA) + lock := "version: '" + parserlock.Version + "'\n" + + "dependencies:\n" + + " 'example/action@main':\n" + + " ref: 'main'\n" + + " commit: 'sha1-" + oldParentSHA + "'\n" + + " owner_id: 1\n" + + " repo_id: 1\n" + + " uses:\n" + + " - 'old/child@v1'\n" + + " 'old/child@v1':\n" + + " ref: 'v1'\n" + + " commit: 'sha1-" + oldChildSHA + "'\n" + + " owner_id: 2\n" + + " repo_id: 2\n" + + " 'new/child@v2':\n" + + " ref: 'v2'\n" + + " commit: 'sha1-" + newChildSHA + "'\n" + + " owner_id: 3\n" + + " repo_id: 3\n" + + "workflows:\n" + + " '.github/workflows/workflow.yml':\n" + + " - 'example/action@main'\n" + + " - 'new/child@v2'\n" + require.NoError(t, os.WriteFile(filepath.Join(".github", "workflows", "actions.lock"), []byte(lock), 0o600)) stdout, _, err := runCommandWithHTTP(t, reg, "--json=findings", workflowPath) require.NoError(t, err) - assert.NotContains(t, stdout, `"category": "stale"`) + assert.Contains(t, stdout, `"category": "stale"`) - lock := readTempLockfilePins(t) - assert.Contains(t, lock, "sha1-"+oldParentSHA) - assert.Contains(t, lock, "'old/child@v1'") - assert.NotContains(t, lock, "'new/child@v2'") + updatedLock := readTempLockfilePins(t) + assert.Contains(t, updatedLock, "sha1-"+oldParentSHA) + assert.Contains(t, updatedLock, "'old/child@v1'") + assert.NotContains(t, updatedLock, "'new/child@v2'") } func TestCheck_DefaultRun_RetainsRecordedClosureAfterPartialResolution(t *testing.T) { diff --git a/internal/dep/dependency.go b/internal/dep/dependency.go index 86dc7f03..4327a929 100644 --- a/internal/dep/dependency.go +++ b/internal/dep/dependency.go @@ -55,6 +55,17 @@ func (d Dependency) OwnerRepo() (string, string) { return owner, repo } +// MatchesActionRef reports whether ref names this dependency directly, +// including a bare SHA that matches symbolic lock metadata. +func (d Dependency) MatchesActionRef(ref parserlock.ActionRef) bool { + owner, repo := d.OwnerRepo() + if !strings.EqualFold(owner, ref.Owner) || !strings.EqualFold(repo, ref.Repo) { + return false + } + return d.Ref == ref.Ref || + parserlock.IsFullSha(ref.Ref) && strings.EqualFold(d.SHA, ref.Ref) +} + // HashAlgoOrDetect returns the hash algorithm, falling back to detection from SHA length. func (d Dependency) HashAlgoOrDetect() string { if d.HashAlgo != "" { diff --git a/internal/pipeline/checks/misleading.go b/internal/pipeline/checks/misleading.go index 46bff138..6fcefee5 100644 --- a/internal/pipeline/checks/misleading.go +++ b/internal/pipeline/checks/misleading.go @@ -3,10 +3,12 @@ package checks import ( "context" "fmt" + "slices" "sort" "strings" parserlock "github.com/github/actions-lockfile/go/pkg/lockfile" + "github.com/github/gh-actions-lock/internal/dep" "github.com/github/gh-actions-lock/internal/resolve" ) @@ -99,6 +101,12 @@ func checkRefMovedAndForgery(ctx context.Context, pw ParsedWorkflow, depIndex ma Repo: parsed.Repo, Ref: parsed.Ref, } + recorded := dep.Dependency{NWO: parsed.NWO, Ref: parsed.Ref, SHA: pin.SHA()} + if slices.ContainsFunc(pw.Refs, func(ref parserlock.ActionRef) bool { + return parserlock.IsFullSha(ref.Ref) && recorded.MatchesActionRef(ref) + }) { + continue + } if f, ok := checkOneRefMoved(ctx, pw, ref, pin, r); ok { out = append(out, f) } diff --git a/internal/pipeline/diagnose.go b/internal/pipeline/diagnose.go index 324c3be2..c936c751 100644 --- a/internal/pipeline/diagnose.go +++ b/internal/pipeline/diagnose.go @@ -8,9 +8,7 @@ import ( "fmt" "strings" - parserlock "github.com/github/actions-lockfile/go/pkg/lockfile" "github.com/github/gh-actions-lock/internal/dep" - "github.com/github/gh-actions-lock/internal/ghapi" "github.com/github/gh-actions-lock/internal/lockfile" "github.com/github/gh-actions-lock/internal/pinpool" "github.com/github/gh-actions-lock/internal/pipeline/checks" @@ -59,22 +57,13 @@ func diagnoseOneParsed(ctx context.Context, pw checks.ParsedWorkflow, r *resolve wr.Findings = append(wr.Findings, selfRepositoryFinding(pw)) } - directRefs := make(map[ghapi.NWORef]bool, len(pw.Refs)) - for _, ref := range pw.Refs { - directRefs[ghapi.ForNWORef(ref.Owner, ref.Repo, ref.Ref)] = true - if parserlock.IsFullSha(ref.Ref) { - directRefs[ghapi.ForNWORef(ref.Owner, ref.Repo, strings.ToLower(ref.Ref))] = true - } - } - // Resolve live state: hits cache when ParseAll's caller pre-warmed the // resolver. Failure degrades to structural-only checks for any refs that // couldn't be resolved — partial results are kept. var liveDeps []dep.Dependency - var resolvedParents dep.ParentMap if r != nil { var resolveErr error - liveDeps, resolvedParents, resolveErr = r.ResolveAllRecursive(ctx, resolvableRefs(pw)) + liveDeps, _, resolveErr = r.ResolveAllRecursive(ctx, resolvableRefs(pw)) if resolveErr != nil { blockingResolverError := false if resolve.IsCompositeLocalPath(resolveErr) { @@ -124,10 +113,10 @@ func diagnoseOneParsed(ctx context.Context, pw checks.ParsedWorkflow, r *resolve wr.Inventory = append(wr.Inventory, checks.InventoryEntry{ Dep: dep, File: pw.Path, - Direct: isDirectDependency(dep, directRefs), + Direct: isDirectDependency(dep, pw.Refs), }) } - parentMap := mergeParentMaps(pw.RecordedParents, resolvedParents) + parentMap := pw.RecordedParents populateInventoryParents(wr.Inventory, parentMap) var checkR checks.CheckResolver @@ -141,7 +130,7 @@ func diagnoseOneParsed(ctx context.Context, pw checks.ParsedWorkflow, r *resolve if f.Category == checks.Stale && isTransitivePin(f, depByKey, parentMap) { continue } - attachParent(&f, depByKey, directRefs, parentMap) + attachParent(&f, depByKey, pw.Refs, parentMap) f.DocURL = DocURLFor(f.Category) wr.Findings = append(wr.Findings, f) } diff --git a/internal/pipeline/finding_enrich.go b/internal/pipeline/finding_enrich.go index 63d6b94f..248fe31d 100644 --- a/internal/pipeline/finding_enrich.go +++ b/internal/pipeline/finding_enrich.go @@ -1,10 +1,10 @@ package pipeline import ( - "strings" + "slices" + parserlock "github.com/github/actions-lockfile/go/pkg/lockfile" "github.com/github/gh-actions-lock/internal/dep" - "github.com/github/gh-actions-lock/internal/ghapi" "github.com/github/gh-actions-lock/internal/pipeline/checks" ) @@ -16,7 +16,7 @@ import ( // and a Dependency synthesized from the workflow ref / lockfile pin. This // is purely about pointing the user at the composite that pulled in a // transitively-pinned dep. -func attachParent(f *checks.Finding, depByKey map[string]dep.Dependency, directRefs map[ghapi.NWORef]bool, parentMap map[string][]string) { +func attachParent(f *checks.Finding, depByKey map[string]dep.Dependency, directRefs []parserlock.ActionRef, parentMap map[string][]string) { if f.Dependency == nil { return } @@ -35,10 +35,8 @@ func attachParent(f *checks.Finding, depByKey map[string]dep.Dependency, directR } } -func isDirectDependency(d dep.Dependency, directRefs map[ghapi.NWORef]bool) bool { - owner, repo := d.OwnerRepo() - return directRefs[ghapi.ForNWORef(owner, repo, d.Ref)] || - d.SHA != "" && directRefs[ghapi.ForNWORef(owner, repo, strings.ToLower(d.SHA))] +func isDirectDependency(d dep.Dependency, directRefs []parserlock.ActionRef) bool { + return slices.ContainsFunc(directRefs, d.MatchesActionRef) } // isTransitivePin reports whether the finding refers to a dep reached via From 9186f95bcc1d84c493087c6c84f98f61c83e7ff7 Mon Sep 17 00:00:00 2001 From: Jeff Martin Date: Tue, 1 Sep 2026 08:29:07 -0700 Subject: [PATCH 13/20] CI: retry CodeQL analysis From 0a7eaaa4c82b9a42ee8ee44564f680cf8643ecf0 Mon Sep 17 00:00:00 2001 From: Jeff Martin Date: Tue, 1 Sep 2026 09:24:17 -0700 Subject: [PATCH 14/20] Lockfile: preserve unscoped transitive edges --- internal/lockfile/state.go | 45 ++++++++++++++++++++++++++++++++++++- internal/pin/commit.go | 6 ++++- internal/pin/commit_test.go | 40 +++++++++++++++++++++++++++++++++ 3 files changed, 89 insertions(+), 2 deletions(-) diff --git a/internal/lockfile/state.go b/internal/lockfile/state.go index 25bb46c1..9f6caf92 100644 --- a/internal/lockfile/state.go +++ b/internal/lockfile/state.go @@ -351,6 +351,16 @@ func (s *State) AllDeps() []dep.Dependency { // Resolution of owner/repo numeric IDs happens lazily per NWO and is cached // for the lifetime of the store. func (s *State) Set(ctx context.Context, workflowKey string, deps []dep.Dependency, parentMap map[string][]string, directKeys map[string]bool) error { + return s.set(ctx, workflowKey, deps, parentMap, directKeys, nil) +} + +// SetScoped updates a workflow while preserving existing graph edges on +// dependencies also reached by workflows outside the current command scope. +func (s *State) SetScoped(ctx context.Context, workflowKey string, deps []dep.Dependency, parentMap map[string][]string, directKeys, scopedWorkflows map[string]bool) error { + return s.set(ctx, workflowKey, deps, parentMap, directKeys, scopedWorkflows) +} + +func (s *State) set(ctx context.Context, workflowKey string, deps []dep.Dependency, parentMap map[string][]string, directKeys, scopedWorkflows map[string]bool) error { // Resolve repo IDs for every unique owner/repo BEFORE taking s.mu so // concurrent pin workers don't serialize on the network round-trip. // lookupIDs is safe to call without s.mu and dedups in-flight fetches @@ -463,7 +473,10 @@ func (s *State) Set(ctx context.Context, workflowKey string, deps []dep.Dependen if ref == "" { ref = existing.Ref } - if existing.Commit == commit { + if existing.Commit == commit || s.reachableFromUnscopedWorkflow(pinKey, workflowKey, scopedWorkflows) { + // ponytail: actions.lock stores a global edge union, so keep the + // old union when an untouched workflow reaches this parent. + // A full-scope refresh can replace it exactly. for _, u := range existing.Uses { usesSet[u] = true } @@ -490,6 +503,36 @@ func (s *State) Set(ctx context.Context, workflowKey string, deps []dep.Dependen return nil } +func (s *State) reachableFromUnscopedWorkflow(target, workflowKey string, scopedWorkflows map[string]bool) bool { + var reaches func(string, map[string]bool) bool + reaches = func(key string, seen map[string]bool) bool { + if key == target { + return true + } + if seen[key] { + return false + } + seen[key] = true + for _, child := range s.file.Dependencies[key].Uses { + if reaches(child, seen) { + return true + } + } + return false + } + for workflow, roots := range s.file.Workflows { + if workflow == workflowKey || scopedWorkflows[workflow] { + continue + } + for _, root := range roots { + if reaches(root, make(map[string]bool)) { + return true + } + } + } + return false +} + // Save persists the lockfile to disk, garbage-collecting orphan action // entries (pins referenced by no workflow). When the in-memory file is empty // after GC, the on-disk file is removed. diff --git a/internal/pin/commit.go b/internal/pin/commit.go index 6dcc3eef..8086e46b 100644 --- a/internal/pin/commit.go +++ b/internal/pin/commit.go @@ -60,6 +60,10 @@ func Commit(ctx context.Context, rec *Record, store *lockfile.State, copts *Comm // Phase 2: Update lockfile entries for each scanned workflow. pinnedByWorkflow := groupPinnedByWorkflow(rec) + scopedWorkflows := make(map[string]bool, len(rec.Workflows)) + for _, wp := range rec.Workflows { + scopedWorkflows[workflowfile.KeyFromPath(wp.Path)] = true + } if len(rec.Workflows) > 0 { progress("Updating lockfile") } @@ -73,7 +77,7 @@ func Commit(ctx context.Context, rec *Record, store *lockfile.State, copts *Comm parentMap := buildParentMap(rec, wfPath) directKeys := buildDirectKeys(rec, wfPath) deps = retainUnresolvablePins(rec, store, wfPath, deps, directKeys) - if err := store.Set(ctx, wfKey, deps, parentMap, directKeys); err != nil { + if err := store.SetScoped(ctx, wfKey, deps, parentMap, directKeys, scopedWorkflows); err != nil { return fmt.Errorf("updating lockfile for %s: %w", wfPath, err) } } diff --git a/internal/pin/commit_test.go b/internal/pin/commit_test.go index 9abe31bf..3e23af36 100644 --- a/internal/pin/commit_test.go +++ b/internal/pin/commit_test.go @@ -150,3 +150,43 @@ jobs: assert.NotContains(t, string(got), "owner/action@main") assert.NotContains(t, string(got), "actions/setup-go@v5") } + +func TestCommitScopedParentAdvanceKeepsUntouchedWorkflowEdges(t *testing.T) { + dir := t.TempDir() + ciPath := filepath.Join(".github", "workflows", "ci.yml") + releasePath := filepath.Join(".github", "workflows", "release.yml") + require.NoError(t, os.MkdirAll(filepath.Join(dir, ".github", "workflows"), 0o755)) + require.NoError(t, os.WriteFile(filepath.Join(dir, ciPath), []byte("on: push\njobs: {}\n"), 0o644)) + require.NoError(t, os.WriteFile(filepath.Join(dir, releasePath), []byte("on: push\njobs: {}\n"), 0o644)) + t.Chdir(dir) + + ctx := context.Background() + store, err := lockfile.LoadState(dir, fakeMeta{}) + require.NoError(t, err) + parent := dep.Dependency{NWO: "owner/composite", Ref: "main", SHA: strings.Repeat("1", 40), HashAlgo: "sha1"} + releaseChild := dep.Dependency{NWO: "owner/release-child", Ref: "v1", SHA: strings.Repeat("2", 40), HashAlgo: "sha1"} + require.NoError(t, store.Set(ctx, ciPath, + []dep.Dependency{parent}, nil, map[string]bool{parent.Key(): true})) + require.NoError(t, store.Set(ctx, releasePath, + []dep.Dependency{parent, releaseChild}, + map[string][]string{releaseChild.Key(): {parent.Key()}}, + map[string]bool{parent.Key(): true})) + require.NoError(t, store.Save()) + + newParentSHA := strings.Repeat("3", 40) + ciChildSHA := strings.Repeat("4", 40) + rec := &Record{ + Entries: []Entry{ + {NWO: parent.NWO, Ref: parent.Ref, SHA: newParentSHA, Resolution: Pinned, Direct: true, Workflows: []string{ciPath}}, + {NWO: "owner/ci-child", Ref: "v2", SHA: ciChildSHA, Resolution: Pinned, RequiredBy: []string{parent.Key()}, Workflows: []string{ciPath}}, + }, + Workflows: []WorkflowPlan{{Path: ciPath}}, + } + require.NoError(t, Commit(ctx, rec, store, nil)) + + file := store.File() + action := file.Dependencies["owner/composite@main"] + assert.Equal(t, "sha1-"+newParentSHA, action.Commit) + assert.Equal(t, []string{"owner/ci-child@v2", "owner/release-child@v1"}, action.Uses) + assert.Contains(t, file.Dependencies, "owner/release-child@v1") +} From b4da9b7ec9c688bf66f996d22a2ceb1096562f4c Mon Sep 17 00:00:00 2001 From: Jeff Martin Date: Tue, 1 Sep 2026 09:28:05 -0700 Subject: [PATCH 15/20] Tests: refresh checkout v4 live fixture --- test/integration/run.rb | 2 +- test/scenarios/catalog.yml | 12 ++++++------ 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/test/integration/run.rb b/test/integration/run.rb index 17a3dc93..d8b7ebed 100644 --- a/test/integration/run.rb +++ b/test/integration/run.rb @@ -346,7 +346,7 @@ def golden_json_diff(expected, actual, path) # ── Fixture data ──────────────────────────────────────────────────────── -CHECKOUT_SHA = "de0fac2e4500dabe0009e67214ff5f5447ce83dd" +CHECKOUT_SHA = "11d5960a326750d5838078e36cf38b85af677262" SETUP_GO_SHA = "4a3601121dd01d1626a1e23e37211e3254c1c06c" CACHE_SHA = "27d5ce7f107fe9357f9df03efb73ab90386fccae" MAIN_BRANCH_SHA = "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" diff --git a/test/scenarios/catalog.yml b/test/scenarios/catalog.yml index 4f1d5e15..f40a4908 100644 --- a/test/scenarios/catalog.yml +++ b/test/scenarios/catalog.yml @@ -1687,7 +1687,7 @@ scenarios: hash_algo: sha1 nwo: actions/checkout ref: v4 - sha: de0fac2e4500dabe0009e67214ff5f5447ce83dd + sha: 11d5960a326750d5838078e36cf38b85af677262 findings: [] path: .github/workflows/ci.yml valid: true @@ -1782,7 +1782,7 @@ scenarios: hash_algo: sha1 nwo: actions/checkout ref: v4 - sha: de0fac2e4500dabe0009e67214ff5f5447ce83dd + sha: 11d5960a326750d5838078e36cf38b85af677262 findings: - category: onboarding-required confidence: high @@ -1860,7 +1860,7 @@ scenarios: hash_algo: sha1 nwo: actions/checkout ref: v4 - sha: de0fac2e4500dabe0009e67214ff5f5447ce83dd + sha: 11d5960a326750d5838078e36cf38b85af677262 findings: [] path: .github/workflows/ci.yml valid: true @@ -1937,7 +1937,7 @@ scenarios: - "version: 'v0.0.2'" - "'actions/checkout@v4':" - "ref: 'v4'" - - "commit: 'sha1-de0fac2e4500dabe0009e67214ff5f5447ce83dd'" + - "commit: 'sha1-11d5960a326750d5838078e36cf38b85af677262'" - name: dbot_impostor_blocks category: dependabot @@ -1993,7 +1993,7 @@ scenarios: - category: unreachable-pin confidence: high dependency: actions/checkout@v4 - detail: "pinned de0fac2e4500 is not an ancestor of bbbbbbbbbbbb \u2014 lockfile may have been tampered with" + detail: "pinned 11d5960a3267 is not an ancestor of bbbbbbbbbbbb \u2014 lockfile may have been tampered with" doc_url: "https://docs.github.com/en/actions/security-for-github-actions/security-guides/security-hardening-for-github-actions#using-third-party-actions" remediation: "investigate immediately \u2014 verify the lockfile entry against upstream history" severity: error @@ -2042,7 +2042,7 @@ scenarios: - "'actions/checkout@main':" - "ref: 'main'" - "commit: 'sha1-aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa'" - lockfile_comment_excludes: "de0fac2e4500dabe0009e67214ff5f5447ce83dd" + lockfile_comment_excludes: "11d5960a326750d5838078e36cf38b85af677262" - name: onboard_roundtrip_v002 category: onboarding From 4bb62a8afaaf035b401c91b75916da07bd6ec16d Mon Sep 17 00:00:00 2001 From: Jeff Martin Date: Tue, 1 Sep 2026 09:31:53 -0700 Subject: [PATCH 16/20] Tests: separate checkout ref fixtures --- test/integration/run.rb | 17 +++++++++++++---- test/scenarios/catalog.yml | 2 +- 2 files changed, 14 insertions(+), 5 deletions(-) diff --git a/test/integration/run.rb b/test/integration/run.rb index d8b7ebed..1b04eddd 100644 --- a/test/integration/run.rb +++ b/test/integration/run.rb @@ -347,6 +347,8 @@ def golden_json_diff(expected, actual, path) # ── Fixture data ──────────────────────────────────────────────────────── CHECKOUT_SHA = "11d5960a326750d5838078e36cf38b85af677262" +CHECKOUT_FULL_SHA = "d632683dd7b4114ad314bca15554477dd762a938" +CHECKOUT_MAIN_SHA = "f548e57e544e1ff5a4c46bf1e1b8685f8e4a348a" SETUP_GO_SHA = "4a3601121dd01d1626a1e23e37211e3254c1c06c" CACHE_SHA = "27d5ce7f107fe9357f9df03efb73ab90386fccae" MAIN_BRANCH_SHA = "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" @@ -441,7 +443,7 @@ def golden_json_diff(expected, actual, path) dependencies: { "actions/checkout@v4.2.0" => { "ref" => "v4.2.0", - "commit" => "sha1-#{CHECKOUT_SHA}", + "commit" => "sha1-#{CHECKOUT_FULL_SHA}", "owner_id" => 44036562, "repo_id" => 197814629 } @@ -460,7 +462,7 @@ def golden_json_diff(expected, actual, path) dependencies: { "actions/checkout@main" => { "ref" => "main", - "commit" => "sha1-#{CHECKOUT_SHA}", + "commit" => "sha1-#{CHECKOUT_MAIN_SHA}", "owner_id" => 44036562, "repo_id" => 197814629 } @@ -576,11 +578,18 @@ def checkout_graphql_success(srv) query = body["query"] || "" if query.include?("expression") + sha = if query.include?("v4.2.0") + CHECKOUT_FULL_SHA + elsif query.include?("main:") + CHECKOUT_MAIN_SHA + else + CHECKOUT_SHA + end [200, { "Content-Type" => "application/json" }, JSON.generate({ data: { a0: { nameWithOwner: "actions/checkout", object: { - oid: CHECKOUT_SHA, + oid: sha, file: { object: { text: "name: Checkout\ndescription: Checkout\nruns:\n using: node20\n main: dist/index.js\n" @@ -795,7 +804,7 @@ def wire_checkout_fresh(s, token) # the move is benign (ref-moved, not an unreachable pin). srv.on(:GET, %r{/repos/actions/checkout/compare/}) do |_req| [200, { "Content-Type" => "application/json" }, - JSON.generate({ status: "behind", merge_base_commit: { sha: CHECKOUT_SHA } })] + JSON.generate({ status: "behind", merge_base_commit: { sha: CHECKOUT_MAIN_SHA } })] end end s.env("GH_TOKEN" => "gho_fake_relock_token") diff --git a/test/scenarios/catalog.yml b/test/scenarios/catalog.yml index f40a4908..0ba8b64c 100644 --- a/test/scenarios/catalog.yml +++ b/test/scenarios/catalog.yml @@ -2042,7 +2042,7 @@ scenarios: - "'actions/checkout@main':" - "ref: 'main'" - "commit: 'sha1-aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa'" - lockfile_comment_excludes: "11d5960a326750d5838078e36cf38b85af677262" + lockfile_comment_excludes: "f548e57e544e1ff5a4c46bf1e1b8685f8e4a348a" - name: onboard_roundtrip_v002 category: onboarding From a995bf0c3635fa75172446130f83d27040f658f1 Mon Sep 17 00:00:00 2001 From: Jeff Martin Date: Tue, 1 Sep 2026 09:38:52 -0700 Subject: [PATCH 17/20] Planner: keep moved refs non-blocking --- cmd/gh-actions-lock/command_test.go | 53 +++++++++++++++++++++++++++++ internal/pin/plan.go | 17 ++------- 2 files changed, 56 insertions(+), 14 deletions(-) diff --git a/cmd/gh-actions-lock/command_test.go b/cmd/gh-actions-lock/command_test.go index af87c542..57f8679f 100644 --- a/cmd/gh-actions-lock/command_test.go +++ b/cmd/gh-actions-lock/command_test.go @@ -1029,6 +1029,59 @@ jobs: assert.True(t, hasRefMoved, "default runs must detect ref movement: %+v", payload.Findings) } +func TestCheck_DefaultRun_MovedRefRemainsNonBlockingAlongsideNewDependency(t *testing.T) { + reg := &httpmock.Registry{} + defer reg.Verify(t) + + staleSHA := "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" + liveSHA := "bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb" + newSHA := "cccccccccccccccccccccccccccccccccccccccc" + reg.Register( + httpmock.GraphQLForRepo("example", "action"), + httpmock.JSONResponse(map[string]any{ + "data": map[string]any{ + "a0": testRepoResponse("example/action", liveSHA, nodeActionYAML), + "a1": testRepoResponse("example/new", newSHA, nodeActionYAML), + }, + }), + ) + reg.Register( + httpmock.REST("GET", "repos/example/action/compare/"), + httpmock.JSONResponse(map[string]any{ + "status": "ahead", + "merge_base_commit": map[string]any{"sha": staleSHA}, + }), + ) + reg.Register( + httpmock.REST("GET", `repos/example/new$`), + httpmock.JSONResponse(map[string]any{ + "id": 3, + "owner": map[string]any{"id": 2}, + }), + ) + + workflowPath := writeTempWorkflow(t, ` +name: ci +on: push +jobs: + test: + runs-on: ubuntu-latest + steps: + - uses: example/action@v1 + - uses: example/new@v2.0.0 +`, + "example/action@v1=sha1-"+staleSHA, + ) + + stdout, _, err := runCommandWithHTTP(t, reg, "--json=findings", workflowPath) + require.NoError(t, err, "ref-moved must remain non-blocking while fixing another dependency") + assert.Contains(t, stdout, `"category": "ref-moved"`) + + lock := readTempLockfilePins(t) + assert.Contains(t, lock, "sha1-"+staleSHA) + assert.Contains(t, lock, "'example/new@v2.0.0'") +} + func TestCheckCommand_JSONDeduplicatesDependencies(t *testing.T) { // When two workflow files share the same dep, top-level dependencies // should deduplicate them. diff --git a/internal/pin/plan.go b/internal/pin/plan.go index 09b4a72a..6a166b0c 100644 --- a/internal/pin/plan.go +++ b/internal/pin/plan.go @@ -284,8 +284,7 @@ func planWorkflow(ctx context.Context, wr checks.WorkflowReport, opts PlanOption // Build entries for all pinned deps (skip any already emitted from inventory). entries = append(entries, buildPinnedEntries(opts, wr, deps, parentMap, rootTracker, inventorySHA)...) - // Record findings that are informational (ref-moved, misleading-sha). - entries = append(entries, informationalEntries(wr, opts)...) + entries = append(entries, misleadingSHAEntries(wr)...) return planResult{entries: entries, wplans: wplans}, nil } @@ -514,20 +513,10 @@ func buildPinnedEntries(opts PlanOptions, wr checks.WorkflowReport, deps []dep.D return out } -// informationalEntries records ref-moved and misleading-sha findings as -// Investigate entries. When the run re-pins moved refs (--relock or -// --accept-moved), ref-moved is resolved by the re-pin and is not recorded -// for investigation. -func informationalEntries(wr checks.WorkflowReport, opts PlanOptions) []Entry { +func misleadingSHAEntries(wr checks.WorkflowReport) []Entry { var out []Entry for _, f := range wr.Findings { - switch f.Category { - case checks.RefMoved: - if repinsMoved(opts) { - continue - } - out = append(out, informationalEntry(f, wr.Path)) - case checks.MisleadingSHA: + if f.Category == checks.MisleadingSHA { out = append(out, informationalEntry(f, wr.Path)) } } From 84cf6be4a6128dcc9d5a9a70bc21dd288b3dda5c Mon Sep 17 00:00:00 2001 From: Jeff Martin Date: Tue, 1 Sep 2026 10:03:31 -0700 Subject: [PATCH 18/20] Lockfile: reconcile scoped graph updates --- cmd/gh-actions-lock/check_json_golden_test.go | 24 ++++- cmd/gh-actions-lock/command_test.go | 93 ++++++++++++++++++- internal/pin/commit.go | 59 ++++++++++++ internal/pin/commit_test.go | 56 +++++++++++ internal/pipeline/diagnose.go | 5 +- internal/pipeline/parse.go | 45 +++++---- internal/pipeline/parse_test.go | 21 +++++ internal/pipeline/run.go | 6 ++ internal/resolve/discovery.go | 29 ++++++ 9 files changed, 312 insertions(+), 26 deletions(-) diff --git a/cmd/gh-actions-lock/check_json_golden_test.go b/cmd/gh-actions-lock/check_json_golden_test.go index 03b34ac0..cb6fc445 100644 --- a/cmd/gh-actions-lock/check_json_golden_test.go +++ b/cmd/gh-actions-lock/check_json_golden_test.go @@ -56,7 +56,7 @@ func TestCheckCommand_JSONGolden(t *testing.T) { " - uses: actions/cache@v4\n" + " - uses: helper/only-transitive@v1\n" - // Direct refs and the recorded closure are resolved in one GraphQL batch. + // Current workflow roots are resolved first. reg.Register( httpmock.GraphQLForRepo("actions", "checkout"), httpmock.JSONResponse(map[string]any{ @@ -64,9 +64,25 @@ func TestCheckCommand_JSONGolden(t *testing.T) { "a0": testRepoResponse("actions/checkout", checkoutSHA, nodeActionYAML), "a1": testRepoResponse("actions/setup-go", setupGoSHA, compositeYAML), "a2": testRepoResponse("actions/cache", cacheSHA, nodeActionYAML), - "a3": testRepoResponse("actions/cache", cacheSHA, nodeActionYAML), - "a4": testRepoResponse("old/dead", staleSHA, nodeActionYAML), - "a5": testRepoResponse("helper/only-transitive", helperSHA, nodeActionYAML), + }, + }), + ) + // Path-aware recursive discovery resolves the composite's children. + reg.Register( + httpmock.GraphQLForRepo("actions", "cache"), + httpmock.JSONResponse(map[string]any{ + "data": map[string]any{ + "a0": testRepoResponse("actions/cache", cacheSHA, nodeActionYAML), + "a1": testRepoResponse("helper/only-transitive", helperSHA, nodeActionYAML), + }, + }), + ) + // Recorded-only refs are validated without recursive discovery. + reg.Register( + httpmock.GraphQLForRepo("old", "dead"), + httpmock.JSONResponse(map[string]any{ + "data": map[string]any{ + "a0": testRepoResponse("old/dead", staleSHA, nodeActionYAML), }, }), ) diff --git a/cmd/gh-actions-lock/command_test.go b/cmd/gh-actions-lock/command_test.go index 57f8679f..9a1851c0 100644 --- a/cmd/gh-actions-lock/command_test.go +++ b/cmd/gh-actions-lock/command_test.go @@ -577,6 +577,79 @@ jobs: } } +func TestCheckCommand_RecordedSubActionClosureDoesNotResolveRepositoryRoot(t *testing.T) { + reg := &httpmock.Registry{} + defer reg.Verify(t) + + parentSHA := strings.Repeat("1", 40) + childSHA := strings.Repeat("2", 40) + compositeYAML := "name: Composite\nruns:\n using: composite\n steps:\n - uses: owner/child/save@v2\n" + rootChildYAML := "name: Wrong root\nruns:\n using: composite\n steps:\n - uses: ./helper\n" + reg.Register( + httpmock.GraphQLForRepo("owner", "composite"), + httpmock.JSONResponse(map[string]any{ + "data": map[string]any{ + "a0": testRepoResponse("owner/composite", parentSHA, compositeYAML), + "a1": testRepoResponse("owner/child", childSHA, rootChildYAML), + }, + }), + ) + reg.Register( + httpmock.GraphQLForRepo("owner", "child"), + httpmock.JSONResponse(map[string]any{ + "data": map[string]any{ + "a0": testRepoResponse("owner/child", childSHA, nodeActionYAML), + }, + }), + ) + reg.Register( + httpmock.GraphQLForRepo("owner", "child"), + httpmock.JSONResponse(map[string]any{ + "data": map[string]any{ + "a0": testRepoResponse("owner/child", childSHA, rootChildYAML), + }, + }), + ) + + workflowPath := writeTempWorkflow(t, ` +name: ci +on: push +jobs: + test: + runs-on: ubuntu-latest + steps: + - uses: owner/composite@v1 +`) + lock := "version: '" + parserlock.Version + "'\n" + + "dependencies:\n" + + " 'owner/composite@v1':\n" + + " ref: 'v1'\n" + + " commit: 'sha1-" + parentSHA + "'\n" + + " owner_id: 1\n" + + " repo_id: 1\n" + + " uses:\n" + + " - 'owner/child@v2'\n" + + " 'owner/child@v2':\n" + + " ref: 'v2'\n" + + " commit: 'sha1-" + childSHA + "'\n" + + " owner_id: 2\n" + + " repo_id: 2\n" + + "workflows:\n" + + " '.github/workflows/workflow.yml':\n" + + " - 'owner/composite@v1'\n" + lockPath := filepath.Join(".github", "workflows", "actions.lock") + require.NoError(t, os.WriteFile(lockPath, []byte(lock), 0o600)) + + _, stderr, err := runCommandWithHTTP(t, reg, workflowPath) + require.NoError(t, err) + assert.NotContains(t, stderr, "uses local path") + + got, readErr := os.ReadFile(lockPath) + require.NoError(t, readErr) + assert.Contains(t, string(got), "'owner/child@v2'") + assert.Contains(t, string(got), " - 'owner/child@v2'") +} + func TestCheckCommand_JSONDependenciesIncludesRecordedClosure(t *testing.T) { reg := &httpmock.Registry{} defer reg.Verify(t) @@ -1421,13 +1494,20 @@ func TestCheck_DefaultRun_RetainsRecordedClosureAfterPartialResolution(t *testin httpmock.JSONResponse(map[string]any{ "data": map[string]any{ "a0": testRepoResponse("example/action", parentSHA, composite), - "a1": nil, + }, + }), + ) + reg.Register( + httpmock.GraphQLForRepo("old", "child"), + httpmock.JSONResponse(map[string]any{ + "data": map[string]any{ + "a0": nil, }, "errors": []any{ map[string]any{ "type": "FORBIDDEN", "message": "Resource protected by organization SAML enforcement.", - "path": []any{"a1"}, + "path": []any{"a0"}, "extensions": map[string]any{ "saml_failure": true, }, @@ -1471,7 +1551,14 @@ func TestCheck_Relock_BumpsMovedTransitiveAlongsideNewDirect(t *testing.T) { "data": map[string]any{ "a0": testRepoResponse("example/action", parentSHA, composite), "a1": testRepoResponse("new/direct", newDirectSHA, nodeActionYAML), - "a2": testRepoResponse("old/child", liveChildSHA, nodeActionYAML), + }, + }), + ) + reg.Register( + httpmock.GraphQLForRepo("old", "child"), + httpmock.JSONResponse(map[string]any{ + "data": map[string]any{ + "a0": testRepoResponse("old/child", liveChildSHA, nodeActionYAML), }, }), ) diff --git a/internal/pin/commit.go b/internal/pin/commit.go index 8086e46b..3050de99 100644 --- a/internal/pin/commit.go +++ b/internal/pin/commit.go @@ -6,6 +6,7 @@ import ( "fmt" "os" "runtime" + "slices" "strings" "github.com/github/gh-actions-lock/internal/dep" @@ -25,6 +26,12 @@ type CommitOptions struct { // fails, previously written files are not rolled back (best-effort), // but the error is returned immediately. func Commit(ctx context.Context, rec *Record, store *lockfile.State, copts *CommitOptions) error { + reconciled, err := reconcileSharedPins(rec, store) + if err != nil { + return err + } + rec = reconciled + progress := func(string) {} if copts != nil && copts.OnProgress != nil { progress = copts.OnProgress @@ -90,6 +97,58 @@ func Commit(ctx context.Context, rec *Record, store *lockfile.State, copts *Comm return nil } +// reconcileSharedPins retains recorded authority when scoped workflow plans +// disagree about the commit or closure of a global lockfile dependency. +func reconcileSharedPins(rec *Record, store *lockfile.State) (*Record, error) { + shasByKey := make(map[string]map[string]bool) + for _, e := range rec.Entries { + if !isPinOrVerified(e.Resolution) { + continue + } + key := strings.ToLower(e.NWO + "@" + e.Ref) + if shasByKey[key] == nil { + shasByKey[key] = make(map[string]bool) + } + shasByKey[key][strings.ToLower(e.SHA)] = true + } + + conflicts := make(map[string]dep.Dependency) + for key, shas := range shasByKey { + if len(shas) < 2 { + continue + } + for _, existing := range store.AllDeps() { + if strings.EqualFold(existing.Key(), key) { + conflicts[key] = existing + break + } + } + if _, ok := conflicts[key]; !ok { + return nil, fmt.Errorf("conflicting planned commits for %s", key) + } + } + if len(conflicts) == 0 { + return rec, nil + } + + reconciled := *rec + reconciled.Entries = slices.Clone(rec.Entries) + for i := range reconciled.Entries { + e := &reconciled.Entries[i] + key := strings.ToLower(e.NWO + "@" + e.Ref) + if existing, ok := conflicts[key]; ok && isPinOrVerified(e.Resolution) { + e.SHA = existing.SHA + e.OnBranch = existing.Branch + e.Tag = existing.Tag + } + e.RequiredBy = slices.DeleteFunc(slices.Clone(e.RequiredBy), func(parent string) bool { + _, conflicted := conflicts[strings.ToLower(parent)] + return conflicted + }) + } + return &reconciled, nil +} + func rewriteWorkflow(wp WorkflowPlan) error { wf, err := workflowfile.Load(wp.Path) if err != nil { diff --git a/internal/pin/commit_test.go b/internal/pin/commit_test.go index 3e23af36..e24e02c6 100644 --- a/internal/pin/commit_test.go +++ b/internal/pin/commit_test.go @@ -190,3 +190,59 @@ func TestCommitScopedParentAdvanceKeepsUntouchedWorkflowEdges(t *testing.T) { assert.Equal(t, []string{"owner/ci-child@v2", "owner/release-child@v1"}, action.Uses) assert.Contains(t, file.Dependencies, "owner/release-child@v1") } + +func TestCommitConflictingScopedPlansKeepRecordedSharedClosure(t *testing.T) { + for _, workflows := range [][]string{ + {".github/workflows/ci.yml", ".github/workflows/release.yml"}, + {".github/workflows/release.yml", ".github/workflows/ci.yml"}, + } { + t.Run(strings.Join(workflows, "_then_"), func(t *testing.T) { + dir := t.TempDir() + require.NoError(t, os.MkdirAll(filepath.Join(dir, ".github", "workflows"), 0o755)) + for _, path := range workflows { + require.NoError(t, os.WriteFile(filepath.Join(dir, path), []byte("on: push\njobs: {}\n"), 0o644)) + } + t.Chdir(dir) + + ctx := context.Background() + store, err := lockfile.LoadState(dir, fakeMeta{}) + require.NoError(t, err) + oldParent := dep.Dependency{NWO: "owner/composite", Ref: "main", SHA: strings.Repeat("1", 40), HashAlgo: "sha1"} + oldChild := dep.Dependency{NWO: "owner/old-child", Ref: "v1", SHA: strings.Repeat("2", 40), HashAlgo: "sha1"} + for _, path := range workflows { + require.NoError(t, store.Set(ctx, path, + []dep.Dependency{oldParent, oldChild}, + dep.ParentMap{oldChild.Key(): {oldParent.Key()}}, + map[string]bool{oldParent.Key(): true})) + } + require.NoError(t, store.Save()) + + newParentSHA := strings.Repeat("3", 40) + newChild := dep.Dependency{NWO: "owner/new-child", Ref: "v2", SHA: strings.Repeat("4", 40), HashAlgo: "sha1"} + entriesByWorkflow := map[string][]Entry{ + ".github/workflows/ci.yml": { + {NWO: oldParent.NWO, Ref: oldParent.Ref, SHA: newParentSHA, Resolution: Pinned, Direct: true, Workflows: []string{".github/workflows/ci.yml"}}, + {NWO: newChild.NWO, Ref: newChild.Ref, SHA: newChild.SHA, Resolution: Pinned, RequiredBy: []string{oldParent.Key()}, Workflows: []string{".github/workflows/ci.yml"}}, + }, + ".github/workflows/release.yml": { + {NWO: oldParent.NWO, Ref: oldParent.Ref, SHA: oldParent.SHA, Resolution: Verified, Direct: true, Workflows: []string{".github/workflows/release.yml"}}, + {NWO: oldChild.NWO, Ref: oldChild.Ref, SHA: oldChild.SHA, Resolution: Verified, RequiredBy: []string{oldParent.Key()}, Workflows: []string{".github/workflows/release.yml"}}, + }, + } + rec := &Record{} + for _, path := range workflows { + rec.Entries = append(rec.Entries, entriesByWorkflow[path]...) + rec.Workflows = append(rec.Workflows, WorkflowPlan{Path: path}) + } + + require.NoError(t, Commit(ctx, rec, store, nil)) + + file := store.File() + action := file.Dependencies[oldParent.Key()] + assert.Equal(t, "sha1-"+oldParent.SHA, action.Commit) + assert.Equal(t, []string{oldChild.Key()}, action.Uses) + assert.Contains(t, file.Dependencies, oldChild.Key()) + assert.NotContains(t, file.Dependencies, newChild.Key()) + }) + } +} diff --git a/internal/pipeline/diagnose.go b/internal/pipeline/diagnose.go index c936c751..7ba23201 100644 --- a/internal/pipeline/diagnose.go +++ b/internal/pipeline/diagnose.go @@ -63,7 +63,10 @@ func diagnoseOneParsed(ctx context.Context, pw checks.ParsedWorkflow, r *resolve var liveDeps []dep.Dependency if r != nil { var resolveErr error - liveDeps, _, resolveErr = r.ResolveAllRecursive(ctx, resolvableRefs(pw)) + liveDeps, _, resolveErr = r.ResolveAllRecursive(ctx, pw.Refs) + recordedLive, recordedErr := r.ResolveAllShallow(ctx, collectRecordedResolvable([]checks.ParsedWorkflow{pw})) + liveDeps = dep.Dedup(append(liveDeps, recordedLive...)) + resolveErr = errors.Join(resolveErr, recordedErr) if resolveErr != nil { blockingResolverError := false if resolve.IsCompositeLocalPath(resolveErr) { diff --git a/internal/pipeline/parse.go b/internal/pipeline/parse.go index 78134b82..c0de9b8f 100644 --- a/internal/pipeline/parse.go +++ b/internal/pipeline/parse.go @@ -107,13 +107,14 @@ func mergeStrings(groups ...[]string) []string { return values } -// CollectResolvable returns the deduplicated union of current workflow refs -// and recorded closure refs across all parsed workflows. +// CollectResolvable returns the deduplicated current workflow roots across all +// parsed workflows. Recorded closure entries lack sub-action paths and must not +// become recursive discovery roots. func CollectResolvable(parsed []checks.ParsedWorkflow) []parserlock.ActionRef { seenRef := make(map[ghapi.ActionRef]bool) var refs []parserlock.ActionRef for _, pw := range parsed { - for _, ref := range resolvableRefs(pw) { + for _, ref := range pw.Refs { key := ghapi.ForActionRef(ref.Owner, ref.Repo, ref.Path, ref.Ref) if seenRef[key] { continue @@ -125,23 +126,31 @@ func CollectResolvable(parsed []checks.ParsedWorkflow) []parserlock.ActionRef { return refs } -func resolvableRefs(pw checks.ParsedWorkflow) []parserlock.ActionRef { - refs := append([]parserlock.ActionRef(nil), pw.Refs...) - current := make(map[ghapi.NWORef]bool, len(pw.Refs)) - for _, ref := range pw.Refs { - current[ghapi.ForNWORef(ref.Owner, ref.Repo, ref.Ref)] = true +// collectRecordedResolvable returns recorded closure refs that are not already +// represented by a path-aware current workflow root. +func collectRecordedResolvable(parsed []checks.ParsedWorkflow) []parserlock.ActionRef { + seenRef := make(map[ghapi.NWORef]bool) + for _, pw := range parsed { + for _, ref := range pw.Refs { + seenRef[ghapi.ForNWORef(ref.Owner, ref.Repo, ref.Ref)] = true + } } - for _, d := range pw.RecordedDeps { - owner, repo := d.OwnerRepo() - if current[ghapi.ForNWORef(owner, repo, d.Ref)] { - continue + + var refs []parserlock.ActionRef + for _, pw := range parsed { + for _, d := range pw.RecordedDeps { + owner, repo := d.OwnerRepo() + key := ghapi.ForNWORef(owner, repo, d.Ref) + if seenRef[key] { + continue + } + seenRef[key] = true + refs = append(refs, parserlock.ActionRef{ + Owner: owner, + Repo: repo, + Ref: d.Ref, + }) } - refs = append(refs, parserlock.ActionRef{ - Owner: owner, - Repo: repo, - Path: d.Path, - Ref: d.Ref, - }) } return refs } diff --git a/internal/pipeline/parse_test.go b/internal/pipeline/parse_test.go index d95c4180..9ff2390f 100644 --- a/internal/pipeline/parse_test.go +++ b/internal/pipeline/parse_test.go @@ -5,6 +5,9 @@ import ( "path/filepath" "testing" + parserlock "github.com/github/actions-lockfile/go/pkg/lockfile" + "github.com/github/gh-actions-lock/internal/dep" + "github.com/github/gh-actions-lock/internal/pipeline/checks" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -46,3 +49,21 @@ jobs: assert.ElementsMatch(t, []string{"$/actions/root", "$/.github/workflows/reusable.yml"}, pw.SelfRepositoryRefs) assert.Empty(t, pw.SelfRepositoryResolutionErrs) } + +func TestCollectResolvableExcludesPathlessRecordedClosure(t *testing.T) { + root := parserlock.ActionRef{Owner: "owner", Repo: "composite", Path: "sub-action", Ref: "v1"} + parsed := []checks.ParsedWorkflow{{ + Refs: []parserlock.ActionRef{root}, + RecordedDeps: []dep.Dependency{ + {NWO: "owner/composite", Ref: "v1", SHA: "111"}, + {NWO: "owner/child", Ref: "v2", SHA: "222"}, + }, + }} + + assert.Equal(t, []parserlock.ActionRef{root}, CollectResolvable(parsed)) + assert.Equal(t, []parserlock.ActionRef{{ + Owner: "owner", + Repo: "child", + Ref: "v2", + }}, collectRecordedResolvable(parsed)) +} diff --git a/internal/pipeline/run.go b/internal/pipeline/run.go index 2855b065..672502dd 100644 --- a/internal/pipeline/run.go +++ b/internal/pipeline/run.go @@ -61,6 +61,7 @@ func Run(ctx context.Context, opts RunOptions) (*RunResult, error) { } } refs := CollectResolvable(unresolved) + recordedRefs := collectRecordedResolvable(unresolved) // Phase 2: Resolve. if r == nil { @@ -77,6 +78,11 @@ func Run(ctx context.Context, opts RunOptions) (*RunResult, error) { _, _, _ = r.ResolveAllRecursive(ctx, refs) endResolve() } + if len(recordedRefs) > 0 { + endResolve := prof.Phase(" resolve recorded refs") + _, _ = r.ResolveAllShallow(ctx, recordedRefs) + endResolve() + } if ctx.Err() != nil { return nil, ctx.Err() diff --git a/internal/resolve/discovery.go b/internal/resolve/discovery.go index c991d384..c5a57d24 100644 --- a/internal/resolve/discovery.go +++ b/internal/resolve/discovery.go @@ -100,6 +100,35 @@ func cacheKey(ref parserlock.ActionRef) ghapi.ActionRef { // fixed request shape. const batchActionFileSize = 20 +// ResolveAllShallow resolves one wave of action refs without recursively +// inspecting their action metadata for transitive dependencies. +func (r *Resolver) ResolveAllShallow(ctx context.Context, refs []parserlock.ActionRef) ([]dep.Dependency, error) { + seen := make(map[ghapi.ActionRef]bool) + requests := make([]resolutionRequest, 0, len(refs)) + uncached := 0 + for _, ref := range refs { + key := cacheKey(ref) + if seen[key] { + continue + } + seen[key] = true + requests = append(requests, resolutionRequest{ref: ref}) + if _, ok := r.cache.Get(key); !ok { + uncached++ + } + } + + var resolveDone atomic.Int64 + var resolveTotal atomic.Int64 + resolveTotal.Store(int64(uncached)) + if uncached > 0 { + r.FireResolveProgress(0, uncached) + } + + deps, _, err := r.resolveWithActionYMLParallel(ctx, requests, 0, &resolveDone, &resolveTotal) + return dep.Dedup(deps), err +} + // ResolveAllRecursive resolves action refs and recursively discovers transitive // dependencies from composite actions by reading their action.yml via GraphQL. // The returned ParentMap (child dep key → parent dep keys) is owned by the From acc41e1fc73cf369760dcf0d7d1177d114837f70 Mon Sep 17 00:00:00 2001 From: Jeff Martin Date: Tue, 1 Sep 2026 10:03:44 -0700 Subject: [PATCH 19/20] CI: retry CodeQL analysis From bd9975474d321f68cb55f3727e4387626a77da6b Mon Sep 17 00:00:00 2001 From: Jeff Martin Date: Tue, 1 Sep 2026 10:42:55 -0700 Subject: [PATCH 20/20] Lockfile: batch authoritative graph updates --- cmd/gh-actions-lock/command_test.go | 79 +++++++++++++ internal/dep/dependency.go | 10 ++ internal/lockfile/state.go | 170 ++++++++++++++++------------ internal/lockfile/state_test.go | 113 +++++++++++++++++- internal/pin/commit.go | 32 +++--- internal/pin/commit_test.go | 34 +++++- internal/pin/plan.go | 50 +++++--- internal/pin/record.go | 5 +- internal/pipeline/checks/finding.go | 9 ++ internal/pipeline/diagnose.go | 62 +++++++++- test/integration/run.rb | 41 +++++-- test/scenarios/catalog.yml | 38 +++++++ 12 files changed, 519 insertions(+), 124 deletions(-) diff --git a/cmd/gh-actions-lock/command_test.go b/cmd/gh-actions-lock/command_test.go index 9a1851c0..9853a1a8 100644 --- a/cmd/gh-actions-lock/command_test.go +++ b/cmd/gh-actions-lock/command_test.go @@ -650,6 +650,85 @@ jobs: assert.Contains(t, string(got), " - 'owner/child@v2'") } +func TestCheckCommand_PathSwitchReplacesSameCommitClosure(t *testing.T) { + reg := &httpmock.Registry{} + defer reg.Verify(t) + + parentSHA := strings.Repeat("1", 40) + oldChildSHA := strings.Repeat("2", 40) + newChildSHA := strings.Repeat("3", 40) + compositeYAML := "name: New sub-action\nruns:\n using: composite\n steps:\n - uses: new/child@v2\n" + reg.Register( + httpmock.GraphQLForRepo("owner", "composite"), + httpmock.JSONResponse(map[string]any{ + "data": map[string]any{ + "a0": testRepoResponse("owner/composite", parentSHA, compositeYAML), + }, + }), + ) + reg.Register( + httpmock.GraphQLForRepo("new", "child"), + httpmock.JSONResponse(map[string]any{ + "data": map[string]any{ + "a0": testRepoResponse("new/child", newChildSHA, nodeActionYAML), + }, + }), + ) + reg.Register( + httpmock.GraphQLForRepo("old", "child"), + httpmock.JSONResponse(map[string]any{ + "data": map[string]any{ + "a0": testRepoResponse("old/child", oldChildSHA, nodeActionYAML), + }, + }), + ) + reg.Register( + httpmock.REST(http.MethodGet, `repos/new/child$`), + httpmock.JSONResponse(map[string]any{ + "id": 3, + "owner": map[string]any{"id": 2}, + }), + ) + + workflowPath := writeTempWorkflow(t, ` +name: ci +on: push +jobs: + test: + runs-on: ubuntu-latest + steps: + - uses: owner/composite/new@v1 +`) + lock := "version: '" + parserlock.Version + "'\n" + + "dependencies:\n" + + " 'owner/composite@v1':\n" + + " ref: 'v1'\n" + + " commit: 'sha1-" + parentSHA + "'\n" + + " owner_id: 1\n" + + " repo_id: 1\n" + + " uses:\n" + + " - 'old/child@v1'\n" + + " 'old/child@v1':\n" + + " ref: 'v1'\n" + + " commit: 'sha1-" + oldChildSHA + "'\n" + + " owner_id: 2\n" + + " repo_id: 2\n" + + "workflows:\n" + + " '.github/workflows/workflow.yml':\n" + + " - 'owner/composite@v1'\n" + lockPath := filepath.Join(".github", "workflows", "actions.lock") + require.NoError(t, os.WriteFile(lockPath, []byte(lock), 0o600)) + + _, _, err := runCommandWithHTTP(t, reg, "--no-narrow", workflowPath) + require.NoError(t, err) + + got, readErr := os.ReadFile(lockPath) + require.NoError(t, readErr) + assert.Contains(t, string(got), "'new/child@v2'") + assert.Contains(t, string(got), " - 'new/child@v2'") + assert.NotContains(t, string(got), "'old/child@v1'") +} + func TestCheckCommand_JSONDependenciesIncludesRecordedClosure(t *testing.T) { reg := &httpmock.Registry{} defer reg.Verify(t) diff --git a/internal/dep/dependency.go b/internal/dep/dependency.go index 4327a929..53e263c9 100644 --- a/internal/dep/dependency.go +++ b/internal/dep/dependency.go @@ -49,6 +49,16 @@ func (d Dependency) Key() string { return d.NWO + "@" + d.Ref } +// NormalizeKey lowercases the repository portion of an NWO@ref key while +// preserving the case-sensitive git ref. +func NormalizeKey(key string) string { + at := strings.LastIndex(key, "@") + if at < 0 { + return strings.ToLower(key) + } + return strings.ToLower(key[:at]) + key[at:] +} + // OwnerRepo splits NWO into owner and repo components. func (d Dependency) OwnerRepo() (string, string) { owner, repo, _ := parserlock.SplitNWO(d.NWO) diff --git a/internal/lockfile/state.go b/internal/lockfile/state.go index 9f6caf92..fc8f020c 100644 --- a/internal/lockfile/state.go +++ b/internal/lockfile/state.go @@ -5,6 +5,7 @@ import ( "context" "errors" "fmt" + "maps" "os" "path/filepath" "sort" @@ -351,104 +352,120 @@ func (s *State) AllDeps() []dep.Dependency { // Resolution of owner/repo numeric IDs happens lazily per NWO and is cached // for the lifetime of the store. func (s *State) Set(ctx context.Context, workflowKey string, deps []dep.Dependency, parentMap map[string][]string, directKeys map[string]bool) error { - return s.set(ctx, workflowKey, deps, parentMap, directKeys, nil) + return s.SetWorkflows(ctx, []WorkflowUpdate{{ + WorkflowKey: workflowKey, + Deps: deps, + ParentMap: parentMap, + DirectKeys: directKeys, + ReplaceGraph: true, + }}) } -// SetScoped updates a workflow while preserving existing graph edges on -// dependencies also reached by workflows outside the current command scope. -func (s *State) SetScoped(ctx context.Context, workflowKey string, deps []dep.Dependency, parentMap map[string][]string, directKeys, scopedWorkflows map[string]bool) error { - return s.set(ctx, workflowKey, deps, parentMap, directKeys, scopedWorkflows) +// WorkflowUpdate is one workflow's contribution to a batch lockfile write. +type WorkflowUpdate struct { + WorkflowKey string + Deps []dep.Dependency + ParentMap dep.ParentMap + DirectKeys map[string]bool + ReplaceGraph bool } -func (s *State) set(ctx context.Context, workflowKey string, deps []dep.Dependency, parentMap map[string][]string, directKeys, scopedWorkflows map[string]bool) error { +// SetWorkflows applies workflow roots and global dependency metadata together. +// Complete live graphs replace recorded edges; incomplete or unscanned +// workflows retain the recorded edge union they still reach. +func (s *State) SetWorkflows(ctx context.Context, updates []WorkflowUpdate) error { // Resolve repo IDs for every unique owner/repo BEFORE taking s.mu so // concurrent pin workers don't serialize on the network round-trip. // lookupIDs is safe to call without s.mu and dedups in-flight fetches // for the same key via singleflight. - seenRepos := make(map[string]struct{}, len(deps)) - for _, d := range deps { - pin, err := depToPin(d) - if err != nil { - return err - } - k := pin.Owner + "/" + pin.Repo - if _, ok := seenRepos[k]; ok { - continue - } - seenRepos[k] = struct{}{} - if _, err := s.lookupIDs(ctx, pin.Owner, pin.Repo); err != nil { - return fmt.Errorf("resolving repo IDs for %s/%s: %w", pin.Owner, pin.Repo, err) + seenRepos := make(map[string]struct{}) + for _, update := range updates { + for _, d := range update.Deps { + pin, err := depToPin(d) + if err != nil { + return err + } + k := pin.Owner + "/" + pin.Repo + if _, ok := seenRepos[k]; ok { + continue + } + seenRepos[k] = struct{}{} + if _, err := s.lookupIDs(ctx, pin.Owner, pin.Repo); err != nil { + return fmt.Errorf("resolving repo IDs for %s/%s: %w", pin.Owner, pin.Repo, err) + } } } s.mu.Lock() defer s.mu.Unlock() - directPins := make([]string, 0) - seenDirect := map[string]bool{} - // keyToPin: Dependency.Key() (NWO@Ref) → canonical pin (NWO@Ref:algo-hex). - keyToPin := make(map[string]string, len(deps)) - for _, d := range deps { - pin, err := depToPin(d) - if err != nil { - return err - } - pin = pin.Canonical() - pinKey := pin.String() - keyToPin[d.Key()] = pinKey - var isDirect bool - if directKeys != nil { - isDirect = directKeys[d.Key()] - } else { - _, hasParent := parentMap[d.Key()] - isDirect = !hasParent - } - if isDirect && !seenDirect[pinKey] { - seenDirect[pinKey] = true - directPins = append(directPins, pinKey) - } - } - - // Invert parentMap (child → parents) into parent → children, in canonical - // pin-key form. Translate both sides via keyToPin; entries that don't - // resolve to a known dep are skipped (they were filtered out before - // reaching the writer). + recordedDependencies := maps.Clone(s.file.Dependencies) + recordedWorkflows := maps.Clone(s.file.Workflows) + replacingWorkflows := make(map[string]bool, len(updates)) + workflowPins := make(map[string][]string, len(updates)) + depsByPin := make(map[string]dep.Dependency) + replacePins := make(map[string]bool) parentToChildren := make(map[string]map[string]bool) - for childDepKey, parents := range parentMap { - childPin, ok := keyToPin[childDepKey] - if !ok { + for _, update := range updates { + replacingWorkflows[update.WorkflowKey] = update.ReplaceGraph + keyToPin := make(map[string]string, len(update.Deps)) + seenDirect := make(map[string]bool) + for _, d := range update.Deps { + pin, err := depToPin(d) + if err != nil { + return err + } + pinKey := pin.Canonical().String() + keyToPin[d.Key()] = pinKey + if existing, ok := depsByPin[pinKey]; ok && !strings.EqualFold(existing.SHA, d.SHA) { + return fmt.Errorf("conflicting commits for %s", pinKey) + } + depsByPin[pinKey] = d + if update.ReplaceGraph { + replacePins[pinKey] = true + } + isDirect := update.DirectKeys[d.Key()] + if update.DirectKeys == nil { + _, hasParent := update.ParentMap[d.Key()] + isDirect = !hasParent + } + if isDirect && !seenDirect[pinKey] { + seenDirect[pinKey] = true + workflowPins[update.WorkflowKey] = append(workflowPins[update.WorkflowKey], pinKey) + } + } + if !update.ReplaceGraph { continue } - for _, parentDepKey := range parents { - parentPin, ok := keyToPin[parentDepKey] + for childDepKey, parents := range update.ParentMap { + childPin, ok := keyToPin[childDepKey] if !ok { continue } - children, exists := parentToChildren[parentPin] - if !exists { - children = make(map[string]bool) - parentToChildren[parentPin] = children + for _, parentDepKey := range parents { + parentPin, ok := keyToPin[parentDepKey] + if !ok { + continue + } + if parentToChildren[parentPin] == nil { + parentToChildren[parentPin] = make(map[string]bool) + } + parentToChildren[parentPin][childPin] = true } - children[childPin] = true } + sort.Strings(workflowPins[update.WorkflowKey]) } - // Now upsert action entries with their per-pin uses lists. - for _, d := range deps { + for pinKey, d := range depsByPin { pin, err := depToPin(d) if err != nil { return err } - pin = pin.Canonical() - pinKey := pin.String() // IDs were pre-resolved above (outside the mutex); read from cache // directly so we don't recursively re-acquire s.mu. ids, ok := s.idCache[strings.ToLower(pin.Owner+"/"+pin.Repo)] if !ok { return fmt.Errorf("resolving repo IDs for %s/%s: not in cache after pre-resolve", pin.Owner, pin.Repo) } - // Merge uses: each workflow contributes its own transitive edges. - // A dep that is a parent in one workflow but direct (no children) - // in another must not clobber the first workflow's uses list. usesSet := make(map[string]bool) if children, ok := parentToChildren[pinKey]; ok { for c := range children { @@ -473,10 +490,7 @@ func (s *State) set(ctx context.Context, workflowKey string, deps []dep.Dependen if ref == "" { ref = existing.Ref } - if existing.Commit == commit || s.reachableFromUnscopedWorkflow(pinKey, workflowKey, scopedWorkflows) { - // ponytail: actions.lock stores a global edge union, so keep the - // old union when an untouched workflow reaches this parent. - // A full-scope refresh can replace it exactly. + if !replacePins[pinKey] || reachableFromPreservedWorkflow(pinKey, replacingWorkflows, recordedWorkflows, recordedDependencies) { for _, u := range existing.Uses { usesSet[u] = true } @@ -498,12 +512,18 @@ func (s *State) set(ctx context.Context, workflowKey string, deps []dep.Dependen Uses: uses, } } - sort.Strings(directPins) - s.file.Workflows[workflowKey] = directPins + for workflowKey, directPins := range workflowPins { + s.file.Workflows[workflowKey] = directPins + } + for _, update := range updates { + if _, ok := workflowPins[update.WorkflowKey]; !ok { + s.file.Workflows[update.WorkflowKey] = nil + } + } return nil } -func (s *State) reachableFromUnscopedWorkflow(target, workflowKey string, scopedWorkflows map[string]bool) bool { +func reachableFromPreservedWorkflow(target string, replacingWorkflows map[string]bool, workflows map[string][]string, dependencies map[string]parserlock.Action) bool { var reaches func(string, map[string]bool) bool reaches = func(key string, seen map[string]bool) bool { if key == target { @@ -513,15 +533,15 @@ func (s *State) reachableFromUnscopedWorkflow(target, workflowKey string, scoped return false } seen[key] = true - for _, child := range s.file.Dependencies[key].Uses { + for _, child := range dependencies[key].Uses { if reaches(child, seen) { return true } } return false } - for workflow, roots := range s.file.Workflows { - if workflow == workflowKey || scopedWorkflows[workflow] { + for workflow, roots := range workflows { + if replacingWorkflows[workflow] { continue } for _, root := range roots { diff --git a/internal/lockfile/state_test.go b/internal/lockfile/state_test.go index fe374c3b..7d4b0de2 100644 --- a/internal/lockfile/state_test.go +++ b/internal/lockfile/state_test.go @@ -5,6 +5,7 @@ import ( "errors" "os" "path/filepath" + "slices" "strings" "testing" @@ -518,8 +519,8 @@ func TestState_GetClosureTraversesRecordedGraph(t *testing.T) { } } -func TestState_SetPreservesOrReplacesRecordedClosureByParentCommit(t *testing.T) { - t.Run("unchanged parent keeps recorded children", func(t *testing.T) { +func TestState_SetPreservesOrReplacesRecordedClosureByAuthority(t *testing.T) { + t.Run("incomplete graph keeps recorded children", func(t *testing.T) { dir := t.TempDir() setupClosure(t, dir) parent := dep.Dependency{ @@ -529,16 +530,59 @@ func TestState_SetPreservesOrReplacesRecordedClosureByParentCommit(t *testing.T) SHA: "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", HashAlgo: "sha1", } + store, err := LoadState(dir, fakeMetadataResolver{}) + if err != nil { + t.Fatal(err) + } + if err := store.SetWorkflows(context.Background(), []WorkflowUpdate{{ + WorkflowKey: ".github/workflows/ci.yml", + Deps: []dep.Dependency{parent}, + DirectKeys: map[string]bool{parent.Key(): true}, + }}); err != nil { + t.Fatal(err) + } + if err := store.Save(); err != nil { + t.Fatal(err) + } + after, err := os.ReadFile(filepath.Join(dir, parserlock.Path)) + if err != nil { + t.Fatal(err) + } + if !strings.Contains(string(after), "actions/cache@v4") { + t.Fatalf("recorded child must survive an incomplete resolution:\n%s", after) + } + }) + + t.Run("complete unchanged parent replaces recorded children", func(t *testing.T) { + dir := t.TempDir() + setupClosure(t, dir) + parent := dep.Dependency{ + NWO: "actions/setup-go", + Ref: "v6", + Branch: "main", + SHA: "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", + HashAlgo: "sha1", + } + child := dep.Dependency{ + NWO: "new/child", + Ref: "v2", + Branch: "main", + SHA: "2222222222222222222222222222222222222222", + HashAlgo: "sha1", + } after := resaveBumped( t, dir, ".github/workflows/ci.yml", - []dep.Dependency{parent}, - nil, + []dep.Dependency{parent, child}, + map[string][]string{child.Key(): {parent.Key()}}, map[string]bool{parent.Key(): true}, ) - if !strings.Contains(string(after), "actions/cache@v4") { - t.Fatalf("recorded child must survive an incomplete resolution:\n%s", after) + if strings.Contains(string(after), "actions/cache@v4") { + t.Fatalf("complete graph must drop its obsolete child:\n%s", after) + } + if !strings.Contains(string(after), "new/child@v2") { + t.Fatalf("complete graph must record its live child:\n%s", after) } }) @@ -576,6 +620,63 @@ func TestState_SetPreservesOrReplacesRecordedClosureByParentCommit(t *testing.T) }) } +func TestState_SetWorkflowsRebuildsCompleteScopedUnion(t *testing.T) { + 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.Fatal(err) + } + parent := dep.Dependency{NWO: "owner/composite", Ref: "v1", SHA: strings.Repeat("1", 40), HashAlgo: "sha1"} + oldChild := dep.Dependency{NWO: "owner/old", Ref: "v1", SHA: strings.Repeat("2", 40), HashAlgo: "sha1"} + for _, workflow := range []string{"ci.yml", "release.yml"} { + if err := store.Set(context.Background(), workflow, + []dep.Dependency{parent, oldChild}, + dep.ParentMap{oldChild.Key(): {parent.Key()}}, + map[string]bool{parent.Key(): true}); err != nil { + t.Fatal(err) + } + } + if err := store.Save(); err != nil { + t.Fatal(err) + } + + ciChild := dep.Dependency{NWO: "owner/ci-child", Ref: "v1", SHA: strings.Repeat("3", 40), HashAlgo: "sha1"} + releaseChild := dep.Dependency{NWO: "owner/release-child", Ref: "v1", SHA: strings.Repeat("4", 40), HashAlgo: "sha1"} + if err := store.SetWorkflows(context.Background(), []WorkflowUpdate{ + { + WorkflowKey: "ci.yml", + Deps: []dep.Dependency{parent, ciChild}, + ParentMap: dep.ParentMap{ciChild.Key(): {parent.Key()}}, + DirectKeys: map[string]bool{parent.Key(): true}, + ReplaceGraph: true, + }, + { + WorkflowKey: "release.yml", + Deps: []dep.Dependency{parent, releaseChild}, + ParentMap: dep.ParentMap{releaseChild.Key(): {parent.Key()}}, + DirectKeys: map[string]bool{parent.Key(): true}, + ReplaceGraph: true, + }, + }); err != nil { + t.Fatal(err) + } + if err := store.Save(); err != nil { + t.Fatal(err) + } + + action := store.File().Dependencies[parent.Key()] + wantUses := []string{ciChild.Key(), releaseChild.Key()} + if !slices.Equal(action.Uses, wantUses) { + t.Fatalf("complete scoped union = %v, want %v", action.Uses, wantUses) + } + if _, ok := store.File().Dependencies[oldChild.Key()]; ok { + t.Fatalf("obsolete child %s survived authoritative rebuild", oldChild.Key()) + } +} + // 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 { diff --git a/internal/pin/commit.go b/internal/pin/commit.go index 3050de99..8c1ab548 100644 --- a/internal/pin/commit.go +++ b/internal/pin/commit.go @@ -67,13 +67,10 @@ func Commit(ctx context.Context, rec *Record, store *lockfile.State, copts *Comm // Phase 2: Update lockfile entries for each scanned workflow. pinnedByWorkflow := groupPinnedByWorkflow(rec) - scopedWorkflows := make(map[string]bool, len(rec.Workflows)) - for _, wp := range rec.Workflows { - scopedWorkflows[workflowfile.KeyFromPath(wp.Path)] = true - } if len(rec.Workflows) > 0 { progress("Updating lockfile") } + updates := make([]lockfile.WorkflowUpdate, 0, len(rec.Workflows)) for _, wp := range rec.Workflows { wfPath := wp.Path wfKey := workflowfile.KeyFromPath(wfPath) @@ -84,9 +81,16 @@ func Commit(ctx context.Context, rec *Record, store *lockfile.State, copts *Comm parentMap := buildParentMap(rec, wfPath) directKeys := buildDirectKeys(rec, wfPath) deps = retainUnresolvablePins(rec, store, wfPath, deps, directKeys) - if err := store.SetScoped(ctx, wfKey, deps, parentMap, directKeys, scopedWorkflows); err != nil { - return fmt.Errorf("updating lockfile for %s: %w", wfPath, err) - } + updates = append(updates, lockfile.WorkflowUpdate{ + WorkflowKey: wfKey, + Deps: deps, + ParentMap: parentMap, + DirectKeys: directKeys, + ReplaceGraph: wp.ReplaceGraph, + }) + } + if err := store.SetWorkflows(ctx, updates); err != nil { + return fmt.Errorf("updating lockfile: %w", err) } // Phase 3: Persist lockfile to disk. @@ -105,7 +109,7 @@ func reconcileSharedPins(rec *Record, store *lockfile.State) (*Record, error) { if !isPinOrVerified(e.Resolution) { continue } - key := strings.ToLower(e.NWO + "@" + e.Ref) + key := dep.NormalizeKey(e.NWO + "@" + e.Ref) if shasByKey[key] == nil { shasByKey[key] = make(map[string]bool) } @@ -118,7 +122,7 @@ func reconcileSharedPins(rec *Record, store *lockfile.State) (*Record, error) { continue } for _, existing := range store.AllDeps() { - if strings.EqualFold(existing.Key(), key) { + if dep.NormalizeKey(existing.Key()) == key { conflicts[key] = existing break } @@ -135,14 +139,14 @@ func reconcileSharedPins(rec *Record, store *lockfile.State) (*Record, error) { reconciled.Entries = slices.Clone(rec.Entries) for i := range reconciled.Entries { e := &reconciled.Entries[i] - key := strings.ToLower(e.NWO + "@" + e.Ref) + key := dep.NormalizeKey(e.NWO + "@" + e.Ref) if existing, ok := conflicts[key]; ok && isPinOrVerified(e.Resolution) { e.SHA = existing.SHA e.OnBranch = existing.Branch e.Tag = existing.Tag } e.RequiredBy = slices.DeleteFunc(slices.Clone(e.RequiredBy), func(parent string) bool { - _, conflicted := conflicts[strings.ToLower(parent)] + _, conflicted := conflicts[dep.NormalizeKey(parent)] return conflicted }) } @@ -242,7 +246,7 @@ func retainUnresolvablePins(rec *Record, store *lockfile.State, wfPath string, d } for _, wf := range e.Workflows { if wf == wfPath { - retain[strings.ToLower(e.NWO+"@"+e.Ref)] = true + retain[dep.NormalizeKey(e.NWO+"@"+e.Ref)] = true } } } @@ -255,10 +259,10 @@ func retainUnresolvablePins(rec *Record, store *lockfile.State, wfPath string, d } have := make(map[string]bool, len(deps)) for _, d := range deps { - have[strings.ToLower(d.NWO+"@"+d.Ref)] = true + have[dep.NormalizeKey(d.NWO+"@"+d.Ref)] = true } for _, d := range existing { - k := strings.ToLower(d.NWO + "@" + d.Ref) + k := dep.NormalizeKey(d.NWO + "@" + d.Ref) if retain[k] && !have[k] { deps = append(deps, d) directKeys[d.Key()] = true diff --git a/internal/pin/commit_test.go b/internal/pin/commit_test.go index e24e02c6..63a03edd 100644 --- a/internal/pin/commit_test.go +++ b/internal/pin/commit_test.go @@ -140,7 +140,7 @@ jobs: Direct: true, Workflows: []string{workflowPath}, }}, - Workflows: []WorkflowPlan{{Path: workflowPath}}, + Workflows: []WorkflowPlan{{Path: workflowPath, ReplaceGraph: true}}, } require.NoError(t, Commit(context.Background(), rec, store, nil)) @@ -180,7 +180,7 @@ func TestCommitScopedParentAdvanceKeepsUntouchedWorkflowEdges(t *testing.T) { {NWO: parent.NWO, Ref: parent.Ref, SHA: newParentSHA, Resolution: Pinned, Direct: true, Workflows: []string{ciPath}}, {NWO: "owner/ci-child", Ref: "v2", SHA: ciChildSHA, Resolution: Pinned, RequiredBy: []string{parent.Key()}, Workflows: []string{ciPath}}, }, - Workflows: []WorkflowPlan{{Path: ciPath}}, + Workflows: []WorkflowPlan{{Path: ciPath, ReplaceGraph: true}}, } require.NoError(t, Commit(ctx, rec, store, nil)) @@ -232,7 +232,10 @@ func TestCommitConflictingScopedPlansKeepRecordedSharedClosure(t *testing.T) { rec := &Record{} for _, path := range workflows { rec.Entries = append(rec.Entries, entriesByWorkflow[path]...) - rec.Workflows = append(rec.Workflows, WorkflowPlan{Path: path}) + rec.Workflows = append(rec.Workflows, WorkflowPlan{ + Path: path, + ReplaceGraph: path == ".github/workflows/ci.yml", + }) } require.NoError(t, Commit(ctx, rec, store, nil)) @@ -246,3 +249,28 @@ func TestCommitConflictingScopedPlansKeepRecordedSharedClosure(t *testing.T) { }) } } + +func TestCommitKeepsCaseSensitiveRefsDistinct(t *testing.T) { + dir := t.TempDir() + workflowPath := filepath.Join(".github", "workflows", "ci.yml") + require.NoError(t, os.MkdirAll(filepath.Join(dir, filepath.Dir(workflowPath)), 0o755)) + require.NoError(t, os.WriteFile(filepath.Join(dir, workflowPath), []byte("on: push\njobs: {}\n"), 0o644)) + t.Chdir(dir) + + store, err := lockfile.LoadState(dir, fakeMeta{}) + require.NoError(t, err) + upperSHA := strings.Repeat("1", 40) + lowerSHA := strings.Repeat("2", 40) + rec := &Record{ + Entries: []Entry{ + {NWO: "owner/action", Ref: "Release", SHA: upperSHA, Resolution: Pinned, Direct: true, Workflows: []string{workflowPath}}, + {NWO: "OWNER/ACTION", Ref: "release", SHA: lowerSHA, Resolution: Pinned, Direct: true, Workflows: []string{workflowPath}}, + }, + Workflows: []WorkflowPlan{{Path: workflowPath, ReplaceGraph: true}}, + } + + require.NoError(t, Commit(context.Background(), rec, store, nil)) + file := store.File() + assert.Equal(t, "sha1-"+upperSHA, file.Dependencies["owner/action@Release"].Commit) + assert.Equal(t, "sha1-"+lowerSHA, file.Dependencies["owner/action@release"].Commit) +} diff --git a/internal/pin/plan.go b/internal/pin/plan.go index 6a166b0c..d590d15a 100644 --- a/internal/pin/plan.go +++ b/internal/pin/plan.go @@ -134,8 +134,11 @@ func planWorkflow(ctx context.Context, wr checks.WorkflowReport, opts PlanOption inventory := pruneStaleInventory(wr.Inventory, wr.Findings, opts.AcceptMoved, opts.Relock) repinMoved := repinsMoved(opts) && wr.CountByCategory(checks.RefMoved) > 0 || opts.AcceptMoved && wr.CountByCategory(checks.UnreachablePin) > 0 + movementRejected := wr.CountByCategory(checks.RefMoved) > 0 && !repinsMoved(opts) || + wr.CountByCategory(checks.UnreachablePin) > 0 && !opts.AcceptMoved + useLiveGraph := wr.LiveComplete && (wr.LiveGraphChanged || repinMoved) && !movementRejected - if !wr.NeedsAttention() && !repinMoved { + if !wr.NeedsAttention() && !repinMoved && !useLiveGraph { entries = verifiedEntries(inventory, wr.Path) rw := narrowVerifiedEntries(ctx, entries, opts, rewriteRefKeys) wplans = append(wplans, WorkflowPlan{Path: wr.Path, Rewrites: rw, SelfActionFiles: wr.SelfActionFiles}) @@ -160,22 +163,32 @@ func planWorkflow(ctx context.Context, wr checks.WorkflowReport, opts PlanOption unrecordedRefs, inventorySHA := partitionByInventory(inventory, wr.ActionRefs) entries = verifiedEntries(inventory, wr.Path) - // A moved transitive is absent from the direct ActionRefs, so refresh the - // complete scoped closure whenever movement is accepted. - if repinMoved { + var deps []dep.Dependency + var parentMap dep.ParentMap + var resolveErr error + if useLiveGraph { + unrecordedRefs, inventorySHA = partitionByInventory(nil, wr.ActionRefs) + entries = verifiedEntries(nil, wr.Path) + deps = append([]dep.Dependency(nil), wr.LiveDeps...) + parentMap = cloneParentMap(wr.LiveParents) + } else if repinMoved { + // A moved transitive is absent from the direct ActionRefs, so refresh + // the complete scoped closure whenever movement is accepted. unrecordedRefs, inventorySHA = partitionByInventory(nil, wr.ActionRefs) entries = verifiedEntries(nil, wr.Path) } - if len(unrecordedRefs) == 0 { + if len(unrecordedRefs) == 0 && !useLiveGraph { rw := narrowVerifiedEntries(ctx, entries, opts, rewriteRefKeys) wplans = append(wplans, WorkflowPlan{Path: wr.Path, Rewrites: rw, SelfActionFiles: wr.SelfActionFiles}) return planResult{entries: entries, wplans: wplans}, nil } - // Resolve live state for unrecorded refs only. - status("resolving " + wr.Path) - deps, parentMap, resolveErr := opts.Resolver.ResolveAllRecursive(ctx, unrecordedRefs) + if !useLiveGraph { + // Resolve live state for unrecorded refs only. + status("resolving " + wr.Path) + deps, parentMap, resolveErr = opts.Resolver.ResolveAllRecursive(ctx, unrecordedRefs) + } if resolveErr != nil { unresolved := unresolvedEntries(wr, unrecordedRefs, deps, resolveErr) if repinMoved { @@ -273,12 +286,13 @@ func planWorkflow(ctx context.Context, wr checks.WorkflowReport, opts PlanOption wplans = append(wplans, WorkflowPlan{ Path: wr.Path, Rewrites: rewrites, + ReplaceGraph: useLiveGraph, SelfActionFiles: wr.SelfActionFiles, }) } else if len(wplans) == 0 { // No rewrites and no plan entry yet — still include the workflow // so EnsureSentinel can be applied during commit. - wplans = append(wplans, WorkflowPlan{Path: wr.Path, SelfActionFiles: wr.SelfActionFiles}) + wplans = append(wplans, WorkflowPlan{Path: wr.Path, ReplaceGraph: useLiveGraph, SelfActionFiles: wr.SelfActionFiles}) } // Build entries for all pinned deps (skip any already emitted from inventory). @@ -289,24 +303,32 @@ func planWorkflow(ctx context.Context, wr checks.WorkflowReport, opts PlanOption return planResult{entries: entries, wplans: wplans}, nil } +func cloneParentMap(parents dep.ParentMap) dep.ParentMap { + cloned := make(dep.ParentMap, len(parents)) + for child, values := range parents { + cloned[child] = append([]string(nil), values...) + } + return cloned +} + // unresolvedEntries flags findings whose refs were attempted but failed to // resolve. On a partial failure deps holds the refs that did resolve, so only // the genuine misses (attempted and not in deps) are marked Unresolved. func unresolvedEntries(wr checks.WorkflowReport, unrecordedRefs []parserlock.ActionRef, deps []dep.Dependency, resolveErr error) []Entry { resolved := make(map[string]bool, len(deps)) for _, d := range deps { - resolved[strings.ToLower(d.NWO+"@"+d.Ref)] = true + resolved[dep.NormalizeKey(d.NWO+"@"+d.Ref)] = true } attempted := make(map[string]bool, len(unrecordedRefs)) for _, ref := range unrecordedRefs { - attempted[strings.ToLower(ref.Owner+"/"+ref.Repo+"@"+ref.Ref)] = true + attempted[dep.NormalizeKey(ref.Owner+"/"+ref.Repo+"@"+ref.Ref)] = true } var out []Entry for _, f := range wr.Findings { if f.ActionRef == nil { continue } - key := strings.ToLower(f.ActionRef.Owner + "/" + f.ActionRef.Repo + "@" + f.ActionRef.Ref) + key := dep.NormalizeKey(f.ActionRef.Owner + "/" + f.ActionRef.Repo + "@" + f.ActionRef.Ref) if !attempted[key] || resolved[key] { continue } @@ -579,14 +601,14 @@ func pruneStaleInventory(inventory []checks.InventoryEntry, findings []checks.Fi continue } d := f.Dependency - stale[strings.ToLower(d.NWO+"@"+d.Ref+":"+d.SHA)] = true + stale[dep.NormalizeKey(d.NWO+"@"+d.Ref)+":"+strings.ToLower(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) + key := dep.NormalizeKey(inv.Dep.NWO+"@"+inv.Dep.Ref) + ":" + strings.ToLower(inv.Dep.SHA) if stale[key] { continue } diff --git a/internal/pin/record.go b/internal/pin/record.go index ad32246a..60da2ed1 100644 --- a/internal/pin/record.go +++ b/internal/pin/record.go @@ -48,8 +48,9 @@ type Entry struct { // WorkflowPlan records what Commit must write for one workflow file. // Internal to the pin lifecycle; not serialized. type WorkflowPlan struct { - Path string - Rewrites map[string]string + Path string + Rewrites map[string]string + ReplaceGraph bool // SelfActionFiles are in-repo action definition files reached from this // workflow through `$/…`. The same rewrites apply to their `uses:` lines. SelfActionFiles []string diff --git a/internal/pipeline/checks/finding.go b/internal/pipeline/checks/finding.go index 86c9dba2..90e08f43 100644 --- a/internal/pipeline/checks/finding.go +++ b/internal/pipeline/checks/finding.go @@ -75,6 +75,15 @@ type WorkflowReport struct { Deps []dep.Dependency // Inventory lists all dependencies with direct/transitive classification. Inventory []InventoryEntry + // LiveDeps and LiveParents are the path-aware graph discovered from the + // current workflow roots. LiveComplete is false when recursive resolution + // returned any error, so callers must retain recorded graph authority. + LiveDeps []dep.Dependency + LiveParents dep.ParentMap + LiveComplete bool + // LiveGraphChanged reports a dependency or edge change at lockfile + // granularity. Commit may replace the recorded graph only when LiveComplete. + LiveGraphChanged bool // ParseWarnings from ExtractActionRefs (e.g. malformed uses: lines). ParseWarnings []string } diff --git a/internal/pipeline/diagnose.go b/internal/pipeline/diagnose.go index 7ba23201..4d63bdee 100644 --- a/internal/pipeline/diagnose.go +++ b/internal/pipeline/diagnose.go @@ -6,8 +6,10 @@ import ( "context" "errors" "fmt" + "sort" "strings" + parserlock "github.com/github/actions-lockfile/go/pkg/lockfile" "github.com/github/gh-actions-lock/internal/dep" "github.com/github/gh-actions-lock/internal/lockfile" "github.com/github/gh-actions-lock/internal/pinpool" @@ -62,11 +64,15 @@ func diagnoseOneParsed(ctx context.Context, pw checks.ParsedWorkflow, r *resolve // couldn't be resolved — partial results are kept. var liveDeps []dep.Dependency if r != nil { - var resolveErr error - liveDeps, _, resolveErr = r.ResolveAllRecursive(ctx, pw.Refs) + var recursiveErr error + var liveParents dep.ParentMap + liveDeps, liveParents, recursiveErr = r.ResolveAllRecursive(ctx, pw.Refs) + wr.LiveDeps, wr.LiveParents = alignLiveRootKeys(liveDeps, liveParents, pw.RecordedDeps, pw.Refs) + wr.LiveComplete = recursiveErr == nil + wr.LiveGraphChanged = wr.LiveComplete && !sameGraph(wr.LiveDeps, wr.LiveParents, pw.RecordedDeps, pw.RecordedParents) recordedLive, recordedErr := r.ResolveAllShallow(ctx, collectRecordedResolvable([]checks.ParsedWorkflow{pw})) liveDeps = dep.Dedup(append(liveDeps, recordedLive...)) - resolveErr = errors.Join(resolveErr, recordedErr) + resolveErr := errors.Join(recursiveErr, recordedErr) if resolveErr != nil { blockingResolverError := false if resolve.IsCompositeLocalPath(resolveErr) { @@ -100,6 +106,7 @@ func diagnoseOneParsed(ctx context.Context, pw checks.ParsedWorkflow, r *resolve if blockingResolverError { return wr } + // Low: we're surfacing the resolver failure itself, not a // verdict about any specific dependency. wr.Findings = append(wr.Findings, checks.Finding{ @@ -151,6 +158,55 @@ func diagnoseOneParsed(ctx context.Context, pw checks.ParsedWorkflow, r *resolve return wr } +func alignLiveRootKeys(live []dep.Dependency, parents dep.ParentMap, recorded []dep.Dependency, roots []parserlock.ActionRef) ([]dep.Dependency, dep.ParentMap) { + aligned := append([]dep.Dependency(nil), live...) + rewrites := make(map[string]string) + for i := range aligned { + for _, root := range roots { + if !parserlock.IsFullSha(root.Ref) || !aligned[i].MatchesActionRef(root) { + continue + } + for _, existing := range recorded { + if existing.MatchesActionRef(root) { + rewrites[aligned[i].Key()] = existing.Key() + aligned[i].Ref = existing.Ref + break + } + } + } + } + return aligned, dep.RekeyParentMap(parents, rewrites) +} + +func sameGraph(aDeps []dep.Dependency, aParents dep.ParentMap, bDeps []dep.Dependency, bParents dep.ParentMap) bool { + signature := func(deps []dep.Dependency, parents dep.ParentMap) map[string]string { + graph := make(map[string]string, len(deps)) + for _, d := range deps { + graph[dep.NormalizeKey(d.Key())] = "" + } + for child, rawParents := range parents { + normalized := make([]string, 0, len(rawParents)) + for _, parent := range rawParents { + normalized = append(normalized, dep.NormalizeKey(parent)) + } + sort.Strings(normalized) + graph[dep.NormalizeKey(child)] = strings.Join(normalized, "\x00") + } + return graph + } + a := signature(aDeps, aParents) + b := signature(bDeps, bParents) + if len(a) != len(b) { + return false + } + for key, parents := range a { + if other, ok := b[key]; !ok || other != parents { + return false + } + } + return true +} + // selfRepositoryFinding builds the informational finding for a workflow that // references same-repo actions via `$/…`. These are inherently pinned. func selfRepositoryFinding(pw checks.ParsedWorkflow) checks.Finding { diff --git a/test/integration/run.rb b/test/integration/run.rb index 1b04eddd..38821441 100644 --- a/test/integration/run.rb +++ b/test/integration/run.rb @@ -346,9 +346,29 @@ def golden_json_diff(expected, actual, path) # ── Fixture data ──────────────────────────────────────────────────────── -CHECKOUT_SHA = "11d5960a326750d5838078e36cf38b85af677262" -CHECKOUT_FULL_SHA = "d632683dd7b4114ad314bca15554477dd762a938" -CHECKOUT_MAIN_SHA = "f548e57e544e1ff5a4c46bf1e1b8685f8e4a348a" +checkout_refs = { + "refs/tags/v4" => "11d5960a326750d5838078e36cf38b85af677262", + "refs/tags/v4.2.0" => "d632683dd7b4114ad314bca15554477dd762a938", + "refs/heads/main" => "f548e57e544e1ff5a4c46bf1e1b8685f8e4a348a" +} +unless ARGV.include?("--stub") + output = `git ls-remote https://github.com/actions/checkout.git #{checkout_refs.keys.join(" ")}` + raise "failed to resolve actions/checkout fixture refs" unless $?.success? + + resolved = {} + output.lines.each do |line| + sha, ref = line.split + resolved[ref] = sha if checkout_refs.key?(ref) + end + missing = checkout_refs.keys - resolved.keys + raise "missing actions/checkout fixture refs: #{missing.join(", ")}" unless missing.empty? + + checkout_refs = resolved +end + +CHECKOUT_SHA = checkout_refs.fetch("refs/tags/v4") +CHECKOUT_FULL_SHA = checkout_refs.fetch("refs/tags/v4.2.0") +CHECKOUT_MAIN_SHA = checkout_refs.fetch("refs/heads/main") SETUP_GO_SHA = "4a3601121dd01d1626a1e23e37211e3254c1c06c" CACHE_SHA = "27d5ce7f107fe9357f9df03efb73ab90386fccae" MAIN_BRANCH_SHA = "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" @@ -578,10 +598,11 @@ def checkout_graphql_success(srv) query = body["query"] || "" if query.include?("expression") - sha = if query.include?("v4.2.0") - CHECKOUT_FULL_SHA - elsif query.include?("main:") - CHECKOUT_MAIN_SHA + expression = body.dig("variables", "expr0").to_s + sha = if expression.start_with?("v4.2.0") + CHECKOUT_FULL_SHA + elsif expression.start_with?("main") + CHECKOUT_MAIN_SHA else CHECKOUT_SHA end @@ -642,6 +663,12 @@ def wire_checkout_fresh(s, token) # not listed here either use no stub or rely on catalog-level config. STUB_WIRING = { + stub_ref_specific_full_tag: ->(s) { + wire_checkout_success(s, "gho_fake_full_tag_token") + }, + stub_ref_specific_branch: ->(s) { + wire_checkout_success(s, "gho_fake_branch_token") + }, fresh_tag_warns_without_cooldown: ->(s) { wire_checkout_fresh(s, "gho_fake_fresh_warn_token") }, diff --git a/test/scenarios/catalog.yml b/test/scenarios/catalog.yml index 0ba8b64c..bcd112b1 100644 --- a/test/scenarios/catalog.yml +++ b/test/scenarios/catalog.yml @@ -704,6 +704,44 @@ scenarios: exit: 0 stdout_is_json: true + - name: stub_ref_specific_full_tag + category: output_modes + description: "GraphQL stub resolves the ref from expression variables for a full tag" + needs_stub: true + tags: [stub] + flags: ["--no-fix", "--json=findings"] + fixtures: + workflows: + ci.yml: + name: CI + actions: ["actions/checkout@v4.2.0"] + lockfile_template: pinned_checkout_full + expect: + exit: 0 + stdout_is_json: true + jq: + - expr: ".findings | length" + equals: "0" + + - name: stub_ref_specific_branch + category: output_modes + description: "GraphQL stub resolves the ref from expression variables for a branch" + needs_stub: true + tags: [stub] + flags: ["--no-fix", "--json=findings"] + fixtures: + workflows: + ci.yml: + name: CI + actions: ["actions/checkout@main"] + lockfile_template: pinned_checkout_main + expect: + exit: 0 + stdout_is_json: true + jq: + - expr: ".findings | length" + equals: "0" + - name: verify_local_all_covered category: output_modes description: "--verify-local with all refs covered — exits 0"