proxy-pair grouping misses turns split across profiles by a header-stripping proxy leg

[vaderyang]

Summary

A header-stripping proxy (one that drops User-Agent / X-Claude-Code-Session-Id
etc. before forwarding upstream) caused the proxy-pair grouping to miss turns
split across profiles. The inbound leg classified by header (e.g. claude-cli
with a UUID session_id), the outbound leg fell through to a body-fingerprint
profile (e.g. generic with a gen-<hash> session_id). Both agent_kind
AND session_id diverged across the boundary even though the body — and
therefore wire_api, final_finish_reason, primary_model, call_count,
and tokens — passed through unchanged. The strict content fingerprint
(session_id, agent_kind, call_count, tokens) admitted no cluster.

Fix

Replaced the strict-fingerprint-only group_all with a body-bucket pass that
clusters by (call_count, total_input_tokens, total_output_tokens) — the
fields a proxy cannot rewrite without altering the body — and admits a
cluster under either of two complementary rules:

• Strict — every member shares session_id + agent_kind. Catches
  header-preserving proxies (haproxy, mirror capture, LiteLLM translation).
• HeaderStrip — every member shares wire_api + final_finish_reason +
  primary_model AND members span ≥2 distinct agent_kinds. Catches
  header-stripping proxies. The diversity guard is the discriminator that
  separates a real header-strip from two coincidentally-same-shape unrelated
  calls in different sessions (which would also share tokens + wire_api +
  finish_reason + model but share one agent_kind).

Both rules can apply simultaneously in a mixed topology (e.g. an inbound
mirror pair on claude-cli plus a stripped upstream hop on generic):
the strict rule covers the mirror, the header-strip rule covers the
mixed-classification cluster, and a single group is emitted for the
whole set.

Tests

Four new unit tests in server/h-turn/src/proxy_pair.rs:

1. header_strip_proxy_leg_pairs_across_profile_split — two-leg case
   (claude-cli inbound + generic outbound) folds into one group.
2. header_strip_pairs_three_legs_when_one_leg_classifies_differently —
   three-leg haproxy topology where the upstream hop is re-classified.
3. header_strip_does_not_pair_when_wire_api_differs — header-stripping
   alone never translates wire_api; differing wire_api is not a
   header-strip signature.
4. header_strip_does_not_pair_unrelated_same_token_calls — same
   agent_kind + same wire_api + different sessions = no diversity,
   no pair.

All 52 existing h-turn tests, 4 pair_sweeper tests, and the full workspace
test suite remain green.

Scope

314 lines added / 37 removed in a single file (server/h-turn/src/proxy_pair.rs).
No new dependencies, no new secrets, no new network calls, no CI changes.

Closes #169

---

🤖 Implemented by wiwi • issue author: @vaderyang
Eligible for auto-merge on vivi APPROVE.

[github-actions]

agent_kind VARCHAR NOT NULL — but NOT NULL permits empty string "". The Strict rule admits a cluster where all members share agent_kind="". That's technically a real agent_kind, so it's not a false positive — it's the strict behavior preserved. Fine.

Now let me check whether the change introduces any issue for the existing pairs_across_api_style_translation test where agent_kind="openclaw" for both legs but wire_api differs. Strict admits (same session_id "S" + same agent_kind "openclaw"). Good — still pairs via Strict, not HeaderStrip.

Let me verify one final concern: the header_strip_does_not_pair_when_wire_api_differs test has agent_kind=claude-cli vs agent_kind=generic (≥2 kinds ✓) but wire_api differs. The HeaderStrip rule checks wires.len() != 1 → returns false → cluster dropped. Good. But this is also the LiteLLM translation case combined with header-stripping. Note that Strict also fails here (sessions differ). So nothing pairs. The test asserts groups.is_empty(). Correct.

I have enough to write the review. Let me also quickly check whether there's a sibling test covering "two unrelated calls, different agent_kinds, same body shape" that the PR doesn't add but should — the closest is header_strip_does_not_pair_unrelated_same_token_calls which uses same agent_kind="generic" for both legs, so it doesn't exercise the dangerous case (different kinds + same body). Let me note this as a Suggestion for test coverage.

Summary

PR #170 replaces the strict content-fingerprint-only group_all bucketing with a two-rule admission policy that additionally folds turns split across a header-stripping proxy: a Strict rule (same session_id+agent_kind, the prior behavior) plus a HeaderStrip rule (shared wire_api+final_finish_reason+primary_model and ≥2 distinct agent_kinds). The change is isolated to server/h-turn/src/proxy_pair.rs, adds four targeted tests, and preserves all existing test invariants. The design is sound and the test scenarios correctly cover the issue-#169 case plus the obvious negatives. Recommendation: APPROVE — with two non-blocking suggestions about a residual false-positive class and test coverage for that class.

Suggestions

• server/h-turn/src/proxy_pair.rs:300–305 — The new bucket key is (call_count, total_input_tokens, total_output_tokens) only. The old key included session_id+agent_kind, which made buckets highly selective. Under a 30-min sweeper lookback (pair_sweeper.rs:63), a high-rate cache-hit workload (many identical trivial exchanges — e.g. call_count=1, in_tok=10, out_tok=5) can land many turns in one bucket. time_clusters is O(n) per bucket and admission is O(n log n) per cluster, so this stays bounded in practice, but if you ever see sweeper CPU climb on a chatty deployment, this is the first place to look. Worth a one-line comment on the bucket noting the cardinality tradeoff vs. the old key.
• server/h-turn/src/proxy_pair.rs:396–429 — The HeaderStrip diversity guard (≥2 distinct agent_kinds) is the only discriminator against a genuine false positive: two unrelated calls in the same 100ms window from two different agent profiles (e.g. one claude-cli user and one codex-cli user) that happen to send the same trivial prompt (cached, identical token counts) to the same model with the same finish reason, with the second turn's timing nested inside the first by coincidence. The guard rejects the same-agent_kind coincidence case (covered by header_strip_does_not_pair_unrelated_same_token_calls), but not the different-agent_kind coincidence. Real-world rate is probably low (time-nesting across unrelated calls + identical exact-token sums is rare), but it's the one residual class the test suite doesn't exercise. Consider adding a negative test: two different agent_kinds, same tokens/wire/finish/model, same session_id not shared, time-nested — assert whether it should pair or not. Right now the code would admit it; if that's intentional, pin it with a test so a future refactor doesn't silently flip it.

Questions

• server/h-turn/src/proxy_pair.rs:294–299 — The doc comment says "Both rules can apply simultaneously … the strict rule covers the mirror, the header-strip rule covers the mixed-classification cluster, and a single group is emitted for the whole set." But in the actual code, Strict and HeaderStrip are both evaluated on the whole cluster — Strict returns false the moment any member has a different agent_kind (so for the 3-leg header_strip_pairs_three_legs_when_one_leg_classifies_differently case, Strict sees all three and returns false; only HeaderStrip admits). Is the "both apply simultaneously" framing describing a topology that genuinely exercises both rules on the same cluster, or is it aspirational? If the latter, the comment slightly overstates the mechanism — a reader will expect per-subset admission.

Verified

• Single-file scope: diff is server/h-turn/src/proxy_pair.rs only — no console, schema, deploy, or SQL changes. No console/src/types/api.ts mirror needed.
• group_all callers: only server/h-storage/src/pair_sweeper.rs:132 consumes it via h_turn::proxy_pair::group_all (re-exported at h-turn/src/lib.rs:13). Signature unchanged (&[PairCandidate] -> Vec<ProxyGroup>); sweeper test sweep_once_folds_three_leg_haproxy_topology still exercises the canonical/role assignment path.
• PairCandidate projection SQL (h-storage-duckdb/src/turns.rs:639–654): reads models_used (small JSON list, not the body column), final_finish_reason, wire_api, agent_kind, session_id, token counts — no LENGTH(body)/MAX(body)/arg_max(body, …) shape. No body-scan smell.
• Existing test invariants preserved: traced does_not_pair_across_sessions (different sessions, same agent_kind → HeaderStrip rejects via kinds.len() < 2), does_not_pair_same_network_view (dropped by assign_roles, not admission), pairs_across_api_style_translation (admitted by Strict, not HeaderStrip), does_not_pair_when_tokens_differ (different body buckets). All still hold under the new admission logic.
• Leakage scan of diff + test data: IPs used are 192.0.2.x (RFC5737), 172.17.0.x (docker0 default) — both in the allowed documentation/default classes. Session IDs are obviously synthetic (deadbeef-…, gen-<hex>). No hostnames, usernames, machine paths, credentials, or private-key material in the diff. Commit message is generic. Clean.
• Schema constraint: agent_kind VARCHAR NOT NULL (schema.rs:125), wire_api VARCHAR NOT NULL, session_id VARCHAR NOT NULL — Strict's !sessions.is_empty() guard is satisfiable but NOT NULL doesn't preclude empty string; if a real turn ever had agent_kind="" all members would share it and Strict would still admit. Not a regression vs. prior behavior.

---

🤖 Reviewed by the review bot • workflow run

[vaderyang]

to be verified in more scenarios