[codex] Keep model chat and thinking behavior in llama - #890
Merged
Conversation
Contributor
|
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 (4)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughThis PR introduces a GLM 4.7 Jinja chat template fallback into the llama.cpp patch suite and refactors skippy-server's request defaulting to remove embedded reasoning/thinking field handling. The chat template fallback uses vocab-signature detection to enable automatic GLM model support. Concurrently, request defaulting is simplified to stop wiring reasoning fields through shared defaults and to derive chat template options from defaults rather than reasoning configuration. ChangesGLM 4.7 Chat Template Fallback Patch
Remove Embedded Reasoning Handling from Request Defaults
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/skippy-server/src/frontend/tests.rs (1)
1535-1557: ⚡ Quick winAdd a malformed-metadata regression test for fallback behavior.
Current tests cover valid JSON metadata and
None, but notSome(invalid_json). Adding that case will lock in fallback-stop behavior for the parse-failure edge.Suggested test addition
+#[test] +fn generation_stop_values_add_chat_control_fallbacks_when_metadata_is_malformed() { + let metadata = "{invalid-json"; + + let stops = generation_stop_values(None, Some(metadata)); + + assert_eq!( + stops, + vec![ + "<|im_end|>", + "<|im_start|>", + "<|system|>", + "<|user|>", + "<|assistant|>", + "<|observation|>", + ] + ); +}🤖 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/tests.rs` around lines 1535 - 1557, Add a new regression test that verifies the fallback behavior when malformed or invalid JSON metadata is passed to the generation_stop_values function. Create a test function similar to generation_stop_values_add_chat_control_fallbacks_for_chat_metadata but instead of passing valid JSON, pass Some() with an invalid JSON string (for example, a string that cannot be parsed as JSON) as the metadata parameter, and assert that the function returns the expected default stop values when the metadata parsing fails.
🤖 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-server/src/frontend/util.rs`:
- Around line 35-52: The fallback stops are currently only added when JSON
parsing succeeds, leaving the system vulnerable if chat_metadata exists but is
malformed. To fix this, capture whether the original chat_metadata string was
present before attempting JSON parsing, then apply the fallback stops based on
that original presence check rather than the result of the JSON parsing
operation. This ensures CHAT_TEMPLATE_FALLBACK_STOPS are added whenever any
chat_metadata exists, regardless of whether it can be successfully parsed as
JSON.
---
Nitpick comments:
In `@crates/skippy-server/src/frontend/tests.rs`:
- Around line 1535-1557: Add a new regression test that verifies the fallback
behavior when malformed or invalid JSON metadata is passed to the
generation_stop_values function. Create a test function similar to
generation_stop_values_add_chat_control_fallbacks_for_chat_metadata but instead
of passing valid JSON, pass Some() with an invalid JSON string (for example, a
string that cannot be parsed as JSON) as the metadata parameter, and assert that
the function returns the expected default stop values when the metadata parsing
fails.
🪄 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: 1e5de95e-c00f-43cb-a962-52781b3fcd00
📒 Files selected for processing (2)
crates/skippy-server/src/frontend/tests.rscrates/skippy-server/src/frontend/util.rs
Comment on lines
35
to
52
| let chat_metadata = | ||
| chat_metadata.and_then(|metadata| serde_json::from_str::<serde_json::Value>(metadata).ok()); | ||
| let additional_stops = chat_metadata.as_ref().and_then(|value| { | ||
| value | ||
| .get("additional_stops") | ||
| .and_then(serde_json::Value::as_array) | ||
| .cloned() | ||
| }); | ||
| if let Some(stops) = additional_stops { | ||
| values.extend( | ||
| stops | ||
| .iter() | ||
| .filter_map(serde_json::Value::as_str) | ||
| .filter(|value| !value.is_empty()) | ||
| .map(str::to_string), | ||
| ); | ||
| for value in stops.iter().filter_map(serde_json::Value::as_str) { | ||
| push_stop_value(&mut values, value); | ||
| } | ||
| } | ||
| if chat_metadata.is_some() { | ||
| for value in CHAT_TEMPLATE_FALLBACK_STOPS { | ||
| push_stop_value(&mut values, value); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Apply fallback stops when chat metadata exists, even if JSON parsing fails.
Line [35]-[52] currently adds fallback stops only after successful JSON parsing. If chat_metadata is present but malformed, fallback protection is skipped and chat control markers can leak into output again.
Suggested fix
- let chat_metadata =
- chat_metadata.and_then(|metadata| serde_json::from_str::<serde_json::Value>(metadata).ok());
- let additional_stops = chat_metadata.as_ref().and_then(|value| {
+ let has_chat_metadata = chat_metadata.is_some();
+ let parsed_chat_metadata =
+ chat_metadata.and_then(|metadata| serde_json::from_str::<serde_json::Value>(metadata).ok());
+ let additional_stops = parsed_chat_metadata.as_ref().and_then(|value| {
value
.get("additional_stops")
.and_then(serde_json::Value::as_array)
.cloned()
});
@@
- if chat_metadata.is_some() {
+ if has_chat_metadata {
for value in CHAT_TEMPLATE_FALLBACK_STOPS {
push_stop_value(&mut values, value);
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| let chat_metadata = | |
| chat_metadata.and_then(|metadata| serde_json::from_str::<serde_json::Value>(metadata).ok()); | |
| let additional_stops = chat_metadata.as_ref().and_then(|value| { | |
| value | |
| .get("additional_stops") | |
| .and_then(serde_json::Value::as_array) | |
| .cloned() | |
| }); | |
| if let Some(stops) = additional_stops { | |
| values.extend( | |
| stops | |
| .iter() | |
| .filter_map(serde_json::Value::as_str) | |
| .filter(|value| !value.is_empty()) | |
| .map(str::to_string), | |
| ); | |
| for value in stops.iter().filter_map(serde_json::Value::as_str) { | |
| push_stop_value(&mut values, value); | |
| } | |
| } | |
| if chat_metadata.is_some() { | |
| for value in CHAT_TEMPLATE_FALLBACK_STOPS { | |
| push_stop_value(&mut values, value); | |
| } | |
| } | |
| let has_chat_metadata = chat_metadata.is_some(); | |
| let parsed_chat_metadata = | |
| chat_metadata.and_then(|metadata| serde_json::from_str::<serde_json::Value>(metadata).ok()); | |
| let additional_stops = parsed_chat_metadata.as_ref().and_then(|value| { | |
| value | |
| .get("additional_stops") | |
| .and_then(serde_json::Value::as_array) | |
| .cloned() | |
| }); | |
| if let Some(stops) = additional_stops { | |
| for value in stops.iter().filter_map(serde_json::Value::as_str) { | |
| push_stop_value(&mut values, value); | |
| } | |
| } | |
| if has_chat_metadata { | |
| for value in CHAT_TEMPLATE_FALLBACK_STOPS { | |
| push_stop_value(&mut values, value); | |
| } | |
| } |
🤖 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/util.rs` around lines 35 - 52, The fallback
stops are currently only added when JSON parsing succeeds, leaving the system
vulnerable if chat_metadata exists but is malformed. To fix this, capture
whether the original chat_metadata string was present before attempting JSON
parsing, then apply the fallback stops based on that original presence check
rather than the result of the JSON parsing operation. This ensures
CHAT_TEMPLATE_FALLBACK_STOPS are added whenever any chat_metadata exists,
regardless of whether it can be successfully parsed as JSON.
i386
force-pushed
the
codex/skippy-chat-stop-fallback
branch
from
June 21, 2026 09:39
e6c79b6 to
33c36fb
Compare
i386
changed the title
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@third_party/llama.cpp/patches/0109-Add-GLM-chat-template-fallback.patch`:
- Line 83: The template line accessing tc.arguments does not include a null or
existence check, which will cause a template rendering error if the arguments
field is missing or null. Add a conditional check in the Jinja2 template to
verify that tc.arguments exists and is not null before attempting to iterate
over it using the for loop. Only render the argument key-value pairs if the
arguments field is present, otherwise skip that section or provide a safe
fallback.
- Around line 101-103: The conditional expression in the chat template that
outputs `</think>` when `enable_thinking` is false is not a valid GLM-4 control
mechanism. Either add documentation explaining this as a tested workaround with
evidence of empirical validation against actual GLM-4 model behavior, or replace
this pattern with the official GLM-4 control mechanism by removing the orphaned
tag logic and ensuring the `enable_thinking` variable properly controls
reasoning suppression through proper chat template configuration, or verify and
document that llama.cpp's GLM-4 integration specifically requires this pattern
as a necessity.
🪄 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: 70399c0b-c632-4725-8000-f5050b5acbdc
📒 Files selected for processing (1)
third_party/llama.cpp/patches/0109-Add-GLM-chat-template-fallback.patch
i386
force-pushed
the
codex/skippy-chat-stop-fallback
branch
from
June 21, 2026 09:45
33c36fb to
11d396d
Compare
i386
changed the title
i386
changed the title
i386
changed the title
i386
force-pushed
the
codex/skippy-chat-stop-fallback
branch
from
June 21, 2026 10:00
11d396d to
f63e029
Compare
michaelneale
approved these changes
Jun 21, 2026
michaelneale
added a commit
that referenced
this pull request
Jun 23, 2026
* origin/main: update guides for dev loop (#895) Add native MTP generation metadata to layer packages (#888) upgrade iroh to 1.0 (#894) fix(runtime): support relocating shared libs Improve LAN direct-path discovery and connection reliability (#853) Add GLM chat template fallback in llama (#890) # Conflicts: # crates/mesh-llm-config/src/model/built_in_schema.rs # crates/mesh-llm-config/src/validate.rs # crates/mesh-llm-host-runtime/src/inference/skippy/deployment.rs # crates/mesh-llm-host-runtime/src/inference/skippy/materialization.rs # crates/mesh-llm-host-runtime/src/inference/skippy/resolver/speculative.rs # crates/mesh-llm-host-runtime/src/inference/skippy/resolver/translation.rs # crates/mesh-llm-host-runtime/src/inference/skippy/stage/mod.rs # crates/mesh-llm-host-runtime/src/inference/skippy/stage/tests.rs # crates/mesh-llm-host-runtime/src/inference/skippy/stage/types.rs # crates/mesh-llm-host-runtime/src/mesh/mod.rs # crates/mesh-llm-host-runtime/src/mesh/tests.rs # crates/mesh-llm-host-runtime/src/runtime/local.rs # crates/skippy-protocol/proto/stage.proto # crates/skippy-server/src/binary_transport.rs # crates/skippy-server/src/frontend.rs # crates/skippy-server/src/frontend/embedded_execution.rs # crates/skippy-server/src/frontend/embedded_generation.rs # crates/skippy-server/src/frontend/generation_flow.rs # crates/skippy-server/src/frontend/prefix_cache.rs # docs/skippy/CONFIGURATION.md
This was referenced Jul 2, 2026
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
2 participants
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
tokenizer.chat_template.additional_stops) rather than adding Skippy fallback stops.reasoning,reasoning_effort,thinking_budget,enable_thinking,chat_template_kwargs). Skippy now leavesenable_thinkingunset so llama/model template defaults decide.Why
Skippy should not maintain parallel model-family policy for chat templates, stop tokens, or chain-of-thought/thinking behavior. Those semantics belong in llama's chat-template/parser layer, where they can be tied to model metadata, tokenizer markers, template capabilities, and parser metadata.
The observed bug was GLM 4.7 Flash: the GGUF advertises GLM tokenizer markers/EOG tokens but has no
tokenizer.chat_template, so llama's common chat path fell back to ChatML. That rendered conversations with the wrong control tokens and could let generation continue past the expected GLM turn boundary.The fix adds the concrete GLM fallback in llama, while removing the broader Skippy-side behavior that tried to infer thinking policy from request-shaped fields.
Behavior
For GGUFs matching the GLM vocabulary signature:
[gMASK]<|endoftext|><|user|>llama now selects the built-in GLM fallback template before falling back to ChatML.
The fallback template follows the official
zai-org/GLM-4.7-Flashchat template, including theenable_thinking=falsegeneration prompt branch that emits a leading</think>. That is upstream GLM template behavior, not a Skippy workaround.Skippy still:
additional_stopsmetadatareasoning_contentinto OpenAI responses when llama provides itSkippy no longer:
enable_thinkingThinking / CoT Ownership
Thinking behavior stays llama-owned in this path:
enable_thinking: None, so the ABI sendsoverride_enable_thinking=false.reasoning_content, Skippy maps it into the OpenAI response; Skippy does not parse<think>tags itself.Validation
LLAMA_WORKDIR=$(mktemp -d /tmp/mesh-llm-llama.XXXXXX) scripts/prepare-llama.sh pinnedjust buildcargo test -p skippy-server --libcargo check -p skippy-servercargo clippy -p skippy-server --all-targets -- -D warningsProtocol
No mesh protocol changes.
Summary by CodeRabbit
New Features
Changes