From 1d7fac0a47335a9b9fcea271a649b45b5ff70c27 Mon Sep 17 00:00:00 2001 From: Eric Rakestraw Date: Sat, 4 Jul 2026 11:32:47 -0500 Subject: [PATCH] Implement fixes to the initial library facade --- convert.go | 63 +++++++++++++++++++++++- engine_test.go | 130 +++++++++++++++++++++++++++++++++++++++++++++++++ errors.go | 8 ++- 3 files changed, 199 insertions(+), 2 deletions(-) diff --git a/convert.go b/convert.go index 7be61ff..da543e4 100644 --- a/convert.go +++ b/convert.go @@ -1,6 +1,10 @@ package scriptorium -import "gitea.maximumdirect.net/eric/scriptorium/internal/domain" +import ( + "reflect" + + "gitea.maximumdirect.net/eric/scriptorium/internal/domain" +) func toDomainRunRequest(req RunRequest) domain.RunRequest { return domain.RunRequest{ @@ -282,6 +286,9 @@ func copyAnyMap(src map[string]any) map[string]any { } func copyAny(value any) any { + if value == nil { + return nil + } switch v := value.(type) { case map[string]any: return copyAnyMap(v) @@ -295,6 +302,60 @@ func copyAny(value any) any { return copyStringSlice(v) case []byte: return copyBytes(v) + default: + return copyReflectValue(reflect.ValueOf(value)).Interface() + } +} + +func copyReflectValue(value reflect.Value) reflect.Value { + if !value.IsValid() { + return value + } + + switch value.Kind() { + case reflect.Interface: + if value.IsNil() { + return reflect.Zero(value.Type()) + } + copied := copyReflectValue(value.Elem()) + if copied.IsValid() && copied.Type().AssignableTo(value.Type()) { + return copied + } + out := reflect.New(value.Type()).Elem() + out.Set(copied) + return out + case reflect.Pointer: + if value.IsNil() { + return reflect.Zero(value.Type()) + } + out := reflect.New(value.Type().Elem()) + out.Elem().Set(copyReflectValue(value.Elem())) + return out + case reflect.Map: + if value.IsNil() { + return reflect.Zero(value.Type()) + } + out := reflect.MakeMapWithSize(value.Type(), value.Len()) + iter := value.MapRange() + for iter.Next() { + out.SetMapIndex(copyReflectValue(iter.Key()), copyReflectValue(iter.Value())) + } + return out + case reflect.Slice: + if value.IsNil() { + return reflect.Zero(value.Type()) + } + out := reflect.MakeSlice(value.Type(), value.Len(), value.Cap()) + for i := 0; i < value.Len(); i++ { + out.Index(i).Set(copyReflectValue(value.Index(i))) + } + return out + case reflect.Array: + out := reflect.New(value.Type()).Elem() + for i := 0; i < value.Len(); i++ { + out.Index(i).Set(copyReflectValue(value.Index(i))) + } + return out default: return value } diff --git a/engine_test.go b/engine_test.go index 420c302..6fc15cc 100644 --- a/engine_test.go +++ b/engine_test.go @@ -5,6 +5,8 @@ import ( "encoding/json" "errors" "os" + "path/filepath" + "reflect" "strings" "testing" @@ -363,6 +365,134 @@ func TestPublicErrorsSupportErrorsIs(t *testing.T) { } } +func TestSelectedProfileRawAPIKeyMapsToProfileLoad(t *testing.T) { + profileDir := t.TempDir() + if err := os.WriteFile(filepath.Join(profileDir, "raw.yaml"), []byte(` +id: raw-profile +endpoint: http://localhost:8000/v1 +model: model +api_key: secret +`), 0644); err != nil { + t.Fatal(err) + } + + engine, err := scriptorium.NewEngine(scriptorium.Config{ + PromptDir: "./examples/prompts", + ProfileDir: profileDir, + SchemaDir: "./examples/schemas", + }) + if err != nil { + t.Fatalf("expected engine construction to succeed, got %v", err) + } + + _, err = engine.Prepare(context.Background(), scriptorium.RunRequest{ + PromptID: "generic.markdown_summary", + ProfileID: "raw-profile", + Inputs: map[string]scriptorium.ArtifactRef{ + "transcript": scriptorium.Inline("Rin opens the gate."), + "glossary": scriptorium.Inline("gate: A guarded passage."), + }, + }) + if !errors.Is(err, scriptorium.ErrProfileLoad) { + t.Fatalf("expected ErrProfileLoad, got %v", err) + } + if errors.Is(err, scriptorium.ErrPromptLoad) { + t.Fatalf("did not expect ErrPromptLoad, got %v", err) + } +} + +func TestSelectedProfileInvalidYAMLMapsToProfileLoad(t *testing.T) { + profileDir := t.TempDir() + if err := os.WriteFile(filepath.Join(profileDir, "broken.yaml"), []byte(` +id: broken-profile +unknown_field: true +`), 0644); err != nil { + t.Fatal(err) + } + + engine, err := scriptorium.NewEngine(scriptorium.Config{ + PromptDir: "./examples/prompts", + ProfileDir: profileDir, + SchemaDir: "./examples/schemas", + }) + if err != nil { + t.Fatalf("expected engine construction to succeed, got %v", err) + } + + _, err = engine.Prepare(context.Background(), scriptorium.RunRequest{ + PromptID: "generic.markdown_summary", + ProfileID: "broken-profile", + Inputs: map[string]scriptorium.ArtifactRef{ + "transcript": scriptorium.Inline("Rin opens the gate."), + "glossary": scriptorium.Inline("gate: A guarded passage."), + }, + }) + if !errors.Is(err, scriptorium.ErrProfileLoad) { + t.Fatalf("expected ErrProfileLoad, got %v", err) + } + if errors.Is(err, scriptorium.ErrPromptLoad) { + t.Fatalf("did not expect ErrPromptLoad, got %v", err) + } +} + +func TestExtraParamsTypedNestedValuesAreCopiedAcrossPublicBoundary(t *testing.T) { + fake := &fakeLLMClient{response: &scriptorium.GenerateResponse{Content: "ok"}} + engine := newExampleEngineWithOptions(t, "./examples/schemas", scriptorium.WithLLMClient(fake)) + + labels := map[string]string{"route": "primary"} + counts := map[string]int{"retry_budget": 2} + weights := []float64{0.25, 0.75} + ids := []int{1, 2, 3} + nested := map[string]any{ + "labels": labels, + "counts": counts, + "weights": weights, + "ids": ids, + } + extraParams := map[string]any{ + "labels": labels, + "counts": counts, + "nested": nested, + } + + _, 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: extraParams}, + }) + if err != nil { + t.Fatalf("expected run to succeed, got %v", err) + } + if len(fake.requests) != 1 { + t.Fatalf("expected one generate request, got %d", len(fake.requests)) + } + + captured := fake.requests[0].Target.ExtraParams + labels["route"] = "mutated" + counts["retry_budget"] = 99 + weights[0] = 9.9 + ids[0] = 99 + nested["added"] = "mutated" + extraParams["new_top_level"] = "mutated" + + want := map[string]any{ + "labels": map[string]string{"route": "primary"}, + "counts": map[string]int{"retry_budget": 2}, + "nested": map[string]any{ + "labels": map[string]string{"route": "primary"}, + "counts": map[string]int{"retry_budget": 2}, + "weights": []float64{0.25, 0.75}, + "ids": []int{1, 2, 3}, + }, + } + if !reflect.DeepEqual(captured, want) { + t.Fatalf("captured extra_params changed after mutating source:\ngot=%#v\nwant=%#v", captured, want) + } +} + func TestWithLLMClientRejectsNilClient(t *testing.T) { _, err := scriptorium.NewEngine(exampleConfig("./examples/schemas"), scriptorium.WithLLMClient(nil)) if !errors.Is(err, scriptorium.ErrInvalidConfig) { diff --git a/errors.go b/errors.go index f324375..298adba 100644 --- a/errors.go +++ b/errors.go @@ -51,7 +51,7 @@ func publicErrorFor(err error) error { return ErrProfileNotFound case errors.Is(err, promptdef.ErrInvalidYAML), errors.Is(err, promptdef.ErrInvalidPromptDefinition): return ErrPromptLoad - case errors.Is(err, profile.ErrInvalidYAML), errors.Is(err, profile.ErrInvalidProfile): + case isProfileLoadCause(err): return ErrProfileLoad case errors.Is(err, usecase.ErrArtifactLoad): return ErrArtifactLoad @@ -69,3 +69,9 @@ func publicErrorFor(err error) error { return nil } } + +func isProfileLoadCause(err error) bool { + return errors.Is(err, profile.ErrInvalidYAML) || + errors.Is(err, profile.ErrInvalidProfile) || + errors.Is(err, profile.ErrRawAPIKeyNotAllowed) +}