feat(commands): add allow-commands-on-draft-prs config option#980
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. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a config-driven allow-list to control which commands may run on draft PRs (global and per-repo). Draft PRs remain short‑circuited unless an allow-list is configured and the event is 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 labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
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 |
![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
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.
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.
myakove-bot
added
lgtm-coderabbitai[bot]
commented-coderabbitai[bot]
needs-rebase
and removed
commented-coderabbitai[bot]
labels
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).
|
@cursor review |
|
Skipping Bugbot: Bugbot is disabled for this repository |
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
Address CodeRabbit review comment - remove unnecessary # noqa: ARG001 from mock_command function in test_issue_comment_handler.py
- Prefix unused mock parameters with underscore (Ruff ARG001) - Loosen timing thresholds in parallel execution tests for CI stability
- Add type validation for allow-commands-on-draft-prs config in github_api.py Logs warning and treats non-list values as None (default-deny) - Cache PR draft status in issue_comment_handler.py Fetches is_draft once per comment instead of per-command to avoid repeated API calls
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.
Adds * before is_draft in user_commands signature to prevent accidental positional boolean misuse.
myakove
force-pushed
the
feat/issue-979-allow-commands-on-draft-prs
branch
from
5e594ce to
7ca9dd0
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@webhook_server/config/schema.yaml`:
- Around line 344-352: Add schema validation tests for the new
allow-commands-on-draft-prs property in webhook_server/config/schema.yaml by
updating webhook_server/tests/test_config_schema.py: add cases to the
valid_full_config fixture and/or new test functions that assert the schema
accepts a valid array of strings for allow-commands-on-draft-prs, accepts an
empty array (meaning “all commands allowed”), accepts omission of the property
(default behavior), and rejects invalid types (e.g., non-array values). Run and
verify with uv run pytest webhook_server/tests/test_config_schema.py -v to
ensure the validator enforces array typing and the described semantics for
allow-commands-on-draft-prs.
In `@webhook_server/libs/handlers/issue_comment_handler.py`:
- Around line 168-185: The handler fetches allow_commands_on_draft via
self.github_webhook.config.get_value and then assumes it's a list; add a
one-line guard after that call to handle a None value (e.g., if
allow_commands_on_draft is None: return) so the draft-PR command check in
issue_comment_handler.py (around is_draft, allow_commands_on_draft, _command and
pull_request.create_issue_comment) does not accidentally allow commands when the
config is missing.
…chema tests - Add guard when allow-commands-on-draft-prs config is None to block commands - Add schema validation tests for allow-commands-on-draft-prs property - Add test for draft PR no-config blocks all commands
|
New container for ghcr.io/myk-org/github-webhook-server:latest published |
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