Repository navigation
docs: fix stale truncate_filename docstring examples (RR-1385) - #1
Open
erichanwang wants to merge 1 commit into
Open
erichanwang wants to merge 1 commit into
erichanwang wants to merge 1 commit into
Conversation
The Usage examples in truncate_filename's docstring showed incorrect output strings that did not match the implementation: - 'very_long_document_name.pdf' @ 20 used to be documented as 'very_long_docu....pdf' but the function actually returns 'very_long_doc....pdf'. - 'file.very_long_extension' @ 15 was correct already. Updated the docstring to show the real return values. No code change; the implementation was never affected. The RR-1385 regression tests continue to pass. Co-authored-by: Buffy <noreply@codebuff.com>
dsapandora
added a commit
that referenced
this pull request
Sep 8, 2026
…ride-org#1780) * feat(llm): capture exact provider token usage in the Adapter for per-user billing Read provider-reported usage (input/output tokens) at the normalized Adapter (rocketride-org#1683) and report it via metrics.counter so per-user token accounting flows through task_metrics/billing. One capture point covers all LangChain providers; the native Anthropic extended-thinking and OpenAI Responses paths are instrumented too. * feat(llm): cover OpenAI streaming usage and split cache tokens; add tests Addresses review on rocketride-org#1780: - Enable stream_usage for OpenAI-family models so streaming reports usage (LangChain does not emit usage_metadata for ChatOpenAI without it). Verified live: gpt-5.4-mini now reports input=21/output=11 where it reported nothing. - Report cache-read and cache-creation tokens on their own counters (disjoint from fresh input) since providers bill them at different rates; captured from the Anthropic native, OpenAI Responses, and LangChain paths. - Log suppressed reporting failures via debug instead of a silent pass. - Add unit tests for report_llm_tokens and the usage/cache split. * fix(llm): report token usage from the finally so partial-stream failures still bill Move report_llm_tokens into the finally of both native adapters (NativeAnthropicAdapter, NativeOpenAIResponsesAdapter) so a mid-stream error after message_start/message_delta still records the provider-reported usage. Adds a regression test that raises mid-stream and asserts the partial usage (input + cache) was recorded. * fix(llm): capture usage on failed OpenAI Responses terminal events response.failed can carry a usage object; read usage on failed/error terminal events (not only response.completed) so a failed-but-chargeable request reports tokens instead of zero. finish_reason stays 'error'. Adds a regression test. * fix(llm): treat response.incomplete as a terminal event in the OpenAI Responses adapter OpenAI Responses can emit response.incomplete separately from response.completed (e.g. max_output_tokens). Handle it as terminal: read its usage and map incomplete_details.reason to finish_reason, so a truncated-but-chargeable request still records tokens instead of zero. Adds a regression test. * fix(llm): capture LangChain token usage for every provider, not just OpenAI The stream_usage gate matched 'OpenAI' in the class name, so ChatXAI never asked for usage and silently reported none, even though it inherits the flag from BaseChatOpenAI; ChatBedrock does not declare the field at all. Gate on the declared flag instead. Report from a finally as well, so a stream that breaks mid-way still records the usage its chunks already carried, matching the two native adapters. * feat(llm): surface per-call token usage in the pipeline Trace report_llm_tokens publishes each call's usage on a ContextVar; the LLM node hangs it on Answer.tokens, which the Trace 'Tokens' grid already renders. One hook at the shared Adapter seam covers every provider. * fix(llm): accumulate per-turn token usage with a per-call breakdown for the Trace The counters and the llm_tokens event stay per-call (billing path unchanged), but the context var feeding answer.tokens now sums every model call in the turn and carries calls plus a breakdown listing each call's token cost. A ReAct/CrewAI turn that hits the model N times shows the whole turn's total and what each call cost in the Trace and action history, instead of only the last call. * fix(trace): render per-call token breakdown instead of crashing on the object The Tokens grid mapped answer.tokens as flat scalars; the new per-call breakdown (an array of objects) made React throw 'Objects are not valid as a React child'. Split the scalar totals (rendered as chips) from breakdown (one row per model call) and type answer.tokens as TokenUsage. * fix(agent): carry per-turn token usage onto the agent's answer The Trace "Tokens" grid renders answer.tokens, but that field was only ever set by the LLM node's own questions-lane path. An agent emits its own Answer from AgentBase.run_agent, so every agentic run reached the Trace with no usage at all — the case report_llm_tokens' calls/breakdown accumulation was written for. Read the same context var here and attach it when present. Covers all five agent frameworks, since they share this base. * feat(trace): show each model call's tokens on its own invoke row An 11-call agent run listed 11 breakdown blocks under the answer, none of them tied to the call that produced them. The data was only ever aggregated onto the final answer, which also arrives after every invoke has already rendered. ask() now attaches its own slice of the turn — the breakdown entries appended while it ran — so each Anthropic INVOKE row shows what that call cost, through the renderer the row already uses. The answer keeps the turn total and drops the per-call list. * fix(llm): thread- and nesting-safe turn usage collector; cover the review findings Replace the per-turn ContextVar with a module-global, lock-guarded usage collector scoped by turn_usage(). A ContextVar could not carry usage back from CrewAI/deepagent kickoffs, which run on a worker thread under contextvars.copy_context() — the agent's answer showed no tokens (dylan rocketride-org#3). The collector spans threads like the metrics singleton, and a seen-mark slice makes it nesting-safe: an agent can drive sub-agents, each its own scope, while the outermost scope reads the whole turn and clears the list so it stays bounded to one turn (asclearuc/dylan rocketride-org#2). Also from the reviews: - reasoning stream (try_openai_compat_reasoning_stream) now asks for include_usage, reads the usage-bearing final chunk, and reports on success — it billed zero before (dylan #1). It falls back to non-streaming if a strict proxy rejects the flag. - do not force stream_usage on a custom base URL (vLLM/proxies 400); usage is still read if the endpoint sends it (dylan rocketride-org#4). - drop the per-call metrics.event that fed an unbounded, cumulatively re-serialized events list (dylan rocketride-org#5); the counters carry the billable totals. - correct the max()-vs-additive comment (asclearuc/dylan rocketride-org#6). Tests: turn total, per-invoke slice, bounding, cross-thread, nested sub-agents, and no bleed between consecutive turns. * test(trace): cover the answer Tokens grid render Per the review: pin that the per-call breakdown is dropped from the chip grid, a tokens object holding only a breakdown renders no Tokens section, and the non-numeric model chip is stringified. Follows the media_renderers.test.tsx harness (node --import tsx --test via shared:test). * test(llm): assert no per-call event is emitted (it was dropped for O(N^2)) test_emits_llm_tokens_event_payload asserted the per-call llm_tokens metrics.event that this branch removed. Flip it: the event must NOT be emitted; the counters carry the totals. * fix(llm): per-turn usage collector safe under concurrent turns Move the turn collector from a module global to a ContextVar holding a mutable list appended in place. Overlapping turns (tool waves, one pipeline per message) each get their own list, so a Trace never shows another turn's tokens and the list dies with its scope (no global clear, no unbounded growth under load). copy_context() shares the list object, so worker-thread kickoffs still land in the open turn; run the rocketride tool wave under copy_context().run so its calls propagate. Meter the whole OpenAI-compatible family: ask for streaming usage on both the LangChain and native reasoning paths and retry once without the flag if the endpoint rejects it before the first chunk, instead of hard-disabling it for every custom base URL (xAI, DeepSeek, Qwen, Together, Groq, vLLM). An explicit stream_usage in stream_kwargs overrides the default. Cover the native reasoning capture point and the concurrent-turn, worker- thread and retry paths with tests. * fix(llm): isolate sibling invoke scopes; gate + log the LangChain usage retry Each turn_usage scope now owns its own list and folds it into the parent on exit, so two invokes running concurrently in one turn (a parallel tool wave) no longer cross-read each other's per-call breakdown in the Trace; the turn total is unchanged. On the LangChain usage-flag retry, warn before retrying (matching the native path) and gate it on status_code == 400 so a 401/429 surfaces without a second round trip, while a provider whose error carries no status_code keeps the broad retry. * fix(llm): gate the usage-flag retry to real 400s; stop reasoning-only double-bill Retry without stream_options only on a 400 (the flag rejection), on both the LangChain and native reasoning paths. A 401/429 or a transient connection error (no status_code) now re-raises / falls back instead of being retried without the flag: no silent unmetered success, and a rate limit stays at two round trips instead of four. On the reasoning path, report captured usage before the empty-content check and return the empty answer (the caller keeps it, since it tests `is not None`), so a reasoning-only turn is not discarded and re-issued via the non-streaming fallback — which billed the provider twice, metered once. Only a stream that produced nothing at all still falls back. * test(llm): cover the LLMBase token seams; widen the usage-flag retry to 4xx writeQuestions and ask both hang the turn's usage on the Answer and neither had a test — the plain llm_* node lane. Adds three, including a second ask on the same instance not inheriting the first's tokens. The usage-flag retry gated on status 400 only, so a FastAPI-based endpoint (vLLM's OpenAI-compatible server) answering 422 lost token-by-token streaming on every request. Both call sites now share is_usage_flag_rejection: any 4xx except 401/403/408/429. Drops the unused `seen` offset from usage_since (now aggregate_usage) — scopes isolate by owning their list, and the docstring still taught the replaced design. Barriers in the concurrency tests get a timeout so a dead thread fails instead of hanging. * fix(llm): keep the fallback for a stream that produced nothing include_usage is now always requested, so usage is set on essentially every successful stream — and gating the fallback on `usage is None` made it unreachable. A stream that emits neither content nor reasoning (a proxy that strips delta.reasoning_content, hitting max_tokens while reasoning) returned the empty string, which chat.py serves as the answer instead of re-issuing the request non-streaming. The usage report already runs before the guard, so ordering alone keeps the stream metered once; the fallback's own metering covers the second request, which the provider bills anyway. Also hang the turn's usage on the error answer: the adapters report from their finally, so the tokens are known — without this the Trace grid is empty for exactly the turn a user opens. * feat(llm): meter the five drivers that bypass the Adapter capture point LangChainAdapter meters every provider that routes through ChatBase, but five drivers override that seam and reach their SDK directly, so the Adapter never sees them and they billed zero. Two seams are overridden: _chat (gemini, ibm_watson, minimax) and the chat seam above it (perplexity, mistral). Gemini is the widest gap: it sets _client and never _llm, so the streaming gate in chat.py is never true and every call — streaming or not — took the override. Each now reports from its own call site, off its SDK's own usage object, keeping the four counters disjoint: Gemini counts prompt_token_count with the cached prefix included (subtracted back out) and bills thoughts_token_count as output; watsonx and Mistral carry no cache detail. _report_usage_metadata is now public so the two LangChain-backed drivers reuse the shared split instead of copying it. Also covers the error-path answer.tokens seam on LLMBase, which landed in 17ce15e without a test. The four llm_vision_* nodes and accessibility_describe override the same chat seam and stay unmetered: they are image-priced, so they need their own accounting decision rather than a copy of this one. * fix(llm): never let a usage read cost the user the answer they paid for The usage split runs before report_llm_tokens' own guard, so a provider or proxy answering a non-numeric count raised out of the metering code and into the caller — which is a retry loop. Verified against the real driver: a successful, already-billed Perplexity response came back to the user as "Perplexity API error: invalid literal for int() with base 10: 'n/a'". report_usage_metadata was already exposed to this before the five drivers were wired to it (LangChainAdapter.collect() calls it on every non-streaming turn); the new call sites widened the same hole to five more providers. Guards the split in report_usage_metadata and in the three SDK readers, sharing one debug_usage_failure() so every capture point fails the same way — the trace loses that call's tokens, the turn keeps its answer. One regression test per driver. * refactor(llm): drop the redundant usage guard in the two pass-through readers report_llm_tokens coerces its own arguments inside its own try and reports the failure through the same debug_usage_failure, so the local try in the watson and mistral readers could not change the outcome. Pass the raw values through: a missing count still reads as zero, a non-numeric one is swallowed one frame deeper, and the per-driver regression test holds. llm_gemini keeps its guard — it subtracts and adds the counts before calling.
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
truncate_filename's docstring(
packages/client-python/src/rocketride/cli/utils/formatters.py).'very_long_document_name.pdf' @ 20was documented as'very_long_docu....pdf'but the function actually returns'very_long_doc....pdf'.'file.very_long_extension' @ 15example was already correct.regression tests continue to pass after the edit.
Test plan
test_formatters_truncate.py) pass against thepatched module (ran locally; 4/4 OK).
truncate_filenameoutput.Resolves RR-1385
🤖 Generated with Codebuff
Co-Authored-By: Buffy noreply@codebuff.com