Conversation
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
Codecov Report✅ All modified and coverable lines are covered by tests. @@ Coverage Diff @@
## main #70 +/- ##
==========================================
+ Coverage 96.38% 97.19% +0.81%
==========================================
Files 13 14 +1
Lines 1301 1357 +56
Branches 191 199 +8
==========================================
+ Hits 1254 1319 +65
+ Misses 47 38 -9
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. |
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"
}
b33b780 to
efe6a43
Compare
na66im
force-pushed
the
task/tssdk-53-download-artifacts-method
branch
from
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"
}
| 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).
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 10 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
packages/sdk/package.json:53
p-retry@8.0.0appears to require Node >=22 (see package-lock), but this package declaresengines.node >=18and the SDK is built/CI-tested on Node 18/20. This dependency choice is likely to break installs/runs on supported Node versions; please pin to ap-retrymajor that supports Node 18 (or raise the SDK/CI Node engine/target accordingly).
"dependencies": {
"axios": "^1.12.2",
"p-retry": "^8.0.0",
"zod": "^4.0.17"
},
"engines": {
"node": ">=18.0.0"
}
| async downloadArtifact(runId: string, artifactId: string): Promise<ArrayBuffer> { | ||
| const client = await this.#getClient(); |
There was a problem hiding this comment.
Parameter naming is inconsistent with the rest of the SDK: other run-related methods use applicationRunId, but this method uses runId. Consider renaming the public parameter (and interface signature) to applicationRunId for consistency and to avoid ambiguity with other IDs.
na66im
force-pushed
the
task/tssdk-53-download-artifacts-method
branch
from
7e69544 to
0cd82ff
Compare
|
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