Support triggering commands via review assignment in Gitlab#2388
Open
igoraj wants to merge 2 commits into
Open
Support triggering commands via review assignment in Gitlab#2388igoraj wants to merge 2 commits into
igoraj wants to merge 2 commits into
Conversation
…b MR Add support for detecting when the PR-Agent bot is assigned as a reviewer on a GitLab merge request via the webhook payload, and automatically running configured commands (e.g. /review) in response. - Add handle_reviewer_assignment toggle and reviewer_commands config under [gitlab] in configuration.toml (disabled by default) - Add is_bot_assigned_as_reviewer() to parse changes.reviewers from the webhook payload and detect initial bot assignment - Add _get_bot_user_id() to auto-resolve the bot's GitLab user ID via API, with per-process caching - Add new dispatch branch in the merge_request event handler for reviewer-assignment updates - Call apply_repo_settings() before reading the toggle, enabling per-project configuration via .pr_agent.toml Assisted-by: opencode:deepseek-v4-pro
Contributor
Review Summary by QodoSupport auto-triggering commands on GitLab reviewer assignment
WalkthroughsDescription• Add GitLab reviewer assignment detection via webhook payload • Auto-trigger configured commands when bot assigned as reviewer • Implement bot user ID resolution with per-process caching • Add configurable toggle and command list for reviewer assignment Diagramflowchart LR
A["GitLab Webhook<br/>MR Update Event"] -->|"Check action=update"| B["is_bot_assigned_as_reviewer()"]
B -->|"Parse reviewers<br/>changes"| C["_get_bot_user_id()"]
C -->|"Resolve via API<br/>with cache"| D["Bot ID Match"]
D -->|"If enabled"| E["apply_repo_settings()"]
E -->|"handle_reviewer_assignment=true"| F["Execute<br/>reviewer_commands"]
F -->|"Run /review etc"| G["Process MR"]
File Changes1. pr_agent/servers/gitlab_webhook.py
|
Contributor
Code Review by Qodo
1. Permanent failure cache
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
Comment on lines
+111
to
+126
| _bot_user_id_cache = None | ||
|
|
||
| def _get_bot_user_id(): | ||
| global _bot_user_id_cache | ||
| if _bot_user_id_cache is not None: | ||
| return _bot_user_id_cache | ||
| try: | ||
| import gitlab | ||
| gl = gitlab.Gitlab( | ||
| get_settings().get("GITLAB.URL", "https://gitlab.com"), | ||
| private_token=get_settings().get("GITLAB.PERSONAL_ACCESS_TOKEN", None), | ||
| ) | ||
| gl.auth() | ||
| _bot_user_id_cache = gl.user.id | ||
| get_logger().info(f"Bot user ID resolved via API: {_bot_user_id_cache}") | ||
| return _bot_user_id_cache |
Contributor
There was a problem hiding this comment.
1. _bot_user_id_cache shared global state 📘 Rule violation ⛨ Security
_get_bot_user_id() caches a token-derived GitLab user ID in the process-global _bot_user_id_cache, which can persist across webhook requests and silently apply the wrong identity when tokens/instances differ. This can cause incorrect authorization/behavior (e.g., reviewer-assignment detection misfiring) and cross-request state leakage in multi-tenant, rotated-token, or warm AWS Lambda deployments.
Agent Prompt
## Issue description
`_bot_user_id_cache` is a module-level cache for a value derived from GitLab credentials (the current user/bot id). In a server handling multiple webhook requests—potentially with different tokens, tenants, or GitLab instances—this shared mutable global can persist across requests (and warm AWS Lambda invocations) and cause incorrect bot identity decisions, including reviewer-assignment detection triggering for the wrong user or failing to trigger.
## Issue Context
The webhook handler can set GitLab credentials dynamically per request (e.g., via secret provider and `X-Gitlab-Token`). The bot user id must be scoped to the effective credentials (and ideally the GitLab URL/instance) rather than stored in a single process-wide variable; otherwise, the cached ID can silently mismatch the current request’s token/instance, violating the intent of PR Compliance ID 18 (avoid shared mutable globals for provider scoping and protect against silent overrides).
## Fix Focus Areas
- pr_agent/servers/gitlab_webhook.py[111-129]
- pr_agent/servers/gitlab_webhook.py[207-243]
- pr_agent/servers/gitlab_lambda_webhook.py[2-7]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+296
to
+302
| # for reviewer assignment triggered merge requests | ||
| elif object_attributes.get('action') == 'update' and is_bot_assigned_as_reviewer(data): | ||
| url = object_attributes.get('url') | ||
| apply_repo_settings(url) | ||
| if get_settings().gitlab.get('handle_reviewer_assignment', False): | ||
| get_logger().info(f"Bot was assigned as reviewer on MR: {url}") | ||
| await _perform_commands_gitlab("reviewer_commands", PRAgent(), url, log_context, data) |
Contributor
There was a problem hiding this comment.
2. No tests for reviewer assignment 📘 Rule violation ⚙ Maintainability
The PR adds new behavior to auto-trigger commands when the bot is assigned as a reviewer, but no corresponding pytest coverage is added/updated. This increases regression risk for webhook parsing and configuration toggle behavior.
Agent Prompt
## Issue description
Reviewer-assignment-triggered command execution is new behavior and needs tests to prevent regressions.
## Issue Context
Add unit tests to validate:
- detection logic for bot assignment based on `changes.reviewers.previous/current`
- `handle_reviewer_assignment` gating
- correct command list used (`reviewer_commands`)
## Fix Focus Areas
- pr_agent/servers/gitlab_webhook.py[296-303]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+118
to
+126
| import gitlab | ||
| gl = gitlab.Gitlab( | ||
| get_settings().get("GITLAB.URL", "https://gitlab.com"), | ||
| private_token=get_settings().get("GITLAB.PERSONAL_ACCESS_TOKEN", None), | ||
| ) | ||
| gl.auth() | ||
| _bot_user_id_cache = gl.user.id | ||
| get_logger().info(f"Bot user ID resolved via API: {_bot_user_id_cache}") | ||
| return _bot_user_id_cache |
Contributor
There was a problem hiding this comment.
3. Auth type ignored 🐞 Bug ≡ Correctness
_get_bot_user_id always authenticates using private_token and does not pass SSL verification settings, while the rest of the GitLab integration supports oauth_token/private_token selection via GITLAB.AUTH_TYPE and SSL_VERIFY. This can prevent bot user-id resolution in oauth_token deployments and silently disable reviewer-assignment triggering.
Agent Prompt
### Issue description
`_get_bot_user_id()` hardcodes `private_token=...` and omits `ssl_verify`, diverging from `GitLabProvider` which selects auth based on `GITLAB.AUTH_TYPE` and configures SSL verification.
### Issue Context
If `GITLAB.AUTH_TYPE` is set to (or defaults to) oauth token usage, the hardcoded `private_token` path can fail and `is_bot_assigned_as_reviewer()` will always return `False`.
### Fix
- Mirror the `GitLabProvider` client creation logic:
- read `GITLAB.AUTH_TYPE` and pass either `oauth_token=` or `private_token=` accordingly
- pass `ssl_verify=get_settings().get("GITLAB.SSL_VERIFY", True)`
- Prefer extracting a shared helper to create an authenticated python-gitlab client to avoid drift.
### Fix Focus Areas
- pr_agent/servers/gitlab_webhook.py[113-126]
- pr_agent/git_providers/gitlab_provider.py[33-65]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
igoraj
force-pushed
the
feature/gitlab-reviewer-assignment-trigger
branch
from
133a196 to
dfe13bf
Compare
Contributor
|
Persistent review updated to latest commit dfe13bf |
Comment on lines
+150
to
+167
| def is_bot_assigned_as_reviewer(data) -> bool: | ||
| try: | ||
| changes = data.get('changes') | ||
| if not isinstance(changes, dict): | ||
| return False | ||
| if 'reviewers' not in changes: | ||
| return False | ||
| reviewers_change = changes['reviewers'] | ||
| if not isinstance(reviewers_change, dict): | ||
| return False | ||
| previous = reviewers_change.get('previous', []) | ||
| current = reviewers_change.get('current', []) | ||
| bot_user_id = _get_bot_user_id() | ||
| if bot_user_id is None: | ||
| return False | ||
| previous_ids = {r.get('id') for r in previous if isinstance(r, dict)} | ||
| current_ids = {r.get('id') for r in current if isinstance(r, dict)} | ||
| return bot_user_id in current_ids and bot_user_id not in previous_ids |
Contributor
There was a problem hiding this comment.
1. New strings use single quotes 📘 Rule violation ⚙ Maintainability
Newly added code uses single-quoted strings (e.g., dict keys like data.get('changes')), which
conflicts with the compliance requirement to use double quotes for strings. This can cause avoidable
Ruff formatting/lint failures.
Agent Prompt
## Issue description
New code uses single-quoted strings, but the compliance checklist requires double quotes for Python strings.
## Issue Context
This affects newly added GitLab reviewer-assignment logic and may trigger Ruff formatting/lint failures.
## Fix Focus Areas
- pr_agent/servers/gitlab_webhook.py[150-167]
- pr_agent/servers/gitlab_webhook.py[320-333]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Contributor
|
Persistent review updated to latest commit efbbf0d |
Comment on lines
+4
to
+6
| os.environ.setdefault("GITLAB__URL", "https://gitlab.example.com") | ||
| import pr_agent.servers.gitlab_webhook as gitlab_webhook | ||
|
|
Contributor
There was a problem hiding this comment.
1. Late import after env 📘 Rule violation ⚙ Maintainability
The test sets GITLAB__URL at module import time and then performs a module-level import of pr_agent.servers.gitlab_webhook, triggering Ruff E402 and making test behavior depend on ambient environment state. This violates lint/style conventions and the requirement that tests explicitly control environment variables/process-global state for determinism.
Agent Prompt
## Issue description
`tests/unittest/test_gitlab_reviewer_assignment.py` executes `os.environ.setdefault(...)` at import time and then imports `pr_agent.servers.gitlab_webhook` at module scope, which triggers Ruff `E402` and makes the test rely on implicit/ambient environment state.
## Issue Context
`pr_agent.servers.gitlab_webhook` performs work at import time (e.g., logger setup using `get_settings()`), so tests that need env/config should set env vars explicitly inside a fixture/test, then import/reload the module deterministically.
## Fix Focus Areas
- tests/unittest/test_gitlab_reviewer_assignment.py[1-6]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Contributor
|
Persistent review updated to latest commit b00b975 |
…b MR GitLab provides built-in commands like /request_review and /assign_reviewer which currently do not trigger the PR-Agent. This PR adds webhook handling for reviewer assignment events to automatically trigger commands (default: /review). Features: - Adds handle_reviewer_assignment toggle under [gitlab] section (default: false) - Supports configurable reviewer_commands array (default: ['/review']) - Bot identifies itself dynamically using the GitLab API - Safely skips draft MRs and invalid events - Validates configurations and payload structures to avoid exceptions Assisted-by: opencode:deepseek-v4-pro
igoraj
force-pushed
the
feature/gitlab-reviewer-assignment-trigger
branch
from
b00b975 to
779a192
Compare
Contributor
|
Persistent review updated to latest commit 779a192 |
Comment on lines
+122
to
+151
| cache_key = hashlib.sha256(f"{gitlab_url}:{gitlab_token}".encode()).hexdigest() | ||
|
|
||
| cached = _bot_user_id_cache.get(cache_key) | ||
| if cached is not None: | ||
| return cached if cached != -1 else None | ||
|
|
||
| def _resolve_sync(): | ||
| import gitlab | ||
|
|
||
| ssl_verify = get_settings().get("GITLAB.SSL_VERIFY", True) | ||
| auth_method = get_settings().get("GITLAB.AUTH_TYPE", "oauth_token") | ||
| kwargs = {"url": gitlab_url, "ssl_verify": ssl_verify} | ||
| if auth_method == "oauth_token": | ||
| kwargs["oauth_token"] = gitlab_token | ||
| else: | ||
| kwargs["private_token"] = gitlab_token | ||
|
|
||
| gl = gitlab.Gitlab(**kwargs) | ||
| gl.auth() | ||
| return gl.user.id | ||
|
|
||
| try: | ||
| user_id = await asyncio.to_thread(_resolve_sync) | ||
| _bot_user_id_cache[cache_key] = user_id | ||
| get_logger().info(f"Bot user ID resolved via API: {user_id}") | ||
| return user_id | ||
| except Exception as e: | ||
| _bot_user_id_cache[cache_key] = -1 | ||
| get_logger().error(f"Failed to resolve bot user ID: {e}") | ||
| return None |
Contributor
There was a problem hiding this comment.
1. Permanent failure cache 🐞 Bug ☼ Reliability
_get_bot_user_id() caches a sentinel (-1) on any exception and then returns None for all future calls for that URL/token, so one transient GitLab auth/network failure can permanently disable reviewer-assignment triggering until process restart.
Agent Prompt
### Issue description
`_get_bot_user_id()` stores `-1` in `_bot_user_id_cache` on *any* exception, and subsequent calls treat that as a permanent failure (returning `None`). This makes reviewer-assignment triggering brittle: a transient outage/rate-limit/timeout can disable the feature until the service is restarted.
### Issue Context
The cache is process-global and has no expiration logic. The code currently uses `-1` as a negative-cache sentinel with no retry window.
### Fix Focus Areas
- pr_agent/servers/gitlab_webhook.py[113-151]
### Suggested fix
- Replace the `-1` sentinel with a small structure like `{value, ts, is_failure}` and only honor negative-cache entries for a short TTL (e.g., 30–300 seconds), after which a retry is allowed.
- Alternatively, do not cache failures at all (or cache only specific non-retryable failures), and keep caching only successful resolutions.
- Add/adjust a unit test to verify retries occur after TTL expiry (or that failures are not cached).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Gitlab comes with built-it in commands
/request_review,/reviewer,/assign_reviewerwhich all pop-up when one tries to write/reviewin comment.I thought it might come handy if assigning reviewer would actually trigger the same thing, as it is now by default ignored.
I made the change as an togglable setting via
handle_reviewer_assignmentwhich is off by default so it does not break current behavior under[gitlab]section.In addition one can set
reviewer_commands = ["/review"]array optionally triggering several commands upon assignment.Example use in config: