fix: resolve issue #295 - bert-e: pr status should be displayed at all time - #296
matthiasL-scality wants to merge 5 commits into
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| (c for c in pull_request.comments | ||
| if c.author == settings.robot and | ||
| c.text.startswith(STATUS_COMMENT_MARKER)), None) | ||
| if existing is None: |
There was a problem hiding this comment.
Bitbucket Comment has no update, but the check only fails after the first comment is posted. With status_comment: true on Bitbucket, the first call posts a status comment. Every later call then raises NotImplementedError and only logs a warning, so a stale status stays on the PR forever. Check that update is supported before calling add_comment, or reject the setting for Bitbucket when settings load.
— Claude Code
| try: | ||
| _update_status_comment(settings, pull_request, comment) | ||
| except NotImplementedError: | ||
| LOG.warning("Status comment is not supported by this git host") |
There was a problem hiding this comment.
Only NotImplementedError is caught here. An API error from the new status comment (requests.HTTPError from PATCH/POST) will propagate and skip the normal comment and bot status. That means an optional feature can break the main notification path. Catch and log errors from _update_status_comment more broadly, e.g. requests.HTTPError.
— Claude Code
| The pull request description is left untouched: the status lives in a | ||
| dedicated comment, edited in place whenever the state changes. | ||
| """ | ||
| if not getattr(settings, 'status_comment', False) or \ |
There was a problem hiding this comment.
This path skips settings.interactive, which _send_comment honors. In interactive mode, the status comment is posted or edited without asking for confirmation. Return early when settings.interactive is set, or apply the same confirm() check.
— Claude Code
| The pull request description is left untouched: the status lives in a | ||
| dedicated comment, edited in place whenever the state changes. | ||
| """ | ||
| if not getattr(settings, 'status_comment', False) or \ |
There was a problem hiding this comment.
Every message except Init/Help overwrites the status, including informational ones like PendingHotfixVersionReminder from jira.py. The "status" then shows a reminder instead of the real PR state. Only update the status for messages that carry a state, e.g. comment.status is not None, or use an explicit allow-list.
— Claude Code
| comment, (exceptions.InitMessage, exceptions.HelpMessage)): | ||
| return | ||
| text = render_status_comment(comment) | ||
| existing = next( |
There was a problem hiding this comment.
On GitHub, pull_request.comments is not cached: it re-lists every page on each access. notify_user already lists comments again in find_comment, so this doubles the comment API calls per notification. That matters on PRs with many comments and for rate limits. Fetch the list once and reuse it.
— Claude Code
|
|
📋 PR Summary This PR adds a single Bert-E status comment that stays up to date and shows the pull request's current state, so users don't have to dig through step-by-step comments. The comment carries a marker, is edited in place when the host supports that (otherwise the old one is deleted and a new one posted), and is left out of regular comment lookups. The latest changes add an Changes
|
![upsun-dispatch[bot]](https://avatars.githubusercontent.com/in/4153575?s=60&v=4){kind=small}
There was a problem hiding this comment.
Warning
Changes suggested — 🟡 4 warnings · 🔵 1 minor point
🔍 Full review · 6 files reviewed
Verification
_update_status_commentonly matches comments whose author equalssettings.robot, so a user's comment containing the marker cannot hijack the status.- The status comment is skipped when
no_commentis set and when the newstatus_commentsetting is off (its default is False). - The
NotImplementedErrorhandler wraps only the status-comment step, so_send_bot_statusand_send_commentstill run when update is unsupported. send_greetingsis not suppressed, becauseInitMessageis excluded and so the status comment can never come before the greeting.
The new unit tests in bert_e/tests/unit/test_status_comment.py use fake PR and comment classes only. Nothing tests the real GitHub Comment.update, the Bitbucket path (which has no update), or how the status comment interacts with comment dedupe.
Review details
- Commit: 81516bd
- Model: claude-opus-5-5
Review 1 of 10 for this pull request · View the full run
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #296 +/- ##
==========================================
+ Coverage 90.21% 90.39% +0.17%
==========================================
Files 82 83 +1
Lines 11293 11490 +197
==========================================
+ Hits 10188 10386 +198
+ Misses 1105 1104 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Note
Reviewed — No new issues found · 5 still open
🔁 Incremental · 3 files reviewed
Outstanding from earlier reviews:
- 🟡 #4155716296 —
bert_e/workflow/pr_utils.py:130: Bitbucket users see a frozen, wrong status that looks authoritative. — Bitbucket still cannot update the comment (the explicit raise was added in this push), andadd_commentstill does not refresh the cached_comments. - 🟡 #4155716328 —
bert_e/workflow/pr_utils.py:120: The always-current status can show the reply to a command instead of the real state. — The exclusion list is unchanged:StatusReport,UnknownCommandand similar replies still replace the status heading. - 🟡 #4155716342 —
bert_e/workflow/pr_utils.py:137: Duplicate bot comments, the noise this feature is meant to reduce. — The status comment is still created before_send_comment. With the defaultdont_repeat_if_in_history=-1it becomes the newest robot comment, so dedupe misses and the message is posted again. The new tests only exercise the10case. - 🟡 #4155716353 —
bert_e/workflow/pr_utils.py:128: Interactive mode no longer gates every comment Bert-E posts. — The status comment still bypasses thesettings.interactiveconfirm. - 🔵 #4155716359 —
bert_e/git_host/github/__init__.py:1022: Code that later compares created_on on an updated comment would get a string, not a datetime. — Partly fixed:Client.patchnow sends PATCH, butComment.updatestill stores the raw JSON inself.datawithout going throughschema.Comment.
Verification
Client.patchnow callssession.patch. Its only callers areComment.updateandAbstractGitHostObject.update, which is used byPullRequest.decline, and GitHub documents both endpoints as PATCH.BertESession.requestretries on any HTTP method, so moving from POST to PATCH does not skip the retry-on-429/5xx logic.- Bitbucket
Comment.updatenow raisesNotImplementedErrorexplicitly, which matches theexcept NotImplementedErroralready innotify_user.
This push adds unit tests in bert_e/tests/unit/test_status_comment.py. They check that GitHub's update sends PATCH, that Bitbucket's update raises, that a host without update still posts the regular comment, and how dedupe behaves. The dedupe tests only use dont_repeat_if_in_history=10, so no test covers the default -1 path, where the duplicate-comment problem still happens.
Review 2 of 10 for this pull request · View the full run
| if c.author == settings.robot and | ||
| c.text.startswith(STATUS_COMMENT_MARKER)), None) | ||
| if existing is None: | ||
| pull_request.add_comment(text) |
There was a problem hiding this comment.
On Bitbucket the status comment is created here, but every later update() raises NotImplementedError. The comment then stays stuck on the first state forever, and a warning is logged on every notification. That is the stale-status problem this PR is meant to fix.
Check that the host supports updates before creating the comment (e.g. a supports_comment_update attribute on the host/comment class), or skip the feature entirely on hosts that don't.
— Claude Code
| (c for c in pull_request.comments | ||
| if c.author == settings.robot and | ||
| c.text.startswith(STATUS_COMMENT_MARKER)), None) | ||
| if existing is None: |
There was a problem hiding this comment.
This skips the settings.interactive confirmation that _send_comment applies, so in CLI interactive mode the status comment is posted or edited without asking. Return early, or prompt, when settings.interactive is set.
— Claude Code
| comment: exceptions.TemplateException): | ||
| """Notify user by sending a comment or a build status in a pull request.""" | ||
| try: | ||
| _update_status_comment(settings, pull_request, comment) |
There was a problem hiding this comment.
With status_comment on, the first notification posts two comments with the same body: the status comment and the regular one. Every later message is still posted as a regular comment too, so thread noise doesn't go down. Consider skipping the regular comment for messages already shown in the status comment, or at least don't post both on the first notification.
— Claude Code
|
| comment: exceptions.TemplateException): | ||
| """Notify user by sending a comment or a build status in a pull request.""" | ||
| try: | ||
| _update_status_comment(settings, pull_request, comment) |
There was a problem hiding this comment.
The status is only refreshed from notify_user, which only runs for TemplateExceptions. SilentExceptions such as BuildInProgress and NothingToDo never reach this code. So after a conflict is fixed and the build starts, the status comment still says Conflict. That is the stale-error problem issue #295 asks to fix. Also refresh the status when a silent exception is raised (e.g. in bert_e.py / handle_pull_request), or at least when status is set on the exception.
— Claude Code
| existing.update(text) | ||
| except NotImplementedError: | ||
| # no in-place edit on this host: replace the stale comment | ||
| existing.delete() |
There was a problem hiding this comment.
On Bitbucket, update always raises, so every state change deletes the status comment and posts a new one at the bottom, next to the regular comment. That doubles the comments and notifications per event, and the status ends up in the thread instead of near the description. Consider turning the feature off (with a warning) on hosts that cannot edit comments, or add update to Bitbucket (PUT .../comments/{id}).
— Claude Code
|
There was a problem hiding this comment.
Warning
Changes suggested — 🟡 1 warning · 🔵 1 minor point · 1 still open
🔁 Incremental · 4 files reviewed
Outstanding from earlier reviews:
- 🟡 #4155716328 —
bert_e/workflow/pr_utils.py:120: The always-current status can show the reply to a command instead of the real state. — Only StatusReport and UnknownCommand were added to the exclusions; other command replies and informational messages still replace the status.
Verification
- GitHub
Comment.updatenow sends its request throughClient.patch, which callssession.patch, and runs the response throughself.load(..., SCHEMA), socreated_onstays a datetime. - Bitbucket
add_commentsets_comments = None, so thecommentsproperty fetches the list again and a secondnotify_userin the same job finds the status comment. find_commentskips the marker comment for regular lookups, sodont_repeat_if_in_history=-1dedupe is no longer broken by a newly created status comment._update_status_commentnow returns early whensettings.interactiveis set, so the status comment can no longer bypass the confirm() gate.
The diff adds unit tests in bert_e/tests/unit/test_status_comment.py for the interactive skip, the command-reply exclusions, dedupe with -1, Bitbucket cache invalidation, the GitHub update schema and the delete-and-replace fallback. All of them use fakes or mocks. No test covers a Bitbucket DELETE that fails, or send_greetings when a status comment is present.
Review 3 of 10 for this pull request · View the full run
| existing.update(text) | ||
| except NotImplementedError: | ||
| # no in-place edit on this host: replace the stale comment | ||
| existing.delete() |
There was a problem hiding this comment.
🟡 Warning — Silent delete failures leave duplicate, conflicting status comments on Bitbucket PRs.
On Bitbucket, the new fallback calls existing.delete() and then add_comment(text) without checking whether the delete worked. Bitbucket Comment.delete goes through AbstractGitHostObject.delete, which calls client.delete(...) and ignores the response. BertESession.request never calls raise_for_status, so a 403, 404 or 5xx on the DELETE fails silently, and a second status comment is posted anyway. On the next state change, next(...) picks the oldest marker comment (get_comments sorts by created_on), which is the stale one that was never deleted. Its delete fails again, and another status comment is added. Each failed delete leaves one more stale status comment on the PR.
There was a problem hiding this comment.
Warning
Changes suggested — 🟡 1 warning · 2 still open
🔁 Incremental · 2 files reviewed
Outstanding from earlier reviews:
- 🟡 #4155973742 —
bert_e/workflow/pr_utils.py:154: Silent delete failures leave duplicate, conflicting status comments on Bitbucket PRs. — existing.delete() is still unchecked before add_comment, so a failed Bitbucket delete adds another status comment. - 🔵 #4155973750 —
bert_e/workflow/pr_utils.py:46: The greeting can be re-posted on PRs that already have a bot status. — find_comment still skips the status comment when startswith is None, so send_greetings can post InitMessage again.
Verification
InformationExceptioncoversInitMessage,IntegrationDataCreatedandPendingHotfixVersionReminder, so the hotfix reminder no longer replaces the status.MagicMock(spec=cls)in the test helper passesisinstanceagainst the new exclusion tuple, so the parametrized test really exercisesSTATUS_COMMENT_EXCLUDED.- State messages such as Conflict, Queued, BuildFailed, SuccessMessage and QueueBuildFailedMessage are not in the exclusion tuple and still update the status comment.
The parametrized unit test test_status_comment_not_replaced_by_command_replies in bert_e/tests/unit/test_status_comment.py covers the widened exclusion list. Nothing checks that state messages still update the comment, beyond the existing QueueConflict and Conflict tests.
Review 4 of 10 for this pull request · View the full run
| # Replies to user commands and purely informational messages: they do not | ||
| # describe the state of the pull request and must not replace the status. | ||
| STATUS_COMMENT_EXCLUDED = ( | ||
| exceptions.InformationException, |
There was a problem hiding this comment.
🟡 Warning — The status comment never meets the issue's integration-PR listing requirement.
Putting exceptions.InformationException in STATUS_COMMENT_EXCLUDED also leaves out IntegrationDataCreated, which subclasses it (exceptions.py:166). That message is the only template that lists the integration branches with their links and the child pull request numbers (integration_data_created.md). None of the state messages that reach the status comment (Queued, Conflict, BuildFailed, ApprovalRequired…) render those names. So once integration data is created, the status comment never shows the integration pull requests or branches. Issue #295 asks for exactly that ('show the name of each integration pull request and branches'). The new parametrized test also locks the omission in by listing IntegrationDataCreated among the excluded command replies.
| if c.author == settings.robot and | ||
| c.text.startswith(STATUS_COMMENT_MARKER)), None) | ||
| if existing is None: | ||
| pull_request.add_comment(text) |
There was a problem hiding this comment.
The status comment is created on the first non-informational notification, which comes after the init message and possibly other comments. It is not "near the description" as the issue asks.
Consider editing the bot's first comment (the init message) in place, or creating the status comment in send_greetings so it is always the first bot comment.
— Claude Code
| existing.update(text) | ||
| except NotImplementedError: | ||
| # no in-place edit on this host: replace the stale comment | ||
| existing.delete() |
There was a problem hiding this comment.
On Bitbucket this fallback runs on every state change, so the status comment is deleted and re-posted at the bottom of the thread each time. That defeats the "prominent place" goal and spams notifications.
Bitbucket supports editing comments (PUT .../pullrequests/{id}/comments/{comment_id} with {"content": {"raw": ...}}). Implement Comment.update there instead of raising.
— Claude Code
| comment: exceptions.TemplateException): | ||
| """Notify user by sending a comment or a build status in a pull request.""" | ||
| try: | ||
| _update_status_comment(settings, pull_request, comment) |
There was a problem hiding this comment.
The status only refreshes when notify_user is called. Some transitions post nothing, for example a conflict that gets resolved, or a PR waiting on a build. In those cases the status keeps showing the old error, which is the exact problem the issue describes.
Refresh the status at the end of each PR job with the current state, not only as a side effect of notifications.
— Claude Code
| pull_request_id=self['id'] | ||
| ) | ||
| # invalidate the cache so that the new comment is visible | ||
| self._comments = None |
There was a problem hiding this comment.
Clearing the cache on every add_comment means the next pull_request.comments access refetches every page of comments. This applies even when status_comment is off, so it adds Bitbucket API calls on every job.
Append the new comment to self._comments instead, if it is already loaded.
— Claude Code
|
There was a problem hiding this comment.
Note
Reviewed — No new issues found · 2 still open
🔁 Incremental · 3 files reviewed
Outstanding from earlier reviews:
- 🟡 #4155973742 —
bert_e/workflow/pr_utils.py:140: Silent delete failures leave duplicate, conflicting status comments on Bitbucket PRs. — The new try/except only helps when delete raises. BitbucketComment.deletegoes toAbstractGitHostObject.delete(base.py:215), which callsclient.deleteand never checks the response. A 403, 404 or 5xx still passes silently, soadd_commentposts a duplicate status comment. - 🟡 #4156062443 —
bert_e/workflow/pr_utils.py:114: The status comment never meets the issue's integration-PR listing requirement. —InformationExceptionis still inSTATUS_COMMENT_EXCLUDED, soIntegrationDataCreatedand its list of integration PRs and branches never reach the status comment.
Verification
send_greetingsnow passesinclude_status=True, so if the status comment is the only robot comment left, the greeting is not posted again.find_commentstill skips the status comment for the_send_commentdedupe lookup, because that caller leavesinclude_statusat its default of False.- jira.py:222 is the only other
find_commentcaller, and it passes an explicitstartswith, so the new keyword does not change its behaviour. _update_status_commentnow returns early onsettings.interactiveand on everySTATUS_COMMENT_EXCLUDEDclass, includingPendingHotfixVersionReminderthroughInformationException.
This diff adds one unit test, test_find_comment_include_status_finds_status_comment, in bert_e/tests/unit/test_status_comment.py. No test covers the new except Exception branch around existing.delete(), and no test covers a Bitbucket DELETE that fails without raising.
Review 5 of 10 for this pull request · View the full run
| """Notify user by sending a comment or a build status in a pull request.""" | ||
| try: | ||
| _update_status_comment(settings, pull_request, comment) | ||
| except NotImplementedError: |
There was a problem hiding this comment.
Only NotImplementedError is caught here. If the PATCH, POST or DELETE for the status comment fails (e.g. requests.HTTPError on a 403/5xx), the error escapes notify_user before _send_comment runs, so the main notification is lost. The status comment is an optional extra and must not break the main flow. Catch the HTTP error too (add import requests):
| except NotImplementedError: | |
| except (NotImplementedError, requests.HTTPError): | |
| LOG.warning("Could not update the status comment", exc_info=True) |
— Claude Code
| # no in-place edit on this host: replace the stale comment | ||
| try: | ||
| existing.delete() | ||
| except Exception: |
There was a problem hiding this comment.
A bare except Exception hides real bugs, like a broken delete() implementation. Catch the HTTP error only:
| except Exception: | |
| except requests.HTTPError: |
— Claude Code
| f"{comment}") | ||
|
|
||
|
|
||
| def _update_status_comment(settings, pull_request: AbstractPullRequest, |
There was a problem hiding this comment.
The status only changes when a TemplateException goes through notify_user. States raised as SilentException never call it: BuildInProgress, BuildNotStarted, NothingToDo. So after a user fixes a Conflict and the build starts, the status still says Conflict. Issue #295 is about exactly this stale state. Also update the status from the place where the job result is handled (e.g. the handler that catches SilentException), not only from notify_user.
— Claude Code
| except NotImplementedError: | ||
| # no in-place edit on this host: replace the stale comment | ||
| try: | ||
| existing.delete() |
There was a problem hiding this comment.
On Bitbucket there is no update, so each state change deletes the status and posts it again. The comment then jumps to the bottom of the thread, which is the opposite of "near the description", and sends a new notification each time. Either implement update for Bitbucket (PUT on the comment endpoint) or document that status_comment is GitHub-only and skip it when update is not supported.
— Claude Code
|
1 participant
Automated Fix for Issue #295
Issue: bert-e: pr status should be displayed at all time
Description:
TL;DR: authors and reviewers find the status of a pull request fast. This decreases the questions about the status.
Context: Bert-E writes a new comment at each step. These comments mix with the comments of humans and other bots. The status must be asked for and sometines the link to integration branch are hard to find or just not mentionned.
What: the user cannot find the current status of the pull request. Old error comments stay visible after the fix. The user cannot see what is the current status of Bert-e.
How: show the status in a prominent place, as close as possible to the description. Do not change the description. Update the status with the latest state, for example Conflict. Show the name of each integration pull request and branches. If possible, show the build status next to it. For example dependabot changes its message when rebasing.
Done:
the status shows near the description of the pull request
the description of the pull request does not change
the status shows the latest state, for example Conflict, and the name of each integration pull request
optional: the status shows the build status next to the state
Changes Made
This PR was automatically generated by Claude Code based on the issue analysis.
AI Evaluation
Files Modified
[
"bert_e/server/api/pr_handler.py",
"bert_e/git_host/github.py",
"bert_e/workflow/gitwaterflow/branches.py",
"bert_e/server/templates/status_message.py",
"bert_e/lib/templates.py"
]
Tests Created
[
"tests/test_pr_status_display.py",
"tests/test_status_message_updates.py",
"tests/integration/test_github_status_display.py"
]
Generated by n8n automation workflow