fix(activity-log): enlist plugin outbox writes in caller tx (BLO-19132)

[kkroo]

Thinking Path

• Paperclip is the open source app people use to manage AI agents for work.
• Activity logging also feeds plugin domain events through the worker-tier outbox.
• PR fix(activity-log): enqueue the plugin outbox row on the caller's tx handle (BLO-19132) #1024 fixed the right atomicity bug: logActivity(db, ...) must enqueue plugin outbox rows through the same handle so rollback cannot leak phantom plugin events.
• Ally's review found the important missing piece: an enlisted PostgreSQL statement failure cannot be swallowed, because the transaction is already failed and hiding the original error makes the caller see a misleading later failure.
• This carrier keeps global/deferred outbox publication best-effort, but lets explicit transactional handles reject so the activity row and outbox row fail as one unit.
• The benefit is explicit, atomic failure for transactional activity/outbox writes without changing deferred publication semantics.

Linked Issues or Issue Description

• Refs BLO-19132.
• Refs [codex] add idempotent approval create and cheap listing (BLO-19132) #953.
• Related duplicate/source PRs found by search: fix(activity-log): enqueue the plugin outbox row on the caller's tx handle (BLO-19132) #1024, [codex] add idempotent approval create and cheap listing (BLO-19132) #953, feat(approvals): idempotency key on create + cheap existence-check listing #945.
• Supersedes fix(activity-log): enqueue the plugin outbox row on the caller's tx handle (BLO-19132) #1024.

What Changed

• Carried the fix(activity-log): enqueue the plugin outbox row on the caller's tx handle (BLO-19132) #1024 logActivity/outbox fix onto current master under an independent author.
• Changed publishPluginDomainEvent(event, db) so explicit caller handles propagate enqueue errors, while omitted/null handles still log and swallow best-effort failures.
• Kept deferred publication on the boot-time global handle because deferred callbacks run after transaction commit.
• Added embedded-Postgres coverage for the review finding: a forced outbox insert failure rejects the outer transaction and rolls back the activity row.

Verification

• git diff --check - passed.
• pnpm --filter @paperclipai/server typecheck - passed.
• pnpm exec vitest run server/src/__tests__/activity-log-transactional-publish.test.ts - test file loaded successfully; embedded Postgres is unsupported in this local Mac session, so the 5 embedded tests were skipped locally and must run in CI.

Risks

Moderate but scoped to plugin-mapped activity logging. Inline transactional callers can now see the original outbox enqueue error instead of a swallowed error followed by an implicit rollback/commit failure. Deferred and global publication remain best-effort.

Model Used

OpenAI Codex, GPT-5 family, with GitHub CLI inspection, local worktree patching, Vitest startup, and TypeScript verification.

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
• I have updated relevant documentation to reflect my changes
• 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
• 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

[allyblockcast]

🔗 Paperclip issue: BLO-19132

[allyblockcast]

Hey @kkroo! Before this PR can be reviewed, a few things need attention:

Missing or incomplete:

• Add the dedup-search checkbox to your PR description and check it once you have searched the GitHub PR list for similar PRs. See the PR template at .github/PULL_REQUEST_TEMPLATE.md and CONTRIBUTING.md → "Before You Start: Search First".

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

— commitperclip

[allyblockcast]

Approved exact head bf9b6ae. This carrier addresses #1024's transactional outbox review finding by propagating enlisted insert failures while keeping global/deferred publication best-effort; review gate is green and there are no unresolved review threads.

Superseded at bf9b6ae: the App review found an unresolved Important issue on this exact head.

[allyblockcast]

Ally — Consolidated PR Review

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

Important Issues (1)

• [pr-review-toolkit/native-codex] server/src/services/activity-log.ts:305 — Every non-deferred logActivity call passes its required db argument into publishPluginDomainEvent, and that function treats any non-null handle as transaction-enlisted (db != null). A normal top-level Db is not a transaction: if the activity insert commits and the subsequent outbox insert fails, logActivity now rejects after durable partial success. Callers can report failure or retry even though the activity row already exists, which breaks the documented best-effort behavior for callers outside transactions.
  • Make strict error propagation/enlistment explicit instead of inferring it from the presence of a handle, or ensure ordinary calls wrap the activity and outbox writes in one transaction. Add a regression test that forces an outbox failure through a plain Db and asserts the intended activity-row and error behavior.

Strengths

• The enlisted transaction path now propagates the original PostgreSQL statement failure instead of swallowing it and surfacing a misleading later transaction failure.
• Embedded-Postgres coverage verifies commit, rollback, deferred publication, and the forced enlisted-insert failure.
• Deferred post-commit publication correctly avoids reusing a released transaction handle.

Recommended Action

1. Address the Important issue before merge.
2. Re-run the embedded-Postgres regression suite after making enlistment explicit.

[allyblockcast]

Ally — Consolidated PR Review

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

Prior Findings Dispositioned (1)

• prior:bf9b6ae important 1 — still-present — server/src/services/activity-log.ts:92 — logActivity still passes every non-deferred caller handle through emit(db) at line 310, while db != null classifies both transactions and ordinary top-level handles as enlisted and routes failures through the rejecting await insert path at lines 99-100.

Important Issues (1)

• prior:bf9b6ae important 1 [pr-review-toolkit/gstack-review/native-codex] server/src/services/activity-log.ts:92 — Ordinary top-level logActivity(db, ...) calls are still treated as transaction-enlisted. If the activity insert autocommits and the subsequent outbox insert fails, the call rejects after durable partial success; callers can report failure or retry even though the activity row already exists.
  • Make transaction enlistment explicit rather than inferring it from a non-null Db. Keep plain top-level handles on the best-effort path, reserve propagated failures for explicitly transactional calls, and add an ordinary-Db outbox-failure regression test.

Strengths

• The enlisted transaction path preserves the original PostgreSQL failure and rolls back the activity and outbox rows together.
• Embedded-Postgres coverage exercises commit, rollback, deferred publication, disabled-outbox behavior, and forced enlisted-insert failure.
• Deferred publication correctly avoids reusing a released transaction handle.

Recommended Action

1. Resolve the remaining Important issue before merge.
2. Re-run the embedded-Postgres regression suite after making transaction enlistment explicit.

[allyblockcast]

Reviewed current head; no active unresolved review threads.

[allyblockcast]

Ally — Consolidated PR Review

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

Prior Findings Dispositioned (1)

• prior:bf9b6ae important 1 — still-present — server/src/services/activity-log.ts:92 — The exact head still derives strict transaction enlistment from db != null, while every non-deferred logActivity passes its caller handle through emit(db) at line 310. This still classifies an ordinary top-level Db as transactional.

Important Issues (2)

• prior:bf9b6ae important 1 [pr-review-toolkit/gstack-review/native-codex] server/src/services/activity-log.ts:92 — Ordinary top-level logActivity(db, ...) calls are still treated as transaction-enlisted. If the activity insert autocommits and the subsequent outbox insert fails, the call rejects after durable partial success and after the live event has fired, so callers can report failure or retry work whose activity row already exists.
  • Make transaction enlistment explicit instead of inferring it from a non-null handle. Keep plain top-level handles on the best-effort path, reserve propagated failures for explicitly transactional calls, and add a plain-Db outbox-failure regression test.
• [gstack-review/native-codex] server/src/services/activity-log.ts:307 — The deferred ActivityPublish callback now starts async emit(null) with void, so synchronous listener errors are converted into an unobserved rejected promise. The existing exact-head caller at server/src/routes/issues.ts:4358-4366 invokes the callback inside try/catch specifically to log publication failures, but that catch can no longer observe them; under strict unhandled-rejection handling this can terminate the process.
  • Change ActivityPublish to return Promise<void> and await it at callers, or attach a terminal .catch(...) inside the deferred callback. Add a regression test with a throwing live-event subscriber.

Strengths

• The enlisted transaction path preserves the original PostgreSQL insert failure and rolls back the activity and outbox rows together.
• Embedded-Postgres coverage exercises commit, rollback, deferred publication, disabled-outbox behavior, and a forced enlisted-insert failure.
• Deferred publication correctly avoids reusing a released transaction handle.

Recommended Action

1. Resolve both Important issues before merge.
2. Re-run the embedded-Postgres regression suite with plain-handle failure and deferred-listener failure coverage.

[allyblockcast]

Ally — Consolidated PR Review

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

Prior Findings Dispositioned (2)

• prior:bf9b6ae important 1 — fixed — server/src/services/activity-log.ts:102 — Strict propagation now requires the explicit enlisted flag, while the ordinary path catches and logs insert failures at lines 113-115; the plain-Db regression test covers the intended committed-activity/best-effort-outbox behavior.
• prior:5f4ffdb important 2 — fixed — server/src/services/activity-log.ts:329 — The deferred callback now returns emit(null, false) instead of discarding its promise, and server/src/routes/issues.ts:4403 awaits it inside the existing notification-failure handler.

Important Issues (2)

• [pr-review-toolkit/native-codex] server/src/services/activity-log.ts:331 — enlistPluginOutbox controls rejection behavior but not handle selection: every non-deferred call still passes the caller's db into the outbox insert. An unannotated transaction therefore enlists implicitly; if that insert fails, the catch at lines 113-115 swallows the original error after PostgreSQL has already aborted the transaction, so the caller gets a misleading later statement/commit failure. This contradicts the documented explicit-opt-in carrier.
  • Pass the caller handle only when transaction enlistment is explicitly requested; otherwise use the global outbox handle. Add a forced-failure test for a transaction handle without enlistPluginOutbox.
• [gstack-review/native-codex] server/src/services/activity-log.ts:303 — The live event fires before the strict enlisted outbox insert. If that insert rejects, the enclosing transaction rolls back but subscribers have already observed activity.logged for state that does not exist. The enlisted-failure test verifies only database rows, so this phantom side effect is currently untested.
  • Perform the strict enlisted insert before publishing the live event, or defer all publication until commit. Extend the failure test to assert that no live event escapes.

Strengths

• The ordinary top-level Db path now preserves best-effort outbox semantics and has focused regression coverage.
• Deferred publication is awaitable, and listener failures are observable by the post-commit caller.
• The tests cover commit, rollback, disabled outbox, strict insert failure, ordinary-handle failure, and deferred publication.

Recommended Action

1. Fix the two transaction-side ordering/carrier issues before merge.
2. Run the embedded-Postgres suite with the missing transaction-without-enlistment and no-phantom-live-event assertions.

[allyblockcast]

Ally — Consolidated PR Review

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

Prior Findings Dispositioned (2)

• prior:22f8101 important 1 — fixed — server/src/services/activity-log.ts:303 — Handle selection is now explicit: only shouldEnlistPluginOutbox passes the caller handle, while the ordinary path calls publishPluginDomainEvent with db: null at lines 332-335 and therefore uses the global best-effort handle.
• prior:22f8101 important 2 — fixed — server/src/services/activity-log.ts:306 — The strict enlisted insert is awaited at lines 306-313 before publishLiveEvent runs, and the failure regression asserts that no live event escapes when that insert rejects.

Critical Issues (0)

Important Issues (1)

• [gstack-review/native-codex] server/src/services/activity-log.ts:340 — deferPublish returns before performing the enlisted outbox write, so it cannot be combined with enlistPluginOutbox (or atomicPluginEvent). Consequently, every newly annotated transactional caller must publish activity.logged inline at line 316 to get an atomic outbox row. If any later statement or commit fails, the database activity and outbox rows roll back but live subscribers have already observed a phantom activity. The rollback test captures live events but never asserts their absence.
• Recommendation: split transactional outbox enqueue from live publication so { enlistPluginOutbox: true, deferPublish: true } enqueues before commit and returns a live-only post-commit callback; use that composition at the transactional call sites and assert no live event on rollback after a successful enqueue.

Strengths

• Plain and unannotated handles preserve the global best-effort outbox behavior.
• Strict outbox insert failures now propagate the original database error and do not emit a live event first.
• Deferred publication is awaitable, making listener failures observable to its caller.

Recommended Action

1. Fix the Important transaction/publication ordering issue before merge.
2. Run the embedded-Postgres regression suite with a rollback-after-successful-enqueue assertion.

[allyblockcast]

Ally's Important finding is correct — and it has a live instance neither of us had named. One correction to its framing.

@kkroo — I authored the #1024 change this PR carries, so I verified the finding against current head b82fe3e2 rather than taking it on trust. It is right, and it should be fixed. I have a verified patch on a branch for you to cherry-pick; I am not pushing to your branch (reasoning at the bottom).

Confirmed: the composition genuinely cannot be expressed

deferPublish returns at activity-log.ts:340 before the enlisted write, and the deferred callback calls emit(null, false) — so outboxDb === null makes shouldEnlistPluginOutbox false at :303 regardless of what the caller asked for. { enlistPluginOutbox: true, deferPublish: true } therefore silently drops the enlistment and falls back to the global handle. A transactional caller of a plugin-mapped action must pick one of atomicity or a live event that cannot outlive a rollback. It cannot have both, and nothing tells it so.

The live instance: approvals.ts withdraw — pre-existing, not from this PR

Ally attributes the exposure to "every newly annotated transactional caller." That part is wrong, and the real instance is somewhere else:

annotated site	action	plugin event?
company-skill-policy.ts:262	company.skill_policy_replaced	none
company-skill-policy.ts:288	company.skill_policy_reset	none
heartbeat.ts:14097	execution_workspace.workspace_validation_quarantined	none
heartbeat.ts:24641	issue.workspace_preflight_blocked	none
pipelines.ts:3790	pipeline.stage_automation_env_updated	none

None is in PLUGIN_EVENT_TYPES or ACTIVITY_ACTION_TO_PLUGIN_EVENT, so eventTypeForActivityAction returns null, pluginEvent is null, and the enlisted branch is dead code at all five. They write no outbox row, so the finding's "the database activity and outbox rows roll back" clause is vacuous for them. Their phantom live event is real but pre-existing and unchanged by this PR — they published inline before the annotation and still do; enlistPluginOutbox: true does not alter live-event timing.

The actual instance is a caller neither review mentioned — approvals.ts:423 withdraw, from #850 (BLO-19079):

• inside db.transaction, and it has already terminate()d the bound agent by that point;
• approval.withdrawn → approval.decided via ACTIVITY_ACTION_TO_PLUGIN_EVENT, so pluginEvent is non-null;
• sets atomicPluginEvent: true (so the outbox row is correctly bound) with no deferPublish.

So it binds the durable event and then announces activity.logged before commit. A failed commit leaves subscribers a withdrawal that did not happen. That is exactly Ally's scenario, on master today.

Severity, stated honestly: the escaping event is an in-memory SSE refresh hint, not the durable outbox row — it reaches connected sessions and self-corrects on the next fetch. That is materially lower severity than the phantom plugin domain event this PR already fixed. I would call it worth closing now because the composition is what this PR's first real consumer needs, not because the leak is severe.

Ally is also right about the test

activity-log-transactional-publish.test.ts:84 calls captureLiveEvents() and live.unsubscribe() and then never asserts on live.seen. The phantom escapes undetected.

Patch: cto/blo-19132-defer-plus-enlist-proposed @ cec4e7b

git cherry-pick cec4e7b041f8b23c253d70afa1eb45b7e682f975

Hoists the enlisted enqueue above both publication paths — it has to run while db is live, and a live event cannot be un-sent, so a failed insert must abort before anything publishes. The deferred callback then withholds only the live event, and re-enqueues on the global handle only when the row was not already enlisted. Adopts the composition at approvals.ts withdraw.

Deliberately not adopted at the five sites above: they write no outbox row, and hoisting a deferred publish out of those heartbeat.ts transactions is a far larger change than a stale refresh hint justifies. Behaviour matrix after the patch:

options	outbox enqueue	live event
none	global, best-effort, inline	inline
deferPublish	global, best-effort, after commit	after commit
enlistPluginOutbox	caller handle, strict, atomic	inline (documented accepted phantom)
both	caller handle, strict, atomic	after commit

Verification — tsc --noEmit -p server/tsconfig.json clean; 11 passed (11) on the suite (8 existing + 3 new), plus 71 passed over 7 approval/activity suites including approval-withdraw-plugin-event.test.ts, and 18 passed over the outbox/issue-activity ones.

Note the two pre-existing withdrawApprovalSchema typecheck errors are a stale local @paperclipai/shared dist, identical with and without my change; pnpm --filter @paperclipai/shared build clears them.

Mutation-checked, and I will be precise about which assertion carries the weight. Restoring the old semantics fails exactly one case:

× enlisted + deferred enqueues inside the transaction but publishes live after commit
  AssertionError: expected [] to have a length of 1 but got +0
Tests  1 failed | 10 passed (11)

The in-transaction visibility check is the discriminator. The rollback case does not discriminate — under the old code the deferred callback simply never ran, so nothing escaped either way; it asserts the correct end state, not the fix.

Why a branch and not a commit on yours

This PR supersedes my App-authored #1024 specifically so the work has an independent author. A commit from allyblockcast[bot] would remove the property the carrier exists to provide. You have also answered all four prior review rounds in 1h35m / 7h20m / 12h45m / 8h04m, so at ~10.5h you are inside your own range and I have no reason to think you are stalled. The patch is yours to take, amend, or reject. I have not re-requested Ally — your push re-triggers review on its own.

[allyblockcast]

Ally — Consolidated PR Review

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

Prior Findings Dispositioned (1)

• prior:b82fe3e important 1 — fixed — server/src/services/activity-log.ts:341 — The outbox insert now runs on the caller's transaction handle before either publication path, and lines 345-354 return a deferred callback that emits only the live event without re-enqueuing. server/src/services/approvals.ts:423-449 uses this composition and invokes the callback only after commit.

Critical Issues (0)

Important Issues (1)

• [pr-review-toolkit/gstack-review/native-codex] server/src/services/activity-log.ts:358 — Ordinary, non-enlisted publication now awaits the global-handle outbox insert. When logActivity runs inside a transaction, that transaction retains one pooled connection while waiting for _outboxDb to acquire another; enough concurrent transactions can occupy the entire pool and wait indefinitely on connections they themselves retain. The previous fire-and-forget path allowed each transaction to commit and release its connection.
• Recommendation: await only enlisted inserts. Keep best-effort global publication detached with a terminal error handler, or explicitly schedule it after commit, and add a constrained-pool regression test for concurrent unannotated transactions.

Strengths

• Combined enlistment and deferral now preserve durable outbox atomicity without leaking a live event before commit.
• The exact-head tests distinguish in-transaction enqueue visibility, rollback behavior, post-commit live publication, and duplicate prevention.
• The approval-withdraw path awaits the deferred live publisher and contains listener failures after the durable transaction commits.

Recommended Action

1. Restore non-blocking behavior for non-enlisted global outbox publication before merge.
2. Add and run the constrained-pool regression test.