Skip to content

Commit 22df596

Browse files
committed
schema: address eighth Copilot review round (plan 156)
Three review findings: 1. validate.go::claimedScopeMatches — the max-exceeded recovery used to fire for any already-claimed scope, including matchers with an unbounded `max == 0`. A non-contiguous occurrence of a `repeat: { min: 1 }` scope (no upper bound) was silently flagged as "exceeds allowed occurrences". Skip unbounded matchers in the helper so the diagnostic only fires when there really is a max to exceed. Test: TestPlan156_UnboundedRepeatDoesNotFlagAsExceeded. 2. parse_inline.go — revert the `closed: true` + empty `sections: []` relaxation from the previous round. `Schema.IsEmpty` ignores `Closed`, so an empty-sections closed schema is skipped by Validate entirely — the relaxation didn't actually enable the "forbid any top-level headings" use case. Restore the stricter check that requires a non-empty `sections:` list and document the IsEmpty rationale. 3. plan/156_schema-entry-unification.md — task 4 now notes explicitly that MDS020's file-schema check still routes through its legacy `parseSchema`/`parsedSchema` pipeline so the `{field}` heading/body sync feature survives. The new `parse_file.go` is exercised by schema-package tests and prepares the ground for the cutover in a follow-up plan; proto.md authors don't yet benefit from the new Matcher / fmvar / digits semantics. https://claude.ai/code/session_012GGH62fZUzLuzP8T4ocGkJ
1 parent ddf3cc2 commit 22df596

4 files changed

Lines changed: 63 additions & 14 deletions

File tree

internal/schema/parse_inline.go

Lines changed: 8 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -63,18 +63,16 @@ func ParseInline(raw map[string]any, source string) (*Schema, error) {
6363
}
6464

6565
// schema-level `closed:` only makes sense when the schema also
66-
// declares `sections:` — strictness has no scope to apply to
67-
// when the kind only constrains front matter / filename. Plan
68-
// 156 surfaces the mismatch at parse time. Key presence is the
69-
// signal (not parsed length) so an explicit `sections: []`
70-
// closed schema — useful for forbidding any top-level
71-
// headings — stays expressible.
72-
_, hasClosed := raw["closed"]
73-
_, hasSections := raw["sections"]
74-
if hasClosed && !hasSections {
66+
// declares a non-empty `sections:` list — strictness has no
67+
// scope to apply to when the kind only constrains front matter
68+
// / filename, and `Schema.IsEmpty` ignores `Closed`, so an
69+
// empty-sections closed schema would be skipped by Validate
70+
// entirely. Plan 156 surfaces the mismatch at parse time.
71+
if _, hasClosed := raw["closed"]; hasClosed && len(sch.Sections) == 0 {
7572
return nil, fmt.Errorf(
7673
"schema.closed: only valid on schemas that declare " +
77-
"`sections:` — drop the key on a frontmatter-only kind")
74+
"a non-empty `sections:` list — drop the key on a " +
75+
"frontmatter-only kind or add at least one section")
7876
}
7977

8078
if err := rejectUnknownTopKeys(raw); err != nil {

internal/schema/plan156_acceptance_test.go

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -459,6 +459,39 @@ func TestPlan156_OutOfOrderSequentialDiagFires(t *testing.T) {
459459
"the unenforced-sequential diagnostic")
460460
}
461461

462+
// TestPlan156_UnboundedRepeatDoesNotFlagAsExceeded regresses a
463+
// Copilot finding: claimedScopeMatches used to fire for any
464+
// claimed scope, including unbounded matchers (`max == 0`). A
465+
// later non-contiguous occurrence of a `repeat: { min: 1 }`
466+
// scope must NOT surface as "exceeds allowed occurrences" —
467+
// the matcher has no upper bound.
468+
func TestPlan156_UnboundedRepeatDoesNotFlagAsExceeded(t *testing.T) {
469+
raw := map[string]any{
470+
"sections": []any{
471+
map[string]any{
472+
"heading": map[string]any{
473+
"regex": "Step",
474+
"repeat": map[string]any{"min": 1},
475+
},
476+
},
477+
map[string]any{"heading": "Summary"},
478+
},
479+
}
480+
sch, err := ParseInline(raw, "kind x")
481+
require.NoError(t, err)
482+
// Two Step sections sandwiching Summary. The second Step
483+
// is non-contiguous; the unbounded matcher has no max to
484+
// exceed.
485+
doc := newDocFile(t, "doc.md",
486+
"# T\n\n## Step\n\nx\n\n## Summary\n\ny\n\n## Step\n\nz\n")
487+
diags := Validate(doc, sch, nil, false, makeDiagForTest)
488+
for _, d := range diags {
489+
assert.NotContains(t, d.Message, "exceeds scope",
490+
"an unbounded repeat must not produce a max-exceeded "+
491+
"diagnostic on a non-contiguous occurrence")
492+
}
493+
}
494+
462495
// TestPlan156_BroadMatcherYieldsBeforeMin regresses a Copilot
463496
// finding: a broad matcher (`.+`) with `repeat: { min: 2 }`
464497
// used to consume a later named scope's heading to satisfy its

internal/schema/validate.go

Lines changed: 15 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -242,9 +242,13 @@ func handleLeftoverHeadings(
242242
}
243243

244244
// claimedScopeMatches returns the index of the first already-claimed
245-
// non-slot, non-preamble scope whose matcher accepts dh, or -1 when
246-
// no claimed scope is a candidate. Used by handleLeftoverHeadings to
247-
// distinguish "max exceeded" from generic "unexpected".
245+
// non-slot, non-preamble scope whose matcher has a bounded `max`
246+
// AND accepts dh, or -1 when no such scope is a candidate. Used by
247+
// handleLeftoverHeadings to distinguish "max exceeded" from
248+
// generic "unexpected". Unbounded matchers (`max == 0`) are
249+
// excluded because they accept any number of occurrences — a
250+
// non-contiguous extra is a normal trailing leftover for them,
251+
// not a cardinality violation.
248252
func claimedScopeMatches(
249253
scopes []Scope, dh DocHeading, claimed map[int]bool, docFM map[string]any,
250254
) int {
@@ -255,6 +259,14 @@ func claimedScopeMatches(
255259
if sc.Preamble || isSlotMatcher(sc.Matcher) {
256260
continue
257261
}
262+
if sc.Matcher == nil {
263+
continue
264+
}
265+
_, max := sc.Matcher.Repeat.Bounds()
266+
if max == 0 {
267+
// Unbounded — no max to exceed.
268+
continue
269+
}
258270
if scopeMatchesHeading(sc, dh, docFM) {
259271
return i
260272
}

plan/156_schema-entry-unification.md

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -138,7 +138,13 @@ Files rewritten as part of implementation:
138138
`Matcher` (`regex: '.+'`, plus
139139
`repeat: { min: 0 }` for `...`). Keep
140140
`{n}` and `{field}` token expansion in
141-
heading rows.
141+
heading rows. (Scope note: MDS020's file-
142+
schema check still routes through its
143+
legacy `parseSchema`/`parsedSchema` pipeline
144+
so the `{field}` heading/body sync feature
145+
survives; this parser is exercised by the
146+
schema-package tests and prepares the
147+
ground for the cutover in a follow-up plan.)
142148
5. [x] Rewrite the validator in
143149
`internal/schema/validate.go` to match
144150
the heading sequence as a positional

0 commit comments

Comments
 (0)