refactor(provider/env): document env var contract and validate reserved names#690
Draft
refactor(provider/env): document env var contract and validate reserved names#690
Conversation
Provider options are uppercased and injected as subprocess env vars. If an option collides with a system-set name like PROVIDER_ID or WORKSPACE_ID, it silently overwrites the system value. This adds validation to reject reserved names at provider config parse time.
Adds comprehensive doc comments to ToEnvironment and GetBaseEnvironment explaining which env vars are set, where they come from, and why DEVPOD_PROVIDER must not be explicitly added to the subprocess env.
Verifies that PROVIDER_ID and WORKSPACE_PROVIDER carry the provider name, that DEVPOD_PROVIDER is not duplicated, and that extraEnv vars are included.
📝 WalkthroughWalkthroughThe changes add documentation comments for environment construction functions, implement validation to reject provider options using reserved DevPod environment variable names, and introduce corresponding unit tests to verify the environment handling and validation behavior. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Suggested labels
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
pkg/provider/env_test.go (1)
54-59: Add a regression test forextraEnv["DEVPOD_PROVIDER"].Current tests don’t cover the contract-critical case where callers explicitly pass
DEVPOD_PROVIDERthroughextraEnv. Add that case so this behavior can’t regress silently.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/provider/env_test.go` around lines 54 - 59, Extend the existing test for ToEnvironment to include the contract-critical key by adding extraEnv["DEVPOD_PROVIDER"] (e.g., set to a sentinel value) and assert the produced environ contains that exact key/value; update or add a test (e.g., modify TestToEnvironment_IncludesExtraEnv or add TestToEnvironment_PreservesDevpodProvider) that calls ToEnvironment(nil, nil, nil, extra) with extra containing DEVPOD_PROVIDER and uses assertEnvContains to verify it is preserved.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/provider/env.go`:
- Around line 38-41: The code currently appends all entries from extraEnv
including DEVPOD_PROVIDER which violates the documented restriction; update the
logic that iterates over extraEnv (the block that appends extraEnv entries in
pkg/provider/env.go) to skip any entry whose key equals "DEVPOD_PROVIDER"
(case-sensitive) so it is never added to the resulting environment for child
devpod processes; include a short comment above the filtering to reference the
DEVPOD_PROVIDER restriction for future readers.
In `@pkg/provider/parse_test.go`:
- Around line 29-39: The test loop in TestValidate_RejectsReservedOptionNames
builds a ProviderConfig without required Exec.Command, so validate(cfg) may fail
on exec.command instead of reserved-name checks; update the cfg in the loop to
include a valid Exec with Command set (e.g., cfg.Exec = &types.Exec{Command:
"true"}) so validation reaches reserved-option-name logic when calling
validate(cfg) for each reserved name.
In `@pkg/provider/parse.go`:
- Around line 25-42: The reservedOptionNames map is missing several
contract-critical env var names, allowing them to be used as provider options;
update the reservedOptionNames declaration to include "DEVPOD_PROVIDER",
"DEVPOD", "DEVPOD_OS", "DEVPOD_ARCH", and "DEVPOD_LOG_LEVEL" (the same missing
keys must also be added where the map is duplicated around the other occurrence
referenced in the comment). Ensure these names match the keys produced by
GetBaseEnvironment so provider options cannot override those system variables.
---
Nitpick comments:
In `@pkg/provider/env_test.go`:
- Around line 54-59: Extend the existing test for ToEnvironment to include the
contract-critical key by adding extraEnv["DEVPOD_PROVIDER"] (e.g., set to a
sentinel value) and assert the produced environ contains that exact key/value;
update or add a test (e.g., modify TestToEnvironment_IncludesExtraEnv or add
TestToEnvironment_PreservesDevpodProvider) that calls ToEnvironment(nil, nil,
nil, extra) with extra containing DEVPOD_PROVIDER and uses assertEnvContains to
verify it is preserved.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2905ad22-124c-480b-be78-524fb5f627fa
📒 Files selected for processing (4)
pkg/provider/env.gopkg/provider/env_test.gopkg/provider/parse.gopkg/provider/parse_test.go
Comment on lines
+38
to
+41
| // Note: DEVPOD_PROVIDER is reserved by the --provider CLI flag via inheritFlagsFromEnvironment | ||
| // in cmd/root.go. It is inherited from the parent process via os.Environ() but must not be | ||
| // explicitly set here, as doing so would override the global provider selection for child | ||
| // devpod processes. |
There was a problem hiding this comment.
Documented DEVPOD_PROVIDER restriction is not enforced for extraEnv.
The comment says DEVPOD_PROVIDER must not be explicitly set, but Line 55-Line 57 blindly appends all extraEnv entries, including DEVPOD_PROVIDER.
Suggested patch
for k, v := range extraEnv {
+ if k == "DEVPOD_PROVIDER" {
+ continue
+ }
osEnviron = append(osEnviron, k+"="+v)
}Also applies to: 55-57
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/provider/env.go` around lines 38 - 41, The code currently appends all
entries from extraEnv including DEVPOD_PROVIDER which violates the documented
restriction; update the logic that iterates over extraEnv (the block that
appends extraEnv entries in pkg/provider/env.go) to skip any entry whose key
equals "DEVPOD_PROVIDER" (case-sensitive) so it is never added to the resulting
environment for child devpod processes; include a short comment above the
filtering to reference the DEVPOD_PROVIDER restriction for future readers.
Comment on lines
+29
to
+39
| for _, name := range reserved { | ||
| cfg := &ProviderConfig{ | ||
| Name: "test-provider", | ||
| Options: map[string]*types.Option{ | ||
| name: {Description: "test"}, | ||
| }, | ||
| } | ||
| err := validate(cfg) | ||
| if err == nil { | ||
| t.Errorf("expected error for reserved option name %q, got nil", name) | ||
| } |
There was a problem hiding this comment.
Reserved-name test can pass for unrelated validation errors.
TestValidate_RejectsReservedOptionNames does not set Exec.Command, so validate(cfg) can fail with exec.command is required even if reserved-name validation is broken. This makes the test a false positive risk.
Suggested patch
import (
+ "strings"
"testing"
"github.com/skevetter/devpod/pkg/types"
)
@@
for _, name := range reserved {
cfg := &ProviderConfig{
Name: "test-provider",
Options: map[string]*types.Option{
name: {Description: "test"},
},
+ Exec: ProviderCommands{
+ Command: []string{"echo hello"},
+ },
}
err := validate(cfg)
- if err == nil {
- t.Errorf("expected error for reserved option name %q, got nil", name)
+ if err == nil || !strings.Contains(err.Error(), "reserved environment variable name") {
+ t.Errorf("expected reserved-name error for option %q, got: %v", name, err)
}
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/provider/parse_test.go` around lines 29 - 39, The test loop in
TestValidate_RejectsReservedOptionNames builds a ProviderConfig without required
Exec.Command, so validate(cfg) may fail on exec.command instead of reserved-name
checks; update the cfg in the loop to include a valid Exec with Command set
(e.g., cfg.Exec = &types.Exec{Command: "true"}) so validation reaches
reserved-option-name logic when calling validate(cfg) for each reserved name.
Comment on lines
+25
to
+42
| var reservedOptionNames = map[string]bool{ | ||
| "PROVIDER_ID": true, | ||
| "PROVIDER_CONTEXT": true, | ||
| "PROVIDER_FOLDER": true, | ||
| "WORKSPACE_ID": true, | ||
| "WORKSPACE_UID": true, | ||
| "WORKSPACE_PROVIDER": true, | ||
| "WORKSPACE_CONTEXT": true, | ||
| "WORKSPACE_FOLDER": true, | ||
| "WORKSPACE_SOURCE": true, | ||
| "WORKSPACE_ORIGIN": true, | ||
| "WORKSPACE_PICTURE": true, | ||
| "MACHINE_ID": true, | ||
| "MACHINE_CONTEXT": true, | ||
| "MACHINE_FOLDER": true, | ||
| "MACHINE_PROVIDER": true, | ||
| "LOFT_PROJECT": true, | ||
| } |
There was a problem hiding this comment.
Reserved-name validation is incomplete and misses contract-critical env vars.
reservedOptionNames currently omits DEVPOD_PROVIDER (explicitly reserved in this PR’s contract) and several system vars set by GetBaseEnvironment (DEVPOD, DEVPOD_OS, DEVPOD_ARCH, DEVPOD_LOG_LEVEL). Those names can still be used as provider options and bypass the new protection.
Suggested patch
var reservedOptionNames = map[string]bool{
+ "DEVPOD": true,
+ "DEVPOD_OS": true,
+ "DEVPOD_ARCH": true,
+ "DEVPOD_LOG_LEVEL": true,
+ "DEVPOD_PROVIDER": true,
"PROVIDER_ID": true,
"PROVIDER_CONTEXT": true,
"PROVIDER_FOLDER": true,
"WORKSPACE_ID": true,
"WORKSPACE_UID": true,
"WORKSPACE_PROVIDER": true,
"WORKSPACE_CONTEXT": true,
"WORKSPACE_FOLDER": true,
"WORKSPACE_SOURCE": true,
"WORKSPACE_ORIGIN": true,
"WORKSPACE_PICTURE": true,
"MACHINE_ID": true,
"MACHINE_CONTEXT": true,
"MACHINE_FOLDER": true,
"MACHINE_PROVIDER": true,
"LOFT_PROJECT": true,
}Also applies to: 101-107
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/provider/parse.go` around lines 25 - 42, The reservedOptionNames map is
missing several contract-critical env var names, allowing them to be used as
provider options; update the reservedOptionNames declaration to include
"DEVPOD_PROVIDER", "DEVPOD", "DEVPOD_OS", "DEVPOD_ARCH", and "DEVPOD_LOG_LEVEL"
(the same missing keys must also be added where the map is duplicated around the
other occurrence referenced in the comment). Ensure these names match the keys
produced by GetBaseEnvironment so provider options cannot override those system
variables.
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
Follow-up to PR #615 which identified architectural confusion around provider environment variables.
PROVIDER_ID,WORKSPACE_ID, etc.) — prevents silent overwrites in subprocess environmentsToEnvironmentandGetBaseEnvironmentwith comprehensive doc commentsPROVIDER_IDis the canonical provider name source andDEVPOD_PROVIDERis not duplicatedDEVPOD_PROVIDERis reserved by the--providerCLI flag and must not be explicitly set in subprocess environmentsTest plan
go test ./pkg/provider/ -vpasses (5 new tests)go build ./...succeedsSummary by CodeRabbit