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".
|
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
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0df79cfea1
ℹ️ 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".
- Polling is live without a PAT for public repos (unauthenticated polling publishes); only a configured-but-rejected token (token.error) or inaccessible repos drop it out. Private repos without a token surface as inaccessible via the poll-access tracking. - Track partial polling cycles separately: a repo that reached some endpoints but not others (e.g. fine-grained PAT with issue-comment but no pull-request access) is a live path flagged Degraded, not cleared/Healthy. - Reaction freshness uses lastReactionPollAt (only advanced when a reaction request actually runs), not lastPollAt, so skipped reaction cycles do not over-state freshness. - Give each failed-delivery row a unique key (deliveryKey) for fan-out. - Bound /user token validation with a timeout so a stalled request cannot block the entire health snapshot. - Reset the cached rate-limit observation when the credential changes (setToken/clearToken).
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9ecc8b9a19
ℹ️ 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: Zhipu
Recommendation: REQUEST_CHANGES (posted as COMMENT only because this is a self-authored PR under the lsm account — GitHub rejects REQUEST_CHANGES/APPROVE on self-PRs.)
Verified against pushed state
9ecc8b9a1== remote tip, clean tree; PR OPEN / MERGEABLE / targetsdev.- Tests: 429 pass across the full
tests/unit/2-handlers/github/directory (13 health-RPC + the broader extension suite — zero regressions); 23 web-panel tests pass; lint/typecheck/knip green. - 5 of 6 fixes verified correct: (1) unauthenticated polling is genuinely live —
gitHubPollingHeadersomitsAuthorizationwhen no token andpollWatchedRepodoesn't gate on a token; (2) partial-error semantics are sound (accessible && pollErrorMessage → lastPartialPollError, cleared on a clean cycle); (3)lastReactionPollAtadvances only on an actual reaction request and is preserved otherwise; (4)deliveryKeyis a NOT-NULL UNIQUE column, always present on delivery rows; (5)AbortSignal.timeout(5s)surfaces astoken.error: 'validation timed out'with no double-handling.
🔴 P1 — resetRateLimitObservation() is incomplete: polling stays blocked after a credential swap during a rate-limit cooldown
github-event-extension.ts:1277 — the method clears lastRateLimitInfo + lastRateLimitObservedAt but not this.rateLimitedUntil / this.rateLimitedFromRetryAfter. Those are the fields that actually matter after a credential change:
- Polling stays blocked.
pollEnabledSpaces(L789) andpollSpace(L815) skip the entire cycle whilenow < rateLimitedUntil, andgetNextPollDelayMs(L921) reschedules past it. An operator who hits a rate limit and swaps in a fresh PAT expecting polling to resume will see it silently skipped until the old credential's cooldown expires — up to ~1h for a primaryX-RateLimit-Resetwindow. - Snapshot is internally inconsistent.
buildHealthSnapshot(L1150) deriveslimited: now < rateLimitedUntil, so right after the swap the panel still reportslimited: true/until: <future>whileremainingandresetAtare now (correctly)null— i.e. "rate limited, but we have no budget info." - This undercuts the commit's own stated goal (#6): "the observed remaining/reset budget belongs to the previous credential." The cooldown is per-credential for primary limits too.
Fix (2 lines):
private resetRateLimitObservation(): void {
this.lastRateLimitInfo = undefined;
this.lastRateLimitObservedAt = 0;
this.rateLimitedUntil = 0;
this.rateLimitedFromRetryAfter = false;
}Extend the existing test to assert rateLimitedUntil === 0 after reset — ideally driving it through the clearToken/setToken RPC (the current test calls the private method directly, so it would not catch a revert of the call sites).
One nuance for the author: secondary (abuse) rate limits are per-IP, not per-token, so clearing the cooldown on credential change also clears a secondary-limit backoff. If that's undesired, keep rateLimitedUntil — but then rateLimit.limited in the snapshot must not report true from a stale cooldown. Either way, the current half-reset is the bug.
Advisory coverage notes (not blocking)
- #5 timeout branch untested: the
AbortSignal.timeout → 'validation timed out'path is never exercised (test fetch resolves immediately). Low risk; a stalled-/usertest would lock it in. - #6 integration unguarded: nothing drives
clearToken/setTokenthen observes a reset snapshot. Folding that into the P1 test above closes both gaps.
The only change I'm asking for is the P1. Once resetRateLimitObservation also clears the cooldown (plus its test), I'm ready to approve.
P1: resetRateLimitObservation() now also clears rateLimitedUntil and rateLimitedFromRetryAfter, so a swapped PAT is no longer silently skipped under the previous credential's cooldown (and the panel stops reporting limited:true with null remaining). A per-IP secondary limit is re-detected by the next poll. Test extended to assert the cooldown clears, driven via the setToken RPC. P2 triage: - Disable Poll now during an active rate-limit cooldown (server skips it). - Surface a credential-store read failure in the token metric. - Lock the health panel during parent repo mutations (disabled || busy). - Refresh the health snapshot after sibling settings changes (refreshNonce). - Persist network-thrown poll failures on the cursor instead of aborting before the access/error signal is committed.
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: Zhipu
Recommendation: APPROVE (posted as COMMENT only because this is a self-authored PR under the lsm account — GitHub rejects APPROVE/REQUEST_CHANGES on self-PRs.)
Round-12 P1 — resolved ✅
resetRateLimitObservation() (github-event-extension.ts:1277) now also clears this.rateLimitedUntil = 0 and this.rateLimitedFromRetryAfter = false. The new "clearToken RPC clears the active cooldown…" test drives it through the actual space.github.setToken RPC (not the private method), seeds a 1h cooldown, and asserts both rateLimitedUntil === 0 and the subsequent snapshot's rateLimit.limited === false / until === 0. That closes the bug and the call-site coverage gap I flagged. The secondary-limit note is the right framing — clearing is safe because a still-active per-IP secondary limit is re-detected by the next poll.
Verified
0c1cf0d12== remote tip, clean tree; PR OPEN / MERGEABLE / targetsdev; all 44 review threads resolved (0 unresolved).- 431 tests pass across
tests/unit/2-handlers/github/(+2 this commit; zero regressions), 23 web-panel tests pass; CI fully green (lint/knip/format/typecheck, all daemon unit + online shards, web tests, coverage gate). - The five P2s are clean, minimal, and correct: Poll now disabled during an active cooldown; credential-store read failure surfaced under "Not configured"; panel locked during parent mutations;
refreshNoncere-fetch after sibling settings changes; per-endpoint network-thrown fetch failures recorded on the cursor instead of aborting the whole cycle.
Two non-blocking notes (not asking for changes)
GitHubHealthPanel.tsx:256— therefreshNonceeffect'sif (refreshNonce === undefined) returnguard never fires (the parent initializeshealthNonceto0), so on first mount this effect runs alongside thespaceIdeffect → one redundantspace.github.healthRPC. Harmless (idempotent); the "skipped on first render" comment just doesn't match reality. AuseReffirst-run flag would realize the intent, but this is cosmetic.github-event-extension.ts:2225— the new network-errortry/catchwraps the main endpoint fetch only; the reaction and check-run fetches are still unguarded, so a transient network throw in those loops still aborts before the cursor commit. Narrow (they run only after the main endpoints succeed) and self-heals next cycle, so not blocking.
Ready to approve. Holding at the human merge gate (squash → dev, no --delete-branch).
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0c1cf0d12f
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 715148a8c4
ℹ️ 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".
…ening Daemon: - Guard check-run and reaction fetches (and the thrown-/pulls path) so a network failure mid-cycle records a partial error instead of aborting before the cursor commits; a thrown /pulls now sets partialScan. - Credential generation: a poll cycle captures the generation at start and discards its own applyRateLimit/lastRateLimitInfo writes if the credential changed mid-flight (setToken/clearToken bumped it), so a replaced PAT is not blocked by the old credential's quota/cooldown. - Re-arm the scheduled poll timer at the normal interval when a credential reset clears a rate-limit cooldown. - Serialize Poll now with the scheduled cycle (runExclusivePoll + timer overlap guard) so concurrent polls cannot interleave cursor reads/writes. - clearWebhookRegistration now clears the stale delivery timestamp so a rotated/disabled hook's history does not revive a false live path. Frontend: - Disable Poll now when the space has no polling repositories. - Request-generation guard so an older same-space refresh cannot overwrite a newer snapshot. - Apply rate-limit degradation only to spaces that use the polling path. - Do not revive a remotely-confirmed-inactive hook from delivery history. - Treat a polling path with an ancient lastPollAt (stalled cycle) as not live.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 962a2a6c19
ℹ️ 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".
| if (this.activePollCycle) { | ||
| try { | ||
| await this.activePollCycle; | ||
| } catch { | ||
| // The queue must not leak a prior cycle's rejection. | ||
| } | ||
| } |
There was a problem hiding this comment.
When two manual RPCs arrive while another poll is active, both callers pass this single activePollCycle check and await the same promise; after it resolves, each independently installs a new cycle and starts fn(), so the polls overlap and the later assignment also hides the other cycle. Multiple clients can therefore race cursor reads and wholesale cursor writes despite the new serialization wrapper; use a permanently chained queue or re-check/acquire ownership after waiting.
Useful? React with 👍 / 👎.
| // A partial failure: some endpoints reached, a later required one failed. | ||
| // Only meaningful when accessible; null on a clean cycle or full failure. | ||
| lastPartialPollError: accessible ? (pollErrorMessage ?? null) : null, |
There was a problem hiding this comment.
When the primary endpoints succeed but a downstream check-run request returns a non-rate-limit 403, such as a fine-grained PAT without Checks read access, the branch sets checkRunPermissionDenied without populating pollErrorMessage. This assignment consequently clears lastPartialPollError, allowing a polling-only integration to remain Healthy while all check-run events are being suppressed; reaction HTTP failures have the same gap.
Useful? React with 👍 / 👎.
| const webhookLive = | ||
| snapshot.webhook.deliveryEnabled && | ||
| (snapshot.webhook.active > 0 || | ||
| (snapshot.webhook.unknown > 0 && snapshot.webhook.lastWebhookAt !== null)); |
There was a problem hiding this comment.
With one unchecked manual hook that has never delivered and a separate remotely confirmed inactive hook that retains historical lastWebhookAt, these independently aggregated fields satisfy this condition. The inactive hook's history therefore revives the unrelated unchecked hook and badges a webhook-only Space Degraded rather than Down even though neither row is a working path; the snapshot needs a count or timestamp for unchecked hooks that themselves delivered.
Useful? React with 👍 / 👎.
| const { lastPollAt, intervalMs } = snapshot.polling; | ||
| if (lastPollAt === null || intervalMs <= 0) return false; | ||
| const window = Math.max(intervalMs * POLLING_STALE_INTERVALS, POLLING_STALE_MIN_MS); | ||
| return Date.now() - lastPollAt > window; |
There was a problem hiding this comment.
When the browser clock differs from the daemon clock, this comparison mixes the client-side Date.now() with the server-generated lastPollAt. A client more than the freshness window ahead can badge a freshly polling integration Down, while a client sufficiently behind can keep a stalled path Healthy; use the snapshot's server-generated timestamp when calculating staleness.
Useful? React with 👍 / 👎.
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.