diff --git a/.github/actions/cora-review-simple/action.yml b/.github/actions/cora-review-simple/action.yml index ba7808ac..cbaf77ef 100644 --- a/.github/actions/cora-review-simple/action.yml +++ b/.github/actions/cora-review-simple/action.yml @@ -39,7 +39,22 @@ runs: shell: bash run: | if [ "${{ inputs.cora-version }}" = "latest" ]; then - VERSION=$(curl -sL https://api.github.com/repos/ajianaz/cora-cli/releases/latest | python3 -c "import sys,json; print(json.load(sys.stdin)['tag_name'])") + # Try up to 3 times with delay + for i in 1 2 3; do + VERSION=$(curl -sfL --connect-timeout 10 \ + https://api.github.com/repos/ajianaz/cora-cli/releases/latest \ + | python3 -c "import sys,json; d=json.load(sys.stdin); print(d.get('tag_name', ''))" 2>/dev/null) + if [ -n "$VERSION" ]; then + break + fi + echo "Attempt $i failed, retrying..." + sleep 5 + done + # Fallback to a known good version if all retries fail + if [ -z "$VERSION" ]; then + echo "::warning::Failed to resolve latest cora-cli version from GitHub API, using fallback" + VERSION="v0.1.6" + fi else VERSION="${{ inputs.cora-version }}" fi diff --git a/.github/actions/cora-review/action.yml b/.github/actions/cora-review/action.yml index cc3ba451..508891aa 100644 --- a/.github/actions/cora-review/action.yml +++ b/.github/actions/cora-review/action.yml @@ -54,7 +54,22 @@ runs: shell: bash run: | if [ "${{ inputs.cora-version }}" = "latest" ]; then - VERSION=$(curl -sL https://api.github.com/repos/ajianaz/cora-cli/releases/latest | python3 -c "import sys,json; print(json.load(sys.stdin)['tag_name'])") + # Try up to 3 times with delay + for i in 1 2 3; do + VERSION=$(curl -sfL --connect-timeout 10 \ + https://api.github.com/repos/ajianaz/cora-cli/releases/latest \ + | python3 -c "import sys,json; d=json.load(sys.stdin); print(d.get('tag_name', ''))" 2>/dev/null) + if [ -n "$VERSION" ]; then + break + fi + echo "Attempt $i failed, retrying..." + sleep 5 + done + # Fallback to a known good version if all retries fail + if [ -z "$VERSION" ]; then + echo "::warning::Failed to resolve latest cora-cli version from GitHub API, using fallback" + VERSION="v0.1.6" + fi else VERSION="${{ inputs.cora-version }}" fi diff --git a/CHANGELOG.md b/CHANGELOG.md index fe2b8790..cf6a8a15 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,24 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +## [0.1.7] - 2026-06-01 + +### Added + +- **Diff-hash caching** — review results cached by SHA-256 of diff + model + temperature in `~/.cache/cora/reviews/`. Cache TTL configurable via `llm.cache_ttl` (#100) +- **`--no-cache` flag** — bypass cache for fresh reviews (#100) +- **Configurable LLM parameters** — `llm.temperature` (default: 0), `llm.max_tokens` (default: 4096), `llm.timeout` (default: 120s), `llm.cache_ttl` (default: 1440 min) in `.cora.yaml` (#98 #101) +- **Git ref validation** — rejects refs containing shell metacharacters or path traversal sequences (#73) + +### Fixed + +- **Temperature default now 0** — eliminates non-deterministic LLM output. Same diff produces identical issues on every run (#98, #97) +- **HTTP timeout actually works** — per-request timeout via reqwest RequestBuilder (not client-level). Configurable timeout respected (#99) +- **Connection pooling** — shared reqwest::Client via LazyLock, reused across all requests (#99) +- **Cache key includes model + temperature** — config changes invalidate cache automatically (#100) +- **Silent config corruption** — malformed `.cora.yaml` now shows clear error with file path and hint (#78) +- **Composite action KeyError on API failure** — version resolution retries 3x with 5s delay, falls back to v0.1.6 with warning. Fixed in both `cora-review` and `cora-review-simple` actions (#102) + ## [0.1.6] - 2026-06-01 ### Added diff --git a/Cargo.lock b/Cargo.lock index f84a1475..f9ad90eb 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -106,6 +106,15 @@ version = "2.11.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "c4512299f36f043ab09a583e57bceb5a5aab7a73db1805848e8fef3c9e8c78b3" +[[package]] +name = "block-buffer" +version = "0.10.4" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "3078c7629b62d3f0439517fa394996acacc5cbc91c5a20d8c658e77abd503a71" +dependencies = [ + "generic-array", +] + [[package]] name = "bstr" version = "1.12.1" @@ -231,7 +240,7 @@ dependencies = [ [[package]] name = "cora-cli" -version = "0.1.5" +version = "0.1.6" dependencies = [ "anyhow", "assert_cmd", @@ -249,6 +258,7 @@ dependencies = [ "serde", "serde_json", "serde_yaml_ng", + "sha2", "tempfile", "tokio", "tokio-test", @@ -258,12 +268,41 @@ dependencies = [ "walkdir", ] +[[package]] +name = "cpufeatures" +version = "0.2.17" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "59ed5838eebb26a2bb2e58f6d5b5316989ae9d08bab10e0e6d103e656d1b0280" +dependencies = [ + "libc", +] + +[[package]] +name = "crypto-common" +version = "0.1.7" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "78c8292055d1c1df0cce5d180393dc8cce0abec0a7102adb6c7b1eef6016d60a" +dependencies = [ + "generic-array", + "typenum", +] + [[package]] name = "difflib" version = "0.4.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "6184e33543162437515c2e2b48714794e37845ec9851711914eec9d308f6ebe8" +[[package]] +name = "digest" +version = "0.10.7" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "9ed9a281f7bc9b7576e61468ba615a66a5c8cfdff42420a70aa82701a3b1e292" +dependencies = [ + "block-buffer", + "crypto-common", +] + [[package]] name = "dirs" version = "6.0.0" @@ -414,6 +453,16 @@ dependencies = [ "slab", ] +[[package]] +name = "generic-array" +version = "0.14.7" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "85649ca51fd72272d7821adaf274ad91c288277713d9c18820d8499a7ff69e9a" +dependencies = [ + "typenum", + "version_check", +] + [[package]] name = "getrandom" version = "0.2.17" @@ -1357,6 +1406,17 @@ dependencies = [ "unsafe-libyaml", ] +[[package]] +name = "sha2" +version = "0.10.9" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "a7507d819769d01a365ab707794a4084392c824f54a7a6a7862f8c3d0892b283" +dependencies = [ + "cfg-if", + "cpufeatures", + "digest", +] + [[package]] name = "sharded-slab" version = "0.1.7" @@ -1740,6 +1800,12 @@ version = "0.2.5" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "e421abadd41a4225275504ea4d6566923418b7f05506fbc9c0fe86ba7396114b" +[[package]] +name = "typenum" +version = "1.20.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "b6f5e870be6c3b371b77fe0ee0bafb859fa4964b4404c27de1d380043c4dda20" + [[package]] name = "unicode-ident" version = "1.0.24" @@ -1812,6 +1878,12 @@ version = "0.2.15" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "accd4ea62f7bb7a82fe23066fb0957d48ef677f6eeb8215f372f52e48bb32426" +[[package]] +name = "version_check" +version = "0.9.5" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "0b928f33d975fc6ad9f86c8f283853ad26bdd5b10b7f1542aa2fa15e2289105a" + [[package]] name = "wait-timeout" version = "0.2.1" diff --git a/Cargo.toml b/Cargo.toml index c155e296..11f82b31 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "cora-cli" -version = "0.1.6" +version = "0.1.7" edition = "2024" description = "CLI-first AI code review — BYOK, diff/scan/branch, pre-commit hooks" license = "MIT" @@ -51,6 +51,7 @@ tracing-subscriber = { version = "0.3", features = ["env-filter"] } # Misc regex = "1" futures-util = "0.3" +sha2 = "0.10" [dev-dependencies] assert_cmd = "2" diff --git a/README.md b/README.md index 28133559..8c605b16 100644 --- a/README.md +++ b/README.md @@ -41,10 +41,10 @@ Download the latest release from [GitHub Releases](https://github.com/ajianaz/co ```bash # Determine your platform tag from the releases page, e.g.: -# cora-aarch64-unknown-linux-gnu-v0.1.6.tar.gz -# cora-x86_64-unknown-linux-gnu-v0.1.6.tar.gz -# cora-aarch64-apple-darwin-v0.1.6.tar.gz -# cora-x86_64-pc-windows-msvc-v0.1.6.zip +# cora-aarch64-unknown-linux-gnu-v0.1.7.tar.gz +# cora-x86_64-unknown-linux-gnu-v0.1.7.tar.gz +# cora-aarch64-apple-darwin-v0.1.7.tar.gz +# cora-x86_64-pc-windows-msvc-v0.1.7.zip # Example: Linux aarch64 VERSION=$(curl -s https://api.github.com/repos/ajianaz/cora-cli/releases/latest | grep tag_name | cut -d'"' -f4) diff --git a/src/commands/review.rs b/src/commands/review.rs index 0484b856..bd0d6b58 100644 --- a/src/commands/review.rs +++ b/src/commands/review.rs @@ -34,6 +34,8 @@ pub struct ReviewOptions { pub quiet: bool, /// Minimum severity level for filtering issues. pub severity: Option, + /// Disable review caching. + pub no_cache: bool, } /// Result of a review command execution. @@ -88,9 +90,14 @@ pub async fn execute_review( ); // 3. Call the LLM engine - let response = - crate::engine::review::review_diff_with_stream(config, llm_config, &diff, opts.stream) - .await?; + let response = crate::engine::review::review_diff_with_cache( + config, + llm_config, + &diff, + opts.stream, + !opts.no_cache, + ) + .await?; // 4. Filter by severity if specified let min_severity = if let Some(ref sev) = opts.severity { diff --git a/src/config/loader.rs b/src/config/loader.rs index f282e2d9..bd143b59 100644 --- a/src/config/loader.rs +++ b/src/config/loader.rs @@ -37,7 +37,14 @@ pub fn find_cora_file(start: &Path) -> Result> { debug!(path = %candidate.display(), "found .cora.yaml"); let content = std::fs::read_to_string(&candidate) .with_context(|| format!("failed to read {}", candidate.display()))?; - let cora = CoraFile::from_str(&content)?; + let cora = CoraFile::from_str(&content).map_err(|e| { + let msg = e.to_string(); + anyhow::anyhow!( + "{}\n → file: {}\n → hint: check for syntax errors (indentation, colons, trailing spaces)", + msg, + candidate.display() + ) + })?; return Ok(Some((candidate, cora))); } @@ -237,7 +244,15 @@ pub fn load_config( let path = Path::new(path); let content = std::fs::read_to_string(path) .with_context(|| format!("failed to read config at {}", path.display()))?; - let cora = CoraFile::from_str(&content)?; + let cora = CoraFile::from_str(&content).map_err(|e| { + let msg = e.to_string(); + // Enhance error message with path info and hint about malformed YAML + anyhow::anyhow!( + "{}\n → file: {}\n → hint: check for syntax errors (indentation, colons, trailing spaces)", + msg, + path.display() + ) + })?; cora.merge_into(&mut config); debug!(path = %path.display(), "loaded explicit config"); } else if let Some((path, cora)) = find_cora_file(&std::env::current_dir()?)? { @@ -340,6 +355,9 @@ pub fn build_llm_config(config: &Config, cli_api_key: Option<&str>) -> Result, /// Optional custom system prompt file path for scan. pub scan_system_prompt_file: Option, + /// LLM temperature for deterministic output. + pub temperature: f32, + /// Max tokens for LLM responses. + pub max_tokens: u32, + /// HTTP timeout in seconds for LLM requests. + pub timeout: u64, + /// Cache TTL in minutes for review caching. + pub cache_ttl: u64, } /// Provider configuration. @@ -98,6 +106,10 @@ impl Default for Config { review_system_prompt_file: None, scan_system_prompt_override: None, scan_system_prompt_file: None, + temperature: 0.0, + max_tokens: 4096, + timeout: 120, + cache_ttl: 1440, // 24h in minutes } } } @@ -128,6 +140,8 @@ pub struct CoraFile { pub review: Option, #[serde(skip_serializing_if = "Option::is_none")] pub scan: Option, + #[serde(skip_serializing_if = "Option::is_none")] + pub llm: Option, } #[derive(Debug, Clone, Serialize, Deserialize, Default)] @@ -186,6 +200,19 @@ pub struct ScanSection { pub system_prompt_file: Option, } +/// LLM-specific configuration section (temperature, max_tokens, timeout, cache_ttl). +#[derive(Debug, Clone, Serialize, Deserialize, Default)] +pub struct LlmSection { + #[serde(skip_serializing_if = "Option::is_none")] + pub temperature: Option, + #[serde(skip_serializing_if = "Option::is_none")] + pub max_tokens: Option, + #[serde(skip_serializing_if = "Option::is_none")] + pub timeout: Option, + #[serde(skip_serializing_if = "Option::is_none")] + pub cache_ttl: Option, +} + impl CoraFile { pub fn from_str(content: &str) -> Result { serde_yaml_ng::from_str(content).context("failed to parse .cora.yaml") @@ -256,6 +283,20 @@ impl CoraFile { config.scan_system_prompt_file = Some(v.clone()); } } + if let Some(llm) = &self.llm { + if let Some(v) = llm.temperature { + config.temperature = v; + } + if let Some(v) = llm.max_tokens { + config.max_tokens = v; + } + if let Some(v) = llm.timeout { + config.timeout = v; + } + if let Some(v) = llm.cache_ttl { + config.cache_ttl = v; + } + } } } @@ -684,4 +725,175 @@ scan: assert_eq!(scan.system_prompt.as_deref(), Some("Performance only.\n")); assert_eq!(scan.system_prompt_file.as_deref(), Some("scan.md")); } + + // ─── LLM section parsing and merge ─── + + #[test] + fn config_default_temperature_is_zero() { + let cfg = Config::default(); + assert_eq!(cfg.temperature, 0.0); + } + + #[test] + fn config_default_max_tokens() { + let cfg = Config::default(); + assert_eq!(cfg.max_tokens, 4096); + } + + #[test] + fn config_default_timeout() { + let cfg = Config::default(); + assert_eq!(cfg.timeout, 120); + } + + #[test] + fn config_default_cache_ttl() { + let cfg = Config::default(); + assert_eq!(cfg.cache_ttl, 1440); + } + + #[test] + fn parse_llm_section() { + let yaml = r#" +llm: + temperature: 0.5 + max_tokens: 8192 + timeout: 60 + cache_ttl: 720 +"#; + let cora = CoraFile::from_str(yaml).unwrap(); + let llm = cora.llm.unwrap(); + assert_eq!(llm.temperature, Some(0.5)); + assert_eq!(llm.max_tokens, Some(8192)); + assert_eq!(llm.timeout, Some(60)); + assert_eq!(llm.cache_ttl, Some(720)); + } + + #[test] + fn parse_llm_section_partial() { + let yaml = r#" +llm: + temperature: 0.3 +"#; + let cora = CoraFile::from_str(yaml).unwrap(); + let llm = cora.llm.unwrap(); + assert_eq!(llm.temperature, Some(0.3)); + assert_eq!(llm.max_tokens, None); + assert_eq!(llm.timeout, None); + assert_eq!(llm.cache_ttl, None); + } + + #[test] + fn merge_llm_temperature() { + let mut cfg = Config::default(); + let cora = CoraFile { + llm: Some(LlmSection { + temperature: Some(0.7), + max_tokens: None, + timeout: None, + cache_ttl: None, + }), + ..Default::default() + }; + cora.merge_into(&mut cfg); + assert_eq!(cfg.temperature, 0.7); + // Other LLM fields should remain at defaults + assert_eq!(cfg.max_tokens, 4096); + assert_eq!(cfg.timeout, 120); + assert_eq!(cfg.cache_ttl, 1440); + } + + #[test] + fn merge_llm_max_tokens() { + let mut cfg = Config::default(); + let cora = CoraFile { + llm: Some(LlmSection { + temperature: None, + max_tokens: Some(2048), + timeout: None, + cache_ttl: None, + }), + ..Default::default() + }; + cora.merge_into(&mut cfg); + assert_eq!(cfg.max_tokens, 2048); + } + + #[test] + fn merge_llm_timeout() { + let mut cfg = Config::default(); + let cora = CoraFile { + llm: Some(LlmSection { + temperature: None, + max_tokens: None, + timeout: Some(300), + cache_ttl: None, + }), + ..Default::default() + }; + cora.merge_into(&mut cfg); + assert_eq!(cfg.timeout, 300); + } + + #[test] + fn merge_llm_all_fields() { + let mut cfg = Config::default(); + let cora = CoraFile { + llm: Some(LlmSection { + temperature: Some(1.0), + max_tokens: Some(16384), + timeout: Some(240), + cache_ttl: Some(2880), + }), + ..Default::default() + }; + cora.merge_into(&mut cfg); + assert_eq!(cfg.temperature, 1.0); + assert_eq!(cfg.max_tokens, 16384); + assert_eq!(cfg.timeout, 240); + assert_eq!(cfg.cache_ttl, 2880); + } + + // ─── Config error on malformed YAML ─── + + #[test] + fn cora_file_malformed_yaml_returns_error() { + let yaml = r#" +provider: + provider: openai + model: gpt-4 + this is not valid yaml: [ +"#; + let result = CoraFile::from_str(yaml); + assert!(result.is_err(), "malformed YAML should return an error"); + let err = result.unwrap_err().to_string(); + assert!( + err.contains("failed to parse .cora.yaml"), + "error message should mention parse failure: {err}" + ); + } + + #[test] + fn cora_file_empty_yaml_is_ok() { + let cora = CoraFile::from_str("").unwrap(); + assert!(cora.llm.is_none()); + assert!(cora.provider.is_none()); + } + + #[test] + fn cora_file_yaml_roundtrip_with_llm() { + let cora = CoraFile { + llm: Some(LlmSection { + temperature: Some(0.5), + max_tokens: Some(8192), + timeout: Some(60), + cache_ttl: None, + }), + ..Default::default() + }; + let yaml = serde_yaml_ng::to_string(&cora).unwrap(); + let back: CoraFile = serde_yaml_ng::from_str(&yaml).unwrap(); + assert_eq!(back.llm.as_ref().unwrap().temperature, Some(0.5)); + assert_eq!(back.llm.as_ref().unwrap().max_tokens, Some(8192)); + } } diff --git a/src/engine/cache.rs b/src/engine/cache.rs new file mode 100644 index 00000000..a1a13d8e --- /dev/null +++ b/src/engine/cache.rs @@ -0,0 +1,272 @@ +use anyhow::{Context, Result}; +use sha2::{Digest, Sha256}; +use std::path::PathBuf; +use std::time::{SystemTime, UNIX_EPOCH}; +use tracing::debug; + +use crate::engine::types::ReviewResponse; + +/// Get the cache directory: ~/.cache/cora/reviews/ +fn cache_dir() -> Result { + let home = dirs::home_dir().context("cannot determine home directory")?; + Ok(home.join(".cache").join("cora").join("reviews")) +} + +/// Compute SHA-256 hex digest of the diff content + config parameters. +/// Includes model and temperature so config changes invalidate the cache. +fn cache_key(diff: &str, model: &str, temperature: f32) -> String { + let mut hasher = Sha256::new(); + hasher.update(diff.as_bytes()); + hasher.update(model.as_bytes()); + hasher.update(temperature.to_le_bytes()); + let result = hasher.finalize(); + result.iter().map(|b| format!("{:02x}", b)).collect() +} + +/// Get cached review response if available and not expired. +/// +/// Returns `None` if no cache exists or if the cache entry has expired +/// (based on `ttl` in minutes). +pub fn get_cached_review( + diff: &str, + model: &str, + temperature: f32, + ttl: u64, +) -> Option { + let hash = cache_key(diff, model, temperature); + let dir = cache_dir().ok()?; + let path = dir.join(format!("{}.json", hash)); + + if !path.is_file() { + debug!("cache miss: file not found"); + return None; + } + + let content = std::fs::read_to_string(&path).ok()?; + let cached: CachedReview = match serde_json::from_str(&content) { + Ok(c) => c, + Err(e) => { + debug!("cache corrupt, ignoring: {}", e); + return None; + } + }; + + let now = SystemTime::now() + .duration_since(UNIX_EPOCH) + .unwrap_or_default() + .as_secs(); + + let age_secs = now.saturating_sub(cached.timestamp); + let ttl_secs = ttl * 60; + + if age_secs > ttl_secs { + debug!(age_secs = age_secs, ttl_secs = ttl_secs, "cache expired"); + // Clean up expired cache file + let _ = std::fs::remove_file(&path); + return None; + } + + debug!(hash = %hash, age_secs = age_secs, "cache hit"); + Some(cached.response) +} + +/// Save a review response to the cache. +pub fn save_cached_review( + diff: &str, + model: &str, + temperature: f32, + response: &ReviewResponse, +) -> Result<()> { + let dir = cache_dir()?; + std::fs::create_dir_all(&dir) + .with_context(|| format!("failed to create cache dir {}", dir.display()))?; + + let hash = cache_key(diff, model, temperature); + let path = dir.join(format!("{}.json", hash)); + + let now = SystemTime::now() + .duration_since(UNIX_EPOCH) + .unwrap_or_default() + .as_secs(); + + let cached = CachedReview { + response: response.clone(), + timestamp: now, + }; + + let json = + serde_json::to_string_pretty(&cached).context("failed to serialize cached review")?; + + std::fs::write(&path, json) + .with_context(|| format!("failed to write cache to {}", path.display()))?; + + debug!(hash = %hash, "saved review to cache"); + Ok(()) +} + +/// Internal cache entry format. +#[derive(serde::Serialize, serde::Deserialize)] +struct CachedReview { + response: ReviewResponse, + timestamp: u64, +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::engine::Severity; + use crate::engine::types::ReviewIssue; + + fn make_response() -> ReviewResponse { + ReviewResponse { + issues: vec![ReviewIssue { + file: "src/main.rs".to_string(), + line: Some(10), + severity: Severity::Major, + issue_type: Some("bugs".to_string()), + title: "Null pointer".to_string(), + body: "Could be null here".to_string(), + suggested_fix: Some("Add a check".to_string()), + }], + summary: "Found 1 issue.".to_string(), + tokens_used: None, + should_block: false, + } + } + + #[test] + fn cache_key_is_deterministic() { + let hash1 = cache_key("hello world", "gpt-4", 0.0); + let hash2 = cache_key("hello world", "gpt-4", 0.0); + assert_eq!(hash1, hash2); + assert_eq!(hash1.len(), 64); // SHA-256 hex = 64 chars + } + + #[test] + fn cache_key_differs_for_different_inputs() { + let hash1 = cache_key("hello world", "gpt-4", 0.0); + let hash2 = cache_key("hello earth", "gpt-4", 0.0); + assert_ne!(hash1, hash2); + } + + #[test] + fn cache_key_includes_model_and_temperature() { + let h1 = cache_key("diff", "gpt-4", 0.0); + let h2 = cache_key("diff", "gpt-3.5", 0.0); + let h3 = cache_key("diff", "gpt-4", 0.7); + assert_ne!(h1, h2, "different models should differ"); + assert_ne!(h1, h3, "different temperatures should differ"); + } + + #[test] + fn cache_key_len_is_64() { + let diff = "diff --git a/file.txt b/file.txt\n+ hello"; + let hash = cache_key(diff, "model", 0.0); + assert_eq!(hash.len(), 64); + } + + #[test] + fn cache_miss_on_different_diff() { + let diff1 = "diff --git a/a.txt b/a.txt\n+ hello"; + let diff2 = "diff --git a/b.txt b/b.txt\n+ world"; + let hash1 = cache_key(diff1, "model", 0.0); + let hash2 = cache_key(diff2, "model", 0.0); + assert_ne!(hash1, hash2); + } + + #[test] + fn cached_review_serialization_roundtrip() { + let response = make_response(); + let now = SystemTime::now() + .duration_since(UNIX_EPOCH) + .unwrap_or_default() + .as_secs(); + + let cached = CachedReview { + response, + timestamp: now, + }; + + let json = serde_json::to_string(&cached).unwrap(); + let back: CachedReview = serde_json::from_str(&json).unwrap(); + assert_eq!(back.timestamp, now); + assert_eq!(back.response.issues.len(), 1); + assert_eq!(back.response.issues[0].file, "src/main.rs"); + assert_eq!(back.response.summary, "Found 1 issue."); + } + + #[test] + fn ttl_expiry() { + let response = make_response(); + let now = SystemTime::now() + .duration_since(UNIX_EPOCH) + .unwrap_or_default() + .as_secs(); + + let cached = CachedReview { + response, + timestamp: now.saturating_sub(2 * 60 * 60), // 2 hours ago + }; + + let json = serde_json::to_string(&cached).unwrap(); + let back: CachedReview = serde_json::from_str(&json).unwrap(); + + // With ttl = 60 (1 hour), this should be expired + let age_secs = now - back.timestamp; + let ttl_secs = 60 * 60; + assert!(age_secs > ttl_secs); + } + + #[test] + fn ttl_not_expired() { + let response = make_response(); + let now = SystemTime::now() + .duration_since(UNIX_EPOCH) + .unwrap_or_default() + .as_secs(); + + let cached = CachedReview { + response, + timestamp: now.saturating_sub(10 * 60), // 10 minutes ago + }; + + let json = serde_json::to_string(&cached).unwrap(); + let back: CachedReview = serde_json::from_str(&json).unwrap(); + + // With ttl = 1440 (24h), this should NOT be expired + let age_secs = now - back.timestamp; + let ttl_secs = 1440 * 60; + assert!(age_secs <= ttl_secs); + } + + #[test] + fn save_and_get_cached_review() { + let tmp = tempfile::tempdir().unwrap(); + let dir = tmp.path(); + + // Manually set up a cache entry in the temp dir + let diff = "test diff content"; + let hash = cache_key(diff, "model", 0.0); + let path = dir.join(format!("{}.json", hash)); + + let response = make_response(); + let now = SystemTime::now() + .duration_since(UNIX_EPOCH) + .unwrap_or_default() + .as_secs(); + + let cached = CachedReview { + response: response.clone(), + timestamp: now, + }; + + let json = serde_json::to_string(&cached).unwrap(); + std::fs::write(&path, json).unwrap(); + + // Verify we can read it back + let content = std::fs::read_to_string(&path).unwrap(); + let back: CachedReview = serde_json::from_str(&content).unwrap(); + assert_eq!(back.response.issues.len(), 1); + assert_eq!(back.response.issues[0].title, "Null pointer"); + } +} diff --git a/src/engine/llm.rs b/src/engine/llm.rs index 82d9bba6..9726732a 100644 --- a/src/engine/llm.rs +++ b/src/engine/llm.rs @@ -2,10 +2,29 @@ use anyhow::{Context, Result}; use indicatif::{ProgressBar, ProgressStyle}; use serde::{Deserialize, Serialize}; use serde_json::Value; +use std::sync::LazyLock; use tracing::debug; use crate::engine::types::{LLMConfig, ReviewIssue, ReviewResponse, TokenUsage}; +/// Shared reqwest::Client with connection pooling. Reused across all LLM requests. +/// Created lazily on first use to avoid blocking initialization. +/// Per-request timeout is set via .timeout() on the RequestBuilder. +static SHARED_CLIENT: LazyLock = LazyLock::new(|| { + reqwest::Client::builder() + .pool_max_idle_per_host(4) + .build() + .unwrap_or_else(|e| { + tracing::error!("failed to build shared HTTP client: {}", e); + reqwest::Client::new() + }) +}); + +/// Return the shared reqwest::Client for LLM API requests. +pub fn shared_client() -> reqwest::Client { + SHARED_CLIENT.clone() +} + /// OpenAI-compatible chat message. #[derive(Debug, Clone, Serialize, Deserialize)] struct ChatMessage { @@ -124,7 +143,7 @@ async fn chat_completion( spinner: Option<&ProgressBar>, response_format: &str, ) -> Result { - let client = reqwest::Client::new(); + let client = shared_client(); let url = format!("{}/chat/completions", config.base_url.trim_end_matches('/')); @@ -147,8 +166,8 @@ async fn chat_completion( content: user_message.into(), }, ], - temperature: 0.2, - max_tokens: 4096, + temperature: config.temperature, + max_tokens: config.max_tokens, response_format: if response_format == "json_object" { Some(serde_json::json!({"type": "json_object"})) } else { @@ -163,9 +182,10 @@ async fn chat_completion( .header("Authorization", format!("Bearer {}", config.api_key)) .header("Content-Type", "application/json") .json(&request) + .timeout(std::time::Duration::from_secs(config.timeout)) .send() .await - .context("LLM API request failed")?; + .context("LLM API request failed (or timed out)")?; let status = response.status(); let body = response @@ -318,7 +338,7 @@ async fn chat_completion_stream( use futures_util::StreamExt; use std::io::Write; - let client = reqwest::Client::new(); + let client = shared_client(); let url = format!("{}/chat/completions", config.base_url.trim_end_matches('/')); let mut request_body = serde_json::json!({ @@ -327,8 +347,8 @@ async fn chat_completion_stream( { "role": "system", "content": system_prompt }, { "role": "user", "content": user_message } ], - "temperature": 0.2, - "max_tokens": 4096, + "temperature": config.temperature, + "max_tokens": config.max_tokens, "stream": true }); @@ -343,9 +363,10 @@ async fn chat_completion_stream( .header("Authorization", format!("Bearer {}", config.api_key)) .header("Content-Type", "application/json") .json(&request_body) + .timeout(std::time::Duration::from_secs(config.timeout)) .send() .await - .context("LLM API streaming request failed")?; + .context("LLM API streaming request failed (or timed out)")?; let status = response.status(); if !status.is_success() { diff --git a/src/engine/mod.rs b/src/engine/mod.rs index d93e79d9..99ec7fbb 100644 --- a/src/engine/mod.rs +++ b/src/engine/mod.rs @@ -1,3 +1,4 @@ +pub mod cache; pub mod llm; pub mod review; pub mod scanner; diff --git a/src/engine/review.rs b/src/engine/review.rs index b1598255..470cf30e 100644 --- a/src/engine/review.rs +++ b/src/engine/review.rs @@ -62,7 +62,7 @@ pub async fn review_diff( llm_config: &LLMConfig, diff: &str, ) -> Result { - review_diff_inner(config, llm_config, diff, false).await + review_diff_inner(config, llm_config, diff, false, true).await } /// Run a code review on the given diff string with optional streaming. @@ -75,7 +75,22 @@ pub async fn review_diff_with_stream( diff: &str, stream: bool, ) -> Result { - review_diff_inner(config, llm_config, diff, stream).await + review_diff_inner(config, llm_config, diff, stream, true).await +} + +/// Run a code review on the given diff string with optional streaming and cache control. +/// +/// When `stream` is true, LLM tokens are printed to stdout in real-time. +/// When `use_cache` is false, the cache is bypassed. +#[instrument(skip_all)] +pub async fn review_diff_with_cache( + config: &Config, + llm_config: &LLMConfig, + diff: &str, + stream: bool, + use_cache: bool, +) -> Result { + review_diff_inner(config, llm_config, diff, stream, use_cache).await } async fn review_diff_inner( @@ -83,6 +98,7 @@ async fn review_diff_inner( llm_config: &LLMConfig, diff: &str, stream: bool, + use_cache: bool, ) -> Result { debug!( diff_len = diff.len(), @@ -99,6 +115,19 @@ async fn review_diff_inner( }); } + // Check cache before calling LLM + if use_cache { + if let Some(cached) = crate::engine::cache::get_cached_review( + diff, + &llm_config.model, + llm_config.temperature, + config.cache_ttl, + ) { + debug!("returning cached review response"); + return Ok(cached); + } + } + // Extract valid file paths for post-parse filtering let valid_files = llm::extract_file_paths_from_diff(diff); @@ -164,6 +193,18 @@ async fn review_diff_inner( "review complete" ); + // Save fully-processed response to cache (after filtering) + if use_cache { + if let Err(e) = crate::engine::cache::save_cached_review( + diff, + &llm_config.model, + llm_config.temperature, + &response, + ) { + debug!("failed to save review to cache: {}", e); + } + } + Ok(response) } diff --git a/src/engine/types.rs b/src/engine/types.rs index 844b3beb..ddf47cfe 100644 --- a/src/engine/types.rs +++ b/src/engine/types.rs @@ -297,6 +297,9 @@ mod tests { assert_eq!(cfg.base_url, "https://api.openai.com/v1"); assert_eq!(cfg.model, "gpt-4o-mini"); assert_eq!(cfg.provider, "openai"); + assert_eq!(cfg.temperature, 0.0); + assert_eq!(cfg.max_tokens, 4096); + assert_eq!(cfg.timeout, 120); } // ─── TokenUsage::default ─── @@ -411,6 +414,9 @@ pub struct LLMConfig { pub base_url: String, pub model: String, pub provider: String, + pub temperature: f32, + pub max_tokens: u32, + pub timeout: u64, } impl Default for LLMConfig { @@ -420,6 +426,9 @@ impl Default for LLMConfig { base_url: "https://api.openai.com/v1".to_string(), model: "gpt-4o-mini".to_string(), provider: "openai".to_string(), + temperature: 0.0, + max_tokens: 4096, + timeout: 120, } } } diff --git a/src/git/diff.rs b/src/git/diff.rs index 00eb8c29..b0d086bb 100644 --- a/src/git/diff.rs +++ b/src/git/diff.rs @@ -4,6 +4,46 @@ use std::process::Command; use anyhow::{Context, Result}; use git2::Repository; +/// Characters that should not appear in git refs passed to our diff commands. +/// This prevents shell injection and path traversal via crafted ref names. +const DANGEROUS_REF_CHARS: &[char] = &[ + ';', '&', '|', '`', '$', '(', ')', '<', '>', '{', '}', '\\', '\n', '\r', '\t', +]; + +/// Validate a git ref string for safety before passing it to git commands. +/// +/// Rejects refs containing shell metacharacters or path traversal sequences (..). +fn validate_ref(ref_str: &str) -> Result<()> { + if ref_str.contains("..") { + // ".." is only allowed in the middle of range expressions like "HEAD..HEAD" or "main...HEAD" + // But we need to be careful about path traversal. Allow it only for git range syntax. + // Actually, git diff ranges use ".." and "..." which are valid git syntax. + // The risk is when ".." appears as part of a path (e.g., "../../etc/passwd"). + // Since git refs shouldn't contain path components with "..", we reject refs + // that start with ".." or contain "/.." or ".." at the end. + if ref_str.starts_with("..") || ref_str.contains("/..") || ref_str.ends_with("..") { + anyhow::bail!( + "invalid ref '{}': contains path traversal sequence '..'", + ref_str + ); + } + } + + if ref_str.contains(DANGEROUS_REF_CHARS) { + let found: String = ref_str + .chars() + .filter(|c| DANGEROUS_REF_CHARS.contains(c)) + .collect(); + anyhow::bail!( + "invalid ref '{}': contains unsafe characters: '{}'", + ref_str, + found + ); + } + + Ok(()) +} + /// Open the git repository at or above the current working directory. pub fn open_repo() -> Result { Repository::discover(std::env::current_dir()?).context("not inside a git repository") @@ -36,6 +76,7 @@ pub fn get_unstaged_diff() -> Result { /// Get the diff between the current branch and `base_branch`. pub fn get_branch_diff(base_branch: &str) -> Result { + validate_ref(base_branch)?; let arg = format!("{}...HEAD", base_branch); git_cmd(&["diff", &arg]) } @@ -51,6 +92,7 @@ pub fn get_unpushed_diff() -> Result { /// - Otherwise (e.g. `HEAD`, `abc123`), uses `git show --format=""` to /// get just the diff without commit metadata. pub fn get_commit_diff(ref_str: &str) -> Result { + validate_ref(ref_str)?; if ref_str.contains("..") { git_cmd(&["diff", ref_str]) } else { @@ -122,3 +164,46 @@ pub fn is_inside_git_repo(dir: Option<&Path>) -> bool { let search_dir = dir.unwrap_or(&cwd); Repository::discover(search_dir).is_ok() } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn validate_ref_simple_name() { + assert!(validate_ref("main").is_ok()); + assert!(validate_ref("HEAD").is_ok()); + assert!(validate_ref("feature/my-branch").is_ok()); + assert!(validate_ref("abc123").is_ok()); + } + + #[test] + fn validate_ref_range_syntax() { + assert!(validate_ref("HEAD~3..HEAD").is_ok()); + assert!(validate_ref("main...develop").is_ok()); + assert!(validate_ref("abc123..def456").is_ok()); + } + + #[test] + fn validate_ref_rejects_path_traversal() { + assert!(validate_ref("../../etc/passwd").is_err()); + assert!(validate_ref("..HEAD").is_err()); + assert!(validate_ref("HEAD..").is_err()); + assert!(validate_ref("branch/../other").is_err()); + } + + #[test] + fn validate_ref_rejects_shell_metacharacters() { + assert!(validate_ref("main;rm -rf /").is_err()); + assert!(validate_ref("main`whoami`").is_err()); + assert!(validate_ref("$(whoami)").is_err()); + assert!(validate_ref("main && echo pwned").is_err()); + assert!(validate_ref("main | cat").is_err()); + assert!(validate_ref("main\nextra").is_err()); + } + + #[test] + fn validate_ref_rejects_angle_brackets() { + assert!(validate_ref("main > /etc/passwd").is_err()); + } +} diff --git a/src/main.rs b/src/main.rs index 5d7f1179..09d1f2aa 100644 --- a/src/main.rs +++ b/src/main.rs @@ -115,6 +115,10 @@ enum Command { #[clap(long, value_parser = ["info", "minor", "major", "critical"])] severity: Option, + /// Disable review caching + #[clap(long)] + no_cache: bool, + /// Upload SARIF output to GitHub Code Scanning after review /// (implies --format sarif) #[clap(long)] @@ -286,6 +290,7 @@ async fn main() -> Result<()> { stream, quiet, severity, + no_cache, upload, repo, ref_name, @@ -303,6 +308,7 @@ async fn main() -> Result<()> { stream, quiet, severity, + no_cache, upload, repo, ref_name, @@ -410,6 +416,7 @@ struct ReviewOpts { repo: Option, ref_name: Option, token: Option, + no_cache: bool, } /// Struct to hold scan options from CLI. @@ -452,6 +459,7 @@ async fn cmd_review(globals: &GlobalOptions, opts: ReviewOpts) -> Result { stream: opts.stream, quiet: opts.quiet, severity: opts.severity.clone(), + no_cache: opts.no_cache, }; // When streaming and not quiet, show a simpler message