Skip to content

Fix: type-check core/test/agents/processors against core/src (31 tsc errors) - #739

Open
AmaadMartin wants to merge 2 commits into
mainfrom
fix/ts-check-core-test-agents-processors
Open

Fix: type-check core/test/agents/processors against core/src (31 tsc errors)#739
AmaadMartin wants to merge 2 commits into
mainfrom
fix/ts-check-core-test-agents-processors

Conversation

@AmaadMartin

@AmaadMartin AmaadMartin commented Aug 6, 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: npm run ts:check reports 31 errors in core/test/agents/processors/. Each of six files imports the processor under test from core/src but its fixtures from '@google/adk', which tsc resolves to the built declarations in core/dist/types. Classes that carry a private member or a brand symbol therefore exist twice, and TypeScript compares those by declaration identity, so the two copies are not assignable to each other. A representative error:
core/test/agents/processors/code_execution_request_processor_test.ts(81,51): error TS2345:
  Argument of type '.../core/dist/types/agents/invocation_context'.InvocationContext
  is not assignable to parameter of type '.../core/src/agents/invocation_context'.InvocationContext.

Solution: The six files now resolve every ADK symbol from core/src, which is the single copy vitest already loads through its '@google/adk' -> ./core/src alias. The package specifier cannot stay: core/package.json exports only ".", and the processors under test are in neither core/src/common.ts nor core/src/index.ts, so no package import can reach them. Two remaining errors come from other causes: two local output-schema fixtures used 'object' as const instead of the Type enum, and the confirmation test passed a second argument that RequestConfirmationLlmRequestProcessor.runAsync does not declare and never reads, so the test now passes only the invocation context. No production file, test title or assertion changes.

Overlap disclosure. Four open fork PRs contain these same hunks inside much larger rewrites: #487, #514, #628 and #294. None is merged and main still shows all 31 errors, so this narrow directory-scoped slice is not a duplicate. If #514 or #487 lands first, close this one. PR #675 renames code_execution_request_processor.ts; it will conflict textually with the import line in that one test file.

I checked all 630 open PRs on the fork for a collision. No open PR is scoped to this directory.

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.

No test is added: the change adds no production code, and the existing suite is the regression gate. Measured on this branch at 81c14222 after npm install && npm run build:

Check Before After
tsc --noEmit errors in core/test/agents/processors/ 31 0
tsc --noEmit errors repo-wide 280 249
vitest run --project unit:core core/test/agents/processors 11 files, 108 tests pass 11 files, 108 tests pass
eslint "core/test/agents/processors/*.ts" exit 0 exit 0
prettier --check "core/test/agents/processors/*.ts" clean clean

The repo-wide count drops by exactly 31, which shows no other file moved.

Proof that the gate can fail. I reverted the change and re-ran the check:

$ git stash;     npx tsc --noEmit --pretty false 2>/dev/null | grep -c core/test/agents/processors
31
$ git stash pop; npx tsc --noEmit --pretty false 2>/dev/null | grep -c core/test/agents/processors
0

Manual End-to-End (E2E) Tests:
Not applicable. The change is type-level and test-only. To reproduce the four checks above:

npm install
npm run build
npx tsc --noEmit --pretty false 2>&1 | grep core/test/agents/processors
npx vitest run --project unit:core core/test/agents/processors
npx eslint "core/test/agents/processors/*.ts"
npx prettier --check "core/test/agents/processors/*.ts"

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.

CI

GitHub Actions reports no checks on this fork, so I validated locally on the pushed commit d127938a:

npm run build                                                    # exit 0
npm run lint                                                     # exit 0
npx tsc --noEmit --pretty false | grep -c core/test/agents/processors   # 0
npx vitest run --project unit:core core/test/agents/processors   # 11 files, 108 tests pass
npx prettier --check "core/test/agents/processors/*.ts"          # clean

Amaad Martin added 2 commits August 6, 2026 15:20
tsc --noEmit reports 31 errors in this directory. The tests import the
processor under test from core/src but their fixtures from '@google/adk',
which tsc resolves to core/dist/types. Classes with private members or
brand symbols then exist twice and are not assignable to each other.

Resolve every ADK symbol from core/src, which is what vitest already loads
through its '@google/adk' -> ./core/src alias. Two output-schema fixtures
now use the Type enum instead of 'object' as const, and the confirmation
processor is held through BaseLlmRequestProcessor so the two-parameter base
contract applies. No test title or assertion changes.
RequestConfirmationLlmRequestProcessor.runAsync takes only the invocation
context and never reads a request, so the second argument was built and
discarded. Dropping it removes the TS2554 error at its root and makes the
BaseLlmRequestProcessor binding, the makeLlmRequest helper and two imports
unnecessary.
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