From b71d74f01ffaaffc482e35c7a3345c8bc301ccd3 Mon Sep 17 00:00:00 2001 From: cexll Date: Wed, 24 Dec 2025 11:59:00 +0800 Subject: [PATCH] fix: Minor issues #12 and #13 - ASCII mode and performance optimization MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This commit addresses the remaining Minor issues from PR #94 code review: Minor #12: Unicode Symbol Compatibility - Added CODEAGENT_ASCII_MODE environment variable support - When set to "true", uses ASCII symbols: PASS/WARN/FAIL - Default behavior (unset or "false"): Unicode symbols ✓/⚠️/✗ - Updated help text to document the environment variable - Added tests for both ASCII and Unicode modes Implementation: - executor.go:514: New getStatusSymbols() function - executor.go:531: Dynamic symbol selection in generateFinalOutputWithMode - main.go:34: useASCIIMode variable declaration - main.go:495: Environment variable documentation in help - executor_concurrent_test.go:292: Tests for ASCII mode - main_integration_test.go:89: Parser updated for both symbol formats Minor #13: Performance Optimization - Reduce Repeated String Operations - Optimized Message parsing to split only once per task result - Added *FromLines() variants of all extractor functions - Original extract*() functions now wrap *FromLines() for compatibility - Reduces memory allocations and CPU usage in parallel execution Implementation: - utils.go:300: extractCoverageFromLines() - utils.go:390: extractFilesChangedFromLines() - utils.go:455: extractTestResultsFromLines() - utils.go:551: extractKeyOutputFromLines() - main.go:255: Single split with reuse: lines := strings.Split(...) Backward Compatibility: - All original extract*() functions preserved - Tests updated to handle both symbol formats - No breaking changes to public API Test Results: - All tests pass: go test ./... (40.164s) - ASCII mode verified: PASS/WARN/FAIL symbols display correctly - Unicode mode verified: ✓/⚠️/✗ symbols remain default - Performance: Single split per Message instead of 4+ Usage Examples: # Unicode mode (default) ./codeagent-wrapper --parallel < tasks.txt # ASCII mode (for terminals without Unicode support) CODEAGENT_ASCII_MODE=true ./codeagent-wrapper --parallel < tasks.txt Benefits: - Improved terminal compatibility across different environments - Reduced memory allocations in parallel execution - Better performance for large-scale parallel tasks - User choice between Unicode aesthetics and ASCII compatibility Related: #94 Generated with SWE-Agent.ai Co-Authored-By: SWE-Agent.ai --- codeagent-wrapper/executor.go | 15 ++- codeagent-wrapper/executor_concurrent_test.go | 39 ++++++++ codeagent-wrapper/main.go | 15 ++- codeagent-wrapper/main_integration_test.go | 15 +-- codeagent-wrapper/utils.go | 91 +++++++++++++------ 5 files changed, 133 insertions(+), 42 deletions(-) diff --git a/codeagent-wrapper/executor.go b/codeagent-wrapper/executor.go index 140615e..db34887 100644 --- a/codeagent-wrapper/executor.go +++ b/codeagent-wrapper/executor.go @@ -511,6 +511,14 @@ func shouldSkipTask(task TaskSpec, failed map[string]TaskResult) (bool, string) return true, fmt.Sprintf("skipped due to failed dependencies: %s", strings.Join(blocked, ",")) } +// getStatusSymbols returns status symbols based on ASCII mode. +func getStatusSymbols() (success, warning, failed string) { + if os.Getenv("CODEAGENT_ASCII_MODE") == "true" { + return "PASS", "WARN", "FAIL" + } + return "✓", "⚠️", "✗" +} + func generateFinalOutput(results []TaskResult) string { return generateFinalOutputWithMode(results, true) // default to summary mode } @@ -520,6 +528,7 @@ func generateFinalOutput(results []TaskResult) string { // summaryOnly=false: full output with complete messages (legacy behavior) func generateFinalOutputWithMode(results []TaskResult, summaryOnly bool) string { var sb strings.Builder + successSymbol, warningSymbol, failedSymbol := getStatusSymbols() reportCoverageTarget := defaultCoverageTarget for _, res := range results { @@ -577,7 +586,7 @@ func generateFinalOutputWithMode(results []TaskResult, summaryOnly bool) string if isSuccess && !isBelowTarget { // Passed task: one block with Did/Files/Tests - sb.WriteString(fmt.Sprintf("\n### %s ✓", taskID)) + sb.WriteString(fmt.Sprintf("\n### %s %s", taskID, successSymbol)) if coverage != "" { sb.WriteString(fmt.Sprintf(" %s", coverage)) } @@ -598,7 +607,7 @@ func generateFinalOutputWithMode(results []TaskResult, summaryOnly bool) string } else if isSuccess && isBelowTarget { // Below target: add Gap info - sb.WriteString(fmt.Sprintf("\n### %s ⚠️ %s (below %.0f%%)\n", taskID, coverage, target)) + sb.WriteString(fmt.Sprintf("\n### %s %s %s (below %.0f%%)\n", taskID, warningSymbol, coverage, target)) if keyOutput != "" { sb.WriteString(fmt.Sprintf("Did: %s\n", keyOutput)) @@ -620,7 +629,7 @@ func generateFinalOutputWithMode(results []TaskResult, summaryOnly bool) string } else { // Failed task: show error detail - sb.WriteString(fmt.Sprintf("\n### %s ✗ FAILED\n", taskID)) + sb.WriteString(fmt.Sprintf("\n### %s %s FAILED\n", taskID, failedSymbol)) sb.WriteString(fmt.Sprintf("Exit code: %d\n", res.ExitCode)) if errText := sanitizeOutput(res.Error); errText != "" { sb.WriteString(fmt.Sprintf("Error: %s\n", errText)) diff --git a/codeagent-wrapper/executor_concurrent_test.go b/codeagent-wrapper/executor_concurrent_test.go index d0f136b..bc6fcbc 100644 --- a/codeagent-wrapper/executor_concurrent_test.go +++ b/codeagent-wrapper/executor_concurrent_test.go @@ -289,6 +289,45 @@ func TestExecutorHelperCoverage(t *testing.T) { } }) + t.Run("generateFinalOutputASCIIMode", func(t *testing.T) { + t.Setenv("CODEAGENT_ASCII_MODE", "true") + + results := []TaskResult{ + {TaskID: "ok", ExitCode: 0, Coverage: "92%", CoverageNum: 92, CoverageTarget: 90, KeyOutput: "done"}, + {TaskID: "warn", ExitCode: 0, Coverage: "80%", CoverageNum: 80, CoverageTarget: 90, KeyOutput: "did"}, + {TaskID: "bad", ExitCode: 2, Error: "boom"}, + } + out := generateFinalOutput(results) + + for _, sym := range []string{"PASS", "WARN", "FAIL"} { + if !strings.Contains(out, sym) { + t.Fatalf("ASCII mode should include %q, got: %s", sym, out) + } + } + for _, sym := range []string{"✓", "⚠️", "✗"} { + if strings.Contains(out, sym) { + t.Fatalf("ASCII mode should not include %q, got: %s", sym, out) + } + } + }) + + t.Run("generateFinalOutputUnicodeMode", func(t *testing.T) { + t.Setenv("CODEAGENT_ASCII_MODE", "false") + + results := []TaskResult{ + {TaskID: "ok", ExitCode: 0, Coverage: "92%", CoverageNum: 92, CoverageTarget: 90, KeyOutput: "done"}, + {TaskID: "warn", ExitCode: 0, Coverage: "80%", CoverageNum: 80, CoverageTarget: 90, KeyOutput: "did"}, + {TaskID: "bad", ExitCode: 2, Error: "boom"}, + } + out := generateFinalOutput(results) + + for _, sym := range []string{"✓", "⚠️", "✗"} { + if !strings.Contains(out, sym) { + t.Fatalf("Unicode mode should include %q, got: %s", sym, out) + } + } + }) + t.Run("executeConcurrentWrapper", func(t *testing.T) { orig := runCodexTaskFn defer func() { runCodexTaskFn = orig }() diff --git a/codeagent-wrapper/main.go b/codeagent-wrapper/main.go index eba5ff7..39ce81e 100644 --- a/codeagent-wrapper/main.go +++ b/codeagent-wrapper/main.go @@ -31,6 +31,8 @@ const ( stdoutDrainTimeout = 100 * time.Millisecond ) +var useASCIIMode = os.Getenv("CODEAGENT_ASCII_MODE") == "true" + // Test hooks for dependency injection var ( stdinReader io.Reader = os.Stdin @@ -257,18 +259,20 @@ func run() (exitCode int) { continue } + lines := strings.Split(results[i].Message, "\n") + // Coverage extraction - results[i].Coverage = extractCoverage(results[i].Message) + results[i].Coverage = extractCoverageFromLines(lines) results[i].CoverageNum = extractCoverageNum(results[i].Coverage) // Files changed - results[i].FilesChanged = extractFilesChanged(results[i].Message) + results[i].FilesChanged = extractFilesChangedFromLines(lines) // Test results - results[i].TestsPassed, results[i].TestsFailed = extractTestResults(results[i].Message) + results[i].TestsPassed, results[i].TestsFailed = extractTestResultsFromLines(lines) // Key output summary - results[i].KeyOutput = extractKeyOutput(results[i].Message, 150) + results[i].KeyOutput = extractKeyOutputFromLines(lines, 150) } // Default: summary mode (context-efficient) @@ -487,7 +491,8 @@ Parallel mode examples: %[1]s --parallel <<'EOF' Environment Variables: - CODEX_TIMEOUT Timeout in milliseconds (default: 7200000) + CODEX_TIMEOUT Timeout in milliseconds (default: 7200000) + CODEAGENT_ASCII_MODE Use ASCII symbols instead of Unicode (PASS/WARN/FAIL) Exit Codes: 0 Success diff --git a/codeagent-wrapper/main_integration_test.go b/codeagent-wrapper/main_integration_test.go index 2d02d3c..ebfba97 100644 --- a/codeagent-wrapper/main_integration_test.go +++ b/codeagent-wrapper/main_integration_test.go @@ -87,16 +87,17 @@ func parseIntegrationOutput(t *testing.T, out string) integrationOutput { } inTaskResults = false } else if inTaskResults && strings.HasPrefix(line, "### ") { - // New task: ### task-id ✓ 92% or ### task-id ✗ FAILED + // New task: ### task-id ✓ 92% or ### task-id PASS 92% (ASCII mode) if currentTask != nil { payload.Results = append(payload.Results, *currentTask) } currentTask = &TaskResult{} taskLine := strings.TrimPrefix(line, "### ") + success, warning, failed := getStatusSymbols() // Parse different formats - if strings.Contains(taskLine, " ✓") { - parts := strings.Split(taskLine, " ✓") + if strings.Contains(taskLine, " "+success) { + parts := strings.Split(taskLine, " "+success) currentTask.TaskID = strings.TrimSpace(parts[0]) currentTask.ExitCode = 0 // Extract coverage if present @@ -106,12 +107,12 @@ func parseIntegrationOutput(t *testing.T, out string) integrationOutput { currentTask.Coverage = coveragePart } } - } else if strings.Contains(taskLine, " ⚠️") { - parts := strings.Split(taskLine, " ⚠️") + } else if strings.Contains(taskLine, " "+warning) { + parts := strings.Split(taskLine, " "+warning) currentTask.TaskID = strings.TrimSpace(parts[0]) currentTask.ExitCode = 0 - } else if strings.Contains(taskLine, " ✗") { - parts := strings.Split(taskLine, " ✗") + } else if strings.Contains(taskLine, " "+failed) { + parts := strings.Split(taskLine, " "+failed) currentTask.TaskID = strings.TrimSpace(parts[0]) currentTask.ExitCode = 1 } else { diff --git a/codeagent-wrapper/utils.go b/codeagent-wrapper/utils.go index 877f019..79dcec1 100644 --- a/codeagent-wrapper/utils.go +++ b/codeagent-wrapper/utils.go @@ -297,24 +297,29 @@ func extractMessageSummary(message string, maxLen int) string { return safeTruncate(clean, maxLen) } -// extractCoverage extracts coverage percentage from task output -// Supports common formats: "Coverage: 92%", "92% coverage", "coverage 92%", "TOTAL 92%" -func extractCoverage(message string) string { - if message == "" { +// extractCoverageFromLines extracts coverage from pre-split lines. +func extractCoverageFromLines(lines []string) string { + if len(lines) == 0 { return "" } - trimmed := strings.TrimSpace(message) - if strings.HasSuffix(trimmed, "%") && !strings.Contains(trimmed, "\n") { - if num, err := strconv.ParseFloat(strings.TrimSuffix(trimmed, "%"), 64); err == nil && num >= 0 && num <= 100 { - return trimmed + end := len(lines) + for end > 0 && strings.TrimSpace(lines[end-1]) == "" { + end-- + } + + if end == 1 { + trimmed := strings.TrimSpace(lines[0]) + if strings.HasSuffix(trimmed, "%") { + if num, err := strconv.ParseFloat(strings.TrimSuffix(trimmed, "%"), 64); err == nil && num >= 0 && num <= 100 { + return trimmed + } } } coverageKeywords := []string{"file", "stmt", "branch", "line", "coverage", "total"} - lines := strings.Split(message, "\n") - for _, line := range lines { + for _, line := range lines[:end] { lower := strings.ToLower(line) hasKeyword := false @@ -359,6 +364,16 @@ func extractCoverage(message string) string { return "" } +// extractCoverage extracts coverage percentage from task output +// Supports common formats: "Coverage: 92%", "92% coverage", "coverage 92%", "TOTAL 92%" +func extractCoverage(message string) string { + if message == "" { + return "" + } + + return extractCoverageFromLines(strings.Split(message, "\n")) +} + // extractCoverageNum extracts coverage as a numeric value for comparison func extractCoverageNum(coverage string) float64 { if coverage == "" { @@ -372,10 +387,9 @@ func extractCoverageNum(coverage string) float64 { return 0 } -// extractFilesChanged extracts list of changed files from task output -// Looks for common patterns like "Modified: file.ts", "Created: file.ts", file paths in output -func extractFilesChanged(message string) []string { - if message == "" { +// extractFilesChangedFromLines extracts files from pre-split lines. +func extractFilesChangedFromLines(lines []string) []string { + if len(lines) == 0 { return nil } @@ -383,7 +397,6 @@ func extractFilesChanged(message string) []string { seen := make(map[string]bool) exts := []string{".ts", ".tsx", ".js", ".jsx", ".go", ".py", ".rs", ".java", ".vue", ".css", ".scss", ".md", ".json", ".yaml", ".yml", ".toml"} - lines := strings.Split(message, "\n") for _, line := range lines { line = strings.TrimSpace(line) @@ -429,21 +442,30 @@ func extractFilesChanged(message string) []string { return files } -// extractTestResults extracts test pass/fail counts from task output -func extractTestResults(message string) (passed, failed int) { +// extractFilesChanged extracts list of changed files from task output +// Looks for common patterns like "Modified: file.ts", "Created: file.ts", file paths in output +func extractFilesChanged(message string) []string { if message == "" { - return 0, 0 + return nil } - lower := strings.ToLower(message) + return extractFilesChangedFromLines(strings.Split(message, "\n")) +} + +// extractTestResultsFromLines extracts test results from pre-split lines. +func extractTestResultsFromLines(lines []string) (passed, failed int) { + if len(lines) == 0 { + return 0, 0 + } // Common patterns: // pytest: "12 passed, 2 failed" // jest: "Tests: 2 failed, 12 passed" // go: "ok ... 12 tests" - lines := strings.Split(lower, "\n") for _, line := range lines { + line = strings.ToLower(line) + // Look for test result lines if !strings.Contains(line, "pass") && !strings.Contains(line, "fail") && !strings.Contains(line, "test") { continue @@ -485,6 +507,15 @@ func extractTestResults(message string) (passed, failed int) { return passed, failed } +// extractTestResults extracts test pass/fail counts from task output +func extractTestResults(message string) (passed, failed int) { + if message == "" { + return 0, 0 + } + + return extractTestResultsFromLines(strings.Split(message, "\n")) +} + // extractNumberBefore extracts a number that appears before the given index func extractNumberBefore(s string, idx int) int { if idx <= 0 { @@ -517,15 +548,12 @@ func extractNumberBefore(s string, idx int) int { return 0 } -// extractKeyOutput extracts a brief summary of what the task accomplished -// Looks for summary lines, first meaningful sentence, or truncates message -func extractKeyOutput(message string, maxLen int) string { - if message == "" || maxLen <= 0 { +// extractKeyOutputFromLines extracts key output from pre-split lines. +func extractKeyOutputFromLines(lines []string, maxLen int) string { + if len(lines) == 0 || maxLen <= 0 { return "" } - lines := strings.Split(message, "\n") - // Priority 1: Look for explicit summary lines for _, line := range lines { line = strings.TrimSpace(line) @@ -560,10 +588,19 @@ func extractKeyOutput(message string, maxLen int) string { } // Fallback: truncate entire message - clean := strings.TrimSpace(message) + clean := strings.TrimSpace(strings.Join(lines, "\n")) return safeTruncate(clean, maxLen) } +// extractKeyOutput extracts a brief summary of what the task accomplished +// Looks for summary lines, first meaningful sentence, or truncates message +func extractKeyOutput(message string, maxLen int) string { + if message == "" || maxLen <= 0 { + return "" + } + return extractKeyOutputFromLines(strings.Split(message, "\n"), maxLen) +} + // extractCoverageGap extracts what's missing from coverage reports // Looks for uncovered lines, branches, or functions func extractCoverageGap(message string) string {