disinto/tests/lib-issue-claim.bats
Claude 9d5cbb4fa2
All checks were successful
ci/woodpecker/push/ci Pipeline was successful
ci/woodpecker/pr/ci Pipeline was successful
ci/woodpecker/pr/smoke-init Pipeline was successful
fix: bug: hire-an-agent does not add the new agent as collaborator on the project repo (#856)
hire-an-agent now adds the new Forgejo user as a `write` collaborator on
`$FORGE_REPO` right after the token step, mirroring the collaborator setup
lib/forge-setup.sh applies to the canonical bot users. Without this, a
freshly hired agent's PATCH to assign itself an issue returned 403 Forbidden
and the dev-agent polled forever logging "claim lost to <none>".

issue_claim() now captures the PATCH HTTP status via `-w '%{http_code}'`
instead of swallowing failures with `curl -sf ... || return 1`. A 403 (or
any non-2xx) now surfaces a distinct log line naming the code — the missing
collaborator root cause would have been diagnosable in seconds instead of
minutes.

Also updates the lib-issue-claim bats mock to handle the new `-w` flag and
adds a regression test covering the HTTP-error log surfacing path.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
2026-04-16 10:47:53 +00:00

212 lines
7.8 KiB
Bash

#!/usr/bin/env bats
# =============================================================================
# tests/lib-issue-claim.bats — Regression guard for the issue_claim TOCTOU
# fix landed in #830.
#
# Before the fix, two dev agents polling concurrently could both observe
# `.assignee == null`, both PATCH the assignee, and Forgejo's last-write-wins
# semantics would leave the loser believing it had claimed successfully.
# Two agents would then implement the same issue and collide at the PR/branch
# stage.
#
# The fix re-reads the assignee after the PATCH and aborts when it doesn't
# match self, with label writes moved AFTER the verification so a losing
# claim leaves no stray `in-progress` label.
#
# These tests stub `curl` with a bash function so each call tree can be
# driven through a specific response sequence (pre-check, PATCH, re-read)
# without a live Forgejo. The stub records every HTTP call to
# `$CALLS_LOG` for assertions.
# =============================================================================
setup() {
ROOT="$(cd "$(dirname "$BATS_TEST_FILENAME")/.." && pwd)"
export FACTORY_ROOT="$ROOT"
export FORGE_TOKEN="dummy-token"
export FORGE_URL="https://forge.example.test"
export FORGE_API="${FORGE_URL}/api/v1"
export CALLS_LOG="${BATS_TEST_TMPDIR}/curl-calls.log"
: > "$CALLS_LOG"
export ISSUE_GET_COUNT_FILE="${BATS_TEST_TMPDIR}/issue-get-count"
echo 0 > "$ISSUE_GET_COUNT_FILE"
# Scenario knobs — overridden per @test.
export MOCK_ME="bot"
export MOCK_INITIAL_ASSIGNEE=""
export MOCK_RECHECK_ASSIGNEE="bot"
# Stand-in for lib/env.sh's forge_api (we don't source env.sh — too
# much unrelated setup). Shape mirrors the real helper closely enough
# that _ilc_ensure_label_id() works.
forge_api() {
local method="$1" path="$2"
shift 2
curl -sf -X "$method" \
-H "Authorization: token ${FORGE_TOKEN}" \
-H "Content-Type: application/json" \
"${FORGE_API}${path}" "$@"
}
# curl shim — parses method + URL out of the argv and dispatches
# canned responses per endpoint. Every call gets logged as
# `METHOD URL` (one line) to $CALLS_LOG for later grep-based asserts.
curl() {
local method="GET" url="" arg want_code=""
while [ $# -gt 0 ]; do
arg="$1"
case "$arg" in
-X) method="$2"; shift 2 ;;
-H|-d|--data-binary|-o) shift 2 ;;
-w) want_code="$2"; shift 2 ;;
-sf|-s|-f|--silent|--fail) shift ;;
*) url="$arg"; shift ;;
esac
done
printf '%s %s\n' "$method" "$url" >> "$CALLS_LOG"
case "$method $url" in
"GET ${FORGE_URL}/api/v1/user")
printf '{"login":"%s"}' "$MOCK_ME"
;;
"GET ${FORGE_API}/issues/"*)
# Distinguish pre-check (first GET) from re-read (subsequent GETs)
# via a counter file that persists across curl invocations in the
# same test.
local n
n=$(cat "$ISSUE_GET_COUNT_FILE")
n=$((n + 1))
echo "$n" > "$ISSUE_GET_COUNT_FILE"
local who
if [ "$n" -eq 1 ]; then
who="$MOCK_INITIAL_ASSIGNEE"
else
who="$MOCK_RECHECK_ASSIGNEE"
fi
if [ -z "$who" ]; then
printf '{"assignee":null}'
else
printf '{"assignee":{"login":"%s"}}' "$who"
fi
;;
"PATCH ${FORGE_API}/issues/"*)
# Accept any PATCH; body ignored. When caller asked for the HTTP
# status via `-w '%{http_code}'` (issue_claim does this since #856
# to surface 403s from missing collaborator permission), emit the
# code configured by the scenario (default 200).
if [ "$want_code" = '%{http_code}' ]; then
printf '%s' "${MOCK_PATCH_CODE:-200}"
fi
;;
"GET ${FORGE_API}/labels")
printf '[]'
;;
"POST ${FORGE_API}/labels")
printf '{"id":99}'
;;
"POST ${FORGE_API}/issues/"*"/labels")
:
;;
"DELETE ${FORGE_API}/issues/"*"/labels/"*)
:
;;
*)
return 1
;;
esac
return 0
}
# shellcheck source=../lib/issue-lifecycle.sh
source "${ROOT}/lib/issue-lifecycle.sh"
}
# ── helpers ──────────────────────────────────────────────────────────────────
# count_calls METHOD URL — count matching lines in $CALLS_LOG.
count_calls() {
local method="$1" url="$2"
grep -cF "${method} ${url}" "$CALLS_LOG" 2>/dev/null || echo 0
}
# ── happy path ───────────────────────────────────────────────────────────────
@test "issue_claim returns 0 when re-read confirms self (no regression, single agent)" {
export MOCK_ME="bot"
export MOCK_INITIAL_ASSIGNEE=""
export MOCK_RECHECK_ASSIGNEE="bot"
run issue_claim 42
[ "$status" -eq 0 ]
# Exactly two GETs to /issues/42 — pre-check and post-PATCH re-read.
[ "$(count_calls GET "${FORGE_API}/issues/42")" -eq 2 ]
# Assignee PATCH fired.
[ "$(count_calls PATCH "${FORGE_API}/issues/42")" -eq 1 ]
# in-progress label added (POST /issues/42/labels).
[ "$(count_calls POST "${FORGE_API}/issues/42/labels")" -eq 1 ]
}
# ── lost race ────────────────────────────────────────────────────────────────
@test "issue_claim returns 1 and leaves no stray in-progress when re-read shows another agent" {
export MOCK_ME="bot"
export MOCK_INITIAL_ASSIGNEE=""
export MOCK_RECHECK_ASSIGNEE="rival"
run issue_claim 42
[ "$status" -eq 1 ]
[[ "$output" == *"claim lost to rival"* ]]
# Re-read happened (two GETs) — this is the new verification step.
[ "$(count_calls GET "${FORGE_API}/issues/42")" -eq 2 ]
# PATCH happened (losers still PATCH before verifying).
[ "$(count_calls PATCH "${FORGE_API}/issues/42")" -eq 1 ]
# CRITICAL: no in-progress label operations on a lost claim.
# (No need to roll back what was never written.)
[ "$(count_calls POST "${FORGE_API}/issues/42/labels")" -eq 0 ]
[ "$(count_calls GET "${FORGE_API}/labels")" -eq 0 ]
}
# ── PATCH HTTP error surfacing (#856) ───────────────────────────────────────
@test "issue_claim logs specific HTTP code on PATCH failure (403 = missing collaborator)" {
export MOCK_ME="bot"
export MOCK_INITIAL_ASSIGNEE=""
export MOCK_RECHECK_ASSIGNEE=""
export MOCK_PATCH_CODE="403"
run issue_claim 42
[ "$status" -eq 1 ]
# The new log message names the HTTP code explicitly — without this,
# a missing-collaborator setup (#856) falls through to the post-PATCH
# verify and masquerades as "claim lost to <none>".
[[ "$output" == *"PATCH assignee failed: HTTP 403"* ]]
# No re-read on PATCH failure (we bail before reaching the verify step).
[ "$(count_calls GET "${FORGE_API}/issues/42")" -eq 1 ]
[ "$(count_calls PATCH "${FORGE_API}/issues/42")" -eq 1 ]
[ "$(count_calls POST "${FORGE_API}/issues/42/labels")" -eq 0 ]
}
# ── pre-check skip ──────────────────────────────────────────────────────────
@test "issue_claim skips early (no PATCH) when pre-check shows another assignee" {
export MOCK_ME="bot"
export MOCK_INITIAL_ASSIGNEE="rival"
export MOCK_RECHECK_ASSIGNEE="rival"
run issue_claim 42
[ "$status" -eq 1 ]
[[ "$output" == *"already assigned to rival"* ]]
# Only the pre-check GET — no PATCH, no re-read, no labels.
[ "$(count_calls GET "${FORGE_API}/issues/42")" -eq 1 ]
[ "$(count_calls PATCH "${FORGE_API}/issues/42")" -eq 0 ]
[ "$(count_calls POST "${FORGE_API}/issues/42/labels")" -eq 0 ]
}