fix: resolve issue #284 - #287
matthiasL-scality wants to merge 1 commit into
Conversation
![upsun-dispatch[bot]](https://avatars.githubusercontent.com/in/4153575?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
1 critical finding in this review. Details below.
Caution
Changes requested — 🔴 1 critical · 🟡 2 warnings
🔍 Full review · 5 files reviewed
Verification
/after_pull_request=1509still passeshandle_commands: the command regex splits it into keyafter_pull_requestand args['=1509'], so no error is raised.@bert-e after_pull_request Nis unaffected by the new branch, which is guarded byslash_shorthand; that form still fails through the TypeError inhandle_options.IncorrectSyntax.keywordis passed on to the template, and the template guards its hint withkeyword is defined and keyword, so older callers that don't pass a keyword still render.
The diff adds a unit test in bert_e/tests/unit/test_reactor.py and an integration test test_after_pull_request_slash_space_syntax in test_bert_e.py. Neither test covers multi-option / comments (e.g. /approve /bypass_jira_check) going through handle_commands, or a second run after the error reply has been posted.
Review details
- Commit: 4f7224f
- Model: claude-opus-5-5
Review 1 of 10 for this pull request · View the full run
| return | ||
| raise NotFound(key) | ||
| if not isinstance(command, Command): | ||
| if slash_shorthand and args and not args[0].startswith("="): |
There was a problem hiding this comment.
🔴 Critical — Valid multi-option comments get rejected and block the merge run.
The new check raises IncorrectSyntax for any /-shorthand option that has trailing tokens not starting with =. That includes valid multi-option comments that handle_options accepts on purpose, through its regex ^/[\w=]+([\s,.\-:;|+]+/[\w=]+)*\s*$. For /approve /bypass_jira_check or /approve, /after_pull_request=3, handle_commands parses key approve with args ['/bypass_jira_check'], and '/bypass_jira_check' does not start with =, so it raises IncorrectSyntax('approve'). I ran both comments through a Reactor subclass: handle_options applied them without error, and handle_commands raised IncorrectSyntax each time. Posted after Bert-E's last message, these valid comments now stop the run with an "Incorrect command syntax" reply that blames approve. Before this change they were accepted. The check should reject only args that are not further /-prefixed option tokens, or should reuse the options regex.
| active_options=job.active_options, command=err.keyword, | ||
| author=author, self_pr=(author == pr_author), comment=text | ||
| ) from err | ||
| except IncorrectSyntax as err: |
There was a problem hiding this comment.
🟡 Warning — The dependency is silently dropped after one reply, so the PR can merge too early.
The new error comes only from the commands loop, and that loop reads only the comments posted after the robot's last message (if author == job.settings.robot: return). Once Bert-E posts the IncorrectCommandSyntax reply, the next run stops at that reply and never sees /after_pull_request 1509 again. The options loop doesn't catch it either: the comment fails the /-shorthand regex in handle_options because the second token has no /, so it is dropped silently. The next run then goes ahead as if the dependency had never been declared and can merge the PR before PR 1509. The message says "edit or delete the comment so I can move on", but the block lasts for one run only. Compare the @bert-e after_pull_request N form, which fails inside handle_options (a TypeError) on every run, so its block persists.
| return | ||
| raise NotFound(key) | ||
| if not isinstance(command, Command): | ||
| if slash_shorthand and args and not args[0].startswith("="): |
There was a problem hiding this comment.
🟡 Warning — One malformed variant is still ignored silently.
/after_pull_request = 1509 (spaces around =) gives args ['=', '1509'], and args[0].startswith('=') passes, so no error is raised. handle_options also drops this form (verified), so the malformed declaration is still ignored silently, the same failure mode issue #284 describes.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| # An option followed by free-form arguments (e.g. | ||
| # ``/after_pull_request 1509``) is not a valid option | ||
| # declaration: options take their argument as ``key=value``. | ||
| raise IncorrectSyntax(key) |
There was a problem hiding this comment.
Minor: the handle_commands docstring Raises section (line ~415) only documents NotFound and NotPrivileged. Consider adding IncorrectSyntax to that list now that this method can raise it.
— Claude Code
|
LGTM — clean fix for #284. The new |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #287 +/- ##
==========================================
+ Coverage 90.21% 90.22% +0.01%
==========================================
Files 82 82
Lines 11293 11320 +27
==========================================
+ Hits 10188 10214 +26
- Misses 1105 1106 +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:
|
1 participant
Reply with an incorrect-syntax error (with usage hint) when an option is given as
/after_pull_request 1509instead of/after_pull_request=1509, rather than silently ignoring it. Fixes #284