From 19c8d04d1d726ed0245224abd13b03bc75a08251 Mon Sep 17 00:00:00 2001 From: Marcus Johansson Date: Thu, 2 Jul 2026 22:11:24 +0200 Subject: [PATCH 1/2] chore: add AGENTS.md and PLAN.md --- AGENTS.md | 47 ++++++++++++++++ PLAN.md | 157 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 204 insertions(+) create mode 100644 AGENTS.md create mode 100644 PLAN.md diff --git a/AGENTS.md b/AGENTS.md new file mode 100644 index 0000000..4626122 --- /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 (search up directory tree for .env-exec.yaml) +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 `Fetch(*config.RootConfig, map[string]string)`. Providers run in fixed order: `plain` → `gcp` → `gitlab`. All populate a shared `envVars` map. +- **Execution model**: Vars are injected into the current process via `os.Setenv` before `exec.Command` spawns the target command. +- **Config loading**: Searches current directory upward for `.env-exec.yaml`. Overridable via `ENV_EXEC_YAML` env var or `--config` flag. + +## Critical Known Issues (Do NOT Regress) + +| Issue | Location | Details | +|-------|----------|---------| +| Windows exit code | `internal/exec/exec.go:21` | Uses `syscall.WaitStatus` which is Unix-only. Fix: use `exitError.ExitCode()` | +| Shell injection | `internal/env/env.go:12` | Only escapes single quotes; `$`, `` ` ``, `\` in values are vulnerable | +| Hardcoded GitLab | `internal/provider/gitlab.go:66` | URL is `https://gitlab.com`; self-hosted unsupported | +| Silent failures | `gcp.go:37-41`, `gitlab.go:40-47` | Provider fetch failures log a warning and skip — downstream command fails with confusing "missing env var" | +| No log control | All providers | Use `log.Printf` for warnings — callers can't control format/destination/level | + +## 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..8f1e307 --- /dev/null +++ b/PLAN.md @@ -0,0 +1,157 @@ +# PLAN.md + +## Bugs + +### 1. Non-cross-platform exit code extraction +- **File**: `internal/exec/exec.go:21-22` +- **Issue**: `syscall.WaitStatus` is Unix-only. On Windows, `Sys()` returns a different type and the type assertion fails, causing the exit code to be silently lost. +- **Fix**: Use `exitError.ExitCode()` — it's cross-platform and available on `*exec.ExitError`. + +### 2. 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). + +### 3. 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. + +### 4. Silent skip on unresolvable GCP secrets +- **File**: `internal/provider/gcp.go:37-41` +- **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. + +### 5. Same silent-skip for GitLab variables +- **File**: `internal/provider/gitlab.go:40-47` +- **Issue**: Identical pattern to Bug 4. +- **Fix**: Same fix as Bug 4. + +### 6. `captureStdout` does not restore `os.Stdout` on panic +- **File**: `internal/env/env_test.go:12-23` +- **Issue**: If the test function panics, `os.Stdout` is never restored, causing cascading failures in subsequent tests. +- **Fix**: Use `defer os.Stdout = old` after saving the original. + +### 7. 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. + +### 8. Having both `value` and `valueFrom` is only a warning +- **File**: `internal/config/validation.go:40` +- **Issue**: A config entry can specify both sources simultaneously; `value` silently takes precedence but the warning may be missed. +- **Fix**: Make this an error, or clearly document that `value` always overrides `valueFrom`. + +## Security + +### 1. Shell injection vulnerability +- **File**: `internal/env/env.go:12` +- **Issue**: Only escapes single quotes. Values containing `$`, `` ` ``, or `\` are vulnerable to shell interpretation when used with `source <(env-exec)`. +- **Fix**: Escape `$`, `` ` ``, and `\` as well, or document that `source <(env-exec)` should only be used with trusted config files. + +### 2. 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`. + +### 3. Deprecated GitLab authorization header +- **File**: `internal/provider/gitlab.go:73` +- **Issue**: `PRIVATE-TOKEN` header is deprecated by GitLab. Modern approach is `Authorization: Bearer `. +- **Fix**: Switch to `Authorization: Bearer `. + +### 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. + +### 5. 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()`. + +### 6. 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 depends on provider execution order and is undefined. +- **Fix**: Add validation that rejects `valueFrom` entries with more than one source set. + +### 7. Swallowed HTTP error body read +- **File**: `internal/provider/gitlab.go:83-85` +- **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 use `defer resp.Body.Close()` at the top of the function. + +## 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 +No client-side check that `gcpSecretKeyRef.name` is non-empty before calling the API — the provider checks it at runtime, but this should be config-time validation. + +## 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 `Fetch` 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:95-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` is Unix-only +- **File**: `internal/exec/exec.go:21` +- **Fix**: Use `exitError.ExitCode()`. + +### 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: Windows exit code + Security 1: Shell injection | Low | High — core functionality | +| 2 | Bug 2: HTTP timeout + Bug 3: Context timeout | Low | High — reliability | +| 3 | Security 2: GitLab host config | Low | Medium — common pain point | +| 4 | Bug 4-5: Silent skip on fetch failure | Medium | Medium — UX | +| 5 | Design 1: Provider enable/disable | Low | Medium — developer UX | +| 6 | Quality 1: Dedup has*() functions | Low | Low — cleanup | +| 7 | Quality 3: Provider registry | Medium | Medium — extensibility | From 26b349eee75354cd1c2d845c76b3e3dc096a0123 Mon Sep 17 00:00:00 2001 From: Marcus Johansson Date: Thu, 2 Jul 2026 22:41:45 +0200 Subject: [PATCH 2/2] chore: fix AGENTS.md and PLAN.md based on code review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Fix provider interface method name: Fetch → Provide (with error return) - Fix config loading description: no upward search, no --config flag - Remove shell injection claim (single quotes protect against $/``/\) - Remove Windows exit code claim (syscall.WaitStatus works on Windows) - Fix GCP fetch failure line numbers (48-51, not 37-41) - Fix: plain.go has no logging, only gcp and gitlab do - Add new finding: validation.go:40 says 'value takes precedence' but valueFrom wins - Add new finding: validation.go:44-47 is dead code (Name != '' ⇒ Name == '' never fires) - Remove PRIVATE-TOKEN deprecation claim (still supported by GitLab) - Remove Body.Close() claim (already deferred at gitlab.go:81) - Remove 'undefined behavior' for dual valueFrom (deterministic last-writer-wins) - Fix defer syntax (os.Stdout = old is not a call; use defer func() { ... }()) - Reweight priority order to remove non-bugs --- AGENTS.md | 16 ++++----- PLAN.md | 106 ++++++++++++++++++++++++++---------------------------- 2 files changed, 59 insertions(+), 63 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 4626122..1e8f2d7 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -8,7 +8,7 @@ ``` cmd/env-exec/main.go # CLI entrypoint, flag parsing, orchestration -internal/config/ # Config loading (search up directory tree for .env-exec.yaml) +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) @@ -26,19 +26,19 @@ go mod tidy ## Architecture -- **Provider pattern**: Each provider implements `Fetch(*config.RootConfig, map[string]string)`. Providers run in fixed order: `plain` → `gcp` → `gitlab`. All populate a shared `envVars` map. +- **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**: Searches current directory upward for `.env-exec.yaml`. Overridable via `ENV_EXEC_YAML` env var or `--config` flag. +- **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 | |-------|----------|---------| -| Windows exit code | `internal/exec/exec.go:21` | Uses `syscall.WaitStatus` which is Unix-only. Fix: use `exitError.ExitCode()` | -| Shell injection | `internal/env/env.go:12` | Only escapes single quotes; `$`, `` ` ``, `\` in values are vulnerable | -| Hardcoded GitLab | `internal/provider/gitlab.go:66` | URL is `https://gitlab.com`; self-hosted unsupported | -| Silent failures | `gcp.go:37-41`, `gitlab.go:40-47` | Provider fetch failures log a warning and skip — downstream command fails with confusing "missing env var" | -| No log control | All providers | Use `log.Printf` for warnings — callers can't control format/destination/level | +| 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 diff --git a/PLAN.md b/PLAN.md index 8f1e307..32ceff1 100644 --- a/PLAN.md +++ b/PLAN.md @@ -2,82 +2,77 @@ ## Bugs -### 1. Non-cross-platform exit code extraction -- **File**: `internal/exec/exec.go:21-22` -- **Issue**: `syscall.WaitStatus` is Unix-only. On Windows, `Sys()` returns a different type and the type assertion fails, causing the exit code to be silently lost. -- **Fix**: Use `exitError.ExitCode()` — it's cross-platform and available on `*exec.ExitError`. +### 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. -### 2. HTTP client with no timeout +### 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). -### 3. No context deadline on GCP calls +### 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. -### 4. Silent skip on unresolvable GCP secrets -- **File**: `internal/provider/gcp.go:37-41` +### 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. -### 5. Same silent-skip for GitLab variables -- **File**: `internal/provider/gitlab.go:40-47` -- **Issue**: Identical pattern to Bug 4. -- **Fix**: Same fix as Bug 4. +### 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. -### 6. `captureStdout` does not restore `os.Stdout` on panic -- **File**: `internal/env/env_test.go:12-23` +### 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 os.Stdout = old` after saving the original. +- **Fix**: Use `defer func() { os.Stdout = old }()` after saving the original. -### 7. Duplicate env var names only produce a warning +### 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. -### 8. Having both `value` and `valueFrom` is only a warning -- **File**: `internal/config/validation.go:40` -- **Issue**: A config entry can specify both sources simultaneously; `value` silently takes precedence but the warning may be missed. -- **Fix**: Make this an error, or clearly document that `value` always overrides `valueFrom`. +### 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`. -## Security +### 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. -### 1. Shell injection vulnerability -- **File**: `internal/env/env.go:12` -- **Issue**: Only escapes single quotes. Values containing `$`, `` ` ``, or `\` are vulnerable to shell interpretation when used with `source <(env-exec)`. -- **Fix**: Escape `$`, `` ` ``, and `\` as well, or document that `source <(env-exec)` should only be used with trusted config files. +## Security -### 2. Hardcoded GitLab instance URL +### 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`. -### 3. Deprecated GitLab authorization header -- **File**: `internal/provider/gitlab.go:73` -- **Issue**: `PRIVATE-TOKEN` header is deprecated by GitLab. Modern approach is `Authorization: Bearer `. -- **Fix**: Switch to `Authorization: Bearer `. - -### 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. - -### 5. No environment variable name validation +### 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()`. -### 6. No mutual exclusion check on `valueFrom` sources +### 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 depends on provider execution order and is undefined. -- **Fix**: Add validation that rejects `valueFrom` entries with more than one source set. +- **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. -### 7. Swallowed HTTP error body read -- **File**: `internal/provider/gitlab.go:83-85` -- **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 use `defer resp.Body.Close()` at the top of the function. +### 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 @@ -87,8 +82,8 @@ No validation that `valueFrom` sources are mutually exclusive. A user could spec ### 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 -No client-side check that `gcpSecretKeyRef.name` is non-empty before calling the API — the provider checks it at runtime, but this should be config-time validation. +### 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 @@ -100,7 +95,7 @@ No client-side check that `gcpSecretKeyRef.name` is non-empty before calling the ### 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 `Fetch` to accept a provider-specific config, or use `interface{}` / options pattern. +- **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` @@ -112,7 +107,7 @@ No client-side check that `gcpSecretKeyRef.name` is non-empty before calling the - **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:95-98` +- **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 @@ -135,9 +130,10 @@ No support for referencing other variables in values (e.g., `{{ .Env.DB_HOST }}` ## Portability -### 1. `syscall.WaitStatus` is Unix-only +### 1. `syscall.WaitStatus` on Windows - **File**: `internal/exec/exec.go:21` -- **Fix**: Use `exitError.ExitCode()`. +- **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` @@ -148,10 +144,10 @@ No support for referencing other variables in values (e.g., `{{ .Env.DB_HOST }}` | Priority | Issue | Effort | Impact | |----------|-------|--------|--------| -| 1 | Bug 1: Windows exit code + Security 1: Shell injection | Low | High — core functionality | -| 2 | Bug 2: HTTP timeout + Bug 3: Context timeout | Low | High — reliability | -| 3 | Security 2: GitLab host config | Low | Medium — common pain point | -| 4 | Bug 4-5: Silent skip on fetch failure | Medium | Medium — UX | -| 5 | Design 1: Provider enable/disable | Low | Medium — developer UX | +| 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 |