fix(github-bot): run automation forwarding independently - #1519
fix(github-bot): run automation forwarding independently#1519ColeMurray wants to merge 1 commit into
Conversation
Terraform Validation Results
Pushed by: @ColeMurray, Action: |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review. 📝 WalkthroughWalkthrough
ChangesWebhook forwarding
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to The change runs built-in dispatch and automation forwarding independently while preserving existing cleanup behavior; no actionable merge-blocking risk remains after normal checks and review. Sequence Diagram(s)sequenceDiagram
participant handleWebhook
participant GitHubEventDispatch
participant forwardGitHubAutomationEvent
participant ControlPlaneRequest
handleWebhook->>GitHubEventDispatch: start built-in event dispatch
handleWebhook->>forwardGitHubAutomationEvent: start automation forwarding
forwardGitHubAutomationEvent->>ControlPlaneRequest: send signed normalized event
GitHubEventDispatch-->>handleWebhook: return dispatch outcome
ControlPlaneRequest-->>forwardGitHubAutomationEvent: return forwarding outcome
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![open-inspect[bot]](https://avatars.githubusercontent.com/in/2661131?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary
PR #1519, fix(github-bot): run automation forwarding independently by @ColeMurray changes GitHub webhook handling so built-in dispatch and automation-event forwarding start together and settle before dispatch-failure cleanup. Reviewed 2 files (+130/-60); the approach is focused and preserves existing forwarding failure isolation while fixing the delayed-forwarding behavior.
Critical Issues
None found.
Suggestions
None.
Nitpicks
None.
Positive Feedback
- Making
dispatchHandlerasync is a clean way to convert synchronous schema failures into a rejected promise, ensuring forwarding is still started. Promise.allSettledkeeps forwarding failures non-fatal while preserving the existing dispatch-failure dedupe cleanup behavior.- The regression test directly exercises the important ordering: forwarding begins while built-in dispatch is still pending.
Questions
None.
Verification
npm test -w @open-inspect/github-bot: 7 test files passed, 131 tests passed.npm run typecheck -w @open-inspect/github-bot: passed.
Verdict
Approve: ready to merge.
| let result: HandlerResult | undefined; | ||
| let dispatchFailure: { error: unknown } | undefined; | ||
| const normalizationPayload = actionResult.success ? actionResult.data : {}; | ||
| const [dispatchResult, forwardingResult] = await Promise.allSettled([ |
There was a problem hiding this comment.
[deep review] This couples dispatch observability to the independent forwarding path. Because Promise.allSettled waits for both operations, webhook.handled is now delayed by a slow forward (or suppressed by a non-settling one), and duration_ms measures max(dispatch, forwarding) rather than built-in dispatch as it did before. There is a simpler code-judo move: start forwardGitHubAutomationEvent(...) and retain its promise before entering the existing try/await dispatchHandler flow, log dispatch as soon as that settles, then await/log forwarding before rethrowing any captured dispatch error. Starting forwarding first also means dispatchHandler does not need to become artificially async; its synchronous schema errors remain caught by the existing try. That preserves the cleanup policy while deleting the two settled-result state machines and retaining accurate telemetry.
There was a problem hiding this comment.
[deep review] Requesting changes for the dispatch-observability regression noted inline. The two operations should start independently, but dispatch logging and timing should not wait on forwarding. Starting the forwarding promise first and then retaining the existing dispatch try/await flow preserves the intended cleanup behavior with substantially less orchestration.
1 participant
Summary
Why
Follow-up to #1506. That change preserves forwarding after a dispatch rejection, but forwarding still did not begin until built-in dispatch settled. A slow or non-settling handler could therefore continue to block an otherwise independent automation event.
Testing
Summary by CodeRabbit