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.