Repository navigation
Conversation
There was a problem hiding this comment.
Copilot was unable to review this pull request because there is no eligible user to bill.
An enterprise owner can allow the organization to be billed for reviews like this by enabling Copilot code review for users without a license.
7573e9f to
51a39de
Compare
alliscode
left a comment
alliscode
left a comment
There was a problem hiding this comment.
I found two issues in the artifact storage contract and filesystem write behavior. Please address the inline comments.
| } | ||
|
|
||
| export type ArtifactFailure = { | ||
| status: "invalid-key" | "too-large" | "not-found" | "already-exists" | "unavailable" |
There was a problem hiding this comment.
This shared union makes every method advertise failures it can never return. For example, read() can supposedly return already-exists, and delete() can return too-large. Could we use separate read, write, and delete result types so callers only handle real outcomes?
There was a problem hiding this comment.
Split the public contract into ArtifactReadResult, ArtifactWriteResult, and ArtifactDeleteResult. Reads retain too-large because bounded reads can reject oversized objects, but no longer advertise already-exists; deletes no longer advertise too-large. Provider contracts remain independent of domain types. See 07d0f91.
| return { status: "ok" } as const | ||
| } finally { | ||
| await handle.close() | ||
| await unlink(staging) |
There was a problem hiding this comment.
At this point link() has already published the artifact. If removing the staging file fails, the outer catch returns unavailable even though the key now exists, and a retry then gets already-exists. Could we handle cleanup failure separately and keep the write successful after the link commit?
There was a problem hiding this comment.
Publication now remains successful after a staging cleanup failure. Cleanup reports a static warning through an injected callback and cannot overwrite the original publication outcome, including already-exists. See 07d0f91.
Route immutable byte operations through an artifacts domain with explicit filesystem or managed-identity Azure Blob selection. Validate configured storage without adding product routes. Co-authored-by: Ben Thomas <25218250+alliscode@users.noreply.github.com> Co-authored-by: Nishiki <1121087+nshki@users.noreply.github.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Ben Thomas <25218250+alliscode@users.noreply.github.com> Co-authored-by: Nishiki <1121087+nshki@users.noreply.github.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Ben Thomas <25218250+alliscode@users.noreply.github.com> Co-authored-by: Nishiki <1121087+nshki@users.noreply.github.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Ben Thomas <25218250+alliscode@users.noreply.github.com> Co-authored-by: Nishiki <1121087+nshki@users.noreply.github.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
51a39de to
4cba183
Compare
Why
Hub needs one artifact contract so product domains do not depend on Azure SDK calls. This is the second layer of #171, based on the production runtime.
What changes
The artifacts domain selects filesystem or managed-identity Azure Blob storage and exposes immutable byte reads, writes, and deletion. It validates opaque keys, bounds each artifact to 32 MiB, and gives each operation precise result types without provider details. Filesystem publication commits a write; staging cleanup failure produces a warning without misreporting that write as failed.
Startup validates explicitly configured storage, with no silent provider fallback. This is a storage foundation, not Build import, preview hosting, or product authorization. Real Azure identity and Blob access remain unverified in a live tenant.
Created from a Slack conversation with @alliscode.