Skip to content

feat(Issue 121): Adding LZ4 Support.#142

Merged
S4tvara merged 4 commits intoS4tvara:mainfrom
cjtaylor1990:issue121
Oct 27, 2025
Merged

feat(Issue 121): Adding LZ4 Support.#142
S4tvara merged 4 commits intoS4tvara:mainfrom
cjtaylor1990:issue121

Conversation

@cjtaylor1990
Copy link
Copy Markdown
Contributor

@cjtaylor1990 cjtaylor1990 commented Oct 23, 2025

Summary

Adding in lz4 dependency and adding it to the options in the prompts.

For the lz4 case, I have added the compression and decompression logic.

Unit tests are to be added for compression and decompression,
as well as decompression bombs.

Updating gzip and lz4 to fail at the max decompression + 1
byte, rather than at max + 2. This was a small bug found while testing.

AI Assistance

  • I used AI assistance (generate code, write tests, refactor, or docs)
    • If yes, briefly describe where and how: Github Copilot for auto-completion. No use of prompting.

Scope & Risk

  • Adds tests or updates existing tests
  • Security/privacy reviewed where applicable
  • Performance/complexity considered

Hacktoberfest Notes

  • Link the issue this PR resolves (required for consideration): Issue 121.
  • Explain the problem and your approach in your own words (1-3 sentences):

LZ4 is a fast compression algorithm that was not previously supported by Sietch. I used the suggested LZ4 library to compress and decompress chunks of data as they are being added to and fetched from Sietch vaults.

To check for decompression bombs, I originally decided to use the Size() method of the lz4 reader. However, it was not working as expected (returning 0 consistently), and as such, I decided to use the same pattern as gzip.

I have added unit tests, one set to check the compression/decompression in general, and then another for the decompression bomb case.

While testing, I did notice that the current implementation of using the extraByte len-1 array to detect decompression bombs in gzip and lz4 would actually result in detection at max size + 2 instead of the expected max size +1. As such, I've fixed that small bug while I was at it in both gzip and lz4.

Given many of the mocking methods in go are based upon dependency injection, I wasn't quite sure how I could cover our "unhappy path" cases in unit tests without further refactoring.

Checklist

  • PR title is descriptive
  • Commits are meaningful (no bulk AI dumps)
  • No generated files or lockfile-only changes unless justified (Justification: Adding a new dependency).

@codecov
Copy link
Copy Markdown

codecov bot commented Oct 23, 2025

Codecov Report

❌ Patch coverage is 64.00000% with 9 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
internal/compression/Compressor.go 71.42% 3 Missing and 3 partials ⚠️
internal/chunk/prompt.go 0.00% 3 Missing ⚠️

📢 Thoughts on this report? Let us know!

Adding lz4 dependency and adding
to the options for compression.

Adding lz4 logic in for decompression
and compression.

Unit tests still need to be written.
@S4tvara
Copy link
Copy Markdown
Owner

S4tvara commented Oct 26, 2025

Hi @cjtaylor1990 ,
Thanks for the contribution!
I've review it, just add tests rest lgtm!

@cjtaylor1990
Copy link
Copy Markdown
Contributor Author

Hi @cjtaylor1990 , Thanks for the contribution! I've review it, just add tests rest lgtm!

Thanks! Apologies for my slow speed. I'll get this in soon though.

Adding compression and decompression tests for
Compressor.go.

Tweaking logic so that decompression can't go over
the limit. Before, it would go to limit +2.
@cjtaylor1990 cjtaylor1990 changed the title feat(Issue 121): Initial implementation of LZ4. feat(Issue 121): Adding LZ4 Support. Oct 27, 2025
@cjtaylor1990 cjtaylor1990 marked this pull request as ready for review October 27, 2025 05:57
Compressor_test.go was failing linting due to typos. Fixed.
Adding zstd case for decom bomb unit test. Uncommenting
gzip decom bomb unit test, as that was left commented out
by accident.
@S4tvara S4tvara merged commit 6e36c9e into S4tvara:main Oct 27, 2025
62 of 66 checks passed
@S4tvara
Copy link
Copy Markdown
Owner

S4tvara commented Oct 27, 2025

@cjtaylor1990 ,
Thanks for your contribution xD

@cjtaylor1990 cjtaylor1990 deleted the issue121 branch October 27, 2025 23:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants