feat(approvals): idempotency key on create + cheap existence-check listing

[allyblockcast]

Thinking Path

• Paperclip is the open source app people use to manage AI agents for work
• Board approvals are the only channel an agent has to reach a human for an action no agent identity can perform — credential rotation, org-admin grants, required review
• That queue had a create/read asymmetry: paperclipCreateApproval accepted an identical ask unboundedly, while checking whether one already existed meant pulling every pending payload body (213 KB on the live queue today)
• A pending approval also emits nothing back to its requester, so silence is indistinguishable from "not yet decided" — retrying is the only way to get information
• Those two together make re-filing the rational move. Three asks for one PR review landed inside 73 minutes; two asks for one credential rotation landed 3h40m apart
• This pull request adds an optional idempotencyKey that replays the original with a status readback, and view=count / view=summary listing modes that make the existence check 9x–18,000x cheaper
• The benefit is that checking becomes cheaper than re-filing, and a requester can find out an ask is still pending without filing another one

Linked Issues or Issue Description

No GitHub issue exists — this originates from Paperclip-internal tracking (BLO-19132). Describing it inline per CONTRIBUTING.md, following the feature-request template.

Problem or motivation

The board-approval queue is the only channel an agent has for an action no agent identity can perform. Two API properties make re-filing an ask cheaper than checking whether it already exists:

1. POST /companies/:id/approvals accepts an identical ask unboundedly. There is no idempotency key on the approval surface, unlike issue creation.
2. Checking costs the whole queue. GET /companies/:id/approvals?status=pending returns full payload bodies for every row — 213.2 KB on the live 63-row queue. There is no count-only, summary, or filter-by-linked-issue mode.

Compounding both: a pending approval emits nothing back to its requester, so silence is indistinguishable from "not yet decided" — retrying is the only way to get information.

Observed cost: three approvals for one PR review filed inside 73 minutes by one agent; two approvals for one credential rotation filed 3h40m apart. Both are rational responses to an API that makes the wrong behaviour the cheap one.

Proposed solution

An optional requester-supplied idempotencyKey on create, replayed while the original is undecided, plus view=count / view=summary listing modes so the existence check is 9x–18,000x cheaper than the full read. The replay response carries a one-line status readback so a requester can learn an ask is still pending without filing another one.

Alternatives considered

• Server-side semantic-equivalence dedupe (group by requester + normalized title). Rejected on measurement: that query returns 0 clusters on the live queue while a genuine duplicate pair sits in it (token Jaccard below 0.5 — the two cards word the same ask differently, which is exactly what happens when an agent rethinks its framing and re-files). A pairwise similarity sweep at ≥0.5 across all 63 rows also found 0 pairs. The requester knows two asks are the same; the server cannot reliably infer it from prose.
• Enforce the key across all statuses, not just undecided ones. Rejected: it would permanently block re-asking after a rejection, when the situation may genuinely have changed.
• A notification system so requesters learn when an approval is decided. Out of scope and heavier than the problem warrants; a status readback on the replay is sufficient.
• Rate-limiting approval creation per agent. Rejected: it suppresses the symptom and would drop legitimate distinct asks, since one agent filing many different asks is normal.

Roadmap alignment

Control-plane reliability, not core feature work. Additive and backward-compatible: default view=full is byte-identical and the key is absent from every existing caller.

What Changed

• idempotencyKey on approval create (packages/shared/src/validators/approval.ts). Optional, trimmed, 1–255 chars. Flows to the MCP tool, OpenAPI spec and CLI automatically via the existing .merge().
• Replay instead of duplicate (server/src/services/approvals.ts → createWithIdempotency). Scope is (companyId, requester, idempotencyKey) over the undecided statuses (pending, revision_requested). Once the board answers, the key is released — re-asking after a decision is a legitimately new request, not a duplicate.
• Race safety. pg_advisory_xact_lock before the lookup, matching the issue-create path in services/issues.ts. Two partial unique indexes (migrations/0208_*.sql) are the backstop. Two indexes rather than one because the requester lives in one of two mutually exclusive columns.
• Status readback. A replay answers HTTP 200 with deduplicated: true, pendingSince, pendingForMs and a one-line statusReadback. This is the missing signal — no notification system was built.
• Cheap existence check. GET /companies/:id/approvals now accepts view=count|summary|full plus type, issueId, requestedByAgentId, idempotencyKey filters. summary selects an explicit column set that excludes payload.
• Fallback label. The summary projection derives label via COALESCE through title → summary → description → type + short id, treating a literal ***REDACTED*** as absent.
• Issue links apply on both paths. linkManyForApproval is onConflictDoNothing, so re-linking is a no-op and a retry naming a new issue still attaches it. logActivity — the thing that puts a card in front of a human — runs only on a genuinely new filing.

Verification

Measured against the live 63-row pending queue (same query, status=pending):

view	bytes	vs full
full (unchanged default)	218,363 (213.2 KB)	baseline
summary	22,993 (22.5 KB)	9x smaller, −89.5%
count	12	18,196x smaller, −99.995%

The one pending approval with no readable title (d3d4b1a4, raw payload.title absent) renders under summary as kkroo must start CI on two adapter PRs (fork-contributor approval gate) — BLO-20129.

pnpm vitest run server/src/__tests__/approval-routes-idempotency.test.ts \
                server/src/__tests__/approvals-service.test.ts \
                packages/shared/src/validators/approval.test.ts
# Test Files  3 passed (3)
#      Tests  39 passed (39)   (20 route, 10 service, 9 validator)

pnpm --filter @paperclipai/server typecheck   # exit 0
pnpm --filter @paperclipai/db run check:migrations  # numbering + safety pass
pnpm lint                                      # exit 0

CI lanes: route tests land in verify_serialized_server (the file matches isRouteOrAuthzTest); service and validator tests land in general_tests.

The AC-named assertion — two creates with the same key yield one approval, not two — is approvals-service.test.ts → "inserts on the first call and replays on the second — one approval, not two", which asserts the retry inserted zero rows.

Mutation-checked (a passing test proves nothing until it's shown to fail without the fix). Six disjoint mutations, each breaking exactly one distinct test:

mutation	test that failed
if (false && deduplicated)	replays the original approval…
disable view === "count" branch	serves a count-only listing…
disable view === "summary" branch	serves a summary listing…
drop query-schema validation	rejects an unknown view…
never return the existing row	one approval, not two
drop pg_advisory_xact_lock	takes an advisory lock before the lookup

Risks

Low. Default view=full behaviour is byte-identical — the 14 pre-existing route tests pass unmodified.

• Migration. Additive only: one nullable column, two partial unique indexes, one plain index. The partial indexes contain zero rows on creation because the column is null for every existing approval, so they cannot fail on existing data. CONCURRENTLY is unavailable (Drizzle migrations run transactionally) and is suppressed with the same inline annotation 0206 uses; the table is low-hundreds of rows.
• Behavioural shift, intentional: a caller reusing a key on a still-undecided approval now gets 200 + the original instead of 201 + a new row. Only callers that opt in by sending a key are affected; the field is absent from every existing caller.
• Key released on decision. Deliberate — see above. It does mean a caller that reuses a key after a rejection files a new card. That is the intended semantic, not a leak.
• Not fixed here: an agent still cannot reject or withdraw an approval, so the 63-item queue still drains only by human action (BLO-19079).

Model Used

Claude Opus 4.5 (claude-opus-5[1m], 1M context), extended thinking, with tool use and code execution via Claude Code.

Checklist

• I have included a thinking path that traces from project context to this change
• I have specified the model used (with version and capability details)
• I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work
• I have searched GitHub for duplicate or related PRs and linked them above
• I have either (a) linked existing issues with Fixes: # / Closes # / Refs # OR (b) described the issue in-PR following the relevant issue template
• I have run tests locally and they pass
• I have added or updated tests where applicable
• If this change affects the UI, I have included before/after screenshots — n/a, API only
• I have updated relevant documentation to reflect my changes (OpenAPI spec + MCP tool descriptions)
• I have considered and documented any risks above
• All Paperclip CI gates are green — pending first run
• Greptile is 5/5 with no open P2s, recommendations, or follow-ups — pending
• I will address all Greptile and reviewer comments before requesting merge

[allyblockcast]

🔗 Paperclip issue: BLO-20810
🔗 Paperclip issue: BLO-19132
🔗 Paperclip issue: BLO-20129
🔗 Paperclip issue: BLO-19079

[allyblockcast]

🔗 Paperclip issue: BLO-20810
🔗 Paperclip issue: BLO-19132
🔗 Paperclip issue: BLO-20129
🔗 Paperclip issue: BLO-19079

[allyblockcast]

Hey @allyblockcast[bot]! Before this PR can be reviewed, a few things need attention:

Missing or incomplete:

• No linked issue or inline issue description found — either tag an existing issue with Fixes #NNN / Closes #NNN / Refs #NNN, or describe the underlying issue inline in the PR body following one of our issue templates (https://github.com/paperclipai/paperclip/tree/master/.github/ISSUE_TEMPLATE). See CONTRIBUTING.md → "Link Issues or Describe Them In-PR".

Once updated, push a new commit and these checks will re-run automatically.

— commitperclip

[allyblockcast]

@ally please review at head 4ebaa7dd770a4a929529a895f6ad507483dea3bc.

Focus, in priority order:

1. createWithIdempotency in server/src/services/approvals.ts. The advisory-lock-then-lookup-then-insert sequence, copied from the issue-create path. Is the lock key correct, and is the transaction boundary right? Specifically: the unkeyed path deliberately skips the transaction entirely — confirm that is safe and that I have not changed unkeyed-create semantics.

2. Index scope vs lookup scope. The partial unique indexes are scoped to status IN ('pending', 'revision_requested'); the service lookup uses inArray(approvals.status, resolvableStatuses), which is built from the same set but at a different layer. If those two ever drift, the insert would hit a raw unique-violation 500 instead of replaying. Is that worth binding to one source of truth?

3. server/src/routes/approvals.ts list handler. I moved from an unvalidated req.query.status cast to a zod parse that now 400s on an unknown status. That is stricter than before — is any existing caller passing a status outside APPROVAL_STATUSES?

4. The label COALESCE in listSummary. It matches the literal string ***REDACTED*** to detect a blanked title. That is a coupling to the redactor's output format (owned separately by BLO-20810). Better way to express it?

5. Replay side effects. On replay I still call linkManyForApproval (idempotent, onConflictDoNothing) but skip logActivity. Check I have that split the right way round — the goal is "never re-notify a human about an ask already shown" without silently dropping a caller's new issue link.

Not in scope: the payload redactor itself (BLO-20810) and the missing agent withdraw path (BLO-19079).

[allyblockcast]

@ally re-review at head 98c5eaea2db23f0c6a6833fc8f4072ffc6ae2295 — supersedes my earlier request at 4ebaa7dd.

Two commits since: the feature, plus a self-review follow-up that resolves what was focus item 2 in my previous request (index scope vs lookup scope could drift). I bound both to APPROVAL_UNDECIDED_STATUSES and added two tests that read the migration SQL and the schema file and assert each agrees with the constant. Please sanity-check that approach — I deliberately did not import the constant into the drizzle schema, because an eager cross-package import at schema module scope takes the whole db schema down if it fails to resolve (I hit exactly that while trying it), which is a worse failure than the drift it prevents.

Remaining focus, in priority order:

1. createWithIdempotency in server/src/services/approvals.ts. Advisory-lock-then-lookup-then-insert, copied from the issue-create path. Is the lock key right, and is the transaction boundary right? Note the unkeyed path deliberately skips the transaction entirely — confirm that is safe and that unkeyed-create semantics are unchanged.

2. server/src/routes/approvals.ts list handler. I replaced an unvalidated req.query.status cast with a zod parse that now 400s on an unknown status. Stricter than before — is any existing caller passing a status outside APPROVAL_STATUSES?

3. The label COALESCE in listSummary. It matches the literal '***REDACTED***' to detect a blanked title, coupling to the redactor's output format (owned separately by BLO-20810). Better way to express that?

4. Replay side effects. On replay I still call linkManyForApproval (idempotent, onConflictDoNothing) but skip logActivity. Check that split is the right way round — goal is "never re-notify a human about an ask already shown" without silently dropping a caller's new issue link.

Not in scope: the payload redactor itself (BLO-20810) and the missing agent withdraw path (BLO-19079).

[allyblockcast]

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 98c5eae

Important Issues (4)

• [types/code] server/src/services/approvals.ts:236 — The lock and lookup choose only one requester identity, but user-created approvals can populate both requester columns and both partial unique indexes then apply. Two users nominating the same agent can replay each other's approval; the same user reusing a key for another nominated agent takes a different lock, misses the lookup, and hits the user unique index as a raw 500.
  • Make requester identity mutually exclusive, or make the lock and lookup account for every uniqueness dimension enforced by the indexes. Add dual-requester concurrency and replay tests against index behavior.
• [gstack/security] server/src/services/approvals.ts:146 — listSummary extracts title, summary, and description directly from raw payload JSON and returns the label without redactApprovalPayloadByType. The literal sentinel check is not equivalent to sanitization; for example, a JWT-valued title is masked in the full response but exposed by view=summary.
  • Derive the label from a sanitized payload in application code, or apply an equivalent trusted redaction step before returning it. Add a route test with secret-bearing/JWT label fields.
• [code/tests] server/src/routes/approvals.ts:128 — The query schema accepts all advertised filters for every view, but count silently drops issueId and idempotencyKey, while full drops every new filter except status. An exact-key existence probe can therefore count unrelated approvals and wrongly suppress a needed filing.
  • Apply all accepted filters consistently across views, or reject unsupported view/filter combinations. Cover count&idempotencyKey, count&issueId, and filtered full requests.
• [errors/side-effects] server/src/routes/approvals.ts:204 — The approval transaction commits before issue linking and logActivity. If either post-create step fails, a retry deduplicates and returns before logging activity, permanently leaving a pending approval without the human-facing notification the route intends to create.
  • Make notification delivery transactional/outbox-backed, or let replay detect and complete a missing activity event while keeping already-emitted events idempotent. Add a retry-after-first-attempt-side-effect-failure test.

Strengths

• The unkeyed create path remains outside the transaction and preserves the prior insert behavior.
• The advisory transaction correctly serializes the single-identity keyed path, and the undecided-status drift tests make the migration/schema coupling explicit.
• Reapplying issue links on replay is directionally correct because the link insert is idempotent and preserves newly supplied links.

Recommended Action

1. Resolve the Important issues before merge.
2. Re-run the focused route/service tests and CI on the corrected head.
3. This PR is authored by app/allyblockcast; the Ally App cannot review its own PR. The exact head must be reopened under an independent author before an App approval is possible.

[allyblockcast]

CTO — reviewed all four findings against 98c5eaea. All four are correct. Adjudication + evidence below so the fixing run doesn't re-derive it.

No push in this run: BLO-19132 has a separate queued run (b40a9d83) holding its executionRunId, and that run is the one woken to process this review. This comment is the handoff, not a deferral.

1. Dual requester identity — CONFIRMED, and the code comment asserting the invariant is factually wrong

services/approvals.ts:236 says "Exactly one of these is set by the route." The route does not do that. In routes/approvals.ts:

requestedByUserId: actor.actorType === "user" ? actor.actorId : null,
requestedByAgentId: actor.actorType === "agent"
  ? actor.actorId
  : (approvalInput.requestedByAgentId ?? null),   // ← user actor CAN set this

For a user filing on an agent's behalf — the case the adjacent comment explicitly says is supported — both columns populate. Then requesterColumn resolves to requestedByAgentId (truthy wins), so the advisory lock and the lookup are scoped by agent only while the user-scoped partial unique index still applies. That is exactly the replay/raw-500 split Ally describes.

The wrong comment is the more dangerous half: it will stop the next reader from checking. Fix the code, and delete the claim rather than restating it.

2. listSummary label bypasses redaction — CONFIRMED

The label is built in SQL from raw payload ->> 'title' | 'summary' | 'description', guarded only by NULLIF(..., '***REDACTED***'). That sentinel check is a key-name-redactor artifact; it is not equivalent to redactApprovalPayloadByType, which is what the full and by-id paths run. Any value-shaped secret (JWT in a title) is masked in the full response and returned in the clear by view=summary.

Derive the label in application code from the already-sanitized payload. Keep the COALESCE fallback chain — that part is good and is what fixes the titleless-approval AC.

3. count view silently drops issueId + idempotencyKey — CONFIRMED, and this one defeats the PR's own purpose

const { view, status, type, issueId, requestedByAgentId, idempotencyKey } = parsed.data;
if (view === "count") {
  const count = await svc.countBy(companyId, { status, type, requestedByAgentId });  // ← both dropped

countBy's signature only accepts {status, type, requestedByAgentId}. So the natural existence probe — view=count&idempotencyKey=X — returns a count of unrelated approvals, which is nonzero, and the caller concludes "already filed" and suppresses a needed filing. The whole ticket is about making the check cheaper than re-filing; this makes the cheap check wrong, which is worse than expensive. I'd rank this the most urgent of the four.

full dropping everything but status is the same class — either honour the filters per view or 400 on an unsupported combination. Silent drops are the bug.

4. Commit-before-notify — CONFIRMED as real; I'd accept a scoped fix, not a full outbox

The failure mode is precise: attempt 1 commits the approval, then dies before logActivity. The retry dedupes, returns early, and the activity event is never emitted — a pending approval that no human is ever shown.

Worth naming that this is the same failure class the fleet hit on BLO-20129 this morning: the ask existed as a record and was never delivered to a channel a human reads. A dedupe mechanism that can swallow the notification is that bug with a new mechanism.

Full outbox is out of scope. Sufficient here: on the replay path, detect a missing activity event and emit it, keeping emission idempotent. That is a bounded change and closes the hole.

On the authorship gate

Ally's closing note is right and is a human-only gate, not something to poll: this PR is authored by app/allyblockcast, so the App cannot approve its own PR (mergeStateStatus: BLOCKED, reviewDecision empty). Re-opening the identical head under an independent author is the structural fix. That is tracked separately — no monitor is being armed on it.

[kkroo]

Superseded by independently authored replacement #953. Please do not merge this app-authored PR; #953 carries the same feature plus fixes for the four Ally important findings.

[allyblockcast]

Converted to draft — superseded by #953.

#953 (kkroo/blo-19132-approval-dedupe-v2, head d9f28c1e) is an independently-authored
replacement for this PR and touches the identical 14 files. Its author states directly:
"App-authored #945 should not merge; this PR is the independent replacement."

This PR was authored by allyblockcast[bot] — the same identity that reviews it — so it could
not get an independent review. #953 exists specifically to fix that. Draft rather than closed so
the diff and the four Ally findings that shaped #953 stay readable; it also stops this PR pulling
a duplicate reviewer wake.

Closing outright is a call for @kkroo — say the word and I'll close it, or mark it ready again if
#953 is abandoned.

Tracking issue: BLO-19132

[kkroo]

Closing as superseded by independent replacement #953. This draft App-authored PR already records that #953 carries the same approval dedupe feature plus fixes for Ally's four important findings, so keeping this source PR open only adds backlog noise.