feat: error when agent-job step outputs are referenced in the prompt body - #48610
Conversation
Add a new compile-time validation that errors when a workflow's markdown
body (the agent prompt) references ${{ steps.STEP_ID.* }} outputs from
steps defined in the frontmatter steps:/pre-steps:/pre-agent-steps:/
post-steps: sections.
The prompt is rendered in the activation job; those steps execute in the
agent job (after activation), so their outputs are unavailable when the
prompt is assembled. The correct pattern is to write results to files
under /tmp/gh-aw/agent/ and reference those file paths in the prompt.
Built-in activation-job steps (e.g. steps.sanitized.outputs.text) are
not flagged because they are not declared in the user-visible step lists.
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Adds compile-time validation to prevent prompts from referencing outputs of later-running agent-job steps.
Changes:
- Detects agent-job step references in prompt expressions.
- Integrates validation into expression checks.
- Adds unit and compilation-pipeline tests.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/steps_output_in_prompt_validation.go |
Implements step-reference validation. |
pkg/workflow/steps_output_in_prompt_validation_test.go |
Tests validation and compiler integration. |
pkg/workflow/compiler_validators.go |
Wires validation into compilation. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comments suppressed due to low confidence (3)
pkg/workflow/steps_output_in_prompt_validation.go:36
- The fast path is stricter than the regex and skips valid expressions with more than one space after
{{, for example${{ steps.compute.outputs.value }}. That expression passes the existing expression parser but this validator returns before checking it. Use the compiled matcher for this precheck.
if !strings.Contains(workflowData.MarkdownContent, "${{ steps.") {
pkg/workflow/steps_output_in_prompt_validation.go:55
- Step IDs are job-scoped, so membership in the agent-job ID set does not prove this reference resolves to that step. For example, a custom agent step may legally use
id: sanitized, while${{ steps.sanitized.outputs.text }}in the prompt resolves to the activation step defined incompiler_activation_steps.go:255; this check would reject that valid built-in reference. Activation-job IDs need to take precedence when classifying prompt references.
if _, isAgentStep := agentStepIDs[stepID]; isAgentStep {
pkg/workflow/steps_output_in_prompt_validation.go:78
- This recovery guidance is incorrect for IDs from
post-steps: those steps run after the agent itself, so writing their result to a file still cannot make it available when the prompt is processed. The message should tell users to move any prompt input computation before the agent, then write it to a file.
"Write step results to a file (e.g. /tmp/gh-aw/agent/result.txt) and reference that file path in the prompt instead of using ${{ steps.STEP_ID.outputs.* }}.",
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Medium
|
|
||
| // stepsRefInPromptPattern matches ${{ steps.STEP_ID ... }} occurrences and | ||
| // captures the step ID so it can be compared against the agent-job step list. | ||
| var stepsRefInPromptPattern = regexp.MustCompile(`\$\{\{\s*steps\.([a-zA-Z0-9_-]+)`) |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
|
✅ Test Quality Sentinel completed test quality analysis. |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
🧠 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.
Review: feat: error when agent-job step outputs are referenced in the prompt body
The implementation is clean and well-tested. The only material issue is already captured in the existing inline comment at line 22:
Regex captures only the first steps.* reference per expression. In an expression like ${{ steps.sanitized.outputs.text || steps.compute.outputs.value }}, only sanitized is captured. If compute is an agent-job step, the validation silently misses it.
No other blocking issues found.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 14.6 AIC · ⌖ 5.63 AIC · ⊞ 5K
🧪 Test Quality Sentinel Report✅ Test Quality Score: 100/100 — Excellent
📊 Metrics (3 tests)
Verdict
|
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (398 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
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd — requesting changes for a missing test coverage gap.
📋 Key Themes & Highlights
Key Themes
- Missing test case:
preAgentSteps(thepre-agent-steps:section) is wired intoextractAgentJobStepIDsbut never exercised by any test — a regression dropping that section would go undetected. - Minor clarity opportunity: the fast-path early-exit at line 56 is correct but benefits from a one-line comment.
Positive Highlights
- ✅ Excellent regex design: captures step ID without requiring the full
.outputs.suffix, so it catches anysteps.IDreference regardless of property. - ✅ Good false-positive handling: built-in activation-job steps (
sanitized, etc.) are excluded because they never appear in user-visible step lists. - ✅ Thorough integration test (
TestValidateStepsOutputsNotInPromptViaCompiler) that validates the validator is actually wired into the compilation pipeline. - ✅ Actionable error message with a concrete file-based alternative — a great example of user-facing error design.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 35.8 AIC · ⌖ 4.86 AIC · ⊞ 6.7K
Comment /matt to run again
|
|
||
| // TestValidateStepsOutputsNotInPromptViaCompiler verifies that the validation | ||
| // is wired into the compilation pipeline and produces a compile error when a | ||
| // step output is referenced in the prompt body. |
There was a problem hiding this comment.
[/tdd] preAgentSteps is never exercised — steps defined under pre-agent-steps: won't be caught if they are accidentally dropped from extractAgentJobStepIDs.
💡 Suggested addition
Add preAgentSteps to the IDs collected from multiple step sections case (or a dedicated subtest):
preAgentSteps: `pre-agent-steps:
- name: Pre-agent
id: pre_agent_step
run: echo pre-agent
`,
expectedIDs: []string{"main_step", "pre_step", "post_step", "pre_agent_step"},Without this a regression that drops PreAgentSteps from extractAgentJobStepIDs would pass undetected.
@copilot please address this.
| } | ||
| stepID := match[1] | ||
| if _, isAgentStep := agentStepIDs[stepID]; isAgentStep { | ||
| if _, already := seen[stepID]; !already { |
There was a problem hiding this comment.
[/tdd] The early-exit fast path (!strings.Contains(..., "${{ steps."))) skips the check when no steps. expression appears in the prompt — but this means a workflow with an expression like ${{ needs.job.outputs.x }} and no steps string will silently pass even if step IDs exist in other sections. This is fine by design, but worth adding a comment so future readers don't second-guess the correctness.
💡 Suggestion
Add a short inline comment on that line explaining the intent:
// Fast path: no steps reference in prompt means nothing to validate.
if !strings.Contains(workflowData.MarkdownContent, "${{ steps.") {
return nil
}@copilot please address this.
There was a problem hiding this comment.
Verdict: Request Changes
Good feature intent, but the fast-path guard is inconsistent with the regex it gates, causing a false negative for a common valid syntax variant — reintroducing exactly the bug class this PR aims to prevent.
💡 Themes
- Blocking:
strings.Contains(..., "${{ steps.")requires exactly one space; the regex allows zero-or-more (\s*). Confirmed locally that${{steps.x.outputs.y}}(no space) bypasses the guard entirely and is never checked. - Non-blocking but worth considering: the regex scans raw markdown including fenced code blocks/inline code, so documentation-style examples of the anti-pattern would trigger false-positive compile errors with no escape hatch.
- Test suite is decent but doesn't cover the no-space syntax variant, code-fence content, or
||compound expressions — these gaps let the guard bug slip through.
Once the guard is aligned with the regex (or removed in favor of a single MatchString check), this looks solid.
🔎 Code quality review by PR Code Quality Reviewer · sonnet50 · 62.4 AIC · ⌖ 4.46 AIC · ⊞ 7.4K
Comment /review to run again
| if workflowData == nil || workflowData.MarkdownContent == "" { | ||
| return nil | ||
| } | ||
| if !strings.Contains(workflowData.MarkdownContent, "${{ steps.") { |
There was a problem hiding this comment.
This fast-path guard has a false-negative gap: it only matches a literal single space after ${{, but the regex it gates allows zero or more spaces (\s*), so ${{steps.foo.outputs.bar}} (no space — valid GitHub Actions syntax) silently bypasses this validator entirely.
💡 Details and fix
Verified locally: strings.Contains("${{steps.compute.outputs.value}}", "${{ steps.") returns false, while stepsRefInPromptPattern matches that same string fine (\s* allows zero spaces). This defeats the entire purpose of the validator for a common, perfectly valid expression form — the exact case this PR is trying to catch would slip through undetected if written without the space.
Fix: make the guard consistent with the regex, e.g. just check for "steps." substring, or better, drop the fast-path Contains check and rely solely on the regex (it's cheap enough), or use stepsRefInPromptPattern.MatchString(...) directly as the guard:
if !stepsRefInPromptPattern.MatchString(workflowData.MarkdownContent) {
return nil
}This also removes the maintenance burden of keeping two independent patterns in sync.
| return nil | ||
| } | ||
|
|
||
| matches := stepsRefInPromptPattern.FindAllStringSubmatch(workflowData.MarkdownContent, -1) |
There was a problem hiding this comment.
The regex scans the raw MarkdownContent, including fenced code blocks and inline code spans, so a workflow prompt that merely documents/demonstrates the anti-pattern (e.g. "don't write ${{ steps.compute.outputs.value }} in your prompt") will trip this validator as a hard compile error.
|
🎉 Great work on this compile-time validation improvement! This PR adds a crucial safeguard that catches a common footgun: authors trying to reference agent-job step outputs in prompts, which silently resolve to empty due to gh-aw's two-job architecture. What I'm seeing:
This is ready for review and merge. Well done!
|
|
@copilot run pr-finisher skill |
Use a two-step approach: extract ${{ ... }} expression bodies first,
then find all steps.STEP_ID occurrences within each body. Previously the
regex anchored to ${{ so only the first operand in expressions like
${{ steps.a.outputs.x || steps.b.outputs.y }} was checked.
Add test case covering the multi-operand expression scenario.
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Fixed in the latest commit. The regex now uses a two-step approach: first extract each |
The prompt is rendered in the activation job;
steps:/pre-steps:/pre-agent-steps:/post-steps:run in the agent job (after activation). Referencing their outputs in the prompt body silently resolves to empty. Steps should write results to files instead.Changes
steps_output_in_prompt_validation.go— new validator called fromvalidateExpressions:idvalues from all four agent-job step YAML sectionsMarkdownContentfor${{ steps.STEP_ID.* }}whereSTEP_IDmatches a collected agent-job step IDcompiler_validators.go— wiresvalidateStepsOutputsNotInPromptinto the existingvalidateExpressionsgatesteps_output_in_prompt_validation_test.go— unit tests + full compilation pipeline testsExample
Built-in activation-job steps (
steps.sanitized.outputs.textetc.) are not flagged — they don't appear in user-visible step lists and are valid in the prompt.