fix(pr_agent/agent/pr_agent.py): validating auto command arguments

[dwin-gharibi]

Closes #2910.

Description

PRAgent._handle_request runs CliArgs.validate_user_args before applying settings. prepare_command, used by the webhook adapters for auto commands, calls update_settings_from_args with no validation at all.

Root cause

Scoping, stated honestly: this is hardening, not a live exploit. Auto commands come from settings, and
repo settings are read from .pr_agent.toml on the default branch of the base repo
(get_repo_settings() calls get_contents(".pr_agent.toml") with no ref), so a fork PR author cannot inject
them. The reachable actor is a maintainer, who already controls the config. What is objectively wrong is that the
same denylist is enforced on one path and skipped on the other — including for extra_config_url, which pulls a
remote TOML a third party may control.

The fix

prepare_command validates the tokenised arguments and drops the individual arguments the denylist rejects, logging which parameter was refused. The command itself still runs.

Behaviour change

Before	An auto command can set openai.key, config.secret_provider or config.review_path
After	Those arguments are dropped with a log line; every other override still applies

Files changed

pr_agent/agent/pr_agent.py | 8 ++++++++
 1 file changed, 8 insertions(+)

Testing

Written test-first: the test was committed red, then the fix turned it green.

New regression coverage in tests/unittest/test_prepare_command_arg_validation.py — 11 tests:

$ PYTHONPATH=. pytest tests/unittest/test_prepare_command_arg_validation.py
11 passed

Proven to be a genuine regression test: with every changed pr_agent/ file reverted to its 739ea8a6
version and the new test file left in place, the suite fails. It only passes with the fix applied.

Full unit suite on this branch:

$ PYTHONPATH=. pytest tests/unittest
3 failed, 2771 passed, 1 skipped, 1 xfailed, 89 warnings in 28.62s

The 3 failures are tests/unittest/test_extra_config_url.py, which fail identically on unmodified main in this
sandbox because they need outbound network; they pass in CI.

Also checked:

• ruff — no new findings vs main; isort — clean on every file touched
• No new code comments authored

Risk / compatibility

Allowed overrides are unaffected, and the quoting behaviour the tokenizer exists to preserve is covered by its own test.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[qodo-code-review]

Code Review by Qodo

Sorry, something went wrong

We weren't able to complete the code review on our side. Please try again manually by commenting /agentic_review on this PR.

[IsmaelMartinez]

Thanks, and the asymmetry you found is real; the revert probe goes properly red here.

The implementation has a false-positive problem though. The denylist substring-matches the whole argument including its value, so ordinary review instructions are silently dropped:

--pr_reviewer.extra_instructions="Flag any hardcoded openai.key in the diff"
--pr_reviewer.extra_instructions="Verify the litellm.base_url override"
--pr_reviewer.extra_instructions="Explain the .system prompt handling"
--pr_reviewer.extra_instructions="Check every config.url is not hardcoded"

All four apply on main today. The tell is not the return value, since a clean argument also returns ['/review']; it is that Updated setting PR_REVIEWER.EXTRA_INSTRUCTIONS never appears.

Matching the key rather than the whole token would keep the protection without the collateral.

[IsmaelMartinez]

This re-validates each argument whole, value included, so --pr_reviewer.extra_instructions whose text merely mentions openai.key, .url, .user, .system or .base_url is dropped along with the genuinely forbidden ones. Splitting on "=" and validating only the key would fix it.

Two smaller things while you are here: only the first offending param is named in the log though all of them are dropped, and "Use instead a configuration file" is carried over from the comment path, where it makes sense; an auto command already comes from one.