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
|
fi
|
||||||
# owner/name split out here until #188 — the GraphQL query took them as
|
# 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.
|
# 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
|
# 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 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
|
# 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
|
# one line and silently loses every declaration after the first. The old
|
||||||
# GraphQL gather sidestepped that with `split("\n")[]`; base64 is the same
|
# GraphQL gather sidestepped that with `split("\n")[]`; base64 is the same
|
||||||
# protection without needing the split to be correct.
|
# 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' \
|
--jq '.[] | .body // "" | @base64' \
|
||||||
| while IFS= read -r b64; do
|
| while IFS= read -r b64; do
|
||||||
[ -n "$b64" ] && printf '%s' "$b64" | base64 -d | closes_references
|
[ -n "$b64" ] && printf '%s' "$b64" | base64 -d | closes_references
|
||||||
|
|
@ -532,7 +532,7 @@ main() {
|
||||||
# meant the CLOSING relation specifically, and reading Refs as closing
|
# meant the CLOSING relation specifically, and reading Refs as closing
|
||||||
# would make every referenced issue look closeable — the distinction #151
|
# would make every referenced issue look closeable — the distinction #151
|
||||||
# was reopened by hand over.
|
# 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)"' \
|
--jq '.[] | select(.merged_at != null) | "\(.number)\t\(.body // "" | @base64)"' \
|
||||||
| while IFS=$'\t' read -r pr b64; do
|
| while IFS=$'\t' read -r pr b64; do
|
||||||
[ -n "$b64" ] || continue
|
[ -n "$b64" ] || continue
|
||||||
|
|
@ -542,7 +542,7 @@ main() {
|
||||||
done)"
|
done)"
|
||||||
|
|
||||||
local n
|
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
|
--jq '.[] | select(has("pull_request") | not) | .number'); do
|
||||||
(
|
(
|
||||||
ISSUE_JSON="$(forge_api "repos/$REPO/issues/$n")"
|
ISSUE_JSON="$(forge_api "repos/$REPO/issues/$n")"
|
||||||
|
|
|
||||||
|
|
@ -330,6 +330,65 @@ assignee_stub
|
||||||
REPO=o/r forge_issue_edit 5 --add-assignee carol
|
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"
|
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 ------------------
|
# --- forge_pr_view: newest verdict per context must win ------------------
|
||||||
# checks_state groups repeated contexts and selects the newest by
|
# checks_state groups repeated contexts and selects the newest by
|
||||||
# [.startedAt, .createdAt, .completedAt]. Mapping only {context,state} left
|
# [.startedAt, .createdAt, .completedAt]. Mapping only {context,state} left
|
||||||
|
|
|
||||||
|
|
@ -968,8 +968,6 @@ for ev in schedule pull_request_target; do
|
||||||
expect "...and deletes nothing" \
|
expect "...and deletes nothing" \
|
||||||
no "$(grep -q '^delete ' "$EXEC/record" && echo yes || echo no)"
|
no "$(grep -q '^delete ' "$EXEC/record" && echo yes || echo no)"
|
||||||
done
|
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)
|
# 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" \
|
expect "on a github-shaped field the filter removes nothing" "nobody" \
|
||||||
"$(outstanding_requests "nobody")"
|
"$(outstanding_requests "nobody")"
|
||||||
expect "an empty request list stays empty" "" "$(outstanding_requests "")"
|
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