Stop compiler from emitting deprecated artifact v3 pins in GHES mode#43298
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #43298 does not have the 'implementation' label and has only 37 new lines of code in business logic directories (threshold is 100). |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Test Quality Sentinel completed test quality analysis. |
There was a problem hiding this comment.
Pull request overview
This PR updates gh aw compile to stop generating deprecated actions/upload-artifact@v3 / actions/download-artifact@v3 pins when GHES compatibility mode is enabled, ensuring compiled lock workflows keep using the standard (non-v3) pinned artifact actions.
Changes:
- Removed the GHES-only hardcoded v3 artifact pin override so action pin resolution is consistent across GHES and non-GHES modes.
- Updated tests to assert GHES mode never emits v3 artifact pins and that GHES/default mode pins match.
- Updated CLI/help/schema text to reflect that GHES mode remains available but artifact pins stay on the latest non-v3 versions.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/repo_config.go | Updates aw.json GHES documentation to reflect non-v3 artifact pins. |
| pkg/workflow/compiler.go | Adjusts GHES-mode logging to reflect non-v3 artifact pin behavior. |
| pkg/workflow/compiler_types.go | Updates GHES compat field/method documentation for the new behavior. |
| pkg/workflow/action_pins.go | Removes the GHES-specific v3 artifact pin override from action pin resolution. |
| pkg/workflow/action_pins_test.go | Updates tests to ensure GHES mode does not emit v3 pins and matches default pins. |
| pkg/parser/schemas/repo_config_schema.json | Updates schema description for ghes to reflect non-v3 artifact pins. |
| pkg/cli/compile_config.go | Updates CompileConfig GHESCompat flag comment to reflect non-v3 artifact pins. |
| pkg/cli/compile_compiler_setup.go | Updates GHES flag wiring/logging text to reflect non-v3 artifact pins. |
| cmd/gh-aw/main.go | Updates --ghes flag help text to reflect non-v3 artifact pins. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 9/9 changed files
- Comments generated: 4
- Review effort level: Low
| if c.ghesArtifactCompat { | ||
| actionPinsLog.Print("GHES artifact compatibility mode enabled: artifact actions will use v3.x pins") | ||
| actionPinsLog.Print("GHES artifact compatibility mode enabled: using latest artifact action pins (v3 pins are deprecated)") | ||
| } |
| // SetGHESCompat enables GHES artifact compatibility mode via the --ghes CLI flag. | ||
| // When true, the compiler emits GHES-compatible v3.x artifact action pins | ||
| // (upload-artifact@v3, download-artifact@v3) instead of the latest v7/v8. | ||
| // v3 artifact actions are deprecated, so artifact actions continue using the | ||
| // latest non-v3 pins. | ||
| // This flag takes precedence over the aw.json ghes field. |
| compileCmd.Flags().Bool("no-models-dev-lookup", false, "Disable compile-time models.dev pricing lookup for models missing from the embedded catalog") | ||
| compileCmd.Flags().String("prior-manifest-file", "", "Path to a JSON file containing pre-cached gh-aw-manifests (map[lockFile]*GHAWManifest); used by the MCP server to supply a tamper-proof manifest baseline captured at startup") | ||
| compileCmd.Flags().Bool("ghes", false, "Enable GitHub Enterprise Server (GHES) compatibility mode: emit upload-artifact@v3 and download-artifact@v3 instead of the latest v7/v8 which are not supported on GHES. Overrides the aw.json ghes field") | ||
| compileCmd.Flags().Bool("ghes", false, "Enable GitHub Enterprise Server (GHES) compatibility mode. Artifact actions continue using latest non-v3 pins (v3 is deprecated). Overrides the aw.json ghes field") |
| // This is the preferred call site for code running inside a Compiler method, since it | ||
| // automatically honours the per-compilation GHES compat flag without any global state. |
🧪 Test Quality Sentinel Report✅ Test Quality Score: 90/100 — Excellent
📊 Metrics (3 tests)
Verdict
What changed: All three tests were updated to flip their behavioral contracts from asserting that GHES compat mode returns v3 pins to asserting it does not return v3 pins — directly mirroring the production change that removed the deprecated Inflation note: The 25:1 addition ratio (test: 25 additions, prod: 1 addition) technically triggers the inflation flag. However, the production change is primarily a deletion (−25 lines removing the hardcoded v3 map), and the test file also nets a reduction (−30 deletions, +25 additions). This is expected churn when removing deprecated functionality, not test padding. References: Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "awmgmcpg"See Network Configuration for more information.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: Stop compiler from emitting deprecated artifact v3 pins in GHES mode
The core change is correct and well-targeted — removing the hardcoded ghesArtifactCompatPins map and the GHES v3 override in getActionPin() prevents compilation from emitting the deprecated actions/upload-artifact@v3 / actions/download-artifact@v3 pins that now hard-fail at runtime.
Two non-blocking concerns worth addressing:
1. ghesArtifactCompat is now a write-only / no-op field (compiler.go:444)
After removing the v3 override, the ghesArtifactCompat field is set and logged but never drives any behavioral branch. --ghes and aw.json ghes: true are silently no-ops beyond the log line. If the flag is reserved for future GHES-specific behavior, a comment stating that intent would be useful. If not, removing the field and its CLI/schema surface reduces confusion.
2. Tests validate a tautology (action_pins_test.go:1607)
The updated tests set compatCompiler.ghesArtifactCompat = true (a private field that now has no effect) and then assert the result matches the default — which trivially passes because the flag does nothing. These tests no longer detect a regression where GHES compat accidentally re-introduces v3 pins. Consider testing through the public API (SetGHESCompat) after a real compile, or at least leaving a comment explaining that the flag is currently inert.
The doc, schema, and log string updates are accurate and clearly communicated. No security or data-correctness issues found.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 78.2 AIC · ⌖ 6.23 AIC · ⊞ 4.9K
| @@ -442,7 +442,7 @@ func (c *Compiler) CompileWorkflowData(workflowData *WorkflowData, markdownPath | |||
| } | |||
| } | |||
| if c.ghesArtifactCompat { | |||
There was a problem hiding this comment.
After removing the v3 pin override from getActionPin(), ghesArtifactCompat is now write-only: it is set (lines 437–441) and logged here, but no code path makes a behavioral decision based on it. This means --ghes / aw.json ghes: true silently have no effect on compilation output beyond this log message.
If GHES mode is intended to gate future non-artifact behavior, this is fine as scaffolding. But if the flag is now truly inert, consider removing the field, CLI flag, and schema entry to avoid misleading users — or add an explicit comment documenting that GHES compat is retained for future use.
@copilot please address this.
| @@ -1607,19 +1607,19 @@ func TestGetActionPinGHESArtifactCompat(t *testing.T) { | |||
| compatCompiler.ghesArtifactCompat = true | |||
There was a problem hiding this comment.
The test sets compatCompiler.ghesArtifactCompat = true directly (a private field). Since the field now has no behavioral effect on getActionPin, this test validates a tautology: it sets a no-op flag, then asserts the result is the same as without the flag. The test will always pass regardless of whether GHES compat is wired up correctly.
Consider driving GHES compat through the public API (compatCompiler.SetGHESCompat(true) followed by a compile step) or testing at integration level via configureCompilerFlags, to ensure the public contract is actually exercised.
@copilot please address this.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs, /tdd, and /codebase-design — commenting only; no blocking correctness issues, but there are follow-on cleanup opportunities worth addressing.
📋 Key Themes & Highlights
Key Themes
- Stale doc comment in
action_pins.gostill claims the method honours the GHES compat flag, which is no longer true. - Semantically dead field —
ghesArtifactCompat(and the supporting two-field pipeline incompiler.go) now only gate a log statement, making the module harder to navigate. - Test precision gaps — one isolation assertion compares the wrong compiler instances, and
TestGHESArtifactCompatResethas a name that no longer describes what it verifies.
Positive Highlights
- ✅ Root cause correctly removed: deleting
ghesArtifactCompatPinseliminates the broken runtime path at the source. - ✅ Regression tests updated to assert the new invariant (GHES mode == default mode for artifact pins).
- ✅ Clean net-deletion PR (-30 lines): simplifies the codebase rather than papering over the problem.
- ✅ Comprehensive doc/comment/schema sweep — all user-facing surfaces updated consistently.
- ✅ Structured subtests (
t.Run) inTestGHESArtifactCompatDoesNotUseV3improve failure localisation.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 84.2 AIC · ⌖ 5.78 AIC · ⊞ 6.6K
Comment /matt to run again
Comments that could not be inline-anchored
pkg/workflow/action_pins.go:72
[/diagnosing-bugs] Stale doc comment — the GHES compat flag no longer affects this function's output, but line 72 still reads: "automatically honours the per-compilation GHES compat flag without any global state." This will mislead future contributors into thinking getActionPin behaves differently based on the flag.
<details>
<summary>💡 Suggested update</summary>
Remove the now-false claim about honouring the GHES compat flag:
// getActionPin returns the pinned reference for t…
</details>
<details><summary>pkg/workflow/compiler_types.go:108</summary>
**[/codebase-design]** The `ghesArtifactCompat` field is now semantically dead for its original purpose — it no longer changes which pin is returned, only which log message is emitted. Consider whether the field (and the entire GHES compat branching path in `compiler.go`) is still needed, or whether it can be collapsed into just logging at the call site where `repoConfig.GHES` is read.
<details>
<summary>💡 Why this matters</summary>
A boolean field that used to gate a meaningful code path bu…
</details>
<details><summary>pkg/workflow/action_pins_test.go:1632</summary>
**[/tdd]** Test name no longer matches intent. `TestGHESArtifactCompatReset` now verifies that compat and default compilers return the **same** pin — which is correct — but "reset" implies it was testing state isolation between sequential calls, not pin equality. The comment inside also reads _"toggling compat mode does not leak state"_, which is subtly incorrect: two separate `NewCompiler()` instances never share state regardless of this change.
<details>
<summary>💡 Suggested rename</summary…
</details>
<details><summary>pkg/workflow/compiler_types.go:248</summary>
**[/codebase-design]** `SetGHESCompat` now sets `ghesCompatFromCLI` but the method is called _SetGHESCompat_, which is still the public name. However, the field it sets is not the one whose value ultimately gates anything meaningful — `ghesArtifactCompat` is set later in `CompileWorkflowData`. If GHES compat is going to be retained as a concept, consider renaming `ghesCompatFromCLI` → `ghesCompatOverride` or simply remove the field entirely and pass the flag directly through the compile path.
…
</details>
<details><summary>pkg/workflow/action_pins_test.go:1599</summary>
**[/tdd]** The assertion that `actions/checkout` is unchanged uses a different compiler instance (`compatCompiler`) than the one that captured `checkoutPinBefore` (`defaultCompiler`). The test comment says _"assert it is unchanged after"_, but since the two compilers are separate instances, this only verifies both compilers return the same checkout pin — not that `defaultCompiler`'s state was unchanged after `compatCompiler` ran. To actually test isolation, re-call `defaultCompiler.getActionPin…
</details>
There was a problem hiding this comment.
Review: Non-blocking observation on test coverage
The core change — removing the GHES v3 artifact pin override — is correct and the doc/comment updates are thorough. One test is now misleading after the behavioral change.
Details
TestGHESArtifactCompatReset (action_pins_test.go:1632) became a tautology when the v3 override was removed. Both compiler instances now execute identical code paths and always return the same pin, so the defaultPin != compatPin assertion can never fire. The test name implies it guards against state leakage, but it no longer does — a future regression reintroducing shared global state would pass this test silently. See inline comment for options.
Everything else looks clean: the logic simplification is correct, the other test rewrites properly invert their assertions, and the docs/comments are consistently updated across all touched files.
Warning
Firewall blocked 2 domains
The following domains were blocked by the firewall during workflow execution:
patchdiff.githubusercontent.comproxy.golang.org
To allow these domains, add them to the
network.allowedlist in your workflow frontmatter:
network:
allowed:
- defaults
- "patchdiff.githubusercontent.com"
- "proxy.golang.org"See Network Configuration for more information.
🔎 Code quality review by PR Code Quality Reviewer · 197.2 AIC · ⌖ 6.55 AIC · ⊞ 5.4K
Comment /review to run again
| @@ -1651,24 +1650,20 @@ func TestGHESArtifactCompatReset(t *testing.T) { | |||
| } | |||
There was a problem hiding this comment.
TestGHESArtifactCompatReset is now a tautology that cannot detect state leakage. Both compiler instances run through identical code paths after the v3 override was removed, so defaultPin == compatPin unconditionally — the assertion if defaultPin != compatPin can never fire regardless of whether state actually leaks.
💡 Suggested fix
The test needs to verify isolation through a mechanism that would differ if state were shared. One option: inject something into compatCompiler's cache that would influence output, then confirm defaultCompiler is unaffected.
Alternatively rename to TestGHESArtifactCompatFlagIsPerInstance and add a comment explaining it asserts same-output (not non-interference) — otherwise future maintainers will trust it as a leakage guard when it provides none.
As written, this test gives false confidence: any future regression that reintroduces shared global state would not be caught here.
|
🛠️ Agentic Maintenance updated this pull request branch. |
|
@copilot merge main and recompile |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
github-actions
Bot
added
pr-batch:compiler-fixes
pr-priority:high
pr-risk:low
pr-type:bug
labels
🤖 PR Triage
Score breakdown: Impact 36 (prevents GHES runtime failures from retired v3 artifact APIs) + Urgency 20 + Quality 14 (clean removal, approved) Removes deprecated GHES artifact v3 pin override. -68/+38 lines, approved by github-actions bot. Ready for human sign-off.
|
Done in commit 7c62b59. I merged latest |
|
@copilot please run the
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed in ff58f4d and 930b2d4. I updated stale GHES wording in |
gh aw compilecould emitactions/upload-artifact@v3/actions/download-artifact@v3under GHES compatibility settings, which now hard-fail at runtime due to retired v3 artifact backend APIs. This change removes that deprecated path so generated lock workflows keep using current non-v3 pinned artifact actions.Compiler behavior
pkg/workflow/action_pins.go.Compiler.getActionPin(...)now consistently resolves artifact actions from the standard pinned dataset (same behavior as non-GHES path).Regression coverage
pkg/workflow/action_pins_test.goto assert:# v3artifact pins.CLI/config contract alignment
cmd/gh-aw/main.gopkg/cli/compile_compiler_setup.gopkg/cli/compile_config.gopkg/workflow/compiler.gopkg/workflow/compiler_types.gopkg/workflow/repo_config.gopkg/parser/schemas/repo_config_schema.json