From aaeff418cf72341c674ff3393cd94f40dc4855f6 Mon Sep 17 00:00:00 2001 From: Devin Foley Date: Sun, 18 Oct 2026 13:59:16 -0700 Subject: [PATCH] fix(acpx): add a positive control to stalled-tool permission evidence (#16769) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work. > - The ACPX engine runs Claude agents. A watchdog ends a turn when a pending tool call gets no update for 30 minutes. > - #16688 added a field to stalled runs. The field records if the stalled call's permission request reached Paperclip. > - Recent stalled runs show `acpStalledToolPermissionRequested: false`. We can read this as "the call stopped before its permission request". That reading is correct only if calls of the same kind usually send a request. > - We have no data that shows if finished calls of that kind sent requests. Thus we cannot tell if the missing request is a real signal. > - This pull request records a positive control: the number of finished calls of the stalled call's kind, and how many of them sent a permission request. > - The benefit: the next stall event tells us if the missing request locates the stall, or if it gives no information. ## Linked Issues or Issue Description Refs #16688 **What happened?** Runs stop with the stalled-tool watchdog error. The diagnostics show that the stalled call sent no permission request. The diagnostics do not show if finished calls of the same kind sent requests. **Expected behavior** The stalled-run diagnostics contain enough data to show if a missing permission request is a real signal. **Steps to reproduce** A pending tool call gets no ACP update for 30 minutes. The watchdog ends the turn. The run result contains the stalled-tool fields. ## What Changed - `packages/adapter-utils/src/acpx-engine/execute.ts`: when the watchdog fires, count the finished calls of the stalled call's kind, and count how many of them sent a permission request. Write the counts to the run result as `acpSettledSameKindToolCount` and `acpSettledSameKindToolPermissionRequestedCount`. - `server/src/services/run-failure-diagnostics.ts`: allow the two new fields as non-negative safe integers. Other values are dropped. - `doc/observability.md`: explain how to read the two fields with the existing permission-request field. - Tests: extend the existing stalled-tool tests in `execute.test.ts` and `run-failure-diagnostics.test.ts`. ## Verification - `pnpm exec vitest run src/acpx-engine/execute.test.ts` in `packages/adapter-utils`: 244/244 pass. - `pnpm exec vitest run src/services/__tests__/run-failure-diagnostics.test.ts` in `server`: 115/115 pass. - Mutation check: remove the kind filter, or count every finished call as "requested". Each mutation makes the stalled-tool test fail. - `tsc --noEmit` passes in `packages/adapter-utils` and in `server`. Note: the full `server` `typecheck` script did not run locally, because the runner vendor build step failed in this sandbox. I ran `tsc --noEmit` directly. ## Risks - Low risk. The change only adds two counts to the run result after the watchdog fires. It does not change the decision to stop the turn, or any permission decision. - The fields hold only counts. They contain no tool titles, inputs, or identifiers. This is the run-log and run-result path. It is not Telemetry. The fields reach Sentry through the existing allowlist in `run-failure-diagnostics.ts`. ## Model Used - Claude Opus 5.5 (`claude-opus-5-5`), Anthropic, used through Claude Code with tool use (shell, file edits, test runs). ## 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 --- doc/observability.md | 19 +++++++++++++++++++ .../src/acpx-engine/execute.test.ts | 14 ++++++++++++++ .../adapter-utils/src/acpx-engine/execute.ts | 13 +++++++++++++ .../__tests__/run-failure-diagnostics.test.ts | 5 ++++- .../src/services/run-failure-diagnostics.ts | 3 ++- 5 files changed, 52 insertions(+), 2 deletions(-) diff --git a/doc/observability.md b/doc/observability.md index 7703888f8fa..1052b718785 100644 --- a/doc/observability.md +++ b/doc/observability.md @@ -1008,6 +1008,25 @@ before the request reached Paperclip. A call whose request arrived stalled later: on the reply's return trip, or after the agent received the decision. Both fields hold only a boolean and a count. +A missing request is informative only if calls like it normally send one. So a +stalled run also records `acpSettledSameKindToolCount` (how many finished calls +of the stalled call's kind the run saw) and +`acpSettledSameKindToolPermissionRequestedCount` (how many of those sent a +permission request). These counts are supporting evidence, not proof of where +the call stalled. The ACP kind is coarse, so peers of the same kind can be +different tools with different permission rules. Read them this way: + +- All or nearly all finished peers sent a request, and the stalled call did not: + this supports a stall before the request reached Paperclip. +- No finished peer sent a request: calls of that kind may skip the request, so + its absence on the stalled call says nothing about where it stalled. +- Some finished peers sent a request and others did not: the counts do not show + whether the stalled call should have sent one. Compare the specific tools in + the run log before drawing a conclusion. +- No finished peers (a count of zero): there is no comparison. + +Both fields hold only counts. + ACP and legacy OpenCode results retain the adapter's resolved wall-clock timeout as `adapterExecutionTimeout` in the instance run result. Finalization uses it for `effectiveTimeoutSec`, `timeoutSource`, and `timeoutConfigured`. Sources are diff --git a/packages/adapter-utils/src/acpx-engine/execute.test.ts b/packages/adapter-utils/src/acpx-engine/execute.test.ts index 3d58d77bd4a..7ef0e510688 100644 --- a/packages/adapter-utils/src/acpx-engine/execute.test.ts +++ b/packages/adapter-utils/src/acpx-engine/execute.test.ts @@ -3539,6 +3539,16 @@ describe("ACP activity diagnostics", () => { inferredKind: "think", } as never, { signal: new AbortController().signal })); yield { type: "tool_call", toolCallId: "private-task", title: "private task", kind: "think", status: "in_progress" }; + for (const toolCallId of ["private-peer-asked", "private-peer-shell"]) { + await options.onPermissionRequest?.({ + sessionId: "session-1", + raw: { sessionId: "session-1", toolCall: { toolCallId }, options: [] }, + inferredKind: "other", + } as never, { signal: new AbortController().signal }); + } + yield { type: "tool_call", toolCallId: "private-peer-asked", kind: "other", status: "completed" }; + yield { type: "tool_call", toolCallId: "private-peer-silent", kind: "other", status: "failed" }; + yield { type: "tool_call", toolCallId: "private-peer-shell", kind: "execute", status: "completed" }; yield { type: "tool_call", toolCallId: "private-call", title: "private lookup", kind: "other", status: "pending" }; currentNow = 7000; yield { type: "tool_call", tag: "tool_call_update", toolCallId: "private-task", status: "in_progress" }; @@ -3572,6 +3582,8 @@ describe("ACP activity diagnostics", () => { acpPendingToolKinds: "other,think", acpStalledToolPermissionRequested: false, acpPendingToolPermissionRequestedCount: 1, + acpSettledSameKindToolCount: 2, + acpSettledSameKindToolPermissionRequestedCount: 1, }); // The host only observes the request; ACPX's permission mode decides it. expect(permissionDecisions).toEqual([undefined]); @@ -3669,6 +3681,8 @@ describe("ACP activity diagnostics", () => { acpStalledToolKind: "other", acpStalledToolPermissionRequested: true, acpPendingToolPermissionRequestedCount: 1, + acpSettledSameKindToolCount: 0, + acpSettledSameKindToolPermissionRequestedCount: 0, }); expect(logs.join("")).toContain("Its permission request reached Paperclip."); }, 15_000); diff --git a/packages/adapter-utils/src/acpx-engine/execute.ts b/packages/adapter-utils/src/acpx-engine/execute.ts index 82c232930dc..425f802e153 100644 --- a/packages/adapter-utils/src/acpx-engine/execute.ts +++ b/packages/adapter-utils/src/acpx-engine/execute.ts @@ -4137,6 +4137,8 @@ export function createAcpxEngineExecutor(deps: AcpxEngineExecutorOptions = {}) { silentMs: number; permissionRequested: boolean; pendingPermissionRequested: number; + settledSameKind: number; + settledSameKindPermissionRequested: number; } | null = null; let stalledToolTimer: ReturnType | undefined; let forcedStop = false; @@ -5055,12 +5057,21 @@ export function createAcpxEngineExecutor(deps: AcpxEngineExecutorOptions = {}) { } if (!oldest) return; clearInterval(stalledToolTimer); + let settledSameKind = 0; + let settledSameKindPermissionRequested = 0; + for (const [toolCallId, tool] of interruptionTools) { + if (isPendingAcpTool(tool) || tool.kind !== oldest.kind) continue; + settledSameKind += 1; + if (permissionRequestedToolCallIds.has(toolCallId)) settledSameKindPermissionRequested += 1; + } const knownKinds: readonly string[] = ACP_TOOL_KINDS; stalledTool = { kind: oldest.kind && knownKinds.includes(oldest.kind) ? oldest.kind : "unknown", silentMs: oldest.silentMs, permissionRequested: permissionRequestedToolCallIds.has(oldest.toolCallId), pendingPermissionRequested, + settledSameKind, + settledSameKindPermissionRequested, }; turnStopRequestedAt = checkedAt; pendingToolsAtStop = [...interruptionTools.values()].filter(isPendingAcpTool); @@ -5352,6 +5363,8 @@ export function createAcpxEngineExecutor(deps: AcpxEngineExecutorOptions = {}) { acpStalledToolSilentMs: stalledTool.silentMs, acpStalledToolPermissionRequested: stalledTool.permissionRequested, acpPendingToolPermissionRequestedCount: stalledTool.pendingPermissionRequested, + acpSettledSameKindToolCount: stalledTool.settledSameKind, + acpSettledSameKindToolPermissionRequestedCount: stalledTool.settledSameKindPermissionRequested, } : {}), ...(failureDiagnostic ? { terminalSessionFailure: failureDiagnostic } : {}), diff --git a/server/src/services/__tests__/run-failure-diagnostics.test.ts b/server/src/services/__tests__/run-failure-diagnostics.test.ts index d60f455bd7b..9ffec4f367d 100644 --- a/server/src/services/__tests__/run-failure-diagnostics.test.ts +++ b/server/src/services/__tests__/run-failure-diagnostics.test.ts @@ -462,10 +462,12 @@ describe("run failure diagnostics", () => { const result = sanitizeRunFailureDiagnostics(collectRunFailureDiagnostics(run({ resultJson: { acpStalledToolKind: "other", acpStalledToolSilentMs: 1_800_000, acpStalledToolTitle: "private lookup", acpStalledToolPermissionRequested: false, acpPendingToolPermissionRequestedCount: 3, + acpSettledSameKindToolCount: 30, acpSettledSameKindToolPermissionRequestedCount: 29, } }), {})); expect(result.execution).toEqual({ acpStalledToolKind: "other", acpStalledToolSilentMs: 1_800_000, acpStalledToolPermissionRequested: false, acpPendingToolPermissionRequestedCount: 3, + acpSettledSameKindToolCount: 30, acpSettledSameKindToolPermissionRequestedCount: 29, }); expect(JSON.stringify(result)).not.toContain("private"); }); @@ -490,7 +492,8 @@ describe("run failure diagnostics", () => { acpLastEventAgeMs: value, acpObservedEventCount: value, acpPendingToolCount: value, acpToolInventoryComplete: value, acpLastEventAgeAtStopMs: value, acpOldestPendingToolAgeAtStopMs: value, acpStalledToolSilentMs: value, acpStalledToolPermissionRequested: value, - acpPendingToolPermissionRequestedCount: value, + acpPendingToolPermissionRequestedCount: value, acpSettledSameKindToolCount: value, + acpSettledSameKindToolPermissionRequestedCount: value, } }), {}); expect(result.execution).toEqual({}); }, diff --git a/server/src/services/run-failure-diagnostics.ts b/server/src/services/run-failure-diagnostics.ts index e2fcffa0932..d87f62e21f8 100644 --- a/server/src/services/run-failure-diagnostics.ts +++ b/server/src/services/run-failure-diagnostics.ts @@ -314,7 +314,8 @@ export function collectRunFailureDiagnostics(run: Run, options: RunFailureReport for (const field of [ "acpLastEventAgeMs", "acpObservedEventCount", "acpPendingToolCount", "acpLastEventAgeAtStopMs", "acpOldestPendingToolAgeAtStopMs", "acpStalledToolSilentMs", - "acpPendingToolPermissionRequestedCount", + "acpPendingToolPermissionRequestedCount", "acpSettledSameKindToolCount", + "acpSettledSameKindToolPermissionRequestedCount", ]) { const value = read(result, field); if (typeof value === "number" && Number.isSafeInteger(value) && value >= 0) execution[field] = value;