feat(inkling): add end-to-end SMG support - #1926
Conversation
Signed-off-by: lightseek-bot <243258330+lightseek-bot@users.noreply.github.com>
github-actions
Bot
added
grpc
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds end-to-end Inkling support across multimodal preprocessing, model registries, reasoning and tool parsers, chat rendering, gateway routing, and request handling. It also adds explicit multimodal feature spans, Lanczos resizing, reasoning-effort deserialization, and regression coverage. ChangesInkling multimodal processing
Inkling reasoning and tool parsers
Gateway and request integration
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
|
👋 The PR description doesn't fully follow
Please update the PR description so reviewers have the context they need. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces comprehensive support for the Inkling model family, adding dedicated audio and image preprocessors, a TML typed-content reasoning parser, and a TML JSON tool-call parser. It also updates the model gateway and protocols to handle Inkling-specific configurations, such as preserving special tokens and mapping reasoning effort levels. The review feedback suggests several robustness improvements to prevent division-by-zero and undefined behavior, specifically by checking for empty slices in normalize_audio_rms, ensuring float finiteness in exact_sample_count, and using a small threshold for subnormal floats in pil_sinc.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| fn normalize_audio_rms(samples: &mut [f32], floor: f64) -> Result<(), TransformError> { | ||
| if !floor.is_finite() || floor < 0.0 { | ||
| return Err(TransformError::ShapeError(format!( | ||
| "audio_rms_norm_floor must be finite and non-negative, got {floor}" | ||
| ))); | ||
| } | ||
| if floor == 0.0 { | ||
| return Ok(()); | ||
| } |
There was a problem hiding this comment.
To prevent potential division-by-zero panics or NaN values if samples is empty, normalize_audio_rms should explicitly check if samples is empty and return early.
| fn normalize_audio_rms(samples: &mut [f32], floor: f64) -> Result<(), TransformError> { | |
| if !floor.is_finite() || floor < 0.0 { | |
| return Err(TransformError::ShapeError(format!( | |
| "audio_rms_norm_floor must be finite and non-negative, got {floor}" | |
| ))); | |
| } | |
| if floor == 0.0 { | |
| return Ok(()); | |
| } | |
| fn normalize_audio_rms(samples: &mut [f32], floor: f64) -> Result<(), TransformError> { | |
| if !floor.is_finite() || floor < 0.0 { | |
| return Err(TransformError::ShapeError(format!( | |
| "audio_rms_norm_floor must be finite and non-negative, got {floor}" | |
| ))); | |
| } | |
| if floor == 0.0 || samples.is_empty() { | |
| return Ok(()); | |
| } |
| fn exact_sample_count(value: f64, name: &str) -> Result<usize, TransformError> { | ||
| let rounded = value.round(); | ||
| if (value - rounded).abs() > 1e-6 { | ||
| return Err(TransformError::ShapeError(format!( | ||
| "{name} must resolve to an integer sample count, got {value}" | ||
| ))); | ||
| } | ||
| if rounded <= 0.0 { | ||
| return Err(TransformError::ShapeError(format!( | ||
| "{name} must be positive, got {rounded}" | ||
| ))); | ||
| } | ||
| Ok(rounded as usize) | ||
| } |
There was a problem hiding this comment.
If value is NaN or Infinity, exact_sample_count will bypass the error checks and attempt to cast NaN to usize, which results in 0 (or undefined behavior in older Rust versions). To ensure robustness, explicitly check that value is finite at the beginning of the function.
fn exact_sample_count(value: f64, name: &str) -> Result<usize, TransformError> {
if !value.is_finite() {
return Err(TransformError::ShapeError(format!(
"{name} must be a finite number, got {value}"
)));
}
let rounded = value.round();
if (value - rounded).abs() > 1e-6 {
return Err(TransformError::ShapeError(format!(
"{name} must resolve to an integer sample count, got {value}"
)));
}
if rounded <= 0.0 {
return Err(TransformError::ShapeError(format!(
"{name} must be positive, got {rounded}"
)));
}
Ok(rounded as usize)
}| fn pil_sinc(x: f64) -> f64 { | ||
| if x == 0.0 { | ||
| 1.0 | ||
| } else { | ||
| let x = x * PI; | ||
| x.sin() / x | ||
| } | ||
| } |
There was a problem hiding this comment.
For extremely small non-zero values of x (e.g., subnormal floats), x * PI can underflow to 0.0, leading to a division-by-zero or NaN when computing x.sin() / x. Using a small threshold (like 1e-9) for the approximation of sinc(x) ≈ 1.0 avoids this numerical instability.
fn pil_sinc(x: f64) -> f64 {
if x.abs() < 1e-9 {
1.0
} else {
let pix = x * PI;
pix.sin() / pix
}
}![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b80dbd8a63
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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".
| pub(crate) fn reasoning_parser_requires_special_tokens( | ||
| reasoning_parser_factory: &ReasoningParserFactory, | ||
| configured_parser: Option<&str>, | ||
| model: &str, | ||
| ) -> bool { | ||
| create_reasoning_parser(reasoning_parser_factory, configured_parser, model).is_some_and( |
There was a problem hiding this comment.
For checkpoints served under a local ID that does not contain inkling, prompt formatting now treats the request as TML via has_tml_token_signature, but this helper can only select the Inkling parser from the configured parser or model name. In that context the preparation checks return false, skip_special_tokens remains enabled, and the response path never creates the Inkling reasoning parser, so TML reasoning/control frames can be stripped or leaked into normal assistant text. Please carry the tokenizer/config TML signal into the special-token/parser selection as well.
Useful? React with 👍 / 👎.
| registry.map_model("inkling*", "inkling"); | ||
| registry.map_model("Inkling*", "inkling"); |
There was a problem hiding this comment.
When no explicit --tool-call-parser is configured, tool parsing depends on resolve_model_to_parser, which only exact-matches or prefix-matches patterns ending in *. A served model such as org/Inkling-Chat is treated as TML elsewhere in this change, but it does not start with either of these patterns, so check_tool_parser_availability returns false and TML <|content_invoke_tool_json|> frames are skipped instead of becoming OpenAI tool calls. Add a namespaced/contains-style mapping or align this with the other Inkling detectors.
Useful? React with 👍 / 👎.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| const STREAM_CONTROL_TOKENS: [&str; 7] = [ | ||
| TOOL_CALL_JSON_START, | ||
| TOOL_CALL_TEXT_START, | ||
| MESSAGE_MODEL, | ||
| CONTENT_TEXT, | ||
| CONTENT_THINKING, | ||
| END_MESSAGE, | ||
| MODEL_END_SAMPLING, | ||
| ]; | ||
|
|
||
| const HEADER_CONTROL_TOKENS: [&str; 7] = [ | ||
| TOOL_CALL_JSON_START, | ||
| TOOL_CALL_TEXT_START, | ||
| MESSAGE_MODEL, | ||
| CONTENT_TEXT, | ||
| CONTENT_THINKING, | ||
| END_MESSAGE, | ||
| MODEL_END_SAMPLING, | ||
| ]; |
There was a problem hiding this comment.
🟡 Nit: STREAM_CONTROL_TOKENS and HEADER_CONTROL_TOKENS are identical arrays. Unless these are expected to diverge in a near-term follow-up, they could share one constant to remove the duplication.
| const STREAM_CONTROL_TOKENS: [&str; 7] = [ | |
| TOOL_CALL_JSON_START, | |
| TOOL_CALL_TEXT_START, | |
| MESSAGE_MODEL, | |
| CONTENT_TEXT, | |
| CONTENT_THINKING, | |
| END_MESSAGE, | |
| MODEL_END_SAMPLING, | |
| ]; | |
| const HEADER_CONTROL_TOKENS: [&str; 7] = [ | |
| TOOL_CALL_JSON_START, | |
| TOOL_CALL_TEXT_START, | |
| MESSAGE_MODEL, | |
| CONTENT_TEXT, | |
| CONTENT_THINKING, | |
| END_MESSAGE, | |
| MODEL_END_SAMPLING, | |
| ]; | |
| const CONTROL_TOKENS: [&str; 7] = [ | |
| TOOL_CALL_JSON_START, | |
| TOOL_CALL_TEXT_START, | |
| MESSAGE_MODEL, | |
| CONTENT_TEXT, | |
| CONTENT_THINKING, | |
| END_MESSAGE, | |
| MODEL_END_SAMPLING, | |
| ]; |
There was a problem hiding this comment.
Thorough review of all 37 changed files across 7 crates and the gateway. The Inkling integration is well-structured — it follows existing model-family patterns (registry, parsers, multimodal processors) cleanly, and the test coverage is solid (golden fingerprint tests for audio, streaming split-point exhaustive tests for both parsers, explicit feature-range boundary tests for multimodal expansion).
Key design decisions that checked out:
chat_completions_api_requestmarker — properly un-spoofable (skip_serializing + ignore deserializer), ensures TML reasoning_effort defaults only apply to the public Chat endpoint.requires_special_tokens()trait method — clean extension point that correctly gatesskip_special_tokens=falseacross all three response paths (chat preparation, messages preparation, streaming).explicit_feature_ranges— solid validation (non-empty, positive lengths, no overlaps, bounds-checked) for the new feature-range mechanism that lets Inkling disambiguate structural tokens from encoder positions.MediaPartOrder::Authored— correctly preserves TML's content-part ordering while keeping the existing vLLM-compatibleMediaFirstdefault for all other models.
Findings: 0 🔴 Important, 2 🟡 Nit, 0 🟣 Pre-existing
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 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/multimodal/src/audio/processors/inkling.rs`:
- Around line 65-69: Validate the configured dMel bounds after applying the
`dmel_min_value` and `dmel_max_value` values and before the downstream `clamp`
call. In the surrounding audio processing flow, detect when the minimum exceeds
the maximum and return `TransformError` instead of allowing `f64::clamp` to
panic; preserve normal processing for valid bounds.
In `@crates/multimodal/src/vision/processors/inkling.rs`:
- Around line 270-276: Update calculate_num_tokens to guard against a zero
patch_size before calling scaled_image_dimensions or patch_grid, reusing the
validation guard used by preprocess where possible. Return the established safe
fallback for invalid patch sizes, while preserving the existing token
calculation for valid configurations.
In `@crates/multimodal/src/vision/transforms.rs`:
- Around line 820-875: Add Lanczos regression tests mirroring
resize_bicubic_pil_rgb_matches_dynamic_path and
resize_bicubic_pil_rgb_skips_identity_axes_bit_exactly. Cover byte-for-byte
parity between resize_lanczos_pil and resize_pil_bytes using
PilResizeFilter::Lanczos, plus exact preservation when either resize axis is
unchanged, including both identity-axis cases.
In `@crates/protocols/src/chat.rs`:
- Around line 202-209: Update the schema metadata for the `reasoning_effort`
field in the request type so its generated `JsonSchema` advertises both string
and numeric JSON representations, matching `deserialize_reasoning_effort`. Add a
regression test that verifies the generated schema accepts and describes both
forms while preserving the public Rust type and existing deserialization
behavior.
In `@crates/tool_parser/src/parsers/inkling.rs`:
- Around line 305-307: Update the frame parsing logic around
complete_json_object_len to detect <|end_message|> terminators outside JSON
strings when the JSON object remains incomplete, then transition into the
existing discard/recovery path instead of continuing to accumulate chunks.
Preserve normal parsing when the terminator is absent or appears inside a JSON
string, and add a regression test covering a terminator split across chunks.
- Around line 284-329: The partial-JSON branch in the Inkling parser emits a
tool-name event before the complete call is validated, leaving a stale event
when parse_tool_call_json rejects malformed arguments. Move name emission out of
that branch and emit both the name and parameters only after
parse_tool_call_json succeeds, while preserving the current_tool_name tracking
needed for streaming chunks.
In `@model_gateway/src/routers/grpc/regular/streaming.rs`:
- Around line 1694-1708: Split the combined `separate_reasoning` decision into
`parse_reasoning` and `emit_reasoning`: keep parser activation whenever
`reasoning_requires_special_tokens` is true, but set `emit_reasoning` only for
`ThinkingConfig::Enabled` or `Adaptive`. Update the downstream reasoning
emission logic around lines 1844-1870 to guard thinking-block deltas with
`emit_reasoning`, while preserving the parser’s filtered `normal_text` output
when thinking is explicitly disabled.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: d923c6da-25a1-4f3a-935e-c598f5f06bd0
📒 Files selected for processing (37)
crates/multimodal/src/audio/mod.rscrates/multimodal/src/audio/processors/inkling.rscrates/multimodal/src/audio/processors/mod.rscrates/multimodal/src/registry/inkling.rscrates/multimodal/src/registry/mod.rscrates/multimodal/src/types.rscrates/multimodal/src/vision/processor.rscrates/multimodal/src/vision/processors/inkling.rscrates/multimodal/src/vision/processors/mod.rscrates/multimodal/src/vision/transforms.rscrates/multimodal/tests/fixtures/golden/inkling_preprocess_fingerprints.jsoncrates/multimodal/tests/inkling_preprocess_golden.rscrates/protocols/src/chat.rscrates/reasoning_parser/src/factory.rscrates/reasoning_parser/src/lib.rscrates/reasoning_parser/src/parsers/inkling.rscrates/reasoning_parser/src/parsers/mod.rscrates/reasoning_parser/src/traits.rscrates/tool_parser/src/factory.rscrates/tool_parser/src/lib.rscrates/tool_parser/src/parsers/inkling.rscrates/tool_parser/src/parsers/mod.rscrates/tool_parser/tests/tool_parser_inkling.rsgrpc_servicer/smg_grpc_servicer/tokenspeed/servicer.pymodel_gateway/src/routers/grpc/context.rsmodel_gateway/src/routers/grpc/multimodal/process.rsmodel_gateway/src/routers/grpc/regular/processor.rsmodel_gateway/src/routers/grpc/regular/responses/conversions.rsmodel_gateway/src/routers/grpc/regular/stages/chat/preparation.rsmodel_gateway/src/routers/grpc/regular/stages/messages/preparation.rsmodel_gateway/src/routers/grpc/regular/streaming.rsmodel_gateway/src/routers/grpc/router.rsmodel_gateway/src/routers/grpc/utils/chat_utils.rsmodel_gateway/src/routers/grpc/utils/mod.rsmodel_gateway/src/routers/grpc/utils/parsers.rsmodel_gateway/src/server.rsmodel_gateway/tests/api/request_formats_test.rs
| if let Some(v) = audio_config.get("dmel_min_value").and_then(|v| v.as_f64()) { | ||
| params.dmel_min_value = v; | ||
| } | ||
| if let Some(v) = audio_config.get("dmel_max_value").and_then(|v| v.as_f64()) { | ||
| params.dmel_max_value = v; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
ast-grep outline crates/multimodal/src/audio/processors/inkling.rs \
--match 'InklingAudioEncoderParams|dmel_bins' --view expanded
rg -n -C3 '\bdmel_(min|max)_value\b' crates/multimodalRepository: lightseekorg/smg
Length of output: 6458
🌐 Web query:
Rust f64::clamp panics when min greater than max or NaN documentation
💡 Result:
In Rust, the f64::clamp function is explicitly documented to panic under specific conditions [1][2]. According to the official Rust documentation, f64::clamp will panic if: - min is greater than max [1][3] - min is NaN [1][3] - max is NaN [1][3] If the value being clamped (self) is NaN, the function returns NaN rather than panicking [1][2]. The requirement to panic when min > max, min is NaN, or max is NaN is part of the standard library's implementation to ensure that the input range is valid, as these conditions are considered logic errors [4][5].
Citations:
- 1: https://doc.rust-lang.org/core/primitive.f64.html
- 2: https://doc.rust-lang.org/1.78.0/core/primitive.f64.html
- 3: https://doc.rust-lang.org/1.67.1/std/primitive.f64.html
- 4: https://rust-lang.github.io/rfcs/1961-clamp.html
- 5: cr1901/rust@9943a44
Validate the dMel bounds before calling clamp.
f64::clamp panics when dmel_min_value > dmel_max_value, so an inverted model config can take down request processing instead of returning TransformError.
🤖 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/multimodal/src/audio/processors/inkling.rs` around lines 65 - 69,
Validate the configured dMel bounds after applying the `dmel_min_value` and
`dmel_max_value` values and before the downstream `clamp` call. In the
surrounding audio processing flow, detect when the minimum exceeds the maximum
and return `TransformError` instead of allowing `f64::clamp` to panic; preserve
normal processing for valid bounds.
| fn calculate_num_tokens(&self, width: u32, height: u32, config: &PreProcessorConfig) -> usize { | ||
| let processor = self.with_preprocessor_config(config); | ||
| let (scaled_width, scaled_height) = processor.scaled_image_dimensions(width, height); | ||
| processor | ||
| .patch_grid(scaled_width as usize, scaled_height as usize) | ||
| .2 | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Locate PatchSize / get_patch_size definitions to check if 0 is reachable.
ast-grep run --pattern 'fn get_patch_size($$$) { $$$ }' --lang rust crates/multimodal
rg -n -C5 'struct PatchSize' crates/multimodal/srcRepository: lightseekorg/smg
Length of output: 1940
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,220p' crates/multimodal/src/vision/preprocessor_config.rs
printf '\n--- inkling ---\n'
sed -n '1,360p' crates/multimodal/src/vision/processors/inkling.rsRepository: lightseekorg/smg
Length of output: 19715
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Focus on validation and token calculation call sites.
rg -n -C4 'validate\(\)|calculate_num_tokens|patch_grid|get_patch_size|patch_size' crates/multimodal/src/visionRepository: lightseekorg/smg
Length of output: 50373
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
p = Path('crates/multimodal/src/vision/preprocessor_config.rs')
text = p.read_text()
for needle in ['fn get_patch_size', 'fn validate', 'impl Default for PatchSize', 'PatchSizeVisitor', 'patch_size']:
idx = text.find(needle)
print(f'=== {needle} ===')
if idx >= 0:
start = text.rfind('\n', 0, idx)
print(text[max(0, idx-500): idx+1800])
else:
print('not found')
PYRepository: lightseekorg/smg
Length of output: 7072
🏁 Script executed:
#!/bin/bash
set -euo pipefail
nl -ba crates/multimodal/src/vision/preprocessor_config.rs | sed -n '1,220p'Repository: lightseekorg/smg
Length of output: 194
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
text = Path('crates/multimodal/src/vision/preprocessor_config.rs').read_text().splitlines()
for i, line in enumerate(text, 1):
if 'fn get_patch_size' in line or 'fn validate' in line or 'PatchSizeVisitor' in line or 'patch_size' in line:
start = max(1, i-12)
end = min(len(text), i+80)
print(f'--- lines {start}-{end} around line {i} ---')
for j in range(start, end+1):
print(f'{j:4}: {text[j-1]}')
print()
PYRepository: lightseekorg/smg
Length of output: 50372
Guard calculate_num_tokens against zero patch sizes
calculate_num_tokens() skips the validate() check used by preprocess(), so a patch_size of 0 can still reach patch_grid() and panic on division by zero. Reuse the same guard here or return a safe fallback.
🤖 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/multimodal/src/vision/processors/inkling.rs` around lines 270 - 276,
Update calculate_num_tokens to guard against a zero patch_size before calling
scaled_image_dimensions or patch_grid, reusing the validation guard used by
preprocess where possible. Return the established safe fallback for invalid
patch sizes, while preserving the existing token calculation for valid
configurations.
| /// Pillow-exact LANCZOS resize (RGB8), matching | ||
| /// `PIL.Image.resize(.., LANCZOS)`. | ||
| pub fn resize_lanczos_pil(image: &DynamicImage, out_w: u32, out_h: u32) -> DynamicImage { | ||
| let rgb = image.to_rgb8(); | ||
| let (in_w, in_h) = rgb.dimensions(); | ||
| let output = resize_pil_bytes( | ||
| rgb.as_raw(), | ||
| in_w, | ||
| in_h, | ||
| out_w, | ||
| out_h, | ||
| false, | ||
| PilResizeFilter::Lanczos, | ||
| ); | ||
| #[expect( | ||
| clippy::expect_used, | ||
| reason = "output is exactly out_w*out_h*3 bytes by construction" | ||
| )] | ||
| DynamicImage::ImageRgb8( | ||
| RgbImage::from_raw(out_w, out_h, output).expect("pil resize buffer size"), | ||
| ) | ||
| } | ||
|
|
||
| fn resize_pil_bytes( | ||
| data: &[u8], | ||
| in_w: u32, | ||
| in_h: u32, | ||
| out_w: u32, | ||
| out_h: u32, | ||
| joint_rgb: bool, | ||
| filter: PilResizeFilter, | ||
| ) -> Vec<u8> { | ||
| let (in_w, in_h, out_w, out_h) = (in_w as usize, in_h as usize, out_w as usize, out_h as usize); | ||
| if in_w == out_w && in_h == out_h { | ||
| data.to_vec() | ||
| } else if in_w == out_w { | ||
| if joint_rgb { | ||
| pil_resample_vertical_rgb(data, in_h, in_w, out_h) | ||
| pil_resample_vertical_rgb(data, in_h, in_w, out_h, filter) | ||
| } else { | ||
| pil_resample_vertical(data, in_h, in_w, out_h, 3) | ||
| pil_resample_vertical(data, in_h, in_w, out_h, 3, filter) | ||
| } | ||
| } else { | ||
| let horiz = if joint_rgb { | ||
| pil_resample_horizontal_rgb(data, in_h, in_w, out_w) | ||
| pil_resample_horizontal_rgb(data, in_h, in_w, out_w, filter) | ||
| } else { | ||
| pil_resample_horizontal(data, in_h, in_w, out_w, 3) | ||
| pil_resample_horizontal(data, in_h, in_w, out_w, 3, filter) | ||
| }; | ||
| if in_h == out_h { | ||
| horiz | ||
| } else if joint_rgb { | ||
| pil_resample_vertical_rgb(&horiz, in_h, out_w, out_h) | ||
| pil_resample_vertical_rgb(&horiz, in_h, out_w, out_h, filter) | ||
| } else { | ||
| pil_resample_vertical(&horiz, in_h, out_w, out_h, 3) | ||
| pil_resample_vertical(&horiz, in_h, out_w, out_h, 3, filter) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
Add Lanczos-analog regression tests (no exactness coverage for the new filter).
The bicubic path has two dedicated tests validating byte-for-byte behavior (resize_bicubic_pil_rgb_matches_dynamic_path, resize_bicubic_pil_rgb_skips_identity_axes_bit_exactly), but the newly introduced resize_lanczos_pil/PilResizeFilter::Lanczos path has no equivalent. Since this is the resize path used by InklingImageProcessor::prepare_rgb_image for actual image preprocessing, and the checked-in Inkling golden fixture only covers audio cases, the Lanczos kernel currently ships without any automated correctness check against a reference.
Consider mirroring the existing bicubic tests for PilResizeFilter::Lanczos (dynamic-vs-bytes parity, identity-axis fast path).
🤖 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/multimodal/src/vision/transforms.rs` around lines 820 - 875, Add
Lanczos regression tests mirroring resize_bicubic_pil_rgb_matches_dynamic_path
and resize_bicubic_pil_rgb_skips_identity_axes_bit_exactly. Cover byte-for-byte
parity between resize_lanczos_pil and resize_pil_bytes using
PilResizeFilter::Lanczos, plus exact preservation when either resize axis is
unchanged, including both identity-axis cases.
| /// Effort level for reasoning models. | ||
| /// | ||
| /// OpenAI-compatible callers normally send a named string, while some | ||
| /// model integrations accept a numeric value. Keep the public Rust shape | ||
| /// as a string for compatibility, but accept either JSON representation at | ||
| /// the HTTP boundary; model-specific normalization happens in the gateway. | ||
| #[serde(default, deserialize_with = "deserialize_reasoning_effort")] | ||
| pub reasoning_effort: Option<String>, |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether reasoning_effort has a Schemars override or schema test.
ast-grep outline crates/protocols/src/chat.rs --match ChatCompletionRequest --view expanded
rg -n -C3 'reasoning_effort|schemars\(.*(with|schema_with)|JsonSchema' \
crates/protocols/src/chat.rs crates/protocolsRepository: lightseekorg/smg
Length of output: 50375
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '198,215p' crates/protocols/src/chat.rs
printf '\n---\n'
sed -n '860,890p' crates/protocols/src/chat.rs
printf '\n---\n'
rg -n 'schemars::|schema_with|with =|reasoning_effort' crates/protocols/src/chat.rsRepository: lightseekorg/smg
Length of output: 4762
Expose reasoning_effort as string-or-number in the request schema. The custom deserializer already accepts numbers, but JsonSchema still derives this field as string, so schema-driven clients and validators will reject a supported request shape. Add an explicit schema override and a regression test.
🤖 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/protocols/src/chat.rs` around lines 202 - 209, Update the schema
metadata for the `reasoning_effort` field in the request type so its generated
`JsonSchema` advertises both string and numeric JSON representations, matching
`deserialize_reasoning_effort`. Add a regression test that verifies the
generated schema accepts and describes both forms while preserving the public
Rust type and existing deserialization behavior.
| if self.current_tool_name.is_none() { | ||
| if let Ok((Value::Object(object), _)) = | ||
| self.partial_json.parse_value(&self.buffer, false) | ||
| { | ||
| if let Some(name) = object.get("name").and_then(Value::as_str) { | ||
| if allowed_tools.contains(name) { | ||
| self.current_tool_name = Some(name.to_string()); | ||
| result.calls.push(ToolCallItem { | ||
| tool_index: self.current_tool_index, | ||
| name: Some(name.to_string()), | ||
| parameters: String::new(), | ||
| }); | ||
| } else { | ||
| tracing::debug!("Inkling attempted to call undefined tool: {}", name); | ||
| self.state = StreamingState::DiscardToolCall; | ||
| return true; | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| let Some(json_len) = complete_json_object_len(&self.buffer) else { | ||
| return false; | ||
| }; | ||
| let json = &self.buffer[..json_len]; | ||
| let Some(call) = parse_tool_call_json(json, Some(allowed_tools)) else { | ||
| self.current_tool_name = None; | ||
| self.state = StreamingState::DiscardToolCall; | ||
| return true; | ||
| }; | ||
|
|
||
| // A complete object may expose the name and arguments in the same | ||
| // chunk. Emit the name first in that case, as required by stream APIs. | ||
| if self.current_tool_name.is_none() { | ||
| result.calls.push(ToolCallItem { | ||
| tool_index: self.current_tool_index, | ||
| name: Some(call.function.name.clone()), | ||
| parameters: String::new(), | ||
| }); | ||
| } | ||
| result.calls.push(ToolCallItem { | ||
| tool_index: self.current_tool_index, | ||
| name: None, | ||
| parameters: call.function.arguments, | ||
| }); | ||
|
|
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Validate the complete call before emitting its name.
Line 291 publishes a tool name from partial JSON. If the completed object later has malformed/non-object args, Lines 309-312 discard it but cannot retract that event; the next valid call also reuses the same index. Delay both streaming events until parse_tool_call_json succeeds.
Proposed fix
if allowed_tools.contains(name) {
self.current_tool_name = Some(name.to_string());
- result.calls.push(ToolCallItem {
- tool_index: self.current_tool_index,
- name: Some(name.to_string()),
- parameters: String::new(),
- });
} else {
...
- if self.current_tool_name.is_none() {
- result.calls.push(ToolCallItem {
- tool_index: self.current_tool_index,
- name: Some(call.function.name.clone()),
- parameters: String::new(),
- });
- }
+ result.calls.push(ToolCallItem {
+ tool_index: self.current_tool_index,
+ name: Some(call.function.name.clone()),
+ parameters: String::new(),
+ });🤖 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/tool_parser/src/parsers/inkling.rs` around lines 284 - 329, The
partial-JSON branch in the Inkling parser emits a tool-name event before the
complete call is validated, leaving a stale event when parse_tool_call_json
rejects malformed arguments. Move name emission out of that branch and emit both
the name and parameters only after parse_tool_call_json succeeds, while
preserving the current_tool_name tracking needed for streaming chunks.
| let Some(json_len) = complete_json_object_len(&self.buffer) else { | ||
| return false; | ||
| }; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Recover from unterminated JSON frames.
Once a frame starts with {, complete_json_object_len returns None forever if the closing brace is missing—even after <|end_message|> arrives. The parser then stalls and accumulates every subsequent chunk. Detect frame terminators outside JSON strings before the object closes, transition to discard/recovery, and add a split-chunk regression test.
🤖 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/tool_parser/src/parsers/inkling.rs` around lines 305 - 307, Update the
frame parsing logic around complete_json_object_len to detect <|end_message|>
terminators outside JSON strings when the JSON object remains incomplete, then
transition into the existing discard/recovery path instead of continuing to
accumulate chunks. Preserve normal parsing when the terminator is absent or
appears inside a JSON string, and add a regression test covering a terminator
split across chunks.
| // Check parser availability once upfront. Run parser when the user explicitly | ||
| // enabled thinking, or when the selected parser needs structural special tokens. | ||
| let reasoning_requires_special_tokens = utils::reasoning_parser_requires_special_tokens( | ||
| &self.reasoning_parser_factory, | ||
| self.configured_reasoning_parser.as_deref(), | ||
| model, | ||
| ); | ||
| let separate_reasoning = reasoning_requires_special_tokens | ||
| || matches!( | ||
| &original_request.thinking, | ||
| Some( | ||
| messages::ThinkingConfig::Enabled { .. } | ||
| | messages::ThinkingConfig::Adaptive { .. } | ||
| ) | ||
| ); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Do not emit thinking blocks when thinking is explicitly disabled.
This flag now conflates “parse structural tokens” with “expose reasoning.” For Inkling, ThinkingConfig::Disabled still activates the parser and downstream lines 1844-1870 emit parsed reasoning as thinking blocks. Split this into parse_reasoning and emit_reasoning: always parse when special tokens require it, but suppress reasoning deltas unless thinking is enabled/adaptive.
Proposed direction
+ let emit_reasoning = matches!(
+ &original_request.thinking,
+ Some(
+ messages::ThinkingConfig::Enabled { .. }
+ | messages::ThinkingConfig::Adaptive { .. }
+ )
+ );
- let separate_reasoning = reasoning_requires_special_tokens
- || matches!(...)
+ let parse_reasoning = reasoning_requires_special_tokens || emit_reasoning;
- let reasoning_parser_available = separate_reasoning
+ let reasoning_parser_available = parse_reasoningGuard thinking-block emission with emit_reasoning while retaining the parser’s filtered normal_text.
🤖 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 `@model_gateway/src/routers/grpc/regular/streaming.rs` around lines 1694 -
1708, Split the combined `separate_reasoning` decision into `parse_reasoning`
and `emit_reasoning`: keep parser activation whenever
`reasoning_requires_special_tokens` is true, but set `emit_reasoning` only for
`ThinkingConfig::Enabled` or `Adaptive`. Update the downstream reasoning
emission logic around lines 1844-1870 to guard thinking-block deltas with
`emit_reasoning`, while preserving the parser’s filtered `normal_text` output
when thinking is explicitly disabled.
…capability Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
…ed ids parse tool calls Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
| fn default() -> Self { | ||
| Self { | ||
| media_order: MediaPartOrder::MediaFirst, | ||
| absent_assistant_content: AbsentAssistantContent::Null, |
There was a problem hiding this comment.
🔴 Important: This changes the default absent_assistant_content from what was effectively EmptyString (for all non-TML models) to Null.
Previously, chat_utils.rs coupled absent-content behavior to MediaPartOrder: MediaFirst → empty string, Authored → null. The old comment in message_utils.rs explicitly guarded this:
"Always insert content — empty string when tool-calls-only. Certain models' chat template requires content to be a string, not null, even when only tool_calls are present."
Now, every model that doesn't override chat_render() gets Null. Since only InklingSpec overrides it (and it already used Null), this is a behavioral change for all other models. Chat templates that do {% if message.content %} or string concatenation on content without a null check may break when processing tool-call-only assistant messages.
Consider defaulting to EmptyString here to preserve the prior behavior, and let Inkling (and future models that want protocol-faithful null) opt into Null via their spec override:
| absent_assistant_content: AbsentAssistantContent::Null, | |
| absent_assistant_content: AbsentAssistantContent::EmptyString, |
| Ok(value) | ||
| } else { | ||
| Err(format!( | ||
| "Inkling reasoning_effort must be a finite number in [0, {max}]" |
There was a problem hiding this comment.
🟡 Nit: The error messages in validate_numeric_reasoning_effort and parse_numeric_reasoning_effort_string still hardcode "Inkling", but these functions are now generic (parameterized by max). If another model family adopts ReasoningEffortStyle::Numeric, users will see misleading "Inkling reasoning_effort must be…" errors. Consider dropping the model name or making it a parameter.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/multimodal/src/audio/processors/inkling.rs`:
- Around line 68-69: Validate the parsed n_fft value in the audio configuration
before assigning it to params.n_fft. Accept only values within the processor’s
supported FFT range, reject out-of-range values without casting or storing them,
and preserve the existing optional-configuration behavior.
In `@crates/tool_parser/src/factory.rs`:
- Around line 242-253: Update the pattern matching in the mapping lookup to
avoid arbitrary substring matches: after case normalization, accept a stem only
when the model ID equals it or starts with it at a full ID/path-segment
boundary, while preserving support for namespaced IDs such as org/Inkling-Chat
and longest-pattern selection. Modify the filter logic in the iterator used by
the parser-resolution function, leaving parser selection behavior unchanged for
valid matches.
In `@model_gateway/src/routers/grpc/multimodal/plan.rs`:
- Around line 156-163: Update
model_gateway/src/routers/grpc/multimodal/plan.rs:156-163 so the model
configuration resolution in the multimodal planning flow propagates the loading
error instead of returning ChatRenderContract::default(). In
model_gateway/src/routers/grpc/regular/stages/chat/preparation.rs:62-76 and
model_gateway/src/routers/grpc/regular/stages/messages/preparation.rs:94-108,
resolve the rendering contract independently of optional multimodal processing
and propagate required metadata/configuration failures rather than falling back
to generic rendering.
In `@model_gateway/src/routers/grpc/utils/chat_utils.rs`:
- Around line 313-320: Update the continuation logic in `continue_final_message`
so final assistant messages with `content: null` are not removed: only pop and
restore messages when their content is a string, or explicitly reject
continuation for null content. Preserve tool-call-only and reasoning-only
assistant messages instead of silently dropping them.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: d109aa4c-1eb4-42ab-9914-97547762bc57
📒 Files selected for processing (17)
crates/multimodal/src/audio/processors/inkling.rscrates/multimodal/src/lib.rscrates/multimodal/src/registry/inkling.rscrates/multimodal/src/registry/mod.rscrates/multimodal/src/registry/traits.rscrates/multimodal/src/vision/processors/inkling.rscrates/multimodal/tests/fixtures/golden/inkling_preprocess_fingerprints.jsoncrates/multimodal/tests/inkling_preprocess_golden.rscrates/tool_parser/src/factory.rscrates/tool_parser/tests/tool_parser_inkling.rsmodel_gateway/src/routers/grpc/multimodal/mod.rsmodel_gateway/src/routers/grpc/multimodal/plan.rsmodel_gateway/src/routers/grpc/regular/responses/conversions.rsmodel_gateway/src/routers/grpc/regular/stages/chat/preparation.rsmodel_gateway/src/routers/grpc/regular/stages/messages/preparation.rsmodel_gateway/src/routers/grpc/utils/chat_utils.rsmodel_gateway/src/routers/grpc/utils/message_utils.rs
| // Case-insensitive substring matching (longest pattern wins) so namespaced | ||
| // and differently-cased ids (e.g. "org/Inkling-Chat") still resolve. | ||
| let model_lower = model.to_lowercase(); | ||
| mapping | ||
| .iter() | ||
| .filter(|(pattern, _)| { | ||
| pattern.ends_with('*') && model.starts_with(&pattern[..pattern.len() - 1]) | ||
| .filter_map(|(pattern, parser_name)| { | ||
| let stem = pattern.strip_suffix('*')?; | ||
| model_lower | ||
| .contains(&stem.to_lowercase()) | ||
| .then_some((stem, parser_name)) | ||
| }) | ||
| .max_by_key(|(pattern, _)| pattern.len()) | ||
| .max_by_key(|(stem, _)| stem.len()) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Preserve model-name boundaries when matching parser patterns.
Substring matching makes unrelated IDs such as notqwen match qwen*, potentially selecting an incompatible parser. Match case-insensitive prefixes at the full ID or path-segment boundary instead; this still supports org/Inkling-Chat.
Proposed fix
let model_lower = model.to_lowercase();
mapping
.iter()
.filter_map(|(pattern, parser_name)| {
let stem = pattern.strip_suffix('*')?;
- model_lower
- .contains(&stem.to_lowercase())
- .then_some((stem, parser_name))
+ let stem_lower = stem.to_lowercase();
+ let matches = model_lower.starts_with(&stem_lower)
+ || model_lower
+ .split('/')
+ .any(|segment| segment.starts_with(&stem_lower));
+ matches.then_some((stem, parser_name))
})📝 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.
| // Case-insensitive substring matching (longest pattern wins) so namespaced | |
| // and differently-cased ids (e.g. "org/Inkling-Chat") still resolve. | |
| let model_lower = model.to_lowercase(); | |
| mapping | |
| .iter() | |
| .filter(|(pattern, _)| { | |
| pattern.ends_with('*') && model.starts_with(&pattern[..pattern.len() - 1]) | |
| .filter_map(|(pattern, parser_name)| { | |
| let stem = pattern.strip_suffix('*')?; | |
| model_lower | |
| .contains(&stem.to_lowercase()) | |
| .then_some((stem, parser_name)) | |
| }) | |
| .max_by_key(|(pattern, _)| pattern.len()) | |
| .max_by_key(|(stem, _)| stem.len()) | |
| // Case-insensitive substring matching (longest pattern wins) so namespaced | |
| // and differently-cased ids (e.g. "org/Inkling-Chat") still resolve. | |
| let model_lower = model.to_lowercase(); | |
| mapping | |
| .iter() | |
| .filter_map(|(pattern, parser_name)| { | |
| let stem = pattern.strip_suffix('*')?; | |
| let stem_lower = stem.to_lowercase(); | |
| let matches = model_lower.starts_with(&stem_lower) | |
| || model_lower | |
| .split('/') | |
| .any(|segment| segment.starts_with(&stem_lower)); | |
| matches.then_some((stem, parser_name)) | |
| }) | |
| .max_by_key(|(stem, _)| stem.len()) |
🤖 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/tool_parser/src/factory.rs` around lines 242 - 253, Update the pattern
matching in the mapping lookup to avoid arbitrary substring matches: after case
normalization, accept a stem only when the model ID equals it or starts with it
at a full ID/path-segment boundary, while preserving support for namespaced IDs
such as org/Inkling-Chat and longest-pattern selection. Modify the filter logic
in the iterator used by the parser-resolution function, leaving parser selection
behavior unchanged for valid matches.
| let model_config = match components | ||
| .config_registry | ||
| .get_or_load(tokenizer_id, tokenizer_source) | ||
| .await | ||
| { | ||
| Ok(config) => config, | ||
| Err(_) => return ChatRenderContract::default(), | ||
| }; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Do not silently downgrade model-specific rendering to the default contract.
Resolution failures currently make Inkling use generic rendering, silently losing numeric reasoning-effort handling and other model-specific behavior.
model_gateway/src/routers/grpc/multimodal/plan.rs#L156-L163: return configuration-loading errors instead of converting them toChatRenderContract::default().model_gateway/src/routers/grpc/regular/stages/chat/preparation.rs#L62-L76: resolve the contract independently of optional multimodal processing and propagate required-metadata failures.model_gateway/src/routers/grpc/regular/stages/messages/preparation.rs#L94-L108: apply the same independent resolution/error behavior to the Messages path.
📍 Affects 3 files
model_gateway/src/routers/grpc/multimodal/plan.rs#L156-L163(this comment)model_gateway/src/routers/grpc/regular/stages/chat/preparation.rs#L62-L76model_gateway/src/routers/grpc/regular/stages/messages/preparation.rs#L94-L108
🤖 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 `@model_gateway/src/routers/grpc/multimodal/plan.rs` around lines 156 - 163,
Update model_gateway/src/routers/grpc/multimodal/plan.rs:156-163 so the model
configuration resolution in the multimodal planning flow propagates the loading
error instead of returning ChatRenderContract::default(). In
model_gateway/src/routers/grpc/regular/stages/chat/preparation.rs:62-76 and
model_gateway/src/routers/grpc/regular/stages/messages/preparation.rs:94-108,
resolve the rendering contract independently of optional multimodal processing
and propagate required metadata/configuration failures rather than falling back
to generic rendering.
Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
clippy --all-features -D warnings rejects the no-op i32->i32 cast in seeded_signal; the value is already i32. Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
|
Incremental review (push Reviewed the new EPD (Encode-Prefill-Decode) disaggregation test infrastructure, CI workflow additions, and CUDA 13 build-script changes. No new issues found. Summary of changes reviewed:
Previous review comments (from the initial push) remain applicable — no changes in this push addressed or conflicted with them. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b039c1eea7
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| fn default() -> Self { | ||
| Self { | ||
| media_order: MediaPartOrder::MediaFirst, | ||
| absent_assistant_content: AbsentAssistantContent::Null, |
There was a problem hiding this comment.
With this default, every model that does not override ChatRenderContract now serializes assistant messages that only contain tool_calls with content: null; before this patch both Chat and Messages paths forced "". There are no production specs opting into AbsentAssistantContent::EmptyString, so tool-call history for non-Inkling models that use string-concatenating chat templates (for example message['content'] + ...) can fail during apply_chat_template or render the wrong value. Please keep the default as empty string and opt only the templates that require null into that behavior, or add the missing per-model overrides.
Useful? React with 👍 / 👎.
…atim The inkling chat template already owns reasoning_effort end to end: it maps named levels to a numeric directive, applies the 0.9 default, and validates the range. The router's parallel numeric mapping was redundant and had drifted (minimal -> 0.0 vs the template's 0.1); since the router pre-converted to a number, the wrong value won. Forward the effort string verbatim for all models and let the template own mapping, defaulting, and validation. Drops ReasoningEffortStyle from ChatRenderContract and removes the now-dead chat_completions_api_request origin marker (its sole purpose was scoping a default the chat template already provides). Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: eba6fcefed
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if token == MESSAGE_MODEL { | ||
| self.block_kind = Some(BlockKind::Header); | ||
| return; |
There was a problem hiding this comment.
When a structured tool call argument contains a literal TML control token (for example a search query like "<|message_model|>"), this branch runs before the BlockKind::Tool handling below, so the reasoning parser treats that string content as a new header instead of forwarding the JSON block verbatim to the tool parser. The resulting normal_text is truncated/malformed and the valid tool call is dropped; once inside a tool JSON frame, control-looking substrings should be ignored until the frame delimiter is found safely.
Useful? React with 👍 / 👎.
| if let Some(reasoning_effort) = &request.reasoning_effort { | ||
| combined.insert( | ||
| REASONING_EFFORT_KEY.to_string(), | ||
| Value::String(reasoning_effort.clone()), |
There was a problem hiding this comment.
When a client sends the newly accepted numeric reasoning_effort form, this always inserts it into the chat-template kwargs as a JSON string. Templates or model integrations that accept numeric effort values therefore receive "0.2" instead of 0.2, so numeric comparisons/arithmetic or type checks can render incorrectly despite the HTTP boundary accepting the numeric form; preserve the original scalar type or reparse numbers before forwarding.
Useful? React with 👍 / 👎.
…capability With reasoning_effort now owned by the chat template, the only per-model rendering behavior left was media-part ordering. absent_assistant_content was dead differentiation: every spec returned Null and EmptyString was test-only. Make absent assistant content universally null (OpenAI-faithful, and verified against the inkling chat template) and replace ChatRenderContract + AbsentAssistantContent with a single ModelProcessorSpec::media_part_order() capability (resolve_chat_render_contract -> resolve_media_part_order). Pure refactor: no behavior change (verified: no spec ever selected EmptyString). Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
2 participants
TokenSpeed PR
lightseekorg/tokenspeed#689
Blog
https://lightseek.org/blog/tokenspeed-inkling.html
Docker
Launch command
Inkling support needs to be available in the public SMG repository for the TokenSpeed integration. This carries the existing private-branch change without rewriting its commit.
Supersedes #1925 after renaming the head branch to
feat/inkling.Summary by CodeRabbit
reasoning_effortnow accepts strings, numbers, or null values.