GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — Use shared artifacts at cache boundaries by andrew · Pull Request #267 · git-pkgs/proxy

Conversation

andrew commented Aug 15, 2026

Copy link

Copy Markdown

Contributor

Uses artifacts.Artifact for 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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

andrew requested a lite review from Copilot

August 15, 2026 21:36

Copilot started reviewing on behalf of andrew

August 15, 2026 21:37

View session

Copilot AI reviewed

Aug 15, 2026

View reviewed changes

Copilot AI left a comment

Copy link

Copy Markdown

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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 CacheResult to carry an artifacts.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.Artifact at 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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

internal/database/queries.go Outdated

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 thread

internal/mirror/mirror.go

Comment on lines +223 to 224

		tracker.bytes.Add(result.Artifact.Size)
		m.logger.Info("mirrored",

andrew added 3 commits

September 3, 2026 17:30


          Use shared artifacts at cache boundaries

66bcfe3


          Address artifact cache review

f76f4e0


          Defer cached artifact validation to checkCache

8aaf5e8

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 36b864f to 8aaf5e8 Compare

September 3, 2026 16:43


          Build stored Artifact after digest and scan checks

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.

andrew marked this pull request as ready for review

September 4, 2026 09:49

andrew requested a lite review from Copilot

September 4, 2026 09:50

Copilot started reviewing on behalf of andrew

September 4, 2026 09:50

View session

Copilot AI reviewed

Sep 4, 2026

View reviewed changes

Copilot AI left a comment

Copy link

Copy Markdown

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

🟡 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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

internal/database/queries.go

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,
		},
		}
		}

andrew merged commit d950919 into main

Sep 4, 2026

9 checks passed

Uh oh!

There was an error while loading. Please reload this page.

andrew deleted the artifacts-adoption branch

September 4, 2026 09:58

pinguinfuss added a commit to pinguinfuss/proxy that referenced this pull request

Sep 6, 2026


          Merge origin/main into issue-305-transfer-compression

288bd78

Resolves the conflict in proxyMetadataStream: keep the acceptEncoding
parameter from this branch and the r.Method request from main (git-pkgs#267).

Reviewers

Copilot code review Copilot

Copilot review effort

Applies to this pull request for everyone.Learn more about Copilot code review.

Copilot left review comments

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

2 participants

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.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.