-
Notifications
You must be signed in to change notification settings - Fork 1
review: retire the thumbs sweep's why? follow-up; the sweep is read-only #364
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
0f7733b
65d1734
15ef0a9
649ea85
6f40b6c
f74205e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| --- | ||
| "review": minor | ||
| --- | ||
|
|
||
| Delete the thumbs sweep entirely (`thumbs-sweep.ts`, `thumbs-sweep-github.ts`, `run-thumbs-sweep.ts`, and their tests), along with its `octokit` dependency; the lib scripts consumers run are dependency-free again. The 2026-08-20 version audit measured 2 reason replies across the 31 "why?" follow-ups ever posted, 26 of them landing as bursts on one PR, each follow-up also registered as an implicit empty COMMENTED review event that pollutes run counts and the PR timeline, and nobody consumed the read-side tallies. Nothing depends on the sweep: a bare thumbs-down has adjudicated the finding directly since v1.17.0 (staging reads thread-opener reactions itself, identity-filtered), and feedback reporting is done manually today via the `review-feedback-audit` skill (the reconciler and claim validation still read thread replies on re-review; no automated job turns them into feedback reports, the weekly feedback report is the planned instrument). Consumer repos should delete their `review-feedback.yml` sweep workflow when bumping; the `review-counters.yml` weekly counters workflow is unaffected. Anyone counting review runs should key on the v1.14.0+ version footer rather than review events: the autofix workflow's thread replies still carry the implicit-review shape. | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,6 @@ | ||
| --- | ||
| name: review-feedback-audit | ||
| description: Audit the PR review workflow's posted output and human feedback in a consumer repo over a time window (typically since a reviewer deploy). Computes volume, verdict mix, label mix, verbosity, duplication at three grains, suppression notes, and human feedback (reactions, thumbs-sweep replies, thread replies), then maps each finding to an open Khan/actions PR or a new-PR candidate. Invoke with a consumer repo, a cutoff timestamp, and optionally the bot login. | ||
| description: Audit the PR review workflow's posted output and human feedback in a consumer repo over a time window (typically since a reviewer deploy). Computes volume, verdict mix, label mix, verbosity, duplication at three grains, suppression notes, and human feedback (reactions, thread replies, and, in windows predating the follow-up retirement, thumbs-sweep follow-up replies), then maps each finding to an open Khan/actions PR or a new-PR candidate. Invoke with a consumer repo, a cutoff timestamp, and optionally the bot login. | ||
| --- | ||
|
|
||
| # Review quality and feedback audit | ||
|
|
@@ -49,7 +49,8 @@ jq --arg bot "$BOT" --arg c "$CUTOFF" \ | |
| "$WORK/pc.json" > "$WORK/bot_inline_raw.json" | ||
| ``` | ||
|
|
||
| **Issue comments** (guidance comments, sweep follow-ups, CI noise). The | ||
| **Issue comments** (guidance comments, CI noise, and, in windows predating | ||
| the follow-up retirement, sweep follow-ups). The | ||
| reviewer's guidance comment is identified by the engine-appended | ||
| `gh-aw-agentic-workflow` marker, not by any bot-authored marker: the ingest | ||
| sanitizer strips agent-written HTML comments (see Known constraints). | ||
|
|
@@ -80,13 +81,15 @@ while read -r pr; do | |
| gh api --paginate "repos/$REPO/pulls/$pr/reviews?per_page=100" \ | ||
| --jq "[.[] | select(.user.login==\"$BOT\" | ||
| and .submitted_at >= \"$CUTOFF\")] | ||
| | .[] | [$pr, .id, .state, .submitted_at, (.body|length)] | @tsv" \ | ||
| | .[] | [$pr, .id, .state, .submitted_at, (.body|length), | ||
| (.body|test(\"review-v[0-9]\"))] | @tsv" \ | ||
| < /dev/null >> "$WORK/reviews.tsv" | ||
| done < "$WORK/prs.txt" | ||
| ``` | ||
|
|
||
| Redirecting stdin from `/dev/null` inside the loop matters: `gh` can consume | ||
| the loop's stdin and truncate the PR list. | ||
| the loop's stdin and truncate the PR list. The last column marks rows whose | ||
| body carries the v1.14.0+ version footer; Step 3's run count keys on it. | ||
|
|
||
| **Reactions.** The listing's `reactions` object gives counts; fetch the | ||
| detail endpoint only for comments with `total_count > 0`, and exclude the | ||
|
|
@@ -150,8 +153,10 @@ Every later step reads `bot_replies.tsv`; `replies.tsv` is an intermediate. | |
| **Conventional-Comment labels.** Parse the label prefix off each bot body: | ||
| `issue`, `suggestion`, `question`, `note`, `nitpick`, `thought`, plus | ||
| variants like `(non-blocking, documentation)` and | ||
| `(non-blocking, best-practice)`. Bodies with no label are usually | ||
| thumbs-sweep follow-ups (next paragraph): | ||
| `(non-blocking, best-practice)`. In windows predating the follow-up | ||
| retirement, bodies with no label are usually thumbs-sweep follow-ups (next | ||
| paragraph); in windows after it, an unlabeled bot body is an anomaly worth | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. suggestion (non-blocking): New prose says an unlabeled bot body is "an anomaly worth reading rather than bucketing", but the pipeline below it still buckets it as review detailsfound by correctness-reviewer |
||
| reading rather than bucketing: | ||
|
|
||
| ```sh | ||
| jq '[.[] | {id, pr: (.pull_request_url|split("/")|last|tonumber), path, | ||
|
|
@@ -163,25 +168,43 @@ jq '[.[] | {id, pr: (.pull_request_url|split("/")|last|tonumber), path, | |
| "$WORK/bot_inline_raw.json" > "$WORK/bot_inline.json" | ||
| ``` | ||
|
|
||
| **Thumbs-sweep follow-ups** carry the `review-thumbs-followup` marker (a | ||
| sweep-posted comment survives the sanitizer because the sweep posts through | ||
| the plain API, not through safe outputs). Count them separately from | ||
| findings, and note that a follow-up posted as a review shows up in | ||
| `reviews.tsv` as a `COMMENTED` review; do not count it as a review run. | ||
|
|
||
| **Reason replies** use the closed vocabulary the follow-up offers: | ||
| `incorrect`, `unimportant`, `unclear`, `duplicate`. Match replies to | ||
| follow-ups by thread and record the latency from downvote to follow-up and | ||
| from follow-up to reply. | ||
| **Thumbs-sweep follow-ups (historical only).** The sweep's "why?" follow-up | ||
| was retired and the sweep itself then deleted entirely (see the | ||
| `workflows/review` CHANGELOG entries for both); PRs reviewed after the | ||
| retirement release never carry follow-ups. | ||
| When the audit window predates the retirement, follow-ups carry the | ||
| `review-thumbs-followup` marker (a sweep-posted comment survived the | ||
| sanitizer because the sweep posted through the plain API, not through safe | ||
| outputs). Count them separately from findings, and note that a follow-up | ||
| posted as a review shows up in `reviews.tsv` as a `COMMENTED` review; the | ||
| run-count rule in Step 3 keeps it out of run totals. | ||
|
|
||
| **Reason replies (historical only).** Follow-ups offered a closed | ||
| vocabulary: `incorrect`, `unimportant`, `unclear`, `duplicate`. It survives | ||
| in code only as the permanently-unpopulated `DownvoteReason` type in | ||
| `workflows/review/eval/judge.ts`, kept to type historical labels. For | ||
| pre-retirement windows, match replies to follow-ups by thread and record | ||
| the latency from downvote to follow-up and from follow-up to reply; skip | ||
| both metrics for windows after the retirement (there is nothing to measure). | ||
|
|
||
| **Human replies**: classify each thread's outcome by reading the exchange: | ||
| `accepted` (author changed code or agreed), `declined` (author rejected with | ||
| a reason), `answered` (bot asked, author answered, no change requested). | ||
| Thread replies reach a maintainer only through this skill or manual reading; | ||
| no automated job consumes them (the weekly report is `counters-report.ts` | ||
| and has no reply handling), so treat an unaddressed reply as unseen, not | ||
| triaged. | ||
|
|
||
| ## Step 3: compute the metrics | ||
|
|
||
| - **Runs and verdicts**: rows of `reviews.tsv` minus sweep follow-ups; | ||
| verdict mix (`APPROVED` / `CHANGES_REQUESTED` / `COMMENTED`). | ||
| - **Runs and verdicts**: count review runs by the v1.14.0+ version footer | ||
| (the `review-v<version>` segment in the review body's collapsed | ||
| `<details>` block), not by review events: the autofix workflow's thread | ||
| replies arrive as implicit empty `COMMENTED` review events (as did sweep | ||
| follow-ups, pre-retirement), so a raw `reviews.tsv` row count overstates | ||
| runs. Verdict mix (`APPROVED` / `CHANGES_REQUESTED` / `COMMENTED`) over | ||
| the footer-bearing rows. For windows predating v1.14.0 no footer exists; | ||
| fall back to review events minus sweep follow-ups and say so in Caveats. | ||
| - **Volume**: bot inline comments per PR and per run; guidance comments. | ||
| - **Label mix**: count per label; blocking vs non-blocking split. | ||
| - **Verbosity**: mean / median / p90 / max body chars over top-level bot | ||
|
|
@@ -210,8 +233,9 @@ jq '[.[] | select(.body | |
| - **Suppression notes**, parsed from review bodies: `not re-posted (already | ||
| tracked)`, `shed under the ... run budget`, and the `N of M prior review | ||
| threads` accountability lines. These show which mitigations fired. | ||
| - **Feedback**: reactions by kind and reactor; sweep follow-up latency; | ||
| reason-reply latency; human replies by outcome class. | ||
| - **Feedback**: reactions by kind and reactor; human replies by outcome | ||
| class; for pre-retirement windows only, sweep follow-up latency and | ||
| reason-reply latency. | ||
| - **Attribution**: check each posted review body and guidance comment for a | ||
| version marker or footer, and report presence per body. | ||
|
|
||
|
|
@@ -241,13 +265,16 @@ jq '[.[] | select(.body | |
| a lower bound. | ||
| - **Sentinel strings live in lib code.** The markers this audit greps for | ||
| are defined in Khan/actions source, and a zero count is indistinguishable | ||
| from a renamed marker: `review-thumbs-followup` and the reason vocabulary | ||
| in `workflows/review/lib/thumbs-sweep.ts`, "A sketch, not a committable | ||
| replacement" in `workflows/review/lib/submission.ts`, the suppression | ||
| note phrasing in `workflows/review/lib/dispatch.ts`, and the | ||
| `gh-aw-agentic-workflow` marker appended by the gh-aw engine. Before | ||
| trusting any zero measurement, re-derive the string from the checkout | ||
| being audited. | ||
| from a renamed marker: "A sketch, not a committable replacement" in | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. note (non-blocking): The "GraphQL may be unavailable" bullet (line 254) still says the thumbs sweep "normally reports" thread-resolution counts, contradicting the newly added lines 193-196. Introduced by this change: the PR deletes the sweep and qualifies every other mention in this skill as historical, but this one bullet keeps the present tense, and it now contradicts the lines this same diff added at 193-196 ("no automated job consumes them"). I grepped Lower-confidence observations (1)
review detailsfound by correctness-reviewer |
||
| `workflows/review/lib/submission.ts`, the suppression note phrasing in | ||
| `workflows/review/lib/dispatch.ts`, and the `gh-aw-agentic-workflow` | ||
| marker appended by the gh-aw engine. Before trusting any zero | ||
| measurement, re-derive the string from the checkout being audited. The | ||
| `review-thumbs-followup` marker and the downvote-reason vocabulary were | ||
| deleted from the lib with the follow-up retirement; when auditing a | ||
| pre-retirement window, re-derive them from the `workflows/review` | ||
| CHANGELOG entry for the retirement or from a pre-retirement tag's | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. suggestion (non-blocking, documentation): Recovery instruction points at a CHANGELOG entry that will not contain the deleted sentinel strings. The added text says to "re-derive them from the review detailsfound by documentation |
||
| `lib/thumbs-sweep.ts`. | ||
|
|
||
| ## Step 4: report | ||
|
|
||
|
|
@@ -260,7 +287,8 @@ Use these sections, in order: | |
| 4. **Verbosity**: the stats above, sketch-block share, review-body and | ||
| guidance-comment sizes. | ||
| 5. **Duplication**: one subsection per grain, with ids. | ||
| 6. **Human feedback**: reactions, sweep loop latencies, replies by outcome. | ||
| 6. **Human feedback**: reactions, replies by outcome, and (pre-retirement | ||
| windows only) sweep follow-up loop latencies. | ||
| 7. **Caveats**: GraphQL availability, marker expectations, sample-size | ||
| limits, anything unverifiable. | ||
|
|
||
|
|
||
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -149,14 +149,15 @@ you can pick the one that says what you mean: | |
| do not join the corpus; a fixed defect that reappears is a fresh finding. | ||
| - **👎 the finding's comment.** Same adjudication as resolving, through the | ||
| reaction channel: a 👎 on a thread's OPENING comment puts its defect in the | ||
| adjudicated corpus whether or not you also resolve. The feedback sweep may | ||
| additionally ask one follow-up ("why?"), which calibrates the eval suite; | ||
| answering it is welcome but the 👎 alone is what suppresses. Reactions on | ||
| replies are conversation, not adjudication. 👎 is the ONLY adjudicating | ||
| reaction: a 😕 triggers the sweep's follow-up question like a 👎 does, but | ||
| it does not suppress (😕 reads as "unclear", not "wrong", and ambiguity is | ||
| worth a question, not a standing suppression). The bot's own seeded nudge | ||
| reactions never count as adjudication either. | ||
| adjudicated corpus whether or not you also resolve. The 👎 alone is what | ||
| suppresses; if you want to say why, reply in the thread: the reconciler and | ||
| claim validation read replies as described above, nothing prompts you for a | ||
| reason any more, and replies only reach a feedback report when a maintainer | ||
| runs the `review-feedback-audit` skill. Reactions on replies are | ||
| conversation, not adjudication. 👎 is the ONLY adjudicating reaction: a 😕 | ||
| does not suppress (😕 reads as "unclear", not "wrong", and ambiguity is | ||
| worth a conversation, not a standing suppression). The bot's own seeded | ||
| nudge reactions never count as adjudication either. | ||
| - **Hide the comment.** Reads as nothing. The reviewer does not see hidden | ||
| state; resolve or 👎 instead. | ||
|
|
||
|
|
@@ -709,42 +710,28 @@ analysis, and a refused security lens would be a silent coverage hole. Any | |
| further per-role promotion (or Sonnet step-down) earns its line through its | ||
| own eval-suite arm. | ||
|
|
||
| ### Feedback signal: thumbs sweep and live counters | ||
|
|
||
| Two small scheduled workflows in each consumer repo turn on the tuning loop's | ||
| production signal. Both are plain GitHub Actions YAML (not gh-aw), both check | ||
| out this repo at the pinned `review-v*` tag and run lib scripts with | ||
| `npx -y tsx`, and neither touches review semantics: | ||
|
|
||
| - **Thumbs sweep** (`lib/run-thumbs-sweep.ts`, every 1-2 hours): collects | ||
| reactions on the reviewer's comments at both grains (inline review comments, | ||
| identified by the code-owned Conventional-Comment label prefixes; the | ||
| risks/patterns summary comment, identified by its hidden marker) and posts | ||
| exactly one "why?" follow-up per newly-downvoted comment, offering the closed | ||
| reason vocabulary (`incorrect` / `unimportant` / `unclear` / `duplicate`). | ||
| Reactions are tallied with the same sets gh-aw's outcome evaluation uses | ||
| (👍/❤️/🎉/🚀 positive, 👎/😕 negative; a 😕 triggers the follow-up like a 👎), | ||
| and resolved inline threads are counted as their own positive column: threads | ||
| also get resolved just to clear noise, so resolution is reported alongside | ||
| the reaction tallies rather than folded into them. Idempotent across restarts | ||
| via the hidden follow-up markers; bounded to PRs updated in the last 14 days | ||
| (`REVIEW_SWEEP_LOOKBACK_DAYS`), skipping PRs closed or merged more than 3 | ||
| days ago (`REVIEW_SWEEP_CLOSED_GRACE_DAYS`; feedback lands around merge time, | ||
| after which a landed PR stops changing). Needs only `pull-requests: write`. | ||
| The sweep run needs `npm ci --omit=dev` in the checked-out | ||
| `workflows/review/` first (the sweep's `octokit` dependency is pinned exactly | ||
| in `package.json`, with the transitive tree locked by the committed | ||
| `package-lock.json`); the other lib scripts remain dependency-free. Each run's | ||
| `SweepResult` and API-request count land in the job summary. | ||
| ### Feedback signal: live counters | ||
|
|
||
| One small scheduled workflow in each consumer repo turns on the tuning loop's | ||
| production signal. It is plain GitHub Actions YAML (not gh-aw), checks out | ||
| this repo at the pinned `review-v*` tag, runs a lib script with `npx -y tsx`, | ||
| and never touches review semantics: | ||
|
|
||
| - **Live counters** (`lib/counters-report.ts`, weekly): the workflow downloads | ||
| the review runs' per-run artifacts (bounded window), and the script | ||
| aggregates them with `lib/counters.ts` into the job summary — verdict mix, | ||
| comments/run, validator drop rate, cost/run. Needs only `actions: read`. | ||
|
|
||
| The reviewer posts as `github-actions[bot]` (gh-aw safe outputs use the | ||
| workflow's own token), so that login is both the sweep's `botLogin` filter and | ||
| the author of its follow-ups; every count in the sweep excludes that login's | ||
| own reactions, so the seeded nudge pair (below) is never live signal. | ||
| comments/run, validator drop rate, cost/run. Needs only `actions: read`, | ||
| and no `npm ci`: the lib scripts consumers run are dependency-free. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. suggestion (non-blocking): The bullet's own claim is right — A sketch, not a committable replacement: review detailsfound by holistic |
||
|
|
||
| There used to be a second workflow here, the thumbs sweep (a 2-hourly poll | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. question (non-blocking): The rewritten "Feedback signal: live counters" section tells the reader the sweep is gone but never tells a consuming repo to delete its own review detailsfound by skill-auditor (out-of-lane) | also flagged by conventions (at line 713) |
||
| that tallied reactions on the reviewer's comments and posted a "why?" | ||
| follow-up per newly-downvoted comment). Both halves are retired: the | ||
| 2026-08-20 audit measured 2 reason replies across the 31 follow-ups ever | ||
| posted, each follow-up also registered as an implicit empty review event, and | ||
| nobody consumed the read-side tallies. A bare 👎 adjudicates directly since | ||
| v1.17.0 (the staging reads thread-opener reactions itself, excluding the | ||
| bot's own seeded nudges), so no scheduled collector is needed for feedback to | ||
| act on the reviewer. | ||
|
|
||
| ### Relationship to the gh-aw outcome-collector | ||
|
|
||
|
|
@@ -753,29 +740,28 @@ classifies every agentic safe output as accepted / rejected / ignored / | |
| pending and exports the results to Sentry over OTLP. The two systems answer | ||
| different questions and neither replaces the other: | ||
|
|
||
| - **Outcome-collector**: passive fleet-wide acceptance telemetry. It never | ||
| writes to GitHub, so it can observe engagement but cannot ask *why* a | ||
| comment was downvoted. Its data lives in Sentry. | ||
| - **Thumbs sweep**: active reason elicitation for the reviewer's tuning loop. | ||
| Its "why?" follow-ups produce the closed reason labels that calibrate the | ||
| eval-suite judge and feed dismissal learning. Its data lives in each run's | ||
| job summary and stdout JSON (not exported to OTel today). | ||
| - **Outcome-collector**: passive fleet-wide acceptance telemetry across every | ||
| agentic workflow. Its data lives in Sentry. It counts any reaction with no | ||
| reactor identity, so it cannot exclude the seeded nudges. | ||
| - **Reviewer-side signal**: the adjudication path reads thread-opener | ||
| reactions identity-filtered at review time, and the live-counters report | ||
| aggregates the per-run artifacts. Neither is exported to OTel today. | ||
|
|
||
| Two known interactions: | ||
|
|
||
| - **Nudge seeding** is planned as a post-time step in the consumer repos' | ||
| review workflow (a custom safe-output job that reacts 👍/👎 to each posted | ||
| comment seconds after posting), not in the sweep: gh-aw cannot react to its | ||
| own safe outputs natively, and comments posted via `GITHUB_TOKEN` emit no | ||
| workflow events, so post-time is the only immediate option. | ||
| comment seconds after posting): gh-aw cannot react to its own safe outputs | ||
| natively, and comments posted via `GITHUB_TOKEN` emit no workflow events, | ||
| so post-time is the only immediate option. | ||
| - Once seeding is live, the outcome-collector's `add_comment` metric for the | ||
| review workflow is **inflated by design**: its evaluator counts any reaction | ||
| as acceptance with no reactor identity, so every seeded summary comment | ||
| reads as `accepted`. The inflation is bounded to that one metric (inline | ||
| comments and submitted reviews are evaluated by other means), and the sweep's | ||
| identity-filtered tallies are the authoritative reviewer-comment engagement | ||
| numbers. An upstream gh-aw change to identity-aware reaction counting would | ||
| retire this caveat. | ||
| comments and submitted reviews are evaluated by other means); the | ||
| adjudication path's identity-filtered reads are the authoritative | ||
| reviewer-comment engagement signal. An upstream gh-aw change to | ||
| identity-aware reaction counting would retire this caveat. | ||
|
|
||
| ### Required secrets / variables | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion (non-blocking, best-practice): Versioning policy: semver as the behavior contract for a feature removal.
workflows/review/README.md(the review-workflow-contract skill file) states under Version attribution: "Semver is the behavior contract: a release that changes the reviewer's behavior bumps the major version, so a consumer pinned toreview-v<major>can assume the fundamental behavior holds within a major." This changeset declares"review": minorfor a release that deletes a documented consumer-facing surface: the README section it also edits used to say the sweep and counters workflows "check out this repo at the pinnedreview-v*tag and run lib scripts withnpx -y tsx", and this PR removeslib/run-thumbs-sweep.tsoutright plus the 👎 follow-up behavior documented in What your feedback does (the diff replaces "The feedback sweep may additionally ask one follow-up ("why?")" with "The 👎 alone is what suppresses"). I checked the checkout before reporting: no in-repo module still imports the deleted files (workflows/review/eval/judge.tsandeval/metrics.tsonly mention thumbs in comments;review.mdhas no reference at all), so the break is consumer-side only, andworkflows/review/CHANGELOG.mdshows the project has shipped behavior changes as minors before (v1.17.0's adjudication change) — which is why this is a suggestion rather than a blocker. Consider amajorbump, or state explicitly in the changeset why a removal insidereview-v1is not a contract break.A sketch, not a committable replacement:
review details
found by skill-auditor | also flagged by holistic