diff --git a/Cargo.lock b/Cargo.lock index acf1473..b03fa84 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1041,7 +1041,7 @@ checksum = "a89322df9ebe1c1578d689c92318e070967d1042b512afbe49518723f4e6d5cd" [[package]] name = "postil-cli" -version = "0.4.1" +version = "0.4.2" dependencies = [ "anyhow", "assert_cmd", diff --git a/Cargo.toml b/Cargo.toml index 259f66e..ac69b2c 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "postil-cli" -version = "0.4.1" +version = "0.4.2" edition = "2024" description = "Postil: a low-noise AI review gate. Silent on clean PRs, hard gate on real risk." license = "Apache-2.0" diff --git a/README.md b/README.md index 2b3d8f6..08cbbde 100644 --- a/README.md +++ b/README.md @@ -190,6 +190,11 @@ REVIEW_MODEL= \ postil review --staged --output json ``` +Hosted remote reviews use a 540-second total LLM budget so model cascades and +scoring finish inside the worker watchdog. Local reviews do not use a total +budget unless `POSTIL_LLM_TOTAL_TIMEOUT_SECS` is set. Each model request still +uses `POSTIL_LLM_REQUEST_TIMEOUT_SECS`, which defaults to 420 seconds. + Use the live benchmark harness before standardizing on a model: ```sh diff --git a/src/llm.rs b/src/llm.rs index 3cb7637..1cfd3f4 100644 --- a/src/llm.rs +++ b/src/llm.rs @@ -126,6 +126,12 @@ pub struct LlmClient { total_deadline: Option, } +#[derive(Debug, Clone, Copy)] +struct LlmTimeouts { + request: Duration, + total: Option, +} + /// Retries per model on transient provider errors before the cascade moves on. const TRANSIENT_RETRIES: u32 = 2; @@ -135,7 +141,7 @@ const TRANSIENT_RETRIES: u32 = 2; /// use their provider default. const REVIEW_MAX_TOKENS: u32 = 16384; const SCORER_MAX_TOKENS: u32 = 4096; -const DEFAULT_REQUEST_TIMEOUT_SECS: u64 = 480; +pub(crate) const DEFAULT_REQUEST_TIMEOUT_SECS: u64 = 480; const REQUEST_TIMEOUT_ENV: &str = "POSTIL_LLM_REQUEST_TIMEOUT_SECS"; const TOTAL_TIMEOUT_ENV: &str = "POSTIL_LLM_TOTAL_TIMEOUT_SECS"; @@ -158,17 +164,38 @@ fn retryable_status(status: u16) -> bool { } impl LlmClient { + /// Local and interactive clients have no built-in total deadline. They only + /// get one when POSTIL_LLM_TOTAL_TIMEOUT_SECS is explicitly set. pub fn from_env(cfg: &Config) -> Result { - let api_key = api_key::resolve_from_process_env().ok_or_else(|| { - let key_names = api_key::names_text(); - anyhow!( - "no API key: set {key_names}. Postil never proxies your inference; bring your own key." - ) - })?; - let request_timeout = - duration_from_env(REQUEST_TIMEOUT_ENV, Some(DEFAULT_REQUEST_TIMEOUT_SECS))? - .expect("default request timeout is always set"); - let total_timeout = duration_from_env(TOTAL_TIMEOUT_ENV, None)?; + let api_key = resolve_api_key()?; + let timeouts = LlmTimeouts::from_env(DEFAULT_REQUEST_TIMEOUT_SECS, None)?; + let total_deadline = timeouts.total.map(|duration| Instant::now() + duration); + Self::build(cfg, api_key, timeouts.request, total_deadline) + } + + pub(crate) fn from_env_for_remote_review( + cfg: &Config, + total_budget_started_at: Instant, + default_request_timeout: Duration, + default_total_timeout: Duration, + ) -> Result { + let api_key = resolve_api_key()?; + let timeouts = LlmTimeouts::from_env( + default_request_timeout.as_secs(), + Some(default_total_timeout.as_secs()), + )?; + let total_deadline = timeouts + .total + .map(|duration| total_budget_started_at + duration); + Self::build(cfg, api_key, timeouts.request, total_deadline) + } + + fn build( + cfg: &Config, + api_key: String, + request_timeout: Duration, + total_deadline: Option, + ) -> Result { Ok(LlmClient { http: reqwest::Client::builder() // Generation time scales with diff size; a thorough review of a @@ -177,7 +204,7 @@ impl LlmClient { .build()?, api_base: cfg.api_base.trim_end_matches('/').to_string(), api_key, - total_deadline: total_timeout.map(|duration| Instant::now() + duration), + total_deadline, }) } @@ -557,6 +584,24 @@ impl LlmClient { } } +impl LlmTimeouts { + fn from_env(request_default_secs: u64, total_default_secs: Option) -> Result { + let request = duration_from_env(REQUEST_TIMEOUT_ENV, Some(request_default_secs))? + .expect("default request timeout is always set"); + let total = duration_from_env(TOTAL_TIMEOUT_ENV, total_default_secs)?; + Ok(Self { request, total }) + } +} + +fn resolve_api_key() -> Result { + api_key::resolve_from_process_env().ok_or_else(|| { + let key_names = api_key::names_text(); + anyhow!( + "no API key: set {key_names}. Postil never proxies your inference; bring your own key." + ) + }) +} + fn duration_from_env(name: &str, default_secs: Option) -> Result> { let Some(raw) = std::env::var(name) .ok() @@ -814,7 +859,56 @@ fn consensus_merge(runs: Vec) -> ModelReview { #[cfg(test)] mod tests { use super::*; + use crate::config::Config; use crate::envelope::{Kind, Severity}; + use std::sync::{Mutex, OnceLock}; + + struct EnvRestore { + saved: Vec<(&'static str, Option)>, + } + + impl EnvRestore { + fn capture(names: &[&'static str]) -> Self { + Self { + saved: names + .iter() + .map(|name| (*name, std::env::var_os(name))) + .collect(), + } + } + + fn remove(name: &str) { + unsafe { + std::env::remove_var(name); + } + } + + fn set(name: &str, value: &str) { + unsafe { + std::env::set_var(name, value); + } + } + } + + impl Drop for EnvRestore { + fn drop(&mut self) { + for (name, value) in &self.saved { + match value { + Some(value) => unsafe { + std::env::set_var(name, value); + }, + None => unsafe { + std::env::remove_var(name); + }, + } + } + } + } + + fn env_lock() -> &'static Mutex<()> { + static LOCK: OnceLock> = OnceLock::new(); + LOCK.get_or_init(|| Mutex::new(())) + } #[test] fn provider_error_downcast_survives_additional_context() { @@ -837,6 +931,124 @@ mod tests { ); } + #[test] + fn remote_budget_start_sets_default_total_deadline_when_env_is_unset() { + let _lock = env_lock().lock().unwrap(); + let _env = EnvRestore::capture(&[REQUEST_TIMEOUT_ENV, TOTAL_TIMEOUT_ENV, "POSTIL_API_KEY"]); + EnvRestore::remove(REQUEST_TIMEOUT_ENV); + EnvRestore::remove(TOTAL_TIMEOUT_ENV); + EnvRestore::set("POSTIL_API_KEY", "test-key"); + + let client = LlmClient::from_env_for_remote_review( + &Config::default(), + Instant::now(), + Duration::from_secs(crate::review::HOSTED_LLM_REQUEST_TIMEOUT_SECS), + Duration::from_secs(crate::review::HOSTED_LLM_TOTAL_TIMEOUT_SECS), + ) + .unwrap(); + let remaining = client.remaining_total_budget().unwrap().unwrap(); + + assert!(remaining <= Duration::from_secs(crate::review::HOSTED_LLM_TOTAL_TIMEOUT_SECS)); + assert!(remaining > Duration::from_secs(crate::review::HOSTED_LLM_TOTAL_TIMEOUT_SECS - 5)); + } + + #[test] + fn local_from_env_has_no_total_deadline_without_env_override() { + let _lock = env_lock().lock().unwrap(); + let _env = EnvRestore::capture(&[REQUEST_TIMEOUT_ENV, TOTAL_TIMEOUT_ENV, "POSTIL_API_KEY"]); + EnvRestore::remove(REQUEST_TIMEOUT_ENV); + EnvRestore::remove(TOTAL_TIMEOUT_ENV); + EnvRestore::set("POSTIL_API_KEY", "test-key"); + + let client = LlmClient::from_env(&Config::default()).unwrap(); + + assert!(client.remaining_total_budget().unwrap().is_none()); + } + + #[test] + fn local_from_env_keeps_the_original_480s_request_timeout() { + let _lock = env_lock().lock().unwrap(); + let _env = EnvRestore::capture(&[REQUEST_TIMEOUT_ENV, TOTAL_TIMEOUT_ENV]); + EnvRestore::remove(REQUEST_TIMEOUT_ENV); + EnvRestore::remove(TOTAL_TIMEOUT_ENV); + + // The hosted path uses a shorter per-request timeout (420s) to fit the + // scorer and margin inside its total budget. Local/interactive runs have + // no total budget by default and must keep the original, more generous + // request timeout rather than inherit the hosted-tuned value. + let timeouts = LlmTimeouts::from_env(DEFAULT_REQUEST_TIMEOUT_SECS, None).unwrap(); + + assert_eq!(timeouts.request, Duration::from_secs(480)); + assert_ne!( + timeouts.request, + Duration::from_secs(crate::review::HOSTED_LLM_REQUEST_TIMEOUT_SECS) + ); + } + + #[test] + fn default_timeout_profile_sets_no_setup_scorer_window_and_worker_margin() { + let _lock = env_lock().lock().unwrap(); + let _env = EnvRestore::capture(&[REQUEST_TIMEOUT_ENV, TOTAL_TIMEOUT_ENV]); + EnvRestore::remove(REQUEST_TIMEOUT_ENV); + EnvRestore::remove(TOTAL_TIMEOUT_ENV); + + let timeouts = LlmTimeouts::from_env( + crate::review::HOSTED_LLM_REQUEST_TIMEOUT_SECS, + Some(crate::review::HOSTED_LLM_TOTAL_TIMEOUT_SECS), + ) + .unwrap(); + + assert_eq!( + timeouts.request, + Duration::from_secs(crate::review::HOSTED_LLM_REQUEST_TIMEOUT_SECS) + ); + assert_eq!( + timeouts.total, + Some(Duration::from_secs( + crate::review::HOSTED_LLM_TOTAL_TIMEOUT_SECS + )) + ); + assert_eq!( + timeouts.total.unwrap() - timeouts.request, + Duration::from_secs(crate::review::SCORER_TIMEOUT_SECS) + ); + assert_eq!( + Duration::from_secs(600) - timeouts.total.unwrap(), + Duration::from_secs(60) + ); + } + + #[test] + fn from_env_charges_elapsed_time_against_supplied_budget_start() { + let _lock = env_lock().lock().unwrap(); + let _env = EnvRestore::capture(&[REQUEST_TIMEOUT_ENV, TOTAL_TIMEOUT_ENV, "POSTIL_API_KEY"]); + EnvRestore::remove(REQUEST_TIMEOUT_ENV); + EnvRestore::remove(TOTAL_TIMEOUT_ENV); + EnvRestore::set("POSTIL_API_KEY", "test-key"); + + let elapsed = Duration::from_secs(10); + let started_at = Instant::now() - elapsed; + let client = LlmClient::from_env_for_remote_review( + &Config::default(), + started_at, + Duration::from_secs(crate::review::HOSTED_LLM_REQUEST_TIMEOUT_SECS), + Duration::from_secs(crate::review::HOSTED_LLM_TOTAL_TIMEOUT_SECS), + ) + .unwrap(); + let remaining = client.remaining_total_budget().unwrap().unwrap(); + + assert!( + remaining + <= Duration::from_secs(crate::review::HOSTED_LLM_TOTAL_TIMEOUT_SECS) - elapsed + ); + assert!( + remaining + > Duration::from_secs(crate::review::HOSTED_LLM_TOTAL_TIMEOUT_SECS) + - elapsed + - Duration::from_secs(5) + ); + } + #[test] fn extracts_json_from_fenced_output() { let text = "Here you go:\n```json\n{\"summary\": \"s\", \"findings\": []}\n```"; diff --git a/src/review.rs b/src/review.rs index a205209..e5b05f8 100644 --- a/src/review.rs +++ b/src/review.rs @@ -1,6 +1,7 @@ //! Review orchestration: one engine for local, CI, and hosted runs. use std::path::PathBuf; +use std::time::{Duration, Instant}; use anyhow::{Context, Result, anyhow}; @@ -26,7 +27,16 @@ const MAX_DIFF_BYTES: usize = 400_000; /// truncated at a line boundary and the review is flagged truncated so the /// uncertainty finding fires (a truncated review must not read as a full pass). const MAX_RAW_DIFF_BYTES: usize = MAX_DIFF_BYTES * 4; -const SCORER_TIMEOUT_SECS: u64 = 120; +const HOSTED_WORKER_WATCHDOG_SECS: u64 = 600; +pub(crate) const HOSTED_LLM_TOTAL_TIMEOUT_SECS: u64 = 540; +/// 540s total minus a 120s scorer reserve: the cascade's own request budget +/// stops with enough of the total left for the scorer call that follows it. +pub(crate) const HOSTED_LLM_REQUEST_TIMEOUT_SECS: u64 = 420; +const FORGE_READ_TIMEOUT_SECS: u64 = 60; +const CHECK_START_TIMEOUT_SECS: u64 = 30; +const CHECK_COMPLETION_TIMEOUT_SECS: u64 = 30; +const REVIEW_POST_TIMEOUT_SECS: u64 = 20; +pub(crate) const SCORER_TIMEOUT_SECS: u64 = 120; #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub enum ForgeKind { @@ -77,6 +87,14 @@ struct ReviewInput<'a> { baseline: Vec, scope: filter::ReconcileScope, force_model: bool, + llm_budget_started_at: Option, +} + +struct RemoteReviewInput<'a> { + meta: &'a PrMeta, + head_sha: &'a str, + prefetched_diff: Option>, + review_started: Instant, } pub async fn run(args: ReviewArgs) -> Result { @@ -164,10 +182,11 @@ async fn run_local(args: &ReviewArgs, cfg: &Config) -> Result { baseline, scope, force_model: false, + llm_budget_started_at: None, }, ) .await?; - finish(args, cfg, envelope, None::<&GitHub>).await + finish(args, cfg, envelope, None::<&GitHub>, None).await } async fn run_remote( @@ -183,7 +202,13 @@ async fn run_remote( // created checks also overlap their startup writes. Incremental diffs still // wait for metadata because they depend on the selected head SHA. let setup = async { - let meta = forge.fetch_pr_meta().await?; + let meta = run_with_hosted_budget( + Some(review_started), + FORGE_READ_TIMEOUT_SECS, + forge.fetch_pr_meta(), + "fetching PR metadata", + ) + .await?; let head_sha = args.sha.clone().unwrap_or_else(|| meta.head_sha.clone()); // Own the check-runs early so a crash can still be reported against them. @@ -192,7 +217,14 @@ async fn run_remote( } else if let (Some(a), Some(g)) = (&args.check_run_id, &args.gate_check_run_id) { Some((a.clone(), g.clone())) } else { - match forge.start_checks(&head_sha).await { + match run_with_hosted_budget( + Some(review_started), + CHECK_START_TIMEOUT_SECS, + forge.start_checks(&head_sha), + "creating check runs", + ) + .await + { Ok(ids) => Some(ids), Err(e) => { // CI tokens without checks:write still get review + exit code. @@ -205,7 +237,15 @@ async fn run_remote( }; let prefetch_diff = async { if args.since_sha.is_none() { - Some(forge.fetch_diff().await) + Some( + run_with_hosted_budget( + Some(review_started), + FORGE_READ_TIMEOUT_SECS, + forge.fetch_diff(), + "fetching diff", + ) + .await, + ) } else { None } @@ -213,7 +253,19 @@ async fn run_remote( let (setup, prefetched_diff) = tokio::join!(setup, prefetch_diff); let (meta, head_sha, checks) = setup?; - let result = remote_review(args, cfg, forge, repo, &meta, &head_sha, prefetched_diff).await; + let result = remote_review( + args, + cfg, + forge, + repo, + RemoteReviewInput { + meta: &meta, + head_sha: &head_sha, + prefetched_diff, + review_started, + }, + ) + .await; match result { Ok(envelope) => { if let Some((a, g)) = &checks { @@ -239,14 +291,18 @@ async fn run_remote( // discard the review this far along: log and keep going so the // envelope/SARIF output and exit code below still land. Mirrors // the Err(e) arm below, which best-effort's this same call. - if let Err(e) = forge - .complete_checks(a, g, advisory_state, gate_state, &envelope) - .await - { + let completed = run_with_hosted_budget( + Some(review_started), + CHECK_COMPLETION_TIMEOUT_SECS, + forge.complete_checks(a, g, advisory_state, gate_state, &envelope), + "completing check runs", + ) + .await; + if let Err(e) = completed { eprintln!("postil: could not update check runs ({e:#})"); } } - finish(args, cfg, envelope, Some(forge)).await + finish(args, cfg, envelope, Some(forge), Some(review_started)).await } Err(e) => { // Fail closed by default: an errored run must never read as a silent @@ -272,9 +328,13 @@ async fn run_remote( } else { CheckState::Success }; - let _ = forge - .complete_checks(a, g, CheckState::Neutral, gate_state, &envelope) - .await; + let _ = run_with_hosted_budget( + Some(review_started), + CHECK_COMPLETION_TIMEOUT_SECS, + forge.complete_checks(a, g, CheckState::Neutral, gate_state, &envelope), + "completing check runs", + ) + .await; } // Emit envelope/SARIF and derive the exit code from the gate. // `finish` itself already downgrades forge posting failures @@ -285,7 +345,7 @@ async fn run_remote( // exit code, so it is downgraded to the gate-derived code rather // than propagated as exit 2. let code = if envelope.gate.failing { 1 } else { 0 }; - match finish(args, cfg, envelope, Some(forge)).await { + match finish(args, cfg, envelope, Some(forge), Some(review_started)).await { Ok(c) => Ok(c), Err(post_err) => { eprintln!("postil: could not post the error review ({post_err:#})"); @@ -301,24 +361,39 @@ async fn remote_review( cfg: &Config, forge: &F, repo: &str, - meta: &PrMeta, - head_sha: &str, - prefetched_diff: Option>, + input: RemoteReviewInput<'_>, ) -> Result { + let RemoteReviewInput { + meta, + head_sha, + prefetched_diff, + review_started, + } = input; let baseline = load_baseline(args)?; let has_carryable_baseline = baseline.iter().any(|f| !f.path.starts_with(".postil/")); let incremental = args.since_sha.as_deref(); let (diff_text, scope, force_model) = match incremental { - Some(since) if since != head_sha => forge - .fetch_diff_since(since, head_sha) - .await - .context("incremental diff fetch") - .map(|diff| (diff, filter::ReconcileScope::Incremental, false))?, + Some(since) if since != head_sha => run_with_hosted_budget( + Some(review_started), + FORGE_READ_TIMEOUT_SECS, + forge.fetch_diff_since(since, head_sha), + "fetching incremental diff", + ) + .await + .context("incremental diff fetch") + .map(|diff| (diff, filter::ReconcileScope::Incremental, false))?, Some(_) => (String::new(), filter::ReconcileScope::Incremental, false), None => ( match prefetched_diff { Some(diff) => diff.context("diff fetch")?, - None => forge.fetch_diff().await.context("diff fetch")?, + None => run_with_hosted_budget( + Some(review_started), + FORGE_READ_TIMEOUT_SECS, + forge.fetch_diff(), + "fetching diff", + ) + .await + .context("diff fetch")?, }, filter::ReconcileScope::Full { trustworthy: false }, false, @@ -334,10 +409,14 @@ async fn remote_review( && diff_text.trim().is_empty() { ( - forge - .fetch_diff() - .await - .context("full diff fallback fetch")?, + run_with_hosted_budget( + Some(review_started), + FORGE_READ_TIMEOUT_SECS, + forge.fetch_diff(), + "fetching full fallback diff", + ) + .await + .context("full diff fallback fetch")?, filter::ReconcileScope::Full { trustworthy: false }, true, ) @@ -355,6 +434,7 @@ async fn remote_review( baseline, scope, force_model, + llm_budget_started_at: Some(review_started), }, ) .await @@ -496,6 +576,7 @@ async fn review_diff(cfg: &Config, args: &ReviewArgs, input: ReviewInput<'_>) -> baseline, scope, force_model, + llm_budget_started_at, } = input; let review_started = std::time::Instant::now(); // Cap the raw diff before parsing so an oversized fetched diff cannot force @@ -559,7 +640,15 @@ async fn review_diff(cfg: &Config, args: &ReviewArgs, input: ReviewInput<'_>) -> is shown above.]", ); } - let client = LlmClient::from_env(cfg)?; + let client = match llm_budget_started_at { + Some(started_at) => LlmClient::from_env_for_remote_review( + cfg, + started_at, + Duration::from_secs(HOSTED_LLM_REQUEST_TIMEOUT_SECS), + Duration::from_secs(HOSTED_LLM_TOTAL_TIMEOUT_SECS), + )?, + None => LlmClient::from_env(cfg)?, + }; match client.review(cfg, &system, &user).await { Ok(model_review) => { let outcome = filter::apply(cfg, &index, model_review.findings)?; @@ -748,6 +837,7 @@ async fn finish( cfg: &Config, envelope: Envelope, forge: Option<&F>, + hosted_budget_started_at: Option, ) -> Result { // Persist artifacts before any forge I/O: a posting hiccup must not // discard the completed review's SARIF or envelope output. @@ -791,7 +881,14 @@ async fn finish( // envelope/SARIF/stdout output above. Log it and keep going — the // exit code always derives from the gate below, never from whether // the forge comment made it out. - if let Err(e) = forge.post_review(&summary, &envelope.findings, &head).await { + let posted = run_with_hosted_budget( + hosted_budget_started_at, + REVIEW_POST_TIMEOUT_SECS, + forge.post_review(&summary, &envelope.findings, &head), + "posting review comment", + ) + .await; + if let Err(e) = posted { eprintln!("postil: could not post review comment ({e:#})"); } } @@ -799,6 +896,36 @@ async fn finish( Ok(if envelope.gate.failing { 1 } else { 0 }) } +async fn run_with_hosted_budget( + hosted_budget_started_at: Option, + cap_secs: u64, + future: impl std::future::Future>, + operation: &str, +) -> Result { + let Some(started_at) = hosted_budget_started_at else { + return future.await; + }; + let Some(remaining) = hosted_worker_remaining(started_at) else { + return Err(anyhow!( + "hosted watchdog budget exhausted before {operation}" + )); + }; + let timeout = remaining.min(Duration::from_secs(cap_secs)); + match tokio::time::timeout(timeout, future).await { + Ok(result) => result, + Err(_) => Err(anyhow!( + "{operation} timed out after {}s of hosted budget", + timeout.as_secs() + )), + } +} + +fn hosted_worker_remaining(started_at: Instant) -> Option { + (started_at + Duration::from_secs(HOSTED_WORKER_WATCHDOG_SECS)) + .checked_duration_since(Instant::now()) + .filter(|remaining| !remaining.is_zero()) +} + fn visible_finding_sets_equal(previous: &[Finding], current: &[Finding]) -> bool { // Synthetic findings represent this run's operational state and must stay // visible even if an earlier run failed in the same way. @@ -949,6 +1076,23 @@ mod tests { .expect("finding should have an assigned ID") } + #[test] + fn default_llm_timeouts_fit_inside_hosted_worker_watchdog() { + const PROCESS_OVERHEAD_SECS: u64 = 10; + + // The request plus scorer equation is the no-setup upper bound. Remote + // setup is anchored to the same total budget and reduces the remaining + // LLM/scorer time before the client is constructed. + assert_eq!( + HOSTED_LLM_TOTAL_TIMEOUT_SECS, + HOSTED_LLM_REQUEST_TIMEOUT_SECS + SCORER_TIMEOUT_SECS + ); + assert_eq!( + HOSTED_WORKER_WATCHDOG_SECS - HOSTED_LLM_TOTAL_TIMEOUT_SECS, + CHECK_COMPLETION_TIMEOUT_SECS + REVIEW_POST_TIMEOUT_SECS + PROCESS_OVERHEAD_SECS + ); + } + #[test] fn stable_ids_do_not_share_duplicate_buckets_across_path_line_boundaries() { let head_sha = "0123456789abcdef0123456789abcdef01234567"; diff --git a/tests/e2e.rs b/tests/e2e.rs index 0178c6c..2d19c6a 100644 --- a/tests/e2e.rs +++ b/tests/e2e.rs @@ -670,6 +670,69 @@ async fn full_remote_review_fetches_metadata_and_diff_concurrently() { ); } +#[tokio::test] +async fn remote_setup_time_counts_against_total_llm_budget() { + let server = MockServer::start().await; + let forge_delay = std::time::Duration::from_millis(1200); + Mock::given(method("POST")) + .and(path("/chat/completions")) + .respond_with(ResponseTemplate::new(200).set_body_json(llm_content(json!([])))) + .mount(&server) + .await; + Mock::given(method("GET")) + .and(path("/repos/acme/api/pulls/7")) + .and(header("Accept", "application/vnd.github.v3.diff")) + .respond_with( + ResponseTemplate::new(200) + .set_delay(forge_delay) + .set_body_string(DIFF), + ) + .mount(&server) + .await; + Mock::given(method("GET")) + .and(path("/repos/acme/api/pulls/7")) + .respond_with( + ResponseTemplate::new(200) + .set_delay(forge_delay) + .set_body_json(json!({ + "title": "t", "body": "b", + "head": {"sha": "headsha111"}, "base": {"sha": "basesha222"} + })), + ) + .mount(&server) + .await; + + let dir = tempfile::tempdir().unwrap(); + let out = postil() + .current_dir(dir.path()) + .env("POSTIL_API_BASE", server.uri()) + .env("POSTIL_LLM_REQUEST_TIMEOUT_SECS", "5") + .env("POSTIL_LLM_TOTAL_TIMEOUT_SECS", "1") + .env("GITHUB_API_URL", server.uri()) + .env("GITHUB_TOKEN", "gh-test-token") + .args([ + "review", + "--repo", + "acme/api", + "--pr", + "7", + "--no-post", + "--output-json", + ]) + .assert() + .code(1); + let env: Value = + serde_json::from_str(&String::from_utf8(out.get_output().stdout.clone()).unwrap()).unwrap(); + assert_eq!(env["findings"][0]["path"], ".postil/provider"); + + let reqs = server.received_requests().await.unwrap(); + let model_calls = reqs + .iter() + .filter(|r| r.url.path() == "/chat/completions") + .count(); + assert_eq!(model_calls, 0); +} + #[tokio::test] async fn sarif_is_written_for_local_review() { let server = MockServer::start().await; @@ -1241,6 +1304,7 @@ async fn slow_model_request_times_out_and_falls_back() { .current_dir(dir.path()) .env("POSTIL_API_BASE", server.uri()) .env("POSTIL_LLM_REQUEST_TIMEOUT_SECS", "1") + .env("POSTIL_LLM_TOTAL_TIMEOUT_SECS", "10") .env("REVIEW_MODEL", "primary-model") .env("REVIEW_MODEL_CASCADE", "backup-model") .args(["review", "--diff-file"]) @@ -1300,6 +1364,62 @@ async fn total_llm_timeout_caps_the_cascade() { assert_eq!(model, "primary-model"); } +#[tokio::test] +async fn shared_total_budget_can_expire_during_cascade_retry() { + let server = MockServer::start().await; + Mock::given(method("POST")) + .and(path("/chat/completions")) + .and(wiremock::matchers::body_string_contains("primary-model")) + .respond_with( + ResponseTemplate::new(400) + .set_delay(std::time::Duration::from_millis(400)) + .set_body_string("bad request"), + ) + .mount(&server) + .await; + Mock::given(method("POST")) + .and(path("/chat/completions")) + .and(wiremock::matchers::body_string_contains("backup-model")) + .respond_with( + ResponseTemplate::new(200) + .set_delay(std::time::Duration::from_millis(2200)) + .set_body_json(llm_content(json!([]))), + ) + .mount(&server) + .await; + + let dir = tempfile::tempdir().unwrap(); + let diff = write_diff(dir.path()); + let out = postil() + .current_dir(dir.path()) + .env("POSTIL_API_BASE", server.uri()) + .env("POSTIL_LLM_REQUEST_TIMEOUT_SECS", "5") + .env("POSTIL_LLM_TOTAL_TIMEOUT_SECS", "2") + .env("REVIEW_MODEL", "primary-model") + .env("REVIEW_MODEL_CASCADE", "backup-model") + .args(["review", "--diff-file"]) + .arg(&diff) + .arg("--output-json") + .assert() + .code(1); + let env: Value = + serde_json::from_str(&String::from_utf8(out.get_output().stdout.clone()).unwrap()).unwrap(); + assert_eq!(env["findings"][0]["path"], ".postil/provider"); + + let requests = server.received_requests().await.unwrap(); + assert_eq!(requests.len(), 2); + let models = requests + .iter() + .map(|request| { + request.body_json::().unwrap()["model"] + .as_str() + .unwrap() + .to_string() + }) + .collect::>(); + assert_eq!(models, vec!["primary-model", "backup-model"]); +} + #[tokio::test] async fn forge_post_failure_on_success_path_keeps_gate_derived_exit_code() { // A completed review (gate failing on a real finding) whose forge comment