mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-10-29 21:07:40 +01:00
## Thinking Path > - Paperclip manages work and human decisions for AI-agent companies. > - Automatic decision queues store membership for pending attention sources. > - The old attention maintenance scan created that membership as a side effect. > - Moving approval retention out of the scan also removed approval queue discovery. > - Queue API consumers do not build the attention feed first, so they can miss pending PR approvals. > - This pull request gives approval queue discovery its own worker and commit signals. > - The worker becomes quiet after it saves every missing membership. ## Linked Issues or Issue Description Refs #17497. Stacked on #17508, following #17505. This is a correctness dependency for the approval retention migration in #17497. Do not treat that migration as complete without this queue-population path. A real PostgreSQL regression found no PR queue when a pending approval had a linked PR work product and the maintenance feed excluded approvals. Related: #13364 proposes pruning stale seeded memberships. This PR preserves existing membership retention and does not implement pruning. #15149 documents the decision-queue APIs. ## What Changed - Add an approval queue worker that selects missing PR-queue memberships from existing durable records. Bound each pass to 100 rows. Continue past failed rows and revisit them on retry. - Reuse the queue service's seed writer through a normalized source contract. Keep queue rules, uniqueness, and audit writes in that service. Keep feed readers compatible. - Wake from committed approval and link changes, issue/company visibility, PR work-product changes and imports, and queue-rule changes. Reuse the existing transaction boundaries. - Recheck approval status under a row lock. Serialize queue-rule changes with the seed writer. Preserve required recovery during drain and shutdown. - Register startup reconstruction under the existing maintenance switch. Leave other attention-source seeding with its current owner until its own migration. - Document the complete producer audit and stack dependency. Add a validated external PostgreSQL option to the existing queue route suite so it runs on hosts without embedded database support. ## Verification Current tested revision: `7948136ef8`. This PR targets master after #17508 merged as `8406047b4e`. Normal ancestry reconciliation preserved the complete tested integration tree, before the review repairs below. Only the original 25 seeding paths differ from master, and database schema/migration metadata is identical. - **56 cases pass in the three complete affected suites:** 28 approval seeding, 6 queue routes and 22 task-watchdog cases. Server typecheck and production build pass. - Three real PostgreSQL retry regressions failed then passed: new matching arrivals cannot postpone a recovered failure indefinitely; successful partial progress resets old failure penalties; an empty cursor rollover does not reset outage backoff. Exact one- and two-second retry assertions, eventual complete recovery and existing capped-backoff/company isolation tests pass. - Three genuinely empty external databases failed before setup repair because tables were missing. Each suite now applies migrations itself. All 55 cases passed on a second set of fresh databases; adding the final rollover guard then passed all 56 cases using those now-migrated fixtures. No production migration or table is added. - The initial expanded fresh run passed 54 cases but failed a cleanup expectation that omitted a required cursor rollover pass. The fixture now permits that explicit continuation while preserving exact deadline and complete-membership assertions. All failure records remain in `doc/idle-approval-queue-seeding.md`. - The prior integrated revision `4e725e2055` passed **522 cases in fifteen complete producer/consumer suites**, plus types/build. Its unchanged consumer code and historical deadlock, cross-company amplification, backoff, gateway integration and setup evidence remain documented; those counts are not relabeled as current acceptance. Commands: server Vitest for approval queue seeding, queue routes and task watchdogs; server typecheck/build. Fresh exact-head Greptile/Ponytail reviews and all CI checks remain required. No full local repository-suite, deployed suspension or cost claim is made. ## Risks - Signals cover the supported in-process service writers. Independent SQL writers and multiple server replicas need a separate notification contract. - The source predicate must stay aligned with unscoped attention maintenance. The first linked issue, company scope, pending status, and execution visibility are tested. A pending approval itself keeps its linked external conversation active. - Existing enabled seed rules can re-add manually removed matching memberships. This preserves the old scan's behavior. Completed or archived membership is not pruned. - Queue seeding and retention are separate owners. Other attention sources still need their own event-driven discovery before the aggregate scan can be retired. - This PR does not extend Cloud wake coverage, relax sleep admission, or claim measured host/database 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>
232 lines
15 KiB
Markdown
232 lines
15 KiB
Markdown
# Approval queue seeding without retention polling
|
|
|
|
## Obligation and stack dependency
|
|
|
|
The attention maintenance scan does two different jobs: retention and automatic
|
|
queue membership. Its call to `materializeSeededQueues` adds pending approvals to
|
|
the PR queue when the first linked issue has a pull-request work product. Queue
|
|
API readers use saved membership; they do not first build the attention feed.
|
|
Removing approvals from maintenance therefore lost automatic queue population.
|
|
A real PostgreSQL regression reproduced an empty queue with a pending linked
|
|
approval and PR work product, without an attention reader.
|
|
|
|
This slice owns automatic PR-queue membership for approvals. It depends on the
|
|
approval producer boundaries introduced in #17497 and is required with that
|
|
retention migration before treating the combined behavior as complete. Join
|
|
requests have no issue association or matching seed rule. Other attention-source
|
|
seed discovery remains in the existing feed scan and must migrate with an
|
|
explicit owner before those sources leave that scan.
|
|
|
|
## Source of truth and ownership
|
|
|
|
Existing approvals, issue approvals, issue work products, queues, and queue items
|
|
are the only durable state. No new table, outbox, external API, or provider branch
|
|
is introduced. `approvalQueueSeedingService` selects missing memberships for:
|
|
|
|
- A pending approval in an active company.
|
|
- Its first linked issue, ordered by issue ID, in the same company and eligible
|
|
under the attention feed's execution-issue visibility predicate.
|
|
- A pull-request work product belonging to that issue and company.
|
|
- A missing PR queue or an enabled seed rule without this approval's membership.
|
|
|
|
Disabled rules, nonmatching approvals, completed approvals, and existing seeded
|
|
history are passive. A pending approval itself prevents its linked Slack/Speko
|
|
conversation from projecting as idle, so this source does not depend on every
|
|
other conversation-state producer. Hidden/harness/conversation-container issue
|
|
changes retain their explicit signals.
|
|
|
|
The queue service owns seed matching, creation, membership uniqueness, and audit
|
|
writes. The feed and worker use the same writer through a small normalized source
|
|
contract. The candidate lookup and writer share the pull-request product SQL
|
|
predicate, so the matching policy has one owner. A worker does not construct a presentation-layer AttentionItem. Queue
|
|
reads keep their existing source authorization checks. A queue-rule update and
|
|
seeding serialize on the existing queue row. Existing memberships are not pruned
|
|
when an approval resolves. This preserves current behavior; pruning is a separate
|
|
change, including the proposal in #13364.
|
|
|
|
## Producer audit
|
|
|
|
| Producer | Commit boundary and effect |
|
|
| --- | --- |
|
|
| Approval create, cancel, revision, resubmit, resolve | `writeApprovalChange` notifies retention and company-scoped queue seeding before writes in the owning transaction. Approve/reject/cancel retain their explicit tool-review notification; ordinary mutations do not wake those queues. |
|
|
| Shared approval records, budget approvals, formal tool approvals | The same approval notification covers their existing transactions. |
|
|
| Issue approval link, unlink, link-many, tool-created links | Existing approval transaction helper. Link removal can expose a different first issue. |
|
|
| Issue visibility changes and issue deletion | Existing approval notification before the update or cascading delete. Routine failure cleanup retains this signal. |
|
|
| Company activation, pause, archive, deletion and budget pause/resume | Existing company notification wakes both approval consumers. |
|
|
| Work-product create, type/issue update, import batch, delete | The owning work-product transaction notifies seeding when a pull-request row is involved. |
|
|
| PR queue creation, rule update and manual approval membership removal | The queue mutation transaction notifies seeding. An enabled rule can re-add a still-matching manually removed item, as the old scan did. |
|
|
|
|
Other direct work-product writes only create artifacts or update title, metadata,
|
|
or trust. They cannot create a pull-request match. Supported writes use these
|
|
in-process service boundaries. Independent SQL writers and multiple server
|
|
replicas are not covered by local transaction signals.
|
|
|
|
Retention-only keep/archive/revive changes do not create queue projection debt.
|
|
The old seed writer ran before retention filtering, so archived or kept approvals
|
|
still follow the same seeding rule. User-specific dismissals do not filter the
|
|
unscoped maintenance feed. No new deadline is inferred from those display states.
|
|
|
|
## Worker lifecycle
|
|
|
|
Startup reconstructs missing memberships across the instance. Each later committed
|
|
producer carries its company ID and wakes only that company's reconciliation.
|
|
Pending scopes coalesce in memory; one company's ordinary mutations cannot restart
|
|
a global scan. Scope rotation prevents one failing company from starving another. Each sweep processes at most 100 approvals, with an ID cursor and a fixed upper ID per scan cycle. The cursor advances past
|
|
failures and finishes that finite cycle before revisiting them; new arrivals cannot
|
|
extend it indefinitely. Each row is rechecked under
|
|
an approval lock before the queue writer runs. One bad row cannot permanently
|
|
hide later candidates. Atomic existing membership uniqueness prevents duplicate
|
|
rows and duplicate insertion audit events.
|
|
|
|
After a sweep, another lookup decides whether actual projection debt remains.
|
|
Unprocessed pages and newer committed signals retain a one-second continuation.
|
|
Failed scopes back off exponentially from one second to one minute. A successful
|
|
nonempty batch resets consecutive failures; an empty cursor rollover while debt
|
|
remains preserves outage backoff. New signals
|
|
do not reset a failing scope's backoff; other companies can proceed independently.
|
|
The next retry is registered with the shared scheduler, with no SQL during the
|
|
wait. A failed scope remains tracked until its owed memberships are reconciled. The generation check follows the final awaited lookup, so a commit arriving
|
|
during an empty read cannot be erased. Once reconciled, there is no deadline and
|
|
no recurring worker SQL. The shared coordinator preserves requested work during
|
|
drain. Passive pending checks inspect memory only; startup reconstructs from rows. Shutdown aborts between rows
|
|
and waits for an admitted transaction; unprocessed rows remain recoverable.
|
|
Registration follows the existing heartbeat-maintenance enablement switch.
|
|
|
|
This slice does not extend Cloud alarm coverage or loosen host sleep admission.
|
|
It proves a worker's quiet/reactivation behavior, not deployed host or database
|
|
suspension. The remaining attention maintenance interval is still present for
|
|
other source owners.
|
|
|
|
## Verification
|
|
|
|
- Before the fix, the PostgreSQL regression found no PR queue after the normal
|
|
maintenance path excluded approvals. After the fix, the queue API finds it
|
|
without a full attention read.
|
|
- Twenty-one real-database worker tests cover empty-to-active-to-quiet behavior,
|
|
startup and completed history, actual approval/link/work-product producers,
|
|
batch import, first-link selection, company isolation, hidden targets, queue
|
|
rule enablement, company activation, resubmission, outer commit and rollback,
|
|
first write during drain, database failure through drain, multiple pages,
|
|
failed-page progress and retry, concurrent resolution, archived/kept history, shutdown/restart, and an admitted transaction at shutdown.
|
|
- 115 existing tests pass across queue routes, attention, approval retention,
|
|
approval service, work products, and startup wiring. The shared writer still
|
|
passes existing plan/question/PR seed tests and authorization tests.
|
|
- Workspace typechecks and the production build pass. Fresh PR checks are
|
|
tracked in the PR and must pass before readiness is claimed.
|
|
- The actual pending lookup on 40,000 completed approvals plus one pending PR
|
|
approval uses the existing partial pending-retention index. The probe runs in
|
|
a rolled-back transaction; execution took 0.094 ms on this local fixture. No new index or migration is needed for this case.
|
|
|
|
The first expanded test run caught two fixture errors: a hand-inserted queue
|
|
omitted required creator attribution, and a failure test expected no idle hold
|
|
while committed projection work was still unresolved. The corrected test waits
|
|
for the active attempt to finish and checks passive SQL directly while preserving
|
|
that required hold. Neither required weakening the worker's recovery contract.
|
|
|
|
Review found a queue/item lock inversion between seeded insertion and manual
|
|
membership changes. A two-connection PostgreSQL regression reproduced `deadlock
|
|
detected`. Manual add/remove now lock the queue before touching its items, matching
|
|
the seed writer. The regression and the existing queue route suite pass (27 cases).
|
|
Server typecheck and build pass after the repair. Fresh-head CI/review is required.
|
|
|
|
Security review found that normal producer wakes could restart a cross-company
|
|
scan. The regression reproduced another company's queue being populated by an
|
|
unrelated company write. Producers now include company hints, including batch
|
|
imports and nested transactions. A separate controlled PostgreSQL test holds the
|
|
final empty read while a real producer commits and verifies the retained wake.
|
|
|
|
After company scoping, all 169 selected cases pass across approval queue seeding,
|
|
approval retention, queue routes, approval services, work products, and issue
|
|
comment routes. Server typechecking passes. The preceding run suffered timeouts
|
|
and a consequent cleanup foreign-key error; it is not counted as passing. The
|
|
route suite separately exposed cold imports inside timed tests. Moving those
|
|
imports to suite loading preserves assertions and prevents timed-out requests
|
|
from leaking into later mock state. The watchdog fixture collision from CI uses
|
|
monotonic identifiers instead; its 19 real-PostgreSQL cases pass.
|
|
|
|
The next CI run passed the security gate and the two originally failing shards.
|
|
Its remaining server shard caught a historical cleanup test checking 33 provider
|
|
inspections after only 32 had completed. The test advanced fake time before the
|
|
previous asynchronous page had necessarily published its next deadline. It now
|
|
waits for all 33 inspections, then retains the zero-further-work assertions. All
|
|
14 environment cleanup cases pass on real PostgreSQL; production code is unchanged.
|
|
|
|
|
|
Further review reproduced 120 failed transactions in two simulated minutes for a
|
|
persistently failing projection. Per-scope backoff now bounds that test to at most
|
|
eight attempts, even with new signals every second. Removing the fault recovers
|
|
the membership. A real coordinator/producer case proves thirty seconds without
|
|
SQL during backoff, another company's successful queue seeding during that wait,
|
|
and eventual recovery without losing the original work. All 25 worker cases pass.
|
|
The first consumer command omitted the external DB setting for queue routes and
|
|
skipped those six cases; the corrected command executes and passes all six.
|
|
The shared PR predicate is exercised by both those writer tests and worker tests.
|
|
Server typecheck and build pass; fresh remote review/checks remain required.
|
|
|
|
The final startup, approval-retention, and attention consumer run passes all 93
|
|
cases. With the 25 worker and six queue cases, 124 affected tests pass.
|
|
|
|
|
|
## Current retention-parent integration
|
|
|
|
The seeding worker reuses `firstApprovalIssueId` from the approval-visibility
|
|
owner rather than retaining its own first-link SQL. Company IDs are part of the
|
|
approval notification contract because projection reconciliation is scoped.
|
|
Generic approval writes notify retention and seeding only; the existing terminal
|
|
approve/reject/cancel owners alone notify tool-review consumers. Current precise
|
|
issue-deletion target notifications, unseen retention controls, finite retention
|
|
cycles and the real source-row lock regressions are preserved.
|
|
|
|
All 522 cases in fifteen complete suites pass at `4e725e2055`, with server
|
|
typecheck and production build. This includes seeding, queue routes, work products,
|
|
issue comments, both retention workers, approvals, attention, retention controls,
|
|
budgets, tool gateway, startup, scoped deadlines, environment cleanup and task
|
|
watchdogs. Four new disposable fixtures were cloned from the already migrated
|
|
parent fixture; this is not a fresh-empty-schema claim. The other suites used
|
|
separate existing disposable databases. No database was shared with another
|
|
concurrent local test run. The subsequent parent merge changes only two documents;
|
|
all tested runtime and acceptance sources remain byte-identical.
|
|
|
|
The first integration typecheck caught two incorrect gateway company references.
|
|
They now use the existing session company, and typecheck plus the complete gateway
|
|
suite pass. Earlier deadlock, cross-company wake, backoff, skipped-fixture and
|
|
teardown failures remain in the historical sections above. The parent's attention
|
|
cleanup timeout and expiry-test CI assertion are recorded in its document; this
|
|
larger current run passes without changing their assertions or timeouts. Fresh
|
|
exact-head reviews and CI remain required before this dependent PR can merge.
|
|
|
|
|
|
## Final master and review repairs
|
|
|
|
Join retention merged as `8406047b4e`. The normal master reconciliation first
|
|
proved that landed tree equals the existing parent. Conflict resolution preserved
|
|
the complete tested seeding tree; it also removed a duplicate helper introduced
|
|
by automatic merge. The PR again differs from master in only its original 25
|
|
seeding paths. Database schema and migration metadata are identical to master.
|
|
|
|
Three retry regressions failed before repair: ongoing arrivals skipped a recovered
|
|
membership, a partial successful batch retained the old sixty-second failure
|
|
penalty, and resetting that counter on an empty cursor rollover weakened outage
|
|
backoff. The worker now fixes the upper ID per cycle and resets consecutive
|
|
failures only on real successful progress or complete reconciliation. Exact
|
|
one-second recovery and two-second consecutive-failure assertions pass; repeated
|
|
signals still cannot reset a failing scope's delay. No new table or retry wrapper
|
|
is introduced.
|
|
|
|
The external seeding, queue-route and watchdog fixtures now apply pending
|
|
migrations before creating their client. Each of three empty-database regressions
|
|
failed with missing tables before this setup repair. All 55 cases then passed on
|
|
a second set of genuinely empty databases, with the suites applying their own
|
|
migrations. The final additional rollover guard brings the three complete suites
|
|
to 56 passing cases (28 seeding, 6 queue-route, 22 watchdog), using those now
|
|
migrated fixtures. Server types/build pass at `7948136ef8`. The earlier 522-case
|
|
integration run remains evidence for its stated revision and unchanged consumers;
|
|
it is not relabeled as a current full-suite run.
|
|
|
|
The first expanded fresh run passed 54 cases and failed the new backoff test's
|
|
cleanup expectation: after an entirely failed full page, a separate empty pass
|
|
ends the cycle before the next pass revisits rows. The corrected fixture permits
|
|
that explicit continuation and still checks complete recovery and the exact retry
|
|
deadline. Earlier failures remain recorded. Fresh final-head CI and reviews are
|
|
required before merge.
|