feat(guardrails): route rewrite calls through workflows#209
feat(guardrails): route rewrite calls through workflows#209SantiagoDePolonia wants to merge 2 commits intomainfrom
Conversation
📝 WalkthroughWalkthroughThis PR introduces an "llm-based altering" guardrail type that rewrites user messages via an auxiliary LLM model before processing the main provider request. Changes include new guardrail executor implementation, config schema extensions, request origin context tracking, internal chat completion executor for auxiliary model calls, and dashboard UI updates to support the new guardrail type. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Server
participant GuardrailService
participant LLMBasedAlteringGuardrail
participant InternalExecutor
participant AuxiliaryProvider
participant MainProvider
Client->>Server: ChatCompletion Request
Server->>GuardrailService: Process(messages)
GuardrailService->>LLMBasedAlteringGuardrail: Process(messages)
LLMBasedAlteringGuardrail->>LLMBasedAlteringGuardrail: Filter messages by role
loop For each message to rewrite
LLMBasedAlteringGuardrail->>InternalExecutor: ChatCompletion(rewrite request)
Note over InternalExecutor: Set RequestOriginGuardrail<br/>Disable guardrails in policy
InternalExecutor->>AuxiliaryProvider: Route rewrite request
AuxiliaryProvider-->>InternalExecutor: Rewritten text response
InternalExecutor-->>LLMBasedAlteringGuardrail: Return rewritten content
end
LLMBasedAlteringGuardrail-->>GuardrailService: Return altered messages
GuardrailService-->>Server: Return processed messages
Server->>MainProvider: Forward altered ChatCompletion Request
MainProvider-->>Server: Response
Server-->>Client: Response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| if rewrittenText == "" { | ||
| return cloneContentParts(parts), nil | ||
| } | ||
| prepended := make([]core.ContentPart, 0, len(parts)+1) |
| if rewrittenText == "" { | ||
| return cloneResponsesInterfaceParts(parts), nil | ||
| } | ||
| prepended := make([]any, 0, len(parts)+1) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/admin/dashboard/templates/index.html (1)
391-414:⚠️ Potential issue | 🟠 MajorMove the checkbox group outside the outer field
<label>wrapper.The checkboxes branch nests
<label class="execution-plan-feature-toggle">inside the parent<label class="alias-form-field ...">, which violates the HTML5 content model — labels cannot contain descendant label elements. This breaks input-label associations and causes unpredictable behavior across browsers. Screen readers will also struggle with the incorrect nesting, violating WCAG 1.3.1 and 4.1.2. Wrap the checkbox group in a<div>or<fieldset>instead, positioned outside the outer label.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/admin/dashboard/templates/index.html` around lines 391 - 414, The checkbox group is currently nested inside the outer label element (class "alias-form-field alias-form-field-wide"), which creates invalid HTML because it contains descendant <label> elements (class "execution-plan-feature-toggle"); move the entire <template x-if="field.input === 'checkboxes'"> block out of the outer <label> wrapper and place it directly after that label (wrap it in a <div> or <fieldset> for semantics), keeping the existing structure and handlers (guardrailArrayFieldSelected, toggleGuardrailArrayFieldValue) and the key expression (field.key + '-' + option.value) intact so behavior and bindings are unchanged. Ensure the visual/layout styles still apply after relocation and that no other input elements remain inside the outer label.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/admin/handler_guardrails_test.go`:
- Around line 162-168: The test currently uses
strings.TrimSpace(fmt.Sprint(defaults["prompt"])) which falsely treats nil as
non-empty; instead, retrieve defaults from typeDef.Defaults, check that the
"prompt" key exists, type-assert it to string (e.g. val, ok :=
defaults["prompt"].(string)), then run strings.TrimSpace on that string and fail
if the assertion fails or the trimmed string is empty so missing, nil, or
non-string prompts are caught; update the assertion in
handler_guardrails_test.go accordingly around the defaults/typeDef.Defaults
usage.
In `@internal/guardrails/llm_based_altering.go`:
- Around line 482-490: The code currently treats a choice that contains a tool
call and empty text as success; update the success condition in the
llm_based_altering return logic to treat any completion with ToolCalls as a
failure. Specifically, in the block reading resp.Choices[0].Message.Content and
resp.Choices[0].Message.ToolCalls, change the guard to return an error whenever
content is empty OR len(resp.Choices[0].Message.ToolCalls) > 0 (i.e., do not
accept tool-call completions), so only non-empty plain-text choices succeed
before calling unwrapAlteredText.
- Around line 432-453: rewriteTexts currently spawns unlimited goroutines (one
per text) and accepts empty tool-call responses as valid rewrites; limit the
per-request fan-out by adding a concurrency limiter (e.g., a semaphore or
buffered channel) around the group.Go calls in rewriteTexts (use a configurable
constant like maxConcurrentRewrites) so at most N rewriteText calls run
concurrently, and modify rewriteText to treat tool-call responses that contain
no text as an error (return a non-nil error) so rewriteTexts' existing error
branch keeps the original text instead of overwriting it with an empty string.
In `@internal/guardrails/provider.go`:
- Around line 1043-1075: isResponsesStructuredContent currently only returns
true for non-empty []any and []core.ContentPart and
rewriteStructuredResponsesContentWithTextRewrite does not handle
[]map[string]any, causing empty or map-based structured arrays to be treated as
scalars; update isResponsesStructuredContent to treat any slice types as
structured (including empty slices) and explicitly detect []map[string]any as
structured, and extend rewriteStructuredResponsesContentWithTextRewrite to
handle the []map[string]any case by delegating to
rewriteStructuredResponsesInterfaceContentParts (or a new helper) so
programmatically-built or empty structured arrays preserve their array shape
instead of falling through to the scalar path.
- Around line 966-993: applyMessagesToResponsesInput currently coerces
non-string inputs into []core.ResponsesInputElement and returns a new slice,
losing any extra top-level fields on original array/map envelopes; instead,
after calling coerceResponsesInputElements(original) and
applyMessagesToResponsesElements, patch the original array-shaped input in place
when the original type is an array/slice or []map[string]any: detect the
original's shape, iterate the coerced elements and copy back the mutated fields
(e.g., role, content, content_null or their map keys) into the corresponding
original entries so you return the original mutated envelope rather than a fresh
[]core.ResponsesInputElement; use applyMessagesToResponsesElements,
coerceResponsesInputElements and applyMessagesToResponsesInput to locate and
implement the in-place update logic.
- Around line 942-955: The current applyMessagesToResponses function folds every
Message with Role == "system" from msgs into result.Instructions, which
incorrectly strips any genuine system items out of ResponsesRequest.Input and
loses their original positions; modify applyMessagesToResponses so that only the
synthetic system instruction derived from req.Instructions is appended to
result.Instructions, while preserving any existing system-role items in
result.Input (i.e., do not remove or concatenate messages that correspond to
entries already present in req.Input); locate logic around
applyMessagesToResponses, msgs, result.Instructions and ResponsesRequest.Input
and change the filter/append behavior so system-role messages from msgs are left
in nonSystem (or reinserted into result.Input) rather than merged into
Instructions.
In `@internal/server/internal_chat_completion_executor.go`:
- Around line 121-130: When e.chain(c) returns an error, decode the recorder
(rec) response body first (use decodeInternalExecutorError with rec.Code and
rec.Body.Bytes()) and return that Gateway-style error if decoding succeeds or
rec indicates an HTTP error; if the recorder has no valid error payload, wrap
the original err into a core.GatewayError (preserving message and any status)
before returning so no untyped framework errors escape; update the error path in
internal_chat_completion_executor.go around the e.chain(c) call to perform these
checks and returns.
---
Outside diff comments:
In `@internal/admin/dashboard/templates/index.html`:
- Around line 391-414: The checkbox group is currently nested inside the outer
label element (class "alias-form-field alias-form-field-wide"), which creates
invalid HTML because it contains descendant <label> elements (class
"execution-plan-feature-toggle"); move the entire <template x-if="field.input
=== 'checkboxes'"> block out of the outer <label> wrapper and place it directly
after that label (wrap it in a <div> or <fieldset> for semantics), keeping the
existing structure and handlers (guardrailArrayFieldSelected,
toggleGuardrailArrayFieldValue) and the key expression (field.key + '-' +
option.value) intact so behavior and bindings are unchanged. Ensure the
visual/layout styles still apply after relocation and that no other input
elements remain inside the outer label.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 11075305-e38c-4f32-bb74-22a3add4abc5
📒 Files selected for processing (28)
README.mdconfig/config.example.yamlconfig/config.godocs/advanced/guardrails.mdxgo.modinternal/admin/dashboard/static/js/modules/guardrails.jsinternal/admin/dashboard/static/js/modules/guardrails.test.jsinternal/admin/dashboard/static/js/modules/timezone-layout.test.jsinternal/admin/dashboard/templates/index.htmlinternal/admin/handler_guardrails_test.gointernal/app/app.gointernal/core/context.gointernal/guardrails/definitions.gointernal/guardrails/executor.gointernal/guardrails/factory.gointernal/guardrails/llm_based_altering.gointernal/guardrails/llm_based_altering_test.gointernal/guardrails/provider.gointernal/guardrails/provider_test.gointernal/guardrails/service.gointernal/guardrails/service_test.gointernal/server/execution_plan_helpers.gointernal/server/execution_policy.gointernal/server/execution_policy_test.gointernal/server/internal_chat_completion_executor.gointernal/server/internal_chat_completion_executor_test.gointernal/server/model_validation.gointernal/server/native_batch_service.go
| var defaults map[string]any | ||
| if err := json.Unmarshal(typeDef.Defaults, &defaults); err != nil { | ||
| t.Fatalf("json.Unmarshal(defaults) error = %v", err) | ||
| } | ||
| if got := strings.TrimSpace(fmt.Sprint(defaults["prompt"])); got == "" { | ||
| t.Fatalf("llm_based_altering defaults.prompt = %q, want built-in prompt", got) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Go, what does fmt.Sprint(nil) return, and would strings.TrimSpace(fmt.Sprint(m["missing"])) be empty for a missing map[string]any key?
💡 Result:
fmt.Sprint(nil) returns the string "nil". For a missing key in map[string]any, m["missing"] returns the zero value of any, which is nil. Thus fmt.Sprint(m["missing"]) returns "nil". strings.TrimSpace("nil") returns "nil", which is not empty. To verify, consider Go source code where nilString = "nil" is used for printing nil values (result 1,15). Map access for missing keys returns zero value nil for interface{} (result 9). Multiple issues confirm fmt prints nil interface as "" or "nil" depending on context, but Sprint uses basic nilString "nil" (results 5,6,17).
Citations:
- 1: https://go.dev/src/fmt/print.go?s=7053:7089
- 2: https://bitfieldconsulting.com/posts/map-key-exists
- 3: https://go.dev/src/fmt/print.go?s=1012%3A1452
- 4: https://go101.org/article/nil.html
- 5: https://pkg.go.dev/fmt@go1.26rc1
- 6: https://pkg.go.dev/fmt
Strengthen the defaults.prompt assertion to catch missing or nil values.
The current check strings.TrimSpace(fmt.Sprint(defaults["prompt"])) == "" is too permissive. When prompt is missing or nil, fmt.Sprint(nil) returns "nil" (not empty), so the test passes even though the prompt is absent. Type-assert the field to string and validate the trimmed result instead.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/admin/handler_guardrails_test.go` around lines 162 - 168, The test
currently uses strings.TrimSpace(fmt.Sprint(defaults["prompt"])) which falsely
treats nil as non-empty; instead, retrieve defaults from typeDef.Defaults, check
that the "prompt" key exists, type-assert it to string (e.g. val, ok :=
defaults["prompt"].(string)), then run strings.TrimSpace on that string and fail
if the assertion fails or the trimmed string is empty so missing, nil, or
non-string prompts are caught; update the assertion in
handler_guardrails_test.go accordingly around the defaults/typeDef.Defaults
usage.
| func (g *LLMBasedAlteringGuardrail) rewriteTexts(ctx context.Context, texts []string) ([]string, error) { | ||
| rewritten := make([]string, len(texts)) | ||
| group, groupCtx := errgroup.WithContext(ctx) | ||
|
|
||
| for i, text := range texts { | ||
| i := i | ||
| text := text | ||
| group.Go(func() error { | ||
| result, err := g.rewriteText(groupCtx, text) | ||
| if err != nil { | ||
| if errors.Is(err, context.Canceled) || errors.Is(err, context.DeadlineExceeded) { | ||
| return err | ||
| } | ||
| rewritten[i] = text | ||
| return nil | ||
| } | ||
| rewritten[i] = result | ||
| return nil | ||
| }) | ||
| } | ||
|
|
||
| if err := group.Wait(); err != nil { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find the file and check its size
fd -t f "llm_based_altering.go" && wc -l internal/guardrails/llm_based_altering.goRepository: ENTERPILOT/GoModel
Length of output: 149
🏁 Script executed:
# Read the relevant section around lines 432-453 and surrounding context
sed -n '420,470p' internal/guardrails/llm_based_altering.goRepository: ENTERPILOT/GoModel
Length of output: 1405
🏁 Script executed:
# Also check the rewriteText method to understand the tool-call validation issue
sed -n '1,100p' internal/guardrails/llm_based_altering.goRepository: ENTERPILOT/GoModel
Length of output: 4663
🏁 Script executed:
# Search for core.GatewayError usage in the file
rg "core\.GatewayError|GatewayError" internal/guardrails/llm_based_altering.go -A 2 -B 2Repository: ENTERPILOT/GoModel
Length of output: 44
🏁 Script executed:
# Check for error handling patterns in the full file
rg "return err|errors\.Is|error\)" internal/guardrails/llm_based_altering.go | head -20Repository: ENTERPILOT/GoModel
Length of output: 1039
🏁 Script executed:
# Read the rewriteText function implementation in full
sed -n '460,520p' internal/guardrails/llm_based_altering.goRepository: ENTERPILOT/GoModel
Length of output: 1753
🏁 Script executed:
# Check the Process method which calls rewriteTexts
sed -n '400,432p' internal/guardrails/llm_based_altering.goRepository: ENTERPILOT/GoModel
Length of output: 806
🏁 Script executed:
# Search for Process method and how errors are handled
rg "func \(g \*LLMBasedAlteringGuardrail\) Process" -A 30 internal/guardrails/llm_based_altering.goRepository: ENTERPILOT/GoModel
Length of output: 753
🏁 Script executed:
# Check how core.GatewayError is used elsewhere in the codebase
find . -type f -name "*.go" -path "*/internal/guardrails/*" -exec grep -l "core\.GatewayError" {} \;Repository: ENTERPILOT/GoModel
Length of output: 132
🏁 Script executed:
# Check what core package exports
rg "type GatewayError" internal/core/ -A 5Repository: ENTERPILOT/GoModel
Length of output: 437
🏁 Script executed:
# Check the service layer to see if errors are wrapped at a higher level
rg "core\.NewGatewayError|core\.GatewayError" internal/guardrails/service.go -B 2 -A 2Repository: ENTERPILOT/GoModel
Length of output: 167
🏁 Script executed:
# Check how the guardrail is used in the service
rg "Process\(" internal/guardrails/service.go -B 2 -A 5Repository: ENTERPILOT/GoModel
Length of output: 44
🏁 Script executed:
# Look at full error handling in service
sed -n '1,100p' internal/guardrails/service.goRepository: ENTERPILOT/GoModel
Length of output: 2368
🏁 Script executed:
# Find where guardrails are applied in the request flow
rg "\.Process\(" internal/ -l | head -5Repository: ENTERPILOT/GoModel
Length of output: 255
🏁 Script executed:
# Check how errors from guardrails are handled
rg "Process\(" internal/ -B 2 -A 5 | head -40Repository: ENTERPILOT/GoModel
Length of output: 2176
🏁 Script executed:
# Look at core.GatewayError construction to understand how errors should be wrapped
cat internal/core/errors.go | head -100Repository: ENTERPILOT/GoModel
Length of output: 2721
Bound the per-request rewrite fan-out and handle tool-call responses correctly.
Lines 436-450 spawn one auxiliary model call per target message without limit, amplifying latency, spend, and rate-limit pressure on requests with many rewriteable messages. Additionally, line 488 accepts LLM tool-call responses containing no text content as successful rewrites, causing rewriteTexts to blank out original message content instead of failing safely.
💡 Suggested fix
func (g *LLMBasedAlteringGuardrail) rewriteTexts(ctx context.Context, texts []string) ([]string, error) {
rewritten := make([]string, len(texts))
group, groupCtx := errgroup.WithContext(ctx)
+ group.SetLimit(4)Additionally, in rewriteText, reject tool-call responses:
content := core.ExtractTextContent(resp.Choices[0].Message.Content)
- if content == "" && len(resp.Choices[0].Message.ToolCalls) == 0 {
+ if content == "" || len(resp.Choices[0].Message.ToolCalls) > 0 {
return "", fmt.Errorf("llm_based_altering returned empty content")
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (g *LLMBasedAlteringGuardrail) rewriteTexts(ctx context.Context, texts []string) ([]string, error) { | |
| rewritten := make([]string, len(texts)) | |
| group, groupCtx := errgroup.WithContext(ctx) | |
| for i, text := range texts { | |
| i := i | |
| text := text | |
| group.Go(func() error { | |
| result, err := g.rewriteText(groupCtx, text) | |
| if err != nil { | |
| if errors.Is(err, context.Canceled) || errors.Is(err, context.DeadlineExceeded) { | |
| return err | |
| } | |
| rewritten[i] = text | |
| return nil | |
| } | |
| rewritten[i] = result | |
| return nil | |
| }) | |
| } | |
| if err := group.Wait(); err != nil { | |
| func (g *LLMBasedAlteringGuardrail) rewriteTexts(ctx context.Context, texts []string) ([]string, error) { | |
| rewritten := make([]string, len(texts)) | |
| group, groupCtx := errgroup.WithContext(ctx) | |
| group.SetLimit(4) | |
| for i, text := range texts { | |
| i := i | |
| text := text | |
| group.Go(func() error { | |
| result, err := g.rewriteText(groupCtx, text) | |
| if err != nil { | |
| if errors.Is(err, context.Canceled) || errors.Is(err, context.DeadlineExceeded) { | |
| return err | |
| } | |
| rewritten[i] = text | |
| return nil | |
| } | |
| rewritten[i] = result | |
| return nil | |
| }) | |
| } | |
| if err := group.Wait(); err != nil { |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/guardrails/llm_based_altering.go` around lines 432 - 453,
rewriteTexts currently spawns unlimited goroutines (one per text) and accepts
empty tool-call responses as valid rewrites; limit the per-request fan-out by
adding a concurrency limiter (e.g., a semaphore or buffered channel) around the
group.Go calls in rewriteTexts (use a configurable constant like
maxConcurrentRewrites) so at most N rewriteText calls run concurrently, and
modify rewriteText to treat tool-call responses that contain no text as an error
(return a non-nil error) so rewriteTexts' existing error branch keeps the
original text instead of overwriting it with an empty string.
| if resp == nil || len(resp.Choices) == 0 { | ||
| return "", fmt.Errorf("llm_based_altering returned no choices") | ||
| } | ||
|
|
||
| content := core.ExtractTextContent(resp.Choices[0].Message.Content) | ||
| if content == "" && len(resp.Choices[0].Message.ToolCalls) == 0 { | ||
| return "", fmt.Errorf("llm_based_altering returned empty content") | ||
| } | ||
| return unwrapAlteredText(content), nil |
There was a problem hiding this comment.
Treat tool-call completions as rewrite failures.
This guardrail expects plain text. Lines 487-490 currently accept tool_calls with empty text as success, so a provider that returns a tool invocation here will clear the original message instead of failing open.
💡 Suggested fix
content := core.ExtractTextContent(resp.Choices[0].Message.Content)
- if content == "" && len(resp.Choices[0].Message.ToolCalls) == 0 {
+ if len(resp.Choices[0].Message.ToolCalls) != 0 {
+ return "", fmt.Errorf("llm_based_altering must return plain text, not tool calls")
+ }
+ if content == "" {
return "", fmt.Errorf("llm_based_altering returned empty content")
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if resp == nil || len(resp.Choices) == 0 { | |
| return "", fmt.Errorf("llm_based_altering returned no choices") | |
| } | |
| content := core.ExtractTextContent(resp.Choices[0].Message.Content) | |
| if content == "" && len(resp.Choices[0].Message.ToolCalls) == 0 { | |
| return "", fmt.Errorf("llm_based_altering returned empty content") | |
| } | |
| return unwrapAlteredText(content), nil | |
| if resp == nil || len(resp.Choices) == 0 { | |
| return "", fmt.Errorf("llm_based_altering returned no choices") | |
| } | |
| content := core.ExtractTextContent(resp.Choices[0].Message.Content) | |
| if len(resp.Choices[0].Message.ToolCalls) != 0 { | |
| return "", fmt.Errorf("llm_based_altering must return plain text, not tool calls") | |
| } | |
| if content == "" { | |
| return "", fmt.Errorf("llm_based_altering returned empty content") | |
| } | |
| return unwrapAlteredText(content), nil |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/guardrails/llm_based_altering.go` around lines 482 - 490, The code
currently treats a choice that contains a tool call and empty text as success;
update the success condition in the llm_based_altering return logic to treat any
completion with ToolCalls as a failure. Specifically, in the block reading
resp.Choices[0].Message.Content and resp.Choices[0].Message.ToolCalls, change
the guard to return an error whenever content is empty OR
len(resp.Choices[0].Message.ToolCalls) > 0 (i.e., do not accept tool-call
completions), so only non-empty plain-text choices succeed before calling
unwrapAlteredText.
| func applyMessagesToResponses(req *core.ResponsesRequest, msgs []Message) (*core.ResponsesRequest, error) { | ||
| result := *req | ||
| var instructions string | ||
| nonSystem := make([]Message, 0, len(msgs)) | ||
| for _, m := range msgs { | ||
| if m.Role == "system" { | ||
| if instructions != "" { | ||
| instructions += "\n" | ||
| } | ||
| instructions += m.Content | ||
| continue | ||
| } | ||
| nonSystem = append(nonSystem, m) | ||
| } |
There was a problem hiding this comment.
Keep ResponsesRequest.Input system items out of Instructions.
Lines 947-952 fold every Role == "system" message into result.Instructions. If the original Input array legitimately contains a system-role item, it gets removed from Input, concatenated into Instructions, and loses its original position. Only the synthetic message created from req.Instructions should round-trip through Instructions.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/guardrails/provider.go` around lines 942 - 955, The current
applyMessagesToResponses function folds every Message with Role == "system" from
msgs into result.Instructions, which incorrectly strips any genuine system items
out of ResponsesRequest.Input and loses their original positions; modify
applyMessagesToResponses so that only the synthetic system instruction derived
from req.Instructions is appended to result.Instructions, while preserving any
existing system-role items in result.Input (i.e., do not remove or concatenate
messages that correspond to entries already present in req.Input); locate logic
around applyMessagesToResponses, msgs, result.Instructions and
ResponsesRequest.Input and change the filter/append behavior so system-role
messages from msgs are left in nonSystem (or reinserted into result.Input)
rather than merged into Instructions.
| func applyMessagesToResponsesInput(original any, msgs []Message) (any, error) { | ||
| switch typed := original.(type) { | ||
| case nil: | ||
| if len(msgs) != 0 { | ||
| return nil, core.NewInvalidRequestError("guardrails cannot add or remove responses input items", nil) | ||
| } | ||
| return nil, nil | ||
| case string: | ||
| if len(msgs) != 1 { | ||
| return nil, core.NewInvalidRequestError("guardrails cannot add or remove responses input items", nil) | ||
| } | ||
| if msgs[0].Role != "user" { | ||
| return nil, core.NewInvalidRequestError("guardrails cannot change the role of a string responses input", nil) | ||
| } | ||
| if msgs[0].ContentNull { | ||
| return "", nil | ||
| } | ||
| return msgs[0].Content, nil | ||
| default: | ||
| _ = typed | ||
| } | ||
|
|
||
| elements, err := coerceResponsesInputElements(original) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| return applyMessagesToResponsesElements(elements, msgs) | ||
| } |
There was a problem hiding this comment.
Patch array-shaped Input values in place instead of rebuilding them.
For non-string inputs, Lines 988-993 always return []core.ResponsesInputElement. That is no longer a patch of the original envelope for requests that arrived as []any or []map[string]any, and any top-level fields not represented on core.ResponsesInputElement cannot survive that rewrite.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/guardrails/provider.go` around lines 966 - 993,
applyMessagesToResponsesInput currently coerces non-string inputs into
[]core.ResponsesInputElement and returns a new slice, losing any extra top-level
fields on original array/map envelopes; instead, after calling
coerceResponsesInputElements(original) and applyMessagesToResponsesElements,
patch the original array-shaped input in place when the original type is an
array/slice or []map[string]any: detect the original's shape, iterate the
coerced elements and copy back the mutated fields (e.g., role, content,
content_null or their map keys) into the corresponding original entries so you
return the original mutated envelope rather than a fresh
[]core.ResponsesInputElement; use applyMessagesToResponsesElements,
coerceResponsesInputElements and applyMessagesToResponsesInput to locate and
implement the in-place update logic.
| func applyGuardedResponsesContentToOriginal(originalContent any, rewrittenText string, contentNull bool) (any, error) { | ||
| if isResponsesStructuredContent(originalContent) { | ||
| return rewriteStructuredResponsesContentWithTextRewrite(originalContent, rewrittenText) | ||
| } | ||
| if rewrittenText != "" { | ||
| contentNull = false | ||
| } | ||
| if contentNull { | ||
| return nil, nil | ||
| } | ||
| return rewrittenText, nil | ||
| } | ||
|
|
||
| func isResponsesStructuredContent(content any) bool { | ||
| switch typed := content.(type) { | ||
| case []any: | ||
| return len(typed) > 0 | ||
| case []core.ContentPart: | ||
| return len(typed) > 0 | ||
| default: | ||
| return false | ||
| } | ||
| } | ||
|
|
||
| func rewriteStructuredResponsesContentWithTextRewrite(originalContent any, rewrittenText string) (any, error) { | ||
| switch typed := originalContent.(type) { | ||
| case []core.ContentPart: | ||
| return rewriteStructuredResponsesTypedContentParts(typed, rewrittenText) | ||
| case []any: | ||
| return rewriteStructuredResponsesInterfaceContentParts(typed, rewrittenText) | ||
| default: | ||
| return nil, core.NewInvalidRequestError("unsupported structured responses content", nil) | ||
| } |
There was a problem hiding this comment.
Treat empty and []map[string]any content arrays as structured content.
isResponsesStructuredContent only recognizes non-empty []any and []core.ContentPart, and the dispatcher below still ignores []map[string]any. Programmatically-built or empty structured content therefore falls through to the scalar path and can round-trip as ""/nil instead of preserving the original array shape.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/guardrails/provider.go` around lines 1043 - 1075,
isResponsesStructuredContent currently only returns true for non-empty []any and
[]core.ContentPart and rewriteStructuredResponsesContentWithTextRewrite does not
handle []map[string]any, causing empty or map-based structured arrays to be
treated as scalars; update isResponsesStructuredContent to treat any slice types
as structured (including empty slices) and explicitly detect []map[string]any as
structured, and extend rewriteStructuredResponsesContentWithTextRewrite to
handle the []map[string]any case by delegating to
rewriteStructuredResponsesInterfaceContentParts (or a new helper) so
programmatically-built or empty structured arrays preserve their array shape
instead of falling through to the scalar path.
| rec := httptest.NewRecorder() | ||
| c := e.echo.NewContext(httpReq, rec) | ||
| c.SetPath("/v1/chat/completions") | ||
|
|
||
| if err := e.chain(c); err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| if rec.Code >= http.StatusBadRequest { | ||
| return nil, decodeInternalExecutorError(rec.Code, rec.Body.Bytes()) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate and read the file to see the actual implementation
find . -name "internal_chat_completion_executor.go" -type fRepository: ENTERPILOT/GoModel
Length of output: 116
🏁 Script executed:
# Check file size and read relevant sections
wc -l internal/server/internal_chat_completion_executor.goRepository: ENTERPILOT/GoModel
Length of output: 118
🏁 Script executed:
# Read the specific section mentioned in the review (lines 110-140)
sed -n '110,140p' internal/server/internal_chat_completion_executor.go | cat -nRepository: ENTERPILOT/GoModel
Length of output: 1359
🏁 Script executed:
# Find the decodeInternalExecutorError function definition
rg -A 15 "func.*decodeInternalExecutorError" internal/server/Repository: ENTERPILOT/GoModel
Length of output: 1510
🏁 Script executed:
# Check imports to verify core.GatewayError availability
head -30 internal/server/internal_chat_completion_executor.go | cat -nRepository: ENTERPILOT/GoModel
Length of output: 1148
🏁 Script executed:
# Verify if core.GatewayError is imported and check package structure
rg "core\.GatewayError|import.*core" internal/server/internal_chat_completion_executor.goRepository: ENTERPILOT/GoModel
Length of output: 90
🏁 Script executed:
# Check if there's a middleware or handler that might translate errors
rg -B 5 -A 10 "chain.*func|errorHandler|HTTPErrorHandler" internal/server/Repository: ENTERPILOT/GoModel
Length of output: 44
🏁 Script executed:
# Get the full context of the ChatCompletion method signature and usage
sed -n '85,130p' internal/server/internal_chat_completion_executor.go | cat -nRepository: ENTERPILOT/GoModel
Length of output: 2110
🏁 Script executed:
# Check how this executor is used - search for ChatCompletion calls
rg -B 3 -A 3 "\.ChatCompletion\(" internal/Repository: ENTERPILOT/GoModel
Length of output: 45019
🏁 Script executed:
# Verify the return type of ChatCompletion method
sed -n '80,95p' internal/server/internal_chat_completion_executor.go | cat -nRepository: ENTERPILOT/GoModel
Length of output: 753
🏁 Script executed:
# Check if there's any error translation happening in calling code
rg -B 5 "InternalChatCompletionExecutor" internal/server/ | head -40Repository: ENTERPILOT/GoModel
Length of output: 3282
🏁 Script executed:
# Check where ChatCompletion is called from - is it client-facing or internal?
rg -B 5 "executor\.ChatCompletion" internal/server/ internal/guardrails/Repository: ENTERPILOT/GoModel
Length of output: 1633
🏁 Script executed:
# Verify the echo chain initialization and what could go wrong
sed -n '30,80p' internal/server/internal_chat_completion_executor.go | cat -nRepository: ENTERPILOT/GoModel
Length of output: 2227
🏁 Script executed:
# Check if there's any Echo error handler that might write to body
rg -A 10 "echo.*ErrorHandler|HTTPErrorHandler" internal/server/Repository: ENTERPILOT/GoModel
Length of output: 44
🏁 Script executed:
# Trace how errors from executor.ChatCompletion flow through guardrails to clients
sed -n '1,50p' internal/guardrails/llm_based_altering.go | cat -nRepository: ENTERPILOT/GoModel
Length of output: 2839
🏁 Script executed:
# Check the processGuardedChat function to see error handling
rg -A 20 "func processGuardedChat" internal/guardrails/Repository: ENTERPILOT/GoModel
Length of output: 1418
🏁 Script executed:
# Check GuardedProvider.ChatCompletion to see if errors are translated
sed -n '25,50p' internal/guardrails/provider.go | cat -nRepository: ENTERPILOT/GoModel
Length of output: 1267
🏁 Script executed:
# Verify if GuardedProvider ultimately serves client responses or is internal
rg -B 5 "GuardedProvider" internal/server/ | head -30Repository: ENTERPILOT/GoModel
Length of output: 44
🏁 Script executed:
# Check if errors from GuardedProvider.ChatCompletion flow to client responses
rg -B 3 -A 8 "guardedProvider|GuardedProvider" internal/server/handlers.goRepository: ENTERPILOT/GoModel
Length of output: 44
🏁 Script executed:
# See the full decodeInternalExecutorError to understand error handling completely
sed -n '140,175p' internal/server/internal_chat_completion_executor.go | cat -nRepository: ENTERPILOT/GoModel
Length of output: 1365
🏁 Script executed:
# Final verification: check if chain errors occur in practice with tests
rg -B 5 -A 10 "TestInternalChatCompletionExecutor" internal/server/internal_chat_completion_executor_test.go | head -60Repository: ENTERPILOT/GoModel
Length of output: 1242
Chain errors must be translated to core.GatewayError before returning.
When e.chain(c) returns an error at line 125, it bypasses the error response body decoding that only runs on successful chain execution. This allows middleware or handler errors to escape as untyped framework errors instead of core.GatewayError, violating the client error contract.
Even if the chain writes an error response to the recorder body, that body is never decoded and translated when an error is returned. The fix must handle both scenarios: decode the recorder body when available (even after chain errors), and wrap any untyped errors in core.GatewayError.
💡 Suggested handling
import (
"bytes"
"context"
"encoding/json"
+ "errors"
"net/http"
"net/http/httptest"
"strings"
@@
- if err := e.chain(c); err != nil {
- return nil, err
- }
+ if err := e.chain(c); err != nil {
+ if rec.Code >= http.StatusBadRequest && rec.Body.Len() > 0 {
+ return nil, decodeInternalExecutorError(rec.Code, rec.Body.Bytes())
+ }
+ var gwErr *core.GatewayError
+ if errors.As(err, &gwErr) {
+ return nil, gwErr
+ }
+ return nil, core.NewProviderError("", http.StatusBadGateway, "internal chat execution failed", err)
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/server/internal_chat_completion_executor.go` around lines 121 - 130,
When e.chain(c) returns an error, decode the recorder (rec) response body first
(use decodeInternalExecutorError with rec.Code and rec.Body.Bytes()) and return
that Gateway-style error if decoding succeeds or rec indicates an HTTP error; if
the recorder has no valid error payload, wrap the original err into a
core.GatewayError (preserving message and any status) before returning so no
untyped framework errors escape; update the error path in
internal_chat_completion_executor.go around the e.chain(c) call to perform these
checks and returns.
2 participants
Summary
llm_based_alteringauxiliary calls through the translated execution stack instead of direct provider callsuser_path-based workflow routing while explicitly disabling recursive guardrailsTesting
Summary by CodeRabbit
Release Notes
New Features
Documentation
Tests