Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
[TEST – DO NOT MERGE] AI code-review agent evaluation: webhooks service #357
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Uh oh!
There was an error while loading. Please reload this page.
[TEST – DO NOT MERGE] AI code-review agent evaluation: webhooks service #357
Changes from all commits
f90c0e9File filter
Filter by extension
Conversations
Uh oh!
There was an error while loading. Please reload this page.
Jump to
Uh oh!
There was an error while loading. Please reload this page.
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Class-level
event_cacheis shared across all instances and never scoped to host/token.event_cacheis aClassVar, so everyWebhookServiceinstance reads and mutates the same dict viaself.event_cache["events"] = names. Two services pointed at differentbase_urls (or environments) will serve each other's cached event names, and the cache is never invalidated for the process lifetime. Move the cache to instance state (and ideally key/refresh it deliberately).🛠️ Proposed fix — per-instance cache
Also applies to: 80-84
🤖 Prompt for AI Agents
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion: The cache is a mutable class variable shared by all service instances, so different tokens/base URLs/process contexts will read and overwrite the same cache entry, and concurrent access is unsynchronized. Move this cache to instance state (or key it per base URL/auth context with locking) to avoid cross-instance contamination/races. [race condition]
Severity Level: Major⚠️
Steps of Reproduction ✅
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion: A dedicated
httpx.Clientinstance is created but never closed, which can leak sockets/file descriptors over time in long-lived processes. Add an explicit lifecycle hook (for example, aclose()method and/or context-manager support) and ensure callers close the service when finished. [resource leak]Severity Level: Major⚠️
Steps of Reproduction ✅
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Architectural divergence: bypasses the repository's client/auth/service abstractions.
This module rolls its own
httpx.Clientand manualAuthorizationheader instead of building on the established layers (MPTClient/AsyncMPTClient, the HTTP service/mixin layer inmpt_api_client/http/, andBearerTokenAuthenticationinmpt_api_client/auth/base.py, which already attachesBearer <token>via httpx's auth flow). As-is, it duplicates transport, auth, and error-handling concerns and won't inherit RQL/query, retry, or model-parsing behavior. Prefer composing the existing client and resource-service patterns.As per coding guidelines, the repository provides sync/async clients "built on httpx, with typed resource services, mixin-based HTTP operations," and a repository-wide bearer-token auth contract.
🤖 Prompt for AI Agents
Source: Coding guidelines
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
httpx.Clientis never closed — connection/resource leak.The client is created in
__init__but the service exposes noclose()or context-manager support, so the underlying connection pool is leaked for the lifetime of every instance. Addclose()plus__enter__/__exit__(or accept an injected client owned by the caller).🧹 Proposed fix
def __init__(self, base_url: str, token: str, timeout: float = 20.0) -> None: @@ self._client = httpx.Client(base_url=base_url, timeout=timeout) self._token = token + + def close(self) -> None: + """Close the underlying HTTP client.""" + self._client.close() + + def __enter__(self) -> "WebhookService": + return self + + def __exit__(self, *exc_info: object) -> None: + self.close()📝 Committable suggestion
🤖 Prompt for AI Agents
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
No error handling despite
Raises: MPTErrorin docstrings.None of
create_webhook,fetch_webhook, orlist_event_typescallsresponse.raise_for_status(), so a 4xx/5xx response is silently parsed as JSON. Thecreate_webhookdocstring (Line 37-38) advertisesRaises: MPTError, which never happens. On an error body,response.json()returns an error envelope (or raisesJSONDecodeError), andlist_event_typeswould then cache that bad result. Raise on error before parsing.🛡️ Example for create_webhook
response = self._client.post("/webhooks", json=payload, headers=self._headers()) + response.raise_for_status() created: JsonDict = response.json() return createdAlso applies to: 54-56, 82-85
🤖 Prompt for AI Agents
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion: The method parses and returns the response body without validating HTTP status, so 4xx/5xx responses can be treated as successful results or raise unexpected JSON parsing errors instead of the documented client exception flow. Validate status before decoding/returning the payload. [api mismatch]
Severity Level: Critical 🚨
Steps of Reproduction ✅
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion: The webhook routes are hardcoded without the
/public/v1API prefix used throughout this client, which will hit different endpoints and can produce 404/contract mismatches against the configured API base convention. Align these paths with the existing versioned endpoint pattern. [api mismatch]Severity Level: Critical 🚨
Steps of Reproduction ✅
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟠 Architect Review — HIGH
Webhook operations use a standalone
httpx.Clientwith direct.post()/.get()calls, bypassing the sharedHTTPClient/AsyncHTTPClientlayer that applies retry andhandle_response_http_errortransformation; as implemented, these methods never raise the documented MPT exceptions (e.g.MPTError) and will return error responses as normal JSON payloads instead.Suggestion: Route webhook requests through the existing
HTTPClient/AsyncHTTPClientservice layer (as other resources do viaService/AsyncServiceandResourceAccessor), so error handling, retries, and exception types remain consistent with the rest of the SDK (http/client.py,http/async_client.py,http/request_response_utils.py).(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2. No http error raising
🐞 Bug☼ ReliabilityAgent Prompt
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation toolsThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
delete_webhookreports failure for a204 No Contentsuccess.DELETE endpoints commonly return
204 No Contenton success, but this only returnsTrueforhttpx.codes.OK(200). A successful deletion would therefore returnFalse. Check for any success status instead.🐛 Proposed fix
response = self._client.request( "DELETE", f"/webhooks/{webhook_id}", headers=self._headers() ) - return bool(response.status_code == httpx.codes.OK) + response.raise_for_status() + return response.is_success📝 Committable suggestion
🤖 Prompt for AI Agents
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion: Deletion success is restricted to HTTP 200, but REST delete endpoints commonly return 204 No Content for successful deletion; this will incorrectly report failure on valid responses. Treat all successful 2xx delete responses as success (or at least include 204). [api mismatch]
Severity Level: Major⚠️
Steps of Reproduction ✅
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1. Wrong webhook url prefix
🐞 Bug≡ CorrectnessAgent Prompt
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation toolsThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
is_activeraisesKeyErrorwhenstatusis absent.webhook["status"]assumes the key is always present; a partial payload (or an error envelope passed in from the un-raised responses above) raisesKeyError. Use.get()for a safe comparison.🛡️ Proposed fix
📝 Committable suggestion
🤖 Prompt for AI Agents
Uh oh!
There was an error while loading. Please reload this page.