Skip to content

Fix: remove the dead AgentLoader.listApps() and getAppFile() pair - #731

Open
AmaadMartin wants to merge 2 commits into
mainfrom
fix/remove-dead-agent-loader-list-apps
Open

Fix: remove the dead AgentLoader.listApps() and getAppFile() pair#731
AmaadMartin wants to merge 2 commits into
mainfrom
fix/remove-dead-agent-loader-list-apps

Conversation

@AmaadMartin

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: AgentLoader in dev/src/utils/agent_loader.ts carries two methods that nothing in dev/src calls: listApps() and getAppFile(). getAppFile() only forwards to getAgentFile(). listApps() loads every discovered candidate a second time through agentFile.load() only to run an isApp() test, which makes the dead method the most expensive one on the class.

Solution: I chose option (a) and deleted both methods. Four facts support that choice: no caller exists in dev/src; AgentLoader is not exported from dev/src/index.ts, which exports only AdkApiClient and AdkApiServer; getAppFile() was a pure alias for getAgentFile(); and the Python reference AgentLoader has no list_apps(), while its list_agents_detailed() flattens an App to its root agent instead of reporting an App/agent distinction. I rejected option (b), wiring up a consumer, because it needs a new GET /list-apps response shape and a UI consumer that neither this repository nor the Python reference asks for.

Notes for the reviewer:

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        # 31 passed
npx vitest run --project unit:dev dev/test/server/adk_api_client_test.ts     # 20 passed
npx vitest run --project integration tests/integration/app_loader/app_loader_test.ts   # 6 passed
npm run lint          # clean
npm run format:check  # clean

npm run ts:check reports 280 errors. All of them are pre-existing in core/test/**; the error set is byte-identical before and after this change, and none of them names a file in this diff.

Mutation tests. Both rewritten assertions were run against mutated source and both failed.

  1. Unit test. I deleted the app.* entrypoint preference in loadAgentFromDirectory(), so only agent.* is discovered:

    -      subFiles.find((f) => f.isFile && f.name === 'app' && isJsFile(f.ext)) ??
           subFiles.find((f) => f.isFile && f.name === 'agent' && isJsFile(f.ext));

    discovers app entrypoint files (e.g. app.js) in directories FAILED:
    AssertionError: expected [ 'agent1', 'agent2', 'agent3' ] to include 'my_service'.

  2. Integration test. I stopped AgentFile.load() caching the App for a module that exports app, so it returns the root agent instead:

         if (isApp(jsModule.app)) {
    -      this.app = jsModule.app;
           this.agent = jsModule.app.rootAgent;
    -      return this.app!;
    +      return this.agent!;
         }

    should discover apps vs agents across directories and standalone files FAILED:
    expected [] to deeply equal [ 'service_alpha', 'standalone_app' ].
    This mutation also fails the pre-existing should load App from directory entrypoint case, so the new assertion is not the only detector, but it does carry signal.

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

npm run build
node dev/dist/esm/cli_entrypoint.js web tests/integration/app_loader/discovery --port 8477
curl -s localhost:8477/list-apps

Result: ["service_alpha","service_beta","standalone_agent","standalone_app"], the expected sorted array.

One environment note: the integration suite runs npm install in its fixture directory, and that install fails on my machine against a private registry mirror. I ran it with npm_config_registry=https://registry.npmjs.org. The failure is local and unrelated to this change, so I did not weaken the test.

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 2 commits August 6, 2026 10:01
The two tests that called AgentLoader.listApps() and getAgentFile() also
cover live behaviour: app.* entrypoint discovery in a directory, and App
classification of the discovered files. Restate that coverage through the
API the dev server and the deploy CLI actually use, so the next commit can
delete the dead pair without losing the regression signal.

The integration assertion is now a sorted toEqual over the full app set,
which is stronger than the previous toHaveLength(2) plus two toContain
checks.
Nothing in dev/src calls either method. getAppFile() only forwards to
getAgentFile(), and listApps() loads every discovered candidate a second
time just to run an isApp() test on the result. The dev server answers
GET /list-apps with listAgents(), which this change does not touch, so the
response is unchanged.

AgentLoader is internal to @google/adk-devtools: dev/src/index.ts exports
only AdkApiClient and AdkApiServer, so this is not a public API break.
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