From 74b63dd031a0b9246f739f6a59a5550b687fb9e3 Mon Sep 17 00:00:00 2001 From: atlas Date: Wed, 14 Oct 2026 10:43:41 +0200 Subject: [PATCH] review classifier: commit shas and index lines stay out of the prompt Every classification's shared context began with the PR's head sha, headed each commit with its sha, and carried each file's `index ..` line in the diff. A push or rebase rewrites all of those, so the provider's prompt cache missed from the first line of the user message even when the commit messages and hunks were unchanged (#5245: "cache hits show as 0.1x"). The header now names only repo#pr, each commit is headed by its position, and the diff drops `index` lines before lockfiles collapse. The `diff --git` paths and every hunk stay. The TryOut's head_sha is untouched: the controller still posts the review against it, and the journal line still logs it. On the ACP runtime the cost sink also adds each classification's tokens to `hyperhive.review_classifier.token.usage`, typed input / output / cacheRead / cacheCreation like claude's own `claude_code.token.usage`, so the hit rate is a number in the metrics store on both runtimes. --- docs/swarm/review-classifier.md | 15 +++ swarm-review-classifier/src/acp.rs | 5 + swarm-review-classifier/src/cost.rs | 137 ++++++++++++++++++++------ swarm-review-classifier/src/prompt.rs | 68 +++++++++++-- 4 files changed, 190 insertions(+), 35 deletions(-) diff --git a/docs/swarm/review-classifier.md b/docs/swarm/review-classifier.md index 120f86ed..a8ebec4d 100644 --- a/docs/swarm/review-classifier.md +++ b/docs/swarm/review-classifier.md @@ -268,6 +268,14 @@ Only the last part differs per classification: that classification's description and the task to classify the PR against it. The identical prefix lets the runtime's prompt cache reuse it across questions. +No object hash is in that prefix: the header names the repo and the PR number, +not the head commit, each commit's heading gives its position instead of its +sha, and the diff loses each file's `index ..` line, keeping its +`diff --git` paths and every hunk. A rebase rewrites all of those hashes, so +with them in, a rebased PR with the same messages and hunks would miss the +cache from the first line. The head commit still goes to the controller, +which posts the review against it. + ### One question, one answer The model answers each classification in JSON. The answer holds: @@ -520,6 +528,13 @@ deadline, or whose agent reports no token counts, adds nothing. opencode reports no cost of its own for a provider without prices in its config, which is why the prices are a setting. +Each such classification also adds the tokens it used to the counter +`hyperhive.review_classifier.token.usage`, with the same labels bar `currency` +and a `type` of `input`, `output`, `cacheRead` or `cacheCreation`. Those are +the types of `claude_code.token.usage`, which `claude` exports on the claude +runtimes, so the cache hit rate reads the same way on every runtime: `cacheRead` +over the sum of `input`, `cacheRead` and `cacheCreation`. + The classifier logs each run's result to its journal, which ships to VictoriaLogs like every swarm service container's ([`services.md`](services.md#logs-victorialogs)). diff --git a/swarm-review-classifier/src/acp.rs b/swarm-review-classifier/src/acp.rs index 5bf98a25..7c5d7475 100644 --- a/swarm-review-classifier/src/acp.rs +++ b/swarm-review-classifier/src/acp.rs @@ -175,6 +175,7 @@ impl Acp { pr: run.pr, model: provider.model.clone(), amount: crate::cost::amount(&provider.prices, usage), + usage: *usage, }); } } @@ -387,6 +388,10 @@ done ("o/r", 7, "qwen") ); assert!((cost.amount - 0.006).abs() < 1e-12, "{}", cost.amount); + assert_eq!( + (cost.usage.input_tokens, cost.usage.output_tokens), + (1000, 200) + ); } #[tokio::test] diff --git a/swarm-review-classifier/src/cost.rs b/swarm-review-classifier/src/cost.rs index 78c0222c..51b95ebc 100644 --- a/swarm-review-classifier/src/cost.rs +++ b/swarm-review-classifier/src/cost.rs @@ -1,6 +1,11 @@ //! What a classification on the ACP runtime cost, exported as the counter //! `hyperhive.review_classifier.cost.usage` with the run's `repo`, `pr`, -//! `model` and `currency`. On the claude runtime `claude` exports its own. +//! `model` and `currency`, and the tokens it used, as the counter +//! `hyperhive.review_classifier.token.usage` with the same labels bar +//! `currency` and a `type` of `input`, `output`, `cacheRead` or +//! `cacheCreation`. Those are `claude_code.token.usage`'s types, which +//! `claude` exports for itself on the claude runtime, so the cache hit rate is +//! `cacheRead` over the three input types on either runtime. //! //! The amount is computed here from the prices the run carries and the //! tokens the agent reports, so it does not depend on how, or whether, the @@ -26,7 +31,7 @@ pub const CURRENCY: &str = "USD"; /// How often the counter is exported. const INTERVAL: Duration = Duration::from_mins(1); -/// One classification's cost, and what it is labelled with. +/// One classification's cost and tokens, and what they are labelled with. #[derive(Debug, Clone, PartialEq)] pub struct Cost { pub repo: String, @@ -34,6 +39,7 @@ pub struct Cost { pub model: String, /// In [`CURRENCY`]. pub amount: f64, + pub usage: TokenUsage, } /// Where a classification's cost goes: the metrics store in production, a @@ -57,43 +63,67 @@ pub fn amount(prices: &Prices, usage: &TokenUsage) -> f64 { * per_token(prices.cache_write.unwrap_or(prices.input)) } -/// The counter in the metrics store. +/// The counters in the metrics store. pub struct Otel; impl CostSink for Otel { fn record(&self, cost: &Cost) { - static COUNTER: OnceLock)>> = OnceLock::new(); - let Some((_, counter)) = COUNTER.get_or_init(build) else { + static COUNTERS: OnceLock> = OnceLock::new(); + let Some((_, counters)) = COUNTERS.get_or_init(build) else { return; }; - add(counter, cost); + add(counters, cost); } } -/// The counter on `provider`. -fn cost_counter(provider: &SdkMeterProvider) -> Counter { - provider - .meter("hyperhive.review_classifier") - .f64_counter("hyperhive.review_classifier.cost.usage") - .build() +/// The cost counter, and the token counter beside it. +struct Counters { + cost: Counter, + tokens: Counter, } -/// Adds `cost` to `counter` with the run's labels. -fn add(counter: &Counter, cost: &Cost) { - counter.add( - cost.amount, - &[ - KeyValue::new("repo", cost.repo.clone()), - KeyValue::new("pr", i64::try_from(cost.pr).unwrap_or(i64::MAX)), - KeyValue::new("model", cost.model.clone()), - KeyValue::new("currency", CURRENCY), - ], - ); +/// The counters on `provider`. +fn counters(provider: &SdkMeterProvider) -> Counters { + let meter = provider.meter("hyperhive.review_classifier"); + Counters { + cost: meter + .f64_counter("hyperhive.review_classifier.cost.usage") + .build(), + tokens: meter + .u64_counter("hyperhive.review_classifier.token.usage") + .build(), + } } -/// The provider, kept alive with the counter since it exports only while it +/// Adds `cost` to `counters` with the run's labels. +fn add(counters: &Counters, cost: &Cost) { + let labels = [ + KeyValue::new("repo", cost.repo.clone()), + KeyValue::new("pr", i64::try_from(cost.pr).unwrap_or(i64::MAX)), + KeyValue::new("model", cost.model.clone()), + ]; + let with = |extra: KeyValue| { + let mut all = labels.to_vec(); + all.push(extra); + all + }; + counters + .cost + .add(cost.amount, &with(KeyValue::new("currency", CURRENCY))); + let usage = &cost.usage; + for (kind, n) in [ + ("input", usage.input_tokens), + ("output", usage.output_tokens), + ("cacheRead", usage.cache_read_input_tokens), + ("cacheCreation", usage.cache_creation_input_tokens), + ] { + counters.tokens.add(n, &with(KeyValue::new("type", kind))); + } +} + +/// The provider, kept alive with the counters since it exports only while it /// lives, or `None` when no collector is configured or the exporter failed. -fn build() -> Option<(SdkMeterProvider, Counter)> { +fn build() -> Option<(SdkMeterProvider, Counters)> { if !std::env::var("OTEL_EXPORTER_OTLP_ENDPOINT").is_ok_and(|s| !s.trim().is_empty()) { return None; } @@ -114,8 +144,8 @@ fn build() -> Option<(SdkMeterProvider, Counter)> { .with_interval(INTERVAL) .build(); let provider = SdkMeterProvider::builder().with_reader(reader).build(); - let counter = cost_counter(&provider); - Some((provider, counter)) + let counters = counters(&provider); + Some((provider, counters)) } #[cfg(test)] @@ -168,7 +198,7 @@ mod tests { let exporter = InMemoryMetricExporter::default(); let reader = PeriodicReader::builder(exporter.clone()).build(); let provider = SdkMeterProvider::builder().with_reader(reader).build(); - let counter = cost_counter(&provider); + let counters = counters(&provider); let prices = Prices { input: 3.0, output: 15.0, @@ -180,8 +210,9 @@ mod tests { pr: 5214, model: "some-model".to_owned(), amount: amount(&prices, &usage(1_000_000, 100_000, 0, 0)), + usage: usage(1_000_000, 100_000, 0, 0), }; - add(&counter, &cost); + add(&counters, &cost); provider.force_flush().expect("flush"); let metrics = exporter.get_finished_metrics().expect("metrics"); @@ -214,4 +245,52 @@ mod tests { assert_eq!(label("currency"), Some(Value::from("USD"))); assert_eq!(labels.len(), 4); } + + #[test] + fn a_record_adds_each_token_class_under_its_type() { + let exporter = InMemoryMetricExporter::default(); + let reader = PeriodicReader::builder(exporter.clone()).build(); + let provider = SdkMeterProvider::builder().with_reader(reader).build(); + let cost = Cost { + repo: "hyperhive/hyperhive".to_owned(), + pr: 5245, + model: "some-model".to_owned(), + amount: 0.0, + usage: usage(100, 20, 9_000, 400), + }; + add(&counters(&provider), &cost); + provider.force_flush().expect("flush"); + + let metrics = exporter.get_finished_metrics().expect("metrics"); + let metric = metrics + .iter() + .flat_map(ResourceMetrics::scope_metrics) + .flat_map(ScopeMetrics::metrics) + .find(|m| m.name() == "hyperhive.review_classifier.token.usage") + .expect("the token counter was exported"); + let AggregatedMetrics::U64(MetricData::Sum(sum)) = metric.data() else { + panic!("not a u64 sum: {:?}", metric.data()); + }; + let mut by_type: Vec<(String, u64, usize)> = sum + .data_points() + .map(|point| { + let kind = point + .attributes() + .find(|kv| kv.key.as_str() == "type") + .map(|kv| kv.value.to_string()) + .unwrap_or_default(); + (kind, point.value(), point.attributes().count()) + }) + .collect(); + by_type.sort(); + assert_eq!( + by_type, + [ + ("cacheCreation".to_owned(), 400, 4), + ("cacheRead".to_owned(), 9_000, 4), + ("input".to_owned(), 100, 4), + ("output".to_owned(), 20, 4), + ] + ); + } } diff --git a/swarm-review-classifier/src/prompt.rs b/swarm-review-classifier/src/prompt.rs index cde6fb19..26e5be12 100644 --- a/swarm-review-classifier/src/prompt.rs +++ b/swarm-review-classifier/src/prompt.rs @@ -30,22 +30,26 @@ Answer with exactly one JSON object and nothing else, no code fence: /// The shared context: the PR's commits with their full messages, then its /// diff with lockfiles collapsed. Built once per run. +/// +/// No object hash goes in: not the head's, a commit's or a blob's. A rebase +/// rewrites all of them, and the provider's cache only serves a prefix that +/// is byte for byte the same as an earlier one. #[must_use] pub fn shared_context(run: &TryOut) -> String { let mut out = format!( - "# Pull request {}#{} at {}\n\n## Commits, oldest first\n\n", - run.repo, run.pr, run.head_sha + "# Pull request {}#{}\n\n## Commits, oldest first\n\n", + run.repo, run.pr ); - for commit in &run.commits { + for (n, commit) in run.commits.iter().enumerate() { let _ = write!( out, - "### {}\n\n{}\n\n", - commit.sha, + "### Commit {}\n\n{}\n\n", + n + 1, commit.message.trim_end() ); } out.push_str("## Diff\n\n```diff\n"); - out.push_str(&collapse_lockfiles(&run.diff)); + out.push_str(&collapse_lockfiles(&strip_index_lines(&run.diff))); out.push_str("```\n\n"); out } @@ -111,6 +115,18 @@ fn collapse_lockfiles(diff: &str) -> String { out } +/// `diff` without the `index ..` line of each file's header. +/// The `diff --git` line, the other header lines and every hunk stay: a hunk +/// line starts with ` `, `+`, `-` or `\`, so none of them is matched. +fn strip_index_lines(diff: &str) -> String { + let mut out = String::with_capacity(diff.len()); + for line in diff.lines().filter(|line| !line.starts_with("index ")) { + out.push_str(line); + out.push('\n'); + } + out +} + /// One lockfile section being collapsed. struct Collapsed { path: String, @@ -264,6 +280,46 @@ diff --git a/src/main.rs b/src/main.rs assert!(shared.contains("+fn main() { run(); }"), "{shared}"); } + #[test] + fn a_rebase_that_changes_only_hashes_leaves_the_shared_context_unchanged() { + let first = run(); + let mut rebased = run(); + rebased.head_sha = "def456".to_owned(); + rebased.commits[0].sha = "def456".to_owned(); + rebased.diff = rebased + .diff + .replace("index 1111..2222", "index 3333..4444") + .replace( + "diff --git a/src/main.rs b/src/main.rs\n", + "diff --git a/src/main.rs b/src/main.rs\nindex 5555..6666 100644\n", + ); + assert_eq!(shared_context(&first), shared_context(&rebased)); + } + + #[test] + fn no_hash_reaches_the_shared_context() { + let mut r = run(); + r.diff + .push_str("diff --git a/b.rs b/b.rs\nindex 7777..8888 100644\n--- a/b.rs\n+++ b/b.rs\n@@ -1 +1 @@\n-a\n+b\n"); + let shared = shared_context(&r); + assert!( + shared.starts_with("# Pull request hyperhive/hyperhive#7\n\n"), + "{shared}" + ); + for hash in ["abc123", "7777", "8888"] { + assert!(!shared.contains(hash), "{hash} in {shared}"); + } + assert!( + shared.contains("### Commit 1\n\nnix: add a thing"), + "{shared}" + ); + assert!( + shared.contains("diff --git a/b.rs b/b.rs\n--- a/b.rs"), + "{shared}" + ); + assert!(shared.contains("-a\n+b\n"), "{shared}"); + } + #[test] fn a_quoted_path_is_read_from_its_b_side() { assert_eq!(