audit: Solidity file placement by kind — own kind in src/ and the suite tree, subject path in the test/src mirror - #68
Conversation
…kind, test/src/** mirrors src/ by subject path The skill classifies files as library/concrete/abstract/interface for the pragma rule but never acts on that classification for PLACEMENT, so a concrete contract in a lib/ directory passes an audit clean (rain.erc4626.words#230's tree, rain.deploy#21's mocks). Add the Domain rule beside the pragma convention: kind-named directories under src/ and test/ (the suite's own support code) place a file by its OWN declared kind, while test/src/** is a mirror of src/ whose .t.sol files are placed by their SUBJECT's path — so a non-.t.sol support file inside the mirror tree is itself a finding, and test/src/lib/LibFoo.t.sol beside its subject is the tree doing its job. util(s)/ is the legacy layout: new files there are findings, existing trees are one INFO migration candidate, and a whole-tree reshuffle stays an owner decision. LOW/INFO, restraint mirrored from the pragma rule; dimension 4 #1 takes this rule as the Domain authority for placement and #7's test/util citation is re-worded as where to LOOK, not an endorsement. Closes #63 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
✨ 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 |
|
QA record for this PR — checks run before push, and the sibling-repo verification the rule is derived from. Checks run
Sibling-repo verification (GitHub API,
|
| repo | src/ kind dirs |
suite tree (test/<kind>/) |
mirror tree (test/src/<kind>/) |
|---|---|---|---|
| rain.interpreter | abstract, concrete, interface, lib (+ error, generated) |
abstract (5), lib (8), legacy utils (1) |
abstract, concrete, lib (127 files) |
| rain.math.float | concrete, lib (+ error, generated) |
abstract (1), concrete (1), lib (3) |
concrete, lib (84 files) |
| rain.flare | abstract, concrete, interface, lib (+ err, generated, vendor) |
abstract (1), lib (1) (+ fork, prod) |
concrete, lib (15 files) |
| rain.dia | abstract, concrete, lib (+ generated) |
lib (3) (+ script) |
concrete, lib (13 files) |
| rain.erc4626.words | abstract, concrete, lib (+ generated) |
lib (1), legacy utils (3) |
abstract, concrete, lib (27 files) |
Declared-kind spot checks (file fetched, first declaration read):
rain.math.float/test/concrete/TestDecimalFloat.sol→contract— suite tree, own kind, correct.rain.math.float/test/abstract/LogTest.sol→abstract contract— correct.rain.interpreter/test/abstract/OpTest.sol→abstract contract— correct.rain.interpreter/test/lib/string/LibCamelToKebab.t.solbesideLibCamelToKebab.sol— a test OF suite-own code sits beside its subject in the suite tree (the example inlined in the rule).rain.deployPR audit: add events-belong-on-the-interface check (Code quality #12) #21 head:test/src/lib/MockDeployable.sol,MockDeployableV2.sol,MockReverter.sol→ allcontract— the rule's inline worked example (home:test/concrete/).- Drift the rule would catch in existing trees (reported, not precedent):
rain.interpreter/test/src/concrete/MockExternBadLiteralIndex.sol(contract, support code inside the mirror tree),rain.erc4626.words/test/src/abstract/{DefaultExternWords,OverriddenExternWords,ERC4626SubParser.externWrapper,ERC4626SubParser.wordParsers.wrapper}.sol(allcontract, wrong on both axes),rain.dia/test/src/lib/dia/LibDiaGetOracleContractExternalWrapper.sol(contract),rain.flare/test/lib/lts/FeedConsumer.sol(contractintest/lib/).
No sibling contradicts the two-tree model; every non-conforming file above is drift the rule is written to catch, and rain.math.float shows both trees operating side by side (test/concrete/TestDecimalFloat.sol beside test/src/concrete/DecimalFloat.*.t.sol).
🤖 Generated with Claude Code
WalkthroughThe audit skill is updated to version 0.23.0, adds Solidity file-placement rules by declaration kind, expands test-helper search paths, and aligns both plugin manifests with the new version. ChangesAudit skill update
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@skills/audit/SKILL.md`:
- Line 157: Clarify the multi-declaration classification rules in the
file-classification guidance: select the declaration matching the filename as
primary, define a deterministic fallback when none matches, and require files
with multiple equally valid candidates or no unambiguous primary to emit one
manual-review/INFO finding instead of choosing based on implementation order.
- Around line 153-160: Clarify the placement policy for interfaces in
suite-owned test code near the documented test partition and flag rule: either
add test/interface/ as the valid destination for test-owned interfaces and
include it in the supported layout, or explicitly exclude suite-owned test
interfaces from the interface placement flag. Ensure the resulting rules do not
leave valid test interfaces without a documented home or flag every possible
placement.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: b6384ad9-df17-4b81-9673-274b2b810445
📒 Files selected for processing (3)
.claude-plugin/marketplace.json.claude-plugin/plugin.jsonskills/audit/SKILL.md
…ake the multi-declaration classifier deterministic
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)
skills/audit/SKILL.md (1)
242-242: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winInclude
test/interface/**in helper discovery.The placement rule now supports four suite-owned partitions, but the Test-util DRY guidance still searches only
test/{lib,abstract,concrete}/**/*.sol. Auditors can therefore miss existing interface helpers.Proposed fix
- 7. **Test-util DRY** — before reviewing tests, read the existing helpers wherever the repo keeps them: `test/{lib,abstract,concrete}/**/*.sol` + 7. **Test-util DRY** — before reviewing tests, read the existing helpers wherever the repo keeps them: `test/{lib,abstract,concrete,interface}/**/*.sol`🤖 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 `@skills/audit/SKILL.md` at line 242, Update the “Test-util DRY” helper-discovery paths to include test/interface/**/*.sol alongside the existing test/lib, test/abstract, and test/concrete partitions, while retaining the legacy test/util(s) lookup guidance and existing placement-rule distinction.
🤖 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 `@skills/audit/SKILL.md`:
- Around line 155-161: Update the classification logic described around the
filename/declaration rule to exempt `test/src/**` mirror tests from own-kind
matching, strip `.t.sol`, and validate their placement against the corresponding
existing `src/**` subject path instead. Add an explicit LOW/INFO finding when a
mirror test’s subject path does not match an existing source file, while
preserving own-kind classification for `test/**` files outside `test/src/**` and
the existing subject-path behavior for valid mirrors.
---
Outside diff comments:
In `@skills/audit/SKILL.md`:
- Line 242: Update the “Test-util DRY” helper-discovery paths to include
test/interface/**/*.sol alongside the existing test/lib, test/abstract, and
test/concrete partitions, while retaining the legacy test/util(s) lookup
guidance and existing placement-rule distinction.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 3c7301e8-cf31-45a5-bdc3-dc744b1d260d
📒 Files selected for processing (1)
skills/audit/SKILL.md
| - **`test/src/**` — a MIRROR of `src/**`, placed by the SUBJECT's path, never the file's own kind.** This tree holds the `.t.sol` tests of first-party source, each at its subject's mirrored path: tests of `src/lib/LibFoo.sol` → `test/src/lib/LibFoo.t.sol`; tests of `src/concrete/Bar.sol` → `test/src/concrete/Bar.parse.t.sol`. Every `.t.sol` file declares a concrete contract, so judging this tree by own-kind would move every test to `concrete/` — exactly wrong; `test/src/lib/LibFoo.t.sol` in `lib/` is the mirror tree doing its job. The discriminator between the two `test/` regimes is the `src/` path segment: `test/src/...` is the mirror tree (`.t.sol` placed by subject path); `test/...` without it is suite-own code (placed by its own kind). | ||
|
|
||
| A file is classified by the declaration whose name EQUALS the filename (minus `.sol`); a subsidiary helper declaration in the same file (a small `library` beside the `contract` the file names) does not move the file. Where NO declaration matches the filename, or several do (overloaded via inheritance tricks), the kind is ambiguous: report ONE INFO manual-review finding naming the candidates rather than picking by declaration order — an order-dependent pick is the **Explicit handling of ambiguous sets** defect applied to a classifier, and the filename-mismatch is usually itself a finding (filename should equal the contract it ships). | ||
|
|
||
| **Flag** (**LOW/INFO** — a convention rule; nothing breaks, but the partition is what lets a reader trust that everything in `lib/` is internal-only, bytecode-free code and everything in `concrete/` deploys — and the pragma rule above keys off the same classification, so a misplaced file invites a mispinned pragma): | ||
| - A file in or under a kind-named directory rooted at `src/<kind>/` or `test/<kind>/` (nested subdirs like `src/lib/op/` included; `test/src/<kind>/` excluded — the mirror tree is judged by subject path, per above) contradicting that kind: a non-`.t.sol` `contract` in `lib/` or `abstract/`, a `library` in `concrete/`, an `abstract contract` in `lib/` or `concrete/`, an `interface` outside `interface/`. | ||
| - A non-`.t.sol` support file anywhere inside `test/src/**`: the mirror tree holds tests of source; support code's home is `test/{lib,concrete,abstract,interface}/` by its own kind. Live shape: `rain.deploy`'s `test/src/lib/MockDeployable.sol` (`contract MockDeployable`) is misplaced on both axes — a concrete in a `lib/` directory, and suite support inside the mirror tree. Its home is `test/concrete/` — NOT `test/src/concrete/`, which would claim tests of a `src/concrete/` that repo does not have. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Exclude mirror tests from own-kind classification and enforce their subject paths explicitly.
Line 157 currently applies the filename/declaration classifier globally. For a conventional test/src/lib/Foo.t.sol, stripping only .sol yields Foo.t, so no Solidity declaration can match and the rule can emit an ambiguity or filename-mismatch finding even though line 155 says mirror tests are judged by their subject path. Also add an explicit finding condition for .t.sol files whose mirror path does not match an existing source subject.
Proposed clarification
- `test/src/**` — a MIRROR of `src/**`, placed by the SUBJECT's path, never the file's own kind.
+ Mirror `.t.sol` files are not classified by their own declarations; resolve them by the
+ corresponding source subject path instead.
+ Flag a `.t.sol` whose path does not mirror an existing source subject.
- A file is classified by the declaration whose name EQUALS the filename (minus `.sol`);
+ For files whose placement is by OWN kind (`src/**` and suite-owned `test/**`), classify
+ by the declaration whose name EQUALS the filename (minus `.sol`);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - **`test/src/**` — a MIRROR of `src/**`, placed by the SUBJECT's path, never the file's own kind.** This tree holds the `.t.sol` tests of first-party source, each at its subject's mirrored path: tests of `src/lib/LibFoo.sol` → `test/src/lib/LibFoo.t.sol`; tests of `src/concrete/Bar.sol` → `test/src/concrete/Bar.parse.t.sol`. Every `.t.sol` file declares a concrete contract, so judging this tree by own-kind would move every test to `concrete/` — exactly wrong; `test/src/lib/LibFoo.t.sol` in `lib/` is the mirror tree doing its job. The discriminator between the two `test/` regimes is the `src/` path segment: `test/src/...` is the mirror tree (`.t.sol` placed by subject path); `test/...` without it is suite-own code (placed by its own kind). | |
| A file is classified by the declaration whose name EQUALS the filename (minus `.sol`); a subsidiary helper declaration in the same file (a small `library` beside the `contract` the file names) does not move the file. Where NO declaration matches the filename, or several do (overloaded via inheritance tricks), the kind is ambiguous: report ONE INFO manual-review finding naming the candidates rather than picking by declaration order — an order-dependent pick is the **Explicit handling of ambiguous sets** defect applied to a classifier, and the filename-mismatch is usually itself a finding (filename should equal the contract it ships). | |
| **Flag** (**LOW/INFO** — a convention rule; nothing breaks, but the partition is what lets a reader trust that everything in `lib/` is internal-only, bytecode-free code and everything in `concrete/` deploys — and the pragma rule above keys off the same classification, so a misplaced file invites a mispinned pragma): | |
| - A file in or under a kind-named directory rooted at `src/<kind>/` or `test/<kind>/` (nested subdirs like `src/lib/op/` included; `test/src/<kind>/` excluded — the mirror tree is judged by subject path, per above) contradicting that kind: a non-`.t.sol` `contract` in `lib/` or `abstract/`, a `library` in `concrete/`, an `abstract contract` in `lib/` or `concrete/`, an `interface` outside `interface/`. | |
| - A non-`.t.sol` support file anywhere inside `test/src/**`: the mirror tree holds tests of source; support code's home is `test/{lib,concrete,abstract,interface}/` by its own kind. Live shape: `rain.deploy`'s `test/src/lib/MockDeployable.sol` (`contract MockDeployable`) is misplaced on both axes — a concrete in a `lib/` directory, and suite support inside the mirror tree. Its home is `test/concrete/` — NOT `test/src/concrete/`, which would claim tests of a `src/concrete/` that repo does not have. | |
| - **`test/src/**` — a MIRROR of `src/**`, placed by the SUBJECT's path, never the file's own kind.** This tree holds the `.t.sol` tests of first-party source, each at its subject's mirrored path: tests of `src/lib/LibFoo.sol` → `test/src/lib/LibFoo.t.sol`; tests of `src/concrete/Bar.sol` → `test/src/concrete/Bar.parse.t.sol`. Every `.t.sol` file declares a concrete contract, so judging this tree by own-kind would move every test to `concrete/` — exactly wrong; `test/src/lib/LibFoo.t.sol` in `lib/` is the mirror tree doing its job. The discriminator between the two `test/` regimes is the `src/` path segment: `test/src/...` is the mirror tree (`.t.sol` placed by subject path); `test/...` without it is suite-own code (placed by its own kind). | |
| Mirror `.t.sol` files are not classified by their own declarations; resolve them by the corresponding source subject path instead. | |
| Flag a `.t.sol` whose path does not mirror an existing source subject. | |
| For files whose placement is by OWN kind (`src/**` and suite-owned `test/**`), classify by the declaration whose name EQUALS the filename (minus `.sol`); a subsidiary helper declaration in the same file (a small `library` beside the `contract` the file names) does not move the file. Where NO declaration matches the filename, or several do (overloaded via inheritance tricks), the kind is ambiguous: report ONE INFO manual-review finding naming the candidates rather than picking by declaration order — an order-dependent pick is the **Explicit handling of ambiguous sets** defect applied to a classifier, and the filename-mismatch is usually itself a finding (filename should equal the contract it ships). | |
| **Flag** (**LOW/INFO** — a convention rule; nothing breaks, but the partition is what lets a reader trust that everything in `lib/` is internal-only, bytecode-free code and everything in `concrete/` deploys — and the pragma rule above keys off the same classification, so a misplaced file invites a mispinned pragma): | |
| - A file in or under a kind-named directory rooted at `src/<kind>/` or `test/<kind>/` (nested subdirs like `src/lib/op/` included; `test/src/<kind>/` excluded — the mirror tree is judged by subject path, per above) contradicting that kind: a non-`.t.sol` `contract` in `lib/` or `abstract/`, a `library` in `concrete/`, an `abstract contract` in `lib/` or `concrete/`, an `interface` outside `interface/`. | |
| - A non-`.t.sol` support file anywhere inside `test/src/**`: the mirror tree holds tests of source; support code's home is `test/{lib,concrete,abstract,interface}/` by its own kind. Live shape: `rain.deploy`'s `test/src/lib/MockDeployable.sol` (`contract MockDeployable`) is misplaced on both axes — a concrete in a `lib/` directory, and suite support inside the mirror tree. Its home is `test/concrete/` — NOT `test/src/concrete/`, which would claim tests of a `src/concrete/` that repo does not have. |
🤖 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 `@skills/audit/SKILL.md` around lines 155 - 161, Update the classification
logic described around the filename/declaration rule to exempt `test/src/**`
mirror tests from own-kind matching, strip `.t.sol`, and validate their
placement against the corresponding existing `src/**` subject path instead. Add
an explicit LOW/INFO finding when a mirror test’s subject path does not match an
existing source file, while preserving own-kind classification for `test/**`
files outside `test/src/**` and the existing subject-path behavior for valid
mirrors.
|
Reviewed 1bf99e3: ready — placement-by-kind with the two orthogonal test trees and the /src/ discriminator, .t.sol by subject path, suite-own code by own kind across all four kinds, util legacy with no forced migration, deterministic filename-equals-declaration classifier with ambiguity as one INFO finding, both CodeRabbit threads fixed and resolved, sibling verification across five repos with the four drift instances reported as drift not precedent. |
Closes #63
The skill already classifies every Solidity file as
library/contract/abstract contract/interface— the pragma rule depends on it — but never acted on that classification for placement, so a concrete contract in alib/directory passed an audit clean. This adds the Solidity file placement by kind (rainlanguage) Domain rule directly beside the pragma convention (same classifier, same per-file-kind register, same restraint against mass edits), bumps0.22.0 -> 0.23.0in all three version declarations, and re-words dimension 4 #7'stest/util/citation so it reads as where to LOOK for helpers, not an endorsement of the legacy layout.The rule
Two placement regimes across three trees, verified live on
mainin rain.interpreter, rain.flare, rain.erc4626.words, rain.dia, and rain.math.float:src/{lib,concrete,abstract,interface}/— a file is placed by its OWN declared kind.test/{lib,concrete,abstract}/(nosrc/segment) — the test suite's OWN support code (mocks, harnesses, fixtures, test bases, test-only libraries), also placed by its OWN kind:rain.math.floattest/concrete/TestDecimalFloat.sol,rain.interpretertest/abstract/OpTest.sol,test/lib/LibDecimalFloatSlow.sol.test/src/**— a MIRROR ofsrc/**:.t.solfiles placed by their SUBJECT's path, never their own kind.The exception and its justification, measured from the siblings
Every
.t.solfile declares a concrete contract, so a naive own-kind rule would demand every test file in the org move toconcrete/— a mass-churn generator that contradicts every sibling repo (rain.interpreter alone has 96 files undertest/src/lib/, all correctly beside their library subjects). The mirror tree is judged by subject path:test/src/lib/LibFoo.t.solinlib/is the tree doing its job. The flip side is new: a non-.t.solsupport file anywhere insidetest/src/**is itself a finding — that tree holds tests of source, and support code's home is the suite tree by its own kind. The discriminator is stated inline:test/src/...= mirror tree,.t.solby subject path;test/...without thesrc/segment = suite-own code by own kind. A test OF suite-own code sits beside its subject in the suite tree (rain.interpretertest/lib/string/LibCamelToKebab.t.sol), also stated inline with the example.The restraint and why
The claim-making unit is the kind-named DIRECTORY, not tree completeness — a lone
src/lib/still promises libraries (so rain.deploy's shape does not exempt itself) — but the rule never orders a reshuffle: an existingtest/util(s)/tree (the LEGACY layout, per the owner's ruling) or a repo with no kind-named directories at all is ONE INFO migration-candidate finding, never N per-file findings, and whole-tree migration stays an owner decision. NEW files added to a wrong kind-directory or to a util directory are findings. Non-kind directories (generated/,error/,vendor/,test/fork/,test/script/) make no claim. Severity LOW/INFO: nothing breaks, but the partition is what lets a reader trustlib/to be internal-only bytecode-free code — and the pragma rule keys off the same classification, so a misplaced file invites a mispinned pragma. Cross-referenced with dimension 4 #1's convention-derivation order: this rule is the Domain-rule authority for placement, so three mocks sitting consistently together intest/src/lib/still read as misplaced, never as the local standard.The two motivating misses
OverriddenExternWords.sol,DefaultExternWords.sol, the wrapper contracts) sit intest/src/abstract/: wrong on both axes (concrete inabstract/, support code inside the mirror tree), flagged nowhere.test/src/lib/MockDeployable.sol,MockDeployableV2.sol,MockReverter.sol: three concrete contracts in alib/directory inside the mirror tree. Their home istest/concrete/— NOTtest/src/concrete/, which would claim tests of asrc/concrete/that repo does not have. This exact disposition is the rule's inline worked example.QA
version-hygieneCI jobs were replicated locally with the nix-storejq1.8.2: plugin.json == marketplace.json (0.23.0), and base0.22.0-> head0.23.0bump confirmed againstmain(42c385b). SKILL.md frontmatterversion:set to0.23.0by hand (not CI-gated, known gap)..t.sol, suite-tree.t.solbeside suite-own subject, nestedsrc/lib/op/, lone-lib/repos, existing vs new util files), which caught and fixed two real ambiguities before push: the restraint example originally described rain.deploy's exact shape and would have exempted the motivating case, and "undertest/directly" could read as immediate-child-only.maintrees of five sibling repos, fetched via the GitHub API independent of the skill text — rain.interpreter (src/{abstract,concrete,error,generated,interface,lib},test/{abstract,lib,utils}+test/src/{abstract,concrete,lib}), rain.math.float (test/{abstract,concrete,lib}+test/src/{concrete,lib}— both trees at work side by side), rain.flare, rain.dia, rain.erc4626.words — plus declared-kind checks (grep '^contract\|^abstract contract\|^library') on 12 individual files includingTestDecimalFloat.sol(contract intest/concrete/),OpTest.sol(abstract intest/abstract/),LibCamelToKebab.t.sol(suite-tree test beside subject), and rain.deploy PR audit: add events-belong-on-the-interface check (Code quality #12) #21's three mocks (allcontract, all intest/src/lib/)..t.solexception so tests are not mass-flagged, (C) the multi-declaration edge decided, (D) the no-partition repo answered with one layout finding, not N. Covered A (Domain rule beside the pragma convention), B (mirror-tree regime with inline examples both ways), C (placement follows the declaration the file is NAMED for), D (no-kind-directories repo = one INFO migration candidate; a lone kind-named dir still makes its claim, per the owner's two-tree model which supersedes the issue's "probably one INFO" sketch for repos like rain.deploy that do have kind-named dirs).🤖 Generated with Claude Code
Summary by CodeRabbit
Documentation
src/<kind>/andtest/<kind>/, including thetest/src/**mirror-tree exception and rules for identifying misplaced files.Chores