From 8cfc71c351daef9c1e375956808f953bd9b578bd Mon Sep 17 00:00:00 2001 From: Eric Rakestraw Date: Tue, 11 Aug 2026 21:12:56 +0000 Subject: [PATCH] Centralize execution setting and session validation --- docs/internal/llm.md | 2 +- docs/internal/overview.md | 2 +- docs/policy/architecture.md | 10 ++- engine_test.go | 30 ++++++++ internal/domain/execution_settings.go | 34 +++++++++ internal/domain/execution_settings_test.go | 73 +++++++++++++++++++ internal/domain/session.go | 3 + internal/domain/session_test.go | 23 +++--- internal/llm/openai_compatible_client.go | 4 +- internal/llm/openai_compatible_client_test.go | 38 +++++++--- internal/profile/filesystem_repository.go | 20 ++--- internal/profile/repository_test.go | 35 +++++++++ internal/prompt/renderer_test.go | 18 +++++ internal/usecase/execution_settings_test.go | 33 +++++++++ internal/usecase/profile_inspection.go | 7 +- internal/usecase/runner.go | 31 ++------ internal/usecase/runner_test.go | 67 +++++++++++------ profiles.go | 19 ++--- 18 files changed, 345 insertions(+), 104 deletions(-) create mode 100644 internal/domain/execution_settings.go create mode 100644 internal/domain/execution_settings_test.go create mode 100644 internal/usecase/execution_settings_test.go diff --git a/docs/internal/llm.md b/docs/internal/llm.md index 913e1f5..75cab78 100644 --- a/docs/internal/llm.md +++ b/docs/internal/llm.md @@ -29,7 +29,7 @@ Construction validates the configured base URL and clones any supplied `http.Client` so Promptkit can apply its timeout default without mutating the caller's client. Generation then: -1. validates request-level timeout and endpoint requirements; +1. validates shared execution-setting invariants and endpoint requirements; 2. maps the internal request into the OpenAI-compatible chat payload; 3. validates and merges extra parameters; 4. resolves authentication; diff --git a/docs/internal/overview.md b/docs/internal/overview.md index 23401a8..05105d7 100644 --- a/docs/internal/overview.md +++ b/docs/internal/overview.md @@ -16,7 +16,7 @@ contributor workflow and validation. | `examples/go-library/run` | Demonstrates an offline downstream consumer using a prompt file, in-memory profile, inline input, an injected deterministic model client, and `Run`. It is not a public library package. | [Example program](../../examples/go-library/run/main.go) | | `internal/backend` | Constructs each engine's immutable registry from the built-in OpenRouter definition and consumer additions, validates and defensively copies definitions through the shared JSON-value package, and consumes the LLM-owned OpenAI-compatible reserved request-field rule. | [Backend registry](../../internal/backend/registry.go) | | `internal/capacity` | Owns engine-local bounded execution admission and FIFO model-generation permits for limited backend IDs, including cancellation-safe waiter removal and client wrapping. | [Internal capacity management](capacity.md) | -| `internal/domain` | Defines internal framework values for requests, artifacts, prompt definitions, profiles, execution targets, rendering, generation, and validation. | [Domain declarations](../../internal/domain/domain.go) | +| `internal/domain` | Defines internal framework values for requests, artifacts, prompt definitions, profiles, execution targets, rendering, generation, and validation, and owns source-neutral invariants for shared execution settings and session identifiers. Source parsing, required fields, source-specific normalization, and boundary-specific error classification remain with their callers. | [Domain declarations](../../internal/domain/domain.go) | | `internal/defaults` | Defines application-neutral framework constants and constructs the default execution target. It contains no CLI, server, or inbound HTTP limits. | [Framework defaults](../../internal/defaults/defaults.go) | | `internal/filecatalog` | Provides deterministic YAML discovery and path helpers for operating-system filesystems and `fs.FS` sources. | [File catalog](../../internal/filecatalog/catalog.go) | | `internal/jsonvalue` | Validates and deeply copies JSON-compatible extra-parameter and prepared-schema trees while preserving supported concrete value types. | [JSON values](../../internal/jsonvalue/jsonvalue.go) | diff --git a/docs/policy/architecture.md b/docs/policy/architecture.md index bc79af1..0b5b1ad 100644 --- a/docs/policy/architecture.md +++ b/docs/policy/architecture.md @@ -19,8 +19,8 @@ results, public values, extension interfaces, profiles, and error sentinels. The implemented internal components consist of: -- `internal/domain`, which owns framework data values shared by later internal - components; +- `internal/domain`, which owns framework data values and source-neutral + invariants shared by later internal components; - `internal/backend`, which owns validated immutable OpenAI-compatible backend definitions and the built-in OpenRouter definition; - `internal/capacity`, which owns engine-local bounded run admission and @@ -92,6 +92,12 @@ coordinates internal components and adapts the supported public extension interfaces to narrow internal abstractions. Internal components must not depend on consumers or on Scriptorium. +`internal/domain` owns source-neutral invariants for values shared across +multiple input and execution boundaries, including execution-setting bounds +and session identifiers. Callers retain source parsing, required-field rules, +source-specific normalization, error classification, and other policy specific +to their own boundary. + ## Repository And Consumer Boundary Scriptorium is a downstream application that consumes Promptkit through diff --git a/engine_test.go b/engine_test.go index 5d91bca..e3f7cb2 100644 --- a/engine_test.go +++ b/engine_test.go @@ -12,6 +12,7 @@ import ( "os" "path/filepath" "reflect" + "strconv" "strings" "testing" "testing/fstest" @@ -1792,6 +1793,35 @@ func TestWithProfilesRejectsDuplicateIDs(t *testing.T) { } } +func TestWithProfilesRejectsInvalidExecutionSettings(t *testing.T) { + type testCase struct { + name string + profile promptkit.Profile + } + tests := []testCase{ + {name: "non-finite temperature", profile: promptkit.Profile{Temperature: math.NaN()}}, + {name: "non-finite top p", profile: promptkit.Profile{TopP: math.Inf(-1)}}, + } + if strconv.IntSize == 64 { + durationLimit := int64(math.MaxInt64 / int64(time.Second)) + tests = append(tests, testCase{name: "unrepresentable timeout", profile: promptkit.Profile{TimeoutSeconds: int(durationLimit) + 1}}) + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + tt.profile.ID = "invalid-settings" + tt.profile.Endpoint = "http://example.test/v1" + tt.profile.Model = "model" + _, err := promptkit.NewEngine(promptkit.Config{PromptDir: frameworkPromptDir}, + promptkit.WithProfiles(tt.profile), + ) + if !errors.Is(err, promptkit.ErrInvalidConfig) { + t.Fatalf("expected ErrInvalidConfig, got %v", err) + } + }) + } +} + func TestOpenAICompatibleProfileRunsThroughNormalProfilePath(t *testing.T) { fake := &fakeLLMClient{response: &promptkit.GenerateResponse{Content: "ok"}} prof := promptkit.OpenAICompatibleProfile(promptkit.OpenAICompatibleProfileConfig{ diff --git a/internal/domain/execution_settings.go b/internal/domain/execution_settings.go new file mode 100644 index 0000000..214522b --- /dev/null +++ b/internal/domain/execution_settings.go @@ -0,0 +1,34 @@ +package domain + +import ( + "errors" + "math" + "time" +) + +const maxExecutionTimeoutSeconds int64 = math.MaxInt64 / int64(time.Second) + +// ValidateExecutionTargetSettings validates source-neutral execution-setting +// invariants on a resolved target. +func ValidateExecutionTargetSettings(target ExecutionTarget) error { + if !isFinite(target.Temperature) || target.Temperature < 0 || target.Temperature > 2 { + return errors.New("temperature must be finite and between 0 and 2") + } + if target.MaxTokens < 0 { + return errors.New("max_tokens must be greater than or equal to 0") + } + if !isFinite(target.TopP) || target.TopP < 0 || target.TopP > 1 { + return errors.New("top_p must be finite and between 0 and 1") + } + if target.TimeoutSeconds < 0 { + return errors.New("timeout_seconds must be greater than or equal to 0") + } + if int64(target.TimeoutSeconds) > maxExecutionTimeoutSeconds { + return errors.New("timeout_seconds exceeds the maximum supported duration") + } + return nil +} + +func isFinite(value float64) bool { + return !math.IsNaN(value) && !math.IsInf(value, 0) +} diff --git a/internal/domain/execution_settings_test.go b/internal/domain/execution_settings_test.go new file mode 100644 index 0000000..f1d3fb7 --- /dev/null +++ b/internal/domain/execution_settings_test.go @@ -0,0 +1,73 @@ +package domain + +import ( + "math" + "strconv" + "strings" + "testing" +) + +func TestValidateExecutionTargetSettings(t *testing.T) { + valid := ExecutionTarget{ + Temperature: 1, + MaxTokens: 1, + TopP: 0.5, + TimeoutSeconds: 1, + } + + type testCase struct { + name string + change func(*ExecutionTarget) + wantErr string + } + tests := []testCase{ + {name: "temperature lower boundary", change: func(v *ExecutionTarget) { v.Temperature = 0 }}, + {name: "temperature finite lower neighbor", change: func(v *ExecutionTarget) { v.Temperature = math.Nextafter(0, 1) }}, + {name: "temperature finite upper neighbor", change: func(v *ExecutionTarget) { v.Temperature = math.Nextafter(2, 0) }}, + {name: "temperature upper boundary", change: func(v *ExecutionTarget) { v.Temperature = 2 }}, + {name: "temperature below lower boundary", change: func(v *ExecutionTarget) { v.Temperature = math.Nextafter(0, math.Inf(-1)) }, wantErr: "temperature"}, + {name: "temperature above upper boundary", change: func(v *ExecutionTarget) { v.Temperature = math.Nextafter(2, math.Inf(1)) }, wantErr: "temperature"}, + {name: "temperature NaN", change: func(v *ExecutionTarget) { v.Temperature = math.NaN() }, wantErr: "temperature"}, + {name: "temperature positive infinity", change: func(v *ExecutionTarget) { v.Temperature = math.Inf(1) }, wantErr: "temperature"}, + {name: "temperature negative infinity", change: func(v *ExecutionTarget) { v.Temperature = math.Inf(-1) }, wantErr: "temperature"}, + {name: "max tokens lower boundary", change: func(v *ExecutionTarget) { v.MaxTokens = 0 }}, + {name: "max tokens finite neighbor", change: func(v *ExecutionTarget) { v.MaxTokens = 1 }}, + {name: "max tokens below lower boundary", change: func(v *ExecutionTarget) { v.MaxTokens = -1 }, wantErr: "max_tokens"}, + {name: "top p lower boundary", change: func(v *ExecutionTarget) { v.TopP = 0 }}, + {name: "top p finite lower neighbor", change: func(v *ExecutionTarget) { v.TopP = math.Nextafter(0, 1) }}, + {name: "top p finite upper neighbor", change: func(v *ExecutionTarget) { v.TopP = math.Nextafter(1, 0) }}, + {name: "top p upper boundary", change: func(v *ExecutionTarget) { v.TopP = 1 }}, + {name: "top p below lower boundary", change: func(v *ExecutionTarget) { v.TopP = math.Nextafter(0, math.Inf(-1)) }, wantErr: "top_p"}, + {name: "top p above upper boundary", change: func(v *ExecutionTarget) { v.TopP = math.Nextafter(1, math.Inf(1)) }, wantErr: "top_p"}, + {name: "top p NaN", change: func(v *ExecutionTarget) { v.TopP = math.NaN() }, wantErr: "top_p"}, + {name: "top p positive infinity", change: func(v *ExecutionTarget) { v.TopP = math.Inf(1) }, wantErr: "top_p"}, + {name: "top p negative infinity", change: func(v *ExecutionTarget) { v.TopP = math.Inf(-1) }, wantErr: "top_p"}, + {name: "timeout lower boundary", change: func(v *ExecutionTarget) { v.TimeoutSeconds = 0 }}, + {name: "timeout finite neighbor", change: func(v *ExecutionTarget) { v.TimeoutSeconds = 1 }}, + {name: "timeout below lower boundary", change: func(v *ExecutionTarget) { v.TimeoutSeconds = -1 }, wantErr: "timeout_seconds"}, + } + if strconv.IntSize == 64 { + durationLimit := maxExecutionTimeoutSeconds + tests = append(tests, + testCase{name: "timeout duration boundary", change: func(v *ExecutionTarget) { v.TimeoutSeconds = int(durationLimit) }}, + testCase{name: "timeout above duration boundary", change: func(v *ExecutionTarget) { v.TimeoutSeconds = int(durationLimit) + 1 }, wantErr: "timeout_seconds"}, + ) + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + target := valid + tt.change(&target) + err := ValidateExecutionTargetSettings(target) + if tt.wantErr == "" { + if err != nil { + t.Fatalf("validate execution settings: %v", err) + } + return + } + if err == nil || !strings.Contains(err.Error(), tt.wantErr) { + t.Fatalf("error = %v, want diagnostic containing %q", err, tt.wantErr) + } + }) + } +} diff --git a/internal/domain/session.go b/internal/domain/session.go index fbd4122..435db87 100644 --- a/internal/domain/session.go +++ b/internal/domain/session.go @@ -8,6 +8,9 @@ import ( // NormalizeSessionID applies the shared session identifier rule. func NormalizeSessionID(raw string) (string, error) { + if !utf8.ValidString(raw) { + return "", fmt.Errorf("session_id must contain valid UTF-8") + } normalized := strings.TrimSpace(raw) if normalized == "" { return "", nil diff --git a/internal/domain/session_test.go b/internal/domain/session_test.go index 2c3173f..fa515fd 100644 --- a/internal/domain/session_test.go +++ b/internal/domain/session_test.go @@ -7,10 +7,10 @@ import ( func TestNormalizeSessionID(t *testing.T) { tests := []struct { - name string - raw string - want string - wantErr bool + name string + raw string + want string + wantErrContains string }{ { name: "trims surrounding Unicode whitespace", @@ -28,21 +28,24 @@ func TestNormalizeSessionID(t *testing.T) { want: strings.Repeat("界", SessionIDMaxLength), }, { - name: "one Unicode code point over maximum is rejected", - raw: strings.Repeat("界", SessionIDMaxLength+1), - wantErr: true, + name: "one Unicode code point over maximum is rejected", + raw: strings.Repeat("界", SessionIDMaxLength+1), + wantErrContains: "exceeds maximum", }, + {name: "invalid UTF-8 before valid content", raw: string([]byte{0xff}) + "session", wantErrContains: "valid UTF-8"}, + {name: "invalid UTF-8 within valid content", raw: "ses" + string([]byte{0xff}) + "sion", wantErrContains: "valid UTF-8"}, + {name: "invalid UTF-8 after valid content", raw: "session" + string([]byte{0xff}), wantErrContains: "valid UTF-8"}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { got, err := NormalizeSessionID(tt.raw) - if tt.wantErr { + if tt.wantErrContains != "" { if err == nil { t.Fatal("expected normalization error") } - if !strings.Contains(err.Error(), "exceeds maximum") { - t.Fatalf("expected useful length diagnostic, got %v", err) + if !strings.Contains(err.Error(), tt.wantErrContains) { + t.Fatalf("expected diagnostic containing %q, got %v", tt.wantErrContains, err) } return } diff --git a/internal/llm/openai_compatible_client.go b/internal/llm/openai_compatible_client.go index cbd3642..788c300 100644 --- a/internal/llm/openai_compatible_client.go +++ b/internal/llm/openai_compatible_client.go @@ -70,8 +70,8 @@ func NewOpenAICompatibleClient(cfg OpenAICompatibleConfig) (*OpenAICompatibleCli } func (c *OpenAICompatibleClient) Generate(ctx context.Context, req domain.GenerateRequest) (*domain.GenerateResponse, error) { - if req.Target.TimeoutSeconds < 0 { - return nil, fmt.Errorf("%w: timeout_seconds must be greater than or equal to 0", ErrInvalidRequest) + if err := domain.ValidateExecutionTargetSettings(req.Target); err != nil { + return nil, fmt.Errorf("%w: %v", ErrInvalidRequest, err) } endpoint := strings.TrimSpace(req.Target.Endpoint) diff --git a/internal/llm/openai_compatible_client_test.go b/internal/llm/openai_compatible_client_test.go index 54ce938..b28e780 100644 --- a/internal/llm/openai_compatible_client_test.go +++ b/internal/llm/openai_compatible_client_test.go @@ -7,6 +7,7 @@ import ( "math" "net/http" "net/http/httptest" + "strconv" "strings" "testing" "time" @@ -1128,7 +1129,7 @@ func TestOpenAICompatibleClientCancellationReturnsRequestFailure(t *testing.T) { } } -func TestOpenAICompatibleClientNegativeTimeoutRejected(t *testing.T) { +func TestOpenAICompatibleClientRejectsInvalidExecutionSettings(t *testing.T) { client, err := NewOpenAICompatibleClient(OpenAICompatibleConfig{ BaseURL: "http://example.com/v1", Model: "m", @@ -1137,15 +1138,34 @@ func TestOpenAICompatibleClientNegativeTimeoutRejected(t *testing.T) { t.Fatal(err) } - _, err = client.Generate(context.Background(), domain.GenerateRequest{ - Prompt: domain.RenderedPrompt{Messages: []domain.RenderedMessage{{Role: "user", Content: "hi"}}}, - Target: domain.ExecutionTarget{TimeoutSeconds: -1}, - }) - if err == nil { - t.Fatal("expected invalid request error") + type testCase struct { + name string + target domain.ExecutionTarget } - if !errors.Is(err, ErrInvalidRequest) { - t.Fatalf("expected ErrInvalidRequest, got %v", err) + tests := []testCase{ + {name: "non-finite temperature", target: domain.ExecutionTarget{Temperature: math.NaN()}}, + {name: "negative max tokens", target: domain.ExecutionTarget{MaxTokens: -1}}, + {name: "non-finite top p", target: domain.ExecutionTarget{TopP: math.Inf(1)}}, + {name: "negative timeout", target: domain.ExecutionTarget{TimeoutSeconds: -1}}, + } + if strconv.IntSize == 64 { + durationLimit := int64(math.MaxInt64 / int64(time.Second)) + tests = append(tests, testCase{name: "unrepresentable timeout", target: domain.ExecutionTarget{TimeoutSeconds: int(durationLimit) + 1}}) + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + _, err := client.Generate(context.Background(), domain.GenerateRequest{ + Prompt: domain.RenderedPrompt{Messages: []domain.RenderedMessage{{Role: "user", Content: "hi"}}}, + Target: tt.target, + }) + if err == nil { + t.Fatal("expected invalid request error") + } + if !errors.Is(err, ErrInvalidRequest) { + t.Fatalf("expected ErrInvalidRequest, got %v", err) + } + }) } } diff --git a/internal/profile/filesystem_repository.go b/internal/profile/filesystem_repository.go index a3f731c..a186012 100644 --- a/internal/profile/filesystem_repository.go +++ b/internal/profile/filesystem_repository.go @@ -197,18 +197,10 @@ func validateProfile(p *domain.ExecutionProfile) error { return errors.New("model is required") } - if p.Temperature < 0 || p.Temperature > 2 { - return errors.New("temperature must be between 0 and 2") - } - if p.MaxTokens < 0 { - return errors.New("max_tokens must be greater than or equal to 0") - } - if p.TopP < 0 || p.TopP > 1 { - return errors.New("top_p must be between 0 and 1") - } - if p.TimeoutSeconds < 0 { - return errors.New("timeout_seconds must be greater than or equal to 0") - } - - return nil + return domain.ValidateExecutionTargetSettings(domain.ExecutionTarget{ + Temperature: p.Temperature, + MaxTokens: p.MaxTokens, + TopP: p.TopP, + TimeoutSeconds: p.TimeoutSeconds, + }) } diff --git a/internal/profile/repository_test.go b/internal/profile/repository_test.go index 97b70a1..98523b6 100644 --- a/internal/profile/repository_test.go +++ b/internal/profile/repository_test.go @@ -406,6 +406,41 @@ model: second }) } +func TestProfileRepositoriesRejectInvalidExecutionSettings(t *testing.T) { + ctx := context.Background() + + t.Run("operating-system filesystem", func(t *testing.T) { + dir := t.TempDir() + writeProfileTestFile(t, filepath.Join(dir, "invalid.yaml"), ` +id: invalid +endpoint: http://localhost:8000/v1 +model: model +temperature: .nan +`) + + _, err := NewFilesystemRepository(dir).GetProfile(ctx, "invalid") + if !errors.Is(err, ErrInvalidProfile) { + t.Fatalf("expected ErrInvalidProfile, got %v", err) + } + }) + + t.Run("fs.FS", func(t *testing.T) { + repo := NewFSRepository(fstest.MapFS{ + "profiles/invalid.yaml": profileMapFile(` +id: invalid +endpoint: http://localhost:8000/v1 +model: model +top_p: .inf +`), + }, "profiles") + + _, err := repo.GetProfile(ctx, "invalid") + if !errors.Is(err, ErrInvalidProfile) { + t.Fatalf("expected ErrInvalidProfile, got %v", err) + } + }) +} + func TestOverlayRepository(t *testing.T) { ctx := context.Background() primaryProfile := &domain.ExecutionProfile{ID: "shared", Endpoint: "http://primary", Model: "primary"} diff --git a/internal/prompt/renderer_test.go b/internal/prompt/renderer_test.go index 414fcf4..d8e53b0 100644 --- a/internal/prompt/renderer_test.go +++ b/internal/prompt/renderer_test.go @@ -228,6 +228,24 @@ func TestGoRenderer_Render(t *testing.T) { } }) + t.Run("malformed rendered session id fails rendering", func(t *testing.T) { + def := &domain.PromptDefinition{ + SessionID: "{{ .session_id }}", + Inputs: []domain.PromptInput{{Name: "transcript", Required: true}}, + Templates: []domain.PromptMessageTemplate{ + {Role: "system", Content: "Speak in a {{.tone}} tone."}, + }, + } + + _, err := renderer.Render(ctx, def, inputs, map[string]string{ + "tone": "concise", + "session_id": "session" + string([]byte{0xff}), + }) + if !errors.Is(err, ErrRenderFailure) { + t.Fatalf("expected ErrRenderFailure, got %v", err) + } + }) + t.Run("inserting required input artifact", func(t *testing.T) { def := &domain.PromptDefinition{ Inputs: []domain.PromptInput{{Name: "transcript", Required: true}}, diff --git a/internal/usecase/execution_settings_test.go b/internal/usecase/execution_settings_test.go new file mode 100644 index 0000000..a0bc7d7 --- /dev/null +++ b/internal/usecase/execution_settings_test.go @@ -0,0 +1,33 @@ +package usecase + +import ( + "context" + "errors" + "math" + "testing" + + "gitea.maximumdirect.net/eric/promptkit/internal/domain" +) + +func TestRunnerPrepareExecutionRejectsInvalidExecutionSettings(t *testing.T) { + runner := NewRunner( + &fakePromptRepo{def: promptDef(domain.FormatText, domain.ValidationNone, 0)}, + &fakeExecutionProfileRepo{profiles: map[string]*domain.ExecutionProfile{"exec": defaultExecutionProfile()}}, + nil, + defaultArtifactReader(), + defaultRenderer(), + &fakeLLM{forbid: true}, + nil, + nil, + ) + + _, err := runner.PrepareExecution(context.Background(), domain.RunRequest{ + PromptID: "p", + ProfileID: "exec", + Inputs: singleInputRef(), + Execution: &domain.ExecutionTargetOverride{TopP: float64Ptr(math.Inf(-1))}, + }) + if !errors.Is(err, ErrInvalidRequest) { + t.Fatalf("expected ErrInvalidRequest, got %v", err) + } +} diff --git a/internal/usecase/profile_inspection.go b/internal/usecase/profile_inspection.go index c46eafc..f3ddd86 100644 --- a/internal/usecase/profile_inspection.go +++ b/internal/usecase/profile_inspection.go @@ -64,7 +64,7 @@ func validateResolvedExecutionTarget(target domain.ExecutionTarget) error { if strings.TrimSpace(target.Model) == "" { return errors.New("execution model is required") } - return nil + return domain.ValidateExecutionTargetSettings(target) } // InspectProfile resolves one explicit profile without prompt or execution work. @@ -86,10 +86,7 @@ func (r *Runner) InspectProfile( if err != nil { return nil, err } - target, _, err := resolveExecutionTarget(selection.backend, selection.profile, nil) - if err != nil { - return nil, fmt.Errorf("%w: %w", ErrProfileLoad, err) - } + target, _ := resolveExecutionTarget(selection.backend, selection.profile, nil) if err := validateResolvedExecutionTarget(target); err != nil { return nil, fmt.Errorf("%w: %w", ErrProfileLoad, err) } diff --git a/internal/usecase/runner.go b/internal/usecase/runner.go index 7ef35c2..2466742 100644 --- a/internal/usecase/runner.go +++ b/internal/usecase/runner.go @@ -286,10 +286,7 @@ func (r *Runner) resolvePreparation( return nil, err } - effectiveModel, targetPresence, err := resolveExecutionTarget(selection.backend, selection.profile, req.Execution) - if err != nil { - return nil, fmt.Errorf("%w: %w", ErrInvalidRequest, err) - } + effectiveModel, targetPresence := resolveExecutionTarget(selection.backend, selection.profile, req.Execution) effectiveModel.APIKey = req.APIKey if err := validateResolvedExecutionTarget(effectiveModel); err != nil { return nil, fmt.Errorf("%w: %w", ErrInvalidRequest, err) @@ -527,7 +524,7 @@ func mergeExecutionTarget(base domain.ExecutionTarget, override domain.Execution return out } -func mergeExecutionTargetOverride(base domain.ExecutionTarget, override domain.ExecutionTargetOverride) (domain.ExecutionTarget, domain.ExecutionTargetPresence, error) { +func mergeExecutionTargetOverride(base domain.ExecutionTarget, override domain.ExecutionTargetOverride) (domain.ExecutionTarget, domain.ExecutionTargetPresence) { out := base var presence domain.ExecutionTargetPresence if override.Endpoint != "" { @@ -537,30 +534,18 @@ func mergeExecutionTargetOverride(base domain.ExecutionTarget, override domain.E out.Model = override.Model } if override.Temperature != nil { - if *override.Temperature < 0 || *override.Temperature > 2 { - return domain.ExecutionTarget{}, domain.ExecutionTargetPresence{}, errors.New("temperature must be between 0 and 2") - } out.Temperature = *override.Temperature presence.Temperature = true } if override.MaxTokens != nil { - if *override.MaxTokens < 0 { - return domain.ExecutionTarget{}, domain.ExecutionTargetPresence{}, errors.New("max_tokens must be greater than or equal to 0") - } out.MaxTokens = *override.MaxTokens presence.MaxTokens = true } if override.TopP != nil { - if *override.TopP < 0 || *override.TopP > 1 { - return domain.ExecutionTarget{}, domain.ExecutionTargetPresence{}, errors.New("top_p must be between 0 and 1") - } out.TopP = *override.TopP presence.TopP = true } if override.TimeoutSeconds != nil { - if *override.TimeoutSeconds < 0 { - return domain.ExecutionTarget{}, domain.ExecutionTargetPresence{}, errors.New("timeout_seconds must be greater than or equal to 0") - } out.TimeoutSeconds = *override.TimeoutSeconds presence.TimeoutSeconds = true } @@ -576,22 +561,18 @@ func mergeExecutionTargetOverride(base domain.ExecutionTarget, override domain.E if len(override.ExtraParams) > 0 { out.ExtraParams = copyExtraParams(override.ExtraParams) } - return out, presence, nil + return out, presence } -func resolveExecutionTarget(backendValue *domain.Backend, profileValue *domain.ExecutionProfile, override *domain.ExecutionTargetOverride) (domain.ExecutionTarget, domain.ExecutionTargetPresence, error) { +func resolveExecutionTarget(backendValue *domain.Backend, profileValue *domain.ExecutionProfile, override *domain.ExecutionTargetOverride) (domain.ExecutionTarget, domain.ExecutionTargetPresence) { out := defaults.ExecutionTargetDefault() out = mergeExecutionTarget(out, backendToTarget(backendValue)) out = mergeExecutionTarget(out, executionProfileToTarget(profileValue)) var presence domain.ExecutionTargetPresence if override != nil { - var err error - out, presence, err = mergeExecutionTargetOverride(out, *override) - if err != nil { - return domain.ExecutionTarget{}, domain.ExecutionTargetPresence{}, err - } + out, presence = mergeExecutionTargetOverride(out, *override) } - return out, presence, nil + return out, presence } func validateAPIKey(apiKeyEnv string, apiKey string, apiKeyRequired bool) error { diff --git a/internal/usecase/runner_test.go b/internal/usecase/runner_test.go index c0bf3d3..03cb883 100644 --- a/internal/usecase/runner_test.go +++ b/internal/usecase/runner_test.go @@ -6,9 +6,11 @@ import ( "encoding/hex" "errors" "fmt" + "math" "path/filepath" "reflect" "regexp" + "strconv" "strings" "sync" "testing" @@ -502,6 +504,35 @@ func TestRunnerDirectSessionResolution(t *testing.T) { t.Fatalf("invalid direct session invoked generation %d times", llmClient.calls) } }) + + t.Run("malformed direct value fails before loading or generation", func(t *testing.T) { + promptRepo := &fakePromptRepo{def: promptDef(domain.FormatText, domain.ValidationNone, 0)} + llmClient := &fakeLLM{resp: &domain.GenerateResponse{Content: "unexpected"}} + runner := NewRunner( + promptRepo, + &fakeExecutionProfileRepo{profiles: map[string]*domain.ExecutionProfile{"exec": defaultExecutionProfile()}}, + nil, + defaultArtifactReader(), + defaultRenderer(), + llmClient, + nil, nil) + + _, err := runner.Run(context.Background(), domain.RunRequest{ + PromptID: "p", + ProfileID: "exec", + SessionID: "session" + string([]byte{0xff}), + Inputs: singleInputRef(), + }) + if !errors.Is(err, ErrInvalidRequest) { + t.Fatalf("expected ErrInvalidRequest, got %v", err) + } + if promptRepo.lastID != "" { + t.Fatalf("invalid direct session loaded prompt %q", promptRepo.lastID) + } + if llmClient.calls != 0 { + t.Fatalf("invalid direct session invoked generation %d times", llmClient.calls) + } + }) } func TestRunnerPrepareUsesPromptDefaultProfileWhenNoExplicitProfileID(t *testing.T) { @@ -704,16 +735,23 @@ func TestRunnerPrepareRequestNumericOverridePresence(t *testing.T) { } func TestRunnerPrepareInvalidRequestNumericOverridesFail(t *testing.T) { - tests := []struct { + type testCase struct { name string override *domain.ExecutionTargetOverride - }{ + } + tests := []testCase{ {name: "temperature below range", override: &domain.ExecutionTargetOverride{Temperature: float64Ptr(-0.1)}}, {name: "temperature above range", override: &domain.ExecutionTargetOverride{Temperature: float64Ptr(2.1)}}, {name: "max tokens below range", override: &domain.ExecutionTargetOverride{MaxTokens: intPtr(-1)}}, {name: "top p below range", override: &domain.ExecutionTargetOverride{TopP: float64Ptr(-0.1)}}, {name: "top p above range", override: &domain.ExecutionTargetOverride{TopP: float64Ptr(1.1)}}, {name: "timeout below range", override: &domain.ExecutionTargetOverride{TimeoutSeconds: intPtr(-1)}}, + {name: "temperature is not finite", override: &domain.ExecutionTargetOverride{Temperature: float64Ptr(math.NaN())}}, + {name: "top p is not finite", override: &domain.ExecutionTargetOverride{TopP: float64Ptr(math.Inf(1))}}, + } + if strconv.IntSize == 64 { + durationLimit := int64(math.MaxInt64 / int64(time.Second)) + tests = append(tests, testCase{name: "timeout cannot be represented as a duration", override: &domain.ExecutionTargetOverride{TimeoutSeconds: intPtr(int(durationLimit) + 1)}}) } for _, tc := range tests { @@ -2300,10 +2338,7 @@ func TestResolveExecutionTargetProfileValuesPopulateAllSupportedFields(t *testin }, } - target, presence, err := resolveExecutionTarget(nil, profileValue, nil) - if err != nil { - t.Fatalf("expected no error, got %v", err) - } + target, presence := resolveExecutionTarget(nil, profileValue, nil) if presence != (domain.ExecutionTargetPresence{}) { t.Fatalf("expected no request override presence, got %+v", presence) } @@ -2355,10 +2390,7 @@ func TestResolveExecutionTargetRuntimeOverridesBeatProfileForAllOverrideableFiel }, } - target, presence, err := resolveExecutionTarget(nil, profileValue, override) - if err != nil { - t.Fatalf("expected no error, got %v", err) - } + target, presence := resolveExecutionTarget(nil, profileValue, override) if presence != (domain.ExecutionTargetPresence{Temperature: true, MaxTokens: true, TopP: true, TimeoutSeconds: true}) { t.Fatalf("unexpected override presence: %+v", presence) } @@ -2405,12 +2437,9 @@ func TestResolveExecutionTargetReasoningOverrideStates(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - target, _, err := resolveExecutionTarget(nil, profileValue, &domain.ExecutionTargetOverride{ + target, _ := resolveExecutionTarget(nil, profileValue, &domain.ExecutionTargetOverride{ ReasoningEffort: tt.override, }) - if err != nil { - t.Fatalf("resolve execution target: %v", err) - } if target.ReasoningEffort != tt.want { t.Fatalf("reasoning effort = %q, want %q", target.ReasoningEffort, tt.want) } @@ -2539,10 +2568,7 @@ func TestResolveExecutionTargetUsesBackendProfileAndRequestPrecedence(t *testing ExtraParams: map[string]any{"request": true}, } - target, _, err := resolveExecutionTarget(backendValue, profileValue, override) - if err != nil { - t.Fatalf("resolve target: %v", err) - } + target, _ := resolveExecutionTarget(backendValue, profileValue, override) if target.BackendID != "custom" { t.Fatalf("endpoint override changed backend identity: %+v", target) } @@ -2553,12 +2579,9 @@ func TestResolveExecutionTargetUsesBackendProfileAndRequestPrecedence(t *testing t.Fatalf("expected whole-map request replacement, got %#v", target.ExtraParams) } - target, _, err = resolveExecutionTarget(backendValue, &domain.ExecutionProfile{ + target, _ = resolveExecutionTarget(backendValue, &domain.ExecutionProfile{ ID: "exec", BackendID: "custom", Model: "profile-model", }, nil) - if err != nil { - t.Fatalf("resolve backend defaults: %v", err) - } if target.Endpoint != backendValue.Endpoint || target.APIKeyEnv != backendValue.APIKeyEnv || !reflect.DeepEqual(target.ExtraParams, backendValue.ExtraParams) { diff --git a/profiles.go b/profiles.go index ffc9bf5..04b649c 100644 --- a/profiles.go +++ b/profiles.go @@ -116,17 +116,10 @@ func validatePublicProfile(prof domain.ExecutionProfile) error { if strings.TrimSpace(prof.Model) == "" { return errors.New("model is required") } - if prof.Temperature < 0 || prof.Temperature > 2 { - return errors.New("temperature must be between 0 and 2") - } - if prof.MaxTokens < 0 { - return errors.New("max_tokens must be greater than or equal to 0") - } - if prof.TopP < 0 || prof.TopP > 1 { - return errors.New("top_p must be between 0 and 1") - } - if prof.TimeoutSeconds < 0 { - return errors.New("timeout_seconds must be greater than or equal to 0") - } - return nil + return domain.ValidateExecutionTargetSettings(domain.ExecutionTarget{ + Temperature: prof.Temperature, + MaxTokens: prof.MaxTokens, + TopP: prof.TopP, + TimeoutSeconds: prof.TimeoutSeconds, + }) }