refactor: consolidatate cancellation

[bobrykov]

No description provided.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: a19b257c-1ff8-4554-92f9-9bc9c7592437

📥 Commits

Reviewing files that changed from the base of the PR and between 18e76c7 and 135b2e9.

📒 Files selected for processing (3)

• src/engine/bare.rs
• src/engine/bare/dispatch.rs
• src/engine/bare/stream.rs

💤 Files with no reviewable changes (1)

• src/engine/bare/dispatch.rs

🚧 Files skipped from review as they are similar to previous changes (2)

• src/engine/bare/stream.rs
• src/engine/bare.rs

---

📝 Walkthrough

Walkthrough

This PR centralizes BareLoop cancellation in process_turn, removes lower-level cancellation races from streaming and tool dispatch, and refactors compaction and session lifecycle hook handling. It also adds cancellation tests and updates module documentation.

Changes

BareLoop execution and lifecycle refactor

Layer / File(s)	Summary
Turn-body extraction and centralized cancellation src/engine/bare.rs	Turn execution moves into run_turn_body and is raced against cancellation in process_turn; failure-state handling and cancellation tests are updated.
Streaming cancellation delegation src/engine/bare/stream.rs	Inline streaming awaits events directly, leaving turn-level cancellation handling to process_turn while preserving StreamHandler cancellation behavior.
Sequential tool dispatch and recovery flow src/engine/bare/dispatch.rs	Tool, pipeline, and retry paths no longer race cancellation; observer notifications, corrections, hooks, and health handling are centralized.
Compaction hook flow src/engine/bare/compact.rs	Compaction uses dedicated pre/post hook helpers, clones the conversation for context fitting, and reports token and duration metrics.
Session lifecycle hook flow src/engine/bare/emission.rs	Session hook dispatch is extracted into helpers, and cancellation takes priority when determining the session end reason.
Module documentation updates src/compact.rs, src/engine/bare/message.rs	Compaction and message-helper documentation is reworded.

Sequence Diagram(s)

sequenceDiagram
  participant Caller
  participant process_turn
  participant run_turn_body
  participant CancelSignal
  Caller->>process_turn: invoke turn
  process_turn->>run_turn_body: run turn body
  process_turn->>CancelSignal: race turn completion with cancellation
  alt cancellation first
    CancelSignal-->>process_turn: notified
    process_turn->>Caller: on_turn_end(error="cancelled")
    process_turn-->>Caller: LoopError::Cancelled
  else turn completes
    run_turn_body-->>process_turn: result
    process_turn-->>Caller: return result
  end

Loading

sequenceDiagram
  participant dispatch_tools
  participant dispatch_tool_with_recovery
  participant Tool
  participant Reflector
  dispatch_tools->>dispatch_tool_with_recovery: dispatch sequentially
  dispatch_tool_with_recovery->>Tool: invoke tool
  Tool-->>dispatch_tool_with_recovery: result or error
  dispatch_tool_with_recovery->>Reflector: recover tool error
  Reflector-->>dispatch_tool_with_recovery: action and correction
  dispatch_tool_with_recovery->>dispatch_tool_with_recovery: fire tool post observer

Loading

Possibly related PRs

• dch-labs/loopctl#19: Introduces the cancellation primitives used by the BareLoop flow refactored here.
• dch-labs/loopctl#22: Provides the context-fitting API consumed by the compaction refactor.
• dch-labs/loopctl#43: Also changes LoopState::Cancelled handling across BareLoop execution paths.

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title matches the main change: refactoring and consolidating cancellation handling across the loop.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 50.00%.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch refactor/cancellation-logic

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

src/engine/bare/stream.rs (1)

44-69: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Stale # Errors doc after removing the inline cancellation race.

With Line 69 now awaiting stream.next() directly, the inline path no longer returns LoopError::Cancelled mid-stream — only the handler path (stream_turn_via_handler) can. Line 44 ("Returns LoopError::Cancelled if the cancellation signal fires mid-stream") overstates this and should be scoped to the handler path or clarified that cancellation is now handled by process_turn's select!.

🤖 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 `@src/engine/bare/stream.rs` around lines 44 - 69, The `stream_turn` doc
comment is stale because the inline path no longer returns
`LoopError::Cancelled` mid-stream after switching to `stream.next().await`
directly. Update the `stream_turn` documentation to reflect that cancellation is
now handled by `process_turn`’s `select!`, and only `stream_turn_via_handler`
can surface mid-stream cancellation; keep the wording scoped to the
`stream_turn`/`stream_turn_via_handler` behavior so the `# Errors` section
matches the current control flow.

🧹 Nitpick comments (2)

src/engine/bare/dispatch.rs (1)

449-452: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Duplicated doc line. The first sentence of dispatch_via_pipeline's rustdoc is repeated verbatim (Lines 449 and 451).

✏️ Proposed fix

     /// Dispatch a tool call through the middleware pipeline.
     ///
-    /// Dispatch a tool call through the middleware pipeline.
-    ///
     /// Builds a [`ToolDispatchContext`] and delegates to the pipeline's

🤖 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 `@src/engine/bare/dispatch.rs` around lines 449 - 452, The rustdoc for
dispatch_via_pipeline contains a duplicated opening sentence, so remove the
repeated line and leave a single clear description for the function. Update the
doc comment block in dispatch_via_pipeline so it only states the purpose once,
keeping the rest of the documentation unchanged.

src/engine/bare.rs (1)

3499-3678: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

The cancellation tests are a good addition, but note process_turn_cancel_during_dispatch_fires_turn_end only asserts turn_ends == 1 and never inspects the resulting SessionResult. A regression like the self.budget loss flagged at Lines 1209–1226 would pass silently. Consider asserting the reported budget (e.g. total_turns/token counts) after cancel-during-dispatch.

🤖 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 `@src/engine/bare.rs` around lines 3499 - 3678, Add an assertion in
process_turn_cancel_during_dispatch_fires_turn_end that inspects the returned
SessionResult from BareLoop::run, not just the turn_ends observer count. The
current test can miss regressions like the self.budget loss, so capture the
result from agent.run("Use slow tool").await and verify the reported budget
fields (such as total_turns and token counts) are still correct after
cancellation during dispatch. Keep the existing cancel and turn_end checks, but
extend the test to validate SessionResult preserves budget state.

🤖 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.

Outside diff comments:
In `@src/engine/bare/stream.rs`:
- Around line 44-69: The `stream_turn` doc comment is stale because the inline
path no longer returns `LoopError::Cancelled` mid-stream after switching to
`stream.next().await` directly. Update the `stream_turn` documentation to
reflect that cancellation is now handled by `process_turn`’s `select!`, and only
`stream_turn_via_handler` can surface mid-stream cancellation; keep the wording
scoped to the `stream_turn`/`stream_turn_via_handler` behavior so the `# Errors`
section matches the current control flow.

---

Nitpick comments:
In `@src/engine/bare.rs`:
- Around line 3499-3678: Add an assertion in
process_turn_cancel_during_dispatch_fires_turn_end that inspects the returned
SessionResult from BareLoop::run, not just the turn_ends observer count. The
current test can miss regressions like the self.budget loss, so capture the
result from agent.run("Use slow tool").await and verify the reported budget
fields (such as total_turns and token counts) are still correct after
cancellation during dispatch. Keep the existing cancel and turn_end checks, but
extend the test to validate SessionResult preserves budget state.

In `@src/engine/bare/dispatch.rs`:
- Around line 449-452: The rustdoc for dispatch_via_pipeline contains a
duplicated opening sentence, so remove the repeated line and leave a single
clear description for the function. Update the doc comment block in
dispatch_via_pipeline so it only states the purpose once, keeping the rest of
the documentation unchanged.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: efc93d92-9efa-455d-aa9e-5e7e0b71f24d

📥 Commits

Reviewing files that changed from the base of the PR and between 1c109db and 18e76c7.

📒 Files selected for processing (7)

• src/compact.rs
• src/engine/bare.rs
• src/engine/bare/compact.rs
• src/engine/bare/dispatch.rs
• src/engine/bare/emission.rs
• src/engine/bare/message.rs
• src/engine/bare/stream.rs