Skip to content

Fix LSP crash on unparseable cell option YAML by handling null lint result - #1123

Merged
juliasilge merged 4 commits into
mainfrom
bugfix/yaml-diagnostics-crash
Sep 11, 2026
Merged

Fix LSP crash on unparseable cell option YAML by handling null lint result#1123
juliasilge merged 4 commits into
mainfrom
bugfix/yaml-diagnostics-crash

Conversation

@juliasilge

Copy link
Copy Markdown
Collaborator

Fixes #1118 (the upstream report is from posit-dev/positron#15965)

This PR fixes a Quarto LSP server crash that occurred when saving a document containing unparseable YAML in code cell options (for example, a %%| fig-cap: value containing an unquoted colon). After five crashes in three minutes, the client stops restarting the server and all language features are lost until reload.

Root cause

When cell option YAML fails to parse, getLint() in the bundled vs-code.mjs module catches the resulting YAMLException, logs "Error found during linting", and resolves to null. The on-save diagnostics path in provideYamlDiagnostics then called .map() on that null value, throwing a TypeError that crashed the language server:

Error found during linting [YAMLException: bad indentation of a mapping entry ...]
TypeError: Cannot read properties of null (reading 'map')
    at computeOnSaveDiagnostics (.../lsp.js)

The other caller of getYamlDiagnostics (in apps/lsp/src/custom.ts) already handled null correctly, which is why this only crashed on save.

I verified against the reproduction document in #1118, and now saving no longer crashes the server.

A possible follow-up

With this fix, a document with broken cell option YAML silently produces no YAML diagnostics on save (the parse error is only logged to the Output channel). A future improvement could surface the parse error as a diagnostic using the position info in the YAMLException; that requires changes in the Quarto CLI, where the exception is currently swallowed. The null guard here is still valuable regardless.

@posit-snyk-bot

posit-snyk-bot commented Sep 10, 2026

Copy link
Copy Markdown
Contributor

Snyk checks have passed. No issues have been found so far.

Status Scan Engine Critical High Medium Low Total (0)
Open Source Security 0 0 0 0 0 issues
Licenses 0 0 0 0 0 issues

💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse.

@juliasilge
juliasilge marked this pull request as ready for review September 10, 2026 15:54
@juliasilge
juliasilge requested a review from lionel- September 10, 2026 15:54

@lionel- lionel- left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since having to handle nulls everywhere is a bit of a footgun, or at least impractical, I'd normalise at the boundary by wrapping getLint() with ?? [] here:

getYamlDiagnostics: quartoModule.getLint,

Then we'd keep the current non-nullable signature in the Quarto interface.

WDYT?

@juliasilge

Copy link
Copy Markdown
Collaborator Author

Oh, that's a better idea, @lionel-. Reworked in de07dbf:

  • Reverted the call-site guard in diagnostics-yaml.ts and the nullable signature on the public Quarto interface
  • initializeQuarto now wraps the external getLint() with ?? [] to normalize at the boundary
  • Kept Promise<Array<LintItem> | null> on the private QuartoYamlModule interface

@juliasilge
juliasilge requested a review from lionel- September 11, 2026 14:23

@lionel- lionel- left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LG!

@juliasilge
juliasilge merged commit 0124345 into main Sep 11, 2026
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Quarto-LSP crash due to bad indentation

3 participants