diff --git a/AGENTS.md b/AGENTS.md new file mode 100644 index 0000000..1e8f2d7 --- /dev/null +++ b/AGENTS.md @@ -0,0 +1,47 @@ +# AGENTS.md + +## Project Overview + +`env-exec` is a Go CLI tool that injects environment variables from multiple sources before executing a command. Inspired by Kubernetes pod spec syntax. Sources: plain values, GCP Secret Manager, GitLab CI/CD variables. + +## Directory Structure + +``` +cmd/env-exec/main.go # CLI entrypoint, flag parsing, orchestration +internal/config/ # Config loading (reads .env-exec.yaml from CWD; overridden by ENV_EXEC_YAML env var) +internal/env/ # Prints export statements, sets process env vars +internal/exec/ # Runs command via os/exec, forwards exit code +internal/provider/ # Provider interface + implementations (plain, gcp, gitlab) +``` + +## Build & Test + +```bash +go build ./... +go test ./... +go mod tidy +``` + +**Go version: 1.26** — keep in sync across `go.mod` and all GitHub workflows. + +## Architecture + +- **Provider pattern**: Each provider implements `Provide(config *config.RootConfig, envVars map[string]string) error`. Providers run in fixed order: `plain` → `gcp` → `gitlab`. All populate a shared `envVars` map. Note: `valueFrom` providers run after `plain`, so `valueFrom` values overwrite `value` entries for the same key. +- **Execution model**: Vars are injected into the current process via `os.Setenv` before `exec.Command` spawns the target command. +- **Config loading**: Reads `.env-exec.yaml` from the current working directory. Overridable via `ENV_EXEC_YAML` env var only (no `--config` flag). + +## Critical Known Issues (Do NOT Regress) + +| Issue | Location | Details | +|-------|----------|---------| +| Exit code style | `internal/exec/exec.go:21-22` | Uses `syscall.WaitStatus` — consider `exitError.ExitCode()` for idiomatic/cross-platform code | +| Silent failures | `internal/provider/gcp.go:48-51`, `internal/provider/gitlab.go:45-47` | Provider fetch failures log a warning and skip — downstream command fails with confusing "missing env var" | +| Wrong precedence warning | `internal/config/validation.go:40` | Warning text says "value takes precedence" but `valueFrom` actually wins (providers overwrite) | +| Dead code | `internal/config/validation.go:44-47` | Inner `Name == ""` check can never fire because `hasGCP` is `Name != ""` — dead code | +| No log control | `gcp.go:49`, `gitlab.go:40,46` | Providers use `log.Printf` for warnings — callers can't control format/destination/level; `plain.go` has no logging | + +## Conventions + +- No comments unless explicitly requested +- Tests in `*_test.go` alongside source files +- Follow existing code style and patterns diff --git a/PLAN.md b/PLAN.md new file mode 100644 index 0000000..32ceff1 --- /dev/null +++ b/PLAN.md @@ -0,0 +1,153 @@ +# PLAN.md + +## Bugs + +### 1. Wrong precedence warning text +- **File**: `internal/config/validation.go:40` +- **Issue**: Warning says "value takes precedence" but `valueFrom` actually wins. Providers run `plain` → `gcp` → `gitlab` (`provider.go:28-32`); `plain.go:9` writes `value` first, then `gcp.go:53` / `gitlab.go:50` overwrite the same map key. Following the suggestion to "document that value always overrides valueFrom" would enshrine the opposite of runtime behavior. +- **Fix**: Correct the warning text to say "valueFrom takes precedence", or make it an error if both are set. + +### 2. Dead code — uncheckable GCP name +- **File**: `internal/config/validation.go:44-47` +- **Issue**: `hasGCP` is `GCPSecretKeyRef.Name != ""`, so the inner check `if env.ValueFrom.GCPSecretKeyRef.Name == ""` can never fire. +- **Fix**: Remove the dead code. + +### 3. HTTP client with no timeout +- **File**: `internal/provider/gitlab.go:75` +- **Issue**: `http.Client{}` has zero timeout. If the GitLab API is slow or unresponsive, the request hangs indefinitely. +- **Fix**: Add a `Timeout` (e.g., 30s). + +### 4. No context deadline on GCP calls +- **File**: `internal/provider/gcp.go:19` +- **Issue**: `context.Background()` with no deadline — hung GCP API calls block forever. +- **Fix**: Use `context.WithTimeout` or pass a context through the provider. + +### 5. Silent skip on unresolvable GCP secrets +- **File**: `internal/provider/gcp.go:48-51` +- **Issue**: Fetch failure logs a warning and `continue`s. The user's command fails with a confusing "missing env var" error and the real cause may be scrolled off screen. +- **Fix**: Make fetch failures fatal (return error), or at minimum buffer all warnings and print them once before execution. + +### 6. Same silent-skip for GitLab variables +- **File**: `internal/provider/gitlab.go:45-47` +- **Issue**: Identical pattern to Bug 5. +- **Fix**: Same fix as Bug 5. + +### 7. `captureStdout` does not restore `os.Stdout` on panic +- **File**: `internal/env/env_test.go:11-24` +- **Issue**: If the test function panics, `os.Stdout` is never restored, causing cascading failures in subsequent tests. +- **Fix**: Use `defer func() { os.Stdout = old }()` after saving the original. + +### 8. Duplicate env var names only produce a warning +- **File**: `internal/config/validation.go:23-26` +- **Issue**: Duplicate names silently overwrite the first value ("last wins"). Missing names are a hard error — this is inconsistent. +- **Fix**: Either make duplicates an error or document the "last wins" behavior. + +### 9. Having both `value` and `valueFrom` is only a warning +- **File**: `internal/config/validation.go:39-41` +- **Issue**: A config entry can specify both sources simultaneously; `valueFrom` wins (providers overwrite). The warning may be missed. +- **Fix**: Make this an error, or clearly document that `valueFrom` always overrides `value`. + +### 10. Swallowed HTTP error body read +- **File**: `internal/provider/gitlab.go:83-84` +- **Issue**: On HTTP error, `bodyBytes, _ := io.ReadAll(resp.Body)` ignores the read error. An I/O failure during error body read is silently lost. +- **Fix**: Check the error or handle the read failure. + +## Security + +### 1. Hardcoded GitLab instance URL +- **File**: `internal/provider/gitlab.go:66` +- **Issue**: `https://gitlab.com/api/v4/...` — self-hosted GitLab instances unsupported. A user could accidentally authenticate with a token that has no access to gitlab.com. +- **Fix**: Add a `gitlabHost` config field (or env var) that defaults to `https://gitlab.com`. + +### 2. No environment variable name validation +- **File**: `internal/config/validation.go` +- **Issue**: No validation on env var name format. POSIX requires `[A-Za-z_][A-Za-z0-9_]*`. Names with spaces, hyphens, or leading digits are silently accepted and will cause the spawned command to fail. +- **Fix**: Validate names in `Validate()`. + +### 3. No mutual exclusion check on `valueFrom` sources +- **File**: `internal/config/validation.go` +- **Issue**: A `valueFrom` block can specify both `gcpSecretKeyRef` and `gitlabVariableKeyRef` simultaneously. The behavior is deterministic last-writer-wins (gitlab overwrites gcp), but undocumented. +- **Fix**: Either add validation that rejects dual-source entries or document the precedence clearly. + +### 4. Token visible in process environment +- **File**: `internal/provider/gitlab.go:29` +- **Issue**: Token lives in process env — visible via `/proc//environ` to any process with sufficient permissions. +- **Fix**: Document that the token should have minimal required permissions. + +## Missing Validation + +### 1. `valueFrom` mutual exclusion +No validation that `valueFrom` sources are mutually exclusive. A user could specify both GCP and GitLab refs simultaneously. + +### 2. GCP secret version format +No client-side validation of the `version` field. Non-numeric strings for numeric versions produce opaque API errors. + +### 3. GCP secret name emptiness (dead code) +No client-side check that `gcpSecretKeyRef.name` is non-empty — the code at `validation.go:44-47` is dead code. + +## Code Quality + +### 1. Duplicate `has*()` functions +- **Files**: `internal/provider/gcp.go:59-66` and `gitlab.go:56-63` +- **Issue**: Two structurally identical functions iterating over `cfg.Env` to check if a specific nested field is non-empty. +- **Fix**: Factor into a generic helper: `func hasValueSource(cfg, check func(EnvConfig) bool) bool`. + +### 2. Empty struct providers +- **File**: `internal/provider/provider.go:12-24` +- **Issue**: Providers are empty structs serving only as type identifiers. This makes adding provider configuration impossible without changing the interface. +- **Fix**: Change `Provide` to accept a provider-specific config, or use `interface{}` / options pattern. + +### 3. Hardcoded provider registry +- **File**: `internal/provider/provider.go:27-33` +- **Issue**: Adding a new provider requires modifying `AllProviders()`. +- **Fix**: Use a registry pattern (`var providers = make(map[string]Provider)`) with `Register(name, Provider)` for extensibility. + +### 4. Package-level struct for single use +- **File**: `internal/provider/gitlab.go:14-21` +- **Issue**: `GitlabVariable` struct is only used inside `getGitlabVariable()`. Should be a local type to reduce package-level API surface. + +### 5. Fragile defer in test loop +- **File**: `internal/env/env_test.go:96-98` +- **Issue**: `defer os.Unsetenv(k)` inside a loop — fragile, breaks with `t.Parallel()`. + +### 6. Non-idiomatic HTTP request construction +- **File**: `internal/provider/gitlab.go:68` +- **Issue**: `http.NewRequest("GET", url, nil)` — modern form is `http.NewRequestWithContext(context.Background(), ...)`. + +## Design Improvements + +### 1. No way to enable/disable providers +All providers always run. No configuration to skip a provider (e.g., skip GCP when running locally during development). + +### 2. `--dry-run` leaks secrets +All values including secrets are printed in plaintext during dry-run. A `--masked` option (similar to GitLab CI masked variables) would prevent accidental secret exposure. + +### 3. No environment-specific configs +No support for dev/staging/prod config selection (e.g., `.env-exec.dev.yaml`). + +### 4. No templating +No support for referencing other variables in values (e.g., `{{ .Env.DB_HOST }}`). + +## Portability + +### 1. `syscall.WaitStatus` on Windows +- **File**: `internal/exec/exec.go:21` +- **Issue**: `syscall.WaitStatus` actually exists on Windows and the assertion succeeds, so it's not broken. However, `exitError.ExitCode()` is more idiomatic and clearer. +- **Fix**: Replace with `exitError.ExitCode()` for readability. + +### 2. POSIX shell output format +- **File**: `internal/env/env.go:12-13` +- **Issue**: Outputs `export VAR='value'` syntax — not compatible with Windows cmd/PowerShell. The `source <(env-exec)` example uses bash-specific process substitution. +- **Fix**: Document the limitation, or add a `--shell` flag for output format selection. + +## Recommended Priority Order + +| Priority | Issue | Effort | Impact | +|----------|-------|--------|--------| +| 1 | Bug 1: Wrong precedence warning + Bug 2: Dead code | Low | Medium — correctness | +| 2 | Bug 3: HTTP timeout + Bug 4: Context timeout | Low | High — reliability | +| 3 | Bug 5-6: Silent skip on fetch failure | Medium | Medium — UX | +| 4 | Bug 7: `captureStdout` no defer | Low | Low — test fragility | +| 5 | Security 1: GitLab host config | Low | Medium — common pain point | +| 6 | Quality 1: Dedup has*() functions | Low | Low — cleanup | +| 7 | Quality 3: Provider registry | Medium | Medium — extensibility |