feat(codex): add message rewind and fork controls#775
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Findings
- [Major] Preserve the steer message localId in HAPI storage —
clientUserMessageIdis generated and sent to Codex, butsession.sendUserMessage(text)emits the visible user bubble without the same top-level socketlocalId. The hub persistslocal_idonly from the top-level socket payload (hub/src/socket/handlers/cli/sessionHandlers.ts:84,hub/src/socket/handlers/cli/sessionHandlers.ts:108), and the UI enables the new Rewind/Fork controls only when a user message hasmetadata.custom.localId(web/src/components/AssistantChat/messages/UserMessage.tsx:60). As a result, user steer messages become visible but cannot be targeted by the PR's new localId-based controls, and later rollback/fork lookups for that steer turn lose the mapping. Evidencecli/src/codex/codexRemoteLauncher.ts:2921
Suggested fix:// ApiSessionClient.sendUserMessage should forward the id as the socket localId, // then pass the steer id through from the Codex RPC handler. session.sendUserMessage(text, undefined, clientUserMessageId);
Questions
- None
Summary
- Review mode: initial
- One issue found: steer-created user messages lose their HAPI localId, which breaks the new rewind/fork affordances for those messages.
Testing
- Not run (automation)
HAPI Bot
| }, { | ||
| signal: this.abortController.signal | ||
| }); | ||
| session.sendUserMessage(text); |
There was a problem hiding this comment.
[Major] Preserve the steer message localId in HAPI storage
clientUserMessageId is generated and sent to Codex, but session.sendUserMessage(text) emits the visible user bubble without the same top-level socket localId. The hub persists local_id only from the top-level socket payload (hub/src/socket/handlers/cli/sessionHandlers.ts:84, hub/src/socket/handlers/cli/sessionHandlers.ts:108), and the UI enables the new Rewind/Fork controls only when a user message has metadata.custom.localId (web/src/components/AssistantChat/messages/UserMessage.tsx:60). As a result, steer messages become visible but cannot be targeted by the new localId-based controls, and later rollback/fork lookups for that steer turn lose the mapping.
Suggested fix:
// ApiSessionClient.sendUserMessage should forward the id as the socket localId,
// then pass the steer id through from the Codex RPC handler.
session.sendUserMessage(text, undefined, clientUserMessageId);
|
Thanks, fixed in Verified:
|
There was a problem hiding this comment.
Findings
- [Major] Validate the rollback target before aborting the active turn —
codexRollbackToMessageinterrupts and resets the current turn beforeresolveRollbackPlan()verifies that the requested HAPIlocalIdexists in the current Codex thread. If the lookup fails (for example a local message row exists but the app-server thread cannot map it), the API returns an error while the in-flight Codex turn has already been cancelled and HAPI messages are left unchanged. Evidencecli/src/codex/codexRemoteLauncher.ts:2832
Suggested fix:const plan = await resolveRollbackPlan(threadId, localId); await interruptActiveTurn(); resetCurrentTurnState(); await appServerClient.rollbackThread({ threadId, numTurns: plan.numTurns }, { signal: this.abortController.signal });
Questions
- None
Summary
- Review mode: follow-up after new commits
- The prior steer
localIdissue is addressed in the follow-up commit. One remaining issue: failed rewind validation can still cancel a running Codex turn before proving the requested target is valid.
Testing
- Not run (automation)
HAPI Bot
| const localId = readLocalIdParam(payload); | ||
| return await runCodexOperation(async () => { | ||
| const threadId = await ensureThreadForRpc(); | ||
| await interruptActiveTurn(); |
There was a problem hiding this comment.
[Major] Validate the rollback target before aborting the active turn
codexRollbackToMessage interrupts and resets the current turn before resolveRollbackPlan() verifies that the requested HAPI localId exists in the current Codex thread. If the lookup fails (for example a local message row exists but the app-server thread cannot map it), the API returns an error while the in-flight Codex turn has already been cancelled and HAPI messages are left unchanged.
Suggested fix:
const plan = await resolveRollbackPlan(threadId, localId);
await interruptActiveTurn();
resetCurrentTurnState();
await appServerClient.rollbackThread({
threadId,
numTurns: plan.numTurns
}, {
signal: this.abortController.signal
});
|
Thanks, fixed in Verified:
|
There was a problem hiding this comment.
Findings
- [Major] Mark steer echo messages as invoked —
codexSteerCurrentTurnnow echoes the steer text back into HAPI withsession.sendUserMessage(..., clientUserMessageId), but that path inserts a DB row withinvoked_at = NULLwhenever alocalIdis present (hub/src/store/messages.ts:69). Normal web prompts are later fixed by the queuemessages-consumedack, but steer bypasses the queue afterturn/steer, so no ack is emitted and the UI keeps the already-sent steer message in the queued/pending state across refreshes. Evidencecli/src/codex/codexRemoteLauncher.ts:2921
Suggested fix:session.sendUserMessage(text, undefined, clientUserMessageId); session.client.emitMessagesConsumed([clientUserMessageId]);
Questions
- None
Summary
- Review mode: follow-up after new commits
- The prior rollback validation issue is fixed in the new commit. One remaining issue: steer messages are persisted as uninvoked queued rows because the new local-id echo does not emit the corresponding consumption ack.
Testing
- Not run (automation)
HAPI Bot
| }, { | ||
| signal: this.abortController.signal | ||
| }); | ||
| session.sendUserMessage(text, undefined, clientUserMessageId); |
There was a problem hiding this comment.
[Major] Mark steer echo messages as invoked
codexSteerCurrentTurn now echoes the steer text back into HAPI with session.sendUserMessage(..., clientUserMessageId), but that path inserts a DB row with invoked_at = NULL whenever a localId is present (hub/src/store/messages.ts:69). Normal web prompts are later fixed by the queue messages-consumed ack, but steer bypasses the queue after turn/steer, so no ack is emitted and the UI keeps the already-sent steer message in the queued/pending state across refreshes.
Suggested fix:
session.sendUserMessage(text, undefined, clientUserMessageId);
session.client.emitMessagesConsumed([clientUserMessageId]);
|
Thanks, fixed in Verified:
|
There was a problem hiding this comment.
Findings
- No high-confidence issues found.
Questions
- None
Summary
- Review mode: follow-up after new commits
- No blocking issues found in the latest added/modified lines. The previous steer-message acknowledgement issue is addressed at the current head. Residual risk: full project tests were not run in this automated review pass.
Testing
- Not run (automation)
HAPI Bot
There was a problem hiding this comment.
Findings
- No high-confidence issues found.
Questions
- None
Summary
- Review mode: follow-up after new commits
- No blocking issues found in the latest added/modified lines. Full PR diff reviewed, with the prior bot review and compare diff used as context only. Residual risk: I did not independently run the test suite in this review pass.
Testing
- Not run (automation)
HAPI Bot
1 participant
Summary
Notes
@filemention support; that is split into feat(codex): support file mentions #774.Tests
bun typecheckcd cli && bun test src/codex/utils/appServerConfig.test.ts src/utils/MessageQueue2.test.tscd cli && bun run test src/codex/codexRemoteLauncher.test.ts src/codex/runCodex.test.tscd hub && bun test src/web/routes/sessions.test.ts src/store/messages.test.ts