Repository navigation
feat(minimal): T45 — Box logs strips terminal control codes when stdout is not a terminal - #3335
minimal-foundry[bot] wants to merge 6 commits into
Conversation
min box logs, min session logs and min task logs now remove terminal control sequences from the output of a box with a terminal when stdout is not a terminal. --raw keeps the bytes as the box wrote them. A terminal on stdout still gets the raw stream.
…trips-terminal-control-codes-when-stdo
|
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 found in the changed hunks. |
There was a problem hiding this comment.
arch-review: CONDITIONS · 2 conditions · the full ruling
The stripping is in the correct place. It is a client-side view, and the captured log stays byte-exact. Two items block the merge. First, --raw is accepted with --exec and does nothing there. The new help text says that output to a non-terminal has its control codes removed, but --exec output keeps them. Design principle 6 says help and behaviour must not drift apart. Make the flags conflict and correct the help text (C1). Second, no spec requirement binds the new default or --raw. Add them under BCLI-012 with the two tests as verify: lines (C2).
Ruled at bc14637 against gominimal/arch@15b603e6afb8 by claude-opus-5-5.
|
Round 1 of 10 on a failed check suite: the coder changed nothing. Revise round 1: the directive lists only the failed macOS |
--exec now refuses --raw, because it never strips. The --raw help says so. A failed box record read writes the log raw and does not fail the command. A line feed ends an OSC or DCS string that has no end, so a malformed sequence cannot hide the rest of the log.
|
/revise Head 1. C1: 2. C2: spec 28 states the behaviour.
Give them Reply on the arch threads with the commit. Run the whole |
|
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. |
|
Round 2 of 10 on the runtime's review findings, pushed as
Verify Converted to a draft: something above needs a person before review. Review threads the coder judged:
Notes from the agent: Revise round 2. I ran both test lines together in one nextest call, and both passed. clippy -p minimal --all-targets is clean. Thread C2 asks for a change to the spec file docs/specs/28-spec-box-cli.md. That file is outside this task's layers, so I made no change for it. |
There was a problem hiding this comment.
arch-review: PASS · the full ruling
The compare conforms to the design. A failed box record read now prints the log raw, and a failed log read still refuses. --exec refuses --raw, so line 479's form has no flag that does nothing. A line feed ends a control string that has no end, so a malformed sequence cannot hide the transcript. The new test assertions prove each change. The agent-verify failure is in otel_cli.rs, which this PR does not touch. Make it pass before the PR leaves draft.
Ruled at cb0f948 against gominimal/arch@15b603e6afb8 by claude-opus-5-5.
Add the two BCLI-012 sub-requirements the arch ruling asks for, and make the --raw help say that --exec output is written unchanged.
|
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:
Notes from the agent: C1 was already in cb0f948: |
Spec lint
Advisory. None of this blocks the merge; the review decides. Advisory, and never a merge gate. Linter: |
| Csi => match byte { | ||
| Self::ESC => Escape, | ||
| 0x40..=0x7e => Ground, | ||
| 0x00..=0x3f | 0x7f => Csi, | ||
| _ => { | ||
| self.state = Ground; | ||
| return self.step(byte, text); | ||
| } |
There was a problem hiding this comment.
LOW · correctness · confidence 0.50 · in ControlStripper::step
An unterminated CSI sequence hides all later output. The Csi state keeps every byte in 0x00..=0x3f, including line feed, until a final byte (0x40..=0x7e) arrives. A truncated ESC [ near the end of a capture chunk, or one cut by the capture bound, therefore removes the following text up to the next letter. EscapeIntermediate has the same shape. The ControlString state already ends at a line feed for this reason, and a terminal runs C0 codes inside a CSI. Handle line feed, and carriage return, in the same way here.
What the reviewer checked
Csi => match byte { ESC => Escape, 0x40..=0x7e => Ground, 0x00..=0x3f | 0x7f => Csi, ... } versus ControlString: b'\n' => { text.push(byte); Ground }
Hand this to an agent
Act on a code-review finding.
File: crates/minimal/src/cmd/boxes.rs
Lines: 275-282
Category: correctness
Severity: low (reviewer confidence 0.50)
Finding:
An unterminated CSI sequence hides all later output. The `Csi` state keeps every byte in 0x00..=0x3f, including line feed, until a final byte (0x40..=0x7e) arrives. A truncated `ESC [` near the end of a capture chunk, or one cut by the capture bound, therefore removes the following text up to the next letter. `EscapeIntermediate` has the same shape. The `ControlString` state already ends at a line feed for this reason, and a terminal runs C0 codes inside a CSI. Handle line feed, and carriage return, in the same way here.
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 33b8f333c627: Csi and EscapeIntermediate now end at a line feed or carriage return, and keep it as text. A test pins both cases.
…trips-terminal-control-codes-when-stdo
A CSI or escape sequence with no final byte hid every later byte up to the next letter. A line feed or carriage return now ends it and stays as text, as it already did for OSC and the other control strings.
|
/revise Head An unterminated CSI sequence hides at most the rest of its line. Reply on the thread with the commit. |
|
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. |
|
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:
Notes from the agent: Revise round 4: a line feed or carriage return now ends an unterminated CSI or escape-intermediate sequence, and the stripper keeps that byte as text. The unit cases are in box_logs_piped_pty_transcript_has_no_control_codes. cargo fmt --check and cargo clippy -p minimal --all-targets are clean. |
|
Round 5 of 10 on a /revise note: the coder changed nothing. The directive asks for the fix to thread PRRT_kwDOPdfcxM622ZH_. Commit 33b8f33 (fix(minimal): end unterminated CSI at a line end in box logs) already makes this change on the branch. In ControlStripper::step, a line feed or carriage return ends the Csi and EscapeIntermediate states and stays as text. The commit also adds assertions in box_logs_piped_pty_transcript_has_no_control_codes for a cut-off |
|
Round 6 of 10 on a failed check suite: the coder changed nothing. Revise round 6 addressed a failed |
There was a problem hiding this comment.
arch-review: PASS · the full ruling
This head conforms. The new commit ends an unterminated CSI or escape sequence at a line end, and it keeps the line end as text. Before this change, such a sequence hid the transcript up to the next final byte. Line 473 of the architecture makes logs the PTY box's transcript. The fix keeps more of that transcript. The merge from main is outside this change. The ruling on 38a3ce2b stands for all other parts.
Ruled at 33b8f33 against gominimal/arch@15b603e6afb8 by claude-opus-5-5.
T45 — Box logs strips terminal control codes when stdout is not a terminal
slice: S2
kind: task
covers: BCLI-012
size: S
est: 50 code, 80 tests
executor: agent
difficulty: needs care
model: strongest
diagnostics: none new
observability: none new
layers:
- client:
- crates/minimal/src/cli.rs:
min box logsand the type nouns'logstake--raw- crates/minimal/src/cmd/boxes.rs:
box_logs_on_hostsstrips terminal control sequences (CSI, OSC and other escape sequences) from a PTY box's transcript when stdout isn't a terminal and--rawisn't given- crates/minimal/src/tests.rs: the in-process minimald test harness gains this layer's tests
test: cargo nextest run -p minimal box_logs_piped_pty_transcript_has_no_control_codes
test: cargo nextest run -p minimal box_logs_raw_keeps_control_codes
produces: none
consumes: none
proof:
min box logs <pty box> | catprints the transcript text with no terminal control sequences;--rawprints them unchanged; a terminal still gets the raw stream.Proof
min box logs <pty box> | cat(and the session and task nouns) now prints the transcript text without CSI, OSC, DCS, charset or other escape sequences.--rawprints the bytes unchanged. A stdout that is a terminal, and a box with no terminal, still get the raw stream.Notes from the agent: box_logs_on_hosts takes a new
stdout_is_terminal: boolargument. cmd_box_logs sets it fromstd::io::stdout().is_terminal(), and the five existing test calls pass false. When it strips, the verb makes one more ShowBox call to readpty_enabledfrom the box's stored spec. ResolvedBox does not carry that field, and address.rs is outside the layers. The stripper is a streaming state machine, so a sequence split across two output chunks is removed whole. It also drops C0 control codes other than tab, LF and CR (BEL, for example). Bytes of 0x80 and up pass through, so 8-bit C1 controls are not stripped, because they collide with UTF-8.--rawis accepted with--execand does nothing there, because the exec rea 8000 d does not strip. New help text for--raw: "Show a terminal box's output byte for byte, with its terminal control codes. Without--raw, output that does not go to a terminal has them removed". The existing tests type_noun_logs_follow and task_run_logs_wait_list_end_to_end also pass. I did not run just ci or clippy-strict.Tests
cargo nextest run -p minimal box_logs_piped_pty_transcript_has_no_control_codescargo nextest run -p minimal box_logs_raw_keeps_control_codesVerify
min task run agent-verifyexited 100, so this PR is a draft.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/minimal/src/tests.rsCloses #3332