Always include license and readme files in packages#7
Conversation
Copies LICENSE, README, NOTICE, and similar files to the package root regardless of exclude patterns. For git and archive sources, copies from the source directory. For NuGet packages, extracts from the nupkg archive root. Existing files are not overwritten. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
When a subpath is configured (e.g., csharp/src/Google.Protobuf), the license file is typically at the repo root. CopyLegalFilesSearchingUp walks from the source dir up to the clone/archive root to find legal files. Closer files take precedence over those found higher up. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR ensures common legal/documentation files (license/readme/notice) are always included at the packaged output root across all supported package sources (git, archive, NuGet), even when exclude patterns would otherwise filter them out.
Changes:
- Add
CopyLegalFilesandExtractLegalFilesFromZiputilities to copy/extract recognized legal files into the package root without overwriting existing files. - Invoke these utilities during Git (unity/raw), archive, and NuGet packaging flows.
- Add unit tests covering filename recognition, copy behavior, overwrite protection, exclude-pattern survival, and zip-root extraction.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/packager/legalfiles.go | Introduces legal file detection plus copy/extract helpers used by packager flows. |
| internal/packager/legalfiles_test.go | Adds tests for legal file matching, copying, and zip extraction behavior. |
| internal/packager/git.go | Calls CopyLegalFiles after filtered copy to ensure legal files are present at package root. |
| internal/packager/archive.go | Calls CopyLegalFiles for both Unity and raw archive packaging modes. |
| internal/packager/nuget.go | Calls ExtractLegalFilesFromZip to pull legal files from the nupkg archive root. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if parent == current { | ||
| break | ||
| } | ||
| current = parent | ||
| } |
There was a problem hiding this comment.
Same issue as CopyLegalFiles: the os.Stat overwrite check treats any error as “not present”. If Stat fails for reasons other than IsNotExist, return that error rather than continuing to create/truncate the file.
| os.WriteFile(filepath.Join(srcDir, "README.md"), []byte("readme"), 0644) | ||
|
|
||
| // CopyFiltered with *.md excluded — README.md goes to Runtime but gets excluded | ||
| CopyFiltered(srcDir, runtimeDir, []string{"*.md"}) |
There was a problem hiding this comment.
CopyFiltered returns an error, but this test ignores it. If CopyFiltered fails, the test can pass/fail for the wrong reason. Capture and assert the returned error (like other tests in this package do).
| CopyFiltered(srcDir, runtimeDir, []string{"*.md"}) | |
| if err := CopyFiltered(srcDir, runtimeDir, []string{"*.md"}); err != nil { | |
| t.Fatalf("CopyFiltered: %v", err) | |
| } |
| os.MkdirAll(subPath, 0755) | ||
|
|
||
| // LICENSE at both levels | ||
| os.WriteFile(filepath.Join(repoRoot, "LICENSE"), []byte("root license"), 0644) | ||
| os.WriteFile(filepath.Join(subPath, "LICENSE"), []byte("sub license"), 0644) | ||
|
|
There was a problem hiding this comment.
The test comment says the nested lib/netstandard2.0/LICENSE should not be extracted, but the test never asserts that it’s absent. Add an explicit os.Stat check for destDir/"LICENSE" (or whatever basename would result) to ensure nested legal files aren’t accidentally extracted in the future.
| var legalFilePatterns = []string{ | ||
| "license", | ||
| "licence", | ||
| "license.md", | ||
| "licence.md", | ||
| "license.txt", | ||
| "licence.txt", | ||
| "license.rst", | ||
| "licence.rst", | ||
| "readme", | ||
| "readme.md", | ||
| "readme.txt", | ||
| "readme.rst", | ||
| "notice", | ||
| "notice.md", | ||
| "notice.txt", | ||
| "third-party-notices", | ||
| "third-party-notices.md", | ||
| "third-party-notices.txt", | ||
| "thirdpartynotices", | ||
| "thirdpartynotices.txt", | ||
| } |
There was a problem hiding this comment.
legalFilePatterns doesn’t fully match the filenames/extensions promised in the PR description (e.g., NOTICE.rst, ThirdPartyNotices.md, ThirdPartyNotices.rst, and third-party-notices.rst are missing). This will cause some legal files to be skipped. Expand the pattern list (or generate patterns programmatically) so all documented variants are recognized consistently.
| // Collect directories from srcDir up to rootDir | ||
| var dirs []string | ||
| current := srcAbs | ||
| for { | ||
| dirs = append(dirs, current) | ||
| if current == rootAbs { | ||
| break | ||
| } | ||
| parent := filepath.Dir(current) | ||
| if parent == current { |
There was a problem hiding this comment.
ExtractLegalFilesFromZip joins destDir with f.Name without a zip-slip/path traversal guard. The current root-level check only looks for /, so names like ..\\LICENSE could escape destDir on Windows. Add a robust validation (e.g., reject any path separators, clean and verify the final path stays within destDir, similar to extractZip), before creating the output file.
| if _, err := os.Stat(destPath); err == nil { | ||
| continue |
There was a problem hiding this comment.
The overwrite check uses os.Stat(destPath) and treats any non-nil error as “file does not exist”. If Stat fails for reasons other than non-existence (permissions, transient IO errors), the code will proceed and likely fail later or mask the real error. Handle err != nil && !os.IsNotExist(err) by returning the error.
| if _, err := os.Stat(destPath); err == nil { | |
| continue | |
| if _, err := os.Stat(destPath); err == nil { | |
| // File already exists at destPath; skip copying. | |
| continue | |
| } else if !os.IsNotExist(err) { | |
| // An unexpected error occurred while checking destPath; surface it. | |
| return err |
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>
Address PR #7 review feedback and add merge policy
Summary
Test plan
TestIsLegalFile— pattern matching for known filenamesTestCopyLegalFiles— copies legal files, ignores othersTestCopyLegalFiles_DoesNotOverwrite— preserves existing filesTestCopyLegalFiles_SurvivesExcludePatterns— legal files appear at package root even when*.mdis excludedTestExtractLegalFilesFromZip— extracts root-level legal files from nupkg, ignores nested ones🤖 Generated with Claude Code