Repository navigation
T64 — Carry and validate a box's timeout in its spec, with the exec timeout's grammar moved into mfile - #3122
Conversation
min box exec --timeout now parses through mfile::duration, with the same grammar and refusal text. spec_timeout reads an entry's timeout, and takes zero as no timeout.
The Box Spec reads timeout from [session] or a task through spec_timeout. validate refuses a bad value with exit 3, naming the key and the entry, and the load no longer reports timeout as unknown.
…validate-a-box-s-timeout-in-its-spec-w
|
Important Review skippedBot user detected. To trigger a single review, invoke the
 |
|
Approved: every review thread resolved at @agent-137 resolved the last thread, which releases the approval this review had withheld. Nothing to do. |
There was a problem hiding this comment.
arch-review: CONDITIONS · 2 conditions · the full ruling
The grammar move and the spec field fit the design. Section Box Spec line 1097 makes zero the same as no timeout, and the change does this. Two edits in crates/mfile/src/spec.rs are necessary. The spec writes execution.timeout as integer seconds, but box.toml line 252 writes a string. Add this to the v0 exceptions that #2410 ends (C1). No daemon reads the box timeout yet, and the load does not warn about it now. Link the enforcement issue in the BoxSpec::timeout doc, as hooks_on_resume links #3111 (C2).
Ruled at 1019c22 against gominimal/arch@7a86ec7bb3c0 by claude-opus-5-5.
|
/revise Arch conditions C1 and C2 (arch_rulings 1039), in substance. C1: in |
|
That 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. |
execution.timeout is written as a TOML integer, which is an i64. A duration string past i64::MAX seconds now exits 3 naming the key and the entry, instead of passing validation. The --timeout grammar is unchanged.
There was a problem hiding this comment.
APPROVE withheld: 3 unresolved threads — minimal-foundry[bot] crates/mfile/src/duration.rs:73, minimal-arch[bot] crates/mfile/src/spec.rs:9, minimal-arch[bot] crates/mfile/src/spec.rs:81.
minimal-review — advisory review
Verdict COMMENT — No findings.
Panel: 2 of 3 personas ran (correctness, supply-chain); did not complete: security.
There was a problem hiding this comment.
arch-review: CONDITIONS · 1 condition · the full ruling
The timeout grammar, the flat entry key, the exit-3 refusal and "0" as none all conform to the design. The new i64::MAX bound makes every validated value serializable. One gap remains. The Box Spec writes timeout as a duration string (architecture.md line 1097, box.toml line 252). This change writes execution.timeout as whole seconds. The module doc in crates/mfile/src/spec.rs lists each v0 departure from box.toml that #2410 ends, but this one is not on the list. Add it as C1 states.
Ruled at 78a76ea against gominimal/arch@7a86ec7bb3c0 by claude-opus-5-5.
|
Round 1 of 10 on the runtime's review findings, pushed as
Verify Review threads the coder judged:
An entry's timeout reaches its Box Spec as execution.timeout. A bad value exits 3 and names the key and the entry. That includes a duration string past i64::MAX seconds, which the TOML spec cannot hold. min box exec --timeout takes and refuses exactly what it did before. Notes from the agent: Revise round 1. The cap is in spec_timeout, not in duration_secs. A cap in duration_secs changes what min box exec --timeout takes, and the task keeps that grammar unchanged. The new refusal word is 'a duration that is too long'. |
The spec writes whole seconds, not the box.toml duration string. The timeout doc now says no daemon reads the key yet and links #3114.
There was a problem hiding this comment.
arch-review: PASS · the full ruling
The change conforms to the Box Spec [execution] timeout line (architecture.md line 1097, box.toml line 252) and to the flat entry key (line 1306). A zero timeout is none. A bad value refuses the load with exit 3 before a spec is built. The last commit closes the open items from the ruling on 78a76ea8. It lists the integer-seconds encoding as a v0 exception (#2410). It also states that the daemon does not enforce the box timeout yet (#3114). Both stay open as filed residuals.
Ruled at 900c869 against gominimal/arch@7a86ec7bb3c0 by claude-opus-5-5.
There was a problem hiding this comment.
APPROVE withheld: 4 unresolved threads — minimal-foundry[bot] crates/mfile/src/duration.rs:73, minimal-arch[bot] crates/mfile/src/spec.rs, minimal-arch[bot] crates/mfile/src/spec.rs:84, minimal-arch[bot] crates/mfile/src/spec.rs.
minimal-review — advisory review
Verdict COMMENT — No findings.
Panel: 2 of 3 personas ran (security, supply-chain); did not complete: correctness.
|
Round 2 of 10 on a /revise note, pushed as
Verify Review threads the coder judged:
An entry's timeout reaches its Box Spec as execution.timeout, a bad value exits 3 naming the key and the entry, and min box exec --timeout takes and refuses exactly what it did before. The spec.rs module doc now lists execution.timeout as a v0 exception (whole seconds, key left out for none), and the BoxSpec::timeout doc says the daemon does not read the key yet and links #3114. Notes from the agent: Revise round 2: doc comments only in crates/mfile/src/spec.rs, per arch conditions C1 and C2. The integer serialization is unchanged. For C1 I used the bullet wording from the /revise note, which is the later of the two wordings. Put #3114 (T66, BOX-037) in the PR body as a residual. I ran only one test line this round because the change touches comments only. |
There was a problem hiding this comment.
minimal-review — approved
Verdict APPROVE — every review thread is resolved; @agent-137 closed the last one. The review of 900c869261d0 withheld this approval on the threads open then. Posted as a comment, as the review itself was; the check and the label carry it.
|
/revise Merge |
…validate-a-box-s-timeout-in-its-spec-w # Conflicts: # crates/mfile/src/lib.rs
There was a problem hiding this comment.
arch-review: CONDITIONS · 1 condition · the full ruling
The merge of #3105 makes [defaults.session] the session table, and legacy::settle reads it into file.session. File::check still labels a bad timeout there as "[session] in minimal.toml". A file that writes only [defaults.session] gets a refusal that names a table it does not contain. Line 1173 of the design makes [defaults.session] a layer of its own. Change the label and pin it in a test, as C1 says.
Ruled at 9f37293 against gominimal/arch@7a86ec7bb3c0 by claude-opus-5-5.
|
Round 3 of 10 on a /revise note, pushed as Resolved conflicts in:
Verify After the merge of main, an entry's timeout still reaches its Box Spec as execution.timeout. A bad value still exits 3 and names the key and the entry. min box exec --timeout takes and refuses the same values as before. The legacy-table additions from main are also present. Notes from the agent: Revise round 3: merged origin/main. The only conflict marker was in the File::validate doc comment in crates/mfile/src/lib.rs. The resolution keeps both sides: it lists Error::InteractiveTask from main and Error::InvalidSpecKey from this PR. mod duration, mod legacy, legacy::settle, the timeout check in check() and the warn_unknown_fields change all merged without conflict and are kept. No other change. The other files in the merge commit come from main. |
|
/revise Arch condition C1 (arch_rulings 1046, head 9f37293), in substance. In |
After legacy::settle, file.session holds the [defaults.session] layer, read from that table or from the legacy [session] table. The refusal now names that table, as expand::layer_words does.
There was a problem hiding this comment.
arch-review: PASS · the full ruling
This head conforms. The session timeout refusal now names [defaults.session], the layer that the design gives at line 1251. The [session] table from before reads into the same layer, and its rename hint prints before the refusal. The rest of the change is the same as the PASS on 900c8692. Issue #3114 owns the daemon work that reads execution.timeout.
Ruled at 1cf44c4 against gominimal/arch@7a86ec7bb3c0 by claude-opus-5-5.
There was a problem hiding this comment.
minimal-review — approval withheld
Verdict COMMENT — APPROVE withheld: 2 unresolved threads — minimal-arch[bot] crates/mfile/src/lib.rs, minimal-arch[bot] crates/mfile/src/lib.rs:3238. No findings.
Panel: 3 of 3 personas ran (correctness, security, supply-chain).
There was a problem hiding this comment.
minimal-review — approved
Verdict APPROVE — every review thread is resolved; @agent-137 closed the last one. The review of 1cf44c461732 withheld this approval on the threads open then. Posted as a comment: the App cannot approve its own pull request; the check and the label carry it.
|
Round 4 of 10 on a /revise note, pushed as
Verify Review threads the coder judged:
An entry's timeout reaches its Box Spec as execution.timeout. A bad value exits 3 and names the key and the entry. A bad session timeout now names Notes from the agent: Round 4 makes only the change that condition C1 asks for. The session timeout refusal label in File::check changes to |
T64 — Carry and validate a box's timeout in its spec, with the exec timeout's grammar moved into mfile
slice: S4
kind: task
covers: none
size: M
est: 140 code, 150 tests
executor: agent
difficulty: routine
model: standard
diagnostics: a bad timeout exits 3 naming the key and the entry; the --timeout refusal is unchanged
observability: none: parsing and a spec field
layers:
- parser:
- crates/mfile/src/duration.rs (new): parse_timeout_secs moved from the CLI, grammar and refusal text unchanged; spec_timeout adds 0 as none
- crates/mfile/src/lib.rs: mod line
- crates/minimal/src/cli.rs: --timeout uses mfile's parser; parse_exec_timeout removed
test: cargo nextest run -p mfile duration_takes_seconds_minutes_hours_and_refuses_the_rest
test: cargo nextest run -p mfile spec_timeout_zero_is_none
test: cargo nextest run -p minimal box_exec_timeout_flag_requests_exec_timeout
test: cargo nextest run -p minimal exec_timeout_returns_124_from_exec_and_wait_exec
- spec:
- crates/mfile/src/spec.rs: execution.timeout on BoxSpec, read from the entry through spec_timeout
- crates/mfile/src/lib.rs: validate refuses a bad timeout; timeout is not an unknown key
- crates/mfile/src/expand.rs: timeout takes the duration shape
test: cargo nextest run -p mfile spec_carries_timeout_under_execution
test: cargo nextest run -p mfile invalid_timeout_exit3_names_key_and_entry
test: cargo nextest run -p mfile timeout_entry_key_is_not_reported_unknown
test: cargo nextest run -p mfile invalid_key_exit3_names_key_and_layer
produces:
- crates/mfile/src/duration.rs
consumes:
- crates/mfile/src/spec.rs
proof: An entry's timeout reaches its Box Spec as execution.timeout, a bad value exits 3 naming the key and the entry, and min box exec --timeout takes and refuses exactly what it did before.
depends on:
- blocked by #2415
Proof
A [session] or [tasks.] timeout such as "8h" or 90 now appears in the Box Spec as execution.timeout, in whole seconds. A timeout of zero leaves the key out. A bad value, such as timeout = "2 days" in [tasks.lint], refuses the load with exit 3 and the message "
[tasks.lint]in minimal.toml setstimeoutto a string that is not a duration, buttimeouttakes ... Fix the value." min box exec --timeout uses mfile::duration::parse_timeout_secs, with the same grammar and the same refusal text as before.Notes from the agent: The timeout refusal reuses Error::InvalidSpecKey with WrongShape, because error.rs is not in the layers. The expected-shape words are new text: 'a whole number of seconds, or a string of a whole number followed by s, m or h, such as "30s", "5m" or "1h"'. The expand path (Shape::Duration replaces Shape::StrOrInt, which had no other user) gives the same words. So the existing invalid_key_exit3_names_key_and_layer case for timeout = 1.5 now expects these words, and that test gets a new case for a bad timeout string. expansion_order_and_merge_rules in expand.rs used label strings such as "defaults" as timeout values. The duration shape refuses them, so the test now uses distinct valid durations (1s to 8s). expand::value_words is now pub(crate), so spec_timeout can name a value of the wrong type. BoxSpec::build now takes the entry's extra map in place of hooks_on_resume, to stay inside clippy's argument limit. hooks_on_resume is still reported as an unknown task or [session] field. This task covered only timeout. SPEC_READ_ENTRY_KEYS in lib.rs is the place to add hooks_on_resume. No daemon reads execution.timeout yet. The whole mfile suite passes: 119 of 119.
Tests
cargo nextest run -p mfile duration_takes_seconds_minutes_hours_and_refuses_the_restcargo nextest run -p mfile spec_timeout_zero_is_nonecargo nextest run -p minimal box_exec_timeout_flag_requests_exec_timeoutcargo nextest run -p minimal exec_timeout_returns_124_from_exec_and_wait_execcargo nextest run -p mfile spec_carries_timeout_under_executioncargo nextest run -p mfile invalid_timeout_exit3_names_key_and_entrycargo nextest run -p mfile timeout_entry_key_is_not_reported_unknowncargo nextest run -p mfile invalid_key_exit3_names_key_and_layerVerify
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.Base
The base branch moved while the coder worked and was merged in before the tests ran. These files changed on BOTH sides since the branch point — read them first, because a rewrite from a stale reading can undo a merge without a conflict:
crates/mfile/src/expand.rsResiduals
execution.timeoutyet, so a box runs with no limit until T66 — End an until-complete box at its timeout, execs included, and say so from min task run #3114 (T66, BOX-037) ends it at its timeout (arch C2, arch_rulings 1039).Closes #3110