fix(gitlab): preserve user-added labels in publish_labels and get_pr_…

[ashearin]

fix(gitlab): preserve user-added labels in publish_labels and get_pr_labels

Summary

The GitLab provider drops user-added labels every time PR-Agent publishes a label set (most visibly during /review). Two compounding bugs in GitLabProvider cause a read-modify-write race that wipes any label a user added after the webhook fired.

This PR fixes both bugs on the GitLab provider. The hot path (no refresh requested, no label changes needed) is unchanged.

What users see

"Every time /review runs, the agent removes the labels I just added to the MR."

It only happens on GitLab. GitHub and other providers are unaffected because the bug is provider-local.

Reproduction

1. Configure PR-Agent against a GitLab project with auto_review enabled (or trigger /review manually).
2. Open a merge request; let the agent post its first review (which applies Review effort N/5 and possibly Possible security concern labels).
3. As a user, add any label of your own (e.g., area/backend).
4. Trigger /review again, or push a new commit so auto_review fires.
5. Observed: the agent's Review effort label is updated, but your area/backend label is gone.
6. Expected: your label is preserved; only the agent-managed labels are updated.

Root cause

PRReviewer.set_review_labels does a read-modify-write cycle specifically to preserve user-added labels:

current_labels = self.git_provider.get_pr_labels(update=True)
current_labels_filtered = [
    label for label in current_labels
    if not label.lower().startswith("review effort")
    and not label.lower().startswith("possible security concern")
]
new_labels = review_labels + current_labels_filtered
self.git_provider.publish_labels(new_labels)

Both halves of that cycle are broken on the GitLab provider.

Bug 1 — get_pr_labels(update=True) ignores the flag

def get_pr_labels(self, update=False):
    return self.mr.labels

The update argument is unused; the method always returns self.mr.labels from the snapshot cached at provider construction. A label the user added after the webhook fired is missing from current_labels, so the filter step drops it out of new_labels.

Bug 2 — publish_labels does a full PUT against the same stale snapshot

def publish_labels(self, pr_types):
    try:
        self.mr.labels = list(set(pr_types))
        self.mr.save()
    ...

Setting self.mr.labels and calling save() translates to a full PUT on the labels array — the server overwrites whatever it has with pr_types. Any label added during the read-modify-write window (or any label missed because of Bug 1) is wiped.

Fix

Both halves of the cycle now refresh, and the write becomes an incremental set-diff rather than a full overwrite.

get_pr_labels(update=True) now actually refreshes

def get_pr_labels(self, update=False):
    if update:
        try:
            self.mr = self._get_merge_request()
        except Exception as e:
            get_logger().warning(f"get_pr_labels: refresh failed, using cached snapshot. error: {e}")
    return self.mr.labels

update=False keeps the existing cached read, so callers that don't ask for a refresh see no behavior change.

publish_labels refreshes and writes a diff

def publish_labels(self, pr_types):
    try:
        desired = set(pr_types)
        try:
            self.mr = self._get_merge_request()
        except Exception as refresh_err:
            get_logger().warning(
                f"publish_labels: failed to refresh MR before save, "
                f"using cached snapshot. error: {refresh_err}"
            )
        current = set(self.mr.labels or [])
        to_add = sorted(desired - current)
        to_remove = sorted(current - desired)
        if not to_add and not to_remove:
            return
        if to_add:
            self.mr.add_labels = ",".join(to_add)
        if to_remove:
            self.mr.remove_labels = ",".join(to_remove)
        self.mr.save()
    except Exception as e:
        get_logger().warning(f"Failed to publish labels, error: {e}")

Why add_labels / remove_labels instead of labels:

• They map to GitLab's add_labels and remove_labels query parameters on PUT /projects/:id/merge_requests/:merge_request_iid (see the GitLab MR API).
• The server then applies a set-diff rather than replacing the whole array.
• Any label a user adds outside the diff during the (now much smaller) race window is preserved by the server, not clobbered.

Failure modes

• If the refresh call fails (network blip, permissions, etc.), both paths fall back to the cached snapshot rather than failing the whole publish. The behavior degrades to pre-PR semantics for that one operation instead of breaking label publishing entirely.

Hot path

• to_add and to_remove are both empty → the method returns without calling save(). No-op publishes are now actually no-ops.

What is NOT changed

• Other providers (GithubProvider, BitbucketProvider, BitbucketServerProvider, AzureDevopsProvider, GiteaProvider, GerritProvider, CodeCommitProvider, LocalGitProvider).
• The public signatures of publish_labels and get_pr_labels.
• The behavior of get_pr_labels(update=False) (still returns the cached snapshot).
• Any caller of set_review_labels, PRDescription, PRGenerateLabels, etc. — they continue to call the same provider methods with the same arguments.

Testing

Manual testing against a self-managed GitLab instance:

1. Open an MR; let /review run.
2. As a user, add a label area/backend.
3. Trigger /review again.
4. Before this PR: area/backend is gone after the second review.
5. After this PR: area/backend is preserved; only the agent's Review effort label is updated.

Edge cases verified manually:

• pr_types equals the current label set → method returns early, no save() call.
• Refresh fails (simulated by killing the GitLab token mid-flight) → warning logged, fallback to cached snapshot, publish still attempts.
• self.mr.labels is None → handled by current = set(self.mr.labels or []).
• User adds a label during /review execution (between refresh and save()) → preserved by the server because we only push the diff, not the whole array.

I did not add automated tests in this PR — the existing tests/unittest/test_gitlab_provider.py does not exercise publish_labels and adding a meaningful test would require either a python-gitlab mock with attribute-side-effect tracking or a live test environment. Happy to add tests in a follow-up if maintainers want them.

Related

• The bug exists in every released version of pr-agent I checked (v0.34.x–v0.37.0).
• This affects every GitLab user who has both auto_review (or repeated manual /review) and any user-curated label workflow.

[qodo-free-for-open-source-projects]

PR Summary by Qodo

Fix GitLab label publishing to preserve user-added MR labels

🐞 Bug fix 🕐 20-40 Minutes

AI Description

• Refresh GitLab MR labels when callers request an updated read.
• Publish label changes via add/remove diffs to avoid clobbering concurrent user labels.
• Make no-op label publishes return early to avoid unnecessary API writes.

Diagram

graph TD
  R["PRReviewer"] -->|"get_pr_labels(update=True)"| GLP["GitLabProvider"] -->|"refresh MR"| API{{"GitLab MR API"}} -->|"labels snapshot"| GLP -->|"compute label diff"| GLP -->|"publish_labels(add/remove)"| API
  subgraph Legend
    direction LR
    _comp["Component"] ~~~ _ext{{"External API"}}
  end

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Always refetch MR (remove caching for labels)

• ➕ Simpler mental model: every read uses the latest server state
• ➕ Reduces risk of other stale-snapshot issues beyond labels
• ➖ Adds API calls to hot paths that currently rely on cached MR data
• ➖ Higher latency and rate-limit pressure for GitLab users

2. Full overwrite after refresh (refresh + set labels array)

• ➕ Less logic than computing add/remove diffs
• ➕ Uses existing update mechanism (set labels then save)
• ➖ Still vulnerable to a race where a user adds a label between refresh and save
• ➖ More likely to clobber concurrent edits than server-applied diffs

Recommendation: Keep the PR’s approach: best-effort refresh plus server-applied add/remove diffs. It preserves user-added labels even with concurrent edits, avoids unnecessary writes on no-op publishes, and maintains the existing fast path for callers that do not request a refresh.

Files changed (1) +45 / -1

Bug fix (1) +45 / -1

gitlab_provider.pyFix GitLab MR label refresh and publish to avoid clobbering user labels +45/-1

Fix GitLab MR label refresh and publish to avoid clobbering user labels

• Updates publish_labels to refresh the MR state, compute add/remove deltas, and apply changes via incremental label updates rather than overwriting the full labels array. Updates get_pr_labels(update=True) to actually refresh the merge request snapshot, with warning-only fallback to cached data if refresh fails.

pr_agent/git_providers/gitlab_provider.py

[qodo-free-for-open-source-projects]

Code Review by Qodo

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

1. Settings custom_labels not restored 📘 Rule violation ☼ Reliability ⭐ New

Description

The new test helper mutates the global Dynaconf custom_labels setting but only restores it when
the original value is not None, which can leak state across tests and cause nondeterministic
failures.

Code

tests/unittest/test_pr_description_output_core.py[R629-646]

+    def _with_settings(self, enable_custom_labels, custom_labels):
+        from pr_agent.config_loader import get_settings
+        settings = get_settings()
+        snap = (
+            settings.config.get("enable_custom_labels", False),
+            settings.get("custom_labels", None),
+        )
+        settings.config.enable_custom_labels = enable_custom_labels
+        if custom_labels is not None:
+            settings.set("custom_labels", custom_labels)
+        return settings, snap
+
+    def _restore(self, settings, snap):
+        original_enable, original_custom = snap
+        settings.config.enable_custom_labels = original_enable
+        if original_custom is not None:
+            settings.set("custom_labels", original_custom)
+

Evidence

PR Compliance ID 19 requires tests that mutate global/module state to snapshot and restore it. Here,
_with_settings() sets custom_labels whenever a non-None value is passed, but _restore() only
resets custom_labels when original_custom is not None, so a previously-None value can be
overwritten and not restored.

tests/unittest/test_pr_description_output_core.py[629-646]
Best Practice: Learned patterns

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`TestDescribeManagedLabelNamespace._with_settings()` can change `settings.custom_labels`, but `_restore()` only restores it when the original value is not `None`. If the original value was `None` and the test sets it to `[]` (or another value), that mutated global/module state can leak into subsequent tests.

## Issue Context
This violates the requirement to snapshot and restore mutated global/module state in tests/requests to avoid flakiness.

## Fix Focus Areas
- tests/unittest/test_pr_description_output_core.py[629-646]

## Suggested approach
- Track whether `_with_settings()` actually changed `custom_labels` (e.g., store a boolean) and always restore it.
- If the original was `None`, explicitly restore to `None` (or delete/reset the key in a Dynaconf-safe way) rather than leaving the test-written value in place.

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

---

2. Missing blank-line separation 🐞 Bug ⚙ Maintainability ⭐ New

Description

pr_agent/algo/utils.py defines _describe_managed_label_names() immediately after
set_custom_labels() without the required two blank lines, which violates Ruff’s enabled E
(pycodestyle) rules and can fail linting/CI.

Code

pr_agent/algo/utils.py[R965-968]

    variables["labels_minimal_to_labels_dict"] = labels_minimal_to_labels_dict

+def _describe_managed_label_names():
+    """Return the lowercase set of label names that ``/describe`` manages.

Evidence

Ruff is configured to select pycodestyle E rules, and the file shows consecutive top-level
function definitions without the required blank-line separation.

pyproject.toml[52-75]
pr_agent/algo/utils.py[942-970]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
Ruff is configured to lint pycodestyle `E` rules, which includes `E305` (expected 2 blank lines after class/function). In `pr_agent/algo/utils.py`, `_describe_managed_label_names()` starts immediately after `set_custom_labels()` with insufficient blank-line separation, which can break CI.

### Issue Context
The repository enables Ruff with `lint.select` including `E`.

### Fix Focus Areas
- pr_agent/algo/utils.py[964-970]
- pyproject.toml[52-75]

### How to fix
Insert the missing blank line(s) so there are **two** blank lines between the end of `set_custom_labels` and the start of `_describe_managed_label_names`.

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

---

3. Single quotes in _is_review_managed 📘 Rule violation ⚙ Maintainability

Description

New code in _is_review_managed uses single-quoted string literals, which can violate the repo’s
Ruff quoting preference and fail linting/formatting checks.

Code

pr_agent/tools/pr_reviewer.py[R410-412]

+                    lowered = label.lower()
+                    return (lowered.startswith('review effort')
+                            or lowered.startswith('possible security concern'))

Evidence

PR Compliance ID 9 requires Python code to follow Ruff settings including preferring double quotes.
The added _is_review_managed implementation uses single-quoted literals in `startswith('review
effort') and startswith('possible security concern')`.

AGENTS.md: Python Code Must Conform to Ruff Settings (Line Length 120, isort Import Grouping, Prefer Double Quotes): AGENTS.md: Python Code Must Conform to Ruff Settings (Line Length 120, isort Import Grouping, Prefer Double Quotes): AGENTS.md: Python Code Must Conform to Ruff Settings (Line Length 120, isort Import Grouping, Prefer Double Quotes): AGENTS.md: Python Code Must Conform to Ruff Settings (Line Length 120, isort Import Grouping, Prefer Double Quotes): AGENTS.md: Python Code Must Conform to Ruff Settings (Line Length 120, isort Import Grouping, Prefer Double Quotes): AGENTS.md: Python Code Must Conform to Ruff Settings (Line Length 120, isort Import Grouping, Prefer Double Quotes): AGENTS.md: Python Code Must Conform to Ruff Settings (Line Length 120, isort Import Grouping, Prefer Double Quotes): AGENTS.md: Python Code Must Conform to Ruff Settings (Line Length 120, isort Import Grouping, Prefer Double Quotes): AGENTS.md: Python Code Must Conform to Ruff Settings (Line Length 120, isort Import Grouping, Prefer Double Quotes): AGENTS.md: Python Code Must Conform to Ruff Settings (Line Length 120, isort Import Grouping, Prefer Double Quotes): AGENTS.md: Python Code Must Conform to Ruff Settings (Line Length 120, isort Import Grouping, Prefer Double Quotes): AGENTS.md: Python Code Must Conform to Ruff Settings (Line Length 120, isort Import Grouping, Prefer Double Quotes): AGENTS.md: Python Code Must Conform to Ruff Settings (Line Length 120, isort Import Grouping, Prefer Double Quotes): AGENTS.md: Python Code Must Conform to Ruff Settings (Line Length 120, isort Import Grouping, Prefer Double Quotes): AGENTS.md: Python Code Must Conform to Ruff Settings (Line Length 120, isort Import Grouping, Prefer Double Quotes): AGENTS.md: Python Code Must Conform to Ruff Settings (Line Length 120, isort Import Grouping, Prefer Double Quotes)
pr_agent/tools/pr_reviewer.py[410-412]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Ruff configuration requires preferring double quotes, but new code uses single-quoted string literals in `_is_review_managed`.
## Issue Context
This was introduced in the new predicate used by `publish_managed_labels`, so it is in-scope for the PR and may cause Ruff failures.
## Fix Focus Areas
- pr_agent/tools/pr_reviewer.py[410-412]

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

---

4. Hardcoded managed_default label set 📘 Rule violation ⚙ Maintainability

Description

_safe_publish_labels introduces a hardcoded default label set (managed_default), duplicating
label classification logic and making behavior harder to override/maintain via configuration.

Code

pr_agent/tools/pr_description.py[R216-227]

+        enable_custom_labels = get_settings().config.get("enable_custom_labels", False)
+        custom_labels = get_settings().get("custom_labels", []) if enable_custom_labels else []
+        managed_default = {"bug fix", "tests", "enhancement", "documentation", "other"}
+
+        def _is_managed(label: str) -> bool:
+            if label is None:
+                return False
+            if label.lower() in managed_default:
+                return True
+            if enable_custom_labels and label in custom_labels:
+                return True
+            return False

Evidence

PR Compliance ID 7 requires using supported configuration mechanisms instead of hardcoding runtime
behavior. The new _safe_publish_labels adds a hardcoded managed_default set controlling which
labels are treated as managed vs user labels.

AGENTS.md: Use Configuration Overrides Instead of Hardcoding Values: AGENTS.md: Use Configuration Overrides Instead of Hardcoding Values: AGENTS.md: Use Configuration Overrides Instead of Hardcoding Values: AGENTS.md: Use Configuration Overrides Instead of Hardcoding Values: AGENTS.md: Use Configuration Overrides Instead of Hardcoding Values: AGENTS.md: Use Configuration Overrides Instead of Hardcoding Values: AGENTS.md: Use Configuration Overrides Instead of Hardcoding Values: AGENTS.md: Use Configuration Overrides Instead of Hardcoding Values: AGENTS.md: Use Configuration Overrides Instead of Hardcoding Values: AGENTS.md: Use Configuration Overrides Instead of Hardcoding Values: AGENTS.md: Use Configuration Overrides Instead of Hardcoding Values: AGENTS.md: Use Configuration Overrides Instead of Hardcoding Values: AGENTS.md: Use Configuration Overrides Instead of Hardcoding Values: AGENTS.md: Use Configuration Overrides Instead of Hardcoding Values: AGENTS.md: Use Configuration Overrides Instead of Hardcoding Values: AGENTS.md: Use Configuration Overrides Instead of Hardcoding Values
pr_agent/tools/pr_description.py[216-227]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`_safe_publish_labels` hardcodes the default /describe-managed labels in `managed_default`, rather than sourcing from a shared configuration/utility, which risks drift and reduces configurability.
## Issue Context
There is already label classification behavior tied to settings like `enable_custom_labels`/`custom_labels`; duplicating the default list here makes future changes error-prone.
## Fix Focus Areas
- pr_agent/tools/pr_description.py[216-227]

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

---

5. Default label not managed 🐞 Bug ≡ Correctness

Description

PRDescription._safe_publish_labels does not classify the default label "Bug fix with tests" as
managed, so if /describe sets it once and later stops recommending it, the label will be preserved
as a “user label” and never removed.

Code

pr_agent/tools/pr_description.py[R216-227]

+        enable_custom_labels = get_settings().config.get("enable_custom_labels", False)
+        custom_labels = get_settings().get("custom_labels", []) if enable_custom_labels else []
+        managed_default = {"bug fix", "tests", "enhancement", "documentation", "other"}
+
+        def _is_managed(label: str) -> bool:
+            if label is None:
+                return False
+            if label.lower() in managed_default:
+                return True
+            if enable_custom_labels and label in custom_labels:
+                return True
+            return False

Evidence

set_custom_labels() can present Bug fix with tests as a default label choice, but
_safe_publish_labels only considers five base labels (plus configured custom_labels) as managed;
that makes Bug fix with tests fall into the “user label” bucket and prevents its removal later.

pr_agent/tools/pr_description.py[216-227]
pr_agent/algo/utils.py[942-953]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`PRDescription._safe_publish_labels` defines the `/describe` “managed namespace” via `managed_default` plus configured `custom_labels`. However, `set_custom_labels()` injects a default label list (when `enable_custom_labels` is on but no `custom_labels` are configured) that includes `Bug fix with tests`, and `_safe_publish_labels` does not treat it as managed. This causes that label to be treated as a user label and therefore never removed on later `/describe` runs.
## Issue Context
- The goal of `publish_managed_labels` is to let the agent remove labels it manages when they’re no longer desired.
- The predicate in `_safe_publish_labels` must cover the full default label set that can be produced when custom labels are enabled but not explicitly configured.
## Fix Focus Areas
- pr_agent/tools/pr_description.py[216-227]
- pr_agent/algo/utils.py[946-953]
## Suggested change
When `enable_custom_labels` is enabled and `custom_labels` is empty/falsey, include the same default set from `set_custom_labels` (notably `bug fix with tests`) in the managed predicate, or add `bug fix with tests` to `managed_default` under the same conditions.

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

---

6. Refresh failure misreported 🐞 Bug ◔ Observability

Description

GitLabProvider.publish_managed_labels returns None both when labels are already correct and when
MR refresh fails; PRReviewer and PRDescription interpret None as “already set/unchanged”, so
refresh failures can be incorrectly logged as successful no-ops.

Code

pr_agent/tools/pr_reviewer.py[R414-416]

+                published = self.git_provider.publish_managed_labels(review_labels, _is_review_managed)
+                if published is None:
+                    get_logger().info(f"Review labels are already set; managed set:\n{review_labels}")

Evidence

The provider returns None on refresh failure and also returns None when nothing changes; both
/review and /describe callers log None as “already set/unchanged”, conflating failure with
success.

pr_agent/git_providers/gitlab_provider.py[932-949]
pr_agent/tools/pr_reviewer.py[414-418]
pr_agent/tools/pr_description.py[229-235]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`GitLabProvider.publish_managed_labels` uses `None` for two different outcomes:
1) no-op (desired labels already match)
2) abort due to refresh failure
Callers (`PRReviewer.set_review_labels` and `PRDescription._safe_publish_labels`) treat `None` as “already set/unchanged”, which misreports refresh failures and makes debugging label-update problems harder.
## Issue Context
- GitLabProvider intentionally aborts on refresh failure to avoid clobbering labels.
- That behavior is fine, but the return value should allow callers to log accurately (or the provider should raise on refresh failure so callers can handle it distinctly).
## Fix Focus Areas
- pr_agent/git_providers/gitlab_provider.py[932-949]
- pr_agent/tools/pr_reviewer.py[414-418]
- pr_agent/tools/pr_description.py[231-235]
## Suggested change
Pick one:
- **Option A (cleanest):** raise the refresh exception on refresh failure (don’t return `None`), and let callers catch/log it (PRDescription already isolates label failures).
- **Option B:** return a distinct sentinel/result type (e.g., an enum/tuple) differentiating `NO_CHANGE` vs `SKIPPED_REFRESH_FAIL`, and update caller logs accordingly.
- **Option C (minimal):** keep returning `None`, but update caller log messages to reflect ambiguity (e.g., “unchanged or skipped (refresh failure logged by provider)”).

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

---

View more (6)

7. _safe_publish_labels masks exceptions 📘 Rule violation ⛨ Security

Description

PRDescription._safe_publish_labels catches Exception broadly and only logs a warning, which can
hide unexpected failures and make debugging/security triage harder. Prefer catching only expected
provider/network exceptions (or re-raising unexpected ones) while still allowing /describe to
continue.

Code

pr_agent/tools/pr_description.py[R217-221]

+        except Exception as label_err:
+            get_logger().warning(
+                f"Failed to update labels during PR description; "
+                f"continuing with description publish. error: {label_err}"
+            )

Evidence

The compliance checklist warns against broad exception masking in security-boundary code paths. The
new helper catches Exception and continues execution after logging, rather than handling only
expected failure modes.

pr_agent/tools/pr_description.py[217-221]
Best Practice: Learned patterns

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`PRDescription._safe_publish_labels` uses a broad `except Exception` and then continues, which is discouraged for security-boundary operations (provider/network calls) because it can mask unexpected failures.
## Issue Context
This helper exists to keep `/describe` resilient, but it should still avoid blanket exception masking per the compliance rule.
## Fix Focus Areas
- pr_agent/tools/pr_description.py[206-221]

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

---

8. Label race still possible 🐞 Bug ≡ Correctness

Description

_safe_publish_labels builds new_labels from one refreshed snapshot, but
GitLabProvider.publish_labels refreshes again and computes to_remove = current - desired, so any
label added between those two refreshes can still be removed. This leaves a timing-dependent user
label loss during /describe (and similarly for other read-modify-write label flows).

Code

pr_agent/git_providers/gitlab_provider.py[R904-918]

+            desired = set(pr_types)
+            try:
+                self.mr = self._get_merge_request()
+            except Exception as refresh_err:
+                # Strict policy: a stale snapshot can produce an incorrect
+                # add/remove diff that re-introduces the bug this method is
+                # meant to fix. Abort the publish, log, and leave server
+                # state untouched rather than risk clobbering user labels.
+                get_logger().warning(
+                    f"publish_labels: aborting, failed to refresh MR before save. error: {refresh_err}"
+                )
+                return
+            current = set(self.mr.labels or [])
+            to_add = sorted(desired - current)
+            to_remove = sorted(current - desired)

Evidence

PRDescription._safe_publish_labels computes new_labels from a refreshed read, then calls
publish_labels(new_labels); publish_labels refreshes again and removes current - desired,
which will include any label added between these two reads.

pr_agent/tools/pr_description.py[197-216]
pr_agent/git_providers/gitlab_provider.py[887-920]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Label updates are still a multi-step read/compute/write across different MR snapshots: callers compute a desired label set from one read, but `GitLabProvider.publish_labels()` refreshes and removes any server labels not present in that earlier desired set. A label added between the caller’s read and `publish_labels()`’ refresh can therefore still be deleted.
### Issue Context
This occurs because `desired` is derived outside the provider (caller), while `current` is derived after a subsequent refresh inside `publish_labels`, and the method removes `current - desired`.
### Fix Focus Areas
- pr_agent/git_providers/gitlab_provider.py[887-941]
- pr_agent/tools/pr_description.py[197-222]
- pr_agent/tools/pr_reviewer.py[365-416]
### Suggested fix approach
Implement an atomic “managed-label update” path that does *refresh + filter + diff + save* based on a single refreshed snapshot (e.g., move the caller’s filtering/merge logic into the provider, or add a provider method that accepts the managed-label predicate/prefixes and the desired managed labels). Then update `/describe` and `/review` to call that atomic method instead of doing a cross-method read-modify-write.

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

---

9. Label-diff tests too weak 🐞 Bug ⚙ Maintainability

Description

The new GitLabProvider.publish_labels tests mostly assert that save() was called and that transient
add_labels/remove_labels attributes were removed afterward, but they never assert the actual
add/remove label payload computed for the request. This can allow regressions where the wrong labels
are added/removed (or none are sent) while the test suite still passes.

Code

tests/unittest/test_gitlab_provider.py[R286-309]

+    def test_publish_labels_adds_only_missing(self, gitlab_provider):
+        mr = self._prime_mr_for_labels(gitlab_provider, ["bug"])
+
+        gitlab_provider.publish_labels(["bug", "review effort 3/5"])
+
+        # Server-side state is left to GitLab to compute; we only assert what
+        # we wrote on the wire.
+        assert mr.save.call_count == 1
+        # ``add_labels`` is deleted on the finally branch after save().
+        assert not hasattr(mr, "add_labels")
+        assert not hasattr(mr, "remove_labels")
+
+    def test_publish_labels_removes_stale_managed_labels(self, gitlab_provider):
+        mr = self._prime_mr_for_labels(
+            gitlab_provider, ["review effort 5/5", "Possible security concern"]
+        )
+
+        # Caller wants to switch the managed labels to a fresh set.
+        gitlab_provider.publish_labels(["review effort 2/5"])
+
+        assert mr.save.call_count == 1
+        assert not hasattr(mr, "add_labels")
+        assert not hasattr(mr, "remove_labels")
+

Evidence

publish_labels computes to_add/to_remove and assigns them to self.mr.add_labels /
self.mr.remove_labels prior to calling self.mr.save(), but the added tests do not validate those
values at the moment of saving—only that save() was invoked and that the attributes are absent
afterward.

pr_agent/git_providers/gitlab_provider.py[887-941]
tests/unittest/test_gitlab_provider.py[286-309]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`GitLabProvider.publish_labels()` now relies on computing a set-diff and sending it via `mr.add_labels` / `mr.remove_labels` before `mr.save()`. The newly added tests only check that `save()` was called and that these transient attributes are not present *after* the call, but they do not assert what values were set at save-time.
### Issue Context
Because `publish_labels()` deletes `add_labels`/`remove_labels` in a `finally` block, the tests need to capture the values *during* the `save()` call (e.g., via `mr.save.side_effect`) and assert they match the expected comma-separated label lists.
### Fix Focus Areas
- tests/unittest/test_gitlab_provider.py[286-309]

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

---

10. get_pr_labels(update=True) can raise ✓ Resolved 📘 Rule violation ☼ Reliability

Description

GitLabProvider.get_pr_labels(update=True) refreshes via _get_merge_request() without handling
network/auth/API failures, so exceptions can propagate and abort higher-level tools that simply want
fresh labels. Because PRDescription.run calls this during its publish flow, a transient GitLab
error can cause /describe to exit early, skipping label updates and potentially the PR description
publish as well.

Code

pr_agent/git_providers/gitlab_provider.py[R961-963]

+        if update:
+            self.mr = self._get_merge_request()
return self.mr.labels

Evidence

The updated get_pr_labels implementation performs a refresh by calling _get_merge_request()
whenever update=True, and because that refresh call is not wrapped in any local error handling,
any exceptions raised during the remote MR fetch will bubble up to callers. The evidence also shows
PRDescription invokes get_pr_labels(update=True) as part of its publishing path (before/while
publishing labels and description), and its outer tool-level exception handling aborts the overall
run on any exception; therefore a transient refresh failure can stop label publishing and cause
/describe to terminate early, skipping description publication. This differs from other providers
(e.g., GitHub) that treat label-fetch failures as non-fatal by catching errors and returning an
empty list, which is why callers aren’t structured to expect exceptions here.

Rule 3: Robust Error Handling
pr_agent/git_providers/gitlab_provider.py[947-963]
pr_agent/tools/pr_description.py[156-168]
pr_agent/tools/pr_description.py[156-203]
pr_agent/git_providers/github_provider.py[934-946]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`GitLabProvider.get_pr_labels(update=True)` can raise raw exceptions from `_get_merge_request()` without local handling, and `PRDescription.run` calls it in the publish flow without isolating failures to just the label-publishing portion. As a result, transient GitLab/API/network/auth errors during refresh can bubble up to the tool-level exception handler and cause `/describe` to exit early, skipping label updates and potentially skipping publishing the PR description for that run.
## Issue Context
Other providers typically treat label-fetch failures as non-fatal (e.g., catch and return an empty list), so callers are not written to expect exceptions from `get_pr_labels`. While the stricter refresh behavior for GitLab can be reasonable for label integrity (avoid clobbering labels with stale data), the failure mode should be controlled: `/describe` should degrade gracefully by skipping label updates when refresh fails, while still publishing the PR description.
## Fix Focus Areas
- pr_agent/git_providers/gitlab_provider.py[947-963]
- pr_agent/tools/pr_description.py[156-170]

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

---

11. publish_labels publishes with cached MR ✓ Resolved 📘 Rule violation ☼ Reliability

Description

If _get_merge_request() refresh fails, both publish_labels and get_pr_labels(update=True) fall
back to the cached self.mr snapshot (computing current / returning self.mr.labels) and may
still proceed to save(). This can reintroduce stale-remote-state behavior, causing incorrect label
add/remove decisions and publishing label updates based on outdated server state during concurrent
changes.

Code

pr_agent/git_providers/gitlab_provider.py[R904-923]

+            try:
+                self.mr = self._get_merge_request()
+            except Exception as refresh_err:
+                # Best-effort refresh: if it fails (network, perms) we fall back
+                # to the cached snapshot rather than failing the whole publish.
+                get_logger().warning(
+                    f"publish_labels: failed to refresh MR before save, "
+                    f"using cached snapshot. error: {refresh_err}"
+                )
+            current = set(self.mr.labels or [])
+            to_add = sorted(desired - current)
+            to_remove = sorted(current - desired)
+            if not to_add and not to_remove:
+                return
+            # GitLab accepts comma-separated strings for these attributes.
+            if to_add:
+                self.mr.add_labels = ",".join(to_add)
+            if to_remove:
+                self.mr.remove_labels = ",".join(to_remove)
self.mr.save()

Evidence

PR Compliance ID 17 requires re-fetching the latest server-side object before reading or publishing
updates to avoid using stale cached remote state. The cited changes introduce a refresh attempt
that, on failure, explicitly falls back to the cached self.mr snapshot: publish_labels still
computes the current labels from cached state and calls self.mr.save(), and
get_pr_labels(update=True) catches refresh failures and returns self.mr.labels anyway, both of
which can be stale and thus lead to incorrect label diffs and updates.

pr_agent/git_providers/gitlab_provider.py[904-923]
pr_agent/git_providers/gitlab_provider.py[937-942]
Best Practice: Learned patterns

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`publish_labels` and `get_pr_labels(update=True)` can proceed using the cached `self.mr` snapshot when the MR refresh fails, which may cause decisions and label updates to be made from stale remote state and lead to incorrect label add/remove behavior during concurrent changes.
## Issue Context
Compliance (PR Compliance ID 17) requires re-fetching the latest server-side object immediately before reading state used to compute updates and before publishing updates, to avoid stale cached remote state. The current behavior falls back to cached `self.mr` data on refresh failure (including returning cached labels for `update=True`) and in `publish_labels` may still call `save()`, risking incorrect diffs and publishing updates based on outdated label state.
## Fix Focus Areas
- pr_agent/git_providers/gitlab_provider.py[904-923]
- pr_agent/git_providers/gitlab_provider.py[937-942]

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

---

12. Stale label diff fields ✓ Resolved 🐞 Bug ≡ Correctness

Description

GitLabProvider.publish_labels sets transient self.mr.add_labels/remove_labels but never clears them,
so a later self.mr.save() (e.g., publish_description) can unintentionally resend prior label
add/remove operations alongside unrelated updates. This can cause unexpected label churn when tools
perform label updates and then update MR fields in the same run.

Code

pr_agent/git_providers/gitlab_provider.py[R919-923]

+            if to_add:
+                self.mr.add_labels = ",".join(to_add)
+            if to_remove:
+                self.mr.remove_labels = ",".join(to_remove)
self.mr.save()

Evidence

publish_labels sets self.mr.add_labels / self.mr.remove_labels conditionally and calls
self.mr.save() without clearing those attributes. The same provider instance later calls
self.mr.save() in publish_description, and the pr_description tool can execute
publish_labels(...) followed by publish_description(...) sequentially, allowing stale label-diff
attributes to leak into unrelated saves.

pr_agent/git_providers/gitlab_provider.py[887-925]
pr_agent/git_providers/gitlab_provider.py[486-492]
pr_agent/tools/pr_description.py[156-187]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`GitLabProvider.publish_labels()` mutates the shared `self.mr` object by setting `add_labels` / `remove_labels` before calling `self.mr.save()`, but it never resets those transient fields. Because other methods (e.g., `publish_description`) also call `self.mr.save()` on the same object, stale `add_labels` / `remove_labels` values can be resent during unrelated updates.
### Issue Context
This is especially likely in flows like `pr_description`, which can publish labels and then publish the description using the same `git_provider` instance.
### Fix Focus Areas
- pr_agent/git_providers/gitlab_provider.py[887-925]
- pr_agent/git_providers/gitlab_provider.py[486-492]
- pr_agent/tools/pr_description.py[156-187]
### Implementation notes
- Ensure `add_labels` and `remove_labels` are cleared on `self.mr` before any early return and after `save()` (e.g., set to `None`/empty string, or `delattr` if present).
- Alternatively, avoid storing these transient fields on the shared `self.mr` by using a local MR object for the label update and then refreshing `self.mr` afterward.

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

---

13. Misleading label param comment ✓ Resolved 🐞 Bug ⚙ Maintainability

Description

In GitLabProvider.publish_labels, the new comment says python-gitlab attributes translate to GitLab
API parameters named "labels_to_add/labels_to_remove", while the code actually uses
"add_labels/remove_labels" attributes. This mismatch can confuse maintainers about which names are
library-level vs API-level and lead to incorrect future edits.

Code

pr_agent/git_providers/gitlab_provider.py[R897-902]

+            # New behavior: re-fetch the MR immediately before computing the
+            # delta, then use python-gitlab's ``add_labels`` / ``remove_labels``
+            # attributes on save. Those translate to GitLab's incremental
+            # ``labels_to_add`` / ``labels_to_remove`` API parameters, so the
+            # server applies a set-diff and concurrent additions outside the
+            # diff are preserved.

Evidence

The cited comment claims labels_to_add/labels_to_remove, but the implementation immediately below
sets add_labels/remove_labels, demonstrating an inconsistency in naming within the same method.

pr_agent/git_providers/gitlab_provider.py[897-902]
pr_agent/git_providers/gitlab_provider.py[927-931]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
A comment in `GitLabProvider.publish_labels` references `labels_to_add` / `labels_to_remove`, while the implementation sets `self.mr.add_labels` / `self.mr.remove_labels`. This is only a documentation mismatch, but it blurs the distinction between python-gitlab attribute names and GitLab REST parameter names.
### Issue Context
The code is likely correct; the comment should be made precise so future maintainers don’t “fix” the wrong thing or copy incorrect parameter names.
### Fix Focus Areas
- pr_agent/git_providers/gitlab_provider.py[897-902]

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

[qodo-free-for-open-source-projects]

Code review by qodo was updated up to the latest commit 594c97f

[qodo-free-for-open-source-projects]

Code review by qodo was updated up to the latest commit 204d145

[qodo-free-for-open-source-projects]

Code review by qodo was updated up to the latest commit ff0ce9c

[qodo-free-for-open-source-projects]

Code review by qodo was updated up to the latest commit bb8ae12

[qodo-free-for-open-source-projects]

Code review by qodo was updated up to the latest commit d2786f5

[qodo-free-for-open-source-projects]

Code review by qodo was updated up to the latest commit a7a8cad

[qodo-free-for-open-source-projects]

Code review by qodo was updated up to the latest commit 36ec454

[qodo-free-for-open-source-projects]

1. Settings custom_labels not restored 📘 Rule violation ☼ Reliability

The new test helper mutates the global Dynaconf custom_labels setting but only restores it when
the original value is not None, which can leak state across tests and cause nondeterministic
failures.

Agent Prompt

## Issue description
`TestDescribeManagedLabelNamespace._with_settings()` can change `settings.custom_labels`, but `_restore()` only restores it when the original value is not `None`. If the original value was `None` and the test sets it to `[]` (or another value), that mutated global/module state can leak into subsequent tests.

## Issue Context
This violates the requirement to snapshot and restore mutated global/module state in tests/requests to avoid flakiness.

## Fix Focus Areas
- tests/unittest/test_pr_description_output_core.py[629-646]

## Suggested approach
- Track whether `_with_settings()` actually changed `custom_labels` (e.g., store a boolean) and always restore it.
- If the original was `None`, explicitly restore to `None` (or delete/reset the key in a Dynaconf-safe way) rather than leaving the test-written value in place.

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

[qodo-free-for-open-source-projects]

2. Missing blank-line separation 🐞 Bug ⚙ Maintainability

pr_agent/algo/utils.py defines _describe_managed_label_names() immediately after
set_custom_labels() without the required two blank lines, which violates Ruff’s enabled E
(pycodestyle) rules and can fail linting/CI.

Agent Prompt

### Issue description
Ruff is configured to lint pycodestyle `E` rules, which includes `E305` (expected 2 blank lines after class/function). In `pr_agent/algo/utils.py`, `_describe_managed_label_names()` starts immediately after `set_custom_labels()` with insufficient blank-line separation, which can break CI.

### Issue Context
The repository enables Ruff with `lint.select` including `E`.

### Fix Focus Areas
- pr_agent/algo/utils.py[964-970]
- pyproject.toml[52-75]

### How to fix
Insert the missing blank line(s) so there are **two** blank lines between the end of `set_custom_labels` and the start of `_describe_managed_label_names`.

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

[qodo-free-for-open-source-projects]

Code review by qodo was updated up to the latest commit 3949680