Merge PR #764: perf: fix top 5 high-performance-go.md violations found by codebase audit

Author: merge-queue-bot

internal/corpus/collect.go

@@ -9,6 +9,8 @@ import (
 	"path/filepath"
 	"strings"
 	"unicode/utf8"
+
+	"github.com/jeduden/mdsmith/internal/bytelimit"
 )
 
 // Collect gathers markdown records from configured sources.
@@ -174,9 +176,35 @@ func collectFile(
 		return Record{}, false, nil
 	}
 
-	content, err := os.ReadFile(fullPath)
+	// Skip a file this function cannot read — oversized, vanished, or
+	// permission-denied — rather than failing the whole source:
+	// collectFromRoot's caller aborts the entire walk (and every record
+	// already collected from every source before it, since Collect
+	// returns on the first error) on any error from this function. A
+	// cloned third-party repository can legitimately contain one
+	// oversized or racy file (a large CHANGELOG, a vendored spec, a file
+	// removed mid-walk) without that file being reason to discard the
+	// rest of the corpus build.
+	info, err := os.Stat(fullPath)
+	if err != nil {
+		reportProgress(cfg, fmt.Sprintf("skipping %s: %v", relPath, err))
+		return Record{}, false, nil
+	}
+	if info.Size() > bytelimit.DefaultMaxInputBytes {
+		reportProgress(cfg, fmt.Sprintf(
+			"skipping %s: %d bytes exceeds the %d byte limit",
+			relPath, info.Size(), bytelimit.DefaultMaxInputBytes))
+		return Record{}, false, nil
+	}
+
+	// bytelimit.ReadFileLimited re-checks the size on the actual read,
+	// so a file that grows past the cap in the window between the Stat
+	// above and this call (or otherwise becomes unreadable) is caught
+	// here too — skipped the same way, not treated as fatal.
+	content, err := bytelimit.ReadFileLimited(fullPath, bytelimit.DefaultMaxInputBytes)
 	if err != nil {
-		return Record{}, false, fmt.Errorf("read file %s: %w", fullPath, err)
+		reportProgress(cfg, fmt.Sprintf("skipping %s: %v", relPath, err))
+		return Record{}, false, nil
 	}
 	raw := normalizeContent(string(content))
 	words := countWords(raw)

internal/corpus/collect_test.go

@@ -1,9 +1,12 @@
 package corpus
 
 import (
+	"bytes"
 	"os"
 	"path/filepath"
 	"testing"
+
+	"github.com/jeduden/mdsmith/internal/bytelimit"
 )
 
 func TestCollect_HappyPath(t *testing.T) {
@@ -207,6 +210,144 @@ func TestCollect_ErrorPath(t *testing.T) {
 	}
 }
 
+// TestCollect_OversizedFile_SkippedNotFatal guards against an unbounded
+// os.ReadFile on a corpus source: collectFile ingests markdown from
+// cloned third-party repositories, which are untrusted input
+// (docs/development/high-performance-go.md — "os.ReadFile on huge
+// inputs: one giant alloc, all resident"). A file over the shared
+// bytelimit.DefaultMaxInputBytes cap must be skipped rather than read
+// into memory in full — and, since a real source repository can contain
+// one large file among many good ones (a big CHANGELOG, a vendored
+// spec), skipping it must not abort collection of the rest of that
+// source, or of sources collected *earlier* in the same run (Collect's
+// loop over cfg.Sources returns on the first error, discarding every
+// record gathered so far — so this uses two sources, not one, to prove
+// the first source's record survives a later source's oversized file).
+func TestCollect_OversizedFile_SkippedNotFatal(t *testing.T) {
+	t.Parallel()
+
+	const prose = "# Title\n\nword word word word word word\n"
+
+	goodRoot := filepath.Join(t.TempDir(), "good")
+	mustMkdirAll(t, goodRoot)
+	mustWriteFile(t, filepath.Join(goodRoot, "early.md"), []byte(prose))
+
+	mixedRoot := filepath.Join(t.TempDir(), "mixed")
+	mustMkdirAll(t, mixedRoot)
+	oversized := bytes.Repeat([]byte("a "), int(bytelimit.DefaultMaxInputBytes)/2+1)
+	mustWriteFile(t, filepath.Join(mixedRoot, "huge.md"), oversized)
+	mustWriteFile(t, filepath.Join(mixedRoot, "normal.md"), []byte(prose))
+
+	cfg := &Config{
+		CollectedAt:      "2026-02-16",
+		MinWords:         1,
+		MinChars:         1,
+		LicenseAllowlist: []string{"MIT"},
+		Sources: []SourceConfig{
+			{
+				Name:       "early",
+				Repository: "github.com/acme/early",
+				Root:       goodRoot,
+				CommitSHA:  "abc123",
+				License:    "MIT",
+			},
+			{
+				Name:       "mixed",
+				Repository: "github.com/acme/mixed",
+				Root:       mixedRoot,
+				CommitSHA:  "def456",
+				License:    "MIT",
+			},
+		},
+	}
+
+	records, err := Collect(cfg, t.TempDir())
+	if err != nil {
+		t.Fatalf("Collect: unexpected error, oversized file should be skipped: %v", err)
+	}
+	if len(records) != 2 {
+		t.Fatalf("record count = %d, want 2 (early.md from the first source, "+
+			"normal.md from the second; huge.md must be skipped)", len(records))
+	}
+	paths := make([]string, len(records))
+	for i, r := range records {
+		paths[i] = r.Source + "/" + r.Path
+	}
+	if paths[0] != "early/early.md" || paths[1] != "mixed/normal.md" {
+		t.Fatalf("records = %v, want [early/early.md mixed/normal.md]", paths)
+	}
+}
+
+// mustMkdirAll creates dir and all parents, failing the test on error.
+func mustMkdirAll(t *testing.T, dir string) {
+	t.Helper()
+	if err := os.MkdirAll(dir, 0o755); err != nil {
+		t.Fatalf("mkdir %s: %v", dir, err)
+	}
+}
+
+// mustWriteFile writes content to path, failing the test on error.
+func mustWriteFile(t *testing.T, path string, content []byte) {
+	t.Helper()
+	if err := os.WriteFile(path, content, 0o644); err != nil {
+		t.Fatalf("write %s: %v", path, err)
+	}
+}
+
+// TestCollectFile_StatError_SkippedNotFatal covers collectFile's os.Stat
+// error branch directly: a file that vanishes between WalkDir listing it
+// and the Stat call inside collectFile (or any other stat failure) must
+// be skipped, not treated as fatal — the same reasoning as the oversized-
+// file case above.
+func TestCollectFile_StatError_SkippedNotFatal(t *testing.T) {
+	t.Parallel()
+
+	root := t.TempDir()
+	missing := filepath.Join(root, "gone.md")
+
+	cfg := &Config{MinWords: 1, MinChars: 1}
+	record, keep, err := collectFile(cfg, SourceConfig{Name: "seed"}, missing, "gone.md", root)
+	if err != nil {
+		t.Fatalf("collectFile: unexpected error for a stat failure: %v", err)
+	}
+	if keep {
+		t.Fatal("collectFile: keep = true, want false for a stat failure")
+	}
+	if record != (Record{}) {
+		t.Fatalf("collectFile: record = %+v, want zero value", record)
+	}
+}
+
+// TestCollectFile_ReadError_SkippedNotFatal covers collectFile's fallback
+// bytelimit.ReadFileLimited error branch directly: a path that passes the
+// Stat-based size pre-check but then fails to read must be skipped, not
+// treated as fatal — the same reasoning as the stat-failure and
+// oversized-file cases above. A directory Stats successfully (size 0,
+// under the cap) but fails to Read as a file ("is a directory"),
+// deterministically reaching this branch regardless of the running
+// user's privileges (unlike a permission-bit test, which root ignores).
+func TestCollectFile_ReadError_SkippedNotFatal(t *testing.T) {
+	t.Parallel()
+
+	root := t.TempDir()
+	notAFile := filepath.Join(root, "not-a-file.md")
+	if err := os.Mkdir(notAFile, 0o755); err != nil {
+		t.Fatalf("mkdir: %v", err)
+	}
+
+	cfg := &Config{MinWords: 1, MinChars: 1}
+	record, keep, err := collectFile(cfg, SourceConfig{Name: "seed"}, notAFile, "not-a-file.md", root)
+	if err != nil {
+		t.Fatalf("collectFile: unexpected error reading a directory as a file: %v", err)
+	}
+	if keep {
+		t.Fatal("collectFile: keep = true, want false when the read fails")
+	}
+	if record != (Record{}) {
+		t.Fatalf("collectFile: record = %+v, want zero value", record)
+	}
+}
+
 // --- reportProgress ---
 
 // TestReportProgress pins all three branches: nil cfg is a no-op,

internal/linkgraph/linkgraph.go

@@ -251,10 +251,10 @@ func linkPosition(f *lint.File, n ast.Node) (int, int) {
 	if offset < 0 {
 		return 1, 1
 	}
-	// f.ColumnOfOffset scans backward from offset to the previous
-	// newline, so it's O(column) per call instead of the O(offset)
-	// forward scan a hand-rolled version would do — meaningful for
-	// `mdsmith list backlinks` which can call this many times per file.
+	// f.ColumnOfOffset binary-searches the cached newline index, so
+	// it's O(log lines) per call instead of the O(column) backward scan
+	// a hand-rolled version would do — meaningful for `mdsmith list
+	// backlinks` which can call this many times per file.
 	return f.LineOfOffset(offset), f.ColumnOfOffset(offset)
 }
 

internal/lint/column_bench_test.go

@@ -0,0 +1,24 @@
+package lint
+
+import (
+	"bytes"
+	"testing"
+)
+
+// BenchmarkColumnOfOffset_LongLine exercises the former worst case for
+// ColumnOfOffset: an offset near the end of a very long line, which used
+// to force a full backward byte-at-a-time scan to find the line start.
+// ColumnOfOffset now reuses LineOfOffset's cached newline index via a
+// binary search instead (docs/development/high-performance-go.md).
+func BenchmarkColumnOfOffset_LongLine(b *testing.B) {
+	line := bytes.Repeat([]byte("x"), 8192)
+	src := append(append([]byte("prefix\n"), line...), '\n')
+	f := &File{Source: src}
+	offset := len(src) - 2 // last byte of the long line
+
+	b.ReportAllocs()
+	b.ResetTimer()
+	for i := 0; i < b.N; i++ {
+		_ = f.ColumnOfOffset(offset)
+	}
+}

internal/lint/file.go

@@ -562,23 +562,9 @@ var lineIndexNewline = []byte{'\n'}
 // strictly before offset (a newline exactly at offset starts the
 // next line, so it does not count) — identical to a linear scan,
 // but O(log n) via binary search over the cached newline index.
-// The search is inlined (sort.Search would force the comparison
-// callback to capture `nl` and `offset` and escape to the heap;
-// engine-bench profiling attributed ~64 k allocations per
-// 10-iteration run to that closure box before plan 195 inlined
-// the binary search here).
 func (f *File) LineOfOffset(offset int) int {
 	nl := f.lineIndex()
-	lo, hi := 0, len(nl)
-	for lo < hi {
-		mid := int(uint(lo+hi) >> 1)
-		if nl[mid] >= offset {
-			hi = mid
-		} else {
-			lo = mid + 1
-		}
-	}
-	return 1 + lo
+	return 1 + newlineSearch(nl, offset)
 }
 
 // ColumnOfOffset converts a byte offset in Source to a 1-based column
@@ -587,9 +573,38 @@ func (f *File) ColumnOfOffset(offset int) int {
 	if offset > len(f.Source) {
 		offset = len(f.Source)
 	}
-	start := offset
-	for start > 0 && f.Source[start-1] != '\n' {
-		start--
+	if offset < 0 {
+		offset = 0
+	}
+	// Reuses LineOfOffset's cached newline index via the same binary
+	// search instead of scanning backward from offset byte by byte —
+	// O(log n) in the newline count instead of O(line length). A single
+	// very long line (a minified table, a long URL list) used to make
+	// every diagnostic on that line pay for a full backward scan.
+	nl := f.lineIndex()
+	lo := newlineSearch(nl, offset)
+	start := 0
+	if lo > 0 {
+		start = nl[lo-1] + 1
 	}
 	return offset - start + 1
 }
+
+// newlineSearch returns the index of the first entry in nl (a sorted
+// list of newline byte offsets) that is >= offset, or len(nl) if none.
+// Inlined rather than sort.Search: sort.Search's comparison closure
+// would capture nl and offset and escape to the heap (engine-bench
+// profiling attributed ~64 k allocations per 10-iteration run to that
+// closure box before plan 195 inlined the binary search here).
+func newlineSearch(nl []int, offset int) int {
+	lo, hi := 0, len(nl)
+	for lo < hi {
+		mid := int(uint(lo+hi) >> 1)
+		if nl[mid] >= offset {
+			hi = mid
+		} else {
+			lo = mid + 1
+		}
+	}
+	return lo
+}

internal/lint/lint_coverage_test.go

@@ -280,6 +280,14 @@ func TestColumnOfOffset_PastEOFClamps(t *testing.T) {
 	assert.Equal(t, 4, f.ColumnOfOffset(999))
 }
 
+func TestColumnOfOffset_NegativeOffsetClamps(t *testing.T) {
+	// A negative offset clamps to the start of the file (column 1),
+	// mirroring the upper-bound EOF clamp above.
+	f := &File{Source: []byte("line1\nline2\n")}
+	assert.Equal(t, 1, f.ColumnOfOffset(-1))
+	assert.Equal(t, 1, f.ColumnOfOffset(-100))
+}
+
 func TestColumnOfOffset_AtNewline(t *testing.T) {
 	// The newline itself sits at the end of its line.
 	f := &File{Source: []byte("ab\nc")}

internal/rules/tablefmt/prealloc_test.go

@@ -0,0 +1,44 @@
+package tablefmt
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/require"
+)
+
+// wideTableLines builds a table with n data rows, used to make append
+// slice-growth in tryParseTable/findTables visible in an alloc count.
+func wideTableLines(n int) [][]byte {
+	lines := [][]byte{
+		[]byte("| Col A | Col B |"),
+		[]byte("| --- | --- |"),
+	}
+	for i := 0; i < n; i++ {
+		lines = append(lines, []byte("| a | b |"))
+	}
+	return lines
+}
+
+// TestFindTables_PreSizedRowSlices pins the allocation count for parsing
+// one large table. rawLines and rows in tryParseTable used to grow via
+// plain append with no capacity hint, paying several slice-growth
+// reallocs per table (docs/development/high-performance-go.md); a
+// pre-sizing pass ahead of the capture loop removes them.
+func TestFindTables_PreSizedRowSlices(t *testing.T) {
+	lines := wideTableLines(50)
+	codeLines := map[int]struct{}{}
+
+	allocs := testing.AllocsPerRun(50, func() {
+		tables := findTables(lines, codeLines)
+		require.Len(t, tables, 1)
+	})
+	t.Logf("findTables allocs/op for a 52-row table = %.0f", allocs)
+	// Most of the remaining allocs come from splitRowBytes's per-row
+	// cells slice, unrelated to this fix. Before pre-sizing
+	// rawLines/rows in tryParseTable, this fixture measured 174
+	// allocs/op; pre-sizing removed the slice-growth reallocs on both
+	// slices, dropping it to 162.
+	require.LessOrEqualf(t, allocs, float64(162),
+		"findTables allocs/op = %.0f; want <= 162 (rawLines+rows pre-sized "+
+			"in tryParseTable)", allocs)
+}