fix: resolve issue #297 - #298
matthiasL-scality wants to merge 3 commits into
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| """Abstract class defining the interface of a pull requests's comment.""" | ||
|
|
||
| @abstractmethod | ||
| def update(self, text: str) -> None: |
There was a problem hiding this comment.
The name update clashes with the AbstractGitHostObject.update(cls, client, data, ...) classmethod (base.py:199). Bitbucket Comment does not override it, so ABC sees the classmethod and does not flag it as abstract. At runtime existing.update(text) calls the classmethod with client=text and raises TypeError, which notify_user swallows. Result: on Bitbucket the status comment is never updated and every run logs a warning.
Implement update on bitbucket.Comment (PUT {'content': {'raw': text}} to the comment URL), or give the method a name that doesn't clash, like edit.
— Claude Code
| # the last bot comment (see dont_repeat_if_in_history). | ||
| try: | ||
| update_status_comment(settings, pull_request, comment) | ||
| except Exception: |
There was a problem hiding this comment.
Broad except Exception hides real bugs here, such as the Bitbucket update TypeError. Catch the expected failures instead (requests.HTTPError, git_host errors) so that programming errors still surface in tests.
— Claude Code
| status=getattr(comment, 'status', None), | ||
| integration_prs=prs, | ||
| active_options=comment.kwargs.get('active_options')) | ||
| if existing is None: |
There was a problem hiding this comment.
On PRs that already have bot comments, the first run after deploy posts the status comment at the bottom instead of near the top. It then becomes the last bot comment, so for messages with dont_repeat_if_in_history=-1 (the default), find_comment(..., max_history=-1) sees a non-matching comment first, returns None, and the previous message gets posted again. Create the status comment only when the PR has no bot comment yet, or make find_comment skip comments that start with STATUS_COMMENT_MARKER.
— Claude Code
| try: | ||
| # Must happen before the message so that the status comment is never | ||
| # the last bot comment (see dont_repeat_if_in_history). | ||
| try: |
There was a problem hiding this comment.
Every bot notification now adds an extra comment at position 0, but the existing integration tests in test_bert_e.py were not updated. They count comments (assertIs(len(list(pr.get_comments())), 1) at L1354, plus L1390/1400/1551/1590-1613/3536-3604), and test_init_message reads pr.comments[0] expecting the greeting. These should fail in the TestBertE jobs. Update those assertions, and add an integration test that runs through the mock host instead of only FakePR.
— Claude Code
|
![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 — 🟡 5 warnings
🔍 Full review · 7 files reviewed
Verification
- The GitHub
Comment.updateinstance method overrides the inherited classmethod, andclient.patchreturns parsed JSON whosebody/user/id/urlkeys match the properties. STATUS_PR_REuses\u2192in a raw str pattern, whichreinterprets correctly, and the-%}in the template keeps each PR line starting with* #.- The status comment never starts with the bot's message text, so
find_commentdedup withmax_history=None(NEVER_REPEAT messages) is unaffected. handle_commentscommand scanning stops at the newest bot comment; an in-place edit does not change GitHub ordering, so new commands after the last message are still seen.
The PR adds tests: bert_e/tests/test_pr_status_comment.py, which uses a fake PR/comment and never touches a real git host or the Bitbucket client, and bert_e/tests/unit/test_status_template.py. Nothing covers the Bitbucket path or the reposting on PRs that already have bot comments, and the existing comment-count and first-comment assertions in test_bert_e.py were not updated.
Review details
- Commit: 413b17d
- Model: claude-opus-5-5
Review 1 of 10 for this pull request · View the full run
| if child_prs: | ||
| return [{'id': pr.id, 'dst': pr.dst_branch, 'src': pr.src_branch} | ||
| for pr in child_prs] | ||
| if previous_text: |
There was a problem hiding this comment.
🟡 Warning — Stale integration PR links stay pinned and point users at closed PRs.
When the current message has no child_prs, _integration_pull_requests copies the list from the previous status comment's text. Only IntegrationDataCreated ever passes child_prs, and it is sent only when a PR is newly created and there are at least 2 wbranches. So the list never shrinks or refreshes. After /reset, a declined or recreated integration PR, or a PR whose cascade drops to one branch, the comment keeps listing the old PR numbers indefinitely.
| """ | ||
| if settings.no_comment or settings.interactive: | ||
| return | ||
| if getattr(comment, 'status', None) is None: |
There was a problem hiding this comment.
The integration PR list will never show up in production. child_prs is only passed by IntegrationDataCreated (notify_integration_data), which is an InformationException with status = None, so this early return skips it. Every later message with a status has no child_prs, and the previous comment has no PRs to carry over either.
Let messages that carry child_prs through this check (for example if comment.status is None and not comment.kwargs.get('child_prs')), and keep the previous state/status when the message has no status of its own.
— Claude Code
| child = SimpleNamespace(id=7, src_branch='w/1.1/x', | ||
| dst_branch='development/1.1') | ||
| err = _conflict() | ||
| err.kwargs['child_prs'] = [child] |
There was a problem hiding this comment.
This test puts child_prs on an IncompatibleSourceBranchPrefix error, and that never happens in the real flow. It passes even though the real flow never fills in the integration PR list. Test it with exceptions.IntegrationDataCreated instead, or add an integration test in test_bert_e.py that checks the status comment lists the w/* PRs after create_integration_pull_requests.
— Claude Code
| status=getattr(comment, 'status', None), | ||
| integration_prs=prs, | ||
| active_options=comment.kwargs.get('active_options')) | ||
| if existing is None: |
There was a problem hiding this comment.
The comment is created the first time a message has a status, not before the first Bert-E message. On new PRs it lands after the greeting (InitMessage has no status). On PRs that already exist when this is deployed, it lands at the bottom of the thread and stays there, so it is not "pinned" early. Either create it from send_greetings before the InitMessage, or change the PR description and docstring to match what the code does.
— Claude Code
|
There was a problem hiding this comment.
Warning
Changes suggested — 🟡 2 warnings · 2 still open
🔁 Incremental · 7 files reviewed
Outstanding from earlier reviews:
- 🟡 #4163260446 —
bert_e/workflow/pr_utils.py:174: The existing test suite fails and blocks the change. — Several count assertions were changed by +1. I could not run them to confirm each now matches, given which messages create the status comment (status-bearing only). - 🟡 #4163260460 —
bert_e/workflow/pr_utils.py:132: Stale integration PR links stay pinned and point users at closed PRs.
Verification
find_commentnow skips comments starting with STATUS_COMMENT_MARKER, so a status comment added after older bot comments no longer breaks themax_history=-1dedup check.- Bitbucket
Comment.editpasses full_name, pull_request_id and comment_id, which fill every placeholder in GET_URL, the URLupdatefalls back to. - The
editrename is consistent across AbstractComment, the GitHub, Bitbucket and mock implementations, and the test FakeComment;updateis no longer called on comments anywhere. - InitMessage has no status, so the greeting still comes first and
test_init_message'spr.comments[0]check is unaffected.
New unit tests in bert_e/tests/test_pr_status_comment.py cover create/edit, skipping informational messages and the dedup skip. They use FakePR only, and they inject child_prs on a message that never carries them in real code. Changed count assertions in test_bert_e.py run under the tox jobs in .github/workflows/main.yaml. Nothing tests Bitbucket Comment.edit against the real API shape.
Review 2 of 10 for this pull request · View the full run
| """ | ||
| if settings.no_comment or settings.interactive: | ||
| return | ||
| if getattr(comment, 'status', None) is None: |
There was a problem hiding this comment.
🟡 Warning — The status comment never shows integration PRs, which the feature promises.
The new early return skips every message whose status is None. IntegrationDataCreated is an InformationException with no status, and it is the only message that carries child_prs (notify_integration_data in gitwaterflow/integration.py:217-225). So update_status_comment returns before _integration_pull_requests ever sees child_prs. Every status-bearing message has no child_prs and no earlier list to copy, so in production the pinned comment never lists integration PRs, which is half of what the PR sets out to do. test_integration_prs_kept_between_updates hides this by putting child_prs on an IncompatibleSourceBranchPrefix, which never happens in real code.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #298 +/- ##
==========================================
- Coverage 90.21% 89.76% -0.46%
==========================================
Files 82 84 +2
Lines 11293 11430 +137
==========================================
+ Hits 10188 10260 +72
- Misses 1105 1170 +65
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.
Warning
Changes suggested — 🟡 1 warning · 2 still open
🔁 Incremental · 3 files reviewed
Outstanding from earlier reviews:
- 🟡 #4163260460 —
bert_e/workflow/pr_utils.py:136: Stale integration PR links stay pinned and point users at closed PRs. —_integration_pull_requestsstill copies the earlier list whenever a message has no child_prs, so declined or recreated integration PRs stay listed. - 🟡 #4164452676 —
bert_e/workflow/pr_utils.py:151: The status comment never shows integration PRs, which the feature promises. — Partly fixed: integration PRs show up only if a status comment already exists, which a clean PR does not have when IntegrationDataCreated is sent.
Verification
- Bitbucket
Comment.editnow PUTs{'content': {'raw': text}}to GET_URL andload(response)callsraise_for_status, so failures surface as HTTPError, which notify_user catches. STATUS_STATE_REandSTATUS_RESULT_REmatch the## Bert-E status:andResult: **...**lines the template renders, so state and status carry over on an integration-only update.- Passing
code=Nonedoes nothing harmful: pr_status_comment.md never referencescode. find_commentnow skips comments that start with STATUS_COMMENT_MARKER, so a status comment appended at the bottom no longer breaks themax_history=-1duplicate check.
The changed test_integration_prs_kept_between_updates in bert_e/tests/test_pr_status_comment.py now uses a real IntegrationDataCreated, but it seeds a status comment first. No test covers integration data that arrives before any status comment, and none covers Bitbucket Comment.edit.
Review 3 of 10 for this pull request · View the full run
| if status is None: | ||
| # only the integration pull requests changed: keep the state | ||
| if existing is None: | ||
| return |
There was a problem hiding this comment.
🟡 Warning — Integration PR list missing on most PRs, defeating half the feature.
The new if existing is None: return throws away the child_prs of an IntegrationDataCreated message whenever no status comment exists yet. That is the normal case on a clean PR. InitMessage carries no status, so the first run creates no status comment. When every check passes, the next message is notify_integration_data (gitwaterflow/init.py:198-200), which hits this return. Later status messages such as ApprovalRequired have no child_prs, and there is no earlier comment text for _integration_pull_requests to copy from, so the status comment gets created with no integration PR list. IntegrationDataCreated is sent only when a w/ branch or PR is newly created, so later runs never recover the list. The list therefore appears only on PRs that hit a status-bearing error before their integration PRs were created. test_integration_prs_kept_between_updates hides this because it calls update_status_comment(..., _conflict()) first to seed a status comment. Fix: create the comment here with a neutral state instead of returning.
| if status is None: | ||
| # only the integration pull requests changed: keep the state | ||
| if existing is None: | ||
| return |
There was a problem hiding this comment.
On a fresh PR with automatic integration branch creation, IntegrationDataCreated is the first message that matters, and it has no status. No status comment exists yet, so this returns and the child PR list is lost. IntegrationDataCreated is sent only when the branches are created, and later messages have no child_prs, so the integration PRs are never shown in the common path. Create the comment here instead of returning, with a neutral state such as comment.title and no result, and add a test that sends IntegrationDataCreated with no existing status comment.
— Claude Code
| # informational messages (greetings, help...) do not change the | ||
| # state of the pull request | ||
| return | ||
| existing = find_status_comment(pull_request, settings.robot) |
There was a problem hiding this comment.
On Bitbucket, PullRequest.comments is cached in _comments and add_comment does not update that cache. If one job calls notify_user twice with a status (e.g. IntegrationDataCreated then BuildNotStarted, once the issue above is fixed), find_status_comment won't see the comment it just created and posts a second one. The mock does not cache, so the tests miss this. Clear or append to _comments after adding the status comment.
— Claude Code
| def id(self) -> int: | ||
| return self.data['id'] | ||
|
|
||
| def edit(self, text: str) -> None: |
There was a problem hiding this comment.
The new edit methods for GitHub and Bitbucket have no tests. Only the mock and FakeComment are tested. Add a unit test that checks the request (method, URL, payload) and that self.data gets updated.
— Claude Code
|
2 participants
Maintain a single bot-owned status comment, edited in place (latest state + integration PRs), created before the first Bert-E message. Adds AbstractComment.update (GitHub + mock).