Cut local lint peak memory from 15.4 GB to 4.3 GB (Fixes #3387) - #3395
Merged
Conversation
`npm run lint` asked for a 12 GB heap and exhausted it, putting the repo's own lint gate out of reach for contributors on 16 GB machines. The issue attributed the cost to the sum of every package's type program and proposed per-package invocations. Measurement did not support that: capped at a 6 GB heap, per-package runs of cli, agents, providers, core, mcp and telemetry all died. The driver is the root tsconfig.json. It declares no `include`, so TypeScript's default `**/*` applies and the project spans the whole repository. Six package tsconfigs excluded individual test files that fail `tsc`; ESLint still linted them, typescript-eslint found no package project containing them, and fell back to that whole-repo program. On packages/cli/src/config/config.test.ts, one invocation, nothing else changed: 8,769,847,296 B / 46.96 s while excluded, versus 1,640,644,608 B / 7.06 s once included. The cost is flat per process, so one such file paid it in full. Two changes: - Each affected package moves its exclusion list into a sibling tsconfig.noemit.json that extends tsconfig.json, and points its typecheck script there. `tsc` reads the same file set as before, verified by comparing resolved `files` from --showConfig: identical in all six packages. `references` is repeated in those configs because TypeScript does not inherit it through `extends`, and losing it fails typecheck with TS6059. - run-lint.ts partitions a full run into one ESLint process per packages/<pkg> plus one for `.` with packages ignored. The halves are exact complements, so the union is the file set of `eslint .`, and the partition is read from the filesystem so a new package needs no runner edit. Scoped runs are partitioned per target for the same reason. Each group gets its own cache file, since ESLint rewrites the whole cache with just the files it linted. Lint failures now accumulate rather than stopping at the first group, so one broken package cannot hide findings in the other sixteen; a signal termination still aborts immediately. Measured end to end, cold, no cache: 16,586,620,928 B / 242.74 s before, 4,610,539,520 B / 161.62 s after. Peak drops 3.6x, wall clock drops 33%. DEFAULT_HEAP_MB and the package.json numbers drop to 6144, which is what the largest group actually needs. lint:ci is unchanged and remains a single-process `eslint .`; routing it through the runner requires changing scripts/eslint-guard, which is tracked as follow-up in the plan.
Coverage hole, found by review and reproduced against ESLint 9: the rest-of-tree group ignored `packages/**`, which drops a file sitting directly in `packages/` as well as the package subtrees. `readPackageDirs` yields directories only, so no group covered such a file. The complement is now `packages/*/**`, which excludes the contents of each immediate package directory and leaves direct entries to the rest group. The repository has no such file today, which is why the whole-tree file-set comparison did not catch it. The unit test for coverage was circular: it fed package discovery back into the builder and asserted the same names came out, so it passed with the hole present. scripts/tests/run-lint-partition.test.ts now runs the real ESLint binary over a real fixture containing a direct file in `packages/`, a dot-named package and ordinary packages, and compares the file set of `eslint .` against the union of the partitioned commands. Reverting the pattern to `packages/**` makes it fail. Per-group ESLint cache files are reverted to the single shared location. .github/workflows/ci.yml caches that exact path, so suffixed files would have silently disabled CI lint caching. The split was unnecessary anyway: ESLint merges into an existing cache rather than pruning entries for files the current run did not visit, verified by running two package groups against one cache file and confirming both packages' entries survived. `--no-error-on-unmatched-pattern` is now applied only to the per-package groups of a full run, whose targets are read from the filesystem and where `packages/lsp` is legitimately all-ignored. Scoped targets come from CI's affected-target selector, so a stale or mistyped one fails loudly again instead of passing with zero files linted. Execution semantics were claimed but untested. `executeLintCommands` is extracted and exercised with real child processes: every group runs when an earlier one fails, the first failing exit code propagates rather than the last, and a signal termination aborts the remaining groups. The tsconfig guard checked only literal existing file entries in the raw `exclude` array, so a glob or a narrowed `include` could orphan files while it stayed green. It now resolves each package project with TypeScript's config parser and asserts every type-aware-linted source file under `packages/<pkg>/src` is in its own package's resolved file set, allowing only the `allowDefaultProject` entry eslint.config.js declares. Adding `src/debug/**` to a package exclude makes it fail. The noemit configs now set `noEmit` themselves, so a direct `tsc -p tsconfig.noemit.json` cannot write build output into the sources, and their header comments describe the actual split rather than implying the whole list moved. Reviewer's proposal to pin the moved files to the root `lib`/`types` is rejected and the reasoning recorded in the plan: a test file in packages/agents runs under agents' settings, so linting it as though ES2023 were available reports on a program that does not exist. Files that were not excluded already linted under their package's settings; this makes the excluded ones match their siblings rather than giving any file weaker treatment. Final measurement on this tree: 4,565,237,760 B peak RSS in 149.25 s, against 16,586,620,928 B in 242.74 s before.
github-actions
Bot
added
the
maintainer:e2e:ok
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
Included review availability: Your plan provides up to 4 included reviews per hour; 1 remains after this review. 📝 WalkthroughWalkthroughThe lint runner now partitions full runs by package and executes groups sequentially. Package typecheck scripts use dedicated ChangesLint execution and validation
Package TypeScript projects
Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: ⚪ Minimal · up to This change reduces lint memory usage and partitions linting while preserving the documented file and typecheck scope; no actionable merge-blocking risk remains beyond normal checks and review. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Full details: Linked Issues checkExplanation The changes satisfy issue Full details: Out of Scope Changes checkExplanation The configuration changes, lint-runner partitioning, memory reduction, and supporting tests directly support issue ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
WalkthroughThis PR changes 25 file(s).
Changes
Magnitude🎯 4 (XL) RelatedNo related items found. Pre-merge Checks
Walkthrough generated by LLxprt PR Review. Planner issue: #2256 |
scripts/tests/issue-2994-lint-scoped.bun.test.ts composed the runner argv and asserted the arguments of `commands[0]`, which encoded the old behavior of one ESLint process carrying every scoped target. A scoped run is now one process per target, so those three assertions described a shape the runner no longer produces. They now assert the argv of every command: one per target, `--fix` forwarded to each, and the shared cache flags on each. This is the same coverage expressed against the current contract, not a relaxation. Caught by CI's scripts shard; missed locally because the file was not in the set I ran.
Contributor
OpenCodeReview — automatic reviews suspendedAutomatic OCR reviews are suspended for this PR after 2 of 2 automatic reviews. To get more reviews you can:
OpenCodeReview — PR #3395
OCR stderr excerptOCR preflight excerptOCR preview stderr excerpt
|
CodeRabbit's docstring-coverage check reported 70% against an 80% threshold over the functions this PR touches. Adds short comments to the runner's command factory and CLI entry point, and to the test helpers that load the real runner module, read a tsconfig exclude list, enumerate package directories and walk a package source tree.
acoliver
added a commit
that referenced
this pull request
Aug 30, 2026
Cover packages/*/test-bun/ in npm run typecheck (Fixes #2995) Conflict resolution: #3395 moved the agents/cli/providers typecheck scripts onto a dedicated tsconfig.noemit.json project to cut lint/typecheck peak memory, while this PR chained a second pass over tsconfig.test-bun.json. Both intents are kept by chaining the two projects: tsc --noEmit -p tsconfig.noemit.json && tsc --noEmit -p tsconfig.test-bun.json storage/tools auto-merged because they have no noemit project.
acoliver
added a commit
that referenced
this pull request
Aug 30, 2026
…reads Integration fallout between #3303 and #3387/#3395. #3303 added useBracketedPaste.test.tsx next to the useBracketedPaste.test.ts that was already on dev. When two files in a directory differ only by extension, TypeScript's include resolution keeps the higher-priority one (.ts) and silently drops the other, so the new file was never typechecked: `tsc -p packages/cli/tsconfig.json --showConfig` listed the .ts and not the .tsx. #3395's tsconfig-project-coverage guard reported it as an orphan, and lint failed with "was not found by the project service". The new file contains no JSX, so it never needed .tsx. It also supersedes the older test rather than complementing it: both cover the same hook, but the new one diffs listener identity sets instead of counting, asserts that the registered cleanup is one shared reference and that invoking it really emits the disable sequence, and adds a repeated mount/unmount leak check. It is therefore kept as useBracketedPaste.test.ts, replacing the weaker file. The only thing the old file had that the new one lacks is a local IS_REACT_ACT_ENVIRONMENT assignment, which packages/cli/bun-test-setup.ts already sets globally for every CLI test. Verified: 4 tests pass, tsconfig-project-coverage is green, no other .ts/.tsx basename collisions exist in packages/.
acoliver
added a commit
that referenced
this pull request
Aug 30, 2026
…onfigs as JSONC Two failures from the full Bun suite after the Wave A/B merges. 1. release-like pack smoke died with EEXIST creating the work-copy node_modules symlink. #3388 moved that link out of packReleaseLikeCli and into copyRepoExcludingGenerated, via a new linkRepoNodeModules helper that guards both the source and the destination. dev had meanwhile edited the inline call it replaced (adding a win32 'junction' : 'dir' ternary), so the textual merge kept dev's inline block *and* took #3388's helper, and the link was attempted twice. Removing the inline block restores exactly the shape #3388 intended; the surviving helper is strictly better, since it is idempotent and returns instead of throwing when node_modules is absent. Note this deliberately follows #3388 in dropping the old hard precondition. 2. The #3360 coverage guard parsed tsconfig.test-bun.json with JSON.parse and threw on the comments this branch added to the agents config. tsconfig is JSONC and this repository comments its tsconfigs freely, so the guard was one comment away from breaking regardless. It now uses ts.readConfigFile, matching the parseTsconfig helper #3395 added for the same reason. package.json parsing is left on JSON.parse, which is correct: manifests are strict JSON. Verified: both previously failing files pass (release smoke 5/5, coverage guard 12/12).
acoliver
added a commit
that referenced
this pull request
Aug 30, 2026
Remove the vestigial gemini-cli Declarative Agent Framework (Fixes #3152) Conflict: packages/agents/tsconfig.json "exclude". This PR still carried the long per-file exclusion list that used to live there; #3395 has since moved that list into tsconfig.noemit.json, deliberately leaving tsconfig.json excluding only node_modules and dist so typescript-eslint resolves every in-package file against its own project. Took the post-#3395 form. Follow-on: this PR deletes packages/agents/src/agents/, and seven of those files were named in the relocated exclusion list, which would have left tsconfig.noemit.json pointing at paths that no longer exist. Removed those seven entries: src/agents/__tests__/executorRun.characterization.test.ts src/agents/executor.execution.test.ts src/agents/executor.recovery.test.ts src/agents/executor.stream-idle-timeout.test.ts src/agents/executor.termination-conditions.test.ts src/agents/executor.test.ts src/agents/invocation.test.ts Verified: tsconfig-project-coverage 7/7, agents noemit project compiles, npm run typecheck clean.
acoliver
added a commit
that referenced
this pull request
Aug 30, 2026
Remove expired token compat and inert runtime surfaces (Fixes #2535) Conflict: packages/mcp/src/auth/file-token-store.ts and its test, both modify/delete (this PR deletes them; HEAD had modified them). Took the deletion. The only change on the HEAD side came from #3305 making the MCP package standalone, and it was purely mechanical import rewiring: @vybestack/llxprt-code-core/utils/errors.js -> .../llxprt-code-tools/... @vybestack/llxprt-code-core/utils/debugLogger.js -> .../llxprt-code-telemetry/... That is not a reason to keep a file this PR removes as dead surface. The store was already superseded by packages/mcp/src/auth/token-storage/, which both sides keep, and a repository-wide search finds no remaining reference to file-token-store or FileTokenStore outside the deleted files themselves. Verified: npm run typecheck clean, no dangling references. Unrelated observation, left alone deliberately: packages/core/tsconfig.noemit.json still excludes src/debug/DebugLogger.test.ts and src/policy/policy-engine.test.ts, which #3243 deleted before any of this. That staleness arrived with #3395's exclusion list and is a no-op for tsc, so it is not touched here.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
TLDR
npm run lintasked Node for a 12 GB heap and exhausted it, so contributors on 16 GB machines could not run the repo's own lint gate. It now peaks at 4,565,237,760 B (4.25 GiB) and finishes in 149.25 s, against 16,586,620,928 B (15.45 GiB) / 242.74 s before. Cold, no cache, same machine. The default heap inpackage.jsonandscripts/run-lint.tsdrops from 12288 to 6144, which is what the largest group actually needs.The cause was not what the issue assumed, so the two things reviewers should look hardest at are the diagnosis in
project-plans/issue3387/README.mdand the coverage proof below.Dive Deeper
The issue's hypothesis did not survive measurement
The issue attributes the cost to "the sum of every package's type program" and proposes per-package invocations. That does not hold. Capped at a 6 GB heap, per-package runs of
cli,agents,providers,core,mcpandtelemetryall died withReached heap limit. Partitioning alone cannot reach the target.What was actually happening
The root
tsconfig.jsondeclares noinclude, so TypeScript's default**/*applies and the project spans the entire repository.Six package tsconfigs excluded individual test files that fail
tsc(cli 303 entries, providers 247, agents 206, core 178, mcp 59, telemetry 6). ESLint still linted those files. typescript-eslint walked up from each one, found its package project did not contain it, and landed on the root config, building a whole-monorepo program to serve it.On
packages/cli/src/config/config.test.ts, one ESLint invocation, nothing else changed:packages/cli/tsconfig.jsonexcludeexcludeConfirmation that the root config was the provider: pointing the root
includeat a nonexistent path makes ESLint reportconfig.test.ts was not found by the project service. The cost is flat per process, not per file. One orphan file cost 8,769,847,296 B; six cost 8,692,482,048 B. Same program either way.Change 1: give every source file its own package project
Each affected package moves its exclusion list into a sibling
tsconfig.noemit.jsonthat extendstsconfig.json, and points itstypecheckscript there.tscverifies the same files as before.Verified rather than asserted: the resolved
filesarrays fromtsc --showConfigare byte-identical before and after in all six packages (426, 2364, 683, 374, 839, 90 files respectively; zero added, zero removed).referencesis repeated in those configs on purpose. TypeScript does not inherit it throughextends, and dropping it turns referenced project sources into ordinary inputs of a composite project, which fails typecheck with TS6059. That was caught during development, not shipped.Per-package effect, all still linting clean:
Change 2: partition the full run
scripts/run-lint.tsemits one ESLint process perpackages/<pkg>directory plus one for.with--ignore-pattern 'packages/*/**'. The partition is read from the filesystem, so a new package is linted the day it appears without editing the runner.The pattern is
packages/*/**, notpackages/**. Review caught that the latter also drops a file sitting directly inpackages/, which no package group covers because discovery yields directories only. Reproduced against ESLint 9 and fixed.Also in the runner:
--no-error-on-unmatched-patternapplies only to the per-package groups, wherepackages/lspis legitimately all-ignored. Scoped targets and the rest-of-tree group keep the error, so a stale CI target still fails loudly.node_modules/.cache/eslint, the exact path CI caches. Per-group cache files would have silently disabled CI lint caching. Splitting was unnecessary anyway: ESLint merges into an existing cache rather than pruning entries for files the current run did not visit, verified directly.Coverage is proved, not argued
The acceptance criterion is that a bare
npm run lintdoes not lint fewer files. Checked by collecting--format jsonfromeslint .and from every partitioned command and comparingfilePathsets across the real repository:What is not in scope
lint:ciremains a single-processeslint .and still asks for 12 GB. Change 1 brings it from 15.45 GiB to 13.41 GiB, but routing it through the runner requires changingscripts/eslint-guard/config-scanner.ts, which demands a literaleslinttoken carrying--max-warnings 0in that script.scripts/pre-push-check.shandnpm run preflightboth use it. Recorded as a follow-up in the plan.The issue's stretch goal of 4 GB is not reached.
packages/cli/tsconfig.jsonpulls providers, auth, mcp, settings and ide-integration sources into one program; splitting that is a separate change.Review also proposed pinning the moved files to the root
lib/typesso type-aware rules see ES2023. Rejected, with reasoning recorded in the plan: a test file inpackages/agentsruns under agents' settings, so linting it as though ES2023 existed reports on a program that does not. Files that were never excluded already linted under their package's settings, so this makes the excluded ones match their siblings rather than giving any file weaker treatment. Lint is clean before and after over the identical 5386-file set, so no diagnostic was suppressed.Reviewer Test Plan
npm run lintfrom a clean checkout. It should print 17 labelled groups and finish well under the old 12 GB. To see the ceiling:/usr/bin/time -l npm run linton macOS,/usr/bin/time -von Linux.filePathvalues against/tmp/full.json.npm run typecheckshould behave exactly as onmain. To check parity directly, comparenpx tsc -p tsconfig.noemit.json --showConfigin a changed package againstnpx tsc -p tsconfig.json --showConfigonmain; thefilesarrays match.tsconfig.jsonexclude, thenbun test scripts/tests/tsconfig-project-coverage.test.ts;PACKAGES_IGNORE_PATTERNback topackages/**, thenbun test scripts/tests/run-lint-partition.test.ts.Both were verified to fail on those mutations and pass when restored.
bun test scripts/tests/run-lint.test.ts scripts/tests/run-lint-partition.test.ts scripts/tests/tsconfig-project-coverage.test.ts(36 + 8 + 7 tests). The partition tests spawn the real ESLint binary against a fixture containing a file directly inpackages/, a dot-named package and ordinary packages. The execution tests spawn real child processes to observe failure accumulation, exit-code fidelity and signal abort.Testing Matrix
Verified on macOS:
npm run test,npm run lint,npm run typecheck,npm run format,npm run build, and thestepfun-37CLI smoke run all pass. The runner uses no platform-specific paths, but the ESLint ignore-pattern behaviour has only been exercised on macOS.Linked issues / bugs
Fixes #3387
Summary by CodeRabbit
Bug Fixes
Type Checking
Tests