Commit 17cd8ba
feat: report diagnostics for malformed PEP 723 inline script metadata (#1785)
Part of #1602 (PEP 723 inline script env support). Does not close it —
this covers only diagnostics for malformed metadata.
## Problem
Malformed PEP 723 metadata is currently **invisible**. The parser
collapses every failure to `undefined`, so a block with a typo — a
missing closing `# ///`, a content line without the required space after
`#`, a TOML syntax error — produces no CodeLens, no warning, nothing.
The file just looks like ordinary Python and the user has no idea why
the "Setup environment" action never appeared.
Worse, a malformed block and a file with *no* block were
indistinguishable, so we couldn't have told the user even if we wanted
to.
## What this does
The parser now returns a discriminated result — `valid` / `invalid` /
`none` — carrying structured problems with source ranges, and a new
`InlineScriptDiagnosticsPublisher` surfaces them as squiggles.
Gated behind the existing `python-envs.inlineScripts.enabled` setting
(default `false`, latched at activation). No Pylance or Python extension
changes; no cross-team dependencies.
---
## Spec conformance
Normative source: [PyPA — Inline script
metadata](https://packaging.python.org/en/latest/specifications/inline-script-metadata/).
Permalinks below are pinned to commit
[`9dd85f2`](https://github.com/pypa/packaging.python.org/blob/9dd85f27266c88c4eca58828c918c337b665d331/source/specifications/inline-script-metadata.rst),
the latest commit touching that file, so line numbers can't drift.
I extracted every RFC 2119 keyword sentence from the spec source
programmatically: **20 total — 11 MUST, 2 MUST NOT, 1 SHOULD, 7 MAY.**
The mapping below is against that complete set, not a sampling.
### Diagnostics backed by an explicit spec MUST
| Code | Severity | Spec text | Strength | Source |
|---|---|---|---|---|
| `invalid-block-marker` | Error | "blocks that MUST start with the line
`# /// TYPE`… Block MUST end with the line `# ///`… The `TYPE` MUST only
consist of ASCII letters, numbers and hyphens" | MUST x3 |
[L20-L26](https://github.com/pypa/packaging.python.org/blob/9dd85f27266c88c4eca58828c918c337b665d331/source/specifications/inline-script-metadata.rst#L20-L26)
|
| `invalid-content-line` | Error | "Every line between these two lines
MUST be a comment starting with `#`. If there are characters after the
`#` then the first character MUST be a space" | MUST x2 |
[L28-L30](https://github.com/pypa/packaging.python.org/blob/9dd85f27266c88c4eca58828c918c337b665d331/source/specifications/inline-script-metadata.rst#L28-L30)
|
| `multiple-blocks` | Error | "When there are multiple comment blocks of
the same `TYPE` defined, tools MUST produce an error" | MUST |
[L54-L55](https://github.com/pypa/packaging.python.org/blob/9dd85f27266c88c4eca58828c918c337b665d331/source/specifications/inline-script-metadata.rst#L54-L55)
|
| `unterminated-block` | **Warning** | "Unclosed blocks MUST be ignored"
| MUST |
[L51-L52](https://github.com/pypa/packaging.python.org/blob/9dd85f27266c88c4eca58828c918c337b665d331/source/specifications/inline-script-metadata.rst#L51-L52)
|
`unterminated-block` is a **warning, not an error**, precisely because
the spec says such blocks MUST be *ignored* — the file is valid Python
and valid per spec. We only hint that the author probably meant to close
it. See the scoping rule below.
Also enforced without needing a diagnostic:
| Spec requirement | Strength | How we conform | Source |
|---|---|---|---|
| "Tools MUST NOT read from metadata blocks with types that have not
been standardized" | MUST NOT | `type === 'script'` filter in both the
block regex and the opener scan |
[L70-L71](https://github.com/pypa/packaging.python.org/blob/9dd85f27266c88c4eca58828c918c337b665d331/source/specifications/inline-script-metadata.rst#L70-L71)
|
| "If they choose not to [respect the encoding declaration], they MUST
process the file as UTF-8" | MAY / MUST | We do not honour the encoding
declaration; we process as UTF-8 and strip BOM |
[L57-L58](https://github.com/pypa/packaging.python.org/blob/9dd85f27266c88c4eca58828c918c337b665d331/source/specifications/inline-script-metadata.rst#L57-L58)
|
| "Tools MAY choose to do a simple textual scan, rather than a full
Python parse" | MAY | Textual scan, per the canonical regex |
[L77](https://github.com/pypa/packaging.python.org/blob/9dd85f27266c88c4eca58828c918c337b665d331/source/specifications/inline-script-metadata.rst#L77)
|
| "the text specification takes precedence" over the canonical regex |
tiebreak | Applied deliberately — see empty blocks below |
[L67-L68](https://github.com/pypa/packaging.python.org/blob/9dd85f27266c88c4eca58828c918c337b665d331/source/specifications/inline-script-metadata.rst#L67-L68)
|
### Deliberately NOT reported
These are conformance decisions, not oversights. Each is measured by a
test.
| Case | Why we stay silent |
|---|---|
| A start line nested inside another block | Spec says tools **MAY**
produce an error — MAY, not MUST. We take the least-aggressive legal
option.
[L51](https://github.com/pypa/packaging.python.org/blob/9dd85f27266c88c4eca58828c918c337b665d331/source/specifications/inline-script-metadata.rst#L51)
|
| **Empty block** (`# /// script` immediately followed by `# ///`) | The
canonical regex requires >=1 content line (`+`), but the prose doesn't.
Spec says prose wins, so this is **valid, empty metadata**. `BLOCK_RE`'s
content group is `*` with a comment marking the deliberate deviation. |
| Unclosed `# /// script` appearing *below* real code | Almost always a
PEP 723 example inside a docstring or tutorial text, not a broken
header. New `headerRegionEnd()` suppresses the warning at/after the
first non-blank, non-`#` line. The spec explicitly says behaviour inside
multi-line strings "is tool-dependent and should not be relied on". |
| `# ///` used as block *content* | Spec's precedence rule plus its own
embedded-C# example. Handled by regex backtracking; verified against the
spec's literal example. |
| Divider comments (`# /////////`), `## ///`, non-`script` types | Not
blocks. |
### Checks that are **ours**, not the spec's
Worth calling out explicitly so reviewers can push back:
**`invalid-field-type`** (Error) fires when `dependencies` isn't an
array of strings, `requires-python` isn't a string, or `tool` isn't a
table. The spec describes these types in prose — "`dependencies`: A list
of strings…", "`requires-python`: A string…"
([L99-L104](https://github.com/pypa/packaging.python.org/blob/9dd85f27266c88c4eca58828c918c337b665d331/source/specifications/inline-script-metadata.rst#L99-L104))
— **but attaches no MUST to the container type.** The only MUST in those
bullets governs entry *validity*.
I kept it as an error because a non-array `dependencies` has no valid
interpretation and the alternative is a confusing downstream resolver
failure. But it is inference, not mandate. Happy to downgrade to a
warning if reviewers prefer strict spec-only conformance.
**`invalid-toml`** (Error) — the spec never literally states the content
is TOML; it's implied by the `[tool]` table semantics and the reference
implementation's `tomllib.loads(content)`.
### Known gaps (not addressed here)
Two genuine spec MUSTs are **not** enforced. Verified by measurement —
both parse as valid with no diagnostic today:
| Spec text | Source | Status |
|---|---|---|
| "Each entry MUST be a valid [dependency
specifier](https://packaging.python.org/en/latest/specifications/dependency-specifiers/)"
(PEP 508) |
[L99-L101](https://github.com/pypa/packaging.python.org/blob/9dd85f27266c88c4eca58828c918c337b665d331/source/specifications/inline-script-metadata.rst#L99-L101)
| Not validated — `dependencies = ["!!! not a specifier !!!"]` accepted
silently |
| "The value of this field MUST be a valid [version
specifier](https://packaging.python.org/en/latest/specifications/version-specifiers/)"
(PEP 440) |
[L102-L104](https://github.com/pypa/packaging.python.org/blob/9dd85f27266c88c4eca58828c918c337b665d331/source/specifications/inline-script-metadata.rst#L102-L104)
| Not validated — `requires-python = "totally-not-a-version"` accepted
silently |
Both need real grammar implementations and would meaningfully expand
scope. Deferred intentionally; happy to file a follow-up issue.
---
## Behaviour change reviewers must notice
This PR is **not purely additive.** One existing behaviour changes:
```python
# /// script
# ///
```
**Before:** parsed as no metadata -> no CodeLens.
**After:** parses as valid, empty metadata -> **the setup CodeLens now
appears.**
That follows from the spec's "text takes precedence" tiebreak, and I
believe it's correct, but it does affect provisioning and not just
diagnostics. Everything else in this PR is diagnostics-only and leaves
the provisioning path byte-identical — I verified that with a harness
running the old and new parsers side by side over 20 scenarios.
---
## Implementation notes
**`src/common/inlineScript/metadata.ts`** — the parser rework.
- Result is now `{ kind: 'valid' | 'invalid' | 'none', problems,
metadata? }`.
- New independent opener scan (`findScriptOpeners`) — this is what makes
"malformed block" distinguishable from "no block". The canonical regex
alone can't tell them apart.
- `toSourceRange` is the **single** offset-to-line/character translation
point. Reviewers should focus here for range correctness.
- `tomlErrorSourceRange` recovers `line`/`col` from `@iarna/toml` errors
that were previously discarded, so the squiggle underlines the actual
offending text instead of the whole block.
**`src/features/inlineScript/diagnostics.ts`** — the publisher.
- Publishes on open, save, and debounced change (**300 ms**, via the
existing `createSimpleDebounce` helper). Debouncing matters here:
without it we'd flash "missing closing marker" at someone who is still
mid-edit.
- `createSimpleDebounce(ms, cb).trigger()` takes no arguments and owns
one timer, so a shared instance would let documents clobber each other —
hence the per-URI `pending` map.
- Clears on close, delete, rename, and dispose, so no stale squiggles
survive.
- Scope: `file:` scheme plus `.py` extension only.
**Validation reads the live editor buffer**, so squiggles match what's
on screen; provisioning keeps reading the saved file. Both call the same
parser, so the two views can't disagree about what "valid" means.
### Edge cases handled
- **BOM and CRLF** — BOM stripped, `\r\n?` normalized to `\n` before
matching. Note JS regex `.` doesn't match `\r` while Python's does, so
the canonical regex can't be used verbatim.
- **Ranges under BOM/CRLF** — the bulk of the test suite. Every range
assertion runs across four variants: LF, CRLF, BOM+LF, BOM+CRLF.
- **8 KiB header slice** — diagnostics apply `sliceHeaderBytes` exactly
as the provisioning path does, so the two can't diverge on large files.
- **Adjacent blocks merge** — `# ///` is itself a legal content line, so
back-to-back blocks are consumed as one. `multiple-blocks` correctly
requires a non-content line between them.
### Known limitation
Indented markers (` # /// script`) are silent. The spec anchors markers
at column 0, so an indented one isn't a block at all. Documented in the
parser.
---
## Testing
- **52 new unit tests** (31 parser/range, 21 publisher).
- Full suite: **2280 passing, 0 failing, 6 pending**, on a fresh `npm
run compile-tests` build.
- `npm run lint` clean, `tsc` clean, Prettier clean.
> Note for anyone re-running: `npm run unittest` executes
`./out/test/**/*.unit.test.js` — **compiled output**. Run `npm run
compile-tests` first or you'll silently validate stale code.
### Manual verification
Set `python-envs.inlineScripts.enabled: true` and **reload the window**
(the flag is latched at activation). Then try:
```python
# /// script
# requires-python = ">=3.11"
# dependencies = ["requests"]
```
-> warning: missing closing `# ///`
```python
# /// script
#requires-python = ">=3.11"
# ///
```
-> error: content line must be exactly `#` or start with `# `
```python
# /// script
# dependencies = [
# ///
```
-> error: TOML error, underlining the offending token
## Review focus
1. **`toSourceRange` correctness** — every range flows through it;
BOM/CRLF offsets are the likeliest bug.
2. **Is `invalid-field-type` too aggressive?** It's the one error not
backed by a spec MUST.
3. **The empty-block behaviour change** — it alters provisioning, not
just diagnostics.
4. **Debounce lifecycle** — per-URI map, disposal on
close/delete/rename.
---------
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>1 parent 6ad1427 commit 17cd8ba
7 files changed
Lines changed: 1434 additions & 42 deletions
File tree
- src
- common
- inlineScript
- features/inlineScript
- test
- common/inlineScript
- features/inlineScript
Large diffs are not rendered by default.
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
35 | 35 | | |
36 | 36 | | |
37 | 37 | | |
| 38 | + | |
| 39 | + | |
| 40 | + | |
| 41 | + | |
| 42 | + | |
| 43 | + | |
| 44 | + | |
| 45 | + | |
| 46 | + | |
| 47 | + | |
| 48 | + | |
| 49 | + | |
| 50 | + | |
| 51 | + | |
| 52 | + | |
| 53 | + | |
| 54 | + | |
| 55 | + | |
| 56 | + | |
| 57 | + | |
| 58 | + | |
| 59 | + | |
| 60 | + | |
| 61 | + | |
| 62 | + | |
| 63 | + | |
| 64 | + | |
| 65 | + | |
| 66 | + | |
| 67 | + | |
| 68 | + | |
| 69 | + | |
| 70 | + | |
| 71 | + | |
| 72 | + | |
| 73 | + | |
| 74 | + | |
| 75 | + | |
| 76 | + | |
| 77 | + | |
| 78 | + | |
| 79 | + | |
| 80 | + | |
| 81 | + | |
38 | 82 | | |
39 | 83 | | |
40 | 84 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
95 | 95 | | |
96 | 96 | | |
97 | 97 | | |
| 98 | + | |
| 99 | + | |
| 100 | + | |
| 101 | + | |
| 102 | + | |
| 103 | + | |
| 104 | + | |
| 105 | + | |
98 | 106 | | |
99 | 107 | | |
100 | 108 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
69 | 69 | | |
70 | 70 | | |
71 | 71 | | |
| 72 | + | |
72 | 73 | | |
73 | 74 | | |
74 | 75 | | |
| |||
229 | 230 | | |
230 | 231 | | |
231 | 232 | | |
| 233 | + | |
| 234 | + | |
| 235 | + | |
| 236 | + | |
232 | 237 | | |
233 | 238 | | |
234 | 239 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
| 1 | + | |
| 2 | + | |
| 3 | + | |
| 4 | + | |
| 5 | + | |
| 6 | + | |
| 7 | + | |
| 8 | + | |
| 9 | + | |
| 10 | + | |
| 11 | + | |
| 12 | + | |
| 13 | + | |
| 14 | + | |
| 15 | + | |
| 16 | + | |
| 17 | + | |
| 18 | + | |
| 19 | + | |
| 20 | + | |
| 21 | + | |
| 22 | + | |
| 23 | + | |
| 24 | + | |
| 25 | + | |
| 26 | + | |
| 27 | + | |
| 28 | + | |
| 29 | + | |
| 30 | + | |
| 31 | + | |
| 32 | + | |
| 33 | + | |
| 34 | + | |
| 35 | + | |
| 36 | + | |
| 37 | + | |
| 38 | + | |
| 39 | + | |
| 40 | + | |
| 41 | + | |
| 42 | + | |
| 43 | + | |
| 44 | + | |
| 45 | + | |
| 46 | + | |
| 47 | + | |
| 48 | + | |
| 49 | + | |
| 50 | + | |
| 51 | + | |
| 52 | + | |
| 53 | + | |
| 54 | + | |
| 55 | + | |
| 56 | + | |
| 57 | + | |
| 58 | + | |
| 59 | + | |
| 60 | + | |
| 61 | + | |
| 62 | + | |
| 63 | + | |
| 64 | + | |
| 65 | + | |
| 66 | + | |
| 67 | + | |
| 68 | + | |
| 69 | + | |
| 70 | + | |
| 71 | + | |
| 72 | + | |
| 73 | + | |
| 74 | + | |
| 75 | + | |
| 76 | + | |
| 77 | + | |
| 78 | + | |
| 79 | + | |
| 80 | + | |
| 81 | + | |
| 82 | + | |
| 83 | + | |
| 84 | + | |
| 85 | + | |
| 86 | + | |
| 87 | + | |
| 88 | + | |
| 89 | + | |
| 90 | + | |
| 91 | + | |
| 92 | + | |
| 93 | + | |
| 94 | + | |
| 95 | + | |
| 96 | + | |
| 97 | + | |
| 98 | + | |
| 99 | + | |
| 100 | + | |
| 101 | + | |
| 102 | + | |
| 103 | + | |
| 104 | + | |
| 105 | + | |
| 106 | + | |
| 107 | + | |
| 108 | + | |
| 109 | + | |
| 110 | + | |
| 111 | + | |
| 112 | + | |
| 113 | + | |
| 114 | + | |
| 115 | + | |
| 116 | + | |
| 117 | + | |
| 118 | + | |
| 119 | + | |
| 120 | + | |
| 121 | + | |
| 122 | + | |
| 123 | + | |
| 124 | + | |
| 125 | + | |
| 126 | + | |
| 127 | + | |
| 128 | + | |
| 129 | + | |
| 130 | + | |
| 131 | + | |
| 132 | + | |
| 133 | + | |
| 134 | + | |
| 135 | + | |
| 136 | + | |
| 137 | + | |
| 138 | + | |
| 139 | + | |
| 140 | + | |
| 141 | + | |
| 142 | + | |
| 143 | + | |
| 144 | + | |
| 145 | + | |
| 146 | + | |
| 147 | + | |
| 148 | + | |
| 149 | + | |
| 150 | + | |
| 151 | + | |
| 152 | + | |
| 153 | + | |
| 154 | + | |
| 155 | + | |
| 156 | + | |
| 157 | + | |
| 158 | + | |
| 159 | + | |
| 160 | + | |
| 161 | + | |
| 162 | + | |
| 163 | + | |
| 164 | + | |
| 165 | + | |
| 166 | + | |
| 167 | + | |
| 168 | + | |
| 169 | + | |
| 170 | + | |
| 171 | + | |
| 172 | + | |
| 173 | + | |
| 174 | + | |
| 175 | + | |
| 176 | + | |
| 177 | + | |
| 178 | + | |
| 179 | + | |
| 180 | + | |
| 181 | + | |
| 182 | + | |
| 183 | + | |
| 184 | + | |
| 185 | + | |
| 186 | + | |
| 187 | + | |
| 188 | + | |
| 189 | + | |
| 190 | + | |
| 191 | + | |
| 192 | + | |
| 193 | + | |
| 194 | + | |
| 195 | + | |
| 196 | + | |
| 197 | + | |
| 198 | + | |
| 199 | + | |
| 200 | + | |
| 201 | + | |
| 202 | + | |
| 203 | + | |
| 204 | + | |
| 205 | + | |
| 206 | + | |
| 207 | + | |
| 208 | + | |
| 209 | + | |
| 210 | + | |
| 211 | + | |
| 212 | + | |
| 213 | + | |
| 214 | + | |
| 215 | + | |
| 216 | + | |
| 217 | + | |
| 218 | + | |
| 219 | + | |
| 220 | + | |
| 221 | + | |
| 222 | + | |
| 223 | + | |
| 224 | + | |
| 225 | + | |
| 226 | + | |
| 227 | + | |
| 228 | + | |
| 229 | + | |
0 commit comments