Skip to content

Fix: route AgentFile.load multiple-apps/agents warnings through AdkLogger - #457

Open
AmaadMartin wants to merge 1 commit into
mainfrom
fix/agent-loader-logger-warn
Open

Fix: route AgentFile.load multiple-apps/agents warnings through AdkLogger#457
AmaadMartin wants to merge 1 commit into
mainfrom
fix/agent-loader-logger-warn

Conversation

@AmaadMartin

@AmaadMartin AmaadMartin commented Aug 1, 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 — no existing issue.

  2. Or, if no issue exists, describe the change:
    Problem: dev/src/utils/agent_loader.ts owns a module-level ADK CLI logger (const logger = new AdkLogger({label: 'AgentLoader', colorize: {all: true}}), line 29), and every diagnostic in the module goes through it — logger.warn at line 223, logger.info at line 410, logger.warn at lines 418/420 — except the two warnings emitted inside AgentFile.load(), which called console.warn directly. Three concrete consequences:

  3. No AgentLoader label. AdkLogger applies winston.format.label({label: 'AgentLoader'}) (dev/src/utils/logger.ts:41-44); these two lines printed bare, so a user could not tell which CLI subsystem produced them.

  4. No colorization. The logger is built with colorize: {all: true} (dev/src/utils/logger.ts:51-53), so every sibling AgentLoader message is colorized and these two were not.

  5. Not silenceable — the real defect. AdkLogger.warn() early-returns when this.logLevel > LogLevel.WARN (dev/src/utils/logger.ts:104-110) and AdkLogger.setLogLevel() is how the CLI raises that threshold. console.warn ignores the gate, so these two messages could not be suppressed by any log-level setting while every neighbouring diagnostic could.

Solution: Route both call sites through the existing module-level logger (2 changed lines). No new logger instance, no constructor injection, no new import — logger was already in scope.

  • The message text is unchanged, character for character. Only the destination and the label/color decoration change.
  • Routing note for reviewers: these two lines now travel through winston's Console transport via AdkLogger rather than through console.warn directly. That is the point of the change — it makes them match the file's other diagnostics — and no test asserts which stream (stdout vs stderr) receives them.
  • Deliberately not in scope: the other console.* calls elsewhere in dev/src (there is no no-console lint rule, so this is not a repo-wide sweep), the AgentLoader.loadAgentFrom{File,Directory} catch blocks, and dev/src/utils/logger.ts itself (no transport/level/format changes).

Why one existing assertion changed. The repo guideline is add a new test, do not rewrite an existing one. The single exception applies here: 'loads first agent if multiple agents exported' spied on console.warn, i.e. it pinned the exact bypass this PR removes, so it could not survive unmodified. The edit is confined to the spy target (vi.spyOn(console, 'warn') -> vi.spyOn(AdkLogger.prototype, 'warn')) and its two references; the fixture, the esbuild mock and the expect(agent.name).toEqual('agent1') assertion are untouched. No test was deleted, skipped or weakened.

Collision check (open PRs on this fork). gh pr list --repo AmaadMartin/adk-js --state open --limit 100 was reviewed before writing any code. Three PRs are plausibly adjacent:

PR Overlap Verdict
#365 fix/log-skipped-agent-files same two files Overlaps, does not collide. It adds logger.warn to the AgentFileLoadingError catch blocks (lines ~515/540) and appends tests at the end of the file; this PR touches lines 254/281 and the AgentFile describe block. The only shared line is the identical import {AdkLogger} from '../../src/utils/logger.js'; addition. Verified empirically rather than assumed: a local git merge of the two branches auto-merged with no conflict, and the merged tree runs green (33 tests). Branched from main on that basis instead of stacking.
#431 feat/logger-get-log-level core/src/utils/logger.ts Adds an accessor to the core logger; no overlap with agent_loader.ts.
#432 feat/shared-level-gated-winston-logger dev/src/utils/logger.ts Refactors the logger implementation behind the same AdkLogger.warn surface this PR calls; no overlap with agent_loader.ts.

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:dev dev/test/utils/agent_loader_test.ts
# Test Files  1 passed (1)
#      Tests  31 passed (31)

One existing test was updated, deliberately — please read this before reviewing the test diff. 'loads first agent if multiple agents exported' asserted on the pre-change console.warn sink (vi.spyOn(console, 'warn')), i.e. it pinned the exact bypass this PR removes, so it cannot pass unmodified and cannot be left alone. Only the spy target and its two references changed (vi.spyOn(console, 'warn') -> vi.spyOn(AdkLogger.prototype, 'warn')); the fixture, the esbuild mock and the expect(agent.name).toEqual('agent1') assertion are byte-identical. No test was deleted, skipped, .only'd or thinned, and no other existing test was touched. This is flagged in the commit message as well as here.

  • Updated (see the note above) 'loads first agent if multiple agents exported' — covers the rootAgents.length > 1 branch (line 281). Updated because its assertion targeted console.warn, the sink this PR replaces.
  • New 'loads first app if multiple apps exported' — covers the rootApps.length > 1 branch (line 254), which had no test at all before this PR. It asserts the exact rendered message, plus isApp(loaded), the selected app name and its root agent.

Coverage of the new code is 100% (2/2 statements, 2/2 branches), read out of the v8 report rather than eyeballed:

line 254 statement hits: 1        branch at line 253 counts: 1
line 281 statement hits: 1        branch at line 280 counts: 1

(The whole-file number for agent_loader.ts under this one test file is 84.04% lines / 84% branches; that is pre-existing and unrelated to these two lines.)

Proof the new tests can fail (mutation testing). Each changed line was reverted to console.warn in turn and the corresponding test was re-run:

  1. Revert line 281 to console.warn ->
    FAIL dev/test/utils/agent_loader_test.ts > AgentLoader > AgentFile > loads first agent if multiple agents exported
    AssertionError: expected "warn" to be called with arguments: [ StringContaining{…} ] Number of calls: 0
  2. Revert line 254 to console.warn ->
    FAIL dev/test/utils/agent_loader_test.ts > AgentLoader > AgentFile > loads first app if multiple apps exported
    AssertionError: expected "warn" to be called with arguments: [ Array(1) ] Number of calls: 0

Both were then restored and the file re-run green (31/31).

Repo gates on the pushed commit:

npm run build                                                                     # ok
npx eslint dev/src/utils/agent_loader.ts dev/test/utils/agent_loader_test.ts      # clean, exit 0
npx prettier --check dev/src/utils/agent_loader.ts dev/test/utils/agent_loader_test.ts
#   All matched files use Prettier code style!
npm run ts:check                                                                  # 0 errors in either changed file

npm run ts:check is red repo-wide on main today (pre-existing errors across core/test/** and tests/integration/**, the subject of several separate open PRs). Filtering its output for the two files this PR touches returns zero errors.

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

  1. npm install && npm run build
  2. Create a scratch directory with a module exporting two LlmAgent instances under names other than rootAgent, and with no default export:
    export const agent1 = new LlmAgent({
      name: 'agent1',
      model: 'gemini-2.0-flash',
    });
    export const agent2 = new LlmAgent({
      name: 'agent2',
      model: 'gemini-2.0-flash',
    });
  3. Run the dev CLI against it (adk web <dir> or adk run <dir>).
  4. Confirm Multiple agents found in <path>. Using the agent1 as a root agent. now renders with the same AgentLoader label and coloring as the loader's other messages (e.g. Detected change in <file>, reloading agents...), and that the wording is unchanged.
  5. Repeat with a module exporting two App instances (names other than app / rootApp, no default export) and confirm Multiple apps found in ... behaves identically.
  6. Raise the log level past WARN and confirm both lines are now suppressed along with the loader's other warnings — before this change they printed regardless.

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.

…AdkLogger

The two "Multiple apps/agents found" diagnostics in AgentFile.load() called
console.warn directly while every other diagnostic in agent_loader.ts goes
through the module-level AdkLogger. As a result they printed without the
AgentLoader label and without colorization, and - the actual defect - they
ignored AdkLogger.setLogLevel(), so they could not be silenced when every
neighbouring warning could.

Route both through the existing module logger. The message text is unchanged
character for character.

Tests: the multiple-agents test now spies on AdkLogger.prototype.warn instead
of console.warn (the assertion it replaced pinned the bypass being removed),
and the previously untested multiple-apps branch gains a sibling test.
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