From feaf24d87f7a61e58245c3b027411a160c24d940 Mon Sep 17 00:00:00 2001 From: Inthuson Anandakumar Date: Sat, 22 Aug 2026 01:42:24 +0000 Subject: [PATCH 1/5] feat(bin): merge GitLab merge requests through the guarded PR merge path bin/fm-pr-lib.sh already parses a GitLab merge request URL for the watcher, but bin/fm-pr-merge.sh refused every non-github provider, so a merge request had to be merged by hand and got none of the recording, guards, or audit trail a pull request gets. The merge path now dispatches on the parsed provider. A GitHub URL keeps its exact previous behavior. A GitLab URL is addressed through glab by the project URL rebuilt from the parsed host and path, so a merge request on any instance resolves and no host is hardcoded, and no merge-method flag is added because the project's own merge method is what should apply. A GitLab merge happens only after one live read of the merge request confirms it is open, detailed_merge_status is mergeable, has_conflicts is false, blocking_discussions_resolved is true, and the head pipeline succeeded at the exact current head. Every failing condition is reported, not just the first. The verified head is bound to the merge with glab's --sha, so a push landing between the read and the merge fails the merge instead of landing commits nothing verified. Recorded metadata is never the authority for any of this: a rebase moves the head and leaves a recorded value stale, so a recorded head that disagrees with the live one is reported rather than trusted, and the recorded value is read before the recording step because that step drops a GitLab head it cannot resolve. --- bin/fm-pr-check.sh | 2 + bin/fm-pr-lib.sh | 4 +- bin/fm-pr-merge.sh | 206 +++++++++++-- docs/architecture.md | 7 +- docs/gitlab-merge-watch.md | 115 +++++++- docs/scripts.md | 2 +- tests/fm-pr-check-security.test.sh | 13 +- tests/fm-pr-merge.test.sh | 456 ++++++++++++++++++++++++++++- 8 files changed, 768 insertions(+), 37 deletions(-) diff --git a/bin/fm-pr-check.sh b/bin/fm-pr-check.sh index 96cb14dc938..dea5e34e7b9 100755 --- a/bin/fm-pr-check.sh +++ b/bin/fm-pr-check.sh @@ -71,6 +71,8 @@ fi # bin/fm-teardown.sh reads the head from the forge at teardown rather than from # metadata and falls back to its provider-agnostic content check, and # bin/fm-review-diff.sh resolves the head from the remote when none is recorded. +# bin/fm-pr-merge.sh reads a GitLab head live at merge time for the same reason, +# and treats a recorded value that disagrees as stale rather than authoritative. WT=$(grep '^worktree=' "$META" | tail -1 | cut -d= -f2- || true) PR_HEAD= if [ "$PROVIDER" = github ] && [ -n "$WT" ] && [ -d "$WT" ] && command -v gh >/dev/null 2>&1; then diff --git a/bin/fm-pr-lib.sh b/bin/fm-pr-lib.sh index b70d8468894..b8ea9eb8fd8 100755 --- a/bin/fm-pr-lib.sh +++ b/bin/fm-pr-lib.sh @@ -163,8 +163,8 @@ fm_pr_gitlab_path_valid() { # # FM_PR_OWNER and FM_PR_REPO are additionally set for github because # bin/fm-pr-merge.sh addresses GitHub by owner/repository. A gitlab URL leaves -# them empty; teaching the merge path about GitLab is a separate change, and -# until then it refuses a GitLab URL rather than merging anything. +# them empty, and that path addresses the project by FM_PR_HOST and FM_PR_PATH +# instead, so a merge request on any instance resolves without a hardcoded host. fm_pr_url_parse() { local raw=${1-} pattern host path local LC_ALL=C diff --git a/bin/fm-pr-merge.sh b/bin/fm-pr-merge.sh index 8226798a673..845e99cba29 100755 --- a/bin/fm-pr-merge.sh +++ b/bin/fm-pr-merge.sh @@ -1,13 +1,33 @@ #!/usr/bin/env bash -# Merge a task's PR after recording pr= and any available pr_head= through +# Merge a task's PR or MR 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 -# owner/repository and PR number are passed to gh-axi as separate arguments. +# The full canonical URL is parsed by bin/fm-pr-lib.sh. A GitHub pull request is +# addressed through gh-axi by the derived owner and repository; a GitLab merge +# request is addressed through glab by the project URL rebuilt from the parsed +# host and path, so any instance works and no host is hardcoded. # -# 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 [-- ] +# Merge method on GitHub defaults to --squash when the caller passes none of +# --squash, --merge, --rebase, or --method after the optional -- separator. +# GitLab adds no method flag at all: its merge method is the project's own +# setting, which the merge API applies, and imposing squash there would override +# that convention rather than mirror the GitHub default. +# +# A GitLab merge is refused unless every pre-merge condition holds, each read +# live at merge time rather than taken from recorded metadata: the merge request +# is open, detailed_merge_status is mergeable, has_conflicts is false, +# blocking_discussions_resolved is true, and the head pipeline succeeded at the +# exact current head commit. Every failing condition is reported, not just the +# first. The verified head is then passed to glab as --sha, so a push that lands +# between that read and the merge fails the merge instead of landing commits +# nothing verified. A recorded pr_head that disagrees with the live head is +# reported rather than trusted, because a rebase moves the head and leaves the +# recorded value stale. Reading that state needs glab and jq, and either one +# absent stops the merge before any state is recorded. +# +# Extra args must not include --repo or -R because the repository comes only +# from the URL, nor --sha on GitLab because the head comes only from the live +# read. +# Usage: fm-pr-merge.sh [-- ] set -eu SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" @@ -24,18 +44,18 @@ if [ "$#" -lt 2 ]; then fi ID=$1 RAW_URL=$2 -# bin/fm-pr-lib.sh parses GitLab merge request URLs so the watcher can follow -# them, but this path still addresses only GitHub by owner/repository. The -# provider check holds that refusal exactly as it was until merge parity lands. -if ! fm_pr_task_id_valid "$ID" || ! fm_pr_url_parse "$RAW_URL" \ - || [ "$FM_PR_PROVIDER" != github ]; then +if ! fm_pr_task_id_valid "$ID" || ! fm_pr_url_parse "$RAW_URL"; then echo "error: invalid PR merge request" >&2 exit 2 fi URL=$FM_PR_URL +PROVIDER=$FM_PR_PROVIDER PR_OWNER=$FM_PR_OWNER PR_REPO=$FM_PR_REPO PR_NUMBER=$FM_PR_NUMBER +# glab resolves the instance from the project URL passed to -R, so the host is +# rebuilt from the parsed identity rather than read from any ambient default. +PROJECT_URL="https://$FM_PR_HOST/$FM_PR_PATH" shift 2 [ "${1:-}" = "--" ] && shift @@ -61,7 +81,20 @@ reject_repo_overrides() { done } +reject_head_overrides() { + local arg + for arg in "$@"; do + case "$arg" in + --sha|--sha=*) + echo "error: extra merge arguments must not override the head commit" >&2 + return 1 + ;; + esac + done +} + reject_repo_overrides "$@" || exit 1 +[ "$PROVIDER" != gitlab ] || reject_head_overrides "$@" || exit 1 # Task-derived paths are constructed only after the canonical ID validation. META="$STATE/$ID.meta" @@ -70,15 +103,154 @@ if [ ! -f "$META" ] || [ -L "$META" ]; then exit 1 fi +# Reading the merge request state needs both tools. Report them together and +# before anything is recorded, so a missing tool is a named prerequisite rather +# than a merge that is armed and then refused for an unexplained reason. +GITLAB_MISSING= +if [ "$PROVIDER" = gitlab ]; then + command -v glab >/dev/null 2>&1 || GITLAB_MISSING="glab" + if ! command -v jq >/dev/null 2>&1; then + GITLAB_MISSING="${GITLAB_MISSING:+$GITLAB_MISSING and }jq" + fi + if [ -n "$GITLAB_MISSING" ]; then + echo "error: merging a GitLab merge request requires $GITLAB_MISSING on PATH" >&2 + exit 1 + fi +fi + +# The recorded head is read before bin/fm-pr-check.sh rewrites the metadata, +# because that script re-records pr= and drops a pr_head= it cannot resolve. +RECORDED_HEAD= +if [ "$PROVIDER" = gitlab ]; then + RECORDED_HEAD=$(grep '^pr_head=' "$META" | tail -1 | cut -d= -f2- || true) +fi + "$SCRIPT_DIR/fm-pr-check.sh" "$ID" "$URL" grep -qxF "pr=$URL" "$META" || { echo "error: PR metadata recording failed" >&2 exit 1 } -merge_args=() -if ! caller_has_merge_method "$@"; then - merge_args=(--squash) -fi +# Pre-merge conditions for a GitLab merge request, read from one live view of +# the merge request. Sets FM_PR_MERGE_HEAD to the verified head on success and +# returns non-zero after reporting every condition that failed. +FM_PR_MERGE_HEAD= +gitlab_verify_mergeable() { + local json fields line + local total=0 named=0 refusals='' + local state='' detail='' conflicts='' discussions='' + local live_head='' pipeline_sha='' pipeline_status='' + + # GITLAB_HOST is set to the same host the project URL already carries, so the + # instance is taken from the parsed URL by both signals and never from the + # operator's configured default. + if ! json=$(GITLAB_HOST="$FM_PR_HOST" glab mr view "$PR_NUMBER" -R "$PROJECT_URL" -F json 2>/dev/null) \ + || [ -z "$json" ]; then + echo "error: could not read the GitLab merge request state before merging" >&2 + return 1 + fi + # One named field per line. The names keep a trailing empty value readable + # after command substitution strips blank lines, and an absent or null field + # becomes an empty string or the literal "null", neither of which satisfies any + # check below, so an unreadable field refuses the merge instead of passing it. + if ! fields=$(printf '%s' "$json" | jq -r ' + if type == "object" then + "state=" + ((.state // "") | tostring), + "detail=" + ((.detailed_merge_status // "") | tostring), + "conflicts=" + (.has_conflicts | tostring), + "discussions=" + (.blocking_discussions_resolved | tostring), + "head=" + ((.sha // "") | tostring), + "pipeline_sha=" + ((.head_pipeline.sha // "") | tostring), + "pipeline_status=" + ((.head_pipeline.status // "") | tostring) + else + error("merge request payload is not an object") + end' 2>/dev/null); then + echo "error: could not read the GitLab merge request state before merging" >&2 + return 1 + fi + while IFS= read -r line; do + total=$((total + 1)) + case "$line" in + state=*) state=${line#state=} ;; + detail=*) detail=${line#detail=} ;; + conflicts=*) conflicts=${line#conflicts=} ;; + discussions=*) discussions=${line#discussions=} ;; + head=*) live_head=${line#head=} ;; + pipeline_sha=*) pipeline_sha=${line#pipeline_sha=} ;; + pipeline_status=*) pipeline_status=${line#pipeline_status=} ;; + *) continue ;; + esac + named=$((named + 1)) + done <&2 + return 1 + fi + + if ! fm_pr_head_valid "$live_head"; then + echo "error: could not read the GitLab merge request head commit before merging" >&2 + return 1 + fi + # A rebase moves the head and leaves the recorded value behind, so the + # disagreement is reported and the live head is what gets verified and merged. + if [ -n "$RECORDED_HEAD" ] && [ "$RECORDED_HEAD" != "$live_head" ]; then + printf 'notice: recorded head %s disagrees with the live head %s; verifying the live head\n' \ + "$RECORDED_HEAD" "$live_head" >&2 + fi + + [ "$state" = opened ] \ + || refusals="$refusals - state is \"${state:-unreadable}\", not open +" + [ "$detail" = mergeable ] \ + || refusals="$refusals - detailed_merge_status is \"${detail:-unreadable}\", not mergeable +" + [ "$conflicts" = false ] \ + || refusals="$refusals - has_conflicts is \"${conflicts:-unreadable}\", not false +" + [ "$discussions" = true ] \ + || refusals="$refusals - blocking_discussions_resolved is \"${discussions:-unreadable}\", not true +" + [ "$pipeline_status" = success ] \ + || refusals="$refusals - the head pipeline status is \"${pipeline_status:-none}\", not success +" + [ "$pipeline_sha" = "$live_head" ] \ + || refusals="$refusals - the head pipeline ran at \"${pipeline_sha:-none}\", not at the current head $live_head +" + + if [ -n "$refusals" ]; then + printf 'error: refusing to merge %s\n' "$URL" >&2 + printf '%s' "$refusals" >&2 + return 1 + fi + printf 'verified: %s is open and mergeable, with a successful pipeline at head %s\n' \ + "$URL" "$live_head" >&2 + FM_PR_MERGE_HEAD=$live_head +} -gh-axi pr merge "$PR_NUMBER" --repo "$PR_OWNER/$PR_REPO" "${merge_args[@]+"${merge_args[@]}"}" "$@" +case "$PROVIDER" in + github) + merge_args=() + if ! caller_has_merge_method "$@"; then + merge_args=(--squash) + fi + gh-axi pr merge "$PR_NUMBER" --repo "$PR_OWNER/$PR_REPO" "${merge_args[@]+"${merge_args[@]}"}" "$@" + ;; + gitlab) + gitlab_verify_mergeable || exit 1 + # --sha binds the merge to the head this run verified, so a push that lands + # in between is refused by GitLab instead of merged unverified. --yes only + # skips the interactive confirmation, which no supervised run can answer; + # the conditions above are what authorize the merge. + GITLAB_HOST="$FM_PR_HOST" glab mr merge "$PR_NUMBER" -R "$PROJECT_URL" \ + --sha "$FM_PR_MERGE_HEAD" --yes "$@" + ;; + *) + echo "error: invalid PR merge request" >&2 + exit 2 + ;; +esac diff --git a/docs/architecture.md b/docs/architecture.md index f5857d56010..e47997dfe48 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -253,8 +253,11 @@ A ship brief records its mode as a fixed machine-readable line and the spawn ref When a selected delivery path calls for a diff, `bin/fm-review-diff.sh` refreshes the authoritative base and, when task meta records `pr=`, always fetches and compares against `refs/pull//head` by default (recorded `pr_head=` is only an offline fallback) before falling back to the local branch with a warning. Where a no-mistakes pipeline stores evidence in the repo, it publishes that PR-viewable validation evidence to an orphan evidence branch that shares no history with code branches, so it never enters the crew branch or the default branch. This repo uses that setting, and its own `.no-mistakes/` directory remains local state that stays gitignored and is rejected by CI if tracked; [`configuration.md`](configuration.md) owns the setting. -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. +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 the forge CLI. +The helper requires a full canonical URL and rejects malformed URLs or repo override flags before recording merge state. +A `https://github.com///pull/` URL invokes `gh-axi pr merge --repo /`, defaults to `--squash`, and preserves explicit merge-method flags. +A `https:////-/merge_requests/` URL (see [docs/gitlab-merge-watch.md](gitlab-merge-watch.md)) invokes `glab mr merge -R https:///`, so the instance comes from the URL, and adds no merge-method flag because the project's own merge method applies. +That path merges only after one live read of the merge request confirms it is open, mergeable, conflict-free, with blocking discussions resolved and a successful pipeline at the current head, and it binds the merge to that verified head; recorded metadata is never the authority for those conditions because a rebase leaves it stale. 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/gitlab-merge-watch.md b/docs/gitlab-merge-watch.md index 0540ed296d1..79dc138e1f6 100644 --- a/docs/gitlab-merge-watch.md +++ b/docs/gitlab-merge-watch.md @@ -1,7 +1,8 @@ -# GitLab merge request watch verification +# GitLab merge request watch and merge verification -Empirical record for the merge watch on GitLab, alongside the existing GitHub watch. -Every command below was run on 2026-07-21 and its output is reproduced exactly. +Empirical record for the merge watch and the merge path on GitLab, alongside the existing GitHub ones. +Every command through "Upgrade path from an existing armed watch" was run on 2026-07-21; "Merging a merge request" was run on 2026-08-22. +Every output is reproduced exactly. ## Versions @@ -13,6 +14,21 @@ $ bash --version | head -1 GNU bash, version 5.3.9(1)-release (x86_64-pc-linux-gnu) ``` +The merge evidence dated 2026-08-22 was collected on a different host, on: + +``` +$ glab --version +glab 1.82.0- () + +$ jq --version +jq-1.8.1 + +$ bash --version | head -1 +GNU bash, version 5.2.15(1)-release (x86_64-amazon-linux-gnu) +``` + +That `glab` is a locally built 1.82.0; only its build tag and commit are elided, because they name a private build rather than a released version. + ## The evidence project All live evidence here reads , a public project that exists only to be this evidence. @@ -190,11 +206,92 @@ merged No armed watch is lost by upgrading. -## What this change does not cover +## Merging a merge request + +`bin/fm-pr-merge.sh` now merges a GitLab merge request through the same recording and the same guards a GitHub pull request gets. +Every run below used a throwaway `FM_HOME`, so no live task record was touched, and a `glab` wrapper that refused any `merge` subcommand outright, so no merge could reach the forge even if a check were wrong. +That wrapper is why the open fixture merge request could be used as evidence at all: it is `mergeable` with discussions resolved, so the pipeline conditions are the only thing between it and a real merge. + +Merging needs `glab` for the read and `jq` to parse it, and either one absent refuses before anything is recorded: + +``` +$ PATH="$noglab" fm-pr-merge.sh e5 https://gitlab.com/KarotKris/gitlab-merge-watch-fixture/-/merge_requests/2 +error: merging a GitLab merge request requires glab on PATH +$ echo $? +1 +$ PATH="$nojq" fm-pr-merge.sh e6 https://gitlab.com/KarotKris/gitlab-merge-watch-fixture/-/merge_requests/2 +error: merging a GitLab merge request requires jq on PATH +$ echo $? +1 +``` + +Neither refusal armed a poll or recorded a `pr=`, so a missing tool leaves no half-prepared merge behind. + +`jq` is not one of firstmate's common tools, which is why the watch poll reads glab's field output instead. +The merge path cannot do the same: `detailed_merge_status`, `has_conflicts`, `blocking_discussions_resolved`, and the head pipeline appear only in glab's JSON. +The poll's silence on a missing tool is safe because silence means "not merged yet"; a merge cannot be silent about it, so the requirement is reported rather than assumed. + +The merged half of the fixture is refused, and every failing condition is listed rather than just the first: + +``` +$ fm-pr-merge.sh e1 https://gitlab.com/KarotKris/gitlab-merge-watch-fixture/-/merge_requests/1 +armed: state/e1.check.sh +error: refusing to merge https://gitlab.com/KarotKris/gitlab-merge-watch-fixture/-/merge_requests/1 + - state is "merged", not open + - detailed_merge_status is "not_open", not mergeable + - the head pipeline status is "none", not success + - the head pipeline ran at "none", not at the current head 33762fcf6777c8d993220d25fb541e56c48081b9 +$ echo $? +1 +``` + +The open half is `mergeable`, conflict-free, and has its discussions resolved, so only the pipeline conditions refuse it. +The fixture runs no CI, so its `head_pipeline` is `null`, which is reported as `none` rather than treated as nothing to check: + +``` +$ fm-pr-merge.sh e2 https://gitlab.com/KarotKris/gitlab-merge-watch-fixture/-/merge_requests/2 +armed: state/e2.check.sh +error: refusing to merge https://gitlab.com/KarotKris/gitlab-merge-watch-fixture/-/merge_requests/2 + - the head pipeline status is "none", not success + - the head pipeline ran at "none", not at the current head 66b8a6777bea5e291d7fa2fc20c42ad7686f6bc8 +$ echo $? +1 +``` + +A project that runs no pipeline at all therefore cannot merge through this path. +That is the intended reading of the requirement rather than an oversight: a successful pipeline at the head is a condition, and "there is no pipeline" does not satisfy it. + +Both refusals came after `pr=` was recorded and the merge poll was armed, exactly as a failing `gh-axi pr merge` does on the GitHub side, so a refusal still leaves the audit trail and the watch in place. + +A recorded `pr_head=` that no longer matches the live head is reported, and the live head is what gets verified. +The stale value below was written into the task record by hand, because a GitLab task never records one on its own: + +``` +$ fm-pr-merge.sh e4 https://gitlab.com/KarotKris/gitlab-merge-watch-fixture/-/merge_requests/2 +armed: state/e4.check.sh +notice: recorded head 1111111111111111111111111111111111111111 disagrees with the live head 66b8a6777bea5e291d7fa2fc20c42ad7686f6bc8; verifying the live head +error: refusing to merge https://gitlab.com/KarotKris/gitlab-merge-watch-fixture/-/merge_requests/2 + - the head pipeline status is "none", not success + - the head pipeline ran at "none", not at the current head 66b8a6777bea5e291d7fa2fc20c42ad7686f6bc8 +``` + +The remaining refusal conditions, and the merge itself, are covered by `tests/fm-pr-merge.test.sh` against fixtures. +The conflict, unresolved-discussion, and running-pipeline conditions were additionally exercised against real merge requests on a private instance; those runs cannot be reproduced here, so their identifiers stay out of this record. +The merge itself is not exercised against any live merge request, in either direction: `glab mr merge` has no dry run, so a live success path would mean merging someone's work to produce evidence. + +## Why the head is read live and bound to the merge + +The verified head is passed to `glab mr merge --sha`, so GitLab refuses the merge if the source branch moved between the read and the merge. +Without it, a push landing in that window would merge commits nothing verified. + +`--yes` is passed for the same reason the watch poll needs no terminal: an unattended run cannot answer a confirmation prompt, and a wedged prompt is worse than a refusal. +It skips only that prompt; the conditions above are what authorize the merge. + +## Why a recorded head is not the authority -`bin/fm-pr-merge.sh` still addresses GitHub only, by owner and repository. -It refuses a GitLab merge request URL rather than sending it to the wrong forge, so merging a merge request stays a deliberate manual step until merge parity lands separately. +`bin/fm-pr-check.sh` records `pr_head=` only for GitHub, where `gh` exposes the head commit as a selectable field. +It is optional by design, and the other consumers already treat it that way: `bin/fm-teardown.sh` reads the head from the forge at teardown and falls back to its provider-agnostic content check, and `bin/fm-review-diff.sh` resolves the head from the remote when none is recorded. -A GitLab task records no `pr_head=`. -`gh` exposes the head commit as a selectable field, while plain `glab` exposes it only inside its JSON output, which would need a JSON processor firstmate does not require. -Both consumers already treat it as optional: `bin/fm-teardown.sh` reads the head from the forge at teardown rather than from metadata and falls back to its provider-agnostic content check, and `bin/fm-review-diff.sh` resolves the head from the remote when none is recorded. +The merge path does not record one either, and deliberately does not depend on one. +A rebase moves the head and leaves any recorded value stale, so a merge decided from metadata can verify a commit that no longer exists. +Reading the head live at merge time, reporting a recorded value that disagrees, and binding the merge to what was actually verified is what closes that gap. diff --git a/docs/scripts.md b/docs/scripts.md index 9f219592d79..41e61973dc0 100644 --- a/docs/scripts.md +++ b/docs/scripts.md @@ -108,7 +108,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, then merge a task's canonical full GitHub or GitLab 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 03c6ce688e3..fee25bfbd91 100755 --- a/tests/fm-pr-check-security.test.sh +++ b/tests/fm-pr-check-security.test.sh @@ -2886,15 +2886,22 @@ EOF esac [ ! -e "$state/task-b.check.sh" ] || fail "refused GitLab arming left a poll armed" - # The merge path still addresses GitHub only, so it refuses rather than - # sending a merge request to the wrong forge. + # The merge path addresses the forge the URL names, and never the other one. + # This fixture's glab answers with the field output the poll reads, so the + # merge's JSON read cannot be parsed, which must refuse rather than merge on a + # state it could not read. write_task_meta "$dir" task-c + : > "$dir/glab.log" set +e run_merge_entry "$dir" task-c "$url" >/dev/null 2>&1 rc=$? set -e - [ "$rc" -eq 2 ] || fail "merge wrapper did not refuse a GitLab merge request URL" + [ "$rc" -ne 0 ] || fail "merge wrapper merged a GitLab merge request it could not read" [ ! -s "$dir/gh-axi.log" ] || fail "merge wrapper reached the GitHub CLI for a GitLab URL" + grep -qF "mr view 7 -R https://gitlab.example/group/subgroup/project" "$dir/glab.log" \ + || fail "merge wrapper did not read the merge request through glab at its own instance" + ! grep -qF ' mr merge ' "$dir/glab.log" \ + || fail "merge wrapper merged despite an unreadable merge request state" pass "GitLab merge requests are followed on any instance and never wake falsely" } diff --git a/tests/fm-pr-merge.test.sh b/tests/fm-pr-merge.test.sh index a064b6919bc..9f1d05e9b37 100755 --- a/tests/fm-pr-merge.test.sh +++ b/tests/fm-pr-merge.test.sh @@ -14,6 +14,15 @@ # (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) a GitLab MR URL resolves and merges through glab instead of erroring +# (j) glab is addressed by the host from the URL, never an assumed one +# (k) no merge method is imposed on GitLab, so the project's own one applies +# (l) each pre-merge condition refuses independently, and all of them report +# (m) a stale recorded pr_head= is reported and the live head is verified +# (n) an unreadable merge request state refuses rather than merging blind +# (o) glab or jq absent refuses before any state is recorded +# (p) --sha in extra GitLab args fails fast, and still forwards on GitHub +# (q) a GitLab refusal still leaves pr= recorded and the merge poll armed set -u # shellcheck source=tests/lib.sh @@ -22,6 +31,18 @@ fm_git_identity fmtest fmtest@example.invalid PR_MERGE="$ROOT/bin/fm-pr-merge.sh" TMP_ROOT=$(fm_test_tmproot fm-pr-merge-tests) +BASE_PATH=$PATH + +# The GitLab fixture. A placeholder host that resolves nowhere, and a namespace +# deeper than one group, because a GitLab project has no owner/repository pair. +MR_HOST=gitlab.example +MR_PATH=group/subgroup/project +MR_PROJECT_URL="https://$MR_HOST/$MR_PATH" +MR_URL="$MR_PROJECT_URL/-/merge_requests/7" +MR_HEAD=aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa +MR_STALE_HEAD=bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb + +JQ_BIN=$(command -v jq) || fail "these tests read glab's JSON with the real jq, which was not found" # Build a fresh sandbox for one test case: a state dir with a task meta and a # fakebin with a gh-axi mock that records how it was invoked. Echoes the case dir. @@ -84,11 +105,114 @@ SH chmod +x "$case_dir/fakebin/gh-axi" "$case_dir/fakebin/gh" } +# glab mock recording every invocation together with the GITLAB_HOST it was +# given, so a test can prove the instance came from the URL. `mr view` answers +# from the case's JSON payload; marker files in the case dir drive the failure +# modes, so no test has to leak environment into a shared runner. +add_glab_mock() { + local case_dir=$1 + cat > "$case_dir/fakebin/glab" <<'SH' +#!/usr/bin/env bash +printf 'GITLAB_HOST=%s %s\n' "${GITLAB_HOST-}" "$*" >> "$FM_TEST_GLAB_LOG" +case_dir=$(dirname "$FM_TEST_GLAB_JSON") +case "${1:-} ${2:-}" in + "mr view") + [ ! -e "$case_dir/glab-view-fails" ] || exit 1 + cat "$FM_TEST_GLAB_JSON" + exit 0 + ;; + "mr merge") + [ ! -e "$case_dir/glab-merge-fails" ] || { echo "error: mr merge failed" >&2 ; exit 1 ; } + exit 0 + ;; +esac +exit 0 +SH + chmod +x "$case_dir/fakebin/glab" + ln -sf "$JQ_BIN" "$case_dir/fakebin/jq" +} + +# write_mr_json [= ...] +# A merge request payload that satisfies every pre-merge condition, with the +# named fields overridden so one case drives exactly one condition. Values are +# written into the JSON as-is, so a value may carry a JSON escape. +write_mr_json() { + local file=$1 kv key value + local state=opened detail=mergeable conflicts=false discussions=true + local head=$MR_HEAD pipeline_sha=$MR_HEAD pipeline_status=success pipeline=present + shift + for kv in "$@"; do + key=${kv%%=*} + value=${kv#*=} + case "$key" in + state) state=$value ;; + detail) detail=$value ;; + conflicts) conflicts=$value ;; + discussions) discussions=$value ;; + head) head=$value ;; + pipeline_sha) pipeline_sha=$value ;; + pipeline_status) pipeline_status=$value ;; + pipeline) pipeline=$value ;; + *) fail "write_mr_json: unknown field '$key'" ;; + esac + done + if [ "$pipeline" = present ]; then + pipeline=$(printf '{"sha":"%s","status":"%s"}' "$pipeline_sha" "$pipeline_status") + fi + printf '{"iid":7,"state":"%s","detailed_merge_status":"%s","has_conflicts":%s,' \ + "$state" "$detail" "$conflicts" > "$file" + printf '"blocking_discussions_resolved":%s,"sha":"%s","head_pipeline":%s}\n' \ + "$discussions" "$head" "$pipeline" >> "$file" +} + +# make_gitlab_case [= ...]: a case dir with both forge +# mocks and a merge request payload. Echoes the case dir. +make_gitlab_case() { + local name=$1 case_dir + shift + case_dir=$(make_case "$name") + mkdir -p "$case_dir/wt" + add_gh_mocks "$case_dir" cccccccccccccccccccccccccccccccccccccccc + add_glab_mock "$case_dir" + : > "$case_dir/gh-axi.log" + : > "$case_dir/glab.log" + write_mr_json "$case_dir/mr.json" "$@" + printf '%s\n' "$case_dir" +} + +# mirror_path_without : the whole search path re-exposed by symlink +# except one tool, because a real copy anywhere on PATH would prove nothing. +mirror_path_without() { + local dir=$1 omit=$2 bindir entry name + mkdir -p "$dir" + while IFS= read -r bindir; do + [ -d "$bindir" ] || continue + for entry in "$bindir"/*; do + [ -e "$entry" ] || continue + name=${entry##*/} + [ "$name" = "$omit" ] && continue + [ -e "$dir/$name" ] || ln -s "$entry" "$dir/$name" 2>/dev/null + done + done </dev/null 2>&1 \ + || fail "the $omit-free search path still resolved $omit" +} + +# The merge line glab was asked to run, so a test asserts one exact invocation +# rather than a substring of the whole log. +glab_merge_line() { + grep -F ' mr merge ' "$1" || true +} + run_pr_merge() { local case_dir=$1 rc; shift FM_ROOT_OVERRIDE="$ROOT" \ FM_STATE_OVERRIDE="$case_dir/state" \ FM_TEST_GH_AXI_LOG="$case_dir/gh-axi.log" \ + FM_TEST_GLAB_LOG="$case_dir/glab.log" \ + FM_TEST_GLAB_JSON="$case_dir/mr.json" \ PATH="$case_dir/fakebin:$PATH" \ "$PR_MERGE" "$@" rc=$? @@ -187,15 +311,18 @@ test_malformed_url_refuses_before_merge() { : > "$case_dir/gh-axi.log" set +e - run_pr_merge "$case_dir" task-x1 'https://gitlab.com/example/repo/-/merge_requests/1' \ + # A near-miss GitLab URL: one namespace segment where a project needs at + # least two. A well-formed merge request URL is merged now, so the refusal + # has to be proven on a URL that genuinely does not parse. + run_pr_merge "$case_dir" task-x1 'https://gitlab.com/example/-/merge_requests/1' \ > "$case_dir/stdout" 2> "$case_dir/stderr" rc=$? set -e - expect_code 2 "$rc" "malformed-url: fm-pr-merge should refuse a non-GitHub PR URL" + expect_code 2 "$rc" "malformed-url: fm-pr-merge should refuse a malformed merge request URL" assert_grep 'error: invalid PR merge request' "$case_dir/stderr" \ "malformed-url: refusal was not fixed and non-probing" - assert_no_grep 'pr=https://gitlab.com/example/repo/-/merge_requests/1' "$case_dir/state/task-x1.meta" \ + assert_no_grep 'pr=https://gitlab.com/example/-/merge_requests/1' "$case_dir/state/task-x1.meta" \ "malformed-url: malformed PR URL was recorded in meta" assert_absent "$case_dir/state/task-x1.check.sh" \ "malformed-url: malformed PR URL armed a merge poll" @@ -301,6 +428,316 @@ test_parses_pr_url_for_gh_axi() { pass "fm-pr-merge parses a GitHub PR URL into gh-axi number and --repo arguments" } +test_gitlab_url_resolves_and_merges() { + local case_dir rc merge_line + case_dir=$(make_gitlab_case gitlab-merges) + + set +e + run_pr_merge "$case_dir" task-x1 "$MR_URL" \ + > "$case_dir/stdout" 2> "$case_dir/stderr" + rc=$? + set -e + + expect_code 0 "$rc" "gitlab-merges: a well-formed merge request URL should merge, not error" + assert_grep "pr=$MR_URL" "$case_dir/state/task-x1.meta" \ + "gitlab-merges: pr= was not recorded before merging" + assert_grep "GITLAB_HOST=$MR_HOST mr view 7 -R $MR_PROJECT_URL -F json" "$case_dir/glab.log" \ + "gitlab-merges: the pre-merge state was not read from the project URL" + merge_line=$(glab_merge_line "$case_dir/glab.log") + [ "$merge_line" = "GITLAB_HOST=$MR_HOST mr merge 7 -R $MR_PROJECT_URL --sha $MR_HEAD --yes" ] \ + || fail "gitlab-merges: unexpected merge invocation: '$merge_line'" + assert_grep "successful pipeline at head $MR_HEAD" "$case_dir/stderr" \ + "gitlab-merges: the verified head was not reported" + [ ! -s "$case_dir/gh-axi.log" ] || fail "gitlab-merges: a merge request reached the GitHub CLI" + pass "fm-pr-merge merges a GitLab merge request through glab instead of refusing it" +} + +test_gitlab_host_comes_from_the_url() { + local case_dir rc host path project_url url + host=gl.self-hosted.example + path=deep/nested/group/project + project_url="https://$host/$path" + url="$project_url/-/merge_requests/31" + case_dir=$(make_gitlab_case gitlab-host-from-url) + + set +e + run_pr_merge "$case_dir" task-x1 "$url" \ + > "$case_dir/stdout" 2> "$case_dir/stderr" + rc=$? + set -e + + expect_code 0 "$rc" "gitlab-host-from-url: a self-hosted merge request should merge" + assert_grep "GITLAB_HOST=$host mr view 31 -R $project_url -F json" "$case_dir/glab.log" \ + "gitlab-host-from-url: the read did not use the host from the URL" + assert_grep "GITLAB_HOST=$host mr merge 31 -R $project_url" "$case_dir/glab.log" \ + "gitlab-host-from-url: the merge did not use the host from the URL" + assert_no_grep 'gitlab.com' "$case_dir/glab.log" \ + "gitlab-host-from-url: a host was assumed instead of taken from the URL" + assert_no_grep '' "$case_dir/glab.log" \ + "gitlab-host-from-url: glab was left to resolve the instance from its own default" + pass "fm-pr-merge takes the GitLab instance from the URL rather than assuming one" +} + +test_gitlab_imposes_no_merge_method() { + local case_dir rc merge_line flag + case_dir=$(make_gitlab_case gitlab-no-method) + + set +e + run_pr_merge "$case_dir" task-x1 "$MR_URL" \ + > "$case_dir/stdout" 2> "$case_dir/stderr" + rc=$? + set -e + + expect_code 0 "$rc" "gitlab-no-method: merge should succeed" + merge_line=$(glab_merge_line "$case_dir/glab.log") + for flag in --squash --rebase --merge --method; do + case "$merge_line" in + *"$flag"*) fail "gitlab-no-method: '$flag' was imposed on GitLab: '$merge_line'" ;; + esac + done + pass "fm-pr-merge imposes no merge method on GitLab, leaving the project's own one" +} + +test_gitlab_extra_args_forwarded() { + local case_dir rc merge_line + case_dir=$(make_gitlab_case gitlab-extra-args) + + set +e + run_pr_merge "$case_dir" task-x1 "$MR_URL" -- --remove-source-branch \ + > "$case_dir/stdout" 2> "$case_dir/stderr" + rc=$? + set -e + + expect_code 0 "$rc" "gitlab-extra-args: merge should succeed" + merge_line=$(glab_merge_line "$case_dir/glab.log") + [ "$merge_line" = "GITLAB_HOST=$MR_HOST mr merge 7 -R $MR_PROJECT_URL --sha $MR_HEAD --yes --remove-source-branch" ] \ + || fail "gitlab-extra-args: extra glab flags were not forwarded: '$merge_line'" + pass "fm-pr-merge forwards extra flags to glab mr merge after the -- separator" +} + +test_gitlab_merge_failure_propagates() { + local case_dir rc + case_dir=$(make_gitlab_case gitlab-merge-fails) + : > "$case_dir/glab-merge-fails" + + set +e + run_pr_merge "$case_dir" task-x1 "$MR_URL" \ + > "$case_dir/stdout" 2> "$case_dir/stderr" + rc=$? + set -e + + expect_code 1 "$rc" "gitlab-merge-fails: a failing glab merge should not report success" + assert_grep "pr=$MR_URL" "$case_dir/state/task-x1.meta" \ + "gitlab-merge-fails: pr= should already be recorded even though the merge failed" + pass "fm-pr-merge propagates a real glab merge failure without silently succeeding" +} + +# Each pre-merge condition, driven one at a time, so no condition can be +# carried by another. The refusal names that condition, no merge is attempted, +# and pr= is still recorded and the poll still armed exactly as the GitHub path +# leaves them when gh-axi itself fails. +test_gitlab_each_condition_refuses_independently() { + local case_dir rc name expected spec + set -- \ + "state|state=closed|state is \"closed\", not open" \ + "detail|detail=need_rebase|detailed_merge_status is \"need_rebase\", not mergeable" \ + "conflicts|conflicts=true|has_conflicts is \"true\", not false" \ + "discussions|discussions=false|blocking_discussions_resolved is \"false\", not true" \ + "pipeline-status|pipeline_status=failed|the head pipeline status is \"failed\", not success" \ + "pipeline-sha|pipeline_sha=$MR_STALE_HEAD|the head pipeline ran at \"$MR_STALE_HEAD\", not at the current head $MR_HEAD" \ + "no-pipeline|pipeline=null|the head pipeline status is \"none\", not success" + for spec in "$@"; do + name=${spec%%|*} + expected=${spec##*|} + spec=${spec#*|} + case_dir=$(make_gitlab_case "gitlab-refuse-$name" "${spec%%|*}") + + set +e + run_pr_merge "$case_dir" task-x1 "$MR_URL" \ + > "$case_dir/stdout" 2> "$case_dir/stderr" + rc=$? + set -e + + expect_code 1 "$rc" "gitlab-refuse-$name: fm-pr-merge should refuse" + assert_grep "error: refusing to merge $MR_URL" "$case_dir/stderr" \ + "gitlab-refuse-$name: refusal did not name the merge request" + assert_grep "$expected" "$case_dir/stderr" \ + "gitlab-refuse-$name: refusal did not name the failing condition" + [ -z "$(glab_merge_line "$case_dir/glab.log")" ] \ + || fail "gitlab-refuse-$name: a merge was attempted despite the refusal" + assert_grep "pr=$MR_URL" "$case_dir/state/task-x1.meta" \ + "gitlab-refuse-$name: a refusal should still leave the recorded PR reference" + assert_present "$case_dir/state/task-x1.check.sh" \ + "gitlab-refuse-$name: a refusal should still leave the merge poll armed" + done + pass "fm-pr-merge refuses on each GitLab pre-merge condition independently" +} + +test_gitlab_reports_every_failing_condition() { + local case_dir rc expected + case_dir=$(make_gitlab_case gitlab-refuse-all \ + state=closed detail=conflict conflicts=true discussions=false \ + pipeline_status=failed "pipeline_sha=$MR_STALE_HEAD") + + set +e + run_pr_merge "$case_dir" task-x1 "$MR_URL" \ + > "$case_dir/stdout" 2> "$case_dir/stderr" + rc=$? + set -e + + expect_code 1 "$rc" "gitlab-refuse-all: fm-pr-merge should refuse" + for expected in \ + 'state is "closed", not open' \ + 'detailed_merge_status is "conflict", not mergeable' \ + 'has_conflicts is "true", not false' \ + 'blocking_discussions_resolved is "false", not true' \ + 'the head pipeline status is "failed", not success' \ + "the head pipeline ran at \"$MR_STALE_HEAD\", not at the current head $MR_HEAD" + do + assert_grep "$expected" "$case_dir/stderr" \ + "gitlab-refuse-all: '$expected' was not reported" + done + pass "fm-pr-merge reports every failing GitLab condition, not only the first" +} + +test_gitlab_stale_recorded_head_is_reported() { + local case_dir rc merge_line + case_dir=$(make_gitlab_case gitlab-stale-head) + # The recorded head is what a rebase leaves behind. It is read before + # fm-pr-check.sh rewrites the metadata, which drops a head it cannot resolve + # for a GitLab task, so reading it afterwards would find nothing at all. + printf 'pr_head=%s\n' "$MR_STALE_HEAD" >> "$case_dir/state/task-x1.meta" + + set +e + run_pr_merge "$case_dir" task-x1 "$MR_URL" \ + > "$case_dir/stdout" 2> "$case_dir/stderr" + rc=$? + set -e + + expect_code 0 "$rc" "gitlab-stale-head: the live head satisfies every condition, so it should merge" + assert_grep "recorded head $MR_STALE_HEAD disagrees with the live head $MR_HEAD" \ + "$case_dir/stderr" "gitlab-stale-head: the stale recorded head was trusted silently" + merge_line=$(glab_merge_line "$case_dir/glab.log") + case "$merge_line" in + *"--sha $MR_HEAD"*) : ;; + *) fail "gitlab-stale-head: the merge was not bound to the live head: '$merge_line'" ;; + esac + assert_no_grep "pr_head=$MR_STALE_HEAD" "$case_dir/state/task-x1.meta" \ + "gitlab-stale-head: the recording step no longer drops an unresolvable GitLab head" + pass "fm-pr-merge reports a stale recorded head and verifies the live one" +} + +test_gitlab_unreadable_state_refuses() { + local case_dir rc name + for name in view-fails not-an-object split-value; do + case_dir=$(make_gitlab_case "gitlab-unreadable-$name") + case "$name" in + view-fails) : > "$case_dir/glab-view-fails" ;; + not-an-object) printf '[]\n' > "$case_dir/mr.json" ;; + # A value carrying a newline splits into a line no field name matches, so + # it must refuse rather than be truncated into a value a check accepts. + split-value) write_mr_json "$case_dir/mr.json" 'state=opened\nnot-a-field' ;; + esac + + set +e + run_pr_merge "$case_dir" task-x1 "$MR_URL" \ + > "$case_dir/stdout" 2> "$case_dir/stderr" + rc=$? + set -e + + expect_code 1 "$rc" "gitlab-unreadable-$name: fm-pr-merge should refuse" + assert_grep 'could not read the GitLab merge request state before merging' \ + "$case_dir/stderr" "gitlab-unreadable-$name: refusal did not name the unreadable state" + [ -z "$(glab_merge_line "$case_dir/glab.log")" ] \ + || fail "gitlab-unreadable-$name: a merge was attempted on an unreadable state" + done + pass "fm-pr-merge refuses an unreadable GitLab merge request state rather than merging blind" +} + +test_gitlab_invalid_head_refuses() { + local case_dir rc + case_dir=$(make_gitlab_case gitlab-invalid-head head=not-a-sha) + + set +e + run_pr_merge "$case_dir" task-x1 "$MR_URL" \ + > "$case_dir/stdout" 2> "$case_dir/stderr" + rc=$? + set -e + + expect_code 1 "$rc" "gitlab-invalid-head: fm-pr-merge should refuse" + assert_grep 'could not read the GitLab merge request head commit before merging' \ + "$case_dir/stderr" "gitlab-invalid-head: refusal did not name the unreadable head" + [ -z "$(glab_merge_line "$case_dir/glab.log")" ] \ + || fail "gitlab-invalid-head: a merge was bound to a head that is not a commit" + pass "fm-pr-merge refuses a GitLab head commit it cannot validate" +} + +test_gitlab_missing_tool_refuses_before_recording() { + local case_dir rc tool + for tool in glab jq; do + case_dir=$(make_gitlab_case "gitlab-no-$tool") + mirror_path_without "$case_dir/no$tool" "$tool" + + set +e + FM_ROOT_OVERRIDE="$ROOT" \ + FM_STATE_OVERRIDE="$case_dir/state" \ + FM_TEST_GH_AXI_LOG="$case_dir/gh-axi.log" \ + FM_TEST_GLAB_LOG="$case_dir/glab.log" \ + FM_TEST_GLAB_JSON="$case_dir/mr.json" \ + PATH="$case_dir/no$tool" \ + "$PR_MERGE" task-x1 "$MR_URL" > "$case_dir/stdout" 2> "$case_dir/stderr" + rc=$? + set -e + + expect_code 1 "$rc" "gitlab-no-$tool: fm-pr-merge should refuse" + assert_grep "error: merging a GitLab merge request requires $tool on PATH" \ + "$case_dir/stderr" "gitlab-no-$tool: refusal did not name the missing tool" + assert_no_grep "pr=$MR_URL" "$case_dir/state/task-x1.meta" \ + "gitlab-no-$tool: a PR reference was recorded despite the missing tool" + assert_absent "$case_dir/state/task-x1.check.sh" \ + "gitlab-no-$tool: a merge poll was armed despite the missing tool" + done + pass "fm-pr-merge refuses before recording anything when glab or jq is absent" +} + +test_gitlab_head_override_args_refuse_before_recording() { + local case_dir rc + case_dir=$(make_gitlab_case gitlab-head-override) + + set +e + run_pr_merge "$case_dir" task-x1 "$MR_URL" -- --sha "$MR_STALE_HEAD" \ + > "$case_dir/stdout" 2> "$case_dir/stderr" + rc=$? + set -e + + expect_code 1 "$rc" "gitlab-head-override: fm-pr-merge should refuse a caller head override" + assert_grep 'extra merge arguments must not override the head commit' "$case_dir/stderr" \ + "gitlab-head-override: refusal did not explain the head override" + assert_no_grep "pr=$MR_URL" "$case_dir/state/task-x1.meta" \ + "gitlab-head-override: the URL was recorded before rejecting the head override" + assert_absent "$case_dir/state/task-x1.check.sh" \ + "gitlab-head-override: a head override armed a merge poll" + [ ! -s "$case_dir/glab.log" ] || fail "gitlab-head-override: glab was invoked despite the head override" + pass "fm-pr-merge refuses a GitLab head override before recording state" +} + +test_github_still_forwards_sha_arg() { + local case_dir + case_dir=$(make_case github-sha-arg) + mkdir -p "$case_dir/wt" + add_gh_mocks "$case_dir" dddddddddddddddddddddddddddddddddddddddd + : > "$case_dir/gh-axi.log" + + # --sha is rejected only where the head is firstmate's to determine. GitHub's + # extra args are the caller's business exactly as they were. + run_pr_merge "$case_dir" task-x1 https://github.com/example/repo/pull/44 -- --sha abc123 \ + > "$case_dir/stdout" 2> "$case_dir/stderr" || fail "github-sha-arg: fm-pr-merge failed" + + grep -qxF 'pr merge 44 --repo example/repo --squash --sha abc123' "$case_dir/gh-axi.log" \ + || fail "github-sha-arg: the GitHub path stopped forwarding a caller --sha" + pass "fm-pr-merge leaves GitHub extra-arg handling unchanged, including --sha" +} + test_records_pr_and_head_before_merging test_merge_failure_propagates_after_recording test_extra_merge_args_forwarded @@ -311,3 +748,16 @@ test_repo_override_args_refuse_before_recording test_explicit_merge_method_not_overridden test_method_equals_merge_method_not_overridden test_parses_pr_url_for_gh_axi +test_github_still_forwards_sha_arg +test_gitlab_url_resolves_and_merges +test_gitlab_host_comes_from_the_url +test_gitlab_imposes_no_merge_method +test_gitlab_extra_args_forwarded +test_gitlab_merge_failure_propagates +test_gitlab_each_condition_refuses_independently +test_gitlab_reports_every_failing_condition +test_gitlab_stale_recorded_head_is_reported +test_gitlab_unreadable_state_refuses +test_gitlab_invalid_head_refuses +test_gitlab_missing_tool_refuses_before_recording +test_gitlab_head_override_args_refuse_before_recording From 614853ddc0719d3b55e2e7c62b5f801e4c803858 Mon Sep 17 00:00:00 2001 From: Inthuson Anandakumar Date: Sat, 22 Aug 2026 02:37:54 +0000 Subject: [PATCH 2/5] no-mistakes(review): reject bundled -R clusters and make tool-absence cases host-independent --- bin/fm-pr-merge.sh | 15 ++++-- tests/fm-pr-check-security.test.sh | 11 +++- tests/fm-pr-merge.test.sh | 87 +++++++++++++++++++++++++++--- 3 files changed, 101 insertions(+), 12 deletions(-) diff --git a/bin/fm-pr-merge.sh b/bin/fm-pr-merge.sh index 845e99cba29..9afd4e4acfa 100755 --- a/bin/fm-pr-merge.sh +++ b/bin/fm-pr-merge.sh @@ -24,9 +24,9 @@ # recorded value stale. Reading that state needs glab and jq, and either one # absent stops the merge before any state is recorded. # -# Extra args must not include --repo or -R because the repository comes only -# from the URL, nor --sha on GitLab because the head comes only from the live -# read. +# Extra args must not include --repo or -R in any form, including a bundled +# short-option cluster such as -yR, because the repository comes only from the +# URL, nor --sha on GitLab because the head comes only from the live read. # Usage: fm-pr-merge.sh [-- ] set -eu @@ -73,7 +73,14 @@ reject_repo_overrides() { local arg for arg in "$@"; do case "$arg" in - --repo|--repo=*|-R|-R?*) + --repo|--repo=*) + echo "error: extra merge arguments must not override the repository" >&2 + return 1 + ;; + --*) ;; + # A single-dash argument is a short-option cluster, which both CLIs expand + # one character at a time, so -yR carries --repo exactly as a bare -R does. + -*R*) echo "error: extra merge arguments must not override the repository" >&2 return 1 ;; diff --git a/tests/fm-pr-check-security.test.sh b/tests/fm-pr-check-security.test.sh index fee25bfbd91..f9795c3d361 100755 --- a/tests/fm-pr-check-security.test.sh +++ b/tests/fm-pr-check-security.test.sh @@ -26,6 +26,10 @@ REAL_MV=$(command -v mv) REAL_STAT=$(command -v stat) REAL_CHMOD=$(command -v chmod) REAL_BASENAME=$(command -v basename) +# The merge path reads a merge request's JSON with the real jq, and BASE_PATH is +# deliberately restricted, so a case that needs jq exposes this one rather than +# depending on the host keeping jq in one of those four directories. +REAL_JQ=$(command -v jq) || fail "these tests read glab's JSON with the real jq, which was not found" ack_watcher_cycle() { # local state=$1 err sequence generation @@ -2892,11 +2896,16 @@ EOF # state it could not read. write_task_meta "$dir" task-c : > "$dir/glab.log" + # The merge path needs jq before it reads anything, so this case supplies it + # and the refusal below is the unreadable state rather than a missing tool. + ln -sf "$REAL_JQ" "$dir/fakebin/jq" set +e - run_merge_entry "$dir" task-c "$url" >/dev/null 2>&1 + run_merge_entry "$dir" task-c "$url" >/dev/null 2> "$dir/merge-c.err" rc=$? set -e [ "$rc" -ne 0 ] || fail "merge wrapper merged a GitLab merge request it could not read" + grep -qF 'could not read the GitLab merge request state before merging' "$dir/merge-c.err" \ + || fail "merge wrapper refused for some reason other than the state it could not read" [ ! -s "$dir/gh-axi.log" ] || fail "merge wrapper reached the GitHub CLI for a GitLab URL" grep -qF "mr view 7 -R https://gitlab.example/group/subgroup/project" "$dir/glab.log" \ || fail "merge wrapper did not read the merge request through glab at its own instance" diff --git a/tests/fm-pr-merge.test.sh b/tests/fm-pr-merge.test.sh index 9f1d05e9b37..2367e8e5d22 100755 --- a/tests/fm-pr-merge.test.sh +++ b/tests/fm-pr-merge.test.sh @@ -13,7 +13,8 @@ # (e) PR URL is parsed to number + --repo for gh-axi (defaults to --squash) # (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 +# (h) repo override args fail fast because the repo comes from the URL, +# including a bundled short-option cluster that carries -R # (i) a GitLab MR URL resolves and merges through glab instead of erroring # (j) glab is addressed by the host from the URL, never an assumed one # (k) no merge method is imposed on GitLab, so the project's own one applies @@ -180,11 +181,16 @@ make_gitlab_case() { printf '%s\n' "$case_dir" } -# mirror_path_without : the whole search path re-exposed by symlink -# except one tool, because a real copy anywhere on PATH would prove nothing. +# mirror_path_without [ ...]: the whole search path +# re-exposed by symlink except one tool, because a real copy anywhere on PATH +# would prove nothing. The named bindirs are mirrored ahead of the search path, +# so the case's own mocks answer for every tool that is not the omitted one and +# the refusal names that tool alone whatever the host happens to have installed. mirror_path_without() { - local dir=$1 omit=$2 bindir entry name + local dir=$1 omit=$2 search bindir entry name + shift 2 mkdir -p "$dir" + search=$(printf '%s\n' "$@"; printf '%s\n' "$BASE_PATH" | tr ':' '\n') while IFS= read -r bindir; do [ -d "$bindir" ] || continue for entry in "$bindir"/*; do @@ -194,7 +200,7 @@ mirror_path_without() { [ -e "$dir/$name" ] || ln -s "$entry" "$dir/$name" 2>/dev/null done done </dev/null 2>&1 \ || fail "the $omit-free search path still resolved $omit" @@ -383,6 +389,67 @@ test_repo_override_args_refuse_before_recording() { pass "fm-pr-merge refuses repo override args before recording state" } +# A bundled short-option cluster carries -R without ever being exactly -R, and +# both CLIs expand it one character at a time, so the guard has to read the +# whole cluster. On GitLab that redirect names an instance, not only a +# repository, so it must refuse before anything is recorded or read. +test_bundled_repo_override_args_refuse_before_recording() { + local case_dir rc + case_dir=$(make_case bundled-repo-override) + mkdir -p "$case_dir/wt" + add_gh_mocks "$case_dir" abababababababababababababababababababab + : > "$case_dir/gh-axi.log" + + set +e + run_pr_merge "$case_dir" task-x1 https://github.com/right/repo/pull/6 -- -dR wrong/repo \ + > "$case_dir/stdout" 2> "$case_dir/stderr" + rc=$? + set -e + + expect_code 1 "$rc" "bundled-repo-override: fm-pr-merge should refuse a bundled repo override" + assert_grep 'extra merge arguments must not override the repository' "$case_dir/stderr" \ + "bundled-repo-override: refusal did not explain the repo override" + assert_no_grep 'pr=https://github.com/right/repo/pull/6' "$case_dir/state/task-x1.meta" \ + "bundled-repo-override: PR URL was recorded before rejecting the bundled repo override" + assert_absent "$case_dir/state/task-x1.check.sh" \ + "bundled-repo-override: a bundled repo override armed a merge poll" + assert_no_grep 'pr merge' "$case_dir/gh-axi.log" \ + "bundled-repo-override: gh-axi pr merge was invoked despite the bundled repo override" + + case_dir=$(make_gitlab_case bundled-repo-override-gitlab) + + set +e + run_pr_merge "$case_dir" task-x1 "$MR_URL" -- -yR https://other.example/g/p \ + > "$case_dir/stdout" 2> "$case_dir/stderr" + rc=$? + set -e + + expect_code 1 "$rc" "bundled-repo-override-gitlab: fm-pr-merge should refuse a bundled instance override" + assert_grep 'extra merge arguments must not override the repository' "$case_dir/stderr" \ + "bundled-repo-override-gitlab: refusal did not explain the repo override" + assert_no_grep "pr=$MR_URL" "$case_dir/state/task-x1.meta" \ + "bundled-repo-override-gitlab: the URL was recorded before rejecting the bundled override" + assert_absent "$case_dir/state/task-x1.check.sh" \ + "bundled-repo-override-gitlab: a bundled override armed a merge poll" + [ ! -s "$case_dir/glab.log" ] \ + || fail "bundled-repo-override-gitlab: glab was invoked despite the bundled override" + + # Only a cluster carrying the repository flag is refused: every other short + # cluster is still the caller's business and still reaches the forge. + case_dir=$(make_case bundled-non-repo-cluster) + mkdir -p "$case_dir/wt" + add_gh_mocks "$case_dir" bcbcbcbcbcbcbcbcbcbcbcbcbcbcbcbcbcbcbcbc + : > "$case_dir/gh-axi.log" + + run_pr_merge "$case_dir" task-x1 https://github.com/example/repo/pull/8 -- -d \ + > "$case_dir/stdout" 2> "$case_dir/stderr" \ + || fail "bundled-non-repo-cluster: fm-pr-merge refused a short flag that overrides nothing" + + grep -qxF 'pr merge 8 --repo example/repo --squash -d' "$case_dir/gh-axi.log" \ + || fail "bundled-non-repo-cluster: a short flag carrying no repository override was not forwarded" + pass "fm-pr-merge refuses a bundled short-option repo override and forwards other short flags" +} + test_explicit_merge_method_not_overridden() { local case_dir case_dir=$(make_case explicit-merge-method) @@ -673,10 +740,15 @@ test_gitlab_invalid_head_refuses() { } test_gitlab_missing_tool_refuses_before_recording() { - local case_dir rc tool + local case_dir rc tool other for tool in glab jq; do + if [ "$tool" = glab ]; then other=jq; else other=glab; fi case_dir=$(make_gitlab_case "gitlab-no-$tool") - mirror_path_without "$case_dir/no$tool" "$tool" + mirror_path_without "$case_dir/no$tool" "$tool" "$case_dir/fakebin" + # One tool absent, the other still answered by this case's own mock, so the + # refusal names exactly one tool on a host that ships neither. + PATH="$case_dir/no$tool" command -v "$other" >/dev/null 2>&1 \ + || fail "gitlab-no-$tool: the $tool-free search path lost the $other mock as well" set +e FM_ROOT_OVERRIDE="$ROOT" \ @@ -745,6 +817,7 @@ test_missing_meta_refuses_before_merge test_malformed_url_refuses_before_merge test_rejects_unsafe_url_segments_before_recording test_repo_override_args_refuse_before_recording +test_bundled_repo_override_args_refuse_before_recording test_explicit_merge_method_not_overridden test_method_equals_merge_method_not_overridden test_parses_pr_url_for_gh_axi From 93fbfa3cfe9229bb0a774ee72f06dd1f7a3c0e35 Mon Sep 17 00:00:00 2001 From: Inthuson Anandakumar Date: Sat, 22 Aug 2026 03:42:05 +0000 Subject: [PATCH 3/5] no-mistakes(test): state authorised GitHub narrowing of bundled -R guard This branch NARROWS GitHub behaviour. The narrowing was authorised deliberately rather than slipping in by accident, and it applies to both providers, GitHub and GitLab alike, because a script that guards one provider and not the other is a trap for the next reader. What bin/fm-pr-merge.sh now refuses is extra merge arguments containing a bundled short-option cluster that includes R, for example "-dR other/repo". The forge CLIs expand such a cluster one character at a time, so it carries "--repo other/repo", and that later value wins over the repository the URL named. Before this change, "fm-pr-merge.sh -- -dR other/repo" reached "gh-axi pr merge 12 --repo example/repo --squash -dR other/repo" and exited 0 with pr= recorded and the merge poll armed. It now exits 1 with "extra merge arguments must not override the repository", records nothing, and invokes no forge merge command. Every other GitHub invocation is byte-identical to the base commit. Closing that hole honours the existing rule rather than departing from it. The file header already forbids --repo and -R because the repository must come only from the URL, so a bundled cluster carrying a repository override was never legitimate behaviour to preserve: it was that guard being evaded. Redirecting a merge to a repository the URL does not name is exactly what the guard exists to prevent. The refusal is already pinned on both paths by the existing case test_bundled_repo_override_args_refuse_before_recording in tests/fm-pr-merge.test.sh. On GitHub ("-dR wrong/repo") and on GitLab ("-yR https://other.example/g/p") it asserts exit 1, the refusal wording, no pr= in the task meta, no armed merge poll, and no forge merge command invoked, with a control case proving a cluster that carries no repository override still reaches the forge. No duplicate assertion was added. Both assertions were confirmed to have teeth by narrowing the guard back to a bare -R and watching each path fail. This commit carries no file change: the guard and its coverage landed in 614853d, and this message exists so the pull request description states the narrowing. From 60464feb9e2c4a7fa82c21a859435cb46fd9d90e Mon Sep 17 00:00:00 2001 From: Inthuson Anandakumar Date: Sat, 22 Aug 2026 04:05:51 +0000 Subject: [PATCH 4/5] no-mistakes(document): fix README pointer for GitLab watch and merge doc --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index 43c1c9e1b10..4a12f9a0e2d 100644 --- a/README.md +++ b/README.md @@ -210,7 +210,7 @@ Firstmate's skills live in two separate places with different audiences: - [docs/cmux-backend.md](docs/cmux-backend.md) - current setup, socket security, and limits for the experimental cmux backend. - [docs/codex-app-backend.md](docs/codex-app-backend.md) - the current blocked Codex App backend boundary and rollout contract. - [docs/verification/runtime-backends.md](docs/verification/runtime-backends.md) - active maintainer verification for runtime backend guarantees. -- [docs/gitlab-merge-watch.md](docs/gitlab-merge-watch.md) - maintainer verification for GitLab merge watching on arbitrary instances. +- [docs/gitlab-merge-watch.md](docs/gitlab-merge-watch.md) - maintainer verification for watching and merging GitLab merge requests on arbitrary instances. - [docs/turnend-guard.md](docs/turnend-guard.md) - the primary session's current "no turn ends blind" backstop, scope, loop safety, and compatibility limits. - [docs/verification/supervision.md](docs/verification/supervision.md) - active maintainer verification for session-start, guard, continuity, and wedge integrations. - [docs/supervision-protocols/](docs/supervision-protocols/) - rendered primary-harness watcher protocols for Claude, Codex, OpenCode, Pi and `pi-signed`, Grok, Cursor, and unknown harness fallback. From cd75012e40761116f3b09bdc8ee1f847bdec6206 Mon Sep 17 00:00:00 2001 From: Inthuson Anandakumar Date: Sat, 22 Aug 2026 05:08:57 +0000 Subject: [PATCH 5/5] no-mistakes: apply CI fixes --- tests/fm-remote-job.test.sh | 28 ++++++++++++++++++++++------ 1 file changed, 22 insertions(+), 6 deletions(-) diff --git a/tests/fm-remote-job.test.sh b/tests/fm-remote-job.test.sh index 753c139efe8..0b6dede4d65 100755 --- a/tests/fm-remote-job.test.sh +++ b/tests/fm-remote-job.test.sh @@ -625,12 +625,22 @@ pass "quarantine clears only after recorded execution has stopped" # A replacement stops a Linux worker by signalling its whole isolated group, and # the supervisor in that group forwards a second stop signal to the same serving -# child, so the serving child is always signalled more than once. Keep signalling -# until it is gone: the first signal starts the shutdown and every later one -# lands inside it, the same way the group signal and the forwarded signal do. A -# shutdown that dies part way through leaves its ownership lock behind holding a -# half-written temp file no later worker can clear, and every replacement then -# fails to report ready. +# child, so the serving child is always signalled more than once. Signal a small +# bounded burst and then keep signalling until it is gone: the first signal +# starts the shutdown and every later one lands inside it, the same way the group +# signal and the forwarded signal do. A shutdown that dies part way through +# leaves its ownership lock behind holding a half-written temp file no later +# worker can clear, and every replacement then fails to report ready. +# +# The burst is bounded and the follow-up signals are paced deliberately. An +# unpaced signal loop delivers hundreds of thousands of signals per second, +# which corrupts the signalled bash's own pending-trap bookkeeping ("warning: +# run_pending_traps: bad value in trap_list[15]") and then kills it part way +# through the shutdown with SIGTERM or SIGSEGV. That reports a shutdown defect +# this worker does not have. Ten back-to-back signals still all land inside the +# shutdown's first file operation, so the repeat this pins is unchanged: with +# the default disposition restored instead of ignored, the ownership lock is +# left behind every run. REPEAT_HOME="$TMP_ROOT/repeat-signal-account" REPEAT_STATE="$TMP_ROOT/repeat-signal-jobs" mkdir -p "$REPEAT_HOME" @@ -645,8 +655,14 @@ for _ in $(seq 1 300); do done assert_present "$REPEAT_STATE/worker.ready" "the repeated-signal worker did not become ready" REPEAT_DEADLINE=$((SECONDS + 30)) +REPEAT_BURST=0 +while [ "$REPEAT_BURST" -lt 10 ]; do + kill -TERM "$REPEAT_WORKER_PID" 2>/dev/null || true + REPEAT_BURST=$((REPEAT_BURST + 1)) +done while kill -0 "$REPEAT_WORKER_PID" 2>/dev/null && [ "$SECONDS" -lt "$REPEAT_DEADLINE" ]; do kill -TERM "$REPEAT_WORKER_PID" 2>/dev/null || true + sleep 0.05 done if kill -0 "$REPEAT_WORKER_PID" 2>/dev/null; then kill -KILL "$REPEAT_WORKER_PID" 2>/dev/null || true