Warn on deprecated needs.activation.outputs.* with gh aw fix hint and track in warning count#43964
needs.activation.outputs.* with gh aw fix hint and track in warning count#43964Conversation
…tion and count tracking Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
needs.activation.outputs.* with gh aw fix hint and track in warning count
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Test Quality Sentinel completed test quality analysis. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
There was a problem hiding this comment.
Pull request overview
Improves the workflow compiler’s diagnostics for deprecated needs.activation.outputs.{text,title,body} expressions by making the warning actionable (includes a gh aw fix hint) and ensuring these deprecations contribute to the compiler’s warning summary count.
Changes:
- Expanded the deprecation warning message to include the exact replacement expression and a
gh aw fixmigration hint. - Added deprecation-warning counting to
ExpressionExtractorand propagated that count into the compiler warning counter during prompt generation. - Added tests that validate warning emission/counting behavior and warning text content.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/expression_extraction.go | Tracks deprecated activation-output rewrites, emits stronger warnings, and exposes a deprecation warning count. |
| pkg/workflow/expression_extraction_test.go | Adds test coverage for deprecation warning emission, deduping, and message content. |
| pkg/workflow/compiler_yaml.go | Propagates extractor deprecation warning counts into the compiler’s warning summary and updates extraction helper signature. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 3/3 changed files
- Comments generated: 2
- Review effort level: Low
| // Capture stderr to check for warnings | ||
| oldStderr := os.Stderr | ||
| r, w, pipeErr := os.Pipe() | ||
| if pipeErr != nil { | ||
| t.Fatalf("os.Pipe() error = %v", pipeErr) | ||
| } | ||
| os.Stderr = w | ||
|
|
||
| extractor := NewExpressionExtractor() | ||
| _, err := extractor.ExtractExpressions(tt.markdown) | ||
|
|
||
| w.Close() | ||
| os.Stderr = oldStderr | ||
| var buf bytes.Buffer | ||
| if _, copyErr := io.Copy(&buf, r); copyErr != nil { | ||
| t.Fatalf("io.Copy() error = %v", copyErr) | ||
| } |
| // processMarkdownBody applies the standard post-processing pipeline to a markdown body: | ||
| // XML comment removal, expression wrapping, expression extraction/substitution, and chunking. | ||
| // It returns the prompt chunks and expression mappings extracted from the content. | ||
| func extractPromptChunksFromMarkdown(body string) ([]string, []*ExpressionMapping) { | ||
| // It returns the prompt chunks, expression mappings, and the number of deprecation warnings | ||
| // emitted for deprecated activation-output expressions. |
🧪 Test Quality Sentinel Report✅ Test Quality Score: 90/100 — Excellent
📊 Metrics (1 test)
|
…ng propagation Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (145 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 Matter
ADRs 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
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review summary
The PR is well-scoped and does exactly what it advertises: improves the deprecation warning message with the gh aw fix migration hint and propagates the deprecation count into the compiler's warning summary. The logic is correct, and deduplication (via the existing mappings map keyed on originalExpr) prevents double-counting.
Three non-blocking issues noted:
-
Whitespace edge case in deduplication (
expression_extraction.goline 135) — two tokens for the same deprecated expression that differ only in whitespace produce differentoriginalExprkeys, so each emits its own warning and increments the counter. Probably not reachable in practice (the regex normalizes whitespace), but worth a comment. -
for range N { IncrementWarningCount() }verbosity (compiler_yaml.go, repeated 4×) — this is functionally equivalent to a singlewarningCount += N. Adding anAddWarningCount(n int)helper toCompilerwould clean this up. -
os.Stderrglobal mutation in tests (expression_extraction_test.goline 580) — swapping the globalos.Stderrwith a pipe is not goroutine-safe under-raceor ift.Parallel()is added later. Injecting anio.WriterintoExpressionExtractor(constructor option or functional option) would let tests pass abytes.Bufferdirectly.
All three are suggestions/improvements, not blockers. The feature logic and test coverage are solid.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 67.9 AIC · ⌖ 5.4 AIC · ⊞ 4.8K
| fmt.Sprintf("Deprecated expression ${{ %s }}: use ${{ %s }} instead.", originalContent, content), | ||
| fmt.Sprintf("Deprecated expression ${{ %s }}: use ${{ %s }} instead. Run `gh aw fix` to automatically update this workflow.", originalContent, content), | ||
| )) | ||
| e.deprecationWarningCount++ |
There was a problem hiding this comment.
Minor: deduplication relies on originalExpr string equality — whitespace variants could slip through
The early-return guard at line 126 uses originalExpr (the full ${{ ... }} token string) as the deduplication key. The deprecation counter is only incremented for unseen expressions, which is correct.
However, if two tokens for the same logical expression differ in internal whitespace (e.g. ${{ needs.activation.outputs.text }} vs ${{needs.activation.outputs.text }}), they produce different originalExpr keys, each triggering its own warning and counter increment even though they refer to the same deprecated output. A brief comment here noting that deduplication is keyed on the raw token (not the transformed content) would clarify the assumption.
@copilot please address this.
| userPromptChunks = append(userPromptChunks, chunks...) | ||
| expressionMappings = append(expressionMappings, exprMaps...) | ||
| for range deprecationCount { | ||
| c.IncrementWarningCount() |
There was a problem hiding this comment.
Style: for range N { c.IncrementWarningCount() } can be simplified to c.AddWarningCount(deprecationCount) if that helper exists, or c.warningCount += deprecationCount
This pattern is repeated 4 times in this file. A single c.IncrementWarningCount() call increments by 1; calling it inside a loop to add N is equivalent to c.warningCount += deprecationCount. If Compiler ever gets an AddWarningCount(n int) helper, or if warningCount is directly accessible, this becomes cleaner. As-is it's correct but verbose — consider extracting a helper or using a single arithmetic increment.
@copilot please address this.
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| // Capture stderr to check for warnings | ||
| oldStderr := os.Stderr |
There was a problem hiding this comment.
Reliability concern: swapping os.Stderr in tests is not goroutine-safe
os.Stderr is a process-global variable. Replacing it with os.Pipe() in a test and restoring it with defer (or manually) will race if the test suite ever runs with -parallel or if t.Parallel() is added later. Any concurrent test or background goroutine that writes to os.Stderr during this window will either be captured in the pipe buffer or write to the wrong descriptor.
Consider injecting a io.Writer for the warning output into ExpressionExtractor (e.g. WithStderr(w io.Writer) or a constructor option), so tests can pass a bytes.Buffer directly without mutating global state.
@copilot please address this.
There was a problem hiding this comment.
REQUEST_CHANGES — 3 blocking issues, 1 non-blocking
Summary: The feature logic is correct and the diagnostic improvement is valuable. Three issues need addressing before merge: a test-reliability bug from missing defer on os.Stderr restoration, stale deprecation count propagated when extraction errors, and a structural design issue (hardcoded os.Stderr) that the new test inadvertently exposes by requiring an os.Pipe workaround.
🔍 Findings detail
Blocking
-
os.Stderrnot restored viadeferin tests (expression_extraction_test.go:585): Any panic or futuret.Fatalinside the test body (between assignment and restore) leavesos.Stderrpointing at a closed pipe for the rest of the test binary. The fix is at.Cleanupcall immediately afteros.Stderr = w. See inline comment. -
Deprecation count propagated on extraction error (
compiler_yaml.go:1130): IfExtractExpressionsfails mid-parse,exprMappingsis discarded butGetDeprecationWarningCount()still returns the partial count, which gets added to the compiler's warning summary. Return0on the error path. See inline comment. -
os.Stderrhardcoded inExpressionExtractor(expression_extraction.go:132): Forces theos.Pipeglobal-mutation hack in tests, making tests fragile and the type non-reusable outside a CLI. An injectableio.Writerfield (defaulting toos.Stderr) would fix both the production design and the test issue simultaneously. See inline comment.
Non-blocking (should fix)
for range N { IncrementWarningCount() }is O(N) arithmetic (compiler_yaml.go:535, same pattern 6×): AddAddWarningCount(n int)toCompiler. See inline comment.
🔎 Code quality review by PR Code Quality Reviewer · 94.8 AIC · ⌖ 8.04 AIC · ⊞ 5.4K
Comment /review to run again
| if pipeErr != nil { | ||
| t.Fatalf("os.Pipe() error = %v", pipeErr) | ||
| } | ||
| os.Stderr = w |
There was a problem hiding this comment.
os.Stderr is not restored via defer — a panic inside ExtractExpressions permanently corrupts the global stderr handle for the rest of the test binary.
💡 Suggested fix
Replace the inline restore pattern with t.Cleanup so cleanup is guaranteed even on panic or t.Fatal:
oldStderr := os.Stderr
r, w, pipeErr := os.Pipe()
if pipeErr != nil {
t.Fatalf("os.Pipe() error = %v", pipeErr)
}
os.Stderr = w
t.Cleanup(func() {
w.Close()
os.Stderr = oldStderr
})
extractor := NewExpressionExtractor()
_, err := extractor.ExtractExpressions(tt.markdown)The current pattern (os.Stderr = w; <work>; w.Close(); os.Stderr = oldStderr) is not panic-safe. If ExtractExpressions ever panics or a future contributor adds a t.Fatal between the assignment and the restore, os.Stderr is left pointing at a closed write-end pipe. Every subsequent test in the binary then writes into a broken pipe — output is silently lost and the file descriptor leaks. Using t.Cleanup is the standard Go idiom to prevent this.
| exprMappings = nil | ||
| } | ||
| return splitContentIntoChunks(body), exprMappings | ||
| return splitContentIntoChunks(body), exprMappings, extractor.GetDeprecationWarningCount() |
There was a problem hiding this comment.
Deprecation warning count is propagated even when ExtractExpressions fails mid-parse, double-counting warnings against a compile run that encountered an error.
💡 Suggested fix
func extractPromptChunksFromMarkdown(body string) ([]string, []*ExpressionMapping, int) {
body = removeXMLComments(body)
body = wrapExpressionsInTemplateConditionals(body)
extractor := NewExpressionExtractor()
exprMappings, err := extractor.ExtractExpressions(body)
if err != nil {
exprMappings = nil
// Do not propagate deprecation count from a failed extraction;
// stderr warnings may have been partially emitted, but the count
// should not inflate the compiler's summary.
return splitContentIntoChunks(body), nil, 0
}
if len(exprMappings) == 0 {
exprMappings = nil
}
return splitContentIntoChunks(body), exprMappings, extractor.GetDeprecationWarningCount()
}Currently, if ExtractExpressions returns an error (expressions were partially processed), exprMappings is discarded, but GetDeprecationWarningCount() still returns however many warnings were emitted before the error. The caller increments the compiler's warning count for those, and the warning messages have already been printed to stderr, so they appear in output but may refer to expressions that weren't fully handled. The correct behavior on extraction failure is to return 0 deprecation warnings so the summary counter stays consistent with the actual compilation state.
| chunks, exprMaps, deprecationCount := extractPromptChunksFromMarkdown(cleaned) | ||
| userPromptChunks = append(userPromptChunks, chunks...) | ||
| expressionMappings = append(expressionMappings, exprMaps...) | ||
| for range deprecationCount { |
There was a problem hiding this comment.
for range deprecationCount is used as arithmetic — an O(N) loop calling IncrementWarningCount() N times when a single addition would do. This pattern is duplicated 6 times in the diff.
💡 Suggested fix
Add an AddWarningCount(n int) helper to Compiler and use it:
// in compiler_types.go
func (c *Compiler) AddWarningCount(n int) {
c.warningCount += n
}Then replace all six occurrences:
// before
for range deprecationCount {
c.IncrementWarningCount()
}
// after
c.AddWarningCount(deprecationCount)The for range <int> idiom (Go 1.22+) is readable for iteration, but using it to accumulate a counter inverts its semantics — it looks like each iteration does meaningful work when it doesn't. With 6 identical patterns in the same function, this warrants a helper. If a workflow triggers 1000 deprecation warnings, the current code spins 6000 no-op iterations just to compute a sum.
| @@ -122,8 +130,9 @@ func (e *ExpressionExtractor) processMatch(originalExpr, rawContent string) { | |||
| // Emit deprecation warning once per unique deprecated activation-output expression | |||
| if content != originalContent && strings.HasPrefix(content, "steps.sanitized.outputs.") { | |||
| fmt.Fprintln(os.Stderr, console.FormatWarningMessage( | |||
There was a problem hiding this comment.
ExpressionExtractor writes directly to os.Stderr — a hidden side-effect that makes the type untestable without an os.Pipe hack and unusable in non-CLI contexts.
💡 Suggested fix
Inject a writer via the extractor or return warnings as values:
type ExpressionExtractor struct {
mappings map[string]*ExpressionMapping
counter int
deprecationWarningCount int
stderr io.Writer // injected; defaults to os.Stderr
}
func NewExpressionExtractor() *ExpressionExtractor {
return &ExpressionExtractor{
mappings: make(map[string]*ExpressionMapping),
stderr: os.Stderr,
}
}Then in processMatch:
fmt.Fprintln(e.stderr, console.FormatWarningMessage(...))In tests, inject bytes.NewBuffer(nil) instead of using os.Pipe. This removes the need to mutate a process-global variable in tests, eliminates the leak risk entirely, and makes the type usable in library contexts (Wasm, embedded, server) where writing to os.Stderr is inappropriate. The new deprecationWarningCount field already exposes the count — adding an injectable writer completes the decoupling.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd, /diagnosing-bugs, and /codebase-design — requesting changes on three targeted issues.
📋 Key Themes & Highlights
Issues to Address
-
os.Stderrglobal mutation in tests (expression_extraction_test.go:585) — not goroutine-safe; will race under-race; restore is also not panic-safe (nodefer). Ideal fix: inject anio.WriterintoExpressionExtractor. -
for range N { IncrementWarningCount() }repeated 6 times (compiler_yaml.go) — adding anAddWarningCount(n int)helper would replace these loops with a single, intent-revealing call each. -
Deprecation predicate infers from transformed output rather than the original key (
expression_extraction.go:131) — future transformers that also emitsteps.sanitized.outputs.*would silently trigger false deprecation warnings. Checking against the known set of deprecated originals is safer.
Positive Highlights
- ✅ Excellent test coverage: 6 cases covering deduplication, non-deprecated cases, and exact replacement text matching.
- ✅
GetDeprecationWarningCount()accessor is clean; count propagation to the compiler summary is correctly wired at all call sites. - ✅ Warning message is now genuinely actionable — the
gh aw fixhint is exactly what users need.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 84.8 AIC · ⌖ 8.23 AIC · ⊞ 6.6K
Comment /matt to run again
| if pipeErr != nil { | ||
| t.Fatalf("os.Pipe() error = %v", pipeErr) | ||
| } | ||
| os.Stderr = w |
There was a problem hiding this comment.
[/tdd] os.Stderr is a global; mutating it in a subtest is not goroutine-safe and will race if the test suite (or any sibling tests) ever runs with -race or t.Parallel().
💡 Safer approach: redirect at the consumer, not the global
Instead of swapping os.Stderr, thread an io.Writer through ExpressionExtractor (e.g. extractor.SetOutput(w)) or use console.SetOutput. That keeps tests hermetic and eliminates the data race.
If a writer field is not available today, at minimum add a defer func() { os.Stderr = oldStderr }() guard so the restore is panic-safe:
oldStderr := os.Stderr
r, w, pipeErr := os.Pipe()
if pipeErr != nil {
t.Fatalf("os.Pipe() error = %v", pipeErr)
}
os.Stderr = w
defer func() { os.Stderr = oldStderr }()@copilot please address this.
| chunks, exprMaps, deprecationCount := extractPromptChunksFromMarkdown(cleaned) | ||
| userPromptChunks = append(userPromptChunks, chunks...) | ||
| expressionMappings = append(expressionMappings, exprMaps...) | ||
| for range deprecationCount { |
There was a problem hiding this comment.
[/codebase-design] for range deprecationCount { c.IncrementWarningCount() } loops N times to add N to a counter — this is semantically c.AddWarningCount(n) but there is no such method. This pattern appears 6 times and obscures intent.
💡 Add an `IncrementWarningCountBy(n int)` or `AddWarningCount(n int)` helper
In compiler_types.go, alongside the existing IncrementWarningCount:
func (c *Compiler) AddWarningCount(n int) {
c.warningCount += n
}Then all six call sites become a single clear line:
c.AddWarningCount(deprecationCount)This is shorter, communicates the intent, and avoids the for range int Go 1.22+ idiom which surprises readers unfamiliar with that version.
@copilot please address this.
| @@ -122,8 +130,9 @@ func (e *ExpressionExtractor) processMatch(originalExpr, rawContent string) { | |||
| // Emit deprecation warning once per unique deprecated activation-output expression | |||
| if content != originalContent && strings.HasPrefix(content, "steps.sanitized.outputs.") { | |||
There was a problem hiding this comment.
[/diagnosing-bugs] The deprecation check fires only when content != originalContent && strings.HasPrefix(content, "steps.sanitized.outputs."). If a future transformer also produces a steps.sanitized.outputs.* value via a different path, it would silently get counted as a deprecation warning — a false positive with no test coverage guarding against it.
💡 Guard against false positives with a dedicated predicate
Introduce an explicit function that checks whether the original content (before any transformation) is a known deprecated activation output, rather than inferring it from the transformed result:
func isDeprecatedActivationOutput(original string) bool {
switch original {
case "needs.activation.outputs.text",
"needs.activation.outputs.title",
"needs.activation.outputs.body":
return true
}
return false
}Then in processMatch:
if isDeprecatedActivationOutput(originalContent) {
// emit warning and increment count
}This makes the intent explicit and ensures new transformers do not accidentally trigger deprecation warnings.
@copilot please address this.
| }, | ||
| }, | ||
| { | ||
| name: "duplicate deprecated expression emits only one warning", |
There was a problem hiding this comment.
[/tdd] The "duplicate deprecated expression" test case has no wantWarningPhrases — it only asserts the count is 1 but does not verify that one warning was actually emitted to stderr. Without asserting the warning text, a silent counter-only increment would pass this test.
💡 Add a stderr phrase assertion to the duplicate case
{
name: "duplicate deprecated expression emits only one warning",
markdown: `${{ needs.activation.outputs.text }} ${{ needs.activation.outputs.text }}`,
wantDeprecationCount: 1,
wantWarningPhrases: []string{
"needs.activation.outputs.text",
"gh aw fix",
},
},This ensures the count and the side-effect (stderr output) are both tested together.
@copilot please address this.
Authors continue using deprecated
needs.activation.outputs.{text,title,body}expressions because the compiler silently rewrites them — the existing warning lacked actionability and wasn't reflected in the compile warning summary.Changes
Stronger diagnostic: Warning message now includes the exact
gh aw fixmigration hint:Warning count propagation (
expression_extraction.go,compiler_yaml.go): AddeddeprecationWarningCounttoExpressionExtractor(exposed viaGetDeprecationWarningCount()). All fourExtractExpressionscall sites in the compiler now propagate this count toc.IncrementWarningCount()so deprecation warnings appear in compile summary statistics.Tests (
expression_extraction_test.go): NewTestExpressionExtractor_DeprecationWarningcovering: single/all-three deprecated expressions emit warnings; duplicates emit only one; non-deprecated activation outputs and unrelated expressions emit nothing; warning text includes both the exact deprecated form and replacement.