Fix/reasoning close only#2
Conversation
Signed-off-by: Ettore Di Giacinto <mudler@localai.io>
Signed-off-by: Ettore Di Giacinto <mudler@localai.io>
Signed-off-by: Ettore Di Giacinto <mudler@localai.io>
PR SummaryWhat Changed
Key Changes by AreaReasoning Extraction: New package with Configuration: Added OpenAI Endpoint: Replaced Files Changed
Review Focus Areas
ArchitectureDesign Decisions: The options pattern ( Scalability & Extensibility: New model formats can be added by extending tag definitions in Risks: Auto-detection relies on prompt inspection which may miss edge cases. This is acceptable as users can override via configuration. The forced-open mode treats all content as reasoning until a closing tag is found, which could misclassify actual response content if the closing tag is missing. |
![codity-dev-utk[bot]](https://avatars.githubusercontent.com/u/250400887?s=60&v=4){kind=small}
| if endIdx != -1 && (startIdx == -1 || endIdx < startIdx) { | ||
| // Found a closing tag without a preceding opening tag | ||
| closingTagPos := endIdx + lastPos | ||
| if earliestStart == -1 || closingTagPos < earliestStart || (isClosingOnly && closingTagPos < earliestEnd) { |
There was a problem hiding this comment.
The isClosingOnly detection logic is broken. When computing endIdx for the closing-tag-only path, the code uses strings.Index(remaining[lastPos:], tagPair.end): a relative offset: but then compares it directly to startIdx (also a relative offset from lastPos). However, when the condition fires (endIdx < startIdx), the code sets earliestStart = lastPos (treating the entire remaining slice as reasoning) and earliestEnd = closingTagPos + len(tagPair.end) where closingTagPos = endIdx + lastPos. This is correct for that branch. But the bug is in the guard: the condition earliestStart == -1 || closingTagPos < earliestStart || (isClosingOnly && closingTagPos < earliestEnd) mixes earliestStart (repurposed to lastPos) with closingTagPos (absolute). Once isClosingOnly is set, earliestStart is set to lastPos, not to the position of any real tag. Subsequent iterations comparing a new closingTagPos to earliestStart (which equals the old lastPos) will produce incorrect results for content with multiple closing-only tags, potentially swallowing content or producing wrong reasoning boundaries.
Also reported at: pkg/reasoning/reasoning.go L163
Prompt for AI assistance
Copy the prompt below and paste it into ChatGPT, Claude, or any LLM:
You are an expert bash developer with deep knowledge of security, performance, and best practices.
### Context
File: pkg/reasoning/reasoning.go
Lines: 163-163
Issue Type: functional-high
Severity: high
Issue Description:
The `isClosingOnly` detection logic is broken. When computing `endIdx` for the closing-tag-only path, the code uses `strings.Index(remaining[lastPos:], tagPair.end)`: a relative offset: but then compares it directly to `startIdx` (also a relative offset from `lastPos`). However, when the condition fires (`endIdx < startIdx`), the code sets `earliestStart = lastPos` (treating the entire remaining slice as reasoning) and `earliestEnd = closingTagPos + len(tagPair.end)` where `closingTagPos = endIdx + lastPos`. This is correct for that branch. But the bug is in the guard: the condition `earliestStart == -1 || closingTagPos < earliestStart || (isClosingOnly && closingTagPos < earliestEnd)` mixes `earliestStart` (repurposed to `lastPos`) with `closingTagPos` (absolute). Once `isClosingOnly` is set, `earliestStart` is set to `lastPos`, not to the position of any real tag. Subsequent iterations comparing a new `closingTagPos` to `earliestStart` (which equals the old `lastPos`) will produce incorrect results for content with multiple closing-only tags, potentially swallowing content or producing wrong reasoning boundaries.
_Also reported at: `pkg/reasoning/reasoning.go` L163_
Current Code:
if earliestStart == -1 || closingTagPos < earliestStart || (isClosingOnly && closingTagPos < earliestEnd) {
earliest Start = lastPos
earliest End = closingTagPos + len(tagPair.end)
isClosingOnly = true
isUnclosed = false
matchedTag = tagPair
}
---
### Instructions
1. Fix the issue described above
2. Maintain the exact indentation and code style from the original
3. Follow bash best practices and language-specific idioms
4. Ensure the fix addresses the root cause, not just the symptoms
5. Add brief inline comments explaining the fix if needed
### Constraints
- Do not change functionality beyond fixing the identified issue
- Preserve existing variable names and function signatures unless they are part of the problem
- Ensure the fix is production-ready
---
| // Configure reasoning extraction options | ||
| // Auto-detect if prompt ends with thinking tag | ||
| // or use explicit config setting | ||
| thinkingForcedOpen := config.ReasoningConfig.ThinkingForcedOpen || reasoning.DetectThinkingForcedOpen(s) |
There was a problem hiding this comment.
The streaming path computes thinkingForcedOpen using the initial value of s (the first streaming token/chunk) via reasoning.DetectThinkingForcedOpen(s). At the point of evaluation, s is the first chunk emitted by the model: it is NOT the full prompt/predInput. DetectThinkingForcedOpen is designed to check whether the prompt ends with a thinking open tag (to detect models that start outputting reasoning content immediately). Passing the first streamed token instead of the full prompt/predInput defeats the purpose of this check: the first token is model output, not the prompt, so the suffix check will almost never match a thinking tag (and even if it does, it would be coincidental). For the non-streaming and tool-use paths, predInput and prompt are correctly passed. The streaming path should use predInput (or the equivalent prompt variable).
| thinkingForcedOpen := config.ReasoningConfig.ThinkingForcedOpen || reasoning.DetectThinkingForcedOpen(s) | |
| thinkingForcedOpen := config.ReasoningConfig.ThinkingForcedOpen || reasoning.DetectThinkingForcedOpen(predInput) |
Prompt for AI assistance
Copy the prompt below and paste it into ChatGPT, Claude, or any LLM:
You are an expert bash developer with deep knowledge of security, performance, and best practices.
### Context
File: core/http/endpoints/openai/chat.go
Lines: 50-50
Issue Type: functional-high
Severity: high
Issue Description:
The streaming path computes `thinkingForcedOpen` using the initial value of `s` (the first streaming token/chunk) via `reasoning.DetectThinkingForcedOpen(s)`. At the point of evaluation, `s` is the first chunk emitted by the model: it is NOT the full prompt/predInput. `DetectThinkingForcedOpen` is designed to check whether the *prompt* ends with a thinking open tag (to detect models that start outputting reasoning content immediately). Passing the first streamed token instead of the full prompt/predInput defeats the purpose of this check: the first token is model output, not the prompt, so the suffix check will almost never match a thinking tag (and even if it does, it would be coincidental). For the non-streaming and tool-use paths, `predInput` and `prompt` are correctly passed. The streaming path should use `predInput` (or the equivalent prompt variable).
Current Code:
thinkingForcedOpen := config.ReasoningConfig.ThinkingForcedOpen || reasoning.DetectThinkingForcedOpen(s)
---
### Instructions
1. Fix the issue described above
2. Maintain the exact indentation and code style from the original
3. Follow bash best practices and language-specific idioms
4. Ensure the fix addresses the root cause, not just the symptoms
5. Add brief inline comments explaining the fix if needed
### Constraints
- Do not change functionality beyond fixing the identified issue
- Preserve existing variable names and function signatures unless they are part of the problem
- Ensure the fix is production-ready
---
| // Configure reasoning extraction options | ||
| // Auto-detect if prompt ends with thinking tag | ||
| // or use explicit config setting | ||
| thinkingForcedOpen := config.ReasoningConfig.ThinkingForcedOpen || reasoning.DetectThinkingForcedOpen(s) |
There was a problem hiding this comment.
thinkingForcedOpen is computed once before ComputeChoices is called, using the value of s in the outer scope (which at that point is the last value assigned from a prior iteration or the zero value). However, predInput (the actual prompt sent to the model) is built later in the function. If predInput is constructed after this line, the detection will be checking the wrong or empty string. The non-streaming and tool-use paths correctly use predInput and prompt after construction. The streaming path should defer this check to inside the callback, or at minimum ensure predInput is available at the point of the check.
| thinkingForcedOpen := config.ReasoningConfig.ThinkingForcedOpen || reasoning.DetectThinkingForcedOpen(s) | |
| thinkingForcedOpen := config.ReasoningConfig.ThinkingForcedOpen || reasoning.DetectThinkingForcedOpen(predInput) |
Prompt for AI assistance
Copy the prompt below and paste it into ChatGPT, Claude, or any LLM:
You are an expert bash developer with deep knowledge of security, performance, and best practices.
### Context
File: core/http/endpoints/openai/chat.go
Lines: 50-50
Issue Type: robustness-medium
Severity: medium
Issue Description:
`thinkingForcedOpen` is computed once before `ComputeChoices` is called, using the value of `s` in the outer scope (which at that point is the last value assigned from a prior iteration or the zero value). However, `predInput` (the actual prompt sent to the model) is built later in the function. If `predInput` is constructed after this line, the detection will be checking the wrong or empty string. The non-streaming and tool-use paths correctly use `predInput` and `prompt` after construction. The streaming path should defer this check to inside the callback, or at minimum ensure `predInput` is available at the point of the check.
Current Code:
thinkingForcedOpen := config.ReasoningConfig.ThinkingForcedOpen || reasoning.DetectThinkingForcedOpen(s)
---
### Instructions
1. Fix the issue described above
2. Maintain the exact indentation and code style from the original
3. Follow bash best practices and language-specific idioms
4. Ensure the fix addresses the root cause, not just the symptoms
5. Add brief inline comments explaining the fix if needed
### Constraints
- Do not change functionality beyond fixing the identified issue
- Preserve existing variable names and function signatures unless they are part of the problem
- Ensure the fix is production-ready
---
Security Scan Summary
No critical security issues detected Scan completed in 21.5sSecurity scan powered by Codity.ai |
License Compliance Scan
Weak copyleft licenses found - verify compatibility Some packages have unknown licenses - manual review required Medium Risk Licenses - 4 packagesMPL-2.0 (4 packages):
Unknown Licenses - 36 packages
...and 16 more Powered by Codity.ai · Docs |
|
@codity review |
PR SummaryWhat Changed
Key Changes by AreaReasoning Extraction: New Configuration: Added API Integration: Updated OpenAI chat endpoint to use new Files Changed
Review Focus Areas
ArchitectureDesign Decisions: The forced-open mode detection is intentionally duplicated across three code paths in the chat endpoint to avoid premature abstraction. The tradeoff accepts maintenance burden for clarity in each path. Risks: The repeated detection pattern in |
| // Configure reasoning extraction options | ||
| // Auto-detect if prompt ends with thinking tag | ||
| // or use explicit config setting | ||
| thinkingForcedOpen := config.ReasoningConfig.ThinkingForcedOpen || reasoning.DetectThinkingForcedOpen(s) |
There was a problem hiding this comment.
The streaming path computes thinkingForcedOpen once, before the streaming loop starts, using the initial value of s (the prompt). However, the outer variable s is reassigned inside each streaming callback invocation (func(s string, …)), which shadows the outer s. The real concern is that DetectThinkingForcedOpen(s) is called with whatever s happens to be at closure-capture time: which is the pre-streaming prompt string. This is actually intentional for detection, BUT the detection happens before ComputeChoices is called, so s here is still the prompt, which is correct. However, if s is the raw prompt and hasn't been processed by the template engine yet (i.e., template expansion happens inside ComputeChoices), the detection will be applied against the un-expanded prompt. In contrast, the tools path and non-streaming path use prompt and predInput respectively: variables that presumably hold the already-rendered prompt. This inconsistency means the auto-detection will produce different results in the streaming path vs. the other two paths for the same model/request.
| thinkingForcedOpen := config.ReasoningConfig.ThinkingForcedOpen || reasoning.DetectThinkingForcedOpen(s) | |
| thinkingForcedOpen := config.ReasoningConfig.ThinkingForcedOpen || reasoning.DetectThinkingForcedOpen(predInput) |
Prompt for AI assistance
Copy the prompt below and paste it into ChatGPT, Claude, or any LLM:
You are an expert bash developer with deep knowledge of security, performance, and best practices.
### Context
File: core/http/endpoints/openai/chat.go
Lines: 50-50
Issue Type: functional-high
Severity: high
Issue Description:
The streaming path computes `thinkingForcedOpen` once, before the streaming loop starts, using the initial value of `s` (the prompt). However, the outer variable `s` is reassigned inside each streaming callback invocation (`func(s string, …)`), which shadows the outer `s`. The real concern is that `DetectThinkingForcedOpen(s)` is called with whatever `s` happens to be at closure-capture time: which is the pre-streaming prompt string. This is actually intentional for detection, BUT the detection happens before `ComputeChoices` is called, so `s` here is still the prompt, which is correct. However, if `s` is the *raw* prompt and hasn't been processed by the template engine yet (i.e., template expansion happens inside `ComputeChoices`), the detection will be applied against the un-expanded prompt. In contrast, the tools path and non-streaming path use `prompt` and `predInput` respectively: variables that presumably hold the already-rendered prompt. This inconsistency means the auto-detection will produce different results in the streaming path vs. the other two paths for the same model/request.
Current Code:
thinkingForcedOpen := config.ReasoningConfig.ThinkingForcedOpen || reasoning.DetectThinkingForcedOpen(s)
---
### Instructions
1. Fix the issue described above
2. Maintain the exact indentation and code style from the original
3. Follow bash best practices and language-specific idioms
4. Ensure the fix addresses the root cause, not just the symptoms
5. Add brief inline comments explaining the fix if needed
### Constraints
- Do not change functionality beyond fixing the identified issue
- Preserve existing variable names and function signatures unless they are part of the problem
- Ensure the fix is production-ready
---
| endIdx := strings.Index(remaining[lastPos:], tagPair.end) | ||
|
|
||
| // Check for closing-only tag (closing tag appears before or without opening tag) | ||
| if endIdx != -1 && (startIdx == -1 || endIdx < startIdx) { |
There was a problem hiding this comment.
The closing-only tag comparison in extractFromTags is incorrect. When a closing-only match is found, earliestStart is set to lastPos and earliestEnd is set to closingTagPos + len(tagPair.end). Later, when choosing among candidates, the guard closingTagPos < earliestStart compares against lastPos (not the position of the previously found closing tag). Additionally isClosingOnly && closingTagPos < earliestEnd compares against earliestEnd which was set to closingTagPos + len(previousTag.end), not closingTagPos. This means when two different closing tags appear at different positions, the selection logic may incorrectly retain a later tag over an earlier one. For example if </think> appears at offset 10 and </thinking> appears at offset 5, the function may still select </think> depending on iteration order. The correct comparator should use the actual closing tag position rather than derived values.
Suggested fix
if endIdx != -1 && (startIdx == -1 || endIdx < startIdx) {
// Found a closing tag without a preceding opening tag
closingTagPos := endIdx + lastPos
if earliestStart == -1 || (isClosingOnly && closingTagPos < earliestEnd-len(matchedTag.end)) || (!isClosingOnly && (earliestStart == -1 || closingTagPos < earliestStart)) {
earliestStart = lastPos
earliestEnd = closingTagPos + len(tagPair.end)
isClosingOnly = true
isUnclosed = false
matchedTag = tagPair
}
continue
}Prompt for AI assistance
Copy the prompt below and paste it into ChatGPT, Claude, or any LLM:
You are an expert bash developer with deep knowledge of security, performance, and best practices.
### Context
File: pkg/reasoning/reasoning.go
Lines: 160-160
Issue Type: functional-high
Severity: high
Issue Description:
The closing-only tag comparison in `extractFromTags` is incorrect. When a closing-only match is found, `earliestStart` is set to `lastPos` and `earliestEnd` is set to `closingTagPos + len(tagPair.end)`. Later, when choosing among candidates, the guard `closingTagPos < earliestStart` compares against `lastPos` (not the position of the previously found closing tag). Additionally `isClosingOnly && closingTagPos < earliestEnd` compares against `earliestEnd` which was set to `closingTagPos + len(previousTag.end)`, not `closingTagPos`. This means when two different closing tags appear at different positions, the selection logic may incorrectly retain a later tag over an earlier one. For example if `</think>` appears at offset 10 and `</thinking>` appears at offset 5, the function may still select `</think>` depending on iteration order. The correct comparator should use the actual closing tag position rather than derived values.
Current Code:
if endIdx != -1 && (startIdx == -1 || endIdx < startIdx) {
// Found a closing tag without a preceding opening tag
closingTagPos := endIdx + lastPos
if earliestStart == -1 || closingTagPos < earliestStart || (isClosingOnly && closingTagPos < earliestEnd) {
earliestStart = lastPos
earliestEnd = closingTagPos + len(tagPair.end)
isClosingOnly = true
isUnclosed = false
matchedTag = tagPair
}
continue
}
---
### Instructions
1. Fix the issue described above
2. Maintain the exact indentation and code style from the original
3. Follow bash best practices and language-specific idioms
4. Ensure the fix addresses the root cause, not just the symptoms
5. Add brief inline comments explaining the fix if needed
### Constraints
- Do not change functionality beyond fixing the identified issue
- Preserve existing variable names and function signatures unless they are part of the problem
- Ensure the fix is production-ready
---
| accumulatedContent += s | ||
| // Extract reasoning from accumulated content | ||
| currentReasoning, cleanedContent := functions.ExtractReasoning(accumulatedContent) | ||
| opts := []reasoning.Option{} |
There was a problem hiding this comment.
Inside the streaming token callback, opts := []reasoning.Option{} allocates a new slice on every invocation, and reasoning.Extract is called on the entire accumulated content (not just the new token). For a long response this results in O(n²) work for reasoning extraction. The opts slice construction is also slightly wasteful: it should be pre-built once outside the callback closure, similar to how the non-streaming path does it.
| opts := []reasoning.Option{} | |
| currentReasoning, cleanedContent := reasoning.Extract(accumulatedContent, extractOpts...) |
Prompt for AI assistance
Copy the prompt below and paste it into ChatGPT, Claude, or any LLM:
You are an expert bash developer with deep knowledge of security, performance, and best practices.
### Context
File: core/http/endpoints/openai/chat.go
Lines: 55-55
Issue Type: performance-medium
Severity: medium
Issue Description:
Inside the streaming token callback, `opts := []reasoning.Option{}` allocates a new slice on every invocation, and `reasoning.Extract` is called on the **entire accumulated content** (not just the new token). For a long response this results in O(n²) work for reasoning extraction. The `opts` slice construction is also slightly wasteful: it should be pre-built once outside the callback closure, similar to how the non-streaming path does it.
Current Code:
opts := []reasoning.Option{}
if thinkingForcedOpen {
opts = append(opts, reasoning.WithThinkingForcedOpen())
}
currentReasoning, cleanedContent := reasoning.Extract(accumulatedContent, opts...)
---
### Instructions
1. Fix the issue described above
2. Maintain the exact indentation and code style from the original
3. Follow bash best practices and language-specific idioms
4. Ensure the fix addresses the root cause, not just the symptoms
5. Add brief inline comments explaining the fix if needed
### Constraints
- Do not change functionality beyond fixing the identified issue
- Preserve existing variable names and function signatures unless they are part of the problem
- Ensure the fix is production-ready
---
| if endIdx != -1 && (startIdx == -1 || endIdx < startIdx) { | ||
| // Found a closing tag without a preceding opening tag | ||
| closingTagPos := endIdx + lastPos | ||
| if earliestStart == -1 || closingTagPos < earliestStart || (isClosingOnly && closingTagPos < earliestEnd) { |
There was a problem hiding this comment.
In the closing-only path inside extractFromTags, closingTagPos is computed as endIdx + lastPos, but endIdx is the result of strings.Index(remaining[lastPos:], tagPair.end): i.e., it is already relative to remaining[lastPos:]. Adding lastPos again is correct and gives the absolute position in remaining. However the check closingTagPos < earliestStart compares the absolute position of the new candidate's closing tag against earliestStart, which for a previous closing-only candidate was set to lastPos (NOT to the previous closing tag's position). So a closing tag at position 5 could be skipped in favour of one at position 50 simply because a prior iteration set earliestStart = lastPos = 0. The guard condition must compare the candidate closing tag's position against the previously-matched closing tag's position (i.e., earliestEnd - len(matchedTag.end)).
Also reported at: pkg/reasoning/reasoning.go L211
Suggested fix
prevClosingTagPos := earliestEnd - len(matchedTag.end)
if earliestStart == -1 || (!isClosingOnly && closingTagPos < earliestStart) || (isClosingOnly && closingTagPos < prevClosingTagPos) {Prompt for AI assistance
Copy the prompt below and paste it into ChatGPT, Claude, or any LLM:
You are an expert bash developer with deep knowledge of security, performance, and best practices.
### Context
File: pkg/reasoning/reasoning.go
Lines: 163-163
Issue Type: functional-medium
Severity: medium
Issue Description:
In the closing-only path inside `extractFromTags`, `closingTagPos` is computed as `endIdx + lastPos`, but `endIdx` is the result of `strings.Index(remaining[lastPos:], tagPair.end)`: i.e., it is already relative to `remaining[lastPos:]`. Adding `lastPos` again is correct and gives the absolute position in `remaining`. However the check `closingTagPos < earliestStart` compares the absolute position of the new candidate's closing tag against `earliestStart`, which for a previous closing-only candidate was set to `lastPos` (NOT to the previous closing tag's position). So a closing tag at position 5 could be skipped in favour of one at position 50 simply because a prior iteration set `earliestStart = lastPos = 0`. The guard condition must compare the candidate closing tag's position against the previously-matched closing tag's position (i.e., `earliestEnd - len(matchedTag.end)`).
_Also reported at: `pkg/reasoning/reasoning.go` L211_
Current Code:
if earliestStart == -1 || closingTagPos < earliestStart || (isClosingOnly && closingTagPos < earliestEnd) {
---
### Instructions
1. Fix the issue described above
2. Maintain the exact indentation and code style from the original
3. Follow bash best practices and language-specific idioms
4. Ensure the fix addresses the root cause, not just the symptoms
5. Add brief inline comments explaining the fix if needed
### Constraints
- Do not change functionality beyond fixing the identified issue
- Preserve existing variable names and function signatures unless they are part of the problem
- Ensure the fix is production-ready
---
| lastPos := 0 | ||
|
|
||
| for { | ||
| // Find the earliest tag start |
There was a problem hiding this comment.
Inside the extractFromTags loop, tagPairs is a slice of anonymous structs allocated on every call. While Go will optimise this to a compile-time constant, the inner loop also calls strings.Index(remaining[lastPos:], tagPair.start) and strings.Index(remaining[lastPos:], tagPair.end) independently, creating two sub-string searches per tag pair per iteration. This is called on every streaming chunk (potentially thousands of times during a single response), and on the entire accumulated content each time. For large responses this is O(n²) in total work. The old implementation had the same issue, but the new multi-tag support doubles the constant factor. Consider operating only on the new suffix of accumulated content rather than re-scanning from the beginning.
Prompt for AI assistance
Copy the prompt below and paste it into ChatGPT, Claude, or any LLM:
You are an expert bash developer with deep knowledge of security, performance, and best practices.
### Context
File: pkg/reasoning/reasoning.go
Lines: 145-145
Issue Type: performance-medium
Severity: medium
Issue Description:
Inside the `extractFromTags` loop, `tagPairs` is a slice of anonymous structs allocated on every call. While Go will optimise this to a compile-time constant, the inner loop also calls `strings.Index(remaining[lastPos:], tagPair.start)` and `strings.Index(remaining[lastPos:], tagPair.end)` independently, creating two sub-string searches per tag pair per iteration. This is called on every streaming chunk (potentially thousands of times during a single response), and on the *entire accumulated content* each time. For large responses this is O(n²) in total work. The old implementation had the same issue, but the new multi-tag support doubles the constant factor. Consider operating only on the new suffix of accumulated content rather than re-scanning from the beginning.
Current Code:
for {
// Find the earliest tag start
earliestStart := -1
earliestEnd := -1
isUnclosed := false
isClosingOnly := false
---
### Instructions
1. Fix the issue described above
2. Maintain the exact indentation and code style from the original
3. Follow bash best practices and language-specific idioms
4. Ensure the fix addresses the root cause, not just the symptoms
5. Add brief inline comments explaining the fix if needed
### Constraints
- Do not change functionality beyond fixing the identified issue
- Preserve existing variable names and function signatures unless they are part of the problem
- Ensure the fix is production-ready
---
| } | ||
| } | ||
|
|
||
| if earliestCloseIdx == -1 { |
There was a problem hiding this comment.
In extractForcedOpen, when no closing tag is found, the entire content is returned as reasoning with TrimSpace applied. This means that during streaming, each incremental call strips leading/trailing whitespace from partial reasoning. If the model is mid-sentence and the partial chunk ends with a space or newline, that trailing whitespace is permanently lost before the final response is assembled. This can mangle the reasoning content, especially for models that produce significant leading whitespace (e.g. <think> → forced open → content starts with a newline that gets trimmed on each streaming tick).
Suggested fix
if earliestCloseIdx == -1 {
// No closing tag found - all content is reasoning (still streaming)
// Do not trim here; trimming is applied only when the closing tag is found
return content, ""
}Prompt for AI assistance
Copy the prompt below and paste it into ChatGPT, Claude, or any LLM:
You are an expert bash developer with deep knowledge of security, performance, and best practices.
### Context
File: pkg/reasoning/reasoning.go
Lines: 91-91
Issue Type: robustness-medium
Severity: medium
Issue Description:
In `extractForcedOpen`, when no closing tag is found, the entire content is returned as reasoning with `TrimSpace` applied. This means that during streaming, each incremental call strips leading/trailing whitespace from partial reasoning. If the model is mid-sentence and the partial chunk ends with a space or newline, that trailing whitespace is permanently lost before the final response is assembled. This can mangle the reasoning content, especially for models that produce significant leading whitespace (e.g. `<think>
` → forced open → content starts with a newline that gets trimmed on each streaming tick).
Current Code:
if earliestCloseIdx == -1 {
// No closing tag found - all content is reasoning (still streaming)
return strings.TrimSpace(content), ""
}
---
### Instructions
1. Fix the issue described above
2. Maintain the exact indentation and code style from the original
3. Follow bash best practices and language-specific idioms
4. Ensure the fix addresses the root cause, not just the symptoms
5. Add brief inline comments explaining the fix if needed
### Constraints
- Do not change functionality beyond fixing the identified issue
- Preserve existing variable names and function signatures unless they are part of the problem
- Ensure the fix is production-ready
---
|
|
||
| // Common thinking/reasoning opening tags used by various models. | ||
| // These match the tags detected by llama.cpp in common/chat.cpp | ||
| var thinkingOpenTags = []string{ |
There was a problem hiding this comment.
The thinkingOpenTags list used by DetectThinkingForcedOpen and the closing tags list inside extractForcedOpen are not kept in sync with the tag pairs list inside extractFromTags. For example, [THINK] is in thinkingOpenTags but only [THINK] (without newline) is in tagPairs as an opening tag. Similarly, <think> and <thinking> are in thinkingOpenTags but the tag-pairs list uses <think> and <thinking> (without the newline). If a model's prompt ends with <think> (newline variant) the detection fires, but extractFromTags will never find a matching <think> opening tag and will fall back to the closing-only path. The three lists should be consolidated or cross-referenced to avoid drift.
Prompt for AI assistance
Copy the prompt below and paste it into ChatGPT, Claude, or any LLM:
You are an expert bash developer with deep knowledge of security, performance, and best practices.
### Context
File: pkg/reasoning/reasoning.go
Lines: 9-9
Issue Type: maintainability-medium
Severity: medium
Issue Description:
The `thinkingOpenTags` list used by `DetectThinkingForcedOpen` and the closing tags list inside `extractForcedOpen` are not kept in sync with the tag pairs list inside `extractFromTags`. For example, `[THINK]
` is in `thinkingOpenTags` but only `[THINK]` (without newline) is in `tagPairs` as an opening tag. Similarly, `<think>
` and `<thinking>
` are in `thinkingOpenTags` but the tag-pairs list uses `<think>` and `<thinking>` (without the newline). If a model's prompt ends with `<think>
` (newline variant) the detection fires, but `extractFromTags` will never find a matching `<think>
` opening tag and will fall back to the closing-only path. The three lists should be consolidated or cross-referenced to avoid drift.
Current Code:
var thinkingOpenTags = []string{
// DeepSeek R1, V3.1, Nemotron V2, MiniMax M2, Hermes 2 Pro, Granite, Exaone MOE
"<think>\n",
"<think>",
// Generic thinking tags
"<thinking>\n",
"<thinking>",
// Apertus
"<|inner_prefix|>",
// Command R7B
"<|START_THINKING|>",
// Seed
"<seed:think>",
// Magistral (not in llama.cpp but common)
"[THINK]\n",
"[THINK]",
}
---
### Instructions
1. Fix the issue described above
2. Maintain the exact indentation and code style from the original
3. Follow bash best practices and language-specific idioms
4. Ensure the fix addresses the root cause, not just the symptoms
5. Add brief inline comments explaining the fix if needed
### Constraints
- Do not change functionality beyond fixing the identified issue
- Preserve existing variable names and function signatures unless they are part of the problem
- Ensure the fix is production-ready
---
Nitpicks (Low Priority)Found 1 low-priority suggestions for code improvement Click to expand nitpicks
|
Security Scan Summary
No critical security issues detected Scan completed in 14.4sSecurity scan powered by Codity.ai |
2 participants
Description
This PR fixes #
Notes for Reviewers
Signed commits