Skip to content

TestAgent.setNextInvokeResult is misleadingly named: variadic, replaces the entire queue #21

@joewalker

Description

@joewalker

Observed behavior

TestAgent.setNextInvokeResult in src/agents/test.ts reads as a singular setter ("set the next invoke result") but:

setNextInvokeResult(...results: Array<InvokeResult>): void {
  this.#results = results;
}

It accepts a variadic list of results, and on each call it replaces the entire queue rather than appending. Both deviations from the apparent contract are easy to misread:

  • The plural variadic signature suggests it queues multiple results.
  • The "set next" naming suggests it sets only the next response (or appends to a queue).
  • The actual behaviour is "replace the whole queue with these values".

The test should replace results when setNextInvokeResult is called again confirms that the replacement behaviour is intentional, but the test only exists because the name does not communicate the behaviour.

In practice every existing caller (src/__test__/loop.test.ts and src/agents/__test__/test-agent.test.ts) passes all of its canned results in a single call, presumably because mixing append/replace semantics would be too risky.

Expected behavior

Rename to something that conveys both "plural" and "replaces" semantics, for example:

setInvokeResults(...results: Array<InvokeResult>): void {
  this.#results = [...results];
}

Or, if append semantics are actually desired (which would also let multiple setNextInvokeResult calls compose), split into two methods:

queueInvokeResult(result: InvokeResult): void { this.#results.push(result); }
clearInvokeResults(): void { this.#results = []; }

Either way the chosen names should make the behaviour obvious without needing to read the implementation or a dedicated test.

Minimal reproduction

const agent = new TestAgent();
agent.setNextInvokeResult({ status: 'success', output: 'a' });
agent.setNextInvokeResult({ status: 'success', output: 'b' }); // silently discards 'a'

await agent.invoke('first');  // returns 'b'
await agent.invoke('second'); // returns the '#results is empty' error

A reader who saw setNextInvokeResult in calling code would reasonably expect the second invoke to return 'a' (because they assumed the call appended to a queue) or 'b' (because they assumed it only set the single next result). Neither expectation matches the actual replace-everything behaviour.

Suggested fix

Rename the method to setInvokeResults (plural) and update the two test files that use it. Also defensively copy the rest array (this.#results = [...results];) so callers can mutate the array they passed in without disturbing the queue.

Metadata

Metadata

Assignees

No one assigned

    Labels

    S4Clean-ups or nits with low behavioral riskbugSomething isn't working

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions