Skip to content

Commit 25ebca3

Browse files
SantiagoDePoloniaSantiagoDePolonia
committed
fix(guardrails): address review follow-ups
1 parent 80c1274 commit 25ebca3

14 files changed

Lines changed: 184 additions & 19 deletions

‎internal/admin/dashboard/static/css/dashboard.css‎

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1115,6 +1115,22 @@ td.col-price {
11151115
color: var(--text-muted);
11161116
}
11171117

1118+
.alias-form-field-fieldset {
1119+
border: 0;
1120+
margin: 0;
1121+
min-inline-size: 0;
1122+
padding: 0;
1123+
}
1124+
1125+
.alias-form-field-legend {
1126+
color: var(--text-muted);
1127+
font-size: 12px;
1128+
font-weight: 600;
1129+
letter-spacing: 0.5px;
1130+
padding: 0;
1131+
text-transform: uppercase;
1132+
}
1133+
11181134
.alias-form-textarea {
11191135
width: 100%;
11201136
min-height: 110px;

‎internal/admin/dashboard/static/js/modules/timezone-layout.test.js‎

Lines changed: 13 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -103,6 +103,7 @@ test('dashboard templates expose a settings page and timezone context in activit
103103

104104
test('guardrails authoring moved to a top-level page while settings keeps the general switch', () => {
105105
const template = readHelperDisclosureTemplateSource();
106+
const css = readFixture('../../css/dashboard.css');
106107

107108
assert.match(template, /<div class="settings-subnav">[\s\S]*class="settings-subnav-btn active"[\s\S]*>General<\/button>/);
108109
assert.match(template, /<div x-show="page==='guardrails'">[\s\S]*<h2>Guardrails<\/h2>/);
@@ -111,5 +112,16 @@ test('guardrails authoring moved to a top-level page while settings keeps the ge
111112
assert.match(template, /x-model="guardrailForm\.type"/);
112113
assert.match(template, /x-effect="guardrailTypes\.length; guardrailForm\.type; \$nextTick\(\(\) => syncGuardrailTypeSelectValue\(\)\)"/);
113114
assert.match(template, /x-model="guardrailForm\.user_path"[^>]*aria-label="Guardrail user path"/);
114-
assert.match(template, /Optional base user path for internal auxiliary rewrite requests\. When empty, the caller user path is reused\./);
115+
assert.match(template, /Only used for auxiliary rewrite \(llm_based_altering\) guardrails; ignored for other guardrail types\./);
116+
assert.match(template, /<fieldset class="alias-form-field alias-form-field-wide alias-form-field-fieldset"[^>]*:aria-describedby="field\.help \? 'guardrail-field-help-' \+ field\.key : null"/);
117+
assert.match(template, /<legend class="alias-form-field-legend" x-text="field\.label"><\/legend>/);
118+
assert.match(template, /<small class="alias-form-hint" :id="'guardrail-field-help-' \+ field\.key" x-show="field\.help" x-text="field\.help"><\/small>/);
119+
120+
const fieldsetRule = readCSSRule(css, '.alias-form-field-fieldset');
121+
assert.match(fieldsetRule, /border:\s*0/);
122+
assert.match(fieldsetRule, /padding:\s*0/);
123+
124+
const legendRule = readCSSRule(css, '.alias-form-field-legend');
125+
assert.match(legendRule, /text-transform:\s*uppercase/);
126+
assert.match(legendRule, /letter-spacing:\s*0\.5px/);
115127
});

‎internal/admin/dashboard/templates/index.html‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -384,7 +384,7 @@ <h3 x-text="guardrailFormMode === 'edit' ? 'Edit Guardrail' : 'Create Guardrail'
384384
<label class="alias-form-field">
385385
<span>User Path</span>
386386
<input type="text" class="filter-input" placeholder="/team/alpha" x-model="guardrailForm.user_path" aria-label="Guardrail user path">
387-
<small class="alias-form-hint">Optional base user path for internal auxiliary rewrite requests. When empty, the caller user path is reused.</small>
387+
<small class="alias-form-hint">Only used for auxiliary rewrite (llm_based_altering) guardrails; ignored for other guardrail types.</small>
388388
</label>
389389

390390
<template x-for="field in guardrailTypeFields(guardrailForm.type)" :key="field.key">
@@ -409,8 +409,8 @@ <h3 x-text="guardrailFormMode === 'edit' ? 'Edit Guardrail' : 'Create Guardrail'
409409
</label>
410410
</template>
411411
<template x-if="field.input === 'checkboxes'">
412-
<div class="alias-form-field alias-form-field-wide">
413-
<span x-text="field.label"></span>
412+
<fieldset class="alias-form-field alias-form-field-wide alias-form-field-fieldset" :aria-describedby="field.help ? 'guardrail-field-help-' + field.key : null">
413+
<legend class="alias-form-field-legend" x-text="field.label"></legend>
414414
<div class="execution-plan-feature-toggles">
415415
<template x-for="option in field.options" :key="field.key + '-' + option.value">
416416
<label class="execution-plan-feature-toggle">
@@ -419,8 +419,8 @@ <h3 x-text="guardrailFormMode === 'edit' ? 'Edit Guardrail' : 'Create Guardrail'
419419
</label>
420420
</template>
421421
</div>
422-
<small class="alias-form-hint" x-show="field.help" x-text="field.help"></small>
423-
</div>
422+
<small class="alias-form-hint" :id="'guardrail-field-help-' + field.key" x-show="field.help" x-text="field.help"></small>
423+
</fieldset>
424424
</template>
425425
</div>
426426
</template>

‎internal/admin/handler_guardrails_test.go‎

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ import (
1111

1212
"github.com/labstack/echo/v5"
1313

14+
"gomodel/internal/core"
1415
"gomodel/internal/executionplans"
1516
"gomodel/internal/guardrails"
1617
)
@@ -313,6 +314,20 @@ func TestUpsertGuardrailRejectsSlashInName(t *testing.T) {
313314
if rec.Code != http.StatusBadRequest {
314315
t.Fatalf("status = %d, want 400", rec.Code)
315316
}
317+
318+
envelope := decodeExecutionPlanErrorEnvelope(t, rec.Body.Bytes())
319+
if envelope.Error.Type != string(core.ErrorTypeInvalidRequest) {
320+
t.Fatalf("error type = %q, want %q", envelope.Error.Type, core.ErrorTypeInvalidRequest)
321+
}
322+
if envelope.Error.Message != "guardrail name cannot contain '/'" {
323+
t.Fatalf("error message = %q, want guardrail name validation failure", envelope.Error.Message)
324+
}
325+
if envelope.Error.Param != nil {
326+
t.Fatalf("error param = %v, want nil", *envelope.Error.Param)
327+
}
328+
if envelope.Error.Code != nil {
329+
t.Fatalf("error code = %v, want nil", *envelope.Error.Code)
330+
}
316331
}
317332

318333
func TestDeleteGuardrailRejectsActiveWorkflowReference(t *testing.T) {

‎internal/auditlog/entry_capture.go‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -201,10 +201,10 @@ func internalJSONAuditHeaders(ctx context.Context, requestID string) http.Header
201201
}
202202

203203
func boundedAuditBody(body []byte, truncate bool) ([]byte, bool) {
204+
if body == nil {
205+
return []byte{}, false
206+
}
204207
if len(body) <= MaxBodyCapture {
205-
if body == nil {
206-
return []byte{}, false
207-
}
208208
cloned := make([]byte, len(body))
209209
copy(cloned, body)
210210
return cloned, false

‎internal/guardrails/factory.go‎

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -61,6 +61,9 @@ func New(ctx context.Context, cfg *config.Config, refreshInterval time.Duration,
6161
if cfg == nil {
6262
return nil, fmt.Errorf("config is required")
6363
}
64+
if err := validateExecutorCount(executors); err != nil {
65+
return nil, err
66+
}
6467
storeConn, err := storage.New(ctx, cfg.Storage.BackendConfig())
6568
if err != nil {
6669
return nil, fmt.Errorf("failed to create storage: %w", err)
@@ -79,10 +82,16 @@ func NewWithSharedStorage(ctx context.Context, shared storage.Storage, refreshIn
7982
if shared == nil {
8083
return nil, fmt.Errorf("shared storage is required")
8184
}
85+
if err := validateExecutorCount(executors); err != nil {
86+
return nil, err
87+
}
8288
return newResult(ctx, shared, refreshInterval, executors...)
8389
}
8490

8591
func newResult(ctx context.Context, storeConn storage.Storage, refreshInterval time.Duration, executors ...ChatCompletionExecutor) (*Result, error) {
92+
if err := validateExecutorCount(executors); err != nil {
93+
return nil, err
94+
}
8695
store, err := createStore(ctx, storeConn)
8796
if err != nil {
8897
return nil, err
@@ -156,3 +165,10 @@ func startGuardrailRefreshLoop(parent context.Context, service *Service, interva
156165
})
157166
}, errs
158167
}
168+
169+
func validateExecutorCount(executors []ChatCompletionExecutor) error {
170+
if len(executors) > 1 {
171+
return fmt.Errorf("only one ChatCompletionExecutor is supported")
172+
}
173+
return nil
174+
}

‎internal/guardrails/llm_based_altering_test.go‎

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ package guardrails
22

33
import (
44
"context"
5+
"errors"
56
"fmt"
67
"sync/atomic"
78
"testing"
@@ -216,6 +217,41 @@ func TestLLMBasedAltering_Process_PropagatesContextCancellation(t *testing.T) {
216217
}
217218
}
218219

220+
func TestLLMBasedAltering_Process_PropagatesMidFlightCancellation(t *testing.T) {
221+
ctx, cancel := context.WithCancel(context.Background())
222+
started := make(chan struct{})
223+
224+
g, err := NewLLMBasedAlteringGuardrail("privacy", LLMBasedAlteringConfig{
225+
Model: "gpt-4o-mini",
226+
}, mockChatCompletionExecutor{
227+
chatFn: func(ctx context.Context, _ *core.ChatRequest) (*core.ChatResponse, error) {
228+
close(started)
229+
<-ctx.Done()
230+
return nil, ctx.Err()
231+
},
232+
})
233+
if err != nil {
234+
t.Fatalf("NewLLMBasedAlteringGuardrail() error = %v", err)
235+
}
236+
237+
resultCh := make(chan error, 1)
238+
go func() {
239+
_, err := g.Process(ctx, []Message{{Role: "user", Content: "John says hello"}})
240+
resultCh <- err
241+
}()
242+
243+
<-started
244+
cancel()
245+
246+
err = <-resultCh
247+
if err == nil {
248+
t.Fatal("expected context cancellation error")
249+
}
250+
if !errors.Is(err, context.Canceled) {
251+
t.Fatalf("error = %v, want context.Canceled", err)
252+
}
253+
}
254+
219255
func TestLLMBasedAltering_Process_FailsOpenOnToolCallCompletion(t *testing.T) {
220256
g, err := NewLLMBasedAlteringGuardrail("privacy", LLMBasedAlteringConfig{
221257
Model: "gpt-4o-mini",

‎internal/guardrails/provider.go‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1420,6 +1420,9 @@ func cloneResponsesInterfacePart(part any) any {
14201420
return cloneStringAnyMap(partMap)
14211421
}
14221422

1423+
// cloneStringAnyMap performs a shallow copy of the map. Nested maps/slices are
1424+
// intentionally shared; callers are expected to either preserve them as-is or
1425+
// replace whole top-level values instead of mutating nested structures in place.
14231426
func cloneStringAnyMap(src map[string]any) map[string]any {
14241427
if src == nil {
14251428
return nil

‎internal/guardrails/service.go‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,9 @@ func NewService(store Store, executors ...ChatCompletionExecutor) (*Service, err
3232
if store == nil {
3333
return nil, fmt.Errorf("store is required")
3434
}
35+
if len(executors) > 1 {
36+
return nil, fmt.Errorf("only one ChatCompletionExecutor is supported")
37+
}
3538
var executor ChatCompletionExecutor
3639
if len(executors) > 0 {
3740
executor = executors[0]

‎internal/guardrails/service_test.go‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -129,6 +129,18 @@ func TestServiceRefreshBuildsPipelineFromDefinitions(t *testing.T) {
129129
}
130130
}
131131

132+
func TestNewServiceRejectsMultipleExecutors(t *testing.T) {
133+
store := newTestStore()
134+
135+
_, err := NewService(store, mockChatCompletionExecutor{}, mockChatCompletionExecutor{})
136+
if err == nil {
137+
t.Fatal("NewService() error = nil, want multiple executor validation error")
138+
}
139+
if err.Error() != "only one ChatCompletionExecutor is supported" {
140+
t.Fatalf("NewService() error = %q, want multiple executor validation error", err)
141+
}
142+
}
143+
132144
func TestServiceRefreshBuildsLLMBasedAlteringPipelineFromDefinitions(t *testing.T) {
133145
store := newTestStore(
134146
Definition{

0 commit comments

Comments
 (0)