fix(activity-log): defer publish at 5 remaining transactional logActivity call sites (BLO-21605)

[allyblockcast]

Thinking Path

• Paperclip is the open source app people use to manage AI agents for work
• logActivity (server/src/services/activity-log.ts) writes the activity_log row and then fires two side effects: an in-process EventEmitter publish and a plugin-event-outbox insert on its own module-level db handle — neither is scoped to whatever transaction logActivity was called with.
• A caller that passes a drizzle transaction (db.transaction(async (tx) => ... logActivity(tx, ...))) gets those side effects fired before the transaction commits: a consumer can receive activity.logged and query the row before it's visible, and a rollback still emits an event for a row that never existed.
• PR fix(issues): record denied issue writes for recovery (BLO-18614 AC3) #806 (BLO-18614) fixed this for recordDeniedIssueWrite by adding an opt-in logActivity(db, input, { deferPublish: true }) that returns the publisher instead of firing it, so the caller can invoke it after commit — but explicitly left 5 other transactional call sites unfixed to keep that PR scoped.
• This PR applies the same deferPublish: true pattern to those 5 remaining sites.
• The benefit is no more phantom activity.logged events on rollback, and no more pre-commit visibility races for consumers of company.skill_policy_replaced, company.skill_policy_reset, execution_workspace.workspace_validation_quarantined, issue.workspace_preflight_blocked, and pipeline.stage_automation_env_updated.

Linked Issues or Issue Description

Fixes: BLO-21605 (Paperclip issue; no public GitHub issue — filed against PR #806 / BLO-18614)

What Changed

• server/src/services/company-skill-policy.ts: replace() and reset() now call logActivity(tx, ..., { deferPublish: true }) and return the publisher out of the transaction; each invokes it (in a try/catch, logging on failure) only after the transaction resolves.
• server/src/services/heartbeat.ts:
  • The workspace-quarantine branch of the bounded-retry-scheduling transaction (scheduleBoundedRetryForRun) now defers publish and carries the publisher on the "scheduled" outcome variant; invoked only after db.transaction resolves.
  • The workspace-preflight-block branch of enqueueWakeup's dispatch transaction now defers publish and carries the publisher on the "skipped" outcome; invoked only after the transaction resolves (narrowed via "activityPublish" in outcome since only that one "skipped" return produces a publisher).
• server/src/services/pipelines.ts: updateStageAutomationEnv() now defers publish and returns { updatedRoutine, publish } from the transaction; invoked only after it resolves.
• All five sites capture the publisher as part of the transaction's return value (not a variable closed over from outside), so a thrown/rolled-back transaction propagates past the publish() call instead of falling through to it.
• Added regression tests per touched service (embedded-Postgres integration tests, following the pattern in issue-denied-write-recovery-persistence.test.ts):
  • company-skill-policy-service.test.ts: rollback emits zero activity.logged events; successful replace/reset publish only once the new revision/deletion is visible on a separate connection.
  • pipelines-service.test.ts: rollback (via a db.transaction proxy that runs the real transaction then throws, standing in for a commit-time failure) emits zero events; success publishes only once the routine revision bump is visible.
  • heartbeat-retry-scheduling.test.ts: same rollback/visibility pair for the workspace-quarantine path.
  • heartbeat-workspace-branch-containment.test.ts: same rollback/visibility pair for the workspace-preflight-block path.

Verification

• pnpm --filter @paperclipai/server exec tsc --noEmit — clean.
• pnpm --filter @paperclipai/server exec vitest run src/__tests__/company-skill-policy-service.test.ts src/__tests__/company-skill-policy-routes.test.ts src/__tests__/company-skills-routes.test.ts — 61/61 passed.
• pnpm --filter @paperclipai/server exec vitest run src/__tests__/pipelines-service.test.ts — 34/34 passed.
• pnpm --filter @paperclipai/server exec vitest run src/__tests__/heartbeat-retry-scheduling.test.ts — 54/54 passed.
• pnpm --filter @paperclipai/server exec vitest run src/__tests__/heartbeat-workspace-branch-containment.test.ts — 8/8 passed.

Risks

• Low risk. No behavior change for the ~400 non-transactional logActivity callers (the deferPublish option is opt-in and defaults to firing inline as before). The five touched sites change only when their publish fires (after commit instead of inline) and add a try/catch around a call that previously couldn't fail independently of the transaction — a publish failure is now logged as a warning rather than able to affect the transaction's outcome (it happens after commit).
• One mechanical follow-on: toHaveBeenCalledWith assertions are arity-exact, so any mock of logActivity at these 5 sites would need updating to expect the third { deferPublish: true } argument. I checked and none of the existing tests for these call sites mock logActivity directly (they either use the real service against embedded Postgres, or mock a different route file), so nothing needed updating.

Model Used

Claude Sonnet 5 (claude-sonnet-5[1m]), 1M context window, agentic tool use (Read/Edit/Bash/Grep) via Claude Code. No extended-thinking transcript included.

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, no UI change
• I have updated relevant documentation to reflect my changes — N/A, no user-facing doc changes needed
• I have considered and documented any risks above
• All Paperclip CI gates are green — pending CI run
• Greptile is 5/5 with no open P2s, recommendations, or follow-ups — pending review
• I will address all Greptile and reviewer comments before requesting merge

[allyblockcast]

🔗 Paperclip issue: BLO-21605
🔗 Paperclip issue: BLO-18614

[allyblockcast]

🔗 Paperclip issue: BLO-21605
🔗 Paperclip issue: BLO-18614

[allyblockcast]

Approved after green required checks and no unresolved review threads.

[allyblockcast]

Ally — Consolidated PR Review

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

Looks good. The five transactional callers now carry the deferred publisher through the transaction result and invoke it only after commit, so rollback cannot produce a phantom activity.logged notification. The outcome-union narrowing in both heartbeat paths also keeps publication limited to the branches that actually logged an activity.

Strengths

• Regression coverage exercises both rollback suppression and post-commit visibility for every affected service path.
• Publisher failures are isolated from already-committed mutations and logged with useful entity context.
• The change follows the established deferPublish contract without changing non-transactional callers.

Recommended Action

No Critical or Important findings remain.

The PR author is app/allyblockcast, so the Ally GitHub App cannot approve its own PR. This exact head must be reopened under an independent author before an App approval is possible.

[allyblockcast]

Ally — Consolidated PR Review

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

Looks good. The five transactional callers now return their deferred activity publishers through the transaction result and invoke them only after commit. Rollbacks therefore cannot emit phantom activity.logged events, while committed mutations are visible before subscribers run.

Critical Issues (0)

Important Issues (0)

Strengths

• Regression tests cover rollback suppression and post-commit visibility across every affected service path.
• Conditional heartbeat outcomes carry publishers only from branches that actually logged activity.
• Publisher failures are isolated from already-committed mutations and logged with useful entity context.

Recommended Action

No Critical or Important findings remain.

The PR author is app/allyblockcast, so the Ally GitHub App cannot review its own PR. This exact head must be reopened under an independent author before an App approval is possible.

[allyblockcast]

Ally — Consolidated PR Review

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

Looks good. Each transactional caller now carries the deferred activity publisher through the successful transaction result and invokes it only after commit. A rollback therefore bypasses publication, while committed state is visible before activity.logged subscribers run.

Critical Issues (0)

Important Issues (0)

Suggestions (0)

Strengths

• Regression coverage exercises rollback suppression and post-commit visibility across all five affected call sites.
• Conditional heartbeat outcomes carry a publisher only from the branch that actually logged the activity.
• Publisher failures are isolated from already-committed mutations and logged with useful entity context.

Recommended Action

No Critical or Important findings remain.

The PR author is app/allyblockcast, so the Ally GitHub App cannot review its own PR. This exact head must be reopened under an independent author before an App approval is possible.