feat(azure): preserve suggestion state across reruns - #2724
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
PR Summary by QodoAzure DevOps: preserve /improve suggestion state across reruns
AI Description
Diagram
High-Level Assessment
Files changed (18)
|
Code Review by Qodo
1. Failed update loses output
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
|
Code review by qodo was updated up to the latest commit b9b3f41 |
b9b3f41 to
f879847
Compare
|
The question argument finding is addressed in f879847. The remaining import-order finding is not applicable: both cited files already match the repository’s configured isort output, and |
|
Code review by qodo was updated up to the latest commit f879847 |
|
Code review by qodo was updated up to the latest commit 484a076 |
|
/agentic_review |
|
Code review by qodo was updated up to the latest commit 484a076 |
IsmaelMartinez
left a comment
IsmaelMartinez
left a comment
There was a problem hiding this comment.
Thanks for staying on top of the findings, and for the 484a076 explanation, which I traced and agree with. Four notes inline, one with a suggestion; suite green merged onto main.
| if git_provider._publish_check_run(pr_comment, name): | ||
| return | ||
|
|
||
| if git_provider.supports_code_suggestion_state() and max_previous_comments <= 0: |
There was a problem hiding this comment.
The new boolean is consumed on one path only. max_history_len defaults to 4, so most runs take the history branch, where the return is dropped at line 459 and the progress note is removed regardless. Not a regression, main does the same, but git_provider.py:392 is its only consumer.
| def supports_code_suggestion_state(self) -> bool: | ||
| return True | ||
|
|
||
| def supports_threaded_pr_questions(self) -> bool: | ||
| return True | ||
|
|
||
| def supports_line_question_history(self) -> bool: | ||
| return True |
There was a problem hiding this comment.
Worth saying in the description which of these need the opt-in. Threaded /ask and line-question history are on by default for every Azure user, and main gates the latter on isinstance(self.git_provider, GithubProvider), so Azure gains something new. The reconcile pass is inert without config.persistent_inline_comments, since it only sees threads carrying the marker line 195 writes.
| default_comment_status = "closed" | ||
|
|
||
| [azure_devops_server] | ||
| agent_identity = "" |
There was a problem hiding this comment.
Empty here means discover the identity from thread history, which reads as disabled. configuration.toml is the authoritative listing, so worth a comment.
| agent_identity = "" | |
| agent_identity = "" # empty: the agent's identity is discovered from its own earlier comments on the PR |
| return True | ||
| except Exception as e: | ||
| get_logger().exception(f"Failed to edit comment, error: {e}") | ||
| return False |
There was a problem hiding this comment.
Hard conflict with #2722, which adds raise here. Worth agreeing which way before either merges.
|
I traced the overlap from #2722 and narrowed its scope so we should be able to keep both behaviors. #2722 no longer changes So #2724 can keep ownership of the Azure If #2722 lands first, when rebasing #2724 onto the updated |
IsmaelMartinez
left a comment
IsmaelMartinez
left a comment
There was a problem hiding this comment.
Thanks for the work on this one, and two things before it can move.
It no longer merges: pr_agent/tools/pr_questions.py conflicts with main at eb5b0abf, and GitHub has the branch as dirty.
And the overlap with #2722 is not resolved, despite the note above. #2722's head still adds raise to AzureDevopsProvider.edit_comment() where this PR returns False, so the two still collide on azuredevops_provider.py and git_provider.py. I have said the same on #2722. The cleanest order is this one landing first, since your True/False contract is what makes #2722's guard work at all, after which #2722 drops its Azure change.
4 participants
Problem
Repeated
/improveruns on Azure DevOps can turn one review into a growing collection of duplicate or near-duplicate threads. Developers may apply a suggestion, reject it, defer it to a backlog item, or explain why it is not appropriate, but a later run does not reliably retain that context. The same finding can then return as a new comment.The discussion is also fragmented:
Expected behavior
From a developer's perspective, suggestion reviews should behave like one continuous conversation:
/improverun reviews the pull request and publishes one summary plus relevant inline suggestions/improve -ireviews only changes after the latest suggestions pass and exits without a model call when no commits are newImplementation
This change keeps the behavior scoped to Azure DevOps:
Documentation covers Azure identity configuration, persistent suggestions, discussion context, and incremental
/improveusage.Validation
Live Azure DevOps Services test
Tested end-to-end against a live Azure DevOps Services pull request with 16 commits and 6 changed files, using the same configuration and model as production.
The final test sequence produced these results:
/improverun created one summary thread and four inline suggestion threads./improve -iwithout new commits exited before the model request.Automated checks
126 passedin the focused Azure suggestion, incremental, deduplication, question, and webhook suites2101 passed, 1 skipped, 1 xfailedin the complete unit suitegit diff --checkpass