mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-10-21 08:22:20 +02:00
fix(acpx): add a positive control to stalled-tool permission evidence (#16769)
## 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 <noreply@anthropic.com>
This commit is contained in:
5 files changed
+52
-2
No files matched your search
@@ -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
|
||||
|
||||
@@ -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);
|
||||
|
||||
@@ -4137,6 +4137,8 @@ export function createAcpxEngineExecutor(deps: AcpxEngineExecutorOptions = {}) {
|
||||
silentMs: number;
|
||||
permissionRequested: boolean;
|
||||
pendingPermissionRequested: number;
|
||||
settledSameKind: number;
|
||||
settledSameKindPermissionRequested: number;
|
||||
} | null = null;
|
||||
let stalledToolTimer: ReturnType<typeof setInterval> | 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 } : {}),
|
||||
|
||||
@@ -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({});
|
||||
},
|
||||
|
||||
@@ -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;
|
||||
|
||||
Reference in new issue
Block a user