Merged
Conversation
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the environment variable directive parsing to prevent nested environment objects and simplify the deserialization logic. It replaces complex custom deserialization with serde's untagged enum approach.
- Simplifies the
Valenum by separating primitive values from configuration maps - Adds
Deserializetrait toEnvDirectiveOptionswith default field attributes - Removes extensive custom deserialization code in favor of serde's built-in mechanisms
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/config/env_directive/mod.rs | Adds Deserialize trait and #[serde(default)] attributes to EnvDirectiveOptions |
| src/config/config_file/mise_toml.rs | Refactors env value parsing by introducing PrimitiveVal enum and simplifying Val enum structure |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Comment on lines
+933
to
+942
| #[derive(Deserialize)] | ||
| #[serde(untagged)] | ||
| pub enum PrimitiveVal { | ||
| Str(String), | ||
| Int(i64), | ||
| Bool(bool), | ||
| } | ||
| #[derive(Deserialize)] | ||
| #[serde(untagged)] | ||
| enum Val { |
There was a problem hiding this comment.
These enum definitions are declared inside a match arm, creating unnecessary nesting and reducing readability. Consider moving PrimitiveVal and Val to module level to improve code organization.
| value: Box<Val>, | ||
| tools: bool, | ||
| redact: bool, | ||
| value: PrimitiveVal, |
There was a problem hiding this comment.
The Map variant only accepts PrimitiveVal for the value field, which prevents nested objects. However, this change should be validated to ensure it doesn't break existing configurations that might rely on nested env objects.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| } | ||
| } | ||
| PrimitiveVal::Bool(false) => EnvDirective::Rm(key, options), | ||
| }; |
There was a problem hiding this comment.
Bug: Refactoring Breaks Compatibility and Hides Errors
This refactoring changes how map-like environment variables are deserialized. { value = false, ... } now creates an Rm directive instead of a Val("false"), which breaks backward compatibility. Additionally, unknown fields in these map directives are now silently ignored, potentially hiding configuration typos.
Contributor
Author
There was a problem hiding this comment.
Oh right, should we preserve this behaviour for backward compatibility?
key="false" can be used to set the env var false, but someone might be using key={value=false} for this purpose.
Owner
There was a problem hiding this comment.
no, = false should always have been removing an env var so that was a bug
jdx
added a commit
that referenced
this pull request
Sep 11, 2025
### 🐛 Bug Fixes - **(backend)** make HTTP installs atomic and serialize with cache lock by @jdx in [#6259](#6259) - **(env)** allow nested env._.path directives by @risu729 in [#6160](#6160) - **(env)** disallow nested env objects by @risu729 in [#6268](#6268) - **(schema)** allow nested arrays in task.depends by @risu729 in [#6265](#6265) - **(task)** resolve jobs=1 hang and keep-order panic; improve Ctrl-C handling by @jdx in [#6264](#6264) - **(tasks)** stop CLI group after first failure without --continue-on-error by @jdx in [#6270](#6270) ### 📚 Documentation - fixed toml issues in URL replacements settings documentation by @ThomasSteinbach in [#6269](#6269) ### Chore - **(schema)** strict oneOf branches and DRY env_directive in schemas by @jdx in [#6271](#6271) - add schema linter by @risu729 in [#6267](#6267) Co-authored-by: mise-en-dev <release@mise.jdx.dev>
netbsd-srcmastr
pushed a commit
to NetBSD/pkgsrc
that referenced
this pull request
Sep 19, 2025
## [2025.9.12](https://github.com/jdx/mise/compare/v2025.9.11..v2025.9.12) - 2025-09-16 ### 🐛 Bug Fixes - **(ci)** properly exclude aqua-registry files from hk linting by @jdx in [42d7758](jdx/mise@42d7758) ### Chore - **(release)** embed aqua-registry under crate and publish like vfox by @jdx in [#6306](jdx/mise#6306) - ignore aqua-registry assets from prettier by @jdx in [047d77b](jdx/mise@047d77b) - disable "useless cat" shellcheck by @jdx in [a6def59](jdx/mise@a6def59) ## [2025.9.11](https://github.com/jdx/mise/compare/v2025.9.10..v2025.9.11) - 2025-09-16 ### 📦 Registry - indicate aws-cli is v2 by @jayvdb in [#6300](jdx/mise#6300) ### 🚀 Features - **(ci)** run all registry tools when workflow_dispatch is triggered by @jdx in [#6295](jdx/mise#6295) - **(cli)** handle non-existent working directories gracefully by @jdx in [#6304](jdx/mise#6304) - **(set)** add -E/--env flag to mise set command by @jdx in [#6291](jdx/mise#6291) - **(tasks)** make auto outputs default by @risu729 in [#6137](jdx/mise#6137) ### 🐛 Bug Fixes - align crate versions to fix release-plz script by @jdx in [5a464e9](jdx/mise@5a464e9) ### 🚜 Refactor - **(aqua)** extract aqua registry into internal subcrate by @jdx in [#6293](jdx/mise#6293) ### 📚 Documentation - fix mkdir paths by @risu729 in [#6298](jdx/mise#6298) - fix rust profile default by @risu729 in [#6305](jdx/mise#6305) ### Chore - **(aqua-registry)** remove unused aqua-registry files by @jdx in [#6294](jdx/mise#6294) - **(release)** make release-plz idempotent for existing crate versions by @jdx in [dbdfd6a](jdx/mise@dbdfd6a) - **(release)** skip publishing mise when aqua-registry is a path dep by @jdx in [47efffd](jdx/mise@47efffd) - keep aqua-registry LICENSE file by @risu729 in [#6297](jdx/mise#6297) ### New Contributors - @jayvdb made their first contribution in [#6300](jdx/mise#6300) ## [2025.9.10](https://github.com/jdx/mise/compare/v2025.9.9..v2025.9.10) - 2025-09-13 ### 📦 Registry - use asdf to install semver-tool by @jylenhof in [#6233](jdx/mise#6233) ### 🐛 Bug Fixes - **(tool-stub)** detect binary names from single-file downloads by @jdx in [#6281](jdx/mise#6281) ### 🚜 Refactor - allow any collection types in deserialize_arr by @risu729 in [#6282](jdx/mise#6282) - use deserialize_arr for task runs by @risu729 in [#6253](jdx/mise#6253) ### 📚 Documentation - **(getting-started)** add backends step with diagram, github example, env vars and simple tasks by @jdx in [#6288](jdx/mise#6288) - simplify OS detection in tool plugin development by @jdx in [#6287](jdx/mise#6287) - update backend plugin template references by @jdx in [942f5eb](jdx/mise@942f5eb) ### 📦️ Dependency Updates - update rust crate chrono to v0.4.42 by @renovate[bot] in [#6274](jdx/mise#6274) - update taiki-e/install-action digest to 0154864 by @renovate[bot] in [#6273](jdx/mise#6273) ### Chore - **(schema)** fix schema for subtasks by @risu729 in [#6289](jdx/mise#6289) - update render:schema task by @risu729 in [#6275](jdx/mise#6275) ## [2025.9.9](https://github.com/jdx/mise/compare/v2025.9.8..v2025.9.9) - 2025-09-11 ### 🐛 Bug Fixes - **(backend)** make HTTP installs atomic and serialize with cache lock by @jdx in [#6259](jdx/mise#6259) - **(env)** allow nested env._.path directives by @risu729 in [#6160](jdx/mise#6160) - **(env)** disallow nested env objects by @risu729 in [#6268](jdx/mise#6268) - **(schema)** allow nested arrays in task.depends by @risu729 in [#6265](jdx/mise#6265) - **(task)** resolve jobs=1 hang and keep-order panic; improve Ctrl-C handling by @jdx in [#6264](jdx/mise#6264) - **(tasks)** stop CLI group after first failure without --continue-on-error by @jdx in [#6270](jdx/mise#6270) ### 📚 Documentation - fixed toml issues in URL replacements settings documentation by @ThomasSteinbach in [#6269](jdx/mise#6269) ### Chore - **(schema)** strict oneOf branches and DRY env_directive in schemas by @jdx in [#6271](jdx/mise#6271) - add schema linter by @risu729 in [#6267](jdx/mise#6267) ## [2025.9.8](https://github.com/jdx/mise/compare/v2025.9.7..v2025.9.8) - 2025-09-10 ### 🐛 Bug Fixes - **(tasks)** prevent hang when task fails in sequence by @jdx in [#6260](jdx/mise#6260) - **(version)** fetch mise version if cached version is older than the current by @risu729 in [#6252](jdx/mise#6252) ### 📦️ Dependency Updates - update rhysd/action-setup-vim action to v1.4.3 by @renovate[bot] in [#6249](jdx/mise#6249) ## [2025.9.7](https://github.com/jdx/mise/compare/v2025.9.6..v2025.9.7) - 2025-09-09 ### 🐛 Bug Fixes - **(env)** allow mixed map for env._.file by @risu729 in [#6148](jdx/mise#6148) - **(tasks)** restore parallel starts with poetry via list_paths cache and stable exec-env cache by @jdx in [#6237](jdx/mise#6237) - add 'unknown' to the list of OS patterns by @efussi in [#6235](jdx/mise#6235) - propagate errors from backend installs by @jdx in [#6236](jdx/mise#6236) ### 📦️ Dependency Updates - update taiki-e/install-action digest to 0c5db7f by @renovate[bot] in [#6244](jdx/mise#6244) - update golang docker tag to v1.25.1 by @renovate[bot] in [#6247](jdx/mise#6247) - update dependency vitepress to v1.6.4 by @renovate[bot] in [#6246](jdx/mise#6246) ### New Contributors - @efussi made their first contribution in [#6235](jdx/mise#6235)
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
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.
This just simplifies the deserializer, but the following config will be correctly rejected.
This had been parsed as
test=hello redact, so no one should be using this.