diff --git a/.planning/phases/11.1-summercms-documentation-for-humans-and-ai-agents/11.1-REVIEW.md b/.planning/phases/11.1-summercms-documentation-for-humans-and-ai-agents/11.1-REVIEW.md new file mode 100644 index 0000000..6b956c3 --- /dev/null +++ b/.planning/phases/11.1-summercms-documentation-for-humans-and-ai-agents/11.1-REVIEW.md @@ -0,0 +1,375 @@ +--- +phase: 11.1-summercms-documentation-for-humans-and-ai-agents +reviewed: 2026-09-30T22:32:58Z +depth: standard +files_reviewed: 93 +files_reviewed_list: + - cmd/summer/docs.go + - cmd/summer/docs_test.go + - cmd/summer/main.go + - cmd/summer/main_test.go + - cmd/summer/phase11_1_acceptance_test.go + - docs/examples/blog/blog_test.go + - docs/examples/blog/classes/doc.go + - docs/examples/blog/console/doc.go + - docs/examples/blog/console/publish.go + - docs/examples/blog/console/publish_test.go + - docs/examples/blog/controllers/doc.go + - docs/examples/blog/controllers/posts.go + - docs/examples/blog/controllers/posts_test.go + - docs/examples/blog/jobs/doc.go + - docs/examples/blog/middleware/doc.go + - docs/examples/blog/models/doc.go + - docs/examples/blog/models/post.go + - docs/examples/blog/models/post_test.go + - docs/examples/blog/plugin.go + - docs/examples/blog/postgres_test.go + - docs/examples/blog/registry.gen.go + - docs/examples/blog/routes.go + - docs/examples/blog/scaffold_layout_test.go + - docs/examples/blog/updates/20260101000000_create_acme_blog_posts.go + - docs/examples/blog/updates/20260101000100_add_published_at.go + - docs/examples/blog/updates/doc.go + - docs/examples/blog/updates/updates_test.go + - internal/docsite/check_commands.go + - internal/docsite/check_forbidden.go + - internal/docsite/check_identifiers.go + - internal/docsite/check_links.go + - internal/docsite/check_policy.go + - internal/docsite/checks_test.go + - internal/docsite/docsite.go + - internal/docsite/docsite_test.go + - internal/docsite/emit.go + - internal/docsite/emit_test.go + - internal/docsite/highlight.go + - internal/docsite/highlight_test.go + - internal/docsite/load.go + - internal/docsite/load_test.go + - internal/docsite/render.go + - internal/docsite/render_test.go + - internal/docsite/serve.go + - internal/docsite/serve_test.go + - internal/docsite/snippet.go + - internal/docsite/snippet_test.go + - internal/docsite/theme/assets/search.js + - internal/docsite/theme/assets/site.css + - internal/docsite/theme/assets/site.js + - internal/docsite/theme/assets/theme-init.js + - internal/docsite/theme/templates/404.html + - internal/docsite/theme/templates/footer.html + - internal/docsite/theme/templates/header.html + - internal/docsite/theme/templates/icons.html + - internal/docsite/theme/templates/page.html + - internal/docsite/theme/templates/pager.html + - internal/docsite/theme/templates/search.html + - internal/docsite/theme/templates/sidebar.html + - internal/docsite/theme/templates/toc.html + - internal/docsite/theme_test.go + - internal/docsite/violations_test.go + - modules/backpack/example_test.go + - modules/beachcomber/example_test.go + - modules/beachcomber/export_docs_test.go + - modules/beachcomber/typesense/example_test.go + - modules/bonfire/example_test.go + - modules/bouncer/example_test.go + - modules/cabana/example_controller_test.go + - modules/cabana/example_test.go + - modules/compass/example_test.go + - modules/conga/example_test.go + - modules/conga/export_docs_test.go + - modules/festival/example_test.go + - modules/fetchguard/example_test.go + - modules/flare/example_test.go + - modules/lagoon/attach/example_test.go + - modules/lagoon/example_test.go + - modules/lagoon/export_docs_test.go + - modules/lighthouse/centrifugo/example_test.go + - modules/lighthouse/example_test.go + - modules/lighthouse/export_docs_test.go + - modules/pact/example_test.go + - modules/party/example_plugin_test.go + - modules/party/example_test.go + - modules/phrasebook/example_test.go + - modules/postcard/example_test.go + - modules/surf/example_test.go + - modules/tide/example_test.go + - modules/towel/example_test.go + - modules/wire/example_test.go + - modules/wristband/example_test.go + - scripts/check-phase11.1.sh +findings: + critical: 1 + warning: 7 + info: 8 + total: 16 +status: 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-remote` is passed. The handler rejects every dot segment. Paths are cleaned and checked lexically against the build dir. The build dir is written only by `writeOutputs`, which creates no symlinks. +- **--out guard.** It resolves symlinks, and `prepareOut` refuses to empty any unmarked non-empty directory. +- **XSS.** `search.js` builds results with `createElement` and `textContent` only. The templates go through `html/template`, and goldmark runs in safe mode. +- **blog:publish.** The command binds the slug as a parameter. The admin controller's `acme.blog.access_posts` permission is registered in `Permissions()` 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 doc` fallback matches case-insensitively, so identifiers with the wrong case pass. +- `golang` fences 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 `
` 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: + +```go +// 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: + +```go +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: + +```go +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 ./ `. 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: + +```go +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: + +```go +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 ` or `./bin/ `, 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 because `m[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/` as well as `./bin/`. 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 `

`, which comes before the `` in document order. + +`search.js:10` calls `document.getElementById("search")` and gets the `

`. 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 `
`. `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: + +```go +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 `` 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:** + +```bash +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_