revert(cli): undo PR #273 AI response parsing changes#275
Conversation
Reverts commit 88967e8 (Merge pull request #273), which introduced sanitizeSuggestedCommand and SSE leading-space handling. The merge turned out to be wrong and is being rolled back. Conflict in commands/query.go was resolved by preserving the getSystemContext(query, ai *model.AIConfig) signature introduced by unrelated PR #274.
Codecov Report❌ Patch coverage is
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 1 file with indirect coverage changes 🚀 New features to boost your workflow:
|
![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: 5a30d5c055
ℹ️ 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".
| continue | ||
| } | ||
| if strings.HasPrefix(line, "data:") { | ||
| data := line[len("data:"):] |
There was a problem hiding this comment.
[DONE] detection
The stream parser takes line[len("data:"):] verbatim, so a valid SSE frame like data: [DONE] yields " [DONE]" and never matches the terminator check. In that case the sentinel is emitted to onToken as part of the suggested command and the loop only ends on EOF, which can break command output and streaming behavior for standards-compliant SSE servers that include the optional space after the colon.
Useful? React with 👍 / 👎.
| color.Red.Println("AI returned an empty response. Try rephrasing your query.") | ||
| return fmt.Errorf("empty AI response") | ||
| } | ||
| newCommand := strings.TrimSpace(result.String()) |
There was a problem hiding this comment.
Replacing sanitizeSuggestedCommand with a plain TrimSpace means markdown-formatted model output (for example fenced blocks or inline backticks) is now passed through unchanged. When auto-run is enabled, executeCommand receives those backticks via shell -c, which can trigger shell parsing/command-substitution errors instead of executing the intended command; classification logic also runs on the malformed text.
Useful? React with 👍 / 👎.
![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 simplifies the command sanitization and SSE parsing logic by removing the sanitizeSuggestedCommand and stripSSEField functions along with their associated tests. Feedback indicates that these changes introduce several regressions: the SSE parser no longer correctly handles optional leading spaces or varied event: error formatting per the SSE specification, which can lead to [DONE] tokens being incorrectly processed as commands. Additionally, removing the sanitization logic prevents the CLI from stripping markdown code fences, likely causing shell execution failures when AI responses are formatted. It is also noted that the removal of comprehensive unit tests significantly reduces coverage for these edge cases.
| if strings.HasPrefix(line, "data:") { | ||
| data := line[len("data:"):] | ||
|
|
||
| if v, ok := stripSSEField(line, "data:"); ok { | ||
| if isError { | ||
| return fmt.Errorf("server error: %s", v) | ||
| return fmt.Errorf("server error: %s", data) | ||
| } | ||
| if v == "[DONE]" { | ||
|
|
||
| if data == "[DONE]" { | ||
| return nil | ||
| } | ||
| onToken(v) | ||
|
|
||
| onToken(data) | ||
| isError = false | ||
| } |
There was a problem hiding this comment.
This logic fails to handle the optional leading space in SSE data fields correctly. Per the SSE spec (§9.2), a single leading space after the colon should be removed. Most importantly, if a server sends data: [DONE] (with the standard space), the data variable will contain " [DONE]". This causes the check at line 89 to fail, and the string [DONE] will be passed to onToken, resulting in it being printed to the user's terminal and appended to the command string.
| if strings.HasPrefix(line, "data:") { | |
| data := line[len("data:"):] | |
| if v, ok := stripSSEField(line, "data:"); ok { | |
| if isError { | |
| return fmt.Errorf("server error: %s", v) | |
| return fmt.Errorf("server error: %s", data) | |
| } | |
| if v == "[DONE]" { | |
| if data == "[DONE]" { | |
| return nil | |
| } | |
| onToken(v) | |
| onToken(data) | |
| isError = false | |
| } | |
| if strings.HasPrefix(line, "data:") { | |
| data := strings.TrimPrefix(line, "data:") | |
| if strings.HasPrefix(data, " ") { | |
| data = data[1:] | |
| } | |
| if isError { | |
| return fmt.Errorf("server error: %s", data) | |
| } | |
| if data == "[DONE]" { | |
| return nil | |
| } | |
| onToken(data) | |
| isError = false | |
| } |
| color.Red.Println("AI returned an empty response. Try rephrasing your query.") | ||
| return fmt.Errorf("empty AI response") | ||
| } | ||
| newCommand := strings.TrimSpace(result.String()) |
There was a problem hiding this comment.
By removing the sanitizeSuggestedCommand function, the CLI no longer strips markdown code fences (e.g., ```bash ... ```) from the AI's response. If the AI returns a formatted response, newCommand will contain these backticks, causing the shell execution at line 139 to fail with a syntax error. Since many LLMs default to markdown, it is recommended to at least strip triple-backtick fences.
| if line == "event: error" { | ||
| isError = true | ||
| continue | ||
| } |
There was a problem hiding this comment.
The string comparison line == "event: error" is too restrictive. The SSE specification allows for an optional space after the colon, and some servers might omit it (e.g., event:error). This hardcoded string comparison will fail to detect errors if the server's formatting varies slightly while remaining spec-compliant.
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
The removal of TestQueryCommandStream_SSEParsing and TestStripSSEField significantly reduces the test coverage for the streaming logic. Even if the implementation is being simplified, it's important to maintain tests that verify how the service handles various SSE inputs (like leading spaces, error events, and the [DONE] signal) to prevent regressions.
1 participant
Summary
getSystemContext(query, ai *model.AIConfig)signature from the later, unrelated feat(cli): send pwd and hostname with shelltime q #274 during conflict resolution.Test plan
go build ./...passesgo test ./model/...passesshelltime qstill streams AI responses end-to-end after merge🤖 Generated with Claude Code