Skip to content

Commit 3b921e6

Browse files
committed
Rewrite summary-rendering check with AST-aware tokenizer
Addresses 15 issues from the multi-angle code review. The previous regex scanner had real correctness gaps and ergonomic limitations. Bugs the old scanner had: 1. Brace inside string literals (e.g. `{{ printf "{%s}" .Params.summary }}`) silently skipped the action — the naive `{{[^{}]*}}` regex stopped at the inner `{`. 2. Positional renderSummary regex matched when `.Params.summary` was nested in a non-RenderString call: `{{ .RenderString (printf "%s" .Params.summary) }}` falsely passed. 3. Piped renderSummary regex matched any later `.RenderString` mention: `{{ .Params.summary | print "x" .Page.RenderString }}` falsely passed because `.RenderString` appears after the pipe. 4. ifPredicate anchored to exact `if .Params.summary`, so compound forms (`if and .Params.summary $cond`) and `else if` were flagged as violations even though they only check presence. 5. Variable assignment `{{ $s := .Params.summary }}` was flagged indiscriminately; subfield access `{{ if .Params.summary.X }}` was flagged because the predicate regex required exact end. 6. Hugo comments mentioning the field (`{{/* .Params.summary */}}`) were treated as live references. The new scanner: - Tokenizes actions with quote-aware lexing (handles double-quoted and backtick strings; `{` and `}` inside strings no longer split actions) — fixes #1. - Strips `{{/* ... */}}` comments before scanning (preserves line numbers via newline padding) — fixes #6. - Classifies each action by leading keyword. `if` / `else if` / `range` are presence predicates regardless of compound form or subfield access — fixes #4 and the subfield case. `with` / `else with` are flagged as rebinding the dot. - Pipeline analysis: splits on `|` at paren-depth 0, walks each stage. `.Params.summary` is safe only when it is a top-level positional argument to `.RenderString` or the head of a pipeline whose terminal stage is `.RenderString`. Nested references inside parens are flagged — fixes #2 and #3. Other fixes in this commit: - `baseof.html` exemption is now by relative path (`_default/baseof.html`), not basename. Hugo idiomatically supports per-type baseof overrides; basename-only would silently exempt all of them. - The walker collects I/O errors into a side slice and continues, so a transient ReadFile failure on one file no longer masks every violation in the remaining files. - `layoutsPath` deleted; the walk calls the existing `repoRoot(t)` helper from messaging_test.go. - Three duplicated regex blocks collapsed into package-level vars and a single `scanSummaryViolations` helper. The multi-line test no longer round-trips through a tempfile. - baseof.html's meta description pipes the summary through `$.RenderString (dict "display" "inline") . | plainify` so backticks become `<code>` HTML and then plain text. SEO snippets ship clean prose instead of literal Markdown punctuation. - The pipe-form alternation (YAGNI in the previous regex) is generalized by the pipeline walker; no current template uses the piped form but the scanner handles it correctly when one does. Verified by: 19-case `TestClassifyAction_TableDriven` covering every safe and unsafe shape, plus three scenario tests (`TestFindActions_BalancedStrings`, `TestScanSummaryViolations_CommentsIgnored`, `TestScanSummaryViolations_MultiLineWith`, `TestScanSummaryViolations_BraceInString`). The real-layout walk (`TestSummaryFrontMatterRenderedThroughRenderString`) passes against the current `website/layouts/` tree, and the rendered meta description on the progressive-disclosure page now shows "Use <?catalog?> ..." rather than "Use \`<?catalog?>\` ...". docs/development/website-config.md updated to enumerate the safe and forbidden forms and explain the baseof.html meta-description plainify path. https://claude.ai/code/session_01Ly1xEwP5pfsLrtTCTKBQE3
1 parent 515559e commit 3b921e6

3 files changed

Lines changed: 610 additions & 200 deletions

File tree

docs/development/website-config.md

Lines changed: 47 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -103,33 +103,56 @@ text, edit the source file and run the sync.
103103
## Summary front-matter rendering
104104

105105
Each docs page carries a `summary` front-matter field.
106-
The field holds inline Markdown. The templates render it
106+
The field holds inline Markdown. Templates render it
107107
through Hugo's `.RenderString` so backticks become
108108
`<code>` and `[text](url)` becomes `<a>`.
109109

110110
A Go test in [`template_summary_test.go`][tpl-test]
111-
walks `website/layouts/**/*.html`. Each template action
112-
that references `.Params.summary` must take one of three
113-
forms. The forms are the predicate
114-
`{{ if .Params.summary }}`, the negated predicate
115-
`{{ if not .Params.summary }}`, or any action that calls
116-
`.RenderString`.
117-
118-
Any other shape fails the test. The regression this
119-
guards against is
120-
`{{ with .Params.summary }}...{{ . }}{{ end }}`. The
121-
`with` rebinds the dot to the summary string. The inner
122-
`{{ . }}` then emits the value raw. A value with
123-
backticks ships as literal backticks instead of `<code>`
124-
tags. The bare `{{ .Params.summary }}` form carries the
125-
same defect without the rebinding.
126-
127-
`baseof.html` is exempt. Its meta-description fallback
128-
emits plain text on purpose; the `<meta>` tag does not
129-
accept HTML.
130-
131-
The test scans each file as one string, not line by
132-
line. A multi-line action that spans newlines is still
133-
caught.
111+
walks `website/layouts/**/*.html`. The scanner
112+
classifies every template action that mentions
113+
`.Params.summary`. It tokenizes actions itself and
114+
respects quoted strings, so braces inside string
115+
literals do not hide an action from the scan.
116+
`{{/* ... */}}` comments are stripped first so a
117+
comment that mentions the field is not flagged.
118+
119+
Safe forms:
120+
121+
- A presence predicate that does not emit output:
122+
`{{ if .Params.summary }}`, `{{ if not .Params.summary }}`,
123+
the compound forms `{{ if and .Params.summary $cond }}`
124+
and `{{ if or .Params.summary $other }}`, the `else if`
125+
variant, and subfield access such as
126+
`{{ if .Params.summary.HTML }}`.
127+
- A `.RenderString` call where `.Params.summary` is a
128+
top-level positional argument:
129+
`{{ .RenderString (dict "display" "inline") .Params.summary }}`.
130+
- A pipeline whose terminal stage is `.RenderString` and
131+
whose head is `.Params.summary`:
132+
`{{ .Params.summary | .RenderString }}` or
133+
`{{ .Params.summary | strings.TrimSpace | .RenderString }}`.
134+
135+
Forbidden forms:
136+
137+
- `{{ with .Params.summary }}` and `{{ else with .Params.summary }}`
138+
these rebind `.` to the summary string and the body
139+
typically emits the value raw.
140+
- The bare `{{ .Params.summary }}` action.
141+
- Variable assignment `{{ $s := .Params.summary }}`
142+
the bound name escapes the per-action scan. Authors
143+
who want a local variable should inline the call
144+
twice instead.
145+
- `.Params.summary` nested inside a non-`.RenderString`
146+
call (`{{ .RenderString (printf "%s" .Params.summary) }}`)
147+
or piped into a function other than `.RenderString`
148+
(`{{ .Params.summary | print .X }}`).
149+
150+
`website/layouts/_default/baseof.html` is exempt by
151+
relative path. Its meta-description fallback emits the
152+
summary as plain text: the `<meta>` content attribute
153+
cannot carry HTML. The template renders the field
154+
through `$.RenderString` then `| plainify`. Backticks
155+
become `<code>` HTML and then plain text. SEO snippets
156+
see clean prose, not literal Markdown punctuation.
134157

135158
[tpl-test]: ../../internal/release/template_summary_test.go

0 commit comments

Comments
 (0)