Report dead simulate inputs instead of silently dropping them - #1362
Merged
Conversation
The TestIndexer simulate path ran a validateSrcAddresses guard that threw for any non-wildcard simulate item whose srcAddress wasn't in the chain's static indexingAddresses snapshot. This broke unit tests for contracts registered dynamically at runtime (context.chain.<Contract>.add(...)): no srcAddress satisfies the guard because dynamic registrations aren't in the snapshot, and the check was chain-wide rather than per-contract. Drop the guard so the simulate path accepts whatever srcAddress the test provides (including the default placeholder) and routes the event to the handler unchanged, matching pre-3.3.0 behavior: - remove SimulateItems.validateSrcAddresses and its call in TestIndexer's patchConfig callback - retarget the now-stale tests (SimulateDynamicAddress, WildcardSimulate, EventHandler) to assert the event reaches its handler instead of being rejected Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HkD5k2XZ4EcZjZGCVNm1vr
The removed pre-flight srcAddress guard rejected contracts registered dynamically at runtime because it checked a static snapshot taken before the run. Replace it with an accurate dead-input check: a simulate run records the non-wildcard items its address filter drops — evaluated against the post-registration indexingAddresses, the same set handler routing uses — so a simulate input whose handler never runs (wrong/placeholder srcAddress, missing registration) is reported instead of passing silently. ChainState owns the per-chain skip list and mirrors the filter applied in FetchState.handleQueryResult; ExitOnCaughtUp surfaces any skipped items through the fatal-error channel before exiting clean. Because the check sees the same addresses the runtime does, a contract registered in the same process() call (or an earlier one) is not flagged. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HkD5k2XZ4EcZjZGCVNm1vr
Contributor
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 20 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughSimulate inputs are now carried through source and indexer state into a dead-input tracker, which records processed batch items and reports unhandled simulate entries at catch-up exit. Simulate parsing no longer performs upfront src-address validation and now rejects duplicate ChangesSimulate dead-input tracking and exit reporting
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scenarios/test_codegen/test/SimulateDynamicAddress_test.res (1)
57-70: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAssert routing here, not just “no error”.
This test passes even if the event is still dropped silently, because it never checks the handler’s observable output. Given the title/comment, assert the routed result (for example
Token.getAll() == [expectedToken]) and letprocess()throwing fail the test naturally.Suggested tightening
-Async.it("accepts a non-wildcard event for a contract registered in the same process() call", async t => { +Async.it("routes a non-wildcard event for a contract registered in the same process() call", async t => { let indexer = Indexer.createTestIndexer() - - let error = try { - let _ = await indexer.process({ - chains: {"1337": {startBlock: 1, endBlock: 100, simulate: [createNft, transferNft]}}, - }) - None - } catch { - | JsExn(err) => err->JsExn.message - } - - t.expect(error).toEqual(None) + let _ = await indexer.process({ + chains: {"1337": {startBlock: 1, endBlock: 100, simulate: [createNft, transferNft]}}, + }) + let tokens = await indexer."Token".getAll() + t.expect(tokens).toEqual([expectedToken]) })🤖 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 `@scenarios/test_codegen/test/SimulateDynamicAddress_test.res` around lines 57 - 70, The test only verifies that process() does not throw, so it can still pass even if the event is silently dropped. Update the Async.it case in SimulateDynamicAddress_test.res to assert the routed observable output from the handler, using the same process() call and symbols like Indexer.createTestIndexer, createNft, transferNft, and Token.getAll() with the expected token result. Remove the manual try/catch-based error check and let process() failures fail naturally, while asserting the handler state after processing.
🧹 Nitpick comments (1)
scenarios/test_codegen/test/SimulateDynamicAddress_test.res (1)
54-56: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRewrite this comment to describe behavior, not the refactor.
“no longer pre-checks
srcAddressagainst a static snapshot” is implementation-history, so it will age poorly in a test file. Reword it around the invariant this test cares about: registrations earlier in the sameprocess()batch make the later non-wildcard event routable. As per coding guidelines, "**/*.res: Never narrate the refactor itself ('previously lived in X', 'centralized here', 'now imports from Y'). That belongs in the commit message, not the code`."🤖 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 `@scenarios/test_codegen/test/SimulateDynamicAddress_test.res` around lines 54 - 56, The test comment should describe the observable behavior instead of the implementation change. Rewrite the note near the dynamic address simulation so it states that a contract registered earlier in the same process() batch can route a later non-wildcard event to its handler, and remove any wording about pre-checks, static snapshots, or refactor history.Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@scenarios/test_codegen/test/SimulateDynamicAddress_test.res`:
- Around line 57-70: The test only verifies that process() does not throw, so it
can still pass even if the event is silently dropped. Update the Async.it case
in SimulateDynamicAddress_test.res to assert the routed observable output from
the handler, using the same process() call and symbols like
Indexer.createTestIndexer, createNft, transferNft, and Token.getAll() with the
expected token result. Remove the manual try/catch-based error check and let
process() failures fail naturally, while asserting the handler state after
processing.
---
Nitpick comments:
In `@scenarios/test_codegen/test/SimulateDynamicAddress_test.res`:
- Around line 54-56: The test comment should describe the observable behavior
instead of the implementation change. Rewrite the note near the dynamic address
simulation so it states that a contract registered earlier in the same process()
batch can route a later non-wildcard event to its handler, and remove any
wording about pre-checks, static snapshots, or refactor history.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e3fba299-b660-4b3f-962b-37578dee6126
📒 Files selected for processing (9)
packages/envio/src/ChainState.respackages/envio/src/ChainState.resipackages/envio/src/ExitOnCaughtUp.respackages/envio/src/SimulateItems.respackages/envio/src/TestIndexer.respackages/envio/src/sources/SimulateSource.resscenarios/test_codegen/test/EventHandler.test.tsscenarios/test_codegen/test/SimulateDynamicAddress_test.resscenarios/test_codegen/test/WildcardSimulate_test.res
💤 Files with no reviewable changes (2)
- scenarios/test_codegen/test/EventHandler.test.ts
- packages/envio/src/SimulateItems.res
Replace the address-filter mirror with an observational check. ChainState seeds a per-chain list of the non-wildcard items a simulate run was given (now exposed on the SimulateSource) and drops each as it appears in a processed batch; whatever remains on caught-up never reached a handler and is reported as a dead simulate input. This removes the duplicated filter application in handleQueryResult — items are matched by reference, since a batch holds the same objects the source provided — and flags an unrouted item whatever the reason, not only a srcAddress that isn't indexed. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HkD5k2XZ4EcZjZGCVNm1vr
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@packages/envio/src/ChainState.res`:
- Around line 226-245: The simulate dead-input tracker is being rebuilt from
chainConfig.sourceConfig in makeInternal, which causes already-consumed simulate
items to reappear after a restart. Update ChainState.makeInternal and the
makeFromDbState resume path so unprocessedSimulateItems is restored from
persisted state (or not re-seeded on resume if simulate runs should not survive
restarts), and ensure the same logic is applied where the tracker is initialized
around the referenced simulate-item handling.
🪄 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: CHILL
Plan: Pro
Run ID: 0730cd9a-ff82-4367-996e-be9e3caab6f3
📒 Files selected for processing (4)
packages/envio/src/ChainState.respackages/envio/src/ChainState.resipackages/envio/src/sources/SimulateSource.respackages/envio/src/sources/Source.res
Track all items a simulate run is given, not only the non-wildcard ones, so a wildcard item excluded by its where/block filter is flagged too. The completion message no longer assumes the cause is an unindexed srcAddress, since an item can now be reported for any reason it never reached a handler. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HkD5k2XZ4EcZjZGCVNm1vr
Replace the per-chain simulate state on ChainState with a SimulateDeadInputTracker owned by IndexerState. ProcessEventBatch feeds it each committed batch and ExitOnCaughtUp reads the remainder, so ChainState and advanceAfterBatch go back to being simulate-agnostic — the test concern no longer rides on the core indexing state machine. The tracker is built from the config (None off the simulate path), and matches items by reference just as before, so behavior is unchanged. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HkD5k2XZ4EcZjZGCVNm1vr
Replace the per-item dump in the dead-input error with each item's index in its chain's simulate array, grouped by chain — so the message stays short and the user finds the input without parsing echoed fields. The tracker now carries each item's chain id and index; ExitOnCaughtUp groups the remainder and singular/plural agrees with the count. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HkD5k2XZ4EcZjZGCVNm1vr
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@packages/envio/src/SimulateDeadInputTracker.res`:
- Around line 14-20: The simulate-item collection in
SimulateDeadInputTracker.res only uses Array.find on Config.CustomSources, so it
stops at the first source with simulateItems and misses later custom sources.
Update the logic in the chainConfig/sourceConfig handling to iterate all
sources, collect every source’s simulateItems, and flatMap them into the result
instead of returning only the first match.
🪄 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: CHILL
Plan: Pro
Run ID: 769fa92f-8de6-4469-be2d-b831b18abce3
📒 Files selected for processing (6)
packages/envio/src/BatchProcessing.respackages/envio/src/ExitOnCaughtUp.respackages/envio/src/IndexerState.respackages/envio/src/IndexerState.resipackages/envio/src/SimulateDeadInputTracker.resscenarios/test_codegen/test/SimulateDynamicAddress_test.res
✅ Files skipped from review due to trivial changes (1)
- packages/envio/src/IndexerState.resi
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/envio/src/ExitOnCaughtUp.res
- scenarios/test_codegen/test/SimulateDynamicAddress_test.res
- Match provided simulate items to processed ones by (chain, block, logIndex) coordinate instead of object identity, so it survives any copy/transform of an item between source and batch rather than silently reporting all as unrouted. - recordProcessed builds a Utils.Set of processed keys once per batch (O(batch + unprocessed)) instead of an O(unprocessed x batch) reference scan. - unroutedByChain groups via Utils.Dict.push instead of a hand-rolled branch. - Move the failure-message formatting onto the tracker (failureMessage); the ExitOnCaughtUp exit path is now simulate-agnostic. - makeFromConfig collects from every simulate source on a chain, not just the first. - Trim comments that restated code. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HkD5k2XZ4EcZjZGCVNm1vr
Event ordering and the dead-input tracker both key items on (chain, block, logIndex). Two simulate items resolving to the same coordinate is an ambiguous input, so catch it at parse time and name both offending indices instead of silently letting one shadow the other. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HkD5k2XZ4EcZjZGCVNm1vr
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@packages/envio/src/SimulateDeadInputTracker.res`:
- Around line 50-53: The `recordProcessed` logic in `SimulateDeadInputTracker`
is treating the empty-string sentinel from `itemKey` as a real key, so non-event
batch items can accidentally match unrelated empty-key entries. Update
`recordProcessed` to ignore empty keys when building `processedKeys` from
`batch.items`, keeping only actual event keys before filtering `t.unprocessed`.
Use the existing `itemKey` helper and the `recordProcessed` function to locate
the change.
🪄 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: CHILL
Plan: Pro
Run ID: 1a95309d-c6b0-4fc7-bfde-403a5fe407f9
📒 Files selected for processing (6)
packages/envio/src/ExitOnCaughtUp.respackages/envio/src/IndexerState.respackages/envio/src/SimulateDeadInputTracker.respackages/envio/src/SimulateDeadInputTracker.resipackages/envio/src/SimulateItems.resscenarios/test_codegen/test/SimulateDynamicAddress_test.res
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/envio/src/IndexerState.res
- scenarios/test_codegen/test/SimulateDynamicAddress_test.res
Comment on lines
+50
to
+53
| let recordProcessed = (t: t, ~batch: Batch.t) => { | ||
| let processedKeys = batch.items->Array.map(itemKey)->Utils.Set.fromArray | ||
| t.unprocessed = t.unprocessed->Array.filter(entry => !(processedKeys->Utils.Set.has(entry.key))) | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Empty-key sentinel can collapse non-event items together.
itemKey returns "" for every non-event item, and recordProcessed maps all batch.items (which routinely include non-event items) into processedKeys. Today this is harmless because real simulate entries are always events (never keyed ""). But it makes the matching fragile: if the "non-event items are never a provided simulate input" invariant ever breaks, a single non-event batch item would mark any empty-key entry as processed. Cheap to harden by excluding empty keys.
🛡️ Proposed hardening
let recordProcessed = (t: t, ~batch: Batch.t) => {
- let processedKeys = batch.items->Array.map(itemKey)->Utils.Set.fromArray
+ let processedKeys =
+ batch.items->Array.filterMap(item => {
+ let key = itemKey(item)
+ key === "" ? None : Some(key)
+ })->Utils.Set.fromArray
t.unprocessed = t.unprocessed->Array.filter(entry => !(processedKeys->Utils.Set.has(entry.key)))
}📝 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.
Suggested change
| let recordProcessed = (t: t, ~batch: Batch.t) => { | |
| let processedKeys = batch.items->Array.map(itemKey)->Utils.Set.fromArray | |
| t.unprocessed = t.unprocessed->Array.filter(entry => !(processedKeys->Utils.Set.has(entry.key))) | |
| } | |
| let recordProcessed = (t: t, ~batch: Batch.t) => { | |
| let processedKeys = | |
| batch.items->Array.filterMap(item => { | |
| let key = itemKey(item) | |
| key === "" ? None : Some(key) | |
| })->Utils.Set.fromArray | |
| t.unprocessed = t.unprocessed->Array.filter(entry => !(processedKeys->Utils.Set.has(entry.key))) | |
| } |
🤖 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 `@packages/envio/src/SimulateDeadInputTracker.res` around lines 50 - 53, The
`recordProcessed` logic in `SimulateDeadInputTracker` is treating the
empty-string sentinel from `itemKey` as a real key, so non-event batch items can
accidentally match unrelated empty-key entries. Update `recordProcessed` to
ignore empty keys when building `processedKeys` from `batch.items`, keeping only
actual event keys before filtering `t.unprocessed`. Use the existing `itemKey`
helper and the `recordProcessed` function to locate the change.
An explicit logIndex left the auto-increment counter untouched, so a
following item that omitted logIndex reused the same index and tripped
the (block, logIndex) collision check on otherwise valid input like
[{logIndex: 0}, {}]. Advance the counter past explicit values so auto
items pick up after them, and regression-test the mixed explicit/auto
case (it throws the spurious collision without the fix).
Also trim the SimulateDeadInputTracker module comment to keep only the
non-obvious design note.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HkD5k2XZ4EcZjZGCVNm1vr
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.
Summary
Changes the simulate path to report non-wildcard events whose srcAddress isn't indexed, rather than silently dropping them via the address filter. This surfaces dead test code (simulate inputs that never reach a handler) as loud failures instead of passing silently.
Key Changes
Removed pre-flight validation: Deleted
SimulateItems.validateSrcAddresses, which ran before the batch and couldn't see addresses registered within the sameprocess()call. This was a false gate that rejected valid simulate items.Moved validation to post-filter: Added tracking in
ChainStateto record non-wildcard simulate items that the address filter drops (after all dynamic registrations in the current run). When the indexer catches up,ExitOnCaughtUpchecks for skipped items and fails loudly with a detailed error message instead of exiting clean.Updated test expectations:
SimulateDynamicAddress_test.res: Changed test that expected rejection to now expect success (contract registered in same batch is now accepted). Added new test verifying that truly unindexed addresses still fail with the new error message.WildcardSimulate_test.res: Removed test for pre-flight validation of non-wildcard events; wildcard events now route to handlers regardless of srcAddress without any validation gate.EventHandler.test.ts: Removed test for pre-flight validation error.Implementation details:
ChainStatenow tracksisSimulateflag andskippedItemsarrayChainState.handleQueryResultmirrors the address filter logic to record dropped itemsExitOnCaughtUp.runchecks for skipped items and reports them with full context (contract, event, srcAddress, chain, block)SimulateSource.sourceNameexported for detection inChainState.makeInternalBehavior Changes
This makes simulate runs fail-fast on dead test code while accepting valid dynamic registration patterns.
https://claude.ai/code/session_01HkD5k2XZ4EcZjZGCVNm1vr
Summary by CodeRabbit
New Features
Bug Fixes
(blockNumber, logIndex)coordinate.Tests