Typecheck the whole workspace so make and the editor agree

[svc-finitelabs]

Follow-up to the non-blocking item in the #2 review: Int64HighLow defined twice, in src/crypto/annotations.lua:32 and vendor/bitn.lua:1742, reported as duplicate-doc-alias by an editor but invisible to make typecheck.

The suggested fix alone would have regressed the check

The review suggested dropping the alias from annotations.lua and letting bitn's stand. --check "$(CURDIR)/src" never scans vendor/, so removing the alias put bitn's definition out of scope and reinstated the 46 undefined-doc-name reports that annotations.lua exists to eliminate: 11 problems to 57.

So the duplicate is a symptom and the scope is the cause

An editor opens the repository as its workspace; the target scoped itself to src/. @alias resolves workspace-wide, so a narrower scope does not give a subset of the findings, it gives a different set: that is exactly why one of us saw the duplicate and the other did not.

Widening the scope alone is not reproducible

Round 1 reported 11 problems, the review measured 18 on the same branch. runtime.version is the whole of that gap, and .luarc.json is where it was set:

runtime.version	result
unset (defaults to Lua 5.4)	11 problems in 3 files
LuaJIT	18 problems in 4 files
Lua 5.1	25 problems in 5 files

.luarc.json is matched by a global gitignore, so it has never been in the repository and the target read whichever private config the developer happened to have. Round 1's config had no runtime.version and checked this library as Lua 5.4; LuaJIT is what it actually targets, and under it vendor/bitn.lua reports 7 of its own.

The fix

.luarc-typecheck.json is committed and the target pins it with --configpath, leaving .luarc.json as personal editor preference. vendor is listed both as a workspace.library and in ignoreDir: as a library its @alias definitions still resolve, and via ignoreDir its files are not diagnosed. Both halves are load-bearing:

config	result
ignoreDir without library	57 (definitions lost, undefined-doc-name returns)
library without ignoreDir	18 (vendor diagnosed)
both	11 problems in 3 files, all in src/

No undefined-doc-name and no duplicate-doc-alias, so Int64HighLow still resolves from vendor/bitn.lua with the alias dropped from annotations.lua.

Verification

On lua-language-server 3.19.0. The target now prints the server version, because the remaining 11 are tool-version sensitive.

• make typecheck: 11 problems in 3 files (aes_gcm 5, ed25519 3, srp 3), the same param-type-mismatch set as before, and unchanged with a deliberately conflicting .luarc.json planted in the working tree
• ./run_tests.sh: 15/15 modules pass
• make format-check: clean

make typecheck exits non-zero whenever it reports anything, on main as well as here, which is why it is not part of check.

Not verified locally: make lint. luacheck 1.2.0 fails under this host's Lua 5.5 (attempt to assign to const variable 'field_name' in its own standards.lua), which is an environment problem rather than a repo one. CI's luacheck leg covers it.

FL-4

[derek-miller]

Im not sure we need all of the added comments. Re-review with emphasis on if these comments you added are adding value

[svc-finitelabs]

Re-reviewed at b018dc6, 11/11 checks green. Taking the three comments separately, because I don't think they're the same call. Short version: one earns its keep, one should be halved, one should be deleted. Plus a factual error in the commit message that is headed for main.

Delete: the Int64HighLow tombstone in annotations.lua

You're right about this one. The "64-bit values on 32-bit-safe runtimes" section now defines nothing: it is a banner, three lines of preamble, and three lines explaining why there is nothing under it. The file went 32 -> 35 lines while going 3 -> 2 alias definitions, and its largest section is empty.

The case for keeping it is that it stops someone re-adding the alias. That case doesn't survive a test. I re-added it and ran the target:

head:            11 problems
alias re-added:  13 problems
                 +1 duplicate-doc-alias   src/crypto/annotations.lua
                 +1 duplicate-doc-alias   vendor/bitn.lua

The mistake announces itself, on both files, in one make typecheck. A comment guarding a failure the tooling already reports loudly is redundant; comments earn their space when the failure is silent.

The 3-line preamble about Lua 5.1/5.2 semantics is orphaned now too: it explains a type this file no longer owns. It can't move next to the definition either, since vendor/bitn.lua is a generated amalgamation. Drop the whole section. If that rationale is worth keeping it belongs in sha512.lua/blake2.lua, where the halves actually get manipulated.

Keep: the one-clause header change

and a type already defined in vendor/ stays there rather than being restated here is the durable form of the same lesson: one clause, sitting with the file's other rules, stating the rule instead of the incident. It also makes the tombstone redundant twice over.

Keep but halve: the Makefile block

Here I'd push back, partly. The comment block above typecheck went 11 -> 18 lines, which is too much. But unlike the tombstone, this failure is silent. "Why are we typechecking vendor/?" is a plausible, well-intentioned future cleanup, and narrowing the scope back doesn't error, it quietly changes which problems get reported. That is exactly the kind of constraint a comment should carry.

I checked whether there's a way to have both, so this isn't a false choice. There isn't:

.luarc.json  workspace.ignoreDir=["vendor"], diagnostics.ignoredFiles=Disable  ->  57 problems
             workspace.ignoreDir=["vendor"], diagnostics.ignoredFiles=Opened   ->  57 problems

ignoreDir drops vendor's definitions along with its diagnostics, so the 46 undefined-doc-name come straight back. Whole-repo scope is the only option, which is itself worth knowing.

So keep the constraint, drop the narrative (the PR body is its right home):

#
# Scope is the whole repo, not `src/`: `@alias` resolves workspace-wide, so
# narrowing it changes which problems are reported rather than reducing them.
# `build/` is still skipped, the server honours .gitignore.

The .gitignore sentence stays on purpose: "won't this scan build/ after make build?" is the first question a reader has, and answering it heads off the same wrong revert.

Separately: the commit message says something untrue

Both files now record why the alias is not defined in src.

Only annotations.lua does. The diff is Makefile + annotations.lua; vendor/bitn.lua:1742 is untouched and is still a bare --- @alias Int64HighLow ... with no rationale, and it shouldn't be touched, being generated. This matters more than a stale line normally would: the repo is squash_merge_commit_message: COMMIT_MESSAGES, so that sentence lands in main's permanent commit body. The PR body carries the same claim.

Not about comments, but worth knowing

Widening the scope couples make typecheck to a vendored third-party file. Confirmed by injecting a bad annotation into vendor/bitn.lua: it adds undefined-doc-name and param-type-mismatch to the target's output. Today vendor contributes 0 of the 11, so it's clean, but a future re-vendor of bitn can redden the target for reasons that aren't this repo's. Given the .luarc.json result above there's no fix available, so this is a thing to know rather than a thing to change.

Verification

• make typecheck at b018dc6: 11 problems in 3 files, all under src/ (4 assign-type-mismatch + 1 return-type-mismatch in aes_gcm, 3 param-type-mismatch each in ed25519 and srp). Zero from vendor/ or tools/.
• Counterfactuals above run on lua-language-server 3.19.0 on this host, tree restored clean after each.
• Not re-run by me: run_tests.sh and make lint. All 11 CI checks are green at this head.

Want me to push the trim? It's the section deletion, the halved Makefile block, and the commit-message fix, so the code change stays the single path edit. Note I can't move reviewDecision on this one either way: GitHub blocks formal self-review on our own app's PR, so this is a comment rather than a verdict.

[derek-miller]

Requesting changes on two counts. The scope diagnosis is right, and measuring that my suggested fix alone would have regressed to 57 problems is a good catch, so the direction is not in question.

1. The numbers do not reproduce, and make typecheck fails

Same machine, same lua-language-server 3.18.2-dev, run back to back:

main,  --check src         Found 11 problems in 3 files
FL-4,  --check $(CURDIR)   Found 18 problems in 4 files    (make exits 1)

The PR states this "lands on 11 problems, identical to the previous baseline". Here it is 18. Per file:

8  src/crypto/aes_gcm.lua
6  vendor/bitn.lua
2  src/crypto/srp.lua
2  src/crypto/ed25519.lua

Two things follow. vendor/bitn.lua contributes 6 of its own, which contradicts "vendor/bitn.lua contributes nothing of its own". And aes_gcm went from 6 to 8 without its scope changing, so this is not a pure scope delta.

Most likely a different language server version on your side. Either way, pin the version in the target or note it in CLAUDE.md: a check whose baseline moves with the tool cannot be "identical to the previous baseline" for anyone else, and this is the second time a stated number has not held on my machine.

Tests and lint are fine: 15/15 modules, and make lint reports 0 warnings / 0 errors across 20 files, which covers the leg you could not run locally.

2. Comment volume

14 lines of comment for a one-word change in the Makefile, and a 5-line note in annotations.lua explaining an absence. Both make the same point three times.

Makefile, replacing the 7 added lines:

# Checks the whole repo, not just src/: an editor's workspace is the repo, and
# @alias resolves workspace-wide, so a narrower scope gives different findings
# rather than fewer.

annotations.lua, replacing the 5-line block:

-- Int64HighLow is defined in vendor/bitn.lua, where these halves come from.

This is a pattern across recent PRs rather than a one-off: the explanation often runs longer than the change and restates itself. One sentence for the non-obvious reason, then stop.

[derek-miller]

Following up on the count discrepancy, because I found the cause and it also answers "keep vendor's type defs, drop its diagnostics".

The mechanism

vendor in both workspace.library and workspace.ignoreDir. As a library its definitions load for resolution; via ignoreDir its files are not diagnosed. Measured on this branch:

ambient config, vendor untouched     18 problems in 4 files
vendor as library + ignoreDir        11 problems in 3 files

All 11 are in src/. No undefined-doc-name, no duplicate-doc-alias, and zero mentions of Int64HighLow, so it still resolves from vendor/bitn.lua with the alias dropped from annotations.lua as you have it. That is the original baseline with vendor contributing nothing, which is what the PR was aiming for.

Why the numbers disagreed

.luarc.json is not in the repository. It is matched by a global gitignore (~/.gitignore_global:30), so it has never been committed and cannot be by adding a repo rule.

So make typecheck currently reads whichever private config the developer happens to have. Mine excludes build, dist and others; yours evidently differs. That is the 11-versus-18, and it means the target is not reproducible today, which matters more than either number.

Incidentally this also corrects the PR body: build/ is skipped here because my local config lists it in ignoreDir, not because the server honours .gitignore. That config sets useGitIgnore: false.

Suggested shape

Commit a config for the check and point the target at it, leaving .luarc.json as personal editor preference:

lua-language-server --check "$(CURDIR)" --checklevel=Warning \
    --configpath="$(CURDIR)/.luarc-typecheck.json" --logpath="$(CURDIR)/build/luals"

{
  "runtime": { "version": "LuaJIT" },
  "workspace": {
    "useGitIgnore": false,
    "library": ["vendor"],
    "ignoreDir": [".claude", ".git", ".idea", ".venv", "build", "dist", "node_modules", "vendor"]
  },
  "diagnostics": { "disable": ["unnecessary-assert"] }
}

Verified with --configpath against exactly that file: 11 problems in 3 files, reproducible regardless of local config. Worth recording the lua-language-server version alongside it, since the remaining 11 are tool-version sensitive.

[svc-finitelabs]

Pushed as 9245a39. Your config is committed as .luarc-typecheck.json and the target pins it with --configpath, both comment blocks are cut to your wording, and the commit message is rewritten. Verified here: 11 problems in 3 files, and unchanged with a deliberately conflicting .luarc.json planted in the tree, so --configpath does override an ambient one.

One correction to the mechanism, because it changes what the config is doing.

The cause is runtime.version, not the presence of .luarc.json

I have no .luarc.json at all, and ambient here is 11, not 18. Isolating one key at a time against this branch:

runtime.version unset (defaults to Lua 5.4)    11 problems in 3 files
runtime.version = LuaJIT                       18 problems in 4 files
runtime.version = Lua 5.1                      25 problems in 5 files

That is the whole of the 11-versus-18. Round 1 had no runtime.version, so it type-checked a Control4 library as Lua 5.4, and vendor/bitn.lua is clean under 5.4 and reports 7 of its own under LuaJIT. So "vendor contributes nothing" was not a measurement of vendor, it was an artifact of checking the wrong language version. Your 18 was the more correct number.

This makes the library + ignoreDir pair load-bearing rather than tidy-up. Both halves are needed only because LuaJIT is right:

ignoreDir without library    57   definitions lost, undefined-doc-name returns
library without ignoreDir    18   vendor diagnosed
both                         11   all in src/

Round 1 told you "you cannot have both". That was wrong, and library is the piece I never tested. Retracted.

The tool version is a second, separate variable

Your 18 split aes_gcm 8, bitn 6, srp 2, ed25519 2. Mine splits aes_gcm 5, bitn 7, srp 3, ed25519 3. Same total, different distribution, so 3.18.2-dev and 3.19.0 genuinely disagree per file on top of the runtime gap. Both land on 11 in 3 files under the committed config, which is the reproducibility you asked for, but I only compared totals there, not the per-file split. The target now prints the version so any quoted count carries it.

Two small things

diagnostics.disable: ["unnecessary-assert"] is inert on 3.19.0: 11 either way. I kept it verbatim on the assumption it is load-bearing on 3.18.2-dev, but if it is not, it should go.

.luarc.json can be committed over a global gitignore, with !.luarc.json in the repo's .gitignore, since repo rules outrank core.excludesFile. Not proposing it, your split is the right one, just flagging the claim.

On comment volume

Taken, and it is the same pattern you flagged on #2. The Makefile block is your three lines plus one sentence on --configpath, which is the non-obvious part. annotations.lua is your single line.

I also rewrote the commit message rather than adding a commit on top, because this repo squashes with COMMIT_MESSAGES, so the old body would otherwise have carried "the server honours .gitignore, so build/ is skipped" and "vendor/bitn.lua reports nothing of its own" onto main permanently. Both are now false: the config sets useGitIgnore: false and lists the directories explicitly. That is a force-push onto the same parent, so b018dc6..9245a39 is the round-2 diff.

make typecheck exits non-zero whenever it reports anything, on main as well, which is why it is not in check. Unchanged by this PR, but worth stating since the review noted it.

Verified: 15/15 modules, make format-check clean. make lint still not runnable here (luacheck 1.2.0 under Lua 5.5); you already covered that leg.

[derek-miller]

Approving 9245a39. Verified on 3.18.2-dev: make typecheck 11 problems in 3 files, matching your number; 15/15 modules; make lint 0 warnings / 0 errors across 20 files; make format-check and make build clean. Comments are down to four lines and one line.

unnecessary-assert is inert on 3.18.2-dev too. 11 problems either way and no such diagnostic is emitted, so it is not load-bearing on the older server. Drop it.

The runtime.version isolation is the better diagnosis and it corrects mine. I said the count moved because the config was untracked; the actual variable was that an absent runtime.version defaults to Lua 5.4, so round 1 type-checked a LuaJIT/Control4 library as 5.4. Making the version explicit in the committed config is what makes any of these numbers mean anything, and it turns library + ignoreDir into a real requirement rather than tidiness.

You are also right that !.luarc.json in the repo's .gitignore would override core.excludesFile. My "cannot by adding a repo rule" was wrong. The split you shipped is still the one I would pick, since it separates editor preference from what the check pins.

[svc-finitelabs]

Post-merge Lua Tests on main is green (31272464352).

One correction to what landed, since the squash body is main's permanent record: "the remaining 11 are tool-version sensitive" does not hold. I ran both binaries against this tree, and 3.18.2-dev and 3.19.0 agree everywhere, including per file:

                    3.18.2-dev      3.19.0          split (aes_gcm/bitn/srp/ed25519)
ambient LuaJIT      18 in 4 files   18 in 4 files   5 / 7 / 3 / 3
committed config    11 in 3 files   11 in 3 files   5 / - / 3 / 3

The per-file disagreement in this thread was one run counted three ways, not two tools:

--check_format=json                  5 / 7 / 3 / 3    18
pretty output, every diagnostic     10 / 7 / 3 / 3    23
pretty output, line-anchored grep    8 / 6 / 2 / 2    18

The last is your tally, and I reproduced it exactly by grepping that way before I checked myself. The pretty stream is not safely countable: the progress bar shares its line with the next diagnostic, so a line-anchored count drops one per batch, and aes_gcm prints 10 messages across 5 positions. --check_format=json is the countable one.

So the version print is harmless but treats a non-cause, and the worry behind your review point retires rather than staying open: it is 11 in 3 files on your version too, verified on the 3.18.2-dev binary rather than inferred.

On the open question from the last round: diagnostics.disable: ["unnecessary-assert"] is inert on 3.18.2-dev as well, at Warning, Information and Hint. It fires nowhere in this repo on either version.

Also re-checked on the merged tree, independently of the round-2 comment: --configpath does beat a conflicting in-tree .luarc.json (still 11), the check still catches an undefined @alias in src/ (a planted one takes it to 13, so "no undefined-doc-name" is live evidence and not a dead check), and a clean run mentions Int64HighLow zero times, so it resolves from vendor/bitn.lua with annotations.lua's copy gone.