review(round3): trim-aware pair dedup, doc accuracy, lock duplicate-start span

Round-3 review of the foreign-regions feature:

- config: dedup foreign-region marker pairs by their *trimmed* markers,
  matching the scanner's whole-line-after-TrimSpace comparison. A raw
  `==` let two pairs that differ only in surrounding whitespace (e.g.
  "<!-- x -->" vs " <!-- x -->") both survive, re-introducing the
  double MDS073 / double-protection the round-2 dedup targeted.
- docs: correct foreign-regions.md — a duplicate `start` still pairs
  the first `start` with the next `end`, so that span *is* protected;
  the old "protects no bytes for it" blanket was wrong for that case.
- tests: lock the duplicate-start behavior with range/span assertions on
  both scan paths (Scan and Restore), and add a whitespace-variant dedup
  test.

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

Author: claude

docs/reference/foreign-regions.md

@@ -76,13 +76,17 @@ opaque to editing, not invisible to size accounting.
 ## Malformed regions (MDS073)
 
 APM requires each marker exactly once. mdsmith reports **MDS073** on a
-malformed region and protects no bytes for it:
+malformed region:
 
-| Condition                                       | Reported on                |
-| ----------------------------------------------- | -------------------------- |
-| A `start` marker with no matching `end`         | the `start` line           |
-| An `end` marker with no preceding `start`       | the `end` line             |
-| A second `start` before the first region closes | the duplicate `start` line |
+| Condition                                       | Reported on                | Bytes protected                              |
+| ----------------------------------------------- | -------------------------- | -------------------------------------------- |
+| A `start` marker with no matching `end`         | the `start` line           | none                                         |
+| An `end` marker with no preceding `start`       | the `end` line             | none                                         |
+| A second `start` before the first region closes | the duplicate `start` line | the first `start` through the matching `end` |
+
+An unmatched `start` or `end` protects no bytes. A duplicate `start`
+still pairs the first `start` with the next `end`, so that span is
+protected while the extra marker only draws the diagnostic.
 
 ## Non-goals
 

internal/config/foreignregion.go

@@ -37,16 +37,25 @@ func EffectiveForeignRegions(cfg *Config, filePath string) []ForeignRegion {
 	return out
 }
 
-// containsForeignRegion reports whether list already holds an identical
+// containsForeignRegion reports whether list already holds an equivalent
 // marker pair. EffectiveForeignRegions dedups with it so a pair declared
 // both top-level and on a matching override (or repeated within a list)
 // contributes one protected span and one MDS073 diagnostic, not two —
-// the check path does not otherwise dedup per-file diagnostics. The
-// marker-pair lists are short (a handful of entries), so the linear scan
-// is cheaper than allocating a set on this per-file hot path.
+// the check path does not otherwise dedup per-file diagnostics.
+//
+// Equivalence is by trimmed markers, matching how the scanner compares a
+// marker against a source line (whole-line equality after TrimSpace). Two
+// pairs that differ only in surrounding whitespace — e.g. "<!-- x -->" and
+// " <!-- x -->" — scan identically, so they must dedup to one span and one
+// diagnostic; a raw `==` comparison would let the whitespace variant slip
+// past and re-introduce the double report. The marker-pair lists are short
+// (a handful of entries), so the linear scan is cheaper than allocating a
+// set on this per-file hot path.
 func containsForeignRegion(list []ForeignRegion, r ForeignRegion) bool {
+	start := strings.TrimSpace(r.Start)
+	end := strings.TrimSpace(r.End)
 	for _, e := range list {
-		if e == r {
+		if strings.TrimSpace(e.Start) == start && strings.TrimSpace(e.End) == end {
 			return true
 		}
 	}

internal/config/foreignregion_more_test.go

@@ -92,6 +92,28 @@ func TestEffectiveForeignRegionsDedupes(t *testing.T) {
 	assert.Equal(t, ForeignRegion{Start: "<!-- c -->", End: "<!-- d -->"}, got[1])
 }
 
+// TestEffectiveForeignRegionsDedupesWhitespaceVariant collapses two pairs
+// that differ only in surrounding marker whitespace into one entry: the
+// scanner compares markers by trimmed-line equality, so the variant scans
+// identically and must protect one span and report one MDS073, not two.
+func TestEffectiveForeignRegionsDedupesWhitespaceVariant(t *testing.T) {
+	cfg := &Config{
+		ForeignRegions: []ForeignRegion{{Start: "<!-- a -->", End: "<!-- b -->"}},
+		Overrides: []Override{
+			{
+				Glob: []string{"AGENTS.md"},
+				ForeignRegions: []ForeignRegion{
+					// Same markers, extra surrounding whitespace.
+					{Start: "  <!-- a -->", End: "<!-- b -->  "},
+				},
+			},
+		},
+	}
+	got := EffectiveForeignRegions(cfg, "AGENTS.md")
+	require.Len(t, got, 1)
+	assert.Equal(t, ForeignRegion{Start: "<!-- a -->", End: "<!-- b -->"}, got[0])
+}
+
 // TestCopyForeignRegionsNil returns nil for a nil input.
 func TestCopyForeignRegionsNil(t *testing.T) {
 	assert.Nil(t, copyForeignRegions(nil))

internal/foreignregion/restore_test.go

@@ -42,3 +42,15 @@ func TestRestoreMultipleRegions(t *testing.T) {
 	got := Restore([]byte(original), []byte(fixed), []config.ForeignRegion{apm})
 	assert.Equal(t, original, string(got))
 }
+
+// TestRestoreDuplicateStartProtectsFirstSpan mirrors Scan's duplicate-start
+// pairing on the byte path: the first start pairs with the following end, so
+// a fixer's edit inside that span is restored even though the region is
+// malformed (a second start opened before it closed).
+func TestRestoreDuplicateStartProtectsFirstSpan(t *testing.T) {
+	s, e := apm.Start, apm.End
+	original := s + "\nA   \n" + s + "\nB   \n" + e + "\n"
+	fixed := s + "\nA\n" + s + "\nB\n" + e + "\n"
+	got := Restore([]byte(original), []byte(fixed), []config.ForeignRegion{apm})
+	assert.Equal(t, original, string(got))
+}

internal/foreignregion/scan_test.go

@@ -65,14 +65,20 @@ func TestScanOrphanedEnd(t *testing.T) {
 }
 
 // TestScanDuplicateStart reports a diagnostic when a second start marker
-// opens before the first region closes.
+// opens before the first region closes, and still pairs the first start
+// with the following end so that span stays protected (the duplicate
+// marker only draws the diagnostic).
 func TestScanDuplicateStart(t *testing.T) {
 	src := "<!-- apm:start -->\na\n<!-- apm:start -->\nb\n<!-- apm:end -->\n"
 	f := newFile(t, src)
-	_, diags := Scan(f, []config.ForeignRegion{apm})
-	require.NotEmpty(t, diags)
+	ranges, diags := Scan(f, []config.ForeignRegion{apm})
+	require.Len(t, diags, 1)
 	assert.Contains(t, diags[0].Message, "duplicate")
 	assert.Equal(t, 3, diags[0].Line)
+	// The first start (line 1) still pairs with the end (line 5): the
+	// span is protected even though the region is flagged malformed.
+	require.Len(t, ranges, 1)
+	assert.Equal(t, lint.LineRange{From: 1, To: 5}, ranges[0])
 }
 
 // TestScanMultiplePairs returns a range for each independent matched