fix: an imported @currentEnv must win over the --env fallback - #1036
fix: an imported @currentEnv must win over the --env fallback#1036chriscors wants to merge 2 commits into
Conversation
A directory whose schema gets `@currentEnv` through `@import()` never loaded its own `.env.[env]` files when a fallback env was supplied. `DirectoryDataSource._finishInit()` resolves `currentEnv` before imports are processed, which is too early for an imported `@currentEnv` — so it falls through to `graph.envFlagFallback`. That value is truthy, so the post-import re-check (guarded on `!currentEnv`) never runs, and the directory stays pinned to the fallback env for the rest of the load. There is no fallback in the CLI's default path, so this only shows up when one is set — which the Next.js integration always does, passing `--env development`/`production` from `next dev`/`next build` to match `@next/env`. Its own comment says the user should be able to "ignore it by setting their own `@currentEnv`", and that holds when the decorator is in the same schema; it does not when the schema imports it. In a monorepo where each app's `.env.schema` is just `@import(../../)` of a root schema that owns `@currentEnv=$APP_ENV`, the root's `.env.[APP_ENV]` loads correctly while the app's own is silently skipped in favour of `.env.production` — with no error, and nothing in the loaded-file list to suggest the app file was considered. `_resolveCurrentEnv()` now reports whether the value came from the fallback. A fallback is treated as provisional: it is not acted on before imports, and the post-import re-check runs for it. Deliberately not acted on early, rather than loaded and later overridden — loading `.env.<fallback>` leaves its values in the graph even once the real env is known, so a key present only there would leak into the wrong environment. A resolved `@currentEnv` is still final and still loads before imports, so import conditions can read those values.
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
Important
Deferring fallback environment files until after imports introduces a confirmed regression in conditional import resolution.
Reviewed changes Reviewed the environment-selection ordering, fallback provenance tracking, imported @currentEnv precedence, and regression coverage.
- Fallback provenance:
_resolveCurrentEnv()now distinguishes a graph-level fallback from a value resolved through@currentEnv. - Import precedence: fallback-specific files are deferred until imports have had a chance to introduce
@currentEnv. - Regression coverage: the new test verifies that an imported
@currentEnvselects the importing directory's matching environment file instead of the fallback file.
azure/gpt-5.6-sol ｜ 𝕏
| // both be wrong and leave that file's values in the graph after the correct env is | ||
| // known. A resolved @currentEnv is final, so it loads immediately (before imports), | ||
| // which is what lets import conditions read those values. | ||
| if (currentEnv && !fromFallback) { |
There was a problem hiding this comment.
Deferring every fallback here breaks schemas whose @import(enabled=...) or imported @disable conditions depend on values overridden by .env.<fallback>. A minimal fallback-only case now resolves the schema default first and then errors when the environment file tries to override that early-resolved item, so the pre-import availability of fallback-specific values needs to be preserved when the fallback remains authoritative.
Technical details
# Fallback-specific values load too late for import conditions
## Affected sites
- `packages/varlock/src/env-graph/lib/data-source.ts:1010` - skips loading `.env.<fallback>` before `_processImports()`.
- `packages/varlock/src/env-graph/lib/data-source.ts:1028` - loads the fallback file only after schema imports have already resolved their condition dependencies.
## Required outcome
- When no imported `@currentEnv` supersedes the fallback, values from `.env.<fallback>` and `.env.<fallback>.local` must be registered before schema import and disable conditions are evaluated.
- When an imported `@currentEnv` does supersede the fallback, values from the fallback-specific files must not remain in the graph.
## Reproduction
A schema with `AUTH_MODE=none` and `@import(./.env.azure, enabled=eq($AUTH_MODE, "azure"))`, a fallback of `staging`, and `.env.staging` containing `AUTH_MODE=azure` now fails with `"AUTH_MODE" was already resolved during early initialization ... and cannot be redefined by .env.staging`. Before this change, the staging value enabled the import.
|
Thanks for this! Will have to dig in a bit. I know it was made considerably more complex by attempting to match next’s default behavior of setting env based on running build vs dev (if no currentEnv is set). I’d rather not do that, but we were trying to make it work as a drop in replacement. |
Deferring `.env.<fallback>` until after imports was a real regression, and the review is right about the mechanism: an import condition or an imported `@disable` may read a value that `.env.<fallback>` overrides. The reviewer's case now has a test — a schema with `AUTH_MODE=none`, a `staging` fallback whose `.env.staging` sets `AUTH_MODE=azure`, and `@import(..., enabled=eq($AUTH_MODE, "azure"))`. Deferring made `AUTH_MODE` early-resolve to `none`, so the import was skipped and the later file then failed with "already resolved during early initialization ... cannot be redefined". Verified as a regression rather than assumed: the new test passes on main and failed on the previous commit here. So the fallback loads before imports exactly as it did. What changes is only the post-import re-check, which now runs when the value came from the fallback and not just when there was no value at all. If a real `@currentEnv` then resolves to a different env, that env's files are loaded too — after the fallback's, so they take precedence. Known limitation, called out in the code: files already loaded for a superseded fallback stay in the graph. A key present ONLY in `.env.<fallback>` will still be set. Dropping them would mean removing a data source and the config items it created, and there is no mechanism for that — it looked worse to invent one here than to document the edge. In the motivating case nothing is loaded early at all, since `.env.production` does not exist.
|
Good catch — that's a real regression, and I've reverted the part of the change that caused it. I verified it rather than taking it on faith: your case as a test passes on Deferring made What changed in 0f25087: On your second required outcome — "values from the fallback-specific files must not remain in the graph" — I have not done that, deliberately. Dropping them means removing a data source and the config items it already created, and I couldn't find a mechanism for that; inventing one felt like a much bigger change than this bug warrants, and easy to get subtly wrong. So a key present only in Happy to take a swing at it if you'd point me at the right seam — or to leave it as a documented edge, since in the motivating case nothing loads early anyway ( Full suite green: 1798 passed, 1 skipped. Typecheck and lint clean. |
|
Certainly! Noticed while trying to implement multi-env overrides across multiple next apps. Let me know if I can provide any add'l followup. |
There was a problem hiding this comment.
Important
The fallback preload fixes the prior regression when the fallback remains authoritative, but it introduces an incorrect graph when an imported @currentEnv supersedes that fallback.
Reviewed changes Reviewed the changes since the prior Pullfrog review that restored fallback-specific loading before imports while retaining post-import @currentEnv resolution.
- Restored fallback preload: Loaded fallback-specific files before schema imports so import and disable conditions can see their overrides.
- Retained imported precedence: Re-resolved
@currentEnvafter imports and loaded a different selected environment afterward. - Added regression coverage: Added a focused test for fallback values used by import conditions.
azure/gpt-5.6-sol ｜ 𝕏
| // real @currentEnv has to win over it, the same way it does when declared in this | ||
| // schema directly. | ||
| // | ||
| // NOTE: files already loaded for a superseded fallback env stay in the graph. The |
There was a problem hiding this comment.
Loading the fallback before _processImports() lets it permanently enable, skip, or disable imports even when this later discovers that another environment is authoritative. Loading the resolved environment afterward cannot undo those decisions, so the final graph can still reflect the fallback rather than the imported @currentEnv.
Technical details
# Superseded fallback controls irreversible import decisions
## Affected sites
- `packages/varlock/src/env-graph/lib/data-source.ts:1008` - registers fallback-specific condition values.
- `packages/varlock/src/env-graph/lib/data-source.ts:1014` - processes imports and imported `@disable` decorators using those values.
- `packages/varlock/src/env-graph/lib/data-source.ts:1028` - discovers the actual environment only after those decisions have been committed.
## Required outcome
- If an imported `@currentEnv` supersedes the fallback, import and disable decisions must reflect the selected environment, with no fallback-specific source affecting the final graph.
- Add a regression case where fallback and selected environments produce opposite results for an import condition or imported `@disable`.
|
Confirmed as real — and it fails on I built your case as a test: fallbackEnv: 'staging',
files: {
'.env.schema': `# @import(./shared/)
# @import(./.env.azure, enabled=eq($AUTH_MODE, "azure"))
# ---
AUTH_MODE=none`,
'shared/.env.schema': `# @currentEnv=$APP_ENV
# ---
APP_ENV=dev`,
'.env.staging': 'AUTH_MODE=azure',
'.env.azure': 'AZURE_ITEM=val-from-.env.azure',
},
expectNotInSchema: ['AZURE_ITEM'],
The two findings can't both be satisfied in one pass. Your first one requires Breaking it needs a design change, and I don't think it belongs in a bug-fix PR from a first-time contributor. The two shapes I can see:
What this PR does is strictly narrower: the env flag now resolves correctly through an import, which is what makes per-app So: happy to leave the PR scoped here, or to attempt (1) or (2) if you tell me which you'd prefer and whether the removal machinery is something you'd want to exist. I can also add the case above as a skipped/ |
|
Thanks again for digging into this. Your original commit (b012969) had the right semantics: |
2 participants
The bug
A directory whose schema gets
@currentEnvthrough@import()never loads its own.env.[env]files when a fallback env is supplied.Minimal repro — same directory, same files, the only difference is the flag:
with:
APP_ENV=stgthroughout. Note the root's.env.stgdoes load — so@currentEnvresolves fine — while the app's own is skipped in favour of a.env.productionthat doesn't exist. No error, and nothing in the loaded-file list hints the app file was considered:Why
DirectoryDataSource._finishInit()resolvescurrentEnvbefore imports are processed, which is too early for an imported@currentEnv, so it falls through tograph.envFlagFallback. That value is truthy, so the post-import re-check — guarded on!currentEnv— never runs, and the directory stays pinned to the fallback for the rest of the load.There's no fallback in the CLI's default path, so this only surfaces when one is set. The Next.js integration always sets one:
next-env-compat.tspasses--env development/productionfromnext dev/next buildto match@next/env. Its comment says the user should be able to "ignore it by setting their own@currentEnv" — which holds when the decorator is in the same schema (there's already a passing test for that), and doesn't when the schema imports it.That makes the monorepo layout in Monorepos → one schema per project unusable for per-environment values under Next.js: each app's
.env.schemais just@import(../../), so no app can have its own.env.[env]. Worse, both staging and production deploys runnext build→--env production, so they resolve identically.The fix
_resolveCurrentEnv()now reports whether the value came from the fallback. A fallback is treated as provisional:Deliberately not acted on early rather than loaded and later overridden: loading
.env.<fallback>leaves its values in the graph even once the real env is known, so a key present only there would leak into the wrong environment.A resolved
@currentEnvis unchanged — still final, still loaded before imports, so import conditions can read those values.Tests
One test added next to the existing
fallback env value is ignored if currentEnv is present, covering the same intent when@currentEnvarrives via an import. It fails onmain:Full suite green with the fix: 1797 passed, 1 skipped.
typecheckandeslintclean.Also verified end to end against the real monorepo this came from — building the patched CLI and running the exact command the Next integration shells out to (
load --env production) now resolves the app-level value.Related
#428 is a different symptom of
@currentEnv+@import(env flag must be declared in the same schema when using a pick list); this path is separate and doesn't fix that one.