fix(openai): codex streaming arg routing + SSE/routing/cache fixes + cli-cycle quality pass + cache-read pricing & tier-model validation - #410
Merged
Conversation
Propagate OpenAI cached token details through provider transforms, traces, compat responses, and spend logs so cached input is visible apart from billable input.
Agent status and tool-result turns can carry the full prompt even when the next step is simple bookkeeping. Let tiers target virtual models so those turns can use fast model slots while complex turns stay on the default route.
Codex non-streaming Responses can return text and tool-call deltas with an empty completed output. Parse those deltas, surface failed and incomplete terminal events, and keep local protocol parse errors out of the retry loop.
…t_index drift On a multi-item Codex Responses stream (reasoning + preamble message + function_call), the upstream output_index can be absent on the function_call's output_item.added (so the block registers under index 0) yet present on the argument deltas (e.g. 2). resolve_responses_fc_output_index trusted the raw output_index first, resolving the deltas to an unregistered index and silently dropping them — Codex then received the tool call with empty arguments and ended the turn without executing. Resolve the block by the call's stable identity (item_id / call_id) before the raw output_index, so each delta and the consolidated output_item.done reattach to the block its added opened, regardless of output_index drift. Add a regression test driving the upstream multi-item SSE with an empty done arguments field (args sourced only from deltas): it fails before the fix and passes after.
…e output_index drift
RUSTSEC-2026-0173 flags proc-macro-error2 (build-time proc-macro pulled via age -> i18n-embed-fl) as unmaintained, failing the cargo-deny pre-push hook. It is compile-time only with no safe upgrade available, so ignore it with justification. Drop the stale RUSTSEC-2025-0134 ignore: rustls-pemfile left the dependency tree and the advisory no longer matches any crate.
Destynova2
enabled auto-merge (squash)
June 8, 2026 14:12
Cache-read tokens are excluded from billable input (they are not re-priced as full input), but the cost calculator only took (input, output), so reads were billed at $0. Add CACHE_READ_COST_RATIO (0.1x input, the Anthropic standard) plus ModelPricing::calculate_cache_read, and thread cache_read_tokens through calculate_cost / with_pricing on the streaming, fan-out, and telemetry spend paths. Token-count metrics stay unchanged (real input); only the cost gains the cache-read component.
A tier may target a pass-through model — forwarded verbatim to a pass_through=true provider and intentionally absent from [[models]]. validate_tiers hard-failed on such configs while the router and fan_out validators only warn. Align tier.model with them (warn); the provider reference stays a hard error.
Destynova2
changed the title
…ilable PricingTable carries an optional per-model cache-read rate. The OpenRouter feed's input_cache_read is parsed and used verbatim; models without it fall back to CACHE_READ_COST_RATIO x input (the prior flat approximation). Refines the cache-read cost from 5613285 with real provider rates when supplied.
Exercises the full retryable set (429/500/502/503/504), terminal codes (400/401/403/404/409/422/501), and the 401-with-rate-limit-payload exception.
A second parallel function_call whose output_item.added carried no output_index collided on default index 0: the duplicate guard dropped it and its argument deltas leaked onto the first block. Allocate a fresh synthetic index (far above any real output_index) when 0 is already held by another index-less call, so each parallel call keeps its own tool_use block.
try_rotate_and_retry rotated once and gave up even with more pooled keys left. Loop over the remaining keys, giving each its own retry budget; stop on the first success, a non-rate-limit failure, or pool exhaustion (rotate_key_pool returns false). Bounded by pool size.
Drives a real grob server (ephemeral port) against a mockito OpenAI Responses upstream, exercising the full handle_responses -> dispatch -> provider -> re-encode SSE path that previously had only unit coverage. Asserts the non-streaming round-trip (object:response/status:completed, message + function_call items), the streaming round-trip (response.created / output_text.delta / completed, function_call args preserved), and the Codex upstream contract (store=false, instructions forwarded verbatim, stream=true).
…d model Virtual keys carried an allowed_models scope that was provisioned but never enforced — a scoped key could reach any model. Enforce it via one helper at the dispatch choke point: on the inbound model in prepare_dispatch (covers /v1/messages, /v1/chat/completions, /v1/responses, count_tokens) and again on the resolved logical model after routing and tier resolution — closing the think/background/websearch/auto-map/subagent-tag and [[tiers]].model bypasses — before any provider mapping or upstream call. Generic 403 (no model-list leak), canonical-vs-canonical comparison, empty/absent list = unscoped. Tests: helper, per-surface inbound, and a real dispatch() wiring test that is red without the resolved-model check.
Add an optional allowed_providers scope to virtual keys, enforced by intersection-filtering candidate mappings in resolve_provider_mappings (empty/absent = unscoped). Applied to every generation surface AND to count_tokens, which now routes through the resolver instead of find_model so a scoped key can no longer trigger a provider call outside its scope. A generic 403 (no provider leak) fires before any upstream call when the scope leaves no mapping. The scope is threaded through the store, CLI, and the RPC key-create path. Adds a shared #[cfg(test)] test_app_state builder (no global Prometheus recorder) and red-without-fix tests for the filter and count_tokens.
…ctive provider ResolvedPolicy carried budget/rate_limit/routing/dlp/log_export overrides but only `hit` was ever consumed, and the policy RequestContext was populated with empty provider/route_type/cost/dlp_triggered so provider- and route-keyed policies never matched. Populate the context post-routing and enforce the budget and rate_limit overrides per effective provider inside the provider loop — after scorer reordering and circuit-breaker/health skips, and for each fan-out participant — so a provider-keyed policy gates the upstream actually tried (including fallbacks). The rate_limit override uses a dedicated limiter (no collision with the pre-handler middleware). routing/log_export/dlp overrides and zone remain deferred follow-ups. Tests drive real dispatch() (budget -> BudgetExceeded before the mock provider; rate_limit rps=1 -> second dispatch RateLimitedLocal; provider-keyed fallback match), red without the per-candidate enforcement.
dispatch_non_streaming never wrapped the response, so human-in-the-loop tool authorization fired only on streaming requests — a non-streaming client bypassed it entirely. Apply the same HIT gate after the provider response and before cache/telemetry: inspect tool_use blocks, authorize via the shared HitAuthorization (Simple/MultiSig/Quorum) and the same signed HMAC receipt as the streaming path, with a bounded wait (deterministic timeout, never an indefinite block). On deny/timeout the tool_use is actually stripped from the returned response. No HIT policy leaves the non-streaming path unchanged. Tests drive real dispatch() (deny strips the tool_use, red without the wiring) and cover the bounded timeout and signed-receipt verification.
… HTTP TraceLayer
The ~45 Prometheus metric families were exported without HELP/TYPE metadata,
and the pulled tower-http `trace` feature was never wired. Register a describe
for every emitted family (catalog in METRIC_FAMILIES) and add a non-circular
test that walks src/ for metrics::{counter,gauge,histogram}! call-sites and
fails if any emitted family lacks a catalog entry. Add a tower-http TraceLayer
for HTTP spans that excludes /metrics, /health, /live, /ready and never
captures the Authorization header or request/response bodies. /metrics stays
Prometheus-only without the otel feature; OTLP metrics export (a metrics->OTLP
fanout recorder) is deferred to a follow-up, and authenticating /metrics is a
separate security follow-up.
…ma validation Pledge profiles were hardcoded and resolved fail-open — an unknown profile name fell back to FULL (all tools). Make profiles configurable in TOML (exact names plus glob patterns validated at load) and fail closed: an unknown profile resolves to none at runtime and is rejected at config load; the four built-ins are kept. Add an inbound tool-schema well-formedness validator (name present, valid JSON-Schema type/required/properties) — explicitly NOT catalog membership, so arbitrary well-formed client tools are preserved; malformed tools are stripped and logged by default, with an opt-in reject (400) mode, wired into dispatch before the provider call. Tests: pattern allow-list, invalid-pattern load error, the three malformed-schema cases stripped, custom tool kept, and a real dispatch() wiring test (red without the validation step).
… guards Add deploy/helm/grob, a Helm chart for deploying grob on Kubernetes. The chart renders a Deployment, Service, ConfigMap, Secret, ServiceAccount and a PVC by default, with optional Ingress, NetworkPolicy, ServiceMonitor and HPA. Four safety properties are baked in: a PVC mounted at GROB_HOME persists the rotating OAuth tokens and append-only spend journals across restarts (Recreate strategy, never two pods on one RWO volume); a budgetGuard that fails the render when replicaCount>1 or HPA maxReplicas>1 without acknowledging that spend enforcement becomes per-pod; a protected control plane (ClusterIP by default, optional Ingress restricted to /v1 and /health, optional default-deny NetworkPolicy); and a ConfigMap/Secret checksum annotation that rolls pods on config change. Pods run with a strict security context (non-root 65534, no capabilities, read-only rootfs, seccomp) and no mounted service-account token. Validated with helm lint and helm template; the README documents the budget/PVC constraints.
…tel feature The telemetry slice deferred OTLP metrics export; wire it now without re-instrumenting any call site. Build the Prometheus recorder and keep its handle for /metrics, then -- only under the otel feature and when OTLP metrics are configured -- install a metrics_util FanoutBuilder that forwards the existing `metrics` instrumentation to both Prometheus and a new OTel recorder. The recorder (src/shared/otel_metrics.rs) holds the SdkMeterProvider so the PeriodicReader keeps pushing, caches one OTel instrument per family and one handle per key (preserving counter/gauge state across macro re-registration), and maps counter/gauge/histogram to the matching OTel instruments with metrics labels as attributes. Without the otel feature the path is unchanged (Prometheus only). Tests: the default suite is unchanged (1355); under otel the fanout re-emits to Prometheus and creates exactly the expected OTel instruments (1359).
Add deploy/grafana/grob-overview.json, a ready-to-import Grafana dashboard driven by grob's Prometheus/OTLP metrics: request rate by model and by provider (provider fallback visible), p50/p95/p99 latency from the request-duration histogram, outcomes by status, provider errors, token throughput, and spend vs. budget. The README documents running it against the grafana/otel-lgtm stack, wiring grob's [otel] metrics export, and the chart's ServiceMonitor path.
Surfaces under the CI `cargo deny --all-features` invocation via axum-server and rustls-acme. PEM parsing only, no security vulnerability; drop when those crates migrate off rustls-pemfile.
GitHub Dependabot flagged the locked openssl 0.10.76 (pulled via reqwest's native-tls and opentelemetry-otlp) for several high-severity issues fixed by 0.10.78-0.10.80: a heap overflow in AES key-wrap-with-padding, out-of-bounds writes in Deriver::derive and MdCtxRef::digest_final, and adjacent-memory leaks in the PSK/cookie trampolines. Bump openssl to 0.10.80 (openssl-sys 0.9.116) and the low-severity rand 0.8.5 to 0.8.6. cargo-deny did not catch these because openssl is target-gated (the host target uses the platform TLS stack).
The /metrics endpoint exposed spend, budget and tenant labels with no auth. Add an opt-in bearer token ([metrics] bearer_token / bearer_token_file): with no token configured /metrics stays public (unchanged); with one set, a request needs a matching Authorization: Bearer header or gets a generic 401, while /health, /live and /ready stay public. The token is compared length-hiding (both sides hashed to 32-byte SHA-256 digests, then constant-time compared) so its length never leaks via timing. Because the token resolves once at startup, a shared guard rejects any reload -- HTTP /api/config/reload and the RPC path -- that would change it, before persisting, with a clear "restart required"; an unreadable bearer_token_file is treated as a blocking change rather than silently leaving /metrics public. The Helm chart mounts the token from a Secret as a file (never inline in the ConfigMap) and wires the ServiceMonitor bearerTokenSecret.
The published binary already used rustls, but openssl 0.10.x (flagged high-severity by Dependabot) re-entered the tree through reqwest's default native-tls -- once via the dev-dependency reqwest and once via opentelemetry-otlp's HTTP exporter. Pin both reqwest entries to default-features = false + rustls-tls-webpki-roots and switch opentelemetry-otlp to grpc-tonic with no default features. openssl, openssl-sys, native-tls, tokio-native-tls, hyper-tls, security-framework and core-foundation are gone from the lock; OTLP export stays on gRPC (tonic). Also unify duplicate crates where safe (crossterm 0.29, a single reqwest 0.12 and rustls 0.23); the remaining duplicates are major-version-bound transitive deps. cargo deny --all-features passes and the otel feature builds and tests clean (1376).
… feature) Logs were the one OpenTelemetry signal grob never emitted -- traces and metrics went out over OTLP but logs stayed on stdout only. Add an opt-in [otel] logs flag that, under the otel feature, builds an OTLP log exporter over gRPC (tonic, the same endpoint and service name as traces) behind an SdkLoggerProvider and wires an opentelemetry-appender-tracing bridge layer into the tracing subscriber, so tracing events are emitted as OTLP log records (and still printed to stdout). The bridge filters out opentelemetry/tonic/h2/hyper/tower to avoid an export feedback loop, the provider is held in a OnceLock and flushed on shutdown, and the whole path stays on rustls/tonic (no openssl/native-tls reintroduced). Without otel.logs=true the behaviour is unchanged.
Completes the three-signal view: the grob dashboard now shows metrics (Prometheus), logs (Loki) and traces (Tempo, TraceQL service.name=grob) side by side.
…exemplars Rename the dashboard to 'grob — OTel (metrics · logs · traces)', wire latency exemplars (Prometheus -> Tempo) and add a Tempo service-graph node panel. Trace -> logs correlation already works via the Tempo datasource (tracesToLogsV2 on trace_id); exemplars light up once grob attaches trace context to metric samples.
…metrics Wire grob's half of "click a latency point -> open the trace": attach the active request span's OpenTelemetry context to the current OTel Context around the per-request metric recording, so a trace-based exemplar reservoir can capture the trace_id. tracing-opentelemetry keeps the OTel span in the tracing-span extensions but not on the OTel thread-local Context, so without this the metrics call site sees an empty Context. Gated on the otel feature and attached only when the span carries a valid recording context. This is for now a no-op for exemplars because opentelemetry_sdk 0.28 hardcodes empty exemplars with no reservoir API; it becomes effective once the OTel stack moves to >= 0.30 with ExemplarFilter::TraceBased (see docs/explanation/otlp-exemplars.md).
…version issue opentelemetry-rust does not capture exemplars at any released version (verified through 0.32.1: the histogram aggregator hardcodes empty exemplars and exposes no ExemplarFilter/reservoir API), so the earlier 'effective at >= 0.30' note was wrong. The trace-context attach in record_request_metrics stays as correct groundwork; only a one-line with_exemplar_filter will be needed once upstream implements capture.
gpt-5.3-codex returns 400 on this ChatGPT account (no entitlement), which killed every mid-tier turn. Mapping medium onto dev collapses it into the complex tier (both gpt-5.5) until a cheaper entitled model is available.
The setup-wizard and responses-e2e tests point the process-wide GROB_HOME at a tempdir. Under plain `cargo test` (the coverage job) every test shares one process, so default_path() can observe that override and the bare `.grob` assertion races. nextest never hits this because it isolates each test in its own process.
Contributor
Mutation testing (PR diff sample)Informational — never blocks merge. Full matrix runs on main.
Legend: clean (no survivors), missed (inspect artifact), timed-out (25 min cap reached). Artifact: mutants-pr-results-c8352869452890a88b71879803343913c59c9e6d. |
This was referenced Jun 12, 2026
Merged
Destynova2
added a commit
that referenced
this pull request
Jun 13, 2026
The mutation matrix flagged three MISSED `delete !` mutants in code merged via #410: - scan_dlp_input:693 `!reports.is_empty()` (DLP triggered flag) - dispatch:346 `!stripped.is_empty()` (tool-validation warn gate) - try_rotate_and_retry:239 `!rotation_unavailable(...)` (key-rotation loop) Following the module's established pattern (dlp_input_scan_disabled, should_escalate_compliance, rotation_unavailable), extract each `!` into a small #[inline] helper and unit-test both directions, rather than excluding them in mutants.toml. The log-gate test uses tracing-test's #[traced_test] to assert the warn fires only on a non-empty strip list. Each test was verified to fail when its `!` is deleted, proving it kills the mutant.
Destynova2
added a commit
that referenced
this pull request
Jul 24, 2026
chore: untrack the local debug config committed in #410
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Bundles four related openai/codex changes, an automated cli-cycle quality pass (per slice), and two follow-up fixes addressing deferred cli-cycle findings.
Slices
function_callarg deltas byitem_idto surviveoutput_indexdrift — fixes the empty-arguments Codex tool-call stall. Regression test (red-without-fix / green-with) + live end-to-end verified.504not retried at provider level, unboundedretry_delayoverflow.)cli-cycle quality pass (one fixup per slice)
Safe readability / local-perf / error-handling improvements + added tests, per slice; risky/cross-cutting findings deferred.
Follow-up fixes (deferred items, now addressed)
CACHE_READ_COST_RATIO(0.1× input, Anthropic standard) +calculate_cache_read, threadedcache_read_tokensthrough the streaming / fan-out / telemetry cost paths. Token-count metrics unchanged; only cost gains the cache-read component.tier.modelabsent from[[models]]now warns instead of erroring, matching the router/fan_out validators — allows pass-through tier models. Provider refs stay a hard error.Verification
cargo test --lib: 1314 passed / 0 failedcargo build,fmt,clippy,cargo deny,cargo audit— all green