Skip to content

Fix: log a warning when AgentLoader skips an unloadable agent file - #365

Open
AmaadMartin wants to merge 1 commit into
mainfrom
fix/log-skipped-agent-files
Open

Fix: log a warning when AgentLoader skips an unloadable agent file#365
AmaadMartin wants to merge 1 commit into
mainfrom
fix/log-skipped-agent-files

Conversation

@AmaadMartin

@AmaadMartin AmaadMartin commented Jul 31, 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):
    Closes: #issue_number
    Related: #issue_number
  2. Or, if no issue exists, describe the change:

Problem: AgentLoader swallows AgentFileLoadingError in two places with no diagnostic output at all — loadAgentFromFile and loadAgentFromDirectory (dev/src/utils/agent_loader.ts) both return; on it. A developer drops an agent file into their agents directory, runs adk web / adk api_server, and the agent simply is not in the list. Nothing is printed at any log level, listAgents() omits it, /apps/<name>/... 404s, and the A2A route is never mounted — all far away from the real cause.

Precisely two throw sites produce AgentFileLoadingError, so these are the only failures currently silenced:

  1. Agent file <path> does not exists — the fsPromises.stat ENOENT path.
  2. Failed to load agent <path>: No @google/adk BaseAgent class instance found. ... — the common one: an empty file, a file that forgot to export its agent, or a rootAgent export that is not a BaseAgent/App.

Everything else is already loud and stays that way: an esbuild BuildFailure (syntax error, unresolvable import) and any error thrown while evaluating the module are not AgentFileLoadingError, so they still hit throw e and reject preloadAgents(). This PR does not change that.

Solution: Keep the swallow and add one logger.warn before each return. The swallow itself is deliberate and correct — preloadAgents() walks every .js/.cjs/.mjs/.ts/.mts/.cts file at the top level of the agents directory, so a plain helper module living next to the agents must not abort discovery. The defect was that the swallow was silent, making a genuinely broken agent and an intentionally-not-an-agent file indistinguishable from the outside.

Two considered alternatives were rejected: converting the swallow to a throw (breaks the mixed-directory use case), and adding an option to choose between the two (a flag for a log line). Log-and-keep-going is what adk-python already does in src/google/adk/cli/utils/agent_loader.py ("Failed to load agent '%s': %s" then continue); this is the TypeScript equivalent of that line, not a field-for-field parity port.

Two details worth calling out:

  • The message prefixes the source path deliberately. AgentFile.load() reassigns its local filePath to the esbuild output before throwing, so e.message alone only names a temp artifact like /tmp/adk_agent_loader/<uuid>/broken_agent.cjs. Prefixing with file.path / possibleEntryFile.path is what makes the warning actionable, which is why the message has both parts. A test mutation below pins exactly this.
  • Trade-off: a mixed directory now prints one warning per non-agent JS/TS file. That is the intended cost of making a broken agent discoverable. It is bounded — preloadAgents() short-circuits on agentsAlreadyPreloaded, so the warnings are emitted once per process, re-emitted only if invalidateAll() fires under --reload_agents.

Deliberately out of scope: the empty catch {} in listApps(), the silent early return for a directory with no agent.*/app.* entrypoint (warning there would fire for every unrelated subdirectory, including node_modules), and the two existing console.warn calls in AgentFile. No new types, exports, options, helpers, or dependencies; AgentFileLoadingError stays unexported and no private member was widened.

Collision check (required before implementation): scanned all 269 open PRs on the fork. Five touch dev/src/utils/agent_loader.ts#309, #285, #328, #275, #264 — but every one of their hunks lands in the AgentFile class / compile-and-bundle path or in isJsFile, and none touches the AgentFileLoadingError catch blocks in loadAgentFromFile / loadAgentFromDirectory. No PR implements this change, so this branches from main rather than stacking.

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.

Two new tests in dev/test/utils/agent_loader_test.ts, one per new line. The pre-existing handles AgentFileLoadingError in directory loading test was left untouched — it still pins the swallow (that bad_agent_dir stays out of listAgents()), and no existing test was modified, deleted, skipped, or weakened.

npx vitest run --project unit:dev dev/test/utils/agent_loader_test.ts
  Test Files  1 passed (1)
       Tests  32 passed (32)

AgentLoader's logger is a module-private instance, so the tests spy on AdkLogger.prototype.warn and mockRestore() at the end. They deliberately do not assert call counts — that prototype is shared (AgentFile.load() also warns about the require cache, and the agent1/agent2/agent3 fixtures load in the same run), so toHaveBeenCalledTimes would be flaky. Each test instead selects the one warning whose text contains its own source path.

Coverage of new code: both new statements are executed and asserted — v8 reports 1 hit on the loadAgentFromFile warn and 2 on the loadAgentFromDirectory warn (the new test plus the pre-existing one). 100% line coverage on new code; no new branches were introduced, since both lines sit inside an if (e instanceof AgentFileLoadingError) block that was already covered.

Proof the tests can fail — coverage is a floor, so each test was run against mutated source. Three mutations:

  1. Delete both logger.warn lines. Both new tests fail:
    FAIL  dev/test/utils/agent_loader_test.ts > warns when a standalone agent file exports no BaseAgent
    AssertionError: expected undefined to be defined
     ❯ dev/test/utils/agent_loader_test.ts:767:27
    FAIL  dev/test/utils/agent_loader_test.ts > warns when a directory entrypoint exports no BaseAgent
    AssertionError: expected undefined to be defined
     ❯ dev/test/utils/agent_loader_test.ts:792:27
    
  2. Delete only the loadAgentFromFile line. Exactly one test fails (standalone agent file ×, directory entrypoint ✓), and vice-versa when only the loadAgentFromDirectory line is deleted — so neither test covers the other's line.
  3. Replace both messages with a bare logger.warn(e.message) (dropping the source-path prefix). Both tests fail, confirming they pin the source path and not merely the error text — the property the fix exists for.

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

Run with no mocks against the built dev output:

mkdir -p /tmp/adk_broken_agents /tmp/adk_broken_agents/broken_dir
printf 'export const notAnAgent = 42;\n'  > /tmp/adk_broken_agents/broken_agent.ts
printf 'export const foo = "bar";\n'      > /tmp/adk_broken_agents/broken_dir/agent.ts
cat > /tmp/adk_broken_agents/good_agent.ts <<'EOF'
import {LlmAgent} from '@google/adk';
export const rootAgent = new LlmAgent({name: 'good_agent', model: 'gemini-2.0-flash'});
EOF

npm run build
npx adk web /tmp/adk_broken_agents --port 8978
curl -s http://localhost:8978/list-apps

Observed — both broken files are named with their source paths, the server comes up normally, and the working agent is still served:

INFO: [ADK API Server] GET /list-apps
[AgentLoader] Skipping /tmp/adk_broken_agents/broken_agent.ts: Failed to load agent /tmp/adk_agent_loader/<uuid>/broken_agent.cjs: No @google/adk BaseAgent class instance found. Please check that file is not empty and it has export of @google/adk BaseAgent class (e.g. LlmAgent) instance.
[AgentLoader] Skipping /tmp/adk_broken_agents/broken_dir/agent.ts: Failed to load agent /tmp/adk_agent_loader/<uuid>/agent.cjs: No @google/adk BaseAgent class instance found. ...

["good_agent"]

Note the warnings appear on the first request that triggers discovery, not at process boot — preloadAgents() is lazy. No flag is needed for them to be visible: AdkLogger defaults to LogLevel.INFO, so warn passes its level check.

Other checks on the pushed commit:

npx eslint dev/src/utils/agent_loader.ts dev/test/utils/agent_loader_test.ts   # clean
npx prettier --check <same two files>                                          # clean
npm run build                                                                  # clean

npm run ts:check reports errors, but all of them are pre-existing on main (verified by stashing this change and re-running) and live in core/test/** and tests/**. Neither file in this diff produces a type error.

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.

CI note (pre-existing Windows/macOS flake, not this change)

All checks are green: run-tests on ubuntu-latest, macos-latest and windows-latest, plus check-license. The Windows leg failed on its first two attempts, and since a flaky-looking failure is exactly how a real regression hides, here is the evidence that it is not one:

Attempt (same commit) Windows result Failing test
1 fail tests/integration/adk_web/webui_test.ts (CLI exited prematurely) + tests/integration/app_loader/app_loader_test.ts (timed out in 40000ms)
2 fail core/test/code_executors/unsafe_local_code_executor_test.ts (timed out in 5000ms) — the two above passed
3 pass
  • The failures are not stable across runs of an unchanged commit, and attempt 2 failed in core/test/code_executors/, which this diff (two files, both under dev/) cannot reach.
  • Every failure is a timeout / premature exit, never an assertion.
  • This change is a strict no-op for both originally-failing suites: it only logs when an AgentFileLoadingError is swallowed, and neither fixture can trigger one — tests/integration/app_loader/discovery/ holds only package.json, standalone_agent.ts, standalone_app.ts, service_alpha/app.ts, service_beta/agent.ts, and tests/integration/adk_web/agent/ holds only agent.ts. All load successfully, so zero new warnings are emitted in either suite.
  • The same app_loader_test.ts 40s timeout fails on Windows on a sibling PR that does not touch agent_loader at all, and a second sibling fails the same way on macOS.
  • Both suites pass locally on this commit: app_loader_test.ts 6/6 (the discovery test in 18.9s, well inside its 40s budget) and webui_test.ts 2/2.

AgentLoader swallowed AgentFileLoadingError in loadAgentFromFile and
loadAgentFromDirectory with no output, so a broken agent file and a
plain non-agent module in the agents directory were indistinguishable:
the agent just never appeared in listAgents() and every request for it
404'd, with nothing in the logs pointing back at the file.

Keep the swallow (a non-agent module may legitimately sit in the agents
directory) but emit one logger.warn naming the source path and the
error. The message prefixes file.path because AgentFile.load()
reassigns its local filePath to the esbuild output before throwing, so
e.message alone only names a temp artifact.
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