Address PR #7 review feedback and add merge policy#8
Merged
Conversation
Fixes from Copilot review: - Generate legal file patterns programmatically to cover all variants (.md, .txt, .rst) consistently - Add proper os.Stat error handling (distinguish IsNotExist from other errors) in CopyLegalFiles and ExtractLegalFilesFromZip - Add zip-slip guard in ExtractLegalFilesFromZip - Check CopyFiltered error in test - Assert nested legal files are not extracted from zip archives - Expand isLegalFile test coverage for .rst and other variants Also adds PR merge policy to CLAUDE.md: always wait for CI checks and Copilot review comments before merging. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Updates legal-file detection/copy/extraction to address prior review feedback (correct pattern coverage, improved error handling, and safer zip extraction), and documents a PR merge policy.
Changes:
- Generate legal filename patterns programmatically (base-name × extension) and expand test coverage (incl.
.rstand ThirdPartyNotices variants). - Improve
os.Stathandling inCopyLegalFiles/ExtractLegalFilesFromZip(treat non-IsNotExisterrors as fatal) and add a zip-slip guard. - Strengthen tests to assert
CopyFilterederrors and ensure only root-level legal files are extracted.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| internal/packager/legalfiles.go | Generates legal file patterns via cross product; tightens os.Stat error handling; adds zip destination safety checks during extraction. |
| internal/packager/legalfiles_test.go | Expands isLegalFile positive cases, checks CopyFiltered error, and adds an extracted-file count assertion to prevent nested leakage. |
| CLAUDE.md | Adds an explicit PR/merge policy requiring CI + Copilot comment resolution before merge. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Use filepath.Rel-based containment check for zip-slip guard instead of string prefix comparison (handles filesystem root edge case) - Check os.ReadDir error in test assertion Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
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.
Summary
Addresses all 6 Copilot review comments from PR #7:
os.Staterror handling inCopyLegalFiles— distinguishesIsNotExistfrom other errors instead of treating all errors as "not present"os.Staterror handling inExtractLegalFilesFromZip— same fixExtractLegalFilesFromZip— usesfilepath.Base+filepath.Cleanand validates the destination stays withindestDirCopyFilterederror in test — now checkedAlso adds merge policy to CLAUDE.md: always wait for CI checks and Copilot review comments before merging.
Test plan
TestIsLegalFilecovers .rst and all new pattern variantsTestExtractLegalFilesFromZipasserts exactly 2 files extracted (no nested leaks)TestCopyLegalFiles_SurvivesExcludePatternsproperly checks CopyFiltered error🤖 Generated with Claude Code