perf: fix +320% regression in CompileComplexWorkflow by eliminating redundant yaml.Unmarshal#40662
Conversation
…formed workflows Replace broad hasAnyExpressionInRunContent check with two targeted text scans: 1. hasExpressionInRunContent(UnsafeContextPattern) - detects template injection risk 2. hasNonAllowedExpressionInRunContent - detects compiler regression cases For well-formed workflows (the common case), both scans return false so yaml.Unmarshal is skipped entirely. This fixes the +320% regression in BenchmarkCompileComplexWorkflow. Benchmark result: ~1.64ms/op (was ~6.8ms baseline; target ~3ms historical) Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Test Quality Sentinel completed test quality analysis. |
There was a problem hiding this comment.
Pull request overview
Improves gh aw workflow compilation performance by preventing unnecessary YAML parsing during template-injection and expression-guardrail validation when schema validation is skipped (the common/default path), while preserving the existing security/regression checks when risk indicators are present.
Changes:
- Replaced the broad “any
${{ }}in run blocks” pre-check with two targeted text scans to decide whetheryaml.Unmarshalis necessary. - Added
hasNonAllowedExpressionInRunContentto quickly detect run-block expressions that are not in the compiler-owned allow-list. - Added focused unit tests for the new scan behavior.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/template_injection_validation.go |
Adds a targeted run-block scan to ignore compiler-allowed expressions and reduce unnecessary YAML parsing. |
pkg/workflow/template_injection_validation_test.go |
Adds unit tests covering allowed vs disallowed expressions in run blocks (inline + multiline). |
pkg/workflow/compiler.go |
Updates validateTemplateInjection Path B to use targeted scans and conditionally run the parsed validators. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 3/3 changed files
- Comments generated: 0
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (208 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 MatterADRs create a searchable, permanent record of why the codebase looks the way it does. This change makes 📋 Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in
|
🧪 Test Quality Sentinel Report✅ Test Quality Score: 100/100 — Excellent
📊 Metrics & Test Classification (1 test analyzed)
Go: 1 ( Verdict
|
|
@copilot run pr-finisher skill |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Request Changes: validation guardrail regression
The performance win is real and the approach (two targeted scans to avoid yaml.Unmarshal) is sound in principle, but the new pre-check introduces a correctness regression that must be addressed before merge.
Blocking issues
Behavioral regression (compiler.go:344) — Replacing hasAnyExpressionInRunContent (trigger on any expression in any detected run block) with hasNonAllowedExpressionInRunContent (trigger only on disallowed expressions in detected run blocks) changes what causes yaml.Unmarshal to fire. In the old code, an allowed expression like ${{ runner.temp }} in a block-style step was enough to trigger full parsed validation, which then caught disallowed expressions in any run block, including ones the text scanner never detected. In the new code, if every text-detected run block contains only allowed expressions, yaml.Unmarshal is skipped entirely—and disallowed expressions in flow-style or quoted-key steps silently pass. See comment on line 344 for a concrete scenario and a suggested fix.
False negatives in the pre-check (template_injection_validation.go:110) — strings.HasPrefix(keyPart, "run:") misses flow-style mappings (- { run: "..." }) and quoted keys ("run":). The parsed validators handle these correctly; the text scanner does not. This is both the root cause of the regression above and a standalone gap in the scanner's coverage.
Non-blocking concerns
- Code duplication (template_injection_validation.go:72) — The ~45-line run-block detection body is copy-pasted between the two scanners. Extracting a shared
walkRunBlockLinescallback would eliminate the divergence risk and make the flow-style fix a one-line change. - Test coverage gap (template_injection_validation_test.go:1442) — No test covers the mixed scenario (allowed expression in block-style step + disallowed expression in flow-style step). Adding it would immediately surface the regression.
🔎 Code quality review by PR Code Quality Reviewer · 126.8 AIC · ⌖ 7.75 AIC · ⊞ 5.1K
| // appears in run: blocks | ||
| // → triggers validateNoGitHubExpressionsInRunScriptsFromParsed | ||
| needsUnsafeCheck := hasExpressionInRunContent(yamlContent, UnsafeContextPattern) | ||
| needsDisallowedCheck := hasNonAllowedExpressionInRunContent(yamlContent) |
There was a problem hiding this comment.
Behavioral regression: disallowed expressions in undetected run blocks can now slip through validation entirely.
The old pre-check (hasAnyExpressionInRunContent) triggered yaml.Unmarshal + both validators whenever any ${{ }} appeared in a text-detected run block—including allowed ones like ${{ runner.temp }}. That side-effect also caused validateNoGitHubExpressionsInRunScriptsFromParsed to inspect all run blocks from the parsed YAML, including flow-style steps and quoted-key steps that the text scanner misses.
The new pre-check only fires when a disallowed expression appears in a text-detected run block. If a workflow has only allowed expressions in block-style steps plus a disallowed expression in a step the scanner does not detect, needsDisallowedCheck stays false, no yaml.Unmarshal is performed, and validateNoGitHubExpressionsInRunScriptsFromParsed is silently skipped.
💡 Concrete regression scenario
# flow-style step: text scanner misses it
- { run: "echo ${{ github.actor }}" } # disallowed, not in allow-list
# block-style step: scanner detects it, but expression is allowed
- run: node ${{ runner.temp }}/foo.cjsOld code (hasAnyExpressionInRunContent): finds ${{ runner.temp }} in the block-style step → true → yaml.Unmarshal → parsed validator sees both steps → flags ${{ github.actor }}.
New code: hasNonAllowedExpressionInRunContent finds ${{ runner.temp }} (allowed, skip); misses the flow-style step entirely → needsDisallowedCheck = false → no yaml.Unmarshal → ${{ github.actor }} passes undetected.
Suggested fix: keep InlineExpressionPattern as the yaml.Unmarshal gate (preserving the original trigger), and use the two targeted flags only to decide which validators to call inside the already-parsed block:
if hasExpressionInRunContent(yamlContent, InlineExpressionPattern) {
var reparsed map[string]any
if err := yaml.Unmarshal(...); err == nil && reparsed != nil {
if hasExpressionInRunContent(yamlContent, UnsafeContextPattern) {
templateErr = validateNoTemplateInjectionFromParsed(reparsed)
}
if templateErr == nil && hasNonAllowedExpressionInRunContent(yamlContent) {
templateErr = validateNoGitHubExpressionsInRunScriptsFromParsed(reparsed)
}
}
}This skips yaml.Unmarshal when there are no expressions at all in detectable run blocks (the dominant fast path) while preserving the original validation safety net.
| if strings.HasPrefix(keyPart, "-") { | ||
| keyPart = strings.TrimSpace(keyPart[1:]) | ||
| } | ||
| if !strings.HasPrefix(keyPart, "run:") { |
There was a problem hiding this comment.
Pre-check silently misses flow-style step mappings and quoted run keys, producing false negatives for the regression guardrail.
strings.HasPrefix(keyPart, "run:") only recognises a plain scalar run: key. The following valid YAML forms are skipped entirely:
- Flow-style mapping:
- { run: "echo ${{ github.actor }}" }—keyPartstarts with{, notrun: - Double-quoted key:
"run": echo ${{ github.actor }}—keyPartstarts with" - Single-quoted key:
'run': echo ${{ github.actor }}—keyPartstarts with'
extractRunBlocks (used by both parsed validators) handles all three forms correctly because it operates on parsed YAML. The text scanner does not, so expressions in those steps never reach validateNoGitHubExpressionsInRunScriptsFromParsed.
💡 How to detect flow-style steps
The minimal surgical fix for flow-style support is to treat a line whose trimmed form contains run: anywhere as a potential match, then extract the value:
// Handle flow-style: - { run: "..." } or { run: "..." }
if idx := strings.Index(keyPart, "run:"); idx >= 0 {
// make sure it is actually a key boundary, not e.g. "dry-run:"
if idx == 0 || keyPart[idx-1] == ' ' || keyPart[idx-1] == '{' || keyPart[idx-1] == ',' {
rest = strings.TrimSpace(keyPart[idx+4:])
// strip trailing flow-mapping characters, then check for expressions
...
}
}Alternatively, since quoted/flow-style keys are not emitted by the current compiler, add a comment documenting this known gap and a guard test that will fail if the compiler ever starts emitting them.
| // allowedRunScriptExpressionRegex (e.g. ${{ runner.temp }}, ${{ env.FOO }}). It is | ||
| // used as a fast pre-check for the regression guardrail inside validateTemplateInjection | ||
| // (Path B) to avoid a yaml.Unmarshal when every run-block expression is compiler-owned. | ||
| func hasNonAllowedExpressionInRunContent(yamlContent string) bool { |
There was a problem hiding this comment.
Run-block detection logic is copy-pasted into two independent scanners, creating a silent divergence footgun.
hasNonAllowedExpressionInRunContent and hasExpressionInRunContent share ~45 lines of identical run-block detection code (indentation tracking, - stripping, block-scalar handling). The security correctness of Path B rests on the assumption that both scanners detect the exact same set of run blocks—so that needsUnsafeCheck = true always implies needsDisallowedCheck = true for the same expression.
If the two copies diverge (e.g., a future bug fix for the flow-style gap is applied to one but not the other), the invariant silently breaks: needsUnsafeCheck could be true while needsDisallowedCheck stays false, causing validateNoGitHubExpressionsInRunScriptsFromParsed to be skipped even after yaml.Unmarshal was already paid for.
💡 Suggested refactor
Extract a shared walkRunBlockLines iterator or callback and let both callers plug in their own content-check logic:
// walkRunBlockLines calls visit(line) for each line inside a run: block.
// Returns true as soon as visit returns true (short-circuit).
func walkRunBlockLines(yamlContent string, visit func(line string) bool) bool { ... }
func hasExpressionInRunContent(yamlContent string, re *regexp.Regexp) bool {
return walkRunBlockLines(yamlContent, func(line string) bool {
return re.MatchString(line)
})
}
func hasNonAllowedExpressionInRunContent(yamlContent string) bool {
return walkRunBlockLines(yamlContent, func(line string) bool {
for _, expr := range InlineExpressionPattern.FindAllString(line, -1) {
if !allowedRunScriptExpressionRegex.MatchString(expr) {
return true
}
}
return false
})
}This also makes the flow-style gap easier to fix once: change walkRunBlockLines in one place.
| assert.Equal(t, tt.expected, result, "hasNonAllowedExpressionInRunContent() mismatch") | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
Missing test for the key false-negative scenario that the regression in compiler.go enables.
All new tests exercise straightforward block-style YAML where the text scanner correctly detects every run block. None of them cover the compound case where:
- A block-style run step has an allowed expression (
${{ runner.temp }}) — triggeringhasAnyExpressionInRunContentin the old code but NOThasNonAllowedExpressionInRunContentin the new code. - A flow-style (or quoted-key) run step in the same workflow has a disallowed expression (
${{ github.actor }}).
Add a test case like:
{
name: "flow-style step with disallowed expression alongside allowed block-style step",
yaml: `jobs:
test:
steps:
- { run: "echo ${{ github.actor }}" }
- run: node ${{ runner.temp }}/foo.cjs`,
expected: true, // disallowed expression must be detected
},This test currently fails with the new code (returns false), confirming the regression described in the companion comment on compiler.go:344. Having it in the suite prevents a silent regression if the implementation is changed again.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnose and /tdd — the fix is correct and well-measured; commenting on test coverage gaps.
📋 Key Themes & Highlights
Key Themes
- Test coverage gaps on allow-list boundaries:
steps.parse-guard-vars.outputs.*,job.services[...].ports[...],github.run_id, andgithub.workspaceare all explicitly inallowedRunScriptExpressionRegexbut have no unit tests — a security boundary that a regex edit could silently widen or narrow. - Missing multi-expression edge case: the
FindAllStringloop covering lines with multiple${{ }}expressions (some allowed, some not) is untested; a regression toFindStringwould be invisible. - Implicit security invariant:
validateNoTemplateInjectionFromParsedis now skipped whenneedsUnsafeCheck=false, a correct but undocumented change from the old unconditional-call behaviour. - No integration-level test for the fast path: the key performance win (both scans return false → zero
yaml.Unmarshal) has no end-to-end test throughvalidateTemplateInjection.
Positive Highlights
- ✅ Clear, reproducible benchmark data (4.1× ns/op, 9× allocs/op) pinpoints the exact regression — textbook
/diagnoseapproach. - ✅ Root cause correctly identified:
hasAnyExpressionInRunContentmatching compiler-owned expressions was the false-positive driving unnecessaryyaml.Unmarshalcalls. - ✅ The two independent flags (
needsUnsafeCheck/needsDisallowedCheck) are a clean decomposition — each validator is gated by exactly the pre-check relevant to it. - ✅ The new function's inline-run and multiline-run paths both apply
FindAllString, which is correct. - ✅ Excellent inline documentation on Path B — future readers will understand the design without having to trace through the old code.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 163.5 AIC · ⌖ 10.3 AIC · ⊞ 6.9K
| } else { | ||
| // Inside run block content — check each expression on this line. | ||
| if InlineExpressionPattern.MatchString(line) { | ||
| for _, expr := range InlineExpressionPattern.FindAllString(line, -1) { |
There was a problem hiding this comment.
[/tdd] The FindAllString loop correctly iterates all expressions on a line, but no test exercises a line containing both an allowed and a disallowed expression (e.g. ${{ env.FOO }} ${{ github.event.issue.title }}). A future accidental replacement with FindString (first-match only) would be invisible to the test suite.
💡 Suggested test case
{
name: "mixed allowed and disallowed expressions on same line",
yaml: `jobs:
test:
steps:
- run: echo ${{ env.FOO }} ${{ github.event.issue.title }}`,
expected: true,
},| echo "${{ github.event.issue.title }}"`, | ||
| expected: true, | ||
| }, | ||
| } |
There was a problem hiding this comment.
[/tdd] The allow-list regex covers steps.parse-guard-vars.outputs.(approval_labels|blocked_users|trusted_users), job.services[...].ports[...], github.run_id, and github.workspace, but none of these patterns have a test case. They are security boundaries — a regex change that narrows or widens them would be undetected by the test suite.
💡 Suggested test cases
{
name: "allowed steps.parse-guard-vars output in run block",
yaml: `jobs:
test:
steps:
- run: echo ${{ steps.parse-guard-vars.outputs.approval_labels }}`,
expected: false,
},
{
name: "disallowed output from non-allow-listed step",
yaml: `jobs:
test:
steps:
- run: echo ${{ steps.other-step.outputs.value }}`,
expected: true,
},
{
name: "allowed github.run_id in run block",
yaml: `jobs:
test:
steps:
- run: echo ${{ github.run_id }}`,
expected: false,
},| if reparsed != nil { | ||
| templateErr = validateNoTemplateInjectionFromParsed(reparsed) | ||
| if templateErr == nil { | ||
| if needsUnsafeCheck { |
There was a problem hiding this comment.
[/diagnose] validateNoTemplateInjectionFromParsed is now conditionally skipped when needsUnsafeCheck=false, which is a behaviour change from the old code (which called both validators whenever any expression existed). The reasoning is sound — the injection validator only catches user-controlled contexts, and if UnsafeContextPattern found nothing there's nothing to inject — but this invariant is implicit. A short comment would help future reviewers confirm no security check was accidentally dropped.
💡 Suggested comment
// validateNoTemplateInjectionFromParsed is intentionally skipped when
// needsUnsafeCheck=false: the validator only catches user-controlled contexts,
// and the UnsafeContextPattern scan above has already confirmed none are present.
if needsUnsafeCheck {
templateErr = validateNoTemplateInjectionFromParsed(reparsed)
}| // needsDisallowedCheck: true when any expression outside the compiler allow-list | ||
| // appears in run: blocks | ||
| // → triggers validateNoGitHubExpressionsInRunScriptsFromParsed | ||
| needsUnsafeCheck := hasExpressionInRunContent(yamlContent, UnsafeContextPattern) |
There was a problem hiding this comment.
[/tdd] The new Path B logic (both-scans-false → skip yaml.Unmarshal) is the core of this performance fix, but there is no compiler-level integration test that exercises this fast path end-to-end. The new unit tests cover hasNonAllowedExpressionInRunContent in isolation; a test at the validateTemplateInjection or Compile level that feeds a workflow containing only ${{ runner.temp }} and asserts no error would anchor the behaviour and catch future regressions at the seam.
💡 Sketch
// Verify that a compiled workflow with only compiler-owned expressions
// in run blocks does not trigger a yaml.Unmarshal error path.
func TestValidateTemplateInjection_AllowedExpressionsOnly(t *testing.T) {
yamlWithAllowed := `on: push
jobs:
test:
runs-on: ubuntu-latest
steps:
- run: node ${{ runner.temp }}/foo.cjs
`
c := NewCompiler()
err := c.validateTemplateInjection(yamlWithAllowed, "", "test.md", nil)
assert.NoError(t, err)
}Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
@copilot review all comments and address unresolved review feedback.
|
|
@copilot refresh the branch and rerun checks.
|
|
``
|
|
``
|
Weekly update covering the week of June 15–22, 2026: - Compiler +320% performance regression fix (#40662) - New deferinloop Go linter (#40679) - gh-aw-detection rollout to 50% of workflows (#40698) - JSON-RPC error handling fix (#40715) - Skillet sparse checkout path fix (#40684) - FNV-1a heredoc delimiter generation (#40696) - Agent of the Week: delight ✨ Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
BenchmarkCompileComplexWorkflowregressed from ~3ms/op to ~12.7ms/op. CPU profiling pinpointed the cause:validateTemplateInjectionPath B (active wheneverskipValidation=true, the default inNewCompiler()) calledhasAnyExpressionInRunContentwhich matches any${{ }}expression — including compiler-owned ones like${{ runner.temp }}. This unconditionally triggered a fullyaml.Unmarshalon the ~1200-line compiled YAML on every invocation, even when no violation was possible.Changes
validateTemplateInjectionPath B (compiler.go): replace the broadhasAnyExpressionInRunContentpre-check with two targeted text scans that together cover both sub-validators.yaml.Unmarshalis now only called when a scan signals a potential issue:hasExpressionInRunContent(UnsafeContextPattern)— user-controlled contexts (template injection risk)hasNonAllowedExpressionInRunContent— expressions not in the compiler-owned allow-list (regression guardrail)template_injection_validation.go: addhasNonAllowedExpressionInRunContent; remove now-unusedhasAnyExpressionInRunContent. The new function short-circuits when every${{ }}expression in arun:block matchesallowedRunScriptExpressionRegex(env.*,vars.*,runner.*, etc.).For well-formed workflows the common case is both scans return false —
yaml.Unmarshalis skipped entirely.Result