Migrate deprecated needs.activation.outputs references to steps.sanitized.outputs - #47264
Conversation
…ized.outputs
- Fix detectTextOutputUsage() to also detect deprecated needs.activation.outputs.{text,title,body}
so workflows not yet migrated still compile correctly when event context alone is insufficient
- Add TestDetectTextOutputUsage cases for deprecated form
- Add TestExpressionExtractor_DeprecatedActivationOutputWarning to verify the
deprecation warning is emitted (and the correct transformed mapping produced)
- Update TestExpressionExtractor_GenerateEnvVarName to use the modern
steps.sanitized.outputs.text form (matches what the pipeline actually passes)
- Update TestExpressionExtractor_NoCollisions to use steps.sanitized.outputs.text
- Update runtime_import.test.cjs tests for wrapExpressionsInTemplateConditionals
and extractAndReplacePlaceholders to use modern form (compiled templates never
contain the deprecated form at runtime)
- Add modern-form fuzz seeds alongside deprecated ones and clarify the purpose
of the deprecated seeds in both FuzzRenderExpressions and FuzzExtractExpressions
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Test Quality Sentinel completed test quality analysis. |
|
❌ Design Decision Gate 🏗️ failed during design decision gate check. |
There was a problem hiding this comment.
Pull request overview
Adds backward-compatible detection for deprecated activation outputs so sanitized text steps are included when required.
Changes:
- Detects deprecated text, title, and body output references.
- Updates Go, fuzz, and JavaScript tests to modern output syntax.
- Adds deprecation-warning coverage.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/compiler_orchestrator_tools.go |
Extends sanitized-output detection. |
pkg/workflow/compute_text_lazy_test.go |
Tests deprecated output detection. |
pkg/workflow/expression_extraction_test.go |
Updates mappings and tests warnings. |
pkg/workflow/expression_extraction_fuzz_test.go |
Clarifies compatibility seeds. |
actions/setup/js/runtime_import.test.cjs |
Uses modern runtime expressions. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 5/5 changed files
- Comments generated: 2
- Review effort level: Medium
| // Also recognise the deprecated needs.activation.outputs.* forms so that workflows | ||
| // using the old syntax still get the sanitized step included during compilation. | ||
| if !hasTextUsage { | ||
| hasTextUsage = strings.Contains(markdownContent, "${{ needs.activation.outputs.text }}") |
| 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 { | ||
| found = true | ||
| break | ||
| } | ||
| } | ||
| if !found { | ||
| t.Errorf("expected mapping for %q but not found", tt.modern) | ||
| } | ||
| }) |
![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 — changes look correct and well-tested.
📋 Key Themes & Highlights
Positive Highlights
- ✅ Root cause addressed:
detectTextOutputUsagenow covers both modern and deprecated expression forms - ✅ Three focused regression tests added to
compute_text_lazy_test.go - ✅
TestExpressionExtractor_DeprecatedActivationOutputWarningverifies the deprecation warning path end-to-end - ✅ Fuzz seeds annotated clearly to distinguish backwards-compat coverage from modern-form coverage
- ✅ JS runtime tests updated to reflect actual compiled output (deprecated form is never present at runtime)
One Minor Observation (non-blocking)
Both the modern-form and new deprecated-form detections use exact string match (strings.Contains), so expressions with fallbacks like ${{ needs.activation.outputs.text || 'default' }} would not be detected. This is consistent with the existing behaviour for the modern form and is acceptable for now, but worth a follow-up if fallback syntax is common in the wild.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 20.1 AIC · ⌖ 4.56 AIC · ⊞ 6.7K
Comment /matt to run again
There was a problem hiding this comment.
The changes are correct and well-structured.
- Bug fix in
detectTextOutputUsageis sound: the fallback checks forneeds.activation.outputs.{text,title,body}are guarded with!hasXUsageso the modern form takes precedence. - Deprecation warning path confirmed:
expression_extraction.goalready emits a warning to stderr for deprecated expressions, so the newTestExpressionExtractor_DeprecatedActivationOutputWarningtest exercises real code. captureStderris defined in the sameworkflowpackage so accessible fromexpression_extraction_test.go.- Runtime test updates correctly reflect that compiled templates only contain
steps.sanitized.outputs.*at runtime. - No security, correctness, or reliability issues found.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 24.2 AIC · ⌖ 5.15 AIC · ⊞ 5K
🧪 Test Quality Sentinel Report✅ Test Quality Score: 85/100 — Excellent
📊 Metrics (15 tests)
Verdict
Key Observations:
|
There was a problem hiding this comment.
Non-blocking observations on the deprecated-syntax backwards-compatibility fix.
### Findings
1. TestExpressionExtractor_DeprecatedActivationOutputWarning misses EnvVar assertion (medium)
The new test verifies m.Content == tt.modern but never checks m.EnvVar. A regression where the transformer rewrites Content correctly but keeps the old env-var name (GH_AW_NEEDS_ACTIVATION_OUTPUTS_TEXT) would pass undetected — and that bug would cause the compiled workflow to reference an unset env var at runtime.
2. JS test removal leaves deprecated-form behaviour at the JS layer undocumented (low)
The deleted tests were the only documentation that the JS runtime does not understand the deprecated form. The extractAndReplacePlaceholders function produces __GH_AW_NEEDS_ACTIVATION_OUTPUTS_TEXT__ for any deprecated expression it receives — an env var that is never set. The compiler is the intended gatekeeper, but there is no test or comment anchoring that assumption at the JS layer.
What's good
- The
detectTextOutputUsagefix is correct and the three newcompute_text_lazy_test.gocases directly cover the bug. transformActivationOutputsand the deprecation-warning logic inexpression_extraction.gopre-exist and work correctly — the new test confirms their behaviour.- Fuzz seeds for the deprecated form are retained, which is the right call.
🔎 Code quality review by PR Code Quality Reviewer · sonnet46 74.6 AIC · ⌖ 4.85 AIC · ⊞ 5.7K
Comment /review to run again
| // Verify the mapping was produced with the modern expression. | ||
| found := false | ||
| for _, m := range mappings { | ||
| if m.Content == tt.modern { |
There was a problem hiding this comment.
Missing EnvVar assertion leaves deprecated-path env-var name generation untested. If the transformation maps needs.activation.outputs.text to the correct Content but produces the wrong EnvVar (e.g., still emitting GH_AW_NEEDS_ACTIVATION_OUTPUTS_TEXT instead of GH_AW_STEPS_SANITIZED_OUTPUTS_TEXT), this test would still pass.
💡 Suggested fix
Add an EnvVar assertion inside the found-mapping check to close this gap:
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
}
}| it("should produce {{#if }} (falsy) for needs.activation.outputs.text when env var is absent", () => { | ||
| expect(wrapExpressionsInTemplateConditionals("{{#if needs.activation.outputs.text}}body{{/if}}")).toBe("{{#if }}body{{/if}}"); | ||
| it("should produce {{#if }} (falsy) for steps.sanitized.outputs.text when env var is absent", () => { | ||
| expect(wrapExpressionsInTemplateConditionals("{{#if steps.sanitized.outputs.text}}body{{/if}}")).toBe("{{#if }}body{{/if}}"); |
There was a problem hiding this comment.
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 extractAndReplacePlaceholders does 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), extractAndReplacePlaceholders produces __GH_AW_NEEDS_ACTIVATION_OUTPUTS_TEXT__ — an env var that is never populated — causing silently empty output.
Consider either:
- Retaining the old test cases as comments that document the intentional behavior gap, or
- Adding a dedicated test asserting what happens when the deprecated form hits the JS layer, with a comment that the compiler is the single source of truth for preventing this.
… outputs detection
|
🎉 This pull request is included in a new release. Release: |
The deprecated
needs.activation.outputs.{text,title,body}syntax accumulated silently: the compiler auto-rewrote it butdetectTextOutputUsageonly scanned for the modern form, meaning workflows using the old syntax could compile without the sanitized step when the event trigger didn't provide content context (e.g.workflow_dispatch-only triggers).Changes
Bug fix
compiler_orchestrator_tools.go—detectTextOutputUsagenow also matches${{ needs.activation.outputs.{text,title,body} }}so the sanitized step is always included for workflows not yet migratedTests
compute_text_lazy_test.go— three newTestDetectTextOutputUsagecases covering the deprecated formexpression_extraction_test.go— newTestExpressionExtractor_DeprecatedActivationOutputWarningverifies the deprecation warning is emitted to stderr and the correct transformed mapping is produced; updatedGenerateEnvVarNameandNoCollisionstests to use the modern form (the deprecated form never reachesgenerateEnvVarNamein the live pipeline)runtime_import.test.cjs— two tests updated to usesteps.sanitized.outputs.text— compiled templates never contain the deprecated form at runtime, so the previous test inputs were unreachable in practice:expression_extraction_fuzz_test.go— modern-form seeds added to both fuzz targets; deprecated seeds retained and annotated as backwards-compatibility transformer coverage