Skip to content

Commit f6b0031

Browse files
committed
schema: address fifth Copilot review round (plan 156)
Four review findings: 1. validate.go::step (wrong-level branch) used to call a `claimRun` helper that bypassed the run accounting, so a repeated matcher could be silently satisfied by a single wrong-level occurrence. Route wrong-level matches through `claimMatch` so consumed / digitsSeen update normally and `repeat.min` + `sequential` still fire. Drop the obsolete `claimRun` helper. Test: TestPlan156_WrongLevelMatchCountsTowardRepeat. 2. validate.go::claimLateScope + claimOutOfOrder emit an explicit "matched 1 times, required at least N (out-of-order recovery only counts the first occurrence)" diagnostic when the late-claimed scope has `repeat.min > 1`. The in-order matchScope path remains the canonical cardinality surface; this keeps the contract enforced on the recovery path without re-implementing the full run-matcher there. Test: TestPlan156_LateClaimFlagsRepeatMin. 3. test_helpers_test.go: literalScope's doc-comment used to point at a nonexistent `matcherScope` helper. Rewrite to describe the intended call-site convention (tests with regex metacharacters construct the `Scope{Matcher: ...}` literal directly). 4. docs/guides/schemas.md: the schemas guide still described `regex:` as a CUE raw-interpolation string even though the reference page was corrected. Align the guide with the helper-only resolver. https://claude.ai/code/session_012GGH62fZUzLuzP8T4ocGkJ
1 parent db626d4 commit f6b0031

5 files changed

Lines changed: 132 additions & 51 deletions

File tree

docs/guides/schemas.md

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -91,10 +91,13 @@ takes one of three shapes:
9191
Cardinality is one.
9292
- **mapping** — the full form:
9393
`{ regex, repeat?, sequential? }`. `regex:` is
94-
required and is the body of a CUE
95-
raw-interpolation string. `repeat:` bounds the
96-
run; `sequential:` (with the `digits` helper)
97-
asserts ordering.
94+
required; the body is a Go RE2 pattern that
95+
accepts two interpolation references —
96+
`\#(digits)` and `\#(fmvar(name))`. `repeat:`
97+
bounds the run; `sequential:` (with `digits`)
98+
asserts ordering. See the
99+
[section-schema reference](../reference/section-schema.md)
100+
for the full grammar.
98101

99102
### The matcher mapping
100103

internal/schema/plan156_acceptance_test.go

Lines changed: 72 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -389,6 +389,78 @@ func TestPlan156_OptionalSpecificClaimsOwnSlot(t *testing.T) {
389389
"a broad `.+` matcher must not absorb it")
390390
}
391391

392+
// TestPlan156_LateClaimFlagsRepeatMin regresses a Copilot
393+
// finding: when a repeated scope (`repeat.min > 1`) appears
394+
// out-of-order with only one occurrence, the late-claim
395+
// recovery path must still surface a "required at least N"
396+
// diagnostic so the cardinality contract is enforced.
397+
func TestPlan156_LateClaimFlagsRepeatMin(t *testing.T) {
398+
raw := map[string]any{
399+
"sections": []any{
400+
map[string]any{"heading": "A"},
401+
map[string]any{"heading": map[string]any{
402+
"regex": "B",
403+
"repeat": map[string]any{"min": 2},
404+
}},
405+
},
406+
}
407+
sch, err := ParseInline(raw, "kind x")
408+
require.NoError(t, err)
409+
// B appears once before A — out of order, and only one
410+
// occurrence so the repeat.min is not satisfied.
411+
doc := newDocFile(t, "doc.md",
412+
"# T\n\n## B\n\nx\n\n## A\n\ny\n")
413+
diags := Validate(doc, sch, nil, false, makeDiagForTest)
414+
var card bool
415+
for _, d := range diags {
416+
if strings.Contains(d.Message, "required at least 2") {
417+
card = true
418+
}
419+
}
420+
assert.True(t, card,
421+
"the late-claim path must flag the repeat.min shortfall")
422+
}
423+
424+
// TestPlan156_WrongLevelMatchCountsTowardRepeat regresses a
425+
// Copilot finding: a wrong-level match used to go through a
426+
// claimRun helper that did not update consumed/digits, so a
427+
// repeated matcher could be silently satisfied by a single
428+
// wrong-level occurrence without `repeat.min` or `sequential`
429+
// enforcement. The path now routes through claimMatch so the
430+
// run accounting still fires.
431+
func TestPlan156_WrongLevelMatchCountsTowardRepeat(t *testing.T) {
432+
raw := map[string]any{
433+
"sections": []any{
434+
map[string]any{"heading": "Outer", "sections": []any{
435+
map[string]any{
436+
"heading": map[string]any{
437+
"regex": "Inner",
438+
"repeat": map[string]any{"min": 2},
439+
},
440+
},
441+
}},
442+
},
443+
}
444+
sch, err := ParseInline(raw, "kind x")
445+
require.NoError(t, err)
446+
// Outer at H2; only one Inner at H2 (wrong level — expected
447+
// H3). The wrong-level match counts as one occurrence, so the
448+
// run accounting must emit "matched 1 times, required at
449+
// least 2".
450+
doc := newDocFile(t, "doc.md",
451+
"# T\n\n## Outer\n\n## Inner\n\nx\n")
452+
diags := Validate(doc, sch, nil, false, makeDiagForTest)
453+
var shortRun bool
454+
for _, d := range diags {
455+
if strings.Contains(d.Message, "required at least 2") {
456+
shortRun = true
457+
}
458+
}
459+
assert.True(t, shortRun,
460+
"wrong-level match must contribute to the run's count "+
461+
"so repeat.min still fires")
462+
}
463+
392464
// TestPlan156_RejectsInvalidFmvarPath regresses a Copilot
393465
// finding: `fmvar(name)` accepted any argument string at parse
394466
// time, so a typo like `fmvar(my-key)` (unquoted hyphenated key)

internal/schema/plan156_coverage_test.go

Lines changed: 15 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -58,15 +58,14 @@ func TestDisplayHeading_Branches(t *testing.T) {
5858
assert.Equal(t, "", displayHeading(Scope{Preamble: true}))
5959
}
6060

61-
// TestClaimRun_WrongLevelMatchWithChildren covers the branch of
62-
// claimRun that recurses into sc.Sections after a wrong-level
63-
// match: an outer scope whose nested children must still be
64-
// validated even when the outer heading appeared at a shallower
65-
// level than expected.
66-
func TestClaimRun_WrongLevelMatchWithChildren(t *testing.T) {
61+
// TestWrongLevelMatch_RecursesIntoChildren covers the branch of
62+
// claimMatch reached via the shallower-than-expected heading
63+
// path: a scope whose nested children must still be validated
64+
// even when the outer heading appeared at the wrong level.
65+
func TestWrongLevelMatch_RecursesIntoChildren(t *testing.T) {
6766
// Schema: Outer at H3 (because of an outer wrapper), Inner at
6867
// H4. Doc emits Outer at H2 (shallower than expected H3).
69-
// claimRun recurses into Inner's checks; the missing-Inner
68+
// claimMatch recurses into Inner's checks; the missing-Inner
7069
// diagnostic surfaces from the nested validateScopes call.
7170
raw := map[string]any{
7271
"sections": []any{
@@ -98,14 +97,15 @@ func TestClaimRun_WrongLevelMatchWithChildren(t *testing.T) {
9897
}
9998
}
10099
assert.True(t, missingInner,
101-
"claimRun's children-recurse branch must validate nested sections")
100+
"the children-recurse branch must validate nested sections")
102101
}
103102

104-
// TestClaimRun_WrongLevelMatch covers claimRun, which fires when
105-
// matchScope sees a shallower-than-expected heading that still
106-
// matches the matcher: the level-mismatch diagnostic is appended
107-
// and the run is claimed.
108-
func TestClaimRun_WrongLevelMatch(t *testing.T) {
103+
// TestWrongLevelMatch_EmitsLevelDiag covers the wrong-level
104+
// match path: matchScope sees a shallower-than-expected heading
105+
// that still matches the matcher, claimMatch emits the
106+
// level-mismatch diagnostic, and the consumed/digits state is
107+
// updated so repeat.min / sequential still apply.
108+
func TestWrongLevelMatch_EmitsLevelDiag(t *testing.T) {
109109
// Nested schema: outer expects H2, inner expects H3. The doc
110110
// emits the inner heading at H2 (shallower than expected).
111111
raw := map[string]any{
@@ -129,7 +129,8 @@ func TestClaimRun_WrongLevelMatch(t *testing.T) {
129129
level = true
130130
}
131131
}
132-
assert.True(t, level, "claimRun should emit the level-mismatch diagnostic")
132+
assert.True(t, level,
133+
"the wrong-level match path must emit the level-mismatch diagnostic")
133134
}
134135

135136
// TestSequentialDiagMessage_NonInteger covers the parse-error path

internal/schema/test_helpers_test.go

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -4,10 +4,12 @@ package schema
44
// going through the parser. Used by unit tests that exercise the
55
// validator's branches directly.
66

7-
// literalScope returns a required-once scope matching the literal
8-
// text. The bare string is treated as plain text (no regex
9-
// metacharacters are honoured); use matcherScope for an explicit
10-
// regex.
7+
// literalScope returns a required-once scope whose Matcher.Regex
8+
// is the input text verbatim. Test inputs use plain alphanumeric
9+
// heading text where regex metacharacters and the source string
10+
// coincide; tests that need a real regex should construct the
11+
// `Scope{Matcher: &Matcher{...}}` literal directly so the intent
12+
// is obvious at the call site.
1113
func literalScope(text string) Scope {
1214
return Scope{
1315
Heading: text,

internal/schema/validate.go

Lines changed: 32 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -241,16 +241,29 @@ func claimLateScope(
241241
docHeads []DocHeading, docIdx int, claimed map[int]bool,
242242
docFM map[string]any, mkDiag MakeDiag,
243243
) (int, []lint.Diagnostic) {
244+
sc := scopes[idx]
244245
dh := docHeads[docIdx]
245246
diags := []lint.Diagnostic{mkDiag(f.Path, dh.Line,
246247
fmt.Sprintf(
247248
"section %q out of order: expected before this position",
248249
formatHeading(dh.Level, dh.Text)))}
250+
// The late-claim path only consumes one heading. A repeated
251+
// scope (`repeat.min > 1`) needs an explicit cardinality
252+
// diagnostic so the contract is still enforced for the
253+
// out-of-order recovery case.
254+
if sc.Matcher != nil && sc.Matcher.Repeat.Min > 1 {
255+
diags = append(diags, mkDiag(f.Path, dh.Line,
256+
fmt.Sprintf(
257+
"section %q matched 1 times, required at least %d "+
258+
"(out-of-order recovery only counts the first occurrence)",
259+
formatHeading(expectedLevel, displayHeading(sc)),
260+
sc.Matcher.Repeat.Min)))
261+
}
249262
claimed[idx] = true
250263
docIdx++
251-
if len(scopes[idx].Sections) > 0 {
264+
if len(sc.Sections) > 0 {
252265
newIdx, childDiags := validateScopes(
253-
f, scopes[idx].Sections, scopes[idx].Closed,
266+
f, sc.Sections, sc.Closed,
254267
docHeads, docIdx, expectedLevel+1, docFM, mkDiag)
255268
diags = append(diags, childDiags...)
256269
docIdx = newIdx
@@ -405,11 +418,12 @@ func (s *matchRun) step(docHeads []DocHeading, docIdx int) (bool, int, bool) {
405418
dh := docHeads[docIdx]
406419
if dh.Level < s.expectedLevel {
407420
if matched, captured := matchHeading(sc.Matcher, dh, s.docFM); matched {
408-
newIdx, runDiags := claimRun(
409-
s.f, sc, s.idx, s.expectedLevel, docHeads, docIdx,
410-
s.claimed, s.docFM, s.mkDiag, captured)
411-
s.diags = append(s.diags, runDiags...)
412-
return true, newIdx, true
421+
// A shallower-than-expected heading that still matches
422+
// the matcher counts toward the run's consumed/digits
423+
// state — claimMatch emits the level-mismatch
424+
// diagnostic on its own. Returning done=false lets the
425+
// outer loop continue scanning for additional matches.
426+
return false, s.claimMatch(docHeads, docIdx, captured), false
413427
}
414428
return true, docIdx, s.consumed > 0
415429
}
@@ -494,28 +508,6 @@ func (s *matchRun) handleNonMatch(docHeads []DocHeading, docIdx int) (bool, int,
494508
return false, docIdx + 1, false
495509
}
496510

497-
// claimRun handles a wrong-level match for a matcher that consumes
498-
// exactly one heading: emit the level-mismatch diagnostic, mark
499-
// the scope as claimed, and recurse into the matched scope's
500-
// children.
501-
func claimRun(
502-
f *lint.File, sc Scope, idx, expectedLevel int,
503-
docHeads []DocHeading, docIdx int, claimed map[int]bool,
504-
docFM map[string]any, mkDiag MakeDiag, _ string,
505-
) (int, []lint.Diagnostic) {
506-
diags := levelDiagIfNeeded(f, docHeads[docIdx], expectedLevel, mkDiag)
507-
claimed[idx] = true
508-
docIdx++
509-
if len(sc.Sections) > 0 {
510-
newIdx, childDiags := validateScopes(
511-
f, sc.Sections, sc.Closed, docHeads, docIdx,
512-
expectedLevel+1, docFM, mkDiag)
513-
diags = append(diags, childDiags...)
514-
docIdx = newIdx
515-
}
516-
return docIdx, diags
517-
}
518-
519511
// laterScopeMatches reports whether dh matches any later scope in
520512
// the same parent window that is more specific than a broad
521513
// matcher. Used by the per-scope walkers (rules, content,
@@ -617,6 +609,17 @@ func claimOutOfOrder(
617609
formatHeading(dh.Level, dh.Text),
618610
formatHeading(expectedLevel, displayHeading(sc))))}
619611
diags = append(diags, levelDiagIfNeeded(f, dh, expectedLevel, mkDiag)...)
612+
// The out-of-order claim only consumes one heading; a repeated
613+
// late scope (`repeat.min > 1`) needs an explicit cardinality
614+
// diagnostic so the contract is enforced even on recovery.
615+
if ooSc.Matcher != nil && ooSc.Matcher.Repeat.Min > 1 {
616+
diags = append(diags, mkDiag(f.Path, dh.Line,
617+
fmt.Sprintf(
618+
"section %q matched 1 times, required at least %d "+
619+
"(out-of-order recovery only counts the first occurrence)",
620+
formatHeading(expectedLevel, displayHeading(ooSc)),
621+
ooSc.Matcher.Repeat.Min)))
622+
}
620623
claimed[ooIdx] = true
621624
docIdx++
622625
if len(ooSc.Sections) > 0 {

0 commit comments

Comments
 (0)