Harden the reactive pipeline path: argument recovery, once on react bindings, wiring advisories, scoped scrapling renders - #442
Conversation
…indings, add wiring advisories Root-causes a session where the same pipeline prompt failed repeatedly: - plan_calls hoists arguments flattened beside `function` into the payload instead of silently dropping them (was: empty payload + misleading "missing field" errors), and recovers stringified payloads via a leading-value parse (models append stray closing braces that a strict parse rejects as trailing data). - `once: true` is now honored on simple harness::react bindings: the registration stamps `__once`, echoes the effective flag, and the binding retires itself after its first successful spawn. Join predecessors keep join-owned lifecycle; only explicit `true` opts in. - New registration advisories: a turn-event join predecessor filtered on parent_session_id (starves multi-key joins) and a standing non-once, non-join reaction (refires on every matching event). - Engine registration failures now carry the engine's rejection reason instead of an opaque "failed".
…t failures - Rule 1 now names argument flattening (args beside `function` instead of inside `payload`) as the top failure, with its exact symptom and a WRONG/RIGHT agent_trigger example; error handling maps the symptom back. - Prove an unproven call shape once before batching parallel copies. - Document `once` semantics on react bindings (explicit true only, ignored on join predecessors, trust the echo) and make one-run pipeline kickoffs `once: true` by default, including in the canonical example. - Join delivery: leaving metadata.session_id out of predecessor specs is what lands the fan-in result back in the registering chat; pinning any id re-aims delivery away from it.
…knobs, sharpen guidance - All four fetch functions accept `css_selector` to scope markdown/text renders to one subtree (render_content already supported it); crawl now forwards `main_content_only`/`css_selector` to per-page fetches. - Injected guidance: `main_content_only` is best-effort on heavy-chrome sites, `css_selector` is the fix when chrome leaks into a render, and never re-fetch with `include_html` (full page + headers/cookies) just to clean up a render you already have.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
skill-check — worker0 verified, 37 skipped (no docs/).
Four for four. Nicely done. |
|
Warning Review limit reached
Next review available in: 27 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)
📝 WalkthroughWalkthroughThis PR adds ChangesHarness once-semantics and payload recovery
Estimated code review effort: 4 (Complex) | ~60 minutes Scrapling css_selector and main_content_only forwarding
Sequence Diagram(s)sequenceDiagram
participant Subscriber
participant SubscribeHandler
participant ReactHandler
participant Engine
Subscriber->>SubscribeHandler: register react binding (once=true)
SubscribeHandler->>SubscribeHandler: react_once() / standing_binding_advisory()
SubscribeHandler->>Engine: register trigger (__once stamped)
Engine-->>SubscribeHandler: engine trigger id
SubscribeHandler-->>Subscriber: SubscribeResponse(once=true)
Note over ReactHandler: on triggered event
ReactHandler->>ReactHandler: spawn_reaction()
ReactHandler->>ReactHandler: resolve_engine_binding()
ReactHandler->>Engine: once_unregister()
Estimated code review effort: 3 (Moderate) | ~25 minutes 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 |
![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 (2)
harness/src/functions/subscribe.rs (1)
540-552: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winStrip caller-supplied
__oncebefore stamping the effective value.
metadatastarts from caller input. If it already contains"__once": truewhile top-levelonceis omitted/false, line 550 leaves it intact;ReactSpec.oncethen auto-unregisters even though the response echoesonce: false.Proposed fix
m.insert( "__subscription_id".to_string(), Value::String(sub_id.clone()), ); + m.remove("__once"); if once { m.insert("__once".to_string(), Value::Bool(true)); }🤖 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/src/functions/subscribe.rs` around lines 540 - 552, The subscription metadata merge in subscribe should not preserve a caller-supplied "__once" flag when the effective top-level once value is false. Update the metadata stamping logic in the block that inserts OWNER_SESSION_KEY and "__subscription_id" so it explicitly removes any existing "__once" from req.metadata before conditionally re-inserting it only when once is true, ensuring the value used by ReactSpec.once always matches the response.harness/src/functions/react.rs (1)
449-453: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftEvict the local subscription only after engine unregister succeeds.
resolve_engine_bindingcallstake/take_by_trigger_idbeforeunregister_subscription. If the engine unregister fails, the durable trigger remains but the local mapping is gone, so lateronce/join cleanup cannot resolvesub_...and a one-shot binding can become orphaned/standing. Split lookup from eviction, or restore the mapping on failure.Also applies to: 605-611, 623-626
🤖 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/src/functions/react.rs` around lines 449 - 453, The subscription cleanup flow in harness::react currently removes the local binding too early via resolve_engine_binding, so if unregister_subscription fails the mapping is lost and later once/join cleanup cannot resolve the sub_... entry. Update the unregister path to separate lookup from eviction: first read the engine binding without taking it, call unregister_subscription using that engine_id, and only evict the local subscription after the engine unregister succeeds. Apply the same ordering in the other cleanup sites referenced by the react helpers so failure paths can leave the mapping intact or restore it before returning.
🧹 Nitpick comments (1)
harness/src/policy.rs (1)
191-217: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: extract payload-recovery match into a named helper.
The nested match with inline map manipulation (flagged as high complexity) is correct but dense. A small
fn recover_agent_trigger_payload(arguments: &Value) -> Valuewould isolate the three recovery paths (string-parse, direct-value, hoist) for readability and easier future testing in isolation.♻️ Suggested extraction
+fn recover_agent_trigger_payload(arguments: &Value) -> Value { + match arguments.get("payload") { + Some(Value::String(s)) => serde_json::Deserializer::from_str(s) + .into_iter::<Value>() + .next() + .and_then(Result::ok) + .filter(Value::is_object) + .unwrap_or_else(|| Value::String(s.clone())), + Some(v) => v.clone(), + None => { + let mut map = match arguments { + Value::Object(m) => m.clone(), + _ => Default::default(), + }; + map.remove("function"); + Value::Object(map) + } + } +} + pub fn plan_calls(message: &AssistantMessage, expose: ExposeMode) -> Vec<PlannedCall> { ... - let payload = match arguments.get("payload") { - ... - }; + let payload = recover_agent_trigger_payload(arguments);🤖 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/src/policy.rs` around lines 191 - 217, The payload recovery logic in the `match arguments.get("payload")` block is correct but too dense; extract it into a small helper such as `recover_agent_trigger_payload(arguments: &Value) -> Value` near the existing policy code. Move the three cases into that helper: stringified payload parsing, direct value cloning, and hoisting flattened arguments from `arguments` when `payload` is missing, so the main flow stays readable and the recovery behavior is easier to test and maintain.
🤖 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/src/functions/subscribe.rs`:
- Around line 324-329: The subscribe flow in `subscribe` (and the matching path
around the other trigger wiring site) currently unregisters the engine trigger
when `deps.subscriptions.set_trigger_id` returns false but still proceeds as
success; update this branch to return an error instead of `Ok(SubscribeResponse
{ subscription_id: sub_id, ... })` so callers never receive an inactive
subscription id. Use the existing `set_trigger_id` and
`unregister_engine_trigger` logic in `subscribe` to clean up, then propagate a
failure result from the surrounding match/async flow when trigger-id wiring
cannot be completed.
---
Outside diff comments:
In `@harness/src/functions/react.rs`:
- Around line 449-453: The subscription cleanup flow in harness::react currently
removes the local binding too early via resolve_engine_binding, so if
unregister_subscription fails the mapping is lost and later once/join cleanup
cannot resolve the sub_... entry. Update the unregister path to separate lookup
from eviction: first read the engine binding without taking it, call
unregister_subscription using that engine_id, and only evict the local
subscription after the engine unregister succeeds. Apply the same ordering in
the other cleanup sites referenced by the react helpers so failure paths can
leave the mapping intact or restore it before returning.
In `@harness/src/functions/subscribe.rs`:
- Around line 540-552: The subscription metadata merge in subscribe should not
preserve a caller-supplied "__once" flag when the effective top-level once value
is false. Update the metadata stamping logic in the block that inserts
OWNER_SESSION_KEY and "__subscription_id" so it explicitly removes any existing
"__once" from req.metadata before conditionally re-inserting it only when once
is true, ensuring the value used by ReactSpec.once always matches the response.
---
Nitpick comments:
In `@harness/src/policy.rs`:
- Around line 191-217: The payload recovery logic in the `match
arguments.get("payload")` block is correct but too dense; extract it into a
small helper such as `recover_agent_trigger_payload(arguments: &Value) -> Value`
near the existing policy code. Move the three cases into that helper:
stringified payload parsing, direct value cloning, and hoisting flattened
arguments from `arguments` when `payload` is missing, so the main flow stays
readable and the recovery behavior is easier to test and maintain.
🪄 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: f841f26d-a577-4e96-8ec2-3288caf03143
📒 Files selected for processing (10)
harness/src/functions/react.rsharness/src/functions/subscribe.rsharness/src/policy.rsprovider-anthropic/prompts/identity.txtscrapling/src/core.pyscrapling/src/crawl.pyscrapling/src/guidance.pyscrapling/src/schemas.pyscrapling/tests/test_crawl.pyscrapling/tests/test_fetch.py
…cal binding only after engine unregister succeeds Review findings from PR #442: - A caller-supplied `__once` in react metadata could make a binding retire while the response echoed `once: false`; the stamp is now stripped before the effective value is conditionally re-inserted. - retire_binding unregisters the engine trigger BEFORE evicting the local slot: a failed engine call now keeps the `sub_` mapping resolvable so the next fire (or a manual unregister) can retry, instead of orphaning the durable binding as a standing refire. The bind-window race keeps its old contract: eviction hands cleanup to the registration path's set_trigger_id=false branch. - Documented why set_trigger_id=false deliberately returns Ok: the once fire already delivered; erroring would invite duplicate re-registration.
1 participant
Fixes MOT-3698
Why
Running the same self-driving research-pipeline prompt repeatedly surfaced a set of failures across the stack (session
console-02fc75a2and successors). This PR fixes each at its root; the final live run of the same prompt completes correctly, self-corrects on advisories, and leaves zero bindings behind.Harness
plan_callsno longer drops arguments. A call with args flattened besidefunction(or the target id used as the tool name) previously forwarded an empty payload, producing misleadingmissing field <x>errors — 11 failed calls in one session. Args are now hoisted into the payload; stringified payloads are recovered with a leading-value parse (models append stray closing braces that strict parsing rejects).once: truehonored on react bindings. Explicittrueon a simple edge stamps__once, echoes the effective flag, and the binding retires itself after its first successful spawn. Join predecessors keep join-owned lifecycle. Previouslyoncewas silently ignored and echoedfalse, leaving standing kickoffs that respawned the whole pipeline on every matching state write. (Extends MOT-3698's one-shot-trigger intent to the react path.)parent_session_id(starves multi-key joins — one live join stuck at 1/2 forever), and a standing non-once, non-join reaction.`harness::turn-completed` failed(cost one agent ~10 messages of guessing).provider-anthropic identity prompt
oncesemantics documented; one-run pipeline kickoffs default toonce: true, including in the canonical example.metadata.session_idout of predecessor specs lands the fan-in result in the registering chat; pinning any id re-aims it away (one live run "lost" its report this way).scrapling
css_selectoron all fetch renders (scope markdown/text to one subtree, e.g.#mw-content-text); crawl forwardsmain_content_only/css_selectorper page.main_content_onlyis best-effort,css_selectoris the fix for leaked chrome, and never re-fetch withinclude_htmljust to clean a render (one run burned a ~460KB response doing exactly that).Testing
cargo test(harness): 182 passed — includes new tests for every fix, each replicating the exact live failure shape (flattened args, trailing-brace payload, starving join filter, once semantics).pytest(scrapling): 82 passed — includes scoped-render and crawl-knob tests.once: trueand self-retire, the starvation advisory fired once and was obeyed, the brief landed in the chat, and the engine finished with 0 leftover bindings and 0 join records.