fix(github-bot): preserve PR lifecycle webhook fields - #1007
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
📝 WalkthroughWalkthroughWebhook handling now forwards normalized lifecycle data from the parsed action payload to the control plane. Tests mock the control-plane request and verify forwarded fields for closed pull requests. ChangesWebhook automation forwarding
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 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 |
Terraform Validation Results
Pushed by: @ColeMurray, Action: |
There was a problem hiding this comment.
Summary
PR #1007, fix(github-bot): preserve PR lifecycle webhook fields, by @ColeMurray changes 2 files (+64/-4). The change correctly normalizes automation events from the passthrough webhook payload so PR lifecycle fields are preserved, and classifies closed/synchronize as automation-only bot events.
Critical Issues
None.
Suggestions
None.
Nitpicks
None.
Positive Feedback
- The regression test verifies the key lifecycle fields that previously disappeared, including PR state, merge/close timestamps, SHA, branches, URL, labels, and repository identity.
- The forwarding path remains isolated from normal bot handling failures, preserving existing webhook behavior.
- The
automation_event_onlyskip reason makes lifecycle-only handling clearer in logs.
Questions
None.
Verdict
Approve.
Verification: npm test -w @open-inspect/github-bot passed all 128 tests, and npm run typecheck -w @open-inspect/github-bot succeeded.
Terraform Validation Results
Pushed by: @open-inspect[bot], Action: |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/github-bot/test/webhook.test.ts (1)
236-307: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest verifies forwarding but not the new
automation_event_onlyclassification itself.This test confirms the control-plane payload is forwarded correctly for a closed PR, but nothing here (or elsewhere in the diff) asserts that
dispatchHandleractually returnsoutcome: "skipped"/skip_reason: "automation_event_only"forpull_request.closed, nor is there a companion case forpull_request.synchronize. Since classifying these actions as lifecycle-only is a stated PR objective, a direct assertion (e.g., via alog.info/wideEventspy, or by inspecting response semantics if exposed) would close the coverage gap for the actual new logic inindex.tslines 256-261.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/github-bot/test/webhook.test.ts` around lines 236 - 307, Extend the webhook tests around dispatchHandler to directly verify the automation_event_only classification: assert that pull_request.closed produces outcome "skipped" with skip_reason "automation_event_only", and add a companion pull_request.synchronize case with the same classification. Use the existing log.info, wideEvent, or exposed response semantics used by the implementation, while preserving the current forwarding assertions for lifecycle payloads.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/github-bot/test/webhook.test.ts`:
- Around line 236-307: Extend the webhook tests around dispatchHandler to
directly verify the automation_event_only classification: assert that
pull_request.closed produces outcome "skipped" with skip_reason
"automation_event_only", and add a companion pull_request.synchronize case with
the same classification. Use the existing log.info, wideEvent, or exposed
response semantics used by the implementation, while preserving the current
forwarding assertions for lifecycle payloads.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 103b800f-85a5-4b39-9b7f-b68c980c7145
📒 Files selected for processing (2)
packages/github-bot/src/index.tspackages/github-bot/test/webhook.test.ts
Summary
Root cause
The webhook Worker received
pull_request.closed, but the summary schema removed nested lifecycle fields before normalization. The resulting control-plane event lackedpullRequest.state, so lifecycle processing skipped the update.Verification
npm test -w @open-inspect/github-botnpm run typecheck -w @open-inspect/github-botgit diff --checkSummary by CodeRabbit