feat: PM-Agent also need to look at PR comments
From Gitea issue #116. Authored by the PM agent's worker; the merge gate is the approver. Refs #116. Agent: pm-agent (issue #116, mode revise)
This commit is contained in:
pm-agent committed
1 parent
2fdb95393c
commit
e8e9de8611
7 files changed
+471
-189
No files matched your search
@@ -1,7 +1,7 @@
|
||||
---
|
||||
date: 2026-11-07
|
||||
branch: feat/pm-agent-pr-mentions
|
||||
title: The PM agent can be @-mentioned on a pull request — trigger, context and reply only, shipped OFF
|
||||
title: The PM agent can be @-mentioned on a pull request — an EXPLICIT mention is the only trigger, reply only, shipped OFF
|
||||
---
|
||||
|
||||
## What changed
|
||||
@@ -12,6 +12,57 @@ the PR, the changes and the thread, and reply in that thread. This branch implem
|
||||
operator's point 6 — revise an agent-owned branch versus open a fresh branch/PR against
|
||||
`main`) is deliberately absent and wants its own issue, as the PM said on the thread.
|
||||
|
||||
### Revised 2026-11-07: continuation removed, an explicit mention is the only trigger
|
||||
|
||||
The first version of this branch carried a **continuation** rule: once a thread held a
|
||||
mention, every later non-bot comment in that thread triggered a run with no second mention.
|
||||
The operator withdrew that requirement after reading the first PR description:
|
||||
|
||||
> Since there is no threads and the PR comments are flat, then the pm-agent should be looking
|
||||
> for explicit mentions rather than a single mention and continued scrapping of comments
|
||||
> within the PR in perpetuity
|
||||
|
||||
Read with their earlier *"the 'same thread' requirement was based on my assumption that
|
||||
comments can be hierarchical. If not, then disregard"*, the requirement is **withdrawn, not
|
||||
relocated**. So the arming state is gone, in both code and fixtures:
|
||||
|
||||
- `plan_pr_replies` no longer carries an `engaged`/armed flag. A comment is answered iff
|
||||
(1) it is not the bot's own, (2) no earlier reply of the bot's marks it answered, and
|
||||
(3) it explicitly mentions the bot account. Nothing a comment's neighbours say can make it
|
||||
actionable, so a comment with no mention produces **no reply and no model call**.
|
||||
- Applied **uniformly** — conversation comments, review bodies and inline review comments.
|
||||
Inline comments *are* genuinely threaded in Gitea, and keeping continuation for them alone
|
||||
was considered and rejected: it would be a second rule the operator did not ask for, and
|
||||
the brief said to stop and ask rather than invent a middle rule. Nothing turned out to be
|
||||
load-bearing on the arming state for the inline path (the inline path keys replies off the
|
||||
(path, position) group, which is a *destination*, not a trigger), so there was nothing to
|
||||
stop and ask about.
|
||||
- Thread identity survives, but **only as a reply destination**. "What counts as the same
|
||||
thread" needed a policy while it was part of the trigger; now it only answers "where does
|
||||
this reply go", and the two answers are Gitea's own: the conversation, or the (path,
|
||||
position) group.
|
||||
- `reason` on a plan is now always `"mention"`. The field is kept because it is what the log
|
||||
line and `PM_PLAN` print as *why you are running*, and the selftest asserts across every
|
||||
fixture that no plan ever carries another value — a second value would mean a second
|
||||
trigger had come back.
|
||||
- **What a mention still batches:** two *unanswered* mentions in one thread are one reply
|
||||
naming both keys, because the at-most-once-reply model is per reply, not per mention.
|
||||
Comments sitting between them are neither answered nor keyed.
|
||||
- The PM persona now tells the PM to **answer as if its reply is the last word it will see**,
|
||||
and to ask the human to tag it again if it needs something back — under the old rule it
|
||||
could end a reply with a question and expect the answer to reach it. It will not.
|
||||
|
||||
Kept untouched, as the brief required: the in-reply idempotency marker and the
|
||||
at-least-once-decision / at-most-once-reply failure model; the self-authored filter as the
|
||||
first check; the hostile-payload fencing and the untrusted banner; `PM_PR_MENTIONS: "false"`,
|
||||
`PM_PLAN`, `PM_DRY_RUN` implying plan mode and the two-step rollout; the `/notifications`
|
||||
query with its fallback and the `read:notification` scope note; `PM_PR_DIFF_MAX_CHARS` and
|
||||
its default. **Context assembly is unchanged** — "explicit mention required" governs what
|
||||
triggers a run and what gets replied to, not how much the model sees, so the payload still
|
||||
carries the PR, the changed files, the diff and the whole comment list. The only payload
|
||||
edit is a line telling the PM that comments which do not mention it are context, not
|
||||
questions put to it.
|
||||
|
||||
**The dispatcher was not structured the way the brief guessed, and that is worth recording.**
|
||||
The brief expected the poll loop under `ci/**` and/or `.gitea/**`. It is not there: it is
|
||||
`infra/agent/pm-dispatch.py`, a long-running Python program in the pm-agent Deployment
|
||||
@@ -63,8 +114,10 @@ There *was* a poll loop to extend, so no architecture was improvised.
|
||||
post into; ask no model, post nothing. Setting it also enables the path, so it is one knob
|
||||
to look before leaping. `PM_DRY_RUN` implies it (see "the decision I made" below).
|
||||
|
||||
`infra/agent/testdata/pr-mention-fixtures.json` (new) — 15 fixture cases driving the trigger
|
||||
logic from `PM_SELFTEST=1 pm-dispatch`, covering the six the brief required plus nine edges.
|
||||
`infra/agent/testdata/pr-mention-fixtures.json` (new) — 18 fixture cases driving the trigger
|
||||
logic from `PM_SELFTEST=1 pm-dispatch`, covering the six the brief required plus twelve edges.
|
||||
The three cases that asserted continuation are **inverted rather than deleted**, so the old
|
||||
behaviour cannot creep back unnoticed, and three were added for the revised rule.
|
||||
`infra/agent/Containerfile` COPYs them into the **verify stage only** (test data has no
|
||||
business in the shipped image) and the selftest **fails rather than skips** if it cannot find
|
||||
them. `infra/agent/pm-persona.md` gains a "When you are reading a PULL REQUEST" section, and
|
||||
@@ -76,24 +129,12 @@ believing it may pick `implement`.
|
||||
`platform/pm-agent/README.md` and `runbooks/pm-agent.md` document the path, the token scope,
|
||||
the plan mode and six new log lines.
|
||||
|
||||
### The two decisions the brief asked me to make explicitly
|
||||
### The decisions made explicitly
|
||||
|
||||
**"The same thread", for conversation comments.** The brief told me to stop and ask rather
|
||||
than pick a policy silently *if there is no existing convention in the repo*. There is one,
|
||||
so I did not stop: the issue path's trigger is "act iff the newest thing in this thread was
|
||||
not written by me", and it already treats a flat comment list as **one** conversation
|
||||
(`last_activity`, and the dispatcher's file header). I applied the same reading — a pull
|
||||
request's conversation is one thread — and refined the arming rule to match the operator's
|
||||
own words rather than the PM's paraphrase. The PM's note on the issue guessed "once the bot
|
||||
has **replied** on a PR, subsequent non-bot comments continue"; the operator wrote "a thread
|
||||
that **starts with a mention** should continue the conversation without subsequent mentions
|
||||
in follow-up questions". Those differ, and the difference is not academic: somebody who
|
||||
types three comments in a row and tags the bot only in the first would, under the PM's
|
||||
weaker reading, have the last two silently ignored. So **continuation is armed by the
|
||||
mention**, not by the reply (a marker in one of the bot's own comments also arms it, for a
|
||||
thread whose arming mention has since been edited away). Fixtures (i) and (o) pin both
|
||||
edges; (l) pins that it is per thread, not per pull request. It is still a policy and not a
|
||||
fact, and it is flagged for the operator on the issue.
|
||||
**The trigger.** Settled by the operator rather than by me: an explicit mention, and nothing
|
||||
else. See the revision section above. The earlier back-and-forth about what "the same thread"
|
||||
means for a flat comment list is moot — the question only existed because continuation needed
|
||||
a notion of sameness, and there is no continuation.
|
||||
|
||||
**Idempotency ordering.** The marker lives *inside the reply the PM posts*
|
||||
(`<!-- pm-agent:replied-to=comment-9001,review-comment-77 -->`), which makes it one atomic
|
||||
@@ -111,9 +152,11 @@ crash swallow the mention with nothing in the thread saying so. `PM_PR_COOLDOWN_
|
||||
agent work"*), any passer-by can start a run, so a bug that let the bot's replies count as
|
||||
triggers is an unbounded loop anyone could light. The self-authored check is the first thing
|
||||
`plan_pr_replies` does with each comment and it is structural — a bot-authored comment can
|
||||
only ever *clear* work, never create it, and it cannot even arm continuation, so a reply of
|
||||
ours containing our own handle is inert. The posted reply additionally never contains
|
||||
`@`-the-bot. Fixture (c) is that case and the selftest asserts the second guard too.
|
||||
only ever *clear* work, never create it, so a reply of ours containing our own handle is
|
||||
inert. The posted reply additionally never contains `@`-the-bot. Fixture (c) is that case and
|
||||
the selftest asserts the second guard too. Dropping continuation narrows this surface
|
||||
further: a reply is now reachable only from a comment that spells the handle out, and the
|
||||
bot's own output never writes it, so the guard is doubled rather than relied upon.
|
||||
|
||||
**Why it ships off.** Not one call on this path has been made against the live Gitea, and
|
||||
the endpoint it most wants is expected to be refused on today's token. `CLAUDE.md` §2b is
|
||||
@@ -122,32 +165,57 @@ rollout precedent is a staged one (`PM_DRY_RUN=true` for a day, `PM_REQUIRE_LABE
|
||||
Shipping it on would be asserting it works. `PM_DRY_RUN` was also made to imply `PM_PLAN`,
|
||||
because the runbook sells `PM_DRY_RUN` as "stop it taking new work" and a path that kept
|
||||
posting replies under it would be a footgun — and on this path the reply *is* the action
|
||||
under test and also the marker that changes future behaviour, so posting it would make a dry
|
||||
run not dry.
|
||||
under test, and it also carries the idempotency marker, so posting it would make a dry run
|
||||
not dry.
|
||||
|
||||
No requirement ID is claimed. The brief is right that nothing in the `R-*` set covers agent
|
||||
tooling, and inventing one would be worse than the gap.
|
||||
|
||||
## What I verified
|
||||
|
||||
**The offline selftest, run in this container, exit 0.** 87 assertions, all passing (55 of
|
||||
them new):
|
||||
**The offline selftest, run in this container, exit 0.** 95 assertions, all passing:
|
||||
|
||||
PM_SELFTEST=1 PM_WORKER_JOB_TEMPLATE=infra/agent/worker-job-template.yaml \
|
||||
PM_PERSONA_FILE=infra/agent/pm-persona.md python3 infra/agent/pm-dispatch.py
|
||||
-> selftest OK, rc=0
|
||||
|
||||
All 15 fixture cases pass, including the six the brief named: (a) a mention in a PR
|
||||
`PM_PERSONA_FILE` has to be named because this worker container's own image is older than
|
||||
this branch, so the baked `/etc/pm-agent/pm-persona.md` is the pre-#116 copy and the
|
||||
"persona states" assertion fails against it. In the shipped image the Containerfile COPYs
|
||||
the branch's persona to that path and the override is unnecessary.
|
||||
|
||||
All 18 fixture cases pass, including the six the brief named: (a) a mention in a PR
|
||||
conversation comment is detected; (b) a mention in an inline review comment is detected as
|
||||
its own thread; (c) a bot-authored comment is ignored *including when it contains the bot's
|
||||
own handle*; (d) an already-marked comment id is skipped; (e) a follow-up with no mention
|
||||
continues a thread; (f) an unrelated comment on an unrelated PR triggers nothing. Plus:
|
||||
`@pm-agent-worker` and `ops@pm-agent.example` are not mentions; a marker pasted by a
|
||||
non-bot author does not suppress a mention; a burst of three comments is one reply covering
|
||||
all three keys; a mention in a review *body* is a conversation trigger; two inline threads on
|
||||
one file stay separate and the newest-anchored one is taken first; continuation is per
|
||||
thread; a position-less inline comment still lands in a stable thread; and a comment made
|
||||
before the mention is not pulled in.
|
||||
own handle*; (d) an already-marked comment id is skipped; (e) a conversation follow-up with
|
||||
no mention is **not** actionable even after the bot replied; (f) an unrelated comment on an
|
||||
unrelated PR triggers nothing.
|
||||
|
||||
The cases that pin the revised rule:
|
||||
|
||||
- **(e), (m), (l) — inverted, not deleted.** (e) a conversation follow-up after a reply, (m)
|
||||
an inline follow-up in a thread the bot has already replied in, and (l) an unmentioned
|
||||
inline comment on a pull request whose conversation was answered: all three now expect
|
||||
**no reply at all**. (m) is the deliberate one — an inline thread is genuinely threaded in
|
||||
Gitea and still gets no continuation.
|
||||
- **(i) — the burst shape the operator described.** Three comments, only the first mentioning
|
||||
the bot: exactly **one** reply, keyed to the first comment only. The other two are carried
|
||||
into the payload as context and are not answered.
|
||||
- **(q) — the consequence that is now closed.** A once-mentioned-and-answered pull request
|
||||
then gets comments from `renovate` and `gitea-actions`: no reply.
|
||||
- **(p)** two *unanswered* mentions in one thread are one reply naming both keys; **(r)** a
|
||||
fresh mention after an answered one gets its own reply, keyed only to itself, so a mention
|
||||
is repeatable; **(o)** a comment made before the mention is still not pulled in.
|
||||
- Three assertions over **every** fixture at once, so a single case drifting cannot hide the
|
||||
regression: every plan's `reason` is `mention`, every comment answered matches the mention
|
||||
pattern, and no bot-authored comment is ever answered.
|
||||
|
||||
Also still passing, unchanged: `@pm-agent-worker` and `ops@pm-agent.example` (and
|
||||
`@pm-agentx`, `mail:foo@pm-agent`, `@pm-agent.worker`) are **not** mentions — which matters
|
||||
more now that the mention is the only trigger; a marker pasted by a non-bot author does not
|
||||
suppress a mention; a mention in a review *body* is a conversation trigger; two inline
|
||||
threads on one file stay separate with the newest-anchored one taken first; a position-less
|
||||
inline comment still lands in a stable thread.
|
||||
|
||||
Also asserted: the reply carries a marker for every key it answers and contains no
|
||||
`@pm-agent`; the refusal comment is marked too; `PR_ACTIONS` is exactly `comment`/`decline`
|
||||
@@ -157,21 +225,34 @@ dropping it; the assembled payload on a hostile PR (a body *and* a diff that eac
|
||||
close the fence and issue orders) has balanced fences, carries the banner twice, and quotes
|
||||
the hostile text verbatim inside a fence; truncation keeps both the head and the tail.
|
||||
|
||||
**The Dispatcher plumbing the selftest does not reach, exercised against a stub Gitea** (a
|
||||
throwaway script under `$TMPDIR`, not committed). Measured: discovery resolves a
|
||||
notification subject URL to index 120 and reports source `notifications`; plan mode printed
|
||||
the payload and posted **nothing** for four different fixtures; with plan mode off and a
|
||||
canned decision, case (a) posted into the conversation carrying marker `comment-9001` and
|
||||
case (b) posted **inline** on `infra/agent/pm-dispatch.py` position 42 carrying marker
|
||||
`review-comment-7701`; an out-of-vocabulary action (`implement`) was refused with a marked
|
||||
explanatory comment and no Job; a `GiteaForbidden` from `/notifications` fell back to
|
||||
`open-pulls`, and in `notifications`-only mode returned `([], 'notifications-unavailable')`
|
||||
rather than silently widening into the scan the operator rejected.
|
||||
**The dispatcher plumbing the selftest does not reach, re-exercised against a stub Gitea
|
||||
after the revision** (a throwaway script under `$TMPDIR`, not committed; 26 assertions, all
|
||||
passing). Measured:
|
||||
|
||||
**The merge gate, in this container:**
|
||||
- discovery resolves a notification subject URL to index 120 and reports source
|
||||
`notifications`; a `GiteaForbidden` from `/notifications` falls back to `open-pulls`; in
|
||||
`notifications`-only mode it returns `([], 'notifications-unavailable')` rather than
|
||||
silently widening into the scan the operator rejected;
|
||||
- plan mode printed the payload and posted **nothing** for four fixtures;
|
||||
- case (a) posted into the **conversation** carrying marker `comment-9001`, and case (b)
|
||||
posted **inline** on `infra/agent/pm-dispatch.py` position 42 carrying marker
|
||||
`review-comment-7701` — so the reply still goes back where the mention was made;
|
||||
- case (i) (burst, only the first mentions) produced exactly one reply whose marker names
|
||||
**only** `comment-9070`, and its payload still carried comments 9071 and 9072 as context;
|
||||
- cases (e), (m), (l) and (q) produced **no reply and no model call** — the stub counted zero
|
||||
calls into `ask_pm_pr` across all four, which is the "no reply and no model call" half of
|
||||
the brief's done-criterion measured rather than asserted;
|
||||
- case (p) produced one reply whose marker names both `comment-9100` and `comment-9102`;
|
||||
- an out-of-vocabulary action (`implement`) was refused with a marked explanatory comment and
|
||||
no Job;
|
||||
- the rendered payload says `why you are running: **mention**` and carries the line telling
|
||||
the PM that unmentioned comments are context, not questions put to it.
|
||||
|
||||
**The merge gate, in this container, re-run after the revision:**
|
||||
|
||||
./ci/gate.sh -> exit 3 (UNDETERMINED)
|
||||
Stages that did not run: spec-immutability gitea-settings
|
||||
diff base: origin/main @ 5b484f6b7884 ... HEAD @ 2fdb95393cc0
|
||||
|
||||
Nothing failed. Those are the two skips `CLAUDE.md` §3 names as expected here — the sibling
|
||||
spec repo is not reachable from the cluster and `SETTINGS_API_TOKEN` exists only in CI — and
|
||||
@@ -218,15 +299,35 @@ not (`platform/kyverno/**` is), no `rbac.yaml` and no `*.sops.yaml` is touched,
|
||||
COPY) has not been run by a container engine — there is none in this container — so the
|
||||
only evidence the selftest passes under the fixture path resolution is the repo-checkout
|
||||
run above, which takes branch 2 of `pr_fixture_path()`. CI's `images` workflow is what
|
||||
exercises branch 3.
|
||||
- **Whether the continuation policy is what the operator wants.** It is a reading of their
|
||||
words, not a fact, and it has no **end condition**: once a thread holds a mention, every
|
||||
later non-bot comment in it triggers a run (at most one reply per comment id, at most one
|
||||
per PR per 15 minutes). I did not invent a termination rule. A named consequence: another
|
||||
bot commenting on a PR where the agent was once mentioned would get a reply. There is no
|
||||
ignore-list for automated authors; adding one is a policy call, not mine.
|
||||
exercises branch 3. Related and worth knowing when reading a failure: the selftest's
|
||||
"persona states" assertion fails in *this* container against the baked
|
||||
`/etc/pm-agent/pm-persona.md`, because the image predates this branch. That is an artefact
|
||||
of where the run happened, not a defect — in the built image the two are the same file —
|
||||
but it does mean the image's own selftest invocation (`PM_SELFTEST=1 pm-dispatch`, with no
|
||||
`PM_PERSONA_FILE`) has not been observed passing with this branch's persona.
|
||||
- **The two open items from the first version of this branch are closed, and that part is
|
||||
verified offline rather than unverified.** They were "continuation has no end condition"
|
||||
and "another bot commenting on a once-mentioned PR would get a reply". Both are gone by
|
||||
construction: there is no continuation left to terminate, and fixture (q) measures that
|
||||
`renovate` and `gitea-actions` commenting on an answered pull request draw nothing. No
|
||||
ignore-list for automated authors was needed or added.
|
||||
|
||||
**The residual case, stated precisely.** A reply is still reachable by *anyone* who writes
|
||||
the literal handle, including an automation, because the operator's answer (3) is "anyone
|
||||
can talk and have the agent work" and there is no author gate. Two concrete shapes:
|
||||
(1) a bot whose comment template happens to contain `@pm-agent` — e.g. somebody quoting a
|
||||
mention into a Renovate PR description that Renovate then echoes — gets one reply per such
|
||||
comment; (2) **a quoted mention counts as a mention.** Gitea gives this program no way to
|
||||
tell `> @pm-agent what do you think?` pasted into a Markdown quote from a fresh request, so
|
||||
re-quoting an earlier mention in a new comment costs one more decision and one more reply.
|
||||
Both are bounded — one reply per comment id, one decision per pull request per 15 minutes —
|
||||
but neither is prevented. Whether to ignore quoted lines, or particular authors, is a
|
||||
policy call for the operator and not one I should take.
|
||||
- **Spend.** `PM_PR_DIFF_MAX_CHARS` defaults to 20000, so a mention costs a decision over a
|
||||
payload meaningfully larger than an issue triage's. No measurement of the real cost exists.
|
||||
Dropping continuation should reduce the *number* of runs substantially — the trigger rate
|
||||
is now how often somebody types the handle rather than how busy a once-mentioned pull
|
||||
request is — but that is reasoning, not a measurement.
|
||||
- **R-TEST-06 (spec immutability) and L2/L3 conformance** were not verified by this run —
|
||||
the two gate skips above. And the `approval-policy` stage reported N/A because there was no
|
||||
`pull_request` event; its verdict on the PR is the authoritative one.
|
||||
Reference in new issue
Block a user