fix(deploy): surface container script stderr instead of generic failure messages#181
Conversation
There was a problem hiding this comment.
Pull request overview
This PR improves deploy/build diagnostics by preserving and surfacing container/remote stderr on non-zero exits, avoiding misleading “no output” failures across human, TUI, JSON, and --runs-on remote modes.
Changes:
- Adds captured-execution APIs that return exit status plus both stdout/stderr for local and remote container runs.
- Updates deploy/build phases to use captured results and emit self-explaining errors (including bounded stderr tails) instead of collapsing failures into “missing stamps” / “produced no output”.
- Ensures important notices (e.g., TUF staging skip) and failure diagnostics are visible in TUI/JSON modes without corrupting NDJSON stdout.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/utils/runs_on.rs | Surfaces remote container failures (bounded stderr) before returning None, and adds a captured-output API. |
| src/utils/remote.rs | Introduces RemoteCommandOutput and a capture method that avoids leaking docker command lines in non-verbose errors. |
| src/utils/container.rs | Adds captured container run output types, stderr-tail formatting, and output-mode-safe failure notice printing. |
| src/commands/runtime/deploy.rs | Uses captured container results for stamp/hash phases and emits accurate, diagnostic-rich phase errors. |
| src/commands/runtime/build.rs | Routes non-fatal staging skip notices through renderer/stderr in TUI/JSON, and uses captured container results for hash collection errors. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
50de745 to
66dac97
Compare
jetm
left a comment
There was a problem hiding this comment.
Independent review - essentially clean. Verified the risky parts: str_tail snaps forward to a char boundary (no multibyte panic; the accented-char repeat test covers it), JSON-mode failure notices go to stderr or serde-escaped step_error events so NDJSON stdout stays valid, and the non-verbose error paths never embed the docker command line or -e KEY=value (only stderr / the ssh target). One low-severity contract nit inline. Two pre-existing nits not worth blocking: verbose remote capture double-logs the command (matches the non-capture path), and deploy.rs's container_failure_message is a thin passthrough over the util fn build.rs calls directly.
…re messages captured container runs previously collapsed any non-zero exit to Ok(None), discarding stderr. callers then invented explanations for the missing output: deploy phase 1 reported "hash collection script produced no output" while the script's actual diagnostic (e.g. the signing-key prerequisite for --connect-sign) was thrown away, and stamp verification misreported container failures as missing build stamps. - split run_in_container_with_output into a full-result capture method (success + stdout + stderr) and a compatibility wrapper for the existing call sites - on failure the wrapper now prints a stderr tail (last 15 non-empty lines) by default instead of hiding it behind --verbose; --verbose still prints full stderr - migrate deploy's stamp-verification and hash-collection sites to the capture method so failures name the real problem, embed the stderr tail in both the cli error and the step_error json event (rendered by the desktop phase strip), and distinguish container failures from genuinely missing stamps - print the underlying error in the remote (--runs-on) output path instead of swallowing it entirely - fix the equivalent misleading context messages in runtime build's tuf staging steps
66dac97 to
ef4998e
Compare
jetm
left a comment
There was a problem hiding this comment.
Re-reviewed at ef4998e: my earlier stdout-trim-contract nit is fixed - both local and remote capture now bind let mut stdout and trim in place, so ContainerRunOutput.stdout has one uniform whitespace contract (stderr stays raw on both). The captured-execution rework is otherwise clean (char-boundary-safe stderr tail, NDJSON-safe JSON mode, no docker-command/-e leakage in non-verbose paths). LGTM.
Problem
When a script run inside the SDK container exits non-zero, the captured-output helpers collapse the failure to "no output" and discard stderr. Callers then invent their own explanation for the missing output, producing misleading errors:
avocado deployreportsHash collection script produced no outputwhen the script actually failed with an actionable diagnostic (for example,--connect-signrequiring a local signing key, or a runtime built without one). The real message was written to stderr and thrown away.--output jsonmodes, failure prints were suppressed entirely, so those runs got no diagnostic at all.--runs-onremote failures were fully silent.Changes
run_in_container_capture, plus a remote counterpart) returns exit status with both streams; the existingrun_in_container_with_outputbecomes a compatibility wrapper with unchanged semantics for its ~22 call sites.step_errorJSON event, and container failures are no longer conflated with genuinely missing stamps.--runs-on) failures report the container's stderr without embedding the docker command line (which carries-e KEY=valuepairs) in error output.--verboseprints the full captured stderr at failure sites; truncation annotations no longer promise output that a mode can't deliver.15 × 4 KiBpeak regardless of stderr size, and captured streams avoid redundant full-buffer copies.Example, deploying a runtime whose build has no update-authority baked:
Before:
After:
Testing
cargo test --lib— 1121 passed.ERROR:diagnostics now appear in human output and in thestep_errorNDJSON event.