Repository navigation
arch(ws-13): add host cancellation accessor - #3648
Conversation
There was a problem hiding this comment.
Code Review
This pull request implements a cancellation mechanism for the agent loop, introducing a LoopCancellationPort trait and integrating frequent checks within the CanonicalAgentLoopExecutor. It also provides infrastructure for managing cancellation signals and updates relevant host implementations. Feedback suggests refactoring the repetitive cancellation check logic using a macro to improve maintainability. Additionally, it is recommended to utilize the metadata within the LoopCancellationSignal rather than hardcoding exit reasons and to ensure the request method is idempotent to align with the trait's contract.
| state = match self.checkpoint_and_exit_if_cancelled(host, state).await? { | ||
| CancelCheck::Continue(state) => state, | ||
| CancelCheck::Exit(exit) => return Ok(exit), | ||
| }; |
There was a problem hiding this comment.
This cancellation check pattern is repeated over 15 times throughout the executor. This significant duplication makes the code harder to read and maintain. Consider introducing a macro or a more flexible helper to encapsulate this logic. Since the return type varies across different functions (e.g., Ok(exit), Ok(ModelStep::Exit(exit)), Ok(BatchStep::Exit(exit))), a macro that accepts an optional exit wrapper could be effective for reducing this boilerplate.
| host: &(dyn AgentLoopDriverHost + Send + Sync), | ||
| state: LoopExecutionState, | ||
| ) -> Result<CancelCheck, AgentLoopExecutorError> { | ||
| let Some(_signal) = host.observe_cancellation() else { |
There was a problem hiding this comment.
The LoopCancellationSignal returned by host.observe_cancellation() is currently ignored (using _signal). This signal contains valuable metadata such as the reason_kind (e.g., UserRequested, Policy) and the requested_at timestamp. By ignoring it here and hardcoding LoopCancelledReasonKind::HostCancellation in cancelled_exit, the specific reason for cancellation is lost in the loop's exit status and transcript. Consider passing the signal to cancelled_exit to allow for more precise reason mapping.
| pub fn request(&self, reason_kind: LoopCancelReasonKind) { | ||
| let signal = LoopCancellationSignal { | ||
| reason_kind, | ||
| requested_at: Utc::now(), | ||
| }; | ||
| *self.signal.write() = Some(signal); | ||
| self.fired.store(true, Ordering::Release); | ||
| } |
There was a problem hiding this comment.
The request method should be idempotent to satisfy the LoopCancellationPort contract, which states that repeated calls to observe_cancellation must return the same signal. Currently, multiple calls to request will overwrite the signal and update the timestamp. It is recommended to check if the cancellation has already been fired before updating the signal to ensure the first request's metadata is preserved.
pub fn request(&self, reason_kind: LoopCancelReasonKind) {
if self.fired.load(Ordering::Acquire) {
return;
}
let mut signal_lock = self.signal.write();
if signal_lock.is_some() {
return;
}
let signal = LoopCancellationSignal {
reason_kind,
requested_at: Utc::now(),
};
*signal_lock = Some(signal);
self.fired.store(true, Ordering::Release);
}
SummaryReviewed WS13 PR #3648 only. Cancellation probes can discard already-completed model/capability work, and cancellation signal handle does not latch first signal. Validation:
Findings
Security/data-flow notes
Missing tests
|
Review notes — WS13 cancellation accessorPull-based cooperative cancellation with Cooperative-only — document the limitation
WS16 wiring will need to compose this with a stream-level abort if true preemption is wanted. Reconcile with WS6a's
|
|
Follow-up from the ws-6a review pass on #3596: #3596 now handles cancellation that is already returned through model/capability host calls: capability This WS13 cancellation-accessor PR still owns cancellation observation between strategy/host calls. Please keep tests for the between-call boundaries ( |
109d2a3 to
0615a4e
Compare
1d592bb to
22f8590
Compare
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f08aad958a
ℹ️ 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".
| Err(_) => Ok(CancelCheck::Exit(LoopExit::failed( | ||
| LoopFailureKind::CheckpointRejected, | ||
| exit_id(host, "failed")?, | ||
| ))), |
There was a problem hiding this comment.
Return executor error instead of invalid Failed exit here
When cancellation is observed under a profile with require_final_checkpoint = true, this fallback emits LoopExit::failed(...) without a checkpoint ID. validate_failed_exit in crates/ironclaw_turns/src/loop_exit.rs still requires a verified final checkpoint for strict profiles, so this exit is guaranteed to be flagged as MissingFinalCheckpoint and remapped as a protocol violation rather than a trusted CheckpointRejected failure. In practice, the strict-path fallback here cannot produce the “well-formed” terminal result it intends; it should surface an executor/host error (or another path that preserves strict checkpoint guarantees) instead.
Useful? React with 👍 / 👎.
serrrfirat
left a comment
There was a problem hiding this comment.
Review findings
crates/ironclaw_agent_loop/src/executor.rs:954
Strict-profile cancellation fallback returns LoopExit::failed(..., checkpoint_id: None) when final checkpoint write fails. validate_failed_exit still requires a verified final checkpoint for strict profiles, so this becomes MissingFinalCheckpoint / protocol violation instead of the intended trusted CheckpointRejected failure. This is already covered by the current inline Codex comment on the PR, and I agree with it. Return an executor/host error or another path that preserves strict checkpoint guarantees.
crates/ironclaw_agent_loop/src/executor.rs:194-200andcrates/ironclaw_agent_loop/src/executor.rs:321-328
Cancellation can now exit while pending_input_ack is queued. After drain_user_inputs or drain_followup, the state cursor is advanced and an ack cursor is stored, but the new cancellation probe can write the final checkpoint and return before pending_input_ack.ack(host) runs. That leaves consumed steering/follow-up input unacked even though the terminal checkpoint made the cursor durable, so the input can be redelivered or remain stuck depending on the host queue. Fix by threading pending acks into the cancellation exit path, or avoid cancellation exits between input drain and the ack that follows the durable checkpoint. Add a regression where cancellation fires during/after poll_inputs.
Reviewed read-only against f08aad958a. I did not run tests.
|
Split the follow-up fixes out of this PR into a small stacked set so this PR can stay focused on WS13:
The current PR should remain the stack anchor; the additional behavior is now isolated in the follow-up PRs. |
684d837 to
e17c627
Compare
…riant ownership, resolve review comments - Remove redundant Arc<dyn> intermediate binding in with_cancellation_factory - Doc comment on AgentLoopExecutorError::Cancelled: constructed for in-flight model/capability cancelled returns, distinct from between-call boundary - Add cooperative-boundary design comment above cancellation check sites - Document intentional HostCancellation coarsening in cancelled_reason_from_signal Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
… fix strict-profile checkpoint failure path - LoopCancellationPort: expand doc comment to explicitly state cancellation is cooperative boundary-observation only, not preempted across in-flight host calls - executor: propagate CheckpointFailed error on strict-profile final checkpoint failure during cancellation (was returning LoopExit::Failed with no checkpoint_id, which fails validate_failed_exit's MissingFinalCheckpoint check) - Update cancelled_exit TODO marker to ws-17 standard format Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ter rebase Rebase onto WS-11 introduced a duplicate `reason_kind` parameter in `cancelled_exit_with_reason` and left three call sites passing a 4th argument to the 3-arg `cancelled_exit`. Drop the duplicate param and route the signal-driven cancel checks through `cancelled_exit_with_reason` directly. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…eHost and ResumePayloadHost Adding LoopCancellationPort to AgentLoopDriverHost supertrait broke two test-only host structs that were not updated. Both return None (no active cancellation signal), consistent with their roles as controlled test doubles that do not model host-initiated cancellation. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
* arch: ws-13-host-cancellation-accessor (iter 0, approved) * fix(ws-13): mark inert cancellation fallback * fix(ws-13): address cancellation review feedback * fix(ws-13): simplify with_cancellation_factory, document Cancelled variant ownership, resolve review comments - Remove redundant Arc<dyn> intermediate binding in with_cancellation_factory - Doc comment on AgentLoopExecutorError::Cancelled: constructed for in-flight model/capability cancelled returns, distinct from between-call boundary - Add cooperative-boundary design comment above cancellation check sites - Document intentional HostCancellation coarsening in cancelled_reason_from_signal Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(ws-13): resolve review comments — doc cancellation cooperativity, fix strict-profile checkpoint failure path - LoopCancellationPort: expand doc comment to explicitly state cancellation is cooperative boundary-observation only, not preempted across in-flight host calls - executor: propagate CheckpointFailed error on strict-profile final checkpoint failure during cancellation (was returning LoopExit::Failed with no checkpoint_id, which fails validate_failed_exit's MissingFinalCheckpoint check) - Update cancelled_exit TODO marker to ws-17 standard format Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(ws-13): fix duplicate parameter and 4-arg cancelled_exit calls after rebase Rebase onto WS-11 introduced a duplicate `reason_kind` parameter in `cancelled_exit_with_reason` and left three call sites passing a 4th argument to the 3-arg `cancelled_exit`. Drop the duplicate param and route the signal-driven cancel checks through `cancelled_exit_with_reason` directly. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(ws-13): cargo fmt after rebase conflict resolution Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(ws-13): cargo fmt after second rebase onto reborn-integration Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(ws-13): add missing LoopCancellationPort impls for ForbiddenResumeHost and ResumePayloadHost Adding LoopCancellationPort to AgentLoopDriverHost supertrait broke two test-only host structs that were not updated. Both return None (no active cancellation signal), consistent with their roles as controlled test doubles that do not model host-initiated cancellation. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
Context
Cancellation observation branch. It gives the planned loop a host-owned cancellation signal without mixing cancellation state into normal chat history or strategy internals.
Master spec:
docs/reborn/agent-loop-skeleton.mdWorkstream brief:
docs/reborn/agent-loop-briefs/host-cancellation-accessor.mdStack base:
arch/ws-8Latest stack maintenance on 2026-05-14:
origin/reborn-integrationthrough WS17 before publishing the PR descriptions.What landed
Reviewer focus
Non-goals / deferred work
Validation
cargo test -p ironclaw_loop_support cancellation_portcargo check -p ironclaw_agent_loop --features test-supportgit diff --checkarch/ws-14-parentvalidation: multi-cratecargo checkwithironclaw_agent_loop/test-support.Stack position