From 1bdeb396c744df5d0feaaa3c8c3d31e8cbc1ede1 Mon Sep 17 00:00:00 2001 From: atrawog Date: Tue, 1 Sep 2026 08:15:03 +0200 Subject: [PATCH] fix(command): apply the step env: modifier to the executed command MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The step-level env: modifier (a base #Op field, spec/schema/_common.cue) was schema-declared but never wired: the command verb ran the command with the container's default env, so an env-scoped probe (e.g. OMARCHY_PATH=/tmp/...) silently tested the wrong configuration. RCA: no executor path read op.Env — the command plugin's RunVerb called cc.Exec().RunCapture(ctx, script) with no env, and the runner's EffectiveEnv (variable expansion) excludes op.Env. Fix: prepend the step env as shell exports (sorted keys, spec/shellquote quoting) to the command for the in-container, host-side, and background legs. Tests: TestCommandVerb_StepEnvReachesCommand (the export reaches the script) + TestCommandVerb_NoEnvLeavesCommandUntouched (no spurious exports). --- candy/plugin-command/plugin.go | 49 ++++++++++++++++++++++++----- candy/plugin-command/plugin_test.go | 47 +++++++++++++++++++++++++++ 2 files changed, 89 insertions(+), 7 deletions(-) diff --git a/candy/plugin-command/plugin.go b/candy/plugin-command/plugin.go index 162dbee..f322434 100644 --- a/candy/plugin-command/plugin.go +++ b/candy/plugin-command/plugin.go @@ -13,13 +13,17 @@ import ( "context" "embed" "errors" + "fmt" "os/exec" + "slices" + "strings" "time" "github.com/opencharly/plugin-command/candy/plugin-command/params" "github.com/opencharly/sdk" "github.com/opencharly/sdk/kit" pb "github.com/opencharly/spec/proto" + "github.com/opencharly/spec/shellquote" "github.com/opencharly/spec/spec" ) @@ -60,6 +64,17 @@ func (verb) RunVerb(ctx context.Context, cc kit.CheckContext, op *spec.Op) kit.R inContainer = false } + // The step's env: modifier (a base #Op field) is applied to the executed + // command as shell exports. RCA: the field was schema-declared but no executor + // path ever read it — the command ran with the container's default env, so an + // env-scoped probe (e.g. OMARCHY_PATH=/tmp/missing-omarchy) silently tested + // the wrong configuration. Prepend exports so both the in-container and + // host-side legs see the step env; values are shell-quoted (spec.ShellQuote). + cmd := in.Command + if len(op.Env) > 0 { + cmd = envExports(op.Env) + cmd + } + // Background path — host-side only, fire-and-forget. Plan teardown reaps via SIGTERM. if in.Background { if inContainer { @@ -68,14 +83,14 @@ func (verb) RunVerb(ctx context.Context, cc kit.CheckContext, op *spec.Op) kit.R if cc.Mode() == kit.ModeBox { return kit.Skip("background command not meaningful under charly check box") } - cmd := exec.Command("sh", "-c", in.Command) // not CommandContext — survives ctx cancel - if err := cmd.Start(); err != nil { + c := exec.Command("sh", "-c", cmd) // not CommandContext — survives ctx cancel + if err := c.Start(); err != nil { return kit.Failf("background start: %v", err) } - cc.AddBackground(cmd.Process.Pid) + cc.AddBackground(c.Process.Pid) // Reap asynchronously so a kill: SIGKILL doesn't leave a zombie. - go func() { _ = cmd.Wait() }() - return kit.Passf("backgrounded pid=%d", cmd.Process.Pid) + go func() { _ = c.Wait() }() + return kit.Passf("backgrounded pid=%d", c.Process.Pid) } var stdout, stderr string @@ -83,12 +98,12 @@ func (verb) RunVerb(ctx context.Context, cc kit.CheckContext, op *spec.Op) kit.R var err error started := time.Now() if inContainer { - stdout, stderr, exitCode, err = cc.Exec().RunCapture(ctx, wrapContainerCommand(in.Command)) + stdout, stderr, exitCode, err = cc.Exec().RunCapture(ctx, wrapContainerCommand(cmd)) } else { if cc.Mode() == kit.ModeBox { return kit.Skip("host-side command not meaningful under charly check box") } - c := exec.CommandContext(ctx, "sh", "-c", in.Command) + c := exec.CommandContext(ctx, "sh", "-c", cmd) stdout, stderr, exitCode, err = captureCmd(c) } elapsed := time.Since(started) @@ -148,6 +163,26 @@ func killedByCeiling(ctx context.Context, exitCode int) bool { return ctx.Err() != nil || exitCode == -1 } +// envExports renders the step's env: modifier as shell export lines, so the +// executed command sees the step env. Values are shell-quoted (spec.ShellQuote) +// so a value with spaces or metacharacters survives interpolation. Deterministic +// order (sorted keys) keeps the emitted script stable across runs. +func envExports(env map[string]string) string { + if len(env) == 0 { + return "" + } + keys := make([]string, 0, len(env)) + for k := range env { + keys = append(keys, k) + } + slices.Sort(keys) + var b strings.Builder + for _, k := range keys { + fmt.Fprintf(&b, "export %s=%s\n", k, shellquote.ShellQuote(env[k])) + } + return b.String() +} + // wrapContainerCommand is the shared kit helper (FU-11 — formerly duplicated in core + plugins). var wrapContainerCommand = kit.WrapContainerCommand diff --git a/candy/plugin-command/plugin_test.go b/candy/plugin-command/plugin_test.go index 747833f..082e477 100644 --- a/candy/plugin-command/plugin_test.go +++ b/candy/plugin-command/plugin_test.go @@ -190,6 +190,53 @@ func TestCommandVerb_NormalExitIsUnaffected(t *testing.T) { // matcher. Relocated from charly/plugin_command_relocated_test.go's // TestRelocatedCommandVerb_DispatchesViaKit (the check-role behavior half; the dispatch // wiring stays in charly). +// recordingExec is a kit.Executor that records the script it was asked to run, +// so a test can assert the step env: modifier was prepended as shell exports. +type recordingExec struct { + script string +} + +func (r *recordingExec) RunCapture(_ context.Context, script string) (string, string, int, error) { + r.script = script + return "", "", 0, nil +} +func (r *recordingExec) Kind() string { return "container" } + +// TestCommandVerb_StepEnvReachesCommand proves the step's env: modifier (a base +// #Op field) is applied to the executed command as shell exports — the RCA fix +// for the schema-declared-but-never-wired env field. The in-container leg must +// see `export KEY='value'` prepended to the command. +func TestCommandVerb_StepEnvReachesCommand(t *testing.T) { + rec := &recordingExec{} + cc := &fakeCC{mode: kit.ModeLive, exec: rec} + op := &spec.Op{ + PluginInput: map[string]any{"command": "test \"$TEST_ENV_VAR\" = \"hello env\""}, + Env: map[string]string{"TEST_ENV_VAR": "hello env"}, + } + res := verb{}.RunVerb(context.Background(), cc, op) + if res.Status != kit.StatusPass { + t.Fatalf("env-scoped command: want pass, got %v: %s", res.Status, res.Message) + } + if !strings.Contains(rec.script, "export TEST_ENV_VAR='hello env'") { + t.Fatalf("env-scoped command: script missing the env export, got: %s", rec.script) + } +} + +// TestCommandVerb_NoEnvLeavesCommandUntouched proves a step without env: is +// passed through unchanged (no spurious exports). +func TestCommandVerb_NoEnvLeavesCommandUntouched(t *testing.T) { + rec := &recordingExec{} + cc := &fakeCC{mode: kit.ModeLive, exec: rec} + op := &spec.Op{PluginInput: map[string]any{"command": "echo plain"}} + res := verb{}.RunVerb(context.Background(), cc, op) + if res.Status != kit.StatusPass { + t.Fatalf("plain command: want pass, got %v: %s", res.Status, res.Message) + } + if strings.Contains(rec.script, "export ") { + t.Fatalf("plain command: script unexpectedly has env exports, got: %s", rec.script) + } +} + func TestCommandVerb_HostSideForeground(t *testing.T) { res := verb{}.RunVerb(context.Background(), &fakeCC{mode: kit.ModeLive}, &spec.Op{PluginInput: map[string]any{"command": "echo charly-cmd-ok", "from_host": true},