Uh oh!
There was an error while loading. Please reload this page.
fix(shell): skip env vars with invalid names instead of breaking the shell - #2883
Conversation
…shell A "//" comment key in a devbox.json env block was exported verbatim as `export //=...`, which zsh rejects with a cryptic "not valid in this context: //" and breaks the entire shell. exportify/exportifyNushell now skip names that aren't valid shell identifiers and emit a single clear warning naming them. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This pull request hardens Devbox’s shell environment export generation by filtering out invalid environment variable names (e.g., JSON “comment keys” like "//") so devbox shell / devbox shellenv no longer emits syntactically invalid shell code and breaks shells like zsh.
Changes:
- Add env var name validation (
isValidEnvName) and a consolidated warning for skipped invalid keys. - Update
exportifyandexportifyNushellto skip exporting invalid identifiers instead of generating invalid shell syntax. - Add unit tests covering env name validation and the “skip invalid names” behavior for both export formats.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| internal/devbox/envvars.go | Adds env var name validation, warning emission, and skips invalid env names in both POSIX and nushell export generators. |
| internal/devbox/envvars_test.go | Adds unit tests for env name validation and verifying invalid names are not emitted in export output. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Address review feedback on invalid env name handling: - exportify/exportifyNushell/warnInvalidEnvNames now take an io.Writer so warnings respect --quiet and caller-provided stderr instead of always writing to os.Stderr - anchor the validation regex in the warning text (^...$) to show the entire name must conform - assert //ccache is skipped in TestExportifySkipsInvalidNames Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Uh oh!
There was an error while loading. Please reload this page.
Summary
A
"//"comment key in adevbox.jsonenvblock was exported verbatim asexport //=..., which zsh rejects with the crypticnot valid in this context: //, breaking the entiredevbox shell.exportifyandexportifyNushellnow skip env vars whose names aren't valid shell identifiers (^[a-zA-Z_][a-zA-Z0-9_]*$) and emit a single clear warning naming the offending keys, instead of generating invalid shell.How was it tested?
Added unit tests for
isValidEnvNameand for both export formats skipping invalid names; the fullinternal/devboxsuite,go vet, andgo build ./...pass. Reproduced the original failure (export //=x→not valid in this context: //) and confirmed the generated shellenv now passeszsh -nwith only valid vars exported and the warning shown on stderr.Community Contribution License
All community contributions in this pull request are licensed to the project
maintainers under the terms of the
Apache 2 License.
By creating this pull request, I represent that I have the right to license the
contributions to the project maintainers under the Apache 2 License as stated in
the
Community Contribution License.