refactor(triggers): extract shared webhook utilities and add Sentry concurrency

[aaight]

Summary

Fixes the webhook handler architecture divergence identified in https://trello.com/c/zR8c4ggY/579-12-webhook-handler-architecture-diverges-across-providers-trello-jira-use-processpmwebhook-pipeline-github-321-lines-and-sentry

• Extracted shared utilities into src/triggers/shared/ to reduce duplication across GitHub/Sentry/PM handlers:
  • concurrency.ts — withAgentTypeConcurrency() wraps the check→mark→execute→clear pattern
  • trigger-resolution.ts — resolveTriggerResult() unifies pre-resolved vs dispatch logic
  • credential-scope.ts — withPMScope() wraps the withPMCredentials + withPMProvider nesting
  • pm-ack.ts — postPMAckComment() consolidates PM ack posting (Trello/JIRA) shared by router and worker handlers
• Added missing concurrency management to Sentry — processSentryWebhook() now uses withAgentTypeConcurrency() (previously missing)
• Documented intentional divergence in src/triggers/README.md — explains why GitHub/Sentry cannot use processPMWebhook() (CI check polling, PR ack comments, persona token management are GitHub-specific)
• Updated GitHub webhook handler to use new shared utilities (withPMScope, withAgentTypeConcurrency, postPMAckComment)
• Updated Sentry webhook handler to use new shared utilities (resolveTriggerResult, withAgentTypeConcurrency, withPMScope)

Test plan

• All existing tests pass (7267 tests, 370 test files)
• github-webhook-handler.test.ts updated to mock new shared utilities
• sentry-webhook-handler.test.ts updated to mock new shared utilities + new test cases for concurrency
• New tests/unit/triggers/shared/concurrency.test.ts (9 tests)
• New tests/unit/triggers/shared/trigger-resolution.test.ts (9 tests)
• New tests/unit/triggers/shared/credential-scope.test.ts (6 tests)
• TypeScript typecheck passes (zero errors)
• Lint passes (zero new errors)

🤖 Generated with Claude Code

🕵️ claude-code · claude-sonnet-4-6 · run details

[codecov]

Codecov Report

❌ Patch coverage is 80.46875% with 25 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
src/triggers/shared/pm-ack.ts	0.00%	21 Missing ⚠️
src/triggers/github/webhook-handler.ts	86.20%	4 Missing ⚠️

📢 Thoughts on this report? Let us know!

[nhopeatall]

Summary

Clean refactoring that extracts duplicated patterns into shared utilities. The shared modules are well-designed, well-documented, and well-tested. Two issues worth addressing.

Code Issues

Should Fix

1. src/triggers/github/webhook-handler.ts:151 + src/triggers/sentry/webhook-handler.ts:64 — Watchdog started before concurrency check

   startWatchdog() is now called before withAgentTypeConcurrency. In the old GitHub handler code, the concurrency check happened first (old line ~170) and returned early if blocked — startWatchdog was only reached for jobs that would actually run an agent. Now, if concurrency blocks, withAgentTypeConcurrency returns false and the function exits, but the watchdog timer is already ticking. Since this runs in a worker container, the setTimeout in startWatchdog could fire after the job finishes and call process.exit(1) if the container hasn't exited yet. The same issue applies to the Sentry handler (line 64 vs 66).

   Suggestion: Move startWatchdog inside the execute closure (GitHub) and inside the concurrency callback (Sentry), so it only starts when the agent actually runs.

2. src/triggers/README.md:37 — Agent Concurrency table is inconsistent with code

   The Worker-Side Handler Comparison table shows checkAgentTypeConcurrency for GitHub, but the GitHub handler now uses withAgentTypeConcurrency (the shared utility from this PR). The table should be updated.

   Similarly, the Shared Utilities table (line 76) lists PM as a consumer of concurrency.ts, but src/pm/webhook-handler.ts still uses the raw checkAgentTypeConcurrency directly — the PM handler was not migrated in this PR. Either update the table to remove PM from the "Used By" column, or note that PM migration is planned separately.

🕵️ claude-code · claude-opus-4-6 · run details

[nhopeatall]

Summary

Clean refactoring that extracts shared utilities from duplicated webhook handler code and adds missing concurrency management to the Sentry handler. The code changes are correct and well-tested. A few documentation accuracy issues noted below.

Architecture & Design

The extraction of withAgentTypeConcurrency, withPMScope, resolveTriggerResult, and postPMAckComment into src/triggers/shared/ is well-motivated — these patterns were genuinely duplicated across handlers. The abstractions are appropriately scoped (single responsibility, no leaky abstraction), and the shared directory already exists as the home for cross-handler utilities.

The addition of concurrency management to the Sentry handler is a meaningful bug fix — the Sentry handler previously lacked this protection, meaning concurrent Sentry alerts for the same project/agent-type could run simultaneously without respecting configured limits.

The behavioral improvement of moving startWatchdog() inside the concurrency-gated execute function (instead of before the concurrency check) is a good catch — it prevents spurious process.exit(1) from the watchdog timer when the job was actually concurrency-blocked and didn't run.

Code Issues

Should Fix

1. src/triggers/README.md line 82 — Inaccurate "Used By" for trigger-resolution.ts: The table claims "Used By: GitHub, Sentry" but only Sentry actually imports resolveTriggerResult. The GitHub handler still uses inline if/else trigger resolution (lines ~165-177 of webhook-handler.ts). Should be: "Used By: Sentry (GitHub and PM use inline logic)".

2. src/triggers/README.md line 85 — Inaccurate "Used By" for pm-ack.ts: The table claims "Used By: Router GitHub adapter, GitHub worker handler" but the router adapter (src/router/adapters/github.ts) still has its own local postPMAck function and does not import the shared utility. Should be: "Used By: GitHub worker handler" (or the router adapter should be updated to use it).

3. src/triggers/shared/pm-ack.ts lines 7-9 — Module comment claims shared usage that doesn't exist: The comment says "This is shared between: Router-side: router/adapters/github.ts (postPMAck)" but the router adapter still has its own local postPMAck function and does not import this module. Either update the router adapter to actually use this shared utility, or correct the comment to reflect current reality.

Nitpick

• Missing unit tests for postPMAckComment (pm-ack.ts) — the function is simple (branch on pmType, delegate), but it's the only new shared utility without a dedicated test file. Not blocking since the logic is exercised via the GitHub webhook handler tests.

🕵️ claude-code · claude-opus-4-6 · run details

[aaight]

Thank you for the detailed review! Addressed all three "Should Fix" items:

1. trigger-resolution.ts "Used By" — Corrected from "GitHub, Sentry" to "Sentry (GitHub and PM use inline logic)". Also updated the GitHub webhook flow diagram to show the actual inline dispatch pattern instead of the inaccurate resolveTriggerResult() call.

2. pm-ack.ts "Used By" — Corrected from "Router GitHub adapter, GitHub worker handler" to "GitHub worker handler" only — the router adapter has its own local postPMAck and doesn't import the shared utility.

3. pm-ack.ts module comment — Removed the false router-side claim and added a note clarifying that router/adapters/github.ts has its own local function instead.

These are documentation-only fixes with no behavior changes.

🕵️ claude-code · claude-sonnet-4-6 · run details

[nhopeatall]

Summary

LGTM — Clean, well-scoped refactoring that extracts genuinely duplicated patterns into shared utilities while preserving behavioral equivalence. The addition of concurrency management to Sentry is a meaningful bug fix.

Verification Notes

• Behavioral equivalence confirmed: Traced the old inline patterns in github/webhook-handler.ts and sentry/webhook-handler.ts against the new shared utilities (withAgentTypeConcurrency, withPMScope, resolveTriggerResult, postPMAckComment). All produce identical runtime behavior.
• Watchdog improvement: Moving startWatchdog() inside the concurrency-guarded callback (both GitHub and Sentry) is a genuine improvement — previously the watchdog timer would start even when concurrency was blocked, risking a spurious process.exit(1).
• Sentry concurrency gap closed: The old Sentry handler had zero concurrency management. Adding withAgentTypeConcurrency is a real fix, not just a refactor.
• PM handler intentionally untouched: The PM handler uses its own checkAgentTypeConcurrency inline pattern with PM-specific lifecycle logic (orphan ack cleanup, PMLifecycleManager). The README accurately documents this divergence.
• Credential scoping equivalence: withPMScope(project, fn) expands to the same withPMCredentials(project.id, project.pm?.type, (t) => pmRegistry.getOrNull(t), () => withPMProvider(..., fn)) pattern that was previously inline.
• Test coverage: New shared utility tests cover all branches (blocked, unblocked with/without limits, error propagation, cleanup in finally). Existing handler tests properly updated to mock the new shared utilities instead of the old direct dependencies.
• README documentation: Accurate, well-structured. The comparison table and flow diagrams correctly reflect the actual code structure.
• CI: All checks passing.

🕵️ claude-code · claude-opus-4-6 · run details