[github-events] ingest status webhook (commit-status / external CI) [#2321] - #2347
Conversation
Subscribe to the GitHub `status` webhook (commit-status API) used by
external/legacy CI (Jenkins/Travis/custom) and re-express it as
`pull_request/<id>.status_<state>`, surfacing `pending` as well as
`failure`/`error`/`success`.
- Add `status` to WEBHOOK_EVENTS / REQUIRED_WEBHOOK_EVENTS so new and
existing auto-managed hooks subscribe to (and are re-registered for) it.
- normalizeGitHubStatus + mapEventType case in github-normalizer.ts; the
payload addresses a commit SHA (no PR ref), so the PR is resolved by the
caller and the identity is scoped per PR.
- handleStatusWebhook resolves commit SHA -> open PR head(s) via
/commits/{sha}/pulls (filtering out PRs that merely merged the commit),
then publishes per PR. Empty/failed resolution drops the event (202).
- event-essence projects state/description/targetUrl/context/sha/statusId.
Closes #2321.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Greptile SummaryThis PR subscribes to the GitHub
Confidence Score: 5/5Safe to merge — the status webhook ingestion is well-guarded, the two previously identified issues have been addressed, and the new code paths are thoroughly tested. The handler correctly gates the SHA→PR API call behind enabled-space checks, deduplication keys are properly scoped per (CI-system × PR), and all four commit-status states are routed consistently through the existing tone/reactivation logic. Test coverage spans the full branching surface of the new code. Files Needing Attention: No files require special attention.
|
| Filename | Overview |
|---|---|
| packages/daemon/src/lib/external-events/github/github-event-extension.ts | Adds handleStatusWebhook and resolvePullRequestNumbersForCommit — the core of the new feature. Enabled/space checks are correctly placed before the SHA→PR API call, pagination is bounded, and merged-commit false positives are filtered by comparing head.sha. |
| packages/daemon/src/lib/external-events/github/github-normalizer.ts | Adds normalizeGitHubStatus with a per-PR deduplication key scoped by id (or sha:context fallback), and routes status through mapEventType as status_. repoFromPayload is exported for use in the extension. |
| packages/daemon/src/lib/space/runtime/space-runtime.ts | Expands isReactivePrCheckFailure regex to also match .status_failure and .status_error topics, correctly excluding status_pending and status_success from task reactivation. |
| packages/daemon/src/lib/rpc-handlers/live-query-handlers.ts | SQL tone/title CASE expressions updated to treat .status_failure and .status_error topics as danger / CI check failed, matching the semantics of .check_failed. status_pending correctly falls through to neutral PR update. |
| packages/daemon/src/lib/external-events/event-essence.ts | Adds the status event type to formatExternalEventEssence, copying the right fields. Clean, consistent with adjacent branches. |
| packages/daemon/tests/unit/2-handlers/github/github-event-extension.test.ts | New tests cover: failure/pending resolution, empty-result drop, merged-commit filter, unwatched-repo 404, and the disabled-target quota guard. |
| packages/daemon/tests/unit/2-handlers/github/github-normalizer.test.ts | Tests all four states, per-PR dedupe scoping, id-absent CI collision fix, commit.sha fallback, and null returns for missing fields. |
| packages/daemon/tests/unit/2-handlers/rpc-handlers/live-query-handlers.test.ts | Adds a test verifying that status_failure/status_error render as danger/CI check failed and status_pending renders neutral. |
| packages/daemon/tests/unit/5-space/runtime/space-runtime-external-events.test.ts | Two new tests verify that status_failure reactivates a done task and that status_pending/success do not. |
Sequence Diagram
sequenceDiagram
participant GH as GitHub
participant WH as Webhook Handler
participant EXT as handleStatusWebhook
participant DB as WatchedRepo DB
participant API as GitHub API
participant PUB as EventPublisher
GH->>WH: POST /webhook/github/space
WH->>WH: verify signature to signatureMatchedRepos
WH->>EXT: handleStatusWebhook(deliveryId, payload, matchedRepos)
EXT->>EXT: extract repo + sha from payload
alt repo or sha missing
EXT-->>GH: 202 Event ignored
end
EXT->>EXT: filter matchedRepos to validForRepo
alt validForRepo empty
EXT-->>GH: 404 Repository not watched
end
loop each watched in validForRepo
EXT->>DB: check watched.enabled
EXT->>DB: getSpaceConfig(spaceId)
note over EXT: collect into targets[]
end
alt targets empty
EXT-->>GH: 200 spaces:0
end
EXT->>API: GET /commits/sha/pulls
API-->>EXT: PR list filtered by head.sha
alt prNumbers empty
EXT-->>GH: 202 no_pull_request
end
loop each watched in targets
loop each prNumber
EXT->>EXT: normalizeGitHubStatus()
EXT->>PUB: publishEvent(spaceId, normalized)
end
EXT->>DB: markWebhookReceived(watched.id)
end
EXT-->>GH: 200 spaces:N
Reviews (3): Last reviewed commit: "fix(space): wire external-CI status fail..." | Re-trigger Greptile
…in dedupe [#2321] - handleStatusWebhook now collects enabled targets (watched row + space enabled) before the SHA→PR resolution GET, so a delivery for a repo watched only by disabled rows/spaces no longer consumes GitHub quota. - normalizeGitHubStatus includes the CI context in the id-absent dedupe fallback so two CI systems on the same SHA/state/PR cannot collide.
lsm
left a comment
There was a problem hiding this comment.
🤖 Review by glm-5.1 (NeoKai)
Model: glm-5.1 | Client: NeoKai | Provider: z.ai
Recommendation: REQUEST_CHANGES
The ingestion itself is clean and well-tested — webhook subscription, SHA→PR resolution with correct head-sha filtering (excludes merged-commit false positives), PR-scoped dedupe, all four states including pending, and a matching essence projection. The two Greptile P2s are already resolved in 68670f5a (enabled-guard short-circuit before the resolution GET; context-aware dedupe fallback) with covering tests. I re-ran the two test files (195 pass) and typecheck/lint are clean.
Two integration findings remain — both about making external/legacy CI failures as actionable as native check_run failures:
P1 — status_failure / status_error don't reactivate a done task
isReactivePrCheckFailure (packages/daemon/src/lib/space/runtime/space-runtime.ts:779) hard-matches .check_failed only:
/^github\/[^/]+\/[^/]+\/pull_request\/[^/.]+\.check_failed$/i.test(event.topic)A done PR task is reactivated on a native check_run failure, but a status_failure/status_error from Jenkins/Travis/custom CI is dropped as target_task_terminal in prepareExternalEventTask (the reactive path is the only action-sensitive branch). Since surfacing external CI is the whole point of #2321, a legacy-CI failure silently doing nothing while a GitHub-Actions failure reopens the task is a confusing gap — the events are ingested and delivered to in-progress agents, but they can't reopen a completed one.
Suggested fix — widen the predicate to also match the failure states (not pending/success):
/^github\/[^/]+\/[^/]+\/pull_request\/[^/.]+(?:\.check_failed|\.status_(?:failure|error))$/iIf you intend to defer this, please open a tracked follow-up and call it out in the PR description — but my read is that it belongs in the PR that introduces status_failure.
P2 — Activity feed renders external CI failures as neutral "PR update"
packages/daemon/src/lib/rpc-handlers/live-query-handlers.ts:1347-1358 special-cases only %.check_failed as danger / "CI check failed". A status_failure/status_error falls through to WHEN ee.topic LIKE '%pull_request%' THEN 'PR update' → neutral tone. External CI failures should get the same treatment so the feed surfaces them as failures, e.g.:
WHEN ee.topic LIKE '%.check_failed'
OR ee.topic LIKE '%.status_failure'
OR ee.topic LIKE '%.status_error' THEN 'danger'(and the matching 'CI check failed' title branch).
Reviewed against working tree @ 68670f5a (both commits). Tests: 195 pass / 0 fail across the two changed test files; tsc + oxlint clean.
…vity feed [#2321] - isReactivePrCheckFailure now also matches pull_request/<id>.status_failure and .status_error, so an external-CI (Jenkins/Travis) failure reopens a done PR task the way a native check_failed does. pending/success stay excluded (they are not failures). - live-query activity feed renders .status_failure / .status_error as danger 'CI check failed' (previously fell through to neutral 'PR update'), matching the check_failed treatment.
lsm
left a comment
There was a problem hiding this comment.
🤖 Review by glm-5.1 (NeoKai)
Model: glm-5.1 | Client: NeoKai | Provider: z.ai
Recommendation: APPROVE
Both findings from my prior review are resolved in 8ff37547a, re-verified fresh this round (zero new findings):
- P1 —
isReactivePrCheckFailure(packages/daemon/src/lib/space/runtime/space-runtime.ts:779) now matches.check_failed | .status_failure | .status_errorvia[^/.]+\.(?:check_failed|status_(?:failure|error))$, so an external-CI failure reopens adonePR task like a nativecheck_run;pending/successstay excluded. Reactivation test (status_failure) + negative test (status_pending stays terminal) added. - P2 — activity-feed SQL (
packages/daemon/src/lib/rpc-handlers/live-query-handlers.ts) now classifies.status_failure/.status_errorasdanger/"CI check failed", matchingcheck_failed; pending renders neutral. Tests added.
Verified: 294 pass / 0 fail (live-query-handlers + space-runtime-external-events), 195 pass / 0 fail (github ingestion tests); tsc, oxlint, knip all clean; 0 unresolved review threads. The ingestion logic and the earlier Greptile fixes (68670f5a) remain intact.
Nice work — external/legacy CI is now first-class alongside native check_run across ingestion, reactivation, and the activity feed.
Subscribes to the GitHub
statuswebhook (the commit-status API used by external/legacy CI — Jenkins, Travis, custom) and re-expresses it aspull_request/<id>.status_<state>, surfacingpendingas well asfailure/error/success.The payload addresses a commit SHA, not a PR, so
handleStatusWebhookresolves the SHA to the open PR(s) whose head it is via/commits/{sha}/pulls(filtering out PRs that merely merged the commit) before publishing. Empty or failed resolution drops the event.statusis added toWEBHOOK_EVENTS/REQUIRED_WEBHOOK_EVENTSso auto-managed hooks subscribe and re-register for it.Closes #2321.