From 8c8391be4ea07ce5a732eb11f68f0085ea6a46cb Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 22 Oct 2025 08:30:00 +0000 Subject: [PATCH 1/2] Initial plan From d2d42fb6d42659d1127398b90ba5287281848651 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 22 Oct 2025 08:50:27 +0000 Subject: [PATCH 2/2] Rename logger variables to "log" and update workflow instructions Changed all logger variable names from descriptive names (trialLog, claudeEngineLog, etc.) to simply "log" as requested in feedback. Since multiple files in pkg/workflow and pkg/cli packages need loggers, converted to function-scoped loggers to avoid package-level variable conflicts: - Function-scoped: log := logger.New("pkg:filename") - Used in pkg/workflow and pkg/cli packages with multiple files Updated go-logger.md workflow instructions to: - Require logger variable always be named "log" - Explain when to use function-scoped vs package-level loggers - Document the package-level variable conflict issue - Update examples and quality checklist All changes verified with successful build and unit tests. Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- .github/workflows/go-logger.lock.yml | 77 +++++++++++++++++++++++----- .github/workflows/go-logger.md | 77 +++++++++++++++++++++++----- pkg/cli/trial_command.go | 15 +++--- pkg/workflow/claude_engine.go | 11 ++-- pkg/workflow/expressions.go | 13 +++-- pkg/workflow/js.go | 10 ++-- pkg/workflow/mcp-config.go | 8 +-- 7 files changed, 155 insertions(+), 56 deletions(-) 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,