Skip to content

feat: add opt-in estimated API cost reporting - #2817

Merged
IsmaelMartinez merged 12 commits into
The-PR-Agent:mainfrom
elijahchancey:feature/estimated-api-cost
Aug 27, 2026
Merged

feat: add opt-in estimated API cost reporting#2817
IsmaelMartinez merged 12 commits into
The-PR-Agent:mainfrom
elijahchancey:feature/estimated-api-cost

Conversation

@elijahchancey

@elijahchancey elijahchancey commented Aug 26, 2026

Copy link
Copy Markdown

Summary

  • Extend the existing run-details collector with Decimal-based known cost totals, successful/priced call counts, completeness state, and per-model aggregation.
  • Add the default-false config.output_run_cost setting. It gates cost collection, while config.output_run_details remains the only gate for public PR-comment output.
  • Render Estimated API cost as complete, partial with a priced-call count, or unavailable. Multi-model runs include a compact known-cost breakdown.
  • Document that LiteLLM-derived cost is an estimate that must be reconciled with provider billing for accounting or chargeback.

Streaming reliability

Some models, including anthropic/claude-opus-5, are forced through LiteLLM streaming. An asynchronous callback or callback kwargs response_cost is not sufficient for these calls: callback delivery can lag the completed request, and the field can be absent or still contain a zero placeholder when the stream is consumed.

This change requests finalized stream usage, retains the real usage object on the completed streaming response, and collects cost synchronously. It first accepts a positive finalized inline response cost when available; otherwise it calls LiteLLM completion_cost with the completed response and full usage object. That lets LiteLLM price cache reads, cache writes, reasoning tokens, and provider-specific categories it understands.

If a completed call has no finalized priceable usage, or LiteLLM cannot price it, the call remains successful but unpriced. Aggregates become partial or unavailable instead of treating missing data as zero. Cost aggregation retains exact Decimal values; public currency output is rounded to two decimal places, and tiny positive values that would round to zero render as less than $0.01 rather than a false $0.00.

Safety and compatibility

  • Cost collection and reporting are disabled by default.
  • config.output_run_cost alone can collect data but never publishes it when config.output_run_details is false.
  • Existing run-details output is byte-for-byte unchanged when cost output is disabled.
  • Public output contains only configured model names and aggregate cost values; it does not add keys, prompts, response bodies, provider request IDs, or other response metadata.
  • No provider calls are made by the tests.

Verification

  • Focused run-details, LiteLLM streaming, rendering, configuration, and wiring suite: 99 passed before the final collection-gate-only adjustment; deterministic mocks cover the final gate as well.
  • Full unit suite in the sandbox: 2,387 passed, 1 skipped, 1 xfailed; seven localhost HTTP-server tests were blocked only by sandbox socket permissions.
  • Two-decimal output update: 27 focused tests passed.
  • Scoped pre-commit hooks: passed.
  • Staged diff whitespace and targeted Flake8 syntax/undefined-name checks: passed.

@qodo-code-review

Copy link
Copy Markdown
Contributor

PR Summary by Qodo

Add opt-in estimated API cost reporting

✨ Enhancement 🧪 Tests 📝 Documentation ⚙️ Configuration changes 🕐 40+ Minutes

Grey Divider

AI Description

• Adds opt-in estimated LiteLLM API cost collection to run details.
• Preserves finalized streaming usage for synchronous, provider-aware pricing.
• Reports complete, partial, unavailable, and per-model Decimal aggregates.
Diagram

graph TD
A["Completed response"] --> B["Finalized usage"] --> C{"Cost enabled?"} -->|Yes| D["Cost resolver"] --> E["Decimal aggregates"] --> F["Run details"] --> G{"Details enabled?"} -->|Yes| H["PR comment"]
C -->|No| F
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Asynchronous callback accounting
  • ➕ Reuses LiteLLM callback metadata
  • ➕ Keeps pricing outside the completion path
  • ➖ Callbacks can arrive after request completion
  • ➖ Streaming costs may be absent or zero placeholders
  • ➖ Run comments can render before accounting completes
2. Provider billing API reconciliation
  • ➕ Can provide invoice-authoritative amounts
  • ➕ Supports centralized accounting workflows
  • ➖ Requires provider-specific credentials and integrations
  • ➖ Billing data may be delayed
  • ➖ Adds network calls and operational complexity

Recommendation: Keep the PR's synchronous completed-response pricing for immediate run-level estimates: it reliably uses finalized streaming usage without adding provider calls. Asynchronous callbacks are too timing-sensitive for comment generation, while billing APIs belong in a separate reconciliation system; the documentation correctly preserves that distinction.

Files changed (11) +517 / -23

Enhancement (4) +187 / -19
litellm_ai_handler.pyResolve completion costs from finalized usage +90/-11

Resolve completion costs from finalized usage

• Gates cost collection behind configuration, accepts positive finalized inline costs, and falls back to LiteLLM completion_cost for priceable usage. Streaming requests now ask for usage and pass the configured model through completion metadata recording.

pr_agent/algo/ai_handlers/litellm_ai_handler.py

litellm_helpers.pyPreserve finalized streaming usage +38/-6

Preserve finalized streaming usage

• Extracts usage from regular or metadata-only stream chunks and returns a completed response wrapper containing the full usage object and model. The wrapper serializes usage for LiteLLM pricing while retaining the original object.

pr_agent/algo/ai_handlers/litellm_helpers.py

run_details.pyAggregate Decimal costs and completeness +38/-2

Aggregate Decimal costs and completeness

• Extends run details with Decimal total and per-model costs, priced-call counts, and complete, partial, or unavailable status. Successful call recording now normalizes and accumulates valid known costs without fabricating zero values.

pr_agent/algo/run_details.py

utils.pyRender cost status and model breakdowns +21/-0

Render cost status and model breakdowns

• Formats USD values to four decimals while preserving tiny positive amounts. Adds complete, partial, unavailable, and multi-model cost output to the gated run-details section.

pr_agent/algo/utils.py

Tests (5) +314 / -4
test_litellm_chat_completion_core.pyVerify streaming usage request wiring +7/-1

Verify streaming usage request wiring

• Updates streaming completion expectations for the completed-response tuple, include_usage stream option, and model propagation into the stream handler.

tests/unittest/test_litellm_chat_completion_core.py

test_litellm_run_details.pyTest LiteLLM cost collection paths +170/-1

Test LiteLLM cost collection paths

• Covers inline and computed costs, disabled collection, zero placeholders, finalized streaming usage, and unavailable streaming costs without provider calls.

tests/unittest/test_litellm_run_details.py

test_run_details.pyTest Decimal cost aggregation states +45/-0

Test Decimal cost aggregation states

• Validates zeroed defaults, per-model Decimal accumulation, fallback runs, and complete, partial, and unavailable cost states.

tests/unittest/test_run_details.py

test_run_details_wiring.pyVerify independent output gates +9/-2

Verify independent output gates

• Checks the new setting defaults to false and confirms cost collection alone cannot publish run details or cost output in PR comments.

tests/unittest/test_run_details_wiring.py

test_show_run_details.pyTest cost rendering and compatibility +83/-0

Test cost rendering and compatibility

• Verifies unchanged output when cost reporting is disabled, plus complete and partial totals, unavailable pricing, per-model breakdowns, rounding, and tiny positive values.

tests/unittest/test_show_run_details.py

Documentation (1) +15 / -0
additional_configurations.mdDocument estimated API cost reporting +15/-0

Document estimated API cost reporting

• Explains the separate collection and publication gates, output formats, streaming behavior, privacy boundaries, and the need to reconcile estimates with provider billing.

docs/docs/usage-guide/additional_configurations.md

Other (1) +1 / -0
configuration.tomlAdd default-off cost output setting +1/-0

Add default-off cost output setting

• Introduces config.output_run_cost as a default-false switch for collecting and displaying estimated LiteLLM costs within enabled run details.

pr_agent/settings/configuration.toml

@qodo-code-review

qodo-code-review Bot commented Aug 26, 2026

Copy link
Copy Markdown
Contributor

Code Review by Qodo

🐞 Bugs (2) 📘 Rule violations (5) 📜 Skill insights (0)

Grey Divider


Action required

1. Cost estimate loses precision ✗ Dismissed 🐞 Bug ≡ Correctness
Description
_format_usd() now rounds every estimate to cents and labels every positive value below half a cent
as merely <$0.01, so distinct costs such as $0.0042 and $0.00001 lose the precision this
feature promises. This also contradicts the PR contract that tiny positive values render as
<$0.0001 rather than $0.0000.
Code

pr_agent/algo/utils.py[1492]

+    rounded = cost.quantize(Decimal("0.01"), rounding=ROUND_HALF_UP)
Relevance

●●● Strong

Recent accepted correctness precedents favor precision-preserving fixes for deterministic
presentation bugs.

PR-#2622
PR-#2772

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The formatter quantizes all costs to 0.01, while the focused tests prove that an exact accumulated
cost of 0.0042 is rendered as <$0.01 and that even 0.00001 uses the same coarse label. Since
aggregation retains Decimal values, this precision loss is introduced solely by the changed
presentation layer.

pr_agent/algo/utils.py[1490-1495]
tests/unittest/test_show_run_details.py[153-162]
tests/unittest/test_show_run_details.py[177-185]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
API cost rendering was changed to cent precision, causing small but valid costs to be reported only as `<$0.01` and violating the feature's stated four-decimal behavior.

## Issue Context
Preserve exact Decimal aggregation, but render normal values to four decimal places and use `<$0.0001` only when a positive value would otherwise round to `$0.0000`. Update the focused output expectations and documentation example consistently.

## Fix Focus Areas
- pr_agent/algo/utils.py[1490-1495]
- tests/unittest/test_show_run_details.py[139-150]
- tests/unittest/test_show_run_details.py[153-162]
- tests/unittest/test_show_run_details.py[177-186]
- docs/docs/usage-guide/additional_configurations.md[49-51]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools



Remediation recommended

2. Breakdown promise is too broad 🐞 Bug ≡ Correctness ⭐ New
Description
The documentation says every multi-model run shows a known-cost breakdown, but show_run_details()
emits breakdown rows only when more than one model has a known cost. A partial two-model run with
one priced and one unpriced call therefore shows no model breakdown, contrary to the documented
behavior and reducing auditability of the partial total.
Code

docs/docs/usage-guide/additional_configurations.md[56]

+`Estimated API cost` is derived synchronously from each completed LiteLLM response and its finalized usage. LiteLLM can account for cache reads, cache writes, reasoning tokens, and provider-specific usage categories when the response and its pricing data include them. Multi-model runs show a compact breakdown of the known costs. Exact `Decimal` values are retained for aggregation, while public currency output is rounded to two decimal places; a tiny positive value that would round to zero is shown as `<$0.01` instead of `$0.00`. If only some successful calls can be priced, the total is marked `partial` with the priced-call count; if none can be priced, the line reports `unavailable`. Missing pricing is never rendered as `$0`.
Relevance

●●● Strong

Recent documentation reviews accept corrections when wording overstates implemented behavior;
renderer evidence directly supports narrowing claim.

PR-#2547
PR-#2617

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The documentation makes the unconditional multi-model claim, while the renderer gates breakdown
output on more than one entry in model_costs_usd, and the collector adds entries only for calls
with a valid known cost.

docs/docs/usage-guide/additional_configurations.md[56-56]
pr_agent/algo/utils.py[1531-1534]
pr_agent/algo/run_details.py[154-159]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The documentation promises a cost breakdown for every multi-model run, while the renderer only includes one when costs are known for at least two models. Align the wording with the implemented behavior so partial multi-model runs with only one priced model are not documented as showing a breakdown.

## Issue Context
`show_run_details()` checks `len(details.model_costs_usd) > 1`; unpriced models are never inserted into `model_costs_usd`. Therefore a run involving two models but only one priceable call renders a partial aggregate without any per-model row.

## Fix Focus Areas
- docs/docs/usage-guide/additional_configurations.md[56-56]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


3. Cost documentation exceeds 120 characters 📘 Rule violation ⚙ Maintainability ⭐ New
Description
The newly modified Estimated API cost paragraph is a single physical line far longer than 120
characters. This violates the repository-wide maximum line-length requirement and makes the
documentation harder to review and maintain.
Code

docs/docs/usage-guide/additional_configurations.md[56]

+`Estimated API cost` is derived synchronously from each completed LiteLLM response and its finalized usage. LiteLLM can account for cache reads, cache writes, reasoning tokens, and provider-specific usage categories when the response and its pricing data include them. Multi-model runs show a compact breakdown of the known costs. Exact `Decimal` values are retained for aggregation, while public currency output is rounded to two decimal places; a tiny positive value that would round to zero is shown as `<$0.01` instead of `$0.00`. If only some successful calls can be priced, the total is marked `partial` with the priced-call count; if none can be priced, the line reports `unavailable`. Missing pricing is never rendered as `$0`.
Relevance

●● Moderate

Recent same-file line-wrapping precedent is mixed: one accepted today, one rejected today.

PR-#2797
PR-#2774

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
PR Compliance ID 2694690 requires every line in a modified, non-generated source file to be at most
120 characters. The added documentation at line 56 places the entire multi-sentence cost explanation
on one line, clearly exceeding that limit.

Rule 2694690: Enforce maximum line length of 120 characters
docs/docs/usage-guide/additional_configurations.md[56-56]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The `Estimated API cost` paragraph exceeds the 120-character maximum line length.

## Issue Context
Preserve the Markdown paragraph and its wording while inserting line breaks so every physical line is at most 120 characters.

## Fix Focus Areas
- docs/docs/usage-guide/additional_configurations.md[56-56]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


4. Free calls reported unavailable 🐞 Bug ≡ Correctness
Description
_as_decimal_cost rejects every zero value even though its own contract notes that LiteLLM returns
zero for explicitly zero-priced local/Ollama models. Those successfully priced calls are therefore
counted as unknown and rendered as “unavailable (no calls could be priced)” instead of a complete
known-zero estimate.
Code

pr_agent/algo/run_details.py[R141-142]

+    if not cost.is_finite() or cost <= 0:
+        return None
Relevance

●●● Strong

Concrete correctness bug affecting cost reporting for zero-priced models; team accepts fixing such
logic bugs.

PR-#2256

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The normalization docstring explicitly identifies zero-priced local/Ollama entries as one source of
0.0, but the changed condition rejects them all. The collector then defines zero known costs as
unavailable, and the renderer publicly claims no calls could be priced; Ollama is a supported model
in this repository.

pr_agent/algo/run_details.py[128-142]
pr_agent/algo/run_details.py[71-78]
pr_agent/algo/utils.py[1520-1525]
pr_agent/algo/init.py[298-298]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Known zero-priced model calls are currently discarded as unpriced, causing free local/Ollama runs to render an unavailable estimate.

## Issue Context
A zero returned for missing pricing must remain unknown, but a zero from a model with an explicit zero-price entry is a valid, complete cost. Preserve enough pricing-result state to distinguish these cases rather than classifying all zero values identically.

## Fix Focus Areas
- pr_agent/algo/run_details.py[127-158]
- pr_agent/algo/ai_handlers/litellm_ai_handler.py[384-406]
- pr_agent/algo/utils.py[1520-1534]
- tests/unittest/test_run_details.py[136-150]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


View medium (7)
5. Pricing comment is narrative ✓ Resolved 📘 Rule violation ⚙ Maintainability
Description
The new exception comment narrates that missing pricing is expected instead of giving an imperative
instruction. This violates the required comment phrasing convention.
Code

pr_agent/algo/ai_handlers/litellm_ai_handler.py[401]

+                    # Missing model pricing or insufficient usage is expected for some providers.
Relevance

●●● Strong

The requested rewrite is a trivial local style fix, matching the repository’s recent acceptance of
imperative comment wording.

PR-#2796

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
Rule 2694688 requires imperative behavioral comments, while the added sentence "Missing model
pricing or insufficient usage is expected" is narrative.

Rule 2694688: Docstrings and comments must use imperative phrasing
pr_agent/algo/ai_handlers/litellm_ai_handler.py[401-402]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The added pricing-failure comment uses descriptive phrasing rather than imperative phrasing.

## Issue Context
PR Compliance 2694688 requires newly added behavioral comments to be written as commands or instructions.

## Fix Focus Areas
- pr_agent/algo/ai_handlers/litellm_ai_handler.py[401-402]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


6. RunDetails comments are narrative ✓ Resolved 📘 Rule violation ⚙ Maintainability
Description
The added cost-field comments describe implementation behavior in passive and third-person prose.
They must be rewritten as imperative instructions.
Code

pr_agent/algo/run_details.py[R48-51]

+    # Costs are accumulated only when cost output is enabled and LiteLLM can
+    # synchronously price a successful response. The known-call count distinguishes
+    # a genuine zero cost from missing pricing data, while the per-model totals keep
+    # fallback and multi-call runs auditable.
Relevance

●●● Strong

Recent precedent explicitly accepted rewriting narrative configuration comments into imperative
phrasing.

PR-#2796

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
Rule 2694688 requires imperative behavioral comments. The added block starts with passive wording,
Costs are accumulated, and continues with descriptive statements about what the counters do.

Rule 2694688: Docstrings and comments must use imperative phrasing
pr_agent/algo/run_details.py[48-51]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The comments introducing the cost aggregation fields use narrative phrasing instead of imperative phrasing.

## Issue Context
PR Compliance 2694688 requires newly added behavioral comments to use command-style wording.

## Fix Focus Areas
- pr_agent/algo/run_details.py[48-51]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


7. completion_cost comment is narrative ✓ Resolved 📘 Rule violation ⚙ Maintainability
Description
The new comment describes what completion_cost does instead of using imperative phrasing. Rewrite
it as an instruction to satisfy the comment-style requirement.
Code

pr_agent/algo/ai_handlers/litellm_ai_handler.py[R393-394]

+                    # completion_cost consumes LiteLLM's full usage object, including cache,
+                    # reasoning, and provider-specific categories. For the small completed
Relevance

●●● Strong

Recent repository precedent accepts imperative rewrites for newly added configuration comments and
similar maintainability wording.

PR-#2796

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
Rule 2694688 requires behavioral comments to use imperative phrasing. The added comment says that
completion_cost "consumes" usage data, which is descriptive third-person wording.

Rule 2694688: Docstrings and comments must use imperative phrasing
pr_agent/algo/ai_handlers/litellm_ai_handler.py[393-395]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The added `completion_cost` comment uses narrative phrasing rather than imperative phrasing.

## Issue Context
PR Compliance 2694688 requires newly added behavioral comments to be phrased as instructions.

## Fix Focus Areas
- pr_agent/algo/ai_handlers/litellm_ai_handler.py[393-395]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


8. output_run_cost missing root sync 📘 Rule violation ≡ Correctness
Description
The new behavior flag is defined only in pr_agent/settings/configuration.toml; the root
.pr_agent.toml has no matching key. This leaves the two required configuration locations out of
sync.
Code

pr_agent/settings/configuration.toml[50]

+output_run_cost=false # if true, collect estimated LiteLLM API cost and include it inside the enabled run details section
Relevance

●●● Strong

The finding identifies a concrete missing configuration default in a required location, and recent
configuration additions were accepted.

PR-#2528

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
Rule 2694685 requires matching configuration keys and defaults in root .pr_agent.toml and the
corresponding pr_agent/settings TOML. The PR adds output_run_cost=false at configuration line
50, while the root file contains no output_run_cost key.

Rule 2694685: Keep .pr_agent.toml and pr_agent/settings/*.toml configuration in sync on behavior changes
pr_agent/settings/configuration.toml[50-50]
.pr_agent.toml[1-27]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The new `output_run_cost` behavior flag exists in the settings configuration but not in the root `.pr_agent.toml`.

## Issue Context
PR Compliance 2694685 requires behavior-changing configuration keys and defaults to remain synchronized across both locations.

## Fix Focus Areas
- pr_agent/settings/configuration.toml[50-50]
- .pr_agent.toml[1-27]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


9. MockResponse docstring lacks imperative ✓ Resolved 📘 Rule violation ⚙ Maintainability
Description
The new class docstring is a descriptive noun phrase rather than an imperative statement. Rewrite it
with an imperative verb such as Represent.
Code

pr_agent/algo/ai_handlers/litellm_helpers.py[104]

+    """Completed streaming response that retains LiteLLM's finalized usage object."""
Relevance

●●● Strong

This is a deterministic docstring-style correction; recent repository behavior accepts imperative
wording changes for new documentation text.

PR-#2796

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
Rule 2694688 requires imperative docstrings. Completed streaming response that retains... is a
descriptive fragment and does not begin with an imperative verb.

Rule 2694688: Docstrings and comments must use imperative phrasing
pr_agent/algo/ai_handlers/litellm_helpers.py[104-104]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The `MockResponse` class docstring uses descriptive phrasing instead of imperative phrasing.

## Issue Context
PR Compliance 2694688 applies to newly added or modified docstrings.

## Fix Focus Areas
- pr_agent/algo/ai_handlers/litellm_helpers.py[104-104]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


10. Test docstring remains narrative 📘 Rule violation ⚙ Maintainability
Description
The modified docstring says that recording zero cost “would render” false output instead of
instructing the reader with imperative phrasing. This violates the required comment and docstring
style.
Code

tests/unittest/test_run_details.py[138]

+    recording it would render a false '$0.00' with cost status complete."""
Relevance

●● Moderate

Imperative-style findings are accepted, but same-day docstring-style suggestions were also
explicitly rejected.

PR-#2807
PR-#2774

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
Rule 2694688 requires imperative phrasing in modified docstrings. The test docstring at lines
137-138 narrates LiteLLM behavior and says recording it would render rather than beginning with an
imperative instruction.

Rule 2694688: Docstrings and comments must use imperative phrasing
tests/unittest/test_run_details.py[136-138]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Rewrite the modified test docstring so its behavior description uses imperative phrasing rather than narrative phrasing such as `would render`.

## Issue Context
PR Compliance ID 2694688 requires newly added or modified docstrings and behavior-describing comments to use imperative phrasing.

## Fix Focus Areas
- tests/unittest/test_run_details.py[136-138]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


11. Breakdown mislabels routed models ✓ Resolved 🐞 Bug ≡ Correctness
Description
chat_completion() passes the internally rewritten LiteLLM routing name into cost aggregation, so
multi-model Azure or GPT-5 runs can display labels such as azure/gpt-5 or stripped _thinking
names instead of the configured model names. This makes the cost breakdown inconsistent with the
run's configured/fallback model identity and the documented public-output contract.
Code

pr_agent/algo/ai_handlers/litellm_ai_handler.py[1004]

+            self._record_completion_metadata(response_obj, model=model)
Relevance

●● Moderate

The concern is a plausible routed-versus-configured identity bug, but available provider-prefix
precedent rejected a related semantic change.

PR-#2401

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The handler saves the configured name as user_model, then may prepend azure/ or rebuild GPT-5
names before passing the mutated model at the added metadata call. In contrast, the existing
run-details model identity is recorded from the original fallback list, and the new documentation
explicitly says public output contains configured model names.

pr_agent/algo/ai_handlers/litellm_ai_handler.py[661-676]
pr_agent/algo/ai_handlers/litellm_ai_handler.py[725-736]
pr_agent/algo/pr_processing.py[334-355]
docs/docs/usage-guide/additional_configurations.md[56-58]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Per-model costs are keyed by LiteLLM's rewritten routing model, causing the public breakdown to mislabel configured models.

## Issue Context
Keep using the rewritten model for `litellm.completion_cost`, but pass the original configured `user_model` (or otherwise preserve a separate display model) to `record_ai_call` for aggregation.

## Fix Focus Areas
- pr_agent/algo/ai_handlers/litellm_ai_handler.py[381-405]
- pr_agent/algo/ai_handlers/litellm_ai_handler.py[661-676]
- pr_agent/algo/ai_handlers/litellm_ai_handler.py[1004-1004]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools



Informational

12. OpenAI comment remains narrative 📘 Rule violation ⚙ Maintainability
Description
The new comment mixes an imperative opening with descriptive statements such as `this handler has no
pricing source and its calls render as unpriced`. Behavior-describing comments must be phrased as
commands or instructions.
Code

pr_agent/algo/ai_handlers/openai_ai_handler.py[R64-67]

+            # Count the call and its tokens but no cost: this handler has no pricing
+            # source wired up, so with output_run_cost enabled its calls render as
+            # unpriced. Left as-is while the path stays cold — no setting selects
+            # this handler, it is reachable only by injecting it programmatically.
Relevance

● Weak

Recent precedent (PR #2774) rejected the identical request to rewrite a comment in imperative mood.

PR-#2774

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
Checklist 2694688 requires behavior comments to use imperative phrasing. The added comment narrates
the handler's pricing state and reachability instead of consistently instructing the reader.

Rule 2694688: Docstrings and comments must use imperative phrasing
pr_agent/algo/ai_handlers/openai_ai_handler.py[64-67]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Rewrite the OpenAI cost-collection comment so every behavior-describing sentence uses imperative phrasing rather than narrative or third-person prose.

## Issue Context
Checklist 2694688 requires newly added comments that describe behavior to be commands or instructions.

## Fix Focus Areas
- pr_agent/algo/ai_handlers/openai_ai_handler.py[64-67]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


13. Pricing comment uses narrative 📘 Rule violation ⚙ Maintainability
Description
The new unavailable-cost comment explains that pricing can be missing and that a handler `never
collects costs` in narrative prose. This violates the required imperative style for
behavior-describing comments.
Code

pr_agent/algo/utils.py[R1522-1524]

+            # No causal claim here: pricing can be missing because usage was absent
+            # (streaming without a final usage chunk) or because the active handler
+            # never collects costs (openai/langchain handlers).
Relevance

● Weak

Recent precedent (PR #2774) rejected the identical request to rewrite a behavior comment in
imperative mood.

PR-#2774

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
Checklist 2694688 requires imperative behavior comments. The cited added lines use descriptive
constructions—pricing can be missing and the active handler never collects costs—rather than
commands.

Rule 2694688: Docstrings and comments must use imperative phrasing
pr_agent/algo/utils.py[1522-1524]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Rewrite the unavailable-cost comment as an imperative instruction while preserving its explanation of missing pricing data.

## Issue Context
Checklist 2694688 requires newly added comments that describe behavior to use command or instructional phrasing.

## Fix Focus Areas
- pr_agent/algo/utils.py[1522-1524]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


  • Author self-review: I have reviewed the code review findings, and addressed the relevant ones.

Grey Divider

Context sources
✅ Compliance rules (platform): 34 rules
Review mode: ⚖️ Balanced

Grey Divider

Tip of the day
💡 Did you know, you can ask Qodo to dismiss a finding you disagree with, with your reason on record

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Grey Divider

Qodo Logo

@IsmaelMartinez IsmaelMartinez left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is good work. Making "we couldn't price this call" and "this call cost zero" two different things is exactly right.

One thing to fix before it goes in. Qodo found it; I've confirmed it and left the change as three suggestions.

You can ignore Qodo's note about the root .pr_agent.toml needing a matching key. That file isn't a copy of configuration.toml: it has no [config] section at all, and it has a [review_agent] section this package doesn't even have. Your setting is in the right place.

Comment thread pr_agent/algo/ai_handlers/litellm_ai_handler.py Outdated
Comment thread pr_agent/algo/ai_handlers/litellm_ai_handler.py Outdated
Comment thread pr_agent/algo/ai_handlers/litellm_ai_handler.py Outdated
@elijahchancey

Copy link
Copy Markdown
Author

Follow-up to the Qodo review in commit b3ad13d:

  • Findings 1-4 — addressed. The new completion-cost comments, pricing-failure comments, MockResponse docstring, and RunDetails cost comments now use imperative phrasing.
  • Finding 5 — intentionally not changed. The root .pr_agent.toml states that it configures the separate hosted Qodo free-for-open-source service and is not the open-source PR-Agent runtime default source. The default-false runtime setting therefore remains in pr_agent/settings/configuration.toml, where PR-Agent loads global defaults.
  • Finding 6 — addressed. Cost calculation continues to receive the rewritten LiteLLM routing model, while aggregation and public output now receive the original configured or fallback model name. This keeps Azure and GPT-5 routing correct without mislabeling the public per-model breakdown.

Verification:

  • 93 focused run-details and LiteLLM tests passed.
  • Added deterministic coverage proving that pricing receives azure/gpt-5 while aggregation records gpt-5_thinking, plus chat-completion wiring coverage for Azure model rewriting.
  • Targeted Flake8 error checks, pre-commit hooks, and git diff --check passed.

@qodo-code-review

Copy link
Copy Markdown
Contributor

Code review by qodo was updated up to the latest commit b3ad13d

elijahchancey and others added 8 commits August 26, 2026 14:38
litellm.completion_cost returns 0.0 rather than raising both for
zero-priced model_cost entries (local/ollama models) and for usage
without billable tokens. _as_decimal_cost accepted any non-negative
value, so those calls were counted as priced and the run rendered
'Estimated API cost: $0.0000 USD' with cost status complete — the
exact false zero the cost feature promises never to show. Reject
cost <= 0 so such calls fall to partial/unavailable, matching the
existing > 0 rule for inline response costs.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SfjC52hJptWtP5QXRJwVKT
_has_priceable_usage walked the whole usage object recursively and
accepted any positive number as a billable quantity, so provider
extras like Groq's queue_time/prompt_time floats passed the gate
even with zero prompt and completion tokens, sending unpriceable
usage to litellm.completion_cost (which returns 0.0 for it instead
of raising). Check the token counters directly instead.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SfjC52hJptWtP5QXRJwVKT
The PR introduced _response_field in litellm_helpers yet inlined
three more copies of the same dict-vs-attribute accessor, and
duplicated run_details' Decimal validation in
_read_positive_response_cost with only a >= vs > difference — the
drift behind the false-$0 bug fixed earlier. Use the shared helper
and run_details._as_decimal_cost (which now enforces > 0) so there
is one implementation of each rule.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SfjC52hJptWtP5QXRJwVKT
The try/except wrapped only litellm.completion_cost, leaving the
model_dump() probes in _read_positive_response_cost and
_has_priceable_usage unguarded — an exotic response object could
raise out of _record_completion_metadata and discard a successful,
already-billed completion. Widen the guard to the whole cost block.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SfjC52hJptWtP5QXRJwVKT
kwargs is built entirely inside chat_completion and the extra-body
allowlist admits only processing_mode/service_tier, so
kwargs.get('stream_options') is always None here; assign the dict
directly instead of merging with a value that cannot exist.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SfjC52hJptWtP5QXRJwVKT
The two force-streaming tests still stubbed
_handle_streaming_response with the removed 2-tuple shape, so the
new (content, finish_reason, completed_response) contract had no
coverage on that path. Return the 3-tuple, assert it is passed
through, and pin the stream_options and model= kwargs the
streaming branch now sends.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SfjC52hJptWtP5QXRJwVKT
'unavailable (priceable usage was not available)' asserted a cause
that is often wrong: the openai and langchain handlers record calls
(with usage) but have no pricing wired up, and their runs would
claim usage was missing. Say 'no calls could be priced' instead,
and document at the OpenAIHandler call site why it records no cost,
mirroring the langchain handler's inline note.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SfjC52hJptWtP5QXRJwVKT
In IMDS mode the Bedrock lock serializes every concurrent call to
protect the os.environ credential swap. Debug logging, prepare_logs,
and the new synchronous cost pricing in _record_completion_metadata
need none of that state, yet ran while holding the lock, so every
waiting coroutine paid for them serially. Move the post-response
bookkeeping after the lock releases; the variables it reads stay in
scope past the with block.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SfjC52hJptWtP5QXRJwVKT
@qodo-code-review

Copy link
Copy Markdown
Contributor

Code review by qodo was updated up to the latest commit 5f3db32

Comment thread pr_agent/algo/utils.py
@qodo-code-review

Copy link
Copy Markdown
Contributor

Code review by qodo was updated up to the latest commit dac9292

@qodo-code-review

Copy link
Copy Markdown
Contributor

Code review by qodo was updated up to the latest commit 1d78c26

@elijahchancey

Copy link
Copy Markdown
Author

@IsmaelMartinez this works in my env. Need any additional changes before you merge it? Thanks!

@IsmaelMartinez IsmaelMartinez left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nothing further needed, and display_model is a better name than the one I suggested.

I checked the split does what we wanted: completion_cost still prices on the routed name, and only the label takes the configured one. Suite is green on your head merged onto today's main.

Good to merge from my side.

@IsmaelMartinez
IsmaelMartinez merged commit 9e6d6a5 into The-PR-Agent:main Aug 27, 2026
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants