Skip to content

feat(design): auto-retry crashed persona once before giving up #77

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
kwliang1 wants to merge 2 commits into from
Closed

feat(design): auto-retry crashed persona once before giving up #77

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3918e32
fix(design): detect all-dead personas and cancel immediately
kwliang1 Jul 3, 2026
62a9f36
feat(design): auto-retry crashed persona once before giving up
kwliang1 Jul 6, 2026
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
119 changes: 109 additions & 10 deletions daemon/design.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { gateway } from './config.js'
import { registry } from './sessions.js'
import { doSpawnSession, killSession, killsInProgress } from './session-lifecycle.js'
import { doSpawnSession, killSession, killsInProgress, waitForBridge, HEALTH_TIMEOUT_MS } from './session-lifecycle.js'
import { transport } from './bridge-transport.js'
import { getReviewByThread } from './adversarial.js'
import { getBuildByThread } from './build.js'
Expand Down Expand Up @@ -59,6 +59,9 @@ export type DesignState = {
refinementExpected: number
refinementResponses: number
refinementRespondedIds: Set<string>

@sf8193 sf8193 Jul 7, 2026

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit (bump): activeDivergence inline type { description: string; personas: string[]; impact: string } is presumably the same shape as divergence objects in refinementQueue. Extract a shared Divergence type to keep them in sync.

activeDivergence?: { description: string; personas: string[]; impact: string }

@sf8193 sf8193 Jul 4, 2026

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: activeDivergence is typed inline. Since refinementQueue presumably holds the same shape, consider referencing a shared Divergence type to avoid drift.

@sf8193 sf8193 Jul 5, 2026

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: This inline type { description: string; personas: string[]; impact: string } is presumably the same shape as divergence objects in refinementQueue. Extract a named type to keep them in sync.

@sf8193 sf8193 Jul 6, 2026

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit (bump): activeDivergence inline type { description: string; personas: string[]; impact: string } is the same shape as divergence objects in refinementQueue. Extract a shared Divergence type to prevent drift.

@sf8193 sf8193 Jul 6, 2026

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit (bump): activeDivergence inline type { description: string; personas: string[]; impact: string } is the same shape as divergence objects in refinementQueue. Extract a shared Divergence type to keep them in sync.

@sf8193 sf8193 Jul 6, 2026

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit (bump): This inline type is the same shape as divergence objects in refinementQueue. Extract a shared Divergence type to prevent drift.

retriedPersonas: Set<string>
retryingPersonas: Set<string>
timeout?: ReturnType<typeof setTimeout>
_synthesizerDisconnectTimer?: ReturnType<typeof setTimeout>
_auditorDisconnectTimer?: ReturnType<typeof setTimeout>
Expand Down Expand Up @@ -154,6 +157,8 @@ export async function startDesign(
refinementExpected: 0,
refinementResponses: 0,
refinementRespondedIds: new Set(),
retriedPersonas: new Set(),
retryingPersonas: new Set(),
}

designs.set(threadId, state)
Expand Down Expand Up @@ -324,7 +329,7 @@ async function cleanupDesignSessions(state: DesignState, reason: string): Promis
// Cancel a design
// ---------------------------------------------------------------------------

export async function cancelDesign(threadId: string): Promise<void> {
export async function cancelDesign(threadId: string, message?: string): Promise<void> {
const state = designs.get(threadId)
if (!state) return

Expand All @@ -337,7 +342,7 @@ export async function cancelDesign(threadId: string): Promise<void> {
await cleanupDesignSessions(state, 'design cancelled')
designs.delete(threadId)
refreshSessionVisual(threadId)
await gateway.send(state.ownerThreadId, `Design session cancelled.`)
await gateway.send(state.ownerThreadId, message ?? `Design session cancelled.`)
}

// ---------------------------------------------------------------------------
Expand Down Expand Up @@ -565,12 +570,13 @@ async function runRefinement(

async function processNextDivergence(state: DesignState): Promise<void> {
if (!state.refinementQueue || state.refinementQueue.length === 0) {
// All divergences refined — auto re-synthesize
state.activeDivergence = undefined
void autoAdvanceAfterRefinement(state)
return
}

const divergence = state.refinementQueue.shift()!
state.activeDivergence = divergence

@sf8193 sf8193 Jul 4, 2026

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: activeDivergence is set here but never cleared (e.g., when refinement completes or the queue is exhausted). Currently safe because retryPersona checks state.phase === 'refinement' before using it, but clearing it explicitly would prevent future bugs.

@sf8193 sf8193 Jul 5, 2026

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should-fix (flagged by both reviewers): activeDivergence is set here but never cleared when refinement for this divergence completes or when the phase changes. If a persona crashes after refinement finishes but before the next divergence (or during synthesis), retryPersona will see the stale divergence, send a refinement prompt, and increment refinementExpected — corrupting the counter.

Fix: set state.activeDivergence = undefined at the end of processNextDivergence (when all responses are in) or on phase transitions out of refinement.

state.currentDivergence++
state.refinementResponses = 0
state.refinementRespondedIds = new Set()
Expand Down Expand Up @@ -615,6 +621,81 @@ async function processNextDivergence(state: DesignState): Promise<void> {
}, PERSONA_TIMEOUT_MS)
}

// ---------------------------------------------------------------------------
// Auto-retry a crashed persona once
// ---------------------------------------------------------------------------

async function retryPersona(
state: DesignState,
deadPersona: { name: string; sessionId: string; proposed: boolean },
threadId: string,
): Promise<void> {
const name = deadPersona.name as PersonaName
const cutoffTs = new Date().toISOString()
const oldSessionId = deadPersona.sessionId

if (state.phase === 'cancelled' || state.phase === 'complete') return
state.retryingPersonas.add(name)

@sf8193 sf8193 Jul 6, 2026

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should-fix (new, flagged by ts-reviewer): No early-exit guard for state.phase === 'cancelled' || 'complete' at the top of retryPersona before calling doSpawnSession. If the design is cancelled (e.g., by the user) while the persona crash is being processed, the retry still proceeds to spawn a new session, wait up to HEALTH_TIMEOUT_MS, and only then clean it up at line 658-662.

Fix: add if (state.phase === 'cancelled' || state.phase === 'complete') return before line 637.


try {
const oldInfo = registry.get(oldSessionId)
if (oldInfo && !killsInProgress.has(oldSessionId)) {
await killSession(oldInfo, 'persona crashed, retrying').catch(() => {})
}

const result = await doSpawnSession(`Design persona: ${name}`, undefined, undefined, {

@sf8193 sf8193 Jul 6, 2026

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should-fix (new, flagged by both reviewers): The old persona session is never killed before spawning the replacement. The bridge disconnected (that's how we got here), but the tmux session and registry entry for the old sessionId may linger. This leaks resources.

Add explicit cleanup before spawning:

const oldInfo = registry.get(deadPersona.sessionId)
if (oldInfo && !killsInProgress.has(deadPersona.sessionId)) {
  await killSession(oldInfo, 'persona crashed, retrying').catch(() => {})
}

joinThread: threadId,
memberLabel: name,
promptBuilder: (sessionId, tmuxName) => designPersonaPrompt({ sessionId, tmuxName, persona: name, topic: state.topic, threadId, cutoffTs }),
})

@sf8193 sf8193 Jul 7, 2026

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocker (bump, flagged by both reviewers): deadPersona.sessionId is mutated to the new session ID before waitForBridge. If the health check fails, killSession runs on the new session, then the catch calls onDesignParticipantDisconnect(deadPersona.sessionId) — which is now the new ID. The fallback path works by accident (the new session was killed so transport.has returns false), but the mutation-then-throw pattern is fragile.

Fix: Defer mutation until after health check:

const newSessionId = result.sessionId
const ok = await waitForBridge(newSessionId, HEALTH_TIMEOUT_MS)
if (!ok) { /* kill newSessionId, throw */ }
deadPersona.sessionId = newSessionId // only now


deadPersona.sessionId = result.sessionId

@sf8193 sf8193 Jul 4, 2026

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should-fix: in-place sessionId mutation loses the old session ID.

Two consequences:

  1. If the old session's process is still lingering (delayed teardown), cleanupDesignSessions iterates state.personas and only sees the new sessionId — the old process leaks.
  2. Late-arriving disconnect events for the old sessionId won't match any persona in state.personas, so they're silently ignored. This is benign but makes debugging harder.

Consider keeping the old ID (e.g., a previousSessionIds set on the persona) or treating the replacement as a distinct entry.

@sf8193 sf8193 Jul 5, 2026

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocker (flagged by both reviewers): deadPersona.sessionId is mutated to the new session ID before the health check. If waitForBridge returns false, killSession runs, then the catch block calls onDesignParticipantDisconnect(deadPersona.sessionId) with the new (already-killed) session ID. The disconnect handler lookup may silently no-op if the kill already cleaned it from the registry, leaving the dead persona unaccounted for — expectations never decrement.

Fix: move this assignment after the waitForBridge success path (after line 649), or stash the original ID and pass it to the recursive onDesignParticipantDisconnect call.

@sf8193 sf8193 Jul 6, 2026

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocker (bump): deadPersona.sessionId is mutated to the new session ID before waitForBridge. If the health check fails, killSession runs on the new session, then the catch calls onDesignParticipantDisconnect(deadPersona.sessionId) with the already-killed new ID — the disconnect handler may no-op (registry cleaned), leaving counters uncorrected.

Fix: move this assignment after the waitForBridge success path, or stash the original ID and use it in the catch.

@sf8193 sf8193 Jul 6, 2026

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocker (bump, flagged by both reviewers): deadPersona.sessionId is mutated to the new session ID before waitForBridge. If the health check fails, killSession runs on the new session, then the catch calls onDesignParticipantDisconnect(deadPersona.sessionId) — but that's now the new session ID, not the old one. The old session's tmux/registry entry is never cleaned up (resource leak), and the disconnect handler looks up the wrong persona.

Additionally, the old session is never explicitly killed before spawning the replacement. Compare with the synthesizer timeout handler which kills the old session before respawning.

Fix: save the old session ID, kill the old session before spawning, and only assign the new session ID after the health check succeeds.

@sf8193 sf8193 Jul 6, 2026

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocker (bump, flagged by both reviewers): deadPersona.sessionId is mutated to the new session ID before waitForBridge. If the health check fails, killSession runs on the new session, then the catch at line 690 deletes retryingPersonas and calls onDesignParticipantDisconnect(deadPersona.sessionId) — but that's now the new (already-killed) session ID, not the original dead one. The disconnect handler either finds no matching persona or finds the stale entry, leading to expectations never being decremented and the phase getting stuck.

Acknowledged: old-session cleanup (line 638-641) and retryingPersonas guard (line 722) were added to address prior feedback — those are good. The core issue is the mutation ordering.

Fix: capture const originalSessionId = deadPersona.sessionId before line 643 and use it in the catch block. Move the mutation to after the health check succeeds.


const ok = await waitForBridge(result.sessionId, HEALTH_TIMEOUT_MS)
if (!ok) {
const info = registry.get(result.sessionId)
if (info) await killSession(info, 'persona retry health check failed').catch(() => {})
throw new Error('bridge did not connect')
}

if (state.phase === 'cancelled' || state.phase === 'complete') {
const info = registry.get(result.sessionId)

@sf8193 sf8193 Jul 7, 2026

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should-fix (new): If the design ends during retry (phase becomes cancelled/complete), this early return skips the state.retryingPersonas.delete(name) at line 694. The persona name remains permanently in retryingPersonas, which blocks future disconnect handling for that persona name.

if (info) await killSession(info, 'design ended during retry').catch(() => {})
return
}

process.stderr.write(`daemon: design: ${name} auto-retried as ${result.name}\n`)
void gateway.send(threadId, `_${name} respawned as **${result.name}**._`).catch(() => {})

if (state.phase === 'independent') {
transport.sendOrQueue(result.sessionId, {
type: 'notification',
content: `[system] You are replacing a crashed persona. Read the thread for context, then post your proposal. Tag with \`[${name}→thread]\`. Be INDEPENDENT.`,
meta: { chat_id: threadId, message_id: '', user: 'system', user_id: 'system', ts: cutoffTs },

@sf8193 sf8193 Jul 7, 2026

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should-fix (new, flagged by both reviewers): When a retried persona re-enters refinement, refinementExpected is not re-incremented. The disconnect handler already decremented it (line 766). If the retried persona responds, refinementResponses increases but the bookkeeping is off — the system may advance to the next divergence prematurely or count the response as unexpected.

Same issue in the independent phase (line 668): proposalsExpected was decremented by the disconnect handler but isn't re-incremented when the retry succeeds.

Fix: Re-increment refinementExpected / proposalsExpected when the retry succeeds and the persona is prompted.

})

@sf8193 sf8193 Jul 7, 2026

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit (new, flagged by both reviewers): replaceAll('_', ' ') here vs replace('_', ' ') (first occurrence only) in processNextDivergence. Persona names with multiple underscores (e.g. ux_design_lead) would match here but not there. Consider extracting a matchesPersonaName(a, b) helper for consistency.

} else if (state.phase === 'refinement' && state.activeDivergence) {
if (state.activeDivergence.personas.some(n => n === name || n === name.replaceAll('_', ' ')) && !state.refinementRespondedIds.has(oldSessionId)) {
transport.sendOrQueue(result.sessionId, {

@sf8193 sf8193 Jul 6, 2026

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit (new, flagged by both reviewers): replaceAll('_', ' ') here but processNextDivergence (pre-existing, line ~584) uses replace('_', ' ') which only replaces the first occurrence. If a persona name has multiple underscores (e.g. ux_design_lead), the match logic diverges. Both sites should use the same normalization — ideally extract a shared personaNameMatches(a, b) helper.

type: 'notification',
content: [
`[system] Refinement requested on divergence: **${state.activeDivergence.description}**`,
``,
`Critique the synthesized composite design from your lens (${name}).`,
`Post your response with: \`[${name}→thread]\``,
].join('\n'),
meta: { chat_id: threadId, message_id: '', user: 'system', user_id: 'system', ts: cutoffTs },
})
}
}
} catch (err) {
process.stderr.write(`daemon: design: ${name} auto-retry failed: ${err}\n`)
state.retryingPersonas.delete(name)
onDesignParticipantDisconnect(deadPersona.sessionId)
return
}
state.retryingPersonas.delete(name)
}

// ---------------------------------------------------------------------------
// Participant disconnect / reconnect — grace timer before cancelling
// ---------------------------------------------------------------------------
Expand All @@ -637,14 +718,32 @@ export function onDesignParticipantDisconnect(sessionId: string): void {
: isAuditor ? 'auditor'
: 'brief writer'

// Personas are expendable — adjust expectations immediately (no grace timer)
// Personas are expendable — try auto-retry once, then adjust expectations
if (persona) {
process.stderr.write(`daemon: design: ${label} disconnected/died\n`)
void gateway.send(threadId, `_${label} disconnected. Continuing with ${state.personas.filter(p => p.sessionId !== sessionId).length} remaining personas._`).catch(() => {})
if (state.retryingPersonas.has(persona.name)) return

const aliveCount = state.personas.filter(p => p.sessionId !== sessionId && transport.has(p.sessionId)).length

@sf8193 sf8193 Jul 4, 2026

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should-fix: aliveCount can be stale under concurrent persona crashes.

If two personas crash near-simultaneously and both retries are exhausted, each computes aliveCount === 1 (seeing the other as alive before its transport is torn down). Neither triggers the aliveCount === 0 cancellation, and the design hangs until timeout.

This is a narrow race but possible under load. A mitigation: recheck aliveCount just before the cancel decision, or move the cancel check into a shared function that recalculates.

@sf8193 sf8193 Jul 6, 2026

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should-fix (bump): aliveCount is computed once at the top but used later for the cancel decision. If two personas crash near-simultaneously and both retries are exhausted, each sees the other as alive before its transport tears down — neither triggers aliveCount === 0, and the design hangs until timeout.

Mitigation: recheck alive count just before the cancel decision.

@sf8193 sf8193 Jul 6, 2026

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should-fix (bump): aliveCount is computed once at the top but used later for the cancel decision. If two personas crash near-simultaneously and both retries are exhausted, each computes aliveCount === 1 (seeing the other as alive), so neither triggers the all-dead cancellation. Result: design stuck with no live personas.

Consider recomputing aliveCount at the point of use, or using an atomic decrement-and-check pattern.

@sf8193 sf8193 Jul 7, 2026

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should-fix (bump): aliveCount is computed once here but used later for the cancel decision at line 737. If two personas crash near-simultaneously and both retries are exhausted, each computes aliveCount === 1 (seeing the other as alive), both skip the cancel path, and both decrement *Expected — potentially advancing the phase with zero participants.

Additionally, aliveCount doesn't exclude personas in retryingPersonas whose old sessions are dead but new sessions aren't in transport yet — this can cause premature "all personas dead" cancellation while a retry is in flight.

Fix: Recompute alive count at the cancel decision point, and exclude retryingPersonas from the dead count.

process.stderr.write(`daemon: design: ${label} disconnected/died (${aliveCount} alive)\n`)

@sf8193 sf8193 Jul 6, 2026

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should-fix (bump): aliveCount is computed once at line 724 but used later for the cancel decision at line 736. If two personas crash near-simultaneously and both retries are exhausted, each computes aliveCount === 1 (seeing the other as alive before its transport tears down) — neither triggers aliveCount === 0, and the design hangs with no personas.

Fix: recompute aliveCount at the cancel decision point, or set state.phase = 'cancelled' synchronously before the void cancelDesign(...) call so subsequent disconnect events short-circuit.

if (!state.retriedPersonas.has(persona.name)) {

@sf8193 sf8193 Jul 6, 2026

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should-fix (new, flagged by both reviewers): retryingPersonas is written to (line 635 add, line 687 delete) but never read anywhere. If a delayed disconnect event for the old sessionId arrives while retryPersona is in-flight, the handler finds the persona, sees retriedPersonas.has(name) is true, and falls through to the retry-exhausted path — decrementing counters even though the retry might succeed.

Add state.retryingPersonas.has(persona.name) as a guard here to skip counter adjustment while a retry is in-flight. Otherwise the set is dead code and should be removed.

state.retriedPersonas.add(persona.name)
void gateway.send(threadId, `_${label} crashed — auto-retrying..._`).catch(() => {})
void retryPersona(state, persona, threadId)

@sf8193 sf8193 Jul 5, 2026

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should-fix: retryPersona is fire-and-forget (void) and takes 30+ seconds (spawn + health check). During that time, timeouts can fire and advance the phase. The phase-specific prompt selection (lines 661-683) uses the phase at completion time, not at crash time — so a persona that crashed during questioning could receive an independent or refinement prompt if the phase advanced while retry was in-flight.

Also, if cancelDesign runs concurrently (user cancel or all-dead from another disconnect), the retry can finish spawning a session that cancel already iterated past, leaving an orphan.

Consider: (1) capture the phase at crash time and use it for prompt selection, (2) check phase before spawn in addition to after.

@sf8193 sf8193 Jul 6, 2026

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should-fix (bump): retryPersona is fire-and-forget and takes ~30s (spawn + health check). During that window, timeouts can fire and advance the phase. The phase-specific prompt at completion time may not match the phase at crash time.

Consider: (1) capture the phase at crash time for prompt selection, (2) check phase before spawn in addition to after.

@sf8193 sf8193 Jul 6, 2026

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should-fix (bump, flagged by both reviewers): retryPersona is fire-and-forget (void) and takes ~30s (spawn + health check). During that window, timeouts can fire and advance the phase. The phase-specific prompt selection at the end of retryPersona (lines 659-680) reads state.phase which may have changed by then. The retried persona gets instructions for a phase that already ended.

Consider capturing the phase at retry start and comparing when the retry completes, or adding a phase-change check before sending the prompt.

@sf8193 sf8193 Jul 7, 2026

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should-fix (bump): retryPersona is fire-and-forget (void) and takes ~30s (spawn + health check). During that window, timeouts can fire and advance the phase. The phase-specific prompt at completion time (lines 668-687) reads state.phase and state.activeDivergence from live state — if the phase advanced while the retry was in flight, the persona gets a stale or wrong prompt.

return
}

@sf8193 sf8193 Jul 6, 2026

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should-fix (bump, flagged by both reviewers): retryPersona is fire-and-forget (void) and takes ~30s (spawn + health check). During that window, the phase timeout can fire and advance the phase. The phase-specific prompt at the end of retryPersona (lines 667-687) reads state.phase at completion time, not at crash time — so a persona that crashed during independent could receive a refinement prompt (or no prompt at all if the phase doesn't match).

Fix: capture const crashPhase = state.phase at call time and use it for prompt selection inside retryPersona.


void gateway.send(threadId, `_${label} disconnected (retry exhausted). ${aliveCount > 0 ? `Continuing with ${aliveCount} remaining persona${aliveCount !== 1 ? 's' : ''}.` : 'All personas dead.'}_`).catch(() => {})

if (aliveCount === 0) {
if (state.timeout) { clearTimeout(state.timeout); state.timeout = undefined }
process.stderr.write(`daemon: design: all personas dead — cancelling\n`)
void cancelDesign(threadId, `All personas crashed. Design cancelled.\nUse \`design: ${state.topic}\` to retry.`).catch(e => process.stderr.write(`daemon: design: cancel failed: ${e}\n`))

@sf8193 sf8193 Jul 6, 2026

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should-fix (new, flagged by both reviewers): cancelDesign is async and fire-and-forget here. Between the void call and cancelDesign setting state.phase = 'cancelled', another disconnect event could enter the handler, see aliveCount === 0 again, and fire a second concurrent cancel.

Fix: set state.phase = 'cancelled' synchronously before the void cancelDesign(...) call:

state.phase = 'cancelled' as any // prevent re-entry
void cancelDesign(threadId, ...).catch(...)

@sf8193 sf8193 Jul 6, 2026

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should-fix (bump): cancelDesign is async and fire-and-forget here. Between the void call and cancelDesign setting state.phase = 'cancelled', another disconnect event could enter onDesignParticipantDisconnect, pass the designs.get(threadId) check, and attempt its own cancel — causing double cleanup.

Consider setting state.phase = 'cancelled' synchronously before awaiting cancelDesign, or adding a guard at the top of cancelDesign for the cancelled phase.

@sf8193 sf8193 Jul 7, 2026

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should-fix (bump): cancelDesign is async and fire-and-forget here. Between the void call and cancelDesign setting state.phase = 'cancelled', another disconnect event could enter onDesignParticipantDisconnect and also trigger cancel — double cleanup. Consider setting state.phase = 'cancelled' synchronously here before the void cancelDesign(...) call.

return
}

@sf8193 sf8193 Jul 6, 2026

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should-fix (bump): cancelDesign is async and fire-and-forget here. Between the void call and cancelDesign setting state.phase = 'cancelled', another disconnect event can enter the handler, see aliveCount === 0 again, and fire a second concurrent cancel — double cleanup, double user message.

Fix: set state.phase = 'cancelled' synchronously before void cancelDesign(...), so subsequent disconnect events see the cancelled state and bail early.


if (state.phase === 'questioning') {
state.questionsExpected--
if (state.questionsExpected > 0 && state.questionsReceived >= state.questionsExpected) {
if (state.questionsReceived >= state.questionsExpected) {

@sf8193 sf8193 Jul 4, 2026

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should-fix (flagged by both reviewers): removing the > 0 guards enables premature phase transitions.

The old code had state.questionsExpected > 0 && (and similarly for proposals/refinement). With the guard removed, if the last persona dies and retry is exhausted, questionsExpected decrements to 0, and 0 >= 0 triggers the transition. The aliveCount === 0 cancellation check should catch this first — but it won't if another persona is still alive (aliveCount > 0) while this counter hits 0. That advances the phase with zero expectations from this counter.

Suggestion: restore the > 0 guards, or move the aliveCount === 0 cancellation to before the per-phase counter adjustment.

@sf8193 sf8193 Jul 5, 2026

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocker (flagged by both reviewers): Removing the questionsExpected > 0 guard (and equivalently at lines 744, 755) allows 0 >= 0 to trigger a phase transition with zero contributions.

Scenario: persona A is alive and working, persona B (retry exhausted) disconnects. aliveCount > 0 so the cancellation path doesn't fire. questionsExpected decrements to 0, questionsReceived is 0, 0 >= 0 is true → phase advances with zero questions. Persona A's eventual submission arrives in the wrong phase and is silently dropped.

Fix: restore a > 0 guard (e.g. state.questionsExpected > 0 && state.questionsReceived >= state.questionsExpected), or ensure the all-dead check accounts for this edge.

@sf8193 sf8193 Jul 6, 2026

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocker (bump): Removing the questionsExpected > 0 guard (and equivalently for proposals/refinement at lines 744, 755) allows 0 >= 0 to trigger a phase transition with zero contributions.

This is reachable when combined with the counter-corruption race from the retryingPersonas finding below: a delayed disconnect can decrement counters while a retry is in-flight, pushing *Expected to 0 while a live persona hasn't yet submitted. Restoring the > 0 guard acts as a safety net against this edge case.

@sf8193 sf8193 Jul 6, 2026

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocker (bump, flagged by both reviewers): Removing the questionsExpected > 0 guard (and equivalently at lines 747, 758) allows 0 >= 0 to trigger a phase transition with zero contributions.

Scenario: 2 personas alive, both crash in the same phase. First crash triggers retry (returns early). Second crash hits the exhausted path, decrements expected to 0. 0 >= 0 is true → advances to the next phase with zero data.

The aliveCount === 0 check at line 728 only fires when retries are exhausted. If one persona is retrying and the other dies with retry exhausted, aliveCount may be 0 but the code reaches the phase-advance path. Restore the > 0 guard or add an explicit zero-expected check.

@sf8193 sf8193 Jul 7, 2026

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocker (bump, flagged by both reviewers): Removing the questionsExpected > 0 guard (and equivalently at lines 756, 767) allows 0 >= 0 to trigger a phase transition with zero contributions.

The aliveCount === 0 cancel path at line 737 is meant to replace this guard, but under concurrent persona crashes + retry failures, aliveCount can be stale (see line 725 comment). If the cancel path doesn't fire, the 0 >= 0 fallthrough advances the phase with no participants.

Fix: Either restore the > 0 guards as belt-and-suspenders, or add an assertion documenting the invariant that this code is only reachable when aliveCount > 0.

if (state.timeout) clearTimeout(state.timeout)
const result = designMachine.transition(state.phase, 'all_questions')

@sf8193 sf8193 Jul 6, 2026

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocker (bump, flagged by both reviewers): Removing the questionsExpected > 0 guard (and equivalently at lines 752, 763) allows 0 >= 0 to trigger a phase transition with zero contributions.

Scenario: 2 personas, both crash in questioning phase. First crash triggers retry (returns early, no decrement). Second crash: retry exhausted → questionsExpected-- → now 0. 0 >= 0 is true → fires aggregateAndPostQuestions with an empty question set. The aliveCount === 0 cancel path fires cancelDesign with void (async), so the synchronous counter check runs first.

Fix: restore > 0 guards, or add if (aliveCount === 0) return before the counter-decrement blocks (the cancel already handles this case).

if (result.ok) {
Expand All @@ -654,7 +753,7 @@ export function onDesignParticipantDisconnect(sessionId: string): void {
}
} else if (state.phase === 'independent' && !persona.proposed) {
state.proposalsExpected--
if (state.proposalsExpected > 0 && state.proposalsReceived >= state.proposalsExpected) {
if (state.proposalsReceived >= state.proposalsExpected) {
if (state.timeout) clearTimeout(state.timeout)
const result = designMachine.transition(state.phase, 'all_proposed')
if (result.ok) {
Expand All @@ -665,7 +764,7 @@ export function onDesignParticipantDisconnect(sessionId: string): void {
}
} else if (state.phase === 'refinement') {
state.refinementExpected--
if (state.refinementExpected > 0 && state.refinementResponses >= state.refinementExpected) {
if (state.refinementResponses >= state.refinementExpected) {
if (state.timeout) clearTimeout(state.timeout)
void processNextDivergence(state)
}
Expand Down