Fix: Prevent cancelled jobs from appearing as failures in CI

[leogdion]

Summary

• Add all-done aggregator job to prevent cancelled jobs appearing as failures
• Update CI workflow configuration and matrix structure
• Drop Swift 6.1 from Windows; exclude wasm for Swift <=6.1 on Ubuntu
• Restructure Ubuntu matrix to include WASM types; fix Android matrix
• Optimize CI workflows with dynamic matrix and cache cleanup

Test plan

• Verify CI passes on all platforms without false failures from cancelled jobs
• Confirm aggregator job correctly reflects overall CI status

🤖 Generated with Claude Code

---

Perform an AI-assisted review on

Summary by CodeRabbit

• New Features

  • Added CodeQL security analysis to CI/CD pipeline
  • Introduced cache cleanup workflows for branch and PR deletions
  • Enabled compiler concurrency warnings and strict safety checks
  • Added new module architecture with TokenVisitor and SyntaxParser components

• Chores

  • Updated GitHub Actions workflow versions and CI/CD configuration
  • Refactored internal module structure
  • Enhanced documentation with SwiftSyntax reference and examples
  • Updated swift-docc-plugin dependency

• Tests

  • Updated documentation test file path resolution

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 481880ac-df61-4f77-9545-df1f6db4a760

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

📝 Walkthrough

Walkthrough

This pull request refactors SyntaxKit's architecture by introducing new SyntaxParser and TokenVisitor modules, migrating types to a protocol-based design, establishing a semantic syntax classification system via SyntaxClassifiable, and updating CI workflows with concurrency management, cache cleanup automation, and CodeQL security analysis.

Changes

Cohort / File(s)	Summary
Workflow Configuration .github/workflows/SyntaxKit.yml, .github/workflows/cleanup-caches.yml, .github/workflows/cleanup-pr-caches.yml, .github/workflows/codeql.yml	Added/updated GitHub Actions workflows with concurrency groups, dynamic matrix builds, conditional job execution, cache cleanup automation for branches and PRs, and CodeQL security scanning with Swift language support.
Package Configuration Package.swift, Package.resolved, .periphery.yml	Enabled unsafe compiler flags for concurrency/testing, added new TokenVisitor and SyntaxParser library targets, updated skit executable dependency to SyntaxParser, upgraded swift-docc-plugin, and added retain_codable_properties configuration.
Core Protocol & Type Framework Sources/TokenVisitor/TreeNodeProtocol.swift, Sources/TokenVisitor/SyntaxClassifiable.swift, Sources/TokenVisitor/SyntaxType.swift, Sources/TokenVisitor/Token.swift, Sources/TokenVisitor/StructureProperty.swift, Sources/TokenVisitor/StructureValue.swift	Introduced protocol-based architecture with TreeNodeProtocol, SyntaxClassifiable protocol, SyntaxType enum (decl/expr/pattern/type/collection/other), and supporting data types (Token, StructureProperty, StructureValue) with Codable, Equatable, and Sendable conformances.
SyntaxClassifiable Conformances Sources/TokenVisitor/SyntaxClassifiable/*.swift (19 files)	Implemented SyntaxClassifiable conformance for SwiftSyntax types (AttributeListSyntax, CatchClauseListSyntax, CodeBlockItemListSyntax, ConditionElementListSyntax, DeclModifierListSyntax, DeclSyntax, ExprSyntax, FunctionParameterListSyntax, IdentifierPatternSyntax, IdentifierTypeSyntax, LabeledExprListSyntax, PatternBindingListSyntax, PatternSyntax, SwitchCaseItemListSyntax, SwitchCaseListSyntax, TypeSyntax, VariableDeclSyntax) to enable semantic classification.
Parser & Visitor Implementation Sources/SyntaxParser/SyntaxParser.swift, Sources/SyntaxParser/TreeNode.swift, Sources/TokenVisitor/TokenVisitor.swift, Sources/TokenVisitor/TreeNodeProtocol+Extensions.swift	Implemented new parsing entry point with public parse(code:) API returning [TreeNode], added TokenVisitor AST rewriter maintaining traversal state and semantic metadata, and added protocol extensions for tree construction and structure analysis.
Syntax Utilities Sources/SyntaxParser/SourceRange.swift, Sources/TokenVisitor/Syntax.swift, Sources/TokenVisitor/TriviaPiece.swift, Sources/TokenVisitor/String.swift	Added SourceRange struct for source location tracking, added Syntax extension properties (syntaxType, cleanClassName), added TriviaPiece extension for trivia serialization, and added String.empty constant.
CLI Entrypoint Sources/skit/Skit.swift, Sources/skit/main.swift	Refactored CLI from main.swift to @main entry point in Skit.swift, replaced stdin parsing with direct SyntaxParser.parse(code:) and JSON encoding, and removed old entrypoint file.
Module Cleanup & Removal Sources/SyntaxKit/Parser/Token.swift, Sources/SyntaxKit/Parser/TreeNode.swift, Sources/SyntaxKit/Parser/StructureProperty.swift, Sources/SyntaxKit/Parser/String.swift, Sources/SyntaxKit/Parser/TokenVisitor.swift, Sources/SyntaxKit/Parser/TokenVisitor+Helpers.swift	Removed internal types and utilities from SyntaxKit that were migrated to new modules.
Documentation & Tests CLAUDE.md, Sources/SyntaxParser/README.md, Tests/SyntaxDocTests/DocumentationExampleTests.swift, Tests/SyntaxDocTests/Settings.swift	Expanded CLAUDE.md with SwiftSyntax reference and examples, added SyntaxParser module README documenting architecture and JSON output, updated test file paths, and made test settings platform-conditional (Android/WASI-aware).

Sequence Diagram(s)

sequenceDiagram
    participant Client
    participant SyntaxParser
    participant SwiftSyntax as SwiftParser/SwiftSyntax
    participant TokenVisitor
    participant TreeNode
    participant JSONEncoder

    Client->>SyntaxParser: parse(code: String)
    SyntaxParser->>SwiftSyntax: SwiftParser.parse(source:)
    SwiftSyntax-->>SyntaxParser: SourceFileSyntax AST
    SyntaxParser->>TokenVisitor: TreeNodeProtocol.parseTree(from:)
    TokenVisitor->>TokenVisitor: Create SourceLocationConverter
    TokenVisitor->>TokenVisitor: Instantiate TokenVisitor<TreeNode>
    TokenVisitor->>SwiftSyntax: rewrite(syntax) via SyntaxRewriter
    loop for each Syntax node
        SwiftSyntax->>TokenVisitor: visitPre(node)
        TokenVisitor->>TreeNode: Create NodeType instance
        TokenVisitor->>TokenVisitor: appendStructure(from:allChildren:)
        TokenVisitor->>TokenVisitor: Link parent/child relationships
        SwiftSyntax->>TokenVisitor: visit(token) for leaf nodes
        TokenVisitor->>TreeNode: Store token kind + trivia
        SwiftSyntax->>TokenVisitor: visitPost(node)
        TokenVisitor->>TokenVisitor: Restore parent context
    end
    TokenVisitor-->>SyntaxParser: [TreeNode] array
    SyntaxParser->>JSONEncoder: encode([TreeNode])
    JSONEncoder-->>Client: [TreeNode] (or deprecated JSON)

Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related PRs

• Windows Support and README Updates #102: Updates .github/workflows/SyntaxKit.yml with Windows build job additions and CI matrix adjustments including action versions and job dependencies.
• Add SwiftSyntax LLM Documentation #104: Makes overlapping code-level changes to add TokenVisitor/SyntaxParser modules, migrate TreeNode/Token/StructureProperty types, and update Package.swift targets and compiler flags.
• Cleaning up Parser Code #103: Performs the same parser-rework refactoring to remove old SyntaxKit Parser types and introduce new SyntaxParser/TokenVisitor module APIs.

Poem

🐰 A rabbit hops through syntax trees,
Classifying declarations with ease!
Protocol patterns bloom anew—
TokenVisitor and Parser too!
From SyntaxKit to modular grace,
The codebase finds its perfect place. ✨

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 47.06% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately summarizes the main objective of the PR: preventing cancelled CI jobs from appearing as failures by introducing an aggregator job.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch 136-cancelled-false-failure

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 78.13%. Comparing base (93e0b5e) to head (bfa7cdd).
⚠️ Report is 1 commits behind head on v0.0.4.

Additional details and impacted files

@@            Coverage Diff             @@
##           v0.0.4     #137      +/-   ##
==========================================
+ Coverage   78.00%   78.13%   +0.13%     
==========================================
  Files         126      126              
  Lines        4555     4555              
==========================================
+ Hits         3553     3559       +6     
+ Misses       1002      996       -6     

Flag	Coverage Δ
spm	78.00% <ø> (+0.15%)	⬆️
swift-6.0	77.95% <ø> (?)
swift-6.1	77.87% <ø> (?)
swift-6.2	78.02% <ø> (?)
swift-6.3	77.89% <ø> (-0.11%)	⬇️
ubuntu	78.13% <ø> (+0.13%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[claude]

PR Review: Fix: Prevent cancelled jobs from appearing as failures in CI

The core change — the all-done aggregator job — is the right pattern for this problem. Using if: always() on the aggregator with branch protection keyed to a single "All Done" check is well-established for surviving cancel-in-progress cancellations. A few issues to flag:

---

Critical: Build failures silently bypass the aggregator

File: .github/workflows/SyntaxKit.yml, all-done job

If any build job (e.g. build-ubuntu) fails, the lint and docs jobs will be skipped — not failed — because their if condition uses !failure():

if: ${{ !cancelled() && !failure() && ... }}

When lint/docs are skipped, all-done checks contains(needs.*.result, 'failure') which evaluates to false (skipped is not failure). So all-done passes even when builds failed. This defeats the protection intended by requiring "All Done" as the branch protection check.

Fix option A — have all-done depend directly on the build jobs:

all-done:
  if: always()
  needs: [lint, docs, build-ubuntu, build-macos, build-macos-full, build-windows, build-android]

Fix option B — treat skipped lint/docs as a signal of upstream failure:

- name: Check for failures
  run: |
    lint_result="${{ needs.lint.result }}"
    docs_result="${{ needs.docs.result }}"
    failures="${{ contains(needs.*.result, 'failure') }}"
    if [[ "$failures" == "true" ]]; then
      echo "One or more required jobs failed."
      exit 1
    fi
    if [[ "$lint_result" == "skipped" || "$docs_result" == "skipped" ]]; then
      echo "lint or docs were skipped, likely due to an upstream build failure."
      exit 1
    fi

Option A is simpler and more robust — it covers any future jobs added to lint/docs's needs without needing to update all-done.

---

Minor: Shell expression interpolation style

The contains(needs.*.result, 'failure') expression is expanded directly into the run script and compared as a bash string. This is safe since it only produces true/false, but the native GHA conditional style avoids the shell comparison:

- name: Fail on job failures
  if: contains(needs.*.result, 'failure')
  run: |
    echo "One or more required jobs failed."
    exit 1
- name: All checks passed
  run: echo "All required checks passed, were skipped, or were cancelled."

---

cleanup-caches.yml — looks good

The ref_type !== 'branch' guard correctly skips tag deletions. Pagination handles repos with many caches. No issues.

cleanup-pr-caches.yml — looks good

Triggered on pull_request: [closed] which covers both merged and abandoned PRs. No issues.

codeql.yml — looks good

The paths-ignore correctly mirrors the main workflow so doc-only pushes won't trigger a full CodeQL scan. Consistent with the rest of CI.

---

Summary

Issue	Severity
Build failures bypass all-done aggregator	High — branch protection would not catch real failures
Shell expression interpolation style	Low / optional
Cleanup workflows, CodeQL workflow	No issues

The all-done pattern is correct; the gap is that the aggregator only observes lint/docs directly, but those jobs silently skip when builds fail (due to !failure() in their conditions). Adding the build jobs to all-done's needs list would close this hole.

[claude]

Code Review

Summary

The all-done aggregator pattern is the right approach for making required status checks work correctly with matrix/conditional jobs. A few things to consider.

---

Issues

1. needs only covers lint and docs — not the build jobs

The PR description says this fixes cancelled jobs appearing as failures, but the aggregator only monitors lint and docs. The build jobs (build-ubuntu, build-macos, build-windows, build-macos-full, build-android) are not included. If the intent is to have a single required check for branch protection, those should be included too — otherwise the fix is partial.

2. All-cancelled scenario passes silently

The current check only fails on failure. If every dependency is cancelled (e.g. an upstream job is skipped), all-done reports success with All required checks passed, were skipped, or were cancelled. Consider also requiring at least one success, so a fully-cancelled run does not green-light a merge.

3. Branch protection rules not updated

This job works as a required check only if All Done is added as a required status check in the GitHub repo branch protection settings. That is a manual step outside this PR — worth noting in the PR description or follow-up so it does not get forgotten.

---

Positives

• if: always() is correctly placed so the aggregator runs even when dependencies are skipped/cancelled
• The pattern itself is the standard/recommended approach for this problem in GitHub Actions
• Minimal, targeted change — easy to review and revert if needed

[claude]

PR Review: Fix: Prevent cancelled jobs from appearing as failures in CI

The approach is sound - an aggregator job with if: always() is the standard pattern for this GitHub Actions limitation. One significant concern:

Issue: Incomplete needs list in all-done

The aggregator job only watches [lint, docs]:

needs: [lint, docs]

But based on the existing workflow, there are several other critical jobs: build-ubuntu, build-macos, build-windows, build-macos-full, build-android. Failures in any of these jobs will not be captured by all-done, which means the PR could appear green even when a build job fails.

The needs array should include all jobs that must pass for a PR to be considered healthy.

Minor: Behaviour with cancelled build jobs

The comment "All required checks passed, were skipped, or were cancelled" is intentional per the PR title, but it is worth confirming this is the desired policy. If a build job is cancelled due to a previous failure (e.g. a prerequisite job failed), the aggregator will still report success. Depending on your branch protection rules, this may or may not be the right trade-off.

What's Good

• The if: always() condition correctly ensures this job always runs regardless of dependent job outcomes
• Using contains(needs.*.result, 'failure') is the right approach and cleanly handles the multi-job case
• This is a well-known pattern for fixing the GitHub Actions "cancelled = failure" problem

[claude]

Code Review: Prevent cancelled jobs from appearing as CI failures

Clear intent, well-known pattern. A few notes:

Positives

• if: always() on the aggregator is correct — without it the job skips when upstream is cancelled, defeating the purpose
• contains(needs.*.result, 'failure') correctly avoids treating cancelled/skipped as failures
• Depending on lint and docs (which aggregate build jobs) keeps the graph clean

Concerns

1. All-cancelled = green - If both lint and docs are cancelled, all-done exits 0. A PR could theoretically merge without any validation running. Consider also asserting at least one success:

if [[ "${{ contains(needs.*.result, 'success') }}" != "true" ]]; then
  echo "No jobs succeeded — all were cancelled or skipped."
  exit 1
fi

2. Dependency chain coverage - The aggregator only watches lint and docs. If a build job fails while lint is skipped, all-done passes. Worth verifying no failure path is missed.

3. Alternative: job-level if condition - Using if: always() && !contains(needs.*.result, 'failure') at the job level instead of shell interpolation avoids expression-in-shell concerns, though the check would show as skipped rather than passing when cancelled.

Summary: Pattern is sound. The main question is whether all-cancelled-passes is acceptable for branch protection policy here.

[claude]

Code Review

Summary: Adds an all-done aggregator job so that cancelled jobs do not surface as failures in PR checks. This is a well-known pattern for GitHub Actions and the implementation is correct.

What looks good:

• if: always() ensures the job runs even when upstream jobs are cancelled or skipped.
• contains(needs.*.result, 'failure') correctly distinguishes real failures from cancellations/skips — cancelled jobs report cancelled, not failure, so the exit 1 is only triggered by genuine failures.
• Depending on lint and docs (which already aggregate build results via their own needs) keeps the dependency graph shallow without losing coverage.

One question worth confirming: The aggregator depends on lint and docs. If either of those jobs is itself skipped (e.g., on a commit that bypasses via ci skip), needs.*.result for them will be skipped, not failure, so the aggregator will still exit 0 — which is likely the intended behavior. Worth a quick sanity check that the branch protection rule is set to require the All Done check specifically.

Verdict: Looks correct and addresses the false-failure problem cleanly. No changes needed.

[claude]

PR 137 Code Review - Fix: Prevent cancelled jobs from appearing as failures in CI

The implementation follows the standard pattern for this problem and is correct for the stated goal.

Logic analysis

• if: always() ensures it runs regardless of upstream cancellations
• Only fails on failure; cancellations and skips pass through
• needs: [lint, docs] covers all build jobs transitively if lint and docs aggregate them

Minor gap: all-skipped case

If both lint and docs are skipped due to event filtering, all-done will succeed with no jobs having actually run. A stricter check would also verify contains(needs.*.result, 'success'). Whether this matters depends on the branch protection configuration.

Important follow-up action

The fix is only effective if All Done is configured as the sole required status check in branch protection. Without that, individual cancelled job statuses will still be evaluated. Consider adding a comment in the workflow documenting this requirement so future maintainers understand why the aggregator exists.

No test changes needed - purely CI configuration.

[claude]

Code Review: Fix - Prevent cancelled jobs from appearing as failures in CI

This is a targeted, well-understood fix for a common GitHub Actions pain point.

Positives

• Using if: always() on the aggregator is the correct pattern to ensure it runs even when upstream jobs are cancelled.
• Only checking for 'failure' (not 'cancelled' or 'skipped') is intentional and correct — cancelled jobs are treated as non-blocking, which is exactly the stated goal.
• The echo messages clearly communicate what state was detected, which helps during debugging.
• Depending on lint and docs (which themselves aggregate the build matrix jobs) is the right dependency chain rather than listing all build jobs directly.

Issues / Suggestions

1. No explicit check that lint and docs both succeeded: The current logic only fails on 'failure', meaning if both lint and docs are somehow skipped or cancelled, the aggregator reports success. This is probably intentional given the PR's goal, but worth confirming that branch protection requires this job and that a cancellation-only run is acceptable.

2. Consider checking for unexpected states: A more defensive version would also fail if neither lint nor docs reported 'success' (i.e., treat unexpected outcomes cautiously). For example:
   if [[ "${{ contains(needs..result, 'failure') }}" == "true" ]] || [[ "${{ !contains(needs..result, 'success') && github.event_name != 'pull_request' }}" == "true" ]]
   But this is an optional hardening — the current approach is reasonable for the stated use case.

3. Job name "All Done" vs check name: Make sure branch protection is configured to require "All Done" (the job name) as the required status check. Otherwise this aggregator provides no protection.

Summary: This is correct and follows the standard GitHub Actions aggregator pattern. The concern about cancellation-only runs is edge-case and likely acceptable given the PR's intent. Approve.

[claude]

Title: PR Review: Fix: Prevent cancelled jobs from appearing as failures in CI

Content:
This is a clean, targeted fix for a real CI pain point. The implementation looks correct. One thing to consider:

Incomplete needs list in all-done job
The aggregator job currently only checks [lint, docs]. If build jobs (build-ubuntu, build-macos, build-windows, etc.) fail, the all-done gate will still pass. Depending on whether those jobs are required checks on the branch protection rules, this may mean the PR can be merged even with a failed build. Consider adding all required jobs to the needs list so the aggregator truly represents overall CI health.

Positives

• Using if: always() on the aggregator is the correct pattern
• contains(needs.*.result, 'failure') correctly ignores cancelled/skipped jobs without treating them as failures
• Minimal diff — only touches what is needed

The PR achieves its stated goal. The main suggestion is to widen the needs list if build jobs should also be gated.

[claude]

PR Review: Fix: Prevent cancelled jobs from appearing as failures in CI

The aggregator pattern is the correct solution here — using if: always() with an explicit failure check is the standard way to prevent cancelled concurrent runs from polluting PR status checks.

---

Correctness

all-done dependency chain

The job depends on [lint, docs]. This works transitively only if lint and docs themselves depend on all the build jobs (build-ubuntu, build-macos, build-windows, etc.). Worth verifying this holds — if a build job fails but lint/docs are skipped rather than failed (e.g. due to an if: guard), all-done would incorrectly pass.

Cancelled jobs treated as success — this is intentional and correct per the PR description. The contains(needs.*.result, 'failure') check explicitly allows cancelled and skipped results through.

---

Minor Issues

PR description / CodeRabbit summary is misleading

The auto-generated CodeRabbit summary mentions features that don't appear in this diff: CodeQL security analysis, cache cleanup workflows, new module architecture (TokenVisitor, SyntaxParser), updated swift-docc-plugin dependency. These appear to be from an earlier, broader version of the branch. The actual changeset is just 14 lines adding the all-done job. Consider updating the PR description to reflect the current scope to avoid reviewer confusion.

---

Nits

• The success message "All required checks passed, were skipped, or were cancelled." accurately describes the semantics — good.
• runs-on: ubuntu-latest is appropriate for this lightweight aggregator job.

---

Summary: The change is correct and minimal. Main ask is to verify the transitive dependency chain covers all required build jobs, and to clean up the PR description.

[claude]

Code Review

Overview

This PR adds an all-done aggregator job to prevent cancelled CI jobs from appearing as failures. The change is small (14 lines, no deletions) and targeted.

What Works Well

• if: always() on the aggregator is the correct pattern. Without it, a cancelled upstream job would skip the aggregator entirely.
• Failing only on failure (not cancelled or skipped) correctly treats cancelled jobs as non-blocking.

Issues and Suggestions

1. Aggregator may miss upstream build failures

all-done only lists needs: [lint, docs]. But lint and docs both use if: ${{ !cancelled() && !failure() ... }}, so they get skipped when a build job fails. all-done then sees skipped and passes — even though a real failure occurred upstream.

Recommended fix: include all build jobs in needs:

all-done:
  needs: [lint, docs, build-ubuntu, build-macos, build-windows, build-macos-full, build-android]

2. Skipped jobs silently treated as success

The success message groups passed, skipped, and cancelled jobs together, but these have meaningfully different causes. A skipped job often signals an upstream failure. Consider whether unexpected skips should also trigger a failure.

3. Branch protection rules

For this aggregator to protect branches, the All Done check needs to be added as a required status check in GitHub repository settings. Worth confirming this is configured.

Summary

The core approach is correct. The main concern is that upstream build failures may not reach all-done through the intermediary lint/docs jobs, creating a blind spot. Adding the build jobs directly to needs would close that gap.

[claude]

Code Review

PR: Fix: Prevent cancelled jobs from appearing as failures in CI

Summary of Change

The final diff is a focused 6-line change to the push trigger in .github/workflows/SyntaxKit.yml.

Before: branches-ignore: ['*WIP'] - runs on all branch pushes except WIP branches
After: branches: [main] + tags: [v*.*.*] - runs only on main and release tags

---

Feedback

Correctness - addresses the root cause well

Switching from an exclusion list to an explicit allowlist is the right fix. Cancelled jobs are typically caused by concurrent runs on short-lived feature branches triggering CI unnecessarily. Restricting push-based CI to main and release tags eliminates most of those concurrent runs at the source rather than papering over them with aggregator jobs (the earlier approach shown in the commit history).

Potential concern - PR checks

With this change, push events on feature branches no longer trigger the workflow at all. CI will now only run on:

1. pull_request: events (if configured - worth verifying this is in the full trigger block)
2. Pushes to main
3. Version tag pushes

If pull_request: triggers exist in the file (which appears to be the case), this is fine. If not, PRs would merge without any CI validation - a regression worth checking before merging.

Simplification win

The commit history shows an earlier, more complex approach (an all-done aggregator job with if: always() logic) was drafted then abandoned in favor of this simpler fix. Good call - fewer moving parts, and fixing the trigger is cleaner than masking results at aggregation time.

Minor: tag pattern coverage

The pattern v[0-9]*.[0-9]*.[0-9]* will match pre-release tags like v1.2.3-beta.1 (no end anchor, [0-9]* allows zero digits). If pre-release tags should not trigger full CI, consider v[0-9]+.[0-9]+.[0-9]+. Otherwise fine to leave as-is if intentional.

---

Verdict

Correct and well-motivated change. Primary thing to verify before merging: pull_request: triggers are present in the workflow file so feature-branch CI still runs on PRs.