Hold a deploy suite key to an alphabet the key list reads back as - #219
Conversation
`UnknownDeploymentSuite` carries the declared keys so a caller who does not already know them is told what they are. `suiteNames()` joins them on ", " and nothing constrained key contents, so the two suite registry keyed `a,b` and `c` rendered `a,b, c` — the same list a three suite registry keyed `a`, `b` and `c` reports. The reader was told a different number of suites exist than do and was sent after `b`, which is declared nowhere. `allSuites()` now refuses a key carrying any character of the separator, in the same pass that refuses a duplicate, so every reader refuses it at once and no lookup can answer with a list that reads back as a set the registry does not hold. The join reads the separator from the constant the check does. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01V8ViHcKLVk2YoS2joH4HdN
|
Warning Review limit reachedNext included review available in 47 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (12)
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 |
# Conflicts: # README.md # src/abstract/RainDeploySuitesBase.sol # test/src/abstract/RainDeploySuitesBase.t.sol
`suiteNames()` joins the declared keys on `", "` and nothing constrained what a key may contain, so `UnknownDeploymentSuite` rendered a two suite registry keyed `a,b` and `c` as `a,b, c` — indistinguishable from a three suite registry keyed `a`, `b` and `c`, and `b` is declared nowhere. The error exists so a caller who does not already know the valid keys is told them; a list that cannot be read back as the set it names is the hardcoded string the registry replaces. Keys are now held to a name of lowercase letters and hyphens, optionally an at sign and a tag of those plus digits and underscores, neither half empty and at most one at sign. That is what a repo declares by hand and what `LibRainDeploySnapshot` generates, so no declaration needs an exemption. The empty key falls to the same rule rather than to a second one beside it: `EmptyDeploySuiteKey(uint256 index)` from #202 becomes `InvalidDeploySuiteKey(uint256 index, string suite)`, keeping the index and gaining the key every refusal that is not the empty one needs. Closes #203. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01V8ViHcKLVk2YoS2joH4HdN
thedavidmeister
changed the title
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01V8ViHcKLVk2YoS2joH4HdN
c3bef6b cut the whole of `InvalidDeploySuiteKey`'s NatSpec down to a one sentence summary of the alphabet. What went with it, off a published abstract that deriving repos read: that a key carries at most one at sign and digits and underscores only after it; that the released keys `LibRainDeploySnapshot` generates satisfy the rule without an exemption; WHY an alphabet exists at all, which is that `suiteNames()` joins the keys on `", "` and a list that reads back as a different set is the hardcoded string this registry replaces; that the empty key is refused by the same rule because it is what `RainDeployBroadcast.run()` substitutes for an absent `DEPLOYMENT_SUITE`, where `CREATE2` under a zero salt makes the mistake permanent; and why the rule is held on the declaration rather than beside the substitution. The tests lost the same arguments where they are pinned: why there is no length floor, why the index is asserted with every refusal, and what the `a,b` fixture demonstrates — that `externalSuiteByName("b")` asks after a key the rendering invents, which is not a typo. Left cut: the `@param` lines on the test wrapper, the inline comment that spells the two array indices below it back, and the `SeparatorKeyDeploySuites` title block, whose argument now stands once on the test that makes it rather than twice. `checkSuiteKey` keeps c3bef6b's shorter form of the empty-half comment. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01V8ViHcKLVk2YoS2joH4HdN
|
1 participant
Closes #203.
The rule
A deploy suite key is a NAME, or a name and a release TAG joined by an at sign:
the name lowercase letters and hyphens, the tag those plus digits and
underscores, neither half empty and at most one at sign.
Digits and underscores are legal ONLY after the at sign. That is what the keys
this repo generates need and all they need:
LibRainDeploySnapshotemits areleased entry as the candidate key it was cut from, the at sign, and the record
directory's tag, which
isTagalready holds toX_Y_Z— soaddress-registry@0_1_10passes as it stands and the hand writtenaddress-registryit was cut from passes as it stands.The rule is held against the WHOLE key rather than against a candidate key with
the released form derived from it, because
releasedSuites()declares finishedkeys:
allSuites()is the one place every key a repo declares is read, and arule that only knew about the candidate half could not be asked about a released
entry at all.
The test fixtures spelled their released key
address-registry-0-0-1, a shapeLibRainDeploySnapshothas never emitted and the rule now refuses, so they moveto
address-registry@0_0_1— the generated shape. That is most of the diff'sline count and none of its behaviour.
What was wrong
UnknownDeploymentSuitecarries the declared keys "because the whole point of aregistry is that the answer is not one hardcoded string the caller has to
already know".
suiteNames()builds that list by joining the registry on", "and nothing constrained what a key may contain, so the rendering was not
injective: the TWO suite registry keyed
a,bandcrendersa,b, c, which isexactly what the THREE suite registry keyed
a,bandcreports. A reader istold a different number of suites exist than do, and
b— declared nowhere — ishanded to them as a valid key.
Code or docs
The code. The NatSpec is the intent oracle and it is right: a list a caller
cannot read back as the set of keys it names IS the hardcoded string the registry
exists to replace, just spelled differently.
The refusal lives in
allSuites(), in the pairwise pass that already refuses aduplicate: both the deploy side and the verify side read the registry through
it, so every reader refuses at once and
suiteByNamecan never answer a failedlookup with an ambiguous list.
Reconciled with the empty-key rule, not stacked on it
#231 (issue #202) landed
EmptyDeploySuiteKey(uint256 index)in this same passwhile this PR was open. An alphabet that admits no zero-byte key already refuses
the empty key, so this does not add a second rule beside it: the two are one
InvalidDeploySuiteKey(uint256 index, string suite), and the empty-keyreasoning moves into its NatSpec unchanged — the empty string is the value
RainDeployBroadcast.run()substitutes for an absentDEPLOYMENT_SUITE, so itis the one refusal here with a broadcast behind it.
The error keeps #231's index, for #231's reason: an empty key names nothing, so
the position in
allSuites()is the only thing that can. It gains the keyitself, which is what a reader needs for every refusal that is not the empty
one.
testEmptySuiteKeyRevertsis kept, asserting the same index at the samereader set, under the error that now refuses it.
Migration
Every key a consumer declares is now held to the alphabet, on every reader —
the deploy script, the verification tests and the chain matrix alike — with
InvalidDeploySuiteKey(<index>, <key>)at declaration read time, which cannotreach a broadcast.
Measured rather than assumed: the 40 distinct keys declared across every repo in
the org that declares one —
rain.extrospection.deploy,rain.factory.deploy,rain.math.float.deploy,rain.metadata.deploy,rain.tofu.erc20-decimals.deploy,rainlang.deployandraindex— were extracted from a fresh clone of each andheld against the rule. Zero are rejected. So are this repo's own: the six
<name>@<tag>released keys undersrc/lib/Lib*Released.soland the twocandidate keys in
RegistryDeploySuites.Suite
nix develop -c forge test -j 2on the merged branch: 535 passed, 2 failed(537 total). Both failures are
testFindDeployBlockZoltuFactoryandtestIsStartBlockAtDeployBlockintest/src/lib/LibRainDeploy.t.sol, and bothare the environmental Base archive read —
HTTP 403 ... Archive requests require a personal tokenfrom the allowed provider, before any assertion. They fail thesame way on unmodified
main.QA
testSuiteKeyAlphabetRefuses,testSeparatorKeyIsRefused,testEmptySuiteKeyReverts- each fails on base, verified two ways. (1) The baseVALUES were pinned against unmodified
main(feefc95) in a separate freshclone: with the
a,b/cfixture,externalAllSuites().length == 2,externalSuiteNames() == "a,b, c"andexternalSuiteByName("b")revertingUnknownDeploymentSuite("b", "a,b, c")— a phantom key answered as valid — allgreen there; throwaway test deleted afterwards. (2) Base behaviour was then
reproduced in place as mutant M12 below, which makes
checkSuiteKeyacceptevery key: all three tests fail under it.
testSuiteKeyAlphabetAcceptsisdeliberately NOT in this list — it is the half that pins which keys the rule
must not refuse, so it passes on base by construction and is here to stop the
rule being narrowed into refusing the repo's own generated release keys.
src/abstract/RainDeploySuitesBase.sol, allKILLED (
mutation-probe, baseline green at 16 passed).if (i == 0 || tagStart != 0)-> drop thei == 0half (empty name admitted)->
testSuiteKeyAlphabetRefuses; same line -> drop thetagStart != 0half(two at signs admitted) ->
testSuiteKeyAlphabetRefuses;nameAlphabet-> hyphen dropped ->testEverySuiteIsSelectableByKey,testEmptySuiteKeyReverts,testSameLengthKeysSelectApartand two more;!(nameAlphabet || (tagStart != 0 && tagAlphabet))->!(nameAlphabet || tagAlphabet)(tag alphabet read over the whole key) ->
testSuiteKeyAlphabetRefuses;if (tagStart == key.length)->if (false)(empty-half guard removed) ->testEmptySuiteKeyReverts,testSuiteKeyAlphabetRefuses; same line ->if (key.length == 0)(narrowed to An unset DEPLOYMENT_SUITE can select and broadcast a suite, because the empty string is a declarable suite key #202's check, trailing at sign admitted) ->testSuiteKeyAlphabetRefuses;revert InvalidDeploySuiteKey(index, suite)->(0, suite)on the alphabet path ->testSuiteKeyAlphabetRefuses, and on theempty-half path ->
testEmptySuiteKeyReverts,testSuiteKeyAlphabetRefuses;if (char == "@")->if (false)(at sign not recognised as the separator) ->testSuiteKeyAlphabetAccepts,testEverySuiteIsSelectableByKeyand three more;checkSuiteKey(i, suites[i].suite)inallSuites()-> deleted ->testSeparatorKeyIsRefused,testEmptySuiteKeyReverts; same line -> guarded toi < released.length(released half only) ->testEmptySuiteKeyReverts; M12,checkSuiteKeybody ->return;(base behaviour, every key accepted) ->testSeparatorKeyIsRefused,testEmptySuiteKeyReverts,testSuiteKeyAlphabetRefuses.UnknownDeploymentSuite's own NatSpec — the list exists so a caller whodoes NOT already know the valid keys is told them — which fixes the expected
behaviour as "the rendering must be injective", independent of any
implementation. The accepting half of the table is oracled by what the
declarations in this repo and its consumers actually spell: the emission shape
in
LibRainDeploySnapshot(<candidate key>+ at sign +tagForRecordPath,already held to
X_Y_ZbyisTag) and the 40 consumer keys in Migration above,neither derived from the check under test.
reader being told a different number of suites than exist, and (C) a phantom key
handed back as valid. All three are one observable of one declaration and are
covered together by
testSeparatorKeyIsRefused, which asserts the refusal atexternalAllSuites,externalSuiteNames,externalSuiteByName("c")(the keythat IS declared) and
externalSuiteByName("b")(the phantom the renderinginvented) — the base values for all of which were pinned on
mainas above. Theissue's own triage question, code or docs, is answered under "Code or docs".
🤖 Generated with Claude Code
https://claude.ai/code/session_01V8ViHcKLVk2YoS2joH4HdN