Skip to content

Conversation

qlyoung
Copy link

@qlyoung qlyoung commented Sep 18, 2025

Description

Streaming responses contain data: data: which breaks OpenAI compatibility as
described in #26. Fix it by removing the extra data:.

While there is a (double-posted) comment in #26 claiming that the fix was committed, there's no reference to a corresponding PR or commit, and I don't see any change in the tree.

Type of Change

  • Bug fix (non-breaking change that fixes an issue)

Shimmy Philosophy Compliance

  • Enhances OpenAI API compatibility

Testing

  • I have tested these changes locally
  • I have run cargo test and all tests pass

The tests don't compile on main.

  • I have run cargo clippy with no warnings

There are 13 warnings on main.

  • I have run cargo fmt

cargo fmt produces a large number of whitespace changes on main.

Legal Compliance

  • All commits are signed off with DCO (git commit -s)

No commits on the main branch have a DCO on them, this doesn't appear to be current practice at all.

  • I have the right to contribute this code under the project license
  • If this includes third-party code, it's properly attributed and licensed

Binary Size Impact

No


Skipped all these.

Enterprise Considerations

  • This change maintains backward compatibility
  • Performance impact has been measured and documented
  • Documentation has been updated where applicable
  • Change aligns with roadmap priorities

Community Impact

  • This change benefits the broader Shimmy community
  • Breaking changes are clearly documented and justified
  • Migration path is provided for breaking changes
    New binary size: ___ MB
    Change: ± ___ MB

Checklist

  • My code follows the project's coding standards
  • I have read the CONTRIBUTING.md guidelines
  • I have added tests for new functionality (if applicable)
  • I have updated documentation (if applicable)
  • This PR addresses an existing issue: #___

Additional Notes

Any additional information or context for reviewers.

Streaming responses contain "data: data:" which breaks openAI compatibility as
described in Michael-A-Kuykendall#26. Fix it by removing the extra data: call.
@qlyoung qlyoung changed the title fix: remove extraneous data: from streaming responses fix: remove extraneous data: from streaming responses Sep 18, 2025
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR fixes OpenAI API compatibility issues by removing duplicate "data:" prefixes from Server-Sent Events (SSE) streaming responses, addressing issue #26.

Key Changes

  • Removes extraneous "data:" prefix and newlines from streaming token responses
  • Simplifies the "[DONE]" sentinel message format
  • Updates both initial chunk, token chunks, and final chunk formatting

"data: {}\n\n",
serde_json::to_string(&initial_chunk).unwrap()
));
let _ = tx_tokens.send(serde_json::to_string(&initial_chunk).unwrap());
Copy link

Copilot AI Sep 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The SSE format requires 'data: ' prefix and double newlines for proper client parsing. Removing these breaks SSE protocol compliance. The format should be 'data: {json}\n\n' for each event.

Copilot uses AI. Check for mistakes.

"data: {}\n\n",
serde_json::to_string(&chunk).unwrap()
));
let _ = tx_tokens.send(serde_json::to_string(&chunk).unwrap());
Copy link

Copilot AI Sep 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Token chunks must follow SSE format with 'data: ' prefix and proper line endings. Raw JSON strings without SSE formatting will not be parsed correctly by SSE clients.

Copilot uses AI. Check for mistakes.

Comment on lines +263 to +264
let _ = tx.send(serde_json::to_string(&final_chunk).unwrap());
let _ = tx.send("[DONE]".to_string());
Copy link

Copilot AI Sep 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both the final chunk and [DONE] sentinel need proper SSE formatting. They should be 'data: {json}\n\n' and 'data: [DONE]\n\n' respectively to maintain SSE protocol compliance.

Copilot uses AI. Check for mistakes.

Copy link
Owner

Hi @qlyoung,

Thank you for taking the time to identify and propose a fix for the OpenAI compatibility issue. I appreciate your attention to the technical details and the clear description of the problem.

However, I need to respectfully decline this PR due to several governance and process requirements that weren't followed. Shimmy operates under strict development standards to maintain quality and consistency:

Technical Process Requirements Not Met:

  • DCO Sign-off: All commits must be signed off with git commit -s (Developer Certificate of Origin)
  • Test Validation: cargo test --all-features must pass before submission
  • Code Quality: cargo clippy must run without warnings
  • Formatting: cargo fmt must be applied

Current Project Governance:

Shimmy is currently maintained under a lead maintainer model where I maintain sole merge authority to ensure consistent project direction during this critical growth phase.

Repository Access Update:

I've now implemented stricter branch protection rules to prevent unauthorized PRs going forward. This should have been in place from the start.

How to Contribute Going Forward:

  1. For Bug Reports: Please continue opening issues - this is extremely valuable
  2. For Code Contributions: Email [email protected] to discuss becoming an approved maintainer
  3. For Feature Requests: GitHub Discussions are perfect for this

About the Bug Fix:

The SSE formatting issue you've identified appears valid and needs addressing. I'll handle implementing the fix following our internal development process, including full test coverage and constitutional compliance.

Your technical insight is valuable, and I encourage you to continue reporting issues you discover.

Thanks for your help!
Mike

@qlyoung
Copy link
Author

qlyoung commented Sep 19, 2025

@Michael-A-Kuykendall As I pointed out in my PR description:

Test Validation: cargo test --all-features must pass before submission

Your tests don't even compile on your main branch:

qlyoung@local ~/D/p/shimmy (main)> git log --oneline | head -n 1
64d0878 fix: Apply automatic code formatting

qlyoung@local ~/D/p/shimmy (main)> cargo test
   Compiling shimmy v1.4.1 (/Users/qlyoung/Documents/projects/shimmy)
error[E0616]: field `search_paths` of struct `shimmy::discovery::ModelDiscovery` is private
   --> tests/regression_tests.rs:181:14
    |
181 |             .search_paths
    |              ^^^^^^^^^^^^ private field

error[E0277]: the trait bound `shimmy::openai_compat::ModelsResponse: serde::Deserialize<'de>` is not satisfied
    --> tests/regression_tests.rs:224:38
     |
224  |         let parsed: ModelsResponse = serde_json::from_str(&json).unwrap();
     |                                      ^^^^^^^^^^^^^^^^^^^^^^^^^^^ the trait `serde_core::de::Deserialize<'_>` is not implemented for `shimmy::openai_compat::ModelsResponse`
     |

I see you noticed this also:
https://github.com/Michael-A-Kuykendall/shimmy/actions/runs/17849409917/job/50754939897

DCO Sign-off: All commits must be signed off with git commit -s (Developer Certificate of Origin)

There is not a single commit in this repo that has a DCO:

qlyoung@local ~/D/p/shimmy (main)> git log --grep "Signed-"
qlyoung@local ~/D/p/shimmy (main)>

Formatting: cargo fmt must be applied

cargo fmt produces a large number of whitespace changes on main unrelated to my PR - you seem to have noticed this one though, now that I pointed it out: 64d0878

Code Quality: cargo clippy must run without warnings

cargo clippy produces tons of warnings on main:

qlyoung@Quentins-MBP ~/D/p/shimmy (main)> cargo clippy
   Compiling shimmy v1.4.1 (/Users/qlyoung/Documents/projects/shimmy)
warning: empty line after doc comment
   --> src/engine/safetensors_native.rs:237:5
    |
237 | /     /// Load model from cached metadata (much faster)
238 | |     /* async fn load_from_cached_metadata(spec: &ModelSpec, metadata: &ModelMetadata, use_mmap: bool) -> Result<Self> {
239 | |         info!("Loading model from cached metadata");
...   |
281 | |     } */
282 | |
    | |_^
...
285 |       fn parse_config_from_json(config_data: &serde_json::Value) -> Result<ModelConfig> {
    |       ------------------------- the comment documents this function
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#empty_line_after_doc_comments
    = note: `#[warn(clippy::empty_line_after_doc_comments)]` on by default
    = help: if the empty line is unintentional, remove it
help: if the doc comment should not document function `parse_config_from_json` then comment it out
    |
237 |     // /// Load model from cached metadata (much faster)
    |     ++

warning: you should consider adding a `Default` implementation for `ResponseCache`
   --> src/cache/response_cache.rs:135:5
    |
135 | /     pub fn new() -> Self {
136 | |         Self::with_config(ResponseCacheConfig::default())
137 | |     }
    | |_____^
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#new_without_default
    = note: `#[warn(clippy::new_without_default)]` on by default
help: try adding this
    |
134 + impl Default for ResponseCache {
135 +     fn default() -> Self {
136 +         Self::new()
137 +     }
138 + }
    |

warning: casting to the same type is unnecessary (`u64` -> `u64`)
   --> src/cache/response_cache.rs:168:21
    |
168 | /                     ((stats.average_response_time_saved.as_millis() as u64 * (stats.hits - 1)
169 | |                         + entry.response_time.as_millis() as u64)
170 | |                         / stats.hits) as u64,
    | |____________________________________________^
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_cast
    = note: `#[warn(clippy::unnecessary_cast)]` on by default
help: try
    |
168 ~                     (((stats.average_response_time_saved.as_millis() as u64 * (stats.hits - 1)
169 +                         + entry.response_time.as_millis() as u64)
170 ~                         / stats.hits)),
    |

warning: you should consider adding a `Default` implementation for `TelemetryCollector`
   --> src/metrics.rs:134:5
    |
134 | /     pub fn new() -> Self {
135 | |         let config_path = Self::get_config_path();
136 | |         Self {
137 | |             config: None,
...   |
145 | |     }
    | |_____^
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#new_without_default
help: try adding this
    |
133 + impl Default for TelemetryCollector {
134 +     fn default() -> Self {
135 +         Self::new()
136 +     }
137 + }
    |

warning: this `map_or` can be simplified
   --> src/metrics.rs:153:9
    |
153 |         self.config.as_ref().map_or(false, |c| c.enabled)
    |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_map_or
    = note: `#[warn(clippy::unnecessary_map_or)]` on by default
help: use is_some_and instead
    |
153 -         self.config.as_ref().map_or(false, |c| c.enabled)
153 +         self.config.as_ref().is_some_and(|c| c.enabled)
    |

warning: field assignment outside of initializer for an instance created with Default::default()
   --> src/metrics.rs:181:9
    |
181 |         config.enabled = enabled;
    |         ^^^^^^^^^^^^^^^^^^^^^^^^^
    |
note: consider initializing the variable with `metrics::TelemetryConfig { enabled: enabled, ..Default::default() }` and removing relevant reassignments
   --> src/metrics.rs:180:9
    |
180 |         let mut config = TelemetryConfig::default();
    |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#field_reassign_with_default
    = note: `#[warn(clippy::field_reassign_with_default)]` on by default

warning: the borrowed expression implements the required traits
   --> src/metrics.rs:420:39
    |
420 | ...                   .args(&["path", "win32_VideoController", "get", "name"])
    |                             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: change this to: `["path", "win32_VideoController", "get", "name"]`
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#needless_borrows_for_generic_args
    = note: `#[warn(clippy::needless_borrows_for_generic_args)]` on by default

warning: the borrowed expression implements the required traits
   --> src/metrics.rs:442:23
    |
442 |                 .args(&["path", "win32_VideoController", "get", "name"])
    |                       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: change this to: `["path", "win32_VideoController", "get", "name"]`
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#needless_borrows_for_generic_args

warning: this `map_or` can be simplified
   --> src/metrics.rs:466:16
    |
466 |             || std::env::var("TERM_PROGRAM").map_or(false, |v| v.contains("vscode"))
    |                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_map_or
help: use is_ok_and instead
    |
466 -             || std::env::var("TERM_PROGRAM").map_or(false, |v| v.contains("vscode"))
466 +             || std::env::var("TERM_PROGRAM").is_ok_and(|v| v.contains("vscode"))
    |

warning: this `if` has identical blocks
  --> src/model_registry.rs:72:46
   |
72 |           } else if name_lower.contains("phi") {
   |  ______________________________________________^
73 | |             "chatml".to_string()
74 | |         } else if name_lower.contains("mistral") {
   | |_________^
   |
note: same as this
  --> src/model_registry.rs:74:50
   |
74 |           } else if name_lower.contains("mistral") {
   |  __________________________________________________^
75 | |             "chatml".to_string()
76 | |         } else if name_lower.contains("qwen") {
   | |_________^
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#if_same_then_else
   = note: `#[warn(clippy::if_same_then_else)]` on by default

warning: this `if` has identical blocks
  --> src/model_registry.rs:74:50
   |
74 |           } else if name_lower.contains("mistral") {
   |  __________________________________________________^
75 | |             "chatml".to_string()
76 | |         } else if name_lower.contains("qwen") {
   | |_________^
   |
note: same as this
  --> src/model_registry.rs:76:47
   |
76 |           } else if name_lower.contains("qwen") {
   |  _______________________________________________^
77 | |             "chatml".to_string()
78 | |         } else if name_lower.contains("gemma") {
   | |_________^
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#if_same_then_else

warning: this `if` has identical blocks
  --> src/model_registry.rs:76:47
   |
76 |           } else if name_lower.contains("qwen") {
   |  _______________________________________________^
77 | |             "chatml".to_string()
78 | |         } else if name_lower.contains("gemma") {
   | |_________^
   |
note: same as this
  --> src/model_registry.rs:78:48
   |
78 |           } else if name_lower.contains("gemma") {
   |  ________________________________________________^
79 | |             "chatml".to_string()
80 | |         } else {
   | |_________^
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#if_same_then_else

warning: this `if` has identical blocks
  --> src/model_registry.rs:78:48
   |
78 |           } else if name_lower.contains("gemma") {
   |  ________________________________________________^
79 | |             "chatml".to_string()
80 | |         } else {
   | |_________^
   |
note: same as this
  --> src/model_registry.rs:80:16
   |
80 |           } else {
   |  ________________^
81 | |             "chatml".to_string() // Default to chatml for most models
82 | |         }
   | |_________^
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#if_same_then_else

warning: called `Iterator::last` on a `DoubleEndedIterator`; this will needlessly iterate the entire iterator
   --> src/openai_compat.rs:147:29
    |
147 |       let last_user_message = req
    |  _____________________________^
148 | |         .messages
149 | |         .iter()
150 | |         .filter(|m| m.role == "user")
151 | |         .last()
    | |__________-----^
    |            |
    |            help: try: `next_back()`
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#double_ended_iterator_last
    = note: `#[warn(clippy::double_ended_iterator_last)]` on by default

warning: redundant pattern matching, consider using `is_ok()`
  --> src/port_manager.rs:71:9
   |
71 | /         match TcpListener::bind(SocketAddr::from(([127, 0, 0, 1], port))) {
72 | |             Ok(_) => true,
73 | |             Err(_) => false,
74 | |         }
   | |_________^ help: try: `TcpListener::bind(SocketAddr::from(([127, 0, 0, 1], port))).is_ok()`
   |
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#redundant_pattern_matching
   = note: `#[warn(clippy::redundant_pattern_matching)]` on by default

warning: redundant closure
  --> src/invariant_ppt.rs:86:65
   |
86 |     match std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| test_fn())) {
   |                                                                 ^^^^^^^^^^^^ help: replace the closure with the function itself: `test_fn`
   |
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#redundant_closure
   = note: `#[warn(clippy::redundant_closure)]` on by default

warning: length comparison to zero
   --> src/invariant_ppt.rs:164:13
    |
164 |             response.len() > 0,
    |             ^^^^^^^^^^^^^^^^^^ help: using `!is_empty` is clearer and more explicit: `!response.is_empty()`
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#len_zero
    = note: `#[warn(clippy::len_zero)]` on by default

warning: manual `Range::contains` implementation
   --> src/invariant_ppt.rs:173:13
    |
173 |             status_code >= 200 && status_code < 600,
    |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use: `(200..600).contains(&status_code)`
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#manual_range_contains
    = note: `#[warn(clippy::manual_range_contains)]` on by default

warning: parameter is only used in recursion
   --> src/workflow.rs:255:10
    |
255 |         &self,
    |          ^^^^
    |
note: parameter used here
   --> src/workflow.rs:281:13
    |
281 |             self.visit_step(dep, steps, visited, temp_visited, order)?;
    |             ^^^^
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#only_used_in_recursion
    = note: `#[warn(clippy::only_used_in_recursion)]` on by default

warning: `shimmy` (lib) generated 19 warnings (run `cargo clippy --fix --lib -p shimmy` to apply 12 suggestions)
warning: the borrowed expression implements the required traits
   --> src/bin/test_real_safetensors.rs:104:15
    |
104 |         .args(&["run", "--bin", "shimmy", "--", "discover"])
    |               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: change this to: `["run", "--bin", "shimmy", "--", "discover"]`
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#needless_borrows_for_generic_args
    = note: `#[warn(clippy::needless_borrows_for_generic_args)]` on by default

warning: the borrowed expression implements the required traits
   --> src/bin/test_real_safetensors.rs:169:19
    |
169 |               .args(&[
    |  ___________________^
170 | |                 "run",
171 | |                 "--bin",
172 | |                 "shimmy",
...   |
175 | |                 &format!("test-{}", size_mb),
176 | |             ])
    | |_____________^
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#needless_borrows_for_generic_args
help: change this to
    |
169 ~             .args([
170 +                 "run",
171 +                 "--bin",
172 +                 "shimmy",
173 +                 "--",
174 +                 "probe",
175 +                 &format!("test-{}", size_mb),
176 ~             ])
    |

warning: the borrowed expression implements the required traits
   --> src/bin/test_real_safetensors.rs:211:15
    |
211 |         .args(&["run", "--bin", "shimmy", "--", "serve"])
    |               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: change this to: `["run", "--bin", "shimmy", "--", "serve"]`
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#needless_borrows_for_generic_args

warning: the borrowed expression implements the required traits
   --> src/bin/test_real_safetensors.rs:220:15
    |
220 |         .args(&["--max-time", "5", "http://127.0.0.1:11435/health"])
    |               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: change this to: `["--max-time", "5", "http://127.0.0.1:11435/health"]`
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#needless_borrows_for_generic_args

warning: `shimmy` (bin "test_real_safetensors") generated 4 warnings (run `cargo clippy --fix --bin "test_real_safetensors"` to apply 4 suggestions)
warning: this import is redundant
 --> src/bin/test_cache_performance.rs:6:1
  |
6 | use tokio;
  | ^^^^^^^^^^ help: remove it entirely
  |
  = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#single_component_path_imports
  = note: `#[warn(clippy::single_component_path_imports)]` on by default

warning: `shimmy` (bin "test_cache_performance") generated 1 warning (run `cargo clippy --fix --bin "test_cache_performance"` to apply 1 suggestion)
warning: `shimmy` (bin "shimmy") generated 7 warnings (7 duplicates)
    Finished `dev` profile [optimized + debuginfo] target(s) in 2.12s

It makes no sense to ask these things of your contributors when your own code doesn't even follow these standards.

In any case, I don't really care if this PR is merged or not, I just want the bug fixed. And again, I didn't find or describe this bug. It's the same bug in #26 that you declared you had committed a fix for, without actually doing it.

Michael-A-Kuykendall added a commit that referenced this pull request Sep 19, 2025
- Fix SSE streaming data: data: bug (remove duplicate prefixes)
- Fix broken test compilation (add missing serde derives, public methods)
- Apply cargo fmt to resolve all formatting issues
- Fix all clippy warnings:
  - Remove redundant template logic in model registry
  - Fix field assignment patterns in telemetry config
  - Convert assert!(true) to meaningful comments
  - Fix doc comment spacing issues
  - Remove unused self parameter in recursive function
- Configure DCO sign-off for all future commits
- Maintain constitutional compliance (lightweight binary, test coverage)

Addresses issues raised in PR #49 and #26. All tests now compile and
code quality standards are enforced consistently.

Signed-off-by: Mike Kuykendall <[email protected]>
Copy link
Owner

Fixed in commit 09744c2. You were right about all the governance issues - tests, clippy warnings, DCO, formatting. All cleaned up now.

The SSE bug is also properly fixed.

Thanks!
Mike

Michael-A-Kuykendall added a commit that referenced this pull request Oct 13, 2025
- Fix SSE streaming data: data: bug (remove duplicate prefixes)
- Fix broken test compilation (add missing serde derives, public methods)
- Apply cargo fmt to resolve all formatting issues
- Fix all clippy warnings:
  - Remove redundant template logic in model registry
  - Fix field assignment patterns in telemetry config
  - Convert assert!(true) to meaningful comments
  - Fix doc comment spacing issues
  - Remove unused self parameter in recursive function
- Configure DCO sign-off for all future commits
- Maintain constitutional compliance (lightweight binary, test coverage)

Addresses issues raised in PR #49 and #26. All tests now compile and
code quality standards are enforced consistently.

Signed-off-by: Mike Kuykendall <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants