Repository navigation
Codify modular-refactor rules into CLAUDE.md - #5111
Conversation
Captures the patterns we enforced while extracting CmuxFoundation, CmuxSocketControl, and CmuxProcess so future package/refactor work follows them without rediscovery: - Testability: test through an injected protocol seam, never a `nonisolated(unsafe) static var fooForTesting` global hook; deleting such a hook (and the lock it needed) is part of extracting the type. - Concurrency: when extracting code that uses a forbidden primitive, redesign it at the seam rather than carrying it across — e.g. an `NSLock` single-resume race becomes a tiny `actor` guard around `withCheckedContinuation`, with `Process` pipes drained on detached tasks keyed by raw fd. - Concurrency exceptions: one-shot `DispatchSource.makeTimerSource` is acceptable (with justification) for a genuine deadline/timeout, since async-native timers are disallowed here; never to poll or fake a sleep. - Testing: `reload.sh` builds only the `cmux` scheme, not the test target, so a green reload does not prove `cmuxTests` compiles; build `cmux-unit` or rely on the `tests` CI job before pushing package/refactor changes. - Package architecture: how to wire a new local package into project.pbxproj, linking into both the `cmux` and `cmux-unit` targets. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughUpdated ChangesDeveloper Guidance Clarifications
Estimated Code Review Effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error, 1 warning)
✅ Passed checks (16 passed)
✨ Finishing Touches🧪 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 |
Greptile SummaryThis docs-only PR codifies the architectural patterns and enforcement rules discovered during the wave 1–2 package extractions (
Confidence Score: 5/5Docs-only change with no production code modified; safe to merge. The entire diff is prose additions to CLAUDE.md. The new guidance is internally consistent, refines rather than contradicts existing rules, and the one known gap (GlobalISel xcodebuild flag) was already flagged in a prior review thread. No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["5. Executable\n(cmuxApp / AppDelegate)\nComposition root — no business logic"] --> B
B["4. UI\nSwiftUI/AppKit views\n(e.g. CmuxSettingsUI)"] --> C
C["3. Domain / State\n@MainActor @Observable Coordinators\n(e.g. CmuxSettings)"] --> D
D["2. Services / Infrastructure\nactor-based capabilities\n(e.g. CmuxSocketControl, CmuxProcess)"] --> E
E["1. Core\nSendable values, IDs, DTOs,\nprotocol seams (e.g. CmuxCore)"]
Reviews (3): Last reviewed commit: "Correct the lock guidance: don't make an..." | Re-trigger Greptile |
|
|
||
| - **E2E / UI tests:** trigger via `gh workflow run test-e2e.yml` (see cmuxterm-hq CLAUDE.md for details) | ||
| - **Unit tests:** `xcodebuild -scheme cmux-unit` is safe (no app launch), but prefer CI | ||
| - **`reload.sh` does not compile the test target.** It builds only the `cmux` scheme, so a green `reload.sh` says nothing about whether `cmuxTests`/`cmuxUITests` still compile. A symbol that is moved or renamed can keep the `cmux` app building while breaking the test target (real case: a `write(to:atomically:)` typo and a removed `TabManager.CommandResult` only surfaced in the `tests` job). Before pushing package/refactor changes, build the `cmux-unit` scheme (with `-derivedDataPath /tmp/cmux-<tag>` and, for `cmuxApp`/`AppDelegate` churn, the GlobalISel workaround flag) or let the `tests` CI job gate it — never treat `reload.sh` alone as proof the tests build. |
There was a problem hiding this comment.
GlobalISel workaround flag left undefined for xcodebuild
The bullet references "the GlobalISel workaround flag" when building the cmux-unit scheme directly, but that flag is only surfaced as --swift-frontend-workaround / --swift-disable-global-isel on reload.sh (which builds cmux, not cmux-unit). A developer or agent following this guidance literally won't know the equivalent xcodebuild argument: OTHER_SWIFT_FLAGS='$(inherited) -Xllvm -aarch64-enable-global-isel-at-O=-1'. Spelling it out — or cross-referencing reload.sh --swift-frontend-workaround and noting the xcodebuild translation — would make the bullet self-contained.
…itory, dependency inversion) to CLAUDE.md These higher-level patterns lived only in the cmuxterm-hq blueprint (blueprint/CONVENTIONS.md), so anyone reading the cmux repo CLAUDE.md alone had the granular rules (no locks, inject, DocC) but not the architecture they serve. Distills the enforceable core: - Five-layer downward-only package DAG (Core / Services / Domain / UI / Executable). - Classify extracted entities by intent: Coordinator (@mainactor @observable orchestrator), Service (actor capability), Repository (actor persistence). - Dependency inversion: lower packages publish protocols, higher depend on `any Protocol`; constructor injection only; the executable app target is the single composition root. - State + SwiftUI wiring: @observable sub-models; @State/@Bindable/@Environment, never @StateObject/@ObservedObject/@EnvironmentObject. - Executable-target boundary: @main + AppDelegate stay; extensions invert into Coordinators/Services and forward; stored state decomposes into sub-models. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Per maintainer feedback: a single-method `actor` whose only job is to guard a
flag is an antipattern, not a fix. It forces synchronous callers (Process
termination handlers, DispatchSource handlers, a withCheckedContinuation resume
race) through `Task { await guard.claim() }`, adding suspension points and
reentrancy to a synchronous compare-and-set.
- Add a narrow lock carve-out: a lock is allowed for a synchronous
compare-and-set called from non-async callbacks (canonical case: a one-shot
resume guard via `OSAllocatedUnfairLock`), where promoting to an actor would
only add Task/await hops. For tiny flags/counters, not ongoing domain state.
- Replace the earlier "extract NSLock into an actor guard" recommendation, which
recommended exactly this antipattern.
- Align the forbidden-locks bullet and the review checklist with the carve-out,
and have reviewers reject a single-method mutex-actor.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Docs-only. Codifies the rules and architecture patterns we enforced across the wave 1-2 package extractions (
CmuxFoundation,CmuxSocketControl,CmuxProcess) into the binding cmuxCLAUDE.md, so future refactor work follows them without rediscovery or having to read the cmuxterm-hq blueprint.Granular rules (placed in their existing sections):
nonisolated(unsafe) static var fooForTestingglobal hook; deleting such a hook is part of extracting the type.NSLocksingle-resume race (process termination vs. timeout vs. spawn failure) becomes a tinyactorguard aroundwithCheckedContinuation; drainProcesspipes on detached tasks keyed by raw fd.DispatchSource.makeTimerSourceis acceptable (with justification) for a genuine deadline/timeout; for true deadlines only.reload.shbuilds only thecmuxscheme, so a green reload does not provecmuxTestscompiles; buildcmux-unitor rely on thetestsjob. (Real misses: awrite(to:atomically:)typo and a removedTabManager.CommandResultthat only surfaced in thetestsjob.)project.pbxproj, linking into both thecmuxandcmux-unittargets.Architecture patterns (new "Refactor architecture" section) — these previously lived only in the cmuxterm-hq blueprint (
docs/cmux-refactor-audit/blueprint/CONVENTIONS.md), so a reader of cmux'sCLAUDE.mdalone had the granular rules but not the architecture they serve:@MainActor @Observableorchestrator), Service (actor capability), Repository (actor persistence).any Protocol; constructor injection only; the executable app target is the single composition root.@Observablesub-models;@State/@Bindable/@Environment, never@StateObject/@ObservedObject/@EnvironmentObject).@main+AppDelegatestay; extensions invert into Coordinators/Services and forward; stored state decomposes into sub-models).🤖 Generated with Claude Code
Summary by CodeRabbit
Note: This release contains no user-facing changes.