Use shared artifacts at cache boundaries - #267
Merged
Merged
Conversation
There was a problem hiding this comment.
Pull request overview
This PR standardizes cached and newly stored package file metadata by representing artifacts with a shared artifacts.Artifact type at cache boundaries, while keeping the stored SHA-256 hex digest format unchanged for ETags and verification.
Changes:
- Refactors
CacheResultto carry anartifacts.Artifact(digest/size/filename/media type) instead of separate fields, updating serving and mirroring logic accordingly. - Validates and converts cached artifact database rows into
artifacts.Artifactat the database boundary (GetCachedArtifact). - Updates tests and adds coverage for malformed cached metadata and malformed storage digests; adds new module dependencies.
Reviewed changes
Copilot reviewed 9 out of 10 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/mirror/mirror.go | Uses shared artifact size metadata for mirror accounting. |
| internal/handler/handler.go | Moves cache/fetch paths to artifacts.Artifact for size/ETag/media type and DB updates. |
| internal/handler/handler_test.go | Updates tests to assert artifact metadata and adds malformed-metadata cases. |
| internal/handler/download_test.go | Updates seeded cached artifact hashes to use encoded digest. |
| internal/handler/container_test.go | Updates ETag assertions to use encoded digest. |
| internal/database/types.go | Changes cached-artifact shape to embed artifacts.Artifact. |
| internal/database/queries.go | Converts/validates cached artifact rows into artifacts.Artifact. |
| internal/database/database_test.go | Updates cached-artifact tests and adds row conversion/validation unit tests. |
| go.mod | Adds github.com/git-pkgs/artifacts and github.com/opencontainers/go-digest. |
| go.sum | Adds checksums for the new dependencies. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+229
to
+234
| if !row.ContentHash.Valid || row.ContentHash.String == "" { | ||
| return nil, fmt.Errorf("cached artifact content hash is missing") | ||
| } | ||
| if !row.Size.Valid { | ||
| return nil, fmt.Errorf("cached artifact size is missing") | ||
| } |
Comment on lines
+223
to
224
| tracker.bytes.Add(result.Artifact.Size) | ||
| m.logger.Info("mirrored", |
Construct artifacts.Artifact from the row without validation so a malformed content hash reaches newIntegrityChecks in checkCache, which clears the row and treats the request as a miss. Erroring at the DB boundary instead surfaced a 500 and left the bad row in place.
andrew
force-pushed
the
artifacts-adoption
branch
from
September 3, 2026 16:43
36b864f to
8aaf5e8
Compare
Construct the shared artifact struct from trusted storage output as a literal, after the hash-mismatch and scanner paths that delete failed downloads, so no path between Store and updateCacheDB can leave bytes in storage without a database row.
There was a problem hiding this comment.
🟡 Changes recommended
The cached-artifact row conversion can construct an invalid non-empty digest value when content_hash is NULL/empty, which should be corrected before merge.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 14/14 changed files
- Comments generated: 1
- Review effort level: Lite
Comment on lines
+264
to
277
| func (row cachedArtifactRow) artifact(versionPURL, filename string) *CachedArtifact { | ||
| return &CachedArtifact{ | ||
| Ecosystem: row.Ecosystem, | ||
| StoragePath: row.StoragePath, | ||
| Integrity: row.Integrity, | ||
| Artifact: artifacts.Artifact{ | ||
| PURL: versionPURL, | ||
| Digest: digest.Digest("sha256:" + row.ContentHash.String), | ||
| Size: row.Size.Int64, | ||
| Filename: filename, | ||
| MediaType: row.ContentType.String, | ||
| }, | ||
| } | ||
| } |
pinguinfuss
added a commit
to pinguinfuss/proxy
that referenced
this pull request
Sep 6, 2026
Resolves the conflict in proxyMetadataStream: keep the acceptEncoding parameter from this branch and the r.Method request from main (git-pkgs#267).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Uses
artifacts.Artifactfor cached and newly stored package files. Cache rows are converted and validated at the database boundary while the stored hexadecimal SHA-256 remains unchanged. Response ETags and cache verification continue using the encoded digest, and mirror accounting reads shared size metadata.