Skip to content

fix: reconcile ActionExecutor merge conflicts and restore regressions #390

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
MyTH-zyxeon wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix: reconcile ActionExecutor merge conflicts and restore regressions #390

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
126 changes: 40 additions & 86 deletions src/actions/action-executor.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,60 +27,35 @@ export class ActionExecutor {
public constructor(
private readonly toolRegistry: ToolRegistry,
maxToolCallsPerTaskOrLogger?: number | RuntimeLogger,
eventBus?: RuntimeEventBus,
maxTrackedTasks = 1_000
eventBusOrLogger?: RuntimeEventBus | RuntimeLogger,
loggerOrMaxTrackedTasks?: RuntimeLogger | number
) {
const maxTrackedTasks =
typeof loggerOrMaxTrackedTasks === "number"
? loggerOrMaxTrackedTasks
: 1_000;
if (!Number.isInteger(maxTrackedTasks) || maxTrackedTasks < 1) {
throw new RangeError("maxTrackedTasks must be a positive integer.");
}

if (typeof maxToolCallsPerTaskOrLogger === "number") {
this.maxToolCallsPerTask = maxToolCallsPerTaskOrLogger;
if (
eventBusOrLogger !== undefined &&
"emit" in eventBusOrLogger &&
typeof eventBusOrLogger.emit === "function"
) {
this.eventBus = eventBusOrLogger;
this.logger = logger;
} else {
this.eventBus = undefined;
this.logger = (eventBusOrLogger as RuntimeLogger | undefined) ?? logger;
}
} else if (
maxToolCallsPerTaskOrLogger !== undefined &&
typeof maxToolCallsPerTaskOrLogger === "object" &&
("info" in maxToolCallsPerTaskOrLogger ||
"warn" in maxToolCallsPerTaskOrLogger ||
"debug" in maxToolCallsPerTaskOrLogger ||
"error" in maxToolCallsPerTaskOrLogger)
) {
this.maxToolCallsPerTask = undefined;
this.logger = maxToolCallsPerTaskOrLogger;
if (
eventBusOrLogger !== undefined &&
"emit" in eventBusOrLogger &&
typeof eventBusOrLogger.emit === "function"
) {
this.eventBus = eventBusOrLogger;
} else {
this.eventBus = undefined;
}
} else {
this.maxToolCallsPerTask = undefined;
if (
eventBusOrLogger !== undefined &&
"emit" in eventBusOrLogger &&
typeof eventBusOrLogger.emit === "function"
) {
this.eventBus = eventBusOrLogger;
this.logger = logger;
} else {
this.eventBus = undefined;
this.logger = (eventBusOrLogger as RuntimeLogger | undefined) ?? logger;
}
}
this.eventBus = eventBus;
this.maxToolCallsPerTask =
typeof maxToolCallsPerTaskOrLogger === "number"
? maxToolCallsPerTaskOrLogger
: undefined;
this.eventBus =
eventBusOrLogger !== undefined && "emit" in eventBusOrLogger
? eventBusOrLogger
: undefined;
this.logger =
(typeof maxToolCallsPerTaskOrLogger === "object"
? maxToolCallsPerTaskOrLogger
: undefined) ??
(eventBusOrLogger !== undefined && "info" in eventBusOrLogger
? eventBusOrLogger
: undefined) ??
(typeof loggerOrMaxTrackedTasks === "object"
? loggerOrMaxTrackedTasks
: undefined);
this.maxTrackedTasks = maxTrackedTasks;
}

Expand All @@ -103,15 +78,22 @@ export class ActionExecutor {
payload: TPayload,
context: RuntimeContext
): Promise<TResult> {
const tool = this.toolRegistry.get(toolName);

const currentCount = this.getToolCallCount(context.taskId);
if (this.maxToolCallsPerTask !== undefined) {
assertMaxToolCalls(currentCount, this.maxToolCallsPerTask);
}

const tool = this.toolRegistry.get(toolName);

if (
!this.toolCallCounts.has(context.taskId) &&
this.toolCallCounts.size >= this.maxTrackedTasks
) {
const oldestTaskId = this.toolCallCounts.keys().next().value;
if (oldestTaskId !== undefined) {
this.toolCallCounts.delete(oldestTaskId);
}
}
this.toolCallCounts.set(context.taskId, currentCount + 1);

const startedAt = Date.now();
Expand All @@ -123,41 +105,13 @@ export class ActionExecutor {
taskId: context.taskId,
agentId: resolveAgentId(context.agent),
toolName,
},
invokedAt: new Date().toISOString()
}
});

try {
const result = await tool.execute(payload, context);
const duration = Date.now() - startedAt;

this.eventBus?.emit({
name: "runtime.tool.success",
payload: {
runtimeId: context.runtimeId,
taskId: context.taskId,
agentId: resolveAgentId(context.agent),
toolName,
duration,
},
});

return result;
} catch (error) {
const duration = Date.now() - startedAt;

this.eventBus?.emit({
name: "runtime.tool.error",
payload: {
runtimeId: context.runtimeId,
taskId: context.taskId,
agentId: resolveAgentId(context.agent),
toolName,
duration,
error,
},
});

throw error;
}
const result = (await tool.execute({ payload, context })) as TResult;
const durationMs = Math.max(0, Date.now() - startedAt);
this.logger?.info("Tool invocation completed.", { toolName, durationMs });
return result;
}
}
}
207 changes: 207 additions & 0 deletions tests/actions/action-executor-merge-regression.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,207 @@
import { describe, expect, it, vi } from "vitest";
import { ActionExecutor } from "../../src/actions/action-executor.js";
import { RuntimeEventBus } from "../../src/events/runtime-events.js";
import type { RuntimeContext } from "../../src/runtime/context.js";
import { ToolRegistry } from "../../src/tools/tool-registry.js";
import type { ToolDefinition } from "../../src/tools/types.js";

// These tools do not access the context's memory, provider, or state ports.
const context = (taskId: string): RuntimeContext =>
({
runtimeId: "merge-regression",
taskId,
agent: { agentId: "agent", createdAt: "2026-01-01T00:00:00.000Z" }
}) as RuntimeContext;

const registryWithPing = (): ToolRegistry => {
const registry = new ToolRegistry();
registry.register({
name: "ping",
description: "ping",
execute: () => "pong"
});
return registry;
};

describe("ActionExecutor merge regression", () => {
it.each(["second", "third", "fourth"] as const)(
"preserves the %s logger argument and the tool/event contracts",
async (position) => {
const registry = new ToolRegistry();
const execute = vi.fn<ToolDefinition["execute"]>(async () => "result");
registry.register({ name: "echo", description: "echo", execute });
const logger = {
info: vi.fn(),
warn: vi.fn(),
error: vi.fn(),
debug: vi.fn()
};
const eventBus = new RuntimeEventBus();
const invoked = vi.fn();
eventBus.on("runtime.tool.invoked", invoked);
const executor =
position === "second"
? new ActionExecutor(registry, logger, eventBus)
: position === "third"
? new ActionExecutor(registry, 1, logger)
: new ActionExecutor(registry, 1, eventBus, logger);
const payload = { value: 7 };
const task = context("contract");

await expect(executor.execute("echo", payload, task)).resolves.toBe(
"result"
);
expect(execute).toHaveBeenCalledExactlyOnceWith({
payload,
context: task
});
expect(execute.mock.calls[0]?.[0].context).toBe(task);
expect(logger.info).toHaveBeenCalledExactlyOnceWith(
"Tool invocation completed.",
{
toolName: "echo",
durationMs: expect.any(Number)
}
);
if (position !== "third") {
expect(invoked).toHaveBeenCalledExactlyOnceWith({
name: "runtime.tool.invoked",
payload: {
runtimeId: task.runtimeId,
taskId: task.taskId,
agentId: "agent",
toolName: "echo",
invokedAt: expect.any(String)
}
});
expect(
Number.isNaN(Date.parse(invoked.mock.calls[0]?.[0].payload.invokedAt))
).toBe(false);
}
}
);

it("does not charge, emit, or evict an existing task on a failed lookup", async () => {
const eventBus = new RuntimeEventBus();
const invoked = vi.fn();
eventBus.on("runtime.tool.invoked", invoked);
const executor = new ActionExecutor(registryWithPing(), 1, eventBus, 1);
await executor.execute("ping", {}, context("existing"));

await expect(
executor.execute("missing", {}, context("new"))
).rejects.toMatchObject({
code: "TOOL_NOT_FOUND",
details: { toolName: "missing" }
});
expect(executor.getToolCallCount("new")).toBe(0);
expect(executor.getToolCallCount("existing")).toBe(1);
expect(invoked).toHaveBeenCalledTimes(1);
await expect(executor.execute("ping", {}, context("new"))).resolves.toBe(
"pong"
);
});

it.each([false, true])(
"charges a registered tool failure (async=%s)",
async (isAsync) => {
const registry = new ToolRegistry();
const failure = new Error("tool failed");
registry.register({
name: "fail",
description: "fail",
execute: () => {
if (isAsync) return Promise.reject(failure);
throw failure;
}
});
const executor = new ActionExecutor(registry, 1);
const task = context("failure");
await expect(executor.execute("fail", {}, task)).rejects.toBe(failure);
expect(executor.getToolCallCount(task.taskId)).toBe(1);
await expect(executor.execute("fail", {}, task)).rejects.toMatchObject({
code: "MAX_TOOL_CALLS_EXCEEDED"
});
}
);

it.each([false, true])(
"reserves an in-flight call even when it rejects=%s",
async (rejects) => {
let resolve!: (value: string) => void;
let reject!: (reason: Error) => void;
const deferred = new Promise<string>((resolvePromise, rejectPromise) => {
resolve = resolvePromise;
reject = rejectPromise;
});
const registry = new ToolRegistry();
const execute = vi.fn(() => deferred);
registry.register({ name: "wait", description: "wait", execute });
const executor = new ActionExecutor(registry, 1);
const task = context("pending");
const pending = executor.execute("wait", {}, task);
expect(executor.getToolCallCount(task.taskId)).toBe(1);
await expect(executor.execute("wait", {}, task)).rejects.toMatchObject({
code: "MAX_TOOL_CALLS_EXCEEDED"
});
expect(execute).toHaveBeenCalledTimes(1);
if (rejects) {
const failure = new Error("deferred failure");
const assertion = expect(pending).rejects.toBe(failure);
reject(failure);
await assertion;
} else {
resolve("done");
await expect(pending).resolves.toBe("done");
}
expect(executor.getToolCallCount(task.taskId)).toBe(1);
}
);

it("retains guard precedence for a zero budget and an exhausted budget", async () => {
const registry = registryWithPing();
const lookup = vi.spyOn(registry, "get");
const disabled = new ActionExecutor(registry, 0);
await expect(
disabled.execute("missing", {}, context("zero"))
).rejects.toMatchObject({
code: "MAX_TOOL_CALLS_EXCEEDED"
});
expect(lookup).not.toHaveBeenCalled();
expect(disabled.getToolCallCount("zero")).toBe(0);
const limited = new ActionExecutor(registry, 1);
await limited.execute("ping", {}, context("full"));
await expect(
limited.execute("missing", {}, context("full"))
).rejects.toMatchObject({
code: "MAX_TOOL_CALLS_EXCEEDED"
});
expect(lookup).toHaveBeenCalledTimes(1);
});

it("retains the numeric fourth argument, FIFO eviction, and reset APIs", async () => {
const executor = new ActionExecutor(registryWithPing(), 1, undefined, 2);
await executor.execute("ping", {}, context("a"));
await executor.execute("ping", {}, context("b"));
await executor.execute("ping", {}, context("c"));
expect(executor.getToolCallCount("a")).toBe(0);
expect(executor.getToolCallCount("b")).toBe(1);
expect(executor.getToolCallCount("c")).toBe(1);
executor.reset("b");
await expect(executor.execute("ping", {}, context("b"))).resolves.toBe(
"pong"
);
executor.resetAll();
expect(executor.getToolCallCount("b")).toBe(0);
expect(executor.getToolCallCount("c")).toBe(0);
});

it.each([0, -1, 1.5, Number.NaN])(
"rejects invalid retention cap %s",
(cap) => {
expect(
() => new ActionExecutor(registryWithPing(), undefined, undefined, cap)
).toThrow("maxTrackedTasks must be a positive integer.");
}
);
});
Loading