diff --git a/convert.go b/convert.go index 522be0f..13d3264 100644 --- a/convert.go +++ b/convert.go @@ -6,7 +6,11 @@ import ( "gitea.maximumdirect.net/eric/scriptorium/internal/domain" ) -func toDomainRunRequest(req RunRequest) domain.RunRequest { +func toDomainRunRequest(req RunRequest) (domain.RunRequest, error) { + execution, err := toDomainExecutionTargetOverride(req.Execution) + if err != nil { + return domain.RunRequest{}, err + } return domain.RunRequest{ PromptID: req.PromptID, PromptVersion: req.PromptVersion, @@ -14,10 +18,10 @@ func toDomainRunRequest(req RunRequest) domain.RunRequest { APIKey: req.APIKey, Inputs: toDomainArtifactRefMap(req.Inputs), Vars: copyStringMap(req.Vars), - Execution: toDomainExecutionTargetOverride(req.Execution), + Execution: execution, Validation: toDomainOutputContractPtr(req.Validation), Metadata: copyStringMap(req.Metadata), - } + }, nil } func fromDomainPreparedRun(prepared *domain.PreparedRun) *PreparedRun { @@ -124,9 +128,13 @@ func fromDomainArtifact(artifact domain.Artifact) Artifact { } } -func toDomainExecutionTargetOverride(override *ExecutionTargetOverride) *domain.ExecutionTargetOverride { +func toDomainExecutionTargetOverride(override *ExecutionTargetOverride) (*domain.ExecutionTargetOverride, error) { if override == nil { - return nil + return nil, nil + } + extraParams, err := copyPublicJSONMap(override.ExtraParams) + if err != nil { + return nil, err } return &domain.ExecutionTargetOverride{ Endpoint: override.Endpoint, @@ -138,8 +146,8 @@ func toDomainExecutionTargetOverride(override *ExecutionTargetOverride) *domain. ServiceTier: override.ServiceTier, ReasoningEffort: override.ReasoningEffort, APIKeyEnv: override.APIKeyEnv, - ExtraParams: copyAnyMap(override.ExtraParams), - } + ExtraParams: extraParams, + }, nil } func fromDomainExecutionTarget(target domain.ExecutionTarget) ExecutionTarget { diff --git a/engine.go b/engine.go index ec109c8..ffdac71 100644 --- a/engine.go +++ b/engine.go @@ -257,7 +257,12 @@ func (e *Engine) Prepare(ctx context.Context, req RunRequest) (*PreparedRun, err return nil, fmt.Errorf("%w: engine is nil", ErrInvalidConfig) } - prepared, err := e.runner.Prepare(ctx, toDomainRunRequest(req)) + domainReq, err := toDomainRunRequest(req) + if err != nil { + return nil, fmt.Errorf("%w: %v", ErrInvalidRequest, err) + } + + prepared, err := e.runner.Prepare(ctx, domainReq) if err != nil { return nil, mapPublicError(err) } @@ -270,7 +275,12 @@ func (e *Engine) Run(ctx context.Context, req RunRequest) (*RunResult, error) { return nil, fmt.Errorf("%w: engine is nil", ErrInvalidConfig) } - result, err := e.runner.Run(ctx, toDomainRunRequest(req)) + domainReq, err := toDomainRunRequest(req) + if err != nil { + return nil, fmt.Errorf("%w: %v", ErrInvalidRequest, err) + } + + result, err := e.runner.Run(ctx, domainReq) if err != nil { return nil, mapPublicError(err) } diff --git a/engine_test.go b/engine_test.go index d858a4e..5a4962b 100644 --- a/engine_test.go +++ b/engine_test.go @@ -4,6 +4,7 @@ import ( "context" "encoding/json" "errors" + "math" "net/http" "net/http/httptest" "os" @@ -1147,6 +1148,66 @@ func TestInMemoryProfileExtraParamsAreCopiedAcrossPublicBoundary(t *testing.T) { } } +func TestWithProfilesRejectsInvalidExtraParams(t *testing.T) { + tests := []struct { + name string + extraParams map[string]any + }{ + {name: "function", extraParams: map[string]any{"bad": func() {}}}, + {name: "channel", extraParams: map[string]any{"bad": make(chan struct{})}}, + {name: "struct", extraParams: map[string]any{"bad": struct{ Name string }{Name: "bad"}}}, + {name: "non string map key", extraParams: map[string]any{"bad": map[int]string{1: "one"}}}, + {name: "nan", extraParams: map[string]any{"bad": math.NaN()}}, + {name: "positive infinity", extraParams: map[string]any{"bad": math.Inf(1)}}, + {name: "negative infinity", extraParams: map[string]any{"bad": math.Inf(-1)}}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + _, err := scriptorium.NewEngine(scriptorium.Config{PromptDir: "./examples/prompts"}, + scriptorium.WithProfiles(scriptorium.Profile{ + ID: "invalid-extra-params", + Endpoint: "http://invalid/v1", + Model: "invalid-model", + ExtraParams: tc.extraParams, + }), + ) + if !errors.Is(err, scriptorium.ErrInvalidConfig) { + t.Fatalf("expected ErrInvalidConfig, got %v", err) + } + }) + } +} + +func TestWithProfilesRejectsCyclicExtraParams(t *testing.T) { + cyclicMap := map[string]any{} + cyclicMap["self"] = cyclicMap + cyclicSlice := []any{nil} + cyclicSlice[0] = cyclicSlice + + tests := []struct { + name string + extraParams map[string]any + }{ + {name: "map", extraParams: cyclicMap}, + {name: "slice", extraParams: map[string]any{"cycle": cyclicSlice}}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + _, err := scriptorium.NewEngine(scriptorium.Config{PromptDir: "./examples/prompts"}, + scriptorium.WithProfiles(scriptorium.Profile{ + ID: "cyclic-extra-params", + Endpoint: "http://cyclic/v1", + Model: "cyclic-model", + ExtraParams: tc.extraParams, + }), + ) + if !errors.Is(err, scriptorium.ErrInvalidConfig) { + t.Fatalf("expected ErrInvalidConfig, got %v", err) + } + }) + } +} + func TestRunStructuredOutputWorksWithSchemaFS(t *testing.T) { fake := &fakeLLMClient{response: &scriptorium.GenerateResponse{Content: `{"events":[]}`}} engine, err := scriptorium.NewEngine(scriptorium.Config{ @@ -1301,6 +1362,78 @@ func TestExtraParamsTypedNestedValuesAreCopiedAcrossPublicBoundary(t *testing.T) } } +func TestRunRejectsInvalidExtraParams(t *testing.T) { + tests := []struct { + name string + extraParams map[string]any + }{ + {name: "function", extraParams: map[string]any{"bad": func() {}}}, + {name: "channel", extraParams: map[string]any{"bad": make(chan struct{})}}, + {name: "struct", extraParams: map[string]any{"bad": struct{ Name string }{Name: "bad"}}}, + {name: "non string map key", extraParams: map[string]any{"bad": map[int]string{1: "one"}}}, + {name: "nan", extraParams: map[string]any{"bad": math.NaN()}}, + {name: "positive infinity", extraParams: map[string]any{"bad": math.Inf(1)}}, + {name: "negative infinity", extraParams: map[string]any{"bad": math.Inf(-1)}}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + fake := &fakeLLMClient{response: &scriptorium.GenerateResponse{Content: "ok"}} + engine := newExampleEngineWithOptions(t, "./examples/schemas", scriptorium.WithLLMClient(fake)) + + _, err := engine.Run(context.Background(), scriptorium.RunRequest{ + PromptID: "generic.markdown_summary", + Inputs: map[string]scriptorium.ArtifactRef{ + "transcript": scriptorium.Inline("Rin opens the gate."), + "glossary": scriptorium.Inline("gate: A guarded passage."), + }, + Execution: &scriptorium.ExecutionTargetOverride{ExtraParams: tc.extraParams}, + }) + if !errors.Is(err, scriptorium.ErrInvalidRequest) { + t.Fatalf("expected ErrInvalidRequest, got %v", err) + } + if len(fake.requests) != 0 { + t.Fatalf("expected invalid request to fail before LLM call, got %d requests", len(fake.requests)) + } + }) + } +} + +func TestRunRejectsCyclicExtraParams(t *testing.T) { + cyclicMap := map[string]any{} + cyclicMap["self"] = cyclicMap + cyclicSlice := []any{nil} + cyclicSlice[0] = cyclicSlice + + tests := []struct { + name string + extraParams map[string]any + }{ + {name: "map", extraParams: cyclicMap}, + {name: "slice", extraParams: map[string]any{"cycle": cyclicSlice}}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + fake := &fakeLLMClient{response: &scriptorium.GenerateResponse{Content: "ok"}} + engine := newExampleEngineWithOptions(t, "./examples/schemas", scriptorium.WithLLMClient(fake)) + + _, err := engine.Run(context.Background(), scriptorium.RunRequest{ + PromptID: "generic.markdown_summary", + Inputs: map[string]scriptorium.ArtifactRef{ + "transcript": scriptorium.Inline("Rin opens the gate."), + "glossary": scriptorium.Inline("gate: A guarded passage."), + }, + Execution: &scriptorium.ExecutionTargetOverride{ExtraParams: tc.extraParams}, + }) + if !errors.Is(err, scriptorium.ErrInvalidRequest) { + t.Fatalf("expected ErrInvalidRequest, got %v", err) + } + if len(fake.requests) != 0 { + t.Fatalf("expected invalid request to fail before LLM call, got %d requests", len(fake.requests)) + } + }) + } +} + func TestWithLLMClientRejectsNilClient(t *testing.T) { _, err := scriptorium.NewEngine(exampleConfig("./examples/schemas"), scriptorium.WithLLMClient(nil)) if !errors.Is(err, scriptorium.ErrInvalidConfig) { diff --git a/json_copy.go b/json_copy.go new file mode 100644 index 0000000..d09e36c --- /dev/null +++ b/json_copy.go @@ -0,0 +1,218 @@ +package scriptorium + +import ( + "encoding/json" + "fmt" + "math" + "reflect" + "strconv" +) + +const maxSafeJSONInteger = 1<<53 - 1 + +type jsonVisit struct { + typ reflect.Type + ptr uintptr +} + +func copyPublicJSONMap(src map[string]any) (map[string]any, error) { + if src == nil { + return nil, nil + } + copied, err := copyPublicJSONValue(reflect.ValueOf(src), "extra_params", make(map[jsonVisit]struct{})) + if err != nil { + return nil, err + } + out, ok := copied.(map[string]any) + if !ok { + return nil, fmt.Errorf("extra_params: expected object") + } + return out, nil +} + +func copyPublicJSONValue(value reflect.Value, path string, seen map[jsonVisit]struct{}) (any, error) { + if !value.IsValid() { + return nil, nil + } + if value.Kind() == reflect.Interface { + if value.IsNil() { + return nil, nil + } + return copyPublicJSONValue(value.Elem(), path, seen) + } + if !value.CanInterface() { + return nil, fmt.Errorf("%s: value cannot be copied", path) + } + if number, ok := value.Interface().(json.Number); ok { + f, err := strconv.ParseFloat(number.String(), 64) + if err != nil || math.IsNaN(f) || math.IsInf(f, 0) { + return nil, fmt.Errorf("%s: invalid JSON number", path) + } + return number, nil + } + + switch value.Kind() { + case reflect.Bool, reflect.String: + return value.Interface(), nil + case reflect.Int, reflect.Int8, reflect.Int16, reflect.Int32, reflect.Int64: + if value.Int() < -maxSafeJSONInteger || value.Int() > maxSafeJSONInteger { + return nil, fmt.Errorf("%s: integer is outside the JSON-safe range", path) + } + return value.Interface(), nil + case reflect.Uint, reflect.Uint8, reflect.Uint16, reflect.Uint32, reflect.Uint64, reflect.Uintptr: + if value.Uint() > maxSafeJSONInteger { + return nil, fmt.Errorf("%s: integer is outside the JSON-safe range", path) + } + return value.Interface(), nil + case reflect.Float32, reflect.Float64: + f := value.Convert(reflect.TypeOf(float64(0))).Float() + if math.IsNaN(f) || math.IsInf(f, 0) { + return nil, fmt.Errorf("%s: floating-point value must be finite", path) + } + return value.Interface(), nil + case reflect.Pointer: + if value.IsNil() { + return nil, nil + } + visit := jsonVisit{typ: value.Type(), ptr: value.Pointer()} + if _, ok := seen[visit]; ok { + return nil, fmt.Errorf("%s: cyclic value is not supported", path) + } + seen[visit] = struct{}{} + defer delete(seen, visit) + return copyPublicJSONValue(value.Elem(), path, seen) + case reflect.Map: + return copyPublicJSONMapValue(value, path, seen) + case reflect.Slice: + if value.IsNil() { + return nil, nil + } + return copyPublicJSONSequenceValue(value, path, seen) + case reflect.Array: + return copyPublicJSONSequenceValue(value, path, seen) + default: + return nil, fmt.Errorf("%s: unsupported JSON value type %s", path, value.Type()) + } +} + +func copyPublicJSONMapValue(value reflect.Value, path string, seen map[jsonVisit]struct{}) (any, error) { + if value.IsNil() { + return nil, nil + } + if value.Type().Key().Kind() != reflect.String { + return nil, fmt.Errorf("%s: map key type %s is not supported", path, value.Type().Key()) + } + + visit := jsonVisit{typ: value.Type(), ptr: value.Pointer()} + if _, ok := seen[visit]; ok { + return nil, fmt.Errorf("%s: cyclic value is not supported", path) + } + seen[visit] = struct{}{} + defer delete(seen, visit) + + type entry struct { + key reflect.Value + name string + value any + } + entries := make([]entry, 0, value.Len()) + preserveType := true + elemType := value.Type().Elem() + iter := value.MapRange() + for iter.Next() { + key := iter.Key() + name := key.String() + copied, err := copyPublicJSONValue(iter.Value(), path+"."+name, seen) + if err != nil { + return nil, err + } + entries = append(entries, entry{key: key, name: name, value: copied}) + if copied == nil { + if !canAssignNil(elemType) { + preserveType = false + } + continue + } + if !reflect.TypeOf(copied).AssignableTo(elemType) { + preserveType = false + } + } + + if preserveType { + out := reflect.MakeMapWithSize(value.Type(), len(entries)) + for _, entry := range entries { + if entry.value == nil { + out.SetMapIndex(entry.key, reflect.Zero(elemType)) + continue + } + out.SetMapIndex(entry.key, reflect.ValueOf(entry.value)) + } + return out.Interface(), nil + } + + out := make(map[string]any, len(entries)) + for _, entry := range entries { + out[entry.name] = entry.value + } + return out, nil +} + +func copyPublicJSONSequenceValue(value reflect.Value, path string, seen map[jsonVisit]struct{}) (any, error) { + var visit jsonVisit + if value.Kind() == reflect.Slice { + visit = jsonVisit{typ: value.Type(), ptr: value.Pointer()} + if _, ok := seen[visit]; ok { + return nil, fmt.Errorf("%s: cyclic value is not supported", path) + } + seen[visit] = struct{}{} + defer delete(seen, visit) + } + + values := make([]any, value.Len()) + preserveType := true + elemType := value.Type().Elem() + for i := 0; i < value.Len(); i++ { + copied, err := copyPublicJSONValue(value.Index(i), fmt.Sprintf("%s[%d]", path, i), seen) + if err != nil { + return nil, err + } + values[i] = copied + if copied == nil { + if !canAssignNil(elemType) { + preserveType = false + } + continue + } + if !reflect.TypeOf(copied).AssignableTo(elemType) { + preserveType = false + } + } + + if preserveType { + out := reflect.New(value.Type()).Elem() + if value.Kind() == reflect.Slice { + out = reflect.MakeSlice(value.Type(), value.Len(), value.Len()) + } + for i, copied := range values { + if copied == nil { + out.Index(i).Set(reflect.Zero(elemType)) + continue + } + out.Index(i).Set(reflect.ValueOf(copied)) + } + return out.Interface(), nil + } + + out := make([]any, len(values)) + copy(out, values) + return out, nil +} + +func canAssignNil(typ reflect.Type) bool { + switch typ.Kind() { + case reflect.Chan, reflect.Func, reflect.Interface, reflect.Map, reflect.Pointer, reflect.Slice: + return true + default: + return false + } +} diff --git a/profiles.go b/profiles.go index ded63eb..ad6f49b 100644 --- a/profiles.go +++ b/profiles.go @@ -60,6 +60,10 @@ func (r *memoryProfileRepository) GetProfile(_ context.Context, id string) (*dom } func toDomainProfile(publicProfile Profile) (domain.ExecutionProfile, error) { + extraParams, err := copyPublicJSONMap(publicProfile.ExtraParams) + if err != nil { + return domain.ExecutionProfile{}, err + } prof := domain.ExecutionProfile{ ID: strings.TrimSpace(publicProfile.ID), Endpoint: publicProfile.Endpoint, @@ -71,7 +75,7 @@ func toDomainProfile(publicProfile Profile) (domain.ExecutionProfile, error) { ServiceTier: publicProfile.ServiceTier, ReasoningEffort: publicProfile.ReasoningEffort, APIKeyRequired: publicProfile.APIKeyRequired, - ExtraParams: copyAnyMap(publicProfile.ExtraParams), + ExtraParams: extraParams, } if err := validatePublicProfile(prof); err != nil { return domain.ExecutionProfile{}, err