[CHG] runbot_merge, forwardbot: merge commands parser

- move all commands parsing to runbot_merge as part of the long-term
  unification effort (#789)
- set up an actual parser-ish structure to parse the commands to
  something approaching a sum type (fixes #507)
- this is mostly prep for reworking the commands set (#673), although
  *strict command parsing* has been implemented (cf update to
  `test_unknown_commands`)

forwardport/models/project.py

@@ -55,55 +55,6 @@ class Project(models.Model):
 
     id: int
     github_prefix: str
-    fp_github_token = fields.Char()
-    fp_github_name = fields.Char(store=True, compute="_compute_git_identity")
-    fp_github_email = fields.Char(store=True, compute="_compute_git_identity")
-
-    def _find_commands(self, comment):
-        if self.env.context.get('without_forward_port'):
-            return super()._find_commands(comment)
-
-        return re.findall(
-            '^\s*[@|#]?{}:? (.*)$'.format(self.fp_github_name),
-            comment, re.MULTILINE | re.IGNORECASE
-        ) + super()._find_commands(comment)
-
-    # technically the email could change at any moment...
-    @api.depends('fp_github_token')
-    def _compute_git_identity(self):
-        s = requests.Session()
-        for project in self:
-            if not project.fp_github_token or (project.fp_github_name and project.fp_github_email):
-                continue
-
-            r0 = s.get('https://api.github.com/user', headers={
-                'Authorization': 'token %s' % project.fp_github_token
-            })
-            if not r0.ok:
-                _logger.error("Failed to fetch forward bot information for project %s: %s", project.name, r0.text or r0.content)
-                continue
-
-            user = r0.json()
-            project.fp_github_name = user['name'] or user['login']
-            if email := user['email']:
-                project.fp_github_email = email
-                continue
-
-            if 'user:email' not in set(re.split(r',\s*', r0.headers['x-oauth-scopes'])):
-                raise UserError("The forward-port github token needs the user:email scope to fetch the bot's identity.")
-            r1 = s.get('https://api.github.com/user/emails', headers={
-                'Authorization': 'token %s' % project.fp_github_token
-            })
-            if not r1.ok:
-                _logger.error("Failed to fetch forward bot emails for project %s: %s", project.name, r1.text or r1.content)
-                continue
-            project.fp_github_email = next((
-                entry['email']
-                for entry in r1.json()
-                if entry['primary']
-            ), None)
-            if not project.fp_github_email:
-                raise UserError("The forward-port bot needs a public or primary email set up.")
 
     def write(self, vals):
         # check on branches both active and inactive so disabling branches doesn't
@@ -404,90 +355,6 @@ class PullRequests(models.Model):
             })
         return r
 
-    def _parse_commands(self, author, comment, login):
-        super(PullRequests, self.with_context(without_forward_port=True))._parse_commands(author, comment, login)
-
-        tokens = [
-            token
-            for line in re.findall('^\s*[@|#]?{}:? (.*)$'.format(self.repository.project_id.fp_github_name), comment['body'] or '', re.MULTILINE | re.IGNORECASE)
-            for token in line.split()
-        ]
-        if not tokens:
-            _logger.info("found no commands in comment of %s (%s) (%s)", author.github_login, author.display_name,
-                 utils.shorten(comment['body'] or '', 50)
-            )
-            return
-
-        # TODO: don't use a mutable tokens iterator
-        tokens = iter(tokens)
-        while True:
-            token = next(tokens, None)
-            if token is None:
-                break
-
-            ping = False
-            close = False
-            msg = None
-            if token in ('ci', 'skipci'):
-                pr = (self.source_id or self)
-                if pr._pr_acl(author).is_reviewer:
-                    pr.fw_policy = token
-                    msg = "Not waiting for CI to create followup forward-ports." if token == 'skipci' else "Waiting for CI to create followup forward-ports."
-                else:
-                    ping = True
-                    msg = "you can't configure ci."
-
-            if token == 'ignore': # replace 'ignore' by 'up to <pr_branch>'
-                token = 'up'
-                tokens = itertools.chain(['to', self.target.name], tokens)
-
-            if token in ('r+', 'review+'):
-                if not self.source_id:
-                    ping = True
-                    msg = "I can only do this on forward-port PRs and this is not one, see {}.".format(
-                        self.repository.project_id.github_prefix
-                    )
-                elif not self.parent_id:
-                    ping = True
-                    msg = "I can only do this on unmodified forward-port PRs, ask {}.".format(
-                        self.repository.project_id.github_prefix
-                    )
-                else:
-                    merge_bot = self.repository.project_id.github_prefix
-                    # don't update the root ever
-                    for pr in (p for p in self._iter_ancestors() if p.parent_id if p.state in RPLUS):
-                        # only the author is delegated explicitely on the
-                        pr._parse_commands(author, {**comment, 'body': merge_bot + ' r+'}, login)
-            elif token == 'close':
-                if self.source_id._pr_acl(author).is_reviewer:
-                    close = True
-                else:
-                    ping = True
-                    msg = "you can't close PRs."
-
-            elif token == 'up' and next(tokens, None) == 'to':
-                limit = next(tokens, None)
-                ping = True
-                if not self._pr_acl(author).is_author:
-                    msg = "you can't set a forward-port limit."
-                elif not limit:
-                    msg = "please provide a branch to forward-port to."
-                else:
-                    ping, msg = self._maybe_update_limit(limit)
-
-            if msg or close:
-                if msg:
-                    _logger.info("%s [%s]: %s", self.display_name, login, msg)
-                else:
-                    _logger.info("%s [%s]: closing", self.display_name, login)
-                self.env['runbot_merge.pull_requests.feedback'].create({
-                    'repository': self.repository.id,
-                    'pull_request': self.number,
-                    'message': f'@{author.github_login} {msg}' if msg and ping else msg,
-                    'close': close,
-                    'token_field': 'fp_github_token',
-                })
-
     def _maybe_update_limit(self, limit: str) -> typing.Tuple[bool, str]:
         limit_id = self.env['runbot_merge.branch'].with_context(active_test=False).search([
             ('project_id', '=', self.repository.project_id.id),

forwardport/tests/test_limit.py

@@ -101,22 +101,22 @@ def test_disable(env, config, make_repo, users):
         pr = prod.make_pr(target='a', head='branch2')
         prod.post_status(c, 'success', 'legal/cla')
         prod.post_status(c, 'success', 'ci/runbot')
-        pr.post_comment('hansen r+\n%s up to' % bot_name, config['role_reviewer']['token'])
+        pr.post_comment(f'hansen r+\n{bot_name} up to', config['role_reviewer']['token'])
-        pr.post_comment('%s up to b' % bot_name, config['role_reviewer']['token'])
+        pr.post_comment(f'{bot_name} up to b', config['role_reviewer']['token'])
-        pr.post_comment('%s up to foo' % bot_name, config['role_reviewer']['token'])
+        pr.post_comment(f'{bot_name} up to foo', config['role_reviewer']['token'])
-        pr.post_comment('%s up to c' % bot_name, config['role_reviewer']['token'])
+        pr.post_comment(f'{bot_name} up to c', config['role_reviewer']['token'])
     env.run_crons()
 
     # use a set because git webhooks delays might lead to mis-ordered
     # responses and we don't care that much
     assert set(pr.comments) == {
-        (users['reviewer'], "hansen r+\n%s up to" % bot_name),
+        (users['reviewer'], f"hansen r+\n{bot_name} up to"),
-        (users['other'], "@%s please provide a branch to forward-port to." % users['reviewer']),
+        (users['other'], "@{reviewer} please provide a branch to forward-port to.".format_map(users)),
-        (users['reviewer'], "%s up to b" % bot_name),
+        (users['reviewer'], f"{bot_name} up to b"),
-        (users['other'], "@%s branch 'b' is disabled, it can't be used as a forward port target." % users['reviewer']),
+        (users['other'], "@{reviewer} branch 'b' is disabled, it can't be used as a forward port target.".format_map(users)),
-        (users['reviewer'], "%s up to foo" % bot_name),
+        (users['reviewer'], f"{bot_name} up to foo"),
-        (users['other'], "@%s there is no branch 'foo', it can't be used as a forward port target." % users['reviewer']),
+        (users['other'], "@{reviewer} there is no branch 'foo', it can't be used as a forward port target.".format_map(users)),
-        (users['reviewer'], "%s up to c" % bot_name),
+        (users['reviewer'], f"{bot_name} up to c"),
         (users['other'], "Forward-porting to 'c'."),
         seen(env, pr, users),
     }

forwardport/tests/test_weird.py

@@ -809,7 +809,7 @@ def test_approve_draft(env, config, make_repo, users):
     assert pr.comments == [
         (users['reviewer'], 'hansen r+'),
         seen(env, pr, users),
-        (users['user'], f"I'm sorry, @{users['reviewer']}: draft PRs can not be approved."),
+        (users['user'], f"@{users['reviewer']} draft PRs can not be approved."),
     ]
 
     with prod:

runbot_merge/models/commands.py

@@ -0,0 +1,231 @@
+import enum
+from collections import abc
+from dataclasses import dataclass, field
+from typing import Iterator, List, Optional, Union
+
+
+def tokenize(line: str) -> Iterator[str]:
+    cur = ''
+    for c in line:
+        if c == '-' and not cur:
+            yield '-'
+        elif c in ' \t+=,':
+            if cur:
+                yield cur
+            cur = ''
+            if not c.isspace():
+                yield c
+        else:
+            cur += c
+
+    if cur:
+        yield cur
+
+def normalize(it: Iterator[str]) -> Iterator[str]:
+    """Converts shorthand tokens to expanded version
+    """
+    for t in it:
+        match t:
+            case 'r':
+                yield 'review'
+            case 'r-':
+                yield 'review'
+                yield '-'
+            case 'p':
+                yield 'priority'
+            case _:
+                yield t
+
+
+@dataclass
+class Peekable(abc.Iterator[str]):
+    it: Iterator[str]
+    memo: Optional[str] = None
+
+    def __iter__(self) -> Iterator[str]:
+        return self
+
+    def __next__(self) -> str:
+        if self.memo is not None:
+            v, self.memo = self.memo, None
+            return v
+        return next(self.it)
+
+    def peek(self) -> Optional[str]:
+        if self.memo is None:
+            self.memo = next(self.it, None)
+        return self.memo
+
+
+def assert_next(it: Iterator[str], val: str):
+    if (actual := next(it, None)) != val:
+        raise CommandError(f"expected {val!r}, got {actual!r}")
+
+
+class CommandError(Exception):
+    pass
+
+
+class Approve:
+    def __str__(self):
+        return 'review+'
+
+
+class Reject:
+    def __str__(self):
+        return 'review-'
+
+
+class MergeMethod(enum.Enum):
+    SQUASH = 'squash'
+    REBASE_FF = 'rebase-ff'
+    REBASE_MERGE = 'rebase-merge'
+    MERGE = 'merge'
+
+    def __str__(self):
+        return self.value
+
+
+class Retry:
+    def __str__(self):
+        return 'retry'
+
+
+class Check:
+    def __str__(self):
+        return 'check'
+
+
+@dataclass
+class Override:
+    statuses: List[str] = field(default_factory=list)
+
+    def __str__(self):
+        return f"override={','.join(self.statuses)}"
+
+
+@dataclass
+class Delegate:
+    users: List[str] = field(default_factory=list)
+
+    def __str__(self):
+        if not self.users:
+            return 'delegate+'
+        return f"delegate={','.join(self.users)}"
+
+
+class Priority(enum.IntEnum):
+    NUKE = 0
+    PRIORITIZE = 1
+    DEFAULT = 2
+
+
+Command = Union[Approve, Reject, MergeMethod, Retry, Check, Override, Delegate, Priority]
+
+
+def parse_mergebot(line: str) -> Iterator[Command]:
+    it = Peekable(normalize(tokenize(line)))
+    for token in it:
+        match token:
+            case 'review':
+                match next(it):
+                    case '+':
+                        yield Approve()
+                    case '-':
+                        yield Reject()
+                    case t:
+                        raise CommandError(f"unknown review {t!r}")
+            case 'squash':
+                yield MergeMethod.SQUASH
+            case 'rebase-ff':
+                yield MergeMethod.REBASE_FF
+            case 'rebase-merge':
+                yield MergeMethod.REBASE_MERGE
+            case 'merge':
+                yield MergeMethod.MERGE
+            case 'retry':
+                yield Retry()
+            case 'check':
+                yield Check()
+            case 'override':
+                assert_next(it, '=')
+                ci = [next(it)]
+                while it.peek() == ',':
+                    next(it)
+                    ci.append(next(it))
+                yield Override(ci)
+            case 'delegate':
+                match next(it):
+                    case '+':
+                        yield Delegate()
+                    case '=':
+                        delegates = [next(it).lstrip('#@')]
+                        while it.peek() == ',':
+                            next(it)
+                            delegates.append(next(it).lstrip('#@'))
+                        yield Delegate(delegates)
+                    case d:
+                        raise CommandError(f"unknown delegation {d!r}")
+            case 'priority':
+                assert_next(it, '=')
+                yield Priority(int(next(it)))
+            case c:
+                raise CommandError(f"unknown command {c!r}")
+
+
+class FWApprove:
+    def __str__(self):
+        return 'review+'
+
+
+@dataclass
+class CI:
+    run: bool
+    def __str__(self):
+        return 'ci' if self.run else 'skipci'
+
+
+@dataclass
+class Limit:
+    branch: Optional[str]
+
+    def __str__(self):
+        if self.branch is None:
+            return 'ignore'
+        return f'up to {self.branch}'
+
+
+class Close:
+    def __str__(self):
+        return 'close'
+
+
+FWCommand = Union[FWApprove, CI, Limit, Close]
+
+
+def parse_forwardbot(line: str) -> Iterator[FWCommand]:
+    it = Peekable(normalize(tokenize(line)))
+    for token in it:
+        match token:
+            case 'review':
+                match next(it):
+                    case '+':
+                        yield FWApprove()
+                    case t:
+                        raise CommandError(f"unknown review {t!r}", True)
+            case 'ci':
+                yield CI(True)
+            case 'skipci':
+                yield CI(False)
+            case 'ignore':
+                yield Limit(None)
+            case 'up':
+                assert_next(it, 'to')
+                if limit := next(it, None):
+                    yield Limit(limit)
+                else:
+                    raise CommandError("please provide a branch to forward-port to.", True)
+            case 'close':
+                yield Close()
+            case c:
+                raise CommandError(f"unknown command {c!r}", True)

runbot_merge/models/project.py

@@ -1,5 +1,6 @@
 import logging
 import re
+from typing import List, Tuple
 
 import requests
 import sentry_sdk
@@ -38,6 +39,9 @@ class Project(models.Model):
         help="Prefix (~bot name) used when sending commands from PR "
              "comments e.g. [hanson retry] or [hanson r+ p=1]",
     )
+    fp_github_token = fields.Char()
+    fp_github_name = fields.Char(store=True, compute="_compute_git_identity")
+    fp_github_email = fields.Char(store=True, compute="_compute_git_identity")
 
     batch_limit = fields.Integer(
         default=8, group_operator=None, help="Maximum number of PRs staged together")
@@ -96,6 +100,43 @@ class Project(models.Model):
             if not project.github_email:
                 raise UserError("The merge bot needs a public or accessible primary email set up.")
 
+    # technically the email could change at any moment...
+    @api.depends('fp_github_token')
+    def _compute_git_identity(self):
+        s = requests.Session()
+        for project in self:
+            if not project.fp_github_token or (project.fp_github_name and project.fp_github_email):
+                continue
+
+            r0 = s.get('https://api.github.com/user', headers={
+                'Authorization': 'token %s' % project.fp_github_token
+            })
+            if not r0.ok:
+                _logger.error("Failed to fetch forward bot information for project %s: %s", project.name, r0.text or r0.content)
+                continue
+
+            user = r0.json()
+            project.fp_github_name = user['name'] or user['login']
+            if email := user['email']:
+                project.fp_github_email = email
+                continue
+
+            if 'user:email' not in set(re.split(r',\s*', r0.headers['x-oauth-scopes'])):
+                raise UserError("The forward-port github token needs the user:email scope to fetch the bot's identity.")
+            r1 = s.get('https://api.github.com/user/emails', headers={
+                'Authorization': 'token %s' % project.fp_github_token
+            })
+            if not r1.ok:
+                _logger.error("Failed to fetch forward bot emails for project %s: %s", project.name, r1.text or r1.content)
+                continue
+            project.fp_github_email = next((
+                entry['email']
+                for entry in r1.json()
+                if entry['primary']
+            ), None)
+            if not project.fp_github_email:
+                raise UserError("The forward-port bot needs a public or primary email set up.")
+
     def _check_stagings(self, commit=False):
         # check branches with an active staging
         for branch in self.env['runbot_merge.branch']\
@@ -132,9 +173,18 @@ class Project(models.Model):
                 if commit:
                     self.env.cr.commit()
 
-    def _find_commands(self, comment):
+    def _find_commands(self, comment: str) -> List[Tuple[str, str]]:
+        """Tries to find all the lines starting (ignoring leading whitespace)
+        with either the merge or the forward port bot identifiers.
+
+        For convenience, the identifier *can* be prefixed with an ``@`` or
+        ``#``, and suffixed with a ``:``.
+        """
+        logins = '|'.join(map(re.escape, filter(None, [self.github_prefix, self.fp_github_name])))
+        # horizontal whitespace (\s - {\n, \r}), but Python doesn't have \h or \p{Blank}
+        h = r'[^\S\r\n]'
         return re.findall(
-            '^\s*[@|#]?{}:? (.*)$'.format(self.github_prefix),
+            fr'^{h}*[@|#]?({logins})(?:{h}+|:{h}*)(.*)$',
             comment, re.MULTILINE | re.IGNORECASE)
 
     def _has_branch(self, name):

runbot_merge/models/pull_requests.py

@@ -8,7 +8,7 @@ import logging
 import pprint
 import re
 import time
-from typing import Optional, Union
+from typing import Optional, Union, List, Literal
 
 import sentry_sdk
 import werkzeug
@@ -16,6 +16,7 @@ import werkzeug
 from odoo import api, fields, models, tools
 from odoo.exceptions import ValidationError
 from odoo.osv import expression
+from . import commands
 
 from .. import github, exceptions, controllers, utils
 
@@ -561,71 +562,46 @@ class PullRequests(models.Model):
             'closing': closing,
         })
 
-    def _parse_command(self, commandstring):
-        for m in re.finditer(
-            r'(\S+?)(?:([+-])|=(\S*))?(?=\s|$)',
-            commandstring,
-        ):
-            name, flag, param = m.groups()
-            if name == 'r':
-                name = 'review'
-            if flag in ('+', '-'):
-                yield name, flag == '+'
-            elif name == 'delegate':
-                if param:
-                    for p in param.split(','):
-                        yield 'delegate', p.lstrip('#@')
-            elif name == 'override':
-                if param:
-                    for p in param.split(','):
-                        yield 'override', p
-            elif name in ('p', 'priority'):
-                if param in ('0', '1', '2'):
-                    yield ('priority', int(param))
-            elif any(name == k for k, _ in type(self).merge_method.selection):
-                yield ('method', name)
-            else:
-                yield name, param
-
     def _parse_commands(self, author, comment, login):
-        """Parses a command string prefixed by Project::github_prefix.
-
-        A command string can contain any number of space-separated commands:
-
-        retry
-          resets a PR in error mode to ready for staging
-        r(eview)+/-
-           approves or disapproves a PR (disapproving just cancels an approval)
-        delegate+/delegate=<users>
-          adds either PR author or the specified (github) users as
-          authorised reviewers for this PR. ``<users>`` is a
-          comma-separated list of github usernames (no @)
-        p(riority)=2|1|0
-          sets the priority to normal (2), pressing (1) or urgent (0).
-          Lower-priority PRs are selected first and batched together.
-        rebase+/-
-          Whether the PR should be rebased-and-merged (the default) or just
-          merged normally.
-        """
         assert self, "parsing commands must be executed in an actual PR"
 
         (login, name) = (author.github_login, author.display_name) if author else (login, 'not in system')
 
-        is_admin, is_reviewer, is_author = self._pr_acl(author)
+        commandlines = self.repository.project_id._find_commands(comment['body'] or '')
-
+        if not commandlines:
-        commands = [
+            _logger.info("found no commands in comment of %s (%s) (%s)", login, name,
-            ps
-            for m in self.repository.project_id._find_commands(comment['body'] or '')
-            for ps in self._parse_command(m)
-        ]
-
-        if not commands:
-            _logger.info("found no commands in comment of %s (%s) (%s)", author.github_login, author.display_name,
                  utils.shorten(comment['body'] or '', 50)
             )
             return 'ok'
 
-        if not (is_author or any(cmd == 'override' for cmd, _ in commands)):
+        def feedback(message: Optional[str] = None, close: bool = False, token: Literal['github_token', 'fp_github_token'] = 'github_token'):
+            self.env['runbot_merge.pull_requests.feedback'].create({
+                'repository': self.repository.id,
+                'pull_request': self.number,
+                'message': message,
+                'close': close,
+                'token_field': token,
+            })
+        try:
+            cmds: List[Union[commands.Command, commands.FWCommand]] = [
+                ps
+                for bot, line in commandlines
+                for ps in (commands.parse_mergebot(line) if bot.casefold() == self.repository.project_id.github_prefix.casefold() else commands.parse_forwardbot(line))
+            ]
+        except Exception as e:
+            _logger.info(
+                "error %s while parsing comment of %s (%s): %s",
+                e,
+                author.github_login, author.display_name,
+                utils.shorten(comment['body'] or '', 50),
+                exc_info=True
+            )
+            feedback(message=f"@{login} {e.args[0]}", token='fp_github_token' if len(e.args) >= 2 and e.args[1] else 'github_token')
+            return 'error'
+
+        is_admin, is_reviewer, is_author = self._pr_acl(author)
+
+        if not (is_author or any(isinstance(cmd, commands.Override) for cmd in cmds)):
             # no point even parsing commands
             _logger.info("ignoring comment of %s (%s): no ACL to %s",
                           login, name, self.display_name)
@@ -636,40 +612,13 @@ class PullRequests(models.Model):
             )
             return 'ignored'
 
-        applied, ignored = [], []
+        rejections = []
-        def reformat(command, param):
+        for command in cmds:
-            if param is None:
+            fwbot, msg = False, None
-                pstr = ''
+            match command:
-            elif isinstance(param, bool):
+                case commands.Approve() if self.draft:
-                pstr = '+' if param else '-'
-            elif isinstance(param, list):
-                pstr = '=' + ','.join(param)
-            else:
-                pstr = '={}'.format(param)
-
-            return '%s%s' % (command, pstr)
-        msgs = []
-        for command, param in commands:
-            ok = False
-            msg = None
-            if command == 'retry':
-                if is_author:
-                    if self.state == 'error':
-                        ok = True
-                        self.state = 'ready'
-                    else:
-                        msg = "retry makes no sense when the PR is not in error."
-            elif command == 'check':
-                if is_author:
-                    self.env['runbot_merge.fetch_job'].create({
-                        'repository': self.repository.id,
-                        'number': self.number,
-                    })
-                    ok = True
-            elif command == 'review':
-                if self.draft:
                     msg = "draft PRs can not be approved."
-                elif param and is_reviewer:
+                case commands.Approve() if is_reviewer:
                     oldstate = self.state
                     newstate = RPLUS.get(self.state)
                     if not author.email:
@@ -679,7 +628,6 @@ class PullRequests(models.Model):
                     else:
                         self.state = newstate
                         self.reviewed_by = author
-                        ok = True
                     _logger.debug(
                         "r+ on %s by %s (%s->%s) status=%s message? %s",
                         self.display_name, author.github_login,
@@ -694,7 +642,7 @@ class PullRequests(models.Model):
                             pull_request=self.number,
                             format_args={'user': login, 'pr': self},
                         )
-                elif not param and is_author:
+                case commands.Reject() if is_author:
                     newstate = RMINUS.get(self.state)
                     if self.priority == 0 or newstate:
                         if newstate:
@@ -707,94 +655,128 @@ class PullRequests(models.Model):
                                 format_args={'user': login, 'pr': self},
                             )
                         self.unstage("unreviewed (r-) by %s", login)
-                        ok = True
                     else:
                         msg = "r- makes no sense in the current PR state."
-            elif command == 'delegate':
+                case commands.MergeMethod() as command if is_reviewer:
-                if is_reviewer:
+                    self.merge_method = command.value
-                    ok = True
+                    explanation = next(label for value, label in type(self).merge_method.selection if value == command.value)
-                    Partners = self.env['res.partner']
-                    if param is True:
-                        delegate = self.author
-                    else:
-                        delegate = Partners.search([('github_login', '=', param)]) or Partners.create({
-                            'name': param,
-                            'github_login': param,
-                        })
-                    delegate.write({'delegate_reviewer': [(4, self.id, 0)]})
-            elif command == 'priority':
-                if is_admin:
-                    ok = True
-                    self.priority = param
-                    if param == 0:
-                        self.target.active_staging_id.cancel(
-                            "P=0 on %s by %s, unstaging target %s",
-                            self.display_name,
-                            author.github_login, self.target.name,
-                        )
-            elif command == 'method':
-                if is_reviewer:
-                    self.merge_method = param
-                    ok = True
-                    explanation = next(label for value, label in type(self).merge_method.selection if value == param)
                     self.env.ref("runbot_merge.command.method")._send(
                         repository=self.repository,
                         pull_request=self.number,
                         format_args={'new_method': explanation, 'pr': self, 'user': login},
                     )
-            elif command == 'override':
+                case commands.Retry() if is_author:
-                overridable = author.override_rights\
+                    if self.state == 'error':
-                    .filtered(lambda r: not r.repository_id or (r.repository_id == self.repository))\
+                        self.state = 'ready'
-                    .mapped('context')
-                if param in overridable:
-                    self.overrides = json.dumps({
-                        **json.loads(self.overrides),
-                        param: {
-                            'state': 'success',
-                            'target_url': comment['html_url'],
-                            'description': f"Overridden by @{author.github_login}",
-                        },
-                    })
-                    c = self.env['runbot_merge.commit'].search([('sha', '=', self.head)])
-                    if c:
-                        c.to_check = True
                     else:
-                        c.create({'sha': self.head, 'statuses': '{}'})
+                        msg = "retry makes no sense when the PR is not in error."
-                    ok = True
+                case commands.Check() if is_author:
-                else:
+                    self.env['runbot_merge.fetch_job'].create({
-                    msg = "you are not allowed to override this status."
+                        'repository': self.repository.id,
-            else:
+                        'number': self.number,
-                # ignore unknown commands
+                    })
-                continue
+                case commands.Delegate(users) if is_reviewer:
+                    if not users:
+                        delegates = self.author
+                    else:
+                        delegates = self.env['res.partner']
+                        for login in users:
+                            delegates |= delegates.search([('github_login', '=', login)]) or delegates.create({
+                                'name': login,
+                                'github_login': login,
+                            })
+                    delegates.write({'delegate_reviewer': [(4, self.id, 0)]})
+                case commands.Priority() if is_admin:
+                    self.priority = int(command)
+                    if command is commands.Priority.NUKE:
+                        self.target.active_staging_id.cancel(
+                            "P=0 on %s by %s, unstaging target %s",
+                            self.display_name,
+                            author.github_login, self.target.name,
+                        )
+                case commands.Override(statuses):
+                    for status in statuses:
+                        overridable = author.override_rights\
+                            .filtered(lambda r: not r.repository_id or (r.repository_id == self.repository))\
+                            .mapped('context')
+                        if status in overridable:
+                            self.overrides = json.dumps({
+                                **json.loads(self.overrides),
+                                status: {
+                                    'state': 'success',
+                                    'target_url': comment['html_url'],
+                                    'description': f"Overridden by @{author.github_login}",
+                                },
+                            })
+                            c = self.env['runbot_merge.commit'].search([('sha', '=', self.head)])
+                            if c:
+                                c.to_check = True
+                            else:
+                                c.create({'sha': self.head, 'statuses': '{}'})
+                        else:
+                            msg = f"you are not allowed to override {status!r}."
+                # FW
+                case commands.FWApprove():
+                    fwbot = True
+                    if not self.source_id:
+                        msg = "I can only do this on forward-port PRs and this is not one, see {}.".format(
+                            self.repository.project_id.github_prefix
+                        )
+                    elif not self.parent_id:
+                        msg = "I can only do this on unmodified forward-port PRs, ask {}.".format(
+                            self.repository.project_id.github_prefix
+                        )
+                    else:
+                        merge_bot = self.repository.project_id.github_prefix
+                        # FIXME: classification of messages from child PR :(
+                        # don't update the root ever
+                        for pr in (p for p in self._iter_ancestors() if p.parent_id if p.state in RPLUS):
+                            # only the author is delegated explicitely on the
+                            pr._parse_commands(author, {**comment, 'body': merge_bot + ' r+'}, login)
+                case commands.Close() if self.source_id._pr_acl(author).is_reviewer:
+                    feedback(close=True, token='fp_github_token')
+                case commands.CI(run):
+                    pr = (self.source_id or self)
+                    if pr._pr_acl(author).is_reviewer:
+                        pr.fw_policy = 'ci' if run else 'skipci'
+                        feedback(
+                            message="Waiting for CI to create followup forward-ports." if run else "Not waiting for CI to create followup forward-ports.",
+                            token='fp_github_token',
+                        )
+                    else:
+                        fwbot = True
+                        msg = "you can't configure ci."
+                case commands.Limit(branch):
+                    fwbot = True
+                    if is_author:
+                        ping, msg = self._maybe_update_limit(branch or self.target.name)
+                        if not ping:
+                            feedback(message=msg, token='fp_github_token')
+                            msg = None
+                    else:
+                        msg = "you can't set a forward-port limit."
+                # NO!
+                case _:
+                    msg = f"you can't {command}. Skill issue."
+            if msg is not None:
+                rejections.append((fwbot, msg))
 
-            _logger.info(
+        if not rejections:
-                "%s %s(%s) on %s by %s (%s)",
+            _logger.info("%s (%s) applied %s", login, name, cmds)
-                "applied" if ok else "ignored",
+            return 'applied ' + ', '.join(map(str, cmds))
-                command, param, self.display_name,
-                author.github_login, author.display_name,
-            )
-            if ok:
-                applied.append(reformat(command, param))
-            else:
-                ignored.append(reformat(command, param))
-                msgs.append(msg or "you can't {}.".format(reformat(command, param)))
 
-        if msgs:
+        self.env.cr.rollback()
-            joiner = ' ' if len(msgs) == 1 else '\n- '
+        rejections_list = ''.join(f'\n- {r}' for fw, r in rejections if not fw)
-            msgs.insert(0, "I'm sorry, @{}:".format(login))
+        fw_rejections_list = ''.join(f'\n- {r}' for fw, r in rejections if fw)
-            self.env['runbot_merge.pull_requests.feedback'].create({
+        _logger.info("%s (%s) tried to apply %s%s", login, name, cmds, rejections_list + fw_rejections_list)
-                'repository': self.repository.id,
+        footer = '' if len(cmds) == len(rejections) else "\n\nFor your own safety I've ignored everything in your comment."
-                'pull_request': self.number,
+        if rejections_list:
-                'message': joiner.join(msgs),
+            rejections = ' ' + rejections_list.removeprefix("\n- ") if rejections_list.count('\n- ') == 1 else rejections_list
-            })
+            feedback(message=f"@{login}{rejections}{footer}")
-
+        if fw_rejections_list:
-        msg = []
+            rejections = ' ' + fw_rejections_list.removeprefix("\n- ") if fw_rejections_list.count('\n- ') else fw_rejections_list
-        if applied:
+            feedback(message=f"@{login}{rejections}{footer}", token='fp_github_token')
-            msg.append('applied ' + ' '.join(applied))
+        return 'rejected'
-        if ignored:
-            ignoredstr = ' '.join(ignored)
-            msg.append('ignored ' + ignoredstr)
-        return '\n'.join(msg)
 
     def _pr_acl(self, user):
         if not self:

runbot_merge/tests/test_basic.py

@@ -1222,7 +1222,7 @@ class TestRetry:
             (users['reviewer'], 'hansen r+'),
             (users['reviewer'], 'hansen retry'),
             seen(env, prx, users),
-            (users['user'], "I'm sorry, @{reviewer}: retry makes no sense when the PR is not in error.".format_map(users)),
+            (users['user'], "@{reviewer} retry makes no sense when the PR is not in error.".format_map(users)),
         ]
 
     @pytest.mark.parametrize('disabler', ['user', 'other', 'reviewer'])
@@ -2909,7 +2909,7 @@ class TestReviewing(object):
             (users['user'], "I'm sorry, @{}. I'm afraid I can't do that.".format(users['other'])),
             (users['reviewer'], 'hansen r+'),
             (users['reviewer'], 'hansen r+'),
-            (users['user'], "I'm sorry, @{}: this PR is already reviewed, reviewing it again is useless.".format(
+            (users['user'], "@{} this PR is already reviewed, reviewing it again is useless.".format(
                  users['reviewer'])),
         ]
 
@@ -2937,7 +2937,7 @@ class TestReviewing(object):
         assert prx.comments == [
             (users['reviewer'], 'hansen r+'),
             seen(env, prx, users),
-            (users['user'], "I'm sorry, @{}: you can't review+.".format(users['reviewer'])),
+            (users['user'], "@{} you can't review+. Skill issue.".format(users['reviewer'])),
         ]
 
     def test_self_review_success(self, env, repo, users, config):
@@ -3092,7 +3092,7 @@ class TestReviewing(object):
             seen(env, pr, users),
             (users['reviewer'], 'hansen delegate+'),
             (users['user'], 'hansen r+'),
-            (users['user'], f"I'm sorry, @{users['user']}: I must know your email before you can review PRs. Please contact an administrator."),
+            (users['user'], f"@{users['user']} I must know your email before you can review PRs. Please contact an administrator."),
         ]
         user_partner.fetch_github_email()
         assert user_partner.email
@@ -3394,6 +3394,8 @@ class TestRecognizeCommands:
             (users['reviewer'], "hansen do the thing"),
             (users['reviewer'], "hansen @bobby-b r+ :+1:"),
             seen(env, pr, users),
+            (users['user'], "@{reviewer} unknown command 'do'".format_map(users)),
+            (users['user'], "@{reviewer} unknown command '@bobby-b'".format_map(users)),
         ]
 
 class TestRMinus:

runbot_merge/tests/test_status_overrides.py

@@ -89,7 +89,7 @@ def test_basic(env, project, make_repo, users, setreviewers, config):
         (users['reviewer'], 'hansen r+'),
         seen(env, pr, users),
         (users['reviewer'], 'hansen override=l/int'),
-        (users['user'], "I'm sorry, @{}: you are not allowed to override this status.".format(users['reviewer'])),
+        (users['user'], "@{} you are not allowed to override 'l/int'.".format(users['reviewer'])),
         (users['other'], "hansen override=l/int"),
     ]
     assert pr_id.statuses == '{}'