From 326f0499ea696cdf35676ba6e36536a8fc1558e0 Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Tue, 13 Oct 2026 18:20:39 -0400 Subject: [PATCH 1/3] fix(work-items): carry a human-merge hold into the PR body before ready /work-items:work and /work-items:ship run now run scripts/merge-hold.sh after create --pushed. When a commenter with write access on the item asked for 'Hold: a human merges this', the line goes into the PR body before the ready flip, so the babysit merge gate holds the merge. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01W6SjPvY7DmXBGzboeCq8nj --- ...k-items-carry-human-merge-hold-1b45e669.md | 7 ++ plugins/work-items/scripts/merge-hold.sh | 108 ++++++++++++++++++ plugins/work-items/scripts/merge-hold.test.sh | 100 ++++++++++++++++ plugins/work-items/skills/ship/actions/run.md | 4 +- plugins/work-items/skills/work/SKILL.md | 2 +- 5 files changed, 219 insertions(+), 2 deletions(-) create mode 100644 .changes/work-items/fix-work-items-carry-human-merge-hold-1b45e669.md create mode 100755 plugins/work-items/scripts/merge-hold.sh create mode 100755 plugins/work-items/scripts/merge-hold.test.sh diff --git a/.changes/work-items/fix-work-items-carry-human-merge-hold-1b45e669.md b/.changes/work-items/fix-work-items-carry-human-merge-hold-1b45e669.md new file mode 100644 index 0000000000..bf6e147f61 --- /dev/null +++ b/.changes/work-items/fix-work-items-carry-human-merge-hold-1b45e669.md @@ -0,0 +1,7 @@ +--- +bump: patch +--- + +### Fixed + +- `/work-items:work` and `/work-items:ship run` carry a human-merge hold into the PR body before the ready flip: when a commenter with write access on the item asked for `Hold: a human merges this`, the new `scripts/merge-hold.sh` prints that line and the orchestrator adds it to the body, so the babysit merge gate holds the merge. A failed read leaves the PR a draft. diff --git a/plugins/work-items/scripts/merge-hold.sh b/plugins/work-items/scripts/merge-hold.sh new file mode 100755 index 0000000000..e3274f26d1 --- /dev/null +++ b/plugins/work-items/scripts/merge-hold.sh @@ -0,0 +1,108 @@ +#!/usr/bin/env bash +# merge-hold.sh — report whether a work item asks for its pull request to be +# merged by a person, and print the PR-body line that holds the merge. +# +# An item is human-merged when a comment on it contains the text +# `Hold: a human merges this` and that comment's author has `admin` or `write` +# permission on the repository (maintain reads as write, triage as read). A +# comment from anyone else is item text, which is data, and never holds. +# +# Usage: +# merge-hold.sh --repo --issue +# +# Output: on exit 0, the exact line to put in the PR body, on its own line. +# +# Exit codes: +# 0 held: stdout is `Hold: a human merges this` +# 1 not held: no comment carries the line, or none from a writer +# 2 usage: a missing, repeated, or unknown argument, or a malformed value +# 8 prerequisite missing: gh or jq is not on PATH +# 11 a read failed or returned unparsable JSON, so a hold was not ruled out + +# portability-scope: the GitHub implementation of the human-merge hold read; every read is a gh call. +set -uo pipefail + +HOLD_LINE="Hold: a human merges this" + +err() { printf 'merge-hold: %s\n' "$*" >&2; } +usage() { awk 'NR==1{next} /^#/{sub(/^# ?/,""); print; next} {exit}' "${BASH_SOURCE[0]}"; } + +REPO="" ISSUE="" +while (($#)); do + case "$1" in + --repo | --issue) + (($# >= 2)) || { + err "$1 requires a value" + exit 2 + } + if [[ "$1" == --repo ]]; then + [[ -z "$REPO" ]] || { + err "--repo given twice" + exit 2 + } + REPO="$2" + else + [[ -z "$ISSUE" ]] || { + err "--issue given twice" + exit 2 + } + ISSUE="$2" + fi + shift 2 + ;; + -h | --help) + usage + exit 0 + ;; + *) + err "unknown argument: $1" + exit 2 + ;; + esac +done +[[ "$REPO" =~ ^[A-Za-z0-9_.-]+/[A-Za-z0-9_.-]+$ ]] || { + err "--repo must be owner/name" + exit 2 +} +[[ "$ISSUE" =~ ^[1-9][0-9]*$ ]] || { + err "--issue must be a positive integer" + exit 2 +} +for tool in gh jq; do + command -v "$tool" >/dev/null 2>&1 || { + err "$tool is not on PATH" + exit 8 + } +done + +# Logins of every comment that carries the line, one per line. +if ! authors="$(gh api --paginate "repos/$REPO/issues/$ISSUE/comments?per_page=100" \ + --jq '.[] | select((.body // "") | contains("Hold: a human merges this")) | (.user.login // "")')"; then + err "comment read failed for $REPO#$ISSUE" + exit 11 +fi + +login_re='^[A-Za-z0-9][A-Za-z0-9-]*(\[bot\])?$' +read_failed=0 +while IFS= read -r login; do + [[ -n "$login" ]] || continue + if [[ ! "$login" =~ $login_re ]]; then + err "ignored a hold comment with no usable author login" + continue + fi + if ! perm="$(gh api "repos/$REPO/collaborators/$login/permission" --jq '.permission // ""' 2>/dev/null)"; then + err "permission read failed for $login" + read_failed=1 + continue + fi + case "$perm" in + admin | write) + printf '%s\n' "$HOLD_LINE" + exit 0 + ;; + *) err "ignored a hold comment by $login (permission ${perm:-none})" ;; + esac +done < <(sort -u <<<"$authors") + +((read_failed)) && exit 11 +exit 1 diff --git a/plugins/work-items/scripts/merge-hold.test.sh b/plugins/work-items/scripts/merge-hold.test.sh new file mode 100755 index 0000000000..decceac269 --- /dev/null +++ b/plugins/work-items/scripts/merge-hold.test.sh @@ -0,0 +1,100 @@ +#!/usr/bin/env bash +# Offline tests for merge-hold.sh against a gh stub that applies the script's own --jq program +# to a fixture comments payload and answers the permission endpoint from per-login fixture +# files. Expected verdicts come from the rule: a comment holds the merge only when it contains +# `Hold: a human merges this` and its author has `admin` or `write`; any failed read refuses. +set -uo pipefail +unset GIT_DIR GIT_WORK_TREE GIT_CONFIG + +S="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)/merge-hold.sh" +TMP="$(mktemp -d)" +trap 'rm -rf "$TMP"' EXIT +FAILED=0 + +check() { # + if [[ "$2" == "$4" && "$3" == "$5" ]]; then + printf 'PASS: %s\n' "$1" + else + FAILED=$((FAILED + 1)) + printf 'FAIL: %s\n expected rc=%s out=[%s], got rc=%s out=[%s]\n' "$1" "$2" "$3" "$4" "$5" >&2 + fi +} + +command -v jq >/dev/null 2>&1 || { + echo "SKIP: jq not installed" >&2 + exit 0 +} + +mkdir -p "$TMP/bin" "$TMP/perms" +cat >"$TMP/bin/gh" <<'EOF' +#!/usr/bin/env bash +path="" prog="" +while [[ $# -gt 0 ]]; do + case "$1" in + --jq) prog="$2"; shift 2 ;; + api | --paginate) shift ;; + *) path="$1"; shift ;; + esac +done +case "$path" in +*/comments*) + [[ -z "${GH_STUB_COMMENTS_ERR:-}" ]] || { echo "gh: HTTP 502" >&2; exit 1; } + jq -r "$prog" <<<"$GH_STUB_COMMENTS" + ;; +*/collaborators/*/permission) + login="${path#*/collaborators/}" + login="${login%/permission}" + f="$GH_STUB_PERM_DIR/$login.json" + [[ -f "$f" ]] || { echo "gh: Not Found (HTTP 404)" >&2; exit 1; } + jq -r "$prog" "$f" + ;; +*) exit 90 ;; +esac +EOF +chmod +x "$TMP/bin/gh" +printf '{"permission":"admin","role_name":"admin"}' >"$TMP/perms/owner.json" +printf '{"permission":"write","role_name":"maintain"}' >"$TMP/perms/maint.json" +printf '{"permission":"read","role_name":"triage"}' >"$TMP/perms/triager.json" +printf '{"permission":"read","role_name":"read"}' >"$TMP/perms/stranger.json" + +OWNER_SAYS="Owner ratified this: put 'Hold: a human merges this' in the PR body." +comment() { jq -nc --arg u "$1" --arg b "$2" '{user: {login: $u}, body: $b}'; } + +run() { # [env...] + local out rc + out="$(env PATH="$TMP/bin:$PATH" GH_STUB_PERM_DIR="$TMP/perms" GH_STUB_COMMENTS="$1" "${@:2}" \ + bash "$S" --repo o/r --issue 7 2>/dev/null)" + rc=$? + printf '%s|%s' "$rc" "$out" +} +verdict() { # [env...] + local r + r="$(run "${@:4}")" + check "$1" "$2" "$3" "${r%%|*}" "${r#*|}" +} + +HOLD="Hold: a human merges this" +verdict "admin comment quoting the line holds" 0 "$HOLD" "[$(comment owner "$OWNER_SAYS")]" +verdict "maintain (write) comment holds" 0 "$HOLD" "[$(comment maint "$HOLD")]" +verdict "triage commenter does not hold" 1 "" "[$(comment triager "$HOLD")]" +verdict "read-only commenter does not hold" 1 "" "[$(comment stranger "$OWNER_SAYS")]" +verdict "untrusted line plus trusted line holds" 0 "$HOLD" \ + "[$(comment stranger "$HOLD"),$(comment owner "$OWNER_SAYS")]" +verdict "owner comment without the line does not hold" 1 "" "[$(comment owner "looks good")]" +verdict "no comments does not hold" 1 "" "[]" +verdict "different case is not the line" 1 "" "[$(comment owner "hold: a human merges this")]" +verdict "comment read failure refuses" 11 "" "[]" GH_STUB_COMMENTS_ERR=1 +verdict "permission read failure refuses" 11 "" "[$(comment ghost "$HOLD")]" +verdict "malformed login is ignored" 1 "" "[$(comment 'a;b' "$HOLD")]" + +for args in "" "--repo o/r" "--issue 7" "--repo bad --issue 7" "--repo o/r --issue 0" "--repo o/r --issue 7 --x 1"; do + # shellcheck disable=SC2086 # word splitting is the point: each string is an argument list + bash "$S" $args >/dev/null 2>&1 + check "usage error: [$args]" 2 "" "$?" "" +done + +((FAILED == 0)) || { + echo "$FAILED failed" >&2 + exit 1 +} +echo "all passed" diff --git a/plugins/work-items/skills/ship/actions/run.md b/plugins/work-items/skills/ship/actions/run.md index 48f7008d64..ac8eb40fd3 100644 --- a/plugins/work-items/skills/ship/actions/run.md +++ b/plugins/work-items/skills/ship/actions/run.md @@ -82,7 +82,9 @@ any run with no human to answer, STOP before any claim and report that `run` is through `/work-items:track done` with a summary naming the branch: the checkpoint. No per-item PR. - **`per-item PRs`.** Each accepted item has a pull request against the default branch; open a - missing one with `/source-control:pull-request create --pushed --worktree `. **Then ready + missing one with `/source-control:pull-request create --pushed --worktree `. Before + `ready`, carry the item's human-merge hold into the PR body exactly as `/work-items:work`'s + open-the-PR step does with `${CLAUDE_PLUGIN_ROOT}/scripts/merge-hold.sh`. **Then ready it:** right after `create --pushed`, unless the consuming project's instructions reserve the ready flip for a person, dispatch `/source-control:pull-request ready` into the item's worktree, at the strong tier's current alias resolved per the loop-lane convention's capability tiers, diff --git a/plugins/work-items/skills/work/SKILL.md b/plugins/work-items/skills/work/SKILL.md index 44c34136e3..6165f587c1 100644 --- a/plugins/work-items/skills/work/SKILL.md +++ b/plugins/work-items/skills/work/SKILL.md @@ -213,7 +213,7 @@ On user confirmation ("yes"): 1. **High-blast-radius diff gate (pre-PR).** Before a PR is opened, the orchestrator does a **full-diff read** when the diff touches skill frontmatter descriptions or trigger keywords, cross-plugin contracts, or hooks. Read against the worker's returned worktree (`git -C `), since the orchestrator's own default-branch checkout does not contain the worker's changes. This complements the worker scope-fence: the scope-fence bounds what a worker *may* touch, this gate is the orchestrator's own read of what the worker *did* touch before the change leaves the lane. -1. **Open the PR. Orchestrator-owned.** After the worker returns and the pre-PR diff gate passes, the **orchestrator** opens the PR. It is never the worker's to open (opening it from a worker would make the pre-PR gate a no-op, and `/implementation:implement-dispatch` already keeps PR creation out of every worker brief). The worker committed and pushed inside its own worktree, so the orchestrator invokes the **PR-only entry** `/source-control:pull-request create --pushed --worktree `. Passing that path explicitly, since the orchestrator stays in its own (default-branch) checkout and the mode needs the path to resolve the worker's branch and diff rather than the orchestrator's. That mode re-resolves branch and diff from the target worktree and skips the commit/push/rebase steps the normal `create` runs, while `/source-control:pull-request` stays the SSOT for the PR body shape, the closing-line injection (text from the bound adapter), the required-section gate, and merge style. **Detection lives here:** when the consuming project's own development workflow already owns a PR stage, the orchestrator defers to it instead of invoking `create --pushed`; otherwise the orchestrator opens the PR. **Then ready it:** right after `create --pushed`, unless the consuming project's instructions reserve the ready flip for a person, the orchestrator dispatches `/source-control:pull-request ready` into the persisted worktree, at the same strong-tier `model` as the post-green review pass. `ready` resolves the PR from the branch checked out there, so the brief names the PR number `create --pushed` returned and the dispatched agent stops when `ready` resolves a different one. The strong tier fits because `ready` merges the base, runs the code and security reviews, verifies the merged head, and only then flips the draft. When `ready` refuses (a hold, a superseded item, a failed gate), the PR stays a draft: carry the reason it reported into this run's report, and never lift the hold or flip the PR another way. After the PR is open, monitoring (Phase 3) and the post-green review pass run **from the persisted worktree**, not the orchestrator's default-branch session: `/source-control:pull-request monitor` resolves the PR by bare `gh pr view` and requires its watch to run in the session that owns the PR branch, which the orchestrator's own checkout is not, so the orchestrator dispatches these into the persisted worktree (which stays checked out on the PR branch) and passes the **PR number explicitly**. Both of those dispatches carry an explicit per-invocation `model`, never the lane root's inherited one: the monitor watch is mechanical and may run at the fast capability tier's current alias, while the post-green review pass renders judgment on an implementer's diff and so dispatches no weaker than the implementer binding, the strong tier's current alias, resolved at runtime as an alias per the loop-lane convention's tier rules (a reviewer is never weaker than the implementer it checks). Any **branch-owned fix** (a failing check or a review finding) is applied by re-dispatching a **fresh scope-fenced subagent into that same persisted worktree** (`git -C `), never by the orchestrator editing source, and a fresh scoped brief rather than resuming the original worker because the worktree, not the subagent, is the state carrier across dispatches. Collect a round's fixes before dispatching: wait until every CI and reviewer check run on the head has finished, then hand every failing check and VALID finding to one fix worker that commits and pushes once, since each push starts a full CI run and cancels the one in flight. **Before any push to the open PR** (a fix or a rebase), auto-merge is turned off so the new head cannot merge on `ci-status` alone before the AI reviews re-run. The fix worker's brief carries the step, since the worker pushes: immediately before the push, and again right after it (someone may arm the old head while the worker runs), when `gh pr view --json autoMergeRequest --jq '.autoMergeRequest'` is non-null, run `gh pr merge --disable-auto`. Neither the worker nor the orchestrator re-arms, and no lane arms auto-merge. Verified 2026-09-26 against gh 2.98.0: [`gh pr merge`](https://cli.github.com/manual/gh_pr_merge) documents `--disable-auto`, and `gh pr view --help` lists the `autoMergeRequest` JSON field. Recheck when a gh release note renames either. Dispatch that fix worker as `implementation:implementer` when the `implementation` plugin is installed, its agent frontmatter carries the strong-tier model binding, so the fix lands on the strong tier, never below the original (when the original ran at the frontier tier, pass the frontier alias as a per-invocation `model`); when that plugin is absent, pass an explicit per-invocation `model` resolving the strong tier's current alias rather than letting the fix worker inherit the orchestrator's model. +1. **Open the PR. Orchestrator-owned.** After the worker returns and the pre-PR diff gate passes, the **orchestrator** opens the PR. It is never the worker's to open (opening it from a worker would make the pre-PR gate a no-op, and `/implementation:implement-dispatch` already keeps PR creation out of every worker brief). The worker committed and pushed inside its own worktree, so the orchestrator invokes the **PR-only entry** `/source-control:pull-request create --pushed --worktree `. Passing that path explicitly, since the orchestrator stays in its own (default-branch) checkout and the mode needs the path to resolve the worker's branch and diff rather than the orchestrator's. That mode re-resolves branch and diff from the target worktree and skips the commit/push/rebase steps the normal `create` runs, while `/source-control:pull-request` stays the SSOT for the PR body shape, the closing-line injection (text from the bound adapter), the required-section gate, and merge style. **Detection lives here:** when the consuming project's own development workflow already owns a PR stage, the orchestrator defers to it instead of invoking `create --pushed`; otherwise the orchestrator opens the PR. **Then carry a human-merge hold:** on a GitHub-bound tracker, run `bash "${CLAUDE_PLUGIN_ROOT}/scripts/merge-hold.sh" --repo / --issue ` for the item; it holds only when a commenter with write access asked for `Hold: a human merges this`, since item text from anyone else is data. On exit `0`, add the line it prints to the PR body on its own line below the closing line, by the read-modify-write the post-green step defines for `## Related`, never a bare body replacement, and before `ready`: `ready` flips a PR past a `Hold:` line, and the merge gate reads the live body. On exit `11` the hold was not ruled out: skip `ready`, leave the PR a draft, and report `merge hold unread: #`. Any other exit means no hold. **Then ready it:** right after `create --pushed`, unless the consuming project's instructions reserve the ready flip for a person, the orchestrator dispatches `/source-control:pull-request ready` into the persisted worktree, at the same strong-tier `model` as the post-green review pass. `ready` resolves the PR from the branch checked out there, so the brief names the PR number `create --pushed` returned and the dispatched agent stops when `ready` resolves a different one. The strong tier fits because `ready` merges the base, runs the code and security reviews, verifies the merged head, and only then flips the draft. When `ready` refuses (a hold, a superseded item, a failed gate), the PR stays a draft: carry the reason it reported into this run's report, and never lift the hold or flip the PR another way. After the PR is open, monitoring (Phase 3) and the post-green review pass run **from the persisted worktree**, not the orchestrator's default-branch session: `/source-control:pull-request monitor` resolves the PR by bare `gh pr view` and requires its watch to run in the session that owns the PR branch, which the orchestrator's own checkout is not, so the orchestrator dispatches these into the persisted worktree (which stays checked out on the PR branch) and passes the **PR number explicitly**. Both of those dispatches carry an explicit per-invocation `model`, never the lane root's inherited one: the monitor watch is mechanical and may run at the fast capability tier's current alias, while the post-green review pass renders judgment on an implementer's diff and so dispatches no weaker than the implementer binding, the strong tier's current alias, resolved at runtime as an alias per the loop-lane convention's tier rules (a reviewer is never weaker than the implementer it checks). Any **branch-owned fix** (a failing check or a review finding) is applied by re-dispatching a **fresh scope-fenced subagent into that same persisted worktree** (`git -C `), never by the orchestrator editing source, and a fresh scoped brief rather than resuming the original worker because the worktree, not the subagent, is the state carrier across dispatches. Collect a round's fixes before dispatching: wait until every CI and reviewer check run on the head has finished, then hand every failing check and VALID finding to one fix worker that commits and pushes once, since each push starts a full CI run and cancels the one in flight. **Before any push to the open PR** (a fix or a rebase), auto-merge is turned off so the new head cannot merge on `ci-status` alone before the AI reviews re-run. The fix worker's brief carries the step, since the worker pushes: immediately before the push, and again right after it (someone may arm the old head while the worker runs), when `gh pr view --json autoMergeRequest --jq '.autoMergeRequest'` is non-null, run `gh pr merge --disable-auto`. Neither the worker nor the orchestrator re-arms, and no lane arms auto-merge. Verified 2026-09-26 against gh 2.98.0: [`gh pr merge`](https://cli.github.com/manual/gh_pr_merge) documents `--disable-auto`, and `gh pr view --help` lists the `autoMergeRequest` JSON field. Recheck when a gh release note renames either. Dispatch that fix worker as `implementation:implementer` when the `implementation` plugin is installed, its agent frontmatter carries the strong-tier model binding, so the fix lands on the strong tier, never below the original (when the original ran at the frontier tier, pass the frontier alias as a per-invocation `model`); when that plugin is absent, pass an explicit per-invocation `model` resolving the strong tier's current alias rather than letting the fix worker inherit the orchestrator's model. 1. **Post-green review pass, then hand off.** After CI is green, run one review pass. The fetch-once → validate → classify → threaded-reply → react → resolve-bot-thread loop is owned by `/source-control:pull-request`; this skill adds only the sequencing and the work-item linkage: fix findings via the same **fresh-subagent-into-the-persisted-worktree** re-dispatch the orchestrator-owned PR step defines (the orchestrator still never edits source). **Fix, don't file:** a small or medium VALID finding is fixed, never filed, and where an unrelated one lands is the scope test `/source-control:pull-request` owns. Only a finding that already reproduces on the base branch may defer; one this change introduced is fixed now at any size (the provenance test `/source-control:pull-request` owns). Of those, only three kinds are deferred: a **structural** finding, large enough to need its own planning pass; an **urgent, real** problem that cannot land in this PR; and a finding **blocked on research** this lane is not positioned to do. Nothing else is filed: no nits, no speculative items, no busy work. A **deferred finding requires a filed follow-up issue**. File it by invoking `/work-items:track add` via the Skill tool following the shared self-observation contract ([`${CLAUDE_PLUGIN_ROOT}/reference/dogfood-filing.md`](${CLAUDE_PLUGIN_ROOT}/reference/dogfood-filing.md): dedupe → categorize → fixed shape → `needs-triage`), then cite that issue **both** in the classification reply **and** in the PR's `## Related` section. **ensure the section exists first**: `/source-control:pull-request`'s scaffold carries `## Related` only when the repo's `pr_body_required_sections` requires it or a genuine reference already populated it at create time, so a deferred finding is frequently the first content that section ever holds. Adding it is a **read-modify-write**, never a bare `--body` replacement. `gh pr edit --body`/`--body-file` REPLACES the whole body (the same identity note the GitHub adapter's [PR closing-keyword mechanics](../../tools/work-item-tracker/adapters/github/README.md) documents for its own body edit), so read the current body first (`gh pr view --json body --jq '.body'`), append the `## Related` section (or its content, if the section already exists) to that read, and write the combined result back via `--body-file -`; a bare `gh pr edit --body "## Related\n..."` would silently drop the closing line and every section `/source-control:pull-request create` wrote. Make that body write before the push that carries the round's fixes, or, with no fix to push, while no CI run is in flight on the head; never as its own edit right after a push, since a body edit re-runs every workflow that triggers on `edited` beside the push's own run. A deferred finding cannot be resolved without it. Then hand the PR off by invoking `/source-control:babysit-prs` via the Skill tool (fleet loop, owned there). From 33b3969120cec2234c1a286481414b9090982178 Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Tue, 13 Oct 2026 18:24:23 -0400 Subject: [PATCH 2/3] fix(work-items): leave the PR a draft on any merge-hold exit but 1 Only exit 1 rules a hold out; usage, missing-tool and unresolved-path exits now keep the PR a draft like a failed read. merge-hold.sh checks only gh, since its jq programs run through gh --jq. Tests add the missing-gh exit and a failed permission read beside a writer. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01W6SjPvY7DmXBGzboeCq8nj --- plugins/work-items/scripts/merge-hold.sh | 12 +++++------- plugins/work-items/scripts/merge-hold.test.sh | 6 ++++++ plugins/work-items/skills/work/SKILL.md | 2 +- 3 files changed, 12 insertions(+), 8 deletions(-) diff --git a/plugins/work-items/scripts/merge-hold.sh b/plugins/work-items/scripts/merge-hold.sh index e3274f26d1..9914a357f2 100755 --- a/plugins/work-items/scripts/merge-hold.sh +++ b/plugins/work-items/scripts/merge-hold.sh @@ -16,7 +16,7 @@ # 0 held: stdout is `Hold: a human merges this` # 1 not held: no comment carries the line, or none from a writer # 2 usage: a missing, repeated, or unknown argument, or a malformed value -# 8 prerequisite missing: gh or jq is not on PATH +# 8 prerequisite missing: gh is not on PATH # 11 a read failed or returned unparsable JSON, so a hold was not ruled out # portability-scope: the GitHub implementation of the human-merge hold read; every read is a gh call. @@ -68,12 +68,10 @@ done err "--issue must be a positive integer" exit 2 } -for tool in gh jq; do - command -v "$tool" >/dev/null 2>&1 || { - err "$tool is not on PATH" - exit 8 - } -done +command -v gh >/dev/null 2>&1 || { + err "gh is not on PATH" + exit 8 +} # Logins of every comment that carries the line, one per line. if ! authors="$(gh api --paginate "repos/$REPO/issues/$ISSUE/comments?per_page=100" \ diff --git a/plugins/work-items/scripts/merge-hold.test.sh b/plugins/work-items/scripts/merge-hold.test.sh index decceac269..d85f0a5866 100755 --- a/plugins/work-items/scripts/merge-hold.test.sh +++ b/plugins/work-items/scripts/merge-hold.test.sh @@ -86,6 +86,12 @@ verdict "different case is not the line" 1 "" "[$(comment owner "hold: a human m verdict "comment read failure refuses" 11 "" "[]" GH_STUB_COMMENTS_ERR=1 verdict "permission read failure refuses" 11 "" "[$(comment ghost "$HOLD")]" verdict "malformed login is ignored" 1 "" "[$(comment 'a;b' "$HOLD")]" +verdict "failed permission read beside a writer holds" 0 "$HOLD" \ + "[$(comment ghost "$HOLD"),$(comment owner "$HOLD")]" + +mkdir -p "$TMP/nogh" +env PATH="$TMP/nogh" "$(command -v bash)" "$S" --repo o/r --issue 7 >/dev/null 2>&1 +check "gh missing exits 8" 8 "" "$?" "" for args in "" "--repo o/r" "--issue 7" "--repo bad --issue 7" "--repo o/r --issue 0" "--repo o/r --issue 7 --x 1"; do # shellcheck disable=SC2086 # word splitting is the point: each string is an argument list diff --git a/plugins/work-items/skills/work/SKILL.md b/plugins/work-items/skills/work/SKILL.md index 6165f587c1..d806760bbe 100644 --- a/plugins/work-items/skills/work/SKILL.md +++ b/plugins/work-items/skills/work/SKILL.md @@ -213,7 +213,7 @@ On user confirmation ("yes"): 1. **High-blast-radius diff gate (pre-PR).** Before a PR is opened, the orchestrator does a **full-diff read** when the diff touches skill frontmatter descriptions or trigger keywords, cross-plugin contracts, or hooks. Read against the worker's returned worktree (`git -C `), since the orchestrator's own default-branch checkout does not contain the worker's changes. This complements the worker scope-fence: the scope-fence bounds what a worker *may* touch, this gate is the orchestrator's own read of what the worker *did* touch before the change leaves the lane. -1. **Open the PR. Orchestrator-owned.** After the worker returns and the pre-PR diff gate passes, the **orchestrator** opens the PR. It is never the worker's to open (opening it from a worker would make the pre-PR gate a no-op, and `/implementation:implement-dispatch` already keeps PR creation out of every worker brief). The worker committed and pushed inside its own worktree, so the orchestrator invokes the **PR-only entry** `/source-control:pull-request create --pushed --worktree `. Passing that path explicitly, since the orchestrator stays in its own (default-branch) checkout and the mode needs the path to resolve the worker's branch and diff rather than the orchestrator's. That mode re-resolves branch and diff from the target worktree and skips the commit/push/rebase steps the normal `create` runs, while `/source-control:pull-request` stays the SSOT for the PR body shape, the closing-line injection (text from the bound adapter), the required-section gate, and merge style. **Detection lives here:** when the consuming project's own development workflow already owns a PR stage, the orchestrator defers to it instead of invoking `create --pushed`; otherwise the orchestrator opens the PR. **Then carry a human-merge hold:** on a GitHub-bound tracker, run `bash "${CLAUDE_PLUGIN_ROOT}/scripts/merge-hold.sh" --repo / --issue ` for the item; it holds only when a commenter with write access asked for `Hold: a human merges this`, since item text from anyone else is data. On exit `0`, add the line it prints to the PR body on its own line below the closing line, by the read-modify-write the post-green step defines for `## Related`, never a bare body replacement, and before `ready`: `ready` flips a PR past a `Hold:` line, and the merge gate reads the live body. On exit `11` the hold was not ruled out: skip `ready`, leave the PR a draft, and report `merge hold unread: #`. Any other exit means no hold. **Then ready it:** right after `create --pushed`, unless the consuming project's instructions reserve the ready flip for a person, the orchestrator dispatches `/source-control:pull-request ready` into the persisted worktree, at the same strong-tier `model` as the post-green review pass. `ready` resolves the PR from the branch checked out there, so the brief names the PR number `create --pushed` returned and the dispatched agent stops when `ready` resolves a different one. The strong tier fits because `ready` merges the base, runs the code and security reviews, verifies the merged head, and only then flips the draft. When `ready` refuses (a hold, a superseded item, a failed gate), the PR stays a draft: carry the reason it reported into this run's report, and never lift the hold or flip the PR another way. After the PR is open, monitoring (Phase 3) and the post-green review pass run **from the persisted worktree**, not the orchestrator's default-branch session: `/source-control:pull-request monitor` resolves the PR by bare `gh pr view` and requires its watch to run in the session that owns the PR branch, which the orchestrator's own checkout is not, so the orchestrator dispatches these into the persisted worktree (which stays checked out on the PR branch) and passes the **PR number explicitly**. Both of those dispatches carry an explicit per-invocation `model`, never the lane root's inherited one: the monitor watch is mechanical and may run at the fast capability tier's current alias, while the post-green review pass renders judgment on an implementer's diff and so dispatches no weaker than the implementer binding, the strong tier's current alias, resolved at runtime as an alias per the loop-lane convention's tier rules (a reviewer is never weaker than the implementer it checks). Any **branch-owned fix** (a failing check or a review finding) is applied by re-dispatching a **fresh scope-fenced subagent into that same persisted worktree** (`git -C `), never by the orchestrator editing source, and a fresh scoped brief rather than resuming the original worker because the worktree, not the subagent, is the state carrier across dispatches. Collect a round's fixes before dispatching: wait until every CI and reviewer check run on the head has finished, then hand every failing check and VALID finding to one fix worker that commits and pushes once, since each push starts a full CI run and cancels the one in flight. **Before any push to the open PR** (a fix or a rebase), auto-merge is turned off so the new head cannot merge on `ci-status` alone before the AI reviews re-run. The fix worker's brief carries the step, since the worker pushes: immediately before the push, and again right after it (someone may arm the old head while the worker runs), when `gh pr view --json autoMergeRequest --jq '.autoMergeRequest'` is non-null, run `gh pr merge --disable-auto`. Neither the worker nor the orchestrator re-arms, and no lane arms auto-merge. Verified 2026-09-26 against gh 2.98.0: [`gh pr merge`](https://cli.github.com/manual/gh_pr_merge) documents `--disable-auto`, and `gh pr view --help` lists the `autoMergeRequest` JSON field. Recheck when a gh release note renames either. Dispatch that fix worker as `implementation:implementer` when the `implementation` plugin is installed, its agent frontmatter carries the strong-tier model binding, so the fix lands on the strong tier, never below the original (when the original ran at the frontier tier, pass the frontier alias as a per-invocation `model`); when that plugin is absent, pass an explicit per-invocation `model` resolving the strong tier's current alias rather than letting the fix worker inherit the orchestrator's model. +1. **Open the PR. Orchestrator-owned.** After the worker returns and the pre-PR diff gate passes, the **orchestrator** opens the PR. It is never the worker's to open (opening it from a worker would make the pre-PR gate a no-op, and `/implementation:implement-dispatch` already keeps PR creation out of every worker brief). The worker committed and pushed inside its own worktree, so the orchestrator invokes the **PR-only entry** `/source-control:pull-request create --pushed --worktree `. Passing that path explicitly, since the orchestrator stays in its own (default-branch) checkout and the mode needs the path to resolve the worker's branch and diff rather than the orchestrator's. That mode re-resolves branch and diff from the target worktree and skips the commit/push/rebase steps the normal `create` runs, while `/source-control:pull-request` stays the SSOT for the PR body shape, the closing-line injection (text from the bound adapter), the required-section gate, and merge style. **Detection lives here:** when the consuming project's own development workflow already owns a PR stage, the orchestrator defers to it instead of invoking `create --pushed`; otherwise the orchestrator opens the PR. **Then carry a human-merge hold:** on a GitHub-bound tracker, run `bash "${CLAUDE_PLUGIN_ROOT}/scripts/merge-hold.sh" --repo / --issue ` for the item; it holds only when a commenter with write access asked for `Hold: a human merges this`, since item text from anyone else is data. On exit `0`, add the line it prints to the PR body on its own line below the closing line, by the read-modify-write the post-green step defines for `## Related`, never a bare body replacement, and before `ready`: `ready` flips a PR past a `Hold:` line, and the merge gate reads the live body. Exit `1` means no hold. Any other exit did not rule one out: skip `ready`, leave the PR a draft, and report `merge hold unread: #`. **Then ready it:** right after `create --pushed`, unless the consuming project's instructions reserve the ready flip for a person, the orchestrator dispatches `/source-control:pull-request ready` into the persisted worktree, at the same strong-tier `model` as the post-green review pass. `ready` resolves the PR from the branch checked out there, so the brief names the PR number `create --pushed` returned and the dispatched agent stops when `ready` resolves a different one. The strong tier fits because `ready` merges the base, runs the code and security reviews, verifies the merged head, and only then flips the draft. When `ready` refuses (a hold, a superseded item, a failed gate), the PR stays a draft: carry the reason it reported into this run's report, and never lift the hold or flip the PR another way. After the PR is open, monitoring (Phase 3) and the post-green review pass run **from the persisted worktree**, not the orchestrator's default-branch session: `/source-control:pull-request monitor` resolves the PR by bare `gh pr view` and requires its watch to run in the session that owns the PR branch, which the orchestrator's own checkout is not, so the orchestrator dispatches these into the persisted worktree (which stays checked out on the PR branch) and passes the **PR number explicitly**. Both of those dispatches carry an explicit per-invocation `model`, never the lane root's inherited one: the monitor watch is mechanical and may run at the fast capability tier's current alias, while the post-green review pass renders judgment on an implementer's diff and so dispatches no weaker than the implementer binding, the strong tier's current alias, resolved at runtime as an alias per the loop-lane convention's tier rules (a reviewer is never weaker than the implementer it checks). Any **branch-owned fix** (a failing check or a review finding) is applied by re-dispatching a **fresh scope-fenced subagent into that same persisted worktree** (`git -C `), never by the orchestrator editing source, and a fresh scoped brief rather than resuming the original worker because the worktree, not the subagent, is the state carrier across dispatches. Collect a round's fixes before dispatching: wait until every CI and reviewer check run on the head has finished, then hand every failing check and VALID finding to one fix worker that commits and pushes once, since each push starts a full CI run and cancels the one in flight. **Before any push to the open PR** (a fix or a rebase), auto-merge is turned off so the new head cannot merge on `ci-status` alone before the AI reviews re-run. The fix worker's brief carries the step, since the worker pushes: immediately before the push, and again right after it (someone may arm the old head while the worker runs), when `gh pr view --json autoMergeRequest --jq '.autoMergeRequest'` is non-null, run `gh pr merge --disable-auto`. Neither the worker nor the orchestrator re-arms, and no lane arms auto-merge. Verified 2026-09-26 against gh 2.98.0: [`gh pr merge`](https://cli.github.com/manual/gh_pr_merge) documents `--disable-auto`, and `gh pr view --help` lists the `autoMergeRequest` JSON field. Recheck when a gh release note renames either. Dispatch that fix worker as `implementation:implementer` when the `implementation` plugin is installed, its agent frontmatter carries the strong-tier model binding, so the fix lands on the strong tier, never below the original (when the original ran at the frontier tier, pass the frontier alias as a per-invocation `model`); when that plugin is absent, pass an explicit per-invocation `model` resolving the strong tier's current alias rather than letting the fix worker inherit the orchestrator's model. 1. **Post-green review pass, then hand off.** After CI is green, run one review pass. The fetch-once → validate → classify → threaded-reply → react → resolve-bot-thread loop is owned by `/source-control:pull-request`; this skill adds only the sequencing and the work-item linkage: fix findings via the same **fresh-subagent-into-the-persisted-worktree** re-dispatch the orchestrator-owned PR step defines (the orchestrator still never edits source). **Fix, don't file:** a small or medium VALID finding is fixed, never filed, and where an unrelated one lands is the scope test `/source-control:pull-request` owns. Only a finding that already reproduces on the base branch may defer; one this change introduced is fixed now at any size (the provenance test `/source-control:pull-request` owns). Of those, only three kinds are deferred: a **structural** finding, large enough to need its own planning pass; an **urgent, real** problem that cannot land in this PR; and a finding **blocked on research** this lane is not positioned to do. Nothing else is filed: no nits, no speculative items, no busy work. A **deferred finding requires a filed follow-up issue**. File it by invoking `/work-items:track add` via the Skill tool following the shared self-observation contract ([`${CLAUDE_PLUGIN_ROOT}/reference/dogfood-filing.md`](${CLAUDE_PLUGIN_ROOT}/reference/dogfood-filing.md): dedupe → categorize → fixed shape → `needs-triage`), then cite that issue **both** in the classification reply **and** in the PR's `## Related` section. **ensure the section exists first**: `/source-control:pull-request`'s scaffold carries `## Related` only when the repo's `pr_body_required_sections` requires it or a genuine reference already populated it at create time, so a deferred finding is frequently the first content that section ever holds. Adding it is a **read-modify-write**, never a bare `--body` replacement. `gh pr edit --body`/`--body-file` REPLACES the whole body (the same identity note the GitHub adapter's [PR closing-keyword mechanics](../../tools/work-item-tracker/adapters/github/README.md) documents for its own body edit), so read the current body first (`gh pr view --json body --jq '.body'`), append the `## Related` section (or its content, if the section already exists) to that read, and write the combined result back via `--body-file -`; a bare `gh pr edit --body "## Related\n..."` would silently drop the closing line and every section `/source-control:pull-request create` wrote. Make that body write before the push that carries the round's fixes, or, with no fix to push, while no CI run is in flight on the head; never as its own edit right after a push, since a body edit re-runs every workflow that triggers on `edited` beside the push's own run. A deferred finding cannot be resolved without it. Then hand the PR off by invoking `/source-control:babysit-prs` via the Skill tool (fleet loop, owned there). From a4480d1acfd4365504258d3b0e146c6b819c3e12 Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Tue, 13 Oct 2026 18:32:42 -0400 Subject: [PATCH 3/3] fix(work-items): accept managed-user logins in merge-hold Enterprise Managed Users logins carry an underscore before the shortcode, which the login pattern rejected, so a writer's hold was skipped. Accept the underscore, and treat a login that still fails the pattern as an unread hold (exit 11) rather than no hold. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01W6SjPvY7DmXBGzboeCq8nj --- plugins/work-items/scripts/merge-hold.sh | 5 +++-- plugins/work-items/scripts/merge-hold.test.sh | 4 +++- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/plugins/work-items/scripts/merge-hold.sh b/plugins/work-items/scripts/merge-hold.sh index 9914a357f2..cdeae5e82a 100755 --- a/plugins/work-items/scripts/merge-hold.sh +++ b/plugins/work-items/scripts/merge-hold.sh @@ -80,12 +80,13 @@ if ! authors="$(gh api --paginate "repos/$REPO/issues/$ISSUE/comments?per_page=1 exit 11 fi -login_re='^[A-Za-z0-9][A-Za-z0-9-]*(\[bot\])?$' +login_re='^[A-Za-z0-9][A-Za-z0-9_-]*(\[bot\])?$' read_failed=0 while IFS= read -r login; do [[ -n "$login" ]] || continue if [[ ! "$login" =~ $login_re ]]; then - err "ignored a hold comment with no usable author login" + err "hold comment author login is not a usable path segment" + read_failed=1 continue fi if ! perm="$(gh api "repos/$REPO/collaborators/$login/permission" --jq '.permission // ""' 2>/dev/null)"; then diff --git a/plugins/work-items/scripts/merge-hold.test.sh b/plugins/work-items/scripts/merge-hold.test.sh index d85f0a5866..20bed08ee5 100755 --- a/plugins/work-items/scripts/merge-hold.test.sh +++ b/plugins/work-items/scripts/merge-hold.test.sh @@ -56,6 +56,7 @@ printf '{"permission":"admin","role_name":"admin"}' >"$TMP/perms/owner.json" printf '{"permission":"write","role_name":"maintain"}' >"$TMP/perms/maint.json" printf '{"permission":"read","role_name":"triage"}' >"$TMP/perms/triager.json" printf '{"permission":"read","role_name":"read"}' >"$TMP/perms/stranger.json" +printf '{"permission":"admin","role_name":"admin"}' >"$TMP/perms/mona-cat_octo.json" OWNER_SAYS="Owner ratified this: put 'Hold: a human merges this' in the PR body." comment() { jq -nc --arg u "$1" --arg b "$2" '{user: {login: $u}, body: $b}'; } @@ -85,7 +86,8 @@ verdict "no comments does not hold" 1 "" "[]" verdict "different case is not the line" 1 "" "[$(comment owner "hold: a human merges this")]" verdict "comment read failure refuses" 11 "" "[]" GH_STUB_COMMENTS_ERR=1 verdict "permission read failure refuses" 11 "" "[$(comment ghost "$HOLD")]" -verdict "malformed login is ignored" 1 "" "[$(comment 'a;b' "$HOLD")]" +verdict "unusable login refuses" 11 "" "[$(comment 'a;b' "$HOLD")]" +verdict "managed-user login with an underscore holds" 0 "$HOLD" "[$(comment mona-cat_octo "$HOLD")]" verdict "failed permission read beside a writer holds" 0 "$HOLD" \ "[$(comment ghost "$HOLD"),$(comment owner "$HOLD")]"