fix(ask-line): enforce conversation history token budget

[junnhwan]

Summary

• enforce the configured prompt token budget for /ask_line after loading review-thread history
• preserve the most recent thread context while retaining the hunk and question
• use the tokenizer for the model actually attempted, including fallback models

Fixes #2861

Root cause

PR_LineQuestions loads GitHub review-thread history in run(), but _get_prediction() sent the rendered prompt without a final token-budget check. The TokenHandler was initialized before that history was loaded, so it could not protect the final request. Fallback attempts could also use the primary model's cached tokenizer.

Changes

• render and count the complete system and user prompts before each model attempt
• binary-search a suffix of the conversation history and add a truncation marker when the history is too large
• fail before the API call when the hunk and question alone exceed the selected model's limit
• allow TokenEncoder callers to request a fallback-model encoder without replacing the primary cached encoder

Tests

• pytest -q -p no:cacheprovider tests/unittest
• git diff origin/main...HEAD --check

[qodo-code-review]

PR Summary by Qodo

Enforce per-model token budgets for /ask_line history

🐞 Bug fix 🧪 Tests 🕐 20-40 Minutes

AI Description

• Enforces /ask_line token limits after adding review-thread history.
• Preserves newest history while retaining the question and selected diff hunk.
• Counts each retry with its attempted model tokenizer and adds regression coverage.

Diagram

graph TD
  A["Review thread"] --> B["Render prompts"] --> C{"Within budget?"}
  C -- Yes --> F["Model request"]
  C -- No --> D{"Base prompt fits?"}
  D -- Yes --> E["Trim oldest history"] --> B
  D -- No --> G["Fail before call"]

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Trim complete review turns

• ➕ Preserves message boundaries and conversational meaning
• ➕ Avoids beginning retained context mid-comment
• ➖ May discard substantially more usable context
• ➖ Requires retaining structured thread data through prompt construction

2. Subtract fixed prompt overhead

• ➕ Could tokenize history once and avoid repeated template rendering
• ➕ May simplify truncation to token slicing
• ➖ Template conditionals and formatting can make overhead assumptions fragile
• ➖ Does not directly verify the final rendered request

Recommendation: Keep the PR's final-render measurement and per-attempt tokenizer selection because they verify the exact request sent to each model. Whole-turn truncation is a reasonable future refinement if preserving message boundaries becomes more important than maximizing recent context.

Files changed (3) +208 / -12

Bug fix (2) +72 / -12

token_handler.pySupport model-specific tokenizer lookup without cache replacement +18/-7

Support model-specific tokenizer lookup without cache replacement

• Allows callers to request an encoder for the model currently being attempted. Explicit fallback-model lookups create an appropriate encoder without replacing the cached primary-model encoder.

pr_agent/algo/token_handler.py

pr_line_questions.pyFit complete /ask_line prompts to each model budget +54/-5

Fit complete /ask_line prompts to each model budget

• Renders and counts the final system and user prompts for every model attempt. It preserves the newest conversation-history suffix with a truncation marker, or fails before the API call when the question and hunk alone exceed the limit.

pr_agent/tools/pr_line_questions.py

Tests (1) +136 / -0

test_pr_line_questions_context_budget.pyCover primary and fallback /ask_line context budgeting +136/-0

Cover primary and fallback /ask_line context budgeting

• Adds regression tests with oversized review-thread history for primary and fallback model paths. The tests verify the final prompt remains within budget while preserving the question, selected hunk, latest reply, and normal response publication.

tests/unittest/test_pr_line_questions_context_budget.py

[qodo-code-review]

Code Review by Qodo

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

1. No completion budget reserved ✓ Resolved 🐞 Bug ≡ Correctness

Description

_fit_conversation_history allows the rendered input prompt to consume all of
get_max_tokens(model), leaving no context capacity for the answer. Requests near that threshold
can exceed the model context once completion tokens are added and fail instead of answering
/ask_line.

Code

pr_agent/tools/pr_line_questions.py[226]

+        if count_prompts(render_with_history(conversation_history)) <= max_tokens:

Relevance

●●● Strong

The finding identifies a direct context-budget correctness risk; repository budgeting fixes and
safeguards are consistently accepted.

PR-#2256
PR-#2744

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The changed comparisons use the full model limit for input alone, while established repository logic
subtracts a 1,000–1,500 token output buffer and the request handler can additionally set a
configured completion allowance.

pr_agent/tools/pr_line_questions.py[221-244]
pr_agent/algo/pr_processing.py[28-29]
pr_agent/algo/pr_processing.py[305-312]
pr_agent/algo/ai_handlers/litellm_ai_handler.py[789-798]

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 final `/ask_line` prompt is fitted against the complete model context limit, so no capacity is reserved for the model response.

## Issue Context
The repository's existing prompt-building paths reserve output tokens before accepting input, and the AI handler may explicitly request `config.max_output_tokens`; otherwise the provider still needs output capacity. Compute an effective input budget by subtracting an appropriate completion allowance, use it consistently for the full-history check, empty-history failure check, and suffix search, and add boundary tests proving prompt plus reserved output fits.

## Fix Focus Areas
- pr_agent/tools/pr_line_questions.py[211-248]
- pr_agent/algo/pr_processing.py[28-29]
- pr_agent/algo/ai_handlers/litellm_ai_handler.py[789-798]
- tests/unittest/test_pr_line_questions_context_budget.py[38-134]

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

---

2. Non-GPT budgets miscounted ✓ Resolved 🐞 Bug ≡ Correctness

Description

_fit_conversation_history directly uses TokenEncoder for the attempted model, but that encoder
selects OpenAI's o200k_base for every non-GPT model and bypasses the repository's model-specific
estimation path. Claude, Gemini, and other supported fallback attempts are therefore fitted using
the wrong tokenization, so the final prompt can be over-truncated or still exceed the selected
model's limit.

Code

pr_agent/tools/pr_line_questions.py[R213-214]

+        encoder = TokenEncoder.get_token_encoder(model)
+        max_tokens = get_max_tokens(model)

Relevance

●● Moderate

Semantic tokenizer correctness concern lacks a close direct precedent, though model-specific token
fallback issues are accepted.

PR-#2256
PR-#2486

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The new fitter calls the raw encoder, whose implementation only selects a model tokenizer when the
name contains gpt; all other model families use o200k_base. The existing TokenHandler has
separate model-type handling and an estimation factor, but this new path bypasses it, and the added
regression test covers only GPT models.

pr_agent/tools/pr_line_questions.py[211-224]
pr_agent/algo/token_handler.py[45-50]
pr_agent/algo/token_handler.py[155-177]
pr_agent/algo/init.py[70-104]
tests/unittest/test_pr_line_questions_context_budget.py[38-43]

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 budget fitter counts all non-GPT fallback prompts with the OpenAI `o200k_base` encoder rather than a counter appropriate for the attempted model.

## Issue Context
Route prompt counting through a model-aware abstraction that accepts the attempted model and applies the repository's accurate or conservative estimation behavior for unsupported local tokenizers. Keep fallback attempts independent of the configured primary model, and add non-GPT fallback coverage that verifies the selected model's counting path is used.

## Fix Focus Areas
- pr_agent/tools/pr_line_questions.py[211-224]
- pr_agent/algo/token_handler.py[45-50]
- pr_agent/algo/token_handler.py[155-177]
- tests/unittest/test_pr_line_questions_context_budget.py[38-134]

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

---

3. TokenEncoder comment is narrative ✓ Resolved 📘 Rule violation ⚙ Maintainability

Description

The added cache explanation uses descriptive phrasing (A fallback attempt may...) rather than an
imperative instruction. Rewrite it in imperative mood to comply with the project's comment
convention.

Code

pr_agent/algo/token_handler.py[R32-34]

+        # A fallback attempt may use a different tokenizer from the configured
+        # primary model. Keep the existing cache for the common path, but
+        # do not replace it when a caller explicitly asks for another model.

Relevance

●●● Strong

Recent repository precedent accepts imperative rewrites for newly added narrative comments.

PR-#2817
PR-#2661

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

Compliance rule 2694688 requires newly added behavioral comments to use command-style imperative
phrasing. Lines 32-34 instead narrate what a fallback attempt may do and describe the cache
behavior.

Rule 2694688: Docstrings and comments must use imperative phrasing
pr_agent/algo/token_handler.py[32-34]

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 newly added fallback-tokenizer comment is narrative rather than imperative.

## Issue Context
Preserve the explanation, but begin with an imperative verb such as `Use` or `Keep`.

## Fix Focus Areas
- pr_agent/algo/token_handler.py[32-34]

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

---

4. Bare except violates linting ✓ Resolved 📘 Rule violation ✧ Quality

Description

The new _create_encoder method catches every exception with a bare except, which triggers the
configured Python lint rule E722. Catch Exception explicitly while preserving the fallback
encoder behavior.

Code

pr_agent/algo/token_handler.py[R49-50]

+        except:
+            return get_encoding("o200k_base")

Relevance

●●● Strong

Repository precedent accepts replacing bare except handlers to satisfy lint and improve explicit
exception handling.

PR-#2097
PR-#2307

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

Compliance rule 2694666 requires changed Python code to pass configured lint checks with zero
errors. The added bare except at line 49 is an E722 violation, and the repository Ruff
configuration enables the E rule family.

Rule 2694666: Python code must pass flake8 in CI with zero errors or warnings
pr_agent/algo/token_handler.py[46-50]
pyproject.toml[52-61]

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 `_create_encoder` method uses a bare `except`, which violates Python lint rule `E722`.

## Issue Context
Keep the existing fallback to `o200k_base`, but catch `Exception` explicitly so process-control exceptions are not swallowed and linting passes.

## Fix Focus Areas
- pr_agent/algo/token_handler.py[46-50]

ⓘ 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.

Context sources

✅ Compliance rules (platform): 34 rules

Review mode: ⚖️ Balanced: This push changes runtime prompt rendering and token-budget enforcement across primary and fallback model paths, creating real correctness risk, but remains a focused concern that does not warrant redundant extended passes.

Tip of the day

💡 Did you know, you can reply 'qodo' on any finding to push back, ask questions, or dig deeper

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

[qodo-code-review]

Code review by qodo was updated up to the latest commit 6abfafe

[IsmaelMartinez]

Merging, thanks. Qodo's pass did real work here, the completion reserve and the non-GPT counting both came from it, and all four findings are resolved.

Coverage checked the hard way: reverting both source files with your tests kept turns three of the four red. I also confirmed the two quiet risks are not risks. select_autoescape(default_for_string=False) renders byte-identically to the old environment, and bypassing the class-level encoder cache for a fallback model costs nothing, because tiktoken caches internally.