From f27abcb53e13d96edd9c64f2e658b5451c3c6bdd Mon Sep 17 00:00:00 2001 From: Eric Rakestraw Date: Sat, 10 Oct 2026 15:29:29 +0000 Subject: [PATCH] Read blob sizes only for selected validation inputs --- docs/internal/validation.md | 26 ++++++------ docs/roadmap/remediation.md | 15 ++++++- internal/git/source.go | 19 +++++---- internal/git/source_test.go | 14 ++++++- internal/validation/file_size.go | 20 ++++++--- internal/validation/file_size_test.go | 12 +++--- internal/validation/scope_checks_test.go | 2 +- internal/validation/tree_test.go | 6 +-- internal/validation/validation.go | 2 +- source_inspection_test.go | 53 ++++++++++++++++++++++++ 10 files changed, 129 insertions(+), 40 deletions(-) diff --git a/docs/internal/validation.md b/docs/internal/validation.md index ad3cf93..8e4e925 100644 --- a/docs/internal/validation.md +++ b/docs/internal/validation.md @@ -59,22 +59,24 @@ workspace-file check using the same candidate inventory. Resolved module bodies are parsed once per blob object ID; only owned filesystem-replacement targets and line numbers remain cached. Each declaration path receives findings from that summary in its own path context. -The optional file-size check uses each blob's captured Git size, including -symlink link-text bytes, and skips entries without a blob size such as gitlinks. -It applies normalized literal file/directory exclusions with path-boundary -matching, then returns the first oversized path with `FileSizeMeasurement`. +The optional file-size check applies normalized literal file/directory +exclusions with path-boundary matching, then requests local Git sizes for the +remaining selected blobs. Sizes include symlink link-text bytes; gitlinks are +omitted. The check returns the first oversized path with `FileSizeMeasurement`. The optional Markdown check follows file-size inspection and uses the same candidate tree. It parses regular Markdown blobs, resolves local destinations, and checks supported anchors; outside-checkout and submodule observations are written to the shared output writer. A nil writer keeps library use quiet. -`CandidateTree` gets metadata and bytes from Git objects, reads blob bytes -lazily, and resolves destinations without consulting the worktree or -neighboring files. Each tree owns one lazy Git blob reader; direct tree users -must close it when finished. The shared validation executor closes its tree on -every return before product checks or build callbacks run. A metadata-only -selection never starts the reader. Per-read contexts cancel an in-flight blob -exchange. The context supplied when creating the tree governs the reader -lifetime, including shutdown and child reaping. +`CandidateTree` enumerates full repository path, type, mode, and object metadata +without requiring blob contents or sizes. Checks acquire sizes and bytes for +their selected inputs and required references. Unreferenced blobs in excluded +modules remain outside those reads. Destinations resolve without consulting +the worktree or neighboring files. Each tree owns one lazy Git blob reader; +direct tree users must close it when finished. The shared validation executor +closes its tree on every return before product checks or build callbacks run. +A metadata-only selection never starts the reader. Per-read contexts cancel +an in-flight blob exchange. The context supplied when creating the tree governs +the reader lifetime, including shutdown and child reaping. Regular-file reads return independent caller-owned byte slices and are not retained by the tree. Symlink targets are retained as immutable owned strings by blob object ID; explicitly reading a symlink still returns independent diff --git a/docs/roadmap/remediation.md b/docs/roadmap/remediation.md index 46ad372..33798e0 100644 --- a/docs/roadmap/remediation.md +++ b/docs/roadmap/remediation.md @@ -1,6 +1,6 @@ # Remediation: module release implementation review -Status: open; issues 1–14 are complete, and issue 15 requires remediation. +Status: complete; all fifteen findings addressed and verified. The initial review examined `90559fe`, compared with planning baseline `dbfae9e`. The follow-up repository audit examined `6b0b8ce`, including those corrections. @@ -544,7 +544,18 @@ network-capable preparation must continue to work. ## Issue 15: Limit blob availability requirements to selected validation inputs (P2) -Status: open. Category: module-scope leakage through eager metadata enrichment. +Status: complete. Category: module-scope leakage through eager metadata enrichment. + +**Completion:** Tree enumeration now acquires path/type/mode/object metadata +independently of blobs. The size check requests sizes only for selected, +non-excluded blobs; other checks read contents and references when needed. +Filtered sparse-clone regressions cover formatting, sizes, workspace metadata, +explicit shared selection, exclusions, required shared contents, and required +Markdown references. They failed before the correction and now preserve each +check's scope and error identity without transport or object-database changes. +Existing size, symlink, gitlink, vendor, boundary, reader/cache, and cancellation +tests pass. The complete offline suite, compiled API/dependency check, +documentation links, and whitespace checks pass for all four follow-up fixes. **Evidence:** [ReadCandidateTreeForScope](../../internal/validation/tree.go) calls [ReadTree](../../internal/git/source.go) before constructing module and diff --git a/internal/git/source.go b/internal/git/source.go index 102c313..7fc9fa0 100644 --- a/internal/git/source.go +++ b/internal/git/source.go @@ -163,8 +163,9 @@ func CommittedPaths(repositoryDir, commit string) PathResolver { } } -// ReadTree enumerates all entries in a commit and records blob sizes. Paths -// are NUL-delimited by Git so whitespace in a filename is preserved. +// ReadTree enumerates path, type, mode, and object metadata for a commit. +// Blob sizes and contents are inspected separately. Paths are NUL-delimited +// by Git so whitespace in a filename is preserved. func ReadTree(ctx context.Context, repositoryDir, commit string) ([]TreeEntry, error) { if !validObjectID(commit) { return nil, fmt.Errorf("invalid commit object ID %q", commit) @@ -177,9 +178,6 @@ func ReadTree(ctx context.Context, repositoryDir, commit string) ([]TreeEntry, e if err != nil { return nil, fmt.Errorf("parse committed tree %s: %w", commit, err) } - if err := populateBlobSizes(ctx, repositoryDir, entries); err != nil { - return nil, fmt.Errorf("read committed blob sizes for %s: %w", commit, err) - } return entries, nil } @@ -223,11 +221,16 @@ func parseTreeEntries(output []byte) ([]TreeEntry, error) { return entries, nil } -func populateBlobSizes(ctx context.Context, repositoryDir string, entries []TreeEntry) error { +// PopulateBlobSizes fills unknown sizes for the supplied blob entries using +// local objects. Callers select the entries whose sizes their check needs. +func PopulateBlobSizes(ctx context.Context, repositoryDir string, entries []TreeEntry) error { + if err := ctx.Err(); err != nil { + return err + } var blobIndexes []int var input strings.Builder for index := range entries { - if entries[index].Type != "blob" { + if entries[index].Type != "blob" || entries[index].HasSize { continue } blobIndexes = append(blobIndexes, index) @@ -250,7 +253,7 @@ func populateBlobSizes(ctx context.Context, repositoryDir string, entries []Tree fields := strings.Fields(line) entry := &entries[blobIndexes[index]] if len(fields) == 2 && fields[0] == entry.ObjectID && fields[1] == "missing" { - return fmt.Errorf("committed blob %s is unavailable locally", entry.ObjectID) + return fmt.Errorf("committed blob %s for %q is unavailable locally", entry.ObjectID, entry.Path) } if len(fields) != 3 || fields[0] != entry.ObjectID || fields[1] != "blob" { return fmt.Errorf("unexpected blob size record %q", line) diff --git a/internal/git/source_test.go b/internal/git/source_test.go index 0f71e95..2bb59bb 100644 --- a/internal/git/source_test.go +++ b/internal/git/source_test.go @@ -85,8 +85,18 @@ func TestCandidateObjectReadsStayLocalInPartialClones(t *testing.T) { t.Setenv("RELEASEKIT_GIT_ATTEMPTS", attempts) t.Setenv("GIT_NO_LAZY_FETCH", "0") t.Setenv("GIT_ALLOW_PROTOCOL", "ssh") - if _, err := ReadTree(t.Context(), repository, fixture.commit); err == nil { - t.Fatal("tree inspection succeeded with unavailable objects") + entries, treeErr := ReadTree(t.Context(), repository, fixture.commit) + if filter == "tree:0" { + if treeErr == nil { + t.Fatal("tree inspection succeeded with unavailable trees") + } + } else { + if treeErr != nil { + t.Fatalf("metadata inspection required unavailable blobs: %v", treeErr) + } + if err := PopulateBlobSizes(t.Context(), repository, entries); err == nil { + t.Fatal("size inspection succeeded with unavailable blobs") + } } _, fileErr := ReadCommitFile(t.Context(), repository, fixture.commit, "source.txt") var fileExitError *exec.ExitError diff --git a/internal/validation/file_size.go b/internal/validation/file_size.go index dedd793..daa54fe 100644 --- a/internal/validation/file_size.go +++ b/internal/validation/file_size.go @@ -1,21 +1,31 @@ package validation import ( + "context" + "fmt" "path" "strings" + + "gitea.maximumdirect.net/eric/ReleaseKit/internal/git" ) const defaultFileSizeLimit int64 = 1_000_000 -func checkFileSize(tree *CandidateTree, settings *FileSizeSettings) error { +func checkFileSize(ctx context.Context, tree *CandidateTree, settings *FileSizeSettings) error { + entries := make([]git.TreeEntry, 0, len(tree.inputEntries)) + for _, entry := range tree.inputEntries { + if entry.Type == "blob" && !excludedByPath(entry.Path, settings.Exclusions) { + entries = append(entries, entry) + } + } + if err := git.PopulateBlobSizes(ctx, tree.repositoryDir, entries); err != nil { + return &Failure{Check: FileSize, Cause: fmt.Errorf("inspect selected committed file sizes: %w", err)} + } limit := defaultFileSizeLimit if settings.LimitBytes != nil { limit = *settings.LimitBytes } - for _, entry := range tree.inputEntries { - if entry.Type != "blob" || !entry.HasSize || excludedByPath(entry.Path, settings.Exclusions) { - continue - } + for _, entry := range entries { if entry.Size > limit { return &Failure{ Check: FileSize, diff --git a/internal/validation/file_size_test.go b/internal/validation/file_size_test.go index 6d7d815..84509f8 100644 --- a/internal/validation/file_size_test.go +++ b/internal/validation/file_size_test.go @@ -33,7 +33,7 @@ func TestCheckFileSizeUsesStrictLimitsAndDefault(t *testing.T) { Path: "file.bin", Type: "blob", Mode: "100644", Size: test.size, HasSize: true, }} tree := &CandidateTree{entries: entries, inputEntries: entries} - err := checkFileSize(tree, &FileSizeSettings{LimitBytes: test.limit}) + err := checkFileSize(t.Context(), tree, &FileSizeSettings{LimitBytes: test.limit}) if !test.wantError { if err != nil { t.Fatalf("checkFileSize() error = %v, want nil", err) @@ -109,7 +109,7 @@ func TestCheckFileSizeAppliesLiteralPathExclusions(t *testing.T) { LimitBytes: int64Pointer(10), Exclusions: test.exclusions, }, ".") - err := checkFileSize(&CandidateTree{entries: test.entries, inputEntries: test.entries}, settings) + err := checkFileSize(t.Context(), &CandidateTree{entries: test.entries, inputEntries: test.entries}, settings) failure, ok := err.(*Failure) if !ok || failure.Check != FileSize || len(failure.Findings) != 1 || failure.Findings[0].Path != test.wantPath { t.Fatalf("checkFileSize() error = %v, want only %q to exceed the limit", err, test.wantPath) @@ -139,12 +139,12 @@ func TestCheckFileSizeUsesCommittedBlobSizes(t *testing.T) { } cleanupCandidateTree(t, tree) - err = checkFileSize(tree, &FileSizeSettings{LimitBytes: int64Pointer(3), Exclusions: []string{"link"}}) + err = checkFileSize(t.Context(), tree, &FileSizeSettings{LimitBytes: int64Pointer(3), Exclusions: []string{"link"}}) if err != nil { t.Fatalf("checkFileSize() error = %v, want small binary and exact-limit blob to pass", err) } - err = checkFileSize(tree, &FileSizeSettings{LimitBytes: int64Pointer(3), Exclusions: []string{"small.bin", "boundary.bin"}}) + err = checkFileSize(t.Context(), tree, &FileSizeSettings{LimitBytes: int64Pointer(3), Exclusions: []string{"small.bin", "boundary.bin"}}) failure, ok := err.(*Failure) if !ok || failure.Check != FileSize || len(failure.Findings) != 1 || failure.Findings[0].Path != "link" { t.Fatalf("checkFileSize() error = %v, want symlink blob to exceed the limit", err) @@ -153,7 +153,7 @@ func TestCheckFileSizeUsesCommittedBlobSizes(t *testing.T) { t.Fatalf("symlink measurement = %#v, want link-text size %d", failure.Measurement, len("small.bin")) } - err = checkFileSize(tree, &FileSizeSettings{LimitBytes: int64Pointer(0), Exclusions: []string{"small.bin", "boundary.bin", "link"}}) + err = checkFileSize(t.Context(), tree, &FileSizeSettings{LimitBytes: int64Pointer(0), Exclusions: []string{"small.bin", "boundary.bin", "link"}}) if err != nil { t.Fatalf("checkFileSize() error = %v, want omitted gitlink size", err) } @@ -174,7 +174,7 @@ func TestCheckFileSizeUsesOnlyCandidateFilesIndividually(t *testing.T) { t.Fatal(err) } cleanupCandidateTree(t, tree) - if err := checkFileSize(tree, &FileSizeSettings{LimitBytes: int64Pointer(10)}); err != nil { + if err := checkFileSize(t.Context(), tree, &FileSizeSettings{LimitBytes: int64Pointer(10)}); err != nil { t.Fatalf("checkFileSize() error = %v, want current individual files to pass", err) } } diff --git a/internal/validation/scope_checks_test.go b/internal/validation/scope_checks_test.go index 6576b93..4b88557 100644 --- a/internal/validation/scope_checks_test.go +++ b/internal/validation/scope_checks_test.go @@ -98,7 +98,7 @@ func TestFileSizeExclusionsAreModuleRelative(t *testing.T) { } cleanupCandidateTree(t, tree) settings := fileSizeSettingsForModule(&FileSizeSettings{LimitBytes: int64Pointer(10), Exclusions: []string{"./dist/"}}, tree.modulePath) - err = checkFileSize(tree, settings) + err = checkFileSize(t.Context(), tree, settings) var failure *Failure if !errors.As(err, &failure) || failure.Check != FileSize || len(failure.Findings) != 1 || failure.Findings[0].Path != "nested/dist-old/output.bin" { t.Fatalf("module-relative file-size exclusion error = %v, want only nested/dist-old/output.bin", err) diff --git a/internal/validation/tree_test.go b/internal/validation/tree_test.go index 26948ee..2924e43 100644 --- a/internal/validation/tree_test.go +++ b/internal/validation/tree_test.go @@ -48,10 +48,10 @@ func TestReadCandidateTreeUsesCapturedCommitAndEnumeratesGitEntries(t *testing.T t.Errorf("committed tree is missing %q", path) } } - if got := entries["binary.dat"]; got.size != 4 || !got.hasSize { - t.Errorf("binary metadata = %#v, want size 4", got) + if got := entries["binary.dat"]; got.mode != "100644" || got.kind != "blob" { + t.Errorf("binary metadata = %#v", got) } - if got := entries["readme-link"]; got.mode != "120000" || got.kind != "blob" || !got.hasSize { + if got := entries["readme-link"]; got.mode != "120000" || got.kind != "blob" { t.Errorf("symlink metadata = %#v", got) } if got := entries["submodule"]; got.mode != "160000" || got.kind != "commit" || got.hasSize { diff --git a/internal/validation/validation.go b/internal/validation/validation.go index 35c032b..8b75526 100644 --- a/internal/validation/validation.go +++ b/internal/validation/validation.go @@ -190,7 +190,7 @@ func Run(ctx context.Context, request Request) (result error) { } } if request.Settings.FileSize != nil { - if err := checkFileSize(tree, fileSizeSettings); err != nil { + if err := checkFileSize(ctx, tree, fileSizeSettings); err != nil { return err } } diff --git a/source_inspection_test.go b/source_inspection_test.go index 2aca7f8..393831f 100644 --- a/source_inspection_test.go +++ b/source_inspection_test.go @@ -46,6 +46,59 @@ func TestSourceOperationsRejectUnavailableNoteWithoutTransport(t *testing.T) { assertInspectionStayedLocal(t, repository, attempts, before) } +func TestScopedValidationReadsOnlyRequiredBlobsInPartialClone(t *testing.T) { + source := newPublishFixture(t) + addNestedReleaseModule(t, source.work, "nested", "example.com/nested", "v1.2.3") + writeFile(t, source.work, "nested/NOTICE.txt", "nested notice\n") + writeFile(t, source.work, "nested/README.md", "# Nested guide\n") + writeFile(t, source.work, "nested/go.work", "go 1.26.0\nuse .\n") + writeFile(t, source.work, "docs/links.md", "# Links\n\n[Guide](../nested/README.md#nested-guide)\n") + gitRun(t, source.work, "add", ".") + gitRun(t, source.work, "commit", "-m", "Add independently scoped module") + repository := newFilteredSparseCheckout(t, source.work, + "/go.mod", "/fixture_test.go", "/race_fixture_test.go", "/source.txt", "/docs/") + before := gitText(t, repository, "cat-file", "--batch-all-objects", "--batch-check=%(objectname)") + for _, name := range []string{"nested/fixture_test.go", "nested/NOTICE.txt", "nested/README.md"} { + object := gitText(t, repository, "rev-parse", "HEAD:"+name) + if strings.Contains(before, object) { + t.Fatalf("fixture already contains excluded blob %q", name) + } + } + attempts := forbidInspectionTransport(t, repository) + for _, tc := range []struct { + name string + selection releasekit.ValidationSpec + wantCheck releasekit.ValidationCheck + wantPath string + }{ + {name: "formatting", selection: releasekit.ValidationSpec{GoFormat: true, SharedFiles: []string{}}}, + {name: "file sizes", selection: releasekit.ValidationSpec{FileSize: &releasekit.FileSizeValidationSpec{}, SharedFiles: []string{}}}, + {name: "workspace metadata", selection: releasekit.ValidationSpec{ModuleDistribution: releasekit.ModuleDistributionValidationSpec{WorkspaceFiles: true}, SharedFiles: []string{}}}, + {name: "explicit shared metadata", selection: releasekit.ValidationSpec{SharedFiles: []string{"nested/NOTICE.txt"}}}, + {name: "excluded shared size", selection: releasekit.ValidationSpec{FileSize: &releasekit.FileSizeValidationSpec{Exclusions: []string{"nested/NOTICE.txt"}}, SharedFiles: []string{"nested/NOTICE.txt"}}}, + {name: "required shared size", selection: releasekit.ValidationSpec{FileSize: &releasekit.FileSizeValidationSpec{}, SharedFiles: []string{"nested/NOTICE.txt"}}, wantCheck: "file-size", wantPath: "nested/NOTICE.txt"}, + {name: "required shared source", selection: releasekit.ValidationSpec{GoFormat: true, SharedFiles: []string{"nested/fixture_test.go"}}, wantCheck: "go-format", wantPath: "nested/fixture_test.go"}, + {name: "required reference", selection: releasekit.ValidationSpec{Markdown: &releasekit.MarkdownValidationSpec{}, SharedFiles: []string{}}, wantCheck: "markdown-links", wantPath: "nested/README.md"}, + } { + t.Run(tc.name, func(t *testing.T) { + _, err := releasekit.Check(t.Context(), releasekit.ReleaseRequest{ + RepositoryDir: repository, Product: "Sample App", Version: "v1.2.3", Validation: tc.selection, + }) + if tc.wantCheck == "" { + if err != nil { + t.Fatalf("scoped validation failed: %v", err) + } + } else { + var failure *releasekit.ValidationError + if !errors.As(err, &failure) || failure.Check != tc.wantCheck || !strings.Contains(err.Error(), tc.wantPath) { + t.Fatalf("validation error = %v; want %s inspection of %q", err, tc.wantCheck, tc.wantPath) + } + } + assertInspectionStayedLocal(t, repository, attempts, before) + }) + } +} + func newFilteredSparseCheckout(t *testing.T, source string, paths ...string) string { t.Helper() gitRun(t, source, "config", "uploadpack.allowFilter", "true")