Python SDK: Use HatchetConfigurationError for invalid token validation#3017
Python SDK: Use HatchetConfigurationError for invalid token validation#3017TheEleventhAvatar wants to merge 11 commits intohatchet-dev:mainfrom
Conversation
Replace generic ValueError exceptions in ClientConfig token validation with HatchetConfigurationError to better reflect configuration failures. This improves clarity and aligns token validation with the SDK’s existing exception hierarchy without changing runtime behavior.
|
@TheEleventhAvatar is attempting to deploy a commit to the Hatchet Team on Vercel. A member of the Team first needs to authorize it. |
There was a problem hiding this comment.
Pull request overview
This PR aims to improve the Python SDK’s configuration error clarity by switching ClientConfig token-validation failures from generic ValueError to a domain-specific HatchetConfigurationError.
Changes:
- Replace
ValueErrorwithHatchetConfigurationErrorfor missing/invalid token validation inClientConfig. - Update token validation error messages to be more configuration-focused (e.g., referencing
HATCHET_CLIENT_TOKEN). - Remove a few explanatory inline comments in unrelated validators.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @model_validator(mode="after") | ||
| def validate_token_and_tenant(self) -> "ClientConfig": | ||
| if not self.token: | ||
| raise ValueError("Token must be set") | ||
| raise HatchetConfigurationError( | ||
| "HATCHET_CLIENT_TOKEN must be set. " | ||
| "Provide it via environment variable or pass token=... to ClientConfig." | ||
| ) | ||
|
|
||
| if not self.token.startswith("ey"): | ||
| raise ValueError( | ||
| f"Token must be a valid JWT. Hint: These are the first few characters of the token provided: {self.token[:5]}" | ||
| raise HatchetConfigurationError( | ||
| "HATCHET_CLIENT_TOKEN must be a valid JWT. " | ||
| f"Received token starting with: '{self.token[:5]}...'" | ||
| ) |
There was a problem hiding this comment.
In Pydantic v2 validators, raising a non-ValueError/TypeError/AssertionError exception typically bypasses normal ValidationError aggregation and will change what callers see when ClientConfig() is constructed. If the intent is to keep Pydantic-style validation errors, make HatchetConfigurationError inherit from ValueError (or use PydanticCustomError); if the intent is to surface a domain-specific exception to users, consider catching ValidationError at the ClientConfig()/Hatchet() construction boundary and rethrowing HatchetConfigurationError there so behavior is explicit and consistent.
| if not self.token: | ||
| raise ValueError("Token must be set") | ||
| raise HatchetConfigurationError( | ||
| "HATCHET_CLIENT_TOKEN must be set. " | ||
| "Provide it via environment variable or pass token=... to ClientConfig." | ||
| ) | ||
|
|
||
| if not self.token.startswith("ey"): | ||
| raise ValueError( | ||
| f"Token must be a valid JWT. Hint: These are the first few characters of the token provided: {self.token[:5]}" | ||
| raise HatchetConfigurationError( | ||
| "HATCHET_CLIENT_TOKEN must be a valid JWT. " | ||
| f"Received token starting with: '{self.token[:5]}...'" | ||
| ) |
There was a problem hiding this comment.
Token validation now emits a different exception type/message, but there are no unit tests covering the missing-token and invalid-JWT paths. Add coverage in sdks/python/tests/test_client.py asserting the raised exception type (and key parts of the message) for ClientConfig(token="") and ClientConfig(token="not-a-jwt") so regressions are caught.
| @field_validator("event_loop_block_threshold_seconds", mode="before") | ||
| @classmethod | ||
| def validate_event_loop_block_threshold_seconds( | ||
| cls, value: timedelta | int | float | str | ||
| ) -> timedelta: | ||
| # Settings env vars are strings; interpret as seconds. | ||
| if isinstance(value, timedelta): | ||
| return value | ||
|
|
||
| if isinstance(value, int | float): | ||
| return timedelta(seconds=float(value)) | ||
|
|
||
| v = value.strip() | ||
| # Allow a small convenience suffix, but keep "seconds" as the contract. | ||
| if v.endswith("s"): | ||
| v = v[:-1].strip() |
There was a problem hiding this comment.
PR description says the change is scoped to token validation only, but this diff also removes explanatory comments in the healthcheck threshold validator. Either keep the scope strictly to token validation (revert these comment-only edits) or update the PR description to reflect the additional changes.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
| @model_validator(mode="after") | ||
| def validate_addresses(self) -> "ClientConfig": | ||
| ## If nothing is set, read from the token | ||
| ## If either is set, override what's in the JWT | ||
| server_url_from_jwt, grpc_broadcast_address_from_jwt = get_addresses_from_jwt( | ||
| self.token | ||
| ) |
There was a problem hiding this comment.
PR description notes the change is intentionally scoped to token validation only, but this diff also removes comments describing how validate_addresses derives overrides from the JWT. Either revert the comment-only edits here or adjust the PR description/scope accordingly.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
mrkaye97
left a comment
There was a problem hiding this comment.
AFAICT, HatchetConfigurationError is never actually defined?
Good catch — thanks! I’ve now defined HatchetError and HatchetConfigurationError in hatchet_sdk/exceptions.py and updated the import accordingly. |
Maybe I'm missing it - I don't see that change |
now it is there! |
| class HatchetError(Exception): | ||
| """Base exception for Hatchet Python SDK.""" | ||
|
|
||
|
|
||
| class HatchetConfigurationError(HatchetError): | ||
| """Raised when required configuration is missing or invalid.""" |
There was a problem hiding this comment.
so unfortunately, as this is defined it's a breaking change. If we want to replace instances of ValueError with these exceptions, they need to inherit from ValueError
There was a problem hiding this comment.
Good point — thanks for calling that out. I’ve updated HatchetConfigurationError to inherit from ValueError to avoid introducing a breaking change.
| def validate_event_loop_block_threshold_seconds( | ||
| cls, value: timedelta | int | float | str | ||
| ) -> timedelta: | ||
| # Settings env vars are strings; interpret as seconds. |
| @model_validator(mode="after") | ||
| def validate_addresses(self) -> "ClientConfig": | ||
| ## If nothing is set, read from the token | ||
| ## If either is set, override what's in the JWT |
There was a problem hiding this comment.
let's leave these comments
|
Thanks for the contribution! Left a couple small notes - we also need to bump the patch version of the package and add a new changelog entry |
there is no version file,do you handle it via tags or CI/CD?i did changelog entry |
it's in the |
updated pyproject and changelog |
35b4524 to
7075531
Compare
|
@mrkaye97 formatted only the exceptions file,.lint check now passes |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if not self.token: | ||
| raise ValueError("Token must be set") | ||
| raise HatchetConfigurationError( | ||
| "HATCHET_CLIENT_TOKEN must be set. " | ||
| "Provide it via environment variable or pass token=... to ClientConfig." | ||
| ) |
There was a problem hiding this comment.
Token validation behavior changed (exception type/message). There are existing tests covering ClientConfig construction, but none assert the error behavior for missing/invalid tokens. Add unit tests that verify missing token and malformed token cases raise the expected configuration error (and, if required for backwards-compat, that it remains a ValueError subclass).
| @field_validator("event_loop_block_threshold_seconds", mode="before") | ||
| @classmethod | ||
| def validate_event_loop_block_threshold_seconds( | ||
| cls, value: timedelta | int | float | str | ||
| ) -> timedelta: | ||
| # Settings env vars are strings; interpret as seconds. | ||
| if isinstance(value, timedelta): |
There was a problem hiding this comment.
This hunk contains comment-only edits unrelated to token validation (the PR description says the change is intentionally scoped to token validation only). Either revert these comment-only changes here or update the PR description/scope to reflect the additional edits.
| #### Changed | ||
| - Python SDK `ClientConfig` token validation now raises `HatchetConfigurationError` instead of `ValueError` for invalid or missing configuration. | ||
| - Minor field validators updated to improve clarity and robustness. | ||
| - Updated package patch version to include these non-breaking improvements. | ||
|
|
There was a problem hiding this comment.
The changelog entry appears to (1) change the heading level from ### Changed to #### Changed (inconsistent with other entries) and (2) replace prior 1.23.4 bullets with new ones that mention changes (e.g., patch version bump / validator updates) that aren’t present in this PR. Please keep the changelog formatting consistent and ensure the 1.23.4 notes accurately reflect the actual code changes in this PR.
| #### Changed | |
| - Python SDK `ClientConfig` token validation now raises `HatchetConfigurationError` instead of `ValueError` for invalid or missing configuration. | |
| - Minor field validators updated to improve clarity and robustness. | |
| - Updated package patch version to include these non-breaking improvements. | |
| ### Changed | |
| - Documentation-only updates to the changelog; no functional changes to the Python SDK. |
|
|
||
|
|
There was a problem hiding this comment.
HatchetConfigurationError is introduced as a new public exception type, but it isn't re-exported from hatchet_sdk.__init__ (unlike the other exceptions). Consider adding it to the package exports (__init__.py imports / __all__) so users can reliably import it from hatchet_sdk like the rest of the SDK exceptions.
| __all__ = [ | |
| "HatchetConfigurationError", | |
| "InvalidDependencyError", | |
| "NonRetryableException", | |
| "DedupeViolationError", | |
| "TASK_RUN_ERROR_METADATA_KEY", | |
| "TaskRunError", | |
| "FailedTaskRunExceptionGroup", | |
| "LoopAlreadyRunningError", | |
| "IllegalTaskOutputError", | |
| "LifespanSetupError", | |
| ] |
Issue #2879
Replace generic
ValueErrorexceptions inClientConfigtoken validationwith
HatchetConfigurationErrorto better reflect configuration failures.This improves clarity and aligns token validation with the SDK’s existing
exception hierarchy without changing runtime behavior.
Type of change
What's Changed
ValueErrorin token validation withHatchetConfigurationErrorNotes
This change is intentionally scoped to token validation only and does not
modify other error handling paths.
Motivation
Using a domain-specific exception makes it easier for users of the SDK
to distinguish configuration errors from generic runtime failures.