Bug RHOAIENG-39115: Preserve command order from frontmatter

[Gkrumbach07]

Summary

Fixes RHOAIENG-39115 - Slash commands now render in the order specified in the custom workflow template instead of alphabetically.

Problem

Slash commands were rendering alphabetically in the dashboard UI, ignoring the order specified in the custom workflow template. This occurred because the backend used os.ReadDir() which returns files in filesystem order (typically alphabetical), with no mechanism to preserve intentional ordering.

Solution

Added support for an order field in command frontmatter:

• Backend parses order field from YAML frontmatter in .claude/commands/*.md files
• Commands are sorted by the order field before being returned to the frontend
• Commands without an order field default to MaxInt (appear at the end)
• Alphabetical fallback when orders are equal ensures consistent ordering

Changes

File Modified: components/backend/handlers/content.go

• Added sort and strconv imports
• Modified ContentWorkflowMetadata() to parse order field from frontmatter
• Implemented sorting logic for commands array based on order field
• Added fallback logic for missing or equal order values

Example Usage

Commands can now specify order in their frontmatter:

---
displayName: "Create RFE"
description: "Start RFE creation process"
order: 1
---

Backward Compatibility

✅ Fully backward compatible:

• Existing workflows without order fields continue to work
• Commands without order appear at the end (alphabetically sorted among themselves)
• No breaking changes to existing API or data structures

Testing Notes

The implementation handles edge cases:

• Invalid order values (non-numeric) are gracefully ignored
• Empty order field defaults to MaxInt
• Type assertions with proper fallbacks in sorting function
• Alphabetical ordering when order values are equal

🤖 Generated with Claude Code

[github-actions]

Claude Code Review

Summary

PR #535 adds support for ordering slash commands in the dashboard UI via an order field in command frontmatter. The implementation is functionally correct and solves the stated problem, but there are several opportunities for improvement in code quality, error handling, and maintainability.

Issues by Severity

🔴 Critical Issues

1. MaxInt Calculation Could Be Simplified (Go 1.18+)

• Location: content.go:523, 546, 549
• Issue: Using int(^uint(0) >> 1) to calculate MaxInt is unnecessarily complex
• Fix: Go 1.17+ has math.MaxInt constant
• Impact: Code readability and maintainability

// Current (complex)
order := int(^uint(0) >> 1)

// Better (Go 1.17+)
import "math"
order := math.MaxInt

🟡 Major Issues

2. Redundant Type Assertions in Sort Function

• Location: content.go:542-558
• Issue: The order field is always set to an int value at line 536, so type assertions in the sort function will always succeed. The fallback logic to MaxInt is unreachable dead code.
• Fix: Simplify the sort function since order is guaranteed to be an int

// Current (unnecessary type checking)
sort.Slice(commands, func(i, j int) bool {
    iOrder, iOk := commands[i]["order"].(int)
    jOrder, jOk := commands[j]["order"].(int)
    if !iOk {
        iOrder = int(^uint(0) >> 1) // Dead code - always succeeds
    }
    if !jOk {
        jOrder = int(^uint(0) >> 1) // Dead code - always succeeds
    }
    // ...
})

// Better (remove dead code)
sort.Slice(commands, func(i, j int) bool {
    iOrder := commands[i]["order"].(int)
    jOrder := commands[j]["order"].(int)
    if iOrder == jOrder {
        iID := commands[i]["id"].(string)
        jID := commands[j]["id"].(string)
        return iID < jID
    }
    return iOrder < jOrder
})

3. Missing Error Logging for Invalid Order Values

• Location: content.go:524-528
• Issue: When strconv.Atoi fails, the error is silently ignored. This makes debugging difficult if users provide invalid order values.
• Fix: Log invalid order values with the command name for debugging

if orderStr := metadata["order"]; orderStr != "" {
    if parsed, err := strconv.Atoi(orderStr); err == nil {
        order = parsed
    } else {
        log.Printf("ContentWorkflowMetadata: invalid order value %q for command %q: %v", orderStr, commandName, err)
    }
}

🔵 Minor Issues

4. Inconsistent Comment Formatting

• Location: content.go:522
• Issue: Comment says "MaxInt32" but the code computes platform-dependent MaxInt (could be MaxInt64 on 64-bit systems)
• Fix: Update comment to be accurate

// Parse order field from frontmatter, default to MaxInt for unordered commands

5. Missing Test Coverage for New Ordering Feature

• Location: content_test.go
• Issue: Existing test at line 832 creates a command without an order field but doesn't verify ordering behavior
• Recommendation: Add test cases for:
  • Commands with explicit order values
  • Commands without order values (should appear at end)
  • Commands with equal order values (should sort alphabetically)
  • Invalid order values (non-numeric)

6. API Response Includes Internal order Field

• Location: content.go:536
• Issue: The order field is added to the command object returned to the frontend, but this is an implementation detail
• Impact: Minor - adds 4 bytes per command to API response
• Recommendation: Consider removing order from the response after sorting (cleanup step)

Positive Highlights

✅ Excellent Backward Compatibility - Commands without order field continue to work as expected

✅ Robust Fallback Strategy - Using MaxInt for unordered commands ensures they appear at the end

✅ Alphabetical Tiebreaker - When orders are equal, alphabetical sorting provides deterministic ordering

✅ Good Problem Statement - PR description clearly explains the issue and solution

✅ Follows Established Patterns - Uses existing parseFrontmatter function consistently

Recommendations

Immediate Actions (Before Merge)

1. Replace MaxInt calculation with math.MaxInt (Critical - readability)
2. Add error logging for invalid order values (Major - debuggability)
3. Simplify sort function (Major - remove dead code)

Follow-up Actions (Can Be Separate PR)

4. Add comprehensive test coverage for ordering scenarios
5. Consider removing order field from API response (cleanup)
6. Update comment to reflect actual MaxInt (not MaxInt32)

Code Quality Assessment

Aspect	Rating	Notes
Correctness	✅ Good	Solves the stated problem correctly
Error Handling	⚠️ Fair	Silent failures for invalid order values
Code Quality	⚠️ Fair	Unnecessary complexity, dead code
Testing	⚠️ Fair	Missing test coverage for new feature
Security	✅ Good	No security concerns
Performance	✅ Good	Sorting is O(n log n), acceptable for small arrays
Maintainability	⚠️ Fair	Complex MaxInt calculation reduces clarity

Security Analysis

✅ No security vulnerabilities detected

• Input validation: strconv.Atoi safely handles malicious input
• No user-controlled data in file paths
• No injection risks
• Follows existing authentication patterns (implicit via ContentWorkflowMetadata handler)

Final Verdict

Conditional Approval - The PR is functionally correct and solves the stated problem, but would benefit from the recommended improvements before merging. The critical issues are about code quality rather than correctness, so the decision to merge depends on project standards for code clarity.

If immediate deployment is needed, the code can be merged as-is with the understanding that a follow-up PR will address the code quality issues.

---

Review performed by: Claude Sonnet 4.5 with full context from:

• CLAUDE.md (project standards)
• Backend development context
• Security standards
• Error handling patterns
• K8s client usage patterns

🤖 Automated code review by Claude Code

---

🔍 View AI decision process (logs available for 90 days)

📋 View memory system files loaded (click to expand)

What Amber Loaded for Code Review

Amber automatically loaded these repository standards from the memory system:

1. CLAUDE.md - Master project instructions, development standards
2. backend-development.md - Go backend, K8s integration patterns
3. frontend-development.md - NextJS, Shadcn UI, React Query patterns
4. security-standards.md - Auth, RBAC, token handling
5. k8s-client-usage.md - User token vs service account patterns
6. error-handling.md - Consistent error patterns
7. react-query-usage.md - Data fetching patterns

Impact: This review used your repository's specific code quality standards, security patterns, and best practices from the memory system (PRs #359, #360) - not just generic code review guidelines.