From 1cb07c7d9114e4ce2c001972d68c329ff0e24b56 Mon Sep 17 00:00:00 2001 From: Eric Rakestraw Date: Tue, 11 Aug 2026 21:21:33 +0000 Subject: [PATCH] Centralize output contract validation --- docs/internal/overview.md | 2 +- docs/internal/runner.md | 3 +- docs/policy/architecture.md | 8 +- internal/domain/output_contract.go | 30 ++++ internal/domain/output_contract_test.go | 68 +++++++ internal/promptdef/filesystem_repository.go | 42 +---- internal/promptdef/repository_test.go | 71 ++++++++ internal/usecase/output_contract_test.go | 185 ++++++++++++++++++++ internal/usecase/runner.go | 16 +- output_contract_contract_test.go | 48 +++++ types.go | 8 +- 11 files changed, 432 insertions(+), 49 deletions(-) create mode 100644 internal/domain/output_contract.go create mode 100644 internal/domain/output_contract_test.go create mode 100644 internal/usecase/output_contract_test.go create mode 100644 output_contract_contract_test.go diff --git a/docs/internal/overview.md b/docs/internal/overview.md index 05105d7..3196e8a 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, 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/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, session identifiers, and output contracts. Source parsing, required fields, source-specific normalization and defaulting, 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/internal/runner.md b/docs/internal/runner.md index 722eb45..1014cce 100644 --- a/docs/internal/runner.md +++ b/docs/internal/runner.md @@ -69,7 +69,8 @@ performs only the work needed to validate routing and admission: 5. resolve application-neutral defaults, backend defaults, profile values, and explicit request overrides in that order; 6. validate endpoint, model, numeric overrides, and credential requirements; -7. resolve the effective output contract without loading its schema; and +7. resolve and validate the effective output contract without loading its + schema; and 8. retain the definition, source identities, effective settings, output contract, and preparation start time in invocation-local state. diff --git a/docs/policy/architecture.md b/docs/policy/architecture.md index 0b5b1ad..1f2f8f2 100644 --- a/docs/policy/architecture.md +++ b/docs/policy/architecture.md @@ -93,10 +93,10 @@ 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. +multiple input and execution boundaries, including execution-setting bounds, +session identifiers, and output-contract legality. Callers retain source +parsing, required-field rules, source-specific normalization, defaulting, +error classification, and other policy specific to their own boundary. ## Repository And Consumer Boundary diff --git a/internal/domain/output_contract.go b/internal/domain/output_contract.go new file mode 100644 index 0000000..a6919cf --- /dev/null +++ b/internal/domain/output_contract.go @@ -0,0 +1,30 @@ +package domain + +import ( + "errors" + "fmt" + "strings" +) + +// ValidateOutputContract validates source-neutral output-contract invariants. +func ValidateOutputContract(contract OutputContract) error { + switch contract.Format { + case FormatText, FormatMarkdown, FormatJSON: + default: + return fmt.Errorf("invalid output format: %q", contract.Format) + } + + switch contract.ValidationMode { + case ValidationNone, ValidationBasic, ValidationJSON, ValidationJSONSchema: + default: + return fmt.Errorf("invalid validation mode: %q", contract.ValidationMode) + } + + if contract.ValidationMode == ValidationJSONSchema && strings.TrimSpace(contract.SchemaPath) == "" { + return errors.New("schema_path is required when validation_mode is json_schema") + } + if contract.RepairAttempts < 0 { + return errors.New("repair_attempts must be greater than or equal to 0") + } + return nil +} diff --git a/internal/domain/output_contract_test.go b/internal/domain/output_contract_test.go new file mode 100644 index 0000000..b9f541a --- /dev/null +++ b/internal/domain/output_contract_test.go @@ -0,0 +1,68 @@ +package domain + +import ( + "strings" + "testing" +) + +func TestValidateOutputContract(t *testing.T) { + valid := OutputContract{ + Format: FormatText, + ValidationMode: ValidationNone, + } + + tests := []struct { + name string + change func(*OutputContract) + wantErr string + }{ + {name: "text format", change: func(c *OutputContract) { c.Format = FormatText }}, + {name: "markdown format", change: func(c *OutputContract) { c.Format = FormatMarkdown }}, + {name: "json format", change: func(c *OutputContract) { c.Format = FormatJSON }}, + {name: "empty format", change: func(c *OutputContract) { c.Format = "" }, wantErr: "format"}, + {name: "unsupported format", change: func(c *OutputContract) { c.Format = OutputFormat("binary") }, wantErr: "format"}, + {name: "none validation", change: func(c *OutputContract) { c.ValidationMode = ValidationNone }}, + {name: "basic validation", change: func(c *OutputContract) { c.ValidationMode = ValidationBasic }}, + {name: "json validation", change: func(c *OutputContract) { c.ValidationMode = ValidationJSON }}, + {name: "json schema validation", change: func(c *OutputContract) { + c.ValidationMode = ValidationJSONSchema + c.SchemaPath = "schema.json" + }}, + {name: "empty validation mode", change: func(c *OutputContract) { c.ValidationMode = "" }, wantErr: "validation mode"}, + {name: "unsupported validation mode", change: func(c *OutputContract) { c.ValidationMode = ValidationMode("unknown") }, wantErr: "validation mode"}, + {name: "negative repair attempts", change: func(c *OutputContract) { c.RepairAttempts = -1 }, wantErr: "repair_attempts"}, + {name: "zero repair attempts", change: func(c *OutputContract) { c.RepairAttempts = 0 }}, + {name: "positive repair attempts", change: func(c *OutputContract) { c.RepairAttempts = 1 }}, + {name: "json schema empty path", change: func(c *OutputContract) { + c.ValidationMode = ValidationJSONSchema + c.SchemaPath = "" + }, wantErr: "schema_path"}, + {name: "json schema whitespace path", change: func(c *OutputContract) { + c.ValidationMode = ValidationJSONSchema + c.SchemaPath = " \t " + }, wantErr: "schema_path"}, + {name: "json schema nonblank path", change: func(c *OutputContract) { + c.ValidationMode = ValidationJSONSchema + c.SchemaPath = " schema.json " + }}, + {name: "non-schema empty path", change: func(c *OutputContract) { c.SchemaPath = "" }}, + {name: "non-schema populated path", change: func(c *OutputContract) { c.SchemaPath = "ignored.json" }}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + contract := valid + tt.change(&contract) + err := ValidateOutputContract(contract) + if tt.wantErr == "" { + if err != nil { + t.Fatalf("validate output contract: %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/promptdef/filesystem_repository.go b/internal/promptdef/filesystem_repository.go index ad86817..df473e3 100644 --- a/internal/promptdef/filesystem_repository.go +++ b/internal/promptdef/filesystem_repository.go @@ -402,17 +402,14 @@ func normalizePromptDefinitionWithContent(raw *promptDefinitionFile, readContent }) } - if !isValidOutputFormat(raw.Output.Format) { - return nil, fmt.Errorf("invalid output format: %q", raw.Output.Format) + outputContract := domain.OutputContract{ + Format: raw.Output.Format, + ValidationMode: raw.Output.ValidationMode, + SchemaPath: strings.TrimSpace(raw.Output.SchemaPath), + RepairAttempts: raw.Output.RepairAttempts, } - if !isValidValidationMode(raw.Output.ValidationMode) { - return nil, fmt.Errorf("invalid validation mode: %q", raw.Output.ValidationMode) - } - if raw.Output.ValidationMode == domain.ValidationJSONSchema && strings.TrimSpace(raw.Output.SchemaPath) == "" { - return nil, errors.New("output.schema_path is required when output.validation_mode is json_schema") - } - if raw.Output.RepairAttempts < 0 { - return nil, errors.New("output.repair_attempts must be greater than or equal to 0") + if err := domain.ValidateOutputContract(outputContract); err != nil { + return nil, fmt.Errorf("output: %w", err) } defaultProfile := "" @@ -432,12 +429,7 @@ func normalizePromptDefinitionWithContent(raw *promptDefinitionFile, readContent Inputs: inputs, Templates: templates, OutputFormat: raw.Output.Format, - Validation: domain.OutputContract{ - Format: raw.Output.Format, - ValidationMode: raw.Output.ValidationMode, - SchemaPath: strings.TrimSpace(raw.Output.SchemaPath), - RepairAttempts: raw.Output.RepairAttempts, - }, + Validation: outputContract, }, nil } @@ -464,21 +456,3 @@ func normalizeCacheControl(raw *cacheControlFile) (*domain.CacheControl, error) TTL: ttl, }, nil } - -func isValidOutputFormat(f domain.OutputFormat) bool { - switch f { - case domain.FormatText, domain.FormatMarkdown, domain.FormatJSON: - return true - default: - return false - } -} - -func isValidValidationMode(m domain.ValidationMode) bool { - switch m { - case domain.ValidationNone, domain.ValidationBasic, domain.ValidationJSON, domain.ValidationJSONSchema: - return true - default: - return false - } -} diff --git a/internal/promptdef/repository_test.go b/internal/promptdef/repository_test.go index d957326..1d2c0a5 100644 --- a/internal/promptdef/repository_test.go +++ b/internal/promptdef/repository_test.go @@ -479,6 +479,77 @@ output: } } +func TestPromptRepositoriesApplyOutputContractRules(t *testing.T) { + tests := []struct { + name string + output string + useFilesystem bool + wantErr bool + wantDiagnostic string + wantSchemaPath string + }{ + { + name: "operating-system source rejects unsupported format", + output: " format: binary\n validation_mode: none\n", + useFilesystem: true, + wantErr: true, + wantDiagnostic: "format", + }, + { + name: "fs source rejects negative repair attempts", + output: " format: text\n validation_mode: none\n repair_attempts: -1\n", + wantErr: true, + wantDiagnostic: "repair_attempts", + }, + { + name: "source normalization trims a valid schema path", + output: " format: json\n validation_mode: json_schema\n schema_path: ' schema.json '\n", + useFilesystem: true, + wantSchemaPath: "schema.json", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + data := `id: output-contract +version: "1" +messages: + - role: user + content: test +output: +` + tt.output + + var repo Repository + if tt.useFilesystem { + dir := t.TempDir() + writePromptTestFile(t, filepath.Join(dir, "output-contract.yaml"), data) + repo = NewFilesystemRepository(dir) + } else { + repo = NewFSRepository(fstest.MapFS{ + "output-contract.yaml": &fstest.MapFile{Data: []byte(data)}, + }, ".") + } + + got, err := repo.GetPromptDefinition(context.Background(), "output-contract", "") + if tt.wantErr { + if !errors.Is(err, ErrInvalidPromptDefinition) { + t.Fatalf("expected ErrInvalidPromptDefinition, got %v", err) + } + if !strings.Contains(err.Error(), tt.wantDiagnostic) { + t.Fatalf("expected error containing %q, got %v", tt.wantDiagnostic, err) + } + return + } + if err != nil { + t.Fatalf("load prompt definition: %v", err) + } + if got.Validation.SchemaPath != tt.wantSchemaPath { + t.Fatalf("schema path = %q, want %q", got.Validation.SchemaPath, tt.wantSchemaPath) + } + }) + } +} + func assertCacheControl(t *testing.T, got *domain.CacheControl, wantType domain.CacheControlType, wantTTL string) { t.Helper() if got == nil { diff --git a/internal/usecase/output_contract_test.go b/internal/usecase/output_contract_test.go new file mode 100644 index 0000000..17cb080 --- /dev/null +++ b/internal/usecase/output_contract_test.go @@ -0,0 +1,185 @@ +package usecase + +import ( + "context" + "errors" + "testing" + + "gitea.maximumdirect.net/eric/promptkit/internal/domain" +) + +type outputContractTestCollaborators struct { + artifacts *fakeArtifactReader + renderer *fakeRenderer + llm *fakeLLM + validator *recordingValidationPreparer + admitter *fakeRunAdmitter +} + +func newOutputContractTestRunner() (*Runner, outputContractTestCollaborators) { + collaborators := outputContractTestCollaborators{ + artifacts: defaultArtifactReader(), + renderer: defaultRenderer(), + llm: &fakeLLM{forbid: true}, + validator: &recordingValidationPreparer{plan: &recordingPreparedValidation{}}, + admitter: &fakeRunAdmitter{}, + } + return NewRunner( + &fakePromptRepo{def: promptDef(domain.FormatText, domain.ValidationNone, 0)}, + &fakeExecutionProfileRepo{profiles: map[string]*domain.ExecutionProfile{"exec": defaultExecutionProfile()}}, + nil, + collaborators.artifacts, + collaborators.renderer, + collaborators.llm, + collaborators.validator, + collaborators.admitter, + ), collaborators +} + +func TestRunnerPreparationNormalizesOutputContractConsistently(t *testing.T) { + tests := []struct { + name string + override domain.OutputContract + want domain.OutputContract + }{ + { + name: "empty replacement format defaults to text", + override: domain.OutputContract{ValidationMode: domain.ValidationNone}, + want: domain.OutputContract{Format: domain.FormatText, ValidationMode: domain.ValidationNone}, + }, + { + name: "markdown basic replacement", + override: domain.OutputContract{Format: domain.FormatMarkdown, ValidationMode: domain.ValidationBasic}, + want: domain.OutputContract{Format: domain.FormatMarkdown, ValidationMode: domain.ValidationBasic}, + }, + { + name: "json replacement preserves non-schema fields", + override: domain.OutputContract{ + Format: domain.FormatJSON, + ValidationMode: domain.ValidationJSON, + SchemaPath: "ignored.json", + RepairAttempts: 2, + }, + want: domain.OutputContract{ + Format: domain.FormatJSON, + ValidationMode: domain.ValidationJSON, + SchemaPath: "ignored.json", + RepairAttempts: 2, + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + runner, _ := newOutputContractTestRunner() + req := domain.RunRequest{ + PromptID: "p", + ProfileID: "exec", + Inputs: singleInputRef(), + Validation: &tt.override, + } + + prepared, err := runner.Prepare(context.Background(), req) + if err != nil { + t.Fatalf("prepare: %v", err) + } + preparedExecution, err := runner.PrepareExecution(context.Background(), req) + if err != nil { + t.Fatalf("prepare execution: %v", err) + } + details := preparedExecution.Details() + if details == nil { + t.Fatal("prepared execution returned nil details") + } + if prepared.OutputContract != tt.want || details.OutputContract != tt.want { + t.Fatalf("output contracts = (%+v, %+v), want %+v", prepared.OutputContract, details.OutputContract, tt.want) + } + }) + } +} + +func TestRunnerPreparationRejectsInvalidOutputContractsBeforeCompletion(t *testing.T) { + tests := []struct { + name string + override domain.OutputContract + }{ + {name: "unsupported format", override: domain.OutputContract{Format: "binary", ValidationMode: domain.ValidationNone}}, + {name: "empty validation mode", override: domain.OutputContract{Format: domain.FormatText}}, + {name: "negative repair attempts", override: domain.OutputContract{Format: domain.FormatText, ValidationMode: domain.ValidationNone, RepairAttempts: -1}}, + {name: "json schema without path", override: domain.OutputContract{Format: domain.FormatJSON, ValidationMode: domain.ValidationJSONSchema}}, + } + + for _, tt := range tests { + for _, operation := range []string{"Prepare", "PrepareExecution"} { + t.Run(tt.name+"/"+operation, func(t *testing.T) { + runner, collaborators := newOutputContractTestRunner() + req := domain.RunRequest{ + PromptID: "p", + ProfileID: "exec", + Inputs: singleInputRef(), + Validation: &tt.override, + } + + var err error + switch operation { + case "Prepare": + var prepared *domain.PreparedRun + prepared, err = runner.Prepare(context.Background(), req) + if prepared != nil { + t.Fatalf("expected no partial prepared run, got %+v", prepared) + } + case "PrepareExecution": + var prepared *PreparedExecution + prepared, err = runner.PrepareExecution(context.Background(), req) + if prepared != nil { + t.Fatalf("expected no partial prepared execution, got %+v", prepared) + } + default: + t.Fatalf("unknown operation %q", operation) + } + + if !errors.Is(err, ErrInvalidRequest) { + t.Fatalf("expected ErrInvalidRequest, got %v", err) + } + assertOutputContractCompletionSkipped(t, collaborators) + }) + } + } +} + +func TestRunnerRunRejectsInvalidOutputContractBeforeAdmission(t *testing.T) { + runner, collaborators := newOutputContractTestRunner() + result, err := runner.Run(context.Background(), domain.RunRequest{ + PromptID: "p", + ProfileID: "exec", + Inputs: singleInputRef(), + Validation: &domain.OutputContract{ + Format: domain.OutputFormat("binary"), + ValidationMode: domain.ValidationNone, + }, + }) + if result != nil { + t.Fatalf("expected no partial result, got %+v", result) + } + if !errors.Is(err, ErrInvalidRequest) { + t.Fatalf("expected ErrInvalidRequest, got %v", err) + } + assertOutputContractCompletionSkipped(t, collaborators) +} + +func assertOutputContractCompletionSkipped(t *testing.T, collaborators outputContractTestCollaborators) { + t.Helper() + if collaborators.artifacts.calls != 0 || collaborators.renderer.calls != 0 || + collaborators.validator.prepareCalls != 0 || collaborators.validator.directValidateCalls != 0 || + len(collaborators.admitter.backendIDs) != 0 || collaborators.llm.calls != 0 { + t.Fatalf( + "invalid output contract reached downstream work: artifacts=%d renderer=%d prepare_validation=%d validation=%d admissions=%d generation=%d", + collaborators.artifacts.calls, + collaborators.renderer.calls, + collaborators.validator.prepareCalls, + collaborators.validator.directValidateCalls, + len(collaborators.admitter.backendIDs), + collaborators.llm.calls, + ) + } +} diff --git a/internal/usecase/runner.go b/internal/usecase/runner.go index 2466742..35f64d4 100644 --- a/internal/usecase/runner.go +++ b/internal/usecase/runner.go @@ -272,6 +272,10 @@ func (r *Runner) resolvePreparation( } def := promptSelection.definition promptDefinitionHash := promptSelection.hash + effectiveContract, err := resolveOutputContract(def, req.Validation) + if err != nil { + return nil, fmt.Errorf("%w: output contract: %v", ErrInvalidRequest, err) + } selectedProfileID := strings.TrimSpace(req.ProfileID) if selectedProfileID == "" { @@ -295,7 +299,6 @@ func (r *Runner) resolvePreparation( return nil, fmt.Errorf("%w: %w", ErrInvalidRequest, err) } - effectiveContract := resolveOutputContract(def, req.Validation) return &preparationState{ definition: def, directSessionID: directSessionID, @@ -639,18 +642,21 @@ func copyExtraParams(src map[string]any) map[string]any { return cp } -func resolveOutputContract(def *domain.PromptDefinition, override *domain.OutputContract) domain.OutputContract { +func resolveOutputContract(def *domain.PromptDefinition, override *domain.OutputContract) (domain.OutputContract, error) { contract := def.Validation if contract.Format == "" { contract.Format = def.OutputFormat } if override != nil { contract = *override + if contract.Format == "" { + contract.Format = domain.FormatText + } } - if contract.Format == "" { - contract.Format = domain.FormatText + if err := domain.ValidateOutputContract(contract); err != nil { + return domain.OutputContract{}, err } - return contract + return contract, nil } func hashRenderedPrompt(p domain.RenderedPrompt) string { diff --git a/output_contract_contract_test.go b/output_contract_contract_test.go new file mode 100644 index 0000000..738db8d --- /dev/null +++ b/output_contract_contract_test.go @@ -0,0 +1,48 @@ +package promptkit_test + +import ( + "context" + "errors" + "testing" + + "gitea.maximumdirect.net/eric/promptkit" +) + +func TestPreparationRejectsInvalidOutputContractWithPublicError(t *testing.T) { + engine, err := promptkit.NewEngine( + promptkit.Config{}, + promptkit.WithPromptFS(contractPromptFS("prompt", "profile", "message"), "."), + promptkit.WithProfiles(promptkit.Profile{ + ID: "profile", + Endpoint: "http://example.test/v1", + Model: "model", + }), + ) + if err != nil { + t.Fatalf("construct engine: %v", err) + } + + req := promptkit.RunRequest{ + PromptID: "prompt", + Validation: &promptkit.OutputContract{ + Format: promptkit.OutputFormat("binary"), + ValidationMode: promptkit.ValidationNone, + }, + } + + prepared, err := engine.Prepare(context.Background(), req) + if prepared != nil { + t.Fatalf("expected no partial prepared run, got %+v", prepared) + } + if !errors.Is(err, promptkit.ErrInvalidRequest) { + t.Fatalf("prepare error = %v, want ErrInvalidRequest", err) + } + + preparedExecution, err := engine.PrepareExecution(context.Background(), req) + if preparedExecution != nil { + t.Fatalf("expected no partial prepared execution, got %+v", preparedExecution) + } + if !errors.Is(err, promptkit.ErrInvalidRequest) { + t.Fatalf("prepare execution error = %v, want ErrInvalidRequest", err) + } +} diff --git a/types.go b/types.go index 4d3288b..256bc40 100644 --- a/types.go +++ b/types.go @@ -541,8 +541,8 @@ type ExecutionTargetPresence struct { // does not merge fields. The public Engine validates generated output once and // does not install an output repairer. type OutputContract struct { - // Format selects generated artifact metadata. An empty effective value - // defaults to FormatText. + // Format selects generated artifact metadata. An empty value in a non-nil + // request replacement defaults to FormatText. Format OutputFormat `json:"format"` // ValidationMode selects the content check. Use one of the declared // ValidationMode constants. @@ -550,8 +550,8 @@ type OutputContract struct { // SchemaPath is required when ValidationMode is ValidationJSONSchema and is // ignored by other modes. SchemaPath string `json:"schema_path"` - // RepairAttempts is a requested repair limit. A non-positive value requests - // no repairs. The public Engine performs no repairs even when this value is + // RepairAttempts is a non-negative requested repair limit. Zero requests no + // repairs. The public Engine performs no repairs even when this value is // positive, so its runs report zero attempts used. RepairAttempts int `json:"repair_attempts"` }