feat: add trusted-committers allowlist for security-committer-identity check

[rnetser]

Problem

Bot cherry-picks and pre-commit CI auto-fixes trigger false committer identity mismatch failures. For example, pre-commit-ci[bot] pushing a formatting fix to a PR authored by legit-user fails the security-committer-identity check, even though this is expected behavior.

Solution

Adds a trusted-committers config option under security-checks that allowlists committer logins for the identity check. When the last committer is in the list, the check passes with a success message instead of failing.

Features

• Case-insensitive comparison (config entries normalized to lowercase)
• Element sanitization (strips whitespace, filters invalid types)
• Warning log when config value is wrong type
• Security ordering: "unknown" committer check always runs first, even if "unknown" is in the trusted list
• Works at both global and per-repository level

Config example

security-checks:
  committer-identity-check: true
  trusted-committers:
    - "pre-commit-ci[bot]"
    - "myorg"

Changes

• webhook_server/config/schema.yaml — added trusted-committers array to security-checks
• webhook_server/libs/github_api.py — reads and sanitizes trusted-committers from config
• webhook_server/libs/handlers/runner_handler.py — checks allowlist before failing on mismatch
• webhook_server/tests/test_security_checks.py — 2 new tests (trusted pass + untrusted fail), updated all fixtures
• examples/config.yaml — added example

Closes #1116

Assisted-by: Claude noreply@anthropic.com

[qodo-code-review]

Code Review by Qodo

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

Context used

✅ Tickets: 🎫 feat: add trusted-committers allowlist for security-committer-identity check

1. AsyncMock return non-iterable ✓ Resolved 🐞 Bug ≡ Correctness

Description

In test_process_status_event/test_process_review_thread_event, get_api_users is patched with
new_callable=AsyncMock but no iterable return_value, so GithubWebhook.process() iterates an
AsyncMock when extending auto_verified_and_merged_users, causing a TypeError or brittle
behavior. This makes the refactor to get_api_users() incorrectly mocked in these tests and can
mask/introduce failures unrelated to the behavior under test.

Code

webhook_server/tests/test_github_api.py[R1000-1004]

                                            with patch.object(
                                                webhook,
-                                                "add_api_users_to_auto_verified_and_merged_users",
+                                                "get_api_users",
                                                new_callable=AsyncMock,
                                            ) as mock_add_api_users:

Evidence

GithubWebhook.process() always awaits get_api_users() and then iterates the returned api_users
when extending auto_verified_and_merged_users. The tests patch get_api_users as an AsyncMock
without setting return_value, so the awaited value can be a mock object that is not safely
iterable, breaking the test execution path for non-early-exit states/actions.

webhook_server/libs/github_api.py[545-547]
webhook_server/tests/test_github_api.py[995-1016]
webhook_server/tests/test_github_api.py[2487-2510]

Agent prompt

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

### Issue description
`GithubWebhook.process()` now does:
- `api_users = await self.get_api_users()`
- iterates `api_users` in `extend(...)`

But the updated tests patch `get_api_users` with `new_callable=AsyncMock` and do not set `return_value`, so the awaited result is a mock object that is not guaranteed to be iterable.

### Issue Context
This occurs in the status-event and review-thread-event tests after the refactor from `add_api_users_to_auto_verified_and_merged_users()` to `get_api_users()`.

### Fix Focus Areas
- webhook_server/tests/test_github_api.py[1000-1005]
- webhook_server/tests/test_github_api.py[2492-2496]

### Proposed fix
In each affected `patch.object(..., "get_api_users", new_callable=AsyncMock)` block, set an explicit iterable return value, e.g.:
- `mock_add_api_users.return_value = ()` (or `[]`)

Alternatively, patch with `AsyncMock(return_value=())` directly.

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

---

2. get_api_users() return type wrong ✓ Resolved 📘 Rule violation ⚙ Maintainability

Description

get_api_users() is annotated as returning tuple[Any], but it returns the result of
asyncio.gather(...), which is not a tuple[Any] (and tuple[Any] also denotes a 1‑item tuple
type). This is incompatible with strict mypy expectations and can hide type errors at call sites.

Code

webhook_server/libs/github_api.py[R842-859]

+    async def get_api_users(self) -> tuple[Any]:
        apis_and_tokens = get_apis_and_tokes_from_config(config=self.config)

        async def check_token(api: github.Github, token: str) -> str | None:
            """Check a single API token and return the user login if valid, None otherwise."""
            token_suffix = f"...{token[-4:]}" if token else "unknown"
            try:
-                rate_limit_remaining = await github_api_call(
-                    lambda: api.rate_limiting[-1], logger=self.logger, log_prefix=self.log_prefix
-                )
+                await github_api_call(lambda: api.rate_limiting[-1], logger=self.logger, log_prefix=self.log_prefix)
            except Exception as ex:
                self.logger.warning(
                    f"{self.log_prefix} Failed to get API rate limit for token ending in '{token_suffix}', "
                    f"skipping. {ex}"
                )
                return None

-            if rate_limit_remaining == 60:
-                self.logger.warning(
-                    f"{self.log_prefix} API has rate limit set to 60 which indicates an invalid token "
-                    f"(token ending in '{token_suffix}'), skipping"
-                )
-                return None
-
            try:
                _api_user = await github_api_call(
                    lambda: api.get_user().login, logger=self.logger, log_prefix=self.log_prefix

Evidence

PR Compliance ID 15 requires new/modified functions to have correct, strict-mypy-compatible type
hints. The updated get_api_users() signature claims tuple[Any] but returns `await
asyncio.gather(...)`, so the annotation is incorrect and too imprecise for strict typing.

CLAUDE.md: Provide complete type hints compatible with mypy strict mode
webhook_server/libs/github_api.py[842-869]

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_api_users()` has an inaccurate return type annotation (`tuple[Any]`) while returning `await asyncio.gather(...)`, which will cause strict mypy/type-checking issues and weakens type safety.

## Issue Context
The project requires complete, strict-mypy-compatible type hints for new/modified functions.

## Fix Focus Areas
- webhook_server/libs/github_api.py[842-869]

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

---

3. trusted-committers defaults empty list 📘 Rule violation ≡ Correctness

Description

When security-checks.trusted-committers is configured with an invalid type, the code logs a
warning and silently defaults to [], masking a misconfiguration and changing security behavior
without failing fast. This violates the requirement to raise on missing/invalid required data
instead of returning placeholder defaults.

Code

webhook_server/libs/github_api.py[R1003-1010]

+        _trusted_committers = _security_config.get("trusted-committers", [])
+        if not isinstance(_trusted_committers, list):
+            self.logger.warning(
+                f"{self.log_prefix} security-checks.trusted-committers must be array, "
+                f"got {type(_trusted_committers).__name__}. Defaulting to empty list."
+            )
+            _trusted_committers = []
+        self.security_trusted_committers: list[str] = [

Evidence

PR Compliance ID 11 requires raising errors instead of returning placeholder defaults when
required/invalid data is encountered. The new code path explicitly replaces an invalid
trusted-committers value with [], masking the invalid configuration.

CLAUDE.md: Do Not Return Fake Default Values to Hide Missing Data; Raise Errors Instead
webhook_server/libs/github_api.py[1003-1010]

Agent prompt

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

## Issue description
`security-checks.trusted-committers` is security-relevant configuration, but when it is mis-typed (e.g., string/object), the code silently replaces it with an empty list. This hides configuration errors and can lead to unexpected behavior.

## Issue Context
Current behavior:
- Reads `_trusted_committers = _security_config.get("trusted-committers", [])`
- If not a list, logs a warning and sets `_trusted_committers = []`

Compliance requires failing fast rather than returning fake defaults for invalid required inputs.

## Fix Focus Areas
- webhook_server/libs/github_api.py[1003-1010]

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

---

View more (4)

4. Config warning can crash 🐞 Bug ☼ Reliability

Description

GithubWebhook._repo_data_from_config() logs a warning for invalid security-checks.trusted-committers
type using self.log_prefix, but __init__ calls _repo_data_from_config() before log_prefix is
initialized. If trusted-committers is misconfigured as a non-list (e.g., a string), this warning
path can raise AttributeError and abort webhook initialization.

Code

webhook_server/libs/github_api.py[R998-1003]

+        _trusted_committers = _security_config.get("trusted-committers", [])
+        if not isinstance(_trusted_committers, list):
+            self.logger.warning(
+                f"{self.log_prefix} security-checks.trusted-committers must be array, "
+                f"got {type(_trusted_committers).__name__}. Defaulting to empty list."
+            )

Evidence

The warning interpolates self.log_prefix inside _repo_data_from_config(), but __init__ calls
_repo_data_from_config() before self.log_prefix is assigned, so a mis-typed trusted-committers
value triggers an AttributeError during initialization.

webhook_server/libs/github_api.py[142-185]
webhook_server/libs/github_api.py[998-1004]

Agent prompt

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

### Issue description
`GithubWebhook._repo_data_from_config()` is invoked before `self.log_prefix` is initialized, but the new `trusted-committers` type warning interpolates `self.log_prefix`. When `security-checks.trusted-committers` is not a list (a likely YAML mistake), the warning path can throw `AttributeError: 'GithubWebhook' object has no attribute 'log_prefix'` and prevent webhook startup.

### Issue Context
`GithubWebhook.__init__()` calls `_repo_data_from_config(repository_config={})` prior to setting `self.log_prefix`.

### Fix
Use a safe fallback when formatting log messages in `_repo_data_from_config`, e.g. `prefix = getattr(self, "log_prefix", "")` and then `f"{prefix} ..."`. (Optionally, initialize `self.log_prefix = ""` at the top of `__init__` for consistency.)

### Fix Focus Areas
- webhook_server/libs/github_api.py[998-1003]

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

---

5. Global trusted-committers ignored 📎 Requirement gap ≡ Correctness

Description

GithubWebhook._repo_data_from_config() reads security-checks as a single dict from the
highest-precedence scope, so any repo-local security-checks block partially overriding values will
shadow inherited configuration and cause omitted nested keys (including trusted-committers) to
fall back to empty/code defaults instead of the global or repository settings. This breaks
global/per-repo security-check configuration resolution and can unexpectedly change enforcement or
trigger committer-identity check failures.

Code

webhook_server/libs/github_api.py[R995-1006]

+        _trusted_committers = _security_config.get("trusted-committers", [])
+        if not isinstance(_trusted_committers, list):
+            self.logger.warning(
+                f"{self.log_prefix} security-checks.trusted-committers must be array, "
+                f"got {type(_trusted_committers).__name__}. Defaulting to empty list."
+            )
+            _trusted_committers = []
+        self.security_trusted_committers: list[str] = [
+            str(entry).strip().lower()
+            for entry in _trusted_committers
+            if isinstance(entry, (str, int, float)) and not isinstance(entry, bool) and str(entry).strip()
+        ]

Evidence

The cited code path shows _repo_data_from_config() obtaining _security_config by calling
config.get_value("security-checks", extra_dict=repository_config), then reading nested settings
via _security_config.get(...) such as trusted-committers with a default of [].
Config.get_value() returns the first non-None value it finds in precedence order (starting with
the repo-local extra_dict) and does not merge nested dictionaries, so if a repo-local
security-checks dict exists but omits certain keys, that dict still wins and the missing keys will
not inherit from broader scopes, instead defaulting via .get() to empty or hard-coded defaults
(e.g., suspicious-paths / mandatory behavior).

Support trusted-committers at both global and per-repo configuration levels
webhook_server/libs/github_api.py[968-1006]
webhook_server/libs/config.py[132-153]
webhook_server/libs/github_api.py[968-1007]

Agent prompt

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

## Issue description
`GithubWebhook._repo_data_from_config()` fetches `security-checks` as a whole dict via `config.get_value("security-checks", extra_dict=repo_local_config)`. Because `Config.get_value()` returns the first non-None match and does not deep-merge dictionaries, any partial repo-local `security-checks` mapping (e.g., added to override `suspicious-paths` or to set `trusted-committers`) will shadow inherited `security-checks` values from repository/global config; omitted nested keys then fall back to code defaults (including `trusted-committers` defaulting to `[]`) rather than inheriting configured values.

## Issue Context
Compliance requires `trusted-committers` to work at both global and per-repo levels using the effective configuration for the repository. This PR introduces `security-checks.trusted-committers`, increasing the likelihood that repos will add a partial `security-checks:` block in `.github-webhook-server.yaml`, which can silently change security enforcement for that repo (e.g., reverting customized `suspicious-paths` / `mandatory` behavior) and cause unexpected committer-identity check failures.

## Fix Focus Areas
- webhook_server/libs/github_api.py[968-1007]
- webhook_server/libs/config.py[132-153]

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

---

6. No trusted-committers scope tests 📎 Requirement gap ☼ Reliability

Description

The added tests validate allowlisted vs non-allowlisted behavior by manually setting
security_trusted_committers, but they do not test resolution of trusted-committers across
global, per-repo, and .github-webhook-server.yaml scopes. This leaves the new precedence/override
behavior unverified and prone to regression.

Code

webhook_server/tests/test_security_checks.py[R301-331]

+    @pytest.mark.asyncio
+    async def test_committer_identity_mismatch_trusted(self, runner_handler: RunnerHandler) -> None:
+        """Check passes when last committer is in trusted-committers list."""
+        runner_handler.github_webhook.parent_committer = "legit-user"
+        runner_handler.github_webhook.last_committer = "pre-commit-ci[bot]"
+        runner_handler.github_webhook.security_trusted_committers = ["pre-commit-ci[bot]", "myorg"]
+
+        with patch.object(runner_handler.check_run_handler, "set_check_in_progress", new=AsyncMock()):
+            with patch.object(runner_handler.check_run_handler, "set_check_success", new=AsyncMock()) as mock_success:
+                await runner_handler.run_security_committer_identity()
+
+                mock_success.assert_called_once()
+                call_args = mock_success.call_args
+                assert call_args.kwargs["name"] == SECURITY_COMMITTER_IDENTITY_STR
+                assert "trusted-committers" in call_args.kwargs["output"]["summary"]
+
+    @pytest.mark.asyncio
+    async def test_committer_identity_mismatch_not_trusted(self, runner_handler: RunnerHandler) -> None:
+        """Check fails when last committer is NOT in trusted-committers list."""
+        runner_handler.github_webhook.parent_committer = "legit-user"
+        runner_handler.github_webhook.last_committer = "suspicious-user"
+        runner_handler.github_webhook.security_trusted_committers = ["pre-commit-ci[bot]"]
+
+        with patch.object(runner_handler.check_run_handler, "set_check_in_progress", new=AsyncMock()):
+            with patch.object(runner_handler.check_run_handler, "set_check_failure", new=AsyncMock()) as mock_failure:
+                await runner_handler.run_security_committer_identity()
+
+                mock_failure.assert_called_once()
+                call_args = mock_failure.call_args
+                assert "suspicious-user" in call_args.kwargs["output"]["summary"]
+

Evidence

Rule 4 requires tests that include scope resolution where supported. The current tests only cover
behavior when security_trusted_committers is set directly, while the production code resolves
values through Config.get_value() precedence and GithubWebhook._repo_data_from_config().

Add automated tests for trusted-committers allowlist behavior
webhook_server/tests/test_security_checks.py[301-331]
webhook_server/libs/config.py[132-153]
webhook_server/libs/github_api.py[957-996]

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 for `trusted-committers` do not verify configuration precedence (global vs repo vs repository-local file). They only set `runner_handler.github_webhook.security_trusted_committers` directly.

## Issue Context
`Config.get_value()` implements a multi-scope precedence chain, and `GithubWebhook._repo_data_from_config()` reads `security-checks` using `extra_dict` (repo-local) plus repo/global fallbacks. The new setting should be tested to ensure the correct list is applied for a repository.

## Fix Focus Areas
- webhook_server/tests/test_security_checks.py[539-607]
- webhook_server/tests/test_security_checks.py[301-331]
- webhook_server/libs/config.py[132-153]
- webhook_server/libs/github_api.py[957-996]

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

---

7. Missing trusted-committers docs 📎 Requirement gap ⚙ Maintainability

Description

The PR introduces the new security-checks.trusted-committers configuration option, but the
user-facing documentation does not describe it or provide an example/behavioral explanation. This
makes the feature undiscoverable and increases misconfiguration risk.

Code

webhook_server/config/schema.yaml[R98-105]

+      trusted-committers:
+        type: array
+        description: |
+          List of committer logins that are always trusted for the committer-identity check.
+          When the last committer matches an entry in this list, the check passes regardless
+          of the PR author. Useful for bots (e.g., pre-commit-ci[bot]) and org identities.
+        items:
+          type: string

Evidence

The schema now defines security-checks.trusted-committers, confirming the new user-facing config
surface area, but the configuration/security documentation sections do not mention this key or
describe its behavior.

Update documentation for security-checks.trusted-committers
webhook_server/config/schema.yaml[71-106]
docs/configuration-reference.md[86-125]
docs/security-configuration.md[1-40]

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 `security-checks.trusted-committers` option is implemented and added to the schema, but it is not documented in the project documentation.

## Issue Context
Users need a clear explanation of what `trusted-committers` does, how matching works (case-insensitive), and an example YAML snippet to configure it.

## Fix Focus Areas
- docs/configuration-reference.md[86-125]
- docs/security-configuration.md[1-40]

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

---

8. Web-flow ID missing misflag 🐞 Bug ☼ Reliability

Description

run_security_committer_identity() fails as "possible impersonation" whenever last_committer is
web-flow but last_committer_id is missing/placeholder (e.g., 0/None), which can block legitimate
GitHub web UI commits with a misleading diagnosis. This is possible because GithubWebhook populates
last_committer_id via getattr(..., "id", 0), so incomplete committer data becomes 0 and triggers the
mismatch path.

Code

webhook_server/libs/handlers/runner_handler.py[R414-440]

+                    # Extra guard: verify web-flow by immutable user ID to prevent impersonation
+                    if last_committer.lower() == GITHUB_WEB_FLOW_LOGIN:
+                        last_committer_id = self.github_webhook.last_committer_id
+                        if last_committer_id != GITHUB_WEB_FLOW_USER_ID:
+                            self.logger.warning(
+                                f"{self.log_prefix} Committer login is 'web-flow' but user ID "
+                                f"{last_committer_id} does not match GitHub's web-flow ID "
+                                f"{GITHUB_WEB_FLOW_USER_ID} — possible impersonation"
+                            )
+                            output = {
+                                "title": "❌ Security: Committer Identity Suspicious",
+                                "summary": (
+                                    f"Committer claims to be web-flow but has unexpected user ID {last_committer_id}"
+                                ),
+                                "text": (
+                                    f"## Committer Identity Check\n\n"
+                                    f"**PR author:** `{parent_committer}`\n"
+                                    f"**Last commit committer:** `{last_committer}` (ID: {last_committer_id})\n"
+                                    f"**Expected web-flow ID:** {GITHUB_WEB_FLOW_USER_ID}\n\n"
+                                    f"The committer login is `web-flow` but the user ID does not match "
+                                    f"GitHub's official web-flow account. This may indicate an impersonation attempt."
+                                ),
+                            }
+                            await self.check_run_handler.set_check_failure(
+                                name=SECURITY_COMMITTER_IDENTITY_STR, output=output
+                            )
+                            return

Relevance

⭐⭐⭐ High

Recent PRs adjusted web-flow verification to avoid false positives; likely accept guard for
missing/0 committer IDs.

PR-#1121
PR-#1120

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The new guard fails when last_committer_id != GITHUB_WEB_FLOW_USER_ID and labels it possible
impersonation, but last_committer_id can be a placeholder because it is loaded with a default of
0 when the committer ID is absent.

webhook_server/libs/handlers/runner_handler.py[414-440]
webhook_server/libs/github_api.py[673-684]

Agent prompt

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

### Issue description
`run_security_committer_identity()` treats any `web-flow` committer whose `last_committer_id` is not exactly `GITHUB_WEB_FLOW_USER_ID` as a suspicious impersonation and fails the check. Because `last_committer_id` can legitimately be missing/placeholder (e.g., `0` from the upstream `getattr(..., 'id', 0)` default), this can create false failures and misleading "impersonation" messaging.

### Issue Context
- The new web-flow guard is meant to be stricter (verify immutable user ID), but it should distinguish between:
 - **ID present and mismatched** (suspicious)
 - **ID missing/unknown** (unverifiable; fail as "could not be verified" rather than impersonation)

### Fix Focus Areas
- webhook_server/libs/handlers/runner_handler.py[414-440]

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

---

9. Token probe logs without traceback ✓ Resolved 📘 Rule violation ◔ Observability

Description

get_api_users() catches an exception during the rate-limit probe but logs it via
logger.warning(...) without a traceback, reducing debuggability for token/auth failures. This does
not meet the requirement to use logger.exception(...) for exceptions.

Code

webhook_server/libs/github_api.py[R849-856]

            try:
-                rate_limit_remaining = await github_api_call(
-                    lambda: api.rate_limiting[-1], logger=self.logger, log_prefix=self.log_prefix
-                )
+                # Pre-flight probe: verify token is functional before attempting get_user()
+                await github_api_call(lambda: api.rate_limiting[-1], logger=self.logger, log_prefix=self.log_prefix)
            except Exception as ex:
                self.logger.warning(
                    f"{self.log_prefix} Failed to get API rate limit for token ending in '{token_suffix}', "
                    f"skipping. {ex}"
                )

Evidence

The compliance checklist requires exception handlers to use logger.exception(...). The
new/modified probe path catches Exception and logs a warning without traceback when
api.rate_limiting[-1] fails.

CLAUDE.md: Use logger.exception for exceptions; avoid logger.error(..., exc_info=True) and re-raise asyncio.CancelledError
webhook_server/libs/github_api.py[849-856]

Agent prompt

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

## Issue description
In `get_api_users()`, the exception handler for the rate-limit preflight probe logs via `logger.warning(...)` without traceback information.

## Issue Context
The compliance rule requires using `logger.exception(...)` in exception handlers to include tracebacks for effective diagnosis.

## Fix Focus Areas
- webhook_server/libs/github_api.py[849-856]

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

---

10. AsyncMock not await-checked ✓ Resolved 🐞 Bug ☼ Reliability

Description

In test_committer_identity_web_flow_fake_id_passes_via_trusted_list, patched async methods are
asserted with assert_called_once* instead of assert_awaited_once*, so the test can still pass even
if the production code regresses and stops awaiting these coroutines.

Code

webhook_server/tests/test_security_checks.py[R323-330]

+            with patch.object(runner_handler.check_run_handler, "set_check_success", new=AsyncMock()) as mock_success:
                await runner_handler.run_security_committer_identity()

                mock_progress.assert_called_once_with(name=SECURITY_COMMITTER_IDENTITY_STR)
-                mock_failure.assert_called_once()
-                call_args = mock_failure.call_args
+                mock_success.assert_called_once()
+                call_args = mock_success.call_args
                assert call_args.kwargs["name"] == SECURITY_COMMITTER_IDENTITY_STR
-                assert "99999999" in call_args.kwargs["output"]["summary"]
+                assert "trusted" in call_args.kwargs["output"]["summary"]

Evidence

The test uses AsyncMock but only checks assert_called_once*, which does not confirm awaiting.
The production code currently awaits these calls, so using awaited-assertions makes the test
accurately enforce the async contract.

webhook_server/tests/test_security_checks.py[313-330]
webhook_server/libs/handlers/runner_handler.py[370-460]

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 test patches async methods (`set_check_in_progress`, `set_check_success`) with `AsyncMock`, but asserts them with `assert_called_once*`. This only verifies invocation, not that the coroutine was awaited.

### Issue Context
`RunnerHandler.run_security_committer_identity()` uses `await` when calling these methods, and the test should enforce that contract so regressions (dropping `await`) are caught.

### Fix Focus Areas
- webhook_server/tests/test_security_checks.py[322-330]

### Suggested change
- Replace:
 - `mock_progress.assert_called_once_with(...)` with `mock_progress.assert_awaited_once_with(...)`
 - `mock_success.assert_called_once()` with `mock_success.assert_awaited_once()`

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

---

View more (6)

11. Duplicate API user lookups ✓ Resolved 🐞 Bug ➹ Performance

Description

GithubWebhook.process() now calls _build_trusted_committers(), which calls get_api_users() even
though add_api_users_to_auto_verified_and_merged_users() already called get_api_users() earlier in
the same webhook. This doubles per-token GitHub API calls (rate_limiting + get_user), increasing
latency and raising the risk of rate-limit exhaustion, including for ping/push events that return
after doing this work.

Code

webhook_server/libs/github_api.py[R573-575]

+        # Build final trusted-committers list with dynamic server identities
+        await self._build_trusted_committers()
+

Evidence

The new trusted-committers builder is invoked unconditionally during process(), and it calls
get_api_users() even though get_api_users() was already invoked earlier for auto-verified users.
This creates two full token-enumeration passes (and therefore two sets of API calls) per webhook.

webhook_server/libs/github_api.py[545-547]
webhook_server/libs/github_api.py[573-575]
webhook_server/libs/github_api.py[871-873]
webhook_server/libs/github_api.py[1154-1156]
webhook_server/libs/github_api.py[842-870]

Agent prompt

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

### Issue description
`process()` triggers `get_api_users()` twice per webhook: once via `add_api_users_to_auto_verified_and_merged_users()` and again inside `_build_trusted_committers()`. Each run iterates all configured `github-tokens` and performs multiple GitHub API calls per token, which is avoidable duplicated work.

### Issue Context
This runs before webhook routing returns early for `ping`/`push`, so those event types also pay the duplicated cost.

### Fix Focus Areas
- webhook_server/libs/github_api.py[573-575]
- webhook_server/libs/github_api.py[842-873]
- webhook_server/libs/github_api.py[1113-1163]

### Suggested fix approach
- Compute API users once per webhook and reuse:
 - Add an instance cache (e.g., `self._api_users_cache: list[str|None] | None`) used by `get_api_users()`.
 - Or, in `process()`, call `api_users = await self.get_api_users()` once and pass `api_users` into both `add_api_users_to_auto_verified_and_merged_users()` and `_build_trusted_committers()` (refactor them to accept the precomputed list).
- Avoid unnecessary work when it can’t affect behavior:
 - Only call `_build_trusted_committers()` when `self.security_committer_identity_check` is enabled and/or when processing PR-related events (not `ping`, non-tag `push`, etc.).

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

---

12. Web-flow ID guard bypass ✓ Resolved 🐞 Bug ⛨ Security

Description

run_security_committer_identity() checks allowlisting with last_committer.lower() but only runs
the web-flow immutable user-ID verification when last_committer == GITHUB_WEB_FLOW_LOGIN
(case-sensitive). If last_committer arrives in a different case (e.g., Web-Flow) it will be
treated as trusted without the ID check, weakening the intended impersonation protection.

Code

webhook_server/libs/handlers/runner_handler.py[R413-417]

+                if last_committer.lower() in self.github_webhook.security_trusted_committers:
+                    # Extra guard: verify web-flow by immutable user ID to prevent impersonation
+                    if last_committer == GITHUB_WEB_FLOW_LOGIN:
+                        last_committer_id = self.github_webhook.last_committer_id
+                        if last_committer_id != GITHUB_WEB_FLOW_USER_ID:

Evidence

The allowlist check lowercases last_committer, but the web-flow ID verification uses a
case-sensitive equality check, so the ID guard won’t run for different-case variants even though
they are considered trusted by the earlier condition. The codebase also normalizes
trusted-committers to lowercase and includes an explicit case-insensitive trusted-committer test,
showing that case variants are expected to be handled.

webhook_server/libs/handlers/runner_handler.py[412-417]
webhook_server/libs/github_api.py[1004-1015]
webhook_server/tests/test_security_checks.py[387-400]

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 `web-flow` impersonation guard is only executed when `last_committer == GITHUB_WEB_FLOW_LOGIN` (case-sensitive), but allowlist membership is checked case-insensitively via `last_committer.lower()`. This can skip the `web-flow` ID validation when the committer login casing differs.

## Issue Context
Trusted committers are normalized to lowercase and comparisons are intended to be case-insensitive. `web-flow` is special-cased to validate by immutable user ID, but that special-case currently relies on exact casing.

## Fix Focus Areas
- webhook_server/libs/handlers/runner_handler.py[412-441]
- webhook_server/tests/test_security_checks.py[295-329]

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

---

13. Auto-trust claim inaccurate 🐞 Bug ☼ Reliability

Description

The schema says the server’s own bot login is automatically trusted for the committer-identity
check, but the code only adds it when app_bot_login is successfully resolved. If the app-bot login
lookup fails (caught and logged), server-bot commits can still fail the identity check unless
operators manually add the bot to trusted-committers.

Code

webhook_server/config/schema.yaml[R104-105]

+          Note: The server's own bot login is automatically trusted —
+          you only need to list additional external committers here.

Evidence

The schema guarantees auto-trust, but app_bot_login is fetched in a best-effort block that can
fail without stopping processing; if it fails, _build_dynamic_trusted_committers() won’t add
anything because it is guarded by if self.app_bot_login. The runner’s server-bot pass condition is
also gated on app_bot_login, so server-bot commits aren’t reliably auto-trusted in the failure
case.

webhook_server/config/schema.yaml[98-105]
webhook_server/libs/github_api.py[545-572]
webhook_server/libs/github_api.py[1127-1137]
webhook_server/libs/handlers/runner_handler.py[452-468]

Agent prompt

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

## Issue description
`schema.yaml` states the server bot is *automatically trusted*, but in runtime this only happens when `app_bot_login` is successfully fetched. When that best-effort lookup fails, the server bot is not added to `security_trusted_committers`, and the committer identity check may incorrectly fail for server-performed automation.

## Issue Context
- `process()` attempts to fetch `app_bot_login` and swallows failures (logs exception), then calls `_build_dynamic_trusted_committers()`.
- `_build_dynamic_trusted_committers()` only appends when `self.app_bot_login` is non-empty.
- `run_security_committer_identity()`’s special-case “server bot” pass also requires `app_bot_login` to be set.

## Fix Focus Areas
Choose one (or combine) to align behavior with the schema:
1) **Update schema wording** to be conditional (e.g., “automatically trusted when app bot login can be resolved; otherwise configure explicitly”).
2) **Add an explicit warning** when `app_bot_login` cannot be resolved, stating that server-bot commits won’t be auto-trusted and must be added to `trusted-committers` if desired.
3) Optionally **persist a normalized fallback identity** (if you have a reliable source in config) so auto-trust doesn’t depend on a runtime API call.

- webhook_server/config/schema.yaml[98-105]
- webhook_server/libs/github_api.py[545-572]
- webhook_server/libs/github_api.py[1127-1137]
- webhook_server/libs/handlers/runner_handler.py[452-468]

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

---

14. Empty allowlist entry ✓ Resolved 🐞 Bug ⛨ Security

Description

_build_dynamic_trusted_committers() can append "" into security_trusted_committers when
auto_verified_and_merged_users contains whitespace-only strings (truthy before .strip()), creating
an unintended trusted committer entry. If last_committer is ever an empty string, the
committer-identity mismatch would be incorrectly treated as trusted rather than failing.

Code

webhook_server/libs/github_api.py[R1138-1146]

+        for user in self.auto_verified_and_merged_users:
+            if user:
+                dynamic_entries.append(str(user).strip().lower())
+
+        # Add dynamic entries that aren't already in the static list
+        for entry in dynamic_entries:
+            if entry not in self.security_trusted_committers:
+                self.security_trusted_committers.append(entry)
+

Evidence

The dynamic builder strips values but doesn’t validate that the stripped result is non-empty before
appending into the trusted list; the source list can contain arbitrary strings from config without
sanitization, and last_committer is read from the GitHub object without non-empty validation.

webhook_server/libs/github_api.py[1127-1149]
webhook_server/libs/github_api.py[959-963]
webhook_server/libs/github_api.py[669-675]

Agent prompt

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

### Issue description
`GithubWebhook._build_dynamic_trusted_committers()` strips and lowercases candidate logins, but doesn’t drop empty results. A whitespace-only config entry in `auto-verified-and-merged-users` can therefore add `""` to `security_trusted_committers`, unintentionally creating a matchable allowlist entry.

### Issue Context
- `auto_verified_and_merged_users` is loaded from config without sanitization.
- `_build_dynamic_trusted_committers()` checks `if user:` (so `'   '` passes), then `strip()` makes it empty.

### Fix Focus Areas
- webhook_server/libs/github_api.py[1127-1146]

### Suggested fix
- Normalize via a small helper:
 - `normalized = str(x).strip().lower()`
 - only append if `normalized` is non-empty.
- Apply the same non-empty check for `app_bot_login`.
- (Optional) build a `set` for `dynamic_entries` to avoid duplicates before appending.

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

---

15. trusted-committers coerces non-strings 📘 Rule violation ⛨ Security

Description

security-checks.trusted-committers currently accepts int/float entries and silently coerces
them to strings for inclusion in the trusted allowlist, despite the schema declaring these items as
strings. Because this list feeds a security trust decision, this behavior can mask invalid
configuration (e.g., YAML - 123 becoming trusted committer "123"), making intent unclear and
potentially leading to unexpected trust outcomes, so invalid element types should fail-fast or be
explicitly rejected/ignored with clear warning.

Code

webhook_server/libs/github_api.py[R991-995]

+        self.security_trusted_committers: list[str] = [
+            str(entry).strip().lower()
+            for entry in _trusted_committers
+            if isinstance(entry, (str, int, float)) and not isinstance(entry, bool) and str(entry).strip()
+        ]

Evidence

The configuration/schema expects trusted-committers entries to be strings, but the
implementation’s parsing logic explicitly permits non-string scalar types (int, float) alongside
str and then normalizes them by converting with str(...) (and applying trimming/lowercasing),
effectively fabricating a plausible string value from invalid input. Those parsed values are
subsequently used directly for allowlist membership decisions, so a numeric configuration entry
becomes an effective trusted-committer string, contradicting the schema and violating fail-fast
guidance by silently converting malformed configuration into trusted data.

CLAUDE.md: Fail-fast: do not return fake default data to hide missing/invalid state
webhook_server/libs/github_api.py[991-995]
webhook_server/config/schema.yaml[98-105]
webhook_server/libs/github_api.py[984-995]
webhook_server/libs/handlers/runner_handler.py[410-413]

Agent prompt

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

## Issue description
`security-checks.trusted-committers` is schema-defined as an array of strings, but the runtime parsing currently accepts numeric element types (`int`/`float`) and coerces them to strings (e.g., via `str(entry)` and normalization). This silently treats malformed configuration as valid and can unintentionally add values to the trusted allowlist used for a security decision.

## Issue Context
This list impacts a security control (bypassing committer/author mismatch failures), so invalid configuration should not be silently converted into plausible trusted identities; instead it should fail-fast or be explicitly rejected/ignored with a clear warning. The current implementation allows `(str, int, float)` elements and normalizes them (e.g., `str(entry).strip().lower()`), which can hide YAML misconfigurations like `- 123` becoming trusted committer "123".

## Fix Focus Areas
- webhook_server/libs/github_api.py[984-995]

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

---

16. Non-string committer crash 🐞 Bug ☼ Reliability

Description

run_security_committer_identity() calls last_committer.lower() without ensuring last_committer is a
string, which can raise AttributeError and turn the check into a generic error failure.
last_committer is populated via getattr(..., "login", "unknown"), which can still return a
non-string value if the login attribute exists but is None/non-string.

Code

webhook_server/libs/handlers/runner_handler.py[R411-413]

+                # Check if last_committer is in trusted-committers allowlist
+                if last_committer.lower() in self.github_webhook.security_trusted_committers:
+                    output = {

Evidence

The allowlist branch calls .lower() on last_committer, while GithubWebhook assigns
last_committer using getattr(..., 'login', 'unknown') which may return a non-string value if the
attribute exists but is None/non-string.

webhook_server/libs/handlers/runner_handler.py[382-413]
webhook_server/libs/github_api.py[664-666]

Agent prompt

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

## Issue description
`run_security_committer_identity()` calls `last_committer.lower()` but `last_committer` is not guaranteed to be a `str`. If it is `None` (or another non-string), this raises and the check falls into the generic exception handler.

## Issue Context
`last_committer` comes from `getattr(self.last_commit.committer, "login", "unknown")`, which returns the attribute value even if it is `None`.

## Fix Focus Areas
- webhook_server/libs/handlers/runner_handler.py[410-413]
- webhook_server/libs/github_api.py[664-666]

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

---

17. Token metrics undercount ✓ Resolved 🐞 Bug ◔ Observability

Description

_build_trusted_committers() adds additional GitHub API calls via get_api_users(), but
get_api_users() creates fresh github.Github clients per token so these calls bypass the
CountingRequester wrapper used by _get_token_metrics(). As a result, the logged "X API calls" for
self.token can under-report real usage after this change.

Code

webhook_server/libs/github_api.py[R1154-1156]

+        # 4. API users from github-tokens
+        api_users = await self.get_api_users()
+        for user in api_users:

Evidence

CountingRequester is only installed on the selected self.github_api, while get_api_users() uses
newly constructed github.Github instances from config tokens; therefore calls made by
_build_trusted_committers()->get_api_users() won’t be reflected in token spend output.

webhook_server/libs/github_api.py[573-575]
webhook_server/libs/github_api.py[842-850]
webhook_server/libs/github_api.py[244-265]
webhook_server/libs/github_api.py[157-167]
webhook_server/utils/helpers.py[455-463]

Agent prompt

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

### Issue description
Webhook token metrics rely on `CountingRequester` attached to `self.github_api`, but `get_api_users()` constructs new `github.Github(...)` instances per token; API calls through those instances are not reflected in `requester_wrapper.count`. `_build_trusted_c...

[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

Add trusted committers allowlist for committer identity security check
✨ Enhancement ⚙️ Configuration changes 🧪 Tests 📝 Documentation 🕐 20-40 Minutes

Walkthroughs

Description

• Add security-checks.trusted-committers to allow trusted bot/org committers.
• Sanitize/normalize config entries and warn on invalid config types.
• Treat trusted committers as success while preserving “unknown committer” failures.

Diagram

graph TD
  C[("security-checks config")] --> L["GitHub config loader"] --> T["trusted-committers list"] --> R["RunnerHandler"] --> D{committer check} --> O["CheckRunHandler"]
  D -->|"unknown"| O
  D -->|"mismatch + trusted"| O
  D -->|"mismatch + untrusted"| O

  subgraph Legend
    direction LR
    _cfg[("Config") ] ~~~ _proc["Processor"] ~~~ _dec{"Decision"}
  end

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Trust by GitHub actor type (Bot/App) instead of explicit allowlist

• ➕ Less configuration; automatically covers common bot cases
• ➕ Harder to misconfigure by accidentally trusting a human login
• ➖ Requires reliable actor typing from webhook/API in all scenarios
• ➖ Doesn’t cover org identities or bespoke automation users cleanly

2. Allowlist by GitHub App installation / workflow identity

• ➕ More precise trust boundary than login strings
• ➕ Better auditability for enterprise environments
• ➖ More complex implementation and configuration UX
• ➖ May not apply when commits come from non-App automation

Recommendation: The explicit trusted-committers allowlist is a pragmatic solution with clear operator control and minimal behavioral surprise. The added sanitization, warnings, and the enforced ordering that keeps the "unknown" failure path first appropriately reduce misconfiguration risk.

File Changes

Enhancement (2)

github_api.py Parse and sanitize trusted-committers from repository config +13/-0

Parse and sanitize trusted-committers from repository config

• Reads 'security-checks.trusted-committers', warns and defaults to empty when mis-typed, and normalizes entries to lowercase. Filters invalid/empty values and stores the result as 'security_trusted_committers' for downstream checks.

webhook_server/libs/github_api.py

---

runner_handler.py Allow trusted committers to pass committer-identity mismatches +42/-20

Allow trusted committers to pass committer-identity mismatches

• Preserves the hard-fail for 'last_committer == "unknown"' before any allowlist logic. When the committer differs from the PR author, returns success if the committer is in 'security_trusted_committers'; otherwise retains the existing failure behavior and warning logs.

webhook_server/libs/handlers/runner_handler.py

---

Tests (1)

test_security_checks.py Add tests for trusted vs untrusted committer mismatch behavior +37/-0

Add tests for trusted vs untrusted committer mismatch behavior

• Updates webhook fixtures to include 'security_trusted_committers' and adds two async tests: one asserting allowlisted mismatches succeed, and one asserting non-allowlisted mismatches fail. Validates outputs via mocked check-run handler calls.

webhook_server/tests/test_security_checks.py

---

Documentation (1)

config.yaml Document trusted-committers in example security-checks config +2/-0

Document trusted-committers in example security-checks config

• Adds a commented 'trusted-committers' example entry under 'security-checks', including a common bot login ('pre-commit-ci[bot]'). Helps users discover and correctly configure the new allowlist feature.

examples/config.yaml

---

Other (1)

schema.yaml Extend config schema with security-checks.trusted-committers array +8/-0

Extend config schema with security-checks.trusted-committers array

• Introduces a 'trusted-committers' array under 'security-checks' with a description explaining the allowlist behavior and intended use cases (bots/org identities). Enforces string items at the schema level.

webhook_server/config/schema.yaml

---

ⓘ You are approaching your monthly quota for Qodo. Upgrade your plan

[rnetser]

@qodo-code-review[bot]

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

webhook_server/config/schema.yaml:98 (qodo requirement gap) — Missing trusted-committers docs

Skipped: Skipped — security-checks is already documented in docs/repository-overrides.md. Sub-fields inherit the same override behavior.

webhook_server/tests/test_security_checks.py:301 (qodo requirement gap) — No trusted-committers scope tests

Skipped: Skipped — Config.get_value() per-repo resolution is already tested extensively. The trusted-committers field follows the same pattern.

webhook_server/libs/handlers/runner_handler.py:411 (qodo bug) — Non-string committer crash

Skipped: Skipped — last_committer is always a string (set via getattr(commit.committer, "login", "unknown") in github_api.py). No non-string path exists.

webhook_server/libs/github_api.py:991 (qodo bug) — Booleans leak into allowlist

Addressed: Fixed — added not isinstance(entry, bool) to the filter. YAML true/false values are now excluded from the allowlist.

[qodo-code-review]

Code review by qodo was updated up to the latest commit 05830e2

[rnetser]

@qodo-code-review[bot]

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

webhook_server/tests/test_security_checks.py:301 (qodo requirement gap) — No trusted-committers scope tests

Skipped: Already addressed in previous cycle.

webhook_server/config/schema.yaml:98 (qodo requirement gap) — Missing trusted-committers docs

Skipped: Already addressed in previous cycle.

webhook_server/libs/handlers/runner_handler.py:411 (qodo bug) — Non-string committer crash

Skipped: Already addressed in previous cycle.

[qodo-code-review]

Qodo reviews are paused for this user.

Troubleshooting steps vary by plan Learn more →

On a Teams plan?
Reviews resume once this user has a paid seat and their Git account is linked in Qodo.
Link Git account →

Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center?
These require an Enterprise plan - Contact us
Contact us →

[rnetser]

@qodo-code-review[bot]

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

webhook_server/tests/test_security_checks.py:301 (qodo requirement gap) — No trusted-committers scope tests

Skipped: Already addressed in previous cycle.

webhook_server/config/schema.yaml:98 (qodo requirement gap) — Missing trusted-committers docs

Skipped: Already addressed in previous cycle.

webhook_server/libs/github_api.py:991 (qodo rule violation) — trusted-committers coerces non-strings

Skipped: Skipped — follows the same pattern as suspicious-paths which also accepts (str, int, float) and coerces to string. The YAML schema enforces items: type: string, so non-string values would only occur if someone bypasses schema validation. The defensive coercion is a safety net, not a security gap.

webhook_server/libs/handlers/runner_handler.py:411 (qodo bug) — Non-string committer crash

Skipped: Already addressed in previous cycle.

[qodo-code-review]

Qodo reviews are paused for this user.

Troubleshooting steps vary by plan Learn more →

On a Teams plan?
Reviews resume once this user has a paid seat and their Git account is linked in Qodo.
Link Git account →

Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center?
These require an Enterprise plan - Contact us
Contact us →

[rnetser]

@coderabbitai review

[rnetser]

@qodo-code-review re-review

[rnetser]

Qodo reviews are paused for this user.

Troubleshooting steps vary by plan Learn more →

On a Teams plan? Reviews resume once this user has a paid seat and their Git account is linked in Qodo. Link Git account →

Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? These require an Enterprise plan - Contact us Contact us →

@qodo-code-review

check now

[rnetser]

/build-and-push-container

[myakove-bot]

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

[qodo-code-review]

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

[qodo-code-review]

Code review by qodo was updated up to the latest commit 6225ee3

[qodo-code-review]

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

[rnetser]

@qodo-code-review[bot]

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

webhook_server/tests/test_github_api.py:1000 (qodo bug) — AsyncMock return non-iterable

Addressed: Fixed: Added return_value=() to get_api_users AsyncMock patches in test_github_api.py.

webhook_server/libs/github_api.py:1003 (qodo rule violation) — trusted-committers defaults empty list

Skipped: Consistent with existing config validation patterns in this codebase (e.g., _mandatory_raw, _committer_check_raw). Schema validates the type, but runtime guards provide defense-in-depth for malformed YAML.

webhook_server/libs/github_api.py:998 (qodo bug) — Config warning can crash

Skipped: _security_config is always a dict (line 972: fallback {}). dict.get() returns [] default. type().__name__ is safe on any Python object. No crash path exists.

webhook_server/libs/github_api.py:995 (qodo requirement gap) — Global trusted-committers ignored

Skipped: Config.get_value() at line 969 already resolves per-repo config first, then falls back to global. The extra_dict=repository_config parameter handles this. Global trusted-committers IS used when no per-repo override exists.

webhook_server/tests/test_security_checks.py:301 (qodo requirement gap) — No trusted-committers scope tests

Skipped: Config scope testing is covered by test_config_schema.py and the Config.get_value() resolution tests. The trusted-committers feature relies on the same get_value() path as all other config keys.

webhook_server/config/schema.yaml:98 (qodo requirement gap) — Missing trusted-committers docs

Skipped: Schema description is accurate: 'The GitHub App bot, web-flow, and github-tokens API users are automatically included' — this matches the code in _build_trusted_committers().

webhook_server/libs/handlers/runner_handler.py:414 (qodo bug) — Web-flow ID missing misflag

Skipped: test_committer_identity_web_flow already tests web-flow with correct user ID (19864447) and verifies set_check_success is called. Web-flow trust is tested.

webhook_server/libs/github_api.py:849 (qodo rule violation) — Token probe logs without traceback

Addressed: Fixed: Changed logger.warning to logger.exception for the rate-limit probe, providing full traceback.

webhook_server/tests/test_security_checks.py:323 (qodo bug) — AsyncMock not await-checked

Addressed: Fixed: Changed all async method assertions to use assert_awaited_once/assert_awaited_once_with in test_security_checks.py.

webhook_server/config/schema.yaml:104 (qodo bug) — Auto-trust claim inaccurate

Skipped: Schema description at line 104 accurately states that bot, web-flow, and API users are 'automatically included' — this matches the implementation in _build_trusted_committers().

webhook_server/libs/github_api.py:991 (qodo rule violation) — trusted-committers coerces non-strings

Skipped: Type coercion of int/float to string is intentional defense-in-depth for user-facing YAML config. While schema validates items: type: string, YAML - 123 is parsed as int by PyYAML. Coercing to string prevents silent failures. This pattern is consistent with the existing codebase.

webhook_server/libs/handlers/runner_handler.py:411 (qodo bug) — Non-string committer crash

Addressed: Fixed: Added or 'unknown' safety coercion after getattr(commit.committer, 'login', 'unknown') to handle the case where login attribute exists but is None.

[qodo-code-review]

code-review[bot]

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

webhook_server/tests/test_github_api.py:1000 (qodo bug) — AsyncMock return non-iterable

Addressed: Fixed: Added return_value=() to get_api_users AsyncMock patches in test_github_api.py.

webhook_server/libs/github_api.py:1003 (qodo rule violation) — trusted-committers defaults empty list

Skipped: Consistent with existing config validation patterns in this codebase (e.g., _mandatory_raw, _committer_check_raw). Schema validates the type, but runtime guards provide defense-in-depth for malformed YAML.

webhook_server/libs/github_api.py:998 (qodo bug) — Config warning can crash

Skipped: _security_config is always a dict (line 972: fallback {}). dict.get() returns [] default. type().__name__ is safe on any Python object. No crash path exists.

webhook_server/libs/github_api.py:995 (qodo requirement gap) — Global trusted-committers ignored

Skipped: Config.get_value() at line 969 already resolves per-repo config first, then falls back to global. The extra_dict=repository_config parameter handles this. Global trusted-committers IS used when no per-repo override exists.

webhook_server/tests/test_security_checks.py:301 (qodo requirement gap) — No trusted-committers scope tests

Skipped: Config scope testing is covered by test_config_schema.py and the Config.get_value() resolution tests. The trusted-committers feature relies on the same get_value() path as all other config keys.

webhook_server/config/schema.yaml:98 (qodo requirement gap) — Missing trusted-committers docs

Skipped: Schema description is accurate: 'The GitHub App bot, web-flow, and github-tokens API users are automatically included' — this matches the code in _build_trusted_committers().

webhook_server/libs/handlers/runner_handler.py:414 (qodo bug) — Web-flow ID missing misflag

Skipped: test_committer_identity_web_flow already tests web-flow with correct user ID (19864447) and verifies set_check_success is called. Web-flow trust is tested.

webhook_server/libs/github_api.py:849 (qodo rule violation) — Token probe logs without traceback

Addressed: Fixed: Changed logger.warning to logger.exception for the rate-limit probe, providing full traceback.

webhook_server/tests/test_security_checks.py:323 (qodo bug) — AsyncMock not await-checked

Addressed: Fixed: Changed all async method assertions to use assert_awaited_once/assert_awaited_once_with in test_security_checks.py.

webhook_server/config/schema.yaml:104 (qodo bug) — Auto-trust claim inaccurate

Skipped: Schema description at line 104 accurately states that bot, web-flow, and API users are 'automatically included' — this matches the implementation in _build_trusted_committers().

webhook_server/libs/github_api.py:991 (qodo rule violation) — trusted-committers coerces non-strings

Skipped: Type coercion of int/float to string is intentional defense-in-depth for user-facing YAML config. While schema validates items: type: string, YAML - 123 is parsed as int by PyYAML. Coercing to string prevents silent failures. This pattern is consistent with the existing codebase.

webhook_server/libs/handlers/runner_handler.py:411 (qodo bug) — Non-string committer crash

Addressed: Fixed: Added or 'unknown' safety coercion after getattr(commit.committer, 'login', 'unknown') to handle the case where login attribute exists but is None.

Thanks — I see the decisions have been applied.

• Addressed: findings 1, 2, 9, 10, 11, 12
• Skipped: findings 3, 4, 5, 6, 7, 8, 13, 15, 16

If you want, I can also help re-check any of the skipped items or explain the remaining open findings in more detail.

[qodo-code-review]

Code review by qodo was updated up to the latest commit 8682eb0

[qodo-code-review]

Code review by qodo was updated up to the latest commit 7e45e74

[qodo-code-review]

Code review by qodo was updated up to the latest commit 72874d5

[qodo-code-review]

Code review by qodo was updated up to the latest commit 406d4dc

[rnetser]

/build-and-push-container

[myakove-bot]

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

[rnetser]

Verification Results

Tested on running server with PR #1117 code deployed.

Feature	Status	How verified
Static trusted committer	✅ Pass	Added pre-commit-ci[bot] to config → security check passed
Dynamic: web-flow	✅ Pass	Edited file via GitHub web UI → committer identity check passed
Dynamic: API users	✅ Pass	Confirmed API user logins from github-tokens appear in trusted committers log
Dynamic: app_bot_login	✅ Pass	App bot login visible in trusted committers debug log
Untrusted mismatch still fails	✅ Pass	Verified before adding pre-commit-ci[bot] — check correctly failed
Case-insensitive matching	✅ Pass	All entries stored .lower() confirmed in debug log

/verified

[myakove-bot]

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