You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@magpie.apache.org by po...@apache.org on 2026/10/10 08:44:18 UTC
(magpie) branch perf/pr-mgmt-10-mention-allowlist created (now 5f519c3a)
This is an automated email from the ASF dual-hosted git repository.
potiuk pushed a change to branch perf/pr-mgmt-10-mention-allowlist
in repository https://gitbox.apache.org/repos/asf/magpie.git
at 5f519c3a feat(pr-management): let a project allow specific @-mentions everywhere
This branch includes the following new commits:
new 5f519c3a feat(pr-management): let a project allow specific @-mentions everywhere
The 1 revisions listed above as "new" are entirely new to this
repository and will be described in separate emails. The revisions
listed as "add" were already present in the repository and have only
been added to this reference.
(magpie) 01/01: feat(pr-management): let a project allow specific @-mentions everywhere
Posted by po...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
potiuk pushed a commit to branch perf/pr-mgmt-10-mention-allowlist
in repository https://gitbox.apache.org/repos/asf/magpie.git
commit 5f519c3a77d38deed259c650af12c10b1b393105
Author: Jarek Potiuk <po...@apache.org>
AuthorDate: Sat Oct 10 08:15:38 2026 +0100
feat(pr-management): let a project allow specific @-mentions everywhere
Every pr-management renderer keeps only the PR author's @-mention live
and backtick-quotes every other handle, and the agent-guard `mention`
guard refuses the rest. There was no way for a project to say "this
bot / this team may be mentioned" short of a per-command override.
A new `mention_allowlist` key in `pr-management-config.md` names the
handles and teams that stay live, honoured by one shared helper in
every renderer — triage notes, stale-sweep comments, mentor comments
and code-review bodies — and by the guard on every surface it checks
(comments, folded notes, reviews, `gh api` writes). One call can add a
handle with `--allow-mention <login>`, paired with
`MAGPIE_ALLOW_MENTIONS=1` on the posting command so the exception is
explicit in both places.
The guard reads the allowlist, and the mentoring hand-off team, from
`.apache-magpie-overrides/` as GitHub serves it from the target
repository's default branch — not from the local checkout, which the
agent can edit, commit to, or swap for a scratch repository. Only a
reviewed, merged change can widen what may be mentioned. The target
must be explicit and unambiguous: `--repo` and/or a PR or issue URL as
the selector, all agreeing, or a plain `repos/OWNER/REPO/…` endpoint
for `gh api`; there is no fallback to the checkout's repository, a URL
in body text does not count, an endpoint with `.`/`..` segments,
percent-encoding or `{owner}` placeholders is refused, and GraphQL
writes get no allowlist. This replaces the local committed-file check
the hand-off exemption used.
Generated-by: Claude Opus 5
Claude-Session: https://claude.ai/code/session_01GTPsZ5aEE5bUVYqp47Dpvv
---
.../mentor/classifications/hand-off-comment.md | 2 +-
.../magpie-pr-management/skills/pr-triage/SKILL.md | 9 +-
.../skills/pr-triage/actions/deliver-note.md | 1 +
.../skills/pr-triage/guards/mention.py | 27 ++-
.../setup/setup_preflight/isolated_fingerprint.py | 2 +-
.../magpie-setup/templates/pr-management-config.md | 1 +
tools/agent-guard/src/agent_guard/__init__.py | 146 +++++++++++---
tools/agent-guard/tests/test_skill_guards.py | 215 ++++++++++++++-------
tools/pr-management/README.md | 2 +
tools/pr-management/src/pr_management/cli.py | 5 +-
.../src/pr_management/code_review/body.py | 25 ++-
.../src/pr_management/code_review/cli.py | 17 +-
tools/pr-management/src/pr_management/config.py | 5 +
tools/pr-management/src/pr_management/mentions.py | 59 ++++++
.../pr-management/src/pr_management/mentor/cli.py | 6 +-
.../src/pr_management/mentor/render.py | 3 +-
.../src/pr_management/stale_sweep/cli.py | 5 +-
.../src/pr_management/stale_sweep/render.py | 3 +-
.../src/pr_management/triage/render.py | 12 +-
tools/pr-management/tests/test_mentions.py | 71 +++++++
20 files changed, 482 insertions(+), 134 deletions(-)
diff --git a/plugins/magpie-pr-management/skills/mentor/classifications/hand-off-comment.md b/plugins/magpie-pr-management/skills/mentor/classifications/hand-off-comment.md
index bd31b673..ad04fb73 100644
--- a/plugins/magpie-pr-management/skills/mentor/classifications/hand-off-comment.md
+++ b/plugins/magpie-pr-management/skills/mentor/classifications/hand-off-comment.md
@@ -20,7 +20,7 @@ One template for all four triggers. It does **not** summarise the conversation
gh issue comment <N> --repo <upstream> --body-file <scratch>/handoff.md
```
- (`gh pr comment` for a PR.) The agent-guard `mention` guard lets this one team mention through: the team is the `maintainer_team_handle` in the committed `.apache-magpie-overrides/mentoring-config.md`, and the body opens with the template's `— handing this off:` line. Any other mention in the body is still blocked.
+ (`gh pr comment` for a PR.) The agent-guard `mention` guard lets this one team mention through: the team is the `maintainer_team_handle` in `.apache-magpie-overrides/mentoring-config.md` as GitHub serves it from the target repository's default branch (a local edit does not count), it belongs to the organisation that owns that repository, and the body opens with the template's `— handing this off:` line. Any other mention in the body is still blocked.
5. Record it: `uv run --project <framework>/tools/pr-management pr-management mentor log --kind <pr|issue> --number <N> --outcome handed-off --trigger <n>`. The skill does not return to a handed-off thread.
The hand-off never contains a summary of the thread, a guess at what the maintainer should do, an apology, a retry offer, or a further doc link: anything added pre-loads the maintainer's read of the conversation.
diff --git a/plugins/magpie-pr-management/skills/pr-triage/SKILL.md b/plugins/magpie-pr-management/skills/pr-triage/SKILL.md
index 2d1ba3a0..8a8fe169 100644
--- a/plugins/magpie-pr-management/skills/pr-triage/SKILL.md
+++ b/plugins/magpie-pr-management/skills/pr-triage/SKILL.md
@@ -27,7 +27,7 @@ argument-hint: "[pr:N] [label:LBL] [author:LOGIN] [review-for-me] [stale] [repo:
capability: capability:triage
surface_hash: sha256:54dc4ba1be7b36d4
license: Apache-2.0
-measured_tokens: 4869
+measured_tokens: 4920
---
<!-- SPDX-License-Identifier: Apache-2.0
https://www.apache.org/licenses/LICENSE-2.0 -->
@@ -292,8 +292,11 @@ author; under `comment` it posts as a comment. The delivery is in
**Golden rule 11 — the note notifies the author, and only the author.**
Only the author is `@`-mentioned and assigned; every maintainer
handle is backtick-quoted. The renderer guarantees it and the
-agent-guard `mention` guard enforces it. Exemption: on your own
-PR, mentioning your reviewers is allowed.
+agent-guard `mention` guard enforces it. Exemptions: on your own
+PR, mentioning your reviewers is allowed; a project's committed
+`mention_allowlist` keeps the handles it names live everywhere;
+and a maintainer can allow one handle for one note with
+`--allow-mention` plus `MAGPIE_ALLOW_MENTIONS=1`.
---
diff --git a/plugins/magpie-pr-management/skills/pr-triage/actions/deliver-note.md b/plugins/magpie-pr-management/skills/pr-triage/actions/deliver-note.md
index 139fd008..6745613f 100644
--- a/plugins/magpie-pr-management/skills/pr-triage/actions/deliver-note.md
+++ b/plugins/magpie-pr-management/skills/pr-triage/actions/deliver-note.md
@@ -27,6 +27,7 @@ Write the group entry's `details` object from the classify output to `<scratch>/
The result names the `body_file`, the `channel`, whether to `assign_author` / `unassign_author`, and a `preview`.
Show the maintainer the preview and the channel before the mutation: under `pr-body` the note folds into the description (only the author is notified); under `comment` it posts as a comment (every subscriber is notified).
Never edit the rendered body to add a mention, and never post a `warnings` entry about an unresolved placeholder — fix the config value instead.
+To keep a handle live, it goes in the project's `mention_allowlist`, or — when the maintainer asks for it on this one note — `--allow-mention <login>` on the render and `MAGPIE_ALLOW_MENTIONS=1` on the posting command.
## 2a. Channel `pr-body` (default) — fold into the description
diff --git a/plugins/magpie-pr-management/skills/pr-triage/guards/mention.py b/plugins/magpie-pr-management/skills/pr-triage/guards/mention.py
index 07a6966d..d0288d0d 100644
--- a/plugins/magpie-pr-management/skills/pr-triage/guards/mention.py
+++ b/plugins/magpie-pr-management/skills/pr-triage/guards/mention.py
@@ -39,12 +39,19 @@ Two further surfaces:
the triage note.
* **Uninspectable text** (a body on stdin, an unreadable body or field
file) is refused: the guard cannot confirm it is clean.
+* **The project allowlist**: handles in `mention_allowlist` of
+ `.apache-magpie-overrides/pr-management-config.md` *as GitHub serves it
+ from the target repository's default branch* are never refused, on any
+ surface — the same key every `pr-management` renderer honours. Only a
+ reviewed, merged change can put a handle there; a local edit, commit or
+ scratch repository cannot.
* **The mentoring hand-off**: `pr-management-mentor` hands a thread to the
maintainers with a body that opens "@<team> — handing this off:". That one
mention is allowed when <team> is the `maintainer_team_handle` of the
- *committed* `.apache-magpie-overrides/mentoring-config.md` (tracked and
- identical to `HEAD`), belongs to the organisation that owns the target
- repository, and is the only non-author mention in the body.
+ `.apache-magpie-overrides/mentoring-config.md` on the target
+ repository's default branch (read from GitHub, like the allowlist),
+ belongs to the organisation that owns the target repository, and is the
+ only non-author mention in the body.
Discovered by
the agent-guard PreToolUse dispatcher from a guards.d directory — see
@@ -75,6 +82,12 @@ def _nobody(what, mentions):
)
+def _allowlist(ctx):
+ """Handles the project lets every surface keep live: the committed `mention_allowlist`."""
+ values = ctx.committed_config_values("pr-management-config.md", "mention_allowlist")
+ return {v.strip().lstrip("@").lower() for v in values if v.strip()}
+
+
def guard(ctx):
# Anything a non-GET `gh api` call sends, whatever the endpoint. No author
# exemption here.
@@ -85,7 +98,7 @@ def guard(ctx):
texts, readable = api
if not readable:
return _uninspectable("API call")
- mentions = sorted({m for text in texts for m in ctx.mentions(text)})
+ mentions = sorted({m for text in texts for m in ctx.mentions(text)} - _allowlist(ctx))
return _nobody("text sent by `gh api`", mentions) if mentions else None
sub = ctx.gh_subcommand()
@@ -97,7 +110,7 @@ def guard(ctx):
body = ctx.gh_body(read_files=True)
if ctx.opt("-F", "--body-file") == "-" or UNREADABLE in body:
return _uninspectable("review")
- mentions = sorted(set(ctx.mentions(body)))
+ mentions = sorted(set(ctx.mentions(body)) - _allowlist(ctx))
return _nobody("a review body", mentions) if mentions else None
sub = ctx.gh_subcommand()
@@ -119,7 +132,7 @@ def guard(ctx):
if ctx.opt("-F", "--body-file") == "-" or UNREADABLE in body:
return _uninspectable("folded triage note" if is_pr_body_edit else "comment")
mentions = ctx.mentions(body)
- if not mentions:
+ if not set(mentions) - _allowlist(ctx):
return None
# Both channels share one rule: only the PR/issue author may be @-mentioned.
@@ -147,7 +160,7 @@ def guard(ctx):
operator = ctx.run(["gh", "api", "user", "--jq", ".login"])
if operator and operator.lower() == author.lower():
return None
- offenders = sorted({m for m in mentions if m != author.lower()})
+ offenders = sorted({m for m in mentions if m != author.lower()} - _allowlist(ctx))
if offenders and is_comment and _is_mentoring_handoff(ctx, offenders):
return None
if offenders:
diff --git a/plugins/magpie-setup/skills/setup/setup_preflight/isolated_fingerprint.py b/plugins/magpie-setup/skills/setup/setup_preflight/isolated_fingerprint.py
index fd5fb57e..a522f649 100644
--- a/plugins/magpie-setup/skills/setup/setup_preflight/isolated_fingerprint.py
+++ b/plugins/magpie-setup/skills/setup/setup_preflight/isolated_fingerprint.py
@@ -21,4 +21,4 @@ The isolated-setup fingerprint of the framework this package shipped with,
for installs that do not carry the framework source (see `isolated.py`).
"""
-FRAMEWORK_FINGERPRINT = "sha256:3c8da0430c35995e"
+FRAMEWORK_FINGERPRINT = "sha256:8697f4a8371559a4"
diff --git a/plugins/magpie-setup/templates/pr-management-config.md b/plugins/magpie-setup/templates/pr-management-config.md
index 362aa338..650ded13 100644
--- a/plugins/magpie-setup/templates/pr-management-config.md
+++ b/plugins/magpie-setup/templates/pr-management-config.md
@@ -88,6 +88,7 @@ default to use the standard variant.
| `backport_branches` | *(empty)* | Base-branch patterns (e.g. `v*-test`, `release/*`) that receive only cherry-picks from the default branch. Enables the [backport check](../../magpie-pr-management/skills/pr-triage/backport-check.md) (Step 0.7) for PRs targeting them. Leave empty if the project does not cherry-pick. |
| `backport_policy` | `fixes-only` | What a backport may carry. `fixes-only`: flag features, behaviour changes, new deprecations, removals and refactors for closing. `any`: skip the change-type check and only verify the backport is a faithful cherry-pick. |
| `session_history_gist` | `enabled` | [Step 6b](../../../skills/pr-management-triage/session-history.md#step-6b--propose-session-history-gist-update) — propose appending each session to a private GitHub gist on the maintainer's account. Set to `disabled` to skip Step 6b unconditionally for this project (overrides the per-invocation `no-history` flag). The local state file (`session-state.json` in the personal config layer) is read regardless so an existing gist remains discoverable. See [...]
+| `mention_allowlist` | *(empty)* | Handles and teams every `pr-management` body may `@`-mention live (e.g. `` `@release-bot` `@acme/docs-team` ``). By default only the PR author is mentioned and every other handle is backtick-quoted. The renderers read this from any config layer; the agent-guard `mention` guard honours it only from `.apache-magpie-overrides/pr-management-config.md` on the target repository's default branch on GitHub, so it takes effect once merged there. One call can ad [...]
## CI check patterns
diff --git a/tools/agent-guard/src/agent_guard/__init__.py b/tools/agent-guard/src/agent_guard/__init__.py
index 421bdd7e..836f2488 100644
--- a/tools/agent-guard/src/agent_guard/__init__.py
+++ b/tools/agent-guard/src/agent_guard/__init__.py
@@ -462,6 +462,46 @@ def _find_repo_root(start: Path) -> Path | None:
LOCK_NAME = ".apache-magpie.lock"
LOCAL_DIR = ".apache-magpie-local"
OVERRIDES_DIR = ".apache-magpie-overrides"
+
+_REPO_SLUG = re.compile(r"[A-Za-z0-9][A-Za-z0-9._-]*/[A-Za-z0-9][A-Za-z0-9._-]*")
+
+#: ``gh api`` flags that take a separate value, so the endpoint is the first other token.
+_GH_API_VALUE_FLAGS = frozenset(
+ {
+ "-X",
+ "--method",
+ "-f",
+ "--raw-field",
+ "-F",
+ "--field",
+ "-H",
+ "--header",
+ "--input",
+ "-q",
+ "--jq",
+ "-t",
+ "--template",
+ "--hostname",
+ "--cache",
+ "-p",
+ "--preview",
+ }
+)
+
+
+def _gh_api_endpoint(args: list[str]) -> str | None:
+ """The endpoint positional of a ``gh api`` argument list (everything after ``api``)."""
+ skip = False
+ for tok in args:
+ if skip:
+ skip = False
+ elif tok in _GH_API_VALUE_FLAGS:
+ skip = True
+ elif not tok.startswith("-"):
+ return tok
+ return None
+
+
GIT_HOME_NAME = "apache-magpie"
@@ -831,36 +871,48 @@ class GuardContext:
def mentions(self, text: str) -> list[str]:
return find_mentions(text)
- def committed_config_value(self, filename: str, key: str) -> str | None:
- """A `` | `key` | value | `` row from the **committed** project config.
+ def _committed_config_text(self, filename: str) -> str | None:
+ """``.apache-magpie-overrides/<filename>`` on the target repository's default branch, on GitHub.
- A value that widens what a guard allows must come from a file the
- agent cannot simply write: ``.apache-magpie-overrides/<filename>`` in
- the repository the hook runs in, tracked by git, not a symlink, and
- byte-identical to its ``HEAD`` version (an uncommitted edit, or a
- file in a scratch repository the agent made, does not count).
+ A value that widens what a guard allows must come from somewhere the
+ agent cannot write. Anything local fails that test — the agent can
+ edit a file, commit it, or ``cd`` into a scratch repository it made —
+ so the guard reads the copy GitHub serves from the default branch of
+ the repository the command posts to, which only a reviewed, merged
+ change can alter. A fork or scratch repository only ever governs
+ itself. Unreadable → None, and the guard falls back to the strict rule.
"""
- root = _find_repo_root(Path(self.cwd or os.getcwd()).resolve())
- if root is None:
- return None
- rel = f"{OVERRIDES_DIR}/{filename}"
- path = root / rel
- if path.is_symlink() or not path.is_file():
+ repo = self.target_repo()
+ if repo is None:
return None
- try:
- text = path.read_text(encoding="utf-8")
- except OSError:
- return None
- committed = _run(["git", "show", f"HEAD:{rel}"], cwd=str(root))
- if committed is None or committed.strip() != text.strip():
- return None
- for line in text.splitlines():
+ return self.run(
+ [
+ "gh",
+ "api",
+ f"repos/{repo}/contents/{OVERRIDES_DIR}/{filename}",
+ "-H",
+ "Accept: application/vnd.github.raw",
+ ]
+ )
+
+ def _committed_row(self, filename: str, key: str) -> str | None:
+ text = self._committed_config_text(filename)
+ for line in (text or "").splitlines():
cells = [c.strip() for c in line.strip().strip("|").split("|")]
if len(cells) >= 2 and cells[0] == f"`{key}`":
- match = re.match(r"`([^`]+)`", cells[1])
- return match.group(1).strip() if match else None
+ return cells[1]
return None
+ def committed_config_value(self, filename: str, key: str) -> str | None:
+ """The first backticked value of a `` | `key` | value | `` row of the config on the target repository's default branch."""
+ cell = self._committed_row(filename, key)
+ match = re.match(r"`([^`]+)`", cell or "")
+ return match.group(1).strip() if match else None
+
+ def committed_config_values(self, filename: str, key: str) -> list[str]:
+ """Every backticked value of a `` | `key` | value | `` row of the config on the target repository's default branch."""
+ return [t.strip() for t in re.findall(r"`([^`]+)`", self._committed_row(filename, key) or "")]
+
def gh_input_json(self) -> object | None:
"""The JSON payload of ``gh api --input <file>``, or None."""
path = self.opt("--input", "--input")
@@ -871,13 +923,49 @@ class GuardContext:
except (OSError, ValueError):
return None
+ def target_repo(self) -> str | None:
+ """``OWNER/REPO`` a ``gh`` command explicitly acts on, or None when that is not certain.
+
+ Only guards that *widen* a rule ask, so anything ambiguous answers
+ None and the strict rule stands. ``gh api`` acts on the repository in
+ a plain ``repos/OWNER/REPO/…`` endpoint; ``graphql`` and any other
+ endpoint can reach any repository, and an endpoint with ``.`` / ``..``
+ segments, percent-encoding or ``{owner}`` placeholders is refused
+ rather than resolved. Every other command must name its repository —
+ through ``--repo`` and/or a PR / issue URL as the selector
+ positional — and all of them must agree. There is no fallback to the
+ checkout: a ``cd`` or ``GH_REPO`` earlier on the line would change
+ what ``gh`` posts to without the guard seeing it.
+ """
+ sub = self.gh_subcommand()
+ if sub is not None and sub[0] == "api" and "api" in self.argv:
+ endpoint = _gh_api_endpoint(self.argv[self.argv.index("api") + 1 :]) or ""
+ path = endpoint.split("?", 1)[0].split("#", 1)[0]
+ if any(c in path for c in "%{}\\") or any(seg in (".", "..") for seg in path.split("/")):
+ return None
+ m = re.fullmatch(r"/?repos/([^/]+/[^/]+)(?:/.*)?", path)
+ found = {m.group(1).lower()} if m else set()
+ else:
+ found = {r.lower() for r in self.opts("-R", "--repo")}
+ # Only the selector positional (`gh pr comment <URL>`) names a
+ # repository; a URL inside --body or any other value is text.
+ selector = self.positional_after(sub[1]) if sub is not None else None
+ m = re.fullmatch(
+ r"https?://(?:www\.)?github\.com/([^/]+/[^/#?]+)/(?:pull|issues)/\d+(?:[/#?].*)?",
+ selector or "",
+ re.IGNORECASE,
+ )
+ if m:
+ found.add(m.group(1).lower())
+ if len(found) != 1:
+ return None
+ repo = found.pop()
+ return repo if _REPO_SLUG.fullmatch(repo) else None
+
def target_repo_owner(self) -> str | None:
- """The owner of the repository a ``gh`` command acts on: ``--repo``, else the checkout's."""
- repo = self.opt("-R", "--repo")
- if repo:
- return repo.split("/", 1)[0].lower() if "/" in repo else None
- owner = self.run(["gh", "repo", "view", "--json", "owner", "--jq", ".owner.login"])
- return owner.lower() if owner else None
+ """The owner of the repository a ``gh`` command acts on (see :meth:`target_repo`)."""
+ repo = self.target_repo()
+ return repo.split("/", 1)[0] if repo else None
def gh_api_posted_text(self) -> tuple[list[str], bool] | None:
"""Every value a non-``GET`` ``gh api`` call would send, and whether all of it was readable.
diff --git a/tools/agent-guard/tests/test_skill_guards.py b/tools/agent-guard/tests/test_skill_guards.py
index bec936c8..922f18dc 100644
--- a/tools/agent-guard/tests/test_skill_guards.py
+++ b/tools/agent-guard/tests/test_skill_guards.py
@@ -184,99 +184,178 @@ def _git(repo, *args):
subprocess.run(["git", "-C", str(repo), *args], check=True, capture_output=True)
-def _repo_with_team(tmp_path, handle, *, commit=True):
- _git(tmp_path, "init", "-q")
- overrides = tmp_path / ".apache-magpie-overrides"
- overrides.mkdir()
- (overrides / "mentoring-config.md").write_text(
- f"| Key | Value | Notes |\n|---|---|---|\n| `maintainer_team_handle` | `{handle}` | x |\n",
- encoding="utf-8",
- )
- if commit:
- _git(tmp_path, "add", "-A")
- _git(
- tmp_path,
- "-c",
- "user.name=t",
- "-c",
- "user.email=t@example.org",
- "-c",
- "commit.gpgsign=false",
- "commit",
- "-q",
- "-m",
- "config",
- )
- return tmp_path
+def served(files, *, author="alice", repo="acme/product"):
+ """A `_run` stub whose GitHub serves `files` (name → text) from `repo`'s default branch.
+ The guard reads widening config only from there; any other repository,
+ or a file not listed, is not found.
+ """
+ base = gh_stub(author=author)
-def _handoff(repo, body, repo_flag="--repo acme/product"):
- f = repo / "b.md"
- f.write_text(body, encoding="utf-8")
- return agent_guard.dispatch(f"gh issue comment 7 {repo_flag} --body-file {f}", cwd=str(repo))
+ def _stub(args, cwd=None):
+ for a in args:
+ if "/contents/.apache-magpie-overrides/" in a:
+ prefix = f"repos/{repo}/contents/.apache-magpie-overrides/"
+ return files.get(a[len(prefix) :]) if a.startswith(prefix) else None
+ if args[:3] == ["gh", "repo", "view"]:
+ return repo
+ return base(args)
+ return _stub
-def test_mentoring_handoff_may_mention_the_configured_team(monkeypatch, tmp_path):
- repo = _repo_with_team(tmp_path, "@acme/committers")
- monkeypatch.setattr(agent_guard, "_run", _with_git(gh_stub(author="alice")))
- assert _handoff(repo, "@acme/committers — handing this off: should this be a plugin?") is None
+def _team(handle):
+ return {
+ "mentoring-config.md": f"| Key | Value | Notes |\n|---|---|---|\n| `maintainer_team_handle` | `{handle}` | x |\n"
+ }
-def _with_git(gh_handler):
- """Real `git` (the committed-config check needs it), stubbed `gh`."""
- import subprocess
- def _stub(args, cwd=None):
- if args and args[0] == "git":
- done = subprocess.run(args, cwd=cwd, capture_output=True, text=True)
- return done.stdout.strip() if done.returncode == 0 else None
- return gh_handler(args)
+def _allow(*handles):
+ cell = " ".join(f"`{h}`" for h in handles)
+ return {
+ "pr-management-config.md": f"| Key | Value | Notes |\n|---|---|---|\n| `mention_allowlist` | {cell} | x |\n"
+ }
- return _stub
+
+def _handoff(tmp_path, body, repo_flag="--repo acme/product"):
+ f = tmp_path / "b.md"
+ f.write_text(body, encoding="utf-8")
+ return agent_guard.dispatch(f"gh issue comment 7 {repo_flag} --body-file {f}", cwd=str(tmp_path))
+
+
+def test_mentoring_handoff_may_mention_the_configured_team(monkeypatch, tmp_path):
+ monkeypatch.setattr(agent_guard, "_run", served(_team("@acme/committers")))
+ assert _handoff(tmp_path, "@acme/committers — handing this off: should this be a plugin?") is None
def test_handoff_exemption_needs_the_template_opening(monkeypatch, tmp_path):
- repo = _repo_with_team(tmp_path, "@acme/committers")
- monkeypatch.setattr(agent_guard, "_run", _with_git(gh_stub(author="alice")))
- reason = _handoff(repo, "hey @acme/committers look")
+ monkeypatch.setattr(agent_guard, "_run", served(_team("@acme/committers")))
+ reason = _handoff(tmp_path, "hey @acme/committers look")
assert reason and "acme/committers" in reason
def test_handoff_exemption_covers_the_team_only(monkeypatch, tmp_path):
- repo = _repo_with_team(tmp_path, "@acme/committers")
- monkeypatch.setattr(agent_guard, "_run", _with_git(gh_stub(author="alice")))
- reason = _handoff(repo, "@acme/committers — handing this off: also @bob")
+ monkeypatch.setattr(agent_guard, "_run", served(_team("@acme/committers")))
+ reason = _handoff(tmp_path, "@acme/committers — handing this off: also @bob")
assert reason and "bob" in reason
-def test_handoff_ignores_an_uncommitted_team(monkeypatch, tmp_path):
- repo = _repo_with_team(tmp_path, "@acme/committers", commit=False)
- monkeypatch.setattr(agent_guard, "_run", _with_git(gh_stub(author="alice")))
- assert _handoff(repo, "@acme/committers — handing this off: q")
+def test_handoff_never_reaches_another_organisation(monkeypatch, tmp_path):
+ monkeypatch.setattr(agent_guard, "_run", served(_team("@other-org/committers")))
+ assert _handoff(tmp_path, "@other-org/committers — handing this off: q")
-def test_handoff_ignores_an_edited_committed_team(monkeypatch, tmp_path):
- repo = _repo_with_team(tmp_path, "@acme/committers")
- (repo / ".apache-magpie-overrides" / "mentoring-config.md").write_text(
- "| `maintainer_team_handle` | `@acme/other` | x |\n"
+def _local_commit(tmp_path, name, text):
+ """A config the agent wrote and committed itself, in a checkout it controls."""
+ _git(tmp_path, "init", "-q")
+ overrides = tmp_path / ".apache-magpie-overrides"
+ overrides.mkdir(exist_ok=True)
+ (overrides / name).write_text(text, encoding="utf-8")
+ _git(tmp_path, "add", "-A")
+ _git(
+ tmp_path,
+ "-c",
+ "user.name=t",
+ "-c",
+ "user.email=t@example.org",
+ "-c",
+ "commit.gpgsign=false",
+ "commit",
+ "-q",
+ "-m",
+ "config",
)
- monkeypatch.setattr(agent_guard, "_run", _with_git(gh_stub(author="alice")))
- assert _handoff(repo, "@acme/other — handing this off: q")
-def test_handoff_never_reaches_another_organisation(monkeypatch, tmp_path):
- repo = _repo_with_team(tmp_path, "@other-org/committers")
- monkeypatch.setattr(agent_guard, "_run", _with_git(gh_stub(author="alice")))
- assert _handoff(repo, "@other-org/committers — handing this off: q")
+def test_handoff_ignores_a_team_committed_only_locally(monkeypatch, tmp_path):
+ ((name, text),) = _team("@acme/committers").items()
+ _local_commit(tmp_path, name, text)
+ monkeypatch.setattr(agent_guard, "_run", served({}))
+ assert _handoff(tmp_path, "@acme/committers — handing this off: q")
-def test_handoff_ignores_a_team_set_only_in_the_personal_layer(monkeypatch, tmp_path):
- _git(tmp_path, "init", "-q")
- local = tmp_path / ".apache-magpie-local"
- local.mkdir()
- (local / "mentoring-config.md").write_text("| `maintainer_team_handle` | `@acme/x` | x |\n")
- monkeypatch.setattr(agent_guard, "_run", _with_git(gh_stub(author="alice")))
- assert _handoff(tmp_path, "@acme/x — handing this off: q")
+def test_handoff_ignores_a_team_served_by_another_repository(monkeypatch, tmp_path):
+ monkeypatch.setattr(agent_guard, "_run", served(_team("@acme/committers"), repo="alice/product"))
+ assert _handoff(tmp_path, "@acme/committers — handing this off: q")
+
+
+def test_allowlisted_handle_passes_in_a_comment(monkeypatch, tmp_path):
+ monkeypatch.setattr(agent_guard, "_run", served(_allow("@release-bot", "@acme/docs-team")))
+ assert (
+ agent_guard.dispatch(
+ 'gh pr comment 5 --repo acme/product --body "@alice see @release-bot"', cwd=str(tmp_path)
+ )
+ is None
+ )
+ reason = agent_guard.dispatch(
+ 'gh pr comment 5 --repo acme/product --body "@release-bot and @bob"', cwd=str(tmp_path)
+ )
+ assert reason and "bob" in reason and "release-bot" not in reason
+
+
+def test_allowlisted_handle_passes_in_reviews_and_api_writes(monkeypatch, tmp_path):
+ monkeypatch.setattr(agent_guard, "_run", served(_allow("@acme/docs-team")))
+ assert (
+ agent_guard.dispatch(
+ 'gh pr review 5 -R acme/product --comment --body "cc @acme/docs-team"', cwd=str(tmp_path)
+ )
+ is None
+ )
+ assert (
+ agent_guard.dispatch(
+ 'gh api repos/acme/product/issues/5/comments -f body="cc @acme/docs-team"', cwd=str(tmp_path)
+ )
+ is None
+ )
+
+
+def test_a_locally_committed_allowlist_is_ignored(monkeypatch, tmp_path):
+ ((name, text),) = _allow("@bob").items()
+ _local_commit(tmp_path, name, text)
+ monkeypatch.setattr(agent_guard, "_run", served({}))
+ assert agent_guard.dispatch('gh pr comment 5 --repo acme/product --body "@bob"', cwd=str(tmp_path))
+
+
+def test_the_allowlist_of_the_api_endpoint_repository_governs(monkeypatch, tmp_path):
+ """An API write to another repository does not borrow the checkout's allowlist."""
+ monkeypatch.setattr(agent_guard, "_run", served(_allow("@bob")))
+ assert agent_guard.dispatch(
+ 'gh api repos/victim/other/issues/5/comments -f body="@bob"', cwd=str(tmp_path)
+ )
+
+
+@pytest.mark.parametrize(
+ "command",
+ [
+ 'gh pr comment 5 --body "@bob"',
+ 'gh pr comment https://github.com/victim/other/pull/5 --repo acme/product --body "@bob"',
+ 'gh pr comment 5 --repo acme/product --repo victim/other --body "@bob"',
+ 'gh api repos/acme/product/../../victim/other/issues/5/comments -f body="@bob"',
+ 'gh api repos/acme/product%2F..%2F../victim/issues/5/comments -f body="@bob"',
+ 'gh api repos/{owner}/{repo}/issues/5/comments -f body="@bob"',
+ 'gh pr comment 5 --body "@bob, see https://github.com/acme/product/pull/1"',
+ ],
+)
+def test_no_allowlist_unless_the_target_is_explicit_and_unambiguous(monkeypatch, tmp_path, command):
+ monkeypatch.setattr(agent_guard, "_run", served(_allow("@bob")))
+ assert agent_guard.dispatch(command, cwd=str(tmp_path))
+
+
+def test_a_pr_url_names_the_target(monkeypatch, tmp_path):
+ monkeypatch.setattr(agent_guard, "_run", served(_allow("@bob")))
+ assert (
+ agent_guard.dispatch(
+ 'gh pr comment https://github.com/acme/product/pull/5 --body "@alice @bob"', cwd=str(tmp_path)
+ )
+ is None
+ )
+
+
+def test_graphql_writes_get_no_allowlist(monkeypatch, tmp_path):
+ monkeypatch.setattr(agent_guard, "_run", served(_allow("@bob")))
+ assert agent_guard.dispatch(
+ 'gh api graphql -f query="mutation { x(body: \\"@bob\\") }"', cwd=str(tmp_path)
+ )
# --- bodies the guard cannot read, and text posted through `gh api` --------- #
diff --git a/tools/pr-management/README.md b/tools/pr-management/README.md
index 6ac50bb3..d2847ec9 100644
--- a/tools/pr-management/README.md
+++ b/tools/pr-management/README.md
@@ -298,6 +298,8 @@ Other skills link here instead of restating them; change a rule in code and here
- **Systemic failure** — a check name failing on at least two of the ten most recently merged PRs.
- **Mergeability** — `UNKNOWN` is "not yet computed", never "mergeable": passing needs `MERGEABLE`, and every guard refuses on `UNKNOWN`.
- **Strip-ready-on-downgrade** — a ready-labelled PR that regresses into a `deterministic_flag` `draft` / `comment` / `close` loses the label, unless a collaborator thread is open (a merit discussion): then `draft` becomes feedback only, `close` delivers its reasoning and label but leaves the PR open, and the label stays.
+- **Mentions** ([`mentions.py`](src/pr_management/mentions.py)) — every renderer (triage notes, stale-sweep comments, mentor comments, code-review bodies) keeps only the PR author live and backtick-quotes every other handle, except those in `mention_allowlist` (`pr-management-config.md`) and those one call adds with `--allow-mention <login>`.
+ The agent-guard `mention` guard honours `mention_allowlist` only from `.apache-magpie-overrides/pr-management-config.md` as GitHub serves it from the target repository's default branch, so a local edit or commit does not widen it; a per-call addition also needs `MAGPIE_ALLOW_MENTIONS=1` on the posting command.
- **Reason strings** — one line, factual, signal first and proposal verb last; no emoji, no editorialising, no generated prose. The table's templates are the whole surface.
- **Routing score** — area 3 each (cap 6), familiarity 2 per path (cap 6), CODEOWNERS 2, load −1 per review above 2 (floor −5); an OVERLOADED member is never the primary.
diff --git a/tools/pr-management/src/pr_management/cli.py b/tools/pr-management/src/pr_management/cli.py
index f149184e..65580c69 100644
--- a/tools/pr-management/src/pr_management/cli.py
+++ b/tools/pr-management/src/pr_management/cli.py
@@ -34,7 +34,7 @@ from fnmatch import fnmatchcase
from pathlib import Path
from typing import Any
-from . import ci, config, mdconfig, model, people
+from . import ci, config, mdconfig, mentions, model, people
from .triage import classify as triage_classify
from .triage import fold as triage_fold
from .triage import guards as triage_guards
@@ -139,7 +139,7 @@ def _triage_classify(args: argparse.Namespace) -> dict[str, Any]:
def _triage_render(args: argparse.Namespace) -> dict[str, Any]:
- cfg = config.load(args.project_root, args.config_dir)
+ cfg = mentions.apply_flag(config.load(args.project_root, args.config_dir), args)
resolver = mdconfig.Resolver(args.project_root, args.config_dir)
saved: Path = args.saved_dir
pages = [Path(p) for p in args.pages] or [saved / SAVED["pages"]]
@@ -369,6 +369,7 @@ def main(argv: Sequence[str] | None = None) -> int:
grd.add_argument("--saved-dir", type=Path, required=True)
ren = triage_sub.add_parser("render", help="render the contributor-facing body of one action")
+ mentions.add_flag(ren)
ren.add_argument(
"--saved-dir", type=Path, required=True, help="the vetted-ops workspace's saved/ directory"
)
diff --git a/tools/pr-management/src/pr_management/code_review/body.py b/tools/pr-management/src/pr_management/code_review/body.py
index 5c0d5f86..e39494e1 100644
--- a/tools/pr-management/src/pr_management/code_review/body.py
+++ b/tools/pr-management/src/pr_management/code_review/body.py
@@ -86,7 +86,7 @@ def verify_footer(body: str, variant: str | None = None) -> dict[str, Any]:
return {"footer_present": False, "variant": None, "action": "block"}
-def escape_handles(text: str) -> str:
+def escape_handles(text: str, allowed: frozenset[str] = frozenset()) -> str:
"""Backtick every live `@handle` outside code, so posting it notifies nobody."""
out = []
inside = False
@@ -99,12 +99,19 @@ def escape_handles(text: str) -> str:
out.append(line)
continue
pieces = re.split(r"(`[^`]*`)", line)
- pieces = [p if p.startswith("`") else _MENTION.sub(lambda m: f"`@{m.group(1)}`", p) for p in pieces]
+ pieces = [
+ p
+ if p.startswith("`")
+ else _MENTION.sub(
+ lambda m: m.group(0) if m.group(1).lower() in allowed else f"`@{m.group(1)}`", p
+ )
+ for p in pieces
+ ]
out.append("".join(pieces))
return "\n".join(out)
-def mention_scan(text: str) -> list[dict[str, Any]]:
+def mention_scan(text: str, allowed: frozenset[str] = frozenset()) -> list[dict[str, Any]]:
"""Live `@`-mentions outside code spans, fences, decorators and cron aliases."""
hits = []
inside = False
@@ -117,7 +124,7 @@ def mention_scan(text: str) -> list[dict[str, Any]]:
prose = re.sub(r"`[^`]*`", "", line)
for match in _MENTION.finditer(prose):
handle = match.group(1)
- if handle.lower() in _CRON:
+ if handle.lower() in _CRON or handle.lower() in allowed:
continue
hits.append({"handle": handle, "line": number, "text": line.strip()})
return hits
@@ -191,6 +198,7 @@ def compose(
reviewers: list[dict[str, Any]] | None = None,
conflict_note: str | None = None,
security_note: str | None = None,
+ allowed: frozenset[str] = frozenset(),
) -> str:
"""The review body, in the template's section order; empty sections are omitted."""
anchored = [i for i, f in enumerate(findings, start=1) if f.get("line")]
@@ -224,11 +232,14 @@ def compose(
"and optional."
)
parts.append(footer_text)
- return escape_handles("\n\n".join(p for p in parts if p))
+ return escape_handles("\n\n".join(p for p in parts if p), allowed)
def inline_comments(
- findings: list[dict[str, Any]], kept: list[int], files: list[difflib.DiffFile]
+ findings: list[dict[str, Any]],
+ kept: list[int],
+ files: list[difflib.DiffFile],
+ allowed: frozenset[str] = frozenset(),
) -> dict[str, Any]:
"""The picked comments as review threads (line/side), and the ones the diff cannot anchor."""
anchored = [f for f in findings if f.get("line")]
@@ -241,7 +252,7 @@ def inline_comments(
files, str(finding["file"]), int(finding["line"]), str(finding.get("side") or "RIGHT")
)
text = escape_handles(
- str(finding.get("comment") or finding.get("explanation") or finding.get("rule_id") or "")
+ str(finding.get("comment") or finding.get("explanation") or finding.get("rule_id") or ""), allowed
)
if spot is None:
unanchorable.append({"index": index, "file": finding["file"], "line": finding["line"]})
diff --git a/tools/pr-management/src/pr_management/code_review/cli.py b/tools/pr-management/src/pr_management/code_review/cli.py
index aac62cd1..82ed3a03 100644
--- a/tools/pr-management/src/pr_management/code_review/cli.py
+++ b/tools/pr-management/src/pr_management/code_review/cli.py
@@ -33,7 +33,7 @@ import shlex
from pathlib import Path
from typing import Any
-from .. import config, model
+from .. import config, mentions, model
from ..triage import guards as triage_guards
from . import body as review_body
from . import codeowners, criteria, data, deps, disposition, reviewers, scans, selectors, session, slop
@@ -423,7 +423,8 @@ def _render(args: argparse.Namespace, cfg: config.Config) -> dict[str, Any]:
summary = Path(args.summary).read_text(encoding="utf-8").strip()
anchored = [f for f in findings if f.get("line")]
picked = review_body.pick(args.keep or "A", len(anchored)) if args.inline == "on" else {"keep": []}
- inline = review_body.inline_comments(findings, picked["keep"], files)
+ allow = mentions.allowed(cfg)
+ inline = review_body.inline_comments(findings, picked["keep"], files, allow)
kept = [k for k in picked["keep"] if k not in {u["index"] for u in inline["unanchorable"]}]
variant = disposition.footer_variant(args.disposition, args.permission or pr.viewer_permission)
project = cfg.project_name or (cfg.upstream_repo or "the project").split("/")[-1].capitalize()
@@ -442,6 +443,7 @@ def _render(args: argparse.Namespace, cfg: config.Config) -> dict[str, Any]:
conflict = "GitHub has not yet computed whether this branch merges cleanly with its base."
security_note = Path(args.security_note).read_text(encoding="utf-8") if args.security_note else None
text = review_body.compose(
+ allowed=allow,
summary=summary,
findings=findings,
inline_kept=kept,
@@ -453,7 +455,7 @@ def _render(args: argparse.Namespace, cfg: config.Config) -> dict[str, Any]:
args.out_dir.mkdir(parents=True, exist_ok=True)
body_file = args.out_dir / f"review-body-{args.pr}.md"
body_file.write_text(text + "\n", encoding="utf-8")
- hits = review_body.mention_scan(text + "\n" + "\n".join(t["body"] for t in inline["threads"]))
+ hits = review_body.mention_scan(text + "\n" + "\n".join(t["body"] for t in inline["threads"]), allow)
check = review_body.verify_footer(text, variant)
flag = {"APPROVE": "--approve", "REQUEST_CHANGES": "--request-changes", "COMMENT": "--comment"}[
args.disposition
@@ -533,7 +535,7 @@ def _slop_comment(args: argparse.Namespace, cfg: config.Config) -> dict[str, Any
disposition.footer_variant("COMMENT", args.permission), project, cfg.contributing_docs_url
),
)
- text = review_body.escape_handles(text)
+ text = review_body.escape_handles(text, mentions.allowed(cfg))
args.out_dir.mkdir(parents=True, exist_ok=True)
path = args.out_dir / f"slop-warning-{args.pr}.md"
path.write_text(text, encoding="utf-8")
@@ -611,6 +613,7 @@ def add_parsers(sub: Any) -> None:
rev.add_argument("--now", default=None)
ren = commands.add_parser("render", help="Steps 7a-7b: the body, the inline threads, the post command")
+ mentions.add_flag(ren)
ren.add_argument("--saved-dir", type=Path, required=True)
ren.add_argument("--pr", type=int, required=True)
ren.add_argument("--findings", required=True)
@@ -628,6 +631,7 @@ def add_parsers(sub: Any) -> None:
pic.add_argument("--count", type=int, required=True)
men = commands.add_parser("mention-scan", help="live @-mentions outside code")
+ mentions.add_flag(men)
men.add_argument("--body", type=Path, required=True)
ver = commands.add_parser("verify-footer", help="Golden rule 5: the verbatim footer is present")
@@ -641,6 +645,7 @@ def add_parsers(sub: Any) -> None:
grd.add_argument("--viewer", required=True)
slc = commands.add_parser("slop-comment", help="the [C] warning body and the [X] close commands")
+ mentions.add_flag(slc)
slc.add_argument("--pr", type=int, required=True)
slc.add_argument("--issues", required=True, help="one plain-English issue per line")
slc.add_argument("--out-dir", type=Path, required=True)
@@ -663,7 +668,7 @@ def add_parsers(sub: Any) -> None:
def dispatch(args: argparse.Namespace) -> dict[str, Any]:
- cfg = config.load(args.project_root, args.config_dir)
+ cfg = mentions.apply_flag(config.load(args.project_root, args.config_dir), args)
command = args.command
if command == "resolve":
try:
@@ -699,7 +704,7 @@ def dispatch(args: argparse.Namespace) -> dict[str, Any]:
if command == "pick":
return review_body.pick(args.spec, args.count)
if command == "mention-scan":
- hits = review_body.mention_scan(args.body.read_text(encoding="utf-8"))
+ hits = review_body.mention_scan(args.body.read_text(encoding="utf-8"), mentions.allowed(cfg))
return {"live_handles": [h["handle"] for h in hits], "prompt_shown": bool(hits), "hits": hits}
if command == "verify-footer":
return review_body.verify_footer(args.body.read_text(encoding="utf-8"), args.variant)
diff --git a/tools/pr-management/src/pr_management/config.py b/tools/pr-management/src/pr_management/config.py
index 421ea834..19ebdbf5 100644
--- a/tools/pr-management/src/pr_management/config.py
+++ b/tools/pr-management/src/pr_management/config.py
@@ -118,6 +118,9 @@ class Config:
check_map: list[CheckCategory] = field(default_factory=list)
conflicts_url: str | None = None
+ #: Handles a rendered body may keep live (`mention_allowlist`); see mentions.py.
+ mention_allowlist: list[str] = field(default_factory=list)
+
marker: str = DEFAULT_MARKER
urls: dict[str, str] = field(default_factory=dict)
footer: str | None = None
@@ -216,6 +219,8 @@ def load(project_root: Path, config_dir: Path | None = None) -> Config:
cfg.backport_branches = mdconfig.tokens(cells[1]) if len(cells) > 1 else []
elif key == "real_ci_patterns" and len(cells) > 1:
cfg.real_ci_patterns = mdconfig.tokens(cells[1])
+ elif key == "mention_allowlist" and len(cells) > 1:
+ cfg.mention_allowlist = mdconfig.tokens(cells[1])
elif key == "static_check_patterns" and len(cells) > 1:
cfg.static_check_patterns = [*STATIC_CHECK_PATTERNS, *mdconfig.tokens(cells[1])]
grace_attr = _GRACE_ROWS.get(cells[0].lower())
diff --git a/tools/pr-management/src/pr_management/mentions.py b/tools/pr-management/src/pr_management/mentions.py
new file mode 100644
index 00000000..0b948c94
--- /dev/null
+++ b/tools/pr-management/src/pr_management/mentions.py
@@ -0,0 +1,59 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""Which `@`-mentions a rendered body may keep live.
+
+Every renderer in this package backtick-quotes handles so a posted body
+notifies nobody but the PR author. A project can name handles that may stay
+live everywhere — `mention_allowlist` in `<project-config>/pr-management-config.md`
+— and one call can add more with `--allow-mention <login>`. The agent-guard
+`mention` guard honours the same key, read from the committed copy only, so
+the two never disagree about a reviewed allowlist.
+"""
+
+from __future__ import annotations
+
+import argparse
+from collections.abc import Iterable
+
+from .config import Config
+
+
+def normalize(handle: str) -> str:
+ """`@Login` / `login` / `@org/team` → lower-case, without the `@`."""
+ return handle.strip().lstrip("@").lower()
+
+
+def allowed(cfg: Config, extra: Iterable[str] = ()) -> frozenset[str]:
+ """The configured allowlist plus any per-call additions."""
+ return frozenset(h for h in (normalize(x) for x in (*cfg.mention_allowlist, *extra)) if h)
+
+
+def add_flag(parser: argparse.ArgumentParser) -> None:
+ parser.add_argument(
+ "--allow-mention",
+ action="append",
+ default=[],
+ metavar="LOGIN",
+ help="keep this @-mention live for this call (repeatable); the guard also needs MAGPIE_ALLOW_MENTIONS=1 "
+ "on the posting command unless the handle is in the mention_allowlist on the target repository's default branch",
+ )
+
+
+def apply_flag(cfg: Config, args: argparse.Namespace) -> Config:
+ """Fold `--allow-mention` values into the loaded config."""
+ cfg.mention_allowlist = [*cfg.mention_allowlist, *getattr(args, "allow_mention", [])]
+ return cfg
diff --git a/tools/pr-management/src/pr_management/mentor/cli.py b/tools/pr-management/src/pr_management/mentor/cli.py
index 60ea1323..152d614b 100644
--- a/tools/pr-management/src/pr_management/mentor/cli.py
+++ b/tools/pr-management/src/pr_management/mentor/cli.py
@@ -25,6 +25,7 @@ from pathlib import Path
from typing import Any
from .. import config as core_config
+from .. import mentions
from ..layers import personal_dir
from ..people import Maintainers, load_permissions, load_team
from . import config, render, thread, tone
@@ -75,6 +76,7 @@ def add_parsers(sub: Any) -> None:
ren.add_argument("--pointer", default=None, help="the convention_pointers trigger the comment links")
ren.add_argument("--open-question-file", type=Path, default=None, help="hand-off: one-line open question")
ren.add_argument("--out", type=Path, required=True)
+ mentions.add_flag(ren)
chk = cmd.add_parser("tone-check", help="run the deterministic tone rules on a draft")
chk.add_argument("--draft", type=Path, required=True)
@@ -132,7 +134,8 @@ def dispatch(args: argparse.Namespace) -> dict[str, Any]:
return result
if args.command == "render":
question = args.open_question_file.read_text(encoding="utf-8") if args.open_question_file else None
- upstream = core_config.load(args.project_root, args.config_dir).upstream_repo
+ core = mentions.apply_flag(core_config.load(args.project_root, args.config_dir), args)
+ upstream = core.upstream_repo
return render.render(
cfg,
kind=args.kind,
@@ -141,6 +144,7 @@ def dispatch(args: argparse.Namespace) -> dict[str, Any]:
open_question=question,
upstream=upstream,
out=args.out,
+ allowed=mentions.allowed(core),
)
if args.command == "tone-check":
return tone.check(args.draft.read_text(encoding="utf-8"), author=args.author, footer=cfg.footer)
diff --git a/tools/pr-management/src/pr_management/mentor/render.py b/tools/pr-management/src/pr_management/mentor/render.py
index 507f9efc..97ed776b 100644
--- a/tools/pr-management/src/pr_management/mentor/render.py
+++ b/tools/pr-management/src/pr_management/mentor/render.py
@@ -73,6 +73,7 @@ def render(
open_question: str | None = None,
upstream: str | None = None,
out: Path,
+ allowed: frozenset[str] = frozenset(),
) -> dict[str, Any]:
errors: list[str] = []
if cfg.footer is None:
@@ -86,7 +87,7 @@ def render(
errors.append("maintainer_team_handle must be an @org/team handle")
if errors:
return {"ok": False, "errors": errors}
- question = _silence_mentions(_link_numbers(question, upstream), set())
+ question = _silence_mentions(_link_numbers(question, upstream), set(allowed))
body = (
_template("hand-off")
.replace("<maintainer_team_handle>", handle)
diff --git a/tools/pr-management/src/pr_management/stale_sweep/cli.py b/tools/pr-management/src/pr_management/stale_sweep/cli.py
index b7ab3843..edc3c94a 100644
--- a/tools/pr-management/src/pr_management/stale_sweep/cli.py
+++ b/tools/pr-management/src/pr_management/stale_sweep/cli.py
@@ -24,7 +24,7 @@ import json
from pathlib import Path
from typing import Any
-from .. import config, model, people
+from .. import config, mentions, model, people
from . import classify as stale_classify
from . import plan as stale_plan
from . import recap as stale_recap
@@ -50,6 +50,7 @@ def add_parsers(sub: Any) -> None:
cls.add_argument("selector", nargs="*")
ren = cmds.add_parser("render", help="Step 4: the comment for one proposal")
+ mentions.add_flag(ren)
ren.add_argument("--saved-dir", type=Path, required=True)
ren.add_argument("--pr", type=int, required=True)
ren.add_argument("--out-dir", type=Path, required=True)
@@ -72,7 +73,7 @@ def _now(value: str | None) -> dt.datetime:
def dispatch(args: argparse.Namespace) -> dict[str, Any]:
- cfg = config.load(args.project_root, args.config_dir)
+ cfg = mentions.apply_flag(config.load(args.project_root, args.config_dir), args)
if args.command == "plan":
return stale_plan.build(
args.selector, args.project_root, args.config_dir, cfg.committers_team
diff --git a/tools/pr-management/src/pr_management/stale_sweep/render.py b/tools/pr-management/src/pr_management/stale_sweep/render.py
index 76053c39..c809608c 100644
--- a/tools/pr-management/src/pr_management/stale_sweep/render.py
+++ b/tools/pr-management/src/pr_management/stale_sweep/render.py
@@ -31,6 +31,7 @@ from importlib import resources
from pathlib import Path
from typing import Any
+from .. import mentions
from ..config import Config
from ..triage.render import enforce
@@ -61,7 +62,7 @@ def render(entry: dict[str, Any], cfg: Config, out_dir: Path) -> dict[str, Any]:
body = _template(TEMPLATES[cls])
for key, value in values.items():
body = body.replace(key, value)
- body, mentioned = enforce(body, entry["author"], cfg.upstream_repo)
+ body, mentioned = enforce(body, entry["author"], cfg.upstream_repo, mentions.allowed(cfg))
leftover = sorted(set(_PLACEHOLDER.findall(re.sub(r"<!--.*?-->", "", body, flags=re.DOTALL))))
number = int(entry["number"])
out_dir.mkdir(parents=True, exist_ok=True)
diff --git a/tools/pr-management/src/pr_management/triage/render.py b/tools/pr-management/src/pr_management/triage/render.py
index bb71feea..b0bf0a98 100644
--- a/tools/pr-management/src/pr_management/triage/render.py
+++ b/tools/pr-management/src/pr_management/triage/render.py
@@ -40,7 +40,7 @@ from importlib import resources
from pathlib import Path
from typing import Any
-from .. import ci, markers, mdconfig
+from .. import ci, markers, mdconfig, mentions
from ..config import CONFIRMATION_MARKER, FOLD_CLOSE, FOLD_OPEN, Config
from ..model import PR
from . import signals
@@ -349,8 +349,10 @@ def _substitute(text: str, values: dict[str, str]) -> str:
return text
-def enforce(text: str, author: str, upstream: str | None) -> tuple[str, list[str]]:
- """Author-only `@`-mentions and no bare `#NNN`, outside code spans and HTML comments."""
+def enforce(
+ text: str, author: str, upstream: str | None, allowed: frozenset[str] = frozenset()
+) -> tuple[str, list[str]]:
+ """Author-only `@`-mentions — plus any `allowed` handle — and no bare `#NNN`, outside code and comments."""
out_lines = []
mentioned: list[str] = []
inside_fence = False
@@ -369,7 +371,7 @@ def enforce(text: str, author: str, upstream: str | None) -> tuple[str, list[str
def handle(match: re.Match[str]) -> str:
login = match.group(1)
- if login.lower() == author.lower():
+ if login.lower() == author.lower() or login.lower() in allowed:
if login not in mentioned:
mentioned.append(login)
return match.group(0)
@@ -510,7 +512,7 @@ def render(
if stem == "ready-flip":
result.unassign_author = True
- body, mentioned = enforce(body, pr.author, cfg.upstream_repo)
+ body, mentioned = enforce(body, pr.author, cfg.upstream_repo, mentions.allowed(cfg))
result.body = body
result.mentions = [f"@{m}" for m in mentioned]
leftover = sorted(set(PLACEHOLDER.findall(re.sub(r"<!--.*?-->", "", body, flags=re.DOTALL))))
diff --git a/tools/pr-management/tests/test_mentions.py b/tools/pr-management/tests/test_mentions.py
new file mode 100644
index 00000000..55e36a16
--- /dev/null
+++ b/tools/pr-management/tests/test_mentions.py
@@ -0,0 +1,71 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""The `@`-mention allowlist every renderer honours."""
+
+from __future__ import annotations
+
+import argparse
+from pathlib import Path
+
+from pr_management import config, mentions
+from pr_management.code_review import body as review_body
+from pr_management.triage.render import enforce
+
+
+def _cfg(tmp_path: Path, cell: str) -> config.Config:
+ d = tmp_path / "cfg"
+ d.mkdir(exist_ok=True)
+ (d / "pr-management-config.md").write_text(
+ f"| Key | Value | Notes |\n|---|---|---|\n| `mention_allowlist` | {cell} | x |\n"
+ )
+ return config.load(tmp_path, d)
+
+
+def test_the_config_key_is_read(tmp_path: Path) -> None:
+ cfg = _cfg(tmp_path, "`@release-bot` `@acme/docs-team`")
+ assert mentions.allowed(cfg) == {"release-bot", "acme/docs-team"}
+
+
+def test_an_unset_key_allows_nothing(tmp_path: Path) -> None:
+ assert mentions.allowed(_cfg(tmp_path, "*(empty)*")) == frozenset()
+
+
+def test_the_flag_adds_to_the_config(tmp_path: Path) -> None:
+ parser = argparse.ArgumentParser()
+ mentions.add_flag(parser)
+ args = parser.parse_args(["--allow-mention", "@Kaxil", "--allow-mention", "potiuk"])
+ cfg = mentions.apply_flag(_cfg(tmp_path, "`@release-bot`"), args)
+ assert mentions.allowed(cfg) == {"release-bot", "kaxil", "potiuk"}
+
+
+def test_triage_and_stale_sweep_keep_allowed_handles_live() -> None:
+ text, mentioned = enforce("@alice — cc @release-bot and @bob", "alice", None, frozenset({"release-bot"}))
+ assert text == "@alice — cc @release-bot and `@bob`"
+ assert mentioned == ["alice", "release-bot"]
+
+
+def test_without_an_allowlist_only_the_author_stays_live() -> None:
+ text, _ = enforce("@alice — cc @release-bot", "alice", None)
+ assert text == "@alice — cc `@release-bot`"
+
+
+def test_code_review_keeps_allowed_handles_live() -> None:
+ allowed = frozenset({"acme/docs-team"})
+ assert (
+ review_body.escape_handles("cc @acme/docs-team and @bob", allowed) == "cc @acme/docs-team and `@bob`"
+ )
+ assert [h["handle"] for h in review_body.mention_scan("cc @acme/docs-team and @bob", allowed)] == ["bob"]