Files

T

Tianyi Cui c6ed980d6f fix review findings: stale abort() docs + move RFC to implemented

Codex's no-ship was a completeness/docs-sync gap, not loop behavior:

- docs/architecture.md: drop the public abort() handle row; the teardown
  signal is now cancel() then await whenIdle().
- cancel.spec.ts: the module doc and the turn-start comment contrasted
  cancel() against a public abort() verb that no longer exists — reword to
  name the loop's private step AbortController.
- packages/ui/acp/src/index.ts: the post-resume-leak comment cited abort();
  cancel() is the surviving stop verb that likewise does not unregister.
- Move the RFC proposed -> implemented/simplification with amended text:
  Status flips, the both-removal proposal is narrowed to abort-only, and an
  implementation note records why whenIdle() is retained (load-bearing
  quiescence primitive with live ACP consumers). Update docs/rfc/README.md.
- AGENTS.md "RFCs are proposals, not golden truth": add the concrete
  abort/whenIdle worked example now that the implemented RFC exists to link.
- Regenerate the cordis catalog (line-number drift from the rebase).

2026-06-21 09:10:56 +08:00

4.9 KiB

Raw Blame History

RFC: Keep one public stop primitive

Status: implemented (proposed 2026-06-20; accepted in amended form — whenIdle() retained)

Implementation note (scope narrowed from the original proposal). This RFC proposed removing BOTH abort() and whenIdle() from the public Agent handle. Only abort() was removed. Validating the premise against the code (AGENTS.md "RFCs are proposals, not golden truth") found whenIdle() to be a load-bearing quiescence primitive, not dead surface: it is the settle signal in several ACP tests (packages/ui/acp/tests/{edges,turns,dispose}.spec.ts) and is backed by a deliberate loop contract (settle waiters without a status transition; handle the replacement-turn race). The RFC's suggested migration — have consumers observe the running→idle transition by hand — is exactly the brittle hand-rolled path AGENTS.md § Defensive patterns warns against ("Async state is not synchronous state"). Deleting a clean primitive to push every consumer onto that is a net loss, so whenIdle() stays. abort() was genuinely dead public surface (no production caller; the loop aborts its own AbortController directly), so it was removed as proposed. The text below is amended to describe what shipped.

Problem

The public Agent handle exposed two overlapping ways to stop in-flight work: abort(reason?) and cancel(reason?). abort() killed only the in-flight step and left queued work alone; cancel() clears queued and steering work, aborts the running step, and handles the pre-step race. In production, ACP uses cancel() for session/cancel, while lifecycle owners tear down agents through AgentHandle.dispose(). No production caller needed bare abort().

The abort()/cancel() distinction is real — abort() preserves queued prompts and steering while cancel() drops them — but no shipping code called the public abort() verb. The loop's own stop paths (cancel() and disposal) abort the current AbortController directly rather than routing through Agent.abort(). Most tests that called abort() interrupt an empty queue and switch to cancel(reason); the steering re-delivery test that deliberately depends on queue preservation drives the in-flight AbortController directly, because cancel() would drop the queued steering it is trying to prove survives a step abort. The no-argument abort() default reason ('aborted') is deleted with the verb rather than preserved by accident; cancel() keeps its own 'cancelled' default.

The extra surface area made the loop carry a public verb that is mostly a teardown internal: abort() had to be documented as distinct from queue-aware cancellation even though a UI cancellation almost always wants the broader operation.

Proposal

Keep cancel() as the only public stop primitive on Agent. Lifecycle owners use AgentHandle.dispose() to stop and unregister an agent; non-owners use cancel() to abandon current and queued work. The implementation keeps a private abort controller, but it is not part of the plugin-facing Agent contract.

whenIdle() is retained as the public quiescence-observation primitive (resolve once the agent settles out of running, resolve immediately when already idle, await the loop exit when disposed). It is not a stop verb; it is how a non-owner observes the stop completing without disposing the agent, and it has live consumers (the ACP bridge's settle points).

Delete public abort(), the tests that exercise it as standalone API, and the docs that describe step-only abort as an embedding feature. Empty-queue abort tests migrate to cancel(reason) where they still prove cancellation behavior; tests whose subject is the loop's internal AbortController behavior drive that controller directly via an in-package typed cast to the private field; tests that only pin the removed no-arg abort() default go away with the method. The disposer remains async and still waits for the loop to stop.

Acceptance criteria

Agent exposes no public abort(); cancel(), whenIdle(), and steer() remain part of the surface.
ACP cancellation continues to call cancel().
Agent teardown continues to await quiescence through handle disposal, and whenIdle() still resolves on quiescence for non-owner observers.
Tests cover cancellation and disposal as the two supported stop paths.

What we give up

A future plugin cannot abort only the current model/tool step while preserving queued prompts through the public interface. If that use case becomes real, it should return with a named consumer and a narrower contract. Today it is latent generality that keeps a private loop mechanic public.

This RFC only removes the redundant stop verb. Mid-turn steering remains an intentional message path; quiescence observation remains via whenIdle(). The resulting public surface is send(), steer(), inject(), cancel(), whenIdle(), status, options, session, and identity.

4.9 KiB Raw Blame History