Skip to content

Commit b86a32d

Browse files
committed
Address review: consistent failed-run payload, sanitized bounds, no waiter leak
- A thrown interactive run now reports through buildWorkflowRunPayload, so the debug surface keeps its summary + verdict shape on hard failures. - max_decisions / max_retries_per_node / decision_timeout_ms are API inputs: anything but a sane integer falls back to the default instead of reaching BoundedHandle (a NaN timeout failed every decision instantly). - waitForEvent's escalation subscription is cancellable, so the done branch winning the race no longer strands a waiter per verdict round trip. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KNZApg6rgnpV9aqs5BTyX7
1 parent d746c8f commit b86a32d

2 files changed

Lines changed: 70 additions & 38 deletions

File tree

packages/websocket/src/debug-sessions.ts

Lines changed: 45 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,12 @@ interface PendingEscalation {
3939
settle: (outcome: DecisionOutcome) => void;
4040
}
4141

42+
/** What escalation subscribers see: the record, never the settle handle. */
43+
export interface ParkedEscalation {
44+
id: string;
45+
escalation: Escalation;
46+
}
47+
4248
/**
4349
* A `SupervisorHandle` that answers nothing itself: each `decide()` parks the
4450
* escalation until `submit()` delivers a verdict (or the decision signal
@@ -47,7 +53,7 @@ interface PendingEscalation {
4753
*/
4854
export class InteractiveEscalationHandle implements SupervisorHandle {
4955
private _pending: PendingEscalation | null = null;
50-
private _waiters: Array<(entry: PendingEscalation) => void> = [];
56+
private _waiters: Array<(entry: ParkedEscalation) => void> = [];
5157
private _counter = 0;
5258
private _closed = false;
5359

@@ -81,19 +87,18 @@ export class InteractiveEscalationHandle implements SupervisorHandle {
8187
return { id: this._pending.id, escalation: this._pending.escalation };
8288
}
8389

84-
/** Resolves with the next parked escalation (immediately if one is parked). */
85-
nextEscalation(): Promise<{ id: string; escalation: Escalation }> {
86-
if (this._pending) {
87-
return Promise.resolve({
88-
id: this._pending.id,
89-
escalation: this._pending.escalation
90-
});
91-
}
92-
return new Promise((resolve) => {
93-
this._waiters.push((entry) =>
94-
resolve({ id: entry.id, escalation: entry.escalation })
95-
);
96-
});
90+
/**
91+
* Subscribe to the next parked escalation. Returns an unsubscribe so a
92+
* caller racing this against run completion can withdraw when the run wins
93+
* — otherwise every verdict round trip would strand one waiter until the
94+
* session is swept.
95+
*/
96+
subscribe(waiter: (entry: ParkedEscalation) => void): () => void {
97+
this._waiters.push(waiter);
98+
return () => {
99+
const index = this._waiters.indexOf(waiter);
100+
if (index !== -1) this._waiters.splice(index, 1);
101+
};
97102
}
98103

99104
/**
@@ -178,18 +183,33 @@ export class DebugSession {
178183
* parked decision fails closed on its own timeout, so this always resolves.
179184
*/
180185
async waitForEvent(): Promise<DebugSessionEvent> {
181-
return Promise.race([
182-
this._done.then(
183-
(report): DebugSessionEvent => ({ kind: "done", report })
184-
),
185-
this._handle.nextEscalation().then(
186-
(entry): DebugSessionEvent => ({
187-
kind: "escalated",
188-
escalationId: entry.id,
189-
escalation: entry.escalation
186+
const pending = this._handle.current();
187+
if (pending) {
188+
return {
189+
kind: "escalated",
190+
escalationId: pending.id,
191+
escalation: pending.escalation
192+
};
193+
}
194+
let unsubscribe: () => void = () => {};
195+
try {
196+
return await Promise.race([
197+
this._done.then(
198+
(report): DebugSessionEvent => ({ kind: "done", report })
199+
),
200+
new Promise<DebugSessionEvent>((resolve) => {
201+
unsubscribe = this._handle.subscribe((entry) =>
202+
resolve({
203+
kind: "escalated",
204+
escalationId: entry.id,
205+
escalation: entry.escalation
206+
})
207+
);
190208
})
191-
)
192-
]);
209+
]);
210+
} finally {
211+
unsubscribe();
212+
}
193213
}
194214

195215
/** Current state without waiting. */

packages/websocket/src/http-api.ts

Lines changed: 25 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -884,18 +884,18 @@ export async function handleWorkflowRun(
884884
// guarantees an LLM supervisor gets — decision/retry caps, a per-decision
885885
// timeout that fails closed, sticky verdicts — just with a timeout sized
886886
// for an agent's tool round trip instead of one model call.
887+
// These are API inputs: anything but a sane integer (NaN, Infinity,
888+
// negatives, fractions) falls back to the default rather than reaching
889+
// `BoundedHandle` — a NaN timeout would fail every decision instantly.
890+
const maxDecisions = boundedRunOption(body?.max_decisions, 1);
891+
const maxRetriesPerNode = boundedRunOption(body?.max_retries_per_node, 0);
887892
interactiveHandle = new InteractiveEscalationHandle();
888893
supervisorHandle = new BoundedHandle(interactiveHandle, {
889-
...(typeof body?.max_decisions === "number"
890-
? { maxDecisions: body.max_decisions }
891-
: {}),
892-
...(typeof body?.max_retries_per_node === "number"
893-
? { maxRetriesPerNode: body.max_retries_per_node }
894-
: {}),
894+
...(maxDecisions !== undefined ? { maxDecisions } : {}),
895+
...(maxRetriesPerNode !== undefined ? { maxRetriesPerNode } : {}),
895896
decisionTimeoutMs:
896-
typeof body?.decision_timeout_ms === "number"
897-
? body.decision_timeout_ms
898-
: INTERACTIVE_DECISION_TIMEOUT_MS
897+
boundedRunOption(body?.decision_timeout_ms, 1) ??
898+
INTERACTIVE_DECISION_TIMEOUT_MS
899899
});
900900
}
901901
runner = new WorkflowRunner(job.id, {
@@ -983,12 +983,17 @@ export async function handleWorkflowRun(
983983
error: String(saveError)
984984
});
985985
}
986-
return {
987-
job_id: job.id,
988-
workflow_id: workflowId,
986+
// Keep the payload shape of a failed run — the debug surface still gets
987+
// a summary and verdict — instead of a bare {status, error} object.
988+
const failed: WorkflowRunResult = {
989989
status: "failed",
990-
error: message
990+
error: message,
991+
messages: [],
992+
outputs: {}
991993
};
994+
return buildWorkflowRunPayload(job.id, workflowId, failed, debug, {
995+
background: false
996+
});
992997
} finally {
993998
supervisorHandle?.close();
994999
}
@@ -1008,6 +1013,13 @@ export async function handleWorkflowRun(
10081013

10091014
type WorkflowRunResult = Awaited<ReturnType<WorkflowRunner["run"]>>;
10101015

1016+
/** An interactive-run bound from the request body: an integer ≥ min, or undefined. */
1017+
function boundedRunOption(value: unknown, min: number): number | undefined {
1018+
return typeof value === "number" && Number.isInteger(value) && value >= min
1019+
? value
1020+
: undefined;
1021+
}
1022+
10111023
async function finalizeWorkflowRunJob(
10121024
job: Job,
10131025
result: WorkflowRunResult

0 commit comments

Comments
 (0)