perf(workflow): cut YAMLGeneration overhead in run-script expression guardrail#44438
Conversation
…sion exists Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #44438 does not have the 'implementation' label and has only 36 new lines of code in business logic directories (threshold: 100). |
|
✅ Test Quality Sentinel completed test quality analysis. |
There was a problem hiding this comment.
Pull request overview
Optimizes the run-script expression guardrail in YAML generation by adding a cheap precheck in the parsed-YAML validation path to avoid expensive heredoc/comment stripping and regex extraction when a run: block cannot contain inline GitHub expressions. Adds a benchmark intended to lock in the new fast path for the common “no expressions” case.
Changes:
- Added
mayContainInlineExpressionshort-circuit invalidateNoGitHubExpressionsInRunScriptsFromParsedto skip heavy processing forrun:blocks without possible${{ ... }}usage. - Added
BenchmarkValidateNoGitHubExpressionsInRunScriptsFromParsed_NoExpressionsto regression-guard the new fast path.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/template_injection_validation.go | Adds a cheap precheck to skip expensive run-script scanning work when inline expressions cannot be present. |
| pkg/workflow/template_injection_validation_benchmark_test.go | Adds a benchmark covering the no-expression parsed validation path. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Low
| var parsed map[string]any | ||
| if err := yaml.Unmarshal([]byte(yamlContent), &parsed); err != nil { | ||
| b.Fatal(err) | ||
| } | ||
|
|
||
| b.ReportAllocs() | ||
| for b.Loop() { |
There was a problem hiding this comment.
Review: perf(workflow): cut YAMLGeneration overhead in run-script expression guardrail
The optimization is correct and well-targeted. The mayContainInlineExpression precheck is zero-false-negative (requires both ${{" and }}` to be present), making the fast-path skip safe. The same pattern is already applied in other guardrail paths in this file (lines 144, 150, 183), so this aligns the parsed-path consistently.
One non-blocking suggestion: see inline comment on benchmark loop style — the new benchmark uses b.Loop() while the existing benchmark in the same file uses the for i := 0; i < b.N; i++ form with an explanatory nolint comment. Aligning the two would keep the file internally consistent.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 28.4 AIC · ⌖ 5.96 AIC · ⊞ 4.8K
| } | ||
|
|
||
| b.ReportAllocs() | ||
| for b.Loop() { |
There was a problem hiding this comment.
The new benchmark uses b.Loop() while the existing benchmark in this file uses for i := 0; i < b.N; i++ with an explicit nolint comment:
(nolint/redacted):intrange // Use the standard testing.B loop form for broad tool compatibility.
for i := 0; i < b.N; i++ {Consider using the same b.N form here for consistency and to match the stated codebase policy:
(nolint/redacted):intrange // Use the standard testing.B loop form for broad tool compatibility.
for i := 0; i < b.N; i++ {
if err := validateNoGitHubExpressionsInRunScriptsFromParsed(parsed); err != nil {
b.Fatalf("validateNoGitHubExpressionsInRunScriptsFromParsed() error = %v", err)
}
}b.Loop() is fine in Go 1.24+ (this module declares go 1.26.3), but the inconsistency with the adjacent benchmark and its explanatory comment is worth aligning.
@copilot please address this.
🧪 Test Quality Sentinel Report✅ Test Quality Score: 90/100 — Excellent
📊 Metrics (1 test)
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — no blocking changes, 3 observations inline.
📋 Key Themes & Highlights
Key Themes
- Benchmark loop inconsistency: New benchmark uses
b.Loop()while the sibling in the same file usesfor i := 0; i < b.N; i++— worth aligning. - Precheck asymmetry not documented:
validateNoTemplateInjectionFromParsed(line 183) guards with a full regex; the updated function now uses the cheapermayContainInlineExpressionsubstring scan. The different strategies are fine but unannounced. - No direct unit tests for
validateNoGitHubExpressionsInRunScriptsFromParsed: The function is only covered through compiler-level integration tests; a small focused table-driven test would lock in the fast-path behaviour.
Positive Highlights
- ✅ The fix is minimal, surgical, and correctly scoped — no semantic changes to validation logic
- ✅
mayContainInlineExpressionis already well-documented as zero-false-negative; reuse here is the right call - ✅ Adding a focused benchmark for the specific fast path is excellent regression discipline — this directly pins the work we care about
- ✅ PR description is thorough: it explains root cause, fix, and the quantified regression clearly
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
proxy.golang.org
To allow these domains, add them to the
network.allowedlist in your workflow frontmatter:
network:
allowed:
- defaults
- "proxy.golang.org"See Network Configuration for more information.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 71.1 AIC · ⌖ 8.39 AIC · ⊞ 6.6K
Comment /matt to run again
| } | ||
|
|
||
| b.ReportAllocs() | ||
| for b.Loop() { |
There was a problem hiding this comment.
[/diagnosing-bugs] Benchmark loop style is inconsistent with the sibling benchmark in this file — the existing BenchmarkValidateTemplateInjectionFastPath uses for i := 0; i < b.N; i++ with a //nolint:intrange comment explaining the choice for "broad tool compatibility".
💡 Options
b.Loop() is actually the better API (Go 1.24+): it resets timers correctly around the loop body. The rest of the codebase already uses it. Two paths forward:
- Preferred: Update the sibling
BenchmarkValidateTemplateInjectionFastPathtofor b.Loop()as a follow-up (the//nolint:intrangecomment becomes unnecessary sinceb.Loop()is not a range form). This makes the file consistent and removes the stale nolint comment. - Conservative: Add a brief comment to the new benchmark explaining the choice, e.g.
// b.Loop() — preferred over b.N loop since Go 1.24 (resets timer correctly).
Neither blocks this PR, but leaving two incompatible patterns in the same file is a maintenance hazard.
@copilot please address this.
| var violations []TemplateInjectionViolation | ||
|
|
||
| for _, runContent := range runBlocks { | ||
| if !mayContainInlineExpression(runContent) { |
There was a problem hiding this comment.
[/diagnosing-bugs] The sibling function validateNoTemplateInjectionFromParsed (line 183) uses InlineExpressionPattern.MatchString(runContent) as its precheck, while this function now uses mayContainInlineExpression(runContent). These are semantically different — full regex vs cheap substring scan — and the asymmetry may confuse future readers.
💡 Suggested clarifying comment
A brief inline comment would make the intentional trade-off explicit:
// Use the cheap substring precheck (not InlineExpressionPattern.MatchString as in
// validateNoTemplateInjectionFromParsed) because for the common no-expression case
// this avoids regex engine overhead. mayContainInlineExpression is zero-false-negative:
// any real "${{ ... }}" must contain both substrings.
if !mayContainInlineExpression(runContent) {
continue
}Alternatively, the sibling function at line 183 could be updated to use mayContainInlineExpression for consistency — it would be a strict improvement there too.
@copilot please address this.
| } | ||
| } | ||
|
|
||
| func BenchmarkValidateNoGitHubExpressionsInRunScriptsFromParsed_NoExpressions(b *testing.B) { |
There was a problem hiding this comment.
[/tdd] The benchmark covers the no-expression fast path but there is no unit test that exercises validateNoGitHubExpressionsInRunScriptsFromParsed directly in template_injection_validation_test.go. Searching the test file shows no TestValidateNoGitHubExpressions* function at all — coverage of this function relies entirely on integration-level compiler tests.
💡 Suggested additions
Adding targeted unit tests would make the fast-path correctness guarantee explicit and catch regressions faster:
func TestValidateNoGitHubExpressionsInRunScriptsFromParsed(t *testing.T) {
tests := []struct {
name string
yaml string
wantErr bool
}{
{
name: "no expressions — fast path, no error",
yaml: `jobs:
test:
steps:
- run: echo hello`,
wantErr: false,
},
{
name: "allowed generated expression passes",
yaml: `jobs:
test:
steps:
- run: node ${{ runner.temp }}/foo.cjs`,
wantErr: false,
},
{
name: "disallowed raw expression is rejected",
yaml: `jobs:
test:
steps:
- run: echo ${{ github.actor }}`,
wantErr: true,
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
var parsed map[string]any
require.NoError(t, yaml.Unmarshal([]byte(tt.yaml), &parsed))
err := validateNoGitHubExpressionsInRunScriptsFromParsed(parsed)
if tt.wantErr {
require.Error(t, err)
} else {
require.NoError(t, err)
}
})
}
}The no-expression case is particularly important to lock in the short-circuit behaviour introduced by this PR.
@copilot please address this.
|
@copilot run pr-finisher skill |
There was a problem hiding this comment.
The optimization is correct and safe — mayContainInlineExpression is genuinely zero-false-negative against InlineExpressionPattern, and the fast-path guard is logically sound. Three non-blocking concerns noted.
### Review themes
Guard asymmetry (template_injection_validation.go line 235): The sibling function validateNoTemplateInjectionFromParsed uses the full compiled regex as its fast-path guard; this function now uses the cheaper string-based precheck. Both are safe, but the divergence is undocumented and will slow down future security audits. A one-line comment explaining the choice closes this.
Benchmark loop style inconsistency (benchmark file line 55): The existing benchmark in the same file deliberately uses for i := 0; i < b.N; i++ with a //nolint:intrange comment. The new benchmark silently uses b.Loop(). Pick one convention and document it.
Benchmark only covers the fast path (benchmark file line 35): The new benchmark measures strings.Cut + strings.Contains on a tiny fixed string — it does not cover the slow path with actual expressions present. The regression the PR claims to fix cannot be measured from this benchmark alone. A companion _WithExpressions benchmark is needed to make the optimization verifiable and to guard against future slow-path regressions.
🔎 Code quality review by PR Code Quality Reviewer · 126.6 AIC · ⌖ 6.77 AIC · ⊞ 5.4K
Comment /review to run again
| var violations []TemplateInjectionViolation | ||
|
|
||
| for _, runContent := range runBlocks { | ||
| if !mayContainInlineExpression(runContent) { |
There was a problem hiding this comment.
Guard asymmetry with sibling function makes security auditing harder: validateNoTemplateInjectionFromParsed (line 183) uses the full compiled regex InlineExpressionPattern.MatchString(runContent) as its fast-path guard, while this new guard uses the cheaper mayContainInlineExpression. There is no comment explaining the intentional divergence.
💡 Details
Both guards are safe in terms of false negatives — mayContainInlineExpression is documented as zero-false-negative. But the two functions now have subtly different skip conditions: mayContainInlineExpression accepts false positives (e.g. a run block containing ${{ in a heredoc body and }} elsewhere) that the full regex would reject. A security auditor comparing the two functions will see inconsistent guards and need to reason through both to confirm neither introduces a bypass. A short inline comment explaining why the cheaper check is correct here would close this gap:
// mayContainInlineExpression is zero-false-negative (see its doc), so it is
// safe to use here instead of the full InlineExpressionPattern regex.
if !mayContainInlineExpression(runContent) {
continue
}| } | ||
|
|
||
| b.ReportAllocs() | ||
| for b.Loop() { |
There was a problem hiding this comment.
b.Loop() inconsistent with the existing benchmark's explicit b.N style in the same file: The benchmark immediately above uses for i := 0; i < b.N; i++ with a //nolint:intrange comment explaining the rationale for that style ('broad tool compatibility'). This new benchmark silently adopts the opposite convention.
💡 Details
Either both benchmarks should use the same style, or this one should carry a comment explaining the intentional divergence. Without that, the next maintainer reading both benchmarks back-to-back will wonder which convention to follow when adding new benchmarks.
To stay consistent with the existing benchmark:
b.ReportAllocs()
(nolint/redacted):intrange // Use the standard testing.B loop form for broad tool compatibility.
for i := 0; i < b.N; i++ {
if err := validateNoGitHubExpressionsInRunScriptsFromParsed(parsed); err != nil {
b.Fatalf("validateNoGitHubExpressionsInRunScriptsFromParsed() error = %v", err)
}
}Or, if b.Loop() is the desired going-forward style, update the existing benchmark and remove its nolint comment too.
| } | ||
| } | ||
|
|
||
| func BenchmarkValidateNoGitHubExpressionsInRunScriptsFromParsed_NoExpressions(b *testing.B) { |
There was a problem hiding this comment.
Benchmark only covers the fast-path skip; the optimization's benefit is not measurable from this benchmark alone: The new benchmark measures a single strings.Cut + strings.Contains call on a short fixed string. It cannot demonstrate the actual performance gain, because there is no companion benchmark for the slow path (run blocks with expressions present).
💡 Details
The PR body claims a significant regression was fixed in BenchmarkYAMLGeneration. Without a slow-path counterpart (e.g. BenchmarkValidateNoGitHubExpressionsInRunScriptsFromParsed_WithExpressions), future contributors cannot:
- Verify the before/after improvement with
go test -bench=. - Detect a future regression where the slow path gets expensive again
A minimal companion benchmark:
func BenchmarkValidateNoGitHubExpressionsInRunScriptsFromParsed_WithExpressions(b *testing.B) {
// workflow with allowed expressions present
yamlContent := `name: with-expressions\non: workflow_dispatch\njobs:\n test:\n runs-on: ubuntu-latest\n steps:\n - run: echo ${{ env.FOO }}\n`
var parsed map[string]any
if err := yaml.Unmarshal([]byte(yamlContent), &parsed); err != nil {
b.Fatal(err)
}
b.ReportAllocs()
for i := 0; i < b.N; i++ {
_ = validateNoGitHubExpressionsInRunScriptsFromParsed(parsed)
}
}This also locks in that the slow path itself does not regress in future refactors.
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Added |
|
🎉 This pull request is included in a new release. Release: |
BenchmarkYAMLGenerationregressed significantly (+143.6% vs historical baseline), with ~5x memory/alloc pressure reported. The hot path was the run-script expression guardrail doing expensive processing even whenrunblocks had no inline expressions.Root-cause scope
validateNoGitHubExpressionsInRunScriptsFromParsedalways performed heredoc/comment stripping and regex extraction perrunblock.${{ ... }}), that work is unnecessary and dominated allocation/CPU in YAML generation.Targeted optimization
mayContainInlineExpression) in the parsed guardrail path.runblock when inline expressions cannot exist, avoiding downstream regex/string processing.Regression guard
BenchmarkValidateNoGitHubExpressionsInRunScriptsFromParsed_NoExpressions