Skip to content

fix(azure): render /improve suggestions as a diff block - #2718

Open
IsmaelMartinez wants to merge 1 commit into
The-PR-Agent:mainfrom
IsmaelMartinez:fix/azure-improve-suggestion-fence
Open

fix(azure): render /improve suggestions as a diff block#2718
IsmaelMartinez wants to merge 1 commit into
The-PR-Agent:mainfrom
IsmaelMartinez:fix/azure-improve-suggestion-fence

Conversation

@IsmaelMartinez

Copy link
Copy Markdown
Collaborator

Azure DevOps has no committable suggestion blocks, but /improve emits GitHub's suggestion fence unconditionally and the Azure provider posts that body verbatim, so it renders as a raw uneditable block. Every other platform without native support rewrites the fence first: both Bitbucket providers swap in a diff block, GitLab rewrites the info string, Gerrit special-cases it.

This applies the Bitbucket treatment to Azure, including the PR-level fallback path. When original_suggestion is missing or unusable the body is left exactly as it is today. Four regression tests, two of which fail on main.

Refs #2110.

Azure DevOps has no committable suggestion blocks, so the fence /improve emits was
published verbatim and rendered as a raw, uneditable block. Rewrite it as a diff
block, matching what the Bitbucket providers already do.

Refs: The-PR-Agent#2110

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@qodo-code-review

Copy link
Copy Markdown
Contributor

PR Summary by Qodo

Render Azure /improve Suggestions as Diff Blocks

🐞 Bug fix 🧪 Tests 🕐 10-20 Minutes

Grey Divider

AI Description

• Render Azure /improve suggestion fences as readable unified diff blocks.
• Preserve existing bodies when source suggestion data is absent or unusable.
• Cover inline and PR-level fallback publishing with regression tests.
Diagram

graph TD
  A["Suggestion payload"] --> B{"Original code?"} -->|Yes| C["Diff renderer"] --> D{"Anchor found?"} -->|Yes| E["Inline comment"]
  B -->|No| D
  D -->|No| F["PR fallback"]
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Shared suggestion-diff utility
  • ➕ Eliminates duplicated conversion logic across Azure and Bitbucket providers.
  • ➕ Provides one location for fence parsing, error handling, and future tests.
  • ➖ Broadens a focused Azure bug fix into a cross-provider refactor.
  • ➖ Could alter established behavior on providers that already render suggestions correctly.

Recommendation: Keep the provider-local implementation for this targeted fix because it matches proven Bitbucket behavior and limits regression risk. Consider extracting a shared renderer separately if additional providers require the same conversion.

Files changed (2) +79 / -0

Bug fix (1) +25 / -0
azuredevops_provider.pyRender Azure suggestion fences as unified diffs +25/-0

Render Azure suggestion fences as unified diffs

• Adds a defensive helper that converts supported suggestion fences into unified diff blocks using the original and improved code. The converted body is used for both inline threads and PR-level fallbacks, while missing or unusable source data preserves existing behavior.

pr_agent/git_providers/azuredevops_provider.py

Tests (1) +54 / -0
test_azure_devops_provider.pyAdd Azure suggestion-fence regression coverage +54/-0

Add Azure suggestion-fence regression coverage

• Adds four tests covering successful diff rendering, unchanged bodies without source data, PR-level fallback rendering, and graceful handling of malformed original suggestions.

tests/unittest/test_azure_devops_provider.py

@qodo-code-review

Copy link
Copy Markdown
Contributor

Code Review by Qodo

🐞 Bugs (3) 📘 Rule violations (3) 📜 Skill insights (0)

Grey Divider


Action required

1. Diff conversion drops changed lines 🐞 Bug ≡ Correctness
Description
The renderer removes the first five splitlines() entries, but existing_code and improved_code
are each forced to end in a newline and then split on \n, so difflib.unified_diff includes a
sixth blank-context line after the -/+ lines. Consequently the slice keeps only that blank line
and Azure receives an empty diff block instead of showing the old and new code.
Code

pr_agent/git_providers/azuredevops_provider.py[R121-122]

+            patch = "\n".join(patch_orig.splitlines()[5:]).strip('\n')
+            diff_code = f"\n\n```diff\n{patch.rstrip()}\n```"
Relevance

●●● Strong

This is a deterministic diff-generation bug, and the PR’s regression test explicitly requires
preserved removed and added lines.

PR-#2137
PR-#2467

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The producer passes improved_code.rstrip() into a body with a trailing newline and the renderer
independently appends a newline to both snippets before calling split('\n'). The new regression
test explicitly requires the removed and added lines to appear, but the five-line slice is followed
by the blank context entry produced by those trailing elements.

pr_agent/tools/pr_code_suggestions.py[657-671]
pr_agent/git_providers/azuredevops_provider.py[116-122]
tests/unittest/test_azure_devops_provider.py[482-492]

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

## Issue description
The generated unified diff contains a trailing empty-line context entry because both snippets are appended with `\n` and split using `split('\\n')`. Slicing from index 5 therefore discards the `-` and `+` lines, producing an empty diff block.

## Issue Context
The regression test expects `-values = []` and `+values = set()` in the Azure comment, but the current slicing removes them. Build the patch from the actual unified-diff hunk lines (or change the input splitting/slicing) while preserving the existing fallback behavior when the original suggestion is unusable.

## Fix Focus Areas
- pr_agent/git_providers/azuredevops_provider.py[116-123]
- tests/unittest/test_azure_devops_provider.py[482-492]

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



Remediation recommended

2. Document Azure diff rendering 📘 Rule violation ⚙ Maintainability
Description
The production change alters the user-visible /improve suggestion format on Azure DevOps from a
raw suggestion fence to a diff block, but the PR adds no corresponding README or documentation
update explaining this behavior.
Code

pr_agent/git_providers/azuredevops_provider.py[R122-123]

+            diff_code = f"\n\n```diff\n{patch.rstrip()}\n```"
+            return re.sub(r'```suggestion.*?```', diff_code, body, flags=re.DOTALL)
Relevance

●●● Strong

Recent documentation reviews accepted updates clarifying user-visible tool behavior and usage
expectations.

PR-#2547
PR-#2491

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The changed code replaces the published suggestion fence with a diff fence for Azure output,
which is a user-visible output-format change. The changed-file set contains no README or docs
update, while the existing README documents /improve as producing actionable suggestions.

Rule 2694680: Update docs when user-facing behavior changes
pr_agent/git_providers/azuredevops_provider.py[122-123]
README.md[229-229]

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

## Issue description
Document that Azure DevOps `/improve` suggestions are rendered as diff blocks because Azure lacks committable suggestion fences.

## Issue Context
The output format is user-visible and now differs from the emitted GitHub suggestion fence; preserve the fallback behavior for missing or unusable original suggestions in the documentation.

## Fix Focus Areas
- pr_agent/git_providers/azuredevops_provider.py[109-123]
- README.md[229-229]

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


3. Multiple fences get duplicated 🐞 Bug ≡ Correctness
Description
re.sub replaces every  `suggestion ... `  match with the same diff derived from one
original_suggestion, so a body containing multiple suggestion fences gets duplicate copies of one
change rather than preserving each suggestion's content. This can produce misleading inline and
PR-level comments for any caller that supplies a body with more than one fence.
Code

pr_agent/git_providers/azuredevops_provider.py[123]

+            return re.sub(r'```suggestion.*?```', diff_code, body, flags=re.DOTALL)
Relevance

●● Moderate

Potential multi-fence duplication is semantic and lacks a closely matching historical decision.

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The renderer receives one original_suggestion but applies re.sub over the whole body; the
resulting body is then assigned back to the suggestion before both inline publication and fallback
collection.

pr_agent/git_providers/azuredevops_provider.py[109-123]
pr_agent/git_providers/azuredevops_provider.py[199-200]
pr_agent/git_providers/azuredevops_provider.py[204-240]

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

## Issue description
The global regular-expression replacement applies one source diff to every suggestion fence in the body, duplicating the same change when multiple fences are present.

## Issue Context
`publish_code_suggestions` accepts the entire body and uses the rendered value for both inline and fallback comments. Either constrain the transformation to the single expected fence or validate/handle multiple fences without reusing one diff for unrelated blocks.

## Fix Focus Areas
- pr_agent/git_providers/azuredevops_provider.py[109-123]
- pr_agent/git_providers/azuredevops_provider.py[168-200]

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


4. Embedded fences break diff rendering 🐞 Bug ≡ Correctness
Description
The generated diff payload is interpolated inside a literal triple-backtick fence without protecting
triple-backtick sequences present in the suggested code. A suggestion for a Markdown file or code
containing such a sequence can terminate the diff block early, leaving the remainder rendered as
unrelated Markdown and making the displayed change incorrect.
Code

pr_agent/git_providers/azuredevops_provider.py[R121-123]

+            patch = "\n".join(patch_orig.splitlines()[5:]).strip('\n')
+            diff_code = f"\n\n```diff\n{patch.rstrip()}\n```"
+            return re.sub(r'```suggestion.*?```', diff_code, body, flags=re.DOTALL)
Relevance

●● Moderate

Potential Markdown edge case lacks a close repository precedent establishing acceptance or
rejection.

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
Both source snippets are copied directly into the unified diff and then interpolated into
\n\n`diff\n...\n```, with no escaping or delimiter-length calculation. The producer carries the
model-provided existing_code and improved_code unchanged in original_suggestion, and Azure
publishes the resulting body in both paths.

pr_agent/git_providers/azuredevops_provider.py[116-123]
pr_agent/tools/pr_code_suggestions.py[626-629]
pr_agent/tools/pr_code_suggestions.py[650-671]

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

## Issue description
Suggested code is inserted verbatim into a triple-backtick `diff` block, so any embedded triple-backtick sequence closes the block prematurely.

## Issue Context
The original and improved snippets come from model-produced suggestion data and are published in Azure comments. Escape embedded fences or choose a fence delimiter longer than any payload sequence before constructing the Markdown block.

## Fix Focus Areas
- pr_agent/git_providers/azuredevops_provider.py[116-123]
- pr_agent/tools/pr_code_suggestions.py[626-671]

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



Informational

5. Use double-quoted literals 📘 Rule violation ⚙ Maintainability
Description
The new Azure suggestion renderer introduces single-quoted string literals for dictionary keys and
newline delimiters, contrary to the repository's required Python string-literal convention.
Code

pr_agent/git_providers/azuredevops_provider.py[R116-119]

+            existing_code = original_suggestion['existing_code'].rstrip() + "\n"
+            improved_code = original_suggestion['improved_code'].rstrip() + "\n"
+            diff = difflib.unified_diff(existing_code.split('\n'),
+                                        improved_code.split('\n'), n=999)
Relevance

● Weak

Recent review rejected essentially identical requests to convert touched single-quoted literals to
double quotes.

PR-#2600
PR-#2599

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The checklist requires all newly changed Python string literals to use double quotes. The renderer
adds single-quoted keys and \n delimiters in the cited lines; the same pattern also appears in the
adjacent regular expression.

Rule 2694657: Use double quotes for all Python string literals
pr_agent/git_providers/azuredevops_provider.py[116-123]

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

## Issue description
The changed renderer uses single-quoted Python string literals, violating the project's double-quote convention.

## Issue Context
Convert the new literals without changing the suggestion rendering behavior, including dictionary keys and newline splitting.

## Fix Focus Areas
- pr_agent/git_providers/azuredevops_provider.py[116-123]

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


6. Non-imperative renderer docstring 📘 Rule violation ⚙ Maintainability
Description
The new _render_suggestion_as_diff docstring begins with the descriptive statement `Azure DevOps
has...` rather than an imperative description of the method's behavior. This violates the required
imperative phrasing for newly added function docstrings.
Code

pr_agent/git_providers/azuredevops_provider.py[R110-112]

+        """Azure DevOps has no committable suggestion blocks, so a ```suggestion fence is
+        published verbatim and renders as an uneditable raw block. Replace it with a diff
+        block, the same way the Bitbucket providers do."""
Relevance

● Weak

Recent same-rule precedent rejected an imperative-docstring complaint in PR #2661.

PR-#2661

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The changed method docstring starts with Azure DevOps has no committable suggestion blocks and
then describes what is done, rather than issuing an imperative instruction such as Render... or
Replace....

Rule 2694688: Docstrings and comments must use imperative phrasing
pr_agent/git_providers/azuredevops_provider.py[109-112]

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

## Issue description
Rewrite the new method docstring so its first behavior description uses imperative phrasing.

## Issue Context
The project convention requires newly added function or method docstrings to begin with an imperative verb such as `Render` or `Replace`.

## Fix Focus Areas
- pr_agent/git_providers/azuredevops_provider.py[109-112]

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

Grey Divider

Context sources
✅ Compliance rules (platform): 34 rules
Review mode: 🚀 Fast: This is a localized Azure suggestion-rendering change with contained fallback behavior and regression tests, avoiding security, API, schema, and other high-risk paths.
ⓘ  1 issues published inline · 6 in summary

Grey Divider

Tip of the day
💡 Did you know, you can show, collapse, or hide each part of a finding: code, evidence, and all

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Grey Divider

Qodo Logo

Comment on lines +121 to +122
patch = "\n".join(patch_orig.splitlines()[5:]).strip('\n')
diff_code = f"\n\n```diff\n{patch.rstrip()}\n```"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Action required

4. Diff conversion drops changed lines 🐞 Bug ≡ Correctness

The renderer removes the first five splitlines() entries, but existing_code and improved_code
are each forced to end in a newline and then split on \n, so difflib.unified_diff includes a
sixth blank-context line after the -/+ lines. Consequently the slice keeps only that blank line
and Azure receives an empty diff block instead of showing the old and new code.
Agent Prompt
## Issue description
The generated unified diff contains a trailing empty-line context entry because both snippets are appended with `\n` and split using `split('\\n')`. Slicing from index 5 therefore discards the `-` and `+` lines, producing an empty diff block.

## Issue Context
The regression test expects `-values = []` and `+values = set()` in the Azure comment, but the current slicing removes them. Build the patch from the actual unified-diff hunk lines (or change the input splitting/slicing) while preserving the existing fallback behavior when the original suggestion is unusable.

## Fix Focus Areas
- pr_agent/git_providers/azuredevops_provider.py[116-123]
- tests/unittest/test_azure_devops_provider.py[482-492]

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

@IsmaelMartinez

Copy link
Copy Markdown
Collaborator Author

Hi @naorpeled, last of this batch, ready for review. Azure DevOps has no committable suggestions, but /improve emits GitHub's fence anyway and it renders as a raw uneditable block; this applies the same rewrite Bitbucket already does.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant