# RFD 006: Turn-Scoped Stream Mutations

- **Status**: Implemented
- **Category**: Design
- **Authors**: Jean Mertz <git@jeanmertz.com>
- **Date**: 2025-07-18
- **Summary**: Restricts `ConversationStream` mutations to turn-scoped accessors
  so invalid event sequences cannot be constructed through the public API.

## Summary

This RFD restricts `ConversationStream` mutations to turn-scoped accessors
(`start_turn` and `current_turn_mut`) so callers cannot construct a stream
without valid turn boundaries through the public API.

## Motivation

`ConversationStream` currently exposes raw `push`, `add_chat_request`,
`add_chat_response`, and similar methods that allow callers to add events in any
order.
Nothing prevents creating a stream without a `TurnStart`, adding a
`ChatResponse` before a `ChatRequest`, or pushing events outside a turn
boundary.

This has caused real bugs:

- Fork with `--from`/`--until` can produce streams that start with assistant
  responses, causing provider API errors (Anthropic rejects streams where the
  first message isn't from the user).
  (Fork's range flags are now `--from`/`--to` and resolve to whole turns, so
  this particular shape no longer arises from range filtering; the invariant the
  builder API enforces still matters for every other caller.)
- Streams without `TurnStart` markers break `--last` (which counts turn
  boundaries).
- `sanitize()` exists specifically to repair these invariant violations after
  the fact.

We added `sanitize()` as a caller-side guard, but it's a patch — callers must
remember to call it, and the raw methods remain available for anyone to misuse.

## Design

### Public API

Two new entry points on `ConversationStream` replace direct event pushing:

```rust
// Start a new turn. Atomically adds TurnStart + ChatRequest.
stream.start_turn(chat_request);

// Get a mutable handle to the current (last) turn.
// If no turn exists, one is injected automatically.
let turn = stream.current_turn_mut();
turn.add_chat_response(response)
    .add_tool_call_request(req)
    .build()?;
```

`current_turn_mut()` is infallible — if the stream has no turns yet, it injects
an empty `TurnStart` and returns a handle to it.
This avoids forcing every call site to handle a `None`/`Err` for a case that
shouldn't happen in practice.

### `TurnMut<'_>`

`TurnMut` wraps `&mut ConversationStream` and buffers events internally.
Events are validated, sanitized, and flushed to the stream when `build()` is
called.
This keeps the stream in a consistent state at all times — partial or invalid
events never appear on the stream.

The turn loop works naturally with this model because `TurnMut` is short-lived:
grab a handle, add events, `build()`, release.
Code that reads the stream (e.g., `build_thread` cloning the stream for each LLM
cycle) always runs between `build()` calls, never while a `TurnMut` is held.

Two method styles for ergonomics:

- **`with_xxx(&mut self, x: X) -> &mut TurnMut`** — borrowed, for chaining on
  an existing binding.
- **`add_xxx(mut self, x: X) -> TurnMut`** — owned, for fluent builder chains.
- **`build(self) -> Result<()>`** — validates the buffered events, sanitizes
  them, flushes to the stream, and releases the borrow.

All methods are `#[must_use]` to prevent silently dropping a `TurnMut` without
calling `build()`.
There is no custom `Drop` implementation.

Exposes:

- `add_chat_request` / `with_chat_request` — for interrupt replies within a
  turn
- `add_chat_response` / `with_chat_response`
- `add_tool_call_request` / `with_tool_call_request`
- `add_tool_call_response` / `with_tool_call_response`
- `add_inquiry_request` / `with_inquiry_request`
- `add_inquiry_response` / `with_inquiry_response`
- `build` — validate and commit

Does **not** expose:

- `add_turn_start` — only `start_turn()` can create turn boundaries
- `push` — raw event insertion is internal-only

### Visibility changes

| Method                 | Current   | Proposed          |
| ---------------------- | --------- | ----------------- |
| `start_turn`           | N/A (new) | `pub`             |
| `current_turn_mut`     | N/A (new) | `pub`             |
| `push`                 | `pub`     | `pub(crate)`      |
| `add_chat_request`     | `pub`     | `pub(crate)`      |
| `add_chat_response`    | `pub`     | `pub(crate)`      |
| `add_turn_start`       | `pub`     | `pub(crate)`      |
| `add_tool_call_*`      | `pub`     | `pub(crate)`      |
| `add_inquiry_*`        | `pub`     | `pub(crate)`      |
| `sanitize`             | `pub`     | `pub` (unchanged) |
| `retain`, `iter`, etc. | `pub`     | `pub` (unchanged) |

Internal code (`sanitize`, `trim_trailing_empty_turn`) continues to access
`self.events` directly.
The builder guards the public API, not internal repair logic.

### Caller migration

**`TurnCoordinator::start_turn`** — currently calls `add_turn_start` +
`add_chat_request`.
Becomes `stream.start_turn(request)`.

**`TurnCoordinator` event handlers** — currently call
`stream.add_chat_response`, `stream.add_tool_call_response`, etc. Become
`stream.current_turn_mut().with_chat_response(...).build()?`.
The `TurnMut` is grabbed and consumed within each synchronous block, avoiding
borrow conflicts across async boundaries.

**`InterruptAction::Reply`** — adds a `ChatRequest` mid-turn via
`stream.add_chat_request(...)`.
Becomes `stream.current_turn_mut().with_chat_request(...).build()?`.
This is valid — a reply within a turn is a `ChatRequest` that doesn't start a
new turn.

**Fork** — uses `extend` and `retain` on the stream.
These are internal operations that don't go through the turn API.
No change needed.

**Tests** — `push` and `add_*` methods remain accessible as `pub(crate)` within
`jp_conversation`.
Tests in other crates can use `start_turn` + `current_turn_mut`, or a test
helper.

## Drawbacks

- **Moderate refactor scope.** Every caller of `add_*` methods in `jp_cli` needs
  updating.
  The turn loop and coordinator are the main touchpoints.
- **Test verbosity.** Constructing test streams requires going through
  `start_turn` + `current_turn_mut` instead of raw `push`.
  Mitigated by keeping `push` as `pub(crate)`.

## Alternatives

**Do nothing.** Keep `sanitize()` as the guard.
This works today but relies on callers remembering to sanitize.
The gap between "what the API allows" and "what produces a valid stream"
remains.

**Make `push` validate invariants.** Each `push` call checks the stream state
and rejects invalid sequences.
Rejected because it moves runtime checks into a hot path and makes the error
handling awkward — `push` would need to return `Result`, changing every call
site anyway.

**Builder that buffers a complete turn.** `start_turn` returns a builder that
collects events and flushes on drop.
Rejected because the turn loop needs to read intermediate stream state during a
turn (e.g., `build_thread` clones the stream mid-turn for each LLM cycle).
A buffering builder would hide in-progress events.

## Non-Goals

- Enforcing event ordering *within* a turn (e.g., ChatResponse must follow
  ChatRequest).
  The turn loop's state machine handles this; the stream doesn't need to
  duplicate that logic.
- Removing `sanitize()`.
  It remains necessary for repairing streams loaded from disk or produced by
  fork filtering.

## Risks and Open Questions

- **What should `build()` validate?** At minimum: the turn contains at least one
  `ChatRequest`.
  Should it also check for orphaned tool call pairs, or leave that to
  `sanitize()`?
- **Should `start_turn` take ownership of `ChatRequest` or accept `impl
  Into<ChatRequest>`?** The current `add_chat_request` uses `impl
  Into<ChatRequest>`.
  Consistency suggests the same pattern.
- **Config delta handling.** `push_with_config_delta` currently exists as a
  public method.
  It should likely move to `TurnMut` or become `pub(crate)`.
- **Forgetting to call `build`.** `#[must_use]` on `TurnMut` warns at compile
  time if the handle is dropped without calling `build()`.
  But it's a warning, not an error.
  Is this sufficient, or do we need a lint-level enforcement?

## Implementation Plan

### Phase 1: Add the new API alongside the old one

Add `start_turn` and `current_turn_mut` / `TurnMut` to `ConversationStream`.
Keep existing `add_*` methods as `pub`.
Migrate `TurnCoordinator` to use the new API.
This can be merged independently.

### Phase 2: Migrate remaining callers

Update the turn loop, interrupt handling, and tool coordinator to use
`current_turn_mut`.
Update tests.
This is mechanical but touches many files.

### Phase 3: Restrict visibility

Change `add_*` and `push` to `pub(crate)`.
This is the breaking change that enforces the invariant.
Any external code still using the old API will fail to compile.

## References

- `crates/jp_conversation/src/stream.rs` — `ConversationStream`
- `crates/jp_cli/src/cmd/query/turn/coordinator.rs` — `TurnCoordinator`
- `crates/jp_cli/src/cmd/query/turn_loop.rs` — turn loop
- `crates/jp_conversation/src/event/turn.rs` — `TurnStart`