From 7854d36b57ce7bd4aa266ca3cc2f60acaf5c98c0 Mon Sep 17 00:00:00 2001 From: Postil Maintainer Date: Sat, 11 Jul 2026 23:32:37 +0000 Subject: [PATCH 1/5] Activate default LLM timeout budget --- Cargo.lock | 2 +- Cargo.toml | 2 +- README.md | 5 ++ src/llm.rs | 207 +++++++++++++++++++++++++++++++++++++++++++++++--- src/review.rs | 200 ++++++++++++++++++++++++++++++++++++++++-------- tests/e2e.rs | 120 +++++++++++++++++++++++++++++ 6 files changed, 492 insertions(+), 44 deletions(-) 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..f19c2b3 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 = 420; const REQUEST_TIMEOUT_ENV: &str = "POSTIL_LLM_REQUEST_TIMEOUT_SECS"; const TOTAL_TIMEOUT_ENV: &str = "POSTIL_LLM_TOTAL_TIMEOUT_SECS"; @@ -158,17 +164,34 @@ 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(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_total_timeout: Duration, + ) -> Result { + let api_key = resolve_api_key()?; + let timeouts = LlmTimeouts::from_env(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 +200,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 +580,24 @@ impl LlmClient { } } +impl LlmTimeouts { + fn from_env(total_default_secs: Option) -> Result { + let request = duration_from_env(REQUEST_TIMEOUT_ENV, Some(DEFAULT_REQUEST_TIMEOUT_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 +855,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 +927,99 @@ 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_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 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(Some(crate::review::HOSTED_LLM_TOTAL_TIMEOUT_SECS)).unwrap(); + + assert_eq!( + timeouts.request, + Duration::from_secs(DEFAULT_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_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..7e14bcf 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,13 @@ 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; +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 +84,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 +179,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 +199,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 +214,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 +234,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 +250,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 +288,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 +325,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 +342,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 +358,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 +406,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 +431,7 @@ async fn remote_review( baseline, scope, force_model, + llm_budget_started_at: Some(review_started), }, ) .await @@ -496,6 +573,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 +637,14 @@ 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_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 +833,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 +877,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 +892,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 +1072,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, + crate::llm::DEFAULT_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 From a7873e0fc830a90028825969296923359da9cbc6 Mon Sep 17 00:00:00 2001 From: Postil Maintainer Date: Sat, 11 Jul 2026 23:38:35 +0000 Subject: [PATCH 2/5] Re-trigger review (prior gate result was stale, quoting text that predates the current commit's actual content) From 89464b92d35488d3a289f5e98b8b9596391564ff Mon Sep 17 00:00:00 2001 From: Postil Maintainer Date: Sat, 11 Jul 2026 23:42:14 +0000 Subject: [PATCH 3/5] Re-trigger review with simplified PR description From 5ecbba7b43ff159125299654a6a4779ffe4fa36d Mon Sep 17 00:00:00 2001 From: Postil Maintainer Date: Sat, 11 Jul 2026 23:48:44 +0000 Subject: [PATCH 4/5] Restore the local default request timeout to 480s The per-request timeout default had dropped from 480s to 420s universally, including for local/interactive runs that have no total budget to fit within -- 420s only makes sense for the hosted path (540s total - 120s scorer). Threads a request-timeout default through from_env_for_remote_review alongside the existing total-timeout default, matching the split already used for the total budget: local keeps 480s, hosted uses 420s explicitly via HOSTED_LLM_REQUEST_TIMEOUT_SECS. --- src/llm.rs | 45 +++++++++++++++++++++++++++++++++++++-------- src/review.rs | 7 ++++++- 2 files changed, 43 insertions(+), 9 deletions(-) diff --git a/src/llm.rs b/src/llm.rs index f19c2b3..1cfd3f4 100644 --- a/src/llm.rs +++ b/src/llm.rs @@ -141,7 +141,7 @@ const TRANSIENT_RETRIES: u32 = 2; /// use their provider default. const REVIEW_MAX_TOKENS: u32 = 16384; const SCORER_MAX_TOKENS: u32 = 4096; -pub(crate) const DEFAULT_REQUEST_TIMEOUT_SECS: u64 = 420; +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"; @@ -168,7 +168,7 @@ impl LlmClient { /// get one when POSTIL_LLM_TOTAL_TIMEOUT_SECS is explicitly set. pub fn from_env(cfg: &Config) -> Result { let api_key = resolve_api_key()?; - let timeouts = LlmTimeouts::from_env(None)?; + 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) } @@ -176,10 +176,14 @@ impl LlmClient { 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(Some(default_total_timeout.as_secs()))?; + 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); @@ -581,8 +585,8 @@ impl LlmClient { } impl LlmTimeouts { - fn from_env(total_default_secs: Option) -> Result { - let request = duration_from_env(REQUEST_TIMEOUT_ENV, Some(DEFAULT_REQUEST_TIMEOUT_SECS))? + 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 }) @@ -938,6 +942,7 @@ mod tests { 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(); @@ -960,6 +965,26 @@ mod tests { 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(); @@ -967,12 +992,15 @@ mod tests { EnvRestore::remove(REQUEST_TIMEOUT_ENV); EnvRestore::remove(TOTAL_TIMEOUT_ENV); - let timeouts = - LlmTimeouts::from_env(Some(crate::review::HOSTED_LLM_TOTAL_TIMEOUT_SECS)).unwrap(); + 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(DEFAULT_REQUEST_TIMEOUT_SECS) + Duration::from_secs(crate::review::HOSTED_LLM_REQUEST_TIMEOUT_SECS) ); assert_eq!( timeouts.total, @@ -1003,6 +1031,7 @@ mod tests { 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(); diff --git a/src/review.rs b/src/review.rs index 7e14bcf..1fd61d4 100644 --- a/src/review.rs +++ b/src/review.rs @@ -29,6 +29,10 @@ const MAX_DIFF_BYTES: usize = 400_000; const MAX_RAW_DIFF_BYTES: usize = MAX_DIFF_BYTES * 4; const HOSTED_WORKER_WATCHDOG_SECS: u64 = 600; pub(crate) const HOSTED_LLM_TOTAL_TIMEOUT_SECS: u64 = 540; +/// 540s total minus a 120s scorer reserve, leaving the cascade room to try +/// every roster model once (4 models * 120s = 480s) before the total budget +/// itself would have expired anyway. +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; @@ -641,6 +645,7 @@ async fn review_diff(cfg: &Config, args: &ReviewArgs, input: ReviewInput<'_>) -> 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)?, @@ -1081,7 +1086,7 @@ mod tests { // LLM/scorer time before the client is constructed. assert_eq!( HOSTED_LLM_TOTAL_TIMEOUT_SECS, - crate::llm::DEFAULT_REQUEST_TIMEOUT_SECS + SCORER_TIMEOUT_SECS + HOSTED_LLM_REQUEST_TIMEOUT_SECS + SCORER_TIMEOUT_SECS ); assert_eq!( HOSTED_WORKER_WATCHDOG_SECS - HOSTED_LLM_TOTAL_TIMEOUT_SECS, From e0698b079761e09ef88b2f37bb6e10c563d7a664 Mon Sep 17 00:00:00 2001 From: Postil Maintainer Date: Sat, 11 Jul 2026 23:51:58 +0000 Subject: [PATCH 5/5] Fix inaccurate doc comment on HOSTED_LLM_REQUEST_TIMEOUT_SECS The comment claimed the 420s value came from '4 models * 120s', which doesn't match the actual constant or its real derivation (540s total minus the 120s scorer reserve = 420s). --- src/review.rs | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/review.rs b/src/review.rs index 1fd61d4..e5b05f8 100644 --- a/src/review.rs +++ b/src/review.rs @@ -29,9 +29,8 @@ const MAX_DIFF_BYTES: usize = 400_000; const MAX_RAW_DIFF_BYTES: usize = MAX_DIFF_BYTES * 4; const HOSTED_WORKER_WATCHDOG_SECS: u64 = 600; pub(crate) const HOSTED_LLM_TOTAL_TIMEOUT_SECS: u64 = 540; -/// 540s total minus a 120s scorer reserve, leaving the cascade room to try -/// every roster model once (4 models * 120s = 480s) before the total budget -/// itself would have expired anyway. +/// 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;