Skip to content

Fix: route the last two core/src console.error calls through the shared logger - #826

Open
AmaadMartin wants to merge 2 commits into
mainfrom
fix/core-console-error-to-logger
Open

Fix: route the last two core/src console.error calls through the shared logger#826
AmaadMartin wants to merge 2 commits into
mainfrom
fix/core-console-error-to-logger

Conversation

@AmaadMartin

@AmaadMartin AmaadMartin commented Aug 8, 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: Two sites in core/src write their failure diagnostics with console.error: the summarizer catch in agent_controlled_context_compactor.ts and the runAsync catch in load_memory_tool.ts. A consumer who calls setLogger(), setLogger(null) or setLogLevel() cannot silence or redirect either line. The records also miss the ADK label, level and timestamp that every other ADK diagnostic carries.

Solution: Both sites now call logger.error from core/src/utils/logger.ts, so only the identifier changes. I used the logger facade rather than a module-level const logger = getLogger(), because the facade reads currentLogger at call time and so still honours a later setLogger(). The caught error stays a second argument, which keeps the Error object intact for a custom logger. Behaviour is otherwise unchanged: the compactor still swallows the error and clears its flags, and LoadMemoryTool still rethrows.

The ERROR in prefix in the load-memory message is dropped because the logger already renders the level, so keeping it produced ERROR: [ADK] <ts> ERROR in LoadMemoryTool runAsync: ....

Collision check: I listed the 400 open PRs on this fork and searched every non-main commit for edits to these two files and to the two message strings. Four PRs change core/src/utils/logger.ts (#807, #795, #697, #683) and two touch other parts of the compactor (refactor/compaction-utils, feat/apply-rewinds-live-event-filter). None converts these two call sites, so this branches from main.

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.

One new test per changed statement. Existing tests are untouched.

  • core/test/context/agent_controlled_context_compactor_test.ts — "logs the summarizer failure"
  • core/test/tools/load_memory_tool_test.ts — "logs the failure before rethrowing"

Both spy on getLogger().error, which the facade forwards to, and both assert on the same Error instance the code under test threw. The load-memory test builds a real Context over an InvocationContext whose memoryService rejects, so it drives the real Context.searchMemory path.

Proof each test can fail. I restored the original console.error line and re-ran the matching test:

  • compactor: AssertionError: expected "error" to be called with arguments: [ 'Compaction failed:', …(1) ] / Number of calls: 0
  • load memory: AssertionError: expected "error" to be called with arguments: [ …(2) ] / Number of calls: 0

Commands run on the pushed commit:

npx vitest run --project unit:core core/test/context/agent_controlled_context_compactor_test.ts core/test/tools/load_memory_tool_test.ts   # 13 passed
npx vitest run --project unit:core                                                                                                        # 201 files, 2733 passed
npm run build --workspace core                                                                                                            # ok
npm run lint                                                                                                                              # clean
npm run format:check                                                                                                                      # clean
npm run ts:check                                                                                                                          # pre-existing errors only; none in the four files I touched

Manual End-to-End (E2E) Tests:
Call setLogger(null) from @google/adk, then drive LoadMemoryTool.runAsync into its catch with a memory service that rejects. Before this change a bare line prints on the console. After it, nothing prints.

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 2 commits August 8, 2026 15:00
…logger

The compactor and LoadMemoryTool wrote their failure diagnostics with
console.error, so a consumer who calls setLogger() or setLogLevel() could
not silence or redirect them. Both sites now use the shared logger facade.

The 'ERROR in ' prefix is dropped because the logger already renders the
level: keeping it produced 'ERROR: [ADK] ... ERROR in LoadMemoryTool ...'.
The Logger API is variadic, so passing the error keeps the Error object
intact for a custom logger installed via setLogger(); a template literal
flattened it to a string before the logger saw it. This also matches the
dominant call shape in the repo (runner.ts, a2a_remote_agent.ts).

The load-memory test now builds a real Context over an InvocationContext
whose memoryService rejects, so it exercises Context.searchMemory and
drops the 'as unknown as Context' cast.
AmaadMartin pushed a commit that referenced this pull request Aug 12, 2026
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