fix: use get_app().slug for app_bot_login instead of get_user().login

[rnetser]

Problem

app_bot_login initialization fails on every webhook with a 403 error:

Failed to get app bot login — bot-PR detection may not work

get_user() always returns 403 for GitHub App installation tokens — this is a GitHub platform limitation, not a permissions issue.

Impact

Without app_bot_login, the server cannot identify its own PRs:

• Cherry-pick retry cannot verify bot ownership of existing PRs
• Rebase auth degradation — falls back to less reliable detection

Fix

Replace get_user().login with get_app() + f"{slug}[bot]". The get_app() endpoint is designed for app tokens and returns app metadata including slug. The bot login format is always {slug}[bot].

Closes #1113

Generated-by: Claude noreply@anthropic.com

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (2) 📘 Rule violations (0) 📎 Requirement gaps (1) 📜 Skill insights (0)

Context used

✅ Tickets: 🎫 fix: app_bot_login fails to initialize — GET /user not available to GitHub App installation tokens

1. get_github_app_slug() swallows exceptions ✓ Resolved 📘 Rule violation ☼ Reliability

Description

get_github_app_slug() catches all exceptions and returns None, which prevents
github_api_call() from observing transient GitHub/API/network failures and applying its
retry/backoff logic. As a result, process() may only log an error and leave app_bot_login unset,
causing silent initialization failures and degraded bot-PR detection until a later webhook succeeds.

Code

webhook_server/utils/github_repository_settings.py[R440-451]

+    try:
+        with open(os.path.join(config_.data_dir, "webhook-server.private-key.pem")) as fd:
+            private_key = fd.read()
+
+        github_app_id: int = config_.root_data["github-app-id"]
+        auth: AppAuth = Auth.AppAuth(app_id=github_app_id, private_key=private_key)
+        app_instance: GithubIntegration = GithubIntegration(auth=auth)
+        return app_instance.get_app().slug
+
+    except Exception:
+        LOGGER.exception("Failed to get GitHub App slug")
+        return None

Evidence

PR Compliance ID 8 requires PyGithub operations to run through github_api_call() so transient
failures can be retried with backoff, but the new get_github_app_slug() wraps access to
app_instance.get_app().slug in a broad exception handler that converts any exception into None.
Since github_api_call() retries only when exceptions propagate out of the callable it wraps,
swallowing exceptions in the helper disables retries for this initialization path even when the
underlying error is retryable (e.g., 500/502/503/504 or other network/API failures).

CLAUDE.md: Wrap All Potentially Blocking PyGithub Operations with github_api_call() (No Direct PyGithub Calls)
webhook_server/utils/github_repository_settings.py[440-451]
webhook_server/utils/github_repository_settings.py[435-451]
webhook_server/libs/github_api.py[544-558]
webhook_server/utils/github_retry.py[65-126]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`get_github_app_slug()` currently wraps its implementation in a broad `try/except Exception` and returns `None` on any failure, which prevents `github_api_call()` from retrying transient GitHub/API/network errors because no exception propagates. This can leave `app_bot_login` unset (after `process()` logs an error), degrading bot-PR detection until a later webhook run succeeds.

## Issue Context
- PR Compliance ID 8 expects PyGithub operations to be executed via `github_api_call()` to get retry/backoff behavior.
- `webhook_server/libs/github_api.py` uses `github_api_call()` to run work off the event loop and to retry transient failures.
- `GithubWebhook.process()` initializes `app_bot_login` via `await github_api_call(get_github_app_slug, ...)`, but the helper’s broad exception handler converts failures from `app_instance.get_app().slug` into `None`, defeating retry/backoff.

## Fix Focus Areas
- webhook_server/utils/github_repository_settings.py[435-451]
- webhook_server/libs/github_api.py[544-558]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. No tests for app_bot_login 📎 Requirement gap ☼ Reliability

Description

app_bot_login initialization behavior was changed to derive the login from get_app().slug, but
there are no accompanying tests validating this initialization path for GitHub App installation
tokens. This risks regressions where app_bot_login becomes unset/incorrect and bot-PR detection
breaks silently.

Code

webhook_server/libs/github_api.py[R554-559]

+                    _app = await github_api_call(
+                        _github_app_api.get_app,
                        logger=self.logger,
                        log_prefix=self.log_prefix,
                    )
+                    self.app_bot_login = f"{_app.slug}[bot]"

Evidence

Rule IDs 2 and 9 require updating/adding automated tests for the new app_bot_login initialization
behavior. The PR changes GithubWebhook.process() to set `self.app_bot_login =
f"{_app.slug}[bot]", but current tests shown rely on manually setting app_bot_login` rather than
validating initialization, leaving the new get_app().slug construction untested.

Update and pass tests covering app bot login initialization for installation tokens
CLAUDE.md: Maintain at least 90% test coverage and add tests for new code
webhook_server/libs/github_api.py[543-565]
webhook_server/tests/test_issue_comment_handler.py[2075-2084]
webhook_server/tests/test_runner_handler.py[2891-2906]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`app_bot_login` initialization was changed to use `get_app().slug` + `[bot]`, but there are no tests asserting this behavior (and existing tests often set `app_bot_login` manually).

## Issue Context
Compliance requires tests that validate `app_bot_login` initializes successfully under GitHub App installation token auth using `GET /app`, and that the constructed value matches `{slug}[bot]`.

## Fix Focus Areas
- webhook_server/libs/github_api.py[543-565]
- webhook_server/tests/test_github_api.py[237-305]
- webhook_server/tests/test_issue_comment_handler.py[2075-2084]
- webhook_server/tests/test_runner_handler.py[2891-2906]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

3. Duplicate app-id lookup ✓ Resolved 🐞 Bug ➹ Performance

Description

get_github_app_slug() reads github-app-id via Config.get_value() and then calls
_create_github_integration(), which reads github-app-id again; this duplicates config lookups
and can also duplicate config file parsing on cold cache or first use.
Because Config.root_data loads YAML from disk on each access, the duplicated get_value() calls
can add avoidable file I/O and parsing overhead in the webhook path.

Code

webhook_server/utils/github_repository_settings.py[R455-457]

+    github_app_id: int | None = config_.get_value("github-app-id")
+    if not github_app_id:
+        raise ValueError("github-app-id not configured — required for GitHub App slug lookup")

Relevance

⭐⭐⭐ High

Team accepts hot-path I/O/perf fixes (e.g., avoid fsync per log, cache repeated lookups/API calls).

PR-#1031
PR-#906
PR-#1108

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The new code shows get_github_app_slug() and _create_github_integration() both calling
config_.get_value("github-app-id"), and Config.root_data is implemented as a property that
reads/parses the config file on each access; get_value() relies on repository_data and
root_data, which can cause repeated root_data access.

webhook_server/utils/github_repository_settings.py[413-426]
webhook_server/utils/github_repository_settings.py[448-466]
webhook_server/libs/config.py[65-84]
webhook_server/libs/config.py[132-153]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`get_github_app_slug()` calls `config_.get_value("github-app-id")` and then calls `_create_github_integration(config_)`, which calls `config_.get_value("github-app-id")` again. This duplicates the config lookup and may duplicate config file reads/parsing.

### Issue Context
`Config.root_data` reads and parses `config.yaml` every time it is accessed, and `Config.get_value()` consults both `repository_data` and `root_data`, which can trigger multiple `root_data` accesses.

### Fix Focus Areas
- webhook_server/utils/github_repository_settings.py[422-424]
- webhook_server/utils/github_repository_settings.py[455-457]

### Suggested fix
- Change `_create_github_integration()` to accept `github_app_id: int` as an optional parameter (or require it), and only call `config_.get_value("github-app-id")` when the parameter is not provided.
- In `get_github_app_slug()`, read `github_app_id` once and pass it into `_create_github_integration(...)` so the value is reused consistently.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

4. Slug cache key may be None ✓ Resolved 📘 Rule violation ☼ Reliability

Description

get_github_app_slug() uses config_.get_value("github-app-id") as a cache key and assumes it is
an int, but get_value() can return None, undermining fail-fast behavior for required config.
This can produce confusing behavior (e.g., caching under None) and delayed errors.

Code

webhook_server/utils/github_repository_settings.py[R453-456]

+    github_app_id: int = config_.get_value("github-app-id")
+
+    if github_app_id in _github_app_slug_cache:
+        return _github_app_slug_cache[github_app_id]

Relevance

⭐⭐⭐ High

Same fail-fast pattern: avoid propagating None into logic/caches; repository has accepted explicit
config validation safeguards (#998).

PR-#998

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The new code uses github_app_id (from get_value(), which can return None) in cache logic. This
conflicts with the fail-fast requirement for required config and can result in placeholder-driven
behavior before an eventual downstream exception.

CLAUDE.md: Avoid Unnecessary Defensive Programming; Enforce Fail-Fast Behavior
webhook_server/utils/github_repository_settings.py[453-456]
webhook_server/libs/config.py[132-154]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`get_github_app_slug()` assumes `github_app_id` is a required `int`, but it is sourced from `Config.get_value()` which may return `None`.

## Issue Context
Fail-fast compliance expects required configuration to raise a clear error immediately, rather than allowing placeholder values to affect caching/control flow.

## Fix Focus Areas
- webhook_server/utils/github_repository_settings.py[453-456]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

5. github-app-id may be None ✓ Resolved 📘 Rule violation ☼ Reliability

Description

_create_github_integration() (and get_github_app_slug()) treats
config_.get_value("github-app-id") as a required int, but get_value() can return None when
the key is absent, allowing an invalid placeholder to flow into Auth.AppAuth and fail later with a
less clear runtime error. This breaks fail-fast expectations and can also lead to misleading/extra
retries when invoked via github_api_call().

Code

webhook_server/utils/github_repository_settings.py[R419-424]

    with open(os.path.join(config_.data_dir, "webhook-server.private-key.pem")) as fd:
        private_key = fd.read()

-    github_app_id: int = config_.root_data["github-app-id"]
+    github_app_id: int = config_.get_value("github-app-id")
    auth: AppAuth = Auth.AppAuth(app_id=github_app_id, private_key=private_key)
-    app_instance: GithubIntegration = GithubIntegration(auth=auth)
+    return GithubIntegration(auth=auth)

Relevance

⭐⭐⭐ High

Team often accepts fail-fast/validation on config-derived values (type/validity checks accepted in
#998; prefers clear errors over silent None).

PR-#998
PR-#1002

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The cited code path switched from direct indexing (config_.root_data["github-app-id"], which fails
fast with a KeyError) to Config.get_value(), and then assigns the result to an int-annotated
variable and passes it into Auth.AppAuth without validating it; however, Config.get_value() is
documented/implemented to return its return_on_none default (i.e., None) when the key is
missing. As a result, a missing required config value no longer triggers an immediate, clear error,
but instead propagates None into auth construction, causing a later, harder-to-diagnose runtime
failure and potentially unnecessary retries higher up the call stack.

CLAUDE.md: Avoid Unnecessary Defensive Programming; Enforce Fail-Fast Behavior
webhook_server/utils/github_repository_settings.py[419-424]
webhook_server/libs/config.py[132-154]
webhook_server/utils/github_repository_settings.py[413-424]
webhook_server/utils/github_repository_settings.py[446-456]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`Config.get_value("github-app-id")` can return `None` (and potentially a non-int), but `_create_github_integration()` and `get_github_app_slug()` currently treat it as a required `int` and pass it into `Auth.AppAuth`, causing confusing downstream runtime failures instead of failing fast with a clear error.

## Issue Context
Per compliance guidance, required configuration should fail fast with an explicit, understandable exception rather than propagating placeholder values that fail later. The new implementation replaced `config_.root_data["github-app-id"]` (which raised `KeyError` when missing) with `get_value()` (which returns `return_on_none`, default `None`, when a key is absent) but did not add explicit validation, which can also lead to misleading/extra retries when the call is made through `github_api_call()`.

## Fix Focus Areas
- webhook_server/utils/github_repository_settings.py[419-424]
- webhook_server/utils/github_repository_settings.py[453-454]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

View more (8)

6. Ignores app-id overrides ✓ Resolved 🐞 Bug ≡ Correctness

Description

get_github_app_slug() reads github-app-id from config_.root_data, bypassing
Config.get_value() precedence (repo/local overrides). In deployments where different repositories
use different GitHub App IDs, this can cache/return the wrong slug and generate an incorrect
app_bot_login, breaking bot-PR detection.

Code

webhook_server/utils/github_repository_settings.py[R453-456]

+    github_app_id: int = config_.root_data["github-app-id"]
+
+    if github_app_id in _github_app_slug_cache:
+        return _github_app_slug_cache[github_app_id]

Relevance

⭐⭐⭐ High

Team repeatedly enforces config override precedence via Config.get_value/extra_dict (accepted in PRs
#1109, #1112).

PR-#1109
PR-#1112

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The new slug helper keys caching off root_data, but the Config abstraction explicitly supports
repository and local overrides and is used elsewhere for github-app-id, so bypassing it can yield
a mismatched app id/slug pairing.

webhook_server/utils/github_repository_settings.py[446-456]
webhook_server/libs/config.py[132-153]
webhook_server/libs/github_api.py[878-883]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`get_github_app_slug()` uses `config_.root_data["github-app-id"]`, which ignores config precedence (repo-level and repo-local overrides). This can make the slug cache key incorrect and return the wrong slug for repositories that override `github-app-id`.

## Issue Context
Other parts of the codebase retrieve `github-app-id` via `Config.get_value(..., extra_dict=repository_config)`, indicating per-repo overrides are supported/expected.

## Fix Focus Areas
- webhook_server/utils/github_repository_settings.py[453-456]

## Suggested fix
- Replace `config_.root_data["github-app-id"]` with an override-aware resolution, e.g. `config_.get_value("github-app-id")` (and/or thread the relevant repo-local `extra_dict` into this helper if needed).
- Ensure the cache key matches the resolved/effective app id for the current repository context.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

7. Tests allow get_user() usage ✓ Resolved 📎 Requirement gap ☼ Reliability

Description

The new app_bot_login tests configure mock_api.get_user() but never assert it is not called
during slug-based initialization, so a regression reintroducing GET /user could still pass. This
weakens the guarantee that installation-token paths do not rely on get_user().

Code

webhook_server/tests/test_github_api.py[R584-601]

+        mock_api = Mock()
+        mock_api.rate_limiting = [100, 5000]
+        mock_user = Mock()
+        mock_user.login = "test-user"
+        mock_api.get_user.return_value = mock_user
+
+        mock_api_rate_limit.return_value = (mock_api, "TOKEN", "USER")
+        mock_repo_api.return_value = Mock()
+        mock_repo_github_api.return_value = Mock()
+        mock_get_apis.return_value = []
+        mock_repo_local_data.return_value = {}
+
+        headers = Headers({"X-GitHub-Event": "unsupported_event"})
+        webhook = GithubWebhook(hook_data=pull_request_payload, headers=headers, logger=Mock())
+        webhook.app_bot_login = ""
+
+        await webhook.process()
+        assert webhook.app_bot_login == "manage-repositories-app[bot]"

Relevance

⭐⭐⭐ High

They frequently accept negative-call assertions to prevent regressions (e.g.,
assert_not_called/not_awaited in PRs #1007, #1036).

PR-#1007
PR-#1036

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

PR Compliance ID 2 requires tests to validate slug-based bot login formatting and to fail if
get_user() is invoked in the installation-token context. In the added tests, mock_api.get_user
is configured, but there is no assertion that it is not called, so a regression could still pass.

Update and add tests to cover app_bot_login initialization via GET /app and bot login formatting
webhook_server/tests/test_github_api.py[584-589]
webhook_server/tests/test_github_api.py[600-601]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The new tests for `app_bot_login` initialization don’t fail if `get_user()` is invoked, because they set up `mock_api.get_user.return_value` and don’t assert `mock_api.get_user.assert_not_called()` (or similar). This leaves the rule “no GET /user for installation tokens” unprotected against regressions.

## Issue Context
Per compliance, installation-token authentication must avoid `GET /user`. Tests should explicitly fail if `get_user()` is called in the slug-based initialization path.

## Fix Focus Areas
- webhook_server/tests/test_github_api.py[564-683]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

8. Misordered patch mock args ✓ Resolved 🐞 Bug ≡ Correctness

Description

The new async tests pass mock_repo_api and mock_repo_github_api in the opposite order of their
stacked @patch(...) decorators, so the return values are configured on the wrong patched
functions. This makes the tests brittle and can silently stop validating the intended behavior (or
mask unintended behavior) depending on future changes to the mocked call sites.

Code

webhook_server/tests/test_github_api.py[R564-580]

+    @patch.dict(os.environ, {"WEBHOOK_SERVER_DATA_DIR": "webhook_server/tests/manifests"})
+    @patch("webhook_server.libs.github_api.get_github_repo_api")
+    @patch("webhook_server.libs.github_api.get_repository_github_app_api")
+    @patch("webhook_server.libs.github_api.get_api_with_highest_rate_limit")
+    @patch("webhook_server.utils.helpers.get_apis_and_tokes_from_config")
+    @patch("webhook_server.libs.config.Config.repository_local_data")
+    @patch("webhook_server.libs.github_api.GithubWebhook.add_api_users_to_auto_verified_and_merged_users")
+    @patch("webhook_server.libs.github_api.get_github_app_slug", return_value="manage-repositories-app")
+    async def test_process_initializes_app_bot_login_from_slug(
+        self,
+        mock_get_slug: Mock,
+        mock_auto_verified_prop: Mock,
+        mock_repo_local_data: Mock,
+        mock_get_apis: Mock,
+        mock_api_rate_limit: Mock,
+        mock_repo_api: Mock,
+        mock_repo_github_api: Mock,

Relevance

⭐⭐⭐ High

Team often accepts test-mocking correctness fixes in this file (e.g., patching/to_thread mocking
fixes in PR #906).

PR-#906

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The decorator stack patches get_github_repo_api above get_repository_github_app_api, but the
test function signature lists mock_repo_api before mock_repo_github_api. With stacked patches,
this reverses which patched function each parameter corresponds to, so the test sets return values
on the wrong patched call sites.

webhook_server/tests/test_github_api.py[564-582]
webhook_server/libs/github_api.py[47-61]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
The stacked `@patch(...)` decorators inject mocks in reverse order (bottom-most patch becomes the first argument). In the new tests, the parameters for the top two patches are swapped (`mock_repo_api` vs `mock_repo_github_api`), so the test configures the wrong mock objects.

### Issue Context
This affects at least these new tests:
- `test_process_initializes_app_bot_login_from_slug`
- `test_process_app_bot_login_skips_when_already_set`
- `test_process_app_bot_login_handles_slug_failure`

### Fix Focus Areas
- webhook_server/tests/test_github_api.py[564-660]

### Expected change
For each affected test, swap the order of the last two injected mocks so it matches the decorator stack (i.e., `get_repository_github_app_api` mock param should come before `get_github_repo_api` mock param), and ensure the `return_value` assignments match the corrected param names.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

9. Tests don’t assert get_value() ✓ Resolved 📘 Rule violation ⚙ Maintainability

Description

The updated tests set both mock_config.root_data and mock_config.get_value.return_value but
never assert that get_value("github-app-id") is called, so a regression back to reading
root_data["github-app-id"] directly would still pass. This weakens test coverage and regression
protection for the newly changed config access behavior.

Code

webhook_server/tests/test_github_repository_settings.py[R726-730]

        mock_config = Mock()
        mock_config.data_dir = "/test/dir"
        mock_config.root_data = {"github-app-id": 12345}
+        mock_config.get_value.return_value = 12345

Relevance

⭐⭐ Medium

Mixed history: extra negative call asserts accepted (#1036) but “tighten assertion” suggestions
often rejected (#994).

PR-#1036
PR-#994

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

PR compliance requires new or changed behavior to be covered by tests, but while these tests now
configure mock_config.get_value.return_value, they also still provide `mock_config.root_data =
{"github-app-id": 12345} and only validate outcomes (e.g., Auth.AppAuth(app_id=12345, ...)`) that
would succeed whether the value came from get_value() or direct root_data access. Because there
is no assertion that get_value() was invoked, the tests do not actually prove the implementation
uses the new access path and therefore would not reliably catch a regression back to root_data.

CLAUDE.md: Meet Minimum Test Coverage (90%) and Add Tests for New Code
webhook_server/tests/test_github_repository_settings.py[724-733]
webhook_server/tests/test_github_repository_settings.py[724-754]
webhook_server/tests/test_github_repository_settings.py[757-783]
webhook_server/tests/test_github_repository_settings.py[786-819]
webhook_server/tests/test_github_repository_settings.py[821-849]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Tests updated alongside the `Config.get_value()` change do not actually verify that `get_value("github-app-id")` is used, because they set `mock_config.get_value.return_value` but never assert the method was called.

## Issue Context
The current test setup provides both `root_data["github-app-id"]` and `get_value()` return values, and the main behavioral check (e.g., constructing `Auth.AppAuth(app_id=12345, ...)`) would pass regardless of whether production code reads from `root_data[...]` or calls `get_value()`. This means the tests won’t fail if the implementation accidentally reverts to reading `config_.root_data["github-app-id"]` directly.

## Fix Focus Areas
- webhook_server/tests/test_github_repository_settings.py[724-754]
- webhook_server/tests/test_github_repository_settings.py[757-783]
- webhook_server/tests/test_github_repository_settings.py[786-819]
- webhook_server/tests/test_github_repository_settings.py[821-849]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

10. Lock blocks threadpool workers 🐞 Bug ➹ Performance

Description

get_github_app_slug() holds a module-level threading.Lock while performing the network call
app_instance.get_app().slug, so concurrent webhooks can tie up multiple asyncio.to_thread workers
waiting on the lock during the initial cache-miss window. This can delay unrelated GitHub API calls
that also rely on github_api_call()'s shared threadpool executor.

Code

webhook_server/utils/github_repository_settings.py[R458-466]

+    with _github_app_slug_lock:
+        # Double-checked locking
+        if _github_app_slug_cache is not None:
+            return _github_app_slug_cache
+
+        LOGGER.debug("Getting GitHub App slug")
+        app_instance = _create_github_integration(config_)
+        slug = app_instance.get_app().slug
+        if not slug:

Evidence

The slug fetch is performed inside a locked section and is invoked via github_api_call(), which
executes work in the shared asyncio.to_thread threadpool. During the initial cache miss, concurrent
webhooks can therefore occupy multiple threadpool workers waiting for the lock while one worker
performs the network call.

webhook_server/utils/github_repository_settings.py[446-469]
webhook_server/utils/github_retry.py[65-126]
webhook_server/libs/github_api.py[546-560]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`get_github_app_slug()` currently acquires `_github_app_slug_lock` and then performs the potentially slow GitHub API request `app_instance.get_app().slug` while holding the lock. When many webhook handlers hit this code concurrently (before the cache is populated), multiple `asyncio.to_thread` workers can block on that lock and reduce overall throughput.

### Issue Context
- `GithubWebhook.process()` initializes `app_bot_login` by calling `github_api_call(get_github_app_slug, ...)`.
- `github_api_call()` runs the callable in the default threadpool via `asyncio.to_thread`, so blocked threads reduce capacity for other GitHub calls.

### Fix Focus Areas
- webhook_server/utils/github_repository_settings.py[446-469]

### Suggested fix
Minimize the lock scope by doing the network fetch outside the lock and using the lock only to publish the cached value. A common pattern:
1) Fast path: return cached value if present.
2) Compute `slug = ...get_app().slug` without holding `_github_app_slug_lock`.
3) Acquire lock and set cache only if still unset (another thread may have already populated it).

This avoids holding the lock across network I/O while keeping the cache semantics intact.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

11. Unscoped slug cache 🐞 Bug ≡ Correctness

Description

get_github_app_slug() caches a single slug in a module-global variable without keying by Config
(github-app-id / data_dir), so a later call with a different Config can return the wrong slug and
generate an incorrect app_bot_login.

Code

webhook_server/utils/github_repository_settings.py[R443-446]

+    global _github_app_slug_cache
+
+    if _github_app_slug_cache is not None:
+        return _github_app_slug_cache

Evidence

The implementation returns a single cached slug regardless of which Config is passed, even though
the slug is derived from config-provided app id and key path; Config instances can legitimately
differ because data_dir is environment-derived and app id is read from config.yaml (which may vary
across configs/repositories/tests).

webhook_server/utils/github_repository_settings.py[45-46]
webhook_server/utils/github_repository_settings.py[436-456]
webhook_server/libs/config.py[13-22]
webhook_server/libs/github_api.py[862-866]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`get_github_app_slug()` stores a single process-wide `_github_app_slug_cache` and returns it for all calls once set. If the same process ever uses multiple `Config` instances with different `github-app-id` and/or different `data_dir` (different private key/config), the cached slug can be wrong, producing an incorrect `{slug}[bot]` login.

## Issue Context
The function already takes `config_` and reads both `config_.data_dir` and `config_.root_data["github-app-id"]`, which implies the returned value is config-dependent. A process-wide singleton cache breaks that implied contract.

## Fix Focus Areas
- webhook_server/utils/github_repository_settings.py[46-46]
- webhook_server/utils/github_repository_settings.py[443-447]
- webhook_server/utils/github_repository_settings.py[454-455]

## Suggested fix
- Change `_github_app_slug_cache` from `str | None` to a dict keyed by something like `(github_app_id, config_.data_dir)` (or `(github_app_id, config_.config_path)`), and cache per-key.
- Alternatively, refactor into a helper `@functools.lru_cache` function that accepts hashable inputs (e.g., `app_id: int, pem_path: str`) and have `get_github_app_slug(config_)` call it.
- Ensure behavior remains: raise on failure so `github_api_call()` can apply retry/backoff.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

12. Uncached GitHub App slug ✓ Resolved 🐞 Bug ➹ Performance

Description

get_github_app_slug() reads the private key and calls GitHub (GET /app) every time it’s invoked,
and GithubWebhook.process() invokes it per webhook because app_bot_login is reset on each new
handler instance. This adds avoidable per-event I/O/API latency and can amplify transient failures
into repeated error logs under high webhook volume.

Code

webhook_server/utils/github_repository_settings.py[R441-447]

+    with open(os.path.join(config_.data_dir, "webhook-server.private-key.pem")) as fd:
+        private_key = fd.read()
+
+    github_app_id: int = config_.root_data["github-app-id"]
+    auth: AppAuth = Auth.AppAuth(app_id=github_app_id, private_key=private_key)
+    app_instance: GithubIntegration = GithubIntegration(auth=auth)
+    return app_instance.get_app().slug

Evidence

A new webhook handler instance is created for each request, and it resets app_bot_login to an
empty string, which causes process() to call get_github_app_slug() on every webhook. The helper
function performs both disk I/O (PEM read) and a GitHub API call (get_app()) each time, so the
overhead repeats per event.

webhook_server/app.py[455-461]
webhook_server/libs/github_api.py[200-200]
webhook_server/libs/github_api.py[545-553]
webhook_server/utils/github_repository_settings.py[435-447]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`get_github_app_slug()` performs disk I/O (reads the PEM) and a network call (`GithubIntegration(...).get_app().slug`) every time it’s called. Because a new `GithubWebhook` object is created per incoming webhook and `app_bot_login` starts empty, this slug lookup runs on every webhook.

## Issue Context
- `GithubWebhook` is instantiated per request.
- `self.app_bot_login` is initialized to `""` in `__init__`, so `process()` always triggers the slug lookup path.

## Fix Focus Areas
- webhook_server/utils/github_repository_settings.py[435-447]
- webhook_server/libs/github_api.py[545-553]

## Suggested fix
- Cache the app slug (or the derived bot login string) at process-level scope (module global) or application scope:
 - Option A: Introduce a module-level cache (e.g., `_APP_SLUG: str | None`) + a lock to ensure only one fetch occurs concurrently.
 - Option B: Store the slug in `Config` (or another long-lived singleton) after the first successful fetch.
- Ensure cache invalidation is either unnecessary (slug is effectively immutable) or explicitly tied to config changes (e.g., app id / data_dir).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

13. Slug read outside retry ✓ Resolved 🐞 Bug ☼ Reliability

Description

process() calls github_api_call(_github_app_api.get_app) but then reads _app.slug outside the
wrapper; if slug is lazily loaded by PyGithub, it can perform synchronous HTTP on the event loop
thread and bypass github_api_call retries/logging. Keep the entire bot-login derivation inside
github_api_call (e.g., return the final string from the wrapped callable).

Code

webhook_server/libs/github_api.py[R554-559]

+                    _app = await github_api_call(
+                        _github_app_api.get_app,
                        logger=self.logger,
                        log_prefix=self.log_prefix,
                    )
+                    self.app_bot_login = f"{_app.slug}[bot]"

Evidence

The project’s retry wrapper explicitly calls out using lambdas for property access because PyGithub
is synchronous, and other parts of the code consistently wrap attribute reads
(pull_request.number, pull_request.user.login) with github_api_call for that reason. The new
code returns an App object from the wrapper but then reads .slug outside of it, which breaks that
pattern and can bypass the wrapper’s thread offload/retry if .slug triggers lazy loading.

webhook_server/libs/github_api.py[543-565]
webhook_server/utils/github_retry.py[1-27]
webhook_server/utils/github_retry.py[65-104]
webhook_server/libs/github_api.py[622-630]
webhook_server/libs/handlers/issue_comment_handler.py[752-756]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`_app.slug` is accessed outside `github_api_call`, which is the project’s wrapper intended to keep all PyGithub (sync) operations off the event loop and apply retry behavior.

### Issue Context
This repo documents using `github_api_call` for both method calls and property access because PyGithub is synchronous and properties may require blocking work.

### Fix Focus Areas
- webhook_server/libs/github_api.py[543-565]

### Suggested change
Refactor to compute the final bot login inside `github_api_call`, e.g.:

```py
self.app_bot_login = await github_api_call(
   lambda: f"{_github_app_api.get_app().slug}[bot]",
   logger=self.logger,
   log_prefix=self.log_prefix,
)
```

(Or fetch `slug` via `github_api_call(lambda: _github_app_api.get_app().slug, ...)` and then format the string.)

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

[myakove-bot]

Report bugs in Issues

Welcome! 🎉

This pull request will be automatically processed with the following features:

🔄 Automatic Actions

• Reviewer Assignment: Reviewers are automatically assigned based on the OWNERS file in the repository root
• Size Labeling: PR size labels (XS, S, M, L, XL, XXL) are automatically applied based on changes
• Issue Creation: Disabled for this repository
• Pre-commit Checks: pre-commit runs automatically if .pre-commit-config.yaml exists
• Branch Labeling: Branch-specific labels are applied to track the target branch
• Auto-verification: Auto-verified users have their PRs automatically marked as verified
• Labels: All label categories are enabled (default configuration)

📋 Available Commands

PR Status Management

• /wip - Mark PR as work in progress (adds WIP: prefix to title)
• /wip cancel - Remove work in progress status
• /hold - Block PR merging (approvers only)
• /hold cancel - Unblock PR merging
• /verified - Mark PR as verified
• /verified cancel - Remove verification status
• /reprocess - Trigger complete PR workflow reprocessing (useful if webhook failed or configuration changed)
• /regenerate-welcome - Regenerate this welcome message
• /security-override - Set security check runs to pass (maintainers only)
• /security-override cancel - Re-run security checks

Review & Approval

• /lgtm - Approve changes (looks good to me)
• /approve - Approve PR (approvers only)
• /automerge - Enable automatic merging when all requirements are met (maintainers and approvers only)
• /assign-reviewers - Assign reviewers based on OWNERS file
• /assign-reviewer @username - Assign specific reviewer
• /check-can-merge - Check if PR meets merge requirements

Testing & Validation

• /retest tox - Run Python test suite with tox
• /retest build-container - Rebuild and test container image
• /retest python-module-install - Test Python package installation
• /retest pre-commit - Run pre-commit hooks and checks
• /retest conventional-title - Validate commit message format
• /retest all - Run all available tests

Container Operations

• /build-and-push-container - Build and push container image (tagged with PR number)
  • Supports additional build arguments: /build-and-push-container --build-arg KEY=value

Cherry-pick Operations

• /cherry-pick <branch> - Schedule cherry-pick to target branch when PR is merged
  • Multiple branches: /cherry-pick branch1 branch2 branch3
• /cherry-pick-retry <branch> - Retry a failed cherry-pick (merged PRs only)

Branch Management

• /rebase - Rebase this PR branch onto its base branch

Label Management

• /<label-name> - Add a label to the PR
• /<label-name> cancel - Remove a label from the PR

✅ Merge Requirements

This PR will be automatically approved when the following conditions are met:

1. Approval: /approve from at least one approver
2. LGTM Count: Minimum 1 /lgtm from reviewers
3. Status Checks: All required status checks must pass
4. No Blockers: No wip, hold, has-conflicts labels and PR must be mergeable (no conflicts)
5. Verified: PR must be marked as verified

📊 Review Process

Approvers and Reviewers

Approvers:

• myakove
• rnetser

Reviewers:

• myakove
• rnetser

Available Labels

• hold
• verified
• wip
• lgtm
• approve
• automerge

AI Features

• Conventional Title: Mode: fix (claude/claude-opus-4-6[1m])
• Cherry-Pick Conflict Resolution: Enabled (claude/claude-opus-4-6[1m])
• Test Oracle: Triggers: approved (claude/claude-opus-4-6[1m]); /test-oracle can be used anytime

Security Checks

• Suspicious Path Detection: Monitors paths: .claude/, .vscode/, .cursor/, .devcontainer/, .pi/, .github/workflows/, .github/actions/
• Committer Identity Check: Verifies last committer matches PR author
• Mandatory: Security checks block merge (use /security-override to bypass — maintainers only)

💡 Tips

• WIP Status: Use /wip when your PR is not ready for review
• Verification: The verified label is removed on new commits unless the push is detected as a clean rebase
• Cherry-picking: Cherry-pick labels are processed when the PR is merged
• Container Builds: Container images are automatically tagged with the PR number
• Permission Levels: Some commands require approver permissions
• Auto-verified Users: Certain users have automatic verification and merge privileges

For more information, please refer to the project documentation or contact the maintainers.

[qodo-code-review]

PR Summary by Qodo

Fix app bot login detection using GitHub App slug (avoid get_user() 403)
🐞 Bug fix 🕐 10-20 Minutes

Walkthroughs

Description

• Fix app_bot_login initialization for GitHub App installation tokens (avoid 403 from get_user()).
• Derive bot login from get_app().slug using the stable {slug}[bot] format.
• Restore reliable bot-owned PR detection for webhook-driven operations (rebase/cherry-pick retry).

Diagram

graph TD
  A["Webhook processor"] --> B["GitHub App API client"] --> C{{"GitHub REST API"}} --> D["App metadata (slug)"] --> E["app_bot_login: {slug}[bot]"]

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Make bot login configurable (env/setting)

• ➕ Avoids any GitHub API call during initialization
• ➕ Works even if GitHub API is partially unavailable
• ➖ Operationally brittle (must match app slug exactly)
• ➖ Harder to deploy across multiple environments/apps

2. Attempt get_user() then fall back to get_app()

• ➕ Potentially preserves behavior for non-app tokens in shared code paths
• ➖ Extra API call and noisy failures for installation tokens (known 403 behavior)
• ➖ More complex control flow without adding real value for the target scenario

Recommendation: Keep the PR’s approach: using get_app() is the correct endpoint for GitHub App installation tokens, and deriving {slug}[bot] is stable and deterministic. Configuration-based approaches add operational risk, and get_user() fallback logic would introduce avoidable errors/noise for the primary failing case.

File Changes

Bug fix (1)

github_api.py Derive app_bot_login from get_app().slug instead of get_user().login +3/-2

Derive app_bot_login from get_app().slug instead of get_user().login

• Replaces the app bot login initialization call that previously used get_user().login (which 403s for installation tokens). The code now calls get_app() to retrieve the app slug and constructs the bot login as '{slug}[bot]'.

webhook_server/libs/github_api.py

---

[rnetser]

@qodo-code-review[bot]

The following review comments were reviewed and a decision was made:

webhook_server/libs/github_api.py:554 (qodo requirement gap) — No tests for app_bot_login

Skipped: Skipped — testing process() init path requires mocking the entire webhook flow. The {slug}[bot] format is trivial string formatting. Existing tests set app_bot_login directly which validates downstream usage.

webhook_server/libs/github_api.py:554 (qodo bug) — Slug read outside retry

Addressed: Fixed — wrapped entire operation in a single github_api_call lambda: lambda: f"{_github_app_api.get_app().slug}[bot]". Both get_app() and .slug access now run in the thread pool with retry.

[qodo-code-review]

Code review by qodo was updated up to the latest commit a5fb576

[rnetser]

/build-and-push-container

[myakove-bot]

New container for ghcr.io/myk-org/github-webhook-server:pr-1114 published

[qodo-code-review]

Code review by qodo was updated up to the latest commit a203182

[rnetser]

@qodo-code-review[bot]

The following review comments were reviewed and a decision was made:

webhook_server/utils/github_repository_settings.py:440 (qodo rule violation) — get_github_app_slug() swallows exceptions

Addressed: Fixed — removed try/except from get_github_app_slug(). Exceptions now propagate through github_api_call() for retry/backoff, then caught by the outer try/except in process().

webhook_server/libs/github_api.py:554 (qodo requirement gap) — No tests for app_bot_login

Skipped: Skipped — testing process() init path requires mocking the entire webhook flow. Existing tests set app_bot_login directly which validates downstream usage.

[qodo-code-review]

Code review by qodo was updated up to the latest commit 0a38fb1

[qodo-code-review]

Code review by qodo was updated up to the latest commit 6e2bd74

[qodo-code-review]

code-review[bot]

The following review comments were reviewed and a decision was made:

webhook_server/libs/github_api.py:554 (qodo requirement gap) — No tests for app_bot_login

Addressed: Tests exist: test_process_initializes_app_bot_login_from_slug, test_process_app_bot_login_skips_when_already_set, test_process_app_bot_login_handles_slug_failure (commits 8e6bb6c, 1654124).

webhook_server/utils/github_repository_settings.py:419 (qodo rule violation) — github-app-id may be None

Addressed: Fixed: Added fail-fast if not github_app_id: raise ValueError(...) in both _create_github_integration() and get_github_app_slug(). Type annotation updated to int | None.

webhook_server/utils/github_repository_settings.py:453 (qodo rule violation) — Slug cache key may be None

Addressed: Fixed: Added fail-fast if not github_app_id: raise ValueError(...) in both _create_github_integration() and get_github_app_slug(). Type annotation updated to int | None.

webhook_server/utils/github_repository_settings.py:458 (qodo bug) — Lock blocks threadpool workers

Addressed: Fixed in commit 318ee8a: Network I/O runs outside the lock.

webhook_server/utils/github_repository_settings.py:443 (qodo bug) — Unscoped slug cache

Addressed: Fixed in commit 318ee8a: Cache keyed by github-app-id.

Thanks — those review comments are addressed.

• app_bot_login now has coverage via the new process tests.
• github-app-id is fail-fast validated before use.
• The slug cache is now scoped by github-app-id.
• Lock contention was removed by moving network I/O outside the lock.

From the current findings index, the remaining active items are finding 2, finding 9, and finding 10. If you want, I can help review those next.

[rnetser]

@qodo-code-review[bot]

The following review comments were reviewed and a decision was made:

webhook_server/tests/test_github_repository_settings.py:726 (qodo rule violation) — Tests don’t assert get_value()

Addressed: Fixed: Added mock_config.get_value.assert_called_with("github-app-id") assertion to all 4 GitHub App tests.

[rnetser]

@qodo-code-review re-review

[qodo-code-review]

code-review[bot]

The following review comments were reviewed and a decision was made:

webhook_server/tests/test_github_repository_settings.py:726 (qodo rule violation) — Tests don’t assert get_value()

Addressed: Fixed: Added mock_config.get_value.assert_called_with("github-app-id") assertion to all 4 GitHub App tests.

Looks good — adding mock_config.get_value.assert_called_with("github-app-id") covers the issue raised in finding 5.

No further action needed from my side.

[qodo-code-review]

code-review re-review

Re-review looks good overall: the app_bot_login flow is cleaner, and the new tests cover the happy path plus failure handling.

Still open

• finding 2 — app_bot_login still needs a more direct test around the production path in webhook_server/libs/github_api.py.
• finding 9 — the new slug lookup path can still hold the lock in a way that may block threadpool workers.
• finding 10 — the slug cache is still global rather than scoped more narrowly.

If you want, I can also give a deeper pass on those remaining items or help draft fixes.

[qodo-code-review]

Code review by qodo was updated up to the latest commit f7d6a0a

[rnetser]

@qodo-code-review[bot]

The following review comments were reviewed and a decision was made:

webhook_server/libs/github_api.py:554 (qodo requirement gap) — No tests for app_bot_login

Addressed: Tests exist: test_process_initializes_app_bot_login_from_slug, test_process_app_bot_login_skips_when_already_set, test_process_app_bot_login_handles_slug_failure.

webhook_server/utils/github_repository_settings.py:455 (qodo bug) — Duplicate app-id lookup

Addressed: Fixed: _create_github_integration() now accepts optional github_app_id parameter. get_github_app_slug() passes the pre-fetched ID, avoiding the duplicate get_value() call.

webhook_server/utils/github_repository_settings.py:458 (qodo bug) — Lock blocks threadpool workers

Addressed: Fixed in commit 318ee8a: Network I/O runs outside the lock.

webhook_server/utils/github_repository_settings.py:443 (qodo bug) — Unscoped slug cache

Addressed: Fixed in commit 318ee8a: Cache keyed by github-app-id.

[rnetser]

@qodo-code-review re-review

[qodo-code-review]

code-review[bot]

The following review comments were reviewed and a decision was made:

webhook_server/libs/github_api.py:554 (qodo requirement gap) — No tests for app_bot_login

Addressed: Tests exist: test_process_initializes_app_bot_login_from_slug, test_process_app_bot_login_skips_when_already_set, test_process_app_bot_login_handles_slug_failure.

webhook_server/utils/github_repository_settings.py:455 (qodo bug) — Duplicate app-id lookup

Addressed: Fixed: _create_github_integration() now accepts optional github_app_id parameter. get_github_app_slug() passes the pre-fetched ID, avoiding the duplicate get_value() call.

webhook_server/utils/github_repository_settings.py:458 (qodo bug) — Lock blocks threadpool workers

Addressed: Fixed in commit 318ee8a: Network I/O runs outside the lock.

webhook_server/utils/github_repository_settings.py:443 (qodo bug) — Unscoped slug cache

Addressed: Fixed in commit 318ee8a: Cache keyed by github-app-id.

Thanks — those comments look resolved.

• finding 2 is addressed by the new app_bot_login tests.
• finding 3 is addressed by passing the pre-fetched github_app_id into _create_github_integration().
• finding 10 is addressed by moving network I/O outside the lock.
• finding 11 is addressed by keying the slug cache by github-app-id.

So the remaining open items are just the active findings not mentioned here: finding 2? Actually from the current index, the active ones are finding 2, finding 10, and finding 11 — but the first two in your summary appear to have been fixed in the latest changes. If you want, I can re-check the remaining active findings against the current diff and call out any that still need attention.

[qodo-code-review]

code-review re-review

I re-checked the PR. Most of the earlier findings now look addressed, but there are still 3 active items worth attention:

• finding 2 — app_bot_login still has no direct test coverage for the new slug-based path.
• finding 10 — the slug cache lock may still block threadpool workers during lookup.
• finding 11 — the slug cache is still unscoped, so it can mix entries across configs/apps.

So overall: the PR is close, but I’d want those three resolved before calling it clean.

[qodo-code-review]

Code review by qodo was updated up to the latest commit 79a9bdc

[rnetser]

/build-and-push-container

[myakove-bot]

New container for ghcr.io/myk-org/github-webhook-server:pr-1114 published

[rnetser]

/verified

[rnetser]

code-review re-review
I re-checked the PR. Most of the earlier findings now look addressed, but there are still 3 active items worth attention:

• finding 2 — app_bot_login still has no direct test coverage for the new slug-based path.
• finding 10 — the slug cache lock may still block threadpool workers during lookup.
• finding 11 — the slug cache is still unscoped, so it can mix entries across configs/apps.

All 3 findings are already addressed:

• Finding 2: Tests added in commit 8e6bb6c and updated in 1654124: test_process_initializes_app_bot_login_from_slug, test_process_app_bot_login_skips_when_already_set, test_process_app_bot_login_handles_slug_failure. All assert get_user.assert_not_called().

• Finding 10: Fixed in commit 318ee8a — network I/O (_create_github_integration + get_app().slug) now runs outside the lock. Lock is only acquired for the cache dict write.

• Finding 11: Fixed in commit 318ee8a — cache changed from single str | None to dict[int, str] keyed by github-app-id. Different app configurations get independent cache entries.

[myakove-bot]

Successfully removed PR tag: ghcr.io/myk-org/github-webhook-server:pr-1114.