Feature: guardrails - system prompt decorator + Data Anonymization / PII Detection (without AI models)#56
Closed
SantiagoDePolonia wants to merge 2 commits into
Closed
Feature: guardrails - system prompt decorator + Data Anonymization / PII Detection (without AI models)#56SantiagoDePolonia wants to merge 2 commits into
SantiagoDePolonia wants to merge 2 commits into
Conversation
Implement request preprocessing guardrails that operate between request parsing and provider routing: System Prompt Injection: - Configurable prompts at global, provider, and model levels - Precedence: model-specific > provider-specific > global - Position options: prepend, append, or replace - Option to preserve user's existing system message PII Anonymization: - Regex-based detection for email, phone, SSN, credit card, IP - Model whitelist support (anonymize only specific models) - Strategies: token ([EMAIL_1]), hash, or mask - De-anonymization for both streaming and non-streaming responses New package: internal/guardrails/ - config.go: Configuration types - guardrails.go: Main Processor interface - system_prompt.go: System prompt injection logic - anonymizer.go: PII detection and anonymization - anonymizer_patterns.go: Regex patterns for PII types - anonymizer_stream.go: Streaming de-anonymization reader Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Update .gitignore to use **/.cache/ pattern instead of /.cache to catch cache directories in subdirectories like tests/integration/.cache/. The cache file was accidentally committed and contains auto-generated timestamps that change on every test run. It does not speed up CI/CD since integration tests use mock providers with synchronous responses. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Contributor
📝 WalkthroughWalkthroughThis PR introduces a comprehensive guardrails system for request/response processing. It adds configuration structures, a Processor for orchestrating features, SystemPromptInjector for dynamic prompt injection, Anonymizer for PII detection with multiple detector types, and streaming support for deanonymization. All components integrate into the application, request handlers, and HTTP server initialization. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Handler
participant Processor as Guardrails<br/>Processor
participant Injector as SystemPrompt<br/>Injector
participant Anonymizer
participant Provider
participant Deanonymizer
Client->>Handler: ChatRequest
Handler->>Processor: ProcessChatRequest(request)
Processor->>Injector: ProcessChatRequest()
Injector-->>Processor: Modified request
Processor->>Anonymizer: AnonymizeChatRequest()
Anonymizer-->>Processor: Modified request<br/>+ tokenMap
Processor-->>Handler: Modified request<br/>+ RequestContext
Handler->>Provider: Forward request
Provider-->>Handler: ChatResponse
Handler->>Deanonymizer: DeanonymizeChatResponse<br/>(response, tokenMap)
Deanonymizer-->>Handler: Original response
Handler-->>Client: ChatResponse
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutes Possibly related PRs
Suggested labels
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
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 11
🤖 Fix all issues with AI agents
In `@config/config.go`:
- Around line 378-396: The Guardrails default block currently hardcodes disabled
settings and isn't included in environment expansion; update the configuration
loading to support env overrides by wiring
GuardrailsConfig/SystemPromptConfig/AnonymizationConfig into the env-expansion
logic (the same path used by logging/usage/metrics/storage) and read specific
variables such as GUARDRAILS_SYSTEM_PROMPT_ENABLED and
GUARDRAILS_ANONYMIZATION_ENABLED (and optionally
GUARDRAILS_ANONYMIZATION_STRATEGY or GUARDRAILS_DEANONYMIZE_RESPONSES) to
override the defaults; ensure expandEnvVars (or the function that applies env
vars) is updated to include guardrails keys so values set in the environment
replace the YAML defaults.
In `@internal/guardrails/anonymizer_patterns.go`:
- Around line 38-43: The SSN entry (Type: PIITypeSSN, Pattern:
regexp.MustCompile(`\b\d{3}[\s\-]?\d{2}[\s\-]?\d{4}\b`)) can match phone-like
sequences; either (A) ensure phone detection runs before SSN (apply the
PIITypePhone / phone pattern prior to PIITypeSSN) so longer/more specific phone
matches are consumed first, or (B) tighten the SSN regex to avoid phone contexts
(e.g., add a negative lookbehind/anchoring so the match is not directly preceded
by a digit, hyphen or extra group typical of phone numbers). Update the ordering
or the Pattern for PIITypeSSN accordingly so SSNs do not match as suffixes of
phone numbers.
- Around line 44-49: The credit-card regex currently tied to the
PIITypeCreditCard entry only matches 16-digit cards; update the Pattern in that
anonymizer_patterns.go entry (the struct with Type: PIITypeCreditCard and
Pattern:) to accept common card lengths (13, 15, and 16 digits) while still
allowing optional separators (spaces or dashes) between logical groups and
keeping word boundaries; implement this by replacing the current regexp with one
that uses a non-capturing alternation for the valid formats (e.g., patterns
covering 13-digit, 15-digit (AmEx), and 16-digit formats with optional [\s-]
separators) so older Visa and AmEx numbers are detected without matching longer
digit sequences.
In `@internal/guardrails/anonymizer_stream.go`:
- Around line 51-53: The Read method currently allocates a new 4096-byte slice
on every call (buf := make([]byte, 4096)) which increases GC pressure; add a
reusable buffer field to the anonymizer stream struct (e.g., a []byte named
readBuf on the same struct that holds reader) and use that slice for reads (pass
a subslice or ensure capacity) instead of allocating each time, initializing it
once (lazy or in constructor) and reuse it for subsequent calls to
r.reader.Read; ensure concurrent safety if the struct is used concurrently.
In `@internal/guardrails/anonymizer_test.go`:
- Around line 355-384: In TestAnonymizer_SameValueSameToken rename the loop
variable that iterates over tokenMap (currently `for t := range tokenMap`) to
avoid shadowing the test parameter `t *testing.T` — e.g., use `for key := range
tokenMap` or `for tokenKey := range tokenMap` — then use that variable when
assigning `token`, keeping the rest of the test logic unchanged.
In `@internal/guardrails/anonymizer.go`:
- Around line 233-242: Replace the mutex-protected increment of a.tokenCounter
with an atomic increment: ensure tokenCounter is a uint64, import sync/atomic,
and use id := atomic.AddUint64(&a.tokenCounter, 1) instead of locking
a.tokenMu/a.tokenMu.Unlock(); then use the id in the fmt.Sprintf call (inside
the StrategyToken/default case) and remove the now-unused tokenMu locking around
this increment.
- Around line 195-214: The current duplicate-detection loop over tokenMap in the
anonymizer (inside the anonymization routine that uses tokenMap and calls
a.generateToken) is O(n) per match; replace it by maintaining a reverse map
originalToToken map[string]string for O(1) lookups: on entry to the
anonymization logic initialize originalToToken (and populate it wherever
tokenMap is populated), change the lookup to check originalToToken[match]
instead of iterating tokenMap, and when creating a new token (via
a.generateToken) write both tokenMap[token] = match and originalToToken[match] =
token; also update callers/constructors (AnonymizeChatRequest and
AnonymizeResponsesRequest) to create and pass the reverse map alongside tokenMap
so the anonymization function can use it.
In `@internal/guardrails/guardrails_test.go`:
- Around line 234-278: In TestProcessor_WrapStreamForDeanonymization add
explicit closes for any io.ReadCloser used: call Close (via defer) on the
original streams (stream, stream2) and on any wrapped readers returned by
p.WrapStreamForDeanonymization (wrapped, notWrapped) so tests demonstrate the
expected usage pattern and ensure streaming responses are closed; update the
test to defer wrapped.Close() after getting wrapped and defer notWrapped.Close()
(or close only the returned readers) to follow the guideline that callers close
io.ReadCloser results.
In `@internal/guardrails/system_prompt_test.go`:
- Around line 279-373: Add a test case in
TestSystemPromptInjector_ProcessResponsesRequest that sets wantUnchanged: true
to cover the "no rules" path; e.g., create a SystemPromptConfig with Enabled:
false (or Global: nil), a non-empty core.ResponsesRequest, and providerType
"openai", then assert the returned pointer equals the original request. Update
the tests slice in TestSystemPromptInjector_ProcessResponsesRequest to include
this case so NewSystemPromptInjector and ProcessResponsesRequest are exercised
when no injection rules apply.
In `@internal/guardrails/system_prompt.go`:
- Around line 49-87: ProcessResponsesRequest makes a shallow copy of
ResponsesRequest into newReq which leaves the mutable Input slice shared with
the original; either deep-copy the Input slice/elements into newReq.Input before
modifying Instructions or add a clear comment/documentation near
ProcessResponsesRequest and the newReq creation that the function intentionally
only mutates Instructions and must not touch Input (and that future changes must
deep-copy Input if they will modify it). Locate the newReq assignment in
ProcessResponsesRequest and implement the deep-copy of req.Input (preserving
element types like []interface{} or []ResponsesInputItem) or add the invariant
comment adjacent to the newReq creation and function docstring.
- Around line 108-203: The switch handling
PositionPrepend/PositionAppend/PositionReplace duplicates logic for replacing or
adding the system message (when rule.PreserveUserSystem is false and when no
existing system message exists); refactor by extracting a helper (e.g.,
buildMessagesWithSystem or applySystemReplacement) that accepts messages,
systemIdx, and the desired systemContent and returns newMessages, then call that
helper from the cases instead of repeating the for-loops and append logic;
ensure the helper handles both the "replace existing" (iterating messages and
replacing at systemIdx) and "add at beginning" (prepend new
core.Message{Role:"system", Content: ...} + messages) behaviors and update
PositionPrepend/PositionAppend to call it for the non-preserve branches while
keeping the PreserveUserSystem branches for combine/append behavior.
Comment on lines
+38
to
+43
| // SSN: US Social Security Number | ||
| // Matches: 123-45-6789, 123 45 6789, 123456789 | ||
| { | ||
| Type: PIITypeSSN, | ||
| Pattern: regexp.MustCompile(`\b\d{3}[\s\-]?\d{2}[\s\-]?\d{4}\b`), | ||
| }, |
Contributor
There was a problem hiding this comment.
SSN pattern may produce false positives with phone numbers.
The SSN regex \d{3}[\s\-]?\d{2}[\s\-]?\d{4} can match the last 9 digits of US phone numbers (e.g., "456-78-9012" within "123-456-78-9012"). Since both detectors can be enabled simultaneously, this could lead to duplicate or incorrect anonymization.
Consider ordering pattern application to prioritize longer/more-specific matches (phone before SSN), or refining the SSN pattern to exclude phone-like contexts.
🤖 Prompt for AI Agents
In `@internal/guardrails/anonymizer_patterns.go` around lines 38 - 43, The SSN
entry (Type: PIITypeSSN, Pattern:
regexp.MustCompile(`\b\d{3}[\s\-]?\d{2}[\s\-]?\d{4}\b`)) can match phone-like
sequences; either (A) ensure phone detection runs before SSN (apply the
PIITypePhone / phone pattern prior to PIITypeSSN) so longer/more specific phone
matches are consumed first, or (B) tighten the SSN regex to avoid phone contexts
(e.g., add a negative lookbehind/anchoring so the match is not directly preceded
by a digit, hyphen or extra group typical of phone numbers). Update the ordering
or the Pattern for PIITypeSSN accordingly so SSNs do not match as suffixes of
phone numbers.
Comment on lines
+44
to
+49
| // Credit Card: 16-digit card numbers with optional separators | ||
| // Matches: 1234567890123456, 1234-5678-9012-3456, 1234 5678 9012 3456 | ||
| { | ||
| Type: PIITypeCreditCard, | ||
| Pattern: regexp.MustCompile(`\b\d{4}[\s\-]?\d{4}[\s\-]?\d{4}[\s\-]?\d{4}\b`), | ||
| }, |
Contributor
There was a problem hiding this comment.
Credit card pattern only matches 16-digit cards.
American Express cards have 15 digits, and some older Visa cards have 13 digits. The current pattern will miss these card types.
♻️ Suggested pattern to support variable card lengths
- // Credit Card: 16-digit card numbers with optional separators
- // Matches: 1234567890123456, 1234-5678-9012-3456, 1234 5678 9012 3456
+ // Credit Card: 13-19 digit card numbers with optional separators
+ // Matches: 1234567890123456, 1234-5678-9012-3456, 4111-1111-1111-111, etc.
{
Type: PIITypeCreditCard,
- Pattern: regexp.MustCompile(`\b\d{4}[\s\-]?\d{4}[\s\-]?\d{4}[\s\-]?\d{4}\b`),
+ Pattern: regexp.MustCompile(`\b(?:\d{4}[\s\-]?){2,3}\d{4,7}\b`),
},📝 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.
Suggested change
| // Credit Card: 16-digit card numbers with optional separators | |
| // Matches: 1234567890123456, 1234-5678-9012-3456, 1234 5678 9012 3456 | |
| { | |
| Type: PIITypeCreditCard, | |
| Pattern: regexp.MustCompile(`\b\d{4}[\s\-]?\d{4}[\s\-]?\d{4}[\s\-]?\d{4}\b`), | |
| }, | |
| // Credit Card: 13-19 digit card numbers with optional separators | |
| // Matches: 1234567890123456, 1234-5678-9012-3456, 4111-1111-1111-111, etc. | |
| { | |
| Type: PIITypeCreditCard, | |
| Pattern: regexp.MustCompile(`\b(?:\d{4}[\s\-]?){2,3}\d{4,7}\b`), | |
| }, |
🤖 Prompt for AI Agents
In `@internal/guardrails/anonymizer_patterns.go` around lines 44 - 49, The
credit-card regex currently tied to the PIITypeCreditCard entry only matches
16-digit cards; update the Pattern in that anonymizer_patterns.go entry (the
struct with Type: PIITypeCreditCard and Pattern:) to accept common card lengths
(13, 15, and 16 digits) while still allowing optional separators (spaces or
dashes) between logical groups and keeping word boundaries; implement this by
replacing the current regexp with one that uses a non-capturing alternation for
the valid formats (e.g., patterns covering 13-digit, 15-digit (AmEx), and
16-digit formats with optional [\s-] separators) so older Visa and AmEx numbers
are detected without matching longer digit sequences.
Comment on lines
+355
to
+384
| func TestAnonymizer_SameValueSameToken(t *testing.T) { | ||
| cfg := AnonymizationConfig{ | ||
| Enabled: true, | ||
| Strategy: StrategyToken, | ||
| Detectors: DetectorConfig{ | ||
| Email: true, | ||
| }, | ||
| } | ||
| a := NewAnonymizer(cfg) | ||
|
|
||
| tokenMap := make(map[string]string) | ||
| input := "Email 1: test@example.com, Email 2: test@example.com" | ||
| result := a.anonymizeText(input, tokenMap) | ||
|
|
||
| // Should only have one token for the same email | ||
| if len(tokenMap) != 1 { | ||
| t.Errorf("expected 1 token for duplicate value, got %d", len(tokenMap)) | ||
| } | ||
|
|
||
| // Count occurrences of the token | ||
| var token string | ||
| for t := range tokenMap { | ||
| token = t | ||
| } | ||
|
|
||
| count := strings.Count(result, token) | ||
| if count != 2 { | ||
| t.Errorf("expected token to appear 2 times, got %d", count) | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Variable shadowing: t shadows the test's *testing.T.
The loop variable t on line 376 shadows the test's t *testing.T parameter. While this doesn't cause a bug here, it could lead to confusing errors if someone tries to add a t.Error() call inside the loop.
♻️ Rename loop variable to avoid shadowing
// Count occurrences of the token
var token string
- for t := range tokenMap {
- token = t
+ for tok := range tokenMap {
+ token = tok
}📝 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.
Suggested change
| func TestAnonymizer_SameValueSameToken(t *testing.T) { | |
| cfg := AnonymizationConfig{ | |
| Enabled: true, | |
| Strategy: StrategyToken, | |
| Detectors: DetectorConfig{ | |
| Email: true, | |
| }, | |
| } | |
| a := NewAnonymizer(cfg) | |
| tokenMap := make(map[string]string) | |
| input := "Email 1: test@example.com, Email 2: test@example.com" | |
| result := a.anonymizeText(input, tokenMap) | |
| // Should only have one token for the same email | |
| if len(tokenMap) != 1 { | |
| t.Errorf("expected 1 token for duplicate value, got %d", len(tokenMap)) | |
| } | |
| // Count occurrences of the token | |
| var token string | |
| for t := range tokenMap { | |
| token = t | |
| } | |
| count := strings.Count(result, token) | |
| if count != 2 { | |
| t.Errorf("expected token to appear 2 times, got %d", count) | |
| } | |
| } | |
| func TestAnonymizer_SameValueSameToken(t *testing.T) { | |
| cfg := AnonymizationConfig{ | |
| Enabled: true, | |
| Strategy: StrategyToken, | |
| Detectors: DetectorConfig{ | |
| Email: true, | |
| }, | |
| } | |
| a := NewAnonymizer(cfg) | |
| tokenMap := make(map[string]string) | |
| input := "Email 1: test@example.com, Email 2: test@example.com" | |
| result := a.anonymizeText(input, tokenMap) | |
| // Should only have one token for the same email | |
| if len(tokenMap) != 1 { | |
| t.Errorf("expected 1 token for duplicate value, got %d", len(tokenMap)) | |
| } | |
| // Count occurrences of the token | |
| var token string | |
| for tok := range tokenMap { | |
| token = tok | |
| } | |
| count := strings.Count(result, token) | |
| if count != 2 { | |
| t.Errorf("expected token to appear 2 times, got %d", count) | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@internal/guardrails/anonymizer_test.go` around lines 355 - 384, In
TestAnonymizer_SameValueSameToken rename the loop variable that iterates over
tokenMap (currently `for t := range tokenMap`) to avoid shadowing the test
parameter `t *testing.T` — e.g., use `for key := range tokenMap` or `for
tokenKey := range tokenMap` — then use that variable when assigning `token`,
keeping the rest of the test logic unchanged.
Comment on lines
+233
to
+242
| case StrategyToken: | ||
| fallthrough | ||
| default: | ||
| // Generate unique token ID | ||
| a.tokenMu.Lock() | ||
| a.tokenCounter++ | ||
| id := a.tokenCounter | ||
| a.tokenMu.Unlock() | ||
| return fmt.Sprintf("[%s_%x]", piiType, id) | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider using atomic.AddUint64 for token counter.
The mutex-protected counter increment is correct but could use atomic.AddUint64 for slightly better performance under contention.
♻️ Optional atomic optimization
+import "sync/atomic"
+
// In generateToken, StrategyToken case:
- a.tokenMu.Lock()
- a.tokenCounter++
- id := a.tokenCounter
- a.tokenMu.Unlock()
+ id := atomic.AddUint64(&a.tokenCounter, 1)
return fmt.Sprintf("[%s_%x]", piiType, id)🤖 Prompt for AI Agents
In `@internal/guardrails/anonymizer.go` around lines 233 - 242, Replace the
mutex-protected increment of a.tokenCounter with an atomic increment: ensure
tokenCounter is a uint64, import sync/atomic, and use id :=
atomic.AddUint64(&a.tokenCounter, 1) instead of locking
a.tokenMu/a.tokenMu.Unlock(); then use the id in the fmt.Sprintf call (inside
the StrategyToken/default case) and remove the now-unused tokenMu locking around
this increment.
Comment on lines
+234
to
+278
| func TestProcessor_WrapStreamForDeanonymization(t *testing.T) { | ||
| cfg := Config{ | ||
| Anonymization: AnonymizationConfig{ | ||
| Enabled: true, | ||
| Strategy: StrategyToken, | ||
| DeanonymizeResponses: true, | ||
| Detectors: DetectorConfig{ | ||
| Email: true, | ||
| }, | ||
| }, | ||
| } | ||
|
|
||
| p := New(cfg) | ||
|
|
||
| // With tokens - should wrap | ||
| ctx := &RequestContext{ | ||
| AnonymizationMap: map[string]string{ | ||
| "[EMAIL_1]": "test@example.com", | ||
| }, | ||
| } | ||
|
|
||
| input := `data: {"content":"[EMAIL_1]"} | ||
|
|
||
| ` | ||
| stream := io.NopCloser(strings.NewReader(input)) | ||
| wrapped := p.WrapStreamForDeanonymization(stream, ctx) | ||
|
|
||
| output, _ := io.ReadAll(wrapped) | ||
| if strings.Contains(string(output), "[EMAIL_1]") { | ||
| t.Error("wrapped stream should de-anonymize tokens") | ||
| } | ||
|
|
||
| // Without tokens - should return original | ||
| emptyCtx := &RequestContext{ | ||
| AnonymizationMap: map[string]string{}, | ||
| } | ||
|
|
||
| stream2 := io.NopCloser(strings.NewReader(input)) | ||
| notWrapped := p.WrapStreamForDeanonymization(stream2, emptyCtx) | ||
|
|
||
| // Should be the same reader (not wrapped) | ||
| if notWrapped != stream2 { | ||
| t.Error("empty context should return original stream") | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider closing streams explicitly for consistency.
While io.NopCloser doesn't require explicit closing, adding defer wrapped.Close() would demonstrate the expected usage pattern and align with the coding guideline to "ensure caller closes streaming responses which return io.ReadCloser".
♻️ Suggested improvement
wrapped := p.WrapStreamForDeanonymization(stream, ctx)
+ defer wrapped.Close()
output, _ := io.ReadAll(wrapped)🤖 Prompt for AI Agents
In `@internal/guardrails/guardrails_test.go` around lines 234 - 278, In
TestProcessor_WrapStreamForDeanonymization add explicit closes for any
io.ReadCloser used: call Close (via defer) on the original streams (stream,
stream2) and on any wrapped readers returned by p.WrapStreamForDeanonymization
(wrapped, notWrapped) so tests demonstrate the expected usage pattern and ensure
streaming responses are closed; update the test to defer wrapped.Close() after
getting wrapped and defer notWrapped.Close() (or close only the returned
readers) to follow the guideline that callers close io.ReadCloser results.
Comment on lines
+279
to
+373
| func TestSystemPromptInjector_ProcessResponsesRequest(t *testing.T) { | ||
| tests := []struct { | ||
| name string | ||
| config SystemPromptConfig | ||
| request *core.ResponsesRequest | ||
| providerType string | ||
| wantInstructions string | ||
| wantUnchanged bool | ||
| }{ | ||
| { | ||
| name: "prepend to empty instructions", | ||
| config: SystemPromptConfig{ | ||
| Enabled: true, | ||
| Global: &SystemPromptRule{ | ||
| Prompt: "You are helpful.", | ||
| Position: PositionPrepend, | ||
| }, | ||
| }, | ||
| request: &core.ResponsesRequest{ | ||
| Model: "gpt-4", | ||
| Instructions: "", | ||
| }, | ||
| providerType: "openai", | ||
| wantInstructions: "You are helpful.", | ||
| }, | ||
| { | ||
| name: "prepend to existing instructions - preserve", | ||
| config: SystemPromptConfig{ | ||
| Enabled: true, | ||
| Global: &SystemPromptRule{ | ||
| Prompt: "You are helpful.", | ||
| Position: PositionPrepend, | ||
| PreserveUserSystem: true, | ||
| }, | ||
| }, | ||
| request: &core.ResponsesRequest{ | ||
| Model: "gpt-4", | ||
| Instructions: "Be concise.", | ||
| }, | ||
| providerType: "openai", | ||
| wantInstructions: "You are helpful.\n\nBe concise.", | ||
| }, | ||
| { | ||
| name: "append to existing instructions - preserve", | ||
| config: SystemPromptConfig{ | ||
| Enabled: true, | ||
| Global: &SystemPromptRule{ | ||
| Prompt: "Always be polite.", | ||
| Position: PositionAppend, | ||
| PreserveUserSystem: true, | ||
| }, | ||
| }, | ||
| request: &core.ResponsesRequest{ | ||
| Model: "gpt-4", | ||
| Instructions: "Be concise.", | ||
| }, | ||
| providerType: "openai", | ||
| wantInstructions: "Be concise.\n\nAlways be polite.", | ||
| }, | ||
| { | ||
| name: "replace instructions", | ||
| config: SystemPromptConfig{ | ||
| Enabled: true, | ||
| Global: &SystemPromptRule{ | ||
| Prompt: "New instructions.", | ||
| Position: PositionReplace, | ||
| }, | ||
| }, | ||
| request: &core.ResponsesRequest{ | ||
| Model: "gpt-4", | ||
| Instructions: "Old instructions.", | ||
| }, | ||
| providerType: "openai", | ||
| wantInstructions: "New instructions.", | ||
| }, | ||
| } | ||
|
|
||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| injector := NewSystemPromptInjector(tt.config) | ||
| result := injector.ProcessResponsesRequest(tt.request, tt.providerType) | ||
|
|
||
| if tt.wantUnchanged { | ||
| if result != tt.request { | ||
| t.Error("expected unchanged request pointer") | ||
| } | ||
| return | ||
| } | ||
|
|
||
| if result.Instructions != tt.wantInstructions { | ||
| t.Errorf("instructions: got %q, want %q", result.Instructions, tt.wantInstructions) | ||
| } | ||
| }) | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding a test case where wantUnchanged: true.
The wantUnchanged field is defined but never set to true in any ProcessResponsesRequest test case. Consider adding a test for when no rules are configured (similar to the first case in ProcessChatRequest tests) to exercise that code path.
♻️ Suggested test case addition
tests := []struct {
name string
config SystemPromptConfig
request *core.ResponsesRequest
providerType string
wantInstructions string
wantUnchanged bool
}{
+ {
+ name: "no rules configured",
+ config: SystemPromptConfig{
+ Enabled: true,
+ },
+ request: &core.ResponsesRequest{
+ Model: "gpt-4",
+ Instructions: "User instructions",
+ },
+ providerType: "openai",
+ wantUnchanged: true,
+ },
{
name: "prepend to empty instructions",📝 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.
Suggested change
| func TestSystemPromptInjector_ProcessResponsesRequest(t *testing.T) { | |
| tests := []struct { | |
| name string | |
| config SystemPromptConfig | |
| request *core.ResponsesRequest | |
| providerType string | |
| wantInstructions string | |
| wantUnchanged bool | |
| }{ | |
| { | |
| name: "prepend to empty instructions", | |
| config: SystemPromptConfig{ | |
| Enabled: true, | |
| Global: &SystemPromptRule{ | |
| Prompt: "You are helpful.", | |
| Position: PositionPrepend, | |
| }, | |
| }, | |
| request: &core.ResponsesRequest{ | |
| Model: "gpt-4", | |
| Instructions: "", | |
| }, | |
| providerType: "openai", | |
| wantInstructions: "You are helpful.", | |
| }, | |
| { | |
| name: "prepend to existing instructions - preserve", | |
| config: SystemPromptConfig{ | |
| Enabled: true, | |
| Global: &SystemPromptRule{ | |
| Prompt: "You are helpful.", | |
| Position: PositionPrepend, | |
| PreserveUserSystem: true, | |
| }, | |
| }, | |
| request: &core.ResponsesRequest{ | |
| Model: "gpt-4", | |
| Instructions: "Be concise.", | |
| }, | |
| providerType: "openai", | |
| wantInstructions: "You are helpful.\n\nBe concise.", | |
| }, | |
| { | |
| name: "append to existing instructions - preserve", | |
| config: SystemPromptConfig{ | |
| Enabled: true, | |
| Global: &SystemPromptRule{ | |
| Prompt: "Always be polite.", | |
| Position: PositionAppend, | |
| PreserveUserSystem: true, | |
| }, | |
| }, | |
| request: &core.ResponsesRequest{ | |
| Model: "gpt-4", | |
| Instructions: "Be concise.", | |
| }, | |
| providerType: "openai", | |
| wantInstructions: "Be concise.\n\nAlways be polite.", | |
| }, | |
| { | |
| name: "replace instructions", | |
| config: SystemPromptConfig{ | |
| Enabled: true, | |
| Global: &SystemPromptRule{ | |
| Prompt: "New instructions.", | |
| Position: PositionReplace, | |
| }, | |
| }, | |
| request: &core.ResponsesRequest{ | |
| Model: "gpt-4", | |
| Instructions: "Old instructions.", | |
| }, | |
| providerType: "openai", | |
| wantInstructions: "New instructions.", | |
| }, | |
| } | |
| for _, tt := range tests { | |
| t.Run(tt.name, func(t *testing.T) { | |
| injector := NewSystemPromptInjector(tt.config) | |
| result := injector.ProcessResponsesRequest(tt.request, tt.providerType) | |
| if tt.wantUnchanged { | |
| if result != tt.request { | |
| t.Error("expected unchanged request pointer") | |
| } | |
| return | |
| } | |
| if result.Instructions != tt.wantInstructions { | |
| t.Errorf("instructions: got %q, want %q", result.Instructions, tt.wantInstructions) | |
| } | |
| }) | |
| } | |
| } | |
| func TestSystemPromptInjector_ProcessResponsesRequest(t *testing.T) { | |
| tests := []struct { | |
| name string | |
| config SystemPromptConfig | |
| request *core.ResponsesRequest | |
| providerType string | |
| wantInstructions string | |
| wantUnchanged bool | |
| }{ | |
| { | |
| name: "no rules configured", | |
| config: SystemPromptConfig{ | |
| Enabled: true, | |
| }, | |
| request: &core.ResponsesRequest{ | |
| Model: "gpt-4", | |
| Instructions: "User instructions", | |
| }, | |
| providerType: "openai", | |
| wantUnchanged: true, | |
| }, | |
| { | |
| name: "prepend to empty instructions", | |
| config: SystemPromptConfig{ | |
| Enabled: true, | |
| Global: &SystemPromptRule{ | |
| Prompt: "You are helpful.", | |
| Position: PositionPrepend, | |
| }, | |
| }, | |
| request: &core.ResponsesRequest{ | |
| Model: "gpt-4", | |
| Instructions: "", | |
| }, | |
| providerType: "openai", | |
| wantInstructions: "You are helpful.", | |
| }, | |
| { | |
| name: "prepend to existing instructions - preserve", | |
| config: SystemPromptConfig{ | |
| Enabled: true, | |
| Global: &SystemPromptRule{ | |
| Prompt: "You are helpful.", | |
| Position: PositionPrepend, | |
| PreserveUserSystem: true, | |
| }, | |
| }, | |
| request: &core.ResponsesRequest{ | |
| Model: "gpt-4", | |
| Instructions: "Be concise.", | |
| }, | |
| providerType: "openai", | |
| wantInstructions: "You are helpful.\n\nBe concise.", | |
| }, | |
| { | |
| name: "append to existing instructions - preserve", | |
| config: SystemPromptConfig{ | |
| Enabled: true, | |
| Global: &SystemPromptRule{ | |
| Prompt: "Always be polite.", | |
| Position: PositionAppend, | |
| PreserveUserSystem: true, | |
| }, | |
| }, | |
| request: &core.ResponsesRequest{ | |
| Model: "gpt-4", | |
| Instructions: "Be concise.", | |
| }, | |
| providerType: "openai", | |
| wantInstructions: "Be concise.\n\nAlways be polite.", | |
| }, | |
| { | |
| name: "replace instructions", | |
| config: SystemPromptConfig{ | |
| Enabled: true, | |
| Global: &SystemPromptRule{ | |
| Prompt: "New instructions.", | |
| Position: PositionReplace, | |
| }, | |
| }, | |
| request: &core.ResponsesRequest{ | |
| Model: "gpt-4", | |
| Instructions: "Old instructions.", | |
| }, | |
| providerType: "openai", | |
| wantInstructions: "New instructions.", | |
| }, | |
| } | |
| for _, tt := range tests { | |
| t.Run(tt.name, func(t *testing.T) { | |
| injector := NewSystemPromptInjector(tt.config) | |
| result := injector.ProcessResponsesRequest(tt.request, tt.providerType) | |
| if tt.wantUnchanged { | |
| if result != tt.request { | |
| t.Error("expected unchanged request pointer") | |
| } | |
| return | |
| } | |
| if result.Instructions != tt.wantInstructions { | |
| t.Errorf("instructions: got %q, want %q", result.Instructions, tt.wantInstructions) | |
| } | |
| }) | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@internal/guardrails/system_prompt_test.go` around lines 279 - 373, Add a test
case in TestSystemPromptInjector_ProcessResponsesRequest that sets
wantUnchanged: true to cover the "no rules" path; e.g., create a
SystemPromptConfig with Enabled: false (or Global: nil), a non-empty
core.ResponsesRequest, and providerType "openai", then assert the returned
pointer equals the original request. Update the tests slice in
TestSystemPromptInjector_ProcessResponsesRequest to include this case so
NewSystemPromptInjector and ProcessResponsesRequest are exercised when no
injection rules apply.
Comment on lines
+49
to
+87
| // ProcessResponsesRequest applies system prompt injection to a ResponsesRequest. | ||
| // For ResponsesRequest, the system prompt goes into the Instructions field. | ||
| func (s *SystemPromptInjector) ProcessResponsesRequest(req *core.ResponsesRequest, providerType string) *core.ResponsesRequest { | ||
| rule := s.resolveRule(req.Model, providerType) | ||
| if rule == nil || rule.Prompt == "" { | ||
| return req | ||
| } | ||
|
|
||
| // Create a copy of the request | ||
| newReq := *req | ||
|
|
||
| position := rule.Position | ||
| if position == "" { | ||
| position = PositionPrepend | ||
| } | ||
|
|
||
| switch position { | ||
| case PositionPrepend: | ||
| if req.Instructions != "" && rule.PreserveUserSystem { | ||
| newReq.Instructions = rule.Prompt + "\n\n" + req.Instructions | ||
| } else if req.Instructions != "" && !rule.PreserveUserSystem { | ||
| newReq.Instructions = rule.Prompt | ||
| } else { | ||
| newReq.Instructions = rule.Prompt | ||
| } | ||
| case PositionAppend: | ||
| if req.Instructions != "" && rule.PreserveUserSystem { | ||
| newReq.Instructions = req.Instructions + "\n\n" + rule.Prompt | ||
| } else if req.Instructions != "" && !rule.PreserveUserSystem { | ||
| newReq.Instructions = rule.Prompt | ||
| } else { | ||
| newReq.Instructions = rule.Prompt | ||
| } | ||
| case PositionReplace: | ||
| newReq.Instructions = rule.Prompt | ||
| } | ||
|
|
||
| return &newReq | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Shallow copy may share mutable Input field with original request.
Line 58 creates a shallow copy of ResponsesRequest, but the Input field (which can be []interface{} or []ResponsesInputItem) is not deep-copied. Modifications to Input elements could affect the original request, though this method only modifies Instructions.
This is currently safe since only Instructions is changed, but adding a comment or documenting this invariant would prevent future bugs if Input processing is added.
🤖 Prompt for AI Agents
In `@internal/guardrails/system_prompt.go` around lines 49 - 87,
ProcessResponsesRequest makes a shallow copy of ResponsesRequest into newReq
which leaves the mutable Input slice shared with the original; either deep-copy
the Input slice/elements into newReq.Input before modifying Instructions or add
a clear comment/documentation near ProcessResponsesRequest and the newReq
creation that the function intentionally only mutates Instructions and must not
touch Input (and that future changes must deep-copy Input if they will modify
it). Locate the newReq assignment in ProcessResponsesRequest and implement the
deep-copy of req.Input (preserving element types like []interface{} or
[]ResponsesInputItem) or add the invariant comment adjacent to the newReq
creation and function docstring.
Comment on lines
+108
to
+203
| switch position { | ||
| case PositionPrepend: | ||
| if systemIdx >= 0 { | ||
| // There's an existing system message | ||
| if rule.PreserveUserSystem { | ||
| // Prepend to existing: combined system message first | ||
| for i, msg := range messages { | ||
| if i == systemIdx { | ||
| newMessages = append(newMessages, core.Message{ | ||
| Role: "system", | ||
| Content: rule.Prompt + "\n\n" + msg.Content, | ||
| }) | ||
| } else { | ||
| newMessages = append(newMessages, msg) | ||
| } | ||
| } | ||
| } else { | ||
| // Replace existing with our prompt | ||
| for i, msg := range messages { | ||
| if i == systemIdx { | ||
| newMessages = append(newMessages, core.Message{ | ||
| Role: "system", | ||
| Content: rule.Prompt, | ||
| }) | ||
| } else { | ||
| newMessages = append(newMessages, msg) | ||
| } | ||
| } | ||
| } | ||
| } else { | ||
| // No existing system message - add one at the beginning | ||
| newMessages = append(newMessages, core.Message{ | ||
| Role: "system", | ||
| Content: rule.Prompt, | ||
| }) | ||
| newMessages = append(newMessages, messages...) | ||
| } | ||
|
|
||
| case PositionAppend: | ||
| if systemIdx >= 0 { | ||
| // There's an existing system message | ||
| if rule.PreserveUserSystem { | ||
| // Append to existing: existing content + our prompt | ||
| for i, msg := range messages { | ||
| if i == systemIdx { | ||
| newMessages = append(newMessages, core.Message{ | ||
| Role: "system", | ||
| Content: msg.Content + "\n\n" + rule.Prompt, | ||
| }) | ||
| } else { | ||
| newMessages = append(newMessages, msg) | ||
| } | ||
| } | ||
| } else { | ||
| // Replace existing with our prompt | ||
| for i, msg := range messages { | ||
| if i == systemIdx { | ||
| newMessages = append(newMessages, core.Message{ | ||
| Role: "system", | ||
| Content: rule.Prompt, | ||
| }) | ||
| } else { | ||
| newMessages = append(newMessages, msg) | ||
| } | ||
| } | ||
| } | ||
| } else { | ||
| // No existing system message - add one at the beginning (even for append mode) | ||
| newMessages = append(newMessages, core.Message{ | ||
| Role: "system", | ||
| Content: rule.Prompt, | ||
| }) | ||
| newMessages = append(newMessages, messages...) | ||
| } | ||
|
|
||
| case PositionReplace: | ||
| // Replace any existing system message or add new one | ||
| if systemIdx >= 0 { | ||
| for i, msg := range messages { | ||
| if i == systemIdx { | ||
| newMessages = append(newMessages, core.Message{ | ||
| Role: "system", | ||
| Content: rule.Prompt, | ||
| }) | ||
| } else { | ||
| newMessages = append(newMessages, msg) | ||
| } | ||
| } | ||
| } else { | ||
| newMessages = append(newMessages, core.Message{ | ||
| Role: "system", | ||
| Content: rule.Prompt, | ||
| }) | ||
| newMessages = append(newMessages, messages...) | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider extracting duplicated system message replacement logic.
The code for replacing an existing system message when PreserveUserSystem=false is duplicated across PositionPrepend (lines 124-136) and PositionAppend (lines 161-173), with identical implementations. Similarly, the "no existing system message" fallback (lines 137-144, 174-181, 196-202) is repeated.
This could be simplified by extracting common patterns into helper functions.
♻️ Example refactoring approach
// Helper to build new messages with modified/added system message
func buildMessagesWithSystem(messages []core.Message, systemIdx int, systemContent string) []core.Message {
newMessages := make([]core.Message, 0, len(messages)+1)
if systemIdx >= 0 {
for i, msg := range messages {
if i == systemIdx {
newMessages = append(newMessages, core.Message{Role: "system", Content: systemContent})
} else {
newMessages = append(newMessages, msg)
}
}
} else {
newMessages = append(newMessages, core.Message{Role: "system", Content: systemContent})
newMessages = append(newMessages, messages...)
}
return newMessages
}🤖 Prompt for AI Agents
In `@internal/guardrails/system_prompt.go` around lines 108 - 203, The switch
handling PositionPrepend/PositionAppend/PositionReplace duplicates logic for
replacing or adding the system message (when rule.PreserveUserSystem is false
and when no existing system message exists); refactor by extracting a helper
(e.g., buildMessagesWithSystem or applySystemReplacement) that accepts messages,
systemIdx, and the desired systemContent and returns newMessages, then call that
helper from the cases instead of repeating the for-loops and append logic;
ensure the helper handles both the "replace existing" (iterating messages and
replacing at systemIdx) and "add at beginning" (prepend new
core.Message{Role:"system", Content: ...} + messages) behaviors and update
PositionPrepend/PositionAppend to call it for the non-preserve branches while
keeping the PreserveUserSystem branches for combine/append behavior.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Release Notes
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.