Skip to content

ci: add CI workflow to build and test on push and PR#12

Merged
atom2ueki merged 3 commits into
mainfrom
ci/tests-on-push
May 1, 2026
Merged

ci: add CI workflow to build and test on push and PR#12
atom2ueki merged 3 commits into
mainfrom
ci/tests-on-push

Conversation

@atom2ueki

Copy link
Copy Markdown
Owner

Summary

  • Adds .github/workflows/ci.yml that runs swift build + swift test on push to main and on pull requests.
  • Matrix runs on macos-14 and macos-15 with fail-fast: false to surface per-OS regressions.
  • Adds a concurrency group so older runs on the same ref are cancelled when new commits land.

Why

Closes the gap raised in #11: until now the only workflow was claude.yml (the @claude review bot), so nothing was actually compiling the package or running the test suite when commits hit main or PRs. That left the existing 46 tests un-enforced in CI — regressions could merge unchecked.

Test plan

  • swift build passes locally (worktree, Swift 6.3.1 / macOS 26).
  • swift test passes locally: 46 tests, 0 failures.
  • First CI run on this PR will demonstrate the workflow itself works on macos-14 and macos-15.

Closes #11

@atom2ueki

Copy link
Copy Markdown
Owner Author

@claude @codex please review.

Specific things to sanity-check:

  • Whether the runner matrix (macos-14 + macos-15) is the right call given the package declares only Apple platforms (.iOS(.v15), .macOS(.v12)) and uses CFSocket. Is there value in adding an iOS xcodebuild job, or is that overkill for a CI gate at this stage?
  • Whether the trigger surface (push: main + pull_request: main) is right, or whether pull_request should fire for any base branch.
  • Any reason the concurrency group key should include the workflow name as well to avoid clashing with future workflows on the same ref.

@atom2ueki atom2ueki requested a review from Copilot May 1, 2026 15:57
@claude

claude Bot commented May 1, 2026

Copy link
Copy Markdown

Claude finished @atom2ueki's task in 1m 39s —— View job


CI Workflow Review

  • Read .github/workflows/ci.yml
  • Read Package.swift for platform declarations
  • Analyze runner matrix (macos-14 vs macos-15)
  • Analyze trigger surface (push: main + pull_request)
  • Analyze concurrency group key design
  • Post full review

Overall the workflow is clean and does the right thing. Three targeted answers to your questions, plus one minor flag:


1. Runner matrix — iOS xcodebuild job?

macos-14 + macos-15 is the right call for now. Here's why:

  • swift build / swift test on a macOS runner compiles and runs the test suite natively. That exercises the thing you actually care about — your 46 tests passing.
  • CFSocket is a BSD socket abstraction. Listening servers built on it work fine on macOS but are heavily sandbox-restricted on iOS. An iOS Simulator test job would verify that the package compiles for iOS (since you declare .iOS(.v15)) but the tests themselves couldn't run meaningfully — you'd need to strip or skip all server-side test cases, which defeats the purpose.
  • The value of an iOS xcodebuild -destination 'platform=iOS Simulator,...' build-only check is that it catches API availability drift (e.g., if someone adds a macOS-only API call without #if canImport(UIKit) guards). That's a real class of bug, but for a web server library it's an edge case that rarely shows up in practice.

Recommendation: Defer the iOS job. When/if the library grows iOS-specific API surface worth protecting, add a separate build-only step:

- name: Build for iOS Simulator
  run: xcodebuild build -scheme SwiftWebServer -destination 'platform=iOS Simulator,name=iPhone 16'

Keep it separate from the swift test job and label it clearly as build-only to avoid confusion.


2. Trigger surface — pull_request: branches: [main] vs open?

The branches filter on pull_request matches the base branch of the PR. So the current config fires CI only when a PR targets main.

For a repo with a single long-lived branch this is fine today, but it has one latent footgun: stacked PRs. If someone opens PR A targeting a feature branch, and PR B targeting main, CI will run on B but not A. That tends to surprise people.

Recommendation: Drop the branches filter from pull_request:

on:
  push:
    branches: [main]
  pull_request:        # fire on all PRs regardless of target branch

The push: branches: [main] still limits post-merge runs to main. This is the safer default and requires no future adjustment if you ever add release or feature branches.


3. Concurrency group key — should it include the workflow name?

Yes, it should. The current key:

group: ci-${{ github.ref }}

uses a hardcoded prefix ci-. If a second workflow (e.g., lint.yml) uses the same hardcoded prefix, the two workflows will cancel each other on the same ref — a silent, hard-to-diagnose interaction.

The idiomatic fix uses ${{ github.workflow }}, which resolves to the workflow's name: field ("CI" here) at runtime:

concurrency:
  group: ${{ github.workflow }}-${{ github.ref }}
  cancel-in-progress: true

This namespaces each workflow's concurrency group independently. It's a no-cost improvement and the accepted community standard. Fix this →


Minor flag — actions/checkout@v6

.github/workflows/ci.yml:22 uses actions/checkout@v6. As of my knowledge cutoff the stable release was v4; verify that v6 is an actual stable tag and not a pre-release or typo before merging. Using an unpinned major tag (@v4, @v6) is acceptable for simplicity, but pinning to a SHA is the security-hardened alternative if that ever becomes a policy requirement.


Summary of suggested changes (priority order):

# Change Impact
1 group: ${{ github.workflow }}-${{ github.ref }} Prevents future cross-workflow cancellations
2 Drop branches: [main] from pull_request Ensures CI on all PRs, not just those targeting main
3 Verify actions/checkout@v6 is a real stable tag Avoids silent breakage if tag doesn't exist
4 iOS xcodebuild job Defer — not worth the complexity at this stage

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds a GitHub Actions CI workflow to ensure the Swift package is built and its test suite runs automatically on pushes to main and on pull requests targeting main, addressing the gap noted in #11.

Changes:

  • Introduces a new CI workflow that runs swift build and swift test.
  • Runs the workflow on a macOS matrix (macos-14, macos-15) with fail-fast: false.
  • Adds concurrency cancellation to avoid redundant runs on the same ref.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread .github/workflows/ci.yml Outdated
Comment on lines +3 to +7
on:
push:
branches: [main]
pull_request:
branches: [main]

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 822703b7ca

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread .github/workflows/ci.yml Outdated
branches: [main]

concurrency:
group: ci-${{ github.ref }}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Scope concurrency group to this workflow

Using ci-${{ github.ref }} at workflow scope can cancel runs from other workflows that later reuse the same group pattern on the same ref, because GitHub Actions concurrency groups are repository-wide. In this repository, that means a future workflow (for example release/deploy on main) could unintentionally cancel CI runs or be canceled by them, leading to skipped checks; include github.workflow in the key to isolate cancellation behavior per workflow.

Useful? React with 👍 / 👎.

@atom2ueki

Copy link
Copy Markdown
Owner Author

Addressed review round 1:

  • Concurrency group (Claude Refactor: Restructure project for SPM and Express-like API #1, Codex P2): now ${{ github.workflow }}-${{ github.ref }} so future workflows on the same ref don't collide.
  • PR triggers (Claude chore(deps): bump github.com/realm/swiftlint from 0.59.1 to 0.63.2 #2): dropped branches: [main] from pull_request so CI fires on PRs targeting any base branch (e.g. stacked PRs).
  • Least-privilege permissions (Copilot inline): added top-level permissions: contents: read — the workflow only reads code, no token scope beyond that needed.
  • actions/checkout@v6 (Claude minor): leaving as-is. The first CI run on commit 822703b resolved @v6 and ran successfully on both macos-14 and macos-15, so the tag is real and matches the existing pattern in claude.yml.
  • iOS xcodebuild job (Claude ci: add Claude Code @claude-mention reviewer workflow #4): deferring per Claude's own recommendation — reconsider when iOS-specific surface lands.

@claude @codex re-review please.

@claude

claude Bot commented May 1, 2026

Copy link
Copy Markdown

Claude finished @atom2ueki's task in 1m 7s —— View job


Re-review — Round 2

  • Read current .github/workflows/ci.yml
  • Verify concurrency group fix
  • Verify PR trigger fix
  • Verify permissions addition
  • Check for any remaining issues
  • Post full re-review

All three round 1 items were addressed correctly. Here's the verification plus two low-priority optional suggestions.


Changes verified ✅

Concurrency group (831f0a6)

# before
group: ci-${{ github.ref }}

# after
group: ${{ github.workflow }}-${{ github.ref }}

Correct. Groups are now workflow-scoped, so no future cross-workflow cancellation risk.

PR trigger (831f0a6)

# before
pull_request:
  branches: [main]

# after
pull_request:

Correct. CI fires on any PR regardless of base branch. Stacked-PR footgun removed.

Permissions (831f0a6)

permissions:
  contents: read

Correct placement (top-level, before concurrency). The workflow only checks out code and runs swiftcontents: read is the exact right scope.


Remaining items (both optional)

1. No job timeout — if swift build or swift test hangs (e.g., a spinning test, a stalled network fetch for a package), the job will consume runner time for up to 6 hours before GitHub kills it. A conservative ceiling:

jobs:
  test:
    timeout-minutes: 30
    ...

Not a blocker, but worth adding before this workflow is the gate for main.

2. No dependency cache — each run does a cold swift build, resolving and compiling all dependencies from scratch. Swift package resolution + compilation on macos-14/macos-15 is slow enough that caching the resolved packages would meaningfully cut run times. A common pattern:

- name: Cache Swift packages
  uses: actions/cache@v4
  with:
    path: .build
    key: ${{ runner.os }}-swift-${{ hashFiles('**/Package.resolved') }}
    restore-keys: ${{ runner.os }}-swift-

Place this step between checkout and Build. Again, not a blocker — swift build -v will still pass without it, just slower.


Summary

Item Status
Concurrency group scoped to workflow ✅ Fixed
PR trigger fires on all base branches ✅ Fixed
Least-privilege permissions ✅ Added
actions/checkout@v6 (CI confirmed it resolves) ✅ Acceptable
iOS xcodebuild job ✅ Deferred (right call)
Job timeout optional
Dependency cache optional

The workflow is ready to merge as-is. The two optional items above are improvements worth adding in a follow-up, not hold-the-PR issues.

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Keep it up!

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

@atom2ueki

Copy link
Copy Markdown
Owner Author

Round 2 follow-up:

Will squash-merge once this final CI run goes green.

@atom2ueki atom2ueki merged commit 5d844bc into main May 1, 2026
2 checks passed
@atom2ueki atom2ueki deleted the ci/tests-on-push branch May 1, 2026 16:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

github action test when push

2 participants