feat: pull request parsing

[coriolinus]

---

PR Submission Checklist for internal contributors

• The PR Title

  • conforms to the style of semantic commits messages¹ supported in Wire's Github Workflow²
  • contains a reference JIRA issue number like SQPIT-764
  • answers the question: If merged, this PR will: ... ³

• The PR Description

  • is free of optional paragraphs and you have filled the relevant parts to the best of your ability

---

What's new in this PR?

Issues

Briefly describe the issue you have solved or implemented with this pull request. If the PR contains multiple issues, use a bullet list.

Causes (Optional)

Briefly describe the causes behind the issues. This could be helpful to understand the adopted solutions behind some nasty bugs or complex issues.

Solutions

Briefly describe the solutions you have implemented for the issues explained above.

Dependencies (Optional)

If there are some other pull requests related to this one (e.g. new releases of frameworks), specify them here.

Needs releases with:

• GitHub link to other pull request

Testing

Test Coverage (Optional)

• I have added automated test to this contribution

How to Test

Briefly describe how this change was tested and if applicable the exact steps taken to verify that it works as expected.

Notes (Optional)

Specify here any other facts that you think are important for this issue.

Attachments (Optional)

Attachments like images, videos, etc. (drag and drop in the text box)

---

PR Post Submission Checklist for internal contributors (Optional)

• Wire's Github Workflow has automatically linked the PR to a JIRA issue

---

PR Post Merge Checklist for internal contributors

• If any soft of configuration variable was introduced by this PR, it has been added to the relevant documents and the CI jobs have been updated.

---

References

1. https://sparkbox.com/foundry/semantic_commit_messages
2. https://github.com/wireapp/.github#usage
3. E.g. feat(conversation-list): Sort conversations by most emojis in the title #SQPIT-764.

[claude]

PR Review: Pull request parsing → automatic webhook provisioning

Nice refactor. This moves the project from manual, per-conversation webhook setup (user copies a payload URL + secret into GitHub) to fully automated provisioning: a posted PR link triggers GitHub-App-authenticated webhook creation, repo→conversation fan-out, and inactivity-based cleanup. The code is clean and idiomatic, matches the existing style (kebab env vars, companion-object constants, runCatching folds), and the EventsHandler/SignatureValidator simplifications are clear wins. A few things worth addressing before merge.

🔴 Bugs / correctness

1. Blocking network I/O on a coroutine dispatcher. onTextMessageReceived is suspend, and it calls registerRepositories → gitHubWebhookManager.ensureWebhookForConversation → GitHubAppClient, which performs synchronous, blocking httpClient.send(...) calls (installation lookup → token → list hooks → create hook = up to 4 sequential round-trips). This blocks the SDK's dispatcher thread for the full duration. Wrap the blocking work in withContext(Dispatchers.IO) (and consider making the client API suspend or using HttpClient.sendAsync). This is the most impactful issue here.

2. Partial state on provisioning failure. ensureWebhookForConversation does sadd(KNOWN_REPOSITORIES_KEY) + sadd(conversations) + markRepositoryActive before calling GitHub. If ensureRepositoryWebhook throws, the repo is now registered with conversations and a fresh last_activity but no webhook_id — cleanup won't reap it (activity is recent), and notifications will never arrive until a user re-posts a link. Consider only recording conversation/known-repo state after the webhook id is secured, or cleaning up on failure.

3. Stale webhook id is never reconciled. If the Redis webhook_id exists, GitHub is skipped entirely. If the hook was deleted on GitHub's side (or the secret rotated), the app silently believes it's still wired up. Not blocking, but worth a comment/follow-up.

🔒 Security

4. Non-constant-time signature comparison. SignatureValidator.isValid ends in challenge == signature, a short-circuiting String.equals — a timing side-channel on the webhook auth boundary. Use a constant-time compare, e.g. MessageDigest.isEqual(challenge.toByteArray(), signature.toByteArray()). (GitHub itself recommends this for X-Hub-Signature-256 verification.)

5. Webhook secret validated lazily at request time. requireNotNull(ENV_VAR_GITHUB_WEBHOOK_SECRET) lives inside the route handler, so a missing secret surfaces as a 500 on every delivery rather than a fast-fail at startup. Prefer validating required GitHub config at boot (as is already done for WIRE_SDK_CRYPTOGRAPHY_STORAGE_KEY).

⚡ Performance

6. No JWT / installation-token caching. Every ensureRepositoryWebhook and every deleteRepositoryWebhook generates a fresh JWT and installation access token (2 extra API calls each). Installation tokens are valid ~1h; the cleanup sweep deleting N repos pays 2N avoidable calls. Caching per installation id would cut this substantially. Low priority at current volume.

🧹 Code quality

7. Hand-rolled ASN.1/DER to wrap PKCS#1 → PKCS#8 (wrapPkcs1PrivateKey + the der* helpers). Well-written, but reinventing ASN.1 encoding is fragile. Since GitHub Apps hand out PKCS#1 keys (BEGIN RSA PRIVATE KEY) this conversion is genuinely needed — but it deserves dedicated unit tests at minimum, and ideally a small dependency (e.g. BouncyCastle) or documenting that operators can pre-convert with openssl pkcs8.

8. ENV_VAR_GITHUB_CLIENT_SECRET is declared but unused — the doc comment even says it's "kept available." Dead config invites confusion; drop it until an OAuth flow actually needs it.

9. WEBHOOK_VERIFY_SSL = "0" is a misleading name. The GitHub field is insecure_ssl, where "0" enables TLS verification. The value is correct, but the constant name reads as "verify SSL = off." Rename (e.g. WEBHOOK_INSECURE_SSL_DISABLED) or add a comment.

10. ENV_VAR_GITHUB_REPO_INACTIVITY_SECONDS.toLong() at class-init throws NumberFormatException (crashing startup) on a non-numeric value. A toLongOrNull() ?: DEFAULT with a warning is friendlier.

✅ Test coverage

The ApplicationTest updates for the new route/signature/secret are good, but the substantial new logic is otherwise untested:

• GitHubPullRequestLinkParser.parse — pure and high-value. Add cases for trailing punctuation, query/fragment suffixes (/pull/123?w=1, /pull/123/files), multiple links (dedupe via Set), repo names with dots, and the deliberate non-match of enterprise hosts (github.mycorp.com).
• GitHubRepository.fromFullName — round-trip and malformed inputs (a/b/c, a, empty).
• GitHubAppClient key parsing (PKCS#1 and PKCS#8 PEM, escaped \n vs literal newlines) + DER wrapping — verify it yields a key the JDK KeyFactory accepts.
• GitHubWebhookManager idempotency (existing webhook_id short-circuits the GitHub call) and cleanupInactiveRepositories cutoff behavior with an injected Clock (the constructor already takes one — great for testing).

Minor

• conversationsForRepository parses storage keys with split("@"), which correctly matches toStorageKey's format — good, but a shared parse/format helper alongside toStorageKey would keep the two from drifting.
• README/.env docs and build.gradle.kts env wiring for the new variables are thorough — appreciated.

Overall a solid, well-structured change. Items #1 (blocking I/O) and #4 (constant-time compare) are the two I'd prioritize, followed by tests for the parser and key handling.

🤖 Automated review — feedback may contain mistakes; please verify before acting.

[coriolinus]

Your AI changed the name of this function but not its body at all, which feels suspicious.

[SimonThormeyer]

It changed the algorithm parameter, see

    private companion object {
        const val ALGORITHM = "HmacSHA256"
    }