Markdown renderer initialization uses stale configuration and passes large struct by value

[osterman]

Markdown Renderer Initialization Uses Stale Configuration

Problem

The markdown renderer is initialized once in cmd/root.go:Execute() with a configuration that doesn't include CLI flag overrides. This causes two issues:

1. Inefficient: utils.InitializeMarkdown() currently accepts AtmosConfiguration by value, creating expensive copies of a large struct (violates Go best practices and likely triggers linting warnings)
2. Stale config: Commands that run later use a different configuration (tmpConfig in PersistentPreRun) that respects CLI flags like --base-path, --config, --config-path, but the markdown renderer still has the old config

Current Code Flow

// cmd/root.go:Execute()
func Execute() error {
    // Load config without CLI flags
    atmosConfig, initErr = cfg.InitCliConfig(schema.ConfigAndStacksInfo{}, false)
    
    // Initialize markdown renderers (happens once, early)
    utils.InitializeMarkdown(atmosConfig)        // ❌ Pass by value (expensive copy)
    errUtils.InitializeMarkdown(&atmosConfig)    // ✅ Pass by pointer
    
    // ... later commands execute
}

// cmd/root.go:PersistentPreRun
PersistentPreRun: func(cmd *cobra.Command, args []string) {
    // Load config WITH CLI flags (different from above!)
    configAndStacksInfo := schema.ConfigAndStacksInfo{
        AtmosBasePath:          bp,   // from --base-path
        AtmosConfigFilesFromArg: cfgFiles,  // from --config
        AtmosConfigDirsFromArg:  cfgDirs,   // from --config-path
    }
    tmpConfig, err := cfg.InitCliConfig(configAndStacksInfo, false)
    
    // ❌ Markdown renderer still uses old atmosConfig, not tmpConfig!
}

Impact

• Markdown renderer doesn't respect CLI flag overrides for width, color profiles, etc.
• Passing AtmosConfiguration by value creates unnecessary copies of a large struct
• Could cause subtle bugs where markdown rendering behaves differently than expected

Proposed Solution

Implement two-stage initialization with proper pointer usage:

Stage 1: Early Initialization (for early errors)

// cmd/root.go:Execute()
atmosConfig, initErr = cfg.InitCliConfig(schema.ConfigAndStacksInfo{}, false)

// Initialize for early errors (before PersistentPreRun)
utils.InitializeMarkdown(&atmosConfig)      // ✅ Pass by pointer
errUtils.InitializeMarkdown(&atmosConfig)

Stage 2: Re-initialize with CLI-aware Config

// cmd/root.go:PersistentPreRun
tmpConfig, err := cfg.InitCliConfig(configAndStacksInfo, false)

// Re-initialize with CLI-aware config
utils.InitializeMarkdown(&tmpConfig)        // ✅ Pass by pointer
errUtils.InitializeMarkdown(&tmpConfig)

Implementation Changes Required

1. Update utils.InitializeMarkdown signature:

   // pkg/utils/markdown_utils.go
   func InitializeMarkdown(atmosConfig *schema.AtmosConfiguration) {
       defer perf.Track(atmosConfig, "utils.InitializeMarkdown")()
   
       var err error
       render, err = markdown.NewTerminalMarkdownRenderer(atmosConfig)
       // ...
   }
   

2. Update markdown.NewTerminalMarkdownRenderer signature:

   // pkg/ui/markdown/renderer.go
   func NewTerminalMarkdownRenderer(atmosConfig *schema.AtmosConfiguration) (*Renderer, error) {
       return NewRenderer(atmosConfig, ...)
   }
   

3. Update markdown.NewRenderer signature:

   // pkg/ui/markdown/renderer.go
   func NewRenderer(atmosConfig *schema.AtmosConfiguration, opts ...Option) (*Renderer, error) {
       r := &Renderer{
           atmosConfig: atmosConfig,  // Store pointer directly
           // ...
       }
   }
   

4. Make initialization idempotent: Allow InitializeMarkdown to be called multiple times safely (already works, just document it)

5. Update all test calls from InitializeMarkdown(config) to InitializeMarkdown(&config)

Benefits

• ✅ Efficient: No expensive struct copies, passes large struct by pointer
• ✅ Correct config: Markdown renderer uses CLI-flag-aware configuration
• ✅ Early errors: Still get markdown rendering for early errors (stage 1)
• ✅ Consistent: Both utils and errUtils use pointer semantics
• ✅ Compliant: Follows Go best practices for passing large structs

Testing

Verify that:

1. Early errors (before PersistentPreRun) still render with markdown
2. Commands respect --base-path, --config, --config-path flags in markdown output
3. Markdown width/color settings from config are applied correctly
4. All existing tests pass
5. No linting warnings about passing large structs by value

Related

• This issue was discovered during PR #1599 (error handling implementation)
• Temporary fix: Reverted to pass-by-value in commit d8b236047 to unblock CI
• Long-term fix tracked here