diff --git a/.github/workflows/go-logger.lock.yml b/.github/workflows/go-logger.lock.yml index 540efc9b1d..0841c7cef4 100644 --- a/.github/workflows/go-logger.lock.yml +++ b/.github/workflows/go-logger.lock.yml @@ -1095,17 +1095,32 @@ jobs: ### Logger Declaration - If a file doesn't have a logger, add this at the top of the file (after imports): + **IMPORTANT: Logger variable MUST always be named `log`** + + The logger variable naming follows these rules: + + 1. **Always use the name `log`** - Never use descriptive names like `trialLog`, `claudeEngineLog`, etc. + 2. **For packages with multiple files needing loggers**, use function-scoped loggers to avoid conflicts: + ```go + func MyFunction() { + log := logger.New("pkg:filename") + log.Printf("Function started") + // ... function body ... + } + ``` - ```go - import "github.com/githubnext/gh-aw/pkg/logger" + 3. **For packages where only one file needs a logger**, use package-level declaration: + ```go + import "github.com/githubnext/gh-aw/pkg/logger" + + var log = logger.New("pkg:filename") + ``` - var log = logger.New("pkg:filename") - ``` + **Why function-scoped loggers?** In Go, package-level variables are shared across all files in the package. If multiple files in the same package (like `pkg/workflow`) declare `var log`, it creates a naming conflict. Function-scoped loggers avoid this issue. - Replace `pkg:filename` with the actual package and filename: + **Logger namespace naming:** Replace `pkg:filename` with the actual package and filename: - For `pkg/workflow/compiler.go` → `"workflow:compiler"` - - For `pkg/cli/compile.go` → `"cli:compile"` + - For `pkg/cli/compile.go` → `"cli:compile"` - For `pkg/parser/frontmatter.go` → `"parser:frontmatter"` ### Logger Usage Patterns @@ -1195,9 +1210,22 @@ jobs: For each file: - 1. **Add logger declaration if missing:** - - Add import: `"github.com/githubnext/gh-aw/pkg/logger"` - - Add logger variable using correct naming: `var log = logger.New("pkg:filename")` + 1. **Add logger declaration:** + - **For packages with many files** (like `pkg/workflow`, `pkg/cli`): Use function-scoped logger + ```go + func MyFunction() { + log := logger.New("pkg:filename") + log.Printf("Function started") + // ... rest of function ... + } + ``` + - **For single-file packages**: Use package-level logger + ```go + import "github.com/githubnext/gh-aw/pkg/logger" + + var log = logger.New("pkg:filename") + ``` + - **IMPORTANT**: Always name the variable `log`, never use descriptive names like `trialLog` or `claudeEngineLog` 2. **Add meaningful logging calls:** - Add logging at function entry for important functions @@ -1266,7 +1294,7 @@ jobs: } ``` - **After:** + **After (function-scoped logger for packages with multiple files):** ```go package workflow @@ -1277,9 +1305,8 @@ jobs: "github.com/githubnext/gh-aw/pkg/logger" ) - var log = logger.New("workflow:compiler") - func CompileWorkflow(path string) error { + log := logger.New("workflow:compiler") log.Printf("Compiling workflow: %s", path) data, err := os.ReadFile(path) @@ -1297,13 +1324,35 @@ jobs: } ``` + **Alternative (package-level logger for single-file packages):** + ```go + package parser + + import ( + "fmt" + "os" + + "github.com/githubnext/gh-aw/pkg/logger" + ) + + var log = logger.New("parser:frontmatter") + + func ParseFrontmatter(path string) error { + log.Printf("Parsing frontmatter: %s", path) + // ... function body ... + } + ``` + ## Quality Checklist Before creating the PR, verify: - [ ] Maximum 5 files modified - [ ] No test files modified (`*_test.go`) - - [ ] Each file has logger declaration with correct naming convention + - [ ] Logger variable is always named `log` (not descriptive names) + - [ ] For packages with multiple files: use function-scoped `log := logger.New(...)` + - [ ] For single-file packages: use package-level `var log = logger.New(...)` + - [ ] Logger namespace follows `pkg:filename` convention - [ ] Logger arguments don't compute anything or cause side effects - [ ] Logging messages are meaningful and helpful - [ ] No duplicate logging with existing logs diff --git a/.github/workflows/go-logger.md b/.github/workflows/go-logger.md index 07d9fbe868..fd40348e68 100644 --- a/.github/workflows/go-logger.md +++ b/.github/workflows/go-logger.md @@ -72,17 +72,32 @@ Add meaningful debug logging calls to Go files in the `pkg/` directory following ### Logger Declaration -If a file doesn't have a logger, add this at the top of the file (after imports): +**IMPORTANT: Logger variable MUST always be named `log`** + +The logger variable naming follows these rules: + +1. **Always use the name `log`** - Never use descriptive names like `trialLog`, `claudeEngineLog`, etc. +2. **For packages with multiple files needing loggers**, use function-scoped loggers to avoid conflicts: + ```go + func MyFunction() { + log := logger.New("pkg:filename") + log.Printf("Function started") + // ... function body ... + } + ``` -```go -import "github.com/githubnext/gh-aw/pkg/logger" +3. **For packages where only one file needs a logger**, use package-level declaration: + ```go + import "github.com/githubnext/gh-aw/pkg/logger" + + var log = logger.New("pkg:filename") + ``` -var log = logger.New("pkg:filename") -``` +**Why function-scoped loggers?** In Go, package-level variables are shared across all files in the package. If multiple files in the same package (like `pkg/workflow`) declare `var log`, it creates a naming conflict. Function-scoped loggers avoid this issue. -Replace `pkg:filename` with the actual package and filename: +**Logger namespace naming:** Replace `pkg:filename` with the actual package and filename: - For `pkg/workflow/compiler.go` → `"workflow:compiler"` -- For `pkg/cli/compile.go` → `"cli:compile"` +- For `pkg/cli/compile.go` → `"cli:compile"` - For `pkg/parser/frontmatter.go` → `"parser:frontmatter"` ### Logger Usage Patterns @@ -172,9 +187,22 @@ For each selected file: For each file: -1. **Add logger declaration if missing:** - - Add import: `"github.com/githubnext/gh-aw/pkg/logger"` - - Add logger variable using correct naming: `var log = logger.New("pkg:filename")` +1. **Add logger declaration:** + - **For packages with many files** (like `pkg/workflow`, `pkg/cli`): Use function-scoped logger + ```go + func MyFunction() { + log := logger.New("pkg:filename") + log.Printf("Function started") + // ... rest of function ... + } + ``` + - **For single-file packages**: Use package-level logger + ```go + import "github.com/githubnext/gh-aw/pkg/logger" + + var log = logger.New("pkg:filename") + ``` + - **IMPORTANT**: Always name the variable `log`, never use descriptive names like `trialLog` or `claudeEngineLog` 2. **Add meaningful logging calls:** - Add logging at function entry for important functions @@ -243,7 +271,7 @@ func CompileWorkflow(path string) error { } ``` -**After:** +**After (function-scoped logger for packages with multiple files):** ```go package workflow @@ -254,9 +282,8 @@ import ( "github.com/githubnext/gh-aw/pkg/logger" ) -var log = logger.New("workflow:compiler") - func CompileWorkflow(path string) error { + log := logger.New("workflow:compiler") log.Printf("Compiling workflow: %s", path) data, err := os.ReadFile(path) @@ -274,13 +301,35 @@ func CompileWorkflow(path string) error { } ``` +**Alternative (package-level logger for single-file packages):** +```go +package parser + +import ( + "fmt" + "os" + + "github.com/githubnext/gh-aw/pkg/logger" +) + +var log = logger.New("parser:frontmatter") + +func ParseFrontmatter(path string) error { + log.Printf("Parsing frontmatter: %s", path) + // ... function body ... +} +``` + ## Quality Checklist Before creating the PR, verify: - [ ] Maximum 5 files modified - [ ] No test files modified (`*_test.go`) -- [ ] Each file has logger declaration with correct naming convention +- [ ] Logger variable is always named `log` (not descriptive names) +- [ ] For packages with multiple files: use function-scoped `log := logger.New(...)` +- [ ] For single-file packages: use package-level `var log = logger.New(...)` +- [ ] Logger namespace follows `pkg:filename` convention - [ ] Logger arguments don't compute anything or cause side effects - [ ] Logging messages are meaningful and helpful - [ ] No duplicate logging with existing logs diff --git a/pkg/cli/trial_command.go b/pkg/cli/trial_command.go index a9455bacbc..9fd347dab4 100644 --- a/pkg/cli/trial_command.go +++ b/pkg/cli/trial_command.go @@ -18,8 +18,6 @@ import ( "github.com/spf13/cobra" ) -var trialLog = logger.New("cli:trial_command") - // WorkflowTrialResult represents the result of running a single workflow trial type WorkflowTrialResult struct { WorkflowName string `json:"workflow_name"` @@ -154,7 +152,8 @@ Trial results are saved both locally (in trials/ directory) and in the host repo // RunWorkflowTrials executes the main logic for trialing one or more workflows func RunWorkflowTrials(workflowSpecs []string, logicalRepoSpec string, cloneRepoSpec string, hostRepoSpec string, deleteHostRepo, forceDeleteHostRepo, quiet bool, timeoutMinutes int, triggerContext string, repeatCount int, autoMergePRs bool, engineOverride string, appendText string, verbose bool) error { - trialLog.Printf("Starting workflow trials: workflow_count=%d, timeout_minutes=%d", len(workflowSpecs), timeoutMinutes) + log := logger.New("cli:trial_command") + log.Printf("Starting workflow trials: workflow_count=%d, timeout_minutes=%d", len(workflowSpecs), timeoutMinutes) // Parse all workflow specifications var parsedSpecs []*WorkflowSpec for _, spec := range workflowSpecs { @@ -164,7 +163,7 @@ func RunWorkflowTrials(workflowSpecs []string, logicalRepoSpec string, cloneRepo } parsedSpecs = append(parsedSpecs, parsedSpec) } - trialLog.Printf("Parsed %d workflow specifications", len(parsedSpecs)) + log.Printf("Parsed %d workflow specifications", len(parsedSpecs)) if len(parsedSpecs) == 1 { fmt.Fprintln(os.Stderr, console.FormatInfoMessage(fmt.Sprintf("Starting trial of workflow '%s' from '%s'", parsedSpecs[0].WorkflowName, parsedSpecs[0].RepoSlug))) @@ -576,7 +575,8 @@ func showTrialConfirmation(parsedSpecs []*WorkflowSpec, logicalRepoSlug, cloneRe // For clone-repo mode, reusing an existing host repository is not allowed // If forceDeleteHostRepo is true, deletes the repository if it exists before creating it func ensureTrialRepository(repoSlug string, cloneRepoSlug string, forceDeleteHostRepo bool, verbose bool) error { - trialLog.Printf("Ensuring trial repository: repo=%s, force_delete=%v", repoSlug, forceDeleteHostRepo) + log := logger.New("cli:trial_command") + log.Printf("Ensuring trial repository: repo=%s, force_delete=%v", repoSlug, forceDeleteHostRepo) parts := strings.Split(repoSlug, "/") if len(parts) != 2 { return fmt.Errorf("invalid repository slug format: %s (expected user/repo)", repoSlug) @@ -585,7 +585,7 @@ func ensureTrialRepository(repoSlug string, cloneRepoSlug string, forceDeleteHos // Check if repository already exists cmd := exec.Command("gh", "repo", "view", repoSlug) if err := cmd.Run(); err == nil { - trialLog.Printf("Trial repository already exists: %s", repoSlug) + log.Printf("Trial repository already exists: %s", repoSlug) // Repository exists - determine what to do if forceDeleteHostRepo { // Force delete mode: delete the existing repository first @@ -880,7 +880,8 @@ func addGitHubTokenSecret(repoSlug string, tracker *TrialSecretTracker, verbose } func triggerWorkflowRun(repoSlug, workflowName string, triggerContext string, verbose bool) (string, error) { - trialLog.Printf("Triggering workflow run: repo=%s, workflow=%s", repoSlug, workflowName) + log := logger.New("cli:trial_command") + log.Printf("Triggering workflow run: repo=%s, workflow=%s", repoSlug, workflowName) if verbose { fmt.Fprintln(os.Stderr, console.FormatInfoMessage(fmt.Sprintf("Triggering workflow run for: %s", workflowName))) } diff --git a/pkg/workflow/claude_engine.go b/pkg/workflow/claude_engine.go index d4dba8e2cc..73c379162f 100644 --- a/pkg/workflow/claude_engine.go +++ b/pkg/workflow/claude_engine.go @@ -12,8 +12,6 @@ import ( "github.com/githubnext/gh-aw/pkg/logger" ) -var claudeEngineLog = logger.New("workflow:claude_engine") - // ClaudeEngine represents the Claude Code agentic engine type ClaudeEngine struct { BaseEngine @@ -36,7 +34,8 @@ func NewClaudeEngine() *ClaudeEngine { } func (e *ClaudeEngine) GetInstallationSteps(workflowData *WorkflowData) []GitHubActionStep { - claudeEngineLog.Printf("Generating installation steps for Claude engine: workflow=%s", workflowData.Name) + log := logger.New("workflow:claude_engine") + log.Printf("Generating installation steps for Claude engine: workflow=%s", workflowData.Name) var steps []GitHubActionStep // Add secret validation step @@ -89,7 +88,8 @@ func (e *ClaudeEngine) GetVersionCommand() string { // GetExecutionSteps returns the GitHub Actions steps for executing Claude func (e *ClaudeEngine) GetExecutionSteps(workflowData *WorkflowData, logFile string) []GitHubActionStep { - claudeEngineLog.Printf("Generating execution steps for Claude: workflow=%s, logfile=%s", workflowData.Name, logFile) + log := logger.New("workflow:claude_engine") + log.Printf("Generating execution steps for Claude: workflow=%s, logfile=%s", workflowData.Name, logFile) // Handle custom steps if they exist in engine config steps := InjectCustomEngineSteps(workflowData, e.convertStepToYAML) @@ -375,7 +375,8 @@ func (e *ClaudeEngine) expandNeutralToolsToClaudeTools(tools map[string]any) map // 3. adds default Claude tools and git commands based on safe outputs configuration // 4. generates the allowed tools string for Claude func (e *ClaudeEngine) computeAllowedClaudeToolsString(tools map[string]any, safeOutputs *SafeOutputsConfig, cacheMemoryConfig *CacheMemoryConfig) string { - claudeEngineLog.Printf("Computing allowed Claude tools: tool_count=%d", len(tools)) + log := logger.New("workflow:claude_engine") + log.Printf("Computing allowed Claude tools: tool_count=%d", len(tools)) // Initialize tools map if nil if tools == nil { tools = make(map[string]any) diff --git a/pkg/workflow/expressions.go b/pkg/workflow/expressions.go index 584e574fc1..804e076a48 100644 --- a/pkg/workflow/expressions.go +++ b/pkg/workflow/expressions.go @@ -9,8 +9,6 @@ import ( "github.com/githubnext/gh-aw/pkg/logger" ) -var expressionsLog = logger.New("workflow:expressions") - // ConditionNode represents a node in a condition expression tree type ConditionNode interface { Render() string @@ -500,7 +498,8 @@ const ( // Supports && (AND), || (OR), ! (NOT), and parentheses for grouping // Example: "condition1 && (condition2 || !condition3)" func ParseExpression(expression string) (ConditionNode, error) { - expressionsLog.Printf("Parsing expression: length=%d", len(expression)) + log := logger.New("workflow:expressions") + log.Printf("Parsing expression: length=%d", len(expression)) if strings.TrimSpace(expression) == "" { return nil, fmt.Errorf("empty expression") } @@ -508,16 +507,16 @@ func ParseExpression(expression string) (ConditionNode, error) { parser := &ExpressionParser{} tokens, err := parser.tokenize(expression) if err != nil { - expressionsLog.Printf("Failed to tokenize expression: %s", err) + log.Printf("Failed to tokenize expression: %s", err) return nil, err } - expressionsLog.Printf("Tokenized expression into %d tokens", len(tokens)) + log.Printf("Tokenized expression into %d tokens", len(tokens)) parser.tokens = tokens parser.pos = 0 result, err := parser.parseOrExpression() if err != nil { - expressionsLog.Printf("Failed to parse expression: %s", err) + log.Printf("Failed to parse expression: %s", err) return nil, err } @@ -526,7 +525,7 @@ func ParseExpression(expression string) (ConditionNode, error) { return nil, fmt.Errorf("unexpected token '%s' at position %d", parser.current().value, parser.current().pos) } - expressionsLog.Print("Successfully parsed expression into condition tree") + log.Print("Successfully parsed expression into condition tree") return result, nil } diff --git a/pkg/workflow/js.go b/pkg/workflow/js.go index eb88dd3042..a7a01ad696 100644 --- a/pkg/workflow/js.go +++ b/pkg/workflow/js.go @@ -8,8 +8,6 @@ import ( "github.com/githubnext/gh-aw/pkg/logger" ) -var jsLog = logger.New("workflow:js") - //go:embed js/create_pull_request.cjs var createPullRequestScript string @@ -413,12 +411,13 @@ func isDigit(r rune) bool { // FormatJavaScriptForYAML formats a JavaScript script with proper indentation for embedding in YAML func FormatJavaScriptForYAML(script string) []string { - jsLog.Printf("Formatting JavaScript for YAML: script_length=%d", len(script)) + log := logger.New("workflow:js") + log.Printf("Formatting JavaScript for YAML: script_length=%d", len(script)) var formattedLines []string // Remove JavaScript comments first cleanScript := removeJavaScriptComments(script) - jsLog.Printf("Removed JavaScript comments: clean_length=%d", len(cleanScript)) + log.Printf("Removed JavaScript comments: clean_length=%d", len(cleanScript)) scriptLines := strings.Split(cleanScript, "\n") for _, line := range scriptLines { @@ -432,7 +431,8 @@ func FormatJavaScriptForYAML(script string) []string { // WriteJavaScriptToYAML writes a JavaScript script with proper indentation to a strings.Builder func WriteJavaScriptToYAML(yaml *strings.Builder, script string) { - jsLog.Printf("Writing JavaScript to YAML builder: script_length=%d", len(script)) + log := logger.New("workflow:js") + log.Printf("Writing JavaScript to YAML builder: script_length=%d", len(script)) // Remove JavaScript comments first cleanScript := removeJavaScriptComments(script) diff --git a/pkg/workflow/mcp-config.go b/pkg/workflow/mcp-config.go index e72d373d5c..6fa0892629 100644 --- a/pkg/workflow/mcp-config.go +++ b/pkg/workflow/mcp-config.go @@ -10,13 +10,12 @@ import ( "github.com/githubnext/gh-aw/pkg/parser" ) -var mcpConfigLog = logger.New("workflow:mcp-config") - // renderPlaywrightMCPConfig generates the Playwright MCP server configuration // Uses npx to launch Playwright MCP instead of Docker for better performance and simplicity // This is a shared function used by both Claude and Custom engines func renderPlaywrightMCPConfig(yaml *strings.Builder, playwrightTool any, isLast bool) { - mcpConfigLog.Print("Rendering Playwright MCP configuration") + log := logger.New("workflow:mcp-config") + log.Print("Rendering Playwright MCP configuration") renderPlaywrightMCPConfigWithOptions(yaml, playwrightTool, isLast, false, false) } @@ -717,7 +716,8 @@ func collectHTTPMCPHeaderSecrets(tools map[string]any) map[string]string { // getMCPConfig extracts MCP configuration from a tool config and returns a structured MCPServerConfig func getMCPConfig(toolConfig map[string]any, toolName string) (*parser.MCPServerConfig, error) { - mcpConfigLog.Printf("Extracting MCP configuration for tool: %s", toolName) + log := logger.New("workflow:mcp-config") + log.Printf("Extracting MCP configuration for tool: %s", toolName) config := MapToolConfig(toolConfig) result := &parser.MCPServerConfig{ Name: toolName,