diff --git a/AGENTS.md b/AGENTS.md index bd40813bf7..99dda6dddf 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -324,6 +324,7 @@ Before deciding any ask-user finding, load `ask-user-authority`; the implementat Never merge a red PR. Without a current explicit captain instruction that states the concrete merge, that default stands, and standing `yolo` cannot authorize a red merge; section 1 owns when such an instruction overrides a Firstmate-written standing rule within its exact scope. Use `bin/fm-pr-merge.sh` for every task PR merge so merge metadata is recorded, and use `bin/fm-merge-local.sh` for approved local-only landing; never call a lower-level merge command around their guards. +`bin/fm-pr-merge.sh` also refuses a merge while an inline review comment's thread is unresolved or a review still requests changes, and it prints that feedback in full; clearing it is a human decision, recorded by rerunning with `--review-comments-override `, and standing `yolo` never authorizes that override. After an autonomous merge, give the captain a one-line full-URL or local-main outcome. ### Validate diff --git a/bin/fm-classify-lib.sh b/bin/fm-classify-lib.sh index 30f0fd027c..51a638cb1c 100755 --- a/bin/fm-classify-lib.sh +++ b/bin/fm-classify-lib.sh @@ -43,9 +43,10 @@ FM_CREW_STATE_BIN="${FM_CREW_STATE_BIN:-$_FM_CLASSIFY_LIB_DIR/fm-crew-state.sh}" # # Free-text tokens (PR ready, checks green, ready in branch, merged) exist only for # legacy lines that lack a standard terminal verb. status_is_captain_relevant is -# verb-aware: a nonterminal working: or paused: line never becomes captain-relevant -# merely because its prose contains one of those tokens (for example -# "working: rebased onto merged #76"). +# verb-aware: a nonterminal working:, note:, or paused: line never becomes +# captain-relevant merely because its prose contains one of those tokens (for +# example "working: rebased onto merged #76", or a note: audit record quoting a +# human's free-text reason). FM_CLASSIFY_CAPTAIN_RE_DEFAULT='done:|needs-decision:|blocked:|failed:|PR ready|checks green|ready in branch|merged' # The deliberate-external-wait verb. A crew (or firstmate steering it) appends @@ -99,16 +100,19 @@ status_is_terminal_verb() { # 0 if the given (last) status line matches a captain-relevant verb. # Verb-aware by default: terminal verbs always match; nonterminal progress verbs -# (working, resolved, captain-held) and paused never match from free-text prose; -# only lines without those leading verbs may still match free-text tokens for -# legacy bare lines such as "merged" or "PR ready". +# (working, resolved, captain-held), the informational note verb, and paused +# never match from free-text prose; only lines without those leading verbs may +# still match free-text tokens for legacy bare lines such as "merged" or +# "PR ready". note: is informational by contract - it is carried by the unread +# status surface and never enters the OPEN DECISIONS fold - so a human's +# free-text reason quoted into a note: audit record cannot fake a decision. status_is_captain_relevant() { local line=$1 verb [ -n "$line" ] || return 1 status_is_paused "$line" && return 1 verb=$(status_line_verb "$line") case "$verb" in - working|resolved|captain-held|"${FM_CLASSIFY_PAUSED_VERB:-$FM_CLASSIFY_PAUSED_VERB_DEFAULT}") + working|resolved|captain-held|note|"${FM_CLASSIFY_PAUSED_VERB:-$FM_CLASSIFY_PAUSED_VERB_DEFAULT}") return 1 ;; esac diff --git a/bin/fm-pr-merge.sh b/bin/fm-pr-merge.sh index 8226798a67..280041d712 100755 --- a/bin/fm-pr-merge.sh +++ b/bin/fm-pr-merge.sh @@ -1,4 +1,6 @@ #!/usr/bin/env bash +# ABOUTME: Records a task's canonical GitHub PR metadata and enforces the review-feedback gate. +# ABOUTME: Merges only past forge-confirmed inline resolution or a logged explicit human override. # Merge a task's PR after recording pr= and any available pr_head= through # bin/fm-pr-check.sh, so teardown can verify landed work after squash merges. # The full canonical GitHub PR URL is parsed by bin/fm-pr-lib.sh and the derived @@ -7,7 +9,9 @@ # Merge method defaults to --squash when the caller passes none of --squash, # --merge, --rebase, or --method after the optional -- separator. Extra args # must not include --repo or -R because the repository comes only from the URL. -# Usage: fm-pr-merge.sh [-- ] +# --review-comments-override requires a non-empty human decision reason. It is +# consumed here, recorded before merge, and never forwarded to the forge. +# Usage: fm-pr-merge.sh [--review-comments-override ] [-- ] set -eu SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" @@ -37,7 +41,49 @@ PR_OWNER=$FM_PR_OWNER PR_REPO=$FM_PR_REPO PR_NUMBER=$FM_PR_NUMBER shift 2 -[ "${1:-}" = "--" ] && shift + +OVERRIDE_SET=0 +OVERRIDE_REASON= +forward_args=() +while [ "$#" -gt 0 ]; do + case "$1" in + --review-comments-override) + if [ "$OVERRIDE_SET" -eq 1 ] || [ "$#" -lt 2 ]; then + echo "error: --review-comments-override requires one non-empty reason" >&2 + exit 2 + fi + OVERRIDE_SET=1 + OVERRIDE_REASON=$2 + shift 2 + ;; + --review-comments-override=*) + if [ "$OVERRIDE_SET" -eq 1 ]; then + echo "error: --review-comments-override may be supplied only once" >&2 + exit 2 + fi + OVERRIDE_SET=1 + OVERRIDE_REASON=${1#*=} + shift + ;; + --) + shift + forward_args+=("$@") + break + ;; + *) + forward_args+=("$1") + shift + ;; + esac +done +set -- "${forward_args[@]+"${forward_args[@]}"}" + +if [ "$OVERRIDE_SET" -eq 1 ]; then + if [ -z "$OVERRIDE_REASON" ] || [[ "$OVERRIDE_REASON" =~ [[:cntrl:]] ]]; then + echo "error: --review-comments-override requires a single-line non-empty reason" >&2 + exit 2 + fi +fi caller_has_merge_method() { local arg @@ -76,6 +122,386 @@ grep -qxF "pr=$URL" "$META" || { exit 1 } +# gh-axi renders every API response as TOON. A table's field positions are +# always read from that table's own header instead of assuming a key order. +TOON_COUNT=0 +TOON_KEYS= + +# Read a TOON table header such as `label[3]{a,b,c}:` into TOON_COUNT/TOON_KEYS. +toon_read_header() { + local line=$1 label=$2 + [[ "$line" =~ ^"$label"\[([0-9]+)\]\{([A-Za-z0-9_]+(,[A-Za-z0-9_]+)*)\}:$ ]] || return 1 + TOON_COUNT=${BASH_REMATCH[1]} + TOON_KEYS=${BASH_REMATCH[2]} + [ "$TOON_COUNT" -ge 1 ] && [ "$TOON_COUNT" -le 100 ] +} + +# Echo the named columns of one TOON data row, tab separated, resolving each +# name through TOON_KEYS so a reordered response cannot shift a field silently. +toon_read_row() { + local row=$1 + shift + local keys=() values=() out= want key index found + case "$row" in + ' '*) row=${row# } ;; + *) return 1 ;; + esac + IFS=, read -r -a keys <<< "$TOON_KEYS" + IFS=, read -r -a values <<< "$row" + [ "${#keys[@]}" -eq "${#values[@]}" ] || return 1 + for want in "$@"; do + found= + index=0 + for key in "${keys[@]}"; do + if [ "$key" = "$want" ]; then + found=${values[$index]} + break + fi + index=$((index + 1)) + done + case "$found" in + \"*\") found=${found#\"}; found=${found%\"} ;; + esac + [ -n "$found" ] || return 1 + [ -z "$out" ] || out+=$'\t' + out+=$found + done + printf '%s\n' "$out" +} + +# Read every page of a paginated TOON list endpoint into FETCHED_RECORDS, one +# tab-separated record per row carrying the named columns in the order asked for. +FETCHED_RECORDS= +fetch_paged_records() { + local endpoint=$1 jq_filter=$2 + shift 2 + local page=1 response rows row record seen + FETCHED_RECORDS= + while :; do + response=$(gh-axi api "$endpoint?per_page=100&page=$page" --jq "$jq_filter") || return 1 + [ "$response" != "[]" ] || return 0 + toon_read_header "${response%%$'\n'*}" '' || return 1 + rows=${response#*$'\n'} + seen=0 + while IFS= read -r row; do + record=$(toon_read_row "$row" "$@") || return 1 + [ -z "$FETCHED_RECORDS" ] || FETCHED_RECORDS+=$'\n' + FETCHED_RECORDS+=$record + seen=$((seen + 1)) + done <<< "$rows" + [ "$seen" -eq "$TOON_COUNT" ] || return 1 + [ "$TOON_COUNT" -eq 100 ] || return 0 + page=$((page + 1)) + done +} + +# The blocking surface is deliberately narrow. It covers inline diff review +# comments from /pulls//comments whose forge thread is still unresolved, +# plus every review /pulls//reviews currently returns in a +# changes-requested state. The merge-past-feedback failure this gate exists to +# stop reached main as an unread inline review comment, which is exactly what +# that surface catches. +# +# Whole-PR conversation comments live on the distinct /issues//comments +# surface. They are fetched and printed in full so a human sees them, but they +# never block: that surface is dominated by automated notices - +# twilwa/session-bored#167 carried a review guide and a preview link - so +# blocking on it would fire on essentially every PR and train humans to override +# reflexively, which destroys the protection the block is here to provide. +# +# Feedback from bots counts exactly as feedback from people does, and severity +# badges in a comment body are never read as authority over what blocks. +INLINE_COMMENTS= +INLINE_API_FAILED=0 +if fetch_paged_records "/repos/$PR_OWNER/$PR_REPO/pulls/$PR_NUMBER/comments" \ + '[.[] | {id, root_id: (.in_reply_to_id // .id), author: .user.login}]' \ + id root_id author; then + INLINE_COMMENTS=$FETCHED_RECORDS +else + INLINE_API_FAILED=1 +fi + +# GitHub returns the full review history in chronological order. COMMENTED and +# PENDING entries do not change a reviewer's standing verdict; APPROVED, +# CHANGES_REQUESTED, and DISMISSED do. Folding that history per reviewer honors +# dismissals and prevents a superseded change request from blocking forever. +CHANGES_REQUESTED_REVIEWS= +REVIEW_API_FAILED=0 +CURRENT_REVIEW_VERDICTS= + +derive_current_review_verdicts() { + local records=$1 review_id review_state review_author extra + local current_id current_state current_author current_extra updated + CURRENT_REVIEW_VERDICTS= + while IFS=$'\t' read -r review_id review_state review_author extra; do + [ -n "$review_id" ] || continue + [ -z "$extra" ] || return 1 + case "$review_id" in + *[!0-9]*) return 1 ;; + esac + [ -n "$review_author" ] || return 1 + case "$review_state" in + COMMENTED|PENDING) + continue + ;; + APPROVED|CHANGES_REQUESTED|DISMISSED) + ;; + *) + return 1 + ;; + esac + + updated= + while IFS=$'\t' read -r current_id current_state current_author current_extra; do + [ -n "$current_id" ] || continue + [ -z "$current_extra" ] || return 1 + [ "$current_author" = "$review_author" ] && continue + [ -z "$updated" ] || updated+=$'\n' + updated+="$current_id"$'\t'"$current_state"$'\t'"$current_author" + done <<< "$CURRENT_REVIEW_VERDICTS" + CURRENT_REVIEW_VERDICTS=$updated + [ -z "$CURRENT_REVIEW_VERDICTS" ] || CURRENT_REVIEW_VERDICTS+=$'\n' + CURRENT_REVIEW_VERDICTS+="$review_id"$'\t'"$review_state"$'\t'"$review_author" + done <<< "$records" +} + +if fetch_paged_records "/repos/$PR_OWNER/$PR_REPO/pulls/$PR_NUMBER/reviews" \ + '[.[] | {id, state, author: .user.login}]' id state author; then + if derive_current_review_verdicts "$FETCHED_RECORDS"; then + while IFS=$'\t' read -r review_id review_state review_author; do + [ -n "$review_id" ] || continue + [ "$review_state" = CHANGES_REQUESTED ] || continue + [ -z "$CHANGES_REQUESTED_REVIEWS" ] || CHANGES_REQUESTED_REVIEWS+=$'\n' + CHANGES_REQUESTED_REVIEWS+="$review_id"$'\t'"$review_author" + done <<< "$CURRENT_REVIEW_VERDICTS" + else + REVIEW_API_FAILED=1 + fi +else + REVIEW_API_FAILED=1 +fi + +# Pull requests are issues on GitHub. This REST endpoint covers whole-PR +# conversation comments, which are not returned by /pulls//comments. +CONVERSATION_COMMENTS= +CONVERSATION_API_FAILED=0 +if fetch_paged_records "/repos/$PR_OWNER/$PR_REPO/issues/$PR_NUMBER/comments" \ + '[.[] | {id, author: .user.login}]' id author; then + CONVERSATION_COMMENTS=$FETCHED_RECORDS +else + CONVERSATION_API_FAILED=1 +fi + +# GitHub's REST review-comment representation has no resolved field. GraphQL's +# review thread is the forge evidence for isResolved and, separately, +# isOutdated. Outdated is never inferred from a missing position or commit. +THREAD_STATES= +THREAD_API_FAILED=0 +if [ "$INLINE_API_FAILED" -eq 0 ] && [ -n "$INLINE_COMMENTS" ]; then + REVIEW_THREADS_QUERY='query($owner: String!, $repo: String!, $number: Int!, $endCursor: String) { + repository(owner: $owner, name: $repo) { + pullRequest(number: $number) { + reviewThreads(first: 100, after: $endCursor) { + nodes { + isResolved + isOutdated + comments(first: 1) { nodes { databaseId } } + } + pageInfo { hasNextPage endCursor } + } + } + } + }' + fetch_thread_states() { + local cursor=null previous_cursor= response end_cursor has_next + local lines=() line index record root_id resolved outdated + THREAD_STATES= + while :; do + response=$(gh-axi api POST graphql \ + --field owner="$PR_OWNER" \ + --field repo="$PR_REPO" \ + --field number="$PR_NUMBER" \ + --field endCursor="$cursor" \ + --field query="$REVIEW_THREADS_QUERY" \ + --jq '{end_cursor: .data.repository.pullRequest.reviewThreads.pageInfo.endCursor, has_next: .data.repository.pullRequest.reviewThreads.pageInfo.hasNextPage, threads: [.data.repository.pullRequest.reviewThreads.nodes[] | {root_id: .comments.nodes[0].databaseId, resolved: .isResolved, outdated: .isOutdated}]}') || return 1 + lines=() + while IFS= read -r line; do + lines+=("$line") + done <<< "$response" + [ "${#lines[@]}" -ge 3 ] || return 1 + case "${lines[0]}" in + 'end_cursor: '*) end_cursor=${lines[0]#end_cursor: } ;; + *) return 1 ;; + esac + case "${lines[1]}" in + 'has_next: true') has_next=true ;; + 'has_next: false') has_next=false ;; + *) return 1 ;; + esac + if [ "${lines[2]}" = 'threads: []' ]; then + TOON_COUNT=0 + else + toon_read_header "${lines[2]}" threads || return 1 + fi + [ "${#lines[@]}" -eq $((3 + TOON_COUNT)) ] || return 1 + index=0 + while [ "$index" -lt "$TOON_COUNT" ]; do + record=$(toon_read_row "${lines[$((3 + index))]}" root_id resolved outdated) || return 1 + IFS=$'\t' read -r root_id resolved outdated <<< "$record" + case "$root_id" in + ''|*[!0-9]*) return 1 ;; + esac + case "$resolved:$outdated" in + true:true|true:false|false:true|false:false) ;; + *) return 1 ;; + esac + [ -z "$THREAD_STATES" ] || THREAD_STATES+=$'\n' + THREAD_STATES+="$root_id"$'\t'"$resolved"$'\t'"$outdated" + index=$((index + 1)) + done + [ "$has_next" = true ] || return 0 + [ "$end_cursor" != null ] && [ -n "$end_cursor" ] \ + && [ "$end_cursor" != "$previous_cursor" ] || return 1 + previous_cursor=$end_cursor + cursor=$end_cursor + done + } + + if ! fetch_thread_states; then + THREAD_API_FAILED=1 + fi +fi + +inline_thread_state() { + local wanted_root=$1 root resolved outdated extra found= + while IFS=$'\t' read -r root resolved outdated extra; do + [ -n "$root" ] || continue + [ "$root" = "$wanted_root" ] || continue + [ -z "$extra" ] || return 1 + case "$resolved:$outdated" in + true:true|true:false|false:true|false:false) ;; + *) return 1 ;; + esac + [ -z "$found" ] || return 1 + found="$resolved:$outdated" + done <<< "$THREAD_STATES" + [ -n "$found" ] || return 1 + printf '%s\n' "$found" +} + +print_feedback() { + local surface=$1 state=$2 author=$3 url=$4 body_path=$5 body_output + printf ' %s (%s) by @%s\n' "$surface" "$state" "$author" >&2 + printf ' %s\n' "$url" >&2 + printf ' body (complete, chunked at 1000 characters):\n' >&2 + if ! body_output=$(gh-axi api "$body_path" \ + --jq '{body_chunks: [range(0; ((.body // "") | length); 1000) as $offset | (.body // "")[$offset:$offset + 1000]]}'); then + echo " [feedback body unavailable because its API request failed]" >&2 + return 1 + fi + printf '%s\n' "$body_output" >&2 +} + +REVIEW_GATE_BLOCKED=0 +if [ "$INLINE_API_FAILED" -eq 1 ]; then + echo "error: could not read inline review comments from /pulls/$PR_NUMBER/comments" >&2 + REVIEW_GATE_BLOCKED=1 +fi +if [ "$REVIEW_API_FAILED" -eq 1 ]; then + echo "error: could not read review states from /pulls/$PR_NUMBER/reviews" >&2 + REVIEW_GATE_BLOCKED=1 +fi +if [ "$CONVERSATION_API_FAILED" -eq 1 ]; then + echo "error: could not read PR conversation comments from /issues/$PR_NUMBER/comments" >&2 + REVIEW_GATE_BLOCKED=1 +fi +if [ "$THREAD_API_FAILED" -eq 1 ]; then + echo "error: could not read inline review thread resolution state" >&2 + REVIEW_GATE_BLOCKED=1 +fi + +if [ "$INLINE_API_FAILED" -eq 0 ] && [ -n "$INLINE_COMMENTS" ]; then + UNRESOLVED_HEADER_PRINTED=0 + while IFS=$'\t' read -r comment_id root_id comment_author extra; do + [ -n "$comment_id" ] || continue + comment_url="$URL#discussion_r$comment_id" + comment_body_path="/repos/$PR_OWNER/$PR_REPO/pulls/comments/$comment_id" + thread_state= + if [ "$THREAD_API_FAILED" -eq 0 ] \ + && thread_state=$(inline_thread_state "$root_id"); then + case "$thread_state" in + true:true|true:false|false:true) + continue + ;; + false:false) + if [ "$UNRESOLVED_HEADER_PRINTED" -eq 0 ]; then + echo "error: unresolved inline review comments block merge:" >&2 + UNRESOLVED_HEADER_PRINTED=1 + fi + print_feedback "inline review comment" "unresolved" \ + "$comment_author" "$comment_url" "$comment_body_path" || true + REVIEW_GATE_BLOCKED=1 + continue + ;; + esac + fi + echo "error: inline review comment resolution state unavailable:" >&2 + print_feedback "inline review comment" "resolution state unavailable" \ + "$comment_author" "$comment_url" "$comment_body_path" || true + REVIEW_GATE_BLOCKED=1 + done <<< "$INLINE_COMMENTS" +fi + +# A reviewer's current changes-requested verdict blocks until that reviewer +# submits a later verdict, the review is dismissed, or a human logs the override. +if [ "$REVIEW_API_FAILED" -eq 0 ] && [ -n "$CHANGES_REQUESTED_REVIEWS" ]; then + echo "error: reviews requesting changes block merge:" >&2 + while IFS=$'\t' read -r review_id review_author extra; do + [ -n "$review_id" ] || continue + review_url="$URL#pullrequestreview-$review_id" + review_body_path="/repos/$PR_OWNER/$PR_REPO/pulls/$PR_NUMBER/reviews/$review_id" + print_feedback "changes-requested review" "changes requested" \ + "$review_author" "$review_url" "$review_body_path" || true + REVIEW_GATE_BLOCKED=1 + done <<< "$CHANGES_REQUESTED_REVIEWS" +fi + +# Conversation comments are surfaced in full for the human but do not block; see +# the blocking-surface rationale above. A body this path cannot retrieve is +# still an unread comment, so an unreadable one falls back to the blocked path. +if [ "$CONVERSATION_API_FAILED" -eq 0 ] && [ -n "$CONVERSATION_COMMENTS" ]; then + echo "PR conversation comments on this PR (informational, not blocking):" >&2 + while IFS=$'\t' read -r comment_id comment_author extra; do + [ -n "$comment_id" ] || continue + comment_url="$URL#issuecomment-$comment_id" + comment_body_path="/repos/$PR_OWNER/$PR_REPO/issues/comments/$comment_id" + print_feedback "PR conversation comment" "informational" \ + "$comment_author" "$comment_url" "$comment_body_path" \ + || REVIEW_GATE_BLOCKED=1 + done <<< "$CONVERSATION_COMMENTS" +fi + +if [ "$REVIEW_GATE_BLOCKED" -eq 1 ]; then + if [ "$OVERRIDE_SET" -eq 0 ]; then + echo "error: merge blocked; after a human decision, rerun with --review-comments-override " >&2 + exit 1 + fi + STATUS="$STATE/$ID.status" + if [ -L "$STATUS" ] || { [ -e "$STATUS" ] && [ ! -f "$STATUS" ]; }; then + echo "error: cannot record the review-comments override in task status" >&2 + exit 1 + fi + umask 077 + if ! printf 'note: merge review-comments override: pr=%s reason=%s\n' \ + "$URL" "$OVERRIDE_REASON" >> "$STATUS"; then + echo "error: cannot record the review-comments override in task status" >&2 + exit 1 + fi + echo "review-comments override recorded; proceeding with the human-authorized merge" >&2 +elif [ "$OVERRIDE_SET" -eq 1 ]; then + echo "note: review-comments override was supplied but no blocking feedback was found; it was not used" >&2 +fi + merge_args=() if ! caller_has_merge_method "$@"; then merge_args=(--squash) diff --git a/docs/architecture.md b/docs/architecture.md index afca3208d7..f4e720262a 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -248,6 +248,10 @@ For target project repos shipped through their own no-mistakes pipeline, commits The firstmate repo itself is the exception: its `.no-mistakes/` directory is local state, stays gitignored, and is rejected by CI if tracked. PR-based task merges go through `bin/fm-pr-merge.sh`, which records `pr=` and any available `pr_head=` through `bin/fm-pr-check.sh` before calling `gh-axi pr merge`. The helper requires a full `https://github.com///pull/` URL, invokes `gh-axi pr merge --repo /`, defaults to `--squash`, preserves explicit merge-method flags, and rejects malformed URLs or repo override flags before recording merge state; a well-formed GitLab merge request URL (see [docs/gitlab-merge-watch.md](gitlab-merge-watch.md)) is refused too, explicitly, rather than sent to the wrong forge. +Between recording metadata and merging it enforces a review-feedback gate whose blocking surface is deliberately narrow: inline diff review comments from `/pulls//comments` whose GraphQL review thread is neither resolved nor outdated, and each reviewer's latest standing verdict from the chronological `/pulls//reviews` history when that verdict is `CHANGES_REQUESTED`; later approvals and dismissals clear earlier change requests, while comments and pending drafts do not replace a standing verdict. +Whole-PR conversation comments from the distinct `/issues//comments` surface are printed in full so a human sees them but never block, because that surface is dominated by automated notices and blocking on it would train humans to override reflexively. +Feedback from bots counts exactly as feedback from people, severity badges in a body are never authority, and any API failure or missing thread-resolution evidence fails closed. +A blocked merge proceeds only when a human reruns it with `--review-comments-override `, which is never inferred, never forwarded to the forge, and is recorded as a `note:` line in the task status before the merge runs. Teardown is fail-closed for ship worktrees: dirty worktrees refuse, and committed work must be landed before the worktree is returned. [`bin/fm-teardown.sh`](../bin/fm-teardown.sh)'s header owns the landed-work proofs, PR-discovery fallback, and stale-lock recovery procedure. diff --git a/docs/scripts.md b/docs/scripts.md index 484911c380..2b83bf026e 100644 --- a/docs/scripts.md +++ b/docs/scripts.md @@ -105,7 +105,7 @@ The shared no-mistakes gate refusal for fleet lifecycle entrypoints is summarize | `fm-pr-poll.sh` | Provide the byte-static watcher program for validated PR/MR-poll sidecars | | `fm-pr-check-migrate.sh` | Quarantine older task polls without execution and rebuild only canonical polls | | `fm-pr-check.sh` | Record validated `pr=` and `pr_head=` values, then atomically arm a static merge poll | -| `fm-pr-merge.sh` | Record PR metadata, then merge a task's canonical full GitHub URL | +| `fm-pr-merge.sh` | Record PR metadata, gate on unresolved inline review feedback, then merge a task's canonical full GitHub URL | | `fm-promote.sh` | Promote a scout task in place to a protected ship task with an explicit delivery mode | | `fm-teardown.sh` | Fail-closed teardown: return landed ship worktrees, require completed scout deliverables, retire secondmate homes | | `fm-harness.sh` | Detect the running harness and resolve crew or secondmate harness, model, and effort | diff --git a/tests/fm-pr-check-security.test.sh b/tests/fm-pr-check-security.test.sh index 03c6ce688e..aaae6c741e 100755 --- a/tests/fm-pr-check-security.test.sh +++ b/tests/fm-pr-check-security.test.sh @@ -85,9 +85,13 @@ case " $* " in ;; esac SH + # Every `api` list read answers with the real empty-collection TOON rendering + # so the merge wrapper's review-feedback gate sees a PR with no feedback rather + # than an unreadable surface it must fail closed on. cat > "$fakebin/gh-axi" <<'SH' #!/usr/bin/env bash printf '%s\n' "$*" >> "$FM_TEST_GH_AXI_LOG" +[ "${1:-}" != api ] || printf '%s\n' '[]' exit "${FM_TEST_GH_AXI_RC:-0}" SH # Plain glab, reproducing the real CLI's contract: its field output on stdout diff --git a/tests/fm-pr-merge.test.sh b/tests/fm-pr-merge.test.sh index a064b6919b..24684d8e2c 100755 --- a/tests/fm-pr-merge.test.sh +++ b/tests/fm-pr-merge.test.sh @@ -1,4 +1,6 @@ #!/usr/bin/env bash +# ABOUTME: Exercises the guarded task-to-GitHub pull request merge entrypoint. +# ABOUTME: Pins metadata, argument, review-feedback, override, and failure behavior. # Tests for bin/fm-pr-merge.sh: the one path firstmate uses to merge a task's # PR, which must always record pr= and any available pr_head= into the task's # meta before merging so fm-teardown.sh's landed-check has a PR reference to @@ -14,6 +16,13 @@ # (f) malformed PR URL fails fast without calling gh-axi # (g) explicit merge method is not overridden by the default --squash # (h) repo override args fail fast because the repo comes from the URL +# (i) unresolved inline comments and current changes-requested reviews block with bodies +# (j) forge-confirmed resolved and outdated inline threads do not block +# (k) whole-PR conversation comments are printed in full but never block +# (l) missing resolution evidence and every surface's API failure fail closed +# (m) an explicit human override is recorded before the merge proceeds +# (n) that override audit record never reads as a captain-relevant decision +# (o) later approvals and dismissals supersede historical change requests set -u # shellcheck source=tests/lib.sh @@ -46,9 +55,10 @@ make_case() { # headRefOid for fm-pr-check.sh's pr_head lookup. Args: case_dir head_sha add_gh_mocks() { local case_dir=$1 head=$2 - cat > "$case_dir/fakebin/gh-axi" <<'SH' +cat > "$case_dir/fakebin/gh-axi" <<'SH' #!/usr/bin/env bash printf '%s\n' "$*" >> "$FM_TEST_GH_AXI_LOG" +[ "${1:-}" != api ] || printf '%s\n' '[]' exit 0 SH cat > "$case_dir/fakebin/gh" <> "$FM_TEST_GH_AXI_LOG" case "${1:-} ${2:-}" in "pr merge") echo "error: pr merge failed" >&2 ; exit 1 ;; esac +[ "${1:-}" != api ] || printf '%s\n' '[]' +exit 0 +SH + cat > "$case_dir/fakebin/gh" <<'SH' +#!/usr/bin/env bash +exit 0 +SH + chmod +x "$case_dir/fakebin/gh-axi" "$case_dir/fakebin/gh" +} + +# gh-axi mock answering each review surface from per-case TOON fixture files. +# The production parser resolves columns from each fixture's header. +add_gh_review_gate_mock() { + local case_dir=$1 + cat > "$case_dir/fakebin/gh-axi" <<'SH' +#!/usr/bin/env bash +printf '%s\n' "$*" >> "$FM_TEST_GH_AXI_LOG" +case "${1:-} ${2:-} ${3:-}" in + "api POST graphql") + [ ! -e "$FM_TEST_CASE_DIR/fail-thread-api" ] || exit 1 + [ ! -f "$FM_TEST_CASE_DIR/thread-states" ] || cat "$FM_TEST_CASE_DIR/thread-states" + exit 0 + ;; +esac +if [ "${1:-}" = api ]; then + case "${2:-}" in + /repos/*/pulls/comments/*) + comment_id=${2##*/} + [ ! -f "$FM_TEST_CASE_DIR/inline-body-$comment_id" ] || cat "$FM_TEST_CASE_DIR/inline-body-$comment_id" + exit 0 + ;; + /repos/*/issues/comments/*) + comment_id=${2##*/} + [ ! -f "$FM_TEST_CASE_DIR/conversation-body-$comment_id" ] || cat "$FM_TEST_CASE_DIR/conversation-body-$comment_id" + exit 0 + ;; + /repos/*/pulls/*/reviews/*) + review_id=${2##*/} + [ ! -f "$FM_TEST_CASE_DIR/review-body-$review_id" ] || cat "$FM_TEST_CASE_DIR/review-body-$review_id" + exit 0 + ;; + /repos/*/pulls/*/reviews*) + [ ! -e "$FM_TEST_CASE_DIR/fail-review-api" ] || exit 1 + [ ! -f "$FM_TEST_CASE_DIR/reviews" ] || cat "$FM_TEST_CASE_DIR/reviews" + [ -f "$FM_TEST_CASE_DIR/reviews" ] || printf '%s\n' '[]' + exit 0 + ;; + /repos/*/pulls/*/comments*) + [ ! -e "$FM_TEST_CASE_DIR/fail-inline-api" ] || exit 1 + [ ! -f "$FM_TEST_CASE_DIR/inline-comments" ] || cat "$FM_TEST_CASE_DIR/inline-comments" + [ -f "$FM_TEST_CASE_DIR/inline-comments" ] || printf '%s\n' '[]' + exit 0 + ;; + /repos/*/issues/*/comments*) + [ ! -e "$FM_TEST_CASE_DIR/fail-conversation-api" ] || exit 1 + [ ! -f "$FM_TEST_CASE_DIR/conversation-comments" ] || cat "$FM_TEST_CASE_DIR/conversation-comments" + [ -f "$FM_TEST_CASE_DIR/conversation-comments" ] || printf '%s\n' '[]' + exit 0 + ;; + esac +fi +case "${1:-} ${2:-}" in + "pr merge") + exit 0 + ;; +esac exit 0 SH cat > "$case_dir/fakebin/gh" <<'SH' @@ -88,6 +164,7 @@ run_pr_merge() { local case_dir=$1 rc; shift FM_ROOT_OVERRIDE="$ROOT" \ FM_STATE_OVERRIDE="$case_dir/state" \ + FM_TEST_CASE_DIR="$case_dir" \ FM_TEST_GH_AXI_LOG="$case_dir/gh-axi.log" \ PATH="$case_dir/fakebin:$PATH" \ "$PR_MERGE" "$@" @@ -117,6 +194,12 @@ test_records_pr_and_head_before_merging() { "records-before-merge: pr= was not recorded" assert_grep 'pr_head=deadbeefcafefeed0000000000000000deadbeef' "$case_dir/state/task-x1.meta" \ "records-before-merge: pr_head= was not recorded" + assert_grep 'api /repos/example/repo/pulls/9/comments?per_page=100&page=1' "$case_dir/gh-axi.log" \ + "records-before-merge: inline review-comment surface was not fetched" + assert_grep 'api /repos/example/repo/issues/9/comments?per_page=100&page=1' "$case_dir/gh-axi.log" \ + "records-before-merge: PR conversation-comment surface was not fetched" + assert_grep 'api /repos/example/repo/pulls/9/reviews?per_page=100&page=1' "$case_dir/gh-axi.log" \ + "records-before-merge: review-state surface was not fetched" grep -qxF 'pr merge 9 --repo example/repo --squash' "$case_dir/gh-axi.log" \ || fail "records-before-merge: gh-axi pr merge was not invoked with number, --repo, and default --squash" pass "fm-pr-merge records pr= and pr_head= before invoking gh-axi pr merge" @@ -141,6 +224,354 @@ test_merge_failure_propagates_after_recording() { pass "fm-pr-merge propagates a real merge failure without silently succeeding" } +test_unresolved_inline_comment_blocks_merge() { + local case_dir rc + case_dir=$(make_case unresolved-inline-comment) + mkdir -p "$case_dir/wt" + add_gh_review_gate_mock "$case_dir" + printf '[1]{id,root_id,author}:\n 501,501,"reviewer-bot[bot]"\n' \ + > "$case_dir/inline-comments" + printf 'end_cursor: null\nhas_next: false\nthreads[1]{root_id,resolved,outdated}:\n 501,false,false\n' \ + > "$case_dir/thread-states" + printf 'body_chunks[1]: "[P1] reviewer permission over-grant"\n' \ + > "$case_dir/inline-body-501" + : > "$case_dir/gh-axi.log" + + set +e + run_pr_merge "$case_dir" task-x1 https://github.com/example/repo/pull/31 \ + > "$case_dir/stdout" 2> "$case_dir/stderr" + rc=$? + set -e + + expect_code 1 "$rc" "unresolved-inline-comment: fm-pr-merge should fail closed" + assert_grep 'unresolved inline review comments block merge' "$case_dir/stderr" \ + "unresolved-inline-comment: refusal did not explain the review-comment gate" + assert_grep '[P1] reviewer permission over-grant' "$case_dir/stderr" \ + "unresolved-inline-comment: refusal summarized away the comment body" + assert_grep 'reviewer-bot[bot]' "$case_dir/stderr" \ + "unresolved-inline-comment: refusal omitted the bot author" + assert_grep 'https://github.com/example/repo/pull/31#discussion_r501' "$case_dir/stderr" \ + "unresolved-inline-comment: refusal omitted the review comment URL" + assert_grep 'isResolved' "$case_dir/gh-axi.log" \ + "unresolved-inline-comment: GraphQL did not request explicit resolved evidence" + assert_grep 'isOutdated' "$case_dir/gh-axi.log" \ + "unresolved-inline-comment: GraphQL did not request separate outdated evidence" + assert_no_grep 'pr merge 31' "$case_dir/gh-axi.log" \ + "unresolved-inline-comment: gh-axi pr merge was invoked" + pass "fm-pr-merge prints and blocks an unresolved inline review comment" +} + +test_resolved_inline_comment_does_not_block() { + local case_dir + case_dir=$(make_case resolved-inline-comment) + mkdir -p "$case_dir/wt" + add_gh_review_gate_mock "$case_dir" + printf '[1]{id,root_id,author}:\n 502,502,reviewer\n' \ + > "$case_dir/inline-comments" + printf 'end_cursor: null\nhas_next: false\nthreads[1]{root_id,resolved,outdated}:\n 502,true,false\n' \ + > "$case_dir/thread-states" + : > "$case_dir/gh-axi.log" + + run_pr_merge "$case_dir" task-x1 https://github.com/example/repo/pull/32 \ + > "$case_dir/stdout" 2> "$case_dir/stderr" \ + || fail "resolved-inline-comment: fm-pr-merge failed" + + assert_grep 'pr merge 32 --repo example/repo --squash' "$case_dir/gh-axi.log" \ + "resolved-inline-comment: resolved thread blocked the merge" + pass "fm-pr-merge permits an inline comment whose GitHub thread is resolved" +} + +test_outdated_inline_comment_does_not_block() { + local case_dir + case_dir=$(make_case outdated-inline-comment) + mkdir -p "$case_dir/wt" + add_gh_review_gate_mock "$case_dir" + printf '[1]{id,root_id,author}:\n 503,503,reviewer\n' \ + > "$case_dir/inline-comments" + printf 'end_cursor: null\nhas_next: false\nthreads[1]{root_id,resolved,outdated}:\n 503,false,true\n' \ + > "$case_dir/thread-states" + : > "$case_dir/gh-axi.log" + + run_pr_merge "$case_dir" task-x1 https://github.com/example/repo/pull/33 \ + > "$case_dir/stdout" 2> "$case_dir/stderr" \ + || fail "outdated-inline-comment: fm-pr-merge failed" + + assert_grep 'pr merge 33 --repo example/repo --squash' "$case_dir/gh-axi.log" \ + "outdated-inline-comment: API-confirmed outdated thread blocked the merge" + pass "fm-pr-merge permits an inline comment whose GitHub thread is outdated" +} + +test_conversation_comment_is_printed_without_blocking() { + local case_dir + case_dir=$(make_case conversation-comment) + mkdir -p "$case_dir/wt" + add_gh_review_gate_mock "$case_dir" + printf '[1]{id,author}:\n 601,"review-bot"\n' \ + > "$case_dir/conversation-comments" + printf 'body_chunks[1]: "Please inspect the authorization boundary"\n' \ + > "$case_dir/conversation-body-601" + : > "$case_dir/gh-axi.log" + + run_pr_merge "$case_dir" task-x1 https://github.com/example/repo/pull/34 \ + > "$case_dir/stdout" 2> "$case_dir/stderr" \ + || fail "conversation-comment: fm-pr-merge failed" + + assert_grep 'Please inspect the authorization boundary' "$case_dir/stderr" \ + "conversation-comment: comment body was summarized away" + assert_grep 'review-bot' "$case_dir/stderr" \ + "conversation-comment: comment author was omitted" + assert_grep 'https://github.com/example/repo/pull/34#issuecomment-601' "$case_dir/stderr" \ + "conversation-comment: comment URL was omitted" + assert_grep 'pr merge 34 --repo example/repo --squash' "$case_dir/gh-axi.log" \ + "conversation-comment: an automated conversation notice blocked the merge" + pass "fm-pr-merge prints PR conversation comments in full without blocking on them" +} + +test_changes_requested_review_blocks_merge() { + local case_dir rc + case_dir=$(make_case changes-requested-review) + mkdir -p "$case_dir/wt" + add_gh_review_gate_mock "$case_dir" + printf '[2]{id,state,author}:\n 701,COMMENTED,earlier-reviewer\n 702,CHANGES_REQUESTED,"strict-bot[bot]"\n' \ + > "$case_dir/reviews" + printf 'body_chunks[1]: "The authorization boundary is still wrong"\n' \ + > "$case_dir/review-body-702" + : > "$case_dir/gh-axi.log" + + set +e + run_pr_merge "$case_dir" task-x1 https://github.com/example/repo/pull/38 \ + > "$case_dir/stdout" 2> "$case_dir/stderr" + rc=$? + set -e + + expect_code 1 "$rc" "changes-requested-review: fm-pr-merge should fail closed" + assert_grep 'reviews requesting changes block merge' "$case_dir/stderr" \ + "changes-requested-review: refusal did not explain the review-state gate" + assert_grep 'The authorization boundary is still wrong' "$case_dir/stderr" \ + "changes-requested-review: review body was summarized away" + assert_grep 'strict-bot[bot]' "$case_dir/stderr" \ + "changes-requested-review: refusal omitted the bot reviewer" + assert_grep 'https://github.com/example/repo/pull/38#pullrequestreview-702' "$case_dir/stderr" \ + "changes-requested-review: refusal omitted the review URL" + assert_no_grep 'earlier-reviewer' "$case_dir/stderr" \ + "changes-requested-review: a review that is not requesting changes was treated as blocking" + assert_no_grep 'pr merge 38' "$case_dir/gh-axi.log" \ + "changes-requested-review: gh-axi pr merge was invoked" + pass "fm-pr-merge blocks on a review the forge still reports as changes-requested" +} + +test_later_approval_supersedes_changes_requested_review() { + local case_dir + case_dir=$(make_case superseded-changes-requested-review) + mkdir -p "$case_dir/wt" + add_gh_review_gate_mock "$case_dir" + printf '[3]{id,state,author}:\n 703,CHANGES_REQUESTED,reviewer\n 704,COMMENTED,reviewer\n 705,APPROVED,reviewer\n' \ + > "$case_dir/reviews" + printf 'body_chunks[1]: "Superseded change request"\n' \ + > "$case_dir/review-body-703" + : > "$case_dir/gh-axi.log" + + run_pr_merge "$case_dir" task-x1 https://github.com/example/repo/pull/43 \ + > "$case_dir/stdout" 2> "$case_dir/stderr" \ + || fail "superseded-changes-requested-review: fm-pr-merge failed" + + assert_no_grep 'Superseded change request' "$case_dir/stderr" \ + "superseded-changes-requested-review: historical verdict still blocked" + assert_grep 'pr merge 43 --repo example/repo --squash' "$case_dir/gh-axi.log" \ + "superseded-changes-requested-review: later approval did not clear the gate" + pass "fm-pr-merge uses each reviewer's latest standing verdict" +} + +test_dismissed_review_remains_nonblocking_after_comment() { + local case_dir + case_dir=$(make_case dismissed-review) + mkdir -p "$case_dir/wt" + add_gh_review_gate_mock "$case_dir" + printf '[2]{id,state,author}:\n 706,DISMISSED,reviewer\n 707,COMMENTED,reviewer\n' \ + > "$case_dir/reviews" + : > "$case_dir/gh-axi.log" + + run_pr_merge "$case_dir" task-x1 https://github.com/example/repo/pull/44 \ + > "$case_dir/stdout" 2> "$case_dir/stderr" \ + || fail "dismissed-review: fm-pr-merge failed" + + assert_grep 'pr merge 44 --repo example/repo --squash' "$case_dir/gh-axi.log" \ + "dismissed-review: dismissed verdict blocked after a later comment" + pass "fm-pr-merge honors dismissed reviews as nonblocking standing verdicts" +} + +test_conversation_api_failure_fails_closed() { + local case_dir rc + case_dir=$(make_case conversation-api-failure) + mkdir -p "$case_dir/wt" + add_gh_review_gate_mock "$case_dir" + : > "$case_dir/fail-conversation-api" + : > "$case_dir/gh-axi.log" + + set +e + run_pr_merge "$case_dir" task-x1 https://github.com/example/repo/pull/39 \ + > "$case_dir/stdout" 2> "$case_dir/stderr" + rc=$? + set -e + + expect_code 1 "$rc" "conversation-api-failure: fm-pr-merge should fail closed" + assert_grep 'could not read PR conversation comments' "$case_dir/stderr" \ + "conversation-api-failure: refusal did not name the failed surface" + assert_no_grep 'pr merge 39' "$case_dir/gh-axi.log" \ + "conversation-api-failure: gh-axi pr merge was invoked" + pass "fm-pr-merge fails closed when the conversation-comment API request fails" +} + +test_review_state_api_failure_fails_closed() { + local case_dir rc + case_dir=$(make_case review-state-api-failure) + mkdir -p "$case_dir/wt" + add_gh_review_gate_mock "$case_dir" + : > "$case_dir/fail-review-api" + : > "$case_dir/gh-axi.log" + + set +e + run_pr_merge "$case_dir" task-x1 https://github.com/example/repo/pull/40 \ + > "$case_dir/stdout" 2> "$case_dir/stderr" + rc=$? + set -e + + expect_code 1 "$rc" "review-state-api-failure: fm-pr-merge should fail closed" + assert_grep 'could not read review states' "$case_dir/stderr" \ + "review-state-api-failure: refusal did not name the failed surface" + assert_no_grep 'pr merge 40' "$case_dir/gh-axi.log" \ + "review-state-api-failure: gh-axi pr merge was invoked" + pass "fm-pr-merge fails closed when the review-state API request fails" +} + +test_thread_resolution_api_failure_fails_closed() { + local case_dir rc + case_dir=$(make_case thread-api-failure) + mkdir -p "$case_dir/wt" + add_gh_review_gate_mock "$case_dir" + printf '[1]{id,root_id,author}:\n 506,506,reviewer\n' \ + > "$case_dir/inline-comments" + : > "$case_dir/fail-thread-api" + : > "$case_dir/gh-axi.log" + + set +e + run_pr_merge "$case_dir" task-x1 https://github.com/example/repo/pull/41 \ + > "$case_dir/stdout" 2> "$case_dir/stderr" + rc=$? + set -e + + expect_code 1 "$rc" "thread-api-failure: fm-pr-merge should fail closed" + assert_grep 'could not read inline review thread resolution state' "$case_dir/stderr" \ + "thread-api-failure: refusal did not name the missing resolution evidence" + assert_no_grep 'pr merge 41' "$case_dir/gh-axi.log" \ + "thread-api-failure: gh-axi pr merge was invoked" + pass "fm-pr-merge fails closed when inline thread resolution state cannot be read" +} + +test_override_audit_line_is_not_captain_relevant() { + local case_dir line + case_dir=$(make_case override-audit-classification) + mkdir -p "$case_dir/wt" + add_gh_review_gate_mock "$case_dir" + printf '[1]{id,root_id,author}:\n 507,507,reviewer\n' \ + > "$case_dir/inline-comments" + printf 'end_cursor: null\nhas_next: false\nthreads[1]{root_id,resolved,outdated}:\n 507,false,false\n' \ + > "$case_dir/thread-states" + printf 'body_chunks[1]: "Human decision required"\n' > "$case_dir/inline-body-507" + : > "$case_dir/gh-axi.log" + + run_pr_merge "$case_dir" task-x1 https://github.com/example/repo/pull/42 \ + --review-comments-override 'reviewer said the branch was already merged' \ + > "$case_dir/stdout" 2> "$case_dir/stderr" \ + || fail "override-audit-classification: fm-pr-merge failed" + + line=$( + . "$ROOT/bin/fm-classify-lib.sh" + last_status_line "$case_dir/state/task-x1.status" + ) + assert_grep 'reviewer said the branch was already merged' "$case_dir/state/task-x1.status" \ + "override-audit-classification: the human's verbatim reason was not preserved" + ( + . "$ROOT/bin/fm-classify-lib.sh" + ! status_is_captain_relevant "$line" + ) || fail "override-audit-classification: the override audit record was classified captain-relevant" + pass "fm-pr-merge's override audit record keeps the reason without faking a captain decision" +} + +test_missing_inline_resolution_state_fails_closed() { + local case_dir rc + case_dir=$(make_case missing-inline-state) + mkdir -p "$case_dir/wt" + add_gh_review_gate_mock "$case_dir" + printf '[1]{id,root_id,author}:\n 504,504,reviewer\n' \ + > "$case_dir/inline-comments" + printf 'end_cursor: null\nhas_next: false\nthreads: []\n' > "$case_dir/thread-states" + printf 'body_chunks[1]: "State unavailable"\n' > "$case_dir/inline-body-504" + : > "$case_dir/gh-axi.log" + + set +e + run_pr_merge "$case_dir" task-x1 https://github.com/example/repo/pull/35 \ + > "$case_dir/stdout" 2> "$case_dir/stderr" + rc=$? + set -e + + expect_code 1 "$rc" "missing-inline-state: fm-pr-merge should fail closed" + assert_grep 'resolution state unavailable' "$case_dir/stderr" \ + "missing-inline-state: refusal did not explain missing forge evidence" + assert_no_grep 'pr merge 35' "$case_dir/gh-axi.log" \ + "missing-inline-state: gh-axi pr merge was invoked" + pass "fm-pr-merge fails closed when GitHub omits an inline thread's state" +} + +test_review_comment_override_is_logged_and_merges() { + local case_dir + case_dir=$(make_case review-comment-override) + mkdir -p "$case_dir/wt" + add_gh_review_gate_mock "$case_dir" + printf '[1]{id,root_id,author}:\n 505,505,reviewer\n' \ + > "$case_dir/inline-comments" + printf 'end_cursor: null\nhas_next: false\nthreads[1]{root_id,resolved,outdated}:\n 505,false,false\n' \ + > "$case_dir/thread-states" + printf 'body_chunks[1]: "Human decision required"\n' > "$case_dir/inline-body-505" + : > "$case_dir/gh-axi.log" + + run_pr_merge "$case_dir" task-x1 https://github.com/example/repo/pull/36 \ + --review-comments-override 'captain reviewed comment 505' \ + > "$case_dir/stdout" 2> "$case_dir/stderr" \ + || fail "review-comment-override: fm-pr-merge failed" + + assert_grep 'note: merge review-comments override: pr=https://github.com/example/repo/pull/36 reason=captain reviewed comment 505' \ + "$case_dir/state/task-x1.status" \ + "review-comment-override: the human decision was not recorded" + assert_grep 'pr merge 36 --repo example/repo --squash' "$case_dir/gh-axi.log" \ + "review-comment-override: explicit override did not reach merge" + assert_no_grep 'review-comments-override' "$case_dir/gh-axi.log" \ + "review-comment-override: private override argument leaked to gh-axi" + pass "fm-pr-merge records an explicit review-comment override before merging" +} + +test_review_comment_api_failure_fails_closed() { + local case_dir rc + case_dir=$(make_case review-comment-api-failure) + mkdir -p "$case_dir/wt" + add_gh_review_gate_mock "$case_dir" + : > "$case_dir/fail-inline-api" + : > "$case_dir/gh-axi.log" + + set +e + run_pr_merge "$case_dir" task-x1 https://github.com/example/repo/pull/37 \ + > "$case_dir/stdout" 2> "$case_dir/stderr" + rc=$? + set -e + + expect_code 1 "$rc" "review-comment-api-failure: fm-pr-merge should fail closed" + assert_grep 'could not read inline review comments' "$case_dir/stderr" \ + "review-comment-api-failure: refusal did not name the failed surface" + assert_no_grep 'pr merge 37' "$case_dir/gh-axi.log" \ + "review-comment-api-failure: gh-axi pr merge was invoked" + pass "fm-pr-merge fails closed when a review-comment API request fails" +} + test_extra_merge_args_forwarded() { local case_dir rc case_dir=$(make_case extra-args) @@ -303,6 +734,20 @@ test_parses_pr_url_for_gh_axi() { test_records_pr_and_head_before_merging test_merge_failure_propagates_after_recording +test_unresolved_inline_comment_blocks_merge +test_resolved_inline_comment_does_not_block +test_outdated_inline_comment_does_not_block +test_conversation_comment_is_printed_without_blocking +test_changes_requested_review_blocks_merge +test_later_approval_supersedes_changes_requested_review +test_dismissed_review_remains_nonblocking_after_comment +test_missing_inline_resolution_state_fails_closed +test_review_comment_override_is_logged_and_merges +test_override_audit_line_is_not_captain_relevant +test_review_comment_api_failure_fails_closed +test_conversation_api_failure_fails_closed +test_review_state_api_failure_fails_closed +test_thread_resolution_api_failure_fails_closed test_extra_merge_args_forwarded test_missing_meta_refuses_before_merge test_malformed_url_refuses_before_merge diff --git a/tests/fm-watch-triage.test.sh b/tests/fm-watch-triage.test.sh index 5c61c16413..476ec056c1 100755 --- a/tests/fm-watch-triage.test.sh +++ b/tests/fm-watch-triage.test.sh @@ -141,6 +141,9 @@ test_signal_reason_is_actionable_classifier() { signal_reason_is_actionable "$state/d.status" || fail "a failed: line was not actionable" printf 'merged\n' > "$state/e.status" signal_reason_is_actionable "$state/e.status" || fail "a legacy merged line was not actionable" + printf 'note: PR ready checks green merged ready in branch failed:\n' > "$state/f.status" + signal_reason_is_actionable "$state/f.status" \ + && fail "an informational note containing legacy wake tokens was actionable" pass "signal_reason_is_actionable: benign absorbed, captain verbs and coalesced batches surfaced" } @@ -188,6 +191,8 @@ test_classifier_primitives() { && fail "working: predecessor prose wrongly recognized as captain-relevant" status_is_captain_relevant "working: PR ready checks green merged ready in branch" \ && fail "working: free-text tokens wrongly recognized as captain-relevant" + status_is_captain_relevant "note: PR ready checks green merged ready in branch failed:" \ + && fail "note: free-text tokens wrongly recognized as captain-relevant" status_is_captain_relevant "done: PR https://x/pull/76 checks green" \ || fail "genuine done: checks green not captain-relevant" status_is_terminal_verb "done: PR https://x/pull/76 checks green" \