Improve AI response parsing and sanitization for command suggestions#273
Improve AI response parsing and sanitization for command suggestions#273
Conversation
Per SSE spec §9.2 the value after "data:" or "event:" may have a single optional leading space that parsers must strip. The previous parser stripped the prefix but not the space, so Gin's "data: [DONE]" was compared as " [DONE]" and the stream never terminated cleanly. Forwarded tokens were also prefixed with stray whitespace. - Extract stripSSEField helper that removes at most one leading space. - Apply it to both event: and data: field names. - Reset isError on blank-line dispatch (event boundary), not inside the data: branch. - Recognize "event:error" in addition to "event: error". https://claude.ai/code/session_01Qa2FSi9HhA49Z4LTMmpxm9
The suggested command often arrived wrapped in Markdown code fences or surrounding single backticks, which broke shell execution. Extract a sanitizeSuggestedCommand helper that strips triple-backtick fences (with optional bash/sh/shell/zsh/fish/pwsh/powershell language tag) and single-line backtick wrapping, then trims whitespace. Fail fast with a clear user-facing error on empty output instead of classifying an empty string as ActionOther and silently returning. https://claude.ai/code/session_01Qa2FSi9HhA49Z4LTMmpxm9
![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: 5373bf2b76
ℹ️ 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".
| } | ||
| } | ||
| s = strings.TrimRight(s, " \t\n") | ||
| s = strings.TrimSuffix(s, "```") |
There was a problem hiding this comment.
When the AI returns a fenced command followed by explanatory prose (for example bash\nls -la\n```\nThis lists files), sanitizeSuggestedCommand only removes a closing fence if it is the final suffix, so it leaves \n```\nThis lists files in the command. Because classification uses the first token, this can now be auto-run as a view/edit command while still containing non-command prose, causing unintended shell execution and failures whenever auto-run is enabled. The sanitizer should extract content up to the first closing fence and ignore trailing text before classification/execution.
Useful? React with 👍 / 👎.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
![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 implements a sanitizeSuggestedCommand utility to normalize AI-generated commands by stripping markdown fences and backticks, and refactors the SSE parsing logic to improve compliance with the specification. The changes include new unit tests for both the sanitization logic and the SSE stream processing. Review feedback suggests refining the SSE parser to ensure the error state is explicitly updated for all event fields and to support multi-line error messages.
| if v == "error" { | ||
| isError = true | ||
| } |
There was a problem hiding this comment.
The isError state should be explicitly set based on whether the event type is exactly "error". Currently, if an event is marked as "error" and then a subsequent event in the same stream (before a blank line) has a different event type, isError will remain true. While SSE events are typically separated by blank lines, it's safer to update the state for every event: field encountered.
| if v == "error" { | |
| isError = true | |
| } | |
| if v, ok := stripSSEField(line, "event:"); ok { | |
| isError = (v == "error") | |
| continue | |
| } |
| if isError { | ||
| return fmt.Errorf("server error: %s", data) | ||
| return fmt.Errorf("server error: %s", v) | ||
| } |
There was a problem hiding this comment.
If the server sends a multi-line error message (multiple data: lines for a single event: error), this implementation will return an error containing only the first line. Consider buffering the error message until the event is fully received (at the blank line) or concatenating it if multi-line errors are expected.
Codecov Report✅ All modified and coverable lines are covered by tests.
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 98 files with indirect coverage changes 🚀 New features to boost your workflow:
|
2 participants
Summary
This PR enhances the AI service's Server-Sent Events (SSE) parsing and adds robust sanitization of AI-generated command suggestions to handle various formatting styles and edge cases.
Key Changes
SSE Parsing Improvements (
model/ai_service.go)stripSSEField()helper function that properly parses SSE field values according to the SSE specification, handling optional leading spacesevent:anddata:fields[DONE]markers with or without leading spacesCommand Sanitization (
commands/query.go)sanitizeSuggestedCommand()function that normalizes raw AI output into executable commands#) verbatim for user visibilityTest Coverage
TestQueryCommandStream_SSEParsing) covering:[DONE]marker formatsstripSSEField()helper functionsanitizeSuggestedCommand()covering:Notable Implementation Details
https://claude.ai/code/session_01Qa2FSi9HhA49Z4LTMmpxm9