Skip to content

refactor: idiomatic cleanups in wxc_common and the AppContainer/WSLC/LXC backends#573

Open
MGudgin wants to merge 1 commit into
mainfrom
user/gudge/idiomatic-cleanups
Open

refactor: idiomatic cleanups in wxc_common and the AppContainer/WSLC/LXC backends#573
MGudgin wants to merge 1 commit into
mainfrom
user/gudge/idiomatic-cleanups

Conversation

@MGudgin

@MGudgin MGudgin commented Jun 26, 2026

Copy link
Copy Markdown
Member

Summary

Idiomatic-Rust cleanups. No intended behavior change — only ownership,
error-signalling, and stdlib-usage improvements. This is the follow-up to the
behavior-bug PR (#570); the larger related refactors (WxcError typed source
errors, SDK/CLI PathBuf migration, mxc_pty typed error, the WinHTTP-shim
RAII rework) are intentionally deferred to their own focused PRs.

Details

  • Capability SID ownership. get_capability_sid_from_name now returns an
    owning OwnedCapabilitySid (frees on drop) instead of a raw *mut c_void
    with a "caller must LocalFree" contract. The helper and the owner type
    were relocated out of the shared wxc_common foundation into the
    appcontainer backend (now private) — capability SIDs are an AppContainer
    concept with a single caller, so this both removes the hand-rolled
    CapabilitySidGuard and keeps backend-specific logic out of the foundation
    crate.
  • string_util::sid_to_string returns Option<String> instead of taking a
    sentinel default_value. The LocalAlloc'd buffer is still read then freed
    exactly once on the success path; the four call sites map None to their
    prior fallback.
  • ErrorEnvelope.code is now the closed MxcErrorCode enum rather than a
    String. The snake_case wire format is unchanged (serde).
  • deserialize_config deserializes from the borrowed &Value instead of
    cloning the backend config subtree.
  • WSLC image enumeration is allocation-free (slice::from_raw_parts + a
    first-NUL scan with full-buffer fallback) instead of manual .offset(i)
    pointer walking and a per-entry Vec<u8>.
  • LXC validate_path drops a provably-unreachable \n/\r branch already
    covered by char::is_whitespace.

Tests

  • cargo test -p wxc_common -p appcontainer_common -p wslc_common — 126 + 45 +
    346 pass (includes the updated sid_to_string_null_returns_none and the
    ErrorEnvelope round-trip/serialisation tests).
  • cargo clippy -p wxc_common -p appcontainer_common -p wslc_common --all-targets -- -D warnings and cargo fmt ... -- --check clean.
  • cargo check -p wxc --all-targets clean (full Windows backend dispatch).
  • Not built on the Windows host: the LXC validate_path change
    (lxc_common is Linux-only). It is the removal of a provably-dead branch
    with no logic change.

Related Issues

  • (none)

Copilot AI review requested due to automatic review settings June 26, 2026 14:52
@MGudgin MGudgin requested a review from a team as a code owner June 26, 2026 14:52

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR performs idiomatic Rust refactors across wxc_common and several backends (AppContainer / WSLC / LXC), aiming for no behavioral change while improving ownership, error typing, and avoiding unnecessary allocations in hot paths.

Changes:

  • Refactors SID handling: sid_to_string now returns Option<String>, and AppContainer capability SID derivation now uses an owned RAII wrapper.
  • Tightens the state-aware error wire model by making ErrorEnvelope.code a closed MxcErrorCode enum (still serialized as snake_case strings).
  • Removes avoidable allocations/clones in config deserialization and WSLC image enumeration logic.
Show a summary per file
File Description
src/core/wxc_common/src/string_util.rs sid_to_string now returns Option<String> and avoids sentinel defaults.
src/core/wxc_common/src/state_aware_request.rs Deserializes per-phase config directly from a borrowed &Value (no subtree clone).
src/core/wxc_common/src/process_util.rs Removes capability SID derivation helper from the shared foundation crate.
src/core/wxc_common/src/mxc_error.rs Makes ErrorEnvelope.code a typed MxcErrorCode while preserving snake_case wire format.
src/core/wxc_common/src/diagnostic.rs Updates current-user SID collection to use Option<String> from sid_to_string.
src/backends/wslc/common/src/wsl_container_runner.rs Refactors WSLC image enumeration to be allocation-free when checking image names.
src/backends/lxc/common/src/filesystem_mounts.rs Simplifies validate_path by relying on char::is_whitespace.
src/backends/appcontainer/common/src/sandbox_tracking.rs Updates SID-to-string conversion to use Option<String> and map failures explicitly.
src/backends/appcontainer/common/src/base_container_runner.rs Adapts SID formatting call sites to Option<String> with fallback.
src/backends/appcontainer/common/src/appcontainer_runner.rs Introduces OwnedCapabilitySid RAII + relocates capability SID derivation into the backend.

Review details

  • Files reviewed: 10/10 changed files
  • Comments generated: 1
  • Review effort level: Low

Comment thread src/backends/wslc/common/src/wsl_container_runner.rs
MGudgin pushed a commit that referenced this pull request Jun 26, 2026
…erminator

Addresses Copilot review feedback on PR #573.

CStr::from_bytes_until_nul errors when info.name has no NUL, which would skip an image whose name exactly fills the 256-byte buffer (a behavior change vs the original take_while). Scan for the first NUL with position() and fall back to the whole buffer when none is found, staying allocation-free.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Generated-with: claude-opus-4.8
@MGudgin MGudgin force-pushed the user/gudge/idiomatic-cleanups branch from 0631c9c to 19d780c Compare June 26, 2026 16:12
…LXC backends

This PR refactors several non-idiomatic constructs into conventional Rust. There
is no intended behavior change — only ownership, error-signalling, and
stdlib-usage cleanups.

Details

* Capability SID ownership: `get_capability_sid_from_name` now returns an owning
  `OwnedCapabilitySid` (frees the SID on drop) instead of a raw `*mut c_void`
  the caller had to `LocalFree`. Both the helper and the owner type were
  relocated out of the shared `wxc_common` foundation into the `appcontainer`
  backend (private), since capability SIDs are an AppContainer concept with a
  single caller — this removes the hand-rolled `CapabilitySidGuard` and keeps
  backend-specific logic out of `wxc_common`.
* `string_util::sid_to_string` now returns `Option<String>` instead of taking a
  sentinel `default_value`; failure is modeled as `None` and the unsafe Win32
  calls stay contained. The four call sites map `None` to their prior fallback.
  The success path still frees the `LocalAlloc`'d buffer exactly once, after
  reading it.
* `ErrorEnvelope.code` is now the closed `MxcErrorCode` enum rather than a
  `String`; the snake_case wire format is unchanged (serde).
* `state_aware_request::deserialize_config` deserializes from the borrowed
  `&Value` instead of cloning the backend config subtree first.
* WSLC image enumeration is allocation-free (`slice::from_raw_parts` + a
  first-NUL scan with full-buffer fallback) instead of manual `.offset(i)`
  pointer walking and a per-entry `Vec<u8>`.
* LXC `validate_path` drops a provably-unreachable `\n`/`\r` branch already
  covered by `char::is_whitespace`.

Tests

* `cargo test -p wxc_common -p appcontainer_common -p wslc_common` — 126 + 45 +
  346 pass (includes the updated `sid_to_string_null_returns_none` and the
  `ErrorEnvelope` round-trip/serialisation tests).
* `cargo clippy -p wxc_common -p appcontainer_common -p wslc_common
  --all-targets -- -D warnings` and `cargo fmt ... -- --check` clean.
* `cargo check -p wxc --all-targets` clean (full Windows backend dispatch).
* Not built on this Windows host: the LXC `validate_path` change (`lxc_common`
  is Linux-only) — a removal of a provably-dead branch with no logic change.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Generated-with: claude-opus-4.8
@MGudgin MGudgin force-pushed the user/gudge/idiomatic-cleanups branch from 19d780c to d92ea0e Compare June 26, 2026 16:16
let mut group_sids: *mut PSID = std::ptr::null_mut();
let mut group_sid_count: u32 = 0;

unsafe {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue: missing safety comment here and throughout

/// handle that frees the SID on drop. Capability SIDs are an AppContainer
/// concept, so this helper lives in the AppContainer backend rather than the
/// shared foundation crate.
fn get_capability_sid_from_name(name: &str) -> Result<OwnedCapabilitySid, WxcError> {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question: should this just be a static member function inside OwnedCapabilitySid rather than a free function? Seems like they go hand in hand.

#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)]
pub struct ErrorEnvelope {
pub code: String,
pub code: MxcErrorCode,

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thought: I'm trying to remember why this was a String instead of the enum error code to begin with but I can't remember. It does seem more natural to use the enum though.

if !self.0 .0.is_null() {
unsafe {
let _ = LocalFree(Some(HLOCAL(sid)));
let _ = LocalFree(Some(HLOCAL(self.0 .0)));

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

note: for readability we should be a bit careful with nested tuple structs, since to get the inner value of them we need to use the .# syntax. In this case, we have to do .#.# to get the windows-rs PSID value so it can be a hard read if we were doing more than just that.

I'd probably just use a regular struct, and do something like:

struct OwnedCapabilitySid {
    sid: PSID,
}

impl OwnedCapabilitySid {
    fn new(sid: PSID) -> Self {
        Self { sid }
    }

    fn as_psid(&self) -> PSID {
        self.sid
    }
}

impl Drop for OwnedCapabilitySid {
    fn drop(&mut self) {
        let ptr = self.sid.0;

        if !ptr.is_null() {
            // SAFETY:
            // `OwnedCapabilitySid` owns this SID pointer. It must have been
            // allocated by the Windows API using LocalAlloc-compatible
            // allocation.
            unsafe {
                let _ = LocalFree(Some(HLOCAL(ptr)));
            }
        }
    }
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants