From 39388e96d4c28000ce6db9580f39473ee0a01a7b Mon Sep 17 00:00:00 2001 From: Eric Rakestraw Date: Mon, 3 Aug 2026 18:35:40 +0000 Subject: [PATCH] Make PromptKit profile handling safer and more consistent --- internal/cli/catalog.go | 11 --- internal/cli/production_contract_test.go | 93 ++++++++++++------- internal/cli/promptkit_profiles_test.go | 13 ++- internal/cli/run.go | 12 ++- internal/core/config/file_config.go | 3 + .../core/config/file_config_contract_test.go | 21 +++++ .../framework/llm/promptkit_client_test.go | 27 ++++++ .../llm/promptkit_profile_fingerprint.go | 35 +++++-- internal/framework/llm/promptkit_profiles.go | 24 ++++- .../framework/pipeline/chunker_registry.go | 4 - .../pipeline/chunker_registry_test.go | 14 ++- .../pipeline/evidence_preparation_test.go | 2 +- internal/framework/pipeline/input_registry.go | 4 - .../framework/pipeline/input_registry_test.go | 30 +++--- internal/framework/pipeline/module.go | 8 -- .../framework/pipeline/output_registry.go | 4 - .../pipeline/output_registry_test.go | 10 +- .../framework/pipeline/preparation_test.go | 12 +-- .../seriatim/input/transcript/runner_test.go | 4 +- 19 files changed, 220 insertions(+), 111 deletions(-) diff --git a/internal/cli/catalog.go b/internal/cli/catalog.go index f74afdb..d65ded9 100644 --- a/internal/cli/catalog.go +++ b/internal/cli/catalog.go @@ -143,17 +143,6 @@ func isEmptyRegistries(registries pipeline.Registries) bool { registries.Outputs == nil } -func productionLLMClientFactory(ctx context.Context, cfg config.Config, profileID string, overrides LLMRuntimeOverrides) (contracts.StructuredLLMClient, []artifacts.LLMProfileManifest, error) { - if err := ctx.Err(); err != nil { - return nil, nil, err - } - assets, err := productionPromptAssets() - if err != nil { - return nil, nil, err - } - return buildProductionLLMClient(ctx, cfg, profileID, overrides, assets) -} - func productionLLMClientFactoryWithAssets(assets *llm.AssetRegistry) LLMClientFactory { return func(ctx context.Context, cfg config.Config, profileID string, overrides LLMRuntimeOverrides) (contracts.StructuredLLMClient, []artifacts.LLMProfileManifest, error) { return buildProductionLLMClient(ctx, cfg, profileID, overrides, assets) diff --git a/internal/cli/production_contract_test.go b/internal/cli/production_contract_test.go index 943d30f..d790937 100644 --- a/internal/cli/production_contract_test.go +++ b/internal/cli/production_contract_test.go @@ -474,39 +474,67 @@ func setNormalizeSpellCatalogSource(t *testing.T, resolved *pipeline.ResolvedPip resolved.Steps[0].ArtifactLanes[0].NormalizeReferences.Bindings = bindings } -func TestProductionLLMClientFactoriesBuildOfflineRuntime(t *testing.T) { +func TestProductionLLMClientFactoryBuildsOfflineRuntime(t *testing.T) { components := productionTestComponents(t) - factories := []struct { - name string - factory LLMClientFactory - }{ - {name: "default production assets", factory: productionLLMClientFactory}, - {name: "provided production assets", factory: productionLLMClientFactoryWithAssets(components.assets)}, + client, manifests, err := productionLLMClientFactoryWithAssets(components.assets)(context.Background(), config.Default(), "test-profile", LLMRuntimeOverrides{}) + if err != nil { + t.Fatalf("build production LLM runtime: %v", err) } - for _, tt := range factories { - t.Run(tt.name, func(t *testing.T) { - client, manifests, err := tt.factory(context.Background(), config.Default(), "test-profile", LLMRuntimeOverrides{}) - if err != nil { - t.Fatalf("build production LLM runtime: %v", err) - } - if client == nil { - t.Fatal("production LLM runtime returned a nil client") - } - if len(manifests) != 0 { - t.Fatalf("eager profile manifests = %#v, want none", manifests) - } - fingerprintProvider, ok := client.(llm.CheckpointFingerprintProvider) - if !ok { - t.Fatalf("production LLM client %T does not provide one profile-source checkpoint fingerprint", client) - } - fingerprints, err := fingerprintProvider.LLMCheckpointFingerprints() - if err != nil || len(fingerprints) != 1 { - t.Fatalf("production LLM checkpoint fingerprints = %#v, error = %v, want one profile-source identity", fingerprints, err) - } - if _, ok := client.(contracts.LLMProfileManifestProvider); !ok { - t.Fatalf("production LLM client %T does not provide profile manifests", client) - } - }) + if client == nil { + t.Fatal("production LLM runtime returned a nil client") + } + if len(manifests) != 0 { + t.Fatalf("eager profile manifests = %#v, want none", manifests) + } + fingerprintProvider, ok := client.(llm.CheckpointFingerprintProvider) + if !ok { + t.Fatalf("production LLM client %T does not provide one profile-source checkpoint fingerprint", client) + } + fingerprints, err := fingerprintProvider.LLMCheckpointFingerprints() + if err != nil || len(fingerprints) != 1 { + t.Fatalf("production LLM checkpoint fingerprints = %#v, error = %v, want one profile-source identity", fingerprints, err) + } + if _, ok := client.(contracts.LLMProfileManifestProvider); !ok { + t.Fatalf("production LLM client %T does not provide profile manifests", client) + } +} + +func TestNormalizeOptionsSharesProductionProfileAssetsWithDefaultRuntime(t *testing.T) { + opts, err := normalizeOptions(Options{ + Catalog: pipeline.ModuleCatalog{Inputs: pipeline.NewInputAdapterRegistry()}, + }) + if err != nil { + t.Fatal(err) + } + if opts.promptKitAssets == nil || opts.LLMClientFactory == nil { + t.Fatalf("normalized options = %#v, want shared profile assets and default runtime factory", opts) + } + if err := validateExplicitPromptKitProfiles(context.Background(), config.Default(), []string{"dnd-extraction"}, opts.promptKitAssets); err != nil { + t.Fatalf("inspect application fallback profile: %v", err) + } + + client, _, err := opts.LLMClientFactory(context.Background(), config.Default(), "dnd-extraction", LLMRuntimeOverrides{}) + if err != nil { + t.Fatalf("build default runtime: %v", err) + } + fingerprintProvider, ok := client.(llm.CheckpointFingerprintProvider) + if !ok { + t.Fatalf("default runtime client %T does not provide checkpoint fingerprints", client) + } + runtimeFingerprints, err := fingerprintProvider.LLMCheckpointFingerprints() + if err != nil { + t.Fatal(err) + } + directClient, err := llm.NewPromptKitClient(llm.PromptKitClientConfig{Assets: opts.promptKitAssets}) + if err != nil { + t.Fatal(err) + } + inspectionFingerprints, err := directClient.LLMCheckpointFingerprints() + if err != nil { + t.Fatal(err) + } + if !reflect.DeepEqual(runtimeFingerprints, inspectionFingerprints) { + t.Fatalf("runtime profile fingerprints = %#v, inspection profile fingerprints = %#v", runtimeFingerprints, inspectionFingerprints) } } @@ -597,7 +625,8 @@ func TestProductionLLMClientFactoriesRejectInvalidConstruction(t *testing.T) { t.Run("canceled context", func(t *testing.T) { ctx, cancel := context.WithCancel(context.Background()) cancel() - client, manifests, err := productionLLMClientFactory(ctx, config.Default(), "test-profile", LLMRuntimeOverrides{}) + components := productionTestComponents(t) + client, manifests, err := productionLLMClientFactoryWithAssets(components.assets)(ctx, config.Default(), "test-profile", LLMRuntimeOverrides{}) if !errors.Is(err, context.Canceled) || client != nil || len(manifests) != 0 { t.Fatalf("client=%T manifests=%#v error=%v, want canceled construction", client, manifests, err) } diff --git a/internal/cli/promptkit_profiles_test.go b/internal/cli/promptkit_profiles_test.go index 9e23ccc..6a76dc9 100644 --- a/internal/cli/promptkit_profiles_test.go +++ b/internal/cli/promptkit_profiles_test.go @@ -47,6 +47,7 @@ api_key_env: NOTARIUS_PROMPTKIT_PROFILE_INSPECTION_TEST_KEY localBackend bool canceled bool wantErr []string + rejectErr []string }{ { name: "configured local backend", @@ -59,7 +60,7 @@ api_key_env: NOTARIUS_PROMPTKIT_PROFILE_INSPECTION_TEST_KEY name: "missing local backend registration", profilePath: writeProfile(t, "local-profile", localProfile), profileID: "local-profile", - wantErr: []string{`inspect PromptKit profile "local-profile"`, `backend "local"`}, + wantErr: []string{`PromptKit profile "local-profile" is invalid or unreadable`}, }, { name: "absent profile", @@ -72,13 +73,14 @@ api_key_env: NOTARIUS_PROMPTKIT_PROFILE_INSPECTION_TEST_KEY name: "malformed profile", profilePath: writeProfile(t, "malformed-profile", "id: malformed-profile\nbackend: [\n"), profileID: "malformed-profile", - wantErr: []string{`inspect PromptKit profile "malformed-profile"`}, + wantErr: []string{`PromptKit profile "malformed-profile" is invalid or unreadable`}, + rejectErr: []string{"malformed-profile.yaml", "backend: ["}, }, { name: "invalid profile source", profilePath: filepath.Join(t.TempDir(), "missing-profile.yaml"), profileID: "missing-profile", - wantErr: []string{"load PromptKit profiles", "failed to access source file"}, + wantErr: []string{"load PromptKit profiles", "profile configuration is invalid or unreadable"}, }, { name: "credential environment intentionally unset", @@ -132,6 +134,11 @@ api_key_env: NOTARIUS_PROMPTKIT_PROFILE_INSPECTION_TEST_KEY t.Fatalf("validation error = %q, want %q", err, want) } } + for _, rejected := range append(tt.rejectErr, tt.profilePath) { + if rejected != "" && strings.Contains(err.Error(), rejected) { + t.Fatalf("validation error = %q, must not expose %q", err, rejected) + } + } }) } if providerCalls.Load() != 0 { diff --git a/internal/cli/run.go b/internal/cli/run.go index 3c361d6..72d7c3a 100644 --- a/internal/cli/run.go +++ b/internal/cli/run.go @@ -123,12 +123,16 @@ func normalizeOptions(opts Options) (Options, error) { opts.Registries = components.registries opts.Catalog = catalogFromRegistries(components.registries) opts.promptKitAssets = components.assets - if opts.LLMClientFactory == nil { - opts.LLMClientFactory = productionLLMClientFactoryWithAssets(components.assets) - } } if opts.LLMClientFactory == nil { - opts.LLMClientFactory = productionLLMClientFactory + if opts.promptKitAssets == nil { + assets, err := productionPromptAssets() + if err != nil { + return Options{}, err + } + opts.promptKitAssets = assets + } + opts.LLMClientFactory = productionLLMClientFactoryWithAssets(opts.promptKitAssets) } return opts, nil } diff --git a/internal/core/config/file_config.go b/internal/core/config/file_config.go index f3c5024..d49c32f 100644 --- a/internal/core/config/file_config.go +++ b/internal/core/config/file_config.go @@ -260,6 +260,9 @@ func (b *fileModuleBinding) UnmarshalYAML(node *yaml.Node) error { return err } b.LLMProfile = strings.TrimSpace(llmProfile) + if b.LLMProfile == "" { + return fmt.Errorf("llm_profile must not be empty when set") + } case "retries": var retries int if err := valueNode.Decode(&retries); err != nil { diff --git a/internal/core/config/file_config_contract_test.go b/internal/core/config/file_config_contract_test.go index d4a7af0..dc0dba6 100644 --- a/internal/core/config/file_config_contract_test.go +++ b/internal/core/config/file_config_contract_test.go @@ -116,6 +116,27 @@ pipelines: } } +func TestFileModuleBindingRejectsExplicitEmptyLLMProfile(t *testing.T) { + const configYAML = `version: 4 +pipelines: + main: + input: + module: input + llm_profile: %s + artifacts: + lane: + extract: extract +` + for _, value := range []string{"''", "' '", "null"} { + t.Run(value, func(t *testing.T) { + _, err := ParseFileConfigYAML([]byte(fmt.Sprintf(configYAML, value))) + if err == nil || !strings.Contains(err.Error(), "llm_profile must not be empty when set") { + t.Fatalf("ParseFileConfigYAML() error = %v, want explicit-empty binding profile rejection", err) + } + }) + } +} + func TestFilePromptKitProfileSourcesSurviveConfigBoundaries(t *testing.T) { tests := []struct { name string diff --git a/internal/framework/llm/promptkit_client_test.go b/internal/framework/llm/promptkit_client_test.go index 0273ce7..0465fe1 100644 --- a/internal/framework/llm/promptkit_client_test.go +++ b/internal/framework/llm/promptkit_client_test.go @@ -429,6 +429,33 @@ func TestPromptKitClientCheckpointFingerprintTracksProfileSource(t *testing.T) { if strings.TrimSpace(fresh[0].Value) == "" { t.Fatal("built-in profile fingerprint is empty") } + + t.Run("directory layout", func(t *testing.T) { + profileDir := t.TempDir() + firstPath := filepath.Join(profileDir, "first-profile.yaml") + secondPath := filepath.Join(profileDir, "second-profile.yaml") + content := []byte("id: directory-profile\nendpoint: http://promptkit.test/v1\nmodel: directory-model\n") + if err := os.WriteFile(firstPath, content, 0o600); err != nil { + t.Fatal(err) + } + first, err := promptKitProfileFingerprint(profileDir, "", "") + if err != nil { + t.Fatal(err) + } + if err := os.Rename(firstPath, secondPath); err != nil { + t.Fatal(err) + } + second, err := promptKitProfileFingerprint(profileDir, "", "") + if err != nil { + t.Fatal(err) + } + if first == second { + t.Fatalf("profile-source fingerprint = %#v after source filename changed", first) + } + if strings.Contains(first.Value, firstPath) || strings.Contains(second.Value, secondPath) { + t.Fatalf("profile-source fingerprint exposes source path: %#v, %#v", first, second) + } + }) } func TestPromptKitProfileFingerprintReadErrorsDoNotExposeSourcePaths(t *testing.T) { diff --git a/internal/framework/llm/promptkit_profile_fingerprint.go b/internal/framework/llm/promptkit_profile_fingerprint.go index 1a33fc6..f774dee 100644 --- a/internal/framework/llm/promptkit_profile_fingerprint.go +++ b/internal/framework/llm/promptkit_profile_fingerprint.go @@ -27,18 +27,21 @@ func promptKitProfileFingerprint(profileDir, profileFile, fallbackProfileDigest switch { case strings.TrimSpace(profileFile) != "": - data, err := os.ReadFile(strings.TrimSpace(profileFile)) + cleanProfileFile := strings.TrimSpace(profileFile) + data, err := os.ReadFile(cleanProfileFile) if err != nil { return CheckpointFingerprint{}, fmt.Errorf("read PromptKit profile file for checkpoint identity") } + writeFingerprintPart(hasher, []byte(filepath.ToSlash(filepath.Base(cleanProfileFile)))) writeFingerprintPart(hasher, data) case strings.TrimSpace(profileDir) != "": - digests, err := promptKitProfileFileDigests(strings.TrimSpace(profileDir)) + files, err := promptKitProfileFiles(strings.TrimSpace(profileDir)) if err != nil { return CheckpointFingerprint{}, err } - for _, digest := range digests { - writeFingerprintPart(hasher, digest) + for _, file := range files { + writeFingerprintPart(hasher, []byte(file.path)) + writeFingerprintPart(hasher, file.digest) } } @@ -58,8 +61,13 @@ func promptKitLocalBackendFingerprint(endpoint string) CheckpointFingerprint { } } -func promptKitProfileFileDigests(root string) ([][]byte, error) { - var digests [][]byte +type promptKitProfileFile struct { + path string + digest []byte +} + +func promptKitProfileFiles(root string) ([]promptKitProfileFile, error) { + var files []promptKitProfileFile err := filepath.WalkDir(root, func(name string, entry fs.DirEntry, walkErr error) error { if walkErr != nil { return walkErr @@ -75,17 +83,24 @@ func promptKitProfileFileDigests(root string) ([][]byte, error) { if err != nil { return err } + relativePath, err := filepath.Rel(root, name) + if err != nil { + return err + } sum := sha256.Sum256(data) - digests = append(digests, append([]byte(nil), sum[:]...)) + files = append(files, promptKitProfileFile{ + path: filepath.ToSlash(relativePath), + digest: append([]byte(nil), sum[:]...), + }) return nil }) if err != nil { return nil, fmt.Errorf("read PromptKit profile directory for checkpoint identity") } - sort.Slice(digests, func(i, j int) bool { - return string(digests[i]) < string(digests[j]) + sort.Slice(files, func(i, j int) bool { + return files[i].path < files[j].path }) - return digests, nil + return files, nil } func writeFingerprintPart(hasher interface{ Write([]byte) (int, error) }, value []byte) { diff --git a/internal/framework/llm/promptkit_profiles.go b/internal/framework/llm/promptkit_profiles.go index a18cb8b..802d816 100644 --- a/internal/framework/llm/promptkit_profiles.go +++ b/internal/framework/llm/promptkit_profiles.go @@ -45,16 +45,34 @@ type PromptKitProfileInspectionError struct { } func (e *PromptKitProfileInspectionError) Error() string { - if errors.Is(e.err, promptkit.ErrProfileNotFound) { + switch { + case errors.Is(e.err, promptkit.ErrProfileNotFound): return fmt.Sprintf("PromptKit profile %q is not configured", e.ProfileID) + case errors.Is(e.err, promptkit.ErrInvalidRequest): + return fmt.Sprintf("PromptKit profile ID %q is invalid", e.ProfileID) + case errors.Is(e.err, promptkit.ErrProfileLoad): + return fmt.Sprintf("PromptKit profile %q is invalid or unreadable", e.ProfileID) + default: + return fmt.Sprintf("PromptKit profile %q could not be inspected", e.ProfileID) } - return fmt.Sprintf("inspect PromptKit profile %q: %v", e.ProfileID, e.err) } func (e *PromptKitProfileInspectionError) Unwrap() error { return e.err } +type promptKitProfileConfigurationError struct { + err error +} + +func (e *promptKitProfileConfigurationError) Error() string { + return "PromptKit profile configuration is invalid or unreadable" +} + +func (e *promptKitProfileConfigurationError) Unwrap() error { + return e.err +} + func NewPromptKitProfileInspector(cfg PromptKitProfileInspectorConfig) (*PromptKitProfileInspector, error) { source, options, err := promptKitProfileSourceEngineOptions(cfg.Source) if err != nil { @@ -74,7 +92,7 @@ func NewPromptKitProfileInspector(cfg PromptKitProfileInspectorConfig) (*PromptK ProfileDir: source.ProfileDir, }, options...) if err != nil { - return nil, fmt.Errorf("create PromptKit profile inspector: %w", err) + return nil, &promptKitProfileConfigurationError{err: err} } return &PromptKitProfileInspector{engine: engine}, nil } diff --git a/internal/framework/pipeline/chunker_registry.go b/internal/framework/pipeline/chunker_registry.go index 05600eb..8cb21de 100644 --- a/internal/framework/pipeline/chunker_registry.go +++ b/internal/framework/pipeline/chunker_registry.go @@ -24,10 +24,6 @@ func NewChunkerRegistry() *ChunkerRegistry { } } -func (r *ChunkerRegistry) Register(key string, constructor ChunkerConstructor) error { - return r.RegisterWithSpec(defaultModuleSpec(key, StageChunk), constructor) -} - func (r *ChunkerRegistry) RegisterWithSpec(spec ModuleSpec, constructor ChunkerConstructor) error { if constructor == nil { return fmt.Errorf("chunker constructor for %q must not be nil", strings.TrimSpace(spec.Key)) diff --git a/internal/framework/pipeline/chunker_registry_test.go b/internal/framework/pipeline/chunker_registry_test.go index 92b326a..58dc953 100644 --- a/internal/framework/pipeline/chunker_registry_test.go +++ b/internal/framework/pipeline/chunker_registry_test.go @@ -29,6 +29,10 @@ type registryBehaviorCase[M any] struct { moduleKey func(M) string } +func testModuleSpec(key string, stage ModuleStage) ModuleSpec { + return ModuleSpec{Key: key, Stage: stage, ExecutionClass: contracts.ExecutionClassDeterministic} +} + func TestChunkerRegistryBehavior(t *testing.T) { runRegistryBehaviorTests(t, registryBehaviorCase[contracts.Chunker]{ name: "ChunkerRegistry", @@ -39,7 +43,9 @@ func TestChunkerRegistryBehavior(t *testing.T) { return NewChunkerRegistry() }, register: func(registry any, key string, constructor func() (contracts.Chunker, error)) error { - return registry.(*ChunkerRegistry).Register(key, constructor) + return registry.(*ChunkerRegistry).RegisterWithSpec(ModuleSpec{ + Key: key, Stage: StageChunk, ExecutionClass: contracts.ExecutionClassDeterministic, + }, constructor) }, registerWithSpec: func(registry any, spec ModuleSpec, constructor func() (contracts.Chunker, error)) error { return registry.(*ChunkerRegistry).RegisterWithSpec(spec, constructor) @@ -55,7 +61,9 @@ func TestChunkerRegistryBehavior(t *testing.T) { }, nilRegister: func(key string, constructor func() (contracts.Chunker, error)) error { var registry *ChunkerRegistry - return registry.Register(key, constructor) + return registry.RegisterWithSpec(ModuleSpec{ + Key: key, Stage: StageChunk, ExecutionClass: contracts.ExecutionClassDeterministic, + }, constructor) }, nilBuild: func(key string) (contracts.Chunker, error) { var registry *ChunkerRegistry @@ -136,7 +144,7 @@ func runRegistryBehaviorTests[M any](t *testing.T, testCase registryBehaviorCase } }) - t.Run(testCase.name+"/default spec from register", func(t *testing.T) { + t.Run(testCase.name+"/minimal explicit spec registration", func(t *testing.T) { registry := testCase.newRegistry() if err := testCase.register(registry, " "+testCase.key+" ", testCase.constructor(testCase.key)); err != nil { t.Fatalf("Register() error = %v, want nil", err) diff --git a/internal/framework/pipeline/evidence_preparation_test.go b/internal/framework/pipeline/evidence_preparation_test.go index 7db87e9..c64cc5a 100644 --- a/internal/framework/pipeline/evidence_preparation_test.go +++ b/internal/framework/pipeline/evidence_preparation_test.go @@ -108,7 +108,7 @@ func registerTestEvidenceOutput(t *testing.T, registries *Registries, policy Evi func registerTestEvidenceOutputWithProfileValidation(t *testing.T, registries *Registries, policy EvidenceContextPolicy, validateProfile OutputProfileOptionValidator) { t.Helper() registry := NewOutputEncoderRegistry() - if err := registry.RegisterBuilderWithProfileValidation(defaultModuleSpec("output", StageOutput), func(options map[string]any) error { + if err := registry.RegisterBuilderWithProfileValidation(testModuleSpec("output", StageOutput), func(options map[string]any) error { return RejectUnknownOptions(options, "known") }, validateProfile, func(BuildRequest) (contracts.OutputEncoder, error) { return testEvidenceOutput{policy: cloneEvidenceContextPolicy(policy)}, nil diff --git a/internal/framework/pipeline/input_registry.go b/internal/framework/pipeline/input_registry.go index 4bfa161..7b602ff 100644 --- a/internal/framework/pipeline/input_registry.go +++ b/internal/framework/pipeline/input_registry.go @@ -24,10 +24,6 @@ func NewInputAdapterRegistry() *InputAdapterRegistry { } } -func (r *InputAdapterRegistry) Register(key string, constructor InputAdapterConstructor) error { - return r.RegisterWithSpec(defaultModuleSpec(key, StageInput), constructor) -} - func (r *InputAdapterRegistry) RegisterWithSpec(spec ModuleSpec, constructor InputAdapterConstructor) error { if constructor == nil { return fmt.Errorf("input adapter constructor for %q must not be nil", strings.TrimSpace(spec.Key)) diff --git a/internal/framework/pipeline/input_registry_test.go b/internal/framework/pipeline/input_registry_test.go index 19d2715..f637074 100644 --- a/internal/framework/pipeline/input_registry_test.go +++ b/internal/framework/pipeline/input_registry_test.go @@ -11,10 +11,14 @@ import ( "gitea.maximumdirect.net/eric/notarius/internal/framework/contracts" ) +func registerTestInput(registry *InputAdapterRegistry, key string, constructor InputAdapterConstructor) error { + return registry.RegisterWithSpec(testModuleSpec(key, StageInput), constructor) +} + func TestInputAdapterRegistryRegisterAndBuild(t *testing.T) { registry := NewInputAdapterRegistry() - if err := registry.Register("generic-input", fakeInputAdapterConstructor("generic-input")); err != nil { + if err := registerTestInput(registry, "generic-input", fakeInputAdapterConstructor("generic-input")); err != nil { t.Fatalf("Register() error = %v, want nil", err) } @@ -30,7 +34,7 @@ func TestInputAdapterRegistryRegisterAndBuild(t *testing.T) { func TestInputAdapterRegistryRegisterAndBuildTrimKeys(t *testing.T) { registry := NewInputAdapterRegistry() - if err := registry.Register(" generic-input ", fakeInputAdapterConstructor("generic-input")); err != nil { + if err := registerTestInput(registry, " generic-input ", fakeInputAdapterConstructor("generic-input")); err != nil { t.Fatalf("Register() error = %v, want nil", err) } @@ -82,10 +86,10 @@ func TestInputAdapterRegistryRegisterWithSpecStoresMetadata(t *testing.T) { } } -func TestInputAdapterRegistryRegisterStoresDefaultSpec(t *testing.T) { +func TestInputAdapterRegistryRegisterWithSpecStoresMinimalMetadata(t *testing.T) { registry := NewInputAdapterRegistry() - if err := registry.Register(" generic-input ", fakeInputAdapterConstructor("generic-input")); err != nil { + if err := registerTestInput(registry, " generic-input ", fakeInputAdapterConstructor("generic-input")); err != nil { t.Fatalf("Register() error = %v, want nil", err) } @@ -123,7 +127,7 @@ func TestInputAdapterRegistrySpecRejectsUnknownKey(t *testing.T) { func TestInputAdapterRegistryRegisterRejectsEmptyKey(t *testing.T) { registry := NewInputAdapterRegistry() - err := registry.Register(" \t", fakeInputAdapterConstructor("generic-input")) + err := registerTestInput(registry, " \t", fakeInputAdapterConstructor("generic-input")) if err == nil { t.Fatal("Register() error = nil, want error") @@ -135,11 +139,11 @@ func TestInputAdapterRegistryRegisterRejectsEmptyKey(t *testing.T) { func TestInputAdapterRegistryRegisterRejectsDuplicateKey(t *testing.T) { registry := NewInputAdapterRegistry() - if err := registry.Register("generic-input", fakeInputAdapterConstructor("generic-input")); err != nil { + if err := registerTestInput(registry, "generic-input", fakeInputAdapterConstructor("generic-input")); err != nil { t.Fatalf("Register() error = %v, want nil", err) } - err := registry.Register(" generic-input ", fakeInputAdapterConstructor("generic-input")) + err := registerTestInput(registry, " generic-input ", fakeInputAdapterConstructor("generic-input")) if err == nil { t.Fatal("Register() error = nil, want error") @@ -152,7 +156,7 @@ func TestInputAdapterRegistryRegisterRejectsDuplicateKey(t *testing.T) { func TestInputAdapterRegistryRegisterRejectsNilConstructor(t *testing.T) { registry := NewInputAdapterRegistry() - err := registry.Register("generic-input", nil) + err := registerTestInput(registry, "generic-input", nil) if err == nil { t.Fatal("Register() error = nil, want error") @@ -178,7 +182,7 @@ func TestInputAdapterRegistryBuildRejectsUnknownKey(t *testing.T) { func TestInputAdapterRegistryBuildWrapsConstructorError(t *testing.T) { registry := NewInputAdapterRegistry() constructorErr := errors.New("constructor failed") - if err := registry.Register("generic-input", func() (contracts.InputAdapter, error) { + if err := registerTestInput(registry, "generic-input", func() (contracts.InputAdapter, error) { return nil, constructorErr }); err != nil { t.Fatalf("Register() error = %v, want nil", err) @@ -199,7 +203,7 @@ func TestInputAdapterRegistryBuildWrapsConstructorError(t *testing.T) { func TestInputAdapterRegistryBuildRejectsNilAdapter(t *testing.T) { registry := NewInputAdapterRegistry() - if err := registry.Register("generic-input", func() (contracts.InputAdapter, error) { + if err := registerTestInput(registry, "generic-input", func() (contracts.InputAdapter, error) { return nil, nil }); err != nil { t.Fatalf("Register() error = %v, want nil", err) @@ -217,7 +221,7 @@ func TestInputAdapterRegistryBuildRejectsNilAdapter(t *testing.T) { func TestInputAdapterRegistryBuildRejectsAdapterKeyMismatch(t *testing.T) { registry := NewInputAdapterRegistry() - if err := registry.Register("generic-input", fakeInputAdapterConstructor("other-input")); err != nil { + if err := registerTestInput(registry, "generic-input", fakeInputAdapterConstructor("other-input")); err != nil { t.Fatalf("Register() error = %v, want nil", err) } @@ -234,7 +238,7 @@ func TestInputAdapterRegistryBuildRejectsAdapterKeyMismatch(t *testing.T) { func TestInputAdapterRegistryRegisteredKeysReturnsSortedCopy(t *testing.T) { registry := NewInputAdapterRegistry() for _, key := range []string{"zeta", "alpha", "middle"} { - if err := registry.Register(key, fakeInputAdapterConstructor(key)); err != nil { + if err := registerTestInput(registry, key, fakeInputAdapterConstructor(key)); err != nil { t.Fatalf("Register(%q) error = %v, want nil", key, err) } } @@ -255,7 +259,7 @@ func TestInputAdapterRegistryRegisteredKeysReturnsSortedCopy(t *testing.T) { func TestInputAdapterRegistryNilRegistryBehavior(t *testing.T) { var registry *InputAdapterRegistry - if err := registry.Register("generic-input", fakeInputAdapterConstructor("generic-input")); err == nil { + if err := registerTestInput(registry, "generic-input", fakeInputAdapterConstructor("generic-input")); err == nil { t.Fatal("Register() error = nil, want error") } if _, err := registry.Build("generic-input"); err == nil { diff --git a/internal/framework/pipeline/module.go b/internal/framework/pipeline/module.go index e95d7ec..5a35d4f 100644 --- a/internal/framework/pipeline/module.go +++ b/internal/framework/pipeline/module.go @@ -30,14 +30,6 @@ type ModuleSpec struct { ReferenceSlots []contracts.ReferenceSlot } -func defaultModuleSpec(key string, stage ModuleStage) ModuleSpec { - return ModuleSpec{ - Key: key, - Stage: stage, - ExecutionClass: contracts.ExecutionClassDeterministic, - } -} - func normalizeModuleSpec(spec ModuleSpec) ModuleSpec { executionClass := contracts.ExecutionClass(strings.TrimSpace(string(spec.ExecutionClass))) return ModuleSpec{ diff --git a/internal/framework/pipeline/output_registry.go b/internal/framework/pipeline/output_registry.go index 1eb4529..5249631 100644 --- a/internal/framework/pipeline/output_registry.go +++ b/internal/framework/pipeline/output_registry.go @@ -32,10 +32,6 @@ func NewOutputEncoderRegistry() *OutputEncoderRegistry { } } -func (r *OutputEncoderRegistry) Register(key string, constructor OutputEncoderConstructor) error { - return r.RegisterWithSpec(defaultModuleSpec(key, StageOutput), constructor) -} - func (r *OutputEncoderRegistry) RegisterWithSpec(spec ModuleSpec, constructor OutputEncoderConstructor) error { if constructor == nil { return fmt.Errorf("output encoder constructor for %q must not be nil", strings.TrimSpace(spec.Key)) diff --git a/internal/framework/pipeline/output_registry_test.go b/internal/framework/pipeline/output_registry_test.go index 533c652..10a54c0 100644 --- a/internal/framework/pipeline/output_registry_test.go +++ b/internal/framework/pipeline/output_registry_test.go @@ -17,7 +17,9 @@ func TestOutputEncoderRegistryBehavior(t *testing.T) { return NewOutputEncoderRegistry() }, register: func(registry any, key string, constructor func() (contracts.OutputEncoder, error)) error { - return registry.(*OutputEncoderRegistry).Register(key, constructor) + return registry.(*OutputEncoderRegistry).RegisterWithSpec(ModuleSpec{ + Key: key, Stage: StageOutput, ExecutionClass: contracts.ExecutionClassDeterministic, + }, constructor) }, registerWithSpec: func(registry any, spec ModuleSpec, constructor func() (contracts.OutputEncoder, error)) error { return registry.(*OutputEncoderRegistry).RegisterWithSpec(spec, constructor) @@ -33,7 +35,9 @@ func TestOutputEncoderRegistryBehavior(t *testing.T) { }, nilRegister: func(key string, constructor func() (contracts.OutputEncoder, error)) error { var registry *OutputEncoderRegistry - return registry.Register(key, constructor) + return registry.RegisterWithSpec(ModuleSpec{ + Key: key, Stage: StageOutput, ExecutionClass: contracts.ExecutionClassDeterministic, + }, constructor) }, nilBuild: func(key string) (contracts.OutputEncoder, error) { var registry *OutputEncoderRegistry @@ -60,7 +64,7 @@ func TestOutputEncoderRegistryBehavior(t *testing.T) { func TestOutputProfileValidationReceivesOwnedOptionsAndLaneIDs(t *testing.T) { registry := NewOutputEncoderRegistry() - if err := registry.RegisterBuilderWithProfileValidation(defaultModuleSpec("profile-output", StageOutput), func(options map[string]any) error { + if err := registry.RegisterBuilderWithProfileValidation(testModuleSpec("profile-output", StageOutput), func(options map[string]any) error { options["nested"].(map[string]any)["value"] = "changed" return nil }, func(context OutputProfileOptionContext, options map[string]any) error { diff --git a/internal/framework/pipeline/preparation_test.go b/internal/framework/pipeline/preparation_test.go index 11cad59..4ffeaa3 100644 --- a/internal/framework/pipeline/preparation_test.go +++ b/internal/framework/pipeline/preparation_test.go @@ -480,19 +480,19 @@ func constructionRegistriesWithHooks(t *testing.T, built *[]string, failure *con if err := RegisterArtifactCodec(registries.ArtifactCodecs, notesCodec()); err != nil { t.Fatal(err) } - if err := registries.Inputs.RegisterBuilderWithSpec(defaultModuleSpec("input", StageInput), strict, func(request BuildRequest) (contracts.InputAdapter, error) { + if err := registries.Inputs.RegisterBuilderWithSpec(testModuleSpec("input", StageInput), strict, func(request BuildRequest) (contracts.InputAdapter, error) { record("input", &request) return input, nil }); err != nil { t.Fatal(err) } - if err := registries.Chunkers.RegisterBuilderWithSpec(defaultModuleSpec("chunk", StageChunk), strict, func(request BuildRequest) (contracts.Chunker, error) { + if err := registries.Chunkers.RegisterBuilderWithSpec(testModuleSpec("chunk", StageChunk), strict, func(request BuildRequest) (contracts.Chunker, error) { record("chunk", &request) return &typedTestChunker{key: "chunk"}, nil }); err != nil { t.Fatal(err) } - extractSpec := defaultModuleSpec("extract", StageExtract) + extractSpec := testModuleSpec("extract", StageExtract) extractSpec.ArtifactKind = "test/notes" if err := RegisterExtractorBuilder(registries.Extractors, extractSpec, strict, func(request BuildRequest) (contracts.Extractor[codecNotes], error) { record("extract", &request) @@ -503,7 +503,7 @@ func constructionRegistriesWithHooks(t *testing.T, built *[]string, failure *con }); err != nil { t.Fatal(err) } - mergeSpec := defaultModuleSpec("merge", StageMerge) + mergeSpec := testModuleSpec("merge", StageMerge) mergeSpec.ArtifactKind = "test/notes" if err := RegisterMergerBuilder(registries.Mergers, mergeSpec, strict, func(request BuildRequest) (contracts.Merger[codecNotes], error) { record("merge", &request) @@ -511,7 +511,7 @@ func constructionRegistriesWithHooks(t *testing.T, built *[]string, failure *con }); err != nil { t.Fatal(err) } - normalizeSpec := defaultModuleSpec("normalize", StageNormalize) + normalizeSpec := testModuleSpec("normalize", StageNormalize) normalizeSpec.ArtifactKind = "test/notes" if err := RegisterNormalizerBuilder(registries.Normalizers, normalizeSpec, strict, func(request BuildRequest) (contracts.Normalizer[codecNotes], error) { record("normalize", &request) @@ -532,7 +532,7 @@ func constructionRegistriesWithHooks(t *testing.T, built *[]string, failure *con }); err != nil { t.Fatal(err) } - if err := registries.Outputs.RegisterBuilderWithSpec(defaultModuleSpec("output", StageOutput), strict, func(request BuildRequest) (contracts.OutputEncoder, error) { + if err := registries.Outputs.RegisterBuilderWithSpec(testModuleSpec("output", StageOutput), strict, func(request BuildRequest) (contracts.OutputEncoder, error) { record("output", &request) if failure.output != nil { return nil, failure.output diff --git a/internal/modules/seriatim/input/transcript/runner_test.go b/internal/modules/seriatim/input/transcript/runner_test.go index c390de4..78c57dc 100644 --- a/internal/modules/seriatim/input/transcript/runner_test.go +++ b/internal/modules/seriatim/input/transcript/runner_test.go @@ -126,7 +126,7 @@ func seriatimRunnerRegistries(t *testing.T, extractor contracts.Extractor[seriat if err := Register(inputs); err != nil { t.Fatalf("register seriatim input: %v", err) } - if err := chunkers.Register("fake/chunk", func() (contracts.Chunker, error) { + if err := chunkers.RegisterWithSpec(pipeline.ModuleSpec{Key: "fake/chunk", Stage: pipeline.StageChunk, ExecutionClass: contracts.ExecutionClassDeterministic}, func() (contracts.Chunker, error) { return runnerSeriatimChunker{}, nil }); err != nil { t.Fatalf("register chunker: %v", err) @@ -146,7 +146,7 @@ func seriatimRunnerRegistries(t *testing.T, extractor contracts.Extractor[seriat }); err != nil { t.Fatalf("register normalizer: %v", err) } - if err := outputs.Register(pipeline.DefaultOutputModule, func() (contracts.OutputEncoder, error) { + if err := outputs.RegisterWithSpec(pipeline.ModuleSpec{Key: pipeline.DefaultOutputModule, Stage: pipeline.StageOutput, ExecutionClass: contracts.ExecutionClassDeterministic}, func() (contracts.OutputEncoder, error) { return runnerSeriatimOutput{}, nil }); err != nil { t.Fatalf("register output: %v", err)