amadan
app: fix round 1 -- revert shell rewrite, gate the attribute grammar at the source
Reverts shellAttributeGrammar: rewriting rastrillo's vendored shell bytes to satisfy a rendered-page test was the wrong fix. The test contradicted the brief's own rule that rastrillo's output is "theirs and expected, ours must not" -- and a string substitution over a dependency's markup fails silently the day rastrillo's shell text changes, which it is slated to before 1.0. Fix is at the source instead: TestOurTemplatesUseTheAttributeGrammar (render_test.go) walks this app's own embedded views/*.html and fails on any class="rst-*" byte in a template this app authors -- correct by construction, since rastrillo's own partials and shell are never in that tree. Proved it bites by planting one and watching it fail, then reverting. Caught a real false positive along the way: list.html's own header comment described the convention in prose that literally contained the banned substring; reworded it. list_test.go's rendered-page check stays as a secondary confirmation, now allowlisting rastrillo's two known, named shell occurrences (rst-skip, rst-page) instead of asserting zero unconditionally.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011F5CivVm4CmL9ByfSSAaXA
4 files changed,
+89
−33
internal/docs/list_test.go+24 −2internal/docs/render.go+1 −26internal/docs/render_test.go+56 −0internal/docs/views/list.html+8 −5
diff --git a/internal/docs/list_test.go b/internal/docs/list_test.go| index f1d7a4a..21def7a 100644 |
| --- a/internal/docs/list_test.go |
| +++ b/internal/docs/list_test.go |
| @@ -4,12 +4,32 @@ import ( |
| "context" |
| "net/http" |
| "net/http/httptest" |
| + "regexp" |
| "strings" |
| "testing" |
| "github.com/carlosframework/docs/internal/docs" |
| ) |
| +// classVocabPattern finds every class="rst-*" attribute on a rendered |
| +// page. shellClassVocab allowlists the two rastrillo's own vendored |
| +// column shell hard-codes outside any block this app can override — |
| +// the skip link (class="rst-skip") and the <main> wrapper |
| +// (class="rst-page"), ui/layouts/column.html in rastrillo v0.21.0. |
| +// That is rastrillo's output, not this app's markup, and is not |
| +// something render_test.go's TestOurTemplatesUseTheAttributeGrammar |
| +// — the test that actually gates this app's own templates — ever |
| +// sees, since it walks only this app's own viewsFS. This rendered-page |
| +// check exists as a secondary, real-page confirmation that nothing |
| +// else on the page slipped in the class spelling; it is not the test |
| +// this app depends on to catch a class in a template it authors. |
| +var classVocabPattern = regexp.MustCompile(`class="rst-([a-zA-Z-]+)"`) |
| + |
| +var shellClassVocab = map[string]bool{ |
| + "skip": true, |
| + "page": true, |
| +} |
| + |
| func TestListShowsDocuments(t *testing.T) { |
| app, _ := docs.NewForTest(t) |
| ctx := context.Background() |
| @@ -28,8 +48,10 @@ func TestListShowsDocuments(t *testing.T) { |
| if !strings.Contains(body, "Board minutes") { |
| t.Error("the document's title is not on the page") |
| } |
| - if strings.Contains(body, `class="rst-`) { |
| - t.Error("class vocabulary found — this app writes the attribute grammar (Global Constraints)") |
| + for _, m := range classVocabPattern.FindAllStringSubmatch(body, -1) { |
| + if !shellClassVocab[m[1]] { |
| + t.Errorf("class vocabulary found (class=%q) outside rastrillo's own shell — this app writes the attribute grammar (Global Constraints)", `rst-`+m[1]) |
| + } |
| } |
| } |
diff --git a/internal/docs/render.go b/internal/docs/render.go| index c74e447..8883a29 100644 |
| --- a/internal/docs/render.go |
| +++ b/internal/docs/render.go |
| @@ -7,7 +7,6 @@ import ( |
| "html/template" |
| "io/fs" |
| "net/http" |
| - "strings" |
| "testing/fstest" |
| "github.com/carlosframework/rastrillo" |
| @@ -111,7 +110,7 @@ func newRenderer() (*renderer, error) { |
| if !ok { |
| return nil, fmt.Errorf("docs: rastrillo ui: shell %q not shipped", shellLayout) |
| } |
| - base, err = base.Parse(string(shellAttributeGrammar(layout))) |
| + base, err = base.Parse(string(layout)) |
| if err != nil { |
| return nil, fmt.Errorf("docs: parse rastrillo shell %q: %w", shellLayout, err) |
| } |
| @@ -131,30 +130,6 @@ func newRenderer() (*renderer, error) { |
| return &renderer{pages: pages, assets: assets}, nil |
| } |
| -// shellAttributeGrammar rewrites the two class="rst-*" spots |
| -// rastrillo's vendored column shell hard-codes — the skip link and |
| -// the <main> wrapper — into their bare-attribute twins, [rst-skip] |
| -// and [rst-page]. Neither is behind an overridable {{block}}, so an |
| -// app cannot reach them by redefining a shell block the way it can |
| -// "brand" or "nav": this is the one place the shell's own bytes have |
| -// to be edited instead. |
| -// |
| -// This changes no styling. tokens.css's header comment ("TWO |
| -// SPELLINGS, ON PURPOSE, FOR ONE RELEASE") pairs every rst- class with |
| -// an attribute selector of equal specificity precisely so a caller can |
| -// do this — ui_test.go's TestEveryClassSelectorHasAnAttributeTwin |
| -// holds both the pairing and the specificity upstream. Skipping this |
| -// step would leave two class="rst-*" occurrences on every page this |
| -// app serves, regardless of what markup Task 3 and its neighbors write |
| -// themselves — this app's own "never class=\"rst-*\"" rule has to |
| -// cover the shell, not just the content each page owns. |
| -func shellAttributeGrammar(layout []byte) []byte { |
| - s := string(layout) |
| - s = strings.ReplaceAll(s, `class="rst-skip"`, `rst-skip`) |
| - s = strings.ReplaceAll(s, `class="rst-page"`, `rst-page`) |
| - return []byte(s) |
| -} |
| - |
| // page renders the named page's "layout" template — its own content |
| // wrapped in rastrillo's shell — into w. |
| func (rd *renderer) page(w http.ResponseWriter, r *http.Request, name string, data any) error { |
diff --git a/internal/docs/render_test.go b/internal/docs/render_test.go| new file mode 100644 |
| index 0000000..e6bcc77 |
| --- /dev/null |
| +++ b/internal/docs/render_test.go |
| @@ -0,0 +1,56 @@ |
| +package docs |
| + |
| +import ( |
| + "io/fs" |
| + "strings" |
| + "testing" |
| +) |
| + |
| +// TestOurTemplatesUseTheAttributeGrammar holds the markup constraint |
| +// at the source, not the rendered page: every HTML file this app |
| +// authors under views/ (embedded as viewsFS) must write the bare |
| +// rst-* attribute grammar (<div rst-box>), never class="rst-*". |
| +// |
| +// This is deliberately a package-internal test, not a docs_test one, |
| +// because it needs viewsFS itself — walking our own template sources |
| +// rather than a rendered response is the whole point: rastrillo's own |
| +// partials and shell emit class="rst-*", and that is their output, |
| +// not this app's markup, so it must never appear in this walk in the |
| +// first place, rather than being screened out after the fact. |
| +// |
| +// This replaces what used to be a rendered-page assertion |
| +// ("no class=\"rst-\" anywhere in the response body"), which could |
| +// not tell the two apart: rastrillo's column shell hard-codes |
| +// class="rst-skip" and class="rst-page" outside any block this app |
| +// can override, so that blanket check failed on every page this app |
| +// serves regardless of what its own templates wrote — see git history |
| +// on this file for the reverted fix (rewriting rastrillo's shell |
| +// bytes at parse time) and why it was wrong: a string substitution |
| +// over a dependency's markup fails silently the day rastrillo changes |
| +// those two spots, which it has already said it will before 1.0. |
| +// Checking our own sources instead needs no such rewrite, is correct |
| +// (rastrillo's vendored output is not ours to hold to our own |
| +// convention), and stays stable across a rastrillo upgrade that |
| +// changes its shell's own spelling, because it does not depend on |
| +// rastrillo's internal template text at all. |
| +func TestOurTemplatesUseTheAttributeGrammar(t *testing.T) { |
| + err := fs.WalkDir(viewsFS, ".", func(path string, d fs.DirEntry, err error) error { |
| + if err != nil { |
| + return err |
| + } |
| + if d.IsDir() { |
| + return nil |
| + } |
| + b, err := fs.ReadFile(viewsFS, path) |
| + if err != nil { |
| + return err |
| + } |
| + if strings.Contains(string(b), `class="rst-`) { |
| + t.Errorf("%s: class vocabulary found — this app writes the attribute grammar (Global Constraints)", path) |
| + } |
| + return nil |
| + }) |
| + if err != nil { |
| + t.Fatalf("walk viewsFS: %v", err) |
| + } |
| +} |
diff --git a/internal/docs/views/list.html b/internal/docs/views/list.html| index b7d2c1b..c84292f 100644 |
| --- a/internal/docs/views/list.html |
| +++ b/internal/docs/views/list.html |
| @@ -3,11 +3,14 @@ |
| through rastrillo.T, EmptyTitle/EmptyBody the same; a row's Title |
| and When are the document's own data and are printed as-is. |
| - Writes the bare rst-* attribute grammar throughout, never |
| - class="rst-*" — this app's whole-screen markup convention (Task |
| - 1's home page set it; render.go's shellAttributeGrammar carries |
| - it into the two spots rastrillo's own shell hard-codes). The |
| - three [docs-*] selectors below are this app's own vocabulary, not |
| + Writes the bare rst-* attribute grammar throughout — the rst- |
| + vocabulary always as a bare attribute, never as a class — which is |
| + this app's whole-screen markup convention (Task 1's home page set |
| + it). render_test.go's TestOurTemplatesUseTheAttributeGrammar walks |
| + this file's own bytes to hold that; rastrillo's vendored shell and |
| + partials keep their own class spelling, which is their output, not |
| + this app's, and is out of that test's reach on purpose. The three |
| + [docs-*] selectors below are this app's own vocabulary, not |
| rastrillo's — SKILL.md §7's "ui is rastrillo's vocabulary, not |
| yours" is why they are not spelled rst-*, and they carry their |
| own tiny stylesheet here rather than a static/ file this |