fix: comprehensive security and quality improvements for PR #85 & #87 (#90)

Co-authored-by: tytsxai <tytsxai@users.noreply.github.com>

codeagent-wrapper/backend.go

@@ -26,15 +26,17 @@ func (ClaudeBackend) Command() string {
 	return "claude"
 }
 func (ClaudeBackend) BuildArgs(cfg *Config, targetArg string) []string {
+	return buildClaudeArgs(cfg, targetArg)
+}
+
+func buildClaudeArgs(cfg *Config, targetArg string) []string {
 	if cfg == nil {
 		return nil
 	}
-	args := []string{"-p", "--dangerously-skip-permissions"}
-
-	// Only skip permissions when explicitly requested
-	// if cfg.SkipPermissions {
-	// 	args = append(args, "--dangerously-skip-permissions")
-	// }
+	args := []string{"-p"}
+	if cfg.SkipPermissions {
+		args = append(args, "--dangerously-skip-permissions")
+	}
 
 	// Prevent infinite recursion: disable all setting sources (user, project, local)
 	// This ensures a clean execution environment without CLAUDE.md or skills that would trigger codeagent
@@ -60,6 +62,10 @@ func (GeminiBackend) Command() string {
 	return "gemini"
 }
 func (GeminiBackend) BuildArgs(cfg *Config, targetArg string) []string {
+	return buildGeminiArgs(cfg, targetArg)
+}
+
+func buildGeminiArgs(cfg *Config, targetArg string) []string {
 	if cfg == nil {
 		return nil
 	}

codeagent-wrapper/backend_test.go

@@ -1,6 +1,7 @@
 package main
 
 import (
+	"os"
 	"reflect"
 	"testing"
 )
@@ -8,16 +9,16 @@ import (
 func TestClaudeBuildArgs_ModesAndPermissions(t *testing.T) {
 	backend := ClaudeBackend{}
 
-	t.Run("new mode uses workdir without skip by default", func(t *testing.T) {
+	t.Run("new mode omits skip-permissions by default", func(t *testing.T) {
 		cfg := &Config{Mode: "new", WorkDir: "/repo"}
 		got := backend.BuildArgs(cfg, "todo")
-		want := []string{"-p", "--dangerously-skip-permissions", "--setting-sources", "", "--output-format", "stream-json", "--verbose", "todo"}
+		want := []string{"-p", "--setting-sources", "", "--output-format", "stream-json", "--verbose", "todo"}
 		if !reflect.DeepEqual(got, want) {
 			t.Fatalf("got %v, want %v", got, want)
 		}
 	})
 
-	t.Run("new mode opt-in skip permissions with default workdir", func(t *testing.T) {
+	t.Run("new mode can opt-in skip-permissions", func(t *testing.T) {
 		cfg := &Config{Mode: "new", SkipPermissions: true}
 		got := backend.BuildArgs(cfg, "-")
 		want := []string{"-p", "--dangerously-skip-permissions", "--setting-sources", "", "--output-format", "stream-json", "--verbose", "-"}
@@ -26,10 +27,10 @@ func TestClaudeBuildArgs_ModesAndPermissions(t *testing.T) {
 		}
 	})
 
-	t.Run("resume mode uses session id and omits workdir", func(t *testing.T) {
+	t.Run("resume mode includes session id", func(t *testing.T) {
 		cfg := &Config{Mode: "resume", SessionID: "sid-123", WorkDir: "/ignored"}
 		got := backend.BuildArgs(cfg, "resume-task")
-		want := []string{"-p", "--dangerously-skip-permissions", "--setting-sources", "", "-r", "sid-123", "--output-format", "stream-json", "--verbose", "resume-task"}
+		want := []string{"-p", "--setting-sources", "", "-r", "sid-123", "--output-format", "stream-json", "--verbose", "resume-task"}
 		if !reflect.DeepEqual(got, want) {
 			t.Fatalf("got %v, want %v", got, want)
 		}
@@ -38,7 +39,16 @@ func TestClaudeBuildArgs_ModesAndPermissions(t *testing.T) {
 	t.Run("resume mode without session still returns base flags", func(t *testing.T) {
 		cfg := &Config{Mode: "resume", WorkDir: "/ignored"}
 		got := backend.BuildArgs(cfg, "follow-up")
-		want := []string{"-p", "--dangerously-skip-permissions", "--setting-sources", "", "--output-format", "stream-json", "--verbose", "follow-up"}
+		want := []string{"-p", "--setting-sources", "", "--output-format", "stream-json", "--verbose", "follow-up"}
+		if !reflect.DeepEqual(got, want) {
+			t.Fatalf("got %v, want %v", got, want)
+		}
+	})
+
+	t.Run("resume mode can opt-in skip permissions", func(t *testing.T) {
+		cfg := &Config{Mode: "resume", SessionID: "sid-123", SkipPermissions: true}
+		got := backend.BuildArgs(cfg, "resume-task")
+		want := []string{"-p", "--dangerously-skip-permissions", "--setting-sources", "", "-r", "sid-123", "--output-format", "stream-json", "--verbose", "resume-task"}
 		if !reflect.DeepEqual(got, want) {
 			t.Fatalf("got %v, want %v", got, want)
 		}
@@ -89,7 +99,11 @@ func TestClaudeBuildArgs_GeminiAndCodexModes(t *testing.T) {
 		}
 	})
 
-	t.Run("codex build args passthrough remains intact", func(t *testing.T) {
+	t.Run("codex build args omits bypass flag by default", func(t *testing.T) {
+		const key = "CODEX_BYPASS_SANDBOX"
+		t.Cleanup(func() { os.Unsetenv(key) })
+		os.Unsetenv(key)
+
 		backend := CodexBackend{}
 		cfg := &Config{Mode: "new", WorkDir: "/tmp"}
 		got := backend.BuildArgs(cfg, "task")
@@ -98,6 +112,20 @@ func TestClaudeBuildArgs_GeminiAndCodexModes(t *testing.T) {
 			t.Fatalf("got %v, want %v", got, want)
 		}
 	})
+
+	t.Run("codex build args includes bypass flag when enabled", func(t *testing.T) {
+		const key = "CODEX_BYPASS_SANDBOX"
+		t.Cleanup(func() { os.Unsetenv(key) })
+		os.Setenv(key, "true")
+
+		backend := CodexBackend{}
+		cfg := &Config{Mode: "new", WorkDir: "/tmp"}
+		got := backend.BuildArgs(cfg, "task")
+		want := []string{"e", "--dangerously-bypass-approvals-and-sandbox", "--skip-git-repo-check", "-C", "/tmp", "--json", "task"}
+		if !reflect.DeepEqual(got, want) {
+			t.Fatalf("got %v, want %v", got, want)
+		}
+	})
 }
 
 func TestClaudeBuildArgs_BackendMetadata(t *testing.T) {

codeagent-wrapper/config.go

@@ -164,6 +164,9 @@ func parseParallelConfig(data []byte) (*ParallelConfig, error) {
 		if content == "" {
 			return nil, fmt.Errorf("task block #%d (%q) missing content", taskIndex, task.ID)
 		}
+		if task.Mode == "resume" && strings.TrimSpace(task.SessionID) == "" {
+			return nil, fmt.Errorf("task block #%d (%q) has empty session_id", taskIndex, task.ID)
+		}
 		if _, exists := seen[task.ID]; exists {
 			return nil, fmt.Errorf("task block #%d has duplicate id: %s", taskIndex, task.ID)
 		}
@@ -232,7 +235,10 @@ func parseArgs() (*Config, error) {
 			return nil, fmt.Errorf("resume mode requires: resume <session_id> <task>")
 		}
 		cfg.Mode = "resume"
-		cfg.SessionID = args[1]
+		cfg.SessionID = strings.TrimSpace(args[1])
+		if cfg.SessionID == "" {
+			return nil, fmt.Errorf("resume mode requires non-empty session_id")
+		}
 		cfg.Task = args[2]
 		cfg.ExplicitStdin = (args[2] == "-")
 		if len(args) > 3 {

codeagent-wrapper/executor.go

@@ -509,23 +509,43 @@ func generateFinalOutput(results []TaskResult) string {
 }
 
 func buildCodexArgs(cfg *Config, targetArg string) []string {
-	if cfg.Mode == "resume" {
-		return []string{
-			"e",
-			"--skip-git-repo-check",
-			"--json",
-			"resume",
-			cfg.SessionID,
-			targetArg,
+	if cfg == nil {
+		panic("buildCodexArgs: nil config")
+	}
+
+	var resumeSessionID string
+	isResume := cfg.Mode == "resume"
+	if isResume {
+		resumeSessionID = strings.TrimSpace(cfg.SessionID)
+		if resumeSessionID == "" {
+			logError("invalid config: resume mode requires non-empty session_id")
+			isResume = false
 		}
 	}
-	return []string{
-		"e",
-		"--skip-git-repo-check",
+
+	args := []string{"e"}
+
+	if envFlagEnabled("CODEX_BYPASS_SANDBOX") {
+		logWarn("CODEX_BYPASS_SANDBOX=true: running without approval/sandbox protection")
+		args = append(args, "--dangerously-bypass-approvals-and-sandbox")
+	}
+
+	args = append(args, "--skip-git-repo-check")
+
+	if isResume {
+		return append(args,
+			"--json",
+			"resume",
+			resumeSessionID,
+			targetArg,
+		)
+	}
+
+	return append(args,
 		"-C", cfg.WorkDir,
 		"--json",
 		targetArg,
-	}
+	)
 }
 
 func runCodexTask(taskSpec TaskSpec, silent bool, timeoutSec int) TaskResult {
@@ -576,6 +596,12 @@ func runCodexTaskWithContext(parentCtx context.Context, taskSpec TaskSpec, backe
 		cfg.WorkDir = defaultWorkdir
 	}
 
+	if cfg.Mode == "resume" && strings.TrimSpace(cfg.SessionID) == "" {
+		result.ExitCode = 1
+		result.Error = "resume mode requires non-empty session_id"
+		return result
+	}
+
 	useStdin := taskSpec.UseStdin
 	targetArg := taskSpec.Task
 	if useStdin {
@@ -745,6 +771,10 @@ func runCodexTaskWithContext(parentCtx context.Context, taskSpec TaskSpec, backe
 			default:
 			}
 		})
+		select {
+		case completeSeen <- struct{}{}:
+		default:
+		}
 		parseCh <- parseResult{message: msg, threadID: tid}
 	}()
 

codeagent-wrapper/executor_concurrent_test.go

@@ -10,6 +10,7 @@ import (
 	"os"
 	"os/exec"
 	"path/filepath"
+	"slices"
 	"strings"
 	"sync"
 	"sync/atomic"
@@ -244,6 +245,10 @@ func TestExecutorHelperCoverage(t *testing.T) {
 	})
 
 	t.Run("generateFinalOutputAndArgs", func(t *testing.T) {
+		const key = "CODEX_BYPASS_SANDBOX"
+		t.Cleanup(func() { os.Unsetenv(key) })
+		os.Unsetenv(key)
+
 		out := generateFinalOutput([]TaskResult{
 			{TaskID: "ok", ExitCode: 0},
 			{TaskID: "fail", ExitCode: 1, Error: "boom"},
@@ -257,11 +262,11 @@ func TestExecutorHelperCoverage(t *testing.T) {
 		}
 
 		args := buildCodexArgs(&Config{Mode: "new", WorkDir: "/tmp"}, "task")
-		if len(args) == 0 || args[3] != "/tmp" {
+		if !slices.Equal(args, []string{"e", "--skip-git-repo-check", "-C", "/tmp", "--json", "task"}) {
 			t.Fatalf("unexpected codex args: %+v", args)
 		}
 		args = buildCodexArgs(&Config{Mode: "resume", SessionID: "sess"}, "target")
-		if args[3] != "resume" || args[4] != "sess" {
+		if !slices.Equal(args, []string{"e", "--skip-git-repo-check", "--json", "resume", "sess", "target"}) {
 			t.Fatalf("unexpected resume args: %+v", args)
 		}
 	})
@@ -298,6 +303,18 @@ func TestExecutorRunCodexTaskWithContext(t *testing.T) {
 	origRunner := newCommandRunner
 	defer func() { newCommandRunner = origRunner }()
 
+	t.Run("resumeMissingSessionID", func(t *testing.T) {
+		newCommandRunner = func(ctx context.Context, name string, args ...string) commandRunner {
+			t.Fatalf("unexpected command execution for invalid resume config")
+			return nil
+		}
+
+		res := runCodexTaskWithContext(context.Background(), TaskSpec{Task: "payload", WorkDir: ".", Mode: "resume"}, nil, nil, false, false, 1)
+		if res.ExitCode == 0 || !strings.Contains(res.Error, "session_id") {
+			t.Fatalf("expected validation error, got %+v", res)
+		}
+	})
+
 	t.Run("success", func(t *testing.T) {
 		var firstStdout *reasonReadCloser
 		newCommandRunner = func(ctx context.Context, name string, args ...string) commandRunner {

codeagent-wrapper/main_test.go

@@ -1038,6 +1038,8 @@ func TestBackendParseArgs_ResumeMode(t *testing.T) {
 		},
 		{name: "resume missing session_id", args: []string{"codeagent-wrapper", "resume"}, wantErr: true},
 		{name: "resume missing task", args: []string{"codeagent-wrapper", "resume", "session-123"}, wantErr: true},
+		{name: "resume empty session_id", args: []string{"codeagent-wrapper", "resume", "", "task"}, wantErr: true},
+		{name: "resume whitespace session_id", args: []string{"codeagent-wrapper", "resume", "   ", "task"}, wantErr: true},
 	}
 
 	for _, tt := range tests {
@@ -1254,6 +1256,18 @@ do something`
 	}
 }
 
+func TestParallelParseConfig_EmptySessionID(t *testing.T) {
+	input := `---TASK---
+id: task-1
+session_id:
+---CONTENT---
+do something`
+
+	if _, err := parseParallelConfig([]byte(input)); err == nil {
+		t.Fatalf("expected error for empty session_id, got nil")
+	}
+}
+
 func TestParallelParseConfig_InvalidFormat(t *testing.T) {
 	if _, err := parseParallelConfig([]byte("invalid format")); err == nil {
 		t.Fatalf("expected error for invalid format, got nil")
@@ -1354,9 +1368,19 @@ func TestRunShouldUseStdin(t *testing.T) {
 }
 
 func TestRunBuildCodexArgs_NewMode(t *testing.T) {
+	const key = "CODEX_BYPASS_SANDBOX"
+	t.Cleanup(func() { os.Unsetenv(key) })
+	os.Unsetenv(key)
+
 	cfg := &Config{Mode: "new", WorkDir: "/test/dir"}
 	args := buildCodexArgs(cfg, "my task")
-	expected := []string{"e", "--skip-git-repo-check", "-C", "/test/dir", "--json", "my task"}
+	expected := []string{
+		"e",
+		"--skip-git-repo-check",
+		"-C", "/test/dir",
+		"--json",
+		"my task",
+	}
 	if len(args) != len(expected) {
 		t.Fatalf("len mismatch")
 	}
@@ -1368,9 +1392,20 @@ func TestRunBuildCodexArgs_NewMode(t *testing.T) {
 }
 
 func TestRunBuildCodexArgs_ResumeMode(t *testing.T) {
+	const key = "CODEX_BYPASS_SANDBOX"
+	t.Cleanup(func() { os.Unsetenv(key) })
+	os.Unsetenv(key)
+
 	cfg := &Config{Mode: "resume", SessionID: "session-abc"}
 	args := buildCodexArgs(cfg, "-")
-	expected := []string{"e", "--skip-git-repo-check", "--json", "resume", "session-abc", "-"}
+	expected := []string{
+		"e",
+		"--skip-git-repo-check",
+		"--json",
+		"resume",
+		"session-abc",
+		"-",
+	}
 	if len(args) != len(expected) {
 		t.Fatalf("len mismatch")
 	}
@@ -1381,6 +1416,61 @@ func TestRunBuildCodexArgs_ResumeMode(t *testing.T) {
 	}
 }
 
+func TestRunBuildCodexArgs_ResumeMode_EmptySessionHandledGracefully(t *testing.T) {
+	const key = "CODEX_BYPASS_SANDBOX"
+	t.Cleanup(func() { os.Unsetenv(key) })
+	os.Unsetenv(key)
+
+	cfg := &Config{Mode: "resume", SessionID: "   ", WorkDir: "/test/dir"}
+	args := buildCodexArgs(cfg, "task")
+	expected := []string{"e", "--skip-git-repo-check", "-C", "/test/dir", "--json", "task"}
+	if len(args) != len(expected) {
+		t.Fatalf("len mismatch")
+	}
+	for i := range args {
+		if args[i] != expected[i] {
+			t.Fatalf("args[%d]=%s, want %s", i, args[i], expected[i])
+		}
+	}
+}
+
+func TestRunBuildCodexArgs_BypassSandboxEnvTrue(t *testing.T) {
+	defer resetTestHooks()
+	tempDir := t.TempDir()
+	t.Setenv("TMPDIR", tempDir)
+
+	logger, err := NewLogger()
+	if err != nil {
+		t.Fatalf("NewLogger() error = %v", err)
+	}
+	setLogger(logger)
+	defer closeLogger()
+
+	t.Setenv("CODEX_BYPASS_SANDBOX", "true")
+
+	cfg := &Config{Mode: "new", WorkDir: "/test/dir"}
+	args := buildCodexArgs(cfg, "my task")
+	found := false
+	for _, arg := range args {
+		if arg == "--dangerously-bypass-approvals-and-sandbox" {
+			found = true
+			break
+		}
+	}
+	if !found {
+		t.Fatalf("expected bypass flag in args, got %v", args)
+	}
+
+	logger.Flush()
+	data, err := os.ReadFile(logger.Path())
+	if err != nil {
+		t.Fatalf("failed to read log file: %v", err)
+	}
+	if !strings.Contains(string(data), "CODEX_BYPASS_SANDBOX=true") {
+		t.Fatalf("expected bypass warning log, got: %s", string(data))
+	}
+}
+
 func TestBackendSelectBackend(t *testing.T) {
 	tests := []struct {
 		name string
@@ -1436,7 +1526,13 @@ func TestBackendBuildArgs_CodexBackend(t *testing.T) {
 	backend := CodexBackend{}
 	cfg := &Config{Mode: "new", WorkDir: "/test/dir"}
 	got := backend.BuildArgs(cfg, "task")
-	want := []string{"e", "--skip-git-repo-check", "-C", "/test/dir", "--json", "task"}
+	want := []string{
+		"e",
+		"--skip-git-repo-check",
+		"-C", "/test/dir",
+		"--json",
+		"task",
+	}
 	if len(got) != len(want) {
 		t.Fatalf("length mismatch")
 	}
@@ -1451,7 +1547,7 @@ func TestBackendBuildArgs_ClaudeBackend(t *testing.T) {
 	backend := ClaudeBackend{}
 	cfg := &Config{Mode: "new", WorkDir: defaultWorkdir}
 	got := backend.BuildArgs(cfg, "todo")
-	want := []string{"-p", "--dangerously-skip-permissions", "--setting-sources", "", "--output-format", "stream-json", "--verbose", "todo"}
+	want := []string{"-p", "--setting-sources", "", "--output-format", "stream-json", "--verbose", "todo"}
 	if len(got) != len(want) {
 		t.Fatalf("length mismatch")
 	}
@@ -1472,7 +1568,7 @@ func TestClaudeBackendBuildArgs_OutputValidation(t *testing.T) {
 	target := "ensure-flags"
 
 	args := backend.BuildArgs(cfg, target)
-	expectedPrefix := []string{"-p", "--dangerously-skip-permissions", "--setting-sources", "", "--output-format", "stream-json", "--verbose"}
+	expectedPrefix := []string{"-p", "--setting-sources", "", "--output-format", "stream-json", "--verbose"}
 
 	if len(args) != len(expectedPrefix)+1 {
 		t.Fatalf("args length=%d, want %d", len(args), len(expectedPrefix)+1)