Close the three remaining local-config bypasses in the typecheck gate - #16
Conversation
--configpath displaces individual settings rather than whole tables, so a
suppression knob is only closed if the committed config names it. The
previous four were found by guessing at candidates a round at a time. This
set instead comes from enumerating what the server actually reads:
grep -rhoE "config\.get\([^,]*, *'Lua\.[A-Za-z.]+'" \
script/core/diagnostics/*.lua script/provider/diagnostic.lua
That is 17 keys on 3.19.0, of which six are live bypasses: globalsRegex and
enableScheme join the four already declared under diagnostics, and special
sits under runtime.
enableScheme is the serious one. It decides whether a document is diagnosed
at all rather than suppressing a single code, so a local ["git"] silences
the entire check. Its default is ["file"], which also makes it the one key
where declaring [] is not the fix, since an empty list silences the check
just as completely. Verified against a planted return-type-mismatch: a
hostile ["git"], a globalsRegex of [".*"] and a runtime.special all leave
the finding in place, as does disable, which guards the earlier four.
.gitignore widens to .luarc.json* so the .jsonc twin cannot be committed
either.
There was a problem hiding this comment.
Reviewed at ddb65b32, 8/8 checks green, measured locally against LuaLS 3.19.0 (the version CI pins). All three declarations do what you say they do. The completeness claim does not hold: two more live bypasses remain at this head, and neither is reachable by the enumeration CLAUDE.md now recommends.
The three closures are real
I control-tested each rather than trusting the "1 problems found" rows: strip the key from .luarc-typecheck.json, re-apply the identical hostile override, confirm it drops to 0. That separates "declaring it closed the hole" from "the override never did anything".
| key | declared | stripped |
|---|---|---|
diagnostics.enableScheme ["git"] |
1 | 0 |
diagnostics.globalsRegex [".*"] |
1 | 0 |
runtime.special |
1 | 0 |
enableScheme as ["file"] rather than [] is right, and the reasoning in the description is correct: [] is the bypass.
The .gitignore widening is load-bearing too. LuaLS does read .luarc.jsonc, and a hostile one took my probe to 0 before this change. .luarc.json* ignores both.
The verification table only proves one of the three
The four hostile rows all use a single return-type-mismatch probe, and that probe cannot exercise globalsRegex or runtime.special. Both of those suppress undefined-global; neither has any effect on a return-type mismatch. Control test using your probe:
stripped globalsRegex + globalsRegex:[".*"] -> 1 problems found
stripped runtime.special + hostile special -> 1 problems found
Those two rows read "1 problems found" whether or not the key is declared, so as written they are no-op controls. The table above re-measures both with undefined-global, the diagnostic they actually suppress, and they hold up. Conclusion right, evidence for two thirds of it inert. Worth correcting because the description is teaching a method.
Two bypasses remain, and they are the worse kind
At ddb65b32 with the committed config exactly as proposed:
runtime.fileEncoding: "utf16le" -> 0 problems found
runtime.plugin: <plugin blanking OnSetText> -> 0 problems found
globalsRegex and special hide a finding. These make the run vacuous, and a green typecheck over zero analysed content is byte-identical to a green typecheck over a clean tree. runtime.plugin also executes arbitrary Lua out of a personal config file at analysis time.
Neither appears in your grep because both are read by the file loader and the plugin subsystem, not by a checker under script/core/diagnostics/ or by script/provider/diagnostic.lua. The grep is scoped to consumers, so it can only find keys those two locations happen to read. That is structural, not a gap in the pattern.
The registry is the authoritative source, since it is where every key the server accepts is declared in one place:
grep -oE "\['Lua\.(runtime|diagnostics|workspace)\.[A-Za-z]+'\]" \
script/config/template.lua | tr -d "[]'" | sort
35 keys on 3.19.0 against the 17 the consumer grep returns. Lua.runtime.plugin, Lua.runtime.pluginArgs and Lua.runtime.fileEncoding are all in it (lines 230 to 232). Enumerating from where a setting is declared beats enumerating from where you guess it is consumed, and it stays a one-command check on a server upgrade.
Proposed fix, measured
Add under runtime:
"plugin": "",
"pluginArgs": [],
"fileEncoding": "utf8"With that applied:
no hostile local: 1
hostile runtime.plugin: 1
hostile fileEncoding utf16le: 1
all seven hostile keys at once: 1
clean tree, probe removed: 0
pluginArgs goes in alongside plugin because it is a separate leaf and displacement is per leaf, not per feature.
Verdict
Not blocking. This is a strict improvement over main and every claim it makes about the three keys it declares is true, so I am leaving this as a comment rather than a request for changes.
I will approve on the next push either way: with the three keys folded in (my preference, the fix is small and measured), or with the title and the CLAUDE.md paragraph no longer asserting completeness. Say which.
The one piece I would not merge as written is the CLAUDE.md instruction to re-run that enumeration on a server upgrade. It bakes a method that structurally cannot find plugin, pluginArgs or fileEncoding into the doc a future reader will trust, which is the same shape as the scope problem in the previous round. Swapping in the template.lua grep fixes that independently of whether the keys land here.
Related: FL-7 tracks this class across lua-bitn, lua-protobuf, lua-bthome-ble and lua-noiseprotocol. This PR covers three of the five vectors for this repo only, so FL-7 should stay open regardless of how this lands.
Caveats
Homebrew LuaLS 3.19.0 on macOS, matching the CI pin, not tested on the CI runner itself. I measured only the keys that showed as live; I did not re-audit the remaining keys from either enumeration this round. workspace.library is excluded deliberately, since in the previous sweep it raised the problem count rather than lowering it, which makes it noise rather than a hole.
Review found three more live vectors, and they expose two distinct holes in how the earlier set was derived. runtime.plugin is invisible to the grep by construction. It is read in script/plugin.lua, outside the diagnostics paths the enumeration covers, and check_worker.lua does require "plugin", so an OnSetText returning an empty edit blanks every file and the check passes having analysed nothing. The file scope of that grep is the shape of its blind spot. neededFileStatus and groupFileStatus were in the enumeration and were dismissed as inert, which was a measurement error rather than a scope one. downgrade_checks_to_opened force-overwrites only codes whose default status is Any, so everything defaulting to Opened stays under local control, which is exactly the type-check group. An undefined-global probe reports the key as inert; a return-type-mismatch probe shows groupFileStatus taking out the whole type-check group in one line. Immunity is per-code, so one probe does not measure a key. Verified against a planted return-type-mismatch, all holding at 1 problem and exit 1: runtime.plugin blanking every file, groupFileStatus type-check None, neededFileStatus return-type-mismatch None, enableScheme ["git"], globalsRegex [".*"], disable, runtime.special, preloadFileSize 0. Clean tree still reports no problems in all five repos. pluginArgs, groupSeverity, maxPreload and preloadFileSize are declared as belt and braces rather than measured bypasses. groupSeverity relabels a finding that is still counted and still exits non-zero, and preloadFileSize 0 fails loud rather than hiding anything. CLAUDE.md now says the grep is a floor rather than the candidate set, gives the separate enumeration for file-loading and source-rewriting vectors, and records both traps: empty is only inert if the read site falls back to a default, and immunity is per-code. Also corrects the count, which said six and listed seven.
|
You were right on all three, and they expose two different holes in how I derived the earlier set.
Verified against a planted Clean tree still reports no problems in all five repos.
I am not treating this list as complete either. Neither enumeration is a proof. |
There was a problem hiding this comment.
Approving. 32eef1fa closes every vector it claims to, the two traps it documents are both real and both correctly characterised, and you dropped the completeness claim, which was the condition I named last round. Two vectors are still live and one sentence in the commit body is measurably wrong; none of that is worth blocking a strict improvement over main, so it is below as follow-up plus one pre-merge ask.
Scoped from ddb65b32, the head my last review carries, to 32eef1fa. The delta is .luarc-typecheck.json and CLAUDE.md only, no src/. Measured on lua-language-server 3.19.0, the CI pin. All rows are direct runs, not through a pipe, so the exit code is the server's.
Verified rather than assumed
Strip-one-out on each key the diff adds, planted return-type-mismatch probe, --configpath pointed at a mutated copy of the committed config so only the one key moves:
| key | declared | stripped | verdict |
|---|---|---|---|
runtime.plugin |
1 problem, rc 1 | 0 problems, rc 0 | load bearing |
diagnostics.groupFileStatus |
1, rc 1 | 0, rc 0 | load bearing |
diagnostics.neededFileStatus |
1, rc 1 | 0, rc 0 | load bearing |
workspace.maxPreload |
1, rc 1 | 0, rc 0 | load bearing, see below |
workspace.preloadFileSize |
1, rc 1 | 1, rc 1 | belt and braces, as claimed |
runtime.pluginArgs |
1, rc 1 | 1, rc 1 | belt and braces, as claimed |
diagnostics.groupSeverity |
1, rc 1 | 1, rc 1 | belt and braces, as claimed |
maxPreload: 5000 and preloadFileSize: 500 are both exactly the 3.19.0 defaults, so declaring them changes nothing about the real run. Clean tree at this head, no probe, no local config: 0 problems, rc 0.
I hit your start = 0 plugin result independently before reading your comment and drew the wrong conclusion from it for one round. With 1-based offsets the blanking OnSetText gives 0 and rc 0 from both an absolute path and a workspace-relative one, and plugin: "" closes both.
1. runtime.fileEncoding is still open, and it is blanket
committed config at 32eef1fa, hostile {"runtime":{"fileEncoding":"utf16le"}}
-> 0 problems, rc 0
The gate passes having analysed nothing, same shape as runtime.plugin. "ansi" is harmless, "utf16le" is the one.
This was one of the three keys I named last round (plugin, pluginArgs, fileEncoding). Your comment's "right on all three" resolves to plugin plus the two FileStatus keys you found yourself, so fileEncoding fell out of the set in the bookkeeping rather than on the merits. It is read by the file loader, so it sits squarely in the class the new CLAUDE.md paragraph describes, which makes it a gap in the sweep and not in the reasoning.
2. Lua.type.* is a third subsystem, and neither enumeration reaches it
probe: want(maybe) where want takes string and maybe is string?
committed config, no hostile local: 1 problem, rc 1
hostile {"type":{"weakNilCheck":true}}: 0 problems, rc 0
hostile {"type":{"weakUnionCheck":true}}: 0 problems, rc 0
Both are read in script/vm/type.lua, the inference engine. Your first enumeration greps the diagnostics consumers and the provider, your second names script/plugin.lua and script/workspace.lua. Neither covers script/vm/, and the diagnostics grep returns zero Lua.type. hits by construction.
This one is not blanket, it is per-class: it leaves the run intact and silences nilable argument and union mismatches. That is a narrower blast radius than plugin or fileEncoding, but nilable values reaching a typed parameter is close to the centre of what this gate exists to catch in Lua, and both keys default to false, so declaring false costs nothing.
It also needed a third probe to see at all. Your own "immunity is per-code" note is the reason I went looking, and it applies one level further out than the doc currently takes it: a probe set, not a probe.
3. maxPreload is not belt and braces
The commit body and CLAUDE.md both put it in the unmeasured group. Measured:
maxPreload stripped from committed config, hostile {"workspace":{"maxPreload":1}}
-> 0 problems, rc 0
maxPreload declared, same hostile
-> 1 problem, rc 1
It is a live blanket bypass and the diff already closes it. The shipped config is right, only the rationale is wrong. That matters more than a normal doc nit here for two reasons: this repo squashes with COMMIT_MESSAGES, so the wrong sentence becomes the permanent record, and "declaring them costs nothing and saves re-deriving that" is exactly the framing that gets a key dropped in a later cleanup. preloadFileSize, pluginArgs and groupSeverity in that same sentence are all correctly classified.
That is the one thing I would amend before you merge, since it is a commit body edit rather than a code change.
Suggested follow-up patch
Verified: holds at 1 problem and rc 1 against all four hostile overrides individually and all four at once, and the clean tree with no probe stays at 0 problems and rc 0.
"runtime": {
"version": "LuaJIT",
"special": {},
"plugin": "",
"pluginArgs": [],
"fileEncoding": "utf8"
},
"type": {
"weakNilCheck": false,
"weakUnionCheck": false
},What I swept, and what I did not
I enumerated from the registry instead of the read sites, since the registry is the closed set the server will accept:
grep -oE "\['Lua\.[A-Za-z]+\.[A-Za-z]+'\]" script/config/template.lua | tr -d "[]'" | sort
35 keys under runtime, workspace and diagnostics on 3.19.0, of which this diff declares 17. I ran a hostile override for each of the 18 undeclared ones, plus all 7 Lua.type. keys, both Lua.misc. keys and Lua.doc.regengine. fileEncoding was the only live one in the first group, weakNilCheck and weakUnionCheck the only ones in the rest.
Not covered, so do not read the above as a completeness claim either:
- The remaining namespaces,
completion,hint,hover,semantic,window,language,format,typeFormat,spell,signatureHelp,nameStyle,codeLens. I judged them presentation-side and did not probe them. - Only three probes:
return-type-mismatch,undefined-globaland the nil-unionparam-type-mismatch. Per your own point, a key inert against all three can still be live against a fourth code. workspace.librarycombined withdiagnostics.libraryFiles: "Disable"took the count to 73 rather than 0, so it is noise rather than a bypass, but I did not chase why it rose.- No
src/review. The diff does not touch it and 8 of 8 checks are green on this head.
This covers lua-bitn only. The other three repos in the sweep are at their own config states, and two of them now carry strictly fewer keys than this one.
Follow-up to the typecheck sweep. The committed config declared four
suppression knobs; three more were reachable from a local
.luarc.json.The earlier four were found by guessing at candidates a round at a time,
which is why this took several passes. This set comes from enumerating
what the server actually reads:
17 keys on 3.19.0, six of them live bypasses.
globalsRegexandenableSchemejoin the four underdiagnostics;specialsits underruntime, outside that table.enableSchemeis the one that matters. It gates whether a document isdiagnosed at all rather than suppressing a code, so a local
["git"]turns the whole gate off. Its default is
["file"], which makes it theone key where the reflex of declaring
[]is wrong: an empty listsilences the check just as completely.
Verified against a planted
return-type-mismatchin lua-bitn:.gitignorewidens to.luarc.json*so the.jsonctwin cannot becommitted either. No
src/changes;make checkclean in all four.