Fix sequential env evaluation and global context for command env#295
Merged
Fix sequential env evaluation and global context for command env#295
Conversation
Reviewer's GuideImplements sequential evaluation of env entries with an overridable base environment, wires global env into command-level env resolution, and adds tests and docs to cover the new semantics, plus some unrelated planning/spec docs for a future dependency failure tree feature. Sequence diagram for global and command env execution with baseEnvsequenceDiagram
participant Main
participant Config
participant GlobalEnvs as Envs_global
participant Command
participant CommandEnvs as Envs_command
Main->>Config: SetupEnv()
activate Config
Config->>GlobalEnvs: Execute(cfg, baseEnv=nil)
activate GlobalEnvs
GlobalEnvs->>GlobalEnvs: resolvedEnv = cloneMap(nil) => {}
loop for each key in Envs.Keys
alt env.Sh is not empty
GlobalEnvs->>GlobalEnvs: executeScript(cfg.Shell, env.Sh, resolvedEnv)
GlobalEnvs-->>GlobalEnvs: result
GlobalEnvs->>GlobalEnvs: env.Value = result
else env.Value already set
end
GlobalEnvs->>GlobalEnvs: resolvedEnv[key] = env.Value
end
GlobalEnvs-->>Config: ready=true
deactivate GlobalEnvs
Config-->>Main: global env resolved
deactivate Config
Main->>Command: GetEnv(cfg)
activate Command
Command->>Config: GetEnv()
Config-->>Command: baseEnv (resolved global env)
Command->>CommandEnvs: Execute(cfg, baseEnv)
activate CommandEnvs
CommandEnvs->>CommandEnvs: resolvedEnv = cloneMap(baseEnv)
loop for each key in Envs.Keys
alt env.Sh is not empty
CommandEnvs->>CommandEnvs: executeScript(cfg.Shell, env.Sh, resolvedEnv)
CommandEnvs-->>CommandEnvs: result
CommandEnvs->>CommandEnvs: env.Value = result
else env.Value already set
end
CommandEnvs->>CommandEnvs: resolvedEnv[key] = env.Value
end
CommandEnvs-->>Command: command env map
deactivate CommandEnvs
Command-->>Main: merged env for command
deactivate Command
Class diagram for Config, Command, Envs, and env execution helpersclassDiagram
class Env {
+string Value
+string Sh
}
class Envs {
+map~string,Env~ Mapping
+[]string Keys
+bool ready
+void Set(key string, value Env)
+error Execute(cfg Config, baseEnv map~string,string~)
}
class Config {
+string Shell
+Envs Env
+map~string,string~ GetEnv()
+error SetupEnv()
}
class Command {
+string Name
+Envs Env
+map~string,string~ GetEnv(cfg Config)
}
class Helpers {
+[]string convertEnvMapToList(envMap map~string,string~)
+string executeScript(shell string, script string, envMap map~string,string~)
}
Config "1" o-- "1" Envs : globalEnv
Command "1" o-- "1" Envs : commandEnv
Envs "*" o-- Env : Mapping
Config ..> Helpers : uses
Command ..> Helpers : uses
Envs ..> Helpers : calls
Config --> Envs : GetEnv() reads resolved Mapping
Config --> Envs : SetupEnv() Execute(cfg, nil)
Command --> Envs : GetEnv(cfg) Execute(cfg, cfg.GetEnv())
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="internal/config/config/env_execute_test.go" line_range="11-20" />
<code_context>
+ WorkDir: ".",
+ }
+
+ t.Run("resolves env entries sequentially", func(t *testing.T) {
+ envs := &Envs{}
+ envs.Set("ENGINE", Env{Name: "ENGINE", Value: "docker"})
+ envs.Set("COMPOSE", Env{Name: "COMPOSE", Sh: `echo "${ENGINE}-compose"`})
+
+ err := envs.Execute(cfg, nil)
+ if err != nil {
+ t.Fatalf("unexpected execute error: %s", err)
+ }
+
+ if got := envs.Mapping["COMPOSE"].Value; got != "docker-compose" {
+ t.Fatalf("expected COMPOSE=docker-compose, got %q", got)
+ }
+ })
+
+ t.Run("uses base env for sh evaluation", func(t *testing.T) {
</code_context>
<issue_to_address>
**issue (testing):** Add a unit test covering error propagation when `sh` execution fails
Current tests only cover successful `sh` executions. Please add a subtest where the script exits non‑zero (e.g. `Sh:
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Contributor
Greptile SummaryThis PR fixes two related gaps in
Confidence Score: 4/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant Executor
participant Config
participant GlobalEnvs as Config.Env (Global Envs)
participant Command
participant CmdEnvs as Command.Env (Command Envs)
participant Shell as Shell (bash -c)
Executor->>Config: SetupEnv()
Config->>GlobalEnvs: Execute(cfg, nil)
loop For each key in declaration order
alt sh entry
GlobalEnvs->>Shell: executeScript(shell, script, resolvedEnv)
Shell-->>GlobalEnvs: result
GlobalEnvs->>GlobalEnvs: resolvedEnv[key] = result
else static / checksum entry
GlobalEnvs->>GlobalEnvs: resolvedEnv[key] = value
end
end
GlobalEnvs-->>Config: ready=true
Executor->>Command: GetEnv(cfg)
Command->>Config: cfg.GetEnv() → Dump() of resolved global env
Config-->>Command: baseEnv (map[string]string)
Command->>CmdEnvs: Execute(cfg, baseEnv)
Note over CmdEnvs: resolvedEnv = clone(baseEnv)<br/>(global env as starting context)
loop For each key in declaration order
alt sh entry
CmdEnvs->>Shell: executeScript(shell, script, resolvedEnv)
Note over Shell: env = os.Environ() + resolvedEnv<br/>(resolvedEnv appended last → overrides)
Shell-->>CmdEnvs: result
CmdEnvs->>CmdEnvs: resolvedEnv[key] = result
else static entry
CmdEnvs->>CmdEnvs: resolvedEnv[key] = static value
end
end
CmdEnvs-->>Command: ready=true
Command-->>Executor: Dump() → map[string]string
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+1
to
+5
| # Dependency Failure Tree Implementation Plan | ||
|
|
||
| > **For agentic workers:** REQUIRED: Use superpowers:subagent-driven-development (if subagents available) or superpowers:executing-plans to implement this plan. Steps use checkbox (`- [ ]`) syntax for tracking. | ||
|
|
||
| **Goal:** When a `lets` command (or any of its `depends`) fails, print an indented tree to stderr showing the full dependency chain with the failing node highlighted in red. |
Contributor
There was a problem hiding this comment.
Unrelated planning document included
This file — along with docs/specs/2026-03-13-dependency-failure-tree-design.md — describes a different, unimplemented feature ("Dependency Failure Tree") that has no relationship to the sequential env-evaluation fix this PR delivers. Neither file contains code that is part of this PR's changeset.
Including future-feature plans/specs in a feature PR for an unrelated change muddies the history and could confuse reviewers. These two documents should either be removed from this branch or landed in their own dedicated PR/branch once the dependency-failure-tree work actually begins.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
kindermax
force-pushed
the
feature/fix/env-sequential-eval
branch
from
eddf099 to
333991d
Compare
- pass resolved env into `sh` evaluation so later keys can use earlier ones - allow command-level `env` scripts to read already-resolved global env - document order-dependent behavior and add unit + bats coverage
kindermax
force-pushed
the
feature/fix/env-sequential-eval
branch
from
66108fb to
709dccb
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
enventries in declaration order soshvalues can reference earlier keys in the same block.shenv values to use global env keys.letsenv values override process env duringshevaluation.Testing
internal/config/config/env_execute_test.go:resolves env entries sequentiallyuses base env for sh evaluationresolved lets env overrides process envkeeps cached values after first executiontests/env_dependency.batswith fixturetests/env_dependency/lets.yaml:shdepends on previously resolved global envshdepends on previously resolved command envshreads global envSummary by Sourcery
Ensure environment variables defined in config are evaluated sequentially and can reference previously resolved values, including global env from commands, and document the new semantics.
Bug Fixes:
Enhancements:
Documentation:
Tests:
Chores: