From 3601ecc2a8df08033ed93e30ba0df4c096958946 Mon Sep 17 00:00:00 2001 From: Edmund Kohlwey Date: Thu, 21 May 2026 09:33:10 -0400 Subject: [PATCH] scriptrun: Generic runner + shared protocol surface Adds the script-execution foundation and the cross-feature protocol surface that the message generator and both auto-resolvers will use. Runner (existing): executes a user-configured script either via 'sh -c' or by writing a shebang script to a temp file and exec'ing it. stdout, stderr, and the exit code are returned without interpretation so each feature can plug its own handler on top. Shared protocol (new in this commit): - ResolveResponse: the JSON document scripts emit on stdout (the union of {title, body, assumptions, questions, unresolved_files} fields). ParseResponse with ErrEmptyOutput / InvalidOutputError for diagnostics. - QAPair: per-question persistent-resolution-file record. - Operation enum and EnvFor(operation, branch, base) returning the shared GS_OPERATION / GS_BRANCH / GS_BASE env that every script receives. Feature-specific env layers on top. Per-repo scratch directory (new in this commit): - internal/spice/spicedir owns /.spice/. - Path / EnsureDir for the directory; ResolutionPath / EnsureResolutionsDir for per-feature resolution files at .spice/resolutions/.json. Does not modify .gitignore -- that's a project policy decision left to the user. Configurable iteration cap (new in this commit): - spice.scriptResolve.maxIterations (int, default 10) bounds the questions Q&A loop across all features. - Config.ScriptResolveMaxIterations() reads it with default+warning fallback for invalid values. Unit tests cover EnvFor, ParseResponse (empty/invalid/valid), spicedir path + EnsureDir, and ScriptResolveMaxIterations parsing. Together these are the surfaces that ed-irl/restack-auto-resolve, ed-irl/integration-auto-resolve, and ed-irl/ai-message-gen will consume in their respective fixups to collapse the existing per-feature divergences. See doc/src/guide/scripts.md on ed-irl/scripts-protocol-guide for the user-facing protocol contract. --- internal/scriptrun/env.go | 41 +++++++++++++ internal/scriptrun/env_test.go | 60 ++++++++++++++++++ internal/scriptrun/errors.go | 26 ++++++++ internal/scriptrun/resolver.go | 58 ++++++++++++++++++ internal/scriptrun/resolver_test.go | 78 ++++++++++++++++++++++++ internal/spice/config.go | 28 +++++++++ internal/spice/config_test.go | 76 +++++++++++++++++++++++ internal/spice/spicedir/spicedir.go | 57 +++++++++++++++++ internal/spice/spicedir/spicedir_test.go | 44 +++++++++++++ 9 files changed, 468 insertions(+) create mode 100644 internal/scriptrun/env.go create mode 100644 internal/scriptrun/env_test.go create mode 100644 internal/scriptrun/errors.go create mode 100644 internal/scriptrun/resolver.go create mode 100644 internal/scriptrun/resolver_test.go create mode 100644 internal/spice/spicedir/spicedir.go create mode 100644 internal/spice/spicedir/spicedir_test.go diff --git a/internal/scriptrun/env.go b/internal/scriptrun/env.go new file mode 100644 index 000000000..fb8659dfd --- /dev/null +++ b/internal/scriptrun/env.go @@ -0,0 +1,41 @@ +package scriptrun + +// Operation names the high-level gs command driving a script call. +// Used as the value of GS_OPERATION in the script's environment so +// scripts can branch on which gs subcommand invoked them. +// +// See doc/src/guide/scripts.md for the full table. +type Operation string + +// Shared GS_OPERATION values. Keep in sync with doc/src/guide/scripts.md. +const ( + OpCommitCreate Operation = "commit-create" + OpCommitAmend Operation = "commit-amend" + OpBranchCreate Operation = "branch-create" + OpBranchSubmit Operation = "branch-submit" + OpBranchSquash Operation = "branch-squash" + OpBranchRestack Operation = "branch-restack" + OpUpstackRestack Operation = "upstack-restack" + OpDownstackRestack Operation = "downstack-restack" + OpStackRestack Operation = "stack-restack" + OpRepoRestack Operation = "repo-restack" + OpIntegrationRebuild Operation = "integration-rebuild" +) + +// EnvFor returns the shared minimum environment that every script +// receives: GS_OPERATION (required), GS_BRANCH and GS_BASE (omitted +// when empty). Feature-specific extras layer on top of this slice via +// the existing RunRequest.Env mechanism. +// +// The returned slice is safe to append to. +func EnvFor(op Operation, branch, base string) []string { + env := make([]string, 0, 3) + env = append(env, "GS_OPERATION="+string(op)) + if branch != "" { + env = append(env, "GS_BRANCH="+branch) + } + if base != "" { + env = append(env, "GS_BASE="+base) + } + return env +} diff --git a/internal/scriptrun/env_test.go b/internal/scriptrun/env_test.go new file mode 100644 index 000000000..81bbd2b8c --- /dev/null +++ b/internal/scriptrun/env_test.go @@ -0,0 +1,60 @@ +package scriptrun_test + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "go.abhg.dev/gs/internal/scriptrun" +) + +func TestEnvFor(t *testing.T) { + tests := []struct { + name string + op scriptrun.Operation + branch string + base string + want []string + }{ + { + name: "OperationOnly", + op: scriptrun.OpIntegrationRebuild, + want: []string{"GS_OPERATION=integration-rebuild"}, + }, + { + name: "BranchPresent", + op: scriptrun.OpBranchRestack, + branch: "feat1", + want: []string{ + "GS_OPERATION=branch-restack", + "GS_BRANCH=feat1", + }, + }, + { + name: "AllThree", + op: scriptrun.OpCommitCreate, + branch: "feat1", + base: "main", + want: []string{ + "GS_OPERATION=commit-create", + "GS_BRANCH=feat1", + "GS_BASE=main", + }, + }, + { + name: "BaseWithoutBranchOmitsBoth", + op: scriptrun.OpStackRestack, + base: "main", + want: []string{ + "GS_OPERATION=stack-restack", + "GS_BASE=main", + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := scriptrun.EnvFor(tt.op, tt.branch, tt.base) + assert.Equal(t, tt.want, got) + }) + } +} diff --git a/internal/scriptrun/errors.go b/internal/scriptrun/errors.go new file mode 100644 index 000000000..d70358d89 --- /dev/null +++ b/internal/scriptrun/errors.go @@ -0,0 +1,26 @@ +package scriptrun + +import ( + "errors" + "fmt" +) + +// ErrEmptyOutput is returned by [ParseResponse] when the script +// produced no stdout. +var ErrEmptyOutput = errors.New("script produced no output") + +// InvalidOutputError wraps a JSON parse failure on script output. +// The original output is retained (truncated by callers as needed +// for logging) so handlers can surface diagnostic context. +type InvalidOutputError struct { + Err error + Output []byte +} + +func (e *InvalidOutputError) Error() string { + return fmt.Sprintf("invalid script output: %v", e.Err) +} + +func (e *InvalidOutputError) Unwrap() error { + return e.Err +} diff --git a/internal/scriptrun/resolver.go b/internal/scriptrun/resolver.go new file mode 100644 index 000000000..525ce9667 --- /dev/null +++ b/internal/scriptrun/resolver.go @@ -0,0 +1,58 @@ +package scriptrun + +import "encoding/json" + +// ResolveResponse is the JSON document a script emits on stdout. +// +// All three script-driven features (commit message generation, +// restack auto-resolve, integration auto-resolve) speak the same +// protocol. Each feature reads the subset of fields that applies. +// +// See doc/src/guide/scripts.md for the user-facing contract. +type ResolveResponse struct { + // Title is the first-line commit message or change-request title. + // Read by message-generation features; ignored by auto-resolvers. + Title string `json:"title,omitempty"` + + // Body is the multi-line body content for a commit or change + // request. Read by message-generation features. + Body string `json:"body,omitempty"` + + // Assumptions are notes the script wants surfaced in the gs log + // so the user can see what choices the script made. All features + // log these at info level. + Assumptions []string `json:"assumptions,omitempty"` + + // Questions, when non-empty, drives the interactive Q&A loop. + // git-spice prompts the user for each question and re-invokes the + // script with the answers persisted in the resolution file. + Questions []string `json:"questions,omitempty"` + + // UnresolvedFiles lists paths the script could not resolve. + // Read by auto-resolve features only; treated as "not done" -- + // the run is not terminal until this slice is empty. + UnresolvedFiles []string `json:"unresolved_files,omitempty"` +} + +// ParseResponse unmarshals raw script output into a ResolveResponse. +// Returns an error wrapping ErrInvalidOutput if the output is not a +// valid JSON document. +func ParseResponse(stdout []byte) (*ResolveResponse, error) { + var res ResolveResponse + if len(stdout) == 0 { + return nil, ErrEmptyOutput + } + if err := json.Unmarshal(stdout, &res); err != nil { + return nil, &InvalidOutputError{Err: err, Output: stdout} + } + return &res, nil +} + +// QAPair is a single question-answer record in a persistent +// resolution file. Resolution-file packages share this type so that +// answers gathered for one feature can be referenced from another (or +// at minimum so the on-disk schema is uniform). +type QAPair struct { + Question string `json:"question"` + Answer string `json:"answer"` +} diff --git a/internal/scriptrun/resolver_test.go b/internal/scriptrun/resolver_test.go new file mode 100644 index 000000000..bbce8f4a5 --- /dev/null +++ b/internal/scriptrun/resolver_test.go @@ -0,0 +1,78 @@ +package scriptrun_test + +import ( + "errors" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "go.abhg.dev/gs/internal/scriptrun" +) + +func TestParseResponse(t *testing.T) { + tests := []struct { + name string + input string + want *scriptrun.ResolveResponse + }{ + { + name: "Empty object", + input: `{}`, + want: &scriptrun.ResolveResponse{}, + }, + { + name: "TitleOnly", + input: `{"title": "Fix the thing"}`, + want: &scriptrun.ResolveResponse{Title: "Fix the thing"}, + }, + { + name: "Full", + input: `{ + "title": "T", + "body": "B", + "assumptions": ["a1"], + "questions": ["q1"], + "unresolved_files": ["f1"] + }`, + want: &scriptrun.ResolveResponse{ + Title: "T", + Body: "B", + Assumptions: []string{"a1"}, + Questions: []string{"q1"}, + UnresolvedFiles: []string{"f1"}, + }, + }, + { + name: "ExtraFieldsIgnored", + input: `{"title": "T", "extra": 42}`, + want: &scriptrun.ResolveResponse{Title: "T"}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, err := scriptrun.ParseResponse([]byte(tt.input)) + require.NoError(t, err) + assert.Equal(t, tt.want, got) + }) + } +} + +func TestParseResponse_emptyInput(t *testing.T) { + _, err := scriptrun.ParseResponse(nil) + assert.ErrorIs(t, err, scriptrun.ErrEmptyOutput) +} + +func TestParseResponse_invalidJSON(t *testing.T) { + _, err := scriptrun.ParseResponse([]byte("not json")) + var invalid *scriptrun.InvalidOutputError + require.ErrorAs(t, err, &invalid) + assert.NotEmpty(t, invalid.Output) + assert.Contains(t, invalid.Error(), "invalid script output") +} + +func TestInvalidOutputError_unwraps(t *testing.T) { + inner := errors.New("boom") + e := &scriptrun.InvalidOutputError{Err: inner} + assert.ErrorIs(t, e, inner) +} diff --git a/internal/spice/config.go b/internal/spice/config.go index a249f993b..b6d8cc56a 100644 --- a/internal/spice/config.go +++ b/internal/spice/config.go @@ -236,6 +236,34 @@ func (c *Config) Shorthands() []string { return slices.Sorted(maps.Keys(c.shorthands)) } +// DefaultScriptResolveMaxIterations is the iteration cap used when +// spice.scriptResolve.maxIterations is unset or invalid. A script that +// keeps returning new questions hits this many iterations before +// git-spice gives up and falls back to manual resolution. +const DefaultScriptResolveMaxIterations = 10 + +// ScriptResolveMaxIterations returns the configured iteration cap for +// script-driven Q&A loops. Read from spice.scriptResolve.maxIterations +// and defaults to [DefaultScriptResolveMaxIterations]. Non-positive or +// non-numeric values fall back to the default and emit a warning. +func (c *Config) ScriptResolveMaxIterations() int { + key := git.ConfigKey("spice.scriptResolve.maxIterations").Canonical() + values, ok := c.items[key] + if !ok || len(values) == 0 { + return DefaultScriptResolveMaxIterations + } + last := strings.TrimSpace(values[len(values)-1]) + n, err := strconv.Atoi(last) + if err != nil || n <= 0 { + c.log.Warnf( + "invalid value for %v: %q (using default %d)", + key, last, DefaultScriptResolveMaxIterations, + ) + return DefaultScriptResolveMaxIterations + } + return n +} + // Validate checks if the configuration is valid for the given application. // This is a no-op, as we allow unknown configuration keys. func (*Config) Validate(*kong.Application) error { return nil } diff --git a/internal/spice/config_test.go b/internal/spice/config_test.go index 6e854ccac..70896020d 100644 --- a/internal/spice/config_test.go +++ b/internal/spice/config_test.go @@ -457,6 +457,82 @@ func TestConfig_ShellCommand(t *testing.T) { } } +func TestConfig_ScriptResolveMaxIterations(t *testing.T) { + tests := []struct { + name string + config string + want int + }{ + { + name: "Unset", + config: "", + want: spice.DefaultScriptResolveMaxIterations, + }, + { + name: "ValidOverride", + config: text.Dedent(` + [spice "scriptResolve"] + maxIterations = 25 + `), + want: 25, + }, + { + name: "InvalidFallsBackToDefault", + config: text.Dedent(` + [spice "scriptResolve"] + maxIterations = banana + `), + want: spice.DefaultScriptResolveMaxIterations, + }, + { + name: "ZeroFallsBackToDefault", + config: text.Dedent(` + [spice "scriptResolve"] + maxIterations = 0 + `), + want: spice.DefaultScriptResolveMaxIterations, + }, + { + name: "NegativeFallsBackToDefault", + config: text.Dedent(` + [spice "scriptResolve"] + maxIterations = -5 + `), + want: spice.DefaultScriptResolveMaxIterations, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + home := t.TempDir() + require.NoError(t, os.WriteFile( + filepath.Join(home, ".gitconfig"), + []byte(tt.config), + 0o600, + ), "write configuration file") + + ctx := t.Context() + gitCfg := git.NewConfig(git.ConfigOptions{ + Log: silogtest.New(t), + Dir: home, + Env: []string{ + "HOME=" + home, + "USER=testuser", + "GIT_CONFIG_NOSYSTEM=1", + }, + }) + spicecfg, err := spice.LoadConfig(ctx, gitCfg, spice.ConfigOptions{ + Log: silogtest.New(t), + }) + require.NoError(t, err, "load configuration") + + assert.Equal(t, tt.want, spicecfg.ScriptResolveMaxIterations()) + }) + } +} + func TestIntegrationConfig_gitConfigReferences(t *testing.T) { t.Setenv("HOME", "") t.Setenv("XDG_CONFIG_HOME", "") diff --git a/internal/spice/spicedir/spicedir.go b/internal/spice/spicedir/spicedir.go new file mode 100644 index 000000000..5d8e7e443 --- /dev/null +++ b/internal/spice/spicedir/spicedir.go @@ -0,0 +1,57 @@ +// Package spicedir owns the on-disk layout for git-spice's +// per-repository scratch directory: /.spice/. +// +// Today the directory holds persistent script-resolution files used by +// the message generator and the two auto-resolve features. Future +// features that need a per-repo cache (workspace snapshots, plan +// scratch, etc.) should put their data here instead of cluttering the +// repo root. +// +// Whether to track .spice/ in version control is the user's choice. +// spicedir does not modify .gitignore. +package spicedir + +import ( + "fmt" + "os" + "path/filepath" +) + +// DirName is the directory name relative to the repository root. +const DirName = ".spice" + +// Path returns the absolute path to the spice directory inside the +// given repository root. Path does not create the directory; call +// [EnsureDir] for that. +func Path(repoRoot string) string { + return filepath.Join(repoRoot, DirName) +} + +// EnsureDir creates the spice directory (and any missing parents) +// under the given repository root. Returns nil if it already exists. +func EnsureDir(repoRoot string) error { + dir := Path(repoRoot) + if err := os.MkdirAll(dir, 0o755); err != nil { + return fmt.Errorf("create spice dir: %w", err) + } + return nil +} + +// ResolutionPath returns the canonical path for a feature's +// resolution-history JSON file: /.spice/resolutions/.json. +// +// Callers should call [EnsureResolutionsDir] before writing to the +// returned path. +func ResolutionPath(repoRoot, feature string) string { + return filepath.Join(Path(repoRoot), "resolutions", feature+".json") +} + +// EnsureResolutionsDir creates /.spice/resolutions/ if it +// does not already exist. +func EnsureResolutionsDir(repoRoot string) error { + dir := filepath.Join(Path(repoRoot), "resolutions") + if err := os.MkdirAll(dir, 0o755); err != nil { + return fmt.Errorf("create resolutions dir: %w", err) + } + return nil +} diff --git a/internal/spice/spicedir/spicedir_test.go b/internal/spice/spicedir/spicedir_test.go new file mode 100644 index 000000000..4bdc13f0a --- /dev/null +++ b/internal/spice/spicedir/spicedir_test.go @@ -0,0 +1,44 @@ +package spicedir_test + +import ( + "os" + "path/filepath" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "go.abhg.dev/gs/internal/spice/spicedir" +) + +func TestPath(t *testing.T) { + got := spicedir.Path("/repo") + assert.Equal(t, filepath.Join("/repo", ".spice"), got) +} + +func TestResolutionPath(t *testing.T) { + got := spicedir.ResolutionPath("/repo", "restack") + assert.Equal(t, filepath.Join("/repo", ".spice", "resolutions", "restack.json"), got) +} + +func TestEnsureDir(t *testing.T) { + root := t.TempDir() + + require.NoError(t, spicedir.EnsureDir(root)) + + info, err := os.Stat(filepath.Join(root, ".spice")) + require.NoError(t, err) + assert.True(t, info.IsDir()) + + // Calling again is idempotent. + require.NoError(t, spicedir.EnsureDir(root)) +} + +func TestEnsureResolutionsDir(t *testing.T) { + root := t.TempDir() + + require.NoError(t, spicedir.EnsureResolutionsDir(root)) + + info, err := os.Stat(filepath.Join(root, ".spice", "resolutions")) + require.NoError(t, err) + assert.True(t, info.IsDir()) +}