feat:support CLI mode #514
Conversation
✅ Deploy Preview for reviewbot-x canceled.
|
![qiniu-x[bot]](https://avatars.githubusercontent.com/in/752628?s=60&v=4){kind=small}
| _ "github.com/qiniu/reviewbot/internal/linters/java/pmdcheck" | ||
| _ "github.com/qiniu/reviewbot/internal/linters/java/stylecheck" | ||
| _ "github.com/qiniu/reviewbot/internal/linters/lua/luacheck" | ||
| _ "github.com/qiniu/reviewbot/internal/linters/shell/shellcheck" |
There was a problem hiding this comment.
File is not gci-ed with --skip-generated -s standard -s default (gci)
Details
lint 解释
这个lint结果表明文件没有按照GCI（Go Code Import）工具的规范进行导入排序。GCI是一个用于自动整理Go代码导入语句的工具，以保持代码的一致性和可读性。
错误用法
以下是一个错误的示例，展示了未按GCI规范排列的导入语句：
package main
import (
"fmt"
"net/http"
"github.com/example/package1"
"github.com/example/package2"
)在这个例子中，导入语句没有按照字母顺序排列。
正确用法
以下是一个正确的示例，展示了按GCI规范排列的导入语句：
package main
import (
"fmt"
"net/http"
"github.com/example/package1"
"github.com/example/package2"
)在这个例子中，导入语句已经按照字母顺序排列。
💡 以上内容由 AI 辅助生成，如有疑问欢迎反馈交流
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #514 +/- ##
==========================================
- Coverage 22.91% 21.67% -1.24%
==========================================
Files 36 36
Lines 5080 5411 +331
==========================================
+ Hits 1164 1173 +9
- Misses 3776 4094 +318
- Partials 140 144 +4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| } | ||
|
|
||
| for _, tc := range tcs { | ||
| t.Run(tc.id, func(t *testing.T) { |
There was a problem hiding this comment.
unnecessary leading newline (whitespace)
Details
lint 解释
这个lint结果表明代码中存在不必要的前导换行符（空白字符）。在Go语言中，通常不需要在文件的开头或函数、方法等的定义之前添加额外的空行。
错误用法
package main
import "fmt"
func main() {
fmt.Println("Hello, World!")
}在这个例子中，在package main和import "fmt"之间有一个不必要的空行。
正确用法
package main
import "fmt"
func main() {
fmt.Println("Hello, World!")
}在这个例子中，去掉了不必要的空行，代码更加简洁。
💡 以上内容由 AI 辅助生成，如有疑问欢迎反馈交流
| log.Fatal(http.ListenAndServe(fmt.Sprintf(":%d", o.port), mux)) | ||
| } | ||
|
|
||
| func cliMode(o cliOptions) { |
There was a problem hiding this comment.
empty-lines: extra empty line at the end of a block (revive)
Details
lint 解释
这个lint结果表明在代码块的末尾存在多余的空行。根据编码规范，通常不建议在代码块的末尾添加额外的空行。
错误用法
func example() {
fmt.Println("Hello, World!")
}在这个示例中，在fmt.Println("Hello, World!")之后有一个多余的空行。
正确用法
func example() {
fmt.Println("Hello, World!")
}在这个示例中，去掉了多余的空行，使得代码更加简洁和规范。
💡 以上内容由 AI 辅助生成，如有疑问欢迎反馈交流
![niupilot[bot]](https://avatars.githubusercontent.com/in/1894735?s=60&v=4){kind=small}
| } | ||
| log.SetFlags(log.LstdFlags | log.Lshortfile | log.Llevel) | ||
| log.SetOutputLevel(2) | ||
| inputPath := os.Args[2] |
There was a problem hiding this comment.
🚨 Security Vulnerability - Path Traversal
This line directly uses os.Args[2] without validation, creating a path traversal vulnerability:
inputPath := os.Args[2]Risk: Attackers can access arbitrary files using paths like ../../../etc/passwd
Fix: Add input validation:
if len(os.Args) < 3 {
log.Fatal("Error: missing input path argument")
}
inputPath := filepath.Clean(os.Args[2])
if filepath.IsAbs(inputPath) && !strings.HasPrefix(inputPath, "/allowed/base/path") {
log.Fatal("Error: absolute paths not allowed")
}
| fs.StringVar(&o.llmAPIKey, "llm.api-key", "", "llm api key") | ||
|
|
||
| err := fs.Parse(os.Args[1:]) | ||
| err := fs.Parse(os.Args[2:]) |
There was a problem hiding this comment.
Hardcoded array index access os.Args[2:] without bounds checking can cause index out of bounds panic.
Fix: Add length validation:
if len(os.Args) < 3 {
log.Fatal("insufficient arguments for server mode")
}
err := fs.Parse(os.Args[2:])
| return nil | ||
| } | ||
|
|
||
| var viewTemplate = template.Must(template.New("view").Parse(` |
There was a problem hiding this comment.
🚨 XSS Vulnerability
The HTML template lacks proper output escaping, allowing XSS attacks through {{.Content}}.
Fix: Use html/template with automatic escaping:
import "html/template"
tmpl := template.Must(template.New("result").Parse(htmlTemplate))
| } | ||
|
|
||
| func (s *Server) withCancel(ctx context.Context, info *codeRequestInfo, fn func(context.Context) error) error { | ||
| func processForCLI(ctx context.Context, path string, o cliOptions) error { |
There was a problem hiding this comment.
🐌 Performance Issue - Sequential Execution
Linters execute sequentially instead of in parallel, causing 5-15x slower performance than necessary.
Impact: CLI mode will be significantly slower for large codebases.
Fix: Implement parallel execution:
var wg sync.WaitGroup
results := make(chan result, len(linters))
for _, linter := range linters {
wg.Add(1)
go func(l Linter) {
defer wg.Done()
// execute linter
results <- result{...}
}(linter)
}
| } | ||
| } | ||
|
|
||
| func serverMode(o serverOptions) { |
There was a problem hiding this comment.
📋 Code Quality - Function Too Complex
The serverMode function is 115 lines long and handles multiple responsibilities (config validation, service init, HTTP setup, debug server).
Recommendation: Break into smaller functions:
validateServerConfig()initializeServices()setupHTTPServers()
This improves testability and maintainability.
Code Review Summary🚨 Critical Security Issues Found - This PR introduces significant security vulnerabilities that must be addressed before merging. Key Findings:
Priority Issues:
The CLI mode feature is functionally complete but requires security hardening and performance optimization before production use. |
![xgopilot[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
| } | ||
| log.SetFlags(log.LstdFlags | log.Lshortfile | log.Llevel) | ||
| log.SetOutputLevel(2) | ||
| inputPath := os.Args[2] |
There was a problem hiding this comment.
🚨 Critical Security Issue: Command Injection Vulnerability
Using os.Args[2] directly without validation creates a command injection vulnerability. An attacker could execute arbitrary commands by crafting malicious file paths.
| inputPath := os.Args[2] | |
| if len(os.Args) < 3 { | |
| log.Fatalf("missing input path argument") | |
| } | |
| inputPath := filepath.Clean(os.Args[2]) | |
| absPath, err := filepath.Abs(inputPath) | |
| if err != nil { | |
| log.Fatalf("invalid path: %v", err) | |
| } | |
| // Validate path is within expected bounds | |
| if !isValidPath(absPath) { | |
| log.Fatalf("path not allowed: %s", inputPath) | |
| } |
CWE-78 (OS Command Injection) - This must be fixed before merging.
| } | ||
| if ln == "golangci-lint" { | ||
| linter.Args = []string{ | ||
| "export GO111MODULE=auto\n", |
There was a problem hiding this comment.
🚨 Security Risk: Hardcoded Shell Commands
These hardcoded shell command strings pose injection risks and are difficult to maintain.
| "export GO111MODULE=auto\n", | |
| const ( | |
| goModTidyCmd = "go mod tidy" | |
| golangciLintCmd = "golangci-lint run --timeout=10m0s --allow-parallel-runners=true --print-issued-lines=false --out-format=line-number" | |
| ) | |
| // Use proper command construction instead of shell strings | |
| linter.Args = []string{ | |
| fmt.Sprintf("export GO111MODULE=auto && %s && %s", goModTidyCmd, golangciLintCmd), | |
| } |
Consider using os/exec with separated arguments for better security.
| func gatherOptions() options { | ||
| o := options{} | ||
| fs := flag.NewFlagSet(os.Args[0], flag.ExitOnError) | ||
| func gatherSeverOptions(fs *flag.FlagSet) serverOptions { |
There was a problem hiding this comment.
This function has 28+ parameters making it difficult to maintain and test. Consider breaking it into smaller, focused functions.
| func gatherSeverOptions(fs *flag.FlagSet) serverOptions { | |
| func gatherServerOptions() *serverOptions { | |
| o := &serverOptions{} | |
| // Break into logical groups | |
| gatherGitHubOptions(o) | |
| gatherGitLabOptions(o) | |
| gatherLLMOptions(o) | |
| gatherGeneralOptions(o) | |
| return o | |
| } |
This improves maintainability and makes testing easier.
Security & Code Quality Review Summary🚨 Critical Issues Found - Do Not Merge Until Fixed Security Vulnerabilities (Must Fix)
Code Quality Issues
Recommendations
The CLI feature implementation is solid but security issues must be addressed first. |
| } | ||
| log.SetFlags(log.LstdFlags | log.Lshortfile | log.Llevel) | ||
| log.SetOutputLevel(2) | ||
| inputPath := os.Args[2] |
There was a problem hiding this comment.
🚨 Critical: Array Index Out of Bounds
Direct access to os.Args[2] without bounds checking will cause a panic if insufficient arguments are provided.
| inputPath := os.Args[2] | |
| if len(os.Args) < 3 { | |
| log.Fatalf("missing input path argument") | |
| } | |
| inputPath := os.Args[2] |
Risk: Application crash with insufficient CLI arguments
| } | ||
| if ln == "golangci-lint" { | ||
| linter.Args = []string{ | ||
| "export GO111MODULE=auto\n", |
There was a problem hiding this comment.
🚨 Security Critical: Command Injection Risk
Hardcoded shell commands create command injection vulnerabilities and maintenance issues.
| "export GO111MODULE=auto\n", | |
| const ( | |
| goModTidyCmd = "go mod tidy" | |
| golangciLintCmd = "golangci-lint run --timeout=10m0s --allow-parallel-runners=true --print-issued-lines=false --out-format=line-number" | |
| ) | |
| // Use structured command execution instead of shell strings | |
| linter.Command = []string{"/bin/bash", "-c"} | |
| linter.Args = []string{ | |
| fmt.Sprintf("export GO111MODULE=auto && %s && %s", goModTidyCmd, golangciLintCmd), | |
| } |
CWE-78: Consider using os/exec with separated arguments for better security.
| } | ||
|
|
||
| func (s *Server) withCancel(ctx context.Context, info *codeRequestInfo, fn func(context.Context) error) error { | ||
| func processForCLI(ctx context.Context, path string, o cliOptions) error { |
There was a problem hiding this comment.
⚡ Performance: Sequential Execution Bottleneck
Linters execute sequentially, causing significant performance issues for large codebases.
Impact: 5 linters taking 30s each = 150s total vs ~35s with parallelization
Recommendation: Implement parallel execution:
func processLintersInParallel(ctx context.Context, linters map[string]func, maxWorkers int) error {
jobs := make(chan linterJob, len(linters))
results := make(chan error, len(linters))
// Start worker pool
for i := 0; i < maxWorkers; i++ {
go func() {
for job := range jobs {
results <- job.fn(ctx, job.agent)
}
}()
}
// Submit jobs and collect results
// ... implementation
}
| if info, err := os.Stat(path); err == nil && !info.IsDir() { | ||
| linterConfig.WorkDir = filepath.Dir(path) | ||
| } | ||
| if o.logDir == "" { |
There was a problem hiding this comment.
♻️ Code Quality: Inefficient Resource Creation
Creating new storage instance for each linter is wasteful. Create once and reuse.
| if o.logDir == "" { | |
| // Move this outside the loop, before line 431 | |
| storage, err := storage.NewLocalStorage(o.logDir) | |
| if err != nil { | |
| return err | |
| } | |
| // Then reuse in the loop: | |
| agent.Storage = storage |
| if len(os.Args) >= 2 && (os.Args[1] == "version" || os.Args[1] == "-v" || os.Args[1] == "--version") { | ||
| fmt.Println(version.Version()) | ||
| if len(os.Args) < 2 { | ||
| fmt.Println("Usage: reviewbot server-run") |
There was a problem hiding this comment.
📚 Documentation: Missing CLI Usage Help
The CLI mode lacks proper usage documentation. Users won't know how to use this new feature.
| fmt.Println("Usage: reviewbot server-run") | |
| fmt.Println("Usage: reviewbot <command> [flags]") | |
| fmt.Println("") | |
| fmt.Println("Available commands:") | |
| fmt.Println(" server-run Start reviewbot in webhook server mode") | |
| fmt.Println(" cli-run Run linters against local files/directories") | |
| fmt.Println(" version Show version information") | |
| fmt.Println("") | |
| fmt.Println("Examples:") | |
| fmt.Println(" reviewbot cli-run .") | |
| fmt.Println(" reviewbot cli-run --logDir=/tmp/logs src/") |
Also needed: Add CLI mode section to README.md
Code Review Summary🎯 CLI Mode Implementation - Good Foundation with Critical Issues This PR successfully adds CLI mode functionality to reviewbot, providing a valuable local development tool. However, several critical security and reliability issues must be addressed before merging. 🚨 Critical Issues (Must Fix)
📋 Key Recommendations
✅ Positive Aspects
Priority: Address security and reliability issues first, then optimize performance. The CLI feature will be a great addition once these concerns are resolved. |
| } | ||
| log.SetFlags(log.LstdFlags | log.Lshortfile | log.Llevel) | ||
| log.SetOutputLevel(2) | ||
| inputPath := os.Args[2] |
There was a problem hiding this comment.
🚨 Critical Security Issue: Command Injection Vulnerability
This line directly uses user-supplied command line arguments without validation, creating a critical security vulnerability:
inputPath := os.Args[2]Issues:
- No bounds checking (could panic if
os.Argshas < 3 elements) - No input validation or sanitization
- Path could contain shell metacharacters leading to command injection
- Potential for path traversal attacks
Recommendation:
if len(os.Args) < 3 {
return fmt.Errorf("missing input path argument")
}
inputPath, err := validatePath(os.Args[2])
if err != nil {
return fmt.Errorf("invalid input path: %w", err)
}
func validatePath(inputPath string) (string, error) {
cleanPath := filepath.Clean(inputPath)
absPath, err := filepath.Abs(cleanPath)
if err != nil {
return "", err
}
// Check for shell metacharacters
if strings.ContainsAny(absPath, ";|&$`(){}[]<>") {
return "", errors.New("invalid characters in path")
}
return absPath, nil
}
| } | ||
| if ln == "golangci-lint" { | ||
| linter.Args = []string{ | ||
| "export GO111MODULE=auto\n", |
There was a problem hiding this comment.
Code Quality Issue: Hardcoded Magic Strings
These hardcoded shell commands should be extracted to constants or configuration:
linter.Args = []string{
"export GO111MODULE=auto\n",
"go mod tidy\n",
"golangci-lint run --timeout=10m0s --allow-parallel-runners=true --print-issued-lines=false --out-format=line-number\n",
}Recommendations:
- Extract these to named constants for maintainability
- Consider making timeout configurable
- Add validation for these command strings
Example:
const (
DefaultGolangCITimeout = "10m0s"
DefaultGolangCIArgs = "run --allow-parallel-runners=true --print-issued-lines=false --out-format=line-number"
)
| return o | ||
| } | ||
|
|
||
| func (o cliOptions) Validate() error { |
There was a problem hiding this comment.
Security & Quality Issue: Missing CLI Options Validation
The Validate() method is currently a no-op but should validate CLI options for security:
func (o cliOptions) Validate() error {
return nil // No validation implemented
}Security Concerns:
logDirparameter is not validated and could be exploited for path traversal- Missing input sanitization
Recommendation:
func (o cliOptions) Validate() error {
if o.logDir != "" {
if strings.ContainsAny(o.logDir, ";|&$`") {
return errors.New("invalid characters in log directory")
}
// Ensure directory is within safe bounds
cleanDir := filepath.Clean(o.logDir)
if filepath.IsAbs(cleanDir) && !strings.HasPrefix(cleanDir, "/tmp/") {
return errors.New("log directory must be under /tmp for security")
}
}
return nil
}
| log := util.FromContext(ctx) | ||
| for name, fn := range lint.TotalPullRequestHandlers() { | ||
| // check if linter is installed | ||
| _, err := exec.LookPath(name) |
There was a problem hiding this comment.
🚨 Security Issue: Unsafe Linter Name Lookup
Using exec.LookPath() with unsanitized linter names poses security risks:
_, err := exec.LookPath(name)Issues:
- Linter names could contain path traversal characters
- Could execute unintended binaries from PATH
- No allowlist validation
Recommendation:
// Add validation before PATH lookup
func validateLinterName(name string) error {
// Only allow alphanumeric, dash, and underscore
if !regexp.MustCompile(`^[a-zA-Z0-9_-]+$`).MatchString(name) {
return fmt.Errorf("invalid linter name: %s", name)
}
return nil
}
// In the code:
if err := validateLinterName(name); err != nil {
return err
}
_, err := exec.LookPath(name)
| if o.logDir == "" { | ||
| o.logDir = "/tmp" | ||
| } | ||
| storage, err := storage.NewLocalStorage(o.logDir) |
There was a problem hiding this comment.
Performance & Resource Management Issue
The processForCLI function creates storage instances without ensuring proper cleanup:
s.storage, err = storage.NewLocalStorage(o.logDir)
if err != nil {
return fmt.Errorf("failed to create local storage: %w", err)
}Issues:
- Missing resource cleanup on subsequent errors
- Storage could leak resources if function exits early
- No defer cleanup patterns
Recommendation:
localStorage, err := storage.NewLocalStorage(o.logDir)
if err != nil {
return fmt.Errorf("failed to create local storage: %w", err)
}
s.storage = localStorage
// Add cleanup if storage has Close() method
if closer, ok := s.storage.(io.Closer); ok {
defer func() {
if closeErr := closer.Close(); closeErr != nil {
log.Errorf("failed to close storage: %v", closeErr)
}
}()
}
🔍 Comprehensive Code Review SummaryI've completed a thorough review of PR #514 using specialized agents for code quality, performance, security, and documentation. Here are the key findings: 🚨 Critical Issues (Must Fix)
|
2 participants
output: