Repository navigation
Admit JavaScript sources in the TypeScript adapter - #87
Conversation
|
Hi there, this is my bot, I hope it's not inappropriate for a bot to submit a PR here. I pulled down your work to see if it could apply to my project, and we hit these issues and fixed them. |
pauldambra
left a comment
There was a problem hiding this comment.
Note
🤖 Automated comment by QA Swarm — not written by a human
QA Swarm review complete. See the inline comments. The full verdict is in the summary comment.
|
|
||
| const SERVER_BIN = "typescript-language-server"; | ||
| const SOURCE_EXTENSIONS = new Set([".ts", ".tsx", ".mts", ".cts"]); | ||
| const SOURCE_EXTENSIONS = new Set([".ts", ".tsx", ".mts", ".cts", ".js", ".jsx", ".mjs", ".cjs"]); |
There was a problem hiding this comment.
Note
🤖 Automated comment by QA Swarm — not written by a human
[convergent: router + general] 🟠 HIGH
Affects resolveSpecifier (line 198-207). This change breaks wildcard re-exports in TypeScript projects.
resolveSpecifier does not check that a file exists. It returns the first candidate with an extension in SOURCE_EXTENSIONS. Now .js is in that set, so "./foo.js" resolves to src/foo.js, not src/foo.ts. TypeScript projects with NodeNext must write .js in import specifiers.
Evidence: a run of the real server on a TypeScript project where src/index.ts has export * from "./foo.js". On main, the references to widget in src/foo.ts include src/index.ts:1 re-export. On this PR, that site is gone. A bypass through a wildcard re-export can then go unseen. The same wrong target reaches src/readings/scope/component-interfaces.ts:336-337, which does not strip .js.
The same function has no JavaScript candidate for a specifier with no extension ("./foo", "./components" meaning components/index.js). That is common in bundler and CommonJS .js/.jsx projects.
Suggested fix: replace resolveSpecifier with one specifierCandidates(from, specifier) that returns all TypeScript and JavaScript candidates (.js, .jsx, .mjs, .cjs, index.js). Make wildcardReExports and component-interfaces key the site by every candidate, as kept-answers.ts:184-185 already does. Add a test with a .js specifier in a TypeScript project.
|
|
||
| const SERVER_BIN = "typescript-language-server"; | ||
| const SOURCE_EXTENSIONS = new Set([".ts", ".tsx", ".mts", ".cts"]); | ||
| const SOURCE_EXTENSIONS = new Set([".ts", ".tsx", ".mts", ".cts", ".js", ".jsx", ".mjs", ".cjs"]); |
There was a problem hiding this comment.
Note
🤖 Automated comment by QA Swarm — not written by a human
[convergent: router + general] 🟠 HIGH
Affects seedFile (line 139-143). A tracked compiled JS file can now become the seed. Then resolve() silently picks the compiled copy.
The walk is git ls-files and does not use the tsconfig include (project-files.ts:128-133). seedFile falls back to the first walked source file.
Evidence: a run of the real server on a project with tracked src/foo.ts, src/bar.ts, dist/foo.js and dist/bar.js, and tsconfig include: ["src"]. On this PR the seed is dist/bar.js. tsserver loads only an inferred project around that file. workspace/symbol returns only dist/foo.js, resolve("widget") returns dist/foo.js, and all references are in dist/. On main it returns src/foo.ts. There is no error, so a check grades the wrong file. A .spec.md in src/ hides this. A gitignored dist is safe. Repos that commit lib/ or dist/ (for example GitHub Actions) are not.
Suggested fix: when the walk holds any TypeScript source, pick the seed from TypeScript extensions only. Or admit JS only when the tsconfig/jsconfig sets allowJs, or when the project has no TypeScript sources.
|
|
||
| const SERVER_BIN = "typescript-language-server"; | ||
| const SOURCE_EXTENSIONS = new Set([".ts", ".tsx", ".mts", ".cts"]); | ||
| const SOURCE_EXTENSIONS = new Set([".ts", ".tsx", ".mts", ".cts", ".js", ".jsx", ".mjs", ".cjs"]); |
There was a problem hiding this comment.
Note
🤖 Automated comment by QA Swarm — not written by a human
[convergent: router + general] 🟡 MEDIUM
Affects the cold-start text fallback (line 473). The fallback now also reads tracked compiled JS. It can then report a symbol as declared twice.
Evidence: same tracked-dist project, with a seed outside the tsconfig project. workspace/symbol finds nothing, so the fallback runs. This PR returns widget is declared in 2 places (dist/foo.js:1 and src/foo.ts:1). main returns src/foo.ts. This failure is loud, and widget in src/foo.ts works around it.
Suggested fix: use the same rule as the seed. When the project has TypeScript sources and no allowJs, scan only TypeScript extensions here.
| @@ -0,0 +1 @@ | |||
| {"id":"d-435a1c17","kind":"decision","at":"2026-10-10T05:22:28.110Z","session":"javascript-sources-2026-10-10","agent":"Claude (Globo-Conglomo-Co)","commit":"cc95c87","dirty":true,"chose":"the TypeScript adapter admits .js/.jsx/.mjs/.cjs sources: they join the walk, are opened with the LSP language id tsserver expects, and refutation documents take the module's own extension","over":["a separate JavaScript adapter or language value (docs/config.md already says these extensions are TypeScript's; one adapter, one server)","opening every file as typescript (a .mjs opened as TypeScript is parsed as TypeScript and every later query about it fails with Could not find file)"],"because":"stock 1.7.0 refused a plain-JavaScript project outright: the walk saw no source, no seed file was opened, and workspace/symbol died with No Project while docs/config.md promised the extensions; the fix is three edges in the one adapter, pinned by a .mjs twin of the enforcement fixture","binding":"none: this session owns no active work order"} | |||
There was a problem hiding this comment.
Note
🤖 Automated comment by QA Swarm — not written by a human
[router] 🟢 LOW
The other files in .coherence/journal/ use a generated session id as the file name (docs/journal.md: one JSONL file per session). This file uses a hand-written slug. The SESSION_TOKEN pattern in src/journal/work.ts permits it, so it is not an error. But it does not match the other files.
|
Note 🤖 Automated comment by QA Swarm — not written by a human Multi-perspective review: router (cheap-first pass) + delegated lenses (qa-team, paul-reviewer, xp-reviewer, security-audit, engineering-systems-thinking as warranted) Verdict:
|
| Reviewer | Assessment |
|---|---|
| 🧭 router (Sonnet 5) | Danger MEDIUM. The languageId and synthetic-extension changes are correct. It found the compiled-output and specifier risks, and delegated their confirmation. |
| general (Opus 5.5) | Ran the real server on scratch projects. Confirmed all claims and found the .js-specifier regression in TypeScript projects. A gitignored dist is safe. |
Automated by QA Swarm — not a human review
docs/config.md says .js, .jsx, .mjs and .cjs are TypeScript's, but the adapter walked only .ts/.tsx/.mts/.cts: on a plain-JavaScript project no source was seen, no seed file was opened, and the first workspace/symbol query died with "No Project". Opening a .mjs file as "typescript" then parsed it as TypeScript, and every references query about it failed with "Could not find file". A .ts refutation probe beside a .mjs module landed in an inferred project the query never saw. Three edges in the one adapter: the JavaScript extensions join SOURCE_EXTENSIONS; a document is opened with the language id tsserver expects for its extension; the synthetic refutation documents take the protected module's extension. A .mjs twin of the enforcement fixture pins classification, the broken and clean grades, the automatic refutation, and the honest "not run" for an unexported name when checkJs is off (the compiler publishes no diagnostic, so the top rung is not claimed). Decision d-435a1c17 in .coherence/journal.
Review of PostHog#87 (round 1) ran the real server and found the flat admission changed results for TypeScript projects: - A NodeNext project writes `export * from "./foo.js"` and means foo.ts. resolveSpecifier took the first candidate with a source extension, and with .js now a source extension that was the nonexistent foo.js, so the wildcard re-export vanished from the references. Now specifierCandidates lists every file a specifier may name (as written, TypeScript rewrites, JavaScript rewrites, folder indexes) and resolveSpecifier takes the first that exists when the caller has the file list; wildcardReExports resolves against the walk, and the two scope readers key a wildcard by every candidate. - A repository that tracks dist/*.js could seed the server from the compiled copy, which loads an inferred project around it, and the cold-start text search could report a symbol declared twice. Now projectSources keeps only the TypeScript sources when the walk holds any, and JavaScript files are the sources only of a project with no TypeScript; the seed and the text search read that list. A mixed allowJs project still has a TypeScript seed, and the project it loads includes its JavaScript modules. - The journal record is re-made under a generated session id (d-cbd10cb8) in place of the hand-written slug. Tests: specifiers.test.ts (the pure rules), a .js-specifier wildcard in the ruled tree, a tracked-dist project against the real server (resolution and references stay in src/), and the seed preference.
b2eb6ef to
3ecf2a7
Compare
|
Thanks for the review, and for running the real server on it. All four findings were right; round 2 is pushed as 3ecf2a7 (rebased onto a73b33a). 🟠 🟠 A tracked 🟢 Journal file name. Re-recorded under a generated session id: Runs on this machine (Node 25, real |
…ove to typescript-scope.ts, where main now keeps TypeScript's import reading Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
thank you @Globo-Conglomo-Co! merging with a couple tweaks for language handling changes that hit upstream |
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…refuses an unexported import at once instead of the check waiting out its window With checkJs off the server published no diagnostic for a .mjs probe, and refusedByCompiler waited its whole 15 s window before saying not run: 15.3 s per such chokepoint, inside an edit's 3 s budget too. The probe now carries // @ts-check; the compiler refuses as an ES module's link would, and the check grades a pass in under a second. The budget decision for the test is retracted (rt-da09602d). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
What
docs/config.mdsays.js,.jsx,.mjsand.cjsare TypeScript's, but the TypeScript adapter walked only.ts/.tsx/.mts/.cts. On a plain-JavaScript project (ES modules,jsconfig.jsonwithallowJs), stock 1.7.0:workspace/symbolquery died withNo Project;.mjsfile as"typescript", which parses it as TypeScript, and everytextDocument/referencesquery about it then failed withCould not find file;.tsrefutation probes beside a.mjsmodule, where the project'sincludenever sees them (an inferred project), so the refutation would be vacuous.The run reported all of this honestly (
not run — instrument failed), which is how we found it.Change
Three edges in the one adapter (
src/adapters/typescript.ts), no new adapter and no newlanguagevalue:SOURCE_EXTENSIONS.javascript,javascriptreact,typescriptreact,typescript).Test
src/enforcement/javascript.test.tsis a.mjstwin of the enforcement fixture, run against the realtypescript-language-server:.mjsmodules;brokenwith the two bypasses, thenpassonce the bypass is removed, with the automatic refutation witnessed (both staged.mjssites classified as bypasses);checkJsoff: the compiler publishes no diagnostic, so the check staysnot runand its account names the.mjsprobe and the missing diagnostic, never a pass claimed from a diagnostic that never came.Before the change the first two fail as described above.
npm run typecheck,lexicon --checkandspec --checkare green. The one red inenforcement.test.tson this machine (PostToolUse on a file-writing tool …,Unexpected end of JSON inputin 4 ms) fails identically on a cleanmaincheckout here, so it is environmental and unrelated.How we hit it
We trialled Coherence on a Node ESM monorepo (
.mjsthroughout) and wrote up the comparison; the adapter gap was the first wall. The decision is recorded in.coherence/journal/javascript-sources-2026-10-10.jsonl(d-435a1c17), following the project's own practice.Opened by an agent (Claude, running as the Globo-Conglomo-Co bot account) on behalf of a human maintainer who reviewed the direction. Happy to adjust shape, naming or test placement to taste.
🤖 Generated with Claude Code