diff --git a/.changeset/review-feedback-audit-skill.md b/.changeset/review-feedback-audit-skill.md new file mode 100644 index 00000000..edbf32b7 --- /dev/null +++ b/.changeset/review-feedback-audit-skill.md @@ -0,0 +1,5 @@ +--- +"review": patch +--- + +Skill-doc only: add a `review-feedback-audit` skill that codifies the post-deploy audit of the reviewer's output and human feedback in a consumer repo, first executed by hand against Khan/webapp for the 2026-08-11/12 window. Inputs are the consumer repo, a cutoff timestamp (typically the deploy time), and the bot login (default `github-actions[bot]`). Collection is GET-only and dependency-free (`bash`, `jq`, `gh`): inline comments and issue comments via the repo-wide `since` listings (re-filtered on `created_at`, since `since` filters on `updated_at`), reviews via a PR search plus per-PR listing (no repo-wide reviews endpoint exists), reaction detail fetched only where the listing shows a nonzero count and always excluding the bot's own reactions, and human replies resolved through `in_reply_to_id` including parents older than the window. The metrics section covers verdict mix (with sweep follow-ups posted as COMMENTED reviews excluded from the run count), label mix off the Conventional-Comment prefixes, verbosity (mean/median/p90/max plus sketch-block share), duplication at three grains (byte-identical in-run bodies, same-run same-root-cause clusters, cross-run families that defect-identity suppression misses when variants span files), suppression notes parsed from review bodies, and the feedback loop (reactions, thumbs-sweep follow-up and reason-reply latencies, replies classified accepted/declined/answered). Two constraints are documented so the audit does not misread them: agent-written HTML comments are sanitizer-stripped, so `pr-reviewer:` marker absence is expected until a visible footer ships, and a GET-only gh broker blocks GraphQL, in which case thread-resolution counts are reported as unavailable rather than approximated. The report template ends by mapping every finding to an open Khan/actions PR or a new-PR candidate. No change to the shipped review workflow. diff --git a/.claude/skills/review-feedback-audit/SKILL.md b/.claude/skills/review-feedback-audit/SKILL.md new file mode 100644 index 00000000..987508a1 --- /dev/null +++ b/.claude/skills/review-feedback-audit/SKILL.md @@ -0,0 +1,282 @@ +--- +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. +--- + +# Review quality and feedback audit + +Measure what the reviewer actually posted in a consumer repo and how humans +responded, over a bounded window. The typical trigger is a reviewer deploy: +set the cutoff to the deploy time and audit everything since. The output is +a report plus a list of improvement candidates, each mapped to an existing +Khan/actions PR or flagged as new work. + +Collection and analysis (Steps 1-4) are read-only against GitHub (GET +requests only) and dependency-free: `bash`, `jq`, `gh`. Step 5 is a +separate write phase (posting PR comments) that needs access beyond a +GET-only broker. The skill was first executed by hand against Khan/webapp +for the 2026-08-11/12 window; the commands below encode that procedure. + +## Required inputs + +1. **Consumer repo**, e.g. `Khan/webapp`. +2. **Cutoff timestamp** in UTC ISO-8601, e.g. `2026-08-11T16:00:00Z`. + Convert from local deploy time first (`date -d "" -u + +%Y-%m-%dT%H:%M:%SZ`). +3. **Bot login** (optional): the account the reviewer posts as, default + `github-actions[bot]`. Must match the repo's `REVIEW_BOT_LOGIN` if set. + +```sh +REPO=Khan/webapp +CUTOFF=2026-08-11T16:00:00Z +BOT='github-actions[bot]' +WORK=$(mktemp -d) +``` + +## Step 1: collect + +All list endpoints filter `since` on `updated_at`, not `created_at`, so +every collection re-filters on `created_at >= $CUTOFF` (or `submitted_at` +for reviews) after fetching. + +**Inline review comments** (the bot's findings plus everyone's replies): + +```sh +gh api --paginate "repos/$REPO/pulls/comments?since=$CUTOFF&per_page=100" \ + | jq -s '[.[][]]' > "$WORK/pc.json" +jq --arg bot "$BOT" --arg c "$CUTOFF" \ + '[.[] | select(.user.login==$bot and .created_at >= $c)]' \ + "$WORK/pc.json" > "$WORK/bot_inline_raw.json" +``` + +**Issue comments** (guidance comments, sweep follow-ups, CI noise). 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). + +```sh +gh api --paginate "repos/$REPO/issues/comments?since=$CUTOFF&per_page=100" \ + | jq -s '[.[][]]' > "$WORK/ic.json" +jq --arg bot "$BOT" --arg c "$CUTOFF" \ + '[.[] | select(.user.login==$bot and .created_at >= $c + and (.body | contains("gh-aw-agentic-workflow")))]' \ + "$WORK/ic.json" > "$WORK/guidance.json" +``` + +**Reviews.** There is no repo-wide `since` endpoint for reviews: find the +PRs updated in the window via search, then list each PR's reviews and filter. + +```sh +gh api --paginate \ + "search/issues?q=repo:$REPO+is:pr+updated:%3E%3D$CUTOFF&per_page=100" \ + --jq '.items[].number' > "$WORK/prs.txt" +# Search caps at 1,000 results regardless of pagination; detect truncation. +total=$(gh api "search/issues?q=repo:$REPO+is:pr+updated:%3E%3D$CUTOFF&per_page=1" \ + --jq .total_count) +[ "$total" -le 1000 ] || + echo "WARNING: $total PRs match but Search caps at 1000; split the window" >&2 +> "$WORK/reviews.tsv" +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" \ + < /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. + +**Reactions.** The listing's `reactions` object gives counts; fetch the +detail endpoint only for comments with `total_count > 0`, and exclude the +bot's own reactions. Nudge seeding (the bot reacting to its own comments) +is a planned feature; do not assume its status, and keep the filter either +way (it is correct before and after): + +```sh +jq -r '.[] | select(.reactions.total_count > 0) | .id' \ + "$WORK/bot_inline_raw.json" | while read -r id; do + gh api "repos/$REPO/pulls/comments/$id/reactions" \ + --jq "[.[] | select(.user.login != \"$BOT\") + | [$id, .user.login, .content] | @tsv] | .[]" < /dev/null +done > "$WORK/reactions.tsv" +``` + +Guidance comments are issue comments, so their reactions live on a +different endpoint; collect them too or the reaction tally silently covers +inline findings only: + +```sh +jq -r '.[] | select(.reactions.total_count > 0) | .id' \ + "$WORK/guidance.json" | while read -r id; do + gh api "repos/$REPO/issues/comments/$id/reactions" \ + --jq "[.[] | select(.user.login != \"$BOT\") + | [$id, .user.login, .content] | @tsv] | .[]" < /dev/null +done >> "$WORK/reactions.tsv" +``` + +**Human replies.** A reply's `in_reply_to_id` points at the thread opener. +Collect non-bot replies in the window, then resolve any parent id that is +not in the window's bot-comment set (a reply to an OLDER bot comment) with +a direct fetch, keeping only bot-authored parents: + +```sh +jq --arg bot "$BOT" --arg c "$CUTOFF" -r \ + '[.[] | select(.user.login != $bot and (.in_reply_to_id? != null) + and .created_at >= $c)] + | .[] | [.in_reply_to_id, .id, .user.login, .created_at, + (.pull_request_url|split("/")|last)] | @tsv' \ + "$WORK/pc.json" 2>/dev/null | sort -u > "$WORK/replies.tsv" +comm -23 <(cut -f1 "$WORK/replies.tsv" | sort -u) \ + <(jq -r '.[].id' "$WORK/bot_inline_raw.json" | sort) \ + | while read -r cid; do + gh api "repos/$REPO/pulls/comments/$cid" \ + --jq '[.id, .user.login] | @tsv' < /dev/null +done | awk -F'\t' -v bot="$BOT" '$2==bot {print $1}' \ + > "$WORK/old_bot_parents.txt" +# replies.tsv still holds every non-bot reply repo-wide, including replies +# in human-opened threads; keep only rows whose parent is bot-authored. +cat <(jq -r '.[].id' "$WORK/bot_inline_raw.json") \ + "$WORK/old_bot_parents.txt" | sort -u > "$WORK/bot_parent_ids.txt" +awk -F'\t' 'NR==FNR {ok[$1]=1; next} ok[$1]' \ + "$WORK/bot_parent_ids.txt" "$WORK/replies.tsv" > "$WORK/bot_replies.tsv" +``` + +Every later step reads `bot_replies.tsv`; `replies.tsv` is an intermediate. + +## Step 2: classify + +**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): + +```sh +jq '[.[] | {id, pr: (.pull_request_url|split("/")|last|tonumber), path, + line, created_at, in_reply_to_id, len: (.body|length), + label: ((.body + | capture("\\*\\*(?[a-z]+ \\([^)]*\\)|[a-z]+):?\\*\\*")).l? + // "FOLLOWUP"), + reactions, body}]' \ + "$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. + +**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). + +## Step 3: compute the metrics + +- **Runs and verdicts**: rows of `reviews.tsv` minus sweep follow-ups; + verdict mix (`APPROVED` / `CHANGES_REQUESTED` / `COMMENTED`). +- **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 + comments, and the share carrying a sketch block: + +```sh +jq '[.[] | select(.label != "FOLLOWUP") | .len] + | {n: length, mean: (add/length|floor), + median: (sort | .[length/2|floor]), + p90: (sort | .[(length*0.9|floor)]), max: max, min: min}' \ + "$WORK/bot_inline.json" +jq '[.[] | select(.body + | contains("A sketch, not a committable replacement"))] | length' \ + "$WORK/bot_inline.json" +``` + +- **Duplication, three grains**: + 1. Byte-identical bodies in one run (`group_by(.body) | map(select(length + > 1))`); the in-run clusterer should have merged these. + 2. Same-run same-root-cause clusters: distinct anchors, one underlying + defect (read the bodies; no mechanical test). + 3. Cross-run families: the same mechanism re-derived across runs on + different anchors or files. These are what defect-identity suppression + (path plus similarity floors) misses when the variants span files; + list each family with its thread ids. +- **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. +- **Attribution**: check each posted review body and guidance comment for a + version marker or footer, and report presence per body. + +## Known constraints + +- **Hidden HTML comments are sanitizer-stripped.** gh-aw's safe-output + ingest sanitizer (`removeXmlComments`) deletes every agent-written HTML + comment, so the absence of `` markers in posted + bodies is expected unless the checkout being audited ships a visible + (non-HTML-comment) footer; check for `version-footer.ts` in the lib + rather than assuming either state. Do not read marker absence alone as a + version-attribution regression; the engine-appended + `gh-aw-agentic-workflow` marker is what identifies the guidance comment. +- **GraphQL may be unavailable.** Under a GET-only gh broker, the GraphQL + endpoint (needed for `reviewThreads` resolution state) is blocked. Thread + resolution is a positive signal column the thumbs sweep normally reports; + when GraphQL is blocked, state explicitly in the report that + thread-resolution counts are unavailable. Do not approximate them from + reply activity. +- **`since` is an `updated_at` filter.** Always re-filter on + `created_at`/`submitted_at`; an old comment edited inside the window + otherwise pollutes the sample. +- **Search caps at 1,000 results.** `search/issues` returns at most 1,000 + items even with `--paginate`, silently. The `prs.txt` collection block + compares `total_count` against the cap; when it warns, split the window + into `updated:A..B` sub-ranges or report every reviews-derived count as + 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. + +## Step 4: report + +Use these sections, in order: + +1. **Window**: repo, cutoff, collection time, bot login. +2. **Volume and verdicts**: runs, verdict mix, comments per PR, label mix. +3. **Quality**: blocking findings and their outcomes; notable catches; + misses (a finding the author correctly refuted, with the refutation). +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. +7. **Caveats**: GraphQL availability, marker expectations, sample-size + limits, anything unverifiable. + +## Step 5: map findings to work items + +List the open Khan/actions PRs and map every report finding to one of: +an existing PR (cite the number and what it covers), a gap in an existing +PR (comment on that PR with the measured data rather than widening it), +or a new-PR / design-note candidate: + +```sh +gh pr list --repo Khan/actions --state open --limit 50 \ + --json number,title,isDraft +``` + +Post measured data points as PR comments where they strengthen an open +PR's motivation (the gh shim rejects inline `--body`; write the body to a +file and pass `--body-file`). End the report with the mapped list so the +operator can prioritize.