diff --git a/.agents/skills/project-management/SKILL.md b/.agents/skills/project-management/SKILL.md index 8feb522bd0c..524996e9efd 100644 --- a/.agents/skills/project-management/SKILL.md +++ b/.agents/skills/project-management/SKILL.md @@ -75,12 +75,13 @@ The captain's request to create that local project authorizes this local initial Run no-mistakes initialization only for `no-mistakes` and `no-mistakes-prod-only` projects: ```sh -cd projects/ && no-mistakes init && no-mistakes doctor +(cd projects/ && no-mistakes init && no-mistakes doctor) && bin/fm-pr-destination-guard.sh projects/ ``` Initialization configures the local gate and does not vendor a no-mistakes skill into the project. Do not create a commit merely because initialization ran. If doctor reports an environment, authentication, or daemon problem, resolve that blocker before dispatching work and never restart the shared daemon from a project operation. +`fm-pr-destination-guard.sh` pins gh's pull-request destination for a GitHub-hosted project to its own `origin`, in both the checkout and the no-mistakes gate, and refuses loudly rather than proceeding if that pin cannot be verified; see docs/architecture.md "Pull request destination is pinned, never gh's default" for why a project that is itself a GitHub fork needs this even when its remotes are already correct. ## Remove diff --git a/.no-mistakes.yaml b/.no-mistakes.yaml index 62bb9e72849..446783ff8ed 100644 --- a/.no-mistakes.yaml +++ b/.no-mistakes.yaml @@ -1,5 +1,14 @@ # Per-repo no-mistakes overrides. +# This repo is a real GitHub fork (joliverMI/firstmate, forked from the public +# kunchenguid/firstmate). There is no key in this file, or anywhere else +# no-mistakes reads, that points its PR step's destination at this repo +# instead of the fork parent: `no-mistakes init --fork-url` solves the +# opposite workflow (push to a fork, PR against the recorded origin) and does +# not apply here. The fix is bin/fm-pr-destination-guard.sh, applied outside +# this file's reach; see docs/architecture.md "Pull request destination is +# pinned, never gh's default" before assuming a key here can solve this again. + # firstmate is an agent-orchestration repo: its AGENTS.md installs a fleet-captain # identity. Disable project-level agent settings/instructions for gate agents so a # no-mistakes review/fix/document/test/lint/pr/rebase/ci agent never adopts that diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 65305797be8..0333bbad55d 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -18,6 +18,8 @@ GitHub Actions and Dependabot are exempt so their automation keeps working, but 1. Fork the repo, then clone the parent repo or set your local `origin` back to the parent (`git@github.com:kunchenguid/firstmate.git`). 2. Create a branch and make your changes. 3. Initialize the gate with your fork as the push target: `no-mistakes init --fork-url git@github.com:/firstmate.git` (firstmate expects **no-mistakes v1.31.2+**; without a fork, plain `no-mistakes init` still works for maintainers with push access). + Then run `bin/fm-pr-destination-guard.sh .`, which pins `gh` in both your clone and its gate to resolve pull requests to your clone's own `origin` rather than to a destination it picks for you. + That matters when your `origin` is itself a GitHub fork, because `gh pr create` then defaults the pull request to the fork's parent repository; [`docs/architecture.md`](docs/architecture.md#pull-request-destination-is-pinned-never-ghs-default) owns that mechanism and what the guard verifies. 4. Commit your changes. 5. Push through the gate instead of pushing to `origin`: diff --git a/bin/fm-brief.sh b/bin/fm-brief.sh index a873c840517..69d0caa7741 100755 --- a/bin/fm-brief.sh +++ b/bin/fm-brief.sh @@ -360,7 +360,13 @@ case "$MODE" in 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: PR {url}\` to the status file and stop. +When it is implemented and committed, push your branch, then open the PR with its destination named in the very same command that creates it. Never let a tool choose that destination: \`gh pr create\` defaults to a fork's parent rather than the fork itself, \`gh repo view\` with no repository argument picks a remote by gh's own preference order (\`upstream\` before \`origin\`), and \`gh-axi\` drops an empty \`--repo\` and falls back to that same default - so a destination that is merely computed earlier, in some other command, fails open onto the parent. +First write the PR title and body into files, each with a quoted heredoc delimiter so the shell expands and executes nothing inside them. Text you author is never safe to inline: an apostrophe in a title ends the quoted string and kills the whole line with a syntax error before any of it runs, and a markdown body naming commands or paths in backticks would be executed before \`gh-axi\` ever saw it, its output substituted into the body. +\`cat > "\$(git rev-parse --absolute-git-dir)/fm-pr-title.txt" <<'FM_PR_TITLE'\` … your title, verbatim … \`FM_PR_TITLE\` +\`cat > "\$(git rev-parse --absolute-git-dir)/fm-pr-body.md" <<'FM_PR_BODY'\` … your body, verbatim … \`FM_PR_BODY\` +Then name the destination and create the PR as one command, so the create call cannot run at all unless the destination was determined with certainty: +\`set -- --title "\$(cat "\$(git rev-parse --absolute-git-dir)/fm-pr-title.txt")" --body-file "\$(git rev-parse --absolute-git-dir)/fm-pr-body.md"; OWNER_REPO=\$("$FM_ROOT/bin/fm-pr-destination-guard.sh" . --print-destination); case \$? in 0) gh-axi pr create --repo "\$OWNER_REPO" "\$@" ;; 3) gh-axi pr create "\$@" ;; *) exit 1 ;; esac\` +Your words go only into those two files; the create command itself is fixed text - run it exactly as written, as one command line, and do not edit or split it. Both files are named by your own worktree's git directory, so they are yours alone - crewmates run concurrently on one host under one user, and a fixed path in a shared directory would let another task's title or body silently replace yours between the write and the create. That guard mode reads this repo's own \`origin\` remote, makes no network call, and never prints a guess: exit 0 means it printed \`owner/repo\` and \`--repo\` carries that verified value straight into the create call - that flag being set from the guard's own value IS the destination guarantee, so there is no separate comparison left to make by eye; exit 3 means this repo is not on a GitHub host, where gh's fork-parent default cannot apply, so the PR is created without a destination override as it always was; any other exit means the destination is undetermined on a repo where the hazard is real, so nothing is created - append \`blocked: {its exact error}\` to the status file and stop. Otherwise append \`done: PR {url}\` to the status file and stop. Do NOT run /no-mistakes. The configured merge authority decides whether to merge the PR; firstmate relays the outcome. EOF ;; @@ -379,7 +385,8 @@ EOF ;; *) # no-mistakes SETUP2=" -2. Run \`no-mistakes doctor\`; if it reports the repo is not initialized here, run \`no-mistakes init\`." +2. Run \`no-mistakes doctor\`; if it reports the repo is not initialized here, run \`no-mistakes init\`. +3. Always run \`$FM_ROOT/bin/fm-pr-destination-guard.sh .\`, whether or not step 2 just ran \`no-mistakes init\`. It pins this repo's pull-request destination to its own \`origin\` (never gh's ambient default) and verifies the pin in both this checkout and its no-mistakes gate. Treat a non-zero exit as a blocker: append \`blocked: {its exact error}\` and stop rather than starting \`/no-mistakes\` unpinned." RULE1='1. Never push to the default branch. Never merge a PR.' IFS= read -r -d '' DOD <&2 return 1 } + "$SCRIPT_DIR/fm-pr-destination-guard.sh" "$dst" || { + echo "error: could not pin the pull-request destination for $project at $dst" >&2 + return 1 + } } write_registry() { diff --git a/bin/fm-pr-check.sh b/bin/fm-pr-check.sh index 96cb14dc938..30528863a07 100755 --- a/bin/fm-pr-check.sh +++ b/bin/fm-pr-check.sh @@ -5,6 +5,9 @@ # 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 whose owner/repository is not the task's own project +# origin is refused rather than recorded; see the check itself for why that +# backstop exists and when it stays silent. # Usage: fm-pr-check.sh set -eu @@ -41,6 +44,30 @@ if [ ! -f "$META" ] || [ -L "$META" ] || [ "$(fm_pr_file_link_count "$META")" != exit 1 fi +WT=$(grep '^worktree=' "$META" | tail -1 | cut -d= -f2- || true) + +# Refuse a GitHub PR reported for a repository other than this task's own +# project. This is a defense-in-depth backstop, not the primary defense +# (bin/fm-pr-destination-guard.sh pins gh's own PR-creation resolution so this +# never fires in the ordinary case): by the time a URL reaches here the PR may +# already exist, so this cannot recall it, but it stops firstmate from +# recording, tracking, or arming a merge watch for the wrong repository - see +# docs/architecture.md "Pull request destination is pinned, never gh's +# default". Silently skipped, not refused, when the task's own origin cannot +# be determined (no recorded worktree, or a non-GitHub remote): this check +# only ever blocks on a positive, confirmed mismatch. +if [ "$PROVIDER" = github ] && [ -n "$WT" ] && [ -d "$WT" ]; then + OWN_ORIGIN=$(git -C "$WT" config --get remote.origin.url 2>/dev/null || true) + if [ -n "$OWN_ORIGIN" ] && fm_pr_github_remote_owner_repo "$OWN_ORIGIN"; then + OWN_REPO_ID=$(fm_pr_lower "$FM_PR_REMOTE_OWNER/$FM_PR_REMOTE_REPO") + PR_REPO_ID=$(fm_pr_lower "$FM_PR_OWNER/$FM_PR_REPO") + if [ "$OWN_REPO_ID" != "$PR_REPO_ID" ]; then + echo "error: PR $URL targets $FM_PR_OWNER/$FM_PR_REPO, not this task's own project $FM_PR_REMOTE_OWNER/$FM_PR_REMOTE_REPO; refusing to record or arm it" >&2 + exit 1 + fi + fi +fi + # A prior exact merged result may have queued its durable wake immediately # before interruption. # Finish only its identity-bound receipt before publishing a replacement poll. @@ -71,7 +98,6 @@ 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. -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 if REMOTE_HEAD=$(cd "$WT" && gh pr view "$URL" --json headRefOid -q .headRefOid 2>/dev/null) \ diff --git a/bin/fm-pr-destination-guard.sh b/bin/fm-pr-destination-guard.sh new file mode 100755 index 00000000000..3ca9c124ad2 --- /dev/null +++ b/bin/fm-pr-destination-guard.sh @@ -0,0 +1,194 @@ +#!/usr/bin/env bash +# Pin gh's ambiguous pull-request destination resolution to this repo's own +# "origin" remote, in both a project checkout and its no-mistakes gate, then +# fail loudly if the pin cannot be verified in either place. +# +# `gh pr create` (and other gh commands) resolve their target repository from +# the working directory's git remotes UNLESS a default is pinned - and when +# the resolved remote is a GitHub fork, gh defaults to the fork's PARENT, not +# the fork itself. joliverMI/firstmate is a real fork of the public +# kunchenguid/firstmate, so any `gh pr create` run without this pin lands on +# the parent. See docs/architecture.md "Pull request destination is pinned, +# never gh's default" for the incident and the full mechanism; no-mistakes' +# own PR step has no config surface to point it elsewhere. +# +# `gh repo set-default origin` writes that pin as git config +# (remote.origin.gh-resolved) - a purely local, non-destructive setting that +# works against an ordinary checkout or a bare repository (no-mistakes' gate +# is bare). This script applies and verifies the pin in both places, because +# no-mistakes' PR step runs `gh` from a worktree of its gate, not from the +# project checkout firstmate or a crewmate is sitting in; the gate's own +# origin remote is set up by `no-mistakes init` and is not this script's +# concern. +# +# Verification is a destination check, not an existence check: the pin must read +# back through `gh repo set-default --view` AND name this project's own +# origin owner/repository. A pin that merely exists proves nothing - a repointed +# origin pins gh just as successfully to the wrong repository - so a read that +# fails, reads back empty, or names anything else is a refusal, never a pass. +# +# That verified destination, not the write, is the invariant. The write is an +# authenticated online round trip that also takes the shared common git config +# lock, and this guard runs in every no-mistakes ship brief's Setup step, from +# concurrent crewmate worktrees of the same checkout. So an already-correct +# destination is confirmed with a local config read plus the offline `--view` +# and left alone; `gh repo set-default origin` is called only when the +# destination is missing or wrong, and a failed write refuses only if the +# destination is still not verifiable afterwards. gh's own stderr is carried +# into every refusal, because that diagnostic is what a blocked crewmate +# records and what an operator recovers from. +# +# An origin whose host this script does not recognize as GitHub is out of scope +# rather than protected or blocked - but it says so out loud, naming the host, +# in both modes. That announcement is the whole safeguard for the case it +# cannot decide: a GitHub Enterprise Server host carries the same fork-parent +# default and is not recognized here, so the first project on such a host +# reports the gap itself instead of quietly proceeding unpinned. +# +# One authority decides both "is this GitHub?" and "which repository?": the +# parse in fm_pr_github_remote_owner_repo. A second, independent scope test +# (a substring glob over the URL, say) can disagree with it, and every +# disagreement is either a task blocked on a correctly configured project or a +# hazard skipped on a real one. So the parse runs first; a URL it cannot name +# refuses only when its HOST is a GitHub host, and is otherwise a forge the +# fork-parent hazard cannot reach. +# +# --print-destination is the machine-readable half of the same determination, +# for a caller that must NAME the destination rather than pin it - a direct-PR +# task, which never runs no-mistakes and so never has a pin to rely on. It +# prints exactly "owner/repo" on stdout and nothing else, or prints nothing on +# stdout, a diagnostic on stderr, and exits non-zero. It is a local git-config +# read and a string parse: no gh, no network, no pin, and no dependence on +# ambient state an earlier step may or may not have left behind. That matters +# because its caller substitutes the result straight into +# `gh-axi pr create --repo`, where an empty value would be dropped and fall +# back to exactly the fork-parent default this whole script exists to close. +# +# Usage: fm-pr-destination-guard.sh [--print-destination] +# Exit 0: the destination is pinned and verified to be this project's own +# repository (or, without --print-destination, origin is not GitHub so +# this guard's fork-parent default does not apply). With +# --print-destination, that repository is on stdout. +# Exit 1: origin is missing, or its host is GitHub but the repository could not +# be named, or the gate cannot be discovered, or the destination could +# not be read back and confirmed to name this project's own repository. +# Never silently proceeds. +# Exit 2: the arguments themselves are wrong. +# Exit 3: --print-destination only - origin is not on a GitHub host, so no +# GitHub destination exists to name and gh's fork-parent default cannot +# apply. Distinct from exit 1 so a caller can carry on unprotected +# where there is nothing to protect against, rather than treating a +# GitLab project as a blocked one. +set -eu + +SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +# shellcheck source=bin/fm-pr-lib.sh disable=SC1091 +. "$SCRIPT_DIR/fm-pr-lib.sh" + +DIR=${1:?usage: fm-pr-destination-guard.sh [--print-destination]} +MODE=verify +case "${2-}" in + '') ;; + --print-destination) MODE=print ;; + *) echo "error: unknown option: $2 (usage: fm-pr-destination-guard.sh [--print-destination])" >&2; exit 2 ;; +esac +[ "$#" -le 2 ] || { echo "error: too many arguments (usage: fm-pr-destination-guard.sh [--print-destination])" >&2; exit 2; } + +[ -d "$DIR" ] || { echo "error: not a directory: $DIR" >&2; exit 1; } + +ORIGIN_URL=$(git -C "$DIR" config --get remote.origin.url 2>/dev/null || true) +if [ -z "$ORIGIN_URL" ]; then + echo "error: $DIR has no 'origin' remote; cannot pin a pull-request destination" >&2 + exit 1 +fi + +# The project checkout's own origin is the single source of truth for where this +# project's pull requests belong. The gate is checked against this same value, +# not against its own origin: a gate whose origin drifted to the fork parent +# would otherwise verify happily against itself. +if ! fm_pr_github_remote_owner_repo "$ORIGIN_URL"; then + if fm_pr_github_host "$FM_PR_REMOTE_HOST"; then + echo "error: $DIR's origin ($(fm_pr_redact_remote_url "$ORIGIN_URL")) is on a GitHub host but names no owner/repository; the pull-request destination cannot be verified" >&2 + exit 1 + fi + UNRECOGNIZED=${FM_PR_REMOTE_HOST:-} + if [ "$MODE" = print ]; then + echo "note: $DIR's origin host ($UNRECOGNIZED) is not a recognized GitHub host, so there is no GitHub pull-request destination to name and gh's fork-parent default cannot apply" >&2 + exit 3 + fi + echo "skip: $DIR's origin host ($UNRECOGNIZED) is not a recognized GitHub host, so nothing here is pinned or verified; gh's fork-parent default this guard closes is GitHub-specific" + exit 0 +fi +EXPECTED="$FM_PR_REMOTE_OWNER/$FM_PR_REMOTE_REPO" + +if [ "$MODE" = print ]; then + printf '%s\n' "$EXPECTED" + exit 0 +fi + +EXPECTED_LC=$(fm_pr_lower "$EXPECTED") + +GH_ERR=$(umask 077; mktemp "${TMPDIR:-/tmp}/fm-pr-destination-guard.XXXXXX") || { + echo "error: could not allocate a scratch file to capture gh's diagnostics" >&2 + exit 1 +} +trap 'rm -f "$GH_ERR"' EXIT INT TERM + +gh_reason() { + sed -n '/[^[:space:]]/{s/\r$//;p;q;}' "$GH_ERR" 2>/dev/null || true +} + +refuse() { # [] + local message=$1 reason=${2-} + if [ -n "$reason" ]; then + echo "error: $message (gh: $reason)" >&2 + else + echo "error: $message" >&2 + fi + exit 1 +} + +destination_of() { # : echo the pinned destination, gh's stderr into $GH_ERR + local dir=$1 viewed + : > "$GH_ERR" + viewed=$(cd "$dir" && gh repo set-default --view 2>"$GH_ERR") || return 1 + printf '%s' "$viewed" | head -n1 | tr -d '[:space:]' +} + +pin_and_verify() { #