Skip to content

skills(review-reviewers): require a named non-bot actor before calling output accepted - #864

Open
tend-agent wants to merge 5 commits into
mainfrom
hourly/review-31081926964
Open

skills(review-reviewers): require a named non-bot actor before calling output accepted#864
tend-agent wants to merge 5 commits into
mainfrom
hourly/review-31081926964

Conversation

@tend-agent

@tend-agent tend-agent commented Aug 6, 2026

Copy link
Copy Markdown
Collaborator

Problem

Step 2 asks the survey subagent whether each run's output was "accepted or rejected" but never says who has to do the accepting. Nothing in the prompt distinguishes a human merging a PR from the bot replying to its own review thread, so the subagent fills the gap with the most natural reading — a reply arrived, therefore someone accepted it — and reports a self-conversation as human acceptance.

That is the worst-shaped failure this skill has: it produces a false all-clear. A survey that credits the bot's own reply to a human doesn't add noise a later gate can filter, it removes the signal that would have sent the leg to Step 3.

Evidence

This leg (run 31081926964, target max-sixty/cargo-affected). The survey subagent reported PR #77 twice under "Runs with accepted output":

Outcome: ACCEPTED - human developer responded positively to feedback
Outcome: ACCEPTED - human developer incorporated feedback

and concluded PR 77: Developer accepted and incorporated bot feedback (2 cycles of acceptance). There is no developer. The PR is bot-authored and every actor on it is the bot:

$ gh api "repos/max-sixty/cargo-affected/pulls/77/comments?per_page=100" --jq '[.[].user.login] | unique'
["cargo-affected-bot"]

The same report also listed the PR author as cargo-affected-bot, so the contradiction was internal to the summary — the acceptance claim was reached without ever comparing the replying login against the bot's.

This is the fourth cheap-subagent mis-attribution recorded in the evidence gist, and the second of this specific sub-class (bot activity attributed to a human):

Occurrence Shape
gist "Analysis-side", leg ending 07:5x inline replies + commit 551e059 credited to "human applied suggested changes"
gist "Analysis-side", following leg PR #54 bodies credited to a run that had not yet started
gist "Analysis-side", streak-break leg fabricated no-op causes; window boundary ignored
this leg bot's own reply chain on PR #77 reported as human acceptance

Prior legs' response was to stop delegating — ten consecutive legs record "this leg used no survey subagent". That is a per-leg workaround for a defect in the shared prompt, and it leaves the skill's stated default (Delegate all broad exploration to a cheap subagent) pointing at a step that mis-reports actors.

Fix

Three small changes to Step 2, all inside the prompt template:

  • Require the subagent to name the login behind every acceptance/rejection signal, sourced from a single gh pr view --json number,state,author,mergedBy,reviews,comments,commits that covers all five actor surfaces — merge actor, reviews (with state), inline comments, conversation comments, and commits. A comments-only check can't source it: neither comments endpoint carries review records or the merge actor, so a maintainer merging a bot PR without commenting would come back as bot-only. Inline commenters need no separate call — every inline comment, including a standalone reply, belongs to a review record that reviews returns. The block notes that commits truncates at 100 (oldest-first) while comments and reviews paginate in full.
  • Give bot-only threads their own report bucket (bot-only — no human signal) so they can't be silently filed under "accepted".
  • Tell the main agent to verify actor attribution before it enters a finding, and to judge bot-only threads on content rather than treating the bucket as either outcome.

The rule is stated so it stays correct when the bot legitimately works on its own PR: bot-only is not a defect (a self-review followed by the author's fix is designed behavior), it just isn't acceptance. Nothing here discourages delegation — it fixes the instruction the delegation was missing.

Gate assessment

  • Evidence level: High — consistent pattern across multiple sessions. 4 occurrences total, 2 in the human/bot sub-class this change addresses. High needs 2–3.
  • Structural or stochastic: structural in the prompt. The subagent is never asked for actor identity, so no wording of the reply can be relied on to supply it; which specific claim it invents is stochastic, the omission is not.
  • Change type: targeted fix — a missing requirement in an existing prompt, plus a report bucket. Normal bar, met.
  • Passes both gates.

Second finding this leg did not pass and is recorded in the gist instead: the three-hop tend-mention chain on the same PR #77. That one is already fixed by open PR #849, and I've added this window's evidence there rather than opening a duplicate.

@tend-agent tend-agent added the claude-behavior Behavioral issues found by review-reviewers label Aug 6, 2026

@tend-agent tend-agent left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The rule is right, but the two queries handed to the subagent can't produce the login the report format now asks for. /pulls/<pr>/comments returns only inline review comments and /issues/<n>/comments only conversation comments — neither carries review records or the merge actor. So — bot reviewed, PR merged by <login> has no source, and the most common acceptance shape here (maintainer merges a bot PR without commenting) comes back as bot-only — no human signal. That flips the failure rather than closing it: instead of a false all-clear, a real acceptance gets filed as no-signal, and a human CHANGES_REQUESTED with no inline comments — the first entry in the negative-signal list just below — is invisible to both queries too.

Checked against this repo: #843 has inline=[tend-agent] conv=[] reviews=[tend-agent] yet merged_by: max-sixty; #844 has all three participant surfaces empty with the same merger. Both would report as bot-only under the block as written.

Second, smaller: the new bucket has no branch in the routing sentence. With bot-only populated and nothing concerning, neither "all outputs are accepted" nor "concerning outcomes exist" holds, so the leg has no stated destination — "judge them on content" resolves it in prose but the condition it feeds is still binary.

Comment thread plugins/tend-ci-runner/skills/review-reviewers/SKILL.md Outdated
Comment thread plugins/tend-ci-runner/skills/review-reviewers/SKILL.md Outdated
… comments

The two comments endpoints can't produce the login the report format asks
for: /pulls/<pr>/comments carries only inline review comments and
/issues/<n>/comments only conversation comments. Neither reaches review
records or the merge actor, so the most common acceptance shape here — a
maintainer merging a bot PR without commenting — came back as bot-only, and
a human CHANGES_REQUESTED with no inline comments was invisible too.

Add the merge-actor, reviews, and commits queries, and give the bot-only
bucket a branch in the routing sentence.
max-sixty pushed a commit that referenced this pull request Aug 7, 2026
…a silent survey (#885)

This hour's Step 2 survey subagent reported **"No visible bot output in
time window"** across 23 successful runs on `max-sixty/tend`. The window
in fact contained 9 reviews (two `APPROVED`), 11 inline review comments,
and 1 conversation comment from `tend-agent` — one of the busiest
windows in the series.

## Evidence

Window 2026-08-07T07:27Z → 08:16Z. The survey's verdict, verbatim:

> **No visible bot output in time window.** All 23 successful runs
(tend-mention, tend-review, tend-review-runs, tend-notifications)
executed and completed but produced zero comments, reviews, or inline
code comments between 2026-08-07T07:27:00Z and 08:16:00Z on tracked
PRs/issues (809, 818, 834, 858, 863, 868, 875, 877, 878, 881, 816, 830).

Two run-independent calls, taking no run ID at all, contradict it
immediately:

```
gh api "repos/max-sixty/tend/issues/comments?since=2026-08-07T07:27:00Z&per_page=100" → 1 tend-agent row
gh api "repos/max-sixty/tend/pulls/comments?since=2026-08-07T07:27:00Z&per_page=100"  → 11 tend-agent rows
```

Plus reviews `4880965990` (COMMENTED), `4881043005` (COMMENTED) and
`4881079314` (APPROVED) on
[#881](#881), `4881113428`
(APPROVED) on [#878](#878), and
five more (all COMMENTED) on
[#809](#809). The survey had
listed 881, 878 and 809 among the PRs it checked, so the numbers were
right and the reads came back empty anyway.

The second-order cost is worse than the omission. Having established
silence, the survey reasoned *from* it: it flagged the window's seven
bot-PR merges as "merged by max-sixty without formal review workflow",
concluding "**direct push/merge bypassing review requirement**, or
**review workflow override via branch protection rule bypass**". Six of
the seven (#818, #834, #858, #868, #875, #877) carry bot `COMMENTED`
reviews predating the window, which is why a `since`-filtered read
missed them; the seventh (#863) is the skill-authorized silence on a
self-authored PR with no concerns. None reads `APPROVED` because GitHub
blocks self-approval — the ordinary shape for a bot PR, not a bypassed
control. Acted on, that summary is a false security finding against the
maintainer.

## Root cause

Every path Step 2 offers is run-keyed: run → `headBranch` → PR →
endpoint. That chain is fine when it works, but it has one failure mode
with no floor — break it anywhere and *every* run returns empty
simultaneously. Uniform absence is exactly what a genuinely quiet hour
looks like, so the summary that comes back is self-consistent and
carries no signal that anything went wrong. The existing sanity-check
line ("note if zero bot activity found across all runs") did fire here,
and the subagent talked itself out of it in the same paragraph — a
prompt to notice absence can't distinguish the two causes, because
nothing in a run-keyed survey can.

## Change

Adds a sweep block to the top of the Step 2 prompt that takes no run ID
— the two `?since=` comment endpoints, bounded on `created_at` at both
ends; a `pr list --search "updated:>"` for the candidate list the review
queries need; and a loop over those candidates counting bot reviews
submitted inside the window, since neither comment endpoint returns
review submissions and an empty-body `APPROVE` is `tend-review`'s most
common output — with instruction to report all four counts and to re-map
from what they found rather than reporting those runs silent. Adds one
sentence at the main-agent review point: an all-quiet report without the
counts isn't usable, and absence isn't a finding to reason from.

This is the check that caught the failure this run. It is four counts
off two run-independent endpoints and one search, and it fails
independently of the mapping it is checking.

## Relation to the other open Step 2 PRs

Distinct problems, non-overlapping edits.
[#864](#864) fixes *who* accepted
(named non-bot actor);
[#869](#869) fixes *which run*
produced an output (confirm from the posting run's log). Both still
start from a candidate PR list reached by run-keyed mapping — neither
makes "no output at all" falsifiable, which is the failure here.

## Gate assessment

- **Evidence level**: High — survey unreliability is recorded in the
evidence gist across prior windows, cumulative **4 → 5** with this one.
High needs 2–3. Prior occurrences were omissions of individual runs and
one mislabelled silence; this is the first categorical zero-output
claim, and the first to produce a fabricated inference from the absence.
- **Structural**: the *specific* empty read is stochastic, but the
skill's exposure is not — Step 2 offers only run-keyed paths, so any
mapping break yields a plausible, uniform, unfalsifiable silence. Replay
it and the summary is equally convincing every time.
- **Change type**: targeted fix (one query block, one sentence) — normal
bar, met.
- **Passes both gates.**

Evidence: https://gist.github.com/e08f6e62d6478163cb425a75648eb7e4

---------

Co-authored-by: tend-agent <270458913+tend-agent@users.noreply.github.com>
max-sixty pushed a commit that referenced this pull request Aug 7, 2026
…nst tend before filing upstream (#891)

## Problem

Two dedup blocks were blind in two different ways, and the cited
duplicate needed both fixed.

**State filter.** `review-runs` Step 5 and `review-reviewers` Step 4
both deduped against PRs with `gh pr list --state open`. A merged PR is
never returned by that query, so a finding whose fix already landed
reads as undeduped and gets filed again. `running-in-ci`'s PR-creation
dedup recheck already gets this right ("with `--state all` so closed and
merged siblings show up"); these two recipes contradicted it.

**Repo scope.** `review-runs` is a generated workflow
([`generator/src/tend/config.py:24`](https://github.com/max-sixty/tend/blob/f65f49f/generator/src/tend/config.py#L24)
lists it in the enabled set), so it runs in each adopter's checkout and
an unqualified `gh pr list` returns *the adopter's* PRs. Step 6 routes
bundled-skill defects upstream to tend, but neither Step 5 nor any of
`running-in-ci`'s dedup recipes — all local-repo — told the agent to
dedup in the target repo before filing there. `--state all` alone does
not close this: the adopter's PR list never contained the upstream fix
at any state.

`review-reviewers` is unaffected by the second half. It runs in
`max-sixty/tend` and files onto tend, so its unqualified `gh pr list`
already resolves to the right repo; only the state filter was wrong
there.

This bites hardest on tend specifically, because of the pinning model:
adopters call `max-sixty/tend/<harness>@X.Y.Z`, so a merged skill fix
stays dormant on their repos until the next release tags. The bug keeps
reproducing after the fix merges — which is exactly the window in which
the analysis legs are looking at it, and exactly when the dedup queries
are blind to the fix.

## What happened

`max-sixty/cargo-affected`'s `tend-review-runs` run
[31160677649](https://github.com/max-sixty/cargo-affected/actions/runs/31160677649)
(08:11:33Z → 08:21:41Z) hit the `| last` evidence-log mis-selection: it
appended ~12 KB of run evidence into the nightly's unrelated comment on
target [#73](max-sixty/cargo-affected#73),
noticed on its post-verify read, restored comment `5188771252`, and
re-appended to the real log `5150650688`. Good recovery. It then filed
[#883](#883) upstream, whose
"Proposed fix" is a `## Run ` heading predicate on the comment selector.

[#875](#875) merged that exact fix
at 07:34:40Z — 46 minutes before the issue was filed — as `test("^## Run
[0-9]")` on the same selector, in the same file. #883 is a duplicate of
a merged PR.

The run made three dedup queries before filing (`gh issue list --state
all --search "tracking issue comment append"`, a broader `gh issue list
--state all` title regex, and a final `gh issue list --state open`
recheck). All three were `gh issue list`, which never returns PRs — and
all three ran against `max-sixty/cargo-affected`. Even had it run Step
5's PR line verbatim, it would not have returned #875, for both reasons:
the state filter excluded merged PRs, and the query's repo was the
adopter's, not tend's.

## The fix

- Both skills: `gh pr list --state open` → `--state all`, projecting
`state,mergedAt` so a merged hit is legible.
- `review-runs` only: add the cross-repo pair (`gh pr list`/`gh issue
list --repo max-sixty/tend --state all`) so a finding heading upstream
under Step 6 is deduped against tend first.
- `review-runs` only: the pinning note is scoped to the upstream repo,
since in that skill the reader is the adopter and the local `gh pr list`
above it has nothing to do with pinned refs. `review-reviewers` keeps
the original wording, where tend is the reader and "on adopters" is the
correct direction.

Both added commands were run against this repo to confirm they parse and
return the expected shape.

## Gate assessment

- **Evidence level**: High. **Occurrences: 1** direct, verified end to
end (session log, both dedup query sets, #875's merge time and diff,
#883's body).
- **Structural, not stochastic.** `gh pr list --state open`
deterministically cannot return a merged PR, and a query scoped to the
adopter's repo deterministically cannot return a tend PR; replayed ten
times it misses #875 ten times. There is no decision point.
- **Change type**: targeted fix — query lines plus one sentence naming
the pinning consequence. It brings both recipes into line with a rule
the same plugin already states in `running-in-ci`, rather than
introducing new policy.
- **Why act at one occurrence**: the six-PR batch merged at 07:34:40Z
([#875](#875),
[#834](#834),
[#868](#868),
[#818](#818),
[#877](#877),
[#858](#858)) is all unreleased,
so six distinct bugs remain observable on every adopter until the next
release. The first analysis leg after that batch produced the first
duplicate. The exposure is six-wide and standing, not one-off.

## Not covered by

No open PR touches either dedup block.
[#845](https://github.com/max-sixty/tend/pull/845)/[#850](https://github.com/max-sixty/tend/pull/850)/[#838](https://github.com/max-sixty/tend/pull/838)
are the run-window cluster;
[#849](#849),
[#856](#856),
[#857](#857),
[#864](#864),
[#869](#869),
[#876](#876),
[#809](#809),
[#837](#837),
[#821](#821),
[#836](#836) are elsewhere.

#883 stays open as a maintainer call — it is redundant with #875 but the
option-2 half of its body (finish the gist migration for `review-runs`)
is not.

Evidence: https://gist.github.com/dca23a6e6a0d8cae2665944ba31676fb

---------

Co-authored-by: tend-agent <270458913+tend-agent@users.noreply.github.com>
# Conflicts:
#	plugins/tend-ci-runner/skills/review-reviewers/SKILL.md
> gh api "repos/$ARGUMENTS/pulls/<pr>" --jq '{merged_by: .merged_by.login, merged_at: .merged_at}'
> gh api "repos/$ARGUMENTS/pulls/<pr>/reviews?per_page=100" --jq '[.[] | {login: .user.login, state}] | unique'
> gh api "repos/$ARGUMENTS/pulls/<pr>/comments?per_page=100" --jq '[.[].user.login] | unique'
> gh api "repos/$ARGUMENTS/issues/<n>/comments?per_page=100" --jq '[.[].user.login] | unique'

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we can't do all of these with a single query?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes — one gh pr view carries all five surfaces, and it's now a single call in 04d9671:

gh -R $ARGUMENTS pr view <pr> --json number,state,author,mergedBy,reviews,comments,commits --jq '{
  pr: .number, state, author: .author.login, merged_by: .mergedBy.login,
  reviews: [.reviews[] | {login: .author.login, state}],
  logins: ([.mergedBy.login, .reviews[].author.login, .comments[].author.login,
            .commits[].authors[].login] | map(select(. != null and . != "")) | unique)
}'

The part I wasn't sure of was inline comments, since reviews and /pulls/<pr>/comments looked like separate surfaces. They aren't: every inline comment carries a pull_request_review_id, including a standalone reply posted through the replies endpoint, and each of those review records shows up in reviews. On this PR all five inline comments map onto the four review records gh pr view returns, so nothing is lost by dropping the separate call.

Checked against the two cases the earlier round turned on — #843 comes back merged_by: max-sixty, logins: ["max-sixty","tend-agent"] (real acceptance), and max-sixty/cargo-affected#77, the false all-clear this PR exists for, comes back logins: ["cargo-affected-bot"]. Also confirmed comments/reviews/commits paginate past 100 rather than truncating, so logins stays sound on a long thread.

Verification

Inline comments → review records on this PR:

$ gh api "repos/max-sixty/tend/pulls/864/comments?per_page=100" --jq '.[] | {id, user: .user.login, pull_request_review_id, in_reply_to_id}'
{"id":3726977948,"in_reply_to_id":null,"pull_request_review_id":4872393980,"user":"tend-agent"}
{"id":3726977955,"in_reply_to_id":null,"pull_request_review_id":4872393980,"user":"tend-agent"}
{"id":3727008234,"in_reply_to_id":3726977948,"pull_request_review_id":4872431767,"user":"tend-agent"}
{"id":3727008359,"in_reply_to_id":3726977955,"pull_request_review_id":4872431925,"user":"tend-agent"}
{"id":3740688673,"in_reply_to_id":null,"pull_request_review_id":4888824594,"user":"max-sixty"}

All four of those review IDs appear in gh pr view 864 --json reviews (as PRR_… node IDs), including 4872431767 / 4872431925, which were created implicitly by reply-only comments.

Outcome on the two decisive PRs:

$ ... pr view 843   → {"merged_by":"max-sixty","logins":["max-sixty","tend-agent"],"state":"MERGED"}
$ ... pr view 77 -R max-sixty/cargo-affected → {"merged_by":null,"logins":["cargo-affected-bot"],"state":"OPEN"}

Pagination, against a --paginated REST count on rust-lang/rust#137944: comments 210 vs 210, reviews 129 vs 129.

@tend-agent tend-agent left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The single-query collapse works — I ran it against #843 and this PR and the projection is right. One claim in the prose beside it isn't: commits doesn't paginate, and that's the one surface where truncation would recreate the false all-clear this PR exists to prevent.

How I checked

gh pr view builds the commits selection as commits(first: 100) (api/query_builder.go, var prCommits), and pkg/cmd/pr/shared/finder.go only preloads further pages for reviews, comments, closingIssuesReferences, and statusCheckRollup — there is no preloadPrCommits.

Empirically, on a 950-commit PR:

$ gh -R NixOS/nixpkgs pr view 541843 --json commits --jq '.commits | length'
100
$ gh -R NixOS/nixpkgs pr view 541843 --json commits --jq '.commits[-1].oid'
e6b2583ec23ed8e6024ebb6a38541969c6a48c7e
$ gh -R NixOS/nixpkgs pr view 541843 --json headRefOid --jq .headRefOid
edc768bb34e2330a613a73af7f19df710cc63d7a

The connection is oldest-first, so the commits that get dropped are the newest — the "who pushed the follow-up" signal the rule above specifically asks for. A human pushing commit 101 to a long bot PR would come back logins: ["$BOT_LOGIN"].

reviews and comments do paginate as claimed; I confirmed the preload functions exist for both.

Comment thread plugins/tend-ci-runner/skills/review-reviewers/SKILL.md Outdated
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

claude-behavior Behavioral issues found by review-reviewers

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants