Skip to content

Fix: adopt AgentFile.loadAgent() and drop the unused loadApp() - #832

Open
AmaadMartin wants to merge 4 commits into
mainfrom
fix/adopt-load-agent-drop-load-app
Open

Fix: adopt AgentFile.loadAgent() and drop the unused loadApp()#832
AmaadMartin wants to merge 4 commits into
mainfrom
fix/adopt-load-agent-drop-load-app

Conversation

@AmaadMartin

@AmaadMartin AmaadMartin commented Aug 9, 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
  2. Or, if no issue exists, describe the change:
    Problem: AgentFile in dev/src/utils/agent_loader.ts exposes loadAgent() and loadApp(), and neither has a production caller. Four production sites spell loadAgent() out by hand as isApp(loaded) ? loaded.rootAgent : loaded. loadApp() cannot be adopted at all: for a bare agent file it synthesizes App({name: agent.name}), and Runner reads input.app?.name ?? input.appName (core/src/runner/runner.ts:155), so that name would replace the loader key used for session and artifact scoping. validateAppName (/^[a-zA-Z][a-zA-Z0-9_-]*$/) is also stricter than validateAgentName (/^[\p{ID_Start}$_][\p{ID_Continue}$_-]*$/u), so an agent named _internal loads through load() and then makes loadApp() throw.

Solution: I deleted loadApp() and adopted loadAgent() at the three sites that only need the root agent: the event-graph route, the adk run reload watcher, and the A2A bootstrap. That bootstrap also passed adkApp ?? agent to getRunner, which is always the loaded value, so it passes load() directly now. adk-python splits the same union the same way: the loader has load_agent() and no load_app(), narrowing lives on the consumer as _get_root_agent (cli/api_server.py:832), and App synthesis takes the caller's app key (_to_app(agent_or_app, app_name), cli/cli.py:60). One site keeps the isApp ternary, because cli_run.ts needs the root agent and the App | undefined together.

Collision check: I listed all 300 open PRs on the fork and read the diffs of the 13 that touch agent_loader.ts, adk_api_server.ts or cli_run.ts. None adds or removes loadApp/AgentFile.loadAgent. #731 removes the separate AgentLoader.listApps()/getAppFile() pair and rewrites the same integration test line, so I edited that test in place rather than deleting the block.

Type-checker note: this branch adds three as unknown as lines, all in test fakes. Each replaces an identical cast that the same file already had: git show main:<file> | grep -c 'as unknown as' and the same count on this branch are equal for both test files. A structural fake cannot satisfy AgentFile or AgentLoader because both declare private fields. No any, no @ts-expect-error, no eslint-disable, no coverage suppression.

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/server/adk_api_server_test.ts dev/test/cli/cli_run_test.ts dev/test/utils/agent_loader_test.ts — 94 passed.

Every new test was run against the unfixed code and fails there:

Mutation Failing test Message
loadAgent() returns the raw union loads the rootAgent of an App file via loadAgent() expected true to be false
load() rebuilds the App under a different name loads the rootAgent of an App file via loadAgent() expected 'renamed' to be 'test_app_default'
Graph route back to load() + ternary should return graph for an App-exporting agent file expected "spy" to be called at least once
A2A bootstrap back to load() + ternary should build the Runner from the App of an App-exporting agent file expected "spy" to be called at least once
getRunner receives the root agent, not the union should build the Runner from the App of an App-exporting agent file expected +0 to be 1
Reload watcher back to load() + ternary should rebuild the runner from the reloaded agent file expected "spy" to be called at least once
Reload failure swallowed without a warning should keep the current runner when the reloaded file fails to load expected "warn" to be called with arguments

Three existing tests were retargeted in place rather than added alongside, because each one called loadApp(), which this PR deletes. They are 'loads an app via loadApp() and rootAgent via loadAgent()' and 'synthesizes an App when loadApp() is called on a BaseAgent file' in dev/test/utils/agent_loader_test.ts, and 'should synthesize App when loadApp() is called on BaseAgent file' in tests/integration/app_loader/app_loader_test.ts. The first keeps its test_app_default App-identity assertion, which the second mutation above pins. No other test was changed, and none was deleted.

Gates on the pushed commit: npm run build, npm run lint and npm run ts:check all pass. ts:check reports 287 pre-existing errors in unrelated test files, and none in any file this branch touches.

CI is green on all three runners for the head commit. An earlier commit needed two re-runs, both for infrastructure flakes unrelated to this branch: the dev build hit ECONNRESET while it downloaded the ADK Web zip from GitHub, and core/test/code_executors/unsafe_local_code_executor_test.ts timed out at 5000 ms on the Windows runner.

tests/integration/app_loader/app_loader_test.ts could not run here. Its beforeAll runs npm install per fixture, and this machine's registry returns 403 Forbidden - GET https://us-npm.pkg.dev/.../@a2a-js%2fsdk. The failure is in the hook, before any assertion, and it reproduces on main.

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

I ran a local end-to-end check with no mocks: real AgentLoader, real esbuild compile, real HTTP. To repeat it:

  1. npm ci && npm run build.
  2. Create an agents directory with plain/agent.ts (exports rootAgent) and wrapped/app.ts (exports app).
  3. Start new AdkApiServer({agentsDir, sessionService, a2a: true}) with a session that already holds a function-call event.
  4. GET /apps/<name>/users/u1/sessions/<id>/events/<eventId>/graph returns 200, and dotSrc names plain_agent and wrapped_agent.
  5. GET /a2a/<name>/.well-known/agent-card.json returns 200 for both.
  6. new AgentFile(path).loadApp is undefined, and loadAgent() returns the root agent for both file shapes.

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 4 commits August 8, 2026 17:27
loadApp() synthesizes an App named after the agent, which would override
the loader key Runner uses as appName, and validateAppName rejects agent
names that validateAgentName accepts. No caller can adopt it without a
behaviour change, so remove it. adk-python keeps App synthesis off the
loader for the same reason.
The graph route and the adk run watcher both open-coded the narrowing
loadAgent() already performs. The A2A bootstrap also rebuilt the loaded
value as `adkApp ?? agent`, which is the loaded value itself.
Three tests now build the same duck-typed AgentFile, so build it in one
place.
The bootstrap kept the hand-written narrowing this change removes
everywhere else. load() is memoized, so the second call is free.

Restore the App-identity assertion the loadApp() test rewrite dropped,
and pin that the bootstrap calls loadAgent().
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