diff --git a/pkg/parser/schema.go b/pkg/parser/schema.go index 97deefceece..9c6f681efa8 100644 --- a/pkg/parser/schema.go +++ b/pkg/parser/schema.go @@ -60,13 +60,19 @@ func ValidateMainWorkflowFrontmatterWithSchema(frontmatter map[string]any) error // Filter out ignored fields before validation filtered := filterIgnoredFields(frontmatter) - // First run the standard schema validation + // First run custom validation for command trigger conflicts (provides better error messages) + if err := validateCommandTriggerConflicts(filtered); err != nil { + schemaLog.Printf("Command trigger validation failed: %v", err) + return err + } + + // Then run the standard schema validation if err := validateWithSchema(filtered, mainWorkflowSchema, "main workflow file"); err != nil { schemaLog.Printf("Schema validation failed for main workflow: %v", err) return err } - // Then run custom validation for engine-specific rules + // Finally run other custom validation rules return validateEngineSpecificRules(filtered) } @@ -75,12 +81,17 @@ func ValidateMainWorkflowFrontmatterWithSchemaAndLocation(frontmatter map[string // Filter out ignored fields before validation filtered := filterIgnoredFields(frontmatter) - // First run the standard schema validation with location + // First run custom validation for command trigger conflicts (provides better error messages) + if err := validateCommandTriggerConflicts(filtered); err != nil { + return err + } + + // Then run the standard schema validation with location if err := validateWithSchemaAndLocation(filtered, mainWorkflowSchema, "main workflow file", filePath); err != nil { return err } - // Then run custom validation for engine-specific rules + // Finally run other custom validation rules return validateEngineSpecificRules(filtered) } @@ -425,13 +436,53 @@ func min(a, b int) int { // validateEngineSpecificRules validates engine-specific rules that are not easily expressed in JSON schema func validateEngineSpecificRules(frontmatter map[string]any) error { - // Currently no engine-specific validation rules - // Network and firewall configuration is handled at the top-level, not under engine - // This function is kept as a placeholder for potential future engine-specific validation + // Custom validation rules are now handled separately + // Command trigger conflicts are validated before schema validation + // This function is kept as a placeholder for potential future validation rules _ = frontmatter return nil } +// validateCommandTriggerConflicts checks that command triggers are not used with conflicting events +func validateCommandTriggerConflicts(frontmatter map[string]any) error { + // Check if 'on' field exists and is a map + onValue, hasOn := frontmatter["on"] + if !hasOn { + return nil + } + + onMap, isMap := onValue.(map[string]any) + if !isMap { + return nil + } + + // Check if command trigger is present + commandValue, hasCommand := onMap["command"] + if !hasCommand || commandValue == nil { + return nil + } + + // List of conflicting events + conflictingEvents := []string{"issues", "issue_comment", "pull_request", "pull_request_review_comment"} + + // Check for conflicts + var foundConflicts []string + for _, eventName := range conflictingEvents { + if eventValue, hasEvent := onMap[eventName]; hasEvent && eventValue != nil { + foundConflicts = append(foundConflicts, eventName) + } + } + + if len(foundConflicts) > 0 { + if len(foundConflicts) == 1 { + return fmt.Errorf("command trigger cannot be used with '%s' event in the same workflow. Command triggers are designed to respond to slash commands in comments and should not be combined with event-based triggers for issues or pull requests", foundConflicts[0]) + } + return fmt.Errorf("command trigger cannot be used with these events in the same workflow: %s. Command triggers are designed to respond to slash commands in comments and should not be combined with event-based triggers for issues or pull requests", strings.Join(foundConflicts, ", ")) + } + + return nil +} + // findFrontmatterBounds finds the start and end indices of frontmatter in file lines // Returns: startIdx (-1 if not found), endIdx (-1 if not found), frontmatterContent func findFrontmatterBounds(lines []string) (startIdx int, endIdx int, frontmatterContent string) { diff --git a/pkg/parser/schema_test.go b/pkg/parser/schema_test.go index b83def39b37..575f7cb9b2c 100644 --- a/pkg/parser/schema_test.go +++ b/pkg/parser/schema_test.go @@ -800,6 +800,124 @@ func TestValidateMainWorkflowFrontmatterWithSchema(t *testing.T) { wantErr: true, errContains: "additional properties 'invalid' not allowed", }, + { + name: "invalid: command trigger with issues event", + frontmatter: map[string]any{ + "on": map[string]any{ + "command": map[string]any{ + "name": "test-bot", + }, + "issues": map[string]any{ + "types": []string{"opened"}, + }, + }, + }, + wantErr: true, + errContains: "command trigger cannot be used with 'issues' event", + }, + { + name: "invalid: command trigger with issue_comment event", + frontmatter: map[string]any{ + "on": map[string]any{ + "command": "test-bot", + "issue_comment": map[string]any{ + "types": []string{"created"}, + }, + }, + }, + wantErr: true, + errContains: "command trigger cannot be used with 'issue_comment' event", + }, + { + name: "invalid: command trigger with pull_request event", + frontmatter: map[string]any{ + "on": map[string]any{ + "command": map[string]any{ + "name": "test-bot", + }, + "pull_request": map[string]any{ + "types": []string{"opened"}, + }, + }, + }, + wantErr: true, + errContains: "command trigger cannot be used with 'pull_request' event", + }, + { + name: "invalid: command trigger with pull_request_review_comment event", + frontmatter: map[string]any{ + "on": map[string]any{ + "command": "test-bot", + "pull_request_review_comment": map[string]any{ + "types": []string{"created"}, + }, + }, + }, + wantErr: true, + errContains: "command trigger cannot be used with 'pull_request_review_comment' event", + }, + { + name: "invalid: command trigger with multiple conflicting events", + frontmatter: map[string]any{ + "on": map[string]any{ + "command": map[string]any{ + "name": "test-bot", + }, + "issues": map[string]any{ + "types": []string{"opened"}, + }, + "pull_request": map[string]any{ + "types": []string{"opened"}, + }, + }, + }, + wantErr: true, + errContains: "command trigger cannot be used with these events", + }, + { + name: "valid: command trigger with non-conflicting events", + frontmatter: map[string]any{ + "on": map[string]any{ + "command": map[string]any{ + "name": "test-bot", + }, + "workflow_dispatch": nil, + "schedule": []map[string]any{ + {"cron": "0 0 * * *"}, + }, + }, + }, + wantErr: false, + }, + { + name: "valid: command trigger alone", + frontmatter: map[string]any{ + "on": map[string]any{ + "command": "test-bot", + }, + }, + wantErr: false, + }, + { + name: "valid: command trigger as null (default workflow name)", + frontmatter: map[string]any{ + "on": map[string]any{ + "command": nil, + }, + }, + wantErr: false, + }, + { + name: "valid: issues event without command", + frontmatter: map[string]any{ + "on": map[string]any{ + "issues": map[string]any{ + "types": []string{"opened"}, + }, + }, + }, + wantErr: false, + }, } for _, tt := range tests { diff --git a/pkg/parser/schemas/main_workflow_schema.json b/pkg/parser/schemas/main_workflow_schema.json index 76ca4c4f11f..129a2ed381a 100644 --- a/pkg/parser/schemas/main_workflow_schema.json +++ b/pkg/parser/schemas/main_workflow_schema.json @@ -2999,6 +2999,75 @@ } }, "additionalProperties": false, + "allOf": [ + { + "if": { + "properties": { + "on": { + "type": "object", + "properties": { + "command": { + "not": { + "type": "null" + } + } + }, + "required": ["command"] + } + } + }, + "then": { + "properties": { + "on": { + "not": { + "anyOf": [ + { + "properties": { + "issues": { + "not": { + "type": "null" + } + } + }, + "required": ["issues"] + }, + { + "properties": { + "issue_comment": { + "not": { + "type": "null" + } + } + }, + "required": ["issue_comment"] + }, + { + "properties": { + "pull_request": { + "not": { + "type": "null" + } + } + }, + "required": ["pull_request"] + }, + { + "properties": { + "pull_request_review_comment": { + "not": { + "type": "null" + } + } + }, + "required": ["pull_request_review_comment"] + } + ] + } + } + } + } + } + ], "$defs": { "engine_config": { "examples": [ diff --git a/pkg/workflow/compiler_test.go b/pkg/workflow/compiler_test.go index f5a3e06d2c7..40f7bb8aaa7 100644 --- a/pkg/workflow/compiler_test.go +++ b/pkg/workflow/compiler_test.go @@ -599,7 +599,7 @@ tools: ---`, filename: "command-with-issues.md", shouldError: true, - expectedErrorMsg: "cannot use 'command' with 'issues' in the same workflow", + expectedErrorMsg: "command trigger cannot be used with 'issues' event", }, { name: "command with conflicting issue_comment event - should error", @@ -615,7 +615,7 @@ tools: ---`, filename: "command-with-issue-comment.md", shouldError: true, - expectedErrorMsg: "cannot use 'command' with 'issue_comment'", + expectedErrorMsg: "command trigger cannot be used with 'issue_comment' event", }, { name: "command with conflicting pull_request event - should error", @@ -631,7 +631,7 @@ tools: ---`, filename: "command-with-pull-request.md", shouldError: true, - expectedErrorMsg: "cannot use 'command' with 'pull_request'", + expectedErrorMsg: "command trigger cannot be used with 'pull_request' event", }, { name: "command with conflicting pull_request_review_comment event - should error", @@ -647,7 +647,7 @@ tools: ---`, filename: "command-with-pull-request-review-comment.md", shouldError: true, - expectedErrorMsg: "cannot use 'command' with 'pull_request_review_comment'", + expectedErrorMsg: "command trigger cannot be used with 'pull_request_review_comment' event", }, }