Skip to content

Fix: merge AgentFileOptions per field so partial options keep the compile/bundle defaults - #309

Open
AmaadMartin wants to merge 3 commits into
mainfrom
fix/agent-loader-merge-partial-options
Open

Fix: merge AgentFileOptions per field so partial options keep the compile/bundle defaults#309
AmaadMartin wants to merge 3 commits into
mainfrom
fix/agent-loader-merge-partial-options

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 and AgentLoader declared their defaults as a whole-object
default parameter value:

constructor(
  private readonly filePath: string,
  private readonly options = DEFAULT_AGENT_FILE_OPTIONS,   // {compile: true, bundle: true}
) {}

A TypeScript default parameter is applied only when the argument is undefined, so passing
any object replaces the defaults wholesale. Every field of AgentFileOptions is optional,
so a partial bag compiles cleanly and silently loses compile/bundle:

new AgentFile(p, {moduleType: FileModuleType.ESM});
// this.options.compile === undefined, this.options.bundle === undefined

load() then computes shouldCompile = this.options.compile || this.options.bundle as
false and skips the whole esbuild block: no compilation, no replaceDirnamePlugin, no
node_modules symlink, and getFilePath() returns the raw source path — the path the Cloud
Run / Agent Engine deployers copy into the deployment bundle. For a .ts agent it simply
throws at import:

TypeError [ERR_UNKNOWN_FILE_EXTENSION]: Unknown file extension ".ts" for .../agent.ts

The first-party CLI escapes this because getAgentFileOptions (dev/src/cli/cli.ts:74)
always materialises compile and bundle as concrete booleans via getBoolean, and
--compile/--bundle are both .default(true). The bug is therefore reachable only from
programmatic callers — AdkApiServer, runAgent, deployToCloudRun, deployToAgentEngine
— which is precisely the audience of the exported @google/adk-devtools API.

A second, smaller defect from the same line: the default handed out the module-level
DEFAULT_AGENT_FILE_OPTIONS object by reference, so every option-less instance shared one
mutable object (readonly pins the binding, not the contents).

Solution: resolve the options in the AgentFile constructor by spreading the caller's
object over the defaults, so omitted fields fall back per field and each instance owns its
own object:

private readonly options: AgentFileOptions;

constructor(
  private readonly filePath: string,
  options: AgentFileOptions = {},
) {
  this.options = {...DEFAULT_AGENT_FILE_OPTIONS, ...options};
}

AgentLoader reads this.options nowhere except new AgentFile(path, this.options), so it
does not repeat the merge — a second merge would be idempotent and would only create a
keep-in-sync hazard. Its default becomes {} instead of the shared constant, which removes
the by-reference hand-out and keeps the "defaults live in exactly one place" property true
rather than coincidental. Constructor arity and positional order are unchanged in both
classes, so new AgentLoader(dir, opts, watch) and the three-argument call in
adk_api_server.ts keep working.

This introduces no new pattern: the repo already defaults options bags per field elsewhere
(e.g. adk_api_server.ts:100-114 uses ?? per field). The agent-loader constructors were
the outlier.

Deliberately not done, to keep the diff minimal: no undefined-stripping helper, no
??-per-field ladder, no generic mergeDefaults utility, no new exported "resolved options"
type, and AgentFileOptions keeps all three fields optional (moduleType must stay absent
by default so load() can fall back to getFileModuleType(filePath)).

Behaviour change to review deliberately. The merge flips exactly one class of caller —
one that passes an options object omitting compile and/or bundle:

Caller shape Before After
no second argument compile+bundle true unchanged
{compile: false, bundle: false} no compile unchanged
{compile: true} bundle undefined → compiles unbundled, unminified bundle true → compiles bundled + minified
{bundle: false} compile undefined → no compile at all compile true → compiles, unbundled
{moduleType: 'esm'} no compile at all compile+bundle true

Row 3 is the one that is arguably a behaviour change rather than a pure fix: someone who
wrote {compile: true} meaning "compile but don't bundle" now also gets bundling. It is
still the correct reading of the contract — AgentFileOptions has no way to express
"omitted" distinctly from "default", the documented default for bundle is true, and the
alternative (defaulting only the fields the caller did not mention unless they mentioned a
sibling field) is unimplementable and unreadable. Callers who want transpile-without-bundle
should say {compile: true, bundle: false}, which is now honoured exactly.

Plain spread semantics are accepted deliberately and documented on the interface:
{compile: undefined} yields undefined, not true. exactOptionalPropertyTypes is off so
this is expressible, but no in-repo caller does it.

Collision check (open PRs on this fork, --state open --limit 100). Three open PRs touch
dev/src/utils/agent_loader.ts: #285 (externalize third-party packages), #275 (only pass
esbuild external when bundling), #264 (scope the esbuild plugin filter / skip
node_modules). I diffed all three: none touches either constructor, so nothing already
lands this change and there is no textual conflict — this PR branches from main rather than
stacking. One semantic interaction is worth flagging for whoever merges second: #275 adds a
test omits esbuild "external" when bundle option is not provided that constructs
new AgentFile(agentPath, {compile: true}) and asserts no external key. Under this fix
{compile: true} resolves bundle to true, so that assertion becomes wrong and the test
must be re-pointed at {compile: true, bundle: false}. #285 carries the same logic. No
change is needed here today because neither is merged.

Public-repo hygiene: the diff and commit messages were grepped for internal-only
references (tracker ids, internal hostnames, short links); none are present.

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.

Five cases added to the existing describe blocks in
dev/test/utils/agent_loader_test.ts, reusing the file's existing harness (the hoisted
vi.mock('esbuild') factory, the (esbuild.build as Mock).mockImplementation pattern, the
compiledPath helper and the fileUtils.getTempDir mock). No new mock layer, no new test
file, and no existing test was modified, weakened or deleted — all 30 pre-existing cases
still pass unchanged.

Test Pins
inherits the compile and bundle defaults when only moduleType is given {moduleType: ESM} now compiles: esbuild called with format: 'esm', bundle: true, minify: true, outfile agent2.mjs
inherits the bundle default when only compile is given row 3 of the table above, made explicit and reviewable rather than incidental
inherits the compile default when only bundle is given {bundle: false} still transpiles (bundle: false, minify: false) instead of skipping esbuild entirely
does not compile when compile and bundle are explicitly false guard: the explicit opt-out still wins and esbuild is never called
forwards options merged with the defaults to every agent file AgentLoader forwarding, plus the third positional watchForChanges argument surviving the signature change

One new fixture, agent2MjsContentMocked, is required because the ESM case writes a .mjs
artifact and the existing CJS fixture's exports.rootAgent cannot be imported from one.

A sixth case from the plan ("no options behaves as before") was not added: the
pre-existing loads .ts agent file and compiles it already constructs new AgentFile(path)
with no options and asserts bundle: true, minify: true on the esbuild call, so it is
already pinned. An "instance isolation" case was also dropped: with nothing mutating
this.options, the property is unobservable without reaching into a private field, and a
test that fails only for the same reason as case 1 is not a real signal.

Proof the new tests can fail (mutation). Mutated the single merge site back to the
unfixed semantics:

-    this.options = {...DEFAULT_AGENT_FILE_OPTIONS, ...options};
+    this.options = options;

Result — the four merge-dependent tests FAIL, the explicit-opt-out guard correctly still
passes:

× AgentFile > inherits the compile and bundle defaults when only moduleType is given
× AgentFile > inherits the bundle default when only compile is given
× AgentFile > inherits the compile default when only bundle is given
✓ AgentFile > does not compile when compile and bundle are explicitly false
× AgentLoader > forwards options merged with the defaults to every agent file

Representative message:

FAIL dev/test/utils/agent_loader_test.ts > AgentFile > inherits the compile and bundle
     defaults when only moduleType is given
AssertionError: expected "spy" to be called with arguments: [ ObjectContaining{…} ]
Number of calls: 0

(The same mutation also breaks four pre-existing tests, since removing the merge removes the
no-options default too — further confirmation the merge is load-bearing.)

Commands run locally on the pushed commit:

npx vitest run --project unit:dev dev/test/utils/agent_loader_test.ts   # 35 passed
npx vitest run --project integration tests/integration/app_loader/app_loader_test.ts  # 6 passed
npm run build          # exit 0
npm run lint           # exit 0
npm run format:check   # exit 0
npm run docs:check     # exit 0

tests/integration/app_loader/app_loader_test.ts was run unmodified and passes; it
exercises the real, unmocked esbuild path via new AgentLoader(projectPath), confirming the
no-options path is unchanged. Note it flakes independently of this change: its beforeAll
runs npm install in a fixture and intermittently exceeds the hook timeout (reported as
6 skipped, tests 160.03s, nothing executed). Two of three local runs hit that; the third
passed all 6. This is a known pre-existing issue with open PRs against it, and a fixture
npm install cannot be affected by a constructor change.

npm run ts:check is not run by CI and fails on main today with pre-existing errors in
core/** and tests/**; this change adds zero errors under dev/.

No suppressions of any kind were added — no any, as any, as never, as unknown as,
@ts-ignore, @ts-expect-error, eslint-disable, or coverage-ignore.

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

Run against the built dev package with real esbuild and no mocks:

  1. Create a scratch project with @google/adk resolvable and a TypeScript agent:
// agents/agent.ts
import {BaseAgent} from '@google/adk';
class ScratchAgent extends BaseAgent {
  constructor(name: string) {
    super({name});
  }
}
export const rootAgent = new ScratchAgent('scratch_agent');
  1. Drive the loader with a partial options bag:
const file = new AgentFile('<abs>/agents/agent.ts', {
  moduleType: FileModuleType.ESM,
});
const agent = await file.loadAgent();
console.log(agent.name, file.getFilePath());
await file.dispose();

const loader = new AgentLoader('<abs>/agents', {
  moduleType: FileModuleType.ESM,
});
console.log(await loader.listAgents());
await loader.disposeAll();

const raw = new AgentFile('<abs>/agents/agent.ts', {
  compile: false,
  bundle: false,
});
await raw.loadAgent().catch((e) => console.log('opt-out ->', e.code));

Observed before the fix (dev rebuilt from main):

TypeError [ERR_UNKNOWN_FILE_EXTENSION]: Unknown file extension ".ts" for .../agents/agent.ts

Observed after the fix:

AgentFile partial options -> scratch_agent /tmp/adk_agent_loader/<id>/agent.mjs
AgentLoader partial options -> [ 'agent' ]
AgentLoader artifact -> /tmp/adk_agent_loader/<id>/agent.mjs
opt-out -> not compiled, raw import failed: ERR_UNKNOWN_FILE_EXTENSION

i.e. the partial bag now yields a compiled .mjs artifact, and the explicit opt-out still
correctly skips compilation.

CLI paths were verified by inspection to be bit-for-bit unaffected: getAgentFileOptions
(dev/src/cli/cli.ts:74-84) always emits compile and bundle as concrete booleans, so the
merge is a no-op for adk run / adk web / adk deploy.

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 3 commits July 30, 2026 06:12
AgentFile and AgentLoader declared DEFAULT_AGENT_FILE_OPTIONS as a
whole-object default parameter, which TypeScript applies only when the
argument is undefined. Any caller-supplied object replaced the defaults
wholesale, so a partial bag such as {moduleType: FileModuleType.ESM}
resolved compile and bundle to undefined and skipped compilation
entirely -- handing a raw .ts agent file to Node's ESM loader.

Resolve the options in each constructor by spreading the caller's
object over the defaults, so omitted fields fall back and each instance
owns its own object instead of aliasing the shared module-level const.
Cover the three partial-options shapes that previously lost a default
({moduleType}, {compile: true}, {bundle: false}), the explicit opt-out
that must keep skipping compilation, and AgentLoader forwarding the
merged options into every AgentFile it constructs.
Resolve the defaults only in AgentFile, which is where they are read.
AgentLoader forwards its bag verbatim, so a second identical merge there
was a no-op with a keep-in-sync hazard; its default becomes an empty bag
instead of the shared module-level constant.

Also drop the {@link} to the unexported DEFAULT_AGENT_FILE_OPTIONS,
which typedoc cannot resolve from an exported interface.
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