feat: 修复评论重复的问题

cmd/opencodereview/config_cmd.go

@@ -8,6 +8,54 @@ import (
 	"strconv"
 )
 
+// Environment variable names that override config file values.
+const (
+	envLLMURL     = "OCR_LLM_URL"
+	envLLMToken   = "OCR_LLM_TOKEN"
+	envLLMModel   = "OCR_LLM_MODEL"
+	envLanguage   = "OCR_LANGUAGE"
+)
+
+func envOrDefault(env, fallback string) string {
+	if v := os.Getenv(env); v != "" {
+		return v
+	}
+	return fallback
+}
+
+// LoadMergedConfig loads config from path, then overrides with environment variables if set.
+// Returns nil, nil if file does not exist and no env vars are set.
+func LoadMergedConfig(path string) (*Config, error) {
+	cfg, err := LoadAppConfig(path)
+	if err != nil {
+		return nil, err
+	}
+
+	hasEnv := os.Getenv(envLLMURL) != "" || os.Getenv(envLLMToken) != "" ||
+		os.Getenv(envLLMModel) != "" || os.Getenv(envLanguage) != ""
+	if !hasEnv && cfg == nil {
+		return nil, nil
+	}
+	if cfg == nil {
+		cfg = &Config{}
+	}
+
+	if v := os.Getenv(envLLMURL); v != "" {
+		cfg.Llm.URL = v
+	}
+	if v := os.Getenv(envLLMToken); v != "" {
+		cfg.Llm.AuthToken = v
+	}
+	if v := os.Getenv(envLLMModel); v != "" {
+		cfg.Llm.Model = v
+	}
+	if v := os.Getenv(envLanguage); v != "" {
+		cfg.Language = v
+	}
+
+	return cfg, nil
+}
+
 // Default config file location: ~/.open-code-review/config.json
 func defaultConfigPath() string {
 	home, err := os.UserHomeDir()

cmd/opencodereview/flags.go

@@ -118,7 +118,7 @@ func parseReviewFlags(args []string) (reviewOptions, error) {
 	a.StringVar(&opts.to, "to", "", "target ref to end diff at (e.g., 'feature-branch')")
 	a.StringVarP(&opts.commit, "commit", "c", "", "single commit hash or tag to review (vs its parent)")
 	a.StringVarP(&opts.outputFormat, "format", "f", "text", "output format: text or json")
-	a.IntVar(&opts.concurrency, "concurrency", 4, "max concurrent file reviews")
+	a.IntVar(&opts.concurrency, "concurrency", 8, "max concurrent file reviews")
 	a.IntVar(&opts.perFileTimeout, "timeout", 10, "per-file timeout in minutes")
 
 	if err := a.Parse(args); err != nil {

cmd/opencodereview/llm_cmd.go

@@ -23,12 +23,12 @@ func runLLM(args []string) error {
 }
 
 func runLLMTest() error {
-	cfg, err := LoadAppConfig(defaultConfigPath())
+	cfg, err := LoadMergedConfig(defaultConfigPath())
 	if err != nil {
 		return fmt.Errorf("load config: %w", err)
 	}
 	if cfg == nil || cfg.Llm.URL == "" || cfg.Llm.AuthToken == "" {
-		return fmt.Errorf("llm.url and llm.auth_token are required in %s", defaultConfigPath())
+		return fmt.Errorf("llm.url and llm.auth_token are required in %s, or set OCR_LLM_URL and OCR_LLM_TOKEN environment variables", defaultConfigPath())
 	}
 
 	task, err := testconnection.LoadDefault()

cmd/opencodereview/main.go

@@ -8,10 +8,13 @@ import (
 	"os"
 	"time"
 
+	"github.com/open-code-review/open-code-review/internal/llm"
 	"github.com/open-code-review/open-code-review/internal/telemetry"
 )
 
 func main() {
+	llm.InitEmbeddedLoader()
+
 	ctx := context.Background()
 	if telemetry.Init(ctx) {
 		defer telemetry.ShutdownWithTimeout(ctx, 5*time.Second)

cmd/opencodereview/review_cmd.go

@@ -9,6 +9,7 @@ import (
 
 	"github.com/open-code-review/open-code-review/internal/agent"
 	"github.com/open-code-review/open-code-review/internal/config/rules"
+	"github.com/open-code-review/open-code-review/internal/diff"
 	"github.com/open-code-review/open-code-review/internal/config/template"
 	"github.com/open-code-review/open-code-review/internal/config/toolsconfig"
 	"github.com/open-code-review/open-code-review/internal/llm"
@@ -57,12 +58,12 @@ func runReview(args []string) error {
 		return fmt.Errorf("resolve repo: %w", err)
 	}
 
-	cfg, err := LoadAppConfig(defaultConfigPath())
+	cfg, err := LoadMergedConfig(defaultConfigPath())
 	if err != nil {
 		return fmt.Errorf("load app config: %w", err)
 	}
 	if cfg == nil || cfg.Llm.URL == "" || cfg.Llm.AuthToken == "" {
-		return fmt.Errorf("llm.url and llm.auth_token are required in $HOME/.open-code-review/config.json")
+		return fmt.Errorf("llm.url and llm.auth_token are required in $HOME/.open-code-review/config.json, or set OCR_LLM_URL and OCR_LLM_TOKEN environment variables")
 	}
 	model := cfg.Llm.Model
 	tpl.ApplyLanguage(cfg.Language)
@@ -112,6 +113,9 @@ func runReview(args []string) error {
 		return fmt.Errorf("review failed: %w", err)
 	}
 
+	// Resolve line numbers by matching existing_code against diff hunks.
+	comments = diff.ResolveLineNumbers(comments, ag.Diffs())
+
 	// Record summary metrics (files_reviewed is refined by agent.Run).
 	duration := time.Since(startTime)
 	telemetry.RecordReviewDuration(ctx, duration)

internal/agent/agent.go

@@ -210,6 +210,11 @@ func (a *Agent) FilesReviewed() int64 {
 	return int64(len(a.diffs))
 }
 
+// Diffs returns the parsed diffs loaded by the agent.
+func (a *Agent) Diffs() []model.Diff {
+	return a.diffs
+}
+
 // TotalTokensUsed returns the accumulated total tokens from all LLM calls.
 func (a *Agent) TotalTokensUsed() int64 {
 	return atomic.LoadInt64(&a.totalTokensUsed)
@@ -265,8 +270,6 @@ func (a *Agent) dispatchSubtasks(ctx context.Context) ([]model.LlmComment, error
 	}()
 
 	var wg sync.WaitGroup
-	var mu sync.Mutex
-	var allComments []model.LlmComment
 
 	concurrency := a.args.MaxConcurrency
 	if concurrency <= 0 {
@@ -293,24 +296,25 @@ func (a *Agent) dispatchSubtasks(ctx context.Context) ([]model.LlmComment, error
 				fileCtx = ctx
 			}
 
-			fileComments, err := a.executeSubtask(fileCtx, d)
-			if err != nil {
+			if err := a.executeSubtask(fileCtx, d); err != nil {
 				fmt.Printf("[ocr] Subtask error for %s: %v\n", d.NewPath, err)
 				telemetry.ErrorEvent(fileCtx, "subtask.error", err,
 					telemetry.AnyToAttr("file.path", d.NewPath))
 			}
-			mu.Lock()
-			allComments = append(allComments, fileComments...)
-			mu.Unlock()
 		}(a.diffs[i])
 	}
 
 	wg.Wait()
-	return allComments, nil
+
+	// All subtasks finished — collect comments from the global collector once.
+	if a.args.CommentWorkerPool != nil {
+		a.args.CommentWorkerPool.Await()
+	}
+	return a.args.CommentCollector.Comments(), nil
 }
 
 // executeSubtask performs the Plan Phase + Main Loop for a single file.
-func (a *Agent) executeSubtask(ctx context.Context, d model.Diff) ([]model.LlmComment, error) {
+func (a *Agent) executeSubtask(ctx context.Context, d model.Diff) error {
 	ctx, span := telemetry.StartSpan(ctx, "subtask.execute."+d.NewPath)
 	defer span.End()
 	telemetry.SetAttr(span, "file.path", d.NewPath)
@@ -319,7 +323,7 @@ func (a *Agent) executeSubtask(ctx context.Context, d model.Diff) ([]model.LlmCo
 	telemetry.SetAttr(span, "lines.deleted", d.Deletions)
 
 	if ctx.Err() != nil {
-		return nil, ctx.Err()
+		return ctx.Err()
 	}
 
 	newPath := d.NewPath
@@ -353,7 +357,7 @@ func (a *Agent) executeSubtask(ctx context.Context, d model.Diff) ([]model.LlmCo
 
 	// Phase 2: Main task loop
 	if len(a.args.Template.MainTask.Messages) == 0 {
-		return nil, fmt.Errorf("main_task.messages is empty in template")
+		return fmt.Errorf("main_task.messages is empty in template")
 	}
 
 	rawMsgs := a.args.Template.MainTask.Messages
@@ -381,7 +385,7 @@ func (a *Agent) executeSubtask(ctx context.Context, d model.Diff) ([]model.LlmCo
 			telemetry.AnyToAttr("file.path", newPath),
 			telemetry.AnyToAttr("tokens", tokenCount),
 			telemetry.AnyToAttr("threshold", a.args.Template.TokenWarningThreshold))
-		return nil, nil
+		return nil
 	}
 
 	return a.performLlmCodeReview(ctx, messages, newPath)
@@ -499,13 +503,13 @@ func formatToolDefs(toolDefs []llm.ToolDef) string {
 // performLlmCodeReview drives the main LLM conversation loop for a single file.
 // It sends messages with tool definitions, handles tool calls returned by the model,
 // and collects review comments until task_done is called or limits are reached.
-func (a *Agent) performLlmCodeReview(ctx context.Context, messages []llm.Message, newPath string) ([]model.LlmComment, error) {
+func (a *Agent) performLlmCodeReview(ctx context.Context, messages []llm.Message, newPath string) error {
 	toolReqCount := a.args.Template.MaxToolRequestTimes
 
 	for toolReqCount > 0 {
 		select {
 		case <-ctx.Done():
-			return diff.ResolveLineNumbers(a.collectPendingComments(), a.diffs), ctx.Err()
+			return ctx.Err()
 		default:
 		}
 
@@ -524,7 +528,7 @@ func (a *Agent) performLlmCodeReview(ctx context.Context, messages []llm.Message
 		if err != nil {
 			rec.SetError(err, duration)
 			telemetry.RecordLLMRequest(ctx, a.args.Model, duration, 0, "error")
-			return diff.ResolveLineNumbers(a.collectPendingComments(), a.diffs), fmt.Errorf("LLM completion error: %w", err)
+			return fmt.Errorf("LLM completion error: %w", err)
 		}
 		rec.SetResponse(resp, duration)
 		// Record LLM metrics with token info from API response usage field.
@@ -596,8 +600,7 @@ func (a *Agent) performLlmCodeReview(ctx context.Context, messages []llm.Message
 		fmt.Printf("[ocr] Max tool requests reached for %s.\n", newPath)
 	}
 
-	comments := a.args.CommentCollector.Comments()
-	return diff.ResolveLineNumbers(comments, a.diffs), nil
+	return nil
 }
 
 // executeToolCall executes a single tool call from the LLM response and records

internal/llm/client.go

@@ -11,6 +11,7 @@ import (
 	"io"
 	"math/rand"
 	"net/http"
+	"os"
 	"strings"
 	"sync"
 	"time"
@@ -274,8 +275,8 @@ func SetModelEncoding(modelName string) error {
 func ensureTokenizer() {
 	tokenizerOnce.Do(func() {
 		if err := SetModelEncoding(""); err != nil {
-			// Fallback should not happen as cl100k_base is built-in
-			panic(err)
+			// Network unavailable or encoding load failed — fall back to byte estimation in CountTokens.
+			fmt.Fprintf(os.Stderr, "[ocr] WARNING: tiktoken initialization failed (%v), using byte-based estimation\n", err)
 		}
 	})
 }

internal/llm/embedded_loader.go

@@ -0,0 +1,75 @@
+package llm
+
+import (
+	"embed"
+	"encoding/base64"
+	"fmt"
+	"strconv"
+	"strings"
+
+	tiktoken "github.com/pkoukk/tiktoken-go"
+)
+
+//go:embed embed/*.tiktoken
+var embedFS embed.FS
+
+const embedPrefix = "embed/"
+
+// initEmbeddedLoader configures tiktoken to use embedded BPE data instead of fetching from network.
+// Call this once during application startup, before any GetEncoding/EncodingForModel calls.
+func InitEmbeddedLoader() {
+	loader := &embeddedBpeLoader{}
+	tiktoken.SetBpeLoader(loader)
+}
+
+// embeddedBpeLoader implements tiktoken.BpeLoader interface.
+// It maps known encoding URLs to local embedded files, eliminating network dependency.
+type embeddedBpeLoader struct{}
+
+var urlToFileMap = map[string]string{
+	"https://openaipublic.blob.core.windows.net/encodings/cl100k_base.tiktoken": "cl100k_base.tiktoken",
+	"https://openaipublic.blob.core.windows.net/encodings/o200k_base.tiktoken":  "o200k_base.tiktoken",
+	"https://openaipublic.blob.core.windows.net/encodings/p50k_base.tiktoken":   "p50k_base.tiktoken",
+	"https://openaipublic.blob.core.windows.net/encodings/r50k_base.tiktoken":   "r50k_base.tiktoken",
+}
+
+func (l *embeddedBpeLoader) LoadTiktokenBpe(tiktokenBpeFile string) (map[string]int, error) {
+	if localName, ok := urlToFileMap[tiktokenBpeFile]; ok {
+		return loadFromEmbed(localName)
+	}
+	return nil, fmt.Errorf("tiktoken encoding file %q is not embedded and cannot be fetched offline", tiktokenBpeFile)
+}
+
+func loadFromEmbed(filename string) (map[string]int, error) {
+	data, err := embedFS.ReadFile(embedPrefix + filename)
+	if err != nil {
+		return nil, fmt.Errorf("embedded tiktoken file %q not found: %w", filename, err)
+	}
+	return parseBpeData(data)
+}
+
+// parseBpeData parses the base64-encoded BPE data format.
+// Each line: <base64-token> <rank>
+func parseBpeData(data []byte) (map[string]int, error) {
+	bpeRanks := make(map[string]int)
+	for _, line := range strings.Split(string(data), "\n") {
+		line = strings.TrimSpace(line)
+		if line == "" {
+			continue
+		}
+		parts := strings.SplitN(line, " ", 2)
+		if len(parts) != 2 {
+			return nil, fmt.Errorf("invalid bpe data line: %q", line)
+		}
+		token, err := base64.StdEncoding.DecodeString(parts[0])
+		if err != nil {
+			return nil, fmt.Errorf("failed to decode token %q: %w", line, err)
+		}
+		rank, err := strconv.Atoi(parts[1])
+		if err != nil {
+			return nil, fmt.Errorf("invalid rank in line %q: %w", line, err)
+		}
+		bpeRanks[string(token)] = rank
+	}
+	return bpeRanks, nil
+}

internal/tool/comment_collector.go

@@ -21,11 +21,11 @@ func NewCommentCollector() *CommentCollector {
 // Add appends a comment to the collector.
 func (c *CommentCollector) Add(cm model.LlmComment) {
 	c.mu.Lock()
+	defer c.mu.Unlock()
 	c.comments = append(c.comments, cm)
-	c.mu.Unlock()
 }
 
-// Comments returns a copy of all collected comments.
+// Comments returns all collected comments.
 func (c *CommentCollector) Comments() []model.LlmComment {
 	c.mu.Lock()
 	defer c.mu.Unlock()