feat(pr-review): add llm-extra-body input for provider-specific request fields - #544
arpit-acceleratedata wants to merge 3 commits into
Conversation
…st fields Some models think by default (for example Qwen3.8 on Model Studio) and only expose the switch as a request-body field such as enable_thinking or thinking_budget. The review agent had no way to send it, so every call paid full reasoning time and long reviews hit the job timeout. The new optional input is a JSON object forwarded to the SDK's existing litellm_extra_body. Invalid JSON or a non-object fails fast with a clear error. Ignored in ACP mode, like llm-base-url.
…nt checks Fail fast in validate_environment(), before any GitHub call, instead of at LLM construction.
Maps to the SDK's max_iteration_per_run. A model that explores without a plan can burn the whole workflow timeout and post nothing; a cap ends the run early with a clear error.
|
👋 This PR needs a couple of things fixed before OpenHands can review it:
Push an update once this is addressed and this check re-runs automatically. This is an automated check - no AI was used to generate this comment. |
all-hands-bot
left a comment
There was a problem hiding this comment.
This review was posted by an AI agent (OpenHands).
Review of PR #544
Scope: action.yml, scripts/agent_script.py, README.md (+52/-0). The feature itself is reasonable and needed: litellm_extra_body is a real SDK field (openhands/sdk/llm/llm.py) and is the correct channel for enable_thinking / thinking_budget on Qwen-style endpoints, and max_iteration_per_run is a real Conversation parameter (default 500). JSON validation and the int / >= 1 checks on MAX_ITERATIONS are sound, and the ACP path is correctly unaffected because it returns before the OpenHands-only config is built.
Blocking
1. create_conversation now requires two new keys in every config dict and breaks a caller in this repo. The new reads use direct subscripting (config["extra_body"], config["max_iterations"]) instead of the optional-key pattern already used a few lines above (config.get("use_sub_agents", False)). validate_environment() always emits both keys, but create_conversation() is also called directly by tests with hand-built config dicts. The repository's own test tests/test_pr_review_review_context.py::test_create_conversation_uses_sdk_project_skill_loader (lines 243-249) passes a config without either key and now dies with KeyError: 'extra_body':
> if config["extra_body"]:
E KeyError: 'extra_body'
plugins/pr-review/scripts/agent_script.py:1107
I ran the CI command (uv run --group test pytest tests/) on this head: 1 failed, 799 passed, 24 skipped. The same test passes on origin/main, so this is a regression introduced by the PR, not pre-existing. Fix by using config.get("extra_body") / config.get("max_iterations") (consistent with use_sub_agents), or by updating the caller's dict.
Non-blocking, but the merge gate is red
2. The Validate PR description check fails on this head. Run 34249346569 reports the HUMAN: / "How to Test" section is the untouched template placeholder (the existing bot comment 5603558088 says the same). Per the repo's own gate (.github/workflows/pr-description-check.yml, .github/scripts/check_pr_description.py, REQUIRED_TEMPLATE_FIELDS), non-draft PRs must keep ## Why, ## Summary, and ## How to Test filled in. The PR body has ## Summary and ## Why but no ## How to Test, so this cannot be attached to a changed line and is reported here. The Tests, Check Extensions, and Deprecation deadlines runs are action_required (fork PR awaiting approval), so they have not yet exercised the failure in finding 1 on CI.
Notes (no action required)
- The
llm-extra-bodyvalue flows from the workflow definition, not from PR content, so a fork PR cannot inject arbitrary request-body fields through it inpull_requestcontext. Using the SDK's structuredlitellm_extra_bodyfield (withdrop_params: True) rather than a hand-rolled passthrough is the right call. - README and
action.ymldescriptions match the implemented behavior, including the "ignored in ACP mode" caveat.
Please fix finding 1 (and fill in ## How to Test). Happy to re-review after a push.
🔄 CHANGES REQUESTED
| } | ||
| if config["base_url"]: | ||
| llm_config["base_url"] = config["base_url"] | ||
| if config["extra_body"]: |
There was a problem hiding this comment.
Direct subscripting makes these two new keys mandatory for every caller of create_conversation(), not just validate_environment(). create_conversation() is also called with hand-built config dicts; tests/test_pr_review_review_context.py::test_create_conversation_uses_sdk_project_skill_loader (lines 243-249) omits both keys and now raises KeyError: 'extra_body'. Running the CI command uv run --group test pytest tests/ on this head gives 1 failed, 799 passed, 24 skipped; the same test passes on origin/main, so this is a regression from this PR.
Use the optional-key pattern already used a few lines above (config.get("use_sub_agents", False)):
if config.get("extra_body"):
llm_config["litellm_extra_body"] = config["extra_body"]The same applies to config["max_iterations"] at line 1147.
Summary
Adds an optional
llm-extra-bodyinput to thepr-reviewaction. It is a JSON object forwarded to the SDKLLMaslitellm_extra_body.Why
Some models think by default and expose the switch only as a request-body field. Qwen3.8 on Alibaba Model Studio is one: thinking is on by default and is controlled by
enable_thinkingorthinking_budget(docs). The action only passes model, key, and base URL, so there was no way to send it. On our repo a review withopenai/qwen3.8-flashmade ~28 model calls with full reasoning and hit the 20 minute job timeout without posting.With
llm-extra-body: '{"enable_thinking": false}'the same call drops from ~280 s to ~12 s in a direct API test.Changes
action.yml: new optional inputllm-extra-body(default''), passed asLLM_EXTRA_BODYto the run step.scripts/agent_script.py: parseLLM_EXTRA_BODYas JSON; must be an object; setlitellm_extra_body. Invalid input exits with a clear error.README.md: input table row.Notes
llm-base-url.