replace github.com/golang/snappy with klauspost/compress/snappy by thaJeztah · Pull Request #13048 · cli/cli

thaJeztah · 2026-03-26T22:02:05Z

The github.com/golang/snappy repository was archived and is no longer maintained. klauspost/compress provides a drop-in replacement, which is actively maintained, and the klauspost/compress module is already an existing (indirect) dependency.

relates to Development policy: how are new dependencies added, existing updated or old ones removed? #11468 (comment)
relates to policy: replace golang/snappy with klauspost/compress/snappy docker/buildx#3686

cc @babakks

The github.com/golang/snappy repository was archived and is no longer maintained. klauspost/compress provides a drop-in replacement, which is actively maintained, and the klauspost/compress module is already an existing (indirect) dependency. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

github-actions · 2026-03-26T22:02:17Z

Thanks for your pull request! Unfortunately, it doesn't meet the minimum requirements for review:

None of the referenced issues have the help wanted label

Please update your PR to address the above. Requirements:

Include a detailed description of what this PR does
Link to an issue with the help wanted label (use Fixes #123 or Closes #123 if it resolves the issue)

This PR will be automatically closed in 7 days if these requirements are not met.

Copilot

Pull request overview

Replaces the archived github.com/golang/snappy dependency with the actively maintained drop-in replacement github.com/klauspost/compress/snappy.

Changes:

Swapped Snappy imports in attestation API code and tests to github.com/klauspost/compress/snappy.
Updated go.mod to remove github.com/golang/snappy and add github.com/klauspost/compress as a direct dependency (previously indirect).
Cleaned up go.sum to drop the removed github.com/golang/snappy checksums.

Reviewed changes

Copilot reviewed 3 out of 4 changed files in this pull request and generated no comments.

File	Description
pkg/cmd/attestation/api/client.go	Updates Snappy import used for bundle decompression.
pkg/cmd/attestation/api/mock_httpClient_test.go	Updates Snappy import used to generate compressed test responses.
go.mod	Removes archived module and promotes `klauspost/compress` to a direct requirement.
go.sum	Removes checksums for the dropped `github.com/golang/snappy` module.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

thaJeztah · 2026-03-26T22:07:05Z

None of the referenced issues have the help wanted label

🙈 probably should've read contributing.md

Let me know if this is still acceptable, without a related ticket @williammartin 😅

andyfeller · 2026-03-27T13:56:07Z

@malancas : since this is changing an underlying dependency of attestation feature set, I thought you and the Package Security team would want to review this.

kommendorkapten · 2026-03-27T14:09:02Z

The change looks solid.

@malancas do you know if we have any unit tests that exercises snappy decompress of attestations prior to verification?

malancas · 2026-03-27T15:47:15Z

The change looks solid.

@malancas do you know if we have any unit tests that exercises snappy decompress of attestations prior to verification?

The decompression functionality is tested before verification through tests within trunk/pkg/cmd/attestation/api/client_test.go that call LiveClient#getBundle. We could add more assert statements around the returned bundles.

CLI: claude-review --pr <URL> GitHub Action: .github/workflows/review.yml for automated PR reviews Structured Markdown output: Summary / Risks / Suggestions / Confidence Score Tested on 2 real GitHub PRs (cli/cli#13046, cli/cli#13048) Payment address: eB51DWp1uECrLZRLsE2cnyZUzfRWvzUzaJzkatTpQV9

thaJeztah · 2026-03-27T19:00:15Z

Thanks for looking! If there were any reasons for not switching that I'm not aware of; happy to close (and no harm done).

I noticed the dependency on an archived repo when I was working on buildx; archived doesn't always mean "bad"; some code is just "complete and stable", but then found that @klauspost offered a drop-in replacement; I know his module is well maintained and widely used (❤️❤️❤️), so switching seemed like a logical choice. Of course assuming all fully compatible.

malancas · 2026-03-27T21:35:31Z

Thanks for looking! If there were any reasons for not switching that I'm not aware of; happy to close (and no harm done).

I noticed the dependency on an archived repo when I was working on buildx; archived doesn't always mean "bad"; some code is just "complete and stable", but then found that @klauspost offered a drop-in replacement; I know his module is well maintained and widely used (❤️❤️❤️), so switching seemed like a logical choice. Of course assuming all fully compatible.

Switching to the actively maintained dependency makes sense to me. These changes LGTM after reviewing the current code and testing. I'll wait until Monday in case @kommendorkapten or @andyfeller had any other thoughts but thank you for opening the pull request

thaJeztah · 2026-03-27T23:44:06Z

For sure, no rush, enjoy the weekend!!

In case relevant; there is a newer patch-release of this module; I think the patch fixes a bug in the zstd implementation, and no changes in snappy, so I didn't want to conflate things with my change (but can update in this PR if you prefer).

kommendorkapten

Thanks!

babakks · 2026-03-30T13:40:28Z

Thanks for the PR, @thaJeztah! 🙏

Great to see the changes are transparent, but I need to callout there maybe a bit of concern around the license file in github.com/klauspost/compress. The repo seems to have both BSD-3 and Apache 2 licenses, concatenated in a single file. I need to check if our license extraction script is happy with it.

So, please don't merge this until I've confirmed it's safe to do so.

babakks · 2026-03-30T14:28:24Z

Tl;DR is we're good. We just print a few more licenses (two) that we don't really need to embed.

As for the details, these are the extracted licenses by running ./script/licenses linux amd64 (all other supported os/arch result in the same output as I checked):

internal/licenses/embed/linux-amd64/third-party/github.com/klauspost/compress/
├── LICENSE                            (BSD-3 + Apache 2.0 + MIT, total 5 licenses)
├── internal/snapref/LICENSE           (BSD-3)
├── s2/LICENSE                         (BSD-3)
├── snappy/LICENSE                     (BSD-3)
└── zstd/internal/xxhash/LICENSE.txt   (MIT)

The extracted licenses belong to the packages we need to build gh (so not necessarily all packages/licenses in the new module end up here). And this is the summary part of the gh licenses output (the top part):

github.com/klauspost/compress (v1.18.4) - MIT - https://github.com/klauspost/compress/blob/v1.18.4/LICENSE
github.com/klauspost/compress (v1.18.4) - Apache-2.0 - https://github.com/klauspost/compress/blob/v1.18.4/LICENSE
github.com/klauspost/compress (v1.18.4) - BSD-3-Clause - https://github.com/klauspost/compress/blob/v1.18.4/LICENSE
github.com/klauspost/compress/internal/snapref (v1.18.4) - BSD-3-Clause - https://github.com/klauspost/compress/blob/v1.18.4/internal/snapref/LICENSE
github.com/klauspost/compress/s2 (v1.18.4) - BSD-3-Clause - https://github.com/klauspost/compress/blob/v1.18.4/s2/LICENSE
github.com/klauspost/compress/snappy (v1.18.4) - BSD-3-Clause - https://github.com/klauspost/compress/blob/v1.18.4/snappy/LICENSE
github.com/klauspost/compress/zstd/internal/xxhash (v1.18.4) - MIT - https://github.com/klauspost/compress/blob/v1.18.4/zstd/internal/xxhash/LICENSE.txt

gh licenses prints the content of the license files discovered (above list). Since the the license file at the root is a mixed one and we print it all, two unused MIT licenses will also be printed (associated with paths s2/cmd/internal/filepathx/* and s2/cmd/internal/readahead/*). They're unused because we don't need those packages (under s2/cmd/) to build gh.

I think it's fine, but I'd like to have @williammartin confirmation on it as he's our licenses hero. 🦸

williammartin

It does bear a striking resemblance to me.

If the tests pass and there's nothing obviously concerning, I'm generally fine with dependency changes, otherwise we'll spend forever doing manual validation in response to dependabot. If something breaks, lesson learned!

No worries about the multi-licenses if they are MIT/BSD/Apache permissive licenses. Our license attribution is best-effort, the ecosystem makes it hard to do better. If someone takes an issue then we would try to work something out.

Thanks for the PR.

thaJeztah · 2026-03-31T10:29:00Z

😂 licensing is always tricky! You try to do the right thing, but it's complicated to navigate all the intricate details 😅 - thanks everyone!

thaJeztah requested a review from a team as a code owner March 26, 2026 22:02

Copilot AI review requested due to automatic review settings March 26, 2026 22:02

thaJeztah requested a review from a team as a code owner March 26, 2026 22:02

thaJeztah requested a review from williammartin March 26, 2026 22:02

github-actions bot added external pull request originating outside of the CLI core team needs-triage needs to be reviewed unmet-requirements labels Mar 26, 2026

github-actions bot removed the needs-triage needs to be reviewed label Mar 26, 2026

Copilot started reviewing on behalf of thaJeztah March 26, 2026 22:02 View session

Copilot AI reviewed Mar 26, 2026

View reviewed changes

D2758695161 mentioned this pull request Mar 27, 2026

feat: AI PR reviewer agent — claude-review CLI + GitHub Action (closes #4) claude-builders-bounty/claude-builders-bounty#89

Open

5 tasks

sungdark mentioned this pull request Mar 27, 2026

feat: AI PR reviewer agent — claude-review CLI + GitHub Action (closes #4) claude-builders-bounty/claude-builders-bounty#102

Closed

5 tasks

This comment was marked as spam.

Sign in to view

kommendorkapten approved these changes Mar 30, 2026

View reviewed changes

williammartin approved these changes Mar 31, 2026

View reviewed changes

williammartin merged commit 40da058 into cli:trunk Mar 31, 2026
21 of 22 checks passed

thaJeztah mentioned this pull request Mar 31, 2026

policy: replace golang/snappy with klauspost/compress/snappy docker/buildx#3686

Open

Conversation

thaJeztah commented Mar 26, 2026

Uh oh!

github-actions bot commented Mar 26, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

thaJeztah commented Mar 26, 2026

Uh oh!

andyfeller commented Mar 27, 2026

Uh oh!

kommendorkapten commented Mar 27, 2026

Uh oh!

malancas commented Mar 27, 2026

Uh oh!

thaJeztah commented Mar 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

malancas commented Mar 27, 2026

Uh oh!

thaJeztah commented Mar 27, 2026

Uh oh!

This comment was marked as spam.

Uh oh!

kommendorkapten left a comment

Choose a reason for hiding this comment

Uh oh!

babakks commented Mar 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

babakks commented Mar 30, 2026

Uh oh!

williammartin left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

thaJeztah commented Mar 31, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

thaJeztah commented Mar 27, 2026 •

edited

Loading

babakks commented Mar 30, 2026 •

edited

Loading

williammartin left a comment •

edited

Loading