From 48f48687c3374cb68ff0674bfffa0a84ee2f08fc Mon Sep 17 00:00:00 2001 From: Don Syme Date: Sat, 25 Oct 2025 16:46:03 +0100 Subject: [PATCH 1/4] improve compilation timing reporting and perf --- pkg/workflow/compiler.go | 137 ++++++++++++++--------- pkg/workflow/compiler_yaml.go | 20 +++- pkg/workflow/timing.go | 201 ++++++++++++++++++++++++++++++++++ pkg/workflow/validation.go | 167 +++++++++++++++++++++++++++- 4 files changed, 463 insertions(+), 62 deletions(-) create mode 100644 pkg/workflow/timing.go diff --git a/pkg/workflow/compiler.go b/pkg/workflow/compiler.go index 15b78e816b5..4d76211032d 100644 --- a/pkg/workflow/compiler.go +++ b/pkg/workflow/compiler.go @@ -62,6 +62,7 @@ type Compiler struct { fileTracker FileTracker // Optional file tracker for tracking created files warningCount int // Number of warnings encountered during compilation stepOrderTracker *StepOrderTracker // Tracks step ordering for validation + timingTracker *TimingTracker // Tracks compilation timing for performance analysis } // NewCompiler creates a new workflow compiler with optional configuration @@ -74,6 +75,7 @@ func NewCompiler(verbose bool, engineOverride string, version string) *Compiler jobManager: NewJobManager(), engineRegistry: GetGlobalEngineRegistry(), stepOrderTracker: NewStepOrderTracker(), + timingTracker: NewTimingTracker(verbose), } return c @@ -221,10 +223,17 @@ type SafeOutputsConfig struct { // CompileWorkflow converts a markdown workflow to GitHub Actions YAML func (c *Compiler) CompileWorkflow(markdownPath string) error { + // Initialize timing tracker + c.timingTracker = NewTimingTracker(c.verbose) + // Parse the markdown file - log.Printf("Parsing workflow file") - workflowData, err := c.ParseWorkflowFile(markdownPath) - if err != nil { + var workflowData *WorkflowData + if err := c.timingTracker.TimeStep("Parse Workflow File", func() error { + log.Printf("Parsing workflow file") + var err error + workflowData, err = c.ParseWorkflowFile(markdownPath) + return err + }); err != nil { // Check if this is already a formatted console error if strings.Contains(err.Error(), ":") && (strings.Contains(err.Error(), "error:") || strings.Contains(err.Error(), "warning:")) { // Already formatted, return as-is @@ -249,6 +258,9 @@ func (c *Compiler) CompileWorkflow(markdownPath string) error { // CompileWorkflowData compiles a workflow from already-parsed WorkflowData // This avoids re-parsing when the data has already been parsed func (c *Compiler) CompileWorkflowData(workflowData *WorkflowData, markdownPath string) error { + // Initialize a new timing tracker for this compilation + c.timingTracker = NewTimingTracker(c.verbose) + c.timingTracker.StartStep("Compilation Setup") // Reset the step order tracker for this compilation c.stepOrderTracker = NewStepOrderTracker() @@ -257,10 +269,13 @@ func (c *Compiler) CompileWorkflowData(workflowData *WorkflowData, markdownPath lockFile := strings.TrimSuffix(markdownPath, ".md") + ".lock.yml" log.Printf("Starting compilation: %s -> %s", markdownPath, lockFile) + c.timingTracker.EndStep() // Validate expression safety - check that all GitHub Actions expressions are in the allowed list - log.Printf("Validating expression safety") - if err := validateExpressionSafety(workflowData.MarkdownContent); err != nil { + if err := c.timingTracker.TimeStep("Expression Safety Validation", func() error { + log.Printf("Validating expression safety") + return validateExpressionSafety(workflowData.MarkdownContent) + }); err != nil { formattedErr := console.FormatError(console.CompilerError{ Position: console.ErrorPosition{ File: markdownPath, @@ -281,8 +296,12 @@ func (c *Compiler) CompileWorkflowData(workflowData *WorkflowData, markdownPath // instead of using a shared action file // Generate the YAML content - yamlContent, err := c.generateYAML(workflowData, markdownPath) - if err != nil { + var yamlContent string + if err := c.timingTracker.TimeStep("YAML Generation", func() error { + var err error + yamlContent, err = c.generateYAML(workflowData, markdownPath) + return err + }); err != nil { formattedErr := console.FormatError(console.CompilerError{ Position: console.ErrorPosition{ File: markdownPath, @@ -297,8 +316,12 @@ func (c *Compiler) CompileWorkflowData(workflowData *WorkflowData, markdownPath // Validate against GitHub Actions schema (unless skipped) if !c.skipValidation { - log.Print("Validating workflow against GitHub Actions schema") - if err := c.validateGitHubActionsSchema(yamlContent); err != nil { + c.timingTracker.StartStep("Validation") + + if err := c.timingTracker.TimeSubStep("GitHub Actions Schema", func() error { + log.Print("Validating workflow against GitHub Actions schema") + return c.validateGitHubActionsSchema(yamlContent) + }); err != nil { formattedErr := console.FormatError(console.CompilerError{ Position: console.ErrorPosition{ File: markdownPath, @@ -317,8 +340,10 @@ func (c *Compiler) CompileWorkflowData(workflowData *WorkflowData, markdownPath } // Validate expression sizes - log.Print("Validating expression sizes") - if err := c.validateExpressionSizes(yamlContent); err != nil { + if err := c.timingTracker.TimeSubStep("Expression Sizes", func() error { + log.Print("Validating expression sizes") + return c.validateExpressionSizes(yamlContent) + }); err != nil { formattedErr := console.FormatError(console.CompilerError{ Position: console.ErrorPosition{ File: markdownPath, @@ -337,26 +362,31 @@ func (c *Compiler) CompileWorkflowData(workflowData *WorkflowData, markdownPath } // Validate container images used in MCP configurations - log.Print("Validating container images") - if err := c.validateContainerImages(workflowData); err != nil { - // Treat container image validation failures as warnings, not errors - // This is because validation may fail due to auth issues locally (e.g., private registries) - formattedWarning := console.FormatError(console.CompilerError{ - Position: console.ErrorPosition{ - File: markdownPath, - Line: 1, - Column: 1, - }, - Type: "warning", - Message: fmt.Sprintf("container image validation failed: %v", err), - }) - fmt.Fprintln(os.Stderr, formattedWarning) - c.IncrementWarningCount() - } + c.timingTracker.TimeSubStep("Container Images", func() error { + log.Print("Validating container images") + if err := c.validateContainerImages(workflowData); err != nil { + // Treat container image validation failures as warnings, not errors + // This is because validation may fail due to auth issues locally (e.g., private registries) + formattedWarning := console.FormatError(console.CompilerError{ + Position: console.ErrorPosition{ + File: markdownPath, + Line: 1, + Column: 1, + }, + Type: "warning", + Message: fmt.Sprintf("container image validation failed: %v", err), + }) + fmt.Fprintln(os.Stderr, formattedWarning) + c.IncrementWarningCount() + } + return nil // Don't fail on container image validation + }) // Validate runtime packages (npx, uv) - log.Print("Validating runtime packages") - if err := c.validateRuntimePackages(workflowData); err != nil { + if err := c.timingTracker.TimeSubStep("Runtime Packages", func() error { + log.Print("Validating runtime packages") + return c.validateRuntimePackages(workflowData) + }); err != nil { formattedErr := console.FormatError(console.CompilerError{ Position: console.ErrorPosition{ File: markdownPath, @@ -370,8 +400,10 @@ func (c *Compiler) CompileWorkflowData(workflowData *WorkflowData, markdownPath } // Validate repository features (discussions, issues) - log.Print("Validating repository features") - if err := c.validateRepositoryFeatures(workflowData); err != nil { + if err := c.timingTracker.TimeSubStep("Repository Features", func() error { + log.Print("Validating repository features") + return c.validateRepositoryFeatures(workflowData) + }); err != nil { formattedErr := console.FormatError(console.CompilerError{ Position: console.ErrorPosition{ File: markdownPath, @@ -383,6 +415,8 @@ func (c *Compiler) CompileWorkflowData(workflowData *WorkflowData, markdownPath }) return errors.New(formattedErr) } + + c.timingTracker.EndStep() } else if c.verbose { fmt.Println(console.FormatWarningMessage("Schema validation available but skipped (use SetSkipValidation(false) to enable)")) c.IncrementWarningCount() @@ -392,8 +426,22 @@ func (c *Compiler) CompileWorkflowData(workflowData *WorkflowData, markdownPath if c.noEmit { log.Print("Validation completed - no lock file generated (--no-emit enabled)") } else { - log.Printf("Writing output to: %s", lockFile) - if err := os.WriteFile(lockFile, []byte(yamlContent), 0644); err != nil { + if err := c.timingTracker.TimeStep("File Output", func() error { + log.Printf("Writing output to: %s", lockFile) + if err := os.WriteFile(lockFile, []byte(yamlContent), 0644); err != nil { + return fmt.Errorf("failed to write lock file: %w", err) + } + + // Validate file size after writing + if lockFileInfo, err := os.Stat(lockFile); err == nil { + if lockFileInfo.Size() > MaxLockFileSize { + lockSize := pretty.FormatFileSize(lockFileInfo.Size()) + maxSize := pretty.FormatFileSize(MaxLockFileSize) + return fmt.Errorf("generated lock file size (%s) exceeds maximum allowed size (%s)", lockSize, maxSize) + } + } + return nil + }); err != nil { formattedErr := console.FormatError(console.CompilerError{ Position: console.ErrorPosition{ File: lockFile, @@ -401,30 +449,15 @@ func (c *Compiler) CompileWorkflowData(workflowData *WorkflowData, markdownPath Column: 1, }, Type: "error", - Message: fmt.Sprintf("failed to write lock file: %v", err), + Message: err.Error(), }) return errors.New(formattedErr) } - - // Validate file size after writing - if lockFileInfo, err := os.Stat(lockFile); err == nil { - if lockFileInfo.Size() > MaxLockFileSize { - lockSize := pretty.FormatFileSize(lockFileInfo.Size()) - maxSize := pretty.FormatFileSize(MaxLockFileSize) - formattedErr := console.FormatError(console.CompilerError{ - Position: console.ErrorPosition{ - File: lockFile, - Line: 1, - Column: 1, - }, - Type: "error", - Message: fmt.Sprintf("generated lock file size (%s) exceeds maximum allowed size (%s)", lockSize, maxSize), - }) - return errors.New(formattedErr) - } - } } + // Print timing summary if verbose mode is enabled + c.timingTracker.PrintSummary() + // Display success message with file size if we generated a lock file if c.noEmit { fmt.Println(console.FormatSuccessMessage(console.ToRelativePath(markdownPath))) diff --git a/pkg/workflow/compiler_yaml.go b/pkg/workflow/compiler_yaml.go index 16f53fd2d40..b600eab0662 100644 --- a/pkg/workflow/compiler_yaml.go +++ b/pkg/workflow/compiler_yaml.go @@ -9,16 +9,22 @@ import ( ) func (c *Compiler) generateYAML(data *WorkflowData, markdownPath string) (string, error) { + c.timingTracker.StartSubStep("Job Setup") // Reset job manager for this compilation c.jobManager = NewJobManager() + c.timingTracker.EndSubStep() // Build all jobs - if err := c.buildJobs(data, markdownPath); err != nil { + if err := c.timingTracker.TimeSubStep("Build Jobs", func() error { + return c.buildJobs(data, markdownPath) + }); err != nil { return "", fmt.Errorf("failed to build jobs: %w", err) } // Validate job dependencies - if err := c.jobManager.ValidateDependencies(); err != nil { + if err := c.timingTracker.TimeSubStep("Validate Job Dependencies", func() error { + return c.jobManager.ValidateDependencies() + }); err != nil { return "", fmt.Errorf("job dependency validation failed: %w", err) } @@ -113,14 +119,20 @@ func (c *Compiler) generateYAML(data *WorkflowData, markdownPath string) (string } // Generate jobs section using JobManager - yaml.WriteString(c.jobManager.RenderToYAML()) + c.timingTracker.TimeSubStep("Generate Jobs YAML", func() error { + yaml.WriteString(c.jobManager.RenderToYAML()) + return nil + }) yamlContent := yaml.String() // If we're in non-cloning trial mode and this workflow has issue triggers, // replace github.event.issue.number with inputs.issue_number if c.trialMode && c.hasIssueTrigger(data.On) { - yamlContent = c.replaceIssueNumberReferences(yamlContent) + c.timingTracker.TimeSubStep("Trial Mode Processing", func() error { + yamlContent = c.replaceIssueNumberReferences(yamlContent) + return nil + }) } return yamlContent, nil diff --git a/pkg/workflow/timing.go b/pkg/workflow/timing.go new file mode 100644 index 00000000000..44275037fb8 --- /dev/null +++ b/pkg/workflow/timing.go @@ -0,0 +1,201 @@ +package workflow + +import ( + "fmt" + "os" + "time" + + "github.com/githubnext/gh-aw/pkg/console" + "github.com/githubnext/gh-aw/pkg/logger" +) + +var timingLog = logger.New("workflow:timing") + +// TimingTracker tracks the duration of compilation steps for performance analysis +type TimingTracker struct { + verbose bool + startTime time.Time + steps []TimingStep + currentStep *TimingStep +} + +// TimingStep represents a single timed step in the compilation process +type TimingStep struct { + Name string + StartTime time.Time + EndTime time.Time + Duration time.Duration + SubSteps []TimingStep + currentSub *TimingStep +} + +// NewTimingTracker creates a new timing tracker +func NewTimingTracker(verbose bool) *TimingTracker { + return &TimingTracker{ + verbose: verbose, + startTime: time.Now(), + steps: make([]TimingStep, 0), + } +} + +// StartStep starts timing a new compilation step +func (t *TimingTracker) StartStep(stepName string) { + if !t.verbose { + return + } + + now := time.Now() + step := TimingStep{ + Name: stepName, + StartTime: now, + SubSteps: make([]TimingStep, 0), + } + + timingLog.Printf("Starting step: %s", stepName) + + // End the previous step if there is one + if t.currentStep != nil { + t.EndStep() + } + + t.currentStep = &step +} + +// StartSubStep starts timing a sub-step within the current step +func (t *TimingTracker) StartSubStep(subStepName string) { + if !t.verbose || t.currentStep == nil { + return + } + + now := time.Now() + subStep := TimingStep{ + Name: subStepName, + StartTime: now, + SubSteps: make([]TimingStep, 0), + } + + timingLog.Printf("Starting sub-step: %s", subStepName) + + // End the previous sub-step if there is one + if t.currentStep.currentSub != nil { + t.EndSubStep() + } + + t.currentStep.currentSub = &subStep +} + +// EndSubStep ends timing the current sub-step +func (t *TimingTracker) EndSubStep() { + if !t.verbose || t.currentStep == nil || t.currentStep.currentSub == nil { + return + } + + now := time.Now() + t.currentStep.currentSub.EndTime = now + t.currentStep.currentSub.Duration = now.Sub(t.currentStep.currentSub.StartTime) + + timingLog.Printf("Completed sub-step: %s (took %v)", + t.currentStep.currentSub.Name, t.currentStep.currentSub.Duration) + + // Add the completed sub-step to the current step + t.currentStep.SubSteps = append(t.currentStep.SubSteps, *t.currentStep.currentSub) + t.currentStep.currentSub = nil +} + +// EndStep ends timing the current step +func (t *TimingTracker) EndStep() { + if !t.verbose || t.currentStep == nil { + return + } + + now := time.Now() + + // End any pending sub-step + if t.currentStep.currentSub != nil { + t.EndSubStep() + } + + t.currentStep.EndTime = now + t.currentStep.Duration = now.Sub(t.currentStep.StartTime) + + timingLog.Printf("Completed step: %s (took %v)", t.currentStep.Name, t.currentStep.Duration) + + // Add the completed step to the list + t.steps = append(t.steps, *t.currentStep) + t.currentStep = nil +} + +// PrintSummary displays a summary of all timing information +func (t *TimingTracker) PrintSummary() { + if !t.verbose { + return + } + + // End any pending step + if t.currentStep != nil { + t.EndStep() + } + + totalDuration := time.Since(t.startTime) + + fmt.Fprintln(os.Stderr, console.FormatInfoMessage("")) + fmt.Fprintln(os.Stderr, console.FormatInfoMessage("=== Compilation Timing Summary ===")) + fmt.Fprintln(os.Stderr, console.FormatInfoMessage(fmt.Sprintf("Total compilation time: %v", formatDuration(totalDuration)))) + fmt.Fprintln(os.Stderr, console.FormatInfoMessage("")) + + if len(t.steps) == 0 { + fmt.Fprintln(os.Stderr, console.FormatInfoMessage("No timing steps recorded")) + return + } + + for i, step := range t.steps { + percentage := float64(step.Duration) / float64(totalDuration) * 100 + fmt.Fprintln(os.Stderr, console.FormatInfoMessage( + fmt.Sprintf("%d. %s: %v (%.1f%%)", + i+1, step.Name, formatDuration(step.Duration), percentage))) + + // Print sub-steps if any + for j, subStep := range step.SubSteps { + subPercentage := float64(subStep.Duration) / float64(step.Duration) * 100 + fmt.Fprintln(os.Stderr, console.FormatInfoMessage( + fmt.Sprintf(" %d.%d %s: %v (%.1f%% of step)", + i+1, j+1, subStep.Name, formatDuration(subStep.Duration), subPercentage))) + } + } + + fmt.Fprintln(os.Stderr, console.FormatInfoMessage("")) +} + +// GetTotalDuration returns the total time elapsed since the tracker was created +func (t *TimingTracker) GetTotalDuration() time.Duration { + return time.Since(t.startTime) +} + +// formatDuration formats a duration for human-readable display +func formatDuration(d time.Duration) string { + if d < time.Millisecond { + return fmt.Sprintf("%dμs", d.Microseconds()) + } else if d < time.Second { + return fmt.Sprintf("%.1fms", float64(d.Nanoseconds())/1_000_000) + } else if d < time.Minute { + return fmt.Sprintf("%.2fs", d.Seconds()) + } else { + minutes := int(d.Minutes()) + seconds := d.Seconds() - float64(minutes*60) + return fmt.Sprintf("%dm %.1fs", minutes, seconds) + } +} + +// TimeStep is a helper function to measure and log the duration of a function call +func (t *TimingTracker) TimeStep(stepName string, fn func() error) error { + t.StartStep(stepName) + defer t.EndStep() + return fn() +} + +// TimeSubStep is a helper function to measure and log the duration of a sub-step function call +func (t *TimingTracker) TimeSubStep(subStepName string, fn func() error) error { + t.StartSubStep(subStepName) + defer t.EndSubStep() + return fn() +} \ No newline at end of file diff --git a/pkg/workflow/validation.go b/pkg/workflow/validation.go index cddcf7dbf3d..849f3b9078d 100644 --- a/pkg/workflow/validation.go +++ b/pkg/workflow/validation.go @@ -19,6 +19,33 @@ import ( var validationLog = logger.New("workflow:validation") +// RepositoryFeatures holds cached information about repository capabilities +type RepositoryFeatures struct { + HasDiscussions bool + HasIssues bool +} + +// Global cache for repository features to amortize API calls across multiple workflow compilations +var ( + repositoryFeaturesCache = make(map[string]*RepositoryFeatures) + repositoryFeaturesMutex = sync.RWMutex{} + currentRepositoryCache = "" + currentRepositoryMutex = sync.RWMutex{} +) + +// ClearRepositoryFeaturesCache clears the repository features cache +// This is useful for testing or when repository settings might have changed +func ClearRepositoryFeaturesCache() { + repositoryFeaturesMutex.Lock() + currentRepositoryMutex.Lock() + defer repositoryFeaturesMutex.Unlock() + defer currentRepositoryMutex.Unlock() + + repositoryFeaturesCache = make(map[string]*RepositoryFeatures) + currentRepositoryCache = "" + validationLog.Print("Repository features and current repository caches cleared") +} + // validateExpressionSizes validates that no expression values in the generated YAML exceed GitHub Actions limits func (c *Compiler) validateExpressionSizes(yamlContent string) error { validationLog.Print("Validating expression sizes in generated YAML") @@ -326,14 +353,27 @@ func (c *Compiler) validateRepositoryFeatures(workflowData *WorkflowData) error // Get the repository from the current git context // This will work when running in a git repository - repo, err := getCurrentRepository() + var repo string + if c.timingTracker != nil && c.timingTracker.verbose { + c.timingTracker.StartSubStep("Get Repository Info") + } + + var err error + repo, err = getCurrentRepository() if err != nil { validationLog.Printf("Could not determine repository: %v", err) // Don't fail if we can't determine the repository (e.g., not in a git repo) // This allows validation to pass in non-git environments + if c.timingTracker != nil && c.timingTracker.verbose { + c.timingTracker.EndSubStep() + } return nil } + if c.timingTracker != nil && c.timingTracker.verbose { + c.timingTracker.EndSubStep() + } + validationLog.Printf("Checking repository features for: %s", repo) // Check if discussions are enabled when create-discussion or add-comment with discussion: true is configured @@ -343,7 +383,19 @@ func (c *Compiler) validateRepositoryFeatures(workflowData *WorkflowData) error *workflowData.SafeOutputs.AddComments.Discussion) if needsDiscussions { - hasDiscussions, err := checkRepositoryHasDiscussions(repo) + var hasDiscussions bool + var err error + + if c.timingTracker != nil && c.timingTracker.verbose { + c.timingTracker.StartSubStep("Check Discussions API") + } + + hasDiscussions, err = checkRepositoryHasDiscussions(repo) + + if c.timingTracker != nil && c.timingTracker.verbose { + c.timingTracker.EndSubStep() + } + if err != nil { // If we can't check, log but don't fail // This could happen due to network issues or auth problems @@ -371,7 +423,19 @@ func (c *Compiler) validateRepositoryFeatures(workflowData *WorkflowData) error // Check if issues are enabled when create-issue is configured if workflowData.SafeOutputs.CreateIssues != nil { - hasIssues, err := checkRepositoryHasIssues(repo) + var hasIssues bool + var err error + + if c.timingTracker != nil && c.timingTracker.verbose { + c.timingTracker.StartSubStep("Check Issues API") + } + + hasIssues, err = checkRepositoryHasIssues(repo) + + if c.timingTracker != nil && c.timingTracker.verbose { + c.timingTracker.EndSubStep() + } + if err != nil { // If we can't check, log but don't fail validationLog.Printf("Warning: Could not check if issues are enabled: %v", err) @@ -395,8 +459,30 @@ func (c *Compiler) validateRepositoryFeatures(workflowData *WorkflowData) error return nil } -// getCurrentRepository gets the current repository from git context +// getCurrentRepository gets the current repository from git context (with caching) func getCurrentRepository() (string, error) { + // Check cache first (read lock) + currentRepositoryMutex.RLock() + if currentRepositoryCache != "" { + repo := currentRepositoryCache + currentRepositoryMutex.RUnlock() + validationLog.Printf("Using cached current repository: %s", repo) + return repo, nil + } + currentRepositoryMutex.RUnlock() + + // Not in cache, acquire write lock and check again (double-checked locking) + currentRepositoryMutex.Lock() + defer currentRepositoryMutex.Unlock() + + // Check cache again in case another goroutine populated it + if currentRepositoryCache != "" { + validationLog.Printf("Using cached current repository: %s", currentRepositoryCache) + return currentRepositoryCache, nil + } + + validationLog.Print("Fetching current repository from gh CLI") + // Use gh CLI to get the current repository // This works when in a git repository with GitHub remote stdOut, _, err := gh.Exec("repo", "view", "--json", "nameWithOwner", "-q", ".nameWithOwner") @@ -409,11 +495,71 @@ func getCurrentRepository() (string, error) { return "", fmt.Errorf("repository name is empty") } + // Cache the result + currentRepositoryCache = repo + validationLog.Printf("Cached current repository: %s", repo) + return repo, nil } -// checkRepositoryHasDiscussions checks if a repository has discussions enabled +// getRepositoryFeatures gets repository features with caching to amortize API calls +func getRepositoryFeatures(repo string) (*RepositoryFeatures, error) { + // Check cache first (read lock) + repositoryFeaturesMutex.RLock() + if features, exists := repositoryFeaturesCache[repo]; exists { + repositoryFeaturesMutex.RUnlock() + validationLog.Printf("Using cached repository features for: %s", repo) + return features, nil + } + repositoryFeaturesMutex.RUnlock() + + // Not in cache, acquire write lock and check again (double-checked locking) + repositoryFeaturesMutex.Lock() + defer repositoryFeaturesMutex.Unlock() + + // Check cache again in case another goroutine populated it + if features, exists := repositoryFeaturesCache[repo]; exists { + validationLog.Printf("Using cached repository features for: %s", repo) + return features, nil + } + + validationLog.Printf("Fetching repository features from API for: %s", repo) + + // Fetch from API + features := &RepositoryFeatures{} + + // Check discussions + hasDiscussions, err := checkRepositoryHasDiscussionsUncached(repo) + if err != nil { + return nil, fmt.Errorf("failed to check discussions: %w", err) + } + features.HasDiscussions = hasDiscussions + + // Check issues + hasIssues, err := checkRepositoryHasIssuesUncached(repo) + if err != nil { + return nil, fmt.Errorf("failed to check issues: %w", err) + } + features.HasIssues = hasIssues + + // Cache the result + repositoryFeaturesCache[repo] = features + validationLog.Printf("Cached repository features for: %s (discussions: %v, issues: %v)", repo, hasDiscussions, hasIssues) + + return features, nil +} + +// checkRepositoryHasDiscussions checks if a repository has discussions enabled (with caching) func checkRepositoryHasDiscussions(repo string) (bool, error) { + features, err := getRepositoryFeatures(repo) + if err != nil { + return false, err + } + return features.HasDiscussions, nil +} + +// checkRepositoryHasDiscussionsUncached checks if a repository has discussions enabled (no caching) +func checkRepositoryHasDiscussionsUncached(repo string) (bool, error) { // Use GitHub GraphQL API to check if discussions are enabled // The hasDiscussionsEnabled field is the canonical way to check this query := `query($owner: String!, $name: String!) { @@ -452,8 +598,17 @@ func checkRepositoryHasDiscussions(repo string) (bool, error) { return response.Data.Repository.HasDiscussionsEnabled, nil } -// checkRepositoryHasIssues checks if a repository has issues enabled +// checkRepositoryHasIssues checks if a repository has issues enabled (with caching) func checkRepositoryHasIssues(repo string) (bool, error) { + features, err := getRepositoryFeatures(repo) + if err != nil { + return false, err + } + return features.HasIssues, nil +} + +// checkRepositoryHasIssuesUncached checks if a repository has issues enabled (no caching) +func checkRepositoryHasIssuesUncached(repo string) (bool, error) { // Use GitHub REST API to check if issues are enabled // The has_issues field indicates if issues are enabled type RepositoryResponse struct { From 609ccf2f122107e69885b9abb653d5e880a1236e Mon Sep 17 00:00:00 2001 From: Don Syme Date: Sat, 25 Oct 2025 16:57:45 +0100 Subject: [PATCH 2/4] improve code --- pkg/workflow/validation.go | 86 ++++++++++++++++---------------------- 1 file changed, 36 insertions(+), 50 deletions(-) diff --git a/pkg/workflow/validation.go b/pkg/workflow/validation.go index 849f3b9078d..f9855bbc77c 100644 --- a/pkg/workflow/validation.go +++ b/pkg/workflow/validation.go @@ -25,24 +25,28 @@ type RepositoryFeatures struct { HasIssues bool } -// Global cache for repository features to amortize API calls across multiple workflow compilations +// Global cache for repository features and current repository info var ( - repositoryFeaturesCache = make(map[string]*RepositoryFeatures) - repositoryFeaturesMutex = sync.RWMutex{} - currentRepositoryCache = "" - currentRepositoryMutex = sync.RWMutex{} + repositoryFeaturesCache = sync.Map{} // sync.Map is thread-safe and efficient for read-heavy workloads + getCurrentRepositoryOnce sync.Once + currentRepositoryResult string + currentRepositoryError error ) // ClearRepositoryFeaturesCache clears the repository features cache // This is useful for testing or when repository settings might have changed func ClearRepositoryFeaturesCache() { - repositoryFeaturesMutex.Lock() - currentRepositoryMutex.Lock() - defer repositoryFeaturesMutex.Unlock() - defer currentRepositoryMutex.Unlock() + // Clear the features cache + repositoryFeaturesCache.Range(func(key, value any) bool { + repositoryFeaturesCache.Delete(key) + return true + }) + + // Reset the current repository cache + getCurrentRepositoryOnce = sync.Once{} + currentRepositoryResult = "" + currentRepositoryError = nil - repositoryFeaturesCache = make(map[string]*RepositoryFeatures) - currentRepositoryCache = "" validationLog.Print("Repository features and current repository caches cleared") } @@ -461,26 +465,20 @@ func (c *Compiler) validateRepositoryFeatures(workflowData *WorkflowData) error // getCurrentRepository gets the current repository from git context (with caching) func getCurrentRepository() (string, error) { - // Check cache first (read lock) - currentRepositoryMutex.RLock() - if currentRepositoryCache != "" { - repo := currentRepositoryCache - currentRepositoryMutex.RUnlock() - validationLog.Printf("Using cached current repository: %s", repo) - return repo, nil - } - currentRepositoryMutex.RUnlock() - - // Not in cache, acquire write lock and check again (double-checked locking) - currentRepositoryMutex.Lock() - defer currentRepositoryMutex.Unlock() + getCurrentRepositoryOnce.Do(func() { + currentRepositoryResult, currentRepositoryError = getCurrentRepositoryUncached() + }) - // Check cache again in case another goroutine populated it - if currentRepositoryCache != "" { - validationLog.Printf("Using cached current repository: %s", currentRepositoryCache) - return currentRepositoryCache, nil + if currentRepositoryError != nil { + return "", currentRepositoryError } + + validationLog.Printf("Using cached current repository: %s", currentRepositoryResult) + return currentRepositoryResult, nil +} +// getCurrentRepositoryUncached fetches the current repository from gh CLI (no caching) +func getCurrentRepositoryUncached() (string, error) { validationLog.Print("Fetching current repository from gh CLI") // Use gh CLI to get the current repository @@ -495,30 +493,15 @@ func getCurrentRepository() (string, error) { return "", fmt.Errorf("repository name is empty") } - // Cache the result - currentRepositoryCache = repo validationLog.Printf("Cached current repository: %s", repo) - return repo, nil } // getRepositoryFeatures gets repository features with caching to amortize API calls func getRepositoryFeatures(repo string) (*RepositoryFeatures, error) { - // Check cache first (read lock) - repositoryFeaturesMutex.RLock() - if features, exists := repositoryFeaturesCache[repo]; exists { - repositoryFeaturesMutex.RUnlock() - validationLog.Printf("Using cached repository features for: %s", repo) - return features, nil - } - repositoryFeaturesMutex.RUnlock() - - // Not in cache, acquire write lock and check again (double-checked locking) - repositoryFeaturesMutex.Lock() - defer repositoryFeaturesMutex.Unlock() - - // Check cache again in case another goroutine populated it - if features, exists := repositoryFeaturesCache[repo]; exists { + // Check cache first using sync.Map + if cached, exists := repositoryFeaturesCache.Load(repo); exists { + features := cached.(*RepositoryFeatures) validationLog.Printf("Using cached repository features for: %s", repo) return features, nil } @@ -542,11 +525,14 @@ func getRepositoryFeatures(repo string) (*RepositoryFeatures, error) { } features.HasIssues = hasIssues - // Cache the result - repositoryFeaturesCache[repo] = features - validationLog.Printf("Cached repository features for: %s (discussions: %v, issues: %v)", repo, hasDiscussions, hasIssues) + // Cache the result using sync.Map's LoadOrStore for atomic caching + // This handles the race condition where multiple goroutines might fetch the same repo + actual, _ := repositoryFeaturesCache.LoadOrStore(repo, features) + actualFeatures := actual.(*RepositoryFeatures) + + validationLog.Printf("Cached repository features for: %s (discussions: %v, issues: %v)", repo, actualFeatures.HasDiscussions, actualFeatures.HasIssues) - return features, nil + return actualFeatures, nil } // checkRepositoryHasDiscussions checks if a repository has discussions enabled (with caching) From 8b5eab50bc129325252de309dc684a524122e341 Mon Sep 17 00:00:00 2001 From: Don Syme Date: Sat, 25 Oct 2025 16:59:39 +0100 Subject: [PATCH 3/4] improve code --- pkg/workflow/compiler.go | 2 +- pkg/workflow/timing.go | 22 +++++++++---------- pkg/workflow/validation.go | 44 +++++++++++++++++++------------------- 3 files changed, 34 insertions(+), 34 deletions(-) diff --git a/pkg/workflow/compiler.go b/pkg/workflow/compiler.go index 4d76211032d..008ce42968a 100644 --- a/pkg/workflow/compiler.go +++ b/pkg/workflow/compiler.go @@ -225,7 +225,7 @@ type SafeOutputsConfig struct { func (c *Compiler) CompileWorkflow(markdownPath string) error { // Initialize timing tracker c.timingTracker = NewTimingTracker(c.verbose) - + // Parse the markdown file var workflowData *WorkflowData if err := c.timingTracker.TimeStep("Parse Workflow File", func() error { diff --git a/pkg/workflow/timing.go b/pkg/workflow/timing.go index 44275037fb8..24f31229c1d 100644 --- a/pkg/workflow/timing.go +++ b/pkg/workflow/timing.go @@ -21,12 +21,12 @@ type TimingTracker struct { // TimingStep represents a single timed step in the compilation process type TimingStep struct { - Name string - StartTime time.Time - EndTime time.Time - Duration time.Duration - SubSteps []TimingStep - currentSub *TimingStep + Name string + StartTime time.Time + EndTime time.Time + Duration time.Duration + SubSteps []TimingStep + currentSub *TimingStep } // NewTimingTracker creates a new timing tracker @@ -52,7 +52,7 @@ func (t *TimingTracker) StartStep(stepName string) { } timingLog.Printf("Starting step: %s", stepName) - + // End the previous step if there is one if t.currentStep != nil { t.EndStep() @@ -94,7 +94,7 @@ func (t *TimingTracker) EndSubStep() { t.currentStep.currentSub.EndTime = now t.currentStep.currentSub.Duration = now.Sub(t.currentStep.currentSub.StartTime) - timingLog.Printf("Completed sub-step: %s (took %v)", + timingLog.Printf("Completed sub-step: %s (took %v)", t.currentStep.currentSub.Name, t.currentStep.currentSub.Duration) // Add the completed sub-step to the current step @@ -151,14 +151,14 @@ func (t *TimingTracker) PrintSummary() { for i, step := range t.steps { percentage := float64(step.Duration) / float64(totalDuration) * 100 fmt.Fprintln(os.Stderr, console.FormatInfoMessage( - fmt.Sprintf("%d. %s: %v (%.1f%%)", + fmt.Sprintf("%d. %s: %v (%.1f%%)", i+1, step.Name, formatDuration(step.Duration), percentage))) // Print sub-steps if any for j, subStep := range step.SubSteps { subPercentage := float64(subStep.Duration) / float64(step.Duration) * 100 fmt.Fprintln(os.Stderr, console.FormatInfoMessage( - fmt.Sprintf(" %d.%d %s: %v (%.1f%% of step)", + fmt.Sprintf(" %d.%d %s: %v (%.1f%% of step)", i+1, j+1, subStep.Name, formatDuration(subStep.Duration), subPercentage))) } } @@ -198,4 +198,4 @@ func (t *TimingTracker) TimeSubStep(subStepName string, fn func() error) error { t.StartSubStep(subStepName) defer t.EndSubStep() return fn() -} \ No newline at end of file +} diff --git a/pkg/workflow/validation.go b/pkg/workflow/validation.go index f9855bbc77c..94f73fd8fef 100644 --- a/pkg/workflow/validation.go +++ b/pkg/workflow/validation.go @@ -27,7 +27,7 @@ type RepositoryFeatures struct { // Global cache for repository features and current repository info var ( - repositoryFeaturesCache = sync.Map{} // sync.Map is thread-safe and efficient for read-heavy workloads + repositoryFeaturesCache = sync.Map{} // sync.Map is thread-safe and efficient for read-heavy workloads getCurrentRepositoryOnce sync.Once currentRepositoryResult string currentRepositoryError error @@ -41,12 +41,12 @@ func ClearRepositoryFeaturesCache() { repositoryFeaturesCache.Delete(key) return true }) - + // Reset the current repository cache getCurrentRepositoryOnce = sync.Once{} currentRepositoryResult = "" currentRepositoryError = nil - + validationLog.Print("Repository features and current repository caches cleared") } @@ -361,7 +361,7 @@ func (c *Compiler) validateRepositoryFeatures(workflowData *WorkflowData) error if c.timingTracker != nil && c.timingTracker.verbose { c.timingTracker.StartSubStep("Get Repository Info") } - + var err error repo, err = getCurrentRepository() if err != nil { @@ -389,17 +389,17 @@ func (c *Compiler) validateRepositoryFeatures(workflowData *WorkflowData) error if needsDiscussions { var hasDiscussions bool var err error - + if c.timingTracker != nil && c.timingTracker.verbose { c.timingTracker.StartSubStep("Check Discussions API") } - + hasDiscussions, err = checkRepositoryHasDiscussions(repo) - + if c.timingTracker != nil && c.timingTracker.verbose { c.timingTracker.EndSubStep() } - + if err != nil { // If we can't check, log but don't fail // This could happen due to network issues or auth problems @@ -429,17 +429,17 @@ func (c *Compiler) validateRepositoryFeatures(workflowData *WorkflowData) error if workflowData.SafeOutputs.CreateIssues != nil { var hasIssues bool var err error - + if c.timingTracker != nil && c.timingTracker.verbose { c.timingTracker.StartSubStep("Check Issues API") } - + hasIssues, err = checkRepositoryHasIssues(repo) - + if c.timingTracker != nil && c.timingTracker.verbose { c.timingTracker.EndSubStep() } - + if err != nil { // If we can't check, log but don't fail validationLog.Printf("Warning: Could not check if issues are enabled: %v", err) @@ -468,11 +468,11 @@ func getCurrentRepository() (string, error) { getCurrentRepositoryOnce.Do(func() { currentRepositoryResult, currentRepositoryError = getCurrentRepositoryUncached() }) - + if currentRepositoryError != nil { return "", currentRepositoryError } - + validationLog.Printf("Using cached current repository: %s", currentRepositoryResult) return currentRepositoryResult, nil } @@ -480,7 +480,7 @@ func getCurrentRepository() (string, error) { // getCurrentRepositoryUncached fetches the current repository from gh CLI (no caching) func getCurrentRepositoryUncached() (string, error) { validationLog.Print("Fetching current repository from gh CLI") - + // Use gh CLI to get the current repository // This works when in a git repository with GitHub remote stdOut, _, err := gh.Exec("repo", "view", "--json", "nameWithOwner", "-q", ".nameWithOwner") @@ -507,31 +507,31 @@ func getRepositoryFeatures(repo string) (*RepositoryFeatures, error) { } validationLog.Printf("Fetching repository features from API for: %s", repo) - + // Fetch from API features := &RepositoryFeatures{} - + // Check discussions hasDiscussions, err := checkRepositoryHasDiscussionsUncached(repo) if err != nil { return nil, fmt.Errorf("failed to check discussions: %w", err) } features.HasDiscussions = hasDiscussions - - // Check issues + + // Check issues hasIssues, err := checkRepositoryHasIssuesUncached(repo) if err != nil { return nil, fmt.Errorf("failed to check issues: %w", err) } features.HasIssues = hasIssues - + // Cache the result using sync.Map's LoadOrStore for atomic caching // This handles the race condition where multiple goroutines might fetch the same repo actual, _ := repositoryFeaturesCache.LoadOrStore(repo, features) actualFeatures := actual.(*RepositoryFeatures) - + validationLog.Printf("Cached repository features for: %s (discussions: %v, issues: %v)", repo, actualFeatures.HasDiscussions, actualFeatures.HasIssues) - + return actualFeatures, nil } From f1404428ac993d2402572c97ae3d0a9eb0d1d4b9 Mon Sep 17 00:00:00 2001 From: Don Syme Date: Sat, 25 Oct 2025 17:10:41 +0100 Subject: [PATCH 4/4] improve code --- pkg/workflow/compiler.go | 139 +++++++++-------------- pkg/workflow/compiler_yaml.go | 20 +--- pkg/workflow/timing.go | 201 ---------------------------------- pkg/workflow/validation.go | 41 +------ 4 files changed, 60 insertions(+), 341 deletions(-) delete mode 100644 pkg/workflow/timing.go diff --git a/pkg/workflow/compiler.go b/pkg/workflow/compiler.go index 008ce42968a..2502ca5f0a9 100644 --- a/pkg/workflow/compiler.go +++ b/pkg/workflow/compiler.go @@ -62,7 +62,6 @@ type Compiler struct { fileTracker FileTracker // Optional file tracker for tracking created files warningCount int // Number of warnings encountered during compilation stepOrderTracker *StepOrderTracker // Tracks step ordering for validation - timingTracker *TimingTracker // Tracks compilation timing for performance analysis } // NewCompiler creates a new workflow compiler with optional configuration @@ -75,7 +74,6 @@ func NewCompiler(verbose bool, engineOverride string, version string) *Compiler jobManager: NewJobManager(), engineRegistry: GetGlobalEngineRegistry(), stepOrderTracker: NewStepOrderTracker(), - timingTracker: NewTimingTracker(verbose), } return c @@ -223,17 +221,10 @@ type SafeOutputsConfig struct { // CompileWorkflow converts a markdown workflow to GitHub Actions YAML func (c *Compiler) CompileWorkflow(markdownPath string) error { - // Initialize timing tracker - c.timingTracker = NewTimingTracker(c.verbose) - // Parse the markdown file - var workflowData *WorkflowData - if err := c.timingTracker.TimeStep("Parse Workflow File", func() error { - log.Printf("Parsing workflow file") - var err error - workflowData, err = c.ParseWorkflowFile(markdownPath) - return err - }); err != nil { + log.Printf("Parsing workflow file") + workflowData, err := c.ParseWorkflowFile(markdownPath) + if err != nil { // Check if this is already a formatted console error if strings.Contains(err.Error(), ":") && (strings.Contains(err.Error(), "error:") || strings.Contains(err.Error(), "warning:")) { // Already formatted, return as-is @@ -258,10 +249,6 @@ func (c *Compiler) CompileWorkflow(markdownPath string) error { // CompileWorkflowData compiles a workflow from already-parsed WorkflowData // This avoids re-parsing when the data has already been parsed func (c *Compiler) CompileWorkflowData(workflowData *WorkflowData, markdownPath string) error { - // Initialize a new timing tracker for this compilation - c.timingTracker = NewTimingTracker(c.verbose) - c.timingTracker.StartStep("Compilation Setup") - // Reset the step order tracker for this compilation c.stepOrderTracker = NewStepOrderTracker() @@ -269,13 +256,10 @@ func (c *Compiler) CompileWorkflowData(workflowData *WorkflowData, markdownPath lockFile := strings.TrimSuffix(markdownPath, ".md") + ".lock.yml" log.Printf("Starting compilation: %s -> %s", markdownPath, lockFile) - c.timingTracker.EndStep() // Validate expression safety - check that all GitHub Actions expressions are in the allowed list - if err := c.timingTracker.TimeStep("Expression Safety Validation", func() error { - log.Printf("Validating expression safety") - return validateExpressionSafety(workflowData.MarkdownContent) - }); err != nil { + log.Printf("Validating expression safety") + if err := validateExpressionSafety(workflowData.MarkdownContent); err != nil { formattedErr := console.FormatError(console.CompilerError{ Position: console.ErrorPosition{ File: markdownPath, @@ -296,12 +280,8 @@ func (c *Compiler) CompileWorkflowData(workflowData *WorkflowData, markdownPath // instead of using a shared action file // Generate the YAML content - var yamlContent string - if err := c.timingTracker.TimeStep("YAML Generation", func() error { - var err error - yamlContent, err = c.generateYAML(workflowData, markdownPath) - return err - }); err != nil { + yamlContent, err := c.generateYAML(workflowData, markdownPath) + if err != nil { formattedErr := console.FormatError(console.CompilerError{ Position: console.ErrorPosition{ File: markdownPath, @@ -316,12 +296,8 @@ func (c *Compiler) CompileWorkflowData(workflowData *WorkflowData, markdownPath // Validate against GitHub Actions schema (unless skipped) if !c.skipValidation { - c.timingTracker.StartStep("Validation") - - if err := c.timingTracker.TimeSubStep("GitHub Actions Schema", func() error { - log.Print("Validating workflow against GitHub Actions schema") - return c.validateGitHubActionsSchema(yamlContent) - }); err != nil { + log.Print("Validating workflow against GitHub Actions schema") + if err := c.validateGitHubActionsSchema(yamlContent); err != nil { formattedErr := console.FormatError(console.CompilerError{ Position: console.ErrorPosition{ File: markdownPath, @@ -340,10 +316,8 @@ func (c *Compiler) CompileWorkflowData(workflowData *WorkflowData, markdownPath } // Validate expression sizes - if err := c.timingTracker.TimeSubStep("Expression Sizes", func() error { - log.Print("Validating expression sizes") - return c.validateExpressionSizes(yamlContent) - }); err != nil { + log.Print("Validating expression sizes") + if err := c.validateExpressionSizes(yamlContent); err != nil { formattedErr := console.FormatError(console.CompilerError{ Position: console.ErrorPosition{ File: markdownPath, @@ -362,31 +336,26 @@ func (c *Compiler) CompileWorkflowData(workflowData *WorkflowData, markdownPath } // Validate container images used in MCP configurations - c.timingTracker.TimeSubStep("Container Images", func() error { - log.Print("Validating container images") - if err := c.validateContainerImages(workflowData); err != nil { - // Treat container image validation failures as warnings, not errors - // This is because validation may fail due to auth issues locally (e.g., private registries) - formattedWarning := console.FormatError(console.CompilerError{ - Position: console.ErrorPosition{ - File: markdownPath, - Line: 1, - Column: 1, - }, - Type: "warning", - Message: fmt.Sprintf("container image validation failed: %v", err), - }) - fmt.Fprintln(os.Stderr, formattedWarning) - c.IncrementWarningCount() - } - return nil // Don't fail on container image validation - }) + log.Print("Validating container images") + if err := c.validateContainerImages(workflowData); err != nil { + // Treat container image validation failures as warnings, not errors + // This is because validation may fail due to auth issues locally (e.g., private registries) + formattedWarning := console.FormatError(console.CompilerError{ + Position: console.ErrorPosition{ + File: markdownPath, + Line: 1, + Column: 1, + }, + Type: "warning", + Message: fmt.Sprintf("container image validation failed: %v", err), + }) + fmt.Fprintln(os.Stderr, formattedWarning) + c.IncrementWarningCount() + } // Validate runtime packages (npx, uv) - if err := c.timingTracker.TimeSubStep("Runtime Packages", func() error { - log.Print("Validating runtime packages") - return c.validateRuntimePackages(workflowData) - }); err != nil { + log.Print("Validating runtime packages") + if err := c.validateRuntimePackages(workflowData); err != nil { formattedErr := console.FormatError(console.CompilerError{ Position: console.ErrorPosition{ File: markdownPath, @@ -400,10 +369,8 @@ func (c *Compiler) CompileWorkflowData(workflowData *WorkflowData, markdownPath } // Validate repository features (discussions, issues) - if err := c.timingTracker.TimeSubStep("Repository Features", func() error { - log.Print("Validating repository features") - return c.validateRepositoryFeatures(workflowData) - }); err != nil { + log.Print("Validating repository features") + if err := c.validateRepositoryFeatures(workflowData); err != nil { formattedErr := console.FormatError(console.CompilerError{ Position: console.ErrorPosition{ File: markdownPath, @@ -415,8 +382,6 @@ func (c *Compiler) CompileWorkflowData(workflowData *WorkflowData, markdownPath }) return errors.New(formattedErr) } - - c.timingTracker.EndStep() } else if c.verbose { fmt.Println(console.FormatWarningMessage("Schema validation available but skipped (use SetSkipValidation(false) to enable)")) c.IncrementWarningCount() @@ -426,22 +391,8 @@ func (c *Compiler) CompileWorkflowData(workflowData *WorkflowData, markdownPath if c.noEmit { log.Print("Validation completed - no lock file generated (--no-emit enabled)") } else { - if err := c.timingTracker.TimeStep("File Output", func() error { - log.Printf("Writing output to: %s", lockFile) - if err := os.WriteFile(lockFile, []byte(yamlContent), 0644); err != nil { - return fmt.Errorf("failed to write lock file: %w", err) - } - - // Validate file size after writing - if lockFileInfo, err := os.Stat(lockFile); err == nil { - if lockFileInfo.Size() > MaxLockFileSize { - lockSize := pretty.FormatFileSize(lockFileInfo.Size()) - maxSize := pretty.FormatFileSize(MaxLockFileSize) - return fmt.Errorf("generated lock file size (%s) exceeds maximum allowed size (%s)", lockSize, maxSize) - } - } - return nil - }); err != nil { + log.Printf("Writing output to: %s", lockFile) + if err := os.WriteFile(lockFile, []byte(yamlContent), 0644); err != nil { formattedErr := console.FormatError(console.CompilerError{ Position: console.ErrorPosition{ File: lockFile, @@ -449,14 +400,30 @@ func (c *Compiler) CompileWorkflowData(workflowData *WorkflowData, markdownPath Column: 1, }, Type: "error", - Message: err.Error(), + Message: fmt.Sprintf("failed to write lock file: %v", err), }) return errors.New(formattedErr) } - } - // Print timing summary if verbose mode is enabled - c.timingTracker.PrintSummary() + // Validate file size after writing + if lockFileInfo, err := os.Stat(lockFile); err == nil { + if lockFileInfo.Size() > MaxLockFileSize { + lockSize := pretty.FormatFileSize(lockFileInfo.Size()) + maxSize := pretty.FormatFileSize(MaxLockFileSize) + err := fmt.Errorf("generated lock file size (%s) exceeds maximum allowed size (%s)", lockSize, maxSize) + formattedErr := console.FormatError(console.CompilerError{ + Position: console.ErrorPosition{ + File: lockFile, + Line: 1, + Column: 1, + }, + Type: "error", + Message: err.Error(), + }) + return errors.New(formattedErr) + } + } + } // Display success message with file size if we generated a lock file if c.noEmit { diff --git a/pkg/workflow/compiler_yaml.go b/pkg/workflow/compiler_yaml.go index b600eab0662..16f53fd2d40 100644 --- a/pkg/workflow/compiler_yaml.go +++ b/pkg/workflow/compiler_yaml.go @@ -9,22 +9,16 @@ import ( ) func (c *Compiler) generateYAML(data *WorkflowData, markdownPath string) (string, error) { - c.timingTracker.StartSubStep("Job Setup") // Reset job manager for this compilation c.jobManager = NewJobManager() - c.timingTracker.EndSubStep() // Build all jobs - if err := c.timingTracker.TimeSubStep("Build Jobs", func() error { - return c.buildJobs(data, markdownPath) - }); err != nil { + if err := c.buildJobs(data, markdownPath); err != nil { return "", fmt.Errorf("failed to build jobs: %w", err) } // Validate job dependencies - if err := c.timingTracker.TimeSubStep("Validate Job Dependencies", func() error { - return c.jobManager.ValidateDependencies() - }); err != nil { + if err := c.jobManager.ValidateDependencies(); err != nil { return "", fmt.Errorf("job dependency validation failed: %w", err) } @@ -119,20 +113,14 @@ func (c *Compiler) generateYAML(data *WorkflowData, markdownPath string) (string } // Generate jobs section using JobManager - c.timingTracker.TimeSubStep("Generate Jobs YAML", func() error { - yaml.WriteString(c.jobManager.RenderToYAML()) - return nil - }) + yaml.WriteString(c.jobManager.RenderToYAML()) yamlContent := yaml.String() // If we're in non-cloning trial mode and this workflow has issue triggers, // replace github.event.issue.number with inputs.issue_number if c.trialMode && c.hasIssueTrigger(data.On) { - c.timingTracker.TimeSubStep("Trial Mode Processing", func() error { - yamlContent = c.replaceIssueNumberReferences(yamlContent) - return nil - }) + yamlContent = c.replaceIssueNumberReferences(yamlContent) } return yamlContent, nil diff --git a/pkg/workflow/timing.go b/pkg/workflow/timing.go deleted file mode 100644 index 24f31229c1d..00000000000 --- a/pkg/workflow/timing.go +++ /dev/null @@ -1,201 +0,0 @@ -package workflow - -import ( - "fmt" - "os" - "time" - - "github.com/githubnext/gh-aw/pkg/console" - "github.com/githubnext/gh-aw/pkg/logger" -) - -var timingLog = logger.New("workflow:timing") - -// TimingTracker tracks the duration of compilation steps for performance analysis -type TimingTracker struct { - verbose bool - startTime time.Time - steps []TimingStep - currentStep *TimingStep -} - -// TimingStep represents a single timed step in the compilation process -type TimingStep struct { - Name string - StartTime time.Time - EndTime time.Time - Duration time.Duration - SubSteps []TimingStep - currentSub *TimingStep -} - -// NewTimingTracker creates a new timing tracker -func NewTimingTracker(verbose bool) *TimingTracker { - return &TimingTracker{ - verbose: verbose, - startTime: time.Now(), - steps: make([]TimingStep, 0), - } -} - -// StartStep starts timing a new compilation step -func (t *TimingTracker) StartStep(stepName string) { - if !t.verbose { - return - } - - now := time.Now() - step := TimingStep{ - Name: stepName, - StartTime: now, - SubSteps: make([]TimingStep, 0), - } - - timingLog.Printf("Starting step: %s", stepName) - - // End the previous step if there is one - if t.currentStep != nil { - t.EndStep() - } - - t.currentStep = &step -} - -// StartSubStep starts timing a sub-step within the current step -func (t *TimingTracker) StartSubStep(subStepName string) { - if !t.verbose || t.currentStep == nil { - return - } - - now := time.Now() - subStep := TimingStep{ - Name: subStepName, - StartTime: now, - SubSteps: make([]TimingStep, 0), - } - - timingLog.Printf("Starting sub-step: %s", subStepName) - - // End the previous sub-step if there is one - if t.currentStep.currentSub != nil { - t.EndSubStep() - } - - t.currentStep.currentSub = &subStep -} - -// EndSubStep ends timing the current sub-step -func (t *TimingTracker) EndSubStep() { - if !t.verbose || t.currentStep == nil || t.currentStep.currentSub == nil { - return - } - - now := time.Now() - t.currentStep.currentSub.EndTime = now - t.currentStep.currentSub.Duration = now.Sub(t.currentStep.currentSub.StartTime) - - timingLog.Printf("Completed sub-step: %s (took %v)", - t.currentStep.currentSub.Name, t.currentStep.currentSub.Duration) - - // Add the completed sub-step to the current step - t.currentStep.SubSteps = append(t.currentStep.SubSteps, *t.currentStep.currentSub) - t.currentStep.currentSub = nil -} - -// EndStep ends timing the current step -func (t *TimingTracker) EndStep() { - if !t.verbose || t.currentStep == nil { - return - } - - now := time.Now() - - // End any pending sub-step - if t.currentStep.currentSub != nil { - t.EndSubStep() - } - - t.currentStep.EndTime = now - t.currentStep.Duration = now.Sub(t.currentStep.StartTime) - - timingLog.Printf("Completed step: %s (took %v)", t.currentStep.Name, t.currentStep.Duration) - - // Add the completed step to the list - t.steps = append(t.steps, *t.currentStep) - t.currentStep = nil -} - -// PrintSummary displays a summary of all timing information -func (t *TimingTracker) PrintSummary() { - if !t.verbose { - return - } - - // End any pending step - if t.currentStep != nil { - t.EndStep() - } - - totalDuration := time.Since(t.startTime) - - fmt.Fprintln(os.Stderr, console.FormatInfoMessage("")) - fmt.Fprintln(os.Stderr, console.FormatInfoMessage("=== Compilation Timing Summary ===")) - fmt.Fprintln(os.Stderr, console.FormatInfoMessage(fmt.Sprintf("Total compilation time: %v", formatDuration(totalDuration)))) - fmt.Fprintln(os.Stderr, console.FormatInfoMessage("")) - - if len(t.steps) == 0 { - fmt.Fprintln(os.Stderr, console.FormatInfoMessage("No timing steps recorded")) - return - } - - for i, step := range t.steps { - percentage := float64(step.Duration) / float64(totalDuration) * 100 - fmt.Fprintln(os.Stderr, console.FormatInfoMessage( - fmt.Sprintf("%d. %s: %v (%.1f%%)", - i+1, step.Name, formatDuration(step.Duration), percentage))) - - // Print sub-steps if any - for j, subStep := range step.SubSteps { - subPercentage := float64(subStep.Duration) / float64(step.Duration) * 100 - fmt.Fprintln(os.Stderr, console.FormatInfoMessage( - fmt.Sprintf(" %d.%d %s: %v (%.1f%% of step)", - i+1, j+1, subStep.Name, formatDuration(subStep.Duration), subPercentage))) - } - } - - fmt.Fprintln(os.Stderr, console.FormatInfoMessage("")) -} - -// GetTotalDuration returns the total time elapsed since the tracker was created -func (t *TimingTracker) GetTotalDuration() time.Duration { - return time.Since(t.startTime) -} - -// formatDuration formats a duration for human-readable display -func formatDuration(d time.Duration) string { - if d < time.Millisecond { - return fmt.Sprintf("%dμs", d.Microseconds()) - } else if d < time.Second { - return fmt.Sprintf("%.1fms", float64(d.Nanoseconds())/1_000_000) - } else if d < time.Minute { - return fmt.Sprintf("%.2fs", d.Seconds()) - } else { - minutes := int(d.Minutes()) - seconds := d.Seconds() - float64(minutes*60) - return fmt.Sprintf("%dm %.1fs", minutes, seconds) - } -} - -// TimeStep is a helper function to measure and log the duration of a function call -func (t *TimingTracker) TimeStep(stepName string, fn func() error) error { - t.StartStep(stepName) - defer t.EndStep() - return fn() -} - -// TimeSubStep is a helper function to measure and log the duration of a sub-step function call -func (t *TimingTracker) TimeSubStep(subStepName string, fn func() error) error { - t.StartSubStep(subStepName) - defer t.EndSubStep() - return fn() -} diff --git a/pkg/workflow/validation.go b/pkg/workflow/validation.go index 94f73fd8fef..fc24a3c8282 100644 --- a/pkg/workflow/validation.go +++ b/pkg/workflow/validation.go @@ -357,27 +357,14 @@ func (c *Compiler) validateRepositoryFeatures(workflowData *WorkflowData) error // Get the repository from the current git context // This will work when running in a git repository - var repo string - if c.timingTracker != nil && c.timingTracker.verbose { - c.timingTracker.StartSubStep("Get Repository Info") - } - - var err error - repo, err = getCurrentRepository() + repo, err := getCurrentRepository() if err != nil { validationLog.Printf("Could not determine repository: %v", err) // Don't fail if we can't determine the repository (e.g., not in a git repo) // This allows validation to pass in non-git environments - if c.timingTracker != nil && c.timingTracker.verbose { - c.timingTracker.EndSubStep() - } return nil } - if c.timingTracker != nil && c.timingTracker.verbose { - c.timingTracker.EndSubStep() - } - validationLog.Printf("Checking repository features for: %s", repo) // Check if discussions are enabled when create-discussion or add-comment with discussion: true is configured @@ -387,18 +374,7 @@ func (c *Compiler) validateRepositoryFeatures(workflowData *WorkflowData) error *workflowData.SafeOutputs.AddComments.Discussion) if needsDiscussions { - var hasDiscussions bool - var err error - - if c.timingTracker != nil && c.timingTracker.verbose { - c.timingTracker.StartSubStep("Check Discussions API") - } - - hasDiscussions, err = checkRepositoryHasDiscussions(repo) - - if c.timingTracker != nil && c.timingTracker.verbose { - c.timingTracker.EndSubStep() - } + hasDiscussions, err := checkRepositoryHasDiscussions(repo) if err != nil { // If we can't check, log but don't fail @@ -427,18 +403,7 @@ func (c *Compiler) validateRepositoryFeatures(workflowData *WorkflowData) error // Check if issues are enabled when create-issue is configured if workflowData.SafeOutputs.CreateIssues != nil { - var hasIssues bool - var err error - - if c.timingTracker != nil && c.timingTracker.verbose { - c.timingTracker.StartSubStep("Check Issues API") - } - - hasIssues, err = checkRepositoryHasIssues(repo) - - if c.timingTracker != nil && c.timingTracker.verbose { - c.timingTracker.EndSubStep() - } + hasIssues, err := checkRepositoryHasIssues(repo) if err != nil { // If we can't check, log but don't fail