fix: resolve parallel tool-call completions in FIFO order#115
Open
Mingye-Lu wants to merge 1 commit into
Open
Conversation
handle_tool_call_complete and ChildTabPanel::apply_event both used .iter_mut().rev().find(...)/.find_map(...) to locate the still-Running live tool-call entry to resolve, which matches the LAST matching entry in insertion order (LIFO) rather than the first (FIFO). When two parallel calls to the same tool (e.g. two navigate calls to different URLs) are in flight, their completions could get attached to the wrong row, showing the wrong output/status next to the wrong tool-call line. Drop .rev() in both locations so the first still-Running matching entry is resolved first, matching the documented FIFO intent.
Owner
Author
|
@codex review |
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
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
1 participant
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
ReplTuiState::handle_tool_call_completeincrates/tui/src/repl_app/input_editor.rsandChildTabPanel::apply_event'sToolCallCompletearm incrates/tui/src/child_tabs.rs) resolved live tool-call rows via.iter_mut().rev().find(...), which matches the last still-Runningentry with a matching name (LIFO) instead of the first (FIFO) as the surrounding doc comment claims.navigatecalls to different URLs, A started before B), A's completion would get attached to B's live row and vice versa, showing the wrong output/status next to the wrong tool-call line in the transcript..rev()in both locations so the first still-Runningmatching entry (in original insertion order) is resolved first, matching the documented FIFO intent. No behavior change for the common case of a single in-flight call per tool name.Test plan
tool_call_complete_resolves_parallel_same_name_calls_fifoincrates/tui/src/repl_app/tests.rs, asserting the first-startednavigatecall is resolved by the first completion and the second staysRunning.test_tool_call_complete_resolves_parallel_same_name_calls_fifoincrates/tui/src/child_tabs.rs, same scenario throughChildTabPanel::apply_event.cargo fmt -p acrawl-tuicargo test -p acrawl-tui(all 169 tests pass)cargo clippy -p acrawl-tui --all-targets -- -D warnings(clean)