Skip to content

Commit f8427cd

Browse files
ymansurozerclaude
andcommitted
feat: batch question delivery and fold open questions into Send
A reviewer can fire several questions before the agent comes back, but each await delivered exactly one event — the rest sat queued with nothing telling the agent to return, so it appeared stuck. One await now drains every queued question into a single event: the new `questions` array carries them in arrival order and the singular `question` field (the oldest) stays for compatibility, so a lone question reads uniformly as a one-element array. Send flushes queued question events — superseded by the round — and ReviewResult gains `openQuestions`: every still-unanswered question (same answered heuristic the UI uses), built with the same payload constructor as /api/ask so the shapes can't drift. The contract adds the loop rule: after handling any event, await again immediately — more may already be queued. Batch-drain safety rests on a documented invariant: a Send flushes all queued questions before enqueuing its review, so a review can never sit between two queued questions. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015jcfPg9g4TTWQksf8vhHps
1 parent adecad0 commit f8427cd

8 files changed

Lines changed: 270 additions & 21 deletions

File tree

scripts/smoke.mjs

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -87,6 +87,34 @@ try {
8787
assert.equal(ev1.question.lineNumber, 2);
8888
console.log("✓ await → question event");
8989

90+
// Two more questions fired back-to-back with no await parked → they batch: a single await
91+
// hands over BOTH, oldest first, with the singular `question` kept for compatibility.
92+
await post("/api/ask", {
93+
path: "a.txt",
94+
lineNumber: 2,
95+
side: "additions",
96+
body: "batched one?",
97+
});
98+
await post("/api/ask", {
99+
path: "a.txt",
100+
lineNumber: 2,
101+
side: "additions",
102+
body: "batched two?",
103+
});
104+
const batchState = await getJson("/api/state");
105+
assert.equal(batchState.queuedQuestions, 2, "both questions queued with no listener");
106+
const evBatch = JSON.parse(cli("await", "--repo", tmp, "--session", ID, "--timeout", "5"));
107+
assert.equal(evBatch.kind, "question");
108+
assert.equal(evBatch.questions.length, 2, "both questions delivered in one event");
109+
assert.deepEqual(
110+
evBatch.questions.map((q) => q.body),
111+
["batched one?", "batched two?"],
112+
);
113+
assert.equal(evBatch.question.body, "batched one?", "singular question is the oldest");
114+
const drainedState = await getJson("/api/state");
115+
assert.equal(drainedState.queuedQuestions, 0, "batch drained in one await");
116+
console.log("✓ multiple questions batch into one await event");
117+
90118
// agent posts ephemeral activity while working → visible in the state payload
91119
const status = JSON.parse(
92120
cli("status", "--repo", tmp, "--session", ID, "--body", "Reading a.txt…"),

src/server.test.ts

Lines changed: 84 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -192,6 +192,90 @@ test("an unconsumed question surfaces as queuedQuestions", async () => {
192192
});
193193
});
194194

195+
test("multiple questions asked before an await batch into one event, oldest first", async () => {
196+
await withServer(async (handle) => {
197+
await post(handle.url, "api/ask", { path: "a.ts", lineNumber: 1, body: "first?" });
198+
await post(handle.url, "api/ask", { path: "a.ts", lineNumber: 2, body: "second?" });
199+
const ev = (await fetch(`${handle.url}api/await-send`).then((r) => r.json())) as {
200+
kind: string;
201+
question: { body: string };
202+
questions: { body: string }[];
203+
};
204+
assert.equal(ev.kind, "question");
205+
assert.equal(ev.questions.length, 2);
206+
assert.deepEqual(
207+
ev.questions.map((q) => q.body),
208+
["first?", "second?"],
209+
);
210+
assert.deepEqual(ev.question, ev.questions[0]); // singular is the oldest
211+
// Both drained — nothing left queued for the UI's waiting indicator.
212+
const st = await getState(handle.url);
213+
assert.equal(st.queuedQuestions, 0);
214+
});
215+
});
216+
217+
test("Send flushes queued questions and folds the unanswered ones into openQuestions", async () => {
218+
await withServer(async (handle, _root, st) => {
219+
// Two open question comments the reviewer left (the source for openQuestions), plus the
220+
// matching live question events (the source for the queue) — two independent representations.
221+
st.comments.push(
222+
{
223+
id: "q1",
224+
path: "a.ts",
225+
side: "additions",
226+
lineNumber: 1,
227+
body: "why q1?",
228+
createdAt: "2026-01-01T00:00:00Z",
229+
updatedAt: "2026-01-01T00:00:00Z",
230+
status: "open",
231+
intent: "question",
232+
role: "user",
233+
},
234+
{
235+
id: "q2",
236+
path: "a.ts",
237+
side: "additions",
238+
lineNumber: 2,
239+
body: "why q2?",
240+
createdAt: "2026-01-01T00:00:01Z",
241+
updatedAt: "2026-01-01T00:00:01Z",
242+
status: "open",
243+
intent: "question",
244+
role: "user",
245+
},
246+
);
247+
await post(handle.url, "api/ask", { path: "a.ts", lineNumber: 1, body: "why q1?" });
248+
await post(handle.url, "api/ask", { path: "a.ts", lineNumber: 2, body: "why q2?" });
249+
250+
const sent = (await post(handle.url, "api/send", await getState(handle.url)).then((r) =>
251+
r.json(),
252+
)) as { sent?: boolean };
253+
assert.equal(sent.sent, true);
254+
255+
// The review is emitted on the send response's 'finish'; poll until it lands. The queued
256+
// questions are flushed in the same step, so queuedQuestions must be 0 by then.
257+
let status = await getState(handle.url);
258+
for (let i = 0; i < 100 && status.queuedReviews === 0; i++) {
259+
await new Promise((r) => setTimeout(r, 10));
260+
status = await getState(handle.url);
261+
}
262+
assert.equal(status.queuedReviews, 1);
263+
assert.equal(status.queuedQuestions, 0); // superseded questions flushed
264+
265+
// Next await is the review (not a stale question), carrying both unanswered questions.
266+
const ev = (await fetch(`${handle.url}api/await-send`).then((r) => r.json())) as {
267+
kind: string;
268+
result: { openQuestions: { body: string }[] };
269+
};
270+
assert.equal(ev.kind, "review");
271+
assert.deepEqual(ev.result.openQuestions.map((q) => q.body).sort(), ["why q1?", "why q2?"]);
272+
273+
// Nothing dribbles in after the round: a bounded await times out (204).
274+
const after = await fetch(`${handle.url}api/await-send?timeout=1`);
275+
assert.equal(after.status, 204);
276+
});
277+
});
278+
195279
test("save strips transient desk-status keys so they never persist on state", async () => {
196280
await withServer(async (handle, _root, st) => {
197281
const payload = await getState(handle.url);

src/server.ts

Lines changed: 40 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -18,11 +18,18 @@ import {
1818
mergeReviewState,
1919
nowIso,
2020
persistReview,
21+
questionPayload,
2122
readGlobalSettings,
2223
syncGitState,
2324
writeGlobalSettings,
2425
} from "./state.js";
25-
import type { AgentActivity, AwaitEvent, DeskStatus, ReviewState } from "./types.js";
26+
import type {
27+
AgentActivity,
28+
AwaitEvent,
29+
DeskStatus,
30+
QuestionPayload,
31+
ReviewState,
32+
} from "./types.js";
2633

2734
const execFileAsync = promisify(execFile);
2835
const __dirname = path.dirname(fileURLToPath(import.meta.url));
@@ -301,7 +308,14 @@ export async function startServer(options: ServerOptions): Promise<ServerHandle>
301308
const resultJson = path.join(sessionDir, `${state.id}-result.json`);
302309
const payload = buildReviewResult(state, { resultJson, sessionDir }, overallNote);
303310
await fs.writeFile(resultJson, JSON.stringify(payload, null, 2) + "\n", "utf8");
304-
res.on("finish", () => emitEvent({ kind: "review", result: payload }));
311+
res.on("finish", () => {
312+
// Drop any still-queued questions: the review supersedes them (their unanswered ones
313+
// ride out in result.openQuestions), so stale question events never dribble in after
314+
// the round lands. This is also the invariant the await-send batch drain relies on.
315+
for (let i = eventQueue.length - 1; i >= 0; i--)
316+
if (eventQueue[i].kind === "question") eventQueue.splice(i, 1);
317+
emitEvent({ kind: "review", result: payload });
318+
});
305319
return json(res, 200, { ok: true, sent: true, resultJson });
306320
}
307321
if (req.method === "POST" && url.pathname === "/api/ask") {
@@ -321,23 +335,37 @@ export async function startServer(options: ServerOptions): Promise<ServerHandle>
321335
"ask requires path and body",
322336
"Send { path, lineNumber, side, body } as JSON.",
323337
);
324-
emitEvent({
325-
kind: "question",
326-
question: {
327-
path: b.path,
328-
lineNumber: Number(b.lineNumber ?? 1),
329-
side: b.side === "deletions" ? "deletions" : "additions",
330-
body: text,
331-
mode: state.mode,
332-
session: state.session,
333-
},
338+
// Bake the singular into a one-element `questions` here so a question handed straight
339+
// to a parked waiter already carries the array — batching only has to merge on drain.
340+
const question = questionPayload(state, {
341+
path: b.path,
342+
lineNumber: Number(b.lineNumber ?? 1),
343+
side: b.side === "deletions" ? "deletions" : "additions",
344+
body: text,
334345
});
346+
emitEvent({ kind: "question", question, questions: [question] });
335347
return json(res, 200, { ok: true });
336348
}
337349
if (req.method === "GET" && url.pathname === "/api/await-send") {
338350
// Long-poll the tagged event stream: resolves with the next queued event
339351
// ({kind:"question"|"review"}). Lets the agent learn of questions and Sends
340352
// without the desk process exiting.
353+
// When the head is a question, drain ALL queued questions into one event (the reviewer
354+
// can fire several before the agent returns) — singular `question` is the oldest,
355+
// `questions` holds them in arrival order. Safe because a Send flushes every queued
356+
// question before enqueuing its review (see /api/send), so a review can never sit
357+
// between two queued questions; draining all questions can't skip past a review.
358+
if (eventQueue[0]?.kind === "question") {
359+
const batched: QuestionPayload[] = [];
360+
for (let i = eventQueue.length - 1; i >= 0; i--) {
361+
const ev = eventQueue[i];
362+
if (ev.kind === "question") {
363+
batched.unshift(...ev.questions);
364+
eventQueue.splice(i, 1);
365+
}
366+
}
367+
return json(res, 200, { kind: "question", question: batched[0], questions: batched });
368+
}
341369
const queued = eventQueue.shift();
342370
if (queued) return json(res, 200, queued);
343371
let settled = false;

src/spec.test.ts

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,10 +22,15 @@ const ANCHORS = [
2222
// a question is READ-ONLY: answer it, don't edit code in response (guards issue 04)
2323
"answering is READ-ONLY",
2424
"NEVER edit tracked",
25+
// question batching + immediate re-await (issue 05)
26+
"batched into this delivery",
27+
"await again immediately",
2528
// result + acting
2629
"ReviewResult",
2730
"approvedFiles",
2831
"overallNote",
32+
// unanswered questions fold into the Send (issue 05)
33+
"openQuestions",
2934
"How to act on a review",
3035
// guided review schema (folded in from the old guide-spec)
3136
"Guide JSON schema",

src/spec.ts

Lines changed: 17 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -53,7 +53,8 @@ done
5353
\`\`\`
5454
- \`galley await [--timeout <s>]\` — block for the next event, print one tagged JSON envelope,
5555
exit. No --timeout → holds open; --timeout <s> → empty stdout (204) after <s>s, re-poll. Exit
56-
non-zero = no live desk (start one).
56+
non-zero = no live desk (start one). After handling ANY event, await again immediately — more
57+
may already be queued (the human keeps working while you act).
5758
- \`galley comment --path <f> --line <n> [--side additions|deletions] --body "…"\` — agent reply.
5859
Live desk → posts over HTTP (~1.5s), threaded under the matching human comment; no desk →
5960
appended to the saved review. Match path/line/side. Agent comments are never echoed back as
@@ -67,13 +68,17 @@ done
6768
6869
## Events
6970
await yields exactly one:
70-
- {"kind":"question","question":{path,lineNumber,side,body,mode,session}} — reviewer wants an
71-
answer NOW. A question asks for an ANSWER, not a code change: answering is READ-ONLY — read the
72-
file for context, answer with \`galley comment\` at path/lineNumber/side. NEVER edit tracked
73-
files in response (same rule as "Between rounds"). The only exception: the question's own text
74-
explicitly asks for an immediate change — then treat it as actionable, and still follow the
75-
between-rounds discipline (edit, then \`galley reload\`). Questions are a live side-channel:
76-
NEVER in a Send/ReviewResult. Slow answer → post \`galley status\` lines so the human sees
71+
- {"kind":"question","question":{path,lineNumber,side,body,mode,session},"questions":[…]} —
72+
reviewer wants an answer NOW. \`questions\` holds every question batched into this delivery
73+
(the human can fire several before you return), arrival order; \`question\` is the oldest, kept
74+
for compatibility. Answer EACH one. A question asks for an ANSWER, not a code change:
75+
answering is READ-ONLY — read the file for context, answer with \`galley comment\` at
76+
path/lineNumber/side.
77+
NEVER edit tracked files in response (same rule as "Between rounds"). The only exception: the
78+
question's own text explicitly asks for an immediate change — then treat it as actionable, and
79+
still follow the between-rounds discipline (edit, then \`galley reload\`). Questions are a live
80+
side-channel: NEVER in a Send/ReviewResult (except openQuestions below, which folds any you
81+
never answered into the round). Slow answer → post \`galley status\` lines so the human sees
7782
progress, not a static spinner.
7883
- {"kind":"review","result":{…ReviewResult…}} — reviewer clicked Send. Act on result.
7984
@@ -86,6 +91,10 @@ The \`result\` field of a review event:
8691
an overall remark, or an afterthought instruction for what to do after applying the review
8792
(e.g. "after applying, run the formatter"). It is NOT tied to any line and not a per-line change.
8893
- stagedFiles[], approvedFiles[]
94+
- openQuestions[]: {path,lineNumber,side,body,mode,session} — questions the reviewer asked but you
95+
never answered, folded into this Send and superseding any queued live question events. Answer
96+
each with \`galley comment\` (same READ-ONLY discipline as a live question) as part of acting on
97+
the round.
8998
- artifacts: {resultJson, sessionDir}, both under
9099
~/.galley/<repoHash>/<session>/ where repoHash = sha256(abs repo root)[:16]
91100
The arrays above ARE the review — act on them directly; there's no prose summary to parse.

src/state.test.ts

Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -467,6 +467,50 @@ test("buildReviewResult excludes questions from requestedChanges, keeps an actio
467467
assert.equal(r.requestedChanges[0]!.body, "rename this");
468468
});
469469

470+
test("buildReviewResult.openQuestions lists unanswered questions and drops answered ones", () => {
471+
const s = state({
472+
session: "sess",
473+
mode: "repo",
474+
comments: [
475+
comment({
476+
id: "open",
477+
path: "a.ts",
478+
lineNumber: 4,
479+
body: "why here?",
480+
intent: "question",
481+
role: "user",
482+
createdAt: "2026-01-01T00:00:00Z",
483+
}),
484+
// Answered: a later agent reply lands in the same thread (same path/side/line).
485+
comment({
486+
id: "answered",
487+
path: "a.ts",
488+
lineNumber: 7,
489+
body: "and this?",
490+
intent: "question",
491+
role: "user",
492+
createdAt: "2026-01-01T00:00:00Z",
493+
}),
494+
comment({
495+
id: "reply",
496+
path: "a.ts",
497+
lineNumber: 7,
498+
body: "because X",
499+
intent: "note",
500+
role: "agent",
501+
createdAt: "2026-01-01T00:01:00Z",
502+
}),
503+
],
504+
});
505+
const r = buildReviewResult(s, { resultJson: "r.json", sessionDir: "d" });
506+
assert.equal(r.openQuestions.length, 1);
507+
assert.equal(r.openQuestions[0]!.body, "why here?");
508+
assert.equal(r.openQuestions[0]!.lineNumber, 4);
509+
// Same shape as an await question — mode/session threaded through.
510+
assert.equal(r.openQuestions[0]!.mode, "repo");
511+
assert.equal(r.openQuestions[0]!.session, "sess");
512+
});
513+
470514
test("buildReviewResult carries mode/target/base", () => {
471515
const s = state({ mode: "pr", target: "feature-x", base: "abc123" });
472516
const r = buildReviewResult(s, { resultJson: "r.json", sessionDir: "d" });

src/state.ts

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@ import {
1515
import type {
1616
ChangeState,
1717
Decision,
18+
QuestionPayload,
1819
ReviewComment,
1920
ReviewFile,
2021
ReviewMode,
@@ -676,6 +677,45 @@ export async function appendComment(
676677
return comment;
677678
}
678679

680+
// The single QuestionPayload constructor — shared by /api/ask (live question event) and
681+
// computeOpenQuestions (questions folded into a Send) so the two payload shapes can't drift.
682+
export function questionPayload(
683+
state: Pick<ReviewState, "mode" | "session">,
684+
q: { path: string; lineNumber: number; side: "additions" | "deletions"; body: string },
685+
): QuestionPayload {
686+
return {
687+
path: q.path,
688+
lineNumber: q.lineNumber,
689+
side: q.side,
690+
body: q.body,
691+
mode: state.mode,
692+
session: state.session,
693+
};
694+
}
695+
696+
// Questions the reviewer asked but the agent hasn't answered yet. Mirrors the UI's "answered"
697+
// heuristic (src/ui/annotations.ts): an open question comment is unanswered until a later agent
698+
// reply lands in the same thread (same path/side/line). These ride out on the Send's ReviewResult
699+
// so an agent that never saw the live await still owes each an answer.
700+
export function computeOpenQuestions(state: ReviewState): QuestionPayload[] {
701+
return state.comments
702+
.filter(
703+
(c) =>
704+
c.intent === "question" &&
705+
c.status === "open" &&
706+
c.role !== "agent" &&
707+
!state.comments.some(
708+
(r) =>
709+
r.role === "agent" &&
710+
r.path === c.path &&
711+
r.side === c.side &&
712+
r.lineNumber === c.lineNumber &&
713+
+new Date(r.createdAt) > +new Date(c.createdAt),
714+
),
715+
)
716+
.map((c) => questionPayload(state, c));
717+
}
718+
679719
export function buildReviewResult(
680720
state: ReviewState,
681721
artifacts: { resultJson: string; sessionDir: string },
@@ -704,6 +744,7 @@ export function buildReviewResult(
704744
overallNote: note || undefined,
705745
stagedFiles: state.stagedFiles,
706746
approvedFiles: computeApprovedFiles(state),
747+
openQuestions: computeOpenQuestions(state),
707748
artifacts,
708749
};
709750
}

0 commit comments

Comments
 (0)