fix(env): remove Env namespace, use direct process.env access#12822
Open
jerome-benoit wants to merge 18 commits intoanomalyco:devfrom
Open
fix(env): remove Env namespace, use direct process.env access#12822jerome-benoit wants to merge 18 commits intoanomalyco:devfrom
jerome-benoit wants to merge 18 commits intoanomalyco:devfrom
Conversation
nix/scripts/*.ts and patches/* are included in node_modules derivation. Changes to these files invalidate hashes but didn't trigger rebuild. Also removes non-existent install/ from node_modules.nix fileset. Fixes anomalyco#12817
…alyco#12698) - Add conditional caching to Env namespace: production mode uses direct process.env access, test mode uses snapshot isolation - Migrate 15 source files from direct process.env to Env.get/set/remove/all API - CLI: 9 files in cmd/ directory (acp, auth, github, uninstall, tui modules) - Core: 4 files (config, ide, lsp/server, share modules) - Enables test mode to isolate env variables per test without affecting global state - All migrations follow constraint: no spreads, no try/catch, no utility functions
Contributor
|
Hey! Your PR title Please update it to start with one of:
Where See CONTRIBUTING.md for details. |
Contributor
|
The following comment was made by an LLM, it may be inaccurate: No duplicate PRs found |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR completes the environment caching refactor by migrating remaining direct process.env usages to the Env namespace, aiming to keep production behavior “live” while isolating env state during tests.
Changes:
- Updated
Envimplementation to bypass cached state in production mode and use cached per-instance state in test mode. - Migrated multiple CLI/core modules from direct
process.envreads/writes toEnv.get/set. - Updated Nix hashing workflow triggers and adjusted
nix/node_modules.nixinputs.
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/opencode/src/env/index.ts | Adds production/test mode branching for env reads/writes and Env.all() behavior. |
| packages/opencode/src/share/share.ts | Uses Env.get for API URL and share-disable flag. |
| packages/opencode/src/share/share-next.ts | Uses Env.get for share-disable flag. |
| packages/opencode/src/lsp/server.ts | Uses Env.get("PATH")/Env.get("VIRTUAL_ENV") for LSP server discovery/spawn config. |
| packages/opencode/src/ide/index.ts | Uses Env.get for IDE detection and caller checks. |
| packages/opencode/src/config/config.ts | Uses Env.set for auth propagation and Env.get for {env:...} interpolation. |
| packages/opencode/src/cli/cmd/uninstall.ts | Uses Env.get for shell/xdg config detection. |
| packages/opencode/src/cli/cmd/tui/util/editor.ts | Uses Env.get for editor detection. |
| packages/opencode/src/cli/cmd/tui/util/clipboard.ts | Uses Env.get for OSC52 passthrough + Wayland detection. |
| packages/opencode/src/cli/cmd/tui/thread.ts | Uses Env.get("PWD") for cwd resolution. |
| packages/opencode/src/cli/cmd/tui/context/route.tsx | Uses Env.get to initialize route from env. |
| packages/opencode/src/cli/cmd/tui/attach.ts | Uses Env.get for server password header. |
| packages/opencode/src/cli/cmd/github.ts | Uses Env.get for GitHub action/runtime env inputs. |
| packages/opencode/src/cli/cmd/auth.ts | Uses Env.get when checking active provider env vars. |
| packages/opencode/src/cli/cmd/acp.ts | Uses Env.set to set OPENCODE_CLIENT. |
| nix/node_modules.nix | Removes ../install from the fileset inputs. |
| .github/workflows/nix-hashes.yml | Expands workflow trigger paths to include Nix/scripts/patches changes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Migrate process.env.KEY -> Env.get("KEY") in 5 files
- Migrate process.env.KEY = value -> Env.set("KEY", value) in 3 files
- Remove 2 obsolete TODO comments from provider.ts
- Add Env import to util/proxied.ts and index.ts
Files modified:
- config/config.ts (2 reads)
- index.ts (2 writes + import)
- provider/provider.ts (4 reads + 2 writes + removed 2 TODOs)
- shell/shell.ts (3 reads)
- util/proxied.ts (4 reads + import)
All process.env dot notation now migrated (except documented exclusions).
jerome-benoit
force-pushed
the
fix-env-caching-12698
branch
from
badedc7 to
4e8a1ef
Compare
jerome-benoit
changed the title
jerome-benoit
force-pushed
the
fix-env-caching-12698
branch
5 times, most recently
from
a0701b7 to
3dcc92b
Compare
# Conflicts: # nix/node_modules.nix # packages/opencode/src/share/share.ts
jerome-benoit
force-pushed
the
fix-env-caching-12698
branch
from
ed0b5ac to
95a2dba
Compare
Remove the Env namespace abstraction that was caching environment variables and causing issues with AWS SDK credential provider which writes to process.env dynamically. Changes: - Empty src/env/index.ts (Env namespace removed) - Replace all Env.get/set/remove/all calls with direct process.env access - Add test isolation via beforeEach/afterEach env snapshot in preload.ts - Delete env.test.ts (tested removed namespace) This ensures environment variable changes made by external libraries (like AWS SDK's credential provider chain) are immediately visible to the rest of the application. Closes anomalyco#12698
jerome-benoit
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Removes the
Envnamespace entirely. Fixes #12698.Why Remove Instead of Conditional Caching?
The suggested fix proposed conditional caching. However, the
Envnamespace has caused repeated issues since its introduction:Direct bugs
Env.set()mutates internal copy, notprocess.env— breaks provider SDKsEnv.all()returns stale snapshotWorkaround required
7ebe352af(fix(provider): use process.env directly for runtime env mutations #11482): bypassEnvin provider code for SAP/Bedrock SDKsRoot cause
The API semantics are misleading:
Env.set(key, value)appears to set an env var but external code (AWS SDK, child processes) never sees the change. This leaky abstraction led to mixedEnv.*/process.envusage and inconsistent behavior.Conditional caching preserves these problems. Removal eliminates them.
Changes
src/env/src/**/*.tsEnv.*→process.env[key]test/preload.tstest/env/env.test.tsVerification
bun test— 980 pass, 5 skip, 0 failbun run typecheck— clean