From fd835b582cad46a80ca82f97057caf5927844655 Mon Sep 17 00:00:00 2001 From: Eric Rakestraw Date: Fri, 3 Jul 2026 18:56:35 +0000 Subject: [PATCH] Fix config validation edge cases --- internal/core/config/file_config.go | 55 +++++++++++---- internal/core/config/file_config_test.go | 83 +++++++++++++++++++++++ internal/core/config/validation.go | 9 --- internal/core/config/validation_test.go | 11 ++- internal/core/diagnostics/run_dir.go | 38 ++++++++--- internal/core/diagnostics/run_dir_test.go | 60 ++++++++++++++++ 6 files changed, 222 insertions(+), 34 deletions(-) diff --git a/internal/core/config/file_config.go b/internal/core/config/file_config.go index 5187587..5df0d51 100644 --- a/internal/core/config/file_config.go +++ b/internal/core/config/file_config.go @@ -5,6 +5,7 @@ import ( "fmt" "os" "regexp" + "sort" "strings" "time" @@ -198,11 +199,23 @@ func (c *Config) applyFileConfigWithLookup(fileCfg FileConfig, lookup func(strin c.Pipelines = map[string]pipeline.PipelineProfile{} } - for rawID, fileProfile := range fileCfg.LLMProfiles { - profileID := strings.TrimSpace(rawID) - if profileID == "" { - return fmt.Errorf("llm profile id must not be empty") + profileIDs, rawLLMProfileIDs, err := normalizedMapKeys(fileCfg.LLMProfiles, "llm profile id") + if err != nil { + return err + } + pipelineIDs, rawPipelineIDs, err := normalizedMapKeys(fileCfg.Pipelines, "pipeline id") + if err != nil { + return err + } + for _, pipelineID := range pipelineIDs { + filePipeline := fileCfg.Pipelines[rawPipelineIDs[pipelineID]] + if _, _, err := normalizedMapKeys(filePipeline.Artifacts, fmt.Sprintf("pipeline %q artifact lane id", pipelineID)); err != nil { + return err } + } + + for _, profileID := range profileIDs { + fileProfile := fileCfg.LLMProfiles[rawLLMProfileIDs[profileID]] profile := c.LLMProfiles[profileID] if fileProfile.Provider != nil { profile.Provider = strings.TrimSpace(*fileProfile.Provider) @@ -233,10 +246,11 @@ func (c *Config) applyFileConfigWithLookup(fileCfg FileConfig, lookup func(strin c.LLMProfiles[profileID] = profile } - for rawID, filePipeline := range fileCfg.Pipelines { - pipelineID := strings.TrimSpace(rawID) - if pipelineID == "" { - return fmt.Errorf("pipeline id must not be empty") + for _, pipelineID := range pipelineIDs { + filePipeline := fileCfg.Pipelines[rawPipelineIDs[pipelineID]] + laneIDs, rawLaneIDs, err := normalizedMapKeys(filePipeline.Artifacts, fmt.Sprintf("pipeline %q artifact lane id", pipelineID)) + if err != nil { + return err } profile := pipeline.PipelineProfile{ ID: pipelineID, @@ -249,11 +263,8 @@ func (c *Config) applyFileConfigWithLookup(fileCfg FileConfig, lookup func(strin if filePipeline.Output != nil { profile.Output = filePipeline.Output.toPipelineBinding() } - for rawLaneID, fileLane := range filePipeline.Artifacts { - laneID := strings.TrimSpace(rawLaneID) - if laneID == "" { - return fmt.Errorf("pipeline %q artifact lane id must not be empty", pipelineID) - } + for _, laneID := range laneIDs { + fileLane := filePipeline.Artifacts[rawLaneIDs[laneID]] lane := pipeline.ArtifactLaneProfile{ Extract: fileLane.Extract.toPipelineBinding(), } @@ -289,6 +300,24 @@ func (c *Config) applyFileConfigWithLookup(fileCfg FileConfig, lookup func(strin return nil } +func normalizedMapKeys[T any](values map[string]T, keyName string) ([]string, map[string]string, error) { + keys := make([]string, 0, len(values)) + rawByNormalized := make(map[string]string, len(values)) + for rawID := range values { + id := strings.TrimSpace(rawID) + if id == "" { + return nil, nil, fmt.Errorf("%s must not be empty", keyName) + } + if _, ok := rawByNormalized[id]; ok { + return nil, nil, fmt.Errorf("%s %q is duplicated after trimming", keyName, id) + } + rawByNormalized[id] = rawID + keys = append(keys, id) + } + sort.Strings(keys) + return keys, rawByNormalized, nil +} + func resolveAPIKeyEnv(envName string, lookup func(string) (string, bool)) (string, error) { name := strings.TrimSpace(envName) if name == "" { diff --git a/internal/core/config/file_config_test.go b/internal/core/config/file_config_test.go index 92e0b27..2009bfe 100644 --- a/internal/core/config/file_config_test.go +++ b/internal/core/config/file_config_test.go @@ -234,6 +234,89 @@ llm_profiles: } } +func TestApplyFileConfigRejectsDuplicateTrimmedLLMProfileIDs(t *testing.T) { + fileCfg, err := ParseFileConfigYAML([]byte(` +version: 1 +llm_profiles: + default: + model: first + " default ": + model: second +`)) + if err != nil { + t.Fatalf("ParseFileConfigYAML: %v", err) + } + + cfg := Default() + err = cfg.applyFileConfigWithLookup(fileCfg, emptyLookup) + if err == nil || !strings.Contains(err.Error(), "llm profile id") || !strings.Contains(err.Error(), "duplicated") { + t.Fatalf("expected duplicate LLM profile ID error, got %v", err) + } +} + +func TestApplyFileConfigRejectsDuplicateTrimmedPipelineIDs(t *testing.T) { + fileCfg, err := ParseFileConfigYAML([]byte(` +version: 1 +pipelines: + example: + input: fake/input + " example ": + input: fake/other-input +`)) + if err != nil { + t.Fatalf("ParseFileConfigYAML: %v", err) + } + + cfg := Default() + err = cfg.applyFileConfigWithLookup(fileCfg, emptyLookup) + if err == nil || !strings.Contains(err.Error(), "pipeline id") || !strings.Contains(err.Error(), "duplicated") { + t.Fatalf("expected duplicate pipeline ID error, got %v", err) + } +} + +func TestApplyFileConfigRejectsDuplicateTrimmedArtifactLaneIDs(t *testing.T) { + fileCfg, err := ParseFileConfigYAML([]byte(` +version: 1 +pipelines: + example: + input: fake/input + artifacts: + events: + extract: fake/extract + " events ": + extract: fake/other-extract +`)) + if err != nil { + t.Fatalf("ParseFileConfigYAML: %v", err) + } + + cfg := Default() + err = cfg.applyFileConfigWithLookup(fileCfg, emptyLookup) + if err == nil || !strings.Contains(err.Error(), `pipeline "example" artifact lane id`) || !strings.Contains(err.Error(), "duplicated") { + t.Fatalf("expected duplicate artifact lane ID error, got %v", err) + } +} + +func TestApplyFileConfigAllowsRetryOnlyLLMProfile(t *testing.T) { + cfg := parseAndApplyConfig(t, ` +version: 1 +llm_profiles: + retry-only: + max_retries: 3 +`) + + profile := cfg.LLMProfiles["retry-only"] + if profile.MaxRetries != 3 { + t.Fatalf("unexpected max retries: %d", profile.MaxRetries) + } + if profile.TimeoutSeconds != 0 { + t.Fatalf("expected unset timeout, got %d", profile.TimeoutSeconds) + } + if profile.MaxConcurrency != 0 { + t.Fatalf("expected unset max concurrency, got %d", profile.MaxConcurrency) + } +} + func TestApplyFileConfigRejectsInvalidAPIKeyEnv(t *testing.T) { tests := []struct { name string diff --git a/internal/core/config/validation.go b/internal/core/config/validation.go index 8c3fde2..b6b2615 100644 --- a/internal/core/config/validation.go +++ b/internal/core/config/validation.go @@ -58,15 +58,6 @@ func validateLLMProfiles(profiles map[string]LLMProfile) error { if profile.MaxConcurrency < 0 { return fmt.Errorf("LLM profile %q max concurrency must not be negative", id) } - if profile.TimeoutSeconds == 0 && profile.MaxRetries == 0 && profile.MaxConcurrency == 0 { - continue - } - if profile.TimeoutSeconds == 0 { - return fmt.Errorf("LLM profile %q timeout seconds must be greater than zero", id) - } - if profile.MaxConcurrency == 0 { - return fmt.Errorf("LLM profile %q max concurrency must be greater than zero", id) - } } return nil } diff --git a/internal/core/config/validation_test.go b/internal/core/config/validation_test.go index 6778fdf..33a7356 100644 --- a/internal/core/config/validation_test.go +++ b/internal/core/config/validation_test.go @@ -79,7 +79,7 @@ func TestValidateRejectsInvalidNumericFields(t *testing.T) { name: "max concurrency", mutate: func(cfg Config) Config { profile := cfg.LLMProfiles["default"] - profile.MaxConcurrency = 0 + profile.MaxConcurrency = -1 cfg.LLMProfiles["default"] = profile return cfg }, @@ -97,6 +97,15 @@ func TestValidateRejectsInvalidNumericFields(t *testing.T) { } } +func TestValidateAllowsPartialLLMProfileNumericConfig(t *testing.T) { + cfg := validConfig() + cfg.LLMProfiles["retry-only"] = LLMProfile{MaxRetries: 3} + + if err := cfg.Validate(); err != nil { + t.Fatalf("Validate: %v", err) + } +} + func TestValidateRejectsInvalidDiagnosticsRetention(t *testing.T) { cfg := validConfig() cfg.Diagnostics.Retention = diagnostics.RetentionMode("sometimes") diff --git a/internal/core/diagnostics/run_dir.go b/internal/core/diagnostics/run_dir.go index 36fa88e..0661ca9 100644 --- a/internal/core/diagnostics/run_dir.go +++ b/internal/core/diagnostics/run_dir.go @@ -12,7 +12,14 @@ import ( "gitea.maximumdirect.net/eric/notarius/internal/framework/contracts" ) -const defaultWorkDir = "/tmp/notarius" +const ( + defaultWorkDir = "/tmp/notarius" + maxRunDirectoryCreateAttempts = 16 +) + +var utcNow = func() time.Time { + return time.Now().UTC() +} // RunDirectory represents a per-run diagnostics directory. type RunDirectory struct { @@ -77,18 +84,27 @@ func NewRunDirectory(workDir string, retention RetentionMode) (*RunDirectory, er return nil, fmt.Errorf("create diagnostics work directory %q: %w", workDir, err) } - createdAt := time.Now().UTC() - runID := fmt.Sprintf("run-%d", createdAt.UnixNano()) - runPath := filepath.Join(workDir, runID) - if err := os.Mkdir(runPath, 0o755); err != nil { - return nil, fmt.Errorf("create diagnostics run directory %q: %w", runPath, err) + var lastRunPath string + for attempt := 0; attempt < maxRunDirectoryCreateAttempts; attempt++ { + createdAt := utcNow() + runID := fmt.Sprintf("run-%d", createdAt.UnixNano()) + runPath := filepath.Join(workDir, runID) + lastRunPath = runPath + if err := os.Mkdir(runPath, 0o755); err != nil { + if os.IsExist(err) { + continue + } + return nil, fmt.Errorf("create diagnostics run directory %q: %w", runPath, err) + } + + return &RunDirectory{ + path: runPath, + retention: retention, + createdAt: createdAt, + }, nil } - return &RunDirectory{ - path: runPath, - retention: retention, - createdAt: createdAt, - }, nil + return nil, fmt.Errorf("create diagnostics run directory %q: exhausted unique run ID attempts", lastRunPath) } func (r *RunDirectory) Path() string { diff --git a/internal/core/diagnostics/run_dir_test.go b/internal/core/diagnostics/run_dir_test.go index a7055ec..4f7ee16 100644 --- a/internal/core/diagnostics/run_dir_test.go +++ b/internal/core/diagnostics/run_dir_test.go @@ -2,6 +2,7 @@ package diagnostics import ( "encoding/json" + "fmt" "os" "path/filepath" "regexp" @@ -35,6 +36,57 @@ func TestNewRunDirectoryCreatesRunDirectoryAndRunID(t *testing.T) { } } +func TestNewRunDirectoryRetriesOnRunIDCollision(t *testing.T) { + workDir := t.TempDir() + first := time.Unix(0, 100).UTC() + second := first.Add(time.Nanosecond) + if err := os.Mkdir(filepath.Join(workDir, fmt.Sprintf("run-%d", first.UnixNano())), 0o755); err != nil { + t.Fatalf("create existing run directory: %v", err) + } + restoreUTCNow := replaceUTCNow(func() func() time.Time { + calls := 0 + return func() time.Time { + calls++ + if calls == 1 { + return first + } + return second + } + }()) + t.Cleanup(restoreUTCNow) + + runDir, err := NewRunDirectory(workDir, RetentionAuto) + if err != nil { + t.Fatalf("NewRunDirectory: %v", err) + } + + wantRunID := fmt.Sprintf("run-%d", second.UnixNano()) + if runDir.RunID() != wantRunID { + t.Fatalf("RunID = %q, want %q", runDir.RunID(), wantRunID) + } + if _, err := os.Stat(runDir.Path()); err != nil { + t.Fatalf("stat run directory: %v", err) + } +} + +func TestNewRunDirectoryReturnsErrorAfterRunIDCollisionsExhausted(t *testing.T) { + workDir := t.TempDir() + collisionTime := time.Unix(0, 200).UTC() + collisionPath := filepath.Join(workDir, fmt.Sprintf("run-%d", collisionTime.UnixNano())) + if err := os.Mkdir(collisionPath, 0o755); err != nil { + t.Fatalf("create existing run directory: %v", err) + } + restoreUTCNow := replaceUTCNow(func() time.Time { + return collisionTime + }) + t.Cleanup(restoreUTCNow) + + _, err := NewRunDirectory(workDir, RetentionAuto) + if err == nil || !strings.Contains(err.Error(), "exhausted unique run ID attempts") { + t.Fatalf("expected exhausted collision error, got %v", err) + } +} + func TestNewRunDirectoryUsesDefaultWorkDirectory(t *testing.T) { runDir, err := NewRunDirectory("", RetentionAuto) if err != nil { @@ -252,6 +304,14 @@ func newTestRunDirectory(t *testing.T) *RunDirectory { return runDir } +func replaceUTCNow(replacement func() time.Time) func() { + original := utcNow + utcNow = replacement + return func() { + utcNow = original + } +} + func readArtifact(t *testing.T, runDir *RunDirectory, name string) []byte { t.Helper() data, err := os.ReadFile(filepath.Join(runDir.Path(), name))