feat(mcp): add ENABLE_MCP_SERVER environment variable check#916
feat(mcp): add ENABLE_MCP_SERVER environment variable check#916
Conversation
Make MCP server endpoints conditional based on ENABLE_MCP_SERVER=true environment variable, matching the pattern used for log server endpoints. Changes: - Add MCP_SERVER_ENABLED constant for environment variable check - Wrap entire MCP initialization block in conditional - Add security warning about network exposure and authentication - Follow DRY principle by using constant instead of duplicate env check Pattern consistency with log server implementation ensures maintainable and secure deployment options.
Remove outdated Podman troubleshooting documentation.
|
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. |
- Fixed 39 failing tests in test_pull_request_handler.py by adding proper mocks and assertions - Added comprehensive tests in test_app.py for websocket, lifespan, and log viewer endpoints - Fixed test_check_run_handler.py output truncation test to verify correct behavior - Added cleanup method tests in test_github_api.py - Achieved 91.46% test coverage (exceeds 90% requirement) - Fixed prek violations: secret detection and import organization - All 929 tests now passing successfully
WalkthroughAdds an MCP enablement flag and conditional MCP lifecycle to the app, introduces async cleanup for GithubWebhook clone directories, refactors check-run text truncation logic, fixes reviewer-removal safety in PR handler, deletes Podman troubleshooting docs, and expands tests across app, handlers, and GitHub API interactions. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (4)📚 Learning: 2025-10-30T00:18:06.176ZApplied to files:
📚 Learning: 2025-05-13T12:06:27.297ZApplied to files:
📚 Learning: 2024-10-29T08:09:57.157ZApplied to files:
📚 Learning: 2024-10-29T10:42:50.163ZApplied to files:
🪛 Ruff (0.14.5)webhook_server/libs/github_api.py726-726: Do not catch blind exception: (BLE001) 🔇 Additional comments (2)
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 |
…feature/mcp-server-conditional
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
webhook_server/libs/github_api.py (1)
715-745: cleanup logic looks good; consider narrowing the exception typeThe new
cleanup()correctly usesasyncio.to_threadand guards onclone_repo_direxistence; pairing it with__del__keeps best-effort cleanup without breaking on partially‑initialized instances.To satisfy BLE001 and keep logs focused, you can narrow the catch to filesystem errors instead of
Exception:- if hasattr(self, "clone_repo_dir") and os.path.exists(self.clone_repo_dir): - try: - # Use to_thread for blocking I/O operation - await asyncio.to_thread(shutil.rmtree, self.clone_repo_dir, ignore_errors=True) - if hasattr(self, "logger"): - self.logger.debug(f"{self.log_prefix} Cleaned up temp directory: {self.clone_repo_dir}") - except Exception as ex: - if hasattr(self, "logger"): - self.logger.warning(f"{self.log_prefix} Failed to cleanup temp directory: {ex}") + if hasattr(self, "clone_repo_dir") and os.path.exists(self.clone_repo_dir): + try: + # Use to_thread for blocking I/O operation + await asyncio.to_thread(shutil.rmtree, self.clone_repo_dir, ignore_errors=True) + if hasattr(self, "logger"): + self.logger.debug(f"{self.log_prefix} Cleaned up temp directory: {self.clone_repo_dir}") + except OSError as ex: + if hasattr(self, "logger"): + self.logger.warning(f"{self.log_prefix} Failed to cleanup temp directory: {ex}")webhook_server/app.py (1)
48-60: MCP server gating and lifecycle look consistent; watch private-API relianceThe MCP integration is cleanly feature‑gated:
MCP_SERVER_ENABLEDmirrors the existingLOG_SERVER_ENABLEDenv‑flag pattern.- MCP globals are only initialized when the flag is true, and
/mcpis only mounted in that case.- Lifespan initializes a
StreamableHTTPSessionManagerin stateless mode and keeps it running via a background task, while the route guard returns 500 if the manager isn’t ready, which is appropriate fail‑closed behavior.The main caveat is reliance on private attributes of the transport (
_session_manager,_manager_task,_manager_started). That’s workable if thefastapi_mcpversion is pinned, but it is brittle against library changes.If the library exposes public hooks for session management in a future release, it would be worth refactoring to those to avoid patching internals.
Also applies to: 155-174, 221-226, 1082-1111
webhook_server/tests/test_github_api.py (1)
1567-1624: Cleanup tests cover success and failure paths; minor lint nits are optionalThe new
test_cleanupandtest_cleanup_exceptioncorrectly:
- Exercise the async
cleanup()path viaasyncio.to_thread.- Assert that
shutil.rmtree(..., ignore_errors=True)is called when the dir exists.- Confirm that debug vs warning logs are emitted on success vs failure.
To quiet Ruff without changing behavior, you could optionally:
- async def test_cleanup( - self, minimal_hook_data: dict, minimal_headers: dict, logger: Mock, to_thread_sync: Any + async def test_cleanup( + self, minimal_hook_data: dict, minimal_headers: dict, to_thread_sync: Any ) -> None: @@ - # Set a fake clone dir - gh.clone_repo_dir = "/tmp/fake-clone-dir" + # Set a fake clone dir (value irrelevant due to mocks) + gh.clone_repo_dir = "/tmp/fake-clone-dir" # noqa: S108 @@ - async def test_cleanup_exception( - self, minimal_hook_data: dict, minimal_headers: dict, logger: Mock, to_thread_sync: Any + async def test_cleanup_exception( + self, minimal_hook_data: dict, minimal_headers: dict, to_thread_sync: Any @@ - def rmtree_fail(*args, **kwargs): + def rmtree_fail(*_args: object, **_kwargs: object) -> None: raise PermissionError("Access denied")These are purely cosmetic; the existing tests are otherwise solid.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
docs/PODMAN_TROUBLESHOOTING.md(0 hunks)webhook_server/app.py(6 hunks)webhook_server/libs/github_api.py(2 hunks)webhook_server/libs/handlers/check_run_handler.py(1 hunks)webhook_server/libs/handlers/pull_request_handler.py(2 hunks)webhook_server/tests/test_app.py(3 hunks)webhook_server/tests/test_check_run_handler.py(1 hunks)webhook_server/tests/test_github_api.py(1 hunks)webhook_server/tests/test_pull_request_handler.py(14 hunks)
💤 Files with no reviewable changes (1)
- docs/PODMAN_TROUBLESHOOTING.md
🧰 Additional context used
🧠 Learnings (10)
📚 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-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.pywebhook_server/tests/test_github_api.pywebhook_server/libs/handlers/pull_request_handler.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_check_run_handler.py
📚 Learning: 2025-10-28T13:04:00.466Z
Learnt from: myakove
Repo: myk-org/github-webhook-server PR: 878
File: webhook_server/libs/handlers/runner_handler.py:491-571
Timestamp: 2025-10-28T13:04:00.466Z
Learning: In webhook_server/libs/handlers/runner_handler.py, the run_build_container method is designed with the pattern that push=True is always called with set_check=False in production code, so no check-run status needs to be finalized after push operations.
Applied to files:
webhook_server/tests/test_check_run_handler.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-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/handlers/pull_request_handler.pywebhook_server/tests/test_pull_request_handler.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/handlers/pull_request_handler.pywebhook_server/app.py
📚 Learning: 2024-10-14T14:13:21.316Z
Learnt from: myakove
Repo: myk-org/github-webhook-server PR: 588
File: webhook_server_container/libs/github_api.py:1632-1637
Timestamp: 2024-10-14T14:13:21.316Z
Learning: In the `ProcessGithubWehook` class in `webhook_server_container/libs/github_api.py`, avoid using environment variables to pass tokens because multiple commands with multiple tokens can run at the same time.
Applied to files:
webhook_server/app.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/app.pywebhook_server/tests/test_pull_request_handler.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_pull_request_handler.py
🧬 Code graph analysis (4)
webhook_server/tests/test_github_api.py (2)
webhook_server/libs/config.py (1)
repository_local_data(56-97)webhook_server/libs/github_api.py (2)
GithubWebhook(50-745)cleanup(715-729)
webhook_server/app.py (1)
webhook_server/libs/github_api.py (2)
process(308-518)cleanup(715-729)
webhook_server/tests/test_app.py (3)
webhook_server/app.py (6)
get_log_viewer_controller(400-412)require_log_server_enabled(68-74)websocket_log_stream(1055-1079)lifespan(78-206)get_log_entries(467-580)export_logs(622-760)webhook_server/web/log_viewer.py (5)
handle_websocket(363-431)get_log_page(73-87)get_log_entries(89-219)export_logs(266-361)shutdown(48-71)webhook_server/libs/exceptions.py (1)
RepositoryNotFoundInConfigError(1-4)
webhook_server/tests/test_pull_request_handler.py (3)
webhook_server/libs/github_api.py (2)
GithubWebhook(50-745)container_repository_and_tag(671-693)webhook_server/libs/handlers/owners_files_handler.py (4)
OwnersFileHandler(23-554)root_approvers(71-76)root_reviewers(63-68)assign_reviewers(406-469)webhook_server/libs/handlers/pull_request_handler.py (9)
PullRequestHandler(45-1244)process_pull_request_webhook_data(64-248)_delete_ghcr_tag_via_github_api(486-608)add_pull_request_owner_as_assingee(917-926)process_opened_or_synchronize_pull_request(713-787)create_issue_for_new_pull_request(789-807)set_pull_request_automerge(815-844)label_pull_request_by_merge_state(870-884)_delete_registry_tag_via_regctl(610-688)
🪛 Ruff (0.14.5)
webhook_server/libs/github_api.py
727-727: Do not catch blind exception: Exception
(BLE001)
webhook_server/tests/test_github_api.py
1568-1568: Unused method argument: logger
(ARG002)
1585-1585: Probable insecure usage of temporary file or directory: "/tmp/fake-clone-dir"
(S108)
1592-1592: Probable insecure usage of temporary file or directory: "/tmp/fake-clone-dir"
(S108)
1597-1597: Unused method argument: logger
(ARG002)
1613-1613: Probable insecure usage of temporary file or directory: "/tmp/fake-clone-dir"
(S108)
1617-1617: Unused function argument: args
(ARG001)
1617-1617: Unused function argument: kwargs
(ARG001)
1618-1618: Avoid specifying long messages outside the exception class
(TRY003)
webhook_server/tests/test_app.py
747-747: Unused method argument: mock_stream_manager
(ARG002)
902-902: Unused method argument: mock_cf_allowlist
(ARG002)
902-902: Unused method argument: mock_gh_allowlist
(ARG002)
webhook_server/tests/test_pull_request_handler.py
73-73: Possible hardcoded password assigned to: "container_repository_password"
(S105)
79-79: Possible hardcoded password assigned to: "token"
(S105)
1577-1577: Possible hardcoded password assigned to: "token"
(S105)
1756-1756: Possible hardcoded password assigned to: "container_repository_password"
(S105)
🔇 Additional comments (24)
webhook_server/libs/handlers/check_run_handler.py (1)
300-337: Robust truncation logic for check run textThe new MAX_LEN/WRAPPER_OVERHEAD handling and error‑first truncation ensure the fenced block is always within the GitHub limit and still ends with a valid closing code fence, matching the updated tests. No issues spotted.
webhook_server/app.py (1)
330-369: Background webhook cleanup is correctly centralizedWrapping
GithubWebhook.process()in atry/finallyand callingawait _api.cleanup()ensures temp clones are released for all code paths (success, expected repo/config errors, and unexpected exceptions). This matches the new explicitcleanup()API and improves lifecycle hygiene without altering request semantics.webhook_server/tests/test_check_run_handler.py (1)
479-486: Test update correctly guards the closing code fenceThe added
endswith("\n```")assertion is a good regression check for the truncation logic and matches the new implementation’s guarantees.webhook_server/libs/handlers/pull_request_handler.py (1)
1056-1063: Fix for potential KeyError when tracking LGTM reviewersGuarding
remove()with a membership check avoidsKeyErrorif a reviewer label appears more than once or isn’t in the computed set, while preserving the intended accounting of LGTM reviewers. This is a safe, focused bug fix.webhook_server/tests/test_github_api.py (1)
1511-1565: Push event test now fully asserts clone + handler behaviorThe normal push (deleted=False) scenario now verifies both that
_clone_repositoryis invoked with the expectedcheckout_refand thatPushHandler.process_push_webhook_dataruns once, which tightly matches the production flow. No issues.webhook_server/tests/test_pull_request_handler.py (9)
48-82: LGTM! Well-structured test fixture with proper specs.The use of
spec=GithubWebhookensures type safety and prevents attribute typos in tests. The comprehensive attribute setup (lines 69-81) provides good coverage for various test scenarios.
96-132: LGTM! Excellent test isolation with comprehensive handler mocking.The fixture properly replaces internal handlers (labels_handler, check_run_handler, runner_handler) with mocks that have AsyncMock methods. This ensures proper unit test isolation while maintaining the correct async interface.
170-181: LGTM! Test correctly verifies conventional title check on title change.The test properly validates that
run_conventional_title_checkis invoked whenconventional_titleis enabled and the title has changed.
1471-1513: LGTM! Good coverage for async exception handling.These tests verify that exceptions in async tasks (during opened/synchronize events) are properly caught and logged, ensuring graceful error handling without crashing the application.
1514-1567: LGTM! Edge case coverage for labeled events and unhandled actions.The tests properly validate:
- Skip logic for
can-be-mergedlabel (lines 1514-1532)- WIP label processing (lines 1533-1551)
- Unhandled action logging (lines 1552-1567)
The assertion pattern using
call_args_liststring search is acceptable for verifying log messages in tests.
1569-1622: LGTM! Good coverage for error scenarios.The tests properly validate:
- Invalid repository format handling (lines 1582-1586)
- Package not found (404) error handling (lines 1588-1599)
- Assignee fallback behavior when initial assignment fails (lines 1601-1622)
The direct testing of
_delete_ghcr_tag_via_github_apiis acceptable for targeting specific error paths that may be difficult to trigger through the public API.
1623-1685: LGTM! Comprehensive coverage for task failure scenarios.The tests properly validate that:
- Setup task failures are caught and logged (lines 1623-1653)
- CI/CD task failures are caught and logged (lines 1654-1685)
The extensive mocking ensures isolated testing of the error handling logic without side effects from other async operations.
1687-1749: LGTM! Good edge case coverage for PR processing.The tests validate:
- Issue creation disabled/auto-verified user scenarios (lines 1687-1713)
- Automerge exception handling (lines 1715-1734)
- Unknown merge state early return (lines 1736-1749)
1751-1782: LGTM! Thorough testing of registry tag deletion failures.The tests validate both login failure and tag deletion failure scenarios, ensuring proper error handling and logging at each step of the regctl workflow.
webhook_server/tests/test_app.py (10)
40-44: LGTM! Essential fixture for test isolation.The autouse fixture ensures that
ALLOWED_IPSis reset to an empty tuple for all tests, preventing IP allowlist checks from interfering with test execution. This is a good practice for maintaining test independence.
656-686: LGTM! Good coverage for log server enablement checks.The tests properly validate:
require_log_server_enabledraises 404 when disabled (lines 656-663)- WebSocket connection rejected when log server disabled (lines 664-671)
- WebSocket connection handled correctly when enabled (lines 672-686)
687-719: LGTM! Tests validate static files and singleton behavior.The tests ensure:
- Static files validation raises appropriate exceptions (lines 687-707)
get_log_viewer_controllerreturns singleton instance (lines 708-719)
720-743: LGTM! Tests verify background task cleanup.The test properly validates that background tasks are awaited during lifespan shutdown, ensuring graceful cleanup.
744-772: LGTM! Tests MCP initialization flow.The test validates that when
MCP_SERVER_ENABLEDis True and the required components are available, the MCP session manager is properly initialized during lifespan startup.
773-830: LGTM! Comprehensive failure scenario coverage.The tests validate proper error handling when:
- Only Cloudflare allowlist is enabled and fails (lines 773-787)
- Only GitHub allowlist is enabled and fails (lines 788-802)
- Background tasks timeout during shutdown (lines 803-830)
831-859: LGTM! Tests validate webhook header and payload requirements.The tests ensure proper error responses (400 Bad Request) when:
- X-GitHub-Event header is missing (lines 831-836)
- repository.name is missing (lines 837-848)
- repository.full_name is missing (lines 849-859)
860-897: LGTM! Tests validate log viewer endpoints.The tests verify that when
LOG_SERVER_ENABLEDis True:
- Log viewer page is served (lines 860-870)
- Log entries API returns data (lines 871-882)
- Log export endpoint streams data (lines 883-897)
898-916: LGTM! Tests verify log viewer controller shutdown.The test ensures that the
LogViewerControllersingleton'sshutdownmethod is called during lifespan cleanup, preventing resource leaks from unclosed WebSocket connections.
917-992: LGTM! Tests validate background error handling.These tests verify that errors in background webhook processing tasks are properly:
- Caught and logged without affecting the HTTP response (returns 200)
- Logged with appropriate error messages (connection error, repository not found)
The pattern of capturing the coroutine from
create_taskand executing it separately is a valid testing approach for background tasks.
|
New container for ghcr.io/myk-org/github-webhook-server:latest published |
2 participants
Make MCP server endpoints conditional based on ENABLE_MCP_SERVER=true
environment variable, matching the pattern used for log server endpoints.
Changes:
Pattern consistency with log server implementation ensures maintainable
and secure deployment options.
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores