integrate GitHub App with GitHub Tickets - #105
Conversation
Pxx500
left a comment
There was a problem hiding this comment.
Review: e652adaf — GitHub App auth and transport
Solid foundation. Token coalesce under lock, one-shot 401 refresh, assignee silent-ignore detection, and error strings that avoid leaking upstream bodies are all in good shape. The unit tests cover the important paths well (JWT claims, refresh, rate limit, concurrent refresh).
Please fix before building more on this
PullRequestSnapshot.node_id— stores RESTid(int), not GraphQLnode_id. Rename or read the realnode_idstring (inline).
Suggestions
- Request timeouts —
_sendshould not rely on an unbounded session (inline). - Rate-limit header casing —
dict(response.headers)makes lookups case-sensitive (inline). webhook_secret/app_id— carried on credentials but unused here. Fine for an incremental commit; just make sure webhook HMAC verification lands before any public endpoint accepts deliveries.
CI
quality was still in progress at review time — please confirm it goes green.
(Submitted as COMMENT because GitHub blocks REQUEST_CHANGES on your own PR; treat item 1 as a blocker before stacking more commits on this snapshot type.)
Pxx500
left a comment
There was a problem hiding this comment.
Review: aaa4be8 — harden GitHub App transport failures
The datetime.UTC → timezone.utc swap is the right Python 3.10 fix, and _integer correctly rejects bool (a subclass of int). The test class annotations and dict[str, Any] for fake-session kwargs are fine.
Blocker: quality is still red on this SHA
quality run failed. Mypy is continue-on-error, so the hard failure is almost certainly Unit tests, not Ruff.
github_app.py does import jwt at module level, and tests/test_github_app_client.py also imports cryptography. isolated_githubtickets_modules now loads github_app for every GitHub Tickets test. CI still installs only:
ruff mypy Pillow pillow-avif-plugin matplotlib aiohttp==3.9.5 pytest pytest-xdist
There is no PyJWT or cryptography on that list (they are in info.json for the cog, not for the workflow). Collection/setup will ModuleNotFoundError unless those packages are added to the quality job install step.
The UTC change was necessary and not sufficient. Please add the two deps to .github/workflows/ci.yml and keep this red until that run is green.
Still open from e652ada (no new inlines)
PullRequestSnapshot.node_idstill stores RESTid(now via_integer). Rename or read GraphQLnode_id._sendstill has no timeout.dict(response.headers)still makes rate-limit lookups case-sensitive.
Treat item 1 as a blocker before stacking more snapshot consumers. GitHub still will not let this account REQUEST_CHANGES on its own PR.
Pxx500
left a comment
There was a problem hiding this comment.
Review: 84e5e2c — address GitHub transport review findings
The three items from e652ada are fixed here:
PullRequestSnapshot.node_id→pull_request_id— the field now matches REST numericidinstead of pretending to be a GraphQLnode_id. Constructor and tests follow.- Request timeouts —
_REQUEST_TIMEOUT(30s total / 10s connect) is passed on every_send, including installation-token fetch. - Rate-limit header casing — headers are casefolded when copied, and lookups use
retry-after/x-ratelimit-reset.
Those three inline threads are resolved.
Still blocking: quality is red on this SHA
quality failed again. .github/workflows/ci.yml still installs only:
ruff mypy Pillow pillow-avif-plugin matplotlib aiohttp==3.9.5 pytest pytest-xdist
github_app.py imports jwt at module level, and the tests import cryptography. Putting those packages in info.json only helps Red's cog installer, not this workflow. Until PyJWT and cryptography are on the quality job install line, collection will keep dying when isolated_githubtickets_modules loads github_app.
Minor
The rate-limit test now sends already-lowercase retry-after, so it no longer proves mixed-case GitHub headers survive. One Retry-After / X-RateLimit-Reset case would lock the casefold path.
GitHub will not let this account REQUEST_CHANGES on its own PR. Treat the CI deps as a merge blocker.
Pxx500
left a comment
There was a problem hiding this comment.
Review: 01a081f — install GitHub App dependencies in CI
This is exactly the fix the previous review asked for. The quality job install line now includes PyJWT and cryptography next to the rest of the test stack, so github_app (and the cryptography-backed tests) can import during collection.
quality on this head completed successfully (run). No further issues on this one-line change.
Still open (unchanged from earlier, not re-reviewed)
The rate-limit unit test still feeds already-lowercase retry-after, so it does not exercise the casefold path against mixed-case GitHub headers. Optional follow-up only.
Pxx500
left a comment
There was a problem hiding this comment.
Review: efd91854 / 80f5ffbd — durable GitHub persistence
This is the real store layer for GitHub work: origin-aware tickets, last-write-wins PR snapshots with immutable identity, a delivery inbox, an outbox that survives ticket/guild delete, and claim/unassign inserting outbox rows in the same IMMEDIATE transaction. Tests cover migration, identity immutability, stale snapshots, retention, cleanup, and the outbox rollback trigger. 80f5ffbd correctly expects author_id is None after privacy redact (the store now NULLs it, not 0).
quality on this head completed successfully (run).
Please fix
_upsert_pull_requestalways writeslast_processed_action, includingNone(the dataclass default). A later observe that only refreshes title/labels/openwithout an action will wipe a stored action. Only update the column when the incoming value is notNone.
Suggestions
idx_github_pull_requests_active_identityon(repository_id, pr_number) WHERE current_ticket_id IS NOT NULLis redundant with the primary key. The useful unique index isidx_github_pull_requests_active_ticketoncurrent_ticket_id.- Leave
_create_schemaas v1. Fresh DBs run v1 then rebuild in migration 2. Foldinggithub_*into_create_schemawould make that migration fail on new installs. github_updated_atuses<=at second precision, so two observes with the same timestamp last-applied win. Delivery inbox order may disagree. Fine if the worker is last-write-wins; otherwise compare strictly<and keep the first.accept_deliveryraisesValueErrorover 1 MiB. GitHub webhook payloads can be larger. The HTTP layer should treat that as ignored/failed and ack, not 5xx, or GitHub will keep retrying.
GitHub will not let this account REQUEST_CHANGES on its own PR; please still treat item 1 as a real fix.
Pxx500
left a comment
There was a problem hiding this comment.
Review: 8182c68 / 1355538 — preserve action + durable webhooks
8182c68 fixes the two items from the last pass: _upsert_pull_request now COALESCEs last_processed_action so a title-only observe cannot wipe a stored action, and the redundant idx_github_pull_requests_active_identity is gone. The new test matches that.
1355538 is a solid receive path: HMAC on the raw bytes before parse, 202 only after accept_delivery, duplicates keep the first payload, store failure is 503, oversized body is 413 (aligned with MAX_DELIVERY_BODY_BYTES, so GitHub will not retry). Ping without installation is handled. quality on this head completed successfully (run).
Please fix
except Exceptionturns store validation into 503.accept_deliveryraisesValueErrorwhenrepository_idandpr_numberare unpaired. Every signed event that has a repository and nopull_request(push,issues,check_run, …) hits that check, and GitHub will retry. CatchValueErroras 400, and for non-PR events either persist with both IDsNoneor 202-ignore them. 503 should stay for real store/IO failures only.
GitHub will not let this account REQUEST_CHANGES on its own PR; please still treat item 1 as a real fix.
Pxx500
left a comment
There was a problem hiding this comment.
Pulled the new process-wide OperationalErrors cog (88aa3aa). The shared entry points, correlation-key recovery, and loading OperationalErrors before the other subcogs are the right shape.
Please fix:
- Honeypot
_record_operational_failurewraps a freshRuntimeError, so the Discord traceback is the wrapper instead of the original failure. - NHMisc still reports once per guild. Combined with a single global error channel, that duplicates alerts.
nhcogs errors channel/maintainer clearskip the private-channel check thatsetuses.
GitHubTickets still only logs exceptions and does not call the new reporter yet. Existing per-guild error channel config is not migrated, so operators will need to run [p]nhcogs errors channel set after deploy.
| ) | ||
| return None | ||
| ): | ||
| return await report_operational_error( |
There was a problem hiding this comment.
This wrapper is fine for guild-scoped work. _report_operational_error_for_guilds still calls it once per guild. That matched the old per-guild channel. With one process-wide destination, a background failure now stores N rows and posts N alerts to the same channel.
Fan out only when the destination is per-guild, or report process-wide failures once.
Pxx500
left a comment
There was a problem hiding this comment.
Review: 4787a83 — accept non pull request webhook deliveries
This closes the two findings from 1355538:
- Non-PR events no longer hit unpaired identity.
_delivery_identitykeepsrepository_id/pr_numberonly for_PULL_REQUEST_EVENTS; everything else (includingcheck_run/push/issues) stores both asNone, soaccept_delivery's pairing rule is satisfied. Org/installation checks still run before the null-out, so a foreign repo on a non-PR event stays 403. ValueError→ 400, other store errors → 503. GitHub will not retry validation failures; real IO failures still come back as 5xx.
The new check_run test locks the happy path. quality is green on this head (run). Resolving the two related inline threads.
Pxx500
left a comment
There was a problem hiding this comment.
Reviewed a4ba61d ("keep operational error configuration private").
Both nhcogs errors channel clear and nhcogs errors maintainer clear now call _require_private_channel before touching config, matching the set commands. The public-mutation test correctly asserts rejection before any read/write and that stored channel/maintainer values stay intact. That closes the earlier gap where Manage Messages from a public channel could disable alerting.
No new issues in this commit.
Pxx500
left a comment
There was a problem hiding this comment.
Review: 3505023 — process GitHub deliveries and assignee intents
Delivery loop, shielded store writes, close-order, dormant mode, the shared mutation lock, and per-PR outbox ordering are in good shape. The tests around retry_at, assignee-unavailable, stale reclaim, and guild-cleanup surviving on repository_full_name are the right ones.
Please fix
- Recovery redelivers deliveries you already have.
_recover_deliveriesonly skips when GitHub status is 2xx and the guid is local. A locally stored delivery whose original GitHub attempt is 5xx isredeliver()'d every cycle. GitHub redeliveries are new deliveries (newX-GitHub-Delivery); the webhook stores by guid withgithub_delivery_id=None, so the same event is processed again. The original GitHub row stays failed, so this repeats every 15 minutes until it burnsmax_redeliveries_per_recovery. Skip whenever the guid is already local. Redeliver only missing guids, and bounddelivered_atto delivery-identity retention so pruned rows do not look like gaps.
Suggestions
list_deliveriesis app-wide. A 403/foreign-install delivery stays failed on GitHub and will keep being redelivered if it is not skipped locally.- This worker is not started from
GitHubTicketsyet. Fine for an incremental commit; it still needs wiring before it can run.
GitHub will not let this account REQUEST_CHANGES on its own PR; please still treat item 1 as a real fix.
Pxx500
left a comment
There was a problem hiding this comment.
quality: unit tests failed on 3505023
quality failed. Install, Ruff, and Mypy succeeded; the red step is Unit tests.
The new helper in tests/test_github_integration_runtime.py is not valid on the quality job's Python 3.10:
async def _wait_until(predicate, *, timeout: float = 1.0) -> None:
async with asyncio.timeout(timeout):asyncio.timeout exists only from 3.11. On 3.10 this raises AttributeError: module 'asyncio' has no attribute 'timeout'. Ten tests in that file call _wait_until, so the pytest step stays red until the helper is 3.10-safe (asyncio.wait_for, or a deadline loop with asyncio.sleep).
Production runtime.py does not use asyncio.timeout. This is a test-only mismatch with .github/workflows/ci.yml (python-version: "3.10").
Pxx500
left a comment
There was a problem hiding this comment.
Reviewed c79aa39 (keep runtime tests compatible with Python 3.10).
_wait_until now wraps the poll loop in asyncio.wait_for instead of asyncio.timeout. That is the right fix for the Python 3.10 quality job: wait_for has been available for a long time, and on expiry it still raises TimeoutError, which these tests already treat as failure. No other issues in this commit.
Pxx500
left a comment
There was a problem hiding this comment.
Reviewed 812837 (complete GitHub ticket lifecycle controls) and af7cf77 (tighten GitHub integration types).
Looks right:
- Pasted-link org gate now runs before defer/fetch.
enabledwith missing tokens raises instead of returning silently; enable distinguishes invalid vs missing credentials; overview shows Invalid / Not configured / Available.author_id is Noneno longer builds{None}into automatic-candidate exclusions.- Draft Keep swaps to remove-only; GitHub close logging is best-effort.
Two issues on the new lifecycle path (inlines): the category ping is inside the creation rollback, and adding categories to an already-claimed ticket still starts automatic reviewer routing.
Quality is green on this head.
Pxx500
left a comment
There was a problem hiding this comment.
Review: clarify process-wide GitHub configuration (2f2874f)
README now states that github group settings, the receiver, and the selected guild are process-wide, and that enable in a guild picks that guild for the bot process. That matches the prior note about guild-only Discord commands writing global config.
Resolved the outdated README thread. No new code issues in this commit.
Pxx500
left a comment
There was a problem hiding this comment.
Reviewed 8d2edee ("disclose GitHub integration data"): package-level NHCogs/info.json now matches the expanded GitHubTickets privacy statement.
One inline on disclosure accuracy: failed deliveries still keep raw_body until the 3-day prune (not cleared on fail), and Red user-data deletion does not remove delivery GUID / unbound PR sync rows the way the closing sentence implies.
Pxx500
left a comment
There was a problem hiding this comment.
quality: unit tests failed on 8d2edee
quality failed. Ruff succeeded; Mypy is continue-on-error, so the hard failure is Unit tests.
tests/test_nhcogs_suite.py test_combined_metadata_preserves_both_data_contracts still asserts:
self.assertIn("GitHubTickets stores guild and user IDs", statement)NHCogs/info.json now says GitHubTickets stores Discord guild and user IDs, so that substring no longer matches. Update the assertion to the expanded wording (the cog-level githubtickets/info.json already uses the Discord-prefixed sentence).
Pxx500
left a comment
There was a problem hiding this comment.
Reviewed e14136f (synchronize Discord ticket actions with GitHub).
Moving the GitHub category prompt before activate_ticket is the right response to the earlier rollback note: failure stays in CREATING, cleanup can retry, and the new tests cover prompt failure plus a later successful create. Resolving that thread.
Discord claim/unassign now enqueue ADD/REMOVE assignee when the ticket is bound and the actor has a unique GitHub mapping. Unbound, unmapped, and ambiguous mappings staying local looks intentional. Decline staying local also matches the new test.
One desync: unassign re-checks uniqueness at unassign time. If claim already wrote ADD_ASSIGNEE and the mapping later becomes missing or ambiguous, REMOVE_ASSIGNEE is skipped and GitHub keeps the assignee. Reuse the login from the claim outbox (or persist it on the ticket) instead of requiring uniqueness again.
Pxx500
left a comment
There was a problem hiding this comment.
Reviewed 4eb869f (harden GitHub delivery recovery) and edb9fd7 (align aggregate data contract test).
The data-contract assertion now matches info.json. Storing assignees on the PR observation and rewriting assigned/unassigned from the authoritative list is the right direction for same-second reorder. Token-update restart and the recovery rate-limit pause look good.
Two recovery problems and the lifecycle stop path still need a fix:
-
runtime.py_recover_delivery_page:should_redeliveris true whenever GitHub's original delivery row is non-2xx (failed_summary), including when the local guid is already PROCESSED or IGNORED. GitHub does not update the original delivery after a redelivery; the original stays failed and the new attempt is a new guid. This will redeliver the same original row every recovery cycle until it burnsmax_redeliveries_per_recovery. Redeliver missing guids and local FAILED/RETRY. Do not redeliver because the original summary is still non-2xx. -
runtime.py_recover_deliveries: GitHub lists app hook deliveries newest-first. Checkpointingnext_page=page+1then starting there on the next cycle skips new misses that landed on page 1. Pastdelivered_at < identity_cutoff, later pages are also expired; continuing the scan delays the reset to page 1. Treat cutoff (or a short page) as end-of-scan and reset the checkpoint. -
event_handler.py: everyGitHubAppLifecycleEventreturns STOPPED, andrepository_idsis unused.installation.suspend/deletedtaking the runtime down is reasonable.new_permissions_acceptedandrepositories_removedshould not. After STOPPED the receiver is closed, sounsuspendnever arrives; the only restart is a token update or cog reload.
Nit: _pull_request_state stores assignees as a tuple, labels as a frozenset, so GitHub reordering assignees looks like a real change.
Pxx500
left a comment
There was a problem hiding this comment.
Review: 8bcf681 — complete GitHub delivery recovery
The PROCESSED/IGNORED redelivery loop from the last pass is gone: should_redeliver no longer follows GitHub's original non-2xx row, RETRY stays on the local worker, and test_recovery_redelivers_failed_summaries_and_local_failures now expects [1, 5]. STOPPED now actually tears the owner down (session, client, enabled=False) and survives a receiver close() failure. Those tests look right.
Quality is still running on this head — not commenting on an in-progress check.
Please fix
- Missing /
AWAITING_REDELIVERYstill redeliver the original GitHub row every cycle. GitHub does not update the original delivery afterredeliver(); the new attempt is a new guid. A missing guid records nothing, so it stays missing. Local FAILED only holdsAWAITING_REDELIVERYforretry_base(30s), then the next recovery redelivers the same original id. Persist a stub (or keep awaiting until identity prune / a redelivery row shows up) instead of using the short local retry delay. See inline. enabled=Falseon every STOPPED delivery. The new callback is the right teardown forinstallation.suspend/deleted. The handler still returns STOPPED fornew_permissions_acceptedandrepositories_removedtoo, so those now persist disabled: token-update cannot restart, andunsuspendnever arrives. Only disable for suspend/deleted. See inline.
Still open (unchanged in this commit)
_recover_deliveriesstill checkpointsnext_page=page+1on a newest-first list, so the next cycle skips fresh misses on page 1. Reset at identity cutoff (or a short page), not only whenlisted_count < 100.- Earlier notes not touched here (unassign uniqueness, empty title, STALE action ACK, claimed-category ping, disclosure wording, Manage Messages binding a socket, NHMisc duplicate alerts, Honeypot wrapper) are left as-is.
GitHub will not let this account REQUEST_CHANGES on its own PR; please still treat items 1 and 2 as real fixes.
Pxx500
left a comment
There was a problem hiding this comment.
Inlines for the missing-guid redelivery loop and lifecycle disable.
| self._github_client = None | ||
| self._github_organization = None | ||
| try: | ||
| await self.config.set_raw("enabled", value=False) |
There was a problem hiding this comment.
Persisting enabled=False is the right teardown for installation.suspend / deleted: the receiver is already closed, so unsuspend will never arrive, and a moderator should re-enable.
event_handler.py still returns STOPPED for every GitHubAppLifecycleEvent, including new_permissions_accepted and repositories_removed. Those are not fatal. After this callback they now stay disabled until github enable; token-update cannot restart because _restart_github_integration bails when enabled is false.
Only call this (or only return STOPPED) for suspend/deleted. Treat permissions and repo-removal as PROCESSED — and use repository_ids if local bindings for removed repos need to drop.
| if local_delivery is None: | ||
| return True, None | ||
| prepared_body = local_delivery.raw_body | ||
| now = self._clock() | ||
| try: | ||
| prepared = await self._await_store( | ||
| self._store.prepare_delivery_redelivery( | ||
| delivery.guid, | ||
| github_delivery_id=delivery.delivery_id, | ||
| now=now, | ||
| next_attempt_at=now + self._retry_base, |
There was a problem hiding this comment.
GitHub does not update the original delivery after redeliver(); the new attempt is a new guid (and the original stays non-2xx).
This branch records nothing for a missing guid, so the next recovery cycle still sees it as missing and redelivers the same original id. For local FAILED, prepare_delivery_redelivery only holds AWAITING_REDELIVERY until now + retry_base (30s). The 15-minute recovery loop then redelivers that original row again until the 7-day prune.
Same-guid accept_delivery cannot save this unless GitHub reuses the guid, which the deliveries API does not.
Insert an AWAITING_REDELIVERY stub for missing guids too, and keep that state until identity retention (or until a redelivery=true row for this event is observed). Do not use the short local retry delay as the redelivery lock.
Pxx500
left a comment
There was a problem hiding this comment.
Reviewed a7ff812 (coalesce overlapping GitHub recovery).
The dirty-bit + lock pattern looks correct under asyncio’s cooperative scheduling: overlapping run_recovery always arms _recovery_requested, then either joins the while-loop or returns while the lock holder runs a follow-up pass after clear(). That fixes the old if locked: return path, which dropped concurrent callers without arming a retry. request_recovery during an in-flight pass still coalesces the same way. The new overlap test matches that contract, and quality is green on this head.
One subtlety: when the early locked() return fires, await run_recovery() resolves before the follow-up pass that drains the dirty bit. Fine for the recovery loop and fire-and-forget callers (and the test correctly awaits the in-flight task). A future caller that needs “recovery fully drained my request” should not treat that await as the drain signal.
Pxx500
left a comment
There was a problem hiding this comment.
Review: 8432609 — fix review findings for GitHub integration
Closes several of the open notes.
Looks good
- STALE observations no longer ACK without a domain transition. Assignment, label, review, and open/closed tests run against the stored snapshot via
_reconciled_action. - Discord unassign now reuses the latest
ADD_ASSIGNEElogin, so profile deletion / a later ambiguous mapping still enqueueREMOVE_ASSIGNEE. - Adding categories on an already-CLAIMED ticket no longer schedules
automatic_ping(and does not wake deadlines). - Disclosure now matches failed-body retention and
redact_user(deliveries / unbound PR rows stay). - Honeypot operational alerts pass the original exception through; the new registry test checks identity and traceback.
Please fix
- Empty title ACKs every event. The old note was only about
update_title_from_githubraising ontitle.strip() == "". The new gate in__call__returnsPROCESSEDfor any delivery whose stored title is whitespace, includinglabeled/assigned/closed/ reviews. Combined with_upsert_pull_requestno longer requiring a title, a whitespace title is persisted, then later actions on that PR are dropped until a non-empty title lands — andeditedstill will not create a missed ticket. Guard empty titles only on the title-update path (or treat them as a no-op there). Keep lifecycle actions. See inline.
Still open (unchanged in this commit)
claim_ticket_from_githubstill returns success for anyCLAIMEDticket, not only matchingassignee_id.- Every
GitHubAppLifecycleEventis stillSTOPPED(new_permissions_accepted/repositories_removedpersist disabled). - Missing-guid /
AWAITING_REDELIVERYstill redeliver the original GitHub row every recovery cycle. github setstill binds a socket with only Manage Messages.- NHMisc still fans out one process-wide operational alert per guild.
GitHub will not let this account REQUEST_CHANGES on its own PR; please still treat item 1 as a real fix.
Pxx500
left a comment
There was a problem hiding this comment.
Review: 82a2f50 — keep lifecycle events independent of title updates
This is the right split after the previous finding.
- The handler no longer ACKs every delivery just because the observed title is whitespace.
closedstill finishes (new test), and other lifecycle actions can run. update_title_from_githubnow returns a settled no-op afterstrip(), soValueError("ticket title is required")no longer escapes onedited. Coordinator test covers that path.- Quality is green on this head.
Resolving the two empty-title threads this commit addresses.
Leftover on a path this commit does not change: _insert_ticket still rejects empty titles. A discord-ticket label on a whitespace-only PR therefore still fails create (CREATE_FAILED) and defers. If that delivery should settle, give create a fallback title or treat empty-title labeled as a no-op — do not put the gate back in __call__.
Pxx500
left a comment
There was a problem hiding this comment.
Overall review (full PR vs master, head 9fdce83)
Architecture is in good shape: App auth/transport, durable webhook accept, observation ordering with conflict refresh, outbox assignee sync, recovery coalesce, link-based /newticket, OperationalErrors extraction, and secret redaction all hang together. Empty-title labeled / ready_for_review is now a settled no-op (good). _require_settled correctly defers failed transitions. Claimed-category routing no longer schedules a dead automatic ping.
Still open (inlines)
claim_ticket_from_github— anyCLAIMEDticket returns success; different assignee never transfers.- Category prompt — still inside creation rollback; a Discord blip on the prompt deletes a labeled ticket.
- Lifecycle STOPPED —
permissions_changed/repositories_removedpermanently disable viaenabled=False. - Recovery missing-guid — redelivers the original delivery every cycle without a retention-scoped stub.
github receiver set/ enable — Manage Messages can bind a host port.
Also still open (not inlineable on this diff)
- NHMisc
_report_operational_error_for_guilds— still fans out once per guild into the process-wide OperationalErrors channel (N duplicate alerts).
Quality was still running at review time; ignored while in progress.
Once those settle, the integration looks ready for a quiet soak.
| ticket = await self._store.get_ticket(ticket_id) | ||
| if ticket is None or ticket.state is not TicketState.OPEN: | ||
| if ticket is not None and ticket.state is TicketState.CLAIMED: | ||
| return TicketResult(True) |
There was a problem hiding this comment.
Any CLAIMED ticket returns success here, not only when ticket.assignee_id == user_id.
Same-assignee replays settle correctly. A different assignee (second GitHub assignee, or assigned / review claim while Discord already has someone else) also ACKs and never transfers the ticket. _handle_unassigned can recover when GitHub sends unassign+assign, but a lone assigned / review claim leaves Discord stale.
Only treat same user_id as settled success. For a different assignee, either transfer (unassign then claim) or return failure so the delivery defers until the unassign path runs.
| if parsed is None: | ||
| return DeliveryDisposition.IGNORED | ||
| if isinstance(parsed, events.GitHubAppLifecycleEvent): | ||
| return DeliveryDisposition.STOPPED |
There was a problem hiding this comment.
Every GitHubAppLifecycleEvent returns STOPPED, and _github_lifecycle_stopped always persists enabled=False.
That is correct for installation.suspend / deleted: the receiver is closed and a moderator should re-enable. It is wrong for new_permissions_accepted and repositories_removed. Those are not fatal, but after this callback the integration stays disabled until github enable, and token-update cannot restart because _restart_github_integration bails when enabled is false.
Only return STOPPED (or only persist disable) for suspend/deleted. Treat permissions and repo-removal as PROCESSED, and use repository_ids if local bindings for removed repos need to drop.
| if not should_redeliver or not redelivery_available: | ||
| return should_redeliver, None | ||
| if local_delivery is None: | ||
| return True, None |
There was a problem hiding this comment.
GitHub does not update the original delivery after redeliver(); the new attempt is a new guid (and the original stays non-2xx).
For a missing guid this returns True without inserting an AWAITING_REDELIVERY stub, so the next recovery cycle still sees it as missing and redelivers the same original id. For local FAILED, prepare_delivery_redelivery only holds AWAITING_REDELIVERY until now + retry_base (30s). The 15-minute recovery loop then redelivers that original row again until the 7-day prune.
Insert an AWAITING_REDELIVERY stub for missing guids too, and keep that state until identity retention (or until a redelivery=true row for this event is observed). Do not use the short local retry delay as the redelivery lock.
| await self._send_group_overview(ctx) | ||
|
|
||
| @githubtickets_github_receiver.command(name="set") | ||
| async def githubtickets_github_receiver_set( |
There was a problem hiding this comment.
This opens a listening socket on the bot host (bind_host/bind_port), including 0.0.0.0.
The cog check (and parent group) only requires Manage Messages — channel moderation, not host/network admin. Anyone who can delete messages can ask the bot process to bind an arbitrary port (and github enable starts that listener).
Tighten receiver set / github enable to administrator or bot owner, or keep Manage Messages for channel/role config and put the network-facing github subgroup behind a stricter check.
| self._github_client = None | ||
| self._github_organization = None | ||
| try: | ||
| await self.config.set_raw("enabled", value=False) |
There was a problem hiding this comment.
Companion to the event-handler STOPPED note: this always persists enabled=False for every lifecycle stop. Only do that for suspend/deleted.
Pxx500
left a comment
There was a problem hiding this comment.
Reviewed 362eb49 ("retry GitHub category prompts after activation").
The create path now activates first, then best-effort prompts with durable category_prompt_retry_at + process_due retries. That fixes rolling back a labeled ticket when Discord blips on the category ping. Schema v3 migration and the updated tests look right.
Two follow-ups on the new retry path:
_recover_creationactivates GitHub tickets (which setscategory_prompt_retry_at) but neither sends the prompt nor wakes deadlines whennext_action_atisNone(normal forRoutingMode.NONE).- A failed prompt in
process_duereturns early and can starve projection sync / routing on the same ticket while retries keep failing.
Resolving the outdated create-path rollback thread this commit addresses.
Pxx500
left a comment
There was a problem hiding this comment.
The category-prompt recovery scheduling looks correct.
_recover_creation now reloads the activated ticket and calls _send_pending_category_prompt, matching the create path. GitHub-origin tickets still have next_action_at is None, but a failed prompt already calls _wake_deadlines() inside _send_pending_category_prompt after defer_category_prompt, so the 5s retry does not wait on an unrelated deadline.
process_due no longer returns after a failed prompt, so projection sync / routing on that ticket can still run. The two new tests cover recovery sending the prompt and a failed prompt not starving edit_ticket.
Resolving the two earlier threads on this path.
integrates a private GitHub App with GitHub Tickets so pull request events can create, synchronize, recover, and finish Discord tickets. adds durable webhook processing, GitHub assignee synchronization, link-only manual ticket creation, and shared operational error reporting