fix: use parentID matching instead of ID ordering for prompt loop exit and message rendering#14307
fix: use parentID matching instead of ID ordering for prompt loop exit and message rendering#14307MakonnenMak wants to merge 2 commits intoanomalyco:devfrom
Conversation
|
Thanks for updating your PR! It now meets our contributing guidelines. 👍 |
|
The following comment was made by an LLM, it may be inaccurate: Found one potentially related PR: Related PR:
The current PR (14307) takes a different approach to solving clock skew problems by using parentID matching instead of relying on timestamp-based ID ordering. PR #11869 may have attempted a fix at the ID generation level rather than the matching logic level. You may want to check if #11869 was merged or closed to understand the context. |
| if (input.assistantMessage.error) return "stop" | ||
| return "continue" | ||
| const exitReason = needsCompaction ? "compact" : blocked ? "stop" : input.assistantMessage.error ? "stop" : "continue" | ||
| return exitReason as any |
There was a problem hiding this comment.
We should avoid using 'any' for typing. See Contributing guidelines
There was a problem hiding this comment.
Fixed on latest commit thanks for flagging @alexyaroshuk . Unsure how to repro the windows e2e failure though, might it be flaky?
There was a problem hiding this comment.
Seems like it was, new run with rebase passed 🙏🏾
0e734cb to
6f428ed
Compare
6f428ed to
fc258ea
Compare
|
There was a conflict with the base dev branch so I had to fix and rebase. Done ✅ |
|
can't see any more issues, lgtm |
|
@adamdotdevin wanted to bump for a review if you have some time 🙏🏾 currently blocking us from using opencode web on a coder.com remote dev env |
|
@adamdotdevin bumping again, would be great to get this out |
|
@rekram1-node want to get your eyes on this as well |
…r prompt loop exit and message rendering
…r prompt loop exit and message rendering
…r prompt loop exit and message rendering
…r prompt loop exit and message rendering
…r prompt loop exit and message rendering
…r prompt loop exit and message rendering
…r prompt loop exit and message rendering
…r prompt loop exit and message rendering
…r prompt loop exit and message rendering
…r prompt loop exit and message rendering
…r prompt loop exit and message rendering
…r prompt loop exit and message rendering
…r prompt loop exit and message rendering
…r prompt loop exit and message rendering
…r prompt loop exit and message rendering
…r prompt loop exit and message rendering
…r prompt loop exit and message rendering
…r prompt loop exit and message rendering
…r prompt loop exit and message rendering
…r prompt loop exit and message rendering
…r prompt loop exit and message rendering
…r prompt loop exit and message rendering
|
Sorry for repushing mid-review. There was a merge conflict with dev so repushed with a small import update |
|
Hey @adamdotdevin @rekram1-node, could you take another look when you get a chance? Thanks! |
|
Hey @adamdotdevin @rekram1-node, wanted to bump again. Thank you 🙏🏾 |
|
We independently arrived at the same fix (parentID matching instead of ID ordering) and adopted this approach in our fork. The |
|
Sorry for the noise @rekram1-node, could you take a look at this when you get a chance? It's a pretty rough experience right now with remote envs that run into this client/server clock skew problem. |
|
Bumping again if possible @rekram1-node , happy to chat it through on the discord as well for faster communication loop 🙏🏾 |
|
This PR cannot be merged into the beta branch due to: Merge conflicts with dev branch Please resolve this issue to include this PR in the next beta release. |
…t and message rendering When the client clock is ahead of the server, user message IDs (generated client-side) sort after assistant message IDs (generated server-side). This broke the prompt loop exit check and the UI message pairing logic. - Extract shouldExitLoop() into a pure function that uses parentID matching instead of relying on ID ordering - Extract findAssistantMessages() with forward+backward scan to handle messages sorted out of expected order due to clock skew - Remove debug console.log statements added during investigation - Add tests for both extracted functions Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
This PR cannot be merged into the beta branch due to: Merge failed Please resolve this issue to include this PR in the next beta release. |
1 similar comment
|
This PR cannot be merged into the beta branch due to: Merge failed Please resolve this issue to include this PR in the next beta release. |
|
@sjqwert on it |
…t-loop-exit # Conflicts: # packages/opencode/src/session/prompt.ts
Issue for this PR
Closes #14236
Type of change
What does this PR do?
Fixes #14236
Large # of line changes test additions.
When the client and server clocks are out of sync (e.g. browser on a local machine, server on a remote host), client-generated message IDs can sort after server-generated assistant IDs because IDs are timestamp-based. This caused two bugs:
SessionTurnonly scanned forward from the user message index to find assistant replies. With skewed IDs, the assistant sorts before the user message and was never found.Fix: Both checks now use
parentIDmatching instead of relying on ID/timestamp ordering:shouldExitLoop()inprompt.ts— checkslastAssistant.parentID === lastUser.idregardless of ID sort order.findAssistantMessages()infind-assistant-messages.tsx— scans forward first, then backward if nothing found, matching onparentID.console.logstatements added during investigation.I was running into this on coder.com remote environments where opencode web seemingly takes minutes to get a response back or just infinitely loops.
How did you verify your code works?
shouldExitLoopunit tests (8 cases): normal exit, clock skew exit, tool-calls, unknown, no assistant, no finish, parentID mismatch, no userfindAssistantMessagesunit tests (8 cases): normal ordering, clock skew ordering, no assistant, multiple assistants, parentID mismatch, scan boundaries, invalid indexScreenshots / recordings
N/A — no UI changes, logic-only fix.
Checklist