add --output-multiple-files flag and automatic cleanup of orphaned generated files

[crhntr]

Summary

This PR refactors the internal package structure and adds two related features for better generated file management:

1. Single-file output by default — All routes now generate into one template_routes.go file. Use --output-multiple-files to split routes into separate *_template_routes_gen.go files per template source.

2. Automatic orphan cleanup — When template files are renamed, deleted, or you switch between single/multi-file modes, muxt now automatically removes stale generated files.

Changes

New Features

• Add --output-multiple-files flag to opt into per-template-file generation
• Automatically delete orphaned generated files matching the current --output-routes-func
• Document the cleanup heuristic in code comments

Refactoring

• Split internal/muxt into focused packages: analysis, generate, muxt, asteval, cli
• Rename Template → Definition with proper encapsulation via getter methods
• Move package loading to asteval.LoadPackages()
• Separate config types for generate, check, and documentation commands

Chores

• Add chk and doc command aliases
• Bump counterfeiter to v6.12.1
• Bump typelate/check to v0.0.4

Cleanup Behavior

Generated files are identified by:

1. Presence of // Code generated by muxt generate comment
2. Matching --output-routes-func value

This allows multiple route sets (with different function names) to coexist. Changing --output-routes-func will not delete files from the previous function name.

[claude]

Code Review: PR #49 - Add --output-multiple-files flag and automatic cleanup

Overview

This is a well-structured PR that introduces two important features: single-file output by default with a multi-file option, and automatic cleanup of orphaned generated files. The refactoring into focused packages (analysis, generate, muxt, asteval, cli) follows good separation of concerns.

✅ Strengths

Code Quality

• Excellent documentation: The cleanup heuristic is thoroughly documented in comments (commands.go:87-104), explaining the behavior clearly
• Well-organized refactoring: Package structure split is logical and improves maintainability
• Backward compatibility: Deprecated flags are maintained with clear deprecation warnings
• Comprehensive test coverage: New reference tests cover all cleanup scenarios (orphaned files, mode switching, different routes functions)

Architecture

• Single responsibility: Each new package has a clear purpose (analysis, generate, cli)
• Good encapsulation: Definition type uses getter methods instead of exported fields
• Flag organization: Clear prefixing (use-*, output-*) makes CLI more intuitive

Feature Implementation

• Smart cleanup logic: Files are identified by both generation comment and routes function name, allowing multiple route sets to coexist
• Safe defaults: Single-file mode is the default, reducing file clutter for most users
• Clear migration path: Documentation and tests cover switching between modes

⚠️ Issues & Recommendations

Critical

1. Potential File Deletion Risk (commands.go:148-187)

func findGeneratedFiles(workingDirectory, routesFunction string) (map[string]bool, error) {
    // ...
    content, err := os.ReadFile(filePath)
    if err != nil {
        continue  // ⚠️ Silently ignores read errors
    }

Issue: Read errors are silently ignored, which could lead to failing to identify generated files for cleanup.

Recommendation: Log read errors or return them. If a file can't be read due to permissions, users should know.

if err != nil {
    return nil, fmt.Errorf("failed to read %s: %w", filePath, err)
}

2. Incomplete Template Reference Tracking (check.go:106-115)

for changed {
    changed = false
    for name := range usedTemplates {
        t := ts.Lookup(name)
        if t == nil || t.Tree == nil {
            continue
        }
        // ⚠️ Loop body is empty - references aren't being collected
    }
}

Issue: The transitive closure logic for finding template references via {{template "name"}} has an empty loop body. This means templates referenced only from other templates will be incorrectly reported as unused.

Recommendation: This appears to be incomplete code. The logic should collect references:

for name := range usedTemplates {
    t := ts.Lookup(name)
    if t == nil || t.Tree == nil {
        continue
    }
    refs := collectTemplateReferences(t.Tree.Root)
    for _, ref := range refs {
        if allNames[ref] && !usedTemplates[ref] {
            usedTemplates[ref] = true
            changed = true
        }
    }
}

Note: collectTemplateReferences was removed in this PR, which suggests this functionality may have been intentionally changed, but it's unclear if the current behavior is correct.

Major

3. Race Condition in Multi-File Generation (commands.go:114-122)

While not a classic race condition, there's a potential issue if generation fails partway through:

for _, file := range files {
    // ...
    if err := os.WriteFile(file.Path, sb.Bytes(), 0o644); err != nil {
        return err  // ⚠️ Partial files written, cleanup not performed
    }
    newGeneratedFiles[file.Path] = true
}

Recommendation: Consider using atomic file operations or cleanup on error:

// Track successfully written files for rollback
var writtenFiles []string
defer func() {
    if err != nil {
        // Rollback on error
        for _, path := range writtenFiles {
            os.Remove(path)
        }
    }
}()

4. Missing Validation in parseRoutesFunctionFromComment (commands.go:189-217)

The function doesn't validate that the extracted routes function name is a valid identifier:

if strings.HasPrefix(part, "--output-routes-func=") {
    return strings.TrimPrefix(part, "--output-routes-func=")  // No validation
}

Recommendation: Validate extracted values:

routesFunc := strings.TrimPrefix(part, "--output-routes-func=")
if !token.IsIdentifier(routesFunc) {
    return defaultRoutesFunctionName
}
return routesFunc

Minor

5. Inconsistent Error Handling

In findGeneratedFiles, directory read errors are returned but file read errors are ignored (line 164). This inconsistency could mask issues.

6. Performance: O(n²) File Scanning

For projects with many Go files, findGeneratedFiles reads every .go file's content. Consider:

• Early termination if first line doesn't match
• Caching results
• Only reading the first ~3 lines instead of entire file

7. Documentation Gap

The PR body mentions that changing --output-routes-func won't delete old files, but this could be more prominent in the CLI docs. Consider adding a warning in the help text.

🔒 Security

No security concerns identified. The file operations use appropriate permissions (0o644) and the cleanup logic is conservative (requires both generation comment and matching routes function name).

⚡ Performance

Minor considerations:

1. findGeneratedFiles reads all .go files in the directory - acceptable for typical project sizes
2. String splitting on every line in file content parsing - could be optimized to only check first line

🧪 Test Coverage

Excellent test coverage with 6 new reference tests covering:

• Cleanup of orphaned files ✅
• Switch between single/multi-file modes ✅
• Different routes function coexistence ✅
• Routes function changes ✅

Consider adding edge case tests for:

• Very long file names
• Special characters in template names
• Concurrent generation (if supported)

📝 Code Style

Follows project conventions well:

• Consistent with existing code style
• Good use of constants for magic values
• Clear variable naming

Minor: Some functions are getting long (e.g., generateCommand at 65 lines, methodHandlerFunc at 125 lines). Consider extracting helper functions for readability.

🎯 Recommendations Summary

Must Fix:

1. Fix empty loop body in findUnusedTemplates (check.go:106-115)
2. Handle read errors in findGeneratedFiles (commands.go:164)

Should Fix:
3. Add validation to parseRoutesFunctionFromComment
4. Improve error handling consistency
5. Consider atomic file operations for multi-file generation

Nice to Have:
6. Optimize file scanning performance
7. Add edge case tests
8. Extract helper functions from long functions

Conclusion

This is a solid PR that adds valuable functionality with good design choices. The automatic cleanup feature is well-thought-out and the package refactoring improves code organization. The main concerns are the incomplete template reference tracking logic and error handling robustness. Once those are addressed, this will be ready to merge.

Overall Assessment: ✅ Approve with requested changes

Great work on maintaining backward compatibility and providing excellent documentation! 🎉

[Copilot]

Pull request overview

This PR performs a significant refactoring of the internal package structure and adds two major features:

Purpose:

• Reorganizes internal/muxt into focused packages (analysis, generate, muxt, asteval, cli)
• Implements single-file output mode as the default behavior
• Adds --output-multiple-files flag for per-template-file generation
• Implements automatic cleanup of orphaned generated files

Key Changes:

• Renamed Template → Definition with encapsulated fields accessed via getter methods
• Moved package loading logic to asteval.LoadPackages()
• Extracted CLI command handling to dedicated cli package
• Added intelligent file cleanup based on generation comment markers and routes function names
• Separated configuration types for different commands (generate, check, documentation)

Reviewed changes

Copilot reviewed 30 out of 31 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
routes.go	Package renamed to generate, refactored to support single/multi-file modes, extracted callHandleFunc function
template_route_path.go	Updated to use muxt.Definition with getter methods
template_data.go	Relocated constants to package scope, updated references to use muxt package
definition.go	Renamed Template → Definition, added getter methods, moved to muxt package
definition_name.go	Updated to use Definition, exported FileNameToPrivateIdentifier
file.go	Package renamed to generate, refactored to use asteval package functions
documentation.go	Moved to analysis package, refactored with separate configuration type
check.go	Moved to analysis package, enhanced template execution tracking
commands.go	New CLI package with command handlers and automatic file cleanup logic
parse.go, package.go, forrest.go	New asteval package files for AST evaluation utilities
*.txt reference files	Test scripts for validating multi-file generation and cleanup behavior
go.mod, go.sum	Dependency updates for counterfeiter and typelate/check

Comments suppressed due to low confidence (3)

routes.go:125

• The comment contains a typo: "parseBasedmuxt.Definitions" should be "parseBasedDefinitions". Remove the extra "muxt." prefix.
  definition.go:104
• SetArgumentType method receives Definition by value instead of by pointer, so it modifies a copy and the changes are never persisted. This should be func (def *Definition) SetArgumentType(name string, tp types.Type) to properly update the pathValueTypes map.
  template_route_path.go:68
• The left and right operand of this operation are identical.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[claude]

Pull Request Review

Summary

This is a substantial PR that introduces two major features (single-file output by default and automatic orphan cleanup) along with significant internal refactoring. Overall, the changes are well-structured and the implementation is solid, but there are a few areas that could benefit from additional attention.

---

🟢 Strengths

1. Excellent Documentation

• The cleanup heuristic is thoroughly documented in internal/cli/commands.go:87-104
• Clear explanation of edge cases
• Good inline comments throughout the code

2. Comprehensive Test Coverage

• Five new test files added for cleanup scenarios covering critical edge cases
• Tests validate orphan cleanup, mode switching, and routes function changes

3. Good Package Organization

• Refactoring into focused packages improves maintainability
• Clear separation of concerns between generation and analysis logic
• Renamed Template → Definition with proper encapsulation

4. Backward Compatibility

• Deprecated flags are properly maintained with helpful warnings
• Existing workflows won't break unexpectedly

---

🟡 Areas for Improvement

1. Cleanup Logic - Race Condition Potential (internal/cli/commands.go:106-132)

Risk: If the process crashes between writing new files and cleanup, stale files remain.

Recommendation: Consider using a more atomic approach with temporary names or add recovery logic.

Severity: Low (edge case, but could cause confusion in CI/CD environments)

2. Filename Validation Logic (internal/generate/routes.go:124)

Issue: Character-based validation is somewhat fragile.

Recommendation: Document exact rules for valid file paths and add test cases with unusual but valid filenames.

3. Error Handling - Silent Failures (internal/cli/commands.go:163-164)

Issue: File read errors during cleanup discovery are silently ignored.

Recommendation: Log warnings for unexpected errors to help debug issues.

---

🔴 Potential Bugs

1. Missing Validation on parseRoutesFunctionFromComment (internal/cli/commands.go:189-217)

Risk: Malformed generation comments could cause incorrect file deletions.

Recommendation: Return (string, error) to distinguish no flag found vs malformed flag.

---

🔒 Security Considerations

✅ No Critical Issues Found

• Appropriate file permissions
• No arbitrary file deletion
• No command injection vectors
• Proper input validation

---

Summary & Recommendation

Overall Assessment: Approve with Minor Changes ⭐

Suggested Action Items Before Merge:

• Add error logging for file read failures during cleanup
• Add test for malformed generation comments
• Improve parseRoutesFunctionFromComment error handling

Post-Merge Nice to Have:

• Add integration tests for filesystem errors
• Consider splitting large routes.go file
• Add migration guide to documentation

---

Great work on this PR! The automatic cleanup feature is a significant quality-of-life improvement.

[claude]

Pull Request Review: Single-file output with automatic cleanup

Summary

This PR introduces significant refactoring with two key features: (1) Single-file output by default, and (2) Automatic orphan cleanup. Overall, this is well-designed and thoughtfully implemented.

---

✅ Strengths

Code Quality & Architecture:

• Excellent package restructuring into focused packages (analysis, generate, muxt, asteval, cli)
• Clear encapsulation with Definition type using getter methods
• Well-documented cleanup heuristic (lines 88-105 in internal/cli/commands.go)
• Comprehensive test coverage with 5 new test scenarios

Implementation Highlights:

• Smart file detection in findGeneratedFiles (lines 154-194)
• Defensive grouping logic with safeguards for non-file-path sources
• Proper error handling with rollback logic on write failures

---

⚠️ Issues & Concerns

1. Potential Race Condition in File Cleanup
Location: internal/cli/commands.go:130-138

• Cleanup happens after new files are written
• If interrupted, orphaned files will remain
• Recommendation: Document this limitation

2. Silent Failure on Non-File-Path Sources
Location: internal/generate/routes.go:118-131

• Templates with special chars are silently reclassified
• Recommendation: Add warning logs

3. Incomplete Input Validation
Location: internal/cli/commands.go:199-227

• parseRoutesFunctionFromComment doesn't validate identifier
• Recommendation: Add token.IsIdentifier() check

4. Minor Performance Concern

• bytes.NewBuffer(nil) could cause reallocations for large templates
• File scanning is synchronous (could be slow for large packages)

---

🔒 Security Review

✅ No security concerns identified

• File operations use 0o644 permissions
• No command injection risks
• Proper path handling with filepath.Join
• No unsafe file path traversal

---

🧪 Test Coverage

Excellent coverage with new tests for:

• Cleanup of orphaned files
• Mode switching (single/multi-file)
• Multiple routes functions
• Backward compatibility

Suggestions:

1. Add test for interrupted cleanup
2. Test non-file-path source handling
3. Test rollback behavior

---

📝 Documentation

Well documented overall. Needs improvement:

• Flag descriptions could mention cleanup behavior
• RoutesFileConfiguration needs field docs
• Definition getters could use doc comments

---

🎬 Final Recommendation

APPROVE with minor suggestions

High-quality PR solving a real problem with excellent test coverage and backwards compatibility.

Suggested improvements (non-blocking):

1. Add validation for routes function name (3 min)
2. Add warning logs for reclassified templates (5 min)
3. Add doc comments to RoutesFileConfiguration (5 min)

Great work! 🎉

The changes demonstrate solid Go best practices, careful edge case consideration, and will improve maintainability.

[claude]

Code Review: PR #49 - Add --output-multiple-files flag and automatic cleanup

Summary

This PR introduces significant improvements to muxt's code generation with two major features:

1. Single-file output by default with opt-in multi-file generation via --output-multiple-files
2. Automatic cleanup of orphaned generated files

Overall, this is a well-designed and carefully implemented PR that significantly improves the developer experience. The code quality is high with excellent documentation.

---

✅ Strengths

Architecture & Design

• Excellent refactoring: The split of internal/muxt into focused packages (analysis, generate, muxt, asteval, cli) improves maintainability and separation of concerns
• Smart cleanup heuristic: The orphan file cleanup is well-documented and handles edge cases appropriately (lines 88-105 in internal/cli/commands.go)
• Backward compatibility: Deprecated flags are maintained, preventing breaking changes for existing users
• Clear encapsulation: Renaming Template → Definition with getter methods provides better API boundaries

Code Quality

• Comprehensive documentation: The cleanup behavior is thoroughly documented with examples and edge cases
• Strong test coverage: Added 5 new test scenarios covering cleanup edge cases (switch modes, orphaned files, routes function changes)
• Conservative approach: The cleanup logic only removes files matching the current routes function, allowing multiple route sets to coexist

Implementation Details

• Proper error handling: The cleanup logic handles errors appropriately (line 134-137 in commands.go)
• File grouping logic: Clean separation between file-based and parse-based templates (lines 114-132 in routes.go)
• Redirect analysis: Static analysis to determine which templates can redirect is thorough and handles circular references

---

🔍 Potential Issues & Suggestions

1. File Cleanup Race Condition (Low Risk)

Location: internal/cli/commands.go:107-138

The cleanup happens after new files are written. If the process crashes between writing files and cleanup, orphaned files remain.

Suggestion: Consider writing to temp files first, then atomic rename after cleanup:

// Write to temp files first
for _, file := range files {
    tempPath := file.Path + ".tmp"
    // write to tempPath
}
// Clean up old files
// Rename temp files to final names

Impact: Low - Only affects crash scenarios, and reruns will clean up correctly.

2. Filename Validation Could Be More Robust

Location: internal/generate/routes.go:123-131

The check for valid file paths uses strings.ContainsAny(baseName, " /\\()") which might miss other invalid characters.

Suggestion: Use a more explicit validation:

if !filepath.IsAbs(sourceFile) && filepath.Clean(sourceFile) == sourceFile {
    // Valid relative file path
}

Impact: Low - Edge case handling.

3. parseRoutesFunctionFromComment Could Be More Defensive

Location: internal/cli/commands.go:199-227

The function doesn't handle malformed comments gracefully. If someone manually edits the generated comment, it could fail silently.

Suggestion: Add logging for malformed comments:

generatedRoutesFunc := parseRoutesFunctionFromComment(firstLine)
if generatedRoutesFunc == "" {
    log.Printf("WARNING: Could not parse routes function from: %s", firstLine)
    continue
}

Current code already does this at line 188 - Good job! ✅

4. Memory Efficiency for Large Projects

Location: internal/cli/commands.go:154-196

The findGeneratedFiles function reads entire file contents for each .go file to check the first line.

Suggestion: For better performance with many files:

file, err := os.Open(filePath)
if err != nil {
    return nil, err
}
defer file.Close()

scanner := bufio.NewScanner(file)
if scanner.Scan() {
    firstLine := scanner.Text()
    // check firstLine
}

Impact: Low - Only noticeable in very large projects with 100+ Go files.

5. Test Coverage for Edge Cases

The test files added are comprehensive, but consider adding:

• Test for concurrent generation (if supported)
• Test for permission errors during cleanup
• Test for very long file paths near OS limits

---

🔒 Security Considerations

✅ No security issues identified

• File operations use proper error checking
• No command injection risks
• No unvalidated user input in file operations
• Generated code maintains existing security properties

---

🎯 Performance Considerations

• File I/O optimization: Current implementation reads all .go files during cleanup. For large projects (100+ files), consider caching or limiting scope.
• AST generation: No performance regressions observed in the refactoring.
• Memory: Definition grouping uses maps appropriately.

Overall Performance Impact: Minimal to none.

---

📋 Best Practices Compliance

✅ Follows Go best practices:

• Proper error wrapping with fmt.Errorf
• Idiomatic Go code structure
• Clear variable naming
• Appropriate use of interfaces and encapsulation

✅ Follows project conventions:

• Consistent with existing code style
• Maintains backward compatibility
• Comprehensive test coverage

---

🧪 Test Coverage Assessment

Excellent test coverage with 5 new txtar-based test scenarios:

1. reference_cleanup_different_routes_func.txt - Multiple route functions coexisting
2. reference_cleanup_orphaned_files.txt - Template file renaming
3. reference_cleanup_routes_func_change.txt - Route function name changes
4. reference_cleanup_switch_to_multiple_files.txt - Mode switching to multi-file
5. reference_cleanup_switch_to_single_file.txt - Mode switching to single-file

These tests cover the critical paths for the cleanup functionality.

Minor gap: No explicit test for cleanup failure scenarios (permission errors, disk full).

---

📝 Documentation Quality

Excellent documentation:

• Inline comments explain complex logic
• CLI documentation updated (docs/reference/cli.md)
• Clear explanation of cleanup heuristic
• Examples show expected behavior

---

🎉 Recommendation

APPROVE with minor suggestions

This is a high-quality PR that:

• Solves a real pain point (manual cleanup of generated files)
• Maintains backward compatibility
• Has excellent test coverage
• Is well-documented
• Follows Go and project best practices

The suggestions above are minor optimizations and edge case handling that don't block merging.

Great work! 🚀

---

Optional Follow-ups (Post-Merge)

1. Monitor for reports of cleanup issues in the wild
2. Consider adding a --dry-run flag to preview cleanup actions
3. Add metrics/logging for cleanup operations in verbose mode
4. Document the cleanup behavior in a migration guide for users upgrading from previous versions