fix: resolve issue #284

[matthiasL-scality]

Fixes #284. /after_pull_request 1509 was silently ignored. It now replies with an incorrect-syntax message explaining that options use key=value.

🤖 Generated with Claude Code

[upsun-dispatch]

Caution

1 critical finding in this review. Details below.

Caution

Changes requested — 🔴 1 critical · 🟡 1 warning · 🔵 1 minor point

🔍 Full review · 4 files reviewed

Verification

• The /after_pull_request 1509 form fails the strict slash regex in handle_options, so only the new handle_commands branch handles it.
• A bare /after_pull_request or @bert-e approve with nothing after it still returns silently from handle_commands.
• The commands loop in handle_comments stops at the robot's last comment, so the new error is not re-raised once the bot has replied.

The new unit test in test_reactor.py covers only the positional-argument case. No test checks that key=value or multi-option comments still pass handle_commands, which is how the regression gets through. Unit and integration tests run through tox in the main.yaml workflow; the existing after_pull_request= cases in test_bert_e.py may catch the regression there.

Review details

• Commit: d968e43
• Model: claude-opus-5-5

---

Review 1 of 10 for this pull request · View the full run

[upsun-dispatch]

🔴 Critical — Correct key=value and multi-option comments now block every PR that uses them.

The if args: guard also fires on option comments that are valid. The command regex (?P<command>[A-Za-z_]+[^= ,])(?P<args>.*) stops the key before =, a space or a comma, so args is non-empty for every option that has a value or is followed by another option. I ran it: @bert-e after_pull_request=1509, /after_pull_request=12, @bert-e approve wait, @bert-e: approve, wait and /approve /wait all raise IncorrectSyntax now. Before this change they returned silently. The docstring documents exactly these forms as valid ({prefix}option1=val1 option2). handle_comments turns the exception into IncorrectCommandSyntax, which blocks the PR. Any user who posts the documented key=value syntax, or several options in one comment, gets an error reply and the PR does not progress. The check should only fire when the key is followed by whitespace and then a bare token without =, e.g. by checking that the remainder after the key does not start with = and that no token is itself a registered option.

[upsun-dispatch]

🟡 Warning — The usage hint in the new error reply leaves out the bot name.

The new messages.IncorrectCommandSyntax(...) is built without robot=. The template renders @{{ robot }} option[=argument], and neither TemplateException nor message.md fills robot in. The reply therefore shows the correct usage as @ option[=argument], with no bot name. The existing after_pull_request handler in commands.py passes robot=job.bert_e.client.login for this reason.

[upsun-dispatch]

🔵 Minor — Users see confusing internal Python error text in bot replies.

The template now renders extra_message. The existing except TypeError path in handle_comments (around line 338) passes extra_message=str(err), and until now that text was never shown. After this change, raw Python TypeError text such as a() takes from 1 to 2 positional arguments but 3 were given is posted verbatim into PR comments. That text exposes internal handler names and can come from bugs unrelated to user syntax.

[claude]

The template uses robot variable but this handler does not pass robot. With StrictUndefined in template_loader.py, this will crash at render time when /after_pull_request 1509 is used. Add robot=job.settings.robot to the kwargs. — Claude Code

[claude]

The template now checks extra_message with a plain if-test, but Jinja2 StrictUndefined raises UndefinedError when the variable is not passed at all. The existing path in commands.py:272 raises IncorrectCommandSyntax without extra_message, so this template change will crash there. Use 'if extra_message is defined and extra_message' instead. — Claude Code

[claude]

Two template rendering bugs that will crash at runtime under Jinja2 StrictUndefined:

• Missing robot parameter (gitwaterflow/init.py:365-370): the new IncorrectSyntax handler raises IncorrectCommandSyntax without passing robot, but the template uses {{ robot }}. Fix: add robot=job.settings.robot to the kwargs.
• Undefined extra_message check (incorrect_command_syntax.md:8): {% if extra_message %} crashes when extra_message is not provided (e.g. from commands.py:272). Fix: use {% if extra_message is defined and extra_message %}.

Review by Claude Code