[security-fix] Fix unhandled errors in pr_command.go cleanup operations (Alerts #403-404)

[github-actions]

Security Fix: Unhandled Errors in Temporary Directory Cleanup

Alert Numbers: #403, #404
Severity: Low (Warning)
Rule: G104 - Errors unhandled
Tool: gosec (Golang security checks)
Location: pkg/cli/pr_command.go:636, 645

Vulnerability Description

The security scanner detected 2 instances in pkg/cli/pr_command.go where os.RemoveAll(tempDir) is called without checking for errors. The G104 rule flags situations where errors from function calls are silently ignored, which can lead to:

1. Silent failures: Cleanup failures go unnoticed, potentially leaving temporary directories behind
2. Resource leaks: Uncleaned temporary directories can accumulate over time
3. Debugging difficulties: Errors during cleanup are not logged, making it harder to diagnose issues
4. Violation of Go best practices: Error values should never be silently ignored

The unhandled errors were found at:

• Line 636: When repository clone fails
• Line 645: When changing directory fails after clone

Fix Applied

Added proper error handling for both os.RemoveAll(tempDir) calls:

Before:

if err := cloneCmd.Run(); err != nil {
    os.RemoveAll(tempDir)
    return fmt.Errorf("failed to clone target repository: %w", err)
}

After:

if err := cloneCmd.Run(); err != nil {
    // Clean up temporary directory on error
    if rmErr := os.RemoveAll(tempDir); rmErr != nil && verbose {
        fmt.Fprintf(os.Stderr, "Warning: failed to clean up temporary directory %s: %v\n", tempDir, rmErr)
    }
    return fmt.Errorf("failed to clone target repository: %w", err)
}

This approach:

• Checks the error returned by os.RemoveAll()
• Logs a warning message when cleanup fails (in verbose mode)
• Follows the same pattern used elsewhere in the file (see lines 662-664, 674-676)
• Maintains backward compatibility - cleanup failures don't block the error flow

Security Best Practices

1. Always handle errors: Even in cleanup/error paths, errors should be checked
2. Consistent error handling: Applied the same pattern used elsewhere in the same file
3. Non-critical operations: Since this is cleanup in an error path, we log warnings rather than returning errors
4. Informative logging: Includes the directory path and error details for debugging

Testing Considerations

• ✅ Build succeeds without errors
• ✅ No breaking changes to existing functionality
• ✅ Error handling is consistent with cleanup operations elsewhere in the file
• ✅ Minimal, surgical changes - only added error checking

Impact Assessment

Risk: Minimal
Breaking Changes: None
Backwards Compatibility: Full
Performance: No impact

Files Modified

• pkg/cli/pr_command.go:
  • Lines 636-640: Added error handling for cleanup after clone failure
  • Lines 648-652: Added error handling for cleanup after chdir failure

References

• CodeQL Alerts:
  • https://github.com/githubnext/gh-aw/security/code-scanning/403
  • https://github.com/githubnext/gh-aw/security/code-scanning/404
• Gosec G104: Audit errors not checked
• Go Error Handling: [Effective Go - Errors]((redacted)

---

🤖 Generated by Security Fix Agent in workflow run 20568043742

AI generated by Security Fix PR