KofTwentyTwo/license-tool
2
0 Fork 0
Code Issues 2 Pull Requests 9 Actions Packages Projects Releases 6 Wiki Activity

apply: HTML header before <!DOCTYPE> triggers browser quirks mode #8

New Issue
Closed
opened 2026-06-04 16:07:09 +00:00 by KofTwentyTwo · 2 comments
KofTwentyTwo commented 2026-06-04 16:07:09 +00:00 (Migrated from github.com)
Copy link
Copy source

Summary

When apply adds a license header to an HTML file, it inserts the header before the document's <!DOCTYPE html> declaration. The XML/HTML file type in the builtin table preserves a leading <?xml ... ?> declaration but does not preserve a leading <!DOCTYPE ...>. Because the doctype is no longer the first thing the browser sees, the page is parsed in quirks mode instead of standards mode, which silently changes layout, box-model, and CSS behavior at runtime.

Per the HTML spec, the doctype must be the very first content of the document (after an optional BOM). An HTML comment (<!-- ... -->, which is exactly the comment syntax this tool writes for HTML) placed before <!DOCTYPE html> triggers quirks mode in all major browsers. This is the same class of bug as the recently-fixed shebang issue (PR #5 / v0.2.1): a construct that MUST lead the file is not being treated as preserve-first, so the header gets spliced in front of it and breaks the file.

Type / Severity

  • Type: Bug (file-corrupting apply mutation for a supported file type)
  • Severity: High. apply --write produces a structurally different HTML document. The output is still valid HTML so neither check nor any compiler catches it; the regression only manifests as visual/CSS differences in a browser (quirks vs. standards mode). It is silent and easy to ship.

Reproduction

Build the tool and run apply against a standards-mode HTML file.

Sample file index.html:

<!DOCTYPE html>
<html lang="en">
  <head>
    <meta charset="utf-8" />
    <title>Hello</title>
  </head>
  <body>
    <p>Hello, world.</p>
  </body>
</html>

Commands:

# minimal config so apply has a license/holder to render
cat > .license-tool.yaml <<'EOF'
license: MIT
holder: Example Corp
year: "2026"
style: reuse
EOF

go run ./cmd/license-tool apply --write index.html
head -n 5 index.html

Observed — the header (an HTML comment) is inserted at byte 0, pushing the doctype below it:

<!--
  SPDX-License-Tool: license-tool:managed
  SPDX-FileCopyrightText: 2026 Example Corp
  SPDX-License-Identifier: MIT
-->

<!DOCTYPE html>
<html lang="en">
  ...

Any browser now renders index.html in quirks mode because <!DOCTYPE html> is no longer first.

Expected — the doctype stays first; the header is inserted immediately after it:

<!DOCTYPE html>
<!--
  SPDX-License-Tool: license-tool:managed
  SPDX-FileCopyrightText: 2026 Example Corp
  SPDX-License-Identifier: MIT
-->

<html lang="en">
  ...

(For an .xml/.svg file with a leading <?xml?> and a doctype, the doctype must also be preserved, after the XML declaration: <?xml?> then <!DOCTYPE?> then header.)

Root cause

The XML/HTML file type does not list a preserve-first rule for a leading <!DOCTYPE>, and neither boundary computation knows how to consume one.

  1. Filetype table — internal/filetype/filetype.go, the XML/HTML entry (lines 147-153):

    {
    Name:          "XML/HTML",
    Extensions:    []string{".xml", ".html", ".htm", ".xhtml", ".svg", ".pom"},
    Filenames:     []string{"pom.xml"},
    CommentStyle:  block("<!--", "-->"),
    PreserveFirst: []model.PreserveRule{after(model.PreserveBOM), after(model.PreserveXMLDecl)},
}

    It preserves PreserveBOM and PreserveXMLDecl but has no rule for the doctype.

  2. Render insert path — internal/render/render.go, preserveBoundary (lines 320-367). The switch rule.Kind handles PreserveBOM, PreserveShebang, PreserveXMLDecl, PreservePHPOpen, and PreserveCodingPragma. There is no case that advances past a <!DOCTYPE ...> line, so insertAt (lines 269-286) computes a boundary that sits before the doctype and splices the header there.

  3. Detect path — internal/detect/detect.go, PreserveBoundary (lines 171-219). The contiguous-prefix loop consumes lines matching allowShebang, allowXML, allowPHP, and allowPragma only. A <!DOCTYPE ...> line is not consumable, so the boundary stops before it (mirroring the render bug — both boundaries must agree per the render.go package-doc safety invariants).

  4. Missing enum — internal/model/model.go, the PreserveKind constants (lines 148-162): PreserveShebang, PreserveXMLDecl, PreservePHPOpen, PreserveBOM, PreserveCodingPragma, PreservePackageDecl. There is no PreserveDoctype kind to attach a rule to.

This is exactly the shebang situation (v0.2.1): a "must-be-first" construct was not in the preserve-first machinery, so the header was written in front of it.

Proposed fix

Follow the universal-shebang precedent (PR #5, v0.2.1), but scope the doctype handling to the markup file types rather than making it truly universal — a <!DOCTYPE> only leads HTML/XML/SVG documents, not arbitrary languages, so a per-rule approach (like PreserveXMLDecl) is the right shape. Both boundary implementations must stay byte-for-byte in agreement (the render.go package doc calls this out as a safety invariant), and both must order the doctype after any BOM and <?xml?> declaration, since the doctype follows the XML declaration when both are present.

  1. Add a PreserveDoctype kind — internal/model/model.go, in the PreserveKind const block (lines 148-162). Append it (e.g. after PreservePackageDecl) so existing iota values do not shift. Document it as <!DOCTYPE ...>.

  2. Filetype table — internal/filetype/filetype.go, XML/HTML entry. Add after(model.PreserveDoctype) to PreserveFirst, ordered after PreserveXMLDecl:

    PreserveFirst: []model.PreserveRule{
    after(model.PreserveBOM),
    after(model.PreserveXMLDecl),
    after(model.PreserveDoctype),
},

    Consider whether the doctype rule should apply to all current extensions or be split: <!DOCTYPE html> is the HTML case (.html, .htm, .xhtml), while .xml/.svg/.pom use <?xml?> and rarely carry a doctype. The matcher (next point) is case-insensitive and only fires on a real <!doctype token, so listing the rule on the shared XML/HTML entry is safe for both — but if you prefer to scope it, split XML/HTML into an HTML entry (.html/.htm/.xhtml, with the doctype rule) and a XML/SVG entry (.xml/.svg/.pom/pom.xml, without it). Splitting is cleaner semantically; the shared-rule approach is a smaller diff. Either is acceptable as long as the boundary matcher is doctype-specific.

  3. Render insert path — internal/render/render.go, preserveBoundary (lines 339-365). Add a case model.PreserveDoctype to the rule switch, mirroring the existing PreserveXMLDecl case, matching a leading doctype case-insensitively:

    case model.PreserveDoctype:
    pos = advanceLineIf(content, pos, func(line string) bool {
        return strings.HasPrefix(strings.ToLower(strings.TrimSpace(line)), "<!doctype")
    })

    This reuses the existing advanceLineIf helper (lines 382-401), which is already CRLF-agnostic, so BOM/CRLF/trailing-newline invariants are preserved automatically.

  4. Detect path — internal/detect/detect.go, PreserveBoundary (lines 171-219). Add an allowDoctype := hasRule(ft, model.PreserveDoctype, false) alongside allowXML/allowPHP/allowPragma (near lines 187-190), and add a case to the consume loop (near lines 201-210):

    case allowDoctype && strings.HasPrefix(strings.ToLower(strings.TrimSpace(trimmed)), "<!doctype"):
    consumed = true

    Keep the doctype gated on the rule list (not universal) so it never fires on non-markup types. Because the loop consumes contiguous prefix lines in file order, a <?xml?> then <!DOCTYPE> stack is handled naturally, matching the render path.

  5. Coverage gate — the repo enforces 100% total coverage via .testcoverage.yml (CI runs go-test-coverage; render.go, detect.go, and filetype.go are NOT in the exclude list). Every new branch (the new case in both boundaries, the allowDoctype true/false paths, and any new filetype entry/extension) must be exercised by tests or CI fails. None of the three touched files is eligible for exclusion.

Tests & acceptance criteria

Mirror the existing shebang tests (the v0.2.1 fix added shebang cases in internal/render/render_test.go and internal/detect/detect_test.go; grep for shebang/#! there for the pattern), and the filetype tests in internal/filetype/filetype_test.go.

Acceptance criteria:

  • apply --write on the sample index.html above leaves <!DOCTYPE html> as the first line and inserts the header immediately after it (Expected output in Reproduction).
  • Doctype matching is case-insensitive: <!doctype html>, <!DOCTYPE HTML>, and <!DOCTYPE html PUBLIC "..."> (legacy/XHTML doctypes) are all preserved.
  • BOM + doctype: a file starting with a UTF-8 BOM then <!DOCTYPE html> keeps the BOM first, then the doctype, then the header. (BOM/CRLF/trailing-newline invariants from the render.go package doc still hold.)
  • XML declaration + doctype: <?xml?> then <!DOCTYPE ...> (e.g. an XHTML or SVG file) preserves both, in order, before the header.
  • render.preserveBoundary and detect.PreserveBoundary return the same offset for every doctype fixture (the boundaries-must-agree invariant). Add a fixture asserting both.
  • Idempotency: running apply --write twice on a doctype-leading HTML file is a no-op on the second run (no stacked headers, no doctype drift).
  • A bare #!-style or hash line that merely contains <!doctype lower in the file is NOT consumed (only the leading line is, via advanceLineIf / the contiguous-prefix loop).
  • If XML/HTML is split into separate HTML and XML/SVG entries, filetype_test.go covers lookup for each extension (.html, .htm, .xhtml, .xml, .svg, .pom, pom.xml).
  • go test ./... passes and the CI coverage gate (.testcoverage.yml, total: 100) stays green — every new branch is exercised.
  • Add a CHANGELOG.md entry under ## [Unreleased] (### Fixed) describing the doctype preservation, in the style of the v0.2.1 shebang entry.

References

  • internal/filetype/filetype.go:147-153 — XML/HTML builtin entry (preserves BOM + XMLDecl, missing doctype)
  • internal/render/render.go:320-367 — preserveBoundary (render insert path; switch rule.Kind to extend)
  • internal/render/render.go:382-401 — advanceLineIf helper (CRLF-agnostic line consumer to reuse)
  • internal/detect/detect.go:171-219 — PreserveBoundary (detect path; allowXML/allowPHP/allowPragma gating + consume loop to extend)
  • internal/model/model.go:148-162 — PreserveKind const block (add PreserveDoctype)
  • internal/render/render.go:1-26 — package doc / safety invariants (both boundaries must agree byte-for-byte)
  • .testcoverage.yml — 100% total coverage gate; the three touched files are not excluded
  • CHANGELOG.md ## [0.2.1] - 2026-06-04 — the universal-shebang precedent (PR #5) this fix mirrors
  • internal/render/render_test.go, internal/detect/detect_test.go — existing shebang test cases to mirror (grep shebang / #!)
  • HTML Standard, "quirks mode": a comment before the doctype forces quirks mode in browsers
## Summary When `apply` adds a license header to an HTML file, it inserts the header before the document's `<!DOCTYPE html>` declaration. The `XML/HTML` file type in the builtin table preserves a leading `<?xml ... ?>` declaration but does **not** preserve a leading `<!DOCTYPE ...>`. Because the doctype is no longer the first thing the browser sees, the page is parsed in **quirks mode** instead of standards mode, which silently changes layout, box-model, and CSS behavior at runtime. Per the HTML spec, the doctype must be the very first content of the document (after an optional BOM). An HTML comment (`<!-- ... -->`, which is exactly the comment syntax this tool writes for HTML) placed before `<!DOCTYPE html>` triggers quirks mode in all major browsers. This is the same class of bug as the recently-fixed shebang issue (PR #5 / v0.2.1): a construct that MUST lead the file is not being treated as preserve-first, so the header gets spliced in front of it and breaks the file. ## Type / Severity - **Type:** Bug (file-corrupting `apply` mutation for a supported file type) - **Severity:** High. `apply --write` produces a structurally different HTML document. The output is still valid HTML so neither `check` nor any compiler catches it; the regression only manifests as visual/CSS differences in a browser (quirks vs. standards mode). It is silent and easy to ship. ## Reproduction Build the tool and run `apply` against a standards-mode HTML file. Sample file `index.html`: ```html <!DOCTYPE html> <html lang="en"> <head> <meta charset="utf-8" /> <title>Hello</title> </head> <body> <p>Hello, world.</p> </body> </html> ``` Commands: ```sh # minimal config so apply has a license/holder to render cat > .license-tool.yaml <<'EOF' license: MIT holder: Example Corp year: "2026" style: reuse EOF go run ./cmd/license-tool apply --write index.html head -n 5 index.html ``` **Observed** — the header (an HTML comment) is inserted at byte 0, pushing the doctype below it: ```html <!-- SPDX-License-Tool: license-tool:managed SPDX-FileCopyrightText: 2026 Example Corp SPDX-License-Identifier: MIT --> <!DOCTYPE html> <html lang="en"> ... ``` Any browser now renders `index.html` in **quirks mode** because `<!DOCTYPE html>` is no longer first. **Expected** — the doctype stays first; the header is inserted immediately after it: ```html <!DOCTYPE html> <!-- SPDX-License-Tool: license-tool:managed SPDX-FileCopyrightText: 2026 Example Corp SPDX-License-Identifier: MIT --> <html lang="en"> ... ``` (For an `.xml`/`.svg` file with a leading `<?xml?>` and a doctype, the doctype must also be preserved, after the XML declaration: `<?xml?>` then `<!DOCTYPE?>` then header.) ## Root cause The `XML/HTML` file type does not list a preserve-first rule for a leading `<!DOCTYPE>`, and neither boundary computation knows how to consume one. 1. **Filetype table** — `internal/filetype/filetype.go`, the `XML/HTML` entry (lines 147-153): ```go { Name: "XML/HTML", Extensions: []string{".xml", ".html", ".htm", ".xhtml", ".svg", ".pom"}, Filenames: []string{"pom.xml"}, CommentStyle: block("<!--", "-->"), PreserveFirst: []model.PreserveRule{after(model.PreserveBOM), after(model.PreserveXMLDecl)}, } ``` It preserves `PreserveBOM` and `PreserveXMLDecl` but has no rule for the doctype. 2. **Render insert path** — `internal/render/render.go`, `preserveBoundary` (lines 320-367). The `switch rule.Kind` handles `PreserveBOM`, `PreserveShebang`, `PreserveXMLDecl`, `PreservePHPOpen`, and `PreserveCodingPragma`. There is no case that advances past a `<!DOCTYPE ...>` line, so `insertAt` (lines 269-286) computes a boundary that sits before the doctype and splices the header there. 3. **Detect path** — `internal/detect/detect.go`, `PreserveBoundary` (lines 171-219). The contiguous-prefix loop consumes lines matching `allowShebang`, `allowXML`, `allowPHP`, and `allowPragma` only. A `<!DOCTYPE ...>` line is not consumable, so the boundary stops before it (mirroring the render bug — both boundaries must agree per the `render.go` package-doc safety invariants). 4. **Missing enum** — `internal/model/model.go`, the `PreserveKind` constants (lines 148-162): `PreserveShebang`, `PreserveXMLDecl`, `PreservePHPOpen`, `PreserveBOM`, `PreserveCodingPragma`, `PreservePackageDecl`. There is no `PreserveDoctype` kind to attach a rule to. This is exactly the shebang situation (v0.2.1): a "must-be-first" construct was not in the preserve-first machinery, so the header was written in front of it. ## Proposed fix Follow the universal-shebang precedent (PR #5, v0.2.1), but scope the doctype handling to the markup file types rather than making it truly universal — a `<!DOCTYPE>` only leads HTML/XML/SVG documents, not arbitrary languages, so a per-rule approach (like `PreserveXMLDecl`) is the right shape. Both boundary implementations must stay byte-for-byte in agreement (the `render.go` package doc calls this out as a safety invariant), and both must order the doctype **after** any BOM and `<?xml?>` declaration, since the doctype follows the XML declaration when both are present. 1. **Add a `PreserveDoctype` kind** — `internal/model/model.go`, in the `PreserveKind` const block (lines 148-162). Append it (e.g. after `PreservePackageDecl`) so existing iota values do not shift. Document it as `<!DOCTYPE ...>`. 2. **Filetype table** — `internal/filetype/filetype.go`, `XML/HTML` entry. Add `after(model.PreserveDoctype)` to `PreserveFirst`, ordered after `PreserveXMLDecl`: ```go PreserveFirst: []model.PreserveRule{ after(model.PreserveBOM), after(model.PreserveXMLDecl), after(model.PreserveDoctype), }, ``` Consider whether the doctype rule should apply to all current extensions or be split: `<!DOCTYPE html>` is the HTML case (`.html`, `.htm`, `.xhtml`), while `.xml`/`.svg`/`.pom` use `<?xml?>` and rarely carry a doctype. The matcher (next point) is case-insensitive and only fires on a real `<!doctype` token, so listing the rule on the shared `XML/HTML` entry is safe for both — but if you prefer to scope it, split `XML/HTML` into an `HTML` entry (`.html`/`.htm`/`.xhtml`, with the doctype rule) and a `XML/SVG` entry (`.xml`/`.svg`/`.pom`/`pom.xml`, without it). Splitting is cleaner semantically; the shared-rule approach is a smaller diff. Either is acceptable as long as the boundary matcher is doctype-specific. 3. **Render insert path** — `internal/render/render.go`, `preserveBoundary` (lines 339-365). Add a `case model.PreserveDoctype` to the rule `switch`, mirroring the existing `PreserveXMLDecl` case, matching a leading doctype **case-insensitively**: ```go case model.PreserveDoctype: pos = advanceLineIf(content, pos, func(line string) bool { return strings.HasPrefix(strings.ToLower(strings.TrimSpace(line)), "<!doctype") }) ``` This reuses the existing `advanceLineIf` helper (lines 382-401), which is already CRLF-agnostic, so BOM/CRLF/trailing-newline invariants are preserved automatically. 4. **Detect path** — `internal/detect/detect.go`, `PreserveBoundary` (lines 171-219). Add an `allowDoctype := hasRule(ft, model.PreserveDoctype, false)` alongside `allowXML`/`allowPHP`/`allowPragma` (near lines 187-190), and add a case to the consume loop (near lines 201-210): ```go case allowDoctype && strings.HasPrefix(strings.ToLower(strings.TrimSpace(trimmed)), "<!doctype"): consumed = true ``` Keep the doctype gated on the rule list (not universal) so it never fires on non-markup types. Because the loop consumes contiguous prefix lines in file order, a `<?xml?>` then `<!DOCTYPE>` stack is handled naturally, matching the render path. 5. **Coverage gate** — the repo enforces 100% total coverage via `.testcoverage.yml` (CI runs go-test-coverage; `render.go`, `detect.go`, and `filetype.go` are NOT in the exclude list). Every new branch (the new `case` in both boundaries, the `allowDoctype` true/false paths, and any new filetype entry/extension) must be exercised by tests or CI fails. None of the three touched files is eligible for exclusion. ## Tests & acceptance criteria Mirror the existing shebang tests (the v0.2.1 fix added shebang cases in `internal/render/render_test.go` and `internal/detect/detect_test.go`; grep for `shebang`/`#!` there for the pattern), and the filetype tests in `internal/filetype/filetype_test.go`. Acceptance criteria: - [ ] `apply --write` on the sample `index.html` above leaves `<!DOCTYPE html>` as the first line and inserts the header immediately after it (Expected output in Reproduction). - [ ] Doctype matching is case-insensitive: `<!doctype html>`, `<!DOCTYPE HTML>`, and `<!DOCTYPE html PUBLIC "...">` (legacy/XHTML doctypes) are all preserved. - [ ] BOM + doctype: a file starting with a UTF-8 BOM then `<!DOCTYPE html>` keeps the BOM first, then the doctype, then the header. (BOM/CRLF/trailing-newline invariants from the `render.go` package doc still hold.) - [ ] XML declaration + doctype: `<?xml?>` then `<!DOCTYPE ...>` (e.g. an XHTML or SVG file) preserves both, in order, before the header. - [ ] `render.preserveBoundary` and `detect.PreserveBoundary` return the **same** offset for every doctype fixture (the boundaries-must-agree invariant). Add a fixture asserting both. - [ ] Idempotency: running `apply --write` twice on a doctype-leading HTML file is a no-op on the second run (no stacked headers, no doctype drift). - [ ] A bare `#!`-style or hash line that merely contains `<!doctype` lower in the file is NOT consumed (only the leading line is, via `advanceLineIf` / the contiguous-prefix loop). - [ ] If `XML/HTML` is split into separate HTML and XML/SVG entries, `filetype_test.go` covers lookup for each extension (`.html`, `.htm`, `.xhtml`, `.xml`, `.svg`, `.pom`, `pom.xml`). - [ ] `go test ./...` passes and the CI coverage gate (`.testcoverage.yml`, total: 100) stays green — every new branch is exercised. - [ ] Add a CHANGELOG.md entry under `## [Unreleased]` (`### Fixed`) describing the doctype preservation, in the style of the v0.2.1 shebang entry. ## References - `internal/filetype/filetype.go:147-153` — `XML/HTML` builtin entry (preserves BOM + XMLDecl, missing doctype) - `internal/render/render.go:320-367` — `preserveBoundary` (render insert path; `switch rule.Kind` to extend) - `internal/render/render.go:382-401` — `advanceLineIf` helper (CRLF-agnostic line consumer to reuse) - `internal/detect/detect.go:171-219` — `PreserveBoundary` (detect path; `allowXML`/`allowPHP`/`allowPragma` gating + consume loop to extend) - `internal/model/model.go:148-162` — `PreserveKind` const block (add `PreserveDoctype`) - `internal/render/render.go:1-26` — package doc / safety invariants (both boundaries must agree byte-for-byte) - `.testcoverage.yml` — 100% total coverage gate; the three touched files are not excluded - CHANGELOG.md `## [0.2.1] - 2026-06-04` — the universal-shebang precedent (PR #5) this fix mirrors - `internal/render/render_test.go`, `internal/detect/detect_test.go` — existing shebang test cases to mirror (grep `shebang` / `#!`) - HTML Standard, "quirks mode": a comment before the doctype forces quirks mode in browsers
KofTwentyTwo commented 2026-06-04 16:29:24 +00:00 (Migrated from github.com)
Copy link
Copy source

Status: confirmed. Built v0.2.1 HEAD and reproduced: apply --write on a <!DOCTYPE html>-leading file inserts the <!-- ... --> header at byte 0, pushing the doctype to line 7. Since the doctype is no longer the first content, every major browser renders the document in quirks mode.

Severity: high (agrees with label). It silently produces a structurally different document for a supported file type; check/compilers never catch it (output is valid HTML), so it ships unnoticed. Not critical because it is layout/CSS-affecting, not data loss, and only the markup types are in scope.

Verified root cause (all four cited locations confirmed, line refs accurate):

  • internal/filetype/filetype.go:147-153 — XML/HTML entry: PreserveFirst: [after(PreserveBOM), after(PreserveXMLDecl)], no doctype rule.
  • internal/render/render.go:320-367 preserveBoundary — switch rule.Kind covers BOM/Shebang/XMLDecl/PHPOpen/CodingPragma; no PreserveDoctype case, so the boundary lands before the doctype and insertAt (render.go:269-286) splices the header there.
  • internal/detect/detect.go:171-219 PreserveBoundary — consume loop gates on allowShebang/allowXML/allowPHP/allowPragma; a <!DOCTYPE> line is not consumable, so the detect boundary stops before it too (both boundaries agree on the wrong offset).
  • internal/model/model.go:148-162 PreserveKind — no PreserveDoctype constant to attach a rule to.

Suggested fix: Mirror the v0.2.1 universal-shebang precedent (CHANGELOG ## [0.2.1]), but keep doctype per-rule / gated (like PreserveXMLDecl) rather than universal like #!, since a doctype only leads markup. Touch all three boundaries + the type table, ordered after BOM and <?xml?>:

  1. model.go — append PreserveDoctype to the const block after PreservePackageDecl (don't shift existing iota values).
  2. filetype.go — add after(model.PreserveDoctype) to the XML/HTML PreserveFirst, after PreserveXMLDecl.
  3. render.go preserveBoundary — add case model.PreserveDoctype reusing advanceLineIf (render.go:382-401, already CRLF-agnostic), matching strings.HasPrefix(strings.ToLower(strings.TrimSpace(line)), "<!doctype").
  4. detect.go PreserveBoundary — add allowDoctype := hasRule(ft, model.PreserveDoctype, false) and a matching case in the consume loop. Keep it gated (not universal) so it never fires on non-markup types.

Because advanceLineIf and the contiguous-prefix loop work in file order, a <?xml?> then <!DOCTYPE> stack is handled naturally on both paths; BOM/CRLF/trailing-newline invariants hold automatically. Coverage gate is real: .testcoverage.yml is total: 100 and excludes only main.go, gen_spdx.go, wizard.go — render/detect/filetype are gated, so every new branch (both cases, allowDoctype true/false, any new filetype entry) must be tested or CI fails.

Corrections to the issue: none material. Every line reference (filetype 147-153, render 320-367 / 269-286 / 382-401, detect 171-219, model 148-162) matches the current tree exactly, and the reproduction reproduces as written. One clarification, not a correction: the issue's optional "split XML/HTML into HTML vs XML/SVG" suggestion is purely cosmetic — the proposed matcher only fires on a real leading <!doctype token, so the shared-entry approach is already safe for .xml/.svg/.pom; the split is a judgment call, not a correctness requirement.

Open questions:

  • Scope: keep the doctype rule on the shared XML/HTML entry (smaller diff) or split into HTML + XML/SVG entries (cleaner semantics)? Either is correct; this is a maintainer style choice that changes the filetype_test.go surface.
  • Should the matcher require a space/> after <!doctype (e.g. <!doctype html>) to avoid a pathological tag like <!doctypex>? The proposed prefix-only match is spec-sufficient but slightly loose; decide if tightening is worth the extra branch under the 100% gate.

Overlaps: none. This is the next instance of the same class as the v0.2.1 shebang fix (PR #5) but a distinct construct/file-type; no open issue duplicates it.

Automated maintainer review.

**Status:** confirmed. Built `v0.2.1` HEAD and reproduced: `apply --write` on a `<!DOCTYPE html>`-leading file inserts the `<!-- ... -->` header at byte 0, pushing the doctype to line 7. Since the doctype is no longer the first content, every major browser renders the document in quirks mode. **Severity:** high (agrees with label). It silently produces a structurally different document for a supported file type; `check`/compilers never catch it (output is valid HTML), so it ships unnoticed. Not critical because it is layout/CSS-affecting, not data loss, and only the markup types are in scope. **Verified root cause** (all four cited locations confirmed, line refs accurate): - `internal/filetype/filetype.go:147-153` — `XML/HTML` entry: `PreserveFirst: [after(PreserveBOM), after(PreserveXMLDecl)]`, no doctype rule. - `internal/render/render.go:320-367` `preserveBoundary` — `switch rule.Kind` covers BOM/Shebang/XMLDecl/PHPOpen/CodingPragma; no `PreserveDoctype` case, so the boundary lands before the doctype and `insertAt` (render.go:269-286) splices the header there. - `internal/detect/detect.go:171-219` `PreserveBoundary` — consume loop gates on `allowShebang/allowXML/allowPHP/allowPragma`; a `<!DOCTYPE>` line is not consumable, so the detect boundary stops before it too (both boundaries agree on the wrong offset). - `internal/model/model.go:148-162` `PreserveKind` — no `PreserveDoctype` constant to attach a rule to. **Suggested fix:** Mirror the v0.2.1 universal-shebang precedent (CHANGELOG `## [0.2.1]`), but keep doctype **per-rule / gated** (like `PreserveXMLDecl`) rather than universal like `#!`, since a doctype only leads markup. Touch all three boundaries + the type table, ordered **after** BOM and `<?xml?>`: 1. `model.go` — append `PreserveDoctype` to the const block after `PreservePackageDecl` (don't shift existing iota values). 2. `filetype.go` — add `after(model.PreserveDoctype)` to the `XML/HTML` `PreserveFirst`, after `PreserveXMLDecl`. 3. `render.go` `preserveBoundary` — add `case model.PreserveDoctype` reusing `advanceLineIf` (render.go:382-401, already CRLF-agnostic), matching `strings.HasPrefix(strings.ToLower(strings.TrimSpace(line)), "<!doctype")`. 4. `detect.go` `PreserveBoundary` — add `allowDoctype := hasRule(ft, model.PreserveDoctype, false)` and a matching `case` in the consume loop. Keep it gated (not universal) so it never fires on non-markup types. Because `advanceLineIf` and the contiguous-prefix loop work in file order, a `<?xml?>` then `<!DOCTYPE>` stack is handled naturally on both paths; BOM/CRLF/trailing-newline invariants hold automatically. Coverage gate is real: `.testcoverage.yml` is `total: 100` and excludes only `main.go`, `gen_spdx.go`, `wizard.go` — render/detect/filetype are gated, so every new branch (both `case`s, `allowDoctype` true/false, any new filetype entry) must be tested or CI fails. **Corrections to the issue:** none material. Every line reference (filetype 147-153, render 320-367 / 269-286 / 382-401, detect 171-219, model 148-162) matches the current tree exactly, and the reproduction reproduces as written. One clarification, not a correction: the issue's optional "split `XML/HTML` into HTML vs XML/SVG" suggestion is purely cosmetic — the proposed matcher only fires on a real leading `<!doctype` token, so the shared-entry approach is already safe for `.xml`/`.svg`/`.pom`; the split is a judgment call, not a correctness requirement. **Open questions:** - Scope: keep the doctype rule on the shared `XML/HTML` entry (smaller diff) or split into `HTML` + `XML/SVG` entries (cleaner semantics)? Either is correct; this is a maintainer style choice that changes the `filetype_test.go` surface. - Should the matcher require a space/`>` after `<!doctype` (e.g. `<!doctype html>`) to avoid a pathological tag like `<!doctypex>`? The proposed prefix-only match is spec-sufficient but slightly loose; decide if tightening is worth the extra branch under the 100% gate. **Overlaps:** none. This is the next instance of the same class as the v0.2.1 shebang fix (PR #5) but a distinct construct/file-type; no open issue duplicates it. _Automated maintainer review._
KofTwentyTwo commented 2026-06-04 20:27:59 +00:00 (Migrated from github.com)
Copy link
Copy source

Resolved on develop in commit 742c124e6ec2afa9312075db3112bc2def3c91ff.

Validation:

Closing as fixed.

Resolved on `develop` in commit `742c124e6ec2afa9312075db3112bc2def3c91ff`. Validation: - CI: success, https://github.com/KofTwentyTwo/license-tool/actions/runs/26977377097 - CodeQL: success, https://github.com/KofTwentyTwo/license-tool/actions/runs/26977376421 - Coverage gate: 100% (2087/2087) Closing as fixed.
Sign in to join this conversation.
No Branch/Tag Specified
Labels
Clear labels
bug
Something isn't working
 dependencies
Pull requests that update a dependency file
 documentation
Improvements or additions to documentation
 duplicate
This issue or pull request already exists
 enhancement
New feature or request
 github_actions
Pull requests that update GitHub Actions code
 go
Pull requests that update go code
 good first issue
Good for newcomers
 help wanted
Extra attention is needed
 invalid
This doesn't seem right
 question
Further information is requested
 wontfix
This will not be worked on
No labels bug
Milestone
No items
No Milestone
Projects
Clear projects
No projects
Assignees
Clear assignees
forge-recovery (Forge Recovery) james.maes (James Maes)
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: KofTwentyTwo/license-tool#8