Skip to content

Commit 983bcbf

Browse files
committed
codex: address PR review feedback (#14618)
1 parent e32cce2 commit 983bcbf

9 files changed

Lines changed: 108 additions & 5 deletions

File tree

‎apps/server/src/mcp/toolkits/scheduling/tools.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,7 @@ export const SchedulingToolkit = Toolkit.make(
2929
.annotate(Tool.Idempotent, false),
3030
Tool.make("list_scheduled_prompts", {
3131
description:
32-
"List the latest 100 schedules in this chat, including their IDs, next run, status, last run, and any delivery error. Use to inspect scheduled work before changing it.",
32+
"List all active and paused schedules in this chat, plus the latest 100 completed or cancelled schedules, including their IDs, next run, status, last run, and any delivery error. Use to inspect scheduled work before changing it.",
3333
success: Schema.Struct({ schedules: Schema.Array(ScheduledPrompt) }),
3434
failure,
3535
dependencies,

‎apps/server/src/orchestration/decider.ts‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1404,6 +1404,7 @@ export const decideOrchestrationCommand = Effect.fn("decideOrchestrationCommand"
14041404
(targetThread.archivedAt !== null ||
14051405
targetThread.latestTurn?.state === "running" ||
14061406
targetThread.session?.status === "starting" ||
1407+
targetThread.session?.status === "running" ||
14071408
(targetThread.session?.activeTurnId !== null &&
14081409
targetThread.session?.activeTurnId !== undefined) ||
14091410
openRequests(targetThread).size > 0 ||

‎apps/server/src/orchestration/decider.userMessageAppend.test.ts‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -152,8 +152,12 @@ it.effect("scheduled turn starts cannot steer running or queued work", () =>
152152
updatedAt: createdAt,
153153
},
154154
};
155+
const runningWithoutTurn = {
156+
...starting,
157+
session: { ...starting.session, status: "running" as const },
158+
};
155159
const archived = { ...target, archivedAt: createdAt };
156-
for (const blocked of [starting, archived]) {
160+
for (const blocked of [starting, runningWithoutTurn, archived]) {
157161
const error = yield* decideOrchestrationCommand({
158162
readModel: { ...readModel, threads: [blocked] },
159163
command: { ...turnStartCommand, requireIdleAt: createdAt },

‎apps/server/src/scheduling/ScheduledPrompts.test.ts‎

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -328,3 +328,31 @@ it.effect("agents can create, inspect, and cancel a schedule through MCP", () =>
328328
expect(commands).toHaveLength(0);
329329
}).pipe(provide),
330330
);
331+
332+
it.effect("lists older live schedules even after more than 100 terminal schedules", () =>
333+
Effect.gen(function* () {
334+
const { schedules } = yield* harness;
335+
const active = yield* schedules.create(threadId, {
336+
prompt: "Keep checking",
337+
schedule: { kind: "interval", seconds: 60 },
338+
});
339+
const paused = yield* schedules.create(threadId, {
340+
prompt: "Paused check",
341+
schedule: { kind: "interval", seconds: 60 },
342+
});
343+
yield* schedules.update(threadId, paused.id, "pause");
344+
yield* TestClock.adjust("1 second");
345+
for (let index = 0; index < 105; index++) {
346+
const terminal = yield* schedules.create(threadId, {
347+
prompt: "Cancelled check",
348+
schedule: { kind: "delay", seconds: 60 },
349+
});
350+
yield* schedules.update(threadId, terminal.id, "cancel");
351+
}
352+
const listed = yield* schedules.list(threadId);
353+
expect(listed).toHaveLength(102);
354+
expect(listed).toContainEqual(expect.objectContaining({ id: active.id, status: "active" }));
355+
expect(listed).toContainEqual(expect.objectContaining({ id: paused.id, status: "paused" }));
356+
expect(listed.filter((schedule) => schedule.status === "cancelled")).toHaveLength(100);
357+
}).pipe(provide),
358+
);

‎apps/server/src/scheduling/ScheduledPrompts.ts‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -133,7 +133,12 @@ export const make = Effect.gen(function* () {
133133

134134
const list = (threadId: ThreadId) =>
135135
locked(
136-
sql<Row>`SELECT * FROM scheduled_prompts WHERE thread_id = ${threadId} ORDER BY created_at DESC LIMIT 100`.pipe(
136+
sql<Row>`WITH terminal AS (
137+
SELECT * FROM scheduled_prompts WHERE thread_id = ${threadId} AND status IN ('completed', 'cancelled')
138+
ORDER BY created_at DESC LIMIT 100
139+
)
140+
SELECT * FROM scheduled_prompts WHERE thread_id = ${threadId} AND status IN ('active', 'paused')
141+
UNION ALL SELECT * FROM terminal ORDER BY created_at DESC`.pipe(
137142
Effect.map((rows) => rows.map(fromRow)),
138143
),
139144
);
@@ -178,6 +183,7 @@ export const make = Effect.gen(function* () {
178183
dispatchedThreads.has(row.thread_id) ||
179184
value.latestTurn?.state === "running" ||
180185
value.session?.status === "starting" ||
186+
value.session?.status === "running" ||
181187
value.session?.activeTurnId ||
182188
value.hasPendingApprovals ||
183189
value.hasPendingUserInput ||

‎apps/server/src/server.test.ts‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -110,6 +110,7 @@ import * as BackgroundPolicy from "./background/BackgroundPolicy.ts";
110110
import * as ServerConfig from "./config.ts";
111111
import * as DeviceService from "./device/DeviceService.ts";
112112
import { HTTP_ROUTER_CONFIG, makeRoutesLayer } from "./server.ts";
113+
import * as ScheduledPrompts from "./scheduling/ScheduledPrompts.ts";
113114
import {
114115
isThreadDetailEvent,
115116
resolveAvailableEditorsForConfig,
@@ -772,6 +773,7 @@ const buildAppUnderTest = (options?: {
772773
// Viewed-file marks for a host that keeps none of its own are rows, so the routes want a
773774
// database. Its own, in memory: nothing here shares a table with the auth store.
774775
makeRoutesLayer.pipe(
776+
Layer.provide(ScheduledPrompts.layer),
775777
Layer.provide(Layer.mergeAll(serviceLauncherClientLayer, SqlitePersistenceMemory)),
776778
),
777779
{

‎apps/server/src/server.ts‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -617,7 +617,6 @@ export const makeRoutesLayer = Layer.mergeAll(
617617
// Last, so no route layer can replace the server's one TracerDisabledWhen.
618618
untracedRequestsLayer,
619619
).pipe(
620-
Layer.provide(ScheduledPrompts.layer),
621620
// Both transports consume the same service instance, so caches single-flight across clients
622621
// and mutations observed on WebSocket invalidate patches subsequently read over HTTP.
623622
Layer.provide(PullRequestServiceLive),
Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,49 @@
1+
import { expect, it } from "@effect/vitest";
2+
import * as Schema from "effect/Schema";
3+
import { CreateScheduledPromptInput } from "./scheduling.ts";
4+
5+
const decode = Schema.decodeSync(CreateScheduledPromptInput);
6+
7+
it("rejects impossible calendar dates for both exact starts and interval starts", () => {
8+
for (const at of [
9+
"2026-02-30T09:00:00Z",
10+
"2025-02-29T09:00:00Z",
11+
"2100-02-29T09:00:00Z",
12+
"2026-04-31T09:00:00+02:00",
13+
"2026-00-01T09:00:00Z",
14+
"2026-01-00T09:00:00Z",
15+
]) {
16+
expect(() => decode({ prompt: "Check", schedule: { kind: "once", at } })).toThrow();
17+
expect(() =>
18+
decode({ prompt: "Check", schedule: { kind: "interval", seconds: 60, startAt: at } }),
19+
).toThrow();
20+
}
21+
});
22+
23+
it("rejects invalid clocks, offsets, and timestamps without an offset", () => {
24+
for (const at of [
25+
"2026-10-01T24:00:00Z",
26+
"2026-10-01T09:60:00Z",
27+
"2026-10-01T09:00:60Z",
28+
"2026-10-01T09:00:00+24:00",
29+
"2026-10-01T09:00:00+02:60",
30+
"2026-10-01T09:00:00",
31+
"garbageTZ",
32+
]) {
33+
expect(() => decode({ prompt: "Check", schedule: { kind: "once", at } })).toThrow();
34+
}
35+
});
36+
37+
it("accepts leap days and explicit offsets without changing the input", () => {
38+
for (const at of [
39+
"2024-02-29T09:00:00Z",
40+
"2000-02-29T09:00:00Z",
41+
"2026-10-01T09:00:00.123+02:00",
42+
"2026-10-01T09:00:00-07:00",
43+
]) {
44+
expect(decode({ prompt: "Check", schedule: { kind: "once", at } }).schedule).toEqual({
45+
kind: "once",
46+
at,
47+
});
48+
}
49+
});

‎packages/contracts/src/scheduling.ts‎

Lines changed: 15 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,21 @@
11
import * as Schema from "effect/Schema";
22
import { IsoDateTime, TrimmedNonEmptyString } from "./baseSchemas.ts";
33

4-
const Timestamp = IsoDateTime.check(Schema.isPattern(/T.*(?:Z|[+-]\d{2}:\d{2})$/));
4+
const timestampPattern =
5+
/^(\d{4})-(0[1-9]|1[0-2])-(0[1-9]|[12]\d|3[01])T(?:[01]\d|2[0-3]):[0-5]\d:[0-5]\d(?:\.\d{1,3})?(?:Z|[+-](?:[01]\d|2[0-3]):[0-5]\d)$/;
6+
const Timestamp = IsoDateTime.check(
7+
Schema.makeFilter((value) => {
8+
const parts = timestampPattern.exec(value);
9+
if (parts === null)
10+
return "Use a valid calendar timestamp with seconds and an explicit UTC offset.";
11+
const year = Number(parts[1]);
12+
const month = Number(parts[2]);
13+
const day = Number(parts[3]);
14+
const leapYear = year % 4 === 0 && (year % 100 !== 0 || year % 400 === 0);
15+
const daysInMonth = [31, leapYear ? 29 : 28, 31, 30, 31, 30, 31, 31, 30, 31, 30, 31];
16+
return day <= daysInMonth[month - 1]! || "The timestamp contains an impossible calendar date.";
17+
}),
18+
);
519

620
const Seconds = Schema.Int.check(Schema.isBetween({ minimum: 60, maximum: 31_536_000 }));
721

0 commit comments

Comments
 (0)