Skip to content

Feat/transient errors - #31

Open
LeFi8 wants to merge 6 commits into
mainfrom
feat/transient-errors
Open

Feat/transient errors#31
LeFi8 wants to merge 6 commits into
mainfrom
feat/transient-errors

Conversation

@LeFi8

@LeFi8 LeFi8 commented Jun 29, 2026

Copy link
Copy Markdown
Owner

No description provided.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Code Review

This pull request introduces a robust fallback mechanism for LLM response formats, adds token usage and cost tracking to LLM errors, and integrates the tenacity library for transient error retries. However, several critical and high-severity issues were identified in the review. Most notably, the enable_json_salvage setting is used but not defined in the Settings class, which will cause a runtime AttributeError. Additionally, the regex for salvaging JSON is too aggressive and may corrupt string literals, and setting num_retries = 0 for structured formats prevents retrying transient errors. Finally, litellm.get_max_tokens() may return the total context window instead of the max output tokens, and the new JSON salvage environment variable should be documented in .env.example.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment on lines +130 to +131
llm_num_retries: int = _env_int("AUTOBACKTEST_LLM_NUM_RETRIES", "2")
response_format_override: str | None = _env_str("AUTOBACKTEST_RESPONSE_FORMAT_OVERRIDE", "auto")

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

critical

The setting enable_json_salvage is used in src/autobacktest/llm/litellm_provider.py (line 306) but is not defined in the Settings class. This will cause an AttributeError at runtime when generate_edit is called.

The unit tests did not catch this because settings was mocked as a MagicMock in test_llm_litellm.py, which dynamically allows any attribute access.

Please add enable_json_salvage to the Settings class.

Suggested change
llm_num_retries: int = _env_int("AUTOBACKTEST_LLM_NUM_RETRIES", "2")
response_format_override: str | None = _env_str("AUTOBACKTEST_RESPONSE_FORMAT_OVERRIDE", "auto")
llm_num_retries: int = _env_int("AUTOBACKTEST_LLM_NUM_RETRIES", "2")
response_format_override: str | None = _env_str("AUTOBACKTEST_RESPONSE_FORMAT_OVERRIDE", "auto")
enable_json_salvage: bool = _env_bool("AUTOBACKTEST_ENABLE_JSON_SALVAGE", "True")

Comment on lines +102 to +104
def _salvage_json(content: str) -> str:
"""Apply conservative repairs to common model JSON formatting mistakes."""
return re.sub(r",\s*([}\]])", r"\1", content)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

high

The regex r",\s*([}\]])" used in _salvage_json is too aggressive because it matches trailing commas inside string literals (such as Python code, markdown, or regex character classes).

For example, if the JSON contains a regex pattern or character class like "pattern": "[a-z, ]", the regex will match , ] and replace it with ], corrupting the pattern to "[a-z]".

To safely salvage JSON, you should avoid modifying characters inside string literals, or use a more robust JSON parser/repair library (like json_repair), or at least restrict the regex to not match inside double quotes.

Comment on lines +275 to +278
# Skip retries for response_format attempts to avoid wasting API calls
# on formats that the provider doesn't support. Only the final None
# format attempt uses retries for transient errors.
num_retries = settings.llm_num_retries if resp_format is None else 0

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

high

Setting num_retries = 0 for structured formats (AgentEditResponse and json_object) means that any transient error (such as a 503 Service Unavailable or 429 Rate Limit) during these attempts will immediately fail the candidate generation without any retries.

Since the orchestrator (generate_candidates) does not retry transient errors (it just returns None for that slot), the configured settings.llm_num_retries is completely ignored for structured format attempts.

To fix this, you should allow retries for transient errors even when using structured formats. You only want to skip retries/fallback if the error is specifically a 400 Bad Request due to unsupported response_format (which is not a transient error anyway).

Comment on lines +52 to +54
Note: ``litellm.get_max_tokens()`` returns the model's max OUTPUT tokens,
not the total context window. Prompt tokens consume input, not output budget,
so they are not subtracted here.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

The comment states that litellm.get_max_tokens() returns the model's max output tokens, but it actually returns the model's maximum context window (input + output tokens), which is 128,000 for models like gpt-4o.

If env_limit or instance_max_tokens is set to a high value, _compute_run_max_tokens could return a value (e.g., 64,000) that exceeds the provider's maximum output token limit (typically 4,096 or 16,384), leading to a 400 Bad Request from the API.

Consider capping the returned value by a sensible maximum output token limit or using litellm.model_cost to lookup the actual max_output_tokens if available.

Comment thread .env.example
Comment on lines +21 to +22
# Number of retries for transient LLM errors (requires tenacity)
AUTOBACKTEST_LLM_NUM_RETRIES=2

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

Document the new AUTOBACKTEST_ENABLE_JSON_SALVAGE environment variable in .env.example so that users are aware of this configuration option.

# Number of retries for transient LLM errors (requires tenacity)
AUTOBACKTEST_LLM_NUM_RETRIES=2

# Enable conservative repairs to common model JSON formatting mistakes
AUTOBACKTEST_ENABLE_JSON_SALVAGE=True

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant