feat(labels): add configurable labels with enable/disable and custom colors#978
feat(labels): add configurable labels with enable/disable and custom colors#978
Conversation
WalkthroughAdds configurable label support: global and per-repository Changes
Sequence Diagram(s)sequenceDiagram
participant Config as ConfigLoader
participant Webhook as GithubWebhook
participant Labels as LabelsHandler
participant PR as PullRequestHandler
participant Git as Git/GitHub
Config->>Webhook: load global + per-repo `labels` config
Webhook->>Webhook: merge configs -> enabled_labels, label_colors
Webhook->>Labels: provide enabled_labels & label_colors
PR->>Labels: request add/remove label for PR event
Labels->>Labels: is_label_enabled(label) -> allow/deny
alt allowed
Labels->>Labels: resolve color (configured exact → prefix → size → default)
Labels->>Git: create/update label with chosen color
Labels->>PR: return boolean indicating change
PR->>PR: perform title/welcome/verify updates only if change
else denied
Labels-->>PR: skip (no-change)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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 |
|
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. |
|
/hold |
…colors - Add labels schema to schema.yaml (global + per-repo levels) - Add enabled_labels and label_colors config loading in github_api.py - Add is_label_enabled() method in labels_handler.py - Update _get_label_color() to use configured colors - Add validation for enabled-labels with warning for invalid categories - Fix infinite recursion bug in _get_custom_pr_size_thresholds() - Use ALL_LABELS_DICT for DEFAULT_LABEL_COLORS to eliminate duplication - Add comprehensive tests for label configuration - Update examples/config.yaml with labels examples Note: reviewed-by labels (approved-*, lgtm-*, etc.) are always enabled and cannot be disabled as they are the source of truth for the approval system. Closes #976
|
/hold |
1 similar comment
|
/hold |
![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_config_schema.py:
- Around line 645-680: Add two additional test cases alongside
test_labels_configuration_schema to cover edge cases: one that writes a config
with an invalid label category in "labels":"enabled-labels" (e.g.,
"invalid-category") and asserts that constructing Config() raises a validation
error or the schema rejects it, and another that writes a config with an empty
"labels":"enabled-labels" array and asserts the resulting Config() behavior
(either accepted with empty list or defaults applied). Use the same helpers
(create_temp_config_dir_and_data, monkeypatch to set WEBHOOK_SERVER_DATA_DIR)
and reference the Config class to load the config; ensure cleanup with
shutil.rmtree in finally blocks.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (7)
examples/config.yamlwebhook_server/config/schema.yamlwebhook_server/libs/github_api.pywebhook_server/libs/handlers/labels_handler.pywebhook_server/tests/test_config_schema.pywebhook_server/tests/test_labels_handler.pywebhook_server/utils/constants.py
🧰 Additional context used
📓 Path-based instructions (7)
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/labels_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/labels_handler.pywebhook_server/utils/constants.pywebhook_server/tests/test_config_schema.pywebhook_server/tests/test_labels_handler.pywebhook_server/libs/github_api.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/labels_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_config_schema.pywebhook_server/tests/test_labels_handler.py
**/*.{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:
examples/config.yamlwebhook_server/config/schema.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/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
🧠 Learnings (11)
📚 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/handlers/labels_handler.pywebhook_server/libs/github_api.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/libs/handlers/labels_handler.pywebhook_server/utils/constants.pywebhook_server/tests/test_config_schema.pywebhook_server/tests/test_labels_handler.pywebhook_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/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/utils/constants.pywebhook_server/tests/test_config_schema.pywebhook_server/tests/test_labels_handler.pywebhook_server/config/schema.yamlwebhook_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/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_labels_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_labels_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_labels_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: 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: 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/**/*.py : Access configuration using `Config(repository='org/repo-name')` and `config.get_value('setting-name', default_value)`
Applied to files:
webhook_server/libs/github_api.py
📚 Learning: 2025-10-30T00:18:06.176Z
Learnt from: myakove
Repo: myk-org/github-webhook-server PR: 878
File: webhook_server/libs/github_api.py:111-118
Timestamp: 2025-10-30T00:18:06.176Z
Learning: In webhook_server/libs/github_api.py, when creating temporary directories or performing operations that need repository names, prefer using self.repository_name (from webhook payload, always available) over dereferencing self.repository.name or self.repository_by_github_app.name, which may be None. This avoids AttributeError and keeps the code simple and reliable.
Applied to files:
webhook_server/libs/github_api.py
📚 Learning: 2024-11-21T16:10:01.777Z
Learnt from: myakove
Repo: myk-org/github-webhook-server PR: 624
File: webhook_server_container/libs/github_api.py:2067-2101
Timestamp: 2024-11-21T16:10:01.777Z
Learning: In `webhook_server_container/libs/github_api.py`, keep the `max_owners_files` limit as a hardcoded value rather than making it configurable through the config.
Applied to files:
webhook_server/libs/github_api.py
🧬 Code graph analysis (3)
webhook_server/tests/test_config_schema.py (1)
webhook_server/libs/config.py (2)
Config(11-142)root_data(33-50)
webhook_server/tests/test_labels_handler.py (1)
webhook_server/libs/handlers/labels_handler.py (3)
_add_label(132-166)is_label_enabled(49-107)_get_label_color(180-218)
webhook_server/libs/github_api.py (1)
webhook_server/libs/config.py (1)
get_value(99-120)
🪛 Ruff (0.14.10)
webhook_server/libs/handlers/labels_handler.py
140-140: Logging statement uses f-string
(G004)
webhook_server/libs/github_api.py
668-669: Logging statement uses f-string
(G004)
🔇 Additional comments (16)
webhook_server/config/schema.yaml (2)
86-118: LGTM! Well-structured global labels configuration schema.The schema correctly:
- Defines
enabled-labelsas an enum array matchingCONFIGURABLE_LABEL_CATEGORIES- Uses
additionalProperties: falseto prevent invalid keys- Documents that reviewed-by labels cannot be disabled
- Maintains backward compatibility (optional block)
335-367: Repository-level labels schema mirrors global structure correctly.The per-repository schema appropriately duplicates the global labels structure, enabling repository-specific overrides while maintaining consistent validation.
examples/config.yaml (2)
36-68: Excellent user-facing documentation for labels configuration.The example clearly demonstrates:
- All 10 configurable label categories
- CSS3 color names for both static labels and dynamic prefixes (e.g.,
approved-)- The behavior when labels config is unset (all enabled)
- That reviewed-by labels cannot be disabled
This will help users understand the feature quickly.
189-196: Good example of repository-level label override.Shows a focused subset (verified, hold, size) with a custom color override, demonstrating how repository config takes precedence.
webhook_server/utils/constants.py (2)
78-80: Clean approach using alias to avoid duplication.
DEFAULT_LABEL_COLORS = ALL_LABELS_DICTmaintains a single source of truth. Since both are used read-only throughout the codebase, the alias is safe.
82-95: Well-defined configurable categories constant.The set correctly enumerates all 10 configurable label categories matching the schema enum. The comment explaining why
reviewed-byis excluded (cannot be disabled) is helpful for maintainability.webhook_server/libs/github_api.py (2)
651-674: Correct labels configuration loading and validation.The implementation properly:
- Merges global and repository configs (repo overrides global)
- Validates
enabled-labelsagainstCONFIGURABLE_LABEL_CATEGORIES- Logs warnings for invalid categories without failing (forward compatible)
- Sets
enabled_labels = Nonewhen unconfigured (all labels enabled semantic)One minor observation: the
or {}on lines 652 and 654 handles bothNoneand empty dict returns, which is defensive but appropriate here.
661-671: Behavior confirmed: empty enabled-labels array is intentional, not an edge case requiring handling.Tests explicitly validate that
enabled-labels: []produces an empty set, disabling all configurable labels while preserving reviewed-by labels (test_labels_handler.py lines 1228–1233, test_reviewed_by_always_enabled). This is differentiated from "not configured" (None), which enables all labels. No changes needed.webhook_server/libs/handlers/labels_handler.py (4)
49-107: Well-implemented label enablement logic.The
is_label_enabledmethod correctly handles:
- reviewed-by labels (always enabled, cannot be disabled)
Nonemeans all labels enabled (backward compatible)- Static labels mapped to categories
- Dynamic labels (size/, branch-, cherry-pick-) checked by prefix
One detail on line 103:
CHERRY_PICKED_LABEL_PREFIXis checked with==whileCHERRY_PICK_LABEL_PREFIXusesstartswith. This is correct sinceCherryPickedis an exact label name, not a prefix pattern.
139-141: Correct guard for disabled labels.The early return when
is_label_enabled(label)is False prevents disabled labels from being added. The debug log helps with troubleshooting.
180-218: Color resolution logic with correct priority.The
_get_label_colormethod follows the right priority:
- Custom colors (exact match)
- Custom colors (prefix match for dynamic labels)
- Size threshold colors
- DEFAULT_LABEL_COLORS (static and dynamic)
- Fallback
D4C5F9This ensures user configuration takes precedence over defaults.
275-283: Critical fix: Prevents infinite recursion.Previously, when no valid thresholds were found after parsing custom config, calling
_get_custom_pr_size_thresholds()recursively would loop forever. Returning static defaults directly fixes this.webhook_server/tests/test_labels_handler.py (4)
54-57: Fixture properly initializes new label config attributes.Setting
enabled_labels = Noneandlabel_colors = {}as defaults ensures existing tests work unchanged (all labels enabled, default colors), which is the backward-compatible behavior.
159-171: Good test for label disable behavior.Tests that when
enabled_labels = {"size"}, theholdlabel is not added. The test correctly:
- Configures only size category as enabled
- Mocks
label_exists_in_pull_requestto isolate the behavior- Verifies
add_to_labelsis never called
1153-1255: Comprehensive test suite for is_label_enabled.Excellent coverage including:
- All labels enabled when not configured (None)
- reviewed-by labels always enabled (even with empty set)
- Category-based enablement (branch, cherry-pick, automerge, size)
- Unknown labels allowed by default
- Empty
enabled_labelsdisables configurable labelsThis validates the core business rule that reviewed-by labels cannot be disabled.
1257-1305: Good coverage of custom label colors.Tests verify:
- Custom colors override defaults for static labels
- Prefix-based custom colors work for dynamic labels (e.g.,
approved-)- Fallback to defaults when no custom color configured
- Invalid color names fall back to lightgray
|
/hold |
1 similar comment
|
/hold |
|
/verified |
|
/regenerate-welcome |
|
/verified |
|
/regenerate-welcome |
myakove-bot
added
commented-coderabbitai[bot]
lgtm-coderabbitai[bot]
and removed
changes-requested-coderabbitai[bot]
labels
myakove-bot
added
lgtm-coderabbitai[bot]
and removed
commented-myakove
commented-coderabbitai[bot]
lgtm-coderabbitai[bot]
labels
- Fix mock_run_command signature to capture positional args - Remove unused noqa: F401 directive - Add OSError protection to _open() call in doRollover - Remove unused minimal_hook_data fixture parameter
There was a problem hiding this comment.
Actionable comments posted: 4
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_github_api.py (1)
1501-1505: LOW: RedundantHeaderswrapping.
minimal_headersis already aHeadersobject (from fixture at line 73-74). Wrapping it again is unnecessary.♻️ Suggested fix
# Create webhook webhook = GithubWebhook( hook_data=minimal_hook_data, - headers=Headers(minimal_headers), + headers=minimal_headers, logger=logger, )
🤖 Fix all issues with AI agents
In @webhook_server/tests/test_github_api.py:
- Around line 1199-1204: The mock async function mock_run_command currently
declares a log_prefix parameter but doesn't use it, triggering Ruff ARG001; fix
by either renaming the parameter to _log_prefix: str in the mock_run_command
signature or by adding a discard assignment inside the function body like _ =
log_prefix so the parameter is explicitly used and the linter warning is
suppressed; update mock_run_command accordingly to match the handling used
elsewhere.
- Around line 1309-1314: The mock_run_command async function declares an unused
parameter log_prefix; rename it to _log_prefix (or prefix with an underscore) in
the mock_run_command signature to silence unused-parameter warnings and update
any references accordingly so the function remains async def
mock_run_command(command: str, _log_prefix: str, **_kwargs: object) ->
tuple[bool, str, str]: with no other behavior changes.
- Around line 1253-1258: The mock_run_command async function has an unused
parameter log_prefix; rename it to _log_prefix (or prefix it with an underscore)
in the mock_run_command signature to mirror the fix at line 1377 so linters
recognize it as intentionally unused, and update any local references
accordingly (keep function name mock_run_command and its return behavior
unchanged).
In @webhook_server/tests/test_safe_rotating_handler.py:
- Around line 221-232: The test TestSafeRotatingHandlerPatch is brittle because
it assumes webhook_server.utils.helpers was imported elsewhere to apply the
patch; explicitly import webhook_server.utils.helpers before asserting that
simple_logger.logger.RotatingFileHandler is SafeRotatingFileHandler so the
SafeRotatingFileHandler patch is applied deterministically (e.g., add an import
of webhook_server.utils.helpers at module top or at start of
test_simple_logger_uses_safe_handler).
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
webhook_server/tests/test_github_api.pywebhook_server/tests/test_safe_rotating_handler.pywebhook_server/utils/safe_rotating_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/utils/safe_rotating_handler.pywebhook_server/tests/test_safe_rotating_handler.pywebhook_server/tests/test_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_safe_rotating_handler.pywebhook_server/tests/test_github_api.py
🧠 Learnings (21)
📓 Common learnings
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.
📚 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/utils/safe_rotating_handler.pywebhook_server/tests/test_safe_rotating_handler.pywebhook_server/tests/test_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/utils/safe_rotating_handler.pywebhook_server/tests/test_safe_rotating_handler.pywebhook_server/tests/test_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/utils/safe_rotating_handler.pywebhook_server/tests/test_safe_rotating_handler.pywebhook_server/tests/test_github_api.py
📚 Learning: 2025-05-10T22:07:53.544Z
Learnt from: myakove
Repo: myk-org/github-webhook-server PR: 773
File: webhook_server/libs/pull_request_handler.py:553-557
Timestamp: 2025-05-10T22:07:53.544Z
Learning: In the GitHub webhook server codebase, `root_approvers` and `root_reviewers` are properties of the GithubWebhook class, not methods, so they can be used with `.copy()` without parentheses.
Applied to files:
webhook_server/utils/safe_rotating_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/utils/safe_rotating_handler.pywebhook_server/tests/test_safe_rotating_handler.pywebhook_server/tests/test_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/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_safe_rotating_handler.pywebhook_server/tests/test_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/tests/**/*.py : Code coverage must be 90% or higher - new code without tests fails CI
Applied to files:
webhook_server/tests/test_safe_rotating_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_safe_rotating_handler.pywebhook_server/tests/test_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/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_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 : Maintain backward compatibility for webhook payload handling - must follow GitHub webhook specification
Applied to files:
webhook_server/tests/test_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/tests/test_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/**/*.py : PyGithub is synchronous/blocking - MUST wrap ALL method calls and property accesses with `asyncio.to_thread()` to avoid blocking the event loop
Applied to files:
webhook_server/tests/test_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/**/*.py : Do NOT use hasattr() checks for library versions we control in `pyproject.toml` (PyGithub >=2.4.0, gql >=3.5.0) - call methods directly
Applied to files:
webhook_server/tests/test_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/**/*.py : When iterating PyGithub PaginatedList, wrap iteration in `asyncio.to_thread(lambda: list(...))` to avoid blocking
Applied to files:
webhook_server/tests/test_github_api.py
📚 Learning: 2025-10-28T16:09:08.689Z
Learnt from: myakove
Repo: myk-org/github-webhook-server PR: 0
File: :0-0
Timestamp: 2025-10-28T16:09:08.689Z
Learning: For this repository, prioritize speed and minimizing API calls in reviews and suggestions: reuse webhook payload data, batch GraphQL queries, cache IDs (labels/users), and avoid N+1 patterns.
Applied to files:
webhook_server/tests/test_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/tests/test_github_api.py
📚 Learning: 2024-10-15T10:37:45.791Z
Learnt from: myakove
Repo: myk-org/github-webhook-server PR: 598
File: webhook_server_container/libs/github_api.py:1860-1874
Timestamp: 2024-10-15T10:37:45.791Z
Learning: In the `process_retest_command` method in `webhook_server_container/libs/github_api.py`, `_target_tests` is defined before use.
Applied to files:
webhook_server/tests/test_github_api.py
📚 Learning: 2024-10-09T09:16:45.452Z
Learnt from: myakove
Repo: myk-org/github-webhook-server PR: 579
File: webhook_server_container/libs/github_api.py:1098-1101
Timestamp: 2024-10-09T09:16:45.452Z
Learning: In the Python method `_run_tox` within `webhook_server_container/libs/github_api.py`, the variable `_tox_tests` is already comma-separated, so removing spaces with `_tox_tests.replace(" ", "")` is appropriate to handle any accidental spaces when specifying Tox environments.
Applied to files:
webhook_server/tests/test_github_api.py
📚 Learning: 2024-11-26T14:30:22.906Z
Learnt from: myakove
Repo: myk-org/github-webhook-server PR: 631
File: webhook_server_container/tests/test_github_api.py:321-327
Timestamp: 2024-11-26T14:30:22.906Z
Learning: In `webhook_server_container/tests/test_github_api.py`, when using `pytest.mark.parametrize` with indirect parameters, the `pytest.param` function may require nested lists to match the expected input structure.
Applied to files:
webhook_server/tests/test_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 with early exit when action != 'completed'
Applied to files:
webhook_server/tests/test_github_api.py
🧬 Code graph analysis (2)
webhook_server/tests/test_safe_rotating_handler.py (1)
webhook_server/utils/safe_rotating_handler.py (2)
SafeRotatingFileHandler(32-111)doRollover(45-111)
webhook_server/tests/test_github_api.py (2)
webhook_server/libs/config.py (2)
repository_local_data(89-130)get_value(132-153)webhook_server/libs/github_api.py (2)
GithubWebhook(80-878)process(408-592)
🪛 Ruff (0.14.11)
webhook_server/tests/test_safe_rotating_handler.py
203-203: Avoid specifying long messages outside the exception class
(TRY003)
webhook_server/tests/test_github_api.py
1200-1200: Unused function argument: log_prefix
(ARG001)
1254-1254: Unused function argument: log_prefix
(ARG001)
1310-1310: Unused function argument: log_prefix
(ARG001)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: can-be-merged
- GitHub Check: can-be-merged
- GitHub Check: can-be-merged
- GitHub Check: can-be-merged
- GitHub Check: can-be-merged
- GitHub Check: pre-commit
- GitHub Check: tox
- GitHub Check: conventional-title
- GitHub Check: build-container
- GitHub Check: python-module-install
🔇 Additional comments (14)
webhook_server/tests/test_github_api.py (6)
46-53: LGTM! Fixture correctly updated to reflect tag-based cloning behavior.The docstring and payload now accurately document that only tag pushes (not branch pushes) trigger cloning. This aligns with the PR's changes where
PushHandleronly processes tags for PyPI upload and container builds.
1332-1398: LGTM! Comprehensive test for PR ref fetch failure.This test properly validates that:
RuntimeErroris raised when PR ref fetch fails- The error message contains the PR number
- Error logging occurs
Good use of the
_ = log_prefixpattern at line 1377 to suppress the unused parameter warning.
1511-1586: LGTM! Good test coverage for tag checkout flow.The test verifies the correct git commands are executed for tag-based cloning:
- Tag refspec fetch:
refs/tags/v1.0.0:refs/tags/v1.0.0- Tag checkout:
checkout v1.0.0The flexible matching approach using list comprehensions is robust against potential git flag variations.
1588-1687: LGTM! Excellent test for fetch ordering verification.This test validates a critical invariant: base branch fetch must occur BEFORE PR ref fetch. The ordering check at lines 1677-1687 provides clear failure messages if the invariant is violated. This helps prevent subtle bugs where PR checkout might fail due to missing base branch.
1775-1835: LGTM! Test correctly updated to reflect branch push skip behavior.The renamed test and updated docstring accurately reflect that branch pushes skip cloning entirely (because
PushHandleronly processes tags for PyPI/container builds). The assertions properly verify:
_clone_repositoryis NOT calledPushHandler.process_push_webhook_datais NOT called- Returns
None
1903-1963: LGTM! Good test coverage for configurable labels feature.This test validates the new
enabled-labelssanitization logic by:
- Providing mixed input types (string, dict, list, int)
- Verifying warning is logged with descriptive type info (
dict(keys=...,list(len=2),int()- Confirming only valid string entries are retained in
enabled_labelsThe assertions at lines 1955-1960 provide clear validation of the warning message content.
webhook_server/utils/safe_rotating_handler.py (4)
1-24: LGTM! Excellent documentation of design rationale.The module docstring clearly explains WHY broad OSError suppression is intentional - logging infrastructure must never crash the application. This is a sound design choice for a logging handler, and the trade-off (some rotation failures may go unnoticed) is explicitly acknowledged.
45-63: LGTM! Clean stream handling with appropriate type annotation.The
type: ignore[assignment]on line 63 is necessary becauseRotatingFileHandler.streamisn't typed asOptionalin the base class, but setting it toNoneis the correct behavior here. The method signature has proper return type annotation.
65-81: LGTM! Backup rotation handles TOCTOU races correctly.The loop correctly iterates backwards to avoid overwriting files. The pattern of catching
FileNotFoundErrorspecifically (for documentation clarity) before the broaderOSErroris good practice. The TOCTOU race betweenexists()and the operation is acknowledged in comments and handled gracefully.
105-111: LGTM! Stream reopening gracefully degrades.When
_open()fails, leavingstreamasNoneis correct behavior. As documented,FileHandler.emit()will attempt to reopen on the next log entry, providing automatic recovery.webhook_server/tests/test_safe_rotating_handler.py (4)
18-42: LGTM! Good basic smoke test with proper resource cleanup.The test correctly triggers multiple rollovers by writing enough data and uses
try/finallyto ensure the handler is closed. This validates the happy path before testing edge cases.
68-106: MEDIUM: Mock patches target globalosmodule - works but is broader than necessary.The patches on
os.path.existsandos.removework correctly since they modify the functions in theosmodule directly. However, for more targeted testing, you could patch at the import location:webhook_server.utils.safe_rotating_handler.os.path.exists.Current approach is acceptable since tests are isolated and cleanup is handled properly.
147-184: LGTM! Windows compatibility handled correctly.Good approach using
mock_existsinstead of actually deleting the file while the handler's stream is open. The comment on lines 165-168 correctly documents why this approach is necessary for cross-platform compatibility.
186-218: LGTM! Comprehensive failure and recovery test.This test verifies both the failure scenario (stream is
Noneafter_open()fails) and the recovery scenario (handler remains usable after restoring normal behavior). The static analysis hint aboutTRY003on line 203 is acceptable - in test code, inline exception messages for simulating errors are fine.
myakove-bot
added
commented-myakove
commented-coderabbitai[bot]
and removed
changes-requested-coderabbitai[bot]
labels
- Fix mock_run_command parameter handling for log_prefix - Add explicit helpers import for test isolation - Remove redundant Headers() wrapping - Revert shallow clone to fix merge unrelated histories bug
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @webhook_server/libs/github_api.py:
- Around line 697-731: Refactor the local helper _sanitize_item inside the
enabled-labels handling to use early returns for each branch (dict, list, else)
instead of nested if/elif/else flow: keep the same max_repr_len logic and output
formats (e.g., "dict(keys=...)", "list(len=...)", and truncating repr for other
types), preserve creation of sanitized_dropped and the subsequent
self.logger.warning calls, and do not change how enabled_set, invalid, or
self.enabled_labels are computed—this is purely a readability refactor of
_sanitize_item with no behavioral changes.
In @webhook_server/tests/test_safe_rotating_handler.py:
- Line 12: The import statement "import webhook_server.utils.helpers" is
intentionally done for side effects but the "noqa: F401" is unnecessary; remove
the "noqa: F401" directive and replace it with a brief inline comment after the
import explaining that the module is imported to patch simple_logger (e.g.,
"Imported for side effects: patches simple_logger"), leaving the import line
itself unchanged.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
webhook_server/libs/github_api.pywebhook_server/tests/test_github_api.pywebhook_server/tests/test_safe_rotating_handler.py
🧰 Additional context used
📓 Path-based instructions (4)
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_safe_rotating_handler.pywebhook_server/libs/github_api.pywebhook_server/tests/test_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_safe_rotating_handler.pywebhook_server/tests/test_github_api.py
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/github_api.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
🧠 Learnings (30)
📓 Common learnings
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.
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.
📚 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_safe_rotating_handler.pywebhook_server/tests/test_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/tests/**/*.py : Code coverage must be 90% or higher - new code without tests fails CI
Applied to files:
webhook_server/tests/test_safe_rotating_handler.pywebhook_server/tests/test_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/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_safe_rotating_handler.pywebhook_server/tests/test_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/tests/test_safe_rotating_handler.pywebhook_server/libs/github_api.pywebhook_server/tests/test_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/tests/test_safe_rotating_handler.pywebhook_server/libs/github_api.pywebhook_server/tests/test_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/tests/test_safe_rotating_handler.pywebhook_server/libs/github_api.pywebhook_server/tests/test_github_api.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_safe_rotating_handler.pywebhook_server/libs/github_api.pywebhook_server/tests/test_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 : Maintain backward compatibility for webhook payload handling - must follow GitHub webhook specification
Applied to files:
webhook_server/libs/github_api.pywebhook_server/tests/test_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.pywebhook_server/tests/test_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/**/*.py : Internal methods in `webhook_server/libs/` can change freely - return types can change (e.g., `Any` → `bool`), method signatures can be modified without deprecation
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/**/*.py : When accessing PyGithub properties that may trigger API calls, wrap in `asyncio.to_thread(lambda: obj.property)` instead of direct access
Applied to files:
webhook_server/libs/github_api.py
📚 Learning: 2025-10-30T00:18:06.176Z
Learnt from: myakove
Repo: myk-org/github-webhook-server PR: 878
File: webhook_server/libs/github_api.py:111-118
Timestamp: 2025-10-30T00:18:06.176Z
Learning: In webhook_server/libs/github_api.py, when creating temporary directories or performing operations that need repository names, prefer using self.repository_name (from webhook payload, always available) over dereferencing self.repository.name or self.repository_by_github_app.name, which may be None. This avoids AttributeError and keeps the code simple and reliable.
Applied to files:
webhook_server/libs/github_api.pywebhook_server/tests/test_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/**/*.py : PyGithub is synchronous/blocking - MUST wrap ALL method calls and property accesses with `asyncio.to_thread()` to avoid blocking the event loop
Applied to files:
webhook_server/libs/github_api.pywebhook_server/tests/test_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/**/*.py : When iterating PyGithub PaginatedList, wrap iteration in `asyncio.to_thread(lambda: list(...))` to avoid blocking
Applied to files:
webhook_server/libs/github_api.py
📚 Learning: 2025-02-25T12:01:42.999Z
Learnt from: myakove
Repo: myk-org/github-webhook-server PR: 666
File: webhook_server_container/libs/github_api.py:2254-2255
Timestamp: 2025-02-25T12:01:42.999Z
Learning: In the ProcessGithubWehook class, the `self.conventional_title` attribute is set during initialization by the `_repo_data_from_config()` method, and doesn't need additional existence checks.
Applied to files:
webhook_server/libs/github_api.py
📚 Learning: 2025-10-28T16:09:08.689Z
Learnt from: myakove
Repo: myk-org/github-webhook-server PR: 0
File: :0-0
Timestamp: 2025-10-28T16:09:08.689Z
Learning: For this repository, prioritize speed and minimizing API calls in reviews and suggestions: reuse webhook payload data, batch GraphQL queries, cache IDs (labels/users), and avoid N+1 patterns.
Applied to files:
webhook_server/libs/github_api.pywebhook_server/tests/test_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.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 : Handler classes must implement `__init__(self, github_webhook, ...)` and `process_event(event_data)` pattern
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.pywebhook_server/tests/test_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 with early exit when action != 'completed'
Applied to files:
webhook_server/libs/github_api.pywebhook_server/tests/test_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/app.py : Use `create_context()` to initialize webhook context in app.py with hook_id, event_type, repository, action, sender, api_user
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/**/*.py : Access configuration using `Config(repository='org/repo-name')` and `config.get_value('setting-name', default_value)`
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/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_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/**/*.py : Do NOT use hasattr() checks for library versions we control in `pyproject.toml` (PyGithub >=2.4.0, gql >=3.5.0) - call methods directly
Applied to files:
webhook_server/tests/test_github_api.py
📚 Learning: 2024-10-15T10:37:45.791Z
Learnt from: myakove
Repo: myk-org/github-webhook-server PR: 598
File: webhook_server_container/libs/github_api.py:1860-1874
Timestamp: 2024-10-15T10:37:45.791Z
Learning: In the `process_retest_command` method in `webhook_server_container/libs/github_api.py`, `_target_tests` is defined before use.
Applied to files:
webhook_server/tests/test_github_api.py
📚 Learning: 2024-10-09T09:16:45.452Z
Learnt from: myakove
Repo: myk-org/github-webhook-server PR: 579
File: webhook_server_container/libs/github_api.py:1098-1101
Timestamp: 2024-10-09T09:16:45.452Z
Learning: In the Python method `_run_tox` within `webhook_server_container/libs/github_api.py`, the variable `_tox_tests` is already comma-separated, so removing spaces with `_tox_tests.replace(" ", "")` is appropriate to handle any accidental spaces when specifying Tox environments.
Applied to files:
webhook_server/tests/test_github_api.py
📚 Learning: 2024-11-26T14:30:22.906Z
Learnt from: myakove
Repo: myk-org/github-webhook-server PR: 631
File: webhook_server_container/tests/test_github_api.py:321-327
Timestamp: 2024-11-26T14:30:22.906Z
Learning: In `webhook_server_container/tests/test_github_api.py`, when using `pytest.mark.parametrize` with indirect parameters, the `pytest.param` function may require nested lists to match the expected input structure.
Applied to files:
webhook_server/tests/test_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/**/*.py : Defensive checks are acceptable for optional parameters that explicitly allow None (parameter type includes `| None` or `Optional`)
Applied to files:
webhook_server/tests/test_github_api.py
📚 Learning: 2024-10-14T14:12:28.924Z
Learnt from: myakove
Repo: myk-org/github-webhook-server PR: 588
File: webhook_server_container/libs/github_api.py:1638-1639
Timestamp: 2024-10-14T14:12:28.924Z
Learning: In `webhook_server_container/libs/github_api.py`, it is acceptable for `self.repository.owner.email` to be `None` since internal commands can handle this case.
Applied to files:
webhook_server/tests/test_github_api.py
🧬 Code graph analysis (1)
webhook_server/tests/test_safe_rotating_handler.py (1)
webhook_server/utils/safe_rotating_handler.py (2)
SafeRotatingFileHandler(32-111)doRollover(45-111)
🪛 Ruff (0.14.11)
webhook_server/tests/test_safe_rotating_handler.py
12-12: Unused noqa directive (non-enabled: F401)
Remove unused noqa directive
(RUF100)
204-204: Avoid specifying long messages outside the exception class
(TRY003)
webhook_server/libs/github_api.py
321-321: Logging statement uses f-string
(G004)
329-329: Logging statement uses f-string
(G004)
342-342: Logging statement uses f-string
(G004)
343-343: Abstract raise to an inner function
(TRY301)
343-343: Avoid specifying long messages outside the exception class
(TRY003)
354-354: Logging statement uses f-string
(G004)
355-355: Abstract raise to an inner function
(TRY301)
355-355: Avoid specifying long messages outside the exception class
(TRY003)
367-367: Logging statement uses f-string
(G004)
446-446: Logging statement uses f-string
(G004)
449-449: Logging statement uses f-string
(G004)
452-452: Logging statement uses f-string
(G004)
705-705: Dynamically typed expressions (typing.Any) are disallowed in item
(ANN401)
721-721: Logging statement uses f-string
(G004)
728-729: Logging statement uses f-string
(G004)
🔇 Additional comments (19)
webhook_server/tests/test_safe_rotating_handler.py (2)
16-219: Comprehensive test coverage for crash resilience scenarios.The test class thoroughly covers the SafeRotatingFileHandler's error handling:
- Basic rollover functionality (smoke test)
- Missing backup files during rollover
- TOCTOU race conditions (file deleted between
exists()and operation)FileNotFoundErrorduringrename()- Base file deletion before
rotate()OSErrorduring_open()with recovery verificationGood practices observed: proper
try/finallycleanup,tempfile.TemporaryDirectoryfor isolation, and clear documentation about Windows compatibility (lines 166-169).
222-233: Good regression guard for the patching mechanism.This test ensures the
simple_loggerpatch remains in place. Since the patch happens at import time inhelpers, this acts as a canary—if the patch mechanism breaks or the import order changes, this test will catch it immediately.webhook_server/tests/test_github_api.py (8)
4-4: LGTM - Import additions for precise typing.Adding
AwaitableandCallablefromcollections.abcaligns with the typing refinements ingithub_api.py. These are the canonical locations for these types since Python 3.9+.
47-53: Fixture update correctly reflects new tag-only cloning semantics.MEDIUM: The docstring and ref change from
refs/heads/maintorefs/tags/v1.0.0accurately reflects that push events now only trigger cloning for tags, not branches. This aligns with the behavioral change ingithub_api.pywhere branch pushes skip cloning entirely.
72-74: Fixture type alignment with production code.Returning
Headersinstead ofdictensures tests match the actual runtime behavior. SinceGithubWebhook.__init__expectsHeaders, this prevents false-positive test passes where a plain dict might work differently than the realHeadersclass.
80-87: Precise typing for async test helper.LOW: The refined signature
Callable[..., Awaitable[object]]is more accurate than usingAny. The inner function's parameter types (Callable[..., object],*args: object,**kwargs: object) provide better type safety for test code.
1514-1589: Comprehensive test for tag checkout fetch path.This test verifies the new tag-specific cloning behavior:
- Confirms the correct refspec is used for tag fetch (
refs/tags/v1.0.0:refs/tags/v1.0.0)- Verifies checkout command targets the correct tag name
- Uses flexible matching to tolerate additional git flags
The test correctly tracks command execution order and validates against expected patterns.
1591-1690: Critical test for PR fetch ordering invariant.HIGH: This test validates an important correctness invariant - the base branch MUST be fetched before the PR ref. This ordering is critical because:
- The base branch is needed for checkout
- Fetching PR ref before base branch could cause checkout failures
The test verifies both the presence of correct fetch commands AND their relative ordering using index comparison.
1778-1838: Test confirms branch push skips cloning.This test validates the optimization where branch pushes skip repository cloning entirely. The assertions verify:
_clone_repositoryis NOT calledPushHandler.process_push_webhook_datais NOT called- Returns
NoneThis is correct because
PushHandleronly processes tags (PyPI upload, container build), so branch pushes have no work to do.
1906-1967: Test for enabled-labels sanitization and logging.This test covers an important defensive path - when users misconfigure
enabled-labelswith non-string entries (dicts, lists, integers from YAML mistakes), the code should:
- Filter out invalid entries
- Log a warning with sanitized representations
- Still process valid string entries
The assertions correctly verify:
- Warning was logged with expected sanitization format (
dict(keys=...),list(len=...),int(...))- Valid entries are preserved in
enabled_labelswebhook_server/libs/github_api.py (9)
12-12: Type hint improvement for background tasks.Using
Task[Any]from theasynciomodule directly is cleaner than the qualifiedasyncio.Task. This is consistent with PEP 585 style.Also applies to: 87-87
294-295: Correct async wrapping for PyGithub property access.HIGH: Per coding guidelines, PyGithub is synchronous/blocking and MUST be wrapped with
asyncio.to_thread(). Usinglambda: self.repository.clone_urlcorrectly wraps the property access that may trigger an API call.
314-324: Owner login access properly wrapped.The
owner.loginproperty access is wrapped withasyncio.to_threadto avoid blocking the event loop. The constructed git config commands use shell-safe quoting with single quotes around the login value.
331-367: Optimized fetch strategy reduces API overhead.MEDIUM: The refactored fetch logic is significantly more efficient:
For PRs:
- Fetch only the base branch (needed for checkout)
- Fetch only the specific PR ref
For tags:
- Fetch only the specific tag refspec
This replaces a blanket "fetch all remote refs" approach, reducing network traffic and GitHub API load. The error handling correctly raises
RuntimeErrorfor critical failures (base branch and PR ref fetches) while only warning for tag fetch failures.
356-367: Verify tag fetch failure is intentionally non-fatal.The tag fetch failure only logs a warning (line 367) rather than raising an exception like the PR/base branch fetches. Is this intentional? If the tag fetch fails, the subsequent checkout will also fail.
Consider whether tag fetch failures should also be fatal:
rc, _, _ = await run_command( command=f"{git_cmd} fetch origin {fetch_refspec}", log_prefix=self.log_prefix, mask_sensitive=self.mask_sensitive, ) if not rc: - self.logger.warning(f"{self.log_prefix} Failed to fetch tag {checkout_ref}") + redacted_err = redact_output(err) + self.logger.error(f"{self.log_prefix} Failed to fetch tag {checkout_ref}: {redacted_err}") + raise RuntimeError(f"Failed to fetch tag {checkout_ref}: {redacted_err}")Note: If the warning is intentional (e.g., the tag may already exist locally from a previous clone), please disregard.
437-453: Branch push skip optimization is correct.Only tag pushes require cloning because
PushHandlerexclusively processes:
- PyPI uploads (triggered by tags)
- Container builds (triggered by tags)
Branch pushes have no associated work in
PushHandler, so skipping cloning saves resources. The logging clearly distinguishes between tag processing and branch skip paths.
619-625: Defensive type coercion for dict configs.MEDIUM: The pattern
_pypi if isinstance(_pypi, dict) else {}preventsAttributeErrorif a user misconfigures YAML with a string or list instead of a dict. This is acceptable defensive coding for optional parameters per coding guidelines.
656-671: Type coercion for list-valued config fields.Same defensive pattern applied to list-valued configurations. This prevents
TypeErrorwhen iterating if a user provides a string instead of a list.
684-739: Labels configuration loading with robust validation.This implementation correctly handles the configurable labels feature:
- Merge strategy: Repo config overrides global config (
{**global, **repo})- None semantics:
enabled_labels = Nonemeans all labels enabled (backward compatible)- Non-string filtering: Protects against YAML mistakes (nested dicts/lists)
- Category validation: Only valid categories from
CONFIGURABLE_LABEL_CATEGORIESare accepted- Safe logging: Sanitizes dropped items to avoid logging large/untrusted YAML blobs
The sanitization helper
_sanitize_itemprovides readable log output without exposing potentially large or sensitive config values.
|
/build-and-push-container |
|
New container for ghcr.io/myk-org/github-webhook-server:pr-978 published |
|
Successfully removed PR tag: ghcr.io/myk-org/github-webhook-server:pr-978. |
|
New container for ghcr.io/myk-org/github-webhook-server:latest published |
2 participants
Summary
Implements configurable labels feature allowing users to:
enabled-labelsconfigcolorsconfigConfiguration
Important
reviewed-by labels (
approved-*,lgtm-*,changes-requested-*,commented-*) are always enabled and cannot be disabled - they are the source of truth for the approval system.Changes
labelsschema toschema.yaml(global + per-repo levels)enabled_labelsandlabel_colorsconfig loading ingithub_api.pyis_label_enabled()method inlabels_handler.py_get_label_color()to use configured colorsenabled-labelswith warning for invalid categories_get_custom_pr_size_thresholds()ALL_LABELS_DICTforDEFAULT_LABEL_COLORSto eliminate duplicationexamples/config.yamlwith labels examplesTest plan
Closes #976
Summary by CodeRabbit
New Features
Behavior Changes
Tests
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.