review(round1): single-pass foreign-region restore + coverage

Code-review round 1 fixes for the foreign-regions feature (PR #749).

Correctness: no bugs found. Scan (line ranges) and matchedRegionSpans
(byte spans) agree on the matched-pair state machine, GeneratedRanges
are produced and consumed in consistent post-front-matter coordinates,
and the parse-time AppendRanges / read-only Diagnostics split avoids
mutating the shared cached *File.

Efficiency/simplification:
- foreignregion.Restore now rebuilds the buffer in a single forward
  pass (spliceSpans) instead of a full-buffer rebuild per span
  (was O(k*n) allocations for k regions).
- checkForeignRegionList trims each marker once instead of up to five
  times in the equality branch.

Coverage (CI diff gate): add unit tests covering the previously
uncovered paths — Apply/AppendRanges/Diagnostics/resolve, the Scan
nil-file guard, EffectiveForeignRegions nil-config and empty-override
branches, the override validation path, empty-end rejection, and
copyForeignRegions. All three flagged files now at 100%.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EwKxDxhjkTQPPkBrNhsrNG

Author: claude

internal/config/foreignregion.go

@@ -57,16 +57,18 @@ func validateForeignRegions(cfg *Config) error {
 
 func checkForeignRegionList(regions []ForeignRegion) error {
 	for i, r := range regions {
-		if strings.TrimSpace(r.Start) == "" {
+		start := strings.TrimSpace(r.Start)
+		end := strings.TrimSpace(r.End)
+		if start == "" {
 			return fmt.Errorf("foreign-regions[%d]: start marker must not be empty", i)
 		}
-		if strings.TrimSpace(r.End) == "" {
+		if end == "" {
 			return fmt.Errorf("foreign-regions[%d]: end marker must not be empty", i)
 		}
-		if strings.TrimSpace(r.Start) == strings.TrimSpace(r.End) {
+		if start == end {
 			return fmt.Errorf(
 				"foreign-regions[%d]: start and end markers must differ (both %q)",
-				i, strings.TrimSpace(r.Start))
+				i, start)
 		}
 	}
 	return nil

internal/config/foreignregion_more_test.go

@@ -0,0 +1,69 @@
+package config
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+// TestEffectiveForeignRegionsNilConfig returns nil for a nil config.
+func TestEffectiveForeignRegionsNilConfig(t *testing.T) {
+	assert.Nil(t, EffectiveForeignRegions(nil, "README.md"))
+}
+
+// TestEffectiveForeignRegionsSkipsEmptyOverride ignores an override that
+// declares no foreign regions, even when its glob matches.
+func TestEffectiveForeignRegionsSkipsEmptyOverride(t *testing.T) {
+	cfg := &Config{
+		ForeignRegions: []ForeignRegion{{Start: "<!-- a -->", End: "<!-- b -->"}},
+		Overrides: []Override{
+			{Glob: []string{"README.md"}}, // matches, but has no ForeignRegions
+		},
+	}
+	got := EffectiveForeignRegions(cfg, "README.md")
+	require.Len(t, got, 1)
+	assert.Equal(t, "<!-- a -->", got[0].Start)
+}
+
+// TestParseForeignRegionsEmptyEndRejected rejects a marker pair with a
+// blank end marker.
+func TestParseForeignRegionsEmptyEndRejected(t *testing.T) {
+	yml := `foreign-regions:
+  - start: "<!-- apm:start -->"
+    end: ""
+`
+	_, err := ParseBytes([]byte(yml))
+	require.Error(t, err)
+	assert.Contains(t, err.Error(), "end marker must not be empty")
+}
+
+// TestParseForeignRegionsOverrideInvalidRejected surfaces a malformed
+// marker pair declared on an override, not just the top-level list.
+func TestParseForeignRegionsOverrideInvalidRejected(t *testing.T) {
+	yml := `overrides:
+  - glob: ["AGENTS.md"]
+    foreign-regions:
+      - start: ""
+        end: "<!-- gen:end -->"
+`
+	_, err := ParseBytes([]byte(yml))
+	require.Error(t, err)
+	assert.Contains(t, err.Error(), "start marker must not be empty")
+}
+
+// TestCopyForeignRegionsNil returns nil for a nil input.
+func TestCopyForeignRegionsNil(t *testing.T) {
+	assert.Nil(t, copyForeignRegions(nil))
+}
+
+// TestCopyForeignRegionsIsolatesBackingArray returns an independent copy
+// whose mutation does not touch the source slice.
+func TestCopyForeignRegionsIsolatesBackingArray(t *testing.T) {
+	src := []ForeignRegion{{Start: "<!-- a -->", End: "<!-- b -->"}}
+	out := copyForeignRegions(src)
+	require.Len(t, out, 1)
+	assert.Equal(t, src[0], out[0])
+	out[0].Start = "mutated"
+	assert.Equal(t, "<!-- a -->", src[0].Start, "copy must not alias the source")
+}

internal/foreignregion/apply_test.go

@@ -0,0 +1,72 @@
+package foreignregion
+
+import (
+	"testing"
+
+	"github.com/jeduden/mdsmith/internal/config"
+	"github.com/jeduden/mdsmith/internal/lint"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+func apmCfg() *config.Config {
+	return &config.Config{ForeignRegions: []config.ForeignRegion{apm}}
+}
+
+// TestScanNilFile returns nothing for a nil *File rather than panicking.
+func TestScanNilFile(t *testing.T) {
+	ranges, diags := Scan(nil, []config.ForeignRegion{apm})
+	assert.Nil(t, ranges)
+	assert.Nil(t, diags)
+}
+
+// TestApplyExtendsRangesAndReturnsDiags appends the matched-pair spans to
+// f.GeneratedRanges and returns the malformed-region diagnostics.
+func TestApplyExtendsRangesAndReturnsDiags(t *testing.T) {
+	// A matched pair plus a trailing unmatched start (malformed).
+	src := "<!-- apm:start -->\nbody\n<!-- apm:end -->\n<!-- apm:start -->\ndangling\n"
+	f := newFile(t, src)
+	diags := Apply(f, apmCfg(), "test.md")
+	require.Len(t, f.GeneratedRanges, 1)
+	assert.Equal(t, lint.LineRange{From: 1, To: 3}, f.GeneratedRanges[0])
+	require.Len(t, diags, 1)
+	assert.Equal(t, "test.md", diags[0].File)
+	assert.Contains(t, diags[0].Message, "no matching end")
+}
+
+// TestApplyNoRegions leaves GeneratedRanges untouched and returns no
+// diagnostics when the config declares no marker pairs.
+func TestApplyNoRegions(t *testing.T) {
+	f := newFile(t, "# Title\n\nbody\n")
+	diags := Apply(f, &config.Config{}, "test.md")
+	assert.Empty(t, f.GeneratedRanges)
+	assert.Nil(t, diags)
+}
+
+// TestAppendRangesPopulatesWithoutDiags extends the exclusion set but
+// discards the malformed diagnostics (the read-only RunSource path).
+func TestAppendRangesPopulatesWithoutDiags(t *testing.T) {
+	src := "<!-- apm:start -->\nbody\n<!-- apm:end -->\n"
+	f := newFile(t, src)
+	AppendRanges(f, apmCfg(), "test.md")
+	require.Len(t, f.GeneratedRanges, 1)
+	assert.Equal(t, lint.LineRange{From: 1, To: 3}, f.GeneratedRanges[0])
+}
+
+// TestDiagnosticsReturnsWithoutMutating rebuilds the malformed diagnostics
+// without touching f.GeneratedRanges.
+func TestDiagnosticsReturnsWithoutMutating(t *testing.T) {
+	src := "<!-- apm:end -->\n"
+	f := newFile(t, src)
+	diags := Diagnostics(f, apmCfg(), "test.md")
+	assert.Empty(t, f.GeneratedRanges)
+	require.Len(t, diags, 1)
+	assert.Contains(t, diags[0].Message, "without a matching start")
+	assert.Equal(t, "test.md", diags[0].File)
+}
+
+// TestDiagnosticsNoRegions returns nil when no marker pairs apply.
+func TestDiagnosticsNoRegions(t *testing.T) {
+	f := newFile(t, "# Title\n")
+	assert.Nil(t, Diagnostics(f, &config.Config{}, "test.md"))
+}

internal/foreignregion/restore.go

@@ -27,21 +27,26 @@ func Restore(original, fixed []byte, regions []config.ForeignRegion) []byte {
 		if len(origSpans) == 0 || len(origSpans) != len(fixedSpans) {
 			continue
 		}
-		// Replace from the last span to the first so each splice leaves
-		// the earlier spans' byte offsets valid.
-		for i := len(fixedSpans) - 1; i >= 0; i-- {
-			origText := original[origSpans[i].start:origSpans[i].end]
-			fs := fixedSpans[i]
-			out := make([]byte, 0, len(fixed)-(fs.end-fs.start)+len(origText))
-			out = append(out, fixed[:fs.start]...)
-			out = append(out, origText...)
-			out = append(out, fixed[fs.end:]...)
-			fixed = out
-		}
+		fixed = spliceSpans(original, fixed, origSpans, fixedSpans)
 	}
 	return fixed
 }
 
+// spliceSpans rebuilds fixed in one pass, replacing each fixed span with
+// the corresponding original span's bytes. Spans are in document order,
+// so a single forward walk suffices — one allocation instead of the
+// per-span full-buffer rebuild a reverse splice would cost.
+func spliceSpans(original, fixed []byte, origSpans, fixedSpans []byteSpan) []byte {
+	out := make([]byte, 0, len(fixed))
+	prev := 0
+	for i, fs := range fixedSpans {
+		out = append(out, fixed[prev:fs.start]...)
+		out = append(out, original[origSpans[i].start:origSpans[i].end]...)
+		prev = fs.end
+	}
+	return append(out, fixed[prev:]...)
+}
+
 // byteSpan is a half-open [start, end) byte range within a buffer.
 type byteSpan struct {
 	start int