Skip to content

Build: Lint core/src and integrations/src with type information - #625

Open
AmaadMartin wants to merge 5 commits into
mainfrom
feat/type-aware-eslint-core-src
Open

Build: Lint core/src and integrations/src with type information#625
AmaadMartin wants to merge 5 commits into
mainfrom
feat/type-aware-eslint-core-src

Conversation

@AmaadMartin

@AmaadMartin AmaadMartin commented Aug 4, 2026

Copy link
Copy Markdown
Owner

Please ensure you have read the contribution guide before creating a pull request.

Link to Issue or Description of Change

  1. Link to an existing issue (if applicable):
    N/A

  2. Or, if no issue exists, describe the change:

Problem: ESLint runs today with no type information. The root eslint.config.js never sets languageOptions.parserOptions.project, so every rule that needs the type checker is inert — no-floating-promises, no-misused-promises, await-thenable, the no-unsafe-* family. In a codebase built on async generators, live bidirectional model connections and MCP sessions, that is a large unlinted class of defect.

Solution: One files-scoped type-aware block in eslint.config.js covering core/src/**/*.ts and integrations/src/**/*.ts, wired to the existing core/tsconfig.json and integrations/tsconfig.json. No new tsconfig, no tsconfig edits, no new dependency, no script change. Every type-checked rule was measured; the 11 that reach zero findings are enabled, and the 12 that do not are explicitly off with their measured counts recorded in the config and below.

Two real defects that the type checker surfaced are fixed at the source (see Behaviour changes).

Collision check (done before any code was written)

gh pr list --repo AmaadMartin/adk-js --state open --limit 1000 (524 open PRs) plus gh pr diff --name-only on every plausibly adjacent PR.

One correction to the approved design

The design called for appending the block at the end of the config. Measured, that silently disables 18 core correctness rules on core/srcno-undef, no-const-assign, no-unreachable, no-dupe-keys, constructor-super, and 13 more — because tseslint.configs.recommendedTypeCheckedOnly bundles typescript-eslint's eslint-recommended, which switches those off, and a trailing block wins over the js/recommended that re-enables them. The block is therefore placed among the presets, ahead of the repo's own "**/*.ts" block, which keeps the last word. This is the layering the file already relies on. The invariant is pinned by a test, not left to comment discipline.

Cost measurement

Local workstation (AMD EPYC 7B13, 64 vCPU, 117 GB RAM), Node v22.22.2, eslint@9.39.4, typescript-eslint@8.61.1, typescript@5.9.3. Warm npm install, npm run build done first, ESLint cache off. Median of 3.

npx eslint "**/*.ts" Median wall clock Peak RSS
Baseline (main) 8.12 s (8.04 / 8.12 / 8.40) 711,784 KB
Type-aware (this PR) 16.22 s (16.22 / 16.22 / 16.72) 1,300,740 KB
Delta +8.10 s (2.00x) +588,956 KB (1.83x)

Interactive / pre-commit path (lint-staged runs eslint --fix on staged files):

Single file Median of 3
eslint --fix core/src/runner/runner.ts (type-aware) 5.31 s (5.25 / 5.41 / 5.31)
eslint --fix core/test/utils/task_test.ts (not type-aware) 2.04 s

16.22 s is far below the ~2 min threshold at which the design asked for a parserOptions.projectService comparison, so the explicit project array ships as specified. projectService: true was benchmarked anyway, since it is the documented alternative and removes the hand-maintained array: it is functionally equivalent over this scope (a rule requiring a program reports the same 82 findings, zero fatal messages) but slower — median 16.88 s (16.87 / 16.88 / 17.39) against 16.22 s, at the same peak RSS. Both numbers are reported here rather than swapping silently; project stays.

TIMING=15 top 10:

Rule Time (ms) Relative
@typescript-eslint/no-misused-promises 2334.387 41.0%
@typescript-eslint/no-unused-vars 998.452 17.5%
@typescript-eslint/await-thenable 532.522 9.3%
@typescript-eslint/no-floating-promises 387.831 6.8%
@typescript-eslint/unbound-method 276.452 4.9%
no-redeclare 200.542 3.5%
no-unexpected-multiline 62.458 1.1%
constructor-super 51.381 0.9%
no-regex-spaces 50.500 0.9%
@typescript-eslint/ban-ts-comment 41.247 0.7%

constructor-super, no-redeclare, no-unreachable, no-this-before-super and no-dupe-keys appearing here is independent confirmation that the 18 core rules are still executing on the type-checked files.

Triage: every type-checked rule, measured

Counts are over core/src + integrations/src; no rule fired anywhere else.

Enabled — 11 rules at zero findings.

Rule Findings Decision
no-floating-promises 0 enable
no-misused-promises 0 enable
unbound-method 0 enable
only-throw-error 0 enable
no-implied-eval 0 enable
no-for-in-array 0 enable
no-array-delete 0 enable
no-duplicate-type-constituents 0 enable
no-unsafe-unary-minus 0 enable
await-thenable 1 -> 0 enable, fixed
prefer-promise-reject-errors 1 -> 0 enable, fixed

Deferred — 12 rules, 285 findings, each off with its count in the config.

Rule Findings Files What it needs
no-unnecessary-type-assertion 82 38 Mechanical eslint --fix sweep; its own review.
require-await 69 37 Many are async interface overrides with nothing to await; enabling it tends to produce exactly the eslint-disable comments this change avoids.
no-unsafe-member-access 42 5 Typed JSON.parse boundaries.
no-unsafe-assignment 34 11 Typed JSON.parse boundaries.
restrict-template-expressions 26 14 Shared unknown -> message helper for the 83 catch clauses.
no-unsafe-enum-comparison 8 4 ADK declares its own TaskState / MessageRole enums while the A2A SDK types those fields as string-literal unions. Reconciling the two is a design decision, not a local fix.
no-unsafe-call 6 4 Typed JSON.parse boundaries.
no-redundant-type-constituents 4 4 3 are one-word fixes; the 4th is rootAgent: BaseAgent | any on the public App type, entangled with a pre-existing no-explicit-any suppression.
restrict-plus-operands 4 2 Same stringification helper.
no-base-to-string 4 4 Same stringification helper. llm_request.ts:69 concatenates systemInstruction, a genuine latent [object Object].
no-unsafe-argument 3 3 Typed JSON.parse boundaries.
no-unsafe-return 3 3 Typed JSON.parse boundaries.

require-await measured 68 before the await-thenable fix and 69 after: removing the stray await leaves getCanonicalUri with nothing to await. The config records the post-fix number.

no-unnecessary-type-assertion is deferred rather than swept in this PR because 82 findings across 38 files is not the "few findings, each with a local fix" the enable criterion asks for, and because fork PR #332 already contains exactly that sweep — a follow-up should adopt those fixes rather than redo them. This is a stated scope decision, not an omission.
Every one of the 12 is a pre-existing finding deferred for staged adoption, not a suppression this change introduces — all 285 exist on main today and are simply invisible because no rule can see them. None is silenced at a call site: each is one off entry carrying its measured count, and the config comment says to burn the count down and re-enable. Four follow-up tasks are queued covering all 12 rules (the no-unsafe-* family via typed JSON.parse boundaries; restrict-template-expressions / no-base-to-string / restrict-plus-operands via a shared error_utils.ts helper; the no-unnecessary-type-assertion sweep; and require-await / no-unsafe-enum-comparison / no-redundant-type-constituents). No TODO(<id>) marker is written into the config because this repository has no public tracking issue for the work, and inventing one would be worse than the prose.

Behaviour changes

Two, both from fixing a rule finding at its root rather than suppressing it:

  1. AsyncQueue.error() now takes Error, not unknown (core/src/utils/async_queue.ts). Its iterator rejected with whatever it was handed; the sole caller forwards the live API's ErrorEvent, which is not an Error, so consumers lost the stack trace and instanceof Error was false. core/src/models/google_llm.ts now wraps it: new Error(error.message, {cause: error}), keeping the original event reachable. Covered by a new test.
  2. getCanonicalUri no longer awaits getArtifactDir (core/src/artifacts/file_artifact_service.ts). getArtifactDir returns a plain string; the other six call sites do not await it. No observable change — the function is async either way — but the rule now prevents it recurring.

One // eslint-disable-next-line no-undef is removed from rest_api_tool.ts: with type information the parser resolves BodyInit as a type-only reference, so the directive became unused and started emitting a warning. This change adds no suppression and removes one.

Notes

  • The type-aware block reuses the existing per-package tsconfigs rather than adding one. Those programs are already known type-clean, since npm run build runs tsc --emitDeclarationOnly over exactly those files in CI.
  • Widening to the test trees is deliberately out of scope: they are in no tsconfig include, and typescript-eslint errors on a parsed file no project contains. dev/src is also excluded — 19 of its files import @google/adk, which resolves through core/dist, so its findings would depend on whether the tree happens to be built. Both are pinned as not type-aware by the test, so widening later has to be a deliberate edit.

Testing Plan

Please describe the tests that you ran to verify your changes. This is required for all PRs that are not small documentation or typo fixes.

Unit Tests:
[x] I have added or updated unit tests for my change.
[x] All unit tests pass locally.

  • tests/integration/lint_config/lint_config_test.ts (new, 15 tests, ~4.5 s) pins the gate: the project wiring and tsconfigRootDir for both type-checked trees; no-floating-promises / no-misused-promises / await-thenable at error; the deferred rules at off; all 18 core rules still at error; and core/test/** + dev/src/** still parsed without type information. It also drives real ESLint end to end: lintFiles on core/src/utils/task.ts reports zero errors, and — the proof that type information is genuinely loaded — enabling a rule that requires a program runs clean over core/src but throws requires type information for the test tree.
  • core/test/models/google_llm_test.ts — one new test (existing tests untouched) asserting the live onerror path surfaces an Error with the ErrorEvent as cause.
  • Targeted runs, all green: google_llm_test.ts (22), and async_queue + gemini_llm_connection + core/test/artifacts + core/test/tools/openapi_tool (15 files, 232 tests).
  • Full CI gate set on the pushed commit: npm run build, npm run lint, npm run format:check, npm run docs:check all exit 0.

Proof each new test can fail. Every mutation was applied to the real tree, run, and reverted:

# Mutation Result
1 Delete the type-aware block from eslint.config.js 7 failedexpected undefined to deeply equal [ './core/tsconfig.json', …(1) ], expected undefined to be 2, and Error while loading rule '@typescript-eslint/no-unnecessary-type-assertion': You have used a rule which requires type information...
2 Add "core/test/**/*.ts" to TYPE_CHECKED_SOURCES 2 failedexpected [ './core/tsconfig.json', …(1) ] to be undefined and expected 2 to be undefined
3 Move the block after the "**/*.ts" block 2 failedconstructor-super: expected +0 to be 2 (this is the 18-rule regression the ordering prevents)
4 Revert the new Error(error.message, {cause: error}) wrap 1 failedexpected an Error, got object
5 Re-add the stray await getArtifactDir(...) npx eslint fails: 561:23 error Unexpected 'await' of a non-Promise (non-"Thenable") value @typescript-eslint/await-thenable

Getting that last assertion right took three attempts, which is worth recording. lintFiles alone passes with and without the block ("zero errors" is trivially true when no type-aware rule runs). Overriding file content via lintText reported the floating promise locally but returned no messages on all three CI runners, and writing a probe file into core/src produced a fatal parse error there — in both cases because the TypeScript program is built from disk at parse time, so the result depends on the environment. The shipped assertion touches no files and only enables an existing rule over existing source, so it behaves identically everywhere.

No eslint-disable, @ts-expect-error, @ts-ignore, any, as any, as never, as unknown as, or coverage suppression was added anywhere in this change (one eslint-disable is removed), and the diff contains no type assertion of any kind. The test constructs a real ErrorEvent via class TestErrorEvent extends Event implements ErrorEvent rather than casting, since Node has no ErrorEvent global; severityOf is typed with Linter.RuleEntry / Linter.RuleSeverity from eslint, because calculateConfigForFile is declared Promise<any> and narrowing an unknown with Array.isArray would launder the element type back through any.

Manual End-to-End (E2E) Tests:

npm install && npm run build
npm run lint                      # exits 0

# The gate bites. Add to any file under core/src:
#   async function scratchFlush(): Promise<void> {}
#   export function scratchShutdown() { scratchFlush(); }
npm run lint                      # @typescript-eslint/no-floating-promises
# The same code under core/test/ is NOT reported.

npx lint-staged                   # pre-commit path, with a core/src file staged

The scope boundary in the middle step is also asserted automatically, so it cannot silently rot.

Checklist

[x] I have read the CONTRIBUTING.md document.
[x] I have performed a self-review of my own code.
[x] I have commented my code, particularly in hard-to-understand areas.
[x] I have added tests that prove my fix is effective or that my feature works.
[x] New and existing unit tests pass locally with my changes.

Amaad Martin added 5 commits August 4, 2026 01:38
… await

Two defects surfaced by running ESLint with type information over core/src.

AsyncQueue.error() accepted `unknown` and rejected its iterator with it. The
only caller forwards the live API's ErrorEvent, which is not an Error, so
consumers lost the stack trace and `instanceof Error` was false. The parameter
is now `Error` and the caller wraps the event, keeping it as `cause`.

getCanonicalUri() awaited getArtifactDir(), which returns a plain string; the
other six call sites do not await it.
Adds a files-scoped type-aware ESLint block wired to the existing per-package
tsconfigs, so the type-checked rules stop being inert. Eleven rules that reach
zero findings are enabled, including no-floating-promises and
no-misused-promises; the rest are off with their measured counts.

The block sits ahead of the "**/*.ts" block because
recommendedTypeCheckedOnly bundles typescript-eslint's eslint-recommended,
which switches off 18 core rules (no-undef, no-const-assign, no-unreachable,
...) that js/recommended re-enables afterwards. An integration test pins that
ordering along with the scope boundary.

The no-undef directive in rest_api_tool.ts is dropped: type-aware parsing
resolves BodyInit as a type-only reference, so the directive is now unused.
lintText() with overridden content is not a reliable way to exercise a
type-aware rule: the TypeScript program reads source from disk, so whether the
override is type-checked depends on the environment. It reported the floating
promise locally and returned no messages on all three CI runners.

The probe is now a real file written under core/src (and core/test for the
negative case) and removed in a finally block, which exercises the same path a
contributor hits.
…nment

The previous assertions both depended on how the TypeScript program resolves
source that is not on disk at parse time. Overriding content with lintText()
reported the floating promise locally but returned nothing on all three CI
runners; writing a probe file into core/src produced a fatal parse error there.

Enabling a rule that requires a program over already-existing source has no
such dependency: it runs clean for core/src and throws 'requires type
information' for the test tree, which is the distinction being pinned.
Inline TYPE_CHECKED_SOURCES; it had one reader thirteen lines below its
declaration.

Reword the deferred-rules header so the list reads as a burn-down of
pre-existing findings rather than a standing exemption. No TODO id is written
because the repository has no public tracking issue for the work; four
follow-up tasks covering all twelve rules are queued instead.

Type severityOf with Linter.RuleEntry / Linter.RuleSeverity. calculateConfigForFile
is declared Promise<any>, so narrowing an unknown with Array.isArray laundered
the element type back through any and needed a cast to return a number. There
is now no type assertion anywhere in the diff.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant