Extend stacked PR max-stack gating to pull_request_review - #49453
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
max-stack gating to pull_request_review
|
@copilot ensure the max stack allows for intermediate value. - inequality |
There was a problem hiding this comment.
Pull request overview
Extends stacked-PR gating to pull_request_review workflows, including schema, compiler output, documentation, and tests.
Changes:
- Adds review-trigger detection and
max-stackextraction. - Adds schema validation and frontmatter cleanup.
- Regenerates affected workflow lock files and expands tests.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/frontmatter_on_section_cleanup.go |
Cleans review-event max-stack. |
pkg/workflow/filters.go |
Implements review stack gating. |
pkg/workflow/filters_stack_test.go |
Adds filter unit tests. |
pkg/workflow/filters_stack_integration_test.go |
Adds compilation integration tests. |
pkg/workflow/compiler_draft_test.go |
Tests frontmatter cleanup. |
pkg/parser/schemas/main_workflow_schema.json |
Defines review max-stack. |
pkg/parser/schema_location_test.go |
Tests schema value constraints. |
docs/src/content/docs/reference/triggers.md |
Documents review stack filtering. |
.github/workflows/visual-regression-checker.lock.yml |
Regenerates stack condition. |
.github/workflows/test-quality-sentinel.lock.yml |
Regenerates stack condition. |
.github/workflows/smoke-update-cross-repo-pr.lock.yml |
Regenerates stack condition. |
.github/workflows/smoke-test-tools.lock.yml |
Regenerates stack condition. |
.github/workflows/smoke-temporary-id.lock.yml |
Regenerates stack condition. |
.github/workflows/smoke-project.lock.yml |
Regenerates stack condition. |
.github/workflows/smoke-pi.lock.yml |
Regenerates stack condition. |
.github/workflows/smoke-opencode.lock.yml |
Regenerates stack condition. |
.github/workflows/smoke-multi-pr.lock.yml |
Regenerates stack condition. |
.github/workflows/smoke-gemini.lock.yml |
Regenerates stack condition. |
.github/workflows/smoke-create-cross-repo-pr.lock.yml |
Regenerates stack condition. |
.github/workflows/smoke-copilot-arm.lock.yml |
Regenerates stack condition. |
.github/workflows/smoke-codex.lock.yml |
Regenerates stack condition. |
.github/workflows/smoke-claude.lock.yml |
Regenerates stack condition. |
.github/workflows/smoke-ci.lock.yml |
Regenerates stack condition. |
.github/workflows/smoke-call-workflow.lock.yml |
Regenerates stack condition. |
.github/workflows/smoke-antigravity.lock.yml |
Regenerates stack condition. |
.github/workflows/smoke-agent-scoped-approved.lock.yml |
Regenerates stack condition. |
.github/workflows/smoke-agent-public-none.lock.yml |
Regenerates stack condition. |
.github/workflows/smoke-agent-public-approved.lock.yml |
Regenerates stack condition. |
.github/workflows/smoke-agent-all-none.lock.yml |
Regenerates stack condition. |
.github/workflows/smoke-agent-all-merged.lock.yml |
Regenerates stack condition. |
.github/workflows/refiner.lock.yml |
Regenerates stack condition. |
.github/workflows/pr-description-caveman.lock.yml |
Regenerates stack condition. |
.github/workflows/pr-code-quality-reviewer.lock.yml |
Regenerates stack condition. |
.github/workflows/mattpocock-skills-reviewer.lock.yml |
Regenerates stack condition. |
.github/workflows/impeccable-skills-reviewer.lock.yml |
Regenerates stack condition. |
.github/workflows/firewall-escape.lock.yml |
Regenerates stack condition. |
.github/workflows/design-decision-gate.lock.yml |
Regenerates stack condition. |
.github/workflows/changeset.lock.yml |
Regenerates stack condition. |
.github/workflows/ai-moderator.lock.yml |
Regenerates stack condition. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 39/39 changed files
- Comments generated: 1
- Review effort level: Balanced
| case float64: | ||
| if v == float64(int(v)) { | ||
| func extractStackMaxStackFromMap(onMap map[string]any) (int, bool) { | ||
| for _, triggerName := range []string{"pull_request", "pull_request_review"} { |
|
✅ Test Quality Sentinel completed test quality analysis. Warning threat detection engine error DetailsThe threat detection engine failed to produce results. Review the workflow run logs for details. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ Warning threat detection engine error DetailsThe threat detection engine failed to produce results. Review the workflow run logs for details. |
|
✅ PR Code Quality Reviewer completed the code quality review. Warning threat detection engine error DetailsThe threat detection engine failed to produce results. Review the workflow run logs for details. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Design Decision Gate — ADR RequiredThis PR makes significant changes to core workflow compilation logic (261 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. 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
|
|
Warning threat detection engine error DetailsThe threat detection engine failed to produce results. Review the workflow run logs for details. 🧪 Test Quality Sentinel Report
📊 Metrics (7 tests)
📋 Test Inventory
✅ VerdictPassed. 29% implementation tests (threshold: 30%). No violations. Solid behavioral coverage with table-driven validation (5 scenarios), end-to-end integration test, and comprehensive trigger-form testing. Quality signals:
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Warning
threat detection engine error
The threat detection engine encountered an error and could not complete analysis. This is a tooling failure, not a security finding.
Details
The threat detection engine failed to produce results.
Review the workflow run logs for details.
The stack-filter extension to pull_request_review looks correct.
github.event.pull_requestis present inpull_request_reviewwebhook payloads, so accessing.stack.positionand.stack.sizeis valid.- The compiler in
filters.gogenerates the updated condition correctly, andfilters_stack_test.goadds test coverage for the new trigger type. - All lock files are mechanically regenerated with a consistent pattern.
No blocking issues found.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 62.8 AIC · ⊞ 5.3K
There was a problem hiding this comment.
Warning
threat detection engine error
The threat detection engine encountered an error and could not complete analysis. This is a tooling failure, not a security finding.
Details
The threat detection engine failed to produce results.
Review the workflow run logs for details.
Verdict: Approve with one correctness concern
Overall the extension of stack gating to pull_request_review is implemented consistently (trigger detection, condition strings, PreStep gate, schema, frontmatter cleanup) and is well covered by new unit/integration tests.
💡 Themes
- Blocking-worthy but scoped:
extractStackMaxStackFromMapresolvesmax-stackby checkingpull_requestbeforepull_request_reviewand returning on first match — if a workflow configures both triggers with differentmax-stackvalues, the value is silently collapsed to whichever trigger is checked first, regardless of which event actually fires. No validation catches this mismatch. Flagged inline. - Condition/PreStep generation for the dual-event case (
github.event_name == 'pull_request' || 'pull_request_review') itself looks correct. - Docs, schema, and lock-file regeneration are consistent with the code changes.
Not requesting changes since this is a narrow edge case (dual-trigger workflows with divergent max-stack values) rather than a regression of the primary single-trigger behavior this PR targets, but it should be tracked and probably fixed before this ships broadly.
🔎 Code quality review by PR Code Quality Reviewer · auto · 42.6 AIC · ⊞ 7.8K
Comment /review to run again
| case float64: | ||
| if v == float64(int(v)) { | ||
| func extractStackMaxStackFromMap(onMap map[string]any) (int, bool) { | ||
| for _, triggerName := range []string{"pull_request", "pull_request_review"} { |
There was a problem hiding this comment.
When both pull_request and pull_request_review triggers are configured on the same workflow with different max-stack values, this always picks pull_request's value regardless of which event actually fires at runtime — silently mis-gating the other trigger.
💡 Details
extractStackMaxStackFromMap iterates ["pull_request", "pull_request_review"] and returns on the first match. Since maxStack is baked into the compiled condition/PreStep as a static int (it can't vary per github.event_name at runtime), a workflow like:
on:
pull_request:
max-stack: 1
pull_request_review:
max-stack: 3will use max-stack: 1 for both event types, even for pull_request_review runs — silently gating away review runs that the author intended to allow through the top 3 stack layers. No validation error is raised for this mismatch, and no test in this PR covers the dual-trigger case.
Suggested fix: either (a) require equal max-stack values across pull_request/pull_request_review when both are present (schema/compile-time validation error on mismatch), or (b) resolve the value per-trigger by embedding both values in the generated condition (e.g., via a ternary on github.event_name) instead of collapsing to a single static int.
There was a problem hiding this comment.
Warning
threat detection engine error
The threat detection engine encountered an error and could not complete analysis. This is a tooling failure, not a security finding.
Details
The threat detection engine failed to produce results.
Review the workflow run logs for details.
Skills-Based Review 🧠
Applied /tdd and /codebase-design — requesting changes for two test coverage gaps and one undocumented contract.
📋 Key Themes & Highlights
Key Themes
- Test coverage gap (positive assertion): The integration test for
pull_request_reviewdefaultmax-stack: 1only checks that no arithmetic appears; it never asserts the event name is present in the generated condition. A regression dropping the event would pass silently. - Test coverage gap (disabled path):
max-stack: -1is only tested forpull_request; the equivalent forpull_request_reviewis untested. - Undocumented priority:
extractStackMaxStackFromMapsilently givespull_requestpriority overpull_request_reviewwhen both are present with different values.
Positive Highlights
- ✅ Clean refactor from
hasPullRequestTrigger/extractPullRequestMaxStackto unified helpers — handles string, array, and mapon:forms consistently. - ✅ Schema validation with
anyOfcorrectly rejects0and invalid negatives. - ✅ Pre-step approach for
max-stack > 1avoids GitHub Actions arithmetic limitation — well-tested end-to-end. - ✅ Lock files regenerated correctly.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 67.4 AIC · ⊞ 7K
Comment /matt to run again
Comments that could not be inline-anchored
pkg/workflow/filters_stack_integration_test.go:113
[/tdd] The integration test for pull_request_review with default max-stack: 1 only asserts absence of arithmetic — it has no positive assertion that the gating condition is actually applied (e.g., that pull_request_review appears in the generated if:).
<details>
<summary>💡 Suggested fix</summary>
Scope a positive assertion to the pull_request_review row, e.g.:
if strings.Contains(tt.name, "pull_request_review") {
require.Contains(t, lockStr, "github.event_name != 'pu…
</details>
<details><summary>pkg/workflow/filters_stack_test.go:88</summary>
**[/tdd]** There is no test for `pull_request_review` with `max-stack: -1` — the only disabled-gating test uses `pull_request`. A regression in `extractStackMaxStackFromMap`'s iteration order could silently break the disable path for review events.
<details>
<summary>💡 Suggested test</summary>
```go
func TestApplyPullRequestStackFilter_PullRequestReviewDisabled(t *testing.T) {
compiler := NewCompiler()
workflowData := &WorkflowData{If: "github.actor != 'dependabot[bot]'"}
frontma…
</details>
<details><summary>pkg/workflow/filters.go:306</summary>
**[/codebase-design]** `extractStackMaxStackFromMap` iterates `["pull_request", "pull_request_review"]` and returns the first match, so `pull_request`'s `max-stack` always wins when both triggers are present in the same map. This silent priority rule is undocumented and untested.
<details>
<summary>💡 Options</summary>
Either add a comment documenting the intended priority (if it's deliberate), or add a test covering the mixed-trigger case:
```go
// When both pull_request and pull_request_re…
</details>
|
@copilot run pr-finisher skill |
… intermediate values Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done in commit |
|
🎉 This pull request is included in a new release. Release: |
Stacked PR support previously gated
pull_requestworkflows but leftpull_request_reviewworkflows ungated, so review-triggered runs still executed on lower stack layers. This PR aligns review-event behavior with the existing stack filter contract.Trigger/filter behavior
pull_request_reviewevents.pull_request:max-stack: 1→ top-of-stack onlymax-stack: N→ topNlayersmax-stack: -1→ disable stack gatingmax-stackextraction paths so object/arrayon:forms behave consistently.Schema contract
on.pull_request_review.max-stackto the main workflow schema.-1or>= 1; rejects0and other invalid negatives).Compiler output / frontmatter cleanup
max-stackunderpull_request_reviewas compiler-processed in the rendered/commentedon:section..lock.ymlworkflows to reflect compiled gating behavior.Coverage
pull_request_reviewdefault and configuredmax-stackif:constraintsmax-stack > 1max-stackvalues