Repository navigation
T55 — Build the CLI's and the VM tests' create requests through the same constructor - #2480
Conversation
The session and task creates, the dashboard's activate and the crate's test fixtures now call CreateSessionRequest::new and set only the fields that differ. A new test refuses any struct literal for the request or its box config, and the version-gate inventory now keys on the constructor.
…ctor The egress, classifier, session and escape harnesses call CreateSessionRequest::new and set only the registered box id and addresses where a test registers the box.
|
Important Review skippedBot user detected. To trigger a single review, invoke the ⚙️ Run configuration
You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
|
Approved: no findings at Nothing to do. Verdict APPROVE, posted as a comment: the App cannot review its own pull request; the check and the label carry it. Clobber: none seen. |
There was a problem hiding this comment.
arch-review: PASS · the full ruling
The design does not govern how the client build
8000
s a create request, so this is a PASS by absence, the same as #2472. All three product creates still send the same-build assertion. The VM harnesses send the same request as before. The deferred VM leg guest_kernel_accepts_rendered_ruleset did not run, and it stays open in CI.
Ruled at 248ca11 against gominimal/arch@109541a48c31 by claude-opus-5-5.
There was a problem hiding this comment.
Babysit review on 248ca11 against T55's plan block. Every create in the plan's layers (create.rs, task.rs, dash/rpc.rs, tests/cli.rs and the four minvmd VM tests) now builds through CreateSessionRequest::new and sets only the fields that differ: hooks_enabled from --no-hooks, and the registration's addresses and box id. Each create path still sets must_match_version from version_assertion(), and the version-gate scan in src/tests.rs now looks for that assignment. The dashboard and its own_ip_config test helper set name back to the form's Option after the constructor, so an unnamed box still sends None, not Some(""). In create.rs, version_assertion() is now read once before the create loop, not on every retry, and it is constant for the process. The new literal scan keeps struct-literal creates from coming back. arch-review PASS.
…an attach or an exec The native daemon's control socket answers only the answerer verbs, so asking it to re-hold a name was a refusal warned on every native attach and exec's stderr (#2213). The re-hold is for Minvmd alone, as the activation's hold is; every caller of box_via_ssh passes the daemon's provider kind. Re-applied onto main's split CLI files (#2479, #2480).
…an attach or an exec The native daemon's control socket answers only the answerer verbs, so asking it to re-hold a name was a refusal warned on every native attach and exec's stderr (#2213). The re-hold is for Minvmd alone, as the activation's hold is; every caller of box_via_ssh passes the daemon's provider kind. Re-applied onto main's split CLI files (#2479, #2480).
…an attach or an exec (gominimal#2225) The native daemon's control socket answers only the answerer verbs, so asking it to re-hold a name was a refusal warned on every native attach and exec's stderr (gominimal#2213). The re-hold is for Minvmd alone, as the activation's hold is; every caller of box_via_ssh passes the daemon's provider kind. Re-applied onto main's split CLI files (gominimal#2479, gominimal#2480).
T55 — Build the CLI's and the VM tests' create requests through the same constructor
slice: S0
kind: enablement
covers: none
size: M
est: 120 code, 160 tests
executor: agent
difficulty: routine
model: standard
diagnostics: none: test literals only, no behaviour
observability: none: test literals only, no signal
layers:
- cli:
- crates/minimal/src/cmd/boxes/create.rs: constructor
- crates/minimal/src/task.rs: constructor
- crates/minimal/src/dash/rpc.rs: constructor
- crates/minimal/src/tests.rs: constructor
- crates/minimal/tests/cli.rs: constructor
test: cargo nextest run -p minimal cli_create_requests_use_the_constructor
- vm-tests:
- crates/minvmd/tests/egress_allowlist_integration.rs: constructor
- crates/minvmd/tests/guest_classifier_integration.rs: constructor
- crates/minvmd/tests/minimald_session_integration.rs: constructor
- crates/minvmd/tests/vm_escape_integration.rs: constructor
test (deferred: vm): cargo nextest run -p minvmd guest_kernel_accepts_rendered_ruleset
produces: none
consumes: none
proof: Every client create request uses one constructor; activation unchanged.
depends on:
- blocked by #2381
- blocked by #2383
Proof
Every create request in the min CLI (min session activate, min task run, the dashboard's activate), in the minimal crate's test fixtures and in the four minvmd VM harnesses now comes from CreateSessionRequest::new. Each site sets only the fields that differ. The new test cli_create_requests_use_the_constructor fails on any struct literal for the request or its BoxConfig under crates/minimal/src or crates/minimal/tests. Activation is unchanged: every product create still sends this build as must_match_version, and the hooks, network, policy and registered addresses are the same as before. The full minimal suite passes (708 tests).
Notes from the agent: Deferred:
cargo nextest run -p minvmd guest_kernel_accepts_rendered_ruleset(lane: vm). That test exists already and I did not run it. The four VM test files sit behind #![cfg(minvmd_libkrun)], and a plain clippy skips them. I type-checked them withLIBKRUN_PREFIX=/tmp/nokrun cargo clippy -p minvmd --tests, which sets the cfg without a link, and it was clean. The dashboard can send an unnamed box, and the constructor always sets a name. So dash::activate and the own_ip_config test helper call new with an empty name and then set config.name from the form. A constructor that takes an Option name is in minimald-rpc, which is outside this task's layers. The existing every_create_session_asserts_the_daemon_build inventory now keys onCreateSessionRequest::new(and on.must_match_version = ...version_assertion()assignments. It also lists dash/rpc.rs::own_ip_config as 'asserts nothing', with a comment, because that test helper builds a config that only a fake create closure receives. In create.rs and task.rs the version assertion is now computed once before the retry loop, not once for each attempt. It reads only the skew-override environment variable, so the value is the same. I ran the full minimal crate once at the end: 708 passed.Tests
cargo nextest run -p minimal cli_create_requests_use_the_constructorVerify
min task run agent-verifyexited 101 under rustc 1.98.1, which the repository does not pin (1.97.0 in rust-toolchain.toml); the PR is not a draft on that alone — CI, which honours the pin, is the gate.Deferred
The plan marked these test lines deferred: the executor lacks the lane, so nothing here ran them and they still owe evidence.
cargo nextest run -p minvmd guest_kernel_accepts_rendered_rulesetCloses #2384