Skip to content

feat: PR security checks — suspicious paths, committer identity, auto-merge override#1109

Open
myakove wants to merge 7 commits into
mainfrom
feat/issue-1106-pr-security-checks
Open

feat: PR security checks — suspicious paths, committer identity, auto-merge override#1109
myakove wants to merge 7 commits into
mainfrom
feat/issue-1106-pr-security-checks

Conversation

@myakove

@myakove myakove commented Jun 9, 2026

Copy link
Copy Markdown
Collaborator

PR Summary by Qodo

Add PR security checks for suspicious paths, committer identity, and auto-merge override
✨ Enhancement 🧪 Tests ⚙️ Configuration changes 📝 Documentation 🕐 40+ Minutes

Grey Divider

Walkthroughs

User Description

Summary

Three configurable security checks to detect and block malicious PR attack vectors:

Suspicious Path Detection

  • New security-suspicious-paths check run
  • Configurable path prefixes (default: .claude/, .vscode/, .cursor/, .devcontainer/, .pi/, .github/workflows/, .github/actions/)
  • Fails check when PR modifies files in sensitive locations

Committer Identity Check

  • New security-committer-identity check run
  • Flags when last committer differs from PR author
  • Handles unknown committer identity explicitly

Auto-Merge Override

  • Blocks auto-merge when PR touches suspicious paths
  • Posts comment explaining why auto-merge was blocked

Security Design

  • Config only from server-side config.yaml (not overridable by in-repo .github-webhook-server.yaml)
  • Prevents attackers from weakening security policy via PR

Closes #1106

AI Description
• Add configurable PR security check runs for suspicious paths and committer identity.
• Block auto-merge and comment when PR touches security-sensitive path prefixes.
• Document/validate security-checks config and add full test coverage for new behavior.
Diagram
graph TD
  cfg["server config.yaml"] --> wh(["GitHubWebhook"]) --> prh(["PullRequestHandler"]) --> ofh(["OwnersFileHandler"])
  prh --> rh(["RunnerHandler"]) --> crh(["CheckRunHandler"]) --> gh{{"GitHub API"}}
  prh -. "comment / enable automerge" .-> gh

  subgraph Legend
    direction LR
    _file["Config/File"] ~~~ _svc(["Handler/Service"]) ~~~ _ext{{"External"}}
  end
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Use CODEOWNERS + branch protection for sensitive paths
  • ➕ Native GitHub enforcement; no extra webhook-side logic
  • ➕ Can require reviews for .github/workflows/ and similar paths
  • ➖ CODEOWNERS lives in-repo; attacker PR may attempt to modify it (mitigations require additional protections)
  • ➖ Doesn’t detect committer mismatch vs PR author
  • ➖ Doesn’t provide an explicit auto-merge override message path
2. Required GitHub Actions workflow with path filters
  • ➕ Clear CI signal in GitHub UI; can block merges via required checks
  • ➕ Easy to extend with additional detectors
  • ➖ Workflows are in-repo and are themselves part of the attack surface
  • ➖ May not run as expected depending on repo settings/permissions; weaker central enforcement than server-side config
3. Org policy: require verified commits / signature enforcement
  • ➕ Stronger identity assurance than comparing GitHub usernames alone
  • ➕ Moves trust decision into cryptographic verification
  • ➖ Org-wide operational overhead; may block legitimate contributors
  • ➖ Doesn’t address suspicious-path modifications; still needs path-based review controls

Recommendation: The PR’s approach (server-side, non-overridable security policy + explicit check-runs + auto-merge override) is a good fit for defending against repo-config supply-chain attacks. Consider adding CODEOWNERS/branch protections as defense-in-depth, but keep these webhook checks as the centrally enforced gate.

Grey Divider

File Changes

Enhancement (4)
github_api.py Load server-side security policy and make committer identity explicit +10/-1

Load server-side security policy and make committer identity explicit

• Sets last_committer to "unknown" when no GitHub user is associated with the last commit committer. Loads security-checks only from server configuration (no per-repo extra_dict override) and exposes security_suspicious_paths and security_committer_identity_check on the webhook context.

webhook_server/libs/github_api.py


pull_request_handler.py Queue/run security check-runs and block auto-merge on suspicious paths +63/-0

Queue/run security check-runs and block auto-merge on suspicious paths

• Adds a Security Checks section to the welcome comment, queues and launches two new security check-runs during PR open/sync, and blocks auto-merge with an explanatory PR comment when changed_files match suspicious path prefixes.

webhook_server/libs/handlers/pull_request_handler.py


runner_handler.py Implement suspicious-path and committer-identity security check runners +114/-0

Implement suspicious-path and committer-identity security check runners

• Adds runner methods to evaluate changed files against configured prefixes and to compare PR author vs last committer (including an explicit "unknown" case). Each runner publishes detailed check-run outputs and sets success/failure accordingly.

webhook_server/libs/handlers/runner_handler.py


constants.py Define security check names and default suspicious path prefixes +14/-0

Define security check names and default suspicious path prefixes

• Introduces constants for the two new check-run names, registers them as non-overridable built-in checks, and defines the default suspicious path prefix list used for detection.

webhook_server/utils/constants.py


Tests (2)
test_pull_request_handler.py Extend PR handler tests to account for new security runners +9/-0

Extend PR handler tests to account for new security runners

• Updates the webhook mock with security-related attributes and patches the new runner methods in existing workflow tests. Ensures PR processing test scaffolding stays compatible with the new queued/started tasks.

webhook_server/tests/test_pull_request_handler.py


test_security_checks.py Add dedicated tests for security checks and auto-merge override +459/-0

Add dedicated tests for security checks and auto-merge override

• Adds coverage for suspicious path detection outcomes, committer identity match/mismatch/unknown, and auto-merge blocking behavior (including comment posting). Also asserts security constants and default suspicious path list are wired into BUILTIN_CHECK_NAMES.

webhook_server/tests/test_security_checks.py


Other (2)
config.yaml Document security-checks settings and defaults in example config +14/-0

Document security-checks settings and defaults in example config

• Adds a new security-checks section with default suspicious path prefixes and a committer identity toggle. Provides inline commentary describing the intent of these checks.

examples/config.yaml


schema.yaml Add JSON schema for security-checks configuration +24/-0

Add JSON schema for security-checks configuration

• Introduces a security-checks schema definition (suspicious-paths list and committer-identity-check boolean) and wires it into the root schema. Documents behavioral effects (check-run failure and auto-merge blocking).

webhook_server/config/schema.yaml


Grey Divider

Qodo Logo

…-merge override

- Add security-suspicious-paths check run detecting modifications to sensitive paths
- Add security-committer-identity check run flagging committer mismatches
- Block auto-merge when PRs modify security-sensitive paths
- Handle unknown committer identity (no GitHub user) explicitly
- Security config only from server-side config.yaml (not overridable by in-repo file)
- Default suspicious paths: .claude/, .vscode/, .cursor/, .devcontainer/, .pi/, .github/workflows/, .github/actions/

Closes #1106
@qodo-code-review

qodo-code-review Bot commented Jun 9, 2026

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (4) 📘 Rule violations (0) 📎 Requirement gaps (4)

Context used

Grey Divider


Action required

1. Automerge test mock breaks 🐞 Bug ☼ Reliability
Description
test_automerge_blocked_by_suspicious_paths patches github_api_call with a bare AsyncMock, but
production code awaits it for label fetch and iterates the returned labels to build _label_names.
Under Python 3.13 this mock does not return an iterable of labels, so the test can fail or stop
exercising the real label-bypass logic.
Code

webhook_server/tests/test_security_checks.py[R382-389]

+            # Should have called github_api_call for labels fetch + blocking comment
+            comment_calls = [
+                c
+                for c in mock_api_call.call_args_list
+                if len(c.args) > 1 and isinstance(c.args[1], str) and "Auto-merge blocked" in c.args[1]
+            ]
+            assert len(comment_calls) == 1
+            assert ".github/workflows/ci.yml" in comment_calls[0].args[1]
Evidence
The production path awaits github_api_call(...) and iterates the returned value to derive label
names, while the test currently replaces github_api_call with an AsyncMock without ensuring it
returns a list-like value for the label-fetch call.

webhook_server/libs/handlers/pull_request_handler.py[1268-1274]
webhook_server/tests/test_security_checks.py[374-389]

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 `github_api_call` with `AsyncMock()` but does not configure return values for calls where production expects a list of label objects. The production code iterates over the awaited result (`{label.name for label in _labels}`), which is incompatible with the current mock setup.

## Issue Context
`set_pull_request_automerge()` now fetches labels before posting the blocking comment, so the test must return an iterable for that call.

## Fix Focus Areas
- webhook_server/tests/test_security_checks.py[374-389]
- webhook_server/libs/handlers/pull_request_handler.py[1268-1274]

## Implementation notes
- In the test, patch `github_api_call` with a `side_effect` that:
 - returns `[]` for the labels-fetch lambda call(s)
 - returns `None` for `create_issue_comment` / `disable_automerge` calls
- Alternatively, patch labels fetch separately by letting the lambda execute (don’t patch `github_api_call` globally), and only patch the comment/disable calls.
- Consider tightening the assertion to match the specific call (`func == mock_pull_request.create_issue_comment`) and accept both positional and `body=` forms.

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


2. security-override allows auto-merge 📎 Requirement gap ⛨ Security
Description
When suspicious paths are detected, set_pull_request_automerge() allows auto-merge to proceed if
the security-override label is present, instead of forcing auto_merge=False and disabling any
existing auto-merge. This violates the requirement that PRs touching suspicious paths are never
auto-merged.
Code

webhook_server/libs/handlers/pull_request_handler.py[R1268-1307]

+                # Check if security-override label is present (maintainer bypass)
+                _labels = await github_api_call(
+                    lambda: list(pull_request.labels), logger=self.logger, log_prefix=self.log_prefix
+                )
+                _label_names = {label.name for label in _labels}
+
+                if SECURITY_OVERRIDE_LABEL_STR not in _label_names:
+                    auto_merge = False
+                    files_list = ", ".join(f"`{f}`" for f in suspicious_matches)
+                    self.logger.info(
+                        f"{self.log_prefix} Auto-merge blocked: "
+                        f"PR modifies security-sensitive paths: {suspicious_matches}"
+                    )
+                    await github_api_call(
+                        pull_request.create_issue_comment,
+                        f"Auto-merge blocked: PR modifies security-sensitive paths: {files_list}",
+                        logger=self.logger,
+                        log_prefix=self.log_prefix,
+                    )
+
+                    # Disable already-enabled auto-merge on the PR
+                    if pull_request.raw_data.get("auto_merge"):
+                        try:
+                            self.logger.info(
+                                f"{self.log_prefix} Suspicious paths detected, disabling existing auto-merge"
+                            )
+                            await github_api_call(
+                                pull_request.disable_automerge, logger=self.logger, log_prefix=self.log_prefix
+                            )
+                        except asyncio.CancelledError:
+                            raise
+                        except Exception:
+                            self.logger.exception(
+                                f"{self.log_prefix} Failed to disable auto-merge for suspicious paths PR"
+                            )
+                else:
+                    self.logger.info(
+                        f"{self.log_prefix} Suspicious paths detected but {SECURITY_OVERRIDE_LABEL_STR} "
+                        f"label present, allowing auto-merge"
+                    )
Evidence
PR Compliance ID 5 requires overriding auto-merge to false (and posting an explanatory comment)
when suspicious paths are modified. The updated code explicitly permits auto-merge when
SECURITY_OVERRIDE_LABEL_STR is present, which means suspicious-path PRs can still be auto-merged.

Add auto-merge override to block auto-merge when suspicious paths are modified
webhook_server/libs/handlers/pull_request_handler.py[1268-1307]

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 auto-merge override is bypassable via the `security-override` label. Compliance requires that PRs modifying suspicious paths are never auto-merged.

## Issue Context
`set_pull_request_automerge()` detects suspicious path modifications, then checks PR labels and logs that it is "allowing auto-merge" when `security-override` is present.

## Fix Focus Areas
- webhook_server/libs/handlers/pull_request_handler.py[1268-1307]

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


3. Override label auth bypass 🐞 Bug ⛨ Security
Description
set_pull_request_automerge() treats the presence of the security-override label as a “maintainer
bypass” without verifying who applied the label, so anyone with label privileges can disable the
suspicious-path auto-merge block. This contradicts the maintainer-only contract enforced by the
/security-override command and weakens mandatory security enforcement.
Code

webhook_server/libs/handlers/pull_request_handler.py[R1268-1275]

+                # Check if security-override label is present (maintainer bypass)
+                _labels = await github_api_call(
+                    lambda: list(pull_request.labels), logger=self.logger, log_prefix=self.log_prefix
+                )
+                _label_names = {label.name for label in _labels}
+
+                if SECURITY_OVERRIDE_LABEL_STR not in _label_names:
+                    auto_merge = False
Evidence
The auto-merge blocker explicitly calls the label a “maintainer bypass” but only checks whether the
label exists. Separately, the /security-override command path enforces maintainer-only usage,
showing intended authorization that is not enforced when simply reading labels.

webhook_server/libs/handlers/pull_request_handler.py[1258-1307]
webhook_server/libs/handlers/issue_comment_handler.py[327-343]
webhook_server/libs/handlers/check_run_handler.py[407-424]

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-override` is treated as a maintainer-only bypass, but the bypass is currently granted solely based on label presence. This allows any actor with label-application permissions to bypass security protections (auto-merge blocking for suspicious paths, and required security checks elsewhere).

## Issue Context
The `/security-override` command explicitly restricts usage to maintainers, but the enforcement logic in PR processing does not validate who applied the label.

## Fix Focus Areas
- webhook_server/libs/handlers/pull_request_handler.py[1268-1307]
- webhook_server/libs/handlers/check_run_handler.py[407-424]
- webhook_server/libs/handlers/issue_comment_handler.py[327-343]

## Implementation notes
- When `security-override` label is present, verify provenance before honoring it (e.g., fetch PR/issue timeline events, find the most recent `labeled` event for `security-override`, and confirm the actor is in `OwnersFileHandler.get_all_repository_maintainers()`).
- If provenance cannot be verified, treat it as NOT overridden (log + optionally comment).
- Alternatively, if provenance validation is not feasible, adjust behavior/docs to reflect that the bypass is for “anyone who can apply labels,” not “maintainers only.”

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


View more (5)
4. Doc says security-checks server-only 📎 Requirement gap ≡ Correctness
Description
examples/.github-webhook-server.yaml now states security-checks is not configurable
per-repository, which conflicts with the required support for configuring/overriding
security-checks at all config levels (including in-repo .github-webhook-server.yaml). This
misleads users and violates the documented configuration requirements for security checks.
Code

examples/.github-webhook-server.yaml[R4-7]

+#
+# NOTE: security-checks is NOT configurable per-repository.
+# Security policy is controlled server-side only (in config.yaml) to prevent
+# attackers from weakening security checks via in-repo config overrides.
Evidence
The new comment explicitly states security-checks is "NOT configurable per-repository" and only
controlled in config.yaml, contradicting the checklist requirements that security-checks be
supported across global/per-repo/in-repo config levels and that the in-repo example shows how to
configure it.

Add security-checks configuration schema (suspicious-paths, committer-identity-check) at all config levels
Update example configuration to include security-checks section
examples/.github-webhook-server.yaml[4-7]

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 example `.github-webhook-server.yaml` claims `security-checks` is not configurable per-repository, which contradicts the compliance requirement that `security-checks` must be definable/overridable at global, per-repo, and in-repo levels.

## Issue Context
Compliance requires `security-checks` to be supported across all config levels and for the in-repo example file to demonstrate the section; the current note tells users the opposite.

## Fix Focus Areas
- examples/.github-webhook-server.yaml[4-7]

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


5. Override label ignored ✓ Resolved 🐞 Bug ≡ Correctness
Description
set_pull_request_automerge() blocks/disables auto-merge for suspicious paths without checking the
maintainer-only security-override label, even though the welcome message and required-checks logic
treat that label as a bypass. This can unexpectedly disable an already-enabled auto-merge even after
a maintainer intentionally applied the override.
Code

webhook_server/libs/handlers/pull_request_handler.py[R1258-1259]

+        # Also disable already-enabled auto-merge (e.g., PR gained suspicious paths after auto-merge was set)
+        if (auto_merge or pull_request.raw_data.get("auto_merge")) and self.github_webhook.security_suspicious_paths:
Evidence
The auto-merge blocker condition doesn’t consult the override label, while other parts of the system
explicitly implement /security-override as a bypass mechanism (command adds the label; required
checks skip when label exists; welcome message advertises bypass).

webhook_server/libs/handlers/pull_request_handler.py[1257-1287]
webhook_server/libs/handlers/check_run_handler.py[407-425]
webhook_server/libs/handlers/issue_comment_handler.py[327-342]
webhook_server/libs/handlers/pull_request_handler.py[641-644]
webhook_server/libs/handlers/pull_request_handler.py[766-768]

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

## Issue description
`PullRequestHandler.set_pull_request_automerge()` blocks (and can disable) auto-merge when suspicious paths are touched, but it does not respect the maintainer-only `security-override` label.

This contradicts:
- the welcome message text that advertises `/security-override` as a bypass for security checks
- the required-checks logic that explicitly skips mandatory security checks when the override label is present

## Issue Context
The new condition `(auto_merge or pull_request.raw_data.get("auto_merge"))` broadens when the suspicious-path block runs; as a result, an already-enabled auto-merge can be disabled even if maintainers applied `security-override` to proceed.

## Fix Focus Areas
- webhook_server/libs/handlers/pull_request_handler.py[1257-1287]
- webhook_server/libs/handlers/check_run_handler.py[407-425]
- webhook_server/libs/handlers/issue_comment_handler.py[327-342]
- webhook_server/libs/handlers/pull_request_handler.py[641-644]

## Suggested fix
1. Before applying the suspicious-path auto-merge block, fetch PR labels (same pattern as in `CheckRunHandler.all_required_status_checks`) and detect `SECURITY_OVERRIDE_LABEL_STR`.
2. If the override label is present, skip:
  - posting the “Auto-merge blocked…” comment
  - disabling existing auto-merge
  - forcing `auto_merge = False`
3. Add/extend a unit test asserting that with `security-override` label present, suspicious-path changes do not block/disable auto-merge.

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


6. disable_automerge swallows cancellation ✓ Resolved 📘 Rule violation ☼ Reliability
Description
set_pull_request_automerge() catches broad Exception around disable_automerge and does not
re-raise asyncio.CancelledError, which can break async cancellation semantics and leave tasks
stuck during shutdown/timeouts.
Code

webhook_server/libs/handlers/pull_request_handler.py[R1279-1287]

+                if pull_request.raw_data.get("auto_merge"):
+                    try:
+                        self.logger.info(f"{self.log_prefix} Suspicious paths detected, disabling existing auto-merge")
+                        await github_api_call(
+                            pull_request.disable_automerge, logger=self.logger, log_prefix=self.log_prefix
+                        )
+                    except Exception:
+                        self.logger.exception(f"{self.log_prefix} Failed to disable auto-merge for suspicious paths PR")
+
Evidence
PR Compliance ID 19 requires re-raising asyncio.CancelledError when caught; the new `except
Exception:` block logs and continues, which would also catch and swallow cancellation exceptions.

CLAUDE.md: Use logger.exception() for Exceptions and Re-raise asyncio.CancelledError
webhook_server/libs/handlers/pull_request_handler.py[1279-1287]

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

## Issue description
`set_pull_request_automerge()` wraps the `pull_request.disable_automerge` call with `except Exception:` and logs, but does not re-raise `asyncio.CancelledError`. This can swallow cancellations in async code.

## Issue Context
Compliance requires that `asyncio.CancelledError` is re-raised when caught, and that exception handling is as specific as feasible.

## Fix Focus Areas
- webhook_server/libs/handlers/pull_request_handler.py[1279-1287]

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


7. Automerge override bypass ✓ Resolved 🐞 Bug ⛨ Security
Description
The suspicious-path override in set_pull_request_automerge() only flips a local auto_merge flag and
posts a comment; it never disables an already-enabled PR auto-merge state. Additionally, this
auto-merge logic is not invoked for the pull_request "synchronize" action, so a PR can gain
suspicious-path changes after auto-merge was enabled without being turned off.
Code

webhook_server/libs/handlers/pull_request_handler.py[R1246-1266]

+        # Security override: block auto-merge if PR modifies suspicious paths
+        if auto_merge and self.github_webhook.security_suspicious_paths:
+            changed_files = self.owners_file_handler.changed_files
+            suspicious_matches = [
+                f
+                for f in changed_files
+                if any(f.startswith(prefix) for prefix in self.github_webhook.security_suspicious_paths)
+            ]
+            if suspicious_matches:
+                auto_merge = False
+                files_list = ", ".join(f"`{f}`" for f in suspicious_matches)
+                self.logger.info(
+                    f"{self.log_prefix} Auto-merge blocked: PR modifies security-sensitive paths: {suspicious_matches}"
+                )
+                await github_api_call(
+                    pull_request.create_issue_comment,
+                    f"Auto-merge blocked: PR modifies security-sensitive paths: {files_list}",
+                    logger=self.logger,
+                    log_prefix=self.log_prefix,
+                )
+
Evidence
The synchronize path returns without calling set_pull_request_automerge(), while
opened/reopened/ready_for_review explicitly calls it. Inside set_pull_request_automerge(),
suspicious-path detection only toggles a local flag and comments; unlike the existing
AI-resolved-conflicts block, it never calls disable_automerge() when suspicious files are present.

webhook_server/libs/handlers/pull_request_handler.py[234-333]
webhook_server/libs/handlers/pull_request_handler.py[1231-1285]

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

### Issue description
Auto-merge blocking for suspicious paths is incomplete:
1) When suspicious paths are detected, the code sets a local `auto_merge = False` but does **not** call `pull_request.disable_automerge()` when auto-merge is already enabled on the PR.
2) The handler does not run `set_pull_request_automerge()` on `pull_request` `synchronize` events, so suspicious changes added after initial enablement can keep auto-merge active.

### Issue Context
This PR introduces a security invariant: *PRs touching suspicious paths must never be auto-merged*. The current implementation only prevents enabling auto-merge in the current call.

### Fix Focus Areas
- webhook_server/libs/handlers/pull_request_handler.py[234-333]
- webhook_server/libs/handlers/pull_request_handler.py[1231-1305]

### Suggested implementation sketch
- Extract a helper like `_apply_suspicious_paths_automerge_override(pull_request)` that:
 - Computes `suspicious_matches`.
 - If matches exist:
   - If `pull_request.raw_data.get("auto_merge")` is truthy, call `pull_request.disable_automerge` via `github_api_call` (with exception handling similar to the AI-resolved conflicts block).
   - Post a single explanatory comment (optional: avoid duplicate comments).
   - Ensure the caller does not re-enable auto-merge.
- Invoke this helper from both:
 - `set_pull_request_automerge()` (before enabling)
 - the `hook_action == "synchronize"` flow (so updates are enforced without necessarily enabling auto-merge on every sync).

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


8. security-checks ignores repo config 📎 Requirement gap ≡ Correctness
Description
security-checks is loaded without passing repository_config into Config.get_value(), so
.github-webhook-server.yaml (in-repo) overrides are ignored for suspicious paths and committer
identity enablement. This breaks the required 3-level config resolution behavior for the new
security checks.
Code

webhook_server/libs/github_api.py[R876-883]

+        _security_checks: dict[str, Any] | None = self.config.get_value(value="security-checks", return_on_none=None)
+        _security_config = _security_checks if isinstance(_security_checks, dict) else {}
+        _suspicious_paths = _security_config.get("suspicious-paths", DEFAULT_SUSPICIOUS_PATHS)
+        self.security_suspicious_paths: list[str] = (
+            _suspicious_paths if isinstance(_suspicious_paths, list) else DEFAULT_SUSPICIOUS_PATHS
+        )
+        self.security_committer_identity_check: bool = _security_config.get("committer-identity-check", True)
+
Evidence
Config.get_value() only consults the in-repo config when the caller passes it via extra_dict;
the new security-checks load path omits extra_dict=repository_config, so resolved values cannot
come from .github-webhook-server.yaml, violating the 3-level config requirement.

Security-checks configuration is loaded and merged at all 3 config levels
webhook_server/libs/github_api.py[876-883]
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
`security-checks` is currently read using `self.config.get_value(...)` without `extra_dict=repository_config`, which prevents in-repo `.github-webhook-server.yaml` from overriding security check settings.

## Issue Context
`_repo_data_from_config(repository_config=...)` is called twice: first with `{}` (server config only), then with the parsed `.github-webhook-server.yaml` content. Most config keys pass `extra_dict=repository_config` to allow in-repo overrides, but `security-checks` currently does not.

## Fix Focus Areas
- webhook_server/libs/github_api.py[876-883]

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



Remediation recommended

9. Tautological sanitization tests 🐞 Bug ⚙ Maintainability
Description
TestSecurityConfigSanitization re-implements the sanitization list-comprehension inline instead of
exercising the production config-loading path, so it can still pass even if GithubWebhook’s real
parsing/sanitization changes or breaks. This gives a false sense of coverage for malformed
security-checks.suspicious-paths handling.
Code

webhook_server/tests/test_security_checks.py[R506-523]

+class TestSecurityConfigSanitization:
+    """Test that malformed config values are handled gracefully."""
+
+    def test_non_string_suspicious_paths_sanitized(self) -> None:
+        """Non-string items in suspicious-paths are converted to strings."""
+        _suspicious_paths: list[Any] = [".github/workflows/", 123, 4.5, "", "  ", ".vscode/"]
+        result = [str(p).strip() for p in _suspicious_paths if isinstance(p, (str, int, float)) and str(p).strip()]
+        assert result == [".github/workflows/", "123", "4.5", ".vscode/"]
+
+    def test_non_list_suspicious_paths_uses_defaults(self) -> None:
+        """Non-list suspicious-paths falls back to defaults."""
+        _suspicious_paths = "not-a-list"
+        result = (
+            [str(p).strip() for p in _suspicious_paths if isinstance(p, (str, int, float)) and str(p).strip()]
+            if isinstance(_suspicious_paths, list)
+            else DEFAULT_SUSPICIOUS_PATHS
+        )
+        assert result == DEFAULT_SUSPICIOUS_PATHS
Evidence
The tests compute result = (...) directly, while production sanitization is performed during
config loading in _repo_data_from_config, meaning the tests don’t validate the real runtime path.

webhook_server/tests/test_security_checks.py[506-523]
webhook_server/libs/github_api.py[876-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
The new `TestSecurityConfigSanitization` tests validate an inline transformation rather than the actual production behavior (config parsing in `GithubWebhook._repo_data_from_config`). This weakens coverage because the tests won’t fail if the production logic diverges.

## Issue Context
Production sanitization of `security-checks.suspicious-paths` happens when loading config into `GithubWebhook.security_suspicious_paths`.

## Fix Focus Areas
- webhook_server/tests/test_security_checks.py[506-525]
- webhook_server/libs/github_api.py[876-886]

## Suggested fix
Replace (or supplement) the inline-comprehension tests with a test that:
1. Instantiates a `GithubWebhook` (or the smallest unit that owns `_repo_data_from_config`) with a mocked `config.get_value()` returning malformed `security-checks` values.
2. Calls `_repo_data_from_config(repository_config=...)`.
3. Asserts on `github_webhook.security_suspicious_paths` for:
  - non-list suspicious-paths falling back to defaults
  - non-string list items being handled as intended

Alternative: extract sanitization into a small helper function (e.g., `sanitize_suspicious_paths(...)`) and unit test that helper, then use it in `_repo_data_from_config()`.

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


10. Mandatory flag unvalidated 🐞 Bug ☼ Reliability
Description
GithubWebhook._repo_data_from_config() assigns security_mandatory directly from config without
ensuring it is a boolean. Because Config.get_value() returns raw YAML values and runtime config
loading only validates labels, a quoted value like mandatory: "false" becomes a truthy string and
incorrectly makes security checks mandatory (blocking merges).
Code

webhook_server/libs/github_api.py[885]

+        self.security_mandatory: bool = _security_config.get("mandatory", True)
Evidence
security_mandatory is sourced from the config dict without type checks, and the config system
returns raw values with no schema enforcement at runtime—so non-bool values can reach this
assignment and be interpreted incorrectly in boolean contexts.

webhook_server/libs/github_api.py[876-886]
webhook_server/libs/config.py[13-26]
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
`security-checks.mandatory` is read from config as-is and stored in `self.security_mandatory`. If the YAML contains a non-boolean (e.g., a quoted string), Python treats it as truthy and the webhook will incorrectly enforce security checks as mandatory.

### Issue Context
The runtime config loader (`Config`) does not enforce the JSON schema types; it returns nested values unchanged. This makes type validation at the point of use necessary for safety.

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

### Suggested fix
- Change assignment to accept only real booleans, otherwise log a warning and fall back to the default:
 - `val = _security_config.get("mandatory", True)`
 - `self.security_mandatory = val if isinstance(val, bool) else True`
 - (optional) `logger.warning(...)` when non-bool provided.
- Add/extend a unit test to cover `mandatory: "false"` (string) resulting in default behavior + warning (if you add logging).

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


11. In-repo example lacks security-checks 📎 Requirement gap ⚙ Maintainability
Description
examples/.github-webhook-server.yaml does not demonstrate the new security-checks keys, so users
lack a working in-repo configuration example. The PR instead adds the section to
examples/config.yaml, which does not satisfy the specific example-file requirement.
Code

examples/config.yaml[R139-152]

+# Security Checks configuration
+# Detects potentially malicious PR patterns like modifications to
+# security-sensitive paths or mismatched committer identities.
+security-checks:
+  suspicious-paths:
+    - ".claude/"
+    - ".vscode/"
+    - ".cursor/"
+    - ".devcontainer/"
+    - ".pi/"
+    - ".github/workflows/"
+    - ".github/actions/"
+  committer-identity-check: true
+
Evidence
The PR adds security-checks to examples/config.yaml, but the required in-repo example file
examples/.github-webhook-server.yaml still contains no security-checks: block near the
configuration sections where it should be documented.

Example configuration demonstrates security-checks usage
examples/config.yaml[139-152]
examples/.github-webhook-server.yaml[150-170]

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 required example file `examples/.github-webhook-server.yaml` is missing a `security-checks` section demonstrating `suspicious-paths` and `committer-identity-check`.

## Issue Context
The PR adds `security-checks` to `examples/config.yaml`, but the compliance item requires updating `examples/.github-webhook-server.yaml` specifically (the in-repo configuration example).

## Fix Focus Areas
- examples/.github-webhook-server.yaml[1-200]
- examples/config.yaml[139-152]

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


View more (1)
12. Security config can crash ✓ Resolved 🐞 Bug ☼ Reliability
Description
security-checks.suspicious-paths is only validated as a list (not list[str]) at load time, but is
later passed into str.startswith(prefix). If the server config.yaml contains any non-string prefix,
RunnerHandler and the auto-merge override can raise TypeError, breaking PR processing or leaving
security check runs stuck in_progress.
Code

webhook_server/libs/handlers/runner_handler.py[R322-331]

+        suspicious_paths = self.github_webhook.security_suspicious_paths
+        if not suspicious_paths:
+            self.logger.debug(f"{self.log_prefix} No suspicious paths configured, skipping security check")
+            return
+
+        await self.check_run_handler.set_check_in_progress(name=SECURITY_SUSPICIOUS_PATHS_STR)
+
+        changed_files = self.owners_file_handler.changed_files
+        matched_files = [f for f in changed_files if any(f.startswith(prefix) for prefix in suspicious_paths)]
+
Evidence
Config loading uses yaml.safe_load and returns raw values without runtime schema validation beyond
label categories; the new security code then passes configured prefixes directly into startswith(),
which throws if any prefix is not a str/tuple[str].

webhook_server/libs/config.py[35-64]
webhook_server/libs/config.py[132-153]
webhook_server/libs/github_api.py[872-883]
webhook_server/libs/handlers/runner_handler.py[316-331]
webhook_server/libs/handlers/pull_request_handler.py[1246-1253]

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

### Issue description
Runtime code assumes `security-checks.suspicious-paths` is `list[str]`, but the loader only checks `isinstance(..., list)`. Since runtime does not enforce the JSON schema, malformed YAML can reach this path and cause `TypeError` in `startswith()`.

### Issue Context
`Config` loads raw YAML and only validates label-category values; no schema validation is applied at runtime. New security code uses `startswith(prefix)` in multiple places.

### Fix Focus Areas
- webhook_server/libs/github_api.py[872-883]
- webhook_server/libs/handlers/runner_handler.py[316-360]
- webhook_server/libs/handlers/pull_request_handler.py[1246-1254]
- webhook_server/libs/config.py[35-64]
- webhook_server/libs/config.py[132-153]

### Suggested implementation sketch
- In `GithubWebhook._repo_data_from_config()`:
 - Sanitize suspicious paths:
   - If value is a list: keep only non-empty strings, `str(p).strip()`; drop invalid entries and log a warning.
   - Otherwise fall back to `DEFAULT_SUSPICIOUS_PATHS`.
 - Sanitize `committer-identity-check`:
   - Only accept `bool`; otherwise log warning and use default.
- (Defense-in-depth) When matching, ensure prefixes are strings (or precompute a tuple of strings) before calling `startswith`.
- Consider wrapping `run_security_suspicious_paths()` matching block in try/except to set check failure output on unexpected exceptions (avoids leaving checks in `in_progress`).

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


Grey Divider

Previous review results

Review updated until commit b6cafbe

Results up to commit d817d49


🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (2) 🎨 UX issues (0)


Action required
1. security-checks ignores repo config 📎 Requirement gap ≡ Correctness
Description
security-checks is loaded without passing repository_config into Config.get_value(), so
.github-webhook-server.yaml (in-repo) overrides are ignored for suspicious paths and committer
identity enablement. This breaks the required 3-level config resolution behavior for the new
security checks.
Code

webhook_server/libs/github_api.py[R876-883]

+        _security_checks: dict[str, Any] | None = self.config.get_value(value="security-checks", return_on_none=None)
+        _security_config = _security_checks if isinstance(_security_checks, dict) else {}
+        _suspicious_paths = _security_config.get("suspicious-paths", DEFAULT_SUSPICIOUS_PATHS)
+        self.security_suspicious_paths: list[str] = (
+            _suspicious_paths if isinstance(_suspicious_paths, list) else DEFAULT_SUSPICIOUS_PATHS
+        )
+        self.security_committer_identity_check: bool = _security_config.get("committer-identity-check", True)
+
Evidence
Config.get_value() only consults the in-repo config when the caller passes it via extra_dict;
the new security-checks load path omits extra_dict=repository_config, so resolved values cannot
come from .github-webhook-server.yaml, violating the 3-level config requirement.

Security-checks configuration is loaded and merged at all 3 config levels
webhook_server/libs/github_api.py[876-883]
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
`security-checks` is currently read using `self.config.get_value(...)` without `extra_dict=repository_config`, which prevents in-repo `.github-webhook-server.yaml` from overriding security check settings.

## Issue Context
`_repo_data_from_config(repository_config=...)` is called twice: first with `{}` (server config only), then with the parsed `.github-webhook-server.yaml` content. Most config keys pass `extra_dict=repository_config` to allow in-repo overrides, but `security-checks` currently does not.

## Fix Focus Areas
- webhook_server/libs/github_api.py[876-883]

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


2. Automerge override bypass ✓ Resolved 🐞 Bug ⛨ Security
Description
The suspicious-path override in set_pull_request_automerge() only flips a local auto_merge flag and
posts a comment; it never disables an already-enabled PR auto-merge state. Additionally, this
auto-merge logic is not invoked for the pull_request "synchronize" action, so a PR can gain
suspicious-path changes after auto-merge was enabled without being turned off.
Code

webhook_server/libs/handlers/pull_request_handler.py[R1246-1266]

+        # Security override: block auto-merge if PR modifies suspicious paths
+        if auto_merge and self.github_webhook.security_suspicious_paths:
+            changed_files = self.owners_file_handler.changed_files
+            suspicious_matches = [
+                f
+                for f in changed_files
+                if any(f.startswith(prefix) for prefix in self.github_webhook.security_suspicious_paths)
+            ]
+            if suspicious_matches:
+                auto_merge = False
+                files_list = ", ".join(f"`{f}`" for f in suspicious_matches)
+                self.logger.info(
+                    f"{self.log_prefix} Auto-merge blocked: PR modifies security-sensitive paths: {suspicious_matches}"
+                )
+                await github_api_call(
+                    pull_request.create_issue_comment,
+                    f"Auto-merge blocked: PR modifies security-sensitive paths: {files_list}",
+                    logger=self.logger,
+                    log_prefix=self.log_prefix,
+                )
+
Evidence
The synchronize path returns without calling set_pull_request_automerge(), while
opened/reopened/ready_for_review explicitly calls it. Inside set_pull_request_automerge(),
suspicious-path detection only toggles a local flag and comments; unlike the existing
AI-resolved-conflicts block, it never calls disable_automerge() when suspicious files are present.

webhook_server/libs/handlers/pull_request_handler.py[234-333]
webhook_server/libs/handlers/pull_request_handler.py[1231-1285]

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

### Issue description
Auto-merge blocking for suspicious paths is incomplete:
1) When suspicious paths are detected, the code sets a local `auto_merge = False` but does **not** call `pull_request.disable_automerge()` when auto-merge is already enabled on the PR.
2) The handler does not run `set_pull_request_automerge()` on `pull_request` `synchronize` events, so suspicious changes added after initial enablement can keep auto-merge active.

### Issue Context
This PR introduces a security invariant: *PRs touching suspicious paths must never be auto-merged*. The current implementation only prevents enabling auto-merge in the current call.

### Fix Focus Areas
- webhook_server/libs/handlers/pull_request_handler.py[234-333]
- webhook_server/libs/handlers/pull_request_handler.py[1231-1305]

### Suggested implementation sketch
- Extract a helper like `_apply_suspicious_paths_automerge_override(pull_request)` that:
 - Computes `suspicious_matches`.
 - If matches exist:
   - If `pull_request.raw_data.get("auto_merge")` is truthy, call `pull_request.disable_automerge` via `github_api_call` (with exception handling similar to the AI-resolved conflicts block).
   - Post a single explanatory comment (optional: avoid duplicate comments).
   - Ensure the caller does not re-enable auto-merge.
- Invoke this helper from both:
 - `set_pull_request_automerge()` (before enabling)
 - the `hook_action == "synchronize"` flow (so updates are enforced without necessarily enabling auto-merge on every sync).

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



Remediation recommended
3. In-repo example lacks security-checks 📎 Requirement gap ⚙ Maintainability
Description
examples/.github-webhook-server.yaml does not demonstrate the new security-checks keys, so users
lack a working in-repo configuration example. The PR instead adds the section to
examples/config.yaml, which does not satisfy the specific example-file requirement.
Code

examples/config.yaml[R139-152]

+# Security Checks configuration
+# Detects potentially malicious PR patterns like modifications to
+# security-sensitive paths or mismatched committer identities.
+security-checks:
+  suspicious-paths:
+    - ".claude/"
+    - ".vscode/"
+    - ".cursor/"
+    - ".devcontainer/"
+    - ".pi/"
+    - ".github/workflows/"
+    - ".github/actions/"
+  committer-identity-check: true
+
Evidence
The PR adds security-checks to examples/config.yaml, but the required in-repo example file
examples/.github-webhook-server.yaml still contains no security-checks: block near the
configuration sections where it should be documented.

Example configuration demonstrates security-checks usage
examples/config.yaml[139-152]
examples/.github-webhook-server.yaml[150-170]

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 required example file `examples/.github-webhook-server.yaml` is missing a `security-checks` section demonstrating `suspicious-paths` and `committer-identity-check`.

## Issue Context
The PR adds `security-checks` to `examples/config.yaml`, but the compliance item requires updating `examples/.github-webhook-server.yaml` specifically (the in-repo configuration example).

## Fix Focus Areas
- examples/.github-webhook-server.yaml[1-200]
- examples/config.yaml[139-152]

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


4. Security config can crash ✓ Resolved 🐞 Bug ☼ Reliability
Description
security-checks.suspicious-paths is only validated as a list (not list[str]) at load time, but is
later passed into str.startswith(prefix). If the server config.yaml contains any non-string prefix,
RunnerHandler and the auto-merge override can raise TypeError, breaking PR processing or leaving
security check runs stuck in_progress.
Code

webhook_server/libs/handlers/runner_handler.py[R322-331]

+        suspicious_paths = self.github_webhook.security_suspicious_paths
+        if not suspicious_paths:
+            self.logger.debug(f"{self.log_prefix} No suspicious paths configured, skipping security check")
+            return
+
+        await self.check_run_handler.set_check_in_progress(name=SECURITY_SUSPICIOUS_PATHS_STR)
+
+        changed_files = self.owners_file_handler.changed_files
+        matched_files = [f for f in changed_files if any(f.startswith(prefix) for prefix in suspicious_paths)]
+
Evidence
Config loading uses yaml.safe_load and returns raw values without runtime schema validation beyond
label categories; the new security code then passes configured prefixes directly into startswith(),
which throws if any prefix is not a str/tuple[str].

webhook_server/libs/config.py[35-64]
webhook_server/libs/config.py[132-153]
webhook_server/libs/github_api.py[872-883]
webhook_server/libs/handlers/runner_handler.py[316-331]
webhook_server/libs/handlers/pull_request_handler.py[1246-1253]

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

### Issue description
Runtime code assumes `security-checks.suspicious-paths` is `list[str]`, but the loader only checks `isinstance(..., list)`. Since runtime does not enforce the JSON schema, malformed YAML can reach this path and cause `TypeError` in `startswith()`.

### Issue Context
`Config` loads raw YAML and only validates label-category values; no schema validation is applied at runtime. New security code uses `startswith(prefix)` in multiple places.

### Fix Focus Areas
- webhook_server/libs/github_api.py[872-883]
- webhook_server/libs/handlers/runner_handler.py[316-360]
- webhook_server/libs/handlers/pull_request_handler.py[1246-1254]
- webhook_server/libs/config.py[35-64]
- webhook_server/libs/config.py[132-153]

### Suggested implementation sketch
- In `GithubWebhook._repo_data_from_config()`:
 - Sanitize suspicious paths:
   - If value is a list: keep only non-empty strings, `str(p).strip()`; drop invalid entries and log a warning.
   - Otherwise fall back to `DEFAULT_SUSPICIOUS_PATHS`.
 - Sanitize `committer-identity-check`:
   - Only accept `bool`; otherwise log warning and use default.
- (Defense-in-depth) When matching, ensure prefixes are strings (or precompute a tuple of strings) before calling `startswith`.
- Consider wrapping `run_security_suspicious_paths()` matching block in try/except to set check failure output on unexpected exceptions (avoids leaving checks in `in_progress`).

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


Results up to commit a537bcb


🐞 Bugs (1) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0)


Action required
1. disable_automerge swallows cancellation ✓ Resolved 📘 Rule violation ☼ Reliability
Description
set_pull_request_automerge() catches broad Exception around disable_automerge and does not
re-raise asyncio.CancelledError, which can break async cancellation semantics and leave tasks
stuck during shutdown/timeouts.
Code

webhook_server/libs/handlers/pull_request_handler.py[R1279-1287]

+                if pull_request.raw_data.get("auto_merge"):
+                    try:
+                        self.logger.info(f"{self.log_prefix} Suspicious paths detected, disabling existing auto-merge")
+                        await github_api_call(
+                            pull_request.disable_automerge, logger=self.logger, log_prefix=self.log_prefix
+                        )
+                    except Exception:
+                        self.logger.exception(f"{self.log_prefix} Failed to disable auto-merge for suspicious paths PR")
+

...

@myakove-bot

Copy link
Copy Markdown
Collaborator

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

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

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

💡 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.

Comment thread webhook_server/libs/github_api.py
Comment thread examples/config.yaml
Comment thread webhook_server/libs/handlers/pull_request_handler.py
Comment thread webhook_server/libs/handlers/runner_handler.py
myakove added 2 commits June 9, 2026 13:47
…ired status checks

- Security checks block can-be-merged when mandatory=true (default)
- Add /security-override command for maintainers to bypass security checks
- Add security-override label to skip security requirements
- Add mandatory config flag (default: true, admin can set false for advisory)
- Add security checks to all_required_status_checks in check_run_handler
- Disable already-enabled auto-merge when suspicious paths detected
- Sanitize suspicious-paths config entries to strings (prevent TypeError)
@qodo-code-review

qodo-code-review Bot commented Jun 9, 2026

Copy link
Copy Markdown

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

Comment thread webhook_server/libs/handlers/pull_request_handler.py Outdated
Comment thread webhook_server/libs/github_api.py Outdated
@myakove

myakove commented Jun 9, 2026

Copy link
Copy Markdown
Collaborator Author

@qodo-code-review[bot]

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

webhook_server/libs/handlers/pull_request_handler.py:1246 (qodo bug) — Automerge override bypass

Addressed: Fixed — calls disable_automerge when suspicious paths detected and auto-merge already enabled.

webhook_server/libs/github_api.py:876 (qodo requirement gap) — security-checks ignores repo config

Addressed: By design — security-checks intentionally not overridable by in-repo config to prevent attackers from weakening security policy. Documented in issue #1106.

examples/config.yaml:139 (qodo requirement gap) — In-repo example lacks security-checks

Addressed: Added note in examples/.github-webhook-server.yaml that security-checks is global-only config.

…ent global-only security

- Expand auto-merge condition to also disable already-enabled auto-merge
  when PR gains suspicious paths on synchronize events
- Add note to in-repo example that security-checks is global-only
- Add config sanitization tests for malformed suspicious-paths values
- Fix triple-quote typo in test class docstring
@qodo-code-review

qodo-code-review Bot commented Jun 9, 2026

Copy link
Copy Markdown

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

Comment thread examples/.github-webhook-server.yaml
Comment thread webhook_server/libs/handlers/pull_request_handler.py
Comment thread webhook_server/tests/test_security_checks.py
… flag

- Add asyncio.CancelledError re-raise before broad except in disable_automerge
- Validate security_mandatory with bool() to handle non-bool config values
@qodo-code-review

qodo-code-review Bot commented Jun 9, 2026

Copy link
Copy Markdown

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

- Check for security-override label before blocking auto-merge for suspicious paths
- If maintainer applied /security-override, auto-merge proceeds normally
- Update test to account for labels fetch in auto-merge flow
@qodo-code-review

qodo-code-review Bot commented Jun 9, 2026

Copy link
Copy Markdown

Code review by qodo was updated up to the latest commit 4ad9062

@myakove

myakove commented Jun 9, 2026

Copy link
Copy Markdown
Collaborator Author

@qodo-code-review[bot]

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

examples/.github-webhook-server.yaml:4 (qodo requirement gap) — Doc says security-checks server-only

Addressed: By design — security-checks is global-only to prevent attackers from weakening security policy via in-repo config.

webhook_server/libs/handlers/pull_request_handler.py:1246 (qodo bug) — Automerge override bypass

Addressed: Previously addressed.

webhook_server/libs/github_api.py:876 (qodo requirement gap) — security-checks ignores repo config

Addressed: By design — intentionally not using extra_dict for security-checks. Documented in issue #1106.

webhook_server/tests/test_security_checks.py:506 (qodo bug) — Tautological sanitization tests

Addressed: Previously addressed.

webhook_server/libs/github_api.py:885 (qodo bug) — Mandatory flag unvalidated

Addressed: Fixed in commit a6f91fd — added bool() cast to ensure non-bool config values are coerced correctly.

examples/config.yaml:139 (qodo requirement gap) — In-repo example lacks security-checks

Addressed: Previously addressed.

Comment thread webhook_server/libs/handlers/pull_request_handler.py
Comment thread webhook_server/libs/handlers/pull_request_handler.py
Comment thread webhook_server/tests/test_security_checks.py
@myakove

myakove commented Jun 9, 2026

Copy link
Copy Markdown
Collaborator Author

@qodo-code-review[bot]

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

webhook_server/libs/handlers/pull_request_handler.py:1268 (qodo requirement gap) — security-override allows auto-merge

Addressed: By design — /security-override is a maintainer-only command that explicitly allows bypassing security checks. The entire purpose of the command is to let maintainers proceed when a legitimate change touches security-sensitive paths. The label is only addable via the /security-override command which validates maintainer permissions.

webhook_server/libs/handlers/pull_request_handler.py:1268 (qodo bug) — Override label auth bypass

Addressed: Acknowledged as a defense-in-depth concern. The /security-override command validates maintainer permissions before adding the label. Direct label application via GitHub UI requires write access (already restricted to collaborators). Adding label provenance verification via timeline events would add significant API cost. Accepted risk for now — documenting that the label should only be applied via the command.

webhook_server/tests/test_security_checks.py:382 (qodo bug) — Automerge test mock breaks

Addressed: Test issue — not a production bug. Mock fixtures updated to match new label-checking behavior.

- Change 'single token configured' to 'single API configured' in helpers.py
- Prevents mask-sensitive-data regex from masking the log line
@qodo-code-review

qodo-code-review Bot commented Jun 9, 2026

Copy link
Copy Markdown

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

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

feat: PR security checks — suspicious paths, committer identity, auto-merge override

2 participants