refactor: extract shared agent pipeline from webhook handlers#307
refactor: extract shared agent pipeline from webhook handlers#307zbigniewsobiecki wants to merge 3 commits intodevfrom
Conversation
Merge dev to main
Merge dev to main
|
🔍 Reviewing PR... |
aaight
left a comment
There was a problem hiding this comment.
Summary
Good refactoring intent but there's a behavioral regression in the GitHub webhook handler: the shared pipeline unconditionally calls lifecycle.handleSuccess/handleFailure for all agents with a workItemId, which conflicts with the original GitHub handler's selective behavior. This results in double handleSuccess calls for implementation agents and new lifecycle side effects for other agent types that didn't have them before.
Code Issues
Blocking
- src/triggers/github/webhook-handler.ts:60-61 + src/triggers/shared/agent-pipeline.ts:171-172 —
handleSuccessis called twice for GitHub implementation agents. The pipeline callslifecycle.handleSuccess(workItemId, agentType, agentResult.prUrl)unconditionally at line 172 ofagent-pipeline.ts(wheneverworkItemIdis present and the agent succeeds), and then the GitHub handler calls it again at line 61 for implementation agents. The original code only called it once, from the handler.
Should Fix
- src/triggers/shared/agent-pipeline.ts:170-178 — The original GitHub handler intentionally did NOT call
lifecycle.handleSuccessfor non-implementation agents (review, respond-to-review, check-failure), and never calledlifecycle.handleFailureat all. The shared pipeline now applies these calls to all agent types. This changes card state management behavior for GitHub-triggered agents. Consider adding ahandleLifecycleResultboolean option (defaulttrue) that the GitHub handler can set tofalse, and have the GitHub handler continue to manage its own selective success/failure handling post-pipeline.
Suggestion
The pipeline needs a way to opt out of the success/failure lifecycle handling, similar to how prepareLifecycle and cleanupLifecycle already work. Something like:
/** Whether to call handleSuccess/handleFailure after execution (default: true) */
handleLifecycleResult?: boolean;Then the GitHub handler can set handleLifecycleResult: false and keep its own conditional handleSuccess call for implementation agents only, while Trello and JIRA handlers use the default true.
Everything else in this PR looks correct:
- Credential scoping correctly mirrors the original per-handler logic
extractPRUrlunification uses the more correct regex (from claude-code) — the base.ts regex was slightly more permissive but the new one is better- JIRA handler correctly injects
cardIdintoagentInput - Trello
setCardActivecall is preserved outside the pipeline - Tests are well-structured and cover the key pipeline behaviors
- LLM env cleanup in
withProjectCredentialscorrectly uses try/finally
| ); | ||
| // GitHub-specific: Move to in-review if implementation completed successfully | ||
| if (cardId && result.agentType === 'implementation' && agentResult.success) { | ||
| await lifecycle.handleSuccess(cardId, result.agentType, agentResult.prUrl); |
There was a problem hiding this comment.
[BLOCKING] Double handleSuccess call: The pipeline (agent-pipeline.ts:172) already calls lifecycle.handleSuccess for all successful agents with a workItemId. This line calls it again for implementation agents. The original code only called it here (once), because the pipeline equivalent didn't exist.
Fix: Either (1) add a handleLifecycleResult: false option to the pipeline so the GitHub handler manages its own success/failure, or (2) remove this line and ensure the pipeline's unconditional handleSuccess is acceptable for all GitHub agent types (but note the original code intentionally skipped handleSuccess for non-implementation agents).
|
|
||
| // Success/failure handling | ||
| if (agentResult.success) { | ||
| await lifecycle.handleSuccess(workItemId, agentType, agentResult.prUrl); |
There was a problem hiding this comment.
[SHOULD_FIX] This unconditional handleSuccess call is correct for Trello and JIRA handlers, but changes behavior for GitHub handlers. The original GitHub handler only called handleSuccess for implementation agents specifically, and never called handleFailure (line 174). Consider adding a handleLifecycleResult boolean option (like prepareLifecycle/cleanupLifecycle) to let callers opt out.
Summary
Refactors webhook handlers to eliminate ~835 lines of duplicated agent execution logic by extracting shared components:
executeAgentPipeline()) consolidates budget-check → lifecycle → runAgent → artifacts → success/failure → auto-debug flowwithProjectCredentials()) handles PM-type-specific credential resolutionextractPRUrl()) utility deduplicated from agents/base.ts and backends/claude-code/index.tsChanges
New files
src/triggers/shared/agent-pipeline.ts— Core pipeline orchestration with configurable lifecycle hookssrc/triggers/shared/credential-scope.ts— Automatic credential scoping based on PM type (Trello/JIRA/GitHub-only)src/utils/prUrl.ts— GitHub PR URL extraction utilityRefactored handlers
src/triggers/trello/webhook-handler.ts— Reduced from 266 to 165 lines (-38%)src/triggers/jira/webhook-handler.ts— Reduced from 280 to 185 lines (-34%)src/triggers/github/webhook-handler.ts— Reduced from 292 to 217 lines (-26%)Tests
tests/unit/triggers/agent-pipeline.test.ts— 8 tests covering pipeline orchestrationtests/unit/triggers/credential-scope.test.ts— 7 tests covering credential resolutiontests/unit/utils/prUrl.test.ts— 10 tests covering URL extraction edge casesTest plan
Key decisions
prepareLifecycle/cleanupLifecycleflags handle GitHub's unique lifecycle (no prep/cleanup)onAgentFailureallows GitHub handler to update PR comments on failureRelated
Implements refactoring plan from card: https://trello.com/c/699351b37cca0bcb78648214
🤖 Generated with Claude Code