Pass reasoning format through Skippy chat templates - #947
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR threads a new reasoning-format option from request defaults through runtime and FFI into chat-template rendering, then updates server parsing and streaming to hide or emit reasoning content accordingly. It also broadens Gemma thinking-segment parsing and tests. ChangesReasoning format feature
Gemma thinking-segment tag parsing
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant StageOpenAiBackend
participant SkippyRuntime
participant SkippyFFI as skippy-ffi/native
participant ChatOutputStreamParser
Client->>StageOpenAiBackend: chat_completion_stream(request)
StageOpenAiBackend->>StageOpenAiBackend: chat_template_options(request_defaults)
StageOpenAiBackend->>SkippyRuntime: apply_chat_template_json(reasoning_format)
SkippyRuntime->>SkippyFFI: skippy_apply_chat_template_json(reasoning_format ptr)
SkippyFFI-->>SkippyRuntime: rendered prompt
StageOpenAiBackend->>StageOpenAiBackend: emit_reasoning from template_options
StageOpenAiBackend->>ChatOutputStreamParser: new(emit_reasoning)
ChatOutputStreamParser-->>StageOpenAiBackend: ReasoningDelta events when enabled
StageOpenAiBackend-->>Client: streamed chat completion
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@i386 someone smarter than me needs to review this! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/skippy-server/src/frontend/backend.rs (1)
431-465: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider bundling stream-generation flags into a config struct.
run_generation_streamnow has two adjacent same-typed booleans (parse_chat_output: bool, emit_reasoning: bool) on top of an already#[allow(clippy::too_many_arguments)]-flagged signature. Adjacent identically-typed positional bools are easy to transpose at a call site without a compiler error. Bundlingparse_chat_output/emit_reasoning(and potentiallyinclude_usage) into a small named struct would remove this footgun and shrink the argument list.♻️ Proposed direction
- async fn run_generation_stream( - &self, - prompt: PreparedGenerationPrompt, - max_tokens: GenerationTokenLimit, - stop: Option<openai_frontend::StopSequence>, - sampling: SamplingConfig, - include_usage: bool, - hook_request: Option<ChatCompletionRequest>, - parse_chat_output: bool, - emit_reasoning: bool, - context: OpenAiRequestContext, - ids: OpenAiGenerationIds, - ) -> OpenAiResult<GenerationStream> { + struct ChatOutputStreamOptions { + parse_chat_output: bool, + emit_reasoning: bool, + } + + async fn run_generation_stream( + &self, + prompt: PreparedGenerationPrompt, + max_tokens: GenerationTokenLimit, + stop: Option<openai_frontend::StopSequence>, + sampling: SamplingConfig, + include_usage: bool, + hook_request: Option<ChatCompletionRequest>, + chat_output: ChatOutputStreamOptions, + context: OpenAiRequestContext, + ids: OpenAiGenerationIds, + ) -> OpenAiResult<GenerationStream> {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/skippy-server/src/frontend/backend.rs` around lines 431 - 465, The `run_generation_stream` signature in `backend.rs` has adjacent positional booleans (`parse_chat_output`, `emit_reasoning`, and possibly `include_usage`) that are easy to mix up and contribute to the too-many-arguments smell. Refactor these stream-generation flags into a small named config struct, update `run_generation_stream` to accept that struct, and adjust the `ChatOutputStreamParser::new` call and all call sites to read flags from the new type so the intent is explicit and the parameter list is shorter.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/skippy-ffi/src/lib.rs`:
- Line 3: The ABI patch was bumped for the new chat-template arity, but
`skippy_apply_chat_template_json` can still be resolved on older runtimes and
then called with an incompatible C signature. Keep `ABI_VERSION_PATCH` at the
new value, and add an early runtime ABI check in `crates/skippy-ffi/src/lib.rs`
so pre-28 runtimes are rejected before any wrapper that uses
`skippy_apply_chat_template_json` can execute.
---
Nitpick comments:
In `@crates/skippy-server/src/frontend/backend.rs`:
- Around line 431-465: The `run_generation_stream` signature in `backend.rs` has
adjacent positional booleans (`parse_chat_output`, `emit_reasoning`, and
possibly `include_usage`) that are easy to mix up and contribute to the
too-many-arguments smell. Refactor these stream-generation flags into a small
named config struct, update `run_generation_stream` to accept that struct, and
adjust the `ChatOutputStreamParser::new` call and all call sites to read flags
from the new type so the intent is explicit and the parameter list is shorter.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 09f59d99-666e-4b63-8088-f980d9979a24
📒 Files selected for processing (12)
crates/mesh-llm-ui/src/features/chat/components/thinking-segments.test.tscrates/mesh-llm-ui/src/features/chat/components/thinking-segments.tscrates/skippy-bench/src/token_lengths.rscrates/skippy-ffi/src/lib.rscrates/skippy-prompt/src/prompt_cli/prompt_format.rscrates/skippy-runtime/src/lib.rscrates/skippy-server/src/frontend.rscrates/skippy-server/src/frontend/backend.rscrates/skippy-server/src/frontend/prompting.rscrates/skippy-server/src/frontend/request.rscrates/skippy-server/src/frontend/tests.rsthird_party/llama.cpp/patches/0011-Pass-reasoning-format-through-stage-chat-templates.patch
|
🤖 Context on why this is larger than Goose: Goose already has this knob exposed by its llama.cpp Rust binding: it sets
Mesh's Skippy path had the same conceptual requirement, but the C ABI boundary did not expose |
@michaelneale Yeah, I'd keep this and pass it directly into the template/parser... I think this is where we should pick apart model family markers. We should still keep skippy-server using the parsed chat metadata, so the server can decide if it wants to parse, hide, or expose reasoning before it leaves the OpenAI backend. One thing we should do though is update Some tests that would help here would be to see hidden reasoning markers get stripped out from the content, and that the |
…raction Assisted-by: goose (Claude Sonnet)
|
@ndizazzo how does this look now? |
3 participants
🤖 Opened by AI agent.
Summary:
Validation:
Summary by CodeRabbit