feat(sdk): structured error responses with typed error codes#27
Open
MDzaja wants to merge 1 commit into
Open
Conversation
Signed-off-by: MDzaja <mirkodzaja0@gmail.com>
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
27 issues found across 152 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="sdk-python/tests/test_common.py">
<violation number="1" location="sdk-python/tests/test_common.py:233">
P2: Removes coverage of the `error_code` backwards-compat fallback that still exists in the parser (filesystem.py:168-169). The old snake_case test payload used `error_code`, which exercised the retro-compat path for older runner builds. Without a dedicated test, this code is now dead-code-grey: it will silently bit-rot without detection.</violation>
</file>
<file name="toolbox-api-client-java/src/main/java/io/daytona/toolbox/client/api/FileSystemApi.java">
<violation number="1" location="toolbox-api-client-java/src/main/java/io/daytona/toolbox/client/api/FileSystemApi.java:30">
P3: Unused `ErrorResponse` import adds dead code noise. Remove it to keep generated client clean and avoid lint/checkstyle failures.</violation>
</file>
<file name="sdk-typescript/src/utils/FileTransfer.ts">
<violation number="1" location="sdk-typescript/src/utils/FileTransfer.ts:37">
P2: `error_code` fallback was removed, so structured code parsing regresses for snake_case responses. This can downgrade typed download errors to generic status-only errors.</violation>
</file>
<file name="sdk-java/src/main/java/io/daytona/sdk/exception/DaytonaException.java">
<violation number="1" location="sdk-java/src/main/java/io/daytona/sdk/exception/DaytonaException.java:47">
P2: Headers are wrapped, not copied, so external map mutations can alter exception headers after creation. Snapshot the map during construction.</violation>
</file>
<file name="toolbox-api-client-java/src/main/java/io/daytona/toolbox/client/model/ErrorResponse.java">
<violation number="1" location="toolbox-api-client-java/src/main/java/io/daytona/toolbox/client/model/ErrorResponse.java:343">
P2: Validation assumes object input and can throw unchecked exceptions for non-object JSON. Add an explicit isJsonObject guard before required-field access.</violation>
<violation number="2" location="toolbox-api-client-java/src/main/java/io/daytona/toolbox/client/model/ErrorResponse.java:403">
P2: Additional-property serialization crashes for non-object values (e.g., enum or null). Serialize the JsonElement directly instead of forcing object casting.</violation>
</file>
<file name="sdk-java/src/main/java/io/daytona/sdk/ExceptionMapper.java">
<violation number="1" location="sdk-java/src/main/java/io/daytona/sdk/ExceptionMapper.java:148">
P2: Status/code mapped exceptions discard response headers. Callers of `getHeaders()` on common mapped errors (e.g., 429) will not receive server metadata like retry hints.</violation>
<violation number="2" location="sdk-java/src/main/java/io/daytona/sdk/ExceptionMapper.java:169">
P2: Transport timeouts no longer map to DaytonaTimeoutException, which is a breaking behavioral change for existing timeout catch blocks.</violation>
</file>
<file name="toolbox-api-client-python-async/daytona_toolbox_api_client_async/api/git_api.py">
<violation number="1" location="toolbox-api-client-python-async/daytona_toolbox_api_client_async/api/git_api.py:106">
P3: Added error mappings in `without_preload_content` methods are unused dead code. These methods return raw response without deserializing, so the new `_response_types_map` entries have no effect.</violation>
</file>
<file name="sdk-java/src/main/java/io/daytona/sdk/exception/DaytonaTimeoutException.java">
<violation number="1" location="sdk-java/src/main/java/io/daytona/sdk/exception/DaytonaTimeoutException.java:12">
P3: New field comment is incorrect: not every DaytonaTimeoutException instance carries HTTP 408.</violation>
<violation number="2" location="sdk-java/src/main/java/io/daytona/sdk/exception/DaytonaTimeoutException.java:39">
P1: Timeout constructor now hardcodes 408, so mapped HTTP 504 errors are reported with the wrong status code.</violation>
</file>
<file name="sdk-ruby/lib/daytona/git.rb">
<violation number="1" location="sdk-ruby/lib/daytona/git.rb:321">
P2: This loses ServerError classification for unlisted 5xx Git API responses. A 501/505/520 toolbox response now raises base Daytona::Sdk::Error, so callers rescuing ServerError miss server-side failures.</violation>
</file>
<file name="sdk-ruby/lib/daytona/file_system.rb">
<violation number="1" location="sdk-ruby/lib/daytona/file_system.rb:154">
P2: `download_file` no longer wraps local filesystem errors. Failures writing `local_path` now escape as raw `Errno`/`IOError` instead of SDK errors.</violation>
<violation number="2" location="sdk-ruby/lib/daytona/file_system.rb:238">
P2: `upload_file` now misses non-API upload failures from local IO handling. This breaks consistent SDK error typing for upload failures before the API call completes.</violation>
</file>
<file name="toolbox-api-client-ruby/lib/daytona_toolbox_api_client/models/error_response.rb">
<violation number="1" location="toolbox-api-client-ruby/lib/daytona_toolbox_api_client/models/error_response.rb:23">
P2: `method` accessor shadows `Object#method`, breaking standard Ruby reflection on `ErrorResponse` instances. Use a non-reserved Ruby attribute name (e.g. `http_method`) mapped to JSON key `method`.</violation>
</file>
<file name="sdk-python/src/daytona/_utils/errors.py">
<violation number="1" location="sdk-python/src/daytona/_utils/errors.py:81">
P2: OpenAPI error parsing dropped backward-compatible `error_code`/`error` keys. Older structured responses will no longer populate `DaytonaError.code` or code-based subclass mapping.</violation>
</file>
<file name="toolbox-api-client-python/daytona_toolbox_api_client/api/port_api.py">
<violation number="1" location="toolbox-api-client-python/daytona_toolbox_api_client/api/port_api.py:341">
P3: This added 400 mapping is dead code in without_preload_content; that variant returns the raw response and never deserializes with _response_types_map.</violation>
</file>
<file name="toolbox-api-client-go/api_lsp.go">
<violation number="1" location="toolbox-api-client-go/api_lsp.go:225">
P3: Inconsistent indentation in generated code: `formatErrorMessage`/`newErr.model` lines use 5 tabs instead of 3. Since this is generated code, the OpenAPI generator template should be fixed upstream.</violation>
</file>
<file name="sdk-python/src/daytona/common/errors.py">
<violation number="1" location="sdk-python/src/daytona/common/errors.py:159">
P2: `DaytonaBadRequestError` docstring claims it's for "HTTP 400" but the deprecated alias `DaytonaValidationError` is used for client-side validation that never involves an HTTP request. The class docstring and example are misleading for the aliased usage. Consider either keeping a separate `DaytonaValidationError` class for client-side errors or updating the docstring to cover both use-cases.</violation>
<violation number="2" location="sdk-python/src/daytona/common/errors.py:195">
P2: `DaytonaConnectionTimeoutError` inherits only from `DaytonaConnectionError`, not `DaytonaTimeoutError`. Users writing `except DaytonaTimeoutError` to catch all timeouts will miss transport-level timeouts. Consider multiple inheritance (`class DaytonaConnectionTimeoutError(DaytonaConnectionError, DaytonaTimeoutError)`) for consistency with `DaytonaProcessExecutionTimeoutError`.</violation>
</file>
<file name="sdk-go/pkg/errors/errors.go">
<violation number="1" location="sdk-go/pkg/errors/errors.go:102">
P3: Doc comment references nonexistent `SourceSDK` symbol, which is misleading for users.</violation>
</file>
<file name="sdk-ruby/lib/daytona/sdk/file_download_patch.rb">
<violation number="1" location="sdk-ruby/lib/daytona/sdk/file_download_patch.rb:81">
P1: Buffered file-download error data is stored in a shared instance variable, which is race-prone for concurrent requests and can attach the wrong error body to raised ApiError.</violation>
<violation number="2" location="sdk-ruby/lib/daytona/sdk/file_download_patch.rb:86">
P2: No guard against double-patching: calling `apply!` twice on the same class creates infinite recursion via `alias_method` re-aliasing the patched method.</violation>
</file>
<file name="sdk-typescript/src/errors/DaytonaError.ts">
<violation number="1" location="sdk-typescript/src/errors/DaytonaError.ts:66">
P2: `new DaytonaValidationError(msg).name` now returns `'DaytonaBadRequestError'` instead of `'DaytonaValidationError'` because the alias re-exports the class directly and `new.target.name` resolves to the actual constructor name. This is a subtle behavioral break for code relying on `error.name`.</violation>
</file>
<file name="sdk-ruby/lib/daytona/sdk/errors.rb">
<violation number="1" location="sdk-ruby/lib/daytona/sdk/errors.rb:271">
P3: Internal helper methods `parse_error_body`, `parsed_message`, `error_class_for`, and `string_or_nil` are publicly exposed on the `Daytona::Sdk` module but are implementation details. Mark them with `private_class_method` to avoid polluting the public API.</violation>
</file>
<file name="toolbox-api-client-java/src/main/java/io/daytona/toolbox/client/api/ProcessApi.java">
<violation number="1" location="toolbox-api-client-java/src/main/java/io/daytona/toolbox/client/api/ProcessApi.java:34">
P2: Unused `ErrorResponse` import introduces dead code and indicates typed error handling is not wired in this API class.</violation>
</file>
<file name="sdk-typescript/src/Daytona.ts">
<violation number="1" location="sdk-typescript/src/Daytona.ts:649">
P2: Timeout rethrow drops `source` metadata, losing structured error origin information.</violation>
</file>
Tip: instead of fixing issues one by one fix them all with cubic
Re-trigger cubic
| } | ||
|
|
||
| public DaytonaTimeoutException(String message, Throwable cause, String code, String source) { | ||
| super(STATUS_CODE, message, cause, code, source); |
There was a problem hiding this comment.
P1: Timeout constructor now hardcodes 408, so mapped HTTP 504 errors are reported with the wrong status code.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At sdk-java/src/main/java/io/daytona/sdk/exception/DaytonaTimeoutException.java, line 39:
<comment>Timeout constructor now hardcodes 408, so mapped HTTP 504 errors are reported with the wrong status code.</comment>
<file context>
@@ -27,4 +30,12 @@ public DaytonaTimeoutException(String message, Throwable cause) {
+ }
+
+ public DaytonaTimeoutException(String message, Throwable cause, String code, String source) {
+ super(STATUS_CODE, message, cause, code, source);
+ }
}
</file context>
| ) | ||
| block&.call(tempfile) | ||
| else | ||
| @_daytona_error_body = error_body unless error_body.empty? |
There was a problem hiding this comment.
P1: Buffered file-download error data is stored in a shared instance variable, which is race-prone for concurrent requests and can attach the wrong error body to raised ApiError.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At sdk-ruby/lib/daytona/sdk/file_download_patch.rb, line 81:
<comment>Buffered file-download error data is stored in a shared instance variable, which is race-prone for concurrent requests and can attach the wrong error body to raised ApiError.</comment>
<file context>
@@ -0,0 +1,122 @@
+ )
+ block&.call(tempfile)
+ else
+ @_daytona_error_body = error_body unless error_body.empty?
+ end
+ end
</file context>
| # variant — payload uses the canonical key. | ||
| message, details = parse_file_download_error_payload( | ||
| b'{"message":"missing","status_code":404,"error_code":"NOT_FOUND"}', | ||
| b'{"message":"missing","status_code":404,"code":"NOT_FOUND"}', |
There was a problem hiding this comment.
P2: Removes coverage of the error_code backwards-compat fallback that still exists in the parser (filesystem.py:168-169). The old snake_case test payload used error_code, which exercised the retro-compat path for older runner builds. Without a dedicated test, this code is now dead-code-grey: it will silently bit-rot without detection.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At sdk-python/tests/test_common.py, line 233:
<comment>Removes coverage of the `error_code` backwards-compat fallback that still exists in the parser (filesystem.py:168-169). The old snake_case test payload used `error_code`, which exercised the retro-compat path for older runner builds. Without a dedicated test, this code is now dead-code-grey: it will silently bit-rot without detection.</comment>
<file context>
@@ -226,19 +226,22 @@ def test_create_file_download_error_requires_error_message(self):
+ # variant — payload uses the canonical key.
message, details = parse_file_download_error_payload(
- b'{"message":"missing","status_code":404,"error_code":"NOT_FOUND"}',
+ b'{"message":"missing","status_code":404,"code":"NOT_FOUND"}',
"application/json",
)
</file context>
| const structuredMessage = payloadObject.message | ||
| const statusCode = payloadObject.statusCode ?? payloadObject.status_code | ||
| const errorCode = payloadObject.code ?? payloadObject.error_code | ||
| const code = payloadObject.code |
There was a problem hiding this comment.
P2: error_code fallback was removed, so structured code parsing regresses for snake_case responses. This can downgrade typed download errors to generic status-only errors.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At sdk-typescript/src/utils/FileTransfer.ts, line 37:
<comment>`error_code` fallback was removed, so structured code parsing regresses for snake_case responses. This can downgrade typed download errors to generic status-only errors.</comment>
<file context>
@@ -34,7 +34,8 @@ function parseDownloadErrorPart(data: Uint8Array, contentType?: string): Downloa
const structuredMessage = payloadObject.message
const statusCode = payloadObject.statusCode ?? payloadObject.status_code
- const errorCode = payloadObject.code ?? payloadObject.error_code
+ const code = payloadObject.code
+ const source = payloadObject.source
</file context>
Suggested change
| const code = payloadObject.code | |
| const code = payloadObject.code ?? payloadObject.error_code |
| Throwable cause) { | ||
| super(message, cause); | ||
| this.statusCode = statusCode; | ||
| this.headers = headers != null ? Collections.unmodifiableMap(headers) : Collections.emptyMap(); |
There was a problem hiding this comment.
P2: Headers are wrapped, not copied, so external map mutations can alter exception headers after creation. Snapshot the map during construction.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At sdk-java/src/main/java/io/daytona/sdk/exception/DaytonaException.java, line 47:
<comment>Headers are wrapped, not copied, so external map mutations can alter exception headers after creation. Snapshot the map during construction.</comment>
<file context>
@@ -25,18 +25,37 @@
+ Throwable cause) {
+ super(message, cause);
+ this.statusCode = statusCode;
+ this.headers = headers != null ? Collections.unmodifiableMap(headers) : Collections.emptyMap();
+ this.code = code;
+ this.source = source;
</file context>
Suggested change
| this.headers = headers != null ? Collections.unmodifiableMap(headers) : Collections.emptyMap(); | |
| this.headers = headers != null ? Collections.unmodifiableMap(new java.util.HashMap<>(headers)) : Collections.emptyMap(); |
| * <p>This exception is generated client-side and is not tied to a single HTTP status code. | ||
| */ | ||
| public class DaytonaTimeoutException extends DaytonaException { | ||
| /** HTTP status code carried by every instance of this class. */ |
There was a problem hiding this comment.
P3: New field comment is incorrect: not every DaytonaTimeoutException instance carries HTTP 408.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At sdk-java/src/main/java/io/daytona/sdk/exception/DaytonaTimeoutException.java, line 12:
<comment>New field comment is incorrect: not every DaytonaTimeoutException instance carries HTTP 408.</comment>
<file context>
@@ -9,6 +9,9 @@
* <p>This exception is generated client-side and is not tied to a single HTTP status code.
*/
public class DaytonaTimeoutException extends DaytonaException {
+ /** HTTP status code carried by every instance of this class. */
+ public static final int STATUS_CODE = 408;
+
</file context>
Suggested change
| /** HTTP status code carried by every instance of this class. */ | |
| /** Default HTTP status code for server-reported timeout responses. */ |
| @@ -338,6 +338,7 @@ def is_port_in_use( | |||
|
|
|||
| _response_types_map: Dict[str, Optional[str]] = { | |||
| '200': "IsPortInUseResponse", | |||
| '400': "ErrorResponse", | |||
There was a problem hiding this comment.
P3: This added 400 mapping is dead code in without_preload_content; that variant returns the raw response and never deserializes with _response_types_map.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At toolbox-api-client-python/daytona_toolbox_api_client/api/port_api.py, line 341:
<comment>This added 400 mapping is dead code in without_preload_content; that variant returns the raw response and never deserializes with _response_types_map.</comment>
<file context>
@@ -338,6 +338,7 @@ def is_port_in_use(
_response_types_map: Dict[str, Optional[str]] = {
'200': "IsPortInUseResponse",
+ '400': "ErrorResponse",
}
response_data = self.api_client.call_api(
</file context>
| @@ -215,6 +215,16 @@ func (a *LspAPIService) CompletionsExecute(r LspAPICompletionsRequest) (*Complet | |||
| body: localVarBody, | |||
There was a problem hiding this comment.
P3: Inconsistent indentation in generated code: formatErrorMessage/newErr.model lines use 5 tabs instead of 3. Since this is generated code, the OpenAPI generator template should be fixed upstream.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At toolbox-api-client-go/api_lsp.go, line 225:
<comment>Inconsistent indentation in generated code: `formatErrorMessage`/`newErr.model` lines use 5 tabs instead of 3. Since this is generated code, the OpenAPI generator template should be fixed upstream.</comment>
<file context>
@@ -215,6 +215,16 @@ func (a *LspAPIService) CompletionsExecute(r LspAPICompletionsRequest) (*Complet
+ newErr.error = err.Error()
+ return localVarReturnValue, localVarHTTPResponse, newErr
+ }
+ newErr.error = formatErrorMessage(localVarHTTPResponse.Status, &v)
+ newErr.model = v
+ }
</file context>
| type DaytonaAuthenticationError struct { | ||
| *DaytonaError | ||
| // NewDaytonaError builds a DaytonaError with the given message, status code | ||
| // and headers. `Source` is left empty — set it explicitly via `SourceSDK` |
There was a problem hiding this comment.
P3: Doc comment references nonexistent SourceSDK symbol, which is misleading for users.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At sdk-go/pkg/errors/errors.go, line 102:
<comment>Doc comment references nonexistent `SourceSDK` symbol, which is misleading for users.</comment>
<file context>
@@ -26,294 +72,195 @@ func (e *DaytonaError) Error() string {
-type DaytonaAuthenticationError struct {
- *DaytonaError
+// NewDaytonaError builds a DaytonaError with the given message, status code
+// and headers. `Source` is left empty — set it explicitly via `SourceSDK`
+// for SDK-internal errors, or via the translation layer for server-side
+// errors. Most callers should use this directly; the sentinels below are
</file context>
| parse_error_body(error.response_body)[:message] | ||
| end | ||
|
|
||
| def self.string_or_nil(value) |
There was a problem hiding this comment.
P3: Internal helper methods parse_error_body, parsed_message, error_class_for, and string_or_nil are publicly exposed on the Daytona::Sdk module but are implementation details. Mark them with private_class_method to avoid polluting the public API.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At sdk-ruby/lib/daytona/sdk/errors.rb, line 271:
<comment>Internal helper methods `parse_error_body`, `parsed_message`, `error_class_for`, and `string_or_nil` are publicly exposed on the `Daytona::Sdk` module but are implementation details. Mark them with `private_class_method` to avoid polluting the public API.</comment>
<file context>
@@ -0,0 +1,275 @@
+ parse_error_body(error.response_body)[:message]
+ end
+
+ def self.string_or_nil(value)
+ value.is_a?(String) && !value.empty? ? value : nil
+ end
</file context>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Original: daytonaio/daytona#4849
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Summary by cubic
Adds structured error envelopes and typed error codes across all SDKs and toolbox clients. Errors now include a machine-readable
codeandsource, and map to consistent, language-idiomatic classes.New Features
ErrorResponseandDaemonErrorCode; all error responses reference it.toolbox-api-client-go/toolbox-api-client-java: decode structured errors and expose them via generatedApiError.sdk-go: singleDaytonaErrorwithcode,source, headers, and status; package sentinels likesdkerrors.ErrNotFound;Client.handleAPIErrordelegates to structured conversion.sdk-java:DaytonaExceptioncarriesstatusCode,code,source; many specific subclasses (e.g.,DaytonaGitAuthFailedException,DaytonaServiceUnavailableException); 422 is nowDaytonaUnprocessableEntityException;ExceptionMapperupdated.sdk-python: errors exposestatus_code,code,source; richer mapping for connection/timeout; filesystem download errors parse envelope; tests updated.sdk-ruby: newDaytona::Sdkerror hierarchy withcode/source; wraps OpenAPIApiError; file-download patch preserves error bodies for streaming; callers now raise consistent SDK errors.sdk-typescript:DaytonaErrorgainscode/source; more subclasses (e.g.,DaytonaInternalServerError,DaytonaServiceUnavailableError); file download error details usecode/source; axios mapping improved; tests updated.Migration
error.errorCodewitherror.code(TypeScript, Python). For file downloads, useerrorDetails.codeanderrorDetails.source.DaytonaUnprocessableEntityExceptioninstead ofDaytonaValidationExceptionfor 422.errors.Is(err, sdkerrors.ErrNotFound)and other sentinels over struct type assertions.Daytona::Sdk::Errorsubclasses; SDK methods now wrap toolbox/main API errors into these types.Written for commit c72c6e1. Summary will update on new commits.