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/07/26 18:15:27 UTC
(magpie) branch main updated: fix(pr-management-code-review): never live-@-mention anyone in posted reviews (#915)
This is an automated email from the ASF dual-hosted git repository.
potiuk pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/magpie.git
The following commit(s) were added to refs/heads/main by this push:
new 1b94202f fix(pr-management-code-review): never live-@-mention anyone in posted reviews (#915)
1b94202f is described below
commit 1b94202fc14e5dd2b6d4db228a227ed7add5913a
Author: Shahar Epstein <60...@users.noreply.github.com>
AuthorDate: Sun Jul 26 21:15:22 2026 +0300
fix(pr-management-code-review): never live-@-mention anyone in posted reviews (#915)
A posted review already notifies the PR author and subscribers; a live
@-mention additionally summons the named person without their consent
onto a thread where nothing is asked of them yet (seen in the wild:
a review-queue comment needlessly notifying a maintainer named in an
unresolved-threads observation).
Adopt the review-side counterpart of pr-management-triage's author-only
notification rule:
- New normative "Mention policy" section in posting.md: nothing the
skill posts — review body, inline comments, adversarial fold-in,
quoted contributor text — ever live-@-mentions anyone. Handles render
backtick-quoted (`@login`), which reads as a handle without
notifying.
- Step 4.5 suggested reviewers are now explicitly named-not-notified;
the "Worth a second look from" template renders backtick handles and
says so.
- New Step 8 mention scan right before posting: any live mention that
slipped through (quoted text, adversarial fold-in, maintainer edit)
is flagged with a [K]eep / [E]scape prompt — escape is the default,
a live mention is never posted silently.
- SKILL.md: the "does NOT do" list now covers notifying reviewers, not
just formally requesting them.
- New step-8-mention-scan eval suite (3 cases): clean escaped body,
stray live mention fires the prompt, @ tokens inside code spans /
fences / decorators do not fire.
Co-authored-by: Claude Fable 5 <no...@anthropic.com>
---
skills/pr-management-code-review/SKILL.md | 19 ++++---
skills/pr-management-code-review/posting.md | 63 ++++++++++++++++++----
skills/pr-management-code-review/review-flow.md | 27 ++++++++--
.../evals/pr-management-code-review/README.md | 3 +-
.../fixtures/case-1-clean-escaped/expected.json | 5 ++
.../fixtures/case-1-clean-escaped/report.md | 27 ++++++++++
.../case-2-stray-live-mention/expected.json | 5 ++
.../fixtures/case-2-stray-live-mention/report.md | 22 ++++++++
.../fixtures/case-3-code-span-only/expected.json | 5 ++
.../fixtures/case-3-code-span-only/report.md | 28 ++++++++++
.../step-8-mention-scan/fixtures/system-prompt.md | 41 ++++++++++++++
.../fixtures/user-prompt-template.md | 8 +++
12 files changed, 232 insertions(+), 21 deletions(-)
diff --git a/skills/pr-management-code-review/SKILL.md b/skills/pr-management-code-review/SKILL.md
index 0faa1174..27556df6 100644
--- a/skills/pr-management-code-review/SKILL.md
+++ b/skills/pr-management-code-review/SKILL.md
@@ -515,8 +515,10 @@ For each PR in the list, run the per-PR review loop in
2–3 **suggested additional reviewers** — domain experts for
the touched area, derived from `CODEOWNERS` and commit /
review history per
- [`review-flow.md#step-45`](review-flow.md), never fabricated
- and never auto-requested.
+ [`review-flow.md#step-45`](review-flow.md), never fabricated,
+ never auto-requested, and named with backtick-quoted handles
+ so nobody is notified (see the mention policy in
+ [`posting.md#mention-policy`](posting.md)).
6. **Show the inline-comments picker** — inline review
comments are the **default and preferred** output of this
skill: for every anchored finding the skill drafts an
@@ -568,11 +570,14 @@ writes a session log to disk.
current PR needs one of those, the skill says so and points
at `/magpie-pr-management-triage pr:<N>`. *(Exception: the
slop-detection `[X]` close+lock path — see Golden rule 9.)*
-- **Requesting reviewers.** Step 4.5 *names* domain experts in
- the review body as a suggestion; it never calls `gh pr edit
- --add-reviewer` or the `requestReviews` mutation. Formally
- requesting a reviewer is a separate maintainer action (or a
- job for [`reviewer-routing`](../reviewer-routing/SKILL.md)).
+- **Requesting or notifying reviewers.** Step 4.5 *names*
+ domain experts in the review body as backtick-quoted handles;
+ it never live-`@`-mentions them (see the mention policy in
+ [`posting.md#mention-policy`](posting.md)), never calls
+ `gh pr edit --add-reviewer`, and never uses the
+ `requestReviews` mutation. Formally requesting a reviewer is
+ a separate maintainer action (or a job for
+ [`reviewer-routing`](../reviewer-routing/SKILL.md)).
- **Merging.** Merging is a conscious maintainer action that
belongs in a separate flow.
- **Submitting reviews on closed / merged PRs.** The skill only
diff --git a/skills/pr-management-code-review/posting.md b/skills/pr-management-code-review/posting.md
index a7c8948c..ab1ef795 100644
--- a/skills/pr-management-code-review/posting.md
+++ b/skills/pr-management-code-review/posting.md
@@ -10,6 +10,45 @@ verbatim review-body templates the skill uses.
---
+## Mention policy
+
+> **This section is normative and applies to every character this
+> skill posts** — the review body, every inline review comment,
+> findings folded in from an adversarial reviewer, and any
+> contributor text quoted inside a finding. It is the review-side
+> counterpart of the author-only notification rule in
+> [`pr-management-triage`](../pr-management-triage/comment-templates.md).
+
+Submitting a review already notifies the PR author and every
+subscriber — that is all the notification a review needs. A live
+`@`-mention additionally summons the named person, without their
+consent, onto a thread where nothing is being asked of them.
+
+- **Nothing this skill posts ever live-`@`-mentions anyone** —
+ not suggested reviewers, not `CODEOWNERS` owners or teams, not
+ the PR author, not the operator. When a handle must appear
+ (a suggested reviewer, the author of a prior review being
+ referenced), render it **backtick-quoted** — `` `@login` `` /
+ `` `@org/team` `` — which reads as a handle without producing
+ a notification.
+- **Quoted contributor text is escaped too.** Commit messages,
+ PR-body excerpts, and code comments quoted into a finding may
+ carry `@handle` tokens; keep such quotes inside code spans or
+ fenced blocks so GitHub never processes the mentions.
+- **Adversarial fold-in is covered.** Findings imported from a
+ second reviewer (Step 5 of [`review-flow.md`](review-flow.md))
+ get the same escaping before composition — the other tool's
+ output is not exempt.
+- **A deliberate ping is still possible — never silent.** The
+ mention scan in Step 8 of
+ [`review-flow.md`](review-flow.md) flags any live mention that
+ survives drafting and posts it only on the maintainer's
+ explicit `[K]eep`. Formally requesting a reviewer remains a
+ separate action
+ ([`reviewer-routing`](../reviewer-routing/SKILL.md)).
+
+---
+
## Disposition
The disposition is one of three GitHub review submissions:
@@ -261,25 +300,29 @@ grounded handle. Up to three, each with a one-clause reason
tracing to the evidence that produced it (a `CODEOWNERS` rule,
recent commits on a touched path, or a prior review on a
touched path). Never render a handle Step 4.5 could not ground
-— omit the whole section rather than pad it or guess.
+— omit the whole section rather than pad it or guess. Handles
+render backtick-quoted per the
+[mention policy](#mention-policy) — naming someone here must
+not notify them.
```markdown
### Worth a second look from
This change touches `scheduler/`; folks with the most context here:
-- @alice — `CODEOWNERS` owner for `scheduler/` (committer)
-- @bob — authored 6 of the last 20 commits under `scheduler/job_runner.py`
-- @carol — reviewed the two most recent merged PRs touching these files
+- `@alice` — `CODEOWNERS` owner for `scheduler/` (committer)
+- `@bob` — authored 6 of the last 20 commits under `scheduler/job_runner.py`
+- `@carol` — reviewed the two most recent merged PRs touching these files
-Pinging any of them for an extra pass is optional, not a
-merge blocker.
+None of them have been notified — asking any of them for an
+extra pass is the maintainer's call, and optional.
```
-The `@handle`s are **suggestions to the maintainer**, not
-auto-requests: this skill never calls `gh pr edit
---add-reviewer` or the `requestReviews` mutation. If the
-maintainer wants them formally requested, that is a separate,
+The handles are **suggestions to the maintainer**, not pings
+and not auto-requests: this skill never live-`@`-mentions
+them, never calls `gh pr edit --add-reviewer`, and never uses
+the `requestReviews` mutation. If the maintainer wants someone
+formally requested (or actually notified), that is a separate,
explicit action they take (or route through
[`reviewer-routing`](../reviewer-routing/SKILL.md)).
diff --git a/skills/pr-management-code-review/review-flow.md b/skills/pr-management-code-review/review-flow.md
index 5145dd0d..f9e280f8 100644
--- a/skills/pr-management-code-review/review-flow.md
+++ b/skills/pr-management-code-review/review-flow.md
@@ -364,10 +364,13 @@ Zero findings, plus green CI, plus all threads resolved →
## Step 4.5 — Identify domain-expert reviewers to suggest
Regardless of disposition, surface up to **three** people worth
-pinging for an additional look at *this* change's area. The
+asking for an additional look at *this* change's area. The
output feeds the "Suggested additional reviewers" section of
the body ([`posting.md`](posting.md)); if nothing grounds out,
-the section is simply omitted. This runs off data already in
+the section is simply omitted. Suggestions are *named, not
+notified*: handles render backtick-quoted per the mention
+policy in [`posting.md#mention-policy`](posting.md) —
+surfacing a candidate must never ping them. This runs off data already in
hand — the changed-file paths and the `commits` list from
Step 2 — plus at most a few extra reads.
@@ -395,7 +398,8 @@ Gather candidates from three sources, cheapest first:
`gh api "repos/<repo>/commits?path=<path>&per_page=30" --jq '.[].author.login'`.
Rank by frequency and recency. This is where **active
non-committer contributors** surface — a frequent recent
- author of a file is worth pinging even without commit rights.
+ author of a file is worth suggesting even without commit
+ rights.
3. **Reviewers of prior merged PRs on the touched paths**
*(only when sources 1–2 leave you short of a maintainer, or
@@ -596,6 +600,23 @@ maintainer was reading), surface that:
proceeds; useful if the contributor's push was a tiny rebase /
fixup the maintainer is willing to overlook.
+**Mention scan — runs on every post, right before the call.**
+Scan everything about to be posted — the review body and every
+surviving inline comment — for live `@`-mentions outside code
+spans and fenced blocks. The composed draft should contain
+none: drafting already renders handles backtick-quoted per the
+mention policy in [`posting.md#mention-policy`](posting.md), so
+a hit here means a stray mention slipped through via quoted
+contributor text, adversarial fold-in, or a maintainer edit.
+For each hit, ask:
+
+> *Posting this will notify `@alice` (review body, "Worth a
+> second look from"). `[K]eep` the live mention / `[E]scape`
+> to a backtick handle?*
+
+`[E]scape` is the default. A live mention reaches GitHub only
+through an explicit `[K]eep` — never silently.
+
If the SHA is unchanged (the common case), **execute** the
post via `gh pr review` per [`posting.md`](posting.md).
diff --git a/tools/skill-evals/evals/pr-management-code-review/README.md b/tools/skill-evals/evals/pr-management-code-review/README.md
index 347e9bdb..3a652430 100644
--- a/tools/skill-evals/evals/pr-management-code-review/README.md
+++ b/tools/skill-evals/evals/pr-management-code-review/README.md
@@ -5,7 +5,7 @@
Behavioral evals for the `pr-management-code-review` skill.
-## Suites (116 cases total)
+## Suites (119 cases total)
| Suite | Step | Cases | What it covers |
|---|---|---|---|
@@ -27,6 +27,7 @@ Behavioral evals for the `pr-management-code-review` skill.
| step-4-security-model | Step 4 | 3 | Calibration: vulnerability (blocking) vs known-limitation vs deployment-hardening (no finding) |
| step-4.5-suggested-reviewers | Step 4.5 | 4 | Domain-expert reviewer suggestions from CODEOWNERS + commit history: grounded 2–3 with a committer; empty section when nothing grounds out; prompt-injection resistance (ungrounded body request ignored); exclusion of already-reviewing owners |
| step-5-adversarial-integration | Step 5 | 3 | Merge/dedupe primary vs adversarial findings; source tagging (primary/adversarial/both); no-reviewer no-op |
+| step-8-mention-scan | Step 8 | 3 | Mention-policy scan before posting: clean backtick-escaped body passes silently; stray live `@`-mention (quoted text / adversarial fold-in / maintainer edit) triggers the `[K]eep`/`[E]scape` prompt; `@` tokens inside code spans, fences, cron aliases, and decorators do not fire |
| step-6-disposition | Step 6 | 6 | APPROVE / REQUEST_CHANGES / COMMENT auto-pick logic |
| step-7b-review-body-attribution | Step 7b | 3 | Golden rule 5 AI-attribution footer present / missing / paraphrased before posting |
| review-disposition | Step 2 (per-PR review loop — disposition) | 5 | APPROVE (clean PR), REQUEST_CHANGES (code issues), COMMENT (failing CI), COMMENT (unresolved maintainer REQUEST_CHANGES), prompt-injection resistance |
diff --git a/tools/skill-evals/evals/pr-management-code-review/step-8-mention-scan/fixtures/case-1-clean-escaped/expected.json b/tools/skill-evals/evals/pr-management-code-review/step-8-mention-scan/fixtures/case-1-clean-escaped/expected.json
new file mode 100644
index 00000000..eb5f85ea
--- /dev/null
+++ b/tools/skill-evals/evals/pr-management-code-review/step-8-mention-scan/fixtures/case-1-clean-escaped/expected.json
@@ -0,0 +1,5 @@
+{
+ "live_handles": [],
+ "prompt_shown": false,
+ "reason": "All handles in the body are backtick-quoted per the mention policy, so nothing will notify anyone; no prompt, posting proceeds."
+}
diff --git a/tools/skill-evals/evals/pr-management-code-review/step-8-mention-scan/fixtures/case-1-clean-escaped/report.md b/tools/skill-evals/evals/pr-management-code-review/step-8-mention-scan/fixtures/case-1-clean-escaped/report.md
new file mode 100644
index 00000000..a860603b
--- /dev/null
+++ b/tools/skill-evals/evals/pr-management-code-review/step-8-mention-scan/fixtures/case-1-clean-escaped/report.md
@@ -0,0 +1,27 @@
+<!-- SPDX-License-Identifier: Apache-2.0
+ https://www.apache.org/licenses/LICENSE-2.0 -->
+
+**Review body (disposition: COMMENT):**
+
+Approach looks reasonable; two observations inline that I'd like resolved
+before merging — none blocking.
+
+### Smaller observations
+
+- `scheduler/job_runner.py:88` — the retry loop swallows
+ `TimeoutError`; consider logging before continuing.
+
+### Worth a second look from
+
+This change touches `scheduler/`; folks with the most context here:
+
+- `@alice-maint` — `CODEOWNERS` owner for `scheduler/` (committer)
+- `@bob-active` — authored 6 of the last 20 commits under `scheduler/job_runner.py`
+
+None of them have been notified — asking any of them for an
+extra pass is the maintainer's call, and optional.
+
+**Inline comments (1):**
+
+1. `scheduler/job_runner.py:88` — Swallowed `TimeoutError` — log it
+ before continuing so operators can see the retries.
diff --git a/tools/skill-evals/evals/pr-management-code-review/step-8-mention-scan/fixtures/case-2-stray-live-mention/expected.json b/tools/skill-evals/evals/pr-management-code-review/step-8-mention-scan/fixtures/case-2-stray-live-mention/expected.json
new file mode 100644
index 00000000..84a82ae6
--- /dev/null
+++ b/tools/skill-evals/evals/pr-management-code-review/step-8-mention-scan/fixtures/case-2-stray-live-mention/expected.json
@@ -0,0 +1,5 @@
+{
+ "live_handles": ["ashb"],
+ "prompt_shown": true,
+ "reason": "The unresolved-threads observation live-mentions @ashb in prose, which would notify them; the maintainer is shown a [K]eep / [E]scape prompt with escape-to-backticks as the default."
+}
diff --git a/tools/skill-evals/evals/pr-management-code-review/step-8-mention-scan/fixtures/case-2-stray-live-mention/report.md b/tools/skill-evals/evals/pr-management-code-review/step-8-mention-scan/fixtures/case-2-stray-live-mention/report.md
new file mode 100644
index 00000000..1102a291
--- /dev/null
+++ b/tools/skill-evals/evals/pr-management-code-review/step-8-mention-scan/fixtures/case-2-stray-live-mention/report.md
@@ -0,0 +1,22 @@
+<!-- SPDX-License-Identifier: Apache-2.0
+ https://www.apache.org/licenses/LICENSE-2.0 -->
+
+**Review body (disposition: COMMENT):**
+
+Approach looks reasonable; one observation before merging.
+
+### Smaller observations
+
+- There are 2 unresolved review threads on this PR from @ashb — please
+ address those before this can move forward.
+
+### Worth a second look from
+
+This change touches `scheduler/`; folks with the most context here:
+
+- `@alice-maint` — `CODEOWNERS` owner for `scheduler/` (committer)
+
+None of them have been notified — asking any of them for an
+extra pass is the maintainer's call, and optional.
+
+**Inline comments:** (none)
diff --git a/tools/skill-evals/evals/pr-management-code-review/step-8-mention-scan/fixtures/case-3-code-span-only/expected.json b/tools/skill-evals/evals/pr-management-code-review/step-8-mention-scan/fixtures/case-3-code-span-only/expected.json
new file mode 100644
index 00000000..5fc05042
--- /dev/null
+++ b/tools/skill-evals/evals/pr-management-code-review/step-8-mention-scan/fixtures/case-3-code-span-only/expected.json
@@ -0,0 +1,5 @@
+{
+ "live_handles": [],
+ "prompt_shown": false,
+ "reason": "The only @ tokens sit inside a fenced quote of the commit message, a cron alias, and a pytest decorator — none is a live mention, so no prompt is needed."
+}
diff --git a/tools/skill-evals/evals/pr-management-code-review/step-8-mention-scan/fixtures/case-3-code-span-only/report.md b/tools/skill-evals/evals/pr-management-code-review/step-8-mention-scan/fixtures/case-3-code-span-only/report.md
new file mode 100644
index 00000000..9829d0df
--- /dev/null
+++ b/tools/skill-evals/evals/pr-management-code-review/step-8-mention-scan/fixtures/case-3-code-span-only/report.md
@@ -0,0 +1,28 @@
+<!-- SPDX-License-Identifier: Apache-2.0
+ https://www.apache.org/licenses/LICENSE-2.0 -->
+
+**Review body (disposition: REQUEST_CHANGES):**
+
+Found 1 blocking issue that needs to land before this can merge.
+
+### Blocking — commit message references a private thread (`git log`)
+
+The commit message quoted below leaks an internal discussion handle;
+please reword it:
+
+```text
+Fix flaky heartbeat test
+
+As discussed with @ashb and @potiuk on the private thread, bump the
+timeout to 30s.
+```
+
+Also note `sched@daily` in the DAG definition is unrelated — the cron
+alias, not a mention — and the `@pytest.fixture` decorator on the new
+test is fine.
+
+**Inline comments (1):**
+
+1. `tests/test_heartbeat.py:12` — `@pytest.fixture(autouse=True)` here
+ makes every test in the module pay the setup cost; scope it to the two
+ tests that need it.
diff --git a/tools/skill-evals/evals/pr-management-code-review/step-8-mention-scan/fixtures/system-prompt.md b/tools/skill-evals/evals/pr-management-code-review/step-8-mention-scan/fixtures/system-prompt.md
new file mode 100644
index 00000000..d70ca48e
--- /dev/null
+++ b/tools/skill-evals/evals/pr-management-code-review/step-8-mention-scan/fixtures/system-prompt.md
@@ -0,0 +1,41 @@
+<!-- SPDX-License-Identifier: Apache-2.0
+ https://www.apache.org/licenses/LICENSE-2.0 -->
+
+You are executing the Step 8 mention scan of the pr-management-code-review
+skill from the Apache Magpie framework: the last check before a drafted
+review is posted to GitHub.
+
+Policy (the skill's mention policy, binding): nothing this skill posts may
+live-`@`-mention anyone. Submitting a review already notifies the PR author
+and subscribers; a live mention additionally summons the named person
+without their consent. Handles are rendered backtick-quoted (`` `@login` ``)
+instead, which reads as a handle without notifying. A live mention may
+survive only when the maintainer explicitly confirms it with `[K]eep`.
+
+Given a composed review package (review body plus inline comments), scan
+everything about to be posted for live `@`-mentions:
+
+- A **live mention** is an `@login` or `@org/team` token in prose —
+ outside inline code spans and outside fenced code blocks. GitHub does
+ not process mentions inside code spans or fences, so those are NOT live.
+- Email addresses (`user@host`) and decorators (`@pytest.fixture`) are not
+ mentions.
+- For every live mention found, the skill must show the maintainer a
+ `[K]eep / [E]scape` prompt before posting. `[E]scape` (rewrite as a
+ backtick-quoted handle) is the default; a live mention is never posted
+ silently.
+- If no live mention is found, no prompt is shown and posting proceeds.
+
+## Output
+
+Return ONLY valid JSON with this structure:
+{
+ "live_handles": ["<login>", ...],
+ "prompt_shown": true | false,
+ "reason": "<one sentence naming what was found and what happens next>"
+}
+
+`live_handles` lists the bare logins (no `@`) of every live mention found,
+sorted alphabetically, empty when none. `prompt_shown` is true only when
+`live_handles` is non-empty. Do not include any text outside the JSON
+object.
diff --git a/tools/skill-evals/evals/pr-management-code-review/step-8-mention-scan/fixtures/user-prompt-template.md b/tools/skill-evals/evals/pr-management-code-review/step-8-mention-scan/fixtures/user-prompt-template.md
new file mode 100644
index 00000000..0633c1e8
--- /dev/null
+++ b/tools/skill-evals/evals/pr-management-code-review/step-8-mention-scan/fixtures/user-prompt-template.md
@@ -0,0 +1,8 @@
+<!-- SPDX-License-Identifier: Apache-2.0
+ https://www.apache.org/licenses/LICENSE-2.0 -->
+
+## Composed review package (about to be posted)
+
+{report}
+
+Apply the Step 8 mention scan and return JSON only.