Skip to content

ci: improve CI workflow#46

Merged
arkavo-com merged 5 commits into
mainfrom
ci/improvements
Jun 22, 2026
Merged

ci: improve CI workflow#46
arkavo-com merged 5 commits into
mainfrom
ci/improvements

Conversation

@arkavo-com

Copy link
Copy Markdown
Contributor

Summary

Improves the GitHub Actions CI workflow with explicit permissions, concurrency controls, faster linting, additional build coverage, code-coverage reporting, and automated benchmark regression detection.

Changes

  • Security / CodeQL alerts
    • Added explicit top-level permissions to resolve CodeQL "Workflow does not contain permissions" alerts.
  • Concurrency
    • Added concurrency config so stale runs for the same branch/PR are cancelled automatically.
  • Lint job
    • Removed brew install swiftformat (SwiftFormat is preinstalled on macOS runners).
    • Added caching for .swiftformat.cache.
    • Removed the commented-out SwiftLint step.
  • Build job
    • Added a release build of OpenTDFKitCLI so the CLI is verified in CI.
  • Test job
    • Tests now run with --enable-code-coverage.
    • Generates an LCOV report via llvm-cov export.
    • Uploads coverage to Codecov (token optional; upload does not fail the build if Codecov is unavailable).
  • Benchmark job
    • Added a lightweight Python parser (scripts/parse_benchmarks.py) that extracts metrics from benchmark logs.
    • Restores a cached baseline, compares the current run, and fails on a configurable regression threshold (30%).
    • Saves the current summary as the new baseline.
    • Uploads both logs and the baseline JSON as artifacts.

Verification

  • swift test passes locally.
  • swiftformat --swiftversion 6.2 . is clean.
  • The benchmark parser was tested locally against existing benchmark logs.

Notes

  • The Node.js 20 deprecation warning for actions/checkout@v4 (and other v4 actions) is a GitHub-side transition; newer action major versions are not yet available. The workflow will continue to run because GitHub is forcing those actions onto Node.js 24 runners.
  • Codecov uploads require a CODECOV_TOKEN secret for the repository; if it is not configured, the upload step is allowed to fail gracefully.

Comment thread .github/workflows/swift.yaml Outdated
Comment thread .github/workflows/swift.yaml Outdated
Comment thread .github/workflows/swift.yaml
@gitar-bot

gitar-bot Bot commented Jun 22, 2026

Copy link
Copy Markdown
Code Review ✅ Approved 3 resolved / 3 findings

Enhances the CI workflow with explicit permissions, concurrency controls, code coverage, and benchmark regression detection. Resolved previous issues regarding failure masking, incorrect baseline keying, and faulty benchmark comparison ordering.

✅ 3 resolved
Bug: Regression check overwrites baseline before comparing, never detects regressions

📄 .github/workflows/swift.yaml:174 📄 scripts/parse_benchmarks.py:153-167
The "Check benchmark regression" step passes the same path for both --baseline and --output:

python3 scripts/parse_benchmarks.py --threshold 0.30 --baseline benchmark-baseline.json --output benchmark-baseline.json *.log

In main(), the script writes the current summary to args.output (lines 159-160) BEFORE it reads args.baseline (lines 164-165). Since both point to benchmark-baseline.json, the restored baseline is clobbered with the current run's metrics, and the subsequent compare_to_baseline() compares the current run against itself. The result is that compare_to_baseline returns no regressions for matching keys — regression detection is effectively a no-op and will never fail CI.

Fix: either write the summary to a distinct file and only promote it to the baseline after the comparison, or read the baseline into memory before writing the output. The simplest robust fix is to read the baseline first in the script (so output/baseline aliasing is harmless), or use separate filenames in the workflow.

Bug: tee masks benchmark/test failures (pipefail not set)

📄 .github/workflows/swift.yaml:156 📄 .github/workflows/swift.yaml:159 📄 .github/workflows/swift.yaml:162
The benchmark steps pipe swift test into tee: swift test ... | tee keystore-benchmark.log. The exit status of a pipeline is the status of the last command (tee), which essentially always succeeds. If swift test crashes or a benchmark test fails, the step still reports success, so broken benchmarks won't surface in CI and the parser silently produces an empty/partial summary. Add set -o pipefail (e.g. via a multi-line run: with set -euo pipefail) so the step fails when swift test fails.

Edge Case: Benchmark baseline is keyed per-ref, so PRs never compare against main

📄 .github/workflows/swift.yaml:168 📄 .github/workflows/swift.yaml:181
The baseline cache key includes ${{ github.ref }} and is saved with ${{ github.run_id }}. For a pull request, github.ref is the PR merge ref (e.g. refs/pull/46/merge), so the first run of any PR finds no matching baseline and silently records its own numbers as the baseline. Subsequent regressions are then measured only against the PR's own prior run, never against main. If the intent is to catch regressions introduced by a PR, consider keying the restore on the base branch baseline (e.g. fall back to benchmark-baseline-${{ runner.os }}-refs/heads/main-) so PRs are compared against the mainline numbers.

Was this helpful? React with 👍 / 👎 | Gitar

@arkavo-com arkavo-com merged commit 30d5919 into main Jun 22, 2026
6 checks passed
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.

1 participant