feat(commands): add allow-commands-on-draft-prs config option #980
Conversation
|
Report bugs in Issues Welcome! 🎉This pull request will be automatically processed with the following features: 🔄 Automatic Actions
📋 Available CommandsPR Status Management
Review & Approval
Testing & Validation
Container Operations
Cherry-pick Operations
Label Management
✅ Merge RequirementsThis PR will be automatically approved when the following conditions are met:
📊 Review ProcessApprovers and ReviewersApprovers:
Reviewers:
Available Labels
💡 Tips
For more information, please refer to the project documentation or contact the maintainers. |
WalkthroughAdds a config-driven allow-list to control which commands may run on draft PRs, with both global and per-repository keys. Draft-shortcircuiting in the webhook handler is preserved unless allow-list config permits issue_comment processing; issue_comment handling enforces the allow-list and comments when commands are disallowed. Changes
Sequence DiagramsequenceDiagram
participant Webhook as Webhook Event
participant GH_API as GitHub API Handler
participant Config as Configuration Store
participant ICH as Issue Comment Handler
participant GH as GitHub API
Webhook->>GH_API: issue_comment event (draft PR)
GH_API->>Config: get allow-commands-on-draft-prs
alt Config unset OR event != issue_comment
Config-->>GH_API: unset / not applicable
GH_API->>GH_API: update metrics
GH_API-->>Webhook: return (draft skipped)
else Config set and event == issue_comment
Config-->>GH_API: config provided
GH_API->>ICH: forward event for processing
ICH->>Config: get repo/global allow-commands-on-draft-prs
alt allow-list is empty
Config-->>ICH: allow-all (empty list)
ICH->>GH: execute command
else allow-list contains command
Config-->>ICH: whitelisted
ICH->>GH: execute command
else not whitelisted
Config-->>ICH: not allowed
ICH->>GH: post comment listing allowed commands
ICH-->>GH_API: return (no exec)
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Allow configuring which commands can be executed on draft PRs. - Add allow-commands-on-draft-prs schema option (global and per-repo) - Filter commands in issue_comment_handler based on config - Empty list allows all commands, specific list restricts to those commands - Not set (default) blocks all commands on draft PRs Closes #979
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@webhook_server/tests/test_issue_comment_handler.py`:
- Around line 147-152: The async test mock function mock_command currently has
an unnecessary linter suppression "# noqa: ARG001" — remove that directive from
the async def signature so the line becomes a normal async def
mock_command(...); leave the parameter list and body as-is since unused
arguments are acceptable for this test mock.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
examples/config.yamlwebhook_server/config/schema.yamlwebhook_server/libs/github_api.pywebhook_server/libs/handlers/issue_comment_handler.pywebhook_server/tests/test_issue_comment_handler.py
🧰 Additional context used
📓 Path-based instructions (7)
**/*.{yaml,yml}
📄 CodeRabbit inference engine (CLAUDE.md)
Maintain backward compatibility for user-facing configuration files (
config.yaml,.github-webhook-server.yaml) - configuration schema changes must support old formats or provide migration
Files:
webhook_server/config/schema.yamlexamples/config.yaml
webhook_server/config/schema.yaml
📄 CodeRabbit inference engine (CLAUDE.md)
webhook_server/config/schema.yaml: Mask sensitive data in logs by default (setmask-sensitive-data: truein configuration)
Configuration schema changes should be tested withuv run pytest webhook_server/tests/test_config_schema.py -v
Files:
webhook_server/config/schema.yaml
webhook_server/libs/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Internal methods in
webhook_server/libs/can change freely - return types can change (e.g.,Any→bool), method signatures can be modified without deprecation
Files:
webhook_server/libs/handlers/issue_comment_handler.pywebhook_server/libs/github_api.py
webhook_server/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
webhook_server/**/*.py: Server fails-fast on startup if critical dependencies are missing - required parameters in__init__()are ALWAYS provided and checking for None on required parameters is pure overhead
Defensive checks in destructors (__del__) should use hasattr() to check for attributes that may not exist if initialization failed
Defensive checks are acceptable for optional parameters that explicitly allow None (parameter type includes| NoneorOptional)
Defensive checks are acceptable for lazy initialization - attribute explicitly starts as None with type annotationattr: SomeType | None = None
Use hasattr() checks only for platform constants that may not exist on all platforms (e.g.,os.O_NOFOLLOW)
Do NOT use hasattr() checks for required parameters in__init__()- if config is required, access it directly without defensive checks
Do NOT use hasattr() checks for library versions we control inpyproject.toml(PyGithub >=2.4.0, gql >=3.5.0) - call methods directly
Use isinstance() for type discrimination instead of hasattr() checks
Never return fake defaults (empty strings, zeros, False, None, empty collections) to hide missing data - fail-fast with ValueError or KeyError exceptions
PyGithub is synchronous/blocking - MUST wrap ALL method calls and property accesses withasyncio.to_thread()to avoid blocking the event loop
When accessing PyGithub properties that may trigger API calls, wrap inasyncio.to_thread(lambda: obj.property)instead of direct access
When iterating PyGithub PaginatedList, wrap iteration inasyncio.to_thread(lambda: list(...))to avoid blocking
Useasyncio.gather()for concurrent PyGithub operations to maximize performance
All imports must be at the top of files - no imports in functions or try/except blocks, except TYPE_CHECKING imports can be conditional
Complete type hints are mandatory - use mypy strict mode with full type annotations on function parameters and return types
Uselogger.exception()for autom...
Files:
webhook_server/libs/handlers/issue_comment_handler.pywebhook_server/libs/github_api.pywebhook_server/tests/test_issue_comment_handler.py
webhook_server/libs/handlers/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
webhook_server/libs/handlers/**/*.py: Do NOT use defensive checks for architecture guarantees likerepository_datawhich is ALWAYS set before handlers instantiate
Handler classes must implement__init__(self, github_webhook, ...)andprocess_event(event_data)pattern
Handlers must useself.github_webhook.unified_apifor all GitHub operations, not direct PyGithub calls
Usectx.start_step(),ctx.complete_step(),ctx.fail_step()for structured webhook execution tracking
Files:
webhook_server/libs/handlers/issue_comment_handler.py
webhook_server/libs/github_api.py
📄 CodeRabbit inference engine (CLAUDE.md)
webhook_server/libs/github_api.py: Maintain backward compatibility for webhook payload handling - must follow GitHub webhook specification
Do NOT use defensive checks for webhook payload fields that are guaranteed to exist (e.g.,user.node_id,user.type,sender) - let KeyError surface legitimate bugs
Repository cloning for check_run events should skip with early exit when action != 'completed'
Repository cloning for check_run events should skip when check_run_conclusion != 'success' and check_run_name == 'can-be-merged'
Files:
webhook_server/libs/github_api.py
webhook_server/tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
webhook_server/tests/**/*.py: Code coverage must be 90% or higher - new code without tests fails CI
Test files should use AsyncMock, Mock, and patch for mocking - use TEST_GITHUB_TOKEN constant for test tokens
Test coverage is required for new handlers - create handler tests inwebhook_server/tests/test_*_handler.py
Files:
webhook_server/tests/test_issue_comment_handler.py
🧠 Learnings (13)
📚 Learning: 2026-01-05T14:43:44.055Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T14:43:44.055Z
Learning: Applies to webhook_server/config/schema.yaml : Configuration schema changes should be tested with `uv run pytest webhook_server/tests/test_config_schema.py -v`
Applied to files:
webhook_server/config/schema.yamlwebhook_server/tests/test_issue_comment_handler.py
📚 Learning: 2026-01-05T14:43:44.055Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T14:43:44.055Z
Learning: Applies to **/*.{yaml,yml} : Maintain backward compatibility for user-facing configuration files (`config.yaml`, `.github-webhook-server.yaml`) - configuration schema changes must support old formats or provide migration
Applied to files:
webhook_server/config/schema.yaml
📚 Learning: 2025-12-29T13:09:56.231Z
Learnt from: rnetser
Repo: myk-org/github-webhook-server PR: 959
File: webhook_server/libs/handlers/pull_request_handler.py:887-887
Timestamp: 2025-12-29T13:09:56.231Z
Learning: In the myk-org/github-webhook-server repository, logging statements in the webhook_server codebase should use f-strings for interpolation. Do not suggest changing to lazy formatting with % (e.g., logger.debug('msg %s', var)) since that is an established style choice. When adding new log statements, prefer f"...{var}..." for readability and performance, and avoid string concatenation or format-style logging unless the project explicitly adopts a different convention.
Applied to files:
webhook_server/libs/handlers/issue_comment_handler.pywebhook_server/libs/github_api.pywebhook_server/tests/test_issue_comment_handler.py
📚 Learning: 2026-01-05T14:43:44.055Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T14:43:44.055Z
Learning: Applies to webhook_server/libs/github_api.py : Maintain backward compatibility for webhook payload handling - must follow GitHub webhook specification
Applied to files:
webhook_server/libs/github_api.py
📚 Learning: 2024-10-29T08:09:57.157Z
Learnt from: myakove
Repo: myk-org/github-webhook-server PR: 612
File: webhook_server_container/libs/github_api.py:2089-2100
Timestamp: 2024-10-29T08:09:57.157Z
Learning: In `webhook_server_container/libs/github_api.py`, when the function `_keep_approved_by_approvers_after_rebase` is called, existing approval labels have already been cleared after pushing new changes, so there's no need to check for existing approvals within this function.
Applied to files:
webhook_server/libs/github_api.py
📚 Learning: 2024-10-08T09:19:56.185Z
Learnt from: myakove
Repo: myk-org/github-webhook-server PR: 586
File: webhook_server_container/libs/github_api.py:1947-1956
Timestamp: 2024-10-08T09:19:56.185Z
Learning: In `webhook_server_container/libs/github_api.py`, the indentation style used in the `set_pull_request_automerge` method is acceptable as per the project's coding standards.
Applied to files:
webhook_server/libs/github_api.py
📚 Learning: 2026-01-05T14:43:44.055Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T14:43:44.055Z
Learning: Applies to webhook_server/libs/handlers/**/*.py : Handlers must use `self.github_webhook.unified_api` for all GitHub operations, not direct PyGithub calls
Applied to files:
webhook_server/libs/github_api.py
📚 Learning: 2026-01-05T14:43:44.055Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T14:43:44.055Z
Learning: Applies to webhook_server/libs/github_api.py : Do NOT use defensive checks for webhook payload fields that are guaranteed to exist (e.g., `user.node_id`, `user.type`, `sender`) - let KeyError surface legitimate bugs
Applied to files:
webhook_server/libs/github_api.py
📚 Learning: 2026-01-05T14:43:44.055Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T14:43:44.055Z
Learning: Applies to webhook_server/libs/github_api.py : Repository cloning for check_run events should skip when check_run_conclusion != 'success' and check_run_name == 'can-be-merged'
Applied to files:
webhook_server/libs/github_api.py
📚 Learning: 2024-10-29T10:42:50.163Z
Learnt from: myakove
Repo: myk-org/github-webhook-server PR: 612
File: webhook_server_container/libs/github_api.py:925-926
Timestamp: 2024-10-29T10:42:50.163Z
Learning: In `webhook_server_container/libs/github_api.py`, the method `self._keep_approved_by_approvers_after_rebase()` must be called after removing labels when synchronizing a pull request. Therefore, it should be placed outside the `ThreadPoolExecutor` to ensure it runs sequentially after label removal.
Applied to files:
webhook_server/libs/github_api.py
📚 Learning: 2025-05-13T12:06:27.297Z
Learnt from: myakove
Repo: myk-org/github-webhook-server PR: 778
File: webhook_server/libs/pull_request_handler.py:327-330
Timestamp: 2025-05-13T12:06:27.297Z
Learning: In the GitHub webhook server, synchronous GitHub API calls (like create_issue_comment, add_to_assignees, etc.) in async methods should be awaited using asyncio.to_thread or loop.run_in_executor to prevent blocking the event loop.
Applied to files:
webhook_server/libs/github_api.pywebhook_server/tests/test_issue_comment_handler.py
📚 Learning: 2026-01-05T14:43:44.055Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T14:43:44.055Z
Learning: Applies to webhook_server/tests/**/*.py : Test files should use AsyncMock, Mock, and patch for mocking - use TEST_GITHUB_TOKEN constant for test tokens
Applied to files:
webhook_server/tests/test_issue_comment_handler.py
📚 Learning: 2026-01-05T14:43:44.055Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T14:43:44.055Z
Learning: Applies to webhook_server/tests/**/*.py : Test coverage is required for new handlers - create handler tests in `webhook_server/tests/test_*_handler.py`
Applied to files:
webhook_server/tests/test_issue_comment_handler.py
🧬 Code graph analysis (1)
webhook_server/libs/github_api.py (1)
webhook_server/libs/config.py (1)
get_value(132-153)
🪛 Ruff (0.14.11)
webhook_server/libs/handlers/issue_comment_handler.py
177-178: Logging statement uses f-string
(G004)
webhook_server/libs/github_api.py
477-477: Logging statement uses f-string
(G004)
480-481: Logging statement uses f-string
(G004)
487-488: Logging statement uses f-string
(G004)
webhook_server/tests/test_issue_comment_handler.py
147-147: Unused noqa directive (unused: ARG001)
Remove unused noqa directive
(RUF100)
148-148: Unused function argument: pull_request
(ARG001)
150-150: Unused function argument: reviewed_user
(ARG001)
151-151: Unused function argument: issue_comment_id
(ARG001)
🔇 Additional comments (11)
webhook_server/config/schema.yaml (2)
86-94: LGTM! Schema addition is well-structured.The top-level
allow-commands-on-draft-prsproperty is correctly defined as an optional array of strings with clear documentation of the three-state semantics. Backward compatibility is preserved since missing property means commands are blocked (existing behavior).Based on learnings, ensure schema changes are tested:
uv run pytest webhook_server/tests/test_config_schema.py -v
344-352: Repository-level override mirrors global schema correctly.The per-repository
allow-commands-on-draft-prsproperty uses identical schema and semantics as the global property, enabling granular control. This aligns with the existing pattern for other repository-level overrides (e.g.,auto-verify-cherry-picked-prs).examples/config.yaml (2)
36-43: LGTM! Documentation examples are clear and comprehensive.The commented-out examples effectively demonstrate all three configuration states with explanatory comments. This follows the existing pattern in the config file for optional features.
198-200: Repository-level example demonstrates practical use case.Showing
build-and-push-containeras an allowed draft PR command is a realistic scenario—teams often want to build preview containers from draft PRs without enabling all commands.webhook_server/libs/handlers/issue_comment_handler.py (1)
169-185: LGTM! Draft PR command filtering is correctly implemented.The implementation properly:
- Wraps PyGithub property access in
asyncio.to_thread(per coding guidelines)- Handles the three-state semantics:
None/not-set passes through (handled upstream ingithub_api.py), empty list allows all, non-empty list filters commands- Provides helpful user feedback listing allowed commands
The
isinstance(..., list) and len(...) > 0type guard is a correct pattern for discriminating between "allow all" ([]) and "allow specific" (["cmd1", "cmd2"]).Note: The static analysis hint about f-strings (G004) is a false positive—f-strings are the established style per project learnings.
webhook_server/libs/github_api.py (1)
472-489: LGTM! Draft PR handling logic correctly gates issue_comment events.The conditional logic properly implements the feature:
allow_commands_on_draft is None→ preserves original behavior (skip draft PRs)self.github_event != "issue_comment"→ correctly limits draft PR processing to command events only (other events likepull_request,pull_request_reviewshould still be skipped for drafts)Context metrics are correctly updated in the early-return path (line 483), preventing metric tracking gaps.
Note: Static analysis hints about f-strings are false positives per project learnings.
webhook_server/tests/test_issue_comment_handler.py (5)
45-47: LGTM! Mock config setup enables draft PR testing.Adding
config.get_value = Mock(return_value=None)to the fixture provides the default "not configured" state, allowing individual tests to override the return value for specific scenarios.
1254-1280: LGTM! Test correctly verifies command blocking on draft PRs.This test validates the core blocking behavior:
- Sets up a draft PR with a restricted allow-list
- Verifies disallowed command triggers a comment with helpful message
- Confirms no reaction is added (command didn't proceed)
The assertion checking the comment content for both the blocked command name and allowed commands list is thorough.
1282-1307: LGTM! Test verifies allowed commands proceed on draft PRs.Good coverage of the "happy path" where a command in the allow-list executes normally, including label addition and reaction.
1309-1337: LGTM! Test confirms empty list allows all commands.This test is critical for validating the
[]= "allow all" semantic described in the schema and PR objectives.
1339-1364: LGTM! Test ensures non-draft PRs bypass draft filtering.Important edge case—verifies that the draft PR config doesn't inadvertently affect non-draft PRs. The test explicitly sets
draft = Falseand confirms the command proceeds despite a restrictive allow-list.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
myakove-bot
added
commented-coderabbitai[bot]
lgtm-coderabbitai[bot]
and removed
changes-requested-coderabbitai[bot]
labels
Address CodeRabbit review comment - remove unnecessary # noqa: ARG001 from mock_command function in test_issue_comment_handler.py
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
webhook_server/tests/test_issue_comment_handler.py (1)
130-221: HIGH: Tight timing assertions risk flaky CI failures.
CI scheduling jitter often exceeds 10–15ms and <0.1s thresholds even when concurrency is correct; deterministic tests avoid red builds for non-functional reasons.🛠 Suggested refactor to reduce timing flakiness
- # VERIFICATION 2: Commands started concurrently (within 10ms of each other) - # In sequential execution, commands would start 50ms apart - # In parallel execution, all start nearly simultaneously - first_start = start_events[0][2] - last_start = start_events[-1][2] - start_time_spread = last_start - first_start - - # All commands should start within 10ms (parallel) - # vs 100ms+ for sequential execution (50ms * 2 delays) - assert start_time_spread < 0.015, f"Commands did not start concurrently (spread: {start_time_spread:.3f}s)" - - # VERIFICATION 3: Total execution time indicates parallel execution - # Sequential: 3 commands * 50ms = 150ms minimum - # Parallel: max(50ms) = 50ms (plus overhead) - # Allow 100ms for parallel (generous overhead buffer) - assert total_duration < 0.1, f"Execution took {total_duration:.3f}s, expected < 0.1s (parallel execution)" - - # Sequential would take at least 150ms - assert total_duration < 0.12, f"Commands appear to run sequentially ({total_duration:.3f}s >= 0.12s)" + # VERIFICATION 2: Commands overlap (parallel execution) + first_end = min(e[2] for e in execution_events if e[1] == "end") + last_start = max(e[2] for e in execution_events if e[1] == "start") + assert last_start < first_end, "Commands did not overlap (expected parallel execution)" + + # VERIFICATION 3: Keep a loose upper bound to catch fully sequential runs + assert total_duration < 0.5, f"Execution took {total_duration:.3f}s, expected < 0.5s"
♻️ Duplicate comments (1)
webhook_server/tests/test_issue_comment_handler.py (1)
147-152: MEDIUM: Ruff ARG001 — mark intentionally unused mock args.
Ruff flags unused parameters to keep lint noise out of the suite; prefix with_to document intent.✅ Suggested fix
- async def mock_command( - pull_request: Mock, - command: str, - reviewed_user: str, - issue_comment_id: int, - ) -> None: + async def mock_command( + _pull_request: Mock, + command: str, + _reviewed_user: str, + _issue_comment_id: int, + ) -> None:
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
webhook_server/tests/test_issue_comment_handler.py
🧰 Additional context used
📓 Path-based instructions (2)
webhook_server/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
webhook_server/**/*.py: Server fails-fast on startup if critical dependencies are missing - required parameters in__init__()are ALWAYS provided and checking for None on required parameters is pure overhead
Defensive checks in destructors (__del__) should use hasattr() to check for attributes that may not exist if initialization failed
Defensive checks are acceptable for optional parameters that explicitly allow None (parameter type includes| NoneorOptional)
Defensive checks are acceptable for lazy initialization - attribute explicitly starts as None with type annotationattr: SomeType | None = None
Use hasattr() checks only for platform constants that may not exist on all platforms (e.g.,os.O_NOFOLLOW)
Do NOT use hasattr() checks for required parameters in__init__()- if config is required, access it directly without defensive checks
Do NOT use hasattr() checks for library versions we control inpyproject.toml(PyGithub >=2.4.0, gql >=3.5.0) - call methods directly
Use isinstance() for type discrimination instead of hasattr() checks
Never return fake defaults (empty strings, zeros, False, None, empty collections) to hide missing data - fail-fast with ValueError or KeyError exceptions
PyGithub is synchronous/blocking - MUST wrap ALL method calls and property accesses withasyncio.to_thread()to avoid blocking the event loop
When accessing PyGithub properties that may trigger API calls, wrap inasyncio.to_thread(lambda: obj.property)instead of direct access
When iterating PyGithub PaginatedList, wrap iteration inasyncio.to_thread(lambda: list(...))to avoid blocking
Useasyncio.gather()for concurrent PyGithub operations to maximize performance
All imports must be at the top of files - no imports in functions or try/except blocks, except TYPE_CHECKING imports can be conditional
Complete type hints are mandatory - use mypy strict mode with full type annotations on function parameters and return types
Uselogger.exception()for autom...
Files:
webhook_server/tests/test_issue_comment_handler.py
webhook_server/tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
webhook_server/tests/**/*.py: Code coverage must be 90% or higher - new code without tests fails CI
Test files should use AsyncMock, Mock, and patch for mocking - use TEST_GITHUB_TOKEN constant for test tokens
Test coverage is required for new handlers - create handler tests inwebhook_server/tests/test_*_handler.py
Files:
webhook_server/tests/test_issue_comment_handler.py
🧠 Learnings (8)
📚 Learning: 2026-01-05T14:43:44.055Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T14:43:44.055Z
Learning: Applies to webhook_server/tests/**/*.py : Test coverage is required for new handlers - create handler tests in `webhook_server/tests/test_*_handler.py`
Applied to files:
webhook_server/tests/test_issue_comment_handler.py
📚 Learning: 2026-01-05T14:43:44.055Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T14:43:44.055Z
Learning: Applies to webhook_server/tests/**/*.py : Test files should use AsyncMock, Mock, and patch for mocking - use TEST_GITHUB_TOKEN constant for test tokens
Applied to files:
webhook_server/tests/test_issue_comment_handler.py
📚 Learning: 2026-01-05T14:43:44.055Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T14:43:44.055Z
Learning: Applies to webhook_server/tests/**/*.py : Code coverage must be 90% or higher - new code without tests fails CI
Applied to files:
webhook_server/tests/test_issue_comment_handler.py
📚 Learning: 2026-01-05T14:43:44.055Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T14:43:44.055Z
Learning: Applies to webhook_server/config/schema.yaml : Configuration schema changes should be tested with `uv run pytest webhook_server/tests/test_config_schema.py -v`
Applied to files:
webhook_server/tests/test_issue_comment_handler.py
📚 Learning: 2025-05-13T12:06:27.297Z
Learnt from: myakove
Repo: myk-org/github-webhook-server PR: 778
File: webhook_server/libs/pull_request_handler.py:327-330
Timestamp: 2025-05-13T12:06:27.297Z
Learning: In the GitHub webhook server, synchronous GitHub API calls (like create_issue_comment, add_to_assignees, etc.) in async methods should be awaited using asyncio.to_thread or loop.run_in_executor to prevent blocking the event loop.
Applied to files:
webhook_server/tests/test_issue_comment_handler.py
📚 Learning: 2024-10-08T09:19:56.185Z
Learnt from: myakove
Repo: myk-org/github-webhook-server PR: 586
File: webhook_server_container/libs/github_api.py:1947-1956
Timestamp: 2024-10-08T09:19:56.185Z
Learning: In `webhook_server_container/libs/github_api.py`, the indentation style used in the `set_pull_request_automerge` method is acceptable as per the project's coding standards.
Applied to files:
webhook_server/tests/test_issue_comment_handler.py
📚 Learning: 2024-10-29T08:09:57.157Z
Learnt from: myakove
Repo: myk-org/github-webhook-server PR: 612
File: webhook_server_container/libs/github_api.py:2089-2100
Timestamp: 2024-10-29T08:09:57.157Z
Learning: In `webhook_server_container/libs/github_api.py`, when the function `_keep_approved_by_approvers_after_rebase` is called, existing approval labels have already been cleared after pushing new changes, so there's no need to check for existing approvals within this function.
Applied to files:
webhook_server/tests/test_issue_comment_handler.py
📚 Learning: 2025-12-29T13:09:56.231Z
Learnt from: rnetser
Repo: myk-org/github-webhook-server PR: 959
File: webhook_server/libs/handlers/pull_request_handler.py:887-887
Timestamp: 2025-12-29T13:09:56.231Z
Learning: In the myk-org/github-webhook-server repository, logging statements in the webhook_server codebase should use f-strings for interpolation. Do not suggest changing to lazy formatting with % (e.g., logger.debug('msg %s', var)) since that is an established style choice. When adding new log statements, prefer f"...{var}..." for readability and performance, and avoid string concatenation or format-style logging unless the project explicitly adopts a different convention.
Applied to files:
webhook_server/tests/test_issue_comment_handler.py
🧬 Code graph analysis (1)
webhook_server/tests/test_issue_comment_handler.py (3)
webhook_server/libs/config.py (1)
get_value(132-153)webhook_server/libs/handlers/issue_comment_handler.py (2)
IssueCommentHandler(46-508)user_commands(150-341)webhook_server/utils/constants.py (1)
REACTIONS(129-137)
🪛 Ruff (0.14.11)
webhook_server/tests/test_issue_comment_handler.py
148-148: Unused function argument: pull_request
(ARG001)
150-150: Unused function argument: reviewed_user
(ARG001)
151-151: Unused function argument: issue_comment_id
(ARG001)
🔇 Additional comments (10)
webhook_server/tests/test_issue_comment_handler.py (10)
45-47: LOW: Draft-PR config mock keeps tests deterministic.
Centralizingconfig.get_valuein the fixture reduces per-test setup and prevents drift in draft-PR gating behavior.
72-76: LOW: Comment-processing test signatures are now consistent.
Addingselfexplicitly aligns with class-based pytest conventions and keeps the suite uniform.Also applies to: 85-88, 97-100, 118-121
230-234: LOW: user_commands path coverage with keyword-arg calls looks solid.
Keyword-arg expectations and consolidated patch contexts make intent clearer and align with handler signatures.Also applies to: 274-289, 296-311, 318-333, 340-359, 375-378, 386-407
432-449: LOW: Label/title edge-case coverage is thorough.
The WIP/HOLD/VERIFIED tests now cover idempotency and prefix edge cases, which are the common sources of regressions.Also applies to: 457-476, 484-503, 511-530, 538-557, 584-599, 610-625, 632-653, 660-681
691-699: LOW: Async mocks and keyword-arg assertions are clean here.
Nice alignment with handler signatures and async behavior in label/reviewer helper tests.Also applies to: 718-720, 735-736, 741-744
760-762: LOW: Cherry-pick tests now validate multi-branch behavior.
Asserting per-branch runner calls and labels gives confidence in fan‑out logic.Also applies to: 771-779, 787-789, 810-831, 835-885
899-902: LOW: Retest scenarios and permission gating are well covered.
Good coverage for “all”, unsupported tests, and user validation paths.Also applies to: 906-909, 928-935, 948-951, 973-987, 990-1010
1013-1016: LOW: Reprocess command coverage is comprehensive.
Registration, authorization, and reaction side effects are all explicitly asserted.Also applies to: 1029-1032, 1049-1081, 1088-1120, 1127-1150, 1173-1176
1179-1202: LOW: Regenerate‑welcome tests align with the new handler signature.
Explicit reaction assertions and “args ignored” paths look good.Also applies to: 1205-1226, 1228-1253
1254-1280: LOW: Draft‑PR allow‑list scenarios are covered end‑to‑end.
Blocked, allowed, empty‑list, and non‑draft behaviors are all exercised and match the config semantics.Also applies to: 1282-1307, 1310-1338, 1340-1364
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
- Prefix unused mock parameters with underscore (Ruff ARG001) - Loosen timing thresholds in parallel execution tests for CI stability
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
webhook_server/tests/test_issue_comment_handler.py (1)
144-189: LOW: Use a monotonic clock for concurrency timing assertions.Wall‑clock time can jump under NTP/VM adjustments and make timing-based tests flaky;
time.perf_counter()is monotonic and purpose-built for duration checks. Also, using min/max avoids any ordering assumptions in the event list.Suggested update
- start_time = time.time() + start_time = time.perf_counter() execution_events.append((command, "start", start_time)) @@ - execution_events.append((command, "error", time.time())) + execution_events.append((command, "error", time.perf_counter())) raise ValueError(f"Simulated error in {command}") - execution_events.append((command, "end", time.time())) + execution_events.append((command, "end", time.perf_counter())) @@ - start = time.time() + start = time.perf_counter() with pytest.raises(RuntimeError, match="Command /approved failed"): await issue_comment_handler.process_comment_webhook_data(Mock()) - total_duration = time.time() - start + total_duration = time.perf_counter() - start @@ - first_start = start_events[0][2] - last_start = start_events[-1][2] - start_time_spread = last_start - first_start + start_times = [e[2] for e in start_events] + start_time_spread = max(start_times) - min(start_times)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
webhook_server/tests/test_issue_comment_handler.py
🧰 Additional context used
📓 Path-based instructions (2)
webhook_server/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
webhook_server/**/*.py: Server fails-fast on startup if critical dependencies are missing - required parameters in__init__()are ALWAYS provided and checking for None on required parameters is pure overhead
Defensive checks in destructors (__del__) should use hasattr() to check for attributes that may not exist if initialization failed
Defensive checks are acceptable for optional parameters that explicitly allow None (parameter type includes| NoneorOptional)
Defensive checks are acceptable for lazy initialization - attribute explicitly starts as None with type annotationattr: SomeType | None = None
Use hasattr() checks only for platform constants that may not exist on all platforms (e.g.,os.O_NOFOLLOW)
Do NOT use hasattr() checks for required parameters in__init__()- if config is required, access it directly without defensive checks
Do NOT use hasattr() checks for library versions we control inpyproject.toml(PyGithub >=2.4.0, gql >=3.5.0) - call methods directly
Use isinstance() for type discrimination instead of hasattr() checks
Never return fake defaults (empty strings, zeros, False, None, empty collections) to hide missing data - fail-fast with ValueError or KeyError exceptions
PyGithub is synchronous/blocking - MUST wrap ALL method calls and property accesses withasyncio.to_thread()to avoid blocking the event loop
When accessing PyGithub properties that may trigger API calls, wrap inasyncio.to_thread(lambda: obj.property)instead of direct access
When iterating PyGithub PaginatedList, wrap iteration inasyncio.to_thread(lambda: list(...))to avoid blocking
Useasyncio.gather()for concurrent PyGithub operations to maximize performance
All imports must be at the top of files - no imports in functions or try/except blocks, except TYPE_CHECKING imports can be conditional
Complete type hints are mandatory - use mypy strict mode with full type annotations on function parameters and return types
Uselogger.exception()for autom...
Files:
webhook_server/tests/test_issue_comment_handler.py
webhook_server/tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
webhook_server/tests/**/*.py: Code coverage must be 90% or higher - new code without tests fails CI
Test files should use AsyncMock, Mock, and patch for mocking - use TEST_GITHUB_TOKEN constant for test tokens
Test coverage is required for new handlers - create handler tests inwebhook_server/tests/test_*_handler.py
Files:
webhook_server/tests/test_issue_comment_handler.py
🧠 Learnings (6)
📚 Learning: 2026-01-05T14:43:44.055Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T14:43:44.055Z
Learning: Applies to webhook_server/tests/**/*.py : Test coverage is required for new handlers - create handler tests in `webhook_server/tests/test_*_handler.py`
Applied to files:
webhook_server/tests/test_issue_comment_handler.py
📚 Learning: 2026-01-05T14:43:44.055Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T14:43:44.055Z
Learning: Applies to webhook_server/tests/**/*.py : Test files should use AsyncMock, Mock, and patch for mocking - use TEST_GITHUB_TOKEN constant for test tokens
Applied to files:
webhook_server/tests/test_issue_comment_handler.py
📚 Learning: 2025-05-13T12:06:27.297Z
Learnt from: myakove
Repo: myk-org/github-webhook-server PR: 778
File: webhook_server/libs/pull_request_handler.py:327-330
Timestamp: 2025-05-13T12:06:27.297Z
Learning: In the GitHub webhook server, synchronous GitHub API calls (like create_issue_comment, add_to_assignees, etc.) in async methods should be awaited using asyncio.to_thread or loop.run_in_executor to prevent blocking the event loop.
Applied to files:
webhook_server/tests/test_issue_comment_handler.py
📚 Learning: 2024-10-08T09:19:56.185Z
Learnt from: myakove
Repo: myk-org/github-webhook-server PR: 586
File: webhook_server_container/libs/github_api.py:1947-1956
Timestamp: 2024-10-08T09:19:56.185Z
Learning: In `webhook_server_container/libs/github_api.py`, the indentation style used in the `set_pull_request_automerge` method is acceptable as per the project's coding standards.
Applied to files:
webhook_server/tests/test_issue_comment_handler.py
📚 Learning: 2024-10-29T08:09:57.157Z
Learnt from: myakove
Repo: myk-org/github-webhook-server PR: 612
File: webhook_server_container/libs/github_api.py:2089-2100
Timestamp: 2024-10-29T08:09:57.157Z
Learning: In `webhook_server_container/libs/github_api.py`, when the function `_keep_approved_by_approvers_after_rebase` is called, existing approval labels have already been cleared after pushing new changes, so there's no need to check for existing approvals within this function.
Applied to files:
webhook_server/tests/test_issue_comment_handler.py
📚 Learning: 2025-12-29T13:09:56.231Z
Learnt from: rnetser
Repo: myk-org/github-webhook-server PR: 959
File: webhook_server/libs/handlers/pull_request_handler.py:887-887
Timestamp: 2025-12-29T13:09:56.231Z
Learning: In the myk-org/github-webhook-server repository, logging statements in the webhook_server codebase should use f-strings for interpolation. Do not suggest changing to lazy formatting with % (e.g., logger.debug('msg %s', var)) since that is an established style choice. When adding new log statements, prefer f"...{var}..." for readability and performance, and avoid string concatenation or format-style logging unless the project explicitly adopts a different convention.
Applied to files:
webhook_server/tests/test_issue_comment_handler.py
🔇 Additional comments (11)
webhook_server/tests/test_issue_comment_handler.py (11)
45-47: LOW: Draft‑PR config mock wiring looks correct.Defaulting
config.get_valuetoNonepreserves the “blocked by default” path, which makes the fixture a stable baseline for the new draft‑PR tests.
72-121: LOW: Updated pytest method signatures are consistent.Adding
selfexplicitly across these class-based tests keeps typing and pytest expectations aligned.
279-413: LOW: Command handler test updates look solid.The consolidated patch context managers and explicit call assertions make these command-path tests clearer and more maintainable.
432-563: LOW: WIP add/remove/idempotency cases are well covered.Nice coverage of prefix edge cases and no-op behavior.
565-687: LOW: Hold/verified command paths are validated cleanly.The tests exercise both add/remove flows and expected reactions in a straightforward way.
688-762: LOW: Custom label/reaction/reviewer tests align with handler contracts.Assertions are updated to match the current handler signatures and behaviors.
764-896: LOW: Cherry‑pick branch coverage is thorough.The multi‑branch assertions make the labeling and cherry‑pick behavior unambiguous.
897-1016: LOW: Retest error/permission paths are well exercised.Good coverage for invalid users, unsupported tests, and task exceptions.
1017-1182: LOW: Reprocess command coverage looks comprehensive.The tests validate authorization behavior and reaction posting clearly.
1183-1257: LOW: Regenerate‑welcome command tests look good.Nice to see the registration and reaction behavior asserted.
1259-1369: LOW: Draft‑PR allow‑list scenarios are well covered.Blocked, allowed, empty‑list (allow‑all), and non‑draft cases are all exercised.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
Sanitize allow-commands-on-draft-prs list entries to strings before join/comparison to prevent TypeError with non-string config values. Increase timing thresholds in memory optimization tests from 5s to 10s and adjust entries_per_second from 400 to 200 to fix flaky tests on slow CI runners.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@webhook_server/libs/handlers/issue_comment_handler.py`:
- Around line 149-151: Change the user_commands signature to make is_draft a
keyword-only parameter to avoid boolean positional misuse: update the method
definition of user_commands(self, pull_request: PullRequest, command: str,
reviewed_user: str, issue_comment_id: int, *, is_draft: bool) -> None so callers
must pass is_draft=...; ensure the function body and any internal references
remain unchanged and verify existing callers (e.g., the call at line 104)
already use the keyword form so no call-site edits are required.
Adds * before is_draft in user_commands signature to prevent accidental positional boolean misuse.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@webhook_server/libs/handlers/issue_comment_handler.py`:
- Around line 168-185: The draft-PR command check currently treats non-list or
missing allow-commands-on-draft-prs as "no restriction" and thus allows commands
— change it so only a proper list is accepted and any missing/mistyped value
blocks commands: fetch allow_commands_on_draft via
github_webhook.config.get_value, if it is None treat as "no commands allowed on
drafts" and post a comment via pull_request.create_issue_comment rejecting the
command; if it exists but is not a list, log a warning with
self.logger.debug/self.log_prefix and likewise post a rejection comment telling
admins to set a list; only when allow_commands_on_draft is a list should you
sanitize entries to strings and apply the current membership check against
_command (with the existing early return when not allowed).
| # Check if command is allowed on draft PRs | ||
| if is_draft: | ||
| allow_commands_on_draft = self.github_webhook.config.get_value("allow-commands-on-draft-prs") | ||
| # Empty list means all commands allowed; non-empty list means only those commands | ||
| if isinstance(allow_commands_on_draft, list) and len(allow_commands_on_draft) > 0: | ||
| # Sanitize: ensure all entries are strings for safe join and comparison | ||
| allow_commands_on_draft = [str(cmd) for cmd in allow_commands_on_draft] | ||
| if _command not in allow_commands_on_draft: | ||
| self.logger.debug( | ||
| f"{self.log_prefix} Command {_command} is not allowed on draft PRs. " | ||
| f"Allowed commands: {allow_commands_on_draft}" | ||
| ) | ||
| await asyncio.to_thread( | ||
| pull_request.create_issue_comment, | ||
| f"Command `/{_command}` is not allowed on draft PRs.\n" | ||
| f"Allowed commands on draft PRs: {', '.join(allow_commands_on_draft)}", | ||
| ) | ||
| return |
There was a problem hiding this comment.
HIGH: Non-list config values fail open and allow all draft commands.
If allow-commands-on-draft-prs is unset or mistyped (e.g., a string), the current guard is skipped and all commands run on draft PRs—contradicting the default “block” behavior and widening permissions on misconfig. Consider explicitly blocking when unset and rejecting non-lists.
🛠️ Proposed fix
if is_draft:
allow_commands_on_draft = self.github_webhook.config.get_value("allow-commands-on-draft-prs")
+ if allow_commands_on_draft is None:
+ self.logger.debug(
+ f"{self.log_prefix} Draft PR commands are disabled (allow-commands-on-draft-prs not set)."
+ )
+ return
+ if not isinstance(allow_commands_on_draft, list):
+ raise ValueError(
+ f"allow-commands-on-draft-prs must be a list of command strings, "
+ f"got {type(allow_commands_on_draft).__name__}"
+ )
# Empty list means all commands allowed; non-empty list means only those commands
- if isinstance(allow_commands_on_draft, list) and len(allow_commands_on_draft) > 0:
+ if len(allow_commands_on_draft) > 0:
# Sanitize: ensure all entries are strings for safe join and comparison
allow_commands_on_draft = [str(cmd) for cmd in allow_commands_on_draft]
if _command not in allow_commands_on_draft:
self.logger.debug(
f"{self.log_prefix} Command {_command} is not allowed on draft PRs. "
f"Allowed commands: {allow_commands_on_draft}"
)🤖 Prompt for AI Agents
In `@webhook_server/libs/handlers/issue_comment_handler.py` around lines 168 -
185, The draft-PR command check currently treats non-list or missing
allow-commands-on-draft-prs as "no restriction" and thus allows commands —
change it so only a proper list is accepted and any missing/mistyped value
blocks commands: fetch allow_commands_on_draft via
github_webhook.config.get_value, if it is None treat as "no commands allowed on
drafts" and post a comment via pull_request.create_issue_comment rejecting the
command; if it exists but is not a list, log a warning with
self.logger.debug/self.log_prefix and likewise post a rejection comment telling
admins to set a list; only when allow_commands_on_draft is a list should you
sanitize entries to strings and apply the current membership check against
_command (with the existing early return when not allowed).
…feat/issue-979-allow-commands-on-draft-prs
myakove-bot
added
needs-rebase
and removed
changes-requested-coderabbitai[bot]
needs-rebase
labels
|
@cursor review |
|
Skipping Bugbot: Bugbot is disabled for this repository |
2 participants
Summary
Adds a new configuration option
allow-commands-on-draft-prsthat allows specific commands (or all commands) to be executed on draft PRs via issue comments.Configuration Options
Changes
allow-commands-on-draft-prsconfig option to schema (global and per-repo levels)github_api.pyto allowissue_commentevents when config is setIssueCommentHandlerwith type guardexamples/config.yamlCloses #979
Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Behavior
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.