fix: unread is a state, incomplete is a verdict, unset is not zero - #23
Merged
Conversation
Three mechanisms turned a read that never happened into a clean report, and each gets closed at its root rather than per symptom. Probes report whether they read. The six tolerant probes in RolesProbes return a zero value beside an ok flag, and most call sites dropped the flag, so a contract the audit could not read became the assertion "the chain holds 0x0, false, an empty set". A snapshot then wrote those zeros into the declaration and the next audit compared them against the same failed reads: the two agreed because neither observed anything, and `sets match` was printed over a contract that answers no getter. Now the snapshot writes an unread field as ABSENT, never as the probe's zero (each omission is logged as UNREAD), the audit refuses to compare a declared value against a failed read (a FAIL naming the getter, not a PASS), and a declared hooks or lockbox contract that answers none of its getters fails once as unauditable instead of cascading. The write scripts that gated authority on a probed owner or admin refuse a failed read before the equality check, whose message would otherwise attribute an owner of 0x0 to a token whose owner was never observed. Old project files may carry zeros written by failed reads; against a still-unreadable contract they now fail instead of reconciling, and a re-run of snapshot-chain on a working RPC rebuilds the block from actual reads (docs/config-schema.md documents the absence-is-unread rule). The doctor returns three outcomes, not a boolean. A run that skipped the RPC rung, the TAR reconcile, the lanes rung and the roles rung printed 0 FAIL, 0 WARN and exited 0, indistinguishable from a run that verified everything. check-chain now counts unverified gaps (a check that was declared or deployed but could not run: an unset RPC, a pool that does not answer), tags each [SKIP] UNVERIFIED so the verdict points at exactly those lines, and ends VERIFIED, INCOMPLETE, or FAILED, with INCOMPLETE reverting under its own marker so a wrapper gating on the exit code cannot read an unchecked chain as a verified one. Designed absences (a non-EVM chain's EVM rungs, an undeclared optional block) stay plain SKIPs and never taint the verdict, so a fully verified run still exits 0 and prints VERIFIED. The roles-check script contract (0/1/2) is unchanged: after the probe fix, every declared-but-unverifiable field is a FAIL there, so its fails-only gate is sound; docs/roles.md documents that exit 1 also covers refused reads and adds the nothing-was-compared branch to the drift runbook. snapshot-chain counts its UNREAD fields and warns that the written block is partial rather than closing success-shaped. Unset variables do not become on-chain values. The rate-limit readers defaulted any unsupplied field of a triggered direction to 0 and wrote it: capacity alone enabled a bucket with rate 0 (the lane works for N tokens, then is permanently dead with TokenRateLimitReached), ENABLED=true alone wrote capacity 0 (every transfer reverts TokenMaxCapacityExceeded), and both printed a success message. A direction's inputs are now all-or-nothing through one shared decision, used by all three consumers (UpdateRateLimiters, SetFinalityConfig, and ApplyChainUpdates' CLI mode): an enabled bucket needs CAPACITY and RATE supplied together, refusal names the missing variable, ENABLED=false alone stays valid (a disabled bucket's zeros are forced by protocol rule, and a nonzero alongside is refused here rather than left to revert on chain), and a value that does not fit uint128 is refused rather than truncated: 1e39 wraps to a plausible wrong capacity and 2^128 to exactly 0. SetDynamicConfig now preserves an unset field's current on-chain value verbatim, address(0) included: it writes the whole struct, and a broadcaster fallback would turn a ROUTER-only run into a silent grant of both admin slots to the acting account. The primitives catalog had its own instance of the defect: authored inputs (for env vars read through test seams, invisible to the generator's vm.env* scan) reached only the markdown pages, while catalog.json kept advertising "inputs": [] for scripts that do read env vars. The generator now applies authored inputs at the source, the same at-the-source rule it already applies to the destructive flag, and the seam-read rate-limit and dynamic-config variables are authored in _meta.json so both the pages and the machine index name them. The decision logic is pure and pinned without touching the process env (vm.setEnv is process-wide while tests run in parallel, and the apply scripts read the same variables): test/roles/UnreadableIsNotEmpty.t.sol reproduces the silent reconcile and its snapshot half, test/config/VerifyChainVerdict.t.sol pins the three outcomes and that a designed skip never taints them, test/configure/RateLimitBucketInputs.t.sol pins the bucket shapes, and test-tooling.sh asserts the rpcEnv-unset doctor run is INCOMPLETE and nonzero.
|
👋 aelmanaa, thanks for creating this pull request! To help reviewers, please consider creating future PRs as drafts first. This allows you to self-review and make any final changes before notifying the team. Once you're ready, you can mark it as "Ready for review" to request feedback. Thanks! |
SyedAsadKazmi
approved these changes
Aug 11, 2026
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
2 participants
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.
This pull request updates documentation and configuration to clarify and enforce the rules for rate limiter inputs, doctor/roles-check verdicts, and dynamic config behaviors. The changes improve accuracy, make error handling more explicit, and ensure that configuration tools and documentation reflect the actual contract and script behavior, especially around partial inputs and environment variable handling.
Key changes include:
Rate Limiter Input Rules and Documentation
Clarified that rate limiter inputs (
*_CAPACITY,*_RATE,*_ENABLED) are all-or-nothing per direction: both capacity and rate must be supplied together to enable a bucket, and partial inputs are refused with a clear error. This rule is now documented in multiple locations, includingdocs/operations/rate-limits.md,docs/primitives/_meta.json, and referenced in operation guides. [1] [2] [3] [4] [5] [6]Updated the primitives catalog and meta documentation to include detailed input descriptions for all relevant operations (
UpdateRateLimiters,SetFinalityConfig, etc.), specifying the all-or-nothing rule and the behavior for unset or disabled buckets. [1] [2] [3]Doctor and Roles-Check Verdicts
Changed documentation and CI output to distinguish between VERIFIED, INCOMPLETE, and FAILED outcomes for
make doctorandroles-check, clarifying that INCOMPLETE (e.g., due to missing RPC env) is not a pass and must be addressed before launch. Output and documentation now explain how refused reads and infrastructure gaps are handled. [1] [2] [3] [4] [5]Improved explanation of how unreadable fields are handled in snapshots and audits, making it clear that missing reads are not written as zero/empty values and cause a FAIL rather than a PASS.
Dynamic Config and Environment Variable Handling
SetDynamicConfig, etc.) to clarify that unset environment variables preserve the current on-chain value verbatim, includingaddress(0), and that no fallback (such as broadcaster) is applied unless explicitly set. [1] [2]Additional Documentation Improvements
These updates ensure that users and developers have clear, accurate guidance on configuration, error handling, and expected behaviors in both tooling and on-chain operations.