Skip to content

fix: attribute PR channel merges to merged_by, not author - #64

Merged
shubham5080 merged 1 commit into
mainfrom
fix/pr-merged-by-attribution
Sep 3, 2026
Merged

fix: attribute PR channel merges to merged_by, not author#64
shubham5080 merged 1 commit into
mainfrom
fix/pr-merged-by-attribution

Conversation

@shubham5080

@shubham5080 shubham5080 commented Sep 3, 2026

Copy link
Copy Markdown
Member

Summary

  • GitHub list PRs does not include merged_by, so Discord channel edits fell back to the PR author (e.g. OrgExplorer #214 showed @jikrana1 instead of Ri1tik).
  • On pr_merged ingestion, if list payload lacks merged_by, fetch GET /pulls/{n} and store the real merger.
  • Channel status line never falls back to the author: show Merged by @user only when known, otherwise just Merged.

Test plan

  • pytest tests/test_notifications.py tests/test_github_ingestion_lifecycle_events.py (68 passed)
  • CI green
  • After merge + deploy: next mentor-merged PR should show the merger login, not the author

Made with Cursor

Summary by CodeRabbit

  • Bug Fixes
    • Merged pull requests now preserve the actual merger’s name when available, even if it is missing from the initial results.
    • Merge and close announcements no longer display an incorrect or unknown actor when attribution is unavailable.
    • Merge announcements no longer attribute the action to the pull request author by default.

List-PRs omits merged_by, so channel edits fell back to the PR author.
Fetch GET /pulls/{n} for the real merger, and never blame the author when
the merger is still unknown.

Co-authored-by: Cursor <cursoragent@cursor.com>
@coderabbitai

coderabbitai Bot commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Team

Run ID: 3dc10cee-2da1-4e20-82ee-27cd960e3c55

📥 Commits

Reviewing files that changed from the base of the PR and between 7c25ac2 and 67cd709.

📒 Files selected for processing (4)
  • src/ghdcbot/adapters/github/rest.py
  • src/ghdcbot/engine/notifications.py
  • tests/test_github_ingestion_lifecycle_events.py
  • tests/test_notifications.py

Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.


Walkthrough

Merged pull request ingestion now fetches missing merger details. Lifecycle notifications no longer attribute merges to the PR author or display @unknown when no actor exists.

Changes

Merged pull request actor handling

Layer / File(s) Summary
Merged PR detail fallback
src/ghdcbot/adapters/github/rest.py, tests/test_github_ingestion_lifecycle_events.py
The adapter fetches full PR details when the list response lacks merged_by. Tests verify the merger login in the pr_merged event.
Lifecycle actor rendering
src/ghdcbot/engine/notifications.py, tests/test_notifications.py
Merge notifications use only merged_by. Missing actors produce **Status:** Merged without attribution. Close notifications retain their fallback order.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Merge Risk: ⚪ Minimal · up to 67cd7

Merged PR notifications now use the actual merger when available and otherwise omit attribution rather than naming the PR author. The change is covered across ingestion and notification behavior with no remaining merge-readiness risk identified.

Sequence Diagram(s)

sequenceDiagram
  participant GitHub
  participant RESTAdapter
  participant NotificationEngine
  participant Discord
  GitHub->>RESTAdapter: List merged pull requests
  RESTAdapter->>GitHub: Fetch details when merged_by is absent
  GitHub-->>RESTAdapter: Return merged_by
  RESTAdapter->>NotificationEngine: Emit pr_merged event
  NotificationEngine->>Discord: Update lifecycle announcement
Loading

Poem

A rabbit fetched the merger’s name
The list was thin, the detail came
No unknown shadow crossed the screen
The status stayed clear and clean
Hop, hop, merged events now gleam

🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: attributing PR channel merges to merged_by instead of the PR author.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/pr-merged-by-attribution

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actions github-actions Bot added size/M and removed size/M labels Sep 3, 2026
@shubham5080
shubham5080 merged commit 2c7dc70 into main Sep 3, 2026
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant