From 5064cf833da7d7c5fcc451ed65fd56f984e636c6 Mon Sep 17 00:00:00 2001 From: Eric Rakestraw Date: Wed, 26 Aug 2026 02:07:15 +0000 Subject: [PATCH] Centralize message role invariants --- docs/roadmap/implementation.md | 2 + internal/domain/message.go | 78 +++++++++++++ internal/domain/message_test.go | 121 ++++++++++++++++++++ internal/prompt/go_renderer.go | 7 +- internal/prompt/renderer_test.go | 29 +++++ internal/promptdef/filesystem_repository.go | 45 +++----- internal/promptdef/repository_test.go | 42 +++++++ internal/usecase/repairer.go | 4 +- 8 files changed, 292 insertions(+), 36 deletions(-) create mode 100644 internal/domain/message.go create mode 100644 internal/domain/message_test.go diff --git a/docs/roadmap/implementation.md b/docs/roadmap/implementation.md index 1528c42..b0288fa 100644 --- a/docs/roadmap/implementation.md +++ b/docs/roadmap/implementation.md @@ -54,6 +54,8 @@ Apply these constraints throughout every stage: ## Stage 1: Centralize Message Invariants And Tighten Prompt Roles +**Status:** Complete + ### Objective Establish one source-neutral owner for chat-message role and cache-control diff --git a/internal/domain/message.go b/internal/domain/message.go new file mode 100644 index 0000000..62533d4 --- /dev/null +++ b/internal/domain/message.go @@ -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 +} diff --git a/internal/domain/message_test.go b/internal/domain/message_test.go new file mode 100644 index 0000000..e8374b4 --- /dev/null +++ b/internal/domain/message_test.go @@ -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) + } +} diff --git a/internal/prompt/go_renderer.go b/internal/prompt/go_renderer.go index be67c76..a49861c 100644 --- a/internal/prompt/go_renderer.go +++ b/internal/prompt/go_renderer.go @@ -68,8 +68,9 @@ func (r *goRenderer) Render(ctx context.Context, definition *domain.PromptDefini return nil, err } - if tmplMsg.Role == "" { - return nil, fmt.Errorf("%w: message %d", ErrInvalidMessageRole, i) + role, err := domain.NormalizeMessageRole(tmplMsg.Role) + if err != nil { + return nil, fmt.Errorf("%w: message %d: %v", ErrInvalidMessageRole, i, err) } 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{ - Role: tmplMsg.Role, + Role: role, Content: buf.String(), CacheControl: cloneCacheControl(tmplMsg.CacheControl), }) diff --git a/internal/prompt/renderer_test.go b/internal/prompt/renderer_test.go index 43a0eff..08937ab 100644 --- a/internal/prompt/renderer_test.go +++ b/internal/prompt/renderer_test.go @@ -361,6 +361,35 @@ func TestGoRenderer_Render(t *testing.T) { 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) { diff --git a/internal/promptdef/filesystem_repository.go b/internal/promptdef/filesystem_repository.go index 8e6a828..4080ff8 100644 --- a/internal/promptdef/filesystem_repository.go +++ b/internal/promptdef/filesystem_repository.go @@ -254,20 +254,27 @@ func normalizePromptDefinitionWithContent(raw *promptDefinitionFile, readContent templates := make([]domain.PromptMessageTemplate, 0, len(raw.Messages)) for i, msg := range raw.Messages { - role := strings.TrimSpace(msg.Role) - if role == "" { - return nil, fmt.Errorf("message %d role is required", i) + role, err := domain.NormalizeMessageRole(msg.Role) + if err != nil { + return nil, fmt.Errorf("message %d role: %w", i, err) } hasContent := strings.TrimSpace(msg.Content) != "" hasContentFile := strings.TrimSpace(msg.ContentFile) != "" 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 { - 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 @@ -275,7 +282,7 @@ func normalizePromptDefinitionWithContent(raw *promptDefinitionFile, readContent if hasContentFile { body, resolvedPath, err := readContentFile(msg.ContentFile) 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 resolvedContentFile = resolvedPath @@ -319,27 +326,3 @@ func normalizePromptDefinitionWithContent(raw *promptDefinitionFile, readContent Validation: outputContract, }, 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 -} diff --git a/internal/promptdef/repository_test.go b/internal/promptdef/repository_test.go index 2b2709a..c332d95 100644 --- a/internal/promptdef/repository_test.go +++ b/internal/promptdef/repository_test.go @@ -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) { t.Helper() if got == nil { diff --git a/internal/usecase/repairer.go b/internal/usecase/repairer.go index b0ed15c..1f19b85 100644 --- a/internal/usecase/repairer.go +++ b/internal/usecase/repairer.go @@ -59,7 +59,7 @@ func (r *defaultOutputRepairer) Repair(ctx context.Context, req RepairRequest) ( copy(messages, req.OriginalMessages) if strings.TrimSpace(req.PreviousOutput) != "" { messages = append(messages, domain.RenderedMessage{ - Role: "assistant", + Role: domain.RoleAssistant, 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." } messages = append(messages, domain.RenderedMessage{ - Role: "user", + Role: domain.RoleUser, Content: fmt.Sprintf( "Repair attempt %d of %d for validation mode %s.\n"+ "Preserve valid values and change only what is necessary.\n"+