Skip to content

feat(github-events): ingest pull_request_review_thread webhook (#2322) - #2349

Open
lsm wants to merge 4 commits into
devfrom
space/github-events-ingest-pull-request-review-thread-webhook-2322
Open

feat(github-events): ingest pull_request_review_thread webhook (#2322)#2349
lsm wants to merge 4 commits into
devfrom
space/github-events-ingest-pull-request-review-thread-webhook-2322

Conversation

@lsm

@lsm lsm commented Aug 3, 2026

Copy link
Copy Markdown
Owner

Subscribes to the pull_request_review_thread (resolved/unresolved) webhook and re-expresses it as .thread_resolved / .thread_unresolved topics — github/owner/repo/pull_request/N.thread_resolved — so the require-conversation-resolution rule can react to thread state changes instead of re-querying the reviewThreads GraphQL connection each time.

Unlike review-comment events, this payload carries the review-thread node id directly (thread.node_id = PullRequestReviewThread.id), so the normalizer populates a resolveHandle keyed to it with no runtime lookup. Resolution toggles recur (resolve → unresolved → resolve), so the dedupe identity includes the delivery id, mirroring the pull_request pattern.

Touches the normalizer (GitHubEventKind, accept guard, normalization branch, mapEventType), the essence formatter (projects threadId/path/line), and WEBHOOK_EVENTS so auto-registered hooks receive the event and validateRemoteHook enforces it.

Subscribe to the pull_request_review_thread (resolved/unresolved) webhook and
re-express it as .thread_resolved / .thread_unresolved topics, powering the
require-conversation-resolution rule as an event-driven signal.

Unlike review-comment events, this payload carries the review-thread node id
directly (thread.node_id = PullRequestReviewThread.id), so the normalizer
populates a resolveHandle keyed to it without a runtime GraphQL reviewThreads
lookup. Resolution toggles recur (resolve -> unresolved -> resolve), so the
dedupe identity includes the delivery id, mirroring the pull_request pattern.
@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

@greptile-apps

greptile-apps Bot commented Aug 3, 2026

Copy link
Copy Markdown

Greptile Summary

This PR wires up the pull_request_review_thread (resolved/unresolved) GitHub webhook end-to-end: normalizer, essence formatter, WEBHOOK_EVENTS list, and UI help text. The key design insight is that this is the only GitHub webhook payload that carries the PullRequestReviewThread.id directly (thread.node_id), so the normalizer can populate a resolveHandle without a runtime GraphQL lookup.

  • Normalizer branch (github-normalizer.ts): extracts thread.node_id as nodeId/threadId, sources occurredAt from pr.updated_at (bumped by the resolution action), falls back to root-comment timestamps for thin payloads, and keys the dedupe identity on the delivery ID so resolution toggles (resolve → unresolve → resolve) each produce a distinct event.
  • Essence projection (event-essence.ts): new branch copies threadId, path, line, and side into the lean essence, matching the pull_request_review_comment projection so agents get full location context without a get_external_event round-trip.
  • Tests: four new normalizer unit tests (resolved/unresolved, dedupe non-collision, absent thread.node_id, thin-PR occurredAt fallback), a new essence-contract test asserting the sentinel-exclusion guarantee, and an end-to-end extension integration test.

Confidence Score: 5/5

Safe to merge — the change is additive (new webhook event type, new normalization branch, new essence projection) with no modifications to existing event handling paths.

Every code path introduced by this PR is exercised by dedicated tests: resolved/unresolved actions, dedupe non-collision across resolution toggles, graceful degradation on absent thread.node_id, occurredAt fallback for thin PR payloads, essence sentinel exclusion, and the full extension integration path. The normalizer design is consistent with existing patterns, and the critical design properties (delivery-ID-keyed dedupe, pr.updated_at timestamp sourcing) are all verified in the test suite.

Files Needing Attention: No files require special attention. The commentId field doc in github-normalizer.ts is the only stale artefact.

Important Files Changed

Filename Overview
packages/daemon/src/lib/external-events/github/github-normalizer.ts Adds pull_request_review_thread to GitHubEventKind, the accept guard, and the normalization branch; dedupe key includes delivery ID to handle resolution toggles; occurredAt sources from pr.updated_at with comment-timestamp fallbacks; mapEventType maps to thread_resolved/thread_unresolved.
packages/daemon/src/lib/external-events/event-essence.ts Adds a new branch for pull_request_review_thread that projects threadId, path, line, and side into the lean essence, matching the pull_request_review_comment projection pattern.
packages/daemon/src/lib/external-events/github/github-event-extension.ts Adds pull_request_review_thread to WEBHOOK_EVENTS so auto-registered hooks and validateRemoteHook enforce the new subscription.
packages/daemon/tests/unit/2-handlers/github/github-normalizer.test.ts Four new tests cover: resolved/unresolved actions, dedupe non-collision across resolution toggles, graceful degradation when thread.node_id is absent, and occurredAt fallback to root-comment timestamp on thin PR payloads.
packages/daemon/tests/unit/2-handlers/github/external-event-essence-contract.test.ts New essence-contract test asserts threadId, resolveHandle, body, path, line, side are projected and that raw payload sentinel values do not leak into the essence.
packages/daemon/tests/unit/2-handlers/github/github-event-extension.test.ts Integration test delivers a pull_request_review_thread webhook and asserts the .thread_resolved topic and resolveHandle payload; four existing WEBHOOK_EVENTS snapshot assertions updated to include the new event type.
packages/web/src/components/space/SpaceExternalEventsSettings.tsx UI help text updated to mention pull_request_review_thread alongside the other manually-configurable webhook events.

Reviews (2): Last reviewed commit: "docs(web): list pull_request_review_thre..." | Re-trigger Greptile

Comment thread packages/daemon/src/lib/external-events/github/github-normalizer.ts Outdated
Comment thread packages/daemon/src/lib/external-events/event-essence.ts Outdated
…ect side

Address review feedback on #2349 (greptile):

- occurredAt now keys off pr.updated_at (bumped by the resolution action) with
  the root-comment timestamps as fallbacks for thin payloads. The previous
  rootComment.updated_at only tracks comment-text edits, which can be days
  stale relative to the actual resolve/unresolve transition.
- Project `side` into the pull_request_review_thread essence, matching the
  pull_request_review_comment projection so the full path/line/side location
  reaches agents.
Comment thread packages/daemon/src/lib/external-events/github/github-normalizer.ts Outdated
Comment thread packages/daemon/src/lib/external-events/event-essence.ts Outdated

@lsm lsm left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 Review by GLM-5.1 (GLM)

Model: GLM-5.1 | Client: NeoKai | Provider: GLM

Recommendation: REQUEST_CHANGES

The ingestion itself is correct and well-tested. I verified independently against GitHub's webhook + GraphQL reference that thread.node_id is genuinely the PullRequestReviewThread.id that resolveReviewThread(input:{threadId}) requires (REST node_id == GraphQL id), so populating resolveHandle without a runtime reviewThreads lookup is sound. The dedupe key correctly anchors on the delivery id, so resolve → unresolved → resolve toggles don't collapse against each other while redeliveries still dedupe. mapEventType, the accept guard, the essence formatter, and WEBHOOK_EVENTS are all updated in lockstep. 195 tests across the three github-handler suites pass.

Two P2 issues block this from landing — both already have correct fixes staged in the worktree (verified green), but the PR head (72fa9fb08) still carries the old code:

  1. P2 — occurredAt sourcing (github-normalizer.ts:313, comment). rootComment.updated_at moves only on comment-body edits, not on resolution; a thread resolved days after its last edit gets a stale timestamp. Prefer pr.updated_at (bumped by the resolution action) with the comment timestamps as fallback.
  2. P2 — side missing from the thread essence (event-essence.ts:71, comment). The review-comment essence projects side; the thread essence does not, though the normalizer populates it. Add 'side' for parity.

Action needed: commit + push the two staged fixes and confirm the github-handler tests stay green.


Non-blocking suggestion (P3): packages/web/src/components/space/SpaceExternalEventsSettings.tsx:772 lists the webhook events for manual local-dev setup and omits pull_request_review_thread. That file isn't in this PR's scope, so I'm not blocking on it — but worth a one-line follow-up so the help text matches WEBHOOK_EVENTS.

Out of scope (not a finding): wiring this event into the require-conversation-resolution validator (pr-ready-validator.ts) so it actually reacts to .thread_resolved/.thread_unresolved instead of re-querying reviewThreads — correctly left for a separate task.

Operational note (not a finding): adding the event to WEBHOOK_EVENTS makes it required, so pre-existing remote hooks will surface a "missing events" error on the next checkWebhook until re-applied via autoConfigureWebhook. This is by design and matches how prior event additions rolled out.

lsm added 2 commits August 2, 2026 23:53
The external-events settings help text enumerates the GitHub webhook events the
daemon subscribes to; add the newly-ingested pull_request_review_thread so the
user-facing list stays accurate.
…vents

The commentId field doc listed thread events among the "empty" cases, but the
pull_request_review_thread branch populates it with the thread's root comment
id; nodeId is the thread node id there too. Bring the field doc in line.

@greptile-apps greptile-apps Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lsm has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.

@lsm lsm left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 Review by GLM-5.1 (GLM)

Model: GLM-5.1 | Client: NeoKai | Provider: GLM

Recommendation: APPROVE

Re-reviewed fresh at head 09180a357. All prior findings are resolved and I found no new issues.

Verified independently:

  • thread.node_id is genuinely the PullRequestReviewThread.id that resolveReviewThread(input:{threadId}) requires (REST node_id == GraphQL id, confirmed against GitHub's webhook + GraphQL reference), so the no-runtime-lookup resolveHandle is sound.
  • The dedupe key anchors on the delivery id, so resolve → unresolved → resolve toggles each stay distinct while redeliveries collapse — covered by a dedicated test.
  • Both P2s are correctly fixed and now tested: occurredAt sources from pr.updated_at (bumped by the resolution action) with root-comment timestamps as fallback (the assertion proves it picks the PR's 00:10 over the comment's older 00:05, plus a thin-PR fallback test); side is projected into the thread essence, matching the review-comment branch.
  • The P3 help-text and the commentId/nodeId field-doc completeness (the only thing Greptile's re-scan flagged) are both fixed in surgical follow-up commits.
  • 495 github-handler tests pass; tsc --build clean; 4/4 review threads resolved; diff is the feature + fixes + tests + docs across 7 files, no scope creep.

Operational note (not blocking): mergeStateStatus is BLOCKED — expected, since dev is protected and is waiting on review approval + CI. No merge conflicts (mergeable: MERGEABLE).

Out of scope, as noted last round: wiring this event into the pr-ready-validator require-conversation-resolution rule (so it reacts to .thread_resolved/.thread_unresolved instead of re-querying reviewThreads) is correctly left to a separate task.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant