Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 36 additions & 0 deletions pkg/constants/constants.go
Original file line number Diff line number Diff line change
Expand Up @@ -658,6 +658,42 @@ var PriorityWorkflowFields = []string{"on", "permissions", "if", "network", "imp
// NOTE: This is now empty as description and applyTo are properly validated by the schema
var IgnoredFrontmatterFields = []string{}

// SharedWorkflowForbiddenFields lists fields that cannot be used in shared/included workflows.
// These fields are only allowed in main workflows (workflows with an 'on' trigger field).
//
// This list is maintained in constants.go to enable easy mining by agents and automated tools.
// The compiler enforces these restrictions at compile time with clear error messages.
//
// Forbidden fields fall into these categories:
// - Workflow triggers: on (defines it as a main workflow)
// - Workflow execution: command, run-name, runs-on, concurrency, if, timeout-minutes, timeout_minutes
// - Workflow metadata: name, tracker-id, strict
// - Workflow features: container, env, environment, sandbox, features
// - Access control: roles, github-token
//
// All other fields defined in main_workflow_schema.json can be used in shared workflows
// and will be properly imported and merged when the shared workflow is imported.
var SharedWorkflowForbiddenFields = []string{
"on", // Trigger field - only for main workflows
"command", // Command for workflow execution
"concurrency", // Concurrency control
"container", // Container configuration
"env", // Environment variables
"environment", // Deployment environment
"features", // Feature flags
"github-token", // GitHub token configuration
"if", // Conditional execution
"name", // Workflow name
"roles", // Role requirements
"run-name", // Run display name
"runs-on", // Runner specification
"sandbox", // Sandbox configuration
"strict", // Strict mode
"timeout-minutes", // Timeout in minutes
"timeout_minutes", // Timeout in minutes (underscore variant)
"tracker-id", // Tracker ID
}

func GetWorkflowDir() string {
return filepath.Join(".github", "workflows")
}
37 changes: 37 additions & 0 deletions pkg/parser/content_extractor.go
Original file line number Diff line number Diff line change
Expand Up @@ -136,6 +136,43 @@ func extractSecretMaskingFromContent(content string) (string, error) {
return extractFrontmatterField(content, "secret-masking", "{}")
}

// extractBotsFromContent extracts bots section from frontmatter as JSON string
func extractBotsFromContent(content string) (string, error) {
return extractFrontmatterField(content, "bots", "[]")
}

// extractPostStepsFromContent extracts post-steps section from frontmatter as YAML string
func extractPostStepsFromContent(content string) (string, error) {
result, err := ExtractFrontmatterFromContent(content)
if err != nil {
return "", nil // Return empty string on error
}

// Extract post-steps section
postSteps, exists := result.Frontmatter["post-steps"]
if !exists {
return "", nil
}

// Convert to YAML string (similar to how steps are handled)
postStepsYAML, err := yaml.Marshal(postSteps)
if err != nil {
return "", nil
}

return strings.TrimSpace(string(postStepsYAML)), nil
}

// extractLabelsFromContent extracts labels section from frontmatter as JSON string
func extractLabelsFromContent(content string) (string, error) {
return extractFrontmatterField(content, "labels", "[]")
}

// extractCacheFromContent extracts cache section from frontmatter as JSON string
func extractCacheFromContent(content string) (string, error) {
return extractFrontmatterField(content, "cache", "{}")
}

// extractFrontmatterField extracts a specific field from frontmatter as JSON string
func extractFrontmatterField(content, fieldName, emptyValue string) (string, error) {
result, err := ExtractFrontmatterFromContent(content)
Expand Down
59 changes: 58 additions & 1 deletion pkg/parser/import_processor.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package parser

import (
"encoding/json"
"fmt"
"os"
"sort"
Expand All @@ -25,6 +26,10 @@ type ImportsResult struct {
MergedNetwork string // Merged network configuration from all imports
MergedPermissions string // Merged permissions configuration from all imports
MergedSecretMasking string // Merged secret-masking steps from all imports
MergedBots []string // Merged bots list from all imports (union of bot names)
MergedPostSteps string // Merged post-steps configuration from all imports (appended in order)
MergedLabels []string // Merged labels from all imports (union of label names)
MergedCaches []string // Merged cache configurations from all imports (appended in order)
ImportedFiles []string // List of imported file paths (for manifest)
AgentFile string // Path to custom agent file (if imported)
// ImportInputs uses map[string]any because input values can be different types (string, number, boolean).
Expand Down Expand Up @@ -163,10 +168,16 @@ func processImportsFromFrontmatterWithManifestAndSource(frontmatter map[string]a
var networkBuilder strings.Builder
var permissionsBuilder strings.Builder
var secretMaskingBuilder strings.Builder
var postStepsBuilder strings.Builder
var engines []string
var safeOutputs []string
var safeInputs []string
var agentFile string // Track custom agent file
var bots []string // Track unique bot names
botsSet := make(map[string]bool) // Set for deduplicating bots
var labels []string // Track unique labels
labelsSet := make(map[string]bool) // Set for deduplicating labels
var caches []string // Track cache configurations (appended in order)
var agentFile string // Track custom agent file
importInputs := make(map[string]any) // Aggregated input values from all imports

// Seed the queue with initial imports
Expand Down Expand Up @@ -432,6 +443,48 @@ func processImportsFromFrontmatterWithManifestAndSource(frontmatter map[string]a
if err == nil && secretMaskingContent != "" && secretMaskingContent != "{}" {
secretMaskingBuilder.WriteString(secretMaskingContent + "\n")
}

// Extract bots from imported file (merge into set to avoid duplicates)
botsContent, err := extractBotsFromContent(string(content))
if err == nil && botsContent != "" && botsContent != "[]" {
// Parse bots JSON array
var importedBots []string
if jsonErr := json.Unmarshal([]byte(botsContent), &importedBots); jsonErr == nil {
for _, bot := range importedBots {
if !botsSet[bot] {
botsSet[bot] = true
bots = append(bots, bot)
}
}
}
}

// Extract post-steps from imported file (append in order)
postStepsContent, err := extractPostStepsFromContent(string(content))
if err == nil && postStepsContent != "" {
postStepsBuilder.WriteString(postStepsContent + "\n")
}

// Extract labels from imported file (merge into set to avoid duplicates)
labelsContent, err := extractLabelsFromContent(string(content))
if err == nil && labelsContent != "" && labelsContent != "[]" {
// Parse labels JSON array
var importedLabels []string
if jsonErr := json.Unmarshal([]byte(labelsContent), &importedLabels); jsonErr == nil {
for _, label := range importedLabels {
if !labelsSet[label] {
labelsSet[label] = true
labels = append(labels, label)
}
}
}
}

// Extract cache from imported file (append to list of caches)
cacheContent, err := extractCacheFromContent(string(content))
if err == nil && cacheContent != "" && cacheContent != "{}" {
caches = append(caches, cacheContent)
}
}

log.Printf("Completed BFS traversal. Processed %d imports in total", len(processedOrder))
Expand All @@ -453,6 +506,10 @@ func processImportsFromFrontmatterWithManifestAndSource(frontmatter map[string]a
MergedNetwork: networkBuilder.String(),
MergedPermissions: permissionsBuilder.String(),
MergedSecretMasking: secretMaskingBuilder.String(),
MergedBots: bots,
MergedPostSteps: postStepsBuilder.String(),
MergedLabels: labels,
MergedCaches: caches,
ImportedFiles: topologicalOrder,
AgentFile: agentFile,
ImportInputs: importInputs,
Expand Down
4 changes: 2 additions & 2 deletions pkg/parser/include_processor.go
Original file line number Diff line number Diff line change
Expand Up @@ -144,8 +144,8 @@ func processIncludedFileWithVisited(filePath, sectionName string, extractTools b
} else {
// For non-workflow files, fall back to relaxed validation with warnings
if len(result.Frontmatter) > 0 {
// Valid fields for non-workflow frontmatter (fields that should not trigger warnings)
// This list should match the properties defined in included_file_schema.json
// Valid fields for non-workflow frontmatter (fields that are allowed in shared workflows)
// This list matches the allowed fields in shared workflows (main_workflow_schema minus forbidden fields)
validFields := map[string]bool{
"tools": true,
"engine": true,
Expand Down
16 changes: 0 additions & 16 deletions pkg/parser/schema_compiler.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,25 +21,19 @@ var schemaCompilerLog = logger.New("parser:schema_compiler")
//go:embed schemas/main_workflow_schema.json
var mainWorkflowSchema string

//go:embed schemas/included_file_schema.json
var includedFileSchema string

//go:embed schemas/mcp_config_schema.json
var mcpConfigSchema string

// validateWithSchema validates frontmatter against a JSON schema
// Cached compiled schemas to avoid recompiling on every validation
var (
mainWorkflowSchemaOnce sync.Once
includedFileSchemaOnce sync.Once
mcpConfigSchemaOnce sync.Once

compiledMainWorkflowSchema *jsonschema.Schema
compiledIncludedFileSchema *jsonschema.Schema
compiledMcpConfigSchema *jsonschema.Schema

mainWorkflowSchemaError error
includedFileSchemaError error
mcpConfigSchemaError error
)

Expand All @@ -51,14 +45,6 @@ func getCompiledMainWorkflowSchema() (*jsonschema.Schema, error) {
return compiledMainWorkflowSchema, mainWorkflowSchemaError
}

// getCompiledIncludedFileSchema returns the compiled included file schema, compiling it once and caching
func getCompiledIncludedFileSchema() (*jsonschema.Schema, error) {
includedFileSchemaOnce.Do(func() {
compiledIncludedFileSchema, includedFileSchemaError = compileSchema(includedFileSchema, "http://contoso.com/included-file-schema.json")
})
return compiledIncludedFileSchema, includedFileSchemaError
}

// getCompiledMcpConfigSchema returns the compiled MCP config schema, compiling it once and caching
func getCompiledMcpConfigSchema() (*jsonschema.Schema, error) {
mcpConfigSchemaOnce.Do(func() {
Expand Down Expand Up @@ -158,8 +144,6 @@ func validateWithSchema(frontmatter map[string]any, schemaJSON, context string)
switch schemaJSON {
case mainWorkflowSchema:
schema, err = getCompiledMainWorkflowSchema()
case includedFileSchema:
schema, err = getCompiledIncludedFileSchema()
case mcpConfigSchema:
schema, err = getCompiledMcpConfigSchema()
default:
Expand Down
6 changes: 3 additions & 3 deletions pkg/parser/schema_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1278,7 +1278,7 @@ func TestValidateIncludedFileFrontmatterWithSchema(t *testing.T) {
"tools": map[string]any{"github": "test"},
},
wantErr: true,
errContains: "additional properties 'on' not allowed",
errContains: "cannot be used in shared workflows",
},
{
name: "invalid frontmatter with multiple unexpected keys",
Expand All @@ -1288,7 +1288,7 @@ func TestValidateIncludedFileFrontmatterWithSchema(t *testing.T) {
"tools": map[string]any{"github": "test"},
},
wantErr: true,
errContains: "additional properties",
errContains: "cannot be used in shared workflows",
},
{
name: "invalid frontmatter with only unexpected keys",
Expand All @@ -1297,7 +1297,7 @@ func TestValidateIncludedFileFrontmatterWithSchema(t *testing.T) {
"permissions": "read",
},
wantErr: true,
errContains: "additional properties",
errContains: "cannot be used in shared workflows",
},
{
name: "valid frontmatter with complex tools object",
Expand Down
73 changes: 67 additions & 6 deletions pkg/parser/schema_validation.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,44 @@ package parser
import (
"fmt"

"github.com/githubnext/gh-aw/pkg/constants"
"github.com/githubnext/gh-aw/pkg/logger"
)

var schemaValidationLog = logger.New("parser:schema_validation")

// sharedWorkflowForbiddenFields is a map for O(1) lookup of forbidden fields in shared workflows
var sharedWorkflowForbiddenFields = buildForbiddenFieldsMap()

// buildForbiddenFieldsMap converts the SharedWorkflowForbiddenFields slice to a map for efficient lookup
func buildForbiddenFieldsMap() map[string]bool {
forbiddenMap := make(map[string]bool)
for _, field := range constants.SharedWorkflowForbiddenFields {
forbiddenMap[field] = true
}
return forbiddenMap
}

// validateSharedWorkflowFields checks that a shared workflow doesn't contain forbidden fields
func validateSharedWorkflowFields(frontmatter map[string]any) error {
var forbiddenFound []string

for key := range frontmatter {
if sharedWorkflowForbiddenFields[key] {
forbiddenFound = append(forbiddenFound, key)
}
}

if len(forbiddenFound) > 0 {
if len(forbiddenFound) == 1 {
return fmt.Errorf("field '%s' cannot be used in shared workflows (only allowed in main workflows with 'on' trigger)", forbiddenFound[0])
}
return fmt.Errorf("fields %v cannot be used in shared workflows (only allowed in main workflows with 'on' trigger)", forbiddenFound)
}

return nil
}

// ValidateMainWorkflowFrontmatterWithSchema validates main workflow frontmatter using JSON schema
func ValidateMainWorkflowFrontmatterWithSchema(frontmatter map[string]any) error {
schemaValidationLog.Print("Validating main workflow frontmatter with schema")
Expand Down Expand Up @@ -57,13 +90,28 @@ func ValidateIncludedFileFrontmatterWithSchema(frontmatter map[string]any) error
// Filter out ignored fields before validation
filtered := filterIgnoredFields(frontmatter)

// First run the standard schema validation
if err := validateWithSchema(filtered, includedFileSchema, "included file"); err != nil {
// First check for forbidden fields in shared workflows
if err := validateSharedWorkflowFields(filtered); err != nil {
schemaValidationLog.Printf("Shared workflow field validation failed: %v", err)
return err
}

// To validate shared workflows against the main schema, we temporarily add an 'on' field
// This allows us to use the full schema validation while still enforcing the forbidden field check above
tempFrontmatter := make(map[string]any)
for k, v := range filtered {
tempFrontmatter[k] = v
}
// Add a temporary 'on' field to satisfy the schema's required field
tempFrontmatter["on"] = "push"

// Validate with the main schema (which will catch unknown fields)
if err := validateWithSchema(tempFrontmatter, mainWorkflowSchema, "included file"); err != nil {
schemaValidationLog.Printf("Schema validation failed for included file: %v", err)
return err
}

// Then run custom validation for engine-specific rules
// Run custom validation for engine-specific rules
return validateEngineSpecificRules(filtered)
}

Expand All @@ -72,12 +120,25 @@ func ValidateIncludedFileFrontmatterWithSchemaAndLocation(frontmatter map[string
// Filter out ignored fields before validation
filtered := filterIgnoredFields(frontmatter)

// First run the standard schema validation with location
if err := validateWithSchemaAndLocation(filtered, includedFileSchema, "included file", filePath); err != nil {
// First check for forbidden fields in shared workflows
if err := validateSharedWorkflowFields(filtered); err != nil {
return err
}

// To validate shared workflows against the main schema, we temporarily add an 'on' field
tempFrontmatter := make(map[string]any)
for k, v := range filtered {
tempFrontmatter[k] = v
}
// Add a temporary 'on' field to satisfy the schema's required field
tempFrontmatter["on"] = "push"

// Validate with the main schema (which will catch unknown fields)
if err := validateWithSchemaAndLocation(tempFrontmatter, mainWorkflowSchema, "included file", filePath); err != nil {
return err
}

// Then run custom validation for engine-specific rules
// Run custom validation for engine-specific rules
return validateEngineSpecificRules(filtered)
}

Expand Down
Loading
Loading