fix: complete prompts on newly forked sessions - #466
Open
aiguy110 wants to merge 1 commit into
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
thread/unsubscribe, so another ACP process can still load the persisted forkACP lifecycle
The ACP session/fork RFD motivates the method with a client flow that forks, issues additional messages, and closes the fork. It also specifies a response shaped like
session/new. The draft schema describes the returned ID as the newly created forked session and does not require an interveningsession/loadorsession/resume.Therefore a successful
session/forkresponse must produce a session that acceptssession/promptdirectly. Explicitsession/loadremains valid for a persisted/inactive fork, but it is not a required activation step after a successful fork on the same ACP connection.Observed behavior
With codex-acp 1.8.0 and Codex 0.152.1:
session/fork(parentSessionId, cwd, mcpServers)returned a fork session ID.session/prompt(forkSessionId, prompt)was accepted.session/updatenotifications.session/promptrequest remained pending indefinitely.A real-Codex reproduction using the repository
/run-codexharness timed out the direct sequence after 30 seconds with zero updates. The control sequencefork -> load -> promptcompleted in 2.7 seconds.Root cause
The fork implementation intentionally calls app-server
thread/unsubscribeafterthread/forkto release the writer for clients such as AIR that may load the fork through another ACP process. codex-acp nevertheless installs the returned fork in its local ACP session map as if it were immediately promptable.Before this change, a direct prompt started a turn on that unsubscribed thread. Codex executed it, but app-server delivered no thread notifications to this connection.
CodexAppServerClient.runTurnconsequently never observedturn/completed, so codex-acp emitted no answer updates and never resolved the ACP prompt.This PR keeps the unsubscribe behavior. The ACP session state records that a new fork needs resubscription, and the first direct prompt resumes the thread before registering prompt handlers and starting the turn. An explicit
session/loadcreates an already-subscribed session state and does not take the lazy-resume path.Regression test
The new test:
agent_message_chunkfor the fork;end_turn;Verification
npm run typechecknpm test— 487 passed, 26 skippedfork -> prompt— completed in 3.3 seconds after the fixfork -> load -> prompt— completed in 3.8 seconds after the fix