Skip to content

fix(agent-computer): honour a Stop that already landed - #733

Open
aniruddhaadak80 wants to merge 1 commit into
CopilotKit:mainfrom
aniruddhaadak80:fix/shell-honour-a-stop-that-already-landed
Open

aniruddhaadak80 wants to merge 1 commit into
CopilotKit:mainfrom
aniruddhaadak80:fix/shell-honour-a-stop-that-already-landed

Conversation

@aniruddhaadak80

Copy link
Copy Markdown

What this changes

shell.run in agent-computer/src/shell.ts is how a person's Stop reaches the command itself, not just the HTTP request that started it. It did that with an abort listener:

const onAbort = stop;
input.signal?.addEventListener("abort", onAbort, { once: true });

An abort listener added to an already-aborted signal never fires. So this only ever honoured a Stop that arrived after that line.

The abort can beat the spawn above it. POST /exec passes request.signal straight through (agent-computer/src/index.ts:1168), and the file already documents the whole chain at index.ts:1266-1267:

The caller going away is the stop signal: the surface aborts its request, the server aborts the one it made to this computer, and Bun aborts this one in turn.

Whenever the person was quick enough, that chain completed before run reached the spawn. The command then ran to its own timeoutMs, timedOut stayed false, and the reply carried no indication that anyone had pressed Stop. The person got no answer until the command finished on its own.

This change reads the flag after subscribing, so a Stop means the same thing whenever it landed. The server already takes exactly this precaution on its side before it fetches (server/src/computer/client.ts:192), for the same reason — an existing precedent rather than a new idea.

Two tests, because only one of the two cases was broken:

  • a Stop that landed before the command started still stops it — the regression. Fails on main: the command runs its full 30 s and the assertion on elapsed time fails.
  • a Stop that lands mid-command still stops it — already worked, pinned here so the change cannot regress it.

Where it runs

  • New state that outlives a request? None. stop() and the timer already existed; this only decides when stop() is called. No state is added, and nothing is retained between calls.
  • What happens on the second replica? Identical. This is inside one agent-computer process, holding no cross-request state. A Bot's computer is one process per Bot, so there is no second replica to disagree with here — and nothing new that a replica would have to share.
  • Anything serialised? Nothing. No write, no row, no lock.
  • Anything fanned out to a browser? No.
  • New listener, port, or schedule? None. The abort listener that already existed is unchanged; this only also fires it when the signal is already aborted.

Boundary and audit

  • Every acting call still goes through the gateway: resolve, decide, audit, then act.

    Not an acting call. This is a Stop, which is the absence of an action — it kills the process group earlier than before. It cannot make a command act that would not have acted.

  • New refusals and new failures each write a row.

    No new refusal or failure. A stopped command still resolves through the same { command, exitCode, stdout, stderr, truncated, timedOut } return, so POST /exec reports it exactly as it did for a mid-flight Stop.

  • Nothing new is trusted from the client that the server can resolve itself.

    Nothing new is read. The only value consulted is the same AbortSignal the server's own request already carries.

Changelog

  • A line in CHANGELOG.md under Unreleased.

    Added. The deployment-visible difference is that a Stop now takes effect immediately rather than at the end of the command.

Proof

Please read this part before judging the tests. This suite is POSIX-only and I could not execute it meaningfully on the machine I work on (Windows, no bash/sh, and the file hardcodes HOME=/root, sleep, /dev/zero). 11 of the 25 tests in shell.test.ts already fail on main locally for that reason, before any change of mine. So I am not claiming a green local run of this file, and my two new tests are not meaningful evidence on Windows — the spawned command fails instantly there regardless of the fix.

What I did run:

  • bun test agent-computer/tests/shell.test.ts — 16 pass, 11 fail, against a main baseline of 14 pass, 11 fail for the same file. The 11 failures are identical and pre-existing; my two additions account for the 14 → 16. Nothing regressed, but as above this is not the verification that matters.
  • bun run typecheck in agent-computer/ — exit 0, no diagnostics.
  • bunx biome lint --error-on-warnings and bunx biome format on both changed files — clean.

The premise I was able to verify platform-independently, which is the part that makes this a bug rather than a guess — an abort listener on an already-aborted signal never fires, while one added before the abort does:

$ bun -e "const s=AbortSignal.abort(); let fired=false;
  s.addEventListener('abort',()=>{fired=true},{once:true});
  console.log('already-aborted, listener fired:', fired, '| signal.aborted:', s.aborted);
  const c=new AbortController(); let f2=false;
  c.signal.addEventListener('abort',()=>{f2=true}); c.abort();
  console.log('abort after subscribing, listener fired:', f2);"
already-aborted, listener fired: false | signal.aborted: true
abort after subscribing, listener fired: true

So the mid-flight path works today and the pre-abort path cannot. That is the whole defect, and it is a property of AbortSignal rather than of this platform.

The two new tests follow the wall-clock pattern the neighbouring tests in this file already use (a command that backgrounds a process is still stopped asserts Date.now() - started is under 10 s), so they are written to be meaningful on the ubuntu-latest runner that CI's test job uses. I would rather say plainly that I expect those two tests to go red on main and green here on CI than claim I watched them do it. If they do not, the fix is wrong and the test is telling the truth about it.

run() attached an abort listener and stopped the process group when it
fired, which is the whole of the person's Stop reaching the command:

  const onAbort = stop;
  input.signal?.addEventListener("abort", onAbort, { once: true });

An abort listener added to an already-aborted signal never fires, so this
only honoured a Stop arriving after that line. The abort can beat the spawn
above it: the surface aborts, the server aborts the request it made to this
computer, and Bun aborts this one in turn, which happens whenever the person
was quick. The command then ran to its own limit, the reply said nothing
about a Stop, and the person had no answer until it finished.

Reading the flag after subscribing is what makes a Stop mean the same thing
whenever it landed. The server already does this on its side before it
fetches, for the same reason.

Both cases are covered: a Stop that landed first, and one that lands
mid-command, which already worked and is pinned so the refactor keeps it.
Copilot AI balanced review requested due to automatic review settings October 4, 2026 09:04

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants