diff --git a/.devcontainer/Dockerfile b/.devcontainer/Dockerfile index 585d054..1b950a1 100644 --- a/.devcontainer/Dockerfile +++ b/.devcontainer/Dockerfile @@ -11,7 +11,6 @@ RUN apt-get update && apt-get install -y --no-install-recommends \ curl \ git \ build-essential \ - libssl-dev \ cmake \ pkg-config \ clang \ diff --git a/.woodpecker/tests.yml b/.woodpecker/tests.yml index e1eda5f..f05e763 100644 --- a/.woodpecker/tests.yml +++ b/.woodpecker/tests.yml @@ -23,6 +23,12 @@ steps: - rustup component add clippy - cargo clippy --workspace --all-targets --all-features -- -D warnings + - name: deny + image: rust:1.99 + commands: + - cargo install cargo-deny --locked --version 0.18.9 + - cargo deny check + - name: wait-for-postgres image: postgres:18 commands: diff --git a/Cargo.lock b/Cargo.lock index 49775d3..1a2406d 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1392,21 +1392,6 @@ version = "0.2.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "77ce24cb58228fbb8aa041425bb1050850ac19177686ea6e0f41a70416f56fdb" -[[package]] -name = "foreign-types" -version = "0.3.2" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "f6f339eb8adc052cd2ca78910fda869aefa38d22d5cb648e6485e4d3fc06f3b1" -dependencies = [ - "foreign-types-shared", -] - -[[package]] -name = "foreign-types-shared" -version = "0.1.1" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "00b0228411908ca8685dba7fc2cdd70ec9990a6e753e89b6ac91a84c40fbaf4b" - [[package]] name = "form_urlencoded" version = "1.2.2" @@ -1811,18 +1796,17 @@ dependencies = [ ] [[package]] -name = "hyper-tls" -version = "0.6.0" +name = "hyper-rustls" +version = "0.27.10" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "70206fc6890eaca9fde8a0bf71caa2ddfc9fe045ac9e5c70df101a7dbde866e0" +checksum = "dfa8e654703247911e29c23fbeaa261834bd9bb74efba2f9acddc37bfb127f53" dependencies = [ - "bytes", - "http-body-util", + "http 1.5.0", "hyper", "hyper-util", - "native-tls", + "rustls", "tokio", - "tokio-native-tls", + "tokio-rustls", "tower-service", ] @@ -2579,23 +2563,6 @@ dependencies = [ "windows-sys 0.61.2", ] -[[package]] -name = "native-tls" -version = "0.2.18" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "465500e14ea162429d264d44189adc38b199b62b1c21eea9f69e4b73cb03bbf2" -dependencies = [ - "libc", - "log", - "openssl", - "openssl-probe", - "openssl-sys", - "schannel", - "security-framework", - "security-framework-sys", - "tempfile", -] - [[package]] name = "nix" version = "0.31.3" @@ -2856,49 +2823,12 @@ version = "0.3.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "c08d65885ee38876c4f86fa503fb49d7b507c2b62552df7c70b2fce627e06381" -[[package]] -name = "openssl" -version = "0.10.81" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "77823a27f0babb03091cb9ed9ef80af3b39dbc82f97e8fa530374b7dafd87a45" -dependencies = [ - "bitflags 2.13.1", - "cfg-if", - "foreign-types", - "libc", - "openssl-macros", - "openssl-sys", -] - -[[package]] -name = "openssl-macros" -version = "0.1.1" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "a948666b637a0f465e8564c73e89d4dde00d72d4d473cc972f390fc3dcee7d9c" -dependencies = [ - "proc-macro2", - "quote", - "syn 2.0.119", -] - [[package]] name = "openssl-probe" version = "0.2.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "7c87def4c32ab89d880effc9e097653c8da5d6ef28e6b539d313baaacfbafcbe" -[[package]] -name = "openssl-sys" -version = "0.9.117" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "b47e7e6bb2c38cd930d25a23b40fa52e068c10e85f3e03a7f5ba5aaca5713695" -dependencies = [ - "cc", - "libc", - "pkg-config", - "vcpkg", -] - [[package]] name = "os_info" version = "3.15.0" @@ -3444,19 +3374,20 @@ dependencies = [ "http-body", "http-body-util", "hyper", - "hyper-tls", + "hyper-rustls", "hyper-util", "js-sys", "log", - "native-tls", "percent-encoding", "pin-project-lite", + "rustls", "rustls-pki-types", + "rustls-platform-verifier", "serde", "serde_json", "sync_wrapper", "tokio", - "tokio-native-tls", + "tokio-rustls", "tokio-util", "tower", "tower-http 0.6.11", @@ -3604,6 +3535,7 @@ version = "0.23.45" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "0d41d731c7d2f962d1ccc364cec258de3c0e93b38c2fb3ba97ac74513048d634" dependencies = [ + "log", "once_cell", "ring", "rustls-pki-types", @@ -3759,8 +3691,8 @@ checksum = "76a88b65feb368d9dde5d531a2b59b25016e36fd6e4978432371a3ee77746ca2" dependencies = [ "cfg_aliases", "httpdate", - "native-tls", "reqwest", + "rustls", "sentry-actix", "sentry-backtrace", "sentry-contexts", @@ -4281,6 +4213,7 @@ dependencies = [ "metrics-exporter-prometheus", "reqwest", "ring", + "rustls", "sentry", "sentry-anyhow", "serde", @@ -4721,16 +4654,6 @@ dependencies = [ "syn 3.0.4", ] -[[package]] -name = "tokio-native-tls" -version = "0.3.1" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "bbae76ab933c85776efabc971569dd6119c580d8f5d448769dec1764bf796ef2" -dependencies = [ - "native-tls", - "tokio", -] - [[package]] name = "tokio-postgres" version = "0.7.18" @@ -5318,14 +5241,13 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "972d7902c8735f2695410b8aed7df6ed12a47394aa1c8d7af49f0497b731a94d" dependencies = [ "base64 0.23.1", - "der 0.8.2", "log", - "native-tls", "percent-encoding", + "rustls", "rustls-pki-types", "ureq-proto", "utf8-zero", - "webpki-root-certs", + "webpki-roots", ] [[package]] diff --git a/Cargo.toml b/Cargo.toml index 26e20b9..e5e4798 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -6,14 +6,31 @@ members = [ resolver = "3" [workspace.dependencies] -reqwest = { version = "0.13", default-features = false, features = ["json", "native-tls", "stream"] } +# TLS is pure Rust (rustls + ring): no libssl is needed at build or run time. +# `rustls-no-provider` leaves the crypto provider to the binary, which installs +# `ring` at startup (see `main`), so that a single provider is ever compiled in. +reqwest = { version = "0.13", default-features = false, features = ["json", "rustls-no-provider", "stream"] } +rustls = { version = "0.23", default-features = false, features = ["ring", "std", "tls12"] } tokio = { version = "1.53", features = ["full"] } tokio-stream = "0.1" tokio-util = "0.7" futures-util = "0.3" serde_json = "1.0" serde = { version = "1.0", features = ["derive"] } -sentry = { version = "0.49", features = ["tower-axum-matched-path"] } +# Sentry's default `transport` feature pulls `native-tls`; the same set of features +# is spelled out here with rustls instead. +sentry = { version = "0.49", default-features = false, features = [ + "backtrace", + "contexts", + "debug-images", + "logs", + "metrics", + "panic", + "release-health", + "reqwest", + "rustls-no-provider", + "tower-axum-matched-path", +] } sentry-anyhow = { version = "0.49", features = ["backtrace"] } dotenvy = "0.15" tower = "0.5" @@ -31,6 +48,9 @@ metrics-exporter-prometheus = { version = "0.18", default-features = false, feat toasty = { version = "0.11", features = ["postgresql", "serde", "jiff"] } toasty-cli = "0.11" jiff = { version = "0.2", features = ["serde"] } +tempfile = "3" +bollard = "0.21" +tar = "0.4" [profile.release] debug = 1 diff --git a/Containerfile b/Containerfile index ea5eacf..bda6797 100644 --- a/Containerfile +++ b/Containerfile @@ -14,9 +14,6 @@ FROM debian:trixie-slim RUN apt-get update && apt-get install -y --no-install-recommends \ ca-certificates \ git \ - libssl3t64 \ - libzstd1 \ - zlib1g \ && rm -rf /var/lib/apt/lists/* diff --git a/README.md b/README.md index 45f19b5..44cd51a 100644 --- a/README.md +++ b/README.md @@ -42,7 +42,7 @@ Steep reads its configuration from environment variables (a `.env` file is suppo | `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` | -| `SANDBOX_MAX_ITERATIONS` | *(optional)* Maximum number of tool-calling iterations per sandboxed review. Defaults to `8` | +| `SANDBOX_MAX_ITERATIONS` | *(optional)* Maximum number of tool-calling iterations per sandboxed review. Defaults to `8`; a value that is set but is not a number stops the startup | | `DOCKER_HOST` | *(optional)* Container daemon socket Steep drives, e.g. `unix:///run/user/1000/podman/podman.sock` for a rootless podman. Defaults to `unix:///var/run/docker.sock` | ## Sandboxed reviews @@ -139,6 +139,24 @@ cargo run -p steep-server Copy `.env.example` to `.env` and fill in your values before running. +## Job queue + +Steep is meant to run as a **single instance**. Webhooks are stored in the `jobs` table +of PostgreSQL, and the worker is woken up as soon as a job is pushed (no polling). +A job still `processing` at startup was interrupted by the previous process: it is put +back in the queue and reviewed again. Do not run two instances on the same database, +they would both take over each other's running jobs. + +## Dependencies + +TLS is handled by [rustls](https://github.com/rustls/rustls) (with `ring`), so neither +the build nor the image needs OpenSSL. The `Containerfile` runtime image only +installs `ca-certificates` and `git`. + +`cargo deny check` audits the dependencies (security advisories, licenses, banned +crates, sources) with the policy of `deny.toml`. It runs in the CI, and it also fails +if `openssl-sys` or `native-tls` comes back into the dependency tree. + ## Metrics Steep optionally exposes a Prometheus metrics endpoint, useful for scraping with an [OpenTelemetry Collector](https://opentelemetry.io/docs/collector/) or any Prometheus-compatible scraper. diff --git a/crates/devcontainer-rs/Cargo.toml b/crates/devcontainer-rs/Cargo.toml index 883b958..6b3f3bf 100644 --- a/crates/devcontainer-rs/Cargo.toml +++ b/crates/devcontainer-rs/Cargo.toml @@ -2,12 +2,13 @@ name = "devcontainer-rs" version = "0.1.0" edition = "2024" +publish = false [dependencies] -bollard = "0.21" +bollard = { workspace = true } bytes = { workspace = true } futures-util = { workspace = true } -tar = "0.4" +tar = { workspace = true } tokio = { workspace = true } tokio-stream = { workspace = true } tracing = { workspace = true } @@ -17,4 +18,4 @@ anyhow = { workspace = true } thiserror = { workspace = true } [dev-dependencies] -tempfile = "3" +tempfile = { workspace = true } diff --git a/crates/devcontainer-rs/src/consts.rs b/crates/devcontainer-rs/src/consts.rs index 9aceac6..4e95d70 100644 --- a/crates/devcontainer-rs/src/consts.rs +++ b/crates/devcontainer-rs/src/consts.rs @@ -1,25 +1,24 @@ -//! Constantes de la crate : endpoint du daemon, timeouts et découpage du -//! contexte de build. +//! Crate constants: daemon endpoint, timeouts and build context chunking. use std::time::Duration; -/// Endpoint affiché dans les logs quand `DOCKER_HOST` n'est pas défini. +/// Endpoint shown in the logs when `DOCKER_HOST` is not set. pub(crate) const DEFAULT_ENDPOINT: &str = "unix:///var/run/docker.sock"; -/// Timeout appliqué aux opérations de build/run/stop/remove. +/// Timeout applied to build/run/stop/remove operations. pub(crate) const DEFAULT_COMMAND_TIMEOUT: Duration = Duration::from_secs(600); -/// Timeout appliqué aux commandes exécutées dans un container en cours d'exécution. +/// Timeout applied to commands run inside a running container. pub(crate) const DEFAULT_EXEC_TIMEOUT: Duration = Duration::from_secs(60); -/// Taille des morceaux du contexte de build envoyés au daemon. +/// Size of the build context chunks sent to the daemon. pub(crate) 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. +/// Number of chunks that can wait in the channel: this is what bounds the +/// memory held by the context, regardless of its size. pub(crate) const CONTEXT_CHUNKS: usize = 4; -/// Lectures successives du code de sortie d'un exec, et attente entre elles. +/// Successive reads of an exec's exit code, and the delay between them. pub(crate) const EXIT_CODE_ATTEMPTS: usize = 10; pub(crate) const EXIT_CODE_DELAY: Duration = Duration::from_millis(20); diff --git a/crates/devcontainer-rs/src/container.rs b/crates/devcontainer-rs/src/container.rs index 7236af8..cf56fdd 100644 --- a/crates/devcontainer-rs/src/container.rs +++ b/crates/devcontainer-rs/src/container.rs @@ -1,4 +1,4 @@ -//! Un devcontainer en cours d'exécution : inspection et exécution de commandes. +//! A running devcontainer: inspection and command execution. use std::time::Duration; @@ -7,7 +7,7 @@ use crate::{ runtime::ContainerRuntime, }; -/// Un devcontainer en cours d'exécution. +/// A running devcontainer. #[derive(Debug, Clone)] pub struct Container { runtime: ContainerRuntime, @@ -19,8 +19,8 @@ pub struct Container { } impl Container { - /// Assemble un container démarré, avec son réseau et, le cas échéant, l'image - /// construite pour lui et à nettoyer. + /// Assembles a started container, with its network and, if any, the image + /// built for it that must be cleaned up. pub(crate) fn new( runtime: ContainerRuntime, name: String, @@ -47,10 +47,10 @@ impl Container { &self.workspace_folder } - /// Exécute une commande dans le container et renvoie sa sortie. + /// Runs a command in the container and returns its output. /// - /// La commande est transmise sous forme de vecteur d'arguments (argv, sans shell), - /// donc aucun échappement ni interpolation n'est effectué. + /// The command is passed as an argument vector (argv, no shell), so no + /// escaping or interpolation is performed. pub async fn exec(&self, cmd: &[&str]) -> Result { self.exec_with_timeout(cmd, DEFAULT_EXEC_TIMEOUT).await } @@ -65,19 +65,19 @@ impl Container { .await } - /// Exécute un script shell dans le container via `sh -c`. + /// Runs a shell script in the container via `sh -c`. pub async fn exec_shell(&self, script: &str) -> Result { self.exec(&["sh", "-c", script]).await } - /// Arrête le container. + /// Stops the container. pub async fn stop(&self) -> Result<(), ContainerError> { self.runtime.stop_container(&self.name).await } - /// Supprime le container, son réseau et son image. + /// Removes the container, its network and its image. /// - /// La suppression du réseau et de l'image est best-effort : ils peuvent déjà être absents. + /// Removing the network and the image is best-effort: they may already be gone. pub async fn remove(&self) -> Result<(), ContainerError> { self.runtime.remove_container(&self.name).await?; diff --git a/crates/devcontainer-rs/src/context.rs b/crates/devcontainer-rs/src/context.rs index 8c80d46..f964d7c 100644 --- a/crates/devcontainer-rs/src/context.rs +++ b/crates/devcontainer-rs/src/context.rs @@ -1,4 +1,4 @@ -//! Empaquetage en flux du contexte de build envoyé au daemon. +//! Streaming packaging of the build context sent to the daemon. use std::{ io::{BufWriter, Write}, @@ -12,13 +12,13 @@ use tokio_stream::wrappers::ReceiverStream; use crate::consts::{CONTEXT_CHUNK_SIZE, CONTEXT_CHUNKS}; -/// Empaquette le contexte de build dans un tar, **en flux**. +/// Packs the build context into a tar, **as a stream**. /// -/// 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. +/// The `docker build` CLI packs the context and then sends it; doing that in one go +/// would load the whole folder into memory, which becomes untenable as soon as the +/// `.devcontainer` ships binaries. Here the archive is written by a blocking task +/// and pushed chunk by chunk, with the channel's backpressure: if the daemon reads +/// slowly, packing slows down. pub(crate) fn tar_directory_stream( dir: &Path, ) -> impl Stream> + Send + 'static { @@ -33,13 +33,13 @@ pub(crate) fn tar_directory_stream( 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. + // `finish` writes the end-of-archive blocks; the buffer still has to be + // flushed so the daemon receives everything, even an empty archive. .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. + // The failure is reported to the daemon through the stream: otherwise it + // would only see a truncated tar and report a confusing error. let _ = failures.blocking_send(Err(error)); } }); @@ -47,9 +47,9 @@ pub(crate) fn tar_directory_stream( ReceiverStream::new(receiver) } -/// Écrit les morceaux du tar dans un canal, en attendant qu'il se vide. +/// Writes the tar chunks into a channel, waiting for it to drain. /// -/// Utilisé depuis une tâche bloquante, seul contexte où `blocking_send` est autorisé. +/// Used from a blocking task, the only context where `blocking_send` is allowed. struct ChannelWriter { sender: mpsc::Sender>, } @@ -80,7 +80,7 @@ mod tests { use futures_util::StreamExt; - /// Rassemble le flux du contexte en un tar complet, pour l'inspecter. + /// Collects the context stream into a complete tar, for inspection. async fn context_tar(dir: &Path) -> Vec { let mut tar = Vec::new(); let mut chunks = tar_directory_stream(dir); diff --git a/crates/devcontainer-rs/src/devcontainer.rs b/crates/devcontainer-rs/src/devcontainer.rs index 4d59d61..a543720 100644 --- a/crates/devcontainer-rs/src/devcontainer.rs +++ b/crates/devcontainer-rs/src/devcontainer.rs @@ -1,5 +1,5 @@ -//! Représentation analysée d'un `devcontainer.json` et son cycle de vie : -//! nommage, build, démarrage et hooks. +//! Parsed representation of a `devcontainer.json` and its lifecycle: naming, +//! build, startup and hooks. use std::{ collections::HashMap, @@ -29,29 +29,29 @@ pub struct DevContainer { } impl DevContainer { - /// Dossier de workspace dans le container, par défaut `/workspace`. + /// Workspace folder inside the container, `/workspace` by default. pub fn workspace_folder(&self) -> String { self.workspace_folder .clone() .unwrap_or_else(|| "/workspace".to_string()) } - /// Tag de l'image construite localement à partir du Dockerfile. + /// Tag of the image built locally from the Dockerfile. fn local_image_tag(&self, id: &str) -> String { format!("devcontainer-rs/{}:{}", self.sanitized_name(), id) } - /// Nom du container de cette sandbox. + /// Name of this sandbox's container. fn container_name(&self, id: &str) -> String { format!("devcontainer-rs-{}-{}", self.sanitized_name(), id) } - /// Nom du devcontainer nettoyé pour servir de nom d'image/container. + /// Devcontainer name, sanitized to be usable as an image/container name. fn sanitized_name(&self) -> String { sanitize(self.name.as_deref().unwrap_or("devcontainer")) } - /// Construit l'image devcontainer sous `image_tag`. + /// Builds the devcontainer image as `image_tag`. async fn build( &self, runtime: &ContainerRuntime, @@ -239,7 +239,7 @@ impl Default for DevContainer { } } -/// Nettoie une chaîne pour qu'elle puisse servir de nom d'image/container docker. +/// Sanitizes a string so it can be used as a docker image/container name. fn sanitize(input: &str) -> String { let sanitized: String = input .chars() @@ -260,7 +260,7 @@ fn sanitize(input: &str) -> String { } } -/// Identifiant unique à une exécution de sandbox, combinant l'identifiant de processus et un horodatage. +/// Identifier unique to a sandbox run, combining the process id and a timestamp. fn unique_id() -> String { let nanos = SystemTime::now() .duration_since(UNIX_EPOCH) diff --git a/crates/devcontainer-rs/src/errors.rs b/crates/devcontainer-rs/src/errors.rs index b6a451d..5968bb8 100644 --- a/crates/devcontainer-rs/src/errors.rs +++ b/crates/devcontainer-rs/src/errors.rs @@ -1,5 +1,4 @@ -//! Erreurs de la crate : échecs du daemon de containers et échecs d'analyse du -//! `devcontainer.json`. +//! Crate errors: container daemon failures and `devcontainer.json` parsing failures. use std::{path::PathBuf, time::Duration}; @@ -7,25 +6,25 @@ use bollard::errors::Error as BollardError; #[derive(Debug, thiserror::Error)] pub enum ContainerError { - /// Le daemon est injoignable, ou a refusé une requête. + /// The daemon is unreachable, or rejected a request. #[error("container daemon request failed: {source}")] Request { #[from] source: BollardError, }, - /// Une opération a dépassé son timeout. + /// An operation exceeded its timeout. #[error("`{operation}` timed out after {timeout:?}")] Timeout { operation: String, timeout: Duration, }, - /// La construction de l'image a échoué. + /// The image build failed. #[error("image build failed: {message}")] Build { message: String }, - /// Le daemon a répondu autre chose que ce qui était attendu. + /// The daemon replied with something other than what was expected. #[error("{0}")] Unexpected(String), } diff --git a/crates/devcontainer-rs/src/exec.rs b/crates/devcontainer-rs/src/exec.rs index 58d2615..d1fc254 100644 --- a/crates/devcontainer-rs/src/exec.rs +++ b/crates/devcontainer-rs/src/exec.rs @@ -1,9 +1,9 @@ -//! Résultat d'une commande exécutée dans un container. +//! Result of a command run in a container. -/// Résultat d'une commande exécutée dans un container. +/// Result of a command run in a container. #[derive(Debug, Clone)] pub struct ExecOutput { - /// Code de sortie, ou `-1` si le processus a été terminé par un signal. + /// Exit code, or `-1` if the process was terminated by a signal. pub status: i32, pub stdout: String, pub stderr: String, diff --git a/crates/devcontainer-rs/src/lib.rs b/crates/devcontainer-rs/src/lib.rs index b0ccb44..fe2050f 100644 --- a/crates/devcontainer-rs/src/lib.rs +++ b/crates/devcontainer-rs/src/lib.rs @@ -1,28 +1,27 @@ -//! Primitives de cycle de vie de container pour un [`DevContainer`] analysé. +//! Container lifecycle primitives for a parsed [`DevContainer`]. //! -//! Cette crate pilote l'API du daemon de containers pour préparer l'image -//! devcontainer (construite depuis un Dockerfile, ou tirée d'un registre), -//! démarrer un container et y copier le workspace, exécuter les -//! hooks `postCreateCommand` / `postStartCommand` et lancer des commandes à -//! l'intérieur du container en cours d'exécution. +//! This crate drives the container daemon API to prepare the devcontainer image +//! (built from a Dockerfile, or pulled from a registry), start a container and +//! copy the workspace into it, run the `postCreateCommand` / `postStartCommand` +//! hooks, and run commands inside the running container. //! -//! Le daemon est joint par son **socket** (celui de l'hôte, monté dans le -//! container de Steep) : [`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. +//! The daemon is reached through its **socket** (the host's, mounted into the +//! Steep container): [`ContainerRuntime::connect`] follows `DOCKER_HOST` like the +//! `docker` CLI does, and falls back to the local socket. No runtime binary is +//! therefore required in the image, and `podman` works the same way as long as it +//! exposes its Docker-compatible socket. //! //! # Isolation //! -//! Chaque sandbox dispose de son propre tag d'image, de son propre container et de -//! son propre réseau. Le container démarre attaché à ce réseau afin que les hooks -//! `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. +//! Each sandbox has its own image tag, its own container and its own network. The +//! container starts attached to that network so that the `postCreateCommand` / +//! `postStartCommand` hooks can fetch dependencies (e.g. `npm install`); once the +//! hooks have run, the container is disconnected from the network for the rest of +//! its lifetime. Every command is bounded by a timeout. //! -//! Les `runArgs` du `devcontainer.json` sont ignorés : 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. +//! The `devcontainer.json` `runArgs` are ignored: they come from the repository, +//! hence from an untrusted pull request, and could attach the container to another +//! network or grant it privileges that would defeat this isolation. mod consts; mod container; diff --git a/crates/devcontainer-rs/src/path.rs b/crates/devcontainer-rs/src/path.rs index 4524c0e..3292aae 100644 --- a/crates/devcontainer-rs/src/path.rs +++ b/crates/devcontainer-rs/src/path.rs @@ -1,9 +1,9 @@ -//! Normalisation lexicale de chemins, sans accès au système de fichiers. +//! Lexical path normalization, without accessing the filesystem. use std::path::{Path, PathBuf}; -/// Normalise lexicalement un chemin, en résolvant `.` et `..` sans toucher au -/// système de fichiers. Renvoie `None` si le chemin sort de sa racine. +/// Lexically normalizes a path, resolving `.` and `..` without touching the +/// filesystem. Returns `None` if the path escapes its root. pub fn normalize(path: &Path) -> Option { use std::path::Component; diff --git a/crates/devcontainer-rs/src/runtime.rs b/crates/devcontainer-rs/src/runtime.rs index 63028c8..4fb7f3c 100644 --- a/crates/devcontainer-rs/src/runtime.rs +++ b/crates/devcontainer-rs/src/runtime.rs @@ -1,4 +1,4 @@ -//! Client de l'API du daemon de containers et opérations de cycle de vie. +//! Container daemon API client and lifecycle operations. use std::{collections::HashMap, path::Path, time::Duration}; @@ -24,7 +24,7 @@ use crate::{ exec::ExecOutput, }; -/// Un client de l'API du daemon de containers. +/// A container daemon API client. #[derive(Debug, Clone)] pub struct ContainerRuntime { docker: Docker, @@ -33,7 +33,7 @@ pub struct ContainerRuntime { } impl ContainerRuntime { - /// Se connecte au daemon désigné par `DOCKER_HOST`, ou au socket local par défaut. + /// Connects to the daemon designated by `DOCKER_HOST`, or to the default local socket. pub fn connect() -> Result { let endpoint = std::env::var("DOCKER_HOST").unwrap_or_else(|_| String::from(DEFAULT_ENDPOINT)); @@ -45,13 +45,13 @@ impl ContainerRuntime { }) } - /// Remplace le timeout appliqué aux opérations de build/run/stop/remove. + /// Replaces the timeout applied to build/run/stop/remove operations. pub fn with_timeout(mut self, timeout: Duration) -> Self { self.timeout = timeout; self } - /// Endpoint du daemon, pour les logs. + /// Daemon endpoint, for logging. pub fn endpoint(&self) -> &str { &self.endpoint } @@ -60,12 +60,12 @@ impl ContainerRuntime { self.timeout } - /// Vérifie que le daemon est joignable et répond. + /// Checks that the daemon is reachable and responding. pub async fn available(&self) -> bool { self.docker.ping().await.is_ok() } - /// Exécute une requête du daemon en appliquant le timeout des commandes. + /// Runs a daemon request, applying the command timeout. async fn request( &self, operation: &str, @@ -81,7 +81,7 @@ impl ContainerRuntime { } } - /// Construit l'image `tag` depuis le contexte `context_dir`. + /// Builds the image `tag` from the `context_dir` context. pub(crate) async fn build_image( &self, context_dir: &Path, @@ -102,8 +102,8 @@ impl ContainerRuntime { .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. + // The stream carries the build progress: the last error reported by the + // daemon makes the operation fail. let consume = async { let mut failure = None; @@ -162,7 +162,7 @@ impl ContainerRuntime { } } - /// Crée un container, sans le démarrer. + /// Creates a container without starting it. pub(crate) async fn create_container( &self, name: &str, @@ -217,7 +217,7 @@ impl ContainerRuntime { Ok(()) } - /// Exécute une commande dans un container et renvoie sa sortie. + /// Runs a command in a container and returns its output. pub(crate) async fn exec( &self, container: &str, @@ -254,8 +254,8 @@ impl ContainerRuntime { 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. + // The timeout covers the execution of the command itself, not just its + // setup. let collect = async { while let Some(message) = output.next().await { match message? { @@ -276,10 +276,10 @@ impl ContainerRuntime { Ok(Ok(())) => {} Ok(Err(source)) => return Err(ContainerError::Request { source }), Err(_) => { - // Le processus continue de tourner dans le container : sans arrêt, il - // consommerait CPU et mémoire et pourrait encore modifier le workspace - // pendant toute la durée de vie de la sandbox. L'API n'offre pas de - // moyen de tuer un exec, on arrête donc le container qui le porte. + // The process keeps running in the container: left alone, it would + // consume CPU and memory and could still modify the workspace for the + // whole lifetime of the sandbox. The API offers no way to kill an exec, + // so we stop the container that hosts it. let _ = self.stop_container(container).await; return Err(ContainerError::Timeout { @@ -289,10 +289,10 @@ impl ContainerRuntime { } } - // 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. + // The exit code is not necessarily published by the time the stream closes: + // we give the daemon time to fill it in, without blocking indefinitely. + // Without this, a successful command can be reported as failed (code -1) and + // the tool returns an error to the model instead of the content. let mut inspected = self .request("inspect exec", self.docker.inspect_exec(&created.id)) .await?; @@ -331,7 +331,7 @@ impl ContainerRuntime { Ok(()) } - /// Supprime un container, ses volumes anonymes et le processus qui y tourne. + /// Removes a container, its anonymous volumes and the process running in it. pub(crate) async fn remove_container(&self, name: &str) -> Result<(), ContainerError> { let options = RemoveContainerOptions { force: true, @@ -363,7 +363,7 @@ impl ContainerRuntime { Ok(()) } - /// Crée le réseau dédié d'une sandbox. + /// Creates a sandbox's dedicated network. pub(crate) async fn create_network(&self, name: &str) -> Result<(), ContainerError> { let request = NetworkCreateRequest { name: String::from(name), @@ -376,7 +376,7 @@ impl ContainerRuntime { Ok(()) } - /// Détache un container d'un réseau, ce qui coupe sa connectivité. + /// Detaches a container from a network, which cuts off its connectivity. pub(crate) async fn disconnect_network( &self, network: &str, diff --git a/crates/devcontainer-rs/src/schema.rs b/crates/devcontainer-rs/src/schema.rs index de7d5e1..7d60fb8 100644 --- a/crates/devcontainer-rs/src/schema.rs +++ b/crates/devcontainer-rs/src/schema.rs @@ -1,5 +1,5 @@ -//! Schéma du `devcontainer.json`, analyse du fichier et résolution des -//! références `${localEnv:…}`. +//! `devcontainer.json` schema, file parsing and resolution of `${localEnv:…}` +//! references. use std::{ collections::HashMap, @@ -23,7 +23,7 @@ struct DevContainerSchema { #[serde(default)] name: Option, - /// Image de base tirée du registre, alternative à `build`. + /// Base image pulled from the registry, alternative to `build`. #[serde(default)] image: Option, @@ -106,16 +106,16 @@ impl TryFrom<(DevContainerSchema, PathBuf)> for DevContainer { } } -/// Résout les références `${localEnv:VAR}` et `${localEnv:VAR:default}` d'un +/// Resolves the `${localEnv:VAR}` and `${localEnv:VAR:default}` references of a /// `devcontainer.json`. /// -/// La spécification veut que `${localEnv:VAR}` soit lu dans l'environnement du -/// client ; ici le `devcontainer.json` vient d'une **pull request**, donc de code -/// non fiable : lire l'environnement du processus lui permettrait de récupérer -/// `GITEA_TOKEN`, `OPEN_ROUTER_API_KEY` ou n'importe quel autre secret de Steep et -/// de l'exfiltrer depuis un `build.args`, un `containerEnv`, son Dockerfile ou un -/// hook. L'environnement n'est donc **jamais** consulté : seule la valeur par -/// défaut est utilisée, et une référence sans défaut devient une chaîne vide. +/// The specification says `${localEnv:VAR}` is read from the client's environment; +/// here the `devcontainer.json` comes from a **pull request**, hence from untrusted +/// code: reading the process environment would let it grab `GITEA_TOKEN`, +/// `OPEN_ROUTER_API_KEY` or any other Steep secret and exfiltrate it through a +/// `build.args`, a `containerEnv`, its Dockerfile or a hook. The environment is +/// therefore **never** consulted: only the default value is used, and a reference +/// without a default becomes an empty string. fn substitute_local_env(input: &str) -> String { const PREFIX: &str = "${localEnv:"; @@ -129,7 +129,7 @@ fn substitute_local_env(input: &str) -> String { match after.find('}') { Some(end) => { let inner = &after[..end]; - // La clé est lue pour délimiter la référence, pas pour la résoudre. + // The key is read to delimit the reference, not to resolve it. let (_key, default) = match inner.split_once(':') { Some((key, default)) => (key, Some(default)), None => (inner, None), @@ -239,8 +239,8 @@ mod tests { let dir = tempfile::tempdir().unwrap(); let devcontainer_path = dir.path().join("devcontainer.json"); - // `runArgs` vient d'une pull request non fiable : il doit être accepté par - // l'analyse mais jamais repris dans le devcontainer. + // `runArgs` comes from an untrusted pull request: it must be accepted by + // parsing but never carried over into the devcontainer. fs::write( &devcontainer_path, r#"{ @@ -271,8 +271,8 @@ mod tests { fn local_env_references_are_never_read_from_the_environment() { unsafe { std::env::set_var("DEVCONTAINER_TEST_SECRET", "s3cret") }; - // Le `devcontainer.json` vient d'une pull request : résoudre la référence - // depuis l'environnement de Steep y exposerait ses jetons. + // The `devcontainer.json` comes from a pull request: resolving the reference + // from Steep's environment would expose its tokens to it. assert_eq!( substitute_local_env("${localEnv:DEVCONTAINER_TEST_SECRET}"), "" diff --git a/crates/steep-server/Cargo.toml b/crates/steep-server/Cargo.toml index e7e19a1..9d2b74a 100644 --- a/crates/steep-server/Cargo.toml +++ b/crates/steep-server/Cargo.toml @@ -2,6 +2,7 @@ name = "steep-server" version = "1.3.0" edition = "2024" +publish = false [dependencies] reqwest = { workspace = true } @@ -26,7 +27,8 @@ hex = { workspace = true } metrics = { workspace = true } metrics-exporter-prometheus = { workspace = true } devcontainer-rs = { path = "../devcontainer-rs" } -tempfile = "3" +tempfile = { workspace = true } +rustls = { workspace = true } toasty = { workspace = true } toasty-cli = { workspace = true } jiff = { workspace = true } diff --git a/crates/steep-server/src/api.rs b/crates/steep-server/src/api.rs index 158b5ff..c99ff82 100644 --- a/crates/steep-server/src/api.rs +++ b/crates/steep-server/src/api.rs @@ -28,8 +28,8 @@ pub async fn start(app_state: AppState) -> anyhow::Result<()> { let http_port = app_state.config.http_port; let shutdown = app_state.shutdown.clone(); - // Le dashboard est servi depuis un dossier statique ; les routes API - // ci-dessous restent prioritaires (le dossier n'est que le fallback). + // The dashboard is served from a static directory; the API routes below take + // precedence (the directory is only the fallback). let front_dir = std::env::var("FRONT_DIR").unwrap_or_else(|_| String::from("front")); if !std::path::Path::new(&front_dir).is_dir() { warn!(dir = %front_dir, "front directory not found, the dashboard will 404"); @@ -38,7 +38,7 @@ pub async fn start(app_state: AppState) -> anyhow::Result<()> { let app = Router::new() .route("/webhook", post(webhook)) - .route("/dedup", get(dedup_list)) + .route("/reviews", get(reviews_list)) .route("/jobs", get(jobs_list)) .route("/events", get(lifecycle_events)) .route("/jobs/{id}/events", get(job_events)) @@ -63,8 +63,8 @@ pub async fn start(app_state: AppState) -> anyhow::Result<()> { Ok(()) } -async fn dedup_list(State(app_state): State) -> Result { - let reviews = app_state.dedup.list_reviews().await?; +async fn reviews_list(State(app_state): State) -> Result { + let reviews = app_state.reviews.list().await?; Ok(Json(json!({ "reviews": reviews @@ -81,10 +81,10 @@ async fn jobs_list(State(app_state): State) -> Result, ) -> Sse>> { @@ -143,7 +143,7 @@ async fn webhook( metrics::webhook_received(event_type); let event_id = wb.event_id(); - match app_state.dedup.is_known(event_id).await { + match is_known(&app_state, event_id).await { Ok(true) => { metrics::webhook_duplicate(event_type); return Err(AppError::AlreadyProcessedErr); @@ -172,6 +172,12 @@ async fn webhook( Ok((StatusCode::CREATED, "Task started")) } +/// Whether an event is already queued, or already reviewed. +async fn is_known(app_state: &AppState, event_id: u64) -> toasty::Result { + Ok(app_state.queue.is_queued(event_id).await? + || app_state.reviews.is_reviewed(event_id).await?) +} + pub struct WebhookExtract(pub WebhookType); impl FromRequest for WebhookExtract diff --git a/crates/steep-server/src/bot.rs b/crates/steep-server/src/bot.rs index c7172cd..94deff8 100644 --- a/crates/steep-server/src/bot.rs +++ b/crates/steep-server/src/bot.rs @@ -1,200 +1,95 @@ -use std::sync::Arc; +use std::{sync::Arc, time::Duration}; -use futures_util::StreamExt; -use serde::{Deserialize, Deserializer}; -use tokio::sync::Semaphore; +use tokio::{sync::Semaphore, task::JoinSet}; use tokio_util::sync::CancellationToken; use tracing::{error, info, instrument}; use crate::{ - bot_actions::review::ReviewOutcome, - dedup::{Dedup, ReviewRecord}, + bot_actions::review::{self, ReviewContext, ReviewOutcome}, event_bus::{EventBus, Reporter}, gitea::{GiteaAPI, WebhookType}, job_queue::JobQueue, metrics, open_router::OpenRouterClient, - sandbox::SandboxConfig, + review_store::{ReviewRecord, ReviewStore}, + sandbox::{SandboxConfig, tools}, schemas::Job, }; -#[derive(Deserialize, Debug)] -pub struct ReviewResult { - pub reviews: Vec, - pub comment: String, - pub cost: Option, -} +/// Delay before claiming again after the queue could not be reached. +const CLAIM_RETRY_DELAY: Duration = Duration::from_secs(1); -#[derive(Deserialize, Debug)] -pub struct ReviewItem { - pub filename: String, - pub line: Option, - #[serde(default, deserialize_with = "deserialize_side")] - pub side: Option, - #[serde(default, deserialize_with = "deserialize_severity")] - pub severity: Option, - pub message: String, -} - -/// Which version of the file a review comment is anchored on. -#[derive(Debug, Clone, Copy, PartialEq, Eq)] -pub enum ReviewSide { - /// The line was added by the pull request: `line` is a line number of the - /// new version of the file. - Added, - /// The line was removed by the pull request: `line` is a line number of the - /// old version of the file. - Removed, -} - -impl ReviewSide { - /// Reads the side the model asked for, tolerating casing and synonyms. - fn parse(raw: &str) -> Option { - match raw.trim().to_ascii_lowercase().as_str() { - "added" | "add" | "new" | "right" => Some(Self::Added), - "removed" | "remove" | "deleted" | "delete" | "old" | "left" => Some(Self::Removed), - _ => None, - } - } -} - -/// What kind of problem a review reports. -/// -/// The categories are the ones the prompt asks for; a review whose severity is -/// missing or unreadable falls back to [`ReviewSeverity::Maintainability`], the -/// least alarming bucket, instead of failing the whole review. -#[derive(Debug, Clone, Copy, PartialEq, Eq)] -pub enum ReviewSeverity { - /// Wrong behaviour. - Bug, - /// A vulnerability. - Security, - /// A resource problem. - Performance, - /// Everything else: readability, structure, naming, tests. - Maintainability, -} - -impl ReviewSeverity { - /// Reads the severity the model asked for, tolerating casing and synonyms. - fn parse(raw: &str) -> Option { - match raw.trim().to_ascii_lowercase().as_str() { - "bug" | "bugs" | "correctness" | "error" => Some(Self::Bug), - "security" | "vulnerability" => Some(Self::Security), - "performance" | "perf" => Some(Self::Performance), - "maintainability" | "quality" | "style" => Some(Self::Maintainability), - _ => None, - } - } - - /// Lower-case label used in the review markdown. - pub fn label(self) -> &'static str { - match self { - Self::Bug => "bug", - Self::Security => "security", - Self::Performance => "performance", - Self::Maintainability => "maintainability", - } - } -} - -/// Reads the side the model asked for. An unreadable value is ignored instead of -/// failing the whole review: the side is then resolved from the changed lines. -fn deserialize_side<'de, D>(deserializer: D) -> Result, D::Error> -where - D: Deserializer<'de>, -{ - let raw = Option::::deserialize(deserializer)?; - - Ok(raw - .as_ref() - .and_then(serde_json::Value::as_str) - .and_then(ReviewSide::parse)) -} - -/// Reads the severity the model asked for, ignoring an unreadable value: the -/// review is then reported as [`ReviewSeverity::Maintainability`]. -fn deserialize_severity<'de, D>(deserializer: D) -> Result, D::Error> -where - D: Deserializer<'de>, -{ - let raw = Option::::deserialize(deserializer)?; - - Ok(raw - .as_ref() - .and_then(serde_json::Value::as_str) - .and_then(ReviewSeverity::parse)) +/// Everything the bot works with: the external services and the stores. +#[derive(Clone)] +pub struct BotDeps { + pub gitea: GiteaAPI, + pub open_router: OpenRouterClient, + pub sandbox: SandboxConfig, + pub reviews: ReviewStore, + pub queue: JobQueue, + pub event_bus: EventBus, } #[derive(Clone)] pub struct Bot { - gitea_api: GiteaAPI, - open_router_client: OpenRouterClient, + deps: BotDeps, max_concurrent: usize, - open_router_model: String, - sandbox: SandboxConfig, - dedup: Dedup, - queue: JobQueue, - event_bus: EventBus, } impl Bot { - #[allow(clippy::too_many_arguments)] - pub fn new( - gitea_api: GiteaAPI, - open_router_client: OpenRouterClient, - max_concurrent: usize, - open_router_model: String, - sandbox: SandboxConfig, - dedup: Dedup, - queue: JobQueue, - event_bus: EventBus, - ) -> Self { + pub fn new(deps: BotDeps, max_concurrent: usize) -> Self { Self { - gitea_api, - open_router_client, + deps, max_concurrent, - open_router_model, - sandbox, - dedup, - queue, - event_bus, } } - /// Consomme la file jusqu'à l'annulation de `shutdown`. + /// Consumes the queue until `shutdown` is cancelled. /// - /// Un sémaphore borne la concurrence ; les jobs en cours sont laissés se - /// terminer. Les jobs non terminés restent en base et seront repris au - /// prochain démarrage. + /// A semaphore bounds the concurrency, and a job is only claimed once a slot is + /// free, so the jobs waiting for one stay `pending`. Running jobs are left to + /// finish; the unfinished ones stay `processing` and are requeued at the next + /// startup. pub async fn start(&self, shutdown: CancellationToken) -> anyhow::Result<()> { info!("Bot started"); let semaphore = Arc::new(Semaphore::new(self.max_concurrent)); - let mut tasks = tokio::task::JoinSet::new(); - let mut pending = std::pin::pin!(self.queue.pending()); + let mut tasks = JoinSet::new(); - loop { - let job = tokio::select! { + 'consume: loop { + let permit = tokio::select! { biased; - _ = shutdown.cancelled() => break, - job = pending.next() => match job { - Some(job) => job, - None => break, - }, + () = shutdown.cancelled() => break, + permit = semaphore.clone().acquire_owned() => permit?, }; - while let Some(res) = tasks.try_join_next() { - if let Err(err) = res { - error!("Task panicked: {err}"); + let job = loop { + while let Some(res) = tasks.try_join_next() { + if let Err(err) = res { + error!("Task panicked: {err}"); + } } - } - let permit = semaphore.clone().acquire_owned().await?; + match self.deps.queue.claim_next().await { + Ok(Some(job)) => break job, + Ok(None) => { + tokio::select! { + biased; + () = shutdown.cancelled() => break 'consume, + () = self.deps.queue.wait_for_work() => {} + } + } + Err(err) => { + error!(%err, "failed to claim a job"); - if let Err(err) = self.queue.mark_processing(job.id).await { - error!(%err, "failed to mark job as processing"); - } + tokio::select! { + biased; + () = shutdown.cancelled() => break 'consume, + () = tokio::time::sleep(CLAIM_RETRY_DELAY) => {} + } + } + } + }; let bot = self.clone(); metrics::increment_task_active(); @@ -212,30 +107,35 @@ impl Bot { Ok(()) } + /// Runs a claimed job and records its outcome. async fn process(&self, job: Job) { + self.run_job(&job).await; + + if let Err(err) = self.deps.queue.remove(job.id).await { + error!(%err, "failed to remove job"); + } + } + + async fn run_job(&self, job: &Job) { + let deps = &self.deps; let event_type = job.payload.event_type_str(); - match self.dedup.is_reviewed(job.event_id).await { + match deps.reviews.is_reviewed(job.event_id).await { Ok(true) => { metrics::task_skipped(event_type); info!("Review already done, skipping"); - self.event_bus.skip(job.id, &job.repo, job.pr); - - if let Err(err) = self.queue.remove(job.id).await { - error!(%err, "failed to remove job"); - } + deps.event_bus.skip(job.id, &job.repo, job.pr); return; } Ok(false) => {} - Err(err) => error!(%err, "Dedup lookup failed, reviewing anyway"), + Err(err) => error!(%err, "Review lookup failed, reviewing anyway"), } - let reporter = self.event_bus.start(job.id, job.repo.clone(), job.pr); + let reporter = deps.event_bus.start(job.id, job.repo.clone(), job.pr); - let payload = job.payload.0.clone(); let started = std::time::Instant::now(); - let result = self.exec(payload, &reporter).await; - let duration_ms = started.elapsed().as_millis() as u64; + let result = self.exec(&job.payload, &reporter).await; + let duration_ms = u64::try_from(started.elapsed().as_millis()).unwrap_or(u64::MAX); let (ok, outcome) = match result { Ok(outcome) => { @@ -251,60 +151,47 @@ impl Bot { } }; - let comments = outcome.comments as u64; - let record = ReviewRecord { event_id: job.event_id, repo: job.repo.clone(), pr: job.pr, ok, - model: self.open_router_model.clone(), + model: deps.open_router.model().to_string(), markdown: outcome.markdown, cost: outcome.cost, - comments, + comments: outcome.comments, duration_ms, }; - if let Err(err) = self.dedup.mark_reviewed(record).await { + if let Err(err) = deps.reviews.record(record).await { error!(%err, "failed to record review"); } - self.event_bus - .finish(job.id, &job.repo, job.pr, ok, comments, duration_ms); - - if let Err(err) = self.queue.remove(job.id).await { - error!(%err, "failed to remove job"); - } + deps.event_bus + .finish(job.id, &job.repo, job.pr, ok, outcome.comments, duration_ms); } - #[instrument(skip(self, webhook, reporter), fields(repo, pr))] - pub async fn exec( + /// Dispatches a webhook to the action it asks for. + #[instrument( + skip_all, + fields(repo = %webhook.repository_full_name(), pr = webhook.pull_request_number()) + )] + async fn exec( &self, - webhook: WebhookType, + webhook: &WebhookType, reporter: &Reporter, ) -> anyhow::Result { - match &webhook { - WebhookType::Review(p) => { - tracing::Span::current().record("repo", &p.repository.full_name); - tracing::Span::current().record("pr", p.pull_request.number); - } + let deps = &self.deps; + let ctx = ReviewContext { + gitea: &deps.gitea, + open_router: &deps.open_router, + sandbox: &deps.sandbox, + reporter, }; - - let tools = crate::sandbox::tools::for_webhook(&webhook); + let tools = tools::for_webhook(webhook); match webhook { - WebhookType::Review(review_payload) => { - crate::bot_actions::review::exec_review( - &self.gitea_api, - &self.open_router_client, - &self.open_router_model, - &self.sandbox, - tools, - review_payload, - reporter, - ) - .await - } + WebhookType::Review(payload) => review::exec_review(&ctx, tools, payload).await, } } } diff --git a/crates/steep-server/src/bot_actions/review.rs b/crates/steep-server/src/bot_actions/review.rs deleted file mode 100644 index 3fe8148..0000000 --- a/crates/steep-server/src/bot_actions/review.rs +++ /dev/null @@ -1,1192 +0,0 @@ -use std::str::FromStr; - -use tracing::{info, instrument, warn}; - -use crate::{ - bot::{ReviewItem, ReviewResult, ReviewSeverity, ReviewSide}, - consts::{BOT_PROCESS_MSG, REVIEW_PROMPT, SANDBOX_SYSTEM_PROMPT}, - event_bus::Reporter, - gitea::{GiteaAPI, PullRequestFile, ReviewPayload}, - metrics, - open_router::{OpenRouterClient, Tool}, - sandbox::{Sandbox, SandboxConfig, agent}, - text::excerpt, -}; - -pub struct ReviewOutcome { - pub markdown: String, - pub cost: Option, - pub comments: usize, -} - -impl ReviewOutcome { - pub fn empty() -> Self { - Self { - markdown: String::new(), - cost: None, - comments: 0, - } - } -} - -#[instrument(skip( - gitea_api, - open_router_client, - sandbox_config, - tools, - review_payload, - reporter -))] -pub async fn exec_review( - gitea_api: &GiteaAPI, - open_router_client: &OpenRouterClient, - model: &str, - sandbox_config: &SandboxConfig, - tools: Vec, - review_payload: ReviewPayload, - reporter: &Reporter, -) -> anyhow::Result { - tracing::info!( - repo = %review_payload.repository.full_name, - pr = review_payload.pull_request.number, - action = %review_payload.action, - "Starting review" - ); - - let new_comment = gitea_api - .comment( - &BOT_PROCESS_MSG.replace("{model}", model), - &review_payload.repository.full_name, - review_payload.pull_request.number, - ) - .await?; - - let bot_result: Result<(ReviewResult, String), anyhow::Error> = async { - let full_name = &review_payload.repository.full_name; - let index = review_payload.pull_request.number; - - let git_diff = gitea_api.pull_request_diff(full_name, index).await?; - - // The file list only refines the paths and describes the changes: a - // failure is not fatal, the diff is the source of truth for the lines. - let files = match gitea_api.pull_request_files(full_name, index).await { - Ok(files) => files, - Err(err) => { - warn!(%err, "Failed to list the pull request files"); - Vec::new() - } - }; - - let mut changed_lines = parse_changed_lines(&git_diff); - drop_generated_files(&mut changed_lines); - resolve_filenames(&mut changed_lines, &files); - - let changes = format_changes(&files, &changed_lines); - - let bot_request = REVIEW_PROMPT - .replace("{subject}", &review_payload.pull_request.title) - .replace("{comment}", &review_payload.comment.body) - .replace("{changes}", &changes); - - let (mut review_result, cost) = run_sandboxed_review( - gitea_api, - open_router_client, - sandbox_config, - tools, - &review_payload, - &bot_request, - reporter, - ) - .await?; - - resolve_review_sides(&mut review_result, &changed_lines); - - review_result.cost = cost; - if let Some(cost) = review_result.cost { - metrics::openrouter_cost_usd(cost); - } - - let final_review_markdown = review_result_to_markdown(&review_result); - - gitea_api - .post_pull_request_review( - &review_result, - &final_review_markdown, - &review_payload.repository.full_name, - review_payload.pull_request.number, - ) - .await?; - - Ok((review_result, final_review_markdown)) - } - .await; - - match bot_result { - Ok((review_result, markdown)) => { - gitea_api - .delete_comment(&review_payload.repository.full_name, new_comment.id) - .await?; - - Ok(ReviewOutcome { - markdown, - cost: review_result.cost, - comments: review_result.reviews.len(), - }) - } - Err(e) => { - gitea_api - .edit_comment( - &format!("Error while reviewing: {}", e), - &review_payload.repository.full_name, - new_comment.id, - ) - .await?; - - Err(e) - } - } -} - -async fn run_sandboxed_review( - gitea_api: &GiteaAPI, - open_router_client: &OpenRouterClient, - sandbox_config: &SandboxConfig, - tools: Vec, - review_payload: &ReviewPayload, - bot_request: &str, - reporter: &Reporter, -) -> anyhow::Result<(ReviewResult, Option)> { - let sandbox = Sandbox::create( - &sandbox_config.runtime, - &review_payload.repository.clone_url, - gitea_api.token(), - review_payload.pull_request.number, - reporter, - ) - .await?; - - let result = agent::run( - open_router_client, - &sandbox, - tools, - SANDBOX_SYSTEM_PROMPT, - bot_request, - sandbox_config.max_iterations, - reporter, - ) - .await; - - if let Err(err) = sandbox.cleanup().await { - warn!(%err, "Failed to clean up sandbox container"); - } - - let result = result?; - info!( - iterations = result.iterations, - cost = ?result.cost, - "Sandboxed review finished" - ); - - Ok((result.answer, result.cost)) -} - -fn review_result_to_markdown(review_result: &ReviewResult) -> String { - if review_result.reviews.is_empty() { - return String::from("No issues found. ✅"); - } - - let mut md = String::from("## Review Feedback\n\n"); - - md.push_str(&format!( - "### {} issues found.\n\n", - review_result.reviews.len() - )); - - let breakdown = severity_breakdown(&review_result.reviews); - if !breakdown.is_empty() { - md.push_str(&breakdown); - md.push('\n'); - } - - if !review_result.comment.is_empty() { - md.push_str("\n---\n\n"); - md.push_str("### Summary\n\n"); - md.push_str(&review_result.comment); - md.push('\n'); - } - - if let Some(cost) = review_result.cost { - md.push_str("\n---\n\n"); - md.push_str(&format!("### Cost: ${}", cost)); - md.push('\n'); - } - - md -} - -/// Counts the reviews per severity, most severe first. -/// -/// A review whose severity the model omitted or wrote unreadably is counted as -/// [`ReviewSeverity::Maintainability`], the same fallback used when posting it. -fn severity_breakdown(reviews: &[ReviewItem]) -> String { - [ - ReviewSeverity::Bug, - ReviewSeverity::Security, - ReviewSeverity::Performance, - ReviewSeverity::Maintainability, - ] - .into_iter() - .filter_map(|severity| { - let count = reviews - .iter() - .filter(|review| review.severity.unwrap_or(ReviewSeverity::Maintainability) == severity) - .count(); - - (count > 0).then(|| format!("- {} {}", count, severity.label())) - }) - .collect::>() - .join("\n") -} - -/// The lines a pull request changed, per file. -/// -/// Line numbers are the ones Gitea expects to place a review comment: -/// [`ReviewSide::Added`] numbers refer to the new version of the file (sent as -/// `new_position`), [`ReviewSide::Removed`] numbers to the old version (sent as -/// `old_position`). -type ChangedLines = Vec; - -struct ChangedFile { - filename: String, - added: Vec, - removed: Vec, -} - -impl ChangedFile { - fn new(filename: &str) -> Self { - Self { - filename: String::from(filename), - added: Vec::new(), - removed: Vec::new(), - } - } - - /// Lines that can be commented on for the given side. - fn lines(&self, side: ReviewSide) -> &[u64] { - match side { - ReviewSide::Added => &self.added, - ReviewSide::Removed => &self.removed, - } - } - - /// Side a line belongs to, used when the model did not state one. - fn side_of(&self, line: u64) -> Option { - if self.added.contains(&line) { - Some(ReviewSide::Added) - } else if self.removed.contains(&line) { - Some(ReviewSide::Removed) - } else { - None - } - } -} - -/// Lists the lines changed by the diff, per file, on both sides. -/// -/// Only the line numbers are kept: the model reads the code itself through the -/// sandbox tools. -fn parse_changed_lines(git_diff: &str) -> ChangedLines { - let mut files = Vec::new(); - let mut current_file: Option = None; - let mut in_hunk = false; - let mut old_line: u64 = 0; - let mut new_line: u64 = 0; - - for line in git_diff.lines() { - if line.starts_with("diff --git ") { - current_file = None; - in_hunk = false; - continue; - } - - // `--- a/x` and `+++ b/x` only appear before the first hunk of a file. - // Inside a hunk, a line may legitimately start with them: removing - // `--x` gives `---x`, adding `++i;` gives `+++i;`. - if !in_hunk && (line.starts_with("--- ") || line.starts_with("+++ ")) { - // A header is never a content line: `+++ /dev/null` on a deleted - // file keeps the path of the other side in place. - if let Some(path) = header_file_path(line) { - current_file = Some(path); - } - continue; - } - - if line.starts_with("@@") { - if let Some((old_start, new_start)) = parse_hunk_starts(line) { - old_line = old_start; - new_line = new_start; - in_hunk = true; - } - continue; - } - - let Some(filename) = current_file.as_deref() else { - continue; - }; - - match line.as_bytes().first() { - Some(b' ') => { - old_line += 1; - new_line += 1; - } - Some(b'-') => { - changed_file(&mut files, filename).removed.push(old_line); - old_line += 1; - } - Some(b'+') => { - changed_file(&mut files, filename).added.push(new_line); - new_line += 1; - } - // `\ No newline at end of file`, and anything unexpected: a line - // that advances neither side. - _ => {} - } - } - - files -} - -/// Path of the file on one side of the diff, from a `--- a/` or -/// `+++ b/` header line. -/// -/// These lines are the only unambiguous source for the path: the `diff --git` -/// line is cut at the first space, and it names the old path of a renamed file. -/// `None` for `/dev/null`, which leaves the path of the other side in place. -fn header_file_path(line: &str) -> Option { - let (prefix, raw) = match line.strip_prefix("--- ") { - Some(raw) => ("a/", raw), - None => ("b/", line.strip_prefix("+++ ")?), - }; - - let path = decode_git_path(raw); - - Some(String::from(path.strip_prefix(prefix)?)) -} - -/// Decodes a path as git writes it in a diff header: git wraps it in quotes and -/// escapes the bytes that need it (`\303\251` for `é`) when the path contains -/// non-printable or non-ASCII characters. -fn decode_git_path(raw: &str) -> String { - let Some(quoted) = raw.strip_prefix('"').and_then(|raw| raw.strip_suffix('"')) else { - return String::from(raw); - }; - - let bytes = quoted.as_bytes(); - let mut decoded = Vec::with_capacity(bytes.len()); - let mut index = 0; - - while let Some(byte) = bytes.get(index) { - index += 1; - - if *byte != b'\\' { - decoded.push(*byte); - continue; - } - - match bytes.get(index) { - // Octal escapes are the ones that matter for a path: git uses them - // for every non-ASCII byte. - Some(digit @ b'0'..=b'7') => { - let mut value = u32::from(digit - b'0'); - let mut digits = 1; - - while digits < 3 { - match bytes.get(index + digits) { - Some(next @ b'0'..=b'7') => { - value = value * 8 + u32::from(next - b'0'); - digits += 1; - } - _ => break, - } - } - - decoded.push(u8::try_from(value).unwrap_or(b'?')); - index += digits; - } - Some(escaped) => { - decoded.push(match escaped { - b't' => b'\t', - b'n' => b'\n', - b'r' => b'\r', - other => *other, - }); - index += 1; - } - None => decoded.push(b'\\'), - } - } - - String::from_utf8_lossy(&decoded).into_owned() -} - -/// Replaces the paths parsed from the diff with the exact paths reported by the -/// API, which are the ones the model sees in the sandbox. -fn resolve_filenames(changed_lines: &mut ChangedLines, files: &[PullRequestFile]) { - for changed in changed_lines.iter_mut() { - let parsed = changed.filename.as_str(); - - let Some(file) = files.iter().find(|file| file.filename == parsed) else { - if !files.is_empty() { - warn!(path = %parsed, "Changed file is not in the pull request file list"); - } - continue; - }; - - changed.filename = file.filename.clone(); - } -} - -/// Basenames of generated files that are never reviewed. -/// -/// Their diffs are machine-written dependency churn: they run to thousands of -/// lines, flood the prompt with line numbers and drown the code the model should -/// look at. A lockfile is also never a place where a review comment belongs. -const IGNORED_FILE_NAMES: [&str; 16] = [ - "Cargo.lock", - "package-lock.json", - "npm-shrinkwrap.json", - "yarn.lock", - "pnpm-lock.yaml", - "bun.lock", - "bun.lockb", - "composer.lock", - "Gemfile.lock", - "poetry.lock", - "uv.lock", - "Pipfile.lock", - "go.sum", - "packages.lock.json", - "flake.lock", - "pubspec.lock", -]; - -/// Whether a changed path is a generated file that is never reviewed. -/// -/// The comparison is on the basename, so a lockfile nested in a workspace (for -/// example `crates/foo/Cargo.lock`) is matched too. -fn is_ignored(filename: &str) -> bool { - let basename = filename.rsplit('/').next().unwrap_or(filename); - - IGNORED_FILE_NAMES.contains(&basename) -} - -/// Drops the generated files from the changed lines. -/// -/// Removing them here keeps the prompt free of their line numbers, and also makes -/// [`resolve_review_sides`] reject any review the model anchors on one of them. -fn drop_generated_files(changed_lines: &mut ChangedLines) { - changed_lines.retain(|file| !is_ignored(&file.filename)); -} - -/// Renders the changes for the model: the files the pull request touches, then -/// the lines to review per file. -fn format_changes(files: &[PullRequestFile], changed_lines: &ChangedLines) -> String { - let mut sections = Vec::new(); - - let described = files - .iter() - .filter(|file| !is_ignored(&file.filename)) - .map(describe_file) - .collect::>() - .join(", "); - - if !described.is_empty() { - sections.push(format!("Files changed by the pull request: {described}")); - } - - sections.push(format!( - "Lines to review, per file:\n{}", - format_changed_lines(changed_lines) - )); - - sections.join("\n\n") -} - -/// Describes a changed file for the model, including how it changed. -fn describe_file(file: &PullRequestFile) -> String { - match &file.previous_filename { - Some(previous) => format!("{} ({} from {})", file.filename, file.status, previous), - None => format!("{} ({})", file.filename, file.status), - } -} - -/// Borrows the entry of `files` for a file, creating it on its first change. -fn changed_file<'a>(files: &'a mut ChangedLines, filename: &str) -> &'a mut ChangedFile { - let index = match files.iter().position(|file| file.filename == filename) { - Some(index) => index, - None => { - files.push(ChangedFile::new(filename)); - files.len() - 1 - } - }; - - &mut files[index] -} - -/// Renders the changed lines as `filename: added 1, 2 / removed 3`, one file per -/// line, keeping only the sides the file actually has. -fn format_changed_lines(changed_lines: &ChangedLines) -> String { - changed_lines - .iter() - .map(|file| { - let mut sides = Vec::new(); - - for (label, lines) in [("added", &file.added), ("removed", &file.removed)] { - if !lines.is_empty() { - sides.push(format!("{label} {}", format_line_numbers(lines))); - } - } - - format!("{}: {}", file.filename, sides.join(" / ")) - }) - .collect::>() - .join("\n") -} - -fn format_line_numbers(lines: &[u64]) -> String { - lines - .iter() - .map(u64::to_string) - .collect::>() - .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. -/// -/// The model is asked to pick a line and a side from the provided lists, but -/// nothing forces it to, and Gitea accepts any position: a wrong one places the -/// comment on an unrelated line of the file instead of failing. A review that -/// omits its side is resolved from the lists, and one that matches no changed -/// line is dropped. -fn resolve_review_sides(review_result: &mut ReviewResult, changed_lines: &ChangedLines) { - let mut dropped = Vec::new(); - - review_result.reviews.retain_mut(|review| { - let side = review.line.and_then(|line| { - let file = changed_lines - .iter() - .find(|file| file.filename == review.filename)?; - - let side = review.side.or_else(|| file.side_of(line))?; - - file.lines(side).contains(&line).then_some(side) - }); - - match side { - Some(side) => { - review.side = Some(side); - true - } - None => { - dropped.push(match review.line { - Some(line) => format!("{}:{line}", review.filename), - None => format!("{}:no line", review.filename), - }); - false - } - } - }); - - if !dropped.is_empty() { - warn!( - dropped = dropped.len(), - reviews = %dropped.join(", "), - "Dropped reviews that are not anchored on a changed line" - ); - } -} - -/// Extracts the old and new starting line numbers of a hunk header such as -/// `@@ -12,3 +12,5 @@`. The counts are optional and git may append a section -/// heading after the closing `@@`. -fn parse_hunk_starts(hunk_header: &str) -> Option<(u64, u64)> { - let body = hunk_header.strip_prefix("@@ ")?; - let body = body.split(" @@").next()?; - - let (old, new) = body.split_once(" +")?; - let old = old.strip_prefix('-')?.split(',').next()?; - let new = new.split(',').next()?; - - Some((old.parse().ok()?, new.parse().ok()?)) -} - -#[cfg(test)] -mod tests { - use super::*; - use crate::bot::ReviewItem; - - /// Additions, a removal and a line changed on both sides. - const DIFF: &str = concat!( - "diff --git a/src/foo.rs b/src/foo.rs\n", - "--- a/src/foo.rs\n", - "+++ b/src/foo.rs\n", - "@@ -1,4 +1,6 @@\n", - " fn main() {\n", - "+ let x = 1;\n", - "- let removed = 0;\n", - " println!(\"hello\");\n", - "+ let y = 2;\n", - "+ let z = 3;\n", - " }\n", - "diff --git a/src/bar.rs b/src/bar.rs\n", - "--- a/src/bar.rs\n", - "+++ b/src/bar.rs\n", - "@@ -10,4 +10,6 @@\n", - " old context\n", - "+ let a = 10;\n", - " more context\n", - "+ let b = 20;\n", - ); - - /// A pull request that only deletes a file. - const DELETION_ONLY: &str = concat!( - "diff --git a/src/old.rs b/src/old.rs\n", - "deleted file mode 100644\n", - "--- a/src/old.rs\n", - "+++ /dev/null\n", - "@@ -1,3 +0,0 @@\n", - "-fn a() {}\n", - "-fn b() {}\n", - "-fn c() {}\n", - ); - - /// A hunk whose content starts with `+++` / `---`, which must not be taken - /// for the file headers. - const TRICKY_CONTENT: &str = concat!( - "diff --git a/src/tricky.js b/src/tricky.js\n", - "--- a/src/tricky.js\n", - "+++ b/src/tricky.js\n", - "@@ -1,4 +1,4 @@\n", - " let i = 0;\n", - "+++i;\n", - "---x;\n", - " console.log(i);\n", - ); - - /// A pull request that renames a file. - const RENAMED: &str = concat!( - "diff --git a/src/old.rs b/src/new.rs\n", - "similarity index 50%\n", - "rename from src/old.rs\n", - "rename to src/new.rs\n", - "--- a/src/old.rs\n", - "+++ b/src/new.rs\n", - "@@ -1,1 +1,1 @@\n", - "-fn old() {}\n", - "+fn new() {}\n", - ); - - /// A file whose path contains a space, which the `diff --git` line cannot - /// express without ambiguity. - const PATH_WITH_SPACE: &str = concat!( - "diff --git a/src/my file.rs b/src/my file.rs\n", - "--- a/src/my file.rs\n", - "+++ b/src/my file.rs\n", - "@@ -1,1 +1,2 @@\n", - " fn a() {}\n", - "+fn b() {}\n", - ); - - /// A file whose path git quotes and escapes (`caf\\303\\251.md` is - /// `caf\u{e9}.md`). - const QUOTED_PATH: &str = concat!( - "diff --git \"a/docs/caf\\303\\251.md\" \"b/docs/caf\\303\\251.md\"\n", - "--- \"a/docs/caf\\303\\251.md\"\n", - "+++ \"b/docs/caf\\303\\251.md\"\n", - "@@ -1,1 +1,2 @@\n", - " intro\n", - "+ajout\n", - ); - - fn review(filename: &str, line: Option, side: Option) -> ReviewItem { - ReviewItem { - filename: String::from(filename), - line, - side, - severity: None, - message: String::from("message"), - } - } - - fn review_result(reviews: Vec) -> ReviewResult { - ReviewResult { - reviews, - comment: String::new(), - cost: None, - } - } - - fn pull_request_file( - filename: &str, - previous_filename: Option<&str>, - status: &str, - ) -> PullRequestFile { - PullRequestFile { - filename: String::from(filename), - previous_filename: previous_filename.map(String::from), - status: String::from(status), - } - } - - #[test] - fn changed_lines_are_listed_per_file_and_side() { - let expected = concat!( - "src/foo.rs: added 2, 4, 5 / removed 2\n", - "src/bar.rs: added 11, 13" - ); - - assert_eq!(format_changed_lines(&parse_changed_lines(DIFF)), expected); - } - - #[test] - fn a_deletion_only_pull_request_lists_removed_lines() { - let expected = "src/old.rs: removed 1, 2, 3"; - - assert_eq!( - format_changed_lines(&parse_changed_lines(DELETION_ONLY)), - expected - ); - } - - #[test] - fn hunk_content_starting_with_plus_or_minus_is_counted() { - let expected = "src/tricky.js: added 2 / removed 2"; - - assert_eq!( - format_changed_lines(&parse_changed_lines(TRICKY_CONTENT)), - expected - ); - } - - #[test] - fn a_renamed_file_uses_its_new_path() { - let expected = "src/new.rs: added 1 / removed 1"; - - assert_eq!( - format_changed_lines(&parse_changed_lines(RENAMED)), - expected - ); - } - - #[test] - fn a_path_with_a_space_is_read_from_the_headers() { - let expected = "src/my file.rs: added 2"; - - assert_eq!( - format_changed_lines(&parse_changed_lines(PATH_WITH_SPACE)), - expected - ); - } - - #[test] - fn a_quoted_path_is_decoded() { - let expected = "docs/café.md: added 2"; - - assert_eq!( - format_changed_lines(&parse_changed_lines(QUOTED_PATH)), - expected - ); - } - - #[test] - fn filenames_are_resolved_against_the_api_list() { - let mut changed_lines = parse_changed_lines(DIFF); - let files = vec![ - pull_request_file("src/bar.rs", None, "modified"), - pull_request_file("src/foo.rs", Some("src/renamed.rs"), "renamed"), - ]; - - resolve_filenames(&mut changed_lines, &files); - - assert_eq!(changed_lines[0].filename, "src/foo.rs"); - assert_eq!(changed_lines[1].filename, "src/bar.rs"); - } - - #[test] - fn a_file_absent_from_the_api_list_keeps_the_diff_path() { - let mut changed_lines = parse_changed_lines(DIFF); - let files = vec![pull_request_file("src/bar.rs", None, "modified")]; - - resolve_filenames(&mut changed_lines, &files); - - assert_eq!(changed_lines[0].filename, "src/foo.rs"); - assert_eq!(changed_lines[1].filename, "src/bar.rs"); - } - - #[test] - fn changes_describe_the_files_then_the_lines() { - let changed_lines = parse_changed_lines(DELETION_ONLY); - let files = vec![pull_request_file("src/old.rs", None, "deleted")]; - - let expected = concat!( - "Files changed by the pull request: src/old.rs (deleted)\n", - "\n", - "Lines to review, per file:\n", - "src/old.rs: removed 1, 2, 3" - ); - - assert_eq!(format_changes(&files, &changed_lines), expected); - } - - #[test] - fn changes_without_the_api_list_only_hold_the_lines() { - let changed_lines = parse_changed_lines(DELETION_ONLY); - - let expected = "Lines to review, per file:\nsrc/old.rs: removed 1, 2, 3"; - - assert_eq!(format_changes(&[], &changed_lines), expected); - } - - #[test] - fn generated_lockfiles_are_ignored() { - assert!(is_ignored("Cargo.lock")); - assert!(is_ignored("crates/steep-server/Cargo.lock")); - assert!(is_ignored("package-lock.json")); - assert!(is_ignored("web/yarn.lock")); - assert!(is_ignored("go.sum")); - - assert!(!is_ignored("src/main.rs")); - assert!(!is_ignored("Cargo.toml")); - assert!(!is_ignored("docs/lockfiles.md")); - } - - #[test] - fn generated_files_are_dropped_from_the_changed_lines() { - const DIFF_WITH_LOCKFILE: &str = concat!( - "diff --git a/Cargo.lock b/Cargo.lock\n", - "--- a/Cargo.lock\n", - "+++ b/Cargo.lock\n", - "@@ -1,1 +1,2 @@\n", - " [[package]]\n", - "+name = \"x\"\n", - "diff --git a/src/foo.rs b/src/foo.rs\n", - "--- a/src/foo.rs\n", - "+++ b/src/foo.rs\n", - "@@ -1,1 +1,2 @@\n", - " fn a() {}\n", - "+fn b() {}\n", - ); - - let mut changed_lines = parse_changed_lines(DIFF_WITH_LOCKFILE); - drop_generated_files(&mut changed_lines); - - assert_eq!(format_changed_lines(&changed_lines), "src/foo.rs: added 2"); - } - - #[test] - fn a_generated_file_is_not_described_to_the_model() { - let files = vec![ - pull_request_file("Cargo.lock", None, "modified"), - pull_request_file("src/foo.rs", None, "modified"), - ]; - let changed_lines = parse_changed_lines(DIFF); - - let changes = format_changes(&files, &changed_lines); - - assert!(!changes.contains("Cargo.lock")); - assert!(changes.contains("src/foo.rs (modified)")); - } - - #[test] - fn a_renamed_file_is_described_with_its_previous_path() { - let file = pull_request_file("src/new.rs", Some("src/old.rs"), "renamed"); - - assert_eq!(describe_file(&file), "src/new.rs (renamed from src/old.rs)"); - } - - #[test] - fn reviews_keep_their_changed_line_and_resolve_their_side() { - let changed_lines = parse_changed_lines(DIFF); - let mut review_result = review_result(vec![ - review("src/foo.rs", Some(4), Some(ReviewSide::Added)), - review("src/foo.rs", Some(2), Some(ReviewSide::Removed)), - review("src/bar.rs", Some(13), None), - ]); - - resolve_review_sides(&mut review_result, &changed_lines); - - let sides = review_result - .reviews - .iter() - .map(|review| review.side) - .collect::>(); - - assert_eq!( - sides, - vec![ - Some(ReviewSide::Added), - Some(ReviewSide::Removed), - Some(ReviewSide::Added) - ] - ); - } - - #[test] - fn a_line_changed_on_both_sides_defaults_to_added() { - let changed_lines = parse_changed_lines(DIFF); - let mut review_result = review_result(vec![review("src/foo.rs", Some(2), None)]); - - resolve_review_sides(&mut review_result, &changed_lines); - - assert_eq!(review_result.reviews[0].side, Some(ReviewSide::Added)); - } - - #[test] - fn reviews_outside_changed_lines_are_dropped() { - let changed_lines = parse_changed_lines(DIFF); - let mut review_result = review_result(vec![ - // valid - review("src/foo.rs", Some(2), Some(ReviewSide::Added)), - // a line that exists but is not part of the change - review("src/foo.rs", Some(3), None), - // a line beyond the change - review("src/foo.rs", Some(999), None), - // a changed line, but on the wrong side - review("src/foo.rs", Some(4), Some(ReviewSide::Removed)), - // a line changed in another file - review("src/foo.rs", Some(11), None), - // an unknown file - review("src/baz.rs", Some(1), None), - // no line at all - review("src/bar.rs", None, None), - ]); - - resolve_review_sides(&mut review_result, &changed_lines); - - assert_eq!(review_result.reviews.len(), 1); - assert_eq!(review_result.reviews[0].filename, "src/foo.rs"); - assert_eq!(review_result.reviews[0].line, Some(2)); - assert_eq!(review_result.reviews[0].side, Some(ReviewSide::Added)); - } - - #[test] - fn a_deleted_line_is_kept_as_a_removed_anchor() { - let changed_lines = parse_changed_lines(DELETION_ONLY); - let mut review_result = review_result(vec![ - review("src/old.rs", Some(2), Some(ReviewSide::Removed)), - review("src/old.rs", Some(2), Some(ReviewSide::Added)), - ]); - - resolve_review_sides(&mut review_result, &changed_lines); - - assert_eq!(review_result.reviews.len(), 1); - 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); - let mut review_result: ReviewResult = serde_json::from_str( - r#"{ - "reviews": [ - { "filename": "src/foo.rs", "line": 4, "side": "Added", "message": "a" }, - { "filename": "src/foo.rs", "line": 2, "side": "new", "message": "b" }, - { "filename": "src/foo.rs", "line": 2, "side": "REMOVED", "message": "c" }, - { "filename": "src/foo.rs", "line": 2, "side": "banana", "message": "d" }, - { "filename": "src/foo.rs", "line": 5, "message": "e" } - ], - "comment": "" - }"#, - ) - .unwrap(); - - resolve_review_sides(&mut review_result, &changed_lines); - - let sides = review_result - .reviews - .iter() - .map(|review| review.side) - .collect::>(); - - assert_eq!( - sides, - vec![ - Some(ReviewSide::Added), - Some(ReviewSide::Added), - Some(ReviewSide::Removed), - Some(ReviewSide::Added), - Some(ReviewSide::Added) - ] - ); - } - - #[test] - fn severities_are_read_from_the_model() { - let result: ReviewResult = serde_json::from_str( - r#"{ - "reviews": [ - { "filename": "a", "line": 1, "severity": "Bug", "message": "x" }, - { "filename": "b", "line": 2, "severity": "SECURITY", "message": "x" }, - { "filename": "c", "line": 3, "severity": "perf", "message": "x" }, - { "filename": "d", "line": 4, "severity": "banana", "message": "x" } - ], - "comment": "" - }"#, - ) - .unwrap(); - - let severities = result - .reviews - .iter() - .map(|review| review.severity) - .collect::>(); - - assert_eq!( - severities, - vec![ - Some(ReviewSeverity::Bug), - Some(ReviewSeverity::Security), - Some(ReviewSeverity::Performance), - None, - ] - ); - } - - #[test] - fn the_breakdown_counts_severities_most_severe_first() { - let reviews = vec![ - ReviewItem { - severity: Some(ReviewSeverity::Maintainability), - ..review("a", Some(1), None) - }, - ReviewItem { - severity: Some(ReviewSeverity::Bug), - ..review("b", Some(2), None) - }, - ReviewItem { - severity: Some(ReviewSeverity::Bug), - ..review("c", Some(3), None) - }, - // A severity the model left out counts as maintainability. - review("d", Some(4), None), - ]; - - assert_eq!(severity_breakdown(&reviews), "- 2 bug\n- 2 maintainability"); - } - - #[test] - fn the_markdown_breaks_the_issues_down_by_severity() { - let mut result = review_result(vec![ - ReviewItem { - severity: Some(ReviewSeverity::Bug), - ..review("a", Some(1), None) - }, - ReviewItem { - severity: Some(ReviewSeverity::Performance), - ..review("b", Some(2), None) - }, - ]); - result.comment = String::from("Le découpage en crates est propre."); - - let markdown = review_result_to_markdown(&result); - - assert!(markdown.contains("- 1 bug")); - assert!(markdown.contains("- 1 performance")); - assert!(markdown.contains("Le découpage en crates est propre.")); - } -} diff --git a/crates/steep-server/src/bot_actions/review/anchor.rs b/crates/steep-server/src/bot_actions/review/anchor.rs new file mode 100644 index 0000000..0691188 --- /dev/null +++ b/crates/steep-server/src/bot_actions/review/anchor.rs @@ -0,0 +1,177 @@ +//! Anchoring of the model's reviews on the lines a pull request changes. + +use tracing::warn; + +use super::diff::ChangedLines; +use crate::domain::review::ReviewResult; + +/// Resolves the side each review is anchored on and drops the reviews that do +/// not match a line the pull request changes. +/// +/// The model is asked to pick a line and a side from the provided lists, but +/// nothing forces it to, and Gitea accepts any position: a wrong one places the +/// comment on an unrelated line of the file instead of failing. A review that +/// omits its side is resolved from the lists, and one that matches no changed +/// line is dropped. +pub(super) fn resolve_review_sides(review_result: &mut ReviewResult, changed_lines: &ChangedLines) { + let mut dropped = Vec::new(); + + review_result.reviews.retain_mut(|review| { + let side = review.line.and_then(|line| { + let file = changed_lines + .iter() + .find(|file| file.filename == review.filename)?; + + let side = review.side.or_else(|| file.side_of(line))?; + + file.lines(side).contains(&line).then_some(side) + }); + + match side { + Some(side) => { + review.side = Some(side); + true + } + None => { + dropped.push(match review.line { + Some(line) => format!("{}:{line}", review.filename), + None => format!("{}:no line", review.filename), + }); + false + } + } + }); + + if !dropped.is_empty() { + warn!( + dropped = dropped.len(), + reviews = %dropped.join(", "), + "Dropped reviews that are not anchored on a changed line" + ); + } +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::{ + bot_actions::review::{diff::parse_changed_lines, fixtures::*}, + domain::review::ReviewSide, + }; + + #[test] + fn reviews_keep_their_changed_line_and_resolve_their_side() { + let changed_lines = parse_changed_lines(DIFF); + let mut review_result = review_result(vec![ + review("src/foo.rs", Some(4), Some(ReviewSide::Added)), + review("src/foo.rs", Some(2), Some(ReviewSide::Removed)), + review("src/bar.rs", Some(13), None), + ]); + + resolve_review_sides(&mut review_result, &changed_lines); + + let sides = review_result + .reviews + .iter() + .map(|review| review.side) + .collect::>(); + + assert_eq!( + sides, + vec![ + Some(ReviewSide::Added), + Some(ReviewSide::Removed), + Some(ReviewSide::Added) + ] + ); + } + + #[test] + fn a_line_changed_on_both_sides_defaults_to_added() { + let changed_lines = parse_changed_lines(DIFF); + let mut review_result = review_result(vec![review("src/foo.rs", Some(2), None)]); + + resolve_review_sides(&mut review_result, &changed_lines); + + assert_eq!(review_result.reviews[0].side, Some(ReviewSide::Added)); + } + + #[test] + fn reviews_outside_changed_lines_are_dropped() { + let changed_lines = parse_changed_lines(DIFF); + let mut review_result = review_result(vec![ + // valid + review("src/foo.rs", Some(2), Some(ReviewSide::Added)), + // a line that exists but is not part of the change + review("src/foo.rs", Some(3), None), + // a line beyond the change + review("src/foo.rs", Some(999), None), + // a changed line, but on the wrong side + review("src/foo.rs", Some(4), Some(ReviewSide::Removed)), + // a line changed in another file + review("src/foo.rs", Some(11), None), + // an unknown file + review("src/baz.rs", Some(1), None), + // no line at all + review("src/bar.rs", None, None), + ]); + + resolve_review_sides(&mut review_result, &changed_lines); + + assert_eq!(review_result.reviews.len(), 1); + assert_eq!(review_result.reviews[0].filename, "src/foo.rs"); + assert_eq!(review_result.reviews[0].line, Some(2)); + assert_eq!(review_result.reviews[0].side, Some(ReviewSide::Added)); + } + + #[test] + fn a_deleted_line_is_kept_as_a_removed_anchor() { + let changed_lines = parse_changed_lines(DELETION_ONLY); + let mut review_result = review_result(vec![ + review("src/old.rs", Some(2), Some(ReviewSide::Removed)), + review("src/old.rs", Some(2), Some(ReviewSide::Added)), + ]); + + resolve_review_sides(&mut review_result, &changed_lines); + + assert_eq!(review_result.reviews.len(), 1); + assert_eq!(review_result.reviews[0].side, Some(ReviewSide::Removed)); + } + + #[test] + fn odd_sides_from_the_model_are_tolerated() { + let changed_lines = parse_changed_lines(DIFF); + let mut review_result: ReviewResult = serde_json::from_str( + r#"{ + "reviews": [ + { "filename": "src/foo.rs", "line": 4, "side": "Added", "message": "a" }, + { "filename": "src/foo.rs", "line": 2, "side": "new", "message": "b" }, + { "filename": "src/foo.rs", "line": 2, "side": "REMOVED", "message": "c" }, + { "filename": "src/foo.rs", "line": 2, "side": "banana", "message": "d" }, + { "filename": "src/foo.rs", "line": 5, "message": "e" } + ], + "comment": "" + }"#, + ) + .unwrap(); + + resolve_review_sides(&mut review_result, &changed_lines); + + let sides = review_result + .reviews + .iter() + .map(|review| review.side) + .collect::>(); + + assert_eq!( + sides, + vec![ + Some(ReviewSide::Added), + Some(ReviewSide::Added), + Some(ReviewSide::Removed), + Some(ReviewSide::Added), + Some(ReviewSide::Added) + ] + ); + } +} diff --git a/crates/steep-server/src/bot_actions/review/diff.rs b/crates/steep-server/src/bot_actions/review/diff.rs new file mode 100644 index 0000000..3a0bc5a --- /dev/null +++ b/crates/steep-server/src/bot_actions/review/diff.rs @@ -0,0 +1,389 @@ +//! Parsing of a pull request unified diff into the lines it changes. + +use tracing::warn; + +use crate::{domain::review::ReviewSide, gitea::PullRequestFile}; + +/// The lines a pull request changed, per file. +/// +/// Line numbers are the ones Gitea expects to place a review comment: +/// [`ReviewSide::Added`] numbers refer to the new version of the file (sent as +/// `new_position`), [`ReviewSide::Removed`] numbers to the old version (sent as +/// `old_position`). +pub(super) type ChangedLines = Vec; + +pub(super) struct ChangedFile { + pub(super) filename: String, + pub(super) added: Vec, + pub(super) removed: Vec, +} + +impl ChangedFile { + fn new(filename: &str) -> Self { + Self { + filename: String::from(filename), + added: Vec::new(), + removed: Vec::new(), + } + } + + /// Lines that can be commented on for the given side. + pub(super) fn lines(&self, side: ReviewSide) -> &[u64] { + match side { + ReviewSide::Added => &self.added, + ReviewSide::Removed => &self.removed, + } + } + + /// Side a line belongs to, used when the model did not state one. + pub(super) fn side_of(&self, line: u64) -> Option { + if self.added.contains(&line) { + Some(ReviewSide::Added) + } else if self.removed.contains(&line) { + Some(ReviewSide::Removed) + } else { + None + } + } +} + +/// Lists the lines changed by the diff, per file, on both sides. +/// +/// Only the line numbers are kept: the model reads the code itself through the +/// sandbox tools. +pub(super) fn parse_changed_lines(git_diff: &str) -> ChangedLines { + let mut files = Vec::new(); + let mut current_file: Option = None; + let mut in_hunk = false; + let mut old_line: u64 = 0; + let mut new_line: u64 = 0; + + for line in git_diff.lines() { + if line.starts_with("diff --git ") { + current_file = None; + in_hunk = false; + continue; + } + + // `--- a/x` and `+++ b/x` only appear before the first hunk of a file. + // Inside a hunk, a line may legitimately start with them: removing + // `--x` gives `---x`, adding `++i;` gives `+++i;`. + if !in_hunk && (line.starts_with("--- ") || line.starts_with("+++ ")) { + // A header is never a content line: `+++ /dev/null` on a deleted + // file keeps the path of the other side in place. + if let Some(path) = header_file_path(line) { + current_file = Some(path); + } + continue; + } + + if line.starts_with("@@") { + if let Some((old_start, new_start)) = parse_hunk_starts(line) { + old_line = old_start; + new_line = new_start; + in_hunk = true; + } + continue; + } + + let Some(filename) = current_file.as_deref() else { + continue; + }; + + match line.as_bytes().first() { + Some(b' ') => { + old_line += 1; + new_line += 1; + } + Some(b'-') => { + changed_file(&mut files, filename).removed.push(old_line); + old_line += 1; + } + Some(b'+') => { + changed_file(&mut files, filename).added.push(new_line); + new_line += 1; + } + // `\ No newline at end of file`, and anything unexpected: a line + // that advances neither side. + _ => {} + } + } + + files +} + +/// Path of the file on one side of the diff, from a `--- a/` or +/// `+++ b/` header line. +/// +/// These lines are the only unambiguous source for the path: the `diff --git` +/// line is cut at the first space, and it names the old path of a renamed file. +/// `None` for `/dev/null`, which leaves the path of the other side in place. +fn header_file_path(line: &str) -> Option { + let (prefix, raw) = match line.strip_prefix("--- ") { + Some(raw) => ("a/", raw), + None => ("b/", line.strip_prefix("+++ ")?), + }; + + let path = decode_git_path(raw); + + Some(String::from(path.strip_prefix(prefix)?)) +} + +/// Decodes a path as git writes it in a diff header: git wraps it in quotes and +/// escapes the bytes that need it (`\303\251` for `é`) when the path contains +/// non-printable or non-ASCII characters. +fn decode_git_path(raw: &str) -> String { + let Some(quoted) = raw.strip_prefix('"').and_then(|raw| raw.strip_suffix('"')) else { + return String::from(raw); + }; + + let bytes = quoted.as_bytes(); + let mut decoded = Vec::with_capacity(bytes.len()); + let mut index = 0; + + while let Some(byte) = bytes.get(index) { + index += 1; + + if *byte != b'\\' { + decoded.push(*byte); + continue; + } + + match bytes.get(index) { + // Octal escapes are the ones that matter for a path: git uses them + // for every non-ASCII byte. + Some(digit @ b'0'..=b'7') => { + let mut value = u32::from(digit - b'0'); + let mut digits = 1; + + while digits < 3 { + match bytes.get(index + digits) { + Some(next @ b'0'..=b'7') => { + value = value * 8 + u32::from(next - b'0'); + digits += 1; + } + _ => break, + } + } + + decoded.push(u8::try_from(value).unwrap_or(b'?')); + index += digits; + } + Some(escaped) => { + decoded.push(match escaped { + b't' => b'\t', + b'n' => b'\n', + b'r' => b'\r', + other => *other, + }); + index += 1; + } + None => decoded.push(b'\\'), + } + } + + String::from_utf8_lossy(&decoded).into_owned() +} + +/// Replaces the paths parsed from the diff with the exact paths reported by the +/// API, which are the ones the model sees in the sandbox. +pub(super) fn resolve_filenames(changed_lines: &mut ChangedLines, files: &[PullRequestFile]) { + for changed in changed_lines.iter_mut() { + let parsed = changed.filename.as_str(); + + let Some(file) = files.iter().find(|file| file.filename == parsed) else { + if !files.is_empty() { + warn!(path = %parsed, "Changed file is not in the pull request file list"); + } + continue; + }; + + changed.filename = file.filename.clone(); + } +} + +/// Basenames of generated files that are never reviewed. +/// +/// Their diffs are machine-written dependency churn: they run to thousands of +/// lines, flood the prompt with line numbers and drown the code the model should +/// look at. A lockfile is also never a place where a review comment belongs. +const IGNORED_FILE_NAMES: [&str; 16] = [ + "Cargo.lock", + "package-lock.json", + "npm-shrinkwrap.json", + "yarn.lock", + "pnpm-lock.yaml", + "bun.lock", + "bun.lockb", + "composer.lock", + "Gemfile.lock", + "poetry.lock", + "uv.lock", + "Pipfile.lock", + "go.sum", + "packages.lock.json", + "flake.lock", + "pubspec.lock", +]; + +/// Whether a changed path is a generated file that is never reviewed. +/// +/// The comparison is on the basename, so a lockfile nested in a workspace (for +/// example `crates/foo/Cargo.lock`) is matched too. +pub(super) fn is_ignored(filename: &str) -> bool { + let basename = filename.rsplit('/').next().unwrap_or(filename); + + IGNORED_FILE_NAMES.contains(&basename) +} + +/// Drops the generated files from the changed lines. +/// +/// Removing them here keeps the prompt free of their line numbers, and also makes +/// [`resolve_review_sides`] reject any review the model anchors on one of them. +pub(super) fn drop_generated_files(changed_lines: &mut ChangedLines) { + changed_lines.retain(|file| !is_ignored(&file.filename)); +} + +/// Borrows the entry of `files` for a file, creating it on its first change. +fn changed_file<'a>(files: &'a mut ChangedLines, filename: &str) -> &'a mut ChangedFile { + let index = match files.iter().position(|file| file.filename == filename) { + Some(index) => index, + None => { + files.push(ChangedFile::new(filename)); + files.len() - 1 + } + }; + + &mut files[index] +} + +/// Extracts the old and new starting line numbers of a hunk header such as +/// `@@ -12,3 +12,5 @@`. The counts are optional and git may append a section +/// heading after the closing `@@`. +fn parse_hunk_starts(hunk_header: &str) -> Option<(u64, u64)> { + let body = hunk_header.strip_prefix("@@ ")?; + let body = body.split(" @@").next()?; + + let (old, new) = body.split_once(" +")?; + let old = old.strip_prefix('-')?.split(',').next()?; + let new = new.split(',').next()?; + + Some((old.parse().ok()?, new.parse().ok()?)) +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::bot_actions::review::{fixtures::*, prompt::format_changed_lines}; + + #[test] + fn a_deletion_only_pull_request_lists_removed_lines() { + let expected = "src/old.rs: removed 1, 2, 3"; + + assert_eq!( + format_changed_lines(&parse_changed_lines(DELETION_ONLY)), + expected + ); + } + + #[test] + fn hunk_content_starting_with_plus_or_minus_is_counted() { + let expected = "src/tricky.js: added 2 / removed 2"; + + assert_eq!( + format_changed_lines(&parse_changed_lines(TRICKY_CONTENT)), + expected + ); + } + + #[test] + fn a_renamed_file_uses_its_new_path() { + let expected = "src/new.rs: added 1 / removed 1"; + + assert_eq!( + format_changed_lines(&parse_changed_lines(RENAMED)), + expected + ); + } + + #[test] + fn a_path_with_a_space_is_read_from_the_headers() { + let expected = "src/my file.rs: added 2"; + + assert_eq!( + format_changed_lines(&parse_changed_lines(PATH_WITH_SPACE)), + expected + ); + } + + #[test] + fn a_quoted_path_is_decoded() { + let expected = "docs/café.md: added 2"; + + assert_eq!( + format_changed_lines(&parse_changed_lines(QUOTED_PATH)), + expected + ); + } + + #[test] + fn filenames_are_resolved_against_the_api_list() { + let mut changed_lines = parse_changed_lines(DIFF); + let files = vec![ + pull_request_file("src/bar.rs", None, "modified"), + pull_request_file("src/foo.rs", Some("src/renamed.rs"), "renamed"), + ]; + + resolve_filenames(&mut changed_lines, &files); + + assert_eq!(changed_lines[0].filename, "src/foo.rs"); + assert_eq!(changed_lines[1].filename, "src/bar.rs"); + } + + #[test] + fn a_file_absent_from_the_api_list_keeps_the_diff_path() { + let mut changed_lines = parse_changed_lines(DIFF); + let files = vec![pull_request_file("src/bar.rs", None, "modified")]; + + resolve_filenames(&mut changed_lines, &files); + + assert_eq!(changed_lines[0].filename, "src/foo.rs"); + assert_eq!(changed_lines[1].filename, "src/bar.rs"); + } + + #[test] + fn generated_lockfiles_are_ignored() { + assert!(is_ignored("Cargo.lock")); + assert!(is_ignored("crates/steep-server/Cargo.lock")); + assert!(is_ignored("package-lock.json")); + assert!(is_ignored("web/yarn.lock")); + assert!(is_ignored("go.sum")); + + assert!(!is_ignored("src/main.rs")); + assert!(!is_ignored("Cargo.toml")); + assert!(!is_ignored("docs/lockfiles.md")); + } + + #[test] + fn generated_files_are_dropped_from_the_changed_lines() { + const DIFF_WITH_LOCKFILE: &str = concat!( + "diff --git a/Cargo.lock b/Cargo.lock\n", + "--- a/Cargo.lock\n", + "+++ b/Cargo.lock\n", + "@@ -1,1 +1,2 @@\n", + " [[package]]\n", + "+name = \"x\"\n", + "diff --git a/src/foo.rs b/src/foo.rs\n", + "--- a/src/foo.rs\n", + "+++ b/src/foo.rs\n", + "@@ -1,1 +1,2 @@\n", + " fn a() {}\n", + "+fn b() {}\n", + ); + + let mut changed_lines = parse_changed_lines(DIFF_WITH_LOCKFILE); + drop_generated_files(&mut changed_lines); + + assert_eq!(format_changed_lines(&changed_lines), "src/foo.rs: added 2"); + } +} diff --git a/crates/steep-server/src/bot_actions/review/fixtures.rs b/crates/steep-server/src/bot_actions/review/fixtures.rs new file mode 100644 index 0000000..a90a29d --- /dev/null +++ b/crates/steep-server/src/bot_actions/review/fixtures.rs @@ -0,0 +1,119 @@ +//! Shared fixtures of the review tests. + +use crate::{ + domain::review::{ReviewItem, ReviewResult, ReviewSide}, + gitea::PullRequestFile, +}; + +/// Additions, a removal and a line changed on both sides. +pub(super) const DIFF: &str = concat!( + "diff --git a/src/foo.rs b/src/foo.rs\n", + "--- a/src/foo.rs\n", + "+++ b/src/foo.rs\n", + "@@ -1,4 +1,6 @@\n", + " fn main() {\n", + "+ let x = 1;\n", + "- let removed = 0;\n", + " println!(\"hello\");\n", + "+ let y = 2;\n", + "+ let z = 3;\n", + " }\n", + "diff --git a/src/bar.rs b/src/bar.rs\n", + "--- a/src/bar.rs\n", + "+++ b/src/bar.rs\n", + "@@ -10,4 +10,6 @@\n", + " old context\n", + "+ let a = 10;\n", + " more context\n", + "+ let b = 20;\n", +); + +/// A pull request that only deletes a file. +pub(super) const DELETION_ONLY: &str = concat!( + "diff --git a/src/old.rs b/src/old.rs\n", + "deleted file mode 100644\n", + "--- a/src/old.rs\n", + "+++ /dev/null\n", + "@@ -1,3 +0,0 @@\n", + "-fn a() {}\n", + "-fn b() {}\n", + "-fn c() {}\n", +); + +/// A hunk whose content starts with `+++` / `---`, which must not be taken +/// for the file headers. +pub(super) const TRICKY_CONTENT: &str = concat!( + "diff --git a/src/tricky.js b/src/tricky.js\n", + "--- a/src/tricky.js\n", + "+++ b/src/tricky.js\n", + "@@ -1,4 +1,4 @@\n", + " let i = 0;\n", + "+++i;\n", + "---x;\n", + " console.log(i);\n", +); + +/// A pull request that renames a file. +pub(super) const RENAMED: &str = concat!( + "diff --git a/src/old.rs b/src/new.rs\n", + "similarity index 50%\n", + "rename from src/old.rs\n", + "rename to src/new.rs\n", + "--- a/src/old.rs\n", + "+++ b/src/new.rs\n", + "@@ -1,1 +1,1 @@\n", + "-fn old() {}\n", + "+fn new() {}\n", +); + +/// A file whose path contains a space, which the `diff --git` line cannot +/// express without ambiguity. +pub(super) const PATH_WITH_SPACE: &str = concat!( + "diff --git a/src/my file.rs b/src/my file.rs\n", + "--- a/src/my file.rs\n", + "+++ b/src/my file.rs\n", + "@@ -1,1 +1,2 @@\n", + " fn a() {}\n", + "+fn b() {}\n", +); + +/// A file whose path git quotes and escapes (`caf\\303\\251.md` is +/// `caf\u{e9}.md`). +pub(super) const QUOTED_PATH: &str = concat!( + "diff --git \"a/docs/caf\\303\\251.md\" \"b/docs/caf\\303\\251.md\"\n", + "--- \"a/docs/caf\\303\\251.md\"\n", + "+++ \"b/docs/caf\\303\\251.md\"\n", + "@@ -1,1 +1,2 @@\n", + " intro\n", + "+ajout\n", +); + +pub(super) fn review(filename: &str, line: Option, side: Option) -> ReviewItem { + ReviewItem { + filename: String::from(filename), + line, + side, + severity: None, + message: String::from("message"), + } +} + +pub(super) fn review_result(reviews: Vec) -> ReviewResult { + ReviewResult { + reviews, + comment: String::new(), + cost: None, + } +} + +pub(super) fn pull_request_file( + filename: &str, + previous_filename: Option<&str>, + status: &str, +) -> PullRequestFile { + PullRequestFile { + filename: String::from(filename), + previous_filename: previous_filename.map(String::from), + status: String::from(status), + } +} diff --git a/crates/steep-server/src/bot_actions/review/mod.rs b/crates/steep-server/src/bot_actions/review/mod.rs new file mode 100644 index 0000000..46c6c8e --- /dev/null +++ b/crates/steep-server/src/bot_actions/review/mod.rs @@ -0,0 +1,210 @@ +//! The pull request review action. +//! +//! [`exec_review`] drives one review end to end. The work is split by concern: +//! - [`diff`] parses the pull request diff into the lines it changes, +//! - [`prompt`] renders those changes for the model, +//! - [`anchor`] checks that the model's reviews land on changed lines, +//! - [`render`] builds the markdown summary posted on the pull request. + +mod anchor; +mod diff; +mod prompt; +mod render; + +#[cfg(test)] +mod fixtures; + +use tracing::{info, instrument, warn}; + +use crate::{ + consts::{BOT_PROCESS_MSG, REVIEW_PROMPT, SANDBOX_SYSTEM_PROMPT}, + domain::review::ReviewResult, + event_bus::Reporter, + gitea::{GiteaAPI, ReviewPayload}, + metrics, + open_router::{OpenRouterClient, Tool}, + sandbox::{Sandbox, SandboxConfig, agent}, +}; + +use self::{ + anchor::resolve_review_sides, + diff::{drop_generated_files, parse_changed_lines, resolve_filenames}, + prompt::format_changes, + render::review_result_to_markdown, +}; + +/// The services a review needs, borrowed from the bot for the duration of one job. +pub struct ReviewContext<'a> { + pub gitea: &'a GiteaAPI, + pub open_router: &'a OpenRouterClient, + pub sandbox: &'a SandboxConfig, + pub reporter: &'a Reporter, +} + +pub struct ReviewOutcome { + pub markdown: String, + pub cost: Option, + pub comments: u64, +} + +impl ReviewOutcome { + pub fn empty() -> Self { + Self { + markdown: String::new(), + cost: None, + comments: 0, + } + } +} + +#[instrument( + skip_all, + fields(repo = %payload.repository.full_name, pr = payload.pull_request.number) +)] +pub async fn exec_review( + ctx: &ReviewContext<'_>, + tools: Vec, + payload: &ReviewPayload, +) -> anyhow::Result { + info!(action = %payload.action, "Starting review"); + + let full_name = &payload.repository.full_name; + let index = payload.pull_request.number; + + let progress_comment = ctx + .gitea + .comment( + &BOT_PROCESS_MSG.replace("{model}", ctx.open_router.model()), + full_name, + index, + ) + .await?; + + let review = review_pull_request(ctx, tools, payload).await; + + // The progress comment is housekeeping: failing to update it must neither turn a + // posted review into a failure nor hide the error that made the review fail. + match review { + Ok((review_result, markdown)) => { + if let Err(err) = ctx + .gitea + .delete_comment(full_name, progress_comment.id) + .await + { + warn!(%err, "Failed to delete the progress comment"); + } + + Ok(ReviewOutcome { + markdown, + cost: review_result.cost, + comments: review_result.reviews.len() as u64, + }) + } + Err(err) => { + if let Err(edit_err) = ctx + .gitea + .edit_comment( + &format!("Error while reviewing: {err}"), + full_name, + progress_comment.id, + ) + .await + { + warn!(err = %edit_err, "Failed to report the error on the progress comment"); + } + + Err(err) + } + } +} + +/// Runs the review and posts it, returning the result and the markdown that was +/// posted. +async fn review_pull_request( + ctx: &ReviewContext<'_>, + tools: Vec, + payload: &ReviewPayload, +) -> anyhow::Result<(ReviewResult, String)> { + let full_name = &payload.repository.full_name; + let index = payload.pull_request.number; + + let git_diff = ctx.gitea.pull_request_diff(full_name, index).await?; + + // The file list only refines the paths and describes the changes: a failure is + // not fatal, the diff is the source of truth for the lines. + let files = match ctx.gitea.pull_request_files(full_name, index).await { + Ok(files) => files, + Err(err) => { + warn!(%err, "Failed to list the pull request files"); + Vec::new() + } + }; + + let mut changed_lines = parse_changed_lines(&git_diff); + drop_generated_files(&mut changed_lines); + resolve_filenames(&mut changed_lines, &files); + + let changes = format_changes(&files, &changed_lines); + + let bot_request = REVIEW_PROMPT + .replace("{subject}", &payload.pull_request.title) + .replace("{comment}", &payload.comment.body) + .replace("{changes}", &changes); + + let (mut review_result, cost) = run_sandboxed_review(ctx, tools, payload, &bot_request).await?; + + resolve_review_sides(&mut review_result, &changed_lines); + + review_result.cost = cost; + if let Some(cost) = review_result.cost { + metrics::openrouter_cost_usd(cost); + } + + let markdown = review_result_to_markdown(&review_result); + + ctx.gitea + .post_pull_request_review(&review_result, &markdown, full_name, index) + .await?; + + Ok((review_result, markdown)) +} + +async fn run_sandboxed_review( + ctx: &ReviewContext<'_>, + tools: Vec, + payload: &ReviewPayload, + bot_request: &str, +) -> anyhow::Result<(ReviewResult, Option)> { + let sandbox = Sandbox::create( + &ctx.sandbox.runtime, + &payload.repository.clone_url, + ctx.gitea.token(), + payload.pull_request.number, + ctx.reporter, + ) + .await?; + + let result = agent::run( + ctx.open_router, + &sandbox, + tools, + SANDBOX_SYSTEM_PROMPT, + bot_request, + ctx.sandbox.max_iterations, + ctx.reporter, + ) + .await; + + if let Err(err) = sandbox.cleanup().await { + warn!(%err, "Failed to clean up sandbox container"); + } + + let result = result?; + info!( + iterations = result.iterations, + cost = ?result.cost, + "Sandboxed review finished" + ); + + Ok((result.answer, result.cost)) +} diff --git a/crates/steep-server/src/bot_actions/review/prompt.rs b/crates/steep-server/src/bot_actions/review/prompt.rs new file mode 100644 index 0000000..c9b5e39 --- /dev/null +++ b/crates/steep-server/src/bot_actions/review/prompt.rs @@ -0,0 +1,125 @@ +//! Rendering of a pull request's changes for the model. + +use super::diff::{ChangedLines, is_ignored}; +use crate::gitea::PullRequestFile; + +/// Renders the changes for the model: the files the pull request touches, then +/// the lines to review per file. +pub(super) fn format_changes(files: &[PullRequestFile], changed_lines: &ChangedLines) -> String { + let mut sections = Vec::new(); + + let described = files + .iter() + .filter(|file| !is_ignored(&file.filename)) + .map(describe_file) + .collect::>() + .join(", "); + + if !described.is_empty() { + sections.push(format!("Files changed by the pull request: {described}")); + } + + sections.push(format!( + "Lines to review, per file:\n{}", + format_changed_lines(changed_lines) + )); + + sections.join("\n\n") +} + +/// Describes a changed file for the model, including how it changed. +fn describe_file(file: &PullRequestFile) -> String { + match &file.previous_filename { + Some(previous) => format!("{} ({} from {})", file.filename, file.status, previous), + None => format!("{} ({})", file.filename, file.status), + } +} + +/// Renders the changed lines as `filename: added 1, 2 / removed 3`, one file per +/// line, keeping only the sides the file actually has. +pub(super) fn format_changed_lines(changed_lines: &ChangedLines) -> String { + changed_lines + .iter() + .map(|file| { + let mut sides = Vec::new(); + + for (label, lines) in [("added", &file.added), ("removed", &file.removed)] { + if !lines.is_empty() { + sides.push(format!("{label} {}", format_line_numbers(lines))); + } + } + + format!("{}: {}", file.filename, sides.join(" / ")) + }) + .collect::>() + .join("\n") +} + +fn format_line_numbers(lines: &[u64]) -> String { + lines + .iter() + .map(u64::to_string) + .collect::>() + .join(", ") +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::bot_actions::review::{diff::parse_changed_lines, fixtures::*}; + + #[test] + fn changed_lines_are_listed_per_file_and_side() { + let expected = concat!( + "src/foo.rs: added 2, 4, 5 / removed 2\n", + "src/bar.rs: added 11, 13" + ); + + assert_eq!(format_changed_lines(&parse_changed_lines(DIFF)), expected); + } + + #[test] + fn changes_describe_the_files_then_the_lines() { + let changed_lines = parse_changed_lines(DELETION_ONLY); + let files = vec![pull_request_file("src/old.rs", None, "deleted")]; + + let expected = concat!( + "Files changed by the pull request: src/old.rs (deleted)\n", + "\n", + "Lines to review, per file:\n", + "src/old.rs: removed 1, 2, 3" + ); + + assert_eq!(format_changes(&files, &changed_lines), expected); + } + + #[test] + fn changes_without_the_api_list_only_hold_the_lines() { + let changed_lines = parse_changed_lines(DELETION_ONLY); + + let expected = "Lines to review, per file:\nsrc/old.rs: removed 1, 2, 3"; + + assert_eq!(format_changes(&[], &changed_lines), expected); + } + + #[test] + fn a_generated_file_is_not_described_to_the_model() { + let files = vec![ + pull_request_file("Cargo.lock", None, "modified"), + pull_request_file("src/foo.rs", None, "modified"), + ]; + let changed_lines = parse_changed_lines(DIFF); + + let changes = format_changes(&files, &changed_lines); + + assert!(!changes.contains("Cargo.lock")); + assert!(changes.contains("src/foo.rs (modified)")); + } + + #[test] + fn a_renamed_file_is_described_with_its_previous_path() { + let file = pull_request_file("src/new.rs", Some("src/old.rs"), "renamed"); + + assert_eq!(describe_file(&file), "src/new.rs (renamed from src/old.rs)"); + } +} diff --git a/crates/steep-server/src/bot_actions/review/render.rs b/crates/steep-server/src/bot_actions/review/render.rs new file mode 100644 index 0000000..ed66520 --- /dev/null +++ b/crates/steep-server/src/bot_actions/review/render.rs @@ -0,0 +1,110 @@ +//! Markdown summary of a review, as posted on the pull request. + +use crate::domain::review::{ReviewItem, ReviewResult, ReviewSeverity}; + +pub(super) fn review_result_to_markdown(review_result: &ReviewResult) -> String { + if review_result.reviews.is_empty() { + return String::from("No issues found. ✅"); + } + + let mut md = String::from("## Review Feedback\n\n"); + + md.push_str(&format!( + "### {} issues found.\n\n", + review_result.reviews.len() + )); + + let breakdown = severity_breakdown(&review_result.reviews); + if !breakdown.is_empty() { + md.push_str(&breakdown); + md.push('\n'); + } + + if !review_result.comment.is_empty() { + md.push_str("\n---\n\n"); + md.push_str("### Summary\n\n"); + md.push_str(&review_result.comment); + md.push('\n'); + } + + if let Some(cost) = review_result.cost { + md.push_str("\n---\n\n"); + md.push_str(&format!("### Cost: ${}", cost)); + md.push('\n'); + } + + md +} + +/// Counts the reviews per severity, most severe first. +/// +/// A review whose severity the model omitted or wrote unreadably is counted as +/// [`ReviewSeverity::Maintainability`], the same fallback used when posting it. +fn severity_breakdown(reviews: &[ReviewItem]) -> String { + [ + ReviewSeverity::Bug, + ReviewSeverity::Security, + ReviewSeverity::Performance, + ReviewSeverity::Maintainability, + ] + .into_iter() + .filter_map(|severity| { + let count = reviews + .iter() + .filter(|review| review.severity.unwrap_or(ReviewSeverity::Maintainability) == severity) + .count(); + + (count > 0).then(|| format!("- {} {}", count, severity.label())) + }) + .collect::>() + .join("\n") +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::bot_actions::review::fixtures::*; + + #[test] + fn the_breakdown_counts_severities_most_severe_first() { + let reviews = vec![ + ReviewItem { + severity: Some(ReviewSeverity::Maintainability), + ..review("a", Some(1), None) + }, + ReviewItem { + severity: Some(ReviewSeverity::Bug), + ..review("b", Some(2), None) + }, + ReviewItem { + severity: Some(ReviewSeverity::Bug), + ..review("c", Some(3), None) + }, + // A severity the model left out counts as maintainability. + review("d", Some(4), None), + ]; + + assert_eq!(severity_breakdown(&reviews), "- 2 bug\n- 2 maintainability"); + } + + #[test] + fn the_markdown_breaks_the_issues_down_by_severity() { + let mut result = review_result(vec![ + ReviewItem { + severity: Some(ReviewSeverity::Bug), + ..review("a", Some(1), None) + }, + ReviewItem { + severity: Some(ReviewSeverity::Performance), + ..review("b", Some(2), None) + }, + ]); + result.comment = String::from("Le découpage en crates est propre."); + + let markdown = review_result_to_markdown(&result); + + assert!(markdown.contains("- 1 bug")); + assert!(markdown.contains("- 1 performance")); + assert!(markdown.contains("Le découpage en crates est propre.")); + } +} diff --git a/crates/steep-server/src/db.rs b/crates/steep-server/src/db.rs index 2eb62e6..fd38ed2 100644 --- a/crates/steep-server/src/db.rs +++ b/crates/steep-server/src/db.rs @@ -1,7 +1,5 @@ use toasty::Db; -use crate::schemas::{Job, JobStatus}; - pub async fn open(url: &str) -> toasty::Result { Db::builder() .models(toasty::models!(crate::*)) @@ -9,35 +7,45 @@ pub async fn open(url: &str) -> toasty::Result { .await } -pub async fn connect(url: &str) -> toasty::Result { - let mut db = open(url).await?; - - Job::update_by_status(JobStatus::Processing) - .status(JobStatus::Pending) - .exec(&mut db) - .await?; - - Ok(db) -} - +/// A connection for a test, on the database `DATABASE_URL` points to. +/// +/// Every `#[tokio::test]` runs on its own runtime, and the connection tasks live on +/// the runtime that opened the pool: sharing one `Db` between tests would leave +/// them with a pool whose runtime is gone. Each test therefore opens its own, and +/// only the migrations are applied once. #[cfg(test)] pub(crate) async fn test_connect() -> Db { - static DB: tokio::sync::OnceCell = tokio::sync::OnceCell::const_new(); + static MIGRATED: tokio::sync::OnceCell<()> = tokio::sync::OnceCell::const_new(); - DB.get_or_init(|| async { - let url = std::env::var("DATABASE_URL").expect("DATABASE_URL must be set"); + let url = std::env::var("DATABASE_URL").expect("DATABASE_URL must be set"); - let migrations = std::path::Path::new(env!("CARGO_MANIFEST_DIR")).join("toasty"); - let config = toasty_cli::Config::new() - .migration(toasty_cli::MigrationConfig::new().path(migrations)); + // The migrations run on a runtime of their own, which outlives the first test. + MIGRATED + .get_or_init(|| async { + let url = url.clone(); - toasty_cli::ToastyCli::with_config(open(&url).await.expect("open pool"), config) - .parse_from(["toasty", "migration", "apply"]) - .await - .expect("apply migrations"); + std::thread::spawn(move || { + tokio::runtime::Runtime::new() + .expect("migration runtime") + .block_on(async { + let migrations = + std::path::Path::new(env!("CARGO_MANIFEST_DIR")).join("toasty"); + let config = toasty_cli::Config::new() + .migration(toasty_cli::MigrationConfig::new().path(migrations)); - connect(&url).await.expect("connect to postgres") - }) - .await - .clone() + toasty_cli::ToastyCli::with_config( + open(&url).await.expect("open pool"), + config, + ) + .parse_from(["toasty", "migration", "apply"]) + .await + .expect("apply migrations"); + }); + }) + .join() + .expect("migrations panicked"); + }) + .await; + + open(&url).await.expect("connect to postgres") } diff --git a/crates/steep-server/src/domain/mod.rs b/crates/steep-server/src/domain/mod.rs new file mode 100644 index 0000000..0755bd4 --- /dev/null +++ b/crates/steep-server/src/domain/mod.rs @@ -0,0 +1 @@ +pub mod review; diff --git a/crates/steep-server/src/domain/review.rs b/crates/steep-server/src/domain/review.rs new file mode 100644 index 0000000..1f8088c --- /dev/null +++ b/crates/steep-server/src/domain/review.rs @@ -0,0 +1,247 @@ +//! Domain types of a review: what the model answers with, and how it is read. + +use std::str::FromStr; + +use serde::{Deserialize, Deserializer}; +use tracing::warn; + +use crate::text::excerpt; + +#[derive(Deserialize, Debug)] +pub struct ReviewResult { + pub reviews: Vec, + pub comment: String, + pub cost: Option, +} + +#[derive(Deserialize, Debug)] +pub struct ReviewItem { + pub filename: String, + pub line: Option, + #[serde(default, deserialize_with = "deserialize_side")] + pub side: Option, + #[serde(default, deserialize_with = "deserialize_severity")] + pub severity: Option, + pub message: String, +} + +/// Which version of the file a review comment is anchored on. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum ReviewSide { + /// The line was added by the pull request: `line` is a line number of the + /// new version of the file. + Added, + /// The line was removed by the pull request: `line` is a line number of the + /// old version of the file. + Removed, +} + +impl ReviewSide { + /// Reads the side the model asked for, tolerating casing and synonyms. + fn parse(raw: &str) -> Option { + match raw.trim().to_ascii_lowercase().as_str() { + "added" | "add" | "new" | "right" => Some(Self::Added), + "removed" | "remove" | "deleted" | "delete" | "old" | "left" => Some(Self::Removed), + _ => None, + } + } +} + +/// What kind of problem a review reports. +/// +/// The categories are the ones the prompt asks for; a review whose severity is +/// missing or unreadable falls back to [`ReviewSeverity::Maintainability`], the +/// least alarming bucket, instead of failing the whole review. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum ReviewSeverity { + /// Wrong behaviour. + Bug, + /// A vulnerability. + Security, + /// A resource problem. + Performance, + /// Everything else: readability, structure, naming, tests. + Maintainability, +} + +impl ReviewSeverity { + /// Reads the severity the model asked for, tolerating casing and synonyms. + fn parse(raw: &str) -> Option { + match raw.trim().to_ascii_lowercase().as_str() { + "bug" | "bugs" | "correctness" | "error" => Some(Self::Bug), + "security" | "vulnerability" => Some(Self::Security), + "performance" | "perf" => Some(Self::Performance), + "maintainability" | "quality" | "style" => Some(Self::Maintainability), + _ => None, + } + } + + /// Lower-case label used in the review markdown. + pub fn label(self) -> &'static str { + match self { + Self::Bug => "bug", + Self::Security => "security", + Self::Performance => "performance", + Self::Maintainability => "maintainability", + } + } +} + +/// Reads the side the model asked for. An unreadable value is ignored instead of +/// failing the whole review: the side is then resolved from the changed lines. +fn deserialize_side<'de, D>(deserializer: D) -> Result, D::Error> +where + D: Deserializer<'de>, +{ + let raw = Option::::deserialize(deserializer)?; + + Ok(raw + .as_ref() + .and_then(serde_json::Value::as_str) + .and_then(ReviewSide::parse)) +} + +/// Reads the severity the model asked for, ignoring an unreadable value: the +/// review is then reported as [`ReviewSeverity::Maintainability`]. +fn deserialize_severity<'de, D>(deserializer: D) -> Result, D::Error> +where + D: Deserializer<'de>, +{ + let raw = Option::::deserialize(deserializer)?; + + Ok(raw + .as_ref() + .and_then(serde_json::Value::as_str) + .and_then(ReviewSeverity::parse)) +} + +/// 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]) +} + +#[cfg(test)] +mod tests { + use super::*; + + #[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 severities_are_read_from_the_model() { + let result: ReviewResult = serde_json::from_str( + r#"{ + "reviews": [ + { "filename": "a", "line": 1, "severity": "Bug", "message": "x" }, + { "filename": "b", "line": 2, "severity": "SECURITY", "message": "x" }, + { "filename": "c", "line": 3, "severity": "perf", "message": "x" }, + { "filename": "d", "line": 4, "severity": "banana", "message": "x" } + ], + "comment": "" + }"#, + ) + .unwrap(); + + let severities = result + .reviews + .iter() + .map(|review| review.severity) + .collect::>(); + + assert_eq!( + severities, + vec![ + Some(ReviewSeverity::Bug), + Some(ReviewSeverity::Security), + Some(ReviewSeverity::Performance), + None, + ] + ); + } +} diff --git a/crates/steep-server/src/env.rs b/crates/steep-server/src/env.rs index adf1973..13a1214 100644 --- a/crates/steep-server/src/env.rs +++ b/crates/steep-server/src/env.rs @@ -1,4 +1,9 @@ -use anyhow::anyhow; +use std::{str::FromStr, time::Duration}; + +use anyhow::Context; + +/// Number of tool-calling iterations a sandboxed review may take, unless configured. +const DEFAULT_SANDBOX_MAX_ITERATIONS: usize = 8; #[derive(Clone)] pub struct EnvConfig { @@ -6,46 +11,31 @@ pub struct EnvConfig { pub webhook_secret: String, pub open_router_api_key: String, pub open_router_model: String, - pub open_router_timeout: u64, + pub open_router_timeout: Duration, pub bot_max_concurrent: usize, pub gitea_url: String, pub gitea_token: String, - pub gitea_timeout: u64, + pub gitea_timeout: Duration, pub database_url: String, pub metrics_bind_addr: Option, pub sandbox_max_iterations: usize, } pub fn load_config() -> anyhow::Result { - let http_port = try_get_env("HTTP_PORT")?.parse()?; - let webhook_secret = try_get_env("WEBHOOK_SIG_HEADER_SECRET")?; - let open_router_api_key = try_get_env("OPEN_ROUTER_API_KEY")?; - let open_router_model = try_get_env("OPEN_ROUTER_MODEL")?; - let open_router_timeout = try_get_env("OPEN_ROUTER_TIMEOUT")?.parse()?; - let bot_max_concurrent = try_get_env("BOT_MAX_CONCURRENT")?.parse()?; - let gitea_url = try_get_env("GITEA_URL")?; - let gitea_token = try_get_env("GITEA_TOKEN")?; - let gitea_timeout = try_get_env("GITEA_TIMEOUT")?.parse()?; - let database_url = try_get_env("DATABASE_URL")?; - let metrics_bind_addr = std::env::var("METRICS_BIND_ADDR").ok(); - let sandbox_max_iterations = std::env::var("SANDBOX_MAX_ITERATIONS") - .ok() - .and_then(|value| value.parse().ok()) - .unwrap_or(8); - Ok(EnvConfig { - http_port, - webhook_secret, - open_router_api_key, - open_router_model, - open_router_timeout, - bot_max_concurrent, - gitea_url, - gitea_token, - gitea_timeout, - database_url, - metrics_bind_addr, - sandbox_max_iterations, + http_port: parse_env("HTTP_PORT")?, + webhook_secret: try_get_env("WEBHOOK_SIG_HEADER_SECRET")?, + open_router_api_key: try_get_env("OPEN_ROUTER_API_KEY")?, + open_router_model: try_get_env("OPEN_ROUTER_MODEL")?, + open_router_timeout: parse_seconds_env("OPEN_ROUTER_TIMEOUT")?, + bot_max_concurrent: parse_env("BOT_MAX_CONCURRENT")?, + gitea_url: try_get_env("GITEA_URL")?, + gitea_token: try_get_env("GITEA_TOKEN")?, + gitea_timeout: parse_seconds_env("GITEA_TIMEOUT")?, + database_url: try_get_env("DATABASE_URL")?, + metrics_bind_addr: std::env::var("METRICS_BIND_ADDR").ok(), + sandbox_max_iterations: parse_optional_env("SANDBOX_MAX_ITERATIONS")? + .unwrap_or(DEFAULT_SANDBOX_MAX_ITERATIONS), }) } @@ -53,16 +43,57 @@ pub fn try_get_env(key: &str) -> anyhow::Result { let env_value = std::env::var(key).map_err(|e| anyhow::anyhow!("{}: {}", key, e))?; if env_value.trim().is_empty() { - return Err(anyhow!(format!("env var {} is empty", key))); + anyhow::bail!("env var {key} is empty"); } Ok(env_value) } +/// Reads a required variable and parses it, naming the variable when it is invalid. +fn parse_env(key: &str) -> anyhow::Result +where + T: FromStr, + T::Err: std::error::Error + Send + Sync + 'static, +{ + parse_value(key, &try_get_env(key)?) +} + +/// Reads an optional variable and parses it. A missing or empty variable is `None`; +/// one that is set but invalid is an error, like it is for the required ones. +fn parse_optional_env(key: &str) -> anyhow::Result> +where + T: FromStr, + T::Err: std::error::Error + Send + Sync + 'static, +{ + match std::env::var(key) { + Ok(value) if !value.trim().is_empty() => parse_value(key, &value).map(Some), + _ => Ok(None), + } +} + +/// Reads a required variable holding a number of seconds. +fn parse_seconds_env(key: &str) -> anyhow::Result { + parse_env::(key).map(Duration::from_secs) +} + +fn parse_value(key: &str, value: &str) -> anyhow::Result +where + T: FromStr, + T::Err: std::error::Error + Send + Sync + 'static, +{ + value + .trim() + .parse() + .with_context(|| format!("env var {key} is invalid: `{value}`")) +} + #[cfg(test)] mod tests { use super::*; + // Every test uses its own variable: the environment is shared by the tests, + // which run in parallel. + #[test] fn test_try_get_env_returns_value() { unsafe { std::env::set_var("TEST_ENV_PRESENT", "hello") }; @@ -88,4 +119,38 @@ mod tests { let err = try_get_env("TEST_ENV_WHITESPACE").unwrap_err(); assert!(err.to_string().contains("TEST_ENV_WHITESPACE")); } + + #[test] + fn a_timeout_is_read_as_seconds() { + unsafe { std::env::set_var("TEST_ENV_SECONDS", "30") }; + assert_eq!( + parse_seconds_env("TEST_ENV_SECONDS").unwrap(), + Duration::from_secs(30) + ); + } + + #[test] + fn an_invalid_number_names_its_variable() { + unsafe { std::env::set_var("TEST_ENV_NOT_A_NUMBER", "soon") }; + + let err = parse_env::("TEST_ENV_NOT_A_NUMBER").unwrap_err(); + + assert!(format!("{err:#}").contains("TEST_ENV_NOT_A_NUMBER")); + assert!(format!("{err:#}").contains("soon")); + } + + #[test] + fn an_absent_optional_variable_is_none() { + unsafe { std::env::remove_var("TEST_ENV_OPTIONAL_ABSENT") }; + assert_eq!( + parse_optional_env::("TEST_ENV_OPTIONAL_ABSENT").unwrap(), + None + ); + } + + #[test] + fn an_invalid_optional_variable_is_an_error_instead_of_the_default() { + unsafe { std::env::set_var("TEST_ENV_OPTIONAL_INVALID", "many") }; + assert!(parse_optional_env::("TEST_ENV_OPTIONAL_INVALID").is_err()); + } } diff --git a/crates/steep-server/src/event_bus.rs b/crates/steep-server/src/event_bus.rs index c9c051e..0c03799 100644 --- a/crates/steep-server/src/event_bus.rs +++ b/crates/steep-server/src/event_bus.rs @@ -11,7 +11,7 @@ const PROGRESS_CAPACITY: usize = 256; const HISTORY_CAPACITY: usize = 256; const MAX_EVENT_ARGUMENTS: usize = 200; -/// Évènement de cycle de vie d'un job, diffusé globalement. +/// Lifecycle event of a job, broadcast globally. #[derive(Clone, Debug, Serialize)] #[serde(tag = "type", rename_all = "snake_case")] pub enum JobEvent { @@ -212,7 +212,7 @@ impl Reporter { }); } - /// Signale la fin d'un appel d'outil. + /// Reports the end of a tool call. pub fn tool_result(&self, iteration: usize, name: &str, truncated: bool, output: &str) { self.channel.emit(Progress::ToolResult { iteration, diff --git a/crates/steep-server/src/gitea.rs b/crates/steep-server/src/gitea.rs index d1616f3..071ce79 100644 --- a/crates/steep-server/src/gitea.rs +++ b/crates/steep-server/src/gitea.rs @@ -8,8 +8,8 @@ use tokio_util::io::StreamReader; use tracing::{instrument, warn}; use crate::{ - bot::{ReviewResult, ReviewSeverity, ReviewSide}, consts::MAX_DIFF_SIZE, + domain::review::{ReviewResult, ReviewSeverity, ReviewSide}, errors::AppError, }; @@ -27,7 +27,7 @@ pub struct GiteaAPI { } impl GiteaAPI { - pub fn new(base_url: &str, token: &str, timeout: u64) -> anyhow::Result { + pub fn new(base_url: &str, token: &str, timeout: Duration) -> anyhow::Result { let mut default_headers = reqwest::header::HeaderMap::new(); default_headers.insert( reqwest::header::HeaderName::from_static("authorization"), @@ -38,7 +38,7 @@ impl GiteaAPI { base_url: String::from(base_url), token: String::from(token), client: reqwest::Client::builder() - .timeout(Duration::from_secs(timeout)) + .timeout(timeout) .default_headers(default_headers) .build()?, }) @@ -320,6 +320,21 @@ pub struct ReviewPayload { pub comment: Comment, } +impl ReviewPayload { + /// Checks that the comment is a new one that mentions the bot. + fn check_addressed_to(&self, bot_name: &str) -> Result<(), AppError> { + if self.action != "created" { + return Err(AppError::InvalidActionErr); + } + + if !self.comment.body.starts_with(&format!("@{bot_name}")) { + return Err(AppError::UnauthorizedUserErr); + } + + Ok(()) + } +} + #[derive(Deserialize, Serialize, Debug, Clone)] pub struct PullRequest { pub number: u64, @@ -357,28 +372,15 @@ pub struct PullRequestFile { impl WebhookType { pub fn from_event(event: &str, bot_name: &str, json: Value) -> Result { - let wb = match event { - "pull_request_comment" => Ok(WebhookType::Review(serde_json::from_value(json)?)), + match event { + "pull_request_comment" => { + let payload: ReviewPayload = serde_json::from_value(json)?; + payload.check_addressed_to(bot_name)?; + + Ok(WebhookType::Review(payload)) + } _ => Err(AppError::UnknownEventErr), - }?; - - let action = match &wb { - WebhookType::Review(review_payload) => &review_payload.action, - }; - - if action != "created" { - return Err(AppError::InvalidActionErr); } - - let pr_body = match &wb { - WebhookType::Review(review_payload) => &review_payload.comment.body, - }; - - if !pr_body.starts_with(&format!("@{}", bot_name)) { - return Err(AppError::UnauthorizedUserErr); - } - - Ok(wb) } } diff --git a/crates/steep-server/src/job_queue.rs b/crates/steep-server/src/job_queue.rs index 1b65737..2c06958 100644 --- a/crates/steep-server/src/job_queue.rs +++ b/crates/steep-server/src/job_queue.rs @@ -1,26 +1,30 @@ -use std::{collections::VecDeque, time::Duration}; +use std::sync::Arc; -use futures_util::stream::{self, Stream}; -use toasty::Db; -use tracing::error; +use toasty::{Db, stmt::Value}; +use tokio::sync::Notify; use crate::{ gitea::WebhookType, schemas::{Job, JobStatus}, }; -/// Délai entre deux interrogations de la table quand la file est vide. -const POLL_INTERVAL: Duration = Duration::from_secs(1); - -/// File des reviews à faire. +/// The reviews to do, stored in PostgreSQL. +/// +/// Steep runs as a single instance, so every job is pushed by this process: the +/// worker is woken up by [`JobQueue::push`] instead of polling the table. The table +/// only makes the queue survive a restart. #[derive(Clone)] pub struct JobQueue { db: Db, + work: Arc, } impl JobQueue { pub fn new(db: Db) -> Self { - Self { db } + Self { + db, + work: Arc::new(Notify::new()), + } } pub async fn push(&self, webhook: WebhookType) -> toasty::Result { @@ -40,54 +44,76 @@ impl JobQueue { .exec(&mut db) .await?; + // `notify_one` keeps a permit when nobody is waiting yet, so a job pushed + // while the worker is busy is not missed. + self.work.notify_one(); + Ok(job) } - pub fn pending(&self) -> impl Stream { - let db = self.db.clone(); - - stream::unfold(VecDeque::new(), move |mut buffer| { - let db = db.clone(); - - async move { - let mut db = db; - - loop { - if let Some(job) = buffer.pop_front() { - return Some((job, buffer)); - } - - match Job::filter_by_status(JobStatus::Pending) - .exec(&mut db) - .await - { - Ok(jobs) if jobs.is_empty() => { - tokio::time::sleep(POLL_INTERVAL).await; - } - Ok(jobs) => buffer.extend(jobs), - Err(err) => { - error!(%err, "failed to poll pending jobs"); - tokio::time::sleep(POLL_INTERVAL).await; - } - } - } - } - }) - } - - /// Marque un job comme en cours de traitement. - pub async fn mark_processing(&self, id: u64) -> toasty::Result<()> { + /// Whether a job is already queued for this event. + pub async fn is_queued(&self, event_id: u64) -> toasty::Result { let mut db = self.db.clone(); - Job::update_by_id(id) - .status(JobStatus::Processing) + Ok(Job::filter_by_event_id(event_id) + .first() + .exec(&mut db) + .await? + .is_some()) + } + + /// Puts back in the queue the jobs a previous process left `processing`. + /// + /// To call once at startup, before the worker starts: it is only correct because + /// no other instance can be working on those jobs. + pub async fn requeue_interrupted(&self) -> toasty::Result<()> { + let mut db = self.db.clone(); + + Job::update_by_status(JobStatus::Processing) + .status(JobStatus::Pending) .exec(&mut db) .await?; Ok(()) } - /// Retire un job de la file, une fois la review enregistrée. + /// Takes the oldest pending job and marks it `processing`, or returns `None` when + /// the queue is empty. + /// + /// The selection and the update are a single statement, so a job cannot be taken + /// twice. + pub async fn claim_next(&self) -> toasty::Result> { + let mut db = self.db.clone(); + + let rows = toasty::sql::query( + "UPDATE jobs + SET status = 'processing' + WHERE id = ( + SELECT id FROM jobs + WHERE status = 'pending' + ORDER BY id + LIMIT 1 + ) + RETURNING id", + ) + .exec(&mut db) + .await?; + + let Some(Value::Record(row)) = rows.into_iter().next() else { + return Ok(None); + }; + + let id = u64::try_from(row.as_slice()[0].clone())?; + + Ok(Some(Job::get_by_id(&mut db, id).await?)) + } + + /// Waits until a job may have been pushed. + pub async fn wait_for_work(&self) { + self.work.notified().await; + } + + /// Removes a job from the queue, once its review is recorded. pub async fn remove(&self, id: u64) -> toasty::Result<()> { let mut db = self.db.clone(); @@ -108,11 +134,13 @@ impl JobQueue { mod tests { use std::time::Duration; - use futures_util::StreamExt; + use tokio::sync::{Mutex, MutexGuard}; use super::*; use crate::gitea::{Comment, PullRequest, Repository, ReviewPayload}; + const FIRST_EVENT: u64 = 999_000_200; + fn webhook(event_id: u64) -> WebhookType { WebhookType::Review(ReviewPayload { action: "created".to_string(), @@ -131,41 +159,40 @@ mod tests { }) } - async fn next_job(queue: &JobQueue, event_id: u64) -> Job { - let mut pending = std::pin::pin!(queue.pending()); + /// The tests claim whichever job is next, so they must not overlap: one would + /// claim the jobs of another. + async fn exclusive_queue() -> (MutexGuard<'static, ()>, JobQueue, Db) { + static LOCK: Mutex<()> = Mutex::const_new(()); - tokio::time::timeout(Duration::from_secs(10), async { - loop { - if let Some(job) = pending.next().await - && job.event_id == event_id - { - break job; - } - } - }) + let guard = LOCK.lock().await; + let db = crate::db::test_connect().await; + + toasty::sql::statement( + "DELETE FROM jobs WHERE event_id >= 999000200 AND event_id < 999000300", + ) + .exec(&mut db.clone()) .await - .expect("timed out waiting for the job") + .unwrap(); + + (guard, JobQueue::new(db.clone()), db) } #[tokio::test] #[ignore = "requires DATABASE_URL"] - async fn a_pushed_job_is_pending_then_removed() { - let db = crate::db::test_connect().await; - let queue = JobQueue::new(db.clone()); + async fn a_pushed_job_is_claimed_then_removed() { + let (_guard, queue, db) = exclusive_queue().await; - let event_id = 999_000_201; - Job::delete_by_event_id(&mut db.clone(), event_id) - .await - .ok(); + queue.push(webhook(FIRST_EVENT)).await.unwrap(); + assert!(queue.is_queued(FIRST_EVENT).await.unwrap()); - queue.push(webhook(event_id)).await.unwrap(); + let job = queue.claim_next().await.unwrap().expect("a job to claim"); + assert_eq!(job.event_id, FIRST_EVENT); + assert_eq!(job.status, JobStatus::Processing); - let job = next_job(&queue, event_id).await; - queue.mark_processing(job.id).await.unwrap(); queue.remove(job.id).await.unwrap(); assert!( - Job::filter_by_event_id(event_id) + Job::filter_by_event_id(FIRST_EVENT) .first() .exec(&mut db.clone()) .await @@ -177,19 +204,70 @@ mod tests { #[tokio::test] #[ignore = "requires DATABASE_URL"] async fn a_duplicate_event_id_is_rejected() { - let db = crate::db::test_connect().await; - let queue = JobQueue::new(db.clone()); + let (_guard, queue, _db) = exclusive_queue().await; - let event_id = 999_000_202; - Job::delete_by_event_id(&mut db.clone(), event_id) - .await - .ok(); + queue.push(webhook(FIRST_EVENT + 1)).await.unwrap(); + assert!(queue.push(webhook(FIRST_EVENT + 1)).await.is_err()); + } - queue.push(webhook(event_id)).await.unwrap(); - assert!(queue.push(webhook(event_id)).await.is_err()); + #[tokio::test] + #[ignore = "requires DATABASE_URL"] + async fn a_claimed_job_is_not_claimed_again() { + let (_guard, queue, _db) = exclusive_queue().await; - Job::delete_by_event_id(&mut db.clone(), event_id) + queue.push(webhook(FIRST_EVENT + 2)).await.unwrap(); + + assert!(queue.claim_next().await.unwrap().is_some()); + assert!(queue.claim_next().await.unwrap().is_none()); + } + + #[tokio::test] + #[ignore = "requires DATABASE_URL"] + async fn jobs_are_claimed_oldest_first() { + let (_guard, queue, _db) = exclusive_queue().await; + + queue.push(webhook(FIRST_EVENT + 3)).await.unwrap(); + queue.push(webhook(FIRST_EVENT + 4)).await.unwrap(); + + let first = queue.claim_next().await.unwrap().unwrap(); + let second = queue.claim_next().await.unwrap().unwrap(); + + assert_eq!(first.event_id, FIRST_EVENT + 3); + assert_eq!(second.event_id, FIRST_EVENT + 4); + } + + #[tokio::test] + #[ignore = "requires DATABASE_URL"] + async fn an_interrupted_job_is_requeued() { + let (_guard, queue, _db) = exclusive_queue().await; + + queue.push(webhook(FIRST_EVENT + 5)).await.unwrap(); + let job = queue.claim_next().await.unwrap().unwrap(); + assert!(queue.claim_next().await.unwrap().is_none()); + + // The process died here: on restart, the job goes back to the queue. + queue.requeue_interrupted().await.unwrap(); + + let again = queue.claim_next().await.unwrap().expect("the job back"); + assert_eq!(again.id, job.id); + } + + #[tokio::test] + #[ignore = "requires DATABASE_URL"] + async fn waiting_for_work_returns_as_soon_as_a_job_is_pushed() { + let (_guard, queue, _db) = exclusive_queue().await; + + let waiter = { + let queue = queue.clone(); + tokio::spawn(async move { queue.wait_for_work().await }) + }; + + tokio::time::sleep(Duration::from_millis(100)).await; + queue.push(webhook(FIRST_EVENT + 6)).await.unwrap(); + + tokio::time::timeout(Duration::from_secs(1), waiter) .await + .expect("the worker was not woken up") .unwrap(); } } diff --git a/crates/steep-server/src/main.rs b/crates/steep-server/src/main.rs index c639505..aa3c9aa 100644 --- a/crates/steep-server/src/main.rs +++ b/crates/steep-server/src/main.rs @@ -1,8 +1,14 @@ use std::time::Duration; use crate::{ - bot::Bot, dedup::Dedup, event_bus::EventBus, gitea::GiteaAPI, job_queue::JobQueue, - open_router::OpenRouterClient, sandbox::SandboxConfig, state::AppState, + bot::{Bot, BotDeps}, + event_bus::EventBus, + gitea::GiteaAPI, + job_queue::JobQueue, + open_router::OpenRouterClient, + review_store::ReviewStore, + sandbox::SandboxConfig, + state::AppState, }; use dotenvy::dotenv; @@ -16,7 +22,7 @@ mod bot; mod bot_actions; mod consts; mod db; -mod dedup; +mod domain; mod env; mod errors; mod event_bus; @@ -24,21 +30,29 @@ mod gitea; mod job_queue; mod metrics; mod open_router; +mod review_store; mod sandbox; mod schemas; mod state; mod text; -/// Délai laissé aux reviews en cours et aux serveurs pour s'arrêter proprement. +/// Time left to the running reviews and the servers to stop cleanly. /// -/// Sans lui, une review prise dans une sandbox récalcitrante garderait le processus -/// en vie jusqu'à ce que le superviseur le tue. +/// Without it, a review stuck in an unresponsive sandbox would keep the process +/// alive until the supervisor kills it. const SHUTDOWN_TIMEOUT: Duration = Duration::from_secs(60); fn main() -> anyhow::Result<()> { dotenv().ok(); - // Sous-commande de migration : `steep-server migration `. + // TLS goes through rustls, which has no default crypto provider here: the + // workspace compiles `ring` only, and it is installed once, for every client + // (reqwest, sentry). + rustls::crypto::ring::default_provider() + .install_default() + .map_err(|_| anyhow::anyhow!("a rustls crypto provider is already installed"))?; + + // Migration subcommand: `steep-server migration `. if std::env::args().nth(1).as_deref() == Some("migration") { return tokio::runtime::Runtime::new()?.block_on(run_migrations()); } @@ -46,7 +60,7 @@ fn main() -> anyhow::Result<()> { tracing_subscriber::registry() .with(fmt::layer()) .with( - EnvFilter::try_from_default_env() // lit RUST_LOG depuis l'env + EnvFilter::try_from_default_env() // reads RUST_LOG from the environment .unwrap_or_else(|_| EnvFilter::new("info")), ) .init(); @@ -124,25 +138,29 @@ async fn run() -> anyhow::Result<()> { ); } - let db = db::connect(&config.database_url).await?; - let dedup = Dedup::new(db.clone()); + let db = db::open(&config.database_url).await?; + let reviews = ReviewStore::new(db.clone()); + let queue = JobQueue::new(db); + queue.requeue_interrupted().await?; + let event_bus = EventBus::new(); let bot = Bot::new( - gitea_api, - open_router_client, + BotDeps { + gitea: gitea_api, + open_router: open_router_client, + sandbox, + reviews: reviews.clone(), + queue: queue.clone(), + event_bus: event_bus.clone(), + }, config.bot_max_concurrent, - config.open_router_model.clone(), - sandbox, - dedup.clone(), - queue.clone(), - event_bus.clone(), ); let app_state = AppState { queue, - dedup, + reviews, event_bus, bot_name: gitea_user.login.clone(), config, diff --git a/crates/steep-server/src/open_router.rs b/crates/steep-server/src/open_router.rs index 938c2fe..040b2ea 100644 --- a/crates/steep-server/src/open_router.rs +++ b/crates/steep-server/src/open_router.rs @@ -254,16 +254,19 @@ pub struct OpenRouterClient { } impl OpenRouterClient { - pub fn new(token: &str, model: &str, timeout: u64) -> anyhow::Result { + pub fn new(token: &str, model: &str, timeout: Duration) -> anyhow::Result { Ok(Self { - client: reqwest::Client::builder() - .timeout(Duration::from_secs(timeout)) - .build()?, + client: reqwest::Client::builder().timeout(timeout).build()?, api_key: String::from(token), model: String::from(model), }) } + /// The model every completion is requested from. + pub fn model(&self) -> &str { + &self.model + } + /// Sends a conversation with tool definitions and returns either a final /// message or the tool calls requested by the model. /// diff --git a/crates/steep-server/src/dedup.rs b/crates/steep-server/src/review_store.rs similarity index 64% rename from crates/steep-server/src/dedup.rs rename to crates/steep-server/src/review_store.rs index 51be893..d278be4 100644 --- a/crates/steep-server/src/dedup.rs +++ b/crates/steep-server/src/review_store.rs @@ -1,7 +1,8 @@ use toasty::Db; -use crate::schemas::{Job, Review}; +use crate::schemas::Review; +/// What is kept of a processed review. pub struct ReviewRecord { pub event_id: u64, pub repo: String, @@ -14,35 +15,19 @@ pub struct ReviewRecord { pub duration_ms: u64, } +/// The history of the reviews Steep has processed. +/// +/// It also is what keeps an event from being reviewed twice. #[derive(Clone)] -pub struct Dedup { +pub struct ReviewStore { db: Db, } -impl Dedup { +impl ReviewStore { pub fn new(db: Db) -> Self { Self { db } } - pub async fn is_known(&self, event_id: u64) -> toasty::Result { - let mut db = self.db.clone(); - - if Job::filter_by_event_id(event_id) - .first() - .exec(&mut db) - .await? - .is_some() - { - return Ok(true); - } - - Ok(Review::filter_by_event_id(event_id) - .first() - .exec(&mut db) - .await? - .is_some()) - } - pub async fn is_reviewed(&self, event_id: u64) -> toasty::Result { let mut db = self.db.clone(); @@ -53,7 +38,8 @@ impl Dedup { .is_some()) } - pub async fn mark_reviewed(&self, record: ReviewRecord) -> toasty::Result<()> { + /// Records a processed review, replacing the one of the same event if any. + pub async fn record(&self, record: ReviewRecord) -> toasty::Result<()> { let mut db = self.db.clone(); Review::upsert_by_event_id(record.event_id) @@ -71,7 +57,7 @@ impl Dedup { Ok(()) } - pub async fn list_reviews(&self) -> toasty::Result> { + pub async fn list(&self) -> toasty::Result> { let mut db = self.db.clone(); let reviews = Review::all().exec(&mut db).await?; @@ -82,24 +68,22 @@ impl Dedup { #[cfg(test)] mod tests { use super::*; - use crate::schemas::Review; #[tokio::test] #[ignore = "requires DATABASE_URL"] - async fn a_recorded_review_is_known_and_reviewed() { + async fn a_recorded_review_is_reviewed() { let db = crate::db::test_connect().await; - let dedup = Dedup::new(db.clone()); + let store = ReviewStore::new(db.clone()); let event_id = 999_000_101; Review::delete_by_event_id(&mut db.clone(), event_id) .await .ok(); - assert!(!dedup.is_known(event_id).await.unwrap()); - assert!(!dedup.is_reviewed(event_id).await.unwrap()); + assert!(!store.is_reviewed(event_id).await.unwrap()); - dedup - .mark_reviewed(ReviewRecord { + store + .record(ReviewRecord { event_id, repo: "owner/repo".to_string(), pr: 1, @@ -113,8 +97,15 @@ mod tests { .await .unwrap(); - assert!(dedup.is_reviewed(event_id).await.unwrap()); - assert!(dedup.is_known(event_id).await.unwrap()); + assert!(store.is_reviewed(event_id).await.unwrap()); + assert!( + store + .list() + .await + .unwrap() + .iter() + .any(|r| r.event_id == event_id) + ); Review::delete_by_event_id(&mut db.clone(), event_id) .await diff --git a/crates/steep-server/src/sandbox/agent.rs b/crates/steep-server/src/sandbox/agent.rs index a2c1768..4ca9643 100644 --- a/crates/steep-server/src/sandbox/agent.rs +++ b/crates/steep-server/src/sandbox/agent.rs @@ -95,8 +95,8 @@ where 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. + // The last turn is no longer an exploration turn: the model must give its + // answer, with what it has already seen. let last = iteration == max_iterations; if last { @@ -132,8 +132,8 @@ where "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. + // On the last turn, tools are refused: even if the provider returns them + // anyway, we do not run them and try the answer instead. if last || response.tool_calls.is_empty() { let answer = response.message.unwrap_or_default(); @@ -181,7 +181,7 @@ where 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. + // Line breaks would hurt the readability of a log line. output = %excerpt(&content, MAX_LOGGED_OUTPUT).replace('\n', " "), elapsed_ms = started.elapsed().as_millis() as u64, "Tool call finished" diff --git a/crates/steep-server/src/sandbox/instance.rs b/crates/steep-server/src/sandbox/instance.rs index 8478c58..cc31fdf 100644 --- a/crates/steep-server/src/sandbox/instance.rs +++ b/crates/steep-server/src/sandbox/instance.rs @@ -1,5 +1,5 @@ -//! Le [`Sandbox`] : clone d'une pull request exécuté dans un devcontainer -//! éphémère, et sa configuration. +//! The [`Sandbox`]: a pull request clone running inside an ephemeral +//! devcontainer, and its configuration. use std::{ path::{Path, PathBuf}, @@ -19,7 +19,7 @@ const DEVCONTAINER_PATHS: [&str; 2] = [".devcontainer/devcontainer.json", ".devc /// Sandbox-related runtime configuration. #[derive(Clone)] pub struct SandboxConfig { - /// Client du daemon de containers qui exécute la sandbox. + /// Client of the container daemon that runs the sandbox. pub runtime: ContainerRuntime, /// Maximum number of tool-calling iterations per agent run. pub max_iterations: usize, @@ -79,12 +79,12 @@ impl Sandbox { Ok(sandbox) } - /// Vérifie que le clone est bien visible dans le container. + /// Checks that the clone is actually visible inside the container. /// - /// Sans ce contrôle, un workspace vide — par exemple un daemon qui n'a pas pu - /// recevoir le clone — 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. + /// Without this check, an empty workspace (for example a daemon that could not + /// receive the clone) makes every tool fail; the model then chains failed calls + /// until the iteration budget is spent, without ever being able to review + /// anything. async fn check_workspace(&self) -> anyhow::Result<()> { let workspace_folder = self.workspace_folder(); let probe = self @@ -118,12 +118,12 @@ impl Sandbox { Ok(()) } - /// Rassemble de quoi expliquer un refus d'accès au workspace. + /// Gathers enough information to explain a refused access to the 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. + /// An `EACCES` has two possible causes, indistinguishable in the `ls` message: + /// the clone's permissions, or kernel confinement blocking the access. The + /// identity of the exec user and the permissions of the mount point tell them + /// apart. async fn diagnose_workspace(&self) -> String { let mut details = Vec::new(); @@ -215,12 +215,12 @@ async fn clone_pull_request( Ok(()) } -/// Rend le clone lisible par les utilisateurs du container sandbox. +/// Makes the clone readable by the users of the sandbox container. /// -/// Le container peut ne pas avoir les mêmes uid que Steep (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 Steep vient de cloner lui-même. +/// The container may not share Steep's uids (rootless podman maps uids through +/// subuid ranges), and the operator's umask may be restrictive: without this, the +/// sandbox tools fail with `Permission denied` on files that Steep itself just +/// cloned. async fn make_readable(repo_dir: &Path) -> anyhow::Result<()> { let output = tokio::process::Command::new("chmod") .args(["-R", "a+rX"]) diff --git a/crates/steep-server/src/sandbox/tools.rs b/crates/steep-server/src/sandbox/tools.rs index 3d4f5a0..7a9d249 100644 --- a/crates/steep-server/src/sandbox/tools.rs +++ b/crates/steep-server/src/sandbox/tools.rs @@ -167,11 +167,11 @@ async fn read_file(sandbox: &Sandbox, args: &Value) -> anyhow::Result { Ok(number_lines(&into_stdout(output)?, first_line)) } -/// Préfixe chaque ligne par son numéro. +/// Prefixes each line with its number. /// -/// Le modèle doit citer une ligne précise pour ancrer son commentaire : sans -/// numéros, il les compte lui-même et se décale de quelques lignes, ce qui place le -/// commentaire à côté du code visé. +/// The model must cite a precise line to anchor its comment: without numbers, it +/// counts them itself and drifts by a few lines, which puts the comment next to +/// the targeted code. fn number_lines(content: &str, first_line: u64) -> String { content .lines() @@ -291,8 +291,8 @@ mod tests { #[test] fn a_range_keeps_the_absolute_line_numbers() { - // Un extrait lu à partir de la ligne 12 doit garder la numérotation du - // fichier : sinon le modèle citerait des lignes décalées. + // An excerpt read from line 12 must keep the file's numbering: otherwise the + // model would cite shifted lines. assert_eq!(number_lines("x\ny", 12), "12:x\n13:y"); } diff --git a/crates/steep-server/src/schemas/review.rs b/crates/steep-server/src/schemas/review.rs index 0715a07..0041d9d 100644 --- a/crates/steep-server/src/schemas/review.rs +++ b/crates/steep-server/src/schemas/review.rs @@ -13,22 +13,22 @@ pub struct Review { pub repo: String, pub pr: u64, - /// Vrai si la review a abouti. + /// True if the review succeeded. pub ok: bool, - /// Modèle utilisé. + /// Model used. pub model: String, - /// Markdown posté sur la pull request (vide en cas d'échec). + /// Markdown posted on the pull request (empty on failure). pub markdown: String, - /// Coût OpenRouter en USD, si connu. + /// OpenRouter cost in USD, if known. pub cost: Option, - /// Nombre de commentaires produits. + /// Number of comments produced. pub comments: u64, - /// Durée du traitement en millisecondes. + /// Processing duration in milliseconds. pub duration_ms: u64, #[default(jiff::Timestamp::now())] diff --git a/crates/steep-server/src/state.rs b/crates/steep-server/src/state.rs index 263ffaf..d1e0d46 100644 --- a/crates/steep-server/src/state.rs +++ b/crates/steep-server/src/state.rs @@ -1,14 +1,14 @@ use tokio_util::sync::CancellationToken; -use crate::{dedup::Dedup, env::EnvConfig, event_bus::EventBus, job_queue::JobQueue}; +use crate::{env::EnvConfig, event_bus::EventBus, job_queue::JobQueue, review_store::ReviewStore}; #[derive(Clone)] pub struct AppState { pub queue: JobQueue, - pub dedup: Dedup, + pub reviews: ReviewStore, pub event_bus: EventBus, pub bot_name: String, pub config: EnvConfig, - /// Jeton annulé au démarrage de l'arrêt gracieux : il sert à clore les flux SSE. + /// Cancelled when the graceful shutdown starts: it closes the SSE streams. pub shutdown: CancellationToken, } diff --git a/deny.toml b/deny.toml new file mode 100644 index 0000000..124dc17 --- /dev/null +++ b/deny.toml @@ -0,0 +1,53 @@ +# cargo-deny configuration: https://embarkstudios.github.io/cargo-deny/ +# Run locally with `cargo deny check`. + +[graph] +all-features = true + +[advisories] +# A vulnerable, unmaintained or yanked dependency fails the build. +version = 2 +yanked = "deny" +ignore = [ + # Unmaintained, but only a thin wrapper around `rustls-pki-types`, and pulled by + # `toasty-driver-postgresql`: it can only go away with a toasty release. + { id = "RUSTSEC-2025-0134", reason = "transitive dependency of toasty, no vulnerability" }, +] + +[licenses] +version = 2 +# The workspace crates are not published, and have no license of their own. +private = { ignore = true } +allow = [ + "Apache-2.0", + "Apache-2.0 WITH LLVM-exception", + "BSD-2-Clause", + "BSD-3-Clause", + "BSL-1.0", + "CC0-1.0", + "CDLA-Permissive-2.0", + "ISC", + "MIT", + "MIT-0", + "MPL-2.0", + "OpenSSL", + "Unicode-3.0", + "Zlib", +] + +[bans] +multiple-versions = "warn" +wildcards = "deny" +# `path` dependencies between the (unpublished) workspace crates carry no version. +allow-wildcard-paths = true +# TLS goes through rustls: linking libssl would be a regression of the image +# (it would need `libssl` installed again). +deny = [ + { crate = "openssl-sys", reason = "TLS is rustls only, so that the image needs no libssl" }, + { crate = "native-tls", reason = "TLS is rustls only, so that the image needs no libssl" }, +] + +[sources] +unknown-registry = "deny" +unknown-git = "deny" +allow-registry = ["https://github.com/rust-lang/crates.io-index"] diff --git a/front/js/api.js b/front/js/api.js index 2f6ac45..144f590 100644 --- a/front/js/api.js +++ b/front/js/api.js @@ -3,7 +3,7 @@ export const endpoints = { jobs: "/jobs", - reviews: "/dedup", + reviews: "/reviews", lifecycle: "/events", jobEvents: (id) => `/jobs/${id}/events`, }; diff --git a/front/js/dashboard.js b/front/js/dashboard.js index 5829184..80309b3 100644 --- a/front/js/dashboard.js +++ b/front/js/dashboard.js @@ -1,6 +1,6 @@ // : assemble le tableau de bord. // -// Au montage, il charge l'état initial via REST (`/jobs`, `/dedup`), puis +// Au montage, il charge l'état initial via REST (`/jobs`, `/reviews`), puis // s'abonne au flux global `/events` pour tenir les listes à jour en direct. // La sélection d'un job ouvre son flux fin via .