Skip to content

skills(review-reviewers): confirm run→output attribution from the posting run's log - #869

Open
tend-agent wants to merge 4 commits into
mainfrom
hourly/review-31085980872-cargo-affected
Open

skills(review-reviewers): confirm run→output attribution from the posting run's log#869
tend-agent wants to merge 4 commits into
mainfrom
hourly/review-31085980872-cargo-affected

Conversation

@tend-agent

Copy link
Copy Markdown
Collaborator

A review-reviewers leg named the wrong run as the author of a public comment, and that run ID then shipped in a public comment on an open PR (#849). The mis-attribution came from mapping outputs to runs by wall-clock inclusion — the comment's timestamp fell inside that run's span, so it was credited to it. Several runs are live at once on a busy PR, so the span proves nothing.

What actually happened

On max-sixty/cargo-affected PR #77, four bot sessions overlapped between 07:21Z and 07:41Z. Attribution read from each session's own log (the tool call that posted):

Output Credited in #849 to Actually posted by (session log)
commit 56406d76 + inline reply 3726782508 mention handle 31081009768 tend-nightly 31080083613 — the run that opened the PR, still running
inline reply 3726818957 mention handle 31081269852 mention handle 31081009768

31081269852 and 31081591885 — the two hops triggered by the synthetic reply containers that #849 exists to suppress — each ran a read-only sweep and posted nothing. Their logs contain no posting call and both end with an explicit silent-exit decision.

This matters for #849: its comment escalates the case to "an unguarded container caused the bot to post additional public output." That did not happen in this window. Both public replies came from a hop that #849's own predicate keeps firing (the genuine self-review) and from a racing sibling session outside the chain entirely. The guard's real value — suppressing two no-op agent runs and their queue slots — is unchanged. Correction posted on the PR.

Root cause

Step 2's run→output mapping is branch- and time-based, which is the right cheap first pass. Nothing then tells the analyst that the mapping is provisional. Step 3 escalates to session logs only on "negative outcome signals (or suspicious lack of output)", so a finding whose claim is an attribution — this run did that — never triggers the one check that could confirm it. The leg that produced the error recorded "session-log inspection: not needed — the mention chain's decisions are fully determined by the review records", which is true of the decisions and false of the authorship.

Change

One paragraph in Step 3 adding a third escalation trigger: when a finding will name which run produced an output, confirm it from that run's log, with the jq | grep for the posting call. Ten lines, at the point where the escalation decision is made.

Gate assessment

  • Evidence level: High. Direct occurrence this window, confirmed against four session logs. The class — attribution errors reaching a finding — stands at 5 across recorded legs: three cheap-subagent mis-attributions, the human-vs-bot actor case now addressed by #864, and this run-vs-run case. The evidence log's standing carry pre-registered exactly this ("verify any subagent claim before it enters a finding, especially actor attribution and run→output mapping"); it has now fired for the second half.
  • Occurrences: 1 direct + 4 historical in class, against a High threshold of 2–3.
  • Structural: yes. Overlapping runs make wall-clock attribution ambiguous every time; the skill offers no other attribution method, so the same reasoning reproduces the same error.
  • Change type: targeted fix (one paragraph + a recipe at an existing decision point), Normal bar.
  • Relation to skills(review-reviewers): require a named non-bot actor before calling output accepted #864: complementary and non-overlapping. skills(review-reviewers): require a named non-bot actor before calling output accepted #864 fixes actor attribution (which login) in Step 2; this fixes run attribution (which session) in Step 3. Textually adjacent, so whichever lands second wants a trivial rebase.

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

…ting run's log

A finding named the wrong run as the author of a public comment: the
comment's timestamp fell inside that run's span, so it was credited to it.
Overlapping sessions on one PR make wall-clock inclusion no evidence of
authorship. Escalate to session logs whenever a finding will name which run
produced an output, and confirm from the posting tool call.

@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 escalation trigger is the right call, but the recipe that implements it can't actually separate a posting run from a read-only one — I ran it against real session logs from this repo and it fails in both directions.

False positives. /(reviews|comments)\b matches GETs. Every run that merely reads the thread — gh api repos/O/R/pulls/N/comments, gh api repos/O/R/issues/comments/ID — trips it, and reading comments is the first thing nearly every tend run does (the review skill's own pre-flight does exactly that). On a subagent log from this repo's most recent review-reviewers leg, the recipe returns three "posting calls"; all three are reads and that subagent posted nothing.

False negatives — the dangerous direction. .[0:200] truncates the tool input before the grep sees it, so a posting call inside a longer command is invisible and the run reads as "posted nothing". This PR's own creation is the demonstration: the run that authored it committed and pushed in one 928-character Bash call, with git push at character 664. The recipe applied to that run's log does not see the push. Across the three logs I checked, truncation drops half the matches (3/6 and 7/15).

Both modes reproduce the mis-attribution the paragraph exists to prevent — the second one especially, since "no posting call in the log" is the sentence the analyst is being told to trust.

Matching the write shape rather than the endpoint fixes both; suggestion inline. Verified against the same three logs: zero matches on the subagent that posted nothing, and the git push and gh issue comment calls both caught.

Two smaller notes on the surrounding sentence, not worth blocking on:

  • A run's output can be posted from a subagent, whose log is a separate subagents/agent-*.jsonl under the session directory. FILE singular invites checking only the top-level log; applying it across every *.jsonl in the downloaded run directory would close that gap.
  • The query is Claude-JSONL-shaped, but Step 3 downloads codex-session-logs* too. On a Codex log it returns nothing regardless of what the run did, which lands as "posted nothing" under the concluding sentence. Pre-existing across the other recipes here, so it's the absolute phrasing that's new.
Verification

Logs used (this repo, current artifacts): claude-session-logs-n866 (a triage run that posted), and claude-session-logs-cdcbac03 — the review-reviewers leg that authored this PR, which ships both a top-level session log and a subagents/agent-*.jsonl.

The recipe as written, against the subagent log that posted nothing:

Bash: {"command":"\n# Check reviews on PR #76\n... api repos/max-sixty/cargo-affected/pulls/76/reviews ...
Bash: {"command":"\n# Get review comments correctly\n... api repos/max-sixty/cargo-affected/pulls/77/comments ...
Bash: {"command":"gh api -R max-sixty/cargo-affected repos/max-sixty/cargo-affected/pulls/77/comments ...

The truncation, against the call that created this PR — the full input is 928 chars and the first posting token sits at 664:

{"command":"cd /home/runner/work/tend/tend\nBRANCH=\"hourly/review-$GITHUB_RUN_ID-cargo-affected\"\ngit checkout -q -b \"$BRANCH\"\ngit add plugins/tend-ci-runner/skills/review-reviewers/SKILL.md\ngit

That is everything the grep receives; git push -u origin "$BRANCH" is past the cut.

The suggested form, same three logs: no match on the read-only subagent, gh issue comment 866 --body-file, git push -u origin "$BRANCH", and gh issue comment 849 --body-file all caught. The remaining noise is the leg quoting grep patterns into the diff it was authoring, which is specific to analyzing a log about posting.

Comment thread plugins/tend-ci-runner/skills/review-reviewers/SKILL.md Outdated
The attribution recipe grepped /(reviews|comments)\b against a tool input
truncated to 200 chars, so it flagged every run that merely read the thread
and missed pushes and comments past the cut. Match write-shaped calls on the
untruncated input, scan subagent logs too, and add a Codex variant.

@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 rewritten recipe holds up — I re-ran both patterns over eight session-log artifacts from this repo. The read-only review-reviewers leg (31087767524) that the old pattern would have flagged now returns zero, and the runs that did post are all caught, including the multi-command Bash calls where the write sits past character 200. The truncation and endpoint problems are fixed.

Two things the new text doesn't reach.

The recipe isn't in the channel that runs it. Step 3 delegates log inspection to a cheap subagent, and the prompt that does the delegating starts eight lines below this recipe. That prompt never mentions attribution — it asks "what decision did the bot make that led to this bad outcome?" — and the JSONL parsing queries it hands over still end in .[0:120], which is the truncation this paragraph exists to warn against. The recipe's own path (/tmp/session-logs/<run-id>) is the directory that prompt's gh run download writes to, so the subagent is clearly the intended executor, but nothing puts the query in front of it. An analyst following the section as written escalates, hands over the existing prompt, and gets back a decision trace with 120-character tool inputs — the same evidence that produced the mis-attribution.

The fix is a paragraph in the prompt block, which is outside this diff — happy to push it if you want it here rather than as a follow-up. Something like a fourth bullet after The concerning outcome was::

If the finding will name which run produced an output, say which output, and confirm it from the log before answering: WRITES=... plus the loop above, run over every *.jsonl under the download directory. Report the matching tool call verbatim, not a summary.

Non-POST writes read as silence. --method POST/-X POST covers creation but not mutation. gh api ... -X PATCH is how a run edits a PR title or body (running-in-ci, "Keeping PR Titles and Descriptions Current") — and PR bodies and issue bodies are two of the four surfaces Step 2 scans, so an edited body is exactly the kind of output a finding names. GraphQL mutations are invisible for the same reason: gh api graphql -F query=@/tmp/resolve-thread.graphql carries no method flag and no endpoint path.

This is measurable in the sample: runs 31087588292 and 31087189520 each resolved review threads on a PR and match nothing under the current WRITES. Both read as "posted nothing" — and the concluding sentence now offers the analyst exactly one reason an empty result might be wrong (wrong harness variant), so a write-active run that only mutated lands as confirmed silence. That's the false-negative direction again, one endpoint class over.

Suggestion inline. Verified across the same eight artifacts: it adds the three previously-invisible mutations and zero new matches anywhere else, including zero on the read-only leg. The PATCH/PUT/gh pr edit arms didn't fire in this sample — no run edited a body in the window — so those are preventative rather than demonstrated.

Verification

Artifacts (this repo, current): runs 31086587678, 31087588292, 31086570277, 31086967402, 31087189520, 31087035362, 31086958145, 31087767524.

Match counts, current pattern vs. suggested:

31086587678  old=4   new=4
31087588292  old=0   new=1     <- resolveReviewThread
31086570277  old=5   new=5
31086967402  old=1   new=1
31087189520  old=0   new=2     <- resolveReviewThread x2
31087035362  old=3   new=3
31086958145  old=11  new=11
31087767524  old=0   new=0     <- read-only leg, still clean

The new-only matches, in full:

Bash: {"command":"cd /home/runner/work/tend/tend\ncat > /tmp/resolve-thread.graphql << 'GRAPHQL'\nmutation($threadId: ID!) {\n  resolveReviewThread(input: {threadId: $threadId}) {\n    thread { id isResolved }\n  }\n}\nGRAPHQL\ngh api graphql -F query=@/tmp/resolve-thread.graphql -f threadId=\"PRRT_...

resolveReviewThread matches because the mutation body is heredoc'd into the same Bash call that invokes it — which is how every tend skill writes this, so it's a reliable anchor rather than a coincidence.

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

WRITES matched creation but not mutation, so a run that only edited a body,
updated the evidence gist, or resolved a review thread read as silence. Add
the PATCH/PUT/edit/resolveReviewThread arms.

Step 3 delegates log inspection to a cheap subagent whose prompt never
mentioned attribution and handed over a truncated query, so the recipe never
reached its executor. Point the prompt at it.

@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 WRITES widening does what it claims — I re-checked the shapes it names against the pattern: gh pr edit, gh api ... -X PATCH, and a heredoc'd resolveReviewThread all match, and a plain gh api repos/O/R/pulls/N/comments read still doesn't. That half is settled.

The prompt paragraph doesn't quite land, though. Everything else in that block resolves inside it — the download command is written out in full, <run-id> and <signal from Step 2> are placeholders the analyst substitutes, and "the truncated query below" points at the JSONL parsing fence a few lines down. The new sentence is the only one that points outward: "the WRITES recipe above" lives in the skill file, which is not what the subagent receives. So a subagent handed this prompt is told not to use the truncated query and given nothing to use instead — the same gap one step further along, and the fallback is the truncated query.

Using the block's own <...> substitution convention keeps it to one line and avoids carrying a second copy of the recipe; suggestion inline. Pasting the fenced block in literally would also work, at the cost of two copies to keep in sync.

Comment thread plugins/tend-ci-runner/skills/review-reviewers/SKILL.md Outdated
…n the subagent prompt

The subagent receives only the quoted block, so a pointer to the recipe
'above' resolves to nothing on its side, leaving the truncated query as the
only one it can see. Use the block's own <...> substitution convention.
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>
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.

1 participant