add template list commands

[crhntr]

No description provided.

[claude]

Pull Request Review: Add Report Commands

Summary

This PR adds powerful new analysis commands to muxt: list-template-callers and list-template-calls. It also refactors the existing documentation command to be the default behavior and improves the overall CLI structure. The changes are well-structured and align with the project's philosophy of compile-time analysis.

✅ Strengths

1. Excellent Code Organization

• The refactoring from documentation.go → routes.go and extraction of shared logic is clean
• New analysis functions follow consistent patterns (NewTemplateCallers, NewTemplateCalls, NewTemplateSource)
• Good separation of concerns with dedicated configuration types

2. Strong Type Safety

• All new code leverages Go's type system effectively
• Proper use of go/types and AST analysis
• Type-safe template rendering with well-defined data structures

3. Template-Based Output

• Using embed.FS and text templates for output formatting is elegant
• Templates are clean and maintainable (*.txt.template files)
• Consistent output format across commands

4. Good Helper Abstractions

• asteval.LoadTemplates consolidates repetitive package/template loading (internal/asteval/package.go:56)
• matchesAny provides reusable filtering logic (internal/analysis/analysis.go:37)

🔍 Issues & Concerns

1. Dead/Commented Code (Minor)

Location: internal/cli/commands.go:53-54, internal/astgen/pkg.go:3-28

The read-template-source command is commented out and the entire astgen/pkg.go file contains only commented code. This should either be:

• Removed if not needed
• Uncommented if intended for this PR
• Moved to a feature branch if work-in-progress

Recommendation: Remove commented code or explain why it's being kept.

2. Inconsistent Error Handling (Minor)

Location: internal/analysis/template_calls.go:50, internal/analysis/template_callers.go:61

_ = check.Execute(global, t.Tree, dataType)

Errors from check.Execute are silently discarded. While this may be intentional for analysis purposes, it should be documented why errors are ignored.

Recommendation: Add a comment explaining why errors are acceptable here, or log them if verbose mode is enabled.

3. Unused Field (Minor)

Location: internal/analysis/analysis.go:20-34

The typeFormatter struct and its qualifier method are defined but never used in this PR.

Recommendation: Remove if unused, or document its future purpose.

4. Missing Validation (Minor)

Location: internal/cli/commands.go:566-576, internal/cli/commands.go:579-603

The newTemplateSourceConfiguration, newListTemplateCallersConfiguration, and newListTemplateCallsConfiguration functions don't validate that TemplatesVariable is a valid identifier (unlike other similar functions).

While they default to defaultTemplatesVariableName, if a user explicitly provides an invalid identifier, it won't be caught until runtime.

Recommendation: Add the same validation as in newListRoutesConfiguration:

if g.TemplatesVariable != "" && !token.IsIdentifier(g.TemplatesVariable) {
    return analysis.TemplateCallersConfiguration{}, fmt.Errorf(useTemplatesVariable + errIdentSuffix)
}

5. Breaking Change in Default Behavior (Medium)

Location: internal/cli/commands.go:41-42

if len(args) == 0 {
    return listRoutes(wd, nil, stdout, stderr)
}

Running muxt with no arguments now executes listRoutes, which requires --use-templates-variable to be specified (internal/cli/commands.go:558). This changes the UX significantly.

Previous behavior: Show help
New behavior: Attempt to list routes (may fail with "no template variable specified")

Recommendation: Either:

• Document this breaking change
• Make listRoutes show help when required flags are missing
• Keep showing help by default and require explicit command for listing routes

6. Documentation Gap (Minor)

The new commands are well-documented in help.txt, but there's no explanation of:

• What "template callers" vs "template calls" means (the distinction is subtle)
• Example use cases for these commands

Recommendation: Add brief descriptions in the help text or examples showing when to use each command.

7. Potential Nil Dereference (Low Risk)

Location: internal/analysis/template_source.go:22-23

if t.Tree != nil && t.Tree.Root != nil {
    source = t.Tree.Root.String()
}

While well-protected, similar code in template_callers.go:59-61 and template_calls.go:48-50 only checks t != nil && t.Tree != nil before calling check.Execute. This is likely safe but inconsistent.

Recommendation: Maintain consistent nil checks across all template tree operations.

🎯 Best Practices Alignment

✅ Follows Project Standards

• Deterministic output (sorted maps with slices.Sorted)
• TDD-friendly structure
• Consistent with existing code patterns
• Clear separation between CLI, analysis, and AST evaluation

✅ Good Go Practices

• Proper package structure
• Clear function names
• Exported types are well-documented
• Uses standard library effectively

🧪 Testing Considerations

Missing Test Coverage

The PR removes test files (howto_use_documentation_command.txt, reference_documentation_output.txt) but I don't see new test files for:

• list-template-callers command
• list-template-calls command
• Default behavior (running muxt with no args)

Recommendation: Add txtar test files following the project's naming convention:

• reference_list_template_callers.txt
• reference_list_template_calls.txt
• reference_default_list_routes.txt

Edge Cases to Test

1. What happens when filtering templates that don't exist?
2. Circular template references
3. Templates that call themselves
4. Empty template sets

🔒 Security Considerations

No security issues identified. The code:

• Doesn't execute user-provided code
• Doesn't handle sensitive data
• Uses safe string operations
• Properly validates file paths

⚡ Performance Considerations

Generally efficient, but one note:

Location: internal/analysis/template_callers.go:43-64

For each ExecuteTemplate call found in the Go code, the function analyzes the entire template tree. In a large codebase with many templates, this could be slow.

Recommendation: Consider caching analyzed templates if performance becomes an issue.

📝 Documentation & Help Text

The help text updates in internal/cli/help.txt are excellent:

• Clear command descriptions
• Good examples
• Proper formatting

Minor suggestion: Add a "SEE ALSO" section to clarify the relationship between commands.

Final Verdict

This is high-quality code that adds valuable functionality to muxt. The implementation is clean, follows project conventions, and demonstrates good Go practices.

Required Before Merge

1. Remove or uncomment dead code (read-template-source command, astgen/pkg.go)
2. Add test coverage for new commands
3. Clarify default behavior change (running muxt with no args)

Recommended Before Merge

1. Add input validation for template variable identifiers
2. Document why check.Execute errors are ignored
3. Remove unused typeFormatter struct
4. Add examples to help distinguish "callers" vs "calls"

Nice to Have

1. Performance testing with large template sets
2. More detailed error messages when templates aren't found
3. Consistent nil checks across template tree operations

Great work! This feature will make debugging template dependencies much easier. 🎉

---

Review conducted using go/types analysis, AST inspection, and project convention verification

[claude]

Pull Request Review: Add Report Commands

This PR adds powerful new analysis commands to muxt while refactoring the documentation command. Overall, this is a solid enhancement that improves introspection capabilities. Below are my findings:

---

✅ Strengths

1. Clean Separation of Concerns

The refactoring splits the monolithic Documentation function into focused, single-responsibility functions:

• NewRoutes: Lists template routes and functions
• NewTemplateCallers: Shows where templates are called from
• NewTemplateCalls: Shows what templates call other templates

This follows the Single Responsibility Principle and makes each function easier to test and maintain.

2. Improved CLI UX

• Default behavior: Running muxt now lists routes (great for discoverability)
• Better help messages: Unknown commands now show help text with an error message
• Command aliases: Short forms like callers and calls improve usability
• Filtering support: --template flag allows focused analysis

3. Template-Based Output

Using Go templates for output formatting (*.txt.template files) is excellent:

• Makes output format changes easier
• Separates presentation from logic
• Consistent formatting across commands

4. Reusable Helper Functions

• asteval.LoadTemplates: Consolidates common template loading logic
• asteval.FindType: Extracts type lookup into reusable function
• matchesAny: Simple but useful filtering utility

---

🐛 Issues Found

1. Dead Code in astgen/pkg.go (internal/astgen/pkg.go:1-28)

The entire file consists of commented-out code with no functionality:

//type TypeFormatter struct {
//	outputPkgPath string
//	imports       map[string]string
//}
// ... more commented code

Recommendation: Either complete the implementation or remove this file entirely. Dead code adds confusion.

2. Inconsistent Whitespace in Template (docs/prompts/muxt-complete.md:1)

The diff shows an unnecessary blank line added at the top:

+
 # Muxt Advanced Reference

Recommendation: Remove this unintentional whitespace change.

3. Missing Test Coverage

No test files were added for the new commands. The grep search shows no tests for:

• muxt list-template-callers
• muxt list-template-calls
• muxt (default command behavior)

Recommendation: Add txtar test files following the existing pattern:

• cmd/muxt/testdata/reference_list_template_callers.txt
• cmd/muxt/testdata/reference_list_template_calls.txt
• cmd/muxt/testdata/reference_default_command.txt

4. Potentially Confusing Command Names

list-template-callers vs list-template-calls are easy to confuse:

• callers: "what calls this template?" (reverse lookup)
• calls: "what does this template call?" (forward lookup)

Recommendation: Consider more descriptive names or ensure documentation clearly explains the difference.

5. Template Reference Output Ambiguity (internal/analysis/analysis.txt.template:12-14)

{{define "template reference" -}}
	{{.Position.String}} {{.Kind}} {{if ne (filepathBase .Position.Filename) .Name}}{{printf "%q" .Name}}{{end}}
{{- end}}

The conditional logic that optionally includes the name is hard to understand. The condition ne (filepathBase .Position.Filename) .Name checks if the filename differs from the template name, but this may not be intuitive to users.

Recommendation: Add a comment explaining when/why the name is omitted, or always show it for clarity.

6. Unused typeFormatter Struct (internal/analysis/analysis.go:20-34)

The typeFormatter struct and its qualifier method are defined but never used in this PR:

type typeFormatter struct {
	outputPkgPath string
	imports       map[string]string
}

func (tf *typeFormatter) qualifier(pkg *types.Package) string {
	// ...
}

Recommendation: Remove if not needed, or clarify its intended future use in a comment.

---

🔒 Security Considerations

✅ No security issues identified. The changes:

• Don't expose sensitive information
• Use standard library packages appropriately
• Handle user input (filters) safely via string matching

---

⚡ Performance Considerations

Minor Concern: Repeated Package Loading

Each command calls asteval.LoadPackages independently. For users running multiple commands sequentially, this could be optimized. However, this is likely not a bottleneck for typical usage.

Note: Not a blocking issue, just an observation for future optimization.

---

📋 Code Quality

Positives:

• ✅ Consistent error handling
• ✅ Clear function naming (mostly)
• ✅ Good use of Go idioms (slices.Collect, maps.Keys, etc.)
• ✅ Proper separation of concerns

Improvements Needed:

• ⚠️ Missing inline comments for complex logic
• ⚠️ No tests for new functionality
• ⚠️ Dead code should be removed

---

📝 Documentation

Help Text Updates

The help.txt is well-structured and includes examples for the new commands. Good job!

Missing Documentation:

• No updates to docs/reference/cli.md (if it exists) documenting the new commands
• No updates to README showing the new introspection capabilities

Recommendation: Update relevant documentation to showcase these new features.

---

✅ Recommendations Summary

Must Fix (Blocking):

1. Add test coverage for new commands (following txtar pattern)
2. Remove dead code in internal/astgen/pkg.go

Should Fix:

3. Remove unused typeFormatter or document its purpose
4. Remove whitespace change in docs/prompts/muxt-complete.md
5. Add documentation for the new commands

Nice to Have:

6. Consider renaming commands for clarity (e.g., who-calls vs what-it-calls)
7. Add comments explaining complex template logic
8. Update README/docs to showcase new analysis features

---

🎯 Overall Assessment

This is a solid enhancement that significantly improves muxt's introspection capabilities. The refactoring improves code organization and maintainability. The main concerns are:

• Missing test coverage
• Dead/unused code that should be cleaned up

Once tests are added and dead code is removed, this PR will be ready to merge.

Rating: 7/10 → 9/10 with test coverage

Great work on the feature addition! 🚀

[claude]

Pull Request Review: Add Report Commands

Summary

This PR refactors the CLI to use the Cobra framework and replaces the documentation command with new list-template-callers and list-template-calls report commands. The changes include significant restructuring of the analysis package and CLI command handling.

---

✅ Positive Changes

1. Improved CLI Structure: Migration to Cobra provides better command organization, help text, and flag management
2. Better Separation of Concerns: The refactoring of analysis/documentation.go into separate routes.go, template_callers.go, and template_calls.go files improves modularity
3. Template-based Output: Using Go templates for output formatting in analysis/*.txt.template is more maintainable than string concatenation
4. Code Reuse: New asteval.LoadTemplates() and asteval.FindType() helper functions reduce duplication

---

🐛 Potential Issues

1. Breaking Change: Error Output Removed (cmd/muxt/main.go:17-19)

func handleError(err error) int {
    if err != nil {
-       _, _ = os.Stderr.WriteString(err.Error() + "\n")
        return 1
    }
    return 0
}

Issue: Error messages are no longer printed to stderr. With Cobra, errors should be printed by the framework, but this needs verification that all error paths properly display errors to users.

Recommendation: Verify that Cobra is configured to print errors in all cases, or restore error printing. Users need to see error messages!

2. Incomplete Code in internal/astgen/pkg.go

The entire file contains commented-out code that appears to be work-in-progress:

//type TypeFormatter struct {
//    outputPkgPath string
//    imports       map[string]string
//}

Issue: This suggests incomplete implementation or leftover debugging code.

Recommendation: Either complete the implementation if needed for this PR, or remove the file entirely if it's not yet ready.

3. Missing Error Handling in Template Execution

In template_callers.go:61 and template_calls.go:50:

_ = check.Execute(global, t.Tree, dataType)

Issue: Template execution errors are silently ignored. While this may be intentional for partial analysis, it could hide important issues.

Recommendation: Consider logging ignored errors when in verbose mode, or add a comment explaining why errors are safely ignored here.

---

🔍 Code Quality & Best Practices

1. Inconsistent Variable Naming

In cli/commands.go:57, the closure captures wd but modifies it:

PersistentPreRunE: func(cmd *cobra.Command, _ []string) error {
    // ... modifies wd variable
    wd = newWd
    return nil
}

Issue: This modifies the outer wd variable within a closure. While functional, it's subtle and could be confusing.

Recommendation: Consider using a pointer to wd explicitly (*workingDirectory pattern used elsewhere) for consistency.

2. Template Loading Efficiency

LoadTemplates is called multiple times with the same parameters:

• listTemplateCallersCommand:331
• listTemplateCallsCommand:361

Recommendation: This is likely fine for CLI usage, but document that templates are re-parsed on each command invocation if that's intentional.

3. Missing Validation in routes.go:78-81

names := slices.Collect(maps.Keys(functions))
for _, name := range names {
    s := strings.TrimPrefix(functions[name].String(), "func")
    funcList = append(funcList, Function{Name: name, Signature: s})
}

Issue: No sorting of function names before iteration, which could lead to non-deterministic output.

Recommendation: Sort names slice after collection to ensure deterministic output:

names := slices.Sorted(maps.Keys(functions))

4. Test Coverage

The PR removes test files:

• howto_use_documentation_command.txt
• reference_documentation_output.txt

Issue: No replacement tests for the new commands are visible in the diff.

Recommendation: Ensure txtar test files exist for:

• muxt list-template-callers
• muxt list-template-calls
• Default command behavior (running muxt with no args)

---

🔒 Security Considerations

No security issues identified. The changes primarily involve CLI parsing and output formatting with no external input processing or privileged operations.

---

⚡ Performance Considerations

1. Package Loading: asteval.LoadPackages() loads multiple packages including encoding, fmt, net/http every time. This is likely fine for CLI usage but could be slow for large codebases.

2. Duplicate Template Parsing: Each report command re-parses templates. For CLI tools this is acceptable, but worth noting.

Assessment: Performance impact is acceptable for a CLI tool. No changes needed.

---

📋 Additional Recommendations

1. Documentation Updates: Update docs/reference/cli.md to reflect:

   • Removal of documentation command
   • Addition of list-template-callers and list-template-calls commands
   • New default behavior when running muxt without arguments

2. Migration Guide: Consider adding a note in the PR description or changelog about how users should migrate from muxt documentation to the new commands.

3. Whitespace Cleanup: docs/prompts/muxt-complete.md:1 has an extra blank line added at the top - likely unintentional.

4. Flag Consistency: The new commands use --use-templates-variable while the flag help text may reference the old name. Verify all help text is updated.

---

Summary Assessment

Overall: This is a well-structured refactoring that improves code organization and CLI usability. The main concerns are:

1. Critical: Verify error messages are displayed to users (removed stderr write in main.go)
2. Important: Remove or complete the astgen/pkg.go stub code
3. Nice-to-have: Add tests for new commands and improve determinism

The code quality is good, follows Go best practices, and the architectural changes are sound. Once the error handling verification is confirmed and the incomplete code is addressed, this should be ready to merge.

Recommendation: Request changes to address items #1 and #2 above before merging.

[claude]

Pull Request Review: Add Report Commands

Summary

This PR refactors the CLI from a custom command parser to use the Cobra framework and introduces new reporting commands (list-template-callers and list-template-calls). The documentation command is replaced with a default root command that shows routes/templates.

Code Quality & Best Practices ✅

Strengths:

• Good refactoring: Migration to Cobra is a solid choice for CLI management, providing better structure and help text
• Clean separation: New analysis package properly separates concerns (routes, template_callers, template_calls)
• Template-driven output: Using Go templates for output formatting is consistent and maintainable
• Reusable helpers: Extracted functions like LoadTemplates() and FindType() in asteval package promote code reuse
• Consistent naming: New report commands follow clear naming conventions

Areas for improvement:

1. Commented-out code in internal/astgen/pkg.go: The entire file is commented out. Should either be removed or implemented

   • Location: internal/astgen/pkg.go:3-28

2. Error handling in main.go: Removed error message output but kept error code

   • Location: cmd/muxt/main.go:17-21
   • The removal of os.Stderr.WriteString(err.Error() + "\n") means errors are silently ignored
   • Cobra likely handles this, but worth verifying

Potential Bugs or Issues ⚠️

1. Inconsistent template output formatting:

   • template_calls.txt.template:4 uses tab indentation
   • template_callers.txt.template:4 uses "- " prefix with spaces
   • Recommend standardizing to one format

2. Whitespace handling in templates:

   • routes.txt.template has {{end -}} on line 8 to trim whitespace, but other templates don't follow this pattern consistently
   • May produce unexpected blank lines in output

3. Missing nil checks:

   • routes.go:86 - def.Template().Tree.Root.String() could panic if Tree or Root is nil
   • Consider defensive checks before dereferencing

4. Variable shadowing in commands.go:

   • commands.go:55 - wd is captured by pointer in workingDirectory *string parameter but reassigned in PersistentPreRunE
   • The reassignment only affects the local copy in the closure, not the pointer target
   • Should modify via: *workingDirectory = newWd instead of wd = newWd

5. Incomplete migration:

   • Deleted test files howto_use_documentation_command.txt and reference_documentation_output.txt without replacement tests for the new root command behavior
   • No tests for new list-template-callers and list-template-calls commands

Performance Considerations 🚀

Positive:

• Reuse of LoadTemplates() reduces duplicate AST parsing
• Template output generation is efficient

Neutral:

• Cobra adds minimal overhead compared to custom parser
• Analysis commands load full package AST, but this is expected for their use case

Security Concerns 🔒

Good practices:

• Input validation for identifiers using token.IsIdentifier()
• File extension validation for output files
• No obvious injection vulnerabilities

Minor concern:

• configToArgs() builds command-line args from config but doesn't escape special characters
• Low risk since values are validated as identifiers, but worth noting

Test Coverage 📊

Issues identified:

1. Deleted tests without replacement:

   • cmd/muxt/testdata/howto_use_documentation_command.txt - deleted
   • cmd/muxt/testdata/reference_documentation_output.txt - deleted
   • The new root command behavior should have equivalent tests

2. Missing tests for new commands:

   • No test coverage for list-template-callers command
   • No test coverage for list-template-calls command
   • Consider adding txtar tests following the existing pattern

3. Deleted unit tests:

   • internal/cli/commands_test.go - entirely deleted (63 lines)
   • Should verify this functionality is covered elsewhere or add new tests

4. Template output tests:

   • No tests validating the output format of *.txt.template files
   • Consider adding golden file tests

Recommendations

High Priority:

1. Fix the wd variable shadowing issue in commands.go:55
2. Add tests for the new report commands
3. Either remove or complete internal/astgen/pkg.go
4. Add nil checks in routes.go:86 before dereferencing template tree

Medium Priority:

5. Standardize template formatting (tabs vs spaces, whitespace trimming)
6. Add tests for root command output (replacement for deleted documentation tests)
7. Verify error handling works correctly after removing stderr write in main.go

Low Priority:

8. Add documentation/comments for the new analysis package functions
9. Consider extracting template output constants for separator strings ("=", "-")

Overall Assessment

This is a solid refactoring that improves the CLI structure and adds useful introspection commands. The migration to Cobra is well-executed, and the new analysis capabilities are valuable additions.

Main concerns: Test coverage gaps and the variable shadowing bug. Once these are addressed, this will be ready to merge.

Would you like me to help add tests or fix any of the identified issues?

---

🤖 Generated with Claude Code

[claude]

PR Review: Add Report Commands

Summary

This PR replaces the documentation command with more focused report commands (list-template-callers and list-template-calls) and refactors the CLI to use Cobra. The changes align well with the project's goals and follow good practices overall.

✅ Code Quality & Best Practices

Strengths:

• Clean separation of concerns: The refactoring splits the old documentation.go into three focused files (routes.go, template_callers.go, template_calls.go), each with a single responsibility
• Consistent naming: New command names follow a clear list-template-* pattern with intuitive aliases (callers, calls)
• Reusable abstractions: The NamedReferences and TemplateReference types are well-designed and shared across analysis functions
• Template-based output: Using text templates for formatting output (via *.txt.template files) provides good separation between logic and presentation
• Backward compatibility: Deprecated flags are handled gracefully with clear deprecation messages

Cobra Integration:

• The migration to Cobra is well-executed and provides better structure for the growing CLI
• Command organization is logical with proper use of aliases and help text
• The Commands() function signature change (os.Args → os.Args[1:]) correctly delegates argument parsing to Cobra

🐛 Potential Issues

Minor Issues:

1. Error message removal in main.go (cmd/muxt/main.go:18-19):

   func handleError(err error) int {
       if err != nil {
   -       _, _ = os.Stderr.WriteString(err.Error() + "\n")
           return 1
       }
       return 0
   }

   The error message writing was removed from handleError(). This is intentional since Cobra now handles error output, but it could be worth documenting in a code comment why this is empty.

2. matchesAny case-insensitivity (internal/analysis/analysis.go:20-28):
   The matchesAny() function performs case-insensitive substring matching. This is user-friendly but could be surprising if users expect exact matches. Consider documenting this behavior in the --template flag help text.

3. Missing nil check for empty filters (internal/analysis/template_callers.go:69):

   if len(config.FilterTemplates) > 0 && !matchesAny(name, config.FilterTemplates) {

   While this works correctly, matchesAny() doesn't handle the case where filters is empty internally. The length check protects it, but making matchesAny() return true for empty filters would be more defensive.

Test Coverage:

Good:

• The new commands have solid test coverage via txtar tests:
  • howto_list_template_callers_alias.txt
  • howto_list_template_callers_filter.txt
  • howto_list_template_calls_alias.txt
  • howto_list_template_calls_filter.txt
  • reference_list_template_callers.txt
  • reference_list_template_calls.txt

Could be improved:

• No error test cases for the new commands (e.g., invalid template variable, missing templates)
• No test coverage for the root command's new default behavior (showing routes)

⚡ Performance Considerations

• AST traversal: The use of ast.Preorder() in both template_callers.go and template_calls.go is appropriate for this use case
• Map usage: The refs and uses maps are reasonable for template analysis and won't scale poorly for typical codebases
• Template parsing: Template parsing happens only once via go:embed, which is efficient
• No performance concerns identified

🔒 Security Concerns

• No security issues identified
• File paths are handled safely through filepath package
• No user-controlled template execution or eval risks
• Dependency updates (golang.org/x, spf13/cobra) are minor version bumps - no known CVEs

📋 Additional Observations

Positive Changes:

1. Improved UX: The new commands are more discoverable and focused than the old documentation command
2. Better error handling: Cobra provides standardized error formatting
3. Consistent flag routing: All flag sets properly route output to stderr (lines 85-91 in commands.go)
4. Clean dependency injection: Commands receive workingDirectory pointer for testability

Suggestions for Follow-up:

1. Consider adding error tests for new commands (following the err_* pattern)
2. Update any documentation that references the removed documentation command
3. Consider adding a migration guide for users switching from documentation to the new commands
4. The blank line added at the top of docs/prompts/muxt-complete.md appears unintentional

🎯 Verdict

This PR is well-crafted and ready to merge with only minor observations:

• The code follows project conventions (see Claude.md)
• Test coverage is good for the happy path
• No blocking issues identified
• The refactoring improves code organization

The removal of the documentation command in favor of more granular commands represents better UX design. Great work! 🎉

[claude]

Pull Request Review: Add Template List Commands

Summary

This PR refactors the CLI from a custom command parser to use the Cobra framework, removes the documentation command, and adds two new commands (list-template-callers and list-template-calls) for analyzing template relationships. The changes are well-structured and improve the CLI's maintainability.

---

✅ Code Quality & Best Practices

Strengths:

• Excellent refactoring: The migration to Cobra is clean and follows idiomatic Go patterns
• Good separation of concerns: New analysis functionality is properly split into focused modules:
  • internal/analysis/template_callers.go - tracks where templates are called
  • internal/analysis/template_calls.go - tracks what each template calls
  • internal/analysis/routes.go - replaces the old documentation functionality
• Consistent naming: Commands use clear, descriptive names with appropriate aliases (callers, calls)
• Template-based output: Using text/template for output formatting (*.txt.template files) provides good separation of logic and presentation
• Comprehensive test coverage: Excellent test files added with both "howto" and "reference" examples following the project's test naming conventions

Minor concerns:

1. Error handling in main.go (lines 7-10): The error message is no longer written to stderr after the change. This could make debugging harder:

   func handleError(err error) int {
       if err \!= nil {
           // Previously: _, _ = os.Stderr.WriteString(err.Error() + "\n")
           return 1
       }
       return 0
   }

   Impact: Errors may be silently swallowed. Cobra should be handling error output, but this should be verified.

2. Unused parameter in routes.go (line 50): The fileSet parameter is explicitly ignored with _ but still passed. Consider if it's actually needed.

---

🐛 Potential Bugs

1. regex compilation without validation context (cli/commands.go:327-333, 365-371):

   for _, pattern := range patterns {
       pat, err := regexp.Compile(pattern)
       if err \!= nil {
           return err  // Error message could be more descriptive
       }
       config.FilterTemplates = append(config.FilterTemplates, pat)
   }

   Issue: Error doesn't indicate which pattern failed to compile. With multiple --match flags, users won't know which one is invalid.

   Recommendation: Add context to the error message:

   return fmt.Errorf("invalid regex pattern %q: %w", pattern, err)

2. Potential nil pointer dereference (analysis/routes.go:86-89):
   The code checks for nil values but doesn't handle all edge cases:

   t := def.Template()
   if t == nil || t.Tree == nil || t.Tree.Root == nil {
       continue
   }

   Question: Can def.Template() panic? Should verify error handling in the muxt.Definitions call.

3. No validation for empty template variable (cli/commands.go:324-326, 362-364):

   if config.TemplatesVariable == "" {
       config.TemplatesVariable = defaultTemplatesVariableName
   }

   This is done in list commands but not consistently in other commands. The root command doesn't have this check.

---

🔒 Security Concerns

Overall: Low Risk

1. Regex DoS potential: User-supplied regex patterns in --match flags could potentially cause ReDoS (Regular Expression Denial of Service) with malicious patterns. However, this is a CLI tool typically run locally, so the risk is minimal.

2. File permissions (cli/commands.go:233): Generated files use 0o644 which is appropriate (owner read/write, group/others read-only).

3. No input sanitization issues: The code properly uses Go's template system and type-safe operations. No SQL injection, command injection, or XSS risks identified.

---

⚡ Performance Considerations

1. Template parsing efficiency: Good - templates are parsed once and reused
2. AST traversal (template_callers.go:45-64): Uses efficient ast.Preorder iterator
3. Map operations: Proper use of maps.Keys and slices.Sorted from Go 1.21+
4. Potential concern: The template analysis walks the entire AST for each command. For large projects, this could be slow, but it's acceptable for a CLI tool.

Recommendation: Consider adding a --verbose flag timing information if users need to debug performance.

---

🧪 Test Coverage

Excellent test coverage:

• ✅ howto_list_template_callers.txt - demonstrates filtering with --match flag
• ✅ howto_list_template_calls.txt - demonstrates filtering functionality
• ✅ reference_list_template_callers.txt - validates basic output format
• ✅ reference_list_template_calls.txt - validates basic output format
• ✅ Tests follow project conventions (txtar format, clear naming)
• ✅ Both positive cases covered
• ⚠️ Missing: Error cases for invalid regex patterns in --match flag
• ⚠️ Missing: Tests for new aliases (callers, calls)

Recommendations:

1. Add error test: err_list_template_calls_invalid_regex.txt
2. Add test verifying aliases work: reference_command_aliases.txt
3. Consider adding test for empty/no templates case

---

📋 Additional Observations

1. Breaking change: The removal of the documentation command is a breaking change. Ensure this is documented in release notes or migration guide.

2. Dependency updates: The PR includes updates to:

   • github.com/spf13/cobra (new dependency)
   • golang.org/x/net, golang.org/x/tools, golang.org/x/sync, etc. (minor version bumps)

   All updates look reasonable and compatible.

3. Args handling change (main.go:14): Changed from os.Args to os.Args[1:] - this is correct for Cobra but should be verified that the command name is no longer needed.

4. New helper function: asteval.FindType is a good addition that could be useful elsewhere in the codebase.

5. Template organization: The new template files (*.txt.template) are well-organized in the same package as their code, following Go conventions.

---

📝 Recommendations Summary

High Priority:

1. ✅ Verify error output behavior after main.go handleError changes
2. 🔧 Add error context to regex compilation failures
3. 🧪 Add test coverage for error cases and command aliases

Medium Priority:
4. 📚 Document the breaking change (removal of documentation command)
5. 🔍 Consider adding validation for edge cases in template references

Low Priority:
6. 💡 Consider adding timing information with --verbose for performance debugging
7. 📊 Add test for empty templates case

---

✨ Conclusion

This is a high-quality PR that significantly improves the CLI architecture. The refactoring to Cobra is well-executed, the new template analysis commands are valuable additions, and the code follows Go best practices. The test coverage is comprehensive and follows project conventions.

Recommendation: Approve with minor fixes (primarily improving error messages and adding error case tests).

Great work on this refactor! 🚀