Repository navigation
Fail activation early for stale provenance workflows - #70348
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
🔎 PR Code Quality Reviewer is reviewing code quality for this pull request... |
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality SentinelOutcome: The agent completed and reported no action. A no-op output alone does not verify that all required work was completed or that publication was possible. Agent messages are omitted from this comment. The category uses structured run status, not interpretation of agent prose.
|
|
✅ Ponytail Reviewer completed successfully! Ponytail ReviewerOutcome: The agent completed and reported no action. A no-op output alone does not verify that all required work was completed or that publication was possible. Agent messages are omitted from this comment. The category uses structured run status, not interpretation of agent prose.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs (triage: bug_fix, confirmed via pr-triage) — requesting changes on a scope issue in the core fix.
📋 Key Themes
Key Themes
- Root-cause mismatch (blocking issue):
check_version_updates.cjsruns for every released-version workflow regardless of whether it declaressafe-outputs:. The newPROVENANCE_MINIMUM_COMPILER_VERSIONgate fails activation for workflows that never touchGH_AW_JOB_CHECK_RUN_ID/appendExecutionProvenance, which is the opposite of what #70273's proposed-implementation-plan step 1 asks for ("avoid globally blocking older compiler versions that still work with their pinned older runtime"). The compiler already hasctx.data.SafeOutputs != nilavailable inaddActivationVersionCheckStepand could pass that signal through to gate the check precisely. - Test gap: no regression test proves a pre-v0.91.10 workflow without safe-outputs configured still passes activation (the actual compatible case the issue asks to preserve). Boundary test at
v0.91.9is also missing.
Positive Highlights
- ✅ Fail-fast-before-agent-execution approach is the right shape for the bug (avoids wasted agent runs that fail only at publish time).
- ✅ Local/offline-safe check (works even when remote
compat.jsonis unreachable) is a solid design choice and well tested for that path. - ✅ Clear, actionable summary/failure messaging pointing users to
gh aw upgrade+gh aw compile. - ✅ Docs update in
releases.mdexplains the new requirement for users encountering it via Dependabot-only upgrades.
See inline comments for the specific fix and a suggested regression test.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet50 · 92.7 AIC · ⌖ 15.1 AIC · ⊞ 10.1K
Comment /matt to run again
| return; | ||
| } | ||
|
|
||
| if (compareVersions(compiledVersion, PROVENANCE_MINIMUM_COMPILER_VERSION) < 0) { |
There was a problem hiding this comment.
[/diagnosing-bugs] This unconditionally fails activation for every workflow compiled before v0.91.10 — including workflows with no safe-outputs: configured at all, which never call appendExecutionProvenance and were never affected by the missing GH_AW_JOB_CHECK_RUN_ID. Issue #70273 explicitly says the fix should "avoid globally blocking older compiler versions that still work with their pinned older runtime."
💡 Root cause vs. symptom
check_version_updates.cjs runs in the activation job for all released-version workflows (pkg/workflow/compiler_activation_steps.go:206, gated only on UpdateCheckDisabled / IsReleasedVersion), with no visibility into whether the compiled workflow actually declares safe-outputs. A workflow with zero safe-outputs will now fail activation purely because it predates v0.91.10, even though it never touches GH_AW_JOB_CHECK_RUN_ID.
Consider gating the check on whether safe-outputs publishing is actually present — e.g. pass a GH_AW_HAS_SAFE_OUTPUTS env var from addActivationVersionCheckStep (compiler has ctx.data.SafeOutputs != nil readily available) and only enforce PROVENANCE_MINIMUM_COMPILER_VERSION when that flag is true. This fixes the actual root cause (missing provenance env var reaching add_comment.cjs) instead of a broader compiler-version proxy that over-blocks unaffected workflows.
A regression test for "old compiler + no safe-outputs configured → should NOT fail" would catch this gap; none of the new tests in check_version_updates.test.cjs cover that case.
@copilot please address this.
There was a problem hiding this comment.
Addressed in bc85ee4e: activation now passes GH_AW_HAS_SAFE_OUTPUTS, and the provenance minimum only applies when safe outputs are configured. Regression coverage includes pre-minimum workflows without safe outputs and v0.91.9.
| mockFetchError(new Error("ECONNREFUSED")); | ||
| await runMain(); | ||
| expect(mockFetch).not.toHaveBeenCalled(); | ||
| expect(mockCore.setFailed).toHaveBeenCalledWith(expect.stringContaining("v0.91.10 or newer")); |
There was a problem hiding this comment.
[/tdd] Good coverage for the reject/accept boundary at v0.91.10, but there is no test for the exact boundary v0.91.9 (one patch below) failing and no test for the scenario above — a pre-v0.91.10 workflow without safe-outputs configured, which this check currently still blocks unnecessarily.
💡 Missing boundary/negative case
it("should reject the patch version immediately below the provenance requirement", async () => {
process.env.GH_AW_COMPILED_VERSION = "v0.91.9";
mockFetchError(new Error("ECONNREFUSED"));
await runMain();
expect(mockCore.setFailed).toHaveBeenCalledWith(expect.stringContaining("v0.91.10 or newer"));
});Exact-boundary tests read as a clearer specification of the cutoff than the current v0.88.7 example alone.
@copilot please address this.
There was a problem hiding this comment.
Added the exact v0.91.9 rejection test and the older-version/no-safe-outputs regression case in bc85ee4e.
There was a problem hiding this comment.
🟡 Changes recommended
The new early return bypasses existing blocked-version warnings and issue notifications.
1 open issue
What changed in this PR
Detects incompatible compiled workflows during activation, before agent execution and safe-output publishing.
Changes:
- Requires compiler v0.91.10 or newer locally.
- Adds compatibility and remote-config outage coverage.
- Documents upgrading, recompiling, and committing regenerated workflows.
| File | Description |
|---|---|
docs/src/content/docs/reference/releases.md |
Adds migration guidance. |
actions/setup/js/check_version_updates.test.cjs |
Tests the local compatibility requirement. |
actions/setup/js/check_version_updates.cjs |
Adds the activation-time compiler minimum. |
.changeset/patch-check-run-provenance-compatibility.md |
Records the patch release change. |
Review effort: Max
| return; | ||
| } | ||
|
|
||
| if (compareVersions(compiledVersion, PROVENANCE_MINIMUM_COMPILER_VERSION) < 0) { |
There was a problem hiding this comment.
Addressed in bc85ee4e: remote blocked-version handling and issue reporting now run before the provenance failure; the local minimum still applies when config fetching fails. Added a blocked v0.85.3 test asserting the blocked summary and issue notification.
|
@copilot address the following outstanding work in one pass:
Push the necessary fixes, reply to each listed review thread and resolve it when addressed. Ignore feedback already answered or resolved. Use the pr-finisher skill and stop when only human review or CI remains; do not trigger CI. Sous-chef head: 2c3ec8a
|
…up-dependency-issue 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>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| return; | ||
| } | ||
|
|
||
| const requiresExecutionProvenance = process.env.GH_AW_HAS_SAFE_OUTPUTS === "true" && compareVersions(compiledVersion, PROVENANCE_MINIMUM_COMPILER_VERSION) < 0; |
There was a problem hiding this comment.
Updated in 6d73d77: missing or unknown GH_AW_HAS_SAFE_OUTPUTS now requires provenance for below-minimum versions, while explicit false remains exempt. Added regression coverage for both missing and unknown signals.
|
@copilot address the following outstanding work in one pass:
Push the necessary fixes, reply to each listed review thread and resolve it when addressed. Ignore feedback already answered or resolved. Use the pr-finisher skill and stop when only human review or CI remains; do not trigger CI. Sous-chef head: bc85ee4
|
…up-dependency-issue 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>
A setup-only dependency upgrade can introduce a safe-output provenance requirement that older compiled workflows do not satisfy, causing publication to fail after agent execution. Activation now detects this mismatch before the agent runs.
Compatibility check: Require compiler version
v0.91.10or newer in runtimes with the provenance requirement. This local check works even when remote compatibility configuration is unavailable and does not change older pinned runtimes.Migration guidance: The activation summary and release documentation direct users to update the compiler, regenerate, and commit workflows: