Fix: pass a winston level name from SimpleLogger.log - #795
Open
AmaadMartin wants to merge 1 commit into
Open
Conversation
SimpleLogger.log handed winston level.toString(). LogLevel is a numeric enum, so winston received '0'..'3' while its levels map is keyed by debug/info/warn/error. The format chain calls colorize(), which looks up the colour for an unknown level, finds undefined and calls it. The write is synchronous, so the resulting TypeError propagates out of the public logger.log() call. Look the name up through an explicit LogLevel -> winston name table. The Record<LogLevel, string> annotation is exhaustive over the enum, so a new LogLevel member becomes a compile error at the table rather than a silent return of this bug.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Please ensure you have read the contribution guide before creating a pull request.
Link to Issue or Description of Change
None.
Problem:
SimpleLogger.log()passedlevel.toString()to winston.LogLevelis a numeric enum, so winston received'0'..'3', while itslevelsmap is keyed bydebug/info/warn/error. The format chain callscolorize(), which looks up the colour for the unknown level, findsundefined, and calls it. The Console write is synchronous, so the resultingTypeErrorpropagates out of the publiclogger.log()call and into user code.Solution:
log()now reads the winston level name from an explicitLogLevel-> name table. TheReadonly<Record<LogLevel, string>>annotation is exhaustive, so adding a fifthLogLevelmember is a compile error at the table (verified:TS2741). I preferred that overLogLevel[level].toLowerCase(), which couples winston's registered level names to the TypeScript member names. The level gate, thelevelsmap, the format chain and the four named methods are untouched.Scope: this PR fixes the
corelogger only.dev/src/utils/logger.tscarries the same one-line bug, and #790 already fixes it with a behaviourally identical change. I did not duplicate that half. The two files are disjoint, so this PR branches frommainrather than stacking on #790, and either can land first.Collision check: I ran
gh pr list --repo AmaadMartin/adk-js --state open --limit 400, then listed the files of all 400 open PRs and inspected every one touching a logger source. Six do. #790 and #672 aredev-only. #697 (stderr routing) and #431 (getLogLevel()) do not touch this line. #432 fixes it, but only inside a bundled logger refactor that is currently CONFLICTING. #683 splits the logger by platform and carrieslevel.toString()across tologger_node.tsunfixed; if it lands first, this same table applies there.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:
I appended a new top-level
describe('SimpleLogger.log')tocore/test/utils/logger_test.tsand changed no existing test. Five cases: anit.eachover the fourLogLevelmembers asserting the resolved winston level name, plus the level gate.The tests capture records at
winston.transports.Console.prototype.log. That is the only seam that works here: vitest replaces the globalconsole, whose_stdoutis notprocess.stdout, so spies onprocess.stdout.writeandconsole.logboth see nothing. The spy asserts oninfo[Symbol.for('level')], which is the name winston resolved, and it calls the transport callback so a second record is not stalled behind the first.Proof the tests can fail. I restored
this.logger.log(level.toString(), ...)and re-ran the file:The fifth case,
suppresses a record below the configured level, passes with and without the fix by design. It is the regression guard on the level gate this change must not disturb, so it is green either way rather than proof of the fix.Coverage. Both lines this PR adds or changes are covered, and branch coverage is 100%. The uncovered lines the report names (83-112, 165-178) are the pre-existing
debug/info/warn/errormethods and the deprecatedloggerproxy, which this one file does not drive.Manual End-to-End (E2E) Tests:
Build the workspace, then run this from the repo root so Node resolves
@google/adkthrough the workspace link:Before, the process exits 1:
After, it exits 0 and
log()renders the same line asinfo():Driving all four levels emits four correctly coloured lines with nothing on stderr.
Local validation on the pushed commit:
npx vitest run --project unit:core core/test/utils/logger_test.ts— 13 passed.npx vitest run --project unit:core— 2736 passed, 201 files, no regressions.npm run build— passed.npx eslinton both files — clean.npx prettier --checkon both files — clean.npm run docs:check— passed.bash scripts/check_license.sh— passed.npm run ts:checkfails, but it fails identically onmainwith this branch stashed. I diffed the two error lists and they are the same. The errors are in othercore/testfiles, and the validation workflow does not runts:check.Checklist