chore(sessions): Add Primites for Paths and Patches - #248
Conversation
📝 WalkthroughWalkthroughIntroduces a realm-tagged path type system with validated ChangesRealm-tagged paths, variables, and patch system
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
Comment |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
crates/sessions/src/loadout.rs (1)
12-12: ⚡ Quick winRemove the commented-out serde attribute.
Line 12 is dead commented-out code now that the real field attribute already lives on
vars.As per coding guidelines, "Do not include dead code, commented-out code, or ownerless TODOs."
🤖 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 `@crates/sessions/src/loadout.rs` at line 12, Remove the dead commented-out serde attribute on the field by deleting the line "// #[serde(default, skip_serializing_if = "BTreeMap::is_empty")]" so only the actual attribute on vars remains; locate the commented line near the vars field in loadout.rs and remove it to comply with the coding guideline against commented-out code.
🤖 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 `@crates/sessions/src/loadout.rs`:
- Around line 8-9: The Loadout struct's vars field currently uses
#[serde(default, skip_serializing_if = "BTreeMap::is_empty")] but does not
accept legacy "env_vars", so legacy configs are dropped; update the vars field
on the Loadout struct to include a serde alias/rename (e.g., add alias =
"env_vars" or rename = "env_vars" in the attribute) so deserialization accepts
both names, and also remove the commented-out serde attribute near the
lifecycle_hooks field to eliminate dead/commented-out code; reference the vars
field and lifecycle_hooks in crates/sessions/src/loadout.rs when making these
edits.
In `@crates/sessions/src/patches.rs`:
- Around line 371-381: PatchDecl currently allows an empty FileSet which lets
configs like `source = []` silently become no-ops; update PatchDecl::new to
reject FileSet::empty() by validating the `source` parameter (e.g., if
source.is_empty() { panic!("PatchDecl source may not be empty"); } or change the
constructor to return Result and Err with a clear message). Modify the `pub fn
new(source: FileSet, dest: PatchDest)` implementation in the PatchDecl impl to
perform this check and emit a clear error, referencing PatchDecl::new,
PatchDecl, and FileSet::is_empty()/FileSet::empty() so callers/deserializers get
a consistent failure.
In `@crates/sessions/src/paths.rs`:
- Around line 333-335: Update stale intra-doc links in the comments to point to
the actual constructor names: replace references to RelPath::new and
AbsPath::new with RelPath::try_new and AbsPath::try_new respectively in
crates/sessions/src/paths.rs (search for the comment that currently mentions
`RelPath::new` and the places mentioning `AbsPath::new`/`RelPath::new` around
the module docs), ensuring all markdown-style rustdoc links use the `try_new`
names so rustdoc resolves correctly.
In `@crates/sessions/src/vars.rs`:
- Around line 1-5: The enum Var is currently using Serde's default
externally-tagged representation so a bare TOML string won't deserialize into
Specified { value: String }; mark the enum as untagged and make Specified a
newtype so plain strings map to it: add #[serde(untagged)] to Var, change
Specified { value: String } to Specified(String), and update any callsites that
accessed Specified.value to use the new inner String (e.g., match
Var::Specified(s) or unwrap the inner string); leave Inherit and
InheritWithDefault { default: String } as-is.
---
Nitpick comments:
In `@crates/sessions/src/loadout.rs`:
- Line 12: Remove the dead commented-out serde attribute on the field by
deleting the line "// #[serde(default, skip_serializing_if =
"BTreeMap::is_empty")]" so only the actual attribute on vars remains; locate the
commented line near the vars field in loadout.rs and remove it to comply with
the coding guideline against commented-out code.
🪄 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: f8a74723-f259-4d4b-a697-3637145e9890
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
Cargo.tomlcrates/sessions/Cargo.tomlcrates/sessions/src/lib.rscrates/sessions/src/lifecyclehook.rscrates/sessions/src/loadout.rscrates/sessions/src/patches.rscrates/sessions/src/paths.rscrates/sessions/src/vars.rs
| #[serde(default, skip_serializing_if = "BTreeMap::is_empty")] | ||
| vars: BTreeMap<String, Var>, |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Serde's default struct deserialization, are unknown fields ignored unless deny_unknown_fields is set?
💡 Result:
Yes. In Serde’s default struct deserialization behavior (for self-describing formats like JSON), unknown fields are ignored unless you opt into stricter checking with #[serde(deny_unknown_fields)]. Serdes’s container attribute docs state that #[serde(deny_unknown_fields)] “Always error during deserialization when encountering unknown fields” and that “When this attribute is not present, by default unknown fields are ignored for self-describing formats like JSON.” [1][2]
Citations:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "struct Loadout|env_vars|vars:|skip_serializing_if|serde.*default|deny_unknown_fields" crates/sessions/src/loadout.rs
nl -ba crates/sessions/src/loadout.rs | sed -n '1,80p'Repository: gominimal/minimal
Length of output: 408
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,120p' crates/sessions/src/loadout.rs
sed -n '120,240p' crates/sessions/src/loadout.rsRepository: gominimal/minimal
Length of output: 513
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "deny_unknown_fields|rename\s*=|alias\s*=|env_vars|vars:" crates/sessions/src/loadout.rsRepository: gominimal/minimal
Length of output: 120
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "env_vars" -S .
rg -n "Loadout" crates/sessions/src -S
rg -n "serde\(([^)]*rename|[^)]*alias|[^)]*deny_unknown_fields)" -S crates/sessions/srcRepository: gominimal/minimal
Length of output: 6582
Prevent legacy env_vars from being ignored during Serde deserialization
Loadout’svarsfield (#[serde(default, skip_serializing_if = "BTreeMap::is_empty")]) has noalias = "env_vars"/rename and there’s nodeny_unknown_fields, so legacy configs providingenv_varswill be treated as unknown and effectively dropped (deserializingvarsas the default empty map).- Remove the commented-out
#[serde(...)]attribute incrates/sessions/src/loadout.rs(near the lifecycle_hooks field) to avoid dead/commented-out code.
🤖 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 `@crates/sessions/src/loadout.rs` around lines 8 - 9, The Loadout struct's vars
field currently uses #[serde(default, skip_serializing_if =
"BTreeMap::is_empty")] but does not accept legacy "env_vars", so legacy configs
are dropped; update the vars field on the Loadout struct to include a serde
alias/rename (e.g., add alias = "env_vars" or rename = "env_vars" in the
attribute) so deserialization accepts both names, and also remove the
commented-out serde attribute near the lifecycle_hooks field to eliminate
dead/commented-out code; reference the vars field and lifecycle_hooks in
crates/sessions/src/loadout.rs when making these edits.
| #[derive(Clone, Debug, PartialEq, Eq, Hash, serde::Serialize, serde::Deserialize)] | ||
| pub struct PatchDecl { | ||
| source: FileSet, | ||
| dest: PatchDest, | ||
| } | ||
|
|
||
| impl PatchDecl { | ||
| /// Construct a declaration without origin. | ||
| #[must_use] | ||
| pub fn new(source: FileSet, dest: PatchDest) -> Self { | ||
| Self { source, dest } |
There was a problem hiding this comment.
Reject empty patch sources.
FileSet::empty() makes sense for policy defaults, but PatchDecl currently accepts the same value for source. That allows patches = [{ dest = "/x", source = [] }] to load as a silent no-op instead of surfacing a config error.
🤖 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 `@crates/sessions/src/patches.rs` around lines 371 - 381, PatchDecl currently
allows an empty FileSet which lets configs like `source = []` silently become
no-ops; update PatchDecl::new to reject FileSet::empty() by validating the
`source` parameter (e.g., if source.is_empty() { panic!("PatchDecl source may
not be empty"); } or change the constructor to return Result and Err with a
clear message). Modify the `pub fn new(source: FileSet, dest: PatchDest)`
implementation in the PatchDecl impl to perform this check and emit a clear
error, referencing PatchDecl::new, PatchDecl, and
FileSet::is_empty()/FileSet::empty() so callers/deserializers get a consistent
failure.
| /// Invariant: `inner.is_absolute()` is always false. Construction goes | ||
| /// through [`RelPath::new`], which validates the input. | ||
| #[must_use] |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n 'RelPath::new|AbsPath::new|fn\s+try_new\b|impl<.*>\s+(AbsPath|RelPath)' crates/sessions/src/paths.rsRepository: gominimal/minimal
Length of output: 487
Fix stale intra-doc links for AbsPath::new / RelPath::new. crates/sessions/src/paths.rs defines AbsPath::try_new and RelPath::try_new, but the docs still reference [RelPath::new] (line 334) and [AbsPath::new] / [RelPath::new] (around line 599-600), which leaves invalid rustdoc links. Update those to the try_new constructors.
🤖 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 `@crates/sessions/src/paths.rs` around lines 333 - 335, Update stale intra-doc
links in the comments to point to the actual constructor names: replace
references to RelPath::new and AbsPath::new with RelPath::try_new and
AbsPath::try_new respectively in crates/sessions/src/paths.rs (search for the
comment that currently mentions `RelPath::new` and the places mentioning
`AbsPath::new`/`RelPath::new` around the module docs), ensuring all
markdown-style rustdoc links use the `try_new` names so rustdoc resolves
correctly.
| #[derive(Clone, Debug, serde::Serialize, serde::Deserialize)] | ||
| pub enum Var { | ||
| Inherit, | ||
| InheritWithDefault { default: String }, | ||
| Specified { value: String }, |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Serde's default derived enum representation for TOML, how does an enum with one unit variant and struct variants serialize/deserialize, and would a bare string like "bar" deserialize into a variant shaped like Specified { value: String }?
💡 Result:
In Serde's default (externally tagged) enum representation, an enum is serialized as a single-key map (or table in TOML) where the key is the variant name and the value is the variant's content [1]. For an enum with a unit variant and a struct variant, serialization and deserialization work as follows: 1. Unit Variant: Serialized as a simple string representing the variant name [2][3]. For example, MyEnum::Unit becomes my_enum = "Unit". 2. Struct Variant: Serialized as a map (table) where the variant name is the key, and the value is another table containing the struct fields [4][2]. For example, MyEnum::Specified { value: String } becomes my_enum = { Specified = { value = "bar" } }. Regarding your second question, a bare string like "bar" will not automatically deserialize into a variant shaped like Specified { value: String } under the default externally tagged representation [4][2]. The deserializer expects the structure to match the externally tagged format (i.e., a map containing the variant name) [1]. A bare string "bar" is expected to match a unit variant named "bar" [2][3]. To achieve deserialization of a bare string into a struct variant, you would need to use alternative Serde attributes such as #[serde(untagged)] on the enum (which changes the entire representation to be content-based rather than tag-based) [1][5] or use a custom deserialization function with #[serde(deserialize_with = "...")] to handle the conversion from a string to that specific struct variant [6]. Note that using #[serde(untagged)] can introduce ambiguity if multiple variants could potentially match the input [7].
Citations:
- 1: https://serde.rs/enum-representations.html
- 2: externally tagged enums toml-rs/toml-rs#225
- 3: https://users.rust-lang.org/t/enum-with-tuple-as-value-serializing-with-serde/68273
- 4: https://stackoverflow.com/questions/57560593/why-do-i-get-an-unsupportedtype-error-when-serializing-to-toml-with-a-manually-i
- 5: https://serde.rs/container-attrs
- 6: https://serde.rs/string-or-struct.html
- 7: How to deserialize a string to an enum that's either a unit variant or a tuple "other" variant ? serde-rs/json#1044
Add an explicit Serde representation for Var.
Var’s bare #[derive(serde::Serialize, serde::Deserialize)] uses Serde’s default externally tagged enum representation, where struct variants like Specified { value: String } are not deserialized from a bare TOML string. In TOML, Specified { value: "bar" } is expected to come in as a table/map shape (e.g., Specified = { value = "bar" }), while a plain string like "bar" won’t map to Specified { value: String }—it only fits unit-variant inputs.
🤖 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 `@crates/sessions/src/vars.rs` around lines 1 - 5, The enum Var is currently
using Serde's default externally-tagged representation so a bare TOML string
won't deserialize into Specified { value: String }; mark the enum as untagged
and make Specified a newtype so plain strings map to it: add #[serde(untagged)]
to Var, change Specified { value: String } to Specified(String), and update any
callsites that accessed Specified.value to use the new inner String (e.g., match
Var::Specified(s) or unwrap the inner string); leave Inherit and
InheritWithDefault { default: String } as-is.
More details can be found in the modules respective doc strings, but at a high level:
Part 1: Paths
If you're just using
PathBufeverywhere for paths, you run into a few problems:-
PathBufcan represent paths with non-utf8 characters. We currently only support utf8 paths so converting between paths and rust strings is a pain-
PathBufcontains no context as to what filesystem a path is for, so there is potential for bugs if we mix them up-
PathBufyou can do weird things like joining an absolute path onto the end of another path which will silently succeed even though it is a logical errorThe solution to this was:
- Adopt
camino'sUtf8PathBufas our core path primitive instead ofstd'sPathBuf- Create a concept of
Realmfor paths which are encoded into wrapper types using Phantom markers using Rust's type system to prevent paths from different realms from mixing without explicit translation- Separate type wrappers for absolute and relative paths so we can use Rust's type system to prevent logically illegal operations (as well as an enum for cases when a path can be either without loosing type safety)
Part 2: Patches
First pass at a data model for file system patches. This includes the
FileSetprimitive which is re-used both in patches themselves and patch policies (allow, deny, ignore lists). Patch primitive for specifying files to be patched, as well as "promoted" forms with provenance as to where the patch came from for logs, error messages, etc.Summary by CodeRabbit
Release Notes
New Features
Chores
caminoandglobsetdependencies