diff --git a/.github/workflows/seidroid-review.yml b/.github/workflows/seidroid-review.yml index 19a0256..4982564 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,60 +1652,313 @@ 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, 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 + # 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. + # + # $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._" \ + -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 + "$6" "$1" "$2" "$3" "$4" "$5" + 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 + 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" 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" "$summary" + 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 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. + # + # 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. + # 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: (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 + # 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. + # + # 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 + 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 + # 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 - # 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 + 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 - # 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") - # 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 + 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" ] && build_index; then + batched=true + 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 4xx is the API refusing the request and + # it creates nothing, so each finding is posted on its own and the one + # 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 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. + # + # 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="$(sed -n '1s|^HTTP/[0-9.]* \([0-9][0-9][0-9]\).*|\1|p' "$response" || true)" + 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") + place_each on_file_or_summary to_summary < <(rows '.loose[]' "$placement") + else + 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 - # 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. 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 ]