review: turn on the review-feedback signal (thumbs-sweep GitHub port + live-counters report)#225
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: cd6e961 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!
This comment has been minimized.
This comment has been minimized.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| 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.
| 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.
…nto review-feedback-signal # Conflicts: # pnpm-lock.yaml
…ack (lockfile, summary test, verdict guard) - Commit workflows/review/package-lock.json so the exact octokit pin also locks the transitive tree; consumer sweep workflows install with npm ci --omit=dev (docstring + README updated). - Add run-thumbs-sweep.test.ts covering renderSweepSummary (normal vs dry-run header, already-followed-up count, conditional downvoted table), making the run-on-import guard comment true. - Prefer REQUEST_CHANGES when a malformed artifact carries multiple review submissions in counters-report's synthesized verdict.
Review live A/BNo reviewable delta: review.md is byte-identical in both arms (baseline |
|
Merged main (pnpm-lock regenerated;
Skipped two:
750 lib tests pass, typecheck and lint clean. |
This comment has been minimized.
This comment has been minimized.
| } | ||
| const value = Number.parseInt(raw, 10); | ||
| if (!Number.isInteger(value) || value <= 0) { | ||
| throw new Error(`${name} must be a positive integer, got: ${raw}`); |
There was a problem hiding this comment.
note (non-blocking): intEnv's positive-integer validation has no test coverage — run-thumbs-sweep.test.ts imports only renderSweepSummary. A regression that let a bad REVIEW_SWEEP_* value (0 or non-numeric) coerce past this guard would ship an unbounded or empty sweep window unnoticed. Exporting and unit-testing intEnv would cheaply lock in the fail-loud path this code intends.
Low-confidence note (1) — surfaced for completeness, not blocking
workflows/review/lib/counters-report.ts:139— Add a source pointer for the1 ai_credit = $0.01conversion. It is corroborated in-repo (review.md:231-235documents gh-aw's defaults as 1000/run $10, 5000/day $50, i.e. $0.01/credit), but that is inferred from the cap config rather than a direct gh-aw source that defines the unit; a one-line pointer to gh-aw'sagent_usage.jsondefinition would pin it.
…nto review-feedback-signal # Conflicts: # workflows/review/package.json
Review Guidancegithub-actions (7 files)
Common patterns2 files: Removed reaction-seeding ( 2 files: Added Excluded from review (4 files)Not individually reviewed — generated, formatting-only, or fully explained by a common pattern above:
|
| "version": "1.5.0" | ||
| "version": "1.5.0", | ||
| "dependencies": { | ||
| "octokit": "5.0.5" |
There was a problem hiding this comment.
suggestion (non-blocking): The port consumes only octokit.request and hand-rolls pagination, so the octokit meta-package pulls in @octokit/app, @octokit/oauth-app, @octokit/webhooks, and @types/aws-lambda that consumer sweeps npm ci and audit but never execute (all four are in the committed lockfile). @octokit/core + @octokit/plugin-retry + @octokit/plugin-throttling gives the same throttling/retry behavior with roughly half the transitive tree. Non-blocking.
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), with the transitive tree locked by a committedpackage-lock.json. Consumer sweep workflows runnpm ci --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
renderSweepSummaryunit test), typecheck and lint clean.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.