Add --env/--env-file flags to validate and sidecar setup#323
Add --env/--env-file flags to validate and sidecar setup#323schurchleycci wants to merge 4 commits into
Conversation
2dda51b to
677b288
Compare
michael-webster
left a comment
There was a problem hiding this comment.
Review Summary
Files reviewed: 7
Issues identified: 4
1. Silent no-op: --env flags ignored on local runs without user feedback
internal/cmd/validate.go:149 | High
When no sidecar is resolved, resolveEnvVars is skipped entirely — env vars are validated for syntax but never forwarded. A user who passes --env FOO=bar on a purely local chunk validate run gets no error and no warning; their flag silently has no effect. Consider emitting a warning via streams.ErrPrintf when len(envVarsFlag) > 0 or envFile != "" and sidecarID == "".
2. nil secrets delegate passed to secrets.ResolveAll without comment
internal/cmd/env.go:32 | High
secrets.ResolveAll(ctx, envVars, nil) passes nil as the resolver delegate. If ResolveAll panics or errors on a nil delegate, failures will surface only when env vars are actually present — non-deterministic in tests. The old sidecar.go inline code also passed nil, but the refactor into a shared helper amplifies the blast radius across all three commands. Is nil a documented/supported code path in secrets.ResolveAll?
3. sidecarSetupRunSetup: dir may be "" when resolving relative --env-file paths
internal/cmd/sidecar.go:776 | High
resolveEnvVars(cmd.Context(), dir, envFile, envVarsFlag) passes dir (the --dir flag), which is "" when not supplied. sidecar ssh explicitly calls os.Getwd() first; sidecar setup does not. With dir == "", filepath.Join("", ".env.local") resolves relative to process cwd — which may work by accident today but is inconsistent and fragile.
Suggested fix: resolve dir to os.Getwd() when empty, mirroring how sidecar ssh handles it.
4. SSH fake env payload parsing: missing bounds check between name and value length fields
internal/testing/fakes/ssh.go:177 | Medium
The manual SSH wire-format decoder checks int(nameLen)+8 > len(req.Payload) before reading the value length field. On a 32-bit platform, a uint32 nameLen value > math.MaxInt32 could silently overflow int, bypassing this check and causing a panic. Additionally, TestOpenSSHSessionNoEnvVars is listed in the PR test plan but is not present in validate_test.go — the no-env-vars path through openSSHSession appears untested.
| s.t.Errorf("ssh fake: env payload too short (%d bytes)", len(req.Payload)) | ||
| continue | ||
| } | ||
| nameLen := binary.BigEndian.Uint32(req.Payload[:4]) |
There was a problem hiding this comment.
What are we checking here exactly?
michael-webster
left a comment
There was a problem hiding this comment.
So the code here is fine, but I'm not really understanding the intent.
Remember, we want the user flow to be "chunk validate" and stuff just works. we do the right thing to load environment settings, create a sidecar if necessary and so on. The env overrides can be useful to debug I guess, but in terms of e.g. the hook configuration there probably won't be individual env flags.
There's a specific problem I ran into running
So in order to get a sidecar where Thinking through this again, I think there's two separate gaps:
All that to say, maybe we should just pare this down to adding env forwarding to setup and leaving validate as is? |
Forwards environment variables (API keys, registry tokens, etc.) to sidecar SSH sessions. --env-file loads a file (defaults to .env.local when the flag is present without a value); repeated -e/--env flags set individual KEY=VALUE pairs; flag values win over file values. A shared resolveEnvVars helper in env.go keeps the logic in one place across validate, sidecar setup, and sidecar ssh. Env loading is gated behind sidecarID != "" so local runs never touch .env.local or hit the secrets API. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Remove unnecessary defensive bounds checks from SSH fake (payload comes from our own SSH client and is always well-formed), and drop the TestValidateEnvLocalRunIgnoresEnvFile test per reviewer feedback. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
c3956ab to
f2d8690
Compare
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Summary
Implementation notes
Test plan
🤖 Generated with Claude Code