Skip to content

integrate GitHub App with GitHub Tickets - #105

Open
Pxx500 wants to merge 46 commits into
masterfrom
github-app-integration
Open

integrate GitHub App with GitHub Tickets#105
Pxx500 wants to merge 46 commits into
masterfrom
github-app-integration

Conversation

@Pxx500

@Pxx500 Pxx500 commented Aug 28, 2026

Copy link
Copy Markdown
Owner

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

@Pxx500 Pxx500 left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test review.

@Pxx500 Pxx500 left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

  1. PullRequestSnapshot.node_id — stores REST id (int), not GraphQL node_id. Rename or read the real node_id string (inline).

Suggestions

  1. Request timeouts_send should not rely on an unbounded session (inline).
  2. Rate-limit header casingdict(response.headers) makes lookups case-sensitive (inline).
  3. 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.)

Comment thread NHCogs/githubtickets/github_app.py
Comment thread NHCogs/githubtickets/github_app.py
Comment thread NHCogs/githubtickets/github_app.py

@Pxx500 Pxx500 left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review: aaa4be8 — harden GitHub App transport failures

The datetime.UTCtimezone.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)

  1. PullRequestSnapshot.node_id still stores REST id (now via _integer). Rename or read GraphQL node_id.
  2. _send still has no timeout.
  3. 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 Pxx500 left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review: 84e5e2c — address GitHub transport review findings

The three items from e652ada are fixed here:

  1. PullRequestSnapshot.node_idpull_request_id — the field now matches REST numeric id instead of pretending to be a GraphQL node_id. Constructor and tests follow.
  2. Request timeouts_REQUEST_TIMEOUT (30s total / 10s connect) is passed on every _send, including installation-token fetch.
  3. 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 Pxx500 left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Pxx500 left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

  1. _upsert_pull_request always writes last_processed_action, including None (the dataclass default). A later observe that only refreshes title/labels/open without an action will wipe a stored action. Only update the column when the incoming value is not None.

Suggestions

  1. idx_github_pull_requests_active_identity on (repository_id, pr_number) WHERE current_ticket_id IS NOT NULL is redundant with the primary key. The useful unique index is idx_github_pull_requests_active_ticket on current_ticket_id.
  2. Leave _create_schema as v1. Fresh DBs run v1 then rebuild in migration 2. Folding github_* into _create_schema would make that migration fail on new installs.
  3. github_updated_at uses <= 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.
  4. accept_delivery raises ValueError over 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.

Comment thread NHCogs/githubtickets/store.py Outdated

@Pxx500 Pxx500 left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

  1. except Exception turns store validation into 503. accept_delivery raises ValueError when repository_id and pr_number are unpaired. Every signed event that has a repository and no pull_request (push, issues, check_run, …) hits that check, and GitHub will retry. Catch ValueError as 400, and for non-PR events either persist with both IDs None or 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.

Comment thread NHCogs/githubtickets/webhook.py Outdated
Comment thread NHCogs/githubtickets/webhook.py Outdated

@Pxx500 Pxx500 left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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_failure wraps a fresh RuntimeError, 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 clear skip the private-channel check that set uses.

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.

Comment thread NHCogs/honeypot/honeypot.py Outdated
Comment thread NHCogs/operationalerrors/cog.py
Comment thread NHCogs/nhmisc/nhmisc.py
)
return None
):
return await report_operational_error(

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Pxx500 left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review: 4787a83 — accept non pull request webhook deliveries

This closes the two findings from 1355538:

  1. Non-PR events no longer hit unpaired identity. _delivery_identity keeps repository_id/pr_number only for _PULL_REQUEST_EVENTS; everything else (including check_run / push / issues) stores both as None, so accept_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.
  2. 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 Pxx500 left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Pxx500 left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

  1. Recovery redelivers deliveries you already have. _recover_deliveries only skips when GitHub status is 2xx and the guid is local. A locally stored delivery whose original GitHub attempt is 5xx is redeliver()'d every cycle. GitHub redeliveries are new deliveries (new X-GitHub-Delivery); the webhook stores by guid with github_delivery_id=None, so the same event is processed again. The original GitHub row stays failed, so this repeats every 15 minutes until it burns max_redeliveries_per_recovery. Skip whenever the guid is already local. Redeliver only missing guids, and bound delivered_at to delivery-identity retention so pruned rows do not look like gaps.

Suggestions

  1. list_deliveries is app-wide. A 403/foreign-install delivery stays failed on GitHub and will keep being redelivered if it is not skipped locally.
  2. This worker is not started from GitHubTickets yet. 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.

Comment thread NHCogs/githubtickets/runtime.py Outdated
Comment thread tests/test_github_integration_runtime.py Outdated

@Pxx500 Pxx500 left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Pxx500 left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Pxx500 left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 812837 (complete GitHub ticket lifecycle controls) and af7cf77 (tighten GitHub integration types).

Looks right:

  • Pasted-link org gate now runs before defer/fetch.
  • enabled with missing tokens raises instead of returning silently; enable distinguishes invalid vs missing credentials; overview shows Invalid / Not configured / Available.
  • author_id is None no 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.

Comment thread NHCogs/githubtickets/coordinator.py
Comment thread NHCogs/githubtickets/coordinator.py

@Pxx500 Pxx500 left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Pxx500 left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread NHCogs/info.json Outdated

@Pxx500 Pxx500 left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Pxx500 left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread NHCogs/githubtickets/coordinator.py Outdated

@Pxx500 Pxx500 left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. runtime.py _recover_delivery_page: should_redeliver is 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 burns max_redeliveries_per_recovery. Redeliver missing guids and local FAILED/RETRY. Do not redeliver because the original summary is still non-2xx.

  2. runtime.py _recover_deliveries: GitHub lists app hook deliveries newest-first. Checkpointing next_page=page+1 then starting there on the next cycle skips new misses that landed on page 1. Past delivered_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.

  3. event_handler.py: every GitHubAppLifecycleEvent returns STOPPED, and repository_ids is unused. installation.suspend / deleted taking the runtime down is reasonable. new_permissions_accepted and repositories_removed should not. After STOPPED the receiver is closed, so unsuspend never 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 Pxx500 left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

  1. Missing / AWAITING_REDELIVERY still redeliver the original GitHub row every cycle. GitHub does not update the original delivery after redeliver(); the new attempt is a new guid. A missing guid records nothing, so it stays missing. Local FAILED only holds AWAITING_REDELIVERY for retry_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.
  2. enabled=False on every STOPPED delivery. The new callback is the right teardown for installation.suspend / deleted. The handler still returns STOPPED for new_permissions_accepted and repositories_removed too, so those now persist disabled: token-update cannot restart, and unsuspend never arrives. Only disable for suspend/deleted. See inline.

Still open (unchanged in this commit)

  • _recover_deliveries still checkpoints next_page=page+1 on a newest-first list, so the next cycle skips fresh misses on page 1. Reset at identity cutoff (or a short page), not only when listed_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 Pxx500 left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +314 to +324
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,

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Pxx500 left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Pxx500 left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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_ASSIGNEE login, so profile deletion / a later ambiguous mapping still enqueue REMOVE_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

  1. Empty title ACKs every event. The old note was only about update_title_from_github raising on title.strip() == "". The new gate in __call__ returns PROCESSED for any delivery whose stored title is whitespace, including labeled / assigned / closed / reviews. Combined with _upsert_pull_request no longer requiring a title, a whitespace title is persisted, then later actions on that PR are dropped until a non-empty title lands — and edited still 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_github still returns success for any CLAIMED ticket, not only matching assignee_id.
  • Every GitHubAppLifecycleEvent is still STOPPED (new_permissions_accepted / repositories_removed persist disabled).
  • Missing-guid / AWAITING_REDELIVERY still redeliver the original GitHub row every recovery cycle.
  • github set still 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.

Comment thread NHCogs/githubtickets/event_handler.py Outdated

@Pxx500 Pxx500 left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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. closed still finishes (new test), and other lifecycle actions can run.
  • update_title_from_github now returns a settled no-op after strip(), so ValueError("ticket title is required") no longer escapes on edited. 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 Pxx500 left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

  1. claim_ticket_from_github — any CLAIMED ticket returns success; different assignee never transfers.
  2. Category prompt — still inside creation rollback; a Discord blip on the prompt deletes a labeled ticket.
  3. Lifecycle STOPPEDpermissions_changed / repositories_removed permanently disable via enabled=False.
  4. Recovery missing-guid — redelivers the original delivery every cycle without a retention-scoped stub.
  5. github receiver set / enable — Manage Messages can bind a host port.

Also still open (not inlineable on this diff)

  1. 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)

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread NHCogs/githubtickets/coordinator.py Outdated
if parsed is None:
return DeliveryDisposition.IGNORED
if isinstance(parsed, events.GitHubAppLifecycleEvent):
return DeliveryDisposition.STOPPED

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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(

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Companion to the event-handler STOPPED note: this always persists enabled=False for every lifecycle stop. Only do that for suspend/deleted.

@Pxx500 Pxx500 left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. _recover_creation activates GitHub tickets (which sets category_prompt_retry_at) but neither sends the prompt nor wakes deadlines when next_action_at is None (normal for RoutingMode.NONE).
  2. A failed prompt in process_due returns 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.

Comment thread NHCogs/githubtickets/coordinator.py
Comment thread NHCogs/githubtickets/coordinator.py Outdated

@Pxx500 Pxx500 left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant