feat(tui): FauxStep::Factory for live request-shape assertions#2107
Open
mvanhorn wants to merge 1 commit into
Open
feat(tui): FauxStep::Factory for live request-shape assertions#2107mvanhorn wants to merge 1 commit into
mvanhorn wants to merge 1 commit into
Conversation
Closes Hmbown#2074 Adds FauxStep::Factory(Box<dyn Fn(&MessageRequest) -> CannedTurn + Send + Sync>) to MockLlmClient. When a Factory step is dequeued, its closure runs against the real outgoing MessageRequest before the response stream is built, so any assert! panic surfaces directly from the client call instead of later in stream polling. Internal storage moves from VecDeque<CannedTurn> to VecDeque<FauxStep>, but every existing public method keeps working: - MockLlmClient::new(Vec<CannedTurn>) wraps each turn in FauxStep::Canned. - push_turn(CannedTurn) appends as FauxStep::Canned. Adds push_factory(closure) for tests that want the Factory branch. Doc comment on the Factory variant captures the DeepSeek V4 thinking-mode tool-call invariant (the v0.4.9-v0.5.1 reasoning_content drop that produced HTTP 400 on follow-up turns). Adds: - crates/tui/tests/reasoning_content_replayed_after_tool_call.rs — a regression test whose factory asserts the assistant tool-call turn carries a Thinking content block after a thinking + tool-call round. - An additional unit test in mock.rs covering create_message_synthesizes_from_factory_turn. All 20 tests in the new file pass, and the existing integration_mock_llm suite (27 tests) is unchanged.
Contributor
There was a problem hiding this comment.
Code Review
This pull request enhances the MockLlmClient by introducing FauxStep, an enum that allows for both static canned responses and dynamic response generation via a factory closure. This update enables more sophisticated testing by allowing assertions on outgoing MessageRequest objects directly within the mock. A new integration test was also added to ensure that reasoning_content is preserved in follow-up requests after tool calls, specifically targeting DeepSeek V4 requirements. I have no feedback to provide.
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.
Closes #2074.
Shape
When a
Factorystep is dequeued, its closure runs against the real outgoingMessageRequestbefore the response stream is built, so anyassert!panic surfaces directly from the client call rather than later in stream polling. No change to existing canned-turn callers.Public surface
MockLlmClient::new(Vec<CannedTurn>)andpush_turn(CannedTurn)keep working — they wrap each turn inFauxStep::Canned.push_factory(closure)is new, for tests that want the Factory branch.FauxStep::Factorycaptures the V4 thinking-mode tool-call invariant (the v0.4.9-v0.5.1reasoning_contentdrop that produced HTTP 400 on follow-up turns).Tests
crates/tui/tests/reasoning_content_replayed_after_tool_call.rswhose factory asserts the assistant tool-call turn carries aThinkingcontent block after a thinking + tool-call round. Mirrors the#[path = "..."]test-support pattern already used byintegration_mock_llm.rs.mock.rs(create_message_synthesizes_from_factory_turn) for the non-streaming path.integration_mock_llmsuite (27 tests) is unchanged and passes.Known limitation
The new regression test asserts against a follow-up request the test itself assembles (with
assistant_thinking_tool_call(...)), not one produced by the real turn-loop. That matches the issue's "Acceptance" wording (request.messages.last().reasoning_content.is_some()) and theintegration_mock_llm.rsdoc-comment which already notes the engine still holds a concreteOption<DeepSeekClient>rather thanArc<dyn LlmClient>— until that wiring lands, the mock can't be wedged into the real turn-loop. This PR delivers theFauxStep::Factoryinfrastructure the issue asks for; the engine-level wiring is the follow-up theintegration_mock_llm.rsignored tests already track.AI was used for assistance.