fix(ws-13): ack drained inputs before cancel exit

[serrrfirat]

Context

Stacked follow-up to #3685. This isolates the input cursor acknowledgement safety fix from live cancellation wiring.

What changed

• Added an ack-aware cancellation boundary helper for pending drained inputs.
• Used it after steering drain and follow-up drain before returning LoopExit::Cancelled.
• Added regression tests for steering-drain and follow-up-drain cancellation after polling.

Validation

• cargo test -p ironclaw_agent_loop cancellation_after

Stack

Base: #3685 / codex/ws13-live-cancel-wiring

[gemini-code-assist]

Code Review

This pull request introduces a mechanism to ensure that pending input acknowledgments are processed when the agent loop is cancelled. It adds a new helper function, checkpoint_and_exit_if_cancelled_after_pending_input_ack, which handles final checkpoints and input acks before exiting, and updates several call sites within the executor. New tests verify that acks are flushed after steering and followup drains. Feedback indicates that the new helper function duplicates existing logic and missed a cancellation boundary at the start of the loop, which could still lead to unacknowledged inputs. It is recommended to merge the functions to ensure consistent behavior across all cancellation points.

[gemini-code-assist]

The new helper function checkpoint_and_exit_if_cancelled_after_pending_input_ack is almost identical to the existing checkpoint_and_exit_if_cancelled. This duplication is error-prone and has already resulted in a missed update at the start of the loop (line 173 in the full file).

If a loop iteration continues after a drain_followup (line 372), pending_input_ack will be non-empty. However, the cancellation check at line 173 still uses the old non-ack-aware version, meaning a cancellation at that point will exit without acknowledging the drained inputs.

Consider merging these functions by adding the &mut PendingInputAck parameter to the original checkpoint_and_exit_if_cancelled and using it at all cancellation boundaries. Since PendingInputAck::ack is a no-op when empty, this is safe for all call sites, including those where pending_input_ack is not explicitly managed (by passing a default/empty instance).

[henrypark133]

Review — fix(ws-13): ack drained inputs before cancel exit

Blocker — strict-profile checkpoint failure diverges from checkpoint_and_exit_if_cancelled

checkpoint_and_exit_if_cancelled (line ~1075) was fixed to propagate Err(error) for the strict-profile arm. The new checkpoint_and_exit_if_cancelled_after_pending_input_ack method's equivalent arm returns Ok(CancelCheck::Exit(LoopExit::failed(..., checkpoint_id: None))) instead. Callers see Ok instead of Err, losing the propagation guarantee.

Fix: change the strict arm to match the original:

Err(error) => Err(error),

Permissive path — correct. Skipping the ack in the permissive failure arm is right: no durable checkpoint was written, so advancing the cursor would be unsafe.

Call-site coverage — complete. Both drain sites (steering and follow-up) converted to the new method. No remaining gaps.

Test gap. No test exercises the strict-profile checkpoint failure path through the new method. That test would have caught the divergence above.

Blocking on the strict-profile arm fix.

[henrypark133]

Caveman review findings

1🔴 2🟡

• 🔴 crates/ironclaw_agent_loop/src/executor.rs checkpoint_and_exit_if_cancelled_after_pending_input_ack fallback Err arm (require_final_checkpoint = false): cancelled_exit called with 4 arguments but signature takes 3. Extra cancelled_reason_from_signal(&signal) parameter does not exist on cancelled_exit(host, state, checkpoint_id). Won't compile. Fix: use cancelled_exit_with_reason(host, fallback_state, cancelled_reason_from_signal(&signal), None).
• 🟡 Same arm: pending_input_ack not acked before returning CancelCheck::Exit on the checkpoint-failure fallback. Helper takes &mut pending_input_ack explicitly to ack on success but the fallback branch skips the ack call. Inputs silently unacknowledged when checkpoint fails under cancel. Fix: call pending_input_ack.ack(host).await? before constructing the return (mirror the Ok branch).
• 🟡 Third Err(_) arm (CheckpointRejected): pending_input_ack also not acked on the failure exit path. Loop exits with LoopExit::failed holding unconsumed ack tokens. Fix: ack before returning, or document why skipping on hard failure is intentional.

[henrypark133]

Tracking from #3684 review (out of scope for that PR, in scope here):

• 🔴 `crates/ironclaw_agent_loop/src/executor.rs` cancel-path checkpoint failure handling: the `Err(_)` arm swallows non-checkpoint errors (e.g. `AgentLoopExecutorError::HostUnavailable`) when `require_final_checkpoint = false`, producing a checkpoint-free `Cancelled` exit instead of propagating. Fix: match on `Err(AgentLoopExecutorError::CheckpointFailed { .. })` specifically; re-return other variants. Applies to both `checkpoint_and_exit_if_cancelled` (inherited from arch(ws-13): add host cancellation accessor #3648) and the new `checkpoint_and_exit_if_cancelled_after_pending_input_ack` helper added by this PR.
• 🟡 `cancelled_after_checkpoint` hardcodes `LoopCancelledReasonKind::HostCancellation`. Currently OK since all reasons coarsen to `HostCancellation`, but as `LoopCancelledReasonKind` expands the divergence between `cancelled_exit` and `cancelled_exit_with_reason` paths grows. Either unify or annotate intent.

[henrypark133]

Code Review — fix(ws-13): ack drained inputs before cancel exit (re-review on amended commit fe7c78c8c)

Overview

Introduces checkpoint_and_exit_if_cancelled_after_pending_input_ack — an ack-aware variant of checkpoint_and_exit_if_cancelled — used at every cooperative cancellation boundary that follows a drain_steering / drain_followup step. On checkpoint success the helper flushes pending_input_ack before constructing the Cancelled exit so input cursors that were durably observed in the same iteration as the cancel are acknowledged. Five call sites converted from the original helper. New regression tests cover steering-drain, followup-drain, and the strict-profile checkpoint failure path. The capability-cancel path (cancelled_after_checkpoint) gains a documentation comment explaining the hardcoded HostCancellation reason.

Amended commit addresses all blocker + tracked findings from prior review rounds (PR #3684 review + henrypark caveman/blocker review on #3686 itself).

Correctness

• ✅ Strict-profile Err propagation fix (BLOCKER): previous version returned Ok(CancelCheck::Exit(LoopExit::failed(CheckpointRejected, ...))) which silently masked the failure. Amended commit returns Err(error) — mirrors the original checkpoint_and_exit_if_cancelled, preserves the require-final-checkpoint contract, lets the runner apply recovery policy.
• ✅ CheckpointFailed-specific permissive arm: both helpers now match Err(AgentLoopExecutorError::CheckpointFailed { .. }) instead of Err(_) for the permissive !require_final_checkpoint arm. HostUnavailable / other variants propagate up. Defensive against future checkpoint() evolution.
• ✅ Permissive ack hold: pending ack is intentionally NOT flushed on permissive checkpoint failure (no durable checkpoint → can't safely advance input cursor). Inline comment documents the invariant. Matches the prior review note confirming this is correct.
• ✅ Ack flush on Ok: pending ack flushes immediately after checkpoint(Final) succeeds, before constructing the LoopExit::Cancelled — input cursor advances only when the receiver has a durable record.
• ✅ cancelled_after_checkpoint documentation: 6-line comment captures why HostCancellation is hardcoded there (no LoopCancellationSignal in scope for capability-cancelled path) and what must change if LoopCancelledReasonKind gains finer variants. Resolves the tracked 🟡 finding.
• ✅ Five drain-boundary call sites consistently converted; `git diff` shows mechanical replacement, no missed sites.

Style / Conventions

• ✅ Inline comments explain why (permissive vs strict semantics, ack hold rationale, propagation contract). Per .claude/rules/skills.md, comments document non-obvious invariants only.
• ✅ Match arms ordered Ok → permissive → strict, mirroring the original helper.
• ✅ No .unwrap()/.expect() in production paths. Tests use .expect(\"lock\") on Mutex (allowed).
• ✅ Strong-typed: matches on AgentLoopExecutorError::CheckpointFailed { .. } variant, not string error inspection. Conforms to .claude/rules/types.md.
• ✅ No pub use re-exports added. Helper kept private to module.

Test Coverage

• ✅ New test cancellation_after_pending_input_ack_strict_profile_propagates_checkpoint_error: locks in the blocker fix — strict profile + Final checkpoint failure must propagate CheckpointFailed, not Ok. Test failed before the fix, passes after.
• ✅ Existing cancellation_after_steering_drain_flushes_pending_input_ack and cancellation_after_followup_drain_flushes_pending_input_ack validate ack-flush on success path.
• ✅ Sibling test cancellation_checkpoint_failure_propagates_executor_error_for_strict_profile (preexisting) exercises the original helper; the new test mirrors it through the new helper.
• ✅ Sibling test cancellation_checkpoint_failure_still_cancels_for_permissive_profile covers permissive checkpoint-failure exit.
• ✅ All 10 cancellation_* tests pass. Workspace cargo clippy --all --tests --examples --all-features -- -D warnings clean. cargo fmt --check clean.
• ❌ No test exercises the permissive arm of the new helper (!require_final_checkpoint + fail_checkpoint(Final) + pending ack). The existing permissive test goes through the original helper; the new helper's permissive arm — including the comment-documented ack-hold behavior — is covered only structurally. Minor: behavior matches the existing test's contract.
• ❌ No test for HostUnavailable propagation through the new permissive arm. The CheckpointFailed-specific match is defensive; in practice checkpoint() only emits CheckpointFailed. Lower priority.

Performance

• One additional pending_input_ack.ack(host).await per cancellation boundary on the success path. Cancellation is rare; cost is irrelevant. No change to the hot loop path (Continue branch is unaffected).
• state.clone() for fallback_state was already present in the original helper; new helper mirrors the pattern.

Security

• ✅ No new boundary surface. All changes internal to ironclaw_agent_loop.
• ✅ No raw secrets, no path leakage, no error-message exposure.
• ✅ Ack tokens are typed (LoopInputAckToken) end-to-end.

Recommendation

APPROVE. The previous blocker (strict-profile divergence) is resolved with a focused fix and locked in by a new regression test. Both 🔴 findings from the #3684-tracked review are addressed (CheckpointFailed-specific match in both helpers). 🟡 reason-kind divergence is annotated with intent at the call site. Two coverage suggestions (permissive arm via new helper, HostUnavailable propagation) are nice-to-haves, not merge blockers.

[henrypark133]

Independent Sonnet Re-Review — fix(ws-13): ack drained inputs before cancel exit

Reviewed at commit fe7c78c8. Base: reborn-integration (post-#3685).

---

Overview

This PR closes the input-cursor acknowledgement gap that existed when a cooperative cancellation boundary fired after a drain (steering or follow-up) had populated pending_input_ack but before the BeforeModel checkpoint flushed it. The fix introduces checkpoint_and_exit_if_cancelled_after_pending_input_ack, replaces the five pre-BeforeModel cancel-boundary call sites with it, and adds three targeted regression tests.

The logic is sound and the core invariant — "do not advance the input cursor unless a durable checkpoint backs it" — is correctly encoded. No blocking issues found.

---

Correctness

🟢 Ack-after-checkpoint ordering is right

In the happy-cancel path, checkpoint() completes before pending_input_ack.ack() is called. If ack() subsequently fails with HostUnavailable, the runner holds a committed checkpoint but the cursor hasn't advanced — the runner can retry from checkpoint and re-drain. This is the correct failure direction.

🟢 No-ack-on-permissive-checkpoint-failure is justified and commented

The permissive-profile branch deliberately skips flushing the ack when no durable checkpoint was written. The inline comment explains the reasoning. Consistent with checkpoint_and_exit_if_cancelled.

🟢 Strict-profile error propagation matches original helper

Err(error) => Err(error) in the final arm is structurally identical to the original helper. The doc comment in the cancelled_after_checkpoint helper was also updated in this PR to call out the coarsening of LoopCancelledReasonKind and the WS16 migration path — that annotation was missing before.

🟡 Five call sites replaced — but scope is narrower than the boundary count implies

Only the five call sites between steering/follow-up drain and the BeforeModel checkpoint were replaced. Call sites after BeforeModel (lines 278, 285, 317, 334, 345, 352, 372, 402, 419, 430 in the PR head, all using checkpoint_and_exit_if_cancelled) are correct because pending_input_ack.ack() has already fired at line 277. This is fine — but it's worth noting that the helper name suggests a general replacement; a reader might wonder why post-BeforeModel sites still use the original. A brief comment at the BeforeModel flush point (line 277) noting "ack flushed here; downstream cancel-checks use the base helper" would close that reasoning gap. Not a blocker.

🟡 cancel_after_poll_inputs fires after the batch is returned, not before

In poll_inputs, the mock calls request_cancellation after it returns Ok(batch). This means observe_cancellation returns Some on the next call, which is the cancel-boundary check immediately after the drain. That matches the test's intent, but the trigger-point comment in the mock is absent — a reader skimming poll_inputs has to mentally trace when the signal becomes visible. Adding a // cancellation signal becomes visible on the next observe_cancellation() comment to the mock would prevent mis-reading it as a pre-return race simulation. Nit-level for test code.

🟢 PendingInputAck::replace double-advance guard still holds

replace returns PlannerContract if tokens are pending when new tokens arrive. The new helper doesn't touch replace — it reads and flushes the existing pending set. No regression path here.

---

Style / Conventions

🟢 Matches CLAUDE.md error-handling rules

No .unwrap_or_default(), no silent-fallback on the ack() error. HostUnavailable propagates up correctly.

🟢 No stringly-typed internals introduced

All new types and enums are existing typed values. cancel_after_poll_inputs is a bool behind a Mutex — appropriate for test infrastructure.

🟡 Helper name is long but unambiguous

checkpoint_and_exit_if_cancelled_after_pending_input_ack is 55 characters. It's accurate and the awkwardness is acceptable given that the shorter name is already taken by the original helper. No suggestion to shorten — a terse alias would obscure the semantics.

---

Test Coverage

🟢 cancellation_after_steering_drain_flushes_pending_input_ack

Covers the primary fix path. The events() assertion (["checkpoint:final", "ack_inputs"]) pins both that a checkpoint was written and that ack was flushed in the right order. Solid.

🟢 cancellation_after_pending_input_ack_strict_profile_propagates_checkpoint_error

Covers the critical strict-profile path: CheckpointFailed propagates rather than Ok(LoopExit::failed). This is the silent-masking regression the PR comment calls out. Correctly uses expect_err and asserts the exact error variant.

🟢 cancellation_after_followup_drain_flushes_pending_input_ack

Covers the follow-up-drain path with family_with_drain(false, true). The events() order (["checkpoint:before_model", "checkpoint:final", "ack_inputs"]) confirms that the BeforeModel checkpoint fires before the drain, and the Final checkpoint + ack fire during the cancel-boundary check after drain. The FixedDrainStrategy helper is clean and reusable.

🟠 No permissive-profile test for the new helper's no-ack branch

The existing cancellation_checkpoint_failure_still_cancels_for_permissive_profile test covers the original helper's permissive path. There is no equivalent test that combines cancel_after_poll_inputs + fail_checkpoint(Final) + permissive profile to verify that the ack is not flushed when the checkpoint fails. The comment in the helper says "intentionally NOT flushed here" — that invariant should be covered by a test. Without it, a future refactor could silently start flushing the ack on permissive-profile checkpoint failure and no test would catch it. This is high rather than blocking because the strict-profile test and the happy-path test already constrain the boundaries, but the missing negative assertion is a real gap.

🟡 No test for HostUnavailable propagation from ack() inside the new helper

If ack_inputs returns an error on the host, the helper propagates HostUnavailable { stage: Input }. There is no test that verifies the checkpoint is already written (and thus safe to retry from) when the ack fails. The existing happy-path tests would catch a regression where ack is called before checkpoint, but a dedicated test would make the ordering contract explicit. Medium — the ordering is clear in the code.

---

Performance

🟢 No allocation hot paths changed. fallback_state = state.clone() is conditional on cancellation being signalled, which is already the case in the original helper. No regression.

---

Security

🟢 No new credential handling, no new external I/O surface, no new unsafe code.

🟢 The no-ack-on-failed-checkpoint invariant is security-relevant in the sense that it prevents a partial-progress acknowledgement from advancing the input cursor without a durable record. The PR correctly preserves that invariant in both the strict and permissive branches.

---

Recommendation

Approve with one follow-up ask.

The code is correct and the key invariants are properly tested. The one gap worth tracking is the missing permissive-profile + pending-ack + checkpoint-failure test (🟠 above). It can ship as a follow-up issue or in the next WS-13 slice rather than blocking this PR — the strict-profile test already exercises the critical path, and the permissive-profile behavior is covered for the original helper.

All other findings are nits or documentation suggestions.