fix(decider): close audited correctness and observability gaps - #495
Merged
Conversation
yordis
commented
Jul 22, 2026
yordis
commented
Member
- A publish path that panics on user-supplied header bytes takes down the whole appender instead of failing one command, so header validation had to become typed and run before any batch member reaches the server.
- Native-vs-WASM parity that never compares stream ids or a domain error's causal chain cannot catch the codegen and codec drift it exists to catch, and a scenario builder that silently drops misused steps hides broken tests.
- The guest session destructor is real guest code, so running it on leftover fuel could trap after a successful decision; a destructor trap must neither discard that outcome nor stay invisible to the traps metric.
- Unbounded replay lets a forgotten or misconfigured snapshot policy grow per-command latency silently; an opt-in replay limit turns that misconfiguration into a loud, typed failure.
- The zero-import rule is the production sandboxing boundary and had no test proving it rejects an importing component, and nothing enforced the non-empty decide contract at the wire boundary where untrusted guests live.
- The workspace's only unsafe block was avoidable, and the given_state escape hatch shipped untested by the crate that owns it.
Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
PR SummaryMedium Risk Overview Native runtime adds optional JetStream append validates user event header names/values up front and returns typed WASM path rejects guest Parity/sim compares resolved stream ids and full domain-error Testing removes unsafe from Reviewed by Cursor Bugbot for commit d118a61. Bugbot is set up for automated code reviews on this repo. Configure here. |
|
Warning Review limit reached
Next review available in: 14 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 (4)
WalkthroughThis PR adds replay caps, fallible NATS event-header validation, WASM decision and teardown enforcement, richer parity results, stricter scenario-builder checks, and safer typestate test harness transitions. ChangesRuntime replay limits
NATS header validation
WASM contracts
Parity simulation
Testing harness
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
This PR tightens correctness and observability guarantees across the decider stack (native runtime, WASM runtime, and simulation/parity tooling), turning previously silent or panicking failure modes into explicit typed errors and testable invariants.
Changes:
- Enforce stronger execution contracts: non-empty
decideresults at the WASM boundary, safe/observable session destructor handling, and an opt-in replay limit with a typed failure. - Harden NATS publishing by validating user-supplied header names/values up front (preventing panics and ensuring no partial batch publish).
- Strengthen simulation/parity tooling by comparing stream ids and preserving domain error causal-chain details; make scenario builders fail loudly on misuse; remove the workspace’s remaining
unsafein the decider test harness.
Reviewed changes
Copilot reviewed 30 out of 31 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| rsworkspace/crates/trogon-semconv/src/gen/attribute.rs | Adds GuestPhase::Drop for destructor-phase observability. |
| rsworkspace/crates/trogon-decider/tests/ui/fail/given_state_then_given.stderr | New trybuild stderr for invalid given_state(...).given(...) chaining. |
| rsworkspace/crates/trogon-decider/tests/ui/fail/given_state_then_given.rs | New trybuild compile-fail case for typestate API misuse. |
| rsworkspace/crates/trogon-decider/src/testing/tests.rs | Adds given_state(...) success-path tests for events and exact errors. |
| rsworkspace/crates/trogon-decider/src/testing.rs | Removes unsafe stage extraction by switching to Option<Stage> + take(). |
| rsworkspace/crates/trogon-decider/src/event/codec/event_payload_error.rs | Clarifies/expands MissingEvent semantics for codecs (encode + decode symmetry). |
| rsworkspace/crates/trogon-decider-wit/wit/world.wit | Documents non-empty decide success invariant at the interface boundary. |
| rsworkspace/crates/trogon-decider-wasm-runtime/src/module/tests.rs | Adds a structural test rejecting WASM components that declare imports. |
| rsworkspace/crates/trogon-decider-wasm-runtime/src/execution/tests.rs | Adds tests for destructor budget handling and empty-decision rejection. |
| rsworkspace/crates/trogon-decider-wasm-runtime/src/execution.rs | Rejects Ok([]) via EmptyDecision; drops sessions with fresh budget and trap logging/metrics under Drop. |
| rsworkspace/crates/trogon-decider-wasm-runtime/Cargo.toml | Adds wat as a dev-dependency for import-declaration tests. |
| rsworkspace/crates/trogon-decider-sim/tests/schedules.rs | Tests run_wasm stream-id resolution behavior (present vs empty scenario). |
| rsworkspace/crates/trogon-decider-sim/tests/parity.rs | Adds parity tests covering declared stream id matching/mismatch. |
| rsworkspace/crates/trogon-decider-sim/src/scenario/tests.rs | Adds builder-misuse panic tests and a multi-step chaining test. |
| rsworkspace/crates/trogon-decider-sim/src/scenario.rs | Makes scenario builder misuse fail loudly (no silent step dropping). |
| rsworkspace/crates/trogon-decider-sim/src/parity/tests.rs | Expands parity unit tests for details comparison + stream-id comparison. |
| rsworkspace/crates/trogon-decider-sim/src/parity.rs | Compares native/wasm resolved stream ids and domain-error details before step outcomes. |
| rsworkspace/crates/trogon-decider-sim/src/native/tests.rs | Updates tests for new ScenarioRun shape and stream-id capture. |
| rsworkspace/crates/trogon-decider-sim/src/native.rs | Returns ScenarioRun (stream id + step outcomes) and normalizes domain error outcomes. |
| rsworkspace/crates/trogon-decider-sim/src/lib.rs | Re-exports new IR types (ScenarioRun, StreamIdOutcome, DomainErrorOutcome). |
| rsworkspace/crates/trogon-decider-sim/src/ir.rs | Introduces DomainErrorOutcome, StreamIdOutcome, ScenarioRun; updates stream-id contract docs. |
| rsworkspace/crates/trogon-decider-runtime/src/replay_limit.rs | Adds ReplayLimit value object + typed construction error. |
| rsworkspace/crates/trogon-decider-runtime/src/lib.rs | Wires in ReplayLimit exports and ReplayLimitExceeded in the public surface. |
| rsworkspace/crates/trogon-decider-runtime/src/execution/tests.rs | Adds replay-limit test coverage (exceeded/at-boundary/unlimited). |
| rsworkspace/crates/trogon-decider-runtime/src/execution.rs | Adds optional replay limit to CommandExecution and typed ReplayLimitExceeded failure. |
| rsworkspace/crates/trogon-decider-nats/src/stream_store/tests.rs | Adds regression tests ensuring invalid headers fail append/build without panicking or partially publishing. |
| rsworkspace/crates/trogon-decider-nats/src/stream_store.rs | Makes publish message building fallible; validates header name/value with async-nats parsers before publishing. |
| rsworkspace/crates/trogon-decider-guest-sdk/src/bridge.rs | Uses Events::try_map to preserve non-empty guarantee across WIT decide. |
| rsworkspace/Cargo.lock | Locks wat dependency addition. |
| otel/semconv/registry/decider.yaml | Adds drop guest phase to the semconv registry. |
| docs/architecture/decider.md | Documents replay limit, header validation, non-empty decide invariant, and destructor drop/trap behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
rsworkspace/crates/trogon-decider-runtime/src/execution.rs (1)
413-466: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winPreserve
ReplayLimitExceededin the error chain. The public typed error lacksstd::error::Error, preventingCommandErrorfrom exposing the underlying replay-limit failure as its source.
rsworkspace/crates/trogon-decider-runtime/src/execution.rs#L413-L466: implementstd::error::ErrorforReplayLimitExceededand mark theCommandErrorvariant field as#[source].rsworkspace/crates/trogon-decider-runtime/src/execution/tests.rs#L706-L713: update the source-chain expectation totrue.Proposed fix
- ReplayLimitExceeded(ReplayLimitExceeded), + ReplayLimitExceeded(#[source] ReplayLimitExceeded), impl std::fmt::Display for ReplayLimitExceeded { // ... } + +impl std::error::Error for ReplayLimitExceeded {}As per coding guidelines, “Every error type must implement
Displayandstd::error::Error,” while preserving typed error context.🤖 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 `@rsworkspace/crates/trogon-decider-runtime/src/execution.rs` around lines 413 - 466, Implement std::error::Error for ReplayLimitExceeded and mark its CommandError variant field as the error source so the typed replay-limit failure remains in the error chain. Update the source-chain expectation in rsworkspace/crates/trogon-decider-runtime/src/execution/tests.rs lines 706-713 from false to true; no direct change is needed there beyond that assertion.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.
Inline comments:
In `@rsworkspace/crates/trogon-decider-runtime/src/execution/tests.rs`:
- Around line 1855-1901: Make both “before folding” tests use a stream event and
decider configuration that would fail during evolution, then assert
ReplayLimitExceeded is returned instead. Update the no-snapshot test at
rsworkspace/crates/trogon-decider-runtime/src/execution/tests.rs:1855-1901 and
add the equivalent assertion to the snapshot replay test at
rsworkspace/crates/trogon-decider-runtime/src/execution/tests.rs:1904-1952,
ensuring replay-limit enforcement occurs before evolve.
---
Outside diff comments:
In `@rsworkspace/crates/trogon-decider-runtime/src/execution.rs`:
- Around line 413-466: Implement std::error::Error for ReplayLimitExceeded and
mark its CommandError variant field as the error source so the typed
replay-limit failure remains in the error chain. Update the source-chain
expectation in rsworkspace/crates/trogon-decider-runtime/src/execution/tests.rs
lines 706-713 from false to true; no direct change is needed there beyond that
assertion.
🪄 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: f17a866a-4339-4650-b34f-1c164f8c6311
⛔ Files ignored due to path filters (2)
rsworkspace/Cargo.lockis excluded by!**/*.lockrsworkspace/crates/trogon-semconv/src/gen/attribute.rsis excluded by!**/gen/**
📒 Files selected for processing (29)
docs/architecture/decider.mdotel/semconv/registry/decider.yamlrsworkspace/crates/trogon-decider-guest-sdk/src/bridge.rsrsworkspace/crates/trogon-decider-nats/src/stream_store.rsrsworkspace/crates/trogon-decider-nats/src/stream_store/tests.rsrsworkspace/crates/trogon-decider-runtime/src/execution.rsrsworkspace/crates/trogon-decider-runtime/src/execution/tests.rsrsworkspace/crates/trogon-decider-runtime/src/lib.rsrsworkspace/crates/trogon-decider-runtime/src/replay_limit.rsrsworkspace/crates/trogon-decider-sim/src/ir.rsrsworkspace/crates/trogon-decider-sim/src/lib.rsrsworkspace/crates/trogon-decider-sim/src/native.rsrsworkspace/crates/trogon-decider-sim/src/native/tests.rsrsworkspace/crates/trogon-decider-sim/src/parity.rsrsworkspace/crates/trogon-decider-sim/src/parity/tests.rsrsworkspace/crates/trogon-decider-sim/src/scenario.rsrsworkspace/crates/trogon-decider-sim/src/scenario/tests.rsrsworkspace/crates/trogon-decider-sim/tests/parity.rsrsworkspace/crates/trogon-decider-sim/tests/schedules.rsrsworkspace/crates/trogon-decider-wasm-runtime/Cargo.tomlrsworkspace/crates/trogon-decider-wasm-runtime/src/execution.rsrsworkspace/crates/trogon-decider-wasm-runtime/src/execution/tests.rsrsworkspace/crates/trogon-decider-wasm-runtime/src/module/tests.rsrsworkspace/crates/trogon-decider-wit/wit/world.witrsworkspace/crates/trogon-decider/src/event/codec/event_payload_error.rsrsworkspace/crates/trogon-decider/src/testing.rsrsworkspace/crates/trogon-decider/src/testing/tests.rsrsworkspace/crates/trogon-decider/tests/ui/fail/given_state_then_given.rsrsworkspace/crates/trogon-decider/tests/ui/fail/given_state_then_given.stderr
Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 37d883f. Configure here.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 31 out of 32 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (2)
rsworkspace/crates/trogon-decider-sim/src/parity.rs:121
compare_stream_idsreturnsOk(())whenever either runner providesNone, which means a regression where one runner stops resolving stream ids (or returnsNoneunexpectedly) would be silently ignored by the parity harness. It should treat(Some(_), None)/(None, Some(_))as a mismatch (likely via a dedicatedParityErrorvariant or by changing the existing mismatch variant to carryOption<StreamIdOutcome>).
let (native, wasm) = match (native, wasm) {
(Some(native), Some(wasm)) => (native, wasm),
_ => return Ok(()),
};
rsworkspace/crates/trogon-decider-sim/src/scenario/tests.rs:56
- The test name implies two steps are fully built, but the assertions check that only one step has been flushed into
scenario.stepsand the second is still pending inscenario.current. Renaming the test (or adjusting assertions to match the name) would avoid confusion when debugging scenario builder behavior.
fn when_then_when_then_builds_two_steps_without_panicking() {
let scenario = SimScenario::new()
.when(command("a"))
.then_accepted()
.when(command("b"))
.then_rejected();
assert_eq!(scenario.steps.len(), 1);
assert!(scenario.current.when.is_some());
assert!(scenario.current.expectation.is_some());
}
Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 31 out of 32 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
rsworkspace/crates/trogon-decider-sim/src/parity.rs:121
compare_stream_idsreturnsOk(())whenever either side isNone, which means aSome(...)vsNonedivergence is silently ignored. Even ifNoneis currently only expected for empty scenarios, treating(Some, None)/(None, Some)as “no stream id to compare” defeats the intent of stream-id parity checking and can mask regressions in how runners resolve (or fail to resolve) the first command’s stream id.
let (native, wasm) = match (native, wasm) {
(Some(native), Some(wasm)) => (native, wasm),
_ => return Ok(()),
};
|
Code Coverage SummaryDetailsDiff against mainResults for commit: d118a61 Minimum allowed coverage is ♻️ This comment has been updated with latest results |
…e honest Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 31 out of 32 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
rsworkspace/crates/trogon-decider-sim/src/scenario/tests.rs:53
- The test name says it builds two steps, but the assertions verify only one flushed step in
scenario.steps(with the second step still held inscenario.current). Renaming the test to match the behavior will avoid confusion when reading failures.
fn when_then_when_then_builds_two_steps_without_panicking() {
… outcomes Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 31 out of 32 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
rsworkspace/crates/trogon-decider-sim/src/scenario/tests.rs:53
- Test name says it "builds two steps", but the assertions check that only one step is flushed into
scenario.steps(with the second still pending inscenario.current). Renaming this test to match what it actually asserts would avoid confusion for future readers.
fn when_then_when_then_builds_two_steps_without_panicking() {
Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
…fails Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
yordis
added a commit
that referenced
this pull request
Jul 28, 2026
Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
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
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.