vinzenz/hyperhive
vinzenz/hyperhive
Watch
0
0
Fork
You've already forked hyperhive
 0
Code Activity

review classifier: commit shas and index lines stay out of the prompt

Browse source 
Every classification's shared context began with the PR's head sha,
headed each commit with its sha, and carried each file's
`index <hash>..<hash>` 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.
This commit is contained in:
atlas 2026-10-14 10:43:41 +02:00 • committed by mara
commit 74b63dd031
4 changed files with 190 additions and 35 deletions
Download patch file Download diff file Expand all files Collapse all files

15
docs/swarm/review-classifier.md
View file

@ -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 <hash>..<hash>` 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)).

5
swarm-review-classifier/src/acp.rs
View file

@ -175,6 +175,7 @@ impl<S: CostSink> Acp<S> {
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]

129
swarm-review-classifier/src/cost.rs
View file

@ -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<Option<(SdkMeterProvider, Counter<f64>)>> = OnceLock::new();
let Some((_, counter)) = COUNTER.get_or_init(build) else {
static COUNTERS: OnceLock<Option<(SdkMeterProvider, Counters)>> = 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<f64> {
provider
.meter("hyperhive.review_classifier")
/// The cost counter, and the token counter beside it.
struct Counters {
cost: Counter<f64>,
tokens: Counter<u64>,
}
/// 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()
.build(),
tokens: meter
.u64_counter("hyperhive.review_classifier.token.usage")
.build(),
}
}
/// Adds `cost` to `counter` with the run's labels.
fn add(counter: &Counter<f64>, cost: &Cost) {
counter.add(
cost.amount,
&[
/// 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()),
KeyValue::new("currency", CURRENCY),
],
);
];
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 counter since it exports only while it
/// 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<f64>)> {
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<f64>)> {
.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),
]
);
}
}

68
swarm-review-classifier/src/prompt.rs
View file

@ -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 <hash>..<hash>` 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!(