refactor(webhooks): extract platform adapters and shared org-access guard#518
refactor(webhooks): extract platform adapters and shared org-access guard#518
Conversation
Manual and retry runs called runAgent() directly without establishing AsyncLocalStorage-based PM context, causing agents (especially review) to crash with "No PMProvider in scope". Webhook-triggered agents already had this wrapping via withPMCredentials() + withPMProvider(). Wrap the runAgent() call in triggerManualRun() with the same PM context pattern used by the GitHub webhook handler. withPMCredentials gracefully falls through when no PM integration is configured. Closes #518 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
🔍 Reviewing code — Examining the PR changes for quality and correctness... |
nhopeatall
left a comment
There was a problem hiding this comment.
Summary
Clean refactoring that extracts webhook platform logic into adapter classes and deduplicates org-access verification. The code is a faithful mechanical extraction — I verified that all platform-specific logic, dedup checks, error codes, and control flow are preserved.
Notes
Minor: Lost error message in runs.ts trigger mutation
The original trigger mutation threw TRPCError({ code: 'NOT_FOUND', message: 'Project not found' }) when project org verification failed. The shared verifyProjectOrgAccess throws TRPCError({ code: 'NOT_FOUND' }) without a message. This is inconsequential since clients should rely on error codes, not messages, but worth being aware of.
Minor: verifyRunOrgAccess is dead code
verifyRunOrgAccess is exported and tested but has no callers in production code. The inline pattern (fetch run → check projectId → verify project org) is still used directly in runs.ts. Not blocking — it's tested and may be useful for future consolidation — but if the intent was to use it in runs.ts, that wasn't done.
Observation: Delete endpoint changed from sequential to parallel
The delete mutation now uses Promise.all instead of sequential per-platform deletion. This is a positive change (faster), but it does mean that if one platform's deletion fails, the others still run (and their results are still collected) — which is actually better error isolation than the original sequential approach where a Trello failure would skip JIRA and GitHub deletions entirely.
Everything else checks out: types are properly re-exported for downstream consumers, adapter credential guards match the original inline checks, dedup URL matching is preserved, and test coverage is solid (37 new tests covering all adapters and the org-access helper).
nhopeatall
left a comment
There was a problem hiding this comment.
LGTM - clean mechanical refactoring with no blocking issues. See previous review for detailed notes.
Summary
Refactors
src/api/routers/webhooks.ts(570 lines) using the platform adapter pattern, and extracts a shared org-access guard used 12+ times across routers.WebhookPlatformAdapterpattern: Extracted Trello, GitHub, and JIRA webhook logic into dedicated adapter classes (TrelloWebhookAdapter,GitHubWebhookAdapter,JiraWebhookAdapter) undersrc/api/routers/webhooks/verifyProjectOrgAccess(projectId, orgId)andverifyRunOrgAccess(runId, orgId)extracted tosrc/api/routers/_shared/orgAccess.ts, replacing 8 copy-pasted inline DB verification blocks acrossruns.ts,agentConfigs.ts, andwebhooks.tswebhooks.tsdropped from 570 → 206 lines;runs.tsfrom 322 → 287 lines;agentConfigs.tsfrom 156 → 130 linesNew files:
src/api/routers/_shared/orgAccess.ts— shared org verification helperssrc/api/routers/webhooks/types.ts— webhook payload interfaces +WebhookPlatformAdapter<T>interface +ProjectContextsrc/api/routers/webhooks/trello.ts—TrelloWebhookAdaptersrc/api/routers/webhooks/github.ts—GitHubWebhookAdaptersrc/api/routers/webhooks/jira.ts—JiraWebhookAdapter+jiraEnsureLabelstests/unit/api/routers/_shared/orgAccess.test.ts— 8 tests for org-access helpertests/unit/api/routers/webhooks/adapters.test.ts— 29 tests for all three adapters + jiraEnsureLabelsTest plan
npx vitest run)npm run typecheck)npm run lint)Trello card: https://trello.com/c/f2jtNgDx/99-refactor-webhooks-router-extract-platform-adapters-and-deduplicate-org-access-pattern
🤖 Generated with Claude Code