Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
@@ -0,0 +1,111 @@
# Configurable Room Turn Timeout

## Summary

OpenMausBot currently stops every room member turn after five minutes, even when the engine is still producing output. The duration and the error message are hard-coded in `server/index.ts`. This behavior is separate from the activity-based turn stall watchdog controlled by `OMB_TURN_STALL_MS`.

Add one global, persisted room turn timeout setting. Keep five minutes as the default, expose the setting in the existing General settings UI, and use the configured value for room turns started after the setting is saved.

## Goals

- Let users configure the maximum duration of a room member turn.
- Preserve the current five-minute behavior for existing installations.
- Make the setting discoverable and editable in the existing app settings UI.
- Apply updates without restarting the server or reloading providers.
- Report the configured duration in timeout activity messages.
- Keep the room turn ceiling distinct from the inactivity-based turn stall watchdog.

## Non-goals

- Per-room or per-bot timeout overrides.
- Changing `OMB_TURN_STALL_MS` or the semantics of the stall watchdog.
- Changing provider-specific approval or RPC timeouts.
- Retiming room turns that are already running when the setting changes.
- Adding an environment-variable override for the room turn ceiling.

## Configuration Model

Add a `rooms` section to the persisted application configuration:

```json
{
"rooms": {
"turnTimeoutMinutes": 5
}
}
```

`turnTimeoutMinutes` is a whole number from 1 through 1,440. Missing values resolve to 5, so existing configuration files retain the current behavior. Stored configuration and API patches reject non-numeric, fractional, out-of-range, and structurally invalid values.

The public config status includes the effective value because it is non-secret:

```json
{
"rooms": {
"turnTimeoutMinutes": 5
}
}
```

Saving only this section must not reload providers or interrupt active turns. The server updates its in-memory application config and broadcasts the new config status through the existing config event.

## Server Behavior

When a room member turn is dispatched, the server reads the effective global timeout and captures it for that turn. The timer uses that captured value, so changing the setting affects the next room turn and does not silently move the deadline of a turn already in progress.

On timeout, the server keeps the existing interruption and room ownership behavior. Only the timer duration and activity text become dynamic. The message uses readable singular and plural forms, for example:

- `Atlas's room turn exceeded 1 minute and was stopped`
- `Atlas's room turn exceeded 20 minutes and was stopped`

The turn stall watchdog remains activity-based and independent. A room turn can therefore stop because it reaches the configured absolute ceiling or because it becomes inactive long enough for the existing watchdog to fire.

## User Interface

Add a `Room turns` card to `Settings > General`, alongside the existing global settings cards. The card follows the current `Card` and input styles instead of introducing a new settings pattern.

The card contains:

- A `Maximum turn length` label.
- A numeric input showing the current value.
- A `minutes` suffix.
- Supporting text explaining that the limit applies to every bot turn in rooms and that direct chats use the inactivity watchdog instead.

The field accepts whole minutes from 1 through 1,440. It saves on blur, matching the Profile fields. Pressing Enter blurs the field and saves. Invalid input remains visible with the existing danger color treatment, shows a concise inline validation message, and is not sent to the server. A failed save also keeps the entered value visible and reports the server error inline.

When a config status update arrives, the field synchronizes to the server value unless the user is actively editing it. This prevents a stale config event from replacing in-progress input.

## Data Flow

1. `GET /api/config` returns `rooms.turnTimeoutMinutes` with an effective default of 5.
2. The app store hydrates and folds config events with the `rooms` status included.
3. The General settings card edits the value and sends `PUT /api/config` with only the `rooms` patch.
4. The server validates and persists the patch, updates the live config object, and broadcasts the resulting config status.
5. Each new room member turn captures the effective duration and starts its absolute timeout timer.
6. If the timer fires first, the server interrupts the provider and records the dynamic timeout activity message.

## Error Handling

- Client-side validation prevents empty, fractional, non-numeric, and out-of-range values from being submitted.
- Server-side schema validation remains authoritative and returns HTTP 400 for invalid patches.
- Save failures appear next to the field without changing the last confirmed setting in application state.
- Existing room timeout cleanup and ownership safeguards remain unchanged.

## Testing

Add focused coverage for:

- Stored configuration parsing with a valid room timeout.
- Defaulting missing room settings to five minutes in config status.
- Rejecting malformed and out-of-range room timeout patches.
- Persisting and returning an updated room timeout through `/api/config`.
- Folding the `rooms` section from config events into client state.
- Client validation and save behavior for the General settings field.
- The room turn timer using a configurable duration and formatting singular and plural timeout messages.
- Existing behavior remaining at five minutes when no setting is present.

Run the focused tests first, followed by the repository typecheck, lint, and relevant full test suite before publishing the pull request.

## Pull Request Scope

The pull request will contain only the configuration contract, room timeout behavior, General settings UI, focused tests, and supporting documentation needed for this change. The pull request title, body, commits, code comments, UI copy, and tests will be written in English.
18 changes: 18 additions & 0 deletions server/config.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import {
loadConfig,
parseConfigPatch,
parseStoredConfig,
roomTurnTimeoutMinutes,
stripWorkspaceCredentialEnv,
syncCredentialEnv,
vpsSshAlias,
Expand Down Expand Up @@ -48,6 +49,23 @@ describe("configuration boundaries", () => {
expect(vpsSshAlias({ vps: { sshAlias: "production-vps" } })).toBe("production-vps");
expect(vpsSshAlias({ vps: { sshAlias: "-bad" } })).toBeNull();
});

it("accepts a persisted global room turn timeout and supplies the legacy default", () => {
expect(parseStoredConfig({ rooms: { turnTimeoutMinutes: 20 } })).toEqual({
rooms: { turnTimeoutMinutes: 20 },
});
expect(roomTurnTimeoutMinutes({ rooms: { turnTimeoutMinutes: 20 } })).toBe(20);
expect(roomTurnTimeoutMinutes({})).toBe(5);
});

it.each([0, 1.5, 1441, "20", null])(
"rejects an invalid room turn timeout: %j",
(turnTimeoutMinutes) => {
expect(() => parseConfigPatch({ rooms: { turnTimeoutMinutes } })).toThrow(
"rooms.turnTimeoutMinutes",
);
},
);
});

describe("default fleet", () => {
Expand Down
19 changes: 18 additions & 1 deletion server/config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,10 @@ import { parseJson, schemaIssue, type JsonObject, type JsonValue } from "./schem
const optionalText = z.string().optional();
const SSH_ALIAS = /^[A-Za-z0-9][A-Za-z0-9_.-]{0,127}$/;

export const DEFAULT_ROOM_TURN_TIMEOUT_MINUTES = 5;
export const MIN_ROOM_TURN_TIMEOUT_MINUTES = 1;
export const MAX_ROOM_TURN_TIMEOUT_MINUTES = 1_440;

export function isValidSshAlias(value: unknown): value is string {
return typeof value === "string" && SSH_ALIAS.test(value);
}
Expand All @@ -36,6 +40,13 @@ const vpsConfigSchema = z.object({
message: "must be a simple SSH config alias",
}).optional(),
});
const roomConfigSchema = z.object({
turnTimeoutMinutes: z
.number()
.int()
.min(MIN_ROOM_TURN_TIMEOUT_MINUTES)
.max(MAX_ROOM_TURN_TIMEOUT_MINUTES),
});
const instanceConfigSchema = z.object({
driver: z.string().min(1),
displayName: optionalText,
Expand All @@ -58,6 +69,7 @@ const appConfigSchema = z.object({
tts: z.object({ key: optionalText, voice: optionalText }).optional(),
/** Non-secret profile details shown in the sidebar. */
profile: z.object({ name: optionalText, email: optionalText }).optional(),
rooms: roomConfigSchema.optional(),
instances: instanceConfigMapSchema.optional(),
});
const appConfigPatchSchema = appConfigSchema.omit({ instances: true });
Expand All @@ -72,6 +84,7 @@ export interface AppConfig {
opencodeGo?: { apiKey?: string };
tts?: { key?: string; voice?: string };
profile?: { name?: string; email?: string };
rooms?: { turnTimeoutMinutes: number };
instances?: InstanceConfigMap;
}
export type ConfigPatch = z.output<typeof appConfigPatchSchema>;
Expand All @@ -94,6 +107,10 @@ export function vpsSshAlias(cfg: AppConfig): string | null {
return isValidSshAlias(cfg.vps?.sshAlias) ? cfg.vps.sshAlias : null;
}

export function roomTurnTimeoutMinutes(cfg: AppConfig): number {
return cfg.rooms?.turnTimeoutMinutes ?? DEFAULT_ROOM_TURN_TIMEOUT_MINUTES;
}

// OMB_DATA_DIR isolates test/soak rigs from the user's real fleet.
export const DATA_DIR = process.env.OMB_DATA_DIR ?? join(homedir(), ".openmausbot");
const LEGACY_DATA_DIR = join(homedir(), ".opengrokbot");
Expand Down Expand Up @@ -193,7 +210,7 @@ export function saveConfig(patch: Partial<AppConfig>): void {
/* first write */
}
const checkedPatch = appConfigSchema.partial().parse(patch);
for (const key of ["xai", "composio", "box", "opencodeGo", "tts", "profile"] as const) {
for (const key of ["xai", "composio", "box", "opencodeGo", "tts", "profile", "rooms"] as const) {
const section = checkedPatch[key];
if (!section) continue;
const current = jsonObjectSchema.safeParse(disk[key]);
Expand Down
96 changes: 91 additions & 5 deletions server/index.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
// the shadow-instance behavior end to end while it's at it.
import { spawn, type ChildProcess } from "node:child_process";
import { createServer, request, type Server } from "node:http";
import { mkdirSync, mkdtempSync, readFileSync, statSync, writeFileSync } from "node:fs";
import { existsSync, mkdirSync, mkdtempSync, readFileSync, rmSync, statSync, writeFileSync } from "node:fs";
import { tmpdir } from "node:os";
import { dirname, join } from "node:path";
import { fileURLToPath } from "node:url";
Expand All @@ -16,6 +16,7 @@ import { openSse } from "./testing/sse.ts";

const SERVER_DIR = dirname(fileURLToPath(import.meta.url));
const ROOT = join(SERVER_DIR, "..");
const FAKE_CLAUDE_CLI = join(SERVER_DIR, "testing", "fake-claude-cli.ts");
const PORT = 18800 + Math.floor(Math.random() * 10_000);
const BASE = `http://127.0.0.1:${PORT}`;
const WEBHOOK_PORT = 39000 + Math.floor(Math.random() * 10_000);
Expand All @@ -27,6 +28,7 @@ let boxStub: Server;
let boxStubPort = 0;
let home: string;
let staticDir: string;
let fakeClaudeDump: string;
let stderr = "";

const api = async (method: string, path: string, body?: unknown): Promise<{ status: number; body: any }> => {
Expand All @@ -51,14 +53,20 @@ const statusWithHeaders = (headers: Record<string, string>): Promise<number> =>
beforeAll(async () => {
home = mkdtempSync(join(tmpdir(), "omb-api-test-"));
staticDir = join(home, "static");
fakeClaudeDump = join(home, "fake-claude-dump.json");
// a fleet of exactly one unknown driver: no CLI probes, no network
mkdirSync(join(home, ".openmausbot"), { recursive: true });
mkdirSync(join(staticDir, "assets"), { recursive: true });
writeFileSync(join(staticDir, "index.html"), "<!doctype html><title>Packaged OpenMausBot</title>");
writeFileSync(join(staticDir, "assets", "smoke.css"), "body { color: white; }");
writeFileSync(
join(home, ".openmausbot", "config.json"),
JSON.stringify({ instances: { ghost: { driver: "not-a-real-driver", displayName: "Ghost" } } }),
JSON.stringify({
instances: {
ghost: { driver: "not-a-real-driver", displayName: "Ghost" },
claude: { driver: "claudeAgent", displayName: "Fixture Claude", config: { cli: FAKE_CLAUDE_CLI } },
},
}),
);
writeFileSync(
join(home, ".openmausbot", "groups.json"),
Expand Down Expand Up @@ -163,6 +171,8 @@ beforeAll(async () => {
OMB_BOX_API: `http://127.0.0.1:${boxStubPort}`,
OMB_COMPOSIO_API: `http://127.0.0.1:${boxStubPort}/api/v3.1`,
OMB_STATIC_DIR: staticDir,
FAKE_CLAUDE_MODE: "hang",
FAKE_CLAUDE_DUMP: fakeClaudeDump,
},
stdio: ["ignore", "pipe", "pipe"],
});
Expand Down Expand Up @@ -277,14 +287,19 @@ describe("harness HTTP API", () => {
it("describes the configured fleet, shadows included", async () => {
const { status, body } = await api("GET", "/api/instances");
expect(status).toBe(200);
expect(body.instances).toHaveLength(1);
expect(body.instances[0]).toMatchObject({
const ghost = body.instances.find((instance: { instanceId: string }) => instance.instanceId === "ghost");
expect(ghost).toMatchObject({
instanceId: "ghost",
driverKind: "not-a-real-driver",
displayName: "Ghost",
snapshot: { state: "unavailable" },
});
expect(body.instances[0].snapshot.reason).toContain("not-a-real-driver");
expect(ghost.snapshot.reason).toContain("not-a-real-driver");
expect(body.instances).toContainEqual(expect.objectContaining({
instanceId: "claude",
driverKind: "claudeAgent",
displayName: "Fixture Claude",
}));
});

it("searches transcripts and exports a conversation", async () => {
Expand Down Expand Up @@ -832,6 +847,77 @@ describe("harness HTTP API", () => {
expect(nothing.status).toBe(400);
});

it("validates and persists the global room turn timeout", async () => {
const before = await api("GET", "/api/config");
expect(before.status).toBe(200);
expect(before.body.rooms).toEqual({ turnTimeoutMinutes: 5 });

for (const turnTimeoutMinutes of [0, 1.5, 1441, "20", null]) {
const invalid = await api("PUT", "/api/config", { rooms: { turnTimeoutMinutes } });
expect(invalid.status).toBe(400);
expect(invalid.body.error).toContain("rooms.turnTimeoutMinutes");
}

const saved = await api("PUT", "/api/config", { rooms: { turnTimeoutMinutes: 20 } });
expect(saved.status).toBe(200);
expect(saved.body.rooms).toEqual({ turnTimeoutMinutes: 20 });

const after = await api("GET", "/api/config");
expect(after.body.rooms).toEqual({ turnTimeoutMinutes: 20 });

const disk = JSON.parse(readFileSync(join(home, ".openmausbot", "config.json"), "utf8"));
expect(disk.rooms).toEqual({ turnTimeoutMinutes: 20 });

await api("PUT", "/api/config", { rooms: { turnTimeoutMinutes: 5 } });
});

it("keeps an active turn alive when only the room timeout changes", async () => {
const created = await api("POST", "/api/bots", {});
const botId = created.body.bot.id;
const room = (await api("POST", "/api/groups", {
name: "Room timeout capture",
memberIds: [botId],
})).body.group;
try {
const selected = await api("PATCH", `/api/bots/${botId}`, {
modelSelection: { instanceId: "claude", model: "claude-sonnet-5" },
});
expect(selected.status).toBe(200);

rmSync(fakeClaudeDump, { force: true });
const sent = await api("POST", `/api/groups/${room.id}/messages`, { text: "stay active" });
expect(sent.status).toBe(202);
await expect.poll(() => existsSync(fakeClaudeDump), { timeout: 5_000 }).toBe(true);

const before = (await api("GET", "/api/bots")).body;
expect(before.bots.find((bot: { id: string }) => bot.id === botId)?.busy).toBe(true);
expect(before.groups.find((group: { id: string }) => group.id === room.id)?.busyBotId).toBe(botId);

const saved = await api("PUT", "/api/config", { rooms: { turnTimeoutMinutes: 20 } });
expect(saved.status).toBe(200);

const after = (await api("GET", "/api/bots")).body;
expect(after.bots.find((bot: { id: string }) => bot.id === botId)?.busy).toBe(true);
const activeRoom = after.groups.find((group: { id: string }) => group.id === room.id);
expect(activeRoom?.busyBotId).toBe(botId);
expect(activeRoom.messages.some((message: { tool?: { name?: string } }) =>
message.tool?.name?.includes("provider settings changed"),
)).toBe(false);
} finally {
await api("POST", `/api/groups/${room.id}/interrupt`);
await expect.poll(async () => {
const state = (await api("GET", "/api/bots")).body;
return {
botBusy: state.bots.find((bot: { id: string }) => bot.id === botId)?.busy,
roomBusyBotId: state.groups.find((group: { id: string }) => group.id === room.id)?.busyBotId,
};
}, { timeout: 5_000 }).toEqual({ botBusy: false, roomBusyBotId: null });
await api("DELETE", `/api/groups/${room.id}`);
await api("DELETE", `/api/bots/${botId}`);
await api("PUT", "/api/config", { rooms: { turnTimeoutMinutes: 5 } });
}
});
Comment thread
coderabbitai[bot] marked this conversation as resolved.

it("validates the non-secret VPS alias and keeps old bots on Box by default", async () => {
const before = await api("GET", "/api/bots");
const bot = before.body.bots[0];
Expand Down
Loading
Loading