Skip to content

Commit 9ac4349

Browse files
committed
Address round-3 review: Splice negative-Start, IsGitHubAlert contract pin
Recall agent surfaced three findings after commit 17cca70; one was the F5 silent-corruption path already closed in commit 131a9f1. The remaining two land here. #2: markdown.Splice's precondition check fired for negative-Start edits via the generic "overlaps previous edit ending at 0" panic. A producer that subtracts past 0 and emits Start=-1 sent the debugger chasing a non-existent previous edit. Add a dedicated `Start < 0` guard that names the actual fault, plus a pin in TestSpliceInvariantViolation. #3: fixGitHubAlerts removed the local (Paragraph, non-empty Lines) re-check and trusts flavor.IsGitHubAlert's contract — but no behavior test pinned that contract. The four existing fix tests exercise inputs where IsGitHubAlert returns true via the real parser; they would not catch a future relaxation of IsGitHubAlert that returns true for a non-Paragraph first child. Add TestIsGitHubAlertContractPostcondition: walk a corpus of alert / non-alert / degenerate blockquotes, and on every IsGitHubAlert==true case assert FirstChild is *ast.Paragraph with non-empty Lines. Coverage in pkg/markdown and pkg/markdown/flavor stays at 100%; mdsmith check and golangci-lint clean. https://claude.ai/code/session_0144ZKUS2Zrg7xBft54qyoti
1 parent ba74889 commit 9ac4349

3 files changed

Lines changed: 72 additions & 0 deletions

File tree

pkg/markdown/flavor/detect_edge_test.go

Lines changed: 55 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -310,6 +310,61 @@ func TestFindHeadingIDNilHeadingReturnsFalse(t *testing.T) {
310310
assert.Equal(t, HeadingIDExtra{}, hx)
311311
}
312312

313+
// TestIsGitHubAlertContractPostcondition pins the documented
314+
// postcondition: IsGitHubAlert returns true ⇒ bq.FirstChild() is
315+
// *ast.Paragraph and Paragraph.Lines.Len() > 0. The rule's
316+
// fixGitHubAlerts trusts this postcondition (its bq.FirstChild()
317+
// type assertion + lines.At(0) are unguarded), so a future refactor
318+
// that relaxes IsGitHubAlert without updating the call site would
319+
// panic in production. Driving the contract from a corpus of
320+
// representative blockquotes — alert and non-alert, well-formed and
321+
// degenerate — catches the drift here instead.
322+
func TestIsGitHubAlertContractPostcondition(t *testing.T) {
323+
corpus := []string{
324+
"> [!NOTE]\n> body\n",
325+
"> [!TIP]\n> body\n",
326+
"> [!IMPORTANT]\n> body\n",
327+
"> [!WARNING]\n> body\n",
328+
"> [!CAUTION]\n> body\n",
329+
"> [!NOTE]\n> first\n> second\n",
330+
"> [!NOTE]\n", // marker only, no continuation
331+
"> plain paragraph\n",
332+
"> # heading not paragraph\n",
333+
"plain paragraph\n",
334+
"```\nfenced\n```\n",
335+
"> [!note]\n> case-sensitive miss\n",
336+
}
337+
for _, src := range corpus {
338+
t.Run(src, func(t *testing.T) {
339+
doc := mkDoc(t, src)
340+
_ = ast.Walk(doc.AST, func(n ast.Node, entering bool) (ast.WalkStatus, error) {
341+
if !entering {
342+
return ast.WalkContinue, nil
343+
}
344+
bq, ok := n.(*ast.Blockquote)
345+
if !ok {
346+
return ast.WalkContinue, nil
347+
}
348+
if !IsGitHubAlert(bq, []byte(src)) {
349+
return ast.WalkContinue, nil
350+
}
351+
// Postcondition: the rule's fixGitHubAlerts trusts
352+
// these two invariants — keep them locked.
353+
para, ok := bq.FirstChild().(*ast.Paragraph)
354+
require.True(t, ok,
355+
"IsGitHubAlert returned true but bq.FirstChild() is %T",
356+
bq.FirstChild())
357+
lines := para.Lines()
358+
require.NotNil(t, lines,
359+
"IsGitHubAlert returned true but Paragraph.Lines() is nil")
360+
require.Greater(t, lines.Len(), 0,
361+
"IsGitHubAlert returned true but Paragraph has empty Lines")
362+
return ast.WalkContinue, nil
363+
})
364+
})
365+
}
366+
}
367+
313368
// TestIsGitHubAlertHandlesEdgeCases pins the defensive branches of
314369
// IsGitHubAlert: nil blockquote, missing-paragraph first child, and
315370
// a paragraph with empty Lines all return false rather than panic.

pkg/markdown/parse_test.go

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -157,6 +157,18 @@ func TestSpliceInvariantViolation(t *testing.T) {
157157
func() { Splice(body, []Edit{{Start: 5, End: 3}}) })
158158
})
159159

160+
t.Run("negative Start panics with a dedicated message", func(t *testing.T) {
161+
// Caller produces an edit with a negative offset (typically a
162+
// buggy producer subtracting past 0). Without this check the
163+
// generic overlap message ("overlaps previous edit ending at 0")
164+
// would fire and mislead the debugger toward a non-existent
165+
// previous edit; the dedicated message names the actual fault.
166+
body := []byte("0123456789")
167+
assert.PanicsWithValue(t,
168+
"markdown.Splice: edit 0 has negative Start ({Start:-1, End:5})",
169+
func() { Splice(body, []Edit{{Start: -1, End: 5}}) })
170+
})
171+
160172
t.Run("edit exceeding body length panics", func(t *testing.T) {
161173
body := []byte("short")
162174
assert.PanicsWithValue(t,

pkg/markdown/produce.go

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -42,6 +42,11 @@ func Splice(body []byte, edits []Edit) []byte {
4242
// an opaque slice-bounds panic during the build loop below.
4343
prevEnd := 0
4444
for i, e := range edits {
45+
if e.Start < 0 {
46+
panic(fmt.Sprintf(
47+
"markdown.Splice: edit %d has negative Start "+
48+
"({Start:%d, End:%d})", i, e.Start, e.End))
49+
}
4550
if e.Start < prevEnd {
4651
panic(fmt.Sprintf(
4752
"markdown.Splice: edits must be ascending and "+

0 commit comments

Comments
 (0)