fix: migration ModelName lookup, reasoning_content, shell regex, loop boundary#932
fix: migration ModelName lookup, reasoning_content, shell regex, loop boundary#9324 commits merged intosipeed:mainfrom
Conversation
1. migration.go: Set ModelName to userModel when provider matches so GetModelConfig(userModel) can find the entry. Previously the migration created entries with the provider name as ModelName (e.g. "moonshot") but lookup used the model name (e.g. "k2p5"), causing "model not found". 2. openai_compat/provider.go: Preserve reasoning_content in conversation history. Thinking models (e.g. Kimi K2, DeepSeek-R1) return reasoning_content which must be echoed back. Without it, APIs return 400: "thinking is enabled but reasoning_content is missing". 3. shell.go: Fix deny pattern regex for format/mkfs/diskpart to use (?:^|\s) instead of \b to avoid matching --format flags. Fix path extraction regex to use submatch to avoid matching flags like -rf as paths. 4. loop.go: Adjust forceCompression mid-point to avoid splitting tool-call/result message pairs, which causes API errors.
There was a problem hiding this comment.
Pull request overview
This PR addresses several startup/runtime correctness issues across config migration, OpenAI-compatible provider interoperability, shell command safety guarding, and agent history compression.
Changes:
- Fix provider→model_list migration so the user-selected model can be found via
GetModelConfig(). - Preserve
reasoning_contentin outgoing OpenAI-compatible conversation history for “thinking” models. - Refine shell safety regexes and adjust history compression midpoint to avoid orphaning tool-result messages.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| pkg/config/migration.go | Updates migrated ModelName to match the user-selected model for correct lookup. |
| pkg/providers/openai_compat/provider.go | Adds reasoning_content to the OpenAI-compatible wire message format. |
| pkg/tools/shell.go | Adjusts deny-pattern and path extraction regex used by the exec tool safety guard. |
| pkg/agent/loop.go | Tweaks forced compression midpoint logic to avoid splitting tool call/result pairs. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Deny regex: expand left boundary to match shell separators (;, &&, ||) to prevent bypass via chained commands like ";format c:" - Path regex: add "." to initial char class to catch hidden dirs (/.ssh), add "=" to left boundary to catch flag-attached paths (--file=/etc/passwd) - Add test: ModelName must match user model for GetModelConfig lookup - Add test: stripSystemParts preserves reasoning_content in wire format - Add test: forceCompression avoids orphaning tool result messages - Add test: deny pattern blocks disk-wiping commands with shell separators while allowing legitimate --format flags Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
nikolasdehor
left a comment
nikolasdehor
left a comment
There was a problem hiding this comment.
LGTM. Four well-targeted fixes, each with regression tests:
-
migration.go ModelName fix: Sets mc.ModelName = userModel so GetModelConfig(userModel) can find migrated entries. Fixes gateway startup failure when user model name differs from provider name.
-
openai_compat reasoning_content: Adds ReasoningContent field to openaiMessage so thinking models (Kimi K2, DeepSeek-R1) get their reasoning echoed back in API calls. Without this, APIs return 400 errors.
-
shell.go deny pattern fix: Replaces word-boundary regex with shell-separator-aware pattern to prevent --format flags from triggering the disk-wipe deny. Path extraction regex also fixed to use submatch groups.
-
loop.go forceCompression: Advances mid-point by 1 when it lands on a tool-result message, keeping assistant(tool_call)+tool pairs together and preventing orphaned tool result API errors.
All four are independent, well-tested, and address real user-facing failures.
Resolve conflicts: - provider.go: keep upstream's serializeMessages (supersedes stripSystemParts) - provider_test.go: keep upstream's serializeMessages tests - loop_test.go: add slices import needed by upstream tests - shell.go: merge PR's --format deny fix with upstream's block device pattern, safePaths, and absolutePathPattern - shell_test.go: include tests from both branches Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…test Address Copilot review feedback: - Use guardCommand() instead of Execute() to avoid running dangerous commands (format, mkfs, diskpart) on the host if regex regresses - Assert error message contains "blocked" for blocked commands - Replace "go fmt ./..." with "echo go fmt ./..." to avoid accidental file modification Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Deny regex: expand left boundary to match shell separators (;, &&, ||) to prevent bypass via chained commands like ";format c:" - Path regex: add "." to initial char class to catch hidden dirs (/.ssh), add "=" to left boundary to catch flag-attached paths (--file=/etc/passwd) - Add test: ModelName must match user model for GetModelConfig lookup - Add test: stripSystemParts preserves reasoning_content in wire format - Add test: forceCompression avoids orphaning tool result messages - Add test: deny pattern blocks disk-wiping commands with shell separators while allowing legitimate --format flags Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Deny regex: expand left boundary to match shell separators (;, &&, ||) to prevent bypass via chained commands like ";format c:" - Path regex: add "." to initial char class to catch hidden dirs (/.ssh), add "=" to left boundary to catch flag-attached paths (--file=/etc/passwd) - Add test: ModelName must match user model for GetModelConfig lookup - Add test: stripSystemParts preserves reasoning_content in wire format - Add test: forceCompression avoids orphaning tool result messages - Add test: deny pattern blocks disk-wiping commands with shell separators while allowing legitimate --format flags Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Summary
migration.go: Set
ModelNametouserModelwhen the user's configured provider matches duringConvertProvidersToModelList. Previously the migration created entries with the provider name asModelName(e.g."moonshot") butGetModelConfig()looked up using the model name (e.g."k2p5"), causing"model not found"errors and preventing gateway startup.openai_compat/provider.go: Preserve
reasoning_contentin conversation history sent back to APIs. Thinking models (e.g. Kimi K2, DeepSeek-R1) returnreasoning_contentwhich must be echoed back in subsequent requests. Without it, APIs return400: "thinking is enabled but reasoning_content is missing in assistant tool call message".shell.go: Fix deny pattern regex for
format/mkfs/diskpart— use(?:^|\s)instead of\bto avoid false-positive matches on flags like--format. Fix path extraction regex to use submatch groups to avoid matching flags (e.g.-rf) as filesystem paths.loop.go: Adjust
forceCompressionmid-point to avoid splitting tool-call/result message pairs. When the mid-point lands on atoolrole message, advance it by one to keep the assistant→tool pair together, preventing API errors from orphaned tool results.Test plan
go build ./pkg/...passesgo vet ./pkg/...passes--formatflag in shell commands no longer triggers deny pattern