Enforce compile-time rejection of queue: max with cancel-in-progress: true in concurrency config#33353
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>
queue: max with cancel-in-progress: true in concurrency config
There was a problem hiding this comment.
Pull request overview
Enforces a compile-time concurrency invariant so that queue: max cannot be combined with cancel-in-progress: true in serialized concurrency YAML, preventing invalid GitHub Actions workflow configs from compiling.
Changes:
- Added
validateConcurrencyQueueConfiguration(...)to detect the forbiddenqueue: max+cancel-in-progress: truecombination. - Wired the new validation into compiler validation for both workflow-level
concurrencyandengine.concurrency. - Added unit + integration tests covering acceptance/rejection cases at both config levels.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/concurrency_validation.go | Adds queue/cancel compatibility validation via regex checks over serialized concurrency YAML. |
| pkg/workflow/compiler_validators.go | Calls the new validation for workflow-level and engine-level concurrency before group-expression validation. |
| pkg/workflow/concurrency_validation_test.go | Adds focused unit tests for the invalid pair and allowed combinations. |
| pkg/workflow/concurrency_validation_integration_test.go | Adds integration cases ensuring compile-time failure for invalid configs at both levels. |
| .github/workflows/smoke-otel-backends.lock.yml | Updates Datadog MCP header env-var reference to DD_APPLICATION_KEY. |
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: 0
|
@copilot Lint Go and fix issues. Lint JavaScript and fix issues. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd and /diagnose based on the validation enforcement and test coverage in this PR.
Key Themes
- Test coverage: Solid foundation with 5 test cases, but a few edge cases are missing (empty strings, omitted
cancel-in-progress, whitespace-only input) - Observability: Adding debug logging for partial matches would improve diagnosability
Positive Highlights
✅ Excellent test structure — Unit tests + integration tests at both workflow and engine levels
✅ Clear error messages — The validation error provides actionable guidance
✅ Consistent validation placement — Applied symmetrically to both concurrency config locations
✅ Good separation of concerns — New validation function is focused and reusable
✅ Comprehensive test coverage — Both valid and invalid scenarios are tested
Verdict
This is a clean, well-tested validation enhancement. The suggestions above are minor improvements around edge case coverage and diagnostic robustness — none are blocking. The core logic is sound and the PR delivers exactly what the title promises.
Approving with confidence! 🚀
Learn more about the skills applied:
/tddskill — Test-driven development patterns/diagnoseskill — Debugging and validation best practices
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · ● 2.9M
| queue: max`, | ||
| wantErr: false, | ||
| }, | ||
| { |
There was a problem hiding this comment.
[/tdd] Missing test case: queue: max without explicit cancel-in-progress.
You have "queue max with cancel false is allowed" on line 783, but consider also testing the omitted case:
{
name: "queue max without cancel-in-progress is allowed",
concurrency: `concurrency:
group: "my-group"
queue: max`,
wantErr: false,
},This tests the common case where users don't explicitly set cancel-in-progress (it defaults to false in GitHub Actions). The current test only covers the explicit false case.
| }, | ||
| } | ||
|
|
||
| for _, tt := range tests { |
There was a problem hiding this comment.
[/tdd] Missing edge case: empty string after whitespace normalization.
Consider adding a test case for empty/whitespace-only concurrency strings to verify the early return path:
{
name: "empty concurrency string is allowed",
concurrency: "",
wantErr: false,
},
{
name: "whitespace-only concurrency is allowed",
concurrency: " \n \t ",
wantErr: false,
},Explicitly testing boundary conditions makes the contract clearer and prevents regressions if the empty-check logic changes.
| return nil | ||
| } | ||
|
|
||
| if concurrencyQueueMaxPattern.MatchString(concurrencyYAML) && |
There was a problem hiding this comment.
[/diagnose] Consider adding debug logging for partial matches.
The current implementation silently succeeds if only one pattern matches. While correct, this makes debugging harder if the regex patterns are wrong. Consider logging when matches occur:
hasQueueMax := concurrencyQueueMaxPattern.MatchString(concurrencyYAML)
hasCancelTrue := concurrencyCancelInProgressTruePattern.MatchString(concurrencyYAML)
if hasQueueMax || hasCancelTrue {
concurrencyValidationLog.Printf("queue:max=%v cancel:true=%v", hasQueueMax, hasCancelTrue)
}
if hasQueueMax && hasCancelTrue {
return NewValidationError(...)
}This makes it easier to diagnose regex issues during development and provides useful debug logs.
|
Commit pushed:
|
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (>100 new lines in AI has analyzed the PR diff and generated a draft ADR to help you get started: 📄 Draft ADR: The draft has been committed directly to this branch and captures the decision (compile-time rejection of 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 Matter
ADRs 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
|
🧪 Test Quality Sentinel ReportTest Quality Score: 80/100✅ Excellent test quality
Test Classification Details
Test Details📋 View Comprehensive Test AnalysisTestValidateConcurrencyQueueConfiguration (Unit Test)File: Design Invariant Enforced: GitHub Actions' constraint that Test Cases (5 total):
Coverage:
What would break if deleted? The validation function Issues:
Integration Test Cases (2 added)File: Design Invariant Enforced: The queue/cancel-in-progress constraint applies at both workflow-level and engine-level concurrency configurations Test Cases (2 total):
Coverage:
What would break if deleted? The integration between the validation function and the compiler's frontmatter parser could regress, allowing invalid configurations to pass through even if the validation function itself is correct. Flagged Tests — Review Suggested
|
|
@copilot review all comments. |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done in 171cb2d.
|
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Completed in 3412b3e. Addressed all actionable comments:
Re-validated with |
This change enforces a missing concurrency invariant in the compiler:
queue: maxmust not be combined withcancel-in-progress: true. The validation now fails fast for both workflow-level and engine-level concurrency blocks.What changed
validateConcurrencyQueueConfiguration(...)to detect the invalid combination in serialized concurrency YAML.concurrencyengine.concurrencyCoverage updates
Behavioral example
This now produces a compiler validation error (instead of allowing an invalid config through).