fix: resolve issue #293 - Reduce verbosity - #294
matthiasL-scality wants to merge 1 commit into
Conversation
|
📋 PR Summary Transient status messages (history mismatch, conflict, approval required, reset, integration data created, and similar) are now marked 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 — 🟡 3 warnings · 🔵 1 minor point
🔍 Full review · 7 files reviewed
Verification
- Edits only happen when the PR's last comment is Bert-E's own marked comment, so
handle_comments(which reads commands after Bert-E's last message) never sees an already-handled command again. - For updatable messages, the dedup in
find_commentstill matches, because the stored text begins with the original message and the marker is only appended after it. _send_bot_statususesstr(comment)without the marker, so check-run summaries are unchanged.jsonis already imported in github/init.py, andclient.patchreturns a decoded dict, which is assigned toComment.dataand still providesbody,urlanduser.
The new bert_e/tests/unit/test_comment_update.py tests _send_comment/notify_user against fake PR and comment objects, and test_bert_e.py was adjusted for the mock host. Both run under the tox jobs in .github/workflows/main.yaml. No test covers the GitHub Comment.update call or Bitbucket's fallback path.
Review details
- Commit: 25cfcdb
- Model: claude-opus-5-5
Review 1 of 10 for this pull request · View the full run
| return | ||
|
|
||
| if updatable: | ||
| msg = f'{msg}\n{STATUS_MARKER}' |
There was a problem hiding this comment.
🟡 Warning — Bitbucket users see stray HTML markup in every Bert-E status comment.
The <!-- bert-e-status --> marker is added to every updatable message before the code knows whether the host can edit comments. On Bitbucket, bitbucket.Comment has no update, so previous.update raises NotImplementedError and the message is posted with add_comment(msg), marker included. Bitbucket Cloud markdown does not support raw HTML, so the marker shows up as literal text at the bottom of every status comment (history mismatch, conflict, approval required, and so on). Bitbucket gets nothing in return, because it still posts a new comment each time.
|
|
||
| class IntegrationDataCreated(InformationException): | ||
| code = 121 | ||
| updatable = True |
There was a problem hiding this comment.
🟡 Warning — Users lose the list of integration branches and PRs Bert-E created.
IntegrationDataCreated is marked updatable, but it is sent from notify_integration_data just before check_approvals and check_build_status run in the same handle. Those almost always raise ApprovalRequired, BuildFailed or AfterPullRequest, which are also updatable, and they immediately overwrite it. So the one message listing the newly created integration branches and child PRs is erased within the same run. That is why the PR had to drop the 'Integration data created' assertion in test_creation_integration_branch_by_approve. The message is informational (NEVER_REPEAT), not a transient status, and it is only sent when branches are newly created, so it will never be shown again.
| "The status comment is already up to date.") | ||
| try: | ||
| LOG.debug('UPDATING MESSAGE %s', msg) | ||
| previous.update(msg) |
There was a problem hiding this comment.
🟡 Warning — Authors silently miss build failures and conflicts that block their PR.
When the previous status is the last comment, a new status such as BuildFailed, Conflict or QueueConflict replaces ApprovalRequired through a comment edit. GitHub sends no notification or email for an edited comment, so the PR author and subscribers are no longer told when the PR moves into a failure state. Before this change, each status was a new comment and triggered a notification.
| self.client.delete(self.data['url']) | ||
|
|
||
| def update(self, msg: str) -> None: | ||
| self.data = self.client.patch( |
There was a problem hiding this comment.
🔵 Minor — A failed edit aborts the job instead of falling back to a new comment.
Comment.update calls client.patch. That method actually sends session.post (existing code at line 272), so the edit only works if GitHub keeps accepting POST in place of PATCH. Any HTTP error from it (for example a 404 because the comment was deleted after pull_request.comments was read, or a 403 or 422) is a requests.HTTPError, not NotImplementedError. _send_comment only catches NotImplementedError, so the error escapes notify_user and aborts the job instead of falling back to add_comment. No test covers the GitHub implementation; only the mock and fakes are tested.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| self.client.delete(self.data['url']) | ||
|
|
||
| def update(self, msg: str) -> None: | ||
| self.data = self.client.patch( |
There was a problem hiding this comment.
client.patch() uses self.session.post() (copy-pasted from the post method) instead of self.session.patch(). This is a pre-existing bug, but Comment.update is the first caller of client.patch(), so editing comments will send HTTP POST instead of PATCH to GitHub's API — which will fail at runtime.
The error propagates as requests.HTTPError, which is not caught by the except NotImplementedError in _send_comment, so the workflow will crash instead of gracefully falling back to creating a new comment.suggestion<br> self.data = self.client.patch(<br>
You also need to fix client.patch() itself (line 272 of this file) to use self.session.patch(...) instead of self.session.post(...).
— Claude Code
| LOG.debug('UPDATING MESSAGE %s', msg) | ||
| previous.update(msg) | ||
| return | ||
| except NotImplementedError: |
There was a problem hiding this comment.
The except NotImplementedError is too narrow. If the GitHub API call fails (e.g. 403, 404, rate-limited), requests.HTTPError is raised, which will not be caught here — crashing notify_user instead of falling back to a new comment.
```suggestion
except Exception:
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #294 +/- ##
==========================================
+ Coverage 90.21% 90.29% +0.08%
==========================================
Files 82 83 +1
Lines 11293 11418 +125
==========================================
+ Hits 10188 10310 +122
- Misses 1105 1108 +3
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:
|
| "The status comment is already up to date.") | ||
| try: | ||
| LOG.debug('UPDATING MESSAGE %s', msg) | ||
| previous.update(msg) |
There was a problem hiding this comment.
GitHub does not send notifications when a comment is edited. With this change, a PR author will no longer be notified when the status moves to Build failed, Conflict, History mismatch or Waiting for approval, which are the statuses they need to act on.
Consider editing in place only when the new status has the same class as the previous one, or keep the actionable failure statuses as new comments.
— Claude Code
|
|
||
| class IntegrationDataCreated(InformationException): | ||
| code = 121 | ||
| updatable = True |
There was a problem hiding this comment.
IntegrationDataCreated is followed right away by another status in the same run (for example ApprovalRequired), so the list of created integration branches and PRs gets overwritten before anyone sees it. The updated test_creation_integration_branch_by_approve shows this: the assertion for 'Integration data created' was removed. Keep this message as its own comment.
| updatable = True | |
| updatable = False |
— Claude Code
| return | ||
|
|
||
| if updatable: | ||
| msg = f'{msg}\n{STATUS_MARKER}' |
There was a problem hiding this comment.
The marker is added even when the host cannot edit comments. Bitbucket's Comment has no update(), so it always gets a new comment, but that comment still carries the <!-- bert-e-status --> marker. Bitbucket may not hide raw HTML comments the way GitHub does. Add the marker only when the host supports editing, for example with a capability flag on the comment or PR class.
— Claude Code
|
1 participant
Automated Fix for Issue #293
Issue: Reduce verbosity
Description:
When Bert-e runs, it publishes a new message with the updated status : “History conflict”, “mismatched version”, “reset”, “integration branch created”….
When the PR is updated, this creates a large number of comments, most of whose are outdated and thus not useful.
→ it would be better to keep updating the same comment, so that it displays the relevant status only. Github allows viewing earlier versions of a comment, so nothing will be lost anyway.
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/server/api/comment_manager.py",
"bert_e/jobs/merge_job.py"
]
Tests Created
[
"tests/test_comment_update.py",
"tests/test_pr_status_message.py",
"tests/integration/test_comment_verbosity.py"
]
Generated by n8n automation workflow