Route executable-operation Actions through scheduler-owned Ticks#688
Merged
Conversation
Contributor
|
Warning Review limit reached
Next review available in: 45 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: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (10)
📝 WalkthroughWalkthroughThis PR routes canonical executable-operation Actions through scheduler-owned atomic Ticks, adding typed outcomes, batch WAL persistence, recovery validation, obstruction handling, integration tests, CI coverage, and lifecycle documentation. ChangesExecutable-operation Action pipeline
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
Possibly related PRs
🚥 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 |
![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: 242392d76e
ℹ️ 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".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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 `@crates/warp-core/src/coordinator.rs`:
- Around line 1741-1765: Document and enforce the positional mapping in this
same-basis correlation branch: bind the ingress_id sorting and receipt.entries()
index lookup to the ordering established by
commit_scheduler_action_batch_to_state_v1, using a debug_assert! or equivalent
invariant check. Preserve no_match() for missing correlations or out-of-range
entries, and ensure any ordering change cannot silently attribute the wrong
disposition.
In `@crates/warp-core/src/trusted_runtime_host.rs`:
- Around line 1572-1614: The scheduler path must stop scanning the full
witnessed-submission history. Update admit_pending_echo_operation_actions_v1 to
consume a bounded pending-submission index, and update the tick_once per-outcome
lookup to use the existing receipt_correlation_by_submission mapping keyed by
submission identity instead of witnessed_submissions().find; preserve admission
and outcome behavior while avoiding repeated durable-history scans.
In `@docs/adr/0025-scheduler-owned-executable-operation-actions.md`:
- Around line 132-138: Update the documented canonical Action order so each Tick
WAL transaction contains exactly one TickReceiptRecorded, followed by one
ordered ReceiptCorrelationRecorded and ActionOutcomeRecorded pair per Action,
then exactly one RuntimeStateDeltaRecorded; remove the implication that
TickReceiptRecorded repeats per Action.
In `@docs/topics/GeneratedRules.md`:
- Around line 143-149: Update the stale corridor prose below the Action
submission bullets to describe composite Action Ticks rather than singleton
ticks: revise the “parent patch and singleton tick entry” statement to
acknowledge multiple Actions, and replace the claim that the current slice lacks
general scheduler batch integration with the shipped behavior where two
independent executable Actions are composed and committed in one durable Tick.
In `@README.md`:
- Around line 260-267: Update the earlier README statement that excludes
scheduler-batch composition so it reflects the implemented behavior described in
the later section: independent Actions can share one scheduler-owned Tick. Keep
any remaining scope limitations accurate and consistent with the
executable-operation discussion.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: b2add678-a8a0-4fbb-aeb3-57269de878f7
📒 Files selected for processing (27)
.github/workflows/ci.yml.github/workflows/macos-local.ymlCHANGELOG.mdREADME.mdcrates/warp-core/Cargo.tomlcrates/warp-core/src/causal_wal.rscrates/warp-core/src/causal_wal_tests.rscrates/warp-core/src/contract_obstruction.rscrates/warp-core/src/coordinator.rscrates/warp-core/src/echo_operation.rscrates/warp-core/src/engine_impl.rscrates/warp-core/src/head_inbox.rscrates/warp-core/src/lib.rscrates/warp-core/src/provenance_codec.rscrates/warp-core/src/receipt.rscrates/warp-core/src/trusted_runtime_host.rscrates/warp-core/tests/executable_operation_pipeline_tests.rsdocs/README.mddocs/adr/0023-admitted-executable-operation-packages.mddocs/adr/0025-scheduler-owned-executable-operation-actions.mddocs/adr/README.mddocs/architecture/application-contract-hosting.mddocs/spec/scheduler-warp-core.mddocs/topics/GeneratedRules.mddocs/topics/WAL.mdscripts/verify-local.shtests/hooks/test_verify_local.sh
Owner
Author
Self-Code Review — merge gate lockedReviewed fresh @codex please independently confirm or rebut each finding against the current head.
Agent-ready mitigation prompts
Judgment: MERGE GATE: LOCKED 🔒. Green CI does not exercise these adversarial/collision/boundary cases. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ea978cd399
ℹ️ 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".
Owner
Author
Code Lawyer Activity SummaryLockdown began from a clean worktree and fresh
Verification
@codex review please. Independently confirm the final head, especially ingress-order bounding, noncommitted decision coordinates, and |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/warp-core/src/head_inbox.rs (1)
833-864: 🚀 Performance & Scalability | 🟠 Major | ⚡ Quick winOrdinary bulk admission regresses from
mem::taketo per-keyBTreeMap::remove.The coordinator calls
admit_partitionedfor every head whenever Action support is active (operation_actions.is_some()), so the non-Action path no longer hitsadmit()'s O(1)std::mem::take. Instead each tick allocates aVec<Hash>of up topending.len()ids (32 B each) and then performs N × O(log N) removals. On a large inbox that is measurable per-SuperTick overhead for work that used to be a single move.Add a drain fast path when the selected category is the entire pending set and the limit does not bind.
⚡ Fast path for the unbounded whole-inbox case
if limit == 0 { return Vec::new(); } + // Whole-inbox, unbounded case: keep the O(1) move instead of + // rebuilding the key set and removing entry by entry. + if !(has_partition && has_other) && limit >= self.pending.len() { + return std::mem::take(&mut self.pending).into_values().collect(); + } let mut selected_ids = Vec::new();🤖 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 `@crates/warp-core/src/head_inbox.rs` around lines 833 - 864, Update the admission logic around policy_limit, limit, and selected_ids to return the entire pending map directly when the selected category covers all entries and limit is unbounded. Use std::mem::take on self.pending for this fast path, while preserving partition filtering and bounded-limit behavior.crates/warp-core/src/trusted_runtime_host.rs (1)
1955-1970: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueDeep-clones every Action outcome per tick just to build a lookup map.
action_outcomes.iter().cloned()copies eachEchoOperationActionOutcomeV1(boxed receipts, footprints, conflict blocker vectors) solely sorecord_tick_receipt_batchcan read them; the originals are then consumed again at Line 2026. A borrowed map avoids the copy —record_tick_receipt_batchonly reads through.get().♻️ Borrow instead of clone
- let action_outcomes_by_submission = - action_outcomes.iter().cloned().collect::<BTreeMap<_, _>>(); + let action_outcomes_by_submission = action_outcomes + .iter() + .map(|(submission_id, outcome)| (*submission_id, outcome)) + .collect::<BTreeMap<_, &EchoOperationActionOutcomeV1>>();Then change
record_tick_receipt_batch's parameter to&BTreeMap<Hash, &EchoOperationActionOutcomeV1>.As per path instructions: "Performance matters — flag unnecessary allocations in hot paths."
🤖 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 `@crates/warp-core/src/trusted_runtime_host.rs` around lines 1955 - 1970, Update the action-outcome lookup construction to borrow outcomes instead of cloning them: collect `action_outcomes.iter()` into a `BTreeMap<Hash, &EchoOperationActionOutcomeV1>`. Change `record_tick_receipt_batch` to accept that borrowed map and keep its lookup logic read-only, while preserving the later consumption of the original `action_outcomes`.Source: Path instructions
🤖 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 `@crates/warp-core/src/echo_operation.rs`:
- Around line 4965-5012: Add a debug_assert_eq! immediately before the decisions
mapping to verify decisions.len() matches tick_receipt.entries().len(). Keep
enumerate() as the source of member_index so positional alignment remains
explicit for recovery validation.
In `@crates/warp-core/src/trusted_runtime_host.rs`:
- Around line 436-441: The recovery performance counters are currently
hard-wired or never updated, making their test assertions vacuous. In
crates/warp-core/src/trusted_runtime_host.rs:436-441, either implement
increments at the actual installed-package lookup and per-Action
installation-set materialization sites (including mutable metrics/counter
bindings) or remove the instrumentation; in
crates/warp-core/tests/executable_operation_pipeline_tests.rs:2932-2943, retain
the two counter assertions only if counters become real, otherwise remove them
while keeping the echo_operation_parent_state_replay_count_for_test == 1
assertion.
In `@crates/warp-core/tests/executable_operation_pipeline_tests.rs`:
- Around line 2932-2943: The kernel instrumentation behind the two
installation-related test counters is inert, so the assertions cannot detect
regressions. Update the relevant trusted-runtime counter definitions and the
validate_recovered_echo_operation_action_outcomes path to increment installation
snapshot and linear comparison counters when those operations occur, while
preserving the existing parent replay counter behavior.
In `@docs/adr/0025-scheduler-owned-executable-operation-actions.md`:
- Around line 131-138: Update the typed executable-Action outcome description to
make the committed member consequence conditional: only applied Actions bind a
committed patch member, while obstruction and footprint-conflict outcomes
explicitly carry no committed consequence. Preserve the existing evidence
requirements for rejected outcomes, including writer head, worldline, Tick
coordinates, commit and receipt identities, and canonical member index.
---
Outside diff comments:
In `@crates/warp-core/src/head_inbox.rs`:
- Around line 833-864: Update the admission logic around policy_limit, limit,
and selected_ids to return the entire pending map directly when the selected
category covers all entries and limit is unbounded. Use std::mem::take on
self.pending for this fast path, while preserving partition filtering and
bounded-limit behavior.
In `@crates/warp-core/src/trusted_runtime_host.rs`:
- Around line 1955-1970: Update the action-outcome lookup construction to borrow
outcomes instead of cloning them: collect `action_outcomes.iter()` into a
`BTreeMap<Hash, &EchoOperationActionOutcomeV1>`. Change
`record_tick_receipt_batch` to accept that borrowed map and keep its lookup
logic read-only, while preserving the later consumption of the original
`action_outcomes`.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 2dfbac7f-3d0f-4d8d-84c4-b7cd794710de
📒 Files selected for processing (12)
CHANGELOG.mdcrates/warp-core/src/contract_obstruction.rscrates/warp-core/src/coordinator.rscrates/warp-core/src/echo_operation.rscrates/warp-core/src/head_inbox.rscrates/warp-core/src/lib.rscrates/warp-core/src/trusted_runtime_host.rscrates/warp-core/tests/executable_operation_pipeline_tests.rsdocs/adr/0025-scheduler-owned-executable-operation-actions.mddocs/spec/scheduler-warp-core.mddocs/topics/WAL.mdscripts/verify-local.sh
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 022ebf7d3e
ℹ️ 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".
Owner
Author
Code Lawyer Final Activity SummaryLockdown began from a clean worktree and fresh
Verification on the final local head
@codex review please. Review the exact final head, with particular attention to per-runnable-head admission capacity, cross-worldline obstruction recovery, global-Tick category alternation, and admission-only drain progress. |
|
Codex Review: Didn't find any major issues. 🎉 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
Owner
Author
Code Lawyer self-finding@codex please confirm the current-head repair after it lands.
|
Summary
Testing
|
Owner
Author
Code Lawyer Final Activity Summary — 27-item closureLockdown began from a clean worktree and fresh
Current-head evidence
@codex review please — independently review this exact head. CodeRabbit is currently rate-limited; its review will be re-requested when the stated cooldown expires. |
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
Owner
Author
|
@coderabbitai review please |
Contributor
|
✅ Action performedReview finished.
|
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.
Summary
Acceptance evidence
cargo fmt --all -- --checkcargo clippy -p warp-core --lib -- -D warnings -D missing_docscargo clippy -p warp-core --lib --features native_rule_bootstrap,trusted_runtime,host_test -- -D warningscargo clippy -p warp-core --features native_rule_bootstrap,trusted_runtime,host_test --test executable_operation_pipeline_tests -- -D warnings -D missing_docscargo test -p warp-core --features native_rule_bootstrap,trusted_runtime,host_testcargo test -p warp-corecargo test -p warp-core --features native_rule_bootstrap,trusted_runtime,host_test --test executable_operation_pipeline_tests scheduler_commits_two_independent_executable_actions_in_one_durable_tickcargo test -p warp-core scheduler::tests::partitioned_admission_never_mixes_native_and_executable_actionscargo test -p warp-core causal_wal_tests::tick_batch_rejects_mixed_action_outcome_framingbash -n scripts/verify-local.sh tests/hooks/test_verify_local.shbash tests/hooks/test_verify_local.sh(162 passed, 0 failed)git diff --checkDocumentation
ADR 0025 defines the scheduler-owned lifecycle and updates the canonical application-hosting, scheduler, WAL, and generated-rules documentation. The changelog records the new Action/Tick/WAL boundary.
Explicit non-goals
Closes #687
Summary by CodeRabbit