From b35a80b19b78f1617d081b545c4701fc0b922e1b Mon Sep 17 00:00:00 2001 From: Jim Huang Date: Thu, 22 Oct 2026 14:27:26 +0800 Subject: [PATCH 1/2] Automate PR review and rebase decisions The PR sweep now says who each pull request waits on, queues the ones waiting on a maintainer for review, lists Refs lines that close nothing, and asks for a rebase on its own when a PR merges cleanly but fails only a cargo-audit advisory main has already fixed. post-review.sh posts a confirmed review only while the PR is where the reader left it. These were steps a triage run did by hand and got wrong at least once, as when a standing request for Closes #141 went unseen. CONTRIBUTING.md now asks pull requests to avoid Refs unless it sits beside a Closes or a full account of the motivation and the implementation. --- .claude/skills/codetrial-contribute/SKILL.md | 7 +- .../codetrial-contribute/collaboration.md | 12 +- .claude/skills/codetrial-triage/SKILL.md | 3 +- .../skills/codetrial-triage/issue-sweep.sh | 20 +- .claude/skills/codetrial-triage/pr-sweep.sh | 161 ++++++-- .claude/skills/codetrial-triage/pulls.md | 99 +++-- .claude/skills/codetrial-triage/waiting.md | 7 +- .claude/skills/codetrial-verify/SKILL.md | 10 +- .claude/skills/codetrial-verify/ci-failure.sh | 85 +++- .../skills/codetrial-verify/post-review.sh | 70 ++++ .claude/skills/codetrial-verify/review.md | 32 +- CONTRIBUTING.md | 7 +- tests/test_skill_scripts.py | 386 +++++++++++++++++- 13 files changed, 788 insertions(+), 111 deletions(-) create mode 100755 .claude/skills/codetrial-verify/post-review.sh diff --git a/.claude/skills/codetrial-contribute/SKILL.md b/.claude/skills/codetrial-contribute/SKILL.md index 253ee49a..0ef660f6 100644 --- a/.claude/skills/codetrial-contribute/SKILL.md +++ b/.claude/skills/codetrial-contribute/SKILL.md @@ -323,9 +323,10 @@ the git steps behind them: commit. - "Append `Closes #N`" or "avoid ambiguous `Refs`": the PR finishes the issue, so end the PR body, and by this tree's habit the commit message, with - `Closes #N`, each on its own line. `Refs #N` is for a PR that only touches - the issue, and then the body says what is left; a bare `Refs: #60` told the - reviewer nothing. + `Closes #N`, each on its own line. Avoid `Refs #N`: it belongs only beside a + `Closes #N`, or in a body that states the motivation and the implementation + in full, including what is left; a bare `Refs: #60` told the reviewer + nothing. - A code change requested in review: check it against the current head, then make it. When any item in the review is unclear, ask about all of them before changing any, since items in one review are often diff --git a/.claude/skills/codetrial-contribute/collaboration.md b/.claude/skills/codetrial-contribute/collaboration.md index 0faf2a0d..a4f3f5b6 100644 --- a/.claude/skills/codetrial-contribute/collaboration.md +++ b/.claude/skills/codetrial-contribute/collaboration.md @@ -70,11 +70,13 @@ for its own issue, not folded in. this pull request is worth proceeding." (#245, closed while #244 fixed #131). Review the open one rather than writing a competitor. - Close what the PR finishes. CONTRIBUTING.md has the rule: `Closes #N` when - the PR finishes the issue, `Refs #N` when it only touches it. Maintainers - call `Refs` on a PR that does finish its issue ambiguous ("Avoid ambiguous - `Refs #210`. Do close an issue when you submit a pull request." on #256, - "Avoid using ambiguous `Refs #261`" on #265), so decide by the issue's acceptance criteria, not by caution. A - `Refs` PR says in its body what remains: + the PR finishes the issue, and `Refs #N` avoided, since a reference that + closes nothing is easy to add to every PR. Maintainers call `Refs` on a PR + that does finish its issue ambiguous ("Avoid ambiguous `Refs #210`. Do close + an issue when you submit a pull request." on #256, "Avoid using ambiguous + `Refs #261`" on #265), so decide by the issue's acceptance criteria, not by + caution. `Refs` belongs only beside a `Closes #N`, or in a body that states + the motivation and the implementation in full, including what remains: [#161](https://github.com/sysprog21/codetrial/issues/161#issuecomment-6131824178) asked for `Refs #161` because the cap and modality toggle stay open. One PR may close two issues when that was agreed, each `Closes #N` on its own line diff --git a/.claude/skills/codetrial-triage/SKILL.md b/.claude/skills/codetrial-triage/SKILL.md index c948df70..1b4d2638 100644 --- a/.claude/skills/codetrial-triage/SKILL.md +++ b/.claude/skills/codetrial-triage/SKILL.md @@ -207,7 +207,8 @@ titles or the same broad symptom are insufficient to establish duplication. reference, and never promise the merge will close it otherwise. When the PR delivers the issue and only says `Refs #N`, the fix is a comment on the PR asking its author to make the last line `Closes #N`, so the merge records it - (#143 for #141). + (#143 for #141). The PR sweep's `refs_only` lists these pairs and whether a + maintainer already asked ([pulls.md](pulls.md#closing-references)). - **Already shipped**: the tree already does what the request asks. `git grep -n` over `src` and `web` finds a candidate and settles nothing, since a string also lives in a fixture, a disabled branch, a generated file or code behind a flag diff --git a/.claude/skills/codetrial-triage/issue-sweep.sh b/.claude/skills/codetrial-triage/issue-sweep.sh index c4acb58d..4efccaf9 100755 --- a/.claude/skills/codetrial-triage/issue-sweep.sh +++ b/.claude/skills/codetrial-triage/issue-sweep.sh @@ -13,11 +13,14 @@ # unanswered issues no maintainer other than the author has commented on, # oldest first, so a collaborator's own follow-up is not an answer; # the viewer's own issues and a deleted account's are left out, -# since nobody else answers the first and nobody can the second +# since nobody else answers the first and nobody can the second; +# closing_prs lists open PRs whose merge closes the issue, which +# makes it the PR sweep's once the PR is read (waiting.md) # waiting issues whose newest maintainer comment is newer than anything # the reporter did since, comment or body edit; overdue once it # is older than wait-days. Issues a maintainer opened are left -# out; claimed marks a reporter who also holds the issue +# out; claimed marks a reporter who also holds the issue, and +# closing_prs is as in unanswered # linked issues a pull request cross-referenced after the issue opened, # split into merged and still open; a PR from another # repository is left out, since its number means nothing here @@ -54,6 +57,9 @@ query='query($owner: String!, $name: String!, $endCursor: String) { totalCount nodes { editedAt editor { login } } } assignees(first: 10) { totalCount nodes { login } } + closedByPullRequestsReferences(first: 10, includeClosedPrs: false) { + nodes { number repository { nameWithOwner } } + } assigned: timelineItems(last: 20, itemTypes: [ASSIGNED_EVENT]) { totalCount nodes { ... on AssignedEvent { createdAt @@ -128,6 +134,10 @@ def cut: .comments.totalCount > 100 or .timelineItems.totalCount > 100 or .userContentEdits.totalCount > 20; def prs: [.timelineItems.nodes[] | select(.source.number != null and .source.repository.nameWithOwner == $repo)]; +# A PR in another repository may close the issue too; as in prs, its number +# means nothing here. +def closing: [.closedByPullRequestsReferences.nodes[]? + | select(.repository.nameWithOwner == $repo) | .number]; def split: {open: [.[] | select(.state == "OPEN") | .number] | unique, merged: [.[] | select(.mergedAt != null) | .number] | unique}; def mine: .author.login as $a | [.comments.nodes[] @@ -194,7 +204,8 @@ def claim: . as $i | maint as $m maintainers_known: ($pushers != null), unanswered: [$issues[] | select(.author != null) | .author.login as $a | select($a != $me and all(maint[]; .author.login == $a)) - | {issue: .number, author: $a, age_days: (.createdAt | age)}] + | {issue: .number, author: $a, age_days: (.createdAt | age), + closing_prs: closing}] | sort_by(-.age_days), waiting: [$issues[] | select(.author != null and (staff | not)) | .number as $n | .author.login as $a @@ -203,7 +214,8 @@ def claim: . as $i | maint as $m | {issue: .number, author: .author.login, asked_by: $ask.author.login, asked_at: $ask.createdAt, idle_days: ($ask.createdAt | age), overdue: (($ask.createdAt | age) >= $days), - claimed: any($claims[]; .issue == $n and .assignee == $a)}] + claimed: any($claims[]; .issue == $n and .assignee == $a), + closing_prs: closing}] | sort_by(-.idle_days), linked: [$issues[] | (.createdAt | ts) as $born | [prs[] | select((.createdAt | ts) >= $born) | .source] as $prs diff --git a/.claude/skills/codetrial-triage/pr-sweep.sh b/.claude/skills/codetrial-triage/pr-sweep.sh index db669132..d9812d83 100755 --- a/.claude/skills/codetrial-triage/pr-sweep.sh +++ b/.claude/skills/codetrial-triage/pr-sweep.sh @@ -19,7 +19,12 @@ # unless the latest verdict is an approval, it is the rebase # request on a PR that still conflicts, or the activity was cut # off at 100 -# rebase conflicting PRs with no current rebase request, to ask +# rebase PRs to send the fixed rebase request, each with its reason: +# conflicts, or stale_base when its only failures are what +# ci-failure.sh calls a stale base. Neither is a draft, the +# viewer's, stacked on another branch, cut off at 100, or already +# holding a current request in any wording. PR_SWEEP_CI names the +# ci-failure.sh to ask, for tests # hide superseded rebase requests in a known wording, to hide # hide_ask other rebase requests, to confirm before hiding # (all three: pulls.md, "Conflicting branches") @@ -32,11 +37,21 @@ # the worst row of the table in grade() that the PR matches, from # blocked, low, medium and medium-high down to high when none # does; draft PRs are set aside. pulls.md, "Rank by size and -# merge confidence", says how to read it. +# merge confidence", says how to read it. waits_on names whose +# move it is (author, maintainer, ci, approval); why names a +# failing job main's latest run also fails, which ci-failure.sh +# settles +# review the PRs, not the viewer's, whose waits_on is maintainer, +# smallest first: nothing recorded blocks them, so they wait on a +# first review, a re-review after a push, or a merge +# refs_only PRs that close no issue and name one with Refs, which +# CONTRIBUTING.md asks PRs to avoid; asked says a maintainer +# already asked for Closes #N on that PR # superseded runs awaiting approval on a head no open PR has any more, # which need nothing # moved with previous.json, a saved earlier output: the PRs whose head, -# mergeability, decision, checks, threads or grade changed +# mergeability, decision, checks, threads, grade or waits_on +# changed # gone with previous.json: PRs no longer open, with merged or closed # prs number, title, updatedAt, head, author, closing issues and files # of every open PR, for judging overlap that shares no issue and @@ -94,8 +109,8 @@ query='query($owner: String!, $name: String!, $endCursor: String) { pullRequests(states: OPEN, first: 50, after: $endCursor) { pageInfo { hasNextPage endCursor } nodes { - number title isDraft authorAssociation headRefOid mergeable baseRefName - updatedAt headRefName headRepository { nameWithOwner } + number title body isDraft authorAssociation headRefOid mergeable + baseRefName updatedAt headRefName headRepository { nameWithOwner } additions deletions reviewDecision author { login } closingIssuesReferences(first: 100) { totalCount nodes { number } } @@ -171,11 +186,26 @@ runs=$(gh api --paginate \ head_repo: .head_repository.full_name, actor: .actor.login}') runs=$(printf '%s\n' "$runs" | jq -s .) +# The jobs main's latest finished run failed, read once the way ci-failure.sh +# reads them, so a PR's reason can say main fails the same job. Null when the +# lookup fails, which names nothing. +main_failed=null +if main_run=$(gh run list --repo "$repo" --workflow check.yml --branch main \ + --event push --status completed --limit 10 --json databaseId,conclusion \ + --jq '[.[] | select(.conclusion != "cancelled")][0].databaseId // empty' \ + 2> /dev/null) && [ -n "$main_run" ]; then + main_failed=$(gh run view "$main_run" --repo "$repo" --json jobs \ + --jq '[.jobs[] | select(.conclusion + | IN("failure", "cancelled", "timed_out", "startup_failure")) + | .name]' 2> /dev/null) || main_failed=null +fi + # What counts as a rebase request, when it is current, and which copies are # hidden or only offered for hiding: pulls.md, "Conflicting branches". out=$(printf '%s\n%s\n' "$prs" "$runs" | jq -s --argjson days "$days" --arg me "$me" \ --arg base "$base" --rawfile text "$text" --argjson known "$known" \ - --argjson pushers "$pushers" --argjson previous "$previous" ' + --argjson pushers "$pushers" --argjson previous "$previous" \ + --argjson main_failed "$main_failed" --arg repo "$repo" ' def human: .author != null and .author.__typename != "Bot"; def ts: if . == null then 0 else fromdateiso8601 end; def pushed: (.commits.nodes[0].commit // {checkSuites: {nodes: []}}) as $c @@ -210,9 +240,12 @@ def asked_here: any(requests[]; .current); # it still conflicts: that request is the standing one, so no fixed one is kept # beside it and it is never offered for hiding. def hand_asked: asked_here and all(asks_rebase[]; .current | not); -def needs_rebase: .mergeable == "CONFLICTING" and (.isDraft | not) - and .baseRefName == $base and $base == "main" - and nudgeable and whole +# Who the fixed text may go to at all: it names main, so a stacked PR is left +# to its reviewer; a draft is still being written; and a PR whose history was +# cut may hold a request past what was read. +def rebase_eligible: (.isDraft | not) and .baseRefName == $base + and $base == "main" and nudgeable and whole; +def needs_rebase: .mergeable == "CONFLICTING" and rebase_eligible and (asked_here | not); # A test is anything under a tests/ directory, the browser check, or a file # named as one (test_x.py, x.test.js); a substring would also catch problem @@ -275,53 +308,89 @@ def open_requests: .headRefOid as $head | group_by(.author.login) | map(max_by(.submittedAt | ts)) | [.[] | select(.state == "CHANGES_REQUESTED" and .commit.oid == $head) | {rebase: (.body | opens_with_rebase)}]; -# One row per condition: when it holds, the reason it adds and the grade it -# caps the PR at. The grade is the worst row that holds, so adding a problem -# never raises it, and the reasons and the grade cannot disagree. A row with -# no reason only strengthens one that already gave its own. +# One row per condition: when it holds, the reason it adds, the grade it caps +# the PR at, and whose move it makes it. The grade is the worst row that holds, +# so adding a problem never raises it, and the reasons and the grade cannot +# disagree. A row with no reason only strengthens one that already gave its +# own. waits_on is read off the same rows, so a new row cannot leave a PR in +# the review queue while grading it down: a held run outranks a pending check, +# which outranks anything the author must fix, and a PR no row hands to anyone +# waits on the maintainer. A failure main shares still reads as one for the author +# here, since only ci-failure.sh can tell it apart. def grade($c; $t): .isDraft as $draft | open_requests as $r | (.reviewDecision == "CHANGES_REQUESTED") as $cr | ($r | length > 0) as $cr_head | any($r[]; .rebase | not) as $cr_work | ($t.human | length > 0) as $talk - | [{when: (.mergeable == "CONFLICTING"), why: "conflicts", grade: "medium"}, - {when: $cr_work, why: "changes requested on this head", grade: "blocked"}, + | [{when: (.mergeable == "CONFLICTING"), why: "conflicts", grade: "medium", + who: "author"}, + {when: $cr_work, why: "changes requested on this head", grade: "blocked", + who: "author"}, {when: ($cr_head and ($cr_work | not)), - why: "changes requested on this head (rebase)", grade: "medium"}, + why: "changes requested on this head (rebase)", grade: "medium", + who: "author"}, {when: ($cr and ($cr_head | not)), why: "changes requested before the last push, or hidden", grade: "medium-high"}, - {when: $talk, grade: "low", + {when: $talk, grade: "low", who: "author", why: "\($t.human | length) open thread(s) by \($t.by | join(", "))"}, {when: ($talk and (.mergeable == "CONFLICTING" or $cr_head)), why: null, grade: "blocked"}, - {when: ($t.bot | length > 0), grade: "medium", + {when: ($t.bot | length > 0), grade: "medium", who: "author", why: "\($t.bot | length) open bot thread(s)"}, - {when: ($c.failing | length > 0), grade: "low", - why: "failing: \($c.failing | join(", "))"}, - {when: ($c.held | length > 0), why: "held for approval", grade: "medium"}, - {when: ($c.pending | length > 0), grade: "medium-high", + # A job main also fails may still fail here for a reason of the PR, + # since a lane runs several gates, so the author keeps it and the reason + # says main shares the name, for ci-failure.sh to settle. + {when: ($c.failing | length > 0), grade: "low", who: "author", + why: ("failing: \($c.failing | join(", "))" + + ([$c.failing[] | select(IN(($main_failed // [])[]))] + | if length > 0 then "; main also fails \(join(", "))" else "" end))}, + {when: ($c.held | length > 0), why: "held for approval", grade: "medium", + who: "approval"}, + {when: ($c.pending | length > 0), grade: "medium-high", who: "ci", why: "pending: \($c.pending | join(", "))"}, - {when: ($c.gates | not), why: "no gates lane ran", grade: "medium"}, + {when: ($c.gates | not), why: "no gates lane ran", grade: "medium", + who: "author"}, {when: $c.truncated, why: "more than 50 checks, not all read", grade: "medium"}, + # GitHub is still computing it, which the sweep retries; nobody should + # review a PR that may not merge. {when: (.mergeable == "UNKNOWN"), why: "mergeability unknown", - grade: "medium"}, + grade: "medium", who: "ci"}, # Past 100 reviews or threads the newest may be the ones not read, so # nothing above can be trusted to have seen them. {when: (.reviews.totalCount > 100 or .reviewThreads.totalCount > 100), why: "more than 100 reviews or threads, not all read", grade: "medium"}] | map(select(.when)) as $hit | ["high", "medium-high", "medium", "low", "blocked"] as $ladder + | ["maintainer", "author", "ci", "approval"] as $turn | {grade: (if $draft then "draft" else $ladder[[$hit[].grade as $g | $ladder | index($g)] | max // 0] end), - why: [$hit[].why | strings]}; + why: [$hit[].why | strings], + waits_on: (if $draft then "author" + else $turn[[$hit[].who | strings as $w | $turn | index($w)] | max // 0] + end)}; def status: checks as $c | threads as $t | {pr: .number, title, author: .author.login, head: .headRefOid, size: size, files_truncated: (.files.totalCount > 100), mergeable, decision: .reviewDecision, checks: $c, threads: {human: ($t.human | length), bot: ($t.bot | length), by: $t.by}, closes: [.closingIssuesReferences.nodes[].number]} + grade($c; $t); + +# An issue named as #N, owner/repo#N or its URL, the forms related.sh in +# codetrial-contribute reads; issue_ref is the regex after a keyword, and +# captures the number. The dot a repository name may hold is escaped. +def ref_prefix: ($repo | gsub("\\."; "\\.")) as $r + | "(?:#|\($r)#|https://github\\.com/\($r)/issues/)"; +def issue_ref: ref_prefix + "([0-9]+)\\b"; + +# Issues the body names with Refs, which the merge leaves open, including each +# of a list such as "Refs #1, #2 and #3". +def refs: (ref_prefix + "[0-9]+\\b") as $one + | [.body // "" + | scan("(?i)\\brefs?:?\\s+(\($one)(?:[ \\t]*,?[ \\t]*(?:and\\s+)?\($one))*)") + | .[0] | scan(issue_ref) | .[0] | tonumber] + | unique; ([$prs[] | status] | sort_by(.size.total)) as $status | { approve: [$prs[] as $p | runs_of($p) @@ -361,7 +430,27 @@ def status: checks as $c | threads as $t and now - $at > $days * 86400) | {pr: .number, author: $author, review_by: $review.by, review_at: $review.at, idle_days: ((now - $at) / 86400 | floor)}], - rebase: [$prs[] | select(needs_rebase) | {pr: .number, author: .author.login}], + rebase: [$prs[] | select(needs_rebase) + | {pr: .number, author: .author.login, reason: "conflicts"}], + + # Merging cleanly, failing for its author, and otherwise as needs_rebase: + # ci-failure.sh decides below whether that failure is a stale base. + stale_candidates: [$prs[] | .number as $n + | select(.mergeable == "MERGEABLE" and rebase_eligible + and (asked_here | not) + and any($status[]; .pr == $n and .waits_on == "author" + and (.checks.failing | length > 0))) + | {pr: .number, author: .author.login, reason: "stale_base"}], + review: [$status[] | select(.waits_on == "maintainer" and .author != $me) + | .pr], + refs_only: [$prs[] | . as $p + | select(.closingIssuesReferences.totalCount == 0) + | refs[] as $n + | {pr: $p.number, issue: $n, + asked: any($p.reviews.nodes[], $p.comments.nodes[]; + human and staff and (.isMinimized | not) + and (.body // "" | [scan("(?i)\\b(?:close[sd]?|fix(?:e[sd])?|resolve[sd]?):?\\s+" + issue_ref) + | .[0] | tonumber] | any(.[]; . == $n)))}], hide: [$prs[] | .number as $n | select(whole) | asks_rebase as $r | ($r | map(.current) | rindex(true) // -1) as $keep @@ -382,7 +471,8 @@ def status: checks as $c | threads as $t | [$status[] | . as $now | $old[.pr | tostring] as $was | if $was == null then {pr, new: true} else {pr, changed: ([ - (("head", "mergeable", "decision", "grade", "checks", "threads") + (("head", "mergeable", "decision", "grade", "waits_on", "checks", + "threads") as $k | select($was[$k] != $now[$k]) | {($k): [$was[$k], $now[$k]]})] | add)} @@ -410,5 +500,18 @@ states=$(for n in $gone; do --jq '{pr: .number, state, mergedAt, merge: .mergeCommit.oid}' \ || jq -n --argjson n "$n" '{pr: $n, state: "UNKNOWN"}' done | jq -s .) -printf '%s\n' "$out" | jq --argjson states "$states" \ - 'if .gone == null then . else .gone = $states end' + +# One ci-failure.sh run per candidate, a handful at most: it reads job logs, +# which the sweep does not. A run that fails leaves its PR out, since this list +# posts without asking. +ci=${PR_SWEEP_CI:-$(dirname "$0")/../codetrial-verify/ci-failure.sh} +stale=$(printf '%s\n' "$out" | jq -r '.stale_candidates[].pr' | while read -r n; do + if found=$(sh "$ci" "$n" "$repo" < /dev/null 2> /dev/null); then + printf '%s\n' "$found" | jq -c --argjson n "$n" \ + 'select(.pr == $n and .stale_base == true) | .pr' || true + fi +done | jq -s .) +printf '%s\n' "$out" | jq --argjson states "$states" --argjson stale "$stale" \ + 'if .gone == null then . else .gone = $states end + | .rebase += [.stale_candidates[] | select(.pr | IN($stale[]))] + | del(.stale_candidates)' diff --git a/.claude/skills/codetrial-triage/pulls.md b/.claude/skills/codetrial-triage/pulls.md index 2935542a..d31caca6 100644 --- a/.claude/skills/codetrial-triage/pulls.md +++ b/.claude/skills/codetrial-triage/pulls.md @@ -37,7 +37,7 @@ Asking to sweep the PRs ("sweep the PRs", "run the PR sweep") authorizes three actions, because none carries text drafted during the sweep, each is cheap to undo or harmless, and holding them for a confirmation only stalls a contributor. A bare "continue" after an issue report is not that request: it -carries the work on, and the three actions go on the confirmed list instead. +carries the work on, and these actions go on the confirmed list instead. - Approving a run in `approve` whose `first_time` is true and whose `touches_workflows` and `files_truncated` are false. A run waits there because @@ -67,14 +67,17 @@ carries the work on, and the three actions go on the confirmed list instead. - Resolving a thread in `outdated`. Outdated means the commented lines changed, not that the concern was met, so report the resolved threads by PR and path; a reviewer who disagrees clicks unresolve. -- Requesting a rebase on a PR in `rebase`, then hiding every entry in `hide` - as outdated. The text is fixed in [rebase.md](rebase.md), so the maintainer - approved it once by writing it, and `hide` holds only requests in a wording - the maintainers have used for the same thing (listed in `pr-sweep.sh`), with - no inline comments; a hidden comment stays one click from readable. "Ask the - conflicting PRs to rebase" asks for this action alone, as "Approve the held - runs" does for approvals; report the conflicting PRs it skips (the account's - own, and those holding a current request) so the count matches the banner. +- Requesting a rebase on each PR in `rebase`, then hiding every entry in + `hide` as outdated. The text is fixed in [rebase.md](rebase.md), so the + maintainer approved it once by writing it, and `hide` holds only requests in a + wording the maintainers have used for the same thing (listed in + `pr-sweep.sh`), with no inline comments; a hidden comment stays one click from + readable. Each entry names its `reason`: `conflicts`, or `stale_base` for a PR + that merges but fails only where a rebase is the whole answer, as #334 did + (the definition is `stale_base` in `ci-failure.sh`). "Ask the conflicting PRs + to rebase" asks for the `conflicts` entries alone, as "Approve the held runs" + does for approvals; report the conflicting PRs it skips (the account's own, + and those holding a current request) so the count matches the banner. ```sh gh api -X POST "repos/$repo/actions/runs/$run/approve" @@ -153,9 +156,10 @@ SKILL.md does not close for age. ## Conflicting branches -`rebase` holds the non-draft PRs against the default branch that GitHub reports -as conflicting (the "This branch has conflicts that must be resolved" banner), -not opened by the account running the sweep, whose own branches are the +`rebase` holds, with `reason: "conflicts"`, the non-draft PRs against the +default branch that GitHub reports as conflicting (the "This branch has +conflicts that must be resolved" banner), not opened by the account running the +sweep, whose own branches are the maintainer's to rebase. A draft is still being written and is left alone, and a PR stacked on another branch is left to its reviewer, since the fixed text names `main`; for the same reason `rebase` stays empty on a repository whose @@ -244,14 +248,11 @@ only; read it from the PR's checks tab. A `cargo-audit` advisory that `main` also has is the maintainer's, and the PR must not carry a lockfile bump to clear it: #304 was told it "needs nothing from you", and #305 to drop its unrelated `Cargo.lock` change. Once `main` lands the -fix (#331 moved to zip 9), a PR still failing on that advisory has a stale base. -When it conflicts it is in `rebase` unless its current head already carries a -request. When it still merges, `rebase` skips it, so read the PR's reviews and -comments first: a request on the current head in any wording already counts, as -[Conflicting branches](#conflicting-branches) says, and posting the fixed text -beside #326's own request sent a second notification for nothing. Only without -one does the fixed request go on the confirmed list, since the sweep requests a -rebase without asking only on a conflicting branch. +fix (#331 moved to zip 9), a PR still failing on that advisory has a stale base, +and the sweep puts it in `rebase` under the same limits as a conflicting one, +so a request in any wording on its head, such as #326's own, keeps it out. A +PR that touches a Cargo file is never a stale base: read whether its lockfile +change brought the advisory before asking for anything. A clean merge does not establish readiness. For a dependent draft, check whether its prerequisite merged and whether the author still needs to rebase, remove @@ -303,19 +304,63 @@ on 9dc74fe), so green there proves less; say so and ask for a rebase or a rerun. Two PRs that rewrite the same harness function (#376 and #396 on `json_graph`) conflict as soon as one merges: name the order. +Each `status` entry carries `waits_on`, read off the same rows as the grade: +`approval` for a held run, `ci` for a pending check or a mergeability GitHub +has not computed, `author` for a conflict, a changes request on the head, an +open thread, a failing check or no gates lane, and `maintainer` otherwise, +which is a first review, a re-review after a push, or a merge. `review` lists +those last PRs, not the account's own, smallest first. A failing job whose name +`main`'s latest run also fails says so in `why`; the lane runs several gates, +so only `ci-failure.sh` can tell a failure `main` shares, which is the +maintainer's, from the PR's own in the same lane. + Save each sweep and pass it back as the third argument the next time. `moved` -then lists the PRs whose head, mergeability, decision, grade, checks or threads -changed, each field as `[before, after]`, plus `{pr, new: true}` for a PR +then lists the PRs whose head, mergeability, decision, grade, `waits_on`, +checks or threads changed, each field as `[before, after]`, plus `{pr, new: true}` for a PR opened since. `gone` says which left the open list, merged or closed and at which commit, or `UNKNOWN` when that lookup failed. A repeated request then costs one call and reports only what moved. A PR that moved to `CONFLICTING` right after a merge is one that shared its files (#373 and #396 after #376); `rebase` lists it once no current request stands. When `moved` and `gone` are -empty, say that nothing changed, then say who each PR waits on from its `why`, -not its grade alone: "held for approval" waits on a maintainer to approve the -run, "pending" on CI, "changes requested before the last push, or hidden" on a -re-review, a `high` PR with an empty `why` on a maintainer to review or merge -it, and any other reason on its author. +empty, say that nothing changed, then say who each PR waits on from its +`waits_on`, with the `why` that put it there. + +## Closing references + +`refs_only` lists each PR that closes no issue and names one with `Refs #N`, so +its merge closes nothing. CONTRIBUTING.md asks PRs to avoid `Refs`: it stands +only beside a `Closes #N`, which is why a PR that closes something is left out +here, or in a body that states the motivation and the implementation in full, +as #110 does for #257 and #362 for its parent #306. Otherwise, when the PR +delivers the issue, ask its author to end the body with `Closes #N` (SKILL.md, +"Superseded by an open pull request"); when it does not, ask for the motivation +and the implementation in the body, or for the `Refs` line to go. `asked` true +means a maintainer already asked for `Closes` on that PR, so the next move is +the author's and nothing is posted again. Either comment needs a judgement of +the PR, so it goes on the confirmed list. + +## Review the queue + +`review` is the queue for "review what is waiting on me". Each PR in it gets the +whole of [review.md](../codetrial-verify/review.md), and they are independent, +so run one read-only agent per PR in parallel rather than reading them in +turn. Each agent gets the PR number, its head, the issue it closes, the +scratch directory, the files it may write there (`pr-$pr-*`), and four rules: +read-only, no posting, cite rather than repeat a finding already in a thread, +and return the checklist, the findings by level with `file:line` at the head, +the commit series, and what was not checked. Name what is particular to each PR in its prompt: the docs/ +owner it likely touches, a sibling PR on the same code, an issue that will +conflict with it. Verify each finding the report leans on in the code before +drafting it; an agent's summary is a lead, not evidence. + +Merge the reports into one local summary, then draft one review per PR under +review.md's step 4. Leave out of the drafts what is the maintainer's decision +rather than the author's work, such as a behavior dropped outside the accepted +scope or which of two sibling PRs wins, and list those separately for the +user. Record each PR's `updatedAt` from `prs` when showing the drafts, and post +each confirmed review with `post-review.sh` in +[codetrial-verify](../codetrial-verify/review.md#4-report), which refuses when +the head or `updatedAt` moved. ## Apply the comments diff --git a/.claude/skills/codetrial-triage/waiting.md b/.claude/skills/codetrial-triage/waiting.md index 2d4a83e1..f6ac01a8 100644 --- a/.claude/skills/codetrial-triage/waiting.md +++ b/.claude/skills/codetrial-triage/waiting.md @@ -15,8 +15,11 @@ The four sweep lists are the queue, and each one has a fixed next move. An on review of that PR, which the PR sweep owns, not on the reporter, once the PR is read and does what the issue asks: `open_prs` holds every PR that cross-referenced the issue, and a mention alone does not suspend the -reporter's reminder. Such an entry gets neither a reminder nor a first answer -on the issue (#370 and #385 sat in +reporter's reminder. `closing_prs` on the `unanswered` or `waiting` entry +itself names the open PRs whose merge closes the issue, whoever wrote them, +which settles the candidates without reading every cross-reference; reading +that one PR's title and scope still decides it. Such an entry gets neither a +reminder nor a first answer on the issue (#370 and #385 sat in `unanswered`, #159 and #298 in overdue `waiting`, all with PRs open): - `unanswered`: no maintainer has spoken. Triage these first, oldest first, with diff --git a/.claude/skills/codetrial-verify/SKILL.md b/.claude/skills/codetrial-verify/SKILL.md index b47691c7..196dd29e 100644 --- a/.claude/skills/codetrial-verify/SKILL.md +++ b/.claude/skills/codetrial-verify/SKILL.md @@ -150,14 +150,16 @@ and the author is not asked to fix it. Otherwise `gates` names what `scripts/test.sh` failed and `errors` the lines that say why; rerun that lane locally with the same head. A `cargo-audit` failure on a PR whose base predates the advisory fix is a stale base, answered by a rebase (#334), not a code -change. The aggregate `check` job only collects the lanes, so its entry points -at the failing `gates ()` one. A check outside Actions has only its -link, and `held` checks have no log until a maintainer approves the run. +change; `stale_base` (defined in the script's header) says exactly that, and +the PR sweep lists such a PR in `rebase` on its own. The aggregate `check` job +only collects the lanes, so its entry points at the failing `gates ()` +one. A check outside Actions has only its link, and `held` checks have no log +until a maintainer approves the run. The raw calls behind it, when one answer needs checking: ```sh -gh pr view "$pr" --repo sysprog21/codetrial --json headRefOid +gh pr view "$pr" --repo sysprog21/codetrial --json headRefOid,files,changedFiles gh pr checks "$pr" --repo sysprog21/codetrial --json name,state,bucket,link gh run view "$run" --repo sysprog21/codetrial --json headSha,status,conclusion,jobs gh run list --repo sysprog21/codetrial --workflow check.yml --branch main \ diff --git a/.claude/skills/codetrial-verify/ci-failure.sh b/.claude/skills/codetrial-verify/ci-failure.sh index bae14d33..f7572916 100755 --- a/.claude/skills/codetrial-verify/ci-failure.sh +++ b/.claude/skills/codetrial-verify/ci-failure.sh @@ -20,6 +20,15 @@ # Actions, its link only. current and on_main are null when the # lookup behind them failed, and gates, errors and excerpt when # the job log could not be read. +# stale_base true when every failing lane failed only gates that read +# outside data (the table in the last jq below: cargo-audit +# reads the advisory database), on the current head, main's +# latest run started after those runs and succeeded in each of +# those lanes, nothing is pending or held, no check outside +# Actions fails, and the PR changes none of the files those gates +# read: main has since moved past the data, and a rebase is the +# whole answer. False otherwise, including when a lookup failed. +# This is the one definition; the docs point here. set -eu @@ -32,7 +41,13 @@ case $pr in ;; esac -head=$(gh pr view "$pr" --repo "$repo" --json headRefOid --jq .headRefOid) +# The head, and the files the PR changes: a PR that changes what a gate reads +# may have brought the failure itself, so it is never a stale base. gh reads at +# most 100 files, and cut says the list may hide one. +meta=$(gh pr view "$pr" --repo "$repo" --json headRefOid,files,changedFiles --jq \ + '{head: .headRefOid, files: [.files[].path], + cut: (.changedFiles > (.files | length))}') +head=$(printf '%s\n' "$meta" | jq -r .head) # gh pr checks exits non-zero when a check fails, and still prints them all, so # its status says nothing; only an array on stdout says the lookup worked. A PR @@ -49,19 +64,29 @@ printf '%s\n' "$checks" | jq -e 'type == "array"' > /dev/null 2>&1 || { # opened from its own main, so only a push counts; a run cancelled whole says # nothing about its jobs and is passed over. A job counts as failed by the same # reading as the PR's checks below, so a lane that times out on both is shared. +# A lane absent from the failures may also have been skipped or never run, which +# says nothing about whether main passes, so stale_base below reads the +# successes from main_jobs itself. A main run older than the PR's says nothing +# about data published since (a new advisory fails every PR before main runs +# again), so its start time is kept too. +main_at= if ! main_run=$(gh run list --repo "$repo" --workflow check.yml --branch main \ - --event push --status completed --limit 10 --json databaseId,conclusion \ - --jq '[.[] | select(.conclusion != "cancelled")][0].databaseId // empty'); then - main_failed=null + --event push --status completed --limit 10 \ + --json databaseId,conclusion,createdAt \ + --jq '[.[] | select(.conclusion != "cancelled")][0] // empty + | "\(.databaseId) \(.createdAt)"'); then + main_jobs=null elif [ -z "$main_run" ]; then - main_failed='[]' + main_jobs='[]' else - main_failed=$(gh run view "$main_run" --repo "$repo" --json jobs \ - --jq '[.jobs[] | select(.conclusion - | IN("failure", "cancelled", "timed_out", "startup_failure")) - | .name]') \ - || main_failed=null + main_at=${main_run#* } + main_run=${main_run% *} + main_jobs=$(gh run view "$main_run" --repo "$repo" --json jobs \ + --jq '[.jobs[] | {name, conclusion}]') || main_jobs=null fi +main_failed=$(printf '%s\n' "$main_jobs" | jq -c 'if . == null then null + else [.[] | select(.conclusion + | IN("failure", "cancelled", "timed_out", "startup_failure")) | .name] end') # Which checks failed, by the same reading as checks in pr-sweep.sh: failed, # cancelled or timed out counts, a run held for approval does not (gh files it @@ -81,7 +106,10 @@ defs='def failed: (.bucket == "fail" or .bucket == "cancel" failing=$(printf '%s\n' "$checks" | jq -r "$defs"'.[] | select(failed) | job as $id | [.name, $id.run, $id.job] | @tsv' \ | while IFS="$(printf '\t')" read -r name run job; do - sha=$(gh run view "$run" --repo "$repo" --json headSha --jq .headSha || true) + ran=$(gh run view "$run" --repo "$repo" --json headSha,createdAt \ + --jq '.headSha + " " + .createdAt' || true) + sha=${ran% *} + ran_at=${ran#* } # Each log line is job, step and a timestamped message; keep the # message. A log that cannot be read leaves gates, errors and excerpt @@ -94,6 +122,7 @@ failing=$(printf '%s\n' "$checks" | jq -r "$defs"'.[] | select(failed) log=$(printf '%s\n' "$raw" | cut -f3-) printf '%s\n' "$log" | jq -Rs --arg name "$name" --arg run "$run" \ --arg job "$job" --arg sha "$sha" --arg head "$head" \ + --arg ran_at "$ran_at" --arg main_at "$main_at" \ --argjson main "$main_failed" --argjson read "$read_log" ' # The timestamp goes, and with it the byte order mark the first # line of a job log starts with. @@ -116,14 +145,42 @@ failing=$(printf '%s\n' "$checks" | jq -r "$defs"'.[] | select(failed) excerpt: (if $at == null then $lines[-30:] else $lines[[$at - 30, 0] | max:$at] end | map(select(length > 0)) | join("\n")), - on_main: (if $main == null then null else $name | IN($main[]) end)} + on_main: (if $main == null then null else $name | IN($main[]) end), + before_main: ($ran_at != "" and $main_at != "" + and $ran_at < $main_at)} | if $read then . else .gates = null | .errors = null | .excerpt = null end' done | jq -s .) +# The check job only collects the lanes, so it says nothing of its own. A +# failing check outside Actions (cubic) has no gates to read, so a rebase is not +# known to clear it, and it rules a stale base out. Each gate that reads data +# from outside the tree maps to the files whose change would make the failure +# the PR's own; a gate added here needs nothing else. printf '%s\n' "$checks" | jq --argjson failing "$failing" --arg head "$head" \ - --argjson pr "$pr" "$defs"'{ + --argjson pr "$pr" --argjson meta "$meta" --argjson jobs "$main_jobs" \ + "$defs"' + # The gate itself, the CI that installs it and the cargo config it reads + # count as well as the lockfile: a PR that changes how the audit runs owns + # its failure even when main passes. + {"cargo-audit": "(^|/)Cargo\\.(lock|toml)$|^\\.cargo/|^scripts/test\\.sh$|^\\.github/"} + as $outside | + [($jobs // [])[] | select(.conclusion == "success") | .name] as $passed | + [$failing[] | select(.name != "check")] as $lanes | + ([$lanes[] | (.gates // [])[]] | unique) as $gates | + [.[] | select(failed and ([job] == []))] as $apps | +{ pr: $pr, head: $head, pending: [.[] | select(.bucket == "pending" and (failed | not)) | .name], held: [.[] | select(.state == "ACTION_REQUIRED") | .name], - failing: ($failing + [.[] | select(failed and ([job] == [])) | {name, link}]) + failing: ([$failing[] | del(.before_main)] + [$apps[] | {name, link}]), + stale_base: (($meta.cut | not) and ($lanes | length > 0) + and ($apps | length == 0) + and ([.[] | select((.bucket == "pending" and (failed | not)) + or .state == "ACTION_REQUIRED")] + | length == 0) + and all($lanes[]; .current == true and .before_main + and (.name | IN($passed[])) + and (.gates | length > 0) and all(.gates[]; $outside[.] != null)) + and all($gates[]; $outside[.] as $re + | all($meta.files[]; test($re) | not))) }' diff --git a/.claude/skills/codetrial-verify/post-review.sh b/.claude/skills/codetrial-verify/post-review.sh new file mode 100755 index 00000000..b322181c --- /dev/null +++ b/.claude/skills/codetrial-verify/post-review.sh @@ -0,0 +1,70 @@ +#!/bin/sh +# Post a review the user confirmed, only while the pull request is still where +# it was when they read it. The payload is the one review.md describes, already +# passed through review-anchors.sh; updatedAt is the value recorded when the +# text was shown to the user. +# +# Usage: post-review.sh [repo] +# Prints the posted review's URL. Exits 3, posting nothing, when the +# head moved, the PR was updated since, or a comment would land outside +# the diff; 2 on bad arguments. +# +# A new head means a new diff, so the anchors and the findings both need reading +# again. A new comment or edit at the same head may answer a finding, which is +# why updatedAt is compared too. The anchors are checked once more rather than +# trusted: GitHub refuses the whole review over one bad line, and +# review-anchors.sh would move it into the body, which is text the user did not +# confirm. + +set -eu + +usage='usage: post-review.sh [repo]' +pr=${1:?$usage} +payload=${2:?$usage} +since=${3:?$usage} +repo=${4:-sysprog21/codetrial} + +# review-anchors.sh refuses a pull request that is not a number or a payload it +# cannot read, with exit 2 and its own message, before any call to GitHub; set +# -e passes that exit on. Its count of inline comments goes to stderr too. +checked=$(sh "$(dirname "$0")/review-anchors.sh" "$pr" "$payload" "$repo") +want=$(jq -er '.commit_id | strings' "$payload") || { + echo "post-review.sh: $payload names no commit_id" >&2 + exit 2 +} + +# Without an event GitHub keeps the review pending, visible to nobody else, and +# still answers with its URL. +jq -e '.event | IN("COMMENT", "APPROVE", "REQUEST_CHANGES")' "$payload" \ + > /dev/null || { + echo "post-review.sh: $payload names no event, so the review would stay pending" >&2 + exit 2 +} +jq -e '.body | strings | length > 0' "$payload" > /dev/null || { + echo "post-review.sh: $payload has no body, which GitHub requires" >&2 + exit 2 +} +before=$(jq '.comments // [] | length' "$payload") +after=$(printf '%s\n' "$checked" | jq '.comments | length') +if [ "$before" != "$after" ]; then + echo "post-review.sh: $((before - after)) comment(s) on #$pr fall outside the diff; run review-anchors.sh and confirm the moved text" >&2 + exit 3 +fi + +# Last, right before the post, so a push or a comment made while the diff was +# read is still caught. The read and the post are two calls, so a change in the +# second or so between them is not; GitHub offers no conditional post. +now=$(gh pr view "$pr" --repo "$repo" --json headRefOid,updatedAt \ + --jq '.headRefOid + " " + .updatedAt') +head=${now% *} +updated=${now#* } +if [ "$head" != "$want" ]; then + echo "post-review.sh: #$pr moved from $want to $head; review the new head" >&2 + exit 3 +fi +if [ "$updated" != "$since" ]; then + echo "post-review.sh: #$pr was updated at $updated, not $since; reread it" >&2 + exit 3 +fi + +gh api -X POST "repos/$repo/pulls/$pr/reviews" --input "$payload" --jq .html_url diff --git a/.claude/skills/codetrial-verify/review.md b/.claude/skills/codetrial-verify/review.md index 24d00f9f..5bc05ab5 100644 --- a/.claude/skills/codetrial-verify/review.md +++ b/.claude/skills/codetrial-verify/review.md @@ -164,30 +164,34 @@ walkthrough (`CONTRIBUTING.md`, Pull requests): each posted finding is one inline comment in plain words, under the GitHub rule in [codetrial-conventions](../codetrial-conventions/SKILL.md). -Post through gh-review when it is installed. Otherwise post the approved -findings as one review, so the author gets one notification. Refetch -`headRefOid` first and reread any line that moved, then check every anchor -against the diff, which moves a finding on a line outside every hunk into the -body instead of letting it fail the whole review: +When the review ran through gh-review, post through it. A review done here +posts as one review, so the author gets one notification. Check every anchor +against the diff first, which moves a finding on a line outside every hunk +into the body instead of letting it fail the whole review, and show the user +the checked text, moved findings included, with the PR's `updatedAt`: ```sh .claude/skills/codetrial-verify/review-anchors.sh "$pr" "$SCRATCH/review-$pr.json" \ > "$SCRATCH/review-$pr.checked.json" ``` -Post the checked file. The review is `event: -"COMMENT"` on `commit_id` (that head), with a one-sentence `body`, which GitHub -requires, and one entry in `comments` per finding, each with `path`, `line` (the -line in the file), `side` (`RIGHT` for an added or unchanged line, `LEFT` for a -deleted one) and its own `body`: +The review is `event: "COMMENT"` on `commit_id` (the head the findings were +read at), with a one-sentence `body`, which GitHub requires, and one entry in +`comments` per finding, each with `path`, `line` (the line in the file), `side` +(`RIGHT` for an added or unchanged line, `LEFT` for a deleted one) and its own +`body`. A finding that sits in a moved line reads differently in the body, so +reword one that leaned on "this line" before showing it. Once the user +confirms, post the checked file: ```sh -gh api -X POST "repos/sysprog21/codetrial/pulls/$pr/reviews" \ - --input "$SCRATCH/review-$pr.checked.json" +.claude/skills/codetrial-verify/post-review.sh "$pr" \ + "$SCRATCH/review-$pr.checked.json" "$updated_at" ``` -A line outside every hunk is refused with 422, which the anchor check above -prevents. +It posts nothing and exits 3 when the head moved, the PR was updated since +`$updated_at`, or a comment would still land outside the diff. A moved head +means the findings are read again at the new one; an update at the same head +means the new comment or edit is read first, since it may answer a finding. When posting is not allowed in this run, leave the payload at `$SCRATCH/review-$pr.json` and say so. diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 65a15826..722b1cb8 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -153,8 +153,11 @@ reuses its subject. An issue title names the symptom or the outcome instead: Issue and pull request bodies are read in a browser, so write each paragraph as one line. The 72-column wrap is for commit messages only. A pull request that -finishes an issue ends with `Closes #N` alone on the last line; one that only -touches it ends with `Refs #N`. +finishes an issue ends with `Closes #N` alone on the last line. Avoid `Refs #N`: +a reference that closes nothing is easy to add to every pull request and tells +the reviewer little. Use it only beside a `Closes #N`, or in a body that already +states the motivation and the implementation in full, so the reference adds +context rather than standing in for it. A review thread carries a correction, a measurement, or nothing: no pasted agent walkthroughs, no severity tables, no re-summarizing a diff that GitHub diff --git a/tests/test_skill_scripts.py b/tests/test_skill_scripts.py index 8495a191..3adceb1a 100644 --- a/tests/test_skill_scripts.py +++ b/tests/test_skill_scripts.py @@ -39,6 +39,8 @@ *collaborators*) printf 'maint\\nviewer\\n' ;; *actions/runs*) cat "$FAKE_RUNS" ;; "api graphql --paginate"*) cat "$FAKE_PAGE" ;; + "run list "*) [ -n "${FAKE_MAIN_FAILED:-}" ] || exit 1; echo 500 ;; + "run view 500 "*) printf '%s\\n' "$FAKE_MAIN_FAILED" ;; "api graphql"*) cat "$FAKE_AGAIN" ;; "pr view 404 "*) echo "no such pull request" >&2; exit 1 ;; "pr view"*) printf '{"pr":%s,"state":"MERGED","merge":"m%s"}\\n' "$3" "$3" ;; @@ -223,6 +225,11 @@ def sweep(self, script, page, previous=None, runs=(), again=(), env=()): "FAKE_AGAIN": json.dumps(list(again)), } args = ["sysprog21/codetrial"] + # ci-failure.sh as the PR sweep runs it: a PR in $STALE is a stale base. + files["PR_SWEEP_CI"] = ( + 'case " $STALE " in *" $1 "*) s=true ;; *) s=false ;; esac\n' + 'printf \'{"pr":%s,"stale_base":%s}\\n\' "$1" "$s"\n' + ) if previous is not None: files["PREVIOUS"] = json.dumps(previous) args += ["7", "$PREVIOUS"] @@ -308,6 +315,32 @@ def test_idle_days_is_unknown_when_no_act_of_the_assignee_was_read(self): self.assertIsNone(claim["checked_at"]) self.assertIsNone(claim["stale"]) + def test_an_open_pr_that_closes_the_issue_is_named_on_its_entry(self): + here = {"nameWithOwner": "sysprog21/codetrial"} + fork = {"nameWithOwner": "someone/codetrial"} + closing = { + "nodes": [ + {"number": 7, "repository": here}, + {"number": 9, "repository": fork}, + ] + } + swept = self.run_issues( + issue(1, "Body" + FILLER, closedByPullRequestsReferences=closing), + issue(2, "Body" + FILLER, comments=listed(comment("maint", 10))), + issue( + 3, + "Body" + FILLER, + comments=listed(comment("maint", 10)), + closedByPullRequestsReferences=closing, + ), + ) + self.assertEqual( + {e["issue"]: e["closing_prs"] for e in swept["unanswered"]}, {1: [7]} + ) + self.assertEqual( + {e["issue"]: e["closing_prs"] for e in swept["waiting"]}, {2: [], 3: [7]} + ) + def test_more_body_edits_than_read_marks_the_issue_truncated(self): edits = {"totalCount": 21, "nodes": []} swept = self.run_issues(issue(1, "Body" + FILLER, userContentEdits=edits)) @@ -364,6 +397,186 @@ def test_grade_follows_what_github_records(self): with self.subTest(grade=grade): self.assertEqual(grades[node["number"]], grade) + def test_waits_on_names_whose_move_it_is_and_review_queues_the_maintainers(self): + cases = { + 1: (pull(1, commits=GREEN), "maintainer"), + 2: (pull(2, commits=checked(("gates (rust)", "FAILURE"))), "author"), + 3: (pull(3, commits=checked(("gates (rust)", None))), "ci"), + 4: ( + pull( + 4, + commits=checked( + ("gates (rust)", "SUCCESS"), ("check", "ACTION_REQUIRED") + ), + ), + "approval", + ), + # The author pushed past the changes request: a re-review is owed. + 5: ( + pull(5, commits=GREEN, reviewDecision="CHANGES_REQUESTED"), + "maintainer", + ), + 6: (pull(6, commits=GREEN, reviewThreads=listed(thread())), "author"), + 7: (pull(7, commits=GREEN, isDraft=True), "author"), + 8: (pull(8, commits=GREEN, mergeable="CONFLICTING"), "author"), + 9: (pull(9, commits=checked(("check", "SUCCESS"))), "author"), + 12: (pull(12, commits=GREEN, author={"login": "viewer"}), "maintainer"), + 10: (pull(10, commits=GREEN, mergeable="UNKNOWN"), "ci"), + # A held run outranks the conflict: nothing runs until approved. + 11: ( + pull( + 11, + mergeable="CONFLICTING", + commits=checked(("check", "ACTION_REQUIRED")), + ), + "approval", + ), + } + swept = self.run_pulls( + *(node for node, _ in cases.values()), + env={"PR_SWEEP_RETRY_DELAY": "0"}, + ) + self.assertEqual( + {entry["pr"]: entry["waits_on"] for entry in swept["status"]}, + {number: who for number, (_, who) in cases.items()}, + ) + # The viewer's own PR waits on nobody else's review. + self.assertEqual(sorted(swept["review"]), [1, 5]) + + def test_a_stale_base_is_asked_to_rebase_under_the_conflicting_limits(self): + failing = checked(("gates (portable)", "FAILURE"), ("check", "FAILURE")) + swept = self.run_pulls( + pull(1, commits=failing), + # ci-failure.sh says its failure is its own. + pull(2, commits=failing), + pull( + 3, commits=failing, comments=listed(request("C3", 1, "Rebase, please.")) + ), + pull(4, commits=failing, baseRefName="feature"), + pull(5, commits=failing, isDraft=True), + pull(6, commits=failing, author={"login": "viewer"}), + pull(7, commits=failing, comments={"totalCount": 101, "nodes": []}), + # Already listed for its conflict, so asked once. + pull(8, commits=failing, mergeable="CONFLICTING"), + # Green: nothing to rebase for. + pull(9, commits=GREEN), + env={"STALE": "1 3 4 5 6 7 8 9"}, + ) + self.assertEqual( + sorted((entry["pr"], entry["reason"]) for entry in swept["rebase"]), + [(1, "stale_base"), (8, "conflicts")], + ) + + def test_a_job_main_also_fails_stays_the_authors_and_says_so(self): + # The lane runs several gates, so a shared name is not a shared cause. + swept = self.run_pulls( + pull(1, commits=checked(("gates (rust)", "FAILURE"), ("check", "FAILURE"))), + env={"FAKE_MAIN_FAILED": json.dumps(["gates (rust)", "check"])}, + ) + [entry] = swept["status"] + self.assertEqual(entry["waits_on"], "author") + self.assertIn( + "failing: check, gates (rust); main also fails check, gates (rust)", + entry["why"], + ) + self.assertEqual(swept["review"], []) + + def test_a_failed_diagnosis_never_asks_for_a_rebase(self): + failing = checked(("gates (portable)", "FAILURE")) + cases = { + # Says stale, then exits non-zero: not trusted. + "exit": 'echo \'{"pr":1,"stale_base":true}\'; exit 1\n', + # Answers for another PR. + "other pr": 'echo \'{"pr":2,"stale_base":true}\'\n', + } + for name, stub in cases.items(): + with self.subTest(name): + page = { + "repository": { + "defaultBranchRef": {"name": "main"}, + "pullRequests": { + "pageInfo": {"hasNextPage": False}, + "nodes": [pull(1, commits=failing)], + }, + } + } + files = { + "FAKE_PAGE": json.dumps({"data": page}), + "FAKE_RUNS": "", + "FAKE_AGAIN": "[]", + "PR_SWEEP_CI": stub, + } + result = run_fake(PR_SWEEP, ["sysprog21/codetrial"], FAKE_GH, files) + self.assertEqual(result.returncode, 0, result.stderr) + self.assertEqual(json.loads(result.stdout)["rebase"], []) + + def test_moved_names_a_change_of_whose_move_it_is(self): + node = pull(1, commits=GREEN) + first = self.run_pulls(node) + earlier = first | { + "status": [entry | {"waits_on": "author"} for entry in first["status"]] + } + swept = self.run_pulls(node, previous=earlier) + self.assertEqual( + swept["moved"], + [{"pr": 1, "changed": {"waits_on": ["author", "maintainer"]}}], + ) + + def test_refs_only_lists_issues_the_merge_leaves_open(self): + asker = request("C1", 1, "Make the last line `Closes #7`.") + outsider = comment("student", 1, "Closes #8 would be better.") | { + "id": "C2", + "isMinimized": False, + } + swept = self.run_pulls( + pull(1, commits=GREEN, body="Shows the camera.\n\nRefs #141"), + # It closes #5, so its Refs #6 stands (CONTRIBUTING.md). + pull( + 2, + commits=GREEN, + body="Refs #6", + closingIssuesReferences=listed({"number": 5}), + ), + pull(3, commits=GREEN, body="Refs: #7", comments=listed(asker)), + pull(4, commits=GREEN, body="Refs #8", comments=listed(outsider)), + # Asked about #70, which is not #7. + pull( + 5, + commits=GREEN, + body="Refs #7", + comments=listed(request("C3", 1, "Closes #70")), + ), + pull(6, commits=GREEN, body=None), + pull( + 7, + commits=GREEN, + body="Refs sysprog21/codetrial#9\n" + "Refs https://github.com/sysprog21/codetrial/issues/10", + comments=listed( + request( + "C4", + 1, + "Fixes https://github.com/sysprog21/codetrial/issues/10", + ) + ), + ), + pull(8, commits=GREEN, body="Refs #11, #12 and #13"), + ) + self.assertEqual( + swept["refs_only"], + [ + {"pr": 1, "issue": 141, "asked": False}, + {"pr": 3, "issue": 7, "asked": True}, + {"pr": 4, "issue": 8, "asked": False}, + {"pr": 5, "issue": 7, "asked": False}, + {"pr": 7, "issue": 9, "asked": False}, + {"pr": 7, "issue": 10, "asked": True}, + {"pr": 8, "issue": 11, "asked": False}, + {"pr": 8, "issue": 12, "asked": False}, + {"pr": 8, "issue": 13, "asked": False}, + ], + ) + def test_human_thread_lowers_the_grade_and_a_bot_thread_only_to_medium(self): grades = self.grades( pull(1, commits=GREEN, reviewThreads=listed(thread())), @@ -499,6 +712,7 @@ def test_previous_sweep_reports_only_what_moved_and_what_left(self): "changed": { "mergeable": ["MERGEABLE", "CONFLICTING"], "grade": ["high", "medium"], + "waits_on": ["maintainer", "author"], }, } ], @@ -522,7 +736,9 @@ def test_a_check_finishing_moves_a_pr_even_when_its_grade_holds(self): first = self.run_pulls(pull(1, commits=waiting, **decided)) swept = self.run_pulls(pull(1, commits=GREEN, **decided), previous=first) self.assertEqual([entry["pr"] for entry in swept["moved"]], [1]) - self.assertEqual(list(swept["moved"][0]["changed"]), ["checks"]) + # The grade holds; the move passes from CI to a re-review. + self.assertEqual(list(swept["moved"][0]["changed"]), ["waits_on", "checks"]) + self.assertEqual(swept["moved"][0]["changed"]["waits_on"], ["ci", "maintainer"]) def test_unknown_mergeability_is_asked_again_for_those_prs_only(self): swept = self.run_pulls( @@ -608,7 +824,10 @@ def test_pr_with_no_commits_does_not_end_the_sweep(self): def test_conflicting_pr_without_a_request_is_asked_to_rebase(self): swept = self.run_pulls(pull(1, mergeable="CONFLICTING")) - self.assertEqual(swept["rebase"], [{"pr": 1, "author": "contributor"}]) + self.assertEqual( + swept["rebase"], + [{"pr": 1, "author": "contributor", "reason": "conflicts"}], + ) def test_current_fixed_request_is_not_repeated_or_hidden(self): swept = self.run_pulls( @@ -724,11 +943,18 @@ def test_branch_meets_the_same_issue_shared_files_and_a_held_issue(self): # main also fails, and a check from an app outside GitHub Actions. FAKE_GH_CI = """#!/bin/sh case "$*" in - "pr view 5 "*) echo h1 ;; + "pr view 5 "*) + while [ "$1" != --jq ]; do shift; done + pr=${FAKE_PR:-} + [ -n "$pr" ] || pr='{"headRefOid":"h1","files":[],"changedFiles":0}' + printf '%s\\n' "$pr" | jq -c "$2" ;; "pr checks 5 "*) cat "$FAKE_CHECKS"; exit 1 ;; - "run list "*"--event push "*) echo 500 ;; - "run view 500 "*) echo '["check"]' ;; - "run view 7 --repo sysprog21/codetrial --json headSha"*) echo h1 ;; + "run list "*"--event push "*) echo "500 ${FAKE_MAIN_AT:-2026-10-22T05:00:00Z}" ;; + "run view 500 "*) + if [ -n "${FAKE_MAIN:-}" ]; then printf '%s\\n' "$FAKE_MAIN" + else printf '%s\\n' '[{"name":"check","conclusion":"failure"},{"name":"gates (portable)","conclusion":"success"}]' + fi ;; + "run view 7 --repo sysprog21/codetrial --json headSha"*) echo "h1 2026-10-22T01:00:00Z" ;; "run view 7 "*"--job 70 "*) cat "$FAKE_LOG" ;; "run view 7 "*"--job 73 "*) printf 'gates (browser)\\tlobby\\t2026-10-22T00:00:00.0Z SyntaxError: Unexpected token\\n' ;; "run view 7 "*"--job 74 "*) : ;; @@ -868,6 +1094,94 @@ def test_ci_failure_names_the_gates_and_what_main_shares(self): self.assertIn("error: 3 vulnerabilities found!", lane["errors"]) self.assertEqual((collector["name"], collector["on_main"]), ("check", True)) self.assertEqual(app, {"name": "cubic", "link": "https://www.cubic.dev/pr/5"}) + # indent failed beside cargo-audit, so a rebase would not clear it. + self.assertIs(found["stale_base"], False) + + def test_an_audit_failure_main_has_fixed_is_a_stale_base(self): + checks = [ + { + "name": name, + "state": "FAILURE", + "bucket": "fail", + "link": f"https://github.com/sysprog21/codetrial/actions/runs/7/job/{job}", + } + for name, job in (("gates (portable)", 70), ("check", 71)) + ] + + def pr_with(*paths, changed=None): + return json.dumps( + { + "headRefOid": "h1", + "files": [{"path": path} for path in paths], + "changedFiles": len(paths) if changed is None else changed, + } + ) + + cubic = { + "name": "cubic", + "state": "FAILURE", + "bucket": "fail", + "link": "https://www.cubic.dev/pr/5", + } + # main skipped the lane: absent from its failures, yet not a pass. + skipped = json.dumps([{"name": "gates (portable)", "conclusion": "skipped"}]) + log = CI_LOG.replace("cargo-audit indent", "cargo-audit") + cases = { + "stale": (checks, {}, True), + "touches Cargo": (checks, {"FAKE_PR": pr_with("Cargo.lock")}, False), + "changes the gate": ( + checks, + {"FAKE_PR": pr_with("scripts/test.sh")}, + False, + ), + "changes CI": ( + checks, + {"FAKE_PR": pr_with(".github/workflows/check.yml")}, + False, + ), + # gh read 100 of 101 files, so the lockfile may be the one cut. + "more files than read": ( + checks, + { + "FAKE_PR": pr_with( + *(f"web/f{n}.js" for n in range(100)), changed=101 + ) + }, + False, + ), + "main skipped the lane": (checks, {"FAKE_MAIN": skipped}, False), + "a check outside Actions fails": (checks + [cubic], {}, False), + # main last ran before the PR did, so a new advisory fails both. + "main ran first": ( + checks, + {"FAKE_MAIN_AT": "2026-10-21T00:00:00Z"}, + False, + ), + "a lane is still pending": ( + checks + + [ + { + "name": "gates (rust)", + "state": "IN_PROGRESS", + "bucket": "pending", + "link": "", + } + ], + {}, + False, + ), + } + for name, (listed_checks, env, stale) in cases.items(): + with self.subTest(name): + result = run_fake( + VERIFY / "ci-failure.sh", + [5], + FAKE_GH_CI, + {"FAKE_CHECKS": json.dumps(listed_checks), "FAKE_LOG": log}, + env=env, + ) + self.assertEqual(result.returncode, 0, result.stderr) + self.assertIs(json.loads(result.stdout)["stale_base"], stale) def test_a_checks_lookup_that_fails_is_not_read_as_a_clean_pr(self): # The fake exits 1 with nothing on stdout, as gh does when the API @@ -885,6 +1199,7 @@ def test_a_pr_that_is_not_a_number_is_refused_before_any_call(self): for script, args in ( ("ci-failure.sh", ["abc"]), ("review-anchors.sh", ["abc", "payload.json"]), + ("post-review.sh", ["abc", "payload.json", "2026-10-22T00:00:00Z"]), ): with self.subTest(script=script): # The fake gh fails every call, so only the script's own @@ -939,6 +1254,65 @@ def note(path, line, side, text): ) +# The PR as post-review.sh rereads it before posting, and the post itself. +FAKE_GH_POST = """#!/bin/sh +case "$*" in + "pr view 5 "*) echo "${FAKE_HEAD:-h1} ${FAKE_UPDATED:-T1}" ;; + "pr diff 5 "*) cat "$FAKE_DIFF" ;; + "api -X POST repos/sysprog21/codetrial/pulls/5/reviews "*) + echo https://github.com/sysprog21/codetrial/pull/5#pullrequestreview-1 ;; + *) echo "unexpected gh call: $*" >&2; exit 1 ;; +esac +""" + + +@unittest.skipUnless(shutil.which("jq"), "jq is not installed") +class PostReviewTests(unittest.TestCase): + def post(self, line=11, env=(), event="COMMENT"): + payload = { + "event": event, + "commit_id": "h1", + "body": "Notes.", + "comments": [ + {"path": "web/a.js", "line": line, "side": "RIGHT", "body": "x"} + ], + } + return run_fake( + VERIFY / "post-review.sh", + [5, "$FAKE_PAYLOAD", "T1"], + FAKE_GH_POST, + {"FAKE_DIFF": DIFF, "FAKE_PAYLOAD": json.dumps(payload)}, + env=env, + ) + + def test_posts_when_the_pr_is_where_the_user_read_it(self): + result = self.post() + self.assertEqual(result.returncode, 0, result.stderr) + self.assertIn("pullrequestreview-1", result.stdout) + + def test_refuses_a_payload_without_an_event(self): + # GitHub would keep it as a pending review only its writer sees. + result = self.post(event=None) + self.assertEqual((result.returncode, result.stdout), (2, "")) + self.assertIn("names no event", result.stderr) + + def test_refuses_without_posting_when_anything_moved(self): + cases = { + "head": ({"FAKE_HEAD": "h2"}, 11, "moved from h1 to h2"), + # Also what a second run meets after a post: the post itself + # moves updatedAt, so a rerun refuses instead of posting twice. + "updated": ({"FAKE_UPDATED": "T2"}, 11, "was updated at T2"), + "anchor": ({}, 40, "outside the diff"), + } + for name, (env, line, said) in cases.items(): + with self.subTest(name): + result = self.post(line, env) + # The fake answers a post with the URL, so empty stdout means + # nothing was posted. + self.assertEqual((result.returncode, result.stdout), (3, "")) + self.assertIn(said, result.stderr) + + GOOD_TARGET = """\ --- name: fixture From 4886d53a696d4364c774e0adbde08293e6b27ae4 Mon Sep 17 00:00:00 2001 From: Jim Huang Date: Thu, 22 Oct 2026 14:42:47 +0800 Subject: [PATCH 2/2] Tie automatic writes to the state that was read A stale-base diagnosis now counts only for the head the sweep read, and post-review.sh posts the checked text instead of rereading a file that may have changed since its checks. closing_prs marks an issue truncated past ten closing PRs, waiting.md says a closing reference narrows the candidates rather than settling them, and review.md records the updatedAt that post-review.sh compares against. --- .../skills/codetrial-triage/issue-sweep.sh | 9 +++---- .claude/skills/codetrial-triage/pr-sweep.sh | 23 +++++++++++------- .claude/skills/codetrial-triage/waiting.md | 8 ++++--- .../skills/codetrial-verify/post-review.sh | 5 +++- .claude/skills/codetrial-verify/review.md | 1 + tests/test_skill_scripts.py | 24 ++++++++++++++----- 6 files changed, 48 insertions(+), 22 deletions(-) diff --git a/.claude/skills/codetrial-triage/issue-sweep.sh b/.claude/skills/codetrial-triage/issue-sweep.sh index 4efccaf9..ec2d2c4d 100755 --- a/.claude/skills/codetrial-triage/issue-sweep.sh +++ b/.claude/skills/codetrial-triage/issue-sweep.sh @@ -37,8 +37,8 @@ # is the newest comment; stale is null when the history is cut, # and idle_days too when none of their acts is in what was read # truncated issues with more than 100 comments or cross-references, 10 -# assignees, 20 assignments or 20 body edits, whose lists above -# may miss what was cut +# assignees, 20 assignments, 20 body edits or 10 closing PRs, +# whose lists above may miss what was cut set -eu @@ -58,7 +58,7 @@ query='query($owner: String!, $name: String!, $endCursor: String) { } assignees(first: 10) { totalCount nodes { login } } closedByPullRequestsReferences(first: 10, includeClosedPrs: false) { - nodes { number repository { nameWithOwner } } + totalCount nodes { number repository { nameWithOwner } } } assigned: timelineItems(last: 20, itemTypes: [ASSIGNED_EVENT]) { totalCount @@ -131,7 +131,8 @@ def ink: [limit(40; split("\n")[] | select(startswith("#") | not) def maint: [.comments.nodes[] | select(human and staff)]; def cut: .comments.totalCount > 100 or .timelineItems.totalCount > 100 or .assignees.totalCount > 10 or .assigned.totalCount > 20 - or .userContentEdits.totalCount > 20; + or .userContentEdits.totalCount > 20 + or (.closedByPullRequestsReferences.totalCount // 0) > 10; def prs: [.timelineItems.nodes[] | select(.source.number != null and .source.repository.nameWithOwner == $repo)]; # A PR in another repository may close the issue too; as in prs, its number diff --git a/.claude/skills/codetrial-triage/pr-sweep.sh b/.claude/skills/codetrial-triage/pr-sweep.sh index d9812d83..6096083d 100755 --- a/.claude/skills/codetrial-triage/pr-sweep.sh +++ b/.claude/skills/codetrial-triage/pr-sweep.sh @@ -440,7 +440,8 @@ def refs: (ref_prefix + "[0-9]+\\b") as $one and (asked_here | not) and any($status[]; .pr == $n and .waits_on == "author" and (.checks.failing | length > 0))) - | {pr: .number, author: .author.login, reason: "stale_base"}], + | {pr: .number, author: .author.login, reason: "stale_base", + head: .headRefOid}], review: [$status[] | select(.waits_on == "maintainer" and .author != $me) | .pr], refs_only: [$prs[] | . as $p @@ -505,13 +506,19 @@ done | jq -s .) # which the sweep does not. A run that fails leaves its PR out, since this list # posts without asking. ci=${PR_SWEEP_CI:-$(dirname "$0")/../codetrial-verify/ci-failure.sh} -stale=$(printf '%s\n' "$out" | jq -r '.stale_candidates[].pr' | while read -r n; do - if found=$(sh "$ci" "$n" "$repo" < /dev/null 2> /dev/null); then - printf '%s\n' "$found" | jq -c --argjson n "$n" \ - 'select(.pr == $n and .stale_base == true) | .pr' || true - fi -done | jq -s .) + +# A diagnosis of another head, after a push mid-sweep, says nothing about the +# head this sweep read, so it is dropped rather than trusted. +stale=$(printf '%s\n' "$out" | jq -r '.stale_candidates[] | "\(.pr) \(.head)"' \ + | while read -r n head; do + if found=$(sh "$ci" "$n" "$repo" < /dev/null 2> /dev/null); then + printf '%s\n' "$found" | jq -c --argjson n "$n" --arg head "$head" \ + 'select(.pr == $n and .head == $head and .stale_base == true) + | .pr' || true + fi + done | jq -s .) printf '%s\n' "$out" | jq --argjson states "$states" --argjson stale "$stale" \ 'if .gone == null then . else .gone = $states end - | .rebase += [.stale_candidates[] | select(.pr | IN($stale[]))] + | .rebase += [.stale_candidates[] | select(.pr | IN($stale[])) + | del(.head)] | del(.stale_candidates)' diff --git a/.claude/skills/codetrial-triage/waiting.md b/.claude/skills/codetrial-triage/waiting.md index f6ac01a8..2f009159 100644 --- a/.claude/skills/codetrial-triage/waiting.md +++ b/.claude/skills/codetrial-triage/waiting.md @@ -17,9 +17,11 @@ PR is read and does what the issue asks: `open_prs` holds every PR that cross-referenced the issue, and a mention alone does not suspend the reporter's reminder. `closing_prs` on the `unanswered` or `waiting` entry itself names the open PRs whose merge closes the issue, whoever wrote them, -which settles the candidates without reading every cross-reference; reading -that one PR's title and scope still decides it. Such an entry gets neither a -reminder nor a first answer on the issue (#370 and #385 sat in +which narrows the candidates without reading every cross-reference. A closing +reference says only what the merge will close, so read each listed PR's scope +and implementation; when none does what the issue asks, the normal +`unanswered` or `waiting` rule applies. Only an entry whose PR does gets +neither a reminder nor a first answer on the issue (#370 and #385 sat in `unanswered`, #159 and #298 in overdue `waiting`, all with PRs open): - `unanswered`: no maintainer has spoken. Triage these first, oldest first, with diff --git a/.claude/skills/codetrial-verify/post-review.sh b/.claude/skills/codetrial-verify/post-review.sh index b322181c..e6f37d2a 100755 --- a/.claude/skills/codetrial-verify/post-review.sh +++ b/.claude/skills/codetrial-verify/post-review.sh @@ -67,4 +67,7 @@ if [ "$updated" != "$since" ]; then exit 3 fi -gh api -X POST "repos/$repo/pulls/$pr/reviews" --input "$payload" --jq .html_url +# The checked text, not the file again, so a rewrite of the file after the +# checks above cannot change what is posted. +printf '%s\n' "$checked" | gh api -X POST "repos/$repo/pulls/$pr/reviews" \ + --input - --jq .html_url diff --git a/.claude/skills/codetrial-verify/review.md b/.claude/skills/codetrial-verify/review.md index 5bc05ab5..06ba4a28 100644 --- a/.claude/skills/codetrial-verify/review.md +++ b/.claude/skills/codetrial-verify/review.md @@ -173,6 +173,7 @@ the checked text, moved findings included, with the PR's `updatedAt`: ```sh .claude/skills/codetrial-verify/review-anchors.sh "$pr" "$SCRATCH/review-$pr.json" \ > "$SCRATCH/review-$pr.checked.json" +updated_at=$(gh pr view "$pr" --repo sysprog21/codetrial --json updatedAt --jq .updatedAt) ``` The review is `event: "COMMENT"` on `commit_id` (the head the findings were diff --git a/tests/test_skill_scripts.py b/tests/test_skill_scripts.py index 3adceb1a..94dc78e7 100644 --- a/tests/test_skill_scripts.py +++ b/tests/test_skill_scripts.py @@ -228,7 +228,8 @@ def sweep(self, script, page, previous=None, runs=(), again=(), env=()): # ci-failure.sh as the PR sweep runs it: a PR in $STALE is a stale base. files["PR_SWEEP_CI"] = ( 'case " $STALE " in *" $1 "*) s=true ;; *) s=false ;; esac\n' - 'printf \'{"pr":%s,"stale_base":%s}\\n\' "$1" "$s"\n' + 'printf \'{"pr":%s,"head":"%s","stale_base":%s}\\n\' ' + '"$1" "${STALE_HEAD:-head$1}" "$s"\n' ) if previous is not None: files["PREVIOUS"] = json.dumps(previous) @@ -341,6 +342,13 @@ def test_an_open_pr_that_closes_the_issue_is_named_on_its_entry(self): {e["issue"]: e["closing_prs"] for e in swept["waiting"]}, {2: [], 3: [7]} ) + def test_more_closing_prs_than_read_marks_the_issue_truncated(self): + closing = {"totalCount": 11, "nodes": []} + swept = self.run_issues( + issue(1, "Body" + FILLER, closedByPullRequestsReferences=closing) + ) + self.assertEqual(swept["truncated"], [1]) + def test_more_body_edits_than_read_marks_the_issue_truncated(self): edits = {"totalCount": 21, "nodes": []} swept = self.run_issues(issue(1, "Body" + FILLER, userContentEdits=edits)) @@ -485,9 +493,11 @@ def test_a_failed_diagnosis_never_asks_for_a_rebase(self): failing = checked(("gates (portable)", "FAILURE")) cases = { # Says stale, then exits non-zero: not trusted. - "exit": 'echo \'{"pr":1,"stale_base":true}\'; exit 1\n', + "exit": 'echo \'{"pr":1,"head":"head1","stale_base":true}\'; exit 1\n', # Answers for another PR. - "other pr": 'echo \'{"pr":2,"stale_base":true}\'\n', + "other pr": 'echo \'{"pr":2,"head":"head1","stale_base":true}\'\n', + # Diagnosed a head pushed after the sweep read this one. + "other head": 'echo \'{"pr":1,"head":"later","stale_base":true}\'\n', } for name, stub in cases.items(): with self.subTest(name): @@ -1259,8 +1269,10 @@ def note(path, line, side, text): case "$*" in "pr view 5 "*) echo "${FAKE_HEAD:-h1} ${FAKE_UPDATED:-T1}" ;; "pr diff 5 "*) cat "$FAKE_DIFF" ;; - "api -X POST repos/sysprog21/codetrial/pulls/5/reviews "*) - echo https://github.com/sysprog21/codetrial/pull/5#pullrequestreview-1 ;; + # Only the checked text on stdin: posting the file again would let a + # rewrite after the checks through. + "api -X POST repos/sysprog21/codetrial/pulls/5/reviews --input - "*) + jq -r '"https://github.com/sysprog21/codetrial/pull/5#pullrequestreview-" + .commit_id' ;; *) echo "unexpected gh call: $*" >&2; exit 1 ;; esac """ @@ -1288,7 +1300,7 @@ def post(self, line=11, env=(), event="COMMENT"): def test_posts_when_the_pr_is_where_the_user_read_it(self): result = self.post() self.assertEqual(result.returncode, 0, result.stderr) - self.assertIn("pullrequestreview-1", result.stdout) + self.assertIn("pullrequestreview-h1", result.stdout) def test_refuses_a_payload_without_an_event(self): # GitHub would keep it as a pending review only its writer sees.