feat(signals): Signal model + GitHub webhook ingest (event spine)#118
Merged
Conversation
Event spine v1: normalized Signal rows upserted idempotently on a delivery-GUID dedupeKey; HMAC-verified /api/ingest/github route that answers fast and defers to a graphile-worker ingest:github job; pure per-event mappers (pull_request, issues, check_suite, deployment_status, release, projects_v2_item) with fixture tests; open-PR cache write-through so /pulls updates without waiting for the 5-minute cron; App manifest now subscribes the six events and points its webhook at the ingest route (manual enablement for existing apps documented). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
|
Important Review skippedNo new commits to review since the last review. ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdds a Prisma ChangesSignal event spine
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/server/github/manifest.ts (1)
26-47: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winAdd
deployments: "read"to the manifest
deployment_statusrequires the Deployments repository permission, so this manifest can’t enable that event without it. Mirror the same permission in the setup docs.🤖 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 `@src/server/github/manifest.ts` around lines 26 - 47, The GitHub manifest in the manifest builder is missing the Deployments repository permission needed for the deployment_status event. Update the default_permissions in the GitHub manifest to include deployments: "read", and mirror the same permission in the related setup docs so the app can subscribe to deployment_status successfully.
🧹 Nitpick comments (1)
src/server/signals/github.ts (1)
306-308: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winRedundant repo lookup within the same job.
processGithubEventresolves the repo viaprisma.repo.findUnique({ where: { nameWithOwner: mapped.repoFullName } })(Line 307), andapplyPullCacheUpdateperforms the same lookup again forpull_requestevents (Line 345). Passing the already-resolved repo through toapplyPullCacheUpdateavoids a duplicate DB round trip per webhook delivery.♻️ Proposed refactor to reuse the resolved repo
- if (job.event === "pull_request") { + if (job.event === "pull_request") { try { - await applyPullCacheUpdate(mapPullRequestCacheUpdate(payload)); + await applyPullCacheUpdate(mapPullRequestCacheUpdate(payload), repo); } catch (error) { // The Signal row is already written — a cache hiccup must not fail (and re-run) the job. console.warn("ingest:github pull cache write-through failed", briefError(error)); } } } -async function applyPullCacheUpdate(update: PullCacheUpdate | null): Promise<void> { +async function applyPullCacheUpdate( + update: PullCacheUpdate | null, + knownRepo?: { id: string; nameWithOwner: string } | null, +): Promise<void> { if (!update) return; if (update.op === "delete") { await prisma.pullRequest.deleteMany({ where: { nodeId: update.nodeId } }); return; } - const repo = await prisma.repo.findUnique({ where: { nameWithOwner: update.repoFullName } }); + const repo = + knownRepo && knownRepo.nameWithOwner === update.repoFullName + ? knownRepo + : await prisma.repo.findUnique({ where: { nameWithOwner: update.repoFullName } }); if (!repo) return; // repo not cached yet — the next syncRepos run picks it upAlso applies to: 339-353
🤖 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 `@src/server/signals/github.ts` around lines 306 - 308, `processGithubEvent` is doing a repo lookup that `applyPullCacheUpdate` repeats for pull_request events, causing an unnecessary extra DB round trip. Refactor the flow so the repo resolved in `processGithubEvent` is passed into `applyPullCacheUpdate` (and any related call sites) instead of re-fetching it there. Update `applyPullCacheUpdate` to accept and reuse the existing repo object, with `prisma.repo.findUnique` used only once per webhook delivery.
🤖 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.
Inline comments:
In `@src/app/api/ingest/github/route.ts`:
- Around line 37-38: The GitHub ingest route currently ignores the boolean
result from enqueueIngestGithub, so a failed enqueue is still acknowledged with
success. Update the route handler in the GitHub webhook endpoint to check the
return value from enqueueIngestGithub and return a non-200 error response when
it is false, while preserving the current success response only for successful
enqueues. Use the route handler and enqueueIngestGithub as the key symbols to
locate the change.
In `@src/server/signals/github.ts`:
- Around line 297-353: The pull request cache write-through in
processGithubEvent/applyPullCacheUpdate can be overwritten by out-of-order
webhook jobs because updates are applied unconditionally by nodeId. Add a
staleness guard using the existing ghUpdatedAt value in the derived
PullCacheUpdate fields so older events are ignored, or otherwise serialize
updates per nodeId before calling prisma.pullRequest.upsert/deleteMany. Use
applyPullCacheUpdate, PullCacheUpdate, and the prisma.pullRequest write path to
locate the fix.
---
Outside diff comments:
In `@src/server/github/manifest.ts`:
- Around line 26-47: The GitHub manifest in the manifest builder is missing the
Deployments repository permission needed for the deployment_status event. Update
the default_permissions in the GitHub manifest to include deployments: "read",
and mirror the same permission in the related setup docs so the app can
subscribe to deployment_status successfully.
---
Nitpick comments:
In `@src/server/signals/github.ts`:
- Around line 306-308: `processGithubEvent` is doing a repo lookup that
`applyPullCacheUpdate` repeats for pull_request events, causing an unnecessary
extra DB round trip. Refactor the flow so the repo resolved in
`processGithubEvent` is passed into `applyPullCacheUpdate` (and any related call
sites) instead of re-fetching it there. Update `applyPullCacheUpdate` to accept
and reuse the existing repo object, with `prisma.repo.findUnique` used only once
per webhook delivery.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 8485f997-8c89-44da-9251-ec24018e142b
📒 Files selected for processing (10)
docs/wiki/GitHub-App-Setup.mdprisma/migrations/signal_event_spine/migration.sqlprisma/schema.prismasrc/app/api/ingest/github/route.tssrc/server/github/manifest.test.tssrc/server/github/manifest.tssrc/server/jobs/enqueue.tssrc/server/jobs/worker.tssrc/server/signals/github.test.tssrc/server/signals/github.ts
…-order webhooks (#115) Route returns 503 when the ingest queue is unavailable so GitHub retries instead of silently dropping a verified delivery. Pull-cache write-through now applies conditionally on ghUpdatedAt (WHERE-guarded updateMany/deleteMany, create-race tolerated) so a stale event processed late can neither overwrite nor delete newer cached state. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The deployment_status webhook event requires the Deployments repository permission — added deployments:read to the manifest and the setup docs. processGithubEvent now passes its already-resolved repo into applyPullCacheUpdate, avoiding a duplicate DB lookup per pull_request delivery. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
# Conflicts: # prisma/schema.prisma
Contributor
Author
|
Addressed the review's two out-of-diff comments in a129d08:
Also merged @coderabbitai please re-review the latest commit. |
Contributor
|
Good call on also merging ✅ Action performedReview finished.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #115
First increment of Phase A (epic #110): the event spine replacing polling-only freshness.
What
Signalmodel (migrationsignal_event_spine): normalized fleet events with a uniquededupeKey, optional repo link, trimmed JSON payload.POST /api/ingest/github: verifiesX-Hub-Signature-256(constant-time) against the stored App webhook secret (503 unconfigured / 401 invalid), answers 200 fast and defers to a newingest:githubgraphile-worker job (jobKey = delivery GUID → redeliveries coalesce).dedupeKey = github:<event>:<delivery-GUID>— GitHub reuses the GUID on redeliveries, so retries upsert onto the same Signal while distinct occurrences stay distinct. (Deliberate refinement of the epic's entity+action sketch: entity-action keys would collapse legitimate repeat occurrences like reopen→close→reopen.)pull_request,issues,check_suite(failure→error, cancelled→warning),deployment_status(failure/error→error),release,projects_v2_item(old+new field values preserved for phase-C cycle analytics) — fixture-tested.pull_requestevents upsert/drop the cachedPullRequestrow immediately (closed → delete, matchingsyncPullssemantics);reviewDecision/checksState/mergeablestay owned by the polling sync.hook_attributes.url→<APP_URL>/api/ingest/github,default_eventsextended to the six spine events; manual enablement for pre-existing apps documented indocs/wiki/GitHub-App-Setup.md.Verification
pnpm check/pnpm test(159 tests, incl. new mapper/signature suites) / docstring gate — all green.repo_provider_keys_multi. Localdb:migrateapply pending (local Docker VM down); will be verified before merge feedback ends.🤖 Generated with Claude Code
Summary by CodeRabbit