Repository navigation
T19 — Take the network mode and egress keys in their canonical shape - #3106
minimal-foundry[bot] wants to merge 7 commits into
Conversation
Rewrite legacy [network] mode spellings and flat egress keys to their current form, with one hint each, and refuse mode none with egress (BOX-079, BOX-080, BOX-157).
The --network flag now prints the hint that [network] mode prints for the same legacy spelling (BOX-079).
The create request carries [network] egress, ingress, bep and [secrets] to the daemon key for key (BOX-080).
|
Important Review skippedBot user detected. To trigger a single review, invoke the
 |
|
Reviewed: 1 finding, none blocking at Address the comments on the diff. Clobber: none seen. |
| pub fn canonicalize(spec: &mut toml::Table) -> Result<Vec<String>, NetworkError> { | ||
| let mut hints = Vec::new(); | ||
| let Some(toml::Value::Table(network)) = spec.get_mut("network") else { | ||
| return Ok(hints); | ||
| }; |
There was a problem hiding this comment.
MEDIUM · correctness · confidence 0.70 · in canonicalize
Nothing outside tests calls canonicalize. A real minimal.toml with mode = "no-net" or a flat egress_allow_* key still reaches the daemon in legacy form with no hint, and mode = "none" with egress is not refused. The PR proof says an old spelling hints and stores the canonical value. Call it after expand on the create path and print the hints.
What the reviewer checked
rg 'network::canonicalize' finds only crates/minimal/src/cli.rs test; the diff does not touch expand.rs or any create path.
Hand this to an agent
Act on a code-review finding.
File: crates/mfile/src/network.rs
Lines: 132-136
Category: correctness
Severity: medium (reviewer confidence 0.70)
Finding:
Nothing outside tests calls `canonicalize`. A real `minimal.toml` with `mode = "no-net"` or a flat `egress_allow_*` key still reaches the daemon in legacy form with no hint, and `mode = "none"` with egress is not refused. The PR proof says an old spelling hints and stores the canonical value. Call it after `expand` on the create path and print the hints.
First verify this against the code yourself. It is model output and
may be wrong. If it is wrong, or the tradeoff was deliberate, say so
and change nothing.
👍 useful · 👎 noise — react to tell the reviewer. Replying works too, and says more.
There was a problem hiding this comment.
Disputed — not addressed: On this branch mfile::expand::expand has no caller outside tests, so no create path exists to call canonicalize after it. Wiring expansion into box creation needs files outside this task's layers (expand.rs is consumed, not changed). canonicalize is the step that path will call. Its hints and the exit-3 refusal are proved by the mfile tests.
There was a problem hiding this comment.
arch-review: CONDITIONS · 2 conditions · the full ruling
The mode rewrite, the flat egress keys and the mode = "none" refusal conform to the Box Spec. The ingress check does not. canonicalize refuses a list network.ingress, but architecture.md line 892, box.toml line 83 and example-minimal.toml line 105 write ingress as a list of tables. The design's own example therefore fails with exit 3. C1 fixes the check and the two test fixtures. C2 adds issues for the two residuals that the PR notes describe: the caller that prints the table hints, and the fold of NetworkError into mfile::Error.
Ruled at 5a90544 against gominimal/arch@7a86ec7bb3c0 by claude-opus-5-5.
| hints.extend(flat_egress_hint(flat)); | ||
| } | ||
|
|
||
| for section in ["egress", "ingress", "bep"] { |
There was a problem hiding this comment.
C1 · This loop refuses ingress when it is not a table. architecture.md line 892 and box.toml line 83 write ingress = [ { proto = "tcp", port = 8080 } ], a list of tables. Check ingress as a list, and refuse a table with expected: "a list". Keep the table check for egress and bep.
There was a problem hiding this comment.
Addressed in aac3341fc28d: canonicalize now requires network.ingress to be a list and refuses a table with expected "a list". egress and bep still require a table. crates/mfile/src/spec.rs confirms that box.toml writes ingress as a [{proto, port}] list.
| egress_deny_subnets = [\"10.1.0.0/16\"]\n\ | ||
| [sessions.dev.network.egress]\n\ | ||
| allow_dns_hosts = [\"b.example\", \"a.example\"]\n\ | ||
| [sessions.dev.network.ingress]\n\ |
There was a problem hiding this comment.
C1 · This fixture writes ingress as a table with ports. The design does not use that shape. Write ingress = [{ proto = "tcp", port = 8080 }] under [sessions.dev.network], and add a case where a table ingress exits 3 with network.ingress.
There was a problem hiding this comment.
Addressed in aac3341fc28d: The fixture now writes ingress = [{ proto = "tcp", port = 8080 }] under [sessions.dev.network]. A new case shows that a table ingress exits 3 with WrongShape naming network.ingress.
| "allow_subnets": ["10.0.0.0/8"], | ||
| "deny_subnets": ["10.1.0.0/16"], | ||
| }, | ||
| "ingress": {"ports": ["8080:80/tcp"]}, |
There was a problem hiding this comment.
C1 · Use the Box Spec shape here: "ingress": [{"proto": "tcp", "port": 8080}]. The test then proves the pass-through for the shape that box.toml line 83 writes.
There was a problem hiding this comment.
Addressed in aac3341fc28d: The pass-through test now sends "ingress": [{"proto": "tcp", "port": 8080}], the box.toml shape.
| /// | ||
| /// A [`NetworkError`] for a mode that is not a mode, a value of the wrong | ||
| /// shape, or `mode = "none"` with an egress key. | ||
| pub fn canonicalize(spec: &mut toml::Table) -> Result<Vec<String>, NetworkError> { |
There was a problem hiding this comment.
C2 · No caller prints the hints that this function returns, because nothing outside mfile calls expand::expand. Add a residual with a linked issue to the PR body for the caller that prints each hint and exits 3 on a NetworkError.
There was a problem hiding this comment.
Addressed in aac3341fc28d: No code change. The caller that prints each hint and exits 3 on a NetworkError is in crates/minimal, outside this task's layers. I recorded it as a residual in this round's notes for the PR body. The linked issue needs a person to file it.
|
Round 1 of 10 on the runtime's review findings: the coder changed nothing. Revise round 1. No code change this round. The one thread is invalid: mfile::expand::expand itself has no caller outside tests, so no create path exists yet to call canonicalize after expand. Wiring one needs files outside this task's layers (expand.rs is a consumes, and the create path in minimal or minimald is not named). That belongs to the task that wires expansion into box creation. I did not re-run the minimal and minimald-rpc test lines because nothing changed since the round that passed them. Review threads the coder judged:
|
…etwork-mode-and-egress-keys-in-their-c
The e2e checks for 'note: --network <legacy>'. box.toml writes ingress as a list of tables, so a table ingress now exits 3.
| pub fn canonicalize(spec: &mut toml::Table) -> Result<Vec<String>, NetworkError> { | ||
| let mut hints = Vec::new(); |
There was a problem hiding this comment.
LOW · correctness · confidence 0.55 · in canonicalize
canonicalize has no caller outside tests. A real [network] mode = "no-net" entry is not rewritten, and no hint prints for it. The PR proof says an old spelling hints and stores the canonical value. The new mode = none with egress refusal also never runs. Call canonicalize after expand in the production path, or state that a later task wires it.
What the reviewer checked
rg 'canonicalize(&mut' finds only network.rs:240 (test helper) and cli.rs:1405 (test).
Hand this to an agent
Act on a code-review finding.
File: crates/mfile/src/network.rs
Lines: 133-134
Category: correctness
Severity: low (reviewer confidence 0.55)
Finding:
`canonicalize` has no caller outside tests. A real `[network] mode = "no-net"` entry is not rewritten, and no hint prints for it. The PR proof says an old spelling hints and stores the canonical value. The new `mode = none` with egress refusal also never runs. Call `canonicalize` after `expand` in the production path, or state that a later task wires it.
First verify this against the code yourself. It is model output and
may be wrong. If it is wrong, or the tradeoff was deliberate, say so
and change nothing.
👍 useful · 👎 noise — react to tell the reviewer. Replying works too, and says more.
There was a problem hiding this comment.
arch-review: PASS · the full ruling
The change now conforms to the Box Spec [network] block. canonicalize takes ingress as a list, as architecture.md line 892 writes it, and refuses a table with exit 3. The legacy mode hint starts with note: --network <legacy> again, and the e2e pins match it. The modes, the flat egress keys and the mode = "none" refusal agree with lines 889 and 916–919. Before a client calls expand, make sure that the same caller also runs canonicalize.
Ruled at aac3341 against gominimal/arch@7a86ec7bb3c0 by claude-opus-5-5.
|
Round 2 of 10 on a failed check suite, pushed as
Verify Review threads the coder judged:
A legacy network spelling, as the --network flag or as mode under [network] in minimal.toml, prints one hint that names the current spelling. The expanded spec stores only the current value. The flag hint again starts with 'note: --network ' and names '--network ', which is the text the session e2e checks for. Notes from the agent: Failed check native-daemon-e2e: the shared hint from round 1 started with 'note: network mode no-net'. scripts/session-e2e.sh (NET-037) matches 'note: --network no-net' ... '--network none'. The hint now reads: 'note: --network and mode = "" still work for one release. The current spelling is --network , or mode = "" under [network] in minimal.toml.' I did not run the e2e on this box. I checked the new text against the script's case patterns by reading them. ci-linux-native-success is the lane aggregator, so it failed because the e2e failed. I also ran legacy_network_spellings_parse_with_hint in crates/minimal/src/tests.rs, which pins the flag's stderr, and it passed. Residual for the PR body (from review thread C2): no caller outside mfile calls expand::expand and network::canonicalize yet. A follow-up task must wire the create path in crates/minimal to print each hint and to exit 3 on a NetworkError. That file is outside this task's layers. I cannot open a linked issue, because I have no credential. A person must file the issue. |
T19 — Take the network mode and egress keys in their canonical shape
slice: S4
kind: task
covers: BOX-079, BOX-080, BOX-157
size: L
est: 280 code, 300 tests
executor: agent
difficulty: routine
model: standard
diagnostics: one hint per legacy spelling
observability: none: expansion emits only hints
layers:
- network:
- crates/mfile/src/network.rs (new): modes, spellings, flat keys
- crates/mfile/src/lib.rs: mod line
test: cargo nextest run -p mfile network_mode_legacy_spelling_hint_and_canonical_value
test: cargo nextest run -p mfile mode_none_with_egress_exit3
test: cargo nextest run -p mfile flat_egress_keys_hint_and_canonical_nested_keys
- flag:
- crates/minimal/src/cli.rs: shared hint
test: cargo nextest run -p minimal network_flag_and_table_share_one_hint
- wire:
- crates/minimald-rpc/src/lib.rs: sections pass unchanged
- crates/minimald-rpc/src/tests.rs: tests
test: cargo nextest run -p minimald-rpc network_bep_secrets_passed_unchanged
produces: none
consumes:
- crates/mfile/src/expand.rs
proof: An old network spelling hints and stores the canonical value.
depends on:
- blocked by #2410
Proof
An entry that writes [network] mode = "no-net", "host-net" or "own-ip" expands, and mfile::network::canonicalize returns one hint that names the current value. The expanded spec and its bytes then carry none, host_ip or own_ip, never the legacy spelling. The flat egress_allow_dns, egress_allow_subnets and egress_deny_subnets keys get one hint each and are stored as egress.allow_dns_hosts, egress.allow_subnets and egress.deny_subnets. mode = "none" with any egress key fails with exit code 3. min --network prints the same hint line as the minimal.toml table.
Notes from the agent: Nothing calls expand::expand outside mfile yet. So canonicalize is a public function that the caller runs on Expansion::spec after expand, and the caller prints the hints it returns. The task names expand.rs as consumes, so expand.rs is not changed. The mode-none error and the shape errors use a new NetworkError type in network.rs (exit_code() returns 3), because error.rs is not in the layers. A later task can fold it into mfile::Error. Printed text changed: the --network legacy hint is now 'note: network mode still works for one release. The current spelling is : --network , or mode = "" under [network] in minimal.toml.' The existing test legacy_network_spellings_parse_with_hint in crates/minimal/src/tests.rs checks only that the hint names both spellings, and it still passes. The new minimal test is in a #[cfg(test)] module in cli.rs, so tests.rs is not changed. The minimald-rpc lib.rs change is a doc comment only, because the request already carries the spec JSON whole. The grace-release exit 3 for legacy spellings (the BOX-050 child) is not in this task.
Tests
cargo nextest run -p mfile network_mode_legacy_spelling_hint_and_canonical_valuecargo nextest run -p mfile mode_none_with_egress_exit3cargo nextest run -p mfile flat_egress_keys_hint_and_canonical_nested_keyscargo nextest run -p minimal network_flag_and_table_share_one_hintcargo nextest run -p minimald-rpc network_bep_secrets_passed_unchangedVerify
min task run agent-verifyexited 100 under rustc 1.99.0, which the repository does not pin (1.99.1 in rust-toolchain.toml); the PR is not a draft on that alone — CI, which honours the pin, is the gate.Closes #2414