From bf318c7d3663ced51a379272ea12ae6113a386c3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micka=C3=ABl=20R=C3=A9mond?= Date: Mon, 21 Sep 2026 14:43:40 +0200 Subject: [PATCH 1/2] fix(bin): require a non-draft pull request before a PR-based done report A PR-based ship could report done, and merge monitoring could be armed, while the pull request was still a draft. A draft cannot be merged, so the poll waited for an event that could not occur and nobody was asked to merge. The PR-based definitions of done now require reading the pull request back from the forge and confirming it is not a draft, and a lane that deliberately holds a draft declares a wait instead of done. bin/fm-pr-check.sh refuses to arm merge monitoring on a draft, naming the draft state, and treats an unreadable draft state as before. The draft reading now lives in bin/fm-pr-lib.sh and bin/fm-pr-merge.sh uses it, with its refusal to merge a draft unchanged. Closes #4757 --- AGENTS.md | 2 +- bin/fm-dod-lib.sh | 15 +++++++++-- bin/fm-pr-check.sh | 16 ++++++++++++ bin/fm-pr-lib.sh | 11 ++++++++ bin/fm-pr-merge.sh | 5 ++-- docs/scripts.md | 2 +- tests/fm-brief.test.sh | 29 +++++++++++++++++++++ tests/fm-pr-check-security.test.sh | 41 ++++++++++++++++++++++++++++++ tests/fm-pr-merge.test.sh | 31 ++++++++++++++++++++++ 9 files changed, 145 insertions(+), 7 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 22613d30afd..44fcb779734 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -391,7 +391,7 @@ The worker reports the PR when CI first becomes green rather than waiting for me ### PR ready, landing, and teardown -For PR-based ship tasks, the ready signal depends on mode: `no-mistakes` reports `done [at=]: PR checks green` after CI is green, while `direct-PR` reports `done [at=]: PR ` after opening the PR. +For PR-based ship tasks, the ready signal depends on mode: `no-mistakes` reports `done [at=]: PR checks green` after CI is green, while `direct-PR` reports `done [at=]: PR ` after opening the PR, each only for a non-draft PR; a lane that deliberately holds a draft declares a wait instead, and `bin/fm-pr-check.sh` refuses to arm merge monitoring on a draft. Run `bin/fm-pr-check.sh ` with the URL copied from that ready signal - it records `pr=` and the forge's `pr_head=` when available in the task's meta and arms the watcher's merge poll. Tell the captain the PR's full `https://...` URL copied from the worker's ready line or the task's `pr=` metadata, a concise outcome summary, and the no-mistakes risk level when applicable. A captain instruction to merge is explicit authority; `yolo` is the only standing routine merge authority. diff --git a/bin/fm-dod-lib.sh b/bin/fm-dod-lib.sh index db70a186f88..d26622c7556 100755 --- a/bin/fm-dod-lib.sh +++ b/bin/fm-dod-lib.sh @@ -10,6 +10,10 @@ # mode is refused rather than silently rendered as the pipeline contract. # The block opens with the fixed machine-readable "Delivery contract: mode=" # line that bin/fm-spawn.sh checks a ship brief against. +# The two PR-based blocks require a non-draft pull request before the done +# report, read back from the forge; a lane that deliberately holds a draft +# declares a paused wait instead. bin/fm-pr-check.sh refuses to arm merge +# monitoring on a draft through the same reading bin/fm-pr-merge.sh uses. # This file is the one owner of the no-mistakes `--intent` contract: only the # brief's `## Captain's intent` subsection plus later captain words, never # `## Firstmate spec` and never the worker's own tradeoffs. @@ -249,7 +253,11 @@ fm_dod_block() { # Delivery contract: mode=direct-PR This task ships **direct-PR**: you raise the PR yourself, without the no-mistakes pipeline. The task is complete only when committed on your branch. -When it is implemented and committed, push your branch and open a PR with \`gh-axi\`, then append \`done [at=]: PR {url}\` to the status file and stop. +When it is implemented and committed, push your branch and open a PR with \`gh-axi\` that is ready for review, not a draft. +Before you report done, read the PR back from the forge and confirm it is not a draft (\`gh pr view --json isDraft\` must print false); if it is a draft, mark it ready with \`gh-axi pr ready\`. +A draft cannot be merged, so a done report on one leaves the merge unasked. +Then append \`done [at=]: PR {url}\` to the status file and stop. +If you deliberately keep the PR a draft, append \`paused [at=]: {why the draft is held}\` instead of done. Do NOT run /no-mistakes. The configured merge authority decides whether to merge the PR; firstmate relays the outcome. EOF ;; @@ -297,7 +305,10 @@ Two firstmate-specific rules layer on top of that guidance: - NEVER pass \`--yes\` (or \`-y\`) to \`no-mistakes axi run\` or \`no-mistakes axi respond\`. It is banned fleet-wide. It auto-resolves every gate including ask-user findings with no escalation, and answering your own ask-user finding is a hard rule violation. -After /no-mistakes reports CI green (the CI-ready return point - do not wait for it to keep monitoring in the background until merge), append \`done [at=]: PR {url} checks green\` and stop. You are finished. +After /no-mistakes reports CI green (the CI-ready return point - do not wait for it to keep monitoring in the background until merge), read the PR back from the forge and confirm it is not a draft (\`gh pr view --json isDraft\` must print false); if it is a draft, mark it ready with \`gh-axi pr ready\`. +A draft cannot be merged, so a done report on one leaves the merge unasked. +Then append \`done [at=]: PR {url} checks green\` and stop. You are finished. +If you deliberately keep the PR a draft, append \`paused [at=]: {why the draft is held}\` instead of done. EOF ;; *) diff --git a/bin/fm-pr-check.sh b/bin/fm-pr-check.sh index c355233fd12..2ad6b180832 100755 --- a/bin/fm-pr-check.sh +++ b/bin/fm-pr-check.sh @@ -5,6 +5,12 @@ # live only in a private sidecar and are never interpolated into shell source. # A GitHub pull request URL and a GitLab merge request URL are both accepted, # including a merge request on a self-hosted GitLab instance. +# A GitHub pull request the forge reports as a draft is refused, naming the draft +# state and recording and arming nothing: a draft cannot be merged, so a poll armed on it +# would wait for an event that cannot occur while nobody is asked to act. +# Mark the pull request ready for review, then arm again; a lane that keeps a +# draft on purpose declares a wait instead of reporting done. An unreadable +# draft state does not refuse, matching how the head read below is optional. # Usage: fm-pr-check.sh set -eu @@ -60,6 +66,16 @@ if [ "$PROVIDER" = gitlab ] && ! command -v glab >/dev/null 2>&1; then exit 1 fi +# The draft state is read before anything is recorded or armed. Only a positive +# draft reading refuses, because an unreadable one must not block arming. +if [ "$PROVIDER" = github ] && command -v gh >/dev/null 2>&1 && command -v jq >/dev/null 2>&1; then + DRAFT_JSON=$(gh pr view "$URL" --json isDraft 2>/dev/null || true) + if [ "$(fm_pr_json_draft_state "$DRAFT_JSON")" = true ]; then + echo "error: $URL is a draft pull request; a draft cannot be merged, so merge monitoring would wait for an event that cannot occur - mark it ready for review and arm again, or declare a wait instead of done if the draft is deliberate" >&2 + exit 1 + fi +fi + "$FM_ROOT/bin/fm-guard.sh" || true # pr_head is recorded only when the forge's CLI can supply it. gh exposes the diff --git a/bin/fm-pr-lib.sh b/bin/fm-pr-lib.sh index 4b97a2f4394..20385f4fb3d 100755 --- a/bin/fm-pr-lib.sh +++ b/bin/fm-pr-lib.sh @@ -217,6 +217,17 @@ fm_pr_head_valid() { [[ "$head" =~ ^[0-9a-f]{40}$|^[0-9a-f]{64}$ ]] } +# The one reading of a GitHub pull request's draft state. Prints "true" or +# "false" for a boolean isDraft and nothing for anything else, so a caller can +# tell a positive draft from an unreadable payload. bin/fm-pr-merge.sh refuses +# a merge unless this prints "false"; bin/fm-pr-check.sh refuses to arm a merge +# poll only when it prints "true". +fm_pr_json_draft_state() { # + printf '%s' "${1-}" | jq -r ' + if type == "object" and (.isDraft | type) == "boolean" then (.isDraft | tostring) else "" end + ' 2>/dev/null || true +} + fm_pr_file_mode() { if [ "$(uname)" = Darwin ]; then /usr/bin/stat -f %Lp "$1" 2>/dev/null diff --git a/bin/fm-pr-merge.sh b/bin/fm-pr-merge.sh index da827f810f9..58dddabfcfa 100755 --- a/bin/fm-pr-merge.sh +++ b/bin/fm-pr-merge.sh @@ -584,7 +584,6 @@ github_verify_mergeable() { if ! fields=$(printf '%s' "$json" | jq -r ' if type == "object" then "state=" + ((.state // "") | tostring), - "draft=" + (if (.isDraft | type) == "boolean" then (.isDraft | tostring) else "" end), "mergeable=" + ((.mergeable // "") | tostring), "merge_state=" + ((.mergeStateStatus // "") | tostring), "head=" + ((.headRefOid // "") | tostring), @@ -599,7 +598,6 @@ github_verify_mergeable() { total=$((total + 1)) case "$line" in state=*) state=${line#state=} ;; - draft=*) draft=${line#draft=} ;; mergeable=*) mergeable=${line#mergeable=} ;; merge_state=*) merge_state=${line#merge_state=} ;; head=*) live_head=${line#head=} ;; @@ -610,11 +608,12 @@ github_verify_mergeable() { done <&2 return 1 fi + draft=$(fm_pr_json_draft_state "$json") if ! fm_pr_head_valid "$live_head"; then echo "error: could not read the GitHub pull request head commit before merging" >&2 return 1 diff --git a/docs/scripts.md b/docs/scripts.md index 1ff6f206419..dba766bed75 100644 --- a/docs/scripts.md +++ b/docs/scripts.md @@ -130,7 +130,7 @@ The shared no-mistakes gate refusal for fleet lifecycle entrypoints is summarize | `fm-pr-lib.sh` | Own canonical task and PR validation plus private atomic PR-poll publication, merge-notification identity, and retirement | | `fm-pr-poll.sh` | Provide the byte-static watcher program for validated PR/MR-poll sidecars | | `fm-contributions.sh` | Observe owned publications, retain exact-head judgments, measure required actors, and wake on maintainer signals | -| `fm-pr-check.sh` | Record validated `pr=` and `pr_head=` values, then atomically arm a static merge poll | +| `fm-pr-check.sh` | Record validated `pr=` and `pr_head=` values, then atomically arm a static merge poll; refuses a GitHub draft | | `fm-pr-merge.sh` | Record PR metadata, merge a task's canonical full GitHub or GitLab URL, then refuse an outcome it cannot prove landed or queued | | `fm-pr-state.sh` | Read-only: print one line per GitHub pull-request blocker it can see, reporting on checks that have reported rather than verdicting merge-readiness | | `fm-pr-reviewers.sh` | Read-only: suggest reviewers from GitHub's own author mapping of recent commits on a pull request's changed files, never requesting one | diff --git a/tests/fm-brief.test.sh b/tests/fm-brief.test.sh index b15dc0bdad2..c41ffd1fcaa 100755 --- a/tests/fm-brief.test.sh +++ b/tests/fm-brief.test.sh @@ -323,6 +323,34 @@ test_faster_paths_use_configured_authority_without_stacked_review() { pass "fm-brief.sh: faster paths use configured authority without stacked review" } +# A PR-based ship must not report done on a draft, which cannot be merged; a +# lane that deliberately holds a draft declares a wait instead. local-only opens +# no PR, so it must not carry the requirement. +test_pr_based_dod_requires_non_draft() { + local home mode id brief + home="$TMP_ROOT/draft-dod-home" + mkdir -p "$home/data" + for mode in no-mistakes direct-PR local-only; do + id="brief-draft-$mode" + FM_HOME="$home" "$ROOT/bin/fm-brief.sh" "$id" some-proj --mode "$mode" >/dev/null 2>&1 + brief="$home/data/$id/brief.md" + assert_present "$brief" "$mode: brief was not scaffolded" + if [ "$mode" = local-only ]; then + assert_no_grep "isDraft" "$brief" "$mode: a branch-only delivery must not require a non-draft PR" + continue + fi + # shellcheck disable=SC2016 # single quotes are deliberate: the backticks must stay literal + assert_grep 'confirm it is not a draft (`gh pr view --json isDraft` must print false)' "$brief" \ + "$mode: done must require reading the PR back from the forge as non-draft" + # shellcheck disable=SC2016 # single quotes are deliberate: the backticks must stay literal + assert_grep 'mark it ready with `gh-axi pr ready`' "$brief" \ + "$mode: a draft must be marked ready before done" + assert_grep "If you deliberately keep the PR a draft, append \`paused" "$brief" \ + "$mode: a deliberate draft must declare a wait instead of done" + done + pass "fm-brief.sh: PR-based done requires a non-draft PR; a deliberate draft declares a wait" +} + # Pin the specific line the bug lived on: the no-mistakes DOD's no-mistakes # reference must render as plain prose with no dangling apostrophe artifact. test_no_mistakes_dod_wording() { @@ -1042,6 +1070,7 @@ test_ship_mode_is_explicit_not_registry test_delivery_flags_are_refused_where_they_do_not_apply test_faster_paths_use_configured_authority_without_stacked_review test_no_mistakes_dod_wording +test_pr_based_dod_requires_non_draft test_ask_user_escalation_format test_ship_project_memory_wording test_herdr_lab_contract_is_explicit_and_complete diff --git a/tests/fm-pr-check-security.test.sh b/tests/fm-pr-check-security.test.sh index c403ea3cae8..9ac386d9019 100755 --- a/tests/fm-pr-check-security.test.sh +++ b/tests/fm-pr-check-security.test.sh @@ -150,6 +150,10 @@ case "${1:-} ${2:-}" in printf '%s\n' "{\"state\":\"OPEN\",\"isDraft\":false,\"mergeable\":\"MERGEABLE\",\"mergeStateStatus\":\"CLEAN\",\"headRefOid\":\"${FM_TEST_GH_HEAD:-0123456789abcdef0123456789abcdef01234567}\",\"baseRefName\":\"main\",\"statusCheckRollup\":[{\"__typename\":\"CheckRun\",\"name\":\"ci\",\"status\":\"COMPLETED\",\"conclusion\":\"SUCCESS\"}]}" exit 0 ;; + *" --json isDraft "*) + printf '%s\n' "{\"isDraft\":${FM_TEST_GH_DRAFT:-false}}" + exit 0 + ;; *headRefOid,reviewDecision*) printf '%s\n' "{\"headRefOid\":\"${FM_TEST_GH_HEAD:-0123456789abcdef0123456789abcdef01234567}\",\"reviewDecision\":\"APPROVED\"}" exit 0 @@ -518,6 +522,42 @@ test_invalid_entrypoints_have_zero_side_effects() { pass "PR and teardown entrypoints reject invalid arguments before every side effect" } +# A draft cannot be merged, so arming a merge poll on one would wait for an event +# that cannot occur. Only a positive draft reading refuses, and it refuses before +# anything is recorded or armed; a ready or unreadable one arms as before. +test_draft_pull_request_is_not_armed() { + local dir rc + dir=$(make_case draft-refused) + write_task_meta "$dir" + cp "$dir/home/state/task-a.meta" "$dir/meta.before" + set +e + FM_TEST_GH_DRAFT=true run_check_entry "$dir" task-a https://github.com/o/r/pull/9 \ + > "$dir/stdout" 2> "$dir/stderr"; rc=$? + set -e + [ "$rc" -ne 0 ] || fail "arming accepted a draft pull request" + grep -qi 'draft' "$dir/stderr" || fail "the refusal did not name the draft state" + grep -qF 'https://github.com/o/r/pull/9' "$dir/stderr" || fail "the refusal did not name the pull request" + cmp -s "$dir/meta.before" "$dir/home/state/task-a.meta" || fail "a refused draft changed the task metadata" + [ ! -e "$dir/home/state/task-a.check.sh" ] || fail "a refused draft armed a poll" + [ ! -e "$dir/home/state/task-a.pr-poll" ] || fail "a refused draft wrote a poll sidecar" + [ ! -s "$dir/guard.log" ] || fail "a refused draft reached the guard" + + dir=$(make_case draft-cleared) + write_task_meta "$dir" + FM_TEST_GH_DRAFT=false run_check_entry "$dir" task-a https://github.com/o/r/pull/9 \ + > "$dir/stdout" 2> "$dir/stderr" || fail "arming refused a pull request that is not a draft" + grep -qxF 'pr=https://github.com/o/r/pull/9' "$dir/home/state/task-a.meta" \ + || fail "a non-draft pull request was not recorded" + [ -f "$dir/home/state/task-a.check.sh" ] || fail "a non-draft pull request was not armed" + + dir=$(make_case draft-unreadable) + write_task_meta "$dir" + FM_TEST_GH_DRAFT=null run_check_entry "$dir" task-a https://github.com/o/r/pull/9 \ + > "$dir/stdout" 2> "$dir/stderr" || fail "an unreadable draft state blocked arming" + [ -f "$dir/home/state/task-a.check.sh" ] || fail "an unreadable draft state was not armed" + pass "arming refuses a draft pull request, naming it, and arms a ready or unreadable one" +} + test_valid_recording_and_merge_derivation() { local dir expected sidecar count rc dir=$(make_case valid-recording) @@ -2774,6 +2814,7 @@ test_retirement_refuses_replacement_and_nonterminal_results test_retirement_queue_failure_and_receipt_tampering test_gitlab_merged_poll_retires test_invalid_entrypoints_have_zero_side_effects +test_draft_pull_request_is_not_armed test_valid_recording_and_merge_derivation test_rejected_metacharacter_bytes_are_inert test_static_poll_contract diff --git a/tests/fm-pr-merge.test.sh b/tests/fm-pr-merge.test.sh index cfa9d4f83af..db541d3a9fb 100755 --- a/tests/fm-pr-merge.test.sh +++ b/tests/fm-pr-merge.test.sh @@ -2404,6 +2404,36 @@ test_github_red_checks_refuse_and_allow_red_waives_named() { pass "fm-pr-merge refuses red GitHub checks and waives only a named --allow-red check" } +# A draft cannot be merged, and neither can a pull request whose draft state the +# forge did not report as a boolean; both refuse before any merge call. +test_github_draft_or_unreadable_draft_state_refuses() { + local case_dir rc head label filter + head=dddddddddddddddddddddddddddddddddddddddd + for label in draft unreadable; do + case_dir=$(make_case "github-$label") + mkdir -p "$case_dir/wt" + add_gh_mocks "$case_dir" "$head" + case "$label" in + draft) filter='.isDraft = true' ;; + *) filter='del(.isDraft)' ;; + esac + jq -c "$filter" "$case_dir/github-view.json" > "$case_dir/github-view.tmp" + mv "$case_dir/github-view.tmp" "$case_dir/github-view.json" + + set +e + run_pr_merge "$case_dir" task-x1 https://github.com/example/repo/pull/82 \ + > "$case_dir/stdout" 2> "$case_dir/stderr" + rc=$? + set -e + expect_code 1 "$rc" "github-$label: a pull request not read as non-draft must refuse" + assert_grep "the pull request is a draft" "$case_dir/stderr" \ + "github-$label: the draft state was not named" + assert_no_grep 'pr merge' "$case_dir/gh.log" \ + "github-$label: gh pr merge ran without a non-draft reading" + done + pass "fm-pr-merge refuses a draft pull request and one with no boolean draft state" +} + # When the base branch advances, GitHub cancels a pull request's in-flight run # and re-triggers it, leaving the cancelled run in the rollup beside the passing # re-run while reporting the pull request itself CLEAN. The merge must follow the @@ -3196,6 +3226,7 @@ test_untraversable_user_backend_config_directory_refuses_the_merge test_absent_user_backend_config_directory_and_backlog_still_merge test_backend_override_bypasses_unreadable_user_config test_github_red_checks_refuse_and_allow_red_waives_named +test_github_draft_or_unreadable_draft_state_refuses test_superseded_failed_check_run_no_longer_refuses test_check_runs_never_supersede_status_contexts test_current_failed_check_run_still_refuses From 454dba3170d3a0cd7dd0c75191fbfa7f9aba53ea Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micka=C3=ABl=20R=C3=A9mond?= Date: Mon, 21 Sep 2026 15:03:39 +0200 Subject: [PATCH 2/2] fix(review): Skip arm-time draft refusal when fm-pr-merge records metadata --- bin/fm-pr-check.sh | 4 +++- bin/fm-pr-merge.sh | 2 +- tests/fm-pr-merge.test.sh | 8 ++++++++ 3 files changed, 12 insertions(+), 2 deletions(-) diff --git a/bin/fm-pr-check.sh b/bin/fm-pr-check.sh index 2ad6b180832..9c65c5084b9 100755 --- a/bin/fm-pr-check.sh +++ b/bin/fm-pr-check.sh @@ -11,6 +11,8 @@ # Mark the pull request ready for review, then arm again; a lane that keeps a # draft on purpose declares a wait instead of reporting done. An unreadable # draft state does not refuse, matching how the head read below is optional. +# bin/fm-pr-merge.sh records through this script with FM_PR_CHECK_MERGE=1 and +# skips this refusal, because its own merge-time draft refusal is authoritative. # Usage: fm-pr-check.sh set -eu @@ -68,7 +70,7 @@ fi # The draft state is read before anything is recorded or armed. Only a positive # draft reading refuses, because an unreadable one must not block arming. -if [ "$PROVIDER" = github ] && command -v gh >/dev/null 2>&1 && command -v jq >/dev/null 2>&1; then +if [ "$PROVIDER" = github ] && [ "${FM_PR_CHECK_MERGE:-}" != 1 ] && command -v gh >/dev/null 2>&1 && command -v jq >/dev/null 2>&1; then DRAFT_JSON=$(gh pr view "$URL" --json isDraft 2>/dev/null || true) if [ "$(fm_pr_json_draft_state "$DRAFT_JSON")" = true ]; then echo "error: $URL is a draft pull request; a draft cannot be merged, so merge monitoring would wait for an event that cannot occur - mark it ready for review and arm again, or declare a wait instead of done if the draft is deliberate" >&2 diff --git a/bin/fm-pr-merge.sh b/bin/fm-pr-merge.sh index 58dddabfcfa..051b6a31323 100755 --- a/bin/fm-pr-merge.sh +++ b/bin/fm-pr-merge.sh @@ -863,7 +863,7 @@ METHODS } record_pr_metadata() { - if ! "$SCRIPT_DIR/fm-pr-check.sh" "$ID" "$URL"; then + if ! FM_PR_CHECK_MERGE=1 "$SCRIPT_DIR/fm-pr-check.sh" "$ID" "$URL"; then return 1 fi grep -qxF "pr=$URL" "$META" || { diff --git a/tests/fm-pr-merge.test.sh b/tests/fm-pr-merge.test.sh index db541d3a9fb..21d297417cb 100755 --- a/tests/fm-pr-merge.test.sh +++ b/tests/fm-pr-merge.test.sh @@ -157,6 +157,10 @@ case "${1:-} ${2:-}" in cat "$FM_TEST_GH_HEAD" exit 0 ;; + *isDraft*) + cat "$FM_TEST_GH_VIEW_JSON" + exit 0 + ;; esac ;; "pr merge") @@ -2430,6 +2434,10 @@ test_github_draft_or_unreadable_draft_state_refuses() { "github-$label: the draft state was not named" assert_no_grep 'pr merge' "$case_dir/gh.log" \ "github-$label: gh pr merge ran without a non-draft reading" + assert_no_grep 'declare a wait instead of done' "$case_dir/stderr" \ + "github-$label: the arm-time draft refusal preempted the merge refusal" + grep -qxF 'pr=https://github.com/example/repo/pull/82' "$case_dir/state/task-x1.meta" \ + || fail "github-$label: pr= was not recorded before the merge refusal" done pass "fm-pr-merge refuses a draft pull request and one with no boolean draft state" }