fix(single_request): 템플릿 경로 생략과 명시 빈 값을 구분한다
`plan_file`/`review_file`이 평범한 문자열이라 decode 직후 trim-empty가 곧 생략으로 취급되었고, 그 결과 loader의 빈 경로 거부가 설정값에 대해 도달할 수 없었다. 명시적으로 설정한 빈 값과 공백 값이 조용히 built-in default로 떨어지는 승인 경계 결함이므로 optional 표현으로 바꿔 생략만 fallback 신호가 되게 한다. 설정된 값은 파일 접근 전에 trim 후 빈 경로 거부까지 도달한다. pointer cell이 snapshot 사이에서 aliasing되지 않도록 Templates 전용 deep clone을 두고, 네 가지 명시 빈/공백 경계와 clone 독립성, 그리고 refresh가 경로나 본문을 노출하지 않는다는 기존 근거를 함께 회귀로 고정한다. Refs: agent-task/single_request_plan_review_templates/PLAN-cloud-G08.md Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
parent
702e62aafc
commit
6f141b9121
4 changed files with 166 additions and 13 deletions
|
|
@ -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}}",
|
||||
},
|
||||
|
|
|
|||
|
|
@ -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:"-"`
|
||||
|
|
|
|||
|
|
@ -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)
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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)
|
||||
}
|
||||
}
|
||||
|
|
|
|||
Loading…
Reference in a new issue