Repository navigation
T5 — Versioned JSON objects and errors, secrets as references - #2960
mitodrummer merged 4 commits into
Conversation
Add cmd/output.rs: every -o json and -o jsonl object carries its min/v1/<type> schema, a machine-mode failure is one min/v1/error object on stderr with code, message and hint, and a Secret renders only its reference in every output mode. main.rs writes its error object through it, and the root cmd span records the verb, the box id and the outcome.
|
Important Review skippedBot user detected. To trigger a single review, invoke the
 |
|
Approved: every review thread resolved at @mitodrummer resolved the last thread, which releases the approval this review had withheld. Nothing to do. |
…json-objects-and-errors-secrets-as-ref
There was a problem hiding this comment.
arch-review: PASS_WITH_CONDITIONS · 1 condition · the full ruling
< 8000 p dir="auto">The output writer, the stderr error object and theSecret type conform to the output conventions in architecture.md lines 381–384. One document is now wrong. docs/reference/cli-min.md lines 490–491 say that an unspecified error object has no hint. The code now always writes hint, and design line 383 shows it on every error object. C1 updates that sentence. You can merge with C1 open if an issue tracks it. Separately, nothing records box_id on the root span yet. The design does not govern that, but the task promised it, so file an issue for it.
Ruled at 4d4191a against gominimal/arch@7a86ec7bb3c0 by claude-opus-5-5.
record_box_id had no caller, so the root span's box_id stayed empty. Dispatch now records the box the command line names.
There was a problem hiding this comment.
arch-review: CONDITIONS · 1 condition · the full ruling
The new commit records the box argument, as the user typed it, on the root cmd span as box_id. That span exports to the user's collector by default. D9 (architecture.md line 795) says command arguments stay behind a separate flag that is off by default. The field also holds a name or prefix, not the box id that line 381 defines. Apply C1: remove the as-typed record and name an issue for the resolved-id call site. The output writer, the error object and the secret references keep their conforming status from the earlier head.
Ruled at 757d235 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:
Every object written through crates/minimal/src/cmd/output.rs carries its min/v1/ schema. A failure under -o json writes one min/v1/error object to stderr and nothing to stdout. No output mode of the writer prints a secret's value. The cmd span now records the box that the command line names. Notes from the agent: Round 1. The cmd span's box_id now gets the box that the command line names (box show, box audit, session attach/exec/destroy/rename/policy), as the user typed it. It is not always the resolved id. Box resolution happens in attach.rs and box_verbs.rs, which are outside this task's layers, so a later task can call output::record_box_id with the resolved id there. The writer is groundwork: no real command renders through write_object yet. Moving min box show and the other -o json commands onto it needs box_verbs.rs, list.rs and session.rs, which are outside the layers. |
The box_id field held the text the user typed, and for box audit --parent it named a different box. Remove named_box, let a drop guard empty the root-span slot on panic or cancel, and say in the comment that no command records box_id yet.
There was a problem hiding this comment.
minimal-review — approval withheld
Verdict COMMENT — APPROVE withheld: 6 unresolved threads — minimal-arch[bot] crates/minimal/src/main.rs:771, minimal-arch[bot] crates/minimal/src/cmd/mod.rs:60, minimal-arch[bot] crates/minimal/src/cmd/mod.rs, minimal-arch[bot] crates/minimal/src/cmd/mod.rs, minimal-foundry[bot] crates/minimal/src/cmd/mod.rs:287, …. No findings.
Panel: 3 of 3 personas ran (correctness, security, supply-chain).
|
Round 2 of 10 on a failed check suite, pushed as
Verify Review threads the coder judged:
Every Notes from the agent: Round 2. I reverted commit 757d235 (named_box) with a new commit, because three threads showed that it recorded typed text and, for --parent, a different box under box_id. run_in_root now holds the root-span slot in a drop guard, so a panic or a cancelled future empties the slot. The comment on the cmd span now says that box_id stays empty until a command calls output::record_box_id, and that no command calls it yet. A follow-up issue must add that call where cmd/boxes.rs resolves the UUIDv7. I have no gh credential, so I did not file that issue. The test-kvm and ci-linux-kvm-success excerpts show only runner setup and cleanup lines, not the failing step. This PR changes only the minimal crate, and its otel_cli trace test passes here. I could not find a cause in this diff, and this box has no /dev/kvm to reproduce the lane. docs/reference/cli-min.md is outside the layers, so I did not change it. just clippy-strict was not run. |
There was a problem hiding this comment.
arch-review: PASS_WITH_CONDITIONS · 2 conditions · the full ruling
This head removes the typed box name from the cmd span. That fixes the D9 condition from the last ruling. The writer, the error object and Secret conform to Output conventions lines 381 to 384. Two parts of the task stay open and have no issue. C1: file an issue that wires record_box_id to the resolved box id. C2: file an issue that moves the -o verbs and policy.rs onto the writer. Link both issues from the PR body.
Ruled at 938369f against gominimal/arch@7a86ec7bb3c0 by claude-opus-5-5.
There was a problem hiding this comment.
Reviewed at 938369f2 against BCLI-033 and BCLI-063, and the plan block in #2891.
- Layers: the diff touches exactly the five files in its layers.
- Error object: one stderr line,
{"schema":"min/v1/error","code","message","hint"}, with the exit codes unchanged. The broken-pipe path still exits 141. - Secrets:
Secretserializes only{"ref":…}, and Display and Debug redact the value. - Property test: the T1 test really generates. A mutation that writes the value fails it at once.
- Schemas: all eight strings are exact.
- Residuals: arch's conditions are residual-only. #2895 wires
record_box_idand #2980 moves the renderers, and both are linked in the body. Thecli-min.mdhintwording and the duplicate-schemacaveat are noted on #2980. - Nits, non-blocking: the hand-built schema loop at
output.rs~448 asserts its own input.ObjectType::ALLis kept by hand, andcommand_verbhardcodes single subverbs.
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 938369f23363 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.
T5 — Versioned JSON objects and errors, secrets as references
slice: S1
kind: task
covers: BCLI-033, BCLI-063
size: M
est: 250 code, 180 tests
executor: agent
difficulty: needs care
model: standard
diagnostics: under
-o json, a failed command writes one object withcode,messageandhintto stderr, and nothing to stdout, pinned by machine_mode_error_object_on_stderrobservability: the command's existing tracing span records the verb, the box id it resolved and the outcome
layers:
- client:
- crates/minimal/src/cmd/output.rs (new): one writer for
min/v1/<type>objects, the stderr error object, and secrets as references- crates/minimal/src/cmd/mod.rs: dispatches the verb
- crates/minimal/src/main.rs: routes a failure under a machine output mode to the error object
- crates/minimal/Cargo.toml: adds proptest as a dev-dependency
- Cargo.lock: records proptest for the minimal crate
test: cargo nextest run -p minimal json_output_schema_per_object
test: cargo nextest run -p minimal machine_mode_error_object_on_stderr
test: cargo nextest run -p minimal secrets_rendered_as_refs_in_every_output_mode
produces:
- crates/minimal/src/cmd/output.rs
consumes: none
proof: Every
-o jsonobject carries itsmin/v1schema, a failure under-o jsonis one error object, and no output mode prints a secret's value.Proof
The new module minimal::output writes every -o json and -o jsonl object, alone or in a listing, with a schema field min/v1/ for the eight types. Under a machine output mode, a failed min command writes one min/v1/error line with code, message and hint to stderr, and nothing to stdout. A Secret field renders only its reference in text, json and jsonl, and a property test pins this. No command flag is new: min session policy -o json and min box audit -o jsonl fail through the new writer, and min box list and min box show do not take -o yet.
Notes from the agent: 1. The error object now always carries the hint key, as an empty string when no remedy is known. Before, the key was left out. I renamed the main.rs test machine_mode_error_line_omits_an_empty_hint to machine_mode_error_line_keeps_an_empty_hint. 2. cli.rs is not in the layers, so no verb has a new -o flag. A later task that adds -o json to box list, show and the other verbs must render through output::write_object or write_objects, with an OutputMode, and hold secrets as output::Secret. 3. The existing session-policy document (cmd/boxes/policy.rs) still writes its own min/v1/session-policy stamp. That file is outside the layers. 4. Observability: the root cmd span now records verb and outcome (ok, error, handoff). output::record_box_id records box_id on it, but nothing calls it yet, because box resolution lives in cmd/boxes.rs, outside the layers. 5. The machine_mode_error_object_on_stderr test drives main's failure path (answer_failure) with a Vec as stderr. It does not run the binary against a daemon. 6. I ran the 19 bin unit tests and the 4 output tests as well. I did not run the full crate suite or just ci.
Tests
cargo nextest run -p minimal json_output_schema_per_objectcargo nextest run -p minimal machine_mode_error_object_on_stderrcargo nextest run -p minimal secrets_rendered_as_refs_in_every_output_modeVerify
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 #2891
Residuals
output::record_box_idfrom the box resolution, so the field holds the resolvedbox_id, never the typed text (notes 4).-o json/-o jsonlrenderer ontooutput::write_object/write_objectswithoutput::Secret, includingmin box list,min box showand themin/v1/session-policystamp incmd/boxes/policy.rs(notes 2 and 3).