diff --git a/cmd/sgai/actions.go b/cmd/sgai/actions.go index 5ae1f99..e5bb325 100644 --- a/cmd/sgai/actions.go +++ b/cmd/sgai/actions.go @@ -30,7 +30,19 @@ type parsedAction struct { } func loadActionConfigs(workspacePath string) ([]actionConfig, error) { - config, errLoad := loadProjectConfig(workspacePath) + return loadActionConfigsFromConfigPath(workspacePath, "") +} + +func loadActionConfigsFromConfigPath(workspacePath, configPath string) ([]actionConfig, error) { + var ( + config *projectConfig + errLoad error + ) + if strings.TrimSpace(configPath) == "" { + config, errLoad = loadProjectConfig(workspacePath) + } else { + config, errLoad = loadProjectConfigPath(configPath) + } if errLoad != nil { return nil, errLoad } diff --git a/cmd/sgai/config.go b/cmd/sgai/config.go index cfebf27..36c6cef 100644 --- a/cmd/sgai/config.go +++ b/cmd/sgai/config.go @@ -54,30 +54,36 @@ type projectConfig struct { } func loadProjectConfig(dir string) (*projectConfig, error) { - configPath := filepath.Join(dir, configFileName) + return loadProjectConfigFile(filepath.Join(dir, configFileName), true) +} - data, err := os.ReadFile(configPath) - if err != nil { - if os.IsNotExist(err) { - return nil, nil - } - if os.IsPermission(err) { - return nil, fmt.Errorf("permission denied reading config file: %s", configPath) +func loadProjectConfigPath(configPath string) (*projectConfig, error) { + return loadProjectConfigFile(configPath, false) +} + +func loadProjectConfigFile(configPath string, allowMissing bool) (*projectConfig, error) { + data, errRead := os.ReadFile(configPath) + if errRead != nil { + if os.IsNotExist(errRead) { + if allowMissing { + return nil, nil + } + return nil, fmt.Errorf("reading config file %s: %w", configPath, errRead) } - return nil, fmt.Errorf("reading config file %s: %w", configPath, err) + return nil, fmt.Errorf("reading config file %s: %w", configPath, errRead) } var config projectConfig - if err := json.Unmarshal(data, &config); err != nil { - if errSyntax, ok := err.(*json.SyntaxError); ok { + if errUnmarshal := json.Unmarshal(data, &config); errUnmarshal != nil { + if errSyntax, ok := errUnmarshal.(*json.SyntaxError); ok { return nil, fmt.Errorf("invalid JSON syntax in config file %s at offset %d: %w", - configPath, errSyntax.Offset, err) + configPath, errSyntax.Offset, errUnmarshal) } - if errUnmarshal, ok := err.(*json.UnmarshalTypeError); ok { - return nil, fmt.Errorf("invalid JSON type in config file %s at field %s: expected %s, got %s", - configPath, errUnmarshal.Field, errUnmarshal.Type, errUnmarshal.Value) + if errType, ok := errUnmarshal.(*json.UnmarshalTypeError); ok { + return nil, fmt.Errorf("invalid JSON type in config file %s at field %s: expected %s, got %s: %w", + configPath, errType.Field, errType.Type, errType.Value, errUnmarshal) } - return nil, fmt.Errorf("parsing config file %s: %w", configPath, err) + return nil, fmt.Errorf("parsing config file %s: %w", configPath, errUnmarshal) } return &config, nil diff --git a/cmd/sgai/config_test.go b/cmd/sgai/config_test.go index d16f08b..f0550db 100644 --- a/cmd/sgai/config_test.go +++ b/cmd/sgai/config_test.go @@ -172,6 +172,35 @@ func TestLoadProjectConfig(t *testing.T) { } } +func TestLoadProjectConfigPathWrapsPermissionError(t *testing.T) { + configPath := filepath.Join(t.TempDir(), configFileName) + require.NoError(t, os.WriteFile(configPath, []byte(`{"defaultModel":"test"}`), 0o600)) + require.NoError(t, os.Chmod(configPath, 0)) + t.Cleanup(func() { + _ = os.Chmod(configPath, 0o600) + }) + + _, errLoad := loadProjectConfigPath(configPath) + + require.Error(t, errLoad) + assert.Contains(t, errLoad.Error(), configPath) + assert.ErrorIs(t, errLoad, os.ErrPermission) +} + +func TestLoadProjectConfigPathWrapsJSONTypeError(t *testing.T) { + configPath := filepath.Join(t.TempDir(), configFileName) + require.NoError(t, os.WriteFile(configPath, []byte(`{"actions":"bad"}`), 0o644)) + + _, errLoad := loadProjectConfigPath(configPath) + + require.Error(t, errLoad) + assert.Contains(t, errLoad.Error(), "invalid JSON type") + var errType *json.UnmarshalTypeError + require.ErrorAs(t, errLoad, &errType) + require.NotNil(t, errType) + assert.Equal(t, "actions", errType.Field) +} + func TestValidateProjectConfig(t *testing.T) { tests := []struct { name string diff --git a/cmd/sgai/main.go b/cmd/sgai/main.go index 0dd00f4..09dd63d 100644 --- a/cmd/sgai/main.go +++ b/cmd/sgai/main.go @@ -61,6 +61,9 @@ func main() { case "help", "-h", "--help": printUsage() return + case "run": + os.Exit(cmdRun(os.Args[2:])) + return case "serve": cmdServe(os.Args[2:]) return @@ -83,7 +86,7 @@ func configureSgaiLogger(w io.Writer) { func requiresOpencode(subcommand string) bool { switch subcommand { - case "help", "-h", "--help", "internal-mcp": + case "help", "-h", "--help", "internal-mcp", "run": return false default: return true @@ -94,16 +97,19 @@ func printUsage() { fmt.Println(`sgai - AI-powered software factory Usage: - sgai [--listen-addr addr] Start web server (default) + sgai [--listen-addr addr] Start web server (default) + sgai run [--config path] [--var key=value] Options: - --listen-addr HTTP server listen address (default: 127.0.0.1:8080) + --listen-addr HTTP server listen address (default: 127.0.0.1:8080) Examples: - sgai - Start web UI on localhost:8080 - sgai --listen-addr 0.0.0.0:8080 - Start web UI accessible externally`) + sgai + Start web UI on localhost:8080 + sgai run --config ./verification/sgai.json --var Name=Ada Summarize + Run a configured action from the CLI + sgai --listen-addr 0.0.0.0:8080 + Start web UI accessible externally`) } // runWorkflow executes the main workflow loop for a target directory. @@ -1488,12 +1494,36 @@ func terminateProcessGroupOnCancel(ctx context.Context, cmd *exec.Cmd, processEx case <-processExited: return } + terminateProcessGroup(cmd, processExited, waitForGracefulProcessExit, syscall.Kill) +} + +func terminateProcessGroup(cmd *exec.Cmd, processExited <-chan struct{}, waitForExit func(<-chan struct{}) bool, signalProcessGroup func(int, syscall.Signal) error) { + if cmd.Process == nil || processGroupExited(processExited) { + return + } pgid := -cmd.Process.Pid - _ = syscall.Kill(pgid, syscall.SIGTERM) + _ = signalProcessGroup(pgid, syscall.SIGTERM) + if waitForExit(processExited) || processGroupExited(processExited) { + return + } + _ = signalProcessGroup(pgid, syscall.SIGKILL) +} + +func waitForGracefulProcessExit(processExited <-chan struct{}) bool { select { + case <-processExited: + return true case <-time.After(gracefulShutdownTimeout): - _ = syscall.Kill(pgid, syscall.SIGKILL) + return processGroupExited(processExited) + } +} + +func processGroupExited(processExited <-chan struct{}) bool { + select { case <-processExited: + return true + default: + return false } } diff --git a/cmd/sgai/main_test.go b/cmd/sgai/main_test.go index fd6314d..10997c0 100644 --- a/cmd/sgai/main_test.go +++ b/cmd/sgai/main_test.go @@ -14,6 +14,7 @@ import ( "runtime" "strings" "sync" + "syscall" "testing" "time" @@ -5214,20 +5215,59 @@ func TestHandleCompleteStatusCoordinatorNoBlockers(t *testing.T) { assert.Equal(t, state.StatusComplete, result.Status) } -func TestTerminateProcessGroupOnCancelWithContext(t *testing.T) { - ctx, cancel := context.WithCancel(context.Background()) - cmd := exec.Command("sleep", "30") - require.NoError(t, cmd.Start()) +func TestTerminateProcessGroupUsesProcessGroupID(t *testing.T) { + exited := make(chan struct{}) + var pids []int + var signals []syscall.Signal + + cmd := exec.Command("true") + cmd.Process = &os.Process{Pid: 42} + terminateProcessGroup(cmd, exited, func(<-chan struct{}) bool { + close(exited) + return false + }, func(pid int, sig syscall.Signal) error { + pids = append(pids, pid) + signals = append(signals, sig) + return nil + }) + assert.Equal(t, []int{-42}, pids) + assert.Equal(t, []syscall.Signal{syscall.SIGTERM}, signals) +} + +func TestTerminateProcessGroupSkipsSignalsWhenProcessAlreadyExited(t *testing.T) { exited := make(chan struct{}) - go func() { - _ = cmd.Wait() + close(exited) + + var signals []syscall.Signal + cmd := exec.Command("true") + cmd.Process = &os.Process{Pid: 42} + terminateProcessGroup(cmd, exited, func(<-chan struct{}) bool { + t.Fatal("unexpected grace-period wait") + return false + }, func(_ int, sig syscall.Signal) error { + signals = append(signals, sig) + return nil + }) + + assert.Empty(t, signals) +} + +func TestTerminateProcessGroupSkipsEscalationAfterProcessExit(t *testing.T) { + exited := make(chan struct{}) + + var signals []syscall.Signal + cmd := exec.Command("true") + cmd.Process = &os.Process{Pid: 42} + terminateProcessGroup(cmd, exited, func(<-chan struct{}) bool { close(exited) - }() + return false + }, func(_ int, sig syscall.Signal) error { + signals = append(signals, sig) + return nil + }) - cancel() - terminateProcessGroupOnCancel(ctx, cmd, exited) - <-exited + assert.Equal(t, []syscall.Signal{syscall.SIGTERM}, signals) } func TestExportSessionMissingBinary(t *testing.T) { diff --git a/cmd/sgai/run.go b/cmd/sgai/run.go new file mode 100644 index 0000000..ac83b81 --- /dev/null +++ b/cmd/sgai/run.go @@ -0,0 +1,221 @@ +package main + +import ( + "bufio" + "context" + "errors" + "flag" + "fmt" + "io" + "os" + "os/exec" + "os/signal" + "path/filepath" + "strings" + "syscall" +) + +type cliRunDeps struct { + workingDir func() (string, error) + runPromptAction func(context.Context, *actionExecutionPlan, io.Writer, io.Writer) error + runScriptAction func(context.Context, *actionExecutionPlan, io.Writer, io.Writer) error +} + +type actionVarFlagValues struct { + values map[string]string +} + +func (v *actionVarFlagValues) String() string { + return "" +} + +func (v *actionVarFlagValues) Set(value string) error { + key, val, found := strings.Cut(value, "=") + if !found || strings.TrimSpace(key) == "" { + return errors.New("--var must use key=value") + } + if v.values == nil { + v.values = make(map[string]string) + } + v.values[key] = val + return nil +} + +func (v *actionVarFlagValues) clone() map[string]string { + return cloneActionValues(v.values) +} + +func cmdRun(args []string) int { + ctx, stop := signal.NotifyContext(context.Background(), os.Interrupt, syscall.SIGTERM) + defer stop() + return runActionSubcommand(ctx, args, os.Stdin, os.Stdout, os.Stderr, defaultCLIRunDeps()) +} + +func defaultCLIRunDeps() cliRunDeps { + return cliRunDeps{ + workingDir: os.Getwd, + runPromptAction: runPromptActionCLI, + runScriptAction: runScriptActionCLI, + } +} + +func runActionSubcommand(ctx context.Context, args []string, stdin io.Reader, stdout, stderr io.Writer, deps cliRunDeps) int { + stderr = normalizeCLIWriter(stderr) + errRun := runActionCommand(ctx, args, stdin, stdout, stderr, deps) + switch { + case errRun == nil, errors.Is(errRun, flag.ErrHelp): + return 0 + case errors.Is(errRun, context.Canceled): + return 130 + default: + _, _ = fmt.Fprintln(stderr, errRun) + return 1 + } +} + +func runActionCommand(ctx context.Context, args []string, stdin io.Reader, stdout, stderr io.Writer, deps cliRunDeps) error { + deps = fillCLIRunDeps(deps) + stderr = normalizeCLIWriter(stderr) + stdout = normalizeCLIWriter(stdout) + + runFlags := flag.NewFlagSet("run", flag.ContinueOnError) + runFlags.SetOutput(stderr) + runFlags.Usage = func() { + _, _ = fmt.Fprintln(stderr, "Usage: sgai run [--config path/to/sgai.json] [--var key=value ...] ") + } + + configPath := runFlags.String("config", "", "Path to sgai.json") + var values actionVarFlagValues + runFlags.Var(&values, "var", "Action variable as key=value; repeat as needed") + + if errParse := runFlags.Parse(args); errParse != nil { + return fmt.Errorf("parse run flags: %w", errParse) + } + + remaining := runFlags.Args() + if len(remaining) != 1 { + runFlags.Usage() + return errors.New("run requires exactly one action name") + } + + workspacePath, resolvedConfigPath, errPaths := resolveRunPaths(deps.workingDir, *configPath) + if errPaths != nil { + return errPaths + } + + reader := bufio.NewReader(stdin) + plan, errPrepare := prepareActionExecution(workspacePath, resolvedConfigPath, remaining[0], values.clone(), func(name string) (string, error) { + return promptForActionVariable(reader, stderr, name) + }) + if errPrepare != nil { + return errPrepare + } + + if plan.parsed.kind == actionKindPrompt { + return deps.runPromptAction(ctx, &plan, stdout, stderr) + } + return deps.runScriptAction(ctx, &plan, stdout, stderr) +} + +func fillCLIRunDeps(deps cliRunDeps) cliRunDeps { + if deps.workingDir == nil { + deps.workingDir = os.Getwd + } + if deps.runPromptAction == nil { + deps.runPromptAction = runPromptActionCLI + } + if deps.runScriptAction == nil { + deps.runScriptAction = runScriptActionCLI + } + return deps +} + +func normalizeCLIWriter(w io.Writer) io.Writer { + if w != nil { + return w + } + return io.Discard +} + +func resolveRunPaths(workingDir func() (string, error), configPath string) (workspacePath, resolvedConfigPath string, err error) { + workspacePath, errWorkingDir := workingDir() + if errWorkingDir != nil { + return "", "", fmt.Errorf("get working directory: %w", errWorkingDir) + } + if strings.TrimSpace(configPath) == "" { + return workspacePath, "", nil + } + + resolvedConfigPath = configPath + if !filepath.IsAbs(resolvedConfigPath) { + resolvedConfigPath = filepath.Join(workspacePath, resolvedConfigPath) + } + resolvedConfigPath = filepath.Clean(resolvedConfigPath) + return filepath.Dir(resolvedConfigPath), resolvedConfigPath, nil +} + +func promptForActionVariable(reader *bufio.Reader, out io.Writer, name string) (string, error) { + if _, errWrite := fmt.Fprintf(out, "%s: ", name); errWrite != nil { + return "", fmt.Errorf("write prompt: %w", errWrite) + } + line, errRead := reader.ReadString('\n') + if errRead != nil && !errors.Is(errRead, io.EOF) { + return "", fmt.Errorf("read prompt: %w", errRead) + } + if errRead != nil && line == "" { + return "", fmt.Errorf("read prompt: %w", errRead) + } + return strings.TrimRight(line, "\r\n"), nil +} + +func runPromptActionCLI(ctx context.Context, plan *actionExecutionPlan, stdout, stderr io.Writer) error { + spec := buildPromptActionCommandSpec(plan.workspacePath, plan.rendered, plan.parsed.model) + return runCLIAction(ctx, plan.workspacePath, &spec, stdout, stderr) +} + +func runScriptActionCLI(ctx context.Context, plan *actionExecutionPlan, stdout, stderr io.Writer) error { + spec, errSpec := buildScriptActionCommandSpec(plan.argv) + if errSpec != nil { + return fmt.Errorf("action %q %w", plan.actionName, errSpec) + } + return runCLIAction(ctx, plan.workspacePath, &spec, stdout, stderr) +} + +func runCLIAction(ctx context.Context, workspacePath string, spec *actionCommandSpec, stdout, stderr io.Writer) error { + if errContext := ctx.Err(); errContext != nil { + return fmt.Errorf("check command context: %w", errContext) + } + + cmd := exec.Command(spec.command, spec.args...) + cmd.Dir = workspacePath + sysProcAttr := new(syscall.SysProcAttr) + sysProcAttr.Setpgid = true + cmd.SysProcAttr = sysProcAttr + if len(spec.env) > 0 { + cmd.Env = opencodeEnv(spec.env...) + } + if spec.stdin != "" { + cmd.Stdin = strings.NewReader(spec.stdin) + } + cmd.Stdout = stdout + cmd.Stderr = stderr + if errStart := cmd.Start(); errStart != nil { + return fmt.Errorf("running %s: %w", spec.command, errStart) + } + + processExited := make(chan struct{}) + go terminateProcessGroupOnCancel(ctx, cmd, processExited) + + errRun := cmd.Wait() + close(processExited) + if errContext := ctx.Err(); errContext != nil { + if errRun == nil { + return fmt.Errorf("running %s: %w", spec.command, errContext) + } + return fmt.Errorf("running %s: %w", spec.command, errors.Join(errContext, errRun)) + } + if errRun != nil { + return fmt.Errorf("running %s: %w", spec.command, errRun) + } + return nil +} diff --git a/cmd/sgai/run_test.go b/cmd/sgai/run_test.go new file mode 100644 index 0000000..a403718 --- /dev/null +++ b/cmd/sgai/run_test.go @@ -0,0 +1,364 @@ +package main + +import ( + "bytes" + "context" + "io" + "os" + "path/filepath" + "strconv" + "strings" + "syscall" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func newRunTestProjectConfig(actions ...actionConfig) projectConfig { + config := newTestProjectConfig() + config.Actions = actions + return config +} + +func newRunTestScriptActionConfig(name, script string) actionConfig { + config := newTestActionConfig() + config.Name = name + config.Script = script + return config +} + +func newRunTestPromptActionConfig(name, model, prompt string) actionConfig { + config := newTestActionConfig() + config.Name = name + config.Model = model + config.Prompt = prompt + return config +} + +func newRunTestDeps(workingDir func() (string, error)) cliRunDeps { + deps := defaultCLIRunDeps() + deps.workingDir = workingDir + return deps +} + +func TestRunActionCommandUsesExplicitConfigPath(t *testing.T) { + configDir := t.TempDir() + require.NoError(t, os.MkdirAll(filepath.Join(configDir, ".sgai"), 0o755)) + writeActionTestConfig(t, configDir, newRunTestProjectConfig(newRunTestScriptActionConfig("Print", `printf "%s" "{{ .Message }}"`))) + + var gotPlan actionExecutionPlan + deps := newRunTestDeps(func() (string, error) { + return filepath.Join(t.TempDir(), "ignored-working-dir"), nil + }) + deps.runPromptAction = func(context.Context, *actionExecutionPlan, io.Writer, io.Writer) error { + t.Fatalf("unexpected prompt action dispatch") + return nil + } + deps.runScriptAction = func(_ context.Context, plan *actionExecutionPlan, _, _ io.Writer) error { + gotPlan = *plan + return nil + } + + errRun := runActionCommand(t.Context(), []string{"--config", filepath.Join(configDir, configFileName), "--var", "Message=hello world", "Print"}, bytes.NewBuffer(nil), io.Discard, io.Discard, deps) + require.NoError(t, errRun) + assert.Equal(t, configDir, gotPlan.workspacePath) + assert.Equal(t, "Print", gotPlan.actionName) + assert.Equal(t, actionKindScript, gotPlan.parsed.kind) + assert.Equal(t, []string{"printf", "%s", "hello world"}, gotPlan.argv) +} + +func TestRunActionCommandUsesWorkspaceConfigWhenConfigIsOmitted(t *testing.T) { + workingDir := t.TempDir() + require.NoError(t, os.MkdirAll(filepath.Join(workingDir, ".sgai"), 0o755)) + writeActionTestConfig(t, workingDir, newRunTestProjectConfig(newRunTestScriptActionConfig("Print", `printf "%s" "{{ .Message }}"`))) + + var gotPlan actionExecutionPlan + deps := newRunTestDeps(func() (string, error) { + return workingDir, nil + }) + deps.runPromptAction = func(context.Context, *actionExecutionPlan, io.Writer, io.Writer) error { + t.Fatalf("unexpected prompt action dispatch") + return nil + } + deps.runScriptAction = func(_ context.Context, plan *actionExecutionPlan, _, _ io.Writer) error { + gotPlan = *plan + return nil + } + + errRun := runActionCommand(t.Context(), []string{"--var", "Message=hello from workspace", "Print"}, bytes.NewBuffer(nil), io.Discard, io.Discard, deps) + require.NoError(t, errRun) + assert.Equal(t, workingDir, gotPlan.workspacePath) + assert.Equal(t, "Print", gotPlan.actionName) + assert.Equal(t, actionKindScript, gotPlan.parsed.kind) + assert.Equal(t, []string{"printf", "%s", "hello from workspace"}, gotPlan.argv) +} + +func TestRunActionCommandAcceptsRepeatedVarFlags(t *testing.T) { + configDir := t.TempDir() + require.NoError(t, os.MkdirAll(filepath.Join(configDir, ".sgai"), 0o755)) + writeActionTestConfig(t, configDir, newRunTestProjectConfig(newRunTestScriptActionConfig("Print", `printf "%s/%s" "{{ .Greeting }}" "{{ .Target }}"`))) + + var gotPlan actionExecutionPlan + deps := newRunTestDeps(func() (string, error) { + return t.TempDir(), nil + }) + deps.runPromptAction = func(context.Context, *actionExecutionPlan, io.Writer, io.Writer) error { + t.Fatalf("unexpected prompt action dispatch") + return nil + } + deps.runScriptAction = func(_ context.Context, plan *actionExecutionPlan, _, _ io.Writer) error { + gotPlan = *plan + return nil + } + + errRun := runActionCommand(t.Context(), []string{"--config", filepath.Join(configDir, configFileName), "--var", "Greeting=hello", "--var", "Target=world", "Print"}, bytes.NewBuffer(nil), io.Discard, io.Discard, deps) + require.NoError(t, errRun) + assert.Equal(t, actionKindScript, gotPlan.parsed.kind) + assert.Equal(t, []string{"printf", "%s/%s", "hello", "world"}, gotPlan.argv) +} + +func TestRunActionCommandPromptsOnlyForMissingVariables(t *testing.T) { + configDir := t.TempDir() + require.NoError(t, os.MkdirAll(filepath.Join(configDir, ".sgai"), 0o755)) + writeActionTestConfig(t, configDir, newRunTestProjectConfig(newRunTestPromptActionConfig("Summarize", "openai/gpt-5.4", "hello {{ .Name }} about {{ .Topic }}"))) + + var gotPlan actionExecutionPlan + deps := newRunTestDeps(func() (string, error) { + return t.TempDir(), nil + }) + deps.runPromptAction = func(_ context.Context, plan *actionExecutionPlan, _, _ io.Writer) error { + gotPlan = *plan + return nil + } + deps.runScriptAction = func(context.Context, *actionExecutionPlan, io.Writer, io.Writer) error { + t.Fatalf("unexpected script action dispatch") + return nil + } + + var stderr bytes.Buffer + errRun := runActionCommand(t.Context(), []string{"--config", filepath.Join(configDir, configFileName), "--var", "Name=Ada", "Summarize"}, bytes.NewBufferString("compiler design\n"), io.Discard, &stderr, deps) + require.NoError(t, errRun) + assert.Equal(t, actionKindPrompt, gotPlan.parsed.kind) + assert.Equal(t, "hello Ada about compiler design", gotPlan.rendered) + assert.Contains(t, stderr.String(), "Topic") + assert.NotContains(t, stderr.String(), "Name") +} + +func TestRunActionCommandFallsBackToDefaultActionsWhenConfigIsAbsent(t *testing.T) { + workingDir := t.TempDir() + require.NoError(t, os.MkdirAll(filepath.Join(workingDir, ".sgai"), 0o755)) + + var gotPlan actionExecutionPlan + deps := newRunTestDeps(func() (string, error) { + return workingDir, nil + }) + deps.runPromptAction = func(_ context.Context, plan *actionExecutionPlan, _, _ io.Writer) error { + gotPlan = *plan + return nil + } + deps.runScriptAction = func(context.Context, *actionExecutionPlan, io.Writer, io.Writer) error { + t.Fatalf("unexpected script action dispatch") + return nil + } + + errRun := runActionCommand(t.Context(), []string{"Start Application"}, bytes.NewBuffer(nil), io.Discard, io.Discard, deps) + require.NoError(t, errRun) + assert.Equal(t, workingDir, gotPlan.workspacePath) + assert.Equal(t, "Start Application", gotPlan.actionName) + assert.Equal(t, actionKindPrompt, gotPlan.parsed.kind) + assert.Contains(t, gotPlan.rendered, "start the application server") +} + +func TestRunActionCommandRejectsUnknownAction(t *testing.T) { + workingDir := t.TempDir() + deps := newRunTestDeps(func() (string, error) { + return workingDir, nil + }) + + errRun := runActionCommand(t.Context(), []string{"Unknown Action"}, bytes.NewBuffer(nil), io.Discard, io.Discard, deps) + require.Error(t, errRun) + assert.Contains(t, errRun.Error(), `action "Unknown Action" not found`) +} + +func TestRunActionCommandRejectsInvalidConfig(t *testing.T) { + configDir := t.TempDir() + require.NoError(t, os.MkdirAll(filepath.Join(configDir, ".sgai"), 0o755)) + require.NoError(t, os.WriteFile(filepath.Join(configDir, configFileName), []byte("not valid json"), 0o644)) + + deps := newRunTestDeps(func() (string, error) { + return t.TempDir(), nil + }) + + errRun := runActionCommand(t.Context(), []string{"--config", filepath.Join(configDir, configFileName), "Anything"}, bytes.NewBuffer(nil), io.Discard, io.Discard, deps) + require.Error(t, errRun) + assert.Contains(t, errRun.Error(), "invalid JSON syntax") +} + +func TestRunActionCommandRejectsInvalidVarFlag(t *testing.T) { + deps := newRunTestDeps(func() (string, error) { + return t.TempDir(), nil + }) + + errRun := runActionCommand(t.Context(), []string{"--var", "missing-separator", "Anything"}, bytes.NewBuffer(nil), io.Discard, io.Discard, deps) + require.Error(t, errRun) + assert.Contains(t, errRun.Error(), "must use key=value") +} + +func TestRunActionSubcommandWritesFailureToStderrAndReturnsFailureExitCode(t *testing.T) { + var stderr bytes.Buffer + + exitCode := runActionSubcommand(t.Context(), nil, bytes.NewBuffer(nil), io.Discard, &stderr, cliRunDeps{ + workingDir: nil, + runPromptAction: nil, + runScriptAction: nil, + }) + + assert.Equal(t, 1, exitCode) + assert.Contains(t, stderr.String(), "Usage: sgai run") + assert.Contains(t, stderr.String(), "run requires exactly one action name") +} + +func TestRunActionSubcommandReturnsCancellationExitCodeWithoutWritingError(t *testing.T) { + configDir := t.TempDir() + require.NoError(t, os.MkdirAll(filepath.Join(configDir, ".sgai"), 0o755)) + writeActionTestConfig(t, configDir, newRunTestProjectConfig(newRunTestScriptActionConfig("Wait", `printf waiting`))) + + ctx, cancel := context.WithCancel(t.Context()) + cancel() + + deps := newRunTestDeps(func() (string, error) { + return t.TempDir(), nil + }) + deps.runPromptAction = func(context.Context, *actionExecutionPlan, io.Writer, io.Writer) error { + t.Fatalf("unexpected prompt action dispatch") + return nil + } + deps.runScriptAction = func(ctx context.Context, _ *actionExecutionPlan, _, _ io.Writer) error { + return ctx.Err() + } + + var stderr bytes.Buffer + exitCode := runActionSubcommand(ctx, []string{"--config", filepath.Join(configDir, configFileName), "Wait"}, bytes.NewBuffer(nil), io.Discard, &stderr, deps) + + assert.Equal(t, 130, exitCode) + assert.Empty(t, stderr.String()) +} + +func TestRunCLIActionStopsProcessGroupOnCancel(t *testing.T) { + workspacePath := t.TempDir() + scriptPath := writeRunCLIActionCancellationFixture(t, workspacePath) + groupPath := filepath.Join(workspacePath, "group.txt") + + ctx, cancel := context.WithCancel(t.Context()) + resultCh := make(chan error, 1) + go func() { + resultCh <- runCLIAction(ctx, workspacePath, &actionCommandSpec{ + command: scriptPath, + args: []string{groupPath}, + stdin: "", + env: nil, + }, io.Discard, io.Discard) + }() + + waitForRunTestPath(t, groupPath) + pgid := readRunTestProcessGroupID(t, groupPath) + t.Cleanup(func() { + killRunTestProcessGroup(pgid) + }) + + cancel() + errRun := waitForRunCLIActionResult(t, resultCh) + require.Error(t, errRun) + waitForRunTestProcessGroupExit(t, pgid) +} + +func TestRunActionCommandReturnsContextCanceledWhenScriptActionInterrupted(t *testing.T) { + workspacePath := t.TempDir() + require.NoError(t, os.MkdirAll(filepath.Join(workspacePath, ".sgai"), 0o755)) + scriptPath := writeRunCLIActionCancellationFixture(t, workspacePath) + groupPath := filepath.Join(workspacePath, "group.txt") + writeActionTestConfig(t, workspacePath, newRunTestProjectConfig(newRunTestScriptActionConfig("Wait", strconv.Quote(scriptPath)+" "+strconv.Quote(groupPath)))) + + ctx, cancel := context.WithCancel(t.Context()) + resultCh := make(chan error, 1) + go func() { + resultCh <- runActionCommand(ctx, []string{"--config", filepath.Join(workspacePath, configFileName), "Wait"}, bytes.NewBuffer(nil), io.Discard, io.Discard, defaultCLIRunDeps()) + }() + + waitForRunTestPath(t, groupPath) + pgid := readRunTestProcessGroupID(t, groupPath) + t.Cleanup(func() { + killRunTestProcessGroup(pgid) + }) + + cancel() + errRun := waitForRunCLIActionResult(t, resultCh) + require.Error(t, errRun) + require.ErrorIs(t, errRun, context.Canceled) + waitForRunTestProcessGroupExit(t, pgid) +} + +func writeRunCLIActionCancellationFixture(t *testing.T, dir string) string { + t.Helper() + + scriptPath := filepath.Join(dir, "run-cli-action-cancel.sh") + script := `#!/bin/sh +group_path="$1" + +sh -c 'while :; do sleep 1; done' & +child=$! +printf %s "$$" > "$group_path" +wait "$child" +` + require.NoError(t, os.WriteFile(scriptPath, []byte(script), 0o755)) + return scriptPath +} + +func waitForRunTestPath(t *testing.T, path string) { + t.Helper() + require.Eventually(t, func() bool { + _, errStat := os.Stat(path) + return errStat == nil + }, 2*time.Second, 10*time.Millisecond) +} + +func waitForRunCLIActionResult(t *testing.T, resultCh <-chan error) error { + t.Helper() + + select { + case errRun := <-resultCh: + return errRun + case <-time.After(2 * time.Second): + t.Fatal("timed out waiting for runCLIAction to return") + return nil + } +} + +func readRunTestProcessGroupID(t *testing.T, groupPath string) int { + t.Helper() + + data, errRead := os.ReadFile(groupPath) + require.NoError(t, errRead) + + pgid, errAtoi := strconv.Atoi(strings.TrimSpace(string(data))) + require.NoError(t, errAtoi) + require.NotZero(t, pgid) + return pgid +} + +func waitForRunTestProcessGroupExit(t *testing.T, pgid int) { + t.Helper() + require.Eventually(t, func() bool { + errKill := syscall.Kill(-pgid, 0) + return errKill == syscall.ESRCH + }, 2*time.Second, 10*time.Millisecond) +} + +func killRunTestProcessGroup(pgid int) { + if pgid == 0 { + return + } + _ = syscall.Kill(-pgid, syscall.SIGKILL) +} diff --git a/cmd/sgai/service_action.go b/cmd/sgai/service_action.go index d5705f1..78cab6b 100644 --- a/cmd/sgai/service_action.go +++ b/cmd/sgai/service_action.go @@ -3,56 +3,118 @@ package main import ( "errors" "fmt" + "maps" "strings" ) +type actionExecutionPlan struct { + workspacePath string + actionName string + parsed parsedAction + rendered string + argv []string +} + func (s *Server) actionRunService(workspacePath, actionName string, values map[string]string) adhocStartResult { + plan, errPrepare := prepareActionExecution(workspacePath, "", actionName, values, nil) + if errPrepare != nil { + return adhocStartError(errPrepare) + } + + if plan.parsed.kind == actionKindPrompt { + return s.runPromptAction(plan.workspacePath, plan.rendered, plan.parsed.model) + } + return s.runScriptAction(plan.workspacePath, plan.actionName, plan.argv) +} + +func findActionConfigByNameWithConfigPath(workspacePath, configPath, actionName string) (actionConfig, error) { + configs, errLoad := loadActionConfigsFromConfigPath(workspacePath, configPath) + if errLoad != nil { + return actionConfig{}, errLoad + } + nameErrors := actionIdentityErrors(configs) + for i, config := range configs { + if strings.TrimSpace(config.Name) == actionName { + if nameErrors[i] != nil { + return actionConfig{}, nameErrors[i] + } + return config, nil + } + } + return actionConfig{}, fmt.Errorf("action %q not found", actionName) +} + +func prepareActionExecution(workspacePath, configPath, actionName string, values map[string]string, promptForMissing func(string) (string, error)) (actionExecutionPlan, error) { actionName = strings.TrimSpace(actionName) if actionName == "" { - return adhocStartError(errors.New("action name is required")) + return actionExecutionPlan{}, errors.New("action name is required") } - config, errFind := findActionConfigByName(workspacePath, actionName) + config, errFind := findActionConfigByNameWithConfigPath(workspacePath, configPath, actionName) if errFind != nil { - return adhocStartError(errFind) + return actionExecutionPlan{}, errFind } parsed, errValidate := validateAndParseAction(&config) if errValidate != nil { - return adhocStartError(errValidate) + return actionExecutionPlan{}, errValidate } - rendered, errRender := renderParsedAction(&parsed, values) + resolvedValues, errResolve := resolveActionValues(&parsed, values, promptForMissing) + if errResolve != nil { + return actionExecutionPlan{}, errResolve + } + + rendered, errRender := renderParsedAction(&parsed, resolvedValues) if errRender != nil { - return adhocStartError(fmt.Errorf("action %q %w", actionName, errRender)) + return actionExecutionPlan{}, fmt.Errorf("action %q %w", actionName, errRender) + } + + plan := actionExecutionPlan{ + workspacePath: workspacePath, + actionName: actionName, + parsed: parsed, + rendered: rendered, + argv: nil, } - if parsed.kind == actionKindPrompt { - return s.runPromptAction(workspacePath, rendered, parsed.model) + if parsed.kind != actionKindScript { + return plan, nil } argv, errSplit := splitActionCommand(rendered) if errSplit != nil { - return adhocStartError(fmt.Errorf("action %q rendered an invalid command: %w", actionName, errSplit)) + return actionExecutionPlan{}, fmt.Errorf("action %q rendered an invalid command: %w", actionName, errSplit) } - return s.runScriptAction(workspacePath, actionName, argv) + plan.argv = argv + return plan, nil } -func findActionConfigByName(workspacePath, actionName string) (actionConfig, error) { - configs, errLoad := loadActionConfigs(workspacePath) - if errLoad != nil { - return actionConfig{}, errLoad +func resolveActionValues(parsed *parsedAction, values map[string]string, promptForMissing func(string) (string, error)) (map[string]string, error) { + resolvedValues := cloneActionValues(values) + if promptForMissing == nil { + return resolvedValues, nil } - nameErrors := actionIdentityErrors(configs) - for i, config := range configs { - if strings.TrimSpace(config.Name) == actionName { - if nameErrors[i] != nil { - return actionConfig{}, nameErrors[i] - } - return config, nil + for _, name := range parsed.variables { + if _, exists := resolvedValues[name]; exists { + continue } + value, errPrompt := promptForMissing(name) + if errPrompt != nil { + return nil, fmt.Errorf("prompting for %s: %w", name, errPrompt) + } + resolvedValues[name] = value } - return actionConfig{}, fmt.Errorf("action %q not found", actionName) + return resolvedValues, nil +} + +func cloneActionValues(values map[string]string) map[string]string { + if len(values) == 0 { + return map[string]string{} + } + cloned := make(map[string]string, len(values)) + maps.Copy(cloned, values) + return cloned } func splitActionCommand(command string) ([]string, error) { diff --git a/cmd/sgai/service_adhoc.go b/cmd/sgai/service_adhoc.go index ee08bbb..2415a2f 100644 --- a/cmd/sgai/service_adhoc.go +++ b/cmd/sgai/service_adhoc.go @@ -57,6 +57,29 @@ type commandStartSpec struct { alreadyRunningMessage string } +type actionCommandSpec struct { + command string + args []string + stdin string + env []string +} + +func buildPromptActionCommandSpec(workspacePath, prompt, model string) actionCommandSpec { + return actionCommandSpec{ + command: "opencode", + args: buildAdhocArgs(model), + stdin: prompt, + env: []string{"OPENCODE_CONFIG_DIR=" + filepath.Join(workspacePath, ".sgai")}, + } +} + +func buildScriptActionCommandSpec(argv []string) (actionCommandSpec, error) { + if len(argv) == 0 || strings.TrimSpace(argv[0]) == "" { + return actionCommandSpec{}, errors.New("rendered an empty command") + } + return actionCommandSpec{command: argv[0], args: argv[1:], stdin: "", env: nil}, nil +} + func (s *Server) adhocStartService(workspacePath, prompt, model string) adhocStartResult { prompt = strings.TrimSpace(prompt) model = strings.TrimSpace(model) @@ -64,14 +87,14 @@ func (s *Server) adhocStartService(workspacePath, prompt, model string) adhocSta return adhocStartError(errors.New("prompt and model are required")) } - args := buildAdhocArgs(model) + actionSpec := buildPromptActionCommandSpec(workspacePath, prompt, model) return s.startCommandService(workspacePath, &commandStartSpec{ - command: "opencode", - args: args, - stdin: prompt, - env: []string{"OPENCODE_CONFIG_DIR=" + filepath.Join(workspacePath, ".sgai")}, + command: actionSpec.command, + args: actionSpec.args, + stdin: actionSpec.stdin, + env: actionSpec.env, logLabel: "adhoc", - headerLines: []string{"$ opencode " + strings.Join(args, " "), "prompt: " + prompt}, + headerLines: []string{"$ opencode " + strings.Join(actionSpec.args, " "), "prompt: " + prompt}, startedMessage: "ad-hoc prompt started", alreadyRunningMessage: "ad-hoc prompt already running", }) @@ -88,14 +111,15 @@ func (s *Server) runScriptAction(workspacePath, actionName string, argv []string if s.scriptActionRunner != nil { return s.scriptActionRunner(workspacePath, actionName, argv) } - if len(argv) == 0 || strings.TrimSpace(argv[0]) == "" { - return adhocStartError(fmt.Errorf("action %q rendered an empty command", actionName)) + actionSpec, errSpec := buildScriptActionCommandSpec(argv) + if errSpec != nil { + return adhocStartError(fmt.Errorf("action %q %w", actionName, errSpec)) } return s.startCommandService(workspacePath, &commandStartSpec{ - command: argv[0], - args: argv[1:], - stdin: "", - env: nil, + command: actionSpec.command, + args: actionSpec.args, + stdin: actionSpec.stdin, + env: actionSpec.env, logLabel: "action", headerLines: []string{"action: " + actionName, formatCommandForLog(argv)}, startedMessage: "action started", diff --git a/cmd/sgai/skel/.sgai/skills/go-subprocess-lifecycle/SKILL.md b/cmd/sgai/skel/.sgai/skills/go-subprocess-lifecycle/SKILL.md new file mode 100644 index 0000000..9a781ff --- /dev/null +++ b/cmd/sgai/skel/.sgai/skills/go-subprocess-lifecycle/SKILL.md @@ -0,0 +1,67 @@ +--- +name: go-subprocess-lifecycle +description: Use when Go code starts external processes, detaches process groups, or needs cancellation and interrupt handling with os/exec. +--- + +# Go Subprocess Lifecycle + +## Overview + +Prevent orphaned subprocesses, lost cancellation errors, and signal-after-exit races when Go code uses `os/exec`. + +## When to Use + +- Writing or reviewing Go code that starts external commands with `os/exec` +- Using `CommandContext`, `Start`/`Wait`, detached process groups, or custom interrupt handling +- Mapping cancellation to caller-visible CLI behavior or exit codes +- Cleaning up child process trees on cancellation +- Don't use this skill for pure in-process concurrency without external commands + +## Process + +### 1. Decide who owns process lifetime +- If no custom cleanup or exit-code mapping is needed, `exec.CommandContext` may be sufficient. +- If you need process-group cleanup or must preserve `context.Canceled` at the API boundary, prefer explicit `exec.Command`, `Start`, `Wait`, and your own cancellation path. + +### 2. Preserve cancellation semantics at the boundary +- The caller-visible contract matters as much as process shutdown. +- If cancellation should still match `context.Canceled`, do not rely on `CommandContext` defaults that can replace it with an `*exec.ExitError`. +- Return an error chain that keeps `context.Canceled` visible to `errors.Is` at the top-level CLI or API boundary. + +### 3. Clean up detached process groups deliberately +- If you set `Setpgid: true` or otherwise detach subprocesses, add an explicit cleanup path. +- Start the command, wait in one goroutine or call path, and coordinate cancellation with process-group termination. +- Keep the cleanup helper private and directly testable. + +### 4. Re-check exit state before signaling or escalating +- Cancellation and natural process exit can race. +- Before sending `SIGTERM`, check whether the process already exited. +- Before escalating to `SIGKILL`, check again whether graceful shutdown already completed. +- Never assume that a pending cancellation means the child is still running. + +### 5. Write focused lifecycle regression tests +- Add one test for caller-visible cancellation behavior. +- Add one test for descendant cleanup when cancellation happens mid-run. +- Add one test for the no-signal-after-exit race. +- Prefer tests that exercise small private helpers directly when the race is otherwise hard to force from the public entrypoint. + +### 6. Verify the whole path +- Run focused Go tests for the lifecycle helpers and boundary behavior first. +- Then run the repository verification required by the project, including lint/build/test commands. +- If the feature is user-facing, add one manual check that proves the final CLI or API behavior matches the intended cancellation contract. + +## Common Mistakes + +- Assuming `CommandContext` preserves the cancellation error you want to expose +- Detaching a process group without any matching cleanup path +- Sending signals after the process already exited +- Testing only the happy path and missing interrupt races +- Verifying process cleanup but not the error returned to the caller + +## Quick Reference + +- Need simple cancellation only: consider `exec.CommandContext` +- Need custom cleanup or error shape: use `exec.Command` + `Start`/`Wait` +- Need detached children: add explicit process-group teardown +- Need exit code mapping: keep `context.Canceled` visible through wrapping +- Need confidence: add focused tests for cleanup, error propagation, and exit races diff --git a/sgai.json b/sgai.json index 38472ea..b0c47ad 100644 --- a/sgai.json +++ b/sgai.json @@ -35,4 +35,4 @@ "description": "Delete unused and dead branches" } ] -} +} \ No newline at end of file