diff --git a/docs/automation.md b/docs/automation.md index cbe9ca4..d496f36 100644 --- a/docs/automation.md +++ b/docs/automation.md @@ -34,6 +34,7 @@ The report shows which findings become visible or suppressed and which gate resu `postil respond` answers a mention on a pull request. GitHub and GitLab also support issue mentions. It does not open pull requests or push commits. Posting the reply requires `--publish`; without it, the reply is printed locally. +CI detection and environment variables do not authorize a forge write. ```sh POSTIL_COMMENT='@postil is this safe?' \ diff --git a/docs/forges.md b/docs/forges.md index 6615469..36a30a0 100644 --- a/docs/forges.md +++ b/docs/forges.md @@ -3,6 +3,7 @@ Postil reviews pull requests on GitHub, GitLab, Bitbucket, and Azure DevOps. Each integration accepts a base URL for self-managed installations. Forge writes require `--publish`. Without it, remote pull requests are fetched and reviewed locally without comments or checks. +CI detection and environment variables do not enable publication. `POSTIL_PUBLISH` and `POSTIL_NO_POST` are rejected because publication must be explicit in the command. ## GitHub diff --git a/src/cli.rs b/src/cli.rs index 91a1f52..b764e9c 100644 --- a/src/cli.rs +++ b/src/cli.rs @@ -6,6 +6,29 @@ use clap::{Parser, Subcommand, ValueEnum}; use crate::output::OutputFormat; +const UNSUPPORTED_PUBLICATION_ENVIRONMENT: [&str; 2] = ["POSTIL_PUBLISH", "POSTIL_NO_POST"]; + +/// Resolve the forge-publication action from command-line flags only. +/// +/// Publication is a mutation, so environment variables never authorize it. +/// Refuse publication-looking legacy variables instead of silently choosing an +/// interpretation that could write to a forge. +pub fn publication_enabled(publish: bool, no_post: bool) -> anyhow::Result { + let present = UNSUPPORTED_PUBLICATION_ENVIRONMENT + .iter() + .copied() + .filter(|name| std::env::var_os(name).is_some()) + .collect::>(); + if !present.is_empty() { + anyhow::bail!( + "{} cannot control forge publication; remove {} and pass --publish explicitly to write to the forge", + present.join(" and "), + if present.len() == 1 { "it" } else { "them" } + ); + } + Ok(publish && !no_post) +} + #[derive(Parser)] #[command( name = "postil", diff --git a/src/main.rs b/src/main.rs index c854d57..5478f0f 100644 --- a/src/main.rs +++ b/src/main.rs @@ -2,7 +2,7 @@ use clap::Parser; #[cfg(feature = "qualification-candidate")] use postil_cli::attribution; -use postil_cli::cli::{Cli, Command, ForgeArg, HookAction}; +use postil_cli::cli::{Cli, Command, ForgeArg, HookAction, publication_enabled}; use postil_cli::config::{Config, qualification_metadata, starter_config}; use postil_cli::review::{ForgeKind, ReviewArgs}; use postil_cli::{doctor, hook, plan, respond, review}; @@ -87,7 +87,7 @@ async fn dispatch(cli: Cli) -> anyhow::Result { config, model, bounded, - no_post: no_post || !publish, + no_post: !publication_enabled(publish, no_post)?, defer_gate_check, }) .await @@ -118,7 +118,7 @@ async fn dispatch(cli: Cli) -> anyhow::Result { comment, config, model, - no_post: no_post || !publish, + no_post: !publication_enabled(publish, no_post)?, }) .await } diff --git a/tests/e2e.rs b/tests/e2e.rs index b4c9996..ed597c8 100644 --- a/tests/e2e.rs +++ b/tests/e2e.rs @@ -723,6 +723,8 @@ fn postil() -> Command { .env_remove("POSTIL_DETAILS_URL") .env_remove("POSTIL_PREVENTION_HINT") .env_remove("POSTIL_PREVENTION_COMMANDS_JSON") + .env_remove("POSTIL_PUBLISH") + .env_remove("POSTIL_NO_POST") .env_remove("GITHUB_SERVER_URL") .env_remove("POSTIL_ENABLE_BITBUCKET_INCREMENTAL") .env("REVIEW_MODEL", "openai/gpt-5-mini") @@ -734,6 +736,86 @@ fn postil() -> Command { cmd } +#[tokio::test] +async fn local_review_stays_local_without_a_forge_token_in_ci() { + let server = MockServer::start().await; + let dir = tempfile::tempdir().unwrap(); + let diff = write_diff(dir.path()); + std::fs::write(dir.path().join(".postil.yaml"), "enabled: false\n").unwrap(); + + let output = postil() + .current_dir(dir.path()) + .env("CI", "true") + .env("GITHUB_ACTIONS", "true") + .env("GITHUB_REPOSITORY", "acme/api") + .env("GITHUB_API_URL", server.uri()) + .env_remove("GITHUB_TOKEN") + .args(["review", "--diff-file"]) + .arg(&diff) + .args(["--output", "json"]) + .assert() + .code(0); + + let envelope: Value = serde_json::from_slice(&output.get_output().stdout).unwrap(); + assert_eq!(envelope["modelUsed"], "none (disabled by config)"); + assert!( + server.received_requests().await.unwrap().is_empty(), + "local review must not contact a forge even when CI metadata names a pull-request repository" + ); +} + +#[tokio::test] +async fn remote_review_without_publish_never_mutates_github_even_in_hosted_ci() { + let server = MockServer::start().await; + mount_github_complete_diff(&server, 7).await; + mount_static_github_pr(&server).await; + let dir = tempfile::tempdir().unwrap(); + disable_review_for_hosted_publication(dir.path(), true); + + postil() + .current_dir(dir.path()) + .env("CI", "true") + .env("GITHUB_ACTIONS", "true") + .env("POSTIL_HOSTED_MODE", "1") + .env("POSTIL_EXPECTED_GITHUB_REPO_ID", "42") + .env("GITHUB_API_URL", server.uri()) + .env("GITHUB_TOKEN", "gh-test-token") + .args([ + "review", "--repo", "acme/api", "--pr", "7", "--output", "json", + ]) + .assert() + .code(0); + + let requests = server.received_requests().await.unwrap(); + assert!( + requests.iter().all(|request| { + !matches!( + request.method, + wiremock::http::Method::POST + | wiremock::http::Method::PATCH + | wiremock::http::Method::PUT + | wiremock::http::Method::DELETE + ) + }), + "remote review without --publish attempted a GitHub mutation: {requests:?}" + ); +} + +#[test] +fn publication_looking_environment_variables_are_rejected() { + for variable in ["POSTIL_PUBLISH", "POSTIL_NO_POST"] { + let output = postil() + .env(variable, "1") + .args(["review", "--diff-file", "missing.diff"]) + .assert() + .code(2); + let stderr = String::from_utf8_lossy(&output.get_output().stderr); + assert!(stderr.contains(&format!("{variable} cannot control forge publication"))); + assert!(stderr.contains("pass --publish explicitly")); + assert!(!stderr.contains("reading diff file")); + } +} + fn assert_model_usage_matches_aggregate(envelope: &Value) { let model_usage = envelope["modelUsage"].as_array().unwrap(); let ordinals = model_usage @@ -8773,6 +8855,61 @@ async fn respond_to_pr_mention_posts_grounded_reply() { assert!(!text.contains("Postil ยท")); } +#[tokio::test] +async fn respond_without_publish_prints_locally_and_never_comments() { + let server = MockServer::start().await; + mount_github_complete_diff(&server, 5).await; + Mock::given(method("POST")) + .and(path("/chat/completions")) + .respond_with(ResponseTemplate::new(200).set_body_json(respond_text( + "Line 41 passes untrusted input to the query. Parameterize it.", + ))) + .mount(&server) + .await; + Mock::given(method("GET")) + .and(path("/repos/acme/api/pulls/5")) + .and(header("Accept", "application/vnd.github.v3.diff")) + .respond_with(ResponseTemplate::new(200).set_body_string(DIFF)) + .mount(&server) + .await; + Mock::given(method("GET")) + .and(path("/repos/acme/api/pulls/5")) + .respond_with(ResponseTemplate::new(200).set_body_json(json!({ + "title": "Add login", "body": "PR body", + "state": "open", "merged": false, + "head": {"sha": "aaaaaaaa"}, "base": {"sha": "bbbbbbbb"}, "changed_files": 1 + }))) + .mount(&server) + .await; + + let dir = tempfile::tempdir().unwrap(); + let output = postil() + .current_dir(dir.path()) + .env("CI", "true") + .env("GITHUB_ACTIONS", "true") + .env("POSTIL_API_BASE", server.uri()) + .env("GITHUB_API_URL", server.uri()) + .env("GITHUB_TOKEN", "gh-test-token") + .args([ + "respond", + "--repo", + "acme/api", + "--pr", + "5", + "--comment", + "@postil is this safe?", + ]) + .assert() + .success(); + + assert!(String::from_utf8_lossy(&output.get_output().stdout).contains("Parameterize it.")); + let requests = server.received_requests().await.unwrap(); + assert!(!requests.iter().any(|request| { + request.method == wiremock::http::Method::POST + && request.url.path() == "/repos/acme/api/issues/5/comments" + })); +} + #[tokio::test] async fn respond_truncation_retries_without_publishing_partial_text() { let server = MockServer::start().await;