Implement Plugins SDK for Python #1
Conversation
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
WalkthroughAdds an SDK for mcpd plugins: core package (BasePlugin, server, exceptions), generated protobuf/gRPC bindings, five example plugins with docs, CI/release workflows, developer scripts and Make targets, packaging config, pre-commit/linters, build tooling, tests, and GitHub issue templates and configuration. Changes
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 33
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (43)
.github/ISSUE_TEMPLATE/bug_report.yaml(1 hunks).github/ISSUE_TEMPLATE/config.yaml(1 hunks).github/ISSUE_TEMPLATE/feature_request.yaml(1 hunks).github/workflows/lint.yml(1 hunks).github/workflows/release.yml(1 hunks).github/workflows/test.yml(1 hunks).gitignore(1 hunks).pre-commit-config.yaml(1 hunks).python-version(1 hunks)BUILD.md(1 hunks)Makefile(1 hunks)README.md(1 hunks)examples/auth_plugin/README.md(1 hunks)examples/auth_plugin/__init__.py(1 hunks)examples/auth_plugin/main.py(1 hunks)examples/logging_plugin/README.md(1 hunks)examples/logging_plugin/__init__.py(1 hunks)examples/logging_plugin/main.py(1 hunks)examples/rate_limit_plugin/README.md(1 hunks)examples/rate_limit_plugin/__init__.py(1 hunks)examples/rate_limit_plugin/main.py(1 hunks)examples/simple_plugin/README.md(1 hunks)examples/simple_plugin/__init__.py(1 hunks)examples/simple_plugin/main.py(1 hunks)examples/transform_plugin/README.md(1 hunks)examples/transform_plugin/__init__.py(1 hunks)examples/transform_plugin/main.py(1 hunks)pyproject.toml(1 hunks)ruff.toml(1 hunks)scripts/build_plugin.sh(1 hunks)scripts/generate_protos.sh(1 hunks)scripts/setup_uv.sh(1 hunks)src/mcpd_plugins/__init__.py(1 hunks)src/mcpd_plugins/base_plugin.py(1 hunks)src/mcpd_plugins/exceptions.py(1 hunks)src/mcpd_plugins/server.py(1 hunks)src/mcpd_plugins/v1/plugins/plugin_pb2.py(1 hunks)src/mcpd_plugins/v1/plugins/plugin_pb2_grpc.py(1 hunks)tests/conftest.py(1 hunks)tests/integration/test_examples.py(1 hunks)tests/unit/test_base_plugin.py(1 hunks)tests/unit/test_exceptions.py(1 hunks)tests/unit/test_server.py(1 hunks)
🧰 Additional context used
🪛 checkmake (0.2.2)
Makefile
[warning] 43-43: Target body for "clean" exceeds allowed length of 5 (7).
(maxbodylength)
[warning] 42-42: Missing required phony target "all"
(minphony)
🪛 LanguageTool
examples/rate_limit_plugin/README.md
[uncategorized] ~32-~32: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...t buckets) - Token bucket algorithm for rate limiting - Custom response headers - Calculating...
(EN_COMPOUND_ADJECTIVE_INTERNAL)
README.md
[uncategorized] ~234-~234: Loose punctuation mark.
Context: ... mode on TCP port 50051. - max_workers: Maximum number of concurrent gRPC worke...
(UNLIKELY_OPENING_PUNCTUATION)
[locale-violation] ~255-~255: License must be spelled with a “c” when used as a noun in British English. Use “licence”.
Context: ...ase see SECURITY.md. ## License This project is licensed under the Apa...
(LICENCE_LICENSE_NOUN_SINGULAR)
[locale-violation] ~257-~257: License must be spelled with a “c” when used as a noun in British English. Use “licence”.
Context: ...is project is licensed under the Apache License 2.0 - see the LICENSE file f...
(LICENCE_LICENSE_NOUN_SINGULAR)
[locale-violation] ~257-~257: LICENSE must be spelled with a “c” when used as a noun in British English. Use “licence”.
Context: ...under the Apache License 2.0 - see the LICENSE file for details. ## Related...
(LICENCE_LICENSE_NOUN_SINGULAR)
BUILD.md
[typographical] ~189-~189: If specifying a range, consider using an en dash instead of a hyphen.
Context: ...w Nuitka Builds Nuitka builds can take 5-30 minutes. To speed up: 1. Use `--show-p...
(HYPHEN_TO_EN)
[grammar] ~195-~195: Did you mean “processes”?
Context: ...he-dir=.nuitka_cache` ## How the Build Process Works ### PyInstaller Process 1. **An...
(WRB_DT_NN_VB)
🪛 markdownlint-cli2 (0.18.1)
examples/transform_plugin/README.md
25-25: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
33-33: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
README.md
118-118: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
126-126: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
135-135: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
143-143: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
151-151: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
🪛 Ruff (0.14.1)
src/mcpd_plugins/exceptions.py
7-7: Unnecessary pass statement
Remove unnecessary pass
(PIE790)
13-13: Unnecessary pass statement
Remove unnecessary pass
(PIE790)
19-19: Unnecessary pass statement
Remove unnecessary pass
(PIE790)
tests/unit/test_exceptions.py
23-23: Avoid specifying long messages outside the exception class
(TRY003)
42-42: Avoid specifying long messages outside the exception class
(TRY003)
60-60: Avoid specifying long messages outside the exception class
(TRY003)
examples/transform_plugin/main.py
31-31: Unused method argument: request
(ARG002)
31-31: Unused method argument: context
(ARG002)
39-39: Unused method argument: request
(ARG002)
39-39: Unused method argument: context
(ARG002)
43-43: Unused method argument: context
(ARG002)
45-45: Logging statement uses f-string
(G004)
61-61: Logging statement uses f-string
(G004)
70-70: Logging statement uses f-string
(G004)
88-88: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
88-88: Logging statement uses f-string
(G004)
96-96: Do not catch blind exception: Exception
(BLE001)
97-97: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
97-97: Logging statement uses f-string
(G004)
examples/auth_plugin/main.py
30-30: Missing return type annotation for special method __init__
Add return type annotation: None
(ANN204)
35-35: Unused method argument: request
(ARG002)
35-35: Unused method argument: context
(ARG002)
43-43: Unused method argument: request
(ARG002)
43-43: Unused method argument: context
(ARG002)
47-47: Unused method argument: context
(ARG002)
49-49: Logging statement uses f-string
(G004)
src/mcpd_plugins/server.py
21-21: Unused function argument: max_workers
(ARG001)
81-84: Avoid specifying long messages outside the exception class
(TRY003)
109-109: Abstract raise to an inner function
(TRY301)
109-109: Avoid specifying long messages outside the exception class
(TRY003)
111-111: Avoid specifying long messages outside the exception class
(TRY003)
118-118: Logging statement uses f-string
(G004)
127-127: Logging statement uses f-string
(G004)
133-133: Logging statement uses f-string
(G004)
138-138: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
138-138: Logging statement uses f-string
(G004)
140-140: Avoid specifying long messages outside the exception class
(TRY003)
examples/rate_limit_plugin/main.py
31-31: Missing return type annotation for special method __init__
Add return type annotation: None
(ANN204)
45-45: Unused method argument: request
(ARG002)
45-45: Unused method argument: context
(ARG002)
53-53: Unused method argument: request
(ARG002)
53-53: Unused method argument: context
(ARG002)
57-57: Unused method argument: context
(ARG002)
60-60: Logging statement uses f-string
(G004)
73-73: Logging statement uses f-string
(G004)
78-78: Logging statement uses f-string
(G004)
src/mcpd_plugins/__init__.py
43-50: __all__ is not sorted
Apply an isort-style sorting to __all__
(RUF022)
examples/simple_plugin/main.py
30-30: Unused method argument: request
(ARG002)
30-30: Unused method argument: context
(ARG002)
38-38: Unused method argument: request
(ARG002)
38-38: Unused method argument: context
(ARG002)
42-42: Unused method argument: context
(ARG002)
44-44: Logging statement uses f-string
(G004)
tests/integration/test_examples.py
21-21: Missing return type annotation for special method __init__
Add return type annotation: None
(ANN204)
tests/conftest.py
13-13: Missing return type annotation for special method __init__
Add return type annotation: None
(ANN204)
24-24: Create your own exception
(TRY002)
24-24: Avoid specifying long messages outside the exception class
(TRY003)
tests/unit/test_base_plugin.py
84-84: Unused method argument: request
(ARG002)
84-84: Unused method argument: context
(ARG002)
92-92: Unused method argument: request
(ARG002)
92-92: Unused method argument: context
(ARG002)
examples/logging_plugin/main.py
34-34: Unused method argument: request
(ARG002)
34-34: Unused method argument: context
(ARG002)
42-42: Unused method argument: request
(ARG002)
42-42: Unused method argument: context
(ARG002)
46-46: Unused method argument: context
(ARG002)
50-50: Logging statement uses f-string
(G004)
51-51: Logging statement uses f-string
(G004)
52-52: Logging statement uses f-string
(G004)
53-53: Logging statement uses f-string
(G004)
61-61: Logging statement uses f-string
(G004)
65-65: Logging statement uses f-string
(G004)
72-72: Unused method argument: context
(ARG002)
76-76: Logging statement uses f-string
(G004)
81-81: Logging statement uses f-string
(G004)
85-85: Logging statement uses f-string
(G004)
src/mcpd_plugins/base_plugin.py
40-40: Unused method argument: request
(ARG002)
40-40: Unused method argument: context
(ARG002)
54-54: Unused method argument: request
(ARG002)
54-54: Unused method argument: context
(ARG002)
68-68: Unused method argument: request
(ARG002)
68-68: Unused method argument: context
(ARG002)
86-86: Unused method argument: request
(ARG002)
86-86: Unused method argument: context
(ARG002)
100-100: Unused method argument: request
(ARG002)
100-100: Unused method argument: context
(ARG002)
114-114: Unused method argument: request
(ARG002)
114-114: Unused method argument: context
(ARG002)
128-128: Unused method argument: request
(ARG002)
128-128: Unused method argument: context
(ARG002)
142-142: Unused method argument: request
(ARG002)
142-142: Unused method argument: context
(ARG002)
src/mcpd_plugins/v1/plugins/plugin_pb2_grpc.py
21-27: Avoid specifying long messages outside the exception class
(TRY003)
33-33: Missing return type annotation for special method __init__
Add return type annotation: None
(ANN204)
92-92: Unused method argument: request
(ARG002)
98-98: Unused method argument: request
(ARG002)
104-104: Unused method argument: request
(ARG002)
110-110: Unused method argument: request
(ARG002)
116-116: Unused method argument: request
(ARG002)
124-124: Unused method argument: request
(ARG002)
130-130: Unused method argument: request
(ARG002)
136-136: Unused method argument: request
(ARG002)
196-196: Missing return type annotation for staticmethod Configure
(ANN205)
202-202: Boolean default positional argument in function definition
(FBT002)
226-226: Missing return type annotation for staticmethod Stop
(ANN205)
232-232: Boolean default positional argument in function definition
(FBT002)
256-256: Missing return type annotation for staticmethod GetMetadata
(ANN205)
262-262: Boolean default positional argument in function definition
(FBT002)
286-286: Missing return type annotation for staticmethod GetCapabilities
(ANN205)
292-292: Boolean default positional argument in function definition
(FBT002)
316-316: Missing return type annotation for staticmethod CheckHealth
(ANN205)
322-322: Boolean default positional argument in function definition
(FBT002)
346-346: Missing return type annotation for staticmethod CheckReady
(ANN205)
352-352: Boolean default positional argument in function definition
(FBT002)
376-376: Missing return type annotation for staticmethod HandleRequest
(ANN205)
382-382: Boolean default positional argument in function definition
(FBT002)
406-406: Missing return type annotation for staticmethod HandleResponse
(ANN205)
412-412: Boolean default positional argument in function definition
(FBT002)
🪛 YAMLlint (1.37.1)
.github/workflows/test.yml
[warning] 3-3: truthy value should be one of [false, true]
(truthy)
[error] 22-22: too many spaces inside brackets
(brackets)
[error] 22-22: too many spaces inside brackets
(brackets)
[error] 23-23: too many spaces inside brackets
(brackets)
[error] 23-23: too many spaces inside brackets
(brackets)
[warning] 29-29: too few spaces before comment: expected 2
(comments)
[warning] 32-32: too few spaces before comment: expected 2
(comments)
🔇 Additional comments (28)
examples/auth_plugin/__init__.py (1)
1-1: LGTM!The module docstring appropriately describes the purpose of this authentication plugin example package, following Python conventions.
.python-version (1)
1-1: Verify Python version alignment: .python-version (3.12) diverges from lint/release workflows (3.11).python-version specifies "3.12" but your lint.yml and release.yml workflows pin 3.11. Additionally, "3.12" lacks a patch version (pyenv typically expects 3.12.0 or similar). Verify and align:
- If local development should mirror linting/release, change to "3.11"
- If 3.12 is intentional, pin a full patch (e.g., "3.12.0") and confirm CI test.yml coverage includes it (it does: 3.11, 3.12, 3.13)
- Document in CONTRIBUTING.md if dev version intentionally differs from lint/release workflows
.github/workflows/test.yml (1)
3-39: Fix YAML formatting issues flagged by linter.Lines 22–23 have excessive spacing inside brackets, and lines 29 and 32 need additional spacing before comments per YAML style conventions.
- name: Checkout # 2 spaces before comment uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2 strategy: fail-fast: false matrix: os: [ubuntu-latest, macos-latest] python-version: ['3.11', '3.12', '3.13']examples/transform_plugin/README.md (1)
24-43: Add blank lines around fenced code blocks per markdown conventions.Lines 25 and 33 are fenced code blocks that need blank lines before and after.
Apply this diff:
**Original request body:** + ```json { "name": "John Doe", @@ -29,6 +30,7 @@ **Transformed request body:** + ```json { "name": "John Doe",.github/workflows/release.yml (1)
1-39: Well-structured release workflow.The workflow correctly uses OIDC-based PyPI publishing, conditional uploads, and pinned action versions. No issues detected.
.github/ISSUE_TEMPLATE/feature_request.yaml (1)
1-54: Template is well-structured and complete.Clear progression from problem description through alternatives, with helpful self-checklist guidance.
ruff.toml (1)
1-75: Ruff configuration is well-tuned for SDK development.Comprehensive rule selection with pragmatic per-file ignores for generated code and appropriate exceptions for protobuf conventions. Unfixable rules appropriately enforce manual review for potential bugs and unused variables.
examples/rate_limit_plugin/README.md (1)
1-53: Documentation is clear and comprehensive.Covers algorithm, configuration, and build workflows effectively. The LanguageTool hint about compound adjectives is a false positive and can be ignored.
.github/ISSUE_TEMPLATE/bug_report.yaml (1)
1-85: Well-designed bug report template with comprehensive coverage.Clear progression through bug description, reproduction, expected/actual behaviour, and environment, with helpful render hints for logs and structured self-checklist.
.github/ISSUE_TEMPLATE/config.yaml (1)
1-5: LGTM!The issue template configuration correctly disables blank issues and provides a clear channel for security vulnerability reporting.
examples/rate_limit_plugin/main.py (3)
45-51: LGTM!The metadata correctly describes the plugin's rate limiting behaviour. The unused arguments are required by the gRPC service interface.
53-55: LGTM!Correctly declares support for the REQUEST flow.
88-104: LGTM!The rate limit response correctly returns a 429 status with appropriate headers including
Retry-Afterguidance.tests/unit/test_exceptions.py (3)
8-24: LGTM!Comprehensive test coverage for the base
PluginErrorexception class, verifying inheritance, message storage, and raise/catch behaviour.
27-42: LGTM!Tests correctly verify that
ConfigurationErrormaintains the exception hierarchy and can be caught asPluginError.
45-60: LGTM!Tests correctly verify that
ServerErrormaintains the exception hierarchy and can be caught asPluginError..github/workflows/lint.yml (1)
1-28: LGTM!The lint workflow is correctly configured with:
- Appropriate triggers (push to main, PRs, manual dispatch)
- Pinned action versions with commit hashes for security
- Consistent use of uv tooling
- Integration with pre-commit hooks
examples/logging_plugin/README.md (1)
1-43: LGTM!Clear and comprehensive documentation that effectively explains the plugin's functionality, key concepts, and build/run instructions. The mention of header redaction for sensitive data is a valuable security highlight.
examples/simple_plugin/README.md (1)
1-44: LGTM!Clear and well-structured documentation that effectively demonstrates the minimal plugin example. The key concepts section provides a good overview of the BasePlugin contract.
.pre-commit-config.yaml (1)
1-24: LGTM!Comprehensive pre-commit configuration that includes:
- Secret detection for security
- Standard code quality checks (merge conflicts, whitespace, EOF)
- Modern Python linting and formatting with Ruff
- Dependency lock management with UV
This configuration aligns well with the lint workflow and enforces consistent code quality across the repository.
examples/auth_plugin/README.md (1)
1-51: LGTM!Clear and comprehensive documentation for the authentication plugin example. It covers all necessary aspects including configuration, key concepts, and build instructions.
BUILD.md (1)
1-259: LGTM!Comprehensive build documentation covering both PyInstaller and Nuitka with clear trade-offs, platform-specific instructions, troubleshooting guidance, and best practices.
src/mcpd_plugins/exceptions.py (1)
4-19: LGTM!Clean exception hierarchy with appropriate base and derived classes. The
passstatements flagged by static analysis are idiomatic Python for empty exception classes and should be retained for clarity.scripts/generate_protos.sh (1)
1-62: LGTM!Well-structured proto generation script with proper error handling, OS-specific sed commands for import fixing, and clear status messages. The use of
set -euo pipefailand curl error checking demonstrates good shell scripting practices.tests/unit/test_base_plugin.py (1)
14-116: LGTM!Comprehensive unit tests covering both default BasePlugin behaviour and custom overrides. The unused argument warnings flagged by static analysis are false positives—the
requestandcontextparameters are required by the method signatures being overridden.src/mcpd_plugins/server.py (1)
95-96: Unix socket URI format is correct.The
unix:///format with three slashes is the correct gRPC Python specification for absolute filesystem paths. No changes required.tests/conftest.py (1)
49-61: Fixtures look good.Representative values; headers/body lengths consistent. No changes needed.
Also applies to: 64-75
src/mcpd_plugins/base_plugin.py (1)
16-38: Note: ensure you register this async servicer on a grpc.aio server.These async methods require
grpc.aio.server(...); using a sync server will fail.Please confirm
src/mcpd_plugins/server.pyusesgrpc.aio.serverwhen wiringBasePlugin.
There was a problem hiding this comment.
Actionable comments posted: 9
♻️ Duplicate comments (2)
src/mcpd_plugins/base_plugin.py (2)
134-147: Optionally silence unused parameter for consistency.For consistency with other no-op methods in this base class (lines 40-132), consider silencing the unused
requestparameter alongsidecontext.async def HandleRequest(self, request: HTTPRequest, context: ServicerContext) -> HTTPResponse: """Handle incoming HTTP request. Default implementation passes through unchanged (continue=True). Args: request: The incoming HTTP request to process. context: gRPC context for the request. Returns: HTTPResponse indicating how to proceed (continue, modify, or reject). """ - _ = context # Required by gRPC interface. + _ = (request, context) # Required by gRPC interface. return HTTPResponse(**{"continue": True})
149-167: Optionally silence unused parameter for consistency.For consistency with other no-op methods in this base class (lines 40-132), consider silencing the unused
responseparameter alongsidecontext.async def HandleResponse(self, response: HTTPResponse, context: ServicerContext) -> HTTPResponse: """Handle outgoing HTTP response. Default implementation passes through unchanged (continue=True). Note: The parameter is named 'response' for clarity, even though the generated gRPC stub names it 'request' (gRPC convention). This semantic naming improves code readability. Args: response: The outgoing HTTP response to process. context: gRPC context for the request. Returns: HTTPResponse indicating how to proceed (continue or modify). """ - _ = context # Required by gRPC interface. + _ = (response, context) # Required by gRPC interface. return HTTPResponse(**{"continue": True})
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (22)
.gitignore(1 hunks)Makefile(1 hunks)README.md(1 hunks)examples/auth_plugin/__init__.py(1 hunks)examples/auth_plugin/main.py(1 hunks)examples/logging_plugin/__init__.py(1 hunks)examples/logging_plugin/main.py(1 hunks)examples/rate_limit_plugin/__init__.py(1 hunks)examples/rate_limit_plugin/main.py(1 hunks)examples/simple_plugin/__init__.py(1 hunks)examples/simple_plugin/main.py(1 hunks)examples/transform_plugin/__init__.py(1 hunks)examples/transform_plugin/main.py(1 hunks)pyproject.toml(1 hunks)ruff.toml(1 hunks)scripts/build_plugin.sh(1 hunks)scripts/setup_uv.sh(1 hunks)src/mcpd_plugins/__init__.py(1 hunks)src/mcpd_plugins/base_plugin.py(1 hunks)src/mcpd_plugins/server.py(1 hunks)tests/conftest.py(1 hunks)tests/integration/test_examples.py(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-28T10:12:05.979Z
Learnt from: peteski22
PR: mozilla-ai/mcpd-plugins-sdk-python#1
File: examples/logging_plugin/main.py:72-72
Timestamp: 2025-10-28T10:12:05.979Z
Learning: In gRPC servicer implementations, the first parameter of RPC handler methods is conventionally named `request` by stub generators, even when the parameter type is semantically a response (e.g., `HandleResponse(self, request: HTTPResponse, ...)`). This follows gRPC terminology where "request" means "the data sent to this RPC method" rather than semantic meaning. This is an expected pattern and should not be flagged as an issue.
Applied to files:
examples/logging_plugin/main.py
🧬 Code graph analysis (15)
examples/auth_plugin/__init__.py (1)
examples/auth_plugin/main.py (1)
AuthPlugin(30-80)
examples/simple_plugin/__init__.py (1)
examples/simple_plugin/main.py (1)
SimplePlugin(27-63)
examples/logging_plugin/__init__.py (1)
examples/logging_plugin/main.py (1)
LoggingPlugin(31-90)
examples/logging_plugin/main.py (2)
src/mcpd_plugins/base_plugin.py (5)
BasePlugin(16-167)GetMetadata(70-87)GetCapabilities(89-102)HandleRequest(134-147)HandleResponse(149-167)src/mcpd_plugins/server.py (1)
serve(18-138)
examples/auth_plugin/main.py (2)
src/mcpd_plugins/base_plugin.py (4)
BasePlugin(16-167)GetMetadata(70-87)GetCapabilities(89-102)HandleRequest(134-147)src/mcpd_plugins/server.py (1)
serve(18-138)
tests/integration/test_examples.py (4)
tests/conftest.py (2)
mock_context(67-69)MockContext(25-63)examples/simple_plugin/main.py (4)
SimplePlugin(27-63)GetMetadata(30-36)GetCapabilities(38-40)HandleRequest(42-63)examples/auth_plugin/main.py (4)
GetMetadata(38-44)GetCapabilities(46-48)HandleRequest(50-69)AuthPlugin(30-80)examples/logging_plugin/main.py (4)
GetMetadata(34-40)GetCapabilities(42-44)HandleRequest(46-70)LoggingPlugin(31-90)
examples/transform_plugin/__init__.py (1)
examples/transform_plugin/main.py (1)
TransformPlugin(28-104)
examples/simple_plugin/main.py (2)
src/mcpd_plugins/base_plugin.py (4)
BasePlugin(16-167)GetMetadata(70-87)GetCapabilities(89-102)HandleRequest(134-147)src/mcpd_plugins/server.py (1)
serve(18-138)
src/mcpd_plugins/server.py (3)
src/mcpd_plugins/base_plugin.py (1)
BasePlugin(16-167)src/mcpd_plugins/exceptions.py (1)
ServerError(16-19)src/mcpd_plugins/v1/plugins/plugin_pb2_grpc.py (1)
add_PluginServicer_to_server(143-188)
src/mcpd_plugins/base_plugin.py (2)
src/mcpd_plugins/v1/plugins/plugin_pb2_grpc.py (17)
PluginServicer(89-140)Configure(92-96)Configure(196-223)Stop(98-102)Stop(226-253)GetMetadata(104-108)GetMetadata(256-283)GetCapabilities(110-114)GetCapabilities(286-313)CheckHealth(116-122)CheckHealth(316-343)CheckReady(124-128)CheckReady(346-373)HandleRequest(130-134)HandleRequest(376-403)HandleResponse(136-140)HandleResponse(406-433)examples/simple_plugin/main.py (3)
GetMetadata(30-36)GetCapabilities(38-40)HandleRequest(42-63)
src/mcpd_plugins/__init__.py (3)
src/mcpd_plugins/base_plugin.py (1)
BasePlugin(16-167)src/mcpd_plugins/exceptions.py (3)
ConfigurationError(10-13)PluginError(4-7)ServerError(16-19)src/mcpd_plugins/server.py (1)
serve(18-138)
tests/conftest.py (1)
tests/integration/test_examples.py (2)
MockContext(18-23)mock_context(15-25)
examples/transform_plugin/main.py (3)
src/mcpd_plugins/base_plugin.py (3)
BasePlugin(16-167)GetMetadata(70-87)HandleRequest(134-147)src/mcpd_plugins/server.py (1)
serve(18-138)examples/simple_plugin/main.py (2)
GetMetadata(30-36)HandleRequest(42-63)
examples/rate_limit_plugin/main.py (2)
src/mcpd_plugins/base_plugin.py (4)
BasePlugin(16-167)GetMetadata(70-87)GetCapabilities(89-102)HandleRequest(134-147)src/mcpd_plugins/server.py (1)
serve(18-138)
examples/rate_limit_plugin/__init__.py (1)
examples/rate_limit_plugin/main.py (1)
RateLimitPlugin(28-106)
🪛 LanguageTool
README.md
[uncategorized] ~238-~238: Loose punctuation mark.
Context: ...mode on TCP port 50051. - grace_period: Seconds to wait during graceful shutdow...
(UNLIKELY_OPENING_PUNCTUATION)
[locale-violation] ~258-~258: License must be spelled with a “c” when used as a noun in British English. Use “licence”.
Context: ...ase see SECURITY.md. ## License This project is licensed under the Apa...
(LICENCE_LICENSE_NOUN_SINGULAR)
[locale-violation] ~260-~260: License must be spelled with a “c” when used as a noun in British English. Use “licence”.
Context: ...is project is licensed under the Apache License 2.0 - see the LICENSE file f...
(LICENCE_LICENSE_NOUN_SINGULAR)
[locale-violation] ~260-~260: LICENSE must be spelled with a “c” when used as a noun in British English. Use “licence”.
Context: ...under the Apache License 2.0 - see the LICENSE file for details. ## Related...
(LICENCE_LICENSE_NOUN_SINGULAR)
🪛 Ruff (0.14.1)
examples/logging_plugin/main.py
34-34: Unused method argument: request
(ARG002)
34-34: Unused method argument: context
(ARG002)
42-42: Unused method argument: request
(ARG002)
42-42: Unused method argument: context
(ARG002)
46-46: Unused method argument: context
(ARG002)
72-72: Unused method argument: context
(ARG002)
examples/auth_plugin/main.py
33-33: Missing return type annotation for special method __init__
Add return type annotation: None
(ANN204)
38-38: Unused method argument: request
(ARG002)
38-38: Unused method argument: context
(ARG002)
46-46: Unused method argument: request
(ARG002)
46-46: Unused method argument: context
(ARG002)
50-50: Unused method argument: context
(ARG002)
tests/integration/test_examples.py
21-21: Missing return type annotation for special method __init__
Add return type annotation: None
(ANN204)
examples/simple_plugin/main.py
30-30: Unused method argument: request
(ARG002)
30-30: Unused method argument: context
(ARG002)
38-38: Unused method argument: request
(ARG002)
38-38: Unused method argument: context
(ARG002)
42-42: Unused method argument: context
(ARG002)
src/mcpd_plugins/server.py
79-82: Avoid specifying long messages outside the exception class
(TRY003)
107-107: Abstract raise to an inner function
(TRY301)
107-107: Avoid specifying long messages outside the exception class
(TRY003)
109-109: Avoid specifying long messages outside the exception class
(TRY003)
138-138: Avoid specifying long messages outside the exception class
(TRY003)
src/mcpd_plugins/base_plugin.py
134-134: Unused method argument: request
(ARG002)
149-149: Unused method argument: response
(ARG002)
src/mcpd_plugins/__init__.py
49-56: __all__ is not sorted
Apply an isort-style sorting to __all__
(RUF022)
examples/transform_plugin/main.py
31-31: Unused method argument: request
(ARG002)
31-31: Unused method argument: context
(ARG002)
39-39: Unused method argument: request
(ARG002)
39-39: Unused method argument: context
(ARG002)
43-43: Unused method argument: context
(ARG002)
93-93: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
examples/rate_limit_plugin/main.py
31-31: Missing return type annotation for special method __init__
Add return type annotation: None
(ANN204)
46-46: Unused method argument: request
(ARG002)
46-46: Unused method argument: context
(ARG002)
54-54: Unused method argument: request
(ARG002)
54-54: Unused method argument: context
(ARG002)
58-58: Unused method argument: context
(ARG002)
🪛 Shellcheck (0.11.0)
scripts/build_plugin.sh
[warning] 17-17: ONEFILE appears unused. Verify use (or export if used externally).
(SC2034)
🔇 Additional comments (14)
.gitignore (1)
1-62: ✅ Duplicate entry resolved; gitignore configuration is well‑structured.The
.gitignorefile is comprehensive and properly organised. The duplicate*.egg-info/entry flagged in the previous review has been removed. The current configuration appropriately covers Python build/distribution artifacts, virtual environments, IDE settings, testing caches, type-checking caches, OS-specific files, and project-specific exclusions (proto files, generated version files, Claude local directory).The file is well-commented with clear section headers, making it maintainable and easy to understand. No further changes are required.
ruff.toml (3)
1-9: Comprehensive Ruff configuration with sensible defaults.The configuration effectively sets up linting and formatting standards for the SDK project. The target Python version, source directory, line length, and autofix settings are appropriate for this project scope.
35-62: Well-justified lint rule selection and unfixable rules.The lint rules are comprehensive and align with Python best practices, covering style, naming, documentation, imports, and pathlib usage. The
unfixablerules for flake8-bugbear and unused variables are sensible—these require manual review rather than blind auto-fixing. The explicit ignore of ARG002 for gRPC interface compatibility is well-justified.
63-69: Per-file ignores appropriately handle generated code and test conventions.The per-file ignores correctly exclude generated protobuf files and version files from linting, whilst permitting relaxed naming conventions in test code and examples. The specific allowances for PascalCase method names in protobuf-related code align with generated code conventions.
examples/rate_limit_plugin/main.py (1)
44-44: Excellent concurrency fix implemented.The per-client asyncio locks correctly address the race condition flagged in the previous review. Token refill, consumption, and balance checks are now properly synchronised, ensuring correct rate limiting behaviour under concurrent load.
Also applies to: 63-88
pyproject.toml (1)
9-10: Dependency versions verified as current and secure.The specified versions are the latest available: grpcio 1.76.0 and protobuf 6.33.0. All published security advisories for grpcio affect versions ≤ 1.54.2. The protobuf DoS issue (HIGH severity, June 2025) affecting 6.30.0rc1 to < 6.31.1 is patched in the specified version 6.33.0. No security concerns identified.
scripts/setup_uv.sh (1)
14-55: LGTM! PATH logic correctly fixed.The previous issue with the PATH check has been properly resolved. The script now captures
INITIAL_PATHbefore modification and correctly checks it later to determine whether to display the permanent PATH setup guidance.examples/simple_plugin/main.py (1)
27-63: LGTM! Clean plugin implementation.The plugin correctly implements the required gRPC interface methods. The unused method arguments flagged by static analysis are required by the gRPC interface and can be safely ignored.
src/mcpd_plugins/__init__.py (1)
6-37: LGTM! Docstring example corrected.The previous issue with the incomplete docstring example has been properly resolved. The example now includes all necessary imports and uses
FLOW_REQUESTcorrectly.scripts/build_plugin.sh (1)
119-173: LGTM! Previous issues resolved.The Nuitka optimisation flags now correctly use
--lto=yes(lines 141, 144), and the redundant DEBUG and ONEFILE conditionals have been removed. The build logic is now clearer and correct.src/mcpd_plugins/server.py (1)
18-138: LGTM! All previous issues resolved.The unused
max_workersparameter has been removed, and the exception logging now correctly useslogger.exception()(line 136) to capture full tracebacks. The server implementation is well-structured with proper signal handling and graceful shutdown.examples/auth_plugin/main.py (1)
30-80: LGTM! Clean authentication implementation.The previous suggestion to use
.removeprefix()has been implemented (line 62), improving code readability. The authentication logic correctly validates Bearer tokens and returns appropriate responses.examples/logging_plugin/main.py (1)
1-94: LGTM! Well-structured example plugin.The logging plugin correctly implements both request and response flows with appropriate observability logging. The implementation follows established patterns from BasePlugin, properly declares capabilities, and handles the gRPC interface requirements correctly. The unused
contextparameters flagged by static analysis are expected—they're required by the gRPC interface but not needed in this straightforward logging implementation.Makefile (1)
1-59: LGTM! Well-structured Makefile following best practices.The Makefile correctly implements all development workflow targets with proper dependency chains, input validation, and error handling. The improvements from past reviews have been successfully incorporated:
- Default
alltarget for conventional behaviour- Robust script execution with conditional directory checks
- Dependency management ensuring
setupruns beforetest/lint- Modular
cleantargets for maintainabilityThe PLUGIN validation in build targets prevents common user errors, and the help target provides clear usage documentation.
peteski22
force-pushed
the
peteski22/project-setup
branch
from
1202e33 to
2300395
Compare
There was a problem hiding this comment.
Actionable comments posted: 7
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (23)
.gitignore(1 hunks)Makefile(1 hunks)README.md(1 hunks)examples/auth_plugin/__init__.py(1 hunks)examples/auth_plugin/main.py(1 hunks)examples/logging_plugin/__init__.py(1 hunks)examples/logging_plugin/main.py(1 hunks)examples/rate_limit_plugin/__init__.py(1 hunks)examples/rate_limit_plugin/main.py(1 hunks)examples/simple_plugin/__init__.py(1 hunks)examples/simple_plugin/main.py(1 hunks)examples/transform_plugin/__init__.py(1 hunks)examples/transform_plugin/main.py(1 hunks)pyproject.toml(1 hunks)ruff.toml(1 hunks)scripts/build_plugin.sh(1 hunks)scripts/setup_uv.sh(1 hunks)src/mcpd_plugins/__init__.py(1 hunks)src/mcpd_plugins/base_plugin.py(1 hunks)src/mcpd_plugins/server.py(1 hunks)tests/conftest.py(1 hunks)tests/integration/test_examples.py(1 hunks)tests/unit/test_server.py(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-28T10:12:05.979Z
Learnt from: peteski22
PR: mozilla-ai/mcpd-plugins-sdk-python#1
File: examples/logging_plugin/main.py:72-72
Timestamp: 2025-10-28T10:12:05.979Z
Learning: In gRPC servicer implementations, the first parameter of RPC handler methods is conventionally named `request` by stub generators, even when the parameter type is semantically a response (e.g., `HandleResponse(self, request: HTTPResponse, ...)`). This follows gRPC terminology where "request" means "the data sent to this RPC method" rather than semantic meaning. This is an expected pattern and should not be flagged as an issue.
Applied to files:
examples/logging_plugin/main.py
🧬 Code graph analysis (15)
examples/rate_limit_plugin/__init__.py (1)
examples/rate_limit_plugin/main.py (1)
RateLimitPlugin(28-106)
tests/integration/test_examples.py (5)
tests/conftest.py (1)
mock_context(67-69)examples/simple_plugin/main.py (4)
SimplePlugin(27-63)GetMetadata(30-36)GetCapabilities(38-40)HandleRequest(42-63)examples/auth_plugin/main.py (4)
GetMetadata(38-44)GetCapabilities(46-48)HandleRequest(50-69)AuthPlugin(30-80)examples/logging_plugin/main.py (4)
GetMetadata(34-40)GetCapabilities(42-44)HandleRequest(46-70)LoggingPlugin(31-90)src/mcpd_plugins/base_plugin.py (3)
GetMetadata(70-87)GetCapabilities(89-102)HandleRequest(134-147)
examples/logging_plugin/__init__.py (1)
examples/logging_plugin/main.py (1)
LoggingPlugin(31-90)
examples/logging_plugin/main.py (2)
src/mcpd_plugins/base_plugin.py (5)
BasePlugin(16-167)GetMetadata(70-87)GetCapabilities(89-102)HandleRequest(134-147)HandleResponse(149-167)src/mcpd_plugins/server.py (1)
serve(18-138)
examples/transform_plugin/main.py (3)
src/mcpd_plugins/base_plugin.py (4)
BasePlugin(16-167)GetMetadata(70-87)GetCapabilities(89-102)HandleRequest(134-147)src/mcpd_plugins/server.py (1)
serve(18-138)examples/simple_plugin/main.py (3)
GetMetadata(30-36)GetCapabilities(38-40)HandleRequest(42-63)
tests/unit/test_server.py (3)
src/mcpd_plugins/base_plugin.py (1)
BasePlugin(16-167)src/mcpd_plugins/exceptions.py (1)
ServerError(16-19)src/mcpd_plugins/server.py (1)
serve(18-138)
examples/rate_limit_plugin/main.py (4)
src/mcpd_plugins/base_plugin.py (4)
BasePlugin(16-167)GetMetadata(70-87)GetCapabilities(89-102)HandleRequest(134-147)src/mcpd_plugins/server.py (1)
serve(18-138)examples/simple_plugin/main.py (3)
GetMetadata(30-36)GetCapabilities(38-40)HandleRequest(42-63)examples/transform_plugin/main.py (3)
GetMetadata(31-37)GetCapabilities(39-41)HandleRequest(43-104)
examples/simple_plugin/__init__.py (1)
examples/simple_plugin/main.py (1)
SimplePlugin(27-63)
src/mcpd_plugins/base_plugin.py (3)
src/mcpd_plugins/v1/plugins/plugin_pb2_grpc.py (17)
PluginServicer(89-140)Configure(92-96)Configure(196-223)Stop(98-102)Stop(226-253)GetMetadata(104-108)GetMetadata(256-283)GetCapabilities(110-114)GetCapabilities(286-313)CheckHealth(116-122)CheckHealth(316-343)CheckReady(124-128)CheckReady(346-373)HandleRequest(130-134)HandleRequest(376-403)HandleResponse(136-140)HandleResponse(406-433)examples/logging_plugin/main.py (4)
GetMetadata(34-40)GetCapabilities(42-44)HandleRequest(46-70)HandleResponse(72-90)examples/simple_plugin/main.py (3)
GetMetadata(30-36)GetCapabilities(38-40)HandleRequest(42-63)
examples/auth_plugin/main.py (2)
src/mcpd_plugins/base_plugin.py (4)
BasePlugin(16-167)GetMetadata(70-87)GetCapabilities(89-102)HandleRequest(134-147)src/mcpd_plugins/server.py (1)
serve(18-138)
examples/simple_plugin/main.py (3)
src/mcpd_plugins/base_plugin.py (4)
BasePlugin(16-167)GetMetadata(70-87)GetCapabilities(89-102)HandleRequest(134-147)src/mcpd_plugins/server.py (1)
serve(18-138)examples/transform_plugin/main.py (3)
GetMetadata(31-37)GetCapabilities(39-41)HandleRequest(43-104)
src/mcpd_plugins/__init__.py (3)
src/mcpd_plugins/base_plugin.py (1)
BasePlugin(16-167)src/mcpd_plugins/exceptions.py (3)
ConfigurationError(10-13)PluginError(4-7)ServerError(16-19)src/mcpd_plugins/server.py (1)
serve(18-138)
src/mcpd_plugins/server.py (3)
src/mcpd_plugins/base_plugin.py (1)
BasePlugin(16-167)src/mcpd_plugins/exceptions.py (1)
ServerError(16-19)src/mcpd_plugins/v1/plugins/plugin_pb2_grpc.py (1)
add_PluginServicer_to_server(143-188)
examples/auth_plugin/__init__.py (1)
examples/auth_plugin/main.py (1)
AuthPlugin(30-80)
examples/transform_plugin/__init__.py (1)
examples/transform_plugin/main.py (1)
TransformPlugin(28-104)
🪛 LanguageTool
README.md
[uncategorized] ~238-~238: Loose punctuation mark.
Context: ...mode on TCP port 50051. - grace_period: Seconds to wait during graceful shutdow...
(UNLIKELY_OPENING_PUNCTUATION)
[locale-violation] ~258-~258: License must be spelled with a “c” when used as a noun in British English. Use “licence”.
Context: ...ase see SECURITY.md. ## License This project is licensed under the Apa...
(LICENCE_LICENSE_NOUN_SINGULAR)
[locale-violation] ~260-~260: License must be spelled with a “c” when used as a noun in British English. Use “licence”.
Context: ...is project is licensed under the Apache License 2.0 - see the LICENSE file f...
(LICENCE_LICENSE_NOUN_SINGULAR)
[locale-violation] ~260-~260: LICENSE must be spelled with a “c” when used as a noun in British English. Use “licence”.
Context: ...under the Apache License 2.0 - see the LICENSE file for details. ## Related...
(LICENCE_LICENSE_NOUN_SINGULAR)
🪛 Ruff (0.14.1)
examples/logging_plugin/main.py
34-34: Unused method argument: request
(ARG002)
34-34: Unused method argument: context
(ARG002)
42-42: Unused method argument: request
(ARG002)
42-42: Unused method argument: context
(ARG002)
46-46: Unused method argument: context
(ARG002)
72-72: Unused method argument: context
(ARG002)
examples/transform_plugin/main.py
31-31: Unused method argument: request
(ARG002)
31-31: Unused method argument: context
(ARG002)
39-39: Unused method argument: request
(ARG002)
39-39: Unused method argument: context
(ARG002)
43-43: Unused method argument: context
(ARG002)
tests/unit/test_server.py
50-50: Unused method argument: mock_signal
(ARG002)
77-77: Unused method argument: mock_signal
(ARG002)
78-78: Unused method argument: mock_add_servicer
(ARG002)
103-103: Unused method argument: mock_signal
(ARG002)
104-104: Unused method argument: mock_add_servicer
(ARG002)
133-133: Unused method argument: mock_signal
(ARG002)
134-134: Unused method argument: mock_add_servicer
(ARG002)
145-145: Probable insecure usage of temporary file or directory: "/tmp/plugin.sock"
(S108)
164-164: Unused method argument: mock_signal
(ARG002)
165-165: Unused method argument: mock_add_servicer
(ARG002)
176-176: Probable insecure usage of temporary file or directory: "/tmp/plugin.sock"
(S108)
200-200: Unused method argument: mock_signal
(ARG002)
201-201: Unused method argument: mock_add_servicer
(ARG002)
225-225: Unused method argument: mock_signal
(ARG002)
226-226: Unused method argument: mock_add_servicer
(ARG002)
249-249: Unused method argument: mock_signal
(ARG002)
250-250: Unused method argument: mock_add_servicer
(ARG002)
277-277: Unused method argument: mock_signal
(ARG002)
299-299: Unused method argument: mock_add_servicer
(ARG002)
310-310: Unused method argument: mock_add_servicer
(ARG002)
315-315: Pattern passed to match= contains metacharacters but is neither escaped nor raw
(RUF043)
331-331: Unused method argument: mock_add_servicer
(ARG002)
362-362: Unused method argument: mock_signal
(ARG002)
363-363: Unused method argument: mock_add_servicer
(ARG002)
389-389: Unused method argument: mock_signal
(ARG002)
390-390: Unused method argument: mock_add_servicer
(ARG002)
examples/rate_limit_plugin/main.py
31-31: Missing return type annotation for special method __init__
Add return type annotation: None
(ANN204)
46-46: Unused method argument: request
(ARG002)
46-46: Unused method argument: context
(ARG002)
54-54: Unused method argument: request
(ARG002)
54-54: Unused method argument: context
(ARG002)
58-58: Unused method argument: context
(ARG002)
src/mcpd_plugins/base_plugin.py
134-134: Unused method argument: request
(ARG002)
149-149: Unused method argument: response
(ARG002)
examples/auth_plugin/main.py
33-33: Missing return type annotation for special method __init__
Add return type annotation: None
(ANN204)
38-38: Unused method argument: request
(ARG002)
38-38: Unused method argument: context
(ARG002)
46-46: Unused method argument: request
(ARG002)
46-46: Unused method argument: context
(ARG002)
50-50: Unused method argument: context
(ARG002)
examples/simple_plugin/main.py
30-30: Unused method argument: request
(ARG002)
30-30: Unused method argument: context
(ARG002)
38-38: Unused method argument: request
(ARG002)
38-38: Unused method argument: context
(ARG002)
42-42: Unused method argument: context
(ARG002)
src/mcpd_plugins/server.py
79-82: Avoid specifying long messages outside the exception class
(TRY003)
107-107: Abstract raise to an inner function
(TRY301)
107-107: Avoid specifying long messages outside the exception class
(TRY003)
109-109: Avoid specifying long messages outside the exception class
(TRY003)
138-138: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (20)
.gitignore (1)
1-62: Well-organised and comprehensive ignore patterns.The file is effectively structured with clear section comments and covers all necessary Python development artefacts, build outputs, IDE files, testing caches, and project-specific generated files. The previously flagged duplicate
*.egg-info/entry has been correctly resolved. The inclusion of Ruff caching, setuptools_scm version file, and project-specific directories (proto downloads, Claude IDE) shows thoughtful consideration of the SDK's build and development workflow.README.md (1)
1-268: Comprehensive documentation – looks excellent!The README provides clear, well-structured documentation covering installation, quick start, core concepts, five example plugins, development workflows, and a complete API reference. The documentation aligns well with the SDK implementation and will serve users effectively.
examples/rate_limit_plugin/__init__.py (1)
1-5: LGTM – clean package export.The module properly re-exports
RateLimitPluginfrom.mainand declares the public API via__all__. This pattern is consistent with the other example plugins.examples/auth_plugin/__init__.py (1)
1-5: LGTM – clean package export.The module properly re-exports
AuthPluginfrom.mainand declares the public API via__all__. Consistent with the SDK's example plugin patterns.examples/logging_plugin/__init__.py (1)
1-5: LGTM – clean package export.The module properly re-exports
LoggingPluginfrom.mainand declares the public API via__all__. Follows the established pattern for example plugins.examples/simple_plugin/__init__.py (1)
1-5: LGTM – clean package export.The module properly re-exports
SimplePluginfrom.mainand declares the public API via__all__. Consistent with the SDK's package structure.pyproject.toml (2)
1-61: Configuration structure is well-organized.The project configuration properly defines the package structure, dependencies, dependency groups, build system, and tool settings. The src layout, dynamic versioning via
setuptools_scm, and modular dependency groups are appropriate for this SDK project.
9-10: Dependency versions verified as current and secure.Verification confirms that
grpcio>=1.76.0andprotobuf>=6.33.0are both the latest available versions on PyPI and are free from known security vulnerabilities. All previously published advisories affecting these packages target older version ranges that fall below the specified minimum versions.ruff.toml (1)
1-77: Well-configured linting rules.The Ruff configuration is comprehensive and appropriate:
- Python 3.11 target aligns with the project's minimum Python version
- Comprehensive rule selection covering code quality, style, and documentation
- Proper exclusions for generated files (
plugin_pb2*.py,_version.py)- Reasonable per-file ignores for tests and protobuf-convention methods
- The
fix = truesetting enables automatic fixes, which is suitable for a project with CI enforcementexamples/transform_plugin/__init__.py (1)
1-5: LGTM – clean package export.The module properly re-exports
TransformPluginfrom.mainand declares the public API via__all__. Maintains consistency with the other example plugins in the SDK.scripts/setup_uv.sh (1)
1-55: LGTM! Script correctly handles PATH modification and user guidance.The PATH check logic has been properly implemented. The script now captures the initial PATH state before modification and uses it to determine whether to show the permanent PATH update guidance to users. The overall flow (UV installation/update, venv bootstrap, pre-commit hooks) is well-structured and safe.
tests/integration/test_examples.py (1)
1-161: LGTM! Comprehensive integration tests with proper assertions.The integration tests properly validate the example plugins' behavior. All issues from previous reviews have been addressed: the duplicate fixture was removed, assertion logic was corrected to verify both response continuation and logging independently, and the duplicate assertion was eliminated. The test coverage effectively validates metadata, capabilities, and request handling for each example plugin.
examples/simple_plugin/main.py (1)
27-63: LGTM! Clean example demonstrating minimal plugin implementation.The SimplePlugin correctly demonstrates the essential pattern for building plugins: override the necessary methods (GetMetadata, GetCapabilities, HandleRequest), create a response with
continue=True, copy and modify request headers, and preserve other request fields. The static analysis warnings about unused method arguments are false positives—these parameters are required by the gRPC interface signature.scripts/build_plugin.sh (1)
1-195: LGTM! Build script correctly handles both PyInstaller and Nuitka backends.The script properly supports both build tools with appropriate flags. All issues from previous reviews have been addressed: the unused ONEFILE variable was removed, Nuitka optimisation flags were corrected to use
--lto=yes, and redundant conditionals were eliminated. The argument parsing, validation, and build orchestration are well-structured and user-friendly.tests/unit/test_server.py (1)
1-405: LGTM! Comprehensive test coverage for server functionality.The test suite thoroughly validates all server behaviour: standalone and mcpd modes, port selection, environment variable handling, address formatting across network types, binding failures, signal handler registration, and lifecycle management. The previous concern about limited test coverage has been fully addressed. Static analysis warnings about unused fixtures are false positives (pytest's dependency injection requires them), and the
/tmpusage is acceptable in test code.src/mcpd_plugins/__init__.py (1)
1-56: LGTM! Clean public API with correct example and exports.The module properly serves as the SDK's public API entry point. All issues from previous reviews have been addressed: the docstring example now includes all necessary imports (FLOW_REQUEST, HTTPResponse) and uses the correct constant syntax, and
__all__is alphabetically sorted. The version fallback handling is appropriate for development installs.examples/rate_limit_plugin/main.py (1)
28-106: LGTM! Rate limiter correctly implements concurrent token bucket algorithm.The rate limiting plugin properly addresses the critical concurrency issue from previous reviews by using per-client asyncio locks (line 44, applied at line 63) to protect token refill and consumption operations. The logging has been improved to use lazy % formatting. The token bucket implementation correctly refills tokens over time, enforces limits, and provides appropriate headers (X-RateLimit-Limit, X-RateLimit-Remaining, Retry-After). As acknowledged by the developer, the "unknown" client IP fallback is acceptable for example code.
examples/transform_plugin/main.py (1)
43-104: LGTM! Transform plugin correctly handles charset extraction and error cases.The transformation plugin properly addresses all issues from previous reviews: charset extraction from the Content-Type header has been added (lines 59-62), error handling now catches both UnicodeDecodeError and JSONDecodeError, and
logger.exception()is used consistently to capture full tracebacks. The implementation correctly filters requests by method and Content-Type, transforms JSON dict payloads by injecting metadata, updates Content-Length, and handles error scenarios appropriately.examples/auth_plugin/main.py (1)
55-66: Authorisation header lookup should be case‑insensitive.HTTP header names are case‑insensitive. Current lookup can falsely reject valid requests.
- # Check for Authorization header. - auth_header = request.headers.get("Authorization", "") + # Case‑insensitive lookup for the 'Authorization' header. + auth_header = next((v for k, v in request.headers.items() if k.lower() == "authorization"), "") @@ - if not auth_header.startswith(f"{BEARER_SCHEME} "): + if not auth_header.startswith(f"{BEARER_SCHEME} "): logger.warning("Missing or invalid Authorization header") return self._unauthorized_response("Missing or invalid Authorization header") @@ - token = auth_header.removeprefix(f"{BEARER_SCHEME} ") + token = auth_header.removeprefix(f"{BEARER_SCHEME} ")Likely an incorrect or invalid review comment.
src/mcpd_plugins/base_plugin.py (1)
134-148: Silence unused parameters in default handlers.Keep signatures intact but bind unused params to
_to satisfy linters.async def HandleRequest(self, request: HTTPRequest, context: ServicerContext) -> HTTPResponse: @@ - _ = context # Required by gRPC interface. + _ = (request, context) # Required by gRPC interface. return HTTPResponse(**{"continue": True}) @@ async def HandleResponse(self, response: HTTPResponse, context: ServicerContext) -> HTTPResponse: @@ - _ = context # Required by gRPC interface. + _ = (response, context) # Required by gRPC interface. return HTTPResponse(**{"continue": True})Also applies to: 149-167
⛔ Skipped due to learnings
Learnt from: peteski22 PR: mozilla-ai/mcpd-plugins-sdk-python#1 File: examples/logging_plugin/main.py:72-72 Timestamp: 2025-10-28T10:12:05.979Z Learning: In gRPC servicer implementations, the first parameter of RPC handler methods is conventionally named `request` by stub generators, even when the parameter type is semantically a response (e.g., `HandleResponse(self, request: HTTPResponse, ...)`). This follows gRPC terminology where "request" means "the data sent to this RPC method" rather than semantic meaning. This is an expected pattern and should not be flagged as an issue.
peteski22
force-pushed
the
peteski22/project-setup
branch
from
2300395 to
d630722
Compare
There was a problem hiding this comment.
Actionable comments posted: 13
♻️ Duplicate comments (3)
ruff.toml (1)
3-5: Document rationale for Python 3.11 pin.Add a brief note (and cross-reference in README) explaining why
target-version = "py311"is required (features, dependency compat, CI/runtime baseline).src/mcpd_plugins/base_plugin.py (2)
134-147: Silence the unusedrequestparameter for consistency.The
requestparameter is not used in the default implementation but is not silenced, which is inconsistent with other methods in this class and triggers a linter warning (Ruff ARG002).Apply this diff to maintain consistency with other methods:
async def HandleRequest(self, request: HTTPRequest, context: ServicerContext) -> HTTPResponse: """Handle incoming HTTP request. Default implementation passes through unchanged (continue=True). Args: request: The incoming HTTP request to process. context: gRPC context for the request. Returns: HTTPResponse indicating how to proceed (continue, modify, or reject). """ - _ = context # Required by gRPC interface. + _ = (request, context) # Required by gRPC interface. return HTTPResponse(**{"continue": True})
149-167: Silence the unusedresponseparameter for consistency.The
responseparameter is not used in the default implementation but is not silenced, which is inconsistent with other methods in this class and triggers a linter warning (Ruff ARG002).Apply this diff to maintain consistency with other methods:
async def HandleResponse(self, response: HTTPResponse, context: ServicerContext) -> HTTPResponse: """Handle outgoing HTTP response. Default implementation passes through unchanged (continue=True). Note: The parameter is named 'response' for clarity, even though the generated gRPC stub names it 'request' (gRPC convention). This semantic naming improves code readability. Args: response: The outgoing HTTP response to process. context: gRPC context for the request. Returns: HTTPResponse indicating how to proceed (continue or modify). """ - _ = context # Required by gRPC interface. + _ = (response, context) # Required by gRPC interface. return HTTPResponse(**{"continue": True})
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (23)
.gitignore(1 hunks)Makefile(1 hunks)README.md(1 hunks)examples/auth_plugin/__init__.py(1 hunks)examples/auth_plugin/main.py(1 hunks)examples/logging_plugin/__init__.py(1 hunks)examples/logging_plugin/main.py(1 hunks)examples/rate_limit_plugin/__init__.py(1 hunks)examples/rate_limit_plugin/main.py(1 hunks)examples/simple_plugin/__init__.py(1 hunks)examples/simple_plugin/main.py(1 hunks)examples/transform_plugin/__init__.py(1 hunks)examples/transform_plugin/main.py(1 hunks)pyproject.toml(1 hunks)ruff.toml(1 hunks)scripts/build_plugin.sh(1 hunks)scripts/setup_uv.sh(1 hunks)src/mcpd_plugins/__init__.py(1 hunks)src/mcpd_plugins/base_plugin.py(1 hunks)src/mcpd_plugins/server.py(1 hunks)tests/conftest.py(1 hunks)tests/integration/test_examples.py(1 hunks)tests/unit/test_server.py(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-28T10:12:05.979Z
Learnt from: peteski22
PR: mozilla-ai/mcpd-plugins-sdk-python#1
File: examples/logging_plugin/main.py:72-72
Timestamp: 2025-10-28T10:12:05.979Z
Learning: In gRPC servicer implementations, the first parameter of RPC handler methods is conventionally named `request` by stub generators, even when the parameter type is semantically a response (e.g., `HandleResponse(self, request: HTTPResponse, ...)`). This follows gRPC terminology where "request" means "the data sent to this RPC method" rather than semantic meaning. This is an expected pattern and should not be flagged as an issue.
Applied to files:
examples/logging_plugin/main.py
🧬 Code graph analysis (15)
examples/auth_plugin/__init__.py (1)
examples/auth_plugin/main.py (1)
AuthPlugin(31-83)
examples/transform_plugin/main.py (3)
src/mcpd_plugins/base_plugin.py (4)
BasePlugin(16-167)GetMetadata(70-87)GetCapabilities(89-102)HandleRequest(134-147)src/mcpd_plugins/server.py (1)
serve(45-179)examples/simple_plugin/main.py (3)
GetMetadata(30-36)GetCapabilities(38-40)HandleRequest(42-63)
examples/simple_plugin/__init__.py (1)
examples/simple_plugin/main.py (1)
SimplePlugin(27-63)
src/mcpd_plugins/server.py (3)
src/mcpd_plugins/base_plugin.py (1)
BasePlugin(16-167)src/mcpd_plugins/exceptions.py (1)
ServerError(16-19)src/mcpd_plugins/v1/plugins/plugin_pb2_grpc.py (1)
add_PluginServicer_to_server(143-188)
examples/simple_plugin/main.py (2)
src/mcpd_plugins/base_plugin.py (4)
BasePlugin(16-167)GetMetadata(70-87)GetCapabilities(89-102)HandleRequest(134-147)src/mcpd_plugins/server.py (1)
serve(45-179)
examples/auth_plugin/main.py (2)
src/mcpd_plugins/base_plugin.py (4)
BasePlugin(16-167)GetMetadata(70-87)GetCapabilities(89-102)HandleRequest(134-147)src/mcpd_plugins/server.py (1)
serve(45-179)
examples/rate_limit_plugin/__init__.py (1)
examples/rate_limit_plugin/main.py (1)
RateLimitPlugin(28-106)
examples/logging_plugin/main.py (2)
src/mcpd_plugins/base_plugin.py (5)
BasePlugin(16-167)GetMetadata(70-87)GetCapabilities(89-102)HandleRequest(134-147)HandleResponse(149-167)src/mcpd_plugins/server.py (1)
serve(45-179)
tests/unit/test_server.py (3)
src/mcpd_plugins/base_plugin.py (1)
BasePlugin(16-167)src/mcpd_plugins/exceptions.py (1)
ServerError(16-19)src/mcpd_plugins/server.py (1)
serve(45-179)
src/mcpd_plugins/__init__.py (3)
src/mcpd_plugins/base_plugin.py (1)
BasePlugin(16-167)src/mcpd_plugins/exceptions.py (3)
ConfigurationError(10-13)PluginError(4-7)ServerError(16-19)src/mcpd_plugins/server.py (1)
serve(45-179)
examples/logging_plugin/__init__.py (1)
examples/logging_plugin/main.py (1)
LoggingPlugin(31-94)
examples/rate_limit_plugin/main.py (2)
src/mcpd_plugins/base_plugin.py (4)
BasePlugin(16-167)GetMetadata(70-87)GetCapabilities(89-102)HandleRequest(134-147)src/mcpd_plugins/server.py (1)
serve(45-179)
examples/transform_plugin/__init__.py (1)
examples/transform_plugin/main.py (1)
TransformPlugin(28-104)
tests/integration/test_examples.py (4)
tests/conftest.py (1)
mock_context(67-69)examples/simple_plugin/main.py (4)
SimplePlugin(27-63)GetMetadata(30-36)GetCapabilities(38-40)HandleRequest(42-63)examples/auth_plugin/main.py (4)
GetMetadata(39-45)GetCapabilities(47-49)HandleRequest(51-70)AuthPlugin(31-83)examples/logging_plugin/main.py (4)
GetMetadata(34-41)GetCapabilities(43-46)HandleRequest(48-73)LoggingPlugin(31-94)
src/mcpd_plugins/base_plugin.py (1)
src/mcpd_plugins/v1/plugins/plugin_pb2_grpc.py (17)
PluginServicer(89-140)Configure(92-96)Configure(196-223)Stop(98-102)Stop(226-253)GetMetadata(104-108)GetMetadata(256-283)GetCapabilities(110-114)GetCapabilities(286-313)CheckHealth(116-122)CheckHealth(316-343)CheckReady(124-128)CheckReady(346-373)HandleRequest(130-134)HandleRequest(376-403)HandleResponse(136-140)HandleResponse(406-433)
🪛 LanguageTool
README.md
[uncategorized] ~238-~238: Loose punctuation mark.
Context: ...mode on TCP port 50051. - grace_period: Seconds to wait during graceful shutdow...
(UNLIKELY_OPENING_PUNCTUATION)
[locale-violation] ~258-~258: License must be spelled with a “c” when used as a noun in British English. Use “licence”.
Context: ...ase see SECURITY.md. ## License This project is licensed under the Apa...
(LICENCE_LICENSE_NOUN_SINGULAR)
[locale-violation] ~260-~260: License must be spelled with a “c” when used as a noun in British English. Use “licence”.
Context: ...is project is licensed under the Apache License 2.0 - see the LICENSE file f...
(LICENCE_LICENSE_NOUN_SINGULAR)
[locale-violation] ~260-~260: LICENSE must be spelled with a “c” when used as a noun in British English. Use “licence”.
Context: ...under the Apache License 2.0 - see the LICENSE file for details. ## Related...
(LICENCE_LICENSE_NOUN_SINGULAR)
🪛 Ruff (0.14.1)
examples/transform_plugin/main.py
31-31: Unused method argument: request
(ARG002)
31-31: Unused method argument: context
(ARG002)
39-39: Unused method argument: request
(ARG002)
39-39: Unused method argument: context
(ARG002)
43-43: Unused method argument: context
(ARG002)
src/mcpd_plugins/server.py
41-41: Avoid specifying long messages outside the exception class
(TRY003)
109-112: Avoid specifying long messages outside the exception class
(TRY003)
140-140: Abstract raise to an inner function
(TRY301)
140-140: Avoid specifying long messages outside the exception class
(TRY003)
142-142: Avoid specifying long messages outside the exception class
(TRY003)
179-179: Avoid specifying long messages outside the exception class
(TRY003)
examples/simple_plugin/main.py
30-30: Unused method argument: request
(ARG002)
30-30: Unused method argument: context
(ARG002)
38-38: Unused method argument: request
(ARG002)
38-38: Unused method argument: context
(ARG002)
42-42: Unused method argument: context
(ARG002)
examples/auth_plugin/main.py
39-39: Unused method argument: request
(ARG002)
39-39: Unused method argument: context
(ARG002)
47-47: Unused method argument: request
(ARG002)
47-47: Unused method argument: context
(ARG002)
51-51: Unused method argument: context
(ARG002)
tests/unit/test_server.py
50-50: Unused method argument: mock_signal
(ARG002)
77-77: Unused method argument: mock_signal
(ARG002)
78-78: Unused method argument: mock_add_servicer
(ARG002)
103-103: Unused method argument: mock_signal
(ARG002)
104-104: Unused method argument: mock_add_servicer
(ARG002)
133-133: Unused method argument: mock_signal
(ARG002)
134-134: Unused method argument: mock_add_servicer
(ARG002)
145-145: Probable insecure usage of temporary file or directory: "/tmp/plugin.sock"
(S108)
164-164: Unused method argument: mock_signal
(ARG002)
165-165: Unused method argument: mock_add_servicer
(ARG002)
176-176: Probable insecure usage of temporary file or directory: "/tmp/plugin.sock"
(S108)
200-200: Unused method argument: mock_signal
(ARG002)
201-201: Unused method argument: mock_add_servicer
(ARG002)
225-225: Unused method argument: mock_signal
(ARG002)
226-226: Unused method argument: mock_add_servicer
(ARG002)
249-249: Unused method argument: mock_signal
(ARG002)
250-250: Unused method argument: mock_add_servicer
(ARG002)
277-277: Unused method argument: mock_signal
(ARG002)
299-299: Unused method argument: mock_add_servicer
(ARG002)
310-310: Unused method argument: mock_add_servicer
(ARG002)
315-315: Pattern passed to match= contains metacharacters but is neither escaped nor raw
(RUF043)
331-331: Unused method argument: mock_add_servicer
(ARG002)
362-362: Unused method argument: mock_signal
(ARG002)
363-363: Unused method argument: mock_add_servicer
(ARG002)
389-389: Unused method argument: mock_signal
(ARG002)
390-390: Unused method argument: mock_add_servicer
(ARG002)
examples/rate_limit_plugin/main.py
46-46: Unused method argument: request
(ARG002)
46-46: Unused method argument: context
(ARG002)
54-54: Unused method argument: request
(ARG002)
54-54: Unused method argument: context
(ARG002)
58-58: Unused method argument: context
(ARG002)
src/mcpd_plugins/base_plugin.py
134-134: Unused method argument: request
(ARG002)
149-149: Unused method argument: response
(ARG002)
🔇 Additional comments (13)
.gitignore (1)
1-62: File structure and coverage look solid.The
.gitignoreis comprehensive and well-organised for a Python SDK project with gRPC/protobuf tooling, covering Python artefacts, virtual environments, IDE junk, testing and type-checking caches, Ruff cache, generated files, and OS-specific entries. The previous duplicate entry has been resolved.examples/simple_plugin/__init__.py (1)
1-5: LGTM: ergonomic re-export.Re-exporting
SimplePluginsimplifies imports for consumers.examples/logging_plugin/__init__.py (1)
1-5: LGTM: ergonomic re-export.Publicly exposing
LoggingPluginat the package root is clean and consistent.examples/auth_plugin/__init__.py (1)
1-5: LGTM: ergonomic re-export.
AuthPluginexport matches the pattern used across examples.examples/transform_plugin/__init__.py (1)
1-5: LGTM: ergonomic re-export.
TransformPluginis cleanly exposed for consumers.examples/rate_limit_plugin/__init__.py (1)
1-5: LGTM: public re-export is tidy and idiomatic.
RateLimitPluginis exposed at the package root and__all__is set correctly. No further action.scripts/build_plugin.sh (1)
118-156: Nuitka path looks solid after LTO fix.LTO enabled for both size/speed presets; include‑package settings cover grpc/protobuf; output naming and onefile are consistent. No blockers.
If you want, I can add optional PGO presets (profile generate/use) guarded by a flag for extra performance.
tests/unit/test_server.py (1)
13-13: Rename unused mock parameters to_to comply with Ruff ARG002; autouse fixture is optional but good practice.The tests already prevent filesystem side-effects through comprehensive mocking of
aio.server()and related gRPC functions. The hard-coded socket paths (/tmp,/var/run) are strings passed only to mocked functions and pose no hermiticity risk.However, multiple test methods receive unused
@patchdecorators and parameters (e.g.,mock_signalis never asserted). Rename these to_to suppress linter warnings:async def test_standalone_uses_default_port_50051( self, mock_event_class, - mock_signal, + _, mock_add_servicer,The autouse fixture suggestion for
_cleanup_unix_socketis valid defensive practice but not necessary given the current test isolation. If added, ensure it patches the function path correctly:+@pytest.fixture(autouse=True) +def _stub_cleanup_unix_socket(): + with patch("mcpd_plugins.server._cleanup_unix_socket"): + yieldtests/conftest.py (1)
28-30: No action required —Metadata()instantiation is valid.In grpcio 1.76.0 (the minimum version specified in
pyproject.toml),grpc.aio.Metadatais a concrete class that can be instantiated. The previous review's concern about it being merely a type alias was incorrect. The code as written is safe and will work correctly.src/mcpd_plugins/base_plugin.py (4)
1-14: Imports and module structure are well-organized.The async-compatible
ServicerContextfromgrpc.aiois correctly used, and all necessary protobuf types are imported appropriately.
16-38: Excellent class documentation with practical example.The docstring clearly explains the purpose of the base class and provides a helpful example demonstrating how to override methods for custom behaviour.
40-102: Lifecycle and metadata methods are correctly implemented.All methods properly silence unused parameters and provide sensible default implementations with comprehensive docstrings. The previous review concerns have been addressed.
104-132: Health check methods correctly implemented.Both methods properly silence unused parameters and provide appropriate default implementations for health and readiness checks.
peteski22
force-pushed
the
peteski22/project-setup
branch
from
d630722 to
ba1409c
Compare
There was a problem hiding this comment.
Actionable comments posted: 19
♻️ Duplicate comments (7)
README.md (1)
17-25: Add pre‑release/local install instructions.Until published on PyPI, include editable/local commands (uv/pip) to avoid confusion.
## Installation ```bash # Using uv (recommended) uv add mcpd-plugins # Using pip pip install mcpd-plugins + +# Pre‑release / local install (from source) +# Using uv +uv add --editable . +# Using pip +uv pip install -e .</blockquote></details> <details> <summary>scripts/setup_uv.sh (2)</summary><blockquote> `22-26`: **Consider pinning the UV installer version.** The script installs the latest UV release, which reduces reproducibility and supply-chain guarantees. Pinning to a known-good version (with an environment variable override) would improve consistency across environments. Example with version pin: ```diff if ! command -v uv &>/dev/null; then echo "uv not found – installing to $LOCAL_BIN" - curl -fsSL https://astral.sh/uv/install.sh | UV_INSTALL_DIR="$LOCAL_BIN" sh + UV_VERSION="${UV_VERSION:-0.5.0}" # Pin to known version; override via env if needed + curl -fsSL https://astral.sh/uv/install.sh | UV_INSTALL_DIR="$LOCAL_BIN" UV_VERSION="$UV_VERSION" sh
39-42: Consider making dependency groups configurable.Using
--group allinstalls all dependencies, which may include heavy build toolchains. Defaulting to lighter development groups (e.g.,dev,lint,tests) with an opt-in mechanism for additional groups would improve setup performance.Apply this diff:
echo "Bootstrapping root .venv in folder $VENV_DIR" uv venv "$VENV_DIR" -uv sync --group all --active +# Default to core dev groups; override with GROUPS env (e.g., GROUPS="all") +uv sync --group "${GROUPS:-dev,lint,tests}" --activeexamples/auth_plugin/main.py (1)
39-70: Silence unused parameters to satisfy linters.The static analysis tool flags unused
requestandcontextparameters inGetMetadata,GetCapabilities, andHandleRequest. Whilst these parameters are required by the gRPC interface, explicitly silencing them improves consistency with the pattern established inBasePlugin.Apply this diff:
async def GetMetadata(self, request: Empty, context: ServicerContext) -> Metadata: """Return plugin metadata.""" + _ = (request, context) return Metadata( name="auth-plugin", version="1.0.0", description="Validates Bearer token authentication", ) async def GetCapabilities(self, request: Empty, context: ServicerContext) -> Capabilities: """Declare support for request flow.""" + _ = (request, context) return Capabilities(flows=[FLOW_REQUEST]) async def HandleRequest(self, request: HTTPRequest, context: ServicerContext) -> HTTPResponse: """Validate Bearer token in Authorization header.""" + _ = context logger.info("Authenticating request: %s %s", request.method, request.url)Based on coding patterns in
src/mcpd_plugins/base_plugin.py.tests/integration/test_examples.py (1)
9-12: Avoid polluting sys.path; import examples as a package.Insert repo root, then import via
examples.*to prevent top‑level name clashes.Apply this diff:
-# Add examples to path. -examples_dir = Path(__file__).parent.parent.parent / "examples" -sys.path.insert(0, str(examples_dir)) +# Add repository root so we can import 'examples.*' packages. +repo_root = Path(__file__).resolve().parents[2] +sys.path.insert(0, str(repo_root))And update imports, e.g.:
-from simple_plugin.main import SimplePlugin +from examples.simple_plugin.main import SimplePluginMakefile (1)
15-21: Gate test/lint on setup to ensure uv/deps exist.Prevents “uv: command not found” on fresh clones.
Apply this diff:
-.PHONY: test -test: ## Run tests with pytest +.PHONY: test +test: setup ## Run tests with pytest uv run pytest tests/ -v .PHONY: lint -lint: ## Run pre-commit hooks on all files +lint: setup ## Run pre-commit hooks on all files uv run pre-commit run --all-filestests/conftest.py (1)
3-7: Fix Metadata typing alias misuse in conftest.py — remove instantiation and add type hints.The
grpc.aio.Metadataimport is a type alias, not a concrete class. Instantiating it withMetadata()causes a runtime error. Changeinvocation_metadatato an empty list with explicit type annotation, remove the problematic import, and add the return type toabort().-import pytest -from google.protobuf.empty_pb2 import Empty -from grpc.aio import Metadata +import pytest +from google.protobuf.empty_pb2 import Empty from mcpd_plugins.v1.plugins.plugin_pb2 import HTTPRequest, PluginConfig @@ -28,7 +28,7 @@ class MockServicerContext: def __init__(self) -> None: """Initialize mock context.""" - self.invocation_metadata = Metadata() + self.invocation_metadata: list[tuple[str, str]] = [] self.peer_identity = None self.code = None self.details = None @@ -51,7 +51,7 @@ class MockServicerContext: - def abort(self, code, details: str): + def abort(self, code, details: str) -> None: """Mock abort method.""" self.code = code self.details = details
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (23)
.gitignore(1 hunks)Makefile(1 hunks)README.md(1 hunks)examples/auth_plugin/__init__.py(1 hunks)examples/auth_plugin/main.py(1 hunks)examples/logging_plugin/__init__.py(1 hunks)examples/logging_plugin/main.py(1 hunks)examples/rate_limit_plugin/__init__.py(1 hunks)examples/rate_limit_plugin/main.py(1 hunks)examples/simple_plugin/__init__.py(1 hunks)examples/simple_plugin/main.py(1 hunks)examples/transform_plugin/__init__.py(1 hunks)examples/transform_plugin/main.py(1 hunks)pyproject.toml(1 hunks)ruff.toml(1 hunks)scripts/build_plugin.sh(1 hunks)scripts/setup_uv.sh(1 hunks)src/mcpd_plugins/__init__.py(1 hunks)src/mcpd_plugins/base_plugin.py(1 hunks)src/mcpd_plugins/server.py(1 hunks)tests/conftest.py(1 hunks)tests/integration/test_examples.py(1 hunks)tests/unit/test_server.py(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-28T10:12:05.979Z
Learnt from: peteski22
PR: mozilla-ai/mcpd-plugins-sdk-python#1
File: examples/logging_plugin/main.py:72-72
Timestamp: 2025-10-28T10:12:05.979Z
Learning: In gRPC servicer implementations, the first parameter of RPC handler methods is conventionally named `request` by stub generators, even when the parameter type is semantically a response (e.g., `HandleResponse(self, request: HTTPResponse, ...)`). This follows gRPC terminology where "request" means "the data sent to this RPC method" rather than semantic meaning. This is an expected pattern and should not be flagged as an issue.
Applied to files:
examples/logging_plugin/main.py
🧬 Code graph analysis (15)
tests/integration/test_examples.py (4)
tests/conftest.py (1)
mock_context(67-69)examples/simple_plugin/main.py (4)
SimplePlugin(27-57)GetMetadata(30-37)GetCapabilities(39-42)HandleRequest(44-57)examples/auth_plugin/main.py (4)
GetMetadata(39-45)GetCapabilities(47-49)HandleRequest(51-70)AuthPlugin(31-83)examples/logging_plugin/main.py (4)
GetMetadata(34-41)GetCapabilities(43-46)HandleRequest(48-73)LoggingPlugin(31-94)
examples/logging_plugin/main.py (2)
src/mcpd_plugins/base_plugin.py (5)
BasePlugin(16-167)GetMetadata(70-87)GetCapabilities(89-102)HandleRequest(134-147)HandleResponse(149-167)src/mcpd_plugins/server.py (1)
serve(45-179)
examples/transform_plugin/__init__.py (1)
examples/transform_plugin/main.py (1)
TransformPlugin(28-104)
examples/logging_plugin/__init__.py (1)
examples/logging_plugin/main.py (1)
LoggingPlugin(31-94)
examples/auth_plugin/main.py (2)
src/mcpd_plugins/base_plugin.py (4)
BasePlugin(16-167)GetMetadata(70-87)GetCapabilities(89-102)HandleRequest(134-147)src/mcpd_plugins/server.py (1)
serve(45-179)
src/mcpd_plugins/server.py (3)
src/mcpd_plugins/base_plugin.py (1)
BasePlugin(16-167)src/mcpd_plugins/exceptions.py (1)
ServerError(16-19)src/mcpd_plugins/v1/plugins/plugin_pb2_grpc.py (1)
add_PluginServicer_to_server(143-188)
examples/simple_plugin/__init__.py (1)
examples/simple_plugin/main.py (1)
SimplePlugin(27-57)
tests/unit/test_server.py (2)
src/mcpd_plugins/exceptions.py (1)
ServerError(16-19)src/mcpd_plugins/server.py (1)
serve(45-179)
examples/auth_plugin/__init__.py (1)
examples/auth_plugin/main.py (1)
AuthPlugin(31-83)
src/mcpd_plugins/base_plugin.py (1)
src/mcpd_plugins/v1/plugins/plugin_pb2_grpc.py (17)
PluginServicer(89-140)Configure(92-96)Configure(196-223)Stop(98-102)Stop(226-253)GetMetadata(104-108)GetMetadata(256-283)GetCapabilities(110-114)GetCapabilities(286-313)CheckHealth(116-122)CheckHealth(316-343)CheckReady(124-128)CheckReady(346-373)HandleRequest(130-134)HandleRequest(376-403)HandleResponse(136-140)HandleResponse(406-433)
src/mcpd_plugins/__init__.py (3)
src/mcpd_plugins/base_plugin.py (1)
BasePlugin(16-167)src/mcpd_plugins/exceptions.py (3)
ConfigurationError(10-13)PluginError(4-7)ServerError(16-19)src/mcpd_plugins/server.py (1)
serve(45-179)
examples/rate_limit_plugin/__init__.py (1)
examples/rate_limit_plugin/main.py (1)
RateLimitPlugin(28-106)
examples/transform_plugin/main.py (3)
src/mcpd_plugins/base_plugin.py (4)
BasePlugin(16-167)GetMetadata(70-87)GetCapabilities(89-102)HandleRequest(134-147)src/mcpd_plugins/server.py (1)
serve(45-179)examples/simple_plugin/main.py (3)
GetMetadata(30-37)GetCapabilities(39-42)HandleRequest(44-57)
examples/rate_limit_plugin/main.py (3)
src/mcpd_plugins/base_plugin.py (4)
BasePlugin(16-167)GetMetadata(70-87)GetCapabilities(89-102)HandleRequest(134-147)src/mcpd_plugins/server.py (1)
serve(45-179)examples/simple_plugin/main.py (3)
GetMetadata(30-37)GetCapabilities(39-42)HandleRequest(44-57)
examples/simple_plugin/main.py (3)
src/mcpd_plugins/base_plugin.py (4)
BasePlugin(16-167)GetMetadata(70-87)GetCapabilities(89-102)HandleRequest(134-147)src/mcpd_plugins/server.py (1)
serve(45-179)examples/transform_plugin/main.py (3)
GetMetadata(31-37)GetCapabilities(39-41)HandleRequest(43-104)
🪛 LanguageTool
README.md
[uncategorized] ~238-~238: Loose punctuation mark.
Context: ...mode on TCP port 50051. - grace_period: Seconds to wait during graceful shutdow...
(UNLIKELY_OPENING_PUNCTUATION)
[locale-violation] ~258-~258: License must be spelled with a “c” when used as a noun in British English. Use “licence”.
Context: ...ase see SECURITY.md. ## License This project is licensed under the Apa...
(LICENCE_LICENSE_NOUN_SINGULAR)
[locale-violation] ~260-~260: License must be spelled with a “c” when used as a noun in British English. Use “licence”.
Context: ...is project is licensed under the Apache License 2.0 - see the LICENSE file f...
(LICENCE_LICENSE_NOUN_SINGULAR)
[locale-violation] ~260-~260: LICENSE must be spelled with a “c” when used as a noun in British English. Use “licence”.
Context: ...under the Apache License 2.0 - see the LICENSE file for details. ## Related...
(LICENCE_LICENSE_NOUN_SINGULAR)
🪛 Ruff (0.14.2)
examples/auth_plugin/main.py
39-39: Unused method argument: request
(ARG002)
39-39: Unused method argument: context
(ARG002)
47-47: Unused method argument: request
(ARG002)
47-47: Unused method argument: context
(ARG002)
51-51: Unused method argument: context
(ARG002)
src/mcpd_plugins/server.py
41-41: Avoid specifying long messages outside the exception class
(TRY003)
109-112: Avoid specifying long messages outside the exception class
(TRY003)
140-140: Abstract raise to an inner function
(TRY301)
140-140: Avoid specifying long messages outside the exception class
(TRY003)
142-142: Avoid specifying long messages outside the exception class
(TRY003)
179-179: Avoid specifying long messages outside the exception class
(TRY003)
tests/unit/test_server.py
50-50: Unused method argument: mock_signal
(ARG002)
77-77: Unused method argument: mock_signal
(ARG002)
78-78: Unused method argument: mock_add_servicer
(ARG002)
103-103: Unused method argument: mock_signal
(ARG002)
104-104: Unused method argument: mock_add_servicer
(ARG002)
133-133: Unused method argument: mock_signal
(ARG002)
134-134: Unused method argument: mock_add_servicer
(ARG002)
166-166: Unused method argument: mock_signal
(ARG002)
167-167: Unused method argument: mock_add_servicer
(ARG002)
204-204: Unused method argument: mock_signal
(ARG002)
205-205: Unused method argument: mock_add_servicer
(ARG002)
231-231: Unused method argument: mock_signal
(ARG002)
232-232: Unused method argument: mock_add_servicer
(ARG002)
255-255: Unused method argument: mock_signal
(ARG002)
256-256: Unused method argument: mock_add_servicer
(ARG002)
283-283: Unused method argument: mock_signal
(ARG002)
305-305: Unused method argument: mock_add_servicer
(ARG002)
316-316: Unused method argument: mock_add_servicer
(ARG002)
337-337: Unused method argument: mock_add_servicer
(ARG002)
368-368: Unused method argument: mock_signal
(ARG002)
369-369: Unused method argument: mock_add_servicer
(ARG002)
395-395: Unused method argument: mock_signal
(ARG002)
396-396: Unused method argument: mock_add_servicer
(ARG002)
src/mcpd_plugins/base_plugin.py
134-134: Unused method argument: request
(ARG002)
149-149: Unused method argument: response
(ARG002)
examples/transform_plugin/main.py
31-31: Unused method argument: request
(ARG002)
31-31: Unused method argument: context
(ARG002)
39-39: Unused method argument: request
(ARG002)
39-39: Unused method argument: context
(ARG002)
43-43: Unused method argument: context
(ARG002)
examples/rate_limit_plugin/main.py
46-46: Unused method argument: request
(ARG002)
46-46: Unused method argument: context
(ARG002)
54-54: Unused method argument: request
(ARG002)
54-54: Unused method argument: context
(ARG002)
58-58: Unused method argument: context
(ARG002)
🔇 Additional comments (8)
ruff.toml (1)
34-51: LGTM: sensible lint rule set and per‑file ignores.The chosen rule families and ignores balance rigour with practicality for generated code and examples.
pyproject.toml (1)
8-12: LGTM: runtime deps and major‑version pin.
grpcio>=1.76.0andprotobuf>=6.33.0,<7look appropriate; setuptools‑scm wiring andpy.typedare correctly configured.examples/simple_plugin/__init__.py (1)
1-5: LGTM!Clean package initialiser following Python best practices. The re-export makes
SimplePluginconveniently accessible from the package root.examples/auth_plugin/__init__.py (1)
1-5: LGTM!Package structure follows the established pattern across example plugins.
examples/logging_plugin/__init__.py (1)
1-5: LGTM!Consistent package structure with other example plugins.
examples/rate_limit_plugin/__init__.py (1)
1-5: LGTM!Package initialiser follows the established pattern.
examples/transform_plugin/__init__.py (1)
1-5: LGTM!Package follows the consistent re-export pattern used across all example plugins.
examples/transform_plugin/main.py (1)
101-104: Clarify fail-open vs fail-closed on unexpected errors.Currently, unexpected exceptions allow traffic to continue. Confirm this is the desired policy for a transform stage; many pipelines prefer fail‑closed for corruption.
Would you like me to flip this path to return
continue=False(with 5xx) or keep fail‑open and add metrics?
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
tests/integration/test_examples.py (1)
10-12: Import via the package path instead of insertingexamplesdirectly.Pushing
examples/ontosys.pathmakes every module under that directory appear as a top-level package (e.g.simple_plugin), which risks clashes if a dependency or another test introduces a module with the same name. Prefer adding the repository root and importing through the package namespace, e.g.:-# Add examples to path. -examples_dir = Path(__file__).parent.parent.parent / "examples" -sys.path.insert(0, str(examples_dir)) +# Add the repository root so we can import via the package namespace. +repo_root = Path(__file__).resolve().parents[2] +sys.path.insert(0, str(repo_root))Then update the imports to
from examples.simple_plugin.main import SimplePlugin,from examples.auth_plugin.main import AuthPlugin, etc., so we never shadow real top-level modules.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
tests/integration/test_examples.py(1 hunks)tests/unit/test_server.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
tests/unit/test_server.py (3)
src/mcpd_plugins/base_plugin.py (1)
BasePlugin(16-167)src/mcpd_plugins/exceptions.py (1)
ServerError(16-19)src/mcpd_plugins/server.py (1)
serve(45-179)
tests/integration/test_examples.py (4)
tests/conftest.py (1)
mock_context(67-69)examples/simple_plugin/main.py (4)
SimplePlugin(27-57)GetMetadata(30-37)GetCapabilities(39-42)HandleRequest(44-57)examples/logging_plugin/main.py (4)
GetMetadata(34-41)GetCapabilities(43-46)HandleRequest(48-73)LoggingPlugin(31-94)examples/auth_plugin/main.py (4)
GetMetadata(39-45)GetCapabilities(47-49)HandleRequest(51-70)AuthPlugin(31-83)
🪛 Ruff (0.14.3)
tests/unit/test_server.py
50-50: Unused method argument: mock_signal
(ARG002)
77-77: Unused method argument: mock_signal
(ARG002)
78-78: Unused method argument: mock_add_servicer
(ARG002)
103-103: Unused method argument: mock_signal
(ARG002)
104-104: Unused method argument: mock_add_servicer
(ARG002)
133-133: Unused method argument: mock_signal
(ARG002)
134-134: Unused method argument: mock_add_servicer
(ARG002)
166-166: Unused method argument: mock_signal
(ARG002)
167-167: Unused method argument: mock_add_servicer
(ARG002)
204-204: Unused method argument: mock_signal
(ARG002)
205-205: Unused method argument: mock_add_servicer
(ARG002)
231-231: Unused method argument: mock_signal
(ARG002)
232-232: Unused method argument: mock_add_servicer
(ARG002)
255-255: Unused method argument: mock_signal
(ARG002)
256-256: Unused method argument: mock_add_servicer
(ARG002)
283-283: Unused method argument: mock_signal
(ARG002)
305-305: Unused method argument: mock_add_servicer
(ARG002)
316-316: Unused method argument: mock_add_servicer
(ARG002)
337-337: Unused method argument: mock_add_servicer
(ARG002)
372-372: Unused method argument: mock_add_servicer
(ARG002)
408-408: Unused method argument: mock_signal
(ARG002)
409-409: Unused method argument: mock_add_servicer
(ARG002)
435-435: Unused method argument: mock_signal
(ARG002)
436-436: Unused method argument: mock_add_servicer
(ARG002)
There was a problem hiding this comment.
Thank you so much @peteski22 ! Being this a first commit, I think I could not give a useful set of comments without actually trying it. So let us do the following:
- I am gonna approve this, knowing there might still be a few issues
- I am gonna try all examples and a new use case (the guardrail one we have already talked about)
- I am gonna let you know how it goes and open issues here if necessary.
WDYT?
If you prefer to have extra feedback you can wait for other reviews ofc, otherwise feel free to merge!
Summary by CodeRabbit
New Features
Documentation
Infrastructure
Tests