Skip to content

Port the managed agent to Rust - #531

Merged
Pedro Henrique Penna (ppenna) merged 5 commits into
devfrom
agents/fix-issue-358-managed-agent
Oct 16, 2026
Merged

Pedro Henrique Penna (ppenna) merged 5 commits into
devfrom
agents/fix-issue-358-managed-agent

Conversation

@ppenna

@ppenna Pedro Henrique Penna (ppenna) commented Oct 13, 2026 •

Copy link
Copy Markdown
Contributor

Summary

  • add the guest-side NVXS attach, ACK, and CREDIT records, the 1 MiB receive
    window, current feature masks, and the nvx_maps=COUNT parser
  • add byte-oriented guest EXEC and MAPS decoders without changing the existing
    UTF-8 host API
  • port nvx-managed-agent to a static Rust binary, including its session,
    dispatch, execution, containment, mapping, helper, launch, TTY, and
    diagnostic behavior
  • put every OS-facing unsafe operation behind documented safe
    nvx::guest::sys APIs, with OwnedFd-backed RAII resources
  • port the C-driven tests and connect the host ControlSession through a
    credit-accounting broker shim to the real agent and exec engine
  • build and test the agent through nvx.py with the other Rust guest binaries
  • confine the host's remaining Unix system calls to a private nvx::host::sys,
    through nix, and drop the direct libc dependency

Part of #358 (PR 2 of 5). Builds on #528, which has merged into dev.

The guests still ship guest/common/nvx-managed-agent.c. PR 5 will switch the
guest builds and remove the C programs and their old test harnesses.

PR #528 conventions

  • Standard-library agent modules deny unsafe code. Safe wrappers in
    nvx::guest::sys use nix 0.31.3 for descriptors, pipes, poll, termios,
    mounts, memfds and seals, identities, signals, kill, and wait operations
    where nix preserves the syscall contract. Focused libc and architecture FFI
    remain only in sys.
  • File descriptors, pipes, the sealed configuration memfd, and mappings are
    RAII-owned. Explicit consuming close/unmap paths retain C cleanup errors;
    Drop handles paths where C ignored cleanup. Raw descriptor integers never
    leave sys.
  • The agent and its helper fail with the shared CommandError and exit with
    the shared ExitStatus: 0, or AgentFailure (125). Mapping, launch,
    environment, process, and protocol failures use typed results and enums
    internally.
  • Every public and private module, type, field, variant, constant, static, and
    function is documented under the crate-wide
    clippy::missing_docs_in_private_items lint.
  • The shared parse_decimal reads the numeric identities and the helper's
    descriptor number as plain decimal, as the C agent's base-10 strtoull and
    strtol calls did.
  • No process changes its own environment. The agent and its helper build each
    workload environment in memory as an owned Vec<CString>, changing it as
    musl's setenv, putenv, and unsetenv would have, and pass it to
    execve. The helper applies the request's entries after the workload's
    identity applies.
  • The helper takes its sealed configuration with
    guest::sys::take_inherited_descriptor, which owns a fresh
    F_DUPFD_CLOEXEC duplicate and marks the inherited original close-on-exec,
    rather than adopting an arbitrary descriptor number.
  • GuestRustBuildConstants.BINARIES includes nvx-managed-agent, so
    nvx.py build-guest-binaries and test-guest-binaries build, inspect,
    manifest, and test it. As Remove the unused nvx-port-io helper #574 does for the retired nvx-port-io, a build
    adds it to the output of an earlier build whose manifest does not record
    it, and still refuses any other existing output.

Unsafe code, counted as unsafe { blocks and unsafe fn definitions outside
comments: guest::sys has 37 / 5, of which this PR adds 27 / 5, and the guest
has none outside it. Before the rework, PR 2's agent, protocol, and binary had
79 blocks / 2 functions outside sys. On the host, the Unix calls are in the
private host::sys, with 8 / 0 and one block test-only, and the host denies
unsafe code elsewhere except in the Windows platform module.

Fork boundary and parity

The safe fork entry point receives owned C strings, owned descriptors, and
precomputed descriptor actions. It constructs the complete argv, envp,
physical-PWD storage, and pointer vectors before fork. The child allocates
nothing, performs no Rust cleanup, runs only direct async-signal-safe
Linux/musl operations, and terminates through execve, execv, or _exit.

  • Direct launch preserves the C order: clear supplementary groups, take the
    workload GID and UID, enter the cwd, regain root, join nvx-exec, then run
    setpriv with --reuid, --regid, --clear-groups,
    --no-new-privs, and all three capability-clearing options.
  • The current boot contract remains one decimal nvx_maps=COUNT token from 1
    through 4096. Retired nvx_map= tokens remain ignored.
  • Linux argv, environment, and path fields remain arbitrary non-NUL bytes.
    Dedicated guest decoders retain the C agent's 4096-byte cwd bound.
  • Canonical cwd names retain the supplied spelling. Noncanonical names fill a
    preallocated physical-PWD slot after the child enters the directory under
    workload credentials. Failures remain cwd-failed with the original errno.
  • Diagnostics and wire bytes remain unchanged, apart from the already
    documented musl strerror wording.

Security review

The earlier independent review of the original port found no vulnerabilities.
Its one non-security cwd/PWD parity finding was fixed before this rework.

The post-refactor self-review found no regression:

  • privilege-drop ordering and the setpriv chain are unchanged
  • every pipe, control, cgroup, barrier, diagnostic, and mapping descriptor is
    close-on-exec; only the sealed helper configuration deliberately survives
    exec, and the helper closes it
  • file mapping targets still use
    O_NOFOLLOW|O_CREAT|O_EXCL|O_CLOEXEC
  • bind remounts still use
    MS_BIND|MS_REMOUNT|MS_NOSUID|MS_NODEV, plus MS_RDONLY when requested;
    the hostfs parent remains root:root mode 0700
  • inherited SIGPIPE is restored at startup; barrier SIGPIPE changes have an
    explicit error path and best-effort RAII restoration
  • process-group and cgroup kills, strict pids.current, orphan reaping, and
    containment-failure latching are unchanged; raw wait-status decoding keeps
    every Linux real-time signal as a terminal workload outcome
  • every host-controlled count, length, and offset is checked before allocation
    or indexing; no untrusted-input path can panic

PID-1-style reaping tests run in isolated test-harness subprocesses, so their
intentional waitpid(-1) behavior cannot collect children owned by parallel
host tests.

Review follow-ups

This PR stays a faithful port of the C agent. Review suggestions that would
change its implementation or the behavior that it inherits from the C agent
wait until the #358 series is done, and each has a tracking issue:

#358 also checks in the C agent's parity fixtures before PR 5 removes it.

Validation

At 887020073

After the rebase onto dev with #574 and the review fixes. The full suites ran
at 2afceeece, and the affected nvx-rs and guest-binary checks ran again
after the DATA-stage diagnostic fix:

  • Windows and WSL Linux: complete check-nvx-rs and
    check-aci-edge-sandboxes commands, the musl guest-binary check, build, and
    prebuilt tests, and the validate-nvx Python suite
  • a guest-binary output that dev built updates in place to the six-binary
    inventory and passes the prebuilt tests

The results below come from earlier heads. The rebase and the review fixes
change no agent behavior.

Windows

  • python scripts\nvx.py verify
  • complete check-nvx-rs: workspace fmt, both Clippy modes, default and
    all-feature tests, synthetic bundle, rustdoc with warnings denied, Rust 1.89
    MSRV, aarch64-apple-darwin, and musl dev/guest-profile Clippy
  • complete check-aci-edge-sandboxes
  • affected guest-binary/CI tests, Ruff check/format, and Linux+Windows Pyright

WSL Linux

  • complete GNU and musl check-nvx-rs commands and complete dependent ACI
    crate checks
  • three consecutive all-feature workspace suites after isolating process tests
  • two fresh nvx.py build-guest-binaries builds are byte-identical; all 22
    prebuilt-binary tests pass
  • a real Port the small guest tools to Rust #528 six-binary output rebuilds safely in place to the seven-binary
    inventory, then passes all prebuilt tests
  • final source digest:
    8425dbbfea5d10453b586bda140233066ec2bebd05673aeaf65437f411c4f2dc

Differential

On both prometheus32 and prometheus30:

  • C versus old Rust, C versus reworked Rust, and old versus reworked Rust each
    match all 3,600 deterministic cases: 1,200 EXEC, 1,200 MAPS, and 1,200
    nvx_maps command lines
  • accepted counts remain 148, 144, and 1,081 respectively
  • old/new loopback protocol transcripts canonicalize to the same 42 events and
    2,433 bytes, SHA-256
    8a90e0da18f2f64729edc009220bfaf3787cd11fafc5130ecde873697fe45576;
    canonicalization removes random request IDs, pipe chunk boundaries, and the
    output-limit case's permitted bounded zero-prefix length

In VM

Fresh final-head binaries were overlaid into copies of the current Alpine and
Ubuntu initramfses on prometheus32 (KVM/baremetal) and prometheus30
(MSHV/baremetal).

  • Alpine: managed-exec-config, sandbox-lifecycle, sandbox-blocks, and
    scratch-snapshot
  • Ubuntu: managed-lifecycle and filesystem-shares
  • in-guest agent identity and cwd parity:
    • KVM SHA-256 d57f7582c7857337d93ce77d37f21099aaedaf16fe3ad8e7d61ebc61d9a799c0
    • MSHV SHA-256 ada79dc0a747d903d8ef6da675863458fa38e2b0bcf6b10101f6c90e00f25509
  • KVM test-aci-edge-sandboxes: 18/18 pass
  • MSHV: 17/18 pass; the new host-loopback test times out waiting for port
    publication on this host and fails identically with the untouched baseline
    initramfs. The agent-relevant tests and the prior 17-test suite pass.

The final Rust agent is a static PIE: 459,232 bytes on prometheus32 and
459,296 bytes on prometheus30, about 35.4% smaller than the 711,400-byte C
agent.

Both hosts' original initramfs files were restored byte-for-byte after every
run. Scratch/state directories were removed, validation worktrees and
submodules are clean, and no OpenVMM process remains.

A second independent review of the reworked agent found no regressions against the C implementation and identified two internal API soundness gaps in the new system layer, which are now fixed.

Copilot AI balanced review requested due to automatic review settings October 13, 2026 19:36

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.

🟡 Changes recommended

Data-record failures no longer preserve the C agent’s diagnostic stages and status details.

1 open finding
What changed in this PR

Ports the managed guest agent to Rust while preserving the existing protocol, execution, containment, mapping, and diagnostic contracts. Guest images continue using the C agent until a later build-switch PR.

Changes:

  • Adds byte-oriented guest protocol decoding and managed-agent session handling.
  • Implements execution, cgroup containment, mappings, launch helpers, and loopback tests.
  • Adds static musl guest binaries, ELF validation, CI coverage, and documentation.
File Description
.github/​actions/​check-nvx-rs/​action.yml Builds and validates musl guest binaries.
doc/​ci.md Documents guest-binary CI checks.
doc/​contribute.md Updates Rust validation guidance.
doc/​project-structure.md Documents guest modules and binaries.
doc/​usage.md Documents check-static-elf.
nvx-rs/​Cargo.toml Defines guest feature, binaries, and profile.
nvx-rs/​README.md Documents guest binaries and agent architecture.
nvx-rs/​src/​lib.rs Exposes Linux guest modules.
nvx-rs/​src/​bin/​nvx-console-pending.rs Adds console-pending entry point.
nvx-rs/​src/​bin/​nvx-device-io.rs Adds freestanding device benchmark entry point.
nvx-rs/​src/​bin/​nvx-exit.rs Adds freestanding VM-exit entry point.
nvx-rs/​src/​bin/​nvx-managed-agent.rs Adds managed-agent entry point.
nvx-rs/​src/​bin/​nvx-mmio-write.rs Adds MMIO writer entry point.
nvx-rs/​src/​bin/​nvx-port-io.rs Adds port-I/O entry point.
nvx-rs/​src/​bin/​nvx-reseed.rs Adds reseed entry point.
nvx-rs/​src/​guest/​agent/​cgroup.rs Implements workload cgroup containment.
nvx-rs/​src/​guest/​agent/​diagnostics.rs Implements agent diagnostics and descriptor I/O.
nvx-rs/​src/​guest/​agent/​dispatch.rs Dispatches control requests.
nvx-rs/​src/​guest/​agent/​exec.rs Supervises workload execution and output.
nvx-rs/​src/​guest/​agent/​exec_config.rs Encodes sealed helper configuration.
nvx-rs/​src/​guest/​agent/​helper.rs Applies helper environment and working directory.
nvx-rs/​src/​guest/​agent/​launch.rs Implements fork and launch preparation.
nvx-rs/​src/​guest/​agent/​loopback.rs Tests real host-to-agent control flow.
nvx-rs/​src/​guest/​agent/​mappings.rs Applies announced host mappings.
nvx-rs/​src/​guest/​agent/​mod.rs Defines managed-agent modes and lifecycle.
nvx-rs/​src/​guest/​agent/​seam.rs Provides production and test system seams.
nvx-rs/​src/​guest/​agent/​session.rs Implements guest protocol sequencing and credit.
nvx-rs/​src/​guest/​agent/​tty.rs Opens and configures the control TTY.
nvx-rs/​src/​guest/​console_pending.rs Ports pending-console inspection.
nvx-rs/​src/​guest/​freestanding/​args.rs Parses kernel-provided arguments.
nvx-rs/​src/​guest/​freestanding/​device_io.rs Ports the freestanding I/O benchmark.
nvx-rs/​src/​guest/​freestanding/​exit.rs Ports the freestanding exit utility.
nvx-rs/​src/​guest/​freestanding/​hosted.rs Provides hosted test entry points.
nvx-rs/​src/​guest/​freestanding/​mem.rs Supplies freestanding memory routines.
nvx-rs/​src/​guest/​freestanding/​mod.rs Organizes freestanding components.
nvx-rs/​src/​guest/​freestanding/​start.rs Supplies _start and panic handling.
nvx-rs/​src/​guest/​freestanding/​syscall.rs Wraps raw x86-64 syscalls.
nvx-rs/​src/​guest/​mmio_write.rs Ports MMIO writing.
nvx-rs/​src/​guest/​mod.rs Organizes guest programs.
nvx-rs/​src/​guest/​parse.rs Reimplements C-compatible numeric parsing.
nvx-rs/​src/​guest/​port_io.rs Ports port and generation-ID operations.
nvx-rs/​src/​guest/​reseed.rs Ports entropy reseeding.
nvx-rs/​src/​guest/​sys.rs Preserves shared C runtime behavior.
nvx-rs/​src/​protocol/​boot.rs Parses nvx_maps=COUNT.
nvx-rs/​src/​protocol/​exec.rs Adds byte-oriented guest EXEC decoding.
nvx-rs/​src/​protocol/​features.rs Defines direct and sandbox feature masks.
nvx-rs/​src/​protocol/​maps.rs Adds byte-oriented guest MAPS decoding.
nvx-rs/​src/​protocol/​mod.rs Exports new guest protocol APIs.
nvx-rs/​src/​protocol/​outer.rs Adds guest attach, ACK, and CREDIT records.
nvx-rs/​src/​protocol/​tests.rs Tests guest protocol parity and boundaries.
nvx-rs/​tests/​guest_tools.rs Exercises built guest binaries.
scripts/​nvx.py Adds the static-ELF command.
scripts/​nvx_tools/​elf.py Implements ELF linkage inspection.
scripts/​test_nvx_tools.py Tests CI coverage and ELF validation.

🧠 Review effort: Balanced


💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread nvx-rs/src/guest/agent/dispatch.rs Outdated
Copilot AI balanced review requested due to automatic review settings October 13, 2026 19:49
@ppenna
Pedro Henrique Penna (ppenna) force-pushed the agents/fix-issue-358-managed-agent branch from 9f13734 to 7548a55 Compare October 13, 2026 19:49

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.

🔵 Needs a closer look

The extensive unsafe fork/exec, protocol, containment, and privileged filesystem changes require final human review despite strong parity coverage.

0 open findings

1 resolved since last review

🧠 Review effort: Balanced

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.

🔵 Needs a closer look

The privileged launch and containment port needs final human review, and unresolved correctness issues remain.

2 open findings

🧠 Review effort: Balanced

Comment thread nvx-rs/src/guest/sys/fd.rs Outdated
Comment thread nvx-rs/src/guest/managed_agent/cgroup.rs
@ppenna
Pedro Henrique Penna (ppenna) force-pushed the agents/fix-issue-358-managed-agent branch from 5378207 to 54f904a Compare October 14, 2026 19:37
Copilot AI balanced review requested due to automatic review settings October 14, 2026 19:37
@ppenna
Pedro Henrique Penna (ppenna) force-pushed the agents/fix-issue-358-managed-agent branch from 54f904a to f4f6b80 Compare October 14, 2026 19:46

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.

🔵 Needs a closer look

Unresolved correctness issues and the privileged fork/exec boundary require fixes and final human review.

2 open findings
2 resolved since last review

🧠 Review effort: Balanced

Comment thread nvx-rs/src/guest/sys/process.rs Outdated
Comment thread nvx-rs/src/guest/agent/exec_config.rs Outdated
Copilot AI balanced review requested due to automatic review settings October 14, 2026 19:46

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.

🔵 Needs a closer look

The privilege-sensitive launch, containment, and system-interface port warrants final human review.

1 open finding
1 resolved since last review

🧠 Review effort: Balanced

@ppenna
Pedro Henrique Penna (ppenna) force-pushed the agents/fix-issue-358-managed-agent branch from f4f6b80 to 8d0a02c Compare October 14, 2026 19:56
Copilot AI balanced review requested due to automatic review settings October 14, 2026 19:56

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.

🔵 Needs a closer look

The privileged agent port warrants final human review, and compatibility with existing build-output directories remains unresolved.

1 open finding
1 resolved since last review

🧠 Review effort: Balanced

Comment thread scripts/nvx_tools/build_constants.py
Copilot AI balanced review requested due to automatic review settings October 14, 2026 20:13
@ppenna
Pedro Henrique Penna (ppenna) force-pushed the agents/fix-issue-358-managed-agent branch from 8d0a02c to 0ed5ad7 Compare October 14, 2026 20:13
Copilot AI balanced review requested due to automatic review settings October 15, 2026 21:16
@ppenna
Pedro Henrique Penna (ppenna) force-pushed the agents/fix-issue-358-managed-agent branch from 61d24f1 to c30c46e Compare October 15, 2026 21:16

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.

🔵 Needs a closer look

The privileged PID-1 execution, containment, and fork/exec boundaries warrant final human review.

0 open findings

🧠 Review effort: Balanced

Comment thread nvx-rs/src/bin/nvx-managed-agent.rs Outdated
@@ -0,0 +1,14 @@
//! `nvx-managed-agent CONTROL_TTY ROOTFS HOSTNAME UID GID USER HOME`; see

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.

We should add a description of what this executable is.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done in aebcc3a: the header now says what the binary is, the managed guest agent that serves the control session and runs one workload at a time, as the guest's PID 1 or in a sandbox's root filesystem. It also gives both command lines, the agent's and its internal --exec-config-fd helper's.

/// Converts arbitrary bytes into a C string, or `EINVAL` if they contain a NUL.
pub(crate) fn c_string(bytes: &[u8]) -> Result<CString, Errno> {
CString::new(bytes).map_err(|_| Errno::EINVAL)
}

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.

will this function only be used in this file?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

No: environment, exec, helper, launch, mappings, and the loopback tests all use it. It isn't a diagnostic, so aebcc3a moves it to guest::sys, next to path_c_string, which now calls it. diagnostics.rs now holds only diagnostics: the session and the helper configuration call FileDescriptor::read_exact and write_all directly instead of two pass-through wrappers.

DataFailureKind::Identity => "data-identity",
DataFailureKind::Application => "data-application",
DataFailureKind::Credit => "data-credit",
};

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.

Make this a function for DataFailureKind?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done in aebcc3a: DataFailureKind::stage() returns the C agent's stage name, and dispatch calls it.

@esaurez Enrique Saurez (esaurez) 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.

Suggestions to make this port easier to build on for the follow-ups tracked in #535: streaming exec (#537), a minimal guest root and descriptor-rooted mapping (#539), a standalone image-backed sandbox (#541), and a second adapter with its own control transport (#545).

The structural comments are intended to keep wire bytes, limits, exit statuses and diagnostics unchanged, so the existing differential cases and loopback transcripts can prove them. They could also land as a small follow-up before PR 5 removes the C agent, while the C agent can still serve as the reference. The command-line and symlink comments concern behavior inherited from the C agent.

const BARRIER_ATTEMPTS: usize = 500;

/// Executes one validated workload request and streams its terminal outcome.
pub(crate) fn run(

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.

Split the execution engine from the NVXC session.

run both supervises the workload (child, pipes, cgroup, timeout, cancel) and speaks NVXC: it reads control records, writes APP_STDOUT/APP_STDERR/APP_EXIT, and answers BUSY. #537 asks for process/stdio/cancel/reap primitives that "a synchronous NVXC loop and an independently framed adapter can drive", and #545 needs a second adapter to reuse them.

Suggestion: a core that owns the child, emits events (output, exit, launch failure) and accepts commands (cancel, terminate). The NVXC v1 loop keeps today's poll order and maps those events to the same frames and BUSY replies.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Agreed. To keep this PR a faithful port, it waits until the #358 series is done, and #537 tracks it (comment). NVXC v1 keeps today's poll order, frames, and BUSY replies.

if let Ok(count) = count
&& count > 0
{
if *output_bytes > protocol::MAX_OUTPUT_BYTES - count {

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.

Pass the v1 output cap and stdin source in as policy.

The 1 MiB cap is enforced here through protocol::MAX_OUTPUT_BYTES, and SpawnPlan always opens null_device for stdin. Both are NVXC v1 rules that #537 replaces with piped stdin and credit-based streaming. If the v1 loop passes them in, for example output_limit: Some(MAX_OUTPUT_BYTES) and Stdin::Null(path), behavior is unchanged and #537 does not need to touch spawn or containment.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Agreed. #537 tracks it for after the #358 series (comment), with the v1 loop passing output_limit: Some(MAX_OUTPUT_BYTES) and Stdin::Null(path).

/// Child report stage for failure to enter the requested working directory.
pub(crate) const LAUNCH_STAGE_CWD: i32 = sys::CHILD_STAGE_CWD;
/// Borrowed launch inputs and owned descriptor endpoints prepared by the execution engine.
pub(crate) struct SpawnRequest<'a> {

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.

Give launch neutral inputs.

SpawnRequest carries AgentConfig (the agent's argv strings) and GuestExecPayload (the decoded NVXC frame), so launching a workload depends on both the agent's command line and the wire format. Converting once at dispatch into a neutral exec spec (argv, environment, inherit flag, cwd, timeout) and an explicit identity would give #539's root-entry and spawn hooks an input that another adapter can build. UID/GID validation can stay at launch time to keep today's failure stage.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Agreed. #539 tracks it for after the #358 series (comment), with UID and GID validation kept at launch time.

}

/// Reaps every currently available child and records the workload's first terminal state.
pub(crate) fn reap(child: ProcessId, status: &mut Option<ChildStatus>) {

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.

Give reaping a single owner.

reap and verify_empty call waitpid(-1, WNOHANG) from the execution path, so they collect any child of the process. That is right for the direct agent, which runs as PID 1 and reaps orphans. It does, however, prevent embedding the core in a process with other children, such as one that supervises a helper or runs an async runtime that waits on its own children (#545).

Suggestion: one agent-owned reaper that keeps PID 1's orphan reaping and records exit statuses by PID, with the execution core asking it for its own child's status. The direct agent's observable behavior stays the same.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Agreed. #539 tracks it for after the #358 series (comment). Until then, the direct agent keeps the C agent's waitpid(-1, WNOHANG) reaping.

MountFlags::MS_NOSUID | MountFlags::MS_NODEV | MountFlags::MS_NOEXEC,
)?;
}
let execution = root.join("nvx-exec");

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.

Pass the execution cgroup in as a value.

nvx-exec is rebuilt in prepare, has_tasks, kill and launch.rs, so only the direct root's fixed cgroup gets cgroup-level settlement; in sandbox mode, terminate kills only the process group. Two follow-ups need the same helpers on other cgroups:

Suggestion: a small ExecCgroup { dir } with prepare/has_tasks/kill/settle/procs. v1 still creates nvx-exec on every EXEC, so the system calls and diagnostics stay the same.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Agreed. #539 tracks it for after the #358 series (comment), with NVXC v1 still creating nvx-exec on every EXEC.

}

/// Creates one target and bind-mounts a validated mapping onto it.
fn apply(system: &mut impl System, mapping: &GuestHostMap) -> Result<(), Errno> {

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.

Make the mapping roots explicit and keep NVXC batching separate.

apply takes the export root from Paths and treats targets as absolute paths in the agent's own root, which is right for the blockless root. #539 asks to separate HostMap semantics from NVXC frames, because another adapter may carry the same table in its own framing, and to create targets only on the sandbox's own overlay.

Suggestion: apply_maps(system, export_root, target_root, first, maps). The NVXC loop keeps HostMappings and its sequence rules and passes /, so the paths stay byte-identical. #539 can then replace the inside with a descriptor-rooted walk without touching dispatch.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Agreed. #539 tracks it for after the #358 series (comment), with the NVXC loop passing / so that paths stay byte-identical.

Comment on lines +39 to +41
let mut command_line = [0u8; 4096];
let count = std::fs::File::open(&system.paths().proc_cmdline)
.and_then(|mut file| file.read(&mut command_line[..4095]))

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.

The command-line buffer and design 5.1.2 disagree.

prepare reads /proc/cmdline once into a 4 KiB buffer. That covers today's guests: x86 Linux keeps at most 2,048 bytes including the NUL (COMMAND_LINE_SIZE), and the host budgets 1,024 bytes for caller tokens (COMMAND_LINE_BUDGET). Design 5.1.2, however, allows a 64 KiB command line. If that is the intended limit, a late nvx_maps= token would be missed and MAPS refused (fail-closed). Either read to EOF with a 64 KiB bound, or align 5.1.2 with the kernel limit. The C agent uses the same 4 KiB buffer.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Right, and the C agent reads the same 4,095 bytes, so the port keeps that for now. #570 already tracks the mismatch between 5.1.2's 64 KiB and the guest kernel's 2,048 bytes, and I added a task there to read /proc/cmdline to its end if the kernel's limit ever rises above 4 KiB.

}

/// Creates every missing directory component in `path`.
fn make_directories(path: &Path, include_last: bool) -> Result<(), Errno> {

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.

Mapping paths follow symlinks in intermediate components.

apply resolves the source and target by name several times. make_directories and both mount calls follow symlinks; only create_file refuses one (O_NOFOLLOW, last component only). Guests may create symlinks in read-write exports (design 3.6.5), and ExportPlan::verify checks the pinned paths on the host before boot, not what the guest resolves later.

Example: work RO, work/out RW, work/out/deps/cache RO. Suppose a writer that runs during the boot, such as another sandbox sharing work/out or a host process, replaces work/out/deps with a symlink to /var after the host check. The bind and its RO/nosuid/nodev remount then land on the guest's /var/cache, if present, which RESERVED_TARGETS excludes. No route to code execution is apparent; the effect is mounts on guest paths that the table never named.

#539 already plans descriptor-rooted application without symlink escapes (openat2 with RESOLVE_BENEATH | RESOLVE_NO_SYMLINKS, then open_tree/mount_setattr/move_mount). A regression test there for this intermediate-component case would cover it. The C agent behaves the same.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Agreed, and the C agent does the same, so the faithful port keeps it for now. #539's descriptor-rooted walk is the fix, and I added this intermediate-component case to its tracking (comment) as a required regression test.

system.mount(None, &target, None, flags)
}

/// Appends arbitrary Unix bytes to a trusted root path.

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.

will we ever use these files functions in other places? Or is only during mappings that the agent will do them? May be worth move them to helper libraries.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Only mapping uses them. They create mapping targets as the C agent does, directories with mode 0755 and files with O_NOFOLLOW|O_CREAT|O_EXCL, over the sys::create_directory and sys::open primitives. #539 replaces them with a descriptor-rooted walk (comment), so they stay private here rather than move to a shared library now.

@esaurez Enrique Saurez (esaurez) 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.

Refresh the PR description to match the final implementation.

The "PR #528 conventions" section still says helper environments use an ownership-transferring putenv wrapper. At c30c46ef, managed_agent/environment.rs and helper.rs instead build an owned Vec<CString> environment and pass it explicitly to execve; workload-request environment entries are applied by the helper after privilege dropping. Please describe that current ownership/safety argument rather than the earlier process-global mutation design.

It would also help to spell out the final inherited-configuration-FD fix: guest::sys::take_inherited_descriptor owns a fresh F_DUPFD_CLOEXEC duplicate and marks the original close-on-exec, rather than adopting an arbitrary descriptor number. These are documentation updates, not requests to reintroduce the old implementations.

/// This differs from [`decode_exec_payload`] only where Linux permits bytes that are not UTF-8
/// and at the C guest's 4096-byte working-directory boundary. The host-facing decoder remains
/// text-oriented, so this guest decoder does not broaden host behavior.
pub fn decode_guest_exec_payload(payload: &[u8]) -> Result<GuestExecPayload, ProtocolError> {

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.

Reduce duplicated decoding while preserving the host API.

decode_guest_exec_payload repeats the framing, flags, lengths, and field validation in decode_exec_payload; MAPS now has the same parallel byte/string implementations. A future format or validation change could update one path but leave the other behind.

Could we share the byte-oriented structural parsing and keep the existing UTF-8 host types behind compatibility conversions? Preserve non-UTF-8 guest acceptance, the public host API and diagnostics, and make intentional host/guest differences explicit. For example, the two inherit_default_env fields currently have different projections when no explicit environment is supplied; this should not be a blind type substitution.

This can be a behavior-preserving follow-up to the faithful port, giving #537 and #545 one parsing implementation to build on rather than another pair of codecs.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Agreed. #581 tracks it for after the #358 series. It keeps the host API, its diagnostics, and the guest's non-UTF-8 acceptance, makes the inherit_default_env difference explicit, and checks the shared decoder against the C agent's parity fixtures rather than against itself.

format!("#define OUTER_HEADER_LEN {OUTER_HEADER_LEN}U"),
format!("#define OUTER_MAX_PAYLOAD {OUTER_MAX_PAYLOAD}U"),
format!("#define OUTER_VERSION {OUTER_VERSION}U"),
format!("#define OUTER_GUEST_ATTACH {OUTER_GUEST_ATTACH}U"),

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.

Preserve the parity reference before removing the C agent.

Could we check in the reported deterministic EXEC/MAPS/nvx_maps cases (or their reproducible generator plus expected C outcomes) and the canonical loopback transcript as versioned fixtures before PR 5 removes the C implementation?

guest_agent_source() currently turns a missing or unreadable C source into None, and the source-comparison tests return early. Removing that file would therefore silently remove these compatibility assertions. The new golden frames are a useful start; please also preserve the broader acceptance and transcript reference described in the PR, and replace C-source-string checks with durable expected constants before deletion.

Expected outcomes should remain independent of the new decoder, so a decoder change cannot regenerate its own passing reference. This belongs in the pre-removal follow-up if it is kept out of the faithful-port PR, and can supply shared conformance cases for #537/#545.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Agreed. This has to land before PR 5 deletes the C agent, so it's now a task in #358, right before the deletion: check in the EXEC, MAPS, and nvx_maps cases with their expected C outcomes, or their generator, and the canonical loopback transcript, and replace the C-source comparisons with expected constants.

Add the guest attachment, acknowledgement, and credit records and
window payloads. Parse the current nvx_maps count contract and expose
the feature sets announced by each agent mode.

Keep byte-oriented EXEC and MAPS decoders for the Linux guest beside the
existing UTF-8 host decoders, including the C guest's 4096-byte cwd
bound. Pin the new records and parser with golden and round-trip tests.

Part of #358.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI balanced review requested due to automatic review settings October 16, 2026 02:42
@ppenna
Pedro Henrique Penna (ppenna) force-pushed the agents/fix-issue-358-managed-agent branch from c30c46e to 2afceee Compare October 16, 2026 02:42
@ppenna

Copy link
Copy Markdown
Contributor Author

Thanks for the review. At 2afceeece:

The branch is also rebased onto dev, including #574's removal of nvx-port-io.

@ppenna
Pedro Henrique Penna (ppenna) marked this pull request as ready for review October 16, 2026 02:44

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.

🔵 Needs a closer look

The privileged process-launch and containment rewrite requires final human review, and diagnostic parity remains unresolved.

1 open finding

🧠 Review effort: Balanced

Comment thread nvx-rs/src/guest/managed_agent/dispatch.rs Outdated
Port the managed agent as a static Rust guest binary while preserving
its control session, execution, cgroup, host-mapping, sealed
configuration, helper, and diagnostic contracts.

Follow the guest programs' conventions. The agent and its execution
helper fail with the shared CommandError and exit with the new shared
ExitStatus::AgentFailure, 125. Errors are nix's Errno, whose
descriptions the diagnostics print, and the safe nvx::guest::sys
wrappers make the system calls through nix. Only the forked child, the
raw wait-status decoding, which keeps real-time signals, and the
duplication of the inherited descriptor reach libc, through nix's
re-export. The standard library parses the decimal identities and
descriptor, and no resource is dropped by hand.

Prepare argv, envp, physical-PWD storage, and descriptor actions before
fork. The child allocates nothing and terminates through exec or _exit.
The helper builds the workload's environment in memory and passes it to
execve, so no process changes its own environment and the binary needs
no unsafe entry point. Keep the privileged seam for production wrappers
and failure-injecting tests, and cover the complete path with the
host-to-agent loopback.

Part of #358.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Exercise the managed agent helper as a real process, including exact and
layered environments, cwd handling, malformed configuration, the
internal descriptor lifetime, and command-line failures. The tests can
use either the Cargo test binary or the prebuilt guest-profile binary.

Part of #358.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Add nvx-managed-agent to the Rust guest-binary inventory. The existing
nvx.py build-guest-binaries and test-guest-binaries commands now build
its static musl executable in the same fresh, reproducible Cargo build
as the other guest programs, check its ELF form, include its digest in
the manifest, and run its prebuilt helper integration tests.

A build removes the retired nvx-port-io from an earlier output whose
manifest records it, and in the same way, it adds nvx-managed-agent to
an earlier output whose manifest does not record it. It still refuses
any other existing output, such as one that lacks an executable that
its manifest records. The command documentation now counts six
binaries.

Part of #358.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Move the host's remaining Unix FFI behind documented safe functions in
a new private host::sys module, which makes its system calls through
nix, as guest::sys does for the guest programs. The host module now
denies unsafe code, except in sys and the unchanged Windows platform
module.

- The control socket's connect and SO_PEERCRED use nix's socket API,
  and the effective IDs, setsid, open, and fcntl use nix's wrappers.
- nix has no wrapper for pidfd_open, pidfd_send_signal, close_range, or
  getdents64, so sys makes them through nix's re-export of libc. The
  pre_exec hook still allocates nothing, so it reads /proc/self/fd with
  getdents64 rather than through nix's Dir.
- The fake OpenVMM polls through nix, and the managed agent's loopback
  tests take errno and signal values from guest::sys.

nvx-rs no longer depends on libc directly. The host and testing
features enable nix, which becomes a Unix rather than a Linux
dependency because the host and the fake OpenVMM also build on macOS,
and aci_edge_sandboxes/Cargo.lock gains it.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI balanced review requested due to automatic review settings October 16, 2026 02:53
@ppenna
Pedro Henrique Penna (ppenna) force-pushed the agents/fix-issue-358-managed-agent branch from 2afceee to 8870200 Compare October 16, 2026 02:53

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.

🔵 Needs a closer look

The extensive privileged launch, containment, and unsafe-boundary changes warrant final human review.

0 open findings

1 resolved since last review

🧠 Review effort: Balanced

@ppenna
Pedro Henrique Penna (ppenna) merged commit f429da8 into dev Oct 16, 2026
34 checks passed
@ppenna
Pedro Henrique Penna (ppenna) deleted the agents/fix-issue-358-managed-agent branch October 16, 2026 11:20
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