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 implement)
This commit is contained in:
pm-agent committed
1 parent
5b484f6b78
commit
2fdb95393c
9 files changed
+2055
-32
No files matched your search
@@ -0,0 +1,232 @@
|
||||
---
|
||||
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
|
||||
---
|
||||
|
||||
## What changed
|
||||
|
||||
Issue #116 asked for the PM agent to react when it is `@`-mentioned on a pull request: read
|
||||
the PR, the changes and the thread, and reply in that thread. This branch implements the
|
||||
**trigger, context assembly, dispatch and reply** half. The change-authoring half (the
|
||||
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.
|
||||
|
||||
**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
|
||||
(`platform/pm-agent/`). `ci/**` and `.gitea/**` are the merge gate and have nothing to do
|
||||
with it. **So this PR touches no approval-policy sensitive path** — I checked every changed
|
||||
path against `infra/gitea/approval-policy.json` by hand (see "What I verified") — and the
|
||||
`ci/approval-policy.sh` stage should pass on a green gate without a human review. The PM's
|
||||
own warning on the issue that the PR would sit red was an honest guess from an unverified
|
||||
belief about where the code lived; it was wrong, and nothing about the change needed to move
|
||||
to make it so.
|
||||
|
||||
There *was* a poll loop to extend, so no architecture was improvised.
|
||||
|
||||
`infra/agent/pm-dispatch.py`:
|
||||
|
||||
- A second, independently-guarded pass per poll — `cycle_pr_mentions()`, called from `run()`
|
||||
rather than from `cycle()`. Two reasons: the issue cycle returns early at the worker cap
|
||||
and a mention creates no worker, so it must not be suppressed by a cap it cannot
|
||||
contribute to; and a Gitea failure on one path must not silence the other.
|
||||
- **Discovery** (`discover_pr_candidates`). Tries Gitea's notification list for the bot
|
||||
account — one bounded call, which is the "way to check for '@'s for the bot account
|
||||
instead of scouring the entire pull request list" the operator asked for — and falls back
|
||||
to listing open pull requests. Mode is a knob (`auto` / `notifications` / `pulls`), and
|
||||
which query answered is logged every cycle. **A 401/403 is handled as its own case**
|
||||
(`GiteaForbidden`) because on this service it is almost certainly a token scope, not a bug:
|
||||
`/api/v1/notifications` sits behind Gitea's `notification` scope category and the bot's
|
||||
token is minted `read:user, write:repository, write:issue, read:package`. The log names
|
||||
`read:notification` as the remedy instead of reporting "no mentions found".
|
||||
- **Detection** (`normalize_pr_entries`, `plan_pr_replies`) — pure functions over normalized
|
||||
comments, so the parts that must be provable offline are provable offline. Reads all three
|
||||
Gitea resources: conversation comments (`/issues/{n}/comments`), review bodies
|
||||
(`/pulls/{n}/reviews`) and inline review comments (`.../reviews/{id}/comments`).
|
||||
- **Context assembly** (`render_pr_thread`) — PR title and body, the changed-file summary,
|
||||
a bounded diff, the full thread being answered with the anchor comment marked, and the
|
||||
surrounding comments. Every field that came from a person goes through a single new
|
||||
`fence()` helper under an explicit untrusted banner; `render_thread` was refactored onto
|
||||
the same helper, which removes a twice-retyped `.replace("```", "`` `")`.
|
||||
- **A narrowed decision shape.** `PR_ACTIONS = ("comment", "decline")` and
|
||||
`PR_DECISION_SCHEMA` with **no `branch` and no `brief` property at all** — the PM has no
|
||||
field in which to ask for work. `handle_pull()` refuses anything outside that set (and
|
||||
still marks it answered, so an unusable decision costs one decision rather than repeating
|
||||
forever), and nothing on this path calls `build_job()`.
|
||||
- **Replying in the thread.** Conversation threads get an issue comment. Inline threads get
|
||||
a one-comment review (`POST /pulls/{n}/reviews`, `event: COMMENT` — never `APPROVE`),
|
||||
which is the only way Gitea creates an inline comment; if Gitea refuses it the reply falls
|
||||
back to the conversation naming the file and position, because a mention answered in the
|
||||
wrong place is better than one answered nowhere.
|
||||
- **`PM_PLAN`** — discover, assemble, print the exact payload and the exact thread it would
|
||||
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/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
|
||||
the selftest asserts it is there — without it the PM would be answering a mention while still
|
||||
believing it may pick `implement`.
|
||||
|
||||
`platform/pm-agent/configmap.yaml` adds the knobs with `PM_PR_MENTIONS: "false"`;
|
||||
`deployment.yaml` bumps `config-generation` 5 → 6 in the same commit, as that file requires.
|
||||
`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 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.
|
||||
|
||||
**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
|
||||
write with the reply and therefore impossible to get out of step with it. The failure mode
|
||||
chosen, deliberately, is **at-least-once model call, at-most-once reply**: a crash before
|
||||
posting costs one repeated decision; a crash after posting is a no-op because the marker is
|
||||
already in the thread; and a POST that errors while Gitea actually created the comment is
|
||||
also a no-op, because nothing is retried inside the cycle and the next cycle re-reads the
|
||||
list. A mention is therefore never silently dropped and a reply is never duplicated. The
|
||||
rejected alternative — write a marker *before* asking the model — buys cheapness by making a
|
||||
crash swallow the mention with nothing in the thread saying so. `PM_PR_COOLDOWN_SECONDS`
|
||||
(900s) bounds the cost of the repeat, which is why paying it is the cheap side of the trade.
|
||||
|
||||
**Loop prevention.** With no author gate (operator answer 3: *"anyone can talk and have the
|
||||
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.
|
||||
|
||||
**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
|
||||
explicit that this repository is not evidence about anything live, and the service's own
|
||||
rollout precedent is a staged one (`PM_DRY_RUN=true` for a day, `PM_REQUIRE_LABEL=agent`).
|
||||
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.
|
||||
|
||||
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):
|
||||
|
||||
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
|
||||
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.
|
||||
|
||||
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`
|
||||
and a subset of `ACTIONS`; `PR_DECISION_SCHEMA` has no `branch`/`brief` and
|
||||
`additionalProperties: false`; `fence()` breaks an embedded triple backtick rather than
|
||||
dropping it; the assembled payload on a hostile PR (a body *and* a diff that each try to
|
||||
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 merge gate, in this container:**
|
||||
|
||||
./ci/gate.sh -> exit 3 (UNDETERMINED)
|
||||
Stages that did not run: spec-immutability gitea-settings
|
||||
|
||||
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
|
||||
they are the *only* two. `decisions-log`, `lint`, `l0`, `render-golden`, `l1` and
|
||||
`approval-policy` all reported green.
|
||||
|
||||
**That this PR touches no sensitive path**, checked path by path against the
|
||||
`sensitive_paths` globs in `infra/gitea/approval-policy.json`: the changed files are under
|
||||
`infra/agent/`, `platform/pm-agent/`, `runbooks/` and `decisions/`. `infra/agent/**` is not
|
||||
in the list (`infra/gitea/**` is, and is a different directory), `platform/pm-agent/**` is
|
||||
not (`platform/kyverno/**` is), no `rbac.yaml` and no `*.sops.yaml` is touched, and
|
||||
`platform-config/kustomization.yaml` and `CLAUDE.md` are untouched. The gate's own
|
||||
`approval-policy` stage is the authority on this and it only evaluates on a
|
||||
`pull_request` event, so **read its verdict on the PR** rather than trusting this paragraph.
|
||||
|
||||
## What I did NOT verify
|
||||
|
||||
- **Nothing here has run against the live Gitea, and nothing here is deployed.** I held no
|
||||
Gitea credential, no kubeconfig and no ServiceAccount token: this container has egress to
|
||||
the in-cluster Gitea and the Anthropic API only, and `kubectl` fails. Every claim below is
|
||||
therefore an operator measurement, not mine.
|
||||
- **Every one of the five new Gitea API calls is UNEXERCISED.** `/notifications`,
|
||||
`/pulls/{n}`, `/pulls/{n}/files`, `/pulls/{n}.diff`, `/pulls/{n}/reviews`,
|
||||
`.../reviews/{id}/comments` and `POST /pulls/{n}/reviews` were written to Gitea's published
|
||||
swagger for the 1.24 line. The fixtures prove this program *parses* those shapes; they
|
||||
prove nothing about whether this Gitea *emits* them. The exact read-only curl commands to
|
||||
settle it are in `platform/pm-agent/README.md`, and replacing the fixtures with a real
|
||||
capture is the cheapest way to close the gap.
|
||||
- **That `/api/v1/notifications` needs `read:notification` is read from Gitea's documented
|
||||
scope model, not measured.** It is the reason the fallback and the named-remedy log line
|
||||
exist; if the endpoint turns out to work on today's token, the fallback simply never fires.
|
||||
- **Whether a notification is actually raised for a mention in an *inline review* comment,
|
||||
and whether `subject.url` is populated for it.** If it is not, discovery via notifications
|
||||
will miss inline mentions and only the `pulls` fallback will find them. Untested; it is the
|
||||
single most likely gap in the cheap query.
|
||||
- **Whether `POST /pulls/{n}/reviews` with one comment lands in the *existing* inline
|
||||
thread** or opens a second conversation at the same line. Gitea groups by (path, position)
|
||||
in its UI, which is why this should work, but I have not seen it. The fallback covers
|
||||
outright refusal, not mis-placement.
|
||||
- **Whether `position` from a stored review comment is still a valid `new_position` on the
|
||||
current head.** It will not be after a force-push, which is exactly when the inline
|
||||
fallback fires.
|
||||
- **The image.** Nothing was built. The Containerfile `verify` stage change (the fixture
|
||||
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.
|
||||
- **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.
|
||||
- **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