fix: resolve issue #299 - bert-e: pr status should be displayed at all time - #301
matthiasL-scality wants to merge 7 commits into
Conversation
Maintain a single status comment per pull request, edited in place, showing the latest state, the integration branches / pull requests and their build status.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| except messages.TemplateException as err: | ||
| # The status comment goes first: the message must stay the most | ||
| # recent comment of the pull request. | ||
| publish_status(job, err) |
There was a problem hiding this comment.
If publish_status raises anything outside EXPECTED_ERRORS (Jinja error, KeyError, the assert in render_status, an error from _check_approvals_status), that error replaces err. notify_user is then skipped and the user never gets the real message. Wrap this call (and the one on line 69) so a failed status update can't hide the original exception, for example by catching Exception and logging it inside publish_status, which matches its "never fatal" docstring.
— Claude Code
| msg = render('pr_status.md', icon=state.icon, label=state.label, | ||
| code=state.code, integration=integration, status=report, | ||
| active_options=job.active_options) | ||
| assert msg.startswith(STATUS_COMMENT_HEADER) |
There was a problem hiding this comment.
Production code shouldn't depend on assert: it is stripped under -O, and when it fails it raises AssertionError, which publish_status doesn't catch (see the comment on handle_pull_request). Log and return instead, or drop the check since a test already covers the template header.
— Claude Code
| pr = prs.get(branch.name) | ||
| build = None | ||
| if key: | ||
| build = job.project_repo.get_build_status( |
There was a problem hiding this comment.
This calls get_build_status once per integration branch, then _build_status_report → _check_builds_status calls it again for the same commits. It also re-clones and runs build_branch_cascade and the approvals checks. All of this happens on every run, including BuildInProgress, which fires on each build webhook. On GitHub, find_status_comment also re-lists all comments each time. That is a lot of extra API calls against rate limits. Fetch the build status once and share it between the table and the checklist, or skip the integration/report refresh for BuildInProgress/BuildNotStarted.
— Claude Code
|
|
📋 PR Summary The PR adds one persistent status comment to each pull request. Bert-E edits this comment in place to show the current state (for example Conflict or Waiting) along with the integration branches and pull requests, and the PR description stays unchanged. To support this, comment editing is added to the git_host layer for GitHub, Bitbucket and the mock host, and the PR also adds a template, settings, documentation and tests. The latest push changes how silent states (exceptions that don't produce their own comment) are matched: the lookup now uses isinstance, so subclasses such as WaitOptionSet also replace the previous state. A test covers this case. 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 · 19 files reviewed
Verification
find_commentand the command scan inhandle_commentsskip the robot's status comment, so thedont_repeat_if_in_historycheck and command detection still see the real last message.- Bitbucket
add_commentappends to the non-empty_commentscache, andeditupdatesdata['content']['raw'], sofind_status_commentdoes not create a duplicate within one run. - GitHub
commentsre-fetches on every access, so a status comment created earlier in the run is found again without needing a cache. render_statusskips cloning, integration rows and the checklist for final states (Merged, Partially merged, Declined).
New unit tests (test_pr_status_comment.py, test_status_template.py, test_github_bitbucket_edit_comment.py), an end-to-end test_status_comment in test_bert_e.py and test_pull_request_edit_comment in test_git_host.py cover the new code, and the tox jobs in .github/workflows/main.yaml run them. No test covers command-answer exceptions (e.g. UnknownCommand) overwriting the state, or an unexpected error inside publish_status.
Review details
- Commit: 38a739a
- 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 #301 +/- ##
==========================================
+ Coverage 90.21% 90.82% +0.61%
==========================================
Files 82 86 +4
Lines 11293 11851 +558
==========================================
+ Hits 10188 10764 +576
+ Misses 1105 1087 -18
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 · 1 still open
🔁 Incremental · 3 files reviewed
Outstanding from earlier reviews:
- 🔵 #4167470304 —
bert_e/workflow/gitwaterflow/status_comment.py:148: Doubles API/git work per run and increases rate-limit pressure on busy repos. — The cache only removes the duplicateget_build_statuscalls. The rest of the per-run API and git work remains on every build webhook.
Verification
- UnknownCommand, NotEnoughCredentials, IncorrectCommandSyntax, IncorrectPullRequestNumber, FlakyGitHost and NotAuthor now set
reports_state = False, soState.from_exceptionreturns None for them. - The
except TemplateExceptionpath inhandle_pull_requestcan no longer losenotify_user:publish_statusnow catchesExceptionaround rendering and upsert. render_statusrestoresget_build_statusin afinallywithdel. This is safe because the host Repository classes define it as a class method, not an instance attribute.render_statusnow logs a missing header instead of asserting.
This push adds test_publish_never_raises_on_unexpected_errors, test_publish_ignores_command_answers and test_publish_does_not_comment_old_declined_pull_requests in bert_e/tests/unit/test_pr_status_comment.py. No test covers the build-status cache sharing, or a failure inside the new declined guard's find_status_comment call.
Review 2 of 10 for this pull request · View the full run
|
|
||
| early_checks(job) | ||
| send_greetings(job) | ||
| ensure_status_comment(job) |
There was a problem hiding this comment.
The status comment is created here as Being analyzed, but NothingToDo (raised below when dst.includes_commit(src) or the source branch was deleted) is not handled in handle_pull_request. The comment then stays Being analyzed forever, for example on a PR that was already merged when this feature is rolled out.
Map NothingToDo to a state in SILENT_STATES (only when a status comment already exists, as done for PullRequestDeclined), or create the comment only once a real state is known.
— Claude Code
| pull_request.add_comment(msg) | ||
| elif comment.text != msg: | ||
| LOG.debug('UPDATING STATUS COMMENT %s', msg) | ||
| comment.edit(msg) |
There was a problem hiding this comment.
On GitHub, each status comment create/edit sends an issue_comment webhook, and handle_github_issue_comment turns it into a new PullRequestJob without checking the author or the action. Most runs now end with an edit, so each one starts another full run (clone, cascade, API calls). It stops only when the text no longer changes, and build statuses keep changing it.
In bert_e/server/webhook.py, ignore issue_comment events written by the robot (at least edited ones).
— Claude Code
| repo.get_build_status = cached_build_status | ||
| try: | ||
| integration = _integration_rows(job) | ||
| report = _build_status_report(job) |
There was a problem hiding this comment.
This runs on every TemplateException path, so each failing run now does a second clone check and cascade build (_build_status_report calls build_branch_cascade again) plus one PR list, approvals and one build status call per integration branch. On large repos this adds a lot of git host API usage, which already hits rate limits.
Reuse job.git.cascade when it is already built, and skip the checklist for states where it adds nothing (for example Conflict).
— Claude Code
|
There was a problem hiding this comment.
Warning
Changes suggested — 🟡 2 warnings · 🔵 1 minor point
🔁 Incremental · 4 files reviewed
Verification
publish_statusnow runs theUPDATE_ONLYfind_status_comment lookup inside the try, so host errors there are logged instead of escaping.render_statuswrapsrepo.get_build_statuswith a per-call cache and restores the original infinally, so the table and checklist share build fetches.settings.robotis aUserDictwhose__eq__matches a string against username or account_id, so the webhook login comparison works.- Command-answer exceptions and
FlakyGitHostsetreports_state = False, so they no longer overwrite the status comment.
Unit tests in bert_e/tests/unit/test_pr_status_comment.py were updated: they now treat NothingToDo as a state and check that handle_pull_request publishes it. No test covers the queued-PR NothingToDo path, the wait path, or the new webhook robot filter.
Review 3 of 10 for this pull request · View the full run
| notify_user(job.settings, job.pull_request, err) | ||
| raise | ||
| except (messages.BuildInProgress, messages.BuildNotStarted, | ||
| messages.PullRequestDeclined, messages.NothingToDo) as err: |
There was a problem hiding this comment.
Publishing NothingToDo overwrites final states. After a queue merge sets the status to "Merged", the next run on the PR (for example the close webhook) hits early_checks, which raises NothingToDo("The pull request is 'MERGED'"), or hits dst.includes_commit(src). Both replace "Merged" with "Nothing to do". The same thing happens to "Declined" on the run after cleanup, and to any state when the wait option is set. Don't publish NothingToDo, or skip the update when the existing comment already shows a final state.
| messages.PullRequestDeclined, messages.NothingToDo) as err: | |
| messages.PullRequestDeclined) as err: |
— Claude Code
|
There was a problem hiding this comment.
Warning
Changes suggested — 🟡 1 warning
🔁 Incremental · 3 files reviewed
Verification
publish_statusnow catches anyExceptionafterEXPECTED_ERRORS, so a failed status update can no longer replace the original exception or skipnotify_user.NothingToDoreplaces only a status comment that still readsState.initial().label, so it no longer overwrites "Queued", "Merged" or "Declined".BuildInProgressandBuildNotStartedare no longer inFINAL_STATES, so the integration table and build statuses render while builds run.- The Bitbucket handler now skips
comment*events whose author nickname, username or account_id equalssettings.robot, matching the GitHub filter.
This push adds settings.robot to the MockBertE fixture in bert_e/tests/test_server.py. I found no test in the diff for the new Bitbucket robot-comment filter or for the NothingToDo rule that only replaces the initial state. The pytest suite (.venv/bin/pytest) is the repository check that covers this code.
Review 4 of 10 for this pull request · View the full run
Superseded: the latest Upsun Dispatch review no longer requests changes.
| raise | ||
| except (messages.BuildInProgress, messages.BuildNotStarted, | ||
| messages.PullRequestDeclined, messages.NothingToDo) as err: | ||
| publish_status(job, err) |
There was a problem hiding this comment.
Every run that ends here (BuildInProgress fires on each build status webhook) now calls render_status with git on: a second clone_git_repo, a second build_branch_cascade via _build_status_report, get_pull_requests, one get_build_status per integration branch, and the approval checks. That roughly doubles the git host API calls per job, so rate limits get hit sooner on busy repos.
Reuse what the main flow already computed (job.git.cascade, integration branches), or use with_git=False for silent states.
— Claude Code
| return cache[(revision, key)] | ||
|
|
||
| # share the build statuses between the table and the checklist | ||
| repo.get_build_status = cached_build_status |
There was a problem hiding this comment.
job.project_repo is the shared bert_e.project_repo, so this monkey-patch changes the repository object for every job. If any other thread calls get_build_status during this window, it reads the per-call cache. And if get_build_status was already an instance attribute (a wrapper or a test mock), the del in finally deletes it instead of restoring it.
Pass a caching wrapper in explicitly, or save the original and put it back with repo.get_build_status = original.
— Claude Code
| if settings.no_comment or settings.interactive: | ||
| LOG.debug('Not sending the status comment.') | ||
| return | ||
| comment = find_status_comment(pull_request, settings.robot) |
There was a problem hiding this comment.
GitHub pull_request.comments is not cached, so each find_status_comment call fetches every comment page again. ensure_status_comment and publish_status (for UPDATE_ONLY) look up the comment first, then this call looks it up again, so each run lists the comments 2 or 3 extra times.
Pass the comment already found into upsert_status_comment instead of searching again.
— Claude Code
|
| job.git.src_branch and job.git.dst_branch): | ||
| clone_git_repo(job) | ||
| repo = job.project_repo | ||
| had_own = 'get_build_status' in vars(repo) |
There was a problem hiding this comment.
This swaps out get_build_status on the shared project_repo object for a while, then puts it back. That is fragile: an exception in the cleanup path, or any other code using the repo object during that window, sees the patched method.
Pass a cache dict to _integration_rows instead. On GitHub, cache.BUILD_STATUS_CACHE already covers repeated lookups.
— Claude Code
| prs = {} | ||
| if names: | ||
| prs = {pr.src_branch: pr for pr in | ||
| job.project_repo.get_pull_requests(src_branch=names) |
There was a problem hiding this comment.
Every run that ends on a state now makes extra API calls. On GitHub that is one PR listing per integration branch (get_pull_requests sends one request per src_branch), plus one get_build_status per branch, plus the approvals and cascade checks in _build_status_report. GitHub also does not cache pr.comments, so ensure_status_comment and upsert_status_comment each list all comments again.
This can hit API rate limits on busy repos. Reuse the integration PRs already known in job, and pass the comment found by ensure_status_comment along instead of searching again.
— Claude Code
| if state is None: | ||
| return | ||
| existing = None | ||
| waiting = (isinstance(outcome, exceptions.NothingToDo) and |
There was a problem hiding this comment.
Matching on 'wait option' in str(outcome) ties this code to the exception message text in check_wait. If that message is reworded, a PR with the wait option silently shows the wrong state.
Raise a dedicated NothingToDo subclass (or set an attribute) for the wait case and check with isinstance.
— Claude Code
|
There was a problem hiding this comment.
Warning
Changes suggested — 🟡 1 warning
🔁 Incremental · 3 files reviewed
Verification
- The per-call monkey-patch of
project_repo.get_build_statusis gone, so the shared repo object is no longer modified while a job runs. check_dependenciesis the only place that raisesWaitOptionSet, andhandle_pull_requeststill catches it because it subclassesNothingToDo.- Both the GitHub and Bitbucket
get_build_statusserve fromBUILD_STATUS_CACHEonly when the cached state is SUCCESSFUL; any other state triggers a new API call.
This push adds and changes no tests. test_pr_status_comment.py has no case for the wait option, so the broken WaitOptionSet path is not covered.
Review 6 of 10 for this pull request · View the full run
| r'(?<=[a-z])(?=[A-Z])', ' ', type(exc).__name__).capitalize() | ||
| return cls(label, exc.state_status or exc.status, exc.code, | ||
| final=isinstance(exc, FINAL_STATES)) | ||
| label = SILENT_STATES.get(type(exc)) |
There was a problem hiding this comment.
type(exc) only matches exact classes, so WaitOptionSet (a subclass of NothingToDo) gets None here. publish_status then returns at if state is None before it reaches the waiting branch, so the 'Waiting (wait option is set)' state never shows. Match on subclasses instead, and add a test with /wait.
| label = SILENT_STATES.get(type(exc)) | |
| label = next((v for k, v in SILENT_STATES.items() | |
| if isinstance(exc, k)), None) |
— Claude Code
|
Superseded: the latest Upsun Dispatch review no longer requests changes.
| def handle_bitbucket_pr_event(bert_e, event, json_data): | ||
| """Handle a Bitbucket webhook sent on a pull request event.""" | ||
| commenter = (json_data.get('comment') or {}).get('user') or {} | ||
| if event.startswith('comment') and bert_e.settings.robot in ( |
There was a problem hiding this comment.
settings.robot is a UserDict, and its __eq__ raises ValueError when compared with None. Bitbucket no longer sends username in user objects (and nickname/account_id can be missing), so robot in (..., None, ...) raises on every comment written by a human. The webhook then fails and the command is never handled. The tests miss this because test_server.py sets robot = 'robot' (a plain str). Drop the empty values before comparing, and add a test with a real UserDict.
— Claude Code
| clone_git_repo(job) | ||
| # the git hosts cache build statuses, which are thus shared between | ||
| # the table and the checklist | ||
| integration = _integration_rows(job) |
There was a problem hiding this comment.
This runs on almost every job, including BuildInProgress/BuildNotStarted, which are the most common outcomes. Each run now adds a clone, one integration PR listing, one build-status call per integration branch and a full _build_status_report (approvals, builds, Jira fix versions, history). On GitHub that roughly doubles API usage per webhook and makes rate limits more likely. Consider skipping the refresh when the state did not change, or using only the data the main flow already fetched.
— Claude Code
|
2 participants
Automated Fix for Issue #299
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/workflow/gitwaterflow/init.py",
"bert_e/workflow/pr_utils.py",
"bert_e/workflow/base.py",
"bert_e/git_host/base.py",
"bert_e/git_host/github/init.py",
"bert_e/git_host/bitbucket/init.py",
"bert_e/git_host/mock.py",
"bert_e/exceptions.py",
"bert_e/templates/status.md",
"bert_e/settings.py"
]
Tests Created
[
"bert_e/tests/unit/test_pr_status_comment.py",
"bert_e/tests/unit/test_status_template.py"
]
Generated by n8n automation workflow