refactor(cli): replace blanket env binding with selective DEVSY_* opt-in#419
Merged
Conversation
Replaces the prior inheritFlagsFromEnvironment loop, which auto-bound every flag in the tree to a DEVSY_<FLAG> env var, with an explicit allowlist driven by flags.BindEnv. Each opt-in flag is wired inline next to its declaration, making the env contract local and reviewable. The uniform rule is DEVSY_ + uppercase(flagName) with "-" replaced by "_", applied via flags.EnvName / flags.BindEnv. BindEnv panics on an unknown flag (programmer bug) and log.Fatalf on rejected env values (operator bug), so misconfigured env vars never silently no-op. Renames --devsy-home to --home for a clean DEVSY_HOME mapping; the SSH ProxyCommand and user docs are updated to match. BuildRoot now returns the GlobalFlags struct alongside the root command, removing the package-level mutable var globalFlags singleton.
✅ Deploy Preview for devsydev canceled.
|
📝 WalkthroughWalkthroughThis PR implements environment variable binding for CLI flags across Devsy commands. It introduces standardized utilities for env var naming and wiring ( ChangesEnvironment Variable Binding for CLI Flags
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add 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 |
skevetter
marked this pull request as ready for review
May 24, 2026 14:35
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd/root.go`:
- Around line 86-99: Capture the prior value of DEVSY_HOME before setting it:
when you check globalFlags.DevsyHome and call os.Setenv(config.EnvHome,
globalFlags.DevsyHome), store the previous value (e.g., prevDevsyHome, and a
flag prevDevsyHomeExists) in a variable accessible to the PersistentPostRunE
closure; then in rootCmd.PersistentPostRunE restore the environment to that
previous state — if prevDevsyHomeExists set it back with
os.Setenv(config.EnvHome, prevDevsyHome), otherwise unset it with
os.Unsetenv(config.EnvHome) — so the code paths around globalFlags.DevsyHome,
os.Setenv, and rootCmd.PersistentPostRunE preserve the caller's original
environment.
🪄 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: 81fcdbcf-5f1b-44f6-b3a4-59e53433f5bf
📒 Files selected for processing (33)
cmd/agent/container/setup.gocmd/env_flags_test.gocmd/flag_aliases_test.gocmd/flags/env.gocmd/flags/env_test.gocmd/flags/flags.gocmd/pro/add/cluster.gocmd/pro/check_health.gocmd/pro/check_update.gocmd/pro/create_workspace.gocmd/pro/daemon/netcheck.gocmd/pro/daemon/start.gocmd/pro/daemon/status.gocmd/pro/flags/flags.gocmd/pro/list_clusters.gocmd/pro/list_projects.gocmd/pro/list_templates.gocmd/pro/list_workspaces.gocmd/pro/provider/rebuild.gocmd/pro/rebuild.gocmd/pro/self.gocmd/pro/sleep.gocmd/pro/start.gocmd/pro/update_provider.gocmd/pro/update_workspace.gocmd/pro/version.gocmd/pro/wakeup.gocmd/pro/watch_workspaces.gocmd/root.gocmd/runusercommands_test.godocs/pages/developing-in-workspaces/create-a-workspace.mdxpkg/ssh/config.gopkg/ssh/config_test.go
Comment on lines
+86
to
+99
| if globalFlags.DevsyHome != "" { | ||
| _ = os.Setenv(config.EnvHome, globalFlags.DevsyHome) | ||
| } | ||
|
|
||
| devsyConfig, err := config.LoadConfig(globalFlags.Context, globalFlags.Provider) | ||
| if err == nil { | ||
| telemetry.StartCLI(devsyConfig, cobraCmd) | ||
| } | ||
| return nil | ||
| } | ||
| rootCmd.PersistentPostRunE = func(_ *cobra.Command, _ []string) error { | ||
| if globalFlags.DevsyHome != "" { | ||
| _ = os.Unsetenv(config.EnvHome) | ||
| } |
There was a problem hiding this comment.
Restore prior DEVSY_HOME instead of always unsetting it
Line 98 always clears DEVSY_HOME when --home was used. If the process already had DEVSY_HOME set, this loses caller state and can leak across same-process command executions/tests. Capture the previous value before Line 87 and restore it in post-run.
Proposed fix
func BuildRoot() (*cobra.Command, *flags.GlobalFlags) {
rootCmd := &cobra.Command{
Use: config.BinaryName,
Short: "Devsy",
SilenceUsage: true,
SilenceErrors: true,
}
+ var prevHome string
+ var hadPrevHome bool
persistentFlags := rootCmd.PersistentFlags()
globalFlags := flags.SetGlobalFlags(persistentFlags)
_ = completion.RegisterFlagCompletionFuns(rootCmd, globalFlags)
rootCmd.PersistentPreRunE = func(cobraCmd *cobra.Command, _ []string) error {
log.Init(log.Config{
Verbosity: globalFlags.Verbosity,
Quiet: globalFlags.Quiet,
Debug: globalFlags.Debug,
Format: globalFlags.LogOutput,
})
klog.SetLogger(logr.New(log.LogrSink()))
if globalFlags.DevsyHome != "" {
+ prevHome, hadPrevHome = os.LookupEnv(config.EnvHome)
_ = os.Setenv(config.EnvHome, globalFlags.DevsyHome)
}
devsyConfig, err := config.LoadConfig(globalFlags.Context, globalFlags.Provider)
if err == nil {
telemetry.StartCLI(devsyConfig, cobraCmd)
}
return nil
}
rootCmd.PersistentPostRunE = func(_ *cobra.Command, _ []string) error {
if globalFlags.DevsyHome != "" {
- _ = os.Unsetenv(config.EnvHome)
+ if hadPrevHome {
+ _ = os.Setenv(config.EnvHome, prevHome)
+ } else {
+ _ = os.Unsetenv(config.EnvHome)
+ }
}
return nil
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if globalFlags.DevsyHome != "" { | |
| _ = os.Setenv(config.EnvHome, globalFlags.DevsyHome) | |
| } | |
| devsyConfig, err := config.LoadConfig(globalFlags.Context, globalFlags.Provider) | |
| if err == nil { | |
| telemetry.StartCLI(devsyConfig, cobraCmd) | |
| } | |
| return nil | |
| } | |
| rootCmd.PersistentPostRunE = func(_ *cobra.Command, _ []string) error { | |
| if globalFlags.DevsyHome != "" { | |
| _ = os.Unsetenv(config.EnvHome) | |
| } | |
| if globalFlags.DevsyHome != "" { | |
| prevHome, hadPrevHome = os.LookupEnv(config.EnvHome) | |
| _ = os.Setenv(config.EnvHome, globalFlags.DevsyHome) | |
| } | |
| devsyConfig, err := config.LoadConfig(globalFlags.Context, globalFlags.Provider) | |
| if err == nil { | |
| telemetry.StartCLI(devsyConfig, cobraCmd) | |
| } | |
| return nil | |
| } | |
| rootCmd.PersistentPostRunE = func(_ *cobra.Command, _ []string) error { | |
| if globalFlags.DevsyHome != "" { | |
| if hadPrevHome { | |
| _ = os.Setenv(config.EnvHome, prevHome) | |
| } else { | |
| _ = os.Unsetenv(config.EnvHome) | |
| } | |
| } | |
| return nil | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cmd/root.go` around lines 86 - 99, Capture the prior value of DEVSY_HOME
before setting it: when you check globalFlags.DevsyHome and call
os.Setenv(config.EnvHome, globalFlags.DevsyHome), store the previous value
(e.g., prevDevsyHome, and a flag prevDevsyHomeExists) in a variable accessible
to the PersistentPostRunE closure; then in rootCmd.PersistentPostRunE restore
the environment to that previous state — if prevDevsyHomeExists set it back with
os.Setenv(config.EnvHome, prevDevsyHome), otherwise unset it with
os.Unsetenv(config.EnvHome) — so the code paths around globalFlags.DevsyHome,
os.Setenv, and rootCmd.PersistentPostRunE preserve the caller's original
environment.
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
inheritFlagsFromEnvironment(which auto-bound every CLI flag toDEVSY_<FLAG>) with an explicit allowlist viaflags.BindEnv, wired inline next to each opt-in flag declaration. The env contract is now local to each call site and reviewable.env = "DEVSY_" + uppercase(flagName)with-→_. Implemented inflags.EnvNameand pinned againstpkg/configconstants byTestEnvName_MatchesConfigConstants.BindEnvis loud on failure:panicon unknown flag (programmer bug at init),log.Fatalfon env values rejected by the flag's type (operator bug, e.g.DEVSY_DEBUG=garbage). Matches the priorlog.Fatalfcontract.--devsy-home→--homefor a cleanDEVSY_HOMEmapping. Hard cutover; SSHProxyCommandemission (pkg/ssh/config.go) and the user docs are updated to match.BuildRootnow returns(*cobra.Command, *flags.GlobalFlags)and the package-levelvar globalFlagssingleton is gone — enables parallel tests, removes shared mutable state.Opt-in flags (env support)
--homeDEVSY_HOME--contextDEVSY_CONTEXT--providerDEVSY_PROVIDER--debugDEVSY_DEBUG--hostDEVSY_HOSTdevsy pro …subcommand--projectDEVSY_PROJECTdevsy pro …subcommand that has it--access-keyDEVSY_ACCESS_KEYdevsy agent container setup--platform-hostDEVSY_PLATFORM_HOSTdevsy agent container setupNotable behavior changes
DEVSY_DEBUGsemantics widened from "must equaltrue" to "any pflag-parseable bool" (1,t,TRUE, …) since it now flows throughpflag.Set.DEVSY_*. Previously every flag did, silently.Test coverage
cmd/env_flags_test.gocovers: env→flag for all 8 opt-in flags; empty-env no-op; CLI overrides env viaExecute(); env satisfiesMarkFlagRequiredviaExecute(); sweep across every pro subcommand asserting--host/--projectbindings stuck (this caught a missing binding onpro startduring development).cmd/flags/env_test.gopinsEnvName(flagName)againstpkg/configconstants so the naming rule and the canonical env-var registry can't drift.Summary by CodeRabbit
Release Notes
New Features
DEVSY_HOST,DEVSY_PROJECT,DEVSY_HOME). CLI arguments take precedence over environment variables.Documentation
--homeflag replaces--devsy-homefor configuring the Devsy home directory.Tests