Skip to content

Commit 18af8d6

Browse files
Copilotpelikhangithub-actions[bot]claude
authored
Fix on.bots/on.roles state leakage that corrupts workflow_run triggers (#41018)
* Initial plan * Plan regression fix for workflow_run + bots/roles Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> * Fix workflow_run trigger corruption with on bots and roles Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> * Add regression coverage for workflow_run with bots and roles Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> * docs(adr): add draft ADR-41018 for on: tracker reset Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * Fix review feedback: inline reset into activateEventSection, expand coverage, revert smoke if Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 parent ed5153f commit 18af8d6

5 files changed

Lines changed: 237 additions & 0 deletions

File tree

‎docs/adr/41018-reset-on-section-array-trackers.md‎

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,41 @@
1+
# ADR-41018: Reset `on:` Extension-Array Trackers at Event-Section Boundaries
2+
3+
**Date**: 2026-06-23
4+
**Status**: Draft
5+
6+
## Context
7+
8+
`commentOutProcessedFieldsInOnSection` in `pkg/workflow/frontmatter_extraction_yaml.go` is a line-based state machine that walks the `on:` block and comments out compiler-processed extension fields (`bots`, `roles`, `labels`, `needs`, and their skip variants). It tracks "am I inside this array" with flat boolean flags (`inBotsArray`, `inRolesArray`, etc.). Because those flags were never reset when a new event section began, array state from one sibling key leaked into the next: when `bots:` or `roles:` appeared alongside `workflow_run:` in the same `on:` block, the stale array tracker caused the walker to comment out `workflow_run.workflows`/`types` list items. The stripped `workflows` then failed the non-empty-workflows compile-time validation, breaking an otherwise valid trigger composition.
9+
10+
## Decision
11+
12+
We will reset all top-level `on:` extension-array trackers at the start of every event section. A `resetOnArrayTrackers` closure clears `inSkipRolesArray`, `inSkipBotsArray`, `inRolesArray`, `inBotsArray`, `inLabelsArray`, and `inNeedsArray`, and it is invoked immediately before `activateEventSection` for each recognized section (`pull_request`, `issues`, `discussion`, `issue_comment`, `deployment_status`, `workflow_run`). This keeps the existing line-based scanner but makes its array state strictly section-local, so no sibling key can inherit a stale "inside array" flag.
13+
14+
## Alternatives Considered
15+
16+
### Alternative 1: Replace the line-based scanner with structured YAML scoping
17+
18+
We could parse the `on:` block into a node tree and decide field-by-field which keys to comment out, eliminating flat boolean state entirely. This is the more robust long-term fix, but it is a substantial rewrite of a security-relevant code path and risks changing comment-out behavior for the many already-covered cases. Given the bug is a narrow state-leak, the targeted reset was preferred over a structural rewrite under this PR's scope.
19+
20+
### Alternative 2: Reset only the bots/roles trackers, or only on entering `workflow_run`
21+
22+
We could clear just the two flags implicated in the reported failure, or reset only when the walker enters `workflow_run:`. This was rejected because the leak is a general property of the flat-flag design — any extension array preceding any event section can leak — so a partial reset would leave latent corruption for other key orderings (e.g. `labels`/`needs` before `issues`). Resetting all trackers at every section boundary fixes the class of bug rather than the single reported instance.
23+
24+
## Consequences
25+
26+
### Positive
27+
- `workflow_run` triggers compose correctly with sibling `bots:`/`roles:` filters; `workflows`/`types` are no longer stripped, so the non-empty-workflows validation passes.
28+
- The fix is minimal and localized, preserving the existing, well-tested line-based comment-out behavior for all other cases.
29+
- Regression coverage is added at two levels: direct comment-out tests in `compiler_draft_test.go` (four key orderings) and strict-mode compile tests in `workflow_run_validation_test.go`.
30+
31+
### Negative
32+
- The fragile flat-boolean state machine remains; the reset is a guard, not a structural fix, so similar state-leak bugs can recur if new trackers are added without being included in `resetOnArrayTrackers`.
33+
- The reset list must be kept in sync with the set of extension-array trackers — a future tracker omitted from the closure would silently reintroduce the leak.
34+
35+
### Neutral
36+
- Behavior is unchanged for `on:` blocks that do not mix extension arrays with event sections; the reset is a no-op when no array flag is set.
37+
- The reset is tied to the fixed set of recognized event-section keys; adding a new event section requires adding the same `resetOnArrayTrackers()` call alongside its `activateEventSection` invocation.
38+
39+
---
40+
41+
*ADR created by [adr-writer agent]. Review and finalize before changing status from Draft to Accepted.*

‎docs/src/content/docs/reference/triggers.md‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -209,6 +209,16 @@ on:
209209
- develop
210210
```
211211

212+
You can also combine `workflow_run` with top-level authorization filters such as `bots:` or `roles:`:
213+
214+
```yaml wrap
215+
on:
216+
workflow_run:
217+
workflows: ["CI"]
218+
types: [completed]
219+
bots: ["dependabot[bot]"]
220+
```
221+
212222
Workflows with `workflow_run` triggers include automatic security protections: `workflows` must list at least one non-empty entry (empty or missing values are rejected at compile time, since GitHub silently disables such triggers); the compiler injects repository ID and fork checks to reject cross-repository or fork-triggered runs; and `branches` is recommended to limit triggering branches (the compiler warns when omitted, or errors in strict mode). See the [Security Architecture](/gh-aw/introduction/architecture/) for details.
213223

214224
#### Conclusion Filtering (`conclusion:`)

‎pkg/workflow/compiler_draft_test.go‎

Lines changed: 114 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -352,6 +352,120 @@ func TestCommentOutProcessedFieldsInOnSection(t *testing.T) {
352352
workflow_dispatch:`,
353353
description: "Should reset workflow_run conclusion tracker when entering a new event section",
354354
},
355+
{
356+
name: "workflow_run followed by bots keeps workflows and types uncommented",
357+
input: `on:
358+
workflow_run:
359+
workflows: ["CI"]
360+
types: [completed]
361+
bots:
362+
- dependabot
363+
workflow_dispatch:`,
364+
expected: `on:
365+
workflow_run:
366+
workflows: ["CI"]
367+
types: [completed]
368+
# bots: # Bots processed as bot check in pre-activation job
369+
# - dependabot # Bots processed as bot check in pre-activation job
370+
workflow_dispatch:`,
371+
description: "Should not let bots array state leak into workflow_run fields",
372+
},
373+
{
374+
name: "bots before workflow_run do not comment workflow_run list items",
375+
input: `on:
376+
bots:
377+
- dependabot
378+
workflow_run:
379+
workflows: ["CI"]
380+
types: [completed]
381+
workflow_dispatch:`,
382+
expected: `on:
383+
# bots: # Bots processed as bot check in pre-activation job
384+
# - dependabot # Bots processed as bot check in pre-activation job
385+
workflow_run:
386+
workflows: ["CI"]
387+
types: [completed]
388+
workflow_dispatch:`,
389+
description: "Should reset bots array tracker before entering workflow_run section",
390+
},
391+
{
392+
name: "bots before workflow_run with multi-line arrays do not comment workflow_run list items",
393+
input: `on:
394+
bots:
395+
- dependabot
396+
workflow_run:
397+
workflows:
398+
- CI
399+
types:
400+
- completed
401+
workflow_dispatch:`,
402+
expected: `on:
403+
# bots: # Bots processed as bot check in pre-activation job
404+
# - dependabot # Bots processed as bot check in pre-activation job
405+
workflow_run:
406+
workflows:
407+
- CI
408+
types:
409+
- completed
410+
workflow_dispatch:`,
411+
description: "Should not comment out multi-line workflow_run.workflows/types items when bots precedes workflow_run",
412+
},
413+
{
414+
name: "skip-if-check-failing before workflow_run does not corrupt workflow_run list items",
415+
input: `on:
416+
skip-if-check-failing:
417+
- build
418+
workflow_run:
419+
workflows:
420+
- CI
421+
types:
422+
- completed
423+
workflow_dispatch:`,
424+
expected: `on:
425+
# skip-if-check-failing: # Skip-if-check-failing processed as check status gate in pre-activation job
426+
# - build
427+
workflow_run:
428+
workflows:
429+
- CI
430+
types:
431+
- completed
432+
workflow_dispatch:`,
433+
description: "Should reset inSkipIfCheckFailing before entering workflow_run to prevent list-item corruption",
434+
},
435+
{
436+
name: "roles before workflow_run do not comment workflow_run list items",
437+
input: `on:
438+
roles:
439+
- write
440+
workflow_run:
441+
workflows: ["CI"]
442+
types: [completed]
443+
workflow_dispatch:`,
444+
expected: `on:
445+
# roles: # Roles processed as role check in pre-activation job
446+
# - write # Roles processed as role check in pre-activation job
447+
workflow_run:
448+
workflows: ["CI"]
449+
types: [completed]
450+
workflow_dispatch:`,
451+
description: "Should reset roles array tracker before entering workflow_run section",
452+
},
453+
{
454+
name: "roles all before workflow_run keeps workflow_run intact",
455+
input: `on:
456+
roles: all
457+
workflow_run:
458+
workflows: ["CI"]
459+
types: [completed]
460+
workflow_dispatch:`,
461+
expected: `on:
462+
# roles: all # Roles processed as role check in pre-activation job
463+
workflow_run:
464+
workflows: ["CI"]
465+
types: [completed]
466+
workflow_dispatch:`,
467+
description: "Should handle inline roles value without affecting workflow_run fields",
468+
},
355469
{
356470
name: "top-level on needs array",
357471
input: `on:

‎pkg/workflow/frontmatter_extraction_yaml.go‎

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -164,7 +164,28 @@ func (c *Compiler) commentOutProcessedFieldsInOnSection(yamlStr string, frontmat
164164
deploymentStatusIndent := -1
165165
workflowRunIndent := -1
166166
// activateEventSection resets all event-section flags and then activates the selected section.
167+
// It also clears every top-level on: extension-array tracker (inBotsArray, inRolesArray,
168+
// inSkipIfCheckFailing, etc.) before entering the new section. This reset is required
169+
// because each activateEventSection call ends with "continue", which bypasses the
170+
// indent-based deactivation logic further down the loop. Without the explicit reset here,
171+
// a stale flag from a preceding bots:/roles:/skip-if-check-failing: block would cause that
172+
// section's list items (e.g. "workflow_run.workflows: - CI") to be incorrectly commented out.
167173
activateEventSection := func(section string, indent int) {
174+
// Clear all top-level on: extension-array state so no sibling section leaks in.
175+
inSkipRolesArray = false
176+
inSkipBotsArray = false
177+
inRolesArray = false
178+
inBotsArray = false
179+
inLabelsArray = false
180+
inNeedsArray = false
181+
// These trackers share the same exit-check-ordering issue: their deactivation
182+
// logic runs after the "continue" that terminates each activateEventSection call,
183+
// so they must also be reset here explicitly.
184+
inSkipIfMatch = false
185+
inSkipIfNoMatch = false
186+
inSkipIfCheckFailing = false
187+
inSkipAuthorAssociations = false
188+
168189
inPullRequest = section == "pull_request"
169190
inIssues = section == "issues"
170191
inDiscussion = section == "discussion"

‎pkg/workflow/workflow_run_validation_test.go‎

Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -119,6 +119,57 @@ Test workflow content.`,
119119
expectWarning: false,
120120
warningCount: 0,
121121
},
122+
{
123+
name: "workflow_run with sibling bots - strict mode - should pass",
124+
frontmatter: `---
125+
on:
126+
bots:
127+
- dependabot
128+
workflow_run:
129+
workflows:
130+
- build
131+
types:
132+
- completed
133+
branches:
134+
- main
135+
tools:
136+
github:
137+
toolsets: [repos]
138+
---
139+
140+
# Workflow Run With Bots
141+
Test workflow content.`,
142+
filename: "workflow-run-with-bots-strict.md",
143+
strictMode: true,
144+
expectError: false,
145+
expectWarning: false,
146+
warningCount: 0,
147+
},
148+
{
149+
name: "workflow_run with sibling roles all - strict mode - should pass",
150+
frontmatter: `---
151+
on:
152+
roles: all
153+
workflow_run:
154+
workflows:
155+
- build
156+
types:
157+
- completed
158+
branches:
159+
- main
160+
tools:
161+
github:
162+
toolsets: [repos]
163+
---
164+
165+
# Workflow Run With Roles
166+
Test workflow content.`,
167+
filename: "workflow-run-with-roles-strict.md",
168+
strictMode: true,
169+
expectError: false,
170+
expectWarning: false,
171+
warningCount: 0,
172+
},
122173
{
123174
name: "workflow_run without workflows - should error",
124175
frontmatter: `---

0 commit comments

Comments
 (0)