feat: add GitHub event integration health panel - #2254
Conversation
Expose a consolidated GitHub event-subsystem health snapshot via a new `space.github.health` RPC. The extension now retains the last rate-limit observation from each poll cycle and aggregates it with token status, polling config, per-repo webhook/poll freshness, reaction-poll targets, and the most recent failed deliveries into a single response so operators have one place to verify the subsystem is healthy.
Render a consolidated health summary inside the Space external-events settings: token, polling interval, rate-limit, webhook registration, reaction freshness, and recent delivery errors, with a Healthy/Degraded/ Down badge plus Poll-now (test delivery) and Re-register webhooks actions. Backed by the new space.github.health RPC.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 42fd1b279c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
lsm
left a comment
lsm
left a comment
There was a problem hiding this comment.
🤖 Review by glm-5.1 (NeoKai)
Model: glm-5.1 | Client: NeoKai | Provider: zai
Recommendation: REQUEST_CHANGES
The health RPC and panel are well structured and the new tests are solid — I verified the new daemon health test, the web panel + settings tests, the 154 existing extension/rate-limit tests, and the full bun run check gate all pass. The aggregation shape, the field accesses against ExternalEventDeliveryLogRecord / ExternalEvent, and the capability gating are all correct. Five P2 issues remain, though, and four of them undercut the panel's core promise — an operator trusting the badge and rate-limit readout to mean "the subsystem is healthy":
- Rate-limit flaps to "Unknown" after a cached poll —
github-event-extension.ts:2221.lastRateLimitInfois overwritten with anInfinity-remaining snapshot on every all-304 cycle, which is the steady state, so the rate-limit panel is effectively blank in normal operation. (Highest practical impact.) - Disabled Spaces can show "Healthy" —
github-event-extension.ts:984. The webhook tally ignoresrepo.enabled, so a disabled space with active remote hooks reports a live delivery path it doesn't have (the webhook handler skips disabled rows;pollingRepoCountis already 0). - Bulk re-register targets manual webhooks —
GitHubHealthPanel.tsx:214. For a manually-configured hook,autoConfigureWebhookcreates a second remote hook, replaces the stored manual secret, and orphans the original. NeedswebhookAutoRegisteredexposed in the snapshot + filtering. busycan stick across Space switches —GitHubHealthPanel.tsx:203. Clear it unconditionally infinally; the stale-result guards belong only on the snapshot/loading setters.- Panel ignores the parent
disabled={saving}lock —SpaceExternalEventsSettings.tsx:687. Threaddisabledinto the panel and OR it into the action buttons.
Findings 1–4 were independently surfaced by the codex inline review; I re-confirmed each against the code and posted anchored comments with suggested fixes. Happy to re-review once these are addressed.
- Preserve rate-limit budget across all-304 poll cycles: merge (not
overwrite) the cycle snapshot so a cached, header-less 304 response can
no longer clobber a previously observed finite remaining/reset.
- Exclude disabled repositories from the webhook health summary so a
disabled space no longer reports a live delivery path it doesn't have.
- Limit bulk re-register to auto-managed hooks (add webhookAutoRegistered
to the repo rollup); manual hooks are no longer sent to
autoConfigureWebhook, which would orphan their original secret.
- Clear the action busy state unconditionally on completion so a mid-action
space switch cannot permanently disable the panel.
- Thread the parent disabled={saving} lock into the panel's action buttons.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6e72b023bb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
lsm
left a comment
lsm
left a comment
There was a problem hiding this comment.
🤖 Review by glm-5.1 (NeoKai)
Model: glm-5.1 | Client: NeoKai | Provider: zai
Recommendation: REQUEST_CHANGES
Round-2 re-review. All five round-1 findings are correctly fixed and regression-tested — I verified the rate-limit merge (mergeRateLimitInfo now preserves a finite budget across all-304 cycles; observedAt only advances on a finite observation), the disabled-row skip in the webhook/poll aggregates (webhook.total still reports all repos; the rollup still lists them with their enabled flag), the webhookAutoRegistered field + bulk-re-register filter, the unconditional setBusy(null), and the threaded disabled lock. 160 daemon tests (incl. 6 health) and 57 web tests (incl. 12 panel) pass, and the full bun run check gate is green.
Two new P2 findings remain, both on GitHubHealthPanel.tsx (surfaced by the codex review and re-confirmed here):
-
Cross-space stale-snapshot race on Re-register (line 231) — the panel is reused across Spaces and
snapshotisn't cleared on switch, so in the load window Re-register filters the previous Space's repos while the request targets the new Space. Real cross-space webhook-registration contamination. Clearsnapshoton space change, or gate actions onsnapshot.spaceId === current. -
"checked now ago" webhook-check display (line 479) —
relativeFromNowreturns'now'for past timestamps, so the last-checked age is always nonsensical. Use an elapsed/absolute formatter for this historical value.
Inline comments posted with fixes. One more round and this should be ready.
- Render webhook lastCheckedAt with an elapsed formatter ("checked 3h
ago"); relativeFromNow is a future-countdown helper and returned "now"
for the past timestamp, producing a misleading "checked now ago".
- Clear the snapshot on space change and gate re-register targets on
snapshot.spaceId === current spaceId, so a stale snapshot from a
navigated-away space can no longer feed re-register targets that would
be configured against the new space.
lsm
left a comment
lsm
left a comment
There was a problem hiding this comment.
🤖 Review by glm-5.1 (NeoKai)
Model: glm-5.1 | Client: NeoKai | Provider: zai
Recommendation: APPROVE
Round-3 re-review — all findings resolved, zero remaining.
Round-2 fixes verified correct: the cross-space stale-snapshot race is closed with a double guard — useEffect([spaceId]) now clears snapshot/error before re-fetching, and both the re-register target list and the button-enable count additionally gate on snapshot.spaceId === current — so a navigated-away space's repos can no longer be re-registered against the new spaceId. Manual Refresh correctly bypasses the clear to avoid a flash. The "checked now ago" display is fixed via a new relativeAgo elapsed formatter; relativeFromNow is retained for future epochs (rate-limit/retry reset). New regression tests cover both — including an end-to-end test that rerenders a new space while its health is pending and asserts Re-register stays disabled until the matching snapshot arrives.
Full PR re-checked across all three commits: the space.github.health aggregation shape, field accesses against ExternalEventDeliveryLogRecord/ExternalEvent, capability gating, and the panel's badge/action logic are all sound. 160 daemon tests (incl. 6 health RPC) and 59 web tests (incl. 14 panel) pass; the full bun run check gate (lint + typecheck + knip + session-guards + db-schema-parity + test-quality) is green. All 14 review conversations are resolved; PR is open and mergeable.
Posted as COMMENT because the reviewer and author share the lsm GitHub account (GitHub rejects APPROVE on self-authored PRs); the recommendation is APPROVE.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b0331f84f7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
deriveStatus treated any pollingRepoCount > 0 as a working delivery path, but configured polling rows survive a global interval of 0 (timer stopped). A polling-only space was therefore badged Healthy while its own Polling metric read Disabled. Polling now counts as a path only when the capability is on, the interval is nonzero, and the space has polling repos.
lsm
left a comment
lsm
left a comment
There was a problem hiding this comment.
🤖 Review by glm-5.1 (NeoKai)
Model: glm-5.1 | Client: NeoKai | Provider: zai
Recommendation: APPROVE (round-4 confirmation on 8d5c6617e)
Verified the round-4 fix for the polling-disabled-as-delivery-path finding: deriveStatus now derives pollingActive = globallyEnabled && intervalMs > 0 && pollingRepoCount > 0 before counting polling as a working delivery path, so a polling-only space whose global interval is 0 (timer stopped, no events deliverable) is badged Down to match its Disabled Polling metric — no longer a false Healthy. The regression test (shows Down when polling repos exist but the global poll interval is 0) covers it.
60 web tests (incl. 15 panel) pass and the full bun run check gate is green at this commit. All 15 review conversations are resolved; PR is open and mergeable. Overall verdict across all four commits remains APPROVE.
Posted as COMMENT (reviewer and author share the lsm account); recommendation is APPROVE.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8d5c6617e2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
lsm
left a comment
lsm
left a comment
There was a problem hiding this comment.
🤖 Review by glm-5.1 (NeoKai)
Model: glm-5.1 | Client: NeoKai | Provider: zai
Recommendation: REQUEST_CHANGES — supersedes my round-3/round-4 APPROVE.
A fresh pass surfaced a cluster of four related P2 correctness gaps in the health-status derivation. They share one root cause: deriveStatus (and the daemon's webhook/poll aggregates) compute "is there a working delivery path" and "is the subsystem degraded" from row/config state without reflecting actual delivery semantics. The round-4 polling fix addressed one instance; these are the rest. I verified each against the code.
- Disabled webhook capability still reads as a delivery path (
GitHubHealthPanel.tsx:149) — whencapabilities.webhooksis false the webhook handler returns202 "Event ignored"and publishes nothing, yetwebhook.active > 0still marks the space Healthy. Add a webhook capability flag to the snapshot (mirrorpolling.globallyEnabled) and gate the path on it. - Token is required even for webhook-only delivery (
GitHubHealthPanel.tsx:150) —handleWebhookverifies the stored secret and never resolves a token, so a webhook-only space with no PAT (removed, or manual signed hook) still delivers but is badged Down. Require a token only for paths that need one (polling), not unconditionally. recentErrorshas no recency window (github-event-extension.ts:1040) —listDeliveryLog({status:'failed'})has no date filter, so one old terminal failure keeps the badge Degraded forever. Bound the health rollup to a window (e.g. 24h); keep the full log for diagnostics.- Verified manual webhooks are never an active path (
github-event-extension.ts:1001) — manual hooks lack awebhookRemoteId, socheckWebhookcan't populatewebhookActive(staysnull→ countedunknown), and a manual-webhook-only space is always Down even while delivering. Use recent successful delivery (lastWebhookAt) as the live signal for manual hooks.
Suggested holistic approach rather than four piecemeal patches: define per-channel "live path" predicates — polling live iff capability+interval+repos (done); webhook live iff webhooks-capability-on AND (active auto-managed hook OR manual hook with fresh delivery) — then require a token only when the only live path is polling, and bound recentErrors by time. Inline comments posted for each with concrete fixes. Happy to re-review once addressed.
Rework deriveStatus and the health snapshot so the badge reflects actual delivery paths rather than stale config: - Add webhook.deliveryEnabled to the snapshot (source on AND webhooks capability not disabled); a webhook-only space no longer reads Healthy when the handler ignores every delivery. - A delivering webhook path is now (active auto hook) OR (any hook that has successfully delivered via lastWebhookAt), so manual webhooks — which never get a remote active status — count as a live path. - Require a token only for the polling path; inbound webhook delivery verifies the stored secret and needs no PAT, so a webhook-only space without a token is no longer badged Down. - Bound recentErrors to a 24h window so one old terminal failure can no longer hold the badge at Degraded indefinitely.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dc26a7088f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
lastWebhookAt, lastCheckedAt, and webhook errors were aggregated from every enabled repo regardless of webhookEnabled, so a repo whose webhook was toggled off kept contributing its historical lastWebhookAt and could be treated as a live delivery path. These webhook-specific aggregates now only count for currently webhook-enabled rows.
lsm
left a comment
lsm
left a comment
There was a problem hiding this comment.
🤖 Review by glm-5.1 (NeoKai)
Model: glm-5.1 | Client: NeoKai | Provider: zai
Recommendation: REQUEST_CHANGES (round-6). The four round-5 findings (A/B/C/D) are correctly fixed and regression-tested, but a fresh pass surfaced one new P2 of the same root-cause family. I verified everything against the pushed commit dc26a7088 (working tree clean, HEAD == dc26a70).
Verified fixed (all at HEAD):
- A — webhook capability —
webhook.deliveryEnabled(=globallyEnabled && capabilities.webhooks !== false) now gates the webhook path inderiveStatus; a webhook-only space with the capability off is Down. ConfirmedisWebhookDeliveryEnabledsemantics. - B — token for webhook-only — token is now required only inside
pollingLive, not the top-leveldowncheck; webhook-only spaces without a PAT are no longer Down (inbound webhooks verify the stored secret, neverresolveToken). - C — recentErrors recency — bounded to a 24h window (
HEALTH_RECENT_ERROR_WINDOW_MS), filter applied post-query;listDeliveryLogordersupdated_at DESCso thelimit:5+ window combo is correct for the badge (any within-24h failure is in the newest set). Full log retained for the deliveries view. - D — manual webhooks —
webhookLivenow also countslastWebhookAt !== null, so manual hooks that have delivered are a live path. Disabled-space (!repo.enabledskip) and disabled-repo handling preserved.
414 daemon tests pass (incl. the full github suite, +8 health), 18 web panel tests pass (+3), and the full bun run check gate (lint + typecheck + knip + session-guards + space-task-handler-tests + db-schema-parity + test-quality) is green at dc26a7088.
New P2 (independently confirmed; also surfaced by codex):
- E — stale
lastWebhookAt/webhookErrorsfrom a webhook-disabled repo leak into the rollup (github-event-extension.ts:1029). Theactive/inactive/unknowntally is correctly gated onwebhookEnabled, butlastWebhookAt,webhookErrors, andlastCheckedAtare aggregated unconditionally. Since toggling a webhook off (upsertWatchedRepowithwebhookEnabled:false) only deletes the remote hook and retains the historicallastWebhookAt, a webhook-disabled repo can makewebhookLivetrue (false Healthy) when polling is down, and a stalewebhookLastErrorcan trip false Degraded. Gate the three webhook-specific aggregations onwebhookEnabled. Anchored comment + reply to the codex thread posted.
One more change and this should land. Posted as COMMENT (reviewer and author share the lsm account); the recommendation is REQUEST_CHANGES.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 359854553e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
The Refresh button's disabled predicate omitted the `disabled` prop, so it stayed clickable during a settings save while Poll now and Re-register were already locked. Refresh now honors the parent disabled state too.
lsm
left a comment
lsm
left a comment
There was a problem hiding this comment.
🤖 Review by glm-5.1 (NeoKai)
Model: glm-5.1 | Client: NeoKai | Provider: zai
Recommendation: APPROVE (round-8). Zero findings remain. Every finding from rounds 1–7 is verified fixed and regression-tested at 7a6dd03f4 (working tree clean, HEAD == remote tip); I verified each fix against the pushed commit.
Health-status derivation now judges the "working delivery path" from real delivery semantics rather than configured rows:
- Polling live iff capability on AND interval>0 AND polling-repos>0 AND token configured.
- Webhook live iff
deliveryEnabled(capability on) AND (an active auto-managed hook OR has delivered — covers manual hooks that never get a remote active status). - Token required only for the polling path (inbound webhooks verify the stored secret and never resolve a token).
recentErrorsbounded to a 24h window; the full unbounded log stays in the deliveries view.- All webhook aggregates (
active/inactive/unknown,lastWebhookAt,webhookErrors,lastCheckedAt) gated onwebhookEnabled; disabled-space rows skipped. Toggled-off webhooks no longer leak a stalelastWebhookAt(false Healthy) or stalewebhookLastError(false Degraded).
Panel actions: all three (Poll now, Re-register webhooks, Refresh) honor the parent disabled={saving} lock; Re-register targets only auto-managed hooks and is gated on snapshot.spaceId === current; busy clears unconditionally in finally; webhook-check age uses an elapsed formatter.
Tests & gate: 415 daemon tests (incl. the github suite + health RPC) and 18 web panel tests pass; the full bun run check gate (lint + typecheck + knip + session-guards + space-task-handler-tests + db-schema-parity + test-quality) is green at 7a6dd03f4.
PR state: OPEN, MERGEABLE, targets dev, 0 of 26 review threads unresolved.
Process note: in round 6 two of my inline comments posted with the literal @/path as the body (lowercase -f body=@file does not interpret @); both were PATCHed to their intended content, so no review content was lost.
Posted as COMMENT because the reviewer and author share the lsm account (GitHub rejects APPROVE on self-authored PRs); the recommendation is APPROVE.
There was a problem hiding this comment.
💡 Codex Review
Reject manual polls when the global interval is disabled
When the global poll interval is set to 0 while the polling capability remains enabled, the new Poll now action can still reach this RPC. It checks only the capability and pollSpace likewise has no interval guard, so it performs GitHub API requests and can publish events despite the integration reporting polling as disabled and the scheduler being stopped. Enforce the zero-interval gate here (and disable the action from the snapshot) so the global setting consistently disables polling.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
A token that is present but rejected by GitHub (token.error, e.g. /user 401) is not a functioning polling credential — every poll with it fails — so it can no longer keep the polling path live. pollingLive now requires a token with no validation error. Webhook delivery is unaffected (it uses the stored secret), so a webhook space with a bad token still has a path and lands in Degraded rather than Down.
lsm
left a comment
lsm
left a comment
There was a problem hiding this comment.
🤖 Review by glm-5.1 (NeoKai)
Model: glm-5.1 | Client: NeoKai | Provider: zai
Recommendation: REQUEST_CHANGES (round-9) — supersedes my round-8 APPROVE.
A new P2 surfaced (codex comment, independently verified) shortly after I posted the round-8 APPROVE — it was genuinely zero-findings at that instant. I'm correcting the verdict rather than leave a "zero findings, APPROVE" standing against a valid finding.
G — a rejected polling token should read as Down, not Degraded (GitHubHealthPanel.tsx, the pollingLive predicate). getTokenStatus returns { configured: true, error: 'HTTP 401' } when the stored PAT is rejected (any non-ok /user, or a thrown fetch). pollingLive checks only token.configured, not whether the token validated, so it stays true. A polling-only space with a dead token therefore hits the token.error branch and is badged Degraded — even though every poll with that token 401s and publishes nothing, i.e. there is no functioning delivery path → it should be Down. (If the space also has a live webhook, webhookLive keeps it off Down, which is correct.)
Fix (one line + a panel test): const tokenValid = snapshot.token.configured && !snapshot.token.error;, then use tokenValid in pollingLive. New test: polling-only space with token: { configured: true, error: 'HTTP 401' } → Down. The webhook-only-without-token case (round-5 finding B) stays correct because webhookLive doesn't depend on the token.
Everything else from rounds 1–8 remains verified-fixed at 7a6dd03f4. Posted as COMMENT (reviewer and author share the lsm account); the recommendation is REQUEST_CHANGES.
lsm
left a comment
lsm
left a comment
There was a problem hiding this comment.
🤖 Review by glm-5.1 (NeoKai)
Model: glm-5.1 | Client: NeoKai | Provider: zai
Recommendation: APPROVE (round-10) — supersedes the round-9 REQUEST_CHANGES. Finding G is verified fixed; zero findings remain. I verified against the pushed commit 3809a963b (HEAD == remote tip, clean tree).
G fixed: pollingLive now requires !snapshot.token.error, so a polling-only space whose stored PAT is rejected (configured: true + HTTP 401) has no functioning delivery path and reads Down instead of Degraded. webhookLive is unchanged (it doesn't touch the token), so a webhook space with a bad token still has a path and lands in Degraded via the existing token.error check — correct. New regression test shows Down for a polling-only space whose token is rejected covers it.
deriveStatus now reads as a complete, consistent statement of real delivery semantics:
pollingLive= capability on AND interval>0 AND polling-repos>0 AND token configured AND token valid (!error);webhookLive=deliveryEnabledAND (active auto-managed hook OR has delivered — covers manual hooks);- Down when neither path is live; Degraded on rate-limit / inactive hook / webhook error / recent (24h) delivery failure / token error; else Healthy.
All findings from rounds 1–9 are resolved and regression-tested: rate-limit retention across cached polls; disabled-space / manual-webhook / toggled-off-webhook rollup gating; token-for-polling-only and token-validity; 24h recentErrors window; webhook-capability delivery path; bulk re-register safety (auto-managed only, current-space guard); busy and parent-disabled locks across all three actions; cross-space stale-snapshot guard; elapsed check-age formatter; and rejected-token Down.
19 web panel tests pass and the full bun run check gate (lint + typecheck + knip + session-guards + space-task-handler-tests + db-schema-parity + test-quality) is green at 3809a963b. PR is OPEN, MERGEABLE, targets dev, 0 of 27 review threads unresolved.
This is my final verdict for the merge gate. (Round-9 had corrected an over-hasty round-8 APPROVE when G surfaced post-approval; with G fixed the verdict returns to APPROVE.) Posted as COMMENT because reviewer and author share the lsm account; recommendation is APPROVE — ready for human sign-off and a squash-merge to dev without --delete-branch.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3809a963b3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
lsm
left a comment
lsm
left a comment
There was a problem hiding this comment.
🤖 Review by glm-5.1 (NeoKai)
Model: glm-5.1 | Client: NeoKai | Provider: zai
Note on the two latest codex advisory comments (H, I) on 3809a963b — both assessed and non-blocking. Recommendation remains APPROVE.
- H — disable Poll now when interval=0: the badge already shows a polling-only space at interval=0 as Down (round-4 fix), but
Poll nowis a deliberate manual test/override ("test event delivery") — distinct from the automatic poll timer that interval=0 stops. Keeping it enabled is a defensible design choice, not a correctness regression. Whether interval=0 should also forbid a manual poll is a product call for the human, not a bug. - I — a valid token without watched-repo access still reads Healthy: real precision gap in principle (a PAT that passes
/userbut 403/404s on a private repo leavespollingLivetrue), but closing it properly means tracking per-repository polling success in the rollup — a deeper enhancement beyond this PR's signal model, where liveness is intentionally inferred from setup (capability + interval + repos + token-configured + token-valid). Worth a follow-up issue, not a blocker here.
The distinction I've held across rounds: correctness bugs in the badge logic (A–G, now all fixed and regression-tested) are blockers; design-preference and out-of-scope enhancement suggestions are advisory and belong at the merge gate. The automated reviewer will keep generating advisories on each commit, so the human is the right place to triage them.
Verdict unchanged: APPROVE — all correctness findings resolved at 3809a963b; PR is OPEN, MERGEABLE, targets dev. Posted as COMMENT (reviewer and author share the lsm account).
- Poll now is disabled client-side when the global poll interval is 0, and space.github.pollOnce now rejects in that state (interval 0 means polling disabled globally), so a manual poll can no longer publish behind the setting. - pollWatchedRepo records whether each cycle could reach the repo (per-cursor lastPollError) and the snapshot exposes polling.inaccessibleRepoCount + a per-repo lastPollError. A valid but unauthorized PAT (403/404 on every endpoint) no longer keeps the polling path live: deriveStatus requires at least one accessible polling repo and marks inaccessible repos Degraded.
lsm
left a comment
lsm
left a comment
There was a problem hiding this comment.
🤖 Review by glm-5.1 (NeoKai)
Model: glm-5.1 | Client: NeoKai | Provider: zai
Recommendation: APPROVE (round-11). Findings H and I are implemented correctly at 9dd7a618a with no regressions. (I had assessed both as non-blocking advisories; the author chose to address them, so I verified the implementation.) Verified against the pushed commit (HEAD == remote tip, clean tree).
- H — Poll now honors interval=0: client-side the button is
disabledwhenpollingIntervalEnabled(snapshot.polling.intervalMs > 0) is false, and server-sidespace.github.pollOncethrowsGitHub polling is disabled (interval is 0)whengetPollIntervalMs() <= 0. Both layers enforce it; a manual poll can no longer publish behind a 0 interval. - I — inaccessible polling repos:
pollWatchedRepotracks a per-cursorlastPollError, set when a cycle reaches no accessible endpoint (all 403/404) and cleared on any 200/304. It is preserved (not clobbered) when a cycle breaks on a rate-limit before any access attempt, so a rate-limit can't falsely mark a repo inaccessible. The snapshot exposespolling.inaccessibleRepoCount(counted over the same enabled+polling denominator aspollingRepoCount) plus per-repolastPollError;deriveStatusnow requirespollingRepoCount - inaccessibleRepoCount > 0for the polling path to be live and marks inaccessible repos Degraded. Default is null/optimistic before the first poll, so fresh setups don't false-Down. Sound.
417 daemon tests (incl. the github suite) and 21 web panel tests pass; the full bun run check gate (lint + typecheck + knip + session-guards + space-task-handler-tests + db-schema-parity + test-quality) is green at 9dd7a618a. PR is OPEN, MERGEABLE, targets dev, 0 of 29 review threads unresolved.
This is my final verification — the PR is ready for the merge gate. All findings across rounds 1–11 are resolved and regression-tested, and the health-status derivation is now a complete, consistent statement of real delivery semantics (per-channel live-path predicates: polling capability + interval + accessible repos + valid token; webhooks capability-on + active-or-delivered; token required only for polling; recentErrors bounded to 24h; disabled/toggled-off rows excluded). Posted as COMMENT (reviewer and author share the lsm account); recommendation is APPROVE — ready for human sign-off and a squash-merge to dev without --delete-branch.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9dd7a618ad
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| snapshot.token.configured && | ||
| !snapshot.token.error; |
There was a problem hiding this comment.
For a polling-only Space watching public repositories without a PAT, this unconditionally makes pollingLive false and badges the integration Down, even though pollWatchedRepo deliberately supports that configuration: gitHubPollingHeaders omits Authorization when no token exists and GitHub's public endpoints can still return and publish events. Base polling health on observed repository accessibility rather than requiring token.configured, while continuing to reject a configured token that has a validation error.
Useful? React with 👍 / 👎.
| // Null when this cycle reached at least one endpoint (accessible); the | ||
| // last access error otherwise. Left untouched (preserved) when the cycle | ||
| // broke on a rate-limit before any access attempt. | ||
| lastPollError: accessible ? null : (pollErrorMessage ?? cursor.lastPollError), |
There was a problem hiding this comment.
When one primary endpoint succeeds but a later required endpoint fails—for example, issue comments return 200 while /pulls returns 403 for a fine-grained token lacking pull-request access—accessible is already true, so this assignment clears the captured pollErrorMessage. The cycle then misses PR, check-run, and reaction events while the health panel can report Healthy; retain and expose partial endpoint failures as a degraded condition instead of clearing every error after any single successful response.
Useful? React with 👍 / 👎.
| if (trackedPrs > 0 && repo.lastPollAt) { | ||
| if (lastReactionActivityAt === null || repo.lastPollAt > lastReactionActivityAt) { | ||
| lastReactionActivityAt = repo.lastPollAt; |
There was a problem hiding this comment.
When the primary endpoint responses leave fewer than 100 requests remaining, canPollReactions skips every reaction request without activating the cooldown used below 10, but updatePollCursor still advances repo.lastPollAt. Using that repository timestamp here makes the panel report fresh reaction activity—and potentially a Healthy integration—even though approval reactions were not checked in that cycle; persist and aggregate a timestamp only after an actual successful reaction request.
Useful? React with 👍 / 👎.
| <ErrorList | ||
| heading="Failed deliveries" | ||
| rows={snapshot.recentErrors.map((entry) => ({ | ||
| key: `err:${entry.eventId}`, |
There was a problem hiding this comment.
A source event can fan out to multiple subscriptions, and listDeliveryLog returns one row per delivery, so two failed agents can produce multiple recentErrors entries with the same eventId. Keying all of those rows as err:${entry.eventId} gives Preact duplicate keys and can cause the wrong failure row to be reused or omitted when the snapshot refreshes; include the delivery key in the snapshot and use it in this list key.
Useful? React with 👍 / 👎.
| private async buildHealthSnapshot(spaceId: string): Promise<GitHubHealthSnapshot> { | ||
| const now = Date.now(); | ||
| const watched = this.repo.listWatchedRepos(spaceId); | ||
| const token = await this.getTokenStatus(); |
There was a problem hiding this comment.
When GitHub's /user request stalls, this await prevents every locally available polling, webhook, and delivery metric from being returned; the client eventually times out the RPC and leaves Poll now and re-registration disabled because no snapshot exists. The parent settings flow explicitly fetches token status in the background to remain usable during this condition, so the health RPC should likewise bound validation or return the local snapshot with an unknown/token-error status instead of blocking the entire panel on an unbounded upstream fetch.
Useful? React with 👍 / 👎.
| private lastRateLimitInfo?: GitHubRateLimitInfo; | ||
| /** Wall-clock epoch (ms) when `lastRateLimitInfo` was last updated; 0 if never. */ | ||
| private lastRateLimitObservedAt = 0; |
There was a problem hiding this comment.
These observations are stored globally on the extension without identifying the credential that produced them, and neither space.github.setToken nor space.github.clearToken resets them. After replacing a PAT with one for another account—or clearing a keychain token and falling back to unauthenticated or env credentials—the refreshed panel continues showing the previous credential's remaining/reset budget, potentially including an obsolete exhausted quota, until a later poll returns finite headers; clear or re-key this state when the effective credential changes.
Useful? React with 👍 / 👎.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
2 similar comments
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
2ab872e to
9dd7a61
Compare
1 participant
Adds a Space-level GitHub integration health view so operators can verify the event subsystem in one place.
space.github.healthRPC aggregates token availability, polling interval/last poll, current rate-limit state (the extension now retains the last rate-limit observation), webhook registration summary, reaction-polling freshness, recent failed deliveries, and a per-repo rollup.GitHubHealthPanelin the Space external-events settings renders the snapshot with a Healthy/Degraded/Down badge plus Poll now (test event delivery) and Re-register webhooks actions.