fix: correct malformed CreateArtifact Twirp request, make upload_artifact failures non-fatal, and add live API integration tests#39008
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…t non-fatal
- Fix artifact_client.cjs to use camelCase field names (workflowRunBackendId,
workflowJobRunBackendId, mimeType, expiresAt) matching the proto3 JSON spec
- Fix mimeType to be a plain string instead of a wrapped { value: ... } object,
which caused HTTP 400 "the json request could not be decoded" from the Twirp API
- Fix hash field in FinalizeArtifact to be a plain string instead of { value: ... }
- Make upload_artifact failures non-fatal in safe_output_handler_manager.cjs:
a failed artifact upload now emits a warning rather than failing the entire
safe_outputs job (matching the existing behaviour for assign_to_agent)
- Update tests to assert correct camelCase field names and plain-string mimeType/hash
- Add upload_artifact to the report-only failure tests
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
@copilot compare changes with sources in actions/toolkit upload-artifact |
Compared against The toolkit uses
Response parsing uses snake_case ( The request encoding in this PR matches what the toolkit sends after protobuf-ts serialises the proto types. |
There was a problem hiding this comment.
Pull request overview
This PR fixes GitHub Actions artifact uploads in safe_outputs by correcting the Twirp/proto3 JSON request encoding for CreateArtifact / FinalizeArtifact, and by making upload_artifact failures non-fatal so they don’t fail an otherwise-successful safe_outputs job.
Changes:
- Update
CreateArtifact/FinalizeArtifactrequest bodies to use proto3 JSON conventions (camelCase field names and plain-stringStringValuefields). - Treat
upload_artifactfailures as report-only (warn + summarize) instead of failing the job. - Add/extend tests covering the request encoding and report-only partitioning behavior.
Show a summary per file
| File | Description |
|---|---|
| actions/setup/js/artifact_client.cjs | Adjusts artifact Twirp request payload fields for proto3 JSON correctness. |
| actions/setup/js/artifact_client.test.cjs | Adds a test asserting the corrected request body encoding. |
| actions/setup/js/safe_output_handler_manager.cjs | Marks upload_artifact failures as report-only and updates reporting output. |
| actions/setup/js/safe_output_handler_manager.test.cjs | Adds tests ensuring upload_artifact failures are treated as report-only (not fatal). |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comments suppressed due to low confidence (1)
actions/setup/js/artifact_client.cjs:360
- The Twirp/proto3 JSON mapping uses lowerCamelCase field names in responses as well. After switching request bodies to camelCase, this code still expects
signed_upload_url, so a canonicalsignedUploadUrlresponse would be treated as invalid and break uploads.
createRequest.expiresAt = expiresAt;
}
/** @type {any} */
const createResponse = await twirpRequest("CreateArtifact", createRequest);
- Files reviewed: 4/4 changed files
- Comments generated: 4
| workflowRunBackendId, | ||
| workflowJobRunBackendId, | ||
| name: artifactName, | ||
| size: String(uploadSize), | ||
| hash: { value: `sha256:${sha256}` }, | ||
| hash: `sha256:${sha256}`, |
| function isReportOnlyFailureResult(result) { | ||
| return isFailedProcessingResult(result) && result?.type === "assign_to_agent"; | ||
| return isFailedProcessingResult(result) && (result?.type === "assign_to_agent" || result?.type === "upload_artifact"); | ||
| } |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #39008 does not have the 'implementation' label (has_implementation_label=false) and has 0 new lines of code in business logic directories (default_business_additions=0, well under the 100-line threshold). Neither Condition A nor Condition B is met. |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
🧪 Test Quality Sentinel completed test quality analysis. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnose and /tdd — no blocking issues, but two maintainability concerns worth addressing before this pattern expands further.
📋 Key Themes & Highlights
Key Themes
- DRY violation in
safe_output_handler_manager.cjs:isReportOnlyFailureResultmaintains its own inline OR chain whileREPORT_ONLY_FAILURE_TYPESSet exists alongside it.partitionFailureResultsuses the Set directly and skips the predicate entirely — two implementations of the same concept that will diverge on the next addition. - Missing test path: the regression test covers
application/octet-streamonly;application/zip(the path from the original bug report) has no coverage.
Positive Highlights
- ✅ Root cause properly identified and fixed at both callsites (
CreateArtifactandFinalizeArtifact) - ✅
REPORT_ONLY_FAILURE_TYPESSet is a good extensibility improvement forpartitionFailureResults - ✅ Warning message now emits actual failure types instead of hardcoded copy — great observability improvement
- ✅ Tests for
isReportOnlyFailureResultcover skipped/cancelled edge cases cleanly - ✅ Mixed fatal+report-only partition test is well-structured
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 254.9 AIC · ⌖ 13.3 AIC · ⊞ 27.8K
| */ | ||
| function isReportOnlyFailureResult(result) { | ||
| return isFailedProcessingResult(result) && result?.type === "assign_to_agent"; | ||
| return isFailedProcessingResult(result) && (result?.type === "assign_to_agent" || result?.type === "upload_artifact"); |
There was a problem hiding this comment.
[/diagnose] isReportOnlyFailureResult duplicates the type list from REPORT_ONLY_FAILURE_TYPES, creating a divergence risk — a future maintainer adding a type to the Set will not automatically update this inline OR chain.
💡 Suggested fix
Move REPORT_ONLY_FAILURE_TYPES above this function, then have the function delegate to the Set:
const REPORT_ONLY_FAILURE_TYPES = new Set(["assign_to_agent", "upload_artifact"]);
function isReportOnlyFailureResult(result) {
return isFailedProcessingResult(result) && REPORT_ONLY_FAILURE_TYPES.has(result?.type);
}This makes the Set the single source of truth for both this predicate and partitionFailureResults.
| const failedResults = results.filter(isFailedProcessingResult); | ||
| const reportOnlyFailures = failedResults.filter(r => r?.type === "assign_to_agent"); | ||
| const fatalFailures = failedResults.filter(r => r?.type !== "assign_to_agent"); | ||
| const reportOnlyFailures = failedResults.filter(r => REPORT_ONLY_FAILURE_TYPES.has(r?.type)); |
There was a problem hiding this comment.
[/diagnose] partitionFailureResults uses REPORT_ONLY_FAILURE_TYPES.has(r?.type) directly rather than calling isReportOnlyFailureResult, so a bug in the predicate (e.g. a missing isFailedProcessingResult guard) would affect tests but not the real partitioning path.
💡 Suggested fix
Once isReportOnlyFailureResult delegates to the Set (see companion comment on line 556), this function can use the predicate directly:
const reportOnlyFailures = failedResults.filter(r => isReportOnlyFailureResult(r));
const fatalFailures = failedResults.filter(r => !isReportOnlyFailureResult(r));This collapses two implementations of the same concept into one.
| vi.stubEnv("ACTIONS_RESULTS_URL", "https://results.example.com"); | ||
|
|
||
| const client = new DefaultArtifactClient(); | ||
| await client.uploadArtifact("my-artifact", [filePath], tmpDir, { skipArchive: true, retentionDays: 30 }); |
There was a problem hiding this comment.
[/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.
| 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 () => { |
There was a problem hiding this comment.
[/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"
There was a problem hiding this comment.
REQUEST_CHANGES: two issues introduced in this PR need fixing before merge.
Blocking issues
isReportOnlyFailureResult diverges from REPORT_ONLY_FAILURE_TYPES
The PR introduces REPORT_ONLY_FAILURE_TYPES as the canonical Set for partitionFailureResults, but isReportOnlyFailureResult keeps a parallel hardcoded OR-chain. Both are exported. A future type addition to the Set will silently leave the predicate stale. Fix: move the Set above the function and have it use REPORT_ONLY_FAILURE_TYPES.has(result?.type).
Non-fatal warning is undiagnosable
The updated core.warning logs only type names, not error messages. The fatal-failure path at line 1509 includes r.error; this warning should too. Also, unlike every other type-list in this file, .map() here has no new Set() dedup, so repeated failures produce a noisy repeated list.
Non-blocking (no comment posted)
core.info("Reported assignment failures: ...")at line 1476 was not updated — the count now includes artifact upload failures but the label still says "assignment".
🔎 Code quality review by PR Code Quality Reviewer · 432.8 AIC · ⌖ 13.2 AIC · ⊞ 16.7K
| */ | ||
| function isReportOnlyFailureResult(result) { | ||
| return isFailedProcessingResult(result) && result?.type === "assign_to_agent"; | ||
| return isFailedProcessingResult(result) && (result?.type === "assign_to_agent" || result?.type === "upload_artifact"); |
There was a problem hiding this comment.
isReportOnlyFailureResult ignores REPORT_ONLY_FAILURE_TYPES: this exported function hardcodes the type list inline, diverging from the Set introduced three lines below—adding a third type to the Set will silently break this function and any callers.
💡 Suggested fix
Move REPORT_ONLY_FAILURE_TYPES above this function and delegate to it:
const REPORT_ONLY_FAILURE_TYPES = new Set(["assign_to_agent", "upload_artifact"]);
function isReportOnlyFailureResult(result) {
return isFailedProcessingResult(result) && REPORT_ONLY_FAILURE_TYPES.has(result?.type);
}The Set was introduced as the canonical list for partitionFailureResults, but isReportOnlyFailureResult was left with an independent hardcoded OR-chain. Any future addition to the Set requires a second independent edit here with no compile-time guarantee they stay in sync.
| const reportOnlyTypes = reportOnlyFailures.map(r => r.type || "unknown"); | ||
| core.warning(`${reportOnlyFailureCount} non-fatal safe output(s) failed but were reported without failing safe_outputs: ${reportOnlyTypes.join(", ")}`); |
There was a problem hiding this comment.
Non-fatal warning omits error details and may be repetitive: unlike the fatal-failure path that logs r.error, this warning only logs type names, making upload_artifact failures undiagnosable from the warning alone.
💡 Suggested fix
Include error strings and deduplicate types like the rest of the file does:
const reportOnlyItems = reportOnlyFailures.map(r => `${r.type || "unknown"}: ${r.error || "unknown error"}`);
core.warning(`${reportOnlyFailureCount} non-fatal safe output(s) failed but were reported without failing safe_outputs:\n${reportOnlyItems.join("\n")}`);Additionally, every other type-list construction in this file uses [...new Set(...)] to deduplicate. If multiple upload_artifact calls fail, the raw .map() here will produce a repeated list of type names in the warning, inconsistent with the rest of the summary logging.
🧪 Test Quality Sentinel Report✅ Test Quality Score: 82/100 — Excellent
📊 Metrics & Test Classification (4 tests analyzed)
Test Classification Details
Language SupportTests analyzed:
Verdict
📖 Understanding Test ClassificationsDesign Tests (High Value) verify what the system does:
Implementation Tests (Low Value) verify how the system does it:
Goal: Shift toward tests that describe the system's behavioral contract — the promises it makes to its users and collaborators. References: §27456668992
|
|
@copilot run pr-finisher skill |
…er_manager Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…n isReportOnlyFailureResult Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done. Addressed all four review threads from the automated review:
All 100 JS tests pass locally. CI needs a human re-trigger after these pushes. |
|
Please re-trigger CI and summarize any remaining blockers.
|
|
|
@copilot please summarize the remaining blocker and next step for this PR.
|
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…n test Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done. Added in commit
|
|
…idation in integration tests Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
Fixed in the latest commit:
|
|
@copilot run npm update to get latest packages (with cooldown) |
upload_artifactwas failing everysafe_outputsjob with HTTP 400malformed: the json request could not be decodeddue to two bugs in the Twirp request body, and a failing upload was propagating as a fatal error that took down the entire job even when the agent and all other safe outputs succeeded.artifact_client.cjs— malformed Twirp request fieldsTwo encoding errors in
CreateArtifact/FinalizeArtifact:StringValuefields encoded as wrapper objects instead of plain strings. Per the proto3 JSON spec,google.protobuf.StringValueserializes as a bare JSON string. The server received{"value":"application/zip"}and could not decode it.workflowRunBackendId, notworkflow_run_backend_id.Same fix applied to
FinalizeArtifact'shashfield ({ value: "sha256:..." }→"sha256:..."). Response parsing reads both camelCase and snake_case variants (signedUploadUrl/signed_upload_url,artifactId/artifact_id) for robustness.safe_output_handler_manager.cjs— upload_artifact failures now non-fatalAdded
upload_artifacttoREPORT_ONLY_FAILURE_TYPESalongsideassign_to_agent. A failed artifact upload now emits a warning and is counted separately rather than callingcore.setFailed, so it no longer brings down an otherwise-successfulsafe_outputsjob.Tests
artifact_client.test.cjs: assertsCreateArtifact/FinalizeArtifactrequests use camelCase names and plain-stringmimeType/hash.safe_output_handler_manager.test.cjs: assertsupload_artifactfailures are partitioned as report-only, not fatal.Live API integration tests (
cjs.yml)Added a new
artifact-integrationCI job tocjs.ymlthat cross-validates ourDefaultArtifactClientagainst the official@actions/artifactSDK end-to-end:DefaultArtifactClient(via TwirpCreateArtifact/FinalizeArtifact), then lists and downloads the artifact using the official@actions/artifactSDK, verifying our request encoding is server-compatible.@actions/artifactSDK, then lists and downloads it with ourDefaultArtifactClient, verifying our list/download logic is compatible with SDK-produced artifacts.Both tests skip gracefully with an explanatory message when
ACTIONS_RUNTIME_TOKEN/ACTIONS_RESULTS_URLare absent (i.e. outside a GitHub Actions job). The job runs withpermissions: actions: writeand injectsGITHUB_TOKENfor the REST-based download step.@actions/artifactis added as a devDependency.