feat(webhooks): log decision reason for each webhook call#645
feat(webhooks): log decision reason for each webhook call#645zbigniewsobiecki merged 2 commits intodevfrom
Conversation
|
✅ Review submitted for PR #645 — clean feature, one minor issue flagged (duplicate decision reason string in Steps 1/2 of webhook processor). See review comments for details. |
nhopeatall
left a comment
There was a problem hiding this comment.
Summary
Clean, well-structured feature that threads decisionReason consistently through the full webhook pipeline — from processor to DB to CLI/Dashboard. Migration is safe (nullable ADD COLUMN), tests cover all decision points, and the UI changes are correct (colSpan updated, truncation with title tooltip). One minor issue worth addressing.
Code Issues
Should Fix
- src/router/webhook-processor.ts:64 — Step 2 (filter check) reuses the identical decision reason as Step 1 (parse failure): both return
'Event unparseable or not processable'. Step 2 fires after the event was successfully parsed — it specifically means "event type not processable" (e.g., a board-level action). Using the same string makes it impossible to distinguish these two distinct cases in webhook logs, which partially defeats the purpose of this feature. Suggest something like'Event type not processable'or'Event filtered (type not handled)'for Step 2.
| if (!adapter.isProcessableEvent(event)) { | ||
| logger.debug(`Ignoring ${adapter.type} event`, { eventType: event.eventType }); | ||
| return { shouldProcess: false }; | ||
| return { shouldProcess: false, decisionReason: 'Event unparseable or not processable' }; |
There was a problem hiding this comment.
[SHOULD_FIX] This reuses the same decision reason string as Step 1 (line 58), but these are distinct decision points. Step 1 means the payload couldn't be parsed at all; Step 2 means it parsed successfully but the event type isn't processable. Using the same string makes these indistinguishable in logs.
Suggest differentiating, e.g.:
The test at line 95 would need updating too.
There was a problem hiding this comment.
Fixed in f267235. Step 2 now returns "Event type not processable: <eventType>" (with the actual event type interpolated), clearly distinguishing it from Step 1's "Event unparseable or not processable". Updated the corresponding test assertion as well.
|
@aaight address code review concerns |
…bhook processor Step 2 (event type filter) now returns "Event type not processable: <eventType>" instead of reusing the Step 1 (parse failure) reason string, making the two distinct decision points distinguishable in logs. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Code Review Feedback AddressedWhat was askedAddress the code review concern about Step 1 and Step 2 in Changes Made
Verification
|
nhopeatall
left a comment
There was a problem hiding this comment.
LGTM — Clean, well-structured feature addition that threads decisionReason through all layers consistently. The nullable TEXT column migration is safe, types are aligned across all interfaces (ProcessRouterWebhookResult → WebhookLogOverrides → WebhookLogInput → InsertWebhookLogInput → DB), every decision branch in processRouterWebhook() provides a distinct human-readable reason, and tests cover all paths. Dashboard and CLI display are handled correctly (colSpan bump, truncation in list view, full display in detail view). No issues found.
Summary
decision_reasonTEXT column towebhook_logsDB table with migration0027_webhook_decision_reason.sqldecisionReasonthrough the full pipeline:ProcessRouterWebhookResult→WebhookLogOverrides→WebhookLogInput→InsertWebhookLogInput→ DB insertprocessRouterWebhook()(unparseable, self-authored, no project, no trigger, no-agent trigger, work-item locked, concurrency limit, queue failed, job queued)createWebhookHandlerfactory (logs"Parse failed: <error>")decisionReasonfrom Trello/GitHub/JIRA router handlers to log overrideswebhooklogs list, new "Decision Reason" field inwebhooklogs showWebhookLogsTableand "Decision Reason" field inWebhookLogDetailDialogwebhookLogger.test.ts,webhookHandlers.test.ts,webhook-processor.test.tsnow verifydecisionReasonpropagation at all decision pointsTest plan
ALTER TABLE webhook_logs ADD COLUMN decision_reason TEXT;— safe nullable ADD COLUMNcascade webhooklogs list/showand Dashboard UITrello card: https://trello.com/c/69abfd22ea94baebe551922b
🤖 Generated with Claude Code