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]

Caution

Review failed

Failed to post review comments

📝 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 docstrings

• Create stacked PR
• Commit on current branch

🧪 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.11%. Comparing base (2862d16) to head (3bd7913).

Additional details and impacted files

@@            Coverage Diff             @@
##           v0.0.4     #137      +/-   ##
==========================================
+ Coverage   77.87%   78.11%   +0.24%     
==========================================
  Files         126      126              
  Lines        4555     4555              
==========================================
+ Hits         3547     3558      +11     
+ Misses       1008      997      -11     

Flag	Coverage Δ
spm	77.84% <ø> (ø)
swift-6.0	77.93% <ø> (?)
swift-6.1	78.02% <ø> (?)
swift-6.2	77.91% <ø> (?)
swift-6.3	77.87% <ø> (ø)
ubuntu	78.11% <ø> (+0.24%)	⬆️

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.