Warn on outdated action versions in user-provided steps#43455
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This pull request adds compile-time diagnostics for user-authored uses: steps that reference an outdated GitHub Action version compared to the embedded action_pins.json “latest” pin, helping users avoid stale major tags like @v3.
Changes:
- Introduces
warnIfOutdatedActionVersionto detect outdated semver-style action tags (major-only vs fully-qualified) and emit a deduplicated warning. - Calls the warning helper from
applyActionPinToTypedStep, the central pinning entry point for user-provided steps. - Adds unit tests covering the warning decision tree and deduplication behavior.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/action_pins.go | Adds outdated-version warning helper and wires it into step pinning. |
| pkg/workflow/action_pins_test.go | Adds tests for warning behavior, deduplication, and integration via applyActionPinToTypedStep. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 2/2 changed files
- Comments generated: 3
- Review effort level: Low
| if tt.expectWarn { | ||
| if output == "" { | ||
| t.Errorf("expected a warning to be printed but got none") | ||
| } | ||
| if tt.warnContains != "" && !strings.Contains(output, tt.warnContains) { | ||
| t.Errorf("warning output %q does not contain %q", output, tt.warnContains) | ||
| } | ||
| if !strings.Contains(output, tt.latestVer) { | ||
| t.Errorf("warning output %q does not mention latest version %q", output, tt.latestVer) | ||
| } | ||
| } else { | ||
| if output != "" { | ||
| t.Errorf("expected no warning but got: %q", output) | ||
| } | ||
| } |
| count := strings.Count(output, "v5.0.0") | ||
| if count != 1 { | ||
| t.Errorf("expected warning to appear exactly once, got %d occurrences in: %q", count, output) | ||
| } |
| if !strings.Contains(output, "v5.0.0") { | ||
| t.Errorf("expected outdated-version warning mentioning v5.0.0 in stderr, got: %q", output) | ||
| } | ||
| if !strings.Contains(output, "v7.0.0") { | ||
| t.Errorf("expected outdated-version warning mentioning latest v7.0.0 in stderr, got: %q", output) | ||
| } |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
|
✅ Test Quality Sentinel completed test quality analysis. |
|
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (271 new lines in Draft ADR committed:
What to do next
Once an ADR is linked in the PR body, this gate will re-run and verify the implementation matches the decision. Why ADRs MatterADRs create a searchable, permanent record of why the codebase looks the way it does. Future contributors (and your future self) will thank you. Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in
|
🧪 Test Quality Sentinel Report✅ Test Quality Score: 80/100 — Excellent
📊 Metrics (4 tests)
| Test | File | Classification | Issues |
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd, /codebase-design, and /grill-with-docs — requesting changes on a few correctness and test-quality issues.
📋 Key Themes & Highlights
Key Themes
- Test fragility:
os.Stderris mutated globally in the new tests withoutt.Cleanupguards, which can cause flaky failures ift.Fatalfires before the manual restore. - Partial-tag edge case: The
isPartialTaglogic only guardsv4(no dot) but notv4.1(major.minor), which would generate a false-positive warning forv4.1when the latest isv4.2.0. A test and a one-line fix are both needed. - Nil-data deduplication gap: When
data == nil, the dedup cache is bypassed and the warning can fire multiple times per compilation — the contract should be tightened or documented. - Message format mismatch: The emitted warning string doesn't match the two-line format shown in the PR description.
- Test brittleness: Integration tests hardcode
v5.0.0/v7.0.0against the embedded pins; they'll silently stop being meaningful when the embedded pins are bumped.
Positive Highlights
- ✅ Excellent overall test coverage: 8 unit cases + deduplication test + 2 integration tests — well above average for a new helper.
- ✅ Clean integration point: calling from
applyActionPinToTypedStepensures alluses:step types (pre-steps, steps, post-steps) are covered by a single hook. - ✅ Smart floating-tag semantics: suppressing warnings for same-major
@vNrefs avoids common false positives. - ✅ Deduplication via
ActionPinWarningsprevents warning spam in multi-step workflows.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 81.8 AIC · ⌖ 6.24 AIC · ⊞ 6.6K
Comment /matt to run again
| if err != nil { | ||
| t.Fatalf("os.Pipe() failed: %v", err) | ||
| } | ||
| os.Stderr = w |
There was a problem hiding this comment.
[/tdd] os.Stderr is a process-global: if a subtest panics or t.Fatal fires before the manual restore, all subsequent tests in the same process will write into the closed pipe — causing flaky or incorrect results in parallel CI runs.
💡 Use t.Cleanup for guaranteed restoration
Replace the manual save/restore with a t.Cleanup call immediately after opening the pipe:
os.Stderr = w
t.Cleanup(func() {
w.Close()
os.Stderr = oldStderr
})This ensures stderr is always restored even when t.Fatal / t.FailNow short-circuits the test, and prevents state leakage if tests are ever run with t.Parallel().
@copilot please address this.
|
|
||
| // Deduplicate: only emit the warning once per repo@version within this compilation. | ||
| cacheKey := "outdated:" + actionpins.FormatCacheKey(actionRepo, rawVersion) | ||
| if data != nil { |
There was a problem hiding this comment.
[/codebase-design] When data is nil, deduplication is silently skipped — meaning each call to warnIfOutdatedActionVersion with a nil data pointer will emit a duplicate warning. This is a subtle correctness gap since applyActionPinToTypedStep can be called with nil data in tests.
💡 Suggested fix
Consider a stricter guard: emit the warning only when data != nil, or document explicitly that callers must always supply a non-nil WorkflowData. If nil data is a valid production path, the warning should either be suppressed or dedup should use a package-level set:
// If data is nil, skip the warning to avoid duplicate noise.
if data == nil {
return
}@copilot please address this.
| // resolve to the latest compatible patch within that major. Only warn when | ||
| // the latest available major version is higher — same-major newer patches are | ||
| // not "outdated" for a floating tag. | ||
| isPartialTag := !strings.Contains(strings.TrimPrefix(rawVersion, "v"), ".") |
There was a problem hiding this comment.
[/codebase-design] The isPartialTag check treats v4.1 (major.minor, no patch) as a *full* tag — but a v4.1 floating ref behaves the same as v4 within the same major: it floats over patch releases. This means v4.1 with latest v4.2.0 would emit a spurious warning even though both are within the same major.
💡 Consider extending the partial-tag logic to major.minor refs
The partial-tag guard could be defined as *fewer than 3 components*:
// Treat tags without a patch component (e.g. "v4", "v4.1") as floating refs.
withoutV := strings.TrimPrefix(rawVersion, "v")
isPartialTag := strings.Count(withoutV, ".") < 2This avoids a false positive for v4.1 → latest v4.2.0 since both share the same major, and aligns the warning semantics with how GitHub Actions resolves floating tags.
@copilot please address this.
| } | ||
|
|
||
| // TestWarnIfOutdatedActionVersion tests the warnIfOutdatedActionVersion helper. | ||
| func TestWarnIfOutdatedActionVersion(t *testing.T) { |
There was a problem hiding this comment.
[/tdd] There is no test case for the v4.1 (major.minor, no patch) partial-tag edge case. Given the current logic, v4.1 with latest v4.2.0 would incorrectly emit a warning — a regression test would catch this and any future regressions in the partial-tag boundary.
💡 Suggested additional test cases
{
name: "minor partial tag same major does not warn",
repo: checkoutRepo,
rawVersion: "v7.0",
latestVer: "v7.1.0",
expectWarn: false,
},
{
name: "minor partial tag older major warns",
repo: checkoutRepo,
rawVersion: "v6.0",
latestVer: "v7.1.0",
expectWarn: true,
warnContains: "v6.0",
},@copilot please address this.
|
|
||
| // TestApplyActionPinToTypedStep_OutdatedWarning checks that applying an outdated action | ||
| // pin via applyActionPinToTypedStep emits a warning on stderr. | ||
| func TestApplyActionPinToTypedStep_OutdatedWarning(t *testing.T) { |
There was a problem hiding this comment.
[/tdd] The integration test TestApplyActionPinToTypedStep_OutdatedWarning hardcodes v5.0.0 and v7.0.0 as implicit knowledge of the embedded pins database. If the embedded pins are updated (e.g. a future v8.0.0 ships), the test still passes — but it no longer proves the warning fires for a version that's actually below latest.
💡 Derive expected values from the live embedded data
Use getLatestActionPinByRepo to dynamically look up the real latest version and construct an older version from it, making the test self-adapting:
latestPin, ok := getLatestActionPinByRepo("actions/checkout")
if !ok {
t.Skip("no embedded pin for actions/checkout")
}
// Use a version guaranteed to be older than whatever is embedded.
olderVersion := fmt.Sprintf("v%d.0.0", latestPin.ParsedVersion().Major-2)
step := &WorkflowStep{Uses: "actions/checkout@" + olderVersion}@copilot please address this.
|
|
||
| warningMsg := fmt.Sprintf("Action %s@%s is outdated; latest available version is %s. Consider upgrading (update the version tag in your workflow file).", | ||
| actionRepo, rawVersion, latestVersion) | ||
| fmt.Fprintln(os.Stderr, console.FormatWarningMessage(warningMsg)) |
There was a problem hiding this comment.
[/grill-with-docs] The warning message emitted to stderr is formatted as a single long sentence with no newline separation from the "Consider upgrading" suggestion. The PR's example output in the description shows a two-line format — the implementation collapses it into one line, which differs from what's advertised.
💡 Match the PR's documented output format
The PR description shows:
⚠ Action actions/checkout@v3 is outdated; latest available version is v7.0.0.
Consider upgrading (update the version tag in your workflow file).
Splitting the message at the period keeps the warning scannable:
warningMsg := fmt.Sprintf("Action %s@%s is outdated; latest available version is %s.\n Consider upgrading (update the version tag in your workflow file).",
actionRepo, rawVersion, latestVersion)@copilot please address this.
|
@copilot invoke pr-finisher skill |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Handled. The follow-up fixes are in |
🤖 PR Triage — Run #7
Score breakdown: impact 28 + urgency 18 + quality 14 New compiler warning for outdated
|
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Fixed in a8e881b. The failing CGO test was caused by a stale |
Agents tend to generate
uses:references with stale major versions (e.g.actions/checkout@v3) even when a newer version is pinned inaction_pins.json. The compiler now detects this and surfaces an actionable warning at compile time.Changes
pkg/workflow/action_pins.go— newwarnIfOutdatedActionVersionhelper called fromapplyActionPinToTypedStep(the single pinning entry point for all user-providedsteps,pre-steps,pre-agent-steps, andpost-steps):@vN): warns only when the latest major is higher — avoids false positives since@v4floats to the latest v4.x anyway@vN.M.P): warns whenlatest > requestedvia semver comparerepo@versionwithin a compilation viaActionPinWarningspkg/workflow/action_pins_test.go— unit tests covering all branching cases plus deduplicationExample output