Skip to content

fix: correct malformed CreateArtifact Twirp request, make upload_artifact failures non-fatal, and add live API integration tests #39008

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.

Sign up for GitHub

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

Merged
pelikhan merged 9 commits into from
Jun 13, 2026
Merged

fix: correct malformed CreateArtifact Twirp request, make upload_artifact failures non-fatal, and add live API integration tests #39008

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
f5cc81d
Initial plan
Copilot Jun 13, 2026
4628656
chore: initial progress report - no changes yet
Copilot Jun 13, 2026
a2efc20
fix: correct malformed CreateArtifact request and make upload_artifac…
Copilot Jun 13, 2026
e0bd62d
Merge branch 'main' into copilot/aw-failures-fix-upload-artifact-request
pelikhan Jun 13, 2026
32d10af
fix: address review feedback on artifact_client and safe_output_handl…
Copilot Jun 13, 2026
0dfc828
fix: use nullish coalescing for artifact ID and explicit type guard i…
Copilot Jun 13, 2026
a9eefe9
feat: add artifact API integration test job to cjs.yml
Copilot Jun 13, 2026
d6d0780
style: use template literals for skip messages in artifact integratio…
Copilot Jun 13, 2026
3f49153
fix: resolve TS typecheck errors, add @actions/artifact SDK cross-val…
Copilot Jun 13, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 34 additions & 0 deletions .github/workflows/cjs.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -151,3 +151,37 @@ jobs:

- name: Check JSON formatting
run: make fmt-check-json

artifact-integration:
name: Artifact API Integration
runs-on: ubuntu-latest
timeout-minutes: 10
permissions:
contents: read
actions: write
concurrency:
group: ci-${{ github.ref }}-artifact-integration
cancel-in-progress: true
steps:
- name: Checkout code
uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2
- name: Set up Node.js
id: setup-node
uses: actions/setup-node@395ad3262231945c25e8478fd5baf05154b1d79f # v6
with:
node-version: "24"
cache: npm
cache-dependency-path: actions/setup/js/package-lock.json
- name: Report Node cache status
run: |
if [ "${{ steps.setup-node.outputs.cache-hit }}" == "true" ]; then
echo "✅ Node cache hit" >> $GITHUB_STEP_SUMMARY
else
echo "⚠️ Node cache miss" >> $GITHUB_STEP_SUMMARY
fi
- name: Install npm dependencies
run: cd actions/setup/js && npm ci
- name: Run artifact API integration tests
env:
GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
run: cd actions/setup/js && npm run test:js-integration-artifact
21 changes: 11 additions & 10 deletions actions/setup/js/artifact_client.cjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -345,32 +345,33 @@ class DefaultArtifactClient {

const { workflowRunBackendId, workflowJobRunBackendId } = getBackendIdsFromRuntimeToken();
const createRequest = {
workflow_run_backend_id: workflowRunBackendId,
workflow_job_run_backend_id: workflowJobRunBackendId,
workflowRunBackendId,
workflowJobRunBackendId,
name: artifactName,
version: 7,
mime_type: { value: contentType },
mimeType: contentType,
};
const expiresAt = formatRetentionTimestamp(options.retentionDays);
if (expiresAt) {
createRequest.expires_at = expiresAt;
createRequest.expiresAt = expiresAt;
}

/** @type {any} */
const createResponse = await twirpRequest("CreateArtifact", createRequest);
if (!createResponse?.ok || !createResponse?.signed_upload_url) {
const signedUploadUrl = createResponse?.signedUploadUrl || createResponse?.signed_upload_url;
if (!createResponse?.ok || !signedUploadUrl) {
throw new Error("CreateArtifact returned an invalid response");
}

const uploadSize = await uploadFileToSignedURL(uploadPath, createResponse.signed_upload_url, contentType);
const uploadSize = await uploadFileToSignedURL(uploadPath, signedUploadUrl, contentType);
const sha256 = await hashFile(uploadPath);

const finalizeRequest = {
workflow_run_backend_id: workflowRunBackendId,
workflow_job_run_backend_id: workflowJobRunBackendId,
workflowRunBackendId,
workflowJobRunBackendId,
name: artifactName,
size: String(uploadSize),
hash: { value: `sha256:${sha256}` },
hash: `sha256:${sha256}`,
Comment on lines +370 to +374
};
/** @type {any} */
const finalizeResponse = await twirpRequest("FinalizeArtifact", finalizeRequest);
Expand All @@ -379,7 +380,7 @@ class DefaultArtifactClient {
}

return {
id: Number(finalizeResponse.artifact_id || 0) || undefined,
id: Number(finalizeResponse.artifactId ?? finalizeResponse.artifact_id ?? 0) || undefined,
size: uploadSize,
digest: sha256,
};
Expand Down
68 changes: 62 additions & 6 deletions actions/setup/js/artifact_client.test.cjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -212,12 +212,68 @@ describe("DefaultArtifactClient.uploadArtifact", () => {
await expect(client.uploadArtifact("art", ["/a", "/b"], tmpDir, { skipArchive: true })).rejects.toThrow("single file");
});

it("sends camelCase field names and mimeType as a plain string in CreateArtifact request", async () => {

@github-actions github-actions Bot Jun 13, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/tdd] The test name says "in CreateArtifact request" but the test also asserts the FinalizeArtifact body (lines 261–268). Consider renaming it to make the full scope discoverable at a glance:

"sends correctly encoded camelCase fields in CreateArtifact and FinalizeArtifact requests"

const filePath = path.join(tmpDir, "test.bin");
fs.writeFileSync(filePath, "test content");

/** @type {Record<string, any>|null} */
let capturedCreateBody = null;
/** @type {Record<string, any>|null} */
let capturedFinalizeBody = null;

const mockFetch = vi.fn().mockImplementation(async (url, opts) => {
const u = String(url);
if (u.includes("CreateArtifact")) {
capturedCreateBody = JSON.parse(opts?.body || "{}");
return makeFetchResponse(200, { ok: true, signedUploadUrl: "https://storage.example.com/upload" });
}
if (u.includes("FinalizeArtifact")) {
capturedFinalizeBody = JSON.parse(opts?.body || "{}");
return makeFetchResponse(200, { ok: true, artifactId: "42" });
}
return makeFetchResponse(200, "");
});
vi.stubGlobal("fetch", mockFetch);

vi.stubEnv("ACTIONS_RUNTIME_TOKEN", buildFakeToken("runId:jobId"));
vi.stubEnv("ACTIONS_RESULTS_URL", "https://results.example.com");

const client = new DefaultArtifactClient();
await client.uploadArtifact("my-artifact", [filePath], tmpDir, { skipArchive: true, retentionDays: 30 });

@github-actions github-actions Bot Jun 13, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/tdd] This test only exercises the skipArchive: true path, which uses application/octet-stream. The original bug was reported with application/zip (the default zip-archive path), so that path has no regression coverage.

💡 Suggested addition

Add a second test without skipArchive to lock in the zip-path encoding:

it("sends mimeType as a plain string when artifact is a zip archive", async () => {
  // same mock setup...
  let capturedMimeType = null;
  // capture mimeType from CreateArtifact body
  await client.uploadArtifact("my-artifact", [filePath], tmpDir, { retentionDays: 1 });
  expect(typeof capturedMimeType).toBe("string");
  expect(capturedMimeType).toBe("application/zip");
});

This would have caught the original { value: "application/zip" } regression.


// CreateArtifact request must use camelCase field names
expect(capturedCreateBody).not.toBeNull();
expect(capturedCreateBody).toHaveProperty("workflowRunBackendId");
expect(capturedCreateBody).toHaveProperty("workflowJobRunBackendId");
expect(capturedCreateBody).not.toHaveProperty("workflow_run_backend_id");
expect(capturedCreateBody).not.toHaveProperty("workflow_job_run_backend_id");

// mimeType must be a plain string, not a wrapped object
expect(typeof capturedCreateBody?.mimeType).toBe("string");
expect(capturedCreateBody?.mimeType).toBe("application/octet-stream");
expect(capturedCreateBody).not.toHaveProperty("mime_type");

// expiresAt must use camelCase
expect(capturedCreateBody).toHaveProperty("expiresAt");
expect(capturedCreateBody).not.toHaveProperty("expires_at");

// FinalizeArtifact request must use camelCase field names
expect(capturedFinalizeBody).not.toBeNull();
expect(capturedFinalizeBody).toHaveProperty("workflowRunBackendId");
expect(capturedFinalizeBody).toHaveProperty("workflowJobRunBackendId");
expect(capturedFinalizeBody).not.toHaveProperty("workflow_run_backend_id");

// hash must be a plain string (not a wrapped object)
expect(typeof capturedFinalizeBody?.hash).toBe("string");
expect(capturedFinalizeBody?.hash).toMatch(/^sha256:/);
});

it("preserves caller-provided artifact name when skipArchive is true", async () => {
const filePath = path.join(tmpDir, "output.bin");
fs.writeFileSync(filePath, "hello artifact");

const createResp = { ok: true, signed_upload_url: "https://storage.example.com/upload" };
const finalizeResp = { ok: true, artifact_id: "99" };
const createResp = { ok: true, signedUploadUrl: "https://storage.example.com/upload" };
const finalizeResp = { ok: true, artifactId: "99" };

let uploadedArtifactName;
const mockFetch = vi.fn().mockImplementation(async (url, opts) => {
Expand Down Expand Up @@ -252,10 +308,10 @@ describe("DefaultArtifactClient.uploadArtifact", () => {
const mockFetch = vi.fn().mockImplementation(async (url, opts) => {
if (String(url).includes("CreateArtifact")) {
capturedName = JSON.parse(opts?.body || "{}").name;
return makeFetchResponse(200, { ok: true, signed_upload_url: "https://example.com/upload" });
return makeFetchResponse(200, { ok: true, signedUploadUrl: "https://example.com/upload" });
}
if (String(url).includes("FinalizeArtifact")) {
return makeFetchResponse(200, { ok: true, artifact_id: "1" });
return makeFetchResponse(200, { ok: true, artifactId: "1" });
}
return makeFetchResponse(200, "");
});
Expand Down Expand Up @@ -292,10 +348,10 @@ describe("DefaultArtifactClient.uploadArtifact", () => {
const mockFetch = vi.fn().mockImplementation(async (url, opts) => {
if (String(url).includes("CreateArtifact")) {
capturedName = JSON.parse(opts?.body || "{}").name;
return makeFetchResponse(200, { ok: true, signed_upload_url: "https://example.com/upload" });
return makeFetchResponse(200, { ok: true, signedUploadUrl: "https://example.com/upload" });
}
if (String(url).includes("FinalizeArtifact")) {
return makeFetchResponse(200, { ok: true, artifact_id: "1" });
return makeFetchResponse(200, { ok: true, artifactId: "1" });
}
return makeFetchResponse(200, "");
});
Expand Down
Loading
Loading