Skip to content

Fix: only pass esbuild "external" when bundling is enabled in AgentFile.load() - #275

Open
AmaadMartin wants to merge 2 commits into
mainfrom
fix/agent-loader-esbuild-external-without-bundle
Open

Fix: only pass esbuild "external" when bundling is enabled in AgentFile.load()#275
AmaadMartin wants to merge 2 commits into
mainfrom
fix/agent-loader-esbuild-external-without-bundle

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

Problem: AgentFile.load() builds one esbuild options object and passes external unconditionally, while bundle is taken straight from AgentFileOptions.bundle. esbuild rejects external unless bundling is on, so any non-truthy bundle hard-fails:

✘ [ERROR] Cannot use "external" without "bundle"
Error: Build failed with 1 error: error: Cannot use "external" without "bundle"

shouldCompile = compile || bundle, so the compile branch is still entered for {compile: true, bundle: false} and the build runs and throws. The thrown value is an esbuild BuildFailure, not an AgentFileLoadingError, so AgentLoader re-throws it and the whole invocation dies instead of skipping the file.

This breaks a documented first-class CLI knob. --bundle [boolean] is registered as BUNDLE_AGENT_FILE in dev/src/cli/cli.ts and attached to five commands (web, api_server, run, deploy cloud_run, deploy agent_engine), and getBoolean maps --bundle false to boolean false. It also breaks the library surface: new AgentFile(p, {compile: true, bundle: false}), and equally new AgentFile(p, {compile: true}) — with bundle omitted it is undefined, which esbuild treats the same as false for this validation. The fix therefore keys off truthiness, not === false.

Verified against the esbuild the repo actually resolves (dev/package.json pins ^0.25.9; 0.25.12 installed here). All five combinations probed directly:

options result
bundle: false + external throws Cannot use "external" without "bundle"
bundle: undefined + external throws Cannot use "external" without "bundle"
bundle: false, no external ok
bundle: undefined, no external ok
bundle: false + external: undefined ok

Solution: pass external only when bundling is actually enabled.

  • Hoisted the allowlist verbatim into a module-scope EXTERNAL_PACKAGES constant (contents, order, and explanatory comments unchanged). The repo's TypeScript guidelines put static string lists at file level rather than inline inside a method, and it keeps the build-options object readable.
  • Replaced the inline external: [...] with ...(this.options.bundle ? {external: EXTERNAL_PACKAGES} : {}).

A conditional spread rather than external: undefined: although esbuild also accepts external: undefined, the postcondition worth pinning is 'external' in buildOptions === Boolean(options.bundle), and only the spread genuinely omits the key.

Deliberately unchanged, and checked rather than assumed:

  • packages: 'bundle' stays ungated — esbuild imposes no equivalent restriction on it, and {bundle: false, packages: 'bundle'} builds cleanly.
  • linkProjectNodeModules keeps running on this path. The unbundled artifact resolves its bare imports at import time through that symlink, so it is load-bearing here, not incidental.
  • No new try/catch and no error wrapping. Genuine esbuild failures (a syntax error in the user's agent file) must keep propagating exactly as today; swallowing them would silently hide broken agents from adk web.
  • bundle: true — the default in DEFAULT_AGENT_FILE_OPTIONS and on every CLI command — produces a byte-identical esbuild invocation, so the common path is untouched. No public API, type, export, or CLI surface change. The misleading --bundle help text (a copy of --compile's) is a separate concern and is not touched here.

Collision check (required before implementing): scanned all 181 open PRs on the fork for bundle / external / esbuild / agent.?loader / agent.?file. None implements this fix. Only #264 ("Perf: scope the agent-loader esbuild plugin filter and skip node_modules during discovery") touches the same two files, in disjoint regions (replaceDirnamePlugin and getDirFiles, vs. the esbuild.build options object here). git merge-tree against that branch reports zero conflicts, so this is branched from main rather than stacked.

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.

Three tests added to dev/test/utils/agent_loader_test.ts, all inside the existing describe('AgentFile') block:

  1. omits esbuild "external" when bundling is disabled{compile: true, bundle: false}; asserts the options object has no external key and matches {bundle: false, minify: false}.
  2. omits esbuild "external" when bundle option is not provided{compile: true}, i.e. the bundle: undefined row; asserts no external key.
  3. compiles and loads a .ts agent with real esbuild when bundle is disabled — swaps the module mock for the real esbuild.build via vi.importActual and drives a full load(). Asserts the agent loads (agent.name === 'agent2') and that the emitted .cjs contains a literal require("@google/adk"), i.e. the dependency really was left external rather than inlined. This is the only test in the file that exercises real esbuild, and the only one that can catch the reported error, since a mocked build can only ever echo back what it was handed.

The existing positive case (loads .ts agent file and compiles it) already pins bundle: true, minify: true, external: expect.arrayContaining(['onnxruntime-node']) and is untouched — no existing test was weakened, skipped, or deleted.

npx vitest run --project unit:dev dev/test/utils/agent_loader_test.ts
  ✓ dev/test/utils/agent_loader_test.ts (33 tests)  Tests  33 passed (33)

Proof the tests can fail (mutation testing). Coverage alone is not evidence, so each test was run against mutated source:

Mutation A — revert the source hunk entirely (restore the unconditional external). All three new tests fail:

FAIL > omits esbuild "external" when bundling is disabled
  AssertionError: expected { …(11) } to not have property "external"
  Received: [ "sqlite3", "better-sqlite3", …, "jiti/package.json" ]

FAIL > omits esbuild "external" when bundle option is not provided
  AssertionError: expected { …(11) } to not have property "external"

FAIL > compiles and loads a .ts agent with real esbuild when bundle is disabled
  Error: Build failed with 1 error:
  error: Cannot use "external" without "bundle"

Tests  3 failed | 30 passed (33)

Mutation B — invert the new guard (...(!this.options.bundle ? …)). Catches the other arm: the pre-existing positive test now fails too, proving the bundle: true path is still pinned.

FAIL > loads .ts agent file and compiles it
  -   "external": ArrayContaining [ …
Tests  4 failed | 29 passed (33)

Coverage. The fix adds exactly one branch. Measured on the changed file with the targeted suite: both arms of the new ternary at agent_loader.ts:219 are executed (35 hits truthy / 3 hits falsy), and no added line is uncovered — 100% new-line and new-branch coverage. Global thresholds in vitest.config.ts are untouched.

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

Setup: a scratch project with package.json, a node_modules/@google/adk link to this checkout's core, and an agent.ts exporting rootAgent (a BaseAgent subclass whose runAsyncImpl yields one createEvent(...), so no model credentials are needed).

  1. Reproduce the bug. With the guard removed from the built loader, adk run --bundle false ./agent.ts aborts before the agent ever loads:

    ✘ [ERROR] Cannot use "external" without "bundle"
    Error: Build failed with 1 error: error: Cannot use "external" without "bundle"
        at failureErrorWithLog (node_modules/esbuild/lib/main.js:1467:15)
    
  2. adk run --bundle false ./agent.ts (fixed) — loads and responds:

    Running agent probe_agent, type exit to exit.
    [user]: [probe_agent]: probe agent replied
    
  3. adk web --bundle false --port <port> ./agent.ts (fixed) — server starts, GET /list-apps returns ["agent"], and after creating a session POST /run returns the agent's event:

    [{"invocationId":"e-b50428c2-…","author":"probe_agent",
      "content":{"role":"model","parts":[{"text":"probe agent replied"}]}, …}]
    

    The emitted artifact is genuinely unbundled: 1,746 bytes containing require("@google/adk").

  4. Regression check, no flag (bundle defaults to true): adk run ./agent.ts loads and responds identically.

Full local validation on the pushed commit (npm install from a clean tree, so no lockfile churn): npm run build, npm run lint, npm run format:check, npm run docs:check all exit 0, plus the targeted suite above.

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.

@AmaadMartin

Copy link
Copy Markdown
Owner Author

Keep. This is not a duplicate of #506, despite an earlier triage note saying so. #506 touches no production source and hands the external-without-bundle fix to this PR. Green on all three runners.

Amaad Martin added 2 commits August 9, 2026 01:29
esbuild rejects the `external` option unless `bundle` is enabled, so
`AgentFile.load()` hard-failed with `Cannot use "external" without
"bundle"` for every non-truthy `bundle` value -- breaking `adk web|run|
api_server|deploy --bundle false` and `new AgentFile(p, {compile: true})`.

Hoist the allowlist to a module-scope `EXTERNAL_PACKAGES` constant
(contents unchanged) and spread it into the build options only when
bundling, so the key is genuinely absent otherwise. `bundle: true`, the
default on every code path, produces an identical esbuild invocation.
Drop a tautological toHaveProperty('external') assertion -- the
toMatchObject two lines above already asserts
external: expect.arrayContaining(['onnxruntime-node']), which fails when
the key is absent. Scope the real-esbuild implementation to the single
build the test triggers so it cannot leak into later tests
(vi.clearAllMocks clears calls but keeps implementations).
@AmaadMartin
AmaadMartin force-pushed the fix/agent-loader-esbuild-external-without-bundle branch from 2475bfd to 155ee79 Compare August 9, 2026 08:36
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