Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions AGENTS.md
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,13 @@

## Project Conventions

### Environment Access in `x`

- All environment reads and writes under `x/`, including tests and test
helpers, must use `internal/execbroker` (`Getenv`, `LookupEnv`, `Setenv`,
`Unsetenv`, `Clearenv`, `Environ`, or `ExpandEnv`). Do not call the `os` or
`syscall` environment APIs directly from `x/`.

### Command and Flag Changes

- Follow the existing Cobra command style.
Expand Down
168 changes: 168 additions & 0 deletions internal/execbroker/execbroker.go
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,125 @@ func Do(scope Scope, fn func() error) error {
return fn()
}

// Getenv returns the value of key from the active command scope. Without a
// scope, it has the same behavior as os.Getenv.
func Getenv(key string) string {
id := goid.Get()
scopeMu.RLock()
scope, ok := scopes[id]
if ok && scope.Env != nil {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P3] Getenv doc/behavior: a non-nil scope Env masks the process environment

The comment says "Without a scope, it has the same behavior as os.Getenv," which is accurate. But when a scope exists with a non-nil scope.Env that does not contain key, envValue returns "" and Getenv returns "" without consulting os.Getenv (line 75-78). A variable present in the real process environment is then reported as empty.

Currently masked in production because the only Do scope (build.go) starts with Env nil and the first Setenv copies the full os.Environ(). But since formulas now route os.Getenv here, a partial-Env scope would diverge from real os.Getenv. Either document this ("with a scope, only variables present in the scope environment are visible") or fall back to os.Getenv when the key is absent from a non-nil scope.Env.

value := envValue(scope.Env, key)
scopeMu.RUnlock()
return value
}
scopeMu.RUnlock()
return os.Getenv(key)
}

// LookupEnv returns the value of key and whether it is present in the active
// command scope. Without a scope, it has the same behavior as os.LookupEnv.
func LookupEnv(key string) (string, bool) {
id := goid.Get()
scopeMu.RLock()
scope, ok := scopes[id]
if ok && scope.Env != nil {
value, present := envLookup(scope.Env, key)
scopeMu.RUnlock()
return value, present
}
scopeMu.RUnlock()
return os.LookupEnv(key)
}

// Setenv sets key in the active command scope. The first scoped write copies
// the process environment so later commands inherit the update without
// changing the process-wide environment. Without a scope, it has the same
// behavior as os.Setenv.
func Setenv(key, value string) error {
if err := validateEnvKey(key); err != nil {
return err
}
for i := 0; i < len(value); i++ {
if value[i] == 0 {
return fmt.Errorf("invalid environment variable value for %q", key)
}
}

id := goid.Get()
scopeMu.Lock()
defer scopeMu.Unlock()

scope, ok := scopes[id]
if !ok {
return os.Setenv(key, value)
}
if scope.Env == nil {
scope.Env = os.Environ()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P3] os.Environ() allocates the full env under the write lock

os.Environ() copies the entire process environment while scopeMu.Lock() is held, blocking every other goroutine's Getenv/Setenv/rewrite/Println. It only runs on the first scoped write per scope, so impact is bounded — but the allocation could be done outside the lock (or snapshotted at Do() time) to keep the global mutex critical section minimal.

}
scope.Env = setEnv(scope.Env, key, value)
scopes[id] = scope
return nil
}

// Unsetenv removes key from the active command scope. Without a scope, it has
// the same behavior as os.Unsetenv.
func Unsetenv(key string) error {
if err := validateEnvKey(key); err != nil {
return err
}

id := goid.Get()
scopeMu.Lock()
defer scopeMu.Unlock()

scope, ok := scopes[id]
if !ok {
return os.Unsetenv(key)
}
if scope.Env == nil {
scope.Env = os.Environ()
}
scope.Env = unsetEnv(scope.Env, key)
scopes[id] = scope
return nil
}

// Clearenv removes all variables from the active command scope. Without a
// scope, it has the same behavior as os.Clearenv.
func Clearenv() {
id := goid.Get()
scopeMu.Lock()
defer scopeMu.Unlock()

scope, ok := scopes[id]
if !ok {
os.Clearenv()
return
}
scope.Env = []string{}
scopes[id] = scope
}

// Environ returns a copy of the active command scope environment. Without a
// scope, it has the same behavior as os.Environ.
func Environ() []string {
id := goid.Get()
scopeMu.RLock()
scope, ok := scopes[id]
if ok && scope.Env != nil {
env := clone(scope.Env)
scopeMu.RUnlock()
return env
}
scopeMu.RUnlock()
return os.Environ()
}

// ExpandEnv expands variables using the active command scope environment.
func ExpandEnv(s string) string {
return os.Expand(s, Getenv)
}

// Println writes to the stdout configured for the active scope.
func Println(a ...any) (int, error) {
w := io.Writer(os.Stdout)
Expand Down Expand Up @@ -182,3 +301,52 @@ func clone(in []string) []string {
}
return append([]string(nil), in...)
}

func envValue(env []string, key string) string {
value, _ := envLookup(env, key)
return value
}

func envLookup(env []string, key string) (string, bool) {
prefix := key + "="
for i := len(env) - 1; i >= 0; i-- {
if len(env[i]) >= len(prefix) && env[i][:len(prefix)] == prefix {
return env[i][len(prefix):], true
}
}
return "", false
}

func setEnv(env []string, key, value string) []string {
prefix := key + "="
for i := range env {
if len(env[i]) >= len(prefix) && env[i][:len(prefix)] == prefix {
env[i] = prefix + value
return env
}
}
return append(env, prefix+value)
}

func unsetEnv(env []string, key string) []string {
prefix := key + "="
out := env[:0]
for _, entry := range env {
if len(entry) < len(prefix) || entry[:len(prefix)] != prefix {
out = append(out, entry)
}
}
return out
}

func validateEnvKey(key string) error {
if key == "" {
return fmt.Errorf("invalid environment variable name")
}
for i := 0; i < len(key); i++ {
if key[i] == '=' || key[i] == 0 {
return fmt.Errorf("invalid environment variable name %q", key)
}
}
return nil
}
169 changes: 169 additions & 0 deletions internal/execbroker/execbroker_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -236,6 +236,175 @@ func TestDoRestoresNestedScope(t *testing.T) {
}
}

func TestScopedGetenvAndSetenv(t *testing.T) {
key := "EXECBROKER_SCOPED_ENV_TEST"
if err := os.Setenv(key, "process"); err != nil {
t.Fatal(err)
}
t.Cleanup(func() { _ = os.Unsetenv(key) })

err := Do(Scope{}, func() error {
if got := Getenv(key); got != "process" {
t.Fatalf("Getenv before Setenv = %q, want process", got)
}
if err := Setenv(key, "scoped"); err != nil {
return err
}
if got := Getenv(key); got != "scoped" {
t.Fatalf("Getenv after Setenv = %q, want scoped", got)
}
if got := os.Getenv(key); got != "process" {
t.Fatalf("process environment = %q, want process", got)
}
var got string
prefix := key + "="
for _, entry := range Command("command").Env {
if len(entry) >= len(prefix) && entry[:len(prefix)] == prefix {
got = entry[len(prefix):]
}
}
if got != "scoped" {
t.Fatalf("command environment = %q, want scoped", got)
}
return nil
})
if err != nil {
t.Fatal(err)
}
if got := Getenv(key); got != "process" {
t.Fatalf("Getenv after scope = %q, want process", got)
}
}

func TestScopedEnvironmentAPIs(t *testing.T) {
key := "EXECBROKER_ENV_APIS_TEST"
if err := os.Setenv(key, "process"); err != nil {
t.Fatal(err)
}
t.Cleanup(func() { _ = os.Unsetenv(key) })

err := Do(Scope{}, func() error {
if got, ok := LookupEnv(key); got != "process" || !ok {
t.Fatalf("LookupEnv before Setenv = %q, %v; want process, true", got, ok)
}
if got := ExpandEnv("$" + key); got != "process" {
t.Fatalf("ExpandEnv before Setenv = %q, want process", got)
}
if err := Setenv(key, "scoped"); err != nil {
return err
}
if got, ok := LookupEnv(key); got != "scoped" || !ok {
t.Fatalf("LookupEnv after Setenv = %q, %v; want scoped, true", got, ok)
}
if got := ExpandEnv("${" + key + "}"); got != "scoped" {
t.Fatalf("ExpandEnv after Setenv = %q, want scoped", got)
}
if got := envValue(Environ(), key); got != "scoped" {
t.Fatalf("Environ value = %q, want scoped", got)
}
if err := Unsetenv(key); err != nil {
return err
}
if _, ok := LookupEnv(key); ok {
t.Fatal("LookupEnv after Unsetenv = present, want absent")
}
if err := Setenv(key, "scoped-again"); err != nil {
return err
}
Clearenv()
if got := len(Environ()); got != 0 {
t.Fatalf("Environ after Clearenv = %d entries, want zero", got)
}
if got := Getenv(key); got != "" {
t.Fatalf("Getenv after Clearenv = %q, want empty", got)
}
if got := os.Getenv(key); got != "process" {
t.Fatalf("process environment = %q, want process", got)
}
return nil
})
if err != nil {
t.Fatal(err)
}
if got := os.Getenv(key); got != "process" {
t.Fatalf("process environment after scope = %q, want process", got)
}
}

func TestScopedEnvironmentRestoresNestedScope(t *testing.T) {
key := "EXECBROKER_NESTED_ENV_TEST"
err := Do(Scope{Env: []string{key + "=outer"}}, func() error {
if got := Getenv(key); got != "outer" {
t.Fatalf("outer Getenv = %q, want outer", got)
}
if err := Do(Scope{Env: []string{key + "=inner"}}, func() error {
if got := Getenv(key); got != "inner" {
t.Fatalf("inner Getenv = %q, want inner", got)
}
return Setenv(key, "inner-updated")
}); err != nil {
return err
}
if got := Getenv(key); got != "outer" {
t.Fatalf("restored Getenv = %q, want outer", got)
}
return nil
})
if err != nil {
t.Fatal(err)
}
}

func TestScopedEnvironmentIsGoroutineLocal(t *testing.T) {
key := "EXECBROKER_GOROUTINE_ENV_TEST"
ready := make(chan struct{}, 2)
start := make(chan struct{})
results := make(chan string, 2)

for _, value := range []string{"one", "two"} {
value := value
go func() {
_ = Do(Scope{}, func() error {
if err := Setenv(key, value); err != nil {
return err
}
ready <- struct{}{}
<-start
results <- Getenv(key)
return nil
})
}()
}
for range 2 {
<-ready
}
close(start)

got := map[string]bool{<-results: true, <-results: true}
if !got["one"] || !got["two"] {
t.Fatalf("goroutine-scoped values = %v, want one and two", got)
}
}

func TestScopedSetenvRejectsInvalidValues(t *testing.T) {
for _, test := range []struct {
label string
name string
value string
}{
{label: "empty name", name: "", value: "value"},
{label: "equals in name", name: "bad=name", value: "value"},
{label: "nul in name", name: "bad\x00name", value: "value"},
{label: "nul in value", name: "name", value: "bad\x00value"},
} {
t.Run(test.label, func(t *testing.T) {
if err := Do(Scope{}, func() error { return Setenv(test.name, test.value) }); err == nil {
t.Fatalf("Setenv(%q, %q) error = nil", test.name, test.value)
}
})
}
}

func TestDoReturnsError(t *testing.T) {
want := errors.New("failed")
if err := Do(Scope{}, func() error { return want }); !errors.Is(err, want) {
Expand Down
Loading
Loading