Skip to content

Fix main: the deploy script test still imported the deleted string lib - #211

Merged
thedavidmeister merged 1 commit into
mainfrom
2026-09-15-fix-main-string-set
Sep 15, 2026
Merged

thedavidmeister merged 1 commit into
mainfrom
2026-09-15-fix-main-string-set

Conversation

@thedavidmeister

@thedavidmeister thedavidmeister commented Sep 15, 2026 •

Copy link
Copy Markdown
Contributor

What

test/script/Deploy.t.sol imports LibMemoryKV instead of the deleted
LibStringSet, and its two membership loops build a store and ask it with
has, exactly as the sixteen sites converted in #210 do.

Why

main does not compile. The import names src/lib/LibStringSet.sol, which #210
deleted.

Neither PR was wrong on its own. #186 added this file while the library still
existed and was green against the main of the time. #210 removed the library and
converted every site that then existed, and was green too. #186 was merged after
#210 without being re-run against the new main, so nothing ever compiled the two
together until main did.

The general shape: a green check is evidence about the main the branch was
tested against, not the main it lands on. Two open PRs that touch the same file
can both be green and still break on merge, and this queue has a dozen branches
cut from a main that has moved under all of them.

QA

  • Discriminating tests: none added. The test this file already carries,
    testDeployBroadcastsToEveryConfiguredNetwork, is the discriminator and is
    unchanged in what it asserts and in both of its failure messages. It passes
    here and cannot compile on main.
  • Mutations applied: none for this change. It is the mechanical substitution
    Replace LibStringSet with LibMemoryKV #210 applied elsewhere, and its evidence is there: the folds are the same
    shape, and Replace LibStringSet with LibMemoryKV #210 probed two of them in src plus thirteen execution probes at
    test sites.
  • Oracle: Replace LibStringSet with LibMemoryKV #210, which is the definition of the conversion, and the NatSpec on
    the store's get, which states that a key set to zero and a key never set are
    indistinguishable by value, so membership reads the existence flag. Every
    member is stored against a zero value and every question is has.
  • Category check: the ask is a red main. Covered: forge build clean,
    DeployTest green at 1 passed, LibRainDeployTest green at 77 passed, and no
    reference to the deleted library remains anywhere in src or test.

🤖 Generated with Claude Code

https://claude.ai/code/session_01V8ViHcKLVk2YoS2joH4HdN

Summary by CodeRabbit

  • Tests
    • Updated deployment verification tests to use memory-backed key-value sets when checking configured network aliases and deployment targets.
    • Added bidirectional containment assertions to confirm aliases and targets are correctly represented.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01V8ViHcKLVk2YoS2joH4HdN
@coderabbitai

coderabbitai Bot commented Sep 15, 2026 •

Copy link
Copy Markdown

Review Change StackReview Change Stack

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: c9f12151-1184-451d-ba8e-ec403d5e88cc

📥 Commits

Reviewing files that changed from the base of the PR and between 4249926 and d83990f.

📒 Files selected for processing (1)
  • test/script/Deploy.t.sol

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


Walkthrough

The deploy test replaces LibStringSet membership checks with MemoryKV maps keyed by hashed names. It verifies that configured RPC aliases and deploy targets contain each other.

Changes

Deploy test membership migration

Layer / File(s) Summary
Replace deploy membership checks
test/script/Deploy.t.sol
The test imports LibMemoryKV, enables its methods for MemoryKV, builds alias and target sets from hashed names, and checks bidirectional membership with has.

Priority: ⬇️ Low

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

Change: Bug fix

Merge Risk: ⚪ Minimal · up to d8399

This change restores the deploy test’s dependency on the maintained membership library without introducing a known production behavior change.

🚥 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 identifies the main change: fixing the deploy script test to remove its import of the deleted string library.
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
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch 2026-09-15-fix-main-string-set

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.

@thedavidmeister
thedavidmeister merged commit 13d8f35 into main Sep 15, 2026
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants