Skip to content

Reviewing: hand a reviewed PR to a person to merge (agent:human-merge, human_merge_paths) - #43

Merged
glennfaison merged 1 commit into
mainfrom
feat/human-merge-outcome
Oct 26, 2026
Merged

glennfaison merged 1 commit into
mainfrom
feat/human-merge-outcome

Conversation

@glennfaison

Copy link
Copy Markdown
Owner

Problem

Reviewing had two exits, merge or send back. Two cases fit neither.

  1. A PR that runs out of max_review_rounds went to agent:stuck, which means an agent gave up and something is broken. Usually the PR is reviewable and a person just needs to decide. It also named at most one human_reviewer.
  2. Some changes should never be merged unattended however well they review, such as the merge gate, trust rules, scripts and release workflow in a repo where agents improve their own instructions. CI cannot tell whether a prose change alters agent behavior.

Changes

  • New outcome, handed to a person (references/human-merge.md, ADR 0008). Label agent:human-merge, with a comment that @mentions the owners and says why an agent is not merging, what was checked at which SHA, what is still open, and what to do. It never calls a PR safe beyond what merge-safety and code-review reported.
  • Trigger 1, review-rounds: replaces the escalation when rounds run out.
  • Trigger 2, protected-paths: new input human_merge_paths (gitignore-style, comma-separated, empty by default). The agents still review, fix and re-review. When the PR would otherwise merge, a person merges it instead.
  • Owners: codeowners_for N reads CODEOWNERS from the PR's base branch, never its head. Owners of the changed files are mentioned and requested for review, together with human_reviewer. With no CODEOWNERS file it falls back to human_reviewer and still labels and comments.
  • Fails closed: protected_files N returns a non-zero status when the changed files cannot be read or are empty, or a pattern is unsupported, and Reviewing then fails the item instead of merging. Renames count under their old name, so a file cannot leave a protected path by being renamed.
  • Ignored by the agents: excluded in shepherd-github-pr(s) and review-github-pr(s) like agent:stuck, and holds no merge lane.
  • A person's decision restarts the counts: the agent:human-merge marker resets review_rounds, shepherd_attempts and failures (in helpers.mjs and lib.sh), so a PR handed back with agent:needs-work is not escalated again on its first review.
  • Small gap fixed: Triaging's housekeeping closed an issue whose PR was merged but left agent:pr-open. That mattered little while only Reviewing merged, and matters now that people do. It now removes the label.
  • Label added to the setup block in labels.md, the setup skill asks for human_merge_paths, protocol tables updated, prompts regenerated with npm run sync, minor changeset.

Verification

  • npm run validate: 17 skills OK. npm test: shell suite 144 passed / 0 failed, JS suite 37 passed / 0 failed. check-plugin-version and sync --check clean.
  • New tests: the path matcher (anchoring, **, trailing /, wildcard last segment, literal regex characters, rejected patterns), protectedFiles refusing an unreadable pattern, CODEOWNERS (last rule wins, ownerless rule, comments, order, no repeats), both CLI commands, and shell tests against the gh stub for protected_files (empty setting, match, rename, unreadable list, empty list, unsupported pattern) and codeowners_for (base-branch lookup, .github/ over docs/, no file). Shared marker fixtures cover the count reset and a forged marker.

Not verified, and limits

  • The skill text itself (Reviewing steps 6 and 8, human-merge.md) has not been run end to end by a real runner, and gh pr edit --add-reviewer with teams and users without access was not exercised.
  • The path matcher is a subset of gitignore and CODEOWNERS (documented in helpers.mjs and the ADR), not full parity.
  • This is a policy gate, not a security boundary. A token that can merge can still bypass the skills. To enforce it, protect the base branch with a ruleset that requires a code-owner review and does not let the agent's account bypass it.

Upgrading

Run the label setup in references/labels.md again to create agent:human-merge. human_merge_paths is empty by default, so nothing changes until a runner sets it.

…, human_merge_paths)

A PR that runs out of review rounds, or that passes review but changes a file in human_merge_paths, is labelled agent:human-merge and its CODEOWNERS (read from the base branch) are asked to review and merge it. See ADR 0008.
@glennfaison
glennfaison merged commit e52cbd9 into main Oct 26, 2026
2 checks passed
@glennfaison
glennfaison deleted the feat/human-merge-outcome branch October 26, 2026 01:31
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