21 KiB
phase, reviewed, depth, files_reviewed, files_reviewed_list, findings, status
| phase | reviewed | depth | files_reviewed | files_reviewed_list | findings | status | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| 11.1-summercms-documentation-for-humans-and-ai-agents | 2026-09-30T22:32:58Z | standard | 93 |
|
|
issues_found |
Phase 11.1: Code Review Report
Reviewed: 2026-09-30T22:32:58Z Depth: standard Files Reviewed: 93 Status: issues_found
Summary
The review covered the docs generator (internal/docsite), the summer docs:* commands, the theme (templates and JS), the phase gate script, the acme/blog walkthrough package and the new module example tests.
The security-sensitive surfaces the orchestrator named hold up:
- docs:serve. It refuses non-loopback addresses unless
--allow-remoteis passed. The handler rejects every dot segment. Paths are cleaned and checked lexically against the build dir. The build dir is written only bywriteOutputs, which creates no symlinks. - --out guard. It resolves symlinks, and
prepareOutrefuses to empty any unmarked non-empty directory. - XSS.
search.jsbuilds results withcreateElementandtextContentonly. The templates go throughhtml/template, and goldmark runs in safe mode. - blog:publish. The command binds the slug as a parameter. The admin controller's
acme.blog.access_postspermission is registered inPermissions()and is enforced by cabana (http.go:754).
The main problems are checkers that fail open. Each finding below was reproduced with the built summer binary against a scratch copy of internal/docsite/testdata/clean:
- A
src=fence inside a blockquote or callout is published with a source caption, but its body is never compared with the source. - An Example that has no
// Output:comment, and every helper it calls, counts as "run by go test". - Files excluded by build constraints count as "compiled".
- The
go docfallback matches case-insensitively, so identifiers with the wrong case pass. golangfences and several command-line forms are not checked at all.
There is also one latent theme bug. A heading whose slug matches a theme element ID breaks search on that page.
Critical Issues
CR-01: src= and Go fences inside blockquotes or callouts bypass snippet verification but are still published as "verified source"
File: internal/docsite/render.go:439-489 (scanFences/openFence), used by internal/docsite/snippet.go:451-485 (checkFences), internal/docsite/check_policy.go:399-415 and internal/docsite/check_commands.go:89-102. The annotation happens in internal/docsite/render.go:54-72.
Issue: Every text-level checker finds fences with scanFences. It strips only leading spaces, so it never sees a fence whose lines begin with > . That excludes any code block inside a > [!NOTE], > [!TIP] or > [!WARNING] callout or a plain blockquote. goldmark does parse such a fence. fenceAnnotator walks the AST and adds data-src/data-href, so the rendered page shows a <figcaption> that names and links the source file, even though the body was never compared with it.
Reproduced on the clean fixture: the following text was appended to docs/extras/faq.md.
> [!TIP]
> ```go src=modules/demo/example_test.go#ExampleHello
> BOGUS
> ```
summer docs:build --check printed "no problems found". docs:build then wrote extras/faq.html containing BOGUS under a caption for modules/demo/example_test.go#ExampleHello. A src= path that does not exist also passes, and so does a go fence with no src= inside a callout.
There is a second path with the same result. checkSnippets skips module README pages (snippet.go:509), but fenceAnnotator still annotates their src= fences.
The acceptance test's independent scanner (cmd/summer/phase11_1_acceptance_test.go:418-442) has the same blind spot, so SC4 cannot catch this either. Together these defeat DOCS-04, the promise that every Go example on the site is compiled and run by go test, without any warning. docs:sync will not repair such fences either.
Fix: Collect fences from the goldmark AST that parseDocs already produces, instead of scanning lines. Use *ast.FencedCodeBlock nodes with Info and Lines(), so blockquotes and list items are handled the way the renderer handles them. At minimum, refuse what the line scanner cannot verify:
// in checkPolicy, over d.doc:
_ = gast.Walk(d.doc, func(n gast.Node, entering bool) (gast.WalkStatus, error) {
fc, ok := n.(*gast.FencedCodeBlock)
if !entering || !ok || fc.Info == nil {
return gast.WalkContinue, nil
}
info := string(fc.Info.Segment.Value(d.body))
_, hasSrc := ParseSrc(info)
if hasSrc && (d.page.Module != "" || insideBlockquote(fc)) {
problems = append(problems, Problem{File: d.file, Line: lineOf(d.body, fc.Info.Segment.Start, d.line),
Rule: "snippet", Message: "src= fences must be top-level blocks of a docs page"})
}
return gast.WalkContinue, nil
})
Also make fenceAnnotator skip README pages, or run checkFences for them too. Update the acceptance scanner to strip > prefixes.
Warnings
WR-01: An Example without // Output: is a reachability root, so code go test never runs is accepted as "run"
File: internal/docsite/snippet.go:344-346 (isRoot), :408-441 (checkTestIdent/checkTestRegion)
Issue: isRoot treats every Example* function as a root of the test graph. go test compiles an Example with no // Output: comment but never runs it. When Extract targets an Example directly, it checks for the output comment (exampleHasOutput). Helpers and regions reached only through such an Example pass checkTestIdent/checkTestRegion anyway.
Reproduced: func Example_neverRun() { neverRunHelper() } without an output comment was added. After that, src=…#neverRunHelper and a docs:start never region inside the helper both passed docs:build --check. go test -v shows the Example never runs.
Code after an unconditional t.Skip also passes, for example a region inside a TestSkipped that calls t.Skip("always") first. That case is harder to close, but it should at least be documented.
Fix: When building the graph, count an Example as a root only when doc.Examples(f) reports output for it. Parse the test files with parser.ParseComments so that information is available:
roots := map[string]bool{}
for _, ex := range doc.Examples(f) {
if ex.Output != "" || ex.EmptyOutput {
roots["Example"+ex.Name] = true
}
}
isRoot := func(d *ast.FuncDecl) bool {
return d.Recv == nil && (strings.HasPrefix(d.Name.Name, "Test") || roots[d.Name.Name])
}
WR-02: Go sources excluded by build constraints pass the "compiled by go test" check
File: internal/docsite/snippet.go:102-114
Issue: For a non-test .go file, Extract checks only two things: the file sits in the root module, and a *_test.go exists beside it. It ignores //go:build constraints and _GOOS/_GOARCH file-name suffixes. It also ignores directories that the go tool skips (testdata, _x, .x) when they hold their own _test.go files.
Reproduced: modules/demo/broken.go was created with //go:build ignore and func Broken() { undefinedCall() }. src=modules/demo/broken.go#Broken synced and passed docs:build --check, and go vet ./... stayed green. Code that does not compile was published as verified.
Fix: Use go/build to confirm the file is part of the package under the default context:
pkg, err := build.ImportDir(filepath.Dir(real), 0)
if err != nil || !slices.Contains(append(pkg.GoFiles, pkg.TestGoFiles...), filepath.Base(real)) &&
!slices.Contains(pkg.XTestGoFiles, filepath.Base(real)) {
return "", snippetError("file is excluded from the default build (build constraints or ignored directory)")
}
WR-03: The go doc fallback accepts identifiers with the wrong case
File: internal/docsite/check_identifiers.go:301
Issue: When a lookup misses the index, goDoc runs go doc ./<dir> <query>. In go doc, a lowercase letter in the query matches either case. Reproduced: with func OpenFromApp() in the fixture, the span `demo.Openfromapp` passed docs:build --check. On the real tree, go doc ./modules/lagoon Openfromapp exits 0. A page can therefore name an API that does not compile, and the identifier rule reports nothing.
Fix: Pass -c so symbol matching is case-sensitive:
cmd := exec.CommandContext(ctx, "go", "doc", "-c", "./"+dir, query)
WR-04: golang fences (and other Go aliases) bypass the "go fence needs src=" policy
File: internal/docsite/check_policy.go:409-414
Issue: The policy checks only fields[0] == "go". chroma resolves golang to the Go lexer, so a ```golang fence renders as highlighted Go with no src= requirement at all. Reproduced: ```golang\nx := 1\n``` passed docs:build --check. The acceptance test (docsGoFences, lang == "go") misses it too.
Fix: Normalise the language through chroma before the comparison:
if l := lexers.Get(fields[0]); l != nil && l.Config().Name == "Go" && !hasSrc { ... }
Or reject any fence whose lexer is Go but whose language word is not exactly go.
WR-05: The command checker skips common shell forms, so unknown commands are published unchecked
File: internal/docsite/check_commands.go:35, :66-80
Issue: commandToken matches only summer <name> or ./bin/<app> <name>, and only at the start of a command. Reproduced: the following forms inside an sh fence all passed --check:
FOO=1 summer no:such(an environment-variable prefix)summer --root . no:such(a flag before the command name, which is skipped becausem[2]starts with-)go run ./cmd/summer no:such
bin/acme migrate, without the leading ./, and text fences are never checked either. Any of these forms is a natural way to write a command, and a typo in one ships without a warning.
Fix: Strip leading VAR=value assignments and a go run ./cmd/summer prefix. Skip flags before the command word, and treat a flag's value as consumed when the flag is known to take one. Accept bin/<app> as well as ./bin/<app>. Add planted cases under testdata/violations/ for each form.
WR-06: Heading IDs can collide with theme element IDs, which breaks search on that page and produces invalid HTML
File: internal/docsite/render.go:207-232 (slugIDs); theme IDs in templates/page.html:36, templates/sidebar.html:2, templates/search.html:2-6, templates/page.html:27
Issue: slugIDs starts every page with an empty used set. A heading such as ## Search, ## Content, ## Sidebar or ## Search results therefore gets the same id as a theme element. Reproduced: the built extras/faq.html contained id="search" twice, first on the <h2>, which comes before the <dialog> in document order.
search.js:10 calls document.getElementById("search") and gets the <h2>. The guard typeof dialog.showModal !== "function" then disables search on that page without any message. The skip link #content jumps to the heading instead of <main>. search-results would make fetch(null) report "Search needs a web server". None of these headings exist in the current tree, but nothing prevents them.
Fix: Reserve the theme IDs in every page's slug table. Because the link checker uses the same parser, it will stay consistent:
var themeIDs = []string{"content", "sidebar", "search", "search-input", "search-status",
"search-results", "search-live", "live-region"}
func newSlugIDs() *slugIDs {
s := &slugIDs{used: map[string]bool{}}
for _, id := range themeIDs {
s.used[id] = true
}
return s
}
Alternatively, prefix the theme IDs (sd-search, …) in the templates and JS.
WR-07: The walkthrough's hand-written files carry "Code generated … DO NOT EDIT" headers
File: docs/examples/blog/console/publish.go:1, docs/examples/blog/controllers/posts.go:1, docs/examples/blog/models/post.go:1, docs/examples/blog/updates/*.go:1
Issue: These files contain hand-written logic: the withDB parameter and the COALESCE update in publish.go, RequiredPermissions/NewRecord in posts.go, and Fillable/Rules/NewPost in post.go. They still keep the generated-code header. By default, golangci-lint (named in the stack) and staticcheck skip files marked as generated, so the only hand-written business logic in the walkthrough is never linted. The walkthrough page also teaches readers to edit files marked DO NOT EDIT. The scaffolder gap is tracked in .planning/todos/pending/scaffold-generated-header-and-command-deps.md, but the example should not model the problem.
Fix: Remove the header from files that have been finished by hand, and say so in porting-a-plugin.md. Keep it only on registry.gen.go, which the scaffolder really regenerates. If the scaffolder is going to keep emitting the header, add a sentence to the page telling readers to delete it once they edit the file.
Info
IN-01: A failed write leaves an unmarked output directory that the next build refuses to clean
File: internal/docsite/docsite.go:95-103
Issue: prepareOut deletes everything in the output directory, including the .summer-docs marker. The marker is written again only after every output succeeds. If a write fails partway (for example ENOSPC or EACCES), the directory is left non-empty and unmarked. Every later docs:build then stops with "refusing to clean … it has no .summer-docs marker".
Fix: Write the marker right after prepareOut succeeds and before writeOutputs.
IN-02: scanFences treats 4-space-indented backticks as fenced code, which gives false positives and differs from goldmark
File: internal/docsite/render.go:462-479
Issue: openFence strips any number of leading spaces. Top-level text such as ```go is an indented code block in CommonMark, but it was reported as "go code block has no src= reference" (reproduced).
Fix: Limit the fence indent to 3 spaces relative to the enclosing block, or take fences from the AST (see CR-01).
IN-03: data-href from source_url is HTML-escaped but its URL scheme is not checked
File: internal/docsite/highlight.go:45-46, internal/docsite/render.go:67-69
Issue: edit_url goes through html/template, which neutralises javascript: URLs. source_url is written into a raw href with only html.EscapeString. site.yaml is trusted, but the two URL paths behave inconsistently.
Fix: Validate source_url in ParseSite: require it to start with http:// or https://, and do the same for edit_url.
IN-04: Dead error branch and a nil request in serve.go
File: internal/docsite/serve.go:79-85, :283
Issue: s.watch only ever returns nil, so the watchErr branch that handles non-nil errors can never run. http.NotFound(w, nil) passes a nil *http.Request. It works only because the current stdlib ignores the argument.
Fix: Remove the unreachable branch, or make watch return real errors. Pass r down to notFound.
IN-05: Search highlighting uses offsets from toLowerCase() on the original string
File: internal/docsite/theme/assets/search.js:196-236
Issue: appendMarked and excerpt look up indices in text.toLowerCase() and then slice text. For characters whose lowercase form has a different length (for example U+0130 "İ"), the <mark> ranges shift. This is not an XSS risk, because textContent is still used throughout.
Fix: Compute the indices on a lowercased copy only when lower.length === text.length, and otherwise skip highlighting.
IN-06: The gate's forbidden sweep treats grep errors as "no hits"
File: scripts/check-phase11.1.sh:131-133
Issue: grep -rliE … 2>/dev/null || true discards grep's exit status 2 (for example an unreadable file or a missing path). A failed sweep therefore reads as clean. The Go checker covers the same rule, so the risk is limited to this redundant stage.
Fix:
forbidden_hits() {
local rc=0
grep -rliE "$FORBIDDEN_RE" "$@" || rc=$?
[ "$rc" -le 1 ] || refuse "forbidden sweep: grep failed ($rc)"
}
IN-07: The gate depends on GNU-only tools
File: scripts/check-phase11.1.sh:285, :289, :376
Issue: sed -i without a suffix argument and find -printf fail on BSD/macOS. Under set -e that surfaces as an unexplained exit.
Fix: Use sed -i.bak … && rm ….bak (or perl), and find … -exec dirname {} \;, or document that GNU userland is required.
IN-08: The identifier checker cannot see several span forms
File: internal/docsite/check_identifiers.go:223, :233-236
Issue: A span whose package prefix is not a module directory name returns "" with no check, so a typo like `lagon.Fill` passes. &lagoon.Foo{}, lagoon.Foo{…} and x := lagoon.Foo() do not match identSpan either. docs:serve also does not watch the root README.md or examples/, although both feed the checks.
Fix: Extend identSpan to allow a leading & and a trailing {…}. Consider flagging lowercase pkg.Ident spans whose pkg is close to a module name, for example by edit distance. Add the root README and examples/ to addWatches.
Reviewed: 2026-09-30T22:32:58Z Reviewer: Claude (gsd-code-reviewer) Depth: standard