feat(github-events): ingest deployment + deployment_status webhooks (#2324)

[lsm]

Subscribes to deployment and deployment_status webhooks and emits .deployment_<state> / .deployment_status_<state> external events keyed on the PR resolved from the deployment ref/sha — the foundation for required-deployments gating on PR workflows.

Deployment payloads carry no pull_requests array, so the PR is resolved via the GitHub API (commit-SHA lookup, then head-ref fallback) before normalizing. Deployments not attributable to a tracked PR return HTTP 202 (deliberately dropped — nothing to recover); transient resolution failures (missing token / API error / timeout) return HTTP 503 so GitHub redelivers once the credential is back, since webhook-only ingestion has no polling fallback and a lost deployment_status could stall the gate. Events stay under resource=pull_request so existing subscribe_pr_events subscribers receive them automatically.

Closes #2324.

[chatgpt-codex-connector]

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

[lsm]

🤖 Review by glm-5.1 (Zhipu/GLM)

Model: glm-5.1 | Client: NeoKai | Provider: Zhipu (GLM)

Recommendation: REQUEST_CHANGES (one comment-accuracy + design-decision item; the rest are nits/non-blocking)

Overall

Solid, well-scoped foundation. The two normalizers are pure and symmetric with normalizeGitHubCheckRun; mapEventType, essence projection, dedupe keys, and topic literals are all correct; and the subscribe_pr_events wildcard (pull_request/N.*) matches the new topics automatically — verified the trie expands 7.* to ^7\.[^/]*$, so 7.deployment_status_success matches. PR resolution (SHA → head-ref fallback) is sensible, bounded (≤2 calls, 5s cap), and every drop path (not-attributable AND transient failure) returns HTTP 202 and is tested. 200 tests across the 3 touched files pass; typecheck + knip clean (no new unused exports — the new barrel re-exports are all consumed).

Verified via sub-agents: publish path has no resource/action allowlist; dedupe (ON CONFLICT(dedupe_key) DO NOTHING) correctly suppresses redelivery while publishing fresh status rows; no missed event-kind enumeration sites (the legacy lib/github/ module is a separate system, correctly untouched); adding the events to REQUIRED_WEBHOOK_EVENTS is benign — checkWebhook is read-only status, no repeated patching, fails loudly+safely if GitHub ever rejects the names.

Findings

P2 — Inaccurate retry/recovery comment + an availability tradeoff worth deciding explicitly (github-event-extension.ts normalizeDeploymentWebhook doc comment)
The comment states "the dedupe layer absorbs any GitHub retry that lands after the credential is available." GitHub only redelivers a webhook on a non-2xx/timeout response; because the handler returns 202 for these drops, GitHub will not redeliver, so that recovery path doesn't exist. A transient token outage / API 5xx / network blip during delivery permanently loses the event — and with webhook-only ingestion there's no polling to recover it. For the eventual "required deployments to succeed" gate, a transient failure on a deployment_status_success could stall a PR indefinitely. The 202-drop is a reasonable scoped choice and matches the PR description — just needs (a) the comment corrected so a future maintainer doesn't assume retry-based recovery, and (b) an explicit decision on whether transient failures should return non-2xx (e.g. 502/503) to let GitHub's automatic retry re-attempt once the token is back, keeping 202 only for the genuinely-unattributable case (deploy to default branch). See the anchored line comment.

P3 (nit) — essence-contract test only covers deployment_status, not deployment
The formatter has distinct branches for both (event-essence.ts), but external-event-essence-contract.test.ts adds only the deployment_status case. The deployment branch is trivial field-copying and its normalizer payload is tested, so risk is low — a symmetric deployment case would just make the contract complete.

Non-blocking observations

• Attribution via /commits/{sha}/pulls will also match a merge commit to its (already-merged) PR, so a production deploy off the default branch could surface a deployment_status_* event on a long-merged PR. Harmless for the PR-head-deploy primary path; worth keeping in mind when a gating consumer keys off PR state.
• Two parallel GitHub-webhook modules exist (this external-events one + the legacy lib/github/). Pre-existing duplication, not introduced here.

No correctness, security, or backwards-compat blockers. The P2 is the only thing I'd like addressed before merge.

[greptile-apps]

Greptile Summary

This PR adds webhook ingestion for deployment and deployment_status GitHub events, emitting them as external events keyed on the resolved PR — the foundation for required-deployments gating. Because these webhook payloads carry no pull_requests array, the PR is resolved out-of-band via up to two sequential GitHub API calls (commit-SHA lookup, then head-ref fallback) before normalization; events that cannot be attributed to a tracked PR or whose resolution fails are dropped gracefully with HTTP 202.

• Adds normalizeGitHubDeployment and normalizeGitHubDeploymentStatus normalizers, routes deployment events through a new normalizeDeploymentWebhook path in GitHubEventExtension, and exposes the new event types under resource=pull_request so existing subscribe_pr_events subscribers receive them automatically.
• Registers deployment and deployment_status in the WEBHOOK_EVENTS list and extends the essence, event-extension, and normalizer test suites with comprehensive coverage of the new flow (SHA lookup, ref fallback, unresolvable drops, and error tolerance).

Confidence Score: 4/5

Safe to merge — the new deployment ingestion path is well-isolated, all error paths return HTTP 202 rather than crashing, and the deduplication layer protects against webhook retries.

The core resolution flow and normalizers are correct. The three findings are all non-blocking: a missing essence contract test for deployment events, a ref-fallback that only searches open PRs without explaining why, and the two sequential 5 s timeouts that together can approach GitHub's delivery deadline. None affect runtime correctness for the common case, but the timeout budget arithmetic and the open-only ref filter are worth tracking before required-deployments gating goes live and webhook volume increases.

Files Needing Attention: github-event-extension.ts (timeout budget and open-only ref filter) and external-event-essence-contract.test.ts (missing deployment essence test).

Important Files Changed

Filename	Overview
packages/daemon/src/lib/external-events/github/github-event-extension.ts	Adds normalizeDeploymentWebhook, resolvePrNumberForDeployment, fetchDeploymentPrNumber, isShaLike, and pickPrNumberFromPulls; registers deployment events in WEBHOOK_EVENTS. Logic is sound but the sequential 5 s timeouts can together reach GitHub's 10 s delivery deadline.
packages/daemon/src/lib/external-events/github/github-normalizer.ts	Adds normalizeGitHubDeployment and normalizeGitHubDeploymentStatus with correct deduplication keys, topic mapping, and null-safe field extraction. mapEventType extended correctly for both new event kinds.
packages/daemon/src/lib/external-events/event-essence.ts	Adds deployment and deployment_status branches to formatExternalEventEssence with the expected field projections; no issues.
packages/daemon/tests/unit/2-handlers/github/external-event-essence-contract.test.ts	Adds a deployment_status essence contract test; the deployment event type has no corresponding essence test, leaving the payload projection unverified against the contract.
packages/daemon/tests/unit/2-handlers/github/github-event-extension.test.ts	Comprehensive integration-level tests covering SHA lookup, ref fallback, unresolvable drop, network failure tolerance, and webhook subscription list. Well-structured with reusable payload helpers.
packages/daemon/tests/unit/2-handlers/github/github-normalizer.test.ts	Good unit coverage of both normalizers: state routing, topic suffix, null-guard on missing prNumber, and all documented deployment_status states.
packages/daemon/src/lib/external-events/github/index.ts	Re-exports the two new normalizers, repoFromPayload, and associated types; straightforward barrel update.

Sequence Diagram

sequenceDiagram
    participant GH as GitHub
    participant EE as GitHubEventExtension
    participant NW as normalizeGitHubWebhook
    participant ND as normalizeDeploymentWebhook
    participant API as GitHub REST API
    participant ES as ExternalEventStore

    GH->>EE: POST /webhook/github (deployment or deployment_status)
    EE->>EE: verify HMAC signature
    EE->>NW: normalizeGitHubWebhook(eventType, payload)
    NW-->>EE: null (deployment types fall through)
    EE->>ND: normalizeDeploymentWebhook(eventType, deliveryId, payload)
    ND->>ND: extract repo, ref, sha from payload
    ND->>API: "GET /commits/{sha}/pulls (timeout 5s)"
    alt SHA resolves a PR
        API-->>ND: PR list with number N
        ND->>ND: "prNumber = N"
    else SHA empty or timeout
        API-->>ND: empty or AbortError
        ND->>API: "GET /pulls?state=open&head={owner}:{ref} (timeout 5s)"
        alt Ref resolves a PR
            API-->>ND: PR list with number N
            ND->>ND: "prNumber = N"
        else No match or failure
            ND-->>EE: null
            EE-->>GH: HTTP 202 dropped
        end
    end
    ND->>ND: normalizeGitHubDeployment or normalizeGitHubDeploymentStatus
    ND-->>EE: NormalizedGitHubEvent (topic suffix deployment_state)
    EE->>ES: publishEvent(spaceId, normalized)
    EE-->>GH: HTTP 200

Loading

Comments Outside Diff (1)

1. packages/daemon/tests/unit/2-handlers/github/external-event-essence-contract.test.ts, line 444-492 (link)

   Missing essence contract test for deployment events

   deployment_status has an essence contract test verifying its field projections (state, environment, targetUrl, etc.) and the rawPayload exclusion sentinel. The deployment event type has no equivalent test, so its essence branch in formatExternalEventEssence (fields deploymentId, environment, ref, sha, task, description) is unverified by the contract suite. If the field list in event-essence.ts drifts from what consumers expect, no test would catch it.

   Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

_{Reviews (1): Last reviewed commit: "feat(github-events): ingest deployment +..." | Re-trigger Greptile}

[greptile-apps]

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

[lsm]

🤖 Review by glm-5.1 (Zhipu/GLM)

Model: glm-5.1 | Client: NeoKai | Provider: Zhipu (GLM)

Recommendation: APPROVE

Re-reviewed commit 2b72d0e fresh. All prior findings addressed; zero P0–P3 code findings remain.

P2 (redelivery) — resolved with option (b), correctly. Transient ref/sha→PR resolution failures now return HTTP 503 (GitHub redelivers once the credential is back); 202 is kept only for genuinely-unattributable deployments. The sawError-based discrimination in resolvePrNumberForDeployment is sound and conservative: if any lookup errored AND no PR resolved, it's transient (retry); only when all lookups completed cleanly with no PR is it unattributable (drop). I walked the edge cases (SHA errors + ref resolves → publish; SHA ok-empty + ref errors → 503; both ok-empty → 202; neither errors but PR found → publish) — all converge correctly. The "dedupe absorbs retry" framing is fixed in the handler doc, the resolution doc, and the timeout constant; the recovery path it now describes is accurate (503 → redeliver → dedupe absorbs the eventual duplicate). Test coverage updated to assert 503 on transient failure while keeping the 202 unattributable test.

P3 (essence test) — resolved. Symmetric deployment essence-contract test added, asserting environment/ref/sha/task/description projection and rawPayload exclusion.

Verification: 201 tests pass across the 3 touched files; typecheck + knip clean. PR is open and mergeable; all 3 review threads (mine + 2 Greptile) resolved, 0 unresolved. Integration paths re-confirmed clean on the first round (publish/dedupe/subscribe/webhook-config) and untouched by this fix — the change is surgical to the handler/resolution flow + docs.

One trivial, non-blocking note: the PR description still reads "any resolution failure … dropped gracefully (HTTP 202)" — that now contradicts the 503 behavior in code. Optional 1-line edit to the PR body for accuracy; not a code finding and doesn't affect correctness.

Nice fix.