From 7f2cdf66d9605dcb6b447697809dcc3dd9b090a0 Mon Sep 17 00:00:00 2001 From: bdchatham Date: Sun, 6 Sep 2026 15:33:38 -0700 Subject: [PATCH 1/5] feat(seidroid-review): post the findings as one review Placement posted one `POST /pulls/{pr}/comments` per finding, so an author with ten findings read ten ungrouped threads and got ten notifications. The findings the diff can hold now ride in a single `pulls/{pr}/reviews` call carrying the review body and every inline comment together, which is how `ai-review.yml` posts. The three rungs hold. A line the diff covers goes on the line, in the batch. A line the diff does not cover goes on its file, which the reviews API has no field for and which therefore keeps a call of its own. A finding that reaches neither travels in the summary. `on_line`, `on_file` and `unplaced` count the same events and reach the verdict comment and the resolve gate unchanged. The batch is all-or-nothing, so the set of commentable lines is read from the pull request's own diff before the call rather than discovered by refusal. A 422 retries each finding on its own; any other failure sends those findings to the summary, because a write that may have landed must not be repeated. Co-Authored-By: Claude Opus 5 (1M context) --- .github/workflows/seidroid-review.yml | 220 ++++++++++++++++++++++---- 1 file changed, 188 insertions(+), 32 deletions(-) diff --git a/.github/workflows/seidroid-review.yml b/.github/workflows/seidroid-review.yml index 19a0256..9d03057 100644 --- a/.github/workflows/seidroid-review.yml +++ b/.github/workflows/seidroid-review.yml @@ -1604,6 +1604,16 @@ jobs: # its file; an untouched file has nowhere to go and is named in the summary. # So the cost of a review that sees past the hunks is paid in placement, # not in lost findings. + # + # Every finding the diff can hold rides in ONE review, which is what + # ai-review posts and what the author reads top to bottom on a single + # notification. Only the middle rung costs a call of its own: the reviews + # API carries a line comment and has no field for a file-level one. + # + # That review is COMMENTED and its body opens with no verdict marker, so the + # withdrawal below -- which selects a CHANGES_REQUESTED review whose body + # opens with VERDICT_MARKER -- does not select it, and the position this run + # takes stays the position step's alone. if: ${{ inputs.mode == 'review' && !cancelled() && steps.drive.outputs.verdict_produced == 'true' }} continue-on-error: true @@ -1633,7 +1643,7 @@ jobs: # recorded. A comment on any other commit points at code the review never # saw, so the head is not read again here. # - # Empty when that record failed. Both calls below need a commit id and the + # Empty when that record failed. Every call below needs a commit id and the # API rejects an empty one, so no comment can reach the diff and every # finding takes the third rung of the ladder above: the summary. The reader # loses the placement, not the finding. @@ -1642,40 +1652,186 @@ jobs: echo "::warning::the reviewed commit was not recorded on $REPO#$PR; every finding goes to the summary instead of the diff" fi on_line=0 on_file=0 unplaced=0 + + # The third rung. Each line opens with a dash and a backtick, which is + # what the summary counts when it cuts the list to fit GitHub's limit. + # shellcheck disable=SC2016 # the backticks are markdown, not a substitution + to_summary() { + printf -- '- `%s:%s` (%s) — %s\n' "$1" "$2" "$4" "$5" >> "$NOTE" + unplaced=$((unplaced+1)) + return 0 + } + + # The second rung, one call per finding. A file-level comment is a field + # the reviews API does not carry, so this one cannot ride in the batch + # below. The file can still be in the pull request, and a comment on it + # reaches the reviewer in the file they are already reading, so the cited + # line rides in the body instead. + # + # Guarded on the commit, not left to the API. Without one the call returns + # 422, and asking for an answer known before the call spends the rate limit + # on it. + on_file_or_summary() { + if [ -n "$head_sha" ] && gh api -X POST "repos/$REPO/pulls/$PR/comments" \ + -f body="$FINDING_MARKER"$'\n'"**$4** — $5"$'\n\n'"_Cited at \`$1:$2\`, outside this diff's changed lines._" \ + -f commit_id="$head_sha" -f path="$1" \ + -f subject_type=file >/dev/null 2>&1; then + on_file=$((on_file+1)) + return 0 + fi + to_summary "$@" + return 0 + } + + # All three rungs for one finding, with the API deciding which one it + # takes. This is the path for a finding the batch could not carry. + place_one() { + if [ -n "$head_sha" ] && gh api -X POST "repos/$REPO/pulls/$PR/comments" \ + -f body="$FINDING_MARKER"$'\n'"**$4** — $5" \ + -f commit_id="$head_sha" -f path="$1" \ + -F line="$2" -f side="$3" >/dev/null 2>&1; then + on_line=$((on_line+1)) + return 0 + fi + on_file_or_summary "$@" + return 0 + } + # The detail is base64 per record, not @tsv. Finding.Detail is raw model # prose with no line constraint on it, and @tsv escapes a newline, a tab or # a backslash into a literal \n, \t or \\ -- so a multi-line detail reached # the pull request showing its escape sequences instead of its text. The # four fields that cannot contain a tab stay plain. - while IFS=$'\t' read -r path line side severity detail_b64; do - [ -z "$path" ] && continue - detail="$(printf '%s' "$detail_b64" | base64 --decode)" - body="$FINDING_MARKER"$'\n'"**${severity}** — ${detail}" - # Guarded on the commit, not left to the API. Without one both calls - # return 422, and asking twice per finding for that answer spends the - # rate limit on a result already known before the loop. - if [ -n "$head_sha" ]; then - if gh api -X POST "repos/$REPO/pulls/$PR/comments" \ - -f body="$body" -f commit_id="$head_sha" -f path="$path" \ - -F line="$line" -f side="$side" >/dev/null 2>&1; then - on_line=$((on_line+1)) - continue - fi - # The line is outside the hunks. The file can still be in the pull - # request, and a comment on it reaches the reviewer in the file they - # are already reading, so the cited line rides in the body instead. - if gh api -X POST "repos/$REPO/pulls/$PR/comments" \ - -f body="$body"$'\n\n'"_Cited at \`$path:$line\`, outside this diff's changed lines._" \ - -f commit_id="$head_sha" -f path="$path" \ - -f subject_type=file >/dev/null 2>&1; then - on_file=$((on_file+1)) - continue + tsv='[.file, .line, .side, .severity, (.detail | @base64)] | @tsv' + rows() { jq -r "$1 | $tsv" "$2"; } + # Redirected into, never piped into. The counters the summary reads are + # this shell's, and a pipe would count them in a subshell that exits. + place_each() { + local placer="$1" path line side severity detail_b64 detail + while IFS=$'\t' read -r path line side severity detail_b64; do + [ -z "$path" ] && continue + detail="$(printf '%s' "$detail_b64" | base64 --decode)" + "$placer" "$path" "$line" "$side" "$severity" "$detail" + done + return 0 + } + + # One shape for every path below: a string file, an integer line, a side + # the API names. A line the model wrote as prose reads as 0, which no diff + # covers, so it takes the file rung rather than reaching gh as a field. + # + # Every field but the last is non-empty here, and that is what the reader + # below rests on. @tsv writes an empty field as nothing between two tabs, + # bash reads a tab as IFS whitespace and folds the pair into one delimiter, + # and the record then reads one field short: the severity arrives in the + # side, the base64 detail arrives in the severity, and a finding reaches the + # pull request with its own encoding printed as its severity. + normalise='[ .[] | { file: (.file | tostring), + line: (.line | if type == "number" then floor + elif type == "string" then ((. | tonumber?) // 0 | floor) + else 0 end), + side: ((.side // "RIGHT") | tostring | ascii_upcase + | if . == "LEFT" then "LEFT" else "RIGHT" end), + severity: (.severity | tostring + | if . == "" then "note" else . end), + detail: (.detail | tostring) } + | select(.file != "") ]' + normal="$RUNNER_TEMP/review-findings-normalised.json" + if ! jq "$normalise" "$FINDINGS" > "$normal"; then + echo "::warning::the findings file for $REPO#$PR could not be read, so this review places nothing on the diff" + { echo "on_line=0"; echo "on_file=0"; echo "unplaced=0"; } >> "$GITHUB_OUTPUT" + exit 0 + fi + + # Which lines a comment can name, walked out of the pull request's own diff + # before anything is posted. RIGHT takes an added or a context line, LEFT + # takes a removed or a context one, and ai-review walks the same patches the + # same way. The walk covers only the files the findings name, so its size is + # the finding count and not the diff's. + # + # Read ahead of the call because the call is all-or-nothing: one line the API + # refuses loses every comment with it, so a refusal has to be predicted here + # rather than met there. + # shellcheck disable=SC2016 # $findings and friends are jq's, and single quotes are what keep them jq's + partition='def hunks: + reduce ((.patch // "") | split("\n"))[] as $l + ({o: 0, n: 0, R: {}, L: {}}; + if ($l | test("^@@ -[0-9]+(,[0-9]+)? [+][0-9]+(,[0-9]+)? @@")) then + ($l | capture("^@@ -(?[0-9]+)(,[0-9]+)? [+](?[0-9]+)(,[0-9]+)? @@")) as $m + | .o = ($m.o | tonumber) | .n = ($m.n | tonumber) + elif ($l | startswith("+")) then .R[.n | tostring] = true | .n += 1 + elif ($l | startswith("-")) then .L[.o | tostring] = true | .o += 1 + elif ($l | startswith(" ")) then + .R[.n | tostring] = true | .L[.o | tostring] = true | .n += 1 | .o += 1 + else . end) + | {RIGHT: .R, LEFT: .L}; + (reduce $findings[0][] as $f ({}; .[$f.file] = true)) as $wanted + | ([ .[] | select($wanted[.filename // ""] // false) + | {key: .filename, value: hunks} ] | from_entries) as $index + | ($findings[0] | map(. + {ok: (($index[.file][.side][.line | tostring]) // false)})) + | { anchored: [ .[] | select(.ok) | del(.ok) ], + loose: [ .[] | select(.ok | not) | del(.ok) ] }' + files="$RUNNER_TEMP/review-pr-files.json" + placement="$RUNNER_TEMP/review-placement.json" + # No --jq on the fetch. --paginate runs that filter once per page and + # would answer once per page; the slurp below reads what gh returns + # whichever shape it takes, one array or one array per page. + batched=false + if [ -n "$head_sha" ] \ + && gh api --paginate "repos/$REPO/pulls/$PR/files?per_page=100" > "$files.raw" \ + && jq -s 'add // []' "$files.raw" > "$files" \ + && jq --slurpfile findings "$normal" "$partition" "$files" > "$placement"; then + batched=true + elif [ -n "$head_sha" ]; then + echo "::warning::the changed files of $REPO#$PR could not be read, so each finding is posted on its own; the review still reaches the code" + fi + + if [ "$batched" = true ]; then + anchored="$(jq '.anchored | length' "$placement")" + if [ "$anchored" -gt 0 ]; then + # The marker opens every comment body, as the first bytes. The history + # read and the resolve step both recognise this tool's own findings by + # that, and a body that merely contains it matches neither. + request="$RUNNER_TEMP/review-placement-request.json" + response="$RUNNER_TEMP/review-placement-response.json" + review_body="Findings on the changed lines. The verdict and the summary are in this tool's comment on this pull request." + # shellcheck disable=SC2016 # $sha and friends are jq's, bound by --arg + jq --arg sha "$head_sha" --arg marker "$FINDING_MARKER" --arg body "$review_body" \ + '{commit_id: $sha, event: "COMMENT", body: $body, + comments: [ .anchored[] + | {path: .file, line: .line, side: .side, + body: "\($marker)\n**\(.severity)** — \(.detail)"} ]}' \ + "$placement" > "$request" + # Two failures, two answers. A 422 is the API refusing the content and + # it creates nothing, so each finding is posted on its own and the one + # line that was refused costs only itself. Any other failure may be a + # write that landed and then lost its connection, and repeating it + # posts the review twice -- so those findings take the summary, where + # the reader still gets every one of them. + # + # The code comes from GitHub's error object on stdout, which is where + # gh leaves it, and this is the endpoint the position step below reads + # a 422 from the same way. An absent code reads as "not 422" and takes + # the summary: a shape this cannot read costs the placement, never a + # duplicate. + if gh api -X POST "repos/$REPO/pulls/$PR/reviews" --input "$request" > "$response"; then + on_line=$((on_line + anchored)) + echo "posted one review carrying $anchored comment(s) on $REPO#$PR" + else + status="$(jq -r '.status // empty' "$response" 2>/dev/null || true)" + if [ "$status" = "422" ]; then + echo "::warning::$REPO#$PR refused the review carrying $anchored comment(s); each finding is posted on its own instead" + place_each place_one < <(rows '.anchored[]' "$placement") + else + echo "::warning::the review carrying $anchored comment(s) could not be posted on $REPO#$PR; those findings are in the summary instead of on the diff" + place_each to_summary < <(rows '.anchored[]' "$placement") + fi fi fi - # shellcheck disable=SC2016 # the backticks are markdown, not a substitution - printf -- '- `%s:%s` (%s) — %s\n' "$path" "$line" "$severity" "$detail" >> "$NOTE" - unplaced=$((unplaced+1)) - done < <(jq -r '.[] | [.file, .line, .side, .severity, (.detail | @base64)] | @tsv' "$FINDINGS") + place_each on_file_or_summary < <(rows '.loose[]' "$placement") + else + place_each place_one < <(rows '.[]' "$normal") + fi # Two headers, because the summary collects findings for two reasons and # only one of them is about the reader's code. With no commit to attach to, # a finding on a changed line lands here as well, and calling it an @@ -1692,10 +1848,10 @@ jobs: } > "$NOTE.tmp" mv "$NOTE.tmp" "$NOTE" fi - # Here and at the early exit above, and nowhere between: the two points where - # these are final. A run that dies in between leaves them unwritten, which is - # the right answer there -- placement neither finished nor was skipped, so no - # number it could publish would be true. + # Here and at the two early exits above, and nowhere between: the points + # where these are final. A run that dies in between leaves them unwritten, + # which is the right answer there -- placement neither finished nor was + # skipped, so no number it could publish would be true. # # One append for all three, so the summary never reads a half-written set. It # requires all three for that reason, including the one no term renders. From cabbc50d299752ad10d33caf44eabd87db650191 Mon Sep 17 00:00:00 2001 From: bdchatham Date: Sun, 6 Sep 2026 15:59:24 -0700 Subject: [PATCH 2/5] fix(seidroid-review): index the diff at the reviewed commit, and name the summary groups MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two claims the placement step made that were not true. The index and the comments named different commits. The index came from `GET /pulls/{n}/files`, which answers for the pull request's current head and takes no commit — it accepts a `sha` parameter and ignores it, measured. Every comment names the recorded `REVIEWED_SHA`. A push during the review therefore marked lines from one diff and commented on another: a line the older commit cannot carry costs the whole batch a 422, and a line it can carry is hidden and falls to the file rung. The index now comes from `GET /compare/{base.sha}...{REVIEWED_SHA}`, which does take a commit and whose three-dot form is the diff the pull request shows. The check run is published against the commit the review read for the same reason. A failure that was not a 422 filed on-diff findings under a heading that denied them. They went to the summary under "Observations off the changed lines. These are about code this pull request does not touch", and they were on changed lines. They now carry their own heading, which says GitHub would not take the review that would have posted them. Both groups still count as unplaced, so the resolve gate is unchanged. Co-Authored-By: Claude Opus 5 (1M context) --- .github/workflows/seidroid-review.yml | 105 +++++++++++++++++++------- 1 file changed, 78 insertions(+), 27 deletions(-) diff --git a/.github/workflows/seidroid-review.yml b/.github/workflows/seidroid-review.yml index 9d03057..fd6512a 100644 --- a/.github/workflows/seidroid-review.yml +++ b/.github/workflows/seidroid-review.yml @@ -1653,14 +1653,28 @@ jobs: fi on_line=0 on_file=0 unplaced=0 - # The third rung. Each line opens with a dash and a backtick, which is - # what the summary counts when it cuts the list to fit GitHub's limit. + # The third rung, in two groups. A finding reaches the summary because the + # diff has nowhere to attach it, or because the diff does carry its line and + # the call that would have posted it failed. The summary tells a reader which, + # so the second group is collected apart and headed for what it is. Both count + # as unplaced: neither reached the code. + # + # Each line opens with a dash and a backtick, which is what the summary counts + # when it cuts the list to fit GitHub's limit. + ondiff="$RUNNER_TEMP/review-unplaced-on-diff.md" + : > "$ondiff" # shellcheck disable=SC2016 # the backticks are markdown, not a substitution to_summary() { printf -- '- `%s:%s` (%s) — %s\n' "$1" "$2" "$4" "$5" >> "$NOTE" unplaced=$((unplaced+1)) return 0 } + # shellcheck disable=SC2016 # the backticks are markdown, not a substitution + to_summary_ondiff() { + printf -- '- `%s:%s` (%s) — %s\n' "$1" "$2" "$4" "$5" >> "$ondiff" + unplaced=$((unplaced+1)) + return 0 + } # The second rung, one call per finding. A file-level comment is a field # the reviews API does not carry, so this one cannot ride in the batch @@ -1671,6 +1685,9 @@ jobs: # Guarded on the commit, not left to the API. Without one the call returns # 422, and asking for an answer known before the call spends the rate limit # on it. + # + # $6 is which summary group takes the finding if this rung refuses it too. + # The caller knows whether the diff carries the line; this does not. on_file_or_summary() { if [ -n "$head_sha" ] && gh api -X POST "repos/$REPO/pulls/$PR/comments" \ -f body="$FINDING_MARKER"$'\n'"**$4** — $5"$'\n\n'"_Cited at \`$1:$2\`, outside this diff's changed lines._" \ @@ -1679,7 +1696,7 @@ jobs: on_file=$((on_file+1)) return 0 fi - to_summary "$@" + "$6" "$1" "$2" "$3" "$4" "$5" return 0 } @@ -1707,11 +1724,11 @@ jobs: # Redirected into, never piped into. The counters the summary reads are # this shell's, and a pipe would count them in a subshell that exits. place_each() { - local placer="$1" path line side severity detail_b64 detail + local placer="$1" summary="$2" path line side severity detail_b64 detail while IFS=$'\t' read -r path line side severity detail_b64; do [ -z "$path" ] && continue detail="$(printf '%s' "$detail_b64" | base64 --decode)" - "$placer" "$path" "$line" "$side" "$severity" "$detail" + "$placer" "$path" "$line" "$side" "$severity" "$detail" "$summary" done return 0 } @@ -1743,8 +1760,8 @@ jobs: exit 0 fi - # Which lines a comment can name, walked out of the pull request's own diff - # before anything is posted. RIGHT takes an added or a context line, LEFT + # Which lines a comment can name, walked out of the diff the fetch below + # reads, before anything is posted. RIGHT takes an added or a context line, LEFT # takes a removed or a context one, and ai-review walks the same patches the # same way. The walk covers only the files the findings name, so its size is # the finding count and not the diff's. @@ -1773,17 +1790,43 @@ jobs: loose: [ .[] | select(.ok | not) | del(.ok) ] }' files="$RUNNER_TEMP/review-pr-files.json" placement="$RUNNER_TEMP/review-placement.json" - # No --jq on the fetch. --paginate runs that filter once per page and - # would answer once per page; the slurp below reads what gh returns - # whichever shape it takes, one array or one array per page. + # The diff at the commit this review read, which is the commit every comment + # below names. GET /pulls/{n}/files answers for the pull request's current + # head and takes no commit: it accepts a sha parameter and ignores it, + # measured. So a push mid-review would index one commit and comment on + # another -- and a line marked from the newer diff that the older commit + # cannot carry costs the whole batch a 422. compare takes a commit. + # + # base.sha with three dots, so the API resolves the merge base itself and + # answers with the diff the pull request shows. A base branch that moves + # during the review does not move that merge base, because the reviewed + # commit is fixed and the fork point with it. + # + # Tested for the shape of a commit id, and not merely for emptiness. gh + # applies --jq to a success and writes the error object itself on a failure, + # so a refused read answers with a line of JSON on stdout and a non-zero + # status. Either one alone leaves that JSON in the variable and sends it to + # the API as a commit. + base_sha="" + if [ -n "$head_sha" ]; then + base_sha="$(gh api "repos/$REPO/pulls/$PR" --jq '.base.sha' 2>/dev/null)" || base_sha="" + case "$base_sha" in (''|*[!0-9a-f]*) base_sha="" ;; esac + if [ -z "$base_sha" ]; then + echo "::warning::the base commit of $REPO#$PR could not be read, so the diff at $head_sha cannot be; each finding is posted on its own" + fi + fi + # No --jq on the fetch. --paginate runs that filter once per page and would + # answer once per page. compare answers with an object per page rather than + # an array, so the pages are read for their files: merging two of these + # objects keeps one page's list and drops the other. batched=false - if [ -n "$head_sha" ] \ - && gh api --paginate "repos/$REPO/pulls/$PR/files?per_page=100" > "$files.raw" \ - && jq -s 'add // []' "$files.raw" > "$files" \ + if [ -n "$head_sha" ] && [ -n "$base_sha" ] \ + && gh api --paginate "repos/$REPO/compare/$base_sha...$head_sha?per_page=100" > "$files.raw" \ + && jq -s '[ .[] | (.files // [])[] ]' "$files.raw" > "$files" \ && jq --slurpfile findings "$normal" "$partition" "$files" > "$placement"; then batched=true elif [ -n "$head_sha" ]; then - echo "::warning::the changed files of $REPO#$PR could not be read, so each finding is posted on its own; the review still reaches the code" + echo "::warning::the diff of $REPO#$PR at $head_sha could not be read, so each finding is posted on its own; the review still reaches the code" fi if [ "$batched" = true ]; then @@ -1821,30 +1864,38 @@ jobs: status="$(jq -r '.status // empty' "$response" 2>/dev/null || true)" if [ "$status" = "422" ]; then echo "::warning::$REPO#$PR refused the review carrying $anchored comment(s); each finding is posted on its own instead" - place_each place_one < <(rows '.anchored[]' "$placement") + place_each place_one to_summary_ondiff < <(rows '.anchored[]' "$placement") else echo "::warning::the review carrying $anchored comment(s) could not be posted on $REPO#$PR; those findings are in the summary instead of on the diff" - place_each to_summary < <(rows '.anchored[]' "$placement") + place_each to_summary_ondiff '' < <(rows '.anchored[]' "$placement") fi fi fi - place_each on_file_or_summary < <(rows '.loose[]' "$placement") + place_each on_file_or_summary to_summary < <(rows '.loose[]' "$placement") else - place_each place_one < <(rows '.[]' "$normal") - fi - # Two headers, because the summary collects findings for two reasons and - # only one of them is about the reader's code. With no commit to attach to, - # a finding on a changed line lands here as well, and calling it an - # observation off the changed lines tells the reader the wrong thing about - # their own diff. - if [ -s "$NOTE" ]; then + place_each place_one to_summary < <(rows '.[]' "$normal") + fi + # A heading per reason, because the summary collects findings for three and + # only one of them is about code the reader did not change. With no commit to + # attach to, a finding on a changed line lands here as well; so does one whose + # review the API would not take. Filing either under "off the changed lines" + # tells the author the wrong thing about their own diff, and the second is the + # one that hides a real issue behind a heading that denies it. + if [ -s "$ondiff" ] || [ -s "$NOTE" ]; then if [ -n "$head_sha" ]; then header='**Observations off the changed lines.** These are about code this pull request does not touch, so there is nowhere in the diff to attach them:' else header='**Every finding is here.** The commit under review was not recorded, so none of these could be attached to a line of the diff:' fi - { printf -- '---\n\n%s\n\n' "$header" - cat "$NOTE" + { printf -- '---\n' + if [ -s "$ondiff" ]; then + printf -- '\n%s\n\n' '**On the changed lines, and not posted.** GitHub would not take the review carrying these, so they are here rather than on the lines they name:' + cat "$ondiff" + fi + if [ -s "$NOTE" ]; then + printf -- '\n%s\n\n' "$header" + cat "$NOTE" + fi } > "$NOTE.tmp" mv "$NOTE.tmp" "$NOTE" fi From 37b2edf67e1f8a6cea048656fb37ba9526b4d845 Mon Sep 17 00:00:00 2001 From: bdchatham Date: Sun, 6 Sep 2026 16:03:05 -0700 Subject: [PATCH 3/5] fix(seidroid-review): read the refusal from the status line, and treat an absent patch as unknown MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two more claims the placement step made that were not true. It read the refusal code from a `status` field in the error body. GitHub's validation-error schema does not declare that field, so a 422 that carries none read as "not 422" and sent every anchorable finding to the summary — worse than the per-finding ladder this rung exists to reach, and `unplaced > 0` then holds every superseded thread open. The code now comes from the response's own status line, which `-i` puts first. A call that reached no response leaves no status line, and that empty code still takes the summary, because that is the case where the write may have landed. It counted a file the API sends without a patch as a file with no commentable lines. A binary file and a file whose diff was too large to send both arrive that way, so every finding on one dropped to the file rung under a body reading `Cited at path:120, outside this diff's changed lines` — which was false. Those findings now form a third group and go to the API one at a time, which is the only thing that can still say whether the line is in the diff. Co-Authored-By: Claude Opus 5 (1M context) --- .github/workflows/seidroid-review.yml | 39 +++++++++++++++++++-------- 1 file changed, 28 insertions(+), 11 deletions(-) diff --git a/.github/workflows/seidroid-review.yml b/.github/workflows/seidroid-review.yml index fd6512a..221b422 100644 --- a/.github/workflows/seidroid-review.yml +++ b/.github/workflows/seidroid-review.yml @@ -1766,6 +1766,12 @@ jobs: # same way. The walk covers only the files the findings name, so its size is # the finding count and not the diff's. # + # Three groups, because a file the API sends without a patch -- a binary one, + # or one whose diff was too large to send -- has unknown lines rather than no + # lines. Counting it as no lines would tell an author their cited line is + # outside the diff when it is in it. Those findings stay out of the batch and + # go to the API one at a time, which is the only thing that can still say. + # # Read ahead of the call because the call is all-or-nothing: one line the API # refuses loses every comment with it, so a refusal has to be predicted here # rather than met there. @@ -1784,10 +1790,15 @@ jobs: | {RIGHT: .R, LEFT: .L}; (reduce $findings[0][] as $f ({}; .[$f.file] = true)) as $wanted | ([ .[] | select($wanted[.filename // ""] // false) - | {key: .filename, value: hunks} ] | from_entries) as $index - | ($findings[0] | map(. + {ok: (($index[.file][.side][.line | tostring]) // false)})) - | { anchored: [ .[] | select(.ok) | del(.ok) ], - loose: [ .[] | select(.ok | not) | del(.ok) ] }' + | {key: .filename, + value: (if has("patch") then hunks else null end)} ] | from_entries) as $index + | ($findings[0] + | map(. as $g | ($index[$g.file]) as $h + | . + { ok: (($h[$g.side][$g.line | tostring]) // false), + unknown: (($index | has($g.file)) and ($h == null)) })) + | { anchored: [ .[] | select(.ok) | del(.ok, .unknown) ], + unknown: [ .[] | select(.ok | not) | select(.unknown) | del(.ok, .unknown) ], + loose: [ .[] | select(.ok | not) | select(.unknown | not) | del(.ok, .unknown) ] }' files="$RUNNER_TEMP/review-pr-files.json" placement="$RUNNER_TEMP/review-placement.json" # The diff at the commit this review read, which is the commit every comment @@ -1852,16 +1863,21 @@ jobs: # posts the review twice -- so those findings take the summary, where # the reader still gets every one of them. # - # The code comes from GitHub's error object on stdout, which is where - # gh leaves it, and this is the endpoint the position step below reads - # a 422 from the same way. An absent code reads as "not 422" and takes - # the summary: a shape this cannot read costs the placement, never a - # duplicate. - if gh api -X POST "repos/$REPO/pulls/$PR/reviews" --input "$request" > "$response"; then + # The code comes from the response's own status line, which -i puts + # first, and not from a status field in the error body. GitHub's + # validation-error schema declares no such field, and a 422 that + # carries none would read as "not 422" and send every anchorable + # finding to the summary -- worse than placing each one by hand, which + # is what this rung is for. + # + # A call that reached no response leaves no status line, and an empty + # code takes the summary. That is the case where the write may have + # landed, so it is the case that must not be repeated. + if gh api -i -X POST "repos/$REPO/pulls/$PR/reviews" --input "$request" > "$response"; then on_line=$((on_line + anchored)) echo "posted one review carrying $anchored comment(s) on $REPO#$PR" else - status="$(jq -r '.status // empty' "$response" 2>/dev/null || true)" + status="$(sed -n '1s|^HTTP/[0-9.]* \([0-9][0-9][0-9]\).*|\1|p' "$response" || true)" if [ "$status" = "422" ]; then echo "::warning::$REPO#$PR refused the review carrying $anchored comment(s); each finding is posted on its own instead" place_each place_one to_summary_ondiff < <(rows '.anchored[]' "$placement") @@ -1871,6 +1887,7 @@ jobs: fi fi fi + place_each place_one to_summary < <(rows '.unknown[]' "$placement") place_each on_file_or_summary to_summary < <(rows '.loose[]' "$placement") else place_each place_one to_summary < <(rows '.[]' "$normal") From 3215d68fa116633c0d567c771bc023c7f7ae161d Mon Sep 17 00:00:00 2001 From: bdchatham Date: Mon, 7 Sep 2026 12:30:42 -0700 Subject: [PATCH 4/5] fix(seidroid-review): read the diff as whole only when its length agrees, and retry on any 4xx Two answers from the review round on the batched placement call. A file list from GET /compare is trusted as the diff only when its length matches the pull request's own changed_files. The endpoint sends at most 300 files and drops the rest in silence: no total, no Link header for them, no flag. Its pages are pages of commits, and a second page carries no files key, so --paginate cannot reach the ones it dropped and is gone. Measured against the live API: a 398-file pull request answers with 300 on one page, while GET /pulls/{n}/files pages to all 398; a 251-file diff answers with 251. A short list read as the diff is the one thing the unknown bucket exists to stop. Every file it dropped looks exactly like a file the pull request never touched, so each finding in one took a file comment whose body told the author their cited line was outside a diff that holds it. A short list now indexes nothing and every finding goes to the API one at a time, which is what base did for all of them. The per-finding retry now takes any 4xx, not 422 alone. The request carries every finding's whole detail, which is model prose under no length bound, so the size of it is refused as readily as the content and GitHub answers that with 413. None of 400, 401, 403, 404, 413 or 422 can be a partial write, and sending them to the summary spent findings that one call each would have placed. That leaves 5xx and the empty status from a call that reached no response on the summary path, which is where the double-post risk lives. The pull request is read once for both fields, so the guard costs no call. Co-Authored-By: Claude Opus 5 (1M context) --- .github/workflows/seidroid-review.yml | 101 ++++++++++++++++++-------- 1 file changed, 70 insertions(+), 31 deletions(-) diff --git a/.github/workflows/seidroid-review.yml b/.github/workflows/seidroid-review.yml index 221b422..4982564 100644 --- a/.github/workflows/seidroid-review.yml +++ b/.github/workflows/seidroid-review.yml @@ -1813,31 +1813,63 @@ jobs: # during the review does not move that merge base, because the reviewed # commit is fixed and the fork point with it. # - # Tested for the shape of a commit id, and not merely for emptiness. gh - # applies --jq to a success and writes the error object itself on a failure, - # so a refused read answers with a line of JSON on stdout and a non-zero - # status. Either one alone leaves that JSON in the variable and sends it to - # the API as a commit. - base_sha="" + # The pull request itself, read once for two fields: the commit its diff + # starts from, and how many files that diff has. + # + # Both tested for shape, and not merely for emptiness. A read that fails + # leaves the API's error object in the file, and a field that object does + # not carry reads as null -- which would otherwise reach the API as a + # commit id. + is_count() { case "${1:-}" in (''|*[!0-9]*) return 1 ;; esac; } + pull="$RUNNER_TEMP/review-pull.json" + base_sha="" changed_files="" if [ -n "$head_sha" ]; then - base_sha="$(gh api "repos/$REPO/pulls/$PR" --jq '.base.sha' 2>/dev/null)" || base_sha="" + if gh api "repos/$REPO/pulls/$PR" > "$pull" 2>/dev/null; then + base_sha="$(jq -r '.base.sha // "" | tostring' "$pull" 2>/dev/null || true)" + changed_files="$(jq -r '.changed_files // "" | tostring' "$pull" 2>/dev/null || true)" + fi case "$base_sha" in (''|*[!0-9a-f]*) base_sha="" ;; esac + is_count "$changed_files" || changed_files="" if [ -z "$base_sha" ]; then echo "::warning::the base commit of $REPO#$PR could not be read, so the diff at $head_sha cannot be; each finding is posted on its own" fi fi - # No --jq on the fetch. --paginate runs that filter once per page and would - # answer once per page. compare answers with an object per page rather than - # an array, so the pages are read for their files: merging two of these - # objects keeps one page's list and drops the other. + # compare sends at most 300 files and drops the rest without saying so: no + # total, no Link header for them, no flag. Its pages are pages of commits, + # and a second page carries no files key at all, so there is nothing to page + # for. Measured: a 398-file pull request answers with 300. + # + # A short list cannot be told apart from a whole one, and a file missing + # from it looks exactly like a file the pull request never touched. Reading + # it as the diff would put a finding on its file under a body telling the + # author their cited line is outside a diff that holds it. So the list is + # counted against the pull request's own total, and a short one indexes + # nothing: every finding then goes to the API one at a time, which is the + # only thing that can still say where it belongs. + # + # 300 is that cap, so a list of 300 is at it. That test only has to carry + # the case where the total could not be read, because the total is exact. + build_index() { + local n + if gh api "repos/$REPO/compare/$base_sha...$head_sha?per_page=100" > "$files.raw" \ + && jq '[ (.files // [])[] ]' "$files.raw" > "$files" \ + && n="$(jq 'length' "$files")" && is_count "$n"; then + if [ -n "$changed_files" ] && [ "$n" -ne "$changed_files" ]; then + echo "::warning::the diff of $REPO#$PR at $head_sha came back with $n of its $changed_files file(s), so this run does not read it as the diff; each finding is posted on its own" + return 1 + fi + if [ -z "$changed_files" ] && [ "$n" -ge 300 ]; then + echo "::warning::the diff of $REPO#$PR at $head_sha came back with $n file(s), which is all this endpoint sends, and the pull request's own total could not be read; each finding is posted on its own" + return 1 + fi + jq --slurpfile findings "$normal" "$partition" "$files" > "$placement" && return 0 + fi + echo "::warning::the diff of $REPO#$PR at $head_sha could not be read, so each finding is posted on its own; the review still reaches the code" + return 1 + } batched=false - if [ -n "$head_sha" ] && [ -n "$base_sha" ] \ - && gh api --paginate "repos/$REPO/compare/$base_sha...$head_sha?per_page=100" > "$files.raw" \ - && jq -s '[ .[] | (.files // [])[] ]' "$files.raw" > "$files" \ - && jq --slurpfile findings "$normal" "$partition" "$files" > "$placement"; then + if [ -n "$head_sha" ] && [ -n "$base_sha" ] && build_index; then batched=true - elif [ -n "$head_sha" ]; then - echo "::warning::the diff of $REPO#$PR at $head_sha could not be read, so each finding is posted on its own; the review still reaches the code" fi if [ "$batched" = true ]; then @@ -1856,17 +1888,23 @@ jobs: | {path: .file, line: .line, side: .side, body: "\($marker)\n**\(.severity)** — \(.detail)"} ]}' \ "$placement" > "$request" - # Two failures, two answers. A 422 is the API refusing the content and + # Two failures, two answers. A 4xx is the API refusing the request and # it creates nothing, so each finding is posted on its own and the one - # line that was refused costs only itself. Any other failure may be a - # write that landed and then lost its connection, and repeating it - # posts the review twice -- so those findings take the summary, where - # the reader still gets every one of them. + # comment that was refused costs only itself. This request carries every + # finding's whole detail, which is model prose under no length bound, so + # the size of it is refused as readily as the content: 413 and 422 are + # the same answer here, and so is a 403 the retry can still get past one + # comment at a time. + # + # Any other failure may be a write that landed and then lost its + # connection, and repeating it posts the review twice -- so those + # findings take the summary, where the reader still gets every one of + # them. # # The code comes from the response's own status line, which -i puts # first, and not from a status field in the error body. GitHub's - # validation-error schema declares no such field, and a 422 that - # carries none would read as "not 422" and send every anchorable + # validation-error schema declares no such field, and a refusal that + # carries none would read as no refusal at all and send every anchorable # finding to the summary -- worse than placing each one by hand, which # is what this rung is for. # @@ -1878,13 +1916,14 @@ jobs: echo "posted one review carrying $anchored comment(s) on $REPO#$PR" else status="$(sed -n '1s|^HTTP/[0-9.]* \([0-9][0-9][0-9]\).*|\1|p' "$response" || true)" - if [ "$status" = "422" ]; then - echo "::warning::$REPO#$PR refused the review carrying $anchored comment(s); each finding is posted on its own instead" - place_each place_one to_summary_ondiff < <(rows '.anchored[]' "$placement") - else - echo "::warning::the review carrying $anchored comment(s) could not be posted on $REPO#$PR; those findings are in the summary instead of on the diff" - place_each to_summary_ondiff '' < <(rows '.anchored[]' "$placement") - fi + case "$status" in + 4??) + echo "::warning::$REPO#$PR refused the review carrying $anchored comment(s) with $status; each finding is posted on its own instead" + place_each place_one to_summary_ondiff < <(rows '.anchored[]' "$placement") ;; + *) + echo "::warning::the review carrying $anchored comment(s) could not be posted on $REPO#$PR; those findings are in the summary instead of on the diff" + place_each to_summary_ondiff '' < <(rows '.anchored[]' "$placement") ;; + esac fi fi place_each place_one to_summary < <(rows '.unknown[]' "$placement") From b7e9f8d69972321c9ac7fe796943ab510eb31289 Mon Sep 17 00:00:00 2001 From: bdchatham Date: Mon, 7 Sep 2026 12:30:59 -0700 Subject: [PATCH 5/5] test(seidroid-review): run the placement harness in CI 165 assertions over 29 cases against the shell and jq of `Place findings on the code`, which is the most intricate script in this workflow and had no check on it. run.sh reads the step's run: block and the workflow's own FINDING_MARKER out of the YAML on every run, so a run tests the file as it stands and cannot pass against a stale copy. bin/gh goes on PATH ahead of the real gh: it logs every call, serves fixture JSON through the step's own jq, keeps the request body the step sent, and decides per case whether a call succeeds. No call leaves the runner, so the job needs no token and takes contents: read. The cases cover the hunk walk, the three-way partition, the batch request's shape and marker, the per-finding ladder, the summary headings, the field shapes a model writes, the index commit, the file-count guard, and the status boundary at 4xx against 5xx. Each guard was checked by removing it: dropping the file-count test breaks 6 assertions, narrowing the retry back to 422 alone breaks 10, and widening it to 5xx breaks 13. The fixtures for the generated at-cap case are built by jq at run time rather than committed, so 300 files cost no diff to review. Co-Authored-By: Claude Opus 5 (1M context) --- .github/workflows/workflow-test-self.yml | 32 ++ test/seidroid-review/.gitignore | 5 + test/seidroid-review/README.md | 36 ++ test/seidroid-review/bin/gh | 105 +++++ test/seidroid-review/extract.py | 10 + test/seidroid-review/fx/all-placeable.json | 1 + test/seidroid-review/fx/broken.json | 1 + test/seidroid-review/fx/empty.json | 0 test/seidroid-review/fx/emptyarray.json | 1 + test/seidroid-review/fx/file-ok.txt | 4 + test/seidroid-review/fx/files-short.json | 1 + test/seidroid-review/fx/files-stale.json | 1 + test/seidroid-review/fx/files.json | 1 + test/seidroid-review/fx/line-ok-minus-one.tsv | 4 + test/seidroid-review/fx/line-ok.tsv | 5 + test/seidroid-review/fx/mixed.json | 1 + test/seidroid-review/fx/none.tsv | 0 test/seidroid-review/fx/none.txt | 0 test/seidroid-review/fx/nopatch.json | 1 + test/seidroid-review/fx/odd.json | 1 + test/seidroid-review/fx/shift.json | 1 + test/seidroid-review/run.sh | 391 ++++++++++++++++++ 22 files changed, 602 insertions(+) create mode 100644 .github/workflows/workflow-test-self.yml create mode 100644 test/seidroid-review/.gitignore create mode 100644 test/seidroid-review/README.md create mode 100755 test/seidroid-review/bin/gh create mode 100644 test/seidroid-review/extract.py create mode 100644 test/seidroid-review/fx/all-placeable.json create mode 100644 test/seidroid-review/fx/broken.json create mode 100644 test/seidroid-review/fx/empty.json create mode 100644 test/seidroid-review/fx/emptyarray.json create mode 100644 test/seidroid-review/fx/file-ok.txt create mode 100644 test/seidroid-review/fx/files-short.json create mode 100644 test/seidroid-review/fx/files-stale.json create mode 100644 test/seidroid-review/fx/files.json create mode 100644 test/seidroid-review/fx/line-ok-minus-one.tsv create mode 100644 test/seidroid-review/fx/line-ok.tsv create mode 100644 test/seidroid-review/fx/mixed.json create mode 100644 test/seidroid-review/fx/none.tsv create mode 100644 test/seidroid-review/fx/none.txt create mode 100644 test/seidroid-review/fx/nopatch.json create mode 100644 test/seidroid-review/fx/odd.json create mode 100644 test/seidroid-review/fx/shift.json create mode 100755 test/seidroid-review/run.sh diff --git a/.github/workflows/workflow-test-self.yml b/.github/workflows/workflow-test-self.yml new file mode 100644 index 0000000..d2668ab --- /dev/null +++ b/.github/workflows/workflow-test-self.yml @@ -0,0 +1,32 @@ +name: Workflow tests +# The shell and jq inside seidroid-review.yml, run against a gh stub. Nothing here +# reaches the GitHub API, so this needs no token and no permissions. +on: + pull_request: + paths: + - '.github/workflows/seidroid-review.yml' + - '.github/workflows/workflow-test-self.yml' + - 'test/seidroid-review/**' + push: + branches: [ main ] + paths: + - '.github/workflows/seidroid-review.yml' + - '.github/workflows/workflow-test-self.yml' + - 'test/seidroid-review/**' +permissions: + contents: read +jobs: + place-findings: + name: Place findings on the code + runs-on: ubuntu-latest + steps: + - name: Checkout code + uses: actions/checkout@v7 + - name: Set up Python + uses: actions/setup-python@v7 + with: + python-version: '3.x' + - name: Install the YAML reader + run: python3 -m pip install --quiet pyyaml + - name: Run the placement harness + run: test/seidroid-review/run.sh diff --git a/test/seidroid-review/.gitignore b/test/seidroid-review/.gitignore new file mode 100644 index 0000000..025aed9 --- /dev/null +++ b/test/seidroid-review/.gitignore @@ -0,0 +1,5 @@ +# Written by run.sh: the step extracted from the workflow, the fixtures it +# generates, and one directory of output per case. +place.sh +fx/gen/ +out/ diff --git a/test/seidroid-review/README.md b/test/seidroid-review/README.md new file mode 100644 index 0000000..36cd1ce --- /dev/null +++ b/test/seidroid-review/README.md @@ -0,0 +1,36 @@ +# `Place findings on the code` + +Runs the placement step of `.github/workflows/seidroid-review.yml` under `bash`, +against a `gh` stub, and checks what it posted and what it counted. + +```sh +test/seidroid-review/run.sh +``` + +The run needs `bash`, `jq`, and `python3` with PyYAML. It exits non-zero on the +first failed assertion count and prints a table of one row per case. + +## How it works + +`extract.py` reads the step's `run:` block and the workflow's `FINDING_MARKER` +out of the YAML on every run, so the harness tests the file as it stands. + +`bin/gh` goes on `PATH` ahead of the real `gh`. It logs every call, serves +fixture JSON through the step's own `jq`, keeps the request body the step sent, +and decides per case whether a call succeeds. `STUB_*` variables in `run_case` +select the fixtures and the answers. + +## The fixtures + +`fx/files*.json` are `GET /compare` responses. One JSON object each: compare +paginates its commits, and a second page carries no `files` key, so the step +reads one page. + +- `files.json` — four files, two with a patch, two without +- `files-short.json` — the same diff with `pkg/b.go` missing, which is what a + truncated response looks like +- `files-stale.json` — a different commit's diff, for the pushed-head case + +`fx/line-ok.tsv` and `fx/file-ok.txt` list the `path`/`side`/`line` and the +paths the stub accepts. A finding outside them is refused, which is how the +per-finding ladder is exercised. diff --git a/test/seidroid-review/bin/gh b/test/seidroid-review/bin/gh new file mode 100755 index 0000000..4268441 --- /dev/null +++ b/test/seidroid-review/bin/gh @@ -0,0 +1,105 @@ +#!/usr/bin/env bash +# gh stub: logs every call, serves fixture JSON, and decides success per scenario. +log() { printf '%s\n' "$*" >> "$STUB_LOG"; } + +argv=("$@") +joined="$*" + +# --- read the pull request object (base.sha and changed_files) ---------------- +case "$joined" in + *"/pulls/$STUB_PR"|*"/pulls/$STUB_PR --jq"*) + log "CALL pull" + if [ "$STUB_BASE" = "FAIL" ]; then + # A failed read answers with the error object, which the step files and reads. + printf '%s\n' '{"message":"Not Found","status":"404"}' + exit 1 + fi + # NONE serves a response carrying no changed_files, which is the only way the + # step reaches its file-count cap test. + if [ "${STUB_CHANGED_FILES:-}" = "NONE" ]; then + printf '{"number":%s,"base":{"sha":"%s"}}\n' "$STUB_PR" "$STUB_BASE" + else + printf '{"number":%s,"base":{"sha":"%s"},"changed_files":%s}\n' \ + "$STUB_PR" "$STUB_BASE" "${STUB_CHANGED_FILES:-4}" + fi + exit 0 + ;; +esac + +# --- the diff between two commits --------------------------------------------- +case "$joined" in + *"/compare/"*) + # The commit range the step asked for, so a test can assert which one it read. + for a in "${argv[@]}"; do case "$a" in *"/compare/"*) range="${a#*/compare/}" ;; esac; done + log "CALL compare ${range%%\?*}" + if [ "$STUB_FILES" = "FAIL" ]; then + printf '%s\n' '{"message":"Not Found","status":"404"}' + exit 1 + fi + # One object. compare paginates its commits, and a second page carries no files + # key at all, so the step asks for one page and reads its files. + fx="$STUB_FILES" + case "${range%%\?*}" in + *"...$STUB_STALE_SHA") fx="${STUB_FILES_STALE:-$STUB_FILES}" ;; + esac + cat "$fx" + exit 0 + ;; +esac + +# --- create a review (the batch) --------------------------------------------- +case "$joined" in + *"/reviews"*) + n=${#argv[@]} + for ((i=0;i> "$STUB_BODIES" + if [ "$subject" = "file" ]; then + log "CALL file-comment $path" + grep -qxF -- "$path" "$STUB_FILE_OK" && exit 0 + printf '%s\n' '{"message":"Validation Failed","status":"422"}' + exit 1 + fi + log "CALL line-comment $path $side $line" + grep -qxF -- "$path $side $line" "$STUB_LINE_OK" && exit 0 + printf '%s\n' '{"message":"Validation Failed","status":"422"}' + exit 1 + ;; +esac + +log "CALL unhandled $joined" +exit 1 diff --git a/test/seidroid-review/extract.py b/test/seidroid-review/extract.py new file mode 100644 index 0000000..249b862 --- /dev/null +++ b/test/seidroid-review/extract.py @@ -0,0 +1,10 @@ +import sys, yaml +path, step, out = sys.argv[1], sys.argv[2], sys.argv[3] +d = yaml.safe_load(open(path, encoding="utf-8")) +for job in d["jobs"].values(): + for s in job.get("steps", []): + if s.get("name") == step: + open(out, "w", encoding="utf-8").write(s["run"]) + print(d["env"]["FINDING_MARKER"]) + sys.exit(0) +sys.exit("step not found: " + step) diff --git a/test/seidroid-review/fx/all-placeable.json b/test/seidroid-review/fx/all-placeable.json new file mode 100644 index 0000000..ad8516f --- /dev/null +++ b/test/seidroid-review/fx/all-placeable.json @@ -0,0 +1 @@ +[{"file": "pkg/a.go", "severity": "blocker", "detail": "First finding.\n\nA second paragraph with a\ttab.", "line": 11, "side": "RIGHT"}, {"file": "pkg/a.go", "severity": "suggestion", "detail": "Backtick `code`, a \"quote\" and a $dollar.", "line": 12, "side": "RIGHT"}, {"file": "pkg/b.go", "severity": "nit", "detail": "Third.", "line": 2, "side": "RIGHT"}, {"file": "pkg/a.go", "severity": "blocker", "detail": "On the removed line.", "line": 11, "side": "LEFT"}] \ No newline at end of file diff --git a/test/seidroid-review/fx/broken.json b/test/seidroid-review/fx/broken.json new file mode 100644 index 0000000..f2602dc --- /dev/null +++ b/test/seidroid-review/fx/broken.json @@ -0,0 +1 @@ +{ not json \ No newline at end of file diff --git a/test/seidroid-review/fx/empty.json b/test/seidroid-review/fx/empty.json new file mode 100644 index 0000000..e69de29 diff --git a/test/seidroid-review/fx/emptyarray.json b/test/seidroid-review/fx/emptyarray.json new file mode 100644 index 0000000..0637a08 --- /dev/null +++ b/test/seidroid-review/fx/emptyarray.json @@ -0,0 +1 @@ +[] \ No newline at end of file diff --git a/test/seidroid-review/fx/file-ok.txt b/test/seidroid-review/fx/file-ok.txt new file mode 100644 index 0000000..6badff2 --- /dev/null +++ b/test/seidroid-review/fx/file-ok.txt @@ -0,0 +1,4 @@ +pkg/a.go +pkg/b.go +assets/logo.png +pkg/huge.go diff --git a/test/seidroid-review/fx/files-short.json b/test/seidroid-review/fx/files-short.json new file mode 100644 index 0000000..ebd21df --- /dev/null +++ b/test/seidroid-review/fx/files-short.json @@ -0,0 +1 @@ +{"files":[{"filename":"pkg/a.go","patch":"@@ -10,4 +10,6 @@\n ctx10\n-gone11\n+added11\n+added12\n ctx13"},{"filename":"assets/logo.png"},{"filename":"pkg/huge.go"}]} diff --git a/test/seidroid-review/fx/files-stale.json b/test/seidroid-review/fx/files-stale.json new file mode 100644 index 0000000..0e6c73b --- /dev/null +++ b/test/seidroid-review/fx/files-stale.json @@ -0,0 +1 @@ +{"files": [{"filename": "pkg/a.go", "patch": "@@ -40,3 +40,4 @@\n ctx40\n+added41\n+added42\n ctx43"}, {"filename": "pkg/b.go", "patch": "@@ -80,2 +80,3 @@\n eighty\n+eightyone\n eightytwo"}]} diff --git a/test/seidroid-review/fx/files.json b/test/seidroid-review/fx/files.json new file mode 100644 index 0000000..d8b4f07 --- /dev/null +++ b/test/seidroid-review/fx/files.json @@ -0,0 +1 @@ +{"files":[{"filename":"pkg/a.go","patch":"@@ -10,4 +10,6 @@\n ctx10\n-gone11\n+added11\n+added12\n ctx13"},{"filename":"pkg/b.go","patch":"@@ -1,2 +1,3 @@\n one\n+two\n three"},{"filename":"assets/logo.png"},{"filename":"pkg/huge.go"}]} diff --git a/test/seidroid-review/fx/line-ok-minus-one.tsv b/test/seidroid-review/fx/line-ok-minus-one.tsv new file mode 100644 index 0000000..3635ccb --- /dev/null +++ b/test/seidroid-review/fx/line-ok-minus-one.tsv @@ -0,0 +1,4 @@ +pkg/a.go RIGHT 11 +pkg/b.go RIGHT 2 +pkg/a.go LEFT 11 +pkg/huge.go RIGHT 120 diff --git a/test/seidroid-review/fx/line-ok.tsv b/test/seidroid-review/fx/line-ok.tsv new file mode 100644 index 0000000..4030781 --- /dev/null +++ b/test/seidroid-review/fx/line-ok.tsv @@ -0,0 +1,5 @@ +pkg/a.go RIGHT 11 +pkg/a.go RIGHT 12 +pkg/b.go RIGHT 2 +pkg/a.go LEFT 11 +pkg/huge.go RIGHT 120 diff --git a/test/seidroid-review/fx/mixed.json b/test/seidroid-review/fx/mixed.json new file mode 100644 index 0000000..1b96cfb --- /dev/null +++ b/test/seidroid-review/fx/mixed.json @@ -0,0 +1 @@ +[{"file": "pkg/a.go", "severity": "blocker", "detail": "First finding.\n\nA second paragraph with a\ttab.", "line": 11, "side": "RIGHT"}, {"file": "pkg/a.go", "severity": "suggestion", "detail": "Backtick `code`, a \"quote\" and a $dollar.", "line": 12, "side": "RIGHT"}, {"file": "pkg/b.go", "severity": "nit", "detail": "Third.", "line": 2, "side": "RIGHT"}, {"file": "pkg/a.go", "severity": "blocker", "detail": "On the removed line.", "line": 11, "side": "LEFT"}, {"file": "pkg/a.go", "severity": "suggestion", "detail": "Outside the hunks.", "line": 999, "side": "RIGHT"}, {"file": "pkg/untouched.go", "severity": "blocker", "detail": "A file the pull request never touches.", "line": 5, "side": "RIGHT"}, {"file": "pkg/b.go", "severity": "suggestion", "detail": "No line at all."}] \ No newline at end of file diff --git a/test/seidroid-review/fx/none.tsv b/test/seidroid-review/fx/none.tsv new file mode 100644 index 0000000..e69de29 diff --git a/test/seidroid-review/fx/none.txt b/test/seidroid-review/fx/none.txt new file mode 100644 index 0000000..e69de29 diff --git a/test/seidroid-review/fx/nopatch.json b/test/seidroid-review/fx/nopatch.json new file mode 100644 index 0000000..b130f0a --- /dev/null +++ b/test/seidroid-review/fx/nopatch.json @@ -0,0 +1 @@ +[{"file": "pkg/huge.go", "line": 120, "side": "RIGHT", "severity": "blocker", "detail": "A line the index cannot see and the API can."}, {"file": "assets/logo.png", "line": 1, "side": "RIGHT", "severity": "nit", "detail": "A binary file has no line to take."}, {"file": "pkg/a.go", "line": 11, "side": "RIGHT", "severity": "suggestion", "detail": "An ordinary anchored finding beside them."}] \ No newline at end of file diff --git a/test/seidroid-review/fx/odd.json b/test/seidroid-review/fx/odd.json new file mode 100644 index 0000000..a65b899 --- /dev/null +++ b/test/seidroid-review/fx/odd.json @@ -0,0 +1 @@ +[{"file": "pkg/a.go", "line": "12", "side": "right", "severity": "nit", "detail": "A line the model wrote as a string."}, {"file": "pkg/a.go", "line": "@/etc/passwd", "side": "RIGHT", "severity": "blocker", "detail": "A line that is not a number."}, {"file": "pkg/b.go", "line": 2, "severity": "suggestion", "detail": "No side named."}, {"file": "", "line": 1, "side": "RIGHT", "severity": "nit", "detail": "No file named."}] \ No newline at end of file diff --git a/test/seidroid-review/fx/shift.json b/test/seidroid-review/fx/shift.json new file mode 100644 index 0000000..fc7f8df --- /dev/null +++ b/test/seidroid-review/fx/shift.json @@ -0,0 +1 @@ +[{"file": "pkg/a.go", "line": 999, "side": "RIGHT", "severity": "", "detail": "An unrated finding off the hunks."}, {"file": "pkg/untouched.go", "line": 3, "side": "", "severity": "", "detail": "An unrated finding nowhere."}] \ No newline at end of file diff --git a/test/seidroid-review/run.sh b/test/seidroid-review/run.sh new file mode 100755 index 0000000..80c2cd0 --- /dev/null +++ b/test/seidroid-review/run.sh @@ -0,0 +1,391 @@ +#!/usr/bin/env bash +# Runs the shipped step script under bash with a gh stub on PATH. +# +# The step and the marker it writes are both read out of the workflow on every +# run, so a run tests what the file says now and cannot pass against a stale copy. +set -uo pipefail +HERE="$(cd "$(dirname "$0")" && pwd)" +REPOROOT="$(cd "$HERE/../.." && pwd)" +WORKFLOW="$REPOROOT/.github/workflows/seidroid-review.yml" +STEP="Place findings on the code" +SCRIPT="$HERE/place.sh" +MARKER="$(python3 "$HERE/extract.py" "$WORKFLOW" "$STEP" "$SCRIPT")" || { + echo "could not read '$STEP' out of $WORKFLOW"; exit 1; } +pass=0 fail=0 +rows=() + +# A file list at the endpoint's cap, generated rather than committed. compare sends +# 300 files however large the diff is, so 300 is the length that tests the cap; the +# findings and the accepted line go with it so the case stays one thing to read. +GEN="$HERE/fx/gen" +rm -rf "$GEN"; mkdir -p "$GEN" +jq -nc '{files: [ range(300) | {filename: "pkg/gen\(.).go", + patch: "@@ -1,2 +1,3 @@\n one\n+two\n three"} ]}' \ + > "$GEN/files-at-cap.json" +jq -nc '[{file: "pkg/gen7.go", line: 2, side: "RIGHT", severity: "blocker", + detail: "A finding in a diff as long as the endpoint will send."}]' \ + > "$GEN/findings-at-cap.json" +printf 'pkg/gen7.go\tRIGHT\t2\n' > "$GEN/line-ok-gen.tsv" + +run_case() { + # $1 name, then KEY=VALUE overrides + local name="$1"; shift + CASE="$HERE/out/$name" + rm -rf "$CASE"; mkdir -p "$CASE" + export STUB_LOG="$CASE/calls.log"; : > "$STUB_LOG" + export STUB_REQUEST="$CASE/request.json"; : > "$STUB_REQUEST" + export STUB_BODIES="$CASE/bodies.txt"; : > "$STUB_BODIES" + export STUB_FILES="$HERE/fx/files.json" + export STUB_FILES_STALE="$HERE/fx/files-stale.json" + # What the pull request says its diff holds. fx/files.json serves exactly this + # many, so the step reads that list as whole; a case that serves a different + # number sets this to match, or to the number that exposes the shortfall. + export STUB_CHANGED_FILES=4 + export STUB_PR=7 + export STUB_BASE=0ba5e0ba5e0ba5e0ba5e0ba5e0ba5e0ba5e0ba5e0 + export STUB_STALE_SHA=facefeed1234facefeed1234facefeed1234face + export STUB_REVIEW=ok + export STUB_LINE_OK="$HERE/fx/line-ok.tsv" + export STUB_FILE_OK="$HERE/fx/file-ok.txt" + export FINDINGS="$HERE/fx/all-placeable.json" + export REVIEWED_SHA=deadbeefcafedeadbeefcafedeadbeefcafedead + for kv in "$@"; do export "${kv?}"; done + + export PATH="$HERE/bin:$PATH" + export RUNNER_TEMP="$CASE/tmp"; mkdir -p "$RUNNER_TEMP" + export GITHUB_OUTPUT="$CASE/output.txt"; : > "$GITHUB_OUTPUT" + export NOTE="$CASE/note.md" + export REPO=owner/repo PR=7 GH_TOKEN=x + export FINDING_MARKER="$MARKER" + bash "$SCRIPT" > "$CASE/stdout.txt" 2> "$CASE/stderr.txt" + echo "$?" > "$CASE/rc" +} + +out() { grep -E "^$1=" "$CASE/output.txt" | tail -1 | cut -d= -f2- ; } +calls() { grep -c "^CALL $1" "$CASE/calls.log" || true; } + +check() { # name expected actual + if [ "$2" = "$3" ]; then pass=$((pass+1)); else fail=$((fail+1)); echo " FAIL $1: want [$2] got [$3]"; fi +} + +report() { # label + rows+=("$(printf '%-34s rc=%s pull=%s cmp=%s reviews=%s line=%s file=%s on_line=%s on_file=%s unplaced=%s note=%s' \ + "$1" "$(cat "$CASE/rc")" "$(calls pull)" "$(calls compare)" "$(calls reviews)" "$(calls line-comment)" "$(calls file-comment)" \ + "$(out on_line)" "$(out on_file)" "$(out unplaced)" "$(grep -c '^- `' "$CASE/note.md" 2>/dev/null; true)")") +} + +echo "== 1. several findings, all placeable ==" +run_case all-placeable +report "1 all placeable" +check "one review call" 1 "$(calls reviews)" +check "no comment calls" 0 "$(( $(calls line-comment) + $(calls file-comment) ))" +check "on_line" 4 "$(out on_line)" +check "on_file" 0 "$(out on_file)" +check "unplaced" 0 "$(out unplaced)" +check "comments in request" 4 "$(jq '.comments | length' "$CASE/request.json")" +check "every body marked" 4 "$(jq --arg m "$FINDING_MARKER" '[.comments[] | select(.body | startswith($m + "\n"))] | length' "$CASE/request.json")" +check "event" COMMENT "$(jq -r .event "$CASE/request.json")" +check "commit_id" deadbeefcafedeadbeefcafedeadbeefcafedead "$(jq -r .commit_id "$CASE/request.json")" +check "body non-empty" true "$(jq -r '(.body | length) > 0' "$CASE/request.json")" +check "paths/lines/sides" 'pkg/a.go:11:RIGHT pkg/a.go:12:RIGHT pkg/b.go:2:RIGHT pkg/a.go:11:LEFT' \ + "$(jq -r '[.comments[] | "\(.path):\(.line):\(.side)"] | join(" ")' "$CASE/request.json")" +check "marker is first bytes" 4 "$(jq --arg m "$FINDING_MARKER" '[.comments[] | select((.body | .[0:($m|length)]) == $m)] | length' "$CASE/request.json")" +check "review body unmarked" false "$(jq -r --arg v "" '.body | startswith($v)' "$CASE/request.json")" +check "review body has no marker" false "$(jq -r --arg m "$FINDING_MARKER" '.body | startswith($m)' "$CASE/request.json")" +check "multi-line detail intact" true \ + "$(jq -r '.comments[0].body | contains("A second paragraph with a\ttab.")' "$CASE/request.json")" +check "quoting intact" true \ + "$(jq -r '.comments[1].body | contains("Backtick `code`, a \"quote\" and a $dollar.")' "$CASE/request.json")" + +echo "== 2. mixed: off-hunk line, untouched file, no line at all ==" +run_case mixed FINDINGS="$HERE/fx/mixed.json" +report "2 mixed" +check "one review call" 1 "$(calls reviews)" +check "no line-comment call" 0 "$(calls line-comment)" +check "three file attempts" 3 "$(calls file-comment)" +check "on_line" 4 "$(out on_line)" +check "on_file" 2 "$(out on_file)" +check "unplaced" 1 "$(out unplaced)" +check "note names the file" 1 "$(grep -c 'pkg/untouched.go:5' "$CASE/note.md")" +check "note header" 1 "$(grep -c 'Observations off the changed lines' "$CASE/note.md")" + +echo "== 3. the batch is rejected with 422 ==" +run_case batch-422 FINDINGS="$HERE/fx/mixed.json" STUB_REVIEW=422 +report "3 batch 422" +check "one review call" 1 "$(calls reviews)" +check "four line retries" 4 "$(calls line-comment)" +check "on_line" 4 "$(out on_line)" +check "on_file" 2 "$(out on_file)" +check "unplaced" 1 "$(out unplaced)" +check "every fallback body marked" 7 "$(grep -cxF -- "$FINDING_MARKER" "$CASE/bodies.txt")" +check "warning emitted" 1 "$(grep -c '::warning::owner/repo#7 refused the review' "$CASE/stdout.txt")" + +echo "== 4. batch 422, and one line the API also refuses on its own ==" +run_case batch-422-partial FINDINGS="$HERE/fx/mixed.json" STUB_REVIEW=422 \ + STUB_LINE_OK="$HERE/fx/line-ok-minus-one.tsv" +report "4 batch 422 + one bad line" +check "on_line" 3 "$(out on_line)" +check "on_file" 3 "$(out on_file)" +check "unplaced" 1 "$(out unplaced)" + +echo "== 5. the batch fails with something other than 422 ==" +run_case batch-500 FINDINGS="$HERE/fx/mixed.json" STUB_REVIEW=500 +report "5 batch 500" +check "one review call" 1 "$(calls reviews)" +check "no line retries" 0 "$(calls line-comment)" +check "on_line" 0 "$(out on_line)" +check "on_file" 2 "$(out on_file)" +check "unplaced" 5 "$(out unplaced)" +check "warning emitted" 1 "$(grep -c '::warning::the review carrying 4 comment(s) could not be posted' "$CASE/stdout.txt")" + +echo "== 6. the batch fails and the error body has no readable code ==" +run_case batch-noshape FINDINGS="$HERE/fx/mixed.json" STUB_REVIEW=noshape +report "6 batch, unreadable error" +check "no line retries" 0 "$(calls line-comment)" +check "on_line" 0 "$(out on_line)" +check "unplaced" 5 "$(out unplaced)" + +echo "== 7. zero findings (empty file) ==" +run_case zero FINDINGS="$HERE/fx/empty.json" +report "7 zero findings" +check "no calls at all" 0 "$(( $(calls reviews) + $(calls line-comment) + $(calls file-comment) + $(calls compare) + $(calls pull) ))" +check "on_line" 0 "$(out on_line)" +check "on_file" 0 "$(out on_file)" +check "unplaced" 0 "$(out unplaced)" + +echo "== 8. an empty findings array ==" +run_case emptyarray FINDINGS="$HERE/fx/emptyarray.json" +report "8 empty array" +check "no review call" 0 "$(calls reviews)" +check "on_line" 0 "$(out on_line)" +check "unplaced" 0 "$(out unplaced)" + +echo "== 9. the findings file cannot be read ==" +run_case broken FINDINGS="$HERE/fx/broken.json" +report "9 unreadable findings" +check "no posting calls" 0 "$(( $(calls reviews) + $(calls line-comment) + $(calls file-comment) ))" +check "on_line" 0 "$(out on_line)" +check "on_file" 0 "$(out on_file)" +check "unplaced" 0 "$(out unplaced)" +check "counts still written" 3 "$(grep -cE '^(on_line|on_file|unplaced)=' "$CASE/output.txt")" + +echo "== 10. the changed-file list cannot be read ==" +run_case nofiles FINDINGS="$HERE/fx/mixed.json" STUB_FILES=FAIL +report "10 file list fails" +check "no review call" 0 "$(calls reviews)" +check "seven line attempts" 7 "$(calls line-comment)" +check "on_line" 4 "$(out on_line)" +check "on_file" 2 "$(out on_file)" +check "unplaced" 1 "$(out unplaced)" + +echo "== 11. the reviewed commit was never recorded ==" +run_case nosha FINDINGS="$HERE/fx/mixed.json" REVIEWED_SHA="" +report "11 no reviewed commit" +check "no calls at all" 0 "$(( $(calls reviews) + $(calls line-comment) + $(calls file-comment) + $(calls compare) + $(calls pull) ))" +check "on_line" 0 "$(out on_line)" +check "on_file" 0 "$(out on_file)" +check "unplaced" 7 "$(out unplaced)" +check "note header" 1 "$(grep -c 'Every finding is here' "$CASE/note.md")" + +echo "== 12. a string line, a junk line, a missing side, a missing file ==" +run_case odd FINDINGS="$HERE/fx/odd.json" +report "12 odd field shapes" +check "one review call" 1 "$(calls reviews)" +check "two anchored" 2 "$(jq '.comments | length' "$CASE/request.json")" +check "string line coerced" 'pkg/a.go:12:RIGHT pkg/b.go:2:RIGHT' \ + "$(jq -r '[.comments[] | "\(.path):\(.line):\(.side)"] | join(" ")' "$CASE/request.json")" +check "junk line to file" 1 "$(calls file-comment)" +check "on_line" 2 "$(out on_line)" +check "on_file" 1 "$(out on_file)" +check "unplaced" 0 "$(out unplaced)" +check "no file named is dropped" 0 "$(grep -c 'No file named' "$CASE/note.md")" + +echo +echo "== 13. a finding with an empty severity ==" +run_case shift FINDINGS="$HERE/fx/shift.json" STUB_REVIEW=ok +report "13 empty severity" +check "two file attempts" 2 "$(calls file-comment)" +check "on_file" 1 "$(out on_file)" +check "unplaced" 1 "$(out unplaced)" +# shellcheck disable=SC2016 # the backticks are the summary's markdown, not a substitution +check "no field shift" 1 "$(grep -c '^- `pkg/untouched.go:3` (note) — An unrated finding nowhere.$' "$CASE/note.md")" +check "both bodies marked" 2 "$(grep -cxF -- "$FINDING_MARKER" "$CASE/bodies.txt")" + +echo +echo "== 14. the head moved during the review: the index reads the reviewed commit ==" +run_case at-reviewed-commit +report "14 index at reviewed commit" +check "one pull read" 1 "$(calls pull)" +check "one compare" 1 "$(calls compare)" +check "compared at REVIEWED_SHA" "CALL compare 0ba5e0ba5e0ba5e0ba5e0ba5e0ba5e0ba5e0ba5e0...deadbeefcafedeadbeefcafedeadbeefcafedead" \ + "$(grep '^CALL compare' "$CASE/calls.log")" +check "one review call" 1 "$(calls reviews)" +check "on_line" 4 "$(out on_line)" + +echo "== 14b. the same run, had the index read the pushed head instead ==" +run_case at-pushed-head REVIEWED_SHA=facefeed1234facefeed1234facefeed1234face STUB_CHANGED_FILES=2 +report "14b index at pushed head" +check "compared at that sha" "CALL compare 0ba5e0ba5e0ba5e0ba5e0ba5e0ba5e0ba5e0ba5e0...facefeed1234facefeed1234facefeed1234face" \ + "$(grep '^CALL compare' "$CASE/calls.log")" +check "no request was built" "" "$(cat "$CASE/request.json")" +check "no review call" 0 "$(calls reviews)" +check "all four to the file rung" 4 "$(calls file-comment)" + +echo "== 14c. base.sha cannot be read ==" +run_case nobase FINDINGS="$HERE/fx/mixed.json" STUB_BASE=FAIL +report "14c no base sha" +check "no compare" 0 "$(calls compare)" +check "no review call" 0 "$(calls reviews)" +check "ladder ran" 7 "$(calls line-comment)" +check "on_line" 4 "$(out on_line)" +check "on_file" 2 "$(out on_file)" +check "unplaced" 1 "$(out unplaced)" + +echo "== 15. a non-422 failure heads its findings for what they are ==" +run_case headings FINDINGS="$HERE/fx/mixed.json" STUB_REVIEW=500 +report "15 headings on a 500" +check "on-diff heading" 1 "$(grep -c '^\*\*On the changed lines, and not posted\.\*\*' "$CASE/note.md")" +check "off-diff heading" 1 "$(grep -c '^\*\*Observations off the changed lines\.\*\*' "$CASE/note.md")" +check "on-diff group first" true \ + "$([ "$(grep -n 'On the changed lines, and not posted' "$CASE/note.md" | cut -d: -f1)" -lt \ + "$(grep -n 'Observations off the changed lines' "$CASE/note.md" | cut -d: -f1)" ] && echo true || echo false)" +check "4 under on-diff" 4 "$(sed -n '/On the changed lines, and not posted/,/Observations off/p' "$CASE/note.md" | grep -c '^- `')" +check "1 under off-diff" 1 "$(sed -n '/Observations off/,$p' "$CASE/note.md" | grep -c '^- `')" +check "one rule, not two" 1 "$(grep -c '^---$' "$CASE/note.md")" +check "unplaced counts both" 5 "$(out unplaced)" +check "summary total" 5 "$(grep -c '^- `' "$CASE/note.md")" + +echo "== 15b. a 422 ladder puts what it believed on-diff in that group ==" +run_case headings-422 FINDINGS="$HERE/fx/mixed.json" STUB_REVIEW=422 \ + STUB_LINE_OK="$HERE/fx/none.tsv" STUB_FILE_OK="$HERE/fx/none.txt" +report "15b headings on a 422" +check "on-diff heading" 1 "$(grep -c '^\*\*On the changed lines, and not posted\.\*\*' "$CASE/note.md")" +check "4 under on-diff" 4 "$(sed -n '/On the changed lines, and not posted/,/Observations off/p' "$CASE/note.md" | grep -c '^- `')" +check "3 under off-diff" 3 "$(sed -n '/Observations off/,$p' "$CASE/note.md" | grep -c '^- `')" +check "unplaced" 7 "$(out unplaced)" + +echo "== 15c. no on-diff group means no on-diff heading ==" +run_case headings-clean FINDINGS="$HERE/fx/mixed.json" +report "15c only off-diff" +check "no on-diff heading" 0 "$(grep -c 'On the changed lines, and not posted' "$CASE/note.md")" +check "off-diff heading" 1 "$(grep -c '^\*\*Observations off the changed lines\.\*\*' "$CASE/note.md")" +check "one rule" 1 "$(grep -c '^---$' "$CASE/note.md")" + +echo +echo "== 16. a 422 whose body carries no status field ==" +run_case no-status-field FINDINGS="$HERE/fx/mixed.json" STUB_REVIEW=422nostatus +report "16 422 with no status field" +check "one review call" 1 "$(calls reviews)" +check "ladder ran" 4 "$(calls line-comment)" +check "on_line" 4 "$(out on_line)" +check "on_file" 2 "$(out on_file)" +check "unplaced" 1 "$(out unplaced)" +check "refusal warning" 1 "$(grep -c 'refused the review carrying' "$CASE/stdout.txt")" + +echo "== 17. a file the diff carries whose patch the API did not send ==" +run_case no-patch FINDINGS="$HERE/fx/nopatch.json" +report "17 file with no patch" +check "one review call" 1 "$(calls reviews)" +check "only the known line batched" 1 "$(jq '.comments | length' "$CASE/request.json")" +check "batched the right one" 'pkg/a.go:11:RIGHT' \ + "$(jq -r '[.comments[] | "\(.path):\(.line):\(.side)"] | join(" ")' "$CASE/request.json")" +check "the API was asked" 2 "$(calls line-comment)" +check "line accepted where the index could not see" 1 \ + "$(grep -c '^CALL line-comment pkg/huge.go RIGHT 120' "$CASE/calls.log")" +check "on_line" 2 "$(out on_line)" +check "on_file" 1 "$(out on_file)" +check "unplaced" 0 "$(out unplaced)" +check "no false 'outside this diff' on the accepted line" 0 \ + "$(grep -c '^CALL file-comment pkg/huge.go' "$CASE/calls.log")" + +echo +echo "== 18. the diff came back short of the pull request's own file count ==" +# pkg/b.go is in the pull request and missing from this list, which is what a +# truncated compare looks like. Read as the diff, it would put the finding on +# pkg/b.go under a body saying line 2 is outside a diff that adds line 2. +run_case short-list STUB_FILES="$HERE/fx/files-short.json" STUB_CHANGED_FILES=4 +report "18 short file list" +check "no review call" 0 "$(calls reviews)" +check "the ladder ran" 4 "$(calls line-comment)" +check "on_line" 4 "$(out on_line)" +check "on_file" 0 "$(out on_file)" +check "unplaced" 0 "$(out unplaced)" +check "nothing sent to a file" 0 "$(calls file-comment)" +check "no false off-diff body" 0 "$(grep -c "outside this diff's changed lines" "$CASE/bodies.txt")" +check "warning names the shortfall" 1 \ + "$(grep -c 'came back with 3 of its 4 file(s)' "$CASE/stdout.txt")" + +echo "== 19. a diff at the cap whose length the pull request confirms ==" +run_case at-cap STUB_FILES="$GEN/files-at-cap.json" STUB_CHANGED_FILES=300 \ + FINDINGS="$GEN/findings-at-cap.json" +report "19 at the cap, count agrees" +check "one review call" 1 "$(calls reviews)" +check "one comment batched" 1 "$(jq '.comments | length' "$CASE/request.json")" +check "batched the right one" 'pkg/gen7.go:2:RIGHT' \ + "$(jq -r '[.comments[] | "\(.path):\(.line):\(.side)"] | join(" ")' "$CASE/request.json")" +check "on_line" 1 "$(out on_line)" +check "unplaced" 0 "$(out unplaced)" +check "no shortfall warning" 0 "$(grep -c 'came back with' "$CASE/stdout.txt")" + +echo "== 20. a diff at the cap whose true length could not be read ==" +run_case at-cap-unknown STUB_FILES="$GEN/files-at-cap.json" STUB_CHANGED_FILES=NONE \ + FINDINGS="$GEN/findings-at-cap.json" STUB_LINE_OK="$GEN/line-ok-gen.tsv" +report "20 at the cap, count unknown" +check "no review call" 0 "$(calls reviews)" +check "the ladder ran" 1 "$(calls line-comment)" +check "on_line" 1 "$(out on_line)" +check "unplaced" 0 "$(out unplaced)" +check "warning names the cap" 1 \ + "$(grep -c 'which is all this endpoint sends' "$CASE/stdout.txt")" + +echo "== 21. a diff under the cap whose true length could not be read ==" +run_case under-cap-unknown STUB_CHANGED_FILES=NONE +report "21 under the cap, count unknown" +check "one review call" 1 "$(calls reviews)" +check "four comments batched" 4 "$(jq '.comments | length' "$CASE/request.json")" +check "on_line" 4 "$(out on_line)" +check "unplaced" 0 "$(out unplaced)" +check "no cap warning" 0 "$(grep -c 'all this endpoint sends' "$CASE/stdout.txt")" + +echo +echo "== 22. the batch is refused with 413, which an unbounded detail invites ==" +run_case batch-413 FINDINGS="$HERE/fx/mixed.json" STUB_REVIEW=413 +report "22 batch 413" +check "one review call" 1 "$(calls reviews)" +check "four line retries" 4 "$(calls line-comment)" +check "on_line" 4 "$(out on_line)" +check "on_file" 2 "$(out on_file)" +check "unplaced" 1 "$(out unplaced)" +check "warning names the code" 1 \ + "$(grep -c 'refused the review carrying 4 comment(s) with 413' "$CASE/stdout.txt")" + +echo "== 23. the batch is refused with 403 ==" +run_case batch-403 FINDINGS="$HERE/fx/mixed.json" STUB_REVIEW=403 +report "23 batch 403" +check "four line retries" 4 "$(calls line-comment)" +check "on_line" 4 "$(out on_line)" +check "unplaced" 1 "$(out unplaced)" + +echo "== 24. the batch is refused with 400 ==" +run_case batch-400 FINDINGS="$HERE/fx/mixed.json" STUB_REVIEW=400 +report "24 batch 400" +check "four line retries" 4 "$(calls line-comment)" +check "on_line" 4 "$(out on_line)" +check "unplaced" 1 "$(out unplaced)" + +echo "== 25. a 502 keeps its findings off the retry: the write may have landed ==" +run_case batch-502 FINDINGS="$HERE/fx/mixed.json" STUB_REVIEW=502 +report "25 batch 502" +check "one review call" 1 "$(calls reviews)" +check "no line retries" 0 "$(calls line-comment)" +check "on_line" 0 "$(out on_line)" +check "on_file" 2 "$(out on_file)" +check "unplaced" 5 "$(out unplaced)" +check "on-diff heading" 1 \ + "$(grep -c '^\*\*On the changed lines, and not posted\.\*\*' "$CASE/note.md")" + +echo +printf '%s\n' "${rows[@]}" +echo +echo "assertions: $pass passed, $fail failed" +[ "$fail" -eq 0 ]