Skip to content

feat(bin): add bounded PR delivery loop - #2631

Closed
hsuperman wants to merge 36 commits into
kunchenguid:mainfrom
hsuperman:fm/pr-delivery-loop
Closed

feat(bin): add bounded PR delivery loop#2631
hsuperman wants to merge 36 commits into
kunchenguid:mainfrom
hsuperman:fm/pr-delivery-loop

Conversation

@hsuperman

@hsuperman hsuperman commented Aug 19, 2026

Copy link
Copy Markdown

Intent

Push PR delivery loop via fork; five captain fixes only; defer secondmate/post-merge orchestration

What Changed

  • Add a bounded main-home PR delivery scan that discovers open PRs for registered merge-capable projects, classifies live GitHub evidence, tracks reason-coded holds, and emits a check: pr-delivery wake for eligible PRs.
  • Guard delivery merges against head changes by recording remote PR heads and passing the expected head to GitHub at merge time.
  • Integrate the delivery loop into session startup and watcher polling, with its wake-handling skill, documentation, and coverage tests.

Risk Assessment

✅ Low: The change is bounded and the reviewed durable-fix paths correctly guard merge heads, delivery-state symlinks, closed-PR state, and ambiguous task bindings.

Testing

Inspected the target change from a clean worktree, ran the focused delivery-discovery and guarded-merge suites, and exercised the public delivery CLI in an isolated fake-forge home; it emitted the merge-eligible wake and left no eligible PR in the blocked queue.

Evidence: End-to-end PR delivery CLI transcript: eligible PR wake and empty blocked queue
merge-eligible: project=alpha repo=acme/alpha pr=42 task=ship42 url=https://github.com/acme/alpha/pull/42 head=aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
repo pr# url task_id reason_code reason_detail
Evidence: Reproducible isolated CLI evidence harness

Source: Reproducible isolated CLI evidence harness

#!/usr/bin/env bash
# Isolated executable-interface evidence for fm-pr-delivery.sh.
set -euo pipefail

root=$1
world=$(mktemp -d)
trap 'rm -rf "$world"' EXIT
home="$world/home"
fixture="$world/fixture"
fakebin="$world/fakebin"
mkdir -p "$home/state" "$home/data" "$home/projects/alpha" "$fixture/open" "$fixture/view" "$fakebin"
printf '%s\n' '- alpha [direct-PR] - delivery evidence' > "$home/data/projects.md"
git -C "$home/projects/alpha" init -q
git -C "$home/projects/alpha" config user.name evidence
git -C "$home/projects/alpha" config user.email evidence@example.invalid
git -C "$home/projects/alpha" commit --allow-empty -qm initial
git -C "$home/projects/alpha" remote add origin https://github.com/acme/alpha.git
printf '%s\n' \
  'window=fm-ship42' \
  "worktree=$home/projects/ship42" \
  'project=alpha' \
  'harness=codex' \
  'kind=ship' \
  'mode=direct-PR' \
  'yolo=on' \
  'pr=https://github.com/acme/alpha/pull/42' > "$home/state/ship42.meta"
printf '%s\n' '[{"number":42,"url":"https://github.com/acme/alpha/pull/42","headRefName":"fm/ship42","headRefOid":"aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa","baseRefName":"main","reviewDecision":"","mergeable":"MERGEABLE","statusCheckRollup":[{"conclusion":"SUCCESS","status":"COMPLETED"}]}]' > "$fixture/open/acme__alpha.json"
printf '%s\n' '{"number":42,"url":"https://github.com/acme/alpha/pull/42","headRefName":"fm/ship42","headRefOid":"aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa","baseRefName":"main","reviewDecision":"","mergeable":"MERGEABLE","statusCheckRollup":[{"conclusion":"SUCCESS","status":"COMPLETED"}],"reviewThreads":{"nodes":[]},"state":"OPEN"}' > "$fixture/view/acme__alpha-42.json"
cat > "$fakebin/gh" <<'SH'
#!/usr/bin/env bash
set -euo pipefail
fixture=${FM_PR_DELIVERY_FIXTURE:?}
repo=''
num=''
owner=''
name=''
query=''
args=("$@")
i=0
while [ "$i" -lt "$#" ]; do
  case "${args[$i]}" in
    --repo) repo="${args[$((i+1))]}"; i=$((i+2)); continue ;;
    view) num="${args[$((i+1))]}"; i=$((i+2)); continue ;;
    owner=*) owner=${args[$i]#owner=}; i=$((i+1)); continue ;;
    name=*) name=${args[$i]#name=}; i=$((i+1)); continue ;;
    number=*) num=${args[$i]#number=}; i=$((i+1)); continue ;;
    query=*) query=${args[$i]#query=}; i=$((i+1)); continue ;;
  esac
  i=$((i+1))
done
if [ "${1:-}" = pr ] && [ "${2:-}" = list ]; then
  jq -c . "$fixture/open/acme__alpha.json"
  exit 0
fi
if [ "${1:-}" = api ] && [ "${2:-}" = graphql ]; then
  if [[ "$query" == *pullRequests* ]]; then
    printf '%s\n' '{"data":{"repository":{"pullRequests":{"nodes":[{"number":42}],"pageInfo":{"hasNextPage":false,"endCursor":null}}}}}'
  else
    jq -c '{data:{repository:{pullRequest:{
      number, url, headRefName, headRefOid, baseRefName, reviewDecision, mergeable, state,
      author:(.author // {login:"author"}),
      commits:{nodes:[{commit:{statusCheckRollup:{contexts:{nodes:(.statusCheckRollup // []),pageInfo:{hasNextPage:false}}}}}]},
      reviews:(.reviews // {nodes:[],pageInfo:{hasPreviousPage:false}}),
      comments:(.comments // {nodes:[],pageInfo:{hasPreviousPage:false}}),
      reviewThreads:(.reviewThreads // {nodes:[]})
    }}}}' "$fixture/view/acme__alpha-42.json"
  fi
  exit 0
fi
exit 99
SH
chmod +x "$fakebin/gh"
env FM_ROOT_OVERRIDE="$root" FM_HOME="$home" FM_STATE_OVERRIDE="$home/state" FM_DATA_OVERRIDE="$home/data" FM_PROJECTS_OVERRIDE="$home/projects" FM_PR_DELIVERY_SECS=60 FM_PR_DELIVERY_FIXTURE="$fixture" GH_BIN=gh PATH="$fakebin:$PATH" "$root/bin/fm-pr-delivery.sh" scan --startup
env FM_ROOT_OVERRIDE="$root" FM_HOME="$home" FM_STATE_OVERRIDE="$home/state" FM_DATA_OVERRIDE="$home/data" FM_PROJECTS_OVERRIDE="$home/projects" FM_PR_DELIVERY_SECS=60 FM_PR_DELIVERY_FIXTURE="$fixture" GH_BIN=gh PATH="$fakebin:$PATH" "$root/bin/fm-pr-delivery.sh" show

Pipeline

Updates from git push no-mistakes

✅ **intent** - passed

✅ No issues found.

✅ **Rebase** - passed

✅ No issues found.

🔧 **Review** - 1 issue found → auto-fixed (3) ✅
  • 🚨 bin/fm-pr-merge.sh:100 - --expected-head is checked before the merge but is not passed to the forge. A contributor can push a new head after fm-pr-check.sh records the expected OID and before this command runs, causing the delivery path to merge that unreviewed new head. Enforce the OID at the final merge boundary (GitHub CLI’s --match-head-commit; extend the approved gh-axi wrapper to forward it if necessary).

🔧 Fix: Enforce PR heads at GitHub merge boundary
2 errors still open:

  • 🚨 bin/fm-pr-delivery.sh:137 - ensure_dirs follows a pre-existing state/pr-delivery symlink while creating its child directories, then checks whether the parent is a symlink only afterward. A malicious/stale symlink can therefore cause scan or accelerate to create and later modify delivery state outside the home. Validate/create each delivery-state directory without following symlinks before traversing it.
  • 🚨 .agents/skills/pr-delivery/SKILL.md:38 - The intent says “defer secondmate/post-merge orchestration,” but this new runtime skill directs a successful merge to run fm-fleet-sync.sh. Confirm whether this post-merge action is intentionally in scope; otherwise remove/defer it.

🔧 Fix: Reject symlinked PR delivery state directories
2 errors still open:

  • 🚨 bin/fm-pr-delivery.sh:720 - A successful open-PR inventory never retires state for PRs absent from that inventory. After an eligible PR is closed (or merged), its delivered fingerprint and blocked-queue row remain; if it is later reopened unchanged, new_fp == marker_fp suppresses the new merge wake indefinitely. On completion of a full repository inventory, retire queue/fingerprint/delivered/accelerate state for absent PRs (while preserving it on partial/deadline scans).
  • 🚨 bin/fm-pr-delivery.sh:249 - Multiple same-project task metadata files may record the same PR URL, but this returns the first glob-ordered task. A stale yolo=on task can therefore win over the actual task’s hold/approval state and authorize a merge under the wrong task. Treat more than one matching task as an ambiguous hold rather than selecting one.

🔧 Fix: Retire closed PR state and hold ambiguity
✅ Re-checked - no issues remain.

✅ **Test** - passed

✅ No issues found.

  • rtk git status --short and target-diff inspection
  • rtk tests/fm-pr-delivery.test.sh
  • rtk tests/fm-pr-merge.test.sh
  • rtk bash /Users/byronhsu/.no-mistakes/evidence/01M0EM5A27AF4GYWTW09JQDPBH/pr_delivery_e2e.sh /Users/byronhsu/.no-mistakes/worktrees/497bd902d278/01M0EM5A27AF4GYWTW09JQDPBH
✅ **Document** - passed

✅ No issues found.

🔧 **Lint** - 1 issue found → auto-fixed ✅
  • ⚠️ linter found issues (exit code 1)

🔧 Fix: Fix ShellCheck issues in PR delivery loop
✅ Re-checked - no issues remain.

✅ **Push** - passed

✅ No issues found.

@hsuperman
hsuperman force-pushed the fm/pr-delivery-loop branch from b84868b to 7b1f35f Compare August 20, 2026 04:13
@hsuperman hsuperman changed the title feat(bin): add bounded main-home PR delivery loop feat(bin): add bounded PR delivery loop Aug 20, 2026
@hsuperman
hsuperman force-pushed the fm/pr-delivery-loop branch 2 times, most recently from ed38fe8 to be75174 Compare August 21, 2026 23:00
@greptile-apps

greptile-apps Bot commented Aug 21, 2026

Copy link
Copy Markdown

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failure remains.

Reviews (3): Last reviewed commit: "no-mistakes: apply CI fixes" | Re-trigger Greptile

Comment thread bin/fm-pr-delivery.sh
hsuperman and others added 24 commits August 22, 2026 10:31
Discover and classify open PRs for merge-capable projects on the watcher
poll cadence and at locked session start, maintain a reason-coded blocked
queue, and queue check: pr-delivery wakes for merge-eligible or post-merge
obligations without relying on secondmate handoffs.

Co-authored-by: Cursor <cursoragent@cursor.com>
@kunchenguid

Copy link
Copy Markdown
Owner

Speaking as Kun's firstmate:

VISION: mixed. Bounded PR delivery discovery is real command-layer work, but the scan is wired into the watcher and locked session-start with FM_PR_DELIVERY_SECS default 300 — new behavior without an enable flag. Merge still goes through bin/fm-pr-merge.sh (GitLab path kept, plus --expected-head). Standing yolo=on can merge a classified-eligible PR without a fresh captain word; that is existing scoped consent, not a new silent grant, but the scan that feeds it is default-on.

Class: default-behavior. Watcher/session-start always run the delivery scan. Not a captain-flag this pass because it is not fully green.

Security: none after review. Main-home only; secondmate homes refuse. Merge is not executed by the scan; skill handling still requires configured authority via fm-pr-merge.sh. No HTTP listener, no workflow-file change.

Overlap: bin/fm-watch.sh with #2749 and #2795. bin/fm-pr-merge.sh with #2586 / #2779 (teardown rewrite currently reverts GitLab merge from #2779 — this PR keeps the GitLab path and adds expected-head). Not the spawn/teardown hold-pair files.

CI: HEAD 2e51ee2b968fb93690757dae543cc6b6ad30c8d1. CI run 32579112463 queued (attempt 2 after approval this pass, not green). Require no-mistakes 32579112537 queued. Structured attestation head_sha=7b1f35f9e61e8205fb3d04692707e6cc2c503868 does NOT match THIS HEAD — no-mistakes is blocking. Greptile SUCCESS — not a gate.

Workflows approved: yes (CI 32579112463, NM 32579112537). Not land-eligible. Waiting on CI, not the captain. Flag captain ONLY if later fully green (default-on scan).

@hsuperman

Copy link
Copy Markdown
Author

Closing per captain decision: Firstmate-repo development is paused, focus is on RecruitMagic only.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants