1.2: Sandboxing #7

Merged
qpismont merged 17 commits from 1.2 into main 2026-09-20 16:58:36 +02:00
16 changed files with 1537 additions and 12 deletions
Showing only changes of commit 99d1c2feef - Show all commits
+5 -1
View File
@@ -18,5 +18,9 @@ SENTRY_DSN=
RUST_LOG=info
RUST_BACKTRACE=1
qpismont marked this conversation as resolved
Review

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.

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`.
METRICS_BIND_ADDR=
# Sandboxed tool execution (optional)
SANDBOX_ENABLED=false
CONTAINER_RUNTIME=docker
SANDBOX_MAX_ITERATIONS=8
Generated
+1
View File
1
@@ -847,6 +847,7 @@ dependencies = [
"sentry-anyhow",
"serde",
"serde_json",
"tempfile",
"thiserror 2.0.20",
"tokio",
"tokio-stream",
+24
View File
@@ -38,6 +38,30 @@ 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` |
qpismont marked this conversation as resolved
Review

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é.
Review

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.
| `SANDBOX_ENABLED` | *(optional)* Run reviews inside a devcontainer sandbox so the model can explore the repository with tools. Defaults to `false` |
| `CONTAINER_RUNTIME` | *(optional)* Container runtime binary used for the sandbox (`docker` or `podman`). Defaults to `docker` |
| `SANDBOX_MAX_ITERATIONS` | *(optional)* Maximum number of tool-calling iterations per sandboxed review. Defaults to `8` |
## Sandboxed reviews
When `SANDBOX_ENABLED=true`, 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. lets the model explore the repository with read-only tools (`ls`, `read_file`,
`grep`, `find`) executed inside the container,
4. posts the review and removes the container and the temporary clone.
The container runtime is selected with `CONTAINER_RUNTIME` (`docker` or
`podman`). The repository must contain a `.devcontainer/devcontainer.json`.
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
qpismont marked this conversation as resolved Outdated
Outdated
Review

L'affirmation « Each sandbox is isolated » est plus forte que la réalité tant que les runArgs/containerEnv du dépôt analysé sont transmis tels quels à docker run (cf. container.rs). Soit durcir le code, soit nuancer la section en listant les limites de l'isolation.

L'affirmation « Each sandbox is isolated » est plus forte que la réalité tant que les `runArgs`/`containerEnv` du dépôt analysé sont transmis tels quels à `docker run` (cf. `container.rs`). Soit durcir le code, soit nuancer la section en listant les limites de l'isolation.
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
+649
View File
@@ -0,0 +1,649 @@
//! Container lifecycle primitives for a parsed [`DevContainer`].
//!
//! This module shells out to a container runtime (`docker` or `podman`) to build
//! the devcontainer image, start a container with the workspace mounted, run the
//! `postCreateCommand` / `postStartCommand` hooks and execute commands inside the
//! running container.
//!
//! It is intentionally runtime-agnostic: any binary exposing the `docker` CLI
//! surface (including `podman`) can be used via [`ContainerRuntime::new`].
//!
//! # Isolation
//!
//! Each sandbox gets its own image tag, its own container and its own network.
//! The container starts attached to that network so the `postCreateCommand` /
//! `postStartCommand` hooks can fetch dependencies (e.g. `npm install`); once the
//! hooks have run, the container is disconnected from the network for the rest of
//! its lifetime. Every command is bounded by a timeout.
use std::{
path::{Path, PathBuf},
process::Stdio,
time::{Duration, SystemTime, UNIX_EPOCH},
};
use tokio::process::Command;
use crate::DevContainer;
/// Timeout applied to build/run/stop/remove operations.
const DEFAULT_COMMAND_TIMEOUT: Duration = Duration::from_secs(600);
/// Timeout applied to commands executed inside a running container.
const DEFAULT_EXEC_TIMEOUT: Duration = Duration::from_secs(60);
/// Result of a command executed inside a container.
#[derive(Debug, Clone)]
pub struct ExecOutput {
/// Exit code, or `-1` if the process was terminated by a signal.
pub status: i32,
pub stdout: String,
pub stderr: String,
}
impl ExecOutput {
pub fn success(&self) -> bool {
self.status == 0
}
/// Turns a non-zero exit code into a [`ContainerError::Command`].
pub fn ensure_success(self, program: &str, args: &[String]) -> Result<Self, ContainerError> {
if self.success() {
return Ok(self);
}
Err(ContainerError::Command {
program: program.to_string(),
args: args.join(" "),
status: self.status,
stderr: self.stderr.trim().to_string(),
})
}
}
#[derive(Debug, thiserror::Error)]
pub enum ContainerError {
#[error("failed to run `{program}`: {source}")]
Spawn {
program: String,
source: std::io::Error,
},
#[error("`{program} {args}` failed with status {status}: {stderr}")]
Command {
program: String,
args: String,
status: i32,
stderr: String,
},
#[error("`{program} {args}` timed out after {timeout:?}")]
Timeout {
Review

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.

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.
Review

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.

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.
Review

[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.

**[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.
program: String,
args: String,
timeout: Duration,
},
}
/// A container runtime binary exposing the `docker` CLI surface.
#[derive(Debug, Clone)]
pub struct ContainerRuntime {
program: String,
timeout: Duration,
}
impl ContainerRuntime {
pub fn new(program: impl Into<String>) -> Self {
Self {
program: program.into(),
timeout: DEFAULT_COMMAND_TIMEOUT,
}
}
pub fn docker() -> Self {
Self::new("docker")
}
pub fn podman() -> Self {
Self::new("podman")
}
/// Overrides the timeout applied to build/run/stop/remove operations.
pub fn with_timeout(mut self, timeout: Duration) -> Self {
self.timeout = timeout;
self
}
pub fn program(&self) -> &str {
&self.program
}
pub fn timeout(&self) -> Duration {
self.timeout
}
/// Checks that the runtime binary is present and responsive.
pub async fn available(&self) -> bool {
Command::new(&self.program)
.arg("version")
.stdin(Stdio::null())
.stdout(Stdio::null())
.stderr(Stdio::null())
.status()
.await
.map(|status| status.success())
.unwrap_or(false)
}
/// Runs the runtime with the given arguments, capturing stdout/stderr.
///
/// Only spawn failures and timeouts are reported as errors; a non-zero exit
/// code is returned in the [`ExecOutput`] so callers can decide how to react.
pub async fn run(&self, args: &[String]) -> Result<ExecOutput, ContainerError> {
self.run_with_timeout(args, self.timeout).await
}
/// Like [`run`](Self::run) with an explicit timeout.
pub async fn run_with_timeout(
&self,
args: &[String],
timeout: Duration,
) -> Result<ExecOutput, ContainerError> {
let output = Command::new(&self.program)
.args(args)
.stdin(Stdio::null())
.kill_on_drop(true)
.output();
match tokio::time::timeout(timeout, output).await {
Ok(Ok(output)) => Ok(ExecOutput {
status: output.status.code().unwrap_or(-1),
stdout: String::from_utf8_lossy(&output.stdout).into_owned(),
stderr: String::from_utf8_lossy(&output.stderr).into_owned(),
}),
Ok(Err(source)) => Err(ContainerError::Spawn {
program: self.program.clone(),
source,
}),
Err(_) => Err(ContainerError::Timeout {
program: self.program.clone(),
args: args.join(" "),
timeout,
}),
}
}
}
/// A running devcontainer.
#[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 {
pub fn name(&self) -> &str {
&self.name
}
pub fn workspace_folder(&self) -> &str {
&self.workspace_folder
}
/// Executes a command inside the container, returning its output.
///
/// The command is passed as an argv vector (no shell), so no quoting or
/// interpolation is performed.
pub async fn exec(&self, cmd: &[&str]) -> Result<ExecOutput, ContainerError> {
self.exec_with_timeout(cmd, DEFAULT_EXEC_TIMEOUT).await
}
async fn exec_with_timeout(
&self,
cmd: &[&str],
timeout: Duration,
) -> Result<ExecOutput, ContainerError> {
let mut args = vec!["exec".to_string()];
if let Some(user) = &self.remote_user {
args.push("--user".to_string());
args.push(user.clone());
}
args.push(self.name.clone());
args.extend(cmd.iter().map(|arg| arg.to_string()));
self.runtime.run_with_timeout(&args, timeout).await
}
/// Executes a shell script inside the container via `sh -c`.
pub async fn exec_shell(&self, script: &str) -> Result<ExecOutput, ContainerError> {
self.exec(&["sh", "-c", script]).await
}
/// Stops the container.
pub async fn stop(&self) -> Result<(), ContainerError> {
let args = vec!["stop".to_string(), self.name.clone()];
self.runtime
.run(&args)
.await?
.ensure_success(self.runtime.program(), &args)?;
Ok(())
}
/// Removes the container, its network and its image.
///
/// Network and image removal are best-effort: they may already be gone.
pub async fn remove(&self) -> Result<(), ContainerError> {
let args = vec![
"rm".to_string(),
"-f".to_string(),
"-v".to_string(),
self.name.clone(),
];
self.runtime
.run(&args)
.await?
.ensure_success(self.runtime.program(), &args)?;
if let Some(network) = &self.network {
let args = vec!["network".to_string(), "rm".to_string(), network.clone()];
let _ = self.runtime.run(&args).await;
}
if let Some(image) = &self.image {
let args = vec!["rmi".to_string(), image.clone()];
let _ = self.runtime.run(&args).await;
}
Ok(())
}
}
impl DevContainer {
/// Base image name derived from the devcontainer name.
pub fn image_name(&self) -> String {
let base = self.name.as_deref().unwrap_or("devcontainer");
format!("devcontainer-rs/{}", sanitize(base))
}
/// Unique image tag for a single sandbox run.
///
/// Uniqueness matters: two concurrent sandboxes (possibly for different
/// repositories sharing a devcontainer name) must not race on a shared tag.
pub fn image_tag(&self) -> String {
format!("{}:{}", self.image_name(), unique_suffix())
}
/// Arguments passed to `docker build` (everything after the `build` verb).
pub fn build_args(&self, image_tag: &str) -> Vec<String> {
let context = self
.container_file_path
.parent()
.unwrap_or_else(|| Path::new("."));
let mut args = vec![
"-f".to_string(),
self.container_file_path.display().to_string(),
"-t".to_string(),
image_tag.to_string(),
];
for (key, value) in &self.build_args {
args.push("--build-arg".to_string());
args.push(format!("{key}={value}"));
}
args.push(context.display().to_string());
args
}
/// Arguments passed to `docker run` (everything after the `run` verb).
pub fn run_args(
&self,
workspace_dir: &Path,
container_name: &str,
image_tag: &str,
network: Option<&str>,
) -> Vec<String> {
let workspace_folder = self.workspace_folder();
let mut args = vec![
"-d".to_string(),
"--name".to_string(),
container_name.to_string(),
"-v".to_string(),
format!("{}:{}", workspace_dir.display(), workspace_folder),
"-w".to_string(),
workspace_folder.to_string(),
];
if let Some(network) = network {
args.push("--network".to_string());
qpismont marked this conversation as resolved Outdated
Outdated
Review

L'option --network est insérée avant self.run_args, qui sont concaténés ensuite (ligne 340) : dans docker run, la dernière option gagne, donc un runArgs du dépôt peut remplacer le réseau dédié et contourner la coupure réseau effectuée en fin de up(). Documenter/verrouiller cet ordre si l'isolation réseau doit être garantie.

L'option `--network` est insérée avant `self.run_args`, qui sont concaténés ensuite (ligne 340) : dans `docker run`, la dernière option gagne, donc un `runArgs` du dépôt peut remplacer le réseau dédié et contourner la coupure réseau effectuée en fin de `up()`. Documenter/verrouiller cet ordre si l'isolation réseau doit être garantie.
args.push(network.to_string());
}
if let Some(user) = &self.remote_user {
args.push("--user".to_string());
args.push(user.clone());
}
for (key, value) in &self.container_env {
args.push("-e".to_string());
args.push(format!("{key}={value}"));
}
args.extend(self.run_args.iter().cloned());
qpismont marked this conversation as resolved Outdated
Outdated
Review

Les runArgs proviennent du devcontainer.json du dépôt analysé, donc d'une PR potentiellement issue d'un fork : ils sont concaténés tels quels aux arguments de docker run. Un runArgs du type ["--privileged", "-v", "/:/host"] (ou --pid=host, --cap-add=SYS_ADMIN) monte l'hôte dans le conteneur et annule totalement l'isolation annoncée. Comme ils sont ajoutés après le --network posé ligne 326, ils peuvent aussi le surcharger (--network host). Prévoir une liste blanche d'options (ou ignorer runArgs/containerEnv pour les dépôts non fiables) avant de démarrer le conteneur.

Les `runArgs` proviennent du `devcontainer.json` du dépôt analysé, donc d'une PR potentiellement issue d'un fork : ils sont concaténés tels quels aux arguments de `docker run`. Un `runArgs` du type `["--privileged", "-v", "/:/host"]` (ou `--pid=host`, `--cap-add=SYS_ADMIN`) monte l'hôte dans le conteneur et annule totalement l'isolation annoncée. Comme ils sont ajoutés après le `--network` posé ligne 326, ils peuvent aussi le surcharger (`--network host`). Prévoir une liste blanche d'options (ou ignorer `runArgs`/`containerEnv` pour les dépôts non fiables) avant de démarrer le conteneur.
args.push(image_tag.to_string());
// Keep the container alive so we can `exec` into it.
args.push("sleep".to_string());
args.push("infinity".to_string());
args
}
/// Workspace folder inside the container, defaulting to `/workspaces/workspace`.
pub fn workspace_folder(&self) -> String {
self.workspace_folder
.clone()
.unwrap_or_else(|| "/workspaces/workspace".to_string())
}
/// Unique container name for this run.
pub fn container_name(&self) -> String {
let base = self.name.as_deref().unwrap_or("devcontainer");
format!("devcontainer-rs-{}-{}", sanitize(base), unique_suffix())
}
/// Builds the devcontainer image under `image_tag`.
pub async fn build(
&self,
runtime: &ContainerRuntime,
image_tag: &str,
) -> Result<(), ContainerError> {
let mut args = vec!["build".to_string()];
args.extend(self.build_args(image_tag));
runtime
.run(&args)
.await?
.ensure_success(runtime.program(), &args)?;
Ok(())
}
/// Builds the image, starts the container with the workspace mounted, runs
/// the `postCreateCommand` / `postStartCommand` hooks with network access,
/// then disconnects the container from the network.
///
/// On any failure the container, network and image are cleaned up before
/// returning, so no resource is leaked.
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");
// Dedicated network so connectivity can be cut after the hooks.
let args = vec!["network".to_string(), "create".to_string(), network.clone()];
if let Err(err) = runtime
.run(&args)
.await
.and_then(|out| out.ensure_success(runtime.program(), &args))
{
let _ = runtime.run(&["rmi".to_string(), image_tag]).await;
return Err(err);
}
let mut args = vec!["run".to_string()];
args.extend(self.run_args(workspace_dir, &name, &image_tag, Some(&network)));
if let Err(err) = runtime
.run(&args)
.await
.and_then(|out| out.ensure_success(runtime.program(), &args))
{
let _ = runtime
.run(&["network".to_string(), "rm".to_string(), network])
.await;
let _ = runtime.run(&["rmi".to_string(), image_tag]).await;
return Err(err);
}
let container = Container {
runtime: runtime.clone(),
name,
workspace_folder: self.workspace_folder(),
remote_user: self.remote_user.clone(),
network: Some(network.clone()),
image: Some(image_tag),
};
// Hooks run with network access (dependency installation, etc.).
if let Err(err) = self.run_hooks(&container).await {
let _ = container.remove().await;
return Err(err);
}
// Cut network access for the rest of the sandbox lifetime.
let args = vec![
"network".to_string(),
"disconnect".to_string(),
network,
container.name.clone(),
];
if let Err(err) = runtime
.run(&args)
.await
.and_then(|out| out.ensure_success(runtime.program(), &args))
{
let _ = container.remove().await;
return Err(err);
}
Ok(container)
}
async fn run_hooks(&self, container: &Container) -> Result<(), ContainerError> {
// Hooks may install dependencies, so they get the long command timeout
// rather than the short one used for tool execution.
let timeout = container.runtime.timeout();
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?;
output.ensure_success(
container.runtime.program(),
&["exec".to_string(), command.clone()],
)?;
}
Ok(())
}
}
/// Sanitizes a string so it can be used as a docker image/container name.
fn sanitize(input: &str) -> String {
qpismont marked this conversation as resolved
Review

La fonction sanitize tronque les tirets et points en début et fin, mais elle pourrait laisser passer des séquences de deux tirets consécutifs qui sont autorisées par Docker. Ce n’est pas critique, mais une validation plus stricte (limiter la longueur, interdire les séquences interdites) améliorerait la robustesse.

La fonction `sanitize` tronque les tirets et points en début et fin, mais elle pourrait laisser passer des séquences de deux tirets consécutifs qui sont autorisées par Docker. Ce n’est pas critique, mais une validation plus stricte (limiter la longueur, interdire les séquences interdites) améliorerait la robustesse.
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()
}
}
/// Suffix unique to a sandbox run, combining the process id and a timestamp.
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)
}
/// Lexically normalizes a path, resolving `.` and `..` without touching the
/// filesystem. Returns `None` if the path escapes its root.
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::*;
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",
"runArgs": ["--userns=keep-id"]
}"#,
)
.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 build_args_include_dockerfile_tag_build_args_and_context() {
let dir = tempfile::tempdir().unwrap();
let dc = devcontainer(dir.path());
let args = dc.build_args("devcontainer-rs/my-project:test");
assert_eq!(args[0], "-f");
assert!(args[1].ends_with("Dockerfile"));
assert_eq!(args[2], "-t");
assert_eq!(args[3], "devcontainer-rs/my-project:test");
assert!(args.contains(&"--build-arg".to_string()));
assert!(args.contains(&"VERSION=1".to_string()));
assert_eq!(args.last().unwrap(), &dir.path().display().to_string());
}
#[test]
fn run_args_mount_workspace_and_keep_alive() {
let dir = tempfile::tempdir().unwrap();
let dc = devcontainer(dir.path());
let workspace = Path::new("/tmp/clone");
let args = dc.run_args(
workspace,
"devcontainer-rs-my-project-42",
"devcontainer-rs/my-project:test",
Some("sandbox-net"),
);
assert!(args.contains(&"-d".to_string()));
assert!(args.contains(&"--name".to_string()));
assert!(args.contains(&"devcontainer-rs-my-project-42".to_string()));
assert!(args.contains(&"/tmp/clone:/workspaces/my-project".to_string()));
assert!(args.contains(&"--network".to_string()));
assert!(args.contains(&"sandbox-net".to_string()));
assert!(args.contains(&"--user".to_string()));
assert!(args.contains(&"dev".to_string()));
assert!(args.contains(&"RUST_LOG=debug".to_string()));
assert!(args.contains(&"--userns=keep-id".to_string()));
assert!(args.contains(&"devcontainer-rs/my-project:test".to_string()));
assert_eq!(
&args[args.len() - 2..],
&["sleep".to_string(), "infinity".to_string()]
);
}
#[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);
}
#[tokio::test]
async fn run_captures_output() {
let runtime = ContainerRuntime::new("echo");
let output = runtime.run(&["hello".to_string()]).await.unwrap();
assert!(output.success());
assert_eq!(output.stdout.trim(), "hello");
}
#[tokio::test]
async fn run_times_out_and_kills_the_process() {
let runtime = ContainerRuntime::new("sleep");
let err = runtime
.run_with_timeout(&["10".to_string()], Duration::from_millis(50))
.await
.unwrap_err();
assert!(matches!(err, ContainerError::Timeout { .. }));
}
}
+99 -3
View File
@@ -5,6 +5,10 @@ use std::{
use serde::Deserialize;
mod container;
pub use container::{Container, ContainerError, ContainerRuntime, ExecOutput, normalize};
#[derive(Debug, Deserialize)]
pub struct DevContainerBuildSchema {
#[serde(default)]
1
@@ -30,6 +34,12 @@ pub struct DevContainerSchema {
#[serde(rename = "postStartCommand", default)]
pub post_start_command: Option<String>,
#[serde(rename = "remoteUser", default)]
pub remote_user: Option<String>,
#[serde(rename = "runArgs", default)]
pub run_args: Vec<String>,
}
#[derive(Debug)]
@@ -41,6 +51,8 @@ pub struct DevContainer {
pub workspace_folder: Option<String>,
pub post_create_command: Option<String>,
pub post_start_command: Option<String>,
pub remote_user: Option<String>,
pub run_args: Vec<String>,
}
#[derive(Debug, thiserror::Error)]
1
@@ -80,18 +92,72 @@ impl TryFrom<(DevContainerSchema, PathBuf)> for DevContainer {
return Err(ParseError::ContainerFileNotFound(container_file_path));
}
let build_args = schema
.build
.args
qpismont marked this conversation as resolved Outdated
Outdated
Review

Si aucun fichier Dockerfile ou Containerfile n'existe, la fonction retourne base_dir.join("Dockerfile"), ce qui échouera plus tard avec ContainerFileNotFound. Cela pourrait être plus explicite en retournant une Option<PathBuf> et en laissant l'appelant gérer l'absence de fichier. Sinon, le message d'erreur actuel est suffisant.

Si aucun fichier `Dockerfile` ou `Containerfile` n'existe, la fonction retourne `base_dir.join("Dockerfile")`, ce qui échouera plus tard avec `ContainerFileNotFound`. Cela pourrait être plus explicite en retournant une `Option<PathBuf>` et en laissant l'appelant gérer l'absence de fichier. Sinon, le message d'erreur actuel est suffisant.
.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)))
qpismont marked this conversation as resolved Outdated
Outdated
Review

La fonction parse est asynchrone, mais les tests unitaires ne la testent pas directement (ils testent TryFrom). Il serait bon d'ajouter un test asynchrone pour parse afin de couvrir le flux complet (lecture du fichier, désérialisation, conversion).

La fonction `parse` est asynchrone, mais les tests unitaires ne la testent pas directement (ils testent `TryFrom`). Il serait bon d'ajouter un test asynchrone pour `parse` afin de couvrir le flux complet (lecture du fichier, désérialisation, conversion).
.collect();
Ok(Self {
container_file_path,
name: schema.name,
build_args: schema.build.args,
container_env: schema.container_env,
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,
run_args: schema.run_args,
})
}
}
/// Resolves `${localEnv:VAR}` and `${localEnv:VAR:default}` references using the
/// current process environment, as described by the devcontainer specification.
/// Unresolved variables without a default expand to an empty string.
fn substitute_local_env(input: &str) -> String {
const PREFIX: &str = "${localEnv:";
let mut out = String::with_capacity(input.len());
let mut rest = input;
qpismont marked this conversation as resolved Outdated
Outdated
Review

Les tests sont synchrones et utilisent std::fs plutôt que tokio::fs. C'est acceptable pour l'instant, mais si l'implémentation venait à dépendre de fonctionnalités asynchrones (comme tokio::fs), les tests pourraient ne plus être représentatifs.

Les tests sont synchrones et utilisent `std::fs` plutôt que `tokio::fs`. C'est acceptable pour l'instant, mais si l'implémentation venait à dépendre de fonctionnalités asynchrones (comme `tokio::fs`), les tests pourraient ne plus être représentatifs.
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];
let (key, default) = match inner.split_once(':') {
Some((key, default)) => (key, Some(default)),
None => (inner, None),
};
match std::env::var(key) {
Ok(value) => out.push_str(&value),
Err(_) => 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)
@@ -136,7 +202,9 @@ mod tests {
"workspaceFolder": "/workspace",
"containerEnv": {
"RUST_LOG": "debug"
}
},
"remoteUser": "dev",
"runArgs": ["--userns=keep-id"]
}"#,
)
.unwrap();
@@ -148,5 +216,33 @@ mod tests {
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"));
assert_eq!(config.run_args, vec!["--userns=keep-id"]);
}
#[test]
fn substitutes_local_env_with_default() {
unsafe { std::env::set_var("DEVCONTAINER_TEST_UID", "1000") };
assert_eq!(
substitute_local_env("${localEnv:DEVCONTAINER_TEST_UID}"),
"1000"
);
assert_eq!(
substitute_local_env("uid=${localEnv:DEVCONTAINER_TEST_UID}"),
"uid=1000"
);
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("no variables here"),
"no variables here"
);
}
}
+1
View File
@@ -28,3 +28,4 @@ bytes = { workspace = true }
metrics = { workspace = true }
qpismont marked this conversation as resolved
Review

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).

`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`).
metrics-exporter-prometheus = { workspace = true }
devcontainer-rs = { path = "../devcontainer-rs" }
tempfile = "3"
+8
View File
@@ -2,6 +2,7 @@ use crate::{
gitea::{GiteaAPI, WebhookType},
qpismont marked this conversation as resolved
Review

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.

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 std::{collections::HashSet, sync::Arc};
@@ -31,6 +32,7 @@ pub struct Bot {
http_client: reqwest::Client,
max_concurrent: usize,
open_router_model: String,
sandbox: SandboxConfig,
actions_handled: Arc<Mutex<HashSet<u64>>>,
}
@@ -42,6 +44,7 @@ impl Bot {
http_client: reqwest::Client,
max_concurrent: usize,
open_router_model: String,
sandbox: SandboxConfig,
) -> Self {
Self {
bot_name,
@@ -50,6 +53,7 @@ impl Bot {
http_client,
max_concurrent,
open_router_model,
sandbox,
actions_handled: Arc::new(Mutex::new(HashSet::new())),
}
}
@@ -113,12 +117,16 @@ 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,
),
}
+74 -5
View File
@@ -1,22 +1,33 @@
use futures_util::stream::TryStreamExt;
qpismont marked this conversation as resolved Outdated
Outdated
Review

Action de review : ajouter des tests unitaires sur la construction du prompt, la gestion des erreurs et les cas limites (contenu vide, réponse LLM tronquée, échec du sandbox). Prévoir aussi une limite de taille de la réponse publiée dans Gitea pour éviter de dépasser les quotas de l'API.

Action de review : ajouter des tests unitaires sur la construction du prompt, la gestion des erreurs et les cas limites (contenu vide, réponse LLM tronquée, échec du sandbox). Prévoir aussi une limite de taille de la réponse publiée dans Gitea pour éviter de dépasser les quotas de l'API.
use openrouter_rs::types::Tool;
use tokio::io::AsyncReadExt;
use tokio_util::io::StreamReader;
use tracing::instrument;
use tracing::{info, instrument, warn};
use crate::{
bot::ReviewResult,
consts::{BOT_PROCESS_MSG, MAX_DIFF_SIZE, REVIEW_PROMPT},
consts::{BOT_PROCESS_MSG, MAX_DIFF_SIZE, REVIEW_PROMPT, SANDBOX_SYSTEM_PROMPT},
gitea::{GiteaAPI, ReviewPayload},
metrics,
open_router::OpenRouterClient,
sandbox::{Sandbox, SandboxConfig, agent},
};
#[instrument(skip(gitea_api, open_router_client, http_client, review_payload))]
#[instrument(skip(
gitea_api,
open_router_client,
http_client,
sandbox_config,
tools,
review_payload
))]
pub async fn exec_review(
gitea_api: &GiteaAPI,
open_router_client: &OpenRouterClient,
http_client: &reqwest::Client,
model: &str,
sandbox_config: &SandboxConfig,
tools: Vec<Tool>,
review_payload: ReviewPayload,
) -> anyhow::Result<()> {
tracing::info!(
@@ -45,10 +56,24 @@ pub async fn exec_review(
.replace("{comment}", &review_payload.comment.body)
.replace("{diff}", &diff_for_llm);
let (message, cost) = if sandbox_config.enabled {
run_sandboxed_review(
gitea_api,
open_router_client,
sandbox_config,
tools,
&review_payload,
&bot_request,
)
.await?
} else {
let chat_result = open_router_client.chat(&bot_request).await?;
let mut review_result = serde_json::from_str::<ReviewResult>(&chat_result.message)?;
(chat_result.message, chat_result.cost)
};
review_result.cost = chat_result.cost;
let mut review_result = serde_json::from_str::<ReviewResult>(&message)?;
review_result.cost = cost;
if let Some(cost) = review_result.cost {
metrics::openrouter_cost_usd(cost);
}
@@ -86,6 +111,50 @@ pub async fn exec_review(
}
}
/// Runs the review inside a sandbox container, letting the model explore the
/// repository with tools before answering.
async fn run_sandboxed_review(
gitea_api: &GiteaAPI,
open_router_client: &OpenRouterClient,
sandbox_config: &SandboxConfig,
tools: Vec<Tool>,
review_payload: &ReviewPayload,
bot_request: &str,
) -> anyhow::Result<(String, Option<f64>)> {
let repo_url = gitea_api.repo_clone_url(&review_payload.repository.full_name);
let sandbox = Sandbox::create(
&sandbox_config.runtime,
&repo_url,
gitea_api.token(),
review_payload.pull_request.number,
)
.await?;
let result = agent::run(
open_router_client,
&sandbox,
tools,
SANDBOX_SYSTEM_PROMPT,
bot_request,
sandbox_config.max_iterations,
)
.await;
if let Err(err) = sandbox.cleanup().await {
warn!(%err, "Failed to clean up sandbox container");
}
let result = result?;
info!(
iterations = result.iterations,
cost = ?result.cost,
"Sandboxed review finished"
);
Ok((result.message, result.cost))
}
fn review_result_to_markdown(review_result: &ReviewResult) -> String {
if review_result.reviews.is_empty() {
return String::from("No issues found. ✅");
+10
View File
1
@@ -8,6 +8,16 @@ 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.
The repository is checked out in your working directory. Use the provided
tools (ls, read_file, grep, find) to explore the code and gather the context
you need before answering. Paths are relative to the repository root.
When you have enough information, answer with the requested JSON only.
";
pub const REVIEW_PROMPT: &str = "
You are a senior software engineer reviewing code changes.
+15
View File
@@ -12,6 +12,9 @@ pub struct EnvConfig {
pub gitea_token: String,
pub gitea_timeout: u64,
pub metrics_bind_addr: Option<String>,
pub container_runtime: String,
qpismont marked this conversation as resolved
Review

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.
Review

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.

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 sandbox_enabled: bool,
pub sandbox_max_iterations: usize,
}
pub fn load_config() -> anyhow::Result<EnvConfig> {
@@ -25,6 +28,15 @@ 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 container_runtime =
std::env::var("CONTAINER_RUNTIME").unwrap_or_else(|_| "docker".to_string());
qpismont marked this conversation as resolved
Review

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.

`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.
let sandbox_enabled = std::env::var("SANDBOX_ENABLED")
.map(|value| matches!(value.as_str(), "1" | "true" | "yes"))
.unwrap_or(false);
let sandbox_max_iterations = std::env::var("SANDBOX_MAX_ITERATIONS")
.ok()
.and_then(|value| value.parse().ok())
.unwrap_or(8);
Ok(EnvConfig {
http_port,
@@ -37,6 +49,9 @@ pub fn load_config() -> anyhow::Result<EnvConfig> {
gitea_token,
gitea_timeout,
metrics_bind_addr,
container_runtime,
sandbox_enabled,
sandbox_max_iterations,
})
}
+16
View File
@@ -9,6 +9,7 @@ use crate::{bot::ReviewResult, errors::AppError};
#[derive(Clone)]
pub struct GiteaAPI {
base_url: String,
token: String,
client: reqwest::Client,
}
@@ -22,6 +23,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 +31,20 @@ impl GiteaAPI {
})
}
/// API token used to authenticate against Gitea.
pub fn token(&self) -> &str {
&self.token
}
/// HTTPS clone URL for a repository, suitable for `git clone`.
pub fn repo_clone_url(&self, full_name: &str) -> String {
format!(
"{}/{}.git",
self.base_url.trim_end_matches('/'),
full_name.trim_start_matches('/')
)
}
#[instrument(skip(self))]
pub async fn get_authorized_user(&self) -> anyhow::Result<User> {
let url = format!("{}/api/v1/user", self.base_url);
+16
View File
1
@@ -2,6 +2,7 @@ use crate::{
bot::Bot,
gitea::{GiteaAPI, WebhookType},
open_router::OpenRouterClient,
sandbox::SandboxConfig,
state::AppState,
};
@@ -20,6 +21,7 @@ mod errors;
mod gitea;
mod metrics;
mod open_router;
mod sandbox;
mod state;
fn main() -> anyhow::Result<()> {
@@ -76,6 +78,19 @@ async fn run() -> anyhow::Result<()> {
let shutdown = CancellationToken::new();
let sandbox = SandboxConfig {
enabled: config.sandbox_enabled,
runtime: devcontainer_rs::ContainerRuntime::new(config.container_runtime.clone()),
max_iterations: config.sandbox_max_iterations,
};
qpismont marked this conversation as resolved Outdated
Outdated
Review

L'absence de runtime conteneur ne produit qu'un warn : le service démarre normalement alors que toutes les revues échoueront ensuite. Le contrôle étant déjà fait au démarrage, un échec rapide (fail-fast) ou un mode dégradé explicite serait plus sûr et plus lisible.

L'absence de runtime conteneur ne produit qu'un `warn` : le service démarre normalement alors que toutes les revues échoueront ensuite. Le contrôle étant déjà fait au démarrage, un échec rapide (fail-fast) ou un mode dégradé explicite serait plus sûr et plus lisible.
if sandbox.enabled && !sandbox.runtime.available().await {
warn!(
runtime = sandbox.runtime.program(),
"Sandbox is enabled but the container runtime is not available"
);
}
let bot = Bot::new(
gitea_user.login,
gitea_api,
@@ -83,6 +98,7 @@ async fn run() -> anyhow::Result<()> {
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);
+43 -1
View File
@@ -1,6 +1,10 @@
use std::time::Duration;
qpismont marked this conversation as resolved
Review

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é.
Review

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.

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.
use openrouter_rs::{Message, api::chat::ChatCompletionRequest};
use openrouter_rs::{
Message,
api::chat::ChatCompletionRequest,
types::{Tool, ToolCall},
};
use tracing::instrument;
pub struct ChatResult {
@@ -8,6 +12,12 @@ pub struct ChatResult {
pub cost: Option<f64>,
}
pub struct ToolChatResult {
pub message: Option<String>,
pub tool_calls: Vec<ToolCall>,
pub cost: Option<f64>,
}
#[derive(Clone)]
pub struct OpenRouterClient {
client: openrouter_rs::OpenRouterClient,
@@ -47,4 +57,36 @@ impl OpenRouterClient {
cost: response.usage.and_then(|u| u.cost),
})
}
/// Sends a conversation with tool definitions and returns either a final
/// message or the tool calls requested by the model.
#[instrument(skip(self, messages, tools), err)]
pub async fn chat_with_tools(
&self,
messages: Vec<Message>,
tools: Vec<Tool>,
) -> anyhow::Result<ToolChatResult> {
let request = ChatCompletionRequest::builder()
.model(&self.model)
.enable_reasoning()
.messages(messages)
.tools(tools)
.tool_choice_auto()
.build()?;
let response = self.client.chat().create(&request).await?;
let choice = response
.choices
.first()
.ok_or_else(|| anyhow::anyhow!("No choices in response"))?;
Ok(ToolChatResult {
message: choice.content().map(String::from),
tool_calls: choice
.tool_calls()
.map(<[ToolCall]>::to_vec)
.unwrap_or_default(),
cost: response.usage.and_then(|u| u.cost),
})
}
}
+129
View File
@@ -0,0 +1,129 @@
//! Tool-calling loop driving the model against a [`Sandbox`].
qpismont marked this conversation as resolved
Review

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.
Review

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 anyhow::Context;
use openrouter_rs::{
Message,
types::{Role, Tool, ToolCall},
};
use serde_json::Value;
use tracing::{debug, warn};
use crate::{
open_router::OpenRouterClient,
sandbox::{Sandbox, tools},
};
/// Final output of an agent run.
pub struct AgentResult {
pub message: String,
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`]).
pub async fn run(
open_router: &OpenRouterClient,
sandbox: &Sandbox,
tool_definitions: Vec<Tool>,
system_prompt: &str,
user_prompt: &str,
max_iterations: usize,
) -> anyhow::Result<AgentResult> {
let mut messages = vec![
Message::new(Role::System, system_prompt),
Message::new(Role::User, user_prompt),
];
let mut total_cost = 0.0_f64;
let mut has_cost = false;
qpismont marked this conversation as resolved Outdated
Outdated
Review

messages.clone() et tool_definitions.clone() à chaque itération recopient toute la conversation à chaque tour (coût O(n²)) alors qu'un passage par référence (&[Message]) suffirait. Par ailleurs rien ne borne la taille du contenu renvoyé par les outils : ajouter un plafond par appel pour maîtriser le contexte et le coût.

`messages.clone()` et `tool_definitions.clone()` à chaque itération recopient toute la conversation à chaque tour (coût O(n²)) alors qu'un passage par référence (`&[Message]`) suffirait. Par ailleurs rien ne borne la taille du contenu renvoyé par les outils : ajouter un plafond par appel pour maîtriser le contexte et le coût.
for iteration in 1..=max_iterations {
let response = open_router
.chat_with_tools(messages.clone(), tool_definitions.clone())
.await?;
if let Some(cost) = response.cost {
total_cost += cost;
has_cost = true;
}
if response.tool_calls.is_empty() {
return Ok(AgentResult {
message: response.message.unwrap_or_default(),
cost: has_cost.then_some(total_cost),
iterations: iteration,
});
}
messages.push(Message::assistant_with_tool_calls(
response.message.unwrap_or_default(),
response.tool_calls.clone(),
));
for call in &response.tool_calls {
debug!(tool = call.name(), "Executing tool call");
let content = execute(sandbox, call).await;
messages.push(Message::tool_response(call.id(), content));
}
}
warn!(max_iterations, "Agent reached the iteration limit");
anyhow::bail!("agent exceeded the maximum number of iterations ({max_iterations})")
}
async fn execute(sandbox: &Sandbox, call: &ToolCall) -> String {
let args = match parse_args(call) {
Ok(args) => args,
Err(err) => return format!("error: {err}"),
};
match tools::dispatch(sandbox, call.name(), &args).await {
Ok(output) => output,
Err(err) => format!("error: {err}"),
}
}
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::*;
#[test]
fn parse_args_accepts_empty_arguments() {
let call = ToolCall::new("id", "ls", "");
assert_eq!(
parse_args(&call).unwrap(),
Value::Object(serde_json::Map::new())
);
}
#[test]
fn parse_args_parses_json_object() {
let call = ToolCall::new("id", "ls", r#"{"path":"src"}"#);
assert_eq!(parse_args(&call).unwrap()["path"], "src");
}
#[test]
fn parse_args_rejects_invalid_json() {
let call = ToolCall::new("id", "ls", "not json");
assert!(parse_args(&call).is_err());
}
}
+197
View File
@@ -0,0 +1,197 @@
//! Sandboxed tool execution for the AI bot.
qpismont marked this conversation as resolved
Review

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).

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).
Review

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.

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;
pub mod tools;
use std::{
path::{Path, PathBuf},
process::Stdio,
};
use anyhow::Context;
use devcontainer_rs::{Container, ContainerRuntime, ExecOutput};
use tempfile::TempDir;
use tracing::{info, instrument};
qpismont marked this conversation as resolved
Review

Attention aux chemins : tous les fichiers créés ou montés dans le sandbox doivent être confinés sous une racine contrôlée (canonicalisation + vérification de préfixe) pour empêcher toute traversée de répertoire (.., liens symboliques). Un chemin dérivé d'un nom de fichier d'une PR est une entrée non fiable.

Attention aux chemins : tous les fichiers créés ou montés dans le sandbox doivent être confinés sous une racine contrôlée (canonicalisation + vérification de préfixe) pour empêcher toute traversée de répertoire (`..`, liens symboliques). Un chemin dérivé d'un nom de fichier d'une PR est une entrée non fiable.
/// 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 {
/// Whether the bot should run its tools inside a sandbox container.
pub enabled: bool,
/// Container runtime binary to drive (e.g. `docker`, `podman`).
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,
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,
repo_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(repo_url, token, pull_request_number, &repo_dir).await?;
let devcontainer_path = find_devcontainer(&repo_dir)
.with_context(|| format!("no devcontainer found in `{repo_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?;
Ok(Self {
_workspace: workspace,
container,
})
}
/// Executes a command in the container as an argv vector (no shell).
pub async fn exec(&self, cmd: &[&str]) -> anyhow::Result<ExecOutput> {
Review

[Élevé] Si check_workspace() échoue, le ? abandonne Sandbox après le démarrage du conteneur, mais Container n'a pas de Drop capable de supprimer les ressources asynchrones. Le conteneur, le réseau et l'image restent donc potentiellement sur le daemon à chaque échec de vérification. Nettoyer explicitement le conteneur avant de retourner l'erreur, avec conservation de l'erreur initiale même si le nettoyage échoue.

[Élevé] Si `check_workspace()` échoue, le `?` abandonne `Sandbox` après le démarrage du conteneur, mais `Container` n'a pas de `Drop` capable de supprimer les ressources asynchrones. Le conteneur, le réseau et l'image restent donc potentiellement sur le daemon à chaque échec de vérification. Nettoyer explicitement le conteneur avant de retourner l'erreur, avec conservation de l'erreur initiale même si le nettoyage échoue.
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<()> {
qpismont marked this conversation as resolved Outdated
Outdated
Review

Le chemin dest est converti en chaîne via display(), ce qui peut planter si le chemin contient des caractères non-UTF-8. Dans un contexte Linux, c’est très rare, mais il serait plus sûr d’utiliser to_string_lossy().

Le chemin `dest` est converti en chaîne via `display()`, ce qui peut planter si le chemin contient des caractères non-UTF-8. Dans un contexte Linux, c’est très rare, mais il serait plus sûr d’utiliser `to_string_lossy()`.
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(())
}
/// Runs git with the token injected through `http.extraHeader`, keeping the
/// secret out of the process arguments.
qpismont marked this conversation as resolved
Review

Le token d’authentification est passé via GIT_CONFIG_VALUE_0. C’est correct car il n’apparaît pas dans les arguments. Toutefois, en cas d’erreur, git peut afficher la ligne de commande dans les logs, ce qui pourrait exposer le token indirectement via l’URL de clonage. Envisager d’utiliser un credential helper ou de filtrer les messages d’erreur.

Le token d’authentification est passé via `GIT_CONFIG_VALUE_0`. C’est correct car il n’apparaît pas dans les arguments. Toutefois, en cas d’erreur, git peut afficher la ligne de commande dans les logs, ce qui pourrait exposer le token indirectement via l’URL de clonage. Envisager d’utiliser un credential helper ou de filtrer les messages d’erreur.
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);
}
}
+248
View File
@@ -0,0 +1,248 @@
//! Tool definitions exposed to the model and their execution inside a
qpismont marked this conversation as resolved
Review

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.

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.
Review

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.

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 openrouter_rs::types::Tool;
use serde_json::{Value, json};
use super::Sandbox;
use crate::gitea::WebhookType;
/// 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(
"read_file",
"Read the content of a text file inside the repository.",
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,
"read_file" => read_file(sandbox, args).await,
"grep" => grep(sandbox, args).await,
"find" => find(sandbox, args).await,
other => bail!("unknown tool `{other}`"),
}
}
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)
}
qpismont marked this conversation as resolved
Review

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` 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).
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 output = if start.is_none() && end.is_none() {
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");
sandbox.exec(&["sed", "-n", &range, "--", &path]).await?
};
into_stdout(output)
}
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()),
}
}
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"))?;
qpismont marked this conversation as resolved Outdated
Outdated
Review

La vérification est redondante : normalize() garantit déjà que le chemin ne peut pas sortir de la racine. Cependant, la double protection ne pose pas de problème de sécurité. On pourrait simplifier en supprimant cette condition si on est certain que normalize() ne retourne qu’un chemin bien formé.

La vérification est redondante : `normalize()` garantit déjà que le chemin ne peut pas sortir de la racine. Cependant, la double protection ne pose pas de problème de sécurité. On pourrait simplifier en supprimant cette condition si on est certain que `normalize()` ne retourne qu’un chemin bien formé.
if !normalized.starts_with(workspace) {
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)
}
Review

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.

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.
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 {
diff_url: "https://example.com/diff".to_string(),
Review

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.

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.
Review

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.

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.
Review

[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.

**[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.
number: 1,
title: "My PR".to_string(),
},
repository: Repository {
full_name: "owner/repo".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", "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"));
}
}