Repository navigation
refactor(typescript): project references + tsc -b typecheck; retire the R11 branches tsc enforces - #3289
Conversation
|
Size Report
Startup median (7 runs, lower is better):
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 31 files
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
… graph Review #3289 (cubic): a completeness claim carries its proof. - project-references.test.ts: the workspace package set, the root manifest workspace:* declarations, and the references lists in the root, examples/sdk, and every package tsconfig must stay equal. tsc -b fails on a missing reference it needs but tolerates extras silently (planted: an undeclared ../kernel reference in packages/xml builds clean), so the guard test is the only owner of edge freshness. Mutation-proven both ways: an extra package dir absent from the root list fails root and examples assertions; an extra reference fails the per-package equality. - insideCompiledSources documents its one divergence from the graph -- the root program also includes scripts/help-conformance-command- validator.ts, which the predicate classifies as uncompiled (fail- closed: R11 keeps the full branch set there, never narrower) -- and the boundary test now pins that file as an assertion instead of implying the exception does not exist.
…ot program Repo Guards -> 'Check numeric ranges' OOMed (exit 134, ~4 GB) on both pushed heads. Cause (#3289 CI diagnosis): freerange's loader walks the resolved tsconfig's projectReferences recursively and keeps every project's ts.Program alive simultaneously. On main the root config had no references, so fr loaded one program (~87s, green). With the #3279 references graph it loads 26 programs / 13,461 file slots and ~2.8 GB heap BEFORE analysis begins; Node's default 4 GB heap dies. Removing only the root references array makes the gate pass (112s); upstream 0.0.5 bundles the same loader, so this is not a version to pin away. Fix at the config, not the heap: scripts/freerange/tsconfig.json extends the root tsconfig — and 'extends' does not inherit references (planted with --showConfig) — so 'fr' gets exactly the single root program it analyzed on main (verified: byte-identical findings and coverage, 971/13495 fully analyzed). tsc -b keeps using the real graph. check:freerange now runs fr from that directory; the reference-free and extends-to-root properties are pinned by a guard test with the OOM as its mutation.
…through tsc -b All 25 workspace packages now declare references derived from the pnpm workspace dependency graph (their manifest workspace:* edges), the root project and examples/sdk reference the packages, and every project points tsBuildInfoFile at its own output directory so one 'tsc -b' pass builds the whole graph. 'pnpm typecheck' becomes 'tsc -b tsconfig.json examples/sdk/tsconfig.json'. Measured on this host (Apple silicon, 12 cores): - cold (package outputs + .tmp wiped): 13.2s -> 9.1s - warm (no edits): 14.2s -> 0.5s - incremental cone: touching packages/host-kit/src/archive.ts rebuilds exactly its 17-project dependent cone; kernel, contracts, xml, ad-script, selectors, session-journal-free branches stay up to date. Root and examples/sdk consume packages through the declaration outputs the graph emits (699 -> 0 package .ts inputs in the root program), so NodeNext resolution errors surface for un-exported subpaths and unknown packages, and composite rootDir fails a package->root relative escape. A missing reference alone does not fail the build (the pnpm link farm resolves the exports-map .ts targets and the program reads sources); that gap stays under R11 and is recorded on #3279.
R11 package-boundaries keeps exactly what the type graph cannot express, per the planted proofs recorded on #3279: - the undeclared workspace:* sibling import (compiled or not): planted in packages/xml, 'tsc -b' exits 0 because pnpm's root link farm resolves the specifier anyway; the manifest declaration is what the published bundle externalizes against; - the root->packages/<name>/src relative tunnel: planted in src/, tsc exits 0; the failure it prevents is Node's runtime double instantiation of a module loaded both relatively and by specifier; - the full specifier sweep for scripts/ and package-harness files, which the compiler never parses (outsideCompiledSources names the set). Retired for compiled sources (proven failing tsc itself): the package-> root relative escape (TS6059 + TS6307), unknown workspace packages and non-exported subpaths (TS2307).
… graph Review #3289 (cubic): a completeness claim carries its proof. - project-references.test.ts: the workspace package set, the root manifest workspace:* declarations, and the references lists in the root, examples/sdk, and every package tsconfig must stay equal. tsc -b fails on a missing reference it needs but tolerates extras silently (planted: an undeclared ../kernel reference in packages/xml builds clean), so the guard test is the only owner of edge freshness. Mutation-proven both ways: an extra package dir absent from the root list fails root and examples assertions; an extra reference fails the per-package equality. - insideCompiledSources documents its one divergence from the graph -- the root program also includes scripts/help-conformance-command- validator.ts, which the predicate classifies as uncompiled (fail- closed: R11 keeps the full branch set there, never narrower) -- and the boundary test now pins that file as an assertion instead of implying the exception does not exist.
…ot program Repo Guards -> 'Check numeric ranges' OOMed (exit 134, ~4 GB) on both pushed heads. Cause (#3289 CI diagnosis): freerange's loader walks the resolved tsconfig's projectReferences recursively and keeps every project's ts.Program alive simultaneously. On main the root config had no references, so fr loaded one program (~87s, green). With the #3279 references graph it loads 26 programs / 13,461 file slots and ~2.8 GB heap BEFORE analysis begins; Node's default 4 GB heap dies. Removing only the root references array makes the gate pass (112s); upstream 0.0.5 bundles the same loader, so this is not a version to pin away. Fix at the config, not the heap: scripts/freerange/tsconfig.json extends the root tsconfig — and 'extends' does not inherit references (planted with --showConfig) — so 'fr' gets exactly the single root program it analyzed on main (verified: byte-identical findings and coverage, 971/13495 fully analyzed). tsc -b keeps using the real graph. check:freerange now runs fr from that directory; the reference-free and extends-to-root properties are pinned by a guard test with the OOM as its mutation.
Rebase overlap with #3118: provider-testmu entered the workspace as the 26th package after this branch's lists were derived. It gets the same treatment as every other package - manifest-derived references and a per-project tsBuildInfoFile - plus entries in the root and examples/sdk reference lists, which the guard test now proves complete again. Its dependency on the root agent-device package is the first edge that has no composite project to point at: the root tsconfig is a noEmit entry point and TS6310 forbids referencing it, so that import type-resolves through the package's own paths mapping. The guard test encodes exactly that: root-named workspace deps drop out of the references equality and must instead have a paths mapping, or the test fails.
e604361 to
8d5df4f
Compare
…t family CI Coverage lane: the round-2 guard tests pushed package-boundaries.test.ts from 972 to 1016 lines, crossing the 1,000-line test-size tripwire, which only splits may answer. The split is along the mirrored source module: the two real-tree facade-surface gates (explicit exports, exhaustive re-exports) answer the facade-surface question shared with facade-exports.ts, not the R11 specifier/declaration rules this file mirrors, so they move verbatim to scripts/layering/facade-surface.test.ts. Move-only; the check:layering glob discovers the new path and the suite stays 292 tests.
There was a problem hiding this comment.
All reported issues were addressed across 2 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
…lapsed names Cubic review P2 on #3289: the exhaustive-re-export gate collapsed every module's re-exports into one export-NAME set, so an export removed from one source stayed satisfied by the same name re-exported from a sibling module - a single-source narrowing passed. The comparison now walks oxc's re-export EDGES grouped by module (new readFacadeReExportEdgesByModule in facade- exports.ts, the module that already owns export enumeration): each source module's declared names must be carried by an edge from THAT module, matched on the source-side name so `export { a as b }` covers `a` and `export * as ns` exempts its module as whole-surface. The check is factored into a pure helper so the planted regression - one source narrowed, the same name published by another module - runs directly: it fails on the collapsed rule and passes on the per-module rule.
The new readFacadeReExportEdgesByModule lives in facade-exports.ts, so its own forms (alias pair direction, namespace binding, bare-star rejection) are pinned there rather than only through the surface gate that consumes it.
|
The typecheck wiring looks good, but I found one problem in the R11 change at e604361: Is the smaller change enough here? That would drop only the TS2307-owned branches (unknown package, un-exported subpath) and leave the relative-escape branch alone, since it costs nothing and tsc covers it only in part. Could examples/sdk avoid typechecking root src a second time through Not blocking: the doc block on insideCompiledSources and the test at package-boundaries.test.ts:129 both say the help-conformance divergence and the root include list are pinned in project-references.test.ts, but that file has no such assertion, so add it or drop the sentence; and insideCompiledSources hard-codes regexes for packages/*/src, src and test instead of deriving them from each tsconfig include/exclude, so derive them or assert that no package tsconfig declares exclude. Take or leave both. The Cubic threads on the help-conformance classification and on pinning the references are fixed at this head, so please resolve them. I read the code only and did not run This review covers e604361. The newer heads up to f75da8b rebase onto main and add or move tests, but |
… or not Maintainer review on #3289: `if (compiled) continue;` retired more than tsc replaces. Planted the reviewer's exact case: packages/host-kit/src/r11-planted-sibling-tunnel.ts import { AppError } from '../../kernel/src/errors.ts'; pnpm exec tsc -b packages/host-kit --force -> exit 0, no diagnostics The reference redirects kernel's sources to their dist-types .d.ts, so nothing outside host-kit's rootDir is emitted and neither TS6059 nor TS6307 fires - the #3279 proof held only for the ROOT escape (TS2307 there, no project redirects it). The sibling tunnel breaks runtime identity instead: Node loads kernel once by path and once by specifier, so two AppError classes coexist and instanceof fails - invisible to any compiler. The escape branch returns for every package source; only the RESOLUTION branches (unknown package, un-exported subpath) stay compiler-owned. checkPackageInternalSites gains the requested compiled=true sibling-tunnel test, mutation-proven: re-adding the skip turns it, the escape test, and the quote-form test red. The non-blocking items are taken, both true: project-references.test.ts now asserts the root include list (pinning the help-conformance divergence the predicate's doc block promises) and the per-package include:[src]/no-exclude shape insideCompiledSources' regexes assume - mutation: adding an exclude to xml or a second scripts entry to root goes red - and check.ts's R11 summary line states the corrected split.
|
Fixed in Blocking — sibling tunnel. Ran your exact case, and Your mechanism explanation matches: the reference redirects kernel's files to their Per your rule ("reject any relative specifier that resolves outside its own package dir, compiled or not, unless a planted proof shows tsc fails it"): "Is the smaller change enough?" Yes — and that is exactly what this head now is: only the two TS2307-owned branches (unknown package, un-exported subpath) are retired for compiled sources; the relative-escape branch is whole again for every package source. I'd previously over-read "tsc covers it" from the root-escape proofs to cover sibling tunnels as well; the planted run shows the coverage is partial, so "costs nothing, keep it" is the right call and is now the code. examples/sdk / root-composite. examples/sdk does re-typecheck the root Non-blocking — both taken.
|
There was a problem hiding this comment.
All reported issues were addressed across 4 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
Cubic re-review on 63fd297: - P2: the per-package include pin filtered `*.global.d.ts` by suffix, so an unrelated include in that shape class drifted past the compiled-set predicate silently. The pin now compares exact include ARRAYS against the two shapes that exist today ([src] and replay-port/session-journal's command-registry global.d.ts sibling); extending it is a deliberate edit that says whether insideCompiledSources must widen too. Mutation: adding a made-up src/generated-env.global.d.ts include to xml goes red. - P3: the escape branch's rationale claimed every tunnel duplicates the module; duplication needs the SAME module also loaded through its package specifier (and type-only tunnels cost nothing at runtime). Doc block and violation message now state the mixing/dual-route condition, matching the file header's existing precise phrasing.
|
I reviewed f105bbf. The code looks right, and all 22 checks pass at that commit, including the typecheck, layering and freerange routes this PR changes. I did not run Not blocking, take or leave: the gross diff is about 1,394 lines, over the 1,000-line budget in docs/agents/pull-requests.md, mostly from expanded reference objects in the package tsconfigs and the moved facade tests; the compact one-line Would the smaller version work: wire the references, switch to On the open threads: all five cubic-dev-ai threads are addressed at f105bbf, so the author can resolve them. #3289 (comment) is fixed by the fail-closed note in package-boundaries.ts and its pin test. #3289 (comment) is benign, because project-references.test.ts:53 asserts both lists equal the workspace package set. #3289 (comment) is fixed by the per-module check at facade-surface.test.ts:117. #3289 (comment) is fixed by the exact include arrays. #3289 (comment) is fixed by the updated checkPackageInternalSites doc and message. |
…fo name Review nits on f105bbf (take-or-leave, both cheap): reference entries were expanded to three lines by the JSON writer that generated the first batch; provider-testmu's hand-written entry was already the compact shape, and it read better next to the manifest it mirrors. Compacting all of them also cuts ~265 lines from the gross diff. provider-testmu's buildinfo keeps the 25-package name (index.tsbuildinfo) so no project deviates from the one convention; the filename is inside the gitignored dist-types and nothing pins it.
|
Item 1 — the cwd/config confirmation, now explicit in the PR body's validation section. Finding, with code refs:
Bonus for the thing you couldn't check: TS 7.0.2's Item 2, positions:
|
|
I reviewed the changes since f105bbf at 77f3a47, and they look good. The delta only compacts the reference entries and makes the tsbuildinfo name uniform. Every reference set is unchanged, and no script outside the tsconfigs names the old tsbuildinfo path. The PR body now answers the freerange cwd question from my last review. All checks pass, there are no conflicts, and nothing else blocks this PR. |
thymikee
added
the
ready-for-human
1 participant
Summary
All 26 workspace packages declare
referencesderived from the real workspace dependency graph (each package's manifestworkspace:*edges; verified programmatically against every tsconfig).pnpm typecheckbecomestsc -b tsconfig.json examples/sdk/tsconfig.json: the root project references the 26 packages, andexamples/sdkis a composite build node referencing them too (it keeps itspathsmapping intosrc/sdk/and self-lists thesrc/closure those paths pull in).R11 package-boundaries (
scripts/layering/package-boundaries.ts+ rule-registry prose only) keeps exactly whattsc -bcannot express, with planted proofs:Rebased onto main after #3118 merged
provider-testmuas the 26th package. Its wiring is derived like the rest; the one edge with no expressible reference is its dependency on the rootagent-devicepackage (agent-device/pluginsimports) — the root tsconfig is a noEmit entry point and TS6310 forbids referencing it, so that import type-resolves through the package's ownpathsmapping. The guard test encodes exactly that rule: root-named workspace deps drop out of the references equality and must have apathsmapping, or it fails (mutation-proven by deleting the mapping).tsc -bexits 0 because pnpm's root link farm resolves the specifier anyway, and the declaration is what the published bundle externalizes against; and the root→packages/*/srcrelative tunnel —tsc -btypechecks it (exit 0); the failure it prevents is Node's runtime double instantiation, which no compiler check sees. Plus the full sweep forscripts/and package-harness files, which sit outside the type graph by design (insideCompiledSources()names the set).63fd2976a): the relative-escape branch fires for every package source, compiled or not. Planted:packages/host-kit/srcimporting'../../kernel/src/errors.ts'→tsc -b packages/host-kit --forceexit 0 (the reference redirects kernel to itsdist-types, so rootDir never sees a foreign file — TS6059/TS6307 only fire on the root escape, where no project redirects). The damage is runtime module double-instantiation, invisible to tsc. The root-escape TS6059+TS6307 proof below stands; it never covered sibling tunnels.Planted proofs on this tree (TS 7.0.2, exact errors):
packages/xmlfile importing'../../../src/daemon/session-state.ts':error TS6059: File '.../src/daemon/session-state.ts' is not under 'rootDir' '.../packages/xml/src'+error TS6307: File ... is not listed within the file list of project '.../packages/xml/tsconfig.json'(exit 2).packages/xmlimporting'@agent-device/kernel/internal-nope'(un-exported subpath) or'@agent-device/nonexistent-pkg/x'(unknown package):error TS2307: Cannot find module ... or its corresponding type declarations.(exit 2).packages/xmlimporting'@agent-device/kernel/errors'with no declaration and no reference:tsc -bexit 0 (buildinfo showskernel/src/errors.tsconsumed as source) → R11's undeclared-dependency branch stays; same for dropping host-kit's references entirely (exit 0, source fallback). This is the "an import from a package lacking a reference" case: references alone don't fence imports here because the exports maps point at.tssources; the fences that work are the ones listed above.'../packages/xml/src/index.ts':tsc -bexit 0 → R11's tunnel branch stays.Typecheck wall time (this host, 12 cores): cold 13.2s → 9.1s; warm 14.2s → 0.5s. Incremental cone proven: touching
packages/host-kit/src/archive.tsrebuilds exactly 18 projects — host-kit plus its dependents (capture-kit, device-selection, provision-kit, proxy, session-journal, the platform/provider packages that consume them, replay-port, maestro, managed-allocation, root, examples/sdk); kernel, contracts, xml, ad-script, selectors, etc. report up-to-date.Root program now consumes 698 package
.d.tsoutputs and zero foreign.tssources. Scope note: no gate-catalog or.fallowrcedits were needed (thetypecheckgate keeps runningpnpm typecheck), so there is nochore(gates)commit beyond the freerange one; every file is references wiring, the R11 pair, or the guard tests.Repo Guards
Check numeric rangesOOM (exit 134) — root cause and fix. freerange's loader (@chenglou/freerange/src/typescript/project.ts) walks the resolved tsconfig'sprojectReferencesrecursively and keeps every project'sts.Programalive simultaneously. On main the root config had no references, sofrloaded 1 program (~87s, green). With this PR's graph it loads 26 programs / 13,461 file slots ≈ 2.8 GB heap before analysis begins → Node's default 4 GB heap dies (~27s, reproduced locally and bisected: main's tsconfigs + this branch's code = green). Removing only the rootreferencesarray makes it pass (112s). Upstream 0.0.5 bundles the identical loader, so there is nothing to pin away. Fix is at the owning config, not the CI heap or a skipped gate:scripts/freerange/tsconfig.jsonextends the root tsconfig — andextendsdoes not inheritreferences(planted with--showConfig) — sofrgets exactly the single root program it analyzed on main (verified byte-identical:No lint findings, coverage971/13495fully analyzed), whiletsc -bkeeps the full graph. The gate ran before references existed and analyzed this same scope; R11/tsc -b deliver their value with the tool as shipped, unpinned and ungutted. The reference-free + extends-to-root properties are pinned by a guard test whose mutation is the OOM itself.Closes #3279
Validation
cd scripts/freerange && frreads relative to cwd (confirmed for review). The ONLY cwd-relative input is the tsconfig, found by upward search fromprocess.cwd():findTypeScriptConfig(process.cwd())→ts.findConfigFile(resolve(cwd), ts.sys.fileExists, 'tsconfig.json')(@chenglou/freerange/src/project.ts:426,src/typescript/project.ts:16-17). There is no baseline, ignore-file, or cache concept at all: the shippeddist/fr.jsbundle's entirenode:fssurface is oneexistsSync(file-mode argument existence) and it contains zeroreadFileSync/writeFileSync/mkdirSync/homedir/tmpdir references — coverage numbers are recomputed from the program every run, which is also why the byte-identical coverage claim was verifiable across configs. The remainingprocess.cwd()uses are the report'sbaseDirectory(src/lower/program.ts:14,src/ir/program.ts:434), a display-onlyrelative(baseDirectory, file)for pretty paths. So thecdexists solely to anchor the upward tsconfig search atscripts/freerange/tsconfig.json(the reference-free wrapper); it reads no config state from the directory it runs in, and moving the wrapper file itself — not the cwd — is the only way to change whatfrloads.77f3a47db(review nits: compact references + uniform tsbuildinfo):AGENT_DEVICE_VITEST_MAX_WORKERS=4 pnpm check:affected --run— all runnable checks passed. Coldtsc -bfrom wipeddist-types/.tmpalso verified exit 0 at this head (the compaction touched all 26 reference lists plus testmu's buildinfo name).f105bbf5d(cubic re-review):AGENT_DEVICE_VITEST_MAX_WORKERS=4 pnpm check:affected --run— all runnable checks passed;check:layering297/297. The include pin now matches exact arrays (["src"],["src","../command-registry/src/global.d.ts"]) instead of a*.global.d.tssuffix class — mutation: a made-upgenerated-env.global.d.tsinclude in xml goes red. The escape-branch rationale states the dual-route duplication condition, not an unconditional double load.63fd2976a(maintainer-review fix):AGENT_DEVICE_VITEST_MAX_WORKERS=4 pnpm check:affected --run— all runnable checks passed;check:layering297/297 with the restored escape branch mutation-proven (re-addingif (compiled) continue;turns three tests red) and the new include/exclude pins mutation-proven (xmlexclude→ red, second rootscripts/include → red).f75da8ba8(cubic P2 fix):AGENT_DEVICE_VITEST_MAX_WORKERS=4 pnpm check:affected --run— all runnable checks passed. The facade exhaustiveness gate now matches per source module (readFacadeReExportEdgesByModuleinfacade-exports.ts, alias-aware,export * as nswhole-surface); the planted single-source narrowing behind a duplicated name fails the new rule and passed the old collapsed-name rule (proof in the thread reply).check:layering293/293.0f1967e9c:AGENT_DEVICE_VITEST_MAX_WORKERS=4 pnpm check:affected --run— all runnable checks passed (unit lanes cover the test-file-size ratchet). The CI Coverage ratchet failure at8d5df4fb8was real: the guard-test round pushedpackage-boundaries.test.tsfrom 972 to 1,016 lines, over the 1,000-line tripwire. Fixed by splitting along the mirrored source module, move-only: the two real-tree facade-surface gates (explicit exports, exhaustive re-exports) share the facade question withfacade-exports.ts, not the R11 specifier/declaration rules this file mirrors, so they moved verbatim toscripts/layering/facade-surface.test.ts(103 lines). No pin raised: the original file is now 928 lines, below its 972 merge-base length. Discovery proven at the new path — thecheck:layeringglob picks it up and the suite stays 292 tests.8d5df4fb8(rebased onto main0c1ebb33a): freshpnpm install --frozen-lockfile+pnpm build, wipeddist-types/.tmp→pnpm typecheckexit 0 cold across the 26-project graph;pnpm check:layering292/292;pnpm check:freerangeexit 0 (No lint findings, coverage971/13547— denominator moved upstream);pnpm format/lintclean;AGENT_DEVICE_VITEST_MAX_WORKERS=4 pnpm check:affected --run— all runnable checks passed (full-set selection from the lockfile delta).e60436198:pnpm check:affected --run— all runnable checks passed;pnpm gate freerange(the exact CI invocation) passes in ~90s. Heads7c00b5636anddeea35c4apassedcheck:affected --runbefore the freerange fix landed.node --test scripts/layering/package-boundaries.test.ts— 16/16, including a mutation-proven test that keeps the compiled/uncompiled boundary honest;pnpm check:layeringgreen (R11 summary line updated).