Repository navigation
T23 — Check type constraints last, and let a derived type only narrow - #3292
mitodrummer merged 8 commits into
Conversation
A type's constraints are a check, never a layer. check_constraints reads a finished expansion, after the override layer, and fails with exit 3 naming the type, the key and the layer that set the value (BOX-061). It never changes the spec, so a constraint never adds a package, a port or an egress entry (BOX-062). Type resolution fails with exit 3 naming the type, the key and the parent when a derived type's constraint admits a value its parent's excludes (BOX-156).
|
Important Review skippedBot user detected. To trigger a single review, invoke the
 |
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Title check |  |
This assessment has not completed. The check will be retried on the next pre-merge check run. | |
| Description check | |
This assessment has not completed. The check will be retried on the next pre-merge check run. |
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
|
Reviewed: no findings; approval withheld, 11 threads unresolved at APPROVE withheld: 11 unresolved threads — minimal-arch[bot] crates/mfile/src/box_types.rs:413, minimal-arch[bot] crates/mfile/src/expand.rs, minimal-arch[bot] crates/mfile/src/expand.rs:319, minimal-arch[bot] crates/mfile/src/box_types.rs:413, minimal-arch[bot] crates/mfile/src/expand.rs, …. Resolving the last thread releases the approval on this head. Resolve the open threads. Resolving the last one releases the approval on this head; no new review is required. Clobber: none seen. |
There was a problem hiding this comment.
arch-review: CONDITIONS · 3 conditions · the full ruling
The checks are in the right crate, and BOX-062 conforms. The built-in roots carry no constraints, so a type that extends task can allow pty_enabled = true. Line 1264 of the design refuses that. Make the change in C1. check_constraints skips a key the spec does not set, but the product then applies the root default. Make the change in C2. No product path calls the check yet. Name the issue for that work in the PR body, as C3 says.
Ruled at a50eb1b against gominimal/arch@15b603e6afb8 by claude-opus-5-5.
| defaults.push(((*t).to_owned(), project.defaults.clone())); | ||
| constraints.push(((*t).to_owned(), project.constraints.clone())); | ||
| } | ||
| check_narrowing(&constraints)?; |
There was a problem hiding this comment.
C1 · The chain starts with an empty table for the built-in root, so this check never sees the root's constraints. A type with extends = "task" and constraints = { pty_enabled = true } resolves. Seed the root's table from the design table, lines 1212-1220, as C1 says.
There was a problem hiding this comment.
Addressed in 70864457b8ee: BoxTypes::resolve seeds the root entry with builtin_constraints(root), so extends = "task" with constraints = { pty_enabled = true } now fails with Loosening naming parent task.
| ) -> Result<(), BoxTypeError> { | ||
| for (_, constraints) in &box_type.constraints { | ||
| for (key, constraint) in constraints { | ||
| let Some(value) = expansion.spec.get(key) else { |
There was a problem hiding this comment.
C2 · An absent key passes here, but the product then applies the built-in root default for it. A type that extends session with pty_enabled = false still gets a PTY. Check the root default for the key, as C2 says (lines 1234 and 1281).
There was a problem hiding this comment.
Addressed in 70864457b8ee: Seeded session with pty_enabled = true, so a type that extends session with pty_enabled = false now fails at resolution as Loosening. A root default for a key that the root does not constrain is still not checked, because the root defaults table is empty in this tree.
| /// [`BoxTypeError::ConstraintBroken`] for the first value that a | ||
| /// constraint excludes. It names `box_type`, the key and the layer that | ||
| /// set the value, and exits 3. | ||
| pub fn check_constraints( |
There was a problem hiding this comment.
C3 · No product path calls this function, and expand does not call it. Name the issue that adds the call after each expand on the create path in the PR body, as C3 says.
There was a problem hiding this comment.
Addressed in 70864457b8ee: No code change. The residual is #3295, which adds the call after each expand in crates/minimal/src/cmd/boxes/create.rs. It is listed in the notes for the PR body.
check_constraints named the entry's own type for every failure, even when the failing constraint came from a parent type. Check the entry's type first and name the type whose constraints table failed.
|
Round 1 of 10 on the runtime's review findings, pushed as
Verify Review threads the coder judged:
An entry that breaks a constraint of its type fails with exit 3. The message names the layer that set the value, the key, and the type whose constraints table holds the broken constraint. When a derived type narrows a parent, the message names the derived type. Notes from the agent: Revise round 1. check_constraints now checks the chain from the entry's own type to the root, and it names the type whose table holds the failing constraint. A type only narrows its parent (BOX-156), so the first failure is the tightest constraint. The existing network.mode case in the test named ci-small for a constraint that only ci holds. It now expects ci. A new entry, small-always, pins the narrowed case. |
There was a problem hiding this comment.
minimal-review — approval withheld
Verdict COMMENT — APPROVE withheld: 4 unresolved threads — minimal-foundry[bot] crates/mfile/src/expand.rs, minimal-arch[bot] crates/mfile/src/box_types.rs:391, minimal-arch[bot] crates/mfile/src/expand.rs:312, minimal-arch[bot] crates/mfile/src/expand.rs:305. No findings.
Panel: 3 of 3 personas ran (correctness, security, supply-chain).
There was a problem hiding this comment.
arch-review: CONDITIONS · 3 conditions · the full ruling
The check is in the right place and never writes the spec. Three gaps remain. The built-in types carry no constraints, so a task entry with pty_enabled = true passes. A project type that loosens task also passes (lines 1212-1216 and 1264, C2). A constraint on pty_enabled or restart misses the same setting written as io.pty_enabled or execution.restart, so an entry can override it (line 1288, C3). No product path calls check_constraints, and the PR names no issue for that work (line 1234, C1).
Ruled at 24d497f against gominimal/arch@15b603e6afb8 by claude-opus-5-5.
| defaults.push(((*t).to_owned(), project.defaults.clone())); | ||
| constraints.push(((*t).to_owned(), project.constraints.clone())); | ||
| } | ||
| check_narrowing(&constraints)?; |
There was a problem hiding this comment.
C2 · The root entry of constraints (line 385) is an empty table. So this check never compares a project type with its built-in parent, and check_constraints never applies the shipped constraints. Seed the root entry from the design table at lines 1212-1220. For example, task holds pty_enabled = false.
There was a problem hiding this comment.
Addressed in 70864457b8ee: The root entry of constraints is now builtin_constraints(root): task holds pty_enabled = false, and build holds exec_enabled, loadout_mode and nesting.allowed as well.
| ) -> Result<(), BoxTypeError> { | ||
| for (type_name, constraints) in box_type.constraints.iter().rev() { | ||
| for (key, constraint) in constraints { | ||
| let Some(value) = expansion.spec.get(key) else { |
There was a problem hiding this comment.
C3 · This line reads only the flat key. expand also accepts the setting under its section, such as io = { pty_enabled = true } or execution = { restart = "always" }, and the daemon reads io.pty_enabled. A constraint on pty_enabled then passes. Check both forms, with the map from the section tables in spec.rs (line 1288).
There was a problem hiding this comment.
Addressed in 70864457b8ee: check_constraints now checks the flat key and
| /// constraint excludes. It names the type of the chain whose | ||
| /// `constraints` table holds that constraint, the key and the layer that | ||
| /// set the value, and exits 3. | ||
| pub fn check_constraints( |
There was a problem hiding this comment.
C1 · No product path calls this function. Add a residual line to the PR body. It must name the issue that calls check_constraints after each expand in crates/minimal/src/cmd/boxes/create.rs (line 1234).
There was a problem hiding this comment.
Addressed in 70864457b8ee: No code change. The residual is #3295, which calls check_constraints after each expand in crates/minimal/src/cmd/boxes/create.rs. It is listed in the notes for the PR body.
|
/revise Arch conditions (arch_rulings 1145, ruled at 24d497f), in full:
C1: the issue is #3295. Name it in the PR body as a residual. |
Each built-in Box Type now ships its constraints, so a project type cannot loosen them and an entry of the type is checked against them. A constrained setting is checked at its flat key and under its section, with the section map taken from spec.rs.
…-constraints-last-and-let-a-derived-ty
|
Round 2 of 10 on a /revise note, pushed as
Verify Outside the task's
Converted to a draft: something above needs a person before review. Review threads the coder judged:
An entry that breaks a constraint of its type fails with exit 3. The error names the type, the key and the layer that set the value. This includes the constraints that the built-in types ship, for example a [tasks.x] entry with pty_enabled = true, and a value set under its section, such as io = { pty_enabled = true }. Notes from the agent: C1: residual for the PR body: #3295 adds the call to mfile::expand::check_constraints after each mfile::expand::expand in crates/minimal/src/cmd/boxes/create.rs. Until it lands, no product expansion runs the BOX-061 check. I cannot edit the PR body, so the runner or a person must add this line. C2: BoxTypes::resolve seeds the root entry with builtin_constraints(root), from the table that the arch note gives. C3: check_constraints checks each flat setting at its flat key and under its section. canonical_constraints lifts a section-form constraint to its flat key, and check_narrowing uses the same form. Out of layers: crates/mfile/src/spec.rs gets one pub(crate) fn flat_key_section, which reads the SECTIONS table. C3 requires that map from spec.rs and forbids a second copy. Printed text: a Loosening error with a built-in parent now says |
There was a problem hiding this comment.
arch-review: CONDITIONS · 1 condition · the full ruling
The constraint check, the narrowing check and the built-in constraints conform to the design. 0be30fda seeds each (constrains) cell of the shipped-types table (lines 1216-1220), and checks section keys. No command calls check_constraints yet, so the product does not do the check that line 1234 requires after every layer. The PR body calls this later work but names no issue. Do C1: open the issue and add its URL to the PR body as a residual.
Ruled at 7086445 against gominimal/arch@15b603e6afb8 by claude-opus-5-5.
| /// constraint excludes. It names the type of the chain whose | ||
| /// `constraints` table holds that constraint, the key and the layer that | ||
| /// set the value, and exits 3. | ||
| pub fn check_constraints( |
There was a problem hiding this comment.
C1 · This function is the check that line 1234 requires after every layer. No product path calls it. The expand calls in crates/minimal/src/cmd/boxes/create.rs (lines 1345 and 1487) do not run it. Open an issue for that call, and add its URL to the PR body as a residual.
|
/revise Arch condition (arch_rulings 1149, ruled at 7086445), in full:
The issue exists: #3295. Add it to the PR body as a residual of T23. No code change is needed. |
|
Round 3 of 10 on a /revise note: the coder changed nothing. Residual of T23: #3295 . No product path calls mfile::expand::check_constraints yet. The expand calls in crates/minimal/src/cmd/boxes/create.rs (lines 1345 and 1487) must resolve the type, call check_constraints(&entry, &expansion, &resolved) after expand, and exit 3 on Err. That file is outside this task's layers, so issue #3295 owns the call. Add this residual line to the PR body. This round changes no code, as the arch note asks. |
There was a problem hiding this comment.
minimal-review — approval withheld
Verdict COMMENT — APPROVE withheld: 8 unresolved threads — minimal-foundry[bot] crates/mfile/src/expand.rs, minimal-arch[bot] crates/mfile/src/box_types.rs:394, minimal-arch[bot] crates/mfile/src/expand.rs, minimal-arch[bot] crates/mfile/src/expand.rs:307, minimal-arch[bot] crates/mfile/src/box_types.rs:394, …. No findings.
Panel: 3 of 3 personas ran (correctness, security, supply-chain).
|
/revise Arch condition (arch_rulings 1149, ruled at 7086445), in full:
The PR body now names #3295 as a residual. Arch has not re-ruled, because no commit followed. Make the change in the code too: on |
|
That What the platform saidcould not start the coder: Error: there is no container instance that can be provided to this durable object Nothing to type again: the command gets a rocket when its round starts, or a refusal that names the reason. No round was counted for it yet. |
…-constraints-last-and-let-a-derived-ty
#3295 adds the call after each expand on the create path.
|
Round 4 of 10 on a /revise note, pushed as
Verify Outside the task's
Converted to a draft: something above needs a person before review. Review threads the coder judged:
An entry that breaks a constraint of its Box Type fails expansion with exit 3. The error names the type that holds the constraint, the key and the layer that set the value. A derived type that loosens a constraint of its parent fails to resolve. The doc comment on check_constraints now says that no create path calls it yet, and that #3295 adds that call. Notes from the agent: Revise round 4: the only change is the doc-comment sentence that the directive asked for on mfile::expand::check_constraints. The call from crates/minimal/src/cmd/boxes/create.rs is outside this task's layers and is the residual #3295. |
| name: (*name).to_owned(), | ||
| key, | ||
| parent: chain[i.saturating_sub(1)].0.to_owned(), | ||
| value: value.map(|v| inline(&v)), | ||
| }); | ||
| } | ||
| resolved.insert(key, derived); |
There was a problem hiding this comment.
LOW · correctness · confidence 0.50 · in check_narrowing
canonical_constraints can return the same flat key twice for one type, for example pty_enabled = false and io = { pty_enabled = true }. Its doc says each form constrains. check_narrowing keeps only the last one in resolved, so the first form is lost for later types. A conflict between the two forms in one type is also compared as if the earlier form came from the parent. The error then blames the immediate parent type, not the type that holds both forms. Only a contrived constraints table triggers this.
What the reviewer checked
box_types.rs:452 resolved.insert(key, derived); parent: chain[i.saturating_sub(1)].0
Hand this to an agent
Act on a code-review finding.
File: crates/mfile/src/box_types.rs
Lines: 446-452
Category: correctness
Severity: low (reviewer confidence 0.50)
Finding:
`canonical_constraints` can return the same flat key twice for one type, for example `pty_enabled = false` and `io = { pty_enabled = true }`. Its doc says each form constrains. `check_narrowing` keeps only the last one in `resolved`, so the first form is lost for later types. A conflict between the two forms in one type is also compared as if the earlier form came from the parent. The error then blames the immediate parent type, not the type that holds both forms. Only a contrived constraints table triggers this.
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.
Addressed in 0af040b7e1ff: check_narrowing now checks each type only against the types before it, and keeps every form of a key for later types. A new case in derived_type_loosening_constraint_exit3_names_type_key_parent covers a key set flat and under its section.
There was a problem hiding this comment.
arch-review: PASS_WITH_CONDITIONS · 1 condition · the full ruling
The constraint check and the narrowing rule conform to the design. A derived type that loosens a constraint fails with exit 3, and the check never changes the spec. No product path calls check_constraints yet, so the product does not check the result after every layer (line 1234). C1 is that call, and #3295 tracks it. One design question is escalated: what the check does when no layer sets a constrained key. Settle it before #3295 wires the check.
Ruled at b749d1d against gominimal/arch@15b603e6afb8 by claude-opus-5-5.
| /// | ||
| /// The check reads the expansion and never changes it (BOX-062). | ||
| /// | ||
| /// No product path calls this function yet. gominimal/minimal#3295 adds |
There was a problem hiding this comment.
C1 · No product path calls this check. Line 1234 says that type constraints are checked on the result, after every layer. #3295 adds the call after each expand in crates/minimal/src/cmd/boxes/create.rs. This is C1, and it can stay open at merge.
There was a problem hiding this comment.
Disputed — not addressed: The gap is real, but the call belongs in crates/minimal/src/cmd/boxes/create.rs. That file is outside this task's layers. #3295 adds the call after each expand. The doc comment on check_constraints names that issue. No change in this PR.
| places.push((format!("{section}.{key}"), section.to_owned(), nested)); | ||
| } | ||
| for (path, top, value) in places { | ||
| let Some(value) = value else { |
There was a problem hiding this comment.
BOX-061 · An absent key passes here. The root layer of defaults is empty, so a session-rooted type that constrains exec_enabled = false passes for an entry that omits the key. The box then gets true. The design does not say what the check must do here (lines 1251-1253, 1281, 1288). This is escalated, so do not change this line in this PR.
There was a problem hiding this comment.
Disputed — not addressed: The reviewer escalated this and asked for no change in this PR, so the line stays as it is. After the merge, the root defaults layer carries builtin_defaults(root), but that holds only loadout_mode. An entry that omits exec_enabled still passes. The escalation must decide what an absent key does.
…-constraints-last-and-let-a-derived-ty # Conflicts: # crates/mfile/src/box_types.rs # crates/mfile/src/expand.rs
A type that set a key flat and under its section was checked against itself, and only its last form reached the types that extend it. Check each type only against the types before it, and keep every form.
|
A merge of main before round 5, not counted against the cap (1 of 3), pushed as Resolved conflicts in:
Verify Outside the task's
Converted to a draft: something above needs a person before review. Review threads the coder judged:
An entry whose expanded spec breaks a constraint of its Box Type fails check_constraints with exit 3. The error names the type, the key and the layer that set the value. A project type that loosens a constraint of the type it extends fails to resolve, and the error names the type, the key and the parent. Notes from the agent: Merged origin/main and resolved the conflicts in box_types.rs and expand.rs. The root layer of a type chain now takes main's builtin_defaults(root) and this branch's builtin_constraints(root). In expand.rs both sides were kept: check_constraints and main's LoadoutMode gate, and both test blocks. Main now seals task-rooted entries, so the loadout case in expanded_spec_violating_constraint_exit3_names_type_and_layer uses a new fixture entry ok-open with loadout_mode = "open". The merge commit includes main's changes to other crates (minimal, minimald, docs). I did not build those crates. The full mfile suite passes (156 tests). No product path calls check_constraints yet. #3295 tracks that call. |
There was a problem hiding this comment.
arch-review: PASS_WITH_CONDITIONS · 1 condition · the full ruling
The compare conforms. The fix in check_narrowing closes a gap. A type that set a key both flat and under its section passed on only one form. A derived type could then widen past the intersection that Box Types › Composition (line 1264) requires. The merge of main keeps the PR's own change intact. C1 stays open as residual #3295: no create path calls check_constraints yet, so line 1234 is not enforced on product expansions.
Ruled at 0af040b against gominimal/arch@15b603e6afb8 by claude-opus-5-5.
| /// | ||
| /// The check reads the expansion and never changes it (BOX-062). | ||
| /// | ||
| /// No product path calls this function yet. gominimal/minimal#3295 adds |
There was a problem hiding this comment.
There was a problem hiding this comment.
APPROVE withheld: 11 unresolved threads — minimal-arch[bot] crates/mfile/src/box_types.rs:413, minimal-arch[bot] crates/mfile/src/expand.rs, minimal-arch[bot] crates/mfile/src/expand.rs:319, minimal-arch[bot] crates/mfile/src/box_types.rs:413, minimal-arch[bot] crates/mfile/src/expand.rs, ….
minimal-review — advisory review
Verdict COMMENT — No findings.
Panel: 2 of 3 personas ran (correctness, security); did not complete: supply-chain.
T23 — Check type constraints last, and let a derived type only narrow
slice: S5
kind: task
covers: BOX-061, BOX-062, BOX-156
size: L
est: 300 code, 240 tests
executor: agent
difficulty: needs care
model: strongest
diagnostics: error names type, key and layer
observability: none: a pure function, no runtime
layers:
- constraints:
- crates/mfile/src/box_types.rs: narrowing only; never adds
- crates/mfile/src/expand.rs: constraints checked last
test: cargo nextest run -p mfile expanded_spec_violating_constraint_exit3_names_type_and_layer
test: cargo nextest run -p mfile derived_type_loosening_constraint_exit3_names_type_key_parent
test: cargo nextest run -p mfile constraint_never_adds_package_port_or_egress
produces: none
consumes:
- crates/mfile/src/box_types.rs
proof: An entry breaking its type's constraint fails naming the layer.
depends on:
- blocked by #2420
Proof
In mfile, an entry whose expanded value breaks its type's constraints fails with exit 3 after the override layer. The error names the type, the key and the layer, for example: "
[boxes.pty]in minimal.toml setspty_enabledtotrue.[box_types.ci]allows onlyfalseforpty_enabled. Change the value." A derived type that loosens its parent's constraint fails to resolve, and the error names the type, the key and the parent. A constraint never changes the spec.Notes from the agent: Design:
expandreturnscrate::Error, and a new variant needserror.rs, which is outside the layers. So the BOX-061 check is the newmfile::expand::check_constraints(entry, &expansion, &resolved_type). It reads the finished expansion, after the override layer, and returns the newBoxTypeError::ConstraintBroken.expanddoes not call it. No product caller calls it yet. The callers in crates/minimal/src/cmd/boxes/create.rs do not resolve the type or filltype_defaultseither. A later task must callcheck_constraintsafter eachexpand. BOX-156 is checked inBoxTypes::resolve, soBoxTypes::from_filefails on a loosening type (newBoxTypeError::Loosening). Constraint meaning: a list admits each of its items. A table constrains each key it names, where the spec sets that key. Any other value admits only itself. A spec list is checked item by item, and the error names 8000 the layer that added the failing item. An absent key passes. Values compare as TOML values, so"10m"and"600s"are different values for atimeoutconstraint. For a table constraint, a derived type must repeat every key its parent constrains, or it counts as a loosening. The error names the immediate parent type (theextendstarget). Ran the whole mfile suite once at the end: 151 passed.cargo clippy -p mfile --all-targetsis clean. Did not runjust clippy-strictorjust lint-prose.Tests
cargo nextest run -p mfile expanded_spec_violating_constraint_exit3_names_type_and_layercargo nextest run -p mfile derived_type_loosening_constraint_exit3_names_type_key_parentcargo nextest run -p mfile constraint_never_adds_package_port_or_egressVerify
min task run agent-verifypassed ([tasks.verify]).Residual: #3295 calls
mfile::expand::check_constraintsafter eachmfile::expand::expandincrates/minimal/src/cmd/boxes/create.rs. Until it lands, no product expansion runs the BOX-061 check.Closes #2424