Repository navigation
Add Swift warning budget CI guard - #3220
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughAdds CI enforcement of a Swift warning budget: captures xcodebuild output to a temp log, runs a new Python CLI to parse/normalize/aggregate Swift compiler warnings against a checked-in TSV budget, and includes a shell test that validates the budget write/validate flows and CI wiring. Changes
Sequence DiagramsequenceDiagram
participant CI as CI Workflow
participant Build as xcodebuild
participant Log as /tmp/cmux-build-output.txt
participant Tool as swift_warning_budget.py
participant TSV as Budget TSV
CI->>Build: trigger xcodebuild (capture stdout+stderr)
Build-->>Log: write combined build output (via tee)
CI->>Tool: run python3 scripts/swift_warning_budget.py --log /tmp/cmux-build-output.txt [--write-budget?]
Tool->>Log: read log (strip ANSI/timestamps)
Tool->>Tool: extract & normalize Swift warnings (regex, normalize text)
Tool->>Tool: filter warnings to repo-owned paths (exclude vendors/packages)
Tool->>TSV: write budget TSV (--write-budget) or load existing TSV (validate)
Tool->>CI: exit 0 (ok) / 1 (budget exceeded) / 2 (missing/invalid budget)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9140f7e965
ℹ️ 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".
Greptile SummaryThis PR introduces a Swift warning budget system: a Python parser ( Confidence Score: 4/5Safe to merge; all findings are P2 style/robustness suggestions with no impact on the warning budget enforcement logic itself. Only P2 findings: unhandled ValueError in main() produces a traceback on malformed budget files, brittle string patterns in the test script, and a theoretical path-extraction edge case for unusual workspace layouts. Core logic is correct — dedup, filtering, comparison, and CI plumbing all work as intended. scripts/swift_warning_budget.py — error handling in main(); tests/test_ci_swift_warning_budget.sh — brittle ci.yml pattern matching. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[CI: Build app step\nxcodebuild ... build] -->|2>&1 pipe| B[tee /tmp/cmux-build-output.txt]
A -->|exit code via pipefail| C{Build succeeded?}
C -->|No| D[Step fails\nSubsequent steps skipped]
C -->|Yes| E[Validate Swift warning budget step\npython3 scripts/swift_warning_budget.py\n--log /tmp/cmux-build-output.txt]
E --> F[collect_warnings\nParse log line-by-line\nStrip ANSI, match WARNING_RE\nDeduplicate by file+line+col+msg\nFilter vendor/SourcePackages/ghostty]
F --> G[WarningBudget Counter\nkeyed by rel_path + message]
E --> H[load_budget\n.github/swift-warning-budget.tsv]
H --> I[Allowed Counter\n196 warnings baseline]
G --> J[compare_budget]
I --> J
J -->|actual > allowed for any key| K[Exit 1\nPrint budget exceeded\nList new warnings]
J -->|actual <= allowed for all keys| L[Exit 0\nOptionally suggest reductions]
Reviews (1): Last reviewed commit: "ci: add Swift warning budget" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/test_ci_swift_warning_budget.sh (1)
38-51: Make the TSV assertions fixed-string checks.These greps are matching literal budget rows, so
grep -Fqis safer than regex matching for theAppDelegate.swiftentry and the negative checks.♻️ Proposed fix
-if ! grep -q $'1\tSources/AppDelegate.swift\tadd' "$BUDGET"; then +if ! grep -Fq $'1\tSources/AppDelegate.swift\tadd' "$BUDGET"; then echo "expected AppDelegate preconcurrency warning budget entry" >&2 exit 1 fi -if grep -q 'vendor/bonsplit' "$BUDGET"; then +if grep -Fq 'vendor/bonsplit' "$BUDGET"; then echo "vendor warning should not be included" >&2 exit 1 fi -if grep -q 'SourcePackages' "$BUDGET"; then +if grep -Fq 'SourcePackages' "$BUDGET"; then echo "package warning should not be included" >&2 exit 1 fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_ci_swift_warning_budget.sh` around lines 38 - 51, The greps in tests/test_ci_swift_warning_budget.sh should use fixed-string matching to avoid regex interpretation; replace the three grep -q calls that check the literal TSV entries (the positive check for $'1\tSources/AppDelegate.swift\tadd' and the two negative checks for 'vendor/bonsplit' and 'SourcePackages') with grep -Fq so they perform fixed-string matches against the BUDGET file variable.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/test_ci_swift_warning_budget.sh`:
- Around line 38-51: The greps in tests/test_ci_swift_warning_budget.sh should
use fixed-string matching to avoid regex interpretation; replace the three grep
-q calls that check the literal TSV entries (the positive check for
$'1\tSources/AppDelegate.swift\tadd' and the two negative checks for
'vendor/bonsplit' and 'SourcePackages') with grep -Fq so they perform
fixed-string matches against the BUDGET file variable.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 93fbfa7c-92bb-4531-8d06-c86cae542f0c
⛔ Files ignored due to path filters (1)
.github/swift-warning-budget.tsvis excluded by!**/*.tsv
📒 Files selected for processing (3)
.github/workflows/ci.ymlscripts/swift_warning_budget.pytests/test_ci_swift_warning_budget.sh
9140f7e to
285f06c
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/test_ci_swift_warning_budget.sh (1)
33-58: Expand exclusion assertions to includeghosttyandhomebrew-cmux.The parser ignores four path families, but this test only validates two (
vendor,SourcePackages). Addingghosttyandhomebrew-cmuxfixtures will close a regression gap.Suggested patch
cat >"$LOG" <<'LOG' /Users/example/cmux/Sources/AppDelegate.swift:10:1: warning: add '@preconcurrency' to suppress 'Sendable'-related warnings from module 'ObjectiveC' /Users/example/cmux/Sources/AppDelegate.swift:10:1: warning: add '@preconcurrency' to suppress 'Sendable'-related warnings from module 'ObjectiveC' /Users/example/cmux/Sources/AppDelegate.swift:42:9: warning: result of call to 'closePanel(_:force:)' is unused 2026-04-28T09:40:13.8874600Z /Users/example/cmux/Sources/AppDelegate.swift:44:9: warning: capture of 'observer' with non-Sendable type '(any NSObjectProtocol)?' in a '@Sendable' closure; this is an error in the Swift 6 language mode 2026-04-28T09:40:13.8874610Z /Users/example/cmux/Sources/AppDelegate.swift:44:9: warning: capture of 'observer' with non-sendable type '(any NSObjectProtocol)?' in a '@Sendable' closure /Users/example/cmux/vendor/bonsplit/Sources/Bonsplit/Public/BonsplitView.swift:1:1: warning: ignored vendor warning +/Users/example/cmux/ghostty/Sources/Ghostty/GhosttyView.swift:1:1: warning: ignored ghostty warning +/Users/example/cmux/homebrew-cmux/Sources/Formula/cmux.rb:1:1: warning: ignored homebrew warning /tmp/cmux/SourcePackages/checkouts/posthog-ios/PostHog/PostHogSDK.swift:1:1: warning: ignored package warning warning: Run script build phase 'Run Script' will be run during every build LOG @@ if grep -q 'vendor/bonsplit' "$BUDGET"; then echo "vendor warning should not be included" >&2 exit 1 fi +if grep -q 'ghostty/' "$BUDGET"; then + echo "ghostty warning should not be included" >&2 + exit 1 +fi + +if grep -q 'homebrew-cmux/' "$BUDGET"; then + echo "homebrew-cmux warning should not be included" >&2 + exit 1 +fi + if grep -q 'SourcePackages' "$BUDGET"; then echo "package warning should not be included" >&2 exit 1 fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_ci_swift_warning_budget.sh` around lines 33 - 58, Test currently only asserts exclusion of 'vendor' and 'SourcePackages' entries; add similar negative assertions for the two other ignored path families by updating tests/test_ci_swift_warning_budget.sh to also grep the budget file for 'ghostty' and 'homebrew-cmux' and fail if either is present (mirror the existing grep -q checks that echo "vendor warning should not be included" and "SourcePackages" check). Ensure the failure messages reference the new fixtures (e.g., "ghostty warning should not be included" and "homebrew-cmux warning should not be included") and keep the checks after the other budget validations that run python3 scripts/swift_warning_budget.py.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/test_ci_swift_warning_budget.sh`:
- Around line 33-58: Test currently only asserts exclusion of 'vendor' and
'SourcePackages' entries; add similar negative assertions for the two other
ignored path families by updating tests/test_ci_swift_warning_budget.sh to also
grep the budget file for 'ghostty' and 'homebrew-cmux' and fail if either is
present (mirror the existing grep -q checks that echo "vendor warning should not
be included" and "SourcePackages" check). Ensure the failure messages reference
the new fixtures (e.g., "ghostty warning should not be included" and
"homebrew-cmux warning should not be included") and keep the checks after the
other budget validations that run python3 scripts/swift_warning_budget.py.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f45415c1-8b0b-4b77-afd4-c86d9f3c6caa
⛔ Files ignored due to path filters (1)
.github/swift-warning-budget.tsvis excluded by!**/*.tsv
📒 Files selected for processing (3)
.github/workflows/ci.ymlscripts/swift_warning_budget.pytests/test_ci_swift_warning_budget.sh
🚧 Files skipped from review as they are similar to previous changes (2)
- scripts/swift_warning_budget.py
- .github/workflows/ci.yml
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 285f06cad3
ℹ️ 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".
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/test_ci_swift_warning_budget.sh (1)
19-26: Tighten CI wiring assertions to reduce false passes.The token check at Line 22 (
"tee") is very broad; unrelated text can satisfy it. Prefer a stricter snippet/pattern that asserts teeing to the expected build log path in one check.Suggested hardening
required_tokens = { "workflow guard step": "Validate Swift warning budget guard", "guard test script": "./tests/test_ci_swift_warning_budget.sh", - "build log tee": "tee", - "build log path": "cmux-build-output.txt", + "build log tee path": "tee /tmp/cmux-build-output.txt", "budget script": "scripts/swift_warning_budget.py", "budget log argument": "--log", }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_ci_swift_warning_budget.sh` around lines 19 - 26, The current required_tokens entry "build log tee": "tee" is too permissive; update the value to assert tee is writing to the expected file by matching the exact invocation (combine the "tee" token with the "build log path" value), e.g., require a snippet like "tee cmux-build-output.txt" or a pipe form " | tee cmux-build-output.txt"; modify the required_tokens map entry "build log tee" accordingly so the CI asserts teeing to the expected build log path.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/test_ci_swift_warning_budget.sh`:
- Around line 76-79: Replace the regex grep call in the budget-check conditional
that currently uses grep -q '.ci-source-packages' "$BUDGET" with a fixed-string
search so the literal token ".ci-source-packages" is matched; update the command
to use grep -F (e.g., change the grep invocation in the if condition) to prevent
accidental matches like "xci-source-packages".
---
Nitpick comments:
In `@tests/test_ci_swift_warning_budget.sh`:
- Around line 19-26: The current required_tokens entry "build log tee": "tee" is
too permissive; update the value to assert tee is writing to the expected file
by matching the exact invocation (combine the "tee" token with the "build log
path" value), e.g., require a snippet like "tee cmux-build-output.txt" or a pipe
form " | tee cmux-build-output.txt"; modify the required_tokens map entry "build
log tee" accordingly so the CI asserts teeing to the expected build log path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e7143b36-1550-4dd1-9a27-991d5611b3da
📒 Files selected for processing (2)
scripts/swift_warning_budget.pytests/test_ci_swift_warning_budget.sh
🚧 Files skipped from review as they are similar to previous changes (1)
- scripts/swift_warning_budget.py
This branch was successfully deployed
1 participant
Summary
Test plan
Summary by CodeRabbit
New Features
Tests
Chores
Summary by cubic
Add a Swift warning budget to CI to block new cmux-owned Xcode warnings. CI fails if the Debug app build adds warnings above the checked-in 196-warning baseline.
New Features
scripts/swift_warning_budget.pyto parsexcodebuildoutput, strip ANSI/GitHub timestamp prefixes, normalize messages (remove Swift 6 suffix, unify “non-sendable”, tweak'@preconcurrency'text), dedupe by file/line/column/message, and compare against.github/swift-warning-budget.tsv; ignoresvendor,ghostty,homebrew-cmux,SourcePackages, and.ci-source-packages. Resolves absolute paths to repo-relative and preservesPackages/...ownership; prints budget summaries and reduction hints; clean errors on malformed budgets (no traceback)..github/swift-warning-budget.tsv(196 warnings)./tmp/cmux-build-output.txtand validate; added a guard step (tests/test_ci_swift_warning_budget.sh) that verifies CI wiring and tests normalization, path handling, ignore rules, and failure behavior.Migration
.github/swift-warning-budget.tsv. To refresh intentionally: runpython3 scripts/swift_warning_budget.py --log /tmp/cmux-build-output.txt --write-budget.Written for commit e553f6e. Summary will update on new commits. Review in cubic