From de6abd9da5c1a69c62dae05a7439d8c68b4b4780 Mon Sep 17 00:00:00 2001 From: quike <616137+quike@users.noreply.github.com> Date: Wed, 16 Sep 2026 15:23:17 -0400 Subject: [PATCH] feat(config): add per-command env, dir, and silent overrides --- docs/CONFIG.md | 55 ++++- internal/cache/cache.go | 14 +- internal/cache/percommand_test.go | 136 ++++++++++++ internal/config/commands_test.go | 144 ++++++++++++ internal/config/config.go | 61 +++-- internal/config/refs.go | 12 + .../test-resources/config-commands-valid.yml | 16 ++ internal/engine/commands_test.go | 6 +- internal/engine/engine.go | 34 ++- internal/engine/engine_test.go | 6 +- internal/engine/envelope_test.go | 4 +- internal/engine/percommand_test.go | 209 ++++++++++++++++++ internal/engine/runner.go | 34 ++- internal/engine/runner_test.go | 19 +- internal/engine/template_integration_test.go | 12 +- 15 files changed, 710 insertions(+), 52 deletions(-) create mode 100644 internal/cache/percommand_test.go create mode 100644 internal/engine/percommand_test.go diff --git a/docs/CONFIG.md b/docs/CONFIG.md index 37929a8..4771f46 100644 --- a/docs/CONFIG.md +++ b/docs/CONFIG.md @@ -156,7 +156,8 @@ caching, gating, and output: order; `exitCode` is the first failing command's (0 when all succeed); `durationMs` covers the whole sequence - `cache` fingerprints every command in the list — changing any entry busts - the cache + the cache (see [Per-command overrides](#per-command-overrides) for how + `env`/`dir`/`silent` participate) - `require`/`skip-if` gate the whole sequence, and templating expands each entry's command/params individually - `retries:` replays the **whole sequence** from the first command on each @@ -176,6 +177,58 @@ execution path. Setting both `command` and `commands` on one group is a load error, as is a string-form entry on a group with no `shell:`, or an empty `commands:` list. +### Per-command overrides + +Map-form entries accept three optional keys. String-form entries do not: the +shape selects the capabilities, so an entry needing an override is written as a +map. + +| Key | Type | Effect | +| -------- | ------ | ----------------------------------------------------------------------------- | +| `env` | map | Layers over the group's `env:` for this command only. | +| `dir` | string | Runs this command in another directory, without needing a shell. | +| `silent` | bool | Suppresses live streaming; the output is still captured. | + +```yaml +groups: + - name: release + env: + BUILD_MODE: release + commands: + - { command: go, params: [build, "./..."], env: { CGO_ENABLED: "0" } } + - { command: ./package.sh, dir: dist } + - { command: ./notify.sh, silent: true } +``` + +`env` precedence, lowest to highest: process environment, global `env:`, group +`env:`, command `env:`. A command's `env` applies to that command alone and does +not leak into the entries around it. + +`dir` is resolved relative to the process working directory and must not be +empty. It is the reason to prefer a map entry over `cd x && ./y`: the latter is +a string-form entry, which forces `shell:` on the whole group and gives up safe +argv execution for every command in it. + +`silent` affects only what is streamed to your terminal. Capture is unchanged, +so `{{ output }}` references, the JSON event stream, and cache replay all still +see the full output. + +Both `env` values and `dir` are templated like `command` and `params`: + +```yaml +commands: + - command: ./package.sh + dir: 'dist/{{ env "TARGET" }}' + env: + SHA: '{{ output "build" }}' +``` + +`env` and `dir` fold into the cache fingerprint, so changing either re-runs the +group. `silent` does not: it changes nothing about what the command does or +produces, so toggling it keeps a valid cached result. A group that uses none of +these keys keeps the fingerprint it had before they existed — upgrading does not +invalidate existing caches. + ### Referencing other groups' output A group's `command` or any of its `params` can include diff --git a/internal/cache/cache.go b/internal/cache/cache.go index 2a11d31..860790a 100644 --- a/internal/cache/cache.go +++ b/internal/cache/cache.go @@ -7,7 +7,9 @@ import ( "encoding/hex" "fmt" "io" + "maps" "os" + "slices" "sort" "time" @@ -35,8 +37,8 @@ type Store interface { } // Compute returns a content fingerprint for the given cache spec. The -// fingerprint changes when the method, any command/param/form in the group's -// command list, or any matched input file changes. For shell-form entries the +// fingerprint changes when the method, any command/param/form/env/dir in the +// group's command list, or any matched input file changes. For shell-form entries the // fingerprint also changes when the shell program changes. A glob that matches // nothing contributes nothing, so adding the first matching file naturally // changes the fingerprint. @@ -57,6 +59,14 @@ func Compute(spec *config.Cache, shell string, commands []config.CommandSpec) (s for _, p := range c.Params { fmt.Fprintf(h, "%s\x01", p) } + // Emitted only when set: a config using neither must keep hashing as it + // did before they existed, or every cache entry silently invalidates. + if c.Dir != "" { + fmt.Fprintf(h, "%s\x03", c.Dir) + } + for _, k := range slices.Sorted(maps.Keys(c.Env)) { + fmt.Fprintf(h, "%s\x04%s\x03", k, c.Env[k]) + } fmt.Fprintf(h, "\x02") } diff --git a/internal/cache/percommand_test.go b/internal/cache/percommand_test.go new file mode 100644 index 0000000..c9c017c --- /dev/null +++ b/internal/cache/percommand_test.go @@ -0,0 +1,136 @@ +package cache + +import ( + "crypto/sha256" + "encoding/hex" + "fmt" + "path/filepath" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/quike/keepup/internal/config" +) + +// fpFor computes a fingerprint for one command against a fixed input file. +func fpFor(t *testing.T, dir string, cs config.CommandSpec) string { + t.Helper() + spec := &config.Cache{Method: config.CacheHash, Reads: []string{filepath.Join(dir, "*.go")}} + fp, err := Compute(spec, "", []config.CommandSpec{cs}) + require.NoError(t, err) + return fp +} + +func TestCompute_PerCommandKnobs(t *testing.T) { + dir := t.TempDir() + writeFile(t, filepath.Join(dir, "a.go"), "package main\n") + base := config.CommandSpec{Command: "go", Params: []string{"build"}} + + tests := []struct { + name string + spec config.CommandSpec + wantBust bool + }{ + { + name: "env value change busts", + spec: config.CommandSpec{Command: "go", Params: []string{"build"}, Env: map[string]string{"CGO_ENABLED": "0"}}, + wantBust: true, + }, + { + name: "dir change busts", + spec: config.CommandSpec{Command: "go", Params: []string{"build"}, Dir: "sub"}, + wantBust: true, + }, + { + name: "silent is display-only and must NOT bust", + spec: config.CommandSpec{Command: "go", Params: []string{"build"}, Silent: true}, + wantBust: false, + }, + { + name: "empty env map is the same as no env", + spec: config.CommandSpec{Command: "go", Params: []string{"build"}, Env: map[string]string{}}, + wantBust: false, + }, + } + + baseFP := fpFor(t, dir, base) + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + got := fpFor(t, dir, tc.spec) + if tc.wantBust { + assert.NotEqual(t, baseFP, got) + return + } + assert.Equal(t, baseFP, got) + }) + } +} + +// Go map iteration order is randomized, so an unsorted encoding would make the +// fingerprint differ between runs of the same config. +func TestCompute_EnvOrderIsDeterministic(t *testing.T) { + dir := t.TempDir() + writeFile(t, filepath.Join(dir, "a.go"), "package main\n") + spec := config.CommandSpec{ + Command: "go", + Env: map[string]string{"A": "1", "B": "2", "C": "3", "D": "4", "E": "5"}, + } + want := fpFor(t, dir, spec) + for range 20 { + assert.Equal(t, want, fpFor(t, dir, spec)) + } +} + +// Two different env maps must not hash the same just because their +// concatenated bytes could line up. +func TestCompute_EnvPairsAreUnambiguous(t *testing.T) { + dir := t.TempDir() + writeFile(t, filepath.Join(dir, "a.go"), "package main\n") + a := config.CommandSpec{Command: "go", Env: map[string]string{"A": "BC"}} + b := config.CommandSpec{Command: "go", Env: map[string]string{"AB": "C"}} + assert.NotEqual(t, fpFor(t, dir, a), fpFor(t, dir, b)) +} + +// Guards the upgrade path: without this, every existing cache entry invalidates. +func TestCompute_UnchangedConfigKeepsItsFingerprint(t *testing.T) { + dir := t.TempDir() + writeFile(t, filepath.Join(dir, "a.go"), "package main\n") + spec := &config.Cache{Method: config.CacheHash, Reads: []string{filepath.Join(dir, "*.go")}} + + got, err := Compute(spec, "/bin/sh", []config.CommandSpec{ + {Command: "go", Params: []string{"build", "./..."}}, + {Command: "echo done", IsShell: true}, + }) + require.NoError(t, err) + + assert.Equal(t, legacyV3(t, spec, "/bin/sh", []config.CommandSpec{ + {Command: "go", Params: []string{"build", "./..."}}, + {Command: "echo done", IsShell: true}, + }), got) +} + +// legacyV3 reproduces the byte stream Compute produced before env/dir existed. +// Written out independently so the back-compat assertion cannot drift along +// with the implementation it is guarding. +func legacyV3(t *testing.T, spec *config.Cache, shell string, commands []config.CommandSpec) string { + t.Helper() + h := sha256.New() + fmt.Fprintf(h, "v3\x00%s\x00", spec.Method) + for _, c := range commands { + fmt.Fprintf(h, "%s\x00%t\x00", c.Command, c.IsShell) + if c.IsShell { + fmt.Fprintf(h, "%s\x00", shell) + } + for _, p := range c.Params { + fmt.Fprintf(h, "%s\x01", p) + } + fmt.Fprintf(h, "\x02") + } + files, err := resolveGlobs(spec.Reads) + require.NoError(t, err) + for _, f := range files { + require.NoError(t, hashFile(h, spec.Method, f)) + } + return "sha256:" + hex.EncodeToString(h.Sum(nil)) +} diff --git a/internal/config/commands_test.go b/internal/config/commands_test.go index 8e1bfce..d6ae8b6 100644 --- a/internal/config/commands_test.go +++ b/internal/config/commands_test.go @@ -67,6 +67,87 @@ func TestCommandSpec_UnmarshalYAML(t *testing.T) { wantErr: "must be a string or a {command, params} map", }, } + runCommandSpecCases(t, tests) +} + +func TestCommandSpec_UnmarshalYAML_PerCommandKnobs(t *testing.T) { + tests := []struct { + name string + yaml string + want CommandSpec + wantErr string + }{ + { + name: "per-command env", + yaml: `{command: go, params: [build], env: {CGO_ENABLED: "0"}}`, + want: CommandSpec{ + Command: "go", + Params: []string{"build"}, + Env: map[string]string{"CGO_ENABLED": "0"}, + }, + }, + { + name: "per-command dir", + yaml: `{command: ./package.sh, dir: dist}`, + want: CommandSpec{Command: "./package.sh", Dir: "dist"}, + }, + { + name: "per-command silent", + yaml: `{command: ./notify.sh, silent: true}`, + want: CommandSpec{Command: "./notify.sh", Silent: true}, + }, + { + name: "all three knobs together", + yaml: `{command: ./x.sh, dir: build, silent: true, env: {A: "1", B: "2"}}`, + want: CommandSpec{ + Command: "./x.sh", + Dir: "build", + Silent: true, + Env: map[string]string{"A": "1", "B": "2"}, + }, + }, + { + name: "empty dir rejected", + yaml: `{command: go, dir: ""}`, + wantErr: `"dir" must not be empty`, + }, + { + name: "empty env key rejected", + yaml: `{command: go, env: {"": "1"}}`, + wantErr: `"env" keys must not be empty`, + }, + { + name: "dir must be a string", + yaml: `{command: go, dir: [a]}`, + wantErr: `"dir" must be a string`, + }, + { + name: "silent must be a boolean", + yaml: `{command: go, silent: maybe}`, + wantErr: `"silent" must be a boolean`, + }, + { + name: "env must be a string map", + yaml: `{command: go, env: [a]}`, + wantErr: `"env" must be a map of strings`, + }, + { + name: "string form stays knob-free", + yaml: `go test ./...`, + want: CommandSpec{Command: "go test ./...", IsShell: true}, + }, + } + runCommandSpecCases(t, tests) +} + +func runCommandSpecCases(t *testing.T, tests []struct { + name string + yaml string + want CommandSpec + wantErr string +}, +) { + t.Helper() for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { var cs CommandSpec @@ -268,6 +349,43 @@ func TestExtractRefs_CommandsEntries(t *testing.T) { assert.Equal(t, []string{"a", "b"}, refs) } +// ExtractRefs drives the dag scheduler's edges, load-time reference validation, +// and `keepup graph`, so dir/env templates must register there too. +func TestExtractRefs_DirAndEnv(t *testing.T) { + g := &Group{Name: "g", Commands: []CommandSpec{{ + Command: "./package.sh", + Dir: `dist/{{ output "target" }}`, + Env: map[string]string{"SHA": `{{ output "build" }}`}, + }}} + refs, err := ExtractRefs(g) + require.NoError(t, err) + assert.ElementsMatch(t, []string{"target", "build"}, refs) +} + +// Env keys are unordered; unstable extraction would shuffle the dag edges. +func TestExtractRefs_EnvOrderIsStable(t *testing.T) { + g := &Group{Name: "g", Commands: []CommandSpec{{ + Command: "x", + Env: map[string]string{ + "A": `{{ output "one" }}`, "B": `{{ output "two" }}`, + "C": `{{ output "three" }}`, "D": `{{ output "four" }}`, + }, + }}} + want, err := ExtractRefs(g) + require.NoError(t, err) + for range 20 { + got, err := ExtractRefs(g) + require.NoError(t, err) + assert.Equal(t, want, got) + } +} + +func TestExtractRefs_MalformedDirTemplate(t *testing.T) { + g := &Group{Name: "g", Commands: []CommandSpec{{Command: "x", Dir: `{{ output "a" `}}} + _, err := ExtractRefs(g) + require.Error(t, err) +} + func TestValidateReferences_CommandsEntryForwardRef(t *testing.T) { yml := ` version: 2 @@ -370,3 +488,29 @@ func TestLoadConfig_CommandsFixture(t *testing.T) { []CommandSpec{{Command: "echo", Params: []string{"single-step"}}}, single.CommandList()) } + +func TestLoadConfig_PerCommandKnobsFixture(t *testing.T) { + cfg, err := LoadConfig("./test-resources/config-commands-valid.yml") + require.NoError(t, err) + + knobs := cfg.GroupByName("knobs") + require.NotNil(t, knobs) + list := knobs.CommandList() + require.Len(t, list, 4) + + assert.Equal(t, map[string]string{"LAYER": "command"}, list[0].Env) + assert.Equal(t, "/tmp", list[1].Dir) + assert.True(t, list[2].Silent) + assert.Equal(t, `/tmp/{{ env "HOME" }}`, list[3].Dir, "templates survive load unrendered") + assert.Equal(t, map[string]string{"SHA": `{{ output "single" }}`}, list[3].Env) + + // Entries that declare no overrides must stay zero-valued rather than + // inheriting the group's env by accident at load time. + assert.Nil(t, list[1].Env) + assert.Empty(t, list[0].Dir) + assert.False(t, list[0].Silent) + + refs, err := ExtractRefs(knobs) + require.NoError(t, err) + assert.Contains(t, refs, "single", "an env template must register as a dependency") +} diff --git a/internal/config/config.go b/internal/config/config.go index 65d4914..d5702a6 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -215,6 +215,11 @@ func (r *RunEntry) UnmarshalYAML(node *yaml.Node) error { type CommandSpec struct { Command string `yaml:"command" json:"command"` Params []string `yaml:"params,omitempty" json:"params,omitempty"` + // Map-form-only overrides: Env layers over the group's env:, Dir needs no + // shell, and Silent suppresses streaming but not capture. + Env map[string]string `yaml:"env,omitempty" json:"env,omitempty"` + Dir string `yaml:"dir,omitempty" json:"dir,omitempty"` + Silent bool `yaml:"silent,omitempty" json:"silent,omitempty"` // IsShell records that the entry was written in string form and therefore // runs through the group's shell:. Argv-form entries always exec directly // and ignore shell:, even when it is set. @@ -238,20 +243,8 @@ func (cs *CommandSpec) UnmarshalYAML(node *yaml.Node) error { return nil case yaml.MappingNode: for i := 0; i+1 < len(node.Content); i += 2 { - key := node.Content[i].Value - val := node.Content[i+1] - switch key { - case "command": - if val.Kind != yaml.ScalarNode { - return fmt.Errorf(`commands entry: "command" must be a string`) - } - cs.Command = val.Value - case "params": - if err := val.Decode(&cs.Params); err != nil { - return fmt.Errorf(`commands entry: "params" must be a list of strings: %w`, err) - } - default: - return fmt.Errorf("commands entry: unexpected key %q (use a string or a {command, params} map)", key) + if err := cs.unmarshalKey(node.Content[i].Value, node.Content[i+1]); err != nil { + return err } } if cs.Command == "" { @@ -264,6 +257,46 @@ func (cs *CommandSpec) UnmarshalYAML(node *yaml.Node) error { return fmt.Errorf("commands entry: must be a string or a {command, params} map") } +// unmarshalKey decodes one key of a map-form commands entry. Rejecting unknown +// keys keeps the namespace reserved; see #32. +func (cs *CommandSpec) unmarshalKey(key string, val *yaml.Node) error { + switch key { + case "command": + if val.Kind != yaml.ScalarNode { + return fmt.Errorf(`commands entry: "command" must be a string`) + } + cs.Command = val.Value + case "params": + if err := val.Decode(&cs.Params); err != nil { + return fmt.Errorf(`commands entry: "params" must be a list of strings: %w`, err) + } + case "env": + if err := val.Decode(&cs.Env); err != nil { + return fmt.Errorf(`commands entry: "env" must be a map of strings: %w`, err) + } + if _, empty := cs.Env[""]; empty { + return fmt.Errorf(`commands entry: "env" keys must not be empty`) + } + case "dir": + if val.Kind != yaml.ScalarNode { + return fmt.Errorf(`commands entry: "dir" must be a string`) + } + // Checked here rather than in validateGroupCommands: only the node + // knows the difference between dir: "" and no dir at all. + if val.Value == "" { + return fmt.Errorf(`commands entry: "dir" must not be empty`) + } + cs.Dir = val.Value + case "silent": + if err := val.Decode(&cs.Silent); err != nil { + return fmt.Errorf(`commands entry: "silent" must be a boolean: %w`, err) + } + default: + return fmt.Errorf("commands entry: unexpected key %q (use a string or a {command, params} map)", key) + } + return nil +} + // NewConfig parses YAML bytes into a Config and validates the schema. func NewConfig(b []byte) (*Config, error) { var cfg Config diff --git a/internal/config/refs.go b/internal/config/refs.go index c86cae3..af26145 100644 --- a/internal/config/refs.go +++ b/internal/config/refs.go @@ -2,6 +2,8 @@ package config import ( "fmt" + "maps" + "slices" "github.com/quike/keepup/internal/template" ) @@ -30,6 +32,16 @@ func ExtractRefs(g *Group) ([]string, error) { return nil, err } } + if err := collect(cs.Dir); err != nil { + return nil, err + } + // Sorted: map order is random, and these refs become dag edges and + // graph output, which must not shuffle between runs. + for _, k := range slices.Sorted(maps.Keys(cs.Env)) { + if err := collect(cs.Env[k]); err != nil { + return nil, err + } + } } return out, nil } diff --git a/internal/config/test-resources/config-commands-valid.yml b/internal/config/test-resources/config-commands-valid.yml index 8aeca1b..386bf53 100644 --- a/internal/config/test-resources/config-commands-valid.yml +++ b/internal/config/test-resources/config-commands-valid.yml @@ -22,11 +22,27 @@ groups: command: echo params: [single-step] + # per-command overrides — map form only + - name: knobs + description: "Per-command env, dir and silent" + env: + LAYER: group + commands: + - { command: printenv, params: [LAYER], env: { LAYER: command } } + - { command: pwd, dir: /tmp } + - { command: echo, params: [quiet], silent: true } + - command: echo + params: [templated] + dir: '/tmp/{{ env "HOME" }}' + env: + SHA: '{{ output "single" }}' + flows: ci: description: "Multi then single" steps: - run: [multi] - run: [single] + - run: [knobs] default: ci diff --git a/internal/engine/commands_test.go b/internal/engine/commands_test.go index de75673..107d201 100644 --- a/internal/engine/commands_test.go +++ b/internal/engine/commands_test.go @@ -31,10 +31,10 @@ type specCall struct { shell string } -func (f *specRunner) Run(_ context.Context, g *config.Group, params []string, _ map[string]string) (result.RunResult, error) { +func (f *specRunner) Run(_ context.Context, g *config.Group, spec config.CommandSpec, _ map[string]string) (result.RunResult, error) { f.mu.Lock() defer f.mu.Unlock() - f.calls = append(f.calls, specCall{command: g.Command, params: append([]string(nil), params...), shell: g.Shell}) + f.calls = append(f.calls, specCall{command: g.Command, params: append([]string(nil), spec.Params...), shell: g.Shell}) out := f.outputs[g.Command] rr := result.RunResult{Stdout: out, Output: out, Status: result.StatusOK, DurationMs: 1} if err, ok := f.failOnce[g.Command]; ok { @@ -199,7 +199,7 @@ type softFailRunner struct { calls int } -func (f *softFailRunner) Run(_ context.Context, _ *config.Group, _ []string, _ map[string]string) (result.RunResult, error) { +func (f *softFailRunner) Run(_ context.Context, _ *config.Group, _ config.CommandSpec, _ map[string]string) (result.RunResult, error) { f.mu.Lock() defer f.mu.Unlock() f.calls++ diff --git a/internal/engine/engine.go b/internal/engine/engine.go index 9e59ad4..45bccb0 100644 --- a/internal/engine/engine.go +++ b/internal/engine/engine.go @@ -185,11 +185,41 @@ func (e *Engine) expandCommands(group *config.Group, data template.Data) ([]conf return nil, fmt.Errorf("group %q: expand param %d: %w", group.Name, j+1, err) } } - expanded[i] = config.CommandSpec{Command: cmd, Params: params, IsShell: s.IsShell} + dir := s.Dir + if dir != "" { + if dir, err = e.expander.Expand(s.Dir, data); err != nil { + return nil, fmt.Errorf("group %q: expand dir: %w", group.Name, err) + } + } + env, err := e.expandEnv(group.Name, s.Env, data) + if err != nil { + return nil, err + } + // Copy the spec so fields that need no expansion (Silent, IsShell, and + // anything added later) carry through instead of being dropped. + out := s + out.Command, out.Params, out.Dir, out.Env = cmd, params, dir, env + expanded[i] = out } return expanded, nil } +// expandEnv renders each value of a per-command env map. Keys are literal. +func (e *Engine) expandEnv(group string, env map[string]string, data template.Data) (map[string]string, error) { + if len(env) == 0 { + return nil, nil + } + out := make(map[string]string, len(env)) + for k, v := range env { + rendered, err := e.expander.Expand(v, data) + if err != nil { + return nil, fmt.Errorf("group %q: expand env %q: %w", group, k, err) + } + out[k] = rendered + } + return out, nil +} + // runGroup decides whether and how to execute a group. The decision order is: // // 1. dry-run → log intent, do nothing else (gating/cache are not evaluated) @@ -328,7 +358,7 @@ func (e *Engine) runSequence(ctx context.Context, group *config.Group, commands if !s.IsShell { sg.Shell = "" // {command, params} entries are always safe argv exec } - out, err := e.runner.Run(ctx, &sg, s.Params, e.cfg.Env) + out, err := e.runner.Run(ctx, &sg, s, e.cfg.Env) agg.Stdout += out.Stdout agg.Stderr += out.Stderr agg.Output += out.Output diff --git a/internal/engine/engine_test.go b/internal/engine/engine_test.go index 8a69521..692c7ce 100644 --- a/internal/engine/engine_test.go +++ b/internal/engine/engine_test.go @@ -25,7 +25,7 @@ type fakeRunner struct { delays map[string]time.Duration } -func (f *fakeRunner) Run(ctx context.Context, g *config.Group, params []string, _ map[string]string) (result.RunResult, error) { +func (f *fakeRunner) Run(ctx context.Context, g *config.Group, spec config.CommandSpec, _ map[string]string) (result.RunResult, error) { if d := f.delays[g.Name]; d > 0 { select { case <-time.After(d): @@ -35,7 +35,7 @@ func (f *fakeRunner) Run(ctx context.Context, g *config.Group, params []string, } f.mu.Lock() defer f.mu.Unlock() - f.calls = append(f.calls, g.Name+":"+strings.Join(params, ",")) + f.calls = append(f.calls, g.Name+":"+strings.Join(spec.Params, ",")) stdout := f.outputs[g.Name] rr := result.RunResult{ Stdout: stdout, @@ -197,7 +197,7 @@ type concurrencyRunner struct { after func() } -func (c *concurrencyRunner) Run(_ context.Context, _ *config.Group, _ []string, _ map[string]string) (result.RunResult, error) { +func (c *concurrencyRunner) Run(_ context.Context, _ *config.Group, _ config.CommandSpec, _ map[string]string) (result.RunResult, error) { c.before() defer c.after() return result.RunResult{Status: "ok"}, nil diff --git a/internal/engine/envelope_test.go b/internal/engine/envelope_test.go index fd92fad..3317289 100644 --- a/internal/engine/envelope_test.go +++ b/internal/engine/envelope_test.go @@ -21,7 +21,7 @@ type flakyRunner struct { output string } -func (r *flakyRunner) Run(_ context.Context, _ *config.Group, _ []string, _ map[string]string) (result.RunResult, error) { +func (r *flakyRunner) Run(_ context.Context, _ *config.Group, _ config.CommandSpec, _ map[string]string) (result.RunResult, error) { n := atomic.AddInt32(&r.calls, 1) if n <= r.failUntil { return result.RunResult{ExitCode: 1}, errors.New("transient failure") @@ -33,7 +33,7 @@ func (r *flakyRunner) Run(_ context.Context, _ *config.Group, _ []string, _ map[ // exercise timeouts. type blockingRunner struct{ calls int32 } -func (r *blockingRunner) Run(ctx context.Context, _ *config.Group, _ []string, _ map[string]string) (result.RunResult, error) { +func (r *blockingRunner) Run(ctx context.Context, _ *config.Group, _ config.CommandSpec, _ map[string]string) (result.RunResult, error) { atomic.AddInt32(&r.calls, 1) <-ctx.Done() return result.RunResult{}, ctx.Err() diff --git a/internal/engine/percommand_test.go b/internal/engine/percommand_test.go new file mode 100644 index 0000000..7270a96 --- /dev/null +++ b/internal/engine/percommand_test.go @@ -0,0 +1,209 @@ +package engine + +import ( + "bytes" + "context" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/quike/keepup/internal/config" + "github.com/quike/keepup/internal/result" + "github.com/quike/keepup/internal/template" +) + +func TestShellRunner_EnvPrecedence(t *testing.T) { + skipOnWindows(t) + t.Setenv("KEEPUP_LAYER", "process") + + tests := []struct { + name string + globalEnv map[string]string + groupEnv map[string]string + cmdEnv map[string]string + want string + }{ + {name: "process only", want: "process"}, + {name: "global over process", globalEnv: map[string]string{"KEEPUP_LAYER": "global"}, want: "global"}, + { + name: "group over global", + globalEnv: map[string]string{"KEEPUP_LAYER": "global"}, + groupEnv: map[string]string{"KEEPUP_LAYER": "group"}, + want: "group", + }, + { + name: "command over group", + globalEnv: map[string]string{"KEEPUP_LAYER": "global"}, + groupEnv: map[string]string{"KEEPUP_LAYER": "group"}, + cmdEnv: map[string]string{"KEEPUP_LAYER": "command"}, + want: "command", + }, + { + name: "command over process with no middle layers", + cmdEnv: map[string]string{"KEEPUP_LAYER": "command"}, + groupEnv: nil, + want: "command", + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + var stdout, stderr bytes.Buffer + r := &ShellRunner{Stdout: &stdout, Stderr: &stderr} + g := &config.Group{Name: "g", Command: "printenv", Env: tc.groupEnv} + spec := config.CommandSpec{Command: "printenv", Params: []string{"KEEPUP_LAYER"}, Env: tc.cmdEnv} + out, err := r.Run(context.Background(), g, spec, tc.globalEnv) + require.NoError(t, err) + assert.Equal(t, tc.want, strings.TrimSpace(out.Stdout)) + }) + } +} + +func TestShellRunner_CommandEnvDoesNotLeak(t *testing.T) { + skipOnWindows(t) + t.Parallel() + var stdout, stderr bytes.Buffer + r := &ShellRunner{Stdout: &stdout, Stderr: &stderr} + g := &config.Group{Name: "g", Command: "printenv"} + + scoped := config.CommandSpec{Command: "printenv", Params: []string{"SCOPED"}, Env: map[string]string{"SCOPED": "yes"}} + out, err := r.Run(context.Background(), g, scoped, nil) + require.NoError(t, err) + assert.Equal(t, "yes", strings.TrimSpace(out.Stdout)) + + // printenv exits non-zero when the variable is unset. + bare := config.CommandSpec{Command: "printenv", Params: []string{"SCOPED"}} + out, err = r.Run(context.Background(), g, bare, nil) + require.Error(t, err) + assert.Empty(t, strings.TrimSpace(out.Stdout)) +} + +func TestShellRunner_Dir(t *testing.T) { + skipOnWindows(t) + t.Parallel() + dir := t.TempDir() + require.NoError(t, os.WriteFile(filepath.Join(dir, "marker.txt"), []byte("x"), 0o600)) + + var stdout, stderr bytes.Buffer + r := &ShellRunner{Stdout: &stdout, Stderr: &stderr} + g := &config.Group{Name: "g", Command: "ls"} + out, err := r.Run(context.Background(), g, + config.CommandSpec{Command: "ls", Dir: dir}, nil) + require.NoError(t, err) + assert.Contains(t, out.Stdout, "marker.txt") +} + +// The alternative, `cd x && y`, would force shell: on the whole group. +func TestShellRunner_DirKeepsArgvExec(t *testing.T) { + skipOnWindows(t) + t.Parallel() + dir := t.TempDir() + var stdout, stderr bytes.Buffer + r := &ShellRunner{Stdout: &stdout, Stderr: &stderr} + g := &config.Group{Name: "g", Command: "echo"} // no Shell set + out, err := r.Run(context.Background(), g, + config.CommandSpec{Command: "echo", Params: []string{"$(whoami)"}, Dir: dir}, nil) + require.NoError(t, err) + assert.Equal(t, "$(whoami)\n", out.Stdout) +} + +func TestShellRunner_DirMissingIsAnError(t *testing.T) { + skipOnWindows(t) + t.Parallel() + var stdout, stderr bytes.Buffer + r := &ShellRunner{Stdout: &stdout, Stderr: &stderr} + g := &config.Group{Name: "g", Command: "ls"} + _, err := r.Run(context.Background(), g, + config.CommandSpec{Command: "ls", Dir: "/no/such/directory/anywhere"}, nil) + require.Error(t, err) +} + +func TestShellRunner_Silent(t *testing.T) { + skipOnWindows(t) + t.Parallel() + + tests := []struct { + name string + silent bool + wantStreamed string + }{ + {name: "streams by default", wantStreamed: "noisy\n"}, + {name: "silent suppresses the live stream", silent: true, wantStreamed: ""}, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + var stdout, stderr bytes.Buffer + r := &ShellRunner{Stdout: &stdout, Stderr: &stderr} + g := &config.Group{Name: "g", Command: "echo"} + out, err := r.Run(context.Background(), g, + config.CommandSpec{Command: "echo", Params: []string{"noisy"}, Silent: tc.silent}, nil) + require.NoError(t, err) + assert.Equal(t, tc.wantStreamed, stdout.String()) + // Capture is unaffected either way. + assert.Equal(t, "noisy\n", out.Stdout) + assert.Equal(t, "noisy\n", out.Output) + }) + } +} + +func TestExpandCommands_RendersDirAndEnv(t *testing.T) { + t.Parallel() + e := New(&config.Config{Version: 2}) + g := &config.Group{Name: "package", Commands: []config.CommandSpec{{ + Command: "./package.sh", + Dir: `dist/{{ env "KEEPUP_TARGET" }}`, + Env: map[string]string{"SHA": `{{ output "build" }}`, "PLAIN": "kept"}, + Silent: true, + }}} + + got, err := e.expandCommands(g, template.Data{ + Outputs: map[string]result.RunResult{"build": {Stdout: "a31550e", Output: "a31550e"}}, + Env: map[string]string{"KEEPUP_TARGET": "linux"}, + }) + require.NoError(t, err) + require.Len(t, got, 1) + assert.Equal(t, "dist/linux", got[0].Dir) + assert.Equal(t, map[string]string{"SHA": "a31550e", "PLAIN": "kept"}, got[0].Env) + assert.True(t, got[0].Silent, "non-templated fields must survive expansion") +} + +func TestExpandCommands_BadDirTemplateFails(t *testing.T) { + t.Parallel() + e := New(&config.Config{Version: 2}) + g := &config.Group{Name: "g", Commands: []config.CommandSpec{ + {Command: "ls", Dir: "{{ bogusfunc }}"}, + }} + _, err := e.expandCommands(g, template.Data{}) + require.Error(t, err) + assert.Contains(t, err.Error(), "expand dir") +} + +func TestExpandCommands_BadEnvTemplateFails(t *testing.T) { + t.Parallel() + e := New(&config.Config{Version: 2}) + g := &config.Group{Name: "g", Commands: []config.CommandSpec{ + {Command: "ls", Env: map[string]string{"X": "{{ bogusfunc }}"}}, + }} + _, err := e.expandCommands(g, template.Data{}) + require.Error(t, err) + assert.Contains(t, err.Error(), "expand env") +} + +func TestShellRunner_SilentSuppressesStderrToo(t *testing.T) { + skipOnWindows(t) + t.Parallel() + var stdout, stderr bytes.Buffer + r := &ShellRunner{Stdout: &stdout, Stderr: &stderr} + g := &config.Group{Name: "g", Command: "sh", Shell: "/bin/sh"} + out, err := r.Run(context.Background(), g, + config.CommandSpec{Command: "echo oops 1>&2", IsShell: true, Silent: true}, nil) + require.NoError(t, err) + assert.Empty(t, stderr.String()) + assert.Equal(t, "oops\n", out.Stderr) +} diff --git a/internal/engine/runner.go b/internal/engine/runner.go index df0f1b6..86fd16d 100644 --- a/internal/engine/runner.go +++ b/internal/engine/runner.go @@ -22,11 +22,12 @@ const ( defaultPosixSh = "/bin/sh" ) -// Runner executes a single group and returns its structured RunResult. The -// params argument is authoritative for the command's arguments; implementations -// must not read g.Params or g.Commands. +// Runner executes one expanded command and returns its structured RunResult. +// The spec is authoritative for the command, its arguments, and its per-command +// overrides; the group supplies only the surrounding context (name, shell, env). +// Implementations must not read g.Command, g.Params, or g.Commands. type Runner interface { - Run(ctx context.Context, g *config.Group, params []string, globalEnv map[string]string) (result.RunResult, error) + Run(ctx context.Context, g *config.Group, spec config.CommandSpec, globalEnv map[string]string) (result.RunResult, error) } // ShellRunner executes a group via os/exec, optionally through a system shell. @@ -53,8 +54,10 @@ func NewShellRunner() *ShellRunner { // // The command and arguments come from user-supplied configuration; that is // the point of this tool. gosec G204 is suppressed for the exec call. -func (r *ShellRunner) Run(ctx context.Context, g *config.Group, params []string, globalEnv map[string]string) (result.RunResult, error) { - cmd := r.buildCmd(ctx, g, params, globalEnv) +func (r *ShellRunner) Run( + ctx context.Context, g *config.Group, spec config.CommandSpec, globalEnv map[string]string, +) (result.RunResult, error) { + cmd := r.buildCmd(ctx, g, spec, globalEnv) captureStdout := &safeBuf{} captureStderr := &safeBuf{} @@ -67,6 +70,11 @@ func (r *ShellRunner) Run(ctx context.Context, g *config.Group, params []string, if stderr == nil { stderr = os.Stderr } + if spec.Silent { + // Drop only the live writers; the capture buffers below are what + // {{ output }} and the cache replay read, so they stay wired. + stdout, stderr = io.Discard, io.Discard + } cmd.Stdout = io.MultiWriter(stdout, captureStdout, captureCombined) cmd.Stderr = io.MultiWriter(stderr, captureStderr, captureCombined) @@ -94,19 +102,21 @@ func (r *ShellRunner) Run(ctx context.Context, g *config.Group, params []string, // buildCmd assembles the exec.Cmd for a group invocation, honoring shell // opt-in and the layered environment. -func (r *ShellRunner) buildCmd(ctx context.Context, g *config.Group, params []string, globalEnv map[string]string) *exec.Cmd { +func (r *ShellRunner) buildCmd(ctx context.Context, g *config.Group, spec config.CommandSpec, globalEnv map[string]string) *exec.Cmd { var cmd *exec.Cmd if g.UseShell() { shell := pickShell(g.Shell) - full := g.Command - if len(params) > 0 { - full = g.Command + " " + strings.Join(params, " ") + full := spec.Command + if len(spec.Params) > 0 { + full = spec.Command + " " + strings.Join(spec.Params, " ") } cmd = exec.CommandContext(ctx, shell, shellFlag(), full) } else { - cmd = exec.CommandContext(ctx, g.Command, params...) //nolint:gosec // user-declared command + cmd = exec.CommandContext(ctx, spec.Command, spec.Params...) //nolint:gosec // user-declared command } - cmd.Env = mergeEnvs(os.Environ(), globalEnv, g.Env) + // Layering: process < global env: < group env: < command env:. + cmd.Env = mergeEnvs(os.Environ(), globalEnv, g.Env, spec.Env) + cmd.Dir = spec.Dir return cmd } diff --git a/internal/engine/runner_test.go b/internal/engine/runner_test.go index 84da0be..14680c9 100644 --- a/internal/engine/runner_test.go +++ b/internal/engine/runner_test.go @@ -58,7 +58,8 @@ func TestShellRunner_DirectExecNoShell(t *testing.T) { var stdout, stderr bytes.Buffer r := &ShellRunner{Stdout: &stdout, Stderr: &stderr} out, err := r.Run(context.Background(), - &config.Group{Name: "g", Command: "echo"}, tc.params, nil) + &config.Group{Name: "g", Command: "echo"}, + config.CommandSpec{Command: "echo", Params: tc.params}, nil) if tc.wantErr { require.Error(t, err) return @@ -77,7 +78,8 @@ func TestShellRunner_ShellModeOptIn(t *testing.T) { r := &ShellRunner{Stdout: &stdout, Stderr: &stderr} // Now shell substitutions DO work because the user opted in. out, err := r.Run(context.Background(), - &config.Group{Name: "g", Command: "echo $((1+2))", Shell: "/bin/sh"}, nil, nil) + &config.Group{Name: "g", Command: "echo $((1+2))", Shell: "/bin/sh"}, + config.CommandSpec{Command: "echo $((1+2))", IsShell: true}, nil) require.NoError(t, err) assert.Equal(t, "3\n", strings.TrimLeft(out.Output, " ")) } @@ -91,7 +93,7 @@ func TestShellRunner_ShellModeWithParams(t *testing.T) { // the result to "sh -c". This exercises the params-join branch. out, err := r.Run(context.Background(), &config.Group{Name: "g", Command: "echo", Shell: "/bin/sh"}, - []string{"hello", "world"}, nil) + config.CommandSpec{Command: "echo", Params: []string{"hello", "world"}, IsShell: true}, nil) require.NoError(t, err) assert.Equal(t, "hello world\n", out.Output) } @@ -104,7 +106,7 @@ func TestShellRunner_NilWritersFallbackToProcessStdio(t *testing.T) { // without asserting on the process stdio. r := &ShellRunner{} // Stdout and Stderr are both nil out, err := r.Run(context.Background(), - &config.Group{Name: "g", Command: "echo"}, []string{"x"}, nil) + &config.Group{Name: "g", Command: "echo"}, config.CommandSpec{Command: "echo", Params: []string{"x"}}, nil) require.NoError(t, err) assert.Equal(t, "x\n", out.Output) } @@ -114,7 +116,7 @@ func TestShellRunner_FailingCommand(t *testing.T) { t.Parallel() r := &ShellRunner{Stdout: io.Discard, Stderr: io.Discard} _, err := r.Run(context.Background(), - &config.Group{Name: "g", Command: "false"}, nil, nil) + &config.Group{Name: "g", Command: "false"}, config.CommandSpec{Command: "false"}, nil) require.Error(t, err) assert.Contains(t, err.Error(), `run "g"`) } @@ -125,7 +127,8 @@ func TestShellRunner_ContextCancellation(t *testing.T) { r := &ShellRunner{Stdout: io.Discard, Stderr: io.Discard} ctx, cancel := context.WithTimeout(context.Background(), 50*time.Millisecond) defer cancel() - _, err := r.Run(ctx, &config.Group{Name: "g", Command: "sleep"}, []string{"5"}, nil) + _, err := r.Run(ctx, &config.Group{Name: "g", Command: "sleep"}, + config.CommandSpec{Command: "sleep", Params: []string{"5"}}, nil) require.Error(t, err) } @@ -139,7 +142,7 @@ func TestShellRunner_EnvOverlayPrecedence(t *testing.T) { Command: "printenv", Env: map[string]string{"X": "group"}, }, - []string{"X"}, + config.CommandSpec{Command: "printenv", Params: []string{"X"}}, map[string]string{"X": "global"}, ) require.NoError(t, err) @@ -197,7 +200,7 @@ func TestShellRunner_SeparatesStdoutAndStderr(t *testing.T) { Command: "printf 'out'; printf 'err' >&2", Shell: "/bin/sh", } - rr, err := r.Run(context.Background(), g, nil, nil) + rr, err := r.Run(context.Background(), g, config.CommandSpec{Command: g.Command, Params: g.Params, IsShell: g.UseShell()}, nil) require.NoError(t, err) assert.Equal(t, "out", rr.Stdout) assert.Equal(t, "err", rr.Stderr) diff --git a/internal/engine/template_integration_test.go b/internal/engine/template_integration_test.go index d079533..64c7d6d 100644 --- a/internal/engine/template_integration_test.go +++ b/internal/engine/template_integration_test.go @@ -106,14 +106,14 @@ type recordingRunner struct { commands map[string]string } -func (r *recordingRunner) Run(_ context.Context, g *config.Group, params []string, _ map[string]string) (result.RunResult, error) { +func (r *recordingRunner) Run(_ context.Context, g *config.Group, spec config.CommandSpec, _ map[string]string) (result.RunResult, error) { r.mu.Lock() defer r.mu.Unlock() if r.params == nil { r.params = map[string]string{} r.commands = map[string]string{} } - r.params[g.Name] = strings.Join(params, ",") + r.params[g.Name] = strings.Join(spec.Params, ",") r.commands[g.Name] = g.Command stdout := r.outputs[g.Name] return result.RunResult{Stdout: stdout, Output: stdout, Status: "ok"}, nil @@ -134,10 +134,12 @@ func TestEngine_Template_BadCommandFailsGroup(t *testing.T) { // and echoes its first param as output so downstream refs resolve. type captureCommandRunner struct{ lastCommand string } -func (r *captureCommandRunner) Run(_ context.Context, g *config.Group, params []string, _ map[string]string) (result.RunResult, error) { +func (r *captureCommandRunner) Run( + _ context.Context, g *config.Group, spec config.CommandSpec, _ map[string]string, +) (result.RunResult, error) { r.lastCommand = g.Command - if len(params) > 0 { - return result.RunResult{Stdout: params[0], Output: params[0], Status: "ok"}, nil + if len(spec.Params) > 0 { + return result.RunResult{Stdout: spec.Params[0], Output: spec.Params[0], Status: "ok"}, nil } return result.RunResult{Status: "ok"}, nil }