Fix on.bots/on.roles state leakage that corrupts workflow_run triggers#41018
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
on.bots/on.roles state leakage that corrupts workflow_run triggers
There was a problem hiding this comment.
Pull request overview
Fixes a compiler bug in gh-aw where on.bots / on.roles list-item comment-out state could leak across sibling on: entries and corrupt workflow_run trigger fields (notably workflows / types). The PR also adds regression tests and documentation to codify the supported trigger composition.
Changes:
- Reset top-level
on:extension-array trackers when entering a new event section during frontmatter comment-out processing, preventing cross-section state leakage. - Add regression tests covering
workflow_runcombined with siblingbots/rolesin both direct comment-out behavior and strict-mode compilation validation. - Document the supported
workflow_run+botscomposition in the trigger reference.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/frontmatter_extraction_yaml.go | Resets top-level on: array-tracking state on event-section transitions to prevent state leakage into workflow_run. |
| pkg/workflow/compiler_draft_test.go | Adds comment-out regression tests ensuring workflow_run.workflows/types remain intact with sibling bots/roles. |
| pkg/workflow/workflow_run_validation_test.go | Adds strict-mode validation coverage for workflow_run with sibling bots / roles: all. |
| docs/src/content/docs/reference/triggers.md | Documents workflow_run combined with top-level bots:/roles: authorization filters. |
| .github/workflows/smoke-claude-on-copilot.lock.yml | Updates compiled workflow output; includes a detection job if: condition change that should be confirmed as intentional. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 5/5 changed files
- Comments generated: 1
| needs: | ||
| - activation | ||
| - agent | ||
| if: > | ||
| always() && needs.agent.result != 'skipped' && (needs.agent.outputs.output_types != '' || needs.agent.outputs.has_patch == 'true') | ||
| if: always() && needs.agent.result != 'skipped' | ||
| runs-on: ubuntu-latest |
|
✅ 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. ✅ |
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 (133 new lines across 📄 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. Even for a focused bug fix, recording that the state-leak was patched with a per-section reset (rather than a parser rewrite) tells future contributors the fragile flat-flag scanner was a known, deliberate trade-off — not an oversight. 📋 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.
REQUEST_CHANGES — The bots/roles leakage is correctly fixed, but the same exit-check-ordering bug exists for inSkipAuthorAssociations and inSkipIfCheckFailing, leaving live corruption paths open. The call-site reset pattern also makes future regressions easy.
Blocking issues
Incomplete tracker reset (high)
inSkipAuthorAssociations is missing from resetOnArrayTrackers(). Its commenting rule (inSkipAuthorAssociations && lineIndent > 2 → comment) would silence every sub-field of workflow_run (workflows:, types:, branches:) when skip-author-associations: precedes workflow_run: in the same on: block — identical in mechanism to the bug being fixed. inSkipIfCheckFailing similarly endangers workflow_run.branches list items. Both are missing from the reset and from the new test matrix.
Fragile call-site pattern (medium)
resetOnArrayTrackers() is manually invoked at 6 separate call sites instead of being folded into activateEventSection(). Any future event addition that skips the call silently reintroduces the class of bug this PR is meant to eliminate.
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
patchdiff.githubusercontent.com
To allow these domains, add them to the
network.allowedlist in your workflow frontmatter:
network:
allowed:
- defaults
- "patchdiff.githubusercontent.com"See Network Configuration for more information.
🔎 Code quality review by PR Code Quality Reviewer · 88.5 AIC · ⌖ 7.77 AIC · ⊞ 5.1K
| inRolesArray = false | ||
| inBotsArray = false | ||
| inLabelsArray = false | ||
| inNeedsArray = false |
There was a problem hiding this comment.
Incomplete fix: inSkipAuthorAssociations (and inSkipIfCheckFailing) are not cleared, leaving an identical workflow_run corruption path still open.
💡 Details and fix
The exit-check that normally clears inSkipAuthorAssociations (lines 431–438) fires after the event-section detection block, which does continue — so it is skipped entirely for the header line itself. If skip-author-associations: appears before workflow_run: in the same on: block:
on:
skip-author-associations:
- FIRST_TIME_CONTRIBUTOR
workflow_run:
workflows: ["CI"]
types: [completed]inSkipAuthorAssociations stays true through the entire workflow_run section. The commenting rule at line 585 is:
} else if inSkipAuthorAssociations && lineIndent > 2 {
shouldComment = trueSince workflows:, types:, and branches: all have indent > 2, every sub-field of workflow_run is commented out — the same corruption this PR claims to fix for bots/roles.
inSkipIfCheckFailing has the same ordering problem; its rule matches - list items, which would hit workflow_run.branches entries.
Fix — expand resetOnArrayTrackers:
resetOnArrayTrackers := func() {
inSkipRolesArray = false
inSkipBotsArray = false
inRolesArray = false
inBotsArray = false
inLabelsArray = false
inNeedsArray = false
// These share the same exit-check-ordering bug:
inSkipAuthorAssociations = false
inSkipIfCheckFailing = false
inSkipIfNoMatch = false
inSkipIfMatch = false
}Also add a regression test with skip-author-associations before workflow_run to prevent this from regressing silently.
| // the permission comment-out logic. | ||
| if !inOnPermissions && !inOnSteps && !inSkipAuthorAssociations { | ||
| if (lineIndent == 2 || lineIndent == 4) && trimmedLine == "pull_request:" { | ||
| resetOnArrayTrackers() |
There was a problem hiding this comment.
Fragile call-site pattern: resetOnArrayTrackers() is invoked manually at 6 separate sites instead of being embedded inside activateEventSection(), making it a regression trap for every future event addition.
💡 Suggested refactor
Move the reset into activateEventSection (line 167) so it fires automatically:
activateEventSection := func(section string, indent int) {
// Reset all top-level on: extension state so no sibling section leaks in.
inSkipRolesArray = false
inSkipBotsArray = false
inRolesArray = false
inBotsArray = false
inLabelsArray = false
inNeedsArray = false
inSkipAuthorAssociations = false // see separate comment
inSkipIfCheckFailing = false
// ...
inPullRequest = section == "pull_request"
// ... rest unchanged
}Then remove the 6 resetOnArrayTrackers() call sites and delete the standalone function. Any future activateEventSection("check_run", ...) call will automatically be safe, with no secondary ritual required.
|
{ \n \n\n### Verdict\n\n> ✅ Check passed. 0% implementation tests (threshold: 30%). All 6 new test rows are high-value behavioral regression tests that directly verify the fixed state-leakage invariant: 📊 Metrics & Test Classification (6 tests analyzed)\n\n| Metric | Value |\n|--------|-------|\n| New/modified tests analyzed | 6 |\n| ✅ Design tests (behavioral contracts) | 6 (100%) |\n|TestCommentOutProcessedFieldsInOnSection — row: workflow_run followed by bots keeps workflows and types uncommented | pkg/workflow/compiler_draft_test.go:~356 | ✅ Design | — |\n| TestCommentOutProcessedFieldsInOnSection — row: bots before workflow_run do not comment workflow_run list items | pkg/workflow/compiler_draft_test.go:~371 | ✅ Design | — |\n| TestCommentOutProcessedFieldsInOnSection — row: roles before workflow_run do not comment workflow_run list items | pkg/workflow/compiler_draft_test.go:~388 | ✅ Design | — |\n| TestCommentOutProcessedFieldsInOnSection — row: roles all before workflow_run keeps workflow_run intact | pkg/workflow/compiler_draft_test.go:~405 | ✅ Design | — |\n| workflow_run validation table — row: workflow_run with sibling bots - strict mode - should pass | pkg/workflow/workflow_run_validation_test.go:~120 | ✅ Design | — |\n| workflow_run validation table — row: workflow_run with sibling roles all - strict mode - should pass | pkg/workflow/workflow_run_validation_test.go:~148 | ✅ Design | — |\n\nGo: 6 (*_test.go); JavaScript: 0. No other languages detected.\n\nTest inflation note: compiler_draft_test.go added 70 lines and workflow_run_validation_test.go added 47 lines (117 total), while the production fix in frontmatter_extraction_yaml.go added only 16 lines. The 7.3:1 ratio technically exceeds the 2:1 threshold and triggers a 10-point penalty. This is the expected pattern for a regression fix — the production change was a targeted bug fix while the tests enumerate multiple ordering scenarios to prevent recurrence.\n\nbots/roles processing state must not corrupt workflow_run field handling. Deleting any of these tests would allow the original bug to regress silently.\n\nReferences:\n- §28032382734"}
|
|
@copilot run pr-finisher skill |
|
Please add the ADR link to the PR body and rerun the gate.
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd, /diagnose, and /zoom-out — requesting changes on test coverage gaps.
📋 Key Themes & Highlights
Key Issue: Regression Tests Don't Reproduce the Bug
The root cause is that activateEventSection() calls end with continue, which bypasses the indent-based array-tracker deactivation logic (lines ~464–528). This means stale inBotsArray / inRolesArray flags persist into the next event section's children — but only when those children use multi-line list syntax (- item). Lines using inline syntax (workflows: ["CI"]) are never matched by the inBotsArray && strings.HasPrefix(trimmedLine, "-") check.
All four new unit tests use inline array syntax and all two new validation tests have the extension field after workflow_run:. Neither configuration reproduces the actual failure path, so the tests would pass on the unfixed code.
What Needs to Change
- Add a unit test with multi-line
workflows:/types:syntax — specifically withbots:(orroles:,labels:,needs:) beforeworkflow_run:and multi-line arrays insideworkflow_run:. - Flip the extension-field ordering in the validation tests — put
bots:/roles:beforeworkflow_run:to match the real production scenario.
Positive Highlights
- ✅ Fix is surgical and correct —
resetOnArrayTrackers()is exactly the right leverage point. - ✅ Good coverage of all 4 sibling-ordering permutations conceptually — just needs the inline→multi-line syntax correction.
- ✅ Docs example locks in the supported
workflow_run+botscomposition pattern. - ✅ Resetting
inLabelsArrayandinNeedsArrayproactively covers those extension fields too.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 102.4 AIC · ⌖ 9.67 AIC · ⊞ 6.5K
| description: "Should not let bots array state leak into workflow_run fields", | ||
| }, | ||
| { | ||
| name: "bots before workflow_run do not comment workflow_run list items", |
There was a problem hiding this comment.
[/tdd] The test input uses inline array syntax workflows: ["CI"] and types: [completed], which does not exercise the failure path — the bug only activates when inBotsArray is stale and a line starts with -. This test would pass even without the fix.
💡 Suggested additional test case
Add a variant with multi-line list syntax so the test actually fails without the fix:
{
name: "bots before workflow_run with multi-line arrays do not comment workflow_run list items",
input: `on:
bots:
- dependabot
workflow_run:
workflows:
- CI
types:
- completed
workflow_dispatch:`,
expected: `on:
# bots: # Bots processed as bot check in pre-activation job
# - dependabot # Bots processed as bot check in pre-activation job
workflow_run:
workflows:
- CI
types:
- completed
workflow_dispatch:`,
description: "Should not comment out multi-line workflow_run.workflows/types items when bots precedes workflow_run",
},This directly triggers the inBotsArray && strings.HasPrefix(trimmedLine, "-") code path and will fail on the unfixed code.
| workflow_run: | ||
| workflows: ["build"] | ||
| types: [completed] | ||
| branches: |
There was a problem hiding this comment.
[/tdd] In both new validation test cases, bots: / roles: appears after workflow_run: in the YAML, so inBotsArray is only set after workflow_run's children have already been processed — the original failure mode required the extension field to come before workflow_run:.
💡 Flip the ordering to cover the real failure path
Change the frontmatter so bots: precedes workflow_run: and use multi-line workflows: syntax, e.g.:
---
on:
bots:
- dependabot
workflow_run:
workflows:
- build
types:
- completed
branches:
- main
tools:
github:
toolsets: [repos]
---This ordering (extension field first) is what causes the stale-flag leakage and is the scenario that was broken in production.
| workflowRunIndent = -1 | ||
| } | ||
| } | ||
| // resetOnArrayTrackers clears top-level on: extension array state when a new |
There was a problem hiding this comment.
[/diagnose] The comment accurately describes what the reset does, but the why — the mechanism that makes it necessary — is the real insight worth capturing. Without this context, a future developer adding a new event section might omit the call.
💡 Suggested comment wording
// resetOnArrayTrackers clears top-level on: extension-array state (inBotsArray,
// inRolesArray, etc.) before entering a new event section.
// This is required because each activateEventSection() call ends with "continue",
// which bypasses the indent-based deactivation logic at lines ~464-528. Without
// the explicit reset here, stale flags from a preceding bots:/roles:/labels: block
// would cause that event section's list items (e.g. workflow_run.workflows - CI)
// to be incorrectly commented out as bot/role/label entries.| - agent | ||
| if: > | ||
| always() && needs.agent.result != 'skipped' && (needs.agent.outputs.output_types != '' || needs.agent.outputs.has_patch == 'true') | ||
| if: always() && needs.agent.result != 'skipped' |
There was a problem hiding this comment.
[/zoom-out] This if: simplification is unrelated to the on.bots/on.roles state-leakage fix. Removing needs.agent.outputs.output_types != '' || needs.agent.outputs.has_patch == 'true' means the downstream job now runs whenever the agent wasn't skipped, regardless of whether it produced any output — a behavioural change with its own risk surface. Would be cleaner to land this independently with explicit reasoning.
…overage, revert smoke if Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
All review threads addressed in commit Code changes (
Tests (
Validation tests (
Smoke workflow (
|
workflow_runtriggers were being corrupted whenon.botsoron.rolesappeared in the sameon:block: list-item commenting logic leaked extension-array state and could strip effectiveworkflow_run.workflows/types, which then surfaced as the non-empty-workflows validation error in newer versions.Compiler fix: isolate
on:extension array state per event sectioncommentOutProcessedFieldsInOnSection, reset top-level extension array trackers before activating a new event section (pull_request,issues,discussion,issue_comment,deployment_status,workflow_run).bots/rolestracking from carrying intoworkflow_runparsing/commenting.Regression coverage:
workflow_run+ sibling auth filterson:comment-out tests for:workflow_runfollowed bybotsbotsbeforeworkflow_runrolesbeforeworkflow_runroles: allbeforeworkflow_runworkflow_run.workflowsandtypesremain intact/uncommented.Validation coverage: strict-mode compile path
workflow_runwith non-emptyworkflowspasses when siblingbotsorroles: allare present.Docs: supported composition made explicit
workflow_run+botsexample to lock in the supported pattern.