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)
334 lines
23 KiB
Markdown
334 lines
23 KiB
Markdown
---
|
|
date: 2026-11-07
|
|
branch: feat/pm-agent-pr-mentions
|
|
title: The PM agent can be @-mentioned on a pull request — an EXPLICIT mention is the only trigger, 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.
|
|
|
|
### 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
|
|
(`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) — 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
|
|
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 decisions made explicitly
|
|
|
|
**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
|
|
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, 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
|
|
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 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.** 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
|
|
|
|
`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 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`
|
|
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, re-exercised against a stub Gitea
|
|
after the revision** (a throwaway script under `$TMPDIR`, not committed; 26 assertions, all
|
|
passing). Measured:
|
|
|
|
- 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
|
|
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. 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.
|