Centralize message role invariants
This commit is contained in:
@@ -54,6 +54,8 @@ Apply these constraints throughout every stage:
|
|||||||
|
|
||||||
## Stage 1: Centralize Message Invariants And Tighten Prompt Roles
|
## Stage 1: Centralize Message Invariants And Tighten Prompt Roles
|
||||||
|
|
||||||
|
**Status:** Complete
|
||||||
|
|
||||||
### Objective
|
### Objective
|
||||||
|
|
||||||
Establish one source-neutral owner for chat-message role and cache-control
|
Establish one source-neutral owner for chat-message role and cache-control
|
||||||
|
|||||||
78
internal/domain/message.go
Normal file
78
internal/domain/message.go
Normal file
@@ -0,0 +1,78 @@
|
|||||||
|
package domain
|
||||||
|
|
||||||
|
import (
|
||||||
|
"errors"
|
||||||
|
"strings"
|
||||||
|
"unicode/utf8"
|
||||||
|
)
|
||||||
|
|
||||||
|
const (
|
||||||
|
RoleDeveloper = "developer"
|
||||||
|
RoleSystem = "system"
|
||||||
|
RoleUser = "user"
|
||||||
|
RoleAssistant = "assistant"
|
||||||
|
)
|
||||||
|
|
||||||
|
// NormalizeMessageRole validates and canonicalizes a provider-bound chat role.
|
||||||
|
func NormalizeMessageRole(role string) (string, error) {
|
||||||
|
if !utf8.ValidString(role) {
|
||||||
|
return "", errors.New("message role must be valid UTF-8")
|
||||||
|
}
|
||||||
|
|
||||||
|
normalized := strings.ToLower(strings.TrimSpace(role))
|
||||||
|
switch normalized {
|
||||||
|
case RoleDeveloper, RoleSystem, RoleUser, RoleAssistant:
|
||||||
|
return normalized, nil
|
||||||
|
default:
|
||||||
|
return "", errors.New("message role must be developer, system, user, or assistant")
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
// NormalizeCacheControl validates, canonicalizes, and copies cache metadata.
|
||||||
|
func NormalizeCacheControl(control *CacheControl) (*CacheControl, error) {
|
||||||
|
if control == nil {
|
||||||
|
return nil, nil
|
||||||
|
}
|
||||||
|
if !utf8.ValidString(string(control.Type)) {
|
||||||
|
return nil, errors.New("cache control type must be valid UTF-8")
|
||||||
|
}
|
||||||
|
if !utf8.ValidString(control.TTL) {
|
||||||
|
return nil, errors.New("cache control ttl must be valid UTF-8")
|
||||||
|
}
|
||||||
|
|
||||||
|
cacheType := strings.TrimSpace(string(control.Type))
|
||||||
|
if cacheType == "" {
|
||||||
|
return nil, errors.New("cache control type is required")
|
||||||
|
}
|
||||||
|
if CacheControlType(cacheType) != CacheControlEphemeral {
|
||||||
|
return nil, errors.New("unsupported type")
|
||||||
|
}
|
||||||
|
|
||||||
|
ttl := strings.TrimSpace(control.TTL)
|
||||||
|
if ttl != "" && ttl != "1h" {
|
||||||
|
return nil, errors.New("unsupported ttl")
|
||||||
|
}
|
||||||
|
|
||||||
|
return &CacheControl{Type: CacheControlType(cacheType), TTL: ttl}, nil
|
||||||
|
}
|
||||||
|
|
||||||
|
// CloneRenderedMessages returns a deep copy of rendered messages.
|
||||||
|
func CloneRenderedMessages(messages []RenderedMessage) []RenderedMessage {
|
||||||
|
cloned := make([]RenderedMessage, len(messages))
|
||||||
|
for index, message := range messages {
|
||||||
|
cloned[index] = message
|
||||||
|
if message.CacheControl != nil {
|
||||||
|
cacheControl := *message.CacheControl
|
||||||
|
cloned[index].CacheControl = &cacheControl
|
||||||
|
}
|
||||||
|
}
|
||||||
|
return cloned
|
||||||
|
}
|
||||||
|
|
||||||
|
// ConcatRenderedMessages returns an independently owned concatenation of messages.
|
||||||
|
func ConcatRenderedMessages(prefix, suffix []RenderedMessage) []RenderedMessage {
|
||||||
|
messages := make([]RenderedMessage, 0, len(prefix)+len(suffix))
|
||||||
|
messages = append(messages, CloneRenderedMessages(prefix)...)
|
||||||
|
messages = append(messages, CloneRenderedMessages(suffix)...)
|
||||||
|
return messages
|
||||||
|
}
|
||||||
121
internal/domain/message_test.go
Normal file
121
internal/domain/message_test.go
Normal file
@@ -0,0 +1,121 @@
|
|||||||
|
package domain
|
||||||
|
|
||||||
|
import (
|
||||||
|
"strings"
|
||||||
|
"testing"
|
||||||
|
)
|
||||||
|
|
||||||
|
func TestNormalizeMessageRole(t *testing.T) {
|
||||||
|
tests := []struct {
|
||||||
|
name string
|
||||||
|
input string
|
||||||
|
want string
|
||||||
|
wantErr bool
|
||||||
|
}{
|
||||||
|
{name: "developer", input: RoleDeveloper, want: RoleDeveloper},
|
||||||
|
{name: "system", input: RoleSystem, want: RoleSystem},
|
||||||
|
{name: "user", input: RoleUser, want: RoleUser},
|
||||||
|
{name: "assistant", input: RoleAssistant, want: RoleAssistant},
|
||||||
|
{name: "surrounding whitespace and mixed case", input: " \u2003UsEr\u2003 ", want: RoleUser},
|
||||||
|
{name: "blank", input: " \t\n ", wantErr: true},
|
||||||
|
{name: "tool", input: "tool", wantErr: true},
|
||||||
|
{name: "function", input: "function", wantErr: true},
|
||||||
|
{name: "custom", input: "custom-role", wantErr: true},
|
||||||
|
{name: "invalid UTF-8", input: string([]byte{0xff}), wantErr: true},
|
||||||
|
}
|
||||||
|
|
||||||
|
for _, test := range tests {
|
||||||
|
t.Run(test.name, func(t *testing.T) {
|
||||||
|
got, err := NormalizeMessageRole(test.input)
|
||||||
|
if test.wantErr {
|
||||||
|
if err == nil {
|
||||||
|
t.Fatal("expected an error")
|
||||||
|
}
|
||||||
|
if strings.Contains(err.Error(), test.input) {
|
||||||
|
t.Fatalf("error exposed the input: %v", err)
|
||||||
|
}
|
||||||
|
return
|
||||||
|
}
|
||||||
|
if err != nil {
|
||||||
|
t.Fatalf("NormalizeMessageRole() error = %v", err)
|
||||||
|
}
|
||||||
|
if got != test.want {
|
||||||
|
t.Fatalf("NormalizeMessageRole() = %q, want %q", got, test.want)
|
||||||
|
}
|
||||||
|
})
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
func TestNormalizeCacheControl(t *testing.T) {
|
||||||
|
tests := []struct {
|
||||||
|
name string
|
||||||
|
input *CacheControl
|
||||||
|
want *CacheControl
|
||||||
|
wantErr bool
|
||||||
|
}{
|
||||||
|
{name: "nil", input: nil, want: nil},
|
||||||
|
{
|
||||||
|
name: "canonical values",
|
||||||
|
input: &CacheControl{Type: CacheControlEphemeral, TTL: "1h"},
|
||||||
|
want: &CacheControl{Type: CacheControlEphemeral, TTL: "1h"},
|
||||||
|
},
|
||||||
|
{
|
||||||
|
name: "trims values",
|
||||||
|
input: &CacheControl{Type: " ephemeral ", TTL: " 1h\t"},
|
||||||
|
want: &CacheControl{Type: CacheControlEphemeral, TTL: "1h"},
|
||||||
|
},
|
||||||
|
{name: "empty type", input: &CacheControl{}, wantErr: true},
|
||||||
|
{name: "unsupported type", input: &CacheControl{Type: "persistent"}, wantErr: true},
|
||||||
|
{name: "unsupported ttl", input: &CacheControl{Type: CacheControlEphemeral, TTL: "5m"}, wantErr: true},
|
||||||
|
{name: "invalid type UTF-8", input: &CacheControl{Type: CacheControlType(string([]byte{0xff}))}, wantErr: true},
|
||||||
|
{name: "invalid ttl UTF-8", input: &CacheControl{Type: CacheControlEphemeral, TTL: string([]byte{0xff})}, wantErr: true},
|
||||||
|
}
|
||||||
|
|
||||||
|
for _, test := range tests {
|
||||||
|
t.Run(test.name, func(t *testing.T) {
|
||||||
|
got, err := NormalizeCacheControl(test.input)
|
||||||
|
if test.wantErr {
|
||||||
|
if err == nil {
|
||||||
|
t.Fatal("expected an error")
|
||||||
|
}
|
||||||
|
return
|
||||||
|
}
|
||||||
|
if err != nil {
|
||||||
|
t.Fatalf("NormalizeCacheControl() error = %v", err)
|
||||||
|
}
|
||||||
|
if got == nil || test.want == nil {
|
||||||
|
if got != test.want {
|
||||||
|
t.Fatalf("NormalizeCacheControl() = %#v, want %#v", got, test.want)
|
||||||
|
}
|
||||||
|
return
|
||||||
|
}
|
||||||
|
if *got != *test.want {
|
||||||
|
t.Fatalf("NormalizeCacheControl() = %#v, want %#v", got, test.want)
|
||||||
|
}
|
||||||
|
if got == test.input {
|
||||||
|
t.Fatal("normalized cache control aliases its input")
|
||||||
|
}
|
||||||
|
})
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
func TestRenderedMessageCloning(t *testing.T) {
|
||||||
|
prefix := []RenderedMessage{{Role: RoleSystem, Content: "prefix", CacheControl: &CacheControl{Type: CacheControlEphemeral, TTL: "1h"}}}
|
||||||
|
suffix := []RenderedMessage{{Role: RoleUser, Content: " suffix "}, {Role: RoleAssistant, Content: "", CacheControl: &CacheControl{Type: CacheControlEphemeral}}}
|
||||||
|
|
||||||
|
cloned := CloneRenderedMessages(prefix)
|
||||||
|
combined := ConcatRenderedMessages(prefix, suffix)
|
||||||
|
if len(combined) != 3 || combined[0].Content != "prefix" || combined[1].Content != " suffix " || combined[2].Content != "" {
|
||||||
|
t.Fatalf("unexpected combined messages: %#v", combined)
|
||||||
|
}
|
||||||
|
if cloned[0].CacheControl == prefix[0].CacheControl || combined[0].CacheControl == prefix[0].CacheControl || combined[2].CacheControl == suffix[1].CacheControl {
|
||||||
|
t.Fatal("cloned cache controls alias their inputs")
|
||||||
|
}
|
||||||
|
|
||||||
|
prefix[0].Content = "changed"
|
||||||
|
prefix[0].CacheControl.TTL = ""
|
||||||
|
suffix[1].CacheControl.Type = "changed"
|
||||||
|
if cloned[0].Content != "prefix" || cloned[0].CacheControl.TTL != "1h" || combined[0].Content != "prefix" || combined[0].CacheControl.TTL != "1h" || combined[2].CacheControl.Type != CacheControlEphemeral {
|
||||||
|
t.Fatalf("cloned messages changed with their inputs: cloned=%#v combined=%#v", cloned, combined)
|
||||||
|
}
|
||||||
|
}
|
||||||
@@ -68,8 +68,9 @@ func (r *goRenderer) Render(ctx context.Context, definition *domain.PromptDefini
|
|||||||
return nil, err
|
return nil, err
|
||||||
}
|
}
|
||||||
|
|
||||||
if tmplMsg.Role == "" {
|
role, err := domain.NormalizeMessageRole(tmplMsg.Role)
|
||||||
return nil, fmt.Errorf("%w: message %d", ErrInvalidMessageRole, i)
|
if err != nil {
|
||||||
|
return nil, fmt.Errorf("%w: message %d: %v", ErrInvalidMessageRole, i, err)
|
||||||
}
|
}
|
||||||
|
|
||||||
if err := ctx.Err(); err != nil {
|
if err := ctx.Err(); err != nil {
|
||||||
@@ -96,7 +97,7 @@ func (r *goRenderer) Render(ctx context.Context, definition *domain.PromptDefini
|
|||||||
}
|
}
|
||||||
|
|
||||||
renderedMessages = append(renderedMessages, domain.RenderedMessage{
|
renderedMessages = append(renderedMessages, domain.RenderedMessage{
|
||||||
Role: tmplMsg.Role,
|
Role: role,
|
||||||
Content: buf.String(),
|
Content: buf.String(),
|
||||||
CacheControl: cloneCacheControl(tmplMsg.CacheControl),
|
CacheControl: cloneCacheControl(tmplMsg.CacheControl),
|
||||||
})
|
})
|
||||||
|
|||||||
@@ -361,6 +361,35 @@ func TestGoRenderer_Render(t *testing.T) {
|
|||||||
t.Fatalf("expected ErrInvalidMessageRole, got %v", err)
|
t.Fatalf("expected ErrInvalidMessageRole, got %v", err)
|
||||||
}
|
}
|
||||||
})
|
})
|
||||||
|
|
||||||
|
t.Run("canonicalizes directly supplied message roles", func(t *testing.T) {
|
||||||
|
def := &domain.PromptDefinition{
|
||||||
|
Templates: []domain.PromptMessageTemplate{
|
||||||
|
{Role: " \u2003DeVeLoPeR\u2003 ", Content: "Hello"},
|
||||||
|
},
|
||||||
|
}
|
||||||
|
res, err := renderer.Render(ctx, def, nil, vars)
|
||||||
|
if err != nil {
|
||||||
|
t.Fatalf("unexpected error: %v", err)
|
||||||
|
}
|
||||||
|
if got := res.Messages[0].Role; got != domain.RoleDeveloper {
|
||||||
|
t.Fatalf("rendered role = %q, want %q", got, domain.RoleDeveloper)
|
||||||
|
}
|
||||||
|
})
|
||||||
|
|
||||||
|
t.Run("rejects unsupported directly supplied message roles", func(t *testing.T) {
|
||||||
|
const unsupportedRole = "consumer-private-role"
|
||||||
|
def := &domain.PromptDefinition{
|
||||||
|
Templates: []domain.PromptMessageTemplate{{Role: unsupportedRole, Content: "Hello"}},
|
||||||
|
}
|
||||||
|
_, err := renderer.Render(ctx, def, nil, vars)
|
||||||
|
if !errors.Is(err, ErrInvalidMessageRole) {
|
||||||
|
t.Fatalf("expected ErrInvalidMessageRole, got %v", err)
|
||||||
|
}
|
||||||
|
if strings.Contains(err.Error(), unsupportedRole) {
|
||||||
|
t.Fatalf("renderer error exposed unsupported role: %v", err)
|
||||||
|
}
|
||||||
|
})
|
||||||
}
|
}
|
||||||
|
|
||||||
func TestGoRendererCancellation(t *testing.T) {
|
func TestGoRendererCancellation(t *testing.T) {
|
||||||
|
|||||||
@@ -254,20 +254,27 @@ func normalizePromptDefinitionWithContent(raw *promptDefinitionFile, readContent
|
|||||||
|
|
||||||
templates := make([]domain.PromptMessageTemplate, 0, len(raw.Messages))
|
templates := make([]domain.PromptMessageTemplate, 0, len(raw.Messages))
|
||||||
for i, msg := range raw.Messages {
|
for i, msg := range raw.Messages {
|
||||||
role := strings.TrimSpace(msg.Role)
|
role, err := domain.NormalizeMessageRole(msg.Role)
|
||||||
if role == "" {
|
if err != nil {
|
||||||
return nil, fmt.Errorf("message %d role is required", i)
|
return nil, fmt.Errorf("message %d role: %w", i, err)
|
||||||
}
|
}
|
||||||
|
|
||||||
hasContent := strings.TrimSpace(msg.Content) != ""
|
hasContent := strings.TrimSpace(msg.Content) != ""
|
||||||
hasContentFile := strings.TrimSpace(msg.ContentFile) != ""
|
hasContentFile := strings.TrimSpace(msg.ContentFile) != ""
|
||||||
if hasContent == hasContentFile {
|
if hasContent == hasContentFile {
|
||||||
return nil, fmt.Errorf("message %d (%s) must set exactly one of content or content_file", i, role)
|
return nil, fmt.Errorf("message %d must set exactly one of content or content_file", i)
|
||||||
}
|
}
|
||||||
|
|
||||||
cacheControl, err := normalizeCacheControl(msg.CacheControl)
|
var rawCacheControl *domain.CacheControl
|
||||||
|
if msg.CacheControl != nil {
|
||||||
|
rawCacheControl = &domain.CacheControl{
|
||||||
|
Type: domain.CacheControlType(msg.CacheControl.Type),
|
||||||
|
TTL: msg.CacheControl.TTL,
|
||||||
|
}
|
||||||
|
}
|
||||||
|
cacheControl, err := domain.NormalizeCacheControl(rawCacheControl)
|
||||||
if err != nil {
|
if err != nil {
|
||||||
return nil, fmt.Errorf("message %d (%s) cache_control: %w", i, role, err)
|
return nil, fmt.Errorf("message %d cache_control: %w", i, err)
|
||||||
}
|
}
|
||||||
|
|
||||||
templateContent := msg.Content
|
templateContent := msg.Content
|
||||||
@@ -275,7 +282,7 @@ func normalizePromptDefinitionWithContent(raw *promptDefinitionFile, readContent
|
|||||||
if hasContentFile {
|
if hasContentFile {
|
||||||
body, resolvedPath, err := readContentFile(msg.ContentFile)
|
body, resolvedPath, err := readContentFile(msg.ContentFile)
|
||||||
if err != nil {
|
if err != nil {
|
||||||
return nil, fmt.Errorf("prompt %q message %d (%s): failed to read content_file %q: %w", id, i, role, msg.ContentFile, err)
|
return nil, fmt.Errorf("prompt %q message %d: failed to read content_file %q: %w", id, i, msg.ContentFile, err)
|
||||||
}
|
}
|
||||||
templateContent = body
|
templateContent = body
|
||||||
resolvedContentFile = resolvedPath
|
resolvedContentFile = resolvedPath
|
||||||
@@ -319,27 +326,3 @@ func normalizePromptDefinitionWithContent(raw *promptDefinitionFile, readContent
|
|||||||
Validation: outputContract,
|
Validation: outputContract,
|
||||||
}, nil
|
}, nil
|
||||||
}
|
}
|
||||||
|
|
||||||
func normalizeCacheControl(raw *cacheControlFile) (*domain.CacheControl, error) {
|
|
||||||
if raw == nil {
|
|
||||||
return nil, nil
|
|
||||||
}
|
|
||||||
|
|
||||||
cacheType := strings.TrimSpace(raw.Type)
|
|
||||||
if cacheType == "" {
|
|
||||||
return nil, errors.New("type is required")
|
|
||||||
}
|
|
||||||
if domain.CacheControlType(cacheType) != domain.CacheControlEphemeral {
|
|
||||||
return nil, fmt.Errorf("unsupported type %q", cacheType)
|
|
||||||
}
|
|
||||||
|
|
||||||
ttl := strings.TrimSpace(raw.TTL)
|
|
||||||
if ttl != "" && ttl != "1h" {
|
|
||||||
return nil, fmt.Errorf("unsupported ttl %q", ttl)
|
|
||||||
}
|
|
||||||
|
|
||||||
return &domain.CacheControl{
|
|
||||||
Type: domain.CacheControlType(cacheType),
|
|
||||||
TTL: ttl,
|
|
||||||
}, nil
|
|
||||||
}
|
|
||||||
|
|||||||
@@ -1087,6 +1087,48 @@ output:
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
func TestPromptDefinitionMessageRoleNormalization(t *testing.T) {
|
||||||
|
const invalidRole = "consumer-private-role"
|
||||||
|
repo := NewFSRepository(fstest.MapFS{
|
||||||
|
"canonical.yaml": {Data: []byte(`
|
||||||
|
id: canonical
|
||||||
|
version: "1"
|
||||||
|
messages:
|
||||||
|
- role: " \u2003SyStEm\u2003 "
|
||||||
|
content: test
|
||||||
|
output:
|
||||||
|
format: text
|
||||||
|
validation_mode: none
|
||||||
|
`)},
|
||||||
|
"invalid.yaml": {Data: []byte(`
|
||||||
|
id: invalid-role
|
||||||
|
version: "1"
|
||||||
|
messages:
|
||||||
|
- role: consumer-private-role
|
||||||
|
content: test
|
||||||
|
output:
|
||||||
|
format: text
|
||||||
|
validation_mode: none
|
||||||
|
`)},
|
||||||
|
}, ".")
|
||||||
|
|
||||||
|
definition, err := repo.GetPromptDefinition(context.Background(), "canonical", "")
|
||||||
|
if err != nil {
|
||||||
|
t.Fatalf("GetPromptDefinition() error = %v", err)
|
||||||
|
}
|
||||||
|
if got := definition.Templates[0].Role; got != domain.RoleSystem {
|
||||||
|
t.Fatalf("normalized role = %q, want %q", got, domain.RoleSystem)
|
||||||
|
}
|
||||||
|
|
||||||
|
_, err = repo.GetPromptDefinition(context.Background(), "invalid-role", "")
|
||||||
|
if !errors.Is(err, ErrInvalidPromptDefinition) {
|
||||||
|
t.Fatalf("expected ErrInvalidPromptDefinition, got %v", err)
|
||||||
|
}
|
||||||
|
if strings.Contains(err.Error(), invalidRole) {
|
||||||
|
t.Fatalf("invalid prompt error exposed the supplied role: %v", err)
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
func assertCacheControl(t *testing.T, got *domain.CacheControl, wantType domain.CacheControlType, wantTTL string) {
|
func assertCacheControl(t *testing.T, got *domain.CacheControl, wantType domain.CacheControlType, wantTTL string) {
|
||||||
t.Helper()
|
t.Helper()
|
||||||
if got == nil {
|
if got == nil {
|
||||||
|
|||||||
@@ -59,7 +59,7 @@ func (r *defaultOutputRepairer) Repair(ctx context.Context, req RepairRequest) (
|
|||||||
copy(messages, req.OriginalMessages)
|
copy(messages, req.OriginalMessages)
|
||||||
if strings.TrimSpace(req.PreviousOutput) != "" {
|
if strings.TrimSpace(req.PreviousOutput) != "" {
|
||||||
messages = append(messages, domain.RenderedMessage{
|
messages = append(messages, domain.RenderedMessage{
|
||||||
Role: "assistant",
|
Role: domain.RoleAssistant,
|
||||||
Content: req.PreviousOutput,
|
Content: req.PreviousOutput,
|
||||||
})
|
})
|
||||||
}
|
}
|
||||||
@@ -69,7 +69,7 @@ func (r *defaultOutputRepairer) Repair(ctx context.Context, req RepairRequest) (
|
|||||||
previousResponse = "The previous response is included immediately before this instruction."
|
previousResponse = "The previous response is included immediately before this instruction."
|
||||||
}
|
}
|
||||||
messages = append(messages, domain.RenderedMessage{
|
messages = append(messages, domain.RenderedMessage{
|
||||||
Role: "user",
|
Role: domain.RoleUser,
|
||||||
Content: fmt.Sprintf(
|
Content: fmt.Sprintf(
|
||||||
"Repair attempt %d of %d for validation mode %s.\n"+
|
"Repair attempt %d of %d for validation mode %s.\n"+
|
||||||
"Preserve valid values and change only what is necessary.\n"+
|
"Preserve valid values and change only what is necessary.\n"+
|
||||||
|
|||||||
Reference in New Issue
Block a user