From 7e62dae28b3e4c0084be30fdb97abd7ec3afc24a Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Thu, 30 Apr 2026 15:04:44 -0400 Subject: [PATCH] chore: add plans --- ...-command-output-streaming-cas-logs-plan.md | 130 ++- ...04-30-preserve-exec-failure-diagnostics.md | 783 ++++++++++++++++++ 2 files changed, 874 insertions(+), 39 deletions(-) create mode 100644 docs/superpowers/plans/2026-04-30-preserve-exec-failure-diagnostics.md diff --git a/docs/plans/2026-04-30-001-feat-command-output-streaming-cas-logs-plan.md b/docs/plans/2026-04-30-001-feat-command-output-streaming-cas-logs-plan.md index a396d7f95..3b5402cbb 100644 --- a/docs/plans/2026-04-30-001-feat-command-output-streaming-cas-logs-plan.md +++ b/docs/plans/2026-04-30-001-feat-command-output-streaming-cas-logs-plan.md @@ -9,49 +9,94 @@ date: 2026-04-30 ## Summary -Implement command-node stdout/stderr handling like CI logs: write streams to server scratch while the command runs, expose live reads through a REST tail endpoint, and finalize both streams into SlateDB CAS blobs when the process reaches a terminal state. +Implement command-node stdout/stderr handling like CI logs: write output to server scratch while the command runs, expose live reads through a REST tail endpoint, and finalize both streams into SlateDB CAS refs when the command reaches a terminal state. + +This plan deliberately separates two concerns: + +- Live log bytes are raw scratch files used by the tail endpoint while the command is running. +- Durable context values are JSON-compatible CAS blobs. `command.output` and `command.stderr` hold `blob://sha256/` refs whose blob payloads are JSON strings, not raw log bytes. Key decisions: -- Use `StageId` (`tests@2`) as the concrete execution identity; no `StageAttemptId`. -- `command.output` and `command.stderr` context values become `blob://sha256/` CAS refs. -- Do not add `command.*_blob` context keys. -- Use REST tailing only for live reads; do not emit stdout/stderr chunk events. +- `StageId` is `@` and is the only public execution identity; no `StageAttemptId`. +- `command.output`, `command.stderr`, `NodeState.stdout`, `NodeState.stderr`, and new `command.completed` stdout/stderr values are CAS refs after finalization, including empty streams. +- Existing old runs with inline stdout/stderr remain supported by treating these fields as text-or-blob-ref strings. +- REST tailing is chosen over stdout/stderr chunk events to avoid high-volume durable event logs and SSE backpressure from chatty commands. Events record lifecycle and final refs; log bytes live in scratch/CAS and are read on demand. +- No automatic redaction is applied to command stdout/stderr in this pass. Command output is trusted, authenticated run data like CI logs. -## Key Changes +## Storage And Runtime -- Add one shared stream enum, `CommandOutputStream { Stdout, Stderr }`, reused by sandbox streaming and the new API. -- Keep `CommandStartedProps`; update `CommandCompletedProps` so `stdout` and `stderr` are CAS ref strings, with added `stdout_bytes` and `stderr_bytes`. -- Keep `NodeState.stdout` and `NodeState.stderr`, but store the same CAS refs there instead of inline output. -- Add `Sandbox::exec_command_streaming(...)` with a default fallback that calls existing `exec_command`; implement real streaming for local, Docker, and Daytona. -- Use Daytona Toolbox session APIs for Daytona streaming: create session, execute async command, poll command details/logs, and diff stdout/stderr from `/process/session/{sessionId}/command/{commandId}/logs`. -- In the command handler, write chunks to: - - `RunScratch::runtime_dir()/stages//command/stdout.log` - - `RunScratch::runtime_dir()/stages//command/stderr.log` -- Percent-encode the stage path segment using the same convention as `ArtifactStore` so unusual node IDs cannot escape the scratch directory. -- On terminal process state, write both scratch files to CAS, including empty streams, using `RunStoreHandle::write_blob`, then set context refs with `format_blob_ref`. -- Preserve current failure behavior as much as possible: successful and nonzero exits return outcomes with `command.output` / `command.stderr` refs; timeout/cancel still finalize CAS and emit `command.completed` before returning the existing error shape. +- Add one shared closed enum, `CommandOutputStream { Stdout, Stderr }`, for sandbox streaming and API path validation. +- Add `Sandbox::exec_command_streaming(...)` with a default compatibility fallback that calls `exec_command` and emits buffered stdout/stderr after process exit. Real streaming is required for local and Docker in this pass. +- Implement a command log recorder with one writer per `(run_id, stage_id, stream)`. The recorder appends bytes to scratch, flushes after each chunk or bounded batch, and tracks `stdout_bytes` / `stderr_bytes`. +- Scratch paths use the existing run scratch root and artifact-style encoding: + - `RunScratch::runtime_dir()/stages/@/command/stdout.log` + - `RunScratch::runtime_dir()/stages/@/command/stderr.log` +- The API accepts the normal `StageId` string (`tests@2`); filesystem storage percent-encodes only the node id and pads the attempt to match `ArtifactStore` conventions. +- The command handler must use the existing `run_dir: &Path` and `StageScope::for_handler(...)`; add a `StageScope::stage_id() -> StageId` helper if needed. Direct handler tests must seed or fall back to attempt `1`. +- Terminal command state means normal exit, nonzero exit, timeout after subprocess spawn, or cancellation after subprocess spawn. Before final CAS writes, stream drain tasks are joined and recorder file handles are flushed. +- On terminal command state, read each scratch log into memory, convert it to the command text value, JSON-encode that string, write it with `RunStoreHandle::write_blob`, and store `format_blob_ref(...)` in `command.output` / `command.stderr`. +- Empty streams are finalized the same way as non-empty streams: their JSON string value is `""`, byte count is `0`, and the context/projection/event fields still contain a CAS ref. +- After finalization, `command.output` and `command.stderr` are already blob refs, so the existing 100KB `offload_large_values` lifecycle step is a no-op for those values. +- If command spawn fails before logs are established, preserve the existing handler error path and do not invent CAS refs. +- If the server crashes mid-command, scratch files may remain readable while the run scratch directory exists, but no CAS refs are created because no `command.completed` finalization occurred. No recovery job or automatic partial CAS finalization is included in v1. +- Scratch files are kept after finalization for the normal run scratch lifetime. The tail endpoint prefers scratch if present and falls back to CAS only when scratch is missing. +- Finalization may hold stdout/stderr in memory. This is acceptable for v1 because expected command outputs are megabytes and deployment hosts have gigabytes of RAM. + +## Events, Context, And Consumers + +- Keep `CommandStartedProps` as the lifecycle start event; do not add chunk/delta events. +- Update `CommandCompletedProps` so new events keep `stdout` and `stderr`, but those fields contain final CAS refs rather than inline text. Add: + - `stdout_bytes` + - `stderr_bytes` + - `streams_separated` + - `live_streaming` +- Add matching optional fields to `NodeState` so projections can expose byte counts and provider fidelity. Existing old events without these fields must deserialize with defaults. +- For local and Docker, `streams_separated = true` and `live_streaming = true`. +- For Daytona, use Toolbox session APIs. Prefer direct HTTP handling of `/process/session/{sessionId}/command/{commandId}/logs` so JSON `{ stdout, stderr, output }` responses can be separated. Poll command logs every 1 second while the command is active; after 3 consecutive transient log-fetch failures, keep the command running but mark `live_streaming = false` and fall back to the final command response when available. If the SDK/API yields only a plain string or prefixed combined output, write combined output to stdout, write empty stderr, set `streams_separated = false`, and set `live_streaming` according to whether output was available before process completion. +- Durable context and checkpoints store refs. Execution-time consumers that need text must resolve `command.output` / `command.stderr` with a shared text-or-blob-ref helper: + - if `parse_blob_ref(value)` succeeds, read the blob and decode it as a JSON string; + - otherwise treat the value as legacy inline text. +- Use that helper for LLM preamble rendering, conditional/router stages, retros, run dumps, CLI displays, server/API consumers, and the web command panel. Models should continue to see the same tail-style command output as today, not a `blob://...` string. +- Nonzero exits still produce `Outcome::fail_classify`; timeout/cancel after spawn still emit `command.completed` with partial refs and then return the existing handler error shape. Failure text should include at most the last 4 KiB from each resolved stream so durable failure records do not embed unbounded logs. ## API And UI - Add OpenAPI route: `GET /api/v1/runs/{id}/stages/{stageId}/logs/{stream}`. -- `stream` is `stdout` or `stderr`. +- The route is under the existing run-scoped API router and must inherit the same auth translation, run authorization, and IP allowlist behavior as other run endpoints. Add an unauthenticated-request test. +- Path params parse into typed `RunId`, `StageId`, and closed `CommandOutputStream`; reject any non-`stdout`/`stderr` stream before touching the filesystem. - Query params: - `offset` default `0` - `limit` default `65536`, max `1048576` - Response shape: - - `stream` - - `offset` - - `next_offset` - - `total_bytes` - - `bytes_base64` - - `eof` - - `cas_ref` -- While a command is running, the endpoint reads from scratch files. After completion, or if scratch is gone, it reads from the CAS ref stored in `NodeState.stdout` / `NodeState.stderr`. -- Return `404` for unknown run/stage/log stream; return `200` with empty bytes when the stream exists but has no new data. + - `stream: "stdout" | "stderr"` + - `offset: u64` + - `next_offset: u64` + - `total_bytes: u64` + - `bytes_base64: string` + - `eof: bool` + - `cas_ref: Option` + - `live_streaming: bool` +- While running, `cas_ref` is `null`. After finalization, `cas_ref` is the final `blob://sha256/` ref. +- The endpoint is byte-offset based and returns raw bytes as base64. The server does not snap reads to UTF-8 boundaries; clients must use a streaming `TextDecoder` and preserve incomplete codepoints between polls. +- State resolution order: + - If scratch exists and no finalized CAS ref is present, serve scratch with `eof: false` and `cas_ref: null`. + - If finalized CAS ref is present, serve scratch if available, otherwise hydrate CAS and serve text bytes with `eof: true`. + - If neither scratch nor CAS exists but the stage exists, return `200` empty with `eof` derived from stage terminal status. + - If the run or stage does not exist, return `404`. +- During scratch-to-CAS transition, the endpoint must not 404. It should seamlessly continue serving from scratch or CAS according to the resolution order above. +- DoS posture: v1 relies on the trusted/single-tenant deployment model plus the 1 MiB per-request limit. No additional rate limiter is included in this pass. - Regenerate Rust and TypeScript API clients after editing `docs/public/api-reference/fabro-api.yaml`. -- Update the web run-stage command panel to treat command `stdout` / `stderr` event fields as refs, then poll the new REST endpoint by offset while the command is running and stop when `eof` is true. -- Update docs that currently say `command.output` and `command.stderr` contain inline text. +- Update the web run-stage command panel: + - show separate stdout and stderr panels, not interleaved output; + - auto-expand stderr when non-empty or the command fails; + - poll every 1 second while the command is running and always do one final poll after `command.completed`; + - cap browser memory per stream to the last 5 MiB and show a truncation indicator; + - preserve follow-tail unless the user scrolls away from the bottom; + - show waiting, running, completed-empty, failed-fetch, timeout, cancel, and CAS-fallback states; + - provide copy for visible log text; + - do not mark the continuously streaming log body as assertive `aria-live`; use stable status text for state changes so screen readers are not flooded; + - leave ANSI color rendering, download, and deep-link-to-byte-offset out of v1. ## Implementation Areas @@ -66,21 +111,26 @@ Key decisions: - `lib/crates/fabro-types/src/run_event/misc.rs` - `lib/crates/fabro-types/src/run_projection.rs` - `lib/crates/fabro-store/src/run_state.rs` + - `lib/crates/fabro-workflow/src/handler/llm/preamble.rs` + - conditional/router and retro paths that read command context values - API/web/docs: - `docs/public/api-reference/fabro-api.yaml` - `lib/crates/fabro-server/src/server.rs` - `apps/fabro-web/app/routes/run-stages.tsx` - `apps/fabro-web/app/lib/query-keys.ts` - `apps/fabro-web/app/lib/queries.ts` + - docs that describe `command.output` and `command.stderr` ## Test Plan -- Unit-test the scratch recorder: interleaved stdout/stderr chunks, byte offsets, empty streams, tails for failure messages, and path encoding. -- Unit-test `CommandCompletedProps` projection: `NodeState.stdout` / `stderr` store CAS refs and byte counts land in `script_timing`. -- Command handler tests: success, nonzero exit, stderr capture, long-running chunk written before completion, timeout finalizes CAS. -- Sandbox tests: local streaming chunks before process exit; Docker streaming from Bollard stdout/stderr frames; Daytona live/ignored test using session command logs when credentials are available. -- Server API tests: offset reads, limit enforcement, completed-run CAS fallback, missing run/stage/stream errors. -- Web tests: query key construction, base64 append behavior, polling stops on `eof`. +- Scratch recorder unit tests: interleaved stdout/stderr chunks, flushed byte counts, empty streams, bounded failure tails, and artifact-style stage path encoding. +- Command handler tests: success, nonzero exit, stderr capture, long-running chunk readable before completion, timeout finalizes partial CAS refs, and empty streams produce refs with zero byte counts. +- Context compatibility tests: preamble, condition/router, retros, dump, CLI display, and web/server consumers handle both legacy inline text and new blob refs via `parse_blob_ref`. +- Blob compatibility tests: command output CAS blobs contain JSON strings and work with existing blob hydration/dump paths. +- Projection/event tests: `command.completed` refs and byte metadata land in `NodeState`, old inline events still deserialize and project. +- Sandbox tests: local streaming chunks before process exit; Docker streaming from Bollard stdout/stderr frames; default fallback emits buffered output after completion; Daytona live/ignored test covers separated JSON logs and combined-output fallback when credentials are available. +- Server API tests: typed path parsing, unauthenticated rejection, offset reads, limit enforcement, `cas_ref: null` while running, completed-run CAS fallback, scratch-to-CAS transition, missing run/stage/stream errors, and orphan scratch behavior. +- Web tests: query key construction, streaming `TextDecoder` behavior across chunk boundaries, final poll after `command.completed`, memory cap/truncation, stdout/stderr panel states, and copy-visible-log behavior. - Verification commands: - `cargo nextest run -p fabro-sandbox -p fabro-workflow -p fabro-store -p fabro-server` - `cargo build -p fabro-api` @@ -89,8 +139,10 @@ Key decisions: ## Assumptions +- `StageId` uses the existing wire form `@`; implementation may still have internal names such as `visit`, but the plan treats that suffix as attempt count. +- Server and worker share the same server storage filesystem. Live tailing depends on this single-node deployment model. +- Command stdout/stderr are treated as text logs for durable context. The live endpoint is byte-oriented over the stored UTF-8/log bytes; arbitrary binary command output fidelity is out of scope. - CAS refs use the existing `blob://sha256/` format. -- Command log CAS blobs store the actual stdout/stderr bytes from the scratch log files. -- REST tailing is the only live-log delivery mechanism for this pass. -- No new stage attempt identifier, no stdout/stderr chunk events, and no `_blob` context keys. -- Streaming CAS writes are out of scope for v1; finalization reads each scratch log once before writing it through the existing CAS API. +- `command.output` and `command.stderr` are refs after finalization; no `_blob` context keys are added. +- No stdout/stderr chunk events are added in this pass. +- No automatic redaction, crash recovery, dedicated scratch GC, or additional request rate limiting is included in v1. diff --git a/docs/superpowers/plans/2026-04-30-preserve-exec-failure-diagnostics.md b/docs/superpowers/plans/2026-04-30-preserve-exec-failure-diagnostics.md new file mode 100644 index 000000000..8f868bb8e --- /dev/null +++ b/docs/superpowers/plans/2026-04-30-preserve-exec-failure-diagnostics.md @@ -0,0 +1,783 @@ +# Preserve Exec Failure Diagnostics Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** Preserve bounded, redacted stdout/stderr tails for failed process executions in durable run events, while keeping server tracing log-safe and avoiding duplicate full process-output types. + +**Architecture:** `fabro_sandbox::ExecResult` remains the only full process execution result type. `fabro_types::ExecOutputTail` is the only durable diagnostic projection: it contains bounded, redacted output excerpts and no exit code, timeout, duration, command, or label fields. Failure events get `exec_output_tail` additively; existing event fields are not removed in this change. + +**Tech Stack:** Rust, serde, thiserror, tracing, `fabro-redact`, Fabro run events, Fabro sandbox abstractions. + +--- + +## Files And Responsibilities + +- Modify `lib/crates/fabro-types/src/run_event/infra.rs`: define `ExecOutputTail` and add optional `exec_output_tail` fields to failure event props. +- Modify `lib/crates/fabro-types/src/lib.rs`: re-export `ExecOutputTail` if needed by downstream crates. +- Modify `lib/crates/fabro-types/src/run_event/mod.rs`: update event serde tests for additive failure fields. +- Modify `lib/crates/fabro-sandbox/src/sandbox.rs`: add `ExecResult` helpers that redact full output before tail extraction. +- Modify `lib/crates/fabro-sandbox/src/error.rs`: refactor `Error::Exec` to store `ExecResult` and keep `Display` output-safe. +- Modify `lib/crates/fabro-sandbox/src/daytona/mod.rs`: update the direct `Error::exec(...)` constructor call to the new signature. +- Modify `lib/crates/fabro-workflow/src/event.rs`: carry `exec_output_tail` through internal events and event-body conversion; trace only safe metadata about tails, not tail content. +- Modify `lib/crates/fabro-workflow/src/sandbox_metadata.rs`, `lib/crates/fabro-workflow/src/lifecycle/git.rs`, and `lib/crates/fabro-workflow/src/pipeline/finalize.rs`: preserve push/write diagnostic projections without storing `fabro_sandbox::Error` inside `MetadataSnapshot`. +- Modify `lib/crates/fabro-workflow/src/pipeline/initialize.rs`: add output-tail diagnostics to setup failures while preserving existing `stderr` field for compatibility. +- Modify `lib/crates/fabro-workflow/src/devcontainer_bridge.rs`: add output-tail diagnostics to devcontainer lifecycle failures while preserving existing `stderr` field for compatibility. +- Modify `lib/crates/fabro-workflow/src/handler/llm/cli.rs`: replace CLI install's ad hoc 500-character embedded error detail with `exec_output_tail`. +- Modify `docs/internal/logging-strategy.md`: document the policy that event payloads may contain bounded redacted tails, while tracing logs must not contain tail content by default. + +## Explicit Non-Goals + +- Do not remove, deprecate, or stop populating `SetupFailedProps.stderr` or `DevcontainerLifecycleFailedProps.stderr` in this change. Any future removal requires a separate public event-contract deprecation plan. +- Do not add stdout/stderr tail content to `server.log`. Tracing should record safe metadata only: whether a tail exists, stream lengths, truncation booleans, and the existing safe error message. +- Do not change `HookDecision::Block.reason` to include stdout/stderr. That is user-visible hook semantics and needs a separate design if we want durable hook diagnostics later. +- Do not broadly refactor `sandbox_git.rs` error plumbing beyond constructor/signature updates needed by the `Error::Exec` refactor. +- Do not cap or reshape `CommandCompletedProps` or `AgentCliCompletedProps`; those are command-output product events, not failure diagnostic tails. + +## Task 1: Add The Durable Diagnostic Projection + +**Files:** +- Modify: `lib/crates/fabro-types/src/run_event/infra.rs` +- Modify: `lib/crates/fabro-types/src/lib.rs` +- Modify: `lib/crates/fabro-types/src/run_event/mod.rs` + +- [ ] **Step 1: Add `ExecOutputTail`** + +Add this type near the infrastructure event props in `infra.rs`: + +```rust +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +pub struct ExecOutputTail { + #[serde(default, skip_serializing_if = "Option::is_none")] + pub stdout: Option, + #[serde(default, skip_serializing_if = "Option::is_none")] + pub stderr: Option, + #[serde(default, skip_serializing_if = "is_false")] + pub stdout_truncated: bool, + #[serde(default, skip_serializing_if = "is_false")] + pub stderr_truncated: bool, +} + +fn is_false(value: &bool) -> bool { + !*value +} + +impl ExecOutputTail { + #[must_use] + pub fn is_empty(&self) -> bool { + self.stdout.as_deref().unwrap_or("").is_empty() + && self.stderr.as_deref().unwrap_or("").is_empty() + } + + #[must_use] + pub fn stdout_len(&self) -> usize { + self.stdout.as_deref().map(str::len).unwrap_or(0) + } + + #[must_use] + pub fn stderr_len(&self) -> usize { + self.stderr.as_deref().map(str::len).unwrap_or(0) + } +} +``` + +Keep `is_false` private to the module. Do not add another full process result type. + +- [ ] **Step 2: Add `exec_output_tail` additively to failure props** + +Add this optional field to `MetadataSnapshotFailedProps`, `SetupFailedProps`, `CliEnsureFailedProps`, and `DevcontainerLifecycleFailedProps`: + +```rust +#[serde(default, skip_serializing_if = "Option::is_none")] +pub exec_output_tail: Option, +``` + +Do not remove existing fields, including `stderr` on setup/devcontainer failure props. + +- [ ] **Step 3: Re-export the projection** + +In `lib.rs`, include `ExecOutputTail` in the `pub use run_event::{ ... }` list if downstream crates need to reference it as `fabro_types::ExecOutputTail`. + +- [ ] **Step 4: Add serde tests** + +Add a test in `run_event/mod.rs` that serializes a `MetadataSnapshotFailedProps` with: + +```rust +exec_output_tail: Some(ExecOutputTail { + stdout: Some("last stdout line".to_string()), + stderr: Some("last stderr line".to_string()), + stdout_truncated: false, + stderr_truncated: true, +}) +``` + +Assert the JSON includes `exec_output_tail.stdout`, `exec_output_tail.stderr`, and `exec_output_tail.stderr_truncated`, and explicitly assert `stdout_truncated` is omitted when false. + +Add a second assertion that `exec_output_tail: None` omits the field. + +- [ ] **Step 5: Run focused type tests** + +Run: + +```bash +cargo nextest run -p fabro-types +``` + +Expected: serde tests pass and existing event payloads remain backward compatible. + +## Task 2: Centralize ExecResult Tail Projection + +**Files:** +- Modify: `lib/crates/fabro-sandbox/src/sandbox.rs` +- Modify: `lib/crates/fabro-sandbox/src/error.rs` +- Modify: `lib/crates/fabro-sandbox/src/daytona/mod.rs` + +- [ ] **Step 1: Add projection helpers to `ExecResult`** + +In `sandbox.rs`, add: + +```rust +pub const DEFAULT_EXEC_OUTPUT_TAIL_BYTES: usize = 8 * 1024; +``` + +Add these methods to `impl ExecResult`: + +```rust +pub fn redacted_output_tail( + &self, + max_bytes_per_stream: usize, +) -> Option { + let (stdout, stdout_truncated) = redacted_tail(&self.stdout, max_bytes_per_stream); + let (stderr, stderr_truncated) = redacted_tail(&self.stderr, max_bytes_per_stream); + let tail = fabro_types::ExecOutputTail { + stdout, + stderr, + stdout_truncated, + stderr_truncated, + }; + (!tail.is_empty()).then_some(tail) +} + +pub fn default_redacted_output_tail(&self) -> Option { + self.redacted_output_tail(DEFAULT_EXEC_OUTPUT_TAIL_BYTES) +} + +/// Converts host process output into the canonical full exec result. +/// +/// This stores raw stdout/stderr. Callers must not log these fields directly; +/// use `default_redacted_output_tail()` for events and tracing metadata. +pub fn from_process_output(output: std::process::Output, duration_ms: u64) -> Self { + Self { + stdout: String::from_utf8_lossy(&output.stdout).into_owned(), + stderr: String::from_utf8_lossy(&output.stderr).into_owned(), + exit_code: output.status.code().unwrap_or(-1), + timed_out: false, + duration_ms, + } +} +``` + +The signal-killed fallback is `-1`, matching existing local/docker sandbox behavior. The 8 KiB limit is applied after redaction and sanitization, so it is an event-size budget, not a promise about how many original process-output bytes are represented. At the default, one failure event can add at most about 16 KiB of tail text plus JSON escaping overhead. + +- [ ] **Step 2: Redact before truncating** + +Implement the private helper so it redacts the full stream first, strips ANSI escape sequences and other terminal control characters in the retained diagnostic string, then takes the tail: + +```rust +fn redacted_tail(text: &str, max_bytes: usize) -> (Option, bool) { + if text.is_empty() || max_bytes == 0 { + return (None, !text.is_empty()); + } + + let redacted = fabro_redact::redact_string(text); + let sanitized = sanitize_exec_output(&redacted); + let truncated = sanitized.len() > max_bytes; + let start = if truncated { + sanitized.floor_char_boundary(sanitized.len() - max_bytes) + } else { + 0 + }; + let tail = sanitized[start..].to_string(); + ((!tail.is_empty()).then_some(tail), truncated) +} + +fn sanitize_exec_output(text: &str) -> String { + let mut sanitized = String::with_capacity(text.len()); + let mut chars = text.chars().peekable(); + while let Some(ch) = chars.next() { + if ch == '\u{1b}' { + match chars.peek().copied() { + Some('[') => { + chars.next(); + for next in chars.by_ref() { + if ('@'..='~').contains(&next) { + break; + } + } + } + Some(']') => { + chars.next(); + let mut saw_esc = false; + for next in chars.by_ref() { + if next == '\u{7}' || (saw_esc && next == '\\') { + break; + } + saw_esc = next == '\u{1b}'; + } + } + Some('(' | ')' | '*' | '+' | '-' | '.' | '/') => { + chars.next(); + chars.next(); + } + Some('@'..='_') => { + chars.next(); + } + _ => {} + } + continue; + } + if ch == '\n' || ch == '\r' || ch == '\t' || !ch.is_control() { + sanitized.push(ch); + } + } + sanitized +} +``` + +If the repository MSRV does not support `str::floor_char_boundary`, use the existing pattern from `fabro-agent/src/truncation.rs` and note that in the implementation comment. + +- [ ] **Step 3: Refactor `Error::Exec` to store `ExecResult`** + +In `error.rs`, replace the existing six-field variant with: + +```rust +#[error( + "{label} failed (exit {exit_code}, timed_out={timed_out}, duration_ms={duration_ms}) - hint: {hint}", + exit_code = result.exit_code, + timed_out = result.timed_out, + duration_ms = result.duration_ms, + hint = classify_exec_failure(&result.stderr) + .or_else(|| classify_exec_failure(&result.stdout)) + .unwrap_or("unclassified") +)] +Exec { + label: String, + result: crate::ExecResult, +}, +``` + +Do not interpolate `{result.stdout}` or `{result.stderr}` in the display template. + +Add: + +```rust +pub fn exec(label: impl Into, result: crate::ExecResult) -> Self { + Self::Exec { + label: label.into(), + result, + } +} + +pub fn exec_result(&self) -> Option<&crate::ExecResult> { + match self { + Self::Exec { result, .. } => Some(result), + _ => None, + } +} + +pub fn default_redacted_output_tail(&self) -> Option { + self.exec_result() + .and_then(crate::ExecResult::default_redacted_output_tail) +} +``` + +Keep the existing `display_with_causes()` method. + +- [ ] **Step 4: Update `ExecResult` error constructors** + +Change: + +```rust +pub fn into_exec_error(self, label: impl Into) -> crate::Error { + crate::Error::exec(label, self) +} + +pub fn into_exec_error_with_redactor( + self, + label: impl Into, + redactor: impl Fn(&str) -> String, +) -> crate::Error { + crate::Error::exec(label, Self { + stdout: redactor(&self.stdout), + stderr: redactor(&self.stderr), + ..self + }) +} +``` + +The invariant is: `into_exec_error_with_redactor` may apply command-specific redaction, such as auth URL redaction, before storing output in the error; `default_redacted_output_tail()` always applies generic secret redaction again before exposure. Double redaction is acceptable and expected. + +Keep `into_exec_error_with_redactor` because current Docker and Daytona sandbox credential paths use it for auth URL redaction. The implementation task should verify this with: + +```bash +rg -n "into_exec_error_with_redactor" lib/crates/fabro-sandbox/src +``` + +If that grep has no production callers after the refactor, delete `into_exec_error_with_redactor` and its dedicated tests instead of retaining speculative API surface. + +- [ ] **Step 5: Update direct constructor call sites** + +Update the direct `Error::exec(...)` call in `daytona/mod.rs` to construct an `ExecResult` and pass it to the new constructor. Use `rg "Error::exec\\(" lib/crates/fabro-sandbox/src` to verify there are no old six-argument calls left. + +- [ ] **Step 6: Add sandbox tests** + +Add or update tests for: + +- `Error::Exec` display does not contain raw stdout/stderr. +- `display_with_causes()` does not reintroduce raw stdout/stderr. +- redaction happens before truncation, using a token whose prefix would be outside the final tail. +- ANSI CSI sequences such as `\x1b[31m`, OSC sequences such as `\x1b]0;title\x07`, and common two-byte escape sequences are removed as a unit, not converted into stray printable fragments. +- `from_process_output` uses exit code `-1` for signal-killed processes where the platform exposes no status code. +- lossy/non-UTF-8 output does not panic and still produces a bounded tail. +- a constructed max-size event with both stdout and stderr tails remains below 40 KiB serialized JSON, documenting the budget created by the 8 KiB-per-stream default. + +Run: + +```bash +cargo nextest run -p fabro-sandbox +``` + +Expected: sandbox tests pass and no safe-display test leaks raw command output. + +## Task 3: Carry Tails Through Events Without Logging Tail Content + +**Files:** +- Modify: `lib/crates/fabro-workflow/src/event.rs` + +- [ ] **Step 1: Add optional tails to internal event variants** + +Add `exec_output_tail: Option` to: + +- `MetadataSnapshotFailed` +- `SetupFailed` +- `CliEnsureFailed` +- `DevcontainerLifecycleFailed` + +Keep existing `stderr` fields on `SetupFailed` and `DevcontainerLifecycleFailed`. + +- [ ] **Step 2: Map tails into `EventBody`** + +In `event_body_from_event`, pass `exec_output_tail.clone()` into the corresponding props for all four variants. + +- [ ] **Step 3: Trace only safe tail metadata** + +Do not add `exec_stdout_tail` or `exec_stderr_tail` fields to tracing. In each failure trace arm, include only: + +```rust +exec_output_tail_present = exec_output_tail.is_some(), +exec_stdout_tail_bytes = exec_output_tail.as_ref().map(fabro_types::ExecOutputTail::stdout_len).unwrap_or(0), +exec_stderr_tail_bytes = exec_output_tail.as_ref().map(fabro_types::ExecOutputTail::stderr_len).unwrap_or(0), +exec_stdout_truncated = exec_output_tail.as_ref().map(|tail| tail.stdout_truncated).unwrap_or(false), +exec_stderr_truncated = exec_output_tail.as_ref().map(|tail| tail.stderr_truncated).unwrap_or(false), +``` + +This preserves the server-log debugging breadcrumb without duplicating output content into `server.log`. + +- [ ] **Step 4: Update event tests** + +Update existing constructors in tests to include `exec_output_tail: None`. + +Add a test that converts a `SetupFailed` or `MetadataSnapshotFailed` event with an `ExecOutputTail` and asserts the canonical event body includes the nested tail. + +Add a test around `build_redacted_event_payload` that uses a secret-looking value in `exec_output_tail` and asserts the persisted payload does not contain the raw token. + +- [ ] **Step 5: Run focused event tests** + +Run: + +```bash +cargo nextest run -p fabro-workflow event +``` + +Expected: event conversion includes additive tails and tracing changes compile. + +## Task 4: Preserve Metadata Snapshot Push/Write Diagnostics + +**Files:** +- Modify: `lib/crates/fabro-workflow/src/sandbox_metadata.rs` +- Modify: `lib/crates/fabro-workflow/src/lifecycle/git.rs` +- Modify: `lib/crates/fabro-workflow/src/pipeline/finalize.rs` + +- [ ] **Step 1: Keep `MetadataSnapshot` serializable/simple** + +Do not store `fabro_sandbox::Error` in `MetadataSnapshot`. + +Add a grouping type so the message and tail cannot drift apart. `MetadataSnapshot` is currently `pub(crate)`, so this type should also be `pub(crate)`; if implementation changes `MetadataSnapshot` visibility, make this type at least as visible. + +```rust +#[derive(Debug, Clone)] +pub(crate) struct MetadataPushError { + pub message: String, + pub exec_output_tail: Option, +} +``` + +Change: + +```rust +pub push_error: Option, +``` + +to: + +```rust +pub push_error: Option, +``` + +- [ ] **Step 2: Capture push tail before stringifying** + +Change the push handling to: + +```rust +let push_result = self.sandbox.git_push_ref(&refspec).await; +let push_error = match push_result { + Ok(()) => None, + Err(err) => Some(MetadataPushError { + message: err.display_with_causes(), + exec_output_tail: err.default_redacted_output_tail(), + }), +}; +``` + +Return this field on `MetadataSnapshot`. The only valid states are `None` for no push failure, or `Some(MetadataPushError { message, exec_output_tail })` for a push failure. There are no parallel `Option` fields. + +- [ ] **Step 3: Carry write-failure tails by projection only** + +Change string-only command/sandbox failures in `SandboxMetadataError` to one diagnostic variant carrying the projection, not the full sandbox error: + +```rust +#[derive(Debug, thiserror::Error)] +pub(crate) enum SandboxMetadataError { + #[error("sandbox git unavailable: {0}")] + GitUnavailable(String), + #[error("metadata dump serialization failed: {0}")] + Dump(#[from] anyhow::Error), + #[error("metadata temp file write failed: {0}")] + LocalTemp(std::io::Error), + #[error("{message}")] + Operation { + message: String, + exec_output_tail: Option, + }, +} + +impl SandboxMetadataError { + pub(crate) fn exec_output_tail(&self) -> Option { + match self { + Self::Operation { exec_output_tail, .. } => exec_output_tail.clone(), + _ => None, + } + } +} +``` + +Use `Operation` for both nonzero `ExecResult` returns and sandbox API errors such as upload failures. The distinction does not affect event behavior, so it should stay in the `message` text rather than in enum shape. + +`Dump(#[from] anyhow::Error)` intentionally has no `exec_output_tail`: run-dump serialization should not execute sandbox commands. If a future dump path performs sandbox exec, that code path must return `Operation` instead. + +- [ ] **Step 4: Update metadata command helpers** + +In `exec_stdout`, use: + +```rust +let result = sandbox + .exec_command(command, 30_000, None, env, None) + .await + .map_err(|err| SandboxMetadataError::Operation { + message: err.display_with_causes(), + exec_output_tail: err.default_redacted_output_tail(), + })?; + +if result.is_success() { + Ok(result.stdout.trim().to_string()) +} else { + let error = result.into_exec_error(command.to_string()); + Err(SandboxMetadataError::Operation { + message: error.display_with_causes(), + exec_output_tail: error.default_redacted_output_tail(), + }) +} +``` + +Delete the local `exec_err` helper after callers no longer use it. + +- [ ] **Step 5: Emit metadata failed events with tails** + +In both `lifecycle/git.rs` and `pipeline/finalize.rs`: + +- For push failures, use `snapshot.push_error.as_ref().expect("push error")`, pass its `message.clone()` for the safe error string, and pass `push_error.exec_output_tail.clone()` to `emit_metadata_snapshot_failed`. +- For write failures, pass `err.exec_output_tail()`. +- Keep the warning message string concise and based on the existing safe error string. + +- [ ] **Step 6: Update metadata tests** + +Update tests that assert `MetadataSnapshotFailedProps` to include: + +```rust +assert_eq!(props.exec_output_tail.as_ref().and_then(|tail| tail.stderr.as_deref()), Some("remote: Permission denied")); +``` + +Use fixture strings that are not likely to trigger `fabro_redact` entropy or token rules. + +Also add a metadata unit test that exercises both push states: + +- successful push returns `MetadataSnapshot { push_error: None, ... }`. +- failed push returns `MetadataSnapshot { push_error: Some(MetadataPushError { message, exec_output_tail }), ... }`. + +There must be no independent message/tail options that can get out of sync. + +Run: + +```bash +cargo nextest run -p fabro-workflow metadata_snapshot +``` + +Expected: metadata push/write failure events contain `exec_output_tail` when command output exists. + +## Task 5: Add Tails To Setup, Devcontainer, And CLI Install Failures + +**Files:** +- Modify: `lib/crates/fabro-workflow/src/pipeline/initialize.rs` +- Modify: `lib/crates/fabro-workflow/src/devcontainer_bridge.rs` +- Modify: `lib/crates/fabro-workflow/src/handler/llm/cli.rs` + +- [ ] **Step 1: Add setup failure tails without removing `stderr`** + +When a setup command returns a nonzero exit code, emit: + +```rust +let exec_output_tail = result.default_redacted_output_tail(); +options.emitter.emit(&Event::SetupFailed { + command: command.clone(), + index, + exit_code: result.exit_code, + stderr: result.stderr.clone(), + exec_output_tail, +}); +``` + +Keep the existing `stderr` value for compatibility in this change. + +- [ ] **Step 2: Add devcontainer failure tails without removing `stderr`** + +For both parallel and single-command lifecycle failures, emit: + +```rust +let exec_output_tail = result.default_redacted_output_tail(); +emitter.emit(&Event::DevcontainerLifecycleFailed { + phase: phase.clone(), + command: name.clone(), + index, + exit_code: result.exit_code, + stderr: result.stderr.clone(), + exec_output_tail, +}); +``` + +Use `phase.to_string()` and `command.to_string()` in the single-command path, matching the current code. + +- [ ] **Step 3: Replace CLI install's embedded output detail** + +In CLI ensure install failure handling, replace the 500-character manual tail embedded in `error_msg` with: + +```rust +let exec_output_tail = install_result.default_redacted_output_tail(); +let error_msg = format!( + "{cli_name} install exited with code {}", + install_result.exit_code +); +emitter.emit(&Event::CliEnsureFailed { + cli_name: cli_name.to_string(), + provider: provider_str.to_string(), + error: error_msg.clone(), + duration_ms, + exec_output_tail, +}); +return Err(Error::handler(error_msg)); +``` + +- [ ] **Step 4: Add focused tests** + +Add or update tests so that: + +- setup failure with stderr preserves `props.stderr` and adds `props.exec_output_tail.stderr`. +- setup failure with stdout-only output adds `props.exec_output_tail.stdout`. +- devcontainer lifecycle failure adds the nested tail while preserving `stderr`. +- CLI ensure failure no longer embeds command output in `error`, but includes `exec_output_tail`. + +Run: + +```bash +cargo nextest run -p fabro-workflow setup +cargo nextest run -p fabro-workflow devcontainer +cargo nextest run -p fabro-workflow cli +``` + +Expected: failure events are additive and backward compatible. + +## Task 6: Update Constructor Call Sites And Avoid Broad Refactors + +**Files:** +- Modify only files that fail to compile from the `Error::exec` signature change. + +- [ ] **Step 1: Find old constructor call sites** + +Run: + +```bash +rg -n "Error::exec\\(|\\.into_exec_error_with_redactor\\(" lib/crates/fabro-sandbox lib/crates/fabro-workflow lib/crates/fabro-hooks +``` + +Expected: old direct `Error::exec(label, exit_code, timed_out, duration_ms, stderr, stdout)` calls are limited and mechanical. + +- [ ] **Step 2: Update direct `Error::exec` calls mechanically** + +For each old direct call, construct an `ExecResult` with the same existing values and pass it to `Error::exec(label, result)`. + +Do not refactor `sandbox_git.rs` return types unless a compile error forces it. + +- [ ] **Step 3: Keep hook behavior unchanged** + +Do not add hook stdout/stderr tails to `HookDecision::Block.reason`. If compilation requires use of `ExecResult::from_process_output`, use it internally only and preserve the existing decision behavior. + +- [ ] **Step 4: Run compile-focused checks** + +Run: + +```bash +cargo check -p fabro-sandbox +cargo check -p fabro-workflow +cargo check -p fabro-hooks +``` + +Expected: constructor refactor compiles without broad unrelated changes. + +## Task 7: Update Logging Policy + +**Files:** +- Modify: `docs/internal/logging-strategy.md` + +- [ ] **Step 1: Preserve the raw-output prohibition** + +Update the prohibited-fields guidance to say: + +```markdown +Raw command stdout/stderr, including raw `git_stderr`, must not be emitted to tracing logs. Durable run events may include `ExecOutputTail`, which is bounded and redacted before serialization. Tracing may include only tail metadata such as presence, byte count, and truncation booleans. +``` + +- [ ] **Step 2: Add a safe tracing example** + +Add an example like: + +```rust +error!( + command, + exit_code, + exec_output_tail_present, + exec_stdout_tail_bytes, + exec_stdout_truncated, + exec_stderr_tail_bytes, + exec_stderr_truncated, + "Setup command failed" +); +``` + +Do not show tail content fields in the tracing example. + +- [ ] **Step 3: Verify docs mention both sides of the policy** + +Run: + +```bash +rg -n "ExecOutputTail|raw command stdout|exec_stderr_tail_bytes|git_stderr" docs/internal/logging-strategy.md +``` + +Expected: docs allow bounded redacted tails in events and prohibit raw/tail content in tracing. + +## Task 8: Full Verification + +**Files:** +- Existing Rust test modules only. + +- [ ] **Step 1: Run focused crate tests** + +Run: + +```bash +cargo nextest run -p fabro-sandbox +cargo nextest run -p fabro-types +cargo nextest run -p fabro-workflow +``` + +Expected: all focused tests pass. + +- [ ] **Step 2: Run hook tests to prove behavior stayed stable** + +Run: + +```bash +cargo nextest run -p fabro-hooks +``` + +Expected: existing hook command behavior remains unchanged. + +- [ ] **Step 3: Run formatting** + +Run: + +```bash +cargo +nightly-2026-04-14 fmt --check --all +``` + +Expected: formatting passes. + +- [ ] **Step 4: Run clippy after tests pass** + +Run: + +```bash +cargo +nightly-2026-04-14 clippy --workspace --all-targets -- -D warnings +``` + +Expected: no new warnings. + +- [ ] **Step 5: Inspect one event payload** + +Run or unit-test a setup failure and inspect the canonical event. The expected shape is additive: + +```json +{ + "event": "setup.failed", + "properties": { + "command": "example command", + "index": 0, + "exit_code": 1, + "stderr": "existing compatibility field", + "exec_output_tail": { + "stdout": "bounded redacted stdout tail", + "stderr": "bounded redacted stderr tail", + "stderr_truncated": true + } + } +} +``` + +Also inspect the matching tracing output and confirm it contains only tail metadata, not `exec_output_tail.stdout` or `exec_output_tail.stderr` content. + +## Assumptions And Defaults + +- Default tail budget is 8192 bytes per stream after redaction and sanitization. +- Tail-only output is deliberate for this change because the explicit debugging gap was missing tail evidence for failed subprocesses. This plan does not introduce head+tail excerpts. If setup/compiler-style failures need first-line diagnostics later, add a separate event-field design rather than changing `ExecOutputTail` semantics in place. +- Existing full-output command events remain unchanged. +- Existing `stderr` failure fields remain unchanged for compatibility in this plan. +- Redaction is best-effort token/secret redaction, not a guarantee against every path, hostname, customer identifier, or PII value. This is why tail content is not duplicated into tracing logs. +- Operators who need unredacted live debugging should use the underlying sandbox/process environment directly; this plan does not add an unredacted support-channel escape hatch.