From 624bc1e02878b0ed387f306bc0d5fdfd19083895 Mon Sep 17 00:00:00 2001 From: qpismont Date: Thu, 17 Sep 2026 19:51:59 +0000 Subject: [PATCH] replace ContainerRuntime CLI to Bollard crate (default: docker socket) limit tool result to open router tool response (with truncated info for ai) Hard kill if graceful shutdown is too long --- .env.example | 2 +- Cargo.lock | 136 ++- Cargo.toml | 2 + Containerfile | 33 +- README.md | 16 +- crates/devcontainer-rs/Cargo.toml | 5 + crates/devcontainer-rs/src/container.rs | 862 ++++++++++++------ crates/devcontainer-rs/src/lib.rs | 12 +- .../herald-server/src/bot_actions/review.rs | 108 ++- crates/herald-server/src/consts.rs | 5 +- crates/herald-server/src/env.rs | 4 - crates/herald-server/src/main.rs | 42 +- crates/herald-server/src/open_router.rs | 48 +- crates/herald-server/src/sandbox/agent.rs | 221 ++++- crates/herald-server/src/sandbox/mod.rs | 104 ++- crates/herald-server/src/text.rs | 81 ++ 16 files changed, 1327 insertions(+), 354 deletions(-) create mode 100644 crates/herald-server/src/text.rs diff --git a/.env.example b/.env.example index d35705b..9f25ae2 100644 --- a/.env.example +++ b/.env.example @@ -20,5 +20,5 @@ RUST_BACKTRACE=1 METRICS_BIND_ADDR= # Sandboxed tool execution -CONTAINER_RUNTIME=docker +# DOCKER_HOST= SANDBOX_MAX_ITERATIONS=8 diff --git a/Cargo.lock b/Cargo.lock index 4444691..2ba0448 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -295,6 +295,49 @@ dependencies = [ "objc2", ] +[[package]] +name = "bollard" +version = "0.21.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "dbe8358268799ebb3e4df23cb9d47f4c72bbc4f5247e2fa6a1bf7b6c0baea220" +dependencies = [ + "base64 0.22.1", + "bollard-stubs", + "bytes", + "futures-core", + "futures-util", + "hex", + "http 1.5.0", + "http-body-util", + "hyper", + "hyper-named-pipe", + "hyper-util", + "hyperlocal", + "log", + "pin-project-lite", + "serde", + "serde_derive", + "serde_json", + "serde_urlencoded", + "thiserror", + "tokio", + "tokio-util", + "tower-service", + "url", + "winapi", +] + +[[package]] +name = "bollard-stubs" +version = "1.53.1-rc.29.3.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "ce412eb6f7096743011dc3cb5c674caeb24ced61d8c498fe07cf7998a4fea889" +dependencies = [ + "serde", + "serde_json", + "serde_repr", +] + [[package]] name = "bumpalo" version = "3.20.3" @@ -432,11 +475,16 @@ name = "devcontainer-rs" version = "0.1.0" dependencies = [ "anyhow", + "bollard", + "bytes", + "futures-util", "serde", "serde_json", + "tar", "tempfile", "thiserror", "tokio", + "tokio-stream", ] [[package]] @@ -488,7 +536,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "39cab71617ae0d63f51a36d69f866391735b51691dbda63cf6f96d042b63efeb" dependencies = [ "libc", - "windows-sys 0.52.0", + "windows-sys 0.61.2", ] [[package]] @@ -508,6 +556,16 @@ version = "2.5.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "da7c62ceae207dd37ea5b845da6a0696c799f85e97da1ab5b7910be3c1c80223" +[[package]] +name = "filetime" +version = "0.2.29" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "5c287a33c7f0a620c38e641e7f60827713987b3c0f26e8ddc9462cc69cf75759" +dependencies = [ + "cfg-if", + "libc", +] + [[package]] name = "find-msvc-tools" version = "0.1.11" @@ -830,6 +888,20 @@ dependencies = [ "want", ] +[[package]] +name = "hyper-named-pipe" +version = "0.1.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "fab3637d6b04a8037af8a266fdf6cf92ea957e8c53981a2bf6136572531025bf" +dependencies = [ + "hex", + "hyper", + "hyper-util", + "pin-project-lite", + "tokio", + "tower-service", +] + [[package]] name = "hyper-tls" version = "0.6.0" @@ -869,6 +941,21 @@ dependencies = [ "tracing", ] +[[package]] +name = "hyperlocal" +version = "0.9.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "986c5ce3b994526b3cd75578e62554abd09f0899d6206de48b3e96ab34ccc8c7" +dependencies = [ + "hex", + "http-body-util", + "hyper", + "hyper-util", + "pin-project-lite", + "tokio", + "tower-service", +] + [[package]] name = "icu_collections" version = "2.3.0" @@ -1777,7 +1864,7 @@ dependencies = [ "errno", "libc", "linux-raw-sys", - "windows-sys 0.52.0", + "windows-sys 0.61.2", ] [[package]] @@ -2066,6 +2153,17 @@ dependencies = [ "serde_core", ] +[[package]] +name = "serde_repr" +version = "0.1.21" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "8d3b1629de253c70a0508c3899572da79ca359fdab27c7920ff00406df418906" +dependencies = [ + "proc-macro2", + "quote", + "syn 3.0.4", +] + [[package]] name = "serde_urlencoded" version = "0.7.1" @@ -2179,6 +2277,17 @@ dependencies = [ "syn 2.0.119", ] +[[package]] +name = "tar" +version = "0.4.46" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "3f6221d9a6003c78398e3b239969f352578258df48c8eb051caadae0015bc840" +dependencies = [ + "filetime", + "libc", + "xattr", +] + [[package]] name = "tempfile" version = "3.27.0" @@ -2189,7 +2298,7 @@ dependencies = [ "getrandom 0.3.4", "once_cell", "rustix", - "windows-sys 0.52.0", + "windows-sys 0.61.2", ] [[package]] @@ -2299,6 +2408,17 @@ dependencies = [ "tokio", ] +[[package]] +name = "tokio-stream" +version = "0.1.19" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "a3d06f0b082ba57c26b79407372e57cf2a1e28124f78e9479fe80322cf53420b" +dependencies = [ + "futures-core", + "pin-project-lite", + "tokio", +] + [[package]] name = "tokio-util" version = "0.7.19" @@ -2796,6 +2916,16 @@ version = "0.6.4" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "3ad82d2a33cdc9674dc7465672f271e096168fcdbe0f799d9e6db8c5892679dc" +[[package]] +name = "xattr" +version = "1.6.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "32e45ad4206f6d2479085147f02bc2ef834ac85886624a23575ae137c8aa8156" +dependencies = [ + "libc", + "rustix", +] + [[package]] name = "yoke" version = "0.8.3" diff --git a/Cargo.toml b/Cargo.toml index 1fef14f..124b5c8 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -8,6 +8,7 @@ resolver = "3" [workspace.dependencies] reqwest = { version = "0.13", default-features = false, features = ["json", "native-tls", "stream"] } tokio = { version = "1.53", features = ["full"] } +tokio-stream = "0.1" tokio-util = "0.7" futures-util = "0.3" serde_json = "1.0" @@ -24,6 +25,7 @@ anyhow = { version = "1", features = ["backtrace"] } thiserror = "2.0" ring = "0.17" hex = "0.4" +bytes = "1.1" metrics = "0.24" metrics-exporter-prometheus = { version = "0.18", default-features = false, features = ["http-listener"] } diff --git a/Containerfile b/Containerfile index 7150f27..c39abf8 100644 --- a/Containerfile +++ b/Containerfile @@ -1,4 +1,4 @@ -FROM rust:1.97-trixie as builder +FROM rust:1.98-trixie as builder WORKDIR /app @@ -10,6 +10,33 @@ RUN cargo build --release --package herald-server FROM debian:trixie-slim +# git clones the pull request. ca-certificates is what every HTTPS call needs +# (Gitea, OpenRouter, git). The shared libraries are the non-base ones the binary +# links against, as reported by `ldd target/release/herald-server`; libc, libm +# and libgcc_s come from the base image. +RUN apt-get update && apt-get install -y --no-install-recommends \ + ca-certificates \ + git \ + libssl3t64 \ + libzstd1 \ + zlib1g \ + && rm -rf /var/lib/apt/lists/* + +# Herald drives the container daemon through its socket (DOCKER_HOST, default +# unix:///var/run/docker.sock), so neither docker nor podman is needed here: the +# compose file mounts the socket. Reaching that socket is what this user needs, +# and the socket is root-equivalent, so either run the container as root or give +# it the socket's group, e.g. `group_add: [""]`. +RUN useradd --create-home --shell /usr/sbin/nologin --uid 10001 herald + WORKDIR /app -COPY --from=builder /app/target/release/herald-server . -CMD [ "./herald-server" ] +COPY --from=builder /app/target/release/herald-server ./herald-server + +# git looks for its configuration under $HOME. +ENV HOME=/home/herald + +USER herald + +# Exec form, so the binary is PID 1 and receives the SIGTERM it handles to shut +# down gracefully. +CMD ["./herald-server"] diff --git a/README.md b/README.md index 235c963..3c69327 100644 --- a/README.md +++ b/README.md @@ -37,8 +37,8 @@ Herald reads its configuration from environment variables (a `.env` file is supp | `METRICS_BIND_ADDR` | *(optional)* Bind address for the Prometheus metrics endpoint (e.g. `0.0.0.0:9100`). If unset, the metrics exporter is disabled. | | `SENTRY_DSN` | *(optional)* Sentry DSN for error tracking | | `RUST_LOG` | *(optional)* Log level, defaults to `info` | -| `CONTAINER_RUNTIME` | *(optional)* Container runtime binary used for the sandbox (`docker` or `podman`). Defaults to `docker` | | `SANDBOX_MAX_ITERATIONS` | *(optional)* Maximum number of tool-calling iterations per sandboxed review. Defaults to `8` | +| `DOCKER_HOST` | *(optional)* Container daemon socket Herald drives, e.g. `unix:///run/user/1000/podman/podman.sock` for a rootless podman. Defaults to `unix:///var/run/docker.sock` | ## Sandboxed reviews @@ -56,8 +56,16 @@ Herald reviews pull requests inside an ephemeral 4. posts the review, anchoring each comment on the added or removed line it refers to, and removes the container and the temporary clone. -The container runtime is selected with `CONTAINER_RUNTIME` (`docker` or -`podman`). The repository must contain a `.devcontainer/devcontainer.json`. +Herald drives the container daemon through its socket: `DOCKER_HOST` (default +`unix:///var/run/docker.sock`), which covers both docker and podman's +Docker-compatible socket. The repository must contain a +`.devcontainer/devcontainer.json`. + +The `runArgs` of that file are read but deliberately **not** passed to the daemon: +they come from an untrusted pull request, and one of them (`--network host`) would +attach the container to another network and quietly +defeat the network cut described below. A devcontainer that relies on them +(`--gpus all`, `--cap-add`, `--shm-size`…) will not get them. Each sandbox is isolated: it gets its own image tag, container and network. The container starts with network access so the `postCreateCommand` / @@ -70,7 +78,7 @@ network and image are removed when the review ends (including on failure). The easiest way to get started is with the provided [Dev Container](https://containers.dev/) (VS Code or Zed with the dev container extension). -Open the project and reopen it in the container — the Rust toolchain is pre-installed. +Open the project and reopen it in the container — the Rust toolchain is pre-installed, along with rootless podman, so sandboxed reviews can be exercised locally: start its API socket with `podman system service --time=0 &` and set `DOCKER_HOST=unix:///run/user/$(id -u)/podman/podman.sock`. **Without Dev Container**, you just need a Rust toolchain: diff --git a/crates/devcontainer-rs/Cargo.toml b/crates/devcontainer-rs/Cargo.toml index 1dd07fb..0294fe6 100644 --- a/crates/devcontainer-rs/Cargo.toml +++ b/crates/devcontainer-rs/Cargo.toml @@ -4,7 +4,12 @@ version = "0.1.0" edition = "2024" [dependencies] +bollard = "0.21" +bytes = { workspace = true } +futures-util = { workspace = true } +tar = "0.4" tokio = { workspace = true } +tokio-stream = { workspace = true } serde = { workspace = true } serde_json = { workspace = true } anyhow = { workspace = true } diff --git a/crates/devcontainer-rs/src/container.rs b/crates/devcontainer-rs/src/container.rs index 54b2f3a..224cf4d 100644 --- a/crates/devcontainer-rs/src/container.rs +++ b/crates/devcontainer-rs/src/container.rs @@ -1,12 +1,15 @@ //! Primitives de cycle de vie de container pour un [`DevContainer`] analysé. //! -//! Ce module invoque un runtime de containers (`docker` ou `podman`) pour construire -//! l'image devcontainer, démarrer un container avec le workspace monté, exécuter les -//! hooks `postCreateCommand` / `postStartCommand` et lancer des commandes à l'intérieur -//! du container en cours d'exécution. +//! Ce module pilote l'API du daemon de containers pour construire l'image +//! devcontainer, démarrer un container avec le workspace monté, exécuter les +//! hooks `postCreateCommand` / `postStartCommand` et lancer des commandes à +//! l'intérieur du container en cours d'exécution. //! -//! Il est volontairement agnostique du runtime : tout binaire exposant l'interface -//! CLI `docker` (y compris `podman`) peut être utilisé via [`ContainerRuntime::new`]. +//! Le daemon est joint par son **socket** (celui de l'hôte, monté dans le +//! container de Herald) : [`ContainerRuntime::connect`] suit `DOCKER_HOST` comme +//! le fait le CLI `docker`, et retombe sur le socket local. Aucun binaire de +//! runtime n'est donc requis dans l'image, et `podman` fonctionne de la même +//! façon dès lors qu'il expose son socket compatible Docker. //! //! # Isolation //! @@ -15,23 +18,59 @@ //! `postCreateCommand` / `postStartCommand` puissent récupérer des dépendances //! (par ex. `npm install`) ; une fois les hooks exécutés, le container est déconnecté //! du réseau pour le reste de sa durée de vie. Chaque commande est bornée par un timeout. +//! +//! Les `runArgs` du `devcontainer.json` sont lus mais pas transmis au daemon : ils +//! viennent du dépôt, donc d'une pull request non fiable, et pourraient rattacher le +//! container à un autre réseau ou lui donner des privilèges qui annuleraient cette +//! isolation. use std::{ + collections::HashMap, + io::{BufWriter, Write}, path::{Path, PathBuf}, - process::Stdio, time::{Duration, SystemTime, UNIX_EPOCH}, }; -use tokio::process::Command; +use bollard::{ + Docker, body_try_stream, + container::LogOutput, + errors::Error as BollardError, + exec::{CreateExecOptions, StartExecOptions, StartExecResults}, + models::{ + BuildInfo, ContainerCreateBody, HostConfig, NetworkCreateRequest, NetworkDisconnectRequest, + }, + query_parameters::{ + BuildImageOptions, CreateContainerOptions, RemoveContainerOptions, RemoveImageOptions, + StartContainerOptions, StopContainerOptions, + }, +}; +use bytes::Bytes; +use futures_util::{Stream, StreamExt}; +use tokio::sync::mpsc; +use tokio_stream::wrappers::ReceiverStream; use crate::DevContainer; +/// Endpoint affiché dans les logs quand `DOCKER_HOST` n'est pas défini. +const DEFAULT_ENDPOINT: &str = "unix:///var/run/docker.sock"; + /// Timeout appliqué aux opérations de build/run/stop/remove. const DEFAULT_COMMAND_TIMEOUT: Duration = Duration::from_secs(600); /// Timeout appliqué aux commandes exécutées dans un container en cours d'exécution. const DEFAULT_EXEC_TIMEOUT: Duration = Duration::from_secs(60); +/// Taille des morceaux du contexte de build envoyés au daemon. +const CONTEXT_CHUNK_SIZE: usize = 64 * 1024; + +/// Nombre de morceaux pouvant attendre dans le canal : c'est ce qui borne la +/// mémoire occupée par le contexte, quelle que soit sa taille. +const CONTEXT_CHUNKS: usize = 4; + +/// Lectures successives du code de sortie d'un exec, et attente entre elles. +const EXIT_CODE_ATTEMPTS: usize = 10; +const EXIT_CODE_DELAY: Duration = Duration::from_millis(20); + /// Résultat d'une commande exécutée dans un container. #[derive(Debug, Clone)] pub struct ExecOutput { @@ -45,67 +84,52 @@ impl ExecOutput { pub fn success(&self) -> bool { self.status == 0 } - - /// Transforme un code de sortie non nul en [`ContainerError::Command`]. - pub fn ensure_success(self, program: &str, args: &[String]) -> Result { - if self.success() { - return Ok(self); - } - - Err(ContainerError::Command { - program: program.to_string(), - args: args.join(" "), - status: self.status, - stderr: self.stderr.trim().to_string(), - }) - } } #[derive(Debug, thiserror::Error)] pub enum ContainerError { - #[error("failed to run `{program}`: {source}")] - Spawn { - program: String, - source: std::io::Error, + /// Le daemon est injoignable, ou a refusé une requête. + #[error("container daemon request failed: {source}")] + Request { + #[from] + source: BollardError, }, - #[error("`{program} {args}` failed with status {status}: {stderr}")] - Command { - program: String, - args: String, - status: i32, - stderr: String, - }, - - #[error("`{program} {args}` timed out after {timeout:?}")] + /// Une opération a dépassé son timeout. + #[error("`{operation}` timed out after {timeout:?}")] Timeout { - program: String, - args: String, + operation: String, timeout: Duration, }, + + /// La construction de l'image a échoué. + #[error("image build failed: {message}")] + Build { message: String }, + + /// Le daemon a répondu autre chose que ce qui était attendu. + #[error("{0}")] + Unexpected(String), } -/// Un binaire de runtime de containers exposant l'interface CLI `docker`. +/// Un client de l'API du daemon de containers. #[derive(Debug, Clone)] pub struct ContainerRuntime { - program: String, + docker: Docker, + endpoint: String, timeout: Duration, } impl ContainerRuntime { - pub fn new(program: impl Into) -> Self { - Self { - program: program.into(), + /// Se connecte au daemon désigné par `DOCKER_HOST`, ou au socket local par défaut. + pub fn connect() -> Result { + let endpoint = + std::env::var("DOCKER_HOST").unwrap_or_else(|_| String::from(DEFAULT_ENDPOINT)); + + Ok(Self { + docker: Docker::connect_with_defaults()?, + endpoint, timeout: DEFAULT_COMMAND_TIMEOUT, - } - } - - pub fn docker() -> Self { - Self::new("docker") - } - - pub fn podman() -> Self { - Self::new("podman") + }) } /// Remplace le timeout appliqué aux opérations de build/run/stop/remove. @@ -114,64 +138,301 @@ impl ContainerRuntime { self } - pub fn program(&self) -> &str { - &self.program + /// Endpoint du daemon, pour les logs. + pub fn endpoint(&self) -> &str { + &self.endpoint } pub fn timeout(&self) -> Duration { self.timeout } - /// Vérifie que le binaire du runtime est présent et répond. + /// Vérifie que le daemon est joignable et répond. pub async fn available(&self) -> bool { - Command::new(&self.program) - .arg("version") - .stdin(Stdio::null()) - .stdout(Stdio::null()) - .stderr(Stdio::null()) - .status() - .await - .map(|status| status.success()) - .unwrap_or(false) + self.docker.ping().await.is_ok() } - /// Exécute le runtime avec les arguments donnés, en capturant stdout/stderr. - /// - /// Seuls les échecs de lancement (spawn) et les timeouts sont des erreurs ; un - /// code non nul est renvoyé dans [`ExecOutput`] afin que les appelants réagissent. - pub async fn run(&self, args: &[String]) -> Result { - self.run_with_timeout(args, self.timeout).await - } - - /// Comme [`run`](Self::run), mais avec un timeout explicite. - pub async fn run_with_timeout( + /// Exécute une requête du daemon en appliquant le timeout des commandes. + async fn request( &self, - args: &[String], - timeout: Duration, - ) -> Result { - let output = Command::new(&self.program) - .args(args) - .stdin(Stdio::null()) - .kill_on_drop(true) - .output(); - - match tokio::time::timeout(timeout, output).await { - Ok(Ok(output)) => Ok(ExecOutput { - status: output.status.code().unwrap_or(-1), - stdout: String::from_utf8_lossy(&output.stdout).into_owned(), - stderr: String::from_utf8_lossy(&output.stderr).into_owned(), - }), - Ok(Err(source)) => Err(ContainerError::Spawn { - program: self.program.clone(), - source, - }), + operation: &str, + request: impl Future>, + ) -> Result { + match tokio::time::timeout(self.timeout, request).await { + Ok(Ok(value)) => Ok(value), + Ok(Err(source)) => Err(ContainerError::Request { source }), Err(_) => Err(ContainerError::Timeout { - program: self.program.clone(), - args: args.join(" "), - timeout, + operation: String::from(operation), + timeout: self.timeout, }), } } + + /// Construit l'image `tag` depuis le contexte `context_dir`. + async fn build_image( + &self, + context_dir: &Path, + dockerfile: &str, + tag: &str, + build_args: &HashMap, + ) -> Result<(), ContainerError> { + let options = BuildImageOptions { + dockerfile: String::from(dockerfile), + t: Some(String::from(tag)), + buildargs: Some(build_args.clone()), + rm: true, + ..Default::default() + }; + + let context = tar_directory_stream(context_dir); + let mut stream = self + .docker + .build_image(options, None, Some(body_try_stream(context))); + + // Le flux porte la progression du build : la dernière erreur signalée par le + // daemon fait échouer l'opération. + let consume = async { + let mut failure = None; + + while let Some(info) = stream.next().await { + let info: BuildInfo = info?; + + if let Some(message) = info.error_detail.and_then(|detail| detail.message) { + failure = Some(message); + } + } + + Ok::<_, BollardError>(failure) + }; + + let failure = match tokio::time::timeout(self.timeout, consume).await { + Ok(Ok(failure)) => failure, + Ok(Err(source)) => return Err(ContainerError::Request { source }), + Err(_) => { + return Err(ContainerError::Timeout { + operation: format!("build image `{tag}`"), + timeout: self.timeout, + }); + } + }; + + match failure { + Some(message) => Err(ContainerError::Build { message }), + None => Ok(()), + } + } + + /// Crée un container, sans le démarrer. + async fn create_container( + &self, + name: &str, + body: ContainerCreateBody, + ) -> Result<(), ContainerError> { + let options = CreateContainerOptions { + name: Some(String::from(name)), + ..Default::default() + }; + + self.request( + "create container", + self.docker.create_container(Some(options), body), + ) + .await?; + + Ok(()) + } + + async fn start_container(&self, name: &str) -> Result<(), ContainerError> { + self.request( + "start container", + self.docker + .start_container(name, None::), + ) + .await?; + + Ok(()) + } + + /// Exécute une commande dans un container et renvoie sa sortie. + async fn exec( + &self, + container: &str, + cmd: &[&str], + user: Option<&str>, + timeout: Duration, + ) -> Result { + let config = CreateExecOptions { + cmd: Some(cmd.iter().map(|arg| String::from(*arg)).collect()), + user: user.map(String::from), + attach_stdout: Some(true), + attach_stderr: Some(true), + ..Default::default() + }; + + let created = self + .request("create exec", self.docker.create_exec(container, config)) + .await?; + + let started = self + .request( + "start exec", + self.docker + .start_exec(&created.id, None::), + ) + .await?; + + let StartExecResults::Attached { mut output, .. } = started else { + return Err(ContainerError::Unexpected(String::from( + "the daemon detached an exec that was not requested as detached", + ))); + }; + + let mut stdout = String::new(); + let mut stderr = String::new(); + + // Le timeout couvre l'exécution de la commande elle-même, pas seulement sa + // mise en place : le processus continue de tourner dans le container, il + // disparaît avec lui. + let collect = async { + while let Some(message) = output.next().await { + match message? { + LogOutput::StdOut { message } | LogOutput::Console { message } => { + stdout.push_str(&String::from_utf8_lossy(&message)); + } + LogOutput::StdErr { message } => { + stderr.push_str(&String::from_utf8_lossy(&message)); + } + LogOutput::StdIn { .. } => {} + } + } + + Ok::<_, BollardError>(()) + }; + + match tokio::time::timeout(timeout, collect).await { + Ok(Ok(())) => {} + Ok(Err(source)) => return Err(ContainerError::Request { source }), + Err(_) => { + return Err(ContainerError::Timeout { + operation: format!("exec {}", cmd.join(" ")), + timeout, + }); + } + } + + // Le code de sortie n'est pas forcément publié au moment où le flux se ferme : + // on laisse au daemon le temps de le renseigner, sans bloquer indéfiniment. + // Sans cela, une commande réussie peut être rapportée en échec (code -1) et + // l'outil renvoie une erreur au modèle à la place du contenu. + let mut inspected = self + .request("inspect exec", self.docker.inspect_exec(&created.id)) + .await?; + + for _ in 0..EXIT_CODE_ATTEMPTS { + if !inspected.running.unwrap_or(false) { + break; + } + + tokio::time::sleep(EXIT_CODE_DELAY).await; + + inspected = self + .request("inspect exec", self.docker.inspect_exec(&created.id)) + .await?; + } + + Ok(ExecOutput { + status: inspected.exit_code.unwrap_or(-1) as i32, + stdout, + stderr, + }) + } + + async fn stop_container(&self, name: &str) -> Result<(), ContainerError> { + let options = StopContainerOptions { + t: Some(1), + ..Default::default() + }; + + self.request( + "stop container", + self.docker.stop_container(name, Some(options)), + ) + .await?; + + Ok(()) + } + + /// Supprime un container, ses volumes anonymes et le processus qui y tourne. + async fn remove_container(&self, name: &str) -> Result<(), ContainerError> { + let options = RemoveContainerOptions { + force: true, + v: true, + ..Default::default() + }; + + self.request( + "remove container", + self.docker.remove_container(name, Some(options)), + ) + .await?; + + Ok(()) + } + + async fn remove_image(&self, tag: &str) -> Result<(), ContainerError> { + let options = RemoveImageOptions { + force: true, + ..Default::default() + }; + + self.request( + "remove image", + self.docker.remove_image(tag, Some(options), None), + ) + .await?; + + Ok(()) + } + + /// Crée le réseau dédié d'une sandbox. + async fn create_network(&self, name: &str) -> Result<(), ContainerError> { + let request = NetworkCreateRequest { + name: String::from(name), + ..Default::default() + }; + + self.request("create network", self.docker.create_network(request)) + .await?; + + Ok(()) + } + + /// Détache un container d'un réseau, ce qui coupe sa connectivité. + async fn disconnect_network( + &self, + network: &str, + container: &str, + ) -> Result<(), ContainerError> { + let request = NetworkDisconnectRequest { + container: String::from(container), + ..Default::default() + }; + + self.request( + "disconnect network", + self.docker.disconnect_network(network, request), + ) + .await?; + + Ok(()) + } + + async fn remove_network(&self, name: &str) -> Result<(), ContainerError> { + self.request("remove network", self.docker.remove_network(name)) + .await?; + + Ok(()) + } } /// Un devcontainer en cours d'exécution. @@ -207,17 +468,9 @@ impl Container { cmd: &[&str], timeout: Duration, ) -> Result { - let mut args = vec!["exec".to_string()]; - - if let Some(user) = &self.remote_user { - args.push("--user".to_string()); - args.push(user.clone()); - } - - args.push(self.name.clone()); - args.extend(cmd.iter().map(|arg| arg.to_string())); - - self.runtime.run_with_timeout(&args, timeout).await + self.runtime + .exec(&self.name, cmd, self.remote_user.as_deref(), timeout) + .await } /// Exécute un script shell dans le container via `sh -c`. @@ -227,37 +480,21 @@ impl Container { /// Arrête le container. pub async fn stop(&self) -> Result<(), ContainerError> { - let args = vec!["stop".to_string(), self.name.clone()]; - self.runtime - .run(&args) - .await? - .ensure_success(self.runtime.program(), &args)?; - Ok(()) + self.runtime.stop_container(&self.name).await } /// Supprime le container, son réseau et son image. /// /// La suppression du réseau et de l'image est best-effort : ils peuvent déjà être absents. pub async fn remove(&self) -> Result<(), ContainerError> { - let args = vec![ - "rm".to_string(), - "-f".to_string(), - "-v".to_string(), - self.name.clone(), - ]; - self.runtime - .run(&args) - .await? - .ensure_success(self.runtime.program(), &args)?; + self.runtime.remove_container(&self.name).await?; if let Some(network) = &self.network { - let args = vec!["network".to_string(), "rm".to_string(), network.clone()]; - let _ = self.runtime.run(&args).await; + let _ = self.runtime.remove_network(network).await; } if let Some(image) = &self.image { - let args = vec!["rmi".to_string(), image.clone()]; - let _ = self.runtime.run(&args).await; + let _ = self.runtime.remove_image(image).await; } Ok(()) @@ -279,74 +516,6 @@ impl DevContainer { format!("{}:{}", self.image_name(), unique_suffix()) } - /// Arguments passés à `docker build` (tout ce qui suit le verbe `build`). - pub fn build_args(&self, image_tag: &str) -> Vec { - let context = self - .container_file_path - .parent() - .unwrap_or_else(|| Path::new(".")); - - let mut args = vec![ - "-f".to_string(), - self.container_file_path.display().to_string(), - "-t".to_string(), - image_tag.to_string(), - ]; - - for (key, value) in &self.build_args { - args.push("--build-arg".to_string()); - args.push(format!("{key}={value}")); - } - - args.push(context.display().to_string()); - args - } - - /// Arguments passés à `docker run` (tout ce qui suit le verbe `run`). - pub fn run_args( - &self, - workspace_dir: &Path, - container_name: &str, - image_tag: &str, - network: Option<&str>, - ) -> Vec { - let workspace_folder = self.workspace_folder(); - - let mut args = vec![ - "-d".to_string(), - "--name".to_string(), - container_name.to_string(), - "-v".to_string(), - format!("{}:{}", workspace_dir.display(), workspace_folder), - "-w".to_string(), - workspace_folder.to_string(), - ]; - - if let Some(network) = network { - args.push("--network".to_string()); - args.push(network.to_string()); - } - - if let Some(user) = &self.remote_user { - args.push("--user".to_string()); - args.push(user.clone()); - } - - for (key, value) in &self.container_env { - args.push("-e".to_string()); - args.push(format!("{key}={value}")); - } - - args.extend(self.run_args.iter().cloned()); - - args.push(image_tag.to_string()); - // Maintient le container en vie pour pouvoir y exécuter `exec`. - args.push("sleep".to_string()); - args.push("infinity".to_string()); - - args - } - /// Dossier de workspace dans le container, par défaut `/workspaces/workspace`. pub fn workspace_folder(&self) -> String { self.workspace_folder @@ -366,20 +535,28 @@ impl DevContainer { runtime: &ContainerRuntime, image_tag: &str, ) -> Result<(), ContainerError> { - let mut args = vec!["build".to_string()]; - args.extend(self.build_args(image_tag)); + let context = self.container_file_path.parent().ok_or_else(|| { + ContainerError::Unexpected(String::from( + "the devcontainer file path has no parent directory", + )) + })?; + + let dockerfile = self + .container_file_path + .file_name() + .and_then(|name| name.to_str()) + .ok_or_else(|| { + ContainerError::Unexpected(String::from( + "the devcontainer file name is not valid UTF-8", + )) + })?; runtime - .run(&args) - .await? - .ensure_success(runtime.program(), &args)?; - - Ok(()) + .build_image(context, dockerfile, image_tag, &self.build_args) + .await } - /// Construit l'image, démarre le container avec le workspace monté, exécute les - /// hooks `postCreateCommand` / `postStartCommand` avec accès au réseau, puis - /// déconnecte le container du réseau. + /// Construit l'image, démarre le container, exécute les hooks puis coupe le réseau. /// /// En cas d'échec, le container, le réseau et l'image sont nettoyés avant de /// renvoyer l'erreur, afin qu'aucune ressource ne soit laissée en place. @@ -395,28 +572,50 @@ impl DevContainer { let network = format!("{name}-net"); // Réseau dédié afin de pouvoir couper la connectivité après les hooks. - let args = vec!["network".to_string(), "create".to_string(), network.clone()]; - if let Err(err) = runtime - .run(&args) - .await - .and_then(|out| out.ensure_success(runtime.program(), &args)) - { - let _ = runtime.run(&["rmi".to_string(), image_tag]).await; + if let Err(err) = runtime.create_network(&network).await { + let _ = runtime.remove_image(&image_tag).await; return Err(err); } - let mut args = vec!["run".to_string()]; - args.extend(self.run_args(workspace_dir, &name, &image_tag, Some(&network))); + let workspace_folder = self.workspace_folder(); + let body = ContainerCreateBody { + image: Some(image_tag.clone()), + cmd: Some(vec![String::from("sleep"), String::from("infinity")]), + user: self.remote_user.clone(), + env: Some( + self.container_env + .iter() + .map(|(key, value)| format!("{key}={value}")) + .collect(), + ), + working_dir: Some(workspace_folder.clone()), + host_config: Some(HostConfig { + binds: Some(vec![format!( + "{}:{workspace_folder}", + workspace_dir.display() + )]), + network_mode: Some(network.clone()), + // Le clone est monté depuis un chemin de l'hôte. Sur une distribution + // à SELinux enforcing, ce chemin n'a pas le label attendu et l'accès + // est refusé (EACCES), ce qui fait échouer tous les outils de la + // sandbox. C'est le compromis inverse de l'alternative `:Z` sur le + // montage, qui re-labellise le clone et garde le confinement SELinux. + security_opt: Some(vec![String::from("label=disable")]), + ..Default::default() + }), + ..Default::default() + }; - if let Err(err) = runtime - .run(&args) - .await - .and_then(|out| out.ensure_success(runtime.program(), &args)) - { - let _ = runtime - .run(&["network".to_string(), "rm".to_string(), network]) - .await; - let _ = runtime.run(&["rmi".to_string(), image_tag]).await; + if let Err(err) = runtime.create_container(&name, body).await { + let _ = runtime.remove_network(&network).await; + let _ = runtime.remove_image(&image_tag).await; + return Err(err); + } + + if let Err(err) = runtime.start_container(&name).await { + let _ = runtime.remove_container(&name).await; + let _ = runtime.remove_network(&network).await; + let _ = runtime.remove_image(&image_tag).await; return Err(err); } @@ -436,17 +635,7 @@ impl DevContainer { } // Coupe l'accès réseau pour le reste de la durée de vie de la sandbox. - let args = vec![ - "network".to_string(), - "disconnect".to_string(), - network, - container.name.clone(), - ]; - if let Err(err) = runtime - .run(&args) - .await - .and_then(|out| out.ensure_success(runtime.program(), &args)) - { + if let Err(err) = runtime.disconnect_network(&network, container.name()).await { let _ = container.remove().await; return Err(err); } @@ -466,16 +655,81 @@ impl DevContainer { let output = container .exec_with_timeout(&["sh", "-c", command], timeout) .await?; - output.ensure_success( - container.runtime.program(), - &["exec".to_string(), command.clone()], - )?; + + if !output.success() { + return Err(ContainerError::Unexpected(format!( + "hook `{command}` failed with status {}: {}", + output.status, + output.stderr.trim() + ))); + } } Ok(()) } } +/// Empaquette le contexte de build dans un tar, **en flux**. +/// +/// Le CLI `docker build` empaquette le contexte puis l'envoie ; le faire d'un coup +/// chargerait tout le dossier en mémoire, ce qui devient intenable dès que le +/// `.devcontainer` embarque des binaires. Ici l'archive est écrite par une tâche +/// bloquante et poussée morceau par morceau, avec la contre-pression du canal : +/// si le daemon lit lentement, l'empaquetage ralentit. +fn tar_directory_stream( + dir: &Path, +) -> impl Stream> + Send + 'static { + let (sender, receiver) = mpsc::channel(CONTEXT_CHUNKS); + let failures = sender.clone(); + let dir = dir.to_path_buf(); + + tokio::task::spawn_blocking(move || { + let writer = BufWriter::with_capacity(CONTEXT_CHUNK_SIZE, ChannelWriter { sender }); + let mut builder = tar::Builder::new(writer); + + let result = builder + .append_dir_all(".", &dir) + .and_then(|()| builder.finish()) + // `finish` écrit les blocs de fin d'archive ; il reste à vider le tampon + // pour que le daemon reçoive tout, y compris une archive vide. + .and_then(|()| builder.get_mut().flush()); + + if let Err(error) = result { + // L'échec est remonté au daemon par le flux : sinon il ne verrait qu'un + // tar tronqué et signalerait une erreur incompréhensible. + let _ = failures.blocking_send(Err(error)); + } + }); + + ReceiverStream::new(receiver) +} + +/// Écrit les morceaux du tar dans un canal, en attendant qu'il se vide. +/// +/// Utilisé depuis une tâche bloquante, seul contexte où `blocking_send` est autorisé. +struct ChannelWriter { + sender: mpsc::Sender>, +} + +impl Write for ChannelWriter { + fn write(&mut self, buf: &[u8]) -> std::io::Result { + self.sender + .blocking_send(Ok(Bytes::copy_from_slice(buf))) + .map_err(|_| { + std::io::Error::new( + std::io::ErrorKind::BrokenPipe, + "the daemon dropped the build context", + ) + })?; + + Ok(buf.len()) + } + + fn flush(&mut self) -> std::io::Result<()> { + Ok(()) + } +} + /// Nettoie une chaîne pour qu'elle puisse servir de nom d'image/container docker. fn sanitize(input: &str) -> String { let sanitized: String = input @@ -550,8 +804,7 @@ mod tests { }, "workspaceFolder": "/workspaces/my-project", "containerEnv": { "RUST_LOG": "debug" }, - "remoteUser": "dev", - "runArgs": ["--userns=keep-id"] + "remoteUser": "dev" }"#, ) .unwrap(); @@ -575,46 +828,13 @@ mod tests { } #[test] - fn build_args_include_dockerfile_tag_build_args_and_context() { + fn container_name_is_sanitized() { let dir = tempfile::tempdir().unwrap(); let dc = devcontainer(dir.path()); - let args = dc.build_args("devcontainer-rs/my-project:test"); - assert_eq!(args[0], "-f"); - assert!(args[1].ends_with("Dockerfile")); - assert_eq!(args[2], "-t"); - assert_eq!(args[3], "devcontainer-rs/my-project:test"); - assert!(args.contains(&"--build-arg".to_string())); - assert!(args.contains(&"VERSION=1".to_string())); - assert_eq!(args.last().unwrap(), &dir.path().display().to_string()); - } - - #[test] - fn run_args_mount_workspace_and_keep_alive() { - let dir = tempfile::tempdir().unwrap(); - let dc = devcontainer(dir.path()); - let workspace = Path::new("/tmp/clone"); - let args = dc.run_args( - workspace, - "devcontainer-rs-my-project-42", - "devcontainer-rs/my-project:test", - Some("sandbox-net"), - ); - - assert!(args.contains(&"-d".to_string())); - assert!(args.contains(&"--name".to_string())); - assert!(args.contains(&"devcontainer-rs-my-project-42".to_string())); - assert!(args.contains(&"/tmp/clone:/workspaces/my-project".to_string())); - assert!(args.contains(&"--network".to_string())); - assert!(args.contains(&"sandbox-net".to_string())); - assert!(args.contains(&"--user".to_string())); - assert!(args.contains(&"dev".to_string())); - assert!(args.contains(&"RUST_LOG=debug".to_string())); - assert!(args.contains(&"--userns=keep-id".to_string())); - assert!(args.contains(&"devcontainer-rs/my-project:test".to_string())); - assert_eq!( - &args[args.len() - 2..], - &["sleep".to_string(), "infinity".to_string()] + assert!( + dc.container_name() + .starts_with("devcontainer-rs-my-project-") ); } @@ -627,23 +847,87 @@ mod tests { assert_eq!(normalize(Path::new("/workspaces/../../etc/passwd")), None); } - #[tokio::test] - async fn run_captures_output() { - let runtime = ContainerRuntime::new("echo"); - let output = runtime.run(&["hello".to_string()]).await.unwrap(); + /// Rassemble le flux du contexte en un tar complet, pour l'inspecter. + async fn context_tar(dir: &Path) -> Vec { + let mut tar = Vec::new(); + let mut chunks = tar_directory_stream(dir); - assert!(output.success()); - assert_eq!(output.stdout.trim(), "hello"); + while let Some(chunk) = chunks.next().await { + tar.extend_from_slice(&chunk.unwrap()); + } + + tar } #[tokio::test] - async fn run_times_out_and_kills_the_process() { - let runtime = ContainerRuntime::new("sleep"); - let err = runtime - .run_with_timeout(&["10".to_string()], Duration::from_millis(50)) - .await - .unwrap_err(); + async fn tar_context_holds_the_devcontainer_files() { + let dir = tempfile::tempdir().unwrap(); + let devcontainer_dir = dir.path().join(".devcontainer"); + fs::create_dir(&devcontainer_dir).unwrap(); + fs::write(devcontainer_dir.join("Dockerfile"), "FROM alpine\n").unwrap(); + fs::write(devcontainer_dir.join("devcontainer.json"), "{}").unwrap(); - assert!(matches!(err, ContainerError::Timeout { .. })); + let tar = context_tar(&devcontainer_dir).await; + + let mut archive = tar::Archive::new(tar.as_slice()); + let names = archive + .entries() + .unwrap() + .map(|entry| entry.unwrap().path().unwrap().display().to_string()) + .collect::>(); + + assert!(names.iter().any(|name| name.ends_with("Dockerfile"))); + assert!(names.iter().any(|name| name.ends_with("devcontainer.json"))); + } + + #[tokio::test] + async fn tar_context_keeps_the_executable_bit() { + let dir = tempfile::tempdir().unwrap(); + let script = dir.path().join("setup.sh"); + fs::write(&script, "#!/bin/sh\n").unwrap(); + + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + fs::set_permissions(&script, fs::Permissions::from_mode(0o755)).unwrap(); + } + + let tar = context_tar(dir.path()).await; + + let mut archive = tar::Archive::new(tar.as_slice()); + let mode = archive + .entries() + .unwrap() + .map(|entry| entry.unwrap()) + .find(|entry| entry.path().unwrap().ends_with("setup.sh")) + .unwrap() + .header() + .mode() + .unwrap(); + + assert_eq!(mode & 0o111, 0o111); + } + + #[tokio::test] + async fn tar_context_is_sent_in_chunks() { + let dir = tempfile::tempdir().unwrap(); + fs::write( + dir.path().join("big.bin"), + vec![0_u8; CONTEXT_CHUNK_SIZE * 3], + ) + .unwrap(); + + let mut chunks = tar_directory_stream(dir.path()); + let mut count = 0; + + while let Some(chunk) = chunks.next().await { + assert!(chunk.unwrap().len() <= CONTEXT_CHUNK_SIZE); + count += 1; + } + + assert!( + count > 1, + "the context should be streamed, not buffered in one piece" + ); } } diff --git a/crates/devcontainer-rs/src/lib.rs b/crates/devcontainer-rs/src/lib.rs index 67914b9..c8f39bc 100644 --- a/crates/devcontainer-rs/src/lib.rs +++ b/crates/devcontainer-rs/src/lib.rs @@ -38,6 +38,12 @@ pub struct DevContainerSchema { #[serde(rename = "remoteUser", default)] pub remote_user: Option, + /// Arguments passés à `docker run`. + /// + /// Lus pour rester fidèle au format `devcontainer.json`, mais + /// **délibérément pas transmis** au runtime : ils viennent d'une pull + /// request non fiable et pourraient casser l'isolation de la sandbox (voir + /// [`DevContainer::run_args`]). #[serde(rename = "runArgs", default)] pub run_args: Vec, } @@ -52,7 +58,6 @@ pub struct DevContainer { pub post_create_command: Option, pub post_start_command: Option, pub remote_user: Option, - pub run_args: Vec, } #[derive(Debug, thiserror::Error)] @@ -114,7 +119,6 @@ impl TryFrom<(DevContainerSchema, PathBuf)> for DevContainer { post_create_command: schema.post_create_command, post_start_command: schema.post_start_command, remote_user: schema.remote_user, - run_args: schema.run_args, }) } } @@ -203,8 +207,7 @@ mod tests { "containerEnv": { "RUST_LOG": "debug" }, - "remoteUser": "dev", - "runArgs": ["--userns=keep-id"] + "remoteUser": "dev" }"#, ) .unwrap(); @@ -217,7 +220,6 @@ mod tests { assert_eq!(config.container_env.get("RUST_LOG").unwrap(), "debug"); assert_eq!(config.workspace_folder.as_deref(), Some("/workspace")); assert_eq!(config.remote_user.as_deref(), Some("dev")); - assert_eq!(config.run_args, vec!["--userns=keep-id"]); } #[test] diff --git a/crates/herald-server/src/bot_actions/review.rs b/crates/herald-server/src/bot_actions/review.rs index 83d9424..61ca7c4 100644 --- a/crates/herald-server/src/bot_actions/review.rs +++ b/crates/herald-server/src/bot_actions/review.rs @@ -1,3 +1,5 @@ +use std::str::FromStr; + use tracing::{info, instrument, warn}; use crate::{ @@ -7,6 +9,7 @@ use crate::{ metrics, open_router::{OpenRouterClient, Tool}, sandbox::{Sandbox, SandboxConfig, agent}, + text::excerpt, }; #[instrument(skip(gitea_api, open_router_client, sandbox_config, tools, review_payload))] @@ -59,7 +62,7 @@ pub async fn exec_review( .replace("{comment}", &review_payload.comment.body) .replace("{changes}", &changes); - let (message, cost) = run_sandboxed_review( + let (mut review_result, cost) = run_sandboxed_review( gitea_api, open_router_client, sandbox_config, @@ -69,7 +72,6 @@ pub async fn exec_review( ) .await?; - let mut review_result = serde_json::from_str::(&message)?; resolve_review_sides(&mut review_result, &changed_lines); review_result.cost = cost; @@ -112,6 +114,10 @@ pub async fn exec_review( /// Runs the review inside a sandbox container, letting the model explore the /// repository with tools before answering. +/// +/// The answer of the model is parsed by [`ReviewResult::from_str`], which the +/// agent loop enforces: an answer that is not a review is sent back to the model +/// for correction. async fn run_sandboxed_review( gitea_api: &GiteaAPI, open_router_client: &OpenRouterClient, @@ -119,7 +125,7 @@ async fn run_sandboxed_review( tools: Vec, review_payload: &ReviewPayload, bot_request: &str, -) -> anyhow::Result<(String, Option)> { +) -> anyhow::Result<(ReviewResult, Option)> { let repo_url = gitea_api.repo_clone_url(&review_payload.repository.full_name); let sandbox = Sandbox::create( @@ -151,7 +157,7 @@ async fn run_sandboxed_review( "Sandboxed review finished" ); - Ok((result.message, result.cost)) + Ok((result.answer, result.cost)) } fn review_result_to_markdown(review_result: &ReviewResult) -> String { @@ -452,6 +458,61 @@ fn format_line_numbers(lines: &[u64]) -> String { .join(", ") } +/// Number of characters of a model answer kept in the logs when it cannot be +/// parsed. +const MAX_LOGGED_ANSWER: usize = 500; + +impl FromStr for ReviewResult { + type Err = anyhow::Error; + + /// Parses the review the model answered with. + /// + /// This is the contract the agent loop enforces: a rejected answer is sent + /// back to the model, with the reason, so that it can correct itself. + /// + /// The contract is a raw JSON object, but models sometimes wrap it in a + /// markdown code fence or surround it with a sentence: the object is then + /// extracted from the answer before failing, and the answer is logged so a + /// breach of the contract can be diagnosed. + fn from_str(message: &str) -> Result { + let error = match serde_json::from_str::(message) { + Ok(review_result) => return Ok(review_result), + Err(error) => error, + }; + + // A markdown code fence or a sentence around the object is tolerated, with + // a warning: the contract asks for a raw JSON object. + if let Some(json) = json_object(message) + && let Ok(review_result) = serde_json::from_str::(json) + { + warn!( + "Model answer is not a raw JSON object, it was extracted from the surrounding text" + ); + + return Ok(review_result); + } + + // The reason of the rejection is logged along with the answer: without it, + // a broken answer is impossible to diagnose. + warn!( + answer = %excerpt(message, MAX_LOGGED_ANSWER), + reason = %error, + "Model answer is not the expected JSON" + ); + + anyhow::bail!("the answer is not valid JSON: {error}") + } +} + +/// Returns the outermost `{...}` of an answer, which ignores a markdown code +/// fence or any text around it. +fn json_object(message: &str) -> Option<&str> { + let start = message.find('{')?; + let end = message.rfind('}')?; + + (start < end).then(|| &message[start..=end]) +} + /// Resolves the side each review is anchored on and drops the reviews that do /// not match a line the pull request changes. /// @@ -824,6 +885,45 @@ mod tests { assert_eq!(review_result.reviews[0].side, Some(ReviewSide::Removed)); } + #[test] + fn a_raw_answer_is_parsed() { + let answer = r#"{"reviews": [], "comment": "ok"}"#; + + assert_eq!(answer.parse::().unwrap().comment, "ok"); + } + + #[test] + fn a_fenced_answer_is_extracted() { + let answer = concat!( + "Voici ma review :\n", + "```json\n", + "{\"reviews\": [], \"comment\": \"rien à signaler\"}\n", + "```\n" + ); + + assert_eq!( + answer.parse::().unwrap().comment, + "rien à signaler" + ); + } + + #[test] + fn a_sentence_around_the_object_is_ignored() { + let answer = r#"Rien à signaler. {"reviews": [], "comment": "ok"} Bonne journée !"#; + + assert_eq!(answer.parse::().unwrap().comment, "ok"); + } + + #[test] + fn an_answer_without_json_is_rejected() { + assert!("Je n'ai rien relevé.".parse::().is_err()); + } + + #[test] + fn an_answer_that_is_an_object_but_not_a_review_is_rejected() { + assert!(r#"{"message": "LGTM"}"#.parse::().is_err()); + } + #[test] fn odd_sides_from_the_model_are_tolerated() { let changed_lines = parse_changed_lines(DIFF); diff --git a/crates/herald-server/src/consts.rs b/crates/herald-server/src/consts.rs index 3d87a6a..4ec599e 100644 --- a/crates/herald-server/src/consts.rs +++ b/crates/herald-server/src/consts.rs @@ -15,7 +15,8 @@ pub const SANDBOX_SYSTEM_PROMPT: &str = " tools (ls, read_file, grep, find) to explore the code and gather the context you need before answering. Paths are relative to the repository root. - When you have enough information, answer with the requested JSON only. + When you have enough information, answer with the requested JSON only, as a raw + JSON object: no markdown code fence, nothing before or after it. "; pub const REVIEW_PROMPT: &str = " @@ -39,6 +40,8 @@ pub const REVIEW_PROMPT: &str = " Return your feedback, in french, with only this json format, reviews must contain each review All fields are mandatory. + Answer with the raw json object only: no markdown code fence, no text before or + after it. (filename field must contain the full path with extension; line must be one of the listed line numbers for that file, and side must be \"added\" when the line comes from the `added` list or \"removed\" when it comes from the `removed` list) diff --git a/crates/herald-server/src/env.rs b/crates/herald-server/src/env.rs index 7cdf68d..a7752de 100644 --- a/crates/herald-server/src/env.rs +++ b/crates/herald-server/src/env.rs @@ -12,7 +12,6 @@ pub struct EnvConfig { pub gitea_token: String, pub gitea_timeout: u64, pub metrics_bind_addr: Option, - pub container_runtime: String, pub sandbox_max_iterations: usize, } @@ -27,8 +26,6 @@ pub fn load_config() -> anyhow::Result { let gitea_token = try_get_env("GITEA_TOKEN")?; let gitea_timeout = try_get_env("GITEA_TIMEOUT")?.parse()?; let metrics_bind_addr = std::env::var("METRICS_BIND_ADDR").ok(); - let container_runtime = - std::env::var("CONTAINER_RUNTIME").unwrap_or_else(|_| "docker".to_string()); let sandbox_max_iterations = std::env::var("SANDBOX_MAX_ITERATIONS") .ok() .and_then(|value| value.parse().ok()) @@ -45,7 +42,6 @@ pub fn load_config() -> anyhow::Result { gitea_token, gitea_timeout, metrics_bind_addr, - container_runtime, sandbox_max_iterations, }) } diff --git a/crates/herald-server/src/main.rs b/crates/herald-server/src/main.rs index d6d4b0b..c22d24f 100644 --- a/crates/herald-server/src/main.rs +++ b/crates/herald-server/src/main.rs @@ -1,3 +1,5 @@ +use std::time::Duration; + use crate::{ bot::Bot, gitea::{GiteaAPI, WebhookType}, @@ -23,6 +25,13 @@ mod metrics; mod open_router; mod sandbox; mod state; +mod text; + +/// Délai laissé aux reviews en cours et aux serveurs pour s'arrêter proprement. +/// +/// Sans lui, une review prise dans une sandbox récalcitrante garderait le processus +/// en vie jusqu'à ce que le superviseur le tue. +const SHUTDOWN_TIMEOUT: Duration = Duration::from_secs(60); fn main() -> anyhow::Result<()> { dotenv().ok(); @@ -79,14 +88,14 @@ async fn run() -> anyhow::Result<()> { let shutdown = CancellationToken::new(); let sandbox = SandboxConfig { - runtime: devcontainer_rs::ContainerRuntime::new(config.container_runtime.clone()), + runtime: devcontainer_rs::ContainerRuntime::connect()?, max_iterations: config.sandbox_max_iterations, }; if !sandbox.runtime.available().await { warn!( - runtime = sandbox.runtime.program(), - "Container runtime is not available, every review will fail" + endpoint = sandbox.runtime.endpoint(), + "Container daemon is not reachable, every review will fail" ); } @@ -119,11 +128,28 @@ async fn run() -> anyhow::Result<()> { anyhow::Ok(()) }; - tokio::try_join!( - bot.start(rx, shutdown.clone()), - api::start(app_state, shutdown.clone()), - signal - )?; + let shutdown_deadline = async { + shutdown.cancelled().await; + tokio::time::sleep(SHUTDOWN_TIMEOUT).await; + }; + + tokio::select! { + result = async { + tokio::try_join!( + bot.start(rx, shutdown.clone()), + api::start(app_state, shutdown.clone()), + signal + ) + } => { + result?; + } + () = shutdown_deadline => { + warn!( + timeout = ?SHUTDOWN_TIMEOUT, + "Shutdown did not finish in time, exiting anyway" + ); + } + } info!("Shutdown complete"); diff --git a/crates/herald-server/src/open_router.rs b/crates/herald-server/src/open_router.rs index 8709091..3c5de7a 100644 --- a/crates/herald-server/src/open_router.rs +++ b/crates/herald-server/src/open_router.rs @@ -17,8 +17,24 @@ use tracing::instrument; /// OpenRouter API root, version prefix included. const BASE_URL: &str = "https://openrouter.ai/api/v1"; -/// The model decides on its own which tool to call. -const TOOL_CHOICE_AUTO: &str = "auto"; +/// What the model is allowed to do on a given turn. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum ToolChoice { + /// The model decides whether to call a tool. + Auto, + /// The model must answer, without calling any tool. The tools stay declared, + /// so the conversation remains the same shape as on the other turns. + None, +} + +impl ToolChoice { + fn as_str(self) -> &'static str { + match self { + Self::Auto => "auto", + Self::None => "none", + } + } +} /// Result of a completion that may contain tool calls. pub struct ToolChatResult { @@ -250,13 +266,18 @@ impl OpenRouterClient { /// Sends a conversation with tool definitions and returns either a final /// message or the tool calls requested by the model. + /// + /// The conversation is borrowed rather than taken by value: the caller keeps + /// appending to it between iterations, and a copy per iteration would be pure + /// waste. #[instrument(skip(self, messages, tools), err)] pub async fn chat_with_tools( &self, - messages: Vec, - tools: Vec, + messages: &[Message], + tools: &[Tool], + tool_choice: ToolChoice, ) -> anyhow::Result { - let response = self.complete(&messages, &tools).await?; + let response = self.complete(messages, tools, tool_choice).await?; let cost = response.usage.and_then(|usage| usage.cost); let message = response @@ -273,13 +294,18 @@ impl OpenRouterClient { }) } - async fn complete(&self, messages: &[Message], tools: &[Tool]) -> anyhow::Result { + async fn complete( + &self, + messages: &[Message], + tools: &[Tool], + tool_choice: ToolChoice, + ) -> anyhow::Result { let request = ChatRequest { model: &self.model, messages, reasoning: Reasoning { enabled: true }, tools, - tool_choice: TOOL_CHOICE_AUTO, + tool_choice: tool_choice.as_str(), }; let response = self @@ -364,7 +390,7 @@ mod tests { messages: &messages, reasoning: Reasoning { enabled: true }, tools: &tools, - tool_choice: TOOL_CHOICE_AUTO, + tool_choice: ToolChoice::Auto.as_str(), }) .unwrap(); @@ -375,6 +401,12 @@ mod tests { assert_eq!(request["messages"][0]["role"], "user"); } + #[test] + fn a_forced_answer_turn_disables_the_tools() { + assert_eq!(ToolChoice::None.as_str(), "none"); + assert_eq!(ToolChoice::Auto.as_str(), "auto"); + } + #[test] fn response_parses_tool_calls_and_cost() { let response: ChatResponse = serde_json::from_value(json!({ diff --git a/crates/herald-server/src/sandbox/agent.rs b/crates/herald-server/src/sandbox/agent.rs index bf9e1e2..7de2702 100644 --- a/crates/herald-server/src/sandbox/agent.rs +++ b/crates/herald-server/src/sandbox/agent.rs @@ -5,18 +5,58 @@ //! back, until the model produces a final message or the iteration budget is //! exhausted. +use std::str::FromStr; +use std::time::Instant; + use anyhow::Context; use serde_json::Value; -use tracing::{debug, warn}; +use tracing::{info, warn}; use crate::{ - open_router::{Message, OpenRouterClient, Role, Tool, ToolCall}, + open_router::{Message, OpenRouterClient, Role, Tool, ToolCall, ToolChoice}, sandbox::{Sandbox, tools}, + text::{excerpt, truncate_bytes}, }; +/// Number of characters of the arguments of a tool call kept in the logs. +const MAX_LOGGED_ARGUMENTS: usize = 200; + +/// Number of characters of a tool result kept in the logs. +/// +/// Sizes alone make a broken tool invisible: a command that fails returns a short +/// `error: …` instead of the expected content, which is exactly what this excerpt +/// makes obvious. +const MAX_LOGGED_OUTPUT: usize = 120; + +/// Maximum number of bytes of a tool result handed to the model. +/// +/// A result stays in the conversation and is re-sent on every following +/// iteration, so an unbounded one (a whole file, a match on every line) inflates +/// the context and the cost of the whole rest of the run — and can overflow the +/// model context window outright. +const MAX_TOOL_OUTPUT_BYTES: usize = 32 * 1024; + +/// Asked of the model when its answer does not satisfy the caller's contract. +const RETRY_PROMPT: &str = " + Your answer is not valid: {error} + + Answer again with the requested format only: a raw json object, without any + markdown code fence and without any text before or after it. +"; + +/// Asked of the model on the last turn, in place of one more exploration turn. +/// +/// Without it, a model still exploring on its last turn would ask for another +/// tool, and the run would end on the iteration budget with no answer at all. +const FINAL_PROMPT: &str = " + You are out of turns: this is your last one. Answer now, with the requested + format, from what you have already gathered, without calling any tool. +"; + /// Final output of an agent run. -pub struct AgentResult { - pub message: String, +pub struct AgentResult { + /// Answer of the model, parsed into the type the caller asked for. + pub answer: A, pub cost: Option, pub iterations: usize, } @@ -26,25 +66,64 @@ pub struct AgentResult { /// /// `tool_definitions` is the set of tools the model may call; it is selected by /// the caller based on the webhook action (see [`tools::for_webhook`]). -pub async fn run( +/// +/// The answer of the model is parsed into `A`, which is how the caller states the +/// format it expects: while [`FromStr`] rejects the answer, the error is sent back +/// to the model so that it can correct itself, which costs an iteration. The +/// parsing happens here because that is where the conversation and the remaining +/// budget are at hand. +pub async fn run( open_router: &OpenRouterClient, sandbox: &Sandbox, tool_definitions: Vec, system_prompt: &str, user_prompt: &str, max_iterations: usize, -) -> anyhow::Result { +) -> anyhow::Result> +where + A: FromStr, + A::Err: std::fmt::Display, +{ let mut messages = vec![ Message::new(Role::System, system_prompt), Message::new(Role::User, user_prompt), ]; + info!( + tools = %tool_definitions + .iter() + .map(|tool| tool.function.name.as_str()) + .collect::>() + .join(", "), + max_iterations, + "Starting tool-calling loop" + ); + let mut total_cost = 0.0_f64; let mut has_cost = false; + let mut last_rejection: Option = None; for iteration in 1..=max_iterations { + // Le dernier tour n'est plus un tour d'exploration : le modèle doit rendre + // sa réponse, avec ce qu'il a déjà vu. + let last = iteration == max_iterations; + + if last { + info!(iteration, "Last turn: asking for the final answer"); + messages.push(Message::new(Role::User, FINAL_PROMPT)); + } + + let started = Instant::now(); let response = open_router - .chat_with_tools(messages.clone(), tool_definitions.clone()) + .chat_with_tools( + &messages, + &tool_definitions, + if last { + ToolChoice::None + } else { + ToolChoice::Auto + }, + ) .await?; if let Some(cost) = response.cost { @@ -52,12 +131,44 @@ pub async fn run( has_cost = true; } - if response.tool_calls.is_empty() { - return Ok(AgentResult { - message: response.message.unwrap_or_default(), - cost: has_cost.then_some(total_cost), - iterations: iteration, - }); + info!( + iteration, + max_iterations, + last_turn = last, + tool_calls = response.tool_calls.len(), + answer_bytes = response.message.as_deref().map_or(0, str::len), + elapsed_ms = started.elapsed().as_millis() as u64, + "Model answered" + ); + + // Au dernier tour, les outils sont refusés : même si le fournisseur les + // renvoie malgré tout, on ne les exécute pas et on tente la réponse. + if last || response.tool_calls.is_empty() { + let answer = response.message.unwrap_or_default(); + + match answer.parse::() { + Ok(parsed) => { + return Ok(AgentResult { + answer: parsed, + cost: has_cost.then_some(total_cost), + iterations: iteration, + }); + } + Err(err) => { + warn!(iteration, %err, "Model answer was rejected, asking for another one"); + + // Keep the rejected answer in the history, so that the model + // sees what it has to fix. + messages.push(Message::new(Role::Assistant, answer)); + messages.push(Message::new( + Role::User, + RETRY_PROMPT.replace("{error}", &err.to_string()), + )); + + last_rejection = Some(err.to_string()); + continue; + } + } } messages.push(Message::assistant_with_tool_calls( @@ -66,28 +177,71 @@ pub async fn run( )); for call in &response.tool_calls { - debug!(tool = call.name(), "Executing tool call"); - let content = execute(sandbox, call).await; + let started = Instant::now(); + let (content, truncated) = execute(sandbox, call).await; + + info!( + iteration, + tool = call.name(), + arguments = %excerpt(call.arguments_json(), MAX_LOGGED_ARGUMENTS), + output_bytes = content.len(), + truncated, + // Les sauts de ligne casseraient la lisibilité d'une ligne de log. + output = %excerpt(&content, MAX_LOGGED_OUTPUT).replace('\n', " "), + elapsed_ms = started.elapsed().as_millis() as u64, + "Tool call finished" + ); + messages.push(Message::tool_response(call.id(), content)); } } - warn!(max_iterations, "Agent reached the iteration limit"); - anyhow::bail!("agent exceeded the maximum number of iterations ({max_iterations})") + warn!( + max_iterations, + "Agent used its last turn without a valid answer" + ); + + match last_rejection { + Some(error) => anyhow::bail!( + "agent exceeded the maximum number of iterations ({max_iterations}), \ + last answer rejected: {error}" + ), + None => anyhow::bail!("agent exceeded the maximum number of iterations ({max_iterations})"), + } } -async fn execute(sandbox: &Sandbox, call: &ToolCall) -> String { +/// Runs a tool call and returns its result for the model, along with whether the +/// result had to be truncated. +async fn execute(sandbox: &Sandbox, call: &ToolCall) -> (String, bool) { let args = match parse_args(call) { Ok(args) => args, - Err(err) => return format!("error: {err}"), + Err(err) => return cap_output(format!("error: {err}")), }; match tools::dispatch(sandbox, call.name(), &args).await { - Ok(output) => output, - Err(err) => format!("error: {err}"), + Ok(output) => cap_output(output), + Err(err) => cap_output(format!("error: {err}")), } } +/// Bounds [`MAX_TOOL_OUTPUT_BYTES`] of a tool result, telling the model what was +/// dropped so that it can narrow its request. +fn cap_output(mut output: String) -> (String, bool) { + let total = output.len(); + + if !truncate_bytes(&mut output, MAX_TOOL_OUTPUT_BYTES) { + return (output, false); + } + + let kept = output.len(); + output.push_str(&format!( + "\n… output truncated: {total} bytes in total, the first {kept} are shown. \ + Narrow the request (path, pattern or line range) to see the rest." + )); + + (output, true) +} + fn parse_args(call: &ToolCall) -> anyhow::Result { let raw = call.arguments_json().trim(); if raw.is_empty() { @@ -134,4 +288,29 @@ mod tests { let call = tool_call("ls", "not json"); assert!(parse_args(&call).is_err()); } + + #[test] + fn a_small_tool_output_is_kept_as_is() { + let (content, truncated) = cap_output(String::from("src/main.rs")); + + assert_eq!(content, "src/main.rs"); + assert!(!truncated); + } + + #[test] + fn a_large_tool_output_is_truncated_and_told_to_the_model() { + let (content, truncated) = cap_output("a".repeat(MAX_TOOL_OUTPUT_BYTES + 1)); + + assert!(truncated); + assert!(content.starts_with(&"a".repeat(MAX_TOOL_OUTPUT_BYTES))); + assert!(content.contains("output truncated")); + } + + #[test] + fn a_truncated_tool_output_stays_valid_utf8() { + let (content, truncated) = cap_output("é".repeat(MAX_TOOL_OUTPUT_BYTES)); + + assert!(truncated); + assert!(content.starts_with('é')); + } } diff --git a/crates/herald-server/src/sandbox/mod.rs b/crates/herald-server/src/sandbox/mod.rs index 362376a..d732d27 100644 --- a/crates/herald-server/src/sandbox/mod.rs +++ b/crates/herald-server/src/sandbox/mod.rs @@ -25,7 +25,7 @@ const DEVCONTAINER_PATHS: [&str; 2] = [".devcontainer/devcontainer.json", ".devc /// Sandbox-related runtime configuration. #[derive(Clone)] pub struct SandboxConfig { - /// Container runtime binary to drive (e.g. `docker`, `podman`). + /// Client du daemon de containers qui exécute la sandbox. pub runtime: ContainerRuntime, /// Maximum number of tool-calling iterations per agent run. pub max_iterations: usize, @@ -35,6 +35,8 @@ pub struct SandboxConfig { pub struct Sandbox { // Owns the temporary directory; dropping it cleans up the clone. _workspace: TempDir, + /// Path of the clone, as bind-mounted into the container. + repo_dir: PathBuf, container: Container, } @@ -54,6 +56,7 @@ impl Sandbox { let repo_dir = workspace.path().join("repo"); clone_pull_request(repo_url, token, pull_request_number, &repo_dir).await?; + make_readable(&repo_dir).await?; let devcontainer_path = find_devcontainer(&repo_dir) .with_context(|| format!("no devcontainer found in `{repo_url}`"))?; @@ -63,10 +66,79 @@ impl Sandbox { info!(image = %devcontainer.image_tag(), "Building and starting sandbox container"); let container = devcontainer.up(runtime, &repo_dir).await?; - Ok(Self { + let sandbox = Self { _workspace: workspace, + repo_dir, container, - }) + }; + + sandbox.check_workspace().await?; + + Ok(sandbox) + } + + /// Vérifie que le clone est bien visible dans le container. + /// + /// Sans ce contrôle, un montage vide — le daemon ne voit pas le clone, par + /// exemple quand Herald est dans un container sur une autre machine — fait + /// échouer chaque outil ; le modèle enchaîne alors les appels ratés jusqu'au + /// budget d'itérations, sans jamais pouvoir reviewer quoi que ce soit. + async fn check_workspace(&self) -> anyhow::Result<()> { + let workspace_folder = self.workspace_folder(); + let probe = self + .container + .exec(&["ls", "-A", "--", workspace_folder]) + .await + .with_context(|| format!("failed to list `{workspace_folder}` in the sandbox"))?; + + if !probe.success() { + let details = self.diagnose_workspace().await; + + anyhow::bail!( + "the sandbox cannot list `{workspace_folder}`: {} ({details})", + probe.stderr.trim() + ); + } + + if probe.stdout.trim().is_empty() { + anyhow::bail!( + "the sandbox workspace `{workspace_folder}` is empty: the container daemon does not see the clone" + ); + } + + info!( + clone = %self.repo_dir.display(), + workspace = %workspace_folder, + entries = probe.stdout.lines().count(), + "Sandbox workspace is readable" + ); + + Ok(()) + } + + /// Rassemble de quoi expliquer un refus d'accès au workspace. + /// + /// Un `EACCES` a deux causes possibles, indistinguables dans le message de + /// `ls` : les permissions du clone, ou un confinement du noyau qui bloque + /// l'accès. L'identité de l'utilisateur d'exec et les permissions du point de + /// montage permettent de trancher. + async fn diagnose_workspace(&self) -> String { + let mut details = Vec::new(); + + if let Ok(output) = self.container.exec(&["id"]).await { + details.push(output.stdout.trim().to_string()); + } + + if let Ok(output) = self + .container + .exec(&["ls", "-ld", "--", self.workspace_folder()]) + .await + && output.success() + { + details.push(output.stdout.trim().to_string()); + } + + details.join(", ") } /// Executes a command in the container as an argv vector (no shell). @@ -141,6 +213,32 @@ async fn clone_pull_request( Ok(()) } +/// Rend le clone lisible par les utilisateurs du container sandbox. +/// +/// Le container peut ne pas avoir les mêmes uid que Herald (podman rootless +/// mappe les uid à travers des plages subuid), et l'umask de l'opérateur peut être +/// restrictif : sans cela, les outils de la sandbox échouent en `Permission +/// denied` sur des fichiers que Herald vient de cloner lui-même. +async fn make_readable(repo_dir: &Path) -> anyhow::Result<()> { + let output = tokio::process::Command::new("chmod") + .args(["-R", "a+rX"]) + .arg(repo_dir) + .stdin(Stdio::null()) + .output() + .await + .context("failed to spawn chmod")?; + + if !output.status.success() { + anyhow::bail!( + "chmod failed on `{}`: {}", + repo_dir.display(), + String::from_utf8_lossy(&output.stderr).trim() + ); + } + + Ok(()) +} + /// Runs git with the token injected through `http.extraHeader`, keeping the /// secret out of the process arguments. async fn run_git(token: &str, args: &[String]) -> anyhow::Result<()> { diff --git a/crates/herald-server/src/text.rs b/crates/herald-server/src/text.rs new file mode 100644 index 0000000..e0471d8 --- /dev/null +++ b/crates/herald-server/src/text.rs @@ -0,0 +1,81 @@ +//! Small text helpers shared by the modules that log what the model answered and +//! bound what the tools return to it. + +/// First `limit` characters of `text`, for logs. +/// +/// Cuts on character boundaries, so the excerpt stays valid UTF-8, and marks a +/// truncation with an ellipsis. +pub fn excerpt(text: &str, limit: usize) -> String { + let mut chars = text.chars(); + let excerpt = chars.by_ref().take(limit).collect::(); + + if chars.next().is_some() { + return format!("{excerpt}…"); + } + + excerpt +} + +/// Truncates `text` in place to at most `limit` bytes, cutting on a character +/// boundary so the result stays valid UTF-8. +/// +/// Returns `true` when something was dropped. +pub fn truncate_bytes(text: &mut String, limit: usize) -> bool { + if text.len() <= limit { + return false; + } + + let mut end = limit; + while !text.is_char_boundary(end) { + end -= 1; + } + + text.truncate(end); + + true +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn a_short_text_is_kept_as_is() { + assert_eq!(excerpt("abc", 3), "abc"); + } + + #[test] + fn a_long_text_is_truncated_and_marked() { + assert_eq!(excerpt("abcdef", 3), "abc…"); + } + + #[test] + fn truncation_cuts_on_character_boundaries() { + assert_eq!(excerpt("ééé", 2), "éé…"); + } + + #[test] + fn a_short_text_is_not_truncated() { + let mut text = String::from("abc"); + + assert!(!truncate_bytes(&mut text, 3)); + assert_eq!(text, "abc"); + } + + #[test] + fn a_long_text_is_truncated_within_the_limit() { + let mut text = String::from("abcdef"); + + assert!(truncate_bytes(&mut text, 4)); + assert_eq!(text, "abcd"); + } + + #[test] + fn byte_truncation_never_splits_a_character() { + let mut text = String::from("ééé"); + + // The limit falls in the middle of the second `é`: it is dropped. + assert!(truncate_bytes(&mut text, 3)); + assert_eq!(text, "é"); + } +}