Skip to content

feat: auto-load .env.<stackname> for per-stack env overrides#2129

Open
simple-agent-manager[bot] wants to merge 4 commits into
mainfrom
sam/implement-envstackname-auto-loading-01ksg2
Open

feat: auto-load .env.<stackname> for per-stack env overrides#2129
simple-agent-manager[bot] wants to merge 4 commits into
mainfrom
sam/implement-envstackname-auto-loading-01ksg2

Conversation

@simple-agent-manager
Copy link
Copy Markdown
Contributor

Closes #2128

Summary

  • add LoaderOptions.EnvFiles and wire additional env files into compose-go while preserving default .env loading
  • discover .env. after stack resolution and pass it to the compose loader when present
  • add loader/session coverage plus package testdata fixtures for precedence, missing/empty files, unresolved placeholders, and stack-only env files

Validation

  • go test -short ./...
  • make lint GO-NO-DEP-GO=1

Note: plain make lint currently fails during local Go bootstrap because the Makefile constructs an invalid go1.25.5.0 download URL from go.mod; GO-NO-DEP-GO=1 uses the installed go1.25.10 toolchain and passes.

Addresses review feedback from CTO-style code review:

1. Replace full Loader instantiation in stackEnvFiles with simple path
   resolution (filepath.Dir of config path or os.Getwd fallback). The
   previous approach created an entire compose Loader just to get a
   directory path — disproportionate overhead with subtle circular
   dependency risk.

2. Add slices.Contains guard in withEnvFiles() to prevent double-append.
   The function is called twice in newProjectOptions (before and after
   config path discovery), and without deduplication, stack env files
   would be appended twice.

3. Add Infof when a stack env file is loaded. Users need visibility into
   which files are affecting their deployment — a file's mere existence
   changing behavior is "action at a distance" that needs observability.

4. Add Debugf when an env file is filtered out (not in working dir).

5. Document the precedence model: compose-go's dotenv parser processes
   files in order with later files overriding earlier ones. Stack env
   files are appended after .env, giving them higher precedence.

6. Document WHY the directory check exists: it prevents stack env files
   resolved from cwd leaking into a project discovered via COMPOSE_FILE
   or directory walking.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@simple-agent-manager
Copy link
Copy Markdown
Contributor Author

Code Review Summary (3-round CTO adversarial review)

I ran this PR through 3 rounds of back-and-forth with a skeptical CTO subagent. Here's what was found and addressed:

Issues Found & Fixed (pushed as e190205f)

  1. Redundant Loader instantiationstackEnvFiles() was creating an entire compose.Loader (with full env loading, config path resolution, etc.) just to determine the working directory. Replaced with simple filepath.Dir(configPaths[0]) / os.Getwd() fallback.

  2. Double-append riskwithEnvFiles() is called twice in newProjectOptions (before and after config path discovery). Without deduplication, stack env files could be appended twice. Added slices.Contains guard.

  3. Silent behavior on load — When a .env.<stack> file IS picked up, the user had no feedback. Added term.Infof("Loading stack environment from %s") so users can trace which files affect their deployment.

  4. Missing documentation — The directory check (filepath.Dir(absEnvFile) == wd) was undocumented, making it look like dead code. Added comments explaining why it exists (prevents cwd-resolved paths leaking into COMPOSE_FILE-discovered projects).

  5. Precedence model undocumented — Added doc comment explaining that compose-go's dotenv parser uses last-wins semantics (verified by reading source at compose-go/v2/dotenv/env.go:GetEnvFromFile).

What Looked Good (no changes needed)

  • Path traversal safety: ValidateStackName enforces ^[a-z][a-z0-9]*$, so stack names can't contain special chars
  • Defensive copy of EnvFiles slice in session.go (prevents mutation of shared options)
  • Test coverage: 4 compose-level tests + 3 session-level test cases covering precedence, missing files, empty files
  • The CreateProjectForDebug refactoring (using GetWorkingDir() instead of os.Getwd()) is a net improvement for that method's correctness

Remaining Notes (not blockers)

  • Empty ConfigPaths case: when compose file is discovered via COMPOSE_FILE env or directory walking, stack env resolution falls back to os.Getwd(). This is correct for the standard workflow (user runs from project dir).
  • Feature is intentionally NOT opt-in — .env.<stack> follows established conventions (.env.local, .env.production). Zero impact on users who don't create the file.

Copy link
Copy Markdown
Member

@lionello lionello left a comment

Choose a reason for hiding this comment

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

Feels very, very sloppy. Agree with intent.

@@ -0,0 +1,8 @@
name: envstackfixture
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

rename folder to envstackfixture to avoid another level of indirection.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

pick up .env.STACK automatically

2 participants