From 21c70e06a470270143385c9506dadce99feae991 Mon Sep 17 00:00:00 2001 From: cluade-reviewer-andresmgsl Date: Tue, 4 Aug 2026 11:53:28 +0000 Subject: [PATCH] fix(forge): an empty REPO cannot become a fact, and the backend verbs are tested MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both panel blockers on c63a550. @kimi found the one that mattered: facts.sh got the REPO fix, release.yml's own four call sites did not. A workflow `run:` shell carries no `set -u`, so an unset REPO expands empty and the verb addresses `repos//…` — which 404s, and the 404 is then read as an ANSWER. Reproduced read-only against this instance before fixing: forge_release_exists 0.4.1 -> "no", rc 0 forge_commit_pulls 7fc9afe4 -> "[]", rc 0 (the !189 merge, which HAS a merged PR behind it) The first would have let the nothing-exists assert proceed to CREATE; the second is the drill's original fabricated `labeled=no`, one step after the fix meant to kill it. Fixed once rather than at four call sites, as kimi suggested: forge_select defaults REPO from GITHUB_REPOSITORY, and forgejo_api_base — which every verb reaches the network through — refuses an empty REPO outright. No fifth call site can forget it. @grok and @kimi both blocked on the same AC gap: the backend suite did not cover the five new verbs, so the two measured asymmetries had no offline coverage. test/forge-backends.test.sh now has 15 cases for them — singular /pull wrapped to an array, 404 as an empty array, 500 refusing, release present/absent/unreadable, POST /tags vs /git/refs, the publish body, and the REPO-empty must-fail. Mutation-checked: reading the plural path fails one case, dropping the REPO guard fails the two must-fails. 1029 assertions, 22 suites, shellcheck-all and actionlint clean. Refs #191 Co-Authored-By: Claude Opus 5 (1M context) --- changelog.d/191.md | 4 ++ lib/forge-forgejo.sh | 24 ++++++++ lib/forge.sh | 6 ++ test/forge-backends.test.sh | 113 ++++++++++++++++++++++++++++++++++++ 4 files changed, 147 insertions(+) diff --git a/changelog.d/191.md b/changelog.d/191.md index 0659f81..89cf80f 100644 --- a/changelog.d/191.md +++ b/changelog.d/191.md @@ -22,3 +22,7 @@ - Forgejo creates tags at `POST /tags` — it serves `/git/refs` GET-only, so GitHub's ref-POST would have 404'd there forever (#191). + +- `forgejo_api_base` refuses when `REPO` is empty. Every verb interpolates + it and every call reaches the network through there, so `repos//…` — + whose 404 reads as "no release" and "no PRs" — is now impossible (#191). diff --git a/lib/forge-forgejo.sh b/lib/forge-forgejo.sh index 94915f1..940751c 100644 --- a/lib/forge-forgejo.sh +++ b/lib/forge-forgejo.sh @@ -18,6 +18,30 @@ forgejo_api_base() { echo "forgejo_api_base: no GITHUB_API_URL or CEREMONY_FORGE_API — cannot reach the forge (#188)" >&2 return 1 fi + # Every verb in this backend interpolates $REPO into its path, and every + # one of them reaches the network through here — so this is the one place + # that can make `repos//…` impossible. + # + # THE TRAP, measured on this instance with REPO unset (#191, caught by + # @kimi on !193 before it shipped): + # + # forge_release_exists 0.4.1 -> "no", rc 0 (repos//releases/tags/0.4.1 + # 404s; a repo-less path read + # as "the release does not + # exist" — and the + # nothing-exists assert would + # then proceed to CREATE) + # forge_commit_pulls -> "[]", rc 0 (a commit that HAS a merged + # PR behind it, read as none) + # + # A workflow `run:` shell carries no `set -u`, so an unset REPO expands + # empty and 404s into a fabricated fact instead of crashing. That is the + # exact failure #191 exists to remove, so it refuses here rather than + # anywhere later. + if [ -z "${REPO:-}" ]; then + echo "forgejo_api_base: REPO is empty — refusing to address 'repos//…', whose 404 would read as a fact (#191)" >&2 + return 1 + fi printf '%s\n' "${base%/}" } diff --git a/lib/forge.sh b/lib/forge.sh index bcf1c26..eda3ffc 100644 --- a/lib/forge.sh +++ b/lib/forge.sh @@ -126,6 +126,12 @@ FORGE_LIB_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" # environment decides via forge_detect. forge_select() { local forge="${1:-}" + # One default for every consumer: the forgejo backend addresses the repo + # through REPO, the github backend reads GITHUB_REPOSITORY. Defaulting + # here means no call site — workflow step or script — can forget it and + # get a repo-less path (#191). The reconcilers still assert their own. + REPO="${REPO:-${GITHUB_REPOSITORY:-}}" + export REPO if [ -z "$forge" ]; then forge="$(forge_detect)" || return 1 fi diff --git a/test/forge-backends.test.sh b/test/forge-backends.test.sh index cc02d1f..2793231 100644 --- a/test/forge-backends.test.sh +++ b/test/forge-backends.test.sh @@ -44,6 +44,12 @@ check "select with no argument reads the environment" 0 "" \ . "$ROOT/lib/forge-github.sh" . "$ROOT/lib/forge-forgejo.sh" +# Every /api/v1 call ceremony makes is repo-scoped, and since #191 the +# backend refuses to build `repos//…` — so the suite names a repo up front, +# the way every real caller does. +REPO=o/r +export REPO + check "github: a bare path gets a query" 0 "" \ eq 'repos/o/r/issues?per_page=100' github_page_url 'repos/o/r/issues' check "github: an existing query is preserved" 0 "" \ @@ -616,4 +622,111 @@ check "...and never fetches a zero-comment review" 1 "" \ check "...and never hits the flat /pulls/{n}/comments endpoint" 1 "" \ grep -E '/pulls/[0-9]+/comments(\?|$)' "$activity_calls" +# --- the release door's verbs, both backends (#191) ----------------------- +# The five verbs the release path now goes through. These carry two measured +# asymmetries that would 404 forever if wrong, and neither is visible to a +# github-only suite: +# +# PRs behind a commit GitHub GET /commits/{sha}/pulls -> ARRAY +# Forgejo GET /commits/{sha}/pull -> ONE OBJECT +# (the plural 404s) +# tag creation GitHub POST /git/refs +# Forgejo POST /tags (/git/refs is GET-only) + +# release_stub — a curl stub answering one canned response and +# recording the method+path it was asked for. +release_stub() { + # Globals, not locals: the curl closure below runs long after this + # function returns, exactly as stub_writes does above. + STUB_CODE="$1" STUB_BODY="$2" + : >"$WRITES" + # shellcheck disable=SC2317 # invoked indirectly, by the forge verbs + curl() { + local hdr="" out="" method=GET url="" payload="" + while [ $# -gt 0 ]; do + case "$1" in + -D) hdr="$2"; shift ;; + -o) out="$2"; shift ;; + -X) method="$2"; shift ;; + -d) payload="$2"; shift ;; + -H | -F) shift ;; + -*) ;; + *) url="$1" ;; + esac + shift + done + [ -n "$hdr" ] && printf 'HTTP/1.1 %s x\r\n\r\n' "$STUB_CODE" >"$hdr" + [ -n "$out" ] && printf '%s' "$STUB_BODY" >"$out" + printf '%s %s %s\n' "$method" "${url##*/api/v1/}" "$payload" >>"$WRITES" + return 0 + } +} + +GITHUB_API_URL=https://forge.example/api/v1 +export GITHUB_API_URL + +# Helpers so the assertions run in THIS shell, where the verbs are defined. +pulls_is_array() { forge_commit_pulls "$1" | jq -e 'type == "array" and length == 1' >/dev/null && echo array-of-1; } +writes_after() { "$@" >/dev/null 2>&1; cat "$WRITES"; } +repo_empty_release() { REPO='' forge_release_exists 1.2.3; } +repo_empty_pulls() { REPO='' forge_commit_pulls deadbeef; } + +release_stub 200 '{"number":7,"merged_at":"2026-01-01T00:00:00Z","labels":[{"name":"release"}]}' +check "forgejo: one PR object becomes a one-element array" 0 '"number":7' \ + forge_commit_pulls deadbeef +check "forgejo: the array is what the call site's jq expects" 0 "array-of-1" \ + pulls_is_array deadbeef +check "forgejo: it reads the SINGULAR path" 0 "commits/deadbeef/pull " \ + writes_after forge_commit_pulls deadbeef + +release_stub 404 '{"message":"pull request does not exist"}' +check "forgejo: 404 is an empty array, not a failure" 0 "[]" forge_commit_pulls deadbeef + +release_stub 500 '{}' +check "forgejo: a 500 refuses rather than saying 'none'" 1 "the answer is unknown, not 'none'" \ + forge_commit_pulls deadbeef + +release_stub 200 '{"tag_name":"1.2.3"}' +check "forgejo: a present release is yes" 0 "yes" forge_release_exists 1.2.3 +release_stub 404 '{}' +check "forgejo: an absent release is no" 0 "no" forge_release_exists 1.2.3 +release_stub 503 '{}' +check "forgejo: an unreadable release refuses, not 'no'" 1 "the answer is unknown, not 'no'" \ + forge_release_exists 1.2.3 + +# THE MUST-FAIL (#191, found by @kimi on !193 before it shipped): with REPO +# empty every path becomes repos//… , whose 404 would read as a fact — "no" +# and "[]" with rc 0. That is the bug this issue exists to remove. +release_stub 404 '{}' +check "REPO empty refuses instead of fabricating 'no'" 1 "refusing to address 'repos//" \ + repo_empty_release +check "REPO empty refuses instead of fabricating '[]'" 1 "refusing to address 'repos//" \ + repo_empty_pulls + +release_stub 201 '{"id":42}' +check "forgejo: a tag is created at /tags, not /git/refs" 0 "POST repos/o/r/tags" \ + writes_after forge_tag_create 1.2.3 cafebabe +release_stub 201 '{"id":42}' +check "forgejo: the tag body names the target sha" 0 '"target":"cafebabe"' \ + writes_after forge_tag_create 1.2.3 cafebabe + +printf 'notes body\n' >"$TMP/notes.md" +release_stub 201 '{"id":42}' +check "forgejo: the publish POSTs to /releases with the notes as body" 0 '"body":"notes body' \ + writes_after forge_release_create 1.2.3 1.2.3 "$TMP/notes.md" + +# --- the github twins address their own paths ---------------------------- +. "$ROOT/lib/forge-github.sh" +GITHUB_REPOSITORY=o/r +export GITHUB_REPOSITORY +GH_CALLS="$TMP/ghcalls" +# shellcheck disable=SC2317 # invoked indirectly, by the forge verbs +gh() { printf '%s\n' "$*" >>"$GH_CALLS"; case "$*" in *commits/*) echo '[]' ;; esac; return 0; } +gh_after() { : >"$GH_CALLS"; "$@" >/dev/null 2>&1; cat "$GH_CALLS"; } + +check "github: the tag goes to /git/refs" 0 "git/refs" \ + gh_after forge_tag_create 1.2.3 cafebabe +check "github: PRs behind a commit use the PLURAL path" 0 "commits/deadbeef/pulls" \ + gh_after forge_commit_pulls deadbeef + summary