Skip to content

Fix: keep winston out of the published browser build by splitting the logger by platform - #683

Open
AmaadMartin wants to merge 6 commits into
mainfrom
fix/core-logger-platform-split-winston-node
Open

Fix: keep winston out of the published browser build by splitting the logger by platform#683
AmaadMartin wants to merge 6 commits into
mainfrom
fix/core-logger-platform-split-winston-node

Conversation

@AmaadMartin

@AmaadMartin AmaadMartin commented Aug 5, 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):
    Part of: Fix: remove orphaned fragment from BasePlugin.onToolErrorCallback JSDoc #611
    Related: Fix: keep temp: state readable for the whole invocation in DatabaseSessionService (stacked on #132) #607
  2. Or, if no issue exists, describe the change:
    Problem: core/src/utils/logger.ts imports winston, and the browser entry point reaches that module through common.ts. The published web build is transpile-only, so esbuild passes the specifier through unchanged. core/dist/web/utils/logger.js therefore ships import * as winston from "winston", which no browser bundler can resolve.

Solution: utils/logger.ts now writes through console, and the winston logger moves unchanged into the new utils/logger_node.ts. The Node entry point installs it through the existing setLogger seam, so Node output stays the same and winston stays a core dependency. #617 proposed deleting winston and was rejected, so this PR does not touch the dependency. An esbuild alias cannot solve this, because esbuild rejects alias without bundle and the published web build does not bundle, so the platform === 'browser' && bundle gate and the rest of core/build.js are left as they are.

load_artifacts_tool.ts and vertex_ai_search_tool.ts held const logger = getLogger() at module scope. installNodeLogger() runs after index.ts evaluates its re-exports, so those two modules would have kept the console logger for the process lifetime, out of reach of setLogLevel(). Both now log through the logger facade, which the other 39 importers already use. That is the only call-site change here.

Behaviour notes

  • resetLogger() now installs the console logger on Node as well. It is internal: neither index.ts nor common.ts exports it, and only core's own tests call it.
  • A deep import of utils/logger.js gets the console logger, because the Node wiring lives in index.ts. The package exports map only maps ".", so a package consumer cannot reach that path.
  • dist/web still contains utils/logger_node.js, and it still names winston. That module is unreachable: core/package.json declares exports for "." only, so a consumer cannot deep-import it, and browser points at index_web.js, whose graph does not include it. Excluding it from the transpile-only build would hide the day a *_node.ts module does become reachable, so the build is left alone.
  • logger.log(level, ...) on Node still throws inside winston, because it passes the numeric enum value as the level name. That behaviour is pre-existing and is preserved here; logger_node_test.ts pins it. The console logger emits normally.
  • The public API surface is unchanged. common.ts is untouched and no symbol is added or removed.

Overlapping work: I checked the open pull requests before starting. #614 makes the web target always bundle and moves Node-only exports out of common.ts; it does not touch core/src/utils/logger.ts, so this change is independent of it. The node:async_hooks half of #611 ships separately, which is why this is Part of and not Closes.

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.

npx vitest run --project unit:core core/test/utils/logger_test.ts core/test/utils/logger_node_test.ts core/test/utils/logger_node_wiring_test.ts core/test/tools/tool_logging_test.ts — 30 passed.

  • logger_test.ts gains the console SimpleLogger cases and a browser safety block. That block reads both sources: logger.ts must contain no import, and logger_node.ts must still import winston, so the first assertion cannot be met by deleting the dependency.
  • logger_node_test.ts (new) captures the Console transport output and checks the line format, the colorized level, level gating, and argument joining.
  • logger_node_wiring_test.ts (new) imports @google/adk and asserts the entry point installed the winston logger.
  • tool_logging_test.ts (new) drives LoadArtifactsTool and VertexAiSearchTool against real in-memory session and artifact services, with no mocks. It asserts that their log records reach the current logger and that setLogLevel(LogLevel.ERROR) suppresses the missing-artifact warning.

npx vitest run --project unit:core --project unit:dev — 2602 passed, 1 failed. The failure is dev/test/cli/cli_create_test.ts > should handle Vertex AI selection with gcloud defaults, which reads the local gcloud configuration. It fails the same way on main, so it is unrelated to this change.

npm run lint, npm run format:check and npm run docs:check are clean. npm run ts:check reports 281 errors on this branch and 281 on main, with an identical per-file breakdown.

Coverage: core/src/utils/logger_node.ts is at 96.96% lines and 93.33% branches. The only uncovered lines are the early return in error(): LogLevel.ERROR is the highest level, so this.logLevel > LogLevel.ERROR can never be true. That guard is pre-existing code moved without modification, so I kept it rather than deleting it for the number. The new code in core/src/utils/logger.ts is fully covered.

Mutation proof: each new test was run against the unfixed code.

Mutation Result
Restore the winston version of core/src/utils/logger.ts logger_test.ts: 8 failures, including browser safety > keeps the browser-reachable logger free of imports
Comment out installNodeLogger() in core/src/index.ts logger_node_wiring_test.ts fails — expected SimpleLogger{ logLevel: 1 } to be an instance of WinstonLogger
Restore const logger = getLogger() in both tools all 3 tool_logging_test.ts cases fail, e.g. expected "warn" to not be called at all, but actually been called 1 times

Manual End-to-End (E2E) Tests:
Please provide instructions on how to manually test your changes, including any necessary setup or configuration.

npm ci && npm run build

The check that matters is what the browser entry point reaches. Bundling it lists every bare specifier a browser bundler would have to resolve:

npx esbuild ./src/index_web.ts --bundle --platform=browser --format=esm \
  --packages=external --outfile=/dev/null --metafile=/tmp/meta.json

On main that list contains winston:

@google-cloud/vertexai, @google/genai, @opentelemetry/api, adm-zip,
google-auth-library, js-yaml, jsonpath-plus, lodash-es, node:async_hooks,
node:dns/promises, node:fs/promises, node:net, node:path, winston, zod,
zod-to-json-schema, zod/v4

On this branch it does not, and no module named logger_node is among the 147 reachable inputs. The remaining node: specifiers are the other half of #611 and #612, and are out of scope here.

$ grep -rln "winston" core/dist/esm core/dist/cjs
core/dist/esm/utils/logger_node.js
core/dist/cjs/utils/logger_node.js

$ grep -rn "from 'winston'" core/src
core/src/utils/logger_node.ts:17:import * as winston from 'winston';

$ npx esbuild core/src/agents/llm_agent.ts --bundle --platform=browser \
    --outfile=/dev/null 2>&1 | grep 'Could not resolve'
✘ [ERROR] Could not resolve "node:async_hooks"

Node output is unchanged. The script below prints the same four lines, with the same colours and the same format, on main and on this branch:

node --input-type=module -e "
  const {setLogLevel, LogLevel, getLogger} = await import('./core/dist/esm/index.js');
  setLogLevel(LogLevel.DEBUG);
  getLogger().debug('d'); getLogger().info('i');
  getLogger().warn('w');  getLogger().error('e');
"
DEBUG: [ADK] <timestamp> d
INFO: [ADK] <timestamp> i
WARN: [ADK] <timestamp> w
ERROR: [ADK] <timestamp> e

dist/esm, dist/cjs and dist/web still mirror core/src file for file, as before.

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 6 commits August 5, 2026 09:14
The browser entry point reaches utils/logger.ts, which imported winston, so
no browser bundler could resolve the published web build. utils/logger.ts now
writes through console, and the winston logger moves to utils/logger_node.ts,
which the Node entry point installs. Node output is unchanged.

Part of #611
dist/web is transpile-only, so esbuild passes every import specifier through
verbatim. Emitting index.ts and the *_node.ts modules would ship winston in
the published browser artifact even though index_web.ts never imports them.

Part of #611
…ools

installNodeLogger() runs after index.ts evaluates its re-export graph, so a
module that captured getLogger() at module scope kept the console logger for
the process lifetime. setLogLevel() never reached those two tools and their
Node output bypassed winston. The logger facade already forwards to the
current logger, which is what the other 39 importers use.

Part of #611
…pin the browser build's entry-point exclusion to src/index.ts

Part of #611
…uild"

The excluded modules were already inert: core/package.json maps exports for
"." only, so a consumer cannot deep-import dist/web, and the browser field
points at index_web.js, whose graph reaches neither index.ts nor logger_node.ts.
Dropping entry points from a transpile-only build would also hide a future
dangling import instead of failing the build.

Part of #611
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