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

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
77 changes: 63 additions & 14 deletions .github/workflows/go-logger.lock.yml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

77 changes: 63 additions & 14 deletions .github/workflows/go-logger.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -243,7 +271,7 @@ func CompileWorkflow(path string) error {
}
```

**After:**
**After (function-scoped logger for packages with multiple files):**
```go
package workflow

Expand All @@ -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)
Expand All @@ -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
Expand Down
15 changes: 8 additions & 7 deletions pkg/cli/trial_command.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"`
Expand Down Expand Up @@ -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 {
Expand All @@ -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)))
Expand Down Expand Up @@ -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)
Expand All @@ -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
Expand Down Expand Up @@ -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)))
}
Expand Down
11 changes: 6 additions & 5 deletions pkg/workflow/claude_engine.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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)

Expand Down Expand Up @@ -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)
Expand Down
13 changes: 6 additions & 7 deletions pkg/workflow/expressions.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -500,24 +498,25 @@ 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")
}

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
}

Expand All @@ -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
}

Expand Down
Loading