fix: get forge lint -D warnings green again by thedavidmeister · Pull Request #217 · rainlanguage/rain.flare

thedavidmeister · 2026-10-07T15:45:34Z

forge lint -D warnings is red on main right now, independently of any open
branch. The rainix pin the sol workflows run moved on 2026-09-29
(RAINIX_SHA -> 8657b83); rain.flare last pushed to main on 2026-09-14, so
nothing has exercised the static job since and the breakage has been sitting
latent. Every branch opened off main reds on it — on the branch I hit this
from, the test and legal jobs pass and only static fails.

Two separate causes, and the first hides the second.

solc aborts before forge-lint is reached

forge lint only compiles what slither's build skipped (slither runs
--skip ./test/** ./script/**), so in CI it compiles test/ fresh — and -D
is a compiler flag, so it denies solc's own diagnostics too:

Error: Compiler run failed:
Warning (2018): Function state mutability can be restricted to view
  --> test/src/lib/flreth/LibDineroFlrEth.t.sol:29:5
  --> test/src/lib/flreth/LibDineroFlrEth.t.sol:33:5

Those are the two expectRevert wrappers, which only read. They get view.

Worth recording because it cost me a wrong diagnosis: locally these warnings
do not appear at all, because any earlier forge build has already cached
those artifacts and solc never re-runs for them. forge clean first is the only
faithful local reproduction of the CI step.

the ten forge-lint findings behind it

With solc passing they become reachable. Each gets the scoped
//forge-lint: disable-next-line(<id>) plus the line saying why, which is what
the workflow's own comment asks for, rather than a wholesale silence:

count	lint	why it is the behavior, not a defect
2	`block-timestamp`	comparing against `block.timestamp` IS the staleness check `LibFtsoV2LTS` / `LibFtsoCurrentPriceUsd` implement; both already carry the matching `slither-disable-next-line timestamp`
6	`unsafe-typecast`	`bytes32("<word name>")` on ASCII literals far under 32 bytes — and the literal is precisely what the test asserts
2	`unsafe-typecast`	`bytes21()` over a `new bytes(21)` allocation, so nothing is dropped; and `bytes1(feedId)`, whose entire job is to keep the leading category byte

QA

Discriminating tests: n/a — no behavior changes, so there is nothing new to
discriminate. The existing suite is the regression check and it is unchanged at
85 passed / 0 failed. The gate itself is the test: forge clean then
forge lint -D warnings goes from Error: Compiler run failed to
Compiler run successful! with no findings printed.
Mutations applied: n/a — a view keyword and ten comment lines, so there is no
behavior to mutate. The two libraries this touches were mutation-probed
separately on the fork-block branch (8 mutants, 8 killed, 0 survived) and the
disables added here change no line that probe targeted.
Oracle: the CI step's own output, read from the failing run's log rather than
inferred — forge lint -D warnings on 8657b83, reproduced locally under the
same pinned shell after forge clean.
Category check: the gate has two independent causes; covered both (solc
mutability warnings, and all ten forge-lint findings). Nothing is silenced
wholesale and no finding is left unexplained.

🤖 Generated with Claude Code

Summary by CodeRabbit

Chores
- Clarified comments around price-staleness checks and feed ID encoding; the checks and behavior remain unchanged.
- Added lint suppressions for intentional timestamp checks and type casts, and clarified a naming-convention suppression.
- Updated test wrappers to declare rate-getter calls as read-only. Test assertions remain unchanged. These updates affect code annotations and test declarations only; they do not change the functionality available to end users.

The rainix pin the sol workflows run moved on 2026-09-29; rain.flare last pushed to main on 2026-09-14, so nothing has exercised the static job since and `forge lint -D warnings` is red on main independently of any open branch. Two separate causes, and the first hides the second. solc aborts first. `forge lint` only sees the files slither's build skipped, so in CI it compiles test/ fresh and `-D warnings` denies solc's own diagnostics: two `Function state mutability can be restricted to view` on the flrETH expectRevert wrappers. Locally they never appear, because a previous `forge build` already cached those artifacts — the only faithful local reproduction is `forge clean` first. With those fixed solc passes and the ten forge-lint findings behind it become reachable, each getting the scoped disable plus its reason that the workflow's own comment asks for rather than a wholesale silence: block-timestamp x2 comparing against block.timestamp IS the staleness check these two libraries implement unsafe-typecast x6 bytes32("<word name>") on ASCII literals far under 32 bytes, and the literal is what the test asserts unsafe-typecast x2 bytes21() over a 21-byte allocation, and bytes1() whose whole job is to keep the leading byte No behavior changes: 85 passed / 0 failed, and `forge clean` followed by `forge lint -D warnings` is now silent. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

coderabbitai · 2026-10-07T15:45:51Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Advanced
Run ID: 7261f3a0-66f7-4092-9bbb-c2d1983b2b01

📥 Commits

Reviewing files that changed from the base of the PR and between 9eabc7a and df9d1b8.

📒 Files selected for processing (3)

src/interface/IDineroFlrEth.sol
src/lib/lts/LibFtsoV2LTS.sol
src/lib/price/LibFtsoCurrentPriceUsd.sol

Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.

Walkthrough

The change adds lint suppressions and comments around timestamp comparisons and byte conversions. It also declares two test wrapper functions view. The stale-price checks and test assertions remain unchanged.

Changes

Solidity lint and test declaration updates

Layer / File(s)	Summary
Lint annotations and test declarations `src/lib/lts/LibFtsoV2LTS.sol`, `src/lib/price/LibFtsoCurrentPriceUsd.sol`, `src/interface/IDineroFlrEth.sol`, `test/src/concrete/*`, `test/src/lib/flreth/LibDineroFlrEth.t.sol`, `test/src/lib/lts/LibFtsoV2LTSFeedIds.t.sol`	The stale-price checks receive lint suppressions. Test assertions and feed-ID conversions receive comments or lint suppressions. Two test wrappers are declared `view`. The interface suppression now names the `naming-convention` detector.

Priority: ⬇️ Low

Estimated code review effort: 1 (Trivial) | ~3 minutes

Change: Other

Merge Risk: ⚪ Minimal · up to df9d1

The changes adjust lint suppressions and mark two existing test wrappers as view; production checks and test assertions remain unchanged. No user-facing risk is evident; merge after routine CI checks.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly summarizes the main change: resolving failures from `forge lint -D warnings`.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 0…
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches 💡 1

🛠️ Fix failing CI checks 💡

Commit to this branch
Create a new PR

🧪 Generate unit tests (beta)

Commit to this branch
Create a new PR

Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands.}

`slither .` exits 255 on this branch and the 255 is this branch's doing. `SlitherCore.has_ignore_comment` reads exactly `min(lines) - 1` for a finding, so `//slither-disable-next-line timestamp` only suppresses while it is the line immediately above the flagged code. Two of the ten scoped `//forge-lint: disable-next-line(block-timestamp)` comments the previous commit added landed between an existing slither comment and its `if`, pushing the slither comment to `min(lines) - 2` and silently unsuppressing both `timestamp` findings. Two results under the default `--fail-on pedantic` is `sys.exit(-1)`. Slither's region form carries no adjacency requirement — `has_ignore_comment` matches on `start < lines[0] and end > lines[-1]` — so both sites take `//slither-disable-start timestamp` / `//slither-disable-end timestamp` around the `if` and leave the forge-lint comment where forge lint needs it. That is the shape `src/interface/IDineroFlrEth.sol` already uses for the same collision. No detector is excluded and `slither.config.json` is untouched. The two `ERROR:Slither:` lines on the same CI run are an older, separate bug that pre-dates both branches. `parse_ignore_comments` does `end_match[0].split(",")`, so a bare `//slither-disable-end` asks for the range of a check literally named `""`, which no `-start` ever opened: it logs "slither-disable-end without slither-disable-start", returns, and leaves `naming-convention` open from line 6 to infinity, after which the second pass over the file logs "Consecutive slither-disable-starts". Hence the end comment now names its detector. Closing the range surfaces nothing new, because `tokensPerLST` is already valid mixedCase; only `LSTPerToken` ever needed it. Unpiped exits in the devshell at the CI pin, after `slither .`'s own `forge clean` so `forge lint` compiles test/ fresh the way CI does: slither . 0 (0 result(s) found, no ERROR lines) forge fmt --check 0 forge lint -D warnings 0 pre-commit --all-files 0 rainix-sol-single-contract 0 comment-loc-cap 0 (src 1631 against a cap of 1874) What separates the two causes: with only the `-end` detector name fixed and the two timestamp sites left as they were, `slither .` is still 255 with the same 2 results and no ERROR lines at all. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

…k change #217 fixes `forge lint -D warnings` and `slither .` being red on `main` for reasons unrelated to the fork block. Carried here so the static job on this branch reports on the fork change rather than on #217's breakage; it drops out of this diff the moment #217 merges. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

linear · 2026-10-07T17:24:22Z

RAI-2981

thedavidmeister mentioned this pull request Oct 7, 2026

test: pin every flare fork test at block 37796420 #218

Merged

thedavidmeister merged commit 1059e7d into main Oct 7, 2026
6 of 7 checks passed

Navigation Menu

Repository navigation

fix: get forge lint -D warnings green again - #217

fix: get forge lint -D warnings green again#217
thedavidmeister merged 2 commits into
mainfrom
2026-10-07-forge-lint-d-warnings

thedavidmeister commented Oct 7, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

coderabbitai Bot commented Oct 7, 2026 •

edited

Loading

Uh oh!

Uh oh!

linear Bot commented Oct 7, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

thedavidmeister commented Oct 7, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

solc aborts before forge-lint is reached

the ten forge-lint findings behind it

QA

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented Oct 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Uh oh!

Uh oh!

linear Bot commented Oct 7, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

thedavidmeister commented Oct 7, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Oct 7, 2026 •

edited

Loading