Add Codex steer for queued messages#636
Conversation
There was a problem hiding this comment.
Findings
- [Major] Failed steer requeues isolated commands as normal messages —
takeByLocalId()preserves the queued item, includingisolate, but the failure path re-adds it withunshift(), whose implementation always writesisolate: false. Ifturn/steerfails for an isolated queued command such as/clear,/compact, or/goal, the command can later batch with following same-mode messages and change behavior (/clearbecomes invalid,/compactgets arguments, goal text can absorb the next prompt). Evidencecli/src/codex/codexRemoteLauncher.ts:2303.
Suggested fix:unshift(message: string, mode: T, localId?: string, isolate = false): void { // ... this.queue.unshift({ message, mode, modeHash, localId, isolate }) // ... } session.queue.unshift(item.message, item.mode, item.localId, item.isolate ?? false)
- [Minor] Hide the Codex-only steer action for non-Codex sessions — the only new
steer-queued-messageRPC handler is registered by the Codex remote launcher, butQueuedMessagesBarnow renders the steer button for every session. Queued Claude/Gemini/OpenCode messages will show a control that can only return an RPC-handler-not-registered failure. Evidenceweb/src/components/AssistantChat/QueuedMessagesBar.tsx:190, related handlercli/src/codex/codexRemoteLauncher.ts:2279.
Suggested fix:<QueuedMessagesBar sessionId={props.session.id} api={props.api} canSteer={agentFlavor === "codex"} /> {canSteer ? ( <button type="button" aria-label="Steer queued message" disabled={!canSteerMessage}> ... </button> ) : null}
Questions
- None.
Summary
- Review mode: initial
- Found two issues: failed Codex steer can corrupt isolated queued-command semantics, and the web UI exposes a Codex-only action for unsupported agent flavors.
Testing
- Not run (automation): attempted
bun test cli/src/utils/MessageQueue2.test.ts, butbunis not installed in this runner.
HAPI Bot
There was a problem hiding this comment.
Findings
- No findings.
Questions
- None.
Summary
- Review mode: follow-up after new commits
- No issues found in the latest diff. Prior HAPI Bot findings around isolated requeue and Codex-only steer gating are addressed in the current head. Residual risk: runtime validation was limited by missing local tooling.
Testing
- Not run (automation):
bunis not installed in this runner.git diff --checkpassed.
HAPI Bot
507e727 to
6440e79
Compare
There was a problem hiding this comment.
Findings
- [Minor] Steer is enabled for future-scheduled queued messages that cannot be steered —
canSteeronly checks server echo / pending state, but future-scheduled rows are displayed in the same queued bar while the hub does not emit them to the CLI queue until they mature. The new CLI RPC only succeeds whensession.queue.takeByLocalId(localId)finds the item, so clicking 引导 on a future scheduled row returnsnot-foundand gives the user a dead control. Evidence:web/src/components/AssistantChat/QueuedMessagesBar.tsx:215, with related route/queue behavior inhub/src/sync/messageService.ts:401andcli/src/codex/codexRemoteLauncher.ts:2290.
Suggested fix:const isFutureScheduled = msg.scheduledAt != null && msg.scheduledAt > Date.now() const canSteer = enableSteer && !isFutureScheduled && computeCanCancel({ id: msg.id, localId: msg.localId, isPending: isPending || isSteering, })
- [Minor] The new steer button hardcodes Chinese labels in all locales —
QueuedMessagesBaralready reads translations viauseTranslation, but the added label renders引导/引导中even when the rest of the UI is English. This regresses localization for English users. Evidence:web/src/components/AssistantChat/QueuedMessagesBar.tsx:316.
Suggested fix:<span className="text-xs"> {isSteering ? t('queuedMessages.steering') : t('queuedMessages.steer')} </span>
Questions
- None.
Summary
- Review mode: follow-up after new commits
- Two minor UI regressions found in the new queued-message steer affordance. No blocker/major correctness or security issues found in the changed RPC/route path. Residual risk: local Bun test/typecheck commands could not be run in this runner.
Testing
- Not run (automation):
bunis not installed in this runner. git diff --checkpassed.
HAPI Bot
There was a problem hiding this comment.
Findings
- [Major] Failed steer can reorder queued messages —
takeByLocalId()can remove any queued row, but the failure path always restores it withunshift(). If the user steers the second item in[A, B, C]andturn/steerfails or times out, the queue becomes[B, A, C], so later normal draining sends prompts in a different order. Evidence:cli/src/codex/codexRemoteLauncher.ts:2303.
Suggested fix:const taken = session.queue.takeByLocalId(localId); if (!taken) return { status: 'not-found' }; try { await appServerClient.steerTurn(...); } catch (error) { session.queue.restoreAt(taken.index, taken.item); return { status: 'failed', error: error instanceof Error ? error.message : String(error) }; }
- [Minor] Steer remains enabled for inactive Codex sessions — the new route requires an active session, but the UI only checks flavor and
controlledByUser. Inactive sessions can still display queued scheduled rows, so the button sends a request that the route rejects instead of being disabled/hidden. Evidence:web/src/components/SessionChat.tsx:552, with the active guard inhub/src/web/routes/messages.ts:94.
Suggested fix:enableSteer={props.session.active && agentFlavor === 'codex' && !controlledByUser}
Questions
- None.
Summary
- Review mode: follow-up after new commits
- The previous bot findings were addressed. The remaining risks are a queue-order regression on steer failure and a stale UI affordance for inactive Codex sessions.
Testing
git diff --checkpassed.- Not run:
bun test cli/src/utils/MessageQueue2.test.tsandbun test hub/src/web/routes/messages.test.ts(bunis not installed in this runner).
HAPI Bot
| }); | ||
| return { status: 'steered', localId }; | ||
| } catch (error) { | ||
| session.queue.unshift(item.message, item.mode, item.localId, item.isolate); |
There was a problem hiding this comment.
[MAJOR] This restores a failed steer with unshift(), which changes the original queue order whenever the steered item was not already first. Example: queue [A, B, C], steer B, turn/steer fails, then this line restores B as [B, A, C]; the next normal drain sends prompts out of order.
Suggested fix:
const taken = session.queue.takeByLocalId(localId);
if (!taken) return { status: 'not-found' };
try {
await appServerClient.steerTurn(...);
} catch (error) {
session.queue.restoreAt(taken.index, taken.item);
return { status: 'failed', error: error instanceof Error ? error.message : String(error) };
}| <QueuedMessagesBar | ||
| sessionId={props.session.id} | ||
| api={props.api} | ||
| enableSteer={agentFlavor === 'codex' && !controlledByUser} |
There was a problem hiding this comment.
[MINOR] The steer route below requires an active session, but this prop enables the button for every remote Codex session regardless of props.session.active. Inactive sessions can still show queued scheduled rows, so the user gets an enabled control that can only fail at the API layer.
Suggested fix:
enableSteer={props.session.active && agentFlavor === 'codex' && !controlledByUser}There was a problem hiding this comment.
Findings
- [Major] Steered messages can remain queued if the RPC ack is lost or times out after
turn/steersucceeds — the CLI removes the item and only the hub-side RPC success path marks it invoked. Because both the hub RPC and nested app-server steer use 30s timeouts, a near-timeout success can leave the DB row withinvoked_at = NULL, so the UI can offer the same prompt again even though Codex already received it. Evidence:cli/src/codex/codexRemoteLauncher.ts:2301,hub/src/sync/syncEngine.ts:365.
Suggested fix:await appServerClient.steerTurn({ threadId: this.currentThreadId, expectedTurnId: this.currentTurnId, input: [{ type: 'text', text: item.message }] }); session.queue.onBatchConsumed?.([localId]); return { status: 'steered', localId };
- [Major] Failed steer can reorder queued messages —
takeByLocalId()can remove any queued item, but the failure path restores it withunshift(). If[A, B, C]is queued and steeringBfails, normal draining later sends[B, A, C]. Evidence:cli/src/codex/codexRemoteLauncher.ts:2303.
Suggested fix:const taken = session.queue.takeByLocalId(localId); if (!taken) return { status: 'not-found' }; try { await appServerClient.steerTurn(...); return { status: 'steered', localId }; } catch (error) { session.queue.restoreAt(taken.index, taken.item); return { status: 'failed', error: error instanceof Error ? error.message : String(error) }; }
- [Minor] Steer remains enabled for inactive Codex sessions — the route requires an active session, but this prop ignores
props.session.active, so inactive sessions can show an enabled control that can only fail at the API layer. Evidence:web/src/components/SessionChat.tsx:552,hub/src/web/routes/messages.ts:99.
Suggested fix:enableSteer={props.session.active && agentFlavor === 'codex' && !controlledByUser}
Questions
- None.
Summary
- Review mode: follow-up after new commits
- The queued-message steer path still has correctness risks around persistence, queue order, and inactive-session UI state.
Testing
git diff --check origin/main...HEADpassed.- Not run: Bun tests/typecheck (
bunis not installed in this runner).
HAPI Bot
| expectedTurnId: this.currentTurnId, | ||
| input: [{ type: 'text', text: item.message }] | ||
| }); | ||
| return { status: 'steered', localId }; |
There was a problem hiding this comment.
[MAJOR] After a successful turn/steer, this path returns an RPC response and relies on the hub to mark the queued row invoked. If the Socket.IO ack is lost or the hub-side 30s RPC timeout wins just as the nested turn/steer succeeds, the item has already been removed from the CLI queue but the DB row stays queued, so the UI can offer/send the same prompt again.
Suggested fix:
await appServerClient.steerTurn({
threadId: this.currentThreadId,
expectedTurnId: this.currentTurnId,
input: [{ type: 'text', text: item.message }]
});
session.queue.onBatchConsumed?.([localId]);
return { status: 'steered', localId };| }); | ||
| return { status: 'steered', localId }; | ||
| } catch (error) { | ||
| session.queue.unshift(item.message, item.mode, item.localId, item.isolate); |
There was a problem hiding this comment.
[MAJOR] This restores a failed steer with unshift(), which changes the original queue order whenever the steered item was not already first. Example: queue [A, B, C], steer B, turn/steer fails, then this line restores B as [B, A, C]; the next normal drain sends prompts out of order.
Suggested fix:
const taken = session.queue.takeByLocalId(localId);
if (!taken) return { status: 'not-found' };
try {
await appServerClient.steerTurn(...);
return { status: 'steered', localId };
} catch (error) {
session.queue.restoreAt(taken.index, taken.item);
return { status: 'failed', error: error instanceof Error ? error.message : String(error) };
}| <QueuedMessagesBar | ||
| sessionId={props.session.id} | ||
| api={props.api} | ||
| enableSteer={agentFlavor === 'codex' && !controlledByUser} |
There was a problem hiding this comment.
[MINOR] The steer route requires an active session, but this prop enables the button for every remote Codex session regardless of props.session.active. Inactive sessions can still show queued rows, so the user gets an enabled control that can only fail at the API layer.
Suggested fix:
enableSteer={props.session.active && agentFlavor === 'codex' && !controlledByUser}
Summary
turn/steersupport and a CLI RPC to steer queued messages into the active turn/messages/:messageId/steerendpoint that marks steered messages invoked and broadcastsmessages-consumedVerification
bun run typecheck:clibun run typecheck:hubbun run typecheck:webcd cli && bun test src/utils/MessageQueue2.test.tscd web && bun test src/components/AssistantChat/QueuedMessagesBar.test.tsx