fix: preserve pending branches in update_state(as_node); reject unenforced steering blocks at compile time - #92
Conversation
…ning pending work instead of replacing it Co-authored-by: Medulla <medulla@tinyhumans.ai>
…me can enforce them Co-authored-by: Medulla <medulla@tinyhumans.ai>
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 57 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 (10)
Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: aea4de73bd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let mut merged: Vec<Activation> = match &base.pending_activations { | ||
| Some(pending) if !pending.is_empty() => pending | ||
| .iter() | ||
| .map(Activation::from) | ||
| .filter(|activation| activation.node != *node) | ||
| .collect(), |
There was a problem hiding this comment.
When an interrupt checkpoint contains both an interrupted node and another pending branch, an attributed update to that other branch now retains the interrupted activation here, but the new checkpoint still clears interrupts and omits the base checkpoint's interrupted_nodes metadata. Consequently, resume(..., Command::resume(value)) finds no interrupted node and fans the human resume value across every merged activation, including successors or Send workers that never interrupted, potentially driving them down incorrect resumed paths. Preserve and filter the base interrupt ownership for retained activations, and cover this attributed-update/resume interaction with a focused routing test.
AGENTS.md reference: AGENTS.md:L61-L69
Useful? React with 👍 / 👎.
1 participant
Summary
Follow-up to #91, addressing the two P1 review comments chatgpt-codex-connector posted after that PR merged (state_api thread, steering thread). Both fixes were independently re-verified and code-reviewed before opening this PR.
1.
update_state(as_node)no longer drops other pending branches (src/graph/compiled/state_api.rs)The reviewer's exact mixed shape (one route call yielding both a barred waiting target and a plain successor) is unreachable today —
route(node, None, _)returns at most one target — but investigating it surfaced the real, broader loss, pre-existing since before #91: an attributed write replaced the checkpoint's schedule with the attributed node's successors, dropping every other in-flight branch. Reachable shapes fixed:Sendactivations lost their args.The fix derives
next_nodesandpending_activationsfrom a single merged activation list: base pending minus the attributed node (with the legacynext_nodesfallback matching the executor's resume predicate), plus routed successors, barrier-withheld targets excluded with arrivals still recorded, plain successors deduped against already-pending nodes,Sendactivations never deduped. This also makes the reviewer's hypothetical shape correct-by-construction if routing ever grows fan-out, and makes anext_nodes/pending_activationsdisagreement structurally impossible. When the attributed node has several pendingSendpackets, the write completes all of them at once (documented).Regression tests (each red on the old code):
attributed_update_keeps_other_pending_branches_scheduled,attributed_update_to_sink_node_keeps_other_pending_branches,attributed_update_preserves_pending_send_args_of_other_branches.2.
steering { … }blocks are rejected at compile time instead of silently discarded (src/language/compiler.rs)#91 made the documented
steeringgrammar parse, but compilation dropped it on the floor — a blueprint could deploy believing its parent/human allowlists were enforced while the runtime received nothing. A faithful lowering is not possible against today's runtime:harness::steering::SteeringPolicyis a single flat allowlist with no actor separation (parent allowvshuman allowwould have to be unioned, silently widening a declared restriction on a security-relevant surface), no delivery-policy concept, and two documented command kinds (add_instruction,request_status) that don't exist asSteeringCommandKinds.So
compile_graphnow fails with aCompileerror naming the node, stating the block is parsed but not yet enforced, and pointing at the working alternative (SteeringPolicyviaNodeFactory). All three entry points funnel through it (compile,compile_with_provenance,compile_source). The reference docs now describesteeringas reserved grammar rather than claiming enforcement.Behavior change: a
.ragsource containing asteering {}block that compiled (unenforced) since #91 now fails to compile with an actionable diagnostic. Downstream consumers should confirm they ship no such sources; failing loudly here is the point of the fix.Regression tests:
steering_block_is_rejected_at_compile_time_until_it_is_enforced,subagent_node_without_steering_still_compiles(rejection scoped to the block, not to subagent nodes); #91's parse-level tests retained.Validation
cargo fmt --checkclean;cargo clippy --all-targets --all-features -- -D warningsclean.cargo test --all-features: all suites green (2,035+ tests, 0 failures).