refactor: reduce CRAP scores in router/workflow/notification

- llm_router.go: Extract getStringFromMap, firstNonEmpty, paramResolver - buildCronSpec: 12 → 4 complexity - buildParameters: 9 → 5 complexity - routing_workflow.go: Extract stateMachine, stateResult types - RoutingWorkflow: 11 → 6 complexity - Separate executeTask/executePass/executeFail - notification.go: Extract checker interface pattern - DeploymentPreCheckActivity: 10 → 5 complexity - goCheckers() returns language-specific checkers - Added 7 new test cases for helper functions - Coverage: internal/routing 63.6% → 66.0%
2026-09-03 08:50:21 -07:00
parent a0e64224a7
commit 1c16869126
6 changed files with 487 additions and 169 deletions
@@ -324,44 +324,52 @@ func (r *LLMRouter) buildSpec(intent *Intent, input LLMRouterInput) (*WorkflowSp
 	}, nil
 }

+// getStringFromMap safely extracts a string from a map
+func getStringFromMap(m map[string]interface{}, key string) string {
+	if m == nil {
+		return ""
+	}
+	if v, ok := m[key].(string); ok {
+		return v
+	}
+	return ""
+}
+
+// firstNonEmpty returns the first non-empty string from the list
+func firstNonEmpty(values ...string) string {
+	for _, v := range values {
+		if v != "" {
+			return v
+		}
+	}
+	return ""
+}
+
 // buildCronSpec creates CronWorkflowSpec from intent
 func (r *LLMRouter) buildCronSpec(intent *Intent, input LLMRouterInput) (*CronWorkflowSpec, error) {
-	// First build regular spec
 	spec, err := r.buildSpec(intent, input)
 	if err != nil {
 		return nil, err
 	}

-	// Get schedule - check both intent and parameters (LLM sometimes puts it in parameters)
-	schedule := intent.CronSchedule
-	if schedule == "" {
-		if sched, ok := intent.Parameters["cronSchedule"].(string); ok {
-			schedule = sched
-		}
-	}
-	if schedule == "" {
-		if sched, ok := spec.Input["cronSchedule"].(string); ok {
-			schedule = sched
-			delete(spec.Input, "cronSchedule") // Remove from input
-		}
-	}
+	// Extract schedule from multiple sources
+	schedule := firstNonEmpty(
+		intent.CronSchedule,
+		getStringFromMap(intent.Parameters, "cronSchedule"),
+		getStringFromMap(spec.Input, "cronSchedule"),
+	)

-	// Get timezone
-	timezone := intent.CronTimezone
-	if timezone == "" {
-		if tz, ok := intent.Parameters["cronTimezone"].(string); ok {
-			timezone = tz
-		}
-	}
-	if timezone == "" {
-		if tz, ok := spec.Input["cronTimezone"].(string); ok {
-			timezone = tz
-			delete(spec.Input, "cronTimezone") // Remove from input
-		}
-	}
-	if timezone == "" {
-		timezone = "UTC"
-	}
+	// Extract timezone from multiple sources, default to UTC
+	timezone := firstNonEmpty(
+		intent.CronTimezone,
+		getStringFromMap(intent.Parameters, "cronTimezone"),
+		getStringFromMap(spec.Input, "cronTimezone"),
+		"UTC",
+	)
+
+	// Clean cron fields from input
+	delete(spec.Input, "cronSchedule")
+	delete(spec.Input, "cronTimezone")

 	return &CronWorkflowSpec{
 		Name:          spec.Name,
@@ -376,44 +384,83 @@ func (r *LLMRouter) buildCronSpec(intent *Intent, input LLMRouterInput) (*CronWo
 	}, nil
 }

-// buildParameters creates parameter map for activity
-func (r *LLMRouter) buildParameters(act *ActivityMetadata, intent *Intent, stateIndex int) map[string]interface{} {
-	params := make(map[string]interface{})
+// isCommonInputField checks if field name is a common workflow input
+func isCommonInputField(name string) bool {
+	switch name {
+	case "repo", "path", "branch":
+		return true
+	}
+	return false
+}

-	for inputName, inputDef := range act.Inputs {
-		// Check if parameter was extracted from intent
-		if val, ok := intent.Parameters[inputName]; ok {
-			params[inputName] = val
-			continue
-		}
+// paramResolver resolves activity parameters from multiple sources
+type paramResolver struct {
+	intent    *Intent
+	kb        *KnowledgeBase
+	prevState string
+}

-		// Check for JSONPath reference from previous state
-		if stateIndex > 0 {
-			prevAct := intent.Activities[stateIndex-1]
-			prevActDef := r.knowledgeBase.GetActivity(prevAct)
-			
-			// Look for matching output from previous activity
-			for outName := range prevActDef.Outputs {
-				if outName == inputName || strings.EqualFold(outName, inputName) {
-					params[inputName] = fmt.Sprintf("${%s.output.%s}", prevAct, outName)
-					break
-				}
-			}
-		}
-
-		// Use default if available
-		if params[inputName] == nil && inputDef.Default != nil {
-			params[inputName] = inputDef.Default
-		}
-
-		// Use input reference for common fields
-		if params[inputName] == nil {
-			if inputName == "repo" || inputName == "path" || inputName == "branch" {
-				params[inputName] = fmt.Sprintf("${input.%s}", inputName)
-			}
-		}
+// resolve finds parameter value from intent, previous output, default, or input ref
+func (r *paramResolver) resolve(inputName string, inputDef InputField) interface{} {
+	// 1. From intent parameters
+	if val, ok := r.intent.Parameters[inputName]; ok {
+		return val
 	}

+	// 2. From previous state output
+	if val := r.fromPrevOutput(inputName); val != nil {
+		return val
+	}
+
+	// 3. Default value
+	if inputDef.Default != nil {
+		return inputDef.Default
+	}
+
+	// 4. Input reference for common fields
+	if isCommonInputField(inputName) {
+		return fmt.Sprintf("${input.%s}", inputName)
+	}
+
+	return nil
+}
+
+// fromPrevOutput checks if previous activity has matching output
+func (r *paramResolver) fromPrevOutput(inputName string) interface{} {
+	if r.prevState == "" {
+		return nil
+	}
+	prevActDef := r.kb.GetActivity(r.prevState)
+	if prevActDef == nil {
+		return nil
+	}
+	for outName := range prevActDef.Outputs {
+		if outName == inputName || strings.EqualFold(outName, inputName) {
+			return fmt.Sprintf("${%s.output.%s}", r.prevState, outName)
+		}
+	}
+	return nil
+}
+
+// buildParameters creates parameter map for activity
+func (r *LLMRouter) buildParameters(act *ActivityMetadata, intent *Intent, stateIndex int) map[string]interface{} {
+	var prevState string
+	if stateIndex > 0 {
+		prevState = intent.Activities[stateIndex-1]
+	}
+
+	resolver := &paramResolver{
+		intent:    intent,
+		kb:        r.knowledgeBase,
+		prevState: prevState,
+	}
+
+	params := make(map[string]interface{})
+	for name, def := range act.Inputs {
+		if val := resolver.resolve(name, def); val != nil {
+			params[name] = val
+		}
+	}
 	return params
 }

@@ -278,6 +278,193 @@ func TestBuildRetryPolicy(t *testing.T) {
 	}
 }

+func TestGetStringFromMap(t *testing.T) {
+	tests := []struct {
+		name     string
+		m        map[string]interface{}
+		key      string
+		expected string
+	}{
+		{"nil map", nil, "key", ""},
+		{"missing key", map[string]interface{}{"a": "b"}, "key", ""},
+		{"found string", map[string]interface{}{"key": "value"}, "key", "value"},
+		{"non-string value", map[string]interface{}{"key": 123}, "key", ""},
+		{"empty string", map[string]interface{}{"key": ""}, "key", ""},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			got := getStringFromMap(tt.m, tt.key)
+			if got != tt.expected {
+				t.Errorf("getStringFromMap() = %q, want %q", got, tt.expected)
+			}
+		})
+	}
+}
+
+func TestFirstNonEmpty(t *testing.T) {
+	tests := []struct {
+		name     string
+		values   []string
+		expected string
+	}{
+		{"all empty", []string{"", "", ""}, ""},
+		{"first non-empty", []string{"first", "second"}, "first"},
+		{"second non-empty", []string{"", "second", "third"}, "second"},
+		{"last non-empty", []string{"", "", "last"}, "last"},
+		{"no values", []string{}, ""},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			got := firstNonEmpty(tt.values...)
+			if got != tt.expected {
+				t.Errorf("firstNonEmpty() = %q, want %q", got, tt.expected)
+			}
+		})
+	}
+}
+
+func TestIsCommonInputField(t *testing.T) {
+	tests := []struct {
+		name     string
+		expected bool
+	}{
+		{"repo", true},
+		{"path", true},
+		{"branch", true},
+		{"unknown", false},
+		{"Repository", false}, // case-sensitive
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			got := isCommonInputField(tt.name)
+			if got != tt.expected {
+				t.Errorf("isCommonInputField(%q) = %v, want %v", tt.name, got, tt.expected)
+			}
+		})
+	}
+}
+
+func TestBuildCronSpecScheduleSources(t *testing.T) {
+	kb, err := LoadKnowledgeBaseFromDefaultPath()
+	if err != nil {
+		t.Fatalf("failed to load knowledge base: %v", err)
+	}
+
+	router := &LLMRouter{knowledgeBase: kb}
+
+	tests := []struct {
+		name           string
+		intentSchedule string
+		intentTimezone string
+		paramSchedule  string
+		paramTimezone  string
+		wantSchedule   string
+		wantTimezone   string
+	}{
+		{
+			name:           "from intent",
+			intentSchedule: "0 2 * * *",
+			intentTimezone: "PST",
+			wantSchedule:   "0 2 * * *",
+			wantTimezone:   "PST",
+		},
+		{
+			name:          "from params",
+			paramSchedule: "0 3 * * *",
+			paramTimezone: "EST",
+			wantSchedule:  "0 3 * * *",
+			wantTimezone:  "EST",
+		},
+		{
+			name:         "default UTC",
+			wantSchedule: "",
+			wantTimezone: "UTC",
+		},
+		{
+			name:           "intent priority",
+			intentSchedule: "0 1 * * *",
+			intentTimezone: "UTC",
+			paramSchedule:  "0 2 * * *",
+			paramTimezone:  "PST",
+			wantSchedule:   "0 1 * * *",
+			wantTimezone:   "UTC",
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			intent := &Intent{
+				Activities:   []string{"CloneRepoActivity"},
+				Parameters:   map[string]interface{}{},
+				IsCron:       true,
+				CronSchedule: tt.intentSchedule,
+				CronTimezone: tt.intentTimezone,
+				WorkflowName: "test",
+			}
+			if tt.paramSchedule != "" {
+				intent.Parameters["cronSchedule"] = tt.paramSchedule
+			}
+			if tt.paramTimezone != "" {
+				intent.Parameters["cronTimezone"] = tt.paramTimezone
+			}
+
+			input := LLMRouterInput{Message: "test"}
+			spec, err := router.buildCronSpec(intent, input)
+			if err != nil {
+				t.Fatalf("buildCronSpec failed: %v", err)
+			}
+
+			if spec.Schedule != tt.wantSchedule {
+				t.Errorf("schedule = %q, want %q", spec.Schedule, tt.wantSchedule)
+			}
+			if spec.Timezone != tt.wantTimezone {
+				t.Errorf("timezone = %q, want %q", spec.Timezone, tt.wantTimezone)
+			}
+		})
+	}
+}
+
+func TestParamResolverFromPrevOutput(t *testing.T) {
+	kb, err := LoadKnowledgeBaseFromDefaultPath()
+	if err != nil {
+		t.Fatalf("failed to load knowledge base: %v", err)
+	}
+
+	resolver := &paramResolver{
+		intent: &Intent{
+			Activities: []string{"CloneRepoActivity", "AnalyzeCodeActivity"},
+			Parameters: map[string]interface{}{},
+		},
+		kb:        kb,
+		prevState: "CloneRepoActivity",
+	}
+
+	// Should find matching output from CloneRepoActivity
+	got := resolver.fromPrevOutput("path")
+	if got == nil {
+		t.Error("expected to find path from prev output")
+	}
+	if got != "${CloneRepoActivity.output.path}" {
+		t.Errorf("got %v, want ${CloneRepoActivity.output.path}", got)
+	}
+
+	// Should not find non-existent output
+	got = resolver.fromPrevOutput("nonexistent")
+	if got != nil {
+		t.Errorf("expected nil for nonexistent, got %v", got)
+	}
+
+	// Empty prevState should return nil
+	resolver.prevState = ""
+	got = resolver.fromPrevOutput("path")
+	if got != nil {
+		t.Errorf("expected nil for empty prevState, got %v", got)
+	}
+}
+
 func TestIntentJSONMarshal(t *testing.T) {
 	intent := &Intent{
 		Activities:    []string{"CloneRepoActivity"},