Skip to content

Fix: route AdkLogger warn and error records to stderr - #672

Open
AmaadMartin wants to merge 2 commits into
mainfrom
fix/dev-logger-stderr-routing
Open

Fix: route AdkLogger warn and error records to stderr#672
AmaadMartin wants to merge 2 commits into
mainfrom
fix/dev-logger-stderr-routing

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):

N/A

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

Problem: AdkLogger builds its winston Console transport with no stderrLevels option, so winston writes every record to stdout, including error. A CI harness, a process manager or docker logs reads stderr to learn why a process failed, and gets an empty stream. This affects adk web, adk api_server, adk run, adk deploy, AdkApiServer and AgentLoader.

Solution: Pass stderrLevels: ['warn', 'error'] to the transport. The level names stay lowercase because winston matches them against the raw level name on info[LEVEL], not against the uppercased display level that the format chain produces. Message content, layout, colours and the logLevel threshold are unchanged; only the file descriptor changes.

dev/src/cli/cli.ts is out of scope. Its two process.exit(1) flush sites are already handled by open PR #591, and #595 also edits that file.

Collision check: I listed all 571 open PRs on the fork and read the file list of every adjacent one. #432 refactors dev/src/utils/logger.ts and adds dev/test/utils/logger_test.ts, but keeps a bare new winston.transports.Console(), so the bug survives it. git log --all -S'stderrLevels' returns nothing. I branched from main rather than stacking on #432, because #432 is a large refactor that is not a prerequisite; if it lands first, the same option moves to wherever the transport ends up.

Testing Plan

Unit Tests:

  • I have added or updated unit tests for my change.
  • All unit tests pass locally.

New file dev/test/utils/logger_test.ts (8 cases). Winston writes to console._stdout / console._stderr rather than calling console.log, so the test installs its own node:console Console over two capture streams and reads the bytes back.

npx vitest run --project unit:dev dev/test/utils/logger_test.ts   # 8 passed
npx vitest run --project unit:dev                                 # 230 passed, 1 pre-existing failure
npm run build && npx eslint "dev/**/*.ts" && npx prettier --check # clean

The one failure in unit:dev is cli_create_test.ts > should handle Vertex AI selection with gcloud defaults. It reads the local gcloud config and fails the same way on a clean tree with my commit stashed.

Proof the tests can fail. I ran the suite against three mutations of dev/src/utils/logger.ts:

Mutation Result
Drop the option: new winston.transports.Console() 5 failed: expected '' to contain 'boom'
Uppercase the names: ['WARN', 'ERROR'] 5 failed: expected '' to contain 'boom'
Widen to ['debug', 'info', 'warn', 'error'] 2 failed: expected '' to contain 'hello'

Coverage. dev/src/utils/logger.ts had no direct test before and now measures 90.66% lines / 94.11% branches. The line I added is covered. Two gaps remain and I left them deliberately:

  • log() (lines 85-90) passes a numeric level to winston, which is a separate defect. A test would pin the bug rather than the fix.
  • The early return in error() needs logLevel > LogLevel.ERROR. ERROR is the highest enum member, so no valid input reaches it.

Manual End-to-End (E2E) Tests:

node -e "require('node:net').createServer().listen(8811,'localhost')" &
node dev/dist/esm/cli_entrypoint.js api_server ./dev/samples --port 8811 1>/dev/null
node dev/dist/esm/cli_entrypoint.js api_server ./dev/samples --port 8811 2>/dev/null

Before: the first command printed nothing and the second printed [ADK CLI] Error starting API server: Port 8811 is already in use. After: they are the other way round.

The start-up banner still goes to stdout, which the integration harness greps for:

node dev/dist/esm/cli_entrypoint.js api_server ./dev/samples --port 8123 2>/dev/null
# +---------------------------+
# | ADK API Server started    |

Checklist

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

Amaad Martin added 2 commits August 5, 2026 02:52
The dev Console transport was built with no stderrLevels option, so winston
routed every record, including error, to stdout. A supervisor that reads
stderr saw nothing when adk web, api_server, run or deploy failed.

Configure the transport with the lowercase levels warn and error. Winston
matches these against the raw level name on info[LEVEL], not the uppercased
display level. Adds dev/test/utils/logger_test.ts, the first direct test for
this logger.
Inline the two-element level list at the transport call site, where the
lowercase caveat reads better than a detached doc comment.

Replace the hand-rolled ANSI stripper with stripVTControlCharacters from
node:util, which also removes the dynamic regex built to dodge no-control-regex.
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