Repository navigation
T1 — Give every record its type, parent and project root at creation - #2521
mitodrummer merged 14 commits into
Conversation
A record written before the fields existed still loads with all three absent. The store can now file a record under an id its caller minted.
The store mints each box id and refuses one a retained record already names. The create span and log carry the id, type and parent.
…-record-its-type-parent-and-project-ro
|
Important Review skippedBot user detected. To trigger a single review, invoke the
 |
|
Round 1 of 10 on a failed check suite, pushed as
Verify Outside the task's
Converted to a draft: something above needs a person before review. A new session's record shows its id, type, parent and project root. Records written before this change still load with those fields empty. The boxes crate now passes clippy with -D warnings, so the clippy check can go green. Notes from the agent: The clippy failure was clippy::similar_names in the boxes test records_without_the_new_fields_still_load. The bindings |
|
Approved: every review thread resolved at @mitodrummer 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 · 3 conditions · the full ruling
The id, the collision refusal and the parent of none conform. Two stamped fields do not. min task run creates through CreateSession too, so each task box is recorded as session. Design principle 8 (architecture line 334) filters the min task verbs by that type. C1 fixes this. project_root is the working path, not the directory of the minimal.toml that the client found. Line 21 and line 485 require the project root. C2 fixes this. C3 moves the id check into DiskLoader::create_with_id, so the check fails closed for every caller.
Ruled at e37dcb2 against gominimal/arch@109541a48c31 by claude-opus-5-5.
|
/revise Apply the three conditions from the C1. Stamp the type from the creating verb.
C2. Stamp
C3. Hold id uniqueness in the public filing function.
Keep |
…-record-its-type-parent-and-project-ro
create_with_id now refuses an id the index already holds, with a kind other than AlreadyExists. BoxConfig gains creating_verb and project_root, and boxes gains TASK_BOX_TYPE.
A task run's box is now recorded as task. The record's project root is the root the client resolved, or the project path for an older client. The box.create span records the stamped type.
min task run sends the task verb and min session activate the session verb. Both send the project's root, resolved also under --sync none.
There was a problem hiding this comment.
arch-review: CONDITIONS · 1 condition · the full ruling
The change conforms to BOX-001 for the id, the type and the parent. The CLI and task paths record the correct project root. The dashboard create in crates/minimal/src/dash/rpc.rs activate() sends no project_root. The daemon then records the form's working path, and that path can be a subdirectory. architecture.md line 21 says the Box Record holds the project root its entry came from. Make the change in C1, and the change conforms.
Outside the diff
crates/minimal/src/dash/rpc.rs:522: C1 · The dashboard create does not sendconfig.project_root, so the daemon recordsproject_pathas the root. architecture.md line 21 says the record holds the project root its entry came from. Callcrate::stamp_project_root(&mut request, project_path.as_path())?here. Add a test for a form path<root>/sub.
Ruled at c657102 against gominimal/arch@109541a48c31 by claude-opus-5-5.
|
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:
A new box's record now shows its UUIDv7 id. It also shows its type: session for min session activate, task for min task run. Its parent is none, and its project root is the directory that holds minimal.toml, also when the command runs from a subdirectory. Notes from the agent: Round 2 applies C1, C2 and C3. Files outside the layers: crates/minimald-rpc/src/lib.rs, crates/minimal/src/task.rs and crates/minimal/src/cmd/boxes/create.rs (the maintainer accepted these). crates/minimald-rpc/src/tests.rs and crates/minimald/src/box_manager/tests.rs fill the new BoxConfig fields in struct literals, so the crates compile. crates/minimald/src/box_actor/host.rs fills box_type, parent and project_root in a Record literal that the merge from main brought in (#2570). Without that, the minimald test build failed. C2: under --sync none, a broken minimal.toml in a parent directory now stops activate before the daemon connection, with the existing resolve_upload_root message. The dashboard create path (crates/minimal/src/dash/rpc.rs) does not send project_root yet, so the daemon records project_path for it. It does send the session verb by default. C3: create_with_id refuses a held id with ErrorKind::Other and writes a warn! log. Store::create in minimald keeps its own check, warn! and message. clippy shows only the existing must_use warnings in crates/boxes/src/core. I did not run just clippy-strict. |
|
/revise Two fixes on c657102. 1. The dashboard's create also stamps 2. Fix the failing Add Run |
…-record-its-type-parent-and-project-ro
The dashboard's activate now stamps the directory that holds the project's minimal.toml, as min session activate and min task run do. The create-site scan lists the two unit-test requests that are never sent.
|
Round 3 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:
A new box record shows its id, type, parent and project root. A box created from the dashboard in a subdirectory now records the directory that holds minimal.toml, the same root that min session activate and min task run record. Notes from the agent: Round 3. Fix 1: dash::rpc activate() now calls crate::stamp_project_root after request.config.name = name. The new test dash::rpc::tests::dashboard_create_from_a_subdirectory_sends_the_project_root checks that a request for /sub carries project_root . The project_path fallback in minimald build_record() stays for older clients. Fix 2: crates/minimal/src/tests.rs is outside the layers. The maintainer accepted this edit in the directive. It adds the two unit-test create sites to the every_create_session_asserts_the_daemon_build list, each with a comment. The stamp in activate() reads minimal.toml on the async worker, as activate_box does. The later upload-root walk in activate() stays in spawn_blocking. clippy -p minimal --all-targets shows only warnings that were already in boxes/src/core files that this branch does not change. |
There was a problem hiding this comment.
arch-review: PASS · the full ruling
The change conforms to the design at this head. The dashboard create now sends the project root, as min session activate and min task run do. Thus every new record holds the project root that architecture.md line 21 and line 1117 define. The id, the type and the parent agree with line 495 and with Gatehouse §5.2. Issue #2397 owns the VM record key.
Ruled at 64cf5ad against gominimal/arch@109541a48c31 by claude-opus-5-5.
The project root is only metadata under --sync none, so a failed lookup records the directory instead of failing the activation (#770). Tarball sync reuses the root it already resolved.
There was a problem hiding this comment.
arch-review: CONDITIONS · 1 condition · the full ruling
The new fallback in activation_project_root() records the activation directory as the project root when the found minimal.toml does not load. Section "minimal.toml layout" (line 1117) makes the CWD the project root only when no file is found. The previous head refused this case. CheckProject already refuses a broken minimal.toml under every --sync mode before this step, so the fallback does not help #770. C1 restores the refusal, keeps the upload_root reuse, and replaces the test.
Ruled at a8fd643 against gominimal/arch@109541a48c31 by claude-opus-5-5.
|
Round 4 of 10 on the runtime's review findings, pushed as
Verify Outside the task's
Converted to a draft: something above needs a person before review. Review threads the coder judged:
A new session's record shows its UUIDv7 id, its type (session or task, from the creating verb), its parent (none outside a box) and the project root that holds its minimal.toml. Notes from the agent: Round 4 changes only crates/minimal/src/cmd/boxes/create.rs. That file is outside the task's layers. An earlier round touched it to send the project root from the CLI create path. The fix splits stamp_project_root into a set_project_root helper and adds activation_project_root. Tarball sync reuses the root it already resolved. --sync none falls back to the directory on a failed lookup and logs it at debug. The new test is sync_none_records_the_directory_when_minimal_toml_is_broken. The task and dashboard paths still call stamp_project_root and still fail on a broken minimal.toml. Those paths always resolve the upload root, so this round leaves them unchanged. cargo clippy -p minimal --all-targets reports only pedantic must_use warnings in boxes, and this round did not touch boxes. |
|
/revise Two fixes on a8fd643. 1. arch-review C1: restore the refusal in
2. minimal-review, dash/rpc.rs:526: no blocking walk on the async worker. Run the plan's test lines plus |
CheckProject already refuses a minimal.toml that does not load under every --sync mode, so the --sync none fallback could only fail open. The dashboard now resolves the project root once, off the async worker, and reuses it as the upload root.
There was a problem hiding this comment.
arch-review: PASS · the full ruling
This head removes the --sync none fallback that recorded the directory as the project root on a broken minimal.toml. The lookup now refuses that file, and CheckProject already refuses it earlier. Thus the record always holds the root its entry came from (architecture.md line 21). The dashboard resolves the root once, before the create. The rest of the change keeps the PASS from 64cf5ad3.
Ruled at 31fac89 against gominimal/arch@109541a48c31 by claude-opus-5-5.
There was a problem hiding this comment.
minimal-review — approval withheld
Verdict COMMENT — APPROVE withheld: 7 unresolved threads — minimal-arch[bot] crates/minimald/src/box_manager.rs, minimal-arch[bot] crates/minimald/src/box_manager.rs, minimal-arch[bot] crates/minimald/src/box_manager.rs, minimal-arch[bot] crates/boxes/src/store.rs:873, minimal-arch[bot] crates/minimal/src/cmd/boxes/create.rs, …. 1 finding, 1 inline.
Panel: 3 of 3 personas ran (correctness, security, supply-chain).
|
Round 5 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:
A new session's record shows its id, type, parent and project root. The project root is the directory that holds minimal.toml, from Notes from the agent: crates/minimal/src/cmd/boxes/create.rs is not in the task's layers. An earlier round changed it, and this round's directive asks for the change there. activation_project_root now returns Result and propagates the resolve_upload_root error. The --sync none fallback and its debug log are gone. The dashboard's activate() resolves the upload root and the VCS-root check once, in one spawn_blocking before the create. It stamps project_root from that result and reuses it for the upload. The not-a-repository refusal still happens after the create, at the same point as before. The minimald test lines ran together in one nextest command. |
There was a problem hiding this comment.
minimal-review — approved
Verdict APPROVE — every review thread is resolved; @mitodrummer closed the last one. The review of 31fac89408b2 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.
There was a problem hiding this comment.
Babysit review on 31fac89 against T1's plan block, extended by the arch conditions.
- Every create stamps the record: id (UUIDv7, refused when the store already holds it, in
DiskLoader::create_with_id),box_typefrom the creating verb (sessionfor activate and the dashboard,taskformin task run),parentnone from the daemon (never from the request), andproject_rootas the directory that holdsminimal.toml. - Each create path resolves that root: activate reuses the tarball upload root. The dashboard resolves it once off the async worker, and
min task runresolves it too. - Old records: they still load, with the new fields empty.
- Broken
minimal.toml:CheckProjectrefuses it before any root lookup, so the lookup propagates its error and never falls back. - Edits outside the layers: the second and third create paths, the version-gate scan list, and test fixtures that gain the new fields. A maintainer accepted them.
- arch-review: PASS on this head.
T1 — Give every record its type, parent and project root at creation
slice: S0
kind: task
covers: BOX-001
size: L
est: 260 code, 300 tests
executor: agent
difficulty: needs care
model: strongest
diagnostics: daemon log names id, type, parent, collisions
observability: create span carries id, type, parent
layers:
- record:
- crates/boxes/src/lib.rs: type, parent, ending fields
- crates/boxes/src/store.rs: new fields
test: cargo nextest run -p boxes records_without_the_new_fields_still_load
- daemon:
- crates/minimald/src/store.rs: refuses a colliding id
- crates/minimald/src/box_manager.rs: stamps type, parent, project root
- crates/minimald/src/exec/tests.rs: new fields
test: cargo nextest run -p minimald create_assigns_uuidv7_id_type_and_parent
test: cargo nextest run -p minimald colliding_box_id_refuses_creation
test: cargo nextest run -p minimald create_records_project_root
- clients:
- crates/minimal-client/src/box_registration.rs: new fields
- crates/minimal/src/dash/rpc.rs: new fields
- crates/minimal/src/dash/render.rs: new fields
- crates/minimal/tests/dash_snapshots.rs: new fields
- crates/minimal/src/cmd/boxes/attach.rs: new fields
test: cargo nextest run -p minimal cli_attach_resumes_the_box_through_the_shared_path
produces: none
consumes: none
proof: A new session's record shows id, type, parent, project.
depends on:
- blocked by #2385
Proof
After
min session activate, the new session's record.json under the daemon's sessions directory shows a UUIDv7id,box_type: "session",parent: nullandproject_rootset to the project directory. A record written before this change still loads, and these three fields are absent from it.Notes from the agent: Outside the layers: two Record struct literals do not compile without the new fields, so I changed them. One is the test module of crates/minimald/src/box_runtime.rs. The other is crates/minimal/src/cmd/boxes/vm_host.rs, which holds the cli_attach_resumes_the_box_through_the_shared_path test. crates/minimal/src/cmd/boxes/attach.rs builds no Record and needed no change. The task's layer note names 'ending fields' for crates/boxes/src/lib.rs. I did not add reason, exit_code or signal: BOX-011 defines them, the task covers only BOX-001, and nothing in this task sets them. box_type, parent and project_root are Option, so a record that predates them loads as None. BOX-003 migration is not in this task. A new record writes parent as null, so a later migration can tell it apart from an old record that has no key. The daemon store now mints the id itself, through a seam (Store::init_with_mint) that tests use. It refuses a minted id that a retained record names, and it logs the collision. boxes::DiskLoader gains create_with_id for this. No caller identity from inside a box exists in this tree, so create_session stamps parent None (BOX-140). The only create path is CreateSession, so box_type is always "session". The BOX-001 tests drive BoxManagerHandle::create_session in a new inline test module in box_manager.rs, because box_manager/tests.rs is not in the layers. The new refusal message says: the new session's id is already in use by another session. No session was created. Run the command again. I did not run just ci or a workspace build. I ran clippy for boxes, minimald, minimal and minimal-client, and it reported no new warnings.
Tests
cargo nextest run -p boxes records_without_the_new_fields_still_loadcargo nextest run -p minimald create_assigns_uuidv7_id_type_and_parentcargo nextest run -p minimald colliding_box_id_refuses_creationcargo nextest run -p minimald create_records_project_rootcargo nextest run -p minimal cli_attach_resumes_the_box_through_the_shared_pathVerify
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.Base
The base branch moved while the coder worked; it was merged in before the tests ran.
Out of scope
These files are outside the task's
layers:, so this PR is a draft:crates/minimal/src/cmd/boxes/vm_host.rscrates/minimald/src/box_runtime.rsCloses #2386
Undrafted 2026-10-25. The two edits outside the layers are test fixtures only. The
Recordliterals incrates/minimal/src/cmd/boxes/vm_host.rs(tests) andcrates/minimald/src/box_runtime.rs(proxy_env_tests) gainbox_type: None, parent: None, project_root: Noneso they compile with the new fields. A maintainer accepted them.Undrafted again 2026-10-26. The edits outside the layers are the ones accepted for the arch conditions: the creating verb and project root on both create paths (
minimald-rpc,task.rs,cmd/boxes/create.rs), the version-gate scan-list entries incrates/minimal/src/tests.rs, and test-fixtureRecordliterals that gain the newNonefields (vm_host.rs,box_runtime.rs,box_actor/host.rs).Undrafted again 2026-10-26 after round 4: the only new file in the list,
crates/minimald/src/box_manager/tests.rs, is a testBoxConfigliteral that gainscreating_verbandproject_root. Its other files are as listed above.