fix: the four findings from the panel round on 2168e4e
@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
This commit is contained in:
parent
2168e4ef9a
commit
9357f09aea
3 changed files with 71 additions and 6 deletions
|
|
@ -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")"
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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 ]
|
||||
|
|
|
|||
Loading…
Reference in a new issue