mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-11-08 01:57:31 +01:00
63306f6660
## Thinking Path
> - Paperclip is the open source app people use to manage AI agents for
work
> - Hosted `claude_local` runs use the ACPX engine. Claude can start
background subagents, and those subagents call the runtime MCP tools
that Paperclip attaches.
> - #16671 added a stalled-tool watchdog. It ends a Claude turn when a
pending tool call gets no ACP update for 30 minutes. That PR said the
most likely cause of the stall is a lost ACP permission round trip, but
it could not prove it.
> - A staging run tripped the watchdog. Two background subagents started
12 MCP calls. ACP reported each call as `pending`, and no call got
another update. The MCP gateway logs show the 9 earlier calls from the
parent agent, but no request from the 12 subagent calls.
`MCP_TOOL_TIMEOUT` (15 minutes) did not end them. So the calls stalled
before Claude sent them to the MCP server.
> - The run record cannot say if the permission request for a stalled
call reached Paperclip. Without that fact, the next stall gives no new
evidence about the cause.
> - This pull request records, for a stalled run, whether the stalled
call's permission request reached Paperclip, and how many pending calls
had a permission request.
> - The benefit is that the next stall tells us which side of the
permission round trip stopped, so the real fix can go to the correct
layer.
## Linked Issues or Issue Description
- Refs #16671 (the stalled-tool watchdog; this PR tests the cause that
PR suspected)
- Refs #16360 (the MCP client deadline that cannot reach a call that was
never sent)
**What happened?**
A hosted staging `claude_local` run started two background subagents.
Each subagent started six `execute-sentry-tool` MCP calls. ACP reported
all 12 calls as `pending`. None got a result, an update, or an error.
The MCP gateway received no request for them. The parent turn finished
its answer but stayed open for the subagents. After 31 minutes the
watchdog ended the turn with `acpx_tool_stalled`. A single background
subagent with six parallel calls to the same tool did not stall in a
later test.
**Expected behavior**
When a tool call stalls, the run record shows if the call's permission
request reached Paperclip. Then the stall can be put on the agent side
or on the relay side of the round trip.
**Steps to reproduce**
1. Start an ACP `claude_local` run in a sandbox that uses two background
subagents to call runtime MCP tools.
2. Wait until a subagent's tool calls stay `pending` with no further ACP
update (in staging this happened without a known trigger).
3. See that the watchdog fails the run with `acpx_tool_stalled`, but the
record has no evidence about the permission round trip.
**Paperclip version or commit**
`master` at 1f27427.
## What Changed
- The ACPX engine passes an `onPermissionRequest` hook to the ACPX
runtime. The hook records the tool call ID of each permission request
and returns `undefined`. ACPX then uses its permission mode to decide
the request, the same as before.
- When the watchdog fires, it records `permissionRequested` for the
stalled call and counts the pending calls that had a permission request.
- A stalled run's result has two new fields:
`acpStalledToolPermissionRequested` (boolean) and
`acpPendingToolPermissionRequestedCount` (non-negative integer). The
`[paperclip]` stderr line also says if the request reached Paperclip.
- The server run-failure diagnostics allowlist the two fields. They
accept only a boolean and a safe non-negative integer.
- `doc/observability.md` describes the fields. It also says that a
background subagent's Task call completes when the subagent starts, so
the watchdog watches the subagent's own calls.
## Verification
- `npx vitest run src/acpx-engine` in `packages/adapter-utils`: 21
files, 423 tests pass.
- Engine tests:
- The existing stall test now sends a permission request for the Task
call only. The run records `acpStalledToolPermissionRequested: false`
and a count of 1. The hook returns `undefined`. The log line says the
request did not reach Paperclip. No tool title or ID appears in the
diagnostics.
- A new test sends the permission request after the relay saw the
stalled call. The run records `acpStalledToolPermissionRequested: true`.
- Mutation check: when the hook does not record the ID, both tests fail.
- `npx vitest run
src/services/__tests__/run-failure-diagnostics.test.ts` in `server`: 115
tests pass. Invalid values for the new fields are dropped.
- `npx tsc --noEmit` passes in `packages/adapter-utils` and in `server`.
- Runtime dependency assumption: the hook must not change permission
decisions. I called the real patched `acpx@0.12.0`
`AcpClient.handlePermissionRequest` with and without the hook in
`approve-all`, `approve-reads` and `deny-all`. The responses and the
permission stats were the same in each mode, and the hook saw each
request. The hook only adds an ID to an in-memory set for the run, so it
adds no remote call and no blocking work.
- Limits: the tests mock the runtime, so they do not prove the hook runs
in a real sandbox relay. The acpx source passes the hook to the client
on every runtime path (`runtime.js`), and the real-client check above
covers the decision path.
## Risks
- Low risk. The hook only observes. If ACPX ever treats a host hook as
the decision owner, the check above would show a different decision.
That is not the case in the pinned `acpx@0.12.0`.
- The set holds one string per permission request for the length of one
run. That is bounded by the number of tool calls in the run.
- This PR does not fix the stall. It gives the evidence for the next
occurrence.
## Model Used
- Claude Opus 5.5 (`claude-opus-5-5`) through Claude Code, with tool use
and code execution. It read the run log, the gateway logs, and the ACP
agent and client code, ran reproduction probes, edited the code, and ran
the tests.
## Checklist
- [x] I have included a thinking path that traces from project context
to this change
- [x] I have specified the model used (with version and capability
details)
- [x] I have checked ROADMAP.md and confirmed this PR does not duplicate
planned core work
- [x] I have searched GitHub for duplicate or related PRs and linked
them above
- [x] I have either (a) linked existing issues with `Fixes: #` / `Closes
#` / `Refs #` OR (b) described the issue in-PR following the relevant
issue template
- [x] I have not referenced internal/instance-local Paperclip issues or
links (only public GitHub `#NNN` / `github.com/paperclipai/paperclip`
URLs)
- [x] My branch name describes the change (e.g. `docs/...`, `fix/...`)
and contains no internal Paperclip ticket id or instance-derived details
- [x] I have run tests locally and they pass
- [x] I have added or updated tests where applicable
- [x] I have updated relevant documentation to reflect my changes
- [x] I have considered and documented any risks above
- [ ] All Paperclip CI gates are green
- [ ] Greptile is 5/5 with no open P2s, recommendations, or follow-ups
- [x] I will address all Greptile and reviewer comments before
requesting merge
🤖 Generated with [Claude Code](https://claude.com/claude-code)
---------
Co-authored-by: devinfoley <139239+devinfoley@users.noreply.github.com>
Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>