Skip to content

Add Codex steer for queued messages#636

Open
NightWatcher314 wants to merge 4 commits into
tiann:mainfrom
NightWatcher314:codex-steer-queued-message
Open

Add Codex steer for queued messages#636
NightWatcher314 wants to merge 4 commits into
tiann:mainfrom
NightWatcher314:codex-steer-queued-message

Conversation

@NightWatcher314
Copy link
Copy Markdown
Contributor

Summary

  • add Codex app-server turn/steer support and a CLI RPC to steer queued messages into the active turn
  • add a hub /messages/:messageId/steer endpoint that marks steered messages invoked and broadcasts messages-consumed
  • add a queued-message “引导” button in the web UI

Verification

  • bun run typecheck:cli
  • bun run typecheck:hub
  • bun run typecheck:web
  • cd cli && bun test src/utils/MessageQueue2.test.ts
  • cd web && bun test src/components/AssistantChat/QueuedMessagesBar.test.tsx

Copy link
Copy Markdown

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

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

Findings

  • [Major] Failed steer requeues isolated commands as normal messages — takeByLocalId() preserves the queued item, including isolate, but the failure path re-adds it with unshift(), whose implementation always writes isolate: false. If turn/steer fails for an isolated queued command such as /clear, /compact, or /goal, the command can later batch with following same-mode messages and change behavior (/clear becomes invalid, /compact gets arguments, goal text can absorb the next prompt). Evidence cli/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-message RPC handler is registered by the Codex remote launcher, but QueuedMessagesBar now 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. Evidence web/src/components/AssistantChat/QueuedMessagesBar.tsx:190, related handler cli/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, but bun is not installed in this runner.

HAPI Bot

Comment thread cli/src/codex/codexRemoteLauncher.ts Outdated
Comment thread web/src/components/AssistantChat/QueuedMessagesBar.tsx Outdated
Copy link
Copy Markdown

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

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

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): bun is not installed in this runner. git diff --check passed.

HAPI Bot

@NightWatcher314 NightWatcher314 force-pushed the codex-steer-queued-message branch from 507e727 to 6440e79 Compare May 18, 2026 16:02
Copy link
Copy Markdown

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

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

Findings

  • [Minor] Steer is enabled for future-scheduled queued messages that cannot be steered — canSteer only 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 when session.queue.takeByLocalId(localId) finds the item, so clicking 引导 on a future scheduled row returns not-found and gives the user a dead control. Evidence: web/src/components/AssistantChat/QueuedMessagesBar.tsx:215, with related route/queue behavior in hub/src/sync/messageService.ts:401 and cli/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 — QueuedMessagesBar already reads translations via useTranslation, 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): bun is not installed in this runner.
  • git diff --check passed.

HAPI Bot

Comment thread web/src/components/AssistantChat/QueuedMessagesBar.tsx Outdated
Comment thread web/src/components/AssistantChat/QueuedMessagesBar.tsx Outdated
Copy link
Copy Markdown

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

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

Findings

  • [Major] Failed steer can reorder queued messages — takeByLocalId() can remove any queued row, but the failure path always restores it with unshift(). If the user steers the second item in [A, B, C] and turn/steer fails 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 in hub/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 --check passed.
  • Not run: bun test cli/src/utils/MessageQueue2.test.ts and bun test hub/src/web/routes/messages.test.ts (bun is not installed in this runner).

HAPI Bot

});
return { status: 'steered', localId };
} catch (error) {
session.queue.unshift(item.message, item.mode, item.localId, item.isolate);
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[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}
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[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}

Copy link
Copy Markdown

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

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

Findings

  • [Major] Steered messages can remain queued if the RPC ack is lost or times out after turn/steer succeeds — 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 with invoked_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 with unshift(). If [A, B, C] is queued and steering B fails, 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...HEAD passed.
  • Not run: Bun tests/typecheck (bun is not installed in this runner).

HAPI Bot

expectedTurnId: this.currentTurnId,
input: [{ type: 'text', text: item.message }]
});
return { status: 'steered', localId };
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[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);
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[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}
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[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}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant