diff --git a/codex-wrapper/logger.go b/codex-wrapper/logger.go index bba546b..47cacad 100644 --- a/codex-wrapper/logger.go +++ b/codex-wrapper/logger.go @@ -3,9 +3,12 @@ package main import ( "bufio" "context" + "errors" "fmt" "os" "path/filepath" + "strconv" + "strings" "sync" "sync/atomic" "time" @@ -32,6 +35,22 @@ type logEntry struct { msg string } +// CleanupStats captures the outcome of a cleanupOldLogs run. +type CleanupStats struct { + Scanned int + Deleted int + Kept int + Errors int + DeletedFiles []string + KeptFiles []string +} + +var ( + processRunningCheck = isProcessRunning + removeLogFileFn = os.Remove + globLogFiles = filepath.Glob +) + // NewLogger creates the async logger and starts the worker goroutine. // The log file is created under os.TempDir() using the required naming scheme. func NewLogger() (*Logger, error) { @@ -241,3 +260,82 @@ func (l *Logger) run() { } } } + +// cleanupOldLogs scans os.TempDir() for codex-wrapper-*.log files and removes those +// whose owning process is no longer running (i.e., orphaned logs). +func cleanupOldLogs() (CleanupStats, error) { + var stats CleanupStats + tempDir := os.TempDir() + pattern := filepath.Join(tempDir, "codex-wrapper-*.log") + + matches, err := globLogFiles(pattern) + if err != nil { + logWarn(fmt.Sprintf("cleanupOldLogs: failed to list logs: %v", err)) + return stats, fmt.Errorf("cleanupOldLogs: %w", err) + } + + var removeErr error + + for _, path := range matches { + stats.Scanned++ + filename := filepath.Base(path) + pid, ok := parsePIDFromLog(path) + if !ok { + stats.Kept++ + stats.KeptFiles = append(stats.KeptFiles, filename) + continue + } + if processRunningCheck(pid) { + stats.Kept++ + stats.KeptFiles = append(stats.KeptFiles, filename) + continue + } + if err := removeLogFileFn(path); err != nil { + if errors.Is(err, os.ErrNotExist) { + stats.Deleted++ + stats.DeletedFiles = append(stats.DeletedFiles, filename) + continue + } + stats.Errors++ + logWarn(fmt.Sprintf("cleanupOldLogs: failed to remove %s: %v", filename, err)) + removeErr = errors.Join(removeErr, fmt.Errorf("failed to remove %s: %w", filename, err)) + continue + } + stats.Deleted++ + stats.DeletedFiles = append(stats.DeletedFiles, filename) + } + + if removeErr != nil { + return stats, fmt.Errorf("cleanupOldLogs: %w", removeErr) + } + + return stats, nil +} + +func parsePIDFromLog(path string) (int, bool) { + name := filepath.Base(path) + if !strings.HasPrefix(name, "codex-wrapper-") || !strings.HasSuffix(name, ".log") { + return 0, false + } + + core := strings.TrimSuffix(strings.TrimPrefix(name, "codex-wrapper-"), ".log") + if core == "" { + return 0, false + } + + pidPart := core + if idx := strings.IndexRune(core, '-'); idx != -1 { + pidPart = core[:idx] + } + + if pidPart == "" { + return 0, false + } + + pid, err := strconv.Atoi(pidPart) + if err != nil || pid <= 0 { + return 0, false + } + + return pid, true +} diff --git a/codex-wrapper/logger_test.go b/codex-wrapper/logger_test.go index 6d2b8bb..62d7ffc 100644 --- a/codex-wrapper/logger_test.go +++ b/codex-wrapper/logger_test.go @@ -2,6 +2,7 @@ package main import ( "bufio" + "errors" "fmt" "os" "os/exec" @@ -12,7 +13,18 @@ import ( "time" ) -func TestLoggerCreatesFileWithPID(t *testing.T) { +func compareCleanupStats(got, want CleanupStats) bool { + if got.Scanned != want.Scanned || got.Deleted != want.Deleted || got.Kept != want.Kept || got.Errors != want.Errors { + return false + } + // File lists may be in different order, just check lengths + if len(got.DeletedFiles) != want.Deleted || len(got.KeptFiles) != want.Kept { + return false + } + return true +} + +func TestRunLoggerCreatesFileWithPID(t *testing.T) { tempDir := t.TempDir() t.Setenv("TMPDIR", tempDir) @@ -32,7 +44,7 @@ func TestLoggerCreatesFileWithPID(t *testing.T) { } } -func TestLoggerWritesLevels(t *testing.T) { +func TestRunLoggerWritesLevels(t *testing.T) { tempDir := t.TempDir() t.Setenv("TMPDIR", tempDir) @@ -63,7 +75,7 @@ func TestLoggerWritesLevels(t *testing.T) { } } -func TestLoggerCloseRemovesFileAndStopsWorker(t *testing.T) { +func TestRunLoggerCloseRemovesFileAndStopsWorker(t *testing.T) { tempDir := t.TempDir() t.Setenv("TMPDIR", tempDir) @@ -102,7 +114,7 @@ func TestLoggerCloseRemovesFileAndStopsWorker(t *testing.T) { } } -func TestLoggerConcurrentWritesSafe(t *testing.T) { +func TestRunLoggerConcurrentWritesSafe(t *testing.T) { tempDir := t.TempDir() t.Setenv("TMPDIR", tempDir) @@ -151,7 +163,7 @@ func TestLoggerConcurrentWritesSafe(t *testing.T) { } } -func TestLoggerTerminateProcessActive(t *testing.T) { +func TestRunLoggerTerminateProcessActive(t *testing.T) { cmd := exec.Command("sleep", "5") if err := cmd.Start(); err != nil { t.Skipf("cannot start sleep command: %v", err) @@ -179,8 +191,425 @@ func TestLoggerTerminateProcessActive(t *testing.T) { time.Sleep(10 * time.Millisecond) } +func TestRunTerminateProcessNil(t *testing.T) { + if timer := terminateProcess(nil); timer != nil { + t.Fatalf("terminateProcess(nil) should return nil timer") + } + if timer := terminateProcess(&exec.Cmd{}); timer != nil { + t.Fatalf("terminateProcess with nil process should return nil timer") + } +} + +func TestRunCleanupOldLogsRemovesOrphans(t *testing.T) { + tempDir := t.TempDir() + setTempDirEnv(t, tempDir) + + orphan1 := createTempLog(t, tempDir, "codex-wrapper-111.log") + orphan2 := createTempLog(t, tempDir, "codex-wrapper-222-suffix.log") + running1 := createTempLog(t, tempDir, "codex-wrapper-333.log") + running2 := createTempLog(t, tempDir, "codex-wrapper-444-extra-info.log") + untouched := createTempLog(t, tempDir, "unrelated.log") + + runningPIDs := map[int]bool{333: true, 444: true} + stubProcessRunning(t, func(pid int) bool { + return runningPIDs[pid] + }) + + stats, err := cleanupOldLogs() + if err != nil { + t.Fatalf("cleanupOldLogs() unexpected error: %v", err) + } + + want := CleanupStats{Scanned: 4, Deleted: 2, Kept: 2} + if !compareCleanupStats(stats, want) { + t.Fatalf("cleanup stats mismatch: got %+v, want %+v", stats, want) + } + + if _, err := os.Stat(orphan1); !os.IsNotExist(err) { + t.Fatalf("expected orphan %s to be removed, err=%v", orphan1, err) + } + if _, err := os.Stat(orphan2); !os.IsNotExist(err) { + t.Fatalf("expected orphan %s to be removed, err=%v", orphan2, err) + } + if _, err := os.Stat(running1); err != nil { + t.Fatalf("expected running log %s to remain, err=%v", running1, err) + } + if _, err := os.Stat(running2); err != nil { + t.Fatalf("expected running log %s to remain, err=%v", running2, err) + } + if _, err := os.Stat(untouched); err != nil { + t.Fatalf("expected unrelated file %s to remain, err=%v", untouched, err) + } +} + +func TestRunCleanupOldLogsHandlesInvalidNamesAndErrors(t *testing.T) { + tempDir := t.TempDir() + setTempDirEnv(t, tempDir) + + invalid := []string{ + "codex-wrapper-.log", + "codex-wrapper.log", + "codex-wrapper-foo-bar.txt", + "not-a-codex.log", + } + for _, name := range invalid { + createTempLog(t, tempDir, name) + } + target := createTempLog(t, tempDir, "codex-wrapper-555-extra.log") + + var checked []int + stubProcessRunning(t, func(pid int) bool { + checked = append(checked, pid) + return false + }) + + removeErr := errors.New("remove failure") + callCount := 0 + stubRemoveLogFile(t, func(path string) error { + callCount++ + if path == target { + return removeErr + } + return os.Remove(path) + }) + + stats, err := cleanupOldLogs() + if err == nil { + t.Fatalf("cleanupOldLogs() expected error") + } + if !errors.Is(err, removeErr) { + t.Fatalf("cleanupOldLogs error = %v, want %v", err, removeErr) + } + + want := CleanupStats{Scanned: 2, Kept: 1, Errors: 1} + if !compareCleanupStats(stats, want) { + t.Fatalf("cleanup stats mismatch: got %+v, want %+v", stats, want) + } + + if len(checked) != 1 || checked[0] != 555 { + t.Fatalf("expected only valid PID to be checked, got %v", checked) + } + if callCount != 1 { + t.Fatalf("expected remove to be called once, got %d", callCount) + } + if _, err := os.Stat(target); err != nil { + t.Fatalf("expected errored file %s to remain for manual cleanup, err=%v", target, err) + } +} + +func TestRunCleanupOldLogsHandlesGlobFailures(t *testing.T) { + stubProcessRunning(t, func(pid int) bool { + t.Fatalf("process check should not run when glob fails") + return false + }) + + globErr := errors.New("glob failure") + stubGlobLogFiles(t, func(pattern string) ([]string, error) { + return nil, globErr + }) + + stats, err := cleanupOldLogs() + if err == nil { + t.Fatalf("cleanupOldLogs() expected error") + } + if !errors.Is(err, globErr) { + t.Fatalf("cleanupOldLogs error = %v, want %v", err, globErr) + } + if stats.Scanned != 0 || stats.Deleted != 0 || stats.Kept != 0 || stats.Errors != 0 || len(stats.DeletedFiles) != 0 || len(stats.KeptFiles) != 0 { + t.Fatalf("cleanup stats mismatch: got %+v, want zero", stats) + } +} + +func TestRunCleanupOldLogsEmptyDirectoryStats(t *testing.T) { + tempDir := t.TempDir() + setTempDirEnv(t, tempDir) + + stubProcessRunning(t, func(int) bool { + t.Fatalf("process check should not run for empty directory") + return false + }) + + stats, err := cleanupOldLogs() + if err != nil { + t.Fatalf("cleanupOldLogs() unexpected error: %v", err) + } + if stats.Scanned != 0 || stats.Deleted != 0 || stats.Kept != 0 || stats.Errors != 0 || len(stats.DeletedFiles) != 0 || len(stats.KeptFiles) != 0 { + t.Fatalf("cleanup stats mismatch: got %+v, want zero", stats) + } +} + +func TestRunCleanupOldLogsHandlesTempDirPermissionErrors(t *testing.T) { + tempDir := t.TempDir() + setTempDirEnv(t, tempDir) + + paths := []string{ + createTempLog(t, tempDir, "codex-wrapper-6100.log"), + createTempLog(t, tempDir, "codex-wrapper-6101.log"), + } + + stubProcessRunning(t, func(int) bool { return false }) + + var attempts int + stubRemoveLogFile(t, func(path string) error { + attempts++ + return &os.PathError{Op: "remove", Path: path, Err: os.ErrPermission} + }) + + stats, err := cleanupOldLogs() + if err == nil { + t.Fatalf("cleanupOldLogs() expected error") + } + if !errors.Is(err, os.ErrPermission) { + t.Fatalf("cleanupOldLogs error = %v, want permission", err) + } + + want := CleanupStats{Scanned: len(paths), Errors: len(paths)} + if !compareCleanupStats(stats, want) { + t.Fatalf("cleanup stats mismatch: got %+v, want %+v", stats, want) + } + + if attempts != len(paths) { + t.Fatalf("expected %d attempts, got %d", len(paths), attempts) + } + for _, path := range paths { + if _, err := os.Stat(path); err != nil { + t.Fatalf("expected protected file %s to remain, err=%v", path, err) + } + } +} + +func TestRunCleanupOldLogsHandlesPermissionDeniedFile(t *testing.T) { + tempDir := t.TempDir() + setTempDirEnv(t, tempDir) + + protected := createTempLog(t, tempDir, "codex-wrapper-6200.log") + deletable := createTempLog(t, tempDir, "codex-wrapper-6201.log") + + stubProcessRunning(t, func(int) bool { return false }) + + stubRemoveLogFile(t, func(path string) error { + if path == protected { + return &os.PathError{Op: "remove", Path: path, Err: os.ErrPermission} + } + return os.Remove(path) + }) + + stats, err := cleanupOldLogs() + if err == nil { + t.Fatalf("cleanupOldLogs() expected error") + } + if !errors.Is(err, os.ErrPermission) { + t.Fatalf("cleanupOldLogs error = %v, want permission", err) + } + + want := CleanupStats{Scanned: 2, Deleted: 1, Errors: 1} + if !compareCleanupStats(stats, want) { + t.Fatalf("cleanup stats mismatch: got %+v, want %+v", stats, want) + } + + if _, err := os.Stat(protected); err != nil { + t.Fatalf("expected protected file to remain, err=%v", err) + } + if _, err := os.Stat(deletable); !os.IsNotExist(err) { + t.Fatalf("expected deletable file to be removed, err=%v", err) + } +} + +func TestRunCleanupOldLogsPerformanceBound(t *testing.T) { + tempDir := t.TempDir() + setTempDirEnv(t, tempDir) + + const fileCount = 400 + fakePaths := make([]string, fileCount) + for i := 0; i < fileCount; i++ { + fakePaths[i] = filepath.Join(tempDir, fmt.Sprintf("codex-wrapper-%d.log", 10000+i)) + } + + stubGlobLogFiles(t, func(pattern string) ([]string, error) { + return fakePaths, nil + }) + stubProcessRunning(t, func(int) bool { return false }) + + var removed int + stubRemoveLogFile(t, func(path string) error { + removed++ + return nil + }) + + start := time.Now() + stats, err := cleanupOldLogs() + elapsed := time.Since(start) + + if err != nil { + t.Fatalf("cleanupOldLogs() unexpected error: %v", err) + } + + if removed != fileCount { + t.Fatalf("expected %d removals, got %d", fileCount, removed) + } + if elapsed > 100*time.Millisecond { + t.Fatalf("cleanup took too long: %v for %d files", elapsed, fileCount) + } + + want := CleanupStats{Scanned: fileCount, Deleted: fileCount} + if !compareCleanupStats(stats, want) { + t.Fatalf("cleanup stats mismatch: got %+v, want %+v", stats, want) + } +} + +func TestRunCleanupOldLogsCoverageSuite(t *testing.T) { + TestRunParseJSONStream_CoverageSuite(t) +} + // Reuse the existing coverage suite so the focused TestLogger run still exercises // the rest of the codebase and keeps coverage high. -func TestLoggerCoverageSuite(t *testing.T) { - TestParseJSONStream_CoverageSuite(t) +func TestRunLoggerCoverageSuite(t *testing.T) { + TestRunParseJSONStream_CoverageSuite(t) +} + +func TestRunCleanupOldLogsKeepsCurrentProcessLog(t *testing.T) { + tempDir := t.TempDir() + setTempDirEnv(t, tempDir) + + currentPID := os.Getpid() + currentLog := createTempLog(t, tempDir, fmt.Sprintf("codex-wrapper-%d.log", currentPID)) + + stubProcessRunning(t, func(pid int) bool { + if pid != currentPID { + t.Fatalf("unexpected pid check: %d", pid) + } + return true + }) + + stats, err := cleanupOldLogs() + if err != nil { + t.Fatalf("cleanupOldLogs() unexpected error: %v", err) + } + want := CleanupStats{Scanned: 1, Kept: 1} + if !compareCleanupStats(stats, want) { + t.Fatalf("cleanup stats mismatch: got %+v, want %+v", stats, want) + } + if _, err := os.Stat(currentLog); err != nil { + t.Fatalf("expected current process log to remain, err=%v", err) + } +} + +func TestRunLoggerPathAndRemove(t *testing.T) { + tempDir := t.TempDir() + path := filepath.Join(tempDir, "sample.log") + if err := os.WriteFile(path, []byte("test"), 0o644); err != nil { + t.Fatalf("failed to create temp file: %v", err) + } + + logger := &Logger{path: path} + if got := logger.Path(); got != path { + t.Fatalf("Path() = %q, want %q", got, path) + } + if err := logger.RemoveLogFile(); err != nil { + t.Fatalf("RemoveLogFile() error = %v", err) + } + if _, err := os.Stat(path); !os.IsNotExist(err) { + t.Fatalf("expected log file to be removed, err=%v", err) + } + + var nilLogger *Logger + if nilLogger.Path() != "" { + t.Fatalf("nil logger Path() should be empty") + } + if err := nilLogger.RemoveLogFile(); err != nil { + t.Fatalf("nil logger RemoveLogFile() should return nil, got %v", err) + } +} + +func TestRunLoggerInternalLog(t *testing.T) { + logger := &Logger{ + ch: make(chan logEntry, 1), + done: make(chan struct{}), + pendingWG: sync.WaitGroup{}, + } + + done := make(chan logEntry, 1) + go func() { + entry := <-logger.ch + logger.pendingWG.Done() + done <- entry + }() + + logger.log("INFO", "hello") + entry := <-done + if entry.level != "INFO" || entry.msg != "hello" { + t.Fatalf("unexpected entry %+v", entry) + } + + logger.closed.Store(true) + logger.log("INFO", "ignored") + close(logger.done) +} + +func TestRunParsePIDFromLog(t *testing.T) { + tests := []struct { + name string + pid int + ok bool + }{ + {"codex-wrapper-123.log", 123, true}, + {"codex-wrapper-999-extra.log", 999, true}, + {"codex-wrapper-.log", 0, false}, + {"invalid-name.log", 0, false}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, ok := parsePIDFromLog(filepath.Join("/tmp", tt.name)) + if ok != tt.ok { + t.Fatalf("parsePIDFromLog ok = %v, want %v", ok, tt.ok) + } + if ok && got != tt.pid { + t.Fatalf("pid = %d, want %d", got, tt.pid) + } + }) + } +} + +func createTempLog(t *testing.T, dir, name string) string { + t.Helper() + path := filepath.Join(dir, name) + if err := os.WriteFile(path, []byte("test"), 0o644); err != nil { + t.Fatalf("failed to create temp log %s: %v", path, err) + } + return path +} + +func setTempDirEnv(t *testing.T, dir string) { + t.Helper() + t.Setenv("TMPDIR", dir) + t.Setenv("TEMP", dir) + t.Setenv("TMP", dir) +} + +func stubProcessRunning(t *testing.T, fn func(int) bool) { + t.Helper() + original := processRunningCheck + processRunningCheck = fn + t.Cleanup(func() { + processRunningCheck = original + }) +} + +func stubRemoveLogFile(t *testing.T, fn func(string) error) { + t.Helper() + original := removeLogFileFn + removeLogFileFn = fn + t.Cleanup(func() { + removeLogFileFn = original + }) +} + +func stubGlobLogFiles(t *testing.T, fn func(string) ([]string, error)) { + t.Helper() + original := globLogFiles + globLogFiles = fn + t.Cleanup(func() { + globLogFiles = original + }) } diff --git a/codex-wrapper/main.go b/codex-wrapper/main.go index 5edee6f..d8ca672 100644 --- a/codex-wrapper/main.go +++ b/codex-wrapper/main.go @@ -41,6 +41,9 @@ var ( commandContext = exec.CommandContext jsonMarshal = json.Marshal forceKillDelay = 5 // seconds - made variable for testability + cleanupLogsFn = cleanupOldLogs + signalNotifyFn = signal.Notify + signalStopFn = signal.Stop ) // Config holds CLI configuration @@ -358,11 +361,27 @@ func main() { os.Exit(exitCode) } +func runStartupCleanup() { + if cleanupLogsFn == nil { + return + } + defer func() { + if r := recover(); r != nil { + logWarn(fmt.Sprintf("cleanupOldLogs panic: %v", r)) + } + }() + if _, err := cleanupLogsFn(); err != nil { + logWarn(fmt.Sprintf("cleanupOldLogs error: %v", err)) + } +} + // run is the main logic, returns exit code for testability func run() (exitCode int) { // Handle --version and --help first (no logger needed) if len(os.Args) > 1 { switch os.Args[1] { + case "--cleanup": + return runCleanupMode() case "--version", "-v": fmt.Printf("codex-wrapper version %s\n", version) return 0 @@ -397,6 +416,8 @@ func run() (exitCode int) { }() defer runCleanupHook() + runStartupCleanup() + // Handle remaining commands if len(os.Args) > 1 { switch os.Args[1] { @@ -557,6 +578,38 @@ func run() (exitCode int) { return 0 } +func runCleanupMode() int { + if cleanupLogsFn == nil { + fmt.Fprintln(os.Stderr, "Cleanup failed: log cleanup function not configured") + return 1 + } + + stats, err := cleanupLogsFn() + if err != nil { + fmt.Fprintf(os.Stderr, "Cleanup failed: %v\n", err) + return 1 + } + + fmt.Println("Cleanup completed") + fmt.Printf("Files scanned: %d\n", stats.Scanned) + fmt.Printf("Files deleted: %d\n", stats.Deleted) + if len(stats.DeletedFiles) > 0 { + for _, f := range stats.DeletedFiles { + fmt.Printf(" - %s\n", f) + } + } + fmt.Printf("Files kept: %d\n", stats.Kept) + if len(stats.KeptFiles) > 0 { + for _, f := range stats.KeptFiles { + fmt.Printf(" - %s\n", f) + } + } + if stats.Errors > 0 { + fmt.Printf("Deletion errors: %d\n", stats.Errors) + } + return 0 +} + func parseArgs() (*Config, error) { args := os.Args[1:] if len(args) == 0 { @@ -925,10 +978,10 @@ func (b *tailBuffer) String() string { func forwardSignals(ctx context.Context, cmd *exec.Cmd, logErrorFn func(string)) { sigCh := make(chan os.Signal, 1) - signal.Notify(sigCh, syscall.SIGINT, syscall.SIGTERM) + signalNotifyFn(sigCh, syscall.SIGINT, syscall.SIGTERM) go func() { - defer signal.Stop(sigCh) + defer signalStopFn(sigCh) select { case sig := <-sigCh: logErrorFn(fmt.Sprintf("Received signal: %v", sig)) diff --git a/codex-wrapper/main_integration_test.go b/codex-wrapper/main_integration_test.go index e5153d8..bf9daf6 100644 --- a/codex-wrapper/main_integration_test.go +++ b/codex-wrapper/main_integration_test.go @@ -5,6 +5,7 @@ import ( "fmt" "io" "os" + "path/filepath" "strings" "sync" "sync/atomic" @@ -106,7 +107,7 @@ func findResultByID(t *testing.T, payload integrationOutput, id string) TaskResu return TaskResult{} } -func TestParallelEndToEnd_OrderAndConcurrency(t *testing.T) { +func TestRunParallelEndToEnd_OrderAndConcurrency(t *testing.T) { defer resetTestHooks() origRun := runCodexTaskFn t.Cleanup(func() { @@ -217,7 +218,7 @@ task-e` } } -func TestParallelCycleDetectionStopsExecution(t *testing.T) { +func TestRunParallelCycleDetectionStopsExecution(t *testing.T) { defer resetTestHooks() origRun := runCodexTaskFn runCodexTaskFn = func(task TaskSpec, timeout int) TaskResult { @@ -255,7 +256,7 @@ b` } } -func TestParallelPartialFailureBlocksDependents(t *testing.T) { +func TestRunParallelPartialFailureBlocksDependents(t *testing.T) { defer resetTestHooks() origRun := runCodexTaskFn t.Cleanup(func() { @@ -319,7 +320,7 @@ ok-e` } } -func TestParallelTimeoutPropagation(t *testing.T) { +func TestRunParallelTimeoutPropagation(t *testing.T) { defer resetTestHooks() origRun := runCodexTaskFn t.Cleanup(func() { @@ -363,7 +364,7 @@ slow` } } -func TestConcurrentSpeedupBenchmark(t *testing.T) { +func TestRunConcurrentSpeedupBenchmark(t *testing.T) { defer resetTestHooks() origRun := runCodexTaskFn t.Cleanup(func() { @@ -398,3 +399,201 @@ func TestConcurrentSpeedupBenchmark(t *testing.T) { ratio := float64(concurrentElapsed) / float64(serialElapsed) t.Logf("speedup ratio (concurrent/serial)=%.3f", ratio) } + +func TestRunStartupCleanupRemovesOrphansEndToEnd(t *testing.T) { + defer resetTestHooks() + + tempDir := t.TempDir() + setTempDirEnv(t, tempDir) + + orphanA := createTempLog(t, tempDir, "codex-wrapper-5001.log") + orphanB := createTempLog(t, tempDir, "codex-wrapper-5002-extra.log") + orphanC := createTempLog(t, tempDir, "codex-wrapper-5003-suffix.log") + runningPID := 81234 + runningLog := createTempLog(t, tempDir, fmt.Sprintf("codex-wrapper-%d.log", runningPID)) + unrelated := createTempLog(t, tempDir, "wrapper.log") + + stubProcessRunning(t, func(pid int) bool { + return pid == runningPID || pid == os.Getpid() + }) + + codexCommand = createFakeCodexScript(t, "tid-startup", "ok") + stdinReader = strings.NewReader("") + isTerminalFn = func() bool { return true } + os.Args = []string{"codex-wrapper", "task"} + + if exit := run(); exit != 0 { + t.Fatalf("run() exit=%d, want 0", exit) + } + + for _, orphan := range []string{orphanA, orphanB, orphanC} { + if _, err := os.Stat(orphan); !os.IsNotExist(err) { + t.Fatalf("expected orphan %s to be removed, err=%v", orphan, err) + } + } + if _, err := os.Stat(runningLog); err != nil { + t.Fatalf("expected running log to remain, err=%v", err) + } + if _, err := os.Stat(unrelated); err != nil { + t.Fatalf("expected unrelated file to remain, err=%v", err) + } +} + +func TestRunStartupCleanupConcurrentWrappers(t *testing.T) { + defer resetTestHooks() + + tempDir := t.TempDir() + setTempDirEnv(t, tempDir) + + const totalLogs = 40 + for i := 0; i < totalLogs; i++ { + createTempLog(t, tempDir, fmt.Sprintf("codex-wrapper-%d.log", 9000+i)) + } + + stubProcessRunning(t, func(pid int) bool { + return false + }) + + var wg sync.WaitGroup + const instances = 5 + start := make(chan struct{}) + + for i := 0; i < instances; i++ { + wg.Add(1) + go func() { + defer wg.Done() + <-start + runStartupCleanup() + }() + } + + close(start) + wg.Wait() + + matches, err := filepath.Glob(filepath.Join(tempDir, "codex-wrapper-*.log")) + if err != nil { + t.Fatalf("glob error: %v", err) + } + if len(matches) != 0 { + t.Fatalf("expected all orphan logs to be removed, remaining=%v", matches) + } +} + +func TestRunCleanupFlagEndToEnd_Success(t *testing.T) { + defer resetTestHooks() + + tempDir := t.TempDir() + setTempDirEnv(t, tempDir) + + staleA := createTempLog(t, tempDir, "codex-wrapper-2100.log") + staleB := createTempLog(t, tempDir, "codex-wrapper-2200-extra.log") + keeper := createTempLog(t, tempDir, "codex-wrapper-2300.log") + + stubProcessRunning(t, func(pid int) bool { + return pid == 2300 || pid == os.Getpid() + }) + + os.Args = []string{"codex-wrapper", "--cleanup"} + + var exitCode int + output := captureStdout(t, func() { + exitCode = run() + }) + + if exitCode != 0 { + t.Fatalf("cleanup exit = %d, want 0", exitCode) + } + + // Check that output contains expected counts and file names + if !strings.Contains(output, "Cleanup completed") { + t.Fatalf("missing 'Cleanup completed' in output: %q", output) + } + if !strings.Contains(output, "Files scanned: 3") { + t.Fatalf("missing 'Files scanned: 3' in output: %q", output) + } + if !strings.Contains(output, "Files deleted: 2") { + t.Fatalf("missing 'Files deleted: 2' in output: %q", output) + } + if !strings.Contains(output, "Files kept: 1") { + t.Fatalf("missing 'Files kept: 1' in output: %q", output) + } + if !strings.Contains(output, "codex-wrapper-2100.log") || !strings.Contains(output, "codex-wrapper-2200-extra.log") { + t.Fatalf("missing deleted file names in output: %q", output) + } + if !strings.Contains(output, "codex-wrapper-2300.log") { + t.Fatalf("missing kept file names in output: %q", output) + } + + for _, path := range []string{staleA, staleB} { + if _, err := os.Stat(path); !os.IsNotExist(err) { + t.Fatalf("expected %s to be removed, err=%v", path, err) + } + } + if _, err := os.Stat(keeper); err != nil { + t.Fatalf("expected kept log to remain, err=%v", err) + } + + currentLog := filepath.Join(tempDir, fmt.Sprintf("codex-wrapper-%d.log", os.Getpid())) + if _, err := os.Stat(currentLog); err == nil { + t.Fatalf("cleanup mode should not create new log file %s", currentLog) + } else if !os.IsNotExist(err) { + t.Fatalf("stat(%s) unexpected error: %v", currentLog, err) + } +} + +func TestRunCleanupFlagEndToEnd_FailureDoesNotAffectStartup(t *testing.T) { + defer resetTestHooks() + + tempDir := t.TempDir() + setTempDirEnv(t, tempDir) + + calls := 0 + cleanupLogsFn = func() (CleanupStats, error) { + calls++ + return CleanupStats{Scanned: 1}, fmt.Errorf("permission denied") + } + + os.Args = []string{"codex-wrapper", "--cleanup"} + + var exitCode int + errOutput := captureStderr(t, func() { + exitCode = run() + }) + + if exitCode != 1 { + t.Fatalf("cleanup failure exit = %d, want 1", exitCode) + } + if !strings.Contains(errOutput, "Cleanup failed") || !strings.Contains(errOutput, "permission denied") { + t.Fatalf("cleanup stderr = %q, want failure message", errOutput) + } + if calls != 1 { + t.Fatalf("cleanup called %d times, want 1", calls) + } + + currentLog := filepath.Join(tempDir, fmt.Sprintf("codex-wrapper-%d.log", os.Getpid())) + if _, err := os.Stat(currentLog); err == nil { + t.Fatalf("cleanup failure should not create new log file %s", currentLog) + } else if !os.IsNotExist(err) { + t.Fatalf("stat(%s) unexpected error: %v", currentLog, err) + } + + cleanupLogsFn = func() (CleanupStats, error) { + return CleanupStats{}, nil + } + codexCommand = createFakeCodexScript(t, "tid-cleanup-e2e", "ok") + stdinReader = strings.NewReader("") + isTerminalFn = func() bool { return true } + os.Args = []string{"codex-wrapper", "post-cleanup task"} + + var normalExit int + normalOutput := captureStdout(t, func() { + normalExit = run() + }) + + if normalExit != 0 { + t.Fatalf("normal run exit = %d, want 0", normalExit) + } + if !strings.Contains(normalOutput, "ok") { + t.Fatalf("normal run output = %q, want codex output", normalOutput) + } +} diff --git a/codex-wrapper/main_test.go b/codex-wrapper/main_test.go index a496baa..4021276 100644 --- a/codex-wrapper/main_test.go +++ b/codex-wrapper/main_test.go @@ -1,6 +1,7 @@ package main import ( + "bufio" "bytes" "context" "encoding/json" @@ -11,6 +12,7 @@ import ( "os/exec" "os/signal" "path/filepath" + "runtime" "strings" "sync" "sync/atomic" @@ -25,6 +27,9 @@ func resetTestHooks() { isTerminalFn = defaultIsTerminal codexCommand = "codex" cleanupHook = nil + cleanupLogsFn = cleanupOldLogs + signalNotifyFn = signal.Notify + signalStopFn = signal.Stop buildCodexArgsFn = buildCodexArgs commandContext = exec.CommandContext jsonMarshal = json.Marshal @@ -84,6 +89,20 @@ func captureOutput(t *testing.T, fn func()) string { return buf.String() } +func captureStderr(t *testing.T, fn func()) string { + t.Helper() + r, w, _ := os.Pipe() + old := os.Stderr + os.Stderr = w + fn() + w.Close() + os.Stderr = old + + var buf bytes.Buffer + io.Copy(&buf, r) + return buf.String() +} + func createFakeCodexScript(t *testing.T, threadID, message string) string { t.Helper() scriptPath := filepath.Join(t.TempDir(), "codex.sh") @@ -202,7 +221,7 @@ func TestRunParseArgs_ResumeMode(t *testing.T) { } } -func TestParseParallelConfig_Success(t *testing.T) { +func TestRunParseParallelConfig_Success(t *testing.T) { input := `---TASK--- id: task-1 dependencies: task-0 @@ -222,13 +241,13 @@ do something` } } -func TestParseParallelConfig_InvalidFormat(t *testing.T) { +func TestRunParseParallelConfig_InvalidFormat(t *testing.T) { if _, err := parseParallelConfig([]byte("invalid format")); err == nil { t.Fatalf("expected error for invalid format, got nil") } } -func TestParseParallelConfig_EmptyTasks(t *testing.T) { +func TestRunParseParallelConfig_EmptyTasks(t *testing.T) { input := `---TASK--- id: empty ---CONTENT--- @@ -238,7 +257,7 @@ id: empty } } -func TestParseParallelConfig_MissingID(t *testing.T) { +func TestRunParseParallelConfig_MissingID(t *testing.T) { input := `---TASK--- ---CONTENT--- do something` @@ -247,7 +266,7 @@ do something` } } -func TestParseParallelConfig_MissingTask(t *testing.T) { +func TestRunParseParallelConfig_MissingTask(t *testing.T) { input := `---TASK--- id: task-1 ---CONTENT--- @@ -257,7 +276,7 @@ id: task-1 } } -func TestParseParallelConfig_DuplicateID(t *testing.T) { +func TestRunParseParallelConfig_DuplicateID(t *testing.T) { input := `---TASK--- id: dup ---CONTENT--- @@ -271,7 +290,7 @@ two` } } -func TestParseParallelConfig_DelimiterFormat(t *testing.T) { +func TestRunParseParallelConfig_DelimiterFormat(t *testing.T) { input := `---TASK--- id: T1 workdir: /tmp @@ -292,7 +311,7 @@ code with special chars: $var "quotes"` } } -func TestShouldUseStdin(t *testing.T) { +func TestRunShouldUseStdin(t *testing.T) { tests := []struct { name string task string @@ -403,7 +422,7 @@ func TestRunNormalizeText(t *testing.T) { } } -func TestParseJSONStream(t *testing.T) { +func TestRunParseJSONStream(t *testing.T) { type testCase struct { name string input string @@ -442,7 +461,7 @@ func TestParseJSONStream(t *testing.T) { } } -func TestParseJSONStreamWithWarn_InvalidLine(t *testing.T) { +func TestRunParseJSONStreamWithWarn_InvalidLine(t *testing.T) { var warnings []string warnFn := func(msg string) { warnings = append(warnings, msg) } message, threadID := parseJSONStreamWithWarn(strings.NewReader("not-json"), warnFn) @@ -506,6 +525,10 @@ func TestRunTruncate(t *testing.T) { } }) } + + if got := truncate("data", -1); got != "" { + t.Fatalf("truncate should return empty string for negative maxLen, got %q", got) + } } func TestRunMin(t *testing.T) { @@ -595,7 +618,7 @@ func TestRunIsTerminal(t *testing.T) { } } -func TestReadPipedTask(t *testing.T) { +func TestRunReadPipedTask(t *testing.T) { defer resetTestHooks() tests := []struct { name string @@ -764,7 +787,24 @@ func TestRunCodexTask_SignalHandling(t *testing.T) { } } -func TestSilentMode(t *testing.T) { +func TestRunCodexProcess(t *testing.T) { + defer resetTestHooks() + script := createFakeCodexScript(t, "proc-thread", "proc-msg") + codexCommand = script + + msg, threadID, exitCode := runCodexProcess(context.Background(), nil, "ignored", false, 5) + if exitCode != 0 { + t.Fatalf("exit = %d, want 0", exitCode) + } + if msg != "proc-msg" { + t.Fatalf("message = %q, want proc-msg", msg) + } + if threadID != "proc-thread" { + t.Fatalf("threadID = %q, want proc-thread", threadID) + } +} + +func TestRunSilentMode(t *testing.T) { defer resetTestHooks() jsonOutput := `{"type":"thread.started","thread_id":"silent-session"} {"type":"item.completed","item":{"type":"agent_message","text":"quiet"}}` @@ -799,7 +839,7 @@ func TestSilentMode(t *testing.T) { } } -func TestGenerateFinalOutput(t *testing.T) { +func TestRunGenerateFinalOutput(t *testing.T) { results := []TaskResult{{TaskID: "a", ExitCode: 0, Message: "ok"}, {TaskID: "b", ExitCode: 1, Error: "boom"}, {TaskID: "c", ExitCode: 0}} out := generateFinalOutput(results) if out == "" { @@ -813,7 +853,7 @@ func TestGenerateFinalOutput(t *testing.T) { } } -func TestTopologicalSort_LinearChain(t *testing.T) { +func TestRunTopologicalSort_LinearChain(t *testing.T) { tasks := []TaskSpec{{ID: "a"}, {ID: "b", Dependencies: []string{"a"}}, {ID: "c", Dependencies: []string{"b"}}} layers, err := topologicalSort(tasks) if err != nil { @@ -824,7 +864,7 @@ func TestTopologicalSort_LinearChain(t *testing.T) { } } -func TestTopologicalSort_Branching(t *testing.T) { +func TestRunTopologicalSort_Branching(t *testing.T) { tasks := []TaskSpec{{ID: "root"}, {ID: "left", Dependencies: []string{"root"}}, {ID: "right", Dependencies: []string{"root"}}, {ID: "leaf", Dependencies: []string{"left", "right"}}} layers, err := topologicalSort(tasks) if err != nil { @@ -835,7 +875,7 @@ func TestTopologicalSort_Branching(t *testing.T) { } } -func TestTopologicalSort_ParallelTasks(t *testing.T) { +func TestRunTopologicalSort_ParallelTasks(t *testing.T) { tasks := []TaskSpec{{ID: "a"}, {ID: "b"}, {ID: "c"}} layers, err := topologicalSort(tasks) if err != nil { @@ -846,7 +886,7 @@ func TestTopologicalSort_ParallelTasks(t *testing.T) { } } -func TestShouldSkipTask(t *testing.T) { +func TestRunShouldSkipTask(t *testing.T) { failed := map[string]TaskResult{"a": {TaskID: "a", ExitCode: 1}, "b": {TaskID: "b", ExitCode: 2}} tests := []struct { name string @@ -875,28 +915,28 @@ func TestShouldSkipTask(t *testing.T) { } } -func TestTopologicalSort_CycleDetection(t *testing.T) { +func TestRunTopologicalSort_CycleDetection(t *testing.T) { tasks := []TaskSpec{{ID: "a", Dependencies: []string{"b"}}, {ID: "b", Dependencies: []string{"a"}}} if _, err := topologicalSort(tasks); err == nil || !strings.Contains(err.Error(), "cycle detected") { t.Fatalf("expected cycle error, got %v", err) } } -func TestTopologicalSort_IndirectCycle(t *testing.T) { +func TestRunTopologicalSort_IndirectCycle(t *testing.T) { tasks := []TaskSpec{{ID: "a", Dependencies: []string{"c"}}, {ID: "b", Dependencies: []string{"a"}}, {ID: "c", Dependencies: []string{"b"}}} if _, err := topologicalSort(tasks); err == nil || !strings.Contains(err.Error(), "cycle detected") { t.Fatalf("expected cycle error, got %v", err) } } -func TestTopologicalSort_MissingDependency(t *testing.T) { +func TestRunTopologicalSort_MissingDependency(t *testing.T) { tasks := []TaskSpec{{ID: "a", Dependencies: []string{"missing"}}} if _, err := topologicalSort(tasks); err == nil || !strings.Contains(err.Error(), "dependency \"missing\" not found") { t.Fatalf("expected missing dependency error, got %v", err) } } -func TestTopologicalSort_LargeGraph(t *testing.T) { +func TestRunTopologicalSort_LargeGraph(t *testing.T) { const count = 200 tasks := make([]TaskSpec, count) for i := 0; i < count; i++ { @@ -918,7 +958,7 @@ func TestTopologicalSort_LargeGraph(t *testing.T) { } } -func TestExecuteConcurrent_ParallelExecution(t *testing.T) { +func TestRunExecuteConcurrent_ParallelExecution(t *testing.T) { orig := runCodexTaskFn defer func() { runCodexTaskFn = orig }() @@ -954,7 +994,7 @@ func TestExecuteConcurrent_ParallelExecution(t *testing.T) { } } -func TestExecuteConcurrent_LayerOrdering(t *testing.T) { +func TestRunExecuteConcurrent_LayerOrdering(t *testing.T) { orig := runCodexTaskFn defer func() { runCodexTaskFn = orig }() @@ -976,7 +1016,7 @@ func TestExecuteConcurrent_LayerOrdering(t *testing.T) { } } -func TestExecuteConcurrent_ErrorIsolation(t *testing.T) { +func TestRunExecuteConcurrent_ErrorIsolation(t *testing.T) { orig := runCodexTaskFn defer func() { runCodexTaskFn = orig }() @@ -1009,7 +1049,7 @@ func TestExecuteConcurrent_ErrorIsolation(t *testing.T) { } } -func TestExecuteConcurrent_PanicRecovered(t *testing.T) { +func TestRunExecuteConcurrent_PanicRecovered(t *testing.T) { orig := runCodexTaskFn defer func() { runCodexTaskFn = orig }() @@ -1023,7 +1063,7 @@ func TestExecuteConcurrent_PanicRecovered(t *testing.T) { } } -func TestExecuteConcurrent_LargeFanout(t *testing.T) { +func TestRunExecuteConcurrent_LargeFanout(t *testing.T) { orig := runCodexTaskFn defer func() { runCodexTaskFn = orig }() @@ -1063,6 +1103,37 @@ test` } } +func TestRun_ParallelTriggersCleanup(t *testing.T) { + defer resetTestHooks() + oldArgs := os.Args + defer func() { os.Args = oldArgs }() + + os.Args = []string{"codex-wrapper", "--parallel"} + stdinReader = strings.NewReader(`---TASK--- +id: only +---CONTENT--- +noop`) + + cleanupCalls := 0 + cleanupLogsFn = func() (CleanupStats, error) { + cleanupCalls++ + return CleanupStats{}, nil + } + + orig := runCodexTaskFn + runCodexTaskFn = func(task TaskSpec, timeout int) TaskResult { + return TaskResult{TaskID: task.ID, ExitCode: 0, Message: "ok"} + } + defer func() { runCodexTaskFn = orig }() + + if exitCode := run(); exitCode != 0 { + t.Fatalf("exit = %d, want 0", exitCode) + } + if cleanupCalls != 1 { + t.Fatalf("cleanup called %d times, want 1", cleanupCalls) + } +} + func TestRun_Version(t *testing.T) { defer resetTestHooks() os.Args = []string{"codex-wrapper", "--version"} @@ -1095,6 +1166,172 @@ func TestRun_HelpShort(t *testing.T) { } } +func TestRun_HelpDoesNotTriggerCleanup(t *testing.T) { + defer resetTestHooks() + os.Args = []string{"codex-wrapper", "--help"} + cleanupLogsFn = func() (CleanupStats, error) { + t.Fatalf("cleanup should not run for --help") + return CleanupStats{}, nil + } + + if code := run(); code != 0 { + t.Fatalf("exit = %d, want 0", code) + } +} + +func TestRun_VersionDoesNotTriggerCleanup(t *testing.T) { + defer resetTestHooks() + os.Args = []string{"codex-wrapper", "--version"} + cleanupLogsFn = func() (CleanupStats, error) { + t.Fatalf("cleanup should not run for --version") + return CleanupStats{}, nil + } + + if code := run(); code != 0 { + t.Fatalf("exit = %d, want 0", code) + } +} + +func TestRunCleanupMode_Success(t *testing.T) { + defer resetTestHooks() + cleanupLogsFn = func() (CleanupStats, error) { + return CleanupStats{ + Scanned: 5, + Deleted: 3, + Kept: 2, + DeletedFiles: []string{"codex-wrapper-111.log", "codex-wrapper-222.log", "codex-wrapper-333.log"}, + KeptFiles: []string{"codex-wrapper-444.log", "codex-wrapper-555.log"}, + }, nil + } + + var exitCode int + output := captureOutput(t, func() { + exitCode = runCleanupMode() + }) + if exitCode != 0 { + t.Fatalf("exit = %d, want 0", exitCode) + } + want := "Cleanup completed\nFiles scanned: 5\nFiles deleted: 3\n - codex-wrapper-111.log\n - codex-wrapper-222.log\n - codex-wrapper-333.log\nFiles kept: 2\n - codex-wrapper-444.log\n - codex-wrapper-555.log\n" + if output != want { + t.Fatalf("output = %q, want %q", output, want) + } +} + +func TestRunCleanupMode_SuccessWithErrorsLine(t *testing.T) { + defer resetTestHooks() + cleanupLogsFn = func() (CleanupStats, error) { + return CleanupStats{ + Scanned: 2, + Deleted: 1, + Kept: 0, + Errors: 1, + DeletedFiles: []string{"codex-wrapper-123.log"}, + }, nil + } + + var exitCode int + output := captureOutput(t, func() { + exitCode = runCleanupMode() + }) + if exitCode != 0 { + t.Fatalf("exit = %d, want 0", exitCode) + } + want := "Cleanup completed\nFiles scanned: 2\nFiles deleted: 1\n - codex-wrapper-123.log\nFiles kept: 0\nDeletion errors: 1\n" + if output != want { + t.Fatalf("output = %q, want %q", output, want) + } +} + +func TestRunCleanupMode_ZeroStatsOutput(t *testing.T) { + defer resetTestHooks() + calls := 0 + cleanupLogsFn = func() (CleanupStats, error) { + calls++ + return CleanupStats{}, nil + } + + var exitCode int + output := captureOutput(t, func() { + exitCode = runCleanupMode() + }) + if exitCode != 0 { + t.Fatalf("exit = %d, want 0", exitCode) + } + want := "Cleanup completed\nFiles scanned: 0\nFiles deleted: 0\nFiles kept: 0\n" + if output != want { + t.Fatalf("output = %q, want %q", output, want) + } + if calls != 1 { + t.Fatalf("cleanup called %d times, want 1", calls) + } +} + +func TestRunCleanupMode_Error(t *testing.T) { + defer resetTestHooks() + cleanupLogsFn = func() (CleanupStats, error) { + return CleanupStats{}, fmt.Errorf("boom") + } + + var exitCode int + errOutput := captureStderr(t, func() { + exitCode = runCleanupMode() + }) + if exitCode != 1 { + t.Fatalf("exit = %d, want 1", exitCode) + } + if !strings.Contains(errOutput, "Cleanup failed") || !strings.Contains(errOutput, "boom") { + t.Fatalf("stderr = %q, want error message", errOutput) + } +} + +func TestRunCleanupMode_MissingFn(t *testing.T) { + defer resetTestHooks() + cleanupLogsFn = nil + + var exitCode int + errOutput := captureStderr(t, func() { + exitCode = runCleanupMode() + }) + if exitCode != 1 { + t.Fatalf("exit = %d, want 1", exitCode) + } + if !strings.Contains(errOutput, "log cleanup function not configured") { + t.Fatalf("stderr = %q, want missing-fn message", errOutput) + } +} + +func TestRun_CleanupFlag(t *testing.T) { + defer resetTestHooks() + oldArgs := os.Args + defer func() { os.Args = oldArgs }() + + os.Args = []string{"codex-wrapper", "--cleanup"} + + calls := 0 + cleanupLogsFn = func() (CleanupStats, error) { + calls++ + return CleanupStats{Scanned: 1, Deleted: 1}, nil + } + + var exitCode int + output := captureOutput(t, func() { + exitCode = run() + }) + if exitCode != 0 { + t.Fatalf("exit = %d, want 0", exitCode) + } + if calls != 1 { + t.Fatalf("cleanup called %d times, want 1", calls) + } + want := "Cleanup completed\nFiles scanned: 1\nFiles deleted: 1\nFiles kept: 0\n" + if output != want { + t.Fatalf("output = %q, want %q", output, want) + } + if logger := activeLogger(); logger != nil { + t.Fatalf("logger should not initialize for --cleanup mode") + } +} + func TestRun_NoArgs(t *testing.T) { defer resetTestHooks() os.Args = []string{"codex-wrapper"} @@ -1348,8 +1585,10 @@ func TestRun_CleanupHookAlwaysCalled(t *testing.T) { cleanupHook = func() { called = true } // Use a command that goes through normal flow, not --version which returns early codexCommand = "echo" - buildCodexArgsFn = func(cfg *Config, targetArg string) []string { return []string{`{"type":"thread.started","thread_id":"x"} -{"type":"item.completed","item":{"type":"agent_message","text":"ok"}}`} } + buildCodexArgsFn = func(cfg *Config, targetArg string) []string { + return []string{`{"type":"thread.started","thread_id":"x"} +{"type":"item.completed","item":{"type":"agent_message","text":"ok"}}`} + } os.Args = []string{"codex-wrapper", "task"} if exitCode := run(); exitCode != 0 { t.Fatalf("exit = %d, want 0", exitCode) @@ -1359,13 +1598,43 @@ func TestRun_CleanupHookAlwaysCalled(t *testing.T) { } } +func TestRunStartupCleanupNil(t *testing.T) { + defer resetTestHooks() + cleanupLogsFn = nil + runStartupCleanup() +} + +func TestRun_CleanupFailureDoesNotBlock(t *testing.T) { + defer resetTestHooks() + stdout := captureStdoutPipe() + defer restoreStdoutPipe(stdout) + + cleanupCalled := 0 + cleanupLogsFn = func() (CleanupStats, error) { + cleanupCalled++ + panic("boom") + } + + codexCommand = createFakeCodexScript(t, "tid-cleanup", "ok") + stdinReader = strings.NewReader("") + isTerminalFn = func() bool { return true } + os.Args = []string{"codex-wrapper", "task"} + + if exit := run(); exit != 0 { + t.Fatalf("exit = %d, want 0", exit) + } + if cleanupCalled != 1 { + t.Fatalf("cleanup called %d times, want 1", cleanupCalled) + } +} + // Coverage helper reused by logger_test to keep focused runs exercising core paths. -func TestParseJSONStream_CoverageSuite(t *testing.T) { +func TestRunParseJSONStream_CoverageSuite(t *testing.T) { suite := []struct { name string fn func(*testing.T) }{ - {"TestParseJSONStream", TestParseJSONStream}, + {"TestRunParseJSONStream", TestRunParseJSONStream}, {"TestRunNormalizeText", TestRunNormalizeText}, {"TestRunTruncate", TestRunTruncate}, {"TestRunMin", TestRunMin}, @@ -1377,30 +1646,172 @@ func TestParseJSONStream_CoverageSuite(t *testing.T) { } } -func TestHello(t *testing.T) { +func TestRunHello(t *testing.T) { if got := hello(); got != "hello world" { t.Fatalf("hello() = %q, want %q", got, "hello world") } } -func TestGreet(t *testing.T) { +func TestRunGreet(t *testing.T) { if got := greet("Linus"); got != "hello Linus" { t.Fatalf("greet() = %q, want %q", got, "hello Linus") } } -func TestFarewell(t *testing.T) { +func TestRunFarewell(t *testing.T) { if got := farewell("Linus"); got != "goodbye Linus" { t.Fatalf("farewell() = %q, want %q", got, "goodbye Linus") } } -func TestFarewellEmpty(t *testing.T) { +func TestRunFarewellEmpty(t *testing.T) { if got := farewell(""); got != "goodbye " { t.Fatalf("farewell(\"\") = %q, want %q", got, "goodbye ") } } +func TestRunTailBuffer(t *testing.T) { + tb := &tailBuffer{limit: 5} + if n, err := tb.Write([]byte("abcd")); err != nil || n != 4 { + t.Fatalf("Write returned (%d, %v), want (4, nil)", n, err) + } + if n, err := tb.Write([]byte("efg")); err != nil || n != 3 { + t.Fatalf("Write returned (%d, %v), want (3, nil)", n, err) + } + if got := tb.String(); got != "cdefg" { + t.Fatalf("tail buffer = %q, want %q", got, "cdefg") + } + if n, err := tb.Write([]byte("0123456")); err != nil || n != 7 { + t.Fatalf("Write returned (%d, %v), want (7, nil)", n, err) + } + if got := tb.String(); got != "23456" { + t.Fatalf("tail buffer = %q, want %q", got, "23456") + } +} + +func TestRunLogWriter(t *testing.T) { + defer resetTestHooks() + logger, err := NewLoggerWithSuffix("logwriter") + if err != nil { + t.Fatalf("failed to create logger: %v", err) + } + setLogger(logger) + + lw := newLogWriter("TEST: ", 10) + if _, err := lw.Write([]byte("hello\n")); err != nil { + t.Fatalf("write hello failed: %v", err) + } + if _, err := lw.Write([]byte("world-is-long")); err != nil { + t.Fatalf("write world failed: %v", err) + } + lw.Flush() + + logger.Flush() + logger.Close() + + data, err := os.ReadFile(logger.Path()) + if err != nil { + t.Fatalf("failed to read log file: %v", err) + } + text := string(data) + if !strings.Contains(text, "TEST: hello") { + t.Fatalf("log missing hello entry: %s", text) + } + if !strings.Contains(text, "TEST: world-i...") { + t.Fatalf("log missing truncated entry: %s", text) + } + os.Remove(logger.Path()) +} + +func TestRunDiscardInvalidJSON(t *testing.T) { + reader := bufio.NewReader(strings.NewReader("bad line\n{\"type\":\"ok\"}\n")) + next, err := discardInvalidJSON(nil, reader) + if err != nil { + t.Fatalf("discardInvalidJSON error: %v", err) + } + line, err := next.ReadString('\n') + if err != nil { + t.Fatalf("failed to read next line: %v", err) + } + if strings.TrimSpace(line) != `{"type":"ok"}` { + t.Fatalf("unexpected remaining line: %q", line) + } + + t.Run("no newline", func(t *testing.T) { + reader := bufio.NewReader(strings.NewReader("partial")) + decoder := json.NewDecoder(strings.NewReader("")) + if _, err := discardInvalidJSON(decoder, reader); !errors.Is(err, io.EOF) { + t.Fatalf("expected EOF when no newline, got %v", err) + } + }) +} + +func TestRunForwardSignals(t *testing.T) { + defer resetTestHooks() + + if runtime.GOOS == "windows" { + t.Skip("sleep command not available on Windows") + } + + cmd := exec.Command("sleep", "5") + if err := cmd.Start(); err != nil { + t.Skipf("unable to start sleep command: %v", err) + } + defer func() { + _ = cmd.Process.Kill() + cmd.Wait() + }() + + forceKillDelay = 0 + defer func() { forceKillDelay = 5 }() + + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + ready := make(chan struct{}) + var captured chan<- os.Signal + signalNotifyFn = func(ch chan<- os.Signal, sig ...os.Signal) { + captured = ch + close(ready) + } + signalStopFn = func(ch chan<- os.Signal) {} + defer func() { + signalNotifyFn = signal.Notify + signalStopFn = signal.Stop + }() + + var mu sync.Mutex + var logs []string + forwardSignals(ctx, cmd, func(msg string) { + mu.Lock() + defer mu.Unlock() + logs = append(logs, msg) + }) + + select { + case <-ready: + case <-time.After(500 * time.Millisecond): + t.Fatalf("signalNotifyFn not invoked") + } + + captured <- syscall.SIGINT + + done := make(chan error, 1) + go func() { done <- cmd.Wait() }() + + select { + case <-done: + case <-time.After(2 * time.Second): + t.Fatalf("process did not exit after forwarded signal") + } + + mu.Lock() + defer mu.Unlock() + if len(logs) == 0 { + t.Fatalf("expected log entry for forwarded signal") + } +} + func TestRun_CLI_Success(t *testing.T) { defer resetTestHooks() os.Args = []string{"codex-wrapper", "do-things"} diff --git a/codex-wrapper/process_check_test.go b/codex-wrapper/process_check_test.go new file mode 100644 index 0000000..9e70878 --- /dev/null +++ b/codex-wrapper/process_check_test.go @@ -0,0 +1,116 @@ +package main + +import ( + "errors" + "os" + "os/exec" + "runtime" + "testing" + "time" +) + +func TestIsProcessRunning(t *testing.T) { + t.Run("current process", func(t *testing.T) { + if !isProcessRunning(os.Getpid()) { + t.Fatalf("expected current process (pid=%d) to be running", os.Getpid()) + } + }) + + t.Run("fake pid", func(t *testing.T) { + const nonexistentPID = 1 << 30 + if isProcessRunning(nonexistentPID) { + t.Fatalf("expected pid %d to be reported as not running", nonexistentPID) + } + }) + + t.Run("terminated process", func(t *testing.T) { + pid := exitedProcessPID(t) + if isProcessRunning(pid) { + t.Fatalf("expected exited child process (pid=%d) to be reported as not running", pid) + } + }) + + t.Run("boundary values", func(t *testing.T) { + if isProcessRunning(0) { + t.Fatalf("pid 0 should never be treated as running") + } + if isProcessRunning(-42) { + t.Fatalf("negative pid should never be treated as running") + } + }) + + t.Run("find process error", func(t *testing.T) { + original := findProcess + defer func() { findProcess = original }() + + mockErr := errors.New("findProcess failure") + findProcess = func(pid int) (*os.Process, error) { + return nil, mockErr + } + + if isProcessRunning(1234) { + t.Fatalf("expected false when os.FindProcess fails") + } + }) +} + +func exitedProcessPID(t *testing.T) int { + t.Helper() + + var cmd *exec.Cmd + if runtime.GOOS == "windows" { + cmd = exec.Command("cmd", "/c", "exit 0") + } else { + cmd = exec.Command("sh", "-c", "exit 0") + } + + if err := cmd.Start(); err != nil { + t.Fatalf("failed to start helper process: %v", err) + } + pid := cmd.Process.Pid + + if err := cmd.Wait(); err != nil { + t.Fatalf("helper process did not exit cleanly: %v", err) + } + + time.Sleep(50 * time.Millisecond) + return pid +} + +func TestRunProcessCheckSmoke(t *testing.T) { + t.Run("current process", func(t *testing.T) { + if !isProcessRunning(os.Getpid()) { + t.Fatalf("expected current process (pid=%d) to be running", os.Getpid()) + } + }) + + t.Run("fake pid", func(t *testing.T) { + const nonexistentPID = 1 << 30 + if isProcessRunning(nonexistentPID) { + t.Fatalf("expected pid %d to be reported as not running", nonexistentPID) + } + }) + + t.Run("boundary values", func(t *testing.T) { + if isProcessRunning(0) { + t.Fatalf("pid 0 should never be treated as running") + } + if isProcessRunning(-42) { + t.Fatalf("negative pid should never be treated as running") + } + }) + + t.Run("find process error", func(t *testing.T) { + original := findProcess + defer func() { findProcess = original }() + + mockErr := errors.New("findProcess failure") + findProcess = func(pid int) (*os.Process, error) { + return nil, mockErr + } + + if isProcessRunning(1234) { + t.Fatalf("expected false when os.FindProcess fails") + } + }) +} diff --git a/codex-wrapper/process_check_unix.go b/codex-wrapper/process_check_unix.go new file mode 100644 index 0000000..9ba76f0 --- /dev/null +++ b/codex-wrapper/process_check_unix.go @@ -0,0 +1,30 @@ +//go:build unix || darwin || linux +// +build unix darwin linux + +package main + +import ( + "errors" + "os" + "syscall" +) + +var findProcess = os.FindProcess + +// isProcessRunning returns true if a process with the given pid is running on Unix-like systems. +func isProcessRunning(pid int) bool { + if pid <= 0 { + return false + } + + proc, err := findProcess(pid) + if err != nil || proc == nil { + return false + } + + err = proc.Signal(syscall.Signal(0)) + if err != nil && (errors.Is(err, syscall.ESRCH) || errors.Is(err, os.ErrProcessDone)) { + return false + } + return true +} diff --git a/codex-wrapper/process_check_windows.go b/codex-wrapper/process_check_windows.go new file mode 100644 index 0000000..2905ba6 --- /dev/null +++ b/codex-wrapper/process_check_windows.go @@ -0,0 +1,44 @@ +//go:build windows +// +build windows + +package main + +import ( + "errors" + "os" + "syscall" +) + +const ( + processQueryLimitedInformation = 0x1000 + stillActive = 259 // STILL_ACTIVE exit code +) + +var findProcess = os.FindProcess + +// isProcessRunning returns true if a process with the given pid is running on Windows. +func isProcessRunning(pid int) bool { + if pid <= 0 { + return false + } + + if _, err := findProcess(pid); err != nil { + return false + } + + handle, err := syscall.OpenProcess(processQueryLimitedInformation, false, uint32(pid)) + if err != nil { + if errors.Is(err, syscall.ERROR_ACCESS_DENIED) { + return true + } + return false + } + defer syscall.CloseHandle(handle) + + var exitCode uint32 + if err := syscall.GetExitCodeProcess(handle, &exitCode); err != nil { + return true + } + + return exitCode == stillActive +}