Files
PaperClipAI/server/src/services/tool-access/activity/review-queue.ts
T
DottaandPaperclip d108aeeaaf fix(server): schedule pending tool-review expiry from committed changes (#17453)
## Thinking Path

> - Paperclip is the open source app people use to manage AI agents for
work.
> - Governed tools can wait for a human review before execution.
> - Pending reviews already save a deadline and must reject approval
after expiry.
> - The server currently scans for expired reviews on every maintenance
tick.
> - Empty scans keep the database active without advancing work.
> - This pull request schedules pending review expiry from committed
changes and saved deadlines.
> - Configured idle state and resolved history no longer cause this
expiry scan.

## Linked Issues or Issue Description

**What happened?**

The combined tool review sweep repeatedly queries pending expiry even
when no pending request has a deadline.

**What did you expect?**

Signing or resolving a request should update the expiry schedule. A
saved expiry should trigger settlement. Once no expiry remains, this
duty should stop querying.

**Steps to reproduce**

Start a claimed instance with heartbeat scheduling enabled. Leave it
idle with no pending tool reviews. Observe pending-review expiry
discovery in the maintenance sweep.

Stacked on #17450; earlier slices are #17446 and #17443. These all reuse
the delivery and transaction foundations from #15625. This PR owns
pending-review expiry only. Approved-action recovery,
interrupted-execution quarantine, and terminal interaction projection
repair remain separate migrations. No duplicate pending-review expiry
migration was found in the open PR search; credential lifetime changes
in #17221 and #17274 concern the parent credential slice.

## What Changed

- Replace repeated company review-deadline aggregates with an indexed
single-row lookup. Add and replay a concurrent covering-index migration
and generate combined snapshots throughout the stack.

- Extract pending-expiry selection and settlement into its owning
service. Keep provider execution and interaction publication with the
gateway.
- Signal deadline assignment and pending resolution inside the owning
SQL transaction. Preserve outer commit/rollback and central uncertainty
handling.
- Register company expiry deadlines with the shared scoped coordinator.
Reconstruct on startup; normal producer hints reload only their company.
Preserve writes during drain and concurrent reconciliation.
- Settle request expiry and invocation failure atomically. Keep
interaction projection recovery in the existing separate sweep.
- Remove pending-expiry scanning from the main tick. Preserve the
combined manual API and startup-only behavior when scheduling is
disabled.
- Add a partial pending-expiry index through an idempotent concurrent
migration. No queue table is added.
- Document supported producers, quiet states, remaining duties, and
external wake limits.

## Verification

- CI run `40574661575` at `f5c935d1b89877033eb82bf890f867baeb372ef3`
lost the runners for general-server shard 9/12 and E2E shard 2/8 at
20:09:03 UTC. Both job logs explicitly show a runner shutdown signal
followed by cancellation; no test assertion failure was reported by
those jobs. A single infrastructure retry is justified once the run
completes; it has not been issued yet. Both current-head reviews are
5/5, and CI must still be green before merge.

- Current master landing at `f5c935d1b89877033eb82bf890f867baeb372ef3`:
128 complete gateway expiry, startup, scoped deadline and
preparation-timing cases pass with no skips. Server types and the
production server build pass. Landed tracing changes from #17918 are
preserved. The master migration journal prefix remains semantically
identical, with only this PR's two migrations added. Fresh remote
CI/review is required before merge.

- Parent reconciliation at `17539e2a6cf8ac4922b3cbe06af6df361bfe5665`:
116 tests pass across the full tool gateway service, startup and scoped
deadline suites against a dedicated migrated PostgreSQL database. No
cases were skipped. Server typecheck passes. Fresh remote CI and review
are pending.

Current head: `ec28076f825e76a267f05517ff0c44a3e2283ce7`. This remains a
single-duty PR stacked on #17450.

- 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/ec28076f825e76a267f05517ff0c44a3e2283ce7/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

- All supported producers must share the database signal owner.
Independent SQL writers and other replicas need a live notification
contract; startup alone does not cover them.
- A crash after authoritative expiry and before interaction projection
still relies on the existing projection-repair duty. That duty,
approved-action recovery, and uncertain execution recovery remain
periodic in this slice.
- Unknown external outcomes remain quarantined. Provider calls stay
outside SQL transactions. Expiry is still checked before approving a
request even if physical cleanup waits during drain.
- The new index uses the existing top-level migration executor because
PostgreSQL concurrent index creation cannot run in a file-wide
transaction. Snapshot pruning follows the repository rule.
- This establishes worker quiet and reactivation, not deployed host
suspension. Existing Cloud idle-safety and external alarm coverage
remain unchanged.

## 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

Latest-head CI is now green after one evidence-backed retry. Greptile is
5/5 and all review threads are resolved.

---------

Co-authored-by: Paperclip <noreply@paperclip.ing>
2026-10-27 15:26:19 -05:00

199 lines
7.3 KiB
TypeScript

import { writeToolReviewChange } from "../../tool-review-work-notifications.js";
import { and, desc, eq, inArray } from "drizzle-orm";
import type { Db } from "@paperclipai/db";
import { toolApplications, toolActionRequests, toolCatalogEntries, toolConnections, toolInvocations } from "@paperclipai/db";
import type { ToolActionRequestListItem, ToolActionRequestStatus } from "@paperclipai/shared";
import { readSignedToolArgumentsPayload, TOOL_ACTION_REQUEST_SIGNING_GRACE_MS } from "../../tool-content-guards.js";
import { toToolActionRequest } from "./projection.js";
/** Pending requests stay hidden until signing completes; invalid signatures are cancelled. */
export function createToolActionReviewQueue(db: Db) {
return {
/**
* List "Ask first" action requests for the review queue, enriched with the
* connection/app context the prosumer card renders. Defaults to pending.
*/
listActionRequests: async (
companyId: string,
status: ToolActionRequestStatus = "pending",
): Promise<ToolActionRequestListItem[]> => {
const requests = await db
.select()
.from(toolActionRequests)
.where(
and(
eq(toolActionRequests.companyId, companyId),
eq(toolActionRequests.status, status),
),
)
.orderBy(desc(toolActionRequests.createdAt));
if (requests.length === 0) return [];
const invocationIds = [
...new Set(requests.map((request) => request.invocationId)),
];
const invocations = await db
.select()
.from(toolInvocations)
.where(
and(
eq(toolInvocations.companyId, companyId),
inArray(toolInvocations.id, invocationIds),
),
);
const invocationById = new Map(
invocations.map((invocation) => [invocation.id, invocation]),
);
let visibleRequests = requests;
if (status === "pending") {
// A pending request that the creator has not signed yet is still being
// set up. The gateway creates the row (signedArguments = null) and signs
// it in a second step, so a review-queue read can observe the row inside
// that window. Hide such a request from the queue, but do not cancel it —
// cancelling here races the two-step create and makes the later approve
// fail with action_not_pending. Only cancel a request that carries a
// signature we cannot verify (secret rotation or tampering), or an
// unsigned row whose creator has exceeded the signing grace period.
const unsignedRequestIds = new Set<string>();
const invalidRequestIds: string[] = [];
for (const request of requests) {
const invocation = invocationById.get(request.invocationId);
if (!invocation) {
invalidRequestIds.push(request.id);
continue;
}
if (request.signedArguments === null) {
if (
Date.now() - request.createdAt.getTime() >=
TOOL_ACTION_REQUEST_SIGNING_GRACE_MS
) {
invalidRequestIds.push(request.id);
} else {
unsignedRequestIds.add(request.id);
}
continue;
}
let readable = false;
try {
readable = Boolean(
readSignedToolArgumentsPayload({
signedArguments: request.signedArguments,
invocationId: invocation.id,
toolName: invocation.toolName,
}),
);
} catch {
readable = false;
}
if (!readable) invalidRequestIds.push(request.id);
}
if (invalidRequestIds.length > 0) {
await writeToolReviewChange(db, companyId, tx => tx
.update(toolActionRequests)
.set({
status: "cancelled",
resolvedAt: new Date(),
updatedAt: new Date(),
})
.where(
and(
eq(toolActionRequests.companyId, companyId),
eq(toolActionRequests.status, "pending"),
inArray(toolActionRequests.id, invalidRequestIds),
),
));
}
const hiddenIds = new Set([
...invalidRequestIds,
...unsignedRequestIds,
]);
if (hiddenIds.size > 0) {
visibleRequests = requests.filter(
(request) => !hiddenIds.has(request.id),
);
}
}
if (visibleRequests.length === 0) return [];
const visibleInvocations = visibleRequests
.map((request) => invocationById.get(request.invocationId))
.filter(
(invocation): invocation is typeof toolInvocations.$inferSelect =>
Boolean(invocation),
);
const connectionIds = [
...new Set(
visibleInvocations
.map((invocation) => invocation.connectionId)
.filter(Boolean),
),
] as string[];
const connections = connectionIds.length
? await db
.select()
.from(toolConnections)
.where(inArray(toolConnections.id, connectionIds))
: [];
const connectionById = new Map(
connections.map((connection) => [connection.id, connection]),
);
const applicationIds = [
...new Set(
connections
.map((connection) => connection.applicationId)
.filter(Boolean),
),
] as string[];
const applications = applicationIds.length
? await db
.select()
.from(toolApplications)
.where(inArray(toolApplications.id, applicationIds))
: [];
const applicationById = new Map(
applications.map((application) => [application.id, application]),
);
const catalogEntryIds = [
...new Set(
visibleInvocations
.map((invocation) => invocation.catalogEntryId)
.filter(Boolean),
),
] as string[];
const catalogEntries = catalogEntryIds.length
? await db
.select()
.from(toolCatalogEntries)
.where(inArray(toolCatalogEntries.id, catalogEntryIds))
: [];
const catalogById = new Map(
catalogEntries.map((entry) => [entry.id, entry]),
);
return visibleRequests.map((request) => {
const invocation = invocationById.get(request.invocationId);
const connection = invocation?.connectionId
? connectionById.get(invocation.connectionId)
: undefined;
const application = connection?.applicationId
? applicationById.get(connection.applicationId)
: undefined;
const catalogEntry = invocation?.catalogEntryId
? catalogById.get(invocation.catalogEntryId)
: undefined;
return {
request: toToolActionRequest(request),
toolName: invocation?.toolName ?? catalogEntry?.toolName ?? "",
toolTitle: catalogEntry?.title ?? null,
connectionId: connection?.id ?? invocation?.connectionId ?? null,
connectionName: connection?.name ?? null,
applicationName: application?.name ?? null,
riskLevel: catalogEntry?.riskLevel ?? null,
requestedByAgentId: request.requestedByAgentId ?? null,
};
});
}
};
}