Skip to content

fix(lint): report an unparseable source instead of scoring it clean (#10653) - #10697

Merged
os-zhuang merged 1 commit into
mainfrom
claude/issue-10653-lint-parse-diagnostics
Aug 21, 2026
Merged

fix(lint): report an unparseable source instead of scoring it clean (#10653)#10697
os-zhuang merged 1 commit into
mainfrom
claude/issue-10653-lint-parse-diagnostics

Conversation

@claude

@claudeclaudeBot commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

Fixes#10653

Three packages/lint validators scored an unparseable source as CLEAN. This is option A from the card's ruling: a packages/lint-local checked parse that returns a finding, with the two dead catch blocks deleted along with it.

The premise, verified before building

The card's central correction is that the try/catch wrappers are not where the false-clean verdict comes from. Verified on this tree (TypeScript 6.0.3) before writing any fix — 10/10 wrecks return a tree and throw nothing:

source ScriptKind.TS ScriptKind.TSX threw?
merge-conflict markers 3 diagnostics 3 diagnostics no
truncated function body 1 diagnostic 1 diagnostic no
unterminated string 1 diagnostic 1 diagnostic no
unterminated block comment 1 diagnostic 1 diagnostic no
a JSX element 1 diagnostic 0 no

createSourceFile cannot throw, so the live path at all three sites was the unread parseDiagnostics. Deleting the catch blocks alone would have closed nothing, and this PR does not rest on them.

Population, re-derived on today's tree

node scripts/check-parse-guard.mjs at 55809a09a5: 28 parses outside scripts/ in 20 files — 4 in shipped package source, unchanged from the card's four rows. Three are the lint validators in scope; the fourth (packages/cli/src/utils/detect-free-identifiers.ts) is CLI runtime and out of scope by the card.

After this change the same census reads 2 parses in 2 files in that tier: the three validators drop out (they no longer call ts.createSourceFile directly) and the single new checked-parse.ts call site takes their place, next to the untouched CLI row. The census moved because the tree moved, which is what it is for.

The harm, reproduced

Not "it throws and we skip" — it parses, the finding vanishes, and the verdict reads clean. Each pair below is the same defect with and without a syntax error, measured before the fix:

validatorwrecked sourceparseDiagnosticsfindings beforefindings after
validateReactPageProps<ObjectForm> behind an unterminated comment101 (react-page-source-unparseable)
findStartupRegistryVerdictsverdict behind an unterminated comment101 (startup-source-unparseable)
extractHookBodyWriteSetctx.input.amout = 0 behind an unterminated comment10 writeswrite set + parseFailure

Every row has a positive control in the tests: the repaired twin produces the finding the wreck loses, so the zero above is a loss and not an empty fixture.

What the fix does, and deliberately does not do

packages/lint/src/checked-parse.ts reads the diagnostics and returns them. It never throws and never exits: a publish-time validator is handed metadata by someone else, so ending the process on their input is not its call. The compiler is a parameter, never an import, so the lazy-load contract (lazy-deps.test.ts) is untouched.

The change is additive. The recovered tree is still walked, so every finding these rules produce today they still produce — including from a wrecked tree. Measured: a source containing 0755 still yields its react-prop-missing-required, and gains the parse finding beside it. Ablation A4 below is the guard on that property; the alternative design (skip the walk on failure) silently drops it.

Severity is warning at all four sites. Three of them have severity: 'warning' as the only inhabitant of their severity type. For the react props gate it is a measured choice, not caution — see the constant's docblock and #10696.

False-positive control

  • Every new rule has an explicit parseable corpus asserted to gain no parse finding, including the TS-only spellings each parser must accept (ScriptKind.TSX generics, type annotations, fragments, spreads; ScriptKind.TS enums and non-null assertions).
  • HOOK_BODY_WRITE_PATTERNS / ACTION_*_PATTERNS ledger examples are swept through the same assertion, so a declared pattern cannot become a false positive.
  • The wider control is the existing suite: 2235 tests pass, and many assert exact finding lists for parseable sources.
  • The corpus sweep lint-startup-registry-verdict.corpus.test.ts walks every .ts under packages/. Measured before implementing: 10 files actually reach the parse (the rest are cut by the raw-text pre-filter) and 0 of them carry a parse diagnostic — so this change cannot redden that ratchet today, and if a future unparseable file lands there, going red is the behaviour that test's own header asks for.

The synthesised wrapper — whose fault an unparseable body is

validate-hook-body-writes.ts parses async function __body(ctx) { … }, so "unparseable" there could mean the synthesis is wrong rather than the author's source. It cannot be, and this is asserted rather than argued: the wrapper is a constant, and a test pins that it parses clean around a trivial body and around every example in the pattern ledger. Positions are reported in the body's own coordinates (synthesizedLinesBefore: 1, clamped), so a diagnostic can never point at a line the author did not write.

A fixture that had to be re-judged, not edited

lint-startup-registry-verdict.test.ts carried expect(findStartupRegistryVerdicts('class { { { getRegisteredNodeTypes(')).toEqual([]) under the heading "an empty or unparseable source is not a verdict about anyone". Its assertion passed precisely because nothing was produced — it pinned the branch this PR changes. Its two halves came apart on inspection and both are now asserted separately: declining to draw a startup verdict from wreckage still holds, and returning nothing at all no longer does.

Ablations

Each mutation was confirmed on disk by anchor count before the run (old:1->0 new:0->1), and each restore leg verified the anchor was gone and the tree clean. Predictions were written before running.

#mutationpredicted redmeasured red
A1checked-parse never sees parseDiagnostics (the internal-API rename hazard)2121
A2drop the synthesised-line offset22 (both position tests)
A3hook rule stops surfacing the extractor's verdict (extractor untouched)11
A4react gate skips the walk on failure (the non-additive design)11 (is ADDITIVE)

Under A1 the two independent controls stay green by design — cannot throw (which calls createSourceFile raw) and the property is really there (which asserts on the compiler, not on this module). That is the non-vacuity proof: if TypeScript ever renames parseDiagnostics, every checked parse silently reverts to scoring wreckage clean and nothing else in the tree would notice.

A process note, since it affected the record: the first ablation pass restored with git checkout -- against files that were still untracked, which silently reverted two source files to HEAD and left A1's mutation live under A2–A4. The tell was that all four runs reported an identical 21 failures against very different predictions. The work was reconstructed, committed, and all four ablations re-run from a committed tree — the numbers above are from that second pass.

Verification

Gate union derived with node scripts/pm/dispatch-gates.mjs (no paths passed) and run at 2dec2dfaf5, the final commit. All 16 green, each quoting its own verdict line:

check:nul-bytes · check:parse-guard · check:cross-package-test-inputs · check:slot-lookup · check:test-source-alias · check:type-source-resolution · check:changeset-gate-self-tests · check:objectui-changeset · check-empty-changeset · check-adr-0087-registration · check-changeset-no-major · check-affected-docs · check:query-options-erasure · check:type-check-coverage · check:engine-double-contract · check:where-matcher

pnpm --filter @objectstack/lint typecheck → exit 0
pnpm --filter @objectstack/lint test → Test Files 80 passed (80) · Tests 2235 passed | 5 skipped (2240)
✓ check:parse-guard: 122 scripts/ file(s) — every TypeScript parse goes through ts-parse.mjs.

Cross-checked by hand where the derivation cannot see: check:parse-guard — the family this card is about — sits in dispatch-gates' unreachable-by-construction bucket (its population literal scripts is refused as too generic), so no path derivation could have named it. Run explicitly above.

One gate not run locally, declared:check:type-check-debt (--re-measure) needs the whole workspace closure built. @objectstack/lint is a TEST_DEBT entry (19), so its test files are measured through a generated test-inclusive project that pnpm --filter … typecheck does not see. Substitute measurement taken instead: a test-inclusive tsc program over packages/lint/src/**/*.ts reports 0 errors, none in any file this PR adds or edits, so the count cannot drift up from this change. (A first attempt at that measurement returned TS2688 — a setup failure, i.e. invalid rather than zero — and was re-run with typeRoots resolved before being read.)

Scope

One file outside the card's three, declared: packages/lint/src/validate-action-body-writes.ts. It consumes the very same extractHookBodyWriteSet parse, so wiring only the hook rule would have left the identical blind spot standing at the call site next door — which that rule's own header forbids ("an action body runs through the same HookBodySchema and the same sandbox, so it gets the same treatment"). The change there is the sibling rule id plus the finding, with its own tests and false-positive control.

Changeset

Added, minor — argued rather than defaulted, since the card asked for it explicitly.

AGENTS.md:943 exempts pure bug fixes, and this is a bug fix. It is not invisible to consumers, which is what the exemption is really about: this is published @objectstack/lint source, and it changes what the validators return. Four new rule ids reach os lint --json, os validate, Studio's finding renderer and suppressWarnings: ['<rule-id>']; a stack carrying a source with a syntax error now lints with a warning where it previously lint clean. The public surface also grows (four rule-id constants, describeParseFailure, PARSE_FAILURE_HINT, three types, and an optional parseFailure on ExtractedHookBodyWriteSet) — additive API, hence minor rather than patch. Nothing is removed or renamed, so there is no migration to carry. skip-changeset would have been wrong here for the reason the card names: tonight's other PRs were scripts/**-only and published nothing.

Adjacent finding, now measured

Filed as #10696 (unassigned; it remains open, and is not addressed by this PR). The card recorded the props-gate/syntax-gate parser divergence as deliberately unmeasured. Measured while doing this work, in both directions: 0755, '\012', 0b2 and 1__0 pass Sucrase and are rejected by TypeScript; with (o) {} goes the other way. Pinned as a regression test here, since it is also the reason this fix does not double-report.


Generated by Claude Code

…10653)
Four validators in packages/lint parsed authored source with
`ts.createSourceFile` and never read `parseDiagnostics`. The call cannot
throw, so a source with syntax errors came back as a recovery tree, was
walked like any other, and produced no findings. Two of the sites wrapped
the parse in a `try/catch` that never ran; deleting only those would have
closed nothing.
Adds a lint-local checked parse (`checked-parse.ts`) that reads the
diagnostics and RETURNS them, and wires four advisory rule ids onto it.
Additive: every finding these rules produce today they still produce.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DdCnBGcHeufjrq7drTD3wt
@github-actions

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

24 anchor(s) derived from 1 changed package(s); no hand-written page names any of them. ✅

What this run could not see
  • 1 changed file(s) yielded no anchor (packages/lint/src/index.ts) — pages documenting those are invisible to this run
  • 3 name(s) were too generic to anchor anything (single lowercase words)
  • the SDK route bridge reached 45 of 221 client-bound route-ledger rows — the other 176 have no registrar path: tail to select them, so pages documenting THEIR client methods cannot appear above, on this or any run: node scripts/docs-audit/affected-docs.mjs --bridge-coverage

Coarse fallback — 4 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): node scripts/docs-audit/affected-docs.mjs --json 47cd3ec1f8d7defc207b7a6669edafc5e9a565f4packageMentionDocs.

Which tree this was computed on

This run read content/docs from 631617a1fefdf017e30a5e0a84db30f244bbb3fa — the merge of head 2dec2dfaf52906228818b6c8ab92ab26613bd204 into base 47cd3ec1f8d7defc207b7a6669edafc5e9a565f4, which is what actions/checkout gives a pull_request run. Not the PR head.

A worktree cut from an older main holds a different content/docs, so re-deriving there can legitimately return a different list — that is a different tree, not a wrong row. To answer on the same tree:

# while this PR is open — GitHub drops the merge commit once it closes
git fetch origin 631617a1fefdf017e30a5e0a84db30f244bbb3fa && git checkout 631617a1fefdf017e30a5e0a84db30f244bbb3fa
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin 47cd3ec1f8d7defc207b7a6669edafc5e9a565f4 2dec2dfaf52906228818b6c8ab92ab26613bd204 && git checkout -B drift-repro 47cd3ec1f8d7defc207b7a6669edafc5e9a565f4 && git merge --no-ff 2dec2dfaf52906228818b6c8ab92ab26613bd204
node scripts/docs-audit/affected-docs.mjs --json 47cd3ec1f8d7defc207b7a6669edafc5e9a565f4

⚠️ That checkout carried uncommitted changes, so the commit above does not fully identify what was read.

@github-actionsgithub-actionsBot added documentation Improvements or additions to documentation tests tooling labels Aug 21, 2026
@claude

claudeBot commented Aug 21, 2026

Copy link
Copy Markdown
ContributorAuthor

PM review — verified against the tree and the diff, not the report. Approving.

The judgment call I flagged as genuinely open, you got right

@objectstack/lintpublishes — no private, version 17.1.0 — and this emits four new rule ids. So this is the one card in tonight's batch where skip-changeset would have been wrong, and you added .changeset/lint-checked-parse-findings.md at minor. Correct tier: additive new rule ids on a published package is not a patch, and nothing breaks. The changeset prose is accurate too, which matters more than usual here since it is what the release notes will compile from.

I checked its most falsifiable sentence. It says "Two of the sites carried a try/catch around the parse that never once ran" — not three, not four. On origin/main: validate-react-page-props.ts:988 is } catch { continue; } and lint-startup-registry-verdict.ts:618 is } catch { return []; }, while validate-hook-body-writes.ts has no catch at all. Exactly two. A round number would have been easier to write and wrong.

The premise was verified before building, which is the right order

0 of 10 wrecks threw — merge markers, truncated body, unterminated string/comment/template, JSX-under-TS, both ScriptKinds — and each parked 1–3 diagnostics. So the catch blocks were dead and the live path was the unread parseDiagnostics, as the card states. Pinning that in checked-parse.test.ts is what stops it rotting when TypeScript changes its mind.

The non-vacuity proof is the part that makes reading an internal API acceptable. Under A1 (checked-parse never sees parseDiagnostics), the two independent controls stayed green by design"cannot throw", which calls createSourceFile raw, and "the property is really there", which asserts on the compiler. Without those, a suite that reads parseDiagnostics and asserts on parseDiagnostics proves only that it is self-consistent.

⭐ The process incident, and why reporting it raises my confidence rather than lowering it

You restored with git checkout -- against files that were still untracked, which silently reverted two source files to HEAD and left A1's mutation live underneath A2–A4. What caught it:

"The tell was four runs reporting an identical 21 failures against very different predictions."

That is the rule working on a methodology failure rather than a code one. Four different ablations cannot legitimately produce the same number; the constant 21 was the signal, and you chased it instead of banking four confirmations. Everything reported is from the second pass, off a committed tree. This is the third time tonight that "the ablation didn't move the way I predicted" has caught something real, and the first time it caught the harness rather than the subject.

Related, and equally right: the first type-check-debt substitute measurement returned TS2688 and you called it "a setup failure, i.e. INVALID rather than zero" and re-ran it with typeRoots resolved. An environment failure read as a clean measurement is the same defect class this PR is about, one level up.

The rest that makes it reviewable

  • The fixture was re-judged, not edited.lint-startup-registry-verdict.test.ts asserted toEqual([]) for an unparseable source and passed because nothing was produced — split into the half that still holds (no startup verdict from wreckage) and the half that does not. A test passing for the wrong reason is exactly this card's subject appearing in its own test suite.
  • Additivity is a guarded property, not a claim. A4 is the ablation that fails if the walk is skipped, and the measurement behind it — a source containing 0755 keeps react-prop-missing-required and gains the parse finding beside it — is the concrete version.
  • Wrapper attribution is pinned where it is delicate. The synthesised async function __body(ctx) {…} must parse clean around a trivial body and around every ledger example, or a wrapper defect would be reported as the author's syntax error. Positions remapped into the body's own coordinates and clamped.
  • The false-positive control was measured first, not assumed: 10 of 1865 corpus files actually reach the parse (the rest are cut by the raw-text pre-filter) and 0 carry a diagnostic, so that ratchet cannot redden today.
  • The scope extension is declared and justified, not smuggled: validate-action-body-writes.ts consumes the sameextractHookBodyWriteSet parse, so wiring only the hook rule leaves the identical blind spot at the call site next door — which that rule's own header explicitly forbids. Same defect, same fix, same gate family, no competing claim on the file. Fine.
  • A1–A4 predictions written first and matched exactly (21/21, 2/2, 1/1, 1/1).

⭐ One thing your hand cross-check surfaced that is bigger than this PR

You found that check:parse-guard — the gate family this card is about — sits in dispatch-gates' unreachable-by-construction bucket, because its population literal is the bare word scripts, refused as too generic. So no path derivation could ever have named it, and you ran it explicitly.

That is the fourth instance of one root cause: #10648 (check:doc-authoring), #10664 (check:doc-formula-expressions), #10314 (check:examples-live-imports, bare examples), and now check:parse-guard (bare scripts). In my review of #10687 I wrote that a per-gate remedy replicated N times is worth pricing against an extractor-level one "before the fourth instance." It has arrived, so I am filing the class card rather than carrying it further. Not this PR's problem — you did the right thing by running the gate and saying why.

#10696 correctly filed, and filed after measuring both directions on something the card had recorded as deliberately unmeasured.

Arming once the 16 in-flight checks clear.


Generated by Claude Code

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentationImprovements or additions to documentationsize/lteststooling

Projects

None yet

Development

Successfully merging this pull request may close these issues.

packages/lint's three validators score an unparseable source CLEAN — the defect is the unread parseDiagnostics, not the try/catch

2 participants

@os-zhuang@claude