-
Notifications
You must be signed in to change notification settings - Fork 477
Migrate deprecated needs.activation.outputs references to steps.sanitized.outputs #47264
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
2678671
dcf590a
5687e0e
55d3a8c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,52 @@ | ||
| # ADR-47264: Extend detectTextOutputUsage to Recognise Deprecated needs.activation.outputs Forms | ||
|
|
||
| **Date**: 2026-07-22 | ||
| **Status**: Draft | ||
| **Deciders**: Unknown (Copilot SWE Agent, pelikhan) | ||
|
|
||
| --- | ||
|
|
||
| ### Context | ||
|
|
||
| The workflow compiler automatically rewrites deprecated `${{ needs.activation.outputs.{text,title,body} }}` expressions to their modern `${{ steps.sanitized.outputs.{text,title,body} }}` equivalents. However, the `detectTextOutputUsage` function — which decides whether the sanitized step must be injected into the compiled workflow — only scanned for the modern form. | ||
|
|
||
| Workflows that had not yet been migrated to the modern syntax would silently compile without the sanitized step. This caused runtime failures particularly for `workflow_dispatch`-only triggers, where no content context is provided and the missing step caused undefined outputs. | ||
|
|
||
| The fix must not break any already-migrated workflows, and must preserve the existing deprecation-warning pathway so users are still notified to migrate. | ||
|
|
||
| ### Decision | ||
|
|
||
| We will extend `detectTextOutputUsage` to also check for `${{ needs.activation.outputs.text }}`, `${{ needs.activation.outputs.title }}`, and `${{ needs.activation.outputs.body }}` alongside the already-supported modern forms, using short-circuit `if !hasXUsage` guards so that a single `strings.Contains` hit is enough to set the flag. The auto-rewrite that transforms deprecated to modern syntax at the expression level is left untouched. | ||
|
|
||
| ### Alternatives Considered | ||
|
|
||
| #### Alternative 1: Reject deprecated syntax with a compile-time error | ||
|
|
||
| Return a compile error (or emit a fatal diagnostic) whenever a deprecated `needs.activation.outputs.*` expression is found, requiring authors to migrate before the workflow compiles. | ||
|
|
||
| Rejected because it is a breaking change for all existing unmigrated workflows. The project's stated intent is a gradual migration; forcing an immediate hard break contradicts that policy and would block legitimate workflows from compiling at all. | ||
|
|
||
| #### Alternative 2: Emit a lint warning and rely on authors to migrate before runtime | ||
|
|
||
| Log a prominent warning, leave `detectTextOutputUsage` as-is (only scanning modern forms), and document that deprecated syntax may produce incorrect compiled output until migrated. | ||
|
|
||
| Rejected because it preserves the silent-failure bug that caused the issue in the first place. Users would see a warning but would still receive a broken compiled workflow, making this behaviour invisible until a runtime failure occurred. | ||
|
|
||
| ### Consequences | ||
|
|
||
| #### Positive | ||
| - Unmigrated workflows now compile correctly and include the sanitized step, eliminating the silent runtime failure. | ||
| - The fix is non-breaking: already-migrated workflows are unaffected because the `if !hasXUsage` guards skip the deprecated-form check once the modern form is found. | ||
| - Deprecation warnings (emitted to stderr by `ExpressionExtractor`) are preserved, continuing to guide authors toward the modern syntax. | ||
|
|
||
| #### Negative | ||
| - The deprecated `needs.activation.outputs.*` strings are now referenced in two places in the compiler: the auto-rewrite in `ExpressionExtractor` and the new detection guards in `detectTextOutputUsage`. This increases the surface area that must be updated when deprecated syntax support is eventually removed. | ||
| - Retaining dual-path detection extends the effective deprecation window, since unmigrated workflows no longer fail visibly and authors have less urgency to migrate. | ||
|
|
||
| #### Neutral | ||
| - Three new test cases were added to `TestDetectTextOutputUsage` and a new `TestExpressionExtractor_DeprecatedActivationOutputWarning` suite was introduced, increasing test coverage of the backwards-compatibility layer. | ||
| - Existing tests that used the deprecated form in `generateEnvVarName` and `NoCollisions` scenarios were updated to the modern form, reflecting that deprecated syntax is transformed before it reaches those code paths in production. | ||
|
|
||
| --- | ||
|
|
||
| *ADR created by [adr-writer agent]. Review and finalize before changing status from Draft to Accepted.* |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -243,9 +243,9 @@ func TestExpressionExtractor_GenerateEnvVarName(t *testing.T) { | |
| wantName: "GH_AW_GITHUB_EVENT_ISSUE_NUMBER", | ||
| }, | ||
| { | ||
| name: "needs output", | ||
| content: "needs.activation.outputs.text", | ||
| wantName: "GH_AW_NEEDS_ACTIVATION_OUTPUTS_TEXT", | ||
| name: "sanitized step outputs", | ||
| content: "steps.sanitized.outputs.text", | ||
| wantName: "GH_AW_STEPS_SANITIZED_OUTPUTS_TEXT", | ||
| }, | ||
| { | ||
| name: "complex expression with operators", | ||
|
|
@@ -345,7 +345,7 @@ func TestExpressionExtractor_NoCollisions(t *testing.T) { | |
| "github.actor", | ||
| "github.run_id", | ||
| "github.event.issue.number", | ||
| "needs.activation.outputs.text", | ||
| "steps.sanitized.outputs.text", | ||
| } | ||
|
|
||
| extractor := NewExpressionExtractor() | ||
|
|
@@ -511,6 +511,68 @@ Other: ${{ needs.activation.outputs.comment_id }} | |
| } | ||
| } | ||
|
|
||
| // TestExpressionExtractor_DeprecatedActivationOutputWarning verifies that extracting | ||
| // a deprecated needs.activation.outputs.* expression emits a deprecation warning to | ||
| // stderr while still producing the correct (transformed) mapping. | ||
| func TestExpressionExtractor_DeprecatedActivationOutputWarning(t *testing.T) { | ||
| tests := []struct { | ||
| name string | ||
| markdown string | ||
| deprecated string | ||
| modern string | ||
| }{ | ||
| { | ||
| name: "text output emits warning", | ||
| markdown: "Content: ${{ needs.activation.outputs.text }}", | ||
| deprecated: "needs.activation.outputs.text", | ||
| modern: "steps.sanitized.outputs.text", | ||
| }, | ||
| { | ||
| name: "title output emits warning", | ||
| markdown: "Title: ${{ needs.activation.outputs.title }}", | ||
| deprecated: "needs.activation.outputs.title", | ||
| modern: "steps.sanitized.outputs.title", | ||
| }, | ||
| { | ||
| name: "body output emits warning", | ||
| markdown: "Body: ${{ needs.activation.outputs.body }}", | ||
| deprecated: "needs.activation.outputs.body", | ||
| modern: "steps.sanitized.outputs.body", | ||
| }, | ||
| } | ||
|
|
||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| extractor := NewExpressionExtractor() | ||
|
|
||
| stderr := captureStderr(func() { | ||
| mappings, err := extractor.ExtractExpressions(tt.markdown) | ||
| if err != nil { | ||
| t.Fatalf("ExtractExpressions() unexpected error: %v", err) | ||
| } | ||
| // Verify the mapping was produced with the modern expression. | ||
| found := false | ||
| for _, m := range mappings { | ||
| if m.Content == tt.modern { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Missing 💡 Suggested fixAdd an for _, m := range mappings {
if m.Content == tt.modern {
found = true
// Also verify the env-var name reflects the modern (transformed) form,
// not the deprecated form. A buggy transformer could rewrite Content
// correctly but leave the old env-var name, causing a runtime miss.
wantEnvVar := "GH_AW_" + strings.ToUpper(
strings.NewReplacer(".", "_").Replace(tt.modern))
if m.EnvVar != wantEnvVar {
t.Errorf("expected EnvVar %q, got %q", wantEnvVar, m.EnvVar)
}
break
}
} |
||
| found = true | ||
| break | ||
| } | ||
| } | ||
| if !found { | ||
| t.Errorf("expected mapping for %q but not found", tt.modern) | ||
| } | ||
| }) | ||
|
Comment on lines
+548
to
+564
|
||
|
|
||
| if !strings.Contains(stderr, tt.deprecated) { | ||
| t.Errorf("expected deprecation warning mentioning %q in stderr, got: %q", tt.deprecated, stderr) | ||
| } | ||
| if !strings.Contains(stderr, tt.modern) { | ||
| t.Errorf("expected deprecation warning mentioning %q in stderr, got: %q", tt.modern, stderr) | ||
| } | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| func TestApplyWorkflowDispatchFallbacks(t *testing.T) { | ||
| tests := []struct { | ||
| name string | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removing the deprecated-form JS test creates an undocumented silent failure path. The PR's claim that "compiled templates never contain the deprecated form at runtime" is correct for newly compiled workflows, but
extractAndReplacePlaceholdersdoes mechanical dot→underscore conversion with no knowledge of the Go-side transformation — so the old placeholder__GH_AW_NEEDS_ACTIVATION_OUTPUTS_TEXT__can still be generated in edge cases, and that env var is never set.💡 Details
If a workflow YAML reaches the JS runtime with the deprecated expression still present (cached artifact, test fixture, direct compiler bypass),
extractAndReplacePlaceholdersproduces__GH_AW_NEEDS_ACTIVATION_OUTPUTS_TEXT__— an env var that is never populated — causing silently empty output.Consider either: