Skip to content

fix: address review feedback for calls feature #5255

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

Merged
noanflaherty merged 1 commit into from
Feb 19, 2026
Merged

fix: address review feedback for calls feature #5255

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
7 changes: 7 additions & 0 deletions assistant/src/calls/call-orchestrator.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,13 @@ export class CallOrchestrator {
registerCallOrchestrator(callSessionId, this);
}

/**
* Returns the current orchestrator state.
*/
getState(): OrchestratorState {
return this.state;
}

/**
* Handle a final caller utterance from the ConversationRelay.
*/
Expand Down
10 changes: 9 additions & 1 deletion assistant/src/calls/twilio-routes.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -218,7 +218,15 @@ export async function handleCallAnswer(req: Request, callSessionId: string): Pro
return Response.json({ error: 'No active orchestrator for this call' }, { status: 409 });
}

// Mark question as answered
if (orchestrator.getState() !== 'waiting_on_user') {
log.warn(
{ callSessionId, state: orchestrator.getState() },
'handleCallAnswer: orchestrator is not in waiting_on_user state',
);
return Response.json({ error: 'Orchestrator is not waiting for an answer' }, { status: 409 });
}

// Mark question as answered — only after confirming the orchestrator is ready
answerPendingQuestion(question.id, body.answer);

// Route answer to the orchestrator
Expand Down
12 changes: 12 additions & 0 deletions assistant/src/tools/calls/call-end.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import { registerTool } from '../registry.js';
import { getCallSession, updateCallSession } from '../../calls/call-store.js';
import { getCallOrchestrator, unregisterCallOrchestrator } from '../../calls/call-state.js';
import { activeRelayConnections } from '../../calls/relay-server.js';
import { TwilioConversationRelayProvider } from '../../calls/twilio-provider.js';
import { getLogger } from '../../util/logger.js';

const log = getLogger('call-end');
Expand Down Expand Up @@ -61,6 +62,17 @@ class CallEndTool implements Tool {

log.info({ callSessionId, reason }, 'Ending call');

// Terminate the call via the provider API so Twilio hangs up,
// even if the relay WebSocket is not connected.
if (session.providerCallSid) {
try {
const provider = new TwilioConversationRelayProvider();
await provider.endCall(session.providerCallSid);
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot Feb 19, 2026

Choose a reason for hiding this comment

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

P1 Badge Avoid blocking teardown on provider hangup call

This await provider.endCall(...) runs before relay/orchestrator cleanup, so call termination now depends on Twilio responsiveness; when Twilio/network is slow or stalls, call_end can hang and never reach the local cleanup path, leaving the active relay/orchestrator alive instead of ending promptly. Because endCall uses a plain fetch without an abort timeout, this regression can surface during provider incidents and prevents the tool from reliably stopping calls.

Useful? React with 👍 / 👎.

} catch (endErr) {
log.warn({ err: endErr, callSessionId, callSid: session.providerCallSid }, 'Failed to terminate call via provider API — proceeding with cleanup');
}
}

// End the relay connection if active
const relayConnection = activeRelayConnections.get(callSessionId);
if (relayConnection) {
Expand Down
15 changes: 15 additions & 0 deletions assistant/src/tools/calls/call-start.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,10 @@ class CallStartTool implements Tool {

const callContext = input.context as string | undefined;

// Create session outside the try block so it's available in the catch block
// for marking as failed if the provider call fails.
let sessionId: string | null = null;

try {
const config = getTwilioConfig();
const provider = new TwilioConversationRelayProvider();
Expand All @@ -81,6 +85,7 @@ class CallStartTool implements Tool {
toNumber: phoneNumber,
task: callContext ? `${task}\n\nContext: ${callContext}` : task,
});
sessionId = session.id;

log.info({ callSessionId: session.id, to: phoneNumber, task }, 'Initiating outbound call');

Expand Down Expand Up @@ -111,6 +116,16 @@ class CallStartTool implements Tool {
} catch (err) {
const msg = err instanceof Error ? err.message : String(err);
log.error({ err, phoneNumber }, 'Failed to initiate call');

// Mark the session as failed so it doesn't stay in 'initiated' state
if (sessionId) {
updateCallSession(sessionId, {
status: 'failed',
endedAt: Date.now(),
Comment on lines +122 to +124
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot Feb 19, 2026

Choose a reason for hiding this comment

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

P2 Badge Don’t mark session failed after post-initiation errors

The new catch-path update applies to any exception after sessionId is assigned, including failures that happen after Twilio has already accepted the outbound call (for example, an error persisting providerCallSid), so we can persist status: 'failed'/endedAt while the call is actually live. That corrupts lifecycle state and can hide an active call from getActiveCallSessionForConversation, so this failed transition should be limited to genuine initiation failures.

Useful? React with 👍 / 👎.

lastError: msg,
});
}

return { content: `Error initiating call: ${msg}`, isError: true };
}
}
Expand Down
Loading