Skip to content

docs(skills): maintainer-driven tool actions and duplicate-cluster resolution - #970

Open
castrojo wants to merge 1 commit into
mainfrom
pr-review-cluster-doctrine
Open

docs(skills): maintainer-driven tool actions and duplicate-cluster resolution#970
castrojo wants to merge 1 commit into
mainfrom
pr-review-cluster-doctrine

Conversation

@castrojo

@castrojo castrojo commented Aug 8, 2026

Copy link
Copy Markdown
Contributor

Why

Downstream in projectbluefin/review, the maintainer's PR-walk tool (bluefin-review queue) detects duplicate clusters but could not act on them: a local test banned pr merge outright — stricter than the doctrine it implements. The ambiguity sat here: human-gates said "agents never self-merge" without distinguishing a tool executing the maintainer's explicit keypress, and pr-review had competing-pair detection with no cluster resolution verb.

What changes

  • human-gates.md (1.1): the Merge Gate now distinguishes agent self-merge (never) from a tool executing the maintainer's explicit per-item keypress with rulesets still enforced (the documented human-decides loop). --admin, submitting reviews, and non-interactive batch mutation stay forbidden for any tool.
  • pr-review/SKILL.md (3.5): new "Duplicate-cluster resolution" subsection — survivor from diff evidence, arm the survivor first with --match-head-commit, comment before close, never --reason "not planned", linked-issue re-check, halt on first failure. Compressed to fit the 500-line hard cap (exactly 500; one stray blank line reclaimed).

Design memo driving this: projectbluefin/review session artifact, org review walk design (§0, §4, §9).

Validation

check-skill-frontmatter.sh exit 0 (500/500, warnings pre-existing), generate_skill_index.py --write + check-skill-index.sh clean.

Assisted-by: Claude Opus 4.8 via GitHub Copilot
Co-authored-by: Copilot 223556219+Copilot@users.noreply.github.com

…solution

human-gates: the Merge Gate binds agents, not the maintainer's own hands.
A review tool that executes a merge or close only on the maintainer's
explicit per-item keypress, with rulesets still enforced by GitHub, is the
human acting at the gate; --admin overrides, submitting reviews, and
non-interactive batch mutation remain forbidden for any tool. This
ambiguity had already produced a walker stricter than the doctrine it
implements.

pr-review: competing-pair detection could find a duplicate cluster but had
no verb for resolving it. Add the ordered procedure: survivor from diff
evidence, arm the survivor first with --match-head-commit, comment before
close, never 'not planned', then the linked-issue re-check — halting on
the first failure.

Assisted-by: Claude Opus 4.8 via GitHub Copilot
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@castrojo
castrojo requested review from a team and repires as code owners August 8, 2026 22:48
@castrojo
castrojo enabled auto-merge August 8, 2026 22:55

@hanthor hanthor left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving. The carve-out is correctly bounded and the duplicate-cluster procedure is careful in the places that matter.

On the human-gates amendment. The distinction it draws is the right one — the concern behind "agents never self-merge" is unreviewed change reaching a protected branch, not the mechanical identity of the process invoking the API. A per-item maintainer decision with rulesets still enforced server-side doesn't create that risk. Retaining the three hard prohibitions is what makes it safe to state:

  • no --admin (the only one that actually bypasses protection)
  • no submitting an approving review (an agent must never manufacture the approval it then acts on — this is the load-bearing one)
  • no non-interactive batch mutation (preserves per-item intent)

--match-head-commit <sha> with the SHA read live is the best detail in here. Converting "the branch moved under us" from a race into a server-side refusal is exactly right, and it's the kind of thing that's easy to omit and impossible to notice until it bites.

Also correct: --body-file rather than --body with prose through a shell, and "a still-open issue with no remaining open PR is a finding to report, not something to silently fix." The latter keeps the agent from quietly papering over a state the human should see.


One gap I'd like addressed in the text: --auto is a deferred action, and the amendment's justification doesn't cover it.

The new human-gates paragraph rests on "the maintainer's explicit per-item keypress." But step (2) arms gh pr merge <S> --squash --auto, which lands the merge at some later point once checks pass — possibly minutes or hours after the keypress, unattended. That's still legitimate, but it isn't the same thing as the human pressing a key and the merge happening; it's the human pre-authorizing a conditional future merge.

--match-head-commit covers the obvious drift (a new push to the head), but not everything that can change between authorization and landing — the base branch advancing, a required check being reconfigured, or the superseded PRs in the cluster being reopened. Worth saying explicitly in human-gates.md that deferred/auto-merge is in scope for this carve-out and why the head-SHA pin is considered sufficient. Right now a reader could reasonably conclude auto-merge falls under "non-interactive batch mutation" and is forbidden, which contradicts the procedure in pr-review/SKILL.md. Since these two files are meant to be read together, that ambiguity is worth closing.

Minor: the ordering in the cluster procedure — arm survivor (2), then close superseded (3–4) — is the safe direction, since a failure mid-sequence leaves duplicates open rather than everything closed with nothing landing. Worth stating that rationale inline; "halting on the first failure" says what but not why this order, and the next editor might reasonably swap them to "clean up first."

Housekeeping: currently DIRTY against main — needs a rebase. Since index.json / index.md are generated by scripts/generate_skill_index.py, regenerate rather than hand-resolve those two if the conflict lands there.

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