review: turn on the review-feedback signal (thumbs-sweep GitHub port + live-counters report)#225
review: turn on the review-feedback signal (thumbs-sweep GitHub port + live-counters report)#225jwbron wants to merge 9 commits into
Conversation
The production wiring for the review-feedback signal (design doc, 'The
tuning loop' -> 'Signal to turn on'):
- thumbs-sweep-github.ts: the octokit-backed ThumbsSweepPort. Reviewer
comments are identified per grain (risks/patterns marker for summary
comments; the code-owned Conventional-Comment label prefixes for
inline ones, which carry no hidden marker at current releases), with
per-reactor logins so seeded reactions never count as signal.
Traversal is bounded to PRs updated in the last 14 days (capped) and
every API request is counted for the auditable job summary.
- run-thumbs-sweep.ts: env-configured CLI for the consumer repos'
scheduled sweep workflows; writes the SweepResult JSON to stdout and
a Markdown digest to GITHUB_STEP_SUMMARY.
- counters-report.ts: aggregates downloaded per-run artifacts into the
weekly live-counters job summary, synthesizing the run summary from
gh-aw engine artifacts (safeoutputs.jsonl, safe-output-items.jsonl,
agent_usage.json) when summary.json is absent, tolerating the known
staging-path nesting.
- counters.ts: accept the production validator artifact shape
({"claims": [...]} with three-state verification) and an optional
fallback source for unjoinable decisions.
- The review package gains its first pinned runtime dependency
(octokit); consumer sweep workflows npm-install it in the checkout.
No review semantics change.
Verified against live data on both consumer repos (dry-run): production summary comments carry gh-aw's engine-emitted workflow-call-id marker but not the pr-reviewer risks/patterns marker, so the port accepts either (workflow ids configurable via REVIEW_SWEEP_WORKFLOW_IDS, default 'review'). REVIEW_SWEEP_DRY_RUN=true traverses and decides without posting or seeding, for first-run audits. octokit is ESM-only while tsx runs these scripts as CJS, so the CLI loads it with a dynamic import.
🦋 Changeset detectedLatest commit: 8f1c339 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
khan-actions-bot
requested review from
a team,
kevinb-khan and
somewhatabstract
and removed request for
a team
jeresig
left a comment
jeresig
left a comment
There was a problem hiding this comment.
I think this is running counter to what we already have going with the gh-outcome-collector logic. I think we want to be using that instead of this. Or, this needs much better documentation as right now it's really not clear how this is being run and when and how the data is being used (or not). Like, is this being integrated into our existing OTel data?
If we are using this we need to integrate the other ways that we're determining success (such as a resolved thread = success, and also other reactions equaling success/rejection). I raised this in your other PRs already.
jeresig
left a comment
jeresig
left a comment
There was a problem hiding this comment.
Ok, now that I see how this is being used, a number of changes requested:
- Resolved threads should be counted as success.
- Closed or merged PRs should only be analyzed once (they aren't going to be changing once they've landed)
- We should take into account other affirmative/negative reactions (I raised this in another PR)
Question on leaving the default thumbs-up/down on the review comment - are you doing that here as it's not possible to do in the gh-aw safe outputs?
|
Thanks, responding to both reviews together. Outcome-collector overlap: agreed it needs explicit positioning. The sweep isn't replaceable by the collector: the collector never writes to GitHub, so it can't ask why a comment was downvoted, and those reason labels (incorrect / unimportant / unclear / duplicate) are what calibrate the eval judge. I'll add a README section spelling out the division: collector = passive acceptance telemetry to Sentry; sweep = active reason elicitation, results in the job summary + stdout JSON (not otel today; I can export the tallies over the same OTLP endpoint review.md uses if you want that). Resolved threads = success: agreed. Matches the gh-aw spec; notably the JS evaluator we run doesn't implement it yet either (only the Go CLI has the dedicated evaluator). I'll add a GraphQL reviewThreads pass and report resolutions as their own positive column rather than folding them into 👍, since threads also get resolved just to clear noise. Closed/merged analyzed once: partly agreed. The sweep is deliberately stateless, and feedback often lands right around merge, so I'd rather skip PRs closed more than ~3 days ago than track analyzed-once state. Same effect at current scale (~31 API requests/run) without a state store. Other reactions: agreed, half done. The core sets already match the collector's (👍 ❤️ 🎉 🚀 / 👎 😕) and the follow-up already triggers on 😕; the live tally only counts literal +1/-1, which I'll fix to use the shared sets. Seeding question: close. gh-aw can't do it natively (the comment IDs don't exist until the handlers post them, and GITHUB_TOKEN-posted comments don't emit events, so an event-triggered seeder never fires). But a custom safe-output job (safe-outputs.jobs with needs: safe_outputs) can react seconds after posting by reading the handler's safe-output-items record. So I'll drop REVIEW_SWEEP_SEED_REACTIONS from this PR and do post-time seeding in the consumer review.md instead. One interaction to flag: seeded pairs make the collector read every review summary comment as accepted (its add_comment evaluator counts any reaction, identity-blind). The damage is bounded to that one metric (inline comments use an existence fallback in JS; submit_pull_request_review is scored by commits/merge state), and the sweep measures reviewer-comment engagement strictly better (identity-filtered, both grains, reason labels). Plan: seed anyway, document that the collector's add_comment result for the review workflow is inflated by design with the sweep authoritative there, and file an upstream gh-aw issue for identity-aware reaction counting (their spec already prefers bot-identity provenance). Shout if you'd rather hold seeding until upstream fixes it. So for this PR: drop sweep-time seeding, count resolved threads, widen the reaction tally, closed-PR grace window, README section on the two systems. Follow-ups: post-time seeding in the consumer repos, upstream gh-aw issue. Sound right? |
…eding, count resolved threads, widen reaction tally, add closed-PR grace window
|
Changes from the review are in (7c601d3): sweep-time seeding removed (post-time seeding follows in the consumer repos), resolved inline threads counted via GraphQL as their own positive column, the live tally widened to the shared reaction sets (👍/❤️/🎉/🚀 vs 👎/😕), closed/merged PRs age out after a 3-day grace window, and the README now has a section positioning the sweep against the outcome-collector (including the add_comment inflation caveat). PR description updated to match. 298 lib tests pass, typecheck and lint clean. |
jeresig
left a comment
jeresig
left a comment
There was a problem hiding this comment.
Great - thanks for the fixes!
Review Guidancegithub-actions (7 files)
Common patterns3 files: 2 files: Removal of the Excluded from review (3 files)Not individually reviewed — generated, formatting-only, or fully explained by a common pattern above:
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| }; | ||
|
|
||
| /** Render the auditable Markdown digest for the job summary. */ | ||
| export const renderSweepSummary = ( |
There was a problem hiding this comment.
suggestion (non-blocking): renderSweepSummary is the only export here and the comment at line 186 says "tests import renderSweepSummary", but nothing does — there is no run-thumbs-sweep.test.ts and no .test.ts references it. Its branches (dry-run vs normal header, the byReason already-followed-up count, the conditional downvoted table) are unexercised, while the sibling new files each have dedicated tests. A small unit test would guard the sweep's only human-auditable output against a rendering regression.
Low-confidence note (1) — surfaced for completeness, not blocking
workflows/review/lib/thumbs-sweep-github.ts:91— Is the call-id-marker premise durable if the review workflow ever posts a secondadd_comment? Summary-grain matching accepts any bot comment carrying thegh-aw-workflow-call-idmarker, and the "only one add_comment output" premise lives in consumer workflow config, not this code. A future consumer enabling status comments or anotheradd_commentwould silently widen the sweep's summary grain.
| "version": "1.4.1" | ||
| "version": "1.4.1", | ||
| "dependencies": { | ||
| "octokit": "5.0.5" |
There was a problem hiding this comment.
note (non-blocking): The exact octokit: 5.0.5 pin only fixes the top-level package — workflows/review ships no lockfile, so a consumer npm install --omit=dev resolves octokit's transitive tree at whatever versions are current that day. Committing a lockfile (or depending on @octokit/core + the throttling plugin, since the port uses only octokit.request) would make the pin mean what the module docstring claims.
| const closedAt = pull["closed_at"]; | ||
| if ( | ||
| pull["state"] === "closed" && | ||
| typeof closedAt === "string" && |
There was a problem hiding this comment.
note (non-blocking): The closed-PR age-out only fires when closed_at is a string; a closed PR whose payload lacks closed_at falls through and is swept every run while inside the lookback window. GitHub always populates closed_at for closed PRs, so this is defensive-only — at worst a few extra API calls, with no correctness or idempotency impact.
| } | ||
| const event = entry["event"]; | ||
| if (event === "APPROVE" || event === "REQUEST_CHANGES") { | ||
| summary["verdict"] = event; |
There was a problem hiding this comment.
thought (non-blocking): This loop assigns summary["verdict"] on each matching submit_pull_request_review with no precedence, so a run emitting both an APPROVE and a REQUEST_CHANGES event would record last-wins. The reviewer emits at most one submission (max: 1), so this is a metrics-only edge case rather than a live defect; preferring REQUEST_CHANGES on conflict would be a cheap defensive guard against a malformed multi-submission artifact.
| for (const pullNumber of pullNumbers) { | ||
| this.pullsScanned += 1; | ||
|
|
||
| const inline = await this.paginate( |
There was a problem hiding this comment.
suggestion (non-blocking): The per-PR traversal is ~1 + 2·N requests (with the 200-PR cap, 400+ per sweep) against the shared per-repo GITHUB_TOKEN budget. GitHub's repo-wide GET /repos/{owner}/{repo}/pulls/comments and .../issues/comments (both support since/sort) list comments in ~(comments/100) requests, and each comment carries pull_request_url/issue_url so the comment→PR map survives; closed-grace filtering would then only need a lazy per-PR fetch for PRs that actually have candidate comments. Not required, but it would cut the sweep's API footprint substantially on busy repos.
2 participants
Was stacked on #223 (thumbs-sweep seeding + self-reaction filtering); #223 has landed and main is merged in.
Implements the Khan/actions side of the design doc's "The tuning loop" -> "Signal to turn on": the production GitHub wiring for the thumbs sweep and the live counters. Consumer-repo PRs (webapp/frontend scheduled workflows, frontend ROUTING) follow separately and pin the release this ships in.
Thumbs sweep
lib/thumbs-sweep-github.ts— the octokit-backedThumbsSweepPort. Reviewer comments are identified per grain by the workflow's markers, not by author alone (github-actions[bot]authors many workflows' comments):pr-reviewer:risks-and-patternsmarker or gh-aw's engine-emittedgh-aw-workflow-call-id: <owner>/<repo>/reviewmarker. Verified against live comments: production summary comments carry only the latter today.render-comment.ts's taxonomy). Inline comments carry no hidden marker at current releases; the label grammar is code-owned and templated, so the match is exact.reviewThreadsquery per PR that has reviewer inline comments) and reported as their own positive column; resolution never triggers a follow-up.REVIEW_SWEEP_CLOSED_GRACE_DAYS; feedback lands around merge time, after which a landed PR stops changing). Reaction detail is fetched only when a comment has any reactions. Every API request is counted and reported.lib/run-thumbs-sweep.ts— env-configured CLI for the consumer workflows (GITHUB_TOKEN+pull-requests: writeis all it needs). Writes theSweepResultJSON to stdout and an auditable Markdown digest to the job summary.REVIEW_SWEEP_DRY_RUN=truetraverses and decides without posting, for first-run audits.reviewpackage gains its first pinned runtime dependency,octokit(throttling/retry built in). Consumer sweep workflows runnpm install --omit=devin the checked-outworkflows/review/; all other lib scripts remain dependency-free. Loaded via dynamicimport()because the package is ESM-only and tsx runs the lib as CJS.Live counters
lib/counters-report.ts— aggregates a directory of downloaded per-run artifacts into the weekly counters job summary viacounters.ts. Wheresummary.jsondoesn't exist (all current production runs), it synthesizes the run summary from gh-aw engine artifacts every run already uploads:safeoutputs.jsonl(verdict event),safe-output-items.jsonl(comments actually posted),agent_usage.json(cost/tokens). Artifact files are located by basename at any depth, tolerating the known gh-aw staging-path nesting.lib/counters.ts— normalization extended to the artifact shapes production actually emits: validator files keyed{"claims": [...]}with three-stateverification(confirmed/plausiblefold to keep,refutedto drop), and an opt-in fallback source ((unknown)) for decisions that can't be joined toclaims.json(which production runs don't upload). Default join behavior is unchanged.Changes from review feedback
seedReactions, the port'saddReactions,REVIEW_SWEEP_SEED_REACTIONS). The nudge pair will instead be seeded at post time by a custom safe-output job in the consumer repos' review workflow (follow-up PR); the sweep's self-reaction filtering stays, keeping those seeds out of all signal.add_commentinflation once post-time seeding exists.Verification
No review semantics change anywhere; the reviewer's prompt and workflow are untouched.
cc @jeresig
Human steps (non-review)
out/upload fix (review: fix the out/ artifact upload (allowed-paths must match staging-relative paths) #243) reaching a release before they see claim data.