feat(new-session-attach): update internal/cli/new.go

[jlaneve]

No description provided.

[claude]

Claude finished @jlaneve's task —— View job

---

PR Review: feat(new-session-attach)

Reviewing the changes to internal/cli/new.go for code quality, potential issues, and best practices.

Review Checklist

• Read and understand the changed file
• Analyze code quality and best practices
• Check for potential bugs or issues
• Review performance considerations
• Assess security concerns
• Evaluate test coverage needs
• Provide comprehensive feedback

Summary

This PR adds automatic session attachment to the cwt new command, providing a smoother user experience by eliminating the need to manually run cwt attach after session creation.

✅ Positive Aspects

1. User Experience Improvement

• Eliminates friction in the workflow - users no longer need to remember to run cwt attach manually
• Maintains consistency with the described workflow in workflows.md

2. Robust Error Handling

• Input validation for empty tmux session names (internal/cli/new.go:93-95)
• Pre-flight verification that tmux session exists before attempting attach (internal/cli/new.go:104-106)
• Proper error wrapping with context

3. Good Documentation

• Clear comments explaining syscall.Exec behavior and why code after it won't execute
• Informative user messages during the attachment process

4. Security Considerations

• Uses exec.LookPath("tmux") to safely locate tmux binary
• Validates tmux session existence before attachment to prevent potential race conditions

⚠️ Areas for Improvement

1. Code Duplication with attach.go
There's some duplication between the new attachToTmuxSession() in new.go and the attachment logic in attach.go:111-120:

// Both files have nearly identical tmux exec logic
tmuxPath, err := exec.LookPath("tmux")
args := []string{"tmux", "attach-session", "-t", sessionName}
err = syscall.Exec(tmuxPath, args, os.Environ())

Recommendation: Consider extracting this to a shared utility function in a utils package or the operations layer.

2. Inconsistent Error Handling

• new.go:116 uses panic("syscall.Exec returned unexpectedly")
• attach.go:123 uses return nil

Both approaches are technically correct since this code should never execute, but consistency would be better.

3. Missing User Feedback
The attach command in attach.go shows helpful messages like:

🔗 Attaching to session 'name' (tmux: cwt-name)...

The new auto-attach only shows:

🔗 Attaching to session...

Recommendation: Include the session name for consistency.

🐛 Potential Issues

1. Race Condition (Minor)
There's a small window between session creation and attachment where the tmux session could theoretically die. The verifyTmuxSessionExists() check mitigates this, but it's worth noting.

2. Process Replacement Behavior
The syscall.Exec completely replaces the current process. This is expected behavior, but users might not realize that any cleanup code won't run after attachment.

🚀 Performance Considerations

No concerns - The added verification step (tmux has-session) is minimal overhead and improves reliability.

🔒 Security Assessment

No security concerns - The implementation properly validates inputs and uses safe path lookup for the tmux executable.

🧪 Test Coverage Recommendations

Consider adding tests for:

1. Happy path: Session creation followed by successful attachment
2. Error cases:
   • Empty tmux session name
   • Tmux not found in PATH
   • Session doesn't exist (edge case)
3. Integration test: End-to-end test that verifies cwt new creates and attaches properly

📝 Minor Suggestions

1. Consider adding a flag to disable auto-attach
Some users might prefer the old behavior:

cmd.Flags().BoolVar(&noAttach, "no-attach", false, "Don't automatically attach to the session")

2. Consistent messaging
Update the attachment message to match the style in attach.go:

fmt.Printf("🔗 Attaching to session '%s' (tmux: %s)...\n", sessionName, tmuxSessionName)

🎯 Overall Assessment

This is a solid improvement that enhances user experience while maintaining good error handling and security practices. The main areas for improvement are around reducing code duplication and improving consistency with existing patterns.

Recommendation: Approve with minor suggestions - The core functionality is sound and adds real value to users.

[jlaneve]

@claude can you fix these issues

[claude]

Claude finished @jlaneve's task —— View job

---

✅ Issues Fixed Successfully

I've successfully addressed all the issues identified in my previous code review:

Todo List

• Read current state of files to understand code duplication
• Identify best location for shared tmux attachment utility
• Extract shared tmux attachment logic
• Update both files to use shared function
• Standardize error handling approach
• Improve user feedback messages
• Test the changes
• Commit and push changes

Changes Implemented

1. Eliminated Code Duplication ✅

• Created shared AttachToTmuxSession() function in internal/operations/sessions.go
• Removed duplicate tmux attachment logic from both new.go and attach.go
• Single source of truth for tmux attachment operations

2. Standardized Error Handling ✅

• Consistent panic("syscall.Exec returned unexpectedly") after syscall.Exec in both commands
• Proper error wrapping and validation in shared function

3. Improved User Feedback ✅

• Both commands now show consistent message: "🔗 Attaching to session 'session-name' (tmux: cwt-session-name)..."
• Session names are properly displayed in attachment messages

4. Code Quality Improvements ✅

• Cleaned up unused imports (os/exec, syscall, os) from CLI files
• Better separation of concerns with business logic in operations layer
• Improved maintainability and testability

Files Modified

• internal/operations/sessions.go - Added shared attachment function
• internal/cli/new.go - Updated to use shared function, removed duplicates
• internal/cli/attach.go - Updated to use shared function, cleaned imports

Commit: 40d1f5e - Successfully pushed to new-session-attach branch

The PR now has much cleaner, more maintainable code with consistent user experience across both commands.

---