Skip to content

Commit 0e8215f

Browse files
backnotpropbacknotprop
committed
Fix daemon review regressions
1 parent 539a612 commit 0e8215f

13 files changed

Lines changed: 153 additions & 59 deletions

File tree

‎apps/hook/server/index.ts‎

Lines changed: 23 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -467,10 +467,6 @@ async function ensureDaemonClient(options: { pluginError?: boolean } = {}) {
467467
fail("daemon-start-failed", "Timed out waiting for the Plannotator daemon to start.");
468468
}
469469

470-
async function runDaemonSessionRequest(request: PluginRequest): Promise<{
471-
result: PluginActionResult;
472-
session: PluginSessionInfo;
473-
}>;
474470
function registerDaemonSessionInterruptCleanup(cancelSession: () => Promise<void>): () => void {
475471
let cancelling = false;
476472
const handleSignal = (exitCode: number) => {
@@ -488,13 +484,26 @@ function registerDaemonSessionInterruptCleanup(cancelSession: () => Promise<void
488484
};
489485
}
490486

487+
async function withProcessCwd<T>(cwd: string | undefined, fn: () => Promise<T>): Promise<T> {
488+
if (!cwd) return fn();
489+
const original = process.cwd();
490+
const target = path.resolve(cwd);
491+
if (target === original) return fn();
492+
process.chdir(target);
493+
try {
494+
return await fn();
495+
} finally {
496+
process.chdir(original);
497+
}
498+
}
499+
491500
async function runDaemonSessionRequest(request: PluginRequest, options: { pluginError?: boolean } = {}): Promise<{
492501
result: PluginActionResult;
493502
session: PluginSessionInfo;
494503
}> {
495504
const fail = options.pluginError ? emitPluginError : emitCommandError;
496505
const daemon = await ensureDaemonClient(options);
497-
const created = await daemon.createSession({ request, wait: true });
506+
const created = await daemon.createSession({ request });
498507
if (created.ok !== true) {
499508
fail(created.error.code, created.error.message);
500509
}
@@ -520,13 +529,15 @@ async function runDaemonSessionRequest(request: PluginRequest, options: { plugin
520529
}
521530

522531
try {
523-
if (request.action === "review") {
524-
await handleReviewServerReady(created.session.url, daemon.state.isRemote, sessionPort);
525-
} else if (request.action === "annotate" || request.action === "annotate-last") {
526-
await handleAnnotateServerReady(created.session.url, daemon.state.isRemote, sessionPort);
527-
} else {
528-
await handleServerReady(created.session.url, daemon.state.isRemote, sessionPort);
529-
}
532+
await withProcessCwd(request.cwd, async () => {
533+
if (request.action === "review") {
534+
await handleReviewServerReady(created.session.url, daemon.state.isRemote, sessionPort);
535+
} else if (request.action === "annotate" || request.action === "annotate-last") {
536+
await handleAnnotateServerReady(created.session.url, daemon.state.isRemote, sessionPort);
537+
} else {
538+
await handleServerReady(created.session.url, daemon.state.isRemote, sessionPort);
539+
}
540+
});
530541

531542
const completed = await daemon.waitForResult<PluginActionResult>(created.session.id);
532543
if (completed.ok !== true) {

‎packages/server/daemon/client.test.ts‎

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -78,17 +78,19 @@ describe("DaemonClient", () => {
7878
const paths = getDaemonPaths({ baseDir });
7979
writeDaemonState(state(), { baseDir });
8080
writeFileSync(paths.lockPath, "123\n", "utf-8");
81-
const calls: string[] = [];
81+
const calls: Request[] = [];
8282

8383
await cleanupDaemonState(state(), {
8484
baseDir,
85-
fetch: async (input) => {
86-
calls.push(String(input));
85+
fetch: async (input, init) => {
86+
calls.push(new Request(input, init));
8787
return Response.json({ ok: true });
8888
},
8989
});
9090

91-
expect(calls).toEqual(["http://localhost:4321/daemon/shutdown"]);
91+
expect(calls.map((call) => call.url)).toEqual(["http://localhost:4321/daemon/shutdown"]);
92+
expect(calls[0].headers.get("content-type")).toBe("application/json");
93+
expect(await calls[0].text()).toBe("{}");
9294
expect(existsSync(paths.statePath)).toBe(false);
9395
expect(existsSync(paths.lockPath)).toBe(false);
9496
});

‎packages/server/daemon/client.ts‎

Lines changed: 13 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -54,11 +54,17 @@ export class DaemonClient {
5454
}
5555

5656
async cancelSession(id: string): Promise<DaemonCancelSessionResponse | DaemonErrorResponse> {
57-
return this.requestJson(`/daemon/sessions/${encodeURIComponent(id)}/cancel`, { method: "POST" }) as Promise<DaemonCancelSessionResponse | DaemonErrorResponse>;
57+
return this.requestJson(`/daemon/sessions/${encodeURIComponent(id)}/cancel`, {
58+
method: "POST",
59+
body: "{}",
60+
}) as Promise<DaemonCancelSessionResponse | DaemonErrorResponse>;
5861
}
5962

6063
async shutdown(): Promise<DaemonShutdownResponse | DaemonErrorResponse> {
61-
return this.requestJson("/daemon/shutdown", { method: "POST" }) as Promise<DaemonShutdownResponse | DaemonErrorResponse>;
64+
return this.requestJson("/daemon/shutdown", {
65+
method: "POST",
66+
body: "{}",
67+
}) as Promise<DaemonShutdownResponse | DaemonErrorResponse>;
6268
}
6369

6470
private async getJson(path: string): Promise<unknown> {
@@ -91,7 +97,11 @@ export async function cleanupDaemonState(state: unknown, options: DaemonClientOp
9197
const baseUrl = stateBaseUrl(state);
9298
if (baseUrl) {
9399
try {
94-
await fetchImpl(`${baseUrl}/daemon/shutdown`, { method: "POST" });
100+
await fetchImpl(`${baseUrl}/daemon/shutdown`, {
101+
method: "POST",
102+
headers: { "content-type": "application/json" },
103+
body: "{}",
104+
});
95105
} catch {
96106
// Best effort only. Do not signal the recorded PID here; stale daemon
97107
// state can outlive the process and the PID may now belong to something else.

‎packages/server/daemon/runtime.test.ts‎

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -88,8 +88,15 @@ describe("startDaemonRuntime", () => {
8888
}),
8989
});
9090

91-
const res = await fetch(`${runtime.state.baseUrl}/daemon/shutdown`, { method: "POST" });
91+
const res = await fetch(`${runtime.state.baseUrl}/daemon/shutdown`, {
92+
method: "POST",
93+
headers: { "content-type": "application/json" },
94+
body: "{}",
95+
});
9296
expect((await res.json()).shuttingDown).toBe(true);
97+
for (let attempt = 0; attempt < 20 && readDaemonState({ baseDir }).kind !== "missing"; attempt++) {
98+
await Bun.sleep(10);
99+
}
93100
expect(readDaemonState({ baseDir }).kind).toBe("missing");
94101
});
95102
});

‎packages/server/daemon/runtime.ts‎

Lines changed: 12 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -42,30 +42,21 @@ export async function startDaemonRuntime(options: StartDaemonRuntimeOptions): Pr
4242
let runtime: DaemonRuntime | undefined;
4343
let cleanupTimer: ReturnType<typeof setInterval> | undefined;
4444
let server: ReturnType<typeof Bun.serve> | undefined;
45+
let handler: ReturnType<typeof createDaemonFetchHandler> | undefined;
4546

4647
try {
47-
let state: DaemonState | undefined;
4848
server = Bun.serve({
4949
hostname,
5050
port: requestedPort,
5151
fetch: (req, server) => {
52-
if (!state) return new Response("Daemon is starting", { status: 503 });
53-
const handler = createDaemonFetchHandler({
54-
state,
55-
store,
56-
createSession: options.createSession,
57-
onShutdown: async () => {
58-
await runtime?.stop();
59-
await options.onShutdown?.();
60-
},
61-
});
52+
if (!handler) return new Response("Daemon is starting", { status: 503 });
6253
return handler(req, {
6354
disableIdleTimeout: () => server.timeout(req, 0),
6455
});
6556
},
6657
});
6758

68-
state = createDaemonState({
59+
const state = createDaemonState({
6960
port: server.port!,
7061
hostname,
7162
isRemote,
@@ -74,6 +65,15 @@ export async function startDaemonRuntime(options: StartDaemonRuntimeOptions): Pr
7465
requestedPort,
7566
});
7667
writeDaemonState(state, options);
68+
handler = createDaemonFetchHandler({
69+
state,
70+
store,
71+
createSession: options.createSession,
72+
onShutdown: async () => {
73+
await runtime?.stop();
74+
await options.onShutdown?.();
75+
},
76+
});
7777
cleanupTimer = setInterval(() => {
7878
void store.cleanupExpired();
7979
}, 60_000);

‎packages/server/daemon/server.test.ts‎

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -166,6 +166,8 @@ describe("daemon HTTP router", () => {
166166
}));
167167
const cancel = await handler(new Request("http://127.0.0.1:4321/daemon/sessions/s1/cancel", {
168168
method: "POST",
169+
headers: { "content-type": "application/json" },
170+
body: "{}",
169171
}));
170172
expect((await cancel.json()).session.status).toBe("cancelled");
171173

@@ -195,4 +197,25 @@ describe("daemon HTTP router", () => {
195197
expect(timeoutDisabled).toBe(1);
196198
expect(body.result.approved).toBe(true);
197199
});
200+
201+
test("rejects simple POST control requests without JSON content type", async () => {
202+
const { handler } = makeHandler();
203+
await handler(new Request("http://127.0.0.1:4321/daemon/sessions", {
204+
method: "POST",
205+
headers: { "content-type": "application/json" },
206+
body: JSON.stringify({ request: { action: "plan", origin: "opencode", plan: "x" } }),
207+
}));
208+
209+
const cancel = await handler(new Request("http://127.0.0.1:4321/daemon/sessions/s1/cancel", {
210+
method: "POST",
211+
}));
212+
const shutdown = await handler(new Request("http://127.0.0.1:4321/daemon/shutdown", {
213+
method: "POST",
214+
}));
215+
216+
expect(cancel.status).toBe(415);
217+
expect((await cancel.json()).error.code).toBe("invalid-request");
218+
expect(shutdown.status).toBe(415);
219+
expect((await shutdown.json()).error.code).toBe("invalid-request");
220+
});
198221
});

‎packages/server/daemon/server.ts‎

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -144,6 +144,9 @@ export function createDaemonFetchHandler(options: DaemonServerOptions) {
144144
}
145145

146146
if (action === "cancel" && req.method === "POST") {
147+
if (!isJsonRequest(req)) {
148+
return json(createDaemonErrorResponse("invalid-request", "Daemon cancel requests must use application/json."), { status: 415 });
149+
}
147150
const cancelled = store.cancel(id);
148151
return json({ ok: true, session: store.summary(cancelled ?? record) });
149152
}
@@ -155,7 +158,13 @@ export function createDaemonFetchHandler(options: DaemonServerOptions) {
155158
}
156159

157160
if (url.pathname === "/daemon/shutdown" && req.method === "POST") {
158-
await options.onShutdown?.();
161+
if (!isJsonRequest(req)) {
162+
return json(createDaemonErrorResponse("invalid-request", "Daemon shutdown requests must use application/json."), { status: 415 });
163+
}
164+
const timer = setTimeout(() => {
165+
void Promise.resolve(options.onShutdown?.()).catch(() => {});
166+
}, 0);
167+
timer.unref?.();
159168
return json({ ok: true, shuttingDown: true });
160169
}
161170

‎packages/server/daemon/session-factory.ts‎

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -103,7 +103,7 @@ async function resolveAnnotateInput(
103103
const rawAnnotateArgs = request.args ?? (typeof request.filePath === "string" ? request.filePath : "");
104104
const parsedArgs = parseAnnotateArgs(rawAnnotateArgs);
105105
const gate = request.gate ?? parsedArgs.gate;
106-
const renderHtml = request.renderHtml ?? parsedArgs.renderHtml;
106+
const renderHtml = request.renderHtml ?? parsedArgs.renderHtml ?? typeof request.rawHtml === "string";
107107

108108
let markdown = directMarkdown ? request.markdown! : "";
109109
let rawHtml = request.rawHtml;
@@ -113,10 +113,16 @@ async function resolveAnnotateInput(
113113
let sourceInfo = request.sourceInfo;
114114
let sourceConverted = request.sourceConverted ?? false;
115115

116-
if (!directMarkdown) {
116+
if (folderPath) {
117+
const resolvedFolder = isAbsolute(folderPath) ? folderPath : resolveUserPath(folderPath, cwd);
118+
folderPath = resolvedFolder;
119+
filePath = resolvedFolder;
120+
markdown = directMarkdown ? markdown : "";
121+
mode = "annotate-folder";
122+
} else if (!directMarkdown && typeof rawHtml !== "string") {
117123
const rawFilePath = parsedArgs.rawFilePath || parsedArgs.filePath;
118124
if (!rawFilePath) {
119-
throw new Error("Plugin annotate requests must include args, markdown, or filePath.");
125+
throw new Error("Plugin annotate requests must include args, markdown, filePath, folderPath, or rawHtml.");
120126
}
121127

122128
const parsedFilePath = parsedArgs.filePath;

‎packages/server/daemon/session-store.ts‎

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -199,15 +199,15 @@ export class DaemonSessionStore {
199199
if (new Date(record.expiresAt).getTime() > now) continue;
200200
if (TERMINAL_STATUSES.has(record.status)) {
201201
expired.push(record);
202-
await this.delete(record.id);
202+
await this.removeRecord(record);
203203
continue;
204204
}
205205
record.status = "expired";
206206
record.error = "Session expired.";
207207
record.updatedAt = iso(now);
208208
expired.push(record);
209209
this.resolveWaiters(record);
210-
await this.delete(record.id);
210+
await this.removeRecord(record);
211211
}
212212
return expired;
213213
}
@@ -237,6 +237,11 @@ export class DaemonSessionStore {
237237
for (const waiter of waiters) waiter.reject(err);
238238
}
239239

240+
private async removeRecord(record: DaemonSessionRecord): Promise<void> {
241+
this.sessions.delete(record.id);
242+
await this.disposeRecord(record);
243+
}
244+
240245
private async disposeRecord(record: DaemonSessionRecord): Promise<void> {
241246
if (record.disposed) return;
242247
record.disposed = true;

‎packages/server/daemon/start-command.test.ts‎

Lines changed: 20 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3,9 +3,27 @@ import { getDaemonStartCommand } from "./start-command";
33

44
describe("getDaemonStartCommand", () => {
55
test("uses Bun plus the source entry when running from TypeScript", () => {
6-
expect(getDaemonStartCommand(["bun", "apps/hook/server/index.ts"], "/usr/local/bin/bun")).toEqual([
6+
expect(getDaemonStartCommand(
7+
["bun", "apps/hook/server/index.ts"],
78
"/usr/local/bin/bun",
8-
"apps/hook/server/index.ts",
9+
"/repo/plannotator",
10+
)).toEqual([
11+
"/usr/local/bin/bun",
12+
"/repo/plannotator/apps/hook/server/index.ts",
13+
"daemon",
14+
"start",
15+
"--foreground",
16+
]);
17+
});
18+
19+
test("keeps absolute source entries absolute", () => {
20+
expect(getDaemonStartCommand(
21+
["bun", "/repo/plannotator/apps/hook/server/index.ts"],
22+
"/usr/local/bin/bun",
23+
"/other",
24+
)).toEqual([
25+
"/usr/local/bin/bun",
26+
"/repo/plannotator/apps/hook/server/index.ts",
927
"daemon",
1028
"start",
1129
"--foreground",

0 commit comments

Comments
 (0)