From 9357f09aea3862baecd19789915a8f7af4db0d0f Mon Sep 17 00:00:00 2001 From: cluade-reviewer-andresmgsl Date: Sun, 2 Aug 2026 19:58:51 +0000 Subject: [PATCH] fix: the four findings from the panel round on 2168e4e MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit @codex-reviewer-andresmgsl #4780, concurred by @grok-reviewer-andresmgsl #4785. All four real. 1. Three issueflow call sites still named per_page=100. The backend sanitized it so it worked, but the frozen term and the changelog both say no call site names a page size — and a contract that holds only because something downstream cleans up is not the contract. Endpoints now carry their logical query alone. 2. The suite's summary and `[ "$fail" -eq 0 ]` gate sat in the MIDDLE of test/labels-reconcile.test.sh, and the eight outstanding_requests expects were appended after them. Proven before fixing: a deliberately broken term-4 assertion printed FAIL, was excluded from the totals, and the suite still exited 0. Those assertions were decorative. The gate moves to the true end, with a note that nothing goes below it; the reported count goes 157 -> 164, which is the eight that were never being counted. 3. forge_labels_add and forge_request_reviewer arrived with the port and had no boundary pins. Both backends now have them, and the labels_add cases pin the property ceremony#128 turns on: an additive POST, never a PUT of the whole set, exactly one write so nothing is read-modify-written. Mutation-verified — making it RMW/PUT, or routing github through `issue edit --add-label`, each red their own cases. 4. The historical comment said the old gathers were `forge_api graphql`. My own mechanical port rewrote it; before #188 they were `gh api graphql` and the abstraction did not exist. Refs #188 --- .../issueflow-reconcile.sh | 8 +-- test/forge-backends.test.sh | 59 +++++++++++++++++++ test/labels-reconcile.test.sh | 10 +++- 3 files changed, 71 insertions(+), 6 deletions(-) diff --git a/actions/issueflow-reconcile/issueflow-reconcile.sh b/actions/issueflow-reconcile/issueflow-reconcile.sh index 4ece7cc..d6004b8 100644 --- a/actions/issueflow-reconcile/issueflow-reconcile.sh +++ b/actions/issueflow-reconcile/issueflow-reconcile.sh @@ -511,7 +511,7 @@ main() { fi # owner/name split out here until #188 — the GraphQL query took them as # separate variables. REST takes the owner/name path whole, so it is gone. - # Both gathers were `forge_api graphql` until #188. Forgejo has NO GraphQL + # Both gathers were `gh api graphql` until #188. Forgejo has NO GraphQL # API — a real forgejo-runner job even arrives with GITHUB_GRAPHQL_URL set # to the empty string (probe task 278) — so these could not be translated # to a Forgejo endpoint; there is none. They are REST + a parser this repo @@ -523,7 +523,7 @@ main() { # one line and silently loses every declaration after the first. The old # GraphQL gather sidestepped that with `split("\n")[]`; base64 is the same # protection without needing the split to be correct. - OPEN_PR_ISSUES="$(forge_api --paginate "repos/$REPO/pulls?state=open&per_page=100" \ + OPEN_PR_ISSUES="$(forge_api --paginate "repos/$REPO/pulls?state=open" \ --jq '.[] | .body // "" | @base64' \ | while IFS= read -r b64; do [ -n "$b64" ] && printf '%s' "$b64" | base64 -d | closes_references @@ -532,7 +532,7 @@ main() { # meant the CLOSING relation specifically, and reading Refs as closing # would make every referenced issue look closeable — the distinction #151 # was reopened by hand over. - MERGED_REF_PR_RECORDS="$(forge_api --paginate "repos/$REPO/pulls?state=closed&per_page=100" \ + MERGED_REF_PR_RECORDS="$(forge_api --paginate "repos/$REPO/pulls?state=closed" \ --jq '.[] | select(.merged_at != null) | "\(.number)\t\(.body // "" | @base64)"' \ | while IFS=$'\t' read -r pr b64; do [ -n "$b64" ] || continue @@ -542,7 +542,7 @@ main() { done)" local n - for n in $(forge_api --paginate "repos/$REPO/issues?state=open&per_page=100" \ + for n in $(forge_api --paginate "repos/$REPO/issues?state=open" \ --jq '.[] | select(has("pull_request") | not) | .number'); do ( ISSUE_JSON="$(forge_api "repos/$REPO/issues/$n")" diff --git a/test/forge-backends.test.sh b/test/forge-backends.test.sh index c7f15e7..229cdd6 100644 --- a/test/forge-backends.test.sh +++ b/test/forge-backends.test.sh @@ -330,6 +330,65 @@ assignee_stub REPO=o/r forge_issue_edit 5 --add-assignee carol check "adding an assignee keeps the existing ones" 0 "" grep -qE '^PATCH repos/o/r/issues/5 .*"alice".*"bob".*"carol"|^PATCH repos/o/r/issues/5 .*"alice".*"carol".*"bob"' "$WRITES" +# --- forge_labels_add / forge_request_reviewer, both backends ------------ +# @codex-reviewer-andresmgsl #4780 item 3. These two writes came in with the +# call-site port and had no boundary pins of their own. + +# ceremony#128 is the whole reason forge_labels_add exists as its own verb. +# The labeler action computed (labels-at-job-start union derived) and PUT the +# whole set, so a label applied while the job ran was silently removed — +# ceremony#128 lost its `release` label, the merge door's declared-intent +# read, two seconds after the builder set it. This write must therefore be an +# ADDITIVE POST and must never read-modify-write. +FAKE_LABELS='[{"name":"scope:docs","id":21}]' FAKE_LABEL_N=1 stub_writes +FAKE_LABELS='[{"name":"scope:docs","id":21}]' FAKE_LABEL_N=1 \ + REPO=o/r forge_labels_add 7 scope:docs scope:cli +check "labels_add POSTs to the issue labels collection" 0 "" \ + grep -q '^POST repos/o/r/issues/7/labels ' "$WRITES" +check "...carrying every name in one request" 0 "" \ + grep -q '"scope:docs","scope:cli"' "$WRITES" +# The regression that would reopen ceremony#128: any PUT, or a GET-then-write. +check "...and never PUTs the whole set (ceremony#128)" 1 "" grep -q '^PUT ' "$WRITES" +check "...exactly one write, so nothing is read-modify-written" 0 "" \ + test "$(wc -l <"$WRITES")" -eq 1 + +FAKE_LABELS='[]' FAKE_LABEL_N=0 stub_writes +FAKE_LABELS='[]' FAKE_LABEL_N=0 REPO=o/r forge_labels_add 7 +check "labels_add with no labels writes nothing" 0 "" test ! -s "$WRITES" + +# The reviewer payload shape. Measured against this instance: the endpoint +# serves post and delete only, and takes {"reviewers":[...]}. +FAKE_LABELS='[]' FAKE_LABEL_N=0 stub_writes +FAKE_LABELS='[]' FAKE_LABEL_N=0 REPO=o/r forge_request_reviewer 9 danmt +check "request_reviewer POSTs to requested_reviewers" 0 "" \ + grep -q '^POST repos/o/r/pulls/9/requested_reviewers ' "$WRITES" +check "...with the reviewers array payload" 0 "" \ + grep -q '{"reviewers":\["danmt"\]}' "$WRITES" + +# The github twin is a 1:1 gh pass-through (term 5), so its parity is pinned +# by the command it builds rather than by an HTTP shape. +gh_calls="$TMP/ghcalls" +: >"$gh_calls" +# shellcheck disable=SC2317 # invoked indirectly, by the github verbs +gh() { printf '%s\n' "$*" >>"$gh_calls"; } +# A subshell so the github backend does not stay loaded over the forgejo +# cases below; REPO is deliberately scoped to it for the same reason. +( + forge_select github + # shellcheck disable=SC2030 # scoping REPO to this subshell is the point + REPO=o/r + forge_labels_add 7 scope:docs scope:cli + forge_request_reviewer 9 danmt +) +check "github labels_add uses the additive api POST, not issue edit" 0 "" \ + grep -q 'api repos/o/r/issues/7/labels -f labels\[\]=scope:docs -f labels\[\]=scope:cli' "$gh_calls" +check "...and never routes through issue edit --add-label" 1 "" \ + grep -q 'issue edit' "$gh_calls" +check "github request_reviewer posts the reviewer" 0 "" \ + grep -q 'api repos/o/r/pulls/9/requested_reviewers -f reviewers\[\]=danmt' "$gh_calls" +unset -f gh +. "$ROOT/lib/forge-forgejo.sh" + # --- forge_pr_view: newest verdict per context must win ------------------ # checks_state groups repeated contexts and selects the newest by # [.startedAt, .createdAt, .completedAt]. Mapping only {context,state} left diff --git a/test/labels-reconcile.test.sh b/test/labels-reconcile.test.sh index 50e0448..87f4107 100755 --- a/test/labels-reconcile.test.sh +++ b/test/labels-reconcile.test.sh @@ -968,8 +968,6 @@ for ev in schedule pull_request_target; do expect "...and deletes nothing" \ no "$(grep -q '^delete ' "$EXEC/record" && echo yes || echo no)" done -printf 'labels-reconcile tests: %d passed, %d failed\n' "$pass" "$fail" -[ "$fail" -eq 0 ] # --------------------------------------------------------------------------- # outstanding_requests — the portable "who still owes a verdict" (#188 term 4) @@ -1010,3 +1008,11 @@ $BOT3")" expect "on a github-shaped field the filter removes nothing" "nobody" \ "$(outstanding_requests "nobody")" expect "an empty request list stays empty" "" "$(outstanding_requests "")" + +# The summary and the gate belong at the TRUE end of the file. They sat in the +# middle until #188: eight outstanding_requests expects were appended after +# them, so a failure there printed FAIL, was left out of the totals, and the +# suite still exited 0 (@codex-reviewer-andresmgsl #4780 item 2). Anything +# appended below this line is ungated — so nothing goes below it. +printf 'labels-reconcile tests: %d passed, %d failed\n' "$pass" "$fail" +[ "$fail" -eq 0 ]