diff --git a/apps/edge/internal/configrefresh/execution_preset_classify_test.go b/apps/edge/internal/configrefresh/execution_preset_classify_test.go index 93ead275..77f0e122 100644 --- a/apps/edge/internal/configrefresh/execution_preset_classify_test.go +++ b/apps/edge/internal/configrefresh/execution_preset_classify_test.go @@ -215,6 +215,13 @@ func TestClassifyModelExecutionPresetLiveApply(t *testing.T) { } } +// templatePath builds an explicitly configured optional template path. A nil +// field means the operator omitted the file, so configured fixtures must carry +// a pointer. +func templatePath(path string) *string { + return &path +} + func TestClassifySingleRequestTemplateContentChange(t *testing.T) { current := &config.EdgeConfig{ ExecutionPresets: []config.ExecutionPreset{ @@ -245,8 +252,8 @@ func TestClassifySingleRequestTemplateContentChange(t *testing.T) { Review: config.ExecutionSingleRequestStageConfig{Model: "gpt-4o", Options: map[string]any{"reasoning_effort": "high"}}, }, Templates: config.ExecutionSingleRequestTemplates{ - PlanFile: "templates/plan.md", - ReviewFile: "templates/review.md", + PlanFile: templatePath("templates/plan.md"), + ReviewFile: templatePath("templates/review.md"), EffectivePlan: "Plan template v1\n# Plan\n## Goal\n{{goal}}\n## Steps\n{{steps}}\n## Verification\n{{verification}}", EffectiveReview: "Review template v1\n# Review\n## Result\nPASS\n## Checks\n{{checks}}\n## Verification\n{{verification}}\n## Summary\n{{summary}}", }, @@ -284,8 +291,8 @@ func TestClassifySingleRequestTemplateContentChange(t *testing.T) { Review: config.ExecutionSingleRequestStageConfig{Model: "gpt-4o", Options: map[string]any{"reasoning_effort": "high"}}, }, Templates: config.ExecutionSingleRequestTemplates{ - PlanFile: "templates/plan.md", // same path - ReviewFile: "templates/review.md", + PlanFile: templatePath("templates/plan.md"), // same path, distinct pointer + ReviewFile: templatePath("templates/review.md"), EffectivePlan: "Plan template v2\n# Plan\n## Goal\n{{goal}}\n## Steps\n{{steps}}\n## Verification\n{{verification}}", // content changed EffectiveReview: "Review template v1\n# Review\n## Result\nPASS\n## Checks\n{{checks}}\n## Verification\n{{verification}}\n## Summary\n{{summary}}", }, diff --git a/packages/go/config/execution_preset_types.go b/packages/go/config/execution_preset_types.go index 5f83478b..91d86c18 100644 --- a/packages/go/config/execution_preset_types.go +++ b/packages/go/config/execution_preset_types.go @@ -283,11 +283,17 @@ type ExecutionSingleRequestPolicy struct { Templates ExecutionSingleRequestTemplates `mapstructure:"templates" yaml:"templates,omitempty"` } +// ExecutionSingleRequestTemplates carries the optional operator-configured +// template paths and the resolved effective templates. A nil path is the only +// omission signal: an explicitly configured value, including an empty or +// whitespace-only one, stays distinguishable from omission so the loader can +// reject it before any filesystem access instead of silently falling back to +// the built-in default. type ExecutionSingleRequestTemplates struct { - PlanFile string `mapstructure:"plan_file" yaml:"plan_file,omitempty"` - ReviewFile string `mapstructure:"review_file" yaml:"review_file,omitempty"` - EffectivePlan string `mapstructure:"-" yaml:"-"` - EffectiveReview string `mapstructure:"-" yaml:"-"` + PlanFile *string `mapstructure:"plan_file" yaml:"plan_file,omitempty"` + ReviewFile *string `mapstructure:"review_file" yaml:"review_file,omitempty"` + EffectivePlan string `mapstructure:"-" yaml:"-"` + EffectiveReview string `mapstructure:"-" yaml:"-"` } // ExecutionSingleRequestLimits carries server-owned absolute resource caps. @@ -336,7 +342,7 @@ func (p *ExecutionSingleRequestPolicy) Clone() *ExecutionSingleRequestPolicy { Work: p.Stages.Work.Clone(), Review: p.Stages.Review.Clone(), }, - Templates: p.Templates, + Templates: p.Templates.Clone(), } return out } @@ -348,6 +354,24 @@ func (s ExecutionSingleRequestStageConfig) Clone() ExecutionSingleRequestStageCo return out } +// Clone returns a deep copy of ExecutionSingleRequestTemplates. The optional +// path cells are copied into fresh pointers so a cloned snapshot never aliases +// the source configuration. +func (t ExecutionSingleRequestTemplates) Clone() ExecutionSingleRequestTemplates { + out := t + out.PlanFile = cloneStringPointer(t.PlanFile) + out.ReviewFile = cloneStringPointer(t.ReviewFile) + return out +} + +func cloneStringPointer(s *string) *string { + if s == nil { + return nil + } + value := *s + return &value +} + // ModeDescriptor is the pure shape descriptor for a registered mode. type ModeDescriptor struct { Name string `yaml:"-"` diff --git a/packages/go/config/load.go b/packages/go/config/load.go index 00c6a938..3d71edf5 100644 --- a/packages/go/config/load.go +++ b/packages/go/config/load.go @@ -681,6 +681,10 @@ func validateWorkspaceNumericLimits(ws WorkspaceDefinition, nodeIdx, wsIdx int) return nil } +// resolveSingleRequestTemplates freezes the effective Plan/Review templates for +// every single-request preset. Only an omitted plan_file/review_file selects the +// built-in default; every configured value is routed through loadTemplateFile so +// an empty or whitespace-only path fails closed instead of falling back. func resolveSingleRequestTemplates(presets []ExecutionPreset, configFilePath string) error { baseDir := filepath.Dir(configFilePath) for i := range presets { @@ -690,10 +694,10 @@ func resolveSingleRequestTemplates(presets []ExecutionPreset, configFilePath str } sr := p.SingleRequest - if strings.TrimSpace(sr.Templates.PlanFile) == "" { + if sr.Templates.PlanFile == nil { sr.Templates.EffectivePlan = singlerequesttemplate.DefaultPlanTemplate } else { - content, err := loadTemplateFile(baseDir, sr.Templates.PlanFile) + content, err := loadTemplateFile(baseDir, *sr.Templates.PlanFile) if err != nil { return fmt.Errorf("presets[%d] id=%q single_request.templates.plan_file: %w", i, p.ID, err) } @@ -703,10 +707,10 @@ func resolveSingleRequestTemplates(presets []ExecutionPreset, configFilePath str sr.Templates.EffectivePlan = content } - if strings.TrimSpace(sr.Templates.ReviewFile) == "" { + if sr.Templates.ReviewFile == nil { sr.Templates.EffectiveReview = singlerequesttemplate.DefaultReviewTemplate } else { - content, err := loadTemplateFile(baseDir, sr.Templates.ReviewFile) + content, err := loadTemplateFile(baseDir, *sr.Templates.ReviewFile) if err != nil { return fmt.Errorf("presets[%d] id=%q single_request.templates.review_file: %w", i, p.ID, err) } diff --git a/packages/go/config/model_execution_preset_config_test.go b/packages/go/config/model_execution_preset_config_test.go index c910bb08..ab5d8991 100644 --- a/packages/go/config/model_execution_preset_config_test.go +++ b/packages/go/config/model_execution_preset_config_test.go @@ -616,6 +616,12 @@ nodes: if sr.Templates.EffectivePlan == "" || sr.Templates.EffectiveReview == "" { t.Errorf("expected effective templates to be set to built-ins") } + if sr.Templates.PlanFile != nil { + t.Errorf("PlanFile = %q, want nil for an omitted field", *sr.Templates.PlanFile) + } + if sr.Templates.ReviewFile != nil { + t.Errorf("ReviewFile = %q, want nil for an omitted field", *sr.Templates.ReviewFile) + } }) t.Run("relative path template resolution config-relative", func(t *testing.T) { @@ -651,6 +657,12 @@ nodes: if sr.Templates.EffectiveReview != customReview { t.Errorf("EffectiveReview = %q, want %q", sr.Templates.EffectiveReview, customReview) } + if sr.Templates.PlanFile == nil || *sr.Templates.PlanFile != "tmpl/custom_plan.md" { + t.Errorf("PlanFile = %v, want the configured relative path", sr.Templates.PlanFile) + } + if sr.Templates.ReviewFile == nil || *sr.Templates.ReviewFile != "tmpl/custom_review.md" { + t.Errorf("ReviewFile = %v, want the configured relative path", sr.Templates.ReviewFile) + } }) t.Run("missing configured template file fails closed", func(t *testing.T) { @@ -768,6 +780,12 @@ nodes: if sr.Templates.EffectiveReview != singlerequesttemplate.DefaultReviewTemplate { t.Errorf("EffectiveReview = %q, want the built-in default", sr.Templates.EffectiveReview) } + if sr.Templates.PlanFile == nil || *sr.Templates.PlanFile != "plan.md" { + t.Errorf("PlanFile = %v, want the configured relative path", sr.Templates.PlanFile) + } + if sr.Templates.ReviewFile != nil { + t.Errorf("ReviewFile = %q, want nil for an omitted field", *sr.Templates.ReviewFile) + } }) t.Run("review_file configured and plan_file falls back", func(t *testing.T) { @@ -791,6 +809,12 @@ nodes: if sr.Templates.EffectivePlan != singlerequesttemplate.DefaultPlanTemplate { t.Errorf("EffectivePlan = %q, want the built-in default", sr.Templates.EffectivePlan) } + if sr.Templates.ReviewFile == nil || *sr.Templates.ReviewFile != "review.md" { + t.Errorf("ReviewFile = %v, want the configured relative path", sr.Templates.ReviewFile) + } + if sr.Templates.PlanFile != nil { + t.Errorf("PlanFile = %q, want nil for an omitted field", *sr.Templates.PlanFile) + } }) t.Run("exact 8192 byte template file accepted", func(t *testing.T) { @@ -939,4 +963,98 @@ nodes: }) } }) + + // Only an omitted field selects the built-in default. An explicitly + // configured empty or whitespace-only path is a configuration error and + // must be rejected with its own field context before the loader touches + // the filesystem. + t.Run("explicitly configured empty template path fails closed", func(t *testing.T) { + cases := []struct { + name string + planFile string + reviewFile string + field string + }{ + {"empty plan_file", `""`, "", "single_request.templates.plan_file"}, + {"whitespace plan_file", `" "`, "", "single_request.templates.plan_file"}, + {"empty review_file", "", `""`, "single_request.templates.review_file"}, + {"whitespace review_file", "", `" "`, "single_request.templates.review_file"}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + rootDir := t.TempDir() + cfgPath := filepath.Join(rootDir, "edge.yaml") + if err := os.WriteFile(cfgPath, []byte(validPresetYAML(tc.planFile, tc.reviewFile)), 0o600); err != nil { + t.Fatalf("write yaml: %v", err) + } + + _, err := config.LoadEdge(cfgPath) + if err == nil { + t.Fatalf("expected error for %s", tc.name) + } + if !strings.Contains(err.Error(), tc.field) { + t.Errorf("expected %s context, got: %v", tc.field, err) + } + if !strings.Contains(err.Error(), "template path must not be empty") { + t.Errorf("expected empty-path rejection, got: %v", err) + } + if strings.Contains(err.Error(), rootDir) { + t.Errorf("rejection reached the filesystem: %v", err) + } + }) + } + }) +} + +// TestExecutionSingleRequestTemplatePathPresenceClone proves the optional path +// cells survive Clone as independent values: mutating a clone must not reach +// the source policy, and the frozen effective templates must be unaffected. +func TestExecutionSingleRequestTemplatePathPresenceClone(t *testing.T) { + planPath := "tmpl/plan.md" + reviewPath := "tmpl/review.md" + original := &config.ExecutionSingleRequestPolicy{ + WorkspaceRef: "ws-1", + Templates: config.ExecutionSingleRequestTemplates{ + PlanFile: &planPath, + ReviewFile: &reviewPath, + EffectivePlan: customPlanTemplate, + EffectiveReview: customReviewTemplate, + }, + } + + clone := original.Clone() + if clone.Templates.PlanFile == nil || clone.Templates.ReviewFile == nil { + t.Fatalf("clone dropped configured template paths: %+v", clone.Templates) + } + if clone.Templates.PlanFile == original.Templates.PlanFile { + t.Errorf("clone aliases the plan_file pointer cell") + } + if clone.Templates.ReviewFile == original.Templates.ReviewFile { + t.Errorf("clone aliases the review_file pointer cell") + } + + *clone.Templates.PlanFile = "tmpl/other_plan.md" + *clone.Templates.ReviewFile = "tmpl/other_review.md" + clone.Templates.EffectivePlan = "mutated plan" + + if *original.Templates.PlanFile != planPath { + t.Errorf("original PlanFile = %q, want %q", *original.Templates.PlanFile, planPath) + } + if *original.Templates.ReviewFile != reviewPath { + t.Errorf("original ReviewFile = %q, want %q", *original.Templates.ReviewFile, reviewPath) + } + if original.Templates.EffectivePlan != customPlanTemplate { + t.Errorf("original EffectivePlan changed: %q", original.Templates.EffectivePlan) + } + if original.Templates.EffectiveReview != customReviewTemplate { + t.Errorf("original EffectiveReview changed: %q", original.Templates.EffectiveReview) + } + + // A nil path must clone as omission, never as a configured empty value. + omitted := &config.ExecutionSingleRequestPolicy{} + omittedClone := omitted.Clone() + if omittedClone.Templates.PlanFile != nil || omittedClone.Templates.ReviewFile != nil { + t.Errorf("clone materialized omitted paths: %+v", omittedClone.Templates) + } }