Repository navigation
ci: lint that every cmuxTests Swift file is wired into pbxproj (#4559) - #4562
Merged
Merged
Conversation
Catches the class of bug surfaced during the #4529 investigation: a test file added to the worktree without a matching entry in cmux.xcodeproj/project.pbxproj is silently ignored by Xcode and never compiles or runs on CI. Both bot reviews and `xcodebuild test -only-testing:cmuxTests/<TestClass>` pass with "Executed 0 tests" — so the missing wiring is indistinguishable from a clean two-commit red/green regression test until a real user hits the bug the test was supposed to catch. This is the RED commit of a two-commit pattern: introducing the lint immediately flags two pre-existing orphans on main (SessionIndexViewTests.swift and SidebarMarkdownRendererTests.swift) and the new `workflow-guard-tests` step turns red. The follow-up commit wires those two files into the cmuxTests target so the lint goes green. Refs #4559 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ument pitfalls GREEN commit of the two-commit pattern started in the previous commit. `scripts/lint-pbxproj-test-wiring.sh` (added previously) flagged two pre-existing test files on `main` that have never compiled or run on CI because they were never added to the cmuxTests target: - cmuxTests/SessionIndexViewTests.swift - cmuxTests/SidebarMarkdownRendererTests.swift Both contain real-looking XCTest coverage (Claude local-command-caveat title formatting, markdown inline-attribute preservation). Wiring them into `cmux.xcodeproj/project.pbxproj` so they actually run, which also closes #4559 and lets the new `workflow-guard-tests` lint step go green on this PR. Also adds two CLAUDE.md "Pitfalls" entries based on what fell out of the #4529 investigation: - Foundation/SwiftUI/AttributeGraph/WebKit semantics change silently between macOS versions (concrete `URL.deletingLastPathComponent` example from #4529). Recommends AWS M4 Pro builders for empirical repro and points to the `regression-hunt` skill. - Test files in cmuxTests/ must be wired into project.pbxproj or they're silently skipped. References the new lint script and the PR #4536 incident that surfaced the class of bug. Self-test of `tests/test_ci_pbxproj_test_wiring.sh` also got a small hardening: the synthetic sandbox pbxproj no longer mentions the orphan test file by name in a comment, which used to produce a spurious `hits=1` and mask the failure detection inside the wrapper. Closes #4559 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 4489758. Configure here.
Contributor
Greptile SummaryAdds a CI lint that catches Swift test files in
Confidence Score: 5/5Safe to merge — adds a CI guard that prevents silently-skipped test files, wires two real orphaned test classes, and includes a comprehensive self-test covering five distinct failure modes. The lint logic is well-scoped: it resolves the exact cmuxTests Sources build phase UUID before checking files, uses fixed-string matching to avoid substring false-positives, and the self-test exercises every failure path the lint is meant to catch. The pbxproj wiring follows the established four-entry template. The fatalError addition in the test helper is safe test-only scaffolding. No production Swift paths are touched. No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[lint-pbxproj-test-wiring.sh] --> B[Locate cmuxTests PBXNativeTarget block via awk]
B --> C{Found?}
C -- No --> EXIT2[exit 2]
C -- Yes --> D[Extract Sources build phase UUID]
D --> E{UUID found?}
E -- No --> EXIT2
E -- Yes --> F[Slice PBXSourcesBuildPhase block matching UUID]
F --> G{Block found?}
G -- No --> EXIT2
G -- Yes --> H[For each cmuxTests/*.swift grep -F in Sources block]
H --> I{All present?}
I -- Yes --> OK[exit 0 ok]
I -- No --> FAIL[exit 1 list missing files]
Reviews (4): Last reviewed commit: "ci: anchor pbxproj membership match agai..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| # file reference UUID, group children list, and target sources phase. | ||
| # Require at least 2 references so we flag both "added but missing | ||
| # entirely" and "stub reference but no sources phase" failures. | ||
| hits="$(grep -c -- "$base" "$PBXPROJ" || true)" |
Contributor
There was a problem hiding this comment.
. in .swift is a BRE metacharacter, so grep -c -- "$base" could match lines where the dot is replaced by any character (e.g., a hypothetical FooTestsXswift entry). While no such patterns exist in a well-formed pbxproj today, using fixed-string matching with -F is the correct approach and makes the intent unambiguous.
Suggested change
| hits="$(grep -c -- "$base" "$PBXPROJ" || true)" | |
| hits="$(grep -cF -- "$base" "$PBXPROJ" || true)" |
Comment on lines
+40
to
+53
| if "$LINT" --repo-root "$SANDBOX" >"$SANDBOX/out" 2>&1; then | ||
| echo "test_ci_pbxproj_test_wiring: lint should have failed on the unwired sandbox test" >&2 | ||
| cat "$SANDBOX/out" >&2 | ||
| exit 1 | ||
| fi | ||
| if ! grep -q "FakeOrphanTests.swift" "$SANDBOX/out"; then | ||
| echo "test_ci_pbxproj_test_wiring: lint output missing FakeOrphanTests.swift" >&2 | ||
| cat "$SANDBOX/out" >&2 | ||
| exit 1 | ||
| fi | ||
| if ! grep -q "hits=0" "$SANDBOX/out"; then | ||
| echo "test_ci_pbxproj_test_wiring: lint output missing hits=0" >&2 | ||
| cat "$SANDBOX/out" >&2 | ||
| exit 1 |
Contributor
There was a problem hiding this comment.
The self-test verifies the lint catches a completely unwired file (hits=0) but does not exercise the "stub reference but no sources phase" path (the case where hits=1). A future regression that accidentally raised the threshold from < 2 to < 1 would break this partially-wired guard silently. Adding a second sandbox case that injects exactly one reference line into the fake pbxproj would complete coverage of both branches the lint comment describes.
📝 WalkthroughWalkthroughAdds a lint ensuring top-level Changespbxproj Test Wiring Lint and CI Enforcement
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 16 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (16 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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 5 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/lint-pbxproj-test-wiring.sh`:
- Line 72: The grep call that sets hits with hits="$(grep -c -- "$base"
"$PBXPROJ" || true)" can produce false positives because it matches the basename
as a substring (e.g., "A.swift" matching "TestHelperA.swift"); change the
pattern to match the filename more precisely by using a fixed-string search and
tighter context around the basename (for example use grep -F and include a
preceding delimiter or whitespace) when computing hits, referencing the
variables base and PBXPROJ and the assignment to hits so the script counts only
exact filename occurrences in the pbxproj.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: a731309d-7cea-4750-a43f-397983cd27bf
📒 Files selected for processing (5)
.github/workflows/ci.ymlCLAUDE.mdcmux.xcodeproj/project.pbxprojscripts/lint-pbxproj-test-wiring.shtests/test_ci_pbxproj_test_wiring.sh
cmuxTests/SessionIndexViewTests.swift was not in the test target until the previous commit wired it into project.pbxproj. While it was orphan, the SessionAgent enum gained a `.registered(RegisteredSessionAgent)` case (Sources/SessionIndexModels.swift:44) that the test's `defaultSpecificsForTesting` switch never accounted for. With the test file now actually compiling on CI, the switch fails: SessionIndexViewTests.swift:391:9: error: switch must be exhaustive note: add missing case: '.registered(_)' The test's call sites only ever pass built-in agents (`.claude`, `.grok`). Adding a `fatalError` on `.registered` keeps the switch exhaustive without inventing fake `CmuxVaultAgentRegistration` data that future readers would have to reconcile; if anyone extends the suite to cover Vault-registered agents, the fatalError points them at the missing helper. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…reference Earlier version of the lint counted any pbxproj line that mentioned the test file basename. That's too permissive: a file can have a PBXFileReference and a group children entry but still not be a member of the cmuxTests target's PBXSourcesBuildPhase, in which case Xcode silently skips it — the exact failure mode that lets a regression test land green without ever running. Switch to counting only lines that end with "<basename>.swift in Sources */", which appear in: 1. the PBXBuildFile entry, and 2. the cmuxTests target's PBXSourcesBuildPhase files list. A target member has hits >= 2 in both. A "group-only" file (referenced in the project tree but not part of the test target) has hits = 0, which is exactly the silent-skip case the lint must catch. Also adds a new (c) sandbox case in `tests/test_ci_pbxproj_test_wiring.sh` that drops a file with a PBXFileReference + group child but no PBXBuildFile / SourcesBuildPhase entry, and asserts the lint flags it. Without this case the wrapper would still pass against the looser bare-filename check. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/lint-pbxproj-test-wiring.sh (1)
30-33:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winGuard
--repo-rootvalue before reading$2.
--repo-rootwithout a value can fail with strict-mode positional access instead of returning a clean invocation error.Minimal fix
case "$1" in --repo-root) + if [ "$#" -lt 2 ] || [ -z "${2:-}" ]; then + echo "Missing value for --repo-root" >&2 + exit 2 + fi REPO_ROOT="$2" shift 2 ;;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/lint-pbxproj-test-wiring.sh` around lines 30 - 33, The --repo-root case currently reads "$2" unguarded which can crash in strict-mode; update the --repo-root handling so you first verify a non-empty, non-option next argument exists before assigning REPO_ROOT. Inside the case for --repo-root, check if [ -z "$2" ] || [[ "$2" == -* ]] and if so print a clear usage/error and exit; otherwise set REPO_ROOT="$2" and shift 2. Ensure you reference the --repo-root branch and the REPO_ROOT variable in your patch.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/lint-pbxproj-test-wiring.sh`:
- Around line 68-80: The current count of "$base in Sources" can be satisfied by
other targets; update the check so it verifies the entries belong specifically
to the cmuxTests target: locate the cmuxTests PBXSourcesBuildPhase block in
"$PBXPROJ" and ensure there's a PBXSourcesBuildPhase entry referencing
"<basename> in Sources" inside that block (and also confirm a PBXBuildFile line
for "<basename> in Sources"), then only consider the file present when both are
found; modify the logic around hits (and any temporary greps) to scope the
search to the cmuxTests Sources build phase instead of a global grep for "$base
in Sources".
---
Outside diff comments:
In `@scripts/lint-pbxproj-test-wiring.sh`:
- Around line 30-33: The --repo-root case currently reads "$2" unguarded which
can crash in strict-mode; update the --repo-root handling so you first verify a
non-empty, non-option next argument exists before assigning REPO_ROOT. Inside
the case for --repo-root, check if [ -z "$2" ] || [[ "$2" == -* ]] and if so
print a clear usage/error and exit; otherwise set REPO_ROOT="$2" and shift 2.
Ensure you reference the --repo-root branch and the REPO_ROOT variable in your
patch.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 54a14859-324c-4114-83f9-c2f310b74124
📒 Files selected for processing (3)
cmuxTests/SessionIndexViewTests.swiftscripts/lint-pbxproj-test-wiring.shtests/test_ci_pbxproj_test_wiring.sh
There was a problem hiding this comment.
1 issue found across 3 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
Earlier iterations counted matches in the whole pbxproj. That accepted two
silent-skip cases:
1. A file with PBXFileReference + group child but no PBXBuildFile /
SourcesBuildPhase entry (filename appears 2x globally, but the file is
not a member of any target — Xcode does not compile it).
2. A file wired into the wrong target (e.g. cmuxUITests instead of
cmuxTests). `<file>.swift in Sources` appears 2x in the pbxproj — once
in PBXBuildFile, once inside cmuxUITests' Sources phase — but Xcode
still does not compile it into the cmuxTests bundle, so the regression
test never runs.
Resolve the cmuxTests PBXNativeTarget and its Sources build phase UUID,
slice that phase block, and look for the file's `in Sources` entry only
inside that block. Threshold becomes 1 hit (membership) rather than a
global count, which is exactly what determines whether Xcode compiles the
file into cmuxTests.
Tighten the test wrapper to exercise all three failure modes against
synthetic pbxprojs that include a real cmuxTests PBXNativeTarget + Sources
phase stub. Verified by inverse: with this change reverted on a scratch
copy of the real pbxproj, the lint misses the wrong-target case; with it
applied, the lint flags it.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
…tives The previous check looked for `<base> in Sources` inside the cmuxTests Sources phase. That's vulnerable to filename-suffix overlap: if the lint target is a substring of another wired file, the longer match still satisfies the grep. The repo already contains an overlapping pair — `SearchIndexTests.swift` is a suffix of `SettingsSearchIndexTests.swift` — so accidentally removing `SearchIndexTests.swift` from the cmuxTests Sources phase would pass the lint. Switch to a fixed-string match against the full PBX comment `/* <base> in Sources */`. The leading `/* ` and trailing ` */` disambiguate the basename. Verified by inverse: scratch-removing `SearchIndexTests.swift` from the real cmuxTests Sources phase now flags the file specifically, while `SettingsSearchIndexTests.swift` is unaffected. Add a fifth sandbox case (e) in the wrapper that wires `PrefixFooTests.swift` but leaves `FooTests.swift` orphan, and asserts the lint flags only the orphan. Without this fixture a later loosening of the grep would not be caught. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
This was referenced May 24, 2026
This was referenced Jun 14, 2026
Merged
Merged
This was referenced Jun 22, 2026
Merged
This was referenced Jun 29, 2026
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Catches the silent-skip-test class of bug surfaced during the #4529 investigation.
What
scripts/lint-pbxproj-test-wiring.shchecks everycmuxTests/*.swiftfile has at least 2 references incmux.xcodeproj/project.pbxproj(a properly wired test has 4 — PBXBuildFile + PBXFileReference + group children + target SourcesBuildPhase). A test file added to the worktree without that wiring is silently ignored by Xcode and never compiles or runs on CI. Bothxcodebuild test -only-testing:cmuxTests/<TestClass>and bot reviews pass with "Executed 0 tests" — so missing wiring is indistinguishable from a clean two-commit red/green regression test until a real user hits the bug.A new
Validate pbxproj test-wiring lintstep inworkflow-guard-testsruns the lint on every PR;tests/test_ci_pbxproj_test_wiring.shself-tests the lint script itself so the guard can't rot into a no-op.Concrete bugs the lint just caught
Running on
main:Both exist on disk under
cmuxTests/, both contain real-looking XCTest coverage (Claude local-command-caveat title formatting, markdown inline-attribute preservation). Neither has ever compiled on CI. Filed as #4559. This PR wires them into the cmuxTests target so they actually run, which lets the new lint step go green on the second commit.Same failure mode also caught on #4536 (
cmuxTests/SessionIndexJSONLStreamTests.swiftadded in the "red" commit but never registered in pbxproj — the new test runs zero assertions on both red and green commits). Posted as a non-blocking finding to that PR; this lint is what catches it at PR time going forward.CLAUDE.md pitfall additions
Two new "Pitfalls" entries based on what fell out of the #4529 investigation:
URL.deletingLastPathComponentexample. Recommends AWS M4 Pro builders for empirical repro and points to theregression-huntskill in the cmuxterm-hq sibling repo.cmuxTests/must be wired into project.pbxproj or they're silently skipped. References the new lint and the PR Fix unbounded Vault JSONL history scans #4536 incident.Two-commit red/green
Both commits compile and the test wrapper script self-tests cleanly. The red/green contract is observable in CI's
workflow-guard-testsjob:7c82f5af2) — adds the lint + the CI step + the wrapper self-test, but does not wire the two pre-existing orphan test files. The newValidate pbxproj test-wiring lintstep inworkflow-guard-testsfails because the lint reportsSessionIndexViewTests.swift (hits=0)andSidebarMarkdownRendererTests.swift (hits=0).44897584a) — wires both files into the cmuxTests target via the standard four pbxproj entries (template copied fromTabManagerUnitTests.swift), and adds the two CLAUDE.md pitfall entries. Lint passes (ok (checked 127 test files)); the CI step turns green.Test plan
./scripts/lint-pbxproj-test-wiring.shreportsokafter the green commit./tests/test_ci_pbxproj_test_wiring.shexits 0 (both real-repo + synthetic-orphan paths)xcodebuild -project cmux.xcodeproj -scheme cmux -configuration Debug -destination 'platform=macOS' -derivedDataPath /tmp/cmux-issue-4529-ci-pbxproj-lint buildsucceeds with the two newly-wired test files includedworkflow-guard-testsCI job turns red on the first commit, green on the second commit (visible in GitHub PR Commits tab once it runs)SessionIndexViewTests,SidebarMarkdownRendererTests) actually execute on thetestsCI job and passCloses #4559
🤖 Generated with Claude Code
Need help on this PR? Tag
@codesmithwith what you need. Autofix is disabled.Note
Low Risk
Low risk: changes are limited to CI/lint bash scripts and add regression coverage to prevent false-positive passes when test filenames overlap by suffix.
Overview
Tightens the
lint-pbxproj-test-wiring.shcheck to match the exact/* <file> in Sources */PBX comment usinggrep -F, preventing false positives when one test filename is a substring/suffix of another.Extends
test_ci_pbxproj_test_wiring.shwith a new fixture that reproduces the suffix-overlap scenario and asserts the lint flags only the truly-unwired file.Reviewed by Cursor Bugbot for commit d79f42d. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Adds a CI lint that fails if any
cmuxTests/*.swiftfile isn’t a member of thecmuxTeststarget’s Sources incmux.xcodeproj/project.pbxproj, now hardened to avoid wrong-target wiring and suffix-overlap false negatives. Wires two orphaned tests, fixes a test helper, and documents the pitfall.New Features
scripts/lint-pbxproj-test-wiring.shthat resolves thecmuxTestsPBXNativeTarget, slices itsPBXSourcesBuildPhase, and requires each test file to appear there; anchors matches to/* <base> in Sources */to prevent suffix-overlap false negatives.Validate pbxproj test-wiring linttoworkflow-guard-testsand expandedtests/test_ci_pbxproj_test_wiring.shto cover no references, group-only files, wrong-target membership, and a suffix-overlap case.Bug Fixes
SessionIndexViewTests.swiftandSidebarMarkdownRendererTests.swiftinto thecmuxTeststarget.SessionIndexViewTestscompile byfatalErroring on.registeredindefaultSpecificsForTesting.CLAUDE.md(macOS API drift; required pbxproj wiring).Written for commit d79f42d. Summary will update on new commits. Review in cubic
Summary by CodeRabbit
Tests
Documentation
Chores / CI