mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-10-29 04:25:52 +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>
211 lines
8.5 KiB
TypeScript
211 lines
8.5 KiB
TypeScript
import { writeToolReviewChange } from "./tool-review-work-notifications.js";
|
|
import { approvalResolutionRecords, resolvableApprovalStatuses as resolvableStatuses } from "../lib/approval-records.js";
|
|
import { notifyHireApproved } from "./hire-hook.js";
|
|
|
|
import { and, asc, eq, inArray, sql, type SQL } from "drizzle-orm";
|
|
import type { Db } from "@paperclipai/db";
|
|
import { approvalComments, approvals } from "@paperclipai/db";
|
|
import { unprocessable } from "../errors.js";
|
|
import { redactCurrentUserText } from "../log-redaction.js";
|
|
import { createAgentLifecycle } from "./agent-lifecycle.js";
|
|
|
|
import { instanceSettingsService } from "./instance-settings.js";
|
|
|
|
export function approvalRecords(db: Db) {
|
|
const instanceSettings = instanceSettingsService(db);
|
|
const { getExistingApproval, resolveApproval: resolveApprovalRecord } = approvalResolutionRecords(db);
|
|
|
|
// Tool reviews waiting for a formal decision stay parked until this producer
|
|
// commits. The shared record owner retains approval validation and CAS rules.
|
|
async function resolveApproval(id: string, status: "approved" | "rejected", userId: string,
|
|
note: string | null | undefined, database: Db = db) {
|
|
const existing = await getExistingApproval(id, database);
|
|
return writeToolReviewChange(database, existing.companyId, tx => resolveApprovalRecord(id, status, userId, note, tx));
|
|
}
|
|
|
|
function redactApprovalComment<T extends { body: string }>(comment: T, censorUsernameInLogs: boolean): T {
|
|
return {
|
|
...comment,
|
|
body: redactCurrentUserText(comment.body, { enabled: censorUsernameInLogs }),
|
|
};
|
|
}
|
|
|
|
return {
|
|
getExistingApproval, resolveApproval,
|
|
list: (companyId: string, status?: string, readCondition?: SQL<boolean>) => {
|
|
const conditions = [eq(approvals.companyId, companyId)];
|
|
if (status) conditions.push(eq(approvals.status, status));
|
|
if (readCondition) conditions.push(readCondition);
|
|
return db.select().from(approvals).where(and(...conditions));
|
|
},
|
|
|
|
getById: (id: string) =>
|
|
db
|
|
.select()
|
|
.from(approvals)
|
|
.where(eq(approvals.id, id))
|
|
.then((rows) => rows[0] ?? null),
|
|
|
|
findOpenHireApprovalForAgent: async (companyId: string, agentId: string) => {
|
|
const rows = await db
|
|
.select()
|
|
.from(approvals)
|
|
.where(
|
|
and(
|
|
eq(approvals.companyId, companyId),
|
|
eq(approvals.type, "hire_agent"),
|
|
inArray(approvals.status, resolvableStatuses),
|
|
sql`${approvals.payload} ->> 'agentId' = ${agentId}`,
|
|
),
|
|
);
|
|
return rows[0] ?? null;
|
|
},
|
|
|
|
create: (companyId: string, data: Omit<typeof approvals.$inferInsert, "companyId">) =>
|
|
db
|
|
.insert(approvals)
|
|
.values({ ...data, companyId })
|
|
.returning()
|
|
.then((rows) => rows[0]),
|
|
|
|
// Cancel an open (pending/revision_requested) approval without a board
|
|
// decision — e.g. when its paired agent is terminated during duplicate
|
|
// cleanup. Idempotent: a no-op on already-resolved approvals.
|
|
cancel: async (id: string, reason?: string | null) => {
|
|
const now = new Date();
|
|
const [existing] = await db.select({ companyId: approvals.companyId }).from(approvals).where(eq(approvals.id, id));
|
|
if (!existing) return null;
|
|
const updated = await writeToolReviewChange(db, existing.companyId, tx => tx
|
|
.update(approvals)
|
|
.set({
|
|
status: "cancelled",
|
|
decisionNote: reason ?? null,
|
|
decidedAt: now,
|
|
updatedAt: now,
|
|
})
|
|
.where(and(eq(approvals.id, id), inArray(approvals.status, resolvableStatuses)))
|
|
.returning()
|
|
.then((rows) => rows[0] ?? null));
|
|
return updated;
|
|
},
|
|
|
|
requestRevision: async (id: string, decidedByUserId: string, decisionNote?: string | null) => {
|
|
const existing = await getExistingApproval(id);
|
|
if (existing.status !== "pending") {
|
|
throw unprocessable("Only pending approvals can request revision");
|
|
}
|
|
|
|
const now = new Date();
|
|
return db
|
|
.update(approvals)
|
|
.set({
|
|
status: "revision_requested",
|
|
decidedByUserId,
|
|
decisionNote: decisionNote ?? null,
|
|
decidedAt: now,
|
|
updatedAt: now,
|
|
})
|
|
.where(eq(approvals.id, id))
|
|
.returning()
|
|
.then((rows) => rows[0]);
|
|
},
|
|
|
|
resubmit: async (id: string, payload?: Record<string, unknown>) => {
|
|
const existing = await getExistingApproval(id);
|
|
if (existing.status !== "revision_requested") {
|
|
throw unprocessable("Only revision requested approvals can be resubmitted");
|
|
}
|
|
|
|
const now = new Date();
|
|
return db
|
|
.update(approvals)
|
|
.set({
|
|
status: "pending",
|
|
payload: payload ?? existing.payload,
|
|
decisionNote: null,
|
|
decidedByUserId: null,
|
|
decidedAt: null,
|
|
updatedAt: now,
|
|
})
|
|
.where(eq(approvals.id, id))
|
|
.returning()
|
|
.then((rows) => rows[0]);
|
|
},
|
|
|
|
listComments: async (approvalId: string) => {
|
|
const existing = await getExistingApproval(approvalId);
|
|
const { censorUsernameInLogs } = await instanceSettings.getGeneral();
|
|
return db
|
|
.select()
|
|
.from(approvalComments)
|
|
.where(
|
|
and(
|
|
eq(approvalComments.approvalId, approvalId),
|
|
eq(approvalComments.companyId, existing.companyId),
|
|
),
|
|
)
|
|
.orderBy(asc(approvalComments.createdAt))
|
|
.then((comments) => comments.map((comment) => redactApprovalComment(comment, censorUsernameInLogs)));
|
|
},
|
|
|
|
addComment: async (
|
|
approvalId: string,
|
|
body: string,
|
|
actor: { agentId?: string; userId?: string },
|
|
) => {
|
|
const existing = await getExistingApproval(approvalId);
|
|
const currentUserRedactionOptions = {
|
|
enabled: (await instanceSettings.getGeneral()).censorUsernameInLogs,
|
|
};
|
|
const redactedBody = redactCurrentUserText(body, currentUserRedactionOptions);
|
|
return db
|
|
.insert(approvalComments)
|
|
.values({
|
|
companyId: existing.companyId,
|
|
approvalId,
|
|
authorAgentId: actor.agentId ?? null,
|
|
authorUserId: actor.userId ?? null,
|
|
body: redactedBody,
|
|
})
|
|
.returning()
|
|
.then((rows) => redactApprovalComment(rows[0], currentUserRedactionOptions.enabled));
|
|
},
|
|
};
|
|
}
|
|
|
|
export function approvalService(db: Db) {
|
|
const { getExistingApproval, resolveApproval, ...records } = approvalRecords(db);
|
|
async function reconcileApprovedBuiltInAgent(companyId: string, payload: Record<string, unknown>, database: Db) {
|
|
const sourceBuiltInAgentKey = typeof payload.sourceBuiltInAgentKey === "string" ? payload.sourceBuiltInAgentKey : null;
|
|
if (!sourceBuiltInAgentKey) return;
|
|
const { builtInAgentService } = await import("./built-in-agents.js");
|
|
await builtInAgentService(database).ensure(companyId, sourceBuiltInAgentKey);
|
|
}
|
|
|
|
async function decideHire(target: string | { approvalId: string }, status: "approved" | "rejected", userId: string, note?: string | null) {
|
|
const lifecycle = createAgentLifecycle(db);
|
|
const result = await (status === "approved" ? lifecycle.approveHire : lifecycle.rejectHire)(target, userId, note);
|
|
if (result?.hireApprovedAgentId && result.approval) {
|
|
await reconcileApprovedBuiltInAgent(result.approval.companyId, result.approval.payload, db);
|
|
void notifyHireApproved(db, { companyId: result.approval.companyId, agentId: result.hireApprovedAgentId,
|
|
source: "approval", sourceId: result.approval.id, approvedAt: result.approval.decidedAt ?? new Date() }).catch(() => {});
|
|
}
|
|
return result;
|
|
}
|
|
|
|
async function decide(id: string, status: "approved" | "rejected", userId: string, note?: string | null) {
|
|
const approval = await getExistingApproval(id);
|
|
if (approval.type === "hire_agent") {
|
|
const result = await decideHire({ approvalId: id }, status, userId, note);
|
|
return { approval: result!.approval!, applied: result!.applied };
|
|
}
|
|
return resolveApproval(id, status, userId, note);
|
|
}
|
|
return { ...records,
|
|
approveHire: (agentId: string, userId: string, note?: string | null) => decideHire(agentId, "approved", userId, note),
|
|
rejectHire: (agentId: string, userId: string, note?: string | null) => decideHire(agentId, "rejected", userId, note),
|
|
approve: (id: string, userId: string, note?: string | null) => decide(id, "approved", userId, note),
|
|
reject: (id: string, userId: string, note?: string | null) => decide(id, "rejected", userId, note),
|
|
};
|
|
}
|