Bläddra i källkod

fix(agent): carry cancel(reason) through the marker-only windows (review)

A reviewer found that `cancel(reason)` only preserved the caller's reason when
an active AbortController observed it (the mid-step path, via
`abort.signal.reason`). The marker-only windows (step-start at loop.ts and the
continuation gate) hardcoded `reason: 'cancelled'`, so the logged `turn/end`
reason was race-dependent on WHERE the cancel landed and the public
`cancel(reason?)` parameter was half-effective.

Capture the resolved reason (`reason ?? 'cancelled'`) on the agent when the
marker is armed, expose it on the LoopHandle as `cancelReason()`, and use it in
both marker branches so a turn dropped without a live controller records the
SAME `{kind:'aborted', reason}` the mid-step path produces.

The two existing window tests asserted `reason: 'cancelled'` while passing
`'from turn-start'` / `'from continuation'` — they documented the bug. Updated
both to assert the caller's reason (behavior + test changed together, per
AGENTS.md "tests document behavior, not golden truth").

Also fixes two stale docs the PR's contract change left behind: the
module-level ACP mapping comment and `codec.ts` both still said `session/cancel
-> agent.abort()`.
Tianyi Cui 3 månader sedan
förälder
incheckning
6a1e381e38

+ 1 - 1
packages/acp/src/codec.ts

@@ -26,7 +26,7 @@ import type { ContentBlock as AcpContentBlock, StopReason } from '@agentclientpr
  *
  * - `completed` → `end_turn` (the model chose to stop)
  * - `max-tokens` → `max_tokens` (cut off at the output-token ceiling)
- * - `aborted`   → `cancelled` (an `agent.abort()`, e.g. from `session/cancel`)
+ * - `aborted`   → `cancelled` (a step abort or a queue-aware `agent.cancel()`, e.g. from `session/cancel`)
  * - `error`     → `end_turn` (defensive fallback only: the bridge REJECTS the
  *                 `session/prompt` RPC on an error turn BEFORE calling this, so
  *                 a client sees a JSON-RPC error, not a stop reason — see

+ 3 - 1
packages/acp/src/index.ts

@@ -13,7 +13,9 @@
  * - `session/load`   → `ctx.agents.resume(...)` then replay the event log
  * - `session/prompt` → `agent.send()`, settle on the owning turn's end (a turn
  *                      that ends in `error` rejects the RPC)
- * - `session/cancel` → `agent.abort()` + settle the in-flight prompt
+ * - `session/cancel` → `agent.cancel()` (the queue-aware cancel: aborts a
+ *                      running step, clears queued + steering work, and drops a
+ *                      turn about to start) + settle the in-flight prompt
  *
  * Multi-session (RFC 011): N concurrent sessions per connection, each mapped to
  * its own `ReactLoopAgent`. Sessions are keyed by id in `sessions` (forward) with an

+ 16 - 0
packages/agent-loop/src/agent.ts

@@ -34,6 +34,17 @@ export class ReactLoopAgent implements Agent {
    * leave it set to wrongly drop a later prompt.
    */
   private cancelRequested = false
+  /**
+   * The resolved reason for the pending {@link cancel} (`reason ?? 'cancelled'`),
+   * read by the driver loop's marker branches so a turn dropped in a
+   * marker-only window (pre-step / continuation, where no `AbortController`
+   * carries the reason) ends with the SAME `{kind:'aborted', reason}` the
+   * mid-step abort path produces from `abort.signal.reason`. Without this the
+   * caller's `cancel(reason)` would be silently replaced by the literal
+   * 'cancelled' whenever the cancel landed outside a running step — making the
+   * logged reason race-dependent and the public `reason?` param half-effective.
+   */
+  private cancelReason = 'cancelled'
   private disposed: Promise<void>
   private resolveDisposed!: () => void
   /** Resolves when the driver loop has fully exited (tests/disposal). */
@@ -196,6 +207,10 @@ export class ReactLoopAgent implements Agent {
     // precisely to cover it.
     if (this._status === 'running' || this.currentAbort !== undefined || this.inbox.hasQueued || this.inbox.hasSteering) {
       this.cancelRequested = true
+      // Capture the resolved reason for the marker-only windows (pre-step /
+      // continuation). The mid-step path reads it from abort.signal.reason
+      // below; the marker path reads it via the LoopHandle's cancelReason().
+      this.cancelReason = reason ?? 'cancelled'
     }
     // Drop all pending queued + steering work (un-started prompts never run; the
     // cancelled turn's steering is not re-enqueued). Cleared directly even when
@@ -251,6 +266,7 @@ export class ReactLoopAgent implements Agent {
       disposed: this.disposed,
       isDisposed: () => this._status === 'disposed',
       isCancelled: () => this.cancelRequested,
+      cancelReason: () => this.cancelReason,
       clearCancel: () => { this.cancelRequested = false },
       // Settle whenIdle() waiters WITHOUT a status transition — the pre-step
       // cancel-skip path drops the about-to-run turn and re-parks without ever

+ 10 - 2
packages/agent-loop/src/loop.ts

@@ -116,6 +116,14 @@ export interface LoopHandle {
    * marker governs exactly one cancellation and never leaks to a later prompt.
    */
   isCancelled(): boolean
+  /**
+   * The resolved reason for the pending cancel (`reason ?? 'cancelled'`), read
+   * by the marker branches (pre-step / continuation) so a turn dropped where no
+   * `AbortController` carries the reason still records the caller's
+   * `cancel(reason)` value — matching the mid-step abort path. Only meaningful
+   * when {@link isCancelled} is true.
+   */
+  cancelReason(): string
   /** Clear the cancel marker (called once per iteration after the turn returns). */
   clearCancel(): void
   /**
@@ -396,7 +404,7 @@ async function runTurn(ctx: Context, agent: ReactLoopAgent, handle: LoopHandle,
       // already-appended step/start.
       if (handle.isCancelled()) {
         handle.setAbort(undefined)
-        reason = { kind: 'aborted', reason: 'cancelled' }
+        reason = { kind: 'aborted', reason: handle.cancelReason() }
         closeStep()
         break
       }
@@ -466,7 +474,7 @@ async function runTurn(ctx: Context, agent: ReactLoopAgent, handle: LoopHandle,
       // ends the turn here. cancel() also cleared the steering FIFO, so the
       // override above did not re-arm continuation.
       if (handle.isCancelled()) {
-        reason = { kind: 'aborted', reason: 'cancelled' }
+        reason = { kind: 'aborted', reason: handle.cancelReason() }
         break
       }
 

+ 7 - 4
packages/agent-loop/tests/cancel.spec.ts

@@ -186,9 +186,11 @@ describe('Agent.cancel()', () => {
     await waitForIdle(ctx, agent)
     dispose()
 
-    // No step streamed (the model never ran), and the turn ended aborted.
+    // No step streamed (the model never ran), and the turn ended aborted with
+    // the CALLER's reason — the marker carries `cancel(reason)` through even
+    // though no AbortController observed it in this window.
     expect(streamed).toBe(false)
-    expect(reasons).toEqual([{ kind: 'aborted', reason: 'cancelled' }])
+    expect(reasons).toEqual([{ kind: 'aborted', reason: 'from turn-start' }])
   })
 
   it('cancel during the continuation window ends the turn aborted and runs no further step', async () => {
@@ -219,9 +221,10 @@ describe('Agent.cancel()', () => {
     await waitForIdle(ctx, agent)
 
     // Only ONE step ran (the second was cancelled in the continuation window),
-    // and the turn ended aborted.
+    // and the turn ended aborted with the CALLER's reason (carried by the
+    // marker, since the finished step's AbortController was already cleared).
     expect(steps).toBe(1)
-    expect(reasons).toEqual([{ kind: 'aborted', reason: 'cancelled' }])
+    expect(reasons).toEqual([{ kind: 'aborted', reason: 'from continuation' }])
   })
 
   it('cancel from a synchronous agent/status(running) listener drops the turn (window 2)', async () => {