effect: never record a CLI-side failure as a run result
commit
255737beffect: never record a CLI-side failure as a run result
docker.rs and sprite.rs mapped any nonzero CLI exit to RunStatus::Fail,
but docker run exits 125 for the daemon’s own failures and sprite
exec exits nonzero for transport failures — infrastructure failures
effect.result-taxonomy says MUST NOT be recorded as a result. A hosted
worker would have signed and pushed a permanent fail, wrongly
discharging the commit’s work-set obligation.
Both backends now wrap the command so its combined output ends with an exit-marker line carrying the command’s own status; the marker’s presence, not the CLI’s exit status, is the completion signal. Marker present: pass/fail from the recorded status. Marker absent: the sandbox never completed the run — Error::Sandbox, never a result. The Sprite script additionally exits before the marker when its cd into the synced workdir fails, so a lost sync is infrastructure too.
Assisted-by: Claude:claude-sonnet-4-6
Reviews
No reviews of this commit yet — record a verdict below.
Start a review
crates/ents-effect/src/docker.rs
@@ -14,7 +14,9 @@
use std::process::{Command, Stdio};
use crate::error::{Error, Result};
-use crate::executor::{Executor, RunOutput, RunStatus, SandboxInputs, activate};
+use crate::executor::{
+ Executor, RunOutput, SandboxInputs, activate, parse_exit_marker, wrap_exit_marker,
+};
/// The minimal base image every effect runs in — no toolchain of its own;
/// everything the command needs comes from the bind-mounted, host-exported
@@ -59,22 +61,23 @@
/// Assemble `docker run`'s argv for one sandboxed run — pure, so the exact
/// invocation is unit tested without a daemon. The command runs under
/// `sh -c`, stderr folded into stdout so the captured recording is one
-/// interleaved stream.
+/// interleaved stream, wrapped by [`wrap_exit_marker`] so a completed
+/// command is distinguishable from a docker-side failure (`docker run`
+/// exits 125 when the daemon itself fails — never a result,
+/// `effect.result-taxonomy`).
///
/// # Examples
///
/// ```
/// use ents_effect::docker::run_args;
+/// use ents_effect::executor::EXIT_MARKER;
/// use std::path::Path;
///
/// let args = run_args(Path::new("/tmp/s/work"), &[], "cargo test");
-/// assert_eq!(
-/// args,
-/// vec![
-/// "run", "--rm", "-v", "/tmp/s/work:/work", "-w", "/work",
-/// "debian:stable-slim", "sh", "-c", "cargo test 2>&1",
-/// ]
-/// );
+/// assert!(args.contains(&"/tmp/s/work:/work".to_owned()));
+/// assert!(args.contains(&"debian:stable-slim".to_owned()));
+/// let script = args.last().expect("has a script");
+/// assert!(script.contains("cargo test") && script.contains(EXIT_MARKER));
/// ```
#[must_use]
pub fn run_args(workdir: &Path, toolchains: &[(String, PathBuf)], command: &str) -> Vec<String> {
@@ -99,7 +102,7 @@
args.push(IMAGE.to_owned());
args.push("sh".to_owned());
args.push("-c".to_owned());
- args.push(format!("{} 2>&1", activate(command, &sandbox_dirs)));
+ args.push(wrap_exit_marker(&activate(command, &sandbox_dirs)));
args
}
@@ -109,6 +112,7 @@
pub struct DockerExecutor;
impl Executor for DockerExecutor {
+ // @relation(effect.result-taxonomy, scope=function)
fn run(&self, inputs: &SandboxInputs<'_>) -> Result<RunOutput> {
ensure_docker()?;
let args = run_args(inputs.workdir, inputs.toolchains, inputs.command);
@@ -119,13 +123,19 @@
program: "docker".to_owned(),
detail: e.to_string(),
})?;
- let log = String::from_utf8_lossy(&output.stdout).into_owned();
- let status = if output.status.success() {
- RunStatus::Pass
- } else {
- RunStatus::Fail
- };
- Ok(RunOutput { status, log })
+ let stdout = String::from_utf8_lossy(&output.stdout);
+ // The marker, not docker's exit status, is the completion signal:
+ // `docker run` exits 125 for the daemon's own failures (unpullable
+ // image, bad mount, daemon dying mid-run), which must surface as an
+ // infrastructure error, never as a recorded `fail`
+ // (`effect.result-taxonomy`).
+ parse_exit_marker(&stdout).ok_or_else(|| {
+ Error::Sandbox(format!(
+ "docker run did not complete the command (exit {:?}): {}",
+ output.status.code(),
+ String::from_utf8_lossy(&output.stderr).trim()
+ ))
+ })
}
}
@@ -144,20 +154,32 @@
assert_eq!(
args,
vec![
- "run",
- "--rm",
- "-v",
- "/tmp/s/work:/work",
- "-w",
- "/work",
- IMAGE,
- "sh",
- "-c",
- "cargo test 2>&1",
+ "run".to_owned(),
+ "--rm".to_owned(),
+ "-v".to_owned(),
+ "/tmp/s/work:/work".to_owned(),
+ "-w".to_owned(),
+ "/work".to_owned(),
+ IMAGE.to_owned(),
+ "sh".to_owned(),
+ "-c".to_owned(),
+ wrap_exit_marker("cargo test"),
]
);
}
+ #[rstest]
+ // @relation(effect.result-taxonomy, scope=function, role=Verifies)
+ fn run_args_script_completes_with_the_exit_marker() {
+ let args = run_args(Path::new("/w"), &[], "true");
+ let script = args.last().expect("has a script");
+ assert!(
+ script.contains(crate::executor::EXIT_MARKER),
+ "without the marker, a docker-side failure (exit 125) would be \
+ indistinguishable from the command failing"
+ );
+ }
+
#[rstest]
// @relation(effect.execution, effect.toolchains, scope=function, role=Verifies)
fn run_args_binds_each_toolchain_read_only_and_activates_it() {
@@ -165,7 +187,7 @@
let args = run_args(Path::new("/w"), &toolchains, "cargo test");
assert!(args.contains(&"/cache/rust/bin:/toolchains/rust/bin:ro".to_owned()));
let last = args.last().expect("has a command");
- assert!(last.starts_with("export PATH=/toolchains/rust/bin:$PATH; cargo test"));
+ assert!(last.contains("export PATH=/toolchains/rust/bin:$PATH; cargo test"));
}
#[rstest]
crates/ents-effect/src/executor.rs
@@ -124,8 +124,85 @@
format!("export PATH={path}:$PATH; {command}")
}
+/// The sentinel [`wrap_exit_marker`] appends after the wrapped command, so
+/// a CLI-driven backend can tell "the command completed and exited with
+/// this status" apart from "the CLI or its transport failed" — the
+/// distinction `effect.result-taxonomy` requires: a completed command's
+/// exit status is always a result, while an infrastructure failure must
+/// never be recorded as one.
+pub const EXIT_MARKER: &str = "__ENTS_EFFECT_EXIT=";
+
+/// Wrap `command` so its combined output ends with an [`EXIT_MARKER`] line
+/// carrying the command's own exit status, and the wrapping script itself
+/// always exits zero once the command has run to completion.
+///
+/// A backend that shells out to a CLI (`docker run`, `sprite exec`) cannot
+/// trust that CLI's exit status to be the command's: `docker run` exits
+/// 125 for the daemon's own failures, and a transport can die mid-stream
+/// and surface any status at all. With this wrapper, the marker's presence
+/// *is* the completion signal — present means the command ran and the
+/// marker carries its status ([`parse_exit_marker`]); absent means the
+/// sandbox never completed the run, which is [`crate::Error::Sandbox`],
+/// never a recorded result (`effect.result-taxonomy`).
+///
+/// # Examples
+///
+/// ```
+/// use ents_effect::executor::{EXIT_MARKER, wrap_exit_marker};
+///
+/// let script = wrap_exit_marker("cargo test");
+/// assert!(script.contains("cargo test"));
+/// assert!(script.contains(EXIT_MARKER));
+/// ```
+// @relation(effect.result-taxonomy, scope=function)
+#[must_use]
+pub fn wrap_exit_marker(command: &str) -> String {
+ format!("{{\n{command}\n}} 2>&1; printf '\\n{EXIT_MARKER}%s\\n' \"$?\"")
+}
+
+/// Read a completed run's status out of `log`, the combined output of a
+/// [`wrap_exit_marker`]-wrapped command: the last [`EXIT_MARKER`] line wins
+/// (a command may echo the marker itself; the wrapper's own line is always
+/// printed after it), and the marker line is stripped from the returned
+/// [`RunOutput::log`].
+///
+/// `None` means the marker never appeared — the sandbox did not complete
+/// the run, and the caller must report [`crate::Error::Sandbox`] rather
+/// than fabricate a `fail` (`effect.result-taxonomy`).
+///
+/// # Examples
+///
+/// ```
+/// use ents_effect::executor::{RunStatus, parse_exit_marker};
+///
+/// let done = parse_exit_marker("hello\n__ENTS_EFFECT_EXIT=0\n").expect("completed");
+/// assert_eq!(done.status, RunStatus::Pass);
+/// assert_eq!(done.log, "hello");
+///
+/// // No marker: the run never completed; this is not a result.
+/// assert!(parse_exit_marker("transport died").is_none());
+/// ```
+// @relation(effect.result-taxonomy, scope=function)
+#[must_use]
+pub fn parse_exit_marker(log: &str) -> Option<RunOutput> {
+ let idx = log.rfind(EXIT_MARKER)?;
+ let tail = log.get(idx.saturating_add(EXIT_MARKER.len())..)?;
+ let code: i32 = tail.lines().next()?.trim().parse().ok()?;
+ let cleaned = log.get(..idx).unwrap_or_default().trim_end().to_owned();
+ Some(RunOutput {
+ status: if code == 0 {
+ RunStatus::Pass
+ } else {
+ RunStatus::Fail
+ },
+ log: cleaned,
+ })
+}
+
#[cfg(test)]
mod tests {
+ #![allow(clippy::expect_used, reason = "unit test")]
+
use rstest::rstest;
use super::*;
@@ -148,4 +225,53 @@
fn activate_is_identity_with_no_toolchains() {
assert_eq!(activate("run", &[]), "run");
}
+
+ #[rstest]
+ #[case::pass("out\n__ENTS_EFFECT_EXIT=0\n", Some((RunStatus::Pass, "out")))]
+ #[case::fail("out\n__ENTS_EFFECT_EXIT=1\n", Some((RunStatus::Fail, "out")))]
+ #[case::high_exit("__ENTS_EFFECT_EXIT=127\n", Some((RunStatus::Fail, "")))]
+ #[case::no_marker_is_not_a_result("transport died mid-stream", None)]
+ #[case::garbled_marker_is_not_a_result("__ENTS_EFFECT_EXIT=oops\n", None)]
+ #[case::empty("", None)]
+ // @relation(effect.result-taxonomy, scope=function, role=Verifies)
+ fn parse_exit_marker_separates_completion_from_infrastructure(
+ #[case] log: &str,
+ #[case] expected: Option<(RunStatus, &str)>,
+ ) {
+ let parsed = parse_exit_marker(log);
+ match expected {
+ Some((status, cleaned)) => {
+ let run = parsed.expect("marker present means the run completed");
+ assert_eq!(run.status, status);
+ assert_eq!(run.log, cleaned);
+ }
+ None => assert!(parsed.is_none(), "no marker must never become a result"),
+ }
+ }
+
+ #[rstest]
+ // @relation(effect.result-taxonomy, scope=function, role=Verifies)
+ fn parse_exit_marker_takes_the_last_marker_when_the_command_echoes_one() {
+ let log = "echoing __ENTS_EFFECT_EXIT=1 for fun\n__ENTS_EFFECT_EXIT=0\n";
+ let run = parse_exit_marker(log).expect("completed");
+ assert_eq!(run.status, RunStatus::Pass);
+ }
+
+ #[rstest]
+ // @relation(effect.result-taxonomy, scope=function, role=Verifies)
+ fn wrap_then_parse_round_trips_through_a_real_shell() {
+ for (command, expected) in [("true", RunStatus::Pass), ("false", RunStatus::Fail)] {
+ let output = std::process::Command::new("sh")
+ .arg("-c")
+ .arg(wrap_exit_marker(command))
+ .output()
+ .expect("sh runs");
+ // The wrapper itself exits zero once the command has run to
+ // completion, whatever the command's own status was.
+ assert!(output.status.success());
+ let stdout = String::from_utf8_lossy(&output.stdout);
+ let run = parse_exit_marker(&stdout).expect("completed");
+ assert_eq!(run.status, expected, "command {command:?}");
+ }
+ }
}
crates/ents-effect/src/sprite.rs
@@ -24,7 +24,9 @@
use std::process::{Command, Stdio};
use crate::error::{Error, Result};
-use crate::executor::{Executor, RunOutput, RunStatus, SandboxInputs, activate};
+use crate::executor::{
+ Executor, RunOutput, SandboxInputs, activate, parse_exit_marker, wrap_exit_marker,
+};
/// Where the workdir is unpacked inside the Sprite.
pub const WORKDIR: &str = "/work";
@@ -270,7 +272,18 @@
}
}
+/// The in-Sprite script for one run: enter the synced workdir, run the
+/// activated command wrapped by [`wrap_exit_marker`]. A `cd` failure (the
+/// workdir sync silently lost) exits before the marker can print, so it
+/// surfaces as infrastructure, not as a recorded `fail` — the same
+/// discrimination the marker gives a dying transport
+/// (`effect.result-taxonomy`).
+fn run_script(activated: &str) -> String {
+ format!("cd {WORKDIR} || exit 70\n{}", wrap_exit_marker(activated))
+}
+
impl Executor for SpriteExecutor {
+ // @relation(effect.result-taxonomy, scope=function)
fn run(&self, inputs: &SandboxInputs<'_>) -> Result<RunOutput> {
ensure_auth()?;
ensure_sprite(&self.name)?;
@@ -283,10 +296,7 @@
sandbox_dirs.push((toolchain_name.clone(), sandbox_dir));
}
- let script = format!(
- "cd {WORKDIR} && {} 2>&1",
- activate(inputs.command, &sandbox_dirs)
- );
+ let script = run_script(&activate(inputs.command, &sandbox_dirs));
let output = Command::new("sprite")
.args(["exec", "-s", &self.name, "--", "sh", "-c", &script])
.output()
@@ -294,13 +304,18 @@
program: "sprite".to_owned(),
detail: e.to_string(),
})?;
- let log = String::from_utf8_lossy(&output.stdout).into_owned();
- let status = if output.status.success() {
- RunStatus::Pass
- } else {
- RunStatus::Fail
- };
- Ok(RunOutput { status, log })
+ let stdout = String::from_utf8_lossy(&output.stdout);
+ // The marker, not `sprite exec`'s exit status, is the completion
+ // signal: the CLI exits nonzero for its own transport failures too,
+ // which must surface as an infrastructure error, never a recorded
+ // `fail` (`effect.result-taxonomy`).
+ parse_exit_marker(&stdout).ok_or_else(|| {
+ Error::Sandbox(format!(
+ "sprite exec did not complete the command (exit {:?}): {}",
+ output.status.code(),
+ String::from_utf8_lossy(&output.stderr).trim()
+ ))
+ })
}
}
@@ -322,6 +337,17 @@
assert_eq!(sprite_name(seed), expected);
}
+ #[rstest]
+ // @relation(effect.result-taxonomy, scope=function, role=Verifies)
+ fn run_script_gates_the_exit_marker_on_a_successful_cd() {
+ let script = run_script("cargo test");
+ // A failed cd exits before the marker can print, so a lost workdir
+ // sync surfaces as infrastructure, never as a recorded fail.
+ assert!(script.starts_with("cd /work || exit 70\n"));
+ assert!(script.contains(crate::executor::EXIT_MARKER));
+ assert!(script.contains("cargo test"));
+ }
+
#[rstest]
// @relation(effect.execution, scope=function, role=Verifies)
fn unpack_script_kills_orphans_before_wiping_the_destination() {