Skip to content

test(cli): ACP follow's stale-attach guard is untested — removing it leaves all 115 ACP tests passing #125

Closed

Description

@dezren39

Finding

PR #58 (merged as a99f15a) added a guard to follow in packages/cli/src/acp/turn.ts:408-410. Before stopping a session's follower (the fiber that keeps forwarding a session's asks to the ACP client between prompts), it checks that its attachment is still the current one. An older, overlapping session/load or session/resume of the same session therefore can't stop the newer attachment's follower.

Nothing tests the guard. With lines 409-410 deleted, every ACP test still passes:

# from packages/cli, with lines 409-410 of src/acp/turn.ts deleted
bun test test/acp --timeout 30000
 115 pass
 0 fail
Ran 115 tests across 15 files.

That ran on 2026-10-19 in a checkout whose packages/cli/src/acp and packages/cli/test/acp are identical to main 5214f4d; neither has changed since a99f15a. The related test "keeps following after a load replaces the attachment mid-prompt" (packages/cli/test/acp/turn.test.ts:447) covers the handoff after a prompt, not this path.

Without the guard, the stale call runs stopFollowing(id), which interrupts the newer attachment's follower. Its own sessions.fork then fails the identity check (packages/cli/src/acp/sessions.ts:187-190) and closes, so nothing follows the session. Later asks from its tree reach the client only when the next prompt, load or resume lists pending asks.

How it can happen

  • loadSession (packages/cli/src/acp/service.ts:148-155) and resumeSession (service.ts:204-210) call sessions.attach, then turn.follow.
  • attach swaps the session's entry synchronously and closes the replaced entry's scope (sessions.ts:145-148). Only then does it await MCP registration (sessions.ts:149), one PUT /api/experimental/mcp/:name per server.
  • A second load of the same session that starts and finishes while the first is still registering leaves the first load calling follow with an attachment that is no longer current.

Suggested test

In packages/cli/test/acp/turn.test.ts, with the wire fixture:

  1. Start load A with one MCP server. Use the fixture's fetch option to hold A's PUT /api/experimental/mcp/<name>: the fixture awaits options.fetch (wire-fixture.ts:656), and body routes return its override first (:675).
  2. While it is held, run load B of the same session with no MCP servers, and let it finish.
  3. Release A's PUT and let load A finish.
  4. Send permissionAsked for the session and wait for the client's reply.

It should pass with the guard and time out without it. Two things to confirm while writing it:

  • The agent must handle the two session/load requests concurrently.
  • Load A must still return after its scope was closed. In effect-smol's source, a fiber forked into a closed scope is interrupted rather than failing (fiberRunIn), so attach's fork of the catalog loop shouldn't fail.

Acceptance criteria

  • A test that fails with lines 409-410 of turn.ts removed and passes with them.
  • It passes 10 of 10 runs with --rerun-each 10, per the unit-tests skill.

Activity

  1. added a commit that references this issue on Oct 20, 2026
    8a60cf9
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions