Conversation
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
Codecov Report❌ Patch coverage is
@@ Coverage Diff @@
## main #70 +/- ##
==========================================
- Coverage 96.38% 94.98% -1.40%
==========================================
Files 13 14 +1
Lines 1301 1357 +56
Branches 191 197 +6
==========================================
+ Hits 1254 1289 +35
- Misses 47 68 +21
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Adds initial support in the SDK for retrieving a downloadable artifact reference (via a new downloadArtifact method) and updates the test/mocking utilities to support the new endpoint and updated response shapes.
Changes:
- Added
downloadArtifact(runId, artifactId)toPlatformSDK/PlatformSDKHttp. - Extended MSW HTTP mocks to handle
GET /v1/runs/:runId/artifacts/:artifactId/fileacross scenarios. - Updated tests and item factories/specs to include the newly-required
input_artifactsfield.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| packages/sdk/src/test-utils/http-mocks.ts | Adds mock handlers for the artifact file URL endpoint; updates item result factory shape (input_artifacts). |
| packages/sdk/src/platform-sdk.ts | Introduces the new SDK method and interface entry for artifact download/URL retrieval. |
| packages/sdk/src/platform-sdk.test.ts | Adds tests for the new artifact method (success, not found, no token). |
| packages/sdk/src/entities/run-item/process-run-item.spec.ts | Updates test fixture to include input_artifacts to match the updated API type shape. |
| applicationId: string, | ||
| version: string | ||
| ): Promise<VersionReadResponse>; | ||
| downloadArtifact(runId: string, artifactId: string): Promise<unknown>; | ||
| } |
There was a problem hiding this comment.
downloadArtifact is typed as Promise<unknown>, which leaks no usable type information to SDK consumers and makes it unclear what the method returns. Since the endpoint handler returns an object with a url, this should return a concrete type (ideally the generated response type, or at least { url: string }).
packages/sdk/src/platform-sdk.ts
Outdated
| async downloadArtifact(runId: string, artifactId: string): Promise<unknown> { | ||
| const client = await this.#getClient(); | ||
| try { | ||
| const response = await client.getArtifactUrlV1RunsRunIdArtifactsArtifactIdFileGet({ | ||
| runId, | ||
| artifactId, | ||
| }); | ||
| return response; |
There was a problem hiding this comment.
The method name downloadArtifact suggests it downloads the artifact bytes, but the implementation only calls the API that returns a presigned URL and then returns the full client response. Consider either (a) renaming this to something like getArtifactDownloadUrl/getArtifactUrl and returning response.data (or the url string), or (b) actually performing the download and returning the file content/stream. Also, returning the raw response is inconsistent with the other SDK methods here, which return response.data.
| async downloadArtifact(runId: string, artifactId: string): Promise<unknown> { | |
| const client = await this.#getClient(); | |
| try { | |
| const response = await client.getArtifactUrlV1RunsRunIdArtifactsArtifactIdFileGet({ | |
| runId, | |
| artifactId, | |
| }); | |
| return response; | |
| async downloadArtifact(runId: string, artifactId: string): Promise<Uint8Array> { | |
| const client = await this.#getClient(); | |
| try { | |
| const response = await client.getArtifactUrlV1RunsRunIdArtifactsArtifactIdFileGet({ | |
| runId, | |
| artifactId, | |
| }); | |
| const artifactUrl = response.data?.url; | |
| if (typeof artifactUrl !== 'string' || artifactUrl.length === 0) { | |
| throw new UnexpectedError('Artifact download URL is missing from the API response'); | |
| } | |
| const downloadResponse = await fetch(artifactUrl); | |
| if (!downloadResponse.ok) { | |
| throw new UnexpectedError( | |
| `Artifact download failed with status ${downloadResponse.status} ${downloadResponse.statusText}` | |
| ); | |
| } | |
| return new Uint8Array(await downloadResponse.arrayBuffer()); |
packages/sdk/src/platform-sdk.ts
Outdated
|
|
||
| async downloadArtifact(runId: string, artifactId: string): Promise<unknown> { | ||
| const client = await this.#getClient(); | ||
| try { | ||
| const response = await client.getArtifactUrlV1RunsRunIdArtifactsArtifactIdFileGet({ | ||
| runId, |
There was a problem hiding this comment.
This new public SDK method is missing the JSDoc block that the other public methods in PlatformSDKHttp have, which makes the public API less discoverable (params, return shape, and whether it returns a URL vs content). Please add documentation consistent with the surrounding methods.
| setMockScenario('success'); | ||
|
|
||
| const result = await sdk.downloadArtifact('test-run-id', 'test-artifact-id'); | ||
| expect(result).toBeDefined(); |
There was a problem hiding this comment.
The success test only asserts that the result is defined, so it wouldn’t catch regressions like returning the wrong shape (e.g., Axios response vs { url }) or an empty URL. Since the mock returns a deterministic URL, assert on the returned value/shape (e.g., that the URL matches the expected presigned URL).
| expect(result).toBeDefined(); | |
| expect(result).toEqual({ | |
| url: 'https://example.com/presigned-download-url', | |
| }); |
na66im
force-pushed
the
task/tssdk-53-download-artifacts-method
branch
from
ee41883 to
817fc31
Compare
na66im
force-pushed
the
task/tssdk-53-download-artifacts-method
branch
from
817fc31 to
b33b780
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 8 changed files in this pull request and generated 4 comments.
Comments suppressed due to low confidence (1)
packages/sdk/package.json:53
- The newly added
p-retrydependency resolves to v8.0.0, which (perpackage-lock.json) requires Node >=22. This repo pins Node 20.18.0 in.node-versionand declares engines >=18, sonpm installwill fail on the supported Node versions. Pinp-retryto a major that supports Node 18/20, or bump the repo's Node/engines consistently if Node 22 is intended.
"dependencies": {
"axios": "^1.12.2",
"p-retry": "^8.0.0",
"zod": "^4.0.17"
},
"engines": {
"node": ">=18.0.0"
}
| import { AxiosResponse } from 'axios'; | ||
| import pRetry, { AbortError } from 'p-retry'; | ||
|
|
||
| const run = (callbackFn: () => Promise<AxiosResponse>, abortStatuses: number[]) => async () => { | ||
| const response = await callbackFn(); | ||
|
|
||
| // Abort retrying if the resource doesn't exist | ||
| if (abortStatuses.includes(response.status)) { | ||
| throw new AbortError(response.statusText); | ||
| } | ||
|
|
||
| return response; | ||
| }; | ||
|
|
||
| export const downloadWithRetry = async ( | ||
| callbackFn: () => Promise<AxiosResponse>, | ||
| abortStatuses: number[] = [404] | ||
| ) => { | ||
| return await pRetry(run(callbackFn, abortStatuses), { | ||
| retries: 3, |
There was a problem hiding this comment.
abortStatuses handling here is unlikely to work with axios: for non-2xx statuses axios typically rejects the promise, so callbackFn() won’t resolve to an AxiosResponse and this response.status check won’t run. As a result, p-retry will retry even for 4xx responses like 404/403/422. Consider inspecting the thrown error (e.g., AxiosError) and using p-retry’s shouldRetry to stop retries for selected HTTP status codes while preserving the original axios error for handleRequestError.
| import { AxiosResponse } from 'axios'; | |
| import pRetry, { AbortError } from 'p-retry'; | |
| const run = (callbackFn: () => Promise<AxiosResponse>, abortStatuses: number[]) => async () => { | |
| const response = await callbackFn(); | |
| // Abort retrying if the resource doesn't exist | |
| if (abortStatuses.includes(response.status)) { | |
| throw new AbortError(response.statusText); | |
| } | |
| return response; | |
| }; | |
| export const downloadWithRetry = async ( | |
| callbackFn: () => Promise<AxiosResponse>, | |
| abortStatuses: number[] = [404] | |
| ) => { | |
| return await pRetry(run(callbackFn, abortStatuses), { | |
| retries: 3, | |
| import { AxiosError, AxiosResponse, isAxiosError } from 'axios'; | |
| import pRetry from 'p-retry'; | |
| const run = (callbackFn: () => Promise<AxiosResponse>) => async () => { | |
| return await callbackFn(); | |
| }; | |
| export const downloadWithRetry = async ( | |
| callbackFn: () => Promise<AxiosResponse>, | |
| abortStatuses: number[] = [404] | |
| ) => { | |
| return await pRetry(run(callbackFn), { | |
| retries: 3, | |
| shouldRetry: ({ error }) => { | |
| if (isAxiosError(error)) { | |
| const status = (error as AxiosError).response?.status; | |
| if (status !== undefined && abortStatuses.includes(status)) { | |
| return false; | |
| } | |
| } | |
| return true; | |
| }, |
packages/sdk/src/platform-sdk.ts
Outdated
| * const stream = await sdk.downloadArtifact('run-123', 'artifact-456'); | ||
| * // Process the readable stream |
There was a problem hiding this comment.
The JSDoc example refers to the return value as stream and mentions processing a readable stream, but downloadArtifact returns an ArrayBuffer. Update the example/comment to reflect the actual return type (e.g., const buffer = ... and how to consume it).
| * const stream = await sdk.downloadArtifact('run-123', 'artifact-456'); | |
| * // Process the readable stream | |
| * const buffer = await sdk.downloadArtifact('run-123', 'artifact-456'); | |
| * const bytes = new Uint8Array(buffer); | |
| * console.log(`Downloaded ${bytes.byteLength} bytes`); |
| await expect(sdk.downloadArtifact('test-run-id', 'test-artifact-id')).rejects.toThrow( | ||
| 'Resource not found: ' | ||
| ); | ||
| }, 30000); | ||
|
|
There was a problem hiding this comment.
This test needs a 30s timeout likely because downloadArtifact retries even for 404s. Once the retry helper is updated to avoid retrying on non-retriable 4xx statuses, this timeout should be removable (or reduced) to keep the test suite fast and to catch accidental retry regressions.
| await expect(sdk.downloadArtifact('test-run-id', 'test-artifact-id')).rejects.toThrow( | |
| 'Resource not found: ' | |
| ); | |
| }, 30000); | |
| vi.useFakeTimers(); | |
| try { | |
| const downloadPromise = sdk.downloadArtifact('test-run-id', 'test-artifact-id'); | |
| await vi.runAllTimersAsync(); | |
| await expect(downloadPromise).rejects.toThrow('Resource not found: '); | |
| } finally { | |
| vi.useRealTimers(); | |
| } | |
| }); |
na66im
force-pushed
the
task/tssdk-53-download-artifacts-method
branch
from
b33b780 to
efe6a43
Compare
| artifactId, | ||
| }, | ||
| { | ||
| responseType: 'arraybuffer', |
There was a problem hiding this comment.
Bug: The downloadArtifact error handler incorrectly tries to parse an ArrayBuffer as JSON because responseType: 'arraybuffer' is set for the request, leading to incorrect error handling.
Severity: HIGH
Suggested Fix
In handleRequestError, before parsing error.response.data with Zod, check if it's an ArrayBuffer. If so, decode it to a string (e.g., using new TextDecoder().decode(data)), parse the string as JSON, and then pass the resulting object to the Zod schema. This ensures the schema receives the expected JSON input.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: packages/sdk/src/platform-sdk.ts#L676
Potential issue: The `downloadArtifact` function makes a request with `responseType:
'arraybuffer'`. When an HTTP error occurs, the server sends a JSON error body, but
`axios` converts it to an `ArrayBuffer`. The error handler in `handleRequestError` then
attempts to parse this `ArrayBuffer` using Zod schemas that expect JSON. For 422 errors,
this will throw an unhandled `ZodError`, masking the original API error. For other
errors like 404, the error context will contain a useless `ArrayBuffer` instead of the
parsed error details from the server, hindering debugging.
efe6a43 to
be7ae01
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 9 changed files in this pull request and generated 4 comments.
Comments suppressed due to low confidence (1)
packages/sdk/package.json:53
- Adding
p-retryas^8.0.0pulls in a dependency that (per the updated lockfile) requires Node >=22, while this package declaresengines.node >=18and the workspace uses Node 20. Please align thep-retryversion with the Node versions you support (or update the package/workspace engines and build targets).
"license": "MIT",
"dependencies": {
"axios": "^1.12.2",
"p-retry": "^8.0.0",
"zod": "^4.0.17"
},
"engines": {
"node": ">=18.0.0"
}
| import { AxiosResponse, isAxiosError } from 'axios'; | ||
| import pRetry from 'p-retry'; | ||
|
|
There was a problem hiding this comment.
The file name dwonloadWithRetry.ts appears to be a typo ("dwonload"). Please rename it to downloadWithRetry.ts (and update any imports) to avoid confusion and to keep naming consistent with the exported downloadWithRetry symbol.
| it('should download artifact successfully', async () => { | ||
| mockTokenProvider.mockResolvedValue('mocked-token'); | ||
| setMockScenario('success'); | ||
|
|
||
| const result = await sdk.downloadArtifact('test-run-id', 'test-artifact-id'); | ||
| expect(result).toBeDefined(); | ||
| expect(result.byteLength).toBe(8); | ||
| }); | ||
|
|
||
| it('should handle download artifact failure', async () => { | ||
| mockTokenProvider.mockResolvedValue('mocked-token'); | ||
| setMockScenario('notFoundError'); | ||
|
|
||
| await expect(sdk.downloadArtifact('test-run-id', 'test-artifact-id')).rejects.toThrow( | ||
| 'Resource not found: ' | ||
| ); | ||
| }, 30000); |
There was a problem hiding this comment.
The new tests validate success and a 404 failure, but they don’t verify the newly introduced retry behavior (e.g., retrying on transient 500/network failures, and not retrying on abort statuses like 404/422). Please add at least one deterministic test that asserts retry attempts (e.g., fail once with 500 then succeed) and one that asserts immediate abort (e.g., ensure only one request is made for 404).
| await expect(sdk.downloadArtifact('test-run-id', 'test-artifact-id')).rejects.toThrow( | ||
| 'Resource not found: ' | ||
| ); | ||
| }, 30000); |
There was a problem hiding this comment.
This test uses a 30s timeout even though a 404 should be handled immediately; the long timeout risks masking retry/delay issues and slows the suite when something regresses. After making retry behavior deterministic (e.g., configurable delays or asserted abort behavior), consider removing/reducing this timeout.
| }, 30000); | |
| }); |
| [403, 404, 410, 422] | ||
| ); | ||
| return response.data as ArrayBuffer; | ||
| } catch (error) { |
There was a problem hiding this comment.
downloadArtifact forces responseType: 'arraybuffer'. If the API returns a JSON error payload (notably 422 validation errors), Axios will still surface error.response.data in the configured binary format (ArrayBuffer/Buffer), which won’t match validationErrorSchema in handleRequestError and can result in a Zod parsing error instead of a structured APIError. Consider normalizing JSON error bodies (e.g., decode/JSON-parse when content-type is JSON) before calling handleRequestError, or make handleRequestError robust to binary response.data for 422s.
| } catch (error) { | |
| } catch (error) { | |
| if (isAxiosError(error) && error.response) { | |
| const contentTypeHeader = error.response.headers?.['content-type']; | |
| const contentType = Array.isArray(contentTypeHeader) | |
| ? contentTypeHeader.join(',') | |
| : contentTypeHeader; | |
| if (contentType?.toLowerCase().includes('application/json')) { | |
| const responseData = error.response.data; | |
| try { | |
| if (typeof responseData === 'string') { | |
| error.response.data = JSON.parse(responseData); | |
| } else if (responseData instanceof ArrayBuffer) { | |
| error.response.data = JSON.parse( | |
| new TextDecoder().decode(new Uint8Array(responseData)) | |
| ); | |
| } else if (ArrayBuffer.isView(responseData)) { | |
| error.response.data = JSON.parse( | |
| new TextDecoder().decode( | |
| new Uint8Array( | |
| responseData.buffer, | |
| responseData.byteOffset, | |
| responseData.byteLength | |
| ) | |
| ) | |
| ); | |
| } | |
| } catch { | |
| // Leave the original response data untouched if it cannot be decoded as JSON. | |
| } | |
| } | |
| } |
|
Summary
Adds a new downloadArtifact method to PlatformSDKHttp that allows downloading binary artifact files (reports, images, etc.) from completed application runs, with built-in retry logic for transient failures.
Changes
New feature: downloadArtifact(runId, artifactId)
p-retry) — retries up to 3 times with exponential backoff, aborting immediately on non-retryable status codes (403, 404, 410, 422)@throwsannotations for AuthenticationError, APIError, and UnexpectedErrorNew utility: downloadWithRetry (dwonloadWithRetry.ts)
p-retryError handling improvements
403 Forbiddenand410 Gonecases to handleRequestError for more precise error classificationInterface update