Files
PaperClipAI/doc/idle-tool-review-expiry.md
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

9.3 KiB

Pending tool-review expiry and idle behavior

Pending tool action requests already save their review deadline in expiresAt. This worker replaces the recurring pending-expiry scan with committed changes, startup reconstruction, and that saved deadline. It does not add a queue table or change the one-hour review policy.

Ownership and producer inventory

The tool gateway owns request signing and authorization. toolReviewExpiryService owns pending-expiry selection and settlement. registerToolReviewExpiryWork supplies company-scoped operations to the existing shared scoped deadline worker. Startup wires that contract without interpreting tool or provider behavior.

The supported writers are:

  • tool-access-policy.ts inserts an unsigned pending request without an expiry. It creates no expiry obligation yet. The existing signing grace remains intact.
  • The normal agent gateway signing path and the board test-call path assign the saved deadline. Each signals in its SQL transaction before that update.
  • Gateway signing failures, competing-call invalidation, invalid signatures, and legacy pending-to-approved/expired transitions signal before removing a pending obligation.
  • commitToolActionReview signals inside its existing human-resolution transaction. The issue/interaction locks, human authority checks, and delivery enqueue remain owned there.
  • Administrative interaction resolution signals in the caller-owned transaction before resolving linked requests. Review-queue invalidation signals before invalidating unreadable or unsigned requests.
  • Company deletion signals its company before removing saved review obligations.
  • The expiry worker signals before its own settlement. A compare-and-set against pending status and the observed update time protects concurrent resolution.

Every notification requires the company ID. Normal commits reload only that company's saved minimum expiry; due cleanup selects only that company. Startup reconstructs all saved company deadlines once. A company with only unsigned null-expiry requests has no deadline.

Only the SQL mutation is wrapped. Signing, provider requests, telemetry, and interaction publication are not moved inside an enclosing recovery transaction. The database signal layer handles nested savepoints, outer commit/rollback, and uncertain transaction outcomes. No recovery object is passed through callers. Independent SQL writers and other database owners/replicas still require a live notification contract. Startup reconstructs saved rows but is not ongoing cross-process discovery.

State and recovery

Expiry updates the authoritative request and invocation in one transaction. If invocation settlement fails, the request remains pending and the worker retries. After commit, the gateway reflects the result in the linked interaction. The existing terminal projection-repair duty remains in sweepActionReviews and repairs a crash or failure between authoritative settlement and projection. That duty is a separate migration; this worker must not claim it has completed.

The public/manual sweepActionReviews() still includes expiry by default. Server startup and the recurring legacy review sweep pass includeExpiry: false when expiry has its own startup pass or worker. Approved-action recovery, interrupted execution quarantine, and terminal projection repair retain their behavior. An unknown external outcome is never replayed by this expiry worker.

The next deadline comes from pending rows only. Startup uses the existing partial expiry index to reconstruct company deadlines. A company notification uses an ordered, single-row lookup backed by the partial (company_id, expires_at) index, rather than aggregating that company's pending backlog. Null-expiry unsigned requests and retained resolved history do not keep this worker active. The concurrent index migration uses the existing top-level executor and is safe to apply again after interruption.

The shared coordinator coalesces changes and preserves a write arriving during reconciliation. Each due pass expires at most 100 rows for one company; companies rotate between passes until the backlog drains. Failed scopes retry independently with exponential backoff from 30 seconds to five minutes. Other companies retain their own deadlines. New writes do not erase a failed sweep's backoff. Once reconciliation completes with no deadline, it has no recurring SQL. Drain retains first writes without passive database scans; resume processes overdue reviews. Startup reconstructs persisted deadlines. Shutdown stops subscription and timer ownership and joins active work.

Approval still rejects an expired request synchronously before physical cleanup. With heartbeat scheduling disabled, startup-only expiry remains as before.

Verification scope

Real PostgreSQL tests record SQL to prove signing, human decisions, and due expiry remain company-scoped. They also verify five-second and earlier replacement deadlines, company deletion, and bounded multi-pass draining. Existing cases cover normal gateway signing and human decisions, configured idle and terminal history, outer commit/rollback, startup/restart, drain/resume, concurrent producer/read, unsigned rows, multi-page expiry, and injected invocation-write failure with atomic rollback and retry. Existing service tests cover approval/execution, interrupted outcomes, and projection repair through the compatible manual entry point. Startup tests cover wiring. The external test database override requires a disposable database ending in _test; the suite deletes fixtures and stops the worker before database cleanup.

This is one worker migration, not proof that the instance can sleep. Other tool review duties remain periodic. Existing Cloud sleep-safety gates and external alarm coverage are unchanged. An in-process deadline cannot wake a stopped host.

Scoped lookup review repair (2026-10-27)

The original scoped aggregate examined 12,004 rows to return one deadline in a real PostgreSQL regression. The repaired lookup returns the earliest non-null pending expiry for the requested company with ORDER BY expires_at LIMIT 1. The grouped startup inventory remains unchanged. Existing company signals, authority checks, expiry settlement, paging, backoff, and sleep gates remain owned by their existing services.

The additive company-expiry index is migration 0383_tool_review_company_expiry. It builds the index concurrently and idempotently through the existing top-level migration executor. Initially reserving number 0402 passed per-branch checks but caused CI to reject descendants whose migrations preceded their target branch. A local 0380 correction fixed that ordering, but master had meanwhile added its own 0378 and 0379 migrations. The integrated stack preserves master's history, renumbers its unmerged migrations after 0379 in PR dependency order, and preserves every migration's SQL bytes. Exact filename references and journal tags/indices follow the new names. Stack journal timestamps remain ordered after master's latest entry. Each generated snapshot includes both master and that PR's schema.

Each PR must pass the exact target-branch migration-order gate, in addition to the existing numbering, safety, five-snapshot retention, and schema-drift checks. Local branch checks alone did not establish the stack's append-only ordering. Fresh application, upgrades from the previous numbered task databases, and replay also require verification. These unmerged stack migrations have not changed the SQL of migrations already on master.

Two failures shaped the repair. The first migration attempt omitted its explicit top-level registration and PostgreSQL rejected the concurrent build inside a transaction; adding the registration fixed application and replay. An initial three-column index also lost to the global expiry index in the planner, which filtered 12,001 foreign-company rows before returning a result. The final index omits the unused request ID. With 12,000 additional pending rows in each of two companies, including earlier foreign deadlines, all three measured reloads examine one row. The assertion counts filtered rows as well as returned rows. This is evidence for the recorded fixture and query plan, not a production latency guarantee or a claim that PostgreSQL can never choose another plan.

The complete gateway service, gateway acceptance, credential expiry, startup, custom-image deadline, and schema snapshot suites pass: 223 tests across six files. The run also exposed a credential test-clock overshoot; its correction belongs to the earlier gateway PR and waits for real SQL before advancing to an actually armed follow-up. Commit/rollback and exact expiry assertions remain. Server typechecking passes. Combined-stack migration upgrades, full build, and fresh remote checks remain required. Full local test:run is still unverified because of embedded PostgreSQL startup/capacity limits; real gateway suites used isolated, migrated test databases in the task-owned PostgreSQL instance.

A later review found an unbound query name in the SQL-capture assertion. The due-row predicate short-circuited before that expression in the passing fixture, and server typechecking excludes test sources. The assertion now independently requires both the due expiry scan and the non-null deadline lookup through each captured read.query. The earlier passing run did not detect this name error; acceptance must include the service suite with both assertions.