From bf058a046e8e4882c78901855dd45fa4f0eea04f Mon Sep 17 00:00:00 2001 From: Eric Rakestraw Date: Tue, 5 May 2026 20:01:45 -0500 Subject: [PATCH] Documentation cleanup and bugfixes --- README.md | 25 +++++++--- architecture.md | 11 +++-- internal/adapter/cli/run.go | 16 ++---- internal/adapter/cli/run_test.go | 26 ++++++++-- internal/adapter/http/dto.go | 15 +++--- internal/adapter/http/handler.go | 10 ++-- internal/adapter/http/handler_test.go | 23 ++++++++- internal/promptdef/repository_test.go | 13 +++++ .../testdata/unknown_input_field.yaml | 13 +++++ internal/usecase/runner_test.go | 49 +++++++++++++++++++ 10 files changed, 162 insertions(+), 39 deletions(-) create mode 100644 internal/promptdef/testdata/unknown_input_field.yaml diff --git a/README.md b/README.md index ee93d22..3aa79db 100644 --- a/README.md +++ b/README.md @@ -138,8 +138,6 @@ Starts the HTTP API. **Optional Flags:** - `--addr`: Listen address (default `:8080`). - `--schema-dir`: Base directory for validation schemas. -- `--model`: Default model override. -- `--timeout`: Default request timeout. ## HTTP API @@ -152,6 +150,7 @@ Executes a prompt. No built-in authentication is provided; deploy behind a trust { "prompt_id": "generic.structured_events", "profile_id": "local-quality", + "include_raw_output": false, "inputs": { "transcript": {"type": "file", "uri": "./examples/fixtures/transcript.md"} }, @@ -166,11 +165,18 @@ Executes a prompt. No built-in authentication is provided; deploy behind a trust } ``` +`profile_id` is optional. If omitted, Scriptorium uses the prompt's `default_profile`. If neither is available, the run fails. + **Response:** Returns a `200 OK` with the generated artifact, validation results, and metadata including the `prompt_id` and the `selected_profile_id`. **Validation Failures:** -If the model output fails validation (e.g., invalid JSON), the API returns `200 OK` with `validation.status = "failed"`. The original `raw_model_output` is preserved in the response to allow debugging. +If the model output fails validation (e.g., invalid JSON), the API returns `200 OK` with `validation.status = "failed"`. + +**Raw Output Exposure:** +- `raw_model_output` is omitted by default. +- Set `include_raw_output: true` in the request to include it in the response. +- Raw output is preserved internally in run results regardless of HTTP exposure. ## Prompt Definition Authoring @@ -186,26 +192,31 @@ default_profile: local-quality inputs: - name: transcript required: true + content_type: text/markdown description: "The raw session transcript" - name: glossary required: false + content_type: application/yaml + description: "Optional glossary terms" -templates: +messages: - role: system content: "You are a helpful assistant." - role: user content_file: messages/extract_events.tmpl -output_format: json -validation: +output: + format: json validation_mode: json_schema schema_path: structured_events.schema.json repair_attempts: 2 ``` **Key Features:** -- **Inline vs File**: Use `content` for short prompts or `content_file` for larger templates. +- **Inline vs File**: Use `content` for short prompts or `content_file` for larger templates. Exactly one must be set per message. +- **Path Resolution**: `content_file` paths are resolved relative to the prompt YAML file. - **Inputs**: Mark inputs as `required` to ensure the runner fails early if they are missing. +- **Input Metadata**: `content_type` is currently descriptive metadata and not enforced yet. - **Validation**: Support `none`, `basic`, `json`, and `json_schema`. - **Repair**: `repair_attempts` enables bounded retries to fix structured output. diff --git a/architecture.md b/architecture.md index 655cfbd..3f2dc51 100644 --- a/architecture.md +++ b/architecture.md @@ -50,7 +50,7 @@ The `Runner.Run` flow executes the following steps: - Built-in application defaults. 5. **Resolve Artifacts**: Load all named input artifacts defined in the request. 6. **Render Prompt**: Apply template variables and input artifacts to the prompt templates. -7. **Call LLM**: Execute the generation request using the resolved `ExecutionTarget`. +7. **Call LLM**: Execute the generation request using the resolved execution target (effective runtime settings). 8. **Validate/Repair**: - Validate the model output against the output contract. - If structured validation fails and `repair_attempts > 0`, perform bounded repair and re-validate. @@ -86,13 +86,18 @@ Key domain types: ### CLI - `run`: Executes a prompt. Uses flags like `--prompt`, `--profile`, `--input`, and various runtime overrides (e.g., `--model`, `--temperature`). -- `serve`: Starts the HTTP API. +- `serve`: Starts the HTTP API using infrastructure-only flags (`--addr`, `--prompt-dir`, `--profile-dir`, `--schema-dir`). It does not introduce a server-level model/runtime precedence layer. ### HTTP API - `POST /v1/runs`: Accepts `RunRequest` JSON and returns `RunResponse` JSON. No built-in auth. +- Request may include runtime overrides under `model` and an `include_raw_output` boolean. +- `raw_model_output` is exposed only when explicitly requested with `include_raw_output=true`. ### YAML Shapes -- **Prompt YAML**: Includes `id`, `version`, `default_profile`, `inputs`, `templates`, and `validation`. +- **Prompt YAML**: Includes `id`, `version`, optional `default_profile`, `inputs`, `messages`, and `output`. + - Inputs support `name`, `required`, optional `content_type`, and `description`. + - Messages require `role` and exactly one of `content` or `content_file`. + - `content_file` resolves relative to the prompt YAML location. - **Profile YAML**: Includes `id`, `endpoint`, `model`, generation params, and `api_key_env`. ## 7. Guardrails diff --git a/internal/adapter/cli/run.go b/internal/adapter/cli/run.go index 6079252..1b860d1 100644 --- a/internal/adapter/cli/run.go +++ b/internal/adapter/cli/run.go @@ -61,9 +61,6 @@ type serveConfig struct { promptDir string profileDir string schemaDir string - llmBaseURL string - model string - timeout time.Duration } type listFlag []string @@ -122,9 +119,7 @@ func runCommand(args []string, stdout, stderr io.Writer) int { } llmClient, err := llm.NewOpenAICompatibleClient(llm.OpenAICompatibleConfig{ - BaseURL: cfg.llmBaseURL, - Model: cfg.model, - Timeout: cfg.timeout, + Timeout: defaults.LLMRequestTimeoutDefault, }) if err != nil { fmt.Fprintf(stderr, "llm client error: %v\n", err) @@ -184,9 +179,7 @@ func serveCommand(args []string, stderr io.Writer) int { } llmClient, err := llm.NewOpenAICompatibleClient(llm.OpenAICompatibleConfig{ - BaseURL: cfg.llmBaseURL, - Model: cfg.model, - Timeout: cfg.timeout, + Timeout: defaults.LLMRequestTimeoutDefault, }) if err != nil { fmt.Fprintf(stderr, "llm client error: %v\n", err) @@ -285,9 +278,6 @@ func parseServeArgs(args []string) (*serveConfig, error) { fs.StringVar(&cfg.promptDir, "prompt-dir", "", "directory containing prompt definition YAML files") fs.StringVar(&cfg.profileDir, "profile-dir", "", "directory containing execution profile YAML files") fs.StringVar(&cfg.schemaDir, "schema-dir", defaults.SchemaDirDefault, "base directory for validation schemas") - fs.StringVar(&cfg.llmBaseURL, "llm-base-url", "", "OpenAI-compatible base URL including /v1") - fs.StringVar(&cfg.model, "model", "", "optional default model") - fs.DurationVar(&cfg.timeout, "timeout", defaults.LLMRequestTimeoutDefault, "LLM request timeout") if err := fs.Parse(args); err != nil { return nil, err @@ -397,5 +387,5 @@ func printSummary(stderr io.Writer, res *domain.RunResult) { func printUsage(w io.Writer) { fmt.Fprintln(w, "usage: scriptorium ...") fmt.Fprintln(w, " run: scriptorium run --prompt-dir DIR --profile-dir DIR --prompt ID --input name=path [--input ...] [--profile ID] [--llm-base-url URL] [--model NAME] [--api-key-env ENV] [--temperature N] [--max-tokens N] [--top-p N] [--var k=v] [--out path] [--timeout 10m]") - fmt.Fprintf(w, " serve: scriptorium serve --addr %s --prompt-dir DIR --profile-dir DIR [--llm-base-url URL] [--schema-dir DIR] [--model NAME] [--timeout 10m]\n", defaults.HTTPAddrDefault) + fmt.Fprintf(w, " serve: scriptorium serve --addr %s --prompt-dir DIR --profile-dir DIR [--schema-dir DIR]\n", defaults.HTTPAddrDefault) } diff --git a/internal/adapter/cli/run_test.go b/internal/adapter/cli/run_test.go index 65c78e0..c3bc925 100644 --- a/internal/adapter/cli/run_test.go +++ b/internal/adapter/cli/run_test.go @@ -155,8 +155,24 @@ func TestParseServeArgsRequiredFlags(t *testing.T) { if cfg.addr != defaults.HTTPAddrDefault { t.Fatalf("expected default addr %s, got %q", defaults.HTTPAddrDefault, cfg.addr) } - if cfg.timeout != defaults.LLMRequestTimeoutDefault { - t.Fatalf("expected default timeout %s, got %s", defaults.LLMRequestTimeoutDefault, cfg.timeout) + if cfg.schemaDir != defaults.SchemaDirDefault { + t.Fatalf("expected default schema dir %q, got %q", defaults.SchemaDirDefault, cfg.schemaDir) + } +} + +func TestParseServeArgsRejectsRuntimeOverrideFlags(t *testing.T) { + base := []string{"--prompt-dir", "./prompts", "--profile-dir", "./profiles"} + tests := [][]string{ + {"--llm-base-url", "http://localhost:8000/v1"}, + {"--model", "gpt-4o-mini"}, + {"--timeout", "30s"}, + } + + for _, tc := range tests { + _, err := parseServeArgs(append(base, tc...)) + if err == nil { + t.Fatalf("expected unknown flag error for %q", tc[0]) + } } } @@ -213,7 +229,7 @@ func TestRunCommandVarsOptional(t *testing.T) { "--profile-dir", "./profiles", "--prompt", "p", "--input", "transcript=./t.md", - "--llm-base-url", "://bad-url", + "--llm-base-url", "http://[::1", "--model", "m", }, &stdout, &stderr) @@ -223,8 +239,8 @@ func TestRunCommandVarsOptional(t *testing.T) { if strings.Contains(stderr.String(), "var parse error") { t.Fatalf("expected --var to be optional, got stderr=%q", stderr.String()) } - if !strings.Contains(stderr.String(), "llm client error") { - t.Fatalf("expected llm client error after parsing succeeds, got stderr=%q", stderr.String()) + if !strings.Contains(stderr.String(), "llm client error") && !strings.Contains(stderr.String(), "run error") { + t.Fatalf("expected post-parse execution error, got stderr=%q", stderr.String()) } if stdout.Len() != 0 { t.Fatalf("expected no stdout output on error, got %q", stdout.String()) diff --git a/internal/adapter/http/dto.go b/internal/adapter/http/dto.go index 3d91e7d..a0c62e4 100644 --- a/internal/adapter/http/dto.go +++ b/internal/adapter/http/dto.go @@ -5,12 +5,13 @@ import ( ) type runRequestDTO struct { - PromptID string `json:"prompt_id"` - PromptVersion string `json:"prompt_version,omitempty"` - ProfileID string `json:"profile_id,omitempty"` - Inputs map[string]inputRefDTO `json:"inputs"` - Vars map[string]string `json:"vars,omitempty"` - Model *modelOverrideRequestDTO `json:"model,omitempty"` + PromptID string `json:"prompt_id"` + PromptVersion string `json:"prompt_version,omitempty"` + ProfileID string `json:"profile_id,omitempty"` + Inputs map[string]inputRefDTO `json:"inputs"` + Vars map[string]string `json:"vars,omitempty"` + Model *modelOverrideRequestDTO `json:"model,omitempty"` + IncludeRawOutput bool `json:"include_raw_output,omitempty"` } type inputRefDTO struct { @@ -35,7 +36,7 @@ type runResponseDTO struct { Artifact artifactDTO `json:"artifact"` Validation validationDTO `json:"validation"` Metadata metadataDTO `json:"metadata"` - RawModelOutput string `json:"raw_model_output"` + RawModelOutput *string `json:"raw_model_output,omitempty"` } type artifactDTO struct { diff --git a/internal/adapter/http/handler.go b/internal/adapter/http/handler.go index b188a04..59303df 100644 --- a/internal/adapter/http/handler.go +++ b/internal/adapter/http/handler.go @@ -90,7 +90,7 @@ func (h *Handler) ServeHTTP(w http.ResponseWriter, r *http.Request) { return } - writeJSON(w, http.StatusOK, runResponseDTO{ + resp := runResponseDTO{ Artifact: artifactDTO{ Name: res.Artifact.Name, ContentType: res.Artifact.ContentType, @@ -133,8 +133,12 @@ func (h *Handler) ServeHTTP(w http.ResponseWriter, r *http.Request) { ValidationStatus: string(res.Validation.Status), RepairAttemptsUsed: res.Validation.RepairAttempts, }, - RawModelOutput: res.RawOutput, - }) + } + if req.IncludeRawOutput { + raw := res.RawOutput + resp.RawModelOutput = &raw + } + writeJSON(w, http.StatusOK, resp) } func mapValidation(v domain.ValidationResult) validationDTO { diff --git a/internal/adapter/http/handler_test.go b/internal/adapter/http/handler_test.go index 21b0aca..0cf8a48 100644 --- a/internal/adapter/http/handler_test.go +++ b/internal/adapter/http/handler_test.go @@ -117,6 +117,9 @@ func TestHandlerPostRunsSuccessWithExplicitProfileID(t *testing.T) { if strings.Contains(w.Body.String(), secret) { t.Fatalf("response leaked raw API key value: %s", w.Body.String()) } + if _, ok := resp["raw_model_output"]; ok { + t.Fatalf("expected raw_model_output to be omitted by default, got %#v", resp["raw_model_output"]) + } if r.last.PromptID != "prompt-1" { t.Fatalf("expected request prompt_id prompt-1, got %q", r.last.PromptID) @@ -265,7 +268,7 @@ func TestHandlerRawAPIKeyRejectedByStrictJSON(t *testing.T) { } } -func TestHandlerValidationFailureStillSuccess(t *testing.T) { +func TestHandlerValidationFailureStillSuccessAndRawOutputOptIn(t *testing.T) { h := NewHandler(&fakeRunner{result: &domain.RunResult{ Artifact: domain.Artifact{Body: []byte("bad json")}, RawOutput: "bad json", @@ -292,6 +295,24 @@ func TestHandlerValidationFailureStillSuccess(t *testing.T) { if status, ok := validation["status"].(string); !ok || status != "failed" { t.Fatalf("expected validation status=failed, got %#v", validation["status"]) } + if _, ok := resp["raw_model_output"]; ok { + t.Fatalf("expected raw_model_output omitted by default, got %#v", resp["raw_model_output"]) + } + + reqWithRaw := httptest.NewRequest(http.MethodPost, "/v1/runs", bytes.NewBufferString(`{"prompt_id":"p","inputs":{"x":{"type":"file","uri":"a"}},"include_raw_output":true}`)) + wWithRaw := httptest.NewRecorder() + h.ServeHTTP(wWithRaw, reqWithRaw) + + if wWithRaw.Code != http.StatusOK { + t.Fatalf("expected 200, got %d: %s", wWithRaw.Code, wWithRaw.Body.String()) + } + var respWithRaw map[string]any + if err := json.Unmarshal(wWithRaw.Body.Bytes(), &respWithRaw); err != nil { + t.Fatalf("invalid JSON response: %v", err) + } + if got, ok := respWithRaw["raw_model_output"].(string); !ok || got != "bad json" { + t.Fatalf("expected raw_model_output to be included when requested, got %#v", respWithRaw["raw_model_output"]) + } } func wrap(stage error, cause error) error { diff --git a/internal/promptdef/repository_test.go b/internal/promptdef/repository_test.go index 050bb64..bfe7b80 100644 --- a/internal/promptdef/repository_test.go +++ b/internal/promptdef/repository_test.go @@ -41,6 +41,12 @@ func TestFilesystemRepository_GetPromptDefinition(t *testing.T) { if len(p.Templates) != 2 { t.Fatalf("expected 2 messages, got %d", len(p.Templates)) } + if len(p.Inputs) != 1 { + t.Fatalf("expected 1 input, got %d", len(p.Inputs)) + } + if p.Inputs[0].ContentType != "text/markdown" { + t.Fatalf("expected input content_type to be preserved, got %q", p.Inputs[0].ContentType) + } }) t.Run("valid file-backed prompt", func(t *testing.T) { @@ -70,6 +76,12 @@ func TestFilesystemRepository_GetPromptDefinition(t *testing.T) { if p.DefaultProfile != "local-default" { t.Fatalf("unexpected default profile: %q", p.DefaultProfile) } + if len(p.Inputs) != 1 { + t.Fatalf("expected one input, got %d", len(p.Inputs)) + } + if p.Inputs[0].ContentType != "" { + t.Fatalf("expected missing content_type to remain empty, got %q", p.Inputs[0].ContentType) + } }) t.Run("version lookup", func(t *testing.T) { @@ -94,6 +106,7 @@ func TestFilesystemRepository_GetPromptDefinition(t *testing.T) { {name: "duplicate input names", id: "duplicate_input_names", targetErr: ErrInvalidPromptDefinition, errSubstrs: []string{"duplicate input name"}}, {name: "invalid validation mode", id: "invalid_validation_mode", targetErr: ErrInvalidPromptDefinition, errSubstrs: []string{"invalid validation mode"}}, {name: "json_schema without schema_path", id: "json_schema_without_schema_path", targetErr: ErrInvalidPromptDefinition, errSubstrs: []string{"schema_path"}}, + {name: "unknown input field", id: "unknown_input_field", targetErr: ErrInvalidYAML, errSubstrs: []string{"field unknown_input_setting not found"}}, } for _, tc := range cases { diff --git a/internal/promptdef/testdata/unknown_input_field.yaml b/internal/promptdef/testdata/unknown_input_field.yaml new file mode 100644 index 0000000..d8dfa35 --- /dev/null +++ b/internal/promptdef/testdata/unknown_input_field.yaml @@ -0,0 +1,13 @@ +id: unknown-input-field +version: "1.0.0" +inputs: + - name: transcript + required: true + unknown_input_setting: true +messages: + - role: user + content: "Hi" +output: + format: text + validation_mode: none + repair_attempts: 0 diff --git a/internal/usecase/runner_test.go b/internal/usecase/runner_test.go index a6d9a40..9216b7f 100644 --- a/internal/usecase/runner_test.go +++ b/internal/usecase/runner_test.go @@ -393,6 +393,55 @@ func TestRunnerRunAPIKeyEnvMissingEnvironmentValueFailsClearly(t *testing.T) { } } +func TestRunnerRunRuntimeAPIKeyEnvOverrideWorks(t *testing.T) { + const envName = "SCRIPTORIUM_RUNTIME_API_KEY" + t.Setenv(envName, "runtime-secret") + + promptRepo := &fakePromptRepo{def: promptDef(domain.FormatText, domain.ValidationNone, 0)} + execRepo := &fakeExecutionProfileRepo{profiles: map[string]*domain.ExecutionProfile{ + "exec": {ID: "exec", Endpoint: "http://profile/v1", Model: "profile-model"}, + }} + runner := NewRunner(promptRepo, execRepo, defaultArtifactReader(), defaultRenderer(), &fakeLLM{resp: &domain.GenerateResponse{Content: "ok"}}, nil) + + res, err := runner.Run(context.Background(), domain.RunRequest{ + PromptID: "p", + ProfileID: "exec", + Inputs: singleInputRef(), + Execution: &domain.ExecutionTarget{APIKeyEnv: envName}, + }) + if err != nil { + t.Fatalf("expected no error, got %v", err) + } + if res.EffectiveModelParams.APIKeyEnv != envName { + t.Fatalf("expected runtime api_key_env override in effective params, got %q", res.EffectiveModelParams.APIKeyEnv) + } +} + +func TestRunnerRunRuntimeAPIKeyEnvOverrideBeatsProfile(t *testing.T) { + const profileEnv = "SCRIPTORIUM_PROFILE_API_KEY" + const runtimeEnv = "SCRIPTORIUM_RUNTIME_API_KEY" + t.Setenv(runtimeEnv, "runtime-secret") + + promptRepo := &fakePromptRepo{def: promptDef(domain.FormatText, domain.ValidationNone, 0)} + execRepo := &fakeExecutionProfileRepo{profiles: map[string]*domain.ExecutionProfile{ + "exec": {ID: "exec", Endpoint: "http://profile/v1", Model: "profile-model", APIKeyEnv: profileEnv}, + }} + runner := NewRunner(promptRepo, execRepo, defaultArtifactReader(), defaultRenderer(), &fakeLLM{resp: &domain.GenerateResponse{Content: "ok"}}, nil) + + res, err := runner.Run(context.Background(), domain.RunRequest{ + PromptID: "p", + ProfileID: "exec", + Inputs: singleInputRef(), + Execution: &domain.ExecutionTarget{APIKeyEnv: runtimeEnv}, + }) + if err != nil { + t.Fatalf("expected no error, got %v", err) + } + if res.EffectiveModelParams.APIKeyEnv != runtimeEnv { + t.Fatalf("expected runtime override to beat profile api_key_env, got %q", res.EffectiveModelParams.APIKeyEnv) + } +} + func TestRunnerRunAPIKeyValueNotPresentInMetadata(t *testing.T) { const envName = "SCRIPTORIUM_TEST_API_KEY" const secret = "top-secret-value"