(MOT-4107) test: add stop and queued-message scenario coverage - #657
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Review limit reached
Next review available in: 39 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 Plus Run ID: 📒 Files selected for processing (25)
📝 WalkthroughWalkthroughThe integration harness adds runner-owned interventions for cancellation cascades and queued-message editing. It adds gated router dispatch, descendant-session evidence, multi-session trace collection, updated floor validation, and direct scenarios INT-011 and INT-012. Stop handling now reports accepted cancellation consistently. ChangesIntervention-driven integration coverage
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
skill-check — worker0 verified, 51 skipped (no docs/).
Four for four. Nicely done. |
ytallo
changed the title
2d7a70a to
db9bea5
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (10)
harness/tests/integration/src/scenario/phases/intervention.rs (4)
673-689: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the unreachable
q_clause inensure_client_entry_id.Line 678 evaluates
!row.entry_id.starts_with("e_") || row.entry_id.starts_with("q_"). If the id starts withe_, it cannot also start withq_, so the second clause never contributes. The first clause already rejects internalq_ids. Keep the prefix check alone, or state the intent in a comment.♻️ Proposed change
- if !row.entry_id.starts_with("e_") || row.entry_id.starts_with("q_") { + // Client-visible queue ids are `e_`-prefixed; internal ids use `q_`. + if !row.entry_id.starts_with("e_") {🤖 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 `@harness/tests/integration/src/scenario/phases/intervention.rs` around lines 673 - 689, Update ensure_client_entry_id to remove the unreachable row.entry_id.starts_with("q_") condition, leaving only the client-visible "e_" prefix validation while preserving the existing error behavior.
225-241: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse one
kindnaming convention, and inline thereleasedflag.Line 226 uses
"stop_cancel_cascade"with underscores. The queued-edit handler uses"queued-message-edit-unqueue"with hyphens (Line 527). Both strings land incontrol.jsonevidence. Pick one convention so verify functions and artifact consumers match a predictable value.Line 241 also assigns
control["released"]after construction, which forces themutbinding and duplicatesgate.released. Put the flag in thejson!literal instead.🤖 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 `@harness/tests/integration/src/scenario/phases/intervention.rs` around lines 225 - 241, Update the control evidence construction around the stop-cancel cascade handler to use the same hyphenated kind naming convention as the queued-edit handler, and add the top-level released flag directly to the json! literal. Remove the post-construction control["released"] assignment and make the binding immutable unless other code requires mutability.
297-334: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDrop the duplicated tuple field and the scenario-specific key literal.
Each
queue_specsentry repeats the same string as bothlabelandsuffix. Uselabelfor the key suffix and remove the third field.Line 305 also hardcodes
integration-012inside a generic intervention handler. Therun_idprefix keeps the keys unique, so this is only a naming concern, but a second fixture using this intervention would emit misleading keys. Consider deriving the prefix from the scenario id.🤖 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 `@harness/tests/integration/src/scenario/phases/intervention.rs` around lines 297 - 334, The queue_specs entries redundantly store label and suffix; remove the third tuple field, iterate over label and message, and use label when constructing idempotency_key. In the same intervention handler, replace the hardcoded “integration-012” segment with the applicable scenario identifier derived from the existing scenario context so keys remain accurately named for every fixture.
109-120: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDerive the expected child count from
expected_in_flight.Line 117 hardcodes
children.len() == 2while the gate wait usesexpected_in_flight(currently 3 = root + 2 children). The two numbers encode the same fixture fact in two places. If a scenario changesexpected_in_flight, this predicate never becomes true and the run fails with a status-readiness timeout instead of a clear premise error.♻️ Proposed change
+ let expected_children = expected_in_flight.saturating_sub(1); let root_before = wait_for_status(services.client(), &self.session_id, deadline, { let expected_root_turn = root_turn_id.clone(); move |status| { status.get("turn_id").and_then(Value::as_str) == Some(expected_root_turn.as_str()) && status.get("status").and_then(Value::as_str) == Some("running") && status .get("children") .and_then(Value::as_array) - .is_some_and(|children| children.len() == 2) + .is_some_and(|children| children.len() == expected_children) } })🤖 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 `@harness/tests/integration/src/scenario/phases/intervention.rs` around lines 109 - 120, The root status readiness predicate in the intervention phase hardcodes the child count instead of deriving it from expected_in_flight. Update the children length check within the wait_for_status closure to use expected_in_flight minus the root turn, preserving the existing expected_in_flight fixture contract and status conditions.harness/tests/integration/src/trace_evidence.rs (1)
84-95: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDrop the redundant sort, and consider asserting that every requested session matched a group.
The
BTreeSetcollection at Line 92 already yields sorted, deduplicated ids, sotrace_ids.sort()at Line 95 is a no-op.
engine::traces::group_byalso useslimit: 100. A tree run now depends on several session groups being present. If the group list is truncated, a child session is dropped silently and the run fails later as a stability timeout instead of a clear premise error. An explicit check that each requested session id appeared ingroupswould fail faster with a better message.🤖 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 `@harness/tests/integration/src/trace_evidence.rs` around lines 84 - 95, Remove the redundant trace_ids.sort() after collecting from BTreeSet, and update the group-matching logic to track requested session IDs that were found; assert that every ID in wanted matched a group, with a clear premise-failure message before proceeding to trace ID collection.harness/tests/integration/src/probe.rs (1)
247-265: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument the ordering constraint against
confirm_completion_binding.
bind_child_completion_observerstores its trigger underLIFECYCLE_TRIGGER_TYPE.confirm_completion_binding(Lines 270-304) unregisters everyLIFECYCLE_TRIGGER_TYPEbinding except the one it just registered. Any later call toconfirm_completion_bindingtherefore removes the parent-filtered child observer silently. The current arm order is safe, but a fault scenario that rebinds after an engine restart would lose child completions. Add a note here, or make the pruning skip parent-filtered bindings.🤖 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 `@harness/tests/integration/src/probe.rs` around lines 247 - 265, Document in bind_child_completion_observer that it must run after confirm_completion_binding, since a later confirm_completion_binding call removes other LIFECYCLE_TRIGGER_TYPE bindings. Preserve the current trigger behavior and clearly note that rebinding after an engine restart must not occur in an order that silently removes the parent-filtered child observer.harness/tests/integration/src/scenario/phases/completion.rs (1)
166-195: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMake the root-first assumption explicit.
skip(1)assumestree_sessions[0]is the root session.run_stop_cancel_cascadebuilds the vector that way, but the contract is implicit across two files. Filter onsession_id != self.session_id, or document the ordering requirement wheretree_sessionsis produced.The
if let Some(control) = active.control.as_object_mut()guard also dropsterminal_statusessilently ifcontrolis not a JSON object. Consider returning a runner error in that case, since every tree run must carry an object control artifact.🤖 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 `@harness/tests/integration/src/scenario/phases/completion.rs` around lines 166 - 195, Update the tree-status collection in the completion flow to exclude the root by comparing each session ID with self.session_id instead of relying on tree_sessions ordering. Replace the silent as_object_mut guard around active.control with a runner error when the control artifact is not a JSON object, while preserving terminal_statuses insertion for valid tree runs.harness/tests/integration/src/scenarios/queued_message_edit_unqueue.rs (1)
167-174: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the redundant
q_prefix conditions.
starts_with("e_")already excludes any id that starts withq_. The two negative checks can never change the result. Keep the assertion focused on the client-visible prefix and the distinctness check.♻️ Proposed simplification
anyhow::ensure!( edit_entry_id.starts_with("e_") - && !edit_entry_id.starts_with("q_") && removed_entry_id.starts_with("e_") - && !removed_entry_id.starts_with("q_") && edit_entry_id != removed_entry_id, "intervention did not use distinct client-visible entry ids: edit={edit_entry_id}, remove={removed_entry_id}" );🤖 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 `@harness/tests/integration/src/scenarios/queued_message_edit_unqueue.rs` around lines 167 - 174, In the assertion around edit_entry_id and removed_entry_id, remove both redundant !starts_with("q_") conditions. Keep the e_ prefix validations and the edit_entry_id != removed_entry_id distinctness check unchanged.harness/tests/integration/src/scenario/floor.rs (1)
101-115: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueConsider handling expectations that declare both
failedandcancelledturns.
declares_failure()matches first, socleanbecomessummary.error_count > 0andcancelled_errors_are_boundnever runs. A mixed-status fixture would then accept error spans bound to any turn. No current fixture mixes the two statuses, so this is a hardening suggestion only.♻️ Proposed refactor: bind error spans whenever cancellation is declared
let clean = if expected.declares_failure() { - summary.error_count > 0 - } else if expected - .turn_statuses - .iter() - .any(|status| status == "cancelled") - { + summary.error_count > 0 + && (!expected + .turn_statuses + .iter() + .any(|status| status == "cancelled") + || cancelled_or_failed_errors_are_bound(run, expected)) + } else if expected + .turn_statuses + .iter() + .any(|status| status == "cancelled") + { cancelled_errors_are_bound(run, expected) } else { summary.error_count == 0 };Also applies to: 139-157
🤖 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 `@harness/tests/integration/src/scenario/floor.rs` around lines 101 - 115, Update the clean-evaluation logic around declares_failure and cancelled turn statuses so cancelled_errors_are_bound is applied whenever cancellation is declared, including mixed failed-and-cancelled expectations. Preserve the existing requirement that declared failures produce at least one error, while ensuring error spans are bound only to declared cancelled turns.harness/tests/integration/src/fixtures/loading.rs (1)
146-151: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUpdate the
expected_turn_statusesdoc to match the new rule.The field doc at Lines 15-17 states that the last status must be
completed. Validation now exempts intervention fixtures, andStopCancelCascaderequires all statuses to becancelled. Adjust the doc so authors do not rely on the old invariant.🤖 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 `@harness/tests/integration/src/fixtures/loading.rs` around lines 146 - 151, Update the documentation for expected_turn_statuses to describe the current validation rules: non-intervention fixtures require a final completed status, while intervention fixtures are exempt and StopCancelCascade requires every status to be cancelled. Remove the outdated unconditional claim that the last status must always be completed.
🤖 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 `@harness/tests/integration/src/scenarios/stop_cancel_cascade.rs`:
- Around line 210-241: Update the lifecycle validation loop to collect each
non-root child’s session_id in a set rather than incrementing child_count per
span. Preserve the existing terminal/cancelled and parent-link checks, then
assert the set contains exactly two distinct child sessions so duplicate
lifecycle spans are accepted.
- Around line 205-208: Update the durable stop assertion in the stop-cancel
cascade test to match the message actually persisted by harness::stop, which
acknowledges cancellation with “stopping: true”; preferably define the expected
wording in harness/src/functions/stop.rs and reuse that shared constant in the
test.
In `@harness/tests/integration/src/scripted_router.rs`:
- Around line 284-306: Fix the lost-wakeup race in wait_for_gate by
creating/registering the gate_notify.notified() future before locking and
reading arrivals, while preserving the existing immediate-success and deadline
timeout behavior. Add a regression test that runs concurrent await_gate arrivals
against wait_for_gate and verifies the waiter completes promptly after the
required arrivals.
---
Nitpick comments:
In `@harness/tests/integration/src/fixtures/loading.rs`:
- Around line 146-151: Update the documentation for expected_turn_statuses to
describe the current validation rules: non-intervention fixtures require a final
completed status, while intervention fixtures are exempt and StopCancelCascade
requires every status to be cancelled. Remove the outdated unconditional claim
that the last status must always be completed.
In `@harness/tests/integration/src/probe.rs`:
- Around line 247-265: Document in bind_child_completion_observer that it must
run after confirm_completion_binding, since a later confirm_completion_binding
call removes other LIFECYCLE_TRIGGER_TYPE bindings. Preserve the current trigger
behavior and clearly note that rebinding after an engine restart must not occur
in an order that silently removes the parent-filtered child observer.
In `@harness/tests/integration/src/scenario/floor.rs`:
- Around line 101-115: Update the clean-evaluation logic around declares_failure
and cancelled turn statuses so cancelled_errors_are_bound is applied whenever
cancellation is declared, including mixed failed-and-cancelled expectations.
Preserve the existing requirement that declared failures produce at least one
error, while ensuring error spans are bound only to declared cancelled turns.
In `@harness/tests/integration/src/scenario/phases/completion.rs`:
- Around line 166-195: Update the tree-status collection in the completion flow
to exclude the root by comparing each session ID with self.session_id instead of
relying on tree_sessions ordering. Replace the silent as_object_mut guard around
active.control with a runner error when the control artifact is not a JSON
object, while preserving terminal_statuses insertion for valid tree runs.
In `@harness/tests/integration/src/scenario/phases/intervention.rs`:
- Around line 673-689: Update ensure_client_entry_id to remove the unreachable
row.entry_id.starts_with("q_") condition, leaving only the client-visible "e_"
prefix validation while preserving the existing error behavior.
- Around line 225-241: Update the control evidence construction around the
stop-cancel cascade handler to use the same hyphenated kind naming convention as
the queued-edit handler, and add the top-level released flag directly to the
json! literal. Remove the post-construction control["released"] assignment and
make the binding immutable unless other code requires mutability.
- Around line 297-334: The queue_specs entries redundantly store label and
suffix; remove the third tuple field, iterate over label and message, and use
label when constructing idempotency_key. In the same intervention handler,
replace the hardcoded “integration-012” segment with the applicable scenario
identifier derived from the existing scenario context so keys remain accurately
named for every fixture.
- Around line 109-120: The root status readiness predicate in the intervention
phase hardcodes the child count instead of deriving it from expected_in_flight.
Update the children length check within the wait_for_status closure to use
expected_in_flight minus the root turn, preserving the existing
expected_in_flight fixture contract and status conditions.
In `@harness/tests/integration/src/scenarios/queued_message_edit_unqueue.rs`:
- Around line 167-174: In the assertion around edit_entry_id and
removed_entry_id, remove both redundant !starts_with("q_") conditions. Keep the
e_ prefix validations and the edit_entry_id != removed_entry_id distinctness
check unchanged.
In `@harness/tests/integration/src/trace_evidence.rs`:
- Around line 84-95: Remove the redundant trace_ids.sort() after collecting from
BTreeSet, and update the group-matching logic to track requested session IDs
that were found; assert that every ID in wanted matched a group, with a clear
premise-failure message before proceeding to trace ID collection.
🪄 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 Plus
Run ID: 42a451b3-ed4c-4c80-af91-3314b6282a3b
📒 Files selected for processing (25)
harness/src/functions/stop.rsharness/tests/integration/README.mdharness/tests/integration/src/evidence_data.rsharness/tests/integration/src/fixtures.rsharness/tests/integration/src/fixtures/loading.rsharness/tests/integration/src/fixtures/tests.rsharness/tests/integration/src/probe.rsharness/tests/integration/src/runtime.rsharness/tests/integration/src/scenario/floor.rsharness/tests/integration/src/scenario/phases/arm.rsharness/tests/integration/src/scenario/phases/completion.rsharness/tests/integration/src/scenario/phases/evidence.rsharness/tests/integration/src/scenario/phases/intervention.rsharness/tests/integration/src/scenario/phases/mod.rsharness/tests/integration/src/scenario/runner.rsharness/tests/integration/src/scenario/state.rsharness/tests/integration/src/scenarios/dsl.rsharness/tests/integration/src/scenarios/mod.rsharness/tests/integration/src/scenarios/queued_message_edit_unqueue.rsharness/tests/integration/src/scenarios/stop_cancel_cascade.rsharness/tests/integration/src/scripted_router.rsharness/tests/integration/src/trace_evidence.rsharness/tests/integration/src/types/scenario/compiled.rsharness/tests/integration/src/types/script.rsharness/tests/integration/tests/determinism.rs
1 participant
What changed
Adds integration coverage for two issues observed during recording:
The harness now records intervention controls, cancellation-tree status, router abort evidence, and trace coverage needed to diagnose these flows.
Root cause covered
harness::stop could acknowledge stopping: false when cancellation finalized the matching turn before the handler reacquired the session lock. The handler now reports the accepted stop request correctly, while the integration floor binds cancellation abort spans to the cancelled turns.
Validation
Refs MOT-4107