mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-10-31 06:04:11 +01:00
## Thinking Path > - Paperclip manages agent work and its governed tool calls. > - A human approval can persist before the approving process dispatches the tool. > - Recovery currently scans approved requests on every maintenance tick. > - Completed and deliberately inert approvals do not owe another execution. > - Test-tab approval also needs a shared execution claim before recovery can safely race the foreground path. > - This PR wakes recovery from committed changes and retries only unresolved approvals. > - Idle history becomes quiet while existing approval and execution checks remain with the gateway. ## Linked Issues or Issue Description Refs #17453. This is the fifth slice of the recurring-server sleep migration, stacked on `codex/sleep-04-tool-review-expiry`. **Problem** Approved tool requests are scanned periodically even when no executable work remains. The Test-tab foreground path lacks the request claim needed to admit concurrent recovery safely. **Proposed Solution** Use the existing transaction-aware review notification boundary and shared deadline worker. Keep signed-payload interpretation, current policy and target checks, and provider dispatch in the gateway. Share a claim between Test-tab foreground and recovery, then persist both authoritative outcome rows atomically. **Alternatives Considered** A longer interval still scans idle history. A new durable queue would duplicate existing request/invocation state. Dispatching Test-tab approvals without a claim risks duplicate external calls. ## What Changed - Recheck both principals for delegated Test-tab approvals. Save their rate receipts in the same claim transaction, share only the same bucket, and roll back new charges on denial. - Preserve delegated gateway authorization and both issuer/target rate receipts inside the existing claimed-invocation transaction; retain the durable actor during approved replay. - Expire stale managed connector payloads before consuming approved-tool admission. - Persist current rate-policy charges and their invocation receipts atomically before dispatch, preserving exact approval claim fencing and read-only preflight. - Recover approved requests at startup and after committed review changes; retry only unresolved approvals. - Leave legacy agent approvals without execute-on-approve intent inert. Repair completed Test-tab history without another provider call. - Fence Test-tab execution and outcome settlement with an atomic claim shared by both callers. Revalidate the approved target and current policy before dispatch. - Keep provider calls outside database transactions. Retain quarantine for interrupted execution and unknown outcomes. - Add a concurrent, idempotent partial index for approved-request pagination. Reuse the parent's company-scoped review signals and shared scoped deadline worker. - Remove approved scanning from the aggregate tick. Preserve the public combined sweep and scheduler-disabled startup behavior. - Document producer ownership, quiet states, lifecycle behavior, and remaining recovery duties. ## Verification Latest published head: `23868c1751c100a27f58087f7ce16b93c52d4eba`, targeting master. The review-found delegated gateway pool deadlock is repaired by passing the admission transaction through token and issuer-authority reads. Foreground and recovery each reproduced the deadlock with a one-connection pool; all three constrained-pool cases, including revoked authority, now pass. All 81 HTTP integration cases pass. The historical foreground rate-slot regression also reproduced before its repair; verified preview hashes are accepted only by read-only preflight, while consuming admission still requires the complete identity under the exact claim. Five full suites passed 329 cases; the gateway service then passed all 141 cases after two additional mixed historical hash controls. Production types/build passed before the final normal integration of master `35820cf398` (including independent Grok/Runner changes); integration validation is running. Initial fixture/assertion errors and the prior E2E restoration/cancellation deadlock remain recorded in [the owning audit](doc/idle-approved-tool-recovery.md#gateway-admission-uses-its-transaction-connection). Fresh-head CI, Greptile 5/5, and passing Ponytail are still required before merge. - Master landing reconciliation at `8dfe3958db6e4e8a0b7499f3f5be0a396ca77361` after #17453 merged: production and server test files are byte-identical to the accepted 320-case repair. The landed migration journal prefix is unchanged, and #17928's independent eval reporting changes are preserved. Fresh CI and reviews on this exact head are required before merge. - Historical-identity repair published at `096b7c7198c3ca864aca9377e261cbaa0a8aad6f` (production/test source identical to integrated acceptance `81231824d773a79fc50c835eccf7d9b3977e79fc`): all 320 accepted gateway, policy, startup, preparation-timing and authenticated HTTP management cases pass. Server types and the production server build pass. Four real historical sensitive-argument regressions failed before repair; invalid-signature, foreign-hash and stale-claim controls remain denied without dispatch or identity changes. Both agent and Test-tab calls deliver the original signed arguments once, and new rate policies still apply. - The first integrated combined run passed 319 cases and hit a Slack bot ownership collision left in a reused HTTP test database. The guard correctly returned 409. The full 78-case HTTP suite passed on a fresh dedicated fixture without product or assertion changes. Earlier fixture-construction errors are distinguished from the four real regressions in the audit document. Fresh remote CI and reviews remain required. - Parent reconciliation at `736e7f56097055e21ce13b4e45d9033391483af8`: 299 cases pass (221 full gateway/policy/startup cases and 78 assembled-server connection management cases), with no skipped cases. Server types pass. This includes real delegated Test-tab approval admission for newly added shared and caller rate rules. Fresh remote CI and Greptile review remain required. Approved delegated Test-tab calls now evaluate caller and target policies in the existing claim transaction. Real HTTP regressions first showed a missing caller charge and shared-limit self-rejection. All 78 connection-management cases and 92 gateway cases now pass on the owning branch, along with server production typechecking. All 78 connection-management cases also pass at combined tip `d25f830db89c1623bff3547bfc68015c1c66e0cf`. The three new cases cover shared charges, separate charges, rollback on denial, duplicate approval, and rejection of a later authorized call. Intermediate counter/identity fixture failures and a reused-database Slack collision are recorded in [the owning audit](https://github.com/paperclipai/paperclip/blob/bcca5b006976a471f4afd1f70c4031aa40a2b3f7/doc/idle-approved-tool-recovery.md#delegated-test-tab-admission-after-policy-changes). The earlier combined checks below remain evidence for their stated revisions. Current-head CI and review remain required. Current head: `bcca5b006976a471f4afd1f70c4031aa40a2b3f7`. This remains a single-duty PR stacked on #17453. - Integrated pinned master `8548173fc9efdc0fa07644fb92dcc0b6851a95e5` with normal merges. Every branch passed generated-schema checks and its target-branch migration-order gate. Existing SQL is unchanged; the 25 stack migrations follow master’s new migration. - Duty-specific producer coverage, lifecycle tests, and earlier failure records: [2026-10-24-recurring-server-sleep-stack.md](https://github.com/paperclipai/paperclip/blob/a021467f04d22be42ee5b0c87b6583b2300a62a9/doc/plans/2026-10-24-recurring-server-sleep-stack.md). - Combined tip `51e6cbe67979b55b340ea9f0f434c5b5c682ccd6`: **355 startup/cancellation/recovery/dispatch/preservation tests**, **75 connection-management tests**, and production server typechecking pass. These combined checks do not mean every intermediate PR independently ran the full repository suite. - Real master-to-stack database upgrade passes: **379 unchanged prior migration receipts plus 25 new migrations**, with the gateway responsibility fixture preserved. - Owning-branch checks: 84 runner installation/settlement cases plus runner typechecking and generated TypeScript/Rust profile agreement; 253 GitHub review workflow cases. Stop-attribution and held-worker readiness regressions failed before their fixes. - Failure accounting: the initial combined run passed 349 cases and failed six idle assertions in a reused preservation database containing old cancelled runs. Repeating the combined command with a fresh preservation database passed. Earlier invalid project selection, merge import/type errors, and the interrupted full local suite are not counted as passing checks. - **Latest-head CI and review remain required.** Prior CI findings include GitHub idle observation, route teardown, attention cleanup, touch-picker E2E, workspace health, and voice-cleanup timing; local passes do not erase those failures. No full local repository-suite, deployed sleep, fleet cost, or live Copilot qualification claim is made. ## Risks - Notifications cover the existing database-owner, single-active-writer contract. Independent SQL writers and other replicas need an explicit wake path. - Unknown provider outcomes cannot be replayed safely. Failed result persistence retains an executing claim and the existing stale-execution quarantine. - Interrupted execution and terminal interaction projection repair remain periodic until their separate slices. This PR changes only approved recovery. - A pending-only review write can trigger one empty approved-recovery pass. Settled idle work schedules no further scan. - The partial index uses the existing top-level concurrent migration executor. Snapshot pruning follows repository policy. - Cloud sleep eligibility and external alarm coverage remain unchanged. Worker quiet is not proof of deployed instance suspension. ## Model Used OpenAI Codex, GPT-6 family, with reasoning and code execution. The exact serving model identifier and context window were not exposed. ## 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 --------- Co-authored-by: Paperclip <noreply@paperclip.ing>
120 lines
6.8 KiB
TypeScript
120 lines
6.8 KiB
TypeScript
import { and, eq } from "drizzle-orm";
|
|
import { toolActionRequests, toolInvocations, type Db } from "@paperclipai/db";
|
|
import type { ToolRedactedValueSummary } from "@paperclipai/shared";
|
|
import { notifyToolReviewChange } from "./tool-review-work-notifications.js";
|
|
|
|
export type ToolApprovalClaim = { actionRequestId: string; invocationId: string; claimedAt: Date };
|
|
export type ToolTestOutcome = {
|
|
status: "succeeded" | "failed" | "timed_out" | "rate_limited";
|
|
result?: ToolRedactedValueSummary;
|
|
errorCode?: string;
|
|
errorMessage?: string;
|
|
};
|
|
|
|
export async function claimToolTestApproval(db: Db, actionRequestId: string, invocationId: string): Promise<ToolApprovalClaim | null> {
|
|
return db.transaction(async tx => {
|
|
const [invocation] = await tx.select().from(toolInvocations)
|
|
.where(eq(toolInvocations.id, invocationId)).for("update");
|
|
const [request] = await tx.select().from(toolActionRequests)
|
|
.where(and(eq(toolActionRequests.id, actionRequestId), eq(toolActionRequests.invocationId, invocationId))).for("update");
|
|
if (!invocation || request?.status !== "approved") return null;
|
|
await notifyToolReviewChange(tx, request.companyId);
|
|
const now = new Date();
|
|
if (invocation.status !== "awaiting_approval") {
|
|
if (["succeeded", "failed", "denied", "timed_out", "rate_limited"].includes(invocation.status)) {
|
|
await tx.update(toolActionRequests).set({ status: invocation.status === "succeeded" ? "executed" : "failed",
|
|
resolvedAt: invocation.completedAt ?? now, updatedAt: now }).where(eq(toolActionRequests.id, actionRequestId));
|
|
} else if (invocation.status === "executing") {
|
|
await tx.update(toolActionRequests).set({ status: "executing", updatedAt: invocation.startedAt ?? invocation.updatedAt })
|
|
.where(eq(toolActionRequests.id, actionRequestId));
|
|
}
|
|
return null;
|
|
}
|
|
await tx.update(toolActionRequests).set({ status: "executing", updatedAt: now })
|
|
.where(eq(toolActionRequests.id, actionRequestId));
|
|
return { actionRequestId, invocationId, claimedAt: now };
|
|
});
|
|
}
|
|
|
|
/** Return only this still-undispatched claim to its existing approval queue. */
|
|
export async function releaseToolApproval(db: Db, claim: ToolApprovalClaim) {
|
|
return db.transaction(async tx => {
|
|
const [invocation] = await tx.select().from(toolInvocations)
|
|
.where(eq(toolInvocations.id, claim.invocationId)).for("update");
|
|
const [request] = await tx.select().from(toolActionRequests)
|
|
.where(and(eq(toolActionRequests.id, claim.actionRequestId),
|
|
eq(toolActionRequests.invocationId, claim.invocationId))).for("update");
|
|
if (invocation?.status !== "awaiting_approval" || invocation.startedAt !== null ||
|
|
request?.status !== "executing" || request.companyId !== invocation.companyId ||
|
|
request.updatedAt.getTime() !== claim.claimedAt.getTime()) return false;
|
|
await notifyToolReviewChange(tx, request.companyId);
|
|
await tx.update(toolActionRequests).set({ status: "approved", updatedAt: new Date() })
|
|
.where(eq(toolActionRequests.id, request.id));
|
|
return true;
|
|
});
|
|
}
|
|
|
|
export async function bindVerifiedToolApprovalArguments(db: Db, claim: ToolApprovalClaim, hashes: { preview: string; verified: string }) {
|
|
return db.transaction(async tx => {
|
|
const [invocation] = await tx.select().from(toolInvocations)
|
|
.where(eq(toolInvocations.id, claim.invocationId)).for("update");
|
|
const [request] = await tx.select().from(toolActionRequests)
|
|
.where(and(eq(toolActionRequests.id, claim.actionRequestId),
|
|
eq(toolActionRequests.invocationId, claim.invocationId))).for("update");
|
|
if (invocation?.status !== "awaiting_approval" || invocation.startedAt !== null ||
|
|
request?.status !== "executing" || request.companyId !== invocation.companyId ||
|
|
request.updatedAt.getTime() !== claim.claimedAt.getTime() ||
|
|
![hashes.preview, hashes.verified].includes(request.canonicalArgumentsHash) ||
|
|
![hashes.preview, hashes.verified].includes(invocation.argumentsHash ?? "")) return false;
|
|
if (invocation.argumentsHash !== hashes.verified) await tx.update(toolInvocations)
|
|
.set({ argumentsHash: hashes.verified, updatedAt: new Date() }).where(eq(toolInvocations.id, claim.invocationId));
|
|
return true;
|
|
});
|
|
}
|
|
|
|
export async function startToolInvocation(db: Db, invocationId: string, claim?: ToolApprovalClaim) {
|
|
const now = new Date();
|
|
if (!claim) {
|
|
await db.update(toolInvocations).set({ status: "executing", startedAt: now, updatedAt: now })
|
|
.where(eq(toolInvocations.id, invocationId));
|
|
return true;
|
|
}
|
|
return db.transaction(async tx => {
|
|
const [invocation] = await tx.select().from(toolInvocations)
|
|
.where(eq(toolInvocations.id, invocationId)).for("update");
|
|
const [request] = await tx.select().from(toolActionRequests)
|
|
.where(eq(toolActionRequests.id, claim.actionRequestId)).for("update");
|
|
if (invocationId !== claim.invocationId || invocation?.status !== "awaiting_approval" || invocation.startedAt !== null ||
|
|
request?.status !== "executing" || request.invocationId !== invocationId || request.companyId !== invocation.companyId ||
|
|
request.updatedAt.getTime() !== claim.claimedAt.getTime()) return false;
|
|
await tx.update(toolInvocations).set({ status: "executing", approvalState: "approved", startedAt: now, updatedAt: now })
|
|
.where(eq(toolInvocations.id, invocationId));
|
|
return true;
|
|
});
|
|
}
|
|
|
|
export async function persistToolTestOutcome(db: Db, invocationId: string, outcome: ToolTestOutcome, claim?: ToolApprovalClaim) {
|
|
const now = new Date();
|
|
const values = { status: outcome.status, ...(outcome.result ? {
|
|
resultHash: outcome.result.sha256 ?? null, resultSummary: outcome.result,
|
|
resultSizeBytes: outcome.result.sizeBytes ?? null,
|
|
} : {}), errorCode: outcome.errorCode, errorMessage: outcome.errorMessage, completedAt: now, updatedAt: now };
|
|
if (!claim) {
|
|
await db.update(toolInvocations).set(values).where(eq(toolInvocations.id, invocationId));
|
|
return true;
|
|
}
|
|
return db.transaction(async tx => {
|
|
const [invocation] = await tx.select({ status: toolInvocations.status }).from(toolInvocations)
|
|
.where(eq(toolInvocations.id, invocationId)).for("update");
|
|
const [request] = await tx.select().from(toolActionRequests)
|
|
.where(eq(toolActionRequests.id, claim.actionRequestId)).for("update");
|
|
if (!invocation || !["awaiting_approval", "executing"].includes(invocation.status) ||
|
|
request?.status !== "executing" || request.updatedAt.getTime() !== claim.claimedAt.getTime()) return false;
|
|
await notifyToolReviewChange(tx, request.companyId);
|
|
await tx.update(toolInvocations).set(values).where(eq(toolInvocations.id, invocationId));
|
|
await tx.update(toolActionRequests).set({ status: outcome.status === "succeeded" ? "executed" : "failed",
|
|
resolvedAt: now, updatedAt: now }).where(eq(toolActionRequests.id, claim.actionRequestId));
|
|
return true;
|
|
});
|
|
}
|