Various bug fixes + reliability improvements on the Framer fork

.github/workflows/ci.yml

@@ -8,7 +8,7 @@ on:
     branches: [main]
 
 permissions:
-  contents: write
+  contents: read
 
 jobs:
   test:
@@ -62,12 +62,14 @@ jobs:
     if: startsWith(github.ref, 'refs/tags/v')
     needs: build
     runs-on: ubuntu-latest
+    permissions:
+      contents: write
     steps:
       - uses: actions/download-artifact@v8
         with:
           path: dist
           merge-multiple: true
-      - uses: softprops/action-gh-release@v3
+      - uses: softprops/action-gh-release@3d0d9888cb7fd7b750713d6e236d1fcb99157228 # v3.0.2
         with:
           files: dist/*
           generate_release_notes: true

Makefile

@@ -30,7 +30,7 @@ $(TARGETS): go.mod
 	make $(platform)
 
 test: deps
-	$(GO) test
+	$(GO) test -race ./...
 
 fmt:
 	$(GO) fmt

README.md

@@ -142,9 +142,19 @@ GLOBAL OPTIONS:
    --version, -v                                              print the version
 ```
 
+## Exit codes
+
+The wrapped command's exit code is passed through unchanged. ``env-aws-params``
+itself uses:
+
+- ``125`` — invalid usage or a Parameter Store error
+- ``126`` — the command was found but could not be started
+- ``127`` — the command was not found
+- ``128+N`` — the command was killed by signal ``N`` (e.g. ``143`` for ``SIGTERM``)
+
 ## Building from source
 
-This project uses [Go modules](https://go.dev/blog/using-go-modules) and requires Go 1.26+.
+This project uses [Go modules](https://go.dev/blog/using-go-modules) and requires Go 1.26.5+.
 
 ```bash
 go mod download

data.go

@@ -1,29 +1,57 @@
 package main
 
 import (
-	"fmt"
+	"maps"
 	"regexp"
-	"sort"
+	"slices"
 	"strings"
+
+	log "github.com/sirupsen/logrus"
 )
 
 var InvalidPattern = regexp.MustCompile(`[^a-zA-Z0-9_]`)
 
-func BuildEnvVars(parameters map[string]string, sanitize bool, strip bool, upcase bool) []string {
-	var vars []string
+// MergeEnvVars combines the inherited environment with SSM-derived variables,
+// letting Parameter Store win whenever a key is present in both.
+//
+// Precedence is expressed purely through ordering: os/exec de-duplicates
+// cmd.Env before handing it to the child and keeps the last occurrence of each
+// key, so ssmVars is appended second. Pass a nil environ to inherit nothing.
+// The result is always non-nil, because a nil cmd.Env makes os/exec fall back
+// to the parent's environment.
+func MergeEnvVars(ssmVars []string, environ []string) []string {
+	merged := make([]string, 0, len(environ)+len(ssmVars))
+	merged = append(merged, environ...)
+	merged = append(merged, ssmVars...)
+	return merged
+}
 
-	for k, v := range parameters {
+func BuildEnvVars(parameters map[string]string, sanitize bool, strip bool, upcase bool) []string {
+	// Transform in sorted parameter order so collisions resolve deterministically.
+	values := make(map[string]string, len(parameters))
+	sources := make(map[string]string, len(parameters))
+	for _, name := range slices.Sorted(maps.Keys(parameters)) {
+		key := name
 		if sanitize {
-			k = InvalidPattern.ReplaceAllString(k, "_")
+			key = InvalidPattern.ReplaceAllString(key, "_")
 		}
 		if strip {
-			k = InvalidPattern.ReplaceAllString(k, "")
+			key = InvalidPattern.ReplaceAllString(key, "")
 		}
 		if upcase {
-			k = strings.ToUpper(k)
+			key = strings.ToUpper(key)
 		}
-		vars = append(vars, fmt.Sprintf("%s=%s", k, v))
+		if prev, ok := sources[key]; ok {
+			log.Warnf("Parameters %q and %q both map to %s; keeping the value of %q", prev, name, key, name)
+		}
+		sources[key] = name
+		values[key] = parameters[name]
+	}
+
+	vars := make([]string, 0, len(values))
+	for key, value := range values {
+		vars = append(vars, key+"="+value)
 	}
-	sort.Strings(vars)
+	slices.Sort(vars)
 	return vars
 }

data_test.go

@@ -2,6 +2,7 @@ package main
 
 import (
 	"fmt"
+	"os/exec"
 	"sort"
 	"testing"
 )
@@ -19,6 +20,59 @@ func AssertEqual(t *testing.T, value []string, expect []string) {
 	}
 }
 
+// AssertEqualOrdered is AssertEqual without the sort, for assertions where the
+// ordering of the slice is itself the thing under test.
+func AssertEqualOrdered(t *testing.T, value []string, expect []string) {
+	t.Helper()
+	if len(value) != len(expect) {
+		t.Fatalf("expected %v entries, got %v: %v", len(expect), len(value), value)
+	}
+	for i := range expect {
+		if value[i] != expect[i] {
+			t.Errorf("Values at offset %v do not match: expected %q, got %q", i, expect[i], value[i])
+		}
+	}
+}
+
+func TestMergeEnvVarsAppendsSSMLast(t *testing.T) {
+	ssmVars := []string{"ONLY_SSM=ssm", "SHARED=from-ssm"}
+	environ := []string{"ONLY_ENV=env", "SHARED=from-environ"}
+
+	expectation := []string{
+		"ONLY_ENV=env", "SHARED=from-environ",
+		"ONLY_SSM=ssm", "SHARED=from-ssm",
+	}
+	AssertEqualOrdered(t, MergeEnvVars(ssmVars, environ), expectation)
+}
+
+func TestMergeEnvVarsPristineInheritsNothing(t *testing.T) {
+	ssmVars := []string{"ONLY_SSM=ssm"}
+	AssertEqualOrdered(t, MergeEnvVars(ssmVars, nil), ssmVars)
+}
+
+func TestMergeEnvVarsIsNeverNil(t *testing.T) {
+	// A nil result would make os/exec inherit the parent environment.
+	if merged := MergeEnvVars(nil, nil); merged == nil {
+		t.Fatal("expected a non-nil slice, got nil")
+	}
+}
+
+// The precedence contract holds only because os/exec keeps the last occurrence
+// of a duplicate key, so assert it against a real child process rather than
+// against the ordering of the slice alone.
+func TestMergeEnvVarsSSMWinsInChildProcess(t *testing.T) {
+	cmd := exec.Command("/bin/sh", "-c", `printf '%s' "$SHARED"`)
+	cmd.Env = MergeEnvVars([]string{"SHARED=from-ssm"}, []string{"SHARED=from-environ"})
+
+	out, err := cmd.Output()
+	if err != nil {
+		t.Fatalf("running child: %v", err)
+	}
+	if got := string(out); got != "from-ssm" {
+		t.Fatalf("expected child to see %q, got %q", "from-ssm", got)
+	}
+}
+
 func TestBuildEnvVarsUpcaseFalse(t *testing.T) {
 	var params map[string]string
 
@@ -56,6 +110,16 @@ func TestBuildEnvVarsUpperSanitize(t *testing.T) {
 	AssertEqual(t, envVars, expectation)
 }
 
+func TestBuildEnvVarsCollisionKeepsLastSortedParameter(t *testing.T) {
+	params := map[string]string{
+		"db-host": "from-dash",
+		"db_host": "from-underscore",
+	}
+
+	envVars := BuildEnvVars(params, true, false, true)
+	AssertEqual(t, envVars, []string{"DB_HOST=from-underscore"})
+}
+
 func TestBuildEnvVarsUpperStrip(t *testing.T) {
 	var params map[string]string
 

go.mod

@@ -1,6 +1,6 @@
 module env-aws-params
 
-go 1.26
+go 1.26.5
 
 require (
 	github.com/aws/aws-sdk-go-v2 v1.41.7

main.go

@@ -8,22 +8,32 @@ import (
 	"os"
 	"os/exec"
 	"strings"
+	"syscall"
 
 	log "github.com/sirupsen/logrus"
 	"github.com/urfave/cli/v3"
 	"golang.org/x/sync/errgroup"
 )
 
-var VersionString string
+var VersionString = "dev"
 
 func main() {
+	// Stop flag parsing at the wrapped command, so its own flags
+	// (e.g. sh -c) are passed through instead of rejected.
+	stopOnFirstArg := 1
 	cmd := &cli.Command{
-		Name:      "env-aws-params",
-		Usage:     "Application entry-point that injects SSM Parameter Store values as Environment Variables",
-		UsageText: "env-aws-params [global options] -p prefix command [command arguments]",
-		Version:   VersionString,
-		Flags:     cliFlags(),
-		Action:    action,
+		Name:         "env-aws-params",
+		Usage:        "Application entry-point that injects SSM Parameter Store values as Environment Variables",
+		UsageText:    "env-aws-params [global options] -p prefix command [command arguments]",
+		Version:      VersionString,
+		Flags:        cliFlags(),
+		Action:       action,
+		StopOnNthArg: &stopOnFirstArg,
+		// Flag parsing errors never reach action, so they are mapped to
+		// the documented usage-error exit code here.
+		OnUsageError: func(_ context.Context, _ *cli.Command, err error, _ bool) error {
+			return cli.Exit(errorPrefix(err), 125)
+		},
 	}
 	if err := cmd.Run(context.Background(), os.Args); err != nil {
 		log.Fatal(err)
@@ -38,16 +48,15 @@ func action(ctx context.Context, cmd *cli.Command) error {
 		log.SetOutput(io.Discard)
 	}
 
-	code, err := validateArgs(cmd.NArg(), cmd.Bool("sanitize"), cmd.Bool("strip"))
-	if code > 0 {
-		return cli.Exit(errorPrefix(err), code)
+	if err := validateArgs(cmd.NArg(), cmd.Bool("sanitize"), cmd.Bool("strip")); err != nil {
+		return cli.Exit(errorPrefix(err), 125)
 	}
 
 	var envVars []string
 	if len(cmd.StringSlice("prefix")) > 0 {
 		params, err := getParameters(ctx, cmd)
 		if err != nil {
-			return cli.Exit(errorPrefix(err), -1)
+			return cli.Exit(errorPrefix(err), 125)
 		}
 
 		envVars = BuildEnvVars(
@@ -64,19 +73,28 @@ func action(ctx context.Context, cmd *cli.Command) error {
 		log.Warn("No prefix set; executing command without retrieving SSM parameters")
 	}
 
-	// SSM-derived values come first so they win over inherited environ:
-	// glibc/musl/Apple libc all return the first match from getenv().
-	if !cmd.Bool("pristine") {
-		envVars = append(envVars, os.Environ()...)
+	// SSM-derived values are merged last so they win over the inherited
+	// environment; --pristine inherits nothing at all. See MergeEnvVars.
+	environ := os.Environ()
+	if cmd.Bool("pristine") {
+		environ = nil
 	}
+	envVars = MergeEnvVars(envVars, environ)
 
 	args := cmd.Args()
 	if err := RunCommand(args.First(), args.Tail(), envVars); err != nil {
 		var exitErr *exec.ExitError
 		if errors.As(err, &exitErr) {
-			return cli.Exit(errorPrefix(err), exitErr.ExitCode())
+			code := exitErr.ExitCode()
+			if status, ok := exitErr.Sys().(syscall.WaitStatus); ok && status.Signaled() {
+				code = 128 + int(status.Signal())
+			}
+			return cli.Exit(errorPrefix(err), code)
 		}
-		return cli.Exit(errorPrefix(err), 128)
+		if errors.Is(err, exec.ErrNotFound) || errors.Is(err, os.ErrNotExist) {
+			return cli.Exit(errorPrefix(err), 127)
+		}
+		return cli.Exit(errorPrefix(err), 126)
 	}
 	return nil
 }
@@ -168,14 +186,14 @@ func getParameters(ctx context.Context, cmd *cli.Command) (map[string]string, er
 	return values, nil
 }
 
-func validateArgs(nargs int, sanitize, strip bool) (int, error) {
+func validateArgs(nargs int, sanitize, strip bool) error {
 	if nargs == 0 {
-		return 1, errors.New("command not specified")
+		return errors.New("command not specified")
 	}
 
 	if sanitize && strip {
-		return 2, errors.New("--sanitize and --strip are mutually exclusive behaviors")
+		return errors.New("--sanitize and --strip are mutually exclusive behaviors")
 	}
 
-	return 0, nil
+	return nil
 }

main_test.go

@@ -6,41 +6,25 @@ import (
 )
 
 func TestValidateArgsNoPrefixIsValid(t *testing.T) {
-	code, err := validateArgs(1, false, false)
-	if code != 0 {
-		t.Fatalf("expected code to be 0, got %v", code)
-	}
-	if err != nil {
+	if err := validateArgs(1, false, false); err != nil {
 		t.Fatalf("expected err to be nil, got %v", err)
 	}
 }
 
 func TestValidateArgsMissingCommand(t *testing.T) {
-	code, err := validateArgs(0, false, false)
-	if code != 1 {
-		t.Fatalf("expected code to be 1, got %v", code)
-	}
-	if err == nil {
+	if err := validateArgs(0, false, false); err == nil {
 		t.Fatalf("expected err to be set, got nil")
 	}
 }
 
 func TestValidateArgsStripAndSanitize(t *testing.T) {
-	code, err := validateArgs(1, true, true)
-	if code != 2 {
-		t.Fatalf("expected code to be 2, got %v", code)
-	}
-	if err == nil {
+	if err := validateArgs(1, true, true); err == nil {
 		t.Fatalf("expected err to be set, got nil")
 	}
 }
 
 func TestValidateArgsValid(t *testing.T) {
-	code, err := validateArgs(1, false, true)
-	if code != 0 {
-		t.Fatalf("expected code to be 0, got %v", code)
-	}
-	if err != nil {
+	if err := validateArgs(1, false, true); err != nil {
 		t.Fatalf("expected err to be nil, got %v", err)
 	}
 }