ReviewOS

stacks/stx

stx typecheck: `<script client>` inside an HTML comment opens a real block — 106 errors from English prose in one file

#1901
Closed glennmichael123 opened this 10 days ago ·
10 days ago

stx typecheck finds script blocks by scanning for the literal tag text without skipping HTML comments. A comment that mentions <script client> in prose opens a block, and everything from there to the next </script> — the remaining prose, the -->, and the real script that follows — is handed to TypeScript.

In a real app this produced 106 errors in one file, every one of them pointing at an English sentence.

Repro

repro.stx, 7 lines:

<!--
  This block is deliberately NOT <script client>.
  It executes once per full page load and never re-runs.
-->
<script>
  const ok = 1
</script>
$ stx typecheck "repro.stx"
repro.stx:3:29  error TS1434  Unexpected keyword or identifier.  [<script client>]
repro.stx:3:34  error TS1434  Unexpected keyword or identifier.  [<script client>]
repro.stx:3:39  error TS1434  Unexpected keyword or identifier.  [<script client>]
repro.stx:3:43  error TS1434  Unexpected keyword or identifier.  [<script client>]
repro.stx:4:1   error TS1003  Identifier expected.               [<script client>]
repro.stx:4:3   error TS1109  Expression expected.               [<script client>]
repro.stx:6:3   error TS1109  Expression expected.               [<script client>]

  Checked 1 script block(s) and 0 template expression(s) in 1 file(s) — 13 error(s), 0 warning(s)

Line 3 is prose. Line 4 is -->.

Control

Byte-identical except the comment says a client script instead of <script client>:

  Checked 0 script block(s) and 0 template expression(s) in 0 file(s) — 0 error(s), 0 warning(s)

So the literal tag text inside the comment is the whole trigger.

Two tells that make this easy to confirm

  • The diagnostics are labelled [<script client>], but the only real script in the file is a plain <script>. The label is coming from the comment.
  • The reported block count is 1 for a file whose sole genuine block is unparseable-as-client.

Why it bites

Documenting why a block is deliberately not a client script is the natural thing to write directly above it, and writing the tag name is the natural way to say it. The comment that explains the decision is what breaks the check.

The failure is also silent-ish in the wrong direction: it does not crash, it emits a large pile of plausible-looking TS parse errors. First reaction is to go looking for broken code that isn't there. 106 errors from one comment is enough noise to get stx typecheck switched off, which is the same outcome as not shipping it.

Same class as #1831 — scanning source text for a marker without excluding comments — in a different code path. #1831 was the server→client bridge; this is script-block detection in typecheck. Worth a look at whether anything else locates blocks the same way.

Suggested fix

Strip <!-- … --> ranges before locating script blocks, or track comment state while scanning so a marker inside one is skipped.

10 days ago

Follow-up: the false errors are the smaller half of this. The phantom block also suppresses real diagnostics across the whole run, not just in the offending file.

Same app, same corpus, the only change being the comment reworded so it no longer contains the literal tag text:

script blocksfileserrors
comment contains <script client>3535106 (all fake, all from prose)
comment reworded3434432 (all real)

The 432 are genuine — Property 'value' does not exist on type 'Element', implicit any parameters, possibly-null. Checked in isolation, one view alone accounts for 198 of them:

$ stx typecheck "resources/views/composer.stx"
  Checked 1 script block(s) and 133 template expression(s) in 1 file(s) — 198 error(s), 0 warning(s)

composer.stx contains no such comment. It was clean in the first run purely because a different file — the layout — had the phantom block.

So the failure mode is worse than noise. A single comment anywhere in the corpus can take the program down to a fraction of its real diagnostics while still exiting with errors, which looks like a working gate. Ours reported 106 problems and hid 432; the number went up four-fold when the "bug" was removed, which is not the direction anyone checks.

Worth confirming whether the parse failure aborts the shared TS program early — the handoff notes a parse failure suppresses other diagnostics in that file, but this is cross-file.

10 days ago

Fixed in e439a07da9.

Your repro is 0 diagnostics now. extractScriptBlocks scans over a copy with comment CONTENTS blanked — HTML and alike — so a tag named in prose cannot open a block.

Positions are preserved: every blanked character becomes a space and every newline stays a newline, so offsets still index the real source, block bodies are sliced from it rather than from the copy, and a diagnostic after a comment still points at the line the author wrote. That last one is pinned, because reporting the right error on the wrong line is its own bug.

The framing in the report is the part worth keeping: documenting the framework in a comment above the thing being documented is the most ordinary thing an author can do, and <script client> is exactly the phrase they would use. 106 errors on English prose is how a gate gets muted, which is the standard the rest of this checker was measured against.

4 tests; removing the blanking fails all 4. Also covered: a commented-out block followed by a real one, and .

Sign in to comment on this issue.