[C. Spencer Beggs (spencerbeggs)]

WS4 (part 2) — Artifact service ⚠️ DRAFT: needs live-runner validation

Part 2 of WS4 (PR-C from the spec). The largest @actions/* coverage hole. Bump: minor.

Draft on purpose. Per maintainer decision #5, the Artifact service must be validated end-to-end on a live GitHub-hosted runner before merge — the v2 backend protocol is undocumented and reverse-engineered from the actions/toolkit source. Stacked on #125 (base ws4-toolkit-parity-io); it extracts the shared Twirp client from the now-HttpClient-based ActionCacheLive.

What's implemented

uploadArtifact / listArtifacts / getArtifact (→ Option) / downloadArtifact / deleteArtifact, plus ArtifactError (with retryable) and an in-memory ArtifactTest. Backend: Twirp github.actions.results.api.v1.ArtifactService over @effect/platform HttpClient + Azure block-blob, with run/job backend IDs decoded from the ACTIONS_RUNTIME_TOKEN scp claim. Zips shell out to zip/unzip (PowerShell fallback on Windows) — zero new deps.

Protocol questions resolved by reading the toolkit source (corrects the spec)

• Finalize hash is SHA-256 hex (sha256:<hex>), NOT CRC64 — blob-upload.ts/upload-artifact.ts use crypto.createHash('sha256'). No crc64 module needed.
• CreateArtifact version is 7, not the spec's guessed 4.
• Backend-ID parse (Actions.Results:<run>:<job> from scp) replicated from shared/util.ts.

Refactor (regression-fenced)

Shared Twirp plumbing extracted from ActionCacheLive into internal/twirp.ts (parameterized by service segment). ActionCacheLive.test.ts is byte-for-byte untouched and green; the retry-keying substrings (HTTP <status>, ECONNRESET) survive the extraction.

⚠️ Must be confirmed on a live runner before merge

1. Twirp JSON field casing (read defensively in camel+snake for now).
2. The finalize hash contract over the exact uploaded bytes.
3. version: 7 acceptance / whether it's required.
4. Duplicate-name signal (HTTP 409 vs ok:false — both handled).
5. Signed download URL is an Azure blob URL the SDK accepts.
6. findBy (cross-run/cross-repo REST path) is a typed stub — implement via GitHubClient if needed.

Verification (mocked; no live backend hit)

• pnpm run lint — exit 0 · pnpm run ci:build — passes · pnpm run test — 1157 passed (Azure SDK + Twirp HttpClient fully mocked; ArtifactLive Windows-only PowerShell zip branches are the only uncovered lines on this darwin host)

Signed-off-by: C. Spencer Beggs spencer@savvyweb.systems

[savvy-web-bot]

Code Review

Current Commit: f80e124

Validation Results

Check	Status
PR Title Validation	✅ success
Conventional Commits	✅ success
Code Quality	✅ success
Markdown	✅ success
Build	✅ success
Tests	✅ success
CodeQL	✅ success (no new alerts)

All checks green. This is a first review — no previous inline comments to resolve.

---

Summary

Solid draft. The protocol research is thorough (the version: 7 / SHA-256 corrections over the spec's guesses are valuable), the twirp.ts extraction is clean, Redacted token handling and Effect.acquireUseRelease resource cleanup are both done right, and the defensive camelCase+snake_case dual-field reading is exactly the right call for a reverse-engineered protocol. Test coverage is comprehensive for a fully mocked implementation — backendIds.ts in particular is well-isolated and tested.

Four items to address before the live-validation pass:

1. Duplicate isRetryableReason [fix before merge]

ArtifactLive.ts:94–97 defines a local isRetryableReason that is byte-for-byte identical to isRetryableTwirpReason already exported from internal/twirp.ts. The entire point of extracting twirp.ts was to share this predicate. Import and use isRetryableTwirpReason instead — see inline comment.

2. FinalizeArtifactResponse reused for DeleteArtifact [nit]

The DeleteArtifact RPC is called with twirpCall<FinalizeArtifactResponse> which works today because both responses happen to share { ok, artifactId }, but silently couples two unrelated protocol operations. A dedicated DeleteArtifactResponse type is cheap insurance. See inline.

3. Azure upload constants duplicated [pre-merge suggestion]

UPLOAD_CHUNK_SIZE, UPLOAD_CONCURRENCY, and UPLOAD_MAX_SINGLE_SHOT are defined identically in both ActionCacheLive.ts and ArtifactLive.ts. If these get tuned after the live-validation run (e.g. chunking strategy differs for artifacts vs cache), the duplication will bite. A shared internal/azureUpload.ts or just re-exporting them from internal/twirp.ts would consolidate them. See inline.

4. Missing test: empty files array [nit]

createZip has an early throw for files.length === 0 but it's the only error path in uploadArtifact without a test. One-liner to add. See inline.

---

Positives worth noting

• The JWT scp-claim parsing in backendIds.ts is well-isolated with its own test file — easy to update if the token format changes.
• getArtifactEnv redacting the token immediately and unwrapping it only at the request boundary (S9) is exactly right.
• Effect.acquireUseRelease for both upload (zip creation → cleanup) and download (zip download → cleanup) correctly handles failure cleanup without try/finally boilerplate.
• The CONFLICT sentinel returning [] from listInternal (line 305) is a graceful fallback over failing on a protocol edge case.
• findByUnsupported as a named helper makes the four findBy stubs easy to scan and update together when the REST path gets implemented.
• The twirp.test.ts retry-schedule test using TestClock is a nice touch — it validates the retry budget without wallclock waits.

[savvy-web-bot]

Two items to address before the live-validation pass (details in the inline comments and summary comment). All previously flagged issues are resolved.

[savvy-web-bot]

Code Review

Current Commit: 4c1dc378bd17864b9963070d5e724f24d191d79f

Note: The branch was amended again after the previous review (cec7d3099f → 4c1dc37). The retentionDays thread has been replied to and resolved. Two inline comments carried forward below.

CI Checks

Check	Status
PR Title Validation	✅ success
Conventional Commits	✅ success
Code Quality	✅ success
Markdown	✅ success
Build	✅ success
Tests	✅ success (1157 passed)

Resolved Since Last Review

• ✅ retentionDays not forwarded — expiresAt is now computed and sent in FinalizeArtifact (lines 408–410). Thread resolved.

Open Issues

1. compressionLevel documented but not implemented (silent no-op)

UploadOptions.compressionLevel is documented as "zlib compression level 0–9 passed to zip. Default 6, matching @actions/artifact", but createZip neither accepts nor applies it. The POSIX zip flag is -N (e.g. -6 for default, -9 for best). Until wired, the option silently does nothing — callers trusting the JSDoc will be misled.

Fix: pass compressionLevel (default 6) to createZip and prepend -${level} to the zip args. Mark as TODO at minimum. Inline comment posted at src/services/Artifact.ts:32.

2. Minor: pickArtifactId typed for FinalizeArtifactResponse, called with DeleteArtifactResponse (line 558)

The DeleteArtifactResponse interface was added (which is great — the previous thread is resolved), but pickArtifactId(deleteResult) still passes a DeleteArtifactResponse to a function that expects FinalizeArtifactResponse. TypeScript accepts it because the shapes are identical, but it contradicts the intent of the comment at lines 112–115. A one-liner pickDeleteArtifactId completes the decoupling. Inline comment posted at src/layers/ArtifactLive.ts:558.

Positive Observations

• retentionDays → expiresAt forwarding is now wired and covered by the test at line 270 of ArtifactLive.test.ts.
• The relative() fix in createZip (storing archive-relative paths) is a solid portability improvement over the original.
• mimeType: "application/zip" in CreateArtifact is a correct protocol detail, confirmed by the test assertion.
• The generic twirpCall<T, E> design in internal/twirp.ts with the onError callback is elegant — it avoids coupling shared transport to service-specific error types.
• internal/backendIds.ts cleanly separates JWT parsing from env reading, with thorough test coverage in backendIds.test.ts.
• The full mock stack (HttpClient + Azure SDK + fs) in ArtifactLive.test.ts gives solid mocked coverage of the upload/download/delete paths.
• internal/azureUpload.ts correctly extracts the shared Azure upload constants so a future tuning flows through to both ActionCacheLive and ArtifactLive.

Summary

One functional gap (compressionLevel silent no-op) and one minor consistency issue (pickArtifactId type) remain before merge. Both are quick fixes. The larger open questions — Twirp field casing, hash contract, version: 7 acceptance, findBy REST path — are correctly deferred to live-runner validation.

[savvy-web-bot]

Two remaining pre-merge items from the previous review (compressionLevel and pickDeleteArtifactId) — both carried forward as inline comments above.

[savvy-web-bot]

compressionLevel is documented as "zlib compression level 0–9 passed to zip" but createZip neither accepts nor uses it — the zip call at ArtifactLive.ts:190 always uses the default level. Users setting this option today get no effect.

POSIX zip supports -0 through -9 (e.g. -6 is the default, matching @actions/artifact). The fix is:

// createZip signature:
const createZip = (files, rootDir, artifact, compressionLevel = 6) => ...

// POSIX branch:
execFileSync("zip", [`-${compressionLevel}qr`, zipPath, ...relFiles], { cwd: rootDirectory, stdio: "pipe" });

Worth implementing or explicitly marking as deferred (e.g. // TODO: compressionLevel not yet wired) before the public API solidifies — right now the JSDoc promise is silently broken.

[savvy-web-bot]

pickArtifactId is typed (r: FinalizeArtifactResponse) but is called here with deleteResult (DeleteArtifactResponse). TypeScript accepts it because the two interfaces are structurally identical today, but it undermines the comment at lines 112–115 ("avoid coupling the two if either diverges").

One-liner fix — add a dedicated picker:

const pickDeleteArtifactId = (r: DeleteArtifactResponse): string | undefined => r.artifactId ?? r.artifact_id;

…and use it here:

const id = Number(pickDeleteArtifactId(deleteResult) ?? match.value.id);