(MOT-4295) test(harness): multi-target integration scenarios INT-014 and INT-016 plus runner enablers - #665
Open
ytallo wants to merge 1 commit into
Open
(MOT-4295) test(harness): multi-target integration scenarios INT-014 and INT-016 plus runner enablers#665ytallo wants to merge 1 commit into
ytallo wants to merge 1 commit into
Conversation
…and INT-016 plus runner enablers Two new scenarios aimed at the coldest harness modules found by the coverage report, plus the runner capabilities they needed: - probe actions now record their responses into RunEvidence (probe_responses + expect_probe_response), unlocking body assertions for state/ledger/teardown reads - scripted hooks: fixtures can declare probe-hosted hook functions (Continue/HoldOnce/Deny/Mutate behaviors) bound to harness::hook::* trigger types before Send, with whole-run hook_calls evidence - Send options gain max_total_tokens; terminal statuses may now end in 'failed' (floor already pins the durable status to it) INT-014 budget-preflight-exceeded: a frozen budget admits generation 1, then a ~240k-char controlled-function result deterministically blows generation 2's reservation — failed turn, harness.budget_exceeded custom error entry, and the harness_budget ledger asserted through a recorded probe response (reconcile + rollback). INT-016 notify-grant-teardown: two standing notify subscriptions; a state fire lands a trigger_fired entry plus the injected '[notification: …]' user message that seeds the second tracked turn; filesystem grant/grants/revoke round-trip; the budget ledger holds the exact reconciled usage; harness::teardown sweeps both bindings. INT-013 held-call-resolve is authored but NOT registered: running it exposed an apparent harness defect — after a pre-trigger hook hold, harness::status times out and the turn record vanishes from state, so the resolve intervention can never find the parked call. Isolation runs prove the scripted-hook plumbing is sound (Continue variant completes in ~2.7s with both hooks served). See the scenario doc comment. All 14 registered direct scenarios pass against the local engine; 92 unit tests pass; zero build warnings.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
skill-check — worker0 verified, 51 skipped (no docs/).
Four for four. Nicely done. |
📝 WalkthroughWalkthroughThe integration harness adds scripted hook configuration and execution, captures probe and hook evidence, supports held-call resolution, adds optional token-budget compilation, and introduces INT-014 and INT-016 scenarios. ChangesIntegration harness extensions
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ScenarioDSL
participant ScenarioProbe
participant HarnessHook
participant RunEvidence
ScenarioDSL->>ScenarioProbe: configure and bind scripted hooks
ScenarioProbe->>HarnessHook: invoke hook behavior
HarnessHook-->>ScenarioProbe: return response and record call
ScenarioProbe->>RunEvidence: publish probe and hook evidence
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 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 |
![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
🧹 Nitpick comments (1)
harness/tests/integration/src/scenarios/held_call_resolve.rs (1)
1-11: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDeclare the module so the fixture keeps compiling.
This file has no
moddeclaration inharness/tests/integration/src/scenarios/mod.rs. Rust therefore never compiles it. Thescenario()builder and thefixture_holds_and_resolves_one_calltest are both inert, and the hook plumbing they exercise stays unverified. The fixture will drift out of sync with the DSL until MOT-4296 is fixed.Declare
mod held_call_resolve;and keep the fixture out ofall(). The compiler then checks the builder chain, and the unit test runs on everycargo test.♻️ Proposed change in
harness/tests/integration/src/scenarios/mod.rs+// INT-013 is authored but withheld from `all()` until MOT-4296 is fixed. +// The module stays declared so the fixture and its unit test keep compiling. +mod held_call_resolve;After that change, you can also drop the
#[allow(dead_code)]onScenario::hookinharness/tests/integration/src/scenarios/dsl.rs.🤖 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/held_call_resolve.rs` around lines 1 - 11, Declare the held_call_resolve module in the scenarios module file so its scenario builder and test compile and run, but do not include it in all(). After registering the module, remove the now-unnecessary #[allow(dead_code)] attribute from Scenario::hook in the DSL.
🤖 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/notify_grant_teardown.rs`:
- Line 157: Update the comments near the trigger teardown fixture, including the
comment above the durable trigger_fired assertion and the earlier subscription
comment, to remove stale “once” and “retired once-sub” wording. Describe the
binding consistently with its declared once: false and retired: false state,
without changing the subscription setup or assertions.
---
Nitpick comments:
In `@harness/tests/integration/src/scenarios/held_call_resolve.rs`:
- Around line 1-11: Declare the held_call_resolve module in the scenarios module
file so its scenario builder and test compile and run, but do not include it in
all(). After registering the module, remove the now-unnecessary
#[allow(dead_code)] attribute from Scenario::hook in the DSL.
🪄 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: 43de1535-1221-4af7-b8a7-805407f5b83a
📒 Files selected for processing (19)
harness/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/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/state.rsharness/tests/integration/src/scenarios/budget_preflight_exceeded.rsharness/tests/integration/src/scenarios/dsl.rsharness/tests/integration/src/scenarios/held_call_resolve.rsharness/tests/integration/src/scenarios/mod.rsharness/tests/integration/src/scenarios/notify_grant_teardown.rsharness/tests/integration/src/types/scenario/compiled.rsharness/tests/integration/tests/determinism.rs
| .verify(|run| { | ||
| run.expect_assistant_texts(["armed", "acknowledged"])?; | ||
|
|
||
| // The fire left a durable trigger_fired entry for the retired once-sub. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix the stale once wording in two comments.
Both subscriptions are declared with once: false, and the assertion at lines 175-180 requires once == false and retired == false. The comment on line 157 calls the binding a "retired once-sub", and the comment on lines 69-70 calls it "the once subscription". Both contradict the fixture and the assertion.
📝 Proposed comment fixes
- // Turn 1 done: trip the once subscription's key. The notify fire injects
- // the user message that seeds turn 2.
+ // Turn 1 done: trip the standing subscription's key. The notify fire
+ // injects the user message that seeds turn 2.- // The fire left a durable trigger_fired entry for the retired once-sub.
+ // The fire left a durable trigger_fired entry, and the standing
+ // subscription stayed live.🤖 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/notify_grant_teardown.rs` at line
157, Update the comments near the trigger teardown fixture, including the
comment above the durable trigger_fired assertion and the earlier subscription
comment, to remove stale “once” and “retired once-sub” wording. Describe the
binding consistently with its declared once: false and retired: false state,
without changing the subscription setup or assertions.
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
1 participant
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.
Part of MOT-4295 (coverage initiative). Two new integration scenarios aimed at the coldest harness modules identified by the stack coverage report, plus the runner capabilities they required.
Runner enablers
{function_id, response}intoRunEvidence.probe_responses(withexpect_probe_response), unlocking body assertions for state/ledger/teardown reads. Additive; existing scenarios unchanged.Continue/HoldOnce/Deny/Mutate) bound toharness::hook::<point>trigger types before Send, with whole-runhook_callsevidence.Sendgainsmax_total_tokens; terminal statuses may now end infailed(the floor pins the durable status to it).Scenarios
budget-preflight-exceeded—budget.rs(prepare/reserve/reconcile/rollback + Exceeded) andturn_loop::finalize_failed: a frozen 30k budget admits generation 1, then a ~240k-char controlled-function result deterministically blows generation 2's reservation (no system-prompt calibration needed). Asserts the failed terminal turn, theharness.budget_exceededcustom error entry, and theharness_budgetledger via a recorded probe response.notify-grant-teardown—subscriptions/notify_agent.rs,functions/filesystem.rs+filesystem_grants.rs, budget happy path, andteardown.rs/reconcile::sweep_owner: a state fire lands atrigger_firedentry plus the injected[notification: …]message that seeds the second tracked turn; grant→grants→revoke round-trip; ledger holds the exact reconciled usage (39 tokens);harness::teardownsweeps both standing bindings.Bug found: MOT-4296
INT-013
held-call-resolveis authored but NOT registered. Running it exposed an apparent harness defect: after apre_triggerhook answershold,harness::statustimes out at the engine and the turn record disappears from state, soharness::function::resolvecan never find the parked call. Isolation runs prove the scripted-hook plumbing is sound (theContinuevariant completes in ~2.7s with both hooks served). The fixture + itsHeldCallResolveintervention are complete and will verify the fix end to end — registering it is a 3-line diff once MOT-4296 lands.Verification
cargo test -p harness-integration), including new coverage forgroupRunFailures-style fixture wiring, hook validation, and the fixture registry counts.Summary by CodeRabbit
New Features
Documentation