1.2: Sandboxing #7
@@ -1,4 +1,4 @@
|
||||
FROM debian:trixie
|
||||
FROM rust:1.98-trixie
|
||||
|
qpismont marked this conversation as resolved
|
||||
|
||||
ARG USERNAME=dev
|
||||
ARG USER_UID=1000
|
||||
@@ -18,11 +18,11 @@ RUN apt-get update && apt-get install -y --no-install-recommends \
|
||||
&& rm -rf /var/lib/apt/lists/*
|
||||
|
||||
RUN groupadd --gid ${USER_GID:-1000} $USERNAME \
|
||||
&& useradd --uid ${USER_UID:-1000} --gid ${USER_GID:-1000} -m $USERNAME
|
||||
&& useradd --uid ${USER_UID:-1000} --gid ${USER_GID:-1000} -m $USERNAME \
|
||||
&& rustup component add clippy \
|
||||
&& rustup component add rustfmt
|
||||
|
||||
|
||||
|
||||
USER $USERNAME
|
||||
WORKDIR /home/$USERNAME
|
||||
|
||||
ENV PATH="/home/${USERNAME}/.cargo/bin:${PATH}"
|
||||
|
||||
RUN curl --proto '=https' --tlsv1.2 -sSf https://sh.rustup.rs | sh -s -- -y --default-toolchain stable
|
||||
|
||||
@@ -19,5 +19,6 @@
|
||||
},
|
||||
"workspaceMount": "source=${localWorkspaceFolder},target=/workspaces/herald,type=bind",
|
||||
"workspaceFolder": "/workspaces/herald",
|
||||
"runArgs": ["--userns=keep-id", "--security-opt", "label=disable"],
|
||||
|
qpismont marked this conversation as resolved
Herald
commented
L'option L'option `--security-opt label=disable` désactive la sécurité SELinux, ce qui réduit l'isolation du conteneur. Pour un environnement de développement, cela peut être acceptable, mais il faut être conscient des implications de sécurité. Envisagez d'utiliser `--security-opt label=type:container_runtime_t` ou d'autres options moins permissives si possible.
Herald
commented
Indentation par tabulation incohérente avec le reste du fichier ; Indentation par tabulation incohérente avec le reste du fichier ; `--security-opt label=disable` désactive l'étiquetage SELinux et `--userns=keep-id` est spécifique à Podman. À nettoyer/justifier, d'autant que ce devcontainer sert aussi de contexte d'exécution à la sandbox.
|
||||
"appPort": [3000]
|
||||
}
|
||||
|
||||
@@ -1,4 +0,0 @@
|
||||
target/
|
||||
|
Herald
commented
La suppression de La suppression de `.dockerignore` fait que le contexte envoyé lors de `buildah bud` contient désormais notamment `.git`, les répertoires de build et le fichier `.env` local. Même si le Containerfile ne le copie pas explicitement, ce contexte est transmis au moteur de build et peut contenir des secrets ou devenir inutilement volumineux. Il faut conserver un `.dockerignore` excluant au minimum `.git`, `.env*`, `target` et les fichiers locaux.
|
||||
.env
|
||||
.devcontainer/
|
||||
docs/
|
||||
@@ -1,5 +1,4 @@
|
||||
HTTP_PORT=3000
|
||||
BOT_NAME=Herald
|
||||
|
||||
WEBHOOK_SIG_HEADER_SECRET=
|
||||
|
||||
@@ -18,5 +17,8 @@ SENTRY_DSN=
|
||||
RUST_LOG=info
|
||||
RUST_BACKTRACE=1
|
||||
|
||||
|
||||
METRICS_BIND_ADDR=
|
||||
|
qpismont marked this conversation as resolved
Herald
commented
Les nouvelles variables doivent être documentées ici avec un commentaire indiquant leur rôle et une valeur d'exemple non sensible. Vérifier qu'aucune vraie clé/token n'a été committée et que le fichier reste aligné avec les variables réellement lues dans Les nouvelles variables doivent être documentées ici avec un commentaire indiquant leur rôle et une valeur d'exemple non sensible. Vérifier qu'aucune vraie clé/token n'a été committée et que le fichier reste aligné avec les variables réellement lues dans `env.rs`.
|
||||
|
||||
# Sandboxed tool execution
|
||||
# DOCKER_HOST=
|
||||
SANDBOX_MAX_ITERATIONS=8
|
||||
|
||||
@@ -1,3 +0,0 @@
|
||||
{
|
||||
"rust-analyzer.check.command": "clippy"
|
||||
}
|
||||
@@ -3,13 +3,27 @@ when:
|
||||
- push
|
||||
|
||||
steps:
|
||||
- name: fmt
|
||||
|
qpismont marked this conversation as resolved
Herald
commented
Pipeline CI : vérifier que Pipeline CI : vérifier que `cargo fmt --check`, `cargo clippy -- -D warnings` et `cargo test --all-features` couvrent bien le nouveau workspace multi-crates (utiliser `--workspace`), et que les tests du sandbox qui nécessitent Docker/root sont soit exécutés, soit explicitement ignorés avec justification.
|
||||
image: rust:1.98
|
||||
commands:
|
||||
- rustup component add rustfmt
|
||||
- cargo fmt --all -- --check
|
||||
|
||||
- name: clippy
|
||||
image: rust:1.96
|
||||
image: rust:1.98
|
||||
commands:
|
||||
- rustup component add clippy
|
||||
- cargo clippy
|
||||
- cargo clippy --workspace --all-targets --all-features -- -D warnings
|
||||
|
||||
- name: test
|
||||
image: rust:1.96
|
||||
image: rust:1.98
|
||||
commands:
|
||||
- cargo test
|
||||
- cargo test --workspace --all-targets
|
||||
|
||||
- name: container-build
|
||||
|
qpismont marked this conversation as resolved
Herald
commented
Le job Le job `container-build` compile l'image mais ne l'exécute jamais : il ne détectera donc pas l'absence de `git`/runtime conteneur dans l'image finale. Ajouter un smoke test (lancer l'image et vérifier la présence de `git` et `docker`/`podman`), et noter que le pipeline ne se déclenche que sur `push`, pas sur les pull requests.
|
||||
image: quay.io/buildah/stable
|
||||
privileged: true
|
||||
volumes:
|
||||
- /data/woodpecker-builds:/data
|
||||
commands:
|
||||
- buildah bud -f Containerfile -t herald-ci .
|
||||
|
||||
@@ -0,0 +1,22 @@
|
||||
{
|
||||
|
qpismont marked this conversation as resolved
Herald
commented
Ajouter la configuration Zed tout en supprimant Ajouter la configuration Zed tout en supprimant `.vscode/settings.json` crée une incohérence pour les contributeurs : il vaut mieux conserver les deux (ou documenter l'éditeur recommandé) et s'assurer qu'aucun chemin absolu local ni préférence personnelle n'est committé ici.
|
||||
"languages": {
|
||||
"Rust": {
|
||||
"format_on_save": "on",
|
||||
"formatter": "language_server"
|
||||
}
|
||||
},
|
||||
"lsp": {
|
||||
"rust-analyzer": {
|
||||
"initialization_options": {
|
||||
"check": {
|
||||
"command": "clippy",
|
||||
"extraArgs": [
|
||||
"--",
|
||||
"-D",
|
||||
"warnings"
|
||||
]
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
@@ -1,32 +1,33 @@
|
||||
[package]
|
||||
name = "herald"
|
||||
version = "1.1.0"
|
||||
edition = "2024"
|
||||
[workspace]
|
||||
|
qpismont marked this conversation as resolved
Herald
commented
Passage en workspace multi-crates : vérifier que les dépendances communes sont déclarées dans Passage en workspace multi-crates : vérifier que les dépendances communes sont déclarées dans `[workspace.dependencies]` et réutilisées avec `workspace = true` dans les crates, pour éviter les divergences de versions. Ajouter aussi les métadonnées de package communes (edition, license, version) via `[workspace.package]`.
|
||||
members = [
|
||||
"crates/herald-server",
|
||||
"crates/devcontainer-rs",
|
||||
]
|
||||
resolver = "3"
|
||||
|
||||
[profile.release]
|
||||
debug = 1
|
||||
|
||||
[dependencies]
|
||||
reqwest = { version = "0.12", default-features = false, features = ["json", "rustls-tls"] }
|
||||
[workspace.dependencies]
|
||||
reqwest = { version = "0.13", default-features = false, features = ["json", "native-tls", "stream"] }
|
||||
tokio = { version = "1.53", features = ["full"] }
|
||||
tokio-stream = "0.1"
|
||||
tokio-util = "0.7"
|
||||
futures-util = "0.3"
|
||||
serde_json = "1.0"
|
||||
serde = { version = "1.0", features = ["derive"] }
|
||||
sentry = { version = "0.48", features = ["tower-axum-matched-path"] }
|
||||
sentry-anyhow = { version = "0.48", features = ["backtrace"] }
|
||||
openrouter-rs = "0.12"
|
||||
sentry = { version = "0.49", features = ["tower-axum-matched-path"] }
|
||||
sentry-anyhow = { version = "0.49", features = ["backtrace"] }
|
||||
dotenvy = "0.15"
|
||||
tower = "0.5"
|
||||
tower-http = {version = "0.6", features = ["trace"] }
|
||||
tower-http = { version = "0.7", features = ["trace"] }
|
||||
tracing = "0.1"
|
||||
tracing-subscriber = { version = "0.3", features=["env-filter"] }
|
||||
tracing-subscriber = { version = "0.3", features = ["env-filter"] }
|
||||
axum = "0.8"
|
||||
anyhow = { version = "1.0", features = ["backtrace"] }
|
||||
anyhow = { version = "1", features = ["backtrace"] }
|
||||
thiserror = "2.0"
|
||||
ring = "0.17"
|
||||
hex = "0.4"
|
||||
bytes = "1.1"
|
||||
metrics = "0.24"
|
||||
metrics-exporter-prometheus = { version = "0.18", default-features = false, features = ["http-listener"] }
|
||||
|
||||
[profile.release]
|
||||
debug = 1
|
||||
|
||||
@@ -1,12 +1,31 @@
|
||||
FROM rust:1.96 as builder
|
||||
FROM rust:1.98-trixie as builder
|
||||
|
||||
WORKDIR /app
|
||||
COPY . .
|
||||
RUN cargo build --release
|
||||
|
||||
COPY Cargo.toml Cargo.lock ./
|
||||
COPY crates/ crates/
|
||||
|
||||
RUN cargo build --release --package herald-server
|
||||
|
||||
|
||||
FROM debian:trixie-slim
|
||||
|
||||
# git clones the pull request. ca-certificates is what every HTTPS call needs
|
||||
# (Gitea, OpenRouter, git). The shared libraries are the non-base ones the binary
|
||||
# links against, as reported by `ldd target/release/herald-server`; libc, libm
|
||||
# and libgcc_s come from the base image.
|
||||
RUN apt-get update && apt-get install -y --no-install-recommends \
|
||||
ca-certificates \
|
||||
git \
|
||||
libssl3t64 \
|
||||
libzstd1 \
|
||||
zlib1g \
|
||||
&& rm -rf /var/lib/apt/lists/*
|
||||
|
||||
|
||||
WORKDIR /app
|
||||
COPY --from=builder /app/target/release/herald .
|
||||
CMD [ "./herald" ]
|
||||
COPY --from=builder /app/target/release/herald-server ./herald-server
|
||||
|
||||
# Exec form, so the binary is PID 1 and receives the SIGTERM it handles to shut
|
||||
# down gracefully.
|
||||
CMD ["./herald-server"]
|
||||
|
||||
@@ -26,7 +26,6 @@ Herald reads its configuration from environment variables (a `.env` file is supp
|
||||
| Variable | Description |
|
||||
|---|---|
|
||||
| `HTTP_PORT` | Port to listen on |
|
||||
| `BOT_NAME` | The bot's Gitea username (used to detect mentions) |
|
||||
| `WEBHOOK_SIG_HEADER_SECRET` | Gitea webhook secret for signature verification |
|
||||
| `OPEN_ROUTER_API_KEY` | OpenRouter API key |
|
||||
| `OPEN_ROUTER_MODEL` | Model to use (e.g. `deepseek/deepseek-v4-flash`) |
|
||||
@@ -38,12 +37,69 @@ Herald reads its configuration from environment variables (a `.env` file is supp
|
||||
| `METRICS_BIND_ADDR` | *(optional)* Bind address for the Prometheus metrics endpoint (e.g. `0.0.0.0:9100`). If unset, the metrics exporter is disabled. |
|
||||
| `SENTRY_DSN` | *(optional)* Sentry DSN for error tracking |
|
||||
| `RUST_LOG` | *(optional)* Log level, defaults to `info` |
|
||||
| `SANDBOX_MAX_ITERATIONS` | *(optional)* Maximum number of tool-calling iterations per sandboxed review. Defaults to `8` |
|
||||
|
qpismont marked this conversation as resolved
Herald
commented
La documentation du sandbox doit préciser ce qui est isolé et ce qui ne l'est pas (réseau, filesystem, credentials), comment le configurer via l'environnement, et rappeler que l'exécution de code issu d'une PR est une opération à risque. Ajouter si possible un exemple de variable d'environnement sécurisé. La documentation du sandbox doit préciser ce qui est isolé et ce qui ne l'est pas (réseau, filesystem, credentials), comment le configurer via l'environnement, et rappeler que l'exécution de code issu d'une PR est une opération à risque. Ajouter si possible un exemple de variable d'environnement sécurisé.
Herald
commented
Documenter le nouveau fonctionnement (workspace multi-crates + sandboxing) : prérequis (Podman/Docker), variables d'environnement nécessaires et limites connues de l'isolation. Documenter le nouveau fonctionnement (workspace multi-crates + sandboxing) : prérequis (Podman/Docker), variables d'environnement nécessaires et limites connues de l'isolation.
|
||||
| `DOCKER_HOST` | *(optional)* Container daemon socket Herald drives, e.g. `unix:///run/user/1000/podman/podman.sock` for a rootless podman. Defaults to `unix:///var/run/docker.sock` |
|
||||
|
||||
## Sandboxed reviews
|
||||
|
||||
Herald reviews pull requests inside an ephemeral
|
||||
[Dev Container](https://containers.dev/). For each review it:
|
||||
|
||||
1. clones the pull request head into a temporary directory,
|
||||
2. builds and starts the repository's devcontainer (`devcontainer-rs`),
|
||||
3. reads the pull request diff and file list from the Gitea API with
|
||||
`GITEA_TOKEN` (so private repositories work), tells the model which files and
|
||||
lines changed — additions and deletions, with the line numbers of the new and
|
||||
old versions of the file respectively — then lets it explore the repository
|
||||
with read-only tools (`ls`, `file_size`, `read_file`, `grep`, `find`) run inside
|
||||
the container: the code itself is not sent, so the model reads it at those lines,
|
||||
4. posts the review, anchoring each comment on the added or removed line it
|
||||
refers to, and removes the container and the temporary clone.
|
||||
|
||||
Generated files are left out of the changes handed to the model: lockfiles
|
||||
(`Cargo.lock`, `package-lock.json`, `yarn.lock`, `go.sum`…) are machine-written
|
||||
dependency churn whose thousands of lines would drown the code under review, and
|
||||
they are never a place where a comment belongs.
|
||||
|
||||
Each comment is tagged with a severity — `bug`, `security`, `performance` or
|
||||
`maintainability` — shown at the start of the comment, and the summary also lists
|
||||
what the pull request does well.
|
||||
|
||||
Herald drives the container daemon through its socket: `DOCKER_HOST` (default
|
||||
`unix:///var/run/docker.sock`), which covers both docker and podman's
|
||||
Docker-compatible socket. The repository must contain a
|
||||
`.devcontainer/devcontainer.json`.
|
||||
|
||||
Herald can therefore run inside a container with only that socket mounted (no
|
||||
shared workspace directory is required): the clone is streamed to the daemon over
|
||||
the socket, like the build context, instead of being bind-mounted from a host
|
||||
path the daemon would have to see. This is the setup the `Containerfile`
|
||||
produces, e.g.:
|
||||
|
||||
```sh
|
||||
podman run --env-file=.env -p 3001:3001 \
|
||||
-v /run/user/$(id -u)/podman/podman.sock:/var/run/docker.sock \
|
||||
herald:latest
|
||||
```
|
||||
|
||||
The `runArgs` of that file are read but deliberately **not** passed to the daemon:
|
||||
they come from an untrusted pull request, and one of them (`--network host`) would
|
||||
attach the container to another network and quietly
|
||||
defeat the network cut described below. A devcontainer that relies on them
|
||||
(`--gpus all`, `--cap-add`, `--shm-size`…) will not get them.
|
||||
|
||||
Each sandbox is isolated: it gets its own image tag, container and network. The
|
||||
container starts with network access so the `postCreateCommand` /
|
||||
`postStartCommand` hooks can install dependencies (e.g. `npm install`); once the
|
||||
hooks have run, the container is disconnected from the network for the rest of
|
||||
the review. Every container command is bounded by a timeout, and the container,
|
||||
network and image are removed when the review ends (including on failure).
|
||||
|
||||
## Development
|
||||
|
||||
The easiest way to get started is with the provided [Dev Container](https://containers.dev/) (VS Code or Zed with the dev container extension).
|
||||
|
||||
Open the project and reopen it in the container — the Rust toolchain is pre-installed.
|
||||
Open the project and reopen it in the container — the Rust toolchain is pre-installed, along with rootless podman, so sandboxed reviews can be exercised locally: start its API socket with `podman system service --time=0 &` and set `DOCKER_HOST=unix:///run/user/$(id -u)/podman/podman.sock`.
|
||||
|
||||
**Without Dev Container**, you just need a Rust toolchain:
|
||||
|
||||
|
||||
@@ -0,0 +1,19 @@
|
||||
[package]
|
||||
name = "devcontainer-rs"
|
||||
version = "0.1.0"
|
||||
edition = "2024"
|
||||
|
||||
[dependencies]
|
||||
bollard = "0.21"
|
||||
bytes = { workspace = true }
|
||||
futures-util = { workspace = true }
|
||||
tar = "0.4"
|
||||
tokio = { workspace = true }
|
||||
tokio-stream = { workspace = true }
|
||||
serde = { workspace = true }
|
||||
serde_json = { workspace = true }
|
||||
anyhow = { workspace = true }
|
||||
thiserror = { workspace = true }
|
||||
|
||||
[dev-dependencies]
|
||||
tempfile = "3"
|
||||
@@ -0,0 +1,24 @@
|
||||
//! Constantes de la crate : endpoint du daemon, timeouts et découpage du
|
||||
//! contexte de build.
|
||||
|
||||
use std::time::Duration;
|
||||
|
||||
/// Endpoint affiché dans les logs quand `DOCKER_HOST` n'est pas défini.
|
||||
pub(crate) const DEFAULT_ENDPOINT: &str = "unix:///var/run/docker.sock";
|
||||
|
||||
/// Timeout appliqué aux opérations de build/run/stop/remove.
|
||||
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.
|
||||
pub(crate) const DEFAULT_EXEC_TIMEOUT: Duration = Duration::from_secs(60);
|
||||
|
||||
/// Taille des morceaux du contexte de build envoyés au 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.
|
||||
pub(crate) const CONTEXT_CHUNKS: usize = 4;
|
||||
|
||||
/// Lectures successives du code de sortie d'un exec, et attente entre elles.
|
||||
pub(crate) const EXIT_CODE_ATTEMPTS: usize = 10;
|
||||
pub(crate) const EXIT_CODE_DELAY: Duration = Duration::from_millis(20);
|
||||
@@ -0,0 +1,93 @@
|
||||
//! Un devcontainer en cours d'exécution : inspection et exécution de commandes.
|
||||
|
||||
use std::time::Duration;
|
||||
|
||||
use crate::{
|
||||
consts::DEFAULT_EXEC_TIMEOUT, errors::ContainerError, exec::ExecOutput,
|
||||
runtime::ContainerRuntime,
|
||||
};
|
||||
|
||||
/// Un devcontainer en cours d'exécution.
|
||||
#[derive(Debug, Clone)]
|
||||
pub struct Container {
|
||||
runtime: ContainerRuntime,
|
||||
name: String,
|
||||
workspace_folder: String,
|
||||
remote_user: Option<String>,
|
||||
network: Option<String>,
|
||||
image: Option<String>,
|
||||
}
|
||||
|
||||
impl Container {
|
||||
/// Assemble un container démarré, avec son réseau et son image à nettoyer.
|
||||
pub(crate) fn new(
|
||||
runtime: ContainerRuntime,
|
||||
name: String,
|
||||
workspace_folder: String,
|
||||
remote_user: Option<String>,
|
||||
network: Option<String>,
|
||||
image: Option<String>,
|
||||
) -> Self {
|
||||
Self {
|
||||
runtime,
|
||||
name,
|
||||
workspace_folder,
|
||||
remote_user,
|
||||
network,
|
||||
image,
|
||||
}
|
||||
}
|
||||
|
||||
pub fn name(&self) -> &str {
|
||||
&self.name
|
||||
}
|
||||
|
||||
pub fn workspace_folder(&self) -> &str {
|
||||
&self.workspace_folder
|
||||
}
|
||||
|
||||
/// Exécute une commande dans le container et renvoie sa sortie.
|
||||
///
|
||||
/// La commande est transmise sous forme de vecteur d'arguments (argv, sans shell),
|
||||
/// donc aucun échappement ni interpolation n'est effectué.
|
||||
pub async fn exec(&self, cmd: &[&str]) -> Result<ExecOutput, ContainerError> {
|
||||
self.exec_with_timeout(cmd, DEFAULT_EXEC_TIMEOUT).await
|
||||
}
|
||||
|
||||
pub(crate) async fn exec_with_timeout(
|
||||
&self,
|
||||
cmd: &[&str],
|
||||
timeout: Duration,
|
||||
) -> Result<ExecOutput, ContainerError> {
|
||||
self.runtime
|
||||
.exec(&self.name, cmd, self.remote_user.as_deref(), timeout)
|
||||
.await
|
||||
}
|
||||
|
||||
/// Exécute un script shell dans le container via `sh -c`.
|
||||
pub async fn exec_shell(&self, script: &str) -> Result<ExecOutput, ContainerError> {
|
||||
self.exec(&["sh", "-c", script]).await
|
||||
}
|
||||
|
||||
/// Arrête le container.
|
||||
pub async fn stop(&self) -> Result<(), ContainerError> {
|
||||
self.runtime.stop_container(&self.name).await
|
||||
}
|
||||
|
||||
/// Supprime le container, son réseau et son image.
|
||||
///
|
||||
/// La suppression du réseau et de l'image est best-effort : ils peuvent déjà être absents.
|
||||
pub async fn remove(&self) -> Result<(), ContainerError> {
|
||||
self.runtime.remove_container(&self.name).await?;
|
||||
|
Herald
commented
La méthode quitte immédiatement si La méthode quitte immédiatement si `remove_container` échoue, alors que sa documentation indique que le réseau et l'image doivent également être supprimés en best-effort. Dans ce cas, l'image et le réseau restent orphelins, et `Sandbox::cleanup` ne peut pas récupérer ces ressources. Il faut tenter les suppressions même après l'échec du container, puis retourner une erreur agrégée ou la première erreur rencontrée.
Herald
commented
Le nettoyage s'arrête immédiatement si Le nettoyage s'arrête immédiatement si `remove_container` échoue, et les suppressions du réseau et de l'image ne sont alors jamais tentées. Une course, un conteneur déjà supprimé ou une erreur transitoire peut donc laisser systématiquement le réseau et l'image derrière lui. Il faut tenter les trois nettoyages indépendamment, puis agréger ou retourner l'erreur pertinente.
Herald
commented
[performance] Si **[performance]** Si `remove_container` échoue, le `?` quitte immédiatement la méthode et le réseau ainsi que l'image ne sont jamais supprimés. Après un daemon indisponible, un container déjà supprimé ou une erreur transitoire, chaque sandbox peut donc laisser des ressources persistantes. Effectuer le nettoyage du réseau et de l'image même lorsque la suppression du container échoue, puis retourner l'erreur.
|
||||
|
||||
if let Some(network) = &self.network {
|
||||
let _ = self.runtime.remove_network(network).await;
|
||||
}
|
||||
|
||||
if let Some(image) = &self.image {
|
||||
let _ = self.runtime.remove_image(image).await;
|
||||
}
|
||||
|
||||
Ok(())
|
||||
}
|
||||
}
|
||||
@@ -0,0 +1,166 @@
|
||||
//! Empaquetage en flux du contexte de build envoyé au daemon.
|
||||
|
||||
use std::{
|
||||
io::{BufWriter, Write},
|
||||
path::Path,
|
||||
};
|
||||
|
||||
use bytes::Bytes;
|
||||
use futures_util::Stream;
|
||||
use tokio::sync::mpsc;
|
||||
use tokio_stream::wrappers::ReceiverStream;
|
||||
|
||||
use crate::consts::{CONTEXT_CHUNK_SIZE, CONTEXT_CHUNKS};
|
||||
|
||||
/// Empaquette le contexte de build dans un tar, **en flux**.
|
||||
///
|
||||
/// Le CLI `docker build` empaquette le contexte puis l'envoie ; le faire d'un coup
|
||||
/// chargerait tout le dossier en mémoire, ce qui devient intenable dès que le
|
||||
/// `.devcontainer` embarque des binaires. Ici l'archive est écrite par une tâche
|
||||
/// bloquante et poussée morceau par morceau, avec la contre-pression du canal :
|
||||
/// si le daemon lit lentement, l'empaquetage ralentit.
|
||||
pub(crate) fn tar_directory_stream(
|
||||
dir: &Path,
|
||||
) -> impl Stream<Item = Result<Bytes, std::io::Error>> + Send + 'static {
|
||||
let (sender, receiver) = mpsc::channel(CONTEXT_CHUNKS);
|
||||
let failures = sender.clone();
|
||||
let dir = dir.to_path_buf();
|
||||
|
||||
tokio::task::spawn_blocking(move || {
|
||||
let writer = BufWriter::with_capacity(CONTEXT_CHUNK_SIZE, ChannelWriter { sender });
|
||||
let mut builder = tar::Builder::new(writer);
|
||||
|
||||
let result = builder
|
||||
.append_dir_all(".", &dir)
|
||||
.and_then(|()| builder.finish())
|
||||
// `finish` écrit les blocs de fin d'archive ; il reste à vider le tampon
|
||||
// pour que le daemon reçoive tout, y compris une archive vide.
|
||||
.and_then(|()| builder.get_mut().flush());
|
||||
|
||||
if let Err(error) = result {
|
||||
// L'échec est remonté au daemon par le flux : sinon il ne verrait qu'un
|
||||
// tar tronqué et signalerait une erreur incompréhensible.
|
||||
let _ = failures.blocking_send(Err(error));
|
||||
}
|
||||
});
|
||||
|
||||
ReceiverStream::new(receiver)
|
||||
}
|
||||
|
||||
/// Écrit les morceaux du tar dans un canal, en attendant qu'il se vide.
|
||||
///
|
||||
/// Utilisé depuis une tâche bloquante, seul contexte où `blocking_send` est autorisé.
|
||||
struct ChannelWriter {
|
||||
sender: mpsc::Sender<Result<Bytes, std::io::Error>>,
|
||||
}
|
||||
|
||||
impl Write for ChannelWriter {
|
||||
fn write(&mut self, buf: &[u8]) -> std::io::Result<usize> {
|
||||
self.sender
|
||||
.blocking_send(Ok(Bytes::copy_from_slice(buf)))
|
||||
.map_err(|_| {
|
||||
std::io::Error::new(
|
||||
std::io::ErrorKind::BrokenPipe,
|
||||
"the daemon dropped the build context",
|
||||
)
|
||||
})?;
|
||||
|
||||
Ok(buf.len())
|
||||
}
|
||||
|
||||
fn flush(&mut self) -> std::io::Result<()> {
|
||||
Ok(())
|
||||
}
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
mod tests {
|
||||
use super::*;
|
||||
use std::fs;
|
||||
|
||||
use futures_util::StreamExt;
|
||||
|
||||
/// Rassemble le flux du contexte en un tar complet, pour l'inspecter.
|
||||
async fn context_tar(dir: &Path) -> Vec<u8> {
|
||||
let mut tar = Vec::new();
|
||||
let mut chunks = tar_directory_stream(dir);
|
||||
|
||||
while let Some(chunk) = chunks.next().await {
|
||||
tar.extend_from_slice(&chunk.unwrap());
|
||||
}
|
||||
|
||||
tar
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn tar_context_holds_the_devcontainer_files() {
|
||||
let dir = tempfile::tempdir().unwrap();
|
||||
let devcontainer_dir = dir.path().join(".devcontainer");
|
||||
fs::create_dir(&devcontainer_dir).unwrap();
|
||||
fs::write(devcontainer_dir.join("Dockerfile"), "FROM alpine\n").unwrap();
|
||||
fs::write(devcontainer_dir.join("devcontainer.json"), "{}").unwrap();
|
||||
|
||||
let tar = context_tar(&devcontainer_dir).await;
|
||||
|
||||
let mut archive = tar::Archive::new(tar.as_slice());
|
||||
let names = archive
|
||||
.entries()
|
||||
.unwrap()
|
||||
.map(|entry| entry.unwrap().path().unwrap().display().to_string())
|
||||
.collect::<Vec<_>>();
|
||||
|
||||
assert!(names.iter().any(|name| name.ends_with("Dockerfile")));
|
||||
assert!(names.iter().any(|name| name.ends_with("devcontainer.json")));
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn tar_context_keeps_the_executable_bit() {
|
||||
let dir = tempfile::tempdir().unwrap();
|
||||
let script = dir.path().join("setup.sh");
|
||||
fs::write(&script, "#!/bin/sh\n").unwrap();
|
||||
|
||||
#[cfg(unix)]
|
||||
{
|
||||
use std::os::unix::fs::PermissionsExt;
|
||||
fs::set_permissions(&script, fs::Permissions::from_mode(0o755)).unwrap();
|
||||
}
|
||||
|
||||
let tar = context_tar(dir.path()).await;
|
||||
|
||||
let mut archive = tar::Archive::new(tar.as_slice());
|
||||
let mode = archive
|
||||
.entries()
|
||||
.unwrap()
|
||||
.map(|entry| entry.unwrap())
|
||||
.find(|entry| entry.path().unwrap().ends_with("setup.sh"))
|
||||
.unwrap()
|
||||
.header()
|
||||
.mode()
|
||||
.unwrap();
|
||||
|
||||
assert_eq!(mode & 0o111, 0o111);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn tar_context_is_sent_in_chunks() {
|
||||
let dir = tempfile::tempdir().unwrap();
|
||||
fs::write(
|
||||
dir.path().join("big.bin"),
|
||||
vec![0_u8; CONTEXT_CHUNK_SIZE * 3],
|
||||
)
|
||||
.unwrap();
|
||||
|
||||
let mut chunks = tar_directory_stream(dir.path());
|
||||
let mut count = 0;
|
||||
|
||||
while let Some(chunk) = chunks.next().await {
|
||||
assert!(chunk.unwrap().len() <= CONTEXT_CHUNK_SIZE);
|
||||
count += 1;
|
||||
}
|
||||
|
||||
assert!(
|
||||
count > 1,
|
||||
"the context should be streamed, not buffered in one piece"
|
||||
);
|
||||
}
|
||||
}
|
||||
@@ -0,0 +1,282 @@
|
||||
//! Représentation analysée d'un `devcontainer.json` et son cycle de vie :
|
||||
//! nommage, build, démarrage et hooks.
|
||||
|
||||
use std::{
|
||||
collections::HashMap,
|
||||
path::{Path, PathBuf},
|
||||
time::{Duration, SystemTime, UNIX_EPOCH},
|
||||
};
|
||||
|
||||
use bollard::models::{ContainerCreateBody, HostConfig};
|
||||
|
||||
use crate::{container::Container, errors::ContainerError, runtime::ContainerRuntime};
|
||||
|
||||
#[derive(Debug)]
|
||||
pub struct DevContainer {
|
||||
pub container_file_path: PathBuf,
|
||||
pub name: Option<String>,
|
||||
pub build_args: HashMap<String, String>,
|
||||
pub container_env: HashMap<String, String>,
|
||||
pub workspace_folder: Option<String>,
|
||||
pub post_create_command: Option<String>,
|
||||
pub post_start_command: Option<String>,
|
||||
pub remote_user: Option<String>,
|
||||
}
|
||||
|
||||
impl DevContainer {
|
||||
/// Nom de l'image de base dérivé du nom du devcontainer.
|
||||
pub fn image_name(&self) -> String {
|
||||
let base = self.name.as_deref().unwrap_or("devcontainer");
|
||||
format!("devcontainer-rs/{}", sanitize(base))
|
||||
}
|
||||
|
||||
/// Tag d'image unique pour une exécution de sandbox donnée.
|
||||
///
|
||||
/// L'unicité est importante : deux sandboxes concurrentes (éventuellement pour des
|
||||
/// dépôts différents partageant un nom de devcontainer) ne doivent pas se disputer le même tag.
|
||||
pub fn image_tag(&self) -> String {
|
||||
format!("{}:{}", self.image_name(), unique_suffix())
|
||||
}
|
||||
|
||||
/// Dossier de workspace dans le container, par défaut `/workspaces/workspace`.
|
||||
pub fn workspace_folder(&self) -> String {
|
||||
self.workspace_folder
|
||||
.clone()
|
||||
.unwrap_or_else(|| "/workspaces/workspace".to_string())
|
||||
}
|
||||
|
||||
/// Nom de container unique pour cette exécution.
|
||||
pub fn container_name(&self) -> String {
|
||||
let base = self.name.as_deref().unwrap_or("devcontainer");
|
||||
format!("devcontainer-rs-{}-{}", sanitize(base), unique_suffix())
|
||||
}
|
||||
|
||||
/// Construit l'image devcontainer sous `image_tag`.
|
||||
pub async fn build(
|
||||
&self,
|
||||
runtime: &ContainerRuntime,
|
||||
image_tag: &str,
|
||||
) -> Result<(), ContainerError> {
|
||||
let context = self.container_file_path.parent().ok_or_else(|| {
|
||||
ContainerError::Unexpected(String::from(
|
||||
"the devcontainer file path has no parent directory",
|
||||
))
|
||||
})?;
|
||||
|
||||
let dockerfile = self
|
||||
.container_file_path
|
||||
.file_name()
|
||||
.and_then(|name| name.to_str())
|
||||
.ok_or_else(|| {
|
||||
ContainerError::Unexpected(String::from(
|
||||
"the devcontainer file name is not valid UTF-8",
|
||||
))
|
||||
})?;
|
||||
|
||||
runtime
|
||||
.build_image(context, dockerfile, image_tag, &self.build_args)
|
||||
.await
|
||||
}
|
||||
|
||||
/// Construit l'image, démarre le container, exécute les hooks puis coupe le réseau.
|
||||
///
|
||||
/// En cas d'échec, le container, le réseau et l'image sont nettoyés avant de
|
||||
/// renvoyer l'erreur, afin qu'aucune ressource ne soit laissée en place.
|
||||
pub async fn up(
|
||||
&self,
|
||||
runtime: &ContainerRuntime,
|
||||
workspace_dir: &Path,
|
||||
) -> Result<Container, ContainerError> {
|
||||
let image_tag = self.image_tag();
|
||||
self.build(runtime, &image_tag).await?;
|
||||
|
||||
let name = self.container_name();
|
||||
let network = format!("{name}-net");
|
||||
|
||||
// Réseau dédié afin de pouvoir couper la connectivité après les hooks.
|
||||
if let Err(err) = runtime.create_network(&network).await {
|
||||
let _ = runtime.remove_image(&image_tag).await;
|
||||
return Err(err);
|
||||
}
|
||||
|
||||
let workspace_folder = self.workspace_folder();
|
||||
let body = ContainerCreateBody {
|
||||
image: Some(image_tag.clone()),
|
||||
cmd: Some(vec![String::from("sleep"), String::from("infinity")]),
|
||||
user: self.remote_user.clone(),
|
||||
env: Some(
|
||||
self.container_env
|
||||
.iter()
|
||||
.map(|(key, value)| format!("{key}={value}"))
|
||||
.collect(),
|
||||
),
|
||||
working_dir: Some(workspace_folder.clone()),
|
||||
host_config: Some(HostConfig {
|
||||
|
Herald
commented
[Élevé] Le conteneur sandbox n'a aucune limite de ressources (mémoire, CPU, processus ou taille disque). Le Dockerfile, les hooks et les commandes demandées par le modèle sont contrôlés indirectement par une PR non fiable et peuvent provoquer une fork bomb, remplir le disque ou monopoliser l'hôte. Ajouter des limites explicites dans [Élevé] Le conteneur sandbox n'a aucune limite de ressources (mémoire, CPU, processus ou taille disque). Le Dockerfile, les hooks et les commandes demandées par le modèle sont contrôlés indirectement par une PR non fiable et peuvent provoquer une fork bomb, remplir le disque ou monopoliser l'hôte. Ajouter des limites explicites dans `HostConfig` et prévoir aussi un nettoyage fiable en cas de dépassement.
|
||||
network_mode: Some(network.clone()),
|
||||
..Default::default()
|
||||
}),
|
||||
..Default::default()
|
||||
};
|
||||
|
||||
if let Err(err) = runtime.create_container(&name, body).await {
|
||||
let _ = runtime.remove_network(&network).await;
|
||||
let _ = runtime.remove_image(&image_tag).await;
|
||||
return Err(err);
|
||||
}
|
||||
|
Herald
commented
[Élevé] [Élevé] `label=disable` désactive explicitement le confinement SELinux pour un conteneur exécutant du code contrôlé par une pull request. Cela élargit fortement l'impact d'une compromission du conteneur et contredit l'objectif de sandboxing. Préférer un montage correctement relabellisé (`:Z`/équivalent selon le runtime) ou rendre ce comportement optionnel et documenter clairement la perte d'isolation.
Herald
commented
`label=disable` désactive explicitement le confinement SELinux du container. Pour une sandbox qui exécute du code provenant d'une PR non fiable, cela élargit fortement l'impact d'une évasion ou d'une vulnérabilité du runtime. Il faudrait privilégier un montage correctement relabellisé (`:Z`/équivalent) ou rendre cette option explicite et désactivée par défaut, avec une justification claire du niveau de risque.
|
||||
|
||||
if let Err(err) = runtime.start_container(&name).await {
|
||||
let _ = runtime.remove_container(&name).await;
|
||||
let _ = runtime.remove_network(&network).await;
|
||||
let _ = runtime.remove_image(&image_tag).await;
|
||||
return Err(err);
|
||||
}
|
||||
|
||||
// Le clone est copié dans le container par le socket, pas monté depuis un
|
||||
// chemin de l'hôte : le daemon n'a pas besoin de voir le clone pour le rendre
|
||||
// visible dans la sandbox, ce qui permet à Herald de tourner dans un container
|
||||
// (avec le socket monté) sans partager de dossier avec l'hôte.
|
||||
if let Err(err) = runtime
|
||||
.upload_directory(&name, &workspace_folder, workspace_dir)
|
||||
.await
|
||||
{
|
||||
let _ = runtime.remove_container(&name).await;
|
||||
let _ = runtime.remove_network(&network).await;
|
||||
let _ = runtime.remove_image(&image_tag).await;
|
||||
return Err(err);
|
||||
}
|
||||
|
||||
let container = Container::new(
|
||||
runtime.clone(),
|
||||
name,
|
||||
self.workspace_folder(),
|
||||
self.remote_user.clone(),
|
||||
Some(network.clone()),
|
||||
Some(image_tag),
|
||||
);
|
||||
|
||||
// Les hooks s'exécutent avec accès au réseau (installation de dépendances, etc.).
|
||||
if let Err(err) = self.run_hooks(&container, runtime.timeout()).await {
|
||||
let _ = container.remove().await;
|
||||
return Err(err);
|
||||
}
|
||||
|
||||
// Coupe l'accès réseau pour le reste de la durée de vie de la sandbox.
|
||||
if let Err(err) = runtime.disconnect_network(&network, container.name()).await {
|
||||
let _ = container.remove().await;
|
||||
return Err(err);
|
||||
}
|
||||
|
||||
Ok(container)
|
||||
}
|
||||
|
||||
async fn run_hooks(
|
||||
&self,
|
||||
container: &Container,
|
||||
timeout: Duration,
|
||||
) -> Result<(), ContainerError> {
|
||||
for command in [&self.post_create_command, &self.post_start_command]
|
||||
.into_iter()
|
||||
.flatten()
|
||||
{
|
||||
let output = container
|
||||
.exec_with_timeout(&["sh", "-c", command], timeout)
|
||||
.await?;
|
||||
|
||||
if !output.success() {
|
||||
return Err(ContainerError::Unexpected(format!(
|
||||
"hook `{command}` failed with status {}: {}",
|
||||
output.status,
|
||||
output.stderr.trim()
|
||||
)));
|
||||
}
|
||||
}
|
||||
|
||||
Ok(())
|
||||
}
|
||||
}
|
||||
|
||||
/// Nettoie une chaîne pour qu'elle puisse servir de nom d'image/container docker.
|
||||
fn sanitize(input: &str) -> String {
|
||||
let sanitized: String = input
|
||||
.chars()
|
||||
.map(|c| {
|
||||
if c.is_ascii_alphanumeric() || c == '-' || c == '_' || c == '.' {
|
||||
c.to_ascii_lowercase()
|
||||
} else {
|
||||
'-'
|
||||
}
|
||||
})
|
||||
.collect();
|
||||
|
||||
let trimmed = sanitized.trim_matches(|c| c == '-' || c == '.' || c == '_');
|
||||
if trimmed.is_empty() {
|
||||
"devcontainer".to_string()
|
||||
} else {
|
||||
trimmed.to_string()
|
||||
}
|
||||
}
|
||||
|
||||
/// Suffixe unique à une exécution de sandbox, combinant l'identifiant de processus et un horodatage.
|
||||
fn unique_suffix() -> String {
|
||||
let nanos = SystemTime::now()
|
||||
.duration_since(UNIX_EPOCH)
|
||||
.map(|duration| duration.as_nanos())
|
||||
.unwrap_or(0);
|
||||
|
||||
format!("{}-{}", std::process::id(), nanos)
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
mod tests {
|
||||
use super::*;
|
||||
use std::fs;
|
||||
|
||||
fn devcontainer(dir: &Path) -> DevContainer {
|
||||
let devcontainer_path = dir.join("devcontainer.json");
|
||||
let dockerfile_path = dir.join("Dockerfile");
|
||||
|
||||
fs::write(&dockerfile_path, "FROM alpine\n").unwrap();
|
||||
fs::write(
|
||||
&devcontainer_path,
|
||||
r#"{
|
||||
"name": "My Project",
|
||||
"build": {
|
||||
"dockerfile": "Dockerfile",
|
||||
"args": { "VERSION": "1" }
|
||||
},
|
||||
"workspaceFolder": "/workspaces/my-project",
|
||||
"containerEnv": { "RUST_LOG": "debug" },
|
||||
"remoteUser": "dev"
|
||||
}"#,
|
||||
)
|
||||
.unwrap();
|
||||
|
||||
let runtime = tokio::runtime::Runtime::new().unwrap();
|
||||
runtime.block_on(crate::parse(&devcontainer_path)).unwrap()
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn image_name_is_sanitized() {
|
||||
let dir = tempfile::tempdir().unwrap();
|
||||
let dc = devcontainer(dir.path());
|
||||
assert_eq!(dc.image_name(), "devcontainer-rs/my-project");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn image_tags_are_unique() {
|
||||
let dir = tempfile::tempdir().unwrap();
|
||||
let dc = devcontainer(dir.path());
|
||||
assert_ne!(dc.image_tag(), dc.image_tag());
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn container_name_is_sanitized() {
|
||||
let dir = tempfile::tempdir().unwrap();
|
||||
let dc = devcontainer(dir.path());
|
||||
|
||||
assert!(
|
||||
dc.container_name()
|
||||
.starts_with("devcontainer-rs-my-project-")
|
||||
);
|
||||
}
|
||||
}
|
||||
@@ -0,0 +1,52 @@
|
||||
//! Erreurs de la crate : échecs du daemon de containers et échecs d'analyse du
|
||||
//! `devcontainer.json`.
|
||||
|
||||
use std::{path::PathBuf, time::Duration};
|
||||
|
||||
use bollard::errors::Error as BollardError;
|
||||
|
||||
#[derive(Debug, thiserror::Error)]
|
||||
pub enum ContainerError {
|
||||
/// Le daemon est injoignable, ou a refusé une requête.
|
||||
#[error("container daemon request failed: {source}")]
|
||||
Request {
|
||||
#[from]
|
||||
source: BollardError,
|
||||
},
|
||||
|
||||
/// Une opération a dépassé son timeout.
|
||||
#[error("`{operation}` timed out after {timeout:?}")]
|
||||
Timeout {
|
||||
operation: String,
|
||||
timeout: Duration,
|
||||
},
|
||||
|
||||
/// La construction de l'image a échoué.
|
||||
#[error("image build failed: {message}")]
|
||||
Build { message: String },
|
||||
|
||||
/// Le daemon a répondu autre chose que ce qui était attendu.
|
||||
#[error("{0}")]
|
||||
Unexpected(String),
|
||||
}
|
||||
|
||||
#[derive(Debug, thiserror::Error)]
|
||||
pub enum ParseError {
|
||||
#[error("failed to read devcontainer file `{path}`: {source}")]
|
||||
Read {
|
||||
path: PathBuf,
|
||||
source: std::io::Error,
|
||||
},
|
||||
|
||||
#[error("invalid devcontainer JSON in `{path}`: {source}")]
|
||||
Json {
|
||||
path: PathBuf,
|
||||
source: serde_json::Error,
|
||||
},
|
||||
|
||||
#[error("container file `{0}` does not exist or is not a regular file")]
|
||||
ContainerFileNotFound(PathBuf),
|
||||
|
||||
#[error("the devcontainer file path has no parent directory: `{0}`")]
|
||||
InvalidDevContainerPath(PathBuf),
|
||||
}
|
||||
@@ -0,0 +1,16 @@
|
||||
//! Résultat d'une commande exécutée dans un container.
|
||||
|
||||
/// Résultat d'une commande exécutée dans un container.
|
||||
#[derive(Debug, Clone)]
|
||||
pub struct ExecOutput {
|
||||
/// Code de sortie, ou `-1` si le processus a été terminé par un signal.
|
||||
pub status: i32,
|
||||
pub stdout: String,
|
||||
pub stderr: String,
|
||||
}
|
||||
|
||||
impl ExecOutput {
|
||||
pub fn success(&self) -> bool {
|
||||
self.status == 0
|
||||
}
|
||||
}
|
||||
@@ -0,0 +1,43 @@
|
||||
//! Primitives de cycle de vie de container pour un [`DevContainer`] analysé.
|
||||
//!
|
||||
//! Cette crate pilote l'API du daemon de containers pour construire l'image
|
||||
//! devcontainer, 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.
|
||||
//!
|
||||
//! Le daemon est joint par son **socket** (celui de l'hôte, monté dans le
|
||||
//! container de Herald) : [`ContainerRuntime::connect`] suit `DOCKER_HOST` comme
|
||||
//! le fait le CLI `docker`, et retombe sur le socket local. Aucun binaire de
|
||||
//! runtime n'est donc requis dans l'image, et `podman` fonctionne de la même
|
||||
//! façon dès lors qu'il expose son socket compatible Docker.
|
||||
//!
|
||||
//! # Isolation
|
||||
//!
|
||||
//! 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.
|
||||
//!
|
||||
//! Les `runArgs` du `devcontainer.json` sont lus mais pas transmis au daemon : ils
|
||||
//! viennent du dépôt, donc d'une pull request non fiable, et pourraient rattacher le
|
||||
//! container à un autre réseau ou lui donner des privilèges qui annuleraient cette
|
||||
//! isolation.
|
||||
|
||||
mod consts;
|
||||
mod container;
|
||||
mod context;
|
||||
mod devcontainer;
|
||||
mod errors;
|
||||
mod exec;
|
||||
mod path;
|
||||
mod runtime;
|
||||
mod schema;
|
||||
|
||||
pub use container::Container;
|
||||
pub use devcontainer::DevContainer;
|
||||
pub use errors::{ContainerError, ParseError};
|
||||
pub use exec::ExecOutput;
|
||||
pub use path::normalize;
|
||||
pub use runtime::ContainerRuntime;
|
||||
pub use schema::{DevContainerBuildSchema, DevContainerSchema, parse};
|
||||
@@ -0,0 +1,40 @@
|
||||
//! Normalisation lexicale de chemins, sans accès au système de fichiers.
|
||||
|
||||
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.
|
||||
pub fn normalize(path: &Path) -> Option<PathBuf> {
|
||||
use std::path::Component;
|
||||
|
||||
let mut out = PathBuf::new();
|
||||
for component in path.components() {
|
||||
match component {
|
||||
Component::RootDir => out.push("/"),
|
||||
Component::CurDir => {}
|
||||
Component::ParentDir => {
|
||||
if !out.pop() {
|
||||
return None;
|
||||
}
|
||||
}
|
||||
Component::Normal(part) => out.push(part),
|
||||
Component::Prefix(_) => return None,
|
||||
}
|
||||
}
|
||||
|
||||
Some(out)
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
mod tests {
|
||||
use super::*;
|
||||
|
||||
#[test]
|
||||
fn normalize_rejects_escaping_paths() {
|
||||
assert_eq!(
|
||||
normalize(Path::new("/workspaces/project/src/../main.rs")),
|
||||
Some(PathBuf::from("/workspaces/project/main.rs"))
|
||||
);
|
||||
assert_eq!(normalize(Path::new("/workspaces/../../etc/passwd")), None);
|
||||
}
|
||||
}
|
||||
@@ -0,0 +1,376 @@
|
||||
//! Client de l'API du daemon de containers et opérations de cycle de vie.
|
||||
|
||||
use std::{collections::HashMap, path::Path, time::Duration};
|
||||
|
||||
use bollard::{
|
||||
Docker, body_try_stream,
|
||||
container::LogOutput,
|
||||
errors::Error as BollardError,
|
||||
exec::{CreateExecOptions, StartExecOptions, StartExecResults},
|
||||
models::{BuildInfo, ContainerCreateBody, NetworkCreateRequest, NetworkDisconnectRequest},
|
||||
query_parameters::{
|
||||
BuildImageOptions, CreateContainerOptions, RemoveContainerOptions, RemoveImageOptions,
|
||||
StartContainerOptions, StopContainerOptions, UploadToContainerOptions,
|
||||
},
|
||||
};
|
||||
use futures_util::StreamExt;
|
||||
|
||||
use crate::{
|
||||
consts::{DEFAULT_COMMAND_TIMEOUT, DEFAULT_ENDPOINT, EXIT_CODE_ATTEMPTS, EXIT_CODE_DELAY},
|
||||
context::tar_directory_stream,
|
||||
errors::ContainerError,
|
||||
exec::ExecOutput,
|
||||
};
|
||||
|
||||
/// Un client de l'API du daemon de containers.
|
||||
#[derive(Debug, Clone)]
|
||||
pub struct ContainerRuntime {
|
||||
docker: Docker,
|
||||
endpoint: String,
|
||||
timeout: Duration,
|
||||
}
|
||||
|
||||
impl ContainerRuntime {
|
||||
/// Se connecte au daemon désigné par `DOCKER_HOST`, ou au socket local par défaut.
|
||||
pub fn connect() -> Result<Self, ContainerError> {
|
||||
let endpoint =
|
||||
std::env::var("DOCKER_HOST").unwrap_or_else(|_| String::from(DEFAULT_ENDPOINT));
|
||||
|
Herald
commented
La valeur de La valeur de `DOCKER_HOST` est seulement stockée dans `endpoint` pour les logs : `Docker::connect_with_defaults()` ne l'utilise pas. Ainsi, la configuration documentée pour Podman ou pour un socket Docker non standard est ignorée et le runtime tente toujours sa connexion par défaut. Il faut construire le client Bollard avec l'endpoint lu dans l'environnement, ou supprimer cette configuration trompeuse.
|
||||
|
||||
Ok(Self {
|
||||
docker: Docker::connect_with_defaults()?,
|
||||
|
Herald
commented
`endpoint` lit `DOCKER_HOST`, mais le client Bollard est créé avec `connect_with_defaults()` sans utiliser cette valeur. Ainsi, le daemon configuré via `DOCKER_HOST` (notamment le socket Podman rootless documenté) risque d'être ignoré, tandis que les logs indiquent un endpoint différent de celui réellement utilisé. Il faut construire le client à partir de l'endpoint résolu, ou vérifier que l'API utilisée prend effectivement en charge `DOCKER_HOST`.
|
||||
endpoint,
|
||||
timeout: DEFAULT_COMMAND_TIMEOUT,
|
||||
})
|
||||
}
|
||||
|
||||
/// Remplace le timeout appliqué aux opérations de build/run/stop/remove.
|
||||
pub fn with_timeout(mut self, timeout: Duration) -> Self {
|
||||
self.timeout = timeout;
|
||||
self
|
||||
}
|
||||
|
||||
/// Endpoint du daemon, pour les logs.
|
||||
pub fn endpoint(&self) -> &str {
|
||||
&self.endpoint
|
||||
}
|
||||
|
||||
pub fn timeout(&self) -> Duration {
|
||||
self.timeout
|
||||
}
|
||||
|
||||
/// Vérifie que le daemon est joignable et répond.
|
||||
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.
|
||||
async fn request<T>(
|
||||
&self,
|
||||
operation: &str,
|
||||
request: impl Future<Output = Result<T, BollardError>>,
|
||||
) -> Result<T, ContainerError> {
|
||||
match tokio::time::timeout(self.timeout, request).await {
|
||||
Ok(Ok(value)) => Ok(value),
|
||||
Ok(Err(source)) => Err(ContainerError::Request { source }),
|
||||
Err(_) => Err(ContainerError::Timeout {
|
||||
operation: String::from(operation),
|
||||
timeout: self.timeout,
|
||||
}),
|
||||
}
|
||||
}
|
||||
|
||||
/// Construit l'image `tag` depuis le contexte `context_dir`.
|
||||
pub(crate) async fn build_image(
|
||||
&self,
|
||||
context_dir: &Path,
|
||||
dockerfile: &str,
|
||||
tag: &str,
|
||||
build_args: &HashMap<String, String>,
|
||||
) -> Result<(), ContainerError> {
|
||||
let options = BuildImageOptions {
|
||||
dockerfile: String::from(dockerfile),
|
||||
t: Some(String::from(tag)),
|
||||
buildargs: Some(build_args.clone()),
|
||||
rm: true,
|
||||
..Default::default()
|
||||
};
|
||||
|
||||
let context = tar_directory_stream(context_dir);
|
||||
let mut stream = self
|
||||
.docker
|
||||
.build_image(options, None, Some(body_try_stream(context)));
|
||||
|
||||
// Le flux porte la progression du build : la dernière erreur signalée par le
|
||||
// daemon fait échouer l'opération.
|
||||
let consume = async {
|
||||
let mut failure = None;
|
||||
|
||||
while let Some(info) = stream.next().await {
|
||||
let info: BuildInfo = info?;
|
||||
|
||||
if let Some(message) = info.error_detail.and_then(|detail| detail.message) {
|
||||
|
Herald
commented
Le flux de build n'est considéré en échec que lorsque Le flux de build n'est considéré en échec que lorsque `error_detail.message` est renseigné. L'API Docker peut aussi fournir l'erreur dans le champ `error` de `BuildInfo`; dans ce cas la méthode retourne `Ok(())` alors que l'image n'a pas été construite. Il faut traiter les deux champs et préserver le message d'erreur le plus utile.
|
||||
failure = Some(message);
|
||||
}
|
||||
}
|
||||
|
||||
Ok::<_, BollardError>(failure)
|
||||
};
|
||||
|
||||
let failure = match tokio::time::timeout(self.timeout, consume).await {
|
||||
Ok(Ok(failure)) => failure,
|
||||
Ok(Err(source)) => return Err(ContainerError::Request { source }),
|
||||
Err(_) => {
|
||||
return Err(ContainerError::Timeout {
|
||||
operation: format!("build image `{tag}`"),
|
||||
timeout: self.timeout,
|
||||
});
|
||||
}
|
||||
};
|
||||
|
||||
match failure {
|
||||
Some(message) => Err(ContainerError::Build { message }),
|
||||
None => Ok(()),
|
||||
}
|
||||
}
|
||||
|
||||
/// Crée un container, sans le démarrer.
|
||||
pub(crate) async fn create_container(
|
||||
&self,
|
||||
name: &str,
|
||||
body: ContainerCreateBody,
|
||||
) -> Result<(), ContainerError> {
|
||||
let options = CreateContainerOptions {
|
||||
name: Some(String::from(name)),
|
||||
..Default::default()
|
||||
};
|
||||
|
||||
self.request(
|
||||
"create container",
|
||||
self.docker.create_container(Some(options), body),
|
||||
)
|
||||
.await?;
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
pub(crate) async fn upload_directory(
|
||||
&self,
|
||||
container: &str,
|
||||
destination: &str,
|
||||
directory: &Path,
|
||||
) -> Result<(), ContainerError> {
|
||||
let options = UploadToContainerOptions {
|
||||
path: String::from(destination),
|
||||
..Default::default()
|
||||
};
|
||||
|
||||
self.request(
|
||||
"upload workspace",
|
||||
self.docker.upload_to_container(
|
||||
container,
|
||||
Some(options),
|
||||
body_try_stream(tar_directory_stream(directory)),
|
||||
),
|
||||
)
|
||||
.await?;
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
pub(crate) async fn start_container(&self, name: &str) -> Result<(), ContainerError> {
|
||||
self.request(
|
||||
"start container",
|
||||
self.docker
|
||||
.start_container(name, None::<StartContainerOptions>),
|
||||
)
|
||||
.await?;
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
/// Exécute une commande dans un container et renvoie sa sortie.
|
||||
pub(crate) async fn exec(
|
||||
&self,
|
||||
container: &str,
|
||||
cmd: &[&str],
|
||||
user: Option<&str>,
|
||||
timeout: Duration,
|
||||
) -> Result<ExecOutput, ContainerError> {
|
||||
let config = CreateExecOptions {
|
||||
cmd: Some(cmd.iter().map(|arg| String::from(*arg)).collect()),
|
||||
user: user.map(String::from),
|
||||
attach_stdout: Some(true),
|
||||
attach_stderr: Some(true),
|
||||
..Default::default()
|
||||
};
|
||||
|
||||
let created = self
|
||||
.request("create exec", self.docker.create_exec(container, config))
|
||||
.await?;
|
||||
|
||||
let started = self
|
||||
.request(
|
||||
"start exec",
|
||||
self.docker
|
||||
.start_exec(&created.id, None::<StartExecOptions>),
|
||||
)
|
||||
.await?;
|
||||
|
||||
let StartExecResults::Attached { mut output, .. } = started else {
|
||||
return Err(ContainerError::Unexpected(String::from(
|
||||
"the daemon detached an exec that was not requested as detached",
|
||||
)));
|
||||
};
|
||||
|
||||
let mut stdout = String::new();
|
||||
|
Herald
commented
[performance] La collecte de la sortie d'un exec accumule toute la sortie dans deux **[performance]** La collecte de la sortie d'un exec accumule toute la sortie dans deux `String` sans limite. La limitation à 32 KiB dans l'agent intervient seulement après le retour de cette fonction ; un `read_file` sur un gros fichier ou un `grep` très bavard peut donc consommer une quantité arbitraire de mémoire avant d'être tronqué. Il faut borner la sortie pendant la lecture, ou interrompre l'exec dès que la limite est atteinte.
|
||||
let mut stderr = String::new();
|
||||
|
||||
// Le timeout couvre l'exécution de la commande elle-même, pas seulement sa
|
||||
// mise en place.
|
||||
let collect = async {
|
||||
while let Some(message) = output.next().await {
|
||||
match message? {
|
||||
LogOutput::StdOut { message } | LogOutput::Console { message } => {
|
||||
stdout.push_str(&String::from_utf8_lossy(&message));
|
||||
}
|
||||
LogOutput::StdErr { message } => {
|
||||
stderr.push_str(&String::from_utf8_lossy(&message));
|
||||
}
|
||||
LogOutput::StdIn { .. } => {}
|
||||
}
|
||||
}
|
||||
|
||||
Ok::<_, BollardError>(())
|
||||
};
|
||||
|
||||
match tokio::time::timeout(timeout, collect).await {
|
||||
Ok(Ok(())) => {}
|
||||
Ok(Err(source)) => return Err(ContainerError::Request { source }),
|
||||
Err(_) => {
|
||||
// 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.
|
||||
let _ = self.stop_container(container).await;
|
||||
|
||||
return Err(ContainerError::Timeout {
|
||||
operation: format!("exec {}", cmd.join(" ")),
|
||||
timeout,
|
||||
});
|
||||
}
|
||||
}
|
||||
|
||||
// Le code de sortie n'est pas forcément publié au moment où le flux se ferme :
|
||||
// on laisse au daemon le temps de le renseigner, sans bloquer indéfiniment.
|
||||
// Sans cela, une commande réussie peut être rapportée en échec (code -1) et
|
||||
// l'outil renvoie une erreur au modèle à la place du contenu.
|
||||
let mut inspected = self
|
||||
.request("inspect exec", self.docker.inspect_exec(&created.id))
|
||||
.await?;
|
||||
|
||||
for _ in 0..EXIT_CODE_ATTEMPTS {
|
||||
if !inspected.running.unwrap_or(false) {
|
||||
break;
|
||||
}
|
||||
|
||||
tokio::time::sleep(EXIT_CODE_DELAY).await;
|
||||
|
||||
inspected = self
|
||||
.request("inspect exec", self.docker.inspect_exec(&created.id))
|
||||
.await?;
|
||||
}
|
||||
|
||||
Ok(ExecOutput {
|
||||
status: inspected.exit_code.unwrap_or(-1) as i32,
|
||||
stdout,
|
||||
stderr,
|
||||
})
|
||||
}
|
||||
|
||||
pub(crate) async fn stop_container(&self, name: &str) -> Result<(), ContainerError> {
|
||||
let options = StopContainerOptions {
|
||||
t: Some(1),
|
||||
..Default::default()
|
||||
};
|
||||
|
||||
self.request(
|
||||
"stop container",
|
||||
self.docker.stop_container(name, Some(options)),
|
||||
)
|
||||
.await?;
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
/// Supprime un container, ses volumes anonymes et le processus qui y tourne.
|
||||
pub(crate) async fn remove_container(&self, name: &str) -> Result<(), ContainerError> {
|
||||
let options = RemoveContainerOptions {
|
||||
force: true,
|
||||
v: true,
|
||||
..Default::default()
|
||||
};
|
||||
|
||||
self.request(
|
||||
"remove container",
|
||||
self.docker.remove_container(name, Some(options)),
|
||||
)
|
||||
.await?;
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
pub(crate) async fn remove_image(&self, tag: &str) -> Result<(), ContainerError> {
|
||||
let options = RemoveImageOptions {
|
||||
force: true,
|
||||
..Default::default()
|
||||
};
|
||||
|
||||
self.request(
|
||||
"remove image",
|
||||
self.docker.remove_image(tag, Some(options), None),
|
||||
)
|
||||
.await?;
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
/// Crée le réseau dédié d'une sandbox.
|
||||
pub(crate) async fn create_network(&self, name: &str) -> Result<(), ContainerError> {
|
||||
let request = NetworkCreateRequest {
|
||||
name: String::from(name),
|
||||
..Default::default()
|
||||
};
|
||||
|
||||
self.request("create network", self.docker.create_network(request))
|
||||
.await?;
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
/// Détache un container d'un réseau, ce qui coupe sa connectivité.
|
||||
pub(crate) async fn disconnect_network(
|
||||
&self,
|
||||
network: &str,
|
||||
container: &str,
|
||||
) -> Result<(), ContainerError> {
|
||||
let request = NetworkDisconnectRequest {
|
||||
container: String::from(container),
|
||||
..Default::default()
|
||||
};
|
||||
|
||||
self.request(
|
||||
"disconnect network",
|
||||
self.docker.disconnect_network(network, request),
|
||||
)
|
||||
.await?;
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
pub(crate) async fn remove_network(&self, name: &str) -> Result<(), ContainerError> {
|
||||
self.request("remove network", self.docker.remove_network(name))
|
||||
.await?;
|
||||
|
||||
Ok(())
|
||||
}
|
||||
}
|
||||
@@ -0,0 +1,230 @@
|
||||
//! Schéma du `devcontainer.json`, analyse du fichier et résolution des
|
||||
//! références `${localEnv:…}`.
|
||||
|
||||
use std::{
|
||||
collections::HashMap,
|
||||
path::{Path, PathBuf},
|
||||
};
|
||||
|
||||
use serde::Deserialize;
|
||||
|
||||
use crate::{devcontainer::DevContainer, errors::ParseError};
|
||||
|
||||
#[derive(Debug, Deserialize)]
|
||||
pub struct DevContainerBuildSchema {
|
||||
#[serde(default)]
|
||||
pub dockerfile: String,
|
||||
#[serde(default)]
|
||||
pub args: HashMap<String, String>,
|
||||
}
|
||||
|
||||
#[derive(Debug, Deserialize)]
|
||||
pub struct DevContainerSchema {
|
||||
#[serde(default)]
|
||||
pub name: Option<String>,
|
||||
pub build: DevContainerBuildSchema,
|
||||
|
Herald
commented
Le schéma ne prend en charge que Le schéma ne prend en charge que `dockerfile` et `args`, puis `DevContainer::build` utilise systématiquement le répertoire du Dockerfile comme contexte. La spécification `devcontainer.json` permet pourtant de définir `build.context` (souvent la racine du dépôt) ; les Dockerfile qui font `COPY . ...` ou qui utilisent un contexte distinct échoueront ou ne verront pas les fichiers attendus. Il faut modéliser et résoudre le contexte de build, avec une validation empêchant qu'il sorte du dépôt.
|
||||
|
||||
#[serde(rename = "workspaceFolder", default)]
|
||||
pub workspace_folder: Option<String>,
|
||||
|
||||
#[serde(rename = "containerEnv", default)]
|
||||
pub container_env: HashMap<String, String>,
|
||||
|
||||
#[serde(rename = "postCreateCommand", default)]
|
||||
pub post_create_command: Option<String>,
|
||||
|
||||
#[serde(rename = "postStartCommand", default)]
|
||||
pub post_start_command: Option<String>,
|
||||
|
||||
#[serde(rename = "remoteUser", default)]
|
||||
pub remote_user: Option<String>,
|
||||
|
||||
/// Arguments passés à `docker run`.
|
||||
///
|
||||
/// Lus pour rester fidèle au format `devcontainer.json`, mais
|
||||
/// **délibérément pas transmis** au runtime : ils viennent d'une pull
|
||||
/// request non fiable et pourraient casser l'isolation de la sandbox, décrite
|
||||
/// dans le doc de la crate.
|
||||
#[serde(rename = "runArgs", default)]
|
||||
pub run_args: Vec<String>,
|
||||
}
|
||||
|
||||
impl TryFrom<(DevContainerSchema, PathBuf)> for DevContainer {
|
||||
type Error = ParseError;
|
||||
|
||||
fn try_from(
|
||||
(schema, devcontainer_path): (DevContainerSchema, PathBuf),
|
||||
) -> Result<Self, Self::Error> {
|
||||
let base_dir = devcontainer_path
|
||||
.parent()
|
||||
.ok_or_else(|| ParseError::InvalidDevContainerPath(devcontainer_path.clone()))?;
|
||||
|
||||
let container_file_path = base_dir.join(schema.build.dockerfile);
|
||||
|
Herald
commented
[Bloquant] Le chemin du Dockerfile est construit directement à partir d'une valeur contrôlée par la pull request. Un chemin absolu ou contenant suffisamment de [Bloquant] Le chemin du Dockerfile est construit directement à partir d'une valeur contrôlée par la pull request. Un chemin absolu ou contenant suffisamment de `..` peut faire sortir le contexte de build du clone ; `build()` empaquettera alors le répertoire parent arbitraire et l'enverra au daemon, ce qui peut exposer des fichiers de l'hôte au build non fiable. Il faut rejeter les chemins absolus et vérifier lexicalement que le Dockerfile et son contexte restent sous la racine du dépôt cloné.
Herald
commented
Le chemin Le chemin `build.dockerfile` fourni par une pull request est concaténé sans normalisation ni vérification qu'il reste dans le dépôt. Une valeur comme `../../...` peut faire choisir un Dockerfile situé hors du clone ; comme son parent devient ensuite le contexte envoyé au daemon, cela peut empaqueter des fichiers d'autres sandboxes ou du système dans le contexte de build. Résolvez le chemin puis imposez qu'il soit situé sous la racine du dépôt avant de l'utiliser.
|
||||
|
||||
if !container_file_path.is_file() {
|
||||
return Err(ParseError::ContainerFileNotFound(container_file_path));
|
||||
}
|
||||
|
||||
let build_args = schema
|
||||
.build
|
||||
.args
|
||||
.into_iter()
|
||||
.map(|(k, v)| (k, substitute_local_env(&v)))
|
||||
.collect();
|
||||
|
||||
let container_env = schema
|
||||
.container_env
|
||||
.into_iter()
|
||||
.map(|(k, v)| (k, substitute_local_env(&v)))
|
||||
.collect();
|
||||
|
||||
Ok(Self {
|
||||
container_file_path,
|
||||
name: schema.name,
|
||||
build_args,
|
||||
container_env,
|
||||
workspace_folder: schema.workspace_folder,
|
||||
post_create_command: schema.post_create_command,
|
||||
post_start_command: schema.post_start_command,
|
||||
remote_user: schema.remote_user,
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
/// Résout les références `${localEnv:VAR}` et `${localEnv:VAR:default}` d'un
|
||||
/// `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 Herald 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.
|
||||
fn substitute_local_env(input: &str) -> String {
|
||||
const PREFIX: &str = "${localEnv:";
|
||||
|
||||
let mut out = String::with_capacity(input.len());
|
||||
let mut rest = input;
|
||||
|
||||
while let Some(start) = rest.find(PREFIX) {
|
||||
out.push_str(&rest[..start]);
|
||||
let after = &rest[start + PREFIX.len()..];
|
||||
|
||||
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.
|
||||
let (_key, default) = match inner.split_once(':') {
|
||||
Some((key, default)) => (key, Some(default)),
|
||||
None => (inner, None),
|
||||
};
|
||||
|
||||
out.push_str(default.unwrap_or(""));
|
||||
|
||||
rest = &after[end + 1..];
|
||||
}
|
||||
None => {
|
||||
out.push_str(PREFIX);
|
||||
rest = after;
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
out.push_str(rest);
|
||||
out
|
||||
}
|
||||
|
||||
pub async fn parse(path: impl AsRef<Path>) -> Result<DevContainer, ParseError> {
|
||||
let path = path.as_ref().to_path_buf();
|
||||
let contents = tokio::fs::read_to_string(&path)
|
||||
.await
|
||||
.map_err(|source| ParseError::Read {
|
||||
path: path.clone(),
|
||||
source,
|
||||
})?;
|
||||
|
||||
let schema = serde_json::from_str::<DevContainerSchema>(&contents).map_err(|source| {
|
||||
ParseError::Json {
|
||||
path: path.clone(),
|
||||
source,
|
||||
}
|
||||
})?;
|
||||
|
||||
DevContainer::try_from((schema, path))
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
mod tests {
|
||||
use super::*;
|
||||
use std::fs;
|
||||
|
||||
#[tokio::test]
|
||||
async fn parses_devcontainer_file() {
|
||||
let dir = tempfile::tempdir().unwrap();
|
||||
let devcontainer_path = dir.path().join("devcontainer.json");
|
||||
let dockerfile_path = dir.path().join("Dockerfile");
|
||||
|
||||
fs::write(&dockerfile_path, "FROM alpine\n").unwrap();
|
||||
fs::write(
|
||||
&devcontainer_path,
|
||||
r#"{
|
||||
"name": "test",
|
||||
"build": {
|
||||
"dockerfile": "Dockerfile",
|
||||
"args": {
|
||||
"VERSION": "1"
|
||||
}
|
||||
},
|
||||
"workspaceFolder": "/workspace",
|
||||
"containerEnv": {
|
||||
"RUST_LOG": "debug"
|
||||
},
|
||||
"remoteUser": "dev"
|
||||
}"#,
|
||||
)
|
||||
.unwrap();
|
||||
|
||||
let config = parse(&devcontainer_path).await.unwrap();
|
||||
|
||||
assert_eq!(config.name.as_deref(), Some("test"));
|
||||
assert_eq!(config.container_file_path, dockerfile_path);
|
||||
assert_eq!(config.build_args.get("VERSION").unwrap(), "1");
|
||||
assert_eq!(config.container_env.get("RUST_LOG").unwrap(), "debug");
|
||||
assert_eq!(config.workspace_folder.as_deref(), Some("/workspace"));
|
||||
assert_eq!(config.remote_user.as_deref(), Some("dev"));
|
||||
}
|
||||
|
||||
#[test]
|
||||
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 Herald y exposerait ses jetons.
|
||||
assert_eq!(
|
||||
substitute_local_env("${localEnv:DEVCONTAINER_TEST_SECRET}"),
|
||||
""
|
||||
);
|
||||
assert_eq!(
|
||||
substitute_local_env("token=${localEnv:DEVCONTAINER_TEST_SECRET}"),
|
||||
"token="
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn local_env_defaults_are_used() {
|
||||
assert_eq!(
|
||||
substitute_local_env("${localEnv:DEVCONTAINER_TEST_MISSING:fallback}"),
|
||||
"fallback"
|
||||
);
|
||||
assert_eq!(
|
||||
substitute_local_env("${localEnv:DEVCONTAINER_TEST_MISSING}"),
|
||||
""
|
||||
);
|
||||
assert_eq!(substitute_local_env("uid=${localEnv:UID:1000}"), "uid=1000");
|
||||
assert_eq!(
|
||||
substitute_local_env("no variables here"),
|
||||
"no variables here"
|
||||
);
|
||||
}
|
||||
}
|
||||
@@ -0,0 +1,28 @@
|
||||
[package]
|
||||
name = "herald-server"
|
||||
version = "1.2.0"
|
||||
edition = "2024"
|
||||
|
||||
[dependencies]
|
||||
reqwest = { workspace = true }
|
||||
tokio = { workspace = true }
|
||||
tokio-util = { workspace = true }
|
||||
futures-util = { workspace = true }
|
||||
serde_json = { workspace = true }
|
||||
serde = { workspace = true }
|
||||
sentry = { workspace = true }
|
||||
sentry-anyhow = { workspace = true }
|
||||
dotenvy = { workspace = true }
|
||||
tower = { workspace = true }
|
||||
tower-http = { workspace = true }
|
||||
tracing = { workspace = true }
|
||||
tracing-subscriber = { workspace = true }
|
||||
axum = { workspace = true }
|
||||
anyhow = { workspace = true }
|
||||
thiserror = { workspace = true }
|
||||
ring = { workspace = true }
|
||||
hex = { workspace = true }
|
||||
metrics = { workspace = true }
|
||||
metrics-exporter-prometheus = { workspace = true }
|
||||
devcontainer-rs = { path = "../devcontainer-rs" }
|
||||
tempfile = "3"
|
||||
|
qpismont marked this conversation as resolved
Herald
commented
`tempfile = "3"` est déclaré en dur alors que toutes les autres dépendances passent par `[workspace.dependencies]` : le placer dans le workspace pour garder une version unique et cohérente (idem pour le dev-dependency de `devcontainer-rs`).
|
||||
@@ -1,7 +1,10 @@
|
||||
use crate::{
|
||||
gitea::{GiteaAPI, WebhookType}, metrics, open_router::OpenRouterClient,
|
||||
gitea::{GiteaAPI, WebhookType},
|
||||
|
qpismont marked this conversation as resolved
Herald
commented
Adaptation aux nouveaux chemins de crates : profiter du déplacement pour vérifier que les imports internes utilisent bien Adaptation aux nouveaux chemins de crates : profiter du déplacement pour vérifier que les imports internes utilisent bien `crate::` (et non des chemins relatifs fragiles) et qu'aucun `use` mort ne subsiste après la réorganisation.
|
||||
metrics,
|
||||
open_router::OpenRouterClient,
|
||||
sandbox::SandboxConfig,
|
||||
};
|
||||
use serde::Deserialize;
|
||||
use serde::{Deserialize, Deserializer};
|
||||
use std::{collections::HashSet, sync::Arc};
|
||||
use tokio::sync::Mutex;
|
||||
use tokio_util::sync::CancellationToken;
|
||||
@@ -18,18 +21,111 @@ pub struct ReviewResult {
|
||||
pub struct ReviewItem {
|
||||
pub filename: String,
|
||||
pub line: Option<u64>,
|
||||
pub code: String,
|
||||
#[serde(default, deserialize_with = "deserialize_side")]
|
||||
pub side: Option<ReviewSide>,
|
||||
#[serde(default, deserialize_with = "deserialize_severity")]
|
||||
pub severity: Option<ReviewSeverity>,
|
||||
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<Self> {
|
||||
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<Self> {
|
||||
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<Option<ReviewSide>, D::Error>
|
||||
where
|
||||
D: Deserializer<'de>,
|
||||
{
|
||||
let raw = Option::<serde_json::Value>::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<Option<ReviewSeverity>, D::Error>
|
||||
where
|
||||
D: Deserializer<'de>,
|
||||
{
|
||||
let raw = Option::<serde_json::Value>::deserialize(deserializer)?;
|
||||
|
||||
Ok(raw
|
||||
.as_ref()
|
||||
.and_then(serde_json::Value::as_str)
|
||||
.and_then(ReviewSeverity::parse))
|
||||
}
|
||||
|
||||
#[derive(Clone)]
|
||||
pub struct Bot {
|
||||
bot_name: String,
|
||||
gitea_api: GiteaAPI,
|
||||
open_router_client: OpenRouterClient,
|
||||
http_client: reqwest::Client,
|
||||
max_concurrent: usize,
|
||||
open_router_model: String,
|
||||
sandbox: SandboxConfig,
|
||||
actions_handled: Arc<Mutex<HashSet<u64>>>,
|
||||
}
|
||||
|
||||
@@ -38,17 +134,17 @@ impl Bot {
|
||||
bot_name: String,
|
||||
gitea_api: GiteaAPI,
|
||||
open_router_client: OpenRouterClient,
|
||||
http_client: reqwest::Client,
|
||||
max_concurrent: usize,
|
||||
open_router_model: String,
|
||||
sandbox: SandboxConfig,
|
||||
) -> Self {
|
||||
Self {
|
||||
bot_name,
|
||||
gitea_api,
|
||||
open_router_client,
|
||||
http_client,
|
||||
max_concurrent,
|
||||
open_router_model,
|
||||
sandbox,
|
||||
actions_handled: Arc::new(Mutex::new(HashSet::new())),
|
||||
}
|
||||
}
|
||||
@@ -112,12 +208,15 @@ impl Bot {
|
||||
}
|
||||
};
|
||||
|
||||
let tools = crate::sandbox::tools::for_webhook(&webhook);
|
||||
|
||||
let exec_result = match webhook {
|
||||
WebhookType::Review(review_payload) => crate::bot_actions::review::exec_review(
|
||||
&self.gitea_api,
|
||||
&self.open_router_client,
|
||||
&self.http_client,
|
||||
&self.open_router_model,
|
||||
&self.sandbox,
|
||||
tools,
|
||||
review_payload,
|
||||
),
|
||||
}
|
||||
@@ -0,0 +1,82 @@
|
||||
pub const GITEA_SIG_HEADER_NAME: &str = "x-gitea-signature";
|
||||
|
qpismont marked this conversation as resolved
Herald
commented
Centraliser les valeurs magiques dans un module de constantes est une bonne pratique : s'assurer que les valeurs liées au sandbox (timeouts, limites mémoire, chemins) sont bien documentées par un commentaire indiquant l'unité (secondes, octets) et la raison du choix. Centraliser les valeurs magiques dans un module de constantes est une bonne pratique : s'assurer que les valeurs liées au sandbox (timeouts, limites mémoire, chemins) sont bien documentées par un commentaire indiquant l'unité (secondes, octets) et la raison du choix.
|
||||
pub const GITEA_EVENT_TYPE_HEADER_NAME: &str = "x-gitea-event-type";
|
||||
|
||||
pub const MAX_WEBHOOK_BODY_SIZE: usize = 1024 * 1024; // 1 MiB
|
||||
pub const MAX_DIFF_SIZE: usize = 1024 * 1024; // 1 MiB
|
||||
|
||||
pub const BOT_PROCESS_MSG: &str = "
|
||||
Review in progress with the model \"{model}\"...
|
||||
";
|
||||
|
||||
pub const SANDBOX_SYSTEM_PROMPT: &str = "
|
||||
You are a senior software engineer reviewing a pull request inside an
|
||||
isolated sandbox.
|
||||
|
||||
The repository is checked out in your working directory. Explore it with the
|
||||
provided read-only tools (ls, file_size, read_file, grep, find); paths are
|
||||
relative to the repository root. Every line you read is prefixed with its
|
||||
absolute line number: that is the number you must cite to anchor a comment.
|
||||
|
||||
Read before you assert: a review anchored on a line you did not read is
|
||||
worthless, so never comment on code you have not seen. Gather enough context
|
||||
to be confident, but do not re-read what you already have.
|
||||
|
||||
When you answer, send the requested JSON only: a raw JSON object, no markdown
|
||||
code fence, nothing before or after it.
|
||||
";
|
||||
|
||||
pub const REVIEW_PROMPT: &str = "
|
||||
You are a senior software engineer reviewing a pull request.
|
||||
|
||||
Judge the changes for correctness, security, resource use and
|
||||
maintainability. Report real problems; do not invent issues, and do not
|
||||
report pure formatting a formatter would fix.
|
||||
|
||||
Be exhaustive: one review per distinct issue, and cover every changed file
|
||||
that has something to report. Do not stop at the first few findings, and do
|
||||
not merge several issues into a single review.
|
||||
|
||||
This is the pull request subject: \"{subject}\"
|
||||
|
||||
This is the user comment: \"{comment}\"
|
||||
If the user asks about something precise, address that first.
|
||||
|
||||
The pull request changes these files and lines:
|
||||
|
||||
{changes}
|
||||
|
||||
The code is not provided. Read the files you need with the available tools
|
||||
before answering.
|
||||
|
||||
`added` line numbers refer to the new version of the file, which is what your
|
||||
working directory contains now. `removed` line numbers refer to the old
|
||||
version, which is not in your working directory: you cannot read a removed
|
||||
line, only the code around where it used to be.
|
||||
|
||||
Every review must anchor on one of the listed line numbers:
|
||||
- filename: the full path exactly as listed above,
|
||||
- line: one of the numbers listed for that file,
|
||||
- side: \"added\" for a number from the `added` list, \"removed\" for one from
|
||||
the `removed` list,
|
||||
- severity: exactly one of \"bug\", \"security\", \"performance\" or
|
||||
\"maintainability\" — a bug is wrong behaviour, security a vulnerability,
|
||||
performance a resource problem, maintainability everything else.
|
||||
|
||||
Answer in french, with the raw json object only: no markdown code fence, no
|
||||
text before or after it. All fields are mandatory. The `comment` field must
|
||||
hold a short summary that lists the issues, and also what the pull request
|
||||
does well: the author should get the compliments too.
|
||||
|
||||
{
|
||||
\"reviews\": [
|
||||
{
|
||||
\"filename\": \"\",
|
||||
\"line\": 0,
|
||||
\"side\": \"\",
|
||||
\"severity\": \"\",
|
||||
\"message\": \"\"
|
||||
}
|
||||
],
|
||||
\"comment\": \"\"
|
||||
}
|
||||
";
|
||||
@@ -12,6 +12,7 @@ pub struct EnvConfig {
|
||||
pub gitea_token: String,
|
||||
pub gitea_timeout: u64,
|
||||
pub metrics_bind_addr: Option<String>,
|
||||
pub sandbox_max_iterations: usize,
|
||||
|
qpismont marked this conversation as resolved
Herald
commented
Nouvelles variables d'environnement : valider leur présence et leur format au démarrage (échouer tôt avec un message explicite) plutôt que de laisser une valeur par défaut silencieuse. Prévoir aussi une valeur par défaut sûre (fail-closed) pour tout réglage désactivant l'isolation. Nouvelles variables d'environnement : valider leur présence et leur format au démarrage (échouer tôt avec un message explicite) plutôt que de laisser une valeur par défaut silencieuse. Prévoir aussi une valeur par défaut sûre (fail-closed) pour tout réglage désactivant l'isolation.
Herald
commented
Nouvelles variables d'environnement : documenter chacune dans Nouvelles variables d'environnement : documenter chacune dans `.env.example` et fournir une valeur par défaut sûre ; une variable manquante ne devrait pas désactiver silencieusement le sandboxing.
|
||||
}
|
||||
|
||||
pub fn load_config() -> anyhow::Result<EnvConfig> {
|
||||
@@ -25,6 +26,10 @@ pub fn load_config() -> anyhow::Result<EnvConfig> {
|
||||
let gitea_token = try_get_env("GITEA_TOKEN")?;
|
||||
let gitea_timeout = try_get_env("GITEA_TIMEOUT")?.parse()?;
|
||||
let metrics_bind_addr = std::env::var("METRICS_BIND_ADDR").ok();
|
||||
let sandbox_max_iterations = std::env::var("SANDBOX_MAX_ITERATIONS")
|
||||
.ok()
|
||||
.and_then(|value| value.parse().ok())
|
||||
.unwrap_or(8);
|
||||
|
qpismont marked this conversation as resolved
Herald
commented
`SANDBOX_MAX_ITERATIONS` retombe silencieusement sur 8 si la valeur est invalide et accepte `0` : avec 0, la boucle de l'agent (`1..=max_iterations`) ne s'exécute pas et la revue échoue immédiatement. Valider la valeur (>= 1) et journaliser en cas de repli sur la valeur par défaut.
|
||||
|
||||
Ok(EnvConfig {
|
||||
http_port,
|
||||
@@ -37,6 +42,7 @@ pub fn load_config() -> anyhow::Result<EnvConfig> {
|
||||
gitea_token,
|
||||
gitea_timeout,
|
||||
metrics_bind_addr,
|
||||
sandbox_max_iterations,
|
||||
})
|
||||
}
|
||||
|
||||
@@ -1,14 +1,28 @@
|
||||
use std::time::Duration;
|
||||
|
||||
use futures_util::stream::TryStreamExt;
|
||||
use serde::Deserialize;
|
||||
use serde_json::{Value, json};
|
||||
use tracing::instrument;
|
||||
use tokio::io::AsyncReadExt;
|
||||
use tokio_util::io::StreamReader;
|
||||
use tracing::{instrument, warn};
|
||||
|
||||
use crate::{bot::ReviewResult, errors::AppError};
|
||||
use crate::{
|
||||
bot::{ReviewResult, ReviewSeverity, ReviewSide},
|
||||
consts::MAX_DIFF_SIZE,
|
||||
errors::AppError,
|
||||
};
|
||||
|
||||
/// Page size requested when listing the files of a pull request.
|
||||
const FILE_PAGE_SIZE: u64 = 50;
|
||||
|
||||
/// Maximum number of pages fetched for a pull request file list.
|
||||
const MAX_FILE_PAGES: u64 = 10;
|
||||
|
||||
#[derive(Clone)]
|
||||
pub struct GiteaAPI {
|
||||
base_url: String,
|
||||
token: String,
|
||||
client: reqwest::Client,
|
||||
}
|
||||
|
||||
@@ -22,6 +36,7 @@ impl GiteaAPI {
|
||||
|
||||
Ok(Self {
|
||||
base_url: String::from(base_url),
|
||||
token: String::from(token),
|
||||
client: reqwest::Client::builder()
|
||||
.timeout(Duration::from_secs(timeout))
|
||||
.default_headers(default_headers)
|
||||
@@ -29,6 +44,11 @@ impl GiteaAPI {
|
||||
})
|
||||
}
|
||||
|
||||
/// API token used to authenticate against Gitea.
|
||||
pub fn token(&self) -> &str {
|
||||
&self.token
|
||||
}
|
||||
|
||||
#[instrument(skip(self))]
|
||||
pub async fn get_authorized_user(&self) -> anyhow::Result<User> {
|
||||
let url = format!("{}/api/v1/user", self.base_url);
|
||||
@@ -119,6 +139,70 @@ impl GiteaAPI {
|
||||
Ok(())
|
||||
}
|
||||
|
||||
/// Raw unified diff of a pull request.
|
||||
///
|
||||
/// The API endpoint is used rather than the `diff_url` carried by the
|
||||
/// webhook: that one points at a web route, which is session authenticated
|
||||
/// and therefore does not serve private repositories to an API token.
|
||||
#[instrument(skip(self))]
|
||||
pub async fn pull_request_diff(&self, full_name: &str, index: u64) -> anyhow::Result<String> {
|
||||
let url = format!(
|
||||
"{}/api/v1/repos/{}/pulls/{}.diff",
|
||||
self.base_url, full_name, index
|
||||
);
|
||||
|
||||
let res = self.client.get(url).send().await?;
|
||||
|
||||
if !res.status().is_success() {
|
||||
return Err(anyhow::anyhow!(
|
||||
"Failed to download pull request diff: {}",
|
||||
res.status()
|
||||
));
|
||||
}
|
||||
|
||||
read_capped(res).await
|
||||
}
|
||||
|
||||
/// Files changed by a pull request, with their exact path and status.
|
||||
#[instrument(skip(self))]
|
||||
pub async fn pull_request_files(
|
||||
&self,
|
||||
full_name: &str,
|
||||
index: u64,
|
||||
) -> anyhow::Result<Vec<PullRequestFile>> {
|
||||
let mut files = Vec::new();
|
||||
|
||||
for page in 1..=MAX_FILE_PAGES {
|
||||
let url = format!(
|
||||
"{}/api/v1/repos/{}/pulls/{}/files?limit={FILE_PAGE_SIZE}&page={page}",
|
||||
self.base_url, full_name, index
|
||||
);
|
||||
|
||||
let res = self.client.get(url).send().await?;
|
||||
|
||||
if !res.status().is_success() {
|
||||
return Err(anyhow::anyhow!(
|
||||
"Failed to list pull request files: {}",
|
||||
res.status()
|
||||
));
|
||||
}
|
||||
|
||||
let page_files = res.json::<Vec<PullRequestFile>>().await?;
|
||||
|
||||
// The instance may cap the page size below the requested one, so a
|
||||
// short page is not the end of the list: an empty one is.
|
||||
if page_files.is_empty() {
|
||||
return Ok(files);
|
||||
}
|
||||
|
||||
files.extend(page_files);
|
||||
}
|
||||
|
||||
warn!(files = files.len(), "Pull request file list was truncated");
|
||||
|
||||
Ok(files)
|
||||
}
|
||||
|
||||
#[instrument(skip(self, review_result))]
|
||||
pub async fn post_pull_request_review(
|
||||
&self,
|
||||
@@ -132,19 +216,31 @@ impl GiteaAPI {
|
||||
self.base_url, full_name, index
|
||||
);
|
||||
|
||||
let comments = &review_result
|
||||
let comments = review_result
|
||||
.reviews
|
||||
.iter()
|
||||
.filter(|r| r.line.is_some())
|
||||
.map(|r| {
|
||||
let path = r.filename.clone();
|
||||
let line = r.line.unwrap_or(0);
|
||||
let body = r.message.clone();
|
||||
.filter_map(|review| {
|
||||
let line = review.line?;
|
||||
let path = review.filename.clone();
|
||||
let severity = review
|
||||
.severity
|
||||
.unwrap_or(ReviewSeverity::Maintainability)
|
||||
.label();
|
||||
let body = format!("**[{severity}]** {}", review.message);
|
||||
|
||||
json!({
|
||||
"path": path,
|
||||
"new_position": line,
|
||||
"body": body
|
||||
// A line removed by the pull request only exists in the old
|
||||
// version of the file, so it is anchored with `old_position`.
|
||||
Some(match review.side {
|
||||
Some(ReviewSide::Removed) => json!({
|
||||
"path": path,
|
||||
"old_position": line,
|
||||
"body": body
|
||||
}),
|
||||
_ => json!({
|
||||
"path": path,
|
||||
"new_position": line,
|
||||
"body": body
|
||||
}),
|
||||
})
|
||||
})
|
||||
.collect::<Vec<_>>();
|
||||
@@ -168,6 +264,23 @@ impl GiteaAPI {
|
||||
}
|
||||
}
|
||||
|
||||
/// Reads a response body, refusing to buffer more than [`MAX_DIFF_SIZE`].
|
||||
async fn read_capped(response: reqwest::Response) -> anyhow::Result<String> {
|
||||
let stream = response.bytes_stream().map_err(std::io::Error::other);
|
||||
|
||||
let mut buf = Vec::with_capacity(MAX_DIFF_SIZE);
|
||||
StreamReader::new(stream)
|
||||
.take((MAX_DIFF_SIZE + 1) as u64)
|
||||
.read_to_end(&mut buf)
|
||||
.await?;
|
||||
|
||||
if buf.len() > MAX_DIFF_SIZE {
|
||||
anyhow::bail!("Pull request diff exceeds the maximum allowed size of 1 MiB");
|
||||
}
|
||||
|
||||
Ok(String::from_utf8_lossy(&buf).into_owned())
|
||||
}
|
||||
|
||||
#[derive(Debug)]
|
||||
pub enum WebhookType {
|
||||
Review(ReviewPayload),
|
||||
@@ -197,8 +310,6 @@ pub struct ReviewPayload {
|
||||
|
||||
#[derive(Deserialize, Debug)]
|
||||
pub struct PullRequest {
|
||||
pub id: u64,
|
||||
pub diff_url: String,
|
||||
pub number: u64,
|
||||
pub title: String,
|
||||
}
|
||||
@@ -207,18 +318,29 @@ pub struct PullRequest {
|
||||
pub struct Comment {
|
||||
pub id: u64,
|
||||
pub body: String,
|
||||
pub user: User,
|
||||
}
|
||||
|
||||
#[derive(Deserialize, Debug)]
|
||||
pub struct User {
|
||||
pub id: u64,
|
||||
pub login: String,
|
||||
}
|
||||
|
||||
#[derive(Deserialize, Debug)]
|
||||
pub struct Repository {
|
||||
pub full_name: String,
|
||||
pub clone_url: String,
|
||||
}
|
||||
|
||||
/// A file changed by a pull request, as reported by the API.
|
||||
#[derive(Deserialize, Debug)]
|
||||
pub struct PullRequestFile {
|
||||
/// Path of the file in the new version of the repository.
|
||||
pub filename: String,
|
||||
/// Previous path, for a renamed file.
|
||||
#[serde(default)]
|
||||
pub previous_filename: Option<String>,
|
||||
/// `added`, `modified`, `deleted`, `renamed`…
|
||||
pub status: String,
|
||||
}
|
||||
|
||||
impl WebhookType {
|
||||
@@ -264,7 +386,8 @@ mod tests {
|
||||
"title": "My PR"
|
||||
},
|
||||
"repository": {
|
||||
"full_name": "owner/repo"
|
||||
"full_name": "owner/repo",
|
||||
"clone_url": "https://github.com/owner/repo.git"
|
||||
},
|
||||
"comment": {
|
||||
"id": 7,
|
||||
@@ -282,10 +405,8 @@ mod tests {
|
||||
match result.unwrap() {
|
||||
WebhookType::Review(payload) => {
|
||||
assert_eq!(payload.action, "created");
|
||||
assert_eq!(payload.pull_request.id, 42);
|
||||
assert_eq!(payload.comment.id, 7);
|
||||
assert_eq!(payload.comment.body, "@test_bot LGTM");
|
||||
assert_eq!(payload.comment.user.id, 100);
|
||||
}
|
||||
}
|
||||
}
|
||||
@@ -329,7 +450,8 @@ mod tests {
|
||||
"title": "My PR"
|
||||
},
|
||||
"repository": {
|
||||
"full_name": "owner/repo"
|
||||
"full_name": "owner/repo",
|
||||
"clone_url": "https://github.com/owner/repo.git"
|
||||
},
|
||||
"comment": {
|
||||
"id": 1,
|
||||
@@ -361,7 +483,8 @@ mod tests {
|
||||
"title": "My PR"
|
||||
},
|
||||
"repository": {
|
||||
"full_name": "owner/repo"
|
||||
"full_name": "owner/repo",
|
||||
"clone_url": "https://github.com/owner/repo.git"
|
||||
},
|
||||
"comment": {
|
||||
"id": 12,
|
||||
@@ -375,10 +498,12 @@ mod tests {
|
||||
|
||||
let payload: ReviewPayload = serde_json::from_value(json).unwrap();
|
||||
assert_eq!(payload.action, "created");
|
||||
assert_eq!(payload.pull_request.id, 99);
|
||||
assert_eq!(payload.comment.id, 12);
|
||||
assert_eq!(payload.comment.body, "Needs work");
|
||||
assert_eq!(payload.comment.user.id, 200);
|
||||
assert_eq!(
|
||||
payload.repository.clone_url,
|
||||
"https://github.com/owner/repo.git"
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
@@ -399,7 +524,8 @@ mod tests {
|
||||
"title": "My PR"
|
||||
},
|
||||
"repository": {
|
||||
"full_name": "owner/repo"
|
||||
"full_name": "owner/repo",
|
||||
"clone_url": "https://github.com/owner/repo.git"
|
||||
},
|
||||
"comment": {
|
||||
"id": 1,
|
||||
@@ -426,7 +552,8 @@ mod tests {
|
||||
"title": "My PR"
|
||||
},
|
||||
"repository": {
|
||||
"full_name": "owner/repo"
|
||||
"full_name": "owner/repo",
|
||||
"clone_url": "https://github.com/owner/repo.git"
|
||||
},
|
||||
"comment": {
|
||||
"id": 1,
|
||||
@@ -1,4 +1,12 @@
|
||||
use crate::{bot::Bot, gitea::{GiteaAPI, WebhookType}, open_router::OpenRouterClient, state::AppState};
|
||||
use std::time::Duration;
|
||||
|
||||
use crate::{
|
||||
bot::Bot,
|
||||
gitea::{GiteaAPI, WebhookType},
|
||||
open_router::OpenRouterClient,
|
||||
sandbox::SandboxConfig,
|
||||
state::AppState,
|
||||
};
|
||||
|
||||
use dotenvy::dotenv;
|
||||
use tokio::signal::unix::{SignalKind, signal};
|
||||
@@ -15,7 +23,15 @@ mod errors;
|
||||
mod gitea;
|
||||
mod metrics;
|
||||
mod open_router;
|
||||
mod sandbox;
|
||||
mod state;
|
||||
mod text;
|
||||
|
||||
/// Délai laissé aux reviews en cours et aux serveurs pour s'arrêter proprement.
|
||||
///
|
||||
/// Sans lui, une review prise dans une sandbox récalcitrante garderait le processus
|
||||
/// en vie jusqu'à ce que le superviseur le tue.
|
||||
const SHUTDOWN_TIMEOUT: Duration = Duration::from_secs(60);
|
||||
|
||||
fn main() -> anyhow::Result<()> {
|
||||
dotenv().ok();
|
||||
@@ -31,14 +47,12 @@ fn main() -> anyhow::Result<()> {
|
||||
let _sentry_guard = if let Ok(sentry_dsn) = env::try_get_env("SENTRY_DSN") {
|
||||
info!("Initialize sentry");
|
||||
|
||||
Some(sentry::init((
|
||||
sentry_dsn,
|
||||
sentry::ClientOptions {
|
||||
release: sentry::release_name!(),
|
||||
send_default_pii: true,
|
||||
..Default::default()
|
||||
},
|
||||
)))
|
||||
Some(sentry::init(
|
||||
sentry::ClientOptions::new()
|
||||
.dsn(&sentry_dsn)
|
||||
.maybe_release(sentry::release_name!())
|
||||
.send_default_pii(true),
|
||||
))
|
||||
} else {
|
||||
warn!("SENTRY_DSN not set, sentry will not be initialized");
|
||||
None
|
||||
@@ -73,13 +87,25 @@ async fn run() -> anyhow::Result<()> {
|
||||
|
||||
let shutdown = CancellationToken::new();
|
||||
|
||||
let sandbox = SandboxConfig {
|
||||
runtime: devcontainer_rs::ContainerRuntime::connect()?,
|
||||
max_iterations: config.sandbox_max_iterations,
|
||||
};
|
||||
|
||||
if !sandbox.runtime.available().await {
|
||||
warn!(
|
||||
endpoint = sandbox.runtime.endpoint(),
|
||||
"Container daemon is not reachable, every review will fail"
|
||||
);
|
||||
}
|
||||
|
||||
let bot = Bot::new(
|
||||
gitea_user.login,
|
||||
gitea_api,
|
||||
open_router_client,
|
||||
reqwest::Client::new(),
|
||||
config.bot_max_concurrent,
|
||||
config.open_router_model.clone(),
|
||||
sandbox,
|
||||
);
|
||||
|
||||
let (tx, rx) = tokio::sync::mpsc::channel::<WebhookType>(config.bot_max_concurrent * 2);
|
||||
@@ -102,11 +128,28 @@ async fn run() -> anyhow::Result<()> {
|
||||
anyhow::Ok(())
|
||||
};
|
||||
|
||||
tokio::try_join!(
|
||||
bot.start(rx, shutdown.clone()),
|
||||
api::start(app_state, shutdown.clone()),
|
||||
signal
|
||||
)?;
|
||||
let shutdown_deadline = async {
|
||||
shutdown.cancelled().await;
|
||||
tokio::time::sleep(SHUTDOWN_TIMEOUT).await;
|
||||
};
|
||||
|
||||
tokio::select! {
|
||||
result = async {
|
||||
tokio::try_join!(
|
||||
bot.start(rx, shutdown.clone()),
|
||||
api::start(app_state, shutdown.clone()),
|
||||
signal
|
||||
)
|
||||
} => {
|
||||
result?;
|
||||
}
|
||||
() = shutdown_deadline => {
|
||||
warn!(
|
||||
timeout = ?SHUTDOWN_TIMEOUT,
|
||||
"Shutdown did not finish in time, exiting anyway"
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
info!("Shutdown complete");
|
||||
|
||||
@@ -1,10 +1,9 @@
|
||||
use std::{net::SocketAddr, str::FromStr};
|
||||
|
||||
use metrics::{Unit, describe_counter, describe_gauge, counter, gauge};
|
||||
use metrics::{Unit, counter, describe_counter, describe_gauge, gauge};
|
||||
|
||||
pub fn webhook_received(event_type: &str) {
|
||||
counter!("herald_webhooks_received_total", "event_type" => event_type.to_string())
|
||||
.increment(1);
|
||||
counter!("herald_webhooks_received_total", "event_type" => event_type.to_string()).increment(1);
|
||||
}
|
||||
|
||||
pub fn webhook_duplicate(event_type: &str) {
|
||||
@@ -31,8 +30,7 @@ pub fn task_completed(event_type: &str) {
|
||||
}
|
||||
|
||||
pub fn task_failed(event_type: &str) {
|
||||
counter!("herald_bot_tasks_failed_total", "event_type" => event_type.to_string())
|
||||
.increment(1);
|
||||
counter!("herald_bot_tasks_failed_total", "event_type" => event_type.to_string()).increment(1);
|
||||
}
|
||||
|
||||
pub fn openrouter_cost_usd(cost: f64) {
|
||||
@@ -40,13 +38,41 @@ pub fn openrouter_cost_usd(cost: f64) {
|
||||
}
|
||||
|
||||
pub fn describe() {
|
||||
describe_counter!("herald_webhooks_received_total", Unit::Count, "Total webhooks received");
|
||||
describe_counter!("herald_webhooks_duplicate_total", Unit::Count, "Webhooks rejected as duplicates");
|
||||
describe_counter!("herald_webhooks_channel_full_total", Unit::Count, "Webhooks dropped because the bot channel was full");
|
||||
describe_gauge!("herald_bot_tasks_active", Unit::Count, "Bot tasks currently in progress");
|
||||
describe_counter!("herald_bot_tasks_completed_total", Unit::Count, "Bot tasks completed successfully");
|
||||
describe_counter!("herald_bot_tasks_failed_total", Unit::Count, "Bot tasks that failed");
|
||||
describe_counter!("herald_openrouter_cost_cents_total", Unit::Count, "Total OpenRouter cost in cents (divide by 100 for USD)");
|
||||
describe_counter!(
|
||||
"herald_webhooks_received_total",
|
||||
Unit::Count,
|
||||
"Total webhooks received"
|
||||
);
|
||||
describe_counter!(
|
||||
"herald_webhooks_duplicate_total",
|
||||
Unit::Count,
|
||||
"Webhooks rejected as duplicates"
|
||||
);
|
||||
describe_counter!(
|
||||
"herald_webhooks_channel_full_total",
|
||||
Unit::Count,
|
||||
"Webhooks dropped because the bot channel was full"
|
||||
);
|
||||
describe_gauge!(
|
||||
"herald_bot_tasks_active",
|
||||
Unit::Count,
|
||||
"Bot tasks currently in progress"
|
||||
);
|
||||
describe_counter!(
|
||||
"herald_bot_tasks_completed_total",
|
||||
Unit::Count,
|
||||
"Bot tasks completed successfully"
|
||||
);
|
||||
describe_counter!(
|
||||
"herald_bot_tasks_failed_total",
|
||||
Unit::Count,
|
||||
"Bot tasks that failed"
|
||||
);
|
||||
describe_counter!(
|
||||
"herald_openrouter_cost_cents_total",
|
||||
Unit::Count,
|
||||
"Total OpenRouter cost in cents (divide by 100 for USD)"
|
||||
);
|
||||
}
|
||||
|
||||
pub fn install(bind_addr: &str) -> anyhow::Result<()> {
|
||||
@@ -0,0 +1,473 @@
|
||||
//! Minimal OpenRouter chat-completions client.
|
||||
|
qpismont marked this conversation as resolved
Herald
commented
Module OpenRouter : la clé d'API doit être lue depuis l'environnement/state (jamais codée en dur, jamais loggée). Vérifier que les erreurs renvoyées par l'API HTTP ne propagent pas la clé ni le contenu complet des prompts dans les logs ou dans le commentaire publié. Ajouter un timeout et une gestion des codes 429/5xx avec retry borné. Module OpenRouter : la clé d'API doit être lue depuis l'environnement/state (jamais codée en dur, jamais loggée). Vérifier que les erreurs renvoyées par l'API HTTP ne propagent pas la clé ni le contenu complet des prompts dans les logs ou dans le commentaire publié. Ajouter un timeout et une gestion des codes 429/5xx avec retry borné.
Herald
commented
Fichier déplacé depuis Fichier déplacé depuis `src/open_router.rs` : s'assurer qu'il n'y a pas de duplication avec l'ancien fichier supprimé et que les nouveaux appels (utilisés par l'agent de sandbox) gèrent les erreurs HTTP et les timeouts.
|
||||
//!
|
||||
//! Herald only needs a non-streaming `POST /chat/completions` with optional
|
||||
//! tool calling, so the wire types are implemented in-tree instead of pulling a
|
||||
//! third-party SDK (and its own `reqwest` version) into the workspace.
|
||||
//!
|
||||
//! Only the response fields Herald consumes are modelled: `content`,
|
||||
//! `tool_calls` and `usage.cost`. Unknown fields are ignored.
|
||||
|
||||
use std::time::Duration;
|
||||
|
||||
use anyhow::Context;
|
||||
use serde::{Deserialize, Serialize};
|
||||
use serde_json::Value;
|
||||
use tracing::instrument;
|
||||
|
||||
/// OpenRouter API root, version prefix included.
|
||||
const BASE_URL: &str = "https://openrouter.ai/api/v1";
|
||||
|
||||
/// What the model is allowed to do on a given turn.
|
||||
#[derive(Debug, Clone, Copy, PartialEq, Eq)]
|
||||
pub enum ToolChoice {
|
||||
/// The model decides whether to call a tool.
|
||||
Auto,
|
||||
/// The model must answer, without calling any tool. The tools stay declared,
|
||||
/// so the conversation remains the same shape as on the other turns.
|
||||
None,
|
||||
}
|
||||
|
||||
impl ToolChoice {
|
||||
fn as_str(self) -> &'static str {
|
||||
match self {
|
||||
Self::Auto => "auto",
|
||||
Self::None => "none",
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
/// Result of a completion that may contain tool calls.
|
||||
pub struct ToolChatResult {
|
||||
pub message: Option<String>,
|
||||
pub tool_calls: Vec<ToolCall>,
|
||||
pub cost: Option<f64>,
|
||||
}
|
||||
|
||||
#[derive(Serialize, Deserialize, Debug, Clone, Copy, PartialEq, Eq)]
|
||||
#[serde(rename_all = "lowercase")]
|
||||
pub enum Role {
|
||||
System,
|
||||
User,
|
||||
Assistant,
|
||||
Tool,
|
||||
}
|
||||
|
||||
/// A single turn of the conversation.
|
||||
#[derive(Serialize, Deserialize, Debug, Clone)]
|
||||
pub struct Message {
|
||||
pub role: Role,
|
||||
#[serde(skip_serializing_if = "Option::is_none")]
|
||||
pub content: Option<String>,
|
||||
#[serde(skip_serializing_if = "Option::is_none")]
|
||||
pub tool_calls: Option<Vec<ToolCall>>,
|
||||
#[serde(skip_serializing_if = "Option::is_none")]
|
||||
pub tool_call_id: Option<String>,
|
||||
}
|
||||
|
||||
impl Message {
|
||||
pub fn new(role: Role, content: impl Into<String>) -> Self {
|
||||
Self {
|
||||
role,
|
||||
content: Some(content.into()),
|
||||
tool_calls: None,
|
||||
tool_call_id: None,
|
||||
}
|
||||
}
|
||||
|
||||
/// Assistant turn requesting tool calls. Models often answer with tool
|
||||
/// calls but no text, in which case the content is sent as `null`.
|
||||
pub fn assistant_with_tool_calls(
|
||||
content: impl Into<String>,
|
||||
tool_calls: Vec<ToolCall>,
|
||||
) -> Self {
|
||||
let content = content.into();
|
||||
|
||||
Self {
|
||||
role: Role::Assistant,
|
||||
content: (!content.is_empty()).then_some(content),
|
||||
tool_calls: Some(tool_calls),
|
||||
tool_call_id: None,
|
||||
}
|
||||
}
|
||||
|
||||
/// Result of a tool call, linked to the request by `tool_call_id`.
|
||||
pub fn tool_response(tool_call_id: impl Into<String>, content: impl Into<String>) -> Self {
|
||||
Self {
|
||||
role: Role::Tool,
|
||||
content: Some(content.into()),
|
||||
tool_calls: None,
|
||||
tool_call_id: Some(tool_call_id.into()),
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
/// A tool the model may call.
|
||||
#[derive(Serialize, Deserialize, Debug, Clone)]
|
||||
pub struct Tool {
|
||||
#[serde(rename = "type", default = "function_type")]
|
||||
pub kind: String,
|
||||
pub function: FunctionDefinition,
|
||||
}
|
||||
|
||||
#[derive(Serialize, Deserialize, Debug, Clone)]
|
||||
pub struct FunctionDefinition {
|
||||
pub name: String,
|
||||
pub description: String,
|
||||
/// JSON schema describing the accepted arguments.
|
||||
pub parameters: Value,
|
||||
}
|
||||
|
||||
impl Tool {
|
||||
pub fn new(name: impl Into<String>, description: impl Into<String>, parameters: Value) -> Self {
|
||||
Self {
|
||||
kind: function_type(),
|
||||
function: FunctionDefinition {
|
||||
name: name.into(),
|
||||
description: description.into(),
|
||||
parameters,
|
||||
},
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
/// A tool call requested by the model.
|
||||
#[derive(Serialize, Deserialize, Debug, Clone)]
|
||||
pub struct ToolCall {
|
||||
pub id: String,
|
||||
#[serde(rename = "type", default = "function_type")]
|
||||
pub kind: String,
|
||||
pub function: FunctionCall,
|
||||
}
|
||||
|
||||
#[derive(Serialize, Deserialize, Debug, Clone)]
|
||||
pub struct FunctionCall {
|
||||
pub name: String,
|
||||
/// Arguments as a JSON-encoded string. Kept verbatim so that echoing the
|
||||
/// call back into the conversation does not re-encode or corrupt it.
|
||||
pub arguments: String,
|
||||
}
|
||||
|
||||
impl ToolCall {
|
||||
pub fn id(&self) -> &str {
|
||||
&self.id
|
||||
}
|
||||
|
||||
pub fn name(&self) -> &str {
|
||||
&self.function.name
|
||||
}
|
||||
|
||||
pub fn arguments_json(&self) -> &str {
|
||||
&self.function.arguments
|
||||
}
|
||||
}
|
||||
|
||||
fn function_type() -> String {
|
||||
String::from("function")
|
||||
}
|
||||
|
||||
#[derive(Serialize)]
|
||||
struct ChatRequest<'a> {
|
||||
model: &'a str,
|
||||
messages: &'a [Message],
|
||||
reasoning: Reasoning,
|
||||
tools: &'a [Tool],
|
||||
tool_choice: &'a str,
|
||||
}
|
||||
|
||||
/// OpenRouter reasoning config; Herald always asks for reasoning.
|
||||
#[derive(Serialize)]
|
||||
struct Reasoning {
|
||||
enabled: bool,
|
||||
}
|
||||
|
||||
#[derive(Deserialize)]
|
||||
struct ChatResponse {
|
||||
#[serde(default)]
|
||||
choices: Vec<Choice>,
|
||||
#[serde(default)]
|
||||
usage: Option<Usage>,
|
||||
}
|
||||
|
||||
#[derive(Deserialize)]
|
||||
struct Choice {
|
||||
message: ResponseMessage,
|
||||
}
|
||||
|
||||
#[derive(Deserialize)]
|
||||
struct ResponseMessage {
|
||||
#[serde(default)]
|
||||
content: Option<Content>,
|
||||
#[serde(default)]
|
||||
tool_calls: Option<Vec<ToolCall>>,
|
||||
}
|
||||
|
||||
/// Message content is a plain string for most models and a list of typed parts
|
||||
/// for others; both are collapsed to text.
|
||||
#[derive(Deserialize)]
|
||||
#[serde(untagged)]
|
||||
enum Content {
|
||||
Text(String),
|
||||
Parts(Vec<ContentPart>),
|
||||
}
|
||||
|
||||
#[derive(Deserialize)]
|
||||
struct ContentPart {
|
||||
#[serde(default)]
|
||||
text: Option<String>,
|
||||
}
|
||||
|
||||
impl Content {
|
||||
fn into_text(self) -> String {
|
||||
match self {
|
||||
Self::Text(text) => text,
|
||||
Self::Parts(parts) => parts
|
||||
.into_iter()
|
||||
.filter_map(|part| part.text)
|
||||
.collect::<Vec<_>>()
|
||||
.join(""),
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
#[derive(Deserialize)]
|
||||
struct Usage {
|
||||
#[serde(default)]
|
||||
cost: Option<f64>,
|
||||
}
|
||||
|
||||
/// Error payload returned by OpenRouter for a failed request.
|
||||
#[derive(Deserialize)]
|
||||
struct ErrorResponse {
|
||||
error: ErrorDetail,
|
||||
}
|
||||
|
||||
#[derive(Deserialize)]
|
||||
struct ErrorDetail {
|
||||
message: String,
|
||||
}
|
||||
|
||||
#[derive(Clone)]
|
||||
pub struct OpenRouterClient {
|
||||
client: reqwest::Client,
|
||||
api_key: String,
|
||||
model: String,
|
||||
}
|
||||
|
||||
impl OpenRouterClient {
|
||||
pub fn new(token: &str, model: &str, timeout: u64) -> anyhow::Result<Self> {
|
||||
Ok(Self {
|
||||
client: reqwest::Client::builder()
|
||||
.timeout(Duration::from_secs(timeout))
|
||||
.build()?,
|
||||
api_key: String::from(token),
|
||||
model: String::from(model),
|
||||
})
|
||||
}
|
||||
|
||||
/// Sends a conversation with tool definitions and returns either a final
|
||||
/// message or the tool calls requested by the model.
|
||||
///
|
||||
/// The conversation is borrowed rather than taken by value: the caller keeps
|
||||
/// appending to it between iterations, and a copy per iteration would be pure
|
||||
/// waste.
|
||||
#[instrument(skip(self, messages, tools), err)]
|
||||
pub async fn chat_with_tools(
|
||||
&self,
|
||||
messages: &[Message],
|
||||
tools: &[Tool],
|
||||
tool_choice: ToolChoice,
|
||||
) -> anyhow::Result<ToolChatResult> {
|
||||
let response = self.complete(messages, tools, tool_choice).await?;
|
||||
|
||||
let cost = response.usage.and_then(|usage| usage.cost);
|
||||
let message = response
|
||||
.choices
|
||||
.into_iter()
|
||||
.next()
|
||||
.ok_or_else(|| anyhow::anyhow!("No choices in response"))?
|
||||
.message;
|
||||
|
||||
Ok(ToolChatResult {
|
||||
message: message.content.map(Content::into_text),
|
||||
tool_calls: message.tool_calls.unwrap_or_default(),
|
||||
cost,
|
||||
})
|
||||
}
|
||||
|
||||
async fn complete(
|
||||
&self,
|
||||
messages: &[Message],
|
||||
tools: &[Tool],
|
||||
tool_choice: ToolChoice,
|
||||
) -> anyhow::Result<ChatResponse> {
|
||||
let request = ChatRequest {
|
||||
model: &self.model,
|
||||
messages,
|
||||
reasoning: Reasoning { enabled: true },
|
||||
tools,
|
||||
tool_choice: tool_choice.as_str(),
|
||||
};
|
||||
|
||||
let response = self
|
||||
.client
|
||||
.post(format!("{BASE_URL}/chat/completions"))
|
||||
.bearer_auth(&self.api_key)
|
||||
.json(&request)
|
||||
.send()
|
||||
.await
|
||||
.context("failed to reach OpenRouter")?;
|
||||
|
||||
let status = response.status();
|
||||
if !status.is_success() {
|
||||
let body = response.text().await.unwrap_or_default();
|
||||
anyhow::bail!("OpenRouter returned {status}: {}", error_message(&body));
|
||||
}
|
||||
|
||||
response
|
||||
.json::<ChatResponse>()
|
||||
.await
|
||||
.context("invalid OpenRouter response")
|
||||
}
|
||||
}
|
||||
|
||||
/// Extracts the message from an OpenRouter error body, falling back to the raw
|
||||
/// body when it is not the expected JSON shape.
|
||||
fn error_message(body: &str) -> String {
|
||||
serde_json::from_str::<ErrorResponse>(body)
|
||||
.map(|response| response.error.message)
|
||||
.unwrap_or_else(|_| body.trim().to_string())
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
mod tests {
|
||||
use super::*;
|
||||
use serde_json::json;
|
||||
|
||||
/// Builds a tool call the way the API returns one, so the fixture also
|
||||
/// covers deserialization.
|
||||
fn tool_call(id: &str, name: &str, arguments: &str) -> ToolCall {
|
||||
serde_json::from_value(json!({
|
||||
"id": id,
|
||||
"type": "function",
|
||||
"function": { "name": name, "arguments": arguments }
|
||||
}))
|
||||
.unwrap()
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn tool_call_arguments_are_kept_verbatim() {
|
||||
let call = tool_call("call_1", "grep", r#"{"pattern":"fn main"}"#);
|
||||
let message = Message::assistant_with_tool_calls("", vec![call]);
|
||||
|
||||
let serialized = serde_json::to_value(&message).unwrap();
|
||||
|
||||
assert_eq!(serialized["content"], Value::Null);
|
||||
assert_eq!(serialized["tool_calls"][0]["type"], "function");
|
||||
assert_eq!(serialized["tool_calls"][0]["function"]["name"], "grep");
|
||||
assert_eq!(
|
||||
serialized["tool_calls"][0]["function"]["arguments"],
|
||||
r#"{"pattern":"fn main"}"#
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn tool_response_carries_role_and_tool_call_id() {
|
||||
let message = Message::tool_response("call_1", "src/main.rs");
|
||||
let serialized = serde_json::to_value(&message).unwrap();
|
||||
|
||||
assert_eq!(serialized["role"], "tool");
|
||||
assert_eq!(serialized["tool_call_id"], "call_1");
|
||||
assert_eq!(serialized["content"], "src/main.rs");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn request_serializes_tool_choice_and_reasoning() {
|
||||
let messages = [Message::new(Role::User, "hi")];
|
||||
let tools = [Tool::new("ls", "List files", json!({"type": "object"}))];
|
||||
|
||||
let request = serde_json::to_value(ChatRequest {
|
||||
model: "some/model",
|
||||
messages: &messages,
|
||||
reasoning: Reasoning { enabled: true },
|
||||
tools: &tools,
|
||||
tool_choice: ToolChoice::Auto.as_str(),
|
||||
})
|
||||
.unwrap();
|
||||
|
||||
assert_eq!(request["model"], "some/model");
|
||||
assert_eq!(request["reasoning"]["enabled"], true);
|
||||
assert_eq!(request["tool_choice"], "auto");
|
||||
assert_eq!(request["tools"][0]["function"]["name"], "ls");
|
||||
assert_eq!(request["messages"][0]["role"], "user");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn a_forced_answer_turn_disables_the_tools() {
|
||||
assert_eq!(ToolChoice::None.as_str(), "none");
|
||||
assert_eq!(ToolChoice::Auto.as_str(), "auto");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn response_parses_tool_calls_and_cost() {
|
||||
let response: ChatResponse = serde_json::from_value(json!({
|
||||
"choices": [{
|
||||
"message": {
|
||||
"role": "assistant",
|
||||
"content": null,
|
||||
"tool_calls": [{
|
||||
"id": "call_1",
|
||||
"type": "function",
|
||||
"function": { "name": "ls", "arguments": "{\"path\":\"src\"}" }
|
||||
}]
|
||||
}
|
||||
}],
|
||||
"usage": { "cost": 0.0021 }
|
||||
}))
|
||||
.unwrap();
|
||||
|
||||
assert_eq!(response.usage.and_then(|usage| usage.cost), Some(0.0021));
|
||||
|
||||
let message = response.choices.into_iter().next().unwrap().message;
|
||||
assert!(message.content.is_none());
|
||||
|
||||
let calls = message.tool_calls.unwrap();
|
||||
assert_eq!(calls[0].name(), "ls");
|
||||
assert_eq!(calls[0].id(), "call_1");
|
||||
assert_eq!(calls[0].arguments_json(), r#"{"path":"src"}"#);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn response_parses_parts_content() {
|
||||
let response: ChatResponse = serde_json::from_value(json!({
|
||||
"choices": [{
|
||||
"message": {
|
||||
"role": "assistant",
|
||||
"content": [
|
||||
{ "type": "text", "text": "hello " },
|
||||
{ "type": "text", "text": "world" }
|
||||
]
|
||||
}
|
||||
}]
|
||||
}))
|
||||
.unwrap();
|
||||
|
||||
assert!(response.usage.is_none());
|
||||
|
||||
let message = response.choices.into_iter().next().unwrap().message;
|
||||
assert_eq!(message.content.unwrap().into_text(), "hello world");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn error_message_prefers_api_message() {
|
||||
let body = r#"{"error":{"message":"No auth credentials found","code":401}}"#;
|
||||
assert_eq!(error_message(body), "No auth credentials found");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn error_message_falls_back_to_raw_body() {
|
||||
assert_eq!(
|
||||
error_message(" <html>bad gateway</html> "),
|
||||
"<html>bad gateway</html>"
|
||||
);
|
||||
}
|
||||
}
|
||||
@@ -0,0 +1,316 @@
|
||||
//! Tool-calling loop driving the model against a [`Sandbox`].
|
||||
|
qpismont marked this conversation as resolved
Herald
commented
Processus lancés dans le sandbox : vérifier qu'un timeout est appliqué, que le processus (et son groupe) est tué en cas de dépassement ou d'annulation, et que les pipes stdout/stderr sont consommés pour éviter un blocage du processus si le buffer se remplit. Prévoir aussi une limite de taille de sortie pour ne pas saturer la mémoire avec un output verbeux ou malveillant. Processus lancés dans le sandbox : vérifier qu'un timeout est appliqué, que le processus (et son groupe) est tué en cas de dépassement ou d'annulation, et que les pipes stdout/stderr sont consommés pour éviter un blocage du processus si le buffer se remplit. Prévoir aussi une limite de taille de sortie pour ne pas saturer la mémoire avec un output verbeux ou malveillant.
Herald
commented
Boucle d'agent exécutant des outils : prévoir un nombre maximal d'itérations et une limite de temps globale, sinon une boucle d'appels d'outils peut consommer indéfiniment des tokens/CPU. Boucle d'agent exécutant des outils : prévoir un nombre maximal d'itérations et une limite de temps globale, sinon une boucle d'appels d'outils peut consommer indéfiniment des tokens/CPU.
|
||||
//!
|
||||
//! The loop sends the conversation and the available tools to OpenRouter,
|
||||
//! executes any requested tool call inside the sandbox and feeds the results
|
||||
//! back, until the model produces a final message or the iteration budget is
|
||||
//! exhausted.
|
||||
|
||||
use std::str::FromStr;
|
||||
use std::time::Instant;
|
||||
|
||||
use anyhow::Context;
|
||||
use serde_json::Value;
|
||||
use tracing::{info, warn};
|
||||
|
||||
use crate::{
|
||||
open_router::{Message, OpenRouterClient, Role, Tool, ToolCall, ToolChoice},
|
||||
sandbox::{Sandbox, tools},
|
||||
text::{excerpt, truncate_bytes},
|
||||
};
|
||||
|
||||
/// Number of characters of the arguments of a tool call kept in the logs.
|
||||
const MAX_LOGGED_ARGUMENTS: usize = 200;
|
||||
|
||||
/// Number of characters of a tool result kept in the logs.
|
||||
///
|
||||
/// Sizes alone make a broken tool invisible: a command that fails returns a short
|
||||
/// `error: …` instead of the expected content, which is exactly what this excerpt
|
||||
/// makes obvious.
|
||||
const MAX_LOGGED_OUTPUT: usize = 120;
|
||||
|
||||
/// Maximum number of bytes of a tool result handed to the model.
|
||||
///
|
||||
/// A result stays in the conversation and is re-sent on every following
|
||||
/// iteration, so an unbounded one (a whole file, a match on every line) inflates
|
||||
/// the context and the cost of the whole rest of the run — and can overflow the
|
||||
/// model context window outright.
|
||||
const MAX_TOOL_OUTPUT_BYTES: usize = 32 * 1024;
|
||||
|
||||
/// Asked of the model when its answer does not satisfy the caller's contract.
|
||||
const RETRY_PROMPT: &str = "
|
||||
Your answer is not valid: {error}
|
||||
|
||||
Answer again with the requested format only: a raw json object, without any
|
||||
markdown code fence and without any text before or after it.
|
||||
";
|
||||
|
||||
/// Asked of the model on the last turn, in place of one more exploration turn.
|
||||
///
|
||||
/// Without it, a model still exploring on its last turn would ask for another
|
||||
/// tool, and the run would end on the iteration budget with no answer at all.
|
||||
const FINAL_PROMPT: &str = "
|
||||
You are out of turns: this is your last one. Answer now, with the requested
|
||||
format, from what you have already gathered, without calling any tool.
|
||||
";
|
||||
|
||||
/// Final output of an agent run.
|
||||
pub struct AgentResult<A> {
|
||||
/// Answer of the model, parsed into the type the caller asked for.
|
||||
pub answer: A,
|
||||
pub cost: Option<f64>,
|
||||
pub iterations: usize,
|
||||
}
|
||||
|
||||
/// Runs the tool-calling loop until the model answers or `max_iterations` is
|
||||
/// reached.
|
||||
///
|
||||
/// `tool_definitions` is the set of tools the model may call; it is selected by
|
||||
/// the caller based on the webhook action (see [`tools::for_webhook`]).
|
||||
///
|
||||
/// The answer of the model is parsed into `A`, which is how the caller states the
|
||||
/// format it expects: while [`FromStr`] rejects the answer, the error is sent back
|
||||
/// to the model so that it can correct itself, which costs an iteration. The
|
||||
/// parsing happens here because that is where the conversation and the remaining
|
||||
/// budget are at hand.
|
||||
pub async fn run<A>(
|
||||
open_router: &OpenRouterClient,
|
||||
sandbox: &Sandbox,
|
||||
tool_definitions: Vec<Tool>,
|
||||
system_prompt: &str,
|
||||
user_prompt: &str,
|
||||
max_iterations: usize,
|
||||
) -> anyhow::Result<AgentResult<A>>
|
||||
where
|
||||
A: FromStr,
|
||||
A::Err: std::fmt::Display,
|
||||
{
|
||||
let mut messages = vec![
|
||||
Message::new(Role::System, system_prompt),
|
||||
Message::new(Role::User, user_prompt),
|
||||
];
|
||||
|
||||
info!(
|
||||
tools = %tool_definitions
|
||||
.iter()
|
||||
.map(|tool| tool.function.name.as_str())
|
||||
.collect::<Vec<_>>()
|
||||
.join(", "),
|
||||
max_iterations,
|
||||
"Starting tool-calling loop"
|
||||
);
|
||||
|
||||
let mut total_cost = 0.0_f64;
|
||||
let mut has_cost = false;
|
||||
let mut last_rejection: Option<String> = None;
|
||||
|
||||
for iteration in 1..=max_iterations {
|
||||
// Le dernier tour n'est plus un tour d'exploration : le modèle doit rendre
|
||||
// sa réponse, avec ce qu'il a déjà vu.
|
||||
let last = iteration == max_iterations;
|
||||
|
||||
if last {
|
||||
info!(iteration, "Last turn: asking for the final answer");
|
||||
messages.push(Message::new(Role::User, FINAL_PROMPT));
|
||||
}
|
||||
|
||||
let started = Instant::now();
|
||||
let response = open_router
|
||||
.chat_with_tools(
|
||||
&messages,
|
||||
&tool_definitions,
|
||||
if last {
|
||||
ToolChoice::None
|
||||
} else {
|
||||
ToolChoice::Auto
|
||||
},
|
||||
)
|
||||
.await?;
|
||||
|
||||
if let Some(cost) = response.cost {
|
||||
total_cost += cost;
|
||||
has_cost = true;
|
||||
}
|
||||
|
||||
info!(
|
||||
iteration,
|
||||
max_iterations,
|
||||
last_turn = last,
|
||||
tool_calls = response.tool_calls.len(),
|
||||
answer_bytes = response.message.as_deref().map_or(0, str::len),
|
||||
elapsed_ms = started.elapsed().as_millis() as u64,
|
||||
"Model answered"
|
||||
);
|
||||
|
||||
// Au dernier tour, les outils sont refusés : même si le fournisseur les
|
||||
// renvoie malgré tout, on ne les exécute pas et on tente la réponse.
|
||||
if last || response.tool_calls.is_empty() {
|
||||
let answer = response.message.unwrap_or_default();
|
||||
|
||||
match answer.parse::<A>() {
|
||||
Ok(parsed) => {
|
||||
return Ok(AgentResult {
|
||||
answer: parsed,
|
||||
cost: has_cost.then_some(total_cost),
|
||||
iterations: iteration,
|
||||
});
|
||||
}
|
||||
Err(err) => {
|
||||
warn!(iteration, %err, "Model answer was rejected, asking for another one");
|
||||
|
||||
// Keep the rejected answer in the history, so that the model
|
||||
// sees what it has to fix.
|
||||
messages.push(Message::new(Role::Assistant, answer));
|
||||
messages.push(Message::new(
|
||||
Role::User,
|
||||
RETRY_PROMPT.replace("{error}", &err.to_string()),
|
||||
));
|
||||
|
||||
last_rejection = Some(err.to_string());
|
||||
continue;
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
messages.push(Message::assistant_with_tool_calls(
|
||||
response.message.unwrap_or_default(),
|
||||
response.tool_calls.clone(),
|
||||
));
|
||||
|
||||
for call in &response.tool_calls {
|
||||
let started = Instant::now();
|
||||
let (content, truncated) = execute(sandbox, call).await;
|
||||
|
||||
info!(
|
||||
iteration,
|
||||
tool = call.name(),
|
||||
arguments = %excerpt(call.arguments_json(), MAX_LOGGED_ARGUMENTS),
|
||||
output_bytes = content.len(),
|
||||
truncated,
|
||||
// Les sauts de ligne casseraient la lisibilité d'une ligne de log.
|
||||
output = %excerpt(&content, MAX_LOGGED_OUTPUT).replace('\n', " "),
|
||||
elapsed_ms = started.elapsed().as_millis() as u64,
|
||||
"Tool call finished"
|
||||
);
|
||||
|
||||
messages.push(Message::tool_response(call.id(), content));
|
||||
}
|
||||
}
|
||||
|
||||
warn!(
|
||||
max_iterations,
|
||||
"Agent used its last turn without a valid answer"
|
||||
);
|
||||
|
||||
match last_rejection {
|
||||
Some(error) => anyhow::bail!(
|
||||
"agent exceeded the maximum number of iterations ({max_iterations}), \
|
||||
last answer rejected: {error}"
|
||||
),
|
||||
None => anyhow::bail!("agent exceeded the maximum number of iterations ({max_iterations})"),
|
||||
}
|
||||
}
|
||||
|
||||
/// Runs a tool call and returns its result for the model, along with whether the
|
||||
/// result had to be truncated.
|
||||
async fn execute(sandbox: &Sandbox, call: &ToolCall) -> (String, bool) {
|
||||
let args = match parse_args(call) {
|
||||
Ok(args) => args,
|
||||
Err(err) => return cap_output(format!("error: {err}")),
|
||||
};
|
||||
|
||||
match tools::dispatch(sandbox, call.name(), &args).await {
|
||||
Ok(output) => cap_output(output),
|
||||
Err(err) => cap_output(format!("error: {err}")),
|
||||
}
|
||||
}
|
||||
|
||||
/// Bounds [`MAX_TOOL_OUTPUT_BYTES`] of a tool result, telling the model what was
|
||||
/// dropped so that it can narrow its request.
|
||||
fn cap_output(mut output: String) -> (String, bool) {
|
||||
let total = output.len();
|
||||
|
||||
if !truncate_bytes(&mut output, MAX_TOOL_OUTPUT_BYTES) {
|
||||
return (output, false);
|
||||
}
|
||||
|
||||
let kept = output.len();
|
||||
output.push_str(&format!(
|
||||
"\n… output truncated: {total} bytes in total, the first {kept} are shown. \
|
||||
Narrow the request (path, pattern or line range) to see the rest."
|
||||
));
|
||||
|
||||
(output, true)
|
||||
}
|
||||
|
||||
fn parse_args(call: &ToolCall) -> anyhow::Result<Value> {
|
||||
let raw = call.arguments_json().trim();
|
||||
if raw.is_empty() {
|
||||
return Ok(Value::Object(serde_json::Map::new()));
|
||||
}
|
||||
|
||||
serde_json::from_str(raw)
|
||||
.with_context(|| format!("invalid arguments for tool `{}`", call.name()))
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
mod tests {
|
||||
use super::*;
|
||||
use serde_json::json;
|
||||
|
||||
/// Builds a tool call the way the API returns one, so the fixture also
|
||||
/// covers deserialization.
|
||||
fn tool_call(name: &str, arguments: &str) -> ToolCall {
|
||||
serde_json::from_value(json!({
|
||||
"id": "call_1",
|
||||
"type": "function",
|
||||
"function": { "name": name, "arguments": arguments }
|
||||
}))
|
||||
.unwrap()
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn parse_args_accepts_empty_arguments() {
|
||||
let call = tool_call("ls", "");
|
||||
assert_eq!(
|
||||
parse_args(&call).unwrap(),
|
||||
Value::Object(serde_json::Map::new())
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn parse_args_parses_json_object() {
|
||||
let call = tool_call("ls", r#"{"path":"src"}"#);
|
||||
assert_eq!(parse_args(&call).unwrap()["path"], "src");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn parse_args_rejects_invalid_json() {
|
||||
let call = tool_call("ls", "not json");
|
||||
assert!(parse_args(&call).is_err());
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn a_small_tool_output_is_kept_as_is() {
|
||||
let (content, truncated) = cap_output(String::from("src/main.rs"));
|
||||
|
||||
assert_eq!(content, "src/main.rs");
|
||||
assert!(!truncated);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn a_large_tool_output_is_truncated_and_told_to_the_model() {
|
||||
let (content, truncated) = cap_output("a".repeat(MAX_TOOL_OUTPUT_BYTES + 1));
|
||||
|
||||
assert!(truncated);
|
||||
assert!(content.starts_with(&"a".repeat(MAX_TOOL_OUTPUT_BYTES)));
|
||||
assert!(content.contains("output truncated"));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn a_truncated_tool_output_stays_valid_utf8() {
|
||||
let (content, truncated) = cap_output("é".repeat(MAX_TOOL_OUTPUT_BYTES));
|
||||
|
||||
assert!(truncated);
|
||||
assert!(content.starts_with('é'));
|
||||
}
|
||||
}
|
||||
@@ -0,0 +1,288 @@
|
||||
//! Le [`Sandbox`] : clone d'une pull request exécuté dans un devcontainer
|
||||
//! éphémère, et sa configuration.
|
||||
|
||||
use std::{
|
||||
path::{Path, PathBuf},
|
||||
process::Stdio,
|
||||
};
|
||||
|
||||
use anyhow::Context;
|
||||
use devcontainer_rs::{Container, ContainerRuntime, ExecOutput};
|
||||
use tempfile::TempDir;
|
||||
use tracing::{info, instrument};
|
||||
|
||||
/// Devcontainer locations recognized within a repository, in priority order.
|
||||
const DEVCONTAINER_PATHS: [&str; 2] = [".devcontainer/devcontainer.json", ".devcontainer.json"];
|
||||
|
||||
/// Sandbox-related runtime configuration.
|
||||
#[derive(Clone)]
|
||||
pub struct SandboxConfig {
|
||||
/// Client du daemon de containers qui exécute la sandbox.
|
||||
pub runtime: ContainerRuntime,
|
||||
/// Maximum number of tool-calling iterations per agent run.
|
||||
pub max_iterations: usize,
|
||||
}
|
||||
|
||||
/// A cloned repository running inside an ephemeral devcontainer.
|
||||
pub struct Sandbox {
|
||||
// Owns the temporary directory; dropping it cleans up the clone.
|
||||
_workspace: TempDir,
|
||||
/// Path of the clone, as copied into the container.
|
||||
repo_dir: PathBuf,
|
||||
container: Container,
|
||||
}
|
||||
|
||||
impl Sandbox {
|
||||
/// Clones the pull request head, builds the devcontainer and starts it.
|
||||
///
|
||||
/// The clone is PR-aware: it fetches `refs/pull/<number>/head`, which works
|
||||
/// for both same-repository and forked pull requests.
|
||||
#[instrument(skip(runtime, token), fields(pr = pull_request_number))]
|
||||
pub async fn create(
|
||||
runtime: &ContainerRuntime,
|
||||
clone_url: &str,
|
||||
token: &str,
|
||||
pull_request_number: u64,
|
||||
) -> anyhow::Result<Self> {
|
||||
let workspace = tempfile::tempdir().context("failed to create sandbox workspace")?;
|
||||
let repo_dir = workspace.path().join("repo");
|
||||
|
||||
clone_pull_request(clone_url, token, pull_request_number, &repo_dir).await?;
|
||||
make_readable(&repo_dir).await?;
|
||||
|
||||
let devcontainer_path = find_devcontainer(&repo_dir)
|
||||
.with_context(|| format!("no devcontainer found in `{clone_url}`"))?;
|
||||
|
||||
let devcontainer = devcontainer_rs::parse(&devcontainer_path).await?;
|
||||
|
||||
info!(image = %devcontainer.image_tag(), "Building and starting sandbox container");
|
||||
let container = devcontainer.up(runtime, &repo_dir).await?;
|
||||
|
||||
let sandbox = Self {
|
||||
_workspace: workspace,
|
||||
repo_dir,
|
||||
container,
|
||||
};
|
||||
|
||||
if let Err(err) = sandbox.check_workspace().await {
|
||||
let _ = sandbox.container.remove().await;
|
||||
return Err(err);
|
||||
}
|
||||
|
||||
Ok(sandbox)
|
||||
}
|
||||
|
||||
/// Vérifie que le clone est bien visible dans le 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.
|
||||
async fn check_workspace(&self) -> anyhow::Result<()> {
|
||||
let workspace_folder = self.workspace_folder();
|
||||
let probe = self
|
||||
.container
|
||||
.exec(&["ls", "-A", "--", workspace_folder])
|
||||
.await
|
||||
.with_context(|| format!("failed to list `{workspace_folder}` in the sandbox"))?;
|
||||
|
||||
if !probe.success() {
|
||||
let details = self.diagnose_workspace().await;
|
||||
|
||||
anyhow::bail!(
|
||||
"the sandbox cannot list `{workspace_folder}`: {} ({details})",
|
||||
probe.stderr.trim()
|
||||
);
|
||||
}
|
||||
|
||||
if probe.stdout.trim().is_empty() {
|
||||
anyhow::bail!(
|
||||
"the sandbox workspace `{workspace_folder}` is empty: the container daemon does not see the clone"
|
||||
);
|
||||
}
|
||||
|
||||
info!(
|
||||
clone = %self.repo_dir.display(),
|
||||
workspace = %workspace_folder,
|
||||
entries = probe.stdout.lines().count(),
|
||||
"Sandbox workspace is readable"
|
||||
);
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
/// Rassemble de quoi expliquer un refus d'accès au workspace.
|
||||
///
|
||||
/// Un `EACCES` a deux causes possibles, indistinguables dans le message de
|
||||
/// `ls` : les permissions du clone, ou un confinement du noyau qui bloque
|
||||
/// l'accès. L'identité de l'utilisateur d'exec et les permissions du point de
|
||||
/// montage permettent de trancher.
|
||||
async fn diagnose_workspace(&self) -> String {
|
||||
let mut details = Vec::new();
|
||||
|
||||
if let Ok(output) = self.container.exec(&["id"]).await {
|
||||
details.push(output.stdout.trim().to_string());
|
||||
}
|
||||
|
||||
if let Ok(output) = self
|
||||
.container
|
||||
.exec(&["ls", "-ld", "--", self.workspace_folder()])
|
||||
.await
|
||||
&& output.success()
|
||||
{
|
||||
details.push(output.stdout.trim().to_string());
|
||||
}
|
||||
|
||||
details.join(", ")
|
||||
}
|
||||
|
||||
/// Executes a command in the container as an argv vector (no shell).
|
||||
pub async fn exec(&self, cmd: &[&str]) -> anyhow::Result<ExecOutput> {
|
||||
Ok(self.container.exec(cmd).await?)
|
||||
}
|
||||
|
||||
/// Path of the repository inside the container.
|
||||
pub fn workspace_folder(&self) -> &str {
|
||||
self.container.workspace_folder()
|
||||
}
|
||||
|
||||
/// Stops and removes the container. The temporary clone is removed on drop.
|
||||
pub async fn cleanup(self) -> anyhow::Result<()> {
|
||||
self.container.remove().await?;
|
||||
Ok(())
|
||||
}
|
||||
}
|
||||
|
||||
fn find_devcontainer(repo_dir: &Path) -> Option<PathBuf> {
|
||||
DEVCONTAINER_PATHS
|
||||
.iter()
|
||||
.map(|relative| repo_dir.join(relative))
|
||||
.find(|candidate| candidate.is_file())
|
||||
}
|
||||
|
||||
async fn clone_pull_request(
|
||||
repo_url: &str,
|
||||
token: &str,
|
||||
pull_request_number: u64,
|
||||
dest: &Path,
|
||||
) -> anyhow::Result<()> {
|
||||
let dest = dest.display().to_string();
|
||||
|
||||
run_git(
|
||||
token,
|
||||
&[
|
||||
"clone".to_string(),
|
||||
"--depth".to_string(),
|
||||
"1".to_string(),
|
||||
repo_url.to_string(),
|
||||
dest.clone(),
|
||||
],
|
||||
)
|
||||
.await?;
|
||||
|
||||
run_git(
|
||||
token,
|
||||
&[
|
||||
"-C".to_string(),
|
||||
dest.clone(),
|
||||
"fetch".to_string(),
|
||||
"--depth".to_string(),
|
||||
"1".to_string(),
|
||||
"origin".to_string(),
|
||||
format!("refs/pull/{pull_request_number}/head"),
|
||||
],
|
||||
)
|
||||
.await?;
|
||||
|
||||
run_git(
|
||||
token,
|
||||
&[
|
||||
"-C".to_string(),
|
||||
dest,
|
||||
"checkout".to_string(),
|
||||
"FETCH_HEAD".to_string(),
|
||||
],
|
||||
)
|
||||
.await?;
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
/// Rend le clone lisible par les utilisateurs du container sandbox.
|
||||
///
|
||||
/// Le container peut ne pas avoir les mêmes uid que Herald (podman rootless
|
||||
/// mappe les uid à travers des plages subuid), et l'umask de l'opérateur peut être
|
||||
/// restrictif : sans cela, les outils de la sandbox échouent en `Permission
|
||||
/// denied` sur des fichiers que Herald vient de cloner lui-même.
|
||||
async fn make_readable(repo_dir: &Path) -> anyhow::Result<()> {
|
||||
let output = tokio::process::Command::new("chmod")
|
||||
.args(["-R", "a+rX"])
|
||||
.arg(repo_dir)
|
||||
.stdin(Stdio::null())
|
||||
.output()
|
||||
.await
|
||||
.context("failed to spawn chmod")?;
|
||||
|
||||
if !output.status.success() {
|
||||
anyhow::bail!(
|
||||
"chmod failed on `{}`: {}",
|
||||
repo_dir.display(),
|
||||
String::from_utf8_lossy(&output.stderr).trim()
|
||||
);
|
||||
}
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
/// Runs git with the token injected through `http.extraHeader`, keeping the
|
||||
/// secret out of the process arguments.
|
||||
async fn run_git(token: &str, args: &[String]) -> anyhow::Result<()> {
|
||||
let output = tokio::process::Command::new("git")
|
||||
.args(args)
|
||||
.env("GIT_CONFIG_COUNT", "1")
|
||||
.env("GIT_CONFIG_KEY_0", "http.extraHeader")
|
||||
.env(
|
||||
"GIT_CONFIG_VALUE_0",
|
||||
format!("Authorization: token {token}"),
|
||||
)
|
||||
.env("GIT_TERMINAL_PROMPT", "0")
|
||||
.stdin(Stdio::null())
|
||||
.output()
|
||||
.await
|
||||
.context("failed to spawn git")?;
|
||||
|
||||
if !output.status.success() {
|
||||
anyhow::bail!(
|
||||
"git {} failed: {}",
|
||||
args.join(" "),
|
||||
String::from_utf8_lossy(&output.stderr).trim()
|
||||
);
|
||||
}
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
mod tests {
|
||||
use super::*;
|
||||
|
||||
#[test]
|
||||
fn find_devcontainer_prefers_dot_devcontainer_dir() {
|
||||
let dir = tempfile::tempdir().unwrap();
|
||||
let nested = dir.path().join(".devcontainer");
|
||||
std::fs::create_dir(&nested).unwrap();
|
||||
std::fs::write(nested.join("devcontainer.json"), "{}").unwrap();
|
||||
std::fs::write(dir.path().join(".devcontainer.json"), "{}").unwrap();
|
||||
|
||||
assert_eq!(
|
||||
find_devcontainer(dir.path()),
|
||||
Some(nested.join("devcontainer.json"))
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn find_devcontainer_returns_none_when_absent() {
|
||||
let dir = tempfile::tempdir().unwrap();
|
||||
assert_eq!(find_devcontainer(dir.path()), None);
|
||||
}
|
||||
}
|
||||
@@ -0,0 +1,13 @@
|
||||
//! Sandboxed tool execution for the AI bot.
|
||||
|
qpismont marked this conversation as resolved
Herald
commented
Nouveau module Nouveau module `sandbox` : ajouter en tête de fichier une documentation (doc-comment) décrivant le modèle de menace et le niveau d'isolation réellement appliqué (namespaces, cgroups, utilisateur non-root, montages read-only, limites CPU/RAM/pid). Sans cela, il est impossible de valider que le nom « sandbox » correspond à une isolation effective et non à un simple répertoire de travail. Préciser aussi le comportement en cas d'échec d'initialisation de l'isolation (échec fermé plutôt qu'ouvert).
Herald
commented
Le module de sandboxing devrait exposer des limites explicites (mémoire, CPU, timeout, réseau désactivé par défaut) et retourner des erreurs typées plutôt que des Le module de sandboxing devrait exposer des limites explicites (mémoire, CPU, timeout, réseau désactivé par défaut) et retourner des erreurs typées plutôt que des `String`, afin que l'appelant puisse décider de la politique de repli.
|
||||
//!
|
||||
//! A [`Sandbox`] clones a pull request into a temporary directory, builds and
|
||||
//! starts its devcontainer (via `devcontainer-rs`) and exposes command
|
||||
//! execution inside the resulting container. The [`tools`] module maps model
|
||||
//! tool calls to commands run in that container, and [`agent`] drives the
|
||||
//! tool-calling loop against OpenRouter.
|
||||
|
||||
pub mod agent;
|
||||
mod instance;
|
||||
pub mod tools;
|
||||
|
||||
pub use instance::{Sandbox, SandboxConfig};
|
||||
@@ -0,0 +1,303 @@
|
||||
//! Tool definitions exposed to the model and their execution inside a
|
||||
|
qpismont marked this conversation as resolved
Herald
commented
Exécution d'outils dans le sandbox : s'assurer que les commandes passent par une liste blanche explicite (allowlist de binaires + arguments validés) et jamais par un shell ( Exécution d'outils dans le sandbox : s'assurer que les commandes passent par une liste blanche explicite (allowlist de binaires + arguments validés) et jamais par un shell (`sh -c`) avec interpolation de chaînes issues du contenu de la PR. Les entrées utilisateur (diff, messages, noms de fichiers) doivent être passées comme arguments séparés, avec validation/échappement. Vérifier également l'absence de `Command::new` construit à partir d'une variable non validée.
Herald
commented
Les outils exposés à l'agent doivent valider leurs entrées (chemins relatifs, pas de Les outils exposés à l'agent doivent valider leurs entrées (chemins relatifs, pas de `..`, pas de shell arbitraire) et documenter clairement le contrat JSON de chaque outil ; c'est la principale surface d'attaque du sandboxing.
|
||||
//! [`Sandbox`].
|
||||
//!
|
||||
//! Every tool is read-only and confined to the repository workspace: paths are
|
||||
//! resolved relative to the container workspace folder and rejected if they
|
||||
//! escape it. Commands are executed as argv vectors (never through a shell),
|
||||
//! so tool arguments cannot be used for shell injection.
|
||||
|
||||
use std::path::Path;
|
||||
|
||||
use anyhow::{Context, bail};
|
||||
use devcontainer_rs::{ExecOutput, normalize};
|
||||
use serde_json::{Value, json};
|
||||
|
||||
use super::Sandbox;
|
||||
use crate::{gitea::WebhookType, open_router::Tool};
|
||||
|
||||
/// Tools available to the model for a given webhook action.
|
||||
///
|
||||
/// The match is exhaustive on [`WebhookType`], so adding a new action forces a
|
||||
/// decision here about which tools that action may use.
|
||||
pub fn for_webhook(webhook: &WebhookType) -> Vec<Tool> {
|
||||
match webhook {
|
||||
WebhookType::Review(_) => review_tools(),
|
||||
}
|
||||
}
|
||||
|
||||
/// Read-only tools used to explore a repository during a review.
|
||||
fn review_tools() -> Vec<Tool> {
|
||||
vec![
|
||||
Tool::new(
|
||||
"ls",
|
||||
"List the entries of a directory inside the repository.",
|
||||
json!({
|
||||
"type": "object",
|
||||
"properties": {
|
||||
"path": {
|
||||
"type": "string",
|
||||
"description": "Directory path relative to the repository root. Defaults to the repository root."
|
||||
}
|
||||
}
|
||||
}),
|
||||
),
|
||||
Tool::new(
|
||||
"file_size",
|
||||
"Get the size of a file inside the repository in bytes.",
|
||||
json!({
|
||||
"type": "object",
|
||||
"properties": {
|
||||
"path": {
|
||||
"type": "string",
|
||||
"description": "File path relative to the repository root."
|
||||
}
|
||||
}
|
||||
}),
|
||||
),
|
||||
Tool::new(
|
||||
"read_file",
|
||||
"Read the content of a text file inside the repository. Every line is \
|
||||
prefixed with its absolute line number, even when only a range is read.",
|
||||
json!({
|
||||
"type": "object",
|
||||
"properties": {
|
||||
"path": {
|
||||
"type": "string",
|
||||
"description": "File path relative to the repository root."
|
||||
},
|
||||
"start_line": {
|
||||
"type": "integer",
|
||||
"description": "First line to read (1-based, inclusive). Defaults to the first line."
|
||||
},
|
||||
"end_line": {
|
||||
"type": "integer",
|
||||
"description": "Last line to read (1-based, inclusive). Defaults to the last line."
|
||||
}
|
||||
},
|
||||
"required": ["path"]
|
||||
}),
|
||||
),
|
||||
Tool::new(
|
||||
"grep",
|
||||
"Search for a regular expression across the repository files.",
|
||||
json!({
|
||||
"type": "object",
|
||||
"properties": {
|
||||
"pattern": {
|
||||
"type": "string",
|
||||
"description": "Extended regular expression to search for."
|
||||
},
|
||||
"path": {
|
||||
"type": "string",
|
||||
"description": "File or directory to search in, relative to the repository root. Defaults to the repository root."
|
||||
}
|
||||
},
|
||||
"required": ["pattern"]
|
||||
}),
|
||||
),
|
||||
Tool::new(
|
||||
"find",
|
||||
"Find files by name pattern inside the repository.",
|
||||
json!({
|
||||
"type": "object",
|
||||
"properties": {
|
||||
"pattern": {
|
||||
"type": "string",
|
||||
"description": "Glob pattern matched against file names, e.g. `*.rs`."
|
||||
},
|
||||
"path": {
|
||||
"type": "string",
|
||||
"description": "Directory to search in, relative to the repository root. Defaults to the repository root."
|
||||
}
|
||||
},
|
||||
"required": ["pattern"]
|
||||
}),
|
||||
),
|
||||
]
|
||||
}
|
||||
|
||||
/// Executes a tool call and returns its textual result.
|
||||
///
|
||||
/// Errors are returned as `Err` so the caller can decide whether to surface
|
||||
/// them to the model or abort.
|
||||
pub async fn dispatch(sandbox: &Sandbox, name: &str, args: &Value) -> anyhow::Result<String> {
|
||||
match name {
|
||||
"ls" => ls(sandbox, args).await,
|
||||
|
qpismont marked this conversation as resolved
Herald
commented
`read_file` exécute `cat` sans aucune borne : un gros fichier (ou binaire) injecte tout son contenu dans la conversation et peut saturer le contexte et faire exploser le coût. Plafonner le nombre de lignes/octets renvoyés avec une note de troncature (idem pour `grep` et `find`, potentiellement très verbeux).
|
||||
"read_file" => read_file(sandbox, args).await,
|
||||
"file_size" => file_size(sandbox, args).await,
|
||||
"grep" => grep(sandbox, args).await,
|
||||
"find" => find(sandbox, args).await,
|
||||
other => bail!("unknown tool `{other}`"),
|
||||
}
|
||||
}
|
||||
|
||||
async fn file_size(sandbox: &Sandbox, args: &Value) -> anyhow::Result<String> {
|
||||
let path = resolve(sandbox, required_str(args, "path")?)?;
|
||||
let size = sandbox.exec(&["du", "-b", "--", &path]).await?;
|
||||
into_stdout(size)
|
||||
}
|
||||
|
||||
async fn ls(sandbox: &Sandbox, args: &Value) -> anyhow::Result<String> {
|
||||
let path = resolve(sandbox, optional_str(args, "path").unwrap_or("."))?;
|
||||
let output = sandbox.exec(&["ls", "-la", "--", &path]).await?;
|
||||
into_stdout(output)
|
||||
}
|
||||
|
||||
async fn read_file(sandbox: &Sandbox, args: &Value) -> anyhow::Result<String> {
|
||||
let path = resolve(sandbox, required_str(args, "path")?)?;
|
||||
|
||||
let start = args.get("start_line").and_then(Value::as_u64);
|
||||
let end = args.get("end_line").and_then(Value::as_u64);
|
||||
|
||||
let (first_line, output) = if start.is_none() && end.is_none() {
|
||||
(1, sandbox.exec(&["cat", "--", &path]).await?)
|
||||
} else {
|
||||
let start = start.unwrap_or(1);
|
||||
let end = end
|
||||
.map(|line| line.to_string())
|
||||
.unwrap_or_else(|| "$".to_string());
|
||||
let range = format!("{start},{end}p");
|
||||
|
||||
(
|
||||
start,
|
||||
sandbox.exec(&["sed", "-n", &range, "--", &path]).await?,
|
||||
)
|
||||
};
|
||||
|
||||
Ok(number_lines(&into_stdout(output)?, first_line))
|
||||
}
|
||||
|
||||
/// Préfixe chaque ligne par son numéro.
|
||||
///
|
||||
/// 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é.
|
||||
fn number_lines(content: &str, first_line: u64) -> String {
|
||||
content
|
||||
.lines()
|
||||
.enumerate()
|
||||
.map(|(offset, line)| format!("{}:{line}", first_line + offset as u64))
|
||||
.collect::<Vec<_>>()
|
||||
.join("\n")
|
||||
}
|
||||
|
||||
async fn grep(sandbox: &Sandbox, args: &Value) -> anyhow::Result<String> {
|
||||
let pattern = required_str(args, "pattern")?;
|
||||
let path = resolve(sandbox, optional_str(args, "path").unwrap_or("."))?;
|
||||
|
||||
let output = sandbox
|
||||
.exec(&["grep", "-rn", "-E", "--", pattern, &path])
|
||||
.await?;
|
||||
|
||||
// grep exits with 1 when there is no match, which is not an error.
|
||||
match output.status {
|
||||
0 | 1 => Ok(output.stdout),
|
||||
_ => bail!("grep failed: {}", output.stderr.trim()),
|
||||
}
|
||||
|
Herald
commented
La normalisation est uniquement lexicale et ne résout pas les liens symboliques. Un dépôt contrôlé par l'auteur de la PR peut donc contenir un lien tel que La normalisation est uniquement lexicale et ne résout pas les liens symboliques. Un dépôt contrôlé par l'auteur de la PR peut donc contenir un lien tel que `secret -> /proc/self/environ` ou `secret -> /etc/...`; `read_file` transmettra ensuite ce chemin à `cat` et permettra de lire des fichiers hors du workspace. Il faut résoudre et valider la cible réelle dans le container, ou refuser les liens symboliques avant toute lecture.
|
||||
}
|
||||
|
||||
async fn find(sandbox: &Sandbox, args: &Value) -> anyhow::Result<String> {
|
||||
let pattern = required_str(args, "pattern")?;
|
||||
let path = resolve(sandbox, optional_str(args, "path").unwrap_or("."))?;
|
||||
|
||||
let output = sandbox
|
||||
.exec(&["find", &path, "-type", "f", "-name", pattern])
|
||||
.await?;
|
||||
|
||||
into_stdout(output)
|
||||
}
|
||||
|
||||
/// Resolves a tool path against the container workspace folder, rejecting any
|
||||
/// path that escapes it.
|
||||
fn resolve(sandbox: &Sandbox, path: &str) -> anyhow::Result<String> {
|
||||
let workspace = sandbox.workspace_folder();
|
||||
let candidate = Path::new(workspace).join(path);
|
||||
|
||||
let normalized =
|
||||
normalize(&candidate).with_context(|| format!("path `{path}` escapes the workspace"))?;
|
||||
|
||||
if !normalized.starts_with(workspace) {
|
||||
|
Herald
commented
La vérification de confinement dépend entièrement de La vérification de confinement dépend entièrement de `workspace`, qui est contrôlé par le `devcontainer.json` de la pull request. Une PR peut définir `workspaceFolder` à `/`, après quoi `starts_with(workspace)` autorise pratiquement tout le système de fichiers du container et les outils peuvent lire des fichiers hors dépôt. Le workspace doit être validé et forcé sous un répertoire dédié au dépôt, indépendamment de la configuration non fiable.
Herald
commented
La vérification ne protège pas réellement la frontière du dépôt lorsque La vérification ne protège pas réellement la frontière du dépôt lorsque `workspaceFolder` vient du dépôt non fiable. Une valeur comme `/` ou `/tmp` passe la normalisation et permet aux outils de lire n'importe quel chemin du conteneur, contrairement au contrat annoncé (« confined to the repository workspace »). Il faut imposer un workspace absolu dédié et vérifier qu'il s'agit bien du répertoire de dépôt, plutôt que de faire confiance à `workspaceFolder` fourni par la PR.
Herald
commented
[security] La vérification **[security]** La vérification `normalized.starts_with(workspace)` compare des chaînes et ne respecte pas les frontières de composants. Avec un workspace `/workspaces/repo`, un chemin comme `../repo-secrets` devient `/workspaces/repo-secrets` et passe ce test, ce qui permet aux outils de lire un répertoire voisin dans le container. Utiliser une comparaison de chemins basée sur les composants, par exemple `strip_prefix(workspace).is_ok()` après normalisation.
|
||||
bail!("path `{path}` escapes the workspace");
|
||||
}
|
||||
|
||||
Ok(normalized.display().to_string())
|
||||
}
|
||||
|
||||
fn required_str<'a>(args: &'a Value, key: &str) -> anyhow::Result<&'a str> {
|
||||
args.get(key)
|
||||
.and_then(Value::as_str)
|
||||
.with_context(|| format!("`{key}` is required"))
|
||||
}
|
||||
|
||||
fn optional_str<'a>(args: &'a Value, key: &str) -> Option<&'a str> {
|
||||
args.get(key).and_then(Value::as_str)
|
||||
}
|
||||
|
||||
fn into_stdout(output: ExecOutput) -> anyhow::Result<String> {
|
||||
if output.success() {
|
||||
Ok(output.stdout)
|
||||
} else {
|
||||
bail!(
|
||||
"command failed (status {}): {}",
|
||||
output.status,
|
||||
output.stderr.trim()
|
||||
)
|
||||
}
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
mod tests {
|
||||
use super::*;
|
||||
use crate::gitea::{Comment, PullRequest, Repository, ReviewPayload};
|
||||
|
||||
fn review_webhook() -> WebhookType {
|
||||
WebhookType::Review(ReviewPayload {
|
||||
action: "created".to_string(),
|
||||
pull_request: PullRequest {
|
||||
number: 1,
|
||||
title: "My PR".to_string(),
|
||||
},
|
||||
repository: Repository {
|
||||
full_name: "owner/repo".to_string(),
|
||||
clone_url: "https://github.com/owner/repo.git".to_string(),
|
||||
},
|
||||
comment: Comment {
|
||||
id: 1,
|
||||
body: "@bot review".to_string(),
|
||||
},
|
||||
})
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn review_webhook_exposes_read_only_tools() {
|
||||
let names: Vec<String> = for_webhook(&review_webhook())
|
||||
.into_iter()
|
||||
.map(|tool| tool.function.name)
|
||||
.collect();
|
||||
|
||||
assert_eq!(names, vec!["ls", "file_size", "read_file", "grep", "find"]);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn required_str_reports_missing_key() {
|
||||
let err = required_str(&json!({}), "path").unwrap_err();
|
||||
assert!(err.to_string().contains("path"));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn lines_are_numbered_from_the_first_one() {
|
||||
assert_eq!(number_lines("a\nb\n", 1), "1:a\n2:b");
|
||||
}
|
||||
|
||||
#[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.
|
||||
assert_eq!(number_lines("x\ny", 12), "12:x\n13:y");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn an_empty_read_stays_empty() {
|
||||
assert_eq!(number_lines("", 1), "");
|
||||
}
|
||||
}
|
||||
@@ -0,0 +1,81 @@
|
||||
//! Small text helpers shared by the modules that log what the model answered and
|
||||
//! bound what the tools return to it.
|
||||
|
||||
/// First `limit` characters of `text`, for logs.
|
||||
///
|
||||
/// Cuts on character boundaries, so the excerpt stays valid UTF-8, and marks a
|
||||
/// truncation with an ellipsis.
|
||||
pub fn excerpt(text: &str, limit: usize) -> String {
|
||||
let mut chars = text.chars();
|
||||
let excerpt = chars.by_ref().take(limit).collect::<String>();
|
||||
|
||||
if chars.next().is_some() {
|
||||
return format!("{excerpt}…");
|
||||
}
|
||||
|
||||
excerpt
|
||||
}
|
||||
|
||||
/// Truncates `text` in place to at most `limit` bytes, cutting on a character
|
||||
/// boundary so the result stays valid UTF-8.
|
||||
///
|
||||
/// Returns `true` when something was dropped.
|
||||
pub fn truncate_bytes(text: &mut String, limit: usize) -> bool {
|
||||
if text.len() <= limit {
|
||||
return false;
|
||||
}
|
||||
|
||||
let mut end = limit;
|
||||
while !text.is_char_boundary(end) {
|
||||
end -= 1;
|
||||
}
|
||||
|
||||
text.truncate(end);
|
||||
|
||||
true
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
mod tests {
|
||||
use super::*;
|
||||
|
||||
#[test]
|
||||
fn a_short_text_is_kept_as_is() {
|
||||
assert_eq!(excerpt("abc", 3), "abc");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn a_long_text_is_truncated_and_marked() {
|
||||
assert_eq!(excerpt("abcdef", 3), "abc…");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn truncation_cuts_on_character_boundaries() {
|
||||
assert_eq!(excerpt("ééé", 2), "éé…");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn a_short_text_is_not_truncated() {
|
||||
let mut text = String::from("abc");
|
||||
|
||||
assert!(!truncate_bytes(&mut text, 3));
|
||||
assert_eq!(text, "abc");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn a_long_text_is_truncated_within_the_limit() {
|
||||
let mut text = String::from("abcdef");
|
||||
|
||||
assert!(truncate_bytes(&mut text, 4));
|
||||
assert_eq!(text, "abcd");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn byte_truncation_never_splits_a_character() {
|
||||
let mut text = String::from("ééé");
|
||||
|
||||
// The limit falls in the middle of the second `é`: it is dropped.
|
||||
assert!(truncate_bytes(&mut text, 3));
|
||||
assert_eq!(text, "é");
|
||||
}
|
||||
}
|
||||
@@ -1,214 +0,0 @@
|
||||
use futures_util::stream::TryStreamExt;
|
||||
use tokio::io::AsyncReadExt;
|
||||
use tokio_util::io::StreamReader;
|
||||
use tracing::instrument;
|
||||
|
||||
use crate::{
|
||||
bot::ReviewResult, consts::{BOT_PROCESS_MSG, MAX_DIFF_SIZE, REVIEW_PROMPT}, gitea::{GiteaAPI, ReviewPayload}, metrics, open_router::OpenRouterClient,
|
||||
};
|
||||
|
||||
#[instrument(skip(gitea_api, open_router_client, http_client, review_payload))]
|
||||
pub async fn exec_review(
|
||||
gitea_api: &GiteaAPI,
|
||||
open_router_client: &OpenRouterClient,
|
||||
http_client: &reqwest::Client,
|
||||
model: &str,
|
||||
review_payload: ReviewPayload,
|
||||
) -> 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, anyhow::Error> = async {
|
||||
let git_diff =
|
||||
download_git_diff(http_client, &review_payload.pull_request.diff_url).await?;
|
||||
|
||||
let diff_for_llm = format_diff_for_review(&git_diff);
|
||||
|
||||
let bot_request = REVIEW_PROMPT
|
||||
.replace("{subject}", &review_payload.pull_request.title)
|
||||
.replace("{comment}", &review_payload.comment.body)
|
||||
.replace("{diff}", &diff_for_llm);
|
||||
|
||||
let chat_result = open_router_client.chat(&bot_request).await?;
|
||||
let mut review_result = serde_json::from_str::<ReviewResult>(&chat_result.message)?;
|
||||
|
||||
review_result.cost = chat_result.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)
|
||||
}
|
||||
.await;
|
||||
|
||||
match bot_result {
|
||||
Ok(_) => {
|
||||
gitea_api
|
||||
.delete_comment(&review_payload.repository.full_name, new_comment.id)
|
||||
.await
|
||||
}
|
||||
Err(e) => {
|
||||
gitea_api
|
||||
.edit_comment(
|
||||
&format!("Error while reviewing: {}", e),
|
||||
&review_payload.repository.full_name,
|
||||
new_comment.id,
|
||||
)
|
||||
.await
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
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()
|
||||
));
|
||||
|
||||
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
|
||||
}
|
||||
|
||||
async fn download_git_diff(http_client: &reqwest::Client, url: &str) -> anyhow::Result<String> {
|
||||
let response = http_client.get(url).send().await?;
|
||||
let stream = response.bytes_stream().map_err(std::io::Error::other);
|
||||
|
||||
let mut buf = Vec::with_capacity(MAX_DIFF_SIZE);
|
||||
StreamReader::new(stream)
|
||||
.take((MAX_DIFF_SIZE + 1) as u64)
|
||||
.read_to_end(&mut buf)
|
||||
.await?;
|
||||
|
||||
if buf.len() > MAX_DIFF_SIZE {
|
||||
anyhow::bail!("Git diff exceeds the maximum allowed size of 1 Mo");
|
||||
}
|
||||
|
||||
Ok(String::from_utf8_lossy(&buf).into_owned())
|
||||
}
|
||||
|
||||
fn format_diff_for_review(git_diff: &str) -> String {
|
||||
let mut output = String::new();
|
||||
let mut current_file: Option<&str> = None;
|
||||
let mut new_line: u64 = 0;
|
||||
|
||||
for line in git_diff.lines() {
|
||||
if let Some(rest) = line.strip_prefix("diff --git a/") {
|
||||
if let Some(end) = rest.find(' ') {
|
||||
current_file = Some(&rest[..end]);
|
||||
}
|
||||
new_line = 0;
|
||||
continue;
|
||||
}
|
||||
|
||||
if line.starts_with("---") || line.starts_with("+++") {
|
||||
continue;
|
||||
}
|
||||
|
||||
if line.starts_with("@@") && line.contains('+') {
|
||||
if let Some(start) = parse_hunk_new_start(line) {
|
||||
new_line = start;
|
||||
}
|
||||
continue;
|
||||
}
|
||||
|
||||
let Some(filename) = current_file else {
|
||||
continue;
|
||||
};
|
||||
|
||||
if line.starts_with(' ') {
|
||||
new_line += 1;
|
||||
continue;
|
||||
}
|
||||
|
||||
if let Some(code) = line.strip_prefix('+') {
|
||||
use std::fmt::Write;
|
||||
let _ = writeln!(output, "{filename}:{new_line}:{code}");
|
||||
new_line += 1;
|
||||
}
|
||||
}
|
||||
|
||||
output
|
||||
}
|
||||
|
||||
fn parse_hunk_new_start(hunk_header: &str) -> Option<u64> {
|
||||
let plus_part = hunk_header.split('+').nth(1)?;
|
||||
let num_str = plus_part.split(|c: char| !c.is_ascii_digit()).next()?;
|
||||
num_str.parse::<u64>().ok()
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
#[test]
|
||||
fn test_format_diff_for_review() {
|
||||
let diff = concat!(
|
||||
"diff --git a/src/foo.rs b/src/foo.rs\n",
|
||||
"--- a/src/foo.rs\n",
|
||||
"+++ b/src/foo.rs\n",
|
||||
"@@ -1,3 +1,6 @@\n",
|
||||
" fn main() {\n",
|
||||
"+ let x = 1;\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",
|
||||
);
|
||||
|
||||
let result = format_diff_for_review(diff);
|
||||
let expected = concat!(
|
||||
"src/foo.rs:2: let x = 1;\n",
|
||||
"src/foo.rs:4: let y = 2;\n",
|
||||
"src/foo.rs:5: let z = 3;\n",
|
||||
"src/bar.rs:11: let a = 10;\n",
|
||||
"src/bar.rs:13: let b = 20;\n",
|
||||
);
|
||||
|
||||
assert_eq!(result, expected);
|
||||
}
|
||||
@@ -1,44 +0,0 @@
|
||||
pub const GITEA_SIG_HEADER_NAME: &str = "x-gitea-signature";
|
||||
pub const GITEA_EVENT_TYPE_HEADER_NAME: &str = "x-gitea-event-type";
|
||||
|
||||
pub const MAX_WEBHOOK_BODY_SIZE: usize = 1024 * 1024; // 1 MiB
|
||||
pub const MAX_DIFF_SIZE: usize = 1024 * 1024; // 1 MiB
|
||||
|
||||
pub const BOT_PROCESS_MSG: &str = "
|
||||
Review in progress with the model \"{model}\"...
|
||||
";
|
||||
|
||||
pub const REVIEW_PROMPT: &str = "
|
||||
You are a senior software engineer reviewing code changes.
|
||||
|
||||
Check good practices and code quality.
|
||||
|
||||
This is the pull request subject: \"{subject}\"
|
||||
|
||||
This is the user comment: \"{comment}\"
|
||||
|
||||
The code changes (only added lines, with line numbers):
|
||||
|
||||
{diff}
|
||||
|
||||
Please review the code changes and provide feedback.
|
||||
|
||||
IMPORTANT: the `line` field must be the line number shown before each line.
|
||||
The provided code has the format: `filename:line:code`
|
||||
|
||||
Return your feedback, in french, with only this json format, reviews must contain each review
|
||||
All fields are mandatory.
|
||||
(filename field must contain the full path with extension) and comment must contain a final summary:
|
||||
|
||||
{
|
||||
\"reviews\": [
|
||||
{
|
||||
\"filename\": \"\",
|
||||
\"line\": ,
|
||||
\"code\": \"\",
|
||||
\"message\": \"\"
|
||||
}
|
||||
],
|
||||
\"comment\": \"\"
|
||||
}
|
||||
";
|
||||
@@ -1,50 +0,0 @@
|
||||
use std::time::Duration;
|
||||
|
||||
use openrouter_rs::{Message, api::chat::ChatCompletionRequest};
|
||||
use tracing::instrument;
|
||||
|
||||
pub struct ChatResult {
|
||||
pub message: String,
|
||||
pub cost: Option<f64>,
|
||||
}
|
||||
|
||||
#[derive(Clone)]
|
||||
pub struct OpenRouterClient {
|
||||
client: openrouter_rs::OpenRouterClient,
|
||||
model: String,
|
||||
}
|
||||
|
||||
impl OpenRouterClient {
|
||||
pub fn new(token: &str, model: &str, timeout: u64) -> anyhow::Result<Self> {
|
||||
Ok(Self {
|
||||
client: openrouter_rs::OpenRouterClient::builder()
|
||||
.api_key(token)
|
||||
.http_client(
|
||||
reqwest::Client::builder()
|
||||
.timeout(Duration::from_secs(timeout))
|
||||
.build()?,
|
||||
)
|
||||
.build()?,
|
||||
model: String::from(model),
|
||||
})
|
||||
}
|
||||
|
||||
#[instrument(skip(self), err)]
|
||||
pub async fn chat(&self, msg: &str) -> anyhow::Result<ChatResult> {
|
||||
let request = ChatCompletionRequest::builder()
|
||||
.model(&self.model)
|
||||
.enable_reasoning()
|
||||
.messages(vec![Message::new(openrouter_rs::types::Role::User, msg)])
|
||||
.build()?;
|
||||
|
||||
let response = self.client.chat().create(&request).await?;
|
||||
|
||||
Ok(ChatResult {
|
||||
message: response.choices[0]
|
||||
.content()
|
||||
.map(String::from)
|
||||
.ok_or(anyhow::anyhow!("No content"))?,
|
||||
cost: response.usage.and_then(|u| u.cost),
|
||||
})
|
||||
}
|
||||
}
|
||||
Épingler l'image de base par digest (ou au moins une version majeure mineure) pour la reproductibilité, et éviter d'exécuter le conteneur de développement en root si possible. Vérifier que les outils ajoutés dans cette image ne sont pas supposés présents en production (le devcontainer ne doit pas être une dépendance implicite du runtime).