Repository navigation
review-bot-rules: no test/debug seam in production Swift Sources - #6455
Merged
Merged
Conversation
Add a focused review rule flagging test-only/debug-only seams added inline to production Swift under Sources/ (not Tests/): #if DEBUG accessors, debug…/…ForTesting/…ForTests/testOnly… members, or visibility widened plus a wrapper accessor so a test can call it. Preferred fix is to observe internal state from the test target via @testable import after widening private to internal, or to isolate a genuinely debug-only facility in a dedicated debug file/folder. Mirrors the rule into CodeRabbit (path_instructions + blocking custom check "cmux no test or debug seam in production source") and Greptile (rule id cmux-no-test-debug-seam-in-production-source + files.json + rules.md). Cites #6452 as the reference fix. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Ignoring CodeRabbit configuration file changes. For security, only the configuration from the base branch is applied for open source repositories. 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 (3)
📝 WalkthroughWalkthroughA new review rule, "No Test or Debug Seam in Production Source," is added across three tooling systems. The rule's canonical definition is authored as a markdown doc, then registered in the review-bot README, Greptile's config and files JSON plus rules summary, and CodeRabbit's path instructions and pre-merge checks—all targeting Swift files under ChangesNo Test/Debug Seam in Production Source Rule
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 22✅ Passed checks (22 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 @.coderabbit.yaml:
- Line 24: The rule patterns for detecting test seams in the
no-test-debug-seam-in-production-source rule are inconsistent between the path
instruction and the pre-merge check, allowing seam names to slip through
enforcement. Update both the instruction at line 24 and the check at line 141 to
uniformly reference the canonical seam name patterns `…TestSeam` and `_test…` so
that naming pattern detection is consistent across both locations and no
violations can be missed.
In @.greptile/config.json:
- Line 83: The rule string in the "rule" field is missing the `_test…` pattern
from the canonical definition, which creates detection drift. Add `_test…` as an
additional naming pattern to the list of member names being checked for
test-only or debug-only seams. This pattern should be included alongside the
existing patterns like debug…, …ForTesting, …ForTests, testOnly…, …TestHook, and
…TestSeam to ensure the configuration matches the canonical definition and
prevents drift between the rule enforcement and documentation.
In @.greptile/rules.md:
- Around line 67-70: Add the missing `_test…` naming pattern to the list of
test-observability accessor patterns in the rule that fails `#if DEBUG`
extensions or members exposing internal state. The pattern list currently
includes `debug…`, `…ForTesting`, `…ForTests`, `testOnly…`, `…TestHook`, and
`…TestSeam`, but is missing `_test…` which exists in the canonical
source-of-truth rule. Insert this pattern into the enumerated list of naming
conventions to maintain complete pattern parity and prevent false negatives in
debug/test seam detection.
🪄 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: 0998cd1b-435b-495a-a766-bbf9de92395b
📒 Files selected for processing (6)
.coderabbit.yaml.github/review-bot-rules/README.md.github/review-bot-rules/no-test-debug-seam-in-production-source.md.greptile/config.json.greptile/files.json.greptile/rules.md
Contributor
Greptile SummaryThis PR is a docs/config-only change that propagates a new "no test or debug seam in production source" review rule into both CodeRabbit and Greptile. No app or runtime code is modified.
Confidence Score: 5/5Docs and review-config only — no production Swift or runtime code is touched, so there is no risk of runtime regression. All six changed files are documentation or bot-configuration files. The canonical rule file is well-formed, and all seven naming patterns (including …TestSeam and _test… that were flagged in earlier review rounds) are now consistently present across every propagated location. The scope glob and pass/fail conditions are correctly aligned between the rule file and both bot configs. No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["PR diff contains Swift file\nmatching **/Sources/**/*.swift"] --> B{Under **/Tests/**?}
B -- Yes --> PASS["PASS — test scaffolding is fine"]
B -- No --> C{Adds #if DEBUG / test-build-guarded\nmember with no production caller?}
C -- Yes --> FAIL["FAIL — test-observability seam\nin production source"]
C -- No --> D{"Member name matches\ndebug… / …ForTesting / …ForTests /\ntestOnly… / …TestHook /\n…TestSeam / _test…?"}
D -- Yes --> FAIL
D -- No --> E{Visibility widened AND\nwrapper accessor added for test?}
E -- Yes --> FAIL
E -- No --> PASS
FAIL --> FIX["Preferred fix:\nwiden private → internal,\nread from test target via\n@testable import\n(ref: cmux PR 6452)"]
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
A["PR diff contains Swift file\nmatching **/Sources/**/*.swift"] --> B{Under **/Tests/**?}
B -- Yes --> PASS["PASS — test scaffolding is fine"]
B -- No --> C{Adds #if DEBUG / test-build-guarded\nmember with no production caller?}
C -- Yes --> FAIL["FAIL — test-observability seam\nin production source"]
C -- No --> D{"Member name matches\ndebug… / …ForTesting / …ForTests /\ntestOnly… / …TestHook /\n…TestSeam / _test…?"}
D -- Yes --> FAIL
D -- No --> E{Visibility widened AND\nwrapper accessor added for test?}
E -- Yes --> FAIL
E -- No --> PASS
FAIL --> FIX["Preferred fix:\nwiden private → internal,\nread from test target via\n@testable import\n(ref: cmux PR 6452)"]
Reviews (2): Last reviewed commit: "review-bot-rules: sync _test… and …TestS..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+22
to
+24
| - path: "**/Sources/**/*.swift" | ||
| instructions: | | ||
| Apply `.github/review-bot-rules/no-test-debug-seam-in-production-source.md` during review. For Swift files under a production `Sources/` path (not under `Tests/`), flag added test-only or debug-only seams: `#if DEBUG` (or other test-build-guarded) extensions/members that expose internal state for tests or a debugger with no production caller, members named like `debug…`/`…ForTesting`/`…ForTests`/`testOnly…`/`…TestHook`, or visibility widened plus a wrapper accessor added "so the test can call it". Prefer reaching internal state from the test target via `@testable import` after widening `private` to `internal`; isolate genuinely debug-only facilities in a dedicated debug file or folder. Pass for `#if DEBUG` blocks gating real product behavior and existing seams not worsened. |
Contributor
There was a problem hiding this comment.
path_instructions block for **/Sources/**/*.swift is missing two naming patterns that appear in the canonical rule file (.github/review-bot-rules/no-test-debug-seam-in-production-source.md line 14): …TestSeam and _test…. The same file's custom_checks block already includes …TestSeam, so this creates an internal inconsistency within .coderabbit.yaml as well. A member named _testAccessQueue or _testWriterQueue would slip past both the path instruction and the custom check as written.
Suggested change
| - path: "**/Sources/**/*.swift" | |
| instructions: | | |
| Apply `.github/review-bot-rules/no-test-debug-seam-in-production-source.md` during review. For Swift files under a production `Sources/` path (not under `Tests/`), flag added test-only or debug-only seams: `#if DEBUG` (or other test-build-guarded) extensions/members that expose internal state for tests or a debugger with no production caller, members named like `debug…`/`…ForTesting`/`…ForTests`/`testOnly…`/`…TestHook`, or visibility widened plus a wrapper accessor added "so the test can call it". Prefer reaching internal state from the test target via `@testable import` after widening `private` to `internal`; isolate genuinely debug-only facilities in a dedicated debug file or folder. Pass for `#if DEBUG` blocks gating real product behavior and existing seams not worsened. | |
| - path: "**/Sources/**/*.swift" | |
| instructions: | | |
| Apply `.github/review-bot-rules/no-test-debug-seam-in-production-source.md` during review. For Swift files under a production `Sources/` path (not under `Tests/`), flag added test-only or debug-only seams: `#if DEBUG` (or other test-build-guarded) extensions/members that expose internal state for tests or a debugger with no production caller, members named like `debug…`/`…ForTesting`/`…ForTests`/`testOnly…`/`…TestHook`/`…TestSeam`/`_test…`, or visibility widened plus a wrapper accessor added "so the test can call it". Prefer reaching internal state from the test target via `@testable import` after widening `private` to `internal`; isolate genuinely debug-only facilities in a dedicated debug file or folder. Pass for `#if DEBUG` blocks gating real product behavior and existing seams not worsened. |
| }, | ||
| { | ||
| "id": "cmux-no-test-debug-seam-in-production-source", | ||
| "rule": "Flag Swift files under a production Sources path (matching **/Sources/** and not under **/Tests/**) that add a test-only or debug-only seam: a #if DEBUG (or other test-build-guarded) extension/member exposing internal state only for tests or a debugger with no production caller, a member named like debug…/…ForTesting/…ForTests/testOnly…/…TestHook/…TestSeam, or visibility widened together with a wrapper accessor added so a test can call it. Prefer observing internal state from the test target via @testable import after widening private to internal, or isolating a genuinely debug-only facility in a dedicated debug file or folder (canonical fix: cmux PR 6452). Pass for #if DEBUG blocks that gate real product behavior, scaffolding inside Tests/ or a test-support module, and existing seams not worsened by the PR.", |
Contributor
There was a problem hiding this comment.
_test… naming pattern defined in the canonical rule file (.github/review-bot-rules/no-test-debug-seam-in-production-source.md line 14) is absent from all propagated versions. It is missing here in config.json, as well as in .coderabbit.yaml's custom_checks block (which otherwise includes …TestSeam) and in .greptile/rules.md. A method like _testQueueDepth() in a production Sources/ file would not be caught by any bot as currently configured.
Suggested change
| "rule": "Flag Swift files under a production Sources path (matching **/Sources/** and not under **/Tests/**) that add a test-only or debug-only seam: a #if DEBUG (or other test-build-guarded) extension/member exposing internal state only for tests or a debugger with no production caller, a member named like debug…/…ForTesting/…ForTests/testOnly…/…TestHook/…TestSeam, or visibility widened together with a wrapper accessor added so a test can call it. Prefer observing internal state from the test target via @testable import after widening private to internal, or isolating a genuinely debug-only facility in a dedicated debug file or folder (canonical fix: cmux PR 6452). Pass for #if DEBUG blocks that gate real product behavior, scaffolding inside Tests/ or a test-support module, and existing seams not worsened by the PR.", | |
| "rule": "Flag Swift files under a production Sources path (matching **/Sources/** and not under **/Tests/**) that add a test-only or debug-only seam: a #if DEBUG (or other test-build-guarded) extension/member exposing internal state only for tests or a debugger with no production caller, a member named like debug…/…ForTesting/…ForTests/testOnly…/…TestHook/…TestSeam/_test…, or visibility widened together with a wrapper accessor added so a test can call it. Prefer observing internal state from the test target via @testable import after widening private to internal, or isolating a genuinely debug-only facility in a dedicated debug file or folder (canonical fix: cmux PR 6452). Pass for #if DEBUG blocks that gate real product behavior, scaffolding inside Tests/ or a test-support module, and existing seams not worsened by the PR.", |
Collaborator
Author
There was a problem hiding this comment.
Fixed in the latest commit: added _test… (and …TestSeam where missing) to the CodeRabbit path_instructions and custom_check, plus the Greptile config.json rule and rules.md, so all propagated copies match the canonical .md source.
— Claude Code
|
|
||
| For Swift files under a production `Sources/` path (matching `**/Sources/**` and not under `**/Tests/**`), flag added test-only or debug-only seams. | ||
|
|
||
| Fail a `#if DEBUG` (or other test-build-guarded) extension or member that exposes internal/private state for tests or a debugger with no production caller, a member named like `debug…`/`…ForTesting`/`…ForTests`/`testOnly…`/`…TestHook`/`…TestSeam`, or visibility widened together with a wrapper accessor added so a test can call it. The compiled-out `#if DEBUG` guard does not make a test-observability accessor acceptable in shipping source. |
Contributor
There was a problem hiding this comment.
_test… omission: the rules section here does not include the _test… naming pattern that the canonical .md source lists. This copy should stay in sync with the source rule file.
Suggested change
| Fail a `#if DEBUG` (or other test-build-guarded) extension or member that exposes internal/private state for tests or a debugger with no production caller, a member named like `debug…`/`…ForTesting`/`…ForTests`/`testOnly…`/`…TestHook`/`…TestSeam`, or visibility widened together with a wrapper accessor added so a test can call it. The compiled-out `#if DEBUG` guard does not make a test-observability accessor acceptable in shipping source. | |
| Fail a `#if DEBUG` (or other test-build-guarded) extension or member that exposes internal/private state for tests or a debugger with no production caller, a member named like `debug…`/`…ForTesting`/`…ForTests`/`testOnly…`/`…TestHook`/`…TestSeam`/`_test…`, or visibility widened together with a wrapper accessor added so a test can call it. The compiled-out `#if DEBUG` guard does not make a test-observability accessor acceptable in shipping source. |
Collaborator
Author
There was a problem hiding this comment.
Fixed: .greptile/rules.md now lists _test… in the fail-member naming list, matching the canonical rule file.
— Claude Code
CodeRabbit/Greptile flagged that the propagated bot-instruction copies dropped the `_test…` fail-pattern (and CodeRabbit path_instructions also dropped `…TestSeam`) that the canonical rule .md enumerates. Add them back so the bot instructions match the source-of-truth rule file. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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.
Propagates a saved human review lesson into the CodeRabbit + Greptile review rules.
Owner review comment (Aziz): "if you're going to extend anything for tests, put it in the tests target. If you're going to extend anything for debugging, put it in its own debug folder if possible."
Context: a
#if DEBUGtest-observability accessor (debugQueuedRequestCount()) was added to productionCmuxMobileRPCsources so a test could read the actor's private writer-queue state. The correct fix (#6452, commit 350dda4) removed the production seam, widened the queue stateprivate->internal, and observed it from the test target via@testable import.What's here
New rule
.github/review-bot-rules/no-test-debug-seam-in-production-source.md: for Swift files under a productionSources/path (notTests/), flag test-only/debug-only seams added inline —#if DEBUG(or other test-build-guarded) accessors with no production caller, members nameddebug…/…ForTesting/…ForTests/testOnly…/…TestHook, or visibility widened plus a wrapper accessor "so the test can call it". Preferred fix: observe internal state from the test target via@testable importafter wideningprivate->internal; isolate a genuinely debug-only facility in a dedicated debug file/folder.Mirrored into both bots:
reviews.path_instructionsscoped to**/Sources/**/*.swift+ blocking custom checkcmux no test or debug seam in production source.cmux-no-test-debug-seam-in-production-source(scope**/Sources/**/*.swift) inconfig.json, plusfiles.jsonentry and arules.mdsection.Review-config and docs only. No app/runtime code, so no tagged reload.
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Summary by cubic
Adds a blocking review rule that prevents test-only or debug-only seams in production Swift
Sources/files. Enforced viaCodeRabbitandGreptile; docs/config only, no runtime changes..github/review-bot-rules/no-test-debug-seam-in-production-source.md; scope**/Sources/**/*.swift(notTests/).#if DEBUGaccessors; members nameddebug…,…ForTesting,…ForTests,testOnly…,…TestHook,…TestSeam,_test…; or visibility widened plus a wrapper added for tests. Preferred fix: use@testable importwithprivate->internal, or isolate debug code in a dedicated file/folder..coderabbit.yamlwith path instructions and a blocking check "cmux no test or debug seam in production source"; syncs_test…and…TestSeampatterns to match the rule..greptile/config.json,.greptile/files.json, and.greptile/rules.mdwith rule idcmux-no-test-debug-seam-in-production-sourceand the same patterns; lists the rule in.github/review-bot-rules/README.md.Written for commit 093643e. Summary will update on new commits.
Summary by CodeRabbit