mirror of
https://github.com/coder/coder.git
synced 2026-09-24 15:04:27 +08:00
Nick Vigilante
9dcb75cd56
## What Adds CI enforcement that fails when docs Markdown contains invalid inline HTML the docs site silently drops or mangles, and fixes the remaining generated-doc placeholders at their source. This is the tooling half of the docs-HTML audit. The hand-written fixes it guards landed in #27298 (kept small and separate so it reviewed fast); this PR carries everything that touches code, CI, or generated output. ## Changes **Linter (`scripts/docshtmlcheck`), wired into `make lint` via `lint/docs-html`.** Markdown-aware: parses each file with goldmark and inspects only raw-HTML nodes, so angle brackets in fenced code blocks, inline code, HTML comments, and `<https://…>` / `<user@host>` autolinks are ignored. Flags swallowed placeholders (`<region>`), void-element end tags (`</br>`), unregistered or incorrectly capitalized component tags (`<Image>`), and unclosed container tags (a `<div class="tabs">` that leaks its wrapper). The one intentional renderer component, `<children>`, is allowed but still balance-checked. **Generator-source placeholder fixes (regenerated via `make gen`).** - `codersdk/chats.go`: backtick `<server>__` in the `ChatContextTool.Name` doc comment (it becomes the Swagger description, so it was swallowed in `reference/api/{chats,schemas}.md`). - `codersdk/deployment.go`: backtick `<region>` in the AWS Bedrock region flag help (swallowed in `reference/cli/server.md`); also updates `coder server --help` output and the golden files. **Temporary allowlist.** `docs/reference/cli/agent-firewall.md`'s `<host>` / `<glob>` come from the external `github.com/coder/boundary` CLI help (still `v0.10.0` on `main`), so they are suppressed on that one file. The suppression is self-clearing: if an allowlisted tag stops appearing on a scanned file, the linter reports `stale-allowlist-entry` and fails until the dead entry is removed, so a dead entry cannot silently mask a later regression of that tag on that page. (An entry whose file is deleted outright is never rescanned, but a missing file yields no findings, so nothing hides behind it either.) ## Review feedback addressed This tool + generator work was reviewed by Coder Agents Review while it was bundled into #27298. Addressed here: - **P1:** tokenize each raw-HTML node as a whole instead of per source line, so a tag whose attributes wrap across lines is no longer torn in half. This fixes both the missed multi-line unclosed `<div>` (a leaked wrapper that passed with exit 0) and the spurious `stray-end-tag` on valid multi-line tags. Each token maps back to its own source line. - Normalize allowlist lookup/report paths to a canonical repo-relative form, so the escape hatch no longer silently misses under absolute / `./` paths. - Route generated-page findings to the generator source. - Add `<search>` to the allowed set; reword the unknown-element message to note that a real element can be added to `allowedElements`. - Self-clearing allowlist guard (above); rename `optionalEndTag(s)` and `kindUnclosed(Tag)`; adopt `slices`/`maps` idioms; move the lint banner to the Makefile recipe; stop aliasing the input slice in `filterAllowed`. - New tests: multi-line tokenization (both classes), interleaved nesting, a pinned line number, `collectMarkdown`, and the stale-allowlist guard. ### Round 2 (Coder Agents Review on this PR) A second `/coder-agents-review` pass on this PR raised 16 findings; addressed in `fix(docshtmlcheck): catch self-closing containers and capitalized tags`: - **P2:** self-closing container tags (`<div class="tabs"/>`) were ignored by the HTML5 parser and leaked their wrapper like the open spelling; the balance check now tracks self-closing tokens too (CRF-1). - **P2:** a capitalized component tag whose lowercase name is a real element (`<Table>`, `<Section>`) slipped through on the `allowedElements` lookup. The tokenizer lowercases tag names, so the check now reads the raw token and reports any capitalized name as a component reference (CRF-2). - Narrowed the `:` / `@` autolink skip to a real URI scheme or a dotted `local@domain`, so `<region:id>` and `<user@host>` stay checked (CRF-3). - Stale-allowlist findings now report against the linter source with no line, and count separately from invalid-HTML issues in the footer (CRF-7, CRF-11). - Comment / README / Makefile wording synced to the honest capitalized-tag behavior; added the deleted-file allowlist caveat and a note that `allowedElements` is hand-maintained against the renderer (CRF-14, CRF-17, CRF-9). - Internal cleanups (`pop` -> `matchEndTag`, extracted `unclosedFinding`) and new tests: self-closing, capitalized open/close, colon/at placeholders, a non-first-token line assertion, `isGeneratedDoc`, and the stale message (CRF-12, CRF-13, CRF-1/2/3/4/5/16). Two findings resolved without a code change: - **CRF-8** (also wire `lint/docs-html` into `lint-light`): declined. `lint-light` is the Go-free fast path; `lint/docs-html` needs the Go toolchain, so it stays in the full `make lint`, which CI runs. Adding it would pull Go into the light path for no coverage gain. - **CRF-9** (`allowedElements` <-> renderer coupling): documented with a maintenance note in the `allowedElements` comment and tracked in DOCS-597 for a cross-repo sync/check decision. Deferred (note, no current trigger): raw-text element interiors (`<script>` / `<style>`) are not scanned for nested tags. No docs page relies on this today; noted for follow-up. ## Merge order #27298 (the hand-written fixes this PR guards) has merged, and this branch is rebased on `main`, so `make lint/docs-html` now reports 0 findings and the `lint` check passes. The two PRs are independent (disjoint files, no stacking). ## Verification - `go test ./scripts/docshtmlcheck/`, `go vet`, `gofmt -l`, `golangci-lint run`: clean. - `make lint/docs-html` (branch rebased on `main`): 0 findings. ## Linear - DOCS-584: https://linear.app/codercom/issue/DOCS-584/add-ci-check-that-fails-on-invalid-inline-html-in-docs - DOCS-551: https://linear.app/codercom/issue/DOCS-551/backtick-placeholder-syntax-in-generated-reference-docs-cli-help - DOCS-597 (follow-up, from CRF-9): https://linear.app/codercom/issue/DOCS-597/track-docshtmlcheck-allowedelements-drift-vs-docs-renderer-component > This PR was created with AI assistance (Coder Agents).
73 lines
3.6 KiB
Markdown
73 lines
3.6 KiB
Markdown
# docshtmlcheck
|
|
|
|
`docshtmlcheck` fails CI when Markdown under `docs/` contains invalid inline
|
|
HTML that the documentation site's renderer silently drops or mangles. It runs
|
|
as `make lint/docs-html` (part of `make lint`).
|
|
|
|
## What it catches
|
|
|
|
- **Swallowed angle-bracket placeholders.** An unwrapped placeholder such as
|
|
`<region>` or `<server>__` is parsed as an unknown HTML tag and stripped from
|
|
the rendered page, so readers see broken text. Wrap placeholders in backticks
|
|
so they render as inline code (see
|
|
[`docs/about/contributing/documentation.md`](../../docs/about/contributing/documentation.md#placeholders-in-angle-brackets)).
|
|
This also covers CLI `--help` strings and Swagger annotations, whose text is
|
|
generated into `docs/reference/**`.
|
|
- **Void-element end tags** such as `</br>`. Void elements like `<br>`, `<img>`,
|
|
and `<hr>` have no end tag.
|
|
- **Capitalized or unregistered component tags** such as `<Image>` or `<Table>`.
|
|
The docs renderer reads a capitalized tag as a component reference and drops
|
|
it unless the component is registered (only the lowercase `<children>`
|
|
directive is). Any name outside the standard HTML5 element set is reported the
|
|
same way.
|
|
- **Unclosed container tags**, for example a `<div class="tabs">` that is never
|
|
closed and leaks its wrapper over the rest of the page.
|
|
|
|
## How it works
|
|
|
|
Each file is parsed with [goldmark](https://github.com/yuin/goldmark) and only
|
|
raw-HTML nodes are inspected, so angle brackets inside fenced code blocks,
|
|
inline code spans, HTML comments, and `<https://…>` / `<user@host>` autolinks
|
|
are ignored. Each raw-HTML node is tokenized as a whole with
|
|
`golang.org/x/net/html`, so a tag whose attributes wrap across lines is not
|
|
torn in half. A tag whose raw name is capitalized is reported as a component
|
|
reference; otherwise any name outside the standard HTML5 element set (plus the
|
|
intentional `<children>` renderer component, which is still balance-checked) is
|
|
reported. Inline SVG and MathML are intentionally **not** in the allowed set (no
|
|
docs page uses them); add the element to `allowedElements` in `main.go` if that
|
|
changes. A finding on a generated page under `docs/reference/**` also prints a
|
|
note pointing at the generator source, since edits to the generated file do not
|
|
persist.
|
|
|
|
## Limitations
|
|
|
|
A few gaps are accepted because no docs page hits them today:
|
|
|
|
- A placeholder whose name is itself a real HTML element (`<input>`, `<time>`)
|
|
is indistinguishable from intended markup and passes. Such placeholders
|
|
almost always live in fenced code blocks, which are ignored.
|
|
- The interior of a raw-text element (`<script>`, `<style>`) is a single opaque
|
|
token to the HTML tokenizer, so a tag nested inside one is not scanned. An
|
|
unclosed `<script>`/`<style>` is still caught.
|
|
|
|
## Usage
|
|
|
|
```console
|
|
$ go run ./scripts/docshtmlcheck # scans docs/
|
|
$ go run ./scripts/docshtmlcheck path/to/file.md path/to/dir
|
|
```
|
|
|
|
## Allowlist
|
|
|
|
`allowedUnknownTags` in `main.go` is a deliberately narrow, per-file escape
|
|
hatch for placeholders whose source is outside this repository (so they cannot
|
|
be fixed by a source edit here). It currently holds a temporary entry for
|
|
`docs/reference/cli/agent-firewall.md` (`<host>`/`<glob>`, generated from the
|
|
external `github.com/coder/boundary` CLI help).
|
|
|
|
The escape hatch is **self-clearing**: if an allowlisted tag no longer appears
|
|
in its file (for example once the upstream fix and dependency bump land and the
|
|
generated page no longer emits the bare placeholders), the linter reports a
|
|
`stale-allowlist-entry` and fails until the dead entry is removed, so a
|
|
suppression can never silently mask a later regression of the same tag.
|