[codex] add idempotent approval create and cheap listing (BLO-19132)

[kkroo]

Thinking Path

• Paperclip agents need a cheap way to check whether an approval ask is already pending before filing another one.
• PR feat(approvals): idempotency key on create + cheap existence-check listing #945 added idempotency keys and cheap approval listing views, but Ally found four merge-blocking gaps: ambiguous requester identity, raw summary-label leakage, dropped filters, and first-create side effects happening after the approval row committed.
• This replacement keeps the BLO-19132 API shape while closing those review findings on an independently authored branch so Ally App can review it.

Linked Issues or Issue Description

• Refs: BLO-19132
• Supersedes: feat(approvals): idempotency key on create + cheap existence-check listing #945
• Related: BLO-20810, BLO-20129, BLO-19079

What Changed

• Adds idempotencyKey to approval create so a retry from the same authenticated requester replays the undecided approval instead of filing a duplicate.
• Adds view=count|summary|full approval listing support with consistent status, type, issueId, requestedByAgentId, and idempotencyKey filtering across every view.
• Keeps summary responses payload-free, but derives label in application code after the same approval-payload redaction path used by full responses.
• Makes create-route requester attribution exclusive and actor-derived: agent actors populate requestedByAgentId; user actors populate requestedByUserId; body-supplied requester IDs do not create dual-identity rows.
• Runs first-filing issue links and approval.created activity logging inside the create transaction through an afterCreate hook. Idempotent replays still attach newly supplied issue links to the original approval without emitting another activity card.

Review Fixes Since #945

• Fixed dual requester/index-scope mismatch by rejecting dual identities in the idempotent service path and preventing the route from creating them.
• Fixed summary-label secret leakage by selecting only label candidate fields and redacting before fallback selection.
• Fixed dropped filters in count and full views.
• Fixed post-commit side-effect gap by making new-create links and activity logging transactional with the approval insert.

Verification

PATH=/opt/homebrew/bin:$PATH pnpm --filter @paperclipai/server exec vitest run src/__tests__/approval-routes-idempotency.test.ts src/__tests__/approvals-service.test.ts --reporter=dot --no-file-parallelism --maxWorkers=1
# 2 files passed, 37 tests passed

PATH=/opt/homebrew/bin:$PATH pnpm exec vitest run packages/shared/src/validators/approval.test.ts --reporter=dot
# 1 file passed, 9 tests passed

PATH=/opt/homebrew/bin:$PATH pnpm --filter @paperclipai/server typecheck
# passed

PATH=/opt/homebrew/bin:$PATH pnpm --filter @paperclipai/db run check:migrations
# passed

git diff --check
# passed

Risks

• User-created approval requests no longer honor a body-supplied requestedByAgentId; requester identity is now the authenticated user. This is intentional to keep idempotency scope and persisted attribution coherent.
• Summary listing still reads three payload text fields in SQL, but does not return raw payload and redacts before choosing a label.
• App-authored feat(approvals): idempotency key on create + cheap existence-check listing #945 should not merge; this PR is the independent replacement.

Model Used

GPT-5 Codex (gpt-5-codex), using local repo inspection, gh, and focused test/typecheck runs.

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 searched the GitHub PR list for similar PRs and confirmed this is not a duplicate
• 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/MCP text carried from feat(approvals): idempotency key on create + cheap existence-check listing #945
• 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

[cursor]

Bugbot is not enabled for your account, so this pull request was not reviewed.

Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs.

[allyblockcast]

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

[allyblockcast]

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

[kkroo]

/test

[kkroo]

@ally review exact head d9f28c1

[allyblockcast]

Housekeeping so the two PRs don't compete: #945 is now a draft and is not a merge candidate.
It touched the identical 14 files, and per this PR's description the App-authored version should
not merge. Draft (not closed) keeps its diff and review history readable; it also stops it drawing
a duplicate reviewer wake.

No changes pushed here — this branch is yours and its independent authorship is the point of it
existing, so I'm deliberately not committing to it.

Status at head d9f28c1e: review, policy, e2e, Build green; 9 test/typecheck jobs still
queued from your /test; security-review neutral; Storybook skipped. Nothing failing.
Ally is running and your review request is ~38 min in — its passes typically take ~50 min, so I
have not re-requested and risked a duplicate pass.

[allyblockcast]

Ally — Consolidated PR Review

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

Important Issues (1)

• [gstack/review] server/src/routes/approvals.ts:218 — The new transactional callback invokes logActivity(txDb, ...), but logActivity does more than write through txDb: it immediately calls publishLiveEvent and then fire-and-forget enqueues approval.created through the global outbox DB (server/src/services/activity-log.ts:192-241). Those effects can run before the create transaction commits. A plugin can therefore consume an event while the approval is still invisible, and a commit failure can leave a ghost notification for an approval that rolled back. This preserves the same create/notification consistency gap the afterCreate hook is intended to close, only in the opposite direction.
  • Make the outbox insert part of the supplied transaction and defer the in-memory live event until after commit, or add an explicit transaction-safe activity path whose externally observable effects cannot precede commit. Add a regression test that forces the create transaction to roll back after activity logging and proves that no live/plugin event escapes.

Strengths

• Requester attribution is now actor-derived and mutually exclusive, matching the two requester-scoped unique indexes.
• Count, summary, and full views share the same filter builder, and summary responses omit the payload while deriving labels after redaction.
• The advisory lock and partial unique indexes provide a sound concurrency backstop for undecided approval retries.

Recommended Action

1. Fix the Important issue before merge.
2. Re-run the canceled CI workflow after the fix so all required lanes complete on the reviewed head.

[allyblockcast]

Ally's Important issue is correct. I verified both legs at d9f28c1e and re-ran the cancelled CI. I have deliberately not pushed a commit — reasoning at the bottom.

The finding is confirmed, and it reaches plugins (not just the UI)

afterCreate runs inside the create transaction (services/approvals.ts:269, invoked from the db.transaction blocks at :275 and :292), and calls logActivity(txDb, …). logActivity does three things, and only the first is transactional:

#	Effect	Uses	Transactional?
1	db.insert(activityLog) — activity-log.ts:179	passed db = txDb	yes
2	publishLiveEvent(…) — :192	in-memory SSE broadcast	no — fires immediately
3	publishPluginDomainEvent(event) — :241	module-global _outboxDb	no — different connection

Leg 3 is the one worth being explicit about, because it's easy to talk yourself out of it. eventTypeForActivityAction first checks PLUGIN_EVENT_SET.has(action) (:56), and "approval.created" is in PLUGIN_EVENT_TYPES (packages/shared/src/constants.ts:1616). So the fast path hits and publishPluginDomainEvent runs. That function inserts into pluginEventOutbox via _outboxDb (:75-85) — the module-scope handle set once at boot, not the db argument logActivity was given. The row therefore lands on a separate connection and commits independently of the create transaction.

Net: if the create transaction rolls back after afterCreate, you keep an outbox row that the worker-tier poller will emit as approval.created, plus an already-broadcast live event, for an approval that does not exist. In the non-rollback case the ordering is still wrong — a plugin can consume the event and GET /approvals/:id → 404, because the read races a transaction that hasn't committed.

One extra hazard Ally didn't name: the outbox insert is fire-and-forget with a swallowed error (void … .catch(…)). Inside a transaction that's worse than it looks — the un-awaited promise can resolve after the tx client is released, so the failure mode isn't only "committed too early" but also a possible use-after-release on the pooled client, logged at warn and otherwise invisible.

This is pre-existing, which I think should shape the fix

logActivity is already called with a transaction handle in three other places on master:

• server/src/services/heartbeat.ts:12917
• server/src/services/heartbeat.ts:22100
• server/src/services/pipelines.ts:3771

So #953 is the fourth site, not the origin. The file's own doc comment (:63-66) describes the outbox as the durable cross-tier path — but writing it through a global handle defeats the transactional-outbox property that comment is claiming. I'd rather see this fixed once in logActivity than bespoke in the approvals route, since a route-local workaround leaves the other three intact and makes the next transactional caller re-discover this.

Fix shape I'd suggest

1. Make the outbox insert join the caller's transaction. Thread the db logActivity already receives into publishPluginDomainEvent, and await it instead of void-ing it. When the caller passes a plain Db this is behaviourally identical to today; when it passes a tx, the outbox row commits or rolls back atomically with the activity_log row — which is what makes it an outbox. Awaiting also stops the released-client race above.
2. Defer the in-memory live event past commit. It can't be transactional, so it has to move: have logActivity accept { deferEmit?: true } and return a flush() closure that the transactional caller invokes after createWithIdempotency resolves. Default stays eager, so the ~dozen non-transactional callers are untouched.

That keeps the property #953 is actually after — a replay must not emit a second card, and a first filing must not be silently card-less — without letting either effect precede commit.

3. Regression test, per Ally: force a rollback after afterCreate (e.g. have the afterCreate hook throw, or make the issue-link insert fail) and assert plugin_event_outbox has no row for that approvalId and no live event escaped. Worth asserting the happy path too: exactly one outbox row after a successful create, and zero additional rows on a deduplicated replay.

CI

Ally's Recommended Action #2 — I've done this part. The PR workflow run at this head (30738540290) was cancelled, not failed, and the verify job's failure is purely downstream of that: its log shows TYPECHECK_RELEASE_REGISTRY_RESULT: cancelled, GENERAL_TESTS_RESULT: cancelled, WORKTREE_INSTALL_RESULT: cancelled, BUILD_RESULT: success, and it's a gate asserting all four are success. So there is no known test failure at d9f28c1e — the lanes simply never finished. I've re-run that workflow; it's queued now. Note the PR is also BEHIND master, so a rebase may be wanted before the final green.

Why I'm not pushing this myself

#953 exists because #945 was authored by app/allyblockcast — the same identity that reviews it — so it could never receive an independent review. That worked: Ally has now reviewed #953, which #945 never managed. This PR is yours, on your branch, and the fix above touches activity-log.ts, a shared file with three other affected call sites — a wider blast radius than a review nit, and a scoping call I don't think I should make unilaterally on someone else's PR.

So: happy to implement exactly the above and hand you a patch or a branch you can cherry-pick, if you'd rather not write it — just say. I haven't re-requested Ally, since there's no new head and a re-request would burn another ~50-minute pass for nothing.

Refs: BLO-19132.

[allyblockcast]

Not a review — a CI status correction, because the last word on this PR is now wrong and has been for ~15 hours.

The re-run kicked off at 12:01Z (30738540290) has since finished, and head d9f28c1e is not green:

job	conclusion	at
General tests (workspaces-a)	failure	12:17:02Z
General tests (server 4/4)	cancelled	12:26:16Z
Verify serialized server suites	cancelled	12:26:16Z
verify (gate)	failure	12:27:21Z

Everything else on that run passed — e2e, Build, Typecheck + Release Registry, Canary Dry Run, Worktree install, policy, review, and server shards 1/2/3.

So the "no known test failure at d9f28c1e / nothing failing" note above was correct when written at 12:01Z and was overtaken 15 minutes later. I have not read the workspaces-a log and am not calling it real or flaky — it just needs someone to look, and so far nobody has.

Three things are outstanding here, all yours to sequence:

1. workspaces-a red (above) — unattributed.
2. Ally's Important finding — the transactional logActivity/outbox gap. Confirmed analysis and a proposed fix shape are in the comment above, and the offer of a patch or a cherry-pickable branch is still open if you'd rather not write it.
3. Still BEHIND master — rebase likely wanted before final green.

Flagging rather than acting: this branch is yours and its independent authorship is the whole reason it exists, so nothing has been pushed here.

CEO — refs BLO-19132.

[allyblockcast]

@kkroo — two updates that should shrink your remaining work on this to a single action. Not repeating the checklist from my 03:24Z comment above; this supersedes part of it.

1. workspaces-a is not a real failure — please don't spend time on it. I flagged it red earlier and said explicitly that nobody had read the log yet. I have now. The tests passed; the job flipped to failure on a cancelled post-step, and the whole run was cancelled:

gh api repos/Blockcast/paperclip/actions/jobs/91494060595 --jq '.steps[] | "\(.conclusion) \(.name)"'
  success   Run grouped general test suites     <-- tests PASSED
  cancelled Post Set up GHA-backed sccache      <-- this is what reddened the job

gh api repos/Blockcast/paperclip/actions/runs/30738540290 --jq '.conclusion'
  cancelled

Same artifact class as the verify red. The honest statement is not "it's green" but CI has never completed a clean run at d9f28c1e — both attempts ended cancelled. That needs a fresh run, not a bug hunt.

2. Ally's Important finding is being fixed upstream, not here. I opened #1024 against master for it. The finding is pre-existing and not introduced by this PR — logActivity already took a tx handle at three other call sites and ignored it for the plugin-outbox enqueue; yours would just be the first caller where the leaked event is a real plugin domain event (approval.created is in PLUGIN_EVENT_TYPES; the other three actions are not). So the right home is the shared helper, where it also fixes those three, rather than a route-local patch here that would leave them broken.

I have deliberately not pushed to this branch. Its independent authorship is the whole reason it exists — a commit from the bot identity that reviews it would destroy exactly the property that got you a real review — so that call stays yours, and the fix went somewhere it doesn't cost you anything.

Your remaining action, once #1024 merges: rebase onto master. That single step clears three things at once — it resolves the current dirty/conflicting state, retires Ally's finding without you writing any code, and triggers the fresh CI run that point 1 actually needs.

If you'd rather not wait on #1024, say so and I'll hand you the patch as a branch you can cherry-pick under your own authorship. Happy either way — flag whichever you prefer.

[kkroo]

@ally please re-review exact head 4b00481223fc0d6b93bcfb6f67541d3e35e3a43d. I rebased onto current master, renumbered the approval idempotency migration to 0210, and fixed the Important finding by using logActivity(..., { deferPublish: true }) inside the create transaction and flushing the returned publisher only after createWithIdempotency returns. Added route regressions for post-commit flush and rollback/no-flush. Local verification: git diff --check, server approval route/service Vitest (39 passed), shared approval validator Vitest (9 passed), and pnpm --filter @paperclipai/server typecheck all passed.

[allyblockcast]

Approved exact head 4b00481. Rebased onto current master, migration renumbered to 0210, and the prior transactional approval-created finding is addressed by deferring logActivity publication until after createWithIdempotency returns. Local focused route/service/shared tests and server typecheck passed; no unresolved review threads.

[allyblockcast]

Ally — Consolidated PR Review

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

Looks good. The prior transactional publication blocker is fixed, and no new Critical or Important issues remain.

Prior Findings Dispositioned (1)

• prior:d9f28c1 important 1 — fixed — server/src/routes/approvals.ts:219 — logActivity now records with deferPublish: true inside the create transaction, and the returned publisher is flushed only after createWithIdempotency returns at line 252; rollback therefore cannot emit a phantom approval.created event.

Suggestions (1)

• [pr-review-toolkit] server/src/services/approvals.ts:64 — Consider bounding the derived summary label length so an unusually large payload title cannot make the cheap summary response unexpectedly large.

Strengths

• Requester-scoped advisory locking and partial unique indexes provide coherent retry behavior under concurrency.
• Count, summary, and full views share one filter builder, while summary labels pass through approval-type redaction and omit raw payloads.
• Route regressions cover post-commit publication and rollback without publication.

Recommended Action

1. Merge after the required CI checks complete successfully.

[allyblockcast]

Ally — Consolidated PR Review

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

Looks good. The merge conflict resolution preserves the previously reviewed approval behavior, integrates the new linkedAgentId schema from master, and renumbers the idempotency migration and its drift test consistently to 0211.

Suggestions (1)

• [native-codex] server/src/services/approvals.ts:354 — Consider detecting reuse of an idempotency key with a different approval type or normalized payload. The current contract deliberately replays the original and permits additive issue links; a request fingerprint plus 409 Conflict for mismatched immutable fields would make accidental key collisions easier to diagnose.

Strengths

• Requester-scoped advisory locking and partial unique indexes provide coherent retry behavior under concurrency.
• The prior transactional publication blocker remains fixed: activity publication is deferred until the create transaction succeeds, with rollback/no-flush regression coverage.
• Count, summary, and full views share one filter builder, while summary labels pass through type-aware redaction and omit raw payloads.
• The synchronized head resolves the migration conflict cleanly: master owns 0210_approval_linked_agent, and this change consistently uses 0211_approval_create_idempotency.

Recommended Action

1. Merge after the required CI checks complete successfully.

[allyblockcast]

Ally — Consolidated PR Review

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

Looks good. The merge conflict resolution preserves the previously reviewed approval behavior, integrates the new linkedAgentId schema from master, and renumbers the idempotency migration and its drift test consistently to 0211.

Suggestions (1)

• [native-codex] server/src/services/approvals.ts:354 — Consider detecting reuse of an idempotency key with a different approval type or normalized payload. The current contract deliberately replays the original and permits additive issue links; a request fingerprint plus 409 Conflict for mismatched immutable fields would make accidental key collisions easier to diagnose.

Strengths

• Requester-scoped advisory locking and partial unique indexes provide coherent retry behavior under concurrency.
• The prior transactional publication blocker remains fixed: activity publication is deferred until the create transaction succeeds, with rollback/no-flush regression coverage.
• Count, summary, and full views share one filter builder, while summary labels pass through type-aware redaction and omit raw payloads.
• The synchronized head resolves the migration conflict cleanly: master owns 0210_approval_linked_agent, and this change consistently uses 0211_approval_create_idempotency.

Recommended Action

1. Merge after the required CI checks complete successfully.