Fix Qwen3.5 reader-stage failure: apply enable_thinking suppression manually - #58
Merged
Merged
Conversation
…anually CF-7 reader calls against Qwen3.5-9B were reporting 0/128 segment acceptance on every run despite the SeqMax runtime fix (PR #56). Root cause: Qwen3.5's own GGUF chat template implements the standard enable_thinking opt-out pattern -- its trailing add_generation_prompt block appends an empty, pre-closed <think></think> seed whenever enable_thinking isn't explicitly true, causing the model to skip straight to its real answer. LLamaSharp's LLamaTemplate (a minimal Jinja subset) renders through "<|im_start|>assistant\n" without error but never evaluates that trailing conditional, so the seed never gets applied and the model free-generates its trained default: full reasoning mode, consuming the reader's entire ~2000-token completion budget before any JSON is emitted. The previously-reported "16/120 and 1/120, both NO-GO" scores were not a capability measurement -- with zero segments ever ingested, B3 could only "pass" the 16 Unanswerable questions in the held-out set (correctly abstaining), meaning the score just reflected how many questions are Unanswerable, not model capability. ModelAdmissionGate had already flagged this exact risk when admitting Qwen3.5 as Provisional: "Visible reasoning traces can consume the response budget or precede the required JSON object." Fix: detect enable_thinking support generically from the raw tokenizer.chat_template metadata (not a filename/family check, so it covers future reasoning models using the same pattern) and append the same empty think-block the official template would render. Both new functions are pure/static with direct unit test coverage. Verified live (Qwen3.5-9B-Q8_0, 3-question smoke test): segment acceptance 0/128 -> 123/128, <think>-prefixed outputs 128/128 -> 0/128, mean completion tokens 2054 -> 294 (~7x faster per call, since none of the completion budget is wasted on reasoning).
Contributor
|
Warning Review limit reached
Next review available in: 24 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughChangesThinking suppression
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant GGUF
participant LLamaSharpRuntime
participant ApplyEmbeddedTemplate
participant RenderedPrompt
GGUF->>LLamaSharpRuntime: tokenizer.chat_template
LLamaSharpRuntime->>ApplyEmbeddedTemplate: cached suppression decision
ApplyEmbeddedTemplate->>RenderedPrompt: render prompt
ApplyEmbeddedTemplate->>RenderedPrompt: append empty think block when prompt ends with newline
🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
OrchestratorIDE/Core/Runtime/LLamaSharpRuntime.cs (1)
502-504: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake suppression idempotent.
ApplyThinkingSuppressionunconditionally appends the block if the string ends with a newline, meaning it will double-append if called twice or if the prompt already has the suppression block. While the runtime currently only calls it once per render, making it idempotent is safer and prevents accidental double-append bugs in the future.♻️ Proposed refactor
internal static string ApplyThinkingSuppression(string renderedPrompt) => - renderedPrompt.EndsWith('\n') ? renderedPrompt + "<think>\n\n</think>\n\n" : renderedPrompt; + renderedPrompt.EndsWith('\n') && !renderedPrompt.EndsWith("<think>\n\n</think>\n\n", StringComparison.Ordinal) + ? renderedPrompt + "<think>\n\n</think>\n\n" + : renderedPrompt;🤖 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 `@OrchestratorIDE/Core/Runtime/LLamaSharpRuntime.cs` around lines 502 - 504, Update ApplyThinkingSuppression so it first detects whether renderedPrompt already ends with the suppression block and returns it unchanged in that case; otherwise preserve the existing newline-gated append behavior.OrchestratorIDE.UnitTests/LLamaSharpRuntimeThinkingSuppressionTests.cs (1)
79-91: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAlign test with idempotency refactor.
The current test name (
Idempotent_Guard_Does_Not_Apply_Twice_Manually) contradicts its assertions (which verify that the method double-appends and is not idempotent).If you apply the idempotency refactor suggested in
LLamaSharpRuntime.cs, update this test to actually assert idempotency.♻️ Proposed refactor
[Test] - public void ApplyThinkingSuppression_Idempotent_Guard_Does_Not_Apply_Twice_Manually() + public void ApplyThinkingSuppression_IsIdempotent_DoesNotApplyTwice() { - // Not a code guarantee (the runtime only calls this once per render via the - // cached _templateSupportsThinkingSuppression flag) -- documents that calling it - // twice WOULD double-append, so any future caller must not do that. var once = LLamaSharpRuntime.ApplyThinkingSuppression("<|im_start|>assistant\n"); var twice = LLamaSharpRuntime.ApplyThinkingSuppression(once); - Assert.That(twice, Is.Not.EqualTo(once)); - Assert.That(twice, Does.Contain("<think>\n\n</think>\n\n<think>\n\n</think>\n\n")); + Assert.That(twice, Is.EqualTo(once)); }🤖 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 `@OrchestratorIDE.UnitTests/LLamaSharpRuntimeThinkingSuppressionTests.cs` around lines 79 - 91, The test ApplyThinkingSuppression_Idempotent_Guard_Does_Not_Apply_Twice_Manually must match the idempotent behavior of LLamaSharpRuntime.ApplyThinkingSuppression: update the implementation as needed so applying suppression twice produces the same result as once, then revise the test name, comments, and assertions to verify equality and prevent duplicate suppression markers.
🤖 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.
Nitpick comments:
In `@OrchestratorIDE.UnitTests/LLamaSharpRuntimeThinkingSuppressionTests.cs`:
- Around line 79-91: The test
ApplyThinkingSuppression_Idempotent_Guard_Does_Not_Apply_Twice_Manually must
match the idempotent behavior of LLamaSharpRuntime.ApplyThinkingSuppression:
update the implementation as needed so applying suppression twice produces the
same result as once, then revise the test name, comments, and assertions to
verify equality and prevent duplicate suppression markers.
In `@OrchestratorIDE/Core/Runtime/LLamaSharpRuntime.cs`:
- Around line 502-504: Update ApplyThinkingSuppression so it first detects
whether renderedPrompt already ends with the suppression block and returns it
unchanged in that case; otherwise preserve the existing newline-gated append
behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 63a669bc-0ef5-4ec2-9c11-259c9d083e00
📒 Files selected for processing (4)
OrchestratorIDE.UnitTests/LLamaSharpRuntimeThinkingSuppressionTests.csOrchestratorIDE/Core/Runtime/LLamaSharpRuntime.csdocs/CONTEXT_FABRIC_BUG_HISTORY.mddocs/CONTEXT_FABRIC_INFRASTRUCTURE_NOTES.md
1. ApplyThinkingSuppression is now idempotent: no-ops if the prompt already contains a <think> marker, guarding against double-application and against a future LLamaSharp version that DOES evaluate the template's own seed. 2. Wrapped the thinking-suppression detection/application in its own try/catch inside ApplyEmbeddedTemplate. A failure there was previously uncaught, propagating to BuildPromptForLoadedModel's outer catch, which treats ANY exception from this method as "the embedded template doesn't work" and permanently falls back to ChatML for the rest of the session -- a much worse regression than just skipping suppression once. 3. Bug History §7c claimed PR #58 included "the first full-scale re-run" -- it doesn't yet; corrected to match Infrastructure Notes, which already said the re-score hadn't run.
…eview) SupportsThinkingSuppression previously matched on "enable_thinking" alone -- a template could reference that variable name for unrelated semantics or a differently-shaped seed, and blindly appending Qwen's exact <think>\n\n</think>\n\n text would be wrong for such a template, not just redundant. Now requires the literal seed text too, tying detection directly to the mechanism being replicated. Re-verified live against Qwen3.5-9B-Q8_0: identical results (123/128 segment acceptance, 0/128 think-prefixed, 294 mean completion tokens) confirming the tightened check still matches the real GGUF template.
…rage (grok review) 1. ApplyThinkingSuppression's idempotency guard scanned the WHOLE prompt for "<think>" -- any earlier message (conversation history, a document being analyzed) mentioning that literal text for unrelated reasons would false-positive and skip suppression entirely, re-enabling full reasoning mode for the whole call. Narrowed to check only the trimmed tail, which is the actual idempotency concern (double-application / template's own seed). 2. Bug History §7c described detection as matching only the "enable_thinking" marker; code (8cc8920) already requires both that AND the literal empty-seed text. Corrected the narrative. 3. Added regression tests: dual-gate rejects enable_thinking-present/ seed-absent templates, and the tail-only fix (item 1) doesn't false-positive on think-mentioning history content.
Owner
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
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
1 participant
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
enable_thinkingopt-out pattern (append an empty, pre-closed<think></think>seed unlessenable_thinkingis explicitlytrue) — but LLamaSharp'sLLamaTemplate(a minimal Jinja subset) renders through<|im_start|>assistant\nwithout error, never evaluating that trailing conditional. With no seed, the model free-generates its trained default: full reasoning mode, consuming the reader's entire ~2000-token completion budget before any JSON is emitted.ModelAdmissionGatehad already flagged this exact risk when admitting Qwen3.5 as Provisional ("Visible reasoning traces can consume the response budget or precede the required JSON object").LLamaSharpRuntime.SupportsThinkingSuppressiondetects the pattern generically from the rawtokenizer.chat_templatemetadata (not a filename/family check — covers future reasoning models), andApplyThinkingSuppressionappends the same empty think-block the official template would render. Both are pure/static functions with direct unit test coverage.CONTEXT_FABRIC_BUG_HISTORY.md§7c (new failure mode),CONTEXT_FABRIC_INFRASTRUCTURE_NOTES.mdcompatibility table updated.Test plan
dotnet test— 541/541 pass (7 new tests for the two pure functions)<think>-prefixed outputs 128/128 → 0/128, mean completion tokens 2054 → 294 (~7x faster per reader call)🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
<think>output.Documentation