fix: address docs checker review findings
Check every link reference definition, decode destinations per CommonMark, report correct lines for empty headings, autolinks and nested definitions, reject symlinked record directories and wrong-case path components, and refuse titles that cannot pass. Only top-level headings count (spec section 6). Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
This commit is contained in:
1 parent
789931d90f
commit
d579e38ba5
8 files changed
+146
-27
No files matched your search
+85
-20
@@ -3,12 +3,14 @@ package docs
|
||||
import (
|
||||
"bytes"
|
||||
"regexp"
|
||||
"slices"
|
||||
"strings"
|
||||
|
||||
"github.com/yuin/goldmark"
|
||||
"github.com/yuin/goldmark/ast"
|
||||
"github.com/yuin/goldmark/parser"
|
||||
"github.com/yuin/goldmark/text"
|
||||
"github.com/yuin/goldmark/util"
|
||||
)
|
||||
|
||||
var (
|
||||
@@ -16,6 +18,8 @@ var (
|
||||
planSections = []string{"Scope", "Steps", "Validation"}
|
||||
)
|
||||
|
||||
var atxRe = regexp.MustCompile(`(?m)^ {0,3}#{1,6}(?:[ \t]|$)`)
|
||||
|
||||
type heading struct {
|
||||
level int
|
||||
text string
|
||||
@@ -36,8 +40,35 @@ func (d *doc) lineAt(off int) int {
|
||||
return d.startLine + bytes.Count(d.src[:min(off, len(d.src))], []byte("\n"))
|
||||
}
|
||||
|
||||
// lineOf is the line of the first text under n, or of the nearest block that has lines.
|
||||
func (d *doc) lineOf(n ast.Node) int {
|
||||
// headingLine is the line of h. An empty ATX heading has no lines, so it is the first
|
||||
// `#` line after the end of the previous top-level block.
|
||||
func (d *doc) headingLine(h *ast.Heading, prev ast.Node) int {
|
||||
if h.Lines().Len() > 0 {
|
||||
return d.lineAt(h.Lines().At(0).Start)
|
||||
}
|
||||
from := 0
|
||||
if prev != nil {
|
||||
_ = ast.Walk(prev, func(c ast.Node, enter bool) (ast.WalkStatus, error) {
|
||||
if enter {
|
||||
for i := range c.Lines().Len() {
|
||||
from = max(from, c.Lines().At(i).Stop)
|
||||
}
|
||||
if t, ok := c.(*ast.Text); ok {
|
||||
from = max(from, t.Segment.Stop)
|
||||
}
|
||||
}
|
||||
return ast.WalkContinue, nil
|
||||
})
|
||||
}
|
||||
if m := atxRe.FindIndex(d.src[from:]); m != nil {
|
||||
return d.lineAt(from + m[0])
|
||||
}
|
||||
return d.startLine
|
||||
}
|
||||
|
||||
// lineOf is the line of the first text under n. Without text (an autolink, an image with
|
||||
// no alt text) it is the line where needle appears in the enclosing block, else the block's first line.
|
||||
func (d *doc) lineOf(n ast.Node, needle string) int {
|
||||
var off = -1
|
||||
_ = ast.Walk(n, func(c ast.Node, enter bool) (ast.WalkStatus, error) {
|
||||
if t, ok := c.(*ast.Text); ok && enter {
|
||||
@@ -49,6 +80,9 @@ func (d *doc) lineOf(n ast.Node) int {
|
||||
for p := n; off < 0 && p != nil; p = p.Parent() {
|
||||
if p.Type() == ast.TypeBlock && p.Lines().Len() > 0 {
|
||||
off = p.Lines().At(0).Start
|
||||
if i := bytes.Index(d.src[off:p.Lines().At(p.Lines().Len()-1).Stop], []byte(needle)); needle != "" && i >= 0 {
|
||||
off += i
|
||||
}
|
||||
}
|
||||
}
|
||||
if off < 0 {
|
||||
@@ -59,15 +93,12 @@ func (d *doc) lineOf(n ast.Node) int {
|
||||
|
||||
func (c *checker) body(r *record, src string, startLine int, title string, titleOK bool) {
|
||||
b := []byte(src)
|
||||
ctx := parser.NewContext()
|
||||
ctx := &refCtx{Context: parser.NewContext()}
|
||||
root := goldmark.DefaultParser().Parse(text.NewReader(b), parser.WithContext(ctx))
|
||||
d := &doc{c: c, r: r, src: b, startLine: startLine}
|
||||
for n := root.FirstChild(); n != nil; n = n.NextSibling() {
|
||||
if h, ok := n.(*ast.Heading); ok {
|
||||
line := d.startLine
|
||||
if h.Lines().Len() > 0 {
|
||||
line = d.lineAt(h.Lines().At(0).Start)
|
||||
}
|
||||
line := d.headingLine(h, n.PreviousSibling())
|
||||
d.heads = append(d.heads, heading{h.Level, strings.TrimSpace(string(h.Lines().Value(b))), line, len(d.kids)})
|
||||
}
|
||||
d.kids = append(d.kids, n)
|
||||
@@ -161,7 +192,7 @@ func (d *doc) decisionOutcome() {
|
||||
// Prose before the first subsection is the decision itself.
|
||||
pre := nodes
|
||||
for i, n := range nodes {
|
||||
if _, ok := n.(*ast.Heading); ok {
|
||||
if h, ok := n.(*ast.Heading); ok && h.Level == 3 {
|
||||
pre = nodes[:i]
|
||||
break
|
||||
}
|
||||
@@ -223,9 +254,8 @@ func (d *doc) inlineText(n ast.Node) string {
|
||||
return s.String()
|
||||
}
|
||||
|
||||
// placeholders reports every marker outside code.
|
||||
func (d *doc) placeholders(root ast.Node) {
|
||||
var code [][2]int
|
||||
// codeRanges returns the source ranges of code blocks and code spans.
|
||||
func codeRanges(root ast.Node) (code [][2]int) {
|
||||
_ = ast.Walk(root, func(n ast.Node, enter bool) (ast.WalkStatus, error) {
|
||||
switch n.(type) {
|
||||
case *ast.FencedCodeBlock, *ast.CodeBlock:
|
||||
@@ -242,6 +272,12 @@ func (d *doc) placeholders(root ast.Node) {
|
||||
}
|
||||
return ast.WalkContinue, nil
|
||||
})
|
||||
return code
|
||||
}
|
||||
|
||||
// placeholders reports every marker outside code.
|
||||
func (d *doc) placeholders(root ast.Node) {
|
||||
code := codeRanges(root)
|
||||
for off := 0; ; {
|
||||
i := strings.Index(string(d.src[off:]), marker)
|
||||
if i < 0 {
|
||||
@@ -259,27 +295,56 @@ func (d *doc) placeholders(root ast.Node) {
|
||||
}
|
||||
}
|
||||
|
||||
// refCtx records every link reference definition; the parser keeps only the first of a label.
|
||||
type refCtx struct {
|
||||
parser.Context
|
||||
all []parser.Reference
|
||||
}
|
||||
|
||||
func (c *refCtx) AddReference(r parser.Reference) {
|
||||
c.all = append(c.all, r)
|
||||
c.Context.AddReference(r)
|
||||
}
|
||||
|
||||
// defRe finds a definition's label at the start of a line, allowing blockquote and list prefixes.
|
||||
var defRe = regexp.MustCompile(`(?m)^[ \t>]*(?:(?:[*+-]|\d+[.)])[ \t]+)*\[([^\]\n]+)\]:`)
|
||||
|
||||
// dest decodes a link destination as CommonMark does before it is used.
|
||||
func dest(b []byte) string {
|
||||
return string(util.ResolveEntityNames(util.ResolveNumericReferences(util.UnescapePunctuations(b))))
|
||||
}
|
||||
|
||||
// links checks every link, image and autolink destination and every reference definition.
|
||||
func (d *doc) links(root ast.Node, ctx parser.Context) {
|
||||
func (d *doc) links(root ast.Node, ctx *refCtx) {
|
||||
_ = ast.Walk(root, func(n ast.Node, enter bool) (ast.WalkStatus, error) {
|
||||
if !enter {
|
||||
return ast.WalkContinue, nil
|
||||
}
|
||||
switch t := n.(type) {
|
||||
case *ast.Link:
|
||||
d.c.checkLink(d.r, d.lineOf(n), string(t.Destination))
|
||||
d.c.checkLink(d.r, d.lineOf(n, "]("+string(t.Destination)), dest(t.Destination))
|
||||
case *ast.Image:
|
||||
d.c.checkLink(d.r, d.lineOf(n), string(t.Destination))
|
||||
d.c.checkLink(d.r, d.lineOf(n, "]("+string(t.Destination)), dest(t.Destination))
|
||||
case *ast.AutoLink:
|
||||
d.c.checkLink(d.r, d.lineOf(n), string(t.URL(d.src)))
|
||||
d.c.checkLink(d.r, d.lineOf(n, "<"+string(t.Label(d.src))), string(t.URL(d.src)))
|
||||
}
|
||||
return ast.WalkContinue, nil
|
||||
})
|
||||
for _, ref := range ctx.References() {
|
||||
line := d.startLine
|
||||
if m := regexp.MustCompile(`(?im)^ {0,3}\[` + regexp.QuoteMeta(string(ref.Label())) + `\]:`).FindIndex(d.src); m != nil {
|
||||
line = d.lineAt(m[0])
|
||||
// shortcut: a definition's line is its label's nth match outside code; a multi-line
|
||||
// definition inside a container may be located by label only. Upgrade by tracking offsets.
|
||||
code := codeRanges(root)
|
||||
lines := map[string][]int{}
|
||||
for _, m := range defRe.FindAllSubmatchIndex(d.src, -1) {
|
||||
if !slices.ContainsFunc(code, func(r [2]int) bool { return m[0] >= r[0] && m[0] < r[1] }) {
|
||||
k := util.ToLinkReference(d.src[m[2]:m[3]])
|
||||
lines[k] = append(lines[k], d.lineAt(m[0]))
|
||||
}
|
||||
d.c.checkLink(d.r, line, string(ref.Destination()))
|
||||
}
|
||||
for _, ref := range ctx.all {
|
||||
line, k := d.startLine, util.ToLinkReference(ref.Label())
|
||||
if len(lines[k]) > 0 {
|
||||
line, lines[k] = lines[k][0], lines[k][1:]
|
||||
}
|
||||
d.c.checkLink(d.r, line, dest(ref.Destination()))
|
||||
}
|
||||
}
|
||||
@@ -57,10 +57,14 @@ func Check(root string) (Result, error) {
|
||||
c := &checker{root: root, realRoot: real}
|
||||
var recs []*record
|
||||
for _, d := range dirs {
|
||||
entries, err := os.ReadDir(filepath.Join(root, d.dir))
|
||||
if os.IsNotExist(err) {
|
||||
if st, err := os.Lstat(filepath.Join(root, d.dir)); os.IsNotExist(err) {
|
||||
continue
|
||||
} else if err != nil {
|
||||
} else if err == nil && !st.IsDir() {
|
||||
c.add(d.dir, 1, "must be a real directory (symlinks are not allowed)")
|
||||
continue
|
||||
}
|
||||
entries, err := os.ReadDir(filepath.Join(root, d.dir))
|
||||
if err != nil {
|
||||
return Result{}, err
|
||||
}
|
||||
for _, e := range entries {
|
||||
|
||||
@@ -235,6 +235,8 @@ func TestCheck(t *testing.T) {
|
||||
{name: "null empty", files: map[string]string{aPath: sub(adrBase, "summary: Run jobs on a queue.", "summary:")}, want: []string{aPath + ":6: must not be null"}},
|
||||
{name: "null word", files: map[string]string{aPath: sub(adrBase, "summary: Run jobs on a queue.", "summary: Null")}, want: []string{aPath + ":6: must not be null"}},
|
||||
{name: "scalars are literal text", files: map[string]string{aPath: sub(adrBase, "title: Use Queue", "title: 2026", "# Use Queue", "# 2026", "date: 2026-10-23", `date: "2026-10-23"`, "summary: Run jobs on a queue.", `summary: "null"`)}},
|
||||
{name: "null lowercase", files: map[string]string{aPath: sub(adrBase, "summary: Run jobs on a queue.", "summary: null")}, want: []string{aPath + ":6: must not be null"}},
|
||||
{name: "null uppercase", files: map[string]string{aPath: sub(adrBase, "summary: Run jobs on a queue.", "summary: NULL")}, want: []string{aPath + ":6: must not be null"}},
|
||||
{name: "list item is null", files: map[string]string{aPath: sub(adrBase, "related: []", "related:\n - ~")}, want: []string{aPath + ":12: must not be null"}},
|
||||
|
||||
// 3.1 ADR keys
|
||||
@@ -272,6 +274,11 @@ func TestCheck(t *testing.T) {
|
||||
{name: "this PR placeholder", files: map[string]string{aPath: sub(adrBase, "implemented_by: []", "implemented_by:\n - This PR")}, want: []string{aPath + ":14: not durable"}},
|
||||
{name: "fragment in frontmatter", files: map[string]string{aPath: sub(adrBase, "implemented_by: []", "implemented_by:\n - /docs/spec.txt#x")}, want: []string{aPath + ":14: fragments"}},
|
||||
{name: "credentials in frontmatter URL", files: map[string]string{aPath: sub(adrBase, "implemented_by: []", "implemented_by:\n - https://u:p@example.com/x")}, want: []string{aPath + ":14: credentials"}},
|
||||
{name: "mailto in frontmatter", files: map[string]string{aPath: sub(adrBase, "implemented_by: []", "implemented_by:\n - mailto:a@example.com")}, want: []string{aPath + ":14: scheme"}},
|
||||
{name: "anchor in frontmatter", files: map[string]string{aPath: sub(adrBase, "implemented_by: []", "implemented_by:\n - \"#anchor\"")}, want: []string{aPath + ":14: relative"}},
|
||||
{name: "symlink escape in frontmatter", files: map[string]string{aPath: sub(adrBase, "implemented_by: []", "implemented_by:\n - /docs/out/f.md")}, links: map[string]string{"docs/out": "@outside"}, want: []string{aPath + ":14: outside the repository"}},
|
||||
{name: "percent-encoded self reference", files: map[string]string{aPath: sub(adrBase, "implemented_by: []", "implemented_by:\n - /docs/adr/20261023T204512345Z-use%2Dqueue.md")}, want: []string{aPath + ":14: itself"}},
|
||||
{name: "supersedes rejects plan path", files: map[string]string{aPath: sub(adrBase, "status: proposed", "status: accepted", "supersedes: []", "supersedes:\n - /"+pPath)}, want: []string{aPath + ":10: supersedes"}},
|
||||
{name: "relative path in frontmatter", files: map[string]string{aPath: sub(adrBase, "implemented_by: []", "implemented_by:\n - ../docs/spec.txt")}, want: []string{aPath + ":14: relative"}},
|
||||
|
||||
// 4. Status
|
||||
@@ -298,6 +305,9 @@ func TestCheck(t *testing.T) {
|
||||
{name: "superseder is proposed", files: map[string]string{
|
||||
aPath: sub(adrBase, "supersedes: []", "supersedes:\n - /"+bPath),
|
||||
bPath: sub(bBase, "status: accepted", "status: superseded", "superseded_by: []", "superseded_by:\n - /"+aPath)}, want: []string{aPath + ":9: must be accepted"}},
|
||||
{name: "superseder is deprecated", files: map[string]string{
|
||||
aPath: sub(adrBase, "status: proposed", "status: superseded", "superseded_by: []", "superseded_by:\n - /"+bPath),
|
||||
bPath: sub(bBase, "status: accepted", "status: deprecated", "supersedes: []", "supersedes:\n - /"+aPath)}},
|
||||
{name: "superseder is rejected", files: map[string]string{
|
||||
aPath: sub(adrBase, "status: proposed", "status: rejected", "implementation: not-started", "implementation: not-applicable", "supersedes: []", "supersedes:\n - /"+bPath),
|
||||
bPath: sub(bBase, "status: accepted", "status: superseded", "superseded_by: []", "superseded_by:\n - /"+aPath)}, want: []string{aPath + ":9: must be accepted"}},
|
||||
@@ -325,7 +335,13 @@ func TestCheck(t *testing.T) {
|
||||
{name: "Consequences without prose", files: map[string]string{aPath: sub(adrBase, "* One more service.", "<!-- later -->")}, want: []string{aPath + ":31: has no prose"}},
|
||||
{name: "Decision Outcome without own prose", files: map[string]string{aPath: sub(adrBase, "Chosen option: Queue.\n\n", "")}, want: []string{aPath + ":27: needs prose before"}},
|
||||
{name: "plan missing section", files: map[string]string{pPath: sub(planBase, "## Steps\n\n1. Do it.\n\n", "")}, want: []string{pPath + ":1: missing required section \"Steps\""}},
|
||||
{name: "plan empty H1", files: map[string]string{pPath: sub(planBase, "# Adopt Queue", "#")}, want: []string{pPath + ":7: H1"}},
|
||||
{name: "plan empty H1", files: map[string]string{pPath: sub(planBase, "# Adopt Queue", "#")}, want: []string{pPath + ":8: H1"}},
|
||||
{name: "empty ADR H1 is on its own line", files: map[string]string{aPath: sub(adrBase, "# Use Queue", "#")}, want: []string{aPath + ":16: must equal title"}},
|
||||
{name: "headings inside blockquotes and lists do not count", files: map[string]string{aPath: adrBase + "\n> ## Consequences\n\n* ## Decision Outcome\n"}},
|
||||
{name: "H4 does not end the Decision Outcome prose", files: map[string]string{aPath: sub(adrBase, "Chosen option: Queue.", "#### Detail\n\nChosen.")}},
|
||||
{name: "plan second H1", files: map[string]string{pPath: planBase + "\n# Again\n"}, want: []string{pPath + ":22: more than one H1"}},
|
||||
{name: "plan repeated section", files: map[string]string{pPath: planBase + "\n## Scope\n\nMore.\n"}, want: []string{pPath + ":22: appears more than once"}},
|
||||
{name: "plan section without prose", files: map[string]string{pPath: sub(planBase, "1. Do it.", "<!-- later -->")}, want: []string{pPath + ":14: has no prose"}},
|
||||
{name: "plan Outcome is optional", files: map[string]string{pPath: planBase + "\n## Outcome\n\nDone.\n"}},
|
||||
|
||||
// 6.3 Placeholders
|
||||
@@ -345,6 +361,12 @@ func TestCheck(t *testing.T) {
|
||||
}
|
||||
}
|
||||
|
||||
func TestCheckSymlinkedDocsDir(t *testing.T) {
|
||||
res, err := Check(tree(t, nil, map[string]string{"docs/plans": "@outside"}))
|
||||
must(t, err)
|
||||
expect(t, res.Problems, []string{"docs/plans:1: real directory"})
|
||||
}
|
||||
|
||||
func TestCheckEmptyRoot(t *testing.T) {
|
||||
res, err := Check(t.TempDir())
|
||||
must(t, err)
|
||||
|
||||
@@ -55,6 +55,8 @@ func slugify(title string) string {
|
||||
return strings.Trim(s[:min(len(s), maxSlug)], "-")
|
||||
}
|
||||
|
||||
var closingRe = regexp.MustCompile(`(^|[ \t])#+$`)
|
||||
|
||||
// New creates a record of the given kind ("adr" or "plan") under root and
|
||||
// returns its root-relative slash path. The file is created exclusively; the
|
||||
// timestamp advances by 1 ms on a collision.
|
||||
@@ -63,6 +65,9 @@ func New(root, kind, title string, now time.Time) (string, error) {
|
||||
if title == "" || strings.ContainsAny(title, "\r\n") {
|
||||
return "", errors.New("title must be non-empty and a single line")
|
||||
}
|
||||
if closingRe.MatchString(title) {
|
||||
return "", errors.New(`title must not end in a "#" run: it would be read as a closing sequence of the H1`)
|
||||
}
|
||||
slug := slugify(title)
|
||||
if slug == "" {
|
||||
return "", errors.New("title needs at least one ASCII letter or digit for the file name")
|
||||
|
||||
@@ -16,6 +16,12 @@ func TestLinkDestinations(t *testing.T) {
|
||||
{"#anchor", ""},
|
||||
{"mailto:a@example.com", ""},
|
||||
{"https://example.com/a?b=1#c", ""},
|
||||
{`/docs/spec\.txt`, ""},
|
||||
{"/docs/spec.txt", ""},
|
||||
{"/docs/spec.txt", ""},
|
||||
{`/docs\\spec.txt`, "backslash"},
|
||||
{"/docs/SPEC.txt", "does not exist"},
|
||||
{"/Docs/spec.txt", "does not exist"},
|
||||
{"foo.md", "relative"},
|
||||
{"./foo.md", "relative"},
|
||||
{"../docs/spec.txt", "relative"},
|
||||
@@ -64,6 +70,11 @@ func TestLinkForms(t *testing.T) {
|
||||
{"image", "", []string{":35: relative"}},
|
||||
{"autolink", "<ftp://example.com/x>", []string{":35: scheme"}},
|
||||
{"reference link and definition", "[x][r]\n\n[r]: foo.md", []string{":35: relative", ":37: relative"}},
|
||||
{"duplicate definition is checked", "[a]: /docs/spec.txt\n\n[a]: ../bad.md", []string{":37: relative"}},
|
||||
{"definition in a blockquote", "> [Zed]: ../bad.md", []string{":35: relative"}},
|
||||
{"definition-like line in code", "```\n[a]: x\n```\n\n[a]: ../bad.md", []string{":39: relative"}},
|
||||
{"autolink on a later line", "Para one\nline two\n<ftp://example.com/x>", []string{":37: scheme"}},
|
||||
{"image without alt on a later line", "Para one\nline two\n", []string{":37: relative"}},
|
||||
{"unused definition", "[r]: foo.md", []string{":35: relative"}},
|
||||
{"link in a list", "* [x](foo.md)", []string{":35: relative"}},
|
||||
{"bare URL and raw HTML are not checked", "http://u:p@example.com and @user #123 abc1234\n\n<a href=\"foo.md\">x</a>", nil},
|
||||
|
||||
@@ -85,7 +85,7 @@ func TestNewCollisions(t *testing.T) {
|
||||
}
|
||||
|
||||
func TestNewRejectsBadTitles(t *testing.T) {
|
||||
for _, title := range []string{"", " ", "a\nb", "!!!"} {
|
||||
for _, title := range []string{"", " ", "a\nb", "!!!", "Drop C #", "#", "C ##"} {
|
||||
if path, err := New(t.TempDir(), "adr", title, now); err == nil {
|
||||
t.Errorf("title %q created %s", title, path)
|
||||
}
|
||||
|
||||
+13
-1
@@ -92,6 +92,18 @@ func parseRef(v string, body bool) (ref, string) {
|
||||
|
||||
// exists reports why a repo-root path is not a regular file inside the repository.
|
||||
func (c *checker) exists(p string) string {
|
||||
// Match each component's case exactly, so case-insensitive filesystems agree with Linux.
|
||||
dir := c.root
|
||||
for _, part := range strings.Split(p, "/") {
|
||||
if part == "" || part == "." {
|
||||
continue
|
||||
}
|
||||
entries, err := os.ReadDir(dir)
|
||||
if err != nil || !slices.ContainsFunc(entries, func(e os.DirEntry) bool { return e.Name() == part }) {
|
||||
return "target " + p + " does not exist"
|
||||
}
|
||||
dir = filepath.Join(dir, part)
|
||||
}
|
||||
real, err := filepath.EvalSymlinks(filepath.Join(c.root, filepath.FromSlash(p)))
|
||||
if err != nil {
|
||||
return "target " + p + " does not exist"
|
||||
@@ -156,7 +168,7 @@ func (m *fm) checkRef(spec refSpec, v, self string) string {
|
||||
return ""
|
||||
case k&kFile == 0 && !(k&kAdr != 0 && adrPathRe.MatchString(v)) && !(k&kPlan != 0 && planPathRe.MatchString(v)):
|
||||
return fmt.Sprintf("%q is not accepted here; accepts %s", v, k)
|
||||
case v == self:
|
||||
case r.path == self:
|
||||
return "a record must not reference itself"
|
||||
}
|
||||
return m.c.exists(r.path)
|
||||
|
||||
Reference in new issue
Block a user